houseplan-card/docs/reviews/SPEC-REVIEW-626-r1.md

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