docs: review document for #403

Issue: #403
User-Visible: no

docs/reviews/SPEC-REVIEW-403-r1.md

@@ -0,0 +1,264 @@
+# SPEC-REVIEW-403-r1
+
+- Issue: https://github.com/Matysh/houseplan-card/issues/403
+- Артефакт ТЗ: `docs/specs/403-area-relocation-safety.md` (полный трек, класс A;
+  без метки `small`/`trivial`)
+- Материал: SHA `1f9d9014` (ветка `issue/403-area-relocation-safety`,
+  коммит «docs: specify area relocation safety (#403)»)
+- Заход: r1 · лимит циклов ревью ТЗ для полного трека — 4 (§4), израсходовано
+  до этого раунда — 0 (пять предыдущих запусков конвейера падали до первого
+  обращения к модели — `is_error: true`, `modelUsage: {}` — вердикта не было
+  ни разу, бюджет не тратили, см. комментарии issue)
+- Ревьюер: Claude (роль «ревьюер ТЗ»), независимая сессия, без устных
+  пояснений автора
+- Первый раунд — разбор полный, раздела «дельта» нет (§2.10 применяется
+  начиная со второго захода)
+
+## Скоуп ревью
+
+Bug P1, класс A, полный трек (issue не помечен `small`): две находки
+свежего кода #126 на одной поверхности (`src/houseplan-card.ts`,
+`_syncAreaRelocations` и обработка отказов):
+
+- **C2 (High из аудита)** — отказ `houseplan/config/set` во время переезда
+  area оставляет `layout` уже удалённым (удаление успело пройти раньше) и
+  не восстанавливает позицию и не помечает устройство как требующее
+  внимания — ручная расстановка маркера теряется молча, самовоспроизводяще
+  (снапшот откатывается на старую area → следующий authoritative-проход
+  снова решает `relocate`).
+- **M1 (Medium из аудита)** — переезд area **любого** устройства чистит
+  **весь** стек Undo позиций (`_devicePositionHistory.clear()`), включая
+  записи устройств, которых переезд не касался, и без уведомления (в
+  отличие от соседнего класса очистки истории — `history.device_stale`).
+
+Задача ложится на J6 `docs/SCOPE.md` («Keep the plan true as the home
+evolves» — оптимистичная блокировка, ручная расстановка маркеров) и на
+стоящее правило SCOPE.md «никогда не удалять данные пользователя по
+догадке» — обе находки именно про это: ручная позиция маркера — данные,
+введённые пользователем руками.
+
+## Как проверялось
+
+1. Прочитаны целиком `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1, §2.4,
+   §2.5, §4, §5, §6, §7.1, §7.2, §8), тело issue #403 и все восемь
+   комментариев (включая последовательность из шести неудачных запусков
+   конвейера — учтено при подсчёте бюджета циклов, ни один из них вердикта
+   не дал).
+2. Прочитан весь текст ТЗ `docs/specs/403-area-relocation-safety.md`.
+3. Каждое фактическое утверждение ТЗ о коде сверено построчно с деревом на
+   `1f9d9014`:
+   - `_maybeRebuildDevices`/`_syncAreaRelocations`
+     (`src/houseplan-card.ts:5016-5254`) прочитаны целиком;
+   - подтверждено: ветка отказа **удаления** layout восстанавливает позицию
+     (`applyDevicePlacement(before)`, фактическая строка `:5184`, ТЗ называет
+     `:5183` — расхождение в одну строку, не искажает факт);
+   - подтверждено: ветка отказа **записи конфига** (`catch` на `:5223-5248`,
+     ТЗ называет тот же диапазон точно) восстанавливает только
+     `marker_area_snapshot`/`new_device_ids` в памяти (если фингерпринт не
+     разошёлся) и **не восстанавливает** удалённую позицию — заявление ТЗ
+     подтверждено буквально, строка в строку;
+   - подтверждён механизм самовоспроизведения: `resolveDeviceAreaRelocations`
+     (`src/device-area-relocation.ts:181-188`) решает `relocate = true`
+     ровно когда `previous.area !== area`; после откатa снапшота на старую
+     area это условие снова истинно на следующем authoritative-проходе —
+     цикл, описанный в ТЗ, воспроизводится логикой резолвера, а не является
+     догадкой;
+   - подтверждена находка M1: `_devicePositionHistory.clear()`
+     (`src/houseplan-card.ts:5062-5065` — ТЗ называет `:5056-5058`,
+     расхождение в 6 строк, см. находку L1) стоит под условием «есть хоть
+     одно переезжающее устройство», без фильтра по `deviceId`;
+   - `CommandStack` (`src/command-stack.ts`) не имеет метода выборочного
+     удаления — подтверждено, что контракт AC5/AC6 («снимается история
+     переехавших, а не весь стек») требует нового метода, но это техническая
+     деталь реализации, а не пробел ТЗ (стек типизирован по
+     `NamedCommand<DevicePositionState>`, `deviceId` уже есть в каждой
+     записи — технически осуществимо без изменения формата данных);
+   - `toast.pos_save_failed`/`toast.cfg_save_failed`/`toast.conflict`/
+     `history.device_stale` — все четыре ключа существуют в `src/i18n/ru.json`
+     (и en/de/fr) — заявления ТЗ о «существующей метке» и «существующем
+     уведомлении» подтверждены, не придуманы;
+   - `registryFollowingBinding`/формат `binding` (`device-area-relocation.ts:95-107`)
+     — подтверждён формат `${bindingKind}:${bindingRef}`, ровно то, что автор
+     сам называет причиной трёх неудачных попыток воспроизведения C2 до
+     финального успешного прогона (комментарии аналитики) — воспроизведение
+     не голословно, ошибка автора зафиксирована и исправлена явно.
+4. Проверено соответствие терминологии `docs/USER-GUIDE.ru.md`: раздел
+   «Устройства» (`:757-771`) документирует именно тот успешный путь, который
+   AC2 требует сохранить («старая позиция удаляется… появляется красная
+   отметка внимания»), и раздел «История редактора» (`:260`, `:808-809`)
+   подтверждает существующий контракт Undo (50 команд, best effort на touch) —
+   спецификация не вводит новых терминов и не противоречит гайду.
+5. Проверено `docs/TOUCH-SUPPORT.md` и DoR-чек-лист §2.5 на обязательные
+   пункты «влияние на touch» и «влияние на производительность» — см. находку
+   H1.
+6. Проверены обязательные разделы §7.1 — присутствуют все (сценарий · что
+   человек увидит до/после · проблема и контракт по каждому пункту · скоуп/
+   не-скоуп · UX · модель данных и миграция · i18n · AC1–AC7 с доказательством ·
+   план автотестов · риски · откат · release-артефакты).
+7. Проверено существование инструментов, на которые ссылается план тестов:
+   `scripts/mutation-gate.mjs` есть, ни одного мутанта `area-relocation-*` в
+   нём пока нет (согласуется с ТЗ — это новые мутанты); `demo/smoke_area_relocation.mjs`
+   существует и уже умеет мокать отказ `houseplan/config/set`
+   (`rejectKettleRelocation`, строки 19/262/279) — план AC1/AC4 технически
+   реализуем на существующей инфраструктуре смоков, не является фантазией.
+8. Гейты кода (`tsc`, `test`, `build`) не гонялись: на этапе ТЗ продуктового
+   диффа нет (класс C — только `docs/specs/**`), гонять их не над чем.
+
+## Находки
+
+### H1 (High, блокирует, в скоупе). ТЗ не называет два обязательных пункта DoR — влияние на touch/kiosk и на производительность
+
+**Где**: весь файл `docs/specs/403-area-relocation-safety.md` — ни разу не
+упоминает touch, kiosk, `TOUCH-SUPPORT.md`, производительность, перф или
+бюджет (`grep -i "touch|kiosk|перф|производительн|performance"` по файлу —
+ноль совпадений).
+
+**Почему это находка, а не формальность**. PROCESS.md §2.5 перечисляет
+обязательные пункты «Готово к разработке» и требует по каждому явную
+запись, а не молчание: «влияние на производительность и бюджеты названо
+(**или явно «нет»**)» и «влияние на touch по `docs/TOUCH-SUPPORT.md`
+(**View и киоск — блокирующие**)». Оба пункта — из списка, помеченного
+«Все пункты обязательны», и: «Если хоть один пункт не выполнен — статус не
+«Готово к разработке», как бы ни хотелось начать». Ни один из двух пунктов
+в ТЗ не назван — ни утвердительно, ни отрицательно.
+
+Это не абстрактная бумажная претензия: у задачи есть настоящая View/kiosk
+грань. AC2 фиксирует видимый на любом клиенте (включая киоск-планшет — J1
+`docs/SCOPE.md`, «Show the whole home … device states») эффект успешного
+переезда — маркер оказывается в новой комнате с отметкой внимания; это
+именно то поведение, что описано в `docs/USER-GUIDE.ru.md:761-767`.
+Симметрично, дефект C2 в необработанном виде **тоже виден на киоске** —
+маркер молча возвращается в центр комнаты. То есть эта ветка кода реально
+затрагивает View/kiosk-наблюдаемое поведение, а не только редакторский
+слой, и именно поэтому DoR требует явного заявления, а не тишины.
+AC5/AC6 (сужение очистки Undo) относятся к редактору устройств, для
+которого touch уже документирован как best effort (`USER-GUIDE.ru.md:260`,
+`:808-809`) — но это тоже должно быть **названо**, а не молчаливо
+унаследовано: без явной строки нельзя отличить «автор сверился с
+TOUCH-SUPPORT.md и решил, что контракт не меняется» от «автор не думал про
+touch вовсе» (тот же аргумент, которым в SPEC-REVIEW-402-r1 было обосновано
+идентичное H1-заключение для #402 — прецедент этого же ревьюера на этом же
+проекте).
+
+Фактическая оценка по существу (для экономии цикла): последствий, скорее
+всего, нет ни для touch, ни для перфа. Обе правки — (1) порядок операций в
+одном async-методе `_syncAreaRelocations` плюс восстановление/повторная
+попытка записи при отказе, (2) точечный фильтр по `deviceId` в
+уже существующей структуре истории на 50 записей. Ни жесты, ни рендер, ни
+DOM, ни сетевые вызовы сверх уже выполняемых не меняются. Но это вывод
+ревьюера, а не факт, зафиксированный автором в ТЗ, — фиксировать обязан
+автор.
+
+**Требуемая правка** (несколько строк текста, не кода): добавить в ТЗ,
+например —
+- `Touch: не затронут — правка меняет порядок серверной записи
+  (_syncAreaRelocations) и фильтр очистки Undo-стека по deviceId, не
+  касается drag/tap-жестов, рендера или DOM; наследует существующий
+  контракт Undo/Redo (best effort на touch, USER-GUIDE §10). View/kiosk:
+  наблюдаемый эффект (AC1/AC2) — позиционный, не входной, контракта View на
+  touch не меняет.`
+- `Производительность: нет — правка не добавляет новых циклов, подписок или
+  сетевых вызовов сверх уже выполняемых `_syncAreaRelocations`/`_writeConfig`;
+  фильтр истории работает на существующем массиве максимум 50 записей.`
+
+### L1 (Low, снимается с записью). Номер строк для сниппета M1 отстал от кода на SHA `1f9d9014`
+
+**Где**: `docs/specs/403-area-relocation-safety.md`, раздел «(2) M1»:
+«`src/houseplan-card.ts:5056-5058`».
+
+**Проверено чтением**: на `1f9d9014` этот диапазон (`:5056-5058`) — три
+строки середины вызова `resolveDeviceAreaRelocations` (`model:`, `layout:`,
+`snapshot:` — параметры объекта опций), не имеющие отношения к M1. Сам
+процитированный в ТЗ сниппет (`this._areaRelocationIds = new
+Set(...); if (...) { this._cancelDeviceDrag(); this._devicePositionHistory.clear();`)
+дословно совпадает с кодом, но находится на строках `:5062-5065`.
+Содержание находки верно и не пострадало (сверено выше, в «Как
+проверялось», п.3), только адрес неточен — вероятно, из-за смещения при
+правках между тем, когда аналитика собирала цитату, и фиксацией ТЗ.
+
+**Решение ревьюера**: не блокирует, не создаёт отдельного цикла. Снимаю с
+записью — исправить номера строк можно попутно при правке по H1 (тот же
+файл будет открыт на редактирование), отдельного возврата ради одной этой
+находки не требуется.
+
+## Что проверено и признано корректным
+
+- **Диагноз C2 точен и воспроизводим**: ветка отказа удаления восстанавливает
+  layout (`:5184`), ветка отказа записи конфига — нет (`:5223-5248`); порядок
+  «delete-first» (`:5147` комментарий «Layout deletion is deliberately
+  completed before provenance advances») подтверждён и корректно процитирован.
+  Самовоспроизводящийся цикл (снапшот откатывается → резолвер снова решает
+  `relocate`) подтверждён логикой `resolveDeviceAreaRelocations`
+  (`device-area-relocation.ts:181-188`), а не выдан за факт без опоры на код.
+- **Диагноз M1 точен**: `clear()` действительно безусловен по всему набору
+  переезжающих устройств, `history.device_stale` действительно существует как
+  образец уже принятого в проекте паттерна уведомления об очистке истории.
+- **Два допустимых исхода C2 (запись первой / восстановление при отказе)
+  сформулированы как решаемая ревьюером/автором техническая развилка**, а не
+  как догадка, выданная за факт — с явным критерием выбора (AC3, свойство
+  delete-first) и явной эскалацией в §"Риски", если восстановление тоже
+  отказывает («потеря неизбежна» → AC1 формулируется как «позиция ИЛИ
+  метка», не «позиция всегда»). Это корректное использование блока
+  «принято предположительно» из §7.1 AGENTS.md для чисто технических решений.
+- **AC1–AC7 однозначны и указывают способ доказательства** (браузерный смок,
+  для AC7 — уже существующий `demo/smoke_area_relocation.mjs`). Способ
+  реалистичен: существующий смок #126 уже умеет мокать отказ `config/set`
+  (`rejectKettleRelocation`), новый смок под #403 — органичное расширение
+  того же приёма, не изобретение с нуля.
+- **Скоуп/не-скоуп корректен и не пересекается** с #126 (критерии переезда,
+  формат снапшота — не трогаются), #74/#397 (механика Undo как таковая — не
+  трогается, трогается только объём очистки), #406 «г» (гигиена снапшотов
+  исчезнувших устройств — не относится к этой находке).
+- **Соответствие `docs/SCOPE.md`**: закрывает J6 (оптимистичная блокировка,
+  ручная расстановка маркеров) и защищает от нарушения стоящего правила
+  «никогда не удалять пользовательские данные по инференсу» — задача не
+  расширяет продукт, а чинит потерю уже введённых пользователем данных.
+- **i18n-раздел корректно условен**: если решение обходится существующей
+  меткой внимания — новых строк нет; если потребуется отдельное уведомление —
+  явно предписано добавить ключ во все четыре словаря. Не оставляет открытого
+  вопроса без явного правила на оба исхода.
+- **Откат и release-артефакты адекватны масштабу**: обе правки локальны
+  (один метод, один фильтр), changelog User-Visible: yes корректно назван
+  (пользователь увидит, что расстановка переживает отказ записи).
+- **Метки issue** (`bug`, `P1`, `S4-spec-review`, без `small`/`trivial`)
+  согласуются с заявленным в ТЗ полным треком — задание корректно определило
+  входной трек.
+- **Инфраструктурная переписка issue** (шесть провалившихся прогонов
+  конвейера, `is_error`/`modelUsage: {}`) не образует циклов ревью:
+  вердикта не было ни в одном из них, бюджет §4 остаётся 0 до этого раунда.
+
+## Чего не проверял
+
+- **Гейты кода** (`npx tsc --noEmit`, `npm test`, `npm run build`,
+  `check-docs`, смоки, инварианты модели) — не гонялись: на этапе ТЗ
+  продуктового кода нет, диффа для гейтов не существует (диапазон
+  `origin/dev..HEAD` состоит только из `docs/specs/403-area-relocation-safety.md`).
+  Это предмет код-ревью после реализации.
+- **`scripts/mutation-gate.mjs` / `demo/smoke_area_relocation.mjs` —
+  запуск**: не запускал ни то, ни другое; только убедился построчным чтением,
+  что оба файла существуют, а механизм мока отказа WS (`rejectKettleRelocation`)
+  в существующем смоке технически совместим с планом автотестов ТЗ (AC1/AC4).
+- **Реальный браузерный повтор репродукции C2/M1** — на этапе ТЗ кода
+  нет; воспроизведение, описанное автором аналитики (лог WS-вызовов,
+  состояние `layoutHasDevice`/`snapshotAreaNow`/`attention`/`undoAvailable`),
+  принято на основании сверки с логикой резолвера и обработчика отказа в
+  текущем дереве (см. «Как проверялось», п.3), а не путём собственного
+  запуска браузера — независимая браузерная перепроверка будет предметом
+  код-ревью, когда появится смок.
+- **`CommandStack`-реализация выборочного удаления** — не проектировал и не
+  требовал конкретного API; отметил только, что текущий тип данных
+  (`NamedCommand<DevicePositionState>` с `deviceId` в каждой записи) делает
+  контракт AC5/AC6 технически осуществимым, выбор метода — за реализацией.
+- **Таблицу `docs/specs/README.md`** — строка для #403 в неё не добавлена;
+  это известный, не относящийся к этой задаче долг (§7.3 п.1 PROCESS.md),
+  не поднимаю отдельной находкой.
+
+## Вывод
+
+Диагноз и контракт по обеим находкам аудита (C2, M1) точны, построчно
+сверены с кодом на `1f9d9014` и не содержат догадок, выданных за факт; AC1–
+AC7 однозначны, доказуемы и реалистичны на существующей тестовой
+инфраструктуре. Единственная блокирующая находка — процедурная (H1):
+ТЗ не называет обязательные по DoR §2.5 пункты про touch/kiosk и
+производительность. По существу риска в обоих пунктах, скорее всего, нет,
+и правка — несколько строк текста; возвращаю жёлтым, не красным.