diff --git a/docs/reviews/SPEC-REVIEW-43-r1.md b/docs/reviews/SPEC-REVIEW-43-r1.md new file mode 100644 index 00000000..5604f7ca --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-43-r1.md @@ -0,0 +1,228 @@ +# 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_`), + `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), правится одной +явной строкой в ТЗ — второй раунд не должен требовать нового расследования.