diff --git a/docs/reviews/CODE-REVIEW-282-r2.md b/docs/reviews/CODE-REVIEW-282-r2.md new file mode 100644 index 00000000..a5a3ee3d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-282-r2.md @@ -0,0 +1,312 @@ +# CODE-REVIEW-282-r2 + +- **Issue:** #282 — стабильная идентичность сегментов стен (ADR Stage 1) +- **Ветка:** `issue/282-wall-geometry-model`, HEAD `3588284f` (после ребейза на `origin/dev` `30698d6e` — merge-base подтверждён, ветка полностью впереди `dev`) +- **ТЗ:** `docs/specs/282-stable-wall-segment-identity.md` (spec-review зелёный на r2, `2f30c481`) +- **Этап:** код-ревью (PROCESS.md §2.7) +- **Заход:** r2 · блокирующих циклов израсходовано 1 из 4 до этого вердикта +- **Предыдущий раунд:** `docs/reviews/CODE-REVIEW-282-r1.md`, красный, HEAD на тот момент `5adfdb5d` (SHA назван в документе — не находка). Этот SHA более не существует в истории ветки: после r1 ветка дважды уходила на ребейз (issue-комментарии `#282`, 19:33 и 21:12/21:21), причём второй ребейз (на `dev` `30698d6e`) потребовал реального разрешения конфликта — по словам автора, «post-rebase fixes also make direct backend Optimize cross the v7→v8 barrier, retain authored active-chain coordinates during conversion, and keep a rejected wall transaction atomic» — то есть новую продуктовую логику, а не только смену родителя. +- **Вердикт:** **красный** + +## 1. Скоуп проверки + +Раунд не локален по критериям PROCESS.md §2.10: ребейз на ушедший вперёд `dev` +с конфликтом, разрешённым новым кодом в самом identity-барьере — ровно та +подсистема, где r1 нашёл H1/H2. Поэтому разбор полный, но выполнен по факту +через дельту `9fc213c6..HEAD` (27 файлов, +1793/−1020: коммит `9fc213c6` — это +сам документ `CODE-REVIEW-282-r1.md`, его родитель `466a6765` — переименованный +рикбейзом эквивалент `5adfdb5d`, на котором получен вердикт r1). Дельта +покрывает: `src/houseplan-card.ts` (`_commitPhysicalGeometry`, +`_applyGeometryState`), `src/wall-segment-model.ts` (lineage hints, off-grid +guard), `src/plan-geometry-preflight.ts`, backend +`validation.py`/`wall_segment_model.py`/`websocket_api.py`, тесты и +mutation-gate. Остальные ~45 файлов исходного 62-файлового диффа r1 (import/export +remap, i18n, документация, backend-зеркало валидации помимо перечисленного) +дельтой не задеты — раздел «Унаследовано из r1» ниже. + +## 2. Как проверялось — таблица гейтов + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный | +| Unit/frontend | `npm test` | 1331 тестов, **1329 passed, 1 failed, 1 skipped** — см. H1 | +| Build + sync бандла | `npm run build && npm run bundle:sync` + `cmp` трёх копий | зелёный, `dist/`, `custom_components/houseplan/frontend/`, `demo/srv/assets/` побайтово идентичны | +| Docs fingerprint | `node scripts/check-docs.mjs` | зелёный (7 файлов, 10 внешних ссылок) — обязателен, дельта трогает `src/**` | +| Model invariants (синтетика) | часть `npm test` (`test/model-invariants.test.mjs`, `test/wall-segment-model.test.mjs`) | зелёный | +| Model invariants (реальная нагрузка) | `node scripts/model-invariants.mjs --config ` | зелёный до миграции («ссылки разрешимы, записи толщины находятся»), зелёный после; `--lattice`: 0.00% шума у узла | +| Smoke selection | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 31 прямое совпадение, 5 зарегистрированных связей, 7 слабых — вывод см. §4 | +| Golden | `npm run golden:verify` | **зелёный**, 129/129 сценариев, в т.ч. оба сценария из M1(r1) | +| Mutation gate | `node scripts/mutation-gate.mjs --check` | **зелёный**, 226/226 мутантов пойманы, включая 3 мутанта #282 (editor-commit/history-restore/optimize barrier) | +| Named/registered smokes | 31 прямых + 5 зарегистрированных = 36 запущено | **1 красный** (`smoke_open_passage.mjs`) — см. H2, остальные 35 зелёные | +| Backend pure (`tests_backend`) | не прогонялся мной — среда без `homeassistant`/`voluptuous`/`.venv-backend` (см. §6) | проверено чтением диффа `validation.py`/`wall_segment_model.py` + нового backend-теста, не исполнением | +| `pytest tests_backend` (полный, Linux CI) | не прогонялся | пре-релизный гейт, вне объёма код-ревью (§8) | +| Performance-профили (полный `performance_smoke`) | не прогонялся | пре-релизный гейт; целевой `npm run benchmark:wall-model` дельтой не задет (файл вне диффа) | + +Дешёвые гейты (`tsc`, `build`, `check-docs`) зелёные. Но `npm test` сам по себе +**красный** — это не пре-релизный, а обязательный на каждом раунде гейт (§8, +§2.10), и он не проходит на предъявленном HEAD. Расширенные гейты (invariants, +smoke-select, golden, mutation-gate, целевые smoke) прогнаны, потому что дельта +меняет геометрию/ссылки на неё и барьер идентичности — именно то сочетание, +для которого §8 требует их «по необходимости». Golden и mutation-gate теперь +полностью зелёные (закрывают M1(r1) и M2(r1) — см. §5 «Закрытие раунда r1»); но +один из 36 запущенных смоков нашёл новую, не связанную с r1 регрессию (H2). + +## 3. High — блокирующие находки + +### H1. `npm test` красный: диагностика больше не соответствует контракту, который сама же проверяет + +**Файл:** `test/open-passage-contract.test.mjs:70-74`, тест «all write/import +paths invoke the semantic passage validator». + +**Воспроизведение:** + +``` +npm test +# ... +not ok 688 - all write/import paths invoke the semantic passage validator + Expected values to be strictly equal: + 3 !== 2 + location: test/open-passage-contract.test.mjs:71 +``` + +Тест жёстко ожидает ровно 2 вхождения `validate_opening_passages(` в +`custom_components/houseplan/websocket_api.py`. Последний коммит дельты, +`3588284f` («fix: preserve passage validation during wall migration»), +осознанно добавил третий вызов — `validate_opening_passages(candidate_config, +config_data.get("config"))` в `ws_plan_optimize` **до** барьера +`commit_wall_segment_model`, рядом с уже существующим вызовом **после** барьера +(строки 1655 и 1672; третий, старый вызов на 1327 — в `ws_config_set`). Это +осмысленное и корректное по существу исправление (см. §5, closure M-out +comment #16 в issue): без него Optimize возвращал общий +`wall_model_migration_blocked` вместо конкретного +`invalid_passage_fields`-кода на конфиге с некорректным passage ещё до миграции. +Но тест, фиксирующий количество мест вызова, не обновлён вместе с фиксом — он +писался под контракт «ровно 2 вызова» и не отражает намеренно добавленный +третий. + +Это прямое нарушение дисциплины: `npm test` — гейт, обязательный **в каждом +раунде** (§2.10, §8), и он сейчас красный на предъявленном коде. Хендофф- +комментарий автора к `3588284f` (issue, 22:04:14) называет только узкий прогон +14 pure-passage-кейсов и process-gate, не полный `npm test` — расхождение +между «verified» и фактическим состоянием gate. + +**Доказательство:** тест уже красный на HEAD, механически (`node --test +test/open-passage-contract.test.mjs`); падение детерминировано, не флуктуация. + +### H2. AC7 нарушен: opening на обычной стене исчезает со Static-карточки после миграции в v8 + +**Файл:** `demo/smoke_open_passage.mjs` — не тронут этой дельтой (0 строк в +`git diff --stat origin/dev...HEAD`). Корень — `src/space-render.ts:217-278` +(`renderSpaceStatic`, единственный потребитель — `space-card.ts:770`, т. е. +`houseplan-space-card`, документированная в `ARCHITECTURE.md` «Second card»); +файл не тронут диффом #282 вовсе (`git diff --stat origin/dev...HEAD -- +src/space-render.ts` → пусто). + +**Воспроизведение:** на `origin/dev` (`30698d6e`, отдельный git-worktree, тот +же `npm run bundle:sync`): + +``` +node demo/smoke_open_passage.mjs → OK (все 13 проверок true) +``` + +На HEAD (`3588284f`), дважды подряд, детерминированно: + +``` +"staticCutsAndFillsPassage": false +FAILED (1) +``` + +Целевая диагностика (см. приложенный debug-прогон) показывает точный механизм: +после того как `_saveOpening()` проводит объект через +`_commitPhysicalGeometry` (v7→v8 baseline-replay из `c694f6ea`), опция +`passage-main` корректно получает `host: {kind:'wall', id:'wall-…', t:0.5}` — +барьер `hostRoomOpenings` (`src/wall-segment-model.ts:530-556`) тегирует +**любой** незакреплённый за partition opening (door/window/gate/passage +одинаково, без разбора по типу) таким `host` при миграции. Но +`renderSpaceStatic` (`space-render.ts:217-222`, `resolvedHosted`) резолвит +`opening.host` только через `resolvePartitionOpeningCompat` — функцию, +понимающую исключительно `host.kind === 'partition'`. Для нашего +`host.kind === 'wall'` резолюция не находится, `resolvedHosted` пуст для этой +записи, и `resolvedRawOpenings` (строки 274-278) на строке `return resolved ? +[materializePartitionOpening(...)] : []` **молча роняет** opening из всего +дальнейшего рендера Static-карточки — не только из tunnel-заливки прохода, а +из любой отрисовки (символ, вырез в стене, hit-box). + +Сам код `src/houseplan-card.ts` (полный/Plan/View card) везде корректно +различает `host?.kind === 'partition'` от прочих случаев (8295, 8325, 8480, +12766, 12804, 12958) — регрессия локализована именно в `space-render.ts`, +единственном месте, которое ADR Stage 1 должен был обновить и не обновил, хотя +сам ТЗ прямо обещает это в AC7: «v8 → legacy… projection даёт текущим full, +**static**… consumers прежние inputs» с доказательством «projection snapshots ++ **existing geometry suite**» — а именно этот existing-geometry смок +(`smoke_open_passage.mjs`, часть suite с #157) и ловит нарушение AC7. + +Blast radius не ограничен синтетическим passage из смока: `hostRoomOpenings` +единообразно тегирует **все** типы openings без partition-host при миграции, +значит после Optimize/любой структурной правки, переводящей пространство в +v8, **любой** door/window/gate/passage на обычной (не-partition) стене +перестаёт отображаться на `houseplan-space-card` — символ, вырез, hit-box. +Это прямой конфликт с J1/J2 из `docs/SCOPE.md` (план должен показывать, что +происходит и где) на реальной, документированной с v1.16.0 поверхности +продукта. + +**Доказательство:** тест умеет падать — зафиксирована пара «зелёный на +`origin/dev` `30698d6e` / красный на HEAD `3588284f`» на байт-идентичном файле +теста, дважды воспроизведено. Причина локализована до конкретных строк (в +отличие от недиагностированных H1/H2 из r1) через целевой debug-прогон, +приложенный к разбору находки, а не только «зелёный/красный». + +**Итог по H1+H2:** оба блокируют. H1 — процессный, лечится синхронизацией +числа в тесте с фактическим количеством вызовов (тривиально, но не сделано). +H2 — продуктовый: явное нарушение собственного AC7 задачи, широкий blast +radius на документированной поверхности продукта, которую эта дельта не +касалась и не тестировала. + +## 4. Medium — по существу задачи + +Новых находок Medium в этом раунде нет. Все четыре Medium из r1 закрыты — см. +§5 «Закрытие раунда r1». Отдельных находок вне скоупа нет — новый issue не +заводился. + +## 5. Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **H1** — guard #278 не отклоняет правку деградированного пространства | `_commitPhysicalGeometry` (`c694f6ea`, `src/houseplan-card.ts:7296-7313`) теперь вызывает `_checkSpacePhysicalGeometry` на `liveCandidate` **до** прохождения identity-барьера (`legacySafe`), а не после — восстановлен исходный порядок guard-а #278 | `node demo/smoke_wall_union_isolation.mjs` → OK, все 11 проверок `true` (перепроверено мной на HEAD, включая три ранее красные: `degradedPhysicalEditRejected`, `rejectedEditHasNoHistoryOrWrite`, `rejectedEditHasLocalizedToast`) | +| **H2** — 8 именованных structural-smoke красные | Переработан `_commitPhysicalGeometry`: для v7-пространства identity выводится из pre-edit baseline (`baselineSource`/`historyBefore`), а не из уже отредактированной геометрии, плюс lineage hints (`fixedTopologyWallLineageHints`) для сохранения ID при чистом split | Все 8 смоков (`smoke_resize_pointer_real_plan`, `smoke_wall_chain_thickness`, `smoke_unified_wall_tool`, `smoke_wall_face_overlap`, `smoke_edit_walk`, `smoke_editor_tabs`, `smoke_plan_snap_overlay`, `smoke_partition_openings`) перепрогнаны мной на HEAD — все зелёные | +| **M1** — golden baseline не обновлён под AC15-строку | Baseline не потребовался: фикстура `optimize-orphan-references-*` (`demo/golden/matrix.mjs:695-720`) уже v8/canonical, `Optimize` на ней не мигрирует ни одного сегмента, `r.wallSegmentsMigrated` = 0 → строка `gs.wall_segments_migrated` не рендерится, визуального различия нет | `npm run golden:verify` → 129/129 passed, включая `optimize-orphan-references-dark-en`/`-light-ru`; `demo/golden/baselines/**` не тронут диффом (подтверждено `git log` — пусто), что согласуется, а не противоречит | +| **M2** — AST/choke-point guard уже, чем заявлено в AC5/`TESTING.md` | Добавлены два новых именованных мутанта: `wall-identity-history-restore-barrier-bypassed` (Undo/Redo restore) и `wall-identity-optimize-barrier-bypassed` (`plan-optimizer.ts`), плюс переименован исходный в `wall-identity-editor-commit-barrier-bypassed` — все три известных writer-пути теперь поимённо покрыты | `node scripts/mutation-gate.mjs --check` → 226/226 `ok`, включая все три `wall-identity-*` мутанта (перепрогнано мной) | +| **M3** — `wall_model_client_outdated` не срабатывает на реалистичном echo-сценарии | `validate_wall_model_transition` (`custom_components/houseplan/validation.py:185-217`) получил новую ветку `old_model >= 8 and new_model >= 8 and geometry_changed`, сверяющую `_wall_catalog_projection` (только `wall_segments`) — расхождение поднимает `WallModelClientOutdatedError` с текстом «unchanged wall catalogue» | Новый тест `tests_backend/test_wall_segment_model.py::test_stale_client_echoing_v8_catalog_gets_the_named_error` — прочитан, логика соответствует описанному сценарию; **не исполнен** (см. §6, нет `homeassistant`/`voluptuous` в этой среде) — проверено чтением, не исполнением | +| **M4** — регрессионный тест #248 ослаблен без обновления фикстуры | `test/fixtures/optimize-storage-roundtrip.json` обновлена под полный v8-выход, `test/plan-optimizer.test.mjs:366` вернул строгий `assert.deepEqual(first.config, storageRoundtripFixture.expected.config)` вместо частичного сравнения только `poly` | Тест прогнан мной в составе `npm test` — проходит (часть 1329 passed); дифф `test/plan-optimizer.test.mjs` (`9fc213c6..HEAD`) показывает возврат полного `deepEqual` | + +Все шесть находок r1 (H1, H2, M1-M4) закрыты предметно, не декларативно. Ни +одна из них не осталась открытой или не переоткрылась при моей перепроверке. + +## 6. Унаследовано из r1 + +Из полного 62-файлового диффа r1 (`git diff origin/dev...5adfdb5d` — уже +недоступен напрямую как SHA, но содержательно эквивалентен диапазону, который +описывает `docs/reviews/CODE-REVIEW-282-r1.md`), эта дельта не касается и +поэтому **не переразобрано**, а принято на основании выводов r1 (документ +`CODE-REVIEW-282-r1.md`, SHA на котором получен вывод — `5adfdb5d`/содержательно +`466a6765` после ребейза): + +- **Migration determinism/idempotence** (`deterministicWallSegmentId`, + суффиксы при коллизии) — r1 §5, не задето дельтой (`test/wall-segment-model.test.mjs` + дельта затрагивает только новые тест-кейсы, не эту логику). +- **Backend-зеркало валидации** — unique-id, owner-count, dangling refs, + edge-geometry parity кроме нового `_wall_catalog_projection`-блока (см. §5 + M3) — r1 §5, файл `wall_segment_model.py` в дельте затронут (+48/-х строк), + но это тот же lineage-код, что и H1/H2-фикс на фронтенде; backend-инварианты + (owner-count/dangling refs) не изменены текстуально в дельте. +- **Import/export remap** (`import_export.py`, `secrets.token_hex(4)`) — не в + дельте (`git diff --stat 9fc213c6..HEAD` не содержит `import_export.py`), + наследуется из r1 §5 без переразбора. +- **i18n-ключи** (`toast.wall_model_migration_blocked`, + `toast.wall_model_client_outdated`, `gs.wall_segments_migrated`, + `wall_model.reason.*`) — не в дельте, наследуется из r1 §5. +- **Документация** (`WALL-THICKNESS.md`, `CANVAS.md`, `USER-GUIDE.{ru,}.md` — + кроме `TESTING.md`, который в дельте, см. §5 M2) — наследуется из r1 §5. +- **`config-field-registry.mjs`** — не в дельте, наследуется. +- **Трейлеры и changelog структуры коммитов r1** — наследуется; трейлеры + каждого нового коммита дельты проверены заново мной отдельно (см. §7). + +Обоснование, почему это не требует переразбора несмотря на общий полный охват +раунда (§1): дельта физически не касается перечисленных файлов +(`git diff --stat 9fc213c6..HEAD`, приведён в §1), а находки этого раунда (H1, +H2) обнаружены и локализованы именно в затронутых дельтой файлах +(`test/open-passage-contract.test.mjs` тестирует код `websocket_api.py`, +который в дельте; `space-render.ts` хоть и не в дельте, но находка H2 — +следствие данных, которые генерирует именно delta-код `wall-segment-model.ts`, +т.е. вызвана дельтой, а не унаследованным кодом). + +## 7. Что проверено и корректно + +- **Трейлеры** — все 6 коммитов дельты (`c694f6ea`, `41d7a63f`, `c24792b6`, + `35fce64a`, `38b03549`, `3588284f`) несут `Issue: #282`; четыре + `User-Visible: yes` (`c694f6ea`, `35fce64a`, `38b03549`, `3588284f`) — + каждый правит оба changelog в том же коммите (проверено `git show --stat` + на каждом). +- **Backend-барьер Optimize** (`35fce64a`) — `ws_plan_optimize` теперь ведёт + тот же v7→v8-барьер, что и frontend `_commitPhysicalGeometry` + (`commit_wall_segment_model` + `too_large`-проверка после миграции + + `WallSegmentMigrationError` в списке перехватываемых) — прочитано полностью, + логика симметрична фронтенду. +- **Guard-переупорядочивание #278** (H1(r1)) — `legacySafe`-проверка на + `liveCandidate` до барьера, restore на отказ — прочитано и подтверждено + прогоном (см. §5). +- **Lineage hints** (`fixedTopologyWallLineageHints`, + `adoptWallSegmentModelCandidateInPlace`) — прочитаны; логика различения + `preferredIds`/`positionalIds` через `collinearOverlap` (защита от + «застарелого ординала» при сплите) выглядит осмысленной и подтверждена всем + доступным smoke/mutation набором. +- **Off-grid guard с authored path** (`38b03549`) — `wallModelOffGridValueCount` + теперь учитывает `_path` (активная рисуемая цепочка) как базовую линию + роста, не как новое отклонение — юнит-тест на это добавлен и проходит. +- **Три мутанта #282** (editor-commit/history-restore/optimize barrier) — + реально пойманы (§2, §5 M2). +- **`npm run invariants`-эквивалент на реальной нагрузке** — прогнан на + `demo/fixtures/large-house.mjs` (3 пространства, 1788 записей по прошлым + измерениям Stage 0) до и после `commitWallSegmentModel`: ссылки разрешимы, + записи толщины находятся, 0% шума у узла. +- **35 из 36 запущенных смоков** зелёные (см. §2, §4). + +## 8. Чего не проверял и почему + +- **`pytest tests_backend`, даже pure-subset** — среда исполнения не содержит + `homeassistant`, а `custom_components/houseplan/__init__.py` безусловно + импортирует `homeassistant.components.frontend`, поэтому даже + не-`test_ha_*.py` модули (`test_validation.py`, `test_wall_segment_model.py`) + не собираются без полного HA-пакета — это не селективный пропуск + `conftest.py`, а полный отказ коллекции. `.venv-backend` в этой среде не + существует (в отличие от cloud-agent окружения, описанного в `AGENTS.md`). + M3 (§5) и общая корректность `validation.py`/`wall_segment_model.py`-диффа + проверены **чтением**, не исполнением — честно назвать это ограничением, не + «verified». +- **Полный `performance_smoke`** — пре-релизный гейт (§8); целевой + `benchmark:wall-model` дельтой не затронут (файл вне диффа), поэтому не + перепрогонялся отдельно в этом раунде — цифры из r1/хендоффа автора (167-284 + мс на p95 при бюджете 500 мс) остаются в силе, так как измеряемый код не в + дельте. +- **7 «слабая связь» смоков** (`smoke_align_guides`, `smoke_card_tool_conflict`, + `smoke_editor_gestures`, `smoke_esc_dialogs`, `smoke_island_rooms`, + `smoke_optional_space_model`, `smoke_pan_any_zoom` — связь только по + `_path`) — не прогонялись: методология идентична r1 (§6 её документа) — уже + найденных блокирующих находок достаточно для возврата, и один из 36 более + сильно связанных смоков (`open_passage`) уже красный. Автору стоит + прогнать все 7 вместе с полным набором перед следующей сдачей. +- **Глубокий line-by-line аудит `buildAtoms`/lineage-логики + (`src/wall-segment-model.ts`, весь диапазон 218 изменённых строк)** — не + выполнен formal-proof, только чтение ключевых веток плюс полное + black-box-покрытие (36 смоков, 226 мутантов, 129 golden, полный `npm test` + кроме H1). Смок `smoke_open_passage.mjs` — прямое доказательство, что + black-box-покрытие не исчерпывающее; глубже эту логику не разбирал, так как + найденных H1/H2 достаточно, чтобы вернуть задачу автору (тот же принцип, что + в r1 §3 «проверять глубже уже не требуется по методологии»). + +## 9. Резюме + +Красный вердикт, второй подряд, но по существу другим находкам, чем r1: все +шесть находок r1 (2 High + 4 Medium) закрыты предметно и перепроверены +исполнением (кроме M3 — чтением, backend-харнесс недоступен). Однако именно +дельта, закрывшая r1, внесла две новые блокирующие находки: + +- **H1** — `npm test` красный прямо сейчас: тест на количество вызовов + `validate_opening_passages(` не обновлён под намеренно добавленный третий + вызов из последнего коммита дельты. Механически тривиально, но нарушает + обязательный на каждом раунде гейт. +- **H2** — нарушение собственного AC7 задачи: `houseplan-space-card` теряет + **любой** wall-hosted opening (не только passage) после миграции + пространства в v8, потому что `src/space-render.ts` — единственный + нетронутый диффом #282 потребитель `opening.host` — не различает + `host.kind === 'wall'` от `host.kind === 'partition'` и молча роняет + нерезолвленные openings из рендера. Реальный blast radius на + документированной поверхности продукта, конфликтующий с J1/J2 + `docs/SCOPE.md`. + +Задача возвращается в «В разработке». Оба фикса локальны и не должны требовать +повторного полного разбора всей подсистемы в r3, если дельта останется +локальной к `websocket_api.py`-тесту и `space-render.ts`; но `space-render.ts` +до сих пор не в диффе #282 вовсе — стоит проверить на этом же файле и другие +пути, потребляющие `opening.host` (hidden-isometric, если он существует +отдельно), а не только `houseplan-space-card`.