From a7e531787e6c71e09b6b89339a9da32127da64a9 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 30 Aug 2026 09:59:51 +0000 Subject: [PATCH] docs: review document for #33 Issue: #33 User-Visible: no --- docs/reviews/CODE-REVIEW-33-r1.md | 211 ++++++++++++++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-33-r1.md diff --git a/docs/reviews/CODE-REVIEW-33-r1.md b/docs/reviews/CODE-REVIEW-33-r1.md new file mode 100644 index 00000000..f79014ed --- /dev/null +++ b/docs/reviews/CODE-REVIEW-33-r1.md @@ -0,0 +1,211 @@ +# CODE-REVIEW-33-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/33 +- ТЗ: docs/specs/033-config-schema-lifecycle.md (ревизия 3, зелёное ревью SPEC-REVIEW-33-r2) +- Ветка: `issue/33-config-schema-lifecycle` +- SHA материала ревью: `572cf928969fd8849299170ba3d42ff61d7ef423` (`git rev-parse HEAD`) +- Заход: r1 (первый код-ревью этого issue; ревью ТЗ не расходует бюджет код-ревью, §10.4/§2.10) +- Диапазон: `origin/dev..HEAD` — 4 коммита (`24681ee2` feat, `867f4762` feat, `f4318721` fix, `572cf928` build) + +## Скоуп + +Ревизия 3 ТЗ, три блока: +1. Генерируемый манифест схемы (`scripts/dump-config-schema.py` → `scripts/config-schema.json`, 265 листовых путей), pytest на свежесть. +2. Parity-тест 8 enum-пар backend↔frontend через `scripts/schema-compat-allowlist.mjs`; тест полноты `config-field-registry.mjs`; новые `as const`-декларации (`OPENING_TYPES`, `VACUUM_TRAIL_MODES`, `ZERO_WALL_STYLES`, `BG_MODES`); паспорта registry (`enforcedBy`, `schema: 'allow-extra'|'lovelace-card'`, 3 новых current-паспорта). +3. Lifecycle-фикстуры (oldest/current/future), pytest на lossless/round-trip; `config-audit.mjs` — контракт exit-code 0/3/2. + +Рантайм-поведение не меняется (заявлено ТЗ, подтверждено чтением: новые `as const`-массивы нигде не потребляются продуктовым кодом, кроме вывода типа). + +## Как проверялось + +Дешёвые гейты прогнаны лично на SHA `572cf928`: + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | чисто | +| unit | `npm test` | 1608 pass / 0 fail / 1 skipped (совпадает с хендоффом) | +| build + сверка бандла | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | идентичны | +| три копии бандла | `npm run bundle:sync` | git status чист после — дерево уже синхронизировано | +| docs fingerprint | `node scripts/check-docs.mjs` | "Documentation checks passed (7 files, 10 external links)" — обязателен, т.к. diff трогает `src/**` (sun.ts, types.ts) | +| бюджет | `npm run bundle:budget` | `initial View: 279147 B gzip (budget 300000 B, headroom 20853 B)` — совпадает с заявленным | +| backend, новый файл | `python3 -m pytest tests_backend/test_config_schema_manifest.py -q` | 3 passed (после `pip install pytest pytest-asyncio voluptuous` — в песочнице их не было) | +| backend, полный `tests_backend` | `python3 -m pytest tests_backend -q` | падает на сборе (`ModuleNotFoundError: homeassistant` в `test_coordinate_canonicalization.py`) — **не регрессия этой задачи**: файл не тронут диффом и не подпадает под `test_ha_*` исключение из `conftest.py`; ограничение окружения без HA, а не дефект #33 | +| дамп свежий | `python3 scripts/dump-config-schema.py --check` | `manifest fresh: 265 paths` | +| parity + audit unit | `node --test test/config-audit.test.mjs test/config-schema-parity.test.mjs` | 8/8 pass | +| model-invariants | `npm run invariants -- --config test/fixtures/config-lifecycle/{oldest-supported,current,future-fields}.json` (все три; diff трогает `types.ts`/geometry-смежные lifecycle-фикстуры) | все три: "Инварианты выполнены: ссылки разрешимы, записи толщины находятся"; на current/future — 4 информационных наблюдения (exact-endpoints вместо ключа), не нарушения | +| мутанты (по делу, не полный набор — дорогой гейт, PROCESS §8/§4) | `node scripts/mutation-gate.mjs --id=schema-manifest-enum-drift` и `--id=registry-selector-dead-decision` | оба «поймано 1 из 1» | + +**Не прогонял и почему:** +- `python -m pytest tests_backend -q` (полный набор с HA) — недоступен в песочнице (нет `homeassistant`, нет `.venv-backend`); согласно AGENTS.md это ожидаемое ограничение локальной среды, а не гейт этой задачи. Новый файл `test_config_schema_manifest.py` — единственный, добавленный диффом, — прогнан отдельно и зелёный. +- Полный `npm run mutation-gate` (весь реестр мутантов) — дорогой предрелизный гейт (документировано в самом скрипте: «место — перед стабильным релизом»); прогнаны только два новых мутанта, относящихся к диффу. +- Браузерные смоки — `node scripts/smoke-select.mjs --base origin/dev --head HEAD` вернул **НЕОПРЕДЕЛЁННОСТЬ**: ни один из 205 смоков не связан с изменёнными символами (`BG_MODES`, `OPENING_TYPES`, `VACUUM_TRAIL_MODES`, `ZERO_WALL_STYLES` и их типы). Проверено чтением: `grep` по `src/*.ts` показывает, что эти константы нигде не потребляются продуктовым кодом кроме вывода типа — они существуют исключительно ради `test/config-schema-parity.test.mjs`. Раз рантайм-поведение этих символов не меняется, смок не может ничего увидеть; решение — не гонять сверх того, что автор уже прогнал (`smoke_space_card`, `smoke_decor` — сами по себе не обязательны, но безвредны). +- `npm run golden:verify` — diff не меняет рендер/геометрию/стили, только типы и build/test-инфраструктуру; не запускал. + +## Находки + +Все три — Medium, все в скоупе задачи (правятся в этом же issue, без Hig +h и без нового issue). + +### M1 — AC7-тест не может поймать то, для чего он написан + +`test/config-schema-parity.test.mjs:96-98`: +```js +test('#33 AC7: no production source imports the schema manifest', () => { + const source = readFileSync(new URL('../src/houseplan-card.ts', ...), 'utf8') + + readFileSync(new URL('../src/houseplan-editor-runtime.ts', ...), 'utf8'); + assert.ok(!source.includes('config-schema-manifest'), ...); +}); +``` +Манифест был переименован в коммите `f4318721` (`scripts/config-schema-manifest.json` → +`scripts/config-schema.json`, ровно из-за конфликта с глобом HACS `*manifest.json`), но +эта проверка по-прежнему ищет старую строку `'config-schema-manifest'`, которой больше +нет нигде в репозитории ни как файла, ни как валидного пути импорта. + +**Воспроизведение (проверено исполнением, не чтением):** дописал в `src/houseplan-card.ts` +строку `fetch('./scripts/config-schema.json')` — реальный импорт манифеста под его текущим +именем — и перезапустил тест: +``` +ok 3 - #33 AC7: no production source imports the schema manifest +``` +Тест остался зелёным. AC7 («манифест не импортируется бандлом — контракт-проверка +отсутствия импорта») сегодня не проверяет вообще ничего: искомая строка не может +встретиться ни в каком корректном имени импорта. + +Отдельно: проверка читает только 2 файла из ~60 в `src/**`, хотя и ТЗ («AC7-контракт +(нет импорта манифеста из src/\*\*)»), и план автотестов говорят про весь `src/**`. Реальную +защиту от роста бюджета сейчас даёт не этот тест, а эмпирический `npm run bundle:budget` +(прогнан лично, см. таблицу выше, budget соблюдён) — но именно регрессионный тест, +который должен ловить будущий случайный импорт, сегодня этого не делает. + +**Правка:** искать актуальное имя (`config-schema.json` либо путь +`scripts/config-schema`) и читать весь `src/**` (например, `readdirSync` рекурсивно или +готовый список файлов сборки), а не два жёстко перечисленных файла. + +### M2 — Дубликат записи в обоих CHANGELOG и в ARCHITECTURE.md + +`docs/CHANGELOG.md:5-17`, `docs/CHANGELOG.ru.md:11-24`, `docs/ARCHITECTURE.md:1503-1550` — один и +тот же абзац/раздел присутствует **дважды подряд**, слово в слово. + +**Причина, видна в истории:** `git diff 24681ee2 867f4762 --stat` показывает, что второй +`feat`-коммит (`867f4762`) добавил ровно те же 24/7/7 строк в +`ARCHITECTURE.md`/`CHANGELOG.md`/`CHANGELOG.ru.md`, которые первый (`24681ee2`) уже добавил — +похоже на случайное повторное применение шага «допиши changelog» при двух подряд идущих +коммитах с идентичным сообщением. Результат в `dev`-кандидате: пользователь, открывший +`CHANGELOG.md`, увидит одну и ту же строку изменений дважды подряд; `ARCHITECTURE.md` несёт +два одинаковых заголовка `## Schema as the source of truth (#33, 2026-08-30)`. + +**Правка:** убрать вторую копию блока в каждом из трёх файлов (внутри этой же задачи — +Medium в скоупе, отдельный issue не заводится, #202). + +### M3 — `config-audit.mjs` расширил exit-code 3 за пределы согласованного контракта, и для `decision-required` это семантически неверно + +ТЗ ревизии 3 (согласовано в SPEC-REVIEW-33-r1/r2, зафиксировано в теле issue и в файле +ТЗ, Блок 3): «`3` — migration available (найдены поля со статусом +**migrate-\*/deprecated-read**)». Реализация (`scripts/config-audit.mjs:83-86`) добавляет +ещё два статуса в `MIGRATION_STATUSES`: +```js +const MIGRATION_STATUSES = new Set([ + 'migrate-on-write', 'migrate-on-settings-save', 'deprecated-read', + 'drop-on-validation', 'decision-required', +]); +``` +`drop-on-validation` (aspect/segments) можно защитить как «тоже legacy-остаток» — это в духе +контракта, хоть и не названо буквально. Но `decision-required` — это статус +`settings.group_lights` и `settings.exclude_integrations` +(`scripts/config-field-registry.mjs`): по собственному описанию регистри это ДЕЙСТВУЮЩИЕ, +поддерживаемые поля, чья судьба не решена архитектурно (issue #44), а не легаси-хвосты и +не что-то, что нужно мигрировать. Их `migration`-текст в registry прямо говорит: «decide +supported UI versus fixed exclusion rules before removal» — то есть миграции для +пользователя СЕГОДНЯ не существует вообще. + +**Сценарий отказа:** владелец экспортирует у себя план, где реально используется +`settings.group_lights` (обычная включённая фича, не легаси), и прогоняет +`npm run audit:config -- house.json` для диагностики. Инструмент возвращает exit-код `3` +«migration available», хотя мигрировать почему-то нечего — поле продолжит жить как есть, +пока #44 не примет решение. Это вводит в заблуждение ровно того человека +(владельца/разработчика), для которого инструмент существует. + +AC6 не ловит это: юнит-тест (`test/config-audit.test.mjs`) прогоняет только три фикстуры +Блока 3, ни одна из которых не содержит `group_lights`/`exclude_integrations`, поэтому +расширение осталось незамеченным собственным тестом задачи. + +**Правка:** либо сузить `MIGRATION_STATUSES` до буквально согласованного набора +(`migrate-on-write`, `migrate-on-settings-save`, `deprecated-read`, и по желанию +`drop-on-validation` с явным примечанием почему), исключив `decision-required`; либо, если +разработчик хочет сохранить более широкий охват, обновить формулировку AC6/контракта в ТЗ +и явно объяснить, почему «decision-required» тоже сигнализирует «migration available» — +на сегодняшний день это фактически неверно для двух из четырёх статусов набора. + +## Что проверено и корректно + +- **AC1** (свежесть/детерминизм/полнота манифеста) — доказано исполнением: + `python3 scripts/dump-config-schema.py --check` → `manifest fresh: 265 paths`; + `tests_backend/test_config_schema_manifest.py::test_issue_33_manifest_is_fresh_and_deterministic` + зелёный (byte-identical + `len(fields) > 200`). Дамп корректно fail-closed на + незнакомые валидаторы (90/265 путей `opaque` — ожидаемо и не судится parity-тестом, + риск явно назван в ТЗ). +- **AC2/AC3** (parity + анти-гниение allow-list) — зелёный тест, и я лично сверил все + 8 пар вручную по `custom_components/houseplan/validation.py` и итоговому + `scripts/config-schema.json` (fill_mode ×2, display, tap_action, opening.type, + trail_mode, zero_wall_style, bg_mode) — значения манифеста совпадают со схемой построчно. + **Тест умеет падать**: мутант `schema-manifest-enum-drift` (фантомное значение в + манифесте) поймал 1/1 при личном прогоне. +- **AC4** (полнота registry) — зелёный; вручную проверил трансляцию тегированных + вариантов манифеста (``, ``) в нетегированные пути и совпадение + с построением селектора из registry. **Тест умеет падать**: мутант + `registry-selector-dead-decision` (испорченный selector) поймал 1/1. + Новые записи (`decor_default_style`, `decor kind:'furniture'`, + `openings[].host=partition`) резолвятся в реальные пути манифеста — проверено чтением + и подтверждено прохождением самого AC4-теста (он бы упал на несуществующем пути). +- **AC5** (lossless фикстуры) — `tests_backend/test_config_schema_manifest.py::test_issue_33_lifecycle_fixtures_pass_the_schema_losslessly` + зелёный на всех трёх фикстурах; потери ограничены задокументированными + `spaces[].aspect`/`spaces[].segments` (vol.Remove). Дополнительно прогнал + `npm run invariants` на всех трёх фикстурах — геометрические инварианты (разрешимость + ссылок, ключ записи толщины = ключ решёточного ребра) не нарушены, только + информационные наблюдения на exact-endpoints, не связанные с #33. + `test_issue_33_future_fields_round_trip_exactly` зелёный — будущие поля на всех + уровнях (root/settings/space/marker) переживают валидацию точно. +- **AC6** (exit-коды 0/3/2) — юнит зелёный на всех трёх кодах (см. M3 по существу + контракта: коды технически работают, но множество триггеров шире согласованного). +- **AC7** (нет импорта в бандл, бюджет не растёт) — эмпирически подтверждено + (`npm run bundle:budget` → 279147/300000, не изменилось от базовой линии — прирост + бюджета отсутствует, манифест не тянется рантаймом); регрессионный тест-контракт + сломан, см. M1. +- Регистри: `enforcedBy` у 4 реализованных механизмов подтверждён чтением кода + (houseplan-card.ts материализация show_all, editor-runtime удаление weather_entity, + normalizeDeviceDisplay + `_dropLegacySegments` для ripple, `vol.Remove` для + aspect/segments) — соответствует находкам предыдущей ревизии владельца (актуализация + бэклога 2026-08-30). +- Переименование манифеста (`f4318721`) применено последовательно везде, где он + упоминается (dump-скрипт, allowlist, mutation-gate, оба теста, ТЗ, ARCHITECTURE.md) — + кроме забытой строки в самом AC7-тесте (M1). +- Трейлеры всех 4 коммитов корректны (`Issue: #33` на каждом, `User-Visible: yes` на + обоих `feat`, `no` на `fix`/`build`); оба changelog правлены в том же коммите, что и + `feat` (хотя и с дублированием, см. M2). +- Класс D (`dist/**`, `custom_components/houseplan/frontend/**`) обновлён отдельным + `build`-коммитом (`572cf928`), не смешан с продуктовым кодом — соответствует таблице + классов AGENTS.md. +- Не-скоуп соблюдён: ни рантайм-поведение конфига, ни судьба + `group_lights`/`exclude_integrations` (кроме паспорта) не тронуты; миграций и новых + ключей i18n нет. + +## Чего не проверял + +- Полный `tests_backend` с реальной Home Assistant (нет `homeassistant`/`.venv-backend` в + этой песочнице) — прогнан только новый файл задачи, который не требует HA. +- Полный набор мутантов (`node scripts/mutation-gate.mjs` без `--id`) — предрелизный + гейт по объёму, прогнаны только два мутанта, введённых этой задачей. +- `golden:verify` и браузерные смоки сверх обоснованного выше — diff не меняет визуал, + smoke-select не нашёл доказанной связи. +- Оговорённая вне скоупа судьба `group_lights`/`exclude_integrations` (#44) — не судил, + это прямо не-скоуп ТЗ. + +## Вердикт + +Жёлтый. High: 0, Medium: 3, все в скоупе — правятся в этом же issue без нового +цикла-issue (#202). Ядро задачи (генерируемый манифест, parity+allow-list, полнота +registry, lifecycle-фикстуры, exit-коды) реализовано добротно и доказано +исполняемыми тестами, включая мутационное доказательство «тест умеет падать» для +AC2 и AC4. Три находки — тест, который не может поймать регрессию, которую он +называет своей целью (M1), задвоенная документация из-за дублирующего коммита (M2) +и семантически неточный exit-код для одного из двух «decision-required» полей (M3).