From 9cf5cdf3bd8ef733053648dea705829d4fc12064 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 05:06:45 +0000 Subject: [PATCH] docs: review document for #160 Issue: #160 User-Visible: no --- docs/reviews/CODE-REVIEW-160-r2.md | 300 +++++++++++++++++++++++++++++ 1 file changed, 300 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-160-r2.md diff --git a/docs/reviews/CODE-REVIEW-160-r2.md b/docs/reviews/CODE-REVIEW-160-r2.md new file mode 100644 index 00000000..12437a26 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-160-r2.md @@ -0,0 +1,300 @@ +# CODE-REVIEW-160-r2 — Isometric Stage 3 + +- Issue: https://github.com/Matysh/houseplan-card/issues/160 +- Этап: код-ревью (PROCESS.md §2.7) +- Заход: r2 · блокирующих циклов израсходовано 1 из 4 (потрачено в r1: красный + вердикт вернул задачу на правки; зелёный вердикт цикла не образует, #227) +- Ветка: `issue/160-isometric-stage3` +- **SHA материала ревью: `8c12474531b7a3ed2665e00e248de24ef6748c64`** (сверено + `git rev-parse HEAD` непосредственно перед выводом, PROCESS.md §2.7) +- Предыдущий раунд: `docs/reviews/CODE-REVIEW-160-r1.md`, материал SHA + `ee7d486924d5b1641a56f671bbf94e965fb76493` (подтверждено `git merge-base + --is-ancestor ee7d4869… HEAD` — да, прямой предок; ребейза между раундами не + было: `git merge-base HEAD origin/dev` = `afba49a3…` совпадает с базой r1) + +## Скоуп раунда — разбор по дельте (PROCESS.md §2.9/§2.10) + +Раунд не первый, и дельта **локальна**: три коммита поверх материала r1, +`git diff ee7d4869..HEAD --stat` — 3 файла продукта (`src/houseplan-card.ts`, +`src/iso-scene-render.ts`, новый `src/opening-symbol-placement.ts`), 3 тестовых +файла, `docs/images/screenshots.json` (только манифест), комментарий в +`scripts/bundle-budget.mjs`, плюс пересборка бандлов и собственный документ r1. +Ни ребейза на ушедший вперёд `dev`, ни смены контракта поведения, ни новой +подсистемы — условия §2.10 для полного разбора не выполнены. Разбор ограничен +находками r1 и тем, до чего дотягивается дельта (лок-бейдж проёмов, safe-point +резолвер, leaf-basis, скриншоты, байты бандла); остальные 15 AC наследуются из +r1 без повторной проверки (раздел ниже). + +Цель раунда — закрытие H1/M1/M2/M3 из r1. Все три коммита несут `Issue: #160` +/ `User-Visible: no` (сверено `git show -s --format=full` на каждом). + +## Как проверялось — гейты + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный | +| Unit | `npm test` | **2075 passed, 1 skipped, 0 failed** (совпадает с хендоффом автора; было 2071 на r1 — +4 новых теста от фиксов M1–M3) | +| Build + sync | `npm run build && npm run bundle:sync` | зелёный; `git status --short` после пересборки чист — три копии бандла (`dist`, `custom_components/houseplan/frontend`, `demo/srv/assets`) байт-в-байт совпадают с закоммиченными | +| Bundle budget | `npm run bundle:budget` | зелёный: initial View 299495 B gzip (потолок 300000±2000, запас 1571 Б); известный долг о малом запасе (#367), не относится к #160 | +| **`node scripts/check-docs.mjs`** | обязателен диффом по `src/**` | **зелёный** — «Documentation checks passed (7 files, 12 external links)». H1 закрыт, см. таблицу ниже | +| `node scripts/no-new-any.mjs --base ee7d4869 --head HEAD` | новый код дельты | зелёный: 52 добавленные строки в 3 файлах, новых `any` нет | +| `git diff --check origin/dev...HEAD` | — | чисто | +| `node scripts/process-gate.mjs` | офлайн, диапазон `origin/dev..HEAD`, 7 коммитов | «гейт пройден, предупреждений 0» (проверка 8 требует `--issues`, не запускал — не нужна ревьюеру) | +| `node scripts/smoke-select.mjs --base ee7d4869 --head HEAD` | выбор смоков по дельте | 6 «прямых совпадений», все — по символу `cellCm` (широко используемое имя, не специфично для лок-бейджа/safe-point/leaf-basis); суждение ревьюера ниже | +| `node demo/smoke_isometric_contract.mjs` | суждение ревьюера: дельта правит raised/Flat lock anchor и safe-point resolver — это ровно контракт этого смока | зелёный, все ассершены `true` | +| `node demo/smoke_isometric_live_touch.mjs` | то же основание | зелёный, все ассершены `true` | +| `node demo/smoke_lock_invariant.mjs` | смежная поверхность: правка трогает код, вычисляющий позицию lock-бейджа (SCOPE.md lock invariant) | зелёный | +| `node demo/smoke_lock_action.mjs` | то же основание | зелёный | +| Мутационная проверка «тест умеет падать» (M1/M2/M3, лично) | см. раздел ниже | 2 из 3 — красный получен ровно на новом тесте; 1 из 3 — красный получен на соседнем непеределанном тесте, не на новом (см. Находку L1) | + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **H1** — `check-docs` красный, скриншоты не пересняты | Каноническая Linux-съёмка через job `Docs screenshots` на коммите `bd60edd2` (fix-коммит), приёмка `npm run docs:accept -- --reviewed`, новый `sourceFingerprint` закоммичен | `docs/images/screenshots.json` (24 строки, только хеши отпечатка) в коммите `8c124745`; независимо проверено мной по логу workflow-прогона (см. ниже) и локальным запуском `node scripts/check-docs.mjs` → «passed» | +| **M1** — два независимых источника формулы floor anchor лок-бейджа проёма | Формула вынесена в `openingLockFloorPlacement()` (`src/opening-symbol-placement.ts:44-59`); `houseplan-card.ts:13086-13093` и `iso-scene-render.ts:856-874` вызывают её напрямую вместо дублирования математики; добавлен parity-тест | `src/opening-symbol-placement.ts` (новый экспорт), `git diff` на обоих сайтах вызова, `test/opening-symbol-placement.test.mjs:33-52` — лично мутировал знак `flipV`-ветки, новый тест покраснел («not ok 2») | +| **M2** — реальный алгоритм `isoRoomSafePoint` (grid-search) не покрыт тестом | Добавлены прямые вызовы `isoRoomSafePoint(room)` без предзаданного `safePoint` для донат-комнаты и вогнутой комнаты, плюс вырожденный кейс | `test/iso-overlays.test.mjs:96-133` — прогнано лично, все проходят; **но** мутационная проверка показала ограничение доказательства для hole-пути, см. Находку L1 | +| **M3** — `leafBasis` flipH не тестируется независимо от flipV | Заменённый тест строит `normal`/`flipH-only`/`flipV-only`/`both` и сравнивает точные `hinge`/`closedVector`/`quarterVector` | `test/iso-openings.test.mjs:209-235` — лично мутировал источники `sx`/`sy` (поменял местами `flipH`↔`flipV`), новый тест покраснел («not ok 10») | + +Все три Medium и единственный High из r1 закрыты предъявленной строкой кода +или теста, а не заявлением автора; каждое закрытие перепроверено мной лично +(включая намеренную порчу защиты там, где это дёшево — юниты). + +## Мутационная проверка (лично, «тест умеет падать») + +Выполнено через `git worktree`-независимую правку рабочего дерева (не +Edit-инструментом — редактирование продуктового кода мне не разрешено как +постоянное действие; правки делались временно, прогонялись и **полностью +откатывались** побайтово, `git status --short` после отката пуст, см. лог +команд ниже): + +1. **M1** (`opening-symbol-placement.ts`): `flipV ? -1 : 1` → `flipV ? 1 : -1`. + `test/opening-symbol-placement.test.mjs` → `not ok 2 - opening lock floor + placement owns both the anchor and its host side`. **Красный подтверждён.** +2. **M3** (`iso-openings.ts::leafBasis`): поменял местами источники + `sx`(было `flipH`)↔`sy`(было `flipV`, кроме gate). `test/iso-openings.test.mjs` + → `not ok 10 - flipH and flipV independently mirror their exact structural + axes`. **Красный подтверждён.** +3. **M2** (`iso-overlays.ts::pointStrictlyInRoom`): убрал цикл проверки дыр + (`for (const hole of room.holes...)`). Результат неожиданный — новый тест + `test/iso-overlays.test.mjs:96` («safe-point search… stays… inside… holes») + **остался зелёным**; красным стал другой, не тронутый этой задачей тест — + `not ok 3 - strict room ownership excludes holes, shared boundaries and + outside points` (уже существовал в r1, вызывает `resolveIsoOverlayOwner` + напрямую с точкой в дыре). См. Находку L1 — новый M2-тест не независим от + собственно проверяемого свойства. + +Все три файла возвращены оригинальным содержимым сразу после соответствующего +прогона; после восстановления перепрогнаны все три тестовых файла целым +набором — 27/27 зелёных, `git status --short` пуст. + +## Находки + +### L1 — новый M2-тест не независимо доказывает «вне любой дыры» (Low, запись без блокировки) + +`test/iso-overlays.test.mjs:96-114` вычисляет `first = isoRoomSafePoint(room)`, +затем проверяет принадлежность через `resolveIsoOverlayOwner({floorAnchor: +first, rooms:[room]})?.id === room.id`. Но `resolveIsoOverlayOwner` определяет +принадлежность вызовом того же `pointStrictlyInRoom` (`iso-overlays.ts:231,233`), +который `isoRoomSafePoint` использует внутри своего `consider()` +(`iso-overlays.ts:202`) для отбора кандидатов. Если у `pointStrictlyInRoom` +сломать именно проверку дыр (мутация выше), обе функции ошибаются согласованно +друг с другом: `isoRoomSafePoint` вернёт точку внутри дыры, а +`resolveIsoOverlayOwner` её не отклонит — тест не заметит. + +Заявленный в r1 риск («если алгоритм вернёт точку в дыре, ни один тест этого +не заметит») сейчас закрыт **не тем тестом, который претендует на это в своём +названии**, а старым, непеределанным `test 3` того же файла — оно продолжает +проверять `pointStrictlyInRoom` через явный `floorAnchor:[50,50]`, независимо +от `isoRoomSafePoint`. Защита существует и я лично подтвердил, что она +срабатывает (мутация ловится набором тестов файла в целом), но она не там, где +её называет новый тест, и последующий рефакторинг, который переименует или +уберёт «чужой» `test 3`, тихо унесёт с собой и защиту M2, а новый тест +продолжит быть зелёным. + +**Не поднимаю до Medium**: реальной прорехи в текущем дереве нет (мутация +ловится), primary-риск r1 («алгоритм вообще не выполняется в тестах») закрыт +по существу — до этой правки ни один тест не проходил через сам +candidate/grid-search, теперь проходит. Это узкое дефектное свойство +конкретно нового теста (assertion пишет о доказательстве, которого сам не +даёт), тот же класс, что и Low-находки r1 (например «`shadows` не защищено +собственным тестом… будущий рефакторинг тихо сломает»). + +**Рекомендация (не блокирует):** заменить/дополнить проверку в +`test/iso-overlays.test.mjs:110-113` прямым геометрическим утверждением, +не зависящим от `pointStrictlyInRoom` — например явно проверить, что `first` +лежит вне bbox дыры `[35,65]×[35,65]` для `donut`, отдельной инлайн-проверкой. + +### L2 — приёмка скриншотов сменила заявленную платформу на `win32` без видимого основания в хендоффе (Low, запись без блокировки) + +`docs/images/screenshots.json:121` — `"acceptedOn"` сменился с `"linux"` +(значение до правки r2) на `"win32"`. Код `scripts/docs-accept.mjs` вызывает +`assertCaptureEnvironment({kind:'docs', stage:'accept'})` **первым действием** +(`scripts/capture-environment.mjs:110-119`), которое **бросает исключение** и +останавливает приёмку на платформе, отличной от `linux`, если не задан +`HP_ALLOW_FOREIGN_CAPTURE=<причина>` (пустая причина не считается). То есть +запись `acceptedOn: "win32"` в закоммиченном файле механически доказывает, что +приёмка либо шла через явный, осознанный обход этой переменной (легитимный +путь — ровно то, для чего он существует: артефакт снят на Linux CI, а сама +команда приёмки выполнена локально на Windows, дневном окружении автора по +AGENTS.md), либо гейт был обойдён иначе. Скрипт не сохраняет саму причину в +JSON (только `platform`), поэтому из закоммиченного дерева её не увидеть. + +Хендофф-комментарий автора («manifest закоммичен в `8c124745`») не называет ни +команду приёмки, ни обход, ни его причину — только результат. Я не нахожу +признаков нелегитимности: воспроизведение самого содержательного свойства — +байт-в-байт совпадение всех 10 PNG — я проверил независимо по логу CI-прогона +(`git diff --check origin/dev` PNG: 0 изменившихся, тот же Chromium +`151.0.7922.34`, тот же `oxipng 10.2.0` — см. ниже), так что H1 закрыт по +существу вне зависимости от площадки приёмки. Но провенанс самой команды +приёмки (в духе дисциплины «verified без названной команды не доказательство») +неполон — это узко процессная, а не продуктовая находка. + +**Не поднимаю до Medium**: PROCESS.md/AGENTS.md явно требуют трейлер +`Baseline-Reviewed` только для правок `demo/golden/baselines/**`; для +`docs/images/**` эквивалентного машинного требования нет, и содержательное +свойство (неизменность байтов) я проверил независимо. **Рекомендация (не +блокирует):** в будущих хендоффах называть точную команду приёмки, включая +`HP_ALLOW_FOREIGN_CAPTURE`, если он был нужен. + +## Независимая проверка H1 по логу CI (не на слово) + +``` +gh api repos/Matysh/houseplan-card/actions/runs/34012427938 --jq '.event,.status,.conclusion' +→ workflow_dispatch, completed, success + +# фактически выбранный actions/checkout ref и итоговый коммит: +ref: issue/160-isometric-stage3 +git log -1 --format=%H → bd60edd23818eac617857de27260dc604e430bfc # ровно fix-коммит r2 + +# шаг «Вердикт» джобы: + M docs/images/screenshots.json +--- изменившихся PNG: 0 +--- Chromium: было «151.0.7922.34», стало «151.0.7922.34» +--- oxipng: было «oxipng 10.2.0», стало «oxipng 10.2.0» +ВЕРДИКТ: ничего не изменилось, принимать нечего. +``` + +Это подтверждает независимо от заявления автора: приёмка не «пробила» новую +съёмку под видом старой — PNG не менялись ни одним байтом, менялся только +метаданный отпечаток исходников (соответствует тому, что диапазон меняет +`src/**`, а не визуальный вывод). + +## Что проверено и корректно (дельта r2) + +- **Формула lock anchor (M1):** идентична побайтово прежним двум копиям — + сверил построчно; вызовы в `houseplan-card.ts` и `iso-scene-render.ts` + передают одинаковые поля (`x/y/angle/flipV/gateFace`), `gateFace` вычисляется + так же, как раньше (`_openingFace`/`partitionOpeningFace`), просто на + вызывающей стороне вместо внутри общей функции — не архитектурный сдвиг. +- **Живой DOM подтверждает отсутствие регрессии:** `smoke_isometric_contract` + и `smoke_isometric_live_touch` (комплексные проверки raised/Flat геометрии, + включая lock-бейдж и safe-point/nudge путь) зелёные на пересобранном бандле + этой дельты. +- **Смежная безопасность (SCOPE.md lock invariant) не задета:** код + actuation-пути (`resolveToggleIntent`, `isControllable`, `_cardToggle`) в + дельте не тронут; `smoke_lock_invariant`/`smoke_lock_action` зелёные. +- **Бюджет бандла:** сдвиг с 299464 → 299495 Б gzip (+31 Б, ожидаемо — + добавленный `import { gridVisualUnits }` в `opening-symbol-placement.ts` и + новый экспорт) не поднимает потолок и не приближается к аварийному порогу + сверх уже известного долга #367; комментарий в `bundle-budget.mjs` обновлён + соответствующими числами (не код, чисто документирующий комментарий). +- **Одно число — один источник:** M1 устраняет ровно риск, который правило + называет — два места считали одну видимую величину (позицию лок-бейджа) + независимо; теперь она в одном месте. Проверил, что `test/single-source-numbers.test.mjs` + не тронут (ожидаемо — задача не меняет формат отображаемого числа). +- **Трейлеры:** все три коммита дельты — `Issue: #160` / `User-Visible: no`, + соответствует «Stage 3 остаётся скрытой» (публичные changelog не менялись). + +## Унаследовано из r1 (без повторной проверки) + +Документ: `docs/reviews/CODE-REVIEW-160-r1.md`, материал SHA +`ee7d486924d5b1641a56f671bbf94e965fb76493` (подтверждён как прямой предок +текущего HEAD). Дельта не касается ни одной из перечисленных ниже +поверхностей (проверено по `git diff ee7d4869..HEAD --stat` — только +`src/houseplan-card.ts`, `src/iso-scene-render.ts`, +`src/opening-symbol-placement.ts`, 3 тестовых файла, манифест скриншотов, +комментарий `bundle-budget.mjs`): + +- **AC2/D1 камера** (`rotDeg=4`, `tiltDeg=20`, единая матрица + `isoPlaneMatrix`/`applyIsoMatrix`, `src/iso-projection.ts`) — не тронуто. +- **AC3/D2 raised/floor split**, включая vacuum floor-bound — не тронуто + (кроме самого anchor lock-бейджа, который переразобран заново выше). +- **AC4 плейт/44×44 hit target** — не тронуто. +- **AC6/D5 проёмы** (jamb/reveal/leaf/frame/sill, hinge/face turn direction, + `iso-scene-render.ts` W9-путь) — не тронуто (leaf-basis M3 переразобран + заново выше, это смежная, но отдельная от W9 функция). +- **AC7 `show_borders:false`** — не тронуто. +- **AC8/AC12 structural fingerprint, LRU cap 8, material defs O(1)** — не + тронуто. +- **AC9 degradation (forced-colors/no-filter, fail-closed Flat fallback)** — + не тронуто. +- **AC10 Zigbee-топология на поднятых DOM-центрах** — не тронуто. +- **AC11 lifecycle/touch/kiosk** — не тронуто (смежная безопасность + перепроверена смоками выше, но не как часть этого пункта). +- **AC13 негативный контракт** (нет новых зависимостей/i18n/storage/config/ + network путей, legacy `hp-labs`/expiry не возвращены) — не тронуто. +- **AC14/AC15 гейты, golden-диагностика, документация подсистемы** (кроме + самого отпечатка скриншотов, переразобранного как H1 выше) — не тронуто; + `npm run golden:verify` в этом раунде не перезапускал: делта не меняет + визуальную геометрию/рендер (чистый рефакторинг формулы + тесты), а живой + DOM-контракт (смоки выше) и byte-identical PNG (независимая проверка по логу + CI) уже показывают отсутствие визуального сдвига; полный golden — не гейт + ревью, а предрелизный (PROCESS.md §8). +- **Производительность (§10, sanity-прогон `isometric-stage3-dense-v1`)** — не + тронуто; делта не меняет структуру рендера, только формулу позиционирования + и покрытие тестами. + +## Чего не проверял и почему + +- **Полный `npm run golden:verify`** — не перезапускал в этом раунде (см. + «Унаследовано» выше); было исчерпывающе проверено в r1 на материале, который + дельта не меняет визуально. +- **Полный набор `demo/smoke_*.mjs` (226 файлов)** — не прогонял; выборка по + дельте (`smoke-select.mjs`) не дала специфичных совпадений (только общий + символ `cellCm`), прогнал по собственному суждению два целевых контрактных + смока плюс два смежных lock-смока — все зелёные. Полный прогон — предрелизный + объём (PROCESS.md §8), не гейт ревью для этой по размеру небольшой дельты. +- **`npm run invariants` / `model-invariants.mjs`** — дельта не меняет + геометрическую модель (комнаты/стены/`layout`/`marker.space`/`open_spans`); + подтверждено diff-статом, ни одного изменения в `custom_components/**` или + схеме конфига. +- **`python -m pytest tests_backend`** — ноль изменений в + `custom_components/houseplan/**/*.py` в этом диапазоне. +- **7-sample exact-SHA performance профили** — канонический гейт остаётся + pre-beta (§8/§10); дельта не меняет структуру рендера/кэша, только формулу + позиционирования и тестовое покрытие, повторный sanity-прогон не требовался + суждением ревьюера. +- **Точная причина `HP_ALLOW_FOREIGN_CAPTURE` для приёмки скриншотов на + `win32`** — не могу восстановить из закоммитированного дерева (скрипт не + пишет причину в манифест, только платформу); зафиксировано как L2. + +## Итог + +**High: 0. Medium: 0.** Все четыре находки r1 (1 High + 3 Medium) закрыты +предъявленной строкой кода/теста и лично перепроверены — три из них через +намеренную порчу защиты с последующим красным прогоном и откатом, одна (H1) +через независимую сверку лога CI-прогона по точному коммиту. Дельта не вносит +новых High/Medium; две узкие Low-находки (L1 — новый M2-тест доказывает +hole-safety не независимо от проверяемой функции; L2 — провенанс площадки +приёмки скриншотов не назван в хендоффе) записаны без блокировки — реальная +защита в обоих случаях сейчас присутствует, риск умозрительный/процессный, не +продуктовый. + +**Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0** + +--- + + + +## Материал раунда + +- Ветка: `issue/160-isometric-stage3`, коммит `8c12474531b7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b09988aa9a28648b66b731e535ddb243372428c3` + ``` + git log --all --format='%H %T' | grep b09988aa9a28 + ``` +- ТЗ `docs/specs/160-isometric-stage3.md`, блоб `20ed447b8f58bdfe9e48695584ff616c36b79b4d` + ``` + git log --all --find-object=20ed447b8f58bdfe9e48695584ff616c36b79b4d -- docs/specs/160-isometric-stage3.md + ```