From 215d59823be2fa98a73aea9d411de0ac25a88790 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:54:38 +0000 Subject: [PATCH] docs: review document for #442 Issue: #442 User-Visible: no --- docs/reviews/SPEC-REVIEW-442-r1.md | 216 +++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-442-r1.md diff --git a/docs/reviews/SPEC-REVIEW-442-r1.md b/docs/reviews/SPEC-REVIEW-442-r1.md new file mode 100644 index 00000000..9d6fc726 --- /dev/null +++ b/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` + +--- + + + +## Материал раунда + +- Ветка: `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 + ```