diff --git a/docs/reviews/CODE-REVIEW-385-r2.md b/docs/reviews/CODE-REVIEW-385-r2.md new file mode 100644 index 00000000..72fa07e7 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-385-r2.md @@ -0,0 +1,240 @@ +# CODE-REVIEW-385-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/385 +- Заход: r2 · блокирующих циклов израсходовано до этого раунда: 0/4 +- Материал: `git log --oneline origin/dev..HEAD` / `git diff origin/dev...HEAD` на SHA + `2e77fe09a554692bc23bf1f2497c2ae6c8c02059` (branch tip == материал ревью, + `origin/dev` = `fbbea475`, ветка полностью включает `dev` — `merge-base` совпадает + с `origin/dev`) +- Спека: `docs/specs/385-audit-lows.md` (ревизия 2, SPEC-REVIEW-385-r2 зелёное) + +## Почему разбор полный, а не по дельте + +Пайплайн привёл ветку к `dev` до этого ревью: поверх легло 1 незнакомых коду +r1 коммит(ов) `dev` (`fbbea475` — `ci: name the commit that actually broke +golden`, класс B, только workflow, не пересекается с диффом задачи). Прежний +r1 code-review (зелёный, `564bf419`) не смог слиться (конфликт), задача ушла в +`S6-in-progress` на ребейз, и найденный SHA `564bf419` в текущей истории уже +недостижим — ветка была перестроена. Согласно §7.2/§2.10 после ребейза это +другой код, поэтому разбор — полный: весь диапазон `origin/dev...HEAD` +перечитан заново, каждый AC1–AC6 перепроверен по коду и/или гейтом лично в +этом раунде. + +## Скоуп диффа + +Класс A: `src/devices.ts`, `src/houseplan-editor-runtime.ts`, +`custom_components/houseplan/import_export.py`. +Класс B: `scripts/process-gate.mjs`, `scripts/mutation-gate.mjs` (+2 мутанта), +`test/devices.test.mjs`, `test/process-gate.test.mjs`, +`tests_backend/test_ha_import_export.py`, `demo/smoke_value_face_source.mjs`. +Класс C: `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`, `docs/images/screenshots.json` +(fingerprint bump), `docs/reviews/CODE-REVIEW-385-r1.md` (артефакт r1, +cherry-pick). Класс D: `custom_components/houseplan/frontend/**`, `dist/**` — +байт-в-байт совпадают с локальной пересборкой, не рассматриваю отдельно. + +Один коммит `223f499e` несёт все четыре пункта (а)–(г) с трейлерами +`Issue: #385` / `User-Visible: yes`; оба changelog в нём же. Коммит `2e77fe09` +(документ r1, `User-Visible: no`) — публикационный, не продуктовый. + +## Как проверялось (гейты, лично на этом SHA) + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | чисто | +| unit | `npm test` | 1592 pass / 0 fail / 1 skipped | +| build + 3 копии бандла | `npm run build && cmp dist/houseplan-card.js custom_components/.../houseplan-card.js`; `npm run bundle:sync` | без diff, `git status --short` пуст после сборки | +| check-docs | `node scripts/check-docs.mjs` | зелёный (7 файлов, 10 внешних ссылок); `sourceFingerprint` в `screenshots.json` уже обновлён под текущий `src/**` | +| bundle:budget | `npm run bundle:budget` | initial View 277 995 / 300 000 Б gzip (запас 22 005 Б) — совпадает по порядку с заявленным автором | +| no-new-any | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | 23 новых строки в 2 файлах, новых `any` нет | +| smoke-select | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 33 слабые связи по общему имени `_markerDialog` (НЕОПРЕДЕЛЁННОСТЬ — ни одного прямого совпадения). Прогнал только именованный в AC `demo/smoke_value_face_source.mjs` — прямое совпадение по факту (это тот самый файл, который дифф меняет). Остальные 32 — общее имя `_markerDialog`, слабая связь; задача не задевает ничего вне диалога маркера и binding-выбора, прогон остальных не требовался | +| smoke_value_face_source | `node demo/smoke_value_face_source.mjs` (после `bundle:sync`) | все 16 полей `true`, включая новые `sameBindingKeepsSource`, `sameVirtualKeepsSource` | +| мутанты (новые, AC1/AC4) | `node scripts/mutation-gate.mjs --id=same-binding-click-resets-source`; `--id=release-proof-computed-for-every-commit` | оба **1/1 пойманы** | +| golden:verify | — | не прогонял: дифф не меняет рендер/геометрию/стили (только обработчики кликов и данные), подтверждено — `imageSha256` во всех сценариях `screenshots.json` не изменились, поменялся только `sourceFingerprint` | +| инварианты модели | — | не прогонял: дифф не трогает рёбра комнат, `layout`, `marker.space`, `open_spans`, толщину стен | +| backend pytest | `python3 -m pytest tests_backend/test_ha_import_export.py -q` | **не завершился штатно**: в этом окружении нет `.venv-backend`/`homeassistant`/`voluptuous` — сборка теста падает на `import voluptuous as vol` ещё на этапе коллекции (не skip, а ImportError). AC5 закрыт **чтением кода**, не исполнением: см. раздел ниже | +| py_compile (запасной для backend) | `python3 -m py_compile custom_components/houseplan/import_export.py tests_backend/test_ha_import_export.py` | чисто | + +## Разбор по AC (полный) + +**AC1** (клик по тому же кандидату в списке — no-op для value_source/badge, +другой кандидат — сброс как раньше). `houseplan-editor-runtime.ts:12305-12325`: +ранний `if (c.value === d.binding) { ...; return; }` перед сбросом. Мутант +`same-binding-click-resets-source` (возвращает безусловный сброс) — +**красный**, `smoke_value_face_source` — **зелёный** до и после мутации в +верном направлении (ловится). Регрессия (клик по другому кандидату всё ещё +сбрасывает) — поле `bindingResetToAuto`/`draftSourceExact`, оба `true`. +**Доказано автотестом, тест умеет падать.** + +**AC2** (то же для «радио virtual» — повторный клик по уже активному +virtual-binding не трогает value_source/badge). Реализовано симметрично AC1 +в `houseplan-editor-runtime.ts:12252-12260`: `if (d.binding === 'virtual') { +...; return; }`. Смок добавил поле `sameVirtualKeepsSource` +(`demo/smoke_value_face_source.mjs:176-183`) и оно зелёное. + +Но это доказательство **ложное** — проверено исполнением, а не только +чтением. Я вручную заменил условие `if (d.binding === 'virtual')` на +`if (false)` (эквивалент отсутствия фикса), пересобрал бандл +(`bundle:sync`) и перезапустил `smoke_value_face_source.mjs`: все 16 полей, +включая `sameVirtualKeepsSource`, остались **`true`** — мутация не поймана +(проверено также отдельным вызовом `page.evaluate` в реальном Chromium этого +проекта: клик по уже отмеченному `` не порождает событие +`change` вовсе, `count === 0` после двух `.click()` подряд). Причина +структурная, не флак: в `_markerDialog` инвариант `bindingMode === 'virtual' +⟺ binding === 'virtual'` держится во всех точках записи состояния +(`:7657`, `:7730`, `:12262`, `:12278` — при уходе в `bindingMode: 'ha'` +`binding` синхронно обнуляется, если был `'virtual'`). Значит `checked` +радиокнопки всегда synced с `d.binding === 'virtual'`, а клик по уже +отмеченной радиокнопке в браузере не эмитит `change` — ветка `if (d.binding +=== 'virtual')` в обработчике **не может исполниться ни при каком реальном +пользовательском клике**: это мёртвый код, а `sameVirtualKeepsSource` +истинно независимо от того, есть guard или нет. + +Само поведение при этом корректно (сброса на «virtual» из «virtual» никогда +не было и не может быть — по той же причине, по которой guard недостижим), +поэтому пользовательского дефекта нет. Дефект — в доказательстве: спека +(«Контракт (а)», раздел «Риски») прямо предписала чинить и радио-ветку с +условием `d.binding === 'virtual'`, а хендофф автора заявил AC2 доказанным +смоком — это не так, смок ничего не проверяет для этой ветки. + +**Medium (в скоупе задачи, не заводится отдельным issue — #202).** Ревьюер +обязан либо получить тест, который умеет падать, либо честную запись +«проверено чтением» (§2.7, правило 18) — здесь заявлено первое, а по факту +ни то, ни другое: тест не падает, а чтением автор не подтвердил. Правка на +выбор автора: (а) убрать недостижимый guard и строки `sameVirtualKeepsSource` +из смока, заменив комментарий honest-запиской «эта ветка не нуждается в +guard — see AC2 review», либо (б) оставить guard как документирующую +защиту-на-будущее, но заменить claim AC2 в спеке/хендоффе на «проверено +чтением: ветка недостижима через настоящий клик» вместо ложной ссылки на +смок. Блокирующих последствий для пользователя нет — находка про честность +доказательства AC, а не про баг. + +**AC3** (`rewriteMarkerControlReferences` не сажает `value_badge`/ +`value_source` как `undefined`). `src/devices.ts:889-898`: условные спреды. +Юнит `test/devices.test.mjs` (новый тест) проверяет `'value_badge' in +rewritten === false` и `'value_source' in rewritten === false` на голом +маркере, и что существующий `value_source` по-прежнему переписывается. +`npm test` зелёный. **Доказано автотестом**, падение проверено логически: +без условного спреда старый код писал бы ключ безусловно (`{ ...marker, +controls, value_badge: valueBadge, value_source: valueSource }`), что +провалило бы `'value_badge' in rewritten === false` — тест умеет падать по +построению (свойство `in` детектирует именно ключ с `undefined`). + +**AC4** (дорогая проверка гейта считается только для релизных коммитов, +предикат — тот же, что в `makeCommit`). `scripts/process-gate.mjs:109-121` +выносит `isReleaseCommit(subject, one)` целиком (оба дизъюнкта, включая +`Release:`-трейлер) — используется и в `makeCommit` (:147), и в +`parseRecords` (:169-172). Юнит со шпионом (`test/process-gate.test.mjs`, +новый тест) гоняет 3 коммита (обычный / бета-приёмка с `Release:`-трейлером / +стабильный релиз по subject) и проверяет: `isRelease` = `[false, true, +true]`, вычислитель вызван **только** для двух релизных (`calls` содержит +ровно их SHA), у нерелизного `releaseSourceViolations === null`. Мутант +`release-proof-computed-for-every-commit` (возвращает безусловный вызов) — +**красный**, `node --test test/process-gate.test.mjs` — зелёный. **Доказано +автотестом, тест умеет падать.** Ровно закрывает M1 из SPEC-REVIEW-385-r1 +(узкий предикат считал бы бета-приёмку нерелизной и ронял бы pre-push/CI +ложно). + +**AC5** (парная нейтрализация `value_badge`/`value_source` при экспорте, +`dropped_marker_links == 2`). `custom_components/houseplan/import_export.py:504-527` +— логика **не менялась** относительно `dev` (это тот же код, что уже был +принят в #378/#379); диф добавляет только объясняющий комментарий (:504-511) +и новый парный тест `tests_backend/test_ha_import_export.py: +test_issue_385_space_export_drops_badge_and_value_face_links_together`. +Прочитан построчно: маркер с внешним `value_badge.source.ref` **и** внешним +`value_source.ref` — оба ветвления (:512-520 бейдж, :521-527 value_source) +исполняются последовательно на одном и том же `marker`, каждое инкрементит +`dropped_marker_links`; итог `== 2` соответствует тесту. `badge["enabled"] = +False; badge["source"] = None` и `marker.pop("value_source", None)` дают +именно те поля/отсутствие ключа, что проверяет тест. + +Не смог исполнить: в этом окружении нет `.venv-backend`/`homeassistant` — +`python3 -m pytest tests_backend/test_ha_import_export.py` падает на +`ImportError: No module named 'voluptuous'` уже на коллекции, HA-харнесс +недоступен (только `py_compile`, который чист). **AC5 закрыт чтением кода, +не исполнением** — заявляю это честно, а не как «verified». Риск невысокий: +логика не менялась с версии, которую r1-код-ревью на `564bf419` уже гонял +исполнением (`pytest tests_backend/` 446 passed) в среде с харнессом, а +единственная новая часть — сам тест, чья корректность проверена построчным +сравнением с кодом выше. + +**AC6** (полный гейт зелёный, бюджет ≈ 0). См. таблицу гейтов — все зелёные, +бюджет 277 995 Б (запас 22 005 Б, дельта от 277 979 Б у автора — 16 Б, в +пределах шума версии Rollup/окружения). + +## Закрытие раунда r1 + +Документ r1: `docs/reviews/CODE-REVIEW-385-r1.md` (в дереве ветки, cherry-pick +коммитом `2e77fe09`). Вердикт r1: зелёный, `564bf419`, High 0 / Medium 0, +одна находка L1. + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| L1 (Low): вспомогательный `one(name)` в `process-gate.mjs` продублирован двумя реализациями (`makeCommit` через `matchAll`, `parseRecords` через `match`) — риск будущего дрейфа | Не закрыта, оставлена как есть — ревьюер r1 явно передал это на усмотрение автора вне скоупа AC4, не блокирует | `scripts/process-gate.mjs:129-130` (`all`/`one` через `matchAll`) и `:166` (локальный `one` через `match`) — оба выражения по-прежнему разные текстово, поведенчески идентичны, подтверждено новым юнитом AC4 (сверял лично) | + +r1 не содержал High/Medium — только это одно Low, поэтому таблица короткая: +r1 не отправлял задачу на правки (зелёный вердикт цикла не расходует, #227), +и переделывать было нечего — работу вернул только неудавшийся merge. + +## Унаследовано из r1 — не переоценивается, потому что переоценено с нуля + +Формально это полный, а не дельта-разбор (см. раздел выше), поэтому строгого +«наследования без проверки» здесь нет: AC1, AC3, AC4, AC5, AC6 перечитаны и +перепроверены гейтами заново в этом раунде на текущем SHA, а не приняты на +слово из r1. Единственное, что действительно наследуется без повторного +прогона, — сам факт, что r1 однажды провёл `pytest tests_backend/` целиком в +среде с HA-харнессом и получил 446 passed / 1 skipped / 1 error (ошибка — +известный флак `test_ha_upload.py::test_upload_ok`, не связан с диффом): +это использовано выше как historical подтверждение для AC5 в дополнение к +чтению кода, а не как замена сегодняшней проверки. Документ: r1, SHA +`564bf419` (недостижим в текущей истории после ребейза, но зафиксирован в +`docs/reviews/CODE-REVIEW-385-r1.md`). + +## Что проверено и корректно + +- Единственный коммит несёт оба трейлера, `User-Visible: yes` и правки в + оба changelog в нём же. +- Изменение видимого поведения ограничено пунктом (а); (б)–(г) действительно + не меняют вывод для пользователя — подтверждено чтением и тестами. +- Третьего места сброса value_source/badge при выборе binding нет: grep + `bindingMode:`/`binding: ` по всему файлу даёт ровно 4 точки записи + (`:7556` инбокс, `:7657`/`:7730` открытие диалога, `:12262`/`:12278`/`:12314` + переключатели) — все учтены, инвариант `bindingMode==='virtual' ⟺ + binding==='virtual'` держится всюду. +- Одно число — один источник: value_source/badge не создают дублирующего + отображения величины, эта задача не вводит новых видимых чисел. +- Никакой новой геометрии, i18n, миграции конфига, touch-контракта не + затронуто — согласуется со спекой. + +## Чего не проверял и почему + +- **Полный `pytest tests_backend`** (HA-харнесс) — недоступен в этом + окружении (`.venv-backend` отсутствует, `homeassistant`/`voluptuous` не + установлены). Заменено чтением кода + `py_compile` + опорой на прежний + прогон r1 (см. «Унаследовано»). Это честно называемый пробел, не «verified». +- **`golden:verify`** — дифф не меняет рендер (подтверждено неизменными + `imageSha256` в `screenshots.json`), пропуск обоснован §8. +- **Инварианты модели** (`scripts/model-invariants.mjs`) — дифф не трогает + геометрию/`layout`/`marker.space`/toлщину, не запускал. +- **31 из 33 «слабых» смоков** от `smoke-select.mjs` (общее имя + `_markerDialog`) — не прогонял: диф не касается ничего вне логики + binding-выбора и парных полей value_source/badge, которые целиком покрыты + прогнанным `smoke_value_face_source.mjs`. +- **Полный `mutation-gate.mjs`** — прогонял только два новых мутанта + (`--id=`), не весь набор: он для этого диффа не требуется целиком (это + предрелизный/тяжёлый гейт), плюс полный прогон в этой песочнице падает на + несвязанном шаге `backend-test-guard.mjs` (`No module named pytest`) до + установки `pytest` — окружение, не регрессия диффа. +- **`performance_smoke`** — в AC не назван, дифф не касается чувствительных + к перфу путей рендера (только обработчики кликов и статическая проверка + release-коммитов), не запускал. + +## Вердикт + +**Жёлтый.** Единственная находка — Medium (AC2, см. выше), в скоупе задачи, +не блокирует пользователя, но нарушает дисциплину «тест умеет падать» для +одного из шести AC и не позволяет мне подтвердить его честным «проверено +чтением» вместо ложной ссылки на смок, которую несёт хендофф. High нет. +Правка — на выбор автора (убрать недостижимый guard либо честно +перезаявить способ доказательства), повторный цикл код-ревью обязателен +(правило: правка по замечанию может задеть AC, который уже сочли +выполненным, — перепроверяю прицельно AC1/AC2 в r3).