diff --git a/docs/reviews/SPEC-REVIEW-230-r1.md b/docs/reviews/SPEC-REVIEW-230-r1.md new file mode 100644 index 00000000..36bd88ea --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-230-r1.md @@ -0,0 +1,271 @@ +# SPEC-REVIEW-230-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/230 +- **ТЗ:** `docs/specs/230-hatch-density-normalization.md` +- **Ветка / SHA:** `issue/230-hatch-density-normalization` @ `2396044` +- **Этап:** spec (PROCESS.md §2.4) · трек: `small` (метка на issue) +- **Заход:** r1 · блокирующих циклов израсходовано 0/4 до этого вердикта +- **Ревьюер:** Claude, роль «ревьюер ТЗ» + +## Скоуп + +Оценивалось ТЗ #230 — нормализация плотности штриховки тела стены к +физическим сантиметрам плана и удаление зумовой компенсации паттерна. Код не +менялся и не оценивался (реализации нет, этап S4). Проверялись: соответствие +`docs/SCOPE.md`, фактическая точность ссылок на код и на golden-фикстуры, +однозначность и доказуемость AC1–AC10, согласованность Scope/Contract/AC между +собой. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` целиком (первый цикл, правило «по + дельте» §2.10 не применяется). +2. Прочитано тело issue #230 и единственный комментарий («ТЗ готово, на + ревью»); в истории issue нет отдельного `S2-analysis`-комментария с + шаблонной «Оценкой» (§7.2) — трек `small` и метки `bug`/`polish` видны + только как текущее состояние. +3. Прочитан файл ТЗ целиком. +4. Построчно проверены все фактические ссылки на код: + - `src/wall-thickness.ts:40-73` (`wallBodyNeedsSolid`, `WALL_HATCH_MIN_PX = 3`, + `clampWallCm`, `wallCmToUnits`) — совпадает с ТЗ; + - `src/houseplan-card.ts:6163-6166` (`_cellCm`, дефолт 5 при нечисловом/≤0 + значении) — совпадает; + - `src/houseplan-card.ts:11079-11093` (`_wallHatchDefs`, `const inv = ...`, + `patternTransform="rotate(45) scale(${inv})"`) — совпадает буквально; + - `src/space-render.ts:509-513` (тот же `` без `scale`, только + `rotate(45)`) — подтверждает заявленное расхождение рендереров уже + сегодня; + - `src/space-geometry.ts:11,197,199` (`NORM_W=1000`, `GRID_N=240`, + `GRID_PITCH=NORM_W/GRID_N≈4.1667`) — подтверждает таблицу в §3 ТЗ; + - `src/houseplan-card.ts:285-286`, `src/plan-optimizer.ts:32-33` + (`CELL_CM_MIN=0.1`, `CELL_CM_MAX=1000`) — подтверждает диапазон `cell_cm` + из §8.3. +5. Алгебраически проверены формулы AC1–AC6: `wallHatchStepUnits(cellCm) = + 8·(5/cellCm)`; отношение `wallCmToUnits(cm,cell,GP)/wallHatchStepUnits(cell) + = cm·GP/40` не зависит от `cell`, то есть AC2 доказывает именно заявленную + инвариантность, а для `cm=30` против `cm=15` отношение ровно вдвое (AC3). + Клампы `[0.5, 80]` алгебраически покрывают весь диапазон `cell_cm` + `0.1…1000` без разрывов (AC5). +6. Проверена схема вызова `wallBodyNeedsSolid` в обоих рендерерах + (`houseplan-card.ts:11312`, `space-render.ts:473`) — точка интеграции для + нового `wallHatchNeedsSolid` (§8.4) существует и однозначна. +7. **Проверены golden-фикстуры, а не только текст ТЗ о них**: + `demo/fixtures/large-house.mjs:209-218` (`perf-floor-1`, `cell_cm: 5`, + стены 15 см, `show_borders: true`) и `demo/golden/matrix.mjs:328-331` + (сценарии `large-house-zoom-040-dark` — `zoom: 0.4`, + `large-house-zoom-250-dark` — `zoom: 2.5`, оба на `fixture: 'large'`, + `space: 'perf-floor-1'`) — см. High-1. +8. Проверено, что `src/space-card.ts` не является третьим независимым + потребителем паттерна: он рендерит через `renderSpaceStatic` из + `space-render.ts` (уже в скоупе §6), CSS-класс `.wallbody{fill:url(#hp-wall-hatch)}` + лишь ссылается на тот же ``, который создаёт вызванный рендерер — + не находка. +9. Проверено `docs/WALL-THICKNESS.md` §3 (Body render) — текущая редакция не + описывает ни зависимость шага паттерна от `cell_cm`, ни зумовую + компенсацию, то есть менять там формально нечего, но раздел про решение + «убрать зумовую компенсацию» отсутствует — см. Low-1. +10. Проверено `docs/USER-GUIDE.ru.md:442` — существующая строка про штриховку + («на малом масштабе штриховка скрывается, тело остаётся») не противоречит + планируемой правке §15. +11. Код не запускался, гейты не гонялись — реализации нет, это ожидаемо на + этапе ревью ТЗ. + +## Находки + +### High-1 — риск-анализ и AC10 построены на неверной посылке о golden-фикстурах + +**Файл:** `docs/specs/230-hatch-density-normalization.md`, §11 «Риски», п.2 +(Golden) и §12, AC10. + +**Формулировка ТЗ:** «Все golden-фикстуры используют `cell_cm: 5` +(`demo/fixtures/*.mjs`, `demo/golden/harness.mjs`) и не меняют зум, поэтому +эталоны не должны измениться ни в одном пикселе. Это проверяемое утверждение: +`golden:verify` обязан пройти без переснятия.» + +**Проблема.** Первая половина утверждения верна: оба файла фикстур +(`demo/fixtures/large-house.mjs`, `demo/fixtures/visual-matrix.mjs`) задают +`cell_cm: 5` для каждого пространства. Вторая половина — «не меняют зум» — +фактически неверна: `demo/golden/matrix.mjs:328-331` определяет ровно два +сценария на фикстуре `large` (пространство `perf-floor-1`, стены 15 см, +`show_borders: true`, то есть тело стены рисуется): + +```js +{ id: 'large-house-zoom-040-dark', fixture: 'large', space: 'perf-floor-1', + mode: 'view', zoom: 0.4, theme: 'dark', viewport: { width: 1180, height: 900 }, ...stage }, +{ id: 'large-house-zoom-250-dark', fixture: 'large', space: 'perf-floor-1', + mode: 'view', zoom: 2.5, theme: 'dark', viewport: { width: 1180, height: 900 }, ...stage }, +``` + +`demo/golden/harness.mjs:440-441` применяет `scenario.zoom` через +`card._applyView(scenario.zoom, 500, 500)` — то есть зум реально +устанавливается перед снимком, а не игнорируется. + +Это не мелкая деталь, а прямое противоречие продуктовому решению владельца из +того же ТЗ (§4, п.2): «Зумовая компенсация убирается... Число полос внутри +стены одинаково на любом зуме». Сегодня `inv = max(0.4, 1/max(zoom,0.4))` при +`zoom=0.4` даёт `inv=2.5`, при `zoom=2.5` даёт `inv=0.4` — то есть паттерн +`hp-wall-hatch` в этих двух golden-сценах **сегодня отрисован с ненулевым +масштабом**, а после правки `patternTransform` вообще не содержит `scale` +(§8.2 ТЗ, дословно: «Множителя `1/zoom` больше нет ни в одном рендерере»). Это +ровно та точка, где линии паттерна на экране видимо переместятся — если тело +стены на этих экранах не провалилось в `solid` (порог `WALL_HATCH_MIN_PX=3`), +разница будет пикселем, особенно на `zoom: 2.5` (зум **внутрь**, то есть +больше пикселей на юнит, а не меньше — вероятность попасть в `solid`-режим +там ниже, чем на `zoom: 0.4`). + +**Почему это находка, а не техническая деталь.** Владелец явно одобрил именно +это изменение видимого поведения при зуме (§4, п.2) — то есть расхождение +этих двух golden-сцен, если оно произойдёт, **не является** «правка задела +больше заявленного» из формулировки риска в самом ТЗ; это прямое и ожидаемое +следствие продуктового решения, зафиксированного в том же документе. ТЗ +противоречит самому себе: §4 обещает видимое изменение при любом зуме ≠ +эталонного поведения на экране, а §11/AC10 обещают, что *ни одна* golden-сцена +не изменится ни на пиксель. Оба обещания не могут быть верны одновременно для +сцен, снятых при `zoom ≠ 1`. + +**Почему это блокирует, а не варьируется по ходу реализации.** AC10 — +единственная строка ТЗ, которая должна поймать регрессию из риска №3 +(расхождение карты и статического рендерера) и защитить от случайного +охвата больше заявленного. Если формулировка AC10 неверна, разработчик либо +(а) увидит красный `golden:verify` там, где всё сделано правильно, и не будет +знать, чинить код или переснимать эталон — ровно то самое решение, которое +процесс явно запрещает принимать в одиночку («переснятие ради зелёного CI», +§12 PROCESS.md), либо (б) продавит переснятие двух сцен без разбора, чтобы +пройти AC10, что легитимно только через `golden:accept -- --reviewed`, но ТЗ +не называет этот путь и не готовит к нему код-ревью. + +**Что нужно.** Явно разобрать оба сценария `large-house-zoom-*-dark` в §11/§12: +1. посчитать/оговорить, действительно ли тело стены `perf-floor-1` (15 см, + `cell_cm: 5`) остаётся в режиме `hatch` (не `solid`) на этих двух зумах при + их `viewport`; и +2. если остаётся — заранее объявить, что эти две сцены **ожидаемо** меняются + (это следствие §4 п.2, не побочный эффект) и должны быть переснятые через + `golden:accept -- --reviewed` с отдельной пометкой в хендоффе, ЛИБО сузить + AC10 формулировкой вида «`golden:verify` проходит без переснятия для всех + сцен, кроме `large-house-zoom-040-dark`/`large-house-zoom-250-dark`, которые + меняются предсказуемо и переснимаются по правилу §12 PROCESS.md». + +### Medium-1 — контракт «оба рендерера» не имеет собственного доказательства для статического рендерера + +**Файл:** `docs/specs/230-hatch-density-normalization.md`, §6, §8.2, AC7, AC8, +§13. + +**Проблема.** §6 (Scope) и §8.2 (Contract) требуют, чтобы **оба** рендерера — +`houseplan-card.ts` (интерактивная карта) и `space-render.ts` (статический +рендерер `Room View card` / киоск) — строили `` через +`wallHatchStepUnits(cellCm)`. Но AC7 и AC8 явно говорят «в разметке карты» / +«на реальной карте», и запланированный смок (§13, `demo/smoke_wall_hatch_density.mjs`) +описан как проверка «на реальной карте при `cell_cm` 5 и 25» — то есть тоже +только интерактивный рендерер. Ни один AC не проверяет разметку паттерна +`space-render.ts` при `cell_cm ≠ 5`. + +AC10 (`golden:verify`) не закрывает этот пробел: как показано в High-1, все +golden-сцены используют `cell_cm: 5`, а при `cell_cm === 5` +`wallHatchStepUnits(5) === 8` — то есть числовое совпадение со старой +константой `width="8" height="8"`. Если разработчик поправит только +`houseplan-card.ts` и оставит в `space-render.ts:509` старый жёстко заданный +`width="8" height="8"`, **все** AC1–AC10 в текущей формулировке останутся +зелёными, а требование §6/§8.2 «тот же паттерн через ту же функцию» останется +невыполненным и непроверенным ни одним автотестом — регрессия того самого +расхождения рендереров, которое ТЗ называет риском №3 и обещает зафиксировать +тестом. + +**Что нужно.** Одна из двух правок: расширить формулировку AC7/AC8 (или +добавить AC7b) так, чтобы она явно требовала проверки разметки паттерна +`space-render.ts` при `cell_cm ≠ 5` — например, через прямой вызов +`renderSpaceStatic`/`buildSpaceDevices` с фикстурой `cell_cm: 25` и разбор +получившейся строки ``, не обязательно в браузере. Это устраняет +пробел без изменения продуктового контракта. + +### Low-1 — канонический документ подсистемы не входит в release-артефакты + +**Файл:** `docs/specs/230-hatch-density-normalization.md`, §15. + +`docs/WALL-THICKNESS.md` §3 («Body render») — канонический документ ровно той +подсистемы, которую меняет эта задача (штриховка тела стены), и в этом же +документе уже принято фиксировать точечные решения такого рода инлайн-ссылками +на issue (`#197`, `#198`, `#201` в тексте §3/§8). §15 ТЗ называет только оба +CHANGELOG и строку в `docs/USER-GUIDE.ru.md`, не упоминая обновление +`WALL-THICKNESS.md` фразой про новую формулу шага и про то, что зумовая +компенсация паттерна больше не существует ни в одном рендерере. Не блокирует +и не влияет на проверяемость AC — снимаю с рекомендацией добавить одну фразу +в §3 канона в том же коммите, что и код (по аналогии с уже принятой в +документе практикой). + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют по существу: сценарий и персона + (§1), видимое изменение до/после (§2), причина (§3), продуктовые решения + владельца (§4), цели (§5), scope/не-scope (§6–7), контракт (§8), данные/i18n + (§9), performance (§10), риски (§11), AC1–AC10 (§12), план тестов (§13), + мутационный гейт (§14), release-артефакты (§15), откат (§16), явный блок + предположений (§17). +- Продуктовая рамка (§1–2) отвечает на оба обязательных вопроса — персона + («ведёт планы двух домов» из `docs/SCOPE.md` J4/J6), поверхность (десктопный + просмотр плана), объём видимого изменения одной фразой без терминов + реализации. +- **Главный технический вывод ТЗ верен и лучше исходного issue.** Формула из + тела issue (`cell_cm/5`) действительно усугубляет разброс, а не устраняет + его — проверено алгебраическим пересчётом (см. «Как проверялось», п.5) тем + же способом, каким ТЗ его приводит. Обратный множитель `5/cellCm` — + корректное решение поставленной владельцем задачи «привязать плотность к + сантиметрам плана». +- AC1, AC4, AC5, AC6, AC9 однозначны, проверяемы и указывают способ + доказательства (`unit`); тривиально вычисляются вручную и дают ожидаемый + автором результат — не оставляют пространства для интерпретации при + написании теста. +- AC2/AC3 математически доказывают именно то, что заявлено целью (§5): + независимость числа полос от `cell_cm` и пропорциональность толщине стены, + а не декларируют это без доказательства. +- Обращение к геттеру `_cellCm` (нечисловой/нулевой/отрицательный → 5) в §8.1 + зафиксировано с точной ссылкой на существующий прецедент + (`houseplan-card.ts:6163`) — не самостоятельно изобретённое правило. +- §7 («Не входит в задачу») корректно исключает угол штриховки, порог + `WALL_HATCH_MIN_PX`, изометрию (`_renderProjection === 'iso'` — подтверждено + чтением `houseplan-card.ts:11303`) и штриховку прочих сущностей — все четыре + исключения проверены по коду, а не заявлены на веру. +- `Touch editor: not exposed` — корректное значение: задача не добавляет ни + одного взаимодействия, только характер отрисовки уже существующего слоя. +- Мутационный гейт (§14) целится в реальные, а не декоративные ошибки: + `hatch-step-inverted` прямо воспроизводит ошибку из формулировки issue, + `hatch-zoom-compensation-back` защищает именно от регрессии, названной в + риске №3. +- Откат (§16) реалистичен: чистая функция, конфиг не меняется, revert + безопасен. + +## Продуктовые вопросы владельцу + +Не требуются. Единственная точка, где решение владельца уже расходится с +текстом того же ТЗ (High-1), решается не новым продуктовым вопросом, а +уточнением формулировки риска/AC — владелец уже высказался по существу (§4, +п.2), автору нужно привести §11/AC10 в соответствие с этим решением, а не +получить новое. + +## Чего не проверял + +- Реализацию — её не существует на этом этапе. +- Действительно ли `large-house-zoom-040-dark` / `large-house-zoom-250-dark` + сегодня попадают в `solid`-режим по `WALL_HATCH_MIN_PX=3` или показывают + видимую штриховку — для этого нужно посчитать `pxPerUnit` через реальный + `viewport`/`_baseVb`/`_applyView` этой фикстуры или прогнать + `golden:verify` по факту. Не требуется для вывода High-1: сам факт, что ТЗ + безусловно утверждает «зум не меняется» при наличии двух сцен с `zoom ≠ 1`, + уже делает риск-анализ и AC10 недостоверными независимо от точного + числового исхода. +- Соответствие метки `small` фактической сложности задачи (три модуля, + golden-риск, требующий отдельного расчёта) — не поднимаю как находку: + ТЗ существует как полноценный файл `docs/specs/230-*.md` независимо от + метки трека, содержание оценивается по существу, а не по тому, в какой + контейнер его положили. +- `demo/golden/harness.mjs` целиком и остальные ~40 golden-сцен построчно — + проверено выборочно (`grep` на `zoom:`/`cell_cm`), достаточно для вывода + High-1; полный построчный обзор оставляю код-ревью при реализации. + +## Вердикт + +Одна блокирующая находка (High-1): риск-анализ и AC10 построены на +проверяемо неверной посылке о golden-фикстурах, что делает главный +защитный AC задачи ненадёжным как написано. Medium-1 и Low-1 в скоупе задачи +и чинятся правкой текста ТЗ, без изменения продуктового решения владельца. +Итог — жёлтый: возврат автору на правку ТЗ, повторный цикл ревью по дельте +(PROCESS.md §2.10).