Files
houseplan-card/docs/reviews/SPEC-REVIEW-43-r1.md
2026-09-02 00:05:36 +00:00

20 KiB
Raw Permalink Blame History

SPEC-REVIEW-43-r1

  • Issue: https://github.com/Matysh/houseplan-card/issues/43
  • Ревьюер: Claude (роль «Ревьюер ТЗ», PROCESS.md §2.4)
  • Материал: docs/specs/043-private-support-report.md, ревизия 2, на коммите 755fa4cf (ветка issue/43-help-feedback), плюс тело issue #43 и все 7 комментариев (аналитика 2026-08-14/2026-08-30, финальное UX-описание владельца 2026-09-01, повторная аналитика, Q1–Q5 и ответы владельца, хендофф автора «ТЗ полностью переработано»).
  • Заход: r1 (первый разбор этого ТЗ, документ прежних раундов не существует — предыдущая ревизия ТЗ была отклонена самим автором в аналитике 2026-09-01, а не ревью, поэтому раздел «Унаследовано» не применяется).
  • Трек: полный (аналитик явно назвал непройденные критерии small: больше одной поверхности, новый UX-контракт, новый внешний transport, privacy/security-контракт, обязательное влияние на touch-View) — лимит циклов ревью ТЗ 4, не 2.

Скоуп

ТЗ описывает новый диалог «Помощь и обратная связь» в шапке карточки: перенос блока «О карточке», языковая ссылка на USER-GUIDE, форму контакт/сообщение и opt-in обезличенный support-пакет с точной геометрией, который уходит через backend House Plan в отдельный project-controlled HTTPS-relay (новый deployable сервис support-relay/) в закрытый support-mailbox. Это первая ревизия, дошедшая до ревью; предыдущая (clipboard-only) была отозвана автором ещё в аналитике.

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

  1. docs/SCOPE.md, AGENTS.md, PROCESS.md (целиком, включая §1, §2.3–2.10, §4, §7.1–7.2) — прочитаны до разбора ТЗ.
  2. Issue #43: тело + все 7 комментариев через gh issue view --json (mcp__github__* были недоступны без разрешения) — восстановлена полная цепочка решений владельца (финальное UX-описание → Q1–Q5 → ответы).
  3. Построчное чтение docs/specs/043-private-support-report.md (646 строк) против §7.1 обязательных разделов, продуктовых вопросов из issue и docs/TOUCH-SUPPORT.md.
  4. Сверка утверждений §3 («подтверждённое текущее состояние») с реальным кодом: src/houseplan-card.ts (кнопка General Settings, _norm && _canEdit, порядок zoom→settings, ленивый import('./houseplan-editor-runtime')), src/houseplan-editor-runtime.ts (блок «О карточке», gs.about_* ключи), custom_components/houseplan/{websocket_api,import_export}.py (create_export), repairs.py (broken_plan_<spaceId>), src/i18n/{ru,en,de,fr}.json (RU/EN/DE/FR уже существуют).
  5. Сверка i18n-подписи и терминологии («О карточке», «Общие настройки», GitHub/Telegram-ссылки) с docs/USER-GUIDE.ru.md и src/i18n/ru.json — расхождений не найдено.
  6. docs/CONFIG-COMPATIBILITY.md — подтверждено, что фича не трогает схему config/layout (ТЗ §10 корректно).
  7. docs/TOUCH-SUPPORT.md — сверка UX/§11 с контрактом «View dialogs and safe device actions: Fully supported» и правилом обязательной пометки Touch editor: ….
  8. docs/specs/README.md — обязательный раздел release-артефактов и двусторонняя ссылка issue↔ТЗ (строка 106) присутствуют.
  9. scripts/process-gate.mjs прочитан целиком по функции classify() и условиям fail(1, …)/fail(8...), чтобы проверить, как гейт классифицирует новый путь support-relay/**, которого ТЗ вводит впервые.

Гейты

Гейт Результат
npx tsc --noEmit зелёный, 6.2 с
node scripts/process-gate.mjs «гейт пройден, предупреждений 0» (офлайн, диапазон origin/dev..HEAD, 1 коммит)
npm test, npm run build, check-docs.mjs, смоки, model-invariants, pytest tests_backend не прогонялись — диф коммита 755fa4cf строго класса C: docs/specs/043-private-support-report.md + docs/specs/README.md (git show --stat HEAD), src/**/custom_components/** не тронуты. Эти гейты бессмысленны для чисто документного диффа на этапе ТЗ; check-docs.mjs условен на изменения src/**, которых нет

Трейлеры коммита 755fa4cf: Issue: #43, User-Visible: no — корректны для docs-only правки.

Находки

Medium (в скоупе задачи) — support-relay/** не попадает ни в один класс изменений

Файл: docs/specs/043-private-support-report.md, §9.1 (строки 404–416) и §18 (строки 606–617).

Суть: ТЗ явно вводит новый top-level каталог support-relay/ — «separately deployable service, excluded from the HACS artifact» — и явно кладёт «минимальный deployable relay» в объём #43 (§5.1). Ни AGENTS.md (таблица «Change classes»), ни PROCESS.md §1 не знают такого пути: класс A ограничен src/** и custom_components/houseplan/**/*.py, класс B — конкретным списком (test/**, tests_backend/**, demo/**, scripts/**, .github/**, …), класс C — документацией. support-relay/** не match-ится ни одним regex.

Я прочитал scripts/process-gate.mjs::classify() (строки 93–99): путь, не попавший ни в CLASS_D/A/B/C, получает '?'. Строка 255–256 превращает это только в предупреждение (warn(0, …)), а не в отказ. Требование трейлера Issue: #NN (правило 1, строка 232) и проверка статуса issue через --issues (правило 8, строки 310/392/510) применяются только когда c.classes.has('A') || c.classes.has('B') — то есть коммит, который трогает только support-relay/**, эти проверки не запускает вовсе.

Сценарий отказа: после старта реализации коммит вида feat: relay rate limiting с diff только внутри support-relay/ пройдёт process-gate.mjs (и локальный pre-push) без единого трейлера Issue:, без метки issue в S5-ready/S6-in-progress/S7-code-review, без ссылки на ТЗ — то есть ровно то, что правило №1 AGENTS.md объявляет запрещённым («Изменение продуктового кода без issue запрещено»). Для обычного продукта это было бы досадной дырой в тулинге; здесь эта дыра открыта ровно в подсистеме, которая пересылает точную геометрию дома и контакт пользователя на внешний сервер — там же, где PROCESS.md требует наибольшей строгости.

Почему это находка ТЗ, а не готового кода: §18 «Documentation and release artifacts» перечисляет docs/USER-GUIDE, docs/ARCHITECTURE.md, docs/SUPPORT-PRIVACY.md, docs/TESTING.md, relay README — но не упоминает обновление таблицы классов в AGENTS.md/PROCESS.md §1. Без явного решения в ТЗ реализация имеет прямой путь создать неклассифицированный каталог и получить только warning вместо gate.

Почему это не продуктовый вопрос владельцу: это техническое решение (файловая раскладка / расширение таблицы классов), которое, по §7.1 PROCESS.md, агенты решают и фиксируют сами («Всё, чего пользователь не наблюдает, агенты решают сами… раскладка файлов»). Автору достаточно добавить в ТЗ (например, в §9.1 или §18) одно из двух решений:

  • либо явно расширить класс A (или завести отдельный подкласс) на support-relay/** в AGENTS.md/PROCESS.md §1 тем же коммитом, где каталог появляется, и включить эту правку в release-артефакты §18;
  • либо разместить relay-код внутри уже классифицированного пути (например, под scripts/support-relay/ — класс B, или отдельно обсудить с владельцем вынос в отдельный репозиторий, если секреты не должны жить в этом дереве вообще).

Оставляю выбор автору — важно, чтобы решение было явно записано в ТЗ, а не осталось implicit-пробелом, который заметят только когда реальный коммит пройдёт гейт с одним warning.

Low (снимается с записью) — отсутствует явная пометка Touch editor: …

Файл: docs/specs/043-private-support-report.md, §11 (строки 461–476).

docs/TOUCH-SUPPORT.md требует: «New editor feature specifications … must state one of: Touch editor: supported; Touch editor: best effort / intentionally degraded; Touch editor: not exposed.» Диалог Help/Feedback доступен не только в View, но и во всех трёх редакторах (Plan/Devices/ Backdrop, §6.1), поэтому формально подпадает под это правило. §11 подробно и корректно описывает touch/a11y-контракт (44×44 px цель, 320 px раскладка, aria-live, фокус-менеджмент) — по содержанию это ровно Touch editor: supported, просто без канонической фразы, по которой обычно грепают такие декларации.

Снимаю как Low без возврата на цикл: содержательно контракт уже сильнее, чем «fully supported» из таблицы TOUCH-SUPPORT.md для View-диалогов, а сам диалог — не операция редактирования геометрии, а сквозной UI-элемент шапки. Автору имеет смысл добавить одну строку «Touch editor: supported» в §11 при следующей правке ТЗ ради единообразия аудита, но это не блокирует переход в S5-ready.

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

  • Обязательные разделы §7.1 — все на месте: сценарий (§1, персона Home admin из SCOPE.md, поверхность View+все редакторы), что человек увидит до/после (§2, одной фразой, без терминов реализации), проблема (§3), скоуп и не-скоуп (§5.1/5.2), контракт поведения (§4, §6–9), UX (§6, §11), модель данных и миграция (§10 — миграции нет, обоснованно: пакет строится на чтении, не меняет schema), i18n (§12), AC1…AC17 с доказательством (§13), план автотестов (§14), риски (§16), откат (§19), release-артефакты (§18).
  • Продуктовые вопросы правильно классифицированы и закрыты владельцем. Q1–Q5 в issue — все действительно продуктовые (куда уходят данные, что считается «бэкапом», что видно в preview, кто видит кнопку, есть ли контакт для ответа) — ни один не технический вопрос, вынесенный по ошибке. Ответы владельца дословно перенесены в §4/§6/§7/§9 ТЗ без искажений (сверено построчно): точная геометрия — да (Q2), preview точных байтов — да (Q3), только can_write, без kiosk (Q4), необязательный контакт вне support-пакета (Q5), HTTPS-relay через backend (Q1).
  • Технические предположения промаркированы. §20 «Принятые технические предположения» явно называет неочевидные архитектурные решения (email-relay вместо issue, package-local sequential псевдонимы вместо стабильного хэша, in-memory кеш preview, 30/24-часовые сроки хранения) как «assumed, change freely» — это именно то, что требует PROCESS.md §7.1, и защищает от «догадки, выданной за факт».
  • Утверждения о текущем состоянии (§3) проверены по коду, а не приняты на веру. Каждое из пяти технических утверждений подтвердилось построчно (см. «Как проверялось» п.4) — расхождений с реальным деревом не найдено.
  • AC однозначны и снабжены методом доказательства. Все 17 AC в таблице §13 имеют явную колонку Evidence с конкретным методом (browser smoke, DOM/i18n unit, backend test, adversarial security test, fake-clock test, performance benchmark) — ни одного «проверить вручную» или недоказуемого критерия не найдено.
  • «Одно число — один источник» уже спроектировано в контракт, а не оставлено на волю реализации. §6.4/§7.1/§8.1/AC5/AC10 и явный пункт mutation-gate «preview bytes are regenerated on submit → exact-byte test red» (§14.4) требуют, чтобы preview, download и отправленные байты были одним и тем же кешированным объектом с TTL-токеном, а не тремя независимыми вычислениями — ровно тот инвариант, который трижды ловил регрессии в проекте (#234, #233).
  • Privacy allowlist/exclusion списки (§7.2/7.3) внутренне непротиворечивы: каждый пункт «включено» имеет зеркальный запрет в «исключено» там, где это применимо (имена/ids/URL/бинарники/undo-backup/HA state исключены явно; точная геометрия включена явно и осознанно как продуктовое решение, а не побочный эффект).
  • i18n-контракт (RU/EN/DE/FR) соответствует уже существующим локалям проекта (src/i18n/{ru,en,de,fr}.json существуют) — фича не расширяет список поддерживаемых языков произвольно.
  • Cross-links. Issue #43 ↔ ТЗ ↔ docs/specs/README.md — ссылки в обе стороны на месте.
  • DoR-зависимости (§17) корректно вынесены как внешние операционные факты (деплой relay, mailbox, staging), а не спрятаны как «предположения»: ТЗ прямо говорит, что при их отсутствии после зелёного ревью issue должен получить S5-ready + blocked, а не мёртвую кнопку — это соответствует PROCESS.md §2.9 и §7.1 «риски перечислены».

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

  • Полные npm test/npm run build/browser-смоки/model-invariants/ pytest tests_backend — не прогонялись: диф чисто документный (класс C), ни один из этих гейтов не имеет предмета для проверки на этом коммите. Они станут обязательны на этапе код-ревью, когда появится реализация.
  • Реальная развёртываемость relay, содержимое support-relay/, secrets- handling — предмета для проверки ещё нет (кода нет), это заявленная DoR-зависимость §17, а не то, что должно быть в ТЗ.
  • Golden/скриншоты, performance-профили — не относятся к докс-only диффу этого раунда.
  • Соответствие ещё не написанных docs/SUPPORT-PRIVACY.md, docs/ARCHITECTURE.md-правок — эти документы появятся вместе с кодом (§18), не на этапе ТЗ.

Вывод

ТЗ методологически одно из самых тщательных, что проходили через этот процесс: 17 однозначных AC с доказательствами, явное разделение продуктовых решений владельца и технических допущений, встроенная защита от «одно число — два источника» и от privacy-регрессий через adversarial-фикстуру. Единственная блокирующая (по бюджету цикла) находка — не продуктовая, а процессная: новый top-level каталог support-relay/ создаёт слепую зону в автоматическом гейте process-gate.mjs, причём именно для той части системы, где цена незамеченного нарушения правила «код только через issue» выше всего. Это Medium-находка в скоупе задачи (сам каталог заводит #43), правится одной явной строкой в ТЗ — второй раунд не должен требовать нового расследования.