mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -0,0 +1,248 @@
|
||||
# CODE-REVIEW-500-r1
|
||||
|
||||
- **Issue:** #500 — Архитектура: выделить одну границу владения config/adoption
|
||||
- **Материал:** `0905f67ec0d8fb830065e909988f442c884320c2` (ветка `issue/500-config-adoption-boundary`, ребейз на `origin/dev` = `0c748282`)
|
||||
- **Диапазон:** `git log --oneline origin/dev..HEAD` — 10 коммитов (4 ТЗ/докс-ревью + 1 реализация модуля + делегаты + 7 вызывающих + 1 тесты/lint/docs + 1 фикс реактивности)
|
||||
- **ТЗ:** тело `docs/specs/500-config-adoption-boundary.md`, зелёное ревью r3 (`docs/reviews/SPEC-REVIEW-500-r3.md`)
|
||||
- **Заход:** r1 · блокирующих циклов израсходовано 0/4 (это первый заход)
|
||||
|
||||
## Скоуп
|
||||
|
||||
Извлечение владения идентичностью `{config, rev, fingerprint}` × `{layout, rev,
|
||||
fingerprint}` в новый модуль `src/config-adoption.ts`; единая гейт-
|
||||
последовательность `adoptAuthoritativeGated` (compare → prepareImage →
|
||||
continuity → adopt → хвост профиля) для семи вызывающих в двух профилях
|
||||
(`reload`/`post-write`); сужение `SummaryPanelHost` и host-контрактов
|
||||
editor-/onboarding-runtime; перенос `rollbackOptimistic` в модуль; единственное
|
||||
намеренное изменение поведения — гейт готовности фона на четырёх post-write
|
||||
путях, ревизии `space/delete` берутся из `config/get`/`layout/get`, а не из
|
||||
ответа `delete`. `User-Visible: no` на всех коммитах, changelog не тронут.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
**Дешёвые гейты** — не гонял заново: Validate на точном материале ревью
|
||||
(`0905f67e`) зелёный целиком —
|
||||
https://github.com/Matysh/houseplan-card/actions/runs/34462124328 (`headSha`
|
||||
подтверждён `gh run view` перед использованием результата). Это покрывает
|
||||
`tsc --noEmit`, `npm test` (2494+ юнитов), `npm run build` + сверку бандл-
|
||||
деревьев, `no-new-any`, `process-gate`, `check-docs` (WARN о протухшем
|
||||
скриншот-отпечатке — это предупреждение обычного push, не гейт кандидата,
|
||||
блокирующим не является).
|
||||
|
||||
**Мутанты по диффу** — проверил логи всех трёх шардов `Мутанты по диффу
|
||||
(N/3)` этого же прогона (`gh run view --job <id> --log`), а не только факт
|
||||
green: все четыре новых мутанта пойманы на материале ревью —
|
||||
|
||||
| Мутант | Файл лога (шард) | Результат |
|
||||
|---|---|---|
|
||||
| `config-adoption-echo-clears-history` | 2/3 | `тест покраснел, как обязан` |
|
||||
| `config-adoption-rev-from-foreign-response` | 3/3 | `тест покраснел, как обязан` |
|
||||
| `config-adoption-rollback-ignores-rev` | 1/3 | `тест покраснел, как обязан` |
|
||||
| `post-write-skips-asset-gate` | 3/3 | `тест покраснел, как обязан` |
|
||||
|
||||
Плюс чистые (немутированные) прогоны в тех же логах подтверждают отсутствие
|
||||
регрессии по соседним защитам на этом материале: `node --test
|
||||
test/config-adoption.test.mjs`, `node demo/smoke_room_resize.mjs` (сам фикс
|
||||
реактивности `0905f67e` — эту же регрессию поймал предыдущий красный Validate
|
||||
на `c360bcc9`), `node demo/smoke_summary_panel.mjs`, `node demo/
|
||||
smoke_marker_write_rollback.mjs`, `node demo/smoke_device_position_history.mjs`,
|
||||
`node demo/smoke_summary_panel_polish.mjs` — все чистые.
|
||||
|
||||
**Браузерные смоки, не покрытые CI на этом SHA.** Тяжёлые job'ы (`smoke`,
|
||||
`golden`, `performance_smoke`) в этом прогоне `skipped` — обычный push без
|
||||
`Release:`, не кандидат (см. `PROCESS.md` §8/AGENTS.md, ожидаемо). Отдельно
|
||||
проверил: ни `demo/smoke_post_write_adoption.mjs` (новый смок, единственное
|
||||
прямое свидетельство AC4), ни `demo/smoke_danger_confirmation.mjs` не
|
||||
зарегистрированы guard'ом ни одного мутанта в `scripts/mutation-gate.mjs` —
|
||||
значит job «Мутанты по диффу» их тоже не запускал ни разу на этом материале
|
||||
(грепом по логам всех трёх шардов подтверждено отсутствие). Автор сам об этом
|
||||
написал в хендоффе («в моей среде нет Chromium»). Прогнал сам:
|
||||
|
||||
```
|
||||
npm run build && npm run bundle:sync
|
||||
node demo/smoke_post_write_adoption.mjs → OK, все 20 проверок true
|
||||
node demo/smoke_danger_confirmation.mjs → OK, все 18 проверок true
|
||||
```
|
||||
|
||||
`node scripts/smoke-select.mjs --base origin/dev --head HEAD` даёт 47 прямых
|
||||
совпадений (порог «широкого» — больше 47, эта задача чуть ниже порога, но
|
||||
широка по природе диффа: тронуты повсеместно читаемые `_cfgRev`/`_layoutRev`/
|
||||
`_model`/`_serverCanWrite`). Полный прогон всех 47 непропорционален объёму
|
||||
находок и является предрелизным гейтом (§8); выбрал те, что прямо касаются
|
||||
изменённых путей (adoption/rollback/reactivity) и либо подтвердил зелёными
|
||||
через логи CI-мутантов, либо прогнал сам. Остальные 45 не прогонял — по
|
||||
диффу они читают эти поля так же, как раньше (делегаты не меняют семантику
|
||||
чтения), а не изменённое поведение.
|
||||
|
||||
**Инварианты модели** — диффом задета идентичность, не геометрия рёбер/
|
||||
толщины/ссылок; `npm run invariants` не запускал (не требуется правилом:
|
||||
геометрия и её ссылки не менялись, только владелец ревизии/тела).
|
||||
|
||||
**pytest/golden/perf** — не задеты диффом (только `src/**`, backend не
|
||||
тронут; визуал не меняется, ТЗ §7 и §14 это явно фиксируют); не гонял.
|
||||
|
||||
## AC → доказательство (проверено чтением и/или прогоном)
|
||||
|
||||
| AC | Чем доказан | Проверено | Чем краснеет |
|
||||
|---|---|---|---|
|
||||
| AC1 | `test/config-adoption-ownership.test.mjs` | чтением: пересчитал оба регекса вручную по `src/**` — `_serverCfg =` 5(editor)+3(card), `_layout =` 5(editor)+9(card), совпадает с allowlist дословно; `IDENTITY_WRITE` не находит присваиваний вне модуля | вернуть присваивание в любом модуле — красный (мутация не нужна, тест — прямой grep) |
|
||||
| AC2 | `test/config-adoption-ownership.test.mjs` + чтение | подтверждено: `_adoptStructuralResponses` как host-метод не существует нигде вне `config-adoption.ts`; `SummaryPanelHost`/`HouseplanEditorHostPort` сужены точно по §6.4 | оставить старый метод в любом файле — красный |
|
||||
| AC3 | `test/config-adoption.test.mjs` (17 кейсов) + существующие смоки/юниты | таблица переходов (эхо/смена/только layout/virtual lights/gated-последовательность/reactive-контракт) прочитана целиком, поведенчески эквивалентна старому `_adoptStructuralResponses`; `post-write` подтверждён без пост-шагов тестом на строке 215 | мутант `config-adoption-echo-clears-history` пойман на материале (см. таблицу выше) |
|
||||
| AC4 | `demo/smoke_post_write_adoption.mjs` + I2-юнит | **прогнал сам** — 20/20 true; частично не покрыт: ветка отказа гейта (`asset-wait`) ни одним из четырёх вызывающих не тестируется (см. находку M-1 ниже) | мутант `post-write-skips-asset-gate` пойман на материале |
|
||||
| AC5 | `test/config-adoption.test.mjs` round-trip | прочитано, ключи `LS_CFG` побайтово совпадают со старым форматом | мутант не заведён (чистый юнит, признание правила §2.7 достаточно) |
|
||||
| AC6 | `test/config-adoption.test.mjs` (перенесённые кейсы #439/#442) | прочитано, идентично прежней логике `rollbackOptimistic` | мутант `config-adoption-rollback-ignores-rev` пойман на материале |
|
||||
| AC7 | `test/core-file-budget.test.mjs`, `bundle:budget`, `test/bundle-assets.test.mjs` | `wc -l src/houseplan-card.ts` = 13605 < потолка 13650; `INITIAL_VIEW_GZIP_CEILING` 300 300 с датированной записью в `scripts/bundle-budget.mjs`; общий бюджет 301 066 не тронут | поднять любой из потолков — красный по правилу теста (не прогонял мутацию, чтение констант и тест-файла достаточно) |
|
||||
| AC8 | ревью кода | `docs/ARCHITECTURE.md` содержит абзац о границе (владелец, три способа смены ревизии, единая последовательность, оба профиля) — соответствует факту кода | — |
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium-1 (в скоупе задачи). Post-write гейт: отказ (`asset-wait`) не останавливает хвост вызывающего ни в одном из четырёх путей — единственная защита AC4 не проверена для своей же отказной ветки и частично не работает
|
||||
|
||||
**Где:** `src/houseplan-editor-runtime.ts:8693` (`_deleteSpace`, editor),
|
||||
`src/houseplan-editor-runtime.ts:9575` (`_undoPlanOptimization`),
|
||||
`src/houseplan-editor-runtime.ts:9734` (`_applyBackupImport`),
|
||||
`src/houseplan-onboarding-runtime.ts:477` (`_deleteSpace`, onboarding).
|
||||
|
||||
**Что происходит.** У трёх reload-вызывающих (`_loadFromServer`,
|
||||
`_reloadConfigOnly`, summary recovery) при `adopted.status !== 'adopted'`
|
||||
метод полностью прерывается (`return`/`throw`) — хвост не выполняется вовсе,
|
||||
структура и на глаз, и в идентичности остаётся прежней, а
|
||||
`_scheduleLoadRetry(true)` (вызванный внутри `adoptAuthoritativeGated`) сам
|
||||
подтянет данные позже. Ни один из четырёх **post-write** вызывающих так не
|
||||
делает:
|
||||
|
||||
- `_undoPlanOptimization` (`:9575`) вообще не читает `adopted.status` —
|
||||
`_geometryHistory.clear()`, `_devicePositionHistory.clear()`,
|
||||
`_canOptimizeUndo = false`, `_undoKind = null`, `_cfgEpoch++`,
|
||||
`_maybeRebuildDevices()`, `_cacheSnapshot()` и тост
|
||||
`gs.optimize_undone`/`backup.import_undone` выполняются безусловно.
|
||||
- `_deleteSpace` (оба входа, `:8693` и onboarding `:477`) и
|
||||
`_applyBackupImport` (`:9734`) читают `adopted.status` **только** для
|
||||
одного шага — выбора видимого пространства (`_commitSpace`/
|
||||
`_adoptInitialSpace`). Закрытие диалога, `_regSignature = ''`,
|
||||
`_maybeRebuildDevices()`, `_cacheSnapshot()` (у Import) и финальный тост
|
||||
выполняются безусловно, тем же порядком, что и при успешном adoption.
|
||||
|
||||
**Почему это дефект, а не стиль.** До #500 у post-write путей вообще не было
|
||||
асинхронного гейта — `_adoptStructuralResponses` было синхронным и всегда
|
||||
адоптировало то, что пришло; ветки «не адоптировано» просто не существовало.
|
||||
Она появляется **именно этой задачей** (тем самым единственным намеренным
|
||||
изменением поведения из §6.3 ТЗ) — и именно поэтому её последствия для
|
||||
вызывающих — предмет этого ревью, а не наследие. Сценарий реалистичен и
|
||||
прямо описан в самом ТЗ (§6.3): «Undo/Import могут вернуть другой фон» — то
|
||||
есть конкурентная смена `plan_url` между записью и повторным чтением, из-за
|
||||
которой `prepareImage` не готов вовремя, — ровно тот случай, который смок
|
||||
`demo/smoke_post_write_adoption.mjs` воспроизводит для успешного пути
|
||||
(`assetReady` всегда `true` в фикстуре). При отказе `prepareImage`
|
||||
(например, временная сетевая ошибка на media-source) сервер уже необратимо
|
||||
выполнил операцию (space удалено / undo применён / импорт применён), а
|
||||
карточка: (а) для Undo — показывает тост «отменено» и гасит аффорданс Undo,
|
||||
хотя видимая модель ещё старая (пост-оптимизационная, не отменённая) —
|
||||
несовпадение тоста с фактическим состоянием экрана; (б) для всех четырёх —
|
||||
пересобирает устройства/кэширует снимок по **старой** (неадоптированной)
|
||||
идентичности, хотя гейт был явно введён, чтобы не показывать переходное/
|
||||
рассинхронизированное состояние. Ретрай внутри `adoptAuthoritativeGated`
|
||||
рано или поздно подтянет верные данные, поэтому дефект самоисцеляющийся и не
|
||||
портит серверные данные — отсюда Medium, а не High.
|
||||
|
||||
**Чем красит/красится.** Ни `test/config-adoption.test.mjs`, ни
|
||||
`demo/smoke_post_write_adoption.mjs` не подают `assetReady: false` ни в один
|
||||
из четырёх post-write сценариев — ветка `status !== 'adopted'` для
|
||||
post-write профиля ни разу не проходится ни юнитом, ни смоком. Убедиться
|
||||
легко: временная правка `card._signer.prepareImage = async () => false;`
|
||||
перед любым из четырёх вызовов в `demo/smoke_post_write_adoption.mjs`
|
||||
сегодня не роняет ни одну из существующих проверок смока (они не смотрят на
|
||||
это состояние) — то есть отсутствие проверки подтверждено, а не
|
||||
предположено.
|
||||
|
||||
**Требуется:** привести хвосты `_deleteSpace` (оба входа),
|
||||
`_undoPlanOptimization`, `_applyBackupImport` к тому же правилу, что уже
|
||||
работает в reload-профиле — полностью пропускать хвост (закрытие диалога,
|
||||
очистки, тост, `_cacheSnapshot`, выбор пространства) при
|
||||
`adopted.status !== 'adopted'`, либо явно обосновать в ТЗ/ревью, почему
|
||||
post-write хвост обязан выполняться даже без структурного adoption — и
|
||||
покрыть выбранное поведение тестом/смоком, который умеет упасть на снятой
|
||||
защите. Находка в скоупе задачи (это ровно те четыре пути, которые ТЗ §6.3
|
||||
называет объектом гейта) — чинится в этом же issue, второй ревью-цикл не по
|
||||
лимиту §4, а по итогу вердикта.
|
||||
|
||||
### Low-1 (снимается записью, не требует правки). Риск-таблица ТЗ §12 содержит неверный факт о `static properties`, но код всё равно корректен
|
||||
|
||||
ТЗ (§12) утверждает: «`_serverCfg`, `_layout`, `_cfgRev` и fingerprint не
|
||||
входят в `static properties` хоста (проверено на `a824acc1`)». Проверка
|
||||
чтением `git show a824acc1:src/houseplan-card.ts` показывает, что `_layout`
|
||||
и `_serverCfg` **были** объявлены `{ state: true }` уже на этом коммите (до
|
||||
#500) — то есть до задачи они были обычными реактивными Lit-полями без
|
||||
собственных аксессоров, и замена их на кастомные getter/setter действительно
|
||||
меняла контракт реактивности (Lit пропускает генерацию своего аксессора,
|
||||
когда `prototype.hasOwnProperty(name)` истинно). Утверждение спеки было
|
||||
неверным в момент зелёного ревью ТЗ r1–r3 — но реальный код это не сломало
|
||||
благодаря отдельному фикс-коммиту `0905f67e` (`onBodyReplaced` →
|
||||
`requestUpdate`), написанному именно потому, что red Validate на `c360bcc9`
|
||||
поймал регрессию `smoke_room_resize` от этого же факта. Итог: код корректен и
|
||||
подтверждён смоком/юнитом, найденная неточность — в тексте уже принятого ТЗ,
|
||||
которое не переоткрывается этим ревью. Фиксирую как Low без блокировки.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Модуль `src/config-adoption.ts`: ссылочная идентичность (I4), приём
|
||||
ревизии только с телом (I2), эхо не трогает историю/эпоху (I3) — все три
|
||||
инварианта подтверждены и чтением, и тестами/мутантами.
|
||||
- Реактивный контракт `onBodyReplaced` → `requestUpdate(field, previous)`
|
||||
корректно воспроизводит поведение снятого Lit-аксессора; `willUpdate`
|
||||
(`houseplan-card.ts:4111`) по-прежнему держит инвариант эпохи геометрии.
|
||||
- Сужение `SummaryPanelHost`/`HouseplanEditorHostPort` до одного метода —
|
||||
соответствует §6.4 дословно (проверено построчно).
|
||||
- `plan-optimize-write.ts`, `space-copy-runtime.ts`, `vacuum-calibration-write.ts`,
|
||||
`editors/vacuum-maps-section.ts` — переведены на `acceptPairWrite`/
|
||||
`beginOptimistic`/`stageLocalConfig`/`host._rollbackOptimistic`, прямых
|
||||
записей идентичности не осталось (подтверждено AC1-тестом и ручным grep).
|
||||
- `serialized-write-queue.ts` лишился `rollbackOptimistic`; единственный
|
||||
вызывающий отката — `houseplan-card.ts` (`_rollbackOptimistic`), как того
|
||||
требует AC2-тест.
|
||||
- Бюджеты AC7: факт (13605 строк, 299 771 Б) внутри новых потолков с
|
||||
датированной записью причины пересчёта.
|
||||
- Трейлеры всех десяти коммитов: `Issue: #500` + `User-Visible: no`,
|
||||
changelog не тронут — согласовано с `User-Visible: no`.
|
||||
- `docs/ARCHITECTURE.md` (AC8) описывает границу фактически точно.
|
||||
|
||||
## Чего не проверял и почему
|
||||
|
||||
- Полный набор из 47 «прямых совпадений» `smoke-select` — прогнал только те,
|
||||
что напрямую относятся к изменённому поведению (adoption/rollback/
|
||||
reactivity), остальные читают делегируемые поля так же, как раньше;
|
||||
полный набор — предрелизный гейт (§8).
|
||||
- `npm run invariants` — диффом не задета геометрия/толщина/ссылки, только
|
||||
владение ревизией и телом.
|
||||
- `python -m pytest tests_backend` — backend не тронут диффом.
|
||||
- `golden:verify`/`performance_smoke` — визуал и перф-профили не в AC и не
|
||||
меняются по диффу (подтверждено ТЗ §7/§14 и составом diff: только `src/**`
|
||||
frontend, никаких изменений в рендер-путях).
|
||||
- Собственный повторный прогон `tsc`/`npm test`/`npm run build` — не
|
||||
дублировал: зелёный Validate подтверждён на точном материале ревью через
|
||||
`gh run view` (headSha сверен).
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- SHA материала: `0905f67ec0d8fb830065e909988f442c884320c2`
|
||||
- Дерево: рабочая копия уже на этом SHA (`git rev-parse HEAD` совпадает)
|
||||
- ТЗ: `docs/specs/500-config-adoption-boundary.md`, зелёное ревью
|
||||
`docs/reviews/SPEC-REVIEW-500-r3.md` на коммите `622470ff`
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/500-config-adoption-boundary`, коммит `0905f67ec0d8` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `5c8ab48044237c58112ba21b6d38a8de22a371c1`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 5c8ab4804423
|
||||
```
|
||||
- Тело issue: `d4a68d8c756302bcc7b8ec4bf3a7f77ffc88848e51f2b054d58736bfda431ec5`
|
||||
- ТЗ `docs/specs/500-config-adoption-boundary.md`, блоб `892240ddf10b28b483d865aa21e7132a1c0b7aaa`
|
||||
```
|
||||
git log --all --find-object=892240ddf10b28b483d865aa21e7132a1c0b7aaa -- docs/specs/500-config-adoption-boundary.md
|
||||
```
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user