docs: review document for #43

Issue: #43
User-Visible: no

docs/reviews/SPEC-REVIEW-43-r3.md

@@ -0,0 +1,205 @@
+# SPEC-REVIEW-43-r3
+
+- Issue: #43 «Диалог помощи и обратной связи с обезличенным support report»
+- Этап: spec (PROCESS.md §2.4)
+- Заход: r3 · блокирующих циклов израсходовано 2 из 4 (после этого раунда)
+- Материал: `docs/specs/043-private-support-report.md` на HEAD `c43bf847`
+  (ветка `issue/43-help-feedback`), плюс код `scripts/support-relay/**` как
+  доказательство исполнимости заявленных в ТЗ §9 контрактов.
+- Предыдущий раунд: r2, вердикт **зелёный**, SHA `e25aa302` (High 0, Medium 0);
+  документ `docs/reviews/SPEC-REVIEW-43-r2.md`, опубликован коммитом `cf7f47db`.
+
+## Почему разбор не сведён к «только дельта»
+
+Между r2 и r3 в issue появилась реальная production-инфраструктура: relay
+поднят на боевом узле, владелец лично проверил rate limiting и попытку подмены
+источника через `X-Forwarded-For`, обнаружил, что прямой Telegram недоступен с
+хостинга проекта, и по итогу сменил канал доставки «последней мили» на приватный
+вебхук собственного Home Assistant. Это смена контракта в подсистеме, которую
+r1/r2 уже проверяли (§9 relay), а не локальная правка формулировки — поэтому
+разбор §9 и связанных разделов (§14.3/§14.4/§16/§17) сделан заново целиком, а не
+только «что изменилось в тексте».
+
+## Дельта r2→r3
+
+`git diff e25aa302..HEAD`:
+
+- `cf7f47db` — публикация `docs/reviews/SPEC-REVIEW-43-r2.md` (только review-артефакт, не часть ТЗ);
+- `00102819 feat(relay): receive support reports on the project stand` — первая реализация `scripts/support-relay/**` (класс B), 1593 строки;
+- `6b10cd52 docs: switch the support sink to a maintainer channel` — правка §5.1/§9.1/§9.3/§12/AC12/§14.3/§16/§17/§18/§19/§20 ТЗ: mailbox → приватный канал мейнтейнера;
+- `c7efcf16 fix(relay): pin the rate-limit source to the proxy-supplied address` — код и тест против подмены источника лимита через `X-Forwarded-For`, найденной живым пентестом на стенде; **ТЗ этим коммитом не тронуто**;
+- `c43bf847 feat(relay): deliver through the maintainer's Home Assistant webhook` — второй канал доставки `ha_webhook`, правка §9.1/§9.3 ТЗ.
+
+Все четыре продуктовых коммита несут `Issue: #43` и `User-Visible: no` —
+консистентно с тем, что видимое поведение карточки (`src/**`) ещё не начато,
+только class-B relay.
+
+## Закрытие раунда r2
+
+r2 закрылся с 0 находок (High 0, Medium 0) — закрывать в r3 нечего, таблица
+пустая. Единственное, что произошло после r2, — новая работа поверх зелёного
+вердикта, а не исправление старых замечаний.
+
+## Унаследовано из r2 (и из r1 через r2)
+
+Без повторной проверки приняты — как есть в `docs/reviews/SPEC-REVIEW-43-r2.md`
+на SHA `e25aa302` — потому что дельта r2→r3 не касается доказательной базы
+этих пунктов:
+
+- §1–§6 (сценарий, UX-контракт кнопки/диалога/формы/preview/submit) — текст не
+  менялся в дельте;
+- §7 (allowlist/pseudonymization/privacy invariant support package v1) — не
+  менялся;
+- §8 (backend API preview/discard/submit) — не менялся;
+- §10 (state/compatibility), §11 (touch/a11y — включая каноническую фразу
+  `Touch editor: supported`, закрытую в r1→r2), §13 AC1–AC11, AC13–AC17
+  (кроме евиденс-колонки AC12, см. находку ниже);
+- классификация `scripts/support-relay/**` как class B по `AGENTS.md`/
+  `PROCESS.md` (закрытая в r1→r2 находка) — подтверждена и в r3 живым прогоном
+  `process-gate.mjs` на новом HEAD (см. «Гейты»), т.к. дерево `scripts/`
+  выросло, но regex-классификация в `process-gate.mjs` не менялась.
+
+## Находки
+
+### Medium (в скоупе #43, чинится этим же ТЗ, без нового цикла к владельцу — вопрос чисто технический)
+
+**Требование к источнику rate-limit не зафиксировано в ТЗ, хотя код и тесты
+уже реализуют его правильно.**
+
+§9.2 ТЗ (не тронут дельтой) по-прежнему говорит только:
+
+> 5 attempts/hour and 20/day per source address plus a global circuit breaker;
+> source IP is used only through a daily-keyed rate-limit hash with ≤24 h TTL
+
+Это не говорит, ОТКУДА берётся «source address», хотя корректность всей
+анти-abuse истории публичного эндпоинта (§16, риск 3 «Public relay attracts
+spam») зависит именно от этого. На реальном стенде это оказалось не
+теоретическим риском: согласно комментарию владельца от 2026-09-01 21:25,
+подмена `X-Forwarded-For` почти позволила обойти лимит одной строкой запроса —
+спасло только совпадение с дефолтом чужого Caddy-конфига. Исправление внесено
+кодом (`c7efcf16`) и покрыто тестом, но **в ТЗ это исправление не попало**:
+
+- `docs/specs/043-private-support-report.md` §9.2 не требует, что источник
+  берётся из доверенного hop-а прокси, а не из клиентского заголовка;
+- AC12 (§13) по-прежнему говорит только «Relay enforces schema, size, hash,
+  idempotency and rate limits» — не называет спуфинг-стойкость как часть
+  контракта;
+- §14.3 (test plan) и особенно §14.4 (mutation requirements) — единственный
+  раздел ТЗ, который явно перечисляет КАЖДУЮ обязательную мутацию для security-
+  инвариантов (checkbox default, redirect/SSRF, forbidden field leak и т.д.) —
+  не содержит пункта «клиент подделывает X-Forwarded-For и выбирает себе новую
+  корзину лимита → тест красный», хотя именно такой тест уже существует в коде
+  (`test_client_cannot_pick_its_own_rate_bucket`,
+  `scripts/support-relay/tests/test_relay.py:454-470`) и хотя именно этот
+  сценарий уже был проверен на боевом трафике.
+
+Реализация де-факто правильная (проверено чтением и исполнением, см. «Гейты»):
+`hp_relay/app.py:147-158` берёт последний элемент `X-Forwarded-For` только при
+`cfg.trusted_proxy` (default `HP_RELAY_TRUSTED_PROXY=1`,
+`hp_relay/config.py:41,82`), а `deploy/Caddyfile.fragment:12-14` полностью
+перезаписывает заголовок на обоих сайтах (`header_up X-Forwarded-For
+{remote_host}`), а не дополняет его. Но эта перепись — рассыпанные по трём
+файлам факты (`app.py`, `Caddyfile.fragment`, `env.example`), а не пункт
+контракта ТЗ. Проблема не «оно не работает сейчас», а «ничто в ТЗ не обязывает
+это работать после следующего рефакторинга»: спека — это то, с чем сверяется
+будущий код-ревью и будущий переписчик `hp_relay/app.py`, а не README
+деплоя.
+
+**Чем закрыть, не открывая вопрос владельцу** (технический пункт, решается
+автором ТЗ по §7.1, не владельцем):
+
+1. в §9.2 добавить явное предложение: источник для rate-limit — последний
+   элемент `X-Forwarded-For`, только если relay сконфигурирован работать за
+   доверенным прокси (`trusted_proxy`), который обязан перезаписывать этот
+   заголовок целиком, а не дополнять; без этой конфигурации — адрес
+   TCP-соединения;
+2. AC12 — добавить пункт: «источник для лимита не может быть выбран клиентом
+   через заголовки»;
+3. §14.4 — добавить строку: «клиент посылает поддельный `X-Forwarded-For` →
+   получает собственную корзину лимита → тест красный».
+
+### Low (не блокирует, можно снять с записью)
+
+Продакшн-канал `ha_webhook` целиком зависит от одной автоматизации на личном
+Home Assistant мейнтейнера («House Plan: приёмщик обратной связи → личка»,
+`scripts/support-relay/README.md:151-152`). Ротация вебхука прямо требует
+правки этой автоматизации, но её конфигурация не задокументирована и не
+сохранена нигде в репозитории — только упоминание, что она существует. Отказ
+этого узла не теряет данные и не обманывает пользователя (отчёт остаётся в
+спуле, клиент честно получает `support_unavailable` — проверено чтением
+`hp_relay/app.py:110-116`), поэтому это не приватность/безопасность и не
+блокирует зелёный; но при потере доступа к личному HA мейнтейнера
+восстановление доставки требует пересоздания автоматизации «с нуля» без
+письменной инструкции. Стоит одной строкой в `scripts/support-relay/README.md`
+зафиксировать минимальную конфигурацию вебхука (принимает POST, поле `text`,
+пересылает в Telegram), не дожидаясь следующего цикла.
+
+## Что проверено и признано корректным
+
+- **Два канала доставки (`telegram`/`ha_webhook`) в §9.1** описаны в ТЗ
+  консистентно с кодом: `hp_relay/delivery.py:141-203` (`TelegramDelivery`,
+  `HaWebhookDelivery`, `build()`) реализует ровно то, что описано —
+  вложение остаётся в спуле и не покидает узел на канале `ha_webhook`
+  (`hp_relay/delivery.py:168-188`, тест
+  `test_webhook_sends_text_and_keeps_the_package_on_the_node`).
+- **«Запись до попытки доставки» (§9.1, «a failed delivery costs a promise, not
+  the user's request»)** — подтверждено чтением: `app.py:107-116` вызывает
+  `store.save()` до `delivery.send()`, а при неуспехе возвращает
+  `support_unavailable`, не удаляя отчёт.
+- **Privacy-текст §9.3** («exact geometry ... transit the project relay and the
+  maintainer messenger») намеренно описывает худший случай для обоих каналов,
+  а не конкретно активный канал: фронтенд не знает, какой канал выбран на
+  деплое (это решение эксплуатации, §9.1 «chosen by deployment»), поэтому
+  общая (не заниженная) формулировка — осознанный выбор, а не забытая правка.
+  Проверено сопоставлением с `hp_relay/config.py:66-68` (канал — переменная
+  окружения, недоступная фронтенду).
+- **§17 DoR** обновлён согласованно с новым каналом (пункт 2 — путь к секрету
+  вебхука как учётные данные), пункты 1/3/4/5 отражены в отдельных комментариях
+  владельца как реально выполненные на стенде — вне объёма самого текста ТЗ,
+  но не противоречат ему.
+- **Лимиты §7.5/§9.5** (8 MiB / 8.5 MiB) совпадают с кодом:
+  `hp_relay/config.py:15-16` (`MAX_REQUEST_BYTES`, `MAX_ATTACHMENT_BYTES`).
+- **Классификация class B** (`scripts/support-relay/**`) остаётся верной после
+  роста дерева — `process-gate.mjs` не меняла regex, живой прогон на HEAD это
+  подтверждает.
+- **SCOPE.md** проверен на предмет открытых вопросов дельты: раздел «Where
+  users are» фиксирует уже существующий *публичный* support-чат
+  `t.me/ha_houseplan` как канал общей обратной связи — пивот к *приватному*
+  каналу мейнтейнера в r3 ему не противоречит и не подменяет его, они решают
+  разные задачи (публичный сигнал vs. приватный geometry-репорт). Собственно
+  вопрос «попадает ли фича в Core user jobs» дельтой r2→r3 не поднимается
+  (§1–§6 не менялись) и наследуется как принятый в r1/r2.
+
+## Чего не проверял и почему
+
+- `npm test` / `npm run build` (сверка трёх копий бандла) / `node
+  scripts/check-docs.mjs` — дельта не касается `src/**`, предмета проверки нет.
+- `python -m pytest tests_backend -q` — дельта не касается
+  `custom_components/**` (relay — отдельный сервис вне HA-интеграции).
+- `npm run invariants` / инварианты модели — дельта не касается геометрии,
+  `layout`, `marker.space`, `open_spans` или толщины стен.
+- Браузерные смоки, `npm run golden:verify` — фронтенд-часть фичи (§1–§8) ещё
+  не реализована, `src/**` не тронут.
+- Полный аудит `scripts/support-relay/hp_relay/{multipart,validate}.py` — эти
+  файлы не входят в дельту r2→r3 (не менялись коммитами `6b10cd52`/`c7efcf16`/
+  `c43bf847` за пределами уже описанного), их корректность — предмет будущего
+  code-review по §13 AC5–AC12, а не этого spec-раунда.
+
+## Гейты, которые прогнал сам (зелёного Validate на `c43bf847` нет)
+
+- `node scripts/process-gate.mjs` → «гейт пройден, предупреждений 0» (диапазон
+  `origin/dev..HEAD`, 8 коммитов, офлайн).
+- `npx tsc --noEmit` → 0 ошибок (дельта не трогает `src/**`, прогнан как
+  дешёвая проверка базовой линии).
+- `python3 -m unittest discover -s scripts/support-relay/tests` → **35
+  проверок, все зелёные**, включая `test_client_cannot_pick_its_own_rate_bucket`
+  и `test_webhook_sends_text_and_keeps_the_package_on_the_node`.
+- Дисциплина «тест должен уметь падать» проверена для обоих названных выше
+  тестов лично, мутациями (изменения отменены после проверки, `git status`
+  чист):
+  - `forwarded.split(",")[-1]` → `[0]` (доверие первому, клиентскому элементу
+    XFF вместо последнего, проксёй-контролируемого) — тест
+    `test_client_cannot_pick_its_own_rate_bucket` упал (3 корзины вместо 1);
+  - `HaWebhookDelivery.send` начинает прикладывать байты вложения в JSON
+    вебхука — тест `test_webhook_sends_text_and_keeps_the_package_on_the_node`
+    упал (лишнее поле `attachment` в теле).