diff --git a/docs/reviews/SPEC-REVIEW-403-r1.md b/docs/reviews/SPEC-REVIEW-403-r1.md new file mode 100644 index 00000000..e7ea2e45 --- /dev/null +++ b/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`, `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` с `deviceId` в каждой записи) делает + контракт AC5/AC6 технически осуществимым, выбор метода — за реализацией. +- **Таблицу `docs/specs/README.md`** — строка для #403 в неё не добавлена; + это известный, не относящийся к этой задаче долг (§7.3 п.1 PROCESS.md), + не поднимаю отдельной находкой. + +## Вывод + +Диагноз и контракт по обеим находкам аудита (C2, M1) точны, построчно +сверены с кодом на `1f9d9014` и не содержат догадок, выданных за факт; AC1– +AC7 однозначны, доказуемы и реалистичны на существующей тестовой +инфраструктуре. Единственная блокирующая находка — процедурная (H1): +ТЗ не называет обязательные по DoR §2.5 пункты про touch/kiosk и +производительность. По существу риска в обоих пунктах, скорее всего, нет, +и правка — несколько строк текста; возвращаю жёлтым, не красным.