diff --git a/docs/reviews/SPEC-REVIEW-397-r1.md b/docs/reviews/SPEC-REVIEW-397-r1.md new file mode 100644 index 00000000..0fef01b7 --- /dev/null +++ b/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`.