diff --git a/docs/reviews/CODE-REVIEW-282-r1.md b/docs/reviews/CODE-REVIEW-282-r1.md new file mode 100644 index 00000000..d6575ed2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-282-r1.md @@ -0,0 +1,305 @@ +# CODE-REVIEW-282-r1 + +- **Issue:** #282 — стабильная идентичность сегментов стен (ADR Stage 1) +- **Ветка:** `issue/282-wall-geometry-model`, HEAD `5adfdb5d` (после ребейза на `origin/dev` `f2460dc8`) +- **ТЗ:** `docs/specs/282-stable-wall-segment-identity.md` (spec-review зелёный на r2, `2f30c481`) +- **Этап:** код-ревью (PROCESS.md §2.7) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 до этого вердикта +- **Вердикт:** **красный** + +## 1. Скоуп проверки + +Диапазон: `git diff origin/dev...HEAD` (62 файла, +3641/−337), т.е. весь рабочий цикл +Stage 1 — persisted `wall_segments[]` catalog, `rooms[].wall_ids[]`, tagged +opening host, единый identity barrier `commitWallSegmentModel`/ +`commitWallSegmentModelInPlace`, backend-зеркало (`wall_segment_model.py`, +`validation.py`), миграция v7→v8, compatibility-проекция `walls[]`, документация, +i18n, тесты (unit/backend/invariants/benchmark/mutation-gate/golden). + +Это не локальная правка — это ревью полной первой реализации основного этапа +задачи, поэтому разбор велся полным объёмом (PROCESS.md §2.10: «разбор остаётся +полным, если ... задета новая подсистема» — здесь затронута каждая структурная +операция геометрии стен). Раздел «Унаследовано из r(N−1)» не нужен: это первый +заход код-ревью. + +## 2. Как проверялось — таблица гейтов + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный | +| Unit/frontend | `npm test` | 1321 тестов, 1320 passed, 1 skipped, 0 failed | +| Build + sync bandle | `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 run invariants -- --config ` | зелёный на pre-v8 и post-v8 конфиге; расширенные v8-проверки (`room_wall_ids`, `opening_host→wall`) в `scripts/model-invariants.mjs` реально добавлены и сработали | +| Migration benchmark | `npm run benchmark:wall-model` | зелёный: 10 000 атомов, p95 208–284 мс (бюджет 500 мс), `passed:true` | +| Smoke selection | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 29 прямых совпадений, 6 зарегистрированных связей, 7 слабых, из 190 всего — вывод приложен ниже (§4) | +| Golden | `npm run golden:verify` | **красный**: 2 из ~140 сценариев — `optimize-orphan-references-dark-en`, `optimize-orphan-references-light-ru` (diffRatio 0.085/0.094 при пороге 0.0008) | +| Named/registered smokes | 6 «зарегистрированная связь» + 28 «прямое совпадение» (все, кроме уже покрытых) | **2 красных из 6** зарегистрированных, **7 красных из 28** прямых — см. §4 | +| Backend pure | не прогонялся мной (см. §6, «чего не проверял») | author указывает 146 passed/1 skipped локально на Windows | +| `pytest tests_backend` (полный, Linux) | не прогонялся | пре-релизный гейт, вне объёма код-ревью (§8) | +| Performance-профили (full performance_smoke) | не прогонялся | пре-релизный гейт | + +Дешёвые гейты все зелёные. Расширенные (invariants, benchmark, smoke-select, +golden, целевые smoke) прогнаны, потому что diff меняет геометрию/ссылки на неё +и видимый результат (AC15 добавляет строку в диалог Optimize) — именно то +сочетание, для которого PROCESS.md §8 требует их «по необходимости». Именно эти +прогоны, а не дешёвые гейты, и нашли блокирующие находки ниже. + +## 3. High — блокирующие находки + +### H1. #278-guard «отклонить правку деградированного пространства» больше не срабатывает + +**Файл:** `demo/smoke_wall_union_isolation.mjs` (не тронут этим диффом — 0 строк в +`git diff --stat origin/dev...HEAD`), корень вероятно в +`src/houseplan-card.ts:7261-7307` (`_commitPhysicalGeometry`). + +**Воспроизведение:** тест не изменён этим диффом. На `origin/dev` (f2460dc8, +собран отдельным git-worktree, тот же `npm run build && npm run bundle:sync`): + +``` +node demo/smoke_wall_union_isolation.mjs → OK (все 11 проверок true) +``` + +На HEAD (`5adfdb5d`), дважды подряд, детерминированно: + +``` +"degradedPhysicalEditRejected": false (ожидалось true) +"rejectedEditHasNoHistoryOrWrite": false (ожидалось true) +"rejectedEditHasLocalizedToast": false (ожидалось true) +FAILED (3) +``` + +Сценарий смока (issue #278): на фикстуре с уже деградированным canonical-union +(`status: 'degraded-extra'`) прямая правка толщины стены (`walls[0].cm += 1`) +должна быть отклонена `_checkSpacePhysicalGeometry` без записи в историю и без +WS-записи, с тостом `toast.geometry_unsafe`. Сейчас `writes !== 0` и/или +`_geometryHistory.size !== 0` — то есть **правка прошла и была сохранена**, хотя +пространство остаётся геометрически некорректным (то самое, что #278 явно +запрещает). + +Это прямое попадание в риск, названный самим ТЗ (§16): «Новая identity +используется для удаления старых geometry guards» — и в AC12: «Точный ID не +является поводом удалить существующую union isolation либо ослабить geometry +preflight». Похоже, что перестройка `wall_segments`/`walls[]` внутри +`commitWallSegmentModel` (переписывает `space.walls` целиком из catalog, +`wall-segment-model.ts:482-486`) меняет вход, который видит +`_checkSpacePhysicalGeometry` в `_commitPhysicalGeometry` (строка 7287), и старая +деградированная конфигурация после прохождения через барьер перестаёт +распознаваться как небезопасная — либо fingerprint-guard на строках 7279-7284 +("no-op" ветка) срабатывает раньше, чем должен. Это не разобрано до конца — это +находка «оно не работает», а не диагноз; автору нужно трассировать +`_commitPhysicalGeometry` на этой фикстуре. + +**Доказательство:** тест умеет падать (см. воспроизведение — падает на HEAD, +проходит на dev с байт-идентичным файлом теста). + +### H2. Широкая регрессия структурных операций редактирования + +**Файлы:** восемь именованных demo-smoke, ни один не тронут этим диффом +(`git diff --stat` подтверждён для каждого — 0 строк). + +Все восемь **зелёные на `origin/dev`** (проверено тем же способом — отдельный +git-worktree на `f2460dc8`, идентичный собранный бандл) и **красные на HEAD**, +детерминированно (перепроверено вторым прогоном для двух из них): + +| Smoke | Упавшая проверка | Что означает | +|---|---|---| +| `smoke_resize_pointer_real_plan.mjs` | `both_rooms_commit_ten_steps`, `wall_metadata_preserved` (24 записи толщины → 29), `undo_byte_exact` | Resize на реальном 8-комнатном плане плодит лишние атомы вместо чистого частичного split; Undo **не** восстанавливает byte-exact исходный конфиг — возвращает раздутый v8-catalog вместо исходного v7-документа | +| `smoke_wall_chain_thickness.mjs` | `legacyResumedFullVector` | продолжение (resume) ранее сохранённой цепочки стен теряет полный вектор толщины | +| `smoke_unified_wall_tool.mjs` | `cleanSplitRetainsParentMetadata`, `cleanSplitConsumesDivider` | «чистый» split цепочки не сохраняет метаданные родителя и не поглощает divider — то самое split-lineage поведение, которое AC4 требует доказать | +| `smoke_wall_face_overlap.mjs` | `partialOverlapKeepsRoomsUntilDecision` | частичное перекрытие граней не удерживает комнаты до явного решения пользователя | +| `smoke_edit_walk.mjs` | `walk_first_floor_seed1/2/3` (все три) | fuzz-walk (случайные ресайзы) на фикстуре `real-plan-first-floor.json` расходится с ожидаемым инвариантом на каждом из 3 сидов; `real-plan-second-floor.json` не задет | +| `smoke_editor_tabs.mjs` | `tabCrossLimitKeepsDraftWithFeedback` | лимит на cross-стены при рисовании перестал сохранять черновик с обратной связью | +| `smoke_plan_snap_overlay.mjs` | `openingGapDoesNotActivateSnap` | снап-оверлей активируется там, где не должен (зазор проёма) | +| `smoke_partition_openings.mjs` | `tooShortPartitionHasNoPreviewAndExplainsWhy`, `hostBodyHasFullDepthOpeningGap`, `rigidHostMoveKeepsTAndMovesProjectionAtomically` | предпросмотр слишком короткой перегородки, полнота глубины проёма и атомарный перенос host+projection при жёстком перемещении — все три ломаются | + +**Доказательство:** все девять (H1 + 8 из этой таблицы) — тесты, которые умеют +падать: зафиксирована пара «зелёный на dev / красный на HEAD» на байт-идентичном +файле теста для каждого. Это не флуктуация — `wall_union_isolation` и +`partition_openings` перепроверены повторным прогоном с тем же результатом. + +Из 34 релевантных по `smoke-select.mjs` смоков (6 «зарегистрированная связь» + 28 +«прямое совпадение», кроме уже покрытых в реестре), не прогнаны только 7 «слабая +связь, одно распространённое имя» (`_path`) — они не были приоритетными на фоне +уже найденных 9 красных, но и не гарантированно зелёные; см. §6. + +**Итог по H1+H2:** это прямой ответ на обязанность ревьюера «оно вообще +работает» (PROCESS.md §2.7). Ответ — нет, не в текущем виде. AC3 (identity не +зависит от геометрии), AC4 (split/merge lineage), AC6 (opening host), AC8 +(read-only/atomic failure), AC12 (regression floor) заявлены как доказанные +именно этими и смежными смоками/автотестами, но конкретно эти девять +доказательства не проходят на предъявленном коде. Обе находки блокируют +(High), проверять глубже уже не требуется по методологии — предъявленных +детерминированных воспроизведений достаточно, чтобы вернуть задачу автору. + +## 4. Medium — по существу задачи (чинятся в этом же issue, #202) + +Ниже сохранены для одного цикла ревью: даже после устранения H1/H2 эти находки +останутся, и лучше не резать возврат на несколько дублирующих раундов. + +### M1. AC15 добавил новую строку в Optimize-диалог — golden baseline не обновлён + +`npm run golden:verify` (не тронут этим диффом: `demo/golden/matrix.mjs` и +`harness.mjs` — 0 строк в diff) красен на двух существующих сценариях: +`optimize-orphan-references-dark-en` (diffRatio 0.0845 при пороге 0.0008), +`optimize-orphan-references-light-ru` (0.0940). Разница — ровно новая строка +диалога «Wall segments stabilised: 25.» / «Стабилизировано сегментов стен: 25.» +(AC15, `gs.wall_segments_migrated`), подтверждено визуально по +`artifacts/golden/diff/optimize-orphan-references-dark-en.png` и `actual/…png`. +Изменение видимого результата — ожидаемое и правильное, но §18 ТЗ и PROCESS.md +§3.13 требуют принятия нового эталона только через +`npm run golden:accept -- --reviewed` на полном Linux CI-артефакте; сейчас +`demo/golden/baselines/**` не тронут диффом вовсе. + +### M2. AC5 «executable source/AST guard + bypass mutant for each writer family» реализован уже, чем заявлено + +`test/wall-segment-model.test.mjs:191-205` — не AST, а построчный `indexOf`/regex +по двум именованным методам (`_commitPhysicalGeometry`, `_applyGeometryState`), +проверяющий, что каждый содержит нужный вызов. Это разумный choke-point тест, +но не «перечисление всех входов записи», как заявлено в AC5 и повторено в +`docs/TESTING.md` («Structural writers are enumerated by +`scripts/mutation-gate.mjs`»). В самом `scripts/mutation-gate.mjs` (строки +2875-2887) — ровно один мутант, откатывающий вызов в +`_commitPhysicalGeometry`; для вызова в `_applyGeometryState` (Undo/Redo restore, +строка ~7397 диффа) и для вызова в `src/plan-optimizer.ts:656` +(`commitWallSegmentModelInPlace` внутри `optimizePlans`) — **никакого мутанта +нет**. Гипотетический новый метод-писатель, минующий оба известных choke-point и +вызывающий `_writeConfig` напрямую, не будет пойман ни тестом, ни +mutation-gate — то есть именно тот сценарий, который явно называет AC5 +(«новый путь записывает … партиции … мимо barrier»), не покрыт механически. +Формулировку в `docs/TESTING.md` и/или объём guard нужно привести в +соответствие. + +### M3. `wall_model_client_outdated` не срабатывает в реалистичном сценарии старого клиента + +`validate_wall_model_transition` (`custom_components/houseplan/validation.py:138-155`) +поднимает `WallModelClientOutdatedError` только когда `old_model >= 8 and +new_model < 8` — то есть только если клиент явно откатил `model_version`. Но §10.2 +ТЗ описывает другой, более реалистичный случай: старый клиент **сохраняет** +`model_version: 8` (просто эхом), не трогает `wall_segments`/`wall_ids`, но +меняет `poly`/`walls`/openings. Этот случай ловит независимая +`_config_wall_segment_invariants` (validation.py:1718-1808, часть +`CONFIG_SCHEMA`, применяется декоратором `websocket_command` **до** тела +`ws_config_set`) — но она поднимает обычный `vol.Invalid`, не +`WallModelClientOutdatedError`, поэтому код ошибки не `wall_model_client_outdated`. +Фронтенд показывает дружелюбный тост только по точному совпадению кода +(`src/houseplan-card.ts:7499`, `:15746`: `e?.code === 'wall_model_client_outdated'`). +Итог: частичная запись всё ещё не проходит (безопасность данных сохраняется), но +пользователь вместо «обновите карточку и перезагрузите страницу» увидит общую +ошибку схемы. `tests_backend/test_wall_segment_model.py:145-168` покрывает только +узкий explicit-downgrade вариант, не этот. + +### M4. Регрессионный тест issue #248 ослаблен без обновления фикстуры + +`test/plan-optimizer.test.mjs`, тест «issue 248 Optimize stays a no-op across the +lattice storage round-trip» (диапазон AC12: #197/#224/#249/…/#288-302 включает +идемпотентность Optimize): + +```diff +- assert.deepEqual(first.config, storageRoundtripFixture.expected.config); ++ assert.equal(first.config.model_version, PLAN_MODEL_VERSION); ++ assert.ok(first.config.spaces.every((space) => Array.isArray(space.wall_segments))); ++ assert.deepEqual( ++ first.config.spaces.map((space) => space.rooms.map((room) => room.poly)), ++ storageRoundtripFixture.expected.config.spaces.map((space) => ( ++ space.rooms.map((room) => room.poly) ++ )), ++ ); +``` + +`test/fixtures/optimize-storage-roundtrip.json` не тронут этим диффом (0 строк) — +его `expected.config` остался в v7-форме и не отражает реальный v8-выход, поэтому +полное побайтовое сравнение стало технически невозможным без правки фикстуры и +было заменено сравнением только `rooms[].poly`. Побочный эффект: тест больше не +проверяет byte-parity `wall_segments`/`wall_ids`/`walls[]`-проекции для этого +именованного регрессионного сценария — то самое ослабление, которое AC12 +запрещает как побочный эффект. + +## 5. Что проверено и корректно + +- **Migration determinism/idempotence** — `test/wall-segment-model.test.mjs` + (детерминированный hash-seed, суффиксы `-2`/`-3` при коллизии, идемпотентность + повторного `commitWallSegmentModel`) читал и прогонял; тесты реальные (проверял + `deterministicWallSegmentId` вручную по независимо вычисленному + SHA-256/base32) — совпадает. +- **Backend-зеркало валидации** (`_config_wall_segment_invariants`, + `validation.py:1718-1808`) — независимая от фронтенда проверка unique-id, + owner-count 1/2, dangling refs, edge-geometry parity, `walls[]`↔`wall_segments` + parity, opening host fit/overlap; прочитано полностью, соответствует AC2/AC11. +- **Import/export remap** (`import_export.py`, `build_space_merge`) — remap всех + id идёт через `secrets.token_hex(4)`, никогда через геометрическое совпадение; + соответствует AC10 и явному запрету ТЗ («collision с target никогда не + resolve-ится геометрическим совпадением»). +- **i18n** — новые ключи `toast.wall_model_migration_blocked`, + `toast.wall_model_client_outdated`, `gs.wall_segments_migrated`, + `wall_model.reason.*` присутствуют и совпадают в en/ru, с одинаковыми + плейсхолдерами. +- **Документация** — `WALL-THICKNESS.md`, `ARCHITECTURE.md`, + `CONFIG-COMPATIBILITY.md`, `CANVAS.md`, `TESTING.md`, `USER-GUIDE.{ru,}.md` + обновлены содержательно и точно описывают новую модель; + `USER-GUIDE.ru.md`/`.md` используют «стабильный внутренний идентификатор» только + в контексте объяснения автоматического обновления старого плана — соответствует + исключению AC16, не течёт в обычный UI. +- **`config-field-registry.mjs`** — `wall_segments`, `rooms[].wall_ids`, + `room_drafts[].segments[].id`, `openings[].host=wall` зарегистрированы как + `current` v8, с описанной миграцией/совместимостью. +- **Performance** — `npm run benchmark:wall-model` реальный (не no-op, + синтетическая геометрия 10 000 атомов), p95 208-284 мс < 500 мс бюджета. +- **Единственный источник числа** — толщина теперь имеет один источник + (`wall_segments[].cm`), `walls[]` — регенерируемая проекция, backend проверяет + их точное соответствие построчно (validation.py:1752-1766); `test/single-source-numbers.test.mjs` + не тронут этим диффом и продолжает покрывать форматирование. +- **Трейлеры** — `Issue: #282`, `User-Visible: yes` на коммите реализации; оба + changelog (RU+EN) правлены в этом же коммите с содержательным описанием + пользовательского эффекта (без обещания нового UI). + +## 6. Чего не проверял и почему + +- **Полный `pytest tests_backend` на Linux/CI** — недоступен в этой среде + (`homeassistant` не установлен, только pure-subset потенциально прогоняем; + чтение кода вместо исполнения для backend-веток `test_ha_import_export.py`, + отмечено как «проверено чтением» в переписке с фоновыми агентами, не + исполнением). Это канонически пре-релизный гейт (PROCESS.md §8), но + AC9/AC10 частично опираются именно на HA-harness тесты. +- **`performance_smoke` (полный)** — пре-релизный гейт, не гейт ревью; AC14 + проверен целевым бенчмарком (см. §5), это достаточно для код-ревью объёма. +- **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`) — + не прогонялись: после того как 9 из 34 более сильно связанных смоков оказались + красными, находка уже достаточна, чтобы вернуть задачу автору; дальнейшая + трассировка именно этих семи не добавила бы новой информации для вердикта. + Автору стоит прогнать их вместе с полным набором перед повторной сдачей. +- **Golden — полный набор** (~140 сценариев) — прогнан полностью (не частично), + результат в §2/§3.M1; это уже полный прогон, не выборка. +- **`npm run docs:accept`/скриншоты** — не пересъёмка, `check-docs.mjs` зелёный, + этого достаточно; коммит `5adfdb5d` уже содержит обновлённые PNG после + ребейза, это не предмет этого ревью. +- **Корневая причина H1/H2** не диагностирована до конкретной строки — воспроиз­ + ведение и локализация («какой смок, какая проверка, зелёный на dev/красный на + HEAD») предоставлены; дальнейшая трассировка — работа автора, не ревьюера + (ревьюер не правит продуктовый код). + +## 7. Резюме + +Красный вердикт: две категории High-находок (H1 — байпас guard #278 на +деградированном пространстве; H2 — детерминированная регрессия в восьми +именованных структурных smoke-тестах: Resize+Undo на реальном плане, wall-chain +resume, unified-wall split lineage, wall-face overlap, edit-walk fuzz на +first-floor фикстуре, editor-tabs cross-limit, plan-snap-overlay, partition +openings), плюс четыре находки Medium в скоупе задачи (устаревший golden +baseline для AC15, недокументированно узкий AC5-guard, необрабатываемый +`wall_model_client_outdated` для реалистичного старого клиента, ослабленный +без обновления фикстуры регрессионный тест #248). + +Дешёвые гейты (`typecheck`/`test`/`build`/`check-docs`/`invariants`/ +`benchmark`) зелёные — модель миграции, backend-валидация, i18n и документация +сами по себе выполнены методологически аккуратно. Но именно те проверки, которые +PROCESS.md §8 требует «по необходимости, определяемой diff'ом» (golden, +целевые browser-smoke), обнаружили, что барьер `commitWallSegmentModel`, +подключённый в существующие структурные пути редактирования, ломает их — +включая явный safety-guard issue #278 и Undo-байт-точность. Задача возвращается +в «В разработке»; после исправления H1/H2 нужен новый заход код-ревью (не только +повторная проверка находок — весь набор smoke, зафиксированный здесь как +красный, должен стать зелёным, и полный regression floor AC12 нужно +перепроверить, поскольку правка H1/H2, скорее всего, коснётся самого барьера).