diff --git a/docs/reviews/CODE-REVIEW-316-r2.md b/docs/reviews/CODE-REVIEW-316-r2.md new file mode 100644 index 00000000..34cf8365 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-316-r2.md @@ -0,0 +1,321 @@ +# CODE-REVIEW-316-r2 + +Issue: [#316](https://github.com/Matysh/houseplan-card/issues/316) — «Нельзя нарисовать комнату на пустом плане: отказ «пространство не преобразовано, проём нельзя однозначно привязать к стене»». +Ветка: `issue/316-migration-auto-resolve`. HEAD на момент ревью: `974796117e50530b63e3a2acc81b15ad66fe2906` (`97479611`), сверено `git rev-parse HEAD` непосредственно перед вынесением вердикта (§2.7/#312). +Заход: **r2**. Циклов израсходовано до этого раунда: 1/4 (r1 — красный). + +Спецификация: `docs/specs/316-opening-host-auto-resolution.md`, ревизия 5 (зелёное spec-ревью r4, `docs/reviews/SPEC-REVIEW-316-r4.md`). + +## Скоуп раунда и почему разбор полный, а не по дельте + +Раунд r2 обязан начинаться с дельты: `git diff ..HEAD` (§2.10). Вердикт r1 +(19:34:52) назвал SHA прямым текстом — `ee672dcd` (после ребейза на `dev`). Эта +команда не выполняется: + +``` +$ git rev-parse ee672dcd +fatal: ambiguous argument 'ee672dcd': unknown revision or path not in the working tree. +$ git fetch origin ee672dcd +fatal: couldn't find remote ref ee672dcd +``` + +SHA, на котором получен вердикт r1, **не существует** в репозитории — ни как +достижимый коммит, ни как объект, который можно дозапросить у origin. Дельту +по протоколу §2.10 построить невозможно. Это не «SHA не назван» (та находка +уже описана в задании как образцовая) — это находка на ступень хуже: SHA был +назван, но история, на которую он указывал, перестала существовать. Разбор +поэтому идёт **полным**, по `origin/dev...HEAD` — ровно тот случай, для +которого протокол прямо разрешает полный разбор («сомневаешься — разбирай +полностью и скажи почему»), и попутно это और оформлено как находка H1 ниже, +потому что причина не в объёме дельты, а в целостности самой истории. + +## H1 (блокирует) — история ветки переписана поверх уже отревьюженного коммита r1 + +**Что произошло.** Комментарий «r1 → r2: H1 и M1 закрыты» (2026-08-26T19:46:03Z) +заявляет `HEAD \`97479611\`` как результат починки — то есть новый коммит поверх +отревьюженного `ee672dcd`. Фактически `974796117e50530b63e3a2acc81b15ad66fe2906` +— это **тот же самый коммит**, который конвейер запушил сразу после вердикта +r1 (`docs: review document for #316`, добавляет только +`docs/reviews/CODE-REVIEW-316-r1.md`, 348 вставленных строк, ни одной другой +правки): + +``` +$ git show --stat 97479611 +commit 974796117e50530b63e3a2acc81b15ad66fe2906 + docs: review document for #316 + Issue: #316 + User-Visible: no + docs/reviews/CODE-REVIEW-316-r1.md | 348 +++++++++++++++++++++++++++++++++++++ +``` + +Никакого нового коммита с фиксом H1/M1 в истории `issue/316-migration-auto-resolve` +нет — `git log --oneline` от `97479611` до корня ветки ровно тот же список, +что был до раунда r1. Фикс, который комментарий описывает («деградированный +пул удалён целиком», «новый TS-тест», «новый pytest»), физически лежит +**внутри исходного коммита реализации** `85ad323b` — того самого, что +рецензировался в r1 под именем `ee672dcd` (это подтверждает независимая +проверка: тот коммит из спецификации ссылается на #319-контекст и трейлер +`User-Visible: yes`, единственный такой в ветке). + +Прямое доказательство переписывания — сам коммит `85ad323b` противоречит +собственной шапке. Заголовок: «Implements spec revision 4 (green r4)». Но +диф этого же коммита правит `docs/specs/316-opening-host-auto-resolution.md` +на: + +``` +-Статус: ревизия 4 (r1: M1/M2/Low; r2: M3/M4; r3: M5). ++Статус: ревизия 5 (spec-ревью r1–r4; код-ревью r1: H1 — §3.3 без дальнего поиска). +``` + +и добавляет в §3.3/§6 формулировки «отклонён код-ревью r1 (H1)» — то есть +коммит, датированный (author date) 2026-08-26 21:38:42+03:00, за 56 минут +**до** вердикта r1 (19:34:52 UTC = 22:34:52+03:00), уже содержит ссылку на +находку из этого будущего вердикта. Это возможно только одним способом: +коммит был **переписан задним числом** (interactive rebase/amend), а не +дополнен новым. Commit date (не author date) подтверждает то же самое — +`85ad323b`, `a2234e40` и текущий `97479611` синхронно получили одинаковый +`CommitDate 2026-08-26 22:39:28+0300` (19:39:28 UTC — между вердиктом r1 +19:34:52 и комментарием автора 19:46:03), при том что `AuthorDate` `97479611` +формально принадлежит `claude[bot]` и датирован 19:35:05 — коммит конвейера +был пересобран в той же операции, что и коммит реализации, и в итоге получил +**новый** хеш, который просто совпал по видимому сообщению со старым. + +**Почему это блокирует, а не просто дефект оформления.** + +1. Ломает именно тот механизм, ради которого написан весь протокол §2.7 + (issue #312) и §2.10 (issue #214/#171/#207/#102): вердикт привязан к SHA, + повторный раунд обязан объявить и проверить дельту от этого SHA. Здесь + SHA стал недостижим — не только этому ревью, а вообще любому будущему + аудиту, включая владельца. +2. Комментарий автора описывает несуществующие коммиты («новый TS-тест», + «новый pytest») как факт истории. Содержательно тест и правка на месте + (проверено ниже, по существу они верны) — но *способ*, которым они туда + попали, не то, что написано: не новый коммит поверх ревью, а тихая замена + уже прочитанного коммита. +3. Сообщение `85ad323b` осталось «Implements spec revision 4» — коммит лжёт + о собственном содержимом: он реализует ревизию 5 и содержит текст, + написанный в ответ на код-ревью, которого при заявленной дате ещё не было. +4. Прецедент опасен сам по себе: если переписывание уже отревьюженного + коммита закрывает находку красного вердикта без следа, лимит 4 циклов + (§4) и весь аппарат «цикл считается по возврату с блокирующей находкой» + становится необязательным к соблюдению для истории задачи — можно всегда + стереть то, что не понравилось ревьюеру. + +**Что не является находкой здесь.** Само по себе H1(r1) — по существу +исправлено корректно (раздел «Закрытие раунда r1» ниже, проверено чтением и +исполнением, не по заявлению автора). Претензия — только к тому, *как* +исправление попало в историю, и к тому, что протокольный механизм дельты +это исправление сделало недоказуемым обычным путём. + +Серьёзность: **High**, в скоупе задачи (это тот же issue #316, тот же +раунд ревью которого он касается) — блокирует зелёный вердикт, возврат +автору. + +## M2 (Medium, в скоупе) — бэкенд-релаксация host-проверки шире границы, заявленной в §2 ТЗ + +`custom_components/houseplan/validation.py`, `_config_wall_segment_invariants` +(строки 1830–1836), было: + +```python +host = opening.get("host") +if host is None: + raise vol.Invalid("v8+ opening requires an explicit host") +``` + +стало: + +```python +host = opening.get("host") +if host is None: + # #316 §3.3: a degraded migration may leave a contour opening + # unhosted. ... + continue +``` + +Это единственная общая проверка `host` для **всех** проёмов пространства — +контурных и партиционных (`host.kind === 'partition'`, #132) — она стоит в +`CONFIG_SCHEMA`, то есть выполняется на **каждой** записи, не только на +initial migration. ТЗ §2 явно фиксирует границу: «Раздел „Independent-wall +opening host (#132)“ (партиционные проёмы) не затрагивается». Код это не +соблюдает буквально: правка ослабляет проверку для любого проёма без `host`, +включая гипотетический партиционный проём, у которого `host` оказался бы +пуст не по сценарию §3 (initial migration), а по любой другой причине. + +Практический риск на сегодня ограничен: единственный реальный писатель, +удаляющий independent-wall («Толщина» → `_confirmPartitionDelete`, +`houseplan-card.ts:8399-8414`), при удалении партиции **удаляет** её +проёмы целиком (`sp.openings = ... .filter(...)`), а не оставляет их с +`host: undefined` — то есть сегодня продукт не производит партиционный +проём без host по живому пути. Единственное место, где такое состояние +существует, — golden-харнесс `coincident-partition-virtual-dark` +(`demo/golden/harness.mjs:100-103`), и то это чисто презентационная сцена +(`mode: 'view'`), которая никогда не проходит через `CONFIG_SCHEMA` (это же +установил spec-review r3, M5). Регрессионного случая сегодня нет — но схема +больше не отличает «контурный проём, легитимно деградировавший по §3.3» от +«партиционный проём с потерянным host по какой-то другой, ещё не написанной +причине», и не покрыта тестом, который бы поймал будущее смешение. + +Рекомендация — не полноценный отдельный чек (kind неизвестен, когда host +отсутствует, различить нечем), а тестовое закрепление того, что уже верно: +регрессионный backend-тест «партиционный проём без host, полученный НЕ через +initial-migration путь §3, всё равно проходит запись» (документирует принятое +решение) **или** уточнение §5.1 ТЗ, что относится к границе точнее — сейчас +формулировка «раздел #132 не затрагивается» технически не соответствует +диффу. В скоупе, чинится тем же коммитом, не требует пересмотра §3/AC. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| **H1** (деградированный пул выбирает дальнюю стену без ограничения расстояния → бэкенд-схема `wall opening geometry must match its host` рвёт запись) | Пул дальних кандидатов удалён из `migrateRoomOpeningHost`: критерий `eligible` теперь идентичен `resolveRoomOpeningHost` (одинаковые допуски по расстоянию/углу/вместимости); без кандидата «на месте» проём сразу уходит в unhosted | `src/wall-segment-model.ts:665-704` (комментарий строк 693-697 прямо называет причину); питон-зеркало `custom_components/houseplan/wall_segment_model.py:570-600`; проверено повторным исполнением найденного в r1 репро (см. «Проверено исполнением» ниже) — результат теперь `host отсутствует`, схема принимает, идемпотентно | +| **M1** (CHANGELOG заявляет «works everywhere», что не соответствовало деградированному случаю) | Формулировка не менялась; после удаления деградированного пула утверждение стало верным для всех случаев без исключения | `docs/CHANGELOG.md:10-15`, `docs/CHANGELOG.ru.md:17-21` — «an opening with no usable wall at all is kept as data — inert until you re-place it», это буквально то, что теперь происходит всегда | + +Обе находки закрыты по существу — но, как описано в H1 этого раунда, способ, +которым они попали в историю ветки, сам стал новой блокирующей находкой. + +## Унаследовано из r1 + +Раунд не строился как дельта (H1 выше объясняет почему), поэтому ничего не +принято молча «по доверию» — весь диф `origin/dev...HEAD` (26 файлов, ++2309/-139) заново прочитан и перепроверен в этом раунде, включая нормативные +правила §3.1–3.4 и AC1–AC6, которые r1 уже проверял. Формально наследовать +нечего; список ниже — что из выводов r1 подтвердилось повторно, а не было +принято без проверки: + +- §3.1/§3.2/§3.4 (атомизация, tie-break, «миграция не кидает opening-host») — + логика, которую сам r1 не находил проблемной, перечитана в + `src/wall-segment-model.ts` заново в составе полного разбора; изменений с + r1 в этой части нет (единственная правка диффа между r1 и текущим HEAD — + сама H1-часть §3.3, разобранная выше). +- Golden-сцена `span-over-door-migrated-dark` и неизменность + `coincident-partition-virtual-dark` — перепрогнаны в этом раунде + (`npm run golden:verify`, 130/130 `passed`, обе сцены `diffRatio: 0`), не + унаследованы со слов автора. + +## AC — проверка + +- **AC1** (репродукция #316 разрешена). Смок `demo/smoke_zero_wall_migration_unblocked.mjs` + воспроизводит ровно исходный баг-репорт: пространство A пустое, B — комната + с `open_span` вдоль стены и дверью на ней; два клика «Стены» в A. Прочитан + целиком, сценарий совпадает с шагами репродукции из аналитики; прогнан — + **green**. +- **AC2** (§3.1: дверь удерживает стену, span вокруг неё — `cm:0`, идемпотентно). + `test/wall-segment-model.test.mjs:415-429` — проверяет разбиение атома на + три части (`0, >0, 0`), `host` на среднем атоме, `commit(commit(x))==commit(x)`. + Дублируется бэкенд-тестом `test_v8_open_span_over_an_opening_spares_the_carrying_atom` + (`tests_backend/test_wall_segment_model.py:379+`). Доказано юнит-тестом; + мутант `span-cut-erases-the-door-wall` зарегистрирован и его guard-патч + применяется чисто (`node scripts/mutation-gate.mjs --check` — ok), сам факт + «тест умеет падать» на этом мутанте зафиксирован в хендоффе автора и не + переисполнялся заново (дорогой гейт, предрелизная обязанность, см. «Гейты»). +- **AC3** (§3.2: неоднозначность на стыке толщин, детерминированный выбор, + идемпотентно). `test/wall-segment-model.test.mjs:431-459` — два контурных + атома стыкуются на смене `cm:10→20`, проём точно на стыке (дистанция 0 до + обоих) → побеждает больший `cm` (20), стабильно при повторном прогоне. + Фикстура реально достижима внутри `resolveRoomOpeningHost` (то, чего не было + в ревизии 1 — M2 spec-ревью). Доказано юнит-тестом, прочитано и логически + проверено (тай-брейк соответствует §3.2 дословно). +- **AC4** (§3.3: проём вдали от всех стен → unhosted, схема принимает, + повторная запись сохраняет). Фронт: `test/wall-segment-model.test.mjs:461-490` + (два случая — «нет кандидатов вовсе» и «есть дальний той же оси, но не + подходит по правилу H1»). Бэкенд: `test_unhosted_contour_opening_is_a_valid_degraded_v9_state` + и `test_far_same_angle_wall_is_not_a_degraded_carrier` + (`tests_backend/test_wall_segment_model.py`). Backend-харнесс в среде ревью + недоступен (`ModuleNotFoundError: No module named 'homeassistant'`, как и в + r1) — компенсировано прямым исполнением `wall_segment_model.py` + + `validation.py` (только `voluptuous`, установлен отдельно) в обход + package `__init__.py` (у него HA-импорт). Воспроизведён ровно сценарий + r1's H1 (room без соседних стен-кандидатов, проём вдали) и ровно сценарий + теста `test_far_same_angle_wall_is_not_a_degraded_carrier`: + ``` + host present: False + schema OK, host in validated: False + idempotent: True + ``` + То есть находка r1 (падение на `CONFIG_SCHEMA`) в текущем коде не + воспроизводится — исправлена по существу. Доказано исполнением, не + заявлением. +- **AC5** (пост-v9 запись, потерявшая носителя, остаётся fail-closed). + `test/wall-segment-model.test.mjs:492-503` и + `test_post_v9_write_that_lost_its_carrier_keeps_the_refusal` — `host` + указывает на несуществующий id → `WallSegmentModelError('opening-host')`/ + `WallSegmentMigrationError`. Мутант `migration-throws-over-an-opening-again` + зарегистрирован, guard применяется чисто; красный прогон зафиксирован в + хендоффе автора, не переисполнялся (дорогой предрелизный гейт). +- **AC6** (идемпотентность на фикстурах AC1–AC4). Каждый юнит-тест выше + заканчивается `commit(commit(x)) === commit(x)`; отдельно — + `test/wall-segment-model.test.mjs:505-511` сверяет байт-в-байт готовую + фикстуру `test/fixtures/316-span-over-door-migrated.json` (`stored` → + `migrated`). Прочитана фикстура: 6 `wall_segments`, один хостовый проём — + не вырожденный случай. + +Все AC1–AC6 подтверждены — частично автотестом, что тест умеет падать +подтверждено для новых мутантов через реестр (`--check`, применимость +патчей) и хендофф автора (полный дорогой прогон — предрелизная обязанность, +см. ниже), частично прямым исполнением backend-модулей в обход недоступного +HA-харнесса. + +## Гейты — что прогнано и почему + +Всегда (§8, дёшево, прогнано в этом раунде заново, а не унаследовано): + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | green, 0 ошибок | +| Unit | `npm test` | 1352/1352 green (+1 skip, не по диффу — тот же не относящийся к #316 skip, что видел r1) | +| Build + 3 копии бандла | `npm run build && npm run bundle:sync` + `cmp` попарно (`dist/`, `custom_components/.../frontend/`, `demo/srv/assets/`) | все три идентичны, `git status` чист | +| `check-docs` (диф трогает `src/**`) | `node scripts/check-docs.mjs` | green, 7 файлов/10 внешних ссылок | +| `model-invariants` (диф трогает геометрию/`host`) | `node scripts/model-invariants.mjs --config <мигрированная фикстура #316>` | `Инварианты выполнены: ссылки разрешимы, записи толщины находятся.` (JSON: `violations: []`); первый прогон случайно указывал прямо на файл-обёртку `{comment,stored,migrated}` без `.spaces` и был бы ложно-зелёным вхолостую — перезапущен на распакованном `migrated`-конфиге, чтобы проверка была не вырожденной | + +По необходимости (определено диффом/AC): + +| Гейт | Команда/выбор | Результат | +|---|---|---| +| Смоки | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → прямое совпадение (`smoke_edit_walk`, `smoke_glow`, `smoke_junction_holes`) + зарегистрированная связь (`smoke_real_plan_masonry`, `smoke_resize_pointer_real_plan`, `smoke_resize_wall_thickness`) + AC1-смок `smoke_zero_wall_migration_unblocked` | все 7 green (`smoke_edit_walk` уложился только с таймаутом 180с — сам смок долгий, не флаки) | +| `golden:verify` (диф меняет зонирование `cm:0`, то есть видимую геометрию) | `npm run golden:verify` (полная матрица, дорого, но правило требует «полная выборка неуместна только когда задача не задевает всё» — здесь задевает зонирование во всей матрице, а не в одной сцене) | 130/130 `passed`; `span-over-door-migrated-dark` (новая сцена) и `coincident-partition-virtual-dark` (контрольная неизменная) — обе `diffRatio: 0` | +| Backend pytest | `python -m pytest tests_backend -q` | **недоступен в среде ревью** (`ModuleNotFoundError: No module named 'homeassistant'`) — как и в r1. Компенсировано прямым исполнением `wall_segment_model.py`/`validation.py` в обход `custom_components/houseplan/__init__.py` (см. AC4) — этим воспроизведена и закрыта находка r1 H1 | +| Mutation gate (дорогой, предрелизный по собственной документации скрипта) | `node scripts/mutation-gate.mjs --check` (дешёвая часть — применимость патчей) | ok для всех, включая два новых мутанта `span-cut-erases-the-door-wall`, `migration-throws-over-an-opening-again`; **полный** прогон (пересборка бандла на мутанте) не запускался — это предрелизная обязанность по собственному комментарию скрипта, red-результат уже зафиксирован в хендоффе автора | + +## Одно число — один источник + +Диф не добавляет и не меняет видимую пользователем величину (площадь, +подпись, подсветку) — только структурную геометрию (`cm:0`/`>0` зонирование +и `host`), которая уже проверяется golden. `test/single-source-numbers.test.mjs` +не затронут диффом и не требовался. + +## Чего не проверял + +- Полный `npm run golden:verify` предыдущего раунда (r1) не переисполнялся + заново для сравнения «до/после этого раунда» — правка между r1 и текущим + HEAD (по факту, не по декларации) ограничена §3.3 (без дальнего поиска) и + не меняет зонирование §3.1, поэтому новый прогон golden в этом раунде + (130/130) достаточен и актуален. +- Полный прогон `mutation-gate.mjs` (пересборка бандла на каждом мутанте) — + дорогой предрелизный гейт, не гейт ревью; ограничился `--check` + (применимость патчей) плюс зафиксированным в хендоффе автора красным + результатом для двух новых мутантов. +- `python -m pytest tests_backend -q` буквально — недоступен здесь так же, + как в r1; заменено прямым исполнением тех же модулей с ручным + воспроизведением конкретных тестов (см. AC4), это не полный прогон полного + файла тестов, а целевая проверка найденного в r1 сценария и одного нового. +- Смоки вне выборки `smoke-select.mjs` + AC1 (135 из 192 в проекте) — не + прогонялись; диф локален к `wall-segment-model.ts`/бэкенду, широкого + геометрического регресса выборка не показала (только прямые совпадения и + зарегистрированные связи, без «широкого» символа >38 смоков). +- Не проверялось, существует ли ещё на сервере GitHub объект `ee672dcd` + (недостижимый локально) — `git fetch origin ee672dcd` отказал; вывод + H1 не зависит от того, жив он там ещё несколько дней или уже нет. + +## Вывод + +AC1–AC6 выполнены и доказаны — частично автотестом с проверенной +способностью падать, частично прямым исполнением backend-модулей в обход +недоступного HA-харнесса. Находка r1 (H1: дальний host ломает бэкенд-схему) +закрыта по существу и переисполнением подтверждена. Но раунд не может быть +зелёным: способ, которым это закрытие попало в историю ветки — переписывание +уже отревьюженного коммита `ee672dcd` вместо нового коммита поверх него, — +делает недостижимым тот самый SHA, к которому привязан вердикт r1, и +противоречит собственному тексту коммита. Плюс один Medium в скоупе +(бэкенд-релаксация host-проверки шире заявленной в ТЗ границы #132). + +**Вердикт: красный · заход r2 · блокирующих циклов 2/4 · High: 1 · Medium: 1 → в задаче**