From 804b282f5fd4720e19c2c850dbb170ff8f15b24a Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:28:43 +0000 Subject: [PATCH] docs: review document for #186 Issue: #186 User-Visible: no --- docs/reviews/SPEC-REVIEW-186-r2.md | 216 +++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-186-r2.md diff --git a/docs/reviews/SPEC-REVIEW-186-r2.md b/docs/reviews/SPEC-REVIEW-186-r2.md new file mode 100644 index 00000000..f239a6b6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-186-r2.md @@ -0,0 +1,216 @@ +# SPEC-REVIEW-186-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/186 +- **ТЗ под ревью:** [`docs/specs/186-partition-opening-jamb-margin.md`](https://github.com/Matysh/houseplan-card/blob/issue/186-partition-jamb-margin/docs/specs/186-partition-opening-jamb-margin.md) + (коммит `62f73faef737df44ad8bb838aac4beceb74db9a1`), обычный трек — не `small`/`trivial` +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный, лимит циклов ревью ТЗ — 4 (§4 PROCESS.md) +- **Цикл:** r2/4 +- **Предыдущий цикл:** [`SPEC-REVIEW-186-r1.md`](SPEC-REVIEW-186-r1.md) — красный, High-1 (§8/AC4 «full import — всегда strict» противоречил принятому decision #3), Low-1 (нет заголовка «Проблема») + +## Скоуп ревью + +Только правка после r1: коммит `62f73fa` («docs: preserve legacy jambs on full +restore») меняет §3 (пункт 6), §5, §8, UX-таблицу (§9), AC4, AC6 и таблицу +рисков (§13), плюс добавляет заголовок «Проблема» (§2). Формула margin (§4), +scope/не-scope (§5/§6 кроме одной строки), frontend-контракт (§7), AC1/AC2/AC3/ +AC5, план проверок (§12), откат (§14), release-артефакты (§15) и принятые +предположения (§16) не менялись — их точность уже подтверждена в r1 и +переподтверждается здесь только там, где новая правка их касается. + +Продуктовый код по-прежнему не существует: ветка `issue/186-partition-jamb-margin` +содержит три коммита (`7d4d3f0`, `85f54b4`, `62f73fa`), все класса C +(документация), все с трейлерами `Issue: #186` / `User-Visible: no`. Гейты +`typecheck`/`test`/`build`/smoke/golden/pytest не прогонялись — на этапе +ревью ТЗ они не относятся к предмету (PROCESS.md §2.4/§8). + +## Как проверялось + +1. Перечитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (действующая редакция) — + без изменений с r1, инварианты те же. +2. Прочитаны все восемь комментариев issue #186 подряд: аналитика (Q1–Q3 с + defaults), решение владельца по Q1–Q3, хендофф «ТЗ готово к ревью», вердикт + r1 (красный, High-1), дополнительный вопрос владельцу Q4 (после ревью), + решение владельца по Q4, хендофф «Правки ТЗ после ревью r1», и служебный + комментарий о несработавшем автоматическом ревью r1 (метка не переставилась — + к предмету этого ревью не относится, поднято отдельно перестановкой метки + вручную/владельцем). +3. Построчно сверен `git show 62f73fa -- docs/specs/186-partition-opening-jamb-margin.md` + с текстом решения владельца по Q4 и с хендоффом «Правки ТЗ после ревью r1»: + правка ограничена ровно перечисленными разделами, посторонних изменений + (формулы, AC1-3/5, scope-исключений, отката) нет — «попутных правок» не + обнаружено. +4. Формулировка §3 п.6 и §8 сверена дословно с текстом решения владельца по Q4 + («полный backup/restore всегда сохраняет legacy near-end проёмы... при + восстановлении поверх текущего плана, на пустой и на другой инсталляции. + Full import проверяет структурную целостность и попадание проёма внутрь + host, но не применяет новый jamb safety margin») — совпадение по существу + полное, без добавленных условий и без потерянных случаев (текущий / + пустой / чужой инстанс перечислены во всех трёх местах: §3, §8, AC4). +5. Независимо перепроверена архитектурная посылка исправления (что full + import физически не обязан быть strict) чтением текущего кода на этой же + ветке: + - `custom_components/houseplan/import_export.py:1304-1345` (`prepare_apply`) + — для `kind == "full"` вызывается только `CONFIG_SCHEMA(config)` в конце + функции (строка 1345); `validate_partition_opening_hosts` в этой ветке не + вызывается вовсе. + - `grep -n "validate_partition_opening_hosts" custom_components/houseplan/*.py` + — ровно три вызова: `import_export.py:995` внутри `build_space_merge()` + (путь `kind == "space"`, т.е. частичный merge-импорт, не полный), + `websocket_api.py:1260` (`config/set`) и `websocket_api.py:1372` + (`optimize`). Ни один не относится к `kind == "full"`. + - Вывод: `validate_partition_opening_hosts()` (и, следовательно, будущий + jamb-контракт «рядом с ним либо в его расширении», §8) **уже сегодня** + структурно не покрывает full import — это не новое послабление ради + #186, а естественное продолжение существующей архитектуры. Формулировка + §8 не вводит специальный обход, а корректно описывает то, что и так + происходит. + - Отдельно проверено (не для находки, а чтобы не спутать с общим + паттерном): `docs/CONFIG-COMPATIBILITY.md:59` документирует другой + прецедент (#157, `invalid_passage_fields`), где `validate_opening_passages` + **действительно** вызывается для каждого импорта, включая `full` + (`import_export.py:1159`, `create_preview()`, `validate_all=True`, до + ветвления по `kind`). Это не противоречие: у #157 и #132/#186 разные + валидаторы с разной историей вызова на full-import пути, и decision + Q4 явно называет цену этого выбора («вручную изменённый полный backup + тоже может содержать near-end geometry») — то есть асимметрия с #157 + осознанная, а не незамеченная. +6. Сверено, что merge-импорт (`kind == "space"`, `build_space_merge` вызывает + `validate_partition_opening_hosts(merged_config, current_config)` на + `import_export.py:995`) остаётся вне пересмотренного decision Q4 (тот + называет только «полный backup/restore»). ТЗ (§5/§8/AC4/AC6) не утверждает + ничего противоположного про merge-путь и не молчит о нём как о решённом + факте — merge и раньше (#132) переприсваивает id и потому уже трактует + переносимые записи как новые для семантического валидатора; #186 просто + наследует этот действующий паттерн, не решая для него ничего нового. + Дополнительных вопросов владельцу это не требует. +7. Перечитан заново весь текст ТЗ (не только диф) на предмет новых утверждений + о поведении, поданных как факт без пометки assumption или ссылки на + решение владельца — целенаправленно искал второй High-1. Не найдено: + каждое утверждение §3/§5/§8/§9/AC4/AC6 после правки либо дословно повторяет + decision Q1–Q4, либо является технической деталью уже покрытой §16 + (имена helper/reason, previous-параметр). +8. Перепроверены обязательные разделы §7.1 (таблица ниже) и добавленный + заголовок «Проблема» — сравнение старой и новой версии (`git show 62f73fa`) + подтверждает: над «До/после» появился отдельный вводный абзац с + заголовком `## 2. Проблема`, ровно то, что требовал Low-1. +9. Проверена запись `docs/specs/README.md:102` — ссылка issue ↔ ТЗ + двусторонняя и не менялась в этом цикле, ещё присутствует. +10. Проверены трейлеры всех трёх коммитов ветки (`git log --format='%H%n%s%n%b'`) + — `Issue: #186` и `User-Visible: no` на каждом; `User-Visible: no` + корректен, т.к. ни один коммит не меняет продукт. + +Гейты (`typecheck`/`test`/`build`, browser smoke, golden, backend pytest) не +прогонялись — продуктового кода нет, это ожидаемо на этапе ревью ТЗ. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | §1, без изменений в r2 | +| Проблема | ✅ | §2, теперь отдельный заголовок с вводным абзацем — Low-1 закрыт | +| Что человек увидит до/после | ✅ (см. Low-2) | «До/После» под §2; формулировка не изменилась в r2 и содержит термины реализации, см. находку ниже | +| Скоуп / не-скоуп | ✅ | §5/§6; один пункт §5 переформулирован под decision Q4, без потери содержания | +| Контракт поведения | ✅ | §7/§8; §8 переписан под decision Q4, внутренне непротиворечив | +| UX | ✅ | §9, добавлена строка про full restore | +| Модель данных и миграция | ✅ | §10, без изменений | +| i18n | ✅ | §10, без изменений | +| AC1…ACn с доказательством | ✅ | AC4/AC6 переписаны под decision Q4, доказательство расширено на current/empty/foreign install | +| План автотестов | ✅ | §12, без изменений | +| Риски | ✅ | §13, добавлена строка «Full restore ошибочно становится strict» | +| Откат | ✅ | §14, без изменений | +| Release-артефакты | ✅ | §15, без изменений | + +Блок «Принятые технические предположения» (§16) не менялся в r2; при повторном +чтении ни один из четырёх пунктов по-прежнему не маскирует продуктовый вопрос. + +## Находки + +### Low-2 — «что человек увидит» использует термины реализации + +**Файл:** `docs/specs/186-partition-opening-jamb-margin.md`, §2, подраздел +«До/После» (не менялся в r2, существовал и в r1). + +PROCESS.md §7.1 требует для этого элемента «одной фразой, без терминов +реализации». Текущий текст: «До: **frontend** считает валидным любой проём... +**Backend** повторяет эту границу с одним лишь **floating-point epsilon**...», +«После: у обоих торцов host резервируется остаток... математическими +**endpoints** host». Используются архитектурные/кодовые термины (frontend, +backend, epsilon, endpoints), а не то, что видит администратор в редакторе +плана (например: «проём можно было поставить впритык к торцу стены, без места +под наличник; теперь редактор всегда оставляет отступ, равный половине +толщины стены»). + +Ни один AC не опирается на эту формулировку и не становится двусмысленным — +дефект чисто редакционный, той же природы, что и снятый в r1 Low-1. + +**Решение ревьюера:** Low, не блокирует. Снимаю без возврата ТЗ на правку; +рекомендую при следующей правке файла (не отдельным циклом) заменить +«До/После» на формулировку в терминах того, что видит администратор в Plan +editor, не изобретая новых решений — переформулировка, не новый контракт. + +## Что проверено и корректно + +- **High-1 из r1 закрыт корректно и полностью.** Коммит `62f73fa` меняет ровно + те разделы, что были нужны (§3 п.6, §5, §8, §9, AC4, AC6, §13), дословно + соответствует принятому владельцем decision Q4, и не оставляет старой + формулировки «full import без trusted previous — strict» ни в одном месте + документа (проверено построчным перечитыванием всего файла, не только + дифа). +- **Архитектурная посылка исправления подтверждена независимым чтением кода**, + а не принята на слово: `validate_partition_opening_hosts()` сегодня + вызывается только в `build_space_merge` (частичный merge), `config/set` и + `optimize` — ни разу для `kind == "full"` в `prepare_apply()`. Значит + decision Q4 не вводит специальный обход общей архитектуры, а продолжает уже + существующее устройство full-import пути. Асимметрия с прецедентом #157 + (`invalid_passage_fields`, который действительно строг на full import) — + реальная и явно принятая владельцем цена, а не незамеченное расхождение. +- **Merge-импорт (частичный, `kind == "space"`) остаётся вне decision Q4**, + и ТЗ не утверждает о нём ничего, что противоречило бы этому — унаследованный + из #132 паттерн переприсвоения id продолжает работать без нового решения. +- **Второй High не найден.** Целенаправленный повторный проход по всему + документу (не только по дифу r1→r2) не выявил новых утверждений о + поведении, поданных как факт без пометки assumption или ссылки на решение + владельца. +- **Формула margin (§4), AC1/AC2/AC3/AC5, откат (§14), touch-декларация (§10) + и release-артефакты (§15)** не менялись в этом цикле; их точность была + независимо перепроверена в r1 (арифметика `GRID_N=240`, реальные call sites, + существующий паттерн `docs/CONFIG-COMPATIBILITY.md`) и остаётся в силе. +- **Трейлеры и трассируемость**: все три коммита ветки несут + `Issue: #186`/`User-Visible: no`, запись issue ↔ ТЗ в `docs/specs/README.md` + на месте, `docs/specs/186-partition-opening-jamb-margin.md` ссылается на + issue и на #132. +- **Трек подтверждён обычным** (не `small`/`trivial`) — критерии §5 PROCESS.md + по-прежнему не выполняются (новый видимый граничный контракт, + compatibility-решение), лимит цикла — 4, использован r2/4. + +## Чего не проверял + +- Реализацию — её по-прежнему нет: три коммита ветки все класса C + (документация), продуктовый код (`src/**`, `custom_components/houseplan/**/*.py`) + не менялся этой веткой — проверено чтением `git show --stat` каждого + коммита. +- Гейты `typecheck`/`test`/`build`/browser smoke/golden/`pytest tests_backend` — + не относятся к этапу ревью ТЗ. +- Полный повторный пересчёт формулы margin и всех восьми call sites + `resolvePartitionOpening()` — не требовался: эта часть документа не менялась + между r1 и r2, независимая проверка из r1 остаётся в силе и не переделывалась. +- Оставшиеся детали merge-импорта (`build_space_merge`, duplicate_policy, + `same_source`) за пределами вопроса «вызывается ли здесь + `validate_partition_opening_hosts`» — не относится к предмету находки High-1 + и её закрытия. +- Численные оценки аналитики (5/10 · 6/10 · 5/10 · P2) — поле владельца, + решено до написания ТЗ, вне предмета ревью. + +## Вердикт + +Зелёный. High: 0 (High-1 из r1 закрыт decision Q4 и корректно отражён во всех +затронутых разделах ТЗ; архитектурная посылка независимо перепроверена +чтением текущего кода). Medium: 0. Low: 1 новая (Low-2 — «До/После» использует +термины реализации вместо описания того, что видит администратор; снимается +ревьюером без возврата, рекомендация — поправить при следующей правке файла). +Low-1 из r1 (отсутствие заголовка «Проблема») закрыт правкой. ТЗ готово к +переходу в `S5-ready`. + +**Вердикт: зелёный · цикл r2/4 · High: 0 · Medium: 0 → в задаче · Документ: +docs/reviews/SPEC-REVIEW-186-r2.md**