diff --git a/docs/reviews/SPEC-REVIEW-295-r1.md b/docs/reviews/SPEC-REVIEW-295-r1.md new file mode 100644 index 00000000..cd6cd692 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-295-r1.md @@ -0,0 +1,246 @@ +# SPEC-REVIEW-295-r1 + +Issue: https://github.com/Matysh/houseplan-card/issues/295 +ТЗ: `docs/specs/295-preflight-diagnostics.md` +Ветка: `issue/295-preflight-diagnostics`, SHA материала ревью: `c1b5ceb6b42b5ef506c5c625037334fca6ddf877` +Заход: r1 · трек: обычный (нет `small`/`trivial`) · лимит циклов 4 · блокирующих циклов израсходовано до этого ревью: 0/4 + +## Скоуп + +Задача делает диагностируемым отказ geometric preflight перед «Оптимизировать»: +причина (7 значений `OptimizeGeometryFailureReason`) должна дойти до пользователя +и до отчёта об ошибке, а не теряться, как сейчас. Issue — тип bug/диагностируемость, +трек обычный, ТЗ файлом. Проверялся только этап ТЗ: файл спеки целиком, тело issue +и оба комментария (аналитика, «ТЗ готово»), плюс код подсистемы, которую контракт +трогает (`src/plan-geometry-preflight.ts`, `src/houseplan-card.ts` — диалог +Optimize/align, `custom_components/houseplan/websocket_api.py`, +`custom_components/houseplan/__init__.py`), канонические `docs/CANVAS.md` и +`docs/ARCHITECTURE.md` (раздел про барьер #199) и существующие юниты +`test/plan-geometry-preflight.test.mjs`. Код не менялся, гейты не гонялись — это +этап ревью ТЗ, не код-ревью. + +## Как проверялось + +Ревью ТЗ — состязательное чтение контракта против фактического кода, а не +пересказ автора: + +1. `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` — прочитаны целиком (процесс, классы + изменений, §7.1 обязательные разделы, лимит циклов). +2. Issue #295 (тело + оба комментария) через `gh issue view --comments`. +3. `docs/specs/295-preflight-diagnostics.md` — прочитан целиком. +4. Каждое техническое утверждение контракта (§1) сверено с реальным кодом: + - `src/plan-geometry-preflight.ts:60-140, 310-397` — типы, `catch`-блоки, + `checkOptimizeGeometry`/`checkSpacePhysicalGeometry`, комментарий + `:312-315` («Geometry values never escape this call…»); + - `src/houseplan-card.ts:15769-15920` (`_checkOptimizeGeometry`, + `_previewAlignDialog`, `_runAlignToGrid` — обе точки вычисления preflight) + и `:16995-17040` (рендер ветки `failed`); + - все вызовы `_checkSpacePhysicalGeometry` (`grep`: `:7155, 7326, 7385, 7498, + 7523, 8865`) — что они читают из результата; + - `src/i18n/ru.json`/`en.json` — существующие ключи `gs.align_preflight_*`, + `gs.optimize_detail_*`, `test/i18n.test.mjs` (правила паритета); + - `custom_components/houseplan/websocket_api.py:1103-1161` + (`houseplan/config/get`), `custom_components/houseplan/__init__.py:95-129` + (регистрация Lovelace-ресурса), `custom_components/houseplan/const.py`, + `import_export.py:526` — источники версии интеграции, доступные фронту; + - `git show 482afb73` — коммит, вводивший барьер #199, и его правки + `docs/CANVAS.md`/`docs/ARCHITECTURE.md`. +5. Существующие смоки/юниты по теме: `ls demo/smoke_*.mjs` (нашёл + `smoke_optimize_geometry_preflight.mjs`), `test/plan-geometry-preflight.test.mjs` + целиком — включая три assert'а, которые прямо противоречат контракту §1.1 + (см. находку H1). + +Тяжёлые гейты (typecheck/test/build/смоки) не гонялись: на этапе ревью ТЗ кода +нет, гонять их не над чем. + +## Находки + +### H1 (High, блокирует) — контракт реверсит документированный и протестированный барьер #199 без единого слова об этом + +`§1.1` предписывает добавить `detail?: string`, заполняемый из +`String((error as Error)?.message ?? error).slice(0, 200)` в трёх catch-блоках +`checkOptimizeGeometry` (`prepare-exception`, `wall-exception`, +`floor-exception`), а `§1.4` кладёт этот `detail` в диагностический блок, который +(а) копируется в буфер пользователем и (б) уходит в `console.warn` при КАЖДОМ +неуспешном preflight — то есть автоматически, не только по решению пользователя. + +Это прямо противоречит существующей, явно задокументированной архитектурной +границе, введённой тем же барьером #199 и не тронутой с тех пор: + +- `src/plan-geometry-preflight.ts:312-315`, doc-комментарий к + `checkOptimizeGeometry`: *«Geometry values never escape this call; the dialog + retains only bounded statuses, names and the candidate fingerprint.»* +- `docs/CANVAS.md` (введено в 482afb73, issue #199): *«The dialog retains only + bounded statuses plus `contentFingerprint(candidate.config)`… not polygon + output **or exception text**.»* +- `docs/ARCHITECTURE.md` (тот же коммит): *«The dialog retains statuses and a + config fingerprint, not polygon output **or exception text**.»* +- `test/plan-geometry-preflight.test.mjs:123-145` — три прямых assert'а, + написанных именно для проверки этого свойства: + ```js + const wallThrows = checkOptimizeGeometry(wallConfig, { wallPass: () => { throw new Error('secret'); } }); + assert.doesNotMatch(JSON.stringify(wallThrows), /secret/); + ... + assert.doesNotMatch(JSON.stringify(floorThrows), /private floor detail/); + ... + assert.doesNotMatch(JSON.stringify(prepareThrows), /private preparation detail/); + ``` + +Спека не упоминает эту границу вообще — ни в контракте, ни в рисках, ни в +не-скоупе. Технически это означает: реализация по тексту §1.1 гарантированно +делает эти три `assert.doesNotMatch` красными, а автор должен будет либо удалить +их, либо решить это на месте, не имея от ТЗ ни слова о том, что граница +изменяется осознанно. Это ровно тот случай, о котором предупреждает PROCESS.md: +утверждение о поведении, реверсирующее задокументированное решение, подано как +решённый контракт, а не как предположение, которое надо было заметить и оспорить. + +Вопрос не продуктовый (владельцу такой вопрос не задаётся), а технический — +разрешаю его сам: граница #199 «exception text никогда не покидает барьер» была +осознанным решением (см. цитаты выше), и одна фраза в ТЗ не может её тихо +отменить. Автору нужно явно выбрать один из двух путей и вписать выбор в ТЗ: + +1. **Не выносить raw `error.message` вообще.** `detail` заменяется на что-то + ограниченное самим типом (например `error.name`/фиксированный код), причина + (`reason`) и отпечатки уже дают инструменту достаточно, чтобы диагностировать + класс отказа без риска утечки содержимого geometry-исключения. Тогда граница + #199 не трогается, обновлять `docs/CANVAS.md`/`ARCHITECTURE.md` и три + существующих `assert.doesNotMatch` не требуется — AC1/AC2/AC3/AC4 всё равно + выполняются (причина видна, блок копируется, dev-лог пишется). +2. **Осознанно расширить границу** — явно вписать в контракт, что «exception + text» теперь ограниченно (≤200 символов) покидает барьер именно в + диагностическом блоке/dev-логе (не в основном тексте диалога), обновить + `docs/CANVAS.md` и `docs/ARCHITECTURE.md` тем же изменением и переписать три + `assert.doesNotMatch` на положительные проверки truncation/redaction, а не + просто удалить их. + +Любой из путей закрывает находку; молчание — не закрывает. Без этого правки +пункт DoR «модель данных… решены» не выполнен: контракт меняет уже принятое +архитектурное решение без ссылки на него. + +### M1 (Medium, в скоупе) — предикат «версия карточки ≠ версия интеграции» (§1.5) ссылается на несуществующий канал данных + +`§1.5`: *«`gs.preflight_update_hint` показывается только если версия карточки +!== версия интеграции (доступно из hass-конфига интеграции/manifest…)»*. Это +утверждение проверено и не подтверждается кодом: + +- `houseplan/config/get` (`custom_components/houseplan/websocket_api.py:1146-1161`, + ответ фронту при каждой загрузке конфигурации) не несёт `integration_version` + вообще — только `config`, `rev`, `virtual_lights`, `can_write`, + `can_optimize_undo`, `undo_kind`. +- `hass.config` (стандартный core-конфиг HA, доступный карточке) не содержит + версию произвольной кастомной интеграции — это не «hass-конфиг интеграции». +- Единственный реальный сигнал во всей системе — query-параметр `?v=` + на зарегистрированном Lovelace-ресурсе (`custom_components/houseplan/__init__.py:116`, + `module_url = f"{FRONTEND_URL}?v={VERSION}"`), который карточка теоретически + могла бы прочитать из `import.meta.url`/`document.currentScript.src` в момент + загрузки модуля — но это (а) не «hass-конфиг/manifest», как написано в ТЗ, + (б) отдельный, неочевидный механизм, ни разу не упомянутый в контракте, + (в) отражает версию интеграции на момент последней регистрации ресурса, а не + гарантированно текущую — то есть именно то расхождение, которое AC6 пытается + поймать, может дать неверный результат этим же путём. +- Поле `integration_version` существует только в **другом**, не относящемся к + делу контексте — снимке резервной копии (`import_export.py:526`, + `document.get("integration_version")`, используется диалогом импорта бэкапа, + `houseplan-card.ts:16263`) — это версия, записанная В МОМЕНТ ЭКСПОРТА файла, + а не текущая версия работающей интеграции. + +AC6 («показывается при расхождении… юнит на предикат + смок одной из веток») +специфицирован против источника данных, которого не существует, и ни аналитика, +ни ТЗ не называют backend-файл затронутой поверхностью — то есть пункт DoR +«перечислены затронутые файлы и модули» неполон, если для AC6 в действительности +нужна правка `websocket_api.py`. + +Дешёвое и точное решение — то же, что уже сделано для бэкапов: добавить +`"integration_version": VERSION` в ответ `houseplan/config/get` +(зеркально `import_export.py:526`), которое карточка и так получает на каждую +загрузку конфигурации. Контракт должен явно это назвать и добавить +`custom_components/houseplan/websocket_api.py` (плюс backend-юнит) в затронутые +файлы. В скоупе задачи — чинится здесь же, отдельный issue не заводится. + +### L1 (Low) — несогласованность `detail: null` в примере блока + +`§1.4` показывает пример `"detail": ""` для всех записей, а +`§1.1` говорит, что для причин без исключения (`wall-null`, +`wall-degraded-extra`, `wall-failed-core`, `floor-null`) `detail` **не +заполняется** — то есть поле у объекта отсутствует (`undefined`), а не равно +`null` (`JSON.stringify` вообще выкинет отсутствующее поле, а не запишет +`null`). Реализующему придётся угадывать, какая форма верна. Правится одной +фразой в §1.4 («поле отсутствует для причин без исключения») — не блокирует. + +### L2 (Low) — влияние на производительность не названо явно + +DoR (§2.5 PROCESS.md) требует явного пункта «влияние на производительность… или +явно "нет"». В спеке §3/§4 это не сказано ни разу (только UX/данные/i18n/touch и +риски). Влияние очевидно нулевое (один `console.warn` на неуспешный preflight, +с дедупликацией по fingerprint — §4.2), но пункт должен быть явным, а не +подразумеваемым. Правится одной строкой. + +## Что проверено и корректно + +- **§0 (сценарий/до-после) и §7.1**: все обязательные разделы PROCESS.md §7.1 + присутствуют (сценарий, что видит человек до/после — явно размечено «До:»/ + «После:», проблема, скоуп/не-скоуп, контракт, UX/данные/i18n/touch, риски, AC + с доказательствами, план тестов, откат, release-артефакты). +- **Технические утверждения о текущем коде** — все совпадают с реальным кодом: + 7 значений `OptimizeGeometryFailureReason` (`plan-geometry-preflight.ts:60-67`) + — совпадает; отсутствие i18n-ключей для причин — подтверждено (только + `gs.align_preflight_failed/hint/space/more` существуют, ни одного + `preflight_reason_*`); диалог печатает только `{spaces}` — подтверждено + (`houseplan-card.ts:17033`); две точки вычисления preflight + (`_previewAlignDialog`/`_runAlignToGrid`, строки близки к заявленным + `:15860`/`:15891`, разошлись на несколько строк из-за более свежего HEAD, что + не меняет смысл находки аналитика) — подтверждено. +- **Безопасность типа для остальных потребителей `checkSpacePhysicalGeometry`**: + проверил все шесть точек вызова (`houseplan-card.ts:7155, 7326, 7385, 7498, + 7523, 8865`) — все читают только `.ok`, не `.reason`/`.detail`. Расширение + общего интерфейса полем `detail` не протекает в другие потребители кроме + диалога Optimize — само по себе расширение типа не создаёт нового скоупа. +- **Мутанты (§5.7)** используют существующий паттерн `test/mutation-gate.test.mjs` + («гварды с типовым прологом») — не новая инфраструктура, реализуемо. +- **Клипборд-фолбэк** (`
`) — паттерн уже есть в этом же диалоге
+  (`gs.optimize_details`/`
`), решение + однородно с остальным UI. +- **i18n**: паритет RU/EN и заполненность проверяются существующим + `test/i18n.test.mjs`; жёсткого формата ключей (запрет дефиса в хвосте ключа) + тест не требует — ключи вида `gs.preflight_reason_wall-degraded-extra` + технически валидны, хоть и не совпадают со стилем соседних ключей (не + блокирует, не стал заводить как Low — чисто стилистически). +- **Скоуп/не-скоуп**: рантайм-отказ владельца прямо исключён из скоупа с + внятной причиной (диагностика для СЛЕДУЮЩЕГО отказа, не для текущего); + барьер #199, решётка #291, экспортный формат — явно не трогаются. Это + корректно резонирует с docs/SCOPE.md: задача не создаёт нового + пользовательского job, а делает диагностируемым уже существующий + административный инструмент (ближе к J6) — вне списка Core user jobs это не + выходит. +- **Откат** (§8) — один revert, ни модели, ни миграции, соответствует + фактическому объёму изменений (UI+i18n+dev-log, без схемы/бэкенд-хранилища). + +## Чего не проверял + +- Не проверял рантайм-отказ самого владельца (out of scope самой задачи — + корректно вынесен в отдельный будущий issue). +- Не гонял ни один гейт (`typecheck`/`test`/`build`/смоки) — на этапе ревью ТЗ + кода нет, для находок H1/M1 достаточно было прочитать существующий код и + существующие тесты; сам факт, что `test/plan-geometry-preflight.test.mjs:125, + 139, 145` красны при реализации §1.1 буквально, установлен чтением тестов и + логики (`assert.doesNotMatch` против буквального текста ошибки), не запуском — + это чтение кода, а не исполнение, но вывод из него однозначен и не требует + прогона, чтобы быть верным. +- Не проверял `docs/USER-GUIDE.ru.md` на предмет текущей формулировки раздела + «Оптимизировать» построчно — спека обещает туда абзац о диагностике (§7), + конкретного текста не предлагает, что на этапе ТЗ нормально (финальная + формулировка — предмет код-ревью/DoR, не блокирует спеку). +- Не искал полностью весь диапазон использования `contentFingerprint`/других + fingerprint-функций за пределами прямо процитированных мест — ограничился + тем, что относится к контракту §1.4. + +## Вывод + +Блокирует одна находка (H1) — контракт тихо отменяет задокументированную и +протестированную границу барьера #199, без единого упоминания этого в ТЗ. +Вторая (M1) в скоупе — предикат AC6 ссылается на источник данных, которого не +существует; чинится маленькой явной правкой backend-ответа. Обе технические, +не продуктовые — владельцу не выносятся, решение остаётся за автором в +следующей редакции ТЗ. L1/L2 — редакционные, можно закрыть тем же проходом. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 1 · Medium: 1 → в задаче**