diff --git a/docs/reviews/SPEC-REVIEW-43-r5.md b/docs/reviews/SPEC-REVIEW-43-r5.md new file mode 100644 index 00000000..55709fb6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-43-r5.md @@ -0,0 +1,197 @@ +# SPEC-REVIEW-43-r5 + +- Issue: #43 — «Диалог помощи и обратной связи с обезличенным support report» +- Этап: ТЗ на ревью (PROCESS.md §2.4), полный трек (issue не помечен `small`) +- Заход: r5 · блокирующих циклов израсходовано 3 из 4 до этого раунда +- Материал: `docs/specs/043-private-support-report.md`, `scripts/support-relay/README.md`, + `scripts/support-relay/tests/test_relay.py` на ветке `issue/43-help-feedback` +- SHA материала r4 (названо в самом вердикте r4): `cd8d01ac` +- SHA материала этого раунда (`git rev-parse HEAD` непосредственно перед выводом): `3ce5bc0ea2bab55c7c586d63b78a76459fd5c4d2` +- Предыдущий документ: `docs/reviews/SPEC-REVIEW-43-r4.md` + +## Скоуп разбора + +Раунд r4 закончился жёлтым вердиктом с единственной находкой уровня Medium (в +скоупе, техническая): §9.2/AC12a/§14.4 описывали чтение `X-Forwarded-For` как +безусловный факт, хотя в коде это работает только под флагом +`HP_RELAY_TRUSTED_PROXY` (default on), а ветка `trusted_proxy=False` не имела +покрытия тестами. + +Дельта `cd8d01ac..3ce5bc0e` строго докс- и тест-only: + +``` +docs/reviews/SPEC-REVIEW-43-r4.md | 210 +++++++++++++ +docs/specs/043-private-support-report.md | 21 +- +scripts/support-relay/README.md | 16 +- +scripts/support-relay/tests/test_relay.py | 51 ++++ +``` + +(первый файл — публикация артефакта r4 предыдущим прогоном конвейера, не +предмет этого разбора). Кода продукта (`src/**`, `custom_components/**`) дельта +не касается, новая подсистема не задета, объём несравним с задачей — это +классический случай «предмет раунда — дельта», разбор по ней, а не заново. + +## Как проверялось + +1. Прочитан полный текст `docs/specs/043-private-support-report.md` заново + (не только изменённые абзацы) — чтобы дельта на 21 строку не спрятала + контекстную рассинхронизацию с остальным документом. +2. Сверено текстовое утверждение §9.2 с фактическим кодом: + `scripts/support-relay/hp_relay/app.py:147-158` (`_source()`) и + `scripts/support-relay/hp_relay/config.py:82` (`trusted_proxy` default) — + поведение обеих позиций переключателя дословно совпадает с новым текстом + ТЗ. +3. Прогнан полный набор relay: `cd scripts/support-relay && python3 -m + unittest discover -s tests -q` → **36 проверок, все зелёные**. +4. Дисциплина «тест умеет падать» применена к новому тесту + `test_direct_node_ignores_the_forwarded_header` лично, а не со слов автора: + в `hp_relay/app.py::_source()` заменено `if service.cfg.trusted_proxy:` на + `if True:` (эмуляция мутации «переключатель проигнорирован») — + `python3 -m unittest tests.test_relay.DirectNodeTestCase -v` упал: + `AssertionError: 3 != 1` (три разных ключа источника вместо одного). Файл + восстановлен из копии, `git diff --stat hp_relay/app.py` после отката — + пусто, повторный полный прогон — снова 36/36. +5. Проверено, что существующий тест `test_client_cannot_pick_its_own_rate_bucket` + (позиция переключателя по умолчанию, `trusted_proxy=True`) не тронут и + продолжает покрывать вторую половину AC12a. +6. `node scripts/process-gate.mjs` — офлайн-прогон на HEAD, диапазон + `origin/dev..HEAD`, 12 коммитов: «гейт пройден, предупреждений 0». +7. Проверены трейлеры обоих коммитов дельты (`git log --format='%B' + cd8d01ac..HEAD`): у обоих `Issue: #43` и `User-Visible: no` — верно, дельта + не меняет пользовательское поведение. +8. Прочитана вся история issue #43 (18 комментариев) для контекста: подтверждено, + что все пять внешних DoR-зависимостей §17 закрыты предыдущими комментариями + владельца (deployment target, канал доставки/credentials, 30-дневное + удаление, staging для CI, ответственный), последний — «`blocked` можно + снимать: внешних зависимостей у реализации больше нет» (2026-09-01 22:04). + Это не переоценивается заново в этом раунде — оно не относится к предмету + дельты (флаг доверенного прокси) и не изменилось между r4 и r5. + +## Закрытие раунда r4 + +| Находка r4 | Чем закрыта | Где видно | +|---|---|---| +| **Medium.** §9.2 описывает чтение `X-Forwarded-For` как безусловный факт, хотя в коде это работает только под флагом `HP_RELAY_TRUSTED_PROXY` (default on); AC12a и §14.4 не называют флаг; нет теста на позицию `trusted_proxy=False`. | §9.2 переписан: флаг назван по имени, обе позиции описаны явно («включён» / «выключен»), добавлено требование «за прокси выключать запрещено» с обоснованием цены (весь публичный эндпоинт делил бы одну корзину). AC12a переписан под обе позиции с указанием двух тестов. §14.4 получил строку про мутацию «читать заголовок независимо от переключателя». Написан и подтверждён тест `test_direct_node_ignores_the_forwarded_header` (не только обещан в тексте — прогнан и лично провален мутацией). | `docs/specs/043-private-support-report.md` §9.2 (абзац «Behind a reverse proxy…»), §13 AC12a, §14.4; `scripts/support-relay/tests/test_relay.py:312-361` (`DirectNodeTestCase`); код без изменений — `hp_relay/app.py:147-158` уже соответствовал описанию. | + +Закрыто полностью, без остатка: расхождение между текстом ТЗ и кодом, из-за +которого требование «не пережило бы рефакторинг `app.py`» (как выразился автор +в комментарии к r4), теперь закреплено и текстом, и работающим тестом, +способным упасть. + +## Унаследовано из r4 (и через r4 из более ранних раундов) + +Со ссылкой на `docs/reviews/SPEC-REVIEW-43-r4.md`, SHA `cd8d01ac`, без повторной +проверки в этом раунде — дельта их не касается: + +- §1–8 (сценарий, персона, скоуп, решения владельца, UX-контракт кнопки/диалога/ + формы/preview/submit) — полностью проверены в r1 построчно против кода + (`src/houseplan-card.ts`, `src/houseplan-editor-runtime.ts`), включая + соответствие пяти утверждений §3 фактическому состоянию; +- §7 (envelope, allowlist, псевдонимизация, privacy invariant, лимиты) — + проверен в r1 на внутреннюю непротиворечивость allowlist/exclusion списков; +- §8 (backend API preview/submit) — контракт «просмотренные байты = отправленные + байты» (кешированный TTL-токен + mutation-gate) проверен в r1 как спроектированный + в контракт, а не оставленный реализации; +- §9.1, §9.3 (архитектура relay, каналы доставки Telegram/`ha_webhook`, ретеншн) — + полностью разобраны заново в r3 после смены продакшн-канала на боевом стенде + (это была не локальная правка, а смена контракта поведения, разбор шёл целиком, + не по дельте); r4 унаследовал их без изменений, этот раунд — тоже, дельта их + не трогает; +- §10–13 (кроме AC12a), §15–20 (state/compat, touch/a11y §11 с канонической + фразой «Touch editor: supported», i18n, AC1–AC17 кроме AC12a, test plan §14.1–14.3, + performance-бюджеты, риски, §17 DoR-зависимости, §18 release-артефакты, §19 + rollback, §20 assumed-блок) — проверены в r1–r3, последняя правка (r4) их не + касалась, эта (r5) — тоже; +- классификация `scripts/support-relay/**` как класс B (`AGENTS.md`/`PROCESS.md`, + `scripts/process-gate.mjs::classify()` regex `/^scripts\//`) — закрыта в r2 + чтением regex и живым прогоном гейта, не пересматривалась; +- фактическое закрытие всех пяти пунктов §17 DoR (deployment target, канал + доставки, 30-дневный ретеншн, staging, ответственный) — установлено серией + комментариев владельца в issue между r2 и r3 с воспроизведёнными командами + (`curl .../health`, systemd-таймеры, README) и последним подтверждением + «внешних зависимостей больше нет»; это не предмет спек-ревью ТЗ как текста + (ТЗ корректно описывает эти зависимости как внешние в §17), а операционный + факт вне дельты — принят как есть. + +## Находки + +**Low, снимается решением ревьюера с записью, цикл не расходует.** +`scripts/support-relay/README.md` (раздел «Тесты»): текст утверждает +«четырнадцать мутаций рабочего кода» и затем перечисляет их в скобках — +пересчёт даёт **тринадцать** пунктов (снять сверку хеша · разрешить лишнюю +часть · не чистить управляющие символы · снять лимит · писать адрес в журнал · +игнорировать идемпотентность · отключить рубильник · отключить ретеншн · не +проверять секции пакета · брать первый элемент `X-Forwarded-For` · подменить +`source` в вебхуке · приложить пакет к вебхуку · читать заголовок независимо от +переключателя — 13). Расхождение уже было в r4-версии текста (там «тринадцать» +против фактических двенадцати пунктов списка) и в этом раунде перенесено на +единицу дальше вместе с добавлением тринадцатого пункта. Не блокирует: это +описательное число в README для человека, читающего перед раскруткой relay, оно +не служит доказательством ни одного AC (доказательство — сам прогон +`unittest discover`, который лично прогнан и даёт 36/36) и не является тем +«одно число — один источник» пользовательским значением, о котором +предупреждает §8 PROCESS.md — это внутренний devops-README, а не +пользовательский интерфейс. Снимаю без возврата автору; при следующей правке +этого README посчитать пункты заново. + +Других находок нет: High — 0, Medium — 0. + +## Что проверено и корректно + +- Текст §9.2/AC12a/§14.4 после правки дословно соответствует поведению кода + (`_source()` в `app.py`, default в `config.py`) для обеих позиций + переключателя; +- Новый тест `DirectNodeTestCase.test_direct_node_ignores_the_forwarded_header` + реально существует, реально прогоняется, реально падает на релевантной + мутации (проверено лично, не со слов автора) и не задевает остальные 35 + проверок; +- Существующий тест на позицию `trusted_proxy=True` + (`test_client_cannot_pick_its_own_rate_bucket`) не тронут дельтой и + по-прежнему проходит; +- Деплой-артефакты (`scripts/support-relay/deploy/env.example`, + `Caddyfile.fragment`) уже фиксируют `HP_RELAY_TRUSTED_PROXY=1` и перезапись + заголовка на обоих продовых хостах — согласуется с новым текстом §9.2 «Both + production hosts run behind Caddy, so both keep the switch on»; +- `node scripts/process-gate.mjs` зелёный (12 коммитов в диапазоне, 0 + предупреждений); трейлеры обоих коммитов дельты корректны + (`Issue: #43`, `User-Visible: no`); +- Документ ТЗ остаётся внутренне непротиворечивым за пределами дельты — не + найдено новых противоречий между §9.2 и остальными разделами при повторном + чтении документа целиком. + +## Чего не проверял и почему + +- `npx tsc --noEmit`, `npm test`, `npm run build` со сверкой копий бандла — + дельта не трогает `src/**`/`custom_components/**`; докс- и Python-тест-онли + изменение не имеет для них предмета. (Ранее на цепочке этого issue Validate + на предыдущих relay-коммитах уже проходил зелёным на CI — здесь новых + фронтенд/бэкенд-в-смысле-Python-интеграции изменений нет вовсе.) +- `node scripts/check-docs.mjs` — требуется только при изменении `src/**`; + здесь его нет. +- `node scripts/model-invariants.mjs` — геометрия/`layout`/ссылки на неё не + затронуты; речь только о rate-limit relay. +- `python -m pytest tests_backend -q` — эта команда покрывает + `custom_components/houseplan/**`, не relay-поддерево; relay использует + отдельный `unittest discover -s scripts/support-relay/tests`, который + прогнан лично (см. выше). +- Браузерные смоки, `golden:verify`, performance-профили — нет UI/визуальных + изменений в этой дельте; продуктовый код (`src/**`) для #43 ещё не начат + (подтверждено комментарием автора «Продуктовый код (`src/**`) не начат — §17 + соблюдён» и отсутствием изменений `src/**` во всём диапазоне `origin/dev..HEAD`). +- Повторная проверка §9.1/§9.3 (архитектура каналов доставки, ретеншн) и + внешних DoR-фактов §17 «с нуля» (например, повторный `curl` на + `support.houseplan.tech`/`support-staging.houseplan.tech`) — не предмет этой + дельты; принято по унаследованной цепочке r2→r3 с воспроизведёнными в issue + командами, см. раздел «Унаследовано из r4» выше. + +## Вердикт + +Единственная Medium-находка r4 закрыта предметно (текст ТЗ + код, который уже +был правильным, + новый работающий и лично провалившийся на мутации тест). +Единственная новая находка этого раунда — Low, косметическая, в devops-README, +не влияющая ни на один AC, снята с записью без возврата автору. + +**Зелёный.** ТЗ готово к `S5-ready`. С учётом комментариев в issue все пять +внешних DoR-зависимостей §17 отмечены автором как закрытые — если это +подтверждается конвейером/владельцем, `blocked` может сниматься вместе с +переходом статуса.