docs: review document for #230

Issue: #230
User-Visible: no

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` (тот же `<pattern>` без `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)}`
+   лишь ссылается на тот же `<pattern>`, который создаёт вызванный рендерер —
+   не находка.
+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` / киоск) — строили `<pattern>` через
+`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` и разбор
+получившейся строки `<pattern>`, не обязательно в браузере. Это устраняет
+пробел без изменения продуктового контракта.
+
+### 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).