mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 04:09:17 +00:00
claude[bot]
@@ -0,0 +1,186 @@
|
||||
# SPEC-REVIEW-434-r1
|
||||
|
||||
- Issue: https://github.com/Matysh/houseplan-card/issues/434
|
||||
- Этап: ревью ТЗ (PROCESS.md §2.4)
|
||||
- ТЗ: `docs/specs/434-v171-polish-audit.md`, проверяемый SHA автора `5566d6f9898e3c6d21f3e92a2ddf94536c86edc4`
|
||||
- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (полный трек, лимит 4)
|
||||
- Вердикт: **жёлтый**
|
||||
|
||||
## Скоуп
|
||||
|
||||
Follow-up к аудиту v1.71.0-beta.1 (`AUDIT-2026-09-03-v1710beta1.md` §3.3): девять
|
||||
независимых мелких дефектов на нескольких поверхностях — физический учёт/удаление
|
||||
orphan decor-blob'ов, capability guard в `houseplan-space-card`, revision-scoped
|
||||
resolve cache, честный `reused`, актуальность locale gate в danger confirmation и
|
||||
симметричная отмена, отрицательный свидетель Area-snapshot cleanup, bounded
|
||||
smoke-выполнение (per-route и per-file timeout), отзыв support preview token.
|
||||
Маршрут — полный (обоснование в ТЗ: несколько независимых контрактов, хранение
|
||||
пользовательских файлов, rolling compatibility, асинхронный safety lifecycle,
|
||||
несколько независимых гейтов — сложность выше лимита `small`); обоснование
|
||||
корректно, критерий лёгкого трека действительно не проходит.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Прочитаны: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §1–§8, §12; тело issue #434
|
||||
и оба комментария (аналитика + хендофф автора); ТЗ целиком (511 строк);
|
||||
`docs/CONFIG-COMPATIBILITY.md`, `docs/ARCHITECTURE.md` (разделы про
|
||||
content-addressed decor store), `docs/SUPPORT-PRIVACY.md`; связанные issue #417,
|
||||
#419, #432 (включая финальный код-ревью #432 для контекста integrity-кэша) и
|
||||
issue #435 (правило «таблица чем краснеет»).
|
||||
|
||||
Поскольку ТЗ на 90% состоит из утверждений о **текущем** поведении кода
|
||||
(«Подтверждённые причины»), а не только из предложений на будущее, каждое из
|
||||
девяти утверждений было сверено построчно с `origin/dev` — своей и параллельным
|
||||
агентом (независимая перепроверка, без пересечения выводов до сравнения):
|
||||
|
||||
| № | Утверждение ТЗ | Файл:строка на `origin/dev` | Результат |
|
||||
|---|---|---|---|
|
||||
| 1 | `read_catalog()` обходит только `*.json`, `_read_catalog_row()` требует `blob.is_file()` | `decor_assets.py:407`, `:388` | подтверждено дословно |
|
||||
| 2 | `HpConfigSnapshot` не переносит `decor_assets_api`; `space-card.ts` вызывает resolve безусловно; `houseplan-card.ts:4306` — с гардом | `config-store.ts:21-29` (нет поля); `space-card.ts:732`; `houseplan-card.ts:4306-4312` | подтверждено дословно |
|
||||
| 3 | `resolveCache` — одна пара id-set→Map на connection, без ревизии config | `decor-assets.ts:23,88-99` | подтверждено дословно |
|
||||
| 4 | `test_catalog_ignores_missing_or_malformed_sidecars` не проверяет «валидный sidecar, blob отсутствует» | `tests_backend/test_decor_assets.py:318-325` | подтверждено, кейс в файле отсутствует |
|
||||
| 5 | Recovery-ветка возвращает `reused:true` без предшествующей catalog-записи | `http_api.py:305-316` (условие `if blob.exists()` → `return row, True`) | подтверждено; фактический `return` на 5 строк ниже цитируемого диапазона — не искажает смысл |
|
||||
| 6 | `_dangerConfirmLocaleGate` — снимок прошлого рендера; открытый confirm не отменяется при переходе в `warm` | `houseplan-card.ts:2161` (поле), `:2183` (чтение), `:11228` (запись только в `_renderBody()`); ни один из `_cancelDangerConfirm()` call-sites (`:1607,2794,4188,7385,7446`) не привязан к смене locale gate | подтверждено; номера строк ТЗ приблизительные (±1–3), сама механика верна и явно помечена в ТЗ как ориентировочная |
|
||||
| 7 | `snapshotBindings.has(binding)` в `resolveAreaSnapshotCleanup()` не имеет отдельного отрицательного теста | `device-area-relocation.ts:171`; `test/device-area-relocation.test.mjs` | подтверждено, все существующие тесты либо берут binding из того же snapshot, либо используют пустой снапшот |
|
||||
| 8 | `germanStarted`/`germanCompleted` асимметричны; отдельный smoke-файл ограничен лишь job-таймаутом | `demo/smoke_danger_confirm_branches.mjs:79-84` — подтверждено дословно | **см. находку ниже** — цифра «20 минут» в самой ТЗ неверна |
|
||||
| 9 | `_buildSupportPreview()` бросает `support_rejected` до `_discardSupportPreview()` при валидном token, но невалидном другом поле | `houseplan-editor-runtime.ts:9190-9199` (throw), `:9226-9233` (catch без discard) | подтверждено дословно |
|
||||
|
||||
Дополнительно проверено: `docs/specs/README.md` — двусторонняя ссылка issue ↔ ТЗ
|
||||
на месте (строка 183); `scripts/mutation-gate.mjs` уже содержит мутанты для
|
||||
Python-файлов бэкенда (прецедент для AC1–AC4); `test/validate-workflow.test.mjs`
|
||||
— прецедент text-based контрактного теста над `.github/workflows/*.yml` (годится
|
||||
для AC9); `docs/CONFIG-COMPATIBILITY.md` раздел «Custom decor images…» подтверждает,
|
||||
что #432 не менял схему/URL/capability — согласуется с разделом ТЗ «Модель
|
||||
данных»; `docs/ARCHITECTURE.md` подтверждает content-addressed модель
|
||||
(`<64 hex>.<ext>`, sidecar JSON), на которой строится вся глава AC1–AC4; `reused`
|
||||
нигде не читается в `src/**` — уточнение его семантики действительно не является
|
||||
изменением публичного/видимого контракта.
|
||||
|
||||
## Находки
|
||||
|
||||
### [Medium, в скоупе] Неверная цифра «20-минутный timeout» job `smoke` — фактическая ошибка, а не предположение
|
||||
|
||||
**Где:** ТЗ, «Подтверждённые причины» п.8; раздел «Контракт поведения» §7
|
||||
(«Глобальный `timeout-minutes: 20`, детерминированное разбиение… не меняются»);
|
||||
раздел «Производительность» («не уменьшает 20-минутный общий бюджет job»);
|
||||
раздел «Риски» («сохраняет глобальные 20 минут»).
|
||||
|
||||
**В чём дефект:** в `.github/workflows/validate.yml` `timeout-minutes: 20`
|
||||
принадлежит **другой** job — `performance_smoke` (строка 715). Job `smoke`
|
||||
(объявлена на строке 486, шаг цикла `for f in demo/smoke_*.mjs` — строка 554)
|
||||
**не имеет собственного `timeout-minutes` вообще** — в файле ровно одно
|
||||
вхождение слова `timeout`, и это не она. Без явного значения GitHub Actions
|
||||
использует дефолт 360 минут, а не 20. Сам issue #434 в исходной формулировке
|
||||
пункта (з) написан точно: «у smoke-job в `validate.yml` своего
|
||||
`timeout-minutes` тоже нет» — то есть корректный факт был в issue, а при
|
||||
переносе в ТЗ он превратился в конкретную (неверную) цифру, не помеченную как
|
||||
предположение.
|
||||
|
||||
**Почему это находка, а не мелочь:** это утверждение — не проходной
|
||||
комментарий, а часть контракта, который ТЗ прямо объявляет неизменным
|
||||
(«не меняются», «не уменьшает»). Реализатор, доверяющий тексту ТЗ, будет
|
||||
считать, что job уже ограничена 20 минутами, и не задаст себе вопрос, нужно ли
|
||||
явно выставить `timeout-minutes` на job `smoke` в рамках этой же задачи (item
|
||||
8/AC9 «bounded execution» — ровно про то, чтобы ни один smoke не мог удерживать
|
||||
раннер бесконечно). Сейчас после фикса по-прежнему не будет верхней границы на
|
||||
уровне job — только на уровне отдельного файла (180 c + 10 c kill grace,
|
||||
максимум ~63 файла), что для 3 шардов и текущего числа смоков даёт время
|
||||
исполнения, которое ТЗ не оценивает и не ограничивает.
|
||||
|
||||
**Воспроизведение:** `grep -n timeout .github/workflows/validate.yml` →
|
||||
единственное совпадение на строке 715 внутри job `performance_smoke` (строки
|
||||
705–781); job `smoke` — строки 486–586, `timeout-minutes` в её теле нет.
|
||||
|
||||
**Что нужно исправить:** либо (a) явно написать в ТЗ, что у job `smoke`
|
||||
сейчас нет собственного ограничения (дефолт GitHub 360 минут), и explicit
|
||||
решить/зафиксировать — фиксируется ли `timeout-minutes` на уровне job этой же
|
||||
задачей, либо остаётся полагаться только на per-file guard; либо (b) если
|
||||
решение «job-level timeout не трогаем» осознанное — убрать из «Контракта» и
|
||||
«Рисков» формулировки, которые ссылаются на несуществующие «текущие 20 минут»
|
||||
как на неизменную величину. Это техническое решение (§7.1: «где хранится
|
||||
состояние» — техническое, «что видит пользователь» — нет), поэтому чинится
|
||||
автором ТЗ без обращения к владельцу.
|
||||
|
||||
Без High-находок это жёлтый вердикт: находка в скоупе задачи (это тот же раздел
|
||||
7/AC9, который задача и меняет), правится в этом же ТЗ, повторный цикл — код не
|
||||
пишется до зелёного ревью ТЗ.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Обязательные разделы §7.1 присутствуют все: сценарий, что человек увидит
|
||||
до/после, проблема («Подтверждённые причины»), скоуп и не-скоуп, контракт
|
||||
поведения (8 подпунктов), UX/accessibility/touch/kiosk/i18n, модель данных и
|
||||
совместимость, критерии приёмки AC1–AC12 с указанием способа доказательства,
|
||||
план автотестов (10 шагов), риски, откат, release-артефакты.
|
||||
- Продуктовые первые два раздела отвечают на оба обязательных вопроса:
|
||||
персона/поверхность/момент (Home admin, оба редактора и обе карточки,
|
||||
штатная эксплуатация + редкий аварийный останов HA) и что видно до/после —
|
||||
без терминов реализации.
|
||||
- Из девяти пунктов «Подтверждённые причины» восемь с половиной проверены
|
||||
дословно точным построчным совпадением с `origin/dev` (см. таблицу выше);
|
||||
ни одна из них не оказалась догадкой, выданной за факт. Автор явно и честно
|
||||
пометил номера строк как ориентировочные там, где они действительно немного
|
||||
разошлись (п.6), и отдельным блоком «Принятые технические предположения» —
|
||||
все решения, которые не являются продуктовыми и не требуют владельца.
|
||||
- Таблица «чем краснеет» (#435) заполнена для всех десяти AC без пустых
|
||||
третьих столбцов; для AC5/AC6 (чистые фронтенд-юниты) корректно применена
|
||||
льгота §2.7 «для чистых юнитов достаточно прогона со снятой защитой» вместо
|
||||
обязательного постоянного мутанта — автор явно прочитал и применил именно
|
||||
эту оговорку, а не общее правило.
|
||||
- Не-скоуп корректно исключает смежные, но более крупные работы: полный
|
||||
рефакторинг LanguageRuntime/support pipeline/smoke-шардирования, повышение
|
||||
`decor_assets_api`/schema/export version, изменение видимого текста/UI.
|
||||
Это не даёт задаче расползтись за пределы девяти найденных дефектов.
|
||||
- Изменение семантики `reused` не является изменением видимого/публичного
|
||||
контракта: поле нигде не читается в `src/**` (проверено grep), так что
|
||||
уточнение не требует продуктового решения владельца и не ломает
|
||||
`docs/CONFIG-COMPATIBILITY.md`.
|
||||
- Явное удаление orphan-blob'ов (AC3) не противоречит standing rule
|
||||
`docs/SCOPE.md` «никогда не удалять файл по предположению»: причиной
|
||||
остаётся явный вызов `houseplan/assets/delete` с точным asset id, а не
|
||||
вывод из отсутствия ссылок — ТЗ прямо проговаривает это соответствие.
|
||||
- AC1–AC12 однозначны и у каждого назван способ доказательства
|
||||
(`backend/unit`, `backend/HA`, `unit/smoke`, `browser smoke`,
|
||||
`unit/CI contract`, `review/docs`, `gates`); ни один не описывает решение
|
||||
расплывчато настолько, чтобы реализация могла разойтись с намерением.
|
||||
- Технический прецедент для новых механик подтверждён по репозиторию:
|
||||
мутанты для Python-файлов уже есть в `scripts/mutation-gate.mjs` (AC1–AC4
|
||||
реализуемы тем же способом); `test/validate-workflow.test.mjs` — рабочий
|
||||
образец YAML-контрактного теста без внешней зависимости (годится для AC9).
|
||||
- Раздел «Откат» корректно называет границы: миграции нет, восстановленные
|
||||
sidecar остаются обычными валидными записями и безопасны при откате.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не проверялся сам код реализации — его ещё нет, это ревью ТЗ, не код-ревью;
|
||||
гейты (`typecheck`/`test`/`build`/backend pytest) не запускались, так как
|
||||
диапазон `origin/dev...HEAD` для этой ветки — это документация (только
|
||||
ТЗ и README, класс C), продуктовый код не менялся.
|
||||
- Не проверялась точность построчных ссылок за пределами девяти утверждений
|
||||
из «Подтверждённые причины» (например, конкретные номера в «Затронутые
|
||||
модули») — они не заявлены как проверяемые факты, а как ожидаемый список
|
||||
файлов, ТЗ прямо говорит «выделение чистых helpers допустимо».
|
||||
- Не оценивалось время исполнения per-file timeout (180 c × число смоков ×
|
||||
3 шарда) относительно фактического суммарного бюджета CI — сама находка
|
||||
выше означает, что этот бюджет ТЗ пока не называет корректно; оценка того,
|
||||
сколько это должно быть в минутах, — предмет исправления ТЗ, не этого
|
||||
ревью.
|
||||
- Не проверялся код `#432` дальше, чем нужно для контекста (не переисследовал
|
||||
его собственное код-ревью по существу — оно уже принято зелёным на своём
|
||||
цикле).
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/434-v171-polish-audit`, коммит `5566d6f9898e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `1e1861969610794ffa6a9d8458b52ef3695496b8`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 1e1861969610
|
||||
```
|
||||
- ТЗ `docs/specs/434-v171-polish-audit.md`, блоб `e0229c0cd6550a1c44b978335c52bd2b264b1c52`
|
||||
```
|
||||
git log --all --find-object=e0229c0cd6550a1c44b978335c52bd2b264b1c52 -- docs/specs/434-v171-polish-audit.md
|
||||
```
|
||||
Reference in New Issue
Block a user