houseplan-card/docs/reviews/SPEC-REVIEW-273-r1.md

SPEC-REVIEW-273-r1

• Issue: #273 — Optimize сохраняет sub-grid островок толщины у T-узла: 22→15→22
• Этап: ТЗ на ревью (S4-spec-review), заход r1 (первый; предыдущих раундов нет)
• Артефакт ТЗ: docs/specs/273-optimize-topology-island.md
• Проверено на SHA: b5fbe633a60f8a2091552204fc7ff9e3da67fb06 (HEAD ветки issue/273-optimize-topology-island на момент ревью)
• Лёгкий/короткий трек: нет (small/trivial не назначены) — ТЗ обязано жить файлом, что соблюдено
• Вердикт: жёлтый

Скоуп

Явное расширение guard'а collapseIsolatedWallThicknessIslands() (#198, src/plan-optimizer.ts:99-200): разрешить схлопывание sub-half-grid островка толщины, когда ровно один его конец — реальный room/T topology node, а второй — синтетическая off-grid точка на том же прямом parent edge, при равных соседних толщинах. Изменение затрагивает только explicit Optimize; runtime/editor нормализация остаётся lossless (не входит в скоуп).

Job по docs/SCOPE.md: J6 («Keep the plan true as the home evolves», explicit maintenance должен приводить план к физически осмысленному и идемпотентному виду) — названо в ТЗ явно (§1), обоснование корректно.

Как проверялось

1. Прочитаны docs/SCOPE.md, AGENTS.md, PROCESS.md целиком (обязательный порядок чтения).
2. Прочитано тело issue #273 и оба комментария (аналитика владельца, хендофф автора «ТЗ готово к ревью»). Второго ревью-раунда в истории issue нет — это первый заход, разбор полный, не по дельте (§2.10 неприменим).
3. Прочитан канонический документ подсистемы docs/WALL-THICKNESS.md, в частности §3 (уже документированный контракт #198 — «neither endpoint is a room vertex or resolved opening endpoint»), чтобы сверить, что ТЗ предлагает именно точечное расширение, а не переписывание контракта.
4. Прочитан сам ТЗ-файл docs/specs/273-optimize-topology-island.md целиком, сверены обязательные разделы §7.1 PROCESS.md.
5. Прочитан текущий код src/plan-optimizer.ts (collapseIsolatedWallThicknessIslands, guard isTopologyNode, сбор nodes из roomPoly() и openCuts) и src/wall-thickness.ts (roomWallProfile, atomicPolyForRoom, floorFootprintGeometry — комментарий «Independent partitions/columns are deliberately not accepted here»), чтобы проверить техническую состоятельность заявленных в ТЗ гарантий, а не поверить формулировкам на слово.
6. Сверены названные в ТЗ файлы/инструменты на существование: demo/smoke_optimize_micro_interval.mjs, scripts/mutation-gate.mjs, wallBodiesGeometry(), docs/TESTING.md, docs/CONFIG-COMPATIBILITY.md — все существуют, ссылки не фиктивны.
7. Сверен список «Обязательные регресс-тесты» из тела issue против AC1–AC9 ТЗ — построчное соответствие (см. таблицу ниже).
8. Гейты кода в этом раунде не гоняются: на этапе ТЗ нет кода для типecheck/test/build — это ожидаемо и не является упущением.

Соответствие регресс-тестов issue → AC ТЗ

Требование issue	AC ТЗ
1. Minimized fixture 22→15→22, T-узел слева, off-grid справа	AC1
2a. Оба конца топологические	AC3
2b. Opening endpoints	AC3
2c. Соседи различаются	AC4
2d. Conflicting exact owners	AC4
2e. Цепочка micro-островков	AC4
3. Идемпотентность, независимость от порядка/направления	AC5, AC6
4. Не только JSON — effective wallIntervals и render-толщина	AC1 (wallIntervals), AC2 (wallBodiesGeometry)
5. Мутант старого a || b guard обязан падать	AC8

Все пункты, которые владелец назвал обязательными в issue, отражены в AC — пропусков нет.

Находки

Medium (в скоупе задачи) — заявленная защита от partition/draft-границы не имеет ни доказательства, ни видимого механизма реализации

Файл: docs/specs/273-optimize-topology-island.md, §6.2 (пункт «второй endpoint не является... endpoint другого physical axis») и §6.3 (пункт «второй endpoint совпадает с отдельной room/partition/draft axis boundary»).

В чём проблема. ТЗ формулирует это как обязательное блокирующее условие наравне с «оба конца топологические» и «opening/open-span endpoint» — но в отличие от них:

• ни в AC3, ни в AC4 нет negative-фикстуры на этот случай (там перечислены только: оба topology/opening endpoints, разные соседи, missing/zero/open neighbor, chain/overlapping, conflicting owners, offset/perpendicular/parallel coincidence, intentional thickness change on topology boundary — партиций/drafts нет);
• текущий код не имеет механизма её обнаружить. collapseIsolatedWallThicknessIslands() строит список topology-узлов (nodes, src/plan-optimizer.ts:111-119) только из roomPoly(room) всех комнат и точек openCuts (это open_spans, а не двери/окна — терминология ТЗ здесь совпадает с существующими тестами #198, это я проверил отдельно и не считаю дефектом). Независимые перегородки (space.partitions) и drafts в эту функцию вообще не передаются как аргумент и нигде не читаются. Комментарий в src/wall-thickness.ts:2311-2313 прямо формулирует принцип модуля: «Independent partitions/columns are deliberately not accepted here, so they can never enlarge the slab perimeter» — то есть весь код, от которого наследует collapseIsolatedWallThicknessIslands, сознательно не видит partition/draft-геометрию.

Конкретный сценарий поломки. Перегородка обычно примыкает к стене комнаты не в вершине полигона комнаты, а в произвольной точке вдоль ребра (T-примыкание). Если синтетическая (некандидатная) граница b микро-островка на ребре комнаты случайно совпадёт именно с такой точкой примыкания перегородки — а не с вершиной какой-либо комнаты и не с концом open_span — сегодняшний isTopologyNode её не увидит вообще, ни до, ни после этого ТЗ: она не входит в nodes ни как room-vertex, ни как open-cut endpoint. Формулировка §6.3 обещает, что такой случай «всегда блокирует», но: (а) реализация этого не делает и в разделе «Ожидаемые файлы» (§10) нет ни новой структуры данных, ни изменения сигнатуры collapseIsolatedWallThicknessIslands для передачи partition/draft-геометрии; (б) ни один AC не проверяет этот случай, значит регресс тут никогда не будет пойман тестом даже если решение владельца — «пусть остаётся как есть».

Это именно тот класс дефекта, о котором предупреждает процесс: «утверждение о поведении, которого нет ни в одном документе и которое не помечено как предположение». Раздел §14 «Принятые технические предположения» перечисляет 5 пунктов и явно не содержит допущения по этому случаю — то есть автор не пометил его как предположение, а выдал как решённый и гарантированный факт.

Почему это не High. Основной репродуцированный дефект (T-node одной комнаты + синтетическая точка на том же parent edge, без перегородок) специфицирован полно, тестируем и напрямую закрывает исходный баг-репорт — партиционный случай гипотетический, не из живого репро, и в худшем случае приводит не к потере геометрии, а к слиянию двух смежных data-записей с одинаковым cm в точке, где могла бы быть полезна отдельная граница для будущего редактирования. Это ухудшение data-model гранулярности, а не пользовательский регресс толщины/геометрии.

Что нужно поправить (любое из трёх, на усмотрение автора — технический спор решается в следующем раунде, не эскалируется владельцу):

1. убрать пункт про partition/draft-boundary из §6.2/§6.3 как отдельную категорию и явно перенести его в §14 как «принято предположительно: коллизия с partition/draft-примыканием посередине ребра считается настолько редкой, что не тестируется в этом issue»; либо
2. добавить AC и negative-фикстуру, доказывающую, что такая коллизия действительно блокируется (и, если для этого нужно новое обнаружение — назвать это явно в §10 «Ожидаемые файлы», это меняет и оценку сложности); либо
3. явно доказать (комментарием в ревью-цикле), что геометрически такая коллизия на этом code path невозможна — если у автора есть довод сильнее прочитанного мной кода, он снимает находку.

Low — не все AC называют способ доказательства явным словом по шаблону §2.5

Файл: docs/specs/273-optimize-topology-island.md, §9, AC3, AC4, AC5, AC7.

AC1/AC2/AC6/AC8 явно называют механизм доказательства (test/plan-optimizer.test.mjs + assertion / production-bundle smoke / mutation). AC3, AC4, AC5, AC7 описывают, что именно проверяется, но не произносят слово «unit» и не называют файл — это восстанавливается из контекста и §10 «Ожидаемые файлы» (test/plan-optimizer.test.mjs), поэтому не блокирует, но формально DoR требует «у каждого [AC] указано, чем он доказывается: unit / backend / smoke / golden / ревью кода» (§2.5). Рекомендация: при правке ТЗ по Medium-находке заодно дописать по одной строке «Доказательство: unit, test/plan-optimizer.test.mjs» к этим четырём AC — цена правки минимальна, откладывать в отдельный цикл не имеет смысла.

Что проверено и корректно

• Все обязательные разделы §7.1 PROCESS.md присутствуют: сценарий (§1), что человек увидит до/после (§4), проблема и подтверждённая причина (§2–§3), скоуп/не-скоуп (§5), контракт поведения (§6), UX/accessibility/touch/security/perf (§8), модель данных и совместимость (§7, явно: без изменения model_version/схемы, старый клиент читает как один obычный wall entry), i18n (§8, явно «новых locale keys нет»), AC1–AC9 (§9), риски (§12), откат (§13), release-артефакты (§11).
• Технический контракт §6.1–6.2 (базовые условия #198 + разрешённый один T-endpoint) корректно описывает существующий код: реальный guard в src/plan-optimizer.ts:155 — ровно isTopologyNode(a) || isTopologyNode(b), как и заявлено в §3 «Подтверждённая причина»; арифметика порога (GRID_PITCH = 4.166667, 0.5 × GRID_PITCH = 2.083333 > 1.381904) сходится с числами из реального экспорта.
• Термин «opening/open-span» в ТЗ корректно соответствует существующему словарю кода и тестов (openCuts = open_spans, не маркеры дверей/окон) — я отдельно перепроверил это по test/plan-optimizer.test.mjs:236-266, где atOpening-кейс тестирует именно openCuts; расхождения терминологии с каноном нет.
• Каждый обязательный регресс-тест из тела issue имеет соответствующий AC (таблица выше) — пропусков нет, автор не сузил скоуп относительно требования владельца.
• Не-скоуп (§5) корректно исключает runtime/editor cleanup, изменение persisted-схемы, #271 (finite-ray) — задачи не смешаны.
• Продуктовых вопросов владельцу нет, и это оправдано: единственная развилка («когда именно можно схлопывать») полностью решена техническим контрактом §6.1–6.3, разночтений в пользовательском сценарии я не нашёл.
• Rollback и release-артефакты реалистичны и не описывают несуществующую миграцию.
• Проверка «одно число — один источник» (PROCESS.md §8): новых пользовательских чисел не вводится, wallsMerged переиспользуется как есть (§7) — вопросов нет.

Чего не проверял

• Не проверял фактическую реализацию (её ещё нет на этом этапе) — только заявленный контракт и его согласованность с текущим кодом collapseIsolatedWallThicknessIslands/roomWallProfile/atomicPolyForRoom.
• Не запускал npm test/typecheck/build — на этапе ТЗ гонять их не над чем, продуктовый код не менялся.
• Не проверял golden/smoke/performance — они относятся к код-ревью и пре-релизному гейту, не к ревью ТЗ.
• Не проверял, насколько часто на реальных планах перегородка примыкает к стене вне вершины комнаты (частотность сценария из Medium-находки) — эмпирических данных нет, вывод сделан только из чтения кода и комментариев модуля.
• Не проверял вручную конкретный приватный экспорт 1.json — он не в репозитории, что соответствует правилу «не добавлять приватный файл»; воспроизвёл только числовую арифметику по описанию.

Унаследовано из r0

Неприменимо — это первый заход (r1), предыдущего цикла нет, наследовать нечего.