diff --git a/docs/reviews/SPEC-REVIEW-434-r1.md b/docs/reviews/SPEC-REVIEW-434-r1.md new file mode 100644 index 00000000..caf2d433 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-434-r1.md @@ -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>.`, 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` дальше, чем нужно для контекста (не переисследовал + его собственное код-ревью по существу — оно уже принято зелёным на своём + цикле). + +--- + + + +## Материал раунда + +- Ветка: `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 + ```