docs: review document for #626

Issue: #626
User-Visible: no

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` |

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 → в задаче**
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/626-acl-policy`, коммит `e52afe63495b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `0977af5c3a98f43e9e74c633f2817e204e35f673`
+  ```
+  git log --all --format='%H %T' | grep 0977af5c3a98
+  ```
+- Тело issue: `83f287ba63d6a04d1ffa6f1205382587c2342cbc04bfaebcde04219d841fa564`
+- Вердикт конвейера: `green` · High 0