diff --git a/docs/reviews/SPEC-REVIEW-250-r1.md b/docs/reviews/SPEC-REVIEW-250-r1.md new file mode 100644 index 00000000..7c836a1c --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-250-r1.md @@ -0,0 +1,161 @@ +# SPEC-REVIEW-250-r1 + +- Issue: [#250](https://github.com/Matysh/houseplan-card/issues/250) — «Проёмы с сохранённым flip_v остались на грани стены: символ обязан стоять на осевой всегда» +- Этап: ТЗ на ревью (PROCESS.md §2.4) +- Заход: r1 (первый), блокирующих циклов израсходовано **0 из 4** +- Ревьюер: Claude (роль «ревьюер ТЗ», отдельная от автора — Codex) +- Артефакт ТЗ: `docs/specs/250-opening-centerline.md` +- SHA ТЗ: `2c776b0c6d00b1276c04d0b51c696f405beddcbb` (ветка `issue/250-opening-centerline`, коммит «docs: specify opening centerline invariant») +- Трек: обычный (метки `bug`, `P2`, `polish`, `S4-spec-review`; `small` не выставлена — файл в `docs/specs/` обязателен, что и сделано) + +## Скоуп проверки + +Первый заход — разбор полный, раунд по дельте (§2.10) неприменим. + +Прочитано перед вердиктом: +- `docs/SCOPE.md` (Core user jobs, лок-инвариант, правило про удаление файлов — не задействовано здесь); +- `AGENTS.md`, `docs/PROCESS.md` целиком (§1–§14, включая §2.4, §4, §7.1, §7.2); +- тело issue #250 и оба комментария (аналитика владельца-эксперта и хендофф автора ТЗ на ревью); +- `docs/USER-GUIDE.ru.md` (раздел «Настройки проёма», «Толстые стены»); +- `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md` (модель `OpeningCfg`), `docs/ISOMETRIC.md` (структурный Iso-контракт); +- сам ТЗ `docs/specs/250-opening-centerline.md` целиком. + +## Как проверялось + +ТЗ утверждает конкретные факты о текущем коде и текущих документах — эти факты +проверялись построчно, а не принимались на слово (это и есть работа +ревьюера ТЗ: не согласиться, а найти невыполнимое или непроверяемое): + +1. **Диагноз §3** (`openingSymbolOffset()` даёт ненулевой offset для door/window + при `flip_v: true`, gate/passage уже нулевые) — сверен с + `src/opening-symbol-placement.ts:20-36`. Подтверждено буквально: строка 26 + `if (!flipV || type === 'gate' || type === 'passage') return { ox: 0, oy: 0 };` + и вычисление `half = Math.hypot(face.ox, face.oy)` дальше. Тест + `test/opening-symbol-placement.test.mjs` («saved door and window flips use + one canonical local edge») прямо ожидает `{ ox: 0, oy: 20 }` для + `positiveFace = { cm: 20, … }` — соответствует утверждению issue. +2. **Использование helper на трёх уровнях** — сверено: `src/render/opening-symbol.ts` + вызывает `openingSymbolOffset` и транслирует только swing-группу (arc/leaf/glass) + через `swingTx/swingTy`, при этом косяки (`` на `x=±half`) рисуются вне + этой группы и не двигаются — значит смещение сегодня действует только на + вращающуюся часть символа, а не на полную глубину косяка; это согласуется с + AC6 ТЗ («jamb depth остаётся 70 см независимо от flip»). `src/iso-openings.ts:87-89` + строит `origin = [x + offset.ox, y + offset.oy]` и передаёт его в `hinge` — + при offset≡0 origin всегда `(x, y)`, а направление у door/window всё равно + мутируется через `sy` внутри `transformVector` на **quarterVector** (не на + hinge), то есть анимация открывания меняет сторону без переноса точки + крепления — ровно то, что заявляет §7.2 ТЗ. +3. **Gate не трогается** — `face.side * 10` в `render/opening-symbol.ts` и + `iso-openings.ts` действительно не зависит от `openingSymbolOffset`; мутант + `opening-gate-flip-cancels-turn` в `scripts/mutation-gate.mjs:1697` целится в + отдельную строку (`gateAngle = spec.face.side * 10 * amount`), не в offset — + ТЗ корректно относит его к «сохраняется», а не «выводится из реестра». +4. **Устаревающие мутанты (§14)** — проверены `opening-symbol-default-uses-room-face`, + `opening-symbol-partition-follows-endpoints`, `opening-gate-flip-translates-leaves` + в `scripts/mutation-gate.mjs:1660-1692`: все три патчат строку с условием + `if (!flipV || type === 'gate' || …)`. Если реализация делает + `openingSymbolOffset` тождественным нулём (вариант, прямо разрешённый + владельцем в теле issue и продублированный в §19.1 ТЗ), эта строка исчезает + и `find`-паттерн патча перестаёт совпадать — три анкера действительно + становятся непредметными, а не произвольно списанными. +5. **Golden-сцены** — все четыре ID (`opening-symbol-room-wall-light`, + `opening-symbol-diagonal-partition-dark`, `opening-symbol-flip-pairs-light`, + `isometric-opening-symbol-parity-dark`) существуют в + `demo/golden/matrix.mjs:288-301` и в `demo/golden/baselines/baselines-index.json`. + `openingFlipContract` (строки 39-55) уже содержит пары `flipV:true` с + `offset: 'edge'` — это ровно то значение, которое реализация обязана сменить + на `'center'`; `harness.mjs:161-179` подтверждает, что `offset` — размеченное, + проверяемое поле контракта (`'center'|'edge'`), а не décor. Технически + заявленный план (§13.3) выполним без структурных изменений сцены. +6. **Документы, которые «больше не обещают edge alignment» (AC8)** — найдены + ровно те формулировки, которые ТЗ обязуется поменять: `docs/WALL-THICKNESS.md:107-110`, + `docs/USER-GUIDE.ru.md:616-620`, `docs/ARCHITECTURE.md:582`, + `docs/ISOMETRIC.md:161-162`. Все действительно говорят «flip_v: true + edge-aligns door/window» на текущем `dev` — значит AC8 указывает на реальные + строки, а не на воображаемую проблему. +7. **Обязательные разделы ТЗ (§7.1 PROCESS.md)** — сверены по списку: сценарий (§1), + что человек увидит до/после (§2), проблема (§3), скоуп и не-скоуп (§6), + контракт поведения (§7), UX (§8), модель данных и migration (§9), i18n/a11y/touch + (§10), AC1…AC9 с доказательством (§12), план автотестов (§13), риски (§18), + откат (§17), release-артефакты (§16) — присутствуют все. +8. **Продуктовая неоднозначность** — решение владельца зафиксировано прямо в теле + issue («Все проёмы … располагаются на осевой линии стены, независимо от + настроек») и повторено в аналитике владельца-эксперта («Решение владельца + однозначно; продуктовых вопросов для ТЗ нет»). ТЗ ничего не додумывает сверх + этого текста; раздел §19 явно и корректно отделяет только техническые + допущения («принято предположительно, поменять свободно») от решённых + владельцем пунктов. + +## Находки + +Блокирующих (High) находок нет. Находок Medium в скоупе или вне скоупа нет. + +**Low (снята с записью, не возвращается автору):** ТЗ #250 не добавлено строкой +в таблицу `docs/specs/README.md` (там пока только #242 и #248 из соседних +задач). Раздел не входит в обязательные §7.1/DoR-пункты, сама таблица отмечена +в PROCESS.md §7.3 как технический долг с другой стороной проблемы (дублирующая +колонка статуса, а не отсутствие строки), и ничего не блокирует ни автора, ни +ревьюера кода. Снимаю без возврата; при желании строка добавляется тем же +коммитом, что и реализация. + +## Что проверено и корректно + +- Сценарий и персона (§1) соответствуют J1/J6 из `docs/SCOPE.md`: «план должен + одинаково и физически понятно показывать проёмы», «сохранённая настройка не + должна скрыто менять их положение» — обе фразы буквально из аналитики + владельца, не изобретены автором ТЗ. +- «Что человек увидит» (§2) сформулировано без терминов реализации, одной + фразой до/после — соответствует требованию PROCESS.md §7.1. +- Контракт (§7) точен и проверяем численно: `{ox:0, oy:0}` — не «около нуля», а + точное значение; 70-см стена → офсет 0, jamb 35 см от оси в каждую сторону, + что для стены 70 см даёт полный охват — арифметика непротиворечива. +- Матрица AC1 (4 типа × 2 flip × horizontal/vertical/diagonal × positive/ + negative/zero/malformed face) полностью покрывает пространство входов текущей + сигнатуры `openingSymbolOffset(type, flipV, angle, face)` — заведомо не + half-baked. +- Скоуп/не-скоуп (§6) корректно исключает `flip_h`, `compareOpeningSides`, + `face.side`, `partitionOpeningFace`, wall cut/tunnel/jambs/Glow/sun, lock + badge, миграцию/backend, новый UI-контрол и публикацию скрытого Iso — + совпадает с диагнозом (§3), который показывает, что все эти механизмы читают + `face`/`side`/`cm` напрямую, а не через удаляемый offset. +- Модель данных (§9) корректно называет это intentional семантическим + изменением уже сохранённого поля, а не миграцией: формат `OpeningCfg.flip_v` + (boolean) не меняется, что действительно не требует version bump по + `docs/CONFIG-COMPATIBILITY.md`. +- Риски (§18) названы предметно и с митигацией через конкретные AC/мутанты, а + не общими словами; §14 mutation guards корректно разводит «анкер, который + перестанет существовать» и «анкер, который обязан продолжать падать». +- Откат (§17) реалистичен: одна code revision восстанавливает старую ветку, + данные не трогаются. +- Технические допущения (§19) явно помечены как решаемые автором/ревьюером, а + не выданы за продуктовое решение — соответствует «догадка, записанная как + факт, — худший вид дефекта». +- Владельцу за этот раунд не задано ни одного вопроса — и это оправдано: все + продуктовые развилки (что видно, что нельзя трогать) уже закрыты текстом + самого issue. + +## Чего не проверял + +- Не запускал `npm run typecheck`/`npm test`/`npm run build` — на этапе ТЗ нет + кода для сборки (диапазон изменений `git diff origin/dev..HEAD` содержит + только новый файл `docs/specs/250-opening-centerline.md`, 348 строк, класс C). +- Не проверял golden/browser smoke — они относятся к код-ревью, здесь нет + реализации, которую можно было бы прогнать. +- Не проверял `check-docs`/`process-gate --issues`, упомянутые автором в + хендоффе, как отдельные команды — это инфраструктурные гейты уровня коммита + документации, а не предмет содержательного ревью ТЗ; факт «класс C коммит, + трейлеры на месте» проверен `git show --stat` (см. выше), этого достаточно + для этого этапа. +- Не оценивал `test/opening-symbol.test.mjs` и `test/iso-openings.test.mjs` на + предмет их будущей полной переписки — план автотестов (§13.1) описывает + направление правки, конкретные assertions появятся в реализации и будут + предметом код-ревью, а не ревью ТЗ. + +## Вердикт + +Зелёный. ТЗ выполнимо, каждый AC проверяем и привязан к способу доказательства, +продуктовая неоднозначность отсутствует (решена владельцем в теле issue), +диагноз и заявленный релиз-impact подтверждены построчным чтением текущего кода +и документов, а не приняты на слово автора. + +`Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче`