diff --git a/docs/reviews/CODE-REVIEW-566-r1.md b/docs/reviews/CODE-REVIEW-566-r1.md new file mode 100644 index 00000000..b849a5c2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-566-r1.md @@ -0,0 +1,201 @@ +# CODE-REVIEW — issue #566, заход r1 + +**Материал:** `f724cca61a9822d9e5780f0e171137f3c2c7fc40` (ветка +`issue/566-stale-layout-notes`, три коммита: `dedcedc8`, `7ecdafef`, `f724cca6`). +Рабочая копия уже стояла на этом SHA, `git fetch`/`checkout` не выполнялись. + +**Трек:** инфраструктурный (§1) — тронуты только `scripts/model-invariants.mjs`, +`scripts/mutation-registry.mjs`, `test/model-invariants.test.mjs`, +`test/geometry-corpus.test.mjs`. Ни одного файла класса A, ускоренный вход +подтверждён. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 1 (в скоупе)** + +## Скоуп + +Issue #566: `checkReferences` в `scripts/model-invariants.mjs` объявлял +нарушением `references/layout_space` любую позицию `layout`, чьё пространство +удалено — даже когда владелец позиции (комната/область/маркер) жив и Optimize +(`src/space-reference-repair.ts`) намеренно хранит запись. Задача сужает +правило: нарушение — только когда владельца тоже нет по конфигурации; +остальное — наблюдение `stale_layout_space`. + +Коммиты `7ecdafef` и `f724cca6` — не часть замысла #566 напрямую, а реанимация +трёх мутантов в `src/wall-thickness.ts`/`src/plan-optimizer.ts`, которых +затянула в отбор правка `mutation-registry.mjs`, и которые ломались на этапе +компиляции (детально разобрано в комментариях автора, заходы 2 и 3). Отдельный +пробел («мутант, падающий на компиляции, неотличим от рабочего, пока его не +выберет дифф») заведён автором как #568 — само собой не проверяется в этом +ревью, я лишь отмечаю, что проверенное здесь не расширяет скоуп #566 +неоправданно: правка лежит в файле реестра, который #566 и так меняет. + +## Как проверялось + +Дешёвые гейты подтверждены Validate на этом SHA (run 34788430876, ссылка в +issue) — `tsc --noEmit`, `npm test`, `npm run build` не перегонялись. Сверх +этого, для защитных AC и для гейта `invariants`, обязательного при правке +`layout`-ссылок, прогнано отдельно в песочнице ревью: + +| Гейт | Команда | Результат | +|---|---|---| +| Мутант «обвинить всех» | `node scripts/mutation-gate.mjs --id=invariants-blame-every-stale-position` | поймано 1 из 1 | +| Мутант «простить пропавшего» | `node scripts/mutation-gate.mjs --id=invariants-forgive-a-vanished-position-owner` | поймано 1 из 1 | +| 3 реанимированных мутанта (#568) | `--id=inner-span-reads-whole-edge-thickness`, `--id=safe-resize-legacy-midpoint-fail-open`, `--id=optimizer-micro-interval-cleanup-disabled` | все «поймано 1 из 1» | +| Структура реестра | `node scripts/mutation-gate.mjs --check` | exit 0, 732 записи `ok`, совпадает с заявленным | +| `npm run invariants -- --config test/fixtures/560-corpus/c6-stale-layout.json` | CLI end-to-end на реальной фикстуре #560 | exit 0, «Инварианты выполнены… Наблюдений (не нарушения): 2» | +| Выбор смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет», смоки не выбираются — `src/**` не тронут | + +`check-docs`, golden, backend pytest, performance-профили не запускались: +`src/**` не тронут, рендер и питон-бэкенд не задеты, ничего перфочувствительного +в диффе нет. Это назывался объём необходимого самим диффом, а не пропуск. + +## Находки + +### Medium (в скоупе) — «наблюдение» ложно утверждает «владелец жив» там, где владелец не подтверждён + +`scripts/model-invariants.mjs:213-231`. Новая ветка судит владельца позиции на +удалённом пространстве по трём случаям (`rl_`/`grp_`/маркер), но только для +`rl_` и `grp_` реально ДОКАЗЫВАЕТ существование владельца по конфигурации +(`allRoomIds`/`allAreas` — множества по всем пространствам, факт, а не догадка). +Для остальных ключей (нет `rl_`/`grp_` — это владелец-маркер или ключ +устройства HA) код проверяет только `removedMarkerIds.has(key)`. Если ключ не +в `removedMarkerIds`, это накрывает ДВА разных случая: (а) ключ — живой маркер +(подтверждено конфигом) и (б) ключ вообще не найден ни в каком маркере — +ровно тот случай, для которого пятью строками ниже, в ветке `unknown_owner` +(живое пространство), уже есть честная формулировка «владелец не найден в +конфигурации (возможно устройство HA)». Для мёртвого пространства оба случая +(а) и (б) получают одинаковый `detail`: **«владелец жив — Optimize хранит +позицию намеренно»** — то есть утверждение о доказанности там, где +доказательств нет, ровно тот стиль ложного сигнала, который сама задача +исправляет для другого случая (#566 body: «инструмент… отвечает "нет" по +причине, которую положено игнорировать»; тот же принцип работает в обратную +сторону — не утверждать то, чего не знаешь). + +Продукт (`src/space-reference-repair.ts:329-388`) для этого же случая ведёт +ТРИ состояния, не два: `live` (маркер активен), `absent` (маркер снят), +`unverified` (владелец не резолвится вообще — `reason: 'unknown_owner'` или +`'registry_unavailable'`). Автор в комментарии к задаче прямо ссылается на эту +трёхчастную модель как на источник правды («Значит дефект целиком на стороне +инструмента… для мёртвого пространства это рассуждение просто не +применялось»), но новая реализация воспроизводит только грань +absent/не-absent, а внутри «не-absent» смешивает `live` и `unverified` под +одной надписью. + +**Воспроизведено исполнением** (импорт функции напрямую, не мутация): + +```js +const config = { + spaces: [{ id: 'sp1', rooms: [{ id: 'r1', area: 'kitchen' }] }], + markers: [{ id: 'm_live', removed: false }], +}; +const layout = { + 'unknown_device_id_1234': { s: 'dead_space' }, // никогда не был маркером, мёртвое пространство + 'unknown_device_id_5678': { s: 'sp1' }, // тот же владелец, живое пространство +}; +``` +Результат: `unknown_device_id_5678` (живое пространство) → `unknown_owner`, +detail «владелец не найден в конфигурации (возможно устройство HA)» — честно. +`unknown_device_id_1234` (мёртвое пространство) → `stale_layout_space`, detail +«пространства не существует, владелец жив — Optimize хранит позицию +намеренно» — то же самое неизвестное состояние владельца, но текст утверждает +обратное тому, что известно. + +Тест задачи (`test/model-invariants.test.mjs`, кейс с ключом +`'980f1446c4ec1a3a9fa9ff5f6d93caed'`) сам этот случай заводит и в комментарии +признаёт неразличимость («это устройство HA либо мусор, отличить нельзя»), но +проверяет только принадлежность к `kind: 'stale_layout_space'` и что ПЕРВАЯ (по +порядку ключей) запись из четырёх несёт нужный `detail` — этой первой +оказывается `rl_r1` (истинно живая комната), поэтому асимметрия тестом не +ловится. + +**Почему Medium, не High:** это не ломает AC issue (нарушения не заводятся, +код возврата CLI остаётся 0, дефект #566 закрыт) и не открывает регрессию в +существующем контракте — это неточность **внутри новой** функциональности, +которую сам диф вводит. Влияние — на доверие к тексту диагностики: человек, +читающий вывод `npm run invariants` на реальном конфиге (ровно сценарий этой +задачи), получит уверенное «владелец жив» для записи, чей владелец на самом +деле не установлен. + +**Правка в скоупе задачи** (#202, §2.7): различить `live` (маркер в +`activeMarkerIds`, множество уже вычислено в начале функции) от `unverified` +(ключ не резолвится ни как `rl_`/`grp_`/маркер) — вторым `kind` или полем +`reason`, как это уже сделано для `unknown_owner` в живом пространстве. + +### Проверено и не вызывает вопросов + +- **`rl_`/`grp_` классификация в мёртвом пространстве.** `allRoomIds`/`allAreas` + собираются по ВСЕМ пространствам (а не только исходному), что на первый + взгляд похоже на риск коллизии id между несвязанными комнатами/областями — + но это ровно тот же приём, что использует сам продукт: `existingRoomIds` в + `space-reference-repair.ts:148` строится идентично (по всем пространствам), + а `roomsByArea` (там же, строка 125) явно допускает несколько кандидатов на + одну область и намеренно не резолвит неоднозначность + (`uniqueAreaRoom` возвращает `null` при >1 кандидате). Комнатные id + генерируются как `` `r${Date.now().toString(36)}-${index}` `` + (`src/houseplan-editor-runtime.ts:6975`), так что практическая коллизия + ничтожно маловероятна. Отдельной находки не завожу: это соответствует + установленному в продукте поведению, а не новый риск диффа. +- **Мутанты #566** (`invariants-blame-every-stale-position`, + `invariants-forgive-a-vanished-position-owner`) — оба прогнаны лично, + «поймано 1 из 1» подтверждено, а не принято на слово. +- **Реанимация трёх мутантов** (#568-приём: статически мёртвая ветка теряет + сужение типов TS) — прогнаны лично, поведение мутанта не изменилось по + смыслу (условие по-прежнему ложно в рантайме), только перестало быть + статически недостижимым. +- **`mutation-gate --check`** — 732 записи, структура реестра (один `find` на + файл) не нарушена. +- **`npm run invariants` на корпусе #560** — сквозной CLI-прогон (не только + юнит-тест) подтверждает исход: 0 нарушений, 2 наблюдения с названной + причиной. +- **Трейлеры.** Все три коммита несут `Issue: #566` и `User-Visible: no` — + верно: изменение не видно пользователю, changelog не требуется. +- **Тест `test/geometry-corpus.test.mjs`** корректно расширен: `violations()` + теперь принимает `notes`, оба вызова (`checkReferences`, `checkWallKeys`) + прокидывают заранее существовавший параметр `{ notes }`, фикстура + `c6-stale-layout` заявляет и нарушения (`[]`), и наблюдения (два + `references/stale_layout_space`) — раньше наблюдения корпус не сверял вовсе. +- **`test/model-invariants.test.mjs`**, новый тест по семи записям на мёртвом + пространстве покрывает все три «нарушение»-случая (комната/область/снятый + маркер отсутствуют) и корректно требует, чтобы наблюдение называло причину + (`assert.match(..., /Optimize хранит позицию намеренно/)`) — сам этот assert, + впрочем, не различает «живой» и «неверифицированный» случаи (см. находку + выше). +- Прежний тест #252 пересмотрен честно (запись `ghost` перекласс­ифицирована + в наблюдение с объяснением), не подогнан молча. + +## Чего не проверял + +- `tsc --noEmit`, `npm test`, `npm run build` со сверкой бандла — не + перегонял: зелёный Validate на этом же SHA уже есть (run 34788430876), + перегон был бы тратой бюджета раунда без нового сигнала. +- `check-docs`, golden, `pytest tests_backend`, performance — не запускал: + `src/**` и `custom_components/**/*.py` не тронуты, рендер не задет. +- Полный отбор 94 мутантов, которые дифф затягивает в CI (`selectForDiff`) — + проверил только те 5, что относятся к содержанию этой задачи и её побочной + правке реестра; на остальные полагаюсь на зелёный Validate CI (шардированный + прогон), как и разрешает соразмерность гейта. +- Полевой конфиг владельца (упомянутый в issue, 4 записи на 2 пространствах) + недоступен ревьюеру — использовал синтетическую фикстуру `c6-stale-layout` + из репозитория, которую подтвердил как реальный CLI-прогон, а не только как + юнит-тест. + +## Что дальше + +Возврат автору (`S6-in-progress`): единственная Medium-находка в скоупе задачи +и решается в этом же issue — различить `live`/`unverified` в ветке +`stale_layout_space`, по образцу уже существующего `unknown_owner`. High нет, +цикл не исчерпан (0/4 израсходовано после этого раунда → 1/4). + +--- + + + +## Материал раунда + +- Ветка: `issue/566-stale-layout-notes`, коммит `f724cca61a98` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b05238054e4015f6f558b9c563d211d0749385e9` + ``` + git log --all --format='%H %T' | grep b05238054e40 + ``` +- Тело issue: `b30fafcfa79bb1bfe11625206ac445bb83a257f0b821202f064d1c4cfdcb2dd7` +- Вердикт конвейера: `yellow` · High 0