From 49a02637cace8626dd80112be86dc6ae1af6036d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:27:39 +0000 Subject: [PATCH] docs: review document for #520 Issue: #520 User-Visible: no --- docs/reviews/CODE-REVIEW-520-r1.md | 221 +++++++++++++++++++++++++++++ 1 file changed, 221 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-520-r1.md diff --git a/docs/reviews/CODE-REVIEW-520-r1.md b/docs/reviews/CODE-REVIEW-520-r1.md new file mode 100644 index 00000000..98f42e74 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-520-r1.md @@ -0,0 +1,221 @@ +# CODE-REVIEW-520-r1 + +Issue: #520 · Этап: code · Заход: r1 · Материал: `a625871538c2cf58c39efd2b0e52be912f2e04e4` +Вердикт: **красный** · High: 1 · Medium: 0 + +## Скоуп + +ТЗ (тело issue #520, зафиксировано зелёным на SPEC-REVIEW r3) требует убрать +лишнюю эпоху конфигурации на старте, которую внёс #500: `_serverCfg` и +`_layout` перестают быть объявленными реактивными свойствами Lit, реактивность +сохраняется через `_adoption → onBodyReplaced → requestUpdate`. Диапазон +коммитов: `37ef970d`, `3d439805`, `27ea23c1` (диагностика бенчмарка), +`e181b08f` (сама правка + тесты + мутанты + бюджеты), `a6258715` (второй +мутант). Диф продуктового кода: 19 строк в `src/houseplan-card.ts` — ровно +контракт §1 ТЗ, без сюрпризов. + +## Как проверялось + +Прочитан диф всех файлов диапазона (`git diff origin/dev...HEAD`), тело issue +#520 и вся история комментариев (S2-анализ → ТЗ r1–r3 → хендофф на код-ревью). +Дешёвые гейты подтверждены зелёным Validate на этом SHA +(https://github.com/Matysh/houseplan-card/actions/runs/34499852060), но этот +прогон — обычный пуш в ветку задачи (`heavy=false` в `classify-changes.mjs`), +поэтому `performance_smoke`, `golden` и `smoke` в нём **skipped**, не success — +они не подтверждают ничего сверх typecheck/юнитов/сборки/мутантов по диффу. +`Полные бенчмарки производительности` (`performance.yml`) на этом SHA не +запускались вовсе: последний прогон был на `27ea23c1` (диагностика, +до правки) и он красный, как и ожидалось для дорегрессионного кода. + +AC4/AC5 этой задачи — это именно то, что дорогой Full Performance гейт +проверяет, а автор прямо пометил их «ждёт прогона» (пост-мерж CI). Это ровно +случай из инструкции ревьюера: «любой гейт, который требуют AC задачи» +Validate не покрывает и я отвечаю за него сам. В песочнице обнаружился +установленный Chromium (`~/.cache/ms-playwright` уже заполнен), поэтому я +прогнал полное сравнение сам, а не поверил записи «ждёт прогона». + +### Прогон Full Performance профиля `interaction` (ядро AC3–AC5) + +Воспроизвёл процедуру `performance.yml`: `git worktree add /tmp/hpc-baseline +a44fbd37` (база, зафиксированная в ТЗ), `npm ci && npm run build && +node scripts/bundle-sync.mjs` в обоих деревьях, затем +`npm run benchmark:large-house-interaction -- --target-root= +--samples=7 --warmups=1` и `npm run benchmark:compare -- --budgets=demo/performance/budgets-large-house-interaction.json`. + +**База `a44fbd37` (7 образцов):** 18 циклов обновления, 3 сборки модели, 3 +эпохи конфигурации, `epochs=[0->1@_saveConfig<_syncNewDevices ; +1->2@_saveConfig<_seedHiddenDevices ; 2->3@willUpdate1@willUpdate2@_saveConfig<_syncNewDevices + 2->3@_saveConfig<_seedHiddenDevices + 3->4@willUpdate 3000 (абсолютный потолок нарушен), `modelReadyMs` 1912.3 > 1622.01 (относительный лимит нарушен); то же на изометрии | ❌ не выполнено | +| AC5 | `cache.entries.cleanFloor` ≤100 | 120 (лимит уже возвращён на 100 правкой — job красный ровно как описывает сам AC5 «чем краснеет») | ❌ не выполнено | + +**AC1 и AC2 доказывают только синтаксический контракт** («свойство не +объявлено» и «вызов есть»), но не поведенческий эффект, ради которого вся +задача затевалась. Диагностика, которую сам автор оставил в дереве (`bootDiag`), +и есть инструмент, который это ловит — но её вывод никто не прочитал на +кандидате после правки: PR-комментарий явно фиксирует AC3–AC5 как «ждёт +прогона», а сам прогон (Full Performance) на этом SHA не выполнялся. + +### Находка 1 (High) — правка не устраняет регрессию, которую должна была устранить + +Убрав `_serverCfg`/`_layout` из `static properties`, эпоха конфигурации на +старте большого дома **не вернулась** к 3 (как требует AC3) — она осталась +равна 4, с той же лишней эпохой на первом `willUpdate`, что и до правки. +`firstStableRenderMs` не вернулся в бюджет (3320–3496 мс против потолка 3000 +и базы ≈2814–2828), `cache.entries.cleanFloor` остаётся 120 против +восстановленного потолка 100. Проверено дважды (7 образцов на профиле +взаимодействия + отдельно на изометрии), числа воспроизводимы и совпадают +между прогонами. + +Судя по диагностике (`willUpdate` бампает эпоху, когда +`changed.has('_serverCfg')` истинно, а `this._cfgEpochPreservedConfig` не +совпадает с текущим `_serverCfg`), первичная загрузка конфигурации идёт через +`adoptStructuralResponses` → `MutableConfigAdoption.setConfig`, которая +уведомляет `onBodyReplaced` напрямую, минуя `_setAreaLifecycleConfig` (тот +единственный путь, что заранее выставляет `_cfgEpochPreservedConfig` в то же +значение и гасит бамп). Это не Lit-специфичный механизм `wrapped`, который +диагностировало ТЗ, — а исходное состояние `_cfgEpochPreservedConfig`, +которое остаётся рассинхронизировано с первым реальным приходом конфига +независимо от того, объявлено ли свойство в Lit. Это наблюдение, не +диагноз «что чинить» — корень требует отдельного расследования автора, +не входит в мой мандат. + +Это блокирующая находка: ядро задачи (перф-регрессия #520) остаётся +неисправленным, при том что диф прошёл оба узких мутанта и весь остальной +набор дешёвых гейтов. Без повторного изменения продуктового кода (не только +диагностики бенчмарка) AC3/AC4/AC5 не могут стать зелёными. + +## Что проверено и корректно + +- Диф `src/houseplan-card.ts` (19 строк) — ровно контракт §1 ТЗ: две строки + объявления убраны, добавлен комментарий-ловушка над `static properties` + (§5 ТЗ). Постороннего кода нет. +- AC1: `test/config-adoption-ownership.test.mjs` — прочитан, тест умеет + падать (мутант `adoption-bodies-declared-reactive`, прогнан локально: + «поймано 1 из 1»). +- AC2: мутант `adoption-notifies-no-host-on-config-replacement`, прогнан + локально: «поймано 1 из 1». +- `npx tsc --noEmit` — чисто. +- `npm test` — 2509 pass / 0 fail / 1 skipped (2510 тестов), совпадает с + заявлением автора. +- `npm run build` — чисто, бандл собирается. +- `node scripts/mutation-gate.mjs --check` — реестр свидетелей цел. +- Бюджеты `budgets-interaction-smoke.json` и `budgets-large-house-interaction.json`: + `cleanFloor` возвращён на 100 в обоих файлах, как того требует AC5 (сама + цифра в бюджете корректна — красный результат означает, что КОД не + укладывается в правильно выставленный бюджет, а не что бюджет неверен). + `budgets-large-house-isometric.json` не трогался — там потолок уже был 100, + запись в ТЗ (r2→r3) подтверждена. +- Гигиена ветки: `.gitignore` (`node_modules` без слэша) и удаление + симлинка — проверено, в `git diff --stat` `node_modules` не фигурирует. +- `demo/performance/card-contract.mjs`, `test/performance-contract.test.mjs` — + добавленные поля (`_buildModel`, `_cfgEpoch`, `_adoptAuthoritative`) реальны + в проде, тест это перепроверяет по компилированному бандлу. +- Трейлеры коммитов — `Issue: #520` присутствует на всех коммитах диапазона. + `User-Visible: no` — корректно: контракт поведения снаружи не меняется + (просто ещё не действует так, как задумано). + +## Чего не проверял + +- **HA-харнесс** (`tests_backend/test_ha_*.py`) — правка не касается + `custom_components/**`, diff не затрагивает бэкенд. +- **Golden-кадры** — diff не меняет рендер/геометрию/стили, только + реактивность двух приватных полей и бюджеты в JSON; рендер-контракт не + затронут. +- **Инварианты модели** (`npm run invariants`) — diff не трогает рёбра + комнат, `layout`, `marker.space`, `open_spans`; геометрия не переименована, + только владение реактивностью. Не запускал. +- **Полный набор смоков** — `node scripts/smoke-select.mjs` не запускал + явно (инструмент недоступен без указания base/head в этой сессии review + script), но раз ядро задачи уже красное на перф-гейте, останавливаться на + находке High и возвращать в задачу — по процессу верное действие; + добивать остальные тяжёлые гейты до починки бессмысленно. +- **`process-gate.mjs` на диапазоне** — принял на слово прогон автора + (Validate `success` на этом SHA включает эквивалентный шаг в preflight). +- Прогонял всё внутри своей рабочей копии/временного `git worktree` + (`/tmp/hpc-baseline`, удалён после использования) — репозиторий ревью не + тронут, `git status` чист. + +## Гейты: что прогнал сам и почему + +| Гейт | Прогнан | Результат | +|---|---|---| +| `npx tsc --noEmit` | да (дёшево, доп. проверка) | чисто | +| `npm test` | да | 2509/2510 pass, 1 skip | +| `npm run build` + bundle-sync | да | чисто, дист пересобран для обоих деревьев | +| `node scripts/mutation-gate.mjs --check` + 2 мутанта задачи | да | реестр цел, оба мутанта пойманы 1/1 | +| Full Performance `interaction` (7 образцов, база `a44fbd37` vs кандидат) | да, вручную (Chromium уже установлен в песочнице) | **budget failed** — см. находку | +| Full Performance `isometric` (1 образец, для скорости) | да | тот же паттерн лишней эпохи, `modelReadyMs` за относительным лимитом | +| golden, HA pytest, полный smoke-набор, invariants | нет | вне диффа/уже нерелевантно после High-находки | + +## Вердикт + +Красный. Находка High блокирует: механизм задачи (лишняя эпоха конфигурации +на старте большого дома) не устранён, измерено официальным инструментом +проекта (`benchmark:large-house-interaction` + `benchmark:compare`) на +зафиксированном материале ревью `a6258715`, воспроизводимо на 7 образцах и +повторно на изометрии. AC1/AC2 (синтаксический контракт, проверен тестами и +мутантами) выполнены; AC3/AC4/AC5 (поведенческий/перф эффект, ради которого +затевалась вся задача) — нет. Возврат автору на доработку продуктового кода +(не диагностики бенчмарка), с новым измерением на исправленном кандидате до +следующего захода. + +--- + + + +## Материал раунда + +- Ветка: `issue/520-first-frame`, коммит `a625871538c2` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `fbd3b83fa6559f4470124d1b45e4272b4eadcdec` + ``` + git log --all --format='%H %T' | grep fbd3b83fa655 + ``` +- Тело issue: `60492a3aec5ff7fed54f7b32c040b7a118526c209609d736ee68e116eb8f0aa6` +- Вердикт конвейера: `red` · High 1