mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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=<VERSION>`
|
||||
на зарегистрированном 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": "<message или null>"` для всех записей, а
|
||||
`§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`
|
||||
(«гварды с типовым прологом») — не новая инфраструктура, реализуемо.
|
||||
- **Клипборд-фолбэк** (`<details><pre>`) — паттерн уже есть в этом же диалоге
|
||||
(`gs.optimize_details`/`<details class="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 → в задаче**
|
||||
Reference in New Issue
Block a user