diff --git a/docs/reviews/CODE-REVIEW-484-r1.md b/docs/reviews/CODE-REVIEW-484-r1.md new file mode 100644 index 00000000..759a263e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-484-r1.md @@ -0,0 +1,234 @@ +# CODE-REVIEW-484-r1 + +Issue: [#484](https://github.com/Matysh/houseplan-card/issues/484) — PDF: внешняя размерная цепь теряет размеры ступенчатого фасада +ТЗ: [docs/specs/484-pdf-exterior-dimension-chain.md](https://github.com/Matysh/houseplan-card/blob/dev/docs/specs/484-pdf-exterior-dimension-chain.md), ревью ТЗ [SPEC-REVIEW-484-r1.md](https://github.com/Matysh/houseplan-card/blob/dev/docs/reviews/SPEC-REVIEW-484-r1.md) — зелёный. +Ветка: `issue/484-pdf-exterior-dimensions`. Материал ревью: `git diff origin/dev...HEAD`, HEAD на момент вывода вердикта — `904b8b225159e61546aba9a1ad46f548a1b3f4db` (сверено непосредственно перед выводом, `git status` чист). +Заход: r1 (первый заход, разделов «Унаследовано»/«Закрытие r0» не требуется — §2.10 применяется со второго цикла). + +## 1. Скоуп диффа + +62 файла, ядро изменения — два продуктовых файла: + +- `src/pdf/pdf-collision.ts` — новый opt-in режим `allowStartExit` в + `pdfSegmentTouchesGeometry()`: собирает пересечения extension line с + архитектурными rings в отсортированные breakpoints, ищет первый свободный + интервал («выход из собственного узла») и после него красит любое + касание/пересечение/overlap как коллизию. +- `src/pdf/pdf-scene.ts` — только две внешние extension lines (`entry.a→entry.oa`, + `entry.b→entry.ob`) переведены с `allowStartBoundary` на `allowStartExit`; + dimension line, shelf, labels и внутренние размеры остались на строгом пути. + +Остальное — тесты (`test/pdf-collision.test.mjs`, `test/pdf-scene.test.mjs`, +`test/golden-matrix.test.mjs`), два обязательных мутанта в +`scripts/mutation-gate.mjs`, новый browser-смок кейс в `demo/smoke_pdf_export.mjs`, +привязка символов в `scripts/smoke-links.mjs`, golden-фикстура/сценарий +(`demo/golden/harness.mjs`, `demo/golden/matrix.mjs`, `demo/golden/run.mjs`, +новый эталон `pdf-export-stepped-dimensions-light` + два обновлённых +`pdf-export-geometry-light`/`pdf-export-polish-light`), документация +(`ARCHITECTURE.md`, `PDF-EXPORT.md`, `USER-GUIDE.md`/`.ru.md`, оба CHANGELOG, +`STATUS.md`, `docs/specs/README.md`) и пересобранный бандл/фингерпринт +скриншотов. + +## 2. Как проверялось + +| Гейт | Команда | Результат | Источник | +|---|---|---|---| +| typecheck+build | `npm run build` (= `tsc --noEmit && rollup -c`) | зелёный | прогнан лично на HEAD | +| unit — целевые | `npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs && node --test --test-name-pattern="external dimension extension\|stepped exterior\|reconstructable dimension chain" test/pdf-collision.test.mjs test/pdf-scene.test.mjs test/golden-matrix.test.mjs` | 3/3 pass | прогнан лично | +| unit — полный набор + backend | — | не перегонял отдельно | зелёный Validate на точном SHA `904b8b22` (https://github.com/Matysh/houseplan-card/actions/runs/34151982086), см. условие сужения в задании ревью | +| мутант AC3 `pdf-own-exit-prefix-disabled` | `node scripts/mutation-gate.mjs --id=pdf-own-exit-prefix-disabled` | «тест покраснел, как обязан», 1/1 | прогнан лично | +| мутант AC4 `pdf-post-exit-collision-ignored` | `node scripts/mutation-gate.mjs --id=pdf-post-exit-collision-ignored` | «тест покраснел, как обязан», 1/1 | прогнан лично | +| смок (по `smoke-select.mjs`) | `node demo/smoke_pdf_export.mjs` (после `npm run bundle:sync`) | зелёный, `steppedExteriorKeepsCompleteDimensionChain: true`, 0 EXC, exit 0 | прогнан лично | +| `check-docs.mjs` | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 12 external links)» | прогнан лично | +| `golden:verify` | `npm run golden:verify` | зелёный, exit 0; отдельно подтверждены три PDF-сценария: `pdf-export-geometry-light` passed, `pdf-export-polish-light` passed, `pdf-export-stepped-dimensions-light` passed | прогнан лично дважды (второй раз — построчный лог) | +| model-invariants | — | не запускал | диф не трогает геометрию плана/сохранённую модель — только временную PDF-сцену коллизий поверх уже построенных `architectureRings`; §6.3/§8 ТЗ явно фиксируют read-only и отсутствие изменений модели | +| `pytest tests_backend` | — | не запускал | Python/бэкенд не тронуты (0 файлов `custom_components/**/*.py` в диффе) | +| performance-профили сверх стандартных | — | не запускал | диф не трогает `src/iso-*`, `src/live-*`, `src/render-*`, `houseplan-render-lifecycle.ts`, `houseplan-card.ts`; названный в AC6 бюджет `<200 ms` проверяется существующим неизменённым тестом `test/pdf-scene.test.mjs:618` («current 20-room large-house space builds... under 200 ms»), покрыт зелёным полным прогоном `test` в Validate на точном SHA | + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD` вернул ровно одну +**зарегистрированную связь** — `demo/smoke_pdf_export.mjs`, объявленную через +символы `pdfSegmentTouchesGeometry`, `SegmentIntersection`, `segmentIntersection`, +`COORDINATE_EPSILON`, `PdfCollisionPoint`, `forEachRingSegment`, `pointOnSegment`, +`segmentParameter` (добавлены автором в `scripts/smoke-links.mjs` этим же диффом). +Других смоков (из 229 в дереве) diff не касается — прогон одного соответствует +объёму задачи (изменение локально для `src/pdf/**`). + +## 3. Разбор по AC + +- **AC1** (полная цепь ступени, оба winding) — доказано `test/pdf-scene.test.mjs` + «stepped exterior keeps the complete reconstructable dimension chain»: цикл по + `[step, [...step].reverse()]`, точные значения `1.20/2.40/3.60/3.75` присутствуют + для обоих обходов. Прогнано лично, зелёный. +- **AC2** (единственность глубины, dedup, без диагонали) — тот же тест: + `values.filter(v => v === '1.20 m').length === 1`, `values.length === 6` + (4 внешних + общие width/height), `command.angle % 90 < 1e-8` для всех подписей. + Зелёный. +- **AC3** (разрешённый начальный выход) — `test/pdf-collision.test.mjs` покрывает + коллинеарный выход вдоль примыкающей ступени, выход через тело инцидентной + стены (квадрат), сегмент, не достигающий свободного участка (fail closed), и + ограничение исключения только сегментами, реально стартующими на своей + границе. Защитный мутант `pdf-own-exit-prefix-disabled` красит тест при + отключении режима — проверено лично, эффект подтверждён. +- **AC4** (запрет повторного входа после выхода) — тот же файл: второй wall + после первого свободного интервала (`combinedSolid`, солид-коллизия) и + касание постороннего угла без солид-интервала (`stepSolid`-only, точечное + касание за счёт финальной проверки `hits.some(hit.t > exitT...)`) — оба дают + `true`. Защитный мутант `pdf-post-exit-collision-ignored` красит тест при + отключении финальной проверки — проверено лично, эффект подтверждён. + Разобрано и по коду (см. §4 ниже) — механизм state-machine «префикс → выход → + любое дальнейшее касание» соответствует §6.2 ТЗ построчно. +- **AC5** (label/dimension line/shelf/внутренние размеры не изменились) — + проверено чтением: `grep -n "segmentTouchesArchitecture" src/pdf/pdf-scene.ts` + показывает ровно один вызов (для двух extension lines внешнего размещения); + `allowStartExit` не используется больше нигде в `src/pdf/pdf-scene.ts`; + `allowStartBoundary` для внешних сегментов заменён полностью (не оставлен как + мёртвый код), а как публичный API `pdfSegmentTouchesGeometry` со старым + поведением по-прежнему покрыт своими исходными тестами (не тронуты этим + диффом). Внутренние размеры комнат этот collision-helper вообще не вызывают. +- **AC6** (winding-независимость, детерминизм, `<200 ms`) — winding-цикл в + AC1-тесте покрывает независимость от обхода; perf-бюджет — существующий + неизменённый тест, зелёный в Validate на точном SHA. +- **AC7** (визуальное присутствие цепи в PDF) — двойное доказательство: + browser-смок `demo/smoke_pdf_export.mjs` реально скачивает PDF из карточки в + Chromium, разбирает через `pdfjs-dist` и проверяет присутствие всех шести + подписей (`1.20/2.40/3.60/3.75/7.35/9.75 m`) — прогнано лично, зелёный; golden + `pdf-export-stepped-dimensions-light` с `pdfSemantic.requiredDimensionLabels` + и `forbiddenText` — прогнано лично (`golden:verify`), passed. +- **AC8** (документация, changelog, docs/golden fingerprints) — оба CHANGELOG + ссылаются на #484 в одном коммите с кодом (`e88ef93c`), `PDF-EXPORT.md` и + `ARCHITECTURE.md` описывают узкий source-exit контракт словами, совпадающими + с §6.2 ТЗ, `USER-GUIDE.md`/`.ru.md` формулируют пользовательское обещание. + `check-docs.mjs` зелёный. Скриншот-фингерпринт обновлён отдельным коммитом + `7b826c19` с трейлером `Baseline-Reviewed: <ссылка на прогон>` — пиксели + десяти doc-кадров не изменились (`decision.replace.length === 0`, + `lastWriteWasFingerprintOnly: true`), что ожидаемо: правка изолирована в PDF + export collision-геометрии и не задевает ни один из синтетических doc-сценариев. + +## 4. Разбор ключевой логики чтением (без исполнения, дополнительно к тестам) + +Прочитан построчно новый ветвь `allowStartExit` в `pdfSegmentTouchesGeometry()` +(`src/pdf/pdf-collision.ts:168-208`). State machine: + +1. Если старт не на границе архитектуры — `true` (нет смысла в исключении, fail closed). +2. Собираются все пересечения `start→end` с рёбрами всех rings (`hits`), включая + точечные касания (`t === endT`) и коллинеарные перекрытия (`overlap: true`, + диапазон `[t, endT]`). +3. Отрезок делится на интервалы по отсортированным уникальным breakpoints + (`0, 1, hit.t…, hit.endT…`, схлопнутым с `parameterEpsilon`); каждый интервал + помечается «blocked», если его середина внутри `isSolid` или внутри + collinear-overlap. +4. Идёт слева направо: пока не найден первый свободный интервал — копится + «собственный префикс» (не важно, что именно его блокирует — коллинеарный + участок примыкающей грани или солид инцидентной стены, оба разрешены §6.2); + как только `exitT` найден — любой следующий blocked-интервал даёт `true`. +5. Если свободного интервала не нашлось вовсе — `true` (line 206, буквально + реализует «линия, которая так и не получает свободного участка» из §6.2 — + fail closed). +6. Финальная строка (207-208) ловит случай, который шаг 4 пропускает: точечное + касание постороннего угла ПОСЛЕ выхода, не создающее blocked-интервал (обе + соседние середины остаются свободны), но являющееся касанием по существу — + `hits.some(hit.t > exitT || hit.endT > exitT)`. Именно этот путь проверяет + тест «a later point contact remains a collision even without a solid + interval», и именно его выключает мутант `pdf-post-exit-collision-ignored`. + +Прослежено вручную на fixture из ТЗ/бага (`(10,3)→(16,3)` вдоль ступени +`(0,0)→(10,0)→(10,3)→(12,3)→(12,6)→(10,6)→(10,10)→(0,10)`): начальный коллинеарный +перекрыв с ребром `(10,3)-(12,3)` даёт blocked-префикс `[0, 0.333]`, `exitT=0.333`, +после чего свободно до конца — линия принимается без коллизии, что и требуется +для восстановления глубины выступа. Согласуется с зелёным `AC1`/`AC3`-тестами. + +Существенных расхождений между кодом, ТЗ §6.2 и текстом issue не найдено. + +## 5. Находки + +Находок High или Medium нет. Two Low-замечания, ни одно не блокирует и не +требует правки: + +1. **Low.** Комментарий в `pdf-collision.ts:204-205` объясняет назначение + финальной проверки, но не называет её отдельно как «point-contact + detector» — при последующей правке этой функции легко принять её за + дублирование шага 4 и удалить как «мёртвый код», сломав ровно тот случай, + который ловит мутант `pdf-post-exit-collision-ignored`. Не блокирует: тест и + мутант физически предотвращают тихую регрессию, комментарий — не более чем + удобство чтения. Оставляю как есть, автор/ревьюер могут усилить комментарий + по желанию, отдельного цикла это не заслуживает. +2. **Low.** `docs/images/screenshots.json` получил `"acceptedOn": "win32"` + вместо `"linux"` в этом же диффе (коммит `7b826c19`, `Baseline-Reviewed` + указывает на CI-прогон). Механизм `#401/#455` (`scripts/docs-accept.mjs`) + намеренно допускает приёмку с любой машины при условии совпадения + witness-хешей с уже закоммиченными кадрами — это не нарушение процесса, а + штатный путь «fingerprint-only» accept (ни один из 10 PNG не заменён: + `lastWriteWasFingerprintOnly: true`). Отмечаю только потому, что поле смены + платформы бросается в глаза при чтении диффа; проверка (`check-docs.mjs`) + зелёная, разбирать глубже не требуется. + +## 6. Что проверено и корректно + +- Контракт `allowStartExit` реализует именно ту state machine, которую + описывает §6.2 ТЗ, включая fail-closed при недостижимости свободного участка + и защиту от post-exit re-entry (в т.ч. точечных касаний). +- `allowStartBoundary` (старое поведение) не удалён и не изменён — используется + собственными старыми тестами и остаётся доступным API; ни один существующий + вызывающий код кроме двух extension lines не переключён на `allowStartExit`. +- AC1/AC2 подтверждены точными числовыми значениями на fixture из issue, для + обоих направлений обхода ring. +- Оба обязательных защитных мутанта (AC3, AC4) реально красят целевой тест при + снятии соответствующей защиты — проверено запуском, а не по описанию. +- Browser smoke реально качает PDF из собранного бандла (`dist` → `bundle:sync` + → demo stand) и разбирает его `pdfjs-dist`, а не читает внутреннее состояние + — сквозной путь до реального артефакта. +- `golden:verify` (полный матрикс, обязателен процессом — частичный запуск + инструмент отвергает) зелёный, включая обновлённые/новый PDF-эталоны. +- Документация (оба changelog, PDF-EXPORT.md, ARCHITECTURE.md, USER-GUIDE и + .ru.md, STATUS.md, specs/README.md) согласована с контрактом ТЗ и терминологией + USER-GUIDE.ru.md. +- Traceability: `Issue: #484` и `User-Visible` есть на всех коммитах диапазона; + оба changelog правились в одном коммите (`e88ef93c`) с поведенческим фиксом. + +## 7. Чего не проверял и почему + +- Полный `npm test`/`npx tsc --noEmit` отдельно от `npm run build` — не + перегонял: Validate зелёный на точном SHA `904b8b22` + (https://github.com/Matysh/houseplan-card/actions/runs/34151982086), что по + условию задания достаточно для этой пары дешёвых гейтов; `build` всё же + прогнал лично (нужен был живой `dist` для смока). +- `python -m pytest tests_backend` — не запускал, диф не касается + `custom_components/**/*.py`. +- `node scripts/model-invariants.mjs` — не запускал: диф не меняет геометрию + плана, `layout`, `marker.space`, `open_spans` или записи толщины стен; PDF + export работает поверх уже готовой read-only сцены и не пишет обратно в + модель (подтверждено §8 ТЗ и чтением: изменения ограничены `pdf-collision.ts` + /`pdf-scene.ts`). +- 228 из 229 файлов `demo/smoke_*.mjs`, не названных `smoke-select.mjs` и не + упомянутых в AC, — не прогонял: diff не расширяется дальше PDF-экспорта, + полный прогон — предрелizный гейт (§8 PROCESS.md), не гейт ревью. +- Performance-профили `large-house-isometric-v1`/`large-house-interaction-v1` — + не запускал: правка не трогает `src/iso-*`/`src/live-*`/`src/render-*` + /`houseplan-render-lifecycle.ts`/`houseplan-card.ts`, названные в §8 условия + их подключения к Validate не выполняются. +- Живой снимок скринов документации (`npm run docs:accept`) — не переснимал: + `check-docs.mjs` уже зелёный на текущем fingerprint, коммит `7b826c19` уже + содержит доказательство (`Baseline-Reviewed`). + +## 8. Вердикт + +Зелёный. Все 8 AC доказаны автотестом (с проверенной способностью падать через +два обязательных мутанта) либо разобраны чтением там, где защита тривиальна +(AC5 — отсутствие новых вызовов). High: 0. Medium: 0. Low: 2, обе сняты без +правки с записью выше (п. 5). + +--- + + + +## Материал раунда + +- Ветка: `issue/484-pdf-exterior-dimensions`, коммит `904b8b225159` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0b96360647a80380beab99dba4225b3f72556d30` + ``` + git log --all --format='%H %T' | grep 0b96360647a8 + ``` +- ТЗ `docs/specs/484-pdf-exterior-dimension-chain.md`, блоб `84fb3a65e309f90c2c8c149b6f7bb4f6bf876f4d` + ``` + git log --all --find-object=84fb3a65e309f90c2c8c149b6f7bb4f6bf876f4d -- docs/specs/484-pdf-exterior-dimension-chain.md + ```