docs: review document for #43

Issue: #43
User-Visible: no

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

@@ -0,0 +1,210 @@
+# SPEC-REVIEW-43-r4
+
+- Issue: #43 «Диалог помощи и обратной связи с обезличенным support report»
+- Этап: spec (PROCESS.md §2.4)
+- Заход: r4 · блокирующих циклов израсходовано 2 из 4 (до этого раунда)
+- Материал: `docs/specs/043-private-support-report.md` на HEAD `cd8d01ac`
+  (ветка `issue/43-help-feedback`), плюс код `scripts/support-relay/**` как
+  доказательство исполнимости заявленных в §9 контрактов — в объёме, до
+  которого дотягивается дельта этого раунда.
+- Предыдущий раунд: r3, вердикт **жёлтый**, SHA `c43bf847` (High 0, Medium 1);
+  документ `docs/reviews/SPEC-REVIEW-43-r3.md`, опубликован коммитом
+  `55b51a02`. SHA r3 в самом документе назван явно (раздел «Материал»); в
+  сжатом комментарии-вердикте в issue он не повторён (там только «HEAD»), но
+  это не «находка №1 инструкции» — первоисточник (файл ревью) SHA не потерял.
+  SHA пересчитан и подтверждён по времени коммитов независимо от текста
+  документа: `git log --date=iso-strict` даёт `c43bf847` = `2026-09-02
+  01:03:57+03:00`, что на 27 с опережает комментарий-вердикт `r3`
+  (`22:21:03Z` = `01:21:03+03:00`, следующий коммит `55b51a02` = сам документ
+  ревью, коммит `22:21:09Z`).
+
+## Дельта r3→r4
+
+`git diff c43bf847..HEAD` (без учёта `docs/reviews/SPEC-REVIEW-43-r3.md`,
+который сам является артефактом прошлого раунда, а не правкой автора):
+
+- `docs/specs/043-private-support-report.md` — 10 строк добавлено, 0 удалено:
+  новый абзац в §9.2 (источник rate-limit), новая строка **AC12a** в §13,
+  новая строка обязательной мутации в §14.4;
+- `scripts/support-relay/README.md` — 27 строк добавлено: минимальная
+  конфигурация автоматизации Home Assistant «House Plan: приёмщик обратной
+  связи → личка» (YAML) плюс пояснение, почему `parse_mode: plain_text` не
+  косметика.
+
+Дельта строго докс-онли и локальна: адресует ровно два пункта прошлого
+вердикта (Medium §9.2 и снятый-с-запиской Low про недокументированную
+автоматизацию), кода не трогает, новую подсистему не задевает, объём
+несравним с исходной задачей. Полный разбор не требуется — разбор по дельте
+плюс всё, до чего эта дельта дотягивается (см. находку ниже: она лежит внутри
+того же абзаца §9.2, который дельта редактирует).
+
+## Закрытие раунда r3
+
+| Находка r3 | Чем закрыта | Где видно |
+|---|---|---|
+| **Medium** — §9.2 не фиксирует источник rate-limit-адреса (только код/README, требование не переживёт рефакторинг `hp_relay/app.py`) | Частично: добавлен абзац в §9.2 «источник — последний элемент `X-Forwarded-For`, потому что прокси перезаписывает заголовок целиком», плюс **AC12a** в §13 и строка мутации в §14.4 | `docs/specs/043-private-support-report.md:446-452` (абзац §9.2), `:539` (AC12a), `:590-591` (§14.4). Текст дословно соответствует тому, что просил r3, **кроме** условности через `trusted_proxy` — см. новую находку ниже, это тот же абзац, не новый пробел, а недозакрытый прежний |
+| **Low** (снят с запиской, не блокировал) — минимальная конфигурация вебхук-автоматизации не задокументирована в репозитории | Закрыт: полный YAML автоматизации (webhook-триггер, `local_only: false`, условие по `source`, `telegram_bot.send_message` с `parse_mode: plain_text`) добавлен в README | `scripts/support-relay/README.md:154-180`. Поля `source`/`text` в шаблоне совпадают буквально с тем, что шлёт код: `scripts/support-relay/hp_relay/delivery.py:180-184` (`HaWebhookDelivery.send`) формирует JSON именно с ключами `source`/`report_id`/`text` |
+
+Low закрыт полностью и без оговорок. Medium закрыт **не полностью** — см. находку.
+
+## Унаследовано из r3 (и через r3 — из r2/r1) без повторной проверки
+
+Со ссылкой на `docs/reviews/SPEC-REVIEW-43-r3.md` (SHA `c43bf847`) и
+транзитивно на r2 (`docs/reviews/SPEC-REVIEW-43-r2.md`, SHA `e25aa302`):
+
+- §1–§8 — сценарий, UX-контракт, support package v1, backend API preview/
+  discard/submit — дельта их не касается;
+- §9.1 — два канала доставки (`telegram`/`ha_webhook`), запись до попытки
+  доставки, privacy-текст §9.3 (кроме уже переоценённого абзаца §9.2) — не
+  менялись этой дельтой, r3 проверил их построчно против кода
+  (`hp_relay/delivery.py`, `app.py:107-116`, `config.py:66-68`);
+- §10, §11 (включая каноническую фразу `Touch editor: supported`), §12;
+- §13 AC1–AC11, AC13–AC17 (кроме новой AC12a);
+- §15–§20, включая классификацию `scripts/support-relay/**` как class B
+  (закрыта в r1→r2, переподтверждена в r3 живым прогоном `process-gate.mjs`);
+- полный аудит `hp_relay/{multipart,validate}.py` — вне дельты r2→r3 и вне
+  дельты r3→r4, предмет будущего код-ревью по AC5–AC12.
+
+## Находки
+
+### Medium (в скоупе #43, чинится в этой же задаче, вопрос технический — не владельцу)
+
+**§9.2 закрывает находку r3 частично: описывает доверие к `X-Forwarded-For`
+как безусловный архитектурный факт, а оно на самом деле включается флагом
+конфигурации `HP_RELAY_TRUSTED_PROXY`, который в спеке, AC и списке
+обязательных мутаций не упомянут вовсе.**
+
+Читаю код (`scripts/support-relay/hp_relay/app.py:147-158`):
+
+```python
+def _source(self) -> str:
+    if service.cfg.trusted_proxy:
+        forwarded = self.headers.get("X-Forwarded-For", "")
+        if forwarded:
+            return forwarded.split(",")[-1].strip()
+    return self.client_address[0]
+```
+
+`trusted_proxy` берётся из `HP_RELAY_TRUSTED_PROXY` (`config.py:82`, default
+`"1"`) — то есть чтение последнего элемента `X-Forwarded-For` действует только
+при этом флаге; иначе источником становится адрес TCP-соединения, то есть
+(при реальной топологии, где Caddy проксирует на `127.0.0.1`) **один и тот же
+адрес для всех клиентов**, что не «безопаснее», а means-один общий
+частотный бакет на весь публичный эндпоинт: одна активная попытка исчерпывает
+лимит для всех источников разом, а не для конкретного клиента, — деградация
+той же анти-abuse истории (§16, риск 3), только в обратную сторону.
+
+Новый абзац §9.2 (добавленный этой дельтой) не называет этот флаг вообще:
+
+> «The relay reads the *last* element of `X-Forwarded-For`, because the first
+> element is whatever the caller sent, and the reverse proxy in front of it is
+> configured to overwrite the header outright rather than append to it.»
+
+Это верно только пока `trusted_proxy=true`; сам разговор о существовании
+такого выключателя, о том, что оба продовых хоста (`support.houseplan.tech`,
+`support-staging.houseplan.tech`) обязаны держать его включённым, и о
+поведении при выключенном — в спеке отсутствует.
+
+**Это ровно тот класс дефекта, который сама находка r3 была призвана
+устранить**: в коде — правильно и обдуманно (для флага есть отдельное имя,
+дефолт и комментарий в `env.example:18-19`), а в ТЗ — не зафиксировано, значит
+не переживёт следующий рефакторинг `hp_relay/app.py` (случайное удаление
+`if service.cfg.trusted_proxy:` при сохранении `.split(",")[-1]` тихо
+расширит доверие к заголовку на любую топологию, включая прямую экспозицию
+порта без прокси). Ни `AC12a`, ни новая строка §14.4 эту ветку не покрывают:
+обе называют только подмену индекса элемента (`[0]` вместо `[-1]`), а не
+факт, что chтение заголовка вообще управляется флагом.
+
+Проверено также, что тестового покрытия ветки `trusted_proxy=False` нет:
+`grep -n "trusted_proxy" scripts/support-relay/tests/test_relay.py` — 0
+совпадений. Ветка «адрес TCP-соединения» сейчас не проверяется ни одним
+тестом.
+
+**Чем закрыть, не открывая вопрос владельцу** (технический пункт по §7.1,
+решает автор ТЗ, ревьюер вправе оспорить):
+
+1. в §9.2 назвать флаг явно: доверие к `X-Forwarded-For` действует только при
+   `HP_RELAY_TRUSTED_PROXY=1` (`trusted_proxy` в конфиге), и это обязательное
+   состояние для `prod`/`staging` — при выключенном или отсутствующем прокси
+   источником становится адрес TCP-соединения, и это осознанно более
+   консервативный, а не эквивалентный режим (одна корзина на весь трафик, а
+   не «без лимита»);
+2. AC12a — добавить условие: гарантия действует только при `trusted_proxy`,
+   включённом на обоих продовых хостах; это часть DoR/release, а не просто
+   поведение по умолчанию;
+3. §14.4 — добавить мутацию: снятие проверки `if service.cfg.trusted_proxy`
+   (доверие XFF независимо от флага) должно ронять новый тест, которого
+   сейчас нет — то есть пункт 3 требует и код (тест), не только текст ТЗ;
+   зафиксировать это явно, а не оставлять как молчаливый пробел.
+
+Серьёзность — Medium: не открывает утечку данных и не позволяет обойти лимит
+сильнее, чем позволяет топология (пока `trusted_proxy` включён по умолчанию
+и прокси настроен верно — как сейчас и есть), но ровно то самое несоответствие
+кода и ТЗ, которое r3 уже один раз квалифицировал как Medium в этом же
+абзаце. Не блокирует по High, но без исправления это жёлтый вердикт.
+
+## Что проверено и признано корректным
+
+- **Low r3 закрыт полностью**: YAML-шаблон автоматизации в README дословно
+  совпадает с полями, которые шлёт `HaWebhookDelivery.send`
+  (`source`/`report_id`/`text`), включая объяснение, почему `parse_mode:
+  plain_text` обязателен (иначе текст пользователя разбирается как разметка).
+- **Caddy-конфигурация** (`scripts/support-relay/deploy/Caddyfile.fragment`)
+  подтверждена чтением: `header_up X-Forwarded-For {remote_host}` стоит на
+  **обоих** сайтах (`support.houseplan.tech`, `support-staging.houseplan.tech`)
+  без исключений — то есть при нынешнем деплое реальная топология
+  соответствует тому, что описывает новый абзац §9.2 (с оговоркой из находки
+  выше: соответствие держится на конфиге, а не на архитектурной гарантии).
+- **AC12a и новая строка §14.4** внутренне непротиворечивы и совпадают с уже
+  существующим (написанным до r3) тестом `test_client_cannot_pick_its_own_rate_bucket`
+  (`scripts/support-relay/tests/test_relay.py:452-467`) — читал тест, он
+  действительно шлёт три поддельных `X-Forwarded-For` и проверяет один общий
+  ключ корзины в `spool/rate/*.json`.
+- **Классификация `scripts/support-relay/**` class B** не затронута дельтой,
+  живой прогон `node scripts/process-gate.mjs` на HEAD `cd8d01ac`: «гейт
+  пройден, предупреждений 0» (офлайн, диапазон `origin/dev..HEAD`, 10
+  коммитов).
+- Коммит `cd8d01ac` несёт `Issue: #43` и `User-Visible: no` (docs-only,
+  корректно — правки в спеку и README не меняют видимое поведение продукта).
+
+## Чего не проверял и почему
+
+- `npm test` / `npm run build` (сверка трёх копий бандла) — не прогонял сам:
+  Validate на этом же SHA `cd8d01ac` завершился success (ссылка дана в
+  задании), покрывает `tsc`/`test`/`build`; дельта раунда и так не трогает
+  `src/**`, предмета для этих гейтов у неё нет.
+- `node scripts/check-docs.mjs` — не требовался: дельта не трогает `src/**`.
+- `npm run invariants` / инварианты модели — не требовались: дельта не
+  трогает геометрию, `layout`, `marker.space`, `open_spans`.
+- `python -m pytest tests_backend -q` — не требовался: дельта не трогает
+  `custom_components/**`.
+- `python3 -m unittest discover -s scripts/support-relay/tests` — **не
+  перегонял сам в этом раунде**: код relay этой дельтой не менялся (только
+  документация), а r3 уже прогнал полный набор (35/35 зелёных) и лично
+  проверил мутациями оба относящихся к этому раунду теста
+  (`test_client_cannot_pick_its_own_rate_bucket`,
+  `test_webhook_sends_text_and_keeps_the_package_on_the_node`) на способность
+  падать — наследую этот результат из r3 (см. таблицу наследования). Находка
+  выше — про то, чего в этом наборе тестов нет (`trusted_proxy=False`), а не
+  про то, что существующие тесты красные.
+- Браузерные смоки, `npm run golden:verify` — не требовались: `src/**` для
+  фичи #43 ещё не реализован (§1–§8 остаются на этапе ТЗ).
+- Полный построчный аудит `hp_relay/{multipart,validate}.py` — вне дельты
+  r3→r4, предмет будущего код-ревью по AC5–AC12.
+
+## Гейты, которые прогнал сам
+
+| Гейт | Результат |
+|---|---|
+| `node scripts/process-gate.mjs` | «гейт пройден, предупреждений 0» (офлайн, `origin/dev..HEAD`, 10 коммитов) |
+| Чтение `hp_relay/app.py`, `config.py`, `env.example`, `test_relay.py`, `delivery.py`, `Caddyfile.fragment` | вручную, построчно — источник находки и подтверждения закрытий |
+| `npx tsc --noEmit` / `npm test` / `npm run build` | не прогонял — Validate зелёный на этом же SHA `cd8d01ac` (см. задание раунда), дельта докс-онли |
+
+## Вывод
+
+Low r3 закрыт полностью. Medium r3 закрыт частично — тот же абзац §9.2
+получил формулировку, которая верна только при включённом (по умолчанию, но
+не зафиксированном как обязательное условие) флаге `trusted_proxy`, и это
+условие не отражено ни в AC12a, ни в §14.4, ни где-либо ещё в ТЗ. High
+находок нет. Вердикт — жёлтый, не блокирующий продвижение по High, но
+возвращающий ТЗ на правку одного абзаца и одной AC-строки.