diff --git a/docs/reviews/SPEC-REVIEW-126-r2.md b/docs/reviews/SPEC-REVIEW-126-r2.md new file mode 100644 index 00000000..3a5c047b --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-126-r2.md @@ -0,0 +1,180 @@ +# SPEC-REVIEW-126-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/126 +- **ТЗ:** `docs/specs/126-ha-area-marker-relocation.md` +- **Коммит материала:** `cfd9c0fd` (`git rev-parse HEAD` перед подведением итогов; ветка `issue/126-ha-area-marker-relocation`) +- **Этап:** ТЗ на ревью (PROCESS.md §2.4) +- **Заход:** r2 · блокирующих циклов израсходовано 1 из 4 +- **Трек:** полный (владелец явно закрыл `small` 2026-08-30) + +## SHA предыдущего раунда — примечание + +Вердикт r1 назвал материал `8bc8ba66`. Этого объекта в текущей истории репозитория +нет (`git cat-file -t 8bc8ba66` → `fatal: Not a valid object name`) — SHA был +назван, но перестал существовать. Причина не в недобросовестности: комментарий +автора в issue от 2026-08-31T05:40 прямо говорит «Ветка перебазирована на +актуальный dev@4683a493cca25a4a527f2b96d3d11723aa9da74c», и `git log -1 +--format='%H %P' a3d343ca` подтверждает — родитель коммита ТЗ теперь `4683a493` +(коммит `docs: review document for #400`), а не старый `dev@6bf39ee9`, на +который опирался `8bc8ba66`. Это ребейз на ушедший вперёд `dev` (PROCESS.md +§2.10/§7.2) — по правилу «после ребейза это другой код», разбор ниже полный, а +не только по дельте текста. + +## Скоуп ревью + +Диапазон материала — весь актуальный текст `docs/specs/126-ha-area-marker-relocation.md` +на `cfd9c0fd` плюс проверка того, что ребейз (диапазон `dev@6bf39ee9..dev@4683a493`) +не задевает ни одного факта, который ТЗ выдаёт за освоенный код. Продуктовый код +по-прежнему не менялся — issue остаётся в `S3-spec`/до-DoR стадии, только текст +спецификации. + +## Дельта с r1 + +`git diff da8cb6fa..cfd9c0fd -- docs/specs/126-ha-area-marker-relocation.md` +(коммит `cfd9c0fd`, +17/−2) — правки по всем трём находкам r1 (M1, L1, L2). +Дополнительно проверено, что ребейз `a3d343ca`/`da8cb6fa`/`0d92712c`/`cfd9c0fd` +на `dev@4683a493` не занёс изменений в подсистемы, о которых ТЗ делает +фактические заявления: + +``` +git diff 6bf39ee9..4683a493 --stat -- src/devices.ts src/logic.ts \ + src/space-geometry.ts src/types.ts \ + custom_components/houseplan/validation.py custom_components/houseplan/websocket_api.py +``` +→ пусто, ни один из этих файлов в диапазоне ребейза не тронут. + +Ребейз действительно принёс продуктовый код, но только по #400 (полировка +furniture-редактора): `src/houseplan-card.ts` — порядок отрисовки handle'ов +selection box у мебели (`HANDLE_PAINT_ORDER`), и `src/houseplan-editor-runtime.ts` +— исключение перетаскиваемого маркера устройства из align-candidates через +`_deviceDrag`. Оба куска — про мебель/выравнивание в редакторе плана, не про +маркеры устройств, Area или layout-запись, на которых стоит ТЗ #126. Ни один +символ, который ТЗ #126 называет (`_livePos`, `_defaultPositions`, +`_syncNewDevices`, `diffNewDevices`, `markerPos`, `pointInPolygon`, +`houseplan/layout/delete`, explicit-override правило #317), в диапазоне ребейза +не задет. Ребейз не инвалидирует ни один факт из «что проверено и признано +корректным» r1. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** (Medium, в скоупе) — §9 ссылался на несуществующий budget `known_devices` | §9 переписан: вводится собственный лимит `MAX_MARKER_AREA_SNAPSHOT = MAX_KNOWN_DEVICES = 20_000` entries плюс общий `MAX_CONFIG_BYTES = 2 MiB`; то же продублировано в §23 | `docs/specs/126-ha-area-marker-relocation.md:204-206, 479-480` (коммит `cfd9c0fd`). Число `20_000` не выдумано: `custom_components/houseplan/validation.py:1092` реально объявляет `MAX_KNOWN_DEVICES = 20000` и применяет его к `known_devices`/`new_device_ids` через `vol.Length(max=MAX_KNOWN_DEVICES)` (`validation.py:1932-1933`) | +| **L1** (Low, снята с записью) — `bindingKind:'entity'` не отличает direct entity marker от composite light group | §8 получил абзац-дискриминатор: eligibility требует сохранённый `marker` с exact `binding === entity:${bindingRef}`, id-префикс — только defensive check; §23 повторяет это как явное техническое решение | `docs/specs/126-ha-area-marker-relocation.md:176-180, 482-483`. Поле `marker.binding: string // 'device:' \| 'entity:' \| 'virtual'` реально существует (`src/types.ts:121`); light-группы markerless и получают синтетический id `'lg_'+eid` (`src/logic.ts:812`, `src/devices.ts:1175`) — у них нет сохранённого `marker.binding`, так что дискриминатор из §8 действительно их отсекает, а не выдуман | +| **L2** (Low, снята с записью) — нет явной строки о влиянии на производительность | В §19 добавлен абзац «Performance budget»: resolver гоняется только на authoritative rebuild, линейная сложность по markers+rooms, не вызывается из render, не пишет при unchanged input | `docs/specs/126-ha-area-marker-relocation.md:433-435` | + +Примечание не по делу закрытия, а по качеству r1: сама находка M1 была верной +по существу (ТЗ действительно ссылалось на несуществующее *название* бюджета, +не подкреплённое кодом), но обоснование r1 содержало неточность — r1 утверждал, +что «явного per-field ограничения... в коде нет», хотя `MAX_KNOWN_DEVICES = 20000` +(`validation.py:1092`) существовал уже тогда и не менялся в диапазоне ребейза. +Похоже, r1 грепал `known_devices|new_device_ids` в `src/types.ts` и +`MAX_CONFIG_BYTES` в `validation.py`, но не искал сам `MAX_KNOWN_DEVICES`. Это +не отменяет находку (несуществующее было именно то конкретное число/имя, +которое ТЗ приписывало — и после правки текст ссылается на реальный, верно +процитированный лимит), поэтому цикл не переоткрывается — только фиксирую для +трассируемости. + +## Унаследовано из r1 + +Документ `docs/reviews/SPEC-REVIEW-126-r1.md`, SHA материала `8bc8ba66` +(недоступен в текущей истории — см. раздел выше; содержательно эквивалентен +`a3d343ca`+`da8cb6fa` до ребейза, что подтверждено отсутствием изменений в +диапазоне ребейза по всем файлам, которые ТЗ цитирует). Принято без повторной +построчной проверки в r2, так как текст этих разделов не менялся в дельте +`da8cb6fa..cfd9c0fd` и ни один цитируемый в них символ не задет ребейзом: + +- полнота обязательных разделов §7.1 (сценарий/персона, «что видно до и + после» без терминов реализации, проблема/диагноз, скоуп/не-скоуп, контракт + поведения, UX/touch/a11y, модель данных и миграция, i18n, AC1–AC12 со + способом доказательства, план тестов, риски/откат, release-артефакты); +- соответствие J6 (`docs/SCOPE.md`); +- построчная сверка с кодом причины дефекта (`_livePos()` + `src/houseplan-card.ts:5174-5190`, `markerPos()` + `src/space-geometry.ts:629-638`, `diffNewDevices()` `src/logic.ts:2092-2100`); +- explicit-override правило #317 (`src/devices.ts:1421-1424`, + `resolveExplicitMarkerPlacement()` `src/devices.ts:1066-1086`); +- реальность эндпойнта `houseplan/layout/delete` + (`custom_components/houseplan/websocket_api.py:1099-1106`); +- переиспользование `pointInPolygon()` (`src/logic.ts:431`) для backfill; +- терминология по `docs/USER-GUIDE.ru.md` (метка «Новое», Find/поиск её не + снимают); +- `docs/specs/README.md` обновлён корректно; +- владельцу не задано новых продуктовых вопросов. + +## Новая находка r2 + +### Low — снимается с записью + +**L3. Строка 4 документа («Статус документа: актуализировано на +`dev@6bf39ee9...`») устарела после ребейза на `dev@4683a493`.** + +Реальный родитель первого коммита ТЗ в текущей истории — +`4683a493cca25a4a527f2b96d3d11723aa9da74c` +(`git log -1 --format='%H %P' a3d343ca`), а не `6bf39ee9`, который документ +называет своей базой. Само по себе не влияет на содержание ТЗ (диапазон ребейза +не касается ни одной подсистемы, на которую опирается спецификация — см. раздел +«Дельта с r1»), но неточная строка провенанса мешает следующему ревьюеру быстро +проверить актуальность базы, как и в этом раунде пришлось восстанавливать факт +ребейза из комментария issue, а не из самого документа. **Снимается с +записью:** не блокирует; рекомендую поправить строку на актуальный `dev@…` при +следующей правке ТЗ (или прямо при переводе в `S5-ready`), отдельного цикла ради +этого не открываю. + +## Как проверялось + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | green, без вывода | +| Unit-тесты | `npm test` | green: `tests 1668, pass 1667, fail 0, skipped 1` (тот же известный environment-sensitive пропуск, не относится к дифу) | +| Build | `npm run build` | green, `dist` собран без ошибок | +| Сверка трёх копий бандла | `npm run bundle:sync` | green; после запуска `git status --porcelain` пуст — `dist`, `custom_components/houseplan/frontend`, `demo/srv/assets` уже были в синхроне (диф #126 не трогает `src/**`) | +| `node scripts/check-docs.mjs` | не запускал | диф #126 (`da8cb6fa..cfd9c0fd`) не трогает `src/**`; ребейз принёс изменения в `src/houseplan-card.ts`/`src/houseplan-editor-runtime.ts`, но это уже смёрженный и прошедший свой гейт код #400, не предмет этого ревью | +| `npm run invariants` / model invariants | не запускал | диф не трогает геометрию, `layout`, `marker.space`, `open_spans` — только текст ТЗ | +| Browser smoke (`demo/smoke_*`) | не запускал | продуктовый код не менялся ни этим дифом, ни ребейзом в затронутых #126 модулях | +| `golden:verify`, `performance_smoke`, `pytest tests_backend` | не запускал | визуальные/perf/backend поверхности не тронуты — диф docs-only | + +Зелёного Validate на SHA `cfd9c0fd` не найдено, поэтому дешёвые гейты прогнаны +вручную повторно в этом раунде (код в дереве изменился из-за ребейза, хотя сам +диф #126 — нет); все зелёные. Тяжёлые гейты не применимы по тем же основаниям, +что и в r1. + +## Что проверено и признано корректным + +- Все три находки r1 (M1/L1/L2) закрыты текстом, а не заявлением — см. таблицу + «Закрытие раунда r1» с точными строками и перекрёстной проверкой по коду. +- Ребейз на `dev@4683a493` не заносит правок ни в один файл/символ, на который + опирается ТЗ #126 — проверено диапазонным `git diff --stat` по всем ключевым + путям. +- AC1–AC12, разделы §5–§6 (скоуп/не-скоуп), §11–§14 (порядок записи, + multi-client, race-guard) и §21 (риски/откат) не менялись в дельте и не + задеты ребейзом — наследуются из r1 без повторной построчной проверки. +- Новый текст §9 корректно цитирует реальный код (`MAX_KNOWN_DEVICES = 20000`, + `validation.py:1092`), а не изобретает число. +- Новый текст §8 корректно опирается на реально существующее поле + `marker.binding` (`src/types.ts:121`) как дискриминатор, а не на + недостаточный `bindingKind`. + +## Чего не проверял + +- Не проверял выполнимость резолвера по коду — кода ещё нет, это спек-ревью. +- Не прогонял golden/performance/browser smoke/backend pytest — диф их не + касается (см. таблицу гейтов). +- Не делал полного повторного построчного код-аудита разделов, унаследованных + из r1 (§7.1-состав, J6, #29/#74/#317 контракты, endpoint layout/delete, + point-in-polygon, терминология USER-GUIDE) — их текст не менялся в дельте, а + ребейз не касается ни одного файла, на который они ссылаются; это явно + зафиксировано в разделе «Унаследовано из r1». +- Не проверял вручную код #400 (мебель/handle paint order, align-candidates) — + это чужой уже смёрженный на `dev` контракт вне предмета #126, тронут только + ребейзом, не текстом ТЗ. + +## Вывод + +High-находок нет. Все Medium/Low из r1 закрыты правкой на конкретных строках, +подтверждённой кодом. Одна новая Low-находка (L3, устаревшая строка провенанса +в шапке документа) снята с записью, не блокирует. + +**Вердикт: зелёный.** ТЗ #126 готово к переводу в «Готово к разработке» (DoR +§2.5): AC пронумерованы и доказуемы, миграция/compatibility, touch, i18n, +performance и откат названы явно, открытых продуктовых вопросов нет.