From 58df908db20618619aaed35a039278da07c4a856 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:38:37 +0000 Subject: [PATCH] docs: review document for #432 Issue: #432 User-Visible: no --- docs/reviews/SPEC-REVIEW-432-r1.md | 194 +++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-432-r1.md diff --git a/docs/reviews/SPEC-REVIEW-432-r1.md b/docs/reviews/SPEC-REVIEW-432-r1.md new file mode 100644 index 00000000..eedbab8d --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-432-r1.md @@ -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 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `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 + ```