docs: review document for #397

Issue: #397
User-Visible: no

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

@@ -0,0 +1,217 @@
+# SPEC-REVIEW-397-r1
+
+- Issue: https://github.com/Matysh/houseplan-card/issues/397
+- ТЗ: `docs/specs/397-device-position-echo.md` (класс A, полный трек — задета
+  логика синхронизации layout/optimistic locking, попадает под критерий
+  "public contracts")
+- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (эта проверка потратит
+  один, см. вердикт)
+- SHA ревью: `83692e79` (docs: #397 spec — correct the fingerprint line number)
+
+## Скоуп
+
+Issue не помечен `small` → полный трек, ТЗ обязано жить в `docs/specs/`.
+Файл на месте, создан и поправлен коммитами `9fcbb646` и `83692e79`, оба
+`User-Visible: no`, `Issue: #397` — трейлеры корректны для чисто
+документационного изменения (класс C, часть DoD этапа ТЗ).
+
+Диф этого раунда — только `docs/specs/397-device-position-echo.md` (174 строки
++ 1 правка номера строки). Продуктовый код (`src/**`) не тронут — это ожидаемо
+для этапа "ТЗ на ревью": код появится после `S5-ready`.
+
+## Как проверялось
+
+Ревью читает ТЗ против **текущего** состояния `src/houseplan-card.ts` и
+`src/device-position-history.ts` на этом SHA — не верит утверждениям на слово,
+перепроверяет построчно.
+
+1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.4/§7.1/§7.2.
+2. Прочитано тело issue #397 (комментариев нет).
+3. Прочитан канонический документ подсистемы — спека #74
+   (`docs/specs/074-device-position-undo.md`), сверена формулировка AC10
+   (строка 254): "Own echo/reconnect same-content сохраняют stack, отличный
+   remote content очищает" — дословно совпадает с тем, как ТЗ #397 её цитирует.
+4. Каждая строчная ссылка ТЗ проверена чтением исходника на этом SHA:
+   - `_persistDevicePlacement`, `src/houseplan-card.ts:5226-5258` (для
+     сравнения — ниже `return;` идёт ветка `_persistLocalLayout()` до
+     закрывающей скобки ~5261, ТЗ обрывает диапазон чуть раньше конца функции —
+     не искажает суть, см. Low ниже);
+   - подтверждено: строка 5230 пишет `this._layout =
+     applyDevicePlacement(this._layout, deviceId, placement)` **сырым**
+     `placement`, строка 5244 канонизирует `this._layout[deviceId]` в
+     локальную `pos`, строка 5248 отправляет `pos` на сервер, но `pos` **не**
+     кладётся обратно в `_layout` — фиксация отпечатка на 5253 считается по
+     нежеканоническому `_layout`. B3 воспроизводится буквально;
+   - для контраста: `_persistLayout` (debounced writer, `:4959-4980`, вызывается
+     из `_savePos`, `:5316-5323`) **правильно** пишет канонический `pos`
+     обратно в `_layout` (`:4967`) до отправки — это именно тот старый паттерн,
+     на который ссылается ТЗ как на `v1.69.0:4776`. Он используется для
+     позиций **подписей комнат** (`rl_*`, только через
+     `houseplan-editor-runtime.ts:10854`), не для маркеров устройств — граница
+     скоупа в ТЗ («не в скоупе» не called out явно, но по факту верна: чужой
+     путь не задет и не сломан);
+   - `_reloadLayoutOnly` (`:4904-4948`): подтверждено, сравнение на `:4928`
+     идёт с `contentFingerprint(this._layout)` — текущим (нежеканоническим)
+     слепком, а не с сохранённым `_layoutContentFingerprint`;
+   - `_adoptStructuralResponses` (`:4203-4269`): подтверждено, сравнение на
+     `:4249` идёт с `this._layoutContentFingerprint` — зафиксированным на
+     `:5253` по нежеканоническому слепку;
+   - `demo/smoke_device_position_history.mjs:235-237`: подтверждено, строка
+     235 (`serverLayout = structuredClone(c._layout)`) перезаписывает уже
+     корректно смоделированный по проводу `serverLayout` (строится из
+     `message.pos` на `:38-49`, канонический с самого начала) локальной копией
+     клиента — именно та подмена, которую описывает M1. Проверка `:237`
+     зелёная независимо от состояния кода.
+5. Сверена типизация/сборка на этом SHA (Validate не найден зелёным):
+   - `npx tsc --noEmit` → чисто, без вывода;
+   - `npm test` → `1655 tests, pass 1654, fail 0, skipped 1` (существующий
+     известный skip, не связан с #397);
+   - `npm run build` → собрался (`dist` создан за 15.9s).
+   Эти три гейта проверяют **текущее** состояние дерева, не содержимое этого
+   ТЗ: на этом этапе продуктовый код ещё не менялся, так что зелёный результат
+   подтверждает только то, что база, от которой стартует реализация, не
+   сломана — он не может ни подтвердить, ни опровергнуть корректность плана.
+
+## Обязательные разделы (PROCESS.md §7.1)
+
+Все пункты присутствуют: сценарий · что человек увидит до/после · проблема ·
+скоуп/не-скоуп · контракт поведения · UX · модель данных и миграция · i18n ·
+критерии приёмки · план автотестов · риски · откат · release-артефакты.
+Сценарий называет персону (хозяин/Home admin), поверхность (редактор плана,
+фон работы карточки) и момент (reconnect/вторая вкладка/перезапуск HA) — в
+одном месте с явным продуктовым "до/после" без терминов реализации. Это
+закрывает J6 SCOPE.md ("Keep the plan true as the home evolves" — drag/resize,
+multi-client sync, optimistic locking), хотя ТЗ не называет строку J6 явно.
+
+Утверждений о поведении, которое существует только в голове автора и подано
+как факт без пометки "предположение", не найдено — каждое техническое
+утверждение о текущем коде (B3, M1) проверено построчно и совпадает буквально
+(см. выше). Раздел рисков документирует два технических решения (порядок
+записи против гонки с `_sentPos`, условие «писать только при отличии» против
+лишнего ре-рендера) с митигацией — по факту это и есть блок «принято
+предположительно», просто не выделен формальным заголовком; не блокирует.
+
+## Находки
+
+### Medium (в скоупе) — AC7 и половина AC5 не имеют способа доказательства
+
+`PROCESS.md` §7.1 требует критерии приёмки "с указанием доказательства"; §2.5
+для DoR — то же самое пофакторно (unit/backend/smoke/golden/"ревью кода").
+
+- **AC7** ("Позиции, отправленные и ещё не подтверждённые (`_sentPos`),
+  продолжают побеждать ответ сервера при слиянии — поведение
+  `_reloadLayoutOnly` в этой части не меняется.") не имеет ни фразы
+  "Доказательство: …", ни строки в разделе "План автотестов". Риски
+  упоминают эту гонку как митигацию ("AC7 фиксирует существующее поведение
+  слияния"), но это не тест, а намерение. Существующий смок
+  `demo/smoke_layout_sync.mjs:87` проверяет только что `_sentPos` **дренируется**
+  после ответа сервера, не что он **побеждает** отличающийся ответ при мердже
+  в `_reloadLayoutOnly`. Ни один из текущих тестов эту ветку не покрывает —
+  проверено грепом `_sentPos`/`_reloadLayoutOnly` по `test/` и `demo/smoke_*.mjs`.
+  Правка B3 меняет ровно тот код (`_persistDevicePlacement`, окружение
+  `_sentPos.set`), который граничит с этой веткой слияния — риск регрессии
+  реален и явно того типа, что #102 уже стоил продукту (правка по соседнему
+  замечанию ломает AC, который никто не проверяет).
+- **AC5**, вторая половина ("эхо удаления историю не чистит"), тоже без
+  доказательства. Юнит-план (`test/device-position-persist.test.mjs`, пункт 2)
+  доказывает только запись/отпечаток для ветки удаления — то есть первую
+  половину AC5 ("локальный ключ удалён до фиксации отпечатка"). Ни один из
+  трёх браузерных смоков в плане (пункты 4–6) не воспроизводит сценарий
+  «удалить → `_reloadLayoutOnly()` с честным сервером, ответившим тем же
+  удалением → `canUndo` остаётся `true`» — все три про перемещение
+  (`перемещение → …`), не про удаление. Существующий смок уже умеет удалять
+  позицию и катать undo/redo (`demo/smoke_device_position_history.mjs:185-221`,
+  сценарий "auto-positioned marker round-trips through delete/update"), но там
+  нет шага reload/reconnect после удаления — сценарий AC5 просто не собран.
+
+**Почему это Medium, не Low**: обе непроверенные ветки лежат ровно там, где
+работает фикс B3 (запись в `_sentPos`, порядок записи в `_layout` до/после
+отправки) — это тот класс изменений, где локальная правка по одному замечанию
+способна тихо сломать соседний, никем не тестируемый инвариант. Без явного
+теста код-ревью не сможет отличить "починили и не сломали AC7/AC5-эхо" от
+"починили и сломали" иначе как чтением, а чтение уже один раз ошиблось на
+похожей логике (#102).
+
+**Что чинить**: добавить в "План автотестов" минимум по одному пункту на
+каждую ветку — например смок 7 "удаление → честный reload с тем же удалением →
+`canUndo === true`" (AC5) и смок 8 (или юнит с моком `hass.callWS`, если
+воспроизвести гонку в браузерном смоке дороже) "`_sentPos` победил при
+`_reloadLayoutOnly`, даже когда сервер вернул другое значение для того же
+устройства" (AC7); проставить "Доказательство: …" у обеих строк AC.
+
+### Low — обрыв диапазона строк для `_persistDevicePlacement`
+
+ТЗ цитирует функцию как `src/houseplan-card.ts:5226-5258`; фактическое тело
+функции (включая ветку `_persistLocalLayout()` без серверного хранилища и
+закрывающую скобку) простирается примерно до `:5261`. Диапазон не искажает ни
+один вывод ТЗ — код до 5258 покрывает всю ветку с багом — не блокирует, автор
+может поправить попутно или оставить.
+
+### Low — отсутствует фраза "Доказательство" у AC3
+
+AC3 не содержит явного "Доказательство: …", но раздел "План автотестов"
+закрывает это пунктом 5 браузерного смока с явной пометкой "(AC3)" — по факту
+доказательство названо, просто не в самой строке AC. Не блокирует, чисто
+стилистическая непоследовательность с AC1/AC2/AC4.
+
+## Что проверено и корректно
+
+- Обязательные разделы §7.1 присутствуют полностью, в правильном порядке,
+  продуктовые "сценарий"/"что человек увидит" отвечают на оба вопроса без
+  терминов реализации.
+- Технический разбор B3 и M1 **воспроизведён построчно на текущем SHA**, а не
+  принят на веру: все номера строк, имена функций и цитаты кода совпадают
+  буквально с содержимым `src/houseplan-card.ts` и
+  `demo/smoke_device_position_history.mjs`.
+- Граница скоупа корректна: путь `_savePos`/`_persistLayout` (используется
+  только для позиций подписи комнаты, `rl_*`, вызывается из
+  `houseplan-editor-runtime.ts:10854`) уже пишет канонический ответ обратно и
+  багом не затронут — ТЗ справедливо его не трогает.
+- Цитата AC10 ТЗ #74 сверена с `docs/specs/074-device-position-undo.md:254` —
+  дословное совпадение.
+- Модель данных/миграция, i18n, откат — корректно описывают отсутствие
+  изменений формата и нулевой миграционный риск для точечной правки в две
+  строки исходника плюс одну строку фикстуры.
+- Release-артефакты: изменение поведения (Undo/Redo больше не гаснет без
+  причины) верно помечено как `User-Visible`, требующее правки обоих
+  changelog — соответствует правилу AGENTS.md о трейлере.
+- AC1, AC2, AC4, AC6 однозначны и имеют названный, воспроизводимый способ
+  доказательства с прослеживаемой связью AC ↔ пункт плана автотестов /
+  мутант.
+- Гейты `npx tsc --noEmit`, `npm test` (1654/1655 pass, 1 known skip),
+  `npm run build` зелёные на этом SHA — база для будущей реализации не
+  сломана (см. оговорку в разделе "Как проверялось": эти гейты не проверяют
+  содержимое самого ТЗ, продуктовый код ещё не менялся).
+
+## Чего не проверял и почему
+
+- `node scripts/check-docs.mjs` — не прогонялся: диф не трогает `src/**`
+  (только `docs/specs/397-*.md`), отпечаток скриншотов документации не мог
+  устареть от этого изменения.
+- `npm run invariants -- --config …` — не прогонялся: диф не меняет геометрию,
+  `layout`-структуру, `marker.space` или `open_spans`; формат данных ТЗ прямо
+  заявляет неизменным.
+- `demo/smoke_*.mjs` (браузерные смоки) — не прогонялись: продуктовый код не
+  менялся на этом SHA (это ревью ТЗ, а не кода), смокам нечего проверять,
+  запускать их сейчас означало бы тестировать `dev`, а не эту задачу.
+  `scripts/smoke-select.mjs` по той же причине не вызывался — нет диффа в
+  `src/**`/`demo/**`, который можно было бы сопоставлять со смоками.
+  Инструмент отложен до этапа код-ревью, когда появится реализация.
+- `npm run golden:verify`, `python -m pytest tests_backend -q`,
+  performance-профили — не прогонялись: рендер/геометрия/стили/слои и
+  `custom_components/**/*.py` этим ТЗ не затронуты (нет кода вообще), и ни
+  один AC не называет perf-путь.
+- `npm run bundle:sync` / `bundle:budget` — не прогонялись: `dist/**` —
+  генерируемый класс D, этот диф в него не пишет.
+
+## Вердикт
+
+Один High не найден. Один Medium **в скоупе** этой же задачи (незакрытый
+пункт "План автотестов" для AC5/AC7) — по правилу #202 чинится тем же автором
+в том же issue, отдельный issue не заводится. Два Low — не блокируют,
+исправить по вкусу автора.
+
+**Жёлтый.** Возврат в "ТЗ в работе": дополнить план автотестов пунктами,
+доказывающими вторую половину AC5 и весь AC7, проставить фразы
+"Доказательство: …" у AC3/AC5/AC7, по желанию поправить диапазон строк
+`_persistDevicePlacement`.