docs: review document for #442

Issue: #442
User-Visible: no

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

@@ -0,0 +1,216 @@
+# SPEC-REVIEW-442-r1
+
+- Issue: https://github.com/Matysh/houseplan-card/issues/442
+- ТЗ: `docs/specs/442-marker-write-rollback.md`
+- Этап: `S4-spec-review`, заход r1, блокирующих циклов израсходовано 0 из 4
+- Ревьюер: Claude (роль «ревьюер ТЗ», отдельная сессия от автора)
+- Материал ревью: ТЗ на коммите `a8426268db90666920801f9e09554aa81343ceba`
+  (ветка `issue/442-marker-write-rollback`), тело issue #442 и все его
+  комментарии на момент разбора (2026-09-03)
+
+## Скоуп разбора
+
+Первый заход — разбор полный: продуктовая рамка (`docs/SCOPE.md`), процесс
+(`PROCESS.md` §2.4/§7.1, `AGENTS.md`), тело issue #442 и вся цепочка
+комментариев (аналитика → вопросы владельцу → решение владельца → готовое
+ТЗ), связанные issue #439 (guarded rollback общих настроек) и #441 (атомарный
+route CRUD), канонические `docs/CONFIG-COMPATIBILITY.md` и `docs/VACUUM.md`,
+терминология `docs/USER-GUIDE.ru.md`. Отдельно верифицированы факты о текущем
+коде, на которых ТЗ строит контракт: `src/serialized-write-queue.ts`,
+`src/houseplan-editor-runtime.ts`, `src/editors/vacuum-maps-section.ts`,
+`custom_components/houseplan/validation.py`, наличие названных тестов/смоков.
+
+## Как проверялось
+
+1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` (§1–§10), `AGENTS.md` полностью.
+2. Прочитаны тело issue #442 и все 4 комментария (аналитика с оценками
+   8/10 польза · 9/10 разработка · 5/10 сложность · 5/10 риск · P2 · full
+   track; пачка вопросов Q1/Q2 с default-вариантами; решение владельца,
+   принявшее оба default; финальный комментарий со ссылкой на готовое ТЗ).
+3. Прочитан весь текст `docs/specs/442-marker-write-rollback.md` (379 строк).
+4. Прочитаны связанные issue #439 и #441 — оба уже приняты и описывают ровно
+   тот прецедент (guarded rollback, атомарный route writer), на который ТЗ
+   #442 ссылается как на эталон и явно не дублирует.
+5. Прочитан `docs/CONFIG-COMPATIBILITY.md` целиком — не найдено противоречий
+   между заявленным «revision + content fingerprint» guard'ом и уже
+   задокументированным контрактом ревизий (#340), геометрии (#282/#306/#224)
+   и маршрутов пылесоса (#162).
+6. Прочитан `docs/VACUUM.md` целиком — раздел «Calibration» действительно не
+   документирует порядок toast/подтверждение относительно ответа сервера, то
+   есть заявленный в ТЗ release-артефакт (обновление `VACUUM.md`) не
+   дублирует уже существующий текст, а закрывает реальный пробел.
+7. Проверена терминология `docs/USER-GUIDE.ru.md`: «Редактор устройств»,
+   «Карты и этажи», «ручная подгонка», «Применить» — используемые в ТЗ
+   термины (кроме одного, см. Low ниже) не расходятся с интерфейсом.
+8. Делегирована фактологическая проверка кода отдельному агенту (Explore) по
+   9 конкретным пунктам — см. «Верификация фактов» ниже. Каждый пункт
+   перепроверен по конкретным строкам файла, не на веру.
+9. Самостоятельно прочитан `src/serialized-write-queue.ts` целиком (60 строк)
+   — независимо подтверждён guard: `host._cfgRev !== attempt.revision ||
+   (current !== attempt.attempted && fingerprint(current) !==
+   attempt.attemptedFingerprint)`, то есть ревизия **и** контент-фингерпринт
+   одновременно, включая ветку одного and того же object identity —
+   именно то, что ТЗ требует мутационно проверить в AC2.
+10. Самостоятельно найден `_writeChain`/`expected_rev` (`src/houseplan-card.ts:7662-7730`,
+    `src/houseplan-editor-runtime.ts:1175` и далее) — сериализация запросов,
+    на которую ссылается контракт §2, существует и используется.
+
+## Верификация фактов ТЗ против текущего кода (HEAD `a8426268`)
+
+| # | Утверждение ТЗ/issue | Результат |
+|---|---|---|
+| 1 | `_saveMarker()` мутирует `cfg.markers` до `await _saveConfigNow()`, `catch` только снимает busy и показывает toast | **Подтверждено** — `src/houseplan-editor-runtime.ts:8270-8285` мутация, `:8330` await, `catch` `:8359-8366` без восстановления `_serverCfg` и без rebuild devices |
+| 2 | *(из тела issue, не из ТЗ)* готовый guarded rollback применён «ровно к одному пути — общим настройкам (`_updateDecorStyle`)» | **Неточность в тексте issue.** Пара `optimisticAttempt`/`rollbackOptimistic` действительно встречается 3 раза (импорт + одна пара вызовов), но пара живёт в `_saveSettingsDialog` (`:10231-10275`, вызовы на `:10260`/`:10271`), а не в `_updateDecorStyle` (`:10204`) — та не содержит optimistic-логики вовсе и делегирует в `_persistDecorStyle` → обычный debounced `_saveConfig()`. Счётчик «18 вызовов `_saveConfig()`» корректен |
+| 3 | `_vacSaveMatrix()` мутирует in-place, коллеры показывают success toast и закрывают UI до подтверждения записи сервером | **Подтверждено** — мутация `m.vacuum`, `_maybeRebuildDevices()`, невawait-нутый debounced `_saveConfig()`; `toast.autocal_done`/`toast.cal_done` показываются сразу после синхронного (boolean) возврата `_vacSaveMatrix`, без ожидания сервера |
+| 4 | Route CRUD в `vacuum-maps-section.ts` уже использует `optimisticAttempt`/`rollbackOptimistic` (эталон #441) | **Подтверждено** — импорт `:21`, вызовы `:105`/`:116` |
+| 5 | Guard сравнивает revision **и** content fingerprint; есть `_writeChain`/`expected_rev` | **Подтверждено** самостоятельно (см. «Как проверялось» п.9-10) |
+| 6 | Четыре семантических валидатора маркера существуют под этими именами | **Подтверждено** ровно с этими именами (кроме generic «проверка controls», которую ТЗ и не называет точным именем) — `validate_marker_value_badges` (`:811`), `validate_marker_vacuum_routes` (`:886`), `validate_marker_light_entities` (`:930`), `validate_marker_controls` (`:972`, цикл — `:1049`) в `custom_components/houseplan/validation.py` |
+| 7 | Названные в плане тестирования файлы существуют | **Подтверждено** — все 6: `test/serialized-write-queue.test.mjs`, `demo/smoke_vacuum_route_draft.mjs`, `demo/smoke_vacuum_firstuse.mjs`, `demo/smoke_vacuum_multifloor.mjs`, `demo/smoke_vacuum.mjs`, `demo/smoke_dialog_zombie.mjs` |
+| 8 | `docs/VACUUM.md` ещё не документирует reject/success UX калибровки | **Подтверждено** — раздел «Calibration» молчит именно об этом |
+| 9 | `scripts/mutation-gate.mjs` существует | **Подтверждено** |
+
+Итог верификации: единственное расхождение с кодом (пункт 2) находится в
+**теле issue**, не в тексте ТЗ — сам документ `docs/specs/442-...md` нигде не
+называет `_updateDecorStyle` и не строит контракт на этом имени; он ссылается
+на механизм обобщённо («общий writer», «уже готовый `optimisticAttempt`/
+`rollbackOptimistic`»), что после проверки оказалось верным независимо от
+того, в какой именно функции этот механизм физически живёт. Так что дефект
+не проникает в исполнимость ТЗ — см. Low ниже, почему он всё равно стоит
+записи.
+
+## Соответствие §7.1
+
+Все обязательные разделы присутствуют: Сценарий · Что человек увидит до и
+после · Подтверждённая проблема · Скоуп/Не-скоуп · Контракт поведения (6
+подпунктов) · Данные и совместимость · Touch/клавиатура/доступность ·
+Ошибки и крайние случаи · AC1–AC9 с доказательством (unit/smoke/test double
+у каждого) · План тестирования · Карта реализации · Риски и rollback ·
+Release-артефакты · Принятые предположения.
+
+Владелец отвечал только на продуктовые вопросы (Q1 — какие marker-пути
+входят в атомизацию; Q2 — что делать с calibration UI при отказе), оба с
+default-вариантом и явным принятием; технические решения (immutable
+candidate, где именно живёт guard, разбиение на pure-модуль) автор
+резервирует себе явным блоком «Принятые предположения» в конце — ровно так,
+как требует процесс.
+
+## Находки
+
+### Low-1. Неточное имя функции в теле issue (не в ТЗ)
+
+Issue #442 в разделе «Проблема» называет уже реализованный guarded-rollback
+путь `_updateDecorStyle`; фактически это `_saveSettingsDialog`
+(`src/houseplan-editor-runtime.ts:10231`). `_updateDecorStyle` — соседняя
+функция без optimistic-логики. Ошибка не искажает контракт ТЗ (там имя
+функции не упоминается вовсе, только «общий writer» и сам механизм), поэтому
+не блокирует и не требует правки ТЗ. Фиксирую как заметку для реализации:
+если разработчик будет ориентироваться на текст issue при поиске «уже
+готового пути», неверное имя может стоить нескольких минут поиска не там —
+не более того.
+
+**Решение ревьюера:** снимается без правки, запись сделана для истории раунда.
+
+### Low-2. Явного раздела «i18n» нет — контент есть, но не под заголовком
+
+§7.1 перечисляет i18n отдельным обязательным разделом. В ТЗ #442 подтверждение
+«новых строк нет» разнесено по двум местам («Новых targets, жестов и строк
+i18n нет» в разделе Touch; «новых текстов ошибок... не добавляется» в
+Не-скоуп) без отдельного заголовка. По содержанию требование выполнено
+(эксплицитно и дважды непротиворечиво), поэтому не блокирует переход в
+`S5-ready` — DoR требует «i18n: ключи en+ru перечислены», а перечислять
+здесь нечего, что и сказано явно. Отмечаю как стилистическую придирку, не
+находку, которая должна кого-то останавливать.
+
+**Решение ревьюера:** снимается без правки.
+
+## Что проверено и корректно
+
+- **Продуктовая рамка.** Задача закрывает J6 (`docs/SCOPE.md`: «Keep the plan
+  true as the home evolves» — «View не должен показывать конфигурацию,
+  которую сервер отверг»), сценарий и «что человек увидит» — оба заполнены
+  без терминов реализации первым предложением, персона (Home admin) и
+  поверхность (Device editor + calibration overlay) названы.
+- **Скоуп/не-скоуп** зеркалят принятые owner defaults Q1/Q2 дословно, без
+  расширения и без урезания против решения владельца.
+- **Технические утверждения о текущем коде** — 8 из 9 проверенных фактов
+  подтверждены чтением реальных строк; единственное расхождение (Low-1) не
+  влияет на сам контракт ТЗ.
+- **Контракт поведения** опирается на реально существующий примитив
+  (`optimisticAttempt`/`rollbackOptimistic` с двойным guard'ом ревизия+
+  фингерпринт, уже обкатанным в #441 и в `_saveSettingsDialog`), а не
+  изобретает новый механизм с нуля — минимизирует риск.
+- **AC1–AC9** каждый указывает способ доказательства (unit / browser smoke /
+  test double) и достаточно конкретен для написания теста без дополнительных
+  решений: например, AC2 явно называет два обязательных мутанта
+  (unconditional rollback; пропуск fingerprint для same identity),
+  предвосхищая требование код-ревью §2.7 о «таблице чем краснеет».
+  Формулировки допускают ровно одну трактовку каждая — не нашёл AC, который
+  можно закрыть двумя разными реализациями с разным видимым поведением.
+- **Регрессионный периметр** (AC8, таблица «Ошибки и крайние случаи», строка
+  «route CRUD #441 reject/retry») явно защищает уже принятый атомарный route
+  writer от повторной реализации или порчи — соответствует правилу «не
+  подменять существующий механизм».
+- **Данные и совместимость**: явно «миграции нет», persisted schema не
+  меняется, что соответствует характеру задачи (только client-side
+  транзакционность) и не противоречит ни одному разделу
+  `CONFIG-COMPATIBILITY.md`.
+- **Touch/доступность**: busy — настоящий disabled, а не только визуальный
+  индикатор; фокус остаётся в открытом диалоге — соответствует
+  `TOUCH-SUPPORT.md` (редакторы desktop-first, но контракт не деградирует
+  специально под touch).
+- **Release-артефакты** называют оба changelog, `CONFIG-COMPATIBILITY.md`,
+  `VACUUM.md` (подтверждено выше, что там действительно есть пробел для
+  заполнения), условие для docs-скриншотов и явное «golden не нужен, если
+  подходящей канонической поверхности нет» — не оставляет открытых пунктов
+  DoR.
+- **Догадки, выданные за решения**: не найдено. Каждое место, где решение
+  не следует напрямую из существующего документа, либо помечено как
+  default-ответ владельца (Q1/Q2), либо вынесено в явный блок «Принятые
+  предположения» в конце ТЗ, что ревьюер вправе (и не находит повода)
+  оспорить.
+
+## Чего не проверял
+
+- Не запускал `npx tsc --noEmit`/`npm test`/`npm run build` — на этапе
+  ревью ТЗ кода ещё нет (это этап `spec`, не `code`), гейты §8 к этому
+  документу неприменимы.
+- Не проверял мутационные сценарии AC2 практически (мутант ещё не написан —
+  это работа код-ревью, не спек-ревью); отметил только, что защитный
+  примитив уже существует и уже используется в проде (#441).
+- Не проверял браузерные смоки исполнением — только их наличие как файлов
+  (см. таблицу верификации, п.7). Их фактическое покрытие AC — задача
+  реализации и код-ревью.
+- Не проверял `docs/specs/README.md` на предмет обязательной строки в
+  таблице issue↔ТЗ — это релиз-артефакт автора ТЗ, не блокирует ревью.
+
+## Вердикт
+
+**Зелёный.** High: 0, Medium: 0. Обе Low-находки сняты без правки текста ТЗ
+(записаны выше для истории раунда, не блокируют). ТЗ полно по §7.1, каждый
+AC однозначен и снабжён способом доказательства, продуктовые вопросы решены
+владельцем до написания ТЗ, а технические утверждения о существующем коде
+подтверждены независимым чтением, а не приняты на веру.
+
+## Материал раунда
+
+- Ветка: `issue/442-marker-write-rollback`
+- SHA ТЗ: `a8426268db90666920801f9e09554aa81343ceba`
+- Путь ТЗ: `docs/specs/442-marker-write-rollback.md`
+- Найти дерево: `git log --all --format='%H %T' | grep <дерево HEAD>`
+- Найти блоб ТЗ: `git log --all --find-object=<блоб> -- docs/specs/442-marker-write-rollback.md`
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/442-marker-write-rollback`, коммит `a8426268db90` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `5d3fbe71add69e722a9f6d87b1ce04dcea586729`
+  ```
+  git log --all --format='%H %T' | grep 5d3fbe71add6
+  ```
+- ТЗ `docs/specs/442-marker-write-rollback.md`, блоб `a9dcca2ff269a1c309387902875a4937efbd4ebf`
+  ```
+  git log --all --find-object=a9dcca2ff269a1c309387902875a4937efbd4ebf -- docs/specs/442-marker-write-rollback.md
+  ```