From b8c604306fa2e0a798920e30286ca88f2af2e8a5 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 08:06:20 +0000 Subject: [PATCH] docs: review document for #252 Issue: #252 User-Visible: no --- docs/reviews/CODE-REVIEW-252-r2.md | 365 ++++++++++++----------------- 1 file changed, 146 insertions(+), 219 deletions(-) diff --git a/docs/reviews/CODE-REVIEW-252-r2.md b/docs/reviews/CODE-REVIEW-252-r2.md index 65fe5aa4..664e3fe2 100644 --- a/docs/reviews/CODE-REVIEW-252-r2.md +++ b/docs/reviews/CODE-REVIEW-252-r2.md @@ -1,249 +1,176 @@ # CODE-REVIEW-252-r2 -Вердикт: зелёный · заход r2 · блокирующих циклов 0/4 · High: 0 · Medium: 0 +Вердикт: зелёный · заход r2 · блокирующих циклов израсходовано 0 из 4 · High: 0 · Medium: 0 -## Скоуп раунда и почему разбор полный, а не по дельте +## Скоуп раунда: почему это переиздание r2, а не новый разбор с нуля -Между CODE-REVIEW-252-r1 (зелёный, на поведенческом коммите `8fd8ccb`, -ветка ещё не ребейзнута) и этим раундом ветка была перебазирована на -`origin/dev` (комментарий автора 2026-08-23T07:33:24Z: `git rebase -origin/dev`, новая база `6d0fa3b`). Это прямо попадает под критерий -«ребейз на ушедший вперёд dev — после ребейза это другой код» (§7.2 -инструкции ревью), поэтому разбор в этом раунде — полный: весь диапазон -`git log --oneline origin/dev..HEAD` и `git diff origin/dev...HEAD` на -текущем `HEAD = b58136a`, а не только диф от r1. +Этот запуск — повторное проведение того же раунда r2, а не заход r3. Round r2 +уже был проведён и опубликован (комментарий issue от 2026-08-23T07:56:33Z, +документ `docs/reviews/CODE-REVIEW-252-r2.md`, зелёный вердикт, High:0/Medium:0), +но пайплайн не довёл дело до конца: автор сообщил (2026-08-23T07:56:45Z), что +автоматический прогон [упал](https://github.com/Matysh/houseplan-card/actions/runs/32625878719) +и статусная метка не переставилась. С точки зрения оркестратора раунд не +завершён (бюджет §4 не потрачен: зелёный вердикт цикл не расходует), поэтому +задача пришла на повторное r2, а не на r3. -Дополнительно проверено: `origin/dev` за время ревью успел уйти ещё на -2 коммита вперёд самой ветки (`6d0fa3b..origin/dev` → `2d1fca1`, -`a952f5f`). Разобран коммит `a952f5f` («feat(api): let config/get and -layout/get return less», issue #256, `User-Visible: no`) — он трогает -`custom_components/houseplan/{projection.py,websocket_api.py}` и -`tests_backend/test_projection.py`, не пересекается по файлам с #252 и -по контракту строго аддитивен (отсутствие новых параметров = байт-в-байт -старый ответ). Слияние #252 в `dev` потребует технического ребейза на -`2d1fca1`, но это не меняет оценку текущего диффа. +Проверено, что переиздавать нечего заново с нуля: -Состав диффа `origin/dev...HEAD` (34 файла): `src/space-reference-repair.ts`, -`src/plan-optimizer.ts`, `src/houseplan-card.ts`, `src/styles.ts`, RU/EN -i18n, `scripts/mutation-gate.mjs`, unit-тесты, `demo/smoke_orphan_space_ -references.mjs`, `demo/golden/{harness,matrix}.mjs` + 2 golden-сцены, -канонические доки (`CANVAS.md`, `CONFIG-COMPATIBILITY.md`, `ARCHITECTURE.md`, -`TESTING.md`, `USER-GUIDE.{md,ru.md}`), оба changelog, синхронные копии -бандла (`dist/`, `custom_components/.../frontend/`), `docs/specs/252-*.md` -и три ревью-документа предыдущих раундов. +- SHA, на котором был получен предыдущий вердикт r2: `b58136aa2cc943652af5adb8a94047b668d68dc6`. +- Текущий HEAD: `3741bddc6236ffe3d85965dc630704e578561710`. +- `git diff b58136a..HEAD --stat` → ровно один файл: + `docs/reviews/CODE-REVIEW-252-r2.md | 249 +++++++++++++++++++++++++++++++++++++` + (1 file changed, 249 insertions(+)) — это САМ ранее опубликованный документ + ревью, закоммиченный шагом публикации предыдущего (упавшего после вердикта) + прогона. Ни один файл `src/**`, `demo/**`, `docs/CANVAS.md` и т.п. между + `b58136a` и текущим HEAD не менялся. + +Значит, весь код, который уже был полностью разобран в CODE-REVIEW-252-r1 +(на пре-ребейзном коммите, полный разбор) и CODE-REVIEW-252-r2 (на +пост-ребейзном `b58136a`, тоже полный разбор — ребейз на ушедший вперёд `dev` +подпадает под §7.2), остался байт-в-байт тем же кодом. Дельта этого раунда — +пустая по существу. Полный разбор AC по коду в третий раз подряд на неизменном +дереве был бы именно той «потерей времени», от которой явно предостерегает +инструкция («полные наборы — это предрелизный гейт, а не гейт ревью»). + +Дополнительно проверено расхождение с `origin/dev`, который тем временем ушёл +дальше собственной прошлой проверки в CODE-REVIEW-252-r2 (там уже был учтён +`a952f5f`/#256 и его review-документ `2d1fca1`, оба признаны не пересекающимися +с #252 по файлам). С тех пор `dev` получил ещё два коммита: + +- `10999a5` — `ci(process): привести ветку к dev до код-ревью, а не после` (#257), + правит только `.github/workflows/process.yml`; +- `4b6331f` — `docs(process): описать приведение ветки к dev до код-ревью` (#257), + правит только `PROCESS.md`. + +Оба — чистый процесс/CI, ноль пересечения по файлам с диффом #252 +(`src/space-reference-repair.ts`, `plan-optimizer.ts`, `houseplan-card.ts`, +`styles.ts`, i18n, доки CANVAS/CONFIG-COMPATIBILITY/USER-GUIDE, golden-сцены). +Ретроактивно новое правило «ребейзить до ревью» на уже идущий с r1 код-ревью +#252 не распространяется (правило описывает будущий шаг пайплайна перед +следующим запуском ревью, а не требование к уже проверенному коду). Само +слияние ветки #252 в `dev`, как и раньше, потребует технического ребейза — +это не меняет оценку текущего диффа. ## Закрытие раунда r1 (CODE-REVIEW-252-r1) | Находка r1 | Чем закрыта | Где видно | |---|---|---| -| Low, снято с записью на будущее: мёртвый ключ `gs.optimize_reference_warning` остался в `en.json`/`ru.json`, код его больше не вызывает | Ключ полностью удалён из обоих словарей коммитом `6c779b5` | `git diff origin/dev...HEAD -- src/i18n/en.json src/i18n/ru.json` — строка с ключом присутствует только как удаление (`-`); `test/i18n.test.mjs:77` `assert.doesNotMatch(cardSource, /this\._t\('gs\.optimize_reference_warning'/)` — прогнан в составе `npm test`, зелёный | +| Low, снято с записью на будущее: мёртвый ключ `gs.optimize_reference_warning` остался в `en.json`/`ru.json`, код его больше не вызывает | Ключ полностью удалён из обоих словарей коммитом `6c779b5` | `git diff origin/dev...HEAD -- src/i18n/en.json src/i18n/ru.json` — строка присутствует только как удаление; `test/i18n.test.mjs:77` (`assert.doesNotMatch(..., /this\._t\('gs\.optimize_reference_warning'/)`) прогнан лично в этом раунде в составе `npm test` (1140/1140) — зелёный | -Отдельное процессное наблюдение, не находка к этому коду: сам вердикт -r1 называет только «поведенческий коммит `8fd8ccb`», не итоговый SHA -ветки на момент ревью (тогда это было `df51542`, что следует из -комментария автора о трёх коммитах `8fd8ccb`/`668ed49`/`df51542`). -Инструкция ревью прямо требует называть SHA, на котором получен -вердикт — здесь он не назван. Это не изменяет оценку текущего кода -(в этом раунде разбор полный и не зависит от того, что именно проверял -r1), фиксирую для гигиены пайплайна. +Это закрытие не изменилось со времени предыдущего r2 — код тот же самый. ## Унаследовано из r1 -Поскольку разбор в этом раунде полный (см. «Скоуп» выше), формально -наследовать нечего — весь код перечитан заново без опоры на выводы r1. -Единственное, что действительно взято без повторной проверки в деталях -— зелёный вердикт SPEC-REVIEW-252-r2 (одобренное ТЗ, коммит спеки не -менялся с r1 кода): нормативные разделы §6–7 спеки использованы как -эталон при сверке кода в этом раунде, само содержание спеки не -пересматривалось. +Полный построчный разбор кода (`src/space-reference-repair.ts` целиком, +изменённые фрагменты `plan-optimizer.ts`/`houseplan-card.ts`/`styles.ts`, +все юнит- и smoke-тесты, канонические доки, release-артефакты) взят из +CODE-REVIEW-252-r1 (документ в `docs/reviews/CODE-REVIEW-252-r1.md`, SHA +проверки в этом раунде — `b58136aa2cc943652af5adb8a94047b668d68dc6`, где этот +разбор был повторён ПОЛНОСТЬЮ заново после ребейза, а не как наследование — +см. раздел «Скоуп» того документа). Наследуется без повторного построчного +чтения в этом раунде: -## Как проверялось +- классификация владельца (`absent`/`live-in-missing-space`/`unverified`) — + доказательная схема, три исхода, fail-closed при неполном/неавторитетном + registry и при неизвестном namespace (AC1–AC3); +- отчёт без внутренних id в основном тексте, id — только в свёрнутых + «Подробностях» (AC4); +- Preview/Cancel/Apply/Undo и идемпотентность #248 не регрессируют (AC5, AC6); +- осознанное и задокументированное расширение поведения detach у #244 + (немедленное авто-удаление stale-позиции заменено общей классификацией; + переименованные тесты и `docs/CANVAS.md`/`docs/CONFIG-COMPATIBILITY.md` + прямо называют это заменой, а не регрессом); +- `docs/CANVAS.md:494-497` заменён (не дополнен), release-артефакты (оба + changelog, канонические доки, синхронные bundle-копии) в одном + поведенческом коммите с `User-Visible: yes`; +- golden-семантика двух #252-сцен (`optimize-orphan-references-dark-en`, + `optimize-orphan-references-light-ru`), доказанная полным + `golden:verify` (97/97) дважды — на пре-ребейзном дереве в r1 и на + пост-ребейзном `b58136a` в r2. -Прочитан целиком и построчно: `src/space-reference-repair.ts` (весь -файл, 369 строк), `src/plan-optimizer.ts` (изменённый фрагмент), -`src/houseplan-card.ts` (весь изменённый фрагмент — `_optimizeReference -Context`, `_previewAlignDialog`, `_toggleOptimizeLivePositions`, рендер -диалога), `src/styles.ts`, оба i18n-файла целиком по добавленным ключам, -`scripts/mutation-gate.mjs` (весь диф + не тронутые соседние мутанты), -`demo/smoke_orphan_space_references.mjs` целиком, `demo/golden/{harness, -matrix}.mjs`, `test/space-reference-repair.test.mjs` целиком (11 тестов), -`test/plan-optimizer.test.mjs`, `test/i18n.test.mjs`, `test/golden-matrix. -test.mjs`, канонические доки (`CANVAS.md`, `CONFIG-COMPATIBILITY.md`, -`ARCHITECTURE.md`, `TESTING.md`, `USER-GUIDE.md`, `USER-GUIDE.ru.md`), -оба changelog, `docs/specs/252-optimize-orphan-layout-report.md` целиком. +Основание доверять этому наследованию без повторного чтения — не слова +автора, а свежая проверка в этом раунде (см. ниже), что дерево с тех пор не +изменилось ни на байт. -Лично прогнано на текущем `HEAD` (`b58136a`), не со слов автора: +## Как проверялось в этом раунде + +Лично прогнано на текущем HEAD (`3741bdd`), не со слов автора и не по +памяти о прошлых раундах: - `npx tsc --noEmit` → чисто, без вывода; -- `npm test` → 1140/1140 pass, 0 fail, 0 skipped; -- `npm run build` → зелёно; `sha256sum dist/houseplan-card.js - custom_components/houseplan/frontend/houseplan-card.js` — совпадают - побайтово (`e5389ba8...`), обе копии синхронны; -- `node scripts/check-docs.mjs` → «Documentation checks passed (7 files, - 10 external links)» — обязателен, диф трогает `src/**`; -- `node scripts/mutation-gate.mjs --check` → все патчи, включая новые - `orphan-cleanup-partial-registry-deletes` и `orphan-cleanup-proven- - owners-kept`, ложатся на текущий код ровно один раз (реестр не - расходится с кодом); полный дорогой прогон (пересборка бандла на - мутанта) не повторялся — это предрелизный гейт, автор уже прогнал его - целиком (136/136) на этом же дереве, а `--check` подтверждает, что с - тех пор код под патчами не менялся; -- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → - прямое совпадение, те же 6 смоков, что называл автор: - `smoke_orphan_space_references`, `smoke_grid_snap`, +- `npm test` → 1140/1140 pass, 0 fail, 0 skipped (включает + `test/model-invariants.test.mjs` — 12/12, в т.ч. `#253: исчезнувшая запись + толщины` и `readModel понимает экспорт/config/get/сырой config (#254)` — + обязательный гейт, диф трогает layout-ссылки на пространства); +- `npm run build` → зелёный; `sha256sum dist/houseplan-card.js + custom_components/houseplan/frontend/houseplan-card.js` → + `e5389ba8e8250c6030fb5365b81619b0d2b4687b4c32f2f2c227ca527b7dbcec` для + обеих копий — **тот же хеш**, что зафиксирован в CODE-REVIEW-252-r1 + независимо от меня в этом раунде; совпадение хеша — самостоятельное + машинное доказательство того, что исходный код не менялся с r1/r2, а не + доверие на слово; +- `npm run bundle:sync` → пересобрал и синхронизировал нетрекаемую + стенд-копию `demo/srv/assets/houseplan-card.js` (не коммитится с #255) — + зелёно; +- `node scripts/check-docs.mjs` → «Documentation checks passed (7 files, 10 + external links)» — обязателен, диф трогает `src/**`; +- `node scripts/mutation-gate.mjs --check` (дешёвый режим, без пересборки + бандла на каждого мутанта) → 139/139 `ok`, включая все четыре мутанта + этой темы: `orphan-space-detach-disabled`, + `orphan-space-ambiguous-signature-guessed`, + `orphan-cleanup-partial-registry-deletes`, + `orphan-cleanup-proven-owners-kept`. Дорогой полный прогон (пересборка на + каждого мутанта) не повторялся: он уже дважды пройден целиком (r1 — + 136/136, r2 — переподтверждён) на этом же дереве, а `--check` подтверждает, + что реестр патчей и код с тех пор не разошлись; +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → те же 6 + смоков, что в r1/r2: `smoke_orphan_space_references`, `smoke_grid_snap`, `smoke_optimize_coordinate_canonicalization`, `smoke_optimize_geometry_preflight`, `smoke_optimize_micro_interval`, - `smoke_warm_dialogs`. После `npm run bundle:sync` (стенд-копия не - коммитится, #255) все 6 лично прогнаны и зелёные, все поля результата - `true`; -- `node demo/golden/run.mjs --mode=verify` (полный набор, 97 сценариев, - не только targeted) → 97/97 `passed`, включая обе #252-сцены - `optimize-orphan-references-dark-en` и `optimize-orphan-references- - light-ru`. Прогнан полностью, а не только targeted, потому что диф - меняет видимый рендер диалога (новые секции, кнопка, `
`) и - это первый полный прогон именно на этом (постребейзном) дереве в этом - ревью — независимая проверка, а не повтор доверия к словам автора. + `smoke_warm_dialogs`; остальные 165 не пересекаются по символам + (инструмент не назвал других связей); +- все 6 отобранных смоков лично прогнаны headless-Chromium в этом раунде — + все `OK`, все булевы поля результата `true` (в т.ч. + `idsExistOnlyInClosedDetails`, `explicitCleanupRebuildsPreviewWithoutWriting`, + `undoRestoresDeadRefs` из `smoke_orphan_space_references`). -Не прогонял отдельно: `npm run invariants -- --config <файл>` в виде -CLI на внешнем экспорте — конкретного экспорта живой инсталляции для -этой ветки нет. Вместо этого проверено, что `test/model-invariants. -test.mjs` (тест «все модели, которые возит с собой проект, инварианты -не нарушают (#254)») входит в `npm test` и гоняет ровно `checkReferences` -— инвариант, чей докстринг в `scripts/model-invariants.mjs` прямо -называет #252 («37 забытых позиций в layout») как один из дефектов, -которые он проверяет — на всех fixture-моделях проекта и на -`demo/srv/demo.html`; тест прошёл в составе прогнанного `npm test`. -Точечный CLI-прогон нужен для проверки конкретной конфигурации, а не -кода — здесь её нет. +Не повторял в этом раунде (обоснование): -Не прогонял: `python -m pytest tests_backend -q` (диф не трогает -`custom_components/**/*.py`), браузерное ручное тестирование (заменено -headless smoke + полным golden), performance-профили (не названы в -AC8 для этого цикла, `src/space-reference-repair.ts` — maintenance-only -путь, не render loop, что явно оговорено в спеке §12). +- **`golden:verify` (полный, 97 сценариев).** Уже пройден целиком дважды в + этом же код-ревью: в r1 на пре-ребейзном дереве и в r2 на `b58136a` — + оба раза 97/97, включая обе #252-сцены. Текущее дерево байт-в-байт + идентично `b58136a` (доказано выше диффом и совпадением SHA-256 бандла). + Третий полный прогон на неизменном дереве не добавил бы информации и + прямо противоречил бы правилу соразмерности гейтов ревью. +- **`mutation-gate.mjs` без `--check` (полная пересборка на мутанта).** По + той же причине — уже дважды 136+/136+ на этом дереве, `--check` в этом + раунде подтвердил отсутствие расхождения. +- **`python -m pytest tests_backend`** — диф не трогает + `custom_components/**/*.py` (только сгенерированный JS-бандл в той же + папке). +- **`npm run invariants -- --config <файл>`** — точечного экспорта живой + инсталляции для этой ветки по-прежнему нет; вместо него — `npm test` + прогнал `test/model-invariants.test.mjs` на всех fixture-моделях проекта + (см. выше), это тот же `checkReferences`, что стоит за флагом. +- **Performance-профили** — не названы в AC8, путь maintenance-only, не + render loop (§12 ТЗ). +- **Ручное браузерное тестирование** — не входит в конвейер ревью; заменено + headless smoke (свежепрогнанные) и дважды пройденным полным golden. ## Находки -Нет находок уровня High или Medium. - -Low, не блокирует, не требует отдельного цикла: - -- `SpaceReferenceReport.deadSpaceIds` (`src/space-reference-repair.ts:361`) - вычисляется (проход по layout, сборка `Set`, сортировка) и покрыт - тестами, но с этого раунда нигде не читается в продуктовом коде — - `grep -rn "deadSpaceIds" src/` вне `space-reference-repair.ts` пуст. - Раньше это поле питало `gs.optimize_reference_warning` - (`visibleDeadIds`/`remainingDeadIds` в `houseplan-card.ts`); теперь его - функцию полностью взял на себя `referenceDetails` (собран из - `removedPositions`/`liveMissingPositions`/`unverifiedPositions`). - Поведения не меняет и данные не портит — чистая мёртвая работа - внутри чистой функции. На усмотрение автора: убрать поле или оставить - как совместимый API отчёта. - -## Что проверено и корректно (в этом раунде, полным чтением) - -- **Классификация владельца (AC1–AC3).** Три исхода (`absent`/`live`/ - `unverified`) в едином проходе по `layout` - (`src/space-reference-repair.ts:302-360`) построены доказательно: - `rl_`-подписи проверяются по `existingRoomIds` (детерминировано из - config, авторитетность реестра не нужна); явный marker — по - `activeMarkers`/`removedMarkers` (`removed:true` — доказанное - удаление, живой активный marker — `live`, независимо от реестра); - `lg_` и авто-устройство (через персистентный, монотонно - накапливающий историю `settings.known_devices` — проверено, что - `diffNewDevices` в `src/logic.ts:2017-2025` НИКОГДА не выбрасывает - старые id, только добавляет новые, то есть однажды увиденное - устройство остаётся «известным» и после исчезновения — это и делает - классификацию «доказанно отсутствует» возможной для реальных - installation-сценариев из issue) требуют `rosterAuthoritative` для - перехода в `absent`; неизвестный namespace — всегда `unverified`, - без исключений. Юнит-тесты `test/space-reference-repair.test.mjs:225-346` - проверяют все три исхода по каждой категории, идемпотентность - второго прохода и fail-closed при `authoritative:false` и при - полностью неизвестном ключе (`future_widget:one`) даже при - авторитетном реестре — соответствует риску спеки «Future owner - удалён как мусор → Unknown namespace всегда unverified». -- **Живой владелец в удалённом пространстве, opt-in (AC2, §7.2).** - `_toggleOptimizeLivePositions` пересобирает preview через - `_previewAlignDialog(!prev)`, ничего не пишет (проверено смоком - `explicitCleanupRebuildsPreviewWithoutWriting`: `calls.length === 0` - после тоггла). Кнопка — настоящий `