docs: review document for #432

Issue: #432 User-Visible: no
2026-10-01 04:09:17 +00:00 · 2026-09-03 09:38:37 +00:00
parent 17a1c10bef
commit 58df908db2
1 changed files with 194 additions and 0 deletions
@@ -0,0 +1,194 @@
+# SPEC-REVIEW-432-r1
+
+- Issue: https://github.com/Matysh/houseplan-card/issues/432
+- ТЗ: `docs/specs/432-asset-resolve-authorization-cache.md`
+- Материал ревью: SHA `17a1c10bef67ecd6235d36e324416e58142f3e11` (HEAD ветки на момент ревью, коммит `docs(spec): define bounded asset resolution`, дерево ветки `issue/432-asset-resolve-authorization-cache`)
+- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (лимит для полного трека — 4; лёгкий/короткий трек не применяется, трек полный)
+- Вердикт: **зелёный**
+
+## Скоуп ревью
+
+Первый заход ревью ТЗ для issue #432 (security/performance баг: `houseplan/assets/resolve`
+без проверки прав и без ограничения стоимости хеширования; тот же дефект стоимости у
+`HouseplanContentView.get()`). Аналитика зафиксировала полный трек (два endpoint/модуля,
+публичный контракт доступа и стоимость файловых операций меняются — критерии `small`
+не выполняются, это явно названо в комментарии аналитики). Владелец ответил на
+единственный продуктовый вопрос (Q1: что видит non-admin при `admin_only`) до написания
+ТЗ; ТЗ фиксирует принятый Default. Ревью — по `PROCESS.md` §2.4 и §7.1, разбор полный
+(первый заход, раздел «Унаследовано» не применяется).
+
+## Как проверялось
+
+1. `docs/SCOPE.md` — сценарий и персоны сверены с J1 (живой обзор), J4 (онбординг/каталог)
+   и J6 (устойчивость интеграции); особо — «View mode is the product for two of the three
+   personas», что прямо мотивирует контракт non-admin в ТЗ.
+2. `AGENTS.md`, `PROCESS.md` §1, §2.3–2.4, §5, §7.1, §7.2 — формат ТЗ, класс изменений
+   (класс C, документ, `Issue:#432`/`User-Visible: no` в коммите `17a1c10b` — сверено
+   `git show --stat`), обязательные разделы, лимит циклов, формат вердикта.
+3. Тело issue #432 и все 4 комментария (аналитика, вопрос Q1, решение владельца по Q1,
+   хендофф ТЗ на ревью) прочитаны целиком.
+4. Код на этом SHA прочитан против каждого фактического утверждения ТЗ, не поверх:
+   - `custom_components/houseplan/websocket_api.py:1127–1161` — `ws_assets_resolve`
+     подтверждён: нет `_check_write`, нет `_runtime()`, полный `read_catalog(root)` +
+     `path.read_bytes()` + SHA-256 на совпавшую строку каталога;
+   - `custom_components/houseplan/http_api.py:157–216` — `HouseplanContentView.get()`
+     подтверждён: полный `read_bytes()` + SHA-256 на каждый GET `assets`, `immutable`
+     заголовок не ограничивает повторные запросы;
+   - `custom_components/houseplan/auth.py:16–31` — `may_write()` подтверждает точную
+     семантику writer/read-only, которую ТЗ использует в AC1–AC3;
+   - `custom_components/houseplan/decor_assets.py:353–408` — `asset_refs()`,
+     `read_catalog()`, `public_asset()` существуют и имеют заявленную сигнатуру;
+     `asset_refs()` действительно покрывает единственное место использования
+     `asset_id` в конфиге (перепроверено по `import_export.py`, `validation.py` —
+     других держателей `asset_id` в config нет);
+   - `custom_components/houseplan/const.py` — квоты 200 файлов / 256 МиБ / 2 МиБ и
+     `DECOR_ASSETS_API_VERSION = 1` подтверждены, совпадают с заявленным в ТЗ §6/§10;
+   - `custom_components/houseplan/store.py:78–92` — `write_lock`/`upload_lock`
+     существуют на `HouseplanData`, паттерн `async with rt.write_lock` уже используется
+     для похожего authoritative snapshot в `ws_assets_list` — контракт §7.3 технически
+     реализуем без изобретения нового примитива.
+5. Сверены смежные документы: `docs/specs/051-custom-decor-images.md:323` — оригинальный
+   контракт `houseplan/assets/resolve` действительно зафиксирован как `authenticated
+   read` (не writer-only); `docs/specs/131-readonly-cold-start.md` — подтверждает, что
+   read-only View обязан быть визуально полным, что обосновывает Default-решение по Q1.
+   `docs/CONFIG-COMPATIBILITY.md:170` — запись про #432 добавлена и указывает на верный
+   файл ТЗ.
+6. Проверено использование `resolveDecorAssets()` (`src/decor-assets.ts`) обеими
+   поверхностями — `src/houseplan-card.ts` и `src/space-card.ts` — что подтверждает
+   заявление ТЗ §11 о parity full/space card и наличие существующего frontend unit
+   теста `test/decor-assets.test.mjs`, на который ТЗ ссылается как на доказательство
+   для read-only View (AC1 покрывается backend-контрактом + этим тестом, а не новым
+   frontend-тестом).
+7. `scripts/mutation-gate.mjs` — подтверждено, что реестр уже содержит мутанты для
+   `custom_components/houseplan/websocket_api.py` с backend pytest guard'ами (например,
+   строки 146–179), то есть план ТЗ §14/AC11 зарегистрировать постоянных свидетелей для
+   backend-защит — не изобретение нового механизма, а использование существующего.
+8. Проверены обязательные разделы §7.1 PROCESS.md построчно (см. таблицу ниже) и
+   однозначность/доказуемость каждого AC1–AC11.
+9. Дешёвые гейты не перегонялись: коммит `17a1c10b` — чистый docs-diff (`docs/specs/
+   432-asset-resolve-authorization-cache.md` + одна строка в `docs/specs/README.md`),
+   подтверждено `git show --stat`; Validate на этом SHA зелёный (см. ссылку в задании).
+   Для документа спецификации без изменений в `src/**`/`custom_components/**/*.py`
+   `typecheck`/`test`/`build`/`check-docs`/инварианты модели не относятся к предмету
+   ревью этого этапа — само содержимое ещё не код, а его читаемость и доказуемость.
+
+## Проверка §7.1 (обязательные разделы) и однозначность AC
+
+| Раздел §7.1 | Есть в ТЗ | Где |
+|---|---|---|
+| Сценарий (персона/поверхность/момент) | ✅ | §1 |
+| Что человек увидит до/после | ✅ | §2 |
+| Проблема | ✅ | §3, подтверждена кодом (см. выше) |
+| Скоуп / не-скоуп | ✅ | §5 / §6 |
+| Контракт поведения | ✅ | §7–§10 (доступ WS, GET, cache, ошибки/совместимость) |
+| UX | ✅ | §11 — явно «новых контролов, текстов… нет» |
+| Модель данных и миграция | ✅ (кратко, по существу — миграции нет) | §10 «Ошибки и совместимость», §20 (cache не persisted) |
+| i18n | ✅ | §11 |
+| AC1…ACn с доказательством | ✅ | §13, каждый AC помечен способом доказательства (`backend/HA`, `backend/unit`, `review/docs`, `mutation gate`) |
+| План автотестов | ✅ | §15, 8 пунктов, включая явный список implementation-гейтов |
+| Риски | ✅ | §17, 6 рисков со смягчением |
+| Откат | ✅ | §18 |
+| Release-артефакты | ✅ | §19 |
+
+Раздел «Модель данных и миграция» не вынесен отдельным заголовком, а распределён между
+§10 и §20 — содержательно раздел закрыт (нет schema/capability migration, cache
+memory-only и не persisted), структурно это Low, не блокирует (см. «Находки»).
+
+Обязательная по правилу #435 таблица защитных доказательств присутствует (§14),
+третий столбец «чем краснеет» заполнен для каждой строки конкретной мутацией и
+наблюдаемым эффектом — не общей фразой.
+
+## Проверка отсутствия непомеченных догадок
+
+Каждое фактическое утверждение о текущем поведении кода в ТЗ (§3, §7.1, §9.3, ссылки на
+`may_write`, `asset_refs`, `read_catalog`, `write_lock`, квоты, capability-версию,
+контракт #51 «authenticated read», обязательность read-only View по #131) сверено с
+реальным кодом/документами выше и подтвердилось. Технические решения, для которых
+однозначного prior art нет (например, точный состав cache signature `size + mtime_ns +
+ctime_ns`, выбор между fail-dark и одной повторной попыткой, место хранения cache —
+`hass.data` либо runtime-сервис), явно вынесены в §20 «Принятые технические
+предположения» с пометкой «ревьюер вправе оспорить» — ни одно не выдано за факт.
+Продуктовый вопрос (Q1) задан владельцу отдельно и заранее, до написания ТЗ, что и
+требует правило «не бывает сложной задачи без единого открытого вопроса» — вопрос был,
+он закрыт до этапа ревью, что для ревью ТЗ корректно (открытых продуктовых вопросов
+к моменту сдачи ТЗ быть не должно).
+
+## Находки
+
+Нет находок уровня High или Medium.
+
+**Low (не блокирует, зафиксировано без правки).**
+
+1. Раздел «модель данных и миграция» из обязательного списка §7.1 PROCESS.md не выделен
+   отдельным заголовком, а распределён по §10/§20. Содержание присутствует и
+   исчерпывающее (нет миграции, cache не persisted), поэтому это вопрос структуры
+   документа, а не пропущенное решение. Снимается без правки: следующий автор того же
+   ТЗ увидит прецедент, что содержание важнее буквального оглавления, когда факт «нет
+   миграции» явно закрыт в другом месте того же документа.
+
+## Что проверено и корректно
+
+- Полная grounding-проверка технических утверждений ТЗ против фактического кода
+  (`websocket_api.py`, `http_api.py`, `auth.py`, `decor_assets.py`, `const.py`,
+  `store.py`) — расхождений не найдено.
+- Product-рамка: сценарий и «что человек увидит» отвечают на оба обязательных
+  продуктовых вопроса, персона и поверхность названы, соответствие J1/J4/J6 по
+  `docs/SCOPE.md` подтверждено, включая явную ссылку на инвариант read-only View (#131).
+- Решение владельца по Q1 корректно перенесено в контракт (§4, §7.3) без искажения:
+  read-only видит только referenced-assets, writer — полный каталог, GET не меняется.
+- AC1–AC11 однозначны, у каждого назван способ доказательства; для защитных AC (AC2,
+  AC3, AC5, AC6, AC7, AC9) заполнена обязательная по #435 таблица «чем доказан / чем
+  краснеет» с конкретной мутацией, а не общей фразой.
+- Не-скоуп (§6) корректно отделяет эту задачу от смежных: quota/upload-валидация,
+  writer-only GET, config schema migration, frontend/i18n, общий cache для других
+  типов файлов — явно исключены и не проросли в контракт.
+- Откат (§18) явно запрещает «тихо» отключать security/performance защиту через флаг —
+  соответствует духу standing rule о необратимых действиях.
+- Release-артефакты (§19) требуют оба changelog, обновление ARCHITECTURE.md и
+  CONFIG-COMPATIBILITY.md, что уже подтверждено записью в README ТЗ на этом SHA.
+- Трейлеры коммита `17a1c10b` (`Issue: #432`, `User-Visible: no`) корректны для
+  docs-only спецификации; `docs/specs/README.md` содержит обратную ссылку на issue.
+
+## Чего не проверял
+
+- Реализацию — код ещё не написан, это этап ревью ТЗ, не код-ревью.
+- Полный набор гейтов (`typecheck`/`test`/`build`/`golden`/backend pytest/browser
+  smoke/`model-invariants`) — не относится к предмету этого этапа: диапазон материала
+  этого раунда — только `docs/specs/432-*.md` и тело issue, изменений в `src/**` или
+  `custom_components/**/*.py` в этом коммите нет. Дешёвые гейты на SHA `17a1c10b`
+  подтверждены зелёным Validate (ссылка в задании), поэтому не перегонялись повторно.
+- Осуществимость точной реализации bounded single-flight (потокобезопасность между
+  executor-потоками HA) — это техническое решение, оставленное автору по правилу §7.1
+  PROCESS.md («всё, чего пользователь не наблюдает, агенты решают сами»); будет
+  предметом код-ревью через AC6 и его mutation witness.
+- Полноту `scripts/mutation-gate.mjs` записей для AC2/AC5/AC6/AC7 — их ещё нет (ТЗ
+  только планирует их появление в §14/AC11), поэтому проверять на этом этапе нечего;
+  это станет предметом код-ревью.
+
+## Материал раунда
+
+- SHA: `17a1c10bef67ecd6235d36e324416e58142f3e11`
+- Дерево: `docs/specs/432-asset-resolve-authorization-cache.md`,
+  `docs/specs/README.md` (запись про #432)
+- Ветка: `issue/432-asset-resolve-authorization-cache`
+- Первый заход — раздел «Унаследовано из r0» не применяется.
+
+## Вердикт
+
+Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/432-asset-resolve-authorization-cache`, коммит `17a1c10bef67` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `efc2aa18b50775a262a444d5ea1544c588646eba`
+  ```
+  git log --all --format='%H %T' | grep efc2aa18b507
+  ```
+- ТЗ `docs/specs/432-asset-resolve-authorization-cache.md`, блоб `8593bd54ad7d7e8a4d6459949fbb960c34ccddc9`
+  ```
+  git log --all --find-object=8593bd54ad7d7e8a4d6459949fbb960c34ccddc9 -- docs/specs/432-asset-resolve-authorization-cache.md
+  ```