houseplan-card/docs/reviews/SPEC-REVIEW-421-r1.md

SPEC-REVIEW-421-r1

• Issue: https://github.com/Matysh/houseplan-card/issues/421
• Этап: S4-spec-review (ревью ТЗ, PROCESS.md §2.4)
• Материал: docs/specs/421-negative-test-proofs.md на коммите c409ec6a (ветка origin/issue/421-negative-test-proofs), тело issue #421, комментарий-аналитика и комментарий «ТЗ готово» того же автора.
• Заход: r1 (первый раунд, разбор полный по определению).

Скоуп ревью

Задача — техдолг: три существующих защитных проверки объявляют доказательство, которого фактически не дают (support-preview lifecycle, reportPageErrors(), acceptance-trace приёмки скриншотов документации). ТЗ утверждает, что production-поведение не меняется, меняются только тесты/гейты/CLI-инструменты (классы B по PROCESS.md §1). Продуктовых вопросов автор не поднимал — их действительно нет: наблюдаемого пользователем поведения задача не касается.

Проверка велась состязательно: не «звучит ли складно», а совпадает ли описание текущего кода в ТЗ с самим кодом, и реализуем ли план технически на существующей инфраструктуре гейтов.

Как проверялось

Гейты кода не гонялись — на этом SHA существует только документ ТЗ (docs-коммит c409ec6a), реализации ещё нет, поэтому typecheck/test/build к предмету ревью (тексту ТЗ) неприменимы; они станут предметом код-ревью следующего этапа. Вместо этого сверено содержание ТЗ с действующим кодом трёх подсистем:

1. docs/SCOPE.md, PROCESS.md (§1, §2.4, §2.5, §5, §7.1), AGENTS.md — прочитаны полностью до разбора ТЗ.
2. custom_components/houseplan/websocket_api.py:2126-2296 — прочитан код ws_support_preview, ws_support_preview_discard, ws_support_submit, _prune_support_previews (строки 237-241). Сверено с описанием в ТЗ и в теле issue.
3. demo/serve.mjs:44-114 — прочитан код roundTripLivePages(), reportPageErrors(), finish(). Сверено с описанием отдельного round-trip в reportPageErrors().
4. demo/guard/verify-guard.mjs, demo/guard/README.md — прочитаны правила размещения проб (не smoke_*, не demo/fixtures/, отдельный каталог вне корпуса sourceFingerprint).
5. scripts/docs-accept.mjs (целиком), scripts/docs-acceptance.mjs (docsAcceptancePlan), test/docs-accept.test.mjs — сверено, что main() действительно строит accepted.acceptance инлайн и что существующий тест бьёт только по verifyDocsCandidate/docsAcceptancePlan, не по этой ветке.
6. scripts/mutation-gate.mjs — прочитан формат реестра (id/guard), существующие мутанты smoke-guard-blind-to-tail и smoke-guard-forgets-to-register-pages, а также использование scripts/backend-test-guard.mjs для адресного запуска одного HA-теста (подтверждает «Принятое предположение» №2 ТЗ).
7. .github/workflows/validate.yml:500-542 — сверено, где именно вызывается verify-guard.mjs (job с реальным Chromium, шард 1, на каждый push) и что это отдельно от дорогого mutation-gate.yml (пре-релизный).
8. scripts/source-fingerprint.mjs — сверено, что корпус sourceFingerprint ограничен src/** + rollup.config.mjs/tsconfig.json/package-lock.json; scripts/** и demo/guard/** в него не входят, поэтому изменения ТЗ не требуют пересъёмки документационных скриншотов, как и заявлено в ТЗ.
9. tests_backend/test_ha_websocket.py:1328-1353 — прочитан существующий тест test_support_preview_replacement_and_discard_are_draft_local полностью: подтверждено, что assert-ов ровно два (уникальность трёх токенов и три успешных discard), submit заменённым/удалённым токеном не отправляется вовсе.

Находки

Medium (в скоупе задачи) — отсутствуют обязательные разделы §7.1

Файл: docs/specs/421-negative-test-proofs.md

PROCESS.md §7.1 перечисляет обязательные разделы ТЗ: сценарий · что человек увидит до и после · проблема · скоуп/не-скоуп · контракт поведения · UX · модель данных и миграция · i18n · AC с доказательством · план автотестов · риски · откат · release-артефакты. В документе отсутствуют явные разделы «Сценарий», «Что человек увидит до и после» и «UX»; «Модель данных и миграция» не выделена отдельно (частично покрыта фразой в разделе «i18n, compatibility, touch и security»: «persisted config, import/export и WebSocket schema не меняются» — но это про конфиг пользователя, не про артефакт screenshots.json, который сама задача трогает).

Почему это не придирка. Правило явно объясняет причину: «ТЗ, которое не может ответить на эти два вопроса, описывает работу, а не изменение продукта» (§7.1). Эта задача действительно не меняет продукт — и именно поэтому короткий явный ответ («сценария нет: ни одна из трёх персон docs/SCOPE.md не наблюдает эффект этой задачи, наблюдаемое поведение до и после идентично») должен стоять на своём месте, а не подразумеваться разбросанными по разным разделам фразами. Без него DoR-чеклист §2.5 («ТЗ существует, ревью зелёное») формально сверяется по неполному документу, и прецедент — что фраза «не меняется», сказанная вскользь в разделе про touch/i18n, заменяет собой продуктовый раздел — плохо масштабируется на следующие тест-инфраструктурные задачи, где эффект на самом деле есть, но автор по инерции решит, что раздел необязателен.

Как воспроизвести: открыть docs/specs/421-negative-test-proofs.md и поискать заголовки «Сценарий», «Что человек увидит», «UX» — их нет; есть «Цель», «Проблема», «Скоуп», «Не-скоуп», «Контракт», «Затрагиваемые файлы», «i18n, compatibility, touch и security», «Производительность», «Критерии приёмки», «План отрицательной проверки», «Риски», «Откат», «Release-артефакты», «Принятые предположения».

Что чинит находку: добавить в тот же документ короткий явный блок (2-4 предложения на каждый из трёх недостающих разделов достаточно): кто и когда эту работу наблюдает (никто — задача не меняет наблюдаемое поведение, подтверждается AC9), почему UX не описывается (нет UI-поверхности), и что единственный затронутый «данные»-артефакт — недокументный docs/images/screenshots.json.acceptance, который не подпадает под CONFIG-COMPATIBILITY.md и не мигрирует, а расширяется совместимо (поле lastWriteWasFingerprintOnly уже описано в разделе «Контракт», просто не под этим заголовком). Правки не расширяют скоуп и не меняют ни один AC.

Low — неточная формулировка каденции новой browser probe (снимаю с записью)

Файл: docs/specs/421-negative-test-proofs.md, раздел «Производительность»: «Новая browser probe и адресные мутанты входят в дорогой mutation/release gate и не запускаются на каждом обычном frontend unit прогоне сверх уже существующего verify-guard...». Читается так, будто новая проба — часть дорогого пре-релизного гейта. По факту verify-guard.mjs (куда AC4 и раздел «Затрагиваемые файлы» explicitly кладут новую пробу) уже вызывается в .github/workflows/validate.yml:542, в лёгкой job «Смоки в браузере», на каждый push, а не только перед релизом — дорогой является лишь часть с адресными мутантами scripts/mutation-gate.mjs.

Почему не поднимаю до Medium: AC4/AC5 и раздел «Затрагиваемые файлы» однозначно фиксируют, что проба живёт в verify-guard.mjs и мутант — в mutation-gate.mjs; реализатор пойдёт за точными разделами, а не за одной обзорной фразой в «Производительности». Снимаю решением ревьюера с записью здесь, автор может поправить формулировку по желанию, блокирующей силы не имеет.

Что проверено и корректно

• Фактическая точность описания текущего кода. Все три «разрыва доказательности», на которых строится ТЗ, подтверждены чтением production-кода, а не пересказом issue:
  • ws_support_preview_discard действительно отвечает {"ok": True} идемпотентно, включая отсутствующий токен (websocket_api.py:2242-2247);
  • test_support_preview_replacement_and_discard_are_draft_local действительно не отправляет submit ни одним из трёх токенов после discard — только проверяет успешность самого discard;
  • reportPageErrors() (demo/serve.mjs:98-99) и finish() (demo/serve.mjs:108-113) вызывают roundTripLivePages() раздельно — зарегистрированные мутанты (scripts/mutation-gate.mjs:120-141) ломают только путь finish()/_livePages;
  • scripts/docs-accept.mjs:150-159 действительно строит acceptance инлайн в main(), читая previous напрямую из файла на диске — вне любой экспортируемой, тестируемой функции.
• AC1–AC9 однозначны и у каждого назван способ доказательства (unit/ backend test/адресный мутант/review), включая явное указание, что зелёный прогон без зафиксированного отрицательного прогона не закрывает AC (раздел «План отрицательной проверки» — прямое применение правила ревью «тест должен уметь падать» уже на этапе ТЗ).
• Технически план реализуем на существующей инфраструктуре, без новых зависимостей и без нового процессного механизма:
  • _prune_support_previews(rt, now) уже принимает now параметром, а time.monotonic() — module-level импорт в websocket_api.py, значит monkeypatch TTL детерминированным способом (AC3) осуществим без sleep;
  • существующий demo/guard/README.md уже объясняет, почему новая проба должна жить именно в demo/guard/, а не в demo/smoke_* или demo/fixtures/ (в т.ч. чтобы не задеть sourceFingerprint) — план ТЗ следует этому правилу, а не изобретает своё;
  • scripts/backend-test-guard.mjs существует и уже используется в mutation-gate.mjs для запуска одного именованного backend-теста — «Принятое предположение» №2 подтверждено, не является голой догадкой;
  • docs/images/screenshots.json.acceptance не читается никаким другим гейтом/скриптом кроме docs-accept.mjs (единственное совпадение вне самого ТЗ и docs-accept.mjs — несвязанные документы SPEC/CODE-REVIEW-406 по слову «acceptance»), поэтому расширение поля совместимо и не требует миграции.
• Не-скоуп сформулирован жёстко и корректно отсекает изменение WebSocket-контракта, idempotency-семантики discard, UI Help & feedback, общего smoke harness и правил приёмки скриншотов — совпадает с тем, что показало чтение кода: реального различия в поведении между «что есть» и «что должно быть по контракту» найдено не было, только в доказательности.
• «Принятые предположения» — действительно предположения, а не факты, выданные за решение: помечены явно, ревьюер вправе их оспорить (оспаривать не стал — они подтверждены чтением кода выше).
• Выбор полного трека вместо small обоснован названным критерием («нарушен критерий одной поверхности: три независимых защитных контура», комментарий-аналитика) — соответствует требованию PROCESS.md §5 называть нарушенный критерий явно, а не писать «обычный трек» без обоснования.
• Риски секции адресуют именно те способы, которыми доказательство может остаться слабым (маскировка relay-мока, flaky TTL-тест, повторное попадание пробы в finish(), коллизия anchor мутанта, расхождение CLI/unit-хелпера) — это ровно те точки, которые я independently проверял бы при код-ревью; их называние в ТЗ снижает будущий риск и на код-ревью.

Чего не проверял

• Гейты typecheck/npm test/npm run build — неприменимо: на этом SHA реализации ещё нет, гейты будут предметом код-ревью.
• Собственно исполнение отрицательных проб (мутантов) — их ещё нет; план проверен на реализуемость чтением инфраструктуры (mutation-gate.mjs, verify-guard.mjs, backend-test-guard.mjs), не запуском.
• Полный текст custom_components/houseplan/websocket_api.py и scripts/docs-accept.mjs вне участков, относящихся к трём заявленным швам — вне скоупа задачи, чтение сосредоточено на затронутых строках.
• docs/TESTING.md — не читал; ТЗ верно оставляет его правку условной («только если без короткого описания новых guard IDs невозможно обнаружить из runbook»), решение по факту реализации.

Материал раунда

• Ветка: origin/issue/421-negative-test-proofs
• SHA: c409ec6a
• ТЗ: docs/specs/421-negative-test-proofs.md на этом SHA
• Issue: #421, комментарии аналитика и автора ТЗ прочитаны полностью

Вердикт

Жёлтый. AC полны, доказуемы и технически реализуемы; описание текущего кода в ТЗ точное (проверено чтением production-кода по всем трём швам). Единственная находка — формальный пробел в обязательных разделах §7.1 («Сценарий», «Что человек увидит», «UX»), который не меняет ни один AC и чинится добавлением нескольких предложений в тот же документ.

---

Материал раунда

• Ветка: issue/421-negative-test-proofs, коммит c409ec6a61b5 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: e7d26df9bdc0c4b77e33a398ff79ed5f0c3ee21b

  git log --all --format='%H %T' | grep e7d26df9bdc0
  

• ТЗ docs/specs/421-negative-test-proofs.md, блоб 1a6b112c576747e4bf99f02fc890926c3bbd0b1b

  git log --all --find-object=1a6b112c576747e4bf99f02fc890926c3bbd0b1b -- docs/specs/421-negative-test-proofs.md