diff --git a/docs/reviews/SPEC-REVIEW-477-r1.md b/docs/reviews/SPEC-REVIEW-477-r1.md new file mode 100644 index 00000000..30ce5c43 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-477-r1.md @@ -0,0 +1,274 @@ +# SPEC-REVIEW-477-r1 + +- **Issue:** #477 — «Оптимизировать планы» не должна требоваться повторно +- **Этап:** ТЗ на ревью (PROCESS.md §2.4), трек **полный** (не `small`) +- **Материал:** `docs/specs/477-editor-writer-fixed-point.md` на SHA + `aa962ed2c5a03bd2f50b4ae066968761cd053585` (дерево `56e39416...`), ветка + `issue/477-optimizer-fixed-point` +- **Заход:** r1 · блокирующих циклов израсходовано 0/4 +- **Вердикт: зелёный** + +## Вердикт + +`Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче | · Документ: docs/reviews/SPEC-REVIEW-477-r1.md` + +## Скоуп разбора + +Первый заход, ТЗ полного трека. Разбирал целиком: `docs/SCOPE.md`, `AGENTS.md`, +`PROCESS.md` целиком, тело issue #477 и все три комментария владельца +(повторная аналитика после #478, поправка про off-grid мебель, ссылка на +готовое ТЗ), сам файл `docs/specs/477-editor-writer-fixed-point.md` (492 +строки) и его запись в `docs/specs/README.md`. Отдельно поднял канонические +документы затронутых подсистем: `docs/CANVAS.md` (§9.4, §9.5), +`docs/WALL-THICKNESS.md` (контракт #276/#281/#296), `docs/USER-GUIDE.ru.md` +(терминология «цепочка», «Оптимизировать планы», `Esc`), `docs/TOUCH-SUPPORT.md` +(контракт touch-редакторов), `docs/ARCHITECTURE.md`/`docs/CONFIG-COMPATIBILITY.md` +(контракты #282, #314). + +Владелец в третьем комментарии прямо попросил независимое ревью отдельно +проверить три границы: seeded reconciliation, Undo/Redo после завершения +wall-chain, исключение continuous transform мебели/изображений из Optimize — +все три разобраны отдельно ниже (§6.1–6.2, §7, §9 документа ТЗ). + +## Как проверялось + +Это ревью ТЗ, не код-ревью: гейты `typecheck`/`test`/`build`/`check-docs` +здесь не применяются — на branch нет продуктового кода (см. ниже), только +документация. Основной метод — **состязательная фактчек-проверка**: каждое +техническое утверждение ТЗ, которое можно спутать с догадкой, выданной за +факт, я сверил прямым чтением текущего `dev`-кода (не поверил заявлению +автора). Не поверил на слово ни одному месту, где заявлен *текущий* дефект +(«функция X сейчас не делает Y»), и перепроверил каждое такое место чтением +тела функции. Список того, что сверено буквально — раздел «Что проверено» +ниже. + +Отдельно проверил состав ветки: `git diff ..HEAD` — +ветка добавляет только `docs/specs/477-editor-writer-fixed-point.md` и одну +строку в `docs/specs/README.md`. Полный `git diff origin/dev..HEAD` показывает +больше файлов (`.github/workflows/validate.yml`, `scripts/mutation-gate.mjs`, +удаление `docs/reviews/CODE-REVIEW-475-r*.md` и др.) — это независимый прогресс +`dev` после точки расхождения (`origin/dev` и эта ветка разошлись от общего +предка `d12b1f20`, ни один не является предком другого), а не изменения этой +задачи; branch не рибейзился на текущий `dev`, но это не создаёт конфликта +содержимого для ревью ТЗ (класс C, документация, DoR ещё не пройден, кода нет). +Отмечаю это для протокола, чтобы автор рибейзнул ветку перед переходом к +`S6-in-progress`, если к тому моменту `dev` уйдёт ещё дальше — блокирующей +находкой не считаю, т.к. на этапе ТЗ рибейз не требуется. + +## §7.1: обязательные разделы — все на месте + +| Раздел §7.1 | Где в ТЗ | +|---|---| +| Сценарий (персона/поверхность/момент) | §1 | +| Что человек увидит до/после | §2 | +| Проблема | §3 | +| Скоуп и не-скоуп | §5 | +| Контракт поведения | §6, §7, §8, §9 | +| UX | §13 | +| Модель данных и миграция | §12 | +| i18n | §13 | +| AC1…ACn с доказательством | §14 | +| План автотестов | §15 | +| Риски | §16 | +| Откат | §17 | +| Release-артефакты | §18 | + +Плюс необязательный, но полезный раздел §19 «Принято предположительно» — +именно то место, где по правилу §7.1 должны жить технические (не +продуктовые) допущения автора, доступные для оспаривания ревьюером. + +## Продуктовая рамка (SCOPE.md) + +Job **J6** — «Keep the plan true as the home evolves» — явно упоминает +drag/resize, merge/split, оптимистичную блокировку как часть этой работы. +Задача устраняет технический долг ровно в этой job: план не должен «портиться» +незаметно после обычного редактирования. Персона (§1 ТЗ) — Home admin, +единственная персона, которая вообще видит редакторы (SCOPE.md: «View mode is +the product», редакторы — admin-only). Сценарий и before/after (§1–2) +отвечают на оба обязательных продуктовых вопроса без терминов реализации в +разделе «что увидит» — придаточные предложения там пользовательские +(«завершение цепочки», «после удаления или объединения комнаты»), а не +кодовые идентификаторы. Технические термины (`_finishWallChain`, +`mergeCollinearPartitions` и т.п.) появляются только в §3 и далее — это +правильное место для них. + +## Открытый вопрос? Разбор по существу + +Инструкция ревью требует: «не бывает сложной задачи без единого открытого +вопроса». Для этой задачи он был — и уже закрыт **до** написания ТЗ: владелец +отработал открытые продуктовые вопросы в двух отдельных комментариях анализа +(«Повторная аналитика после #478» и «Поправка… off-grid геометрия мебели»), +явно сузив скоуп (исключил `room_drafts`/room-acceptance, разобранные #478) и +дав прямое продуктовое решение по мебели/изображениям («их transform должен +сохраняться целиком»). Раздел §19 документирует остаточные **технические** +развилки (не продуктовые), как и требует процесс: продуктовая неопределённость +устранена перепиской до захода в `S3-spec`, техническая — явным блоком +«поменять свободно». Отдельного неотвеченного продуктового вопроса в тексте +ТЗ не осталось, и это не тихое замалчивание, а прослеживаемый результат. + +## Что проверено буквальным чтением кода (не поверил на слово) + +Все ссылки — на исходники на SHA `aa962ed2` (текущий `dev`, из которого ТЗ +исходит: продуктовый код в этой задаче ещё не пишется). + +- **`_finishWallChain`** (`src/houseplan-editor-runtime.ts:1474-1484`) — + подтверждено: тело только очищает `_path`, `_activeWallChainId`, + `_activeWallChainPartitionIds`, `_wallChainSegmentCms`, `_wallChainRedo`, + `_closingWallCm`, вызывает `_clearPlanSnapHover()`. Merge/reconcile не + вызывается — центральное утверждение проблемы (§3) верно буквально. +- **`_mergeSpacePartitions`** (`houseplan-editor-runtime.ts:1458`) — существует, + принимает `seedIds?: string[]`, оборачивает `mergeCollinearPartitions`; по + всему `src/*.ts` вызывается только собственным определением/обёрткой в + `houseplan-card.ts` — реально нигде не вызывается в рантайме. Утверждение + «фактически не вызывается» — верно. +- **`mergeCollinearPartitions`** (`src/wall-merge.ts:148`) — контракт seed-scope, + epsilons, детерминированный survivor, цикл до fixed point — присутствуют в + коде уже сейчас (задел под #229). ТЗ §6.1 описывает использование + существующего контракта, а не выдумывает новый. +- **`commitWallChainSegmentGeometry()`** (`src/draft-live-commit.ts:60`) — + прочитан целиком: один local physical check, один junction pass, один + `_recordGeometry` + один `_saveConfig()`; ни merge, ни reconcile, ни + `optimizePlans` не вызываются. §11 верно буквально. +- **`_commitPhysicalGeometry`** guard `wallModelOffGridValueCount(after) <= + wallModelOffGridValueCount(before, authored)` — подтверждён + (`houseplan-editor-runtime.ts:2113-2114`, тот же паттерн в + `draft-live-commit.ts:107-108`). `wallModelOffGridValueCount` определена в + `src/wall-segment-model.ts:88`. +- **`_canCommitSpace`** (`houseplan-card.ts:1475`) — admin/authority guard, + используется в путях commit — §12 «current writer не расширяет права» не + голословно. +- **`_confirmRoomDelete`** (`houseplan-editor-runtime.ts:5485-5579`) и + **`_commitMerge`** (`:6408-6430`) прочитаны целиком: обе фильтруют + `sp.rooms`, ни одна не трогает `marker.room_id` или + `marker.vacuum.segment_map`. Починка orphan `room_id` сейчас существует + только в `space-reference-repair.ts`, вызываемой из `plan-optimizer.ts` — то + есть баг реален именно там, где заявлен (§3/§8): чинится только следующим + Optimize, а не в момент удаления/слияния. +- **`resizeFurnitureTransform`** (`src/furniture.ts:524`) — существует, + комментарий в коде прямо говорит про continuous resize (#383). +- **`alignAllToGrid`** (`src/align-grid.ts:145`) — decor-цикл не выделяет + `furniture`/`image` в отдельную ветку и гонит их через общий box-snap, + инкрementируя `moved`/`maxShiftCm` — подтверждает текущий баг, который §9 + ТЗ должен закрыть. +- **`docs/CANVAS.md` §9.4** (строка 512) — «Furniture resize is the explicit + exception to positional quantisation… Furniture placement and movement + remain grid-bound» — дословно то же, что заявляет §9 ТЗ. Раздел пока не + упоминает `image`, но §18 ТЗ обязывает обновить `CANVAS.md` этим же + коммитом — не расхождение, а зафиксированный будущий DoD. +- **`docs/WALL-THICKNESS.md`** (строка 512) — контракт #276/#281/#296 (proven + section absorption, deterministic residuals, `max(roomCm, partitionCm)`) + присутствует и совпадает с §6.2–6.3 ТЗ. +- **Performance-бюджеты** (`demo/benchmark_wall_draw_click.mjs:51-57`) — + `medianMs: 150, maxMs: 250, remoteMedianMs: medianMs*1.5+20` совпадают с + числами §11 ТЗ буквально — не выдуманы. +- **#282/#314** — реальные, ранее принятые контракты (ADR стабильной + идентичности стен и rollback после отказа записи), многократно + цитируются в `docs/ARCHITECTURE.md`, `docs/CONFIG-COMPATIBILITY.md`, + комментариях кода — не выдуманные номера. +- **`marker.vacuum.segment_map`** — поле есть в `src/types.ts:140`. +- **`docs/specs/README.md`** и **#478** — запись про #478 подтверждена, + `docs/specs/478-remove-room-drafts.md` и `docs/reviews/CODE-REVIEW-478-r*.md` + существуют: #478 реализована и прорецензирована, а не просто задумана — ТЗ + #477 корректно опирается на закрытый факт, а не на обещание. +- **`_routeDepartureHandled`**, `hashchange`-listener в `houseplan-card.ts` — + подтверждают, что «route departure» и смена space через hash — существующие + механизмы, на которые §6.4 ссылается как на finish-owners. +- **`async_save_config_state`/`canonicalize_config_geometry`** + (`custom_components/houseplan/store.py:200,220`) — вызов канонизации перед + записью подтверждён; сам факт («числовая канонизация — на каждой записи», + из тела issue) верен. Не искал отдельно, называется ли обёртка ровно + `async_save_config_state` во всех путях сохранения — не критично для ТЗ, + которое само по себе на эту функцию не опирается (это констатация уже + закрытого класса, не предмет #477). + +Отдельно проверил семантическую состоятельность формального инварианта §4: +`optimizePlans(P).changed === false ⇒ optimizePlans(W(P)).changed === false` +для одной операции `W` индукцией распространяется на последовательность +операций (после `W(P)` условие снова выполнено, значит следующая `W'` +сохраняет его же). Формулировка корректна и не скрывает индукционный шаг. + +## Три границы, которые владелец попросил проверить отдельно + +1. **Seeded reconciliation (§6.1–6.2).** Scope ограничен «связным + merge-компонентом, к которому относится хотя бы один seed» — это + осмысленная граница: incidental collinear соседи, физически примыкающие к + только что дорисованной цепочке, обязаны мержиться (иначе шов остаётся), + а несвязанные партиции плана трогать нельзя (иначе цена unbounded и это + прямо запрещено §5.2 «merge partitions всего пространства на каждом + клике»). §11 честно называет риск («если нельзя доказать без full-space + sweep — назад в S3-spec») вместо того, чтобы тихо платить цену на каждом + клике. +2. **Undo/Redo после finish (§7).** Контракт различает активную цепочку + (буквальный snapshot, посегментный Undo) и завершённую (canonical + snapshot через тот же lossless finalizer на каждом шаге истории) без + добавления скрытой history-команды — соответствует принципу «пользователь + рисовал стены, а не запускал скрытую команду» (§6.3) и не открывает второй + источник этой же геометрии. +3. **Furniture/image exclusion (§9).** Формулировка «весь transform… byte- + equivalent» и явное решение не пытаться отличить legacy off-grid transform + от нового legal continuous resize (провенанс недоступен) — прямое + повторение продуктового решения владельца из второго комментария, ничего + не додумано. + +## Находки + +Нет. High: 0, Medium: 0, Low: 0. + +Единственное отмеченное выше по разделу «Как проверялось» (branch не +рибейзнут на текущий `dev`) — не находка, а операционное замечание для +следующего этапа; на этапе ТЗ (класс C, без кода) оно не блокирует переход в +`S5-ready`. + +## Что не проверял + +- Не проверял `python -m pytest` / typecheck / build — на ветке нет + продуктового кода, гейты неприменимы к этапу ТЗ. +- Не проверял, что ровно эта функция называется `async_save_config_state` во + всех путях сохранения бэкенда (см. пояснение выше) — не влияет на выводы + ТЗ. +- Не оценивал реализуемость perf-бюджета §11 количественно (насколько + реалистичен «не более 1.5× median на удвоении unrelated партиций») — + это то, что должен доказать сам код и его perf-witness на код-ревью, а не + ТЗ; сам числовой контракт корректно унаследован от #461. +- Не проводил самостоятельный код-ревью существующих `wall-merge.ts` / + `coincident-partitions.ts` на предмет иных дефектов, не связанных с #477 — + вне скоупа ревью ТЗ. +- Единственная умышленно неполная иллюстрация — таблица §10 «минимальная + матрица» не перечисляет явно Split Room. Это не пробел ТЗ: сама таблица + названа «минимальной», а AC8 требует от реализации exhaustive-перечисления + **всех** текущих structural/layout writers с тестом, который краснеет на + недостающей строке — split, если у него найдётся долг, будет пойман тем же + AC8 на код-ревью. Ни владелец, ни разбор автора не называли Split + источником долга, и текущий код Split строит room-граничные стены, а не + независимые партиции, так что источника такого класса проблем там + структурно нет — но это не проверено построчным чтением `_splitClick`/ + `splitRoom`, поэтому отмечаю как то, что не проверял, а не как принятое на + веру утверждение. + +## Итог + +ТЗ полное по §7.1, каждый AC проверяем и указывает способ доказательства, +технические допущения явно обособлены в §19 и оспоримы, продуктовые вопросы +закрыты владельцем до входа в этап ТЗ, а все проверенные технические +утверждения о текущем состоянии кода подтвердились буквальным чтением — без +единой догадки, выданной за факт. Три специально запрошенных владельцем +границы (seeded reconciliation, Undo/Redo, furniture/image exclusion) +разобраны предметно и непротиворечиво. Оснований для Medium/High нет. + +**Вердикт: зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/477-optimizer-fixed-point`, коммит `aa962ed2c5a0` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `56e39416687f5bb6bd1242ed4c5722ca2ba34cc4` + ``` + git log --all --format='%H %T' | grep 56e39416687f + ``` +- ТЗ `docs/specs/477-editor-writer-fixed-point.md`, блоб `a07a7248e008229e6e865ef793b213fdb524d5e4` + ``` + git log --all --find-object=a07a7248e008229e6e865ef793b213fdb524d5e4 -- docs/specs/477-editor-writer-fixed-point.md + ```