diff --git a/docs/reviews/CODE-REVIEW-43-r1.md b/docs/reviews/CODE-REVIEW-43-r1.md new file mode 100644 index 00000000..5d36cb3a --- /dev/null +++ b/docs/reviews/CODE-REVIEW-43-r1.md @@ -0,0 +1,395 @@ +# CODE-REVIEW-43-r1 + +- Issue: [#43](https://github.com/Matysh/houseplan-card/issues/43) — Диалог помощи и обратной связи с обезличенным support report +- Ветка: `issue/43-help-feedback` +- SHA ревью: `1ba5363038055b3f3442b62a761161d05c6d03c7` (merge `origin/dev` в issue-ветку поверх `1e2e0fa6` — продуктовая реализация) +- Заход: r1 (первый код-ревью; ТЗ прошло 5 раундов ревью и зелёное, `S5-ready` → `S7-code-review`) +- Вердикт: **жёлтый** · блокирующих циклов 1/4 · High: 0 · Medium: 3 → в задаче + +## 1. Скоуп + +Диапазон `origin/dev...HEAD`: 71 файл, +7023/−411. Реализация ТЗ +`docs/specs/043-private-support-report.md` (ревизия 2, зелёное ревью r5): + +- фронтенд: кнопка «Помощь и обратная связь» в шапке, новый диалог (перенос + «О карточке», языковая ссылка на USER-GUIDE, форма отчёта, preview + диагностического пакета) — `src/houseplan-card.ts`, + `src/houseplan-editor-runtime.ts`, `src/support-feedback.ts`, + `src/hp-dialog.ts`, `src/styles/*.ts`, `src/i18n/*.json`; +- backend: три websocket-команды `houseplan/support/{preview,preview/discard,submit}`, + allowlist-проекция `support_package.py`, транспорт `support_transport.py`, + константы в `const.py`; +- отдельный class-B сервис `scripts/support-relay/**` (написан и развёрнут на + проектном стенде ещё во время цикла ТЗ, при закрытых DoR-зависимостях §17; + это уже было предметом пяти раундов ревью ТЗ, включая живой пентест владельца + на трассировке `X-Forwarded-For`); +- документация: `USER-GUIDE.{md,ru.md}`, `ARCHITECTURE.md`, + новый `SUPPORT-PRIVACY.md`, `TESTING.md`, оба `CHANGELOG`. + +Это первый код-ревью задачи — раунд полный, дельты по §2.10 нет. + +## 2. Как проверялось + +Ревью кода отвечает на вопрос «оно вообще работает» вместо ручного +тестирования (§2.7). Ниже — что выполнено лично, а не заявлено. + +### 2.1 Дешёвые гейты (прогнаны лично, HEAD `1ba53630`) + +| Гейт | Команда | Результат | +|---|---|---| +| Типы | `npx tsc --noEmit` | зелёный, 0 ошибок, 6.2 с | +| Юнит/интеграционные (frontend) | `npm test` | 1724 теста: 1723 passed, 1 skipped, 0 failed, 28.2 с | +| Сборка + сверка бандла | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | идентичны; `npm run bundle:sync` не меняет рабочее дерево (`git status` чист) — все три копии (`dist`, `custom_components/.../frontend`, `demo/srv/assets`) уже синхронны в коммите | +| Новый `any` | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | **красный** — 4 новых явных `any` без `// any-ok:` (находка Medium №3 ниже) | +| Docs-контракт | `node scripts/check-docs.mjs` | **красный** — `screenshot source fingerprint is stale` (находка Medium №1 ниже) | +| process-gate | `node scripts/process-gate.mjs --range origin/dev..HEAD --issues` | «гейт пройден, предупреждений 0», 15 коммитов, включая проверку статуса issue через `gh` | + +`check-docs` обязателен, потому что диапазон меняет `src/**` +(`houseplan-card.ts`, `houseplan-editor-runtime.ts`, `hp-dialog.ts`, оба +`styles/*.ts`, все 4 `i18n/*.json`) — правило §8 не оставляет выбора. +Проверено, что стойкость не унаследована из `dev`: тот же скрипт на чистом +`origin/dev` (отдельный worktree, `319b25c2`) даёт «Documentation checks passed +(7 files, 10 external links)» — значит, устаревание отпечатка порождено именно +этим диффом, а не фоновой работой #410. + +### 2.2 Бэкенд Python (доступность харнесса) + +Локальный образ ревьюера без `.venv-backend` (как и описанный в `AGENTS.md` +случай «Local Windows checkout»). Установил недостающие пакеты вручную, чтобы +не оставлять этот участок полностью непроверенным: + +- `pip install pytest voluptuous aiohttp pytest-asyncio homeassistant` (без + пина) → `tests_backend/test_support_package.py`: **10 passed** (модуль + `support_package.py` намеренно не импортирует HA, тест не нуждается в + харнессе); `tests_backend/test_ha_support_transport.py`: **7 passed** (этому + файлу тоже хватает голого `homeassistant.helpers.aiohttp_client`, харнесс + `pytest-homeassistant-custom-component` не нужен). +- `pip install pytest-homeassistant-custom-component` (снова без пина; CI пинует + `homeassistant==2026.8.3` + `phcc==0.13.357` под Python 3.14, здесь только + 3.12) → резолвер подобрал несовместимую пару, `python -m pytest + tests_backend -q` дал **96 failed / 387 passed** по всему `test_ha_*.py`, + включая файлы, никак не связанные с #43 (`test_ha_upload.py`, + `test_ha_virtual_lights.py`). Это диагностика рассинхронизации версий + харнесса, а не регрессия диффа — **результат не используется как + доказательство ни в одну сторону**. Четыре support-специфичных теста в + `test_ha_websocket.py` (`test_support_preview_is_authorized_exact_and_consumed_only_after_success`, + `test_support_preview_replacement_and_discard_are_draft_local`, + `test_support_text_only_submit_carries_safe_versions_without_plan_data`, + `test_support_commands_reject_read_only_user_before_build_or_transport`, + `test_support_preview_schema_does_not_coerce_client_facts[...]`) упали той же + генерической `assert False`, что и заведомо исправные несвязанные тесты — + подтверждает, что причина в среде, не в тесте. +- Итог: полный HA-харнесс в этом ревью не поднят (нет Python 3.14 в песочнице, + тянуть его ради одного прогона — непропорциональная трата времени). Авторский + хендофф в issue называет точные числа с зелёными прогонами на его машине + (`privacy/backend targeted — 15 passed`, `support relay — 36 passed`) — + доверяю числу для support_package.py/support_transport.py (сам перепроверил, + совпадает — 10+7=17, близко к заявленным 15 при другом подсчёте узкого + таргета), но для `test_ha_websocket.py` авторские числа не перепроверены + исполнением. Соответствующие AC (AC9, AC10) закрыты ниже пометкой «проверено + чтением, не исполнением», не автотестом с моей стороны. + +### 2.3 Relay (`scripts/support-relay/**`) + +`python3 -m unittest discover -s scripts/support-relay/tests -q` → **36 +passed** (тот же набор, что и в последнем зелёном спек-ревью r5; код `hp_relay/**` +этим диапазоном не менялся — сверено `git diff origin/dev...HEAD -- +scripts/support-relay/hp_relay` вместе с диапазоном коммитов истории: relay был +написан и вычитан во время цикла ТЗ пятью раундами ревью, включая живой пентест +владельца против подмены `X-Forwarded-For`). Прочитал код заново (не как +унаследованное, а как часть этого код-ревью — предмет здесь другой гейт, +код-ревью, а не спек-ревью): + +- `hp_relay/app.py:147-158` — источник rate-limit берётся из последнего + элемента `X-Forwarded-For` только при `trusted_proxy=True` (default), + иначе — адрес TCP-соединения; совпадает с `scripts/support-relay/deploy/Caddyfile.fragment` + (`header_up X-Forwarded-For {remote_host}` — заголовок перезаписывается + целиком на обоих продовых сайтах). +- `hp_relay/delivery.py` — оба канала (`TelegramDelivery`, `HaWebhookDelivery`) + не используют `parse_mode`, то есть Telegram показывает текст пользователя + буквально; ответ провайдера не отражается наружу и не попадает в HTTP-ответ + клиенту (`app.py` мапит статус на закрытый список кодов). +- `hp_relay/app.py` — единственный лог-метод (`log_message`) переопределён и не + печатает адрес источника, только команду и путь. + +Отдельно нашёл несостыковку единиц измерения в конфигурации прокси (находка +Low №4 ниже) — не блокирует, эффекта на легитимный трафик не имеет по расчёту. + +### 2.4 Смоки и golden + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD` печатает ~40 +существующих смоков — все совпадают по диффу через уже существующие символы +(`_config`, `_editorRuntime`, `_infoCard`, `_markerDialog`), ни один не создан и +не изменён этим диффом. `ls demo/smoke_*.mjs | grep -iE "support|help|feedback"` +находит `smoke_feedback_v2.mjs` и `smoke_help_affordance.mjs` — оба +существовали в `origin/dev` до этой ветки и относятся к issue #68 (контекстная +помощь `hp-help`/`.rlgearbtn`), никак не к диалогу #43. **Ни один существующий, +ни один новый браузерный смок не касается нового диалога, кнопки или формы.** +`git diff --stat origin/dev...HEAD -- demo/` — пусто; `-- demo/golden` — пусто. +Причина находки Medium №2 ниже. + +### 2.5 Инварианты модели + +Не прогонял `npm run invariants -- --config <...>`. Диапазон не меняет ни +`validation.py`, ни модуль геометрии/толщины стен, ни схему `layout` (сверено +`git diff --stat` — `custom_components/houseplan/{diagnostics,import_export,validation}.py` +не тронуты). `support_package.py` только **читает** уже провалидированные +config/layout под тем же `write_lock` и строит отдельный allowlist-объект; +ничего не пишет обратно в хранимую модель. Инварианты о ключах записи толщины и +разрешимости ссылок относятся к хранимой модели, а не к экспортному снапшоту — +гейт не по предмету этого диффа, не «пропущен», а не применим. + +### 2.6 Одно число — один источник + +Проследил цепочку preview: backend строит `bytes`/`sha256`/`size` один раз в +`ws_support_preview` (`websocket_api.py:2153-2221`), кладёт в +`rt.support_previews[token]` и больше не пересчитывает — submit +(`websocket_api.py:2260-2312`) берёт `preview.get("bytes")`/`preview.get("sha256")` +из того же токена, никогда не перестраивая пакет. На фронтенде +`_buildSupportPreview` (`houseplan-editor-runtime.ts:9193-9250`) сверяет +`response.size` с независимо посчитанным `TextEncoder().encode(text).byteLength` +и падает в `support_rejected` при расхождении, затем кладёт результат в один +`SupportPreview`-объект (`support-feedback.ts:6-16`), который читают бейдж +размера, строка SHA-256, `