From 4a91d5cbe88d6868947f2d7f73c9830869478b56 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 12 Sep 2026 18:51:40 +0000 Subject: [PATCH] docs: review document for #543 Issue: #543 User-Visible: no --- docs/reviews/SPEC-REVIEW-543-r1.md | 189 +++++++++++++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-543-r1.md diff --git a/docs/reviews/SPEC-REVIEW-543-r1.md b/docs/reviews/SPEC-REVIEW-543-r1.md new file mode 100644 index 00000000..5551e0fc --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-543-r1.md @@ -0,0 +1,189 @@ +# SPEC-REVIEW-543-r1 + +Issue: #543 · Этап: spec (PROCESS.md §2.4) · Заход: r1 · блокирующих циклов 0/4 + +Материал: тело issue #543, раздел `## ТЗ`, на момент ревью (метка `S4-spec-review`, +два предыдущих комментария — S2-аналитика и передача ТЗ на ревью, оба того же +автора, без предыдущих раундов ревью). Это первый заход, разбор полный, без +раздела «Унаследовано». + +Продуктовая постановка (аудиторский блок `## Пользовательская проблема` … +`## Состояние при заведении`) и `## ТЗ` — согласованы: ТЗ прямо цитирует те же +строки кода и тот же механизм (`_reloadConfigOnly` / `adoptAuthoritativeGated`), +без расхождений. + +## Скоуп ревью + +Проверялось: полнота обязательных разделов §7.1, однозначность и доказуемость +каждого AC, соответствие продуктовой рамке `docs/SCOPE.md` (J1 «актуальное +состояние», J6 «согласованность нескольких клиентов»), фактическая проверка +технических утверждений ТЗ по коду на точном SHA `6d2facc2e37724a6e6e1a96c5e026aa24b595a07` +(рабочая копия репозитория стоит на этом коммите — сверено `git rev-parse HEAD`). + +Продуктовых вопросов к владельцу нет и не требовалось: сценарий и видимое +поведение зафиксированы однозначно («До/После»), а всё техническое оформлено +отдельным блоком «Принятые технические предположения — можно менять на +ревью». Технического спора с автором, требующего отдельного цикла из-за +разногласия, нет — единственная находка ниже касается точности ссылки на +тестовый артефакт, а не архитектуры решения. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.4/§2.9/§7.1/§7.2/§4. +2. Прочитано тело issue #543 целиком, включая исходный аудиторский блок + (источник, воспроизведение, причина по коду, ожидаемый результат) и оба + комментария (S2-аналитика с приоритетом P2 и передача ТЗ на ревью). +3. Прочитан текущий код на названном SHA и построчно сверен с ключевыми + техническими утверждениями ТЗ: + - `src/houseplan-card.ts:4417,4466-4501` — `_reloadConfigOnly`: `observedRev` + сравнивается с `_cfgRev` только на входе (`:4473`), дальше идёт + `await this._getAuthoritativeConfig()` и `await this._adoptAuthoritative(...)` + без повторной проверки владения после первого await — подтверждает + «Подтверждённую причину» ТЗ дословно; + - `src/config-adoption.ts:428-460` — `adoptAuthoritativeGated`: `await + host._signer.prepareImage(...)` (`:439`) идёт до `adoptStructuralResponses` + (`:451`) без промежуточной проверки заявки — подтверждает К2 буквально; + - все шесть вызывающих `_adoptAuthoritative`/`adoptAuthoritativeGated` мест + (`houseplan-card.ts:4306,4485`, `summary-panel-runtime-loaded.ts:630`, + `houseplan-onboarding-runtime.ts:477`, `houseplan-editor-runtime.ts:8693,9577,9737`) + уже проверяют `status !== 'adopted'`, то есть новый статус `superseded` + естественно поглощается существующей веткой без правки самих call sites — + подтверждает заявление ТЗ «менять call sites только если явная обработка + нужна»; + - все `_reloadConfigOnly(true)` call sites (`houseplan-card.ts:5267,7933`, + `summary-panel-runtime-loaded.ts:671`, `houseplan-onboarding-runtime.ts:287,410,497`, + `space-copy-runtime.ts:224,248`, `houseplan-editor-runtime.ts:1842,8623,8703,9552`) — + подтверждает, что «force/restore» путей на практике много, К7 не + придумана под единственный случай; + - `docs/ARCHITECTURE.md:1375-1393` (#500) — «Config/layout identity has one + owner», единая точка `adoptAuthoritativeGated` — центр, в котором ТЗ + предлагает разместить общую проверку заявки, соответствует уже + установленному инварианту, а не вводит второй путь принятия; + - прецедент generation-guard уже существует в кодовой базе: + `_liveSyncGeneration`/`_liveSyncConnection` (`houseplan-card.ts:948-949, + 4376-4405`) — подтверждает, что предлагаемый механизм claim/generation не + изобретение с нуля, а расширение уже принятого паттерна; + - «route leave» — установленный в коде термин (`houseplan-card.ts:2059,2752, + 7447`), а не придуманный ТЗ; + - `test/config-adoption-ownership.test.mjs` — прочитан целиком: это + статический regex-сканер по `src/**` (`IDENTITY_WRITE`, `BODY_WRITE`), + ратчет-тест другого инварианта #500 («только `ConfigAdoption` пишет + `_cfgRev`/`_layoutRev`/fingerprint»), см. находку ниже; + - `demo/smoke_save_race.mjs` — образец существующего паттерна: смок вызывает + приватный `c._reloadConfigOnly()` напрямую через `page.evaluate` и + подменяет `hass.callWS`; подтверждает техническую реализуемость AC2/AC4 + без новых инструментов; + - `package.json` — `gate:small`, `invariants`, `docs:accept`, `golden:verify` + существуют как названо в плане автотестов; + - `demo/smoke_plan_upload_race.mjs`, `smoke_save_race.mjs`, + `smoke_ws_resilience.mjs`, `smoke_version_recovery.mjs` — существуют, как + названо в плане автотестов п.6. +4. Продуктового кода к задаче ещё нет (этап — ревью ТЗ), поэтому гейты + typecheck/test/build/golden не прогонялись и не требовались. + +## Находки + +### Medium (в скоупе задачи) — двусмысленная ссылка на тестовый артефакт в AC7 и списке файлов + +`AC7` и раздел «Ожидаемые файлы и модули» называют `test/config-adoption-ownership.test.mjs` +как один из свидетелей порядка `compare → asset readiness → continuity → adopt +→ reload tail → caller hook`. Этот файл уже существует и уже документирован +(`docs/ARCHITECTURE.md:1375-1393`, докстринг файла) как нечто другое: чисто +статический regex-сканер исходников, доказывающий не поведенческий порядок +операций, а то, что `_cfgRev`/`_layoutRev`/fingerprint пишет только +`ConfigAdoption` (ратчет-инвариант #500, счётчики `BODY_STAGING_ALLOWLIST` +могут только уменьшаться). Он не выполняет ни одного `_reloadConfigOnly`, +не мокает `prepareImage`, не видит async-порядка вообще — механически не +способен подтвердить то, что ему приписывает AC7. + +**Сценарий проявления:** разработчик, ориентируясь на прямое упоминание в +ТЗ, либо (а) добавляет поведенческие async-тесты заявки/generation в этот +файл, ломая его текущую единственную задачу и докстринг («one owner for +config/layout identity»), либо (b) недоумевает и оставляет AC7 доказанным +только `test/config-adoption.test.mjs`, а «ownership tests» в ревью кода +считается недоказанным пунктом AC. Оба исхода — трение, которого не было бы +при точной ссылке. + +Это не блокирует ТЗ целиком: основной свидетель AC7 — +`test/config-adoption.test.mjs`, который действительно предназначен для +поведенческих проверок (план автотестов п.1, deferred-promise матрица) и +способен нести эту нагрузку в одиночку. Также не пострадала ни одна связь с +`docs/CONFIG-COMPATIBILITY.md` или другим каноном — это чисто internal +naming/reference вопрос. + +**Правка:** в AC7 и в «Ожидаемых файлах» либо явно развести два разных смысла +слова «ownership» (например, назвать новый файл/describe-блок иначе — `test/config-adoption-attempt.test.mjs` +или новый describe в `config-adoption.test.mjs`), либо прямо указать, что +`config-adoption-ownership.test.mjs` в этой задаче не трогается и упомянут по +ошибке. Без правки автор может честно закрыть AC7 текстом «доказано +`config-adoption.test.mjs`», но текущая формулировка ТЗ вводит в заблуждение. + +## Проверка обязательных разделов (PROCESS.md §7.1) + +Все обязательные разделы на месте и содержательны: сценарий · что человек +увидит до/после · проблема (аудиторский блок) · скоуп и не-скоуп · контракт +поведения (К1–К9) · UX (пусто, обоснованно) · модель данных и миграция (нет +изменений схемы, обоснованно) · i18n (нет) · критерии приёмки AC1–AC9 с +доказательством (`unit`/`smoke`/`lifecycle matrix`/`force/reset`/`adoption +regression`/`mutation`/`gates`) · план автотестов · риски · откат · +release-артефакты. + +- **Сценарий/видимое поведение** — персона-центричны (SCOPE J1/J6), не + используют внутренних терминов реализации сверх необходимого; «claim», + «generation», «high-water» вводятся только в контракте (К1–К9), не в + сценарии. +- **AC однозначны и проверяемы**, у AC1/AC7/AC8 явно назван отрицательный + свидетель («тест умеет падать» — уже в самом ТЗ, а не отложено на код-ревью). + AC2/AC5 требуют реального host/lifecycle-прогона, а не последовательного + unit-теста — соответствует явному требованию «Ожидаемого результата» из + исходного аудиторского блока issue. +- **Технические предположения** оформлены отдельным блоком «можно менять на + ревью», 5 пунктов, каждый — реальное техническое решение (кто выигрывает + гонку, что считается новым baseline, поведение `superseded`, судьба + signer-кэша просроченной попытки, исключение layout reload), ни один не + маскирует продуктовый вопрос. +- **Скоуп/не-скоуп** согласован с явным исключением протокола backend/WS, + глобальной сериализации reload и правила «никогда не принимать меньшую + revision» — совпадает с «Ожидаемым результатом» аудита («не вводить + безусловный вечный запрет»). +- **Риски** — шесть пунктов, каждый называет конкретный способ, которым + наивная реализация могла бы сломать defense (не декоративный список). +- **Полный трек оправдан явно**: названы нарушенные критерии `small` + (multi-client state contract, несколько lifecycle-границ, асинхронный asset + gate, риск для continuity/cache) — соответствует §5. + +## Что не проверялось и почему + +- Гейты `typecheck`/`test`/`build`/`golden`/`invariants` — не прогонялись: + продуктового кода ещё нет, предмет этапа — текст ТЗ. +- Реализация `request-owner/coordinator` модуля, будущего мутанта AC8 и + содержимого `demo/smoke_config_reload_race.mjs` — не существуют на этом + этапе, их проверит код-ревью. +- Частота гонки на реальном HA в проде — сам аудит и ТЗ прямо признают это + неизмеренным; для приоритета P2 и полного трека этого достаточно, отдельно + не перепроверялось. +- Совместимость с бэкендом/WS-протоколом — вне скоупа задачи по её же + формулировке, не проверялась. + +## Вердикт + +Жёлтый: единственная находка — Medium в скоупе задачи (двусмысленная ссылка +на `test/config-adoption-ownership.test.mjs` в AC7 и списке файлов). High нет. +Технический контракт (К1–К9), причина по коду и большинство AC подтверждены +прямым чтением кода на названном SHA, а не приняты на веру. Автор правит +ссылку на тестовый артефакт в теле issue, изменение проходит повторный цикл +ревью ТЗ по дельте (PROCESS.md §2.10). + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `6d2facc2e377` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `bd3acd278b0c6dd3d0cc572096f8c0d2c7e3b38c` + ``` + git log --all --format='%H %T' | grep bd3acd278b0c + ``` +- Тело issue: `06cae6bc0c1c834e055a858a18d406231ff7a696f929be70277a82a675f6779a` +- Вердикт конвейера: `yellow` · High 0