houseplan-card/docs/reviews/SPEC-REVIEW-536-r1.md

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