docs: review document for #536

Issue: #536
User-Visible: no

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`.
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `dev`, коммит `05a2c68da23b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `d334191242147a177483837271e9462955b4807e`
+  ```
+  git log --all --format='%H %T' | grep d33419124214
+  ```
+- Тело issue: `fdc4c4f20bae1574a65b8c8bac06c43f4ecdab6d3394149baea09c34bec75cc4`
+- Вердикт конвейера: `green` · High 0