From 4906f0a97ffbc95fe639eba2dc1f87fd58c17918 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 12 Sep 2026 03:40:45 +0000 Subject: [PATCH] docs: review document for #536 Issue: #536 User-Visible: no --- docs/reviews/SPEC-REVIEW-536-r1.md | 247 +++++++++++++++++++++++++++++ 1 file changed, 247 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-536-r1.md diff --git a/docs/reviews/SPEC-REVIEW-536-r1.md b/docs/reviews/SPEC-REVIEW-536-r1.md new file mode 100644 index 00000000..119e54c9 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-536-r1.md @@ -0,0 +1,247 @@ +# SPEC-REVIEW-536-r1 + +**Issue:** #536 — «Плашка несовпадения версий не гарантирует свой снос: `disconnect()` обнуляет `_banner` без `hooks.changed()`» +**Этап:** S4-spec-review (ревью ТЗ, PROCESS.md §2.4) +**Трек:** лёгкий (`small`), лимит циклов ревью ТЗ — 2 +**Заход:** r1 · блокирующих циклов израсходовано 0 из 2 +**Ревьюер:** Claude (Sonnet 5), роль «ревьюер ТЗ» +**Автор ТЗ:** Claude (класс A по прямому указанию владельца, отмечено в теле issue) + +## Материал раунда + +- Тело issue #536, раздел `## ТЗ` (Продуктовая рамка, Контракт К1–К3, таблица + AC1–AC3, Риски, Затронутые файлы, Откат), плюс предшествующий ему разбор + («Симптом», «Где именно», «Замер», «Насколько это видно пользователю»). +- SHA256 тела issue, как получено через `gh issue view 536 --json body -q .body`: + `42fc4ceae09539fe52bd339ae55bb4812367a7a996e4a417165481c259b028af` +- Единственный комментарий issue («Аналитика и ТЗ — на ревью», S1→S4 одним ходом). +- Код на момент ревью: `dev` @ `05a2c68da23b890de049d8507dd41c1ba2c9910e` (рабочая + копия чиста, HEAD == origin/dev). Читался только для проверки выполнимости и + однозначности ТЗ — продуктовый код не менялся, правки нет ни одной. + +## Скоуп ревью + +Обязательные для лёгкого трека разделы ТЗ (§5: проблема · контракт · AC1…ACn с +доказательством · откат, плюс два продуктовых раздела §7.1), однозначность и +проверяемость AC1–AC3, наличие «чем краснеет» для защитных AC (§2.7), отсутствие +невыделенных догадок о поведении, соответствие текущей реализации +`src/version-recovery.ts` и `src/version-recovery-card.ts`, корректность +классификации лёгкого трека и `User-Visible: no`. + +## Как проверялось + +1. Прочитан `docs/SCOPE.md` — задача не вводит новую функциональность и не + расширяет ни один Core user job; это исправление надёжности уже существующего + механизма (плашка версии — служебное уведомление, привязанное к J6 «Keep the + plan true» лишь косвенно). Конфликтов со скоупом нет. +2. Прочитан `AGENTS.md` (классы файлов, лёгкий трек — умолчание) и `PROCESS.md` + §1, §2.2–2.4 (лёгкий трек одним ходом S1→S4, что законно и явно объяснено + автором в комментарии), §4 (бюджет циклов, лёгкий трек — 2), §5 (критерии + лёгкого трека), §7.1 (обязательные разделы), §7.2 (формат вердикта). +3. Прочитан текущий `src/version-recovery.ts` целиком. Симптом подтверждён + построчно: `disconnect()` (строки 199–206) действительно обнуляет `_banner` + без вызова `this.hooks.changed()`, а `_hideBanner()` (строки 320–330) уже + пуст к моменту вызова после `disconnect()` и молча возвращается. Найденное в + issue «Замер» (последовательность `changed=1` четыре раза подряд, включая + `connect()` в конце) воспроизводится по коду вручную, а не принимается на + слово. +4. Прослежен вручную предложенный контракт К1–К3 на конкретной + последовательности из AC1 («несовпадение → disconnect → версии сошлись → + connect»), с учётом того, что плашка уже видна до начала последовательности + (это следует из «Замера» в issue, а не додумано): `_reconcile()` при + несовпадении зовёт `_showBanner` → `changed` #1; правленный `disconnect()` + при непустой `_banner` зовёт `changed` #2 и обнуляет поле; последующий + `update()` к состоянию `equal` вызывает `_reconcile()`, но `_connected` уже + `false`, и `_hideBanner()` рано выходит (поле уже `null`) — без нового + `changed`; `connect()` вызывает `_reconcile()` при `relation.kind !== 'mismatch'` + — снова ранний выход `_hideBanner()`, без нового `changed`. Итог: ровно два + вызова `changed`, плашка пуста — совпадает с формулировкой AC1 дословно. +5. Отдельно проверено АC2 логически: путь `disconnect()` без `_banner` не входит + в изменённую ветку (`if (this._banner) { … }`), лишнего `changed()` там нет ни + до, ни после правки — это тривиальное следствие формы правки, а не отдельный + риск. +6. Прочитан `src/version-recovery-card.ts` (строки 147–160) — `hooks.changed` + действительно равен `() => { host.requestUpdate(); }`, как и заявлено в ТЗ. +7. Проверена состоятельность риска №1 («`requestUpdate()` из + `disconnectedCallback` — путь не новый»), а не принята на слово: в + `src/houseplan-card.ts` переопределение `requestUpdate()` (строки 680–694) не + делает исключения для отключённого состояния — оно безусловно вызывает + `super.requestUpdate(...)`. В `disconnectedCallback()` самой карточки (строки + 2743–2751) `this._versionRecovery.disconnect()` стоит **первой** строкой, + перед `_clearRoomFocus(true)` и `_cancelDangerConfirm()`, а `super.disconnectedCallback()` + зовётся в конце метода — стандартный для этого файла паттерн (тот же порядок + в `space-card.ts`, `hp-dialog.ts`, `hp-help.ts`, `hp-color-opacity.ts`). + Довод автора подтверждён чтением, а не верой в комментарий issue. +8. Прочитан `test/version-recovery.test.mjs` (строки 1–71) — инфраструктура для + AC1/AC2 уже существует: `harness()` собирает `events.push({ kind: 'changed' })` + при каждом вызове `hooks.changed`, так что подсчёт «два вызова, а не один» — + не гипотетический план, а тривиальное расширение уже работающего каркаса. +9. Проверено `scripts/mutation-gate.mjs` на предмет коллизии имени: существующие + мутанты для этого файла (`version-recovery-treats-unknown-as-mismatch`, + `version-recovery-auto-reloads-ordinary-view` и другие, строки 890–970) не + пересекаются с предложенным `version-banner-disconnect-silent` — новое имя + свободно. +10. Проверено `docs/USER-GUIDE.ru.md` и весь `docs/` на упоминания плашки + версии/`version-recovery` — терминология нигде не зафиксирована как + пользовательская (это внутренний механизм восстановления, не описанный в + гайде), поэтому вопрос словаря интерфейса не встаёт. +11. Оценена корректность `User-Visible: no`: в худшем случае непочиненный дефект + даёт как раз наблюдаемый артефакт (зависшая плашка) — это отмечено + автором честно, а не скрыто. Довод «сегодня корректность держится на + 98%-совпадении, а не на контракте» и «в браузерной пробе всегда чинится + посторонней перерисовкой» проверяемы только через существующий смок + панели — на этапе ревью ТЗ смок не гонялся (кода ещё нет), поэтому запись + ниже помечена как решение ревьюера по существу, а не как перепроверенный + факт. + +Гейты (typecheck/test/build/golden/backend) не гонялись — это этап ревью ТЗ, +продуктовый код ещё не написан; §2.4 их не требует. + +## Разбор по существу + +### Обязательные разделы (лёгкий трек, §5 + два продуктовых раздела §7.1) + +| Раздел | Есть? | Где | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | «Продуктовая рамка»: «любая персона, кто видит плашку» — оправданно широко, плашка не персонифицирована ни в одном документе | +| Что человек увидит до/после | ⚠️ формально | «До/После» присутствует, но использует термин реализации «контроллер решил её снять» — см. Low №1 | +| Проблема | ✅ | «Симптом» + «Где именно» + «Замер» до раздела `## ТЗ` | +| Контракт поведения | ✅ | К1–К3, дословно привязан к текущему коду | +| AC1…ACn с доказательством и «чем краснеет» | ✅ | Таблица AC1–AC3, у каждого юнит-тест и мутант/контрпроба | +| Риски | ✅ | 2 пункта, оба по существу (см. проверку риска №1 выше) | +| Откат | ✅ | «Ревёрт одной строки» — соответствует реальному размеру правки | +| Скоуп/не-скоуп | ⚠️ неявно | К2 «Больше ничего не меняется» плюс список затронутых файлов пинует скоуп к одному модулю, но отдельного заголовка «не-скоуп» нет — см. Low №2 | +| Миграция/i18n/perf/touch: явное «нет» | ⚠️ подразумевается | Не выписано отдельными строками, следует из перечисления критериев трека — см. Low №2 | + +### Техническая состоятельность К1–К3 и AC1–AC3 + +Разобрано построчно в шагах 3–7 «Как проверялось» выше. Контракт реализуем +буквально: правка — оборачивание существующего `if (this._banner) this._banner = null;` +в блок, добавляющий `this.hooks.changed()` до или после обнуления. АС1 +арифметически подтверждён вручную (два вызова `changed`, а не один или три); +АС2 — тривиальное следствие формы правки; АС3 — регресс на неизменённых тестах, +корректный выбор для «ничего больше не меняется». + +Риск №1 (безопасность `requestUpdate()` из `disconnectedCallback`) — не догадка +автора, а подтверждённый чтением существующий паттерн в этом же файле +(`houseplan-card.ts:2743`), что снимает главный источник тревоги: не откроет +ли фикс новый путь падения при демонтаже карточки. Риск корректно передаёт +дальнейшую проверку («существующий смок панели и юниты карточки») ревью кода — +это стадия, где это можно доказать исполнением, а не чтением. + +### Классификация трека и `User-Visible: no` + +Оба вопроса автор прямо адресовал ревьюеру, а не владельцу — это правильная +маршрутизация: ни один из них не о том, что видит или делает персона, оба +решаются встроенным сюда разбором. + +- **Лёгкий трек.** Все критерии §5 выполнены одновременно и ни один не нарушен: + одна поверхность (`src/version-recovery.ts`), сложность и риск — реально 2 (это + подтверждает и объём настоящего разбора: вся логика прослеживается вручную за + один проход), миграции конфига нет, нового UX-контракта нет (внешнее поведение + не меняется ни в одном уже покрытом тестами сценарии — AC3 это фиксирует), + влияния на touch/perf нет. **Подтверждаю классификацию.** +- **`User-Visible: no`.** Довод автора состоятелен: сегодня корректность снятия + плашки после реаттача держится на том, что в реальном приложении между + disconnect и следующим кадром почти всегда происходит посторонняя + перерисовка (`_maybeRebuildDevices` и другие), а не на гарантии контракта. + Правка не меняет ни одного уже наблюдавшегося поведения (AC3), а закрывает + единственный путь, который по признанию самого автора не удалось пронаблюдать + вживую. Записывать в changelog нечего показать пользователю — там нет «было + Х, стало Y» с наблюдаемой разницей. **Подтверждаю `User-Visible: no`.** + +### Проверка «не выдана ли догадка за решение» + +- «Замысел прежней редакции сохраняется дословно» (обоснование, зачем `_banner` + вообще обнуляется в `disconnect()`) — не догадка, а прямое чтение + существующего кода и комментария к нему; К2 явно фиксирует это как то, что + не трогается. +- Утверждение «в браузерной пробе не видно — 11 посторонних перерисовок» — + заявлено как результат конкретного замера, не как общее свойство системы; + автор честно помечает, что корректность «держится на совпадении», а не + утверждает, что дефекта нет вообще. +- Риск №1 подан как риск с указанием, чем закрывается (существующий путь плюс + смок/юниты), не как решённый факт — и подтверждён теперь ещё и чтением + ревьюера. + +Непомеченных догадок о поведении, которого нет ни в одном документе, не найдено. + +## Находки + +Блокирующих (High) находок нет. Находок Medium — в скоупе или вне скоупа — нет. + +### Low (сняты решением ревьюера, с записью, без возврата на цикл) + +1. **«До/После» использует термин реализации.** Формулировка «уходит тогда, + когда контроллер решил её снять» называет внутреннюю сущность + (`VersionRecoveryController`) вместо чисто наблюдаемой фразы. По содержанию + вреда нет: правка не меняет наблюдаемого поведения ни в одном + протестированном сценарии (сама пара «До/После» и заявляет «внешне то же + самое»), поэтому смысловой ошибки, которая исказила бы приёмку, здесь нет. + Снимаю без возврата — переписывать одну фразу ради буквы правила не стоит + цикла ревью на лёгком треке (лимит 2). +2. **Не-скоуп, миграция/i18n/perf/touch не выписаны отдельными строками.** Всё + это выводится однозначно: список «Затронутые файлы» называет ровно один + модуль плюс тест и реестр мутантов, а обоснование лёгкого трека в конце ТЗ + перечисляет «нет миграции, нет UX-контракта, нет влияния на перф и touch» — + содержательно то же самое, что отдельные строки DoR-чек-листа (§2.5) + потребовали бы дословно. Снимаю с записью: автор или разработчик могут + дописать явные строки при переводе в `S5-ready` без нового цикла ревью ТЗ — + аналогично прецеденту SPEC-REVIEW-535-r1, Low №2. + +## Что проверено и корректно + +- Симптом воспроизведён чтением текущего `src/version-recovery.ts`: `disconnect()` + обнуляет `_banner` без `hooks.changed()`, `_hideBanner()` после этого — пустой + ранний выход. Соответствует «Замеру» в issue. +- К1–К3 реализуемы буквально текущим кодом; АС1 арифметически проверен вручную + (ровно два вызова `changed`, плашка пуста в конце последовательности); АС2 и + АС3 — корректные, проверяемые утверждения с названным способом доказательства + и «чем краснеет». +- Риск №1 (безопасность `requestUpdate()` из `disconnectedCallback`) подтверждён + независимым чтением `houseplan-card.ts` (переопределение `requestUpdate()` не + делает исключения для отключённого состояния; вызов `_versionRecovery.disconnect()` + стоит первой строкой `disconnectedCallback()`, `super.disconnectedCallback()` — + последней, как и в остальных компонентах этого репозитория). +- Тестовая инфраструктура для AC1/AC2 уже существует (`harness()` считает + события `changed`) — план автотестов не вымышленный. +- Имя предлагаемого мутанта (`version-banner-disconnect-silent`) не пересекается + с существующим реестром `scripts/mutation-gate.mjs`. +- Классификация лёгкого трека и `User-Visible: no` — оба вопроса, адресованные + ревьюеру автором, разобраны по существу и подтверждены, решение принято здесь, + а не вынесено владельцу (это не продуктовый вопрос «что видит/делает персона»). +- Непомеченных догадок о недокументированном поведении не найдено. +- Терминология плашки версий нигде не зафиксирована в `docs/USER-GUIDE.ru.md`, + поэтому вопрос словаря интерфейса не встаёт. + +## Чего не проверял + +- Гейты (`typecheck`/`test`/`build`/golden/backend) — не требуются на этапе + ревью ТЗ (§2.4), кода ещё нет. +- Реальный прогон юнит-теста и мутанта `version-banner-disconnect-silent` — они + ещё не написаны; это предмет код-ревью. +- Существующий смок панели и юниты карточки на предмет «отсоединённый рендер не + падает» (риск №1) — упомянуты автором как свидетель, но не перезапускались + мной; это стадия код-ревью, где можно доказать исполнением. +- Реальное поведение в браузере (11 посторонних перерисовок и т.п.) — измерение + автора, не переизмерял. + +## Вердикт + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/2 · High: 0 · Medium: 0 → в задаче + +Issue #536 переходит в `S5-ready`. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `05a2c68da23b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `d334191242147a177483837271e9462955b4807e` + ``` + git log --all --format='%H %T' | grep d33419124214 + ``` +- Тело issue: `fdc4c4f20bae1574a65b8c8bac06c43f4ecdab6d3394149baea09c34bec75cc4` +- Вердикт конвейера: `green` · High 0