german/houseplan-card
1
0
Fork 0
mirror of https://github.com/Matysh/houseplan-card synced 2026-09-29 03:09:36 +00:00
Code Issues Actions 6 Packages Projects Releases Wiki Activity

docs: review document for #316

Browse Source 
Issue: #316
User-Visible: no
This commit is contained in:
claude[bot]
2026-08-26 23:41:35 +03:00
committed by Codex
parent dff476ff35
commit 1d92193c5d
1 changed files with 321 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/reviews/CODE-REVIEW-316-r2.md
+321
View File
@@ -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 <SHA r1>..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 → в задаче**