diff --git a/docs/reviews/SPEC-REVIEW-43-r4.md b/docs/reviews/SPEC-REVIEW-43-r4.md new file mode 100644 index 00000000..02ac3236 --- /dev/null +++ b/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-строки.