19 KiB
CODE-REVIEW-113-r2 — honest optional contract for _spaceModel(), post-rebase re-check
- Issue: https://github.com/Matysh/houseplan-card/issues/113
- ТЗ:
docs/specs/113-optional-space-model.md, ревью ТЗ —docs/reviews/SPEC-REVIEW-113-r1.md(зелёный, Medium-1 → #184, не блокирует) - Предыдущий цикл:
docs/reviews/CODE-REVIEW-113-r1.md— зелёный, High 0, Medium 0. Слияние автоматически не прошло (конфликтissue/113-optional-space-model↔dev), поэтому issue вернулся вS6-in-progressне за переделку кода, а за ребейз (AGENTS.md «Two-agent workflow»: «после ребейза на сдвинутыйdevэто другой код, и принимать его без проверки нельзя»). - Диапазон:
origin/dev..HEAD,origin/dev=c1676cf(docs: review document for #117).git merge-base origin/dev HEAD=c1676cf— ветка полностью перебазирована, дивергенции нет. - Коммиты (5):
0c2a5de(specify contract),135497b(spec review doc),1e8503b(реализация — «Make empty space model explicit», это переехавший09b5a74из r1),047363c(r1 code-review doc),66fa8f4(новый после r1 — «Keep mutation anchor aligned after optional model change»). - Цикл: r2/4 (счётчик по этапу код-ревью; ревью ТЗ отдельно на r1)
- Роль: ревьюер кода (Claude), свежая сессия, без контекста реализации автора и без контекста собственного прогона r1.
1. Скоуп цикла r2
dev продвинулся между r1 и этим ребейзом: слиты #132/#157 (hosted/partition openings) и #173
(unified wall topology). Автор перебазировал issue/113-optional-space-model на новый dev
(c1676cf) и разрешил конфликты, сохранив обе стороны. Это другой код, чем тот, что получил
зелёный вердикт в r1 — задача этого цикла не «доверять r1», а заново пройти по чек-листу §2.7 на
итоговом дереве.
Diff origin/dev...HEAD (16 файлов) идентичен по содержанию r1 плюс один новый коммит:
66fa8f4меняет толькоscripts/mutation-gate.mjs: якорь мутацииsame-space-room-change-recentersиспользовал буквальный текстprevPos.s === targetSpace, но ребейз/реализация переименовала локальную переменную вtargetSpaceId(проверено чтениемsrc/houseplan-card.ts:12908,13018,13033— переменная везде называетсяtargetSpaceId, старого имениtargetSpaceв файле нет). Мутационный якорь — точное совпадение строки, поэтому несовпадающий текст сделал бы мутационный тест тихо неработающим (find не совпадает → патч не применяется → «поймано» покажет ложныйOK, а не реальную проверку).66fa8f4возвращает якорь к соответствию исходнику.Issue: #113/User-Visible: noна месте на всех 5 коммитах, включая новый.
Остальные 15 файлов — то же содержание, что было предметом r1 (проверено git diff: единственный
новый функциональный код появляется только в 66fa8f4). Часть строк diff'а (_saveMarker,
partitions) теперь физически стоит рядом с кодом #132/#157/#173, слитым в dev до ребейза — эти
call sites _spaceModel()/.partitions уже были в скоупе оригинальной реализации (видно по
targetSpaceModel, space.partitions/model.partitions в diff'е — не новые правки этого цикла,
перенеслись без изменений при ребейзе). Отдельно проверено: слитый в dev код не добавил новых
необработанных обращений к _spaceModel() вне diff'а этой ветки (см. §3).
User-Visible: no остаётся в силе без изменений (§13 ТЗ), changelog не трогается — согласовано.
2. Как проверялось — таблица гейтов
| Гейт | Команда | Результат |
|---|---|---|
| Typecheck | npx tsc --noEmit |
OK, без ошибок |
| Unit | npm test |
877/877 OK (было 811/811 в r1 — рост за счёт тестов, слитых в dev вместе с #132/#157/#173, не связан с этим диффом) |
| Build + 3 копии бандла | npm run build, затем cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js и cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js |
OK, обе сверки — байт-в-байт совпадение |
| Именованный smoke (AC3–AC6) | node demo/smoke_optional_space_model.mjs |
OK, все 9 подпроверок true |
| Именованный smoke (AC6, read-only cold start) | node demo/smoke_readonly_cold_start.mjs |
OK, все 13 подпроверок true |
| Мутационный guard AC10/AC4/AC5 (не менялся этим циклом) | node scripts/mutation-gate.mjs --id=empty-space-cleanup-disabled |
поймано 1 из 1, чистый прогон зелёный |
Мутационный guard, якорь которого правит 66fa8f4 |
node scripts/mutation-gate.mjs --id=same-space-room-change-recenters |
поймано 1 из 1 — якорь совпал с исходником, патч применился, smoke_subarea.mjs покраснел, как обязан. Это прямое подтверждение, что правка якоря не сделала тест немым |
| Именованный smoke затронутого якоря | node demo/smoke_subarea.mjs (чистое дерево) |
OK, все 14 подпроверок true |
Regression: поверхности, слитые из #132/#157/#173, рядом с которыми правился diff (_saveMarker/partitions) |
node demo/smoke_partition_openings.mjs, node demo/smoke_unified_wall_tool.mjs, node demo/smoke_room_autoclose.mjs |
OK, все зелёные |
| Process gate | node scripts/process-gate.mjs --issues |
диапазон origin/dev..HEAD, коммитов 5, гейт пройден, предупреждений 0 |
| Контракт call sites | grep -n "_spaceModel()!" src/houseplan-card.ts и grep -n "_spaceModel()\s*\." src/houseplan-card.ts | grep -v "?\." |
пусто — ни non-null assertion, ни naked deref после _spaceModel() в production source |
Не прогонялось, и почему:
- Полный набор 135(+) браузерных смоков — не расширился в этом цикле относительно r1: этот
ребейз не добавил новых поверхностей, кроме тех трёх (
partition_openings,unified_wall_tool,room_autoclose), которые были рядом с изменёнными строками и прогнаны выше. Остальное логическое обоснование r1 (guard, недостижимый при непустой модели) не изменилось — новый функциональный код этого цикла ограничен одной правкой строки в тестовом скрипте, не в продукте. npm run golden:verify— не прогонялся; тот же аргумент, что и в r1 (guard добавлен до существующей логики и физически недостижим при непустой модели), плюс этот цикл не меняет ни одной строкиsrc/houseplan-card.tsотносительно r1 — толькоscripts/mutation-gate.mjs(класс B, не влияет на визуальный результат).python -m pytest tests_backend— ни один файлcustom_components/**/*.pyне тронут ни в этом коммите, ни в диапазоне.- Performance-профили — не названы в AC, hot path не тронут этим циклом (единственная правка — тестовый скрипт).
- Остальные смоки вне таблицы — сознательно не прогонялись; сужение соразмерно объёму нового кода
цикла (одна строка в
mutation-gate.mjs), а не всей задаче #113 заново.
3. Проверка AC (docs/specs/113-optional-space-model.md §10) — переподтверждение после ребейза
Продуктовый код (src/houseplan-card.ts, src/space-model-selection.ts) идентичен принятому в r1
(проверено git diff — единственный новый коммит цикла трогает только scripts/mutation-gate.mjs),
поэтому доказательства AC1–AC9 из r1 переносятся без изменений; ниже — что перепроверено заново на
итоговом дереве, а не просто процитировано.
| AC | Статус в r2 |
|---|---|
| AC1–AC2 (optional тип, отсутствие non-null assertions) | перепроверено заново: tsc --noEmit зелёный на итоговом дереве; grep на _spaceModel()!/naked deref — пусто (см. §2) |
| AC3–AC6 (empty lifecycle, delete-last, pending action abort, read-only cold start) | перепроверено исполнением: smoke_optional_space_model.mjs, smoke_readonly_cold_start.mjs — оба зелёные на итоговом дереве после ребейза |
| AC7 (non-empty View/editors pixel/action parity) | перепроверено smoke-регрессией на поверхностях, слитых из #132/#157/#173 и физически соседствующих с diff'ом (partition_openings, unified_wall_tool, room_autoclose, subarea) — все зелёные; логический аргумент r1 (guard недостижим при непустой модели) не изменился, т.к. продуктовый код не менялся |
| AC8 (stale explicit id не мутирует первый space) | код не менялся; test/space-model-selection.test.mjs в составе зелёного npm test |
| AC9 (active-id fallback) | код не менялся; тот же unit-тест |
| AC10 (type/source gates ловят регрессию сигнатуры) | перепроверено исполнением заново — empty-space-cleanup-disabled (не менялся) и, отдельно, мутационный тест, чей якорь чинит этот цикл (same-space-room-change-recenters): подтверждено, что после правки якорь совпадает с исходником и мутация по-прежнему ловится (§2, §4) |
4. Дисциплина «тест умеет падать» — что именно проверено исполнением в этом цикле
Ядро этого цикла — не переисполнение доказательств r1 (код там не менялся), а верификация того, что
единственная новая правка (66fa8f4) не превратила существующий мутационный гейт в фиктивный:
- До правки старый якорь (
prevPos.s === targetSpace) не совпал бы ни с одной строкой текущегоsrc/houseplan-card.ts(переменная называетсяtargetSpaceIdво всех трёх местах использования — проверено grep'ом). Несовпадающийfindвscripts/mutation-gate.mjsне бросает ошибку — он просто не применяет патч, тест против чистого дерева проходит с эффективно нулевым мутационным покрытием, апоймано 1 из 1в CI осталось бы зелёным без реальной проверки — то есть перед66fa8f4этот конкретный guard тихо перестал бы что-либо ловить. - Прогон
node scripts/mutation-gate.mjs --id=same-space-room-change-recentersна текущем дереве подтверждает обратное:findсовпал, патч применился,smoke_subarea.mjsпокраснел — тест умеет падать по факту исполнения, не предположительно. - То же самое для
empty-space-cleanup-disabled(якорь не менялся этим циклом, но перепрогнан заново на итоговом дереве после ребейза, а не процитирован из r1):поймано 1 из 1.
5. Находки
Нет находок High. Нет находок Medium.
Два Low из r1 (стейл _splitSel/_mergeDialog/_wallDialog при пустом переходе; недостижимый
защитный if (!space) return nothing в render()) относятся к продуктовому коду, который в этом
цикле не менялся — они разобраны и сняты в docs/reviews/CODE-REVIEW-113-r1.md §5, повторно не
переоткрываю.
6. Что проверено и корректно
git merge-base origin/dev HEADсовпадает сorigin/dev— ветка действительно линейно перебазирована, диапазонorigin/dev..HEADне содержит посторонних коммитов и не пропускает ни один слитый коммит основной линии.- Продуктовый diff (
src/houseplan-card.ts,src/space-model-selection.ts,docs/ARCHITECTURE.md,docs/TESTING.md, тесты, smoke) идентичен r1 построчно — единственная дельта цикла ограниченаscripts/mutation-gate.mjs(класс B). - Call sites, физически расположенные рядом со слитыми из #132/#157/#173 фичами (
_saveMarkertargetSpaceModel/targetSpaceId,.partitionsв_openPairs/соседних хелперах), уже были частью оригинальной классификации #113 (видно поtargetSpaceModel/space.partitionsв diff'е — не новые изменения этого цикла); слитый код сам по себе использует новую optional-сигнатуру корректно там, где обращается к_spaceModel()(например,_spaceModel()?.partitionsв коде, пришедшем изdev) — проверено чтением и не нарушено ни одним из трёх smoke по этим поверхностям. docs/ARCHITECTURE.md: баннер «Updated» корректно продвинут на дату ребейза, инвариант optional-space описан консистентно с кодом;docs/TESTING.mdчек-лист ссылается на реальные smoke/unit/mutation имена, все они существуют и зелёные.- Трейлеры на всех 5 коммитах диапазона:
Issue: #113,User-Visible: no— включая новый66fa8f4.node scripts/process-gate.mjs --issuesзелёный, 0 предупреждений. tsconfig.test.jsonиdocs/specs/README.md— механические регистрации нового файла/записи, соответствуют содержимому.
7. Чего не проверял
- Полный браузерный набор смоков и
golden:verify— обоснование в §2 (нулевой продуктовый diff относительно r1, узкий новый код цикла — одна строка в тестовом скрипте). pytest tests_backend— класс A/B бэкенда не тронут.- Performance-профили — не названы в AC, hot path не тронут ни в r1, ни в этом цикле.
- Содержательный повторный аудит AC1–AC9 «с нуля» без опоры на r1 — не требуется: код, который они
доказывают, не менялся байт в байт (проверено
git diffмежду продуктовым кодом r1 и текущим деревом), поэтому это не «доверие прошлому вердикту», а подтверждённый факт отсутствия изменений в этой части. - Medium-1/#184 (fallback-семантика ТЗ §6) — по-прежнему отдельный issue, не в скоупе этого код-ревью и не в скоупе #113.
8. Вердикт
Ребейз на c1676cf не изменил ни строки продуктового кода #113: единственная новая правка цикла —
восстановление мутационного якоря, сломанного переименованием переменной в результате мержа. Правка
проверена исполнением на способность ловить регрессию (не голословно), все фаст-гейты и релевантные
smoke зелёные на итоговом дереве, AC1–AC10 подтверждены заново (без изменений там, где код не
менялся, и напрямую — там, где менялся). High: 0. Medium: 0.
Вердикт: зелёный · цикл r2/4 · High: 0 · Medium: 0 → нет · Документ: docs/reviews/CODE-REVIEW-113-r2.md