diff --git a/docs/reviews/SPEC-REVIEW-376-r1.md b/docs/reviews/SPEC-REVIEW-376-r1.md new file mode 100644 index 00000000..11b982e6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-376-r1.md @@ -0,0 +1,253 @@ +# SPEC-REVIEW-376-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/376 — «Пачка Low из + аудита beta.4: title:null в space-card, roomlabel в Background, персист + цвета декора, iso-штрихи мебели, стейл-док, truthy light_pools (а–е)» +- Этап: ТЗ на ревью (PROCESS.md §2.4), лёгкий трек (метка `small`) +- ТЗ: тело issue, раздел «# ТЗ (лёгкий трек, редакция 1)», зафиксировано + `updatedAt: 2026-08-29T17:34:47Z` +- Материал ревью: тело issue #376 (аналитика + ТЗ, один автор, без + дальнейших комментариев на момент ревью) сверено с `dev` на `4eede5bc` +- Заход: r1 · блокирующих циклов израсходовано **1 из 2** (лёгкий трек) + +## Скоуп ревью + +Первый заход — предыдущего вердикта нет, разбор полный. Проверялось: +соответствие `docs/SCOPE.md`; корректность выбора трека `small` по §5 +PROCESS.md (все критерии обязаны выполняться одновременно); обязательный +минимальный шаблон лёгкого трека («проблема · контракт · AC1…ACn с +доказательством · откат»); однозначность и доказуемость каждого AC; +отсутствие догадок, выданных за факт; сверка каждого фактического +утверждения ТЗ о текущем коде с исходниками. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (жизненный + цикл §2, лимит циклов §4, критерии трека §5, артефакты §7, гейты §8). +2. Прочитано тело issue #376 полностью (аналитика §2.2 + ТЗ §1–§6), других + комментариев на issue нет. +3. Прочитан `docs/CONFIG-COMPATIBILITY.md` целиком — сверить, что такое + «новое compatibility-поле» в терминах именно этого проекта, и подобрать + аналог для пункта (в). +4. Найден и прочитан прямой прецедент: issue **#369** («Аудит после + v1.69.0-beta.3 (V5) — семь Low-находок») — структурно идентичный кейс: + пачка несвязанных Low-находок из точно такого же adversarial-аудита. + `SPEC-REVIEW-369-r1.md` и `-r2.md` зафиксировали: «Этап: ТЗ на ревью, + **полный трек** (владелец назвал критерий «одна поверхность» §5 как не + выполненный — **семь несвязанных поверхностей**)» и результатом стал файл + `docs/specs/369-audit-lows.md`, а не ТЗ в теле issue. +5. Найден и прочитан второй релевантный прецедент: `SPEC-REVIEW-372-r1.md` + (родитель пункта (а) этого issue) — там тот же самый компакт-контракт + `title` тоже потребовал **полного** трека («аналитика назвала критерий + `small`, который задача не проходит — новый UX-контракт для `title`»). +6. Каждое фактическое утверждение ТЗ о текущем коде сверено с `dev`, не + принято на слово: + - `src/space-card.ts:811-813` — `title = this._config.title !== undefined + ? this._config.title : sp?.title || ''`; `:821` + `compactTopFrame: this._config.title === ''`; `:870` + `${title ? html\`
…` — подтверждён факт «`null` + уже скрывает заголовок (falsy), но кадр остаётся симметричным» и что + `undefined` (отсутствие ключа) не задета правкой. + - `src/space-card.ts:290-291` — `if (!this._config.light_pools) + disposeGlowRuntime(...)` (truthy-гейт) против рендер-гейта `:842` + `lightPools: this._config.light_pools === true` — подтверждён факт: + `light_pools: 1` не dispose'ит рантайм (`!1` ложно) и переходит в ветку + `_resolveGlowBlend()` (:291), при этом пулы не рендерятся (`1 !== true`). + Расхождение реально существует и воспроизводится по описанному пути. + - `src/styles/plan.styles.ts:863-865` — `.stage.mode-decor .devlayer, + .stage.mode-decor .devlayer *, .stage.mode-decor .dev::before { + pointer-events: none; }`; строки 617-619 несут независимое правило + `.stage.markup .roomlabel { pointer-events: auto; }` для **другого** + режима (`markup`, не `mode-decor`). `.roomlabel` рендерится внутри + `.devlayer` (`src/houseplan-card.ts:11064` открывает `.devlayer`, + `:11962` — `class="roomlabel …"` рендерится в этом поддереве) — заявление + «`.devlayer *` перебивает предыдущий auto для roomlabel в Background» + подтверждено: для `mode-decor` специфичность конкурирующих правил равна + (0,3,0), и позже объявленное правило (:863) побеждает по порядку + источника. Не код-правка — доку добавлять корректно, код не трогается. + - `custom_components/houseplan/validation.py:1883` — `vol.Optional( + "settings", default=dict): vol.Schema({...}, extra=vol.ALLOW_EXTRA)` — + номер строки и факт «опциональный settings-объект с ALLOW_EXTRA» + подтверждены; это снижает риск нового ключа (старый бэкенд/фронт не + ломается на неизвестном поле), но само по себе не влияет на + классификацию трека — см. находку H1. + - `src/houseplan-card.ts:984` — `private _decorStyle: DecorStyle = { + ...DEFAULT_DECOR_STYLE }` — подтверждено, поле только in-memory, + персиста нет. + - `src/furniture.ts:392-406` (`furniturePlanScreenScale`) и + `src/houseplan-card.ts:8084` (вызов) — подтверждена формула + `Math.min(viewportW/viewW, viewportH/viewH)`; `src/houseplan-card.ts:10791` + (`class=${isoLayers?.structural ? 'iso-floor-scene' : nothing}`) и `:10830` + (`preserveAspectRatio="none"`) подтверждают наличие отдельной + iso-трансформации, под которую эта формула не рассчитана. + - `docs/TESTING.md:1704` — строка «static room cards show the same + data/base projection but no live pools» подтверждена дословно, без + оговорки про `light_pools`/#374. +7. Проверено отсутствие незакрытых меток неопределённости в тексте + («уточнить», TBD, голый вопросительный знак вне markdown-разметки) — не + найдено. + +## Находки + +### H1 (High, блокирует) — трек `small` не проходит собственные критерии §5 одновременно; прямой прецедент #369 уже разрешил тот же вопрос иначе + +**Файл:** тело issue #376 (раздел «8. Трек» аналитики + весь раздел «ТЗ»). + +Критерии §5 PROCESS.md обязаны выполняться **все одновременно**; ни один не +допускает исключения по «сумме несложных правок». Этот пакет нарушает как +минимум два: + +1. **«Одна поверхность (один диалог, один модуль, один эндпоинт)».** Сама + аналитика перечисляет поверхности: `space-card.ts` (а, е), доки (б, д), + `houseplan-card.ts` + `houseplan-editor-runtime.ts` + `validation.py` (в), + отдельная iso-ветка `houseplan-card.ts` (г). Это минимум четыре разных + модуля продуктового кода плюс документация — не «один модуль» ни по + какому прочтению. +2. **«Нет миграции конфига и новых compatibility-полей».** Пункт (в) вводит + новый персистентный ключ `settings.decor_default_style` — новую серверную + схему (`validation.py`), новый путь чтения при инициализации и новый путь + записи с `expected_rev`. Это ровно тот класс изменения, который + `docs/CONFIG-COMPATIBILITY.md` каталогизирует как compatibility-поле + (пусть и низкого риска благодаря `ALLOW_EXTRA` — см. «Как проверялось» + п.6), а не разрешённое исключение для лёгкого трека. + +Это не абстрактное толкование: у проекта есть **прямой прецедент с той же +формой задачи**. Issue #369 — пачка из семи Low-находок ровно того же +adversarial-аудита — была явно переведена на полный трек с формулировкой +владельца «семь несвязанных поверхностей» (`SPEC-REVIEW-369-r1.md`, +`SPEC-REVIEW-369-r2.md`), и результат лёг в `docs/specs/369-audit-lows.md`, +а не в тело issue. #376 повторяет ту же форму (пачка Low из аудита, +затрагивающая несвязанные модули) без единого слова о том, почему в этот +раз критерий «одна поверхность» вдруг выполнен. Второй, более узкий +прецедент — issue #372 (прямой родитель пункта (а) этого issue): та же +самая пара «header/compact frame» для `title` уже один раз потребовала +полного трека («новый UX-контракт для title»), хотя по объёму правки была +проще, чем нынешний пакет из шести пунктов. + +PROCESS.md §5, последний абзац: «Если по ходу выясняется, что критерий +нарушен (появилась миграция, задело второй модуль) — метка `small` +снимается, issue возвращается в `S3-spec` и получает нормальный файл ТЗ. +Это не провал, это ранняя диагностика.» Это именно тот момент. + +**Почему High, а не Medium.** Ошибка классификации меняет саму форму +процесса вокруг задачи: бюджет ревью (2 цикла вместо 4), обязательность +файла `docs/specs/376-*.md`, необходимость завести отдельный полный ТЗ хотя +бы для (в). Это нельзя «починить тем же комментарием» без реструктуризации +задачи — по определению не Medium-в-скоупе. + +**Продуктовые решения владельца не оспариваются.** Оба решения 29.08 +(`title: null ≡ ''`; «цвет декора персистится в серверный конфиг») остаются +в силе — это ответы на продуктовые вопросы, они не про трек. Находка +касается исключительно процессной классификации, которая по PROCESS.md +решается аналитиком/ревьюером техническим порядком, не владельцем. + +**Рекомендация (одна из двух, автор выбирает):** +- Выделить (в) в отдельный issue на полном треке (серверная схема + + кросс-модульный путь записи — ровно профиль, для которого полный трек + существует), оставить (а, б, г, д, е) на `small` — это уже гораздо ближе к + «одному модулю» каждый по отдельности и без нового персистентного поля; + либо +- Снять `small` со всего #376 по образцу #369, написать + `docs/specs/376-*.md` с полными разделами §7.1. + +### M1 (Medium, в скоупе) — раздел «откат» отсутствует полностью + +**Файл:** тело issue #376, весь раздел «# ТЗ». + +Минимальный обязательный шаблон лёгкого трека (PROCESS.md §5): «проблема · +контракт · AC1…ACn с доказательством · **откат**». В тексте нет ни одного +предложения о том, как откатить любую из шести правок. Особенно заметно для +(в): текст не говорит, что происходит с уже записанным +`settings.decor_default_style` при откате коммита/фронтенда — хотя ответ +уже виден из кода (`extra=vol.ALLOW_EXTRA`, «Как проверялось» п.6: старый +бэкенд/фронт читает конфиг с незнакомым ключом без ошибки), в ТЗ это нигде +не зафиксировано как часть контракта. + +**Чем чинится в этой же задаче:** один абзац — «откат: правки (а, б, г, д, +е) — обычный `git revert`, поведения не переживают перезапуск процесса. +(в): неизвестный ключ `decor_default_style` безопасно игнорируется старым +бэкендом/фронтом (`ALLOW_EXTRA`); откат кода не требует отдельной обратной +миграции, задокументированный дефолт `DEFAULT_DECOR_STYLE` продолжает +действовать.» + +### M2 (Medium, в скоупе) — пункты (б) и (д) заявлены в скоупе, но не имеют собственного AC + +**Файл:** тело issue #376, раздел «## 2. AC» против раздела «## 1. Объём». + +Раздел «Объём» явно называет (б) (предложение в USER-GUIDE(.ru) про +Background-редактор) и (д) (оговорка в `docs/TESTING.md:1704`) как часть +поставки. Раздел «AC» перечисляет AC-а, AC-в1…в3, AC-г, AC-е, AC-общ — ни +одного пункта, который проверял бы именно текст (б) или (д). Без +собственного AC код-ревьюеру нечем формально подтвердить, что эти два +пункта скоупа выполнены так, как задумано (а не забыты или не +сформулированы иначе). + +**Чем чинится в этой же задаче:** два дополнительных пункта, например +«AC-б: `docs/USER-GUIDE.ru.md`, раздел Редактора подложки, содержит +предложение о неинтерактивности лейблов комнат в этом режиме — доказательство: +ревью текста» и «AC-д: `docs/TESTING.md:1704` содержит оговорку про +`light_pools`/#374 — доказательство: ревью текста». + +## Что проверено и корректно + +- Задача в скоупе `docs/SCOPE.md` — все шесть пунктов чинят существующее + поведение уже принятых фич (#372, #362, #360, #361, #374), новых + пользовательских job не открывают и не входят в «Out of scope». +- Продуктовых вопросов владельцу не осталось: оба места, где решение было + нужно ((а) `null`≡`''`; (в) персист или нет), уже закрыты записанным + решением владельца от 29.08 — текст ТЗ явно на них ссылается, а не + переизобретает. +- Ни одно техническое утверждение ТЗ о текущем поведении кода не + оказалось догадкой, выданной за факт — все семь проверенных пунктов (см. + «Как проверялось» п.6) подтвердились дословно по номерам строк. +- Каждый отдельный AC (AC-а, AC-в1…в3, AC-г, AC-е, AC-общ) сформулирован + как проверяемое утверждение с названным способом доказательства + (`unit`/`смок`) и явной регресс-веткой там, где правка могла задеть + соседнее поведение (AC-а: `undefined` и `' '` остаются нетронутыми; + AC-г: существующий юнит #361 для 2D не должен ослабнуть). +- Раздел «Принятые предположения» корректно выносит технические, не + наблюдаемые пользователем решения (формат ключа snake/camelCase, путь + записи только из runtime-редактора) в явный блок вместо того, чтобы + спрятать их в тексте контракта как решённые факты. +- i18n корректно отмечен как незадетый — ни один из шести пунктов не + добавляет пользовательский текст в UI (только документацию). +- Риски (в), (а) названы конкретно вместе со снимающим их механизмом + (дебаунс + `expected_rev`/#340; регресс-ветка AC-а), а не общими словами. + +## Чего не проверял + +- Гейты код-ревью (`typecheck`/`test`/`build`/смоки/`check-docs`/ + инварианты модели/`pytest tests_backend`) — не запускал: на стадии + ТЗ продуктовый код не менялся, диффа против `dev` в этом issue нет, + гейтам нечего было бы доказать. Ссылка на зелёный Validate `4eede5bc` + (https://github.com/Matysh/houseplan-card/actions/runs/33252620807) + относится к текущему `dev`, не к материалу этого ТЗ. +- Точную будущую формулировку абзацев USER-GUIDE(.ru)/TESTING.md для (б, + д) — контракт называет смысл и место, не финальный текст; это в пределах + «принято предположительно», не находка (при условии, что M2 закрыт + добавлением AC на сам факт правки). +- Реализуемость дебаунса «≥1 с» для записи (в) на практике — оценка риска + по описанию, кода ещё нет. +- Согласованность нового бэкенд-ключа `decor_default_style` с полным + реестром `docs/CONFIG-COMPATIBILITY.md`/`scripts/config-field-registry.mjs` + за пределами вопроса классификации трека (H1) — это станет предметом + код-ревью, если/когда (в) пойдёт в разработку. + +## Вердикт + +Технически пакет описан аккуратно: каждое утверждение о текущем коде +проверено построчно и подтвердилось, AC по большинству пунктов проверяемы +и снабжены регресс-ветками, продуктовые решения владельца зафиксированы, а +не додуманы. Но сама форма задачи — пачка из шести точечных находок +adversarial-аудита поперёк как минимум четырёх модулей, одна из которых +(в) добавляет новый персистентный серверный ключ с кросс-модульным путём +записи, — не проходит собственные критерии лёгкого трека PROCESS.md §5 +одновременно («одна поверхность», «нет новых compatibility-полей»), причём +для задачи ровно такой же формы («пачка Low одного аудита») этот же +проект уже принимал обратное решение на issue #369. Это блокирующая +находка: она не про содержание AC, а про то, в каком процессе задачу +вообще можно продолжать. Два Medium (отсутствующий раздел «откат», +отсутствующие AC на пункты (б)/(д)) чинятся в этой же задаче без +отдельного issue, независимо от исхода H1. + +`Вердикт: красный · заход r1 · блокирующих циклов 1/2 · High: 1 · Medium: 2 → в задаче`