diff --git a/docs/reviews/SPEC-REVIEW-162-r1.md b/docs/reviews/SPEC-REVIEW-162-r1.md new file mode 100644 index 00000000..29ce885c --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-162-r1.md @@ -0,0 +1,174 @@ +# SPEC-REVIEW-162-r1 + +- Issue: [#162](https://github.com/Matysh/houseplan-card/issues/162) +- ТЗ: `docs/specs/162-vacuum-map-space-routing.md`, редакция 2, коммит `086b3682` (ветка `issue/162-vacuum-map-space-routing`, HEAD detached на момент ревью) +- Трек: полный (P2 bug, сложность 8/10, риск 9/10 — критерии `small` §5 PROCESS.md не выполнены: миграция конфига, несколько поверхностей, новый UX-контракт) +- Заход: r1 (первый прогон `S4-spec-review` для этого issue — предыдущие редакции ТЗ его не проходили) +- Вердикт: **жёлтый** + +## Скоуп + +Многоэтажный робот-пылесос: House Plan хранит калибровку по `map_id`, но +marker несёт одно `space`, поэтому дока и live-оверлей фактически привязаны к +одному этажу. ТЗ вводит `marker.vacuum.map_routes[]` — явное сопоставление +(source, map_id) → target space, независимое от положения дока, с +resolution-контрактом, editor UI, backend trail routing, lifecycle/export и +release-артефактами. Продукт: J1 (достоверное местоположение), J6 +(конфигурация остаётся корректной при нескольких пространствах) — +подтверждено в `docs/SCOPE.md`. + +Задача проверялась целиком (заход r1, документ первый), раздел «Унаследовано +из r0» не применим. + +## Как проверялось + +1. Прочитан issue #162 целиком (тело + 4 комментария аналитики/актуализации/ТЗ) через `gh issue view`. +2. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (весь файл, включая §2.9/§2.10, §4, §7.1–7.2, §12) и `docs/USER-GUIDE.ru.md` §16 (роботы-пылесосы). +3. Прочитан канонический документ подсистемы `docs/VACUUM.md` целиком. +4. Прочитано ТЗ `docs/specs/162-vacuum-map-space-routing.md` целиком (21 раздел). +5. **Каждое фактическое утверждение §3.2 «Подтверждённая техническая база» проверено чтением кода на текущем HEAD (`086b3682`, `dev` слит):** + - `src/houseplan-card.ts:11334` — фильтр `devs` по `d.space === space.id` — подтверждено буквально; + - `src/houseplan-card.ts:12347` — выбор матрицы по `calibration?.[mapNow]` и silent `continue` — подтверждено буквально (строка `if (!matrix || matrix.length !== 6) continue;`); + - `src/types.ts` — состав полей `Marker.vacuum` (семь полей, без ссылки на space) — подтверждено; + - `src/vacuum.ts:310,322` — `vacMapIdFromAttrs`/`vacMapIdWithFallback`, nullish-цепочка — подтверждено; + - `custom_components/houseplan/trails.py` — run как `{"map_id", "started", "ended", "points"}`, `can_resume_trail_run()` сравнивает только `map_id`, **поле `source` в записи run отсутствует вовсе** — подтверждено чтением всего файла; + - `docs/USER-GUIDE.ru.md:1504` — «Для каждого `map_id` хранится собственная калибровка, поэтому многоэтажный робот может работать с несколькими пространствами» — подтверждено, это и есть завышенное обещание, которое ТЗ обязано снять (§18 release-артефактов). +6. Проверено существование каждого инструмента/модуля, на который ссылается ТЗ и не которые не выдумка: `scripts/mutation-gate.mjs`, `scripts/smoke-select.mjs`, `scripts/model-invariants.mjs`, `scripts/bundle-budget.mjs` (поля `initialViewFiles`/`lazyEditorFiles` в нём есть), `src/space-deletion.ts` (`collectSpaceMarkerDependencies`, `createSpaceDeletionCandidate`), `src/space-reference-repair.ts` (`repairSpaceReferences`, доктрина «не трогает вложенные calibration-данные» — подтверждена дословно в шапке файла), `src/render-device-snapshot.ts`. +7. Проверен «храповик» `test/core-file-budget.test.mjs`: потолки 13659/14323 подтверждены; фактическая длина файлов на HEAD `086b3682` — `wc -l` даёт 13544/14319 (в ТЗ 13545/14320 — расхождение на 1 объясняется разной трактовкой конечного перевода строки, не находка). +8. Проверен прецедент общей TS/Python JSON-фикстуры (`test/vacuum.test.mjs:446` + `tests_backend/test_trail_recorder.py:425` + `test/fixtures/vacuum-attrs/map-id.json`) — план §16.2 «shared TS/Python route fixture» — не новый паттерн, а продолжение существующего. +9. Прогнаны дешёвые гейты (зелёного Validate на `086b3682` нет): + - `npx tsc --noEmit` → чисто, без ошибок; + - `npm test` → **1819 pass / 0 fail / 1 skip** — совпадает с базовой линией, заявленной в ТЗ §3.4 буквально; + - `npm run build` → бандл собран без ошибок (16.1s). + `check-docs.mjs`/`model-invariants.mjs`/смоки/golden не прогонялись — на этом коммите нет изменений в `src/**` (диф спец-ревью — только `docs/specs/162-*.md`), эти гейты относятся к код-ревью после реализации, не к ревью ТЗ. + +## Находки + +### Medium (в скоупе) — M1: «compatible source» в §11.3.2 не определено и не проверяемо по факту схемы run + +**Файл:** `docs/specs/162-vacuum-map-space-routing.md:537` (§11.3, пункт 2) + +**Формулировка ТЗ:** «Run без `route_id`: … 2. при explicit routes сопоставляется +только если ровно один route имеет тот же `map_id` и **compatible source**». + +**Почему это находка.** Термин «compatible source» встречается в документе +ровно один раз и нигде не раскрыт — ни определением, ни примером, ни +блоком «принято предположительно». При этом текущая (и не меняемая этим ТЗ) +схема серверного run в `custom_components/houseplan/trails.py` — +`{"map_id", "started", "ended", "points"}` — **не содержит поля `source` +вообще** (`TrailBook.on_point`, `can_resume_trail_run`, весь файл прочитан +целиком). Легаси-run физически не хранит, с каким источником он записан — +только `map_id`. Значит по факту хранимых данных «совместимость источника» +нельзя вычислить сравнением полей run и route: это либо (а) требует ещё не +описанного косвенного механизма (например: «совместим», если единственный +кандидат-route на данный `map_id` в момент показа одновременно проходит +resolver §8.2 как `ready`/`needs_calibration`, то есть реально наблюдается на +этом source прямо сейчас), либо (б) реализация обязана будет **угадать** +конкретную трактовку самостоятельно — то есть ровно тот случай, который +раздел «Ambiguity is asked, not guessed» и §8.2 этого же ТЗ («Exact +source/map identity никогда не ретаргетится молча») запрещают в остальных +местах документа. + +Это не отвлечённая формальность: от этого определения зависит AC14 +(«Legacy run без route id отображается только при unique match; ambiguity +fail-closed | TS/backend unit») — как написано, эту проверку нельзя +детерминированно закодировать unit-тестом, потому что неизвестно, что именно +тест обязан признать «совместимым». + +**Почему Medium, не High.** Вопрос не продуктовый (пользователь не видит +слово «source» — он видит, показался путь или нет) и не требует владельца: +это техническая формулировка алгоритма, которую автор ТЗ решает сам по +§7.1 PROCESS.md («всё, чего пользователь не наблюдает, агенты решают сами»). +Фикс — одна-две фразы в §11.3, например: «под "compatible source" понимается +X» либо явная привязка к уже описанному в §8.2 механизму. Он не расширяет +скоуп и не меняет ни одного другого раздела ТЗ. + +**Что нужно от автора при возврате:** заменить «compatible source» точным +правилом, вычислимым из данных, которые run реально хранит (или из +результата resolver §8.2 на момент показа), и явно указать это правило +рядом с AC14. + +## Что проверено и признано корректным + +- Обязательные разделы §7.1 PROCESS.md присутствуют и в правильном порядке: + сценарий/персоны (§2), что видно до/после (§2), проблема (§1), скоуп/не-скоуп + (§5–6), контракт поведения (§4, §7–12), UX (§9), модель данных и миграция + (§7, §7.3, §20), i18n (§13), AC1…AC20 с указанным способом доказательства + (§15), план автотестов (§16), риски (§19), откат (§20), release-артефакты + (§18). +- Все 20 AC пронумерованы, для каждого назван способ доказательства + (`unit`/`backend`/`smoke`/`golden`/комбинации); ни один не оставлен без + доказательства. +- §16.5 «Мутанты защитных контрактов» заранее перечисляет 7 мутантов (M-A…M-G) + с точным тестом-свидетелем на каждый — это ровно тот формат, который + потребует код-ревью (#435/PROCESS §2.7), заложен уже на этапе ТЗ. +- Технический раздел §3.2 «Подтверждённая техническая база» — не голословен: + каждая строка кода, на которую ссылается ТЗ, проверена чтением и совпадает + дословно (см. «Как проверялось», п.5). Расхождений между заявленным и + фактическим поведением текущего `dev` не найдено. +- §3.4 (ограничения после первой редакции — храповик core-file-budget, + ленивый граф/бюджет бандла, требование мутантов, взаимодействие с + `space-deletion.ts`/`space-reference-repair.ts`, снимок фактов) — + все упомянутые файлы, скрипты и потолки существуют и имеют указанные + значения; план реализации (§17) корректно распределяет новый код по + отдельным модулям, не расширяя `houseplan-card.ts`/`houseplan-editor-runtime.ts`, + что обязательно при остатке бюджета в единицы строк. +- Overpromise, который ТЗ обязуется снять (`docs/USER-GUIDE.ru.md:1504`), + подтверждён буквально — раздел §18 «Release-артефакты» верно называет + `docs/USER-GUIDE.ru.md` для правки. +- Продуктовая рамка соответствует `docs/SCOPE.md`: J1/J6, View/kiosk — + блокирующие поверхности, editor touch — best effort (согласуется с + `docs/TOUCH-SUPPORT.md`), Static space card корректно исключена (§6, вне + скоупа по существующей доктрине — `docs/VACUUM.md` не описывает live-оверлей + для неё). +- Ни одного продуктового вопроса, вынесенного на автора вместо ревьюера, не + обнаружено — все технические развилки (имена модулей, route id формат, + раскладка файлов) явно помечены в §21 «assumed, change freely», как того + требует §7.1 PROCESS.md. +- Downgrade/rollback (§20) явно называет небезопасный путь (полный откат + backend после canonical-записи) и safe-путь — соответствует требованию + «откат» из чек-листа DoR §2.5. +- Дешёвые гейты — тесты в разделе выше — зелёные и совпадают с + зафиксированной в ТЗ базовой линией `npm test` (1819/0/1) один-в-один. + +## Чего не проверял + +- Смоки (`demo/smoke_*.mjs`), `golden:verify`, `pytest tests_backend`, + performance-профили и `check-docs.mjs` — не прогонялись: на этом SHA нет + изменений в `src/**`/`custom_components/**/*.py` (диф ревью — только текст + ТЗ), эти гейты относятся к циклу код-ревью после реализации (PROCESS.md §8 + прямо разделяет: «полные наборы — предрелизный/код-ревью гейт, не гейт ТЗ»). +- Модельные инварианты (`npm run invariants`) — геометрия комнат/стен этим ТЗ + не затрагивается (маршруты пылесоса не являются рёбрами/толщиной), поэтому + не запускались. +- Не проверялась реализация — её ещё нет, ТЗ находится на входе в + `S5-ready`. +- Мутанты M-A…M-G из §16.5 не прогонялись — они относятся к будущему коду, + которого на этом коммите нет; на этапе ТЗ проверялось только то, что для + каждого защитного контракта назван мутант и тест-свидетель (это выполнено). + +## Итог + +Один Medium-дефект **в скоупе задачи**: термин «compatible source» в §11.3.2 +не определён и, с учётом того что легаси-run вообще не хранит поле `source`, +не может быть проверен буквальным сравнением данных — AC14 в текущей +формулировке не поддаётся детерминированному тесту. High-находок нет, +остальные 19 AC однозначны и проверяемы, техническая база ТЗ подтверждена +чтением кода без расхождений. Вердикт — жёлтый: правка ограничивается одним +разделом ТЗ, повторный цикл может быть точечным. + +--- + + + +## Материал раунда + +- Ветка: `issue/162-vacuum-map-space-routing`, коммит `086b3682e96e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `2b5694be2febfde87f514b1261d14525b54a3c90` + ``` + git log --all --format='%H %T' | grep 2b5694be2feb + ``` +- ТЗ `docs/specs/162-vacuum-map-space-routing.md`, блоб `22176219387e22288dcb3197806937edd7ad073c` + ``` + git log --all --find-object=22176219387e22288dcb3197806937edd7ad073c -- docs/specs/162-vacuum-map-space-routing.md + ```