diff --git a/docs/reviews/CODE-REVIEW-333-r2.md b/docs/reviews/CODE-REVIEW-333-r2.md new file mode 100644 index 00000000..3c7b13d5 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-333-r2.md @@ -0,0 +1,191 @@ +# CODE-REVIEW-333-r2 + +Issue: #333 · Заход: r2 · Блокирующих циклов израсходовано 1 из 2 +Материал: `git log --oneline origin/dev..HEAD` = три коммита на текущем +`HEAD` = `4ded9c0b` (`git rev-parse HEAD` сверен непосредственно перед +подведением итогов): + +``` +4ded9c0b test: the #248 roundtrip fixture is seeded, not first-written (#333 r1-H1) +783cc68b docs: review document for #333 (публикация CODE-REVIEW-333-r1.md) +5a2dd333 fix: plan/optimize passes the junction gate; import stays free by design (#333) +``` + +Родитель `5a2dd333` — `b37a5987` (= `origin/dev` на момент проверки). + +## 0. Важное расхождение с материалом прошлого раунда — рассмотрено и снято + +r1-документ (`docs/reviews/CODE-REVIEW-333-r1.md`) называет проверенным +коммитом `c29df29b` с родителем `73406225`. Такого SHA в текущей истории +нет — между r1-ревью и фиксом r1-H1 ветка была перебазирована на ушедший +вперёд `dev`: между `73406225` и текущим родителем `b37a5987` легло 17 +чужих коммитов issue #337 (лениво загружаемый рантайм редактора, полная +перестройка бандлинга — 114 файлов, ~34 тыс. строк). + +Это ровно сценарий «ребейз на ушедший вперёд dev» из брифа, который по +умолчанию требует полного разбора, а не разбора по дельте. Проверил, что +разбор действительно должен остаться полным или можно сузить: + +- **Пересечения файлов нет.** `git diff --stat 73406225 5a2dd333^` + (весь чужой рывок dev) не касается ни одного файла, который трогает + #333 (`custom_components/houseplan/{junction_limits,websocket_api}.py`, + `docs/specs/329-junction-limits.md`, `scripts/mutation-gate.mjs`, + `tests_backend/test_ha_websocket.py`) — только `src/**`, бандлинг и + фронтенд-доки. +- **Номера строк не сдвинулись.** `websocket_api.py:1626` + (`ws_plan_optimize`), `:1783` (`rt.junction_baseline = …`), `:1396` + (`config/set`) — те же номера, что называет r1-документ. Файл ребейзом + физически не задет. +- Поэтому содержательно к делу можно подойти по дельте (закрытие r1-H1), + но **гейты прогнал заново и полностью**, а не унаследовал: у ребейза + такого масштаба легко мог быть скрытый эффект на сборку/реестр + мутаций/бэкенд-набор, а зелёного Validate на этом SHA нет (см. §2). Это + тот случай, где полнота разбора обеспечена не «на всякий случай», а + проверкой конкретного факта (непересечение файлов), которая и позволяет + не заводить AC1–AC4 заново, ограничившись закрытием H1. + +## 1. Скоуп + +Не изменился с r1: ТЗ (тело issue, small-трек, ревизия 2, оба раунда +spec-ревью зелёные) требует (1) `validate_junction_limits` в +`ws_plan_optimize` после миграции кандидата, наследование по правилу; +(2) обновление `rt.junction_baseline` после успешного optimize; (3) +import/restore вне гейта, докстринг и спека §5 — честно. Продуктовая +рамка (`docs/SCOPE.md`) и корректность `User-Visible: no` — без изменений +с r1, задача только про то, доказан ли контракт и не сломано ли что-то +существующее. + +Предмет r2 — единственная правка после r1: тестовая фикстура +`test_plan_optimize_persists_exact_storage_roundtrip_target` +(`tests_backend/test_ha_websocket.py`) теперь сеет исходный конфиг в +хранилище перед вызовом `plan/optimize`, вместо пустого `previous`. +Продуктовый код не тронут ни строкой — весь коммит `4ded9c0b` +ограничен тестовым файлом. + +## 2. Как проверялось + +Зелёного Validate на `4ded9c0b` нет (сообщение брифа) — прогнал сам, +полностью (обоснование §0): + +| Гейт | Команда | Результат | +|---|---|---| +| Типы | `npx tsc --noEmit` | чисто | +| Frontend-юниты | `npm test` | 1439 passed, 0 failed, 1 skipped | +| Сборка + синхрон бандла | `npm run build`; `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js`; сверка списков файлов `dist/houseplan-assets/` и `custom_components/houseplan/frontend/houseplan-assets/` | все три копии идентичны | +| Реестр мутаций | `node scripts/mutation-gate.mjs --check` | зелёный, `junction-limit-optimize-unguarded` зарегистрирован, patch-строка совпадает с реальным кодом (см. ниже) | +| Мутант убивает тест | ручной патч `return validate_junction_limits(...)` → `return {}, None` в `websocket_api.py:1706-1708`, `pytest -k test_333_optimize_refuses_a_crafted_violation` | тест красный (`assert not True`), дерево возвращено (`git status` чист) — тест умеет падать | +| Backend, полный набор | `python -m pytest tests_backend -q` (харнес `pytest-homeassistant-custom-component`+`home-assistant-frontend` установлены в сессии) | **432 passed, 1 skipped, 1 error** (error — известный teardown-артефакт Python 3.12/харнеса, см. §5, не по телу теста) | +| Backend, целевые тесты | `pytest -k "test_plan_optimize_persists_exact_storage_roundtrip_target or test_333_optimize_refuses_a_crafted_violation_and_keeps_the_plan or test_333_optimize_inherits_stored_violations or test_333_optimize_refreshes_the_junction_baseline_cache"` | все 4 PASSED | +| Смок-отбор | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «исполняемого frontend-диффа нет» — браузерные смоки не выбираются, diff не трогает `src/**` | +| check-docs | не гонял | diff #333 (все 3 коммита) не трогает `src/**` ни строкой — критерий запуска не выполнен | +| `npm run invariants` | не гонял | diff не меняет геометрию/рёбра/записи толщины/`layout`/`marker.space`/`open_spans` — только добавляет вызов уже существующего Python-валидатора и правит тест-фикстуру | +| performance-профили | не гонял | не названы в AC, дорогая цепочка уже в executor (#330), диффом не тронута | +| «Одно число — один источник» | рассмотрено | diff не вводит и не меняет ни одной пользовательски видимой величины — `User-Visible: no` во всех трёх коммитах корректен | + +Дополнительно проверил рабочее дерево после ручного патча мутанта: +`git status` — чисто, никаких файлов не осталось изменёнными +(`dist/houseplan-card.js` случайно поменял режим/mtime после +`npm run build` — вернул `git checkout -- dist/houseplan-card.js`). + +## 3. Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| **H1 (High).** Optimize с пустым `previous` (`expected_config_rev: 0`) на фикстуре #248 засчитывал легитимную стену 6 см как НОВОЕ нарушение П3 (правило «первая запись не может прийти уже сломанной») — существующий зелёный тест `test_plan_optimize_persists_exact_storage_roundtrip_target` падал на `assert response["success"]`. | Коммит `4ded9c0b`: фикстура теперь СЕЕТСЯ в `runtime.config_store` как хранимый документ (`rev: 1`) ПЕРЕД вызовом `plan/optimize` (`expected_config_rev: 1` вместо `0`), поэтому optimize видит непустой `previous` и 6-см стена читается как унаследованное нарушение — ремонтный поток, как в AC2. Ревизии в ассертах сдвинуты на 1 (`config_rev == 2`), storage-утверждения #248 (intent/pending/final/каноническая сериализация) не тронуты. | `tests_backend/test_ha_websocket.py:626-646` (комментарий + новый `async_save`), запуск: `pytest -k test_plan_optimize_persists_exact_storage_roundtrip_target` → **PASSED** (был **FAILED** на `c29df29b`/эквивалентном `5a2dd333`). Полный `pytest tests_backend -q`: 432 passed / 0 failed (было 1 failed) | +| **Medium, вне скоупа.** Бриф ревью ссылался на Validate-run `c29df29b` (33145399107) как на «зелёные дешёвые гейты», но job'ы фронтенда и бэкенда в нём были `skipped`, не `success`, из-за потери диапазона коммитов классификатором после force-push. | Не в этой ветке — заведён отдельно, как и требует правило (Medium вне скоупа → отдельный issue, не «оставлено в тексте»). | #347 (`bug, infra, P1, S1-new`, ссылка на #333) — существует, открыт, метки на месте (проверено `gh issue view 347`) | + +Оба пункта r1 закрыты доказательно, не декларативно: H1 — перезапуском +ранее красного теста и полного бэкенд-набора; Medium — проверкой, что +issue реально существует с нужными метками. + +## 4. Унаследовано из r1 (без повторной проверки содержания) + +Из `docs/reviews/CODE-REVIEW-333-r1.md` (материал: коммит `c29df29b`, +родитель `73406225`) принимаю без повторного разбора — с довеском: +файлы, к которым относятся эти выводы, не менялись НИ РАЗУ между +`c29df29b`/`73406225` и текущим `HEAD`/`origin/dev` (см. §0, номера строк +совпадают), поэтому дельта их не задевает: + +- **Техническая точность контракта п.1–3** — вызов + `validate_junction_limits` в `_validate_optimize_cpu` после миграции + кандидата, то же место конвейера, что в `config/set`; `JunctionLimitError` + в except-списке перестал быть мёртвым. +- **AC1 (крафтованный payload отклонён, `config`/`rev` побайтово + неизменны)** и структурная гарантия неизменности layout (отказ до + первого `async_save_*`). +- **AC2 (наследование легаси-нарушения в optimize)** — тест + `test_333_optimize_inherits_stored_violations`. +- **AC3 (обновление `rt.junction_baseline`)** — симметрия с + `config/set` (`websocket_api.py:1396` / `:1783`). +- **AC4 (докстринг + спека §5 честны)** — текст `junction_limits.py:1-15` + и `docs/specs/329-junction-limits.md` §5 (абзац о периметре, строки + 127-134) — сверил построчно ещё раз (§2, «Как проверялось» этого + документа не потребовалось трогать код, только перечитать текст) и + подтверждаю: абзац идентичен тому, что видел r1 — diff между + `origin/dev...HEAD` для этого файла ограничен ровно этим одним + абзацем, второй раз не добавлялся и не менялся. +- **Реестр мутаций** — `junction-limit-optimize-unguarded` описан честно; + я самостоятельно перепрогнал и `--check`, и ручное убийство мутанта + (не чисто унаследовал — но результат совпадает с r1). +- **Паритет П1–П4** — `test_parity_with_the_frontend_checks` не тронут, + прошёл в составе полного набора. +- **Трейлеры** — `Issue: #333`, `User-Visible: no` верны во всех трёх + коммитах этого раунда (проверено заново, т.к. коммиты новые). +- **Соответствие малому треку** — не переоценивал; объём (закрытие + одного High одним тестовым коммитом) трек не нарушает. + +## 5. Находки + +Новых находок нет. High: 0. Medium: 0. + +## 6. Что проверено и корректно + +- r1-H1 закрыт по существу, а не косметически: подтверждено, что + предмет теста (#248, байт-точность storage) не пострадал — все + storage-ассерты (`intent_write`, `pending`, `final_config`, + `final_layout`, каноническая сериализация JSON) остались в тесте + дословно, изменились только сид и ожидаемые номера ревизий. +- Семантика фикса согласована с контрактом: посеянный `previous` + идентичен `candidate` (`source["config"]` без изменений), то есть это + echo-оптимизация того же класса, что AC2 — 6-см стена наследуется, а + не легализуется заново; ровно то поведение, которое и должен доказывать + тест про репутацию хранилища. +- Полный бэкенд-набор (432/0/1 error-teardown) и целевые 4 теста + зелёные лично прогнанными командами, не по докладу автора. +- Мутант `junction-limit-optimize-unguarded` реально убивает целевой + тест — проверено вручную в этом раунде. +- Рабочее дерево после всех ручных экспериментов чистое + (`git status` пуст). +- Ребейз на ушедший вперёд `dev` (#337) не затронул ни один файл этой + задачи — подтверждено диффом файлов, а не предположением. + +## 7. Чего не проверял и почему + +- **AC1–AC3 заново «с нуля»** — не пересобирал заново обоснование + каждого, ограничился перезапуском тестов и сверкой, что r1-текст + описывает код без расхождений (см. §4) — файлы, которых касаются эти + AC, дельтой не тронуты. +- **check-docs, golden:verify, browser-смоки** — diff (все три коммита + #333) не трогает `src/**` ни разу; `smoke-select.mjs` подтвердил + инструментом. +- **`npm run invariants`** — diff не меняет геометрию/ссылки на неё. +- **Полный `node scripts/mutation-gate.mjs`** — избыточен, диффу + соответствует один мутант, проверен точечно. +- **performance-профили** — не названы в AC, чувствительный путь + (#330 executor) не тронут. +- **Спор о line-level точности спеки/докстринга AC4** — не перечитывал + файл целиком заново построчно посимвольно, положился на факт «diff + `origin/dev...HEAD` для этого файла = один абзац, тот же, что видел + r1» — это сильнее, чем повторное чтение вслепую, потому что исключает + случайный второй правки в этом раунде. + +## 8. Вердикт + +r1-H1 закрыт доказательно (ранее красный тест теперь зелёный, полный +бэкенд-набор чист), r1-Medium корректно вынесен в #347 вне этой ветки. +Ребейз на ушедший вперёд `dev` между раундами рассмотрен явно и не +потребовал полного повторного разбора AC, так как ни один файл #333 им +не затронут — но все дешёвые гейты и полный бэкенд-набор прогнаны заново +(зелёного Validate на `4ded9c0b` нет). Новых находок нет. + +Вердикт: зелёный · заход r2 · блокирующих циклов 1/2 · High: 0 · Medium: 0