diff --git a/docs/reviews/CODE-REVIEW-626-r2.md b/docs/reviews/CODE-REVIEW-626-r2.md new file mode 100644 index 00000000..ddb143bc --- /dev/null +++ b/docs/reviews/CODE-REVIEW-626-r2.md @@ -0,0 +1,169 @@ +# CODE-REVIEW-626-r2 + +Issue: #626 · этап: code · заход: r2 · материал: `b80193f7268f4ae9a332857ea6100773f5fd0d88` +(рабочая копия уже на нём; `git rev-parse HEAD` = `b80193f7268f4ae9a332857ea6100773f5fd0d88`) +Ветка: `issue/626-acl-policy` · база: `origin/dev` +Validate на материале: success, `workflow_dispatch`, +https://github.com/Matysh/houseplan-card/actions/runs/36128068669 +(`headSha` рана сверен командой — совпадает с материалом) + +## Скоуп раунда + +Единственная находка r1 (жёлтый вердикт, Medium в скоупе, High: 0): +потеря прямой HTTP-регрессии «non-admin + `admin_only=true` (умолчание) → +403» в `tests_backend/test_ha_upload.py` — старый тест `test_issue_617_ +plan_upload_refuses_non_admin` был переименован и его `_setup` переведён +на `admin_only=False`, поэтому ветка по умолчанию осталась без своего +HTTP round-trip теста. + +Дельта r1→r2 (`git diff 552504b834c7..HEAD`) — ровно три файла: + +``` +tests_backend/test_ha_upload.py | 15 +++++++++++++++ (продукт теста) +docs/reviews/CODE-REVIEW-626-r1.md | 239 ++++++++++++++++ (артефакт r1, не код) +docs/reviews/INDEX.md | 3 +- (сгенерирован пайплайном) +``` + +Дельта строго локальна: один новый тест в одном файле, ни `auth.py`, ни +`websocket_api.py`, ни документация, ни `mutation-registry.mjs` не +менялись между r1 и r2. Разбор по PROCESS.md §2.10 сужен до AC5 +(единственный AC, которого касается правка); AC1–AC4, AC6, AC7 — +унаследованы без повторной проверки (раздел ниже). + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium: `tests_backend/test_ha_upload.py` — потерян HTTP round-trip регресс `admin_only=true` (умолчание) + non-admin → 403, план автотестов ТЗ требовал «расширить», а не заменить | Коммит `b80193f7` добавляет `test_issue_617_plan_upload_refuses_non_admin_by_default` — точная копия тела старого (удалённого переименованием) теста `test_issue_617_plan_upload_refuses_non_admin` с `origin/dev` (сверено `git show origin/dev:tests_backend/test_ha_upload.py` — идентичное тело: `_setup(hass)` без опций ⇒ `admin_only=true` по умолчанию, `hass_read_only_access_token` как представитель non-admin, `POST /plans/upload` → `403 unauthorized`, каталог планов не меняется) | `tests_backend/test_ha_upload.py:301-315`; тест `test_issue_626_plan_upload_refuses_read_only_but_allows_household` (`:316+`, `admin_only=false`) сохранён отдельно — оба сценария сосуществуют, один не подменяет другой | + +Проверка «умеет ли тест падать» (защитный AC5-HTTP): guard стоит в +`http_api.py:390` (`if not may_write(hass, request.get("hass_user")): return +403`) внутри `HouseplanPlanUploadView.post` — тот самый эндпойнт, который +дергает тест (`/api/houseplan/plans/upload`). Убери этот `if` — ответ +станет `200`/`ok`, а не `403`, и оба ассерта (`resp.status == 403`, +`error == "unauthorized"`) покраснеют; третий ассерт (`_plans_listing` +до/после) ловит побочный эффект (файл всё же попал бы на диск). +Специального мутанта в `scripts/mutation-registry.mjs` на этот guard нет +(и не требовался в r1 — `http_api.py` этим диффом не тронут), но сам +тест исполнялся: job «Бэкенд: pytest в Home Assistant» на Validate +`b80193f7` — success, `headSha` совпадает с материалом. Дополнительно +это не новый тест «с нуля»: он воспроизводит код, который годами жил на +`dev` под этим же именем-предком — падение при поломке guard уже +исторически доказано тем же телом теста. + +Находка закрыта полностью, без остатка. Второй Low-находки r1 (комментарий +в `auth.py:35-37`) касается не эта дельта — она была снята ревьюером r1 +без возврата автору и не переоткрывается. + +## Унаследовано из r1 + +Всё, кроме AC5-HTTP, принято без повторного чтения — код между +материалами r1 (`552504b834c7`) и r2 (`b80193f7`) не менялся: + +| AC | Документ/материал r1 | SHA | +|---|---|---| +| AC1 (`may_write` матрица, fail-closed) | `docs/reviews/CODE-REVIEW-626-r1.md`, мутант `acl-readonly-group-becomes-writer` пойман | `552504b834c73484779720a356150aaee92afc28` | +| AC2 (6 команд × роли) | там же, `test_issue_626_authenticated_read_acl_matrix_and_trail_projection` | `552504b834c73484779720a356150aaee92afc28` | +| AC3 (`plans/list` unauthorized, без сканирования) | там же, мутант `acl-plan-catalog-open-to-viewers` пойман | `552504b834c73484779720a356150aaee92afc28` | +| AC4 (`trail/get` без `source`, storage не мутируется) | там же, мутант `acl-trail-view-exposes-source-entity` пойман | `552504b834c73484779720a356150aaee92afc28` | +| AC5-WS (`config/set` отклоняет read-only, пропускает household) | там же, `test_issue_626_authenticated_read_acl_matrix_and_trail_projection` | `552504b834c73484779720a356150aaee92afc28` | +| AC6 (документация непротиворечива) | там же, `check-docs.mjs` green на материале r1; диффом r1→r2 документы не тронуты | `552504b834c73484779720a356150aaee92afc28` | +| AC7 (регресс: `virtual_light/toggle`, `layout/get`, `assets/list`) | там же, существующие HA-harness тесты | `552504b834c73484779720a356150aaee92afc28` | + +Также без повторной проверки принято: не-скоуп соблюдён (entity-level +фильтрация, ACL радаров, `virtual_light/toggle` — не тронуты ни r1, ни +r2); трейлеры `552504b8`/`e6987459` корректны (сверено в r1); Low-находка +про комментарий в `auth.py` снята в r1 и не переоткрывается. + +## Как проверялось в r2 + +| Гейт | Статус | Источник | +|---|---|---| +| `npx tsc --noEmit` | не гонял | Validate green на материале (job «Фронтенд»); диффа по `src/**` нет ни в r1, ни в дельте r1→r2 | +| `npm test` | не гонял | то же | +| `npm run build` + сверка 3 копий бандла | не гонял | Validate green («Фронтенд: …синхрон бандла») | +| `node scripts/check-docs.mjs` | не гонял | job «Предполёт» green на материале; дельта r1→r2 документов не касается | +| `python -m pytest tests_backend -q` (реальный HA) | не гонял — `homeassistant` не установлен в этой среде | job «Бэкенд: pytest в Home Assistant» на `b80193f7` — success; проверил `headSha` рана командой `gh run view` — совпадает с `git rev-parse HEAD` | +| Мутанты по диффу (6 шардов) | не гонял — CI это уже сделал | все 6 шардов «Мутанты по диффу» на `b80193f7` — success | +| `node scripts/smoke-select.mjs` | не гонял повторно | дельта r1→r2 не касается `src/**`; вывод r1 («исполняемого frontend-диффа нет») остаётся в силе, дельта его не меняет | +| `golden:verify` | не применимо | нет видимого изменения рендера ни в r1, ни в дельте | +| Инварианты геометрии | не применимо | правка не касается геометрии | +| Performance-профили | не применимо | в AC не названы | + +Помимо CI: прочитан весь дифф `552504b834c7..HEAD` (три файла, см. выше), +сверено тело нового теста с телом его предка на `origin/dev` посимвольно, +прочитан guard `http_api.py:372-398` (`HouseplanPlanUploadView.post`), +подтверждено расположение `may_write()`-проверки на `http_api.py:390` до +любой записи на диск. + +## Находки + +Ни одной новой High/Medium/Low находки в дельте r1→r2. Единственная +находка r1 закрыта полностью (раздел «Закрытие раунда r1» выше). + +## Что проверено и корректно + +- Восстановленный тест `test_issue_617_plan_upload_refuses_non_admin_by_ + default` — точное воспроизведение исторически существовавшего теста; + покрывает именно ту ветку (`admin_only=true` по умолчанию), которую + потеряла правка r1. +- Оба HTTP-сценария (`admin_only=true` non-admin → 403, + `admin_only=false` read-only → 403 / household → 200) теперь + сосуществуют как отдельные тесты — ни один не подменяет другой; план + автотестов ТЗ («расширить существующий тест… либо добавить отдельный») + выполнен вариантом «добавить отдельный». +- Коммит `b80193f7`: трейлеры `Issue: #626`, `User-Visible: no` — + верно, правка тестовая, продуктовое поведение не меняется, оба + changelog не требуются. +- `docs/reviews/INDEX.md` — правка сгенерирована пайплайном + (`node scripts/reviews-index.mjs`) вместе с публикацией документа r1, + не ручная правка автора. +- Не-скоуп по-прежнему соблюдён: дифф r1→r2 не касается ни одного файла + вне `tests_backend/test_ha_upload.py`. + +## Чего не проверял + +- Не выполнял `tsc`/`npm test`/`npm run build`/`pytest tests_backend` + вручную — только чтением и по зелёному Validate на точном SHA + материала (`headSha` рана сверен с `git rev-parse HEAD`). +- Не устанавливал `homeassistant` в этой среде — полагаюсь на + canonical Linux CI backend job, зелёный на этом самом SHA. +- Не прогонял browser-smokes и golden — дельта r1→r2 не трогает + `src/**`, вывод `smoke-select.mjs` из r1 (пусто) остаётся в силе. +- Не проверял поведение при реальном HA-инстансе за пределами + backend-job CI — тот же остаточный риск, что и в r1, принят ТЗ. +- Не проверял производительность — не заявлена в AC, дельта тестовая. + +## Вердикт + +Единственная Medium-находка r1 закрыта восстановленным тестом, +воспроизводящим тело своего исторического предка; новых находок в +дельте r1→r2 нет. Все семь AC подтверждены (AC1–AC4, AC6, AC7 — +унаследованы от r1 без изменений в коде; AC5 — переподтверждён целиком, +включая ранее ослабленную HTTP-ветку). Изменение решает заявленный +сценарий (`docs/SCOPE.md` J2/J6: read-only остаётся зрителем, +администратор не теряет доступ) и не ухудшает ни один смежный core user +job. + +--- + + + +## Материал раунда + +- Ветка: `issue/626-acl-policy`, коммит `b80193f7268f4ae9a332857ea6100773f5fd0d88`. +- Вердикт: green + +--- + + + +## Материал раунда + +- Ветка: `issue/626-acl-policy`, коммит `b80193f7268f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `338932c37fe9f6fbed43aa4db18abbbbc91fb515` + ``` + git log --all --format='%H %T' | grep 338932c37fe9 + ``` +- Тело issue: `83f287ba63d6a04d1ffa6f1205382587c2342cbc04bfaebcde04219d841fa564` +- Вердикт конвейера: `green` · High 0 diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 2e498c30..29b5fb5b 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,6 +1,6 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1054, issue: 372. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1055, issue: 372. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| @@ -49,6 +49,7 @@ | #627 | [CODE-REVIEW-627-r3.md](CODE-REVIEW-627-r3.md) | code · r3 | 🟢 зелёный | 0 | 0 | — | — | | #626 | [SPEC-REVIEW-626-r1.md](SPEC-REVIEW-626-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | Раздел «что человек увидит» | — | | #626 | [CODE-REVIEW-626-r1.md](CODE-REVIEW-626-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | Потеряна прямая регрессионная проверка HTTP-загрузки при admin_only=true (значение по у…; custom_components/houseplan/auth.py:35-37 — комментарий «HA normally exposes admin grou… | `tests_backend/test_ha_upload.py` `http_api.py` `scripts/mutation-registry.mjs` `test_ha_upload.py` `custom_components/houseplan/auth.py` | +| #626 | [CODE-REVIEW-626-r2.md](CODE-REVIEW-626-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | | #625 | [SPEC-REVIEW-625-r1.md](SPEC-REVIEW-625-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | новый инвариант markers[].id не описывает исход для уже испорченной хранимой конфигурации; продуктовые формулировки §7.1 неполны; не проговорены явные «нет» по i18n/touch | `validation.py` `__init__.py` | | #625 | [SPEC-REVIEW-625-r2.md](SPEC-REVIEW-625-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | | #625 | [CODE-REVIEW-625-r1.md](CODE-REVIEW-625-r1.md) | code · r1 | 🟡 жёлтый | 0 | 0 | Три из четырёх точек вызова validate_active_marker_ids не имеют ни одного теста, exerci…; AC5 текстуально обещает «отдельные тесты сохраняют поведение при отсутствующем length» …; store.py:async_save_config_state — controller.async_flush() и последующий controller.re… | `custom_components/houseplan/websocket_api.py` `test_validation.py` |