diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index dd764a86..30e7542b 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,6 +1,6 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1052, issue: 371. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1053, issue: 372. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| @@ -47,6 +47,7 @@ | #627 | [CODE-REVIEW-627-r1.md](CODE-REVIEW-627-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | demo/smoke_danger_confirmation.mjs не переведён на ожидание составного гейта; диалог оп… | `demo/smoke_danger_confirmation.mjs` `src/houseplan-card.ts` `de.ts` `smoke_danger_confirm_branches.mjs` | | #627 | [CODE-REVIEW-627-r2.md](CODE-REVIEW-627-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | | #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 | Раздел «что человек увидит» | — | | #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` | diff --git a/docs/reviews/SPEC-REVIEW-626-r1.md b/docs/reviews/SPEC-REVIEW-626-r1.md new file mode 100644 index 00000000..5fba4424 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-626-r1.md @@ -0,0 +1,246 @@ +# SPEC-REVIEW-626-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/626 +- **Этап:** `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- **Трек:** полный (по аналитике автора, комментарий + `#issuecomment-5805305722`: «лёгкий трек: нет (две поверхности, публичный + контракт прав)») +- **Материал:** тело issue #626, раздел `## ТЗ` (снимок на момент ревью, + 2026-09-25, второй комментарий владельца «Решения владельца зафиксированы: + Q1–Q3 — defaults») +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **Роль:** ревьюер ТЗ (не автор) + +## Скоуп ревью + +ТЗ чинит три независимых дефекта прав доступа, найденных аудитом 22.09 +(T8, часть C «ACL-матрица»): + +1. `may_write()` при `admin_only=false` делает writer-ом любого вошедшего + пользователя, включая HA-группу `system-read-only` — нужно исключить + read-only группу из writer-набора. +2. `houseplan/plans/list` отдаёт список файлов планов (имена, размеры, + `used_by`) без проверки прав вообще — нужно закрыть тем же + `_check_write()`. +3. `houseplan/trail/get` отдаёт сырые записи трейлов вместе с полем + `source` (внутренний источник карты) без проекции — нужно вырезать + `source` из ответа, не трогая хранилище. + +Проверялось: обязательные разделы §7.1, однозначность и проверяемость +каждого AC (включая колонку «чем краснеет» для защитных AC), соответствие +описания текущего бага коду «как есть», согласие продуктовых решений +владельца (Q1–Q3) с итоговым текстом ТЗ, отсутствие домыслов вместо фактов, +DoR-пункты §2.5 (миграция/touch/performance/i18n/откат), согласованность с +`docs/USER-GUIDE.ru.md` и `docs/ARCHITECTURE.md`. + +## Как проверялось + +1. Прочитан `docs/SCOPE.md` — задача не добавляет функциональность, а чинит + дефект существующей модели прав (`may_write`, admin-only опция), которая + обслуживает все персоны/поверхности разом (домашний админ настраивает + права, домочадцы/гости получают View). Отдельной строки Core user jobs + под «ACL» нет, но это ожидаемо для security bug fix в существующем + механизме, а не новой фичи — вне зоны «нужна строка в SCOPE». +2. Прочитан `docs/process/REVIEWER.md`, разделы PROCESS.md §2.3–§2.5, §7.1, + §4, §7.2, §10.4, `AGENTS.md` целиком. +3. Прочитано тело issue #626 (аналитика + полное ТЗ) и оба комментария — + вопросы владельцу и их разрешение. +4. Сверены умолчания из вопросов владельца с текстом ТЗ построчно: + - Q1 «кто пишет при `admin_only=false`» → умолчание «любой, кроме + `system-read-only`» — совпадает со «Скоуп» и АС1 ТЗ дословно. + - Q2 «что видно из служебного» → умолчание «`plans/list` — только + писателям; `trail/get` — оставить, но без `source`» — совпадает со + «Скоуп», АС3, АС4. + - Q3 «фильтровать ли `config/get` по entity ACL» → умолчание «нет, + задокументировать явно» — совпадает с «Решения владельца» п.3 и ACL- + таблицей ТЗ. + Расхождений между тем, что владелец согласовал, и тем, что записано в + ТЗ как контракт, не найдено. +5. Прочитан код, который ТЗ описывает как текущий баг: + - `custom_components/houseplan/auth.py:15-31` — `may_write` возвращает + `is_admin if admin_only else True`; при `admin_only=false` группы + пользователя (`system-read-only` включительно) действительно не + смотрятся. Совпадает с «Фактами» ТЗ дословно. + - `custom_components/houseplan/websocket_api.py:1052-1096` (`ws_plans_list`) + — подтверждено отсутствие `_check_write()`/`may_write()` в теле функции; + каталог планов сканируется (`hass.async_add_executor_job(_scan)`) и + отдаётся любому аутентифицированному клиенту. Совпадает с фактами. + - `custom_components/houseplan/websocket_api.py:2391-2394` (`ws_trail_get`) + — подтверждено: `connection.send_result(msg["id"], {"trails": rec.book.data if rec else {}})` + без проекции, без гварда. `rec.book.data` — прямая ссылка на + живое хранилище `TrailBook.data` (`trails.py:95-103`), не копия. + - `custom_components/houseplan/websocket_api.py:1191-1193` (`ws_assets_list`) + — подтверждено, что этот эндпойнт уже writer-only, как заявляет ТЗ в + «Фактах» («ws_assets_list закрыт для не-писателей»); АС7 верно требует + не трогать этот контракт. + - `custom_components/houseplan/websocket_api.py:1469` (`ws_config_get`) — + `"can_write": may_write(hass, ...)` уже сейчас единственный источник + флага; `src/config-adoption.ts:381` на фронтенде просто копирует + `cfgResp.can_write` в `host._serverCanWrite`, второго вычисления нет. + Значит требование АС2 «`config/get.can_write` совпадает с `may_write`» + не описывает будущий рефакторинг, а фиксирует уже верную связь — + тест на неё осмыслен и, что важно, не будет тривиально «всегда + зелёным»: он покраснеет, если кто-то продублирует вычисление на + клиенте или разъединит вызовы. + - `custom_components/houseplan/trails.py:104-127` (`TrailBook.on_point`) — + `source` пишется ровно в одном месте на запись (`cur["source"] = source`, + плоское поле на уровне `current`/`previous`, не вложено в `points`). + Формулировка ТЗ «рекурсивно удалить ключ `source`» сильнее, чем требует + реальная структура (хватило бы двух плоских удалений), но это не + ошибка и не двусмысленность — просто более защитный, чем необходимо, + способ; выбор конкретного метода прямо отдан автору реализации + («форму… функции безопасной проекции trail выбирает автор реализации»). +6. Проверена доступность тестовой инфраструктуры, на которую опирается план + автотестов: `hass_read_only_access_token` — уже существующая fixture, + используется в `tests_backend/test_ha_upload.py`, `test_ha_virtual_lights.py`, + `test_ha_websocket.py`. АС1/АС2/АС5 не описывают несуществующий + механизм. +7. Проверены реальные вызовы `houseplan/plans/list` на фронтенде — + `src/houseplan-editor-runtime.ts:7988` и + `src/houseplan-onboarding-runtime.ts:165`, оба в admin/writer-only + потоках (редактор плана, онбординг), у обоих уже есть обработка ошибки + (`toast.plans_list_failed`). Введение `unauthorized` для read-only не + ломает ни одного легитимного текущего вызова UI — read-only пользователь + и так не видит этих экранов (`can_write=false` прячет редакторы). +8. Сверены `docs/USER-GUIDE.ru.md:157-169` (таблица «Права пользователей», + §2) и `docs/USER-GUIDE.ru.md:2173-2183` (§20 «Где лежат данные») — + текущая таблица §2 буквально документирует старый баг («Только + администраторы — выключено» → «Все вошедшие пользователи» получают + право редактирования), это ровно то место, которое АС6/«Затрагиваемые + файлы» обязывает поправить. Согласованность найдена, а не предположена. +9. Сверен `docs/ARCHITECTURE.md:1230-1247` (таблица WS-команд) — описание + ответов `plans/list`/`trail/get`/`assets/list` не противоречит ТЗ и + тоже требует правки (АС6 верно называет этот файл). +10. Проверены `docs/CONFIG-COMPATIBILITY.md` и `docs/TOUCH-SUPPORT.md` на + применимость: задача не меняет ни одно персистентное поле конфигурации + (реестр `config-field-registry.mjs` не затрагивается) и не трогает + View/touch-рендер (только серверная авторизация и проекция ответа) — + ТЗ верно не ссылается на эти документы, и явные «нет» в разделах + «Модель данных» и «UX» корректны. + +Гейты не гонялись: этап `spec`, продуктового кода к этой правке ещё нет — +диапазон ревью это текст ТЗ в теле issue, а не диапазон коммитов веток. Это +ожидаемо для ревью ТЗ, а не пропуск. + +## Находки + +### Low-1 (снято ревьюером, без правки). Раздел «что человек увидит» +называет внутреннее имя поля API вместо пользовательского языка + +**Место:** тело issue #626, раздел «Пользовательский сценарий и результат»: +«После изменения `config/get.can_write=false`, редакторы скрыты, а сервер +независимо отклоняет запись.» + +**Что не так.** §7.1 требует, чтобы это предложение читалось «без терминов +реализации» — а `config/get.can_write` это буквальное имя протокольного +поля, не то, что видит пользователь. По существу описание верное и +однозначное (read-only пользователь теряет доступ к редакторам, сервер +дублирует проверку), просто сформулировано языком протокола, а не +интерфейса. + +**Почему снимаю, а не отправляю на доработку текста.** Формулировка не +создаёт двусмысленности и не меняет поведение AC — переформулировка чисто +косметическая, а Low-находки процесс разрешает снимать решением ревьюера с +записью (PROCESS.md §2.4). Блокировать полный цикл ревью ради одной фразы +было бы дороже, чем сама находка. + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют все и в правильном порядке: + сценарий → что человек увидит → проблема → скоуп/не-скоуп → контракт + поведения (ACL-таблица) → UX → модель данных и совместимость → i18n → + AC1–AC7 с доказательством → план автотестов → риски → откат → + release-артефакты → «принято предположительно». +- Все три продуктовых решения владельца (Q1–Q3) корректно и без искажений + перенесены в контракт, скоуп и не-скоуп — сверено построчно (см. «Как + проверялось», п.4). +- Каждый факт из раздела «Факты» подтверждён чтением кода как есть, а не + принят на слово: `may_write` (auth.py:15-31), незащищённый `plans/list` + (websocket_api.py:1052-1096), незащищённый и непроецируемый `trail/get` + (websocket_api.py:2391-2394, trails.py:104-127), уже writer-only + `assets/list` (websocket_api.py:1191-1193), уже единый источник + `can_write` (websocket_api.py:1469, src/config-adoption.ts:381). +- ACL-таблица внутренне непротиворечива: колонки `admin_only=true/false` + для `plans/list`/`assets/list` в точности следуют из определения + `may_write`, а не вводят отдельный, второй класс прав; `system-read-only` + единая колонка обоснована тем, что результат для неё не зависит от + `admin_only` (АС1 требует `false` при обоих значениях). +- Каждый AC называет способ доказательства; для защитных AC (АС1, АС3, + АС4, АС5) явно назван и механизм «чем краснеет»: АС1 — негативные + user/group case; АС3 — spy на сканирование каталога, не выполняемое при + отказе; АС4 — сравнение `storage` до/после проекции, гарантирующее + отсутствие мутации; АС5 — «удаление group check должно сделать + негативный тест красным», плюс отдельный пункт плана автотестов (6) + требует внести эти защиты в mutant registry. Пустой третьей колонки нет + ни у одного защитного AC. +- Тестовая инфраструктура, на которую опирается план автотестов + (`hass_read_only_access_token`), реально существует и уже используется в + соседних тестах — план не описывает вымышленный механизм. +- Риски раздела покрывают все дефекты из «Фактов» (регресс обычных + домочадцев, fail-closed при неполном user, немутирующая проекция trail, + дрейф UI/backend через единственный `can_write`, регрессия View) — + пробелов не найдено. +- Откат — «откатить продуктовый коммит целиком», без миграции данных, + корректно исключает частичный откат документации/кода порознь (именно + так и рассинхронизировалась бы модель). +- DoR-пункты §2.5 закрыты явными «нет»/значением там, где применимо: + миграции нет (совместимость подтверждена — задача не трогает + `config-field-registry.mjs`), touch-влияния нет (backend-only правка, + View-рендер не меняется), i18n — новых строк нет, release-артефакты + (changelog RU+EN, USER-GUIDE, SCOPE, ARCHITECTURE) названы явно. +- `User-Visible: yes` подразумевается («публичное изменение отражается в + обоих changelog») и корректно требует правки обоих changelog в одном + коммите (AGENTS.md). +- Открытых продуктовых вопросов владельцу нет: комментарий-аналитика + поставил ровно три продуктовых вопроса с умолчаниями, второй комментарий + владельца принял умолчания — процесс §7.1 (вопрос с готовым дефолтом) + соблюдён, а не обойдён. +- Не-скоуп сформулирован явно (entity-filtering `config/get`, координаты + трейла/marker ID, `virtual_light/toggle`, ACL радаров, новая роль/ + настройка) и не пересекается с контрактом. + +## Чего не проверял + +- Гейты (`tsc`, `npm test`, `npm run build`, `check-docs.mjs`, backend + pytest, инварианты) — не прогонял: этап `spec`, кода к задаче ещё нет + (ветка `issue/626-acl-policy` упомянута в занятии, но не является + материалом ревью текста). Это предмет код-ревью после реализации. +- Не проверял вручную поведение реального HA-инстанса (группы + `system-read-only`, `hass_read_only_access_token`) — ревью ТЗ не + предполагает исполнения; grounding сделан чтением кода и существующих + тестов, а не запуском. +- Не оценивал перформанс на реальных объёмах (число групп пользователя, + размер trail-payload) — ТЗ явно называет сложность O(n) и заявляет + отсутствие новых запросов/подписок; количественной проверки на этом + этапе не требуется и не проводилась. +- Не проверял построчно весь `docs/ARCHITECTURE.md` и весь + `docs/USER-GUIDE.ru.md` — только участки, прямо относящиеся к + `plans/list`, `trail/get`, `config/get.can_write` и таблице прав + (§2, §20, ACL-таблица команд). Полная сверка этих документов — работа + АС6 при код-ревью, когда правки уже будут внесены. + +## Вердикт + +Найдено 0 High и 0 Medium. Одна Low-находка (формулировка «что человек +увидит» использует имя API-поля вместо пользовательского языка) снята +решением ревьюера без возврата автору — она не влияет ни на один AC и не +создаёт двусмысленности. ТЗ грамотно замкнуто на трёх продуктовых +умолчаниях владельца, каждый факт о текущем баге подтверждён чтением кода, +каждый AC — включая четыре защитных — называет и доказательство, и мутацию, +на которой тест обязан покраснеть. Задача готова к разработке. + +**Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче** + +--- + + + +## Материал раунда + +- Ветка: `issue/626-acl-policy`, коммит `e52afe63495b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0977af5c3a98f43e9e74c633f2817e204e35f673` + ``` + git log --all --format='%H %T' | grep 0977af5c3a98 + ``` +- Тело issue: `83f287ba63d6a04d1ffa6f1205382587c2342cbc04bfaebcde04219d841fa564` +- Вердикт конвейера: `green` · High 0