diff --git a/docs/reviews/SPEC-REVIEW-310-r1.md b/docs/reviews/SPEC-REVIEW-310-r1.md new file mode 100644 index 00000000..5d7c4fb6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-310-r1.md @@ -0,0 +1,89 @@ +# SPEC-REVIEW-310-r1 + +Issue: #310 «Парный острый стык: вернуть полное остриё, зубец торца толстой стены срезать по граням острия (follow-up #309)» +Этап: ревью ТЗ (PROCESS.md §2.4) +Документ ТЗ: `docs/specs/310-pair-apex.md` (ветка `issue/310-pair-apex`, ревизия 1, коммит `c8d56b6c`) +Заход: r1 · блокирующих циклов израсходовано 0 из 4 +Метка: `small` не установлена → полный трек, файл ТЗ обязателен (создан) — соответствует. + +## Скоуп + +Ревью покрывает `docs/specs/310-pair-apex.md` целиком и его соответствие: +- телу issue #310 и обоим комментариям владельца (аналитика + «ТЗ готово»); +- `docs/SCOPE.md` (продуктовая рамка); +- `docs/WALL-THICKNESS.md` (канонический документ подсистемы, разделы про #302/#271/#309); +- фактическому коду `src/wall-thickness.ts`, `src/physical-geometry.ts`, `src/houseplan-card.ts` — не для код-ревью (правок в них нет), а чтобы отличить в ТЗ утверждение-факт от догадки, выданной за решение (обязательная проверка этапа spec). + +Изменений кода в диапазоне нет: единственный коммит на ветке — `c8d56b6c docs: spec for #310 pair apex and butt-end trim` (`docs/specs/310-pair-apex.md`, 70 строк, `User-Visible: no`, трейлер `Issue: #310` на месте). Гейты типа `tsc`/`test`/`build` не запускал — это этап ТЗ, продуктовый код не менялся. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §§1–7.2 — заход r1, правило §2.10 (разбор по дельте) не применяется. +2. Прочитано тело issue #310 и оба комментария (`gh issue view 310 --json ... `) — решения владельца от 2026-08-25 сверены построчно с §1 и §2 ТЗ. +3. Прочитан `docs/WALL-THICKNESS.md` (разделы про #302 junction nodes, #309 visual mitre limit, #271 lateral trim) — технический контекст ТЗ сверен с зафиксированным контрактом подсистемы. +4. Прочитан код: `linearWallJoinPatches`, `chamferApex`, `linearWallBody`, `physicalBodyParts`/`physicalBodySet`, `buildMultiWallNodeMap`, `junctionNodeGeometry`, `junctionContractHoles`, `virtualJunctionPatches`, `unionJunctionPatches`, все три сайта использования `linearWallJoinPatches` (`wall-thickness.ts:1261` превью, `physical-geometry.ts:242` физическая геометрия, `houseplan-card.ts:19961` превью протяжки) и все четыре сайта `physicalBodyParts(...)` в `houseplan-card.ts`/`space-render.ts` — чтобы подтвердить или опровергнуть каждое техническое утверждение ТЗ (не мог ли автор выдать догадку за факт). +5. Прочитана фикстура `test/fixtures/309-junction-teeth.json` и посчитаны вручную лучи в каждом из трёх узлов (`step`, `spike`, `hump`) по координатам партиций, чтобы проверить утверждение ТЗ «step/hump не трогаются, потому что это узлы ≥3 лучей». +6. Прочитан и воспроизведён логически существующий тест `test/wall-thickness.test.mjs:3178` («issue 309 the full teeth fixture leaves no junction holes»), включая его собственный assert `map.nodes.length >= 2` — это прямое доказательство того, что делает находка №1 ниже. +7. Сверены AC1–AC7 и план тестов (§8) на однозначность, наличие способа доказательства и реальное существование называемых идентификаторов/функций/тестов (все существуют в коде — не фантазия автора). + +## Находки + +### [High] AC5 и Риск №2 называют детектор, который структурно не видит узел-пару — риск «дыры на границе среза» остаётся без рабочей проверки + +**Файл:** `docs/specs/310-pair-apex.md`, §5 (риск 2) и §7 (AC5). + +**Что не так.** Риск 2 прямо называет главную опасность задачи: «Новое вычитание — дыры на границе среза», и единственным гейтом под этот риск объявляет «детектор дыр (#302) на фикстуре и сценах» — то есть AC5: «детектор `junctionContractHoles` пуст на фикстуре и сценах». Это не так: `junctionContractHoles(geometry, map, options)` перебирает исключительно `map.nodes`, а `map` строится через `buildMultiWallNodeMap`, которая **отбрасывает узел, если у него меньше трёх канонических лучей** — дважды, явным `continue`: + +``` +src/wall-thickness.ts:1984 if (rays.length < 3) continue; +src/wall-thickness.ts:2032 if (canonicalRays.length < 3) continue; +``` + +Узел-пара (ровно два луча) — это ровно тот случай, который #310 создаёт и меняет: `linearWallJoinPatches` явно обрабатывает его отдельной ручной веткой именно потому, что «#309: узел с тремя и более каноническими лучами закрывается веерами вместо парных патчей» (комментарий в коде, `wall-thickness.ts:1137-1141`). Он никогда не попадёт в `map.nodes`, а значит `junctionContractHoles` никогда не просемплирует окрестность этого узла — независимо от того, есть там дыра или нет. + +**Это не гипотеза, а воспроизводимый факт на собственной фикстуре задачи.** В `test/fixtures/309-junction-teeth.json` у узла `spike` (2.2208, 1.35) ровно два луча — партиции `mt8liuxi-0` (10 см) и `mt8liuxi-1` (20 см), сходящиеся только там. У узлов `step` (0.8083, 1.2333) и `hump` (1.7583, 1.6125) — 4 и 3 луча соответственно (посчитано по координатам концов партиций в фикстуре). Уже существующий тест `test/wall-thickness.test.mjs:3178` («issue 309 the full teeth fixture leaves no junction holes») строит `map` из этой же фикстуры и утверждает `assert.ok(map.nodes.length >= 2, ...)` — то есть в `map.nodes` попадают ровно `step` и `hump`, а `spike` — нет. Это тот самый узел, который #310 меняет и в котором вводит новое адресное вычитание. + +**Почему это блокирует, а не мелочь.** Ручного тестирования нет ни на одном этапе процесса — по правилам этапа код-ревью именно автотест отвечает на вопрос «оно вообще работает», а на этапе ТЗ моя задача — убедиться, что AC доказуемо тем способом, который в нём назван. AC5, как написан, доказывает отсутствие дыр там, где #310 ничего не меняет (веера ≥3 лучей — они и так «не-скоуп», §2.3), и не доказывает ничего в узле, где появляется новая булева вычитающая операция — ровно там, где риск и назван. Ни один из остальных AC этот пробел не закрывает: AC2 проверяет одну конкретную точку в прежней зоне зубца (не «нет ли дыры где-то ещё на границе среза»), AC3 — только тело **за пределами** `2·halfDepth` (граница среза находится внутри этого радиуса, а не за ним), AC4 — байтовое неизменность **других** узлов, AC6 — не относящийся случай, а мутанты §7.7(a-c) моделируют «фаска вернулась», «трим выключен», «трим не ограничен» — ни один не моделирует «трим сработал в правильных границах, но оставил щель/дыру на стыке с патчем». То есть при реализации с корректной формой контура (AC1/AC2 пройдут) и корректным поведением за радиусом (AC3 пройдёт) дыра ровно на границе среза может пройти весь набор ACs незамеченной. + +**Воспроизведение (как убедиться самостоятельно):** +``` +grep -n "if (rays.length < 3) continue;\|if (canonicalRays.length < 3) continue;" src/wall-thickness.ts +# 1984 и 2032 — оба фильтра ограничивают map.nodes узлами ≥3 лучей +sed -n '3178,3200p' test/wall-thickness.test.mjs +# assert.ok(map.nodes.length >= 2, 'fixture keeps its multi-wall nodes') — из трёх узлов +# фикстуры в map попадают только 2 (степ и горб); spike (парный) исключён по построению. +``` + +**Что нужно поправить в ТЗ** (не мой выбор решения — это техническая деталь, автор/ревьюер решают сами по §7.1): AC5 должен либо (а) получить отдельный способ доказательства для узла-пары — например прямая проверка «нет дырки» через сэмплирование окрестности узла на `physicalBodySet.geometry` тем же приёмом, что и `junctionContractHoles`, но без фильтра по числу лучей, либо (б) явно расширить вход `junctionContractHoles`/его карту так, чтобы 2-лучевые узлы тоже туда попадали. Любой вариант — правка внутри этого же ТЗ, не смена подхода. + +### [Low] Технические решения реализации не оформлены явным блоком «принято предположительно, поменять свободно» — принимаю сам, без правки ТЗ + +**Файл:** `docs/specs/310-pair-apex.md`, §2.2 (место среза в конвейере: «второе адресное вычитание рядом с тримом #271, той же фазой в потребителях»). + +Это решение, которое пользователь не наблюдает (внутренняя фазировка), и по PROCESS.md такие решения должны попадать в явный блок «принято предположительно, поменять свободно», чтобы ревьюер мог его оспорить целенаправленно. Блока нет — решения просто вписаны в контракт. По существу возражений нет: разместить новый трим рядом с существующим #271-тримом и в той же фазе потребителей — разумный дефолт, не создающий новой архитектурной сущности. Снимаю с записью, правки не требую. + +## Что проверено и корректно + +- **Продуктовая рамка (§0 ТЗ).** Сценарий и «до/после» присутствуют, персона (Home admin, десктоп-редакторы) и связь с J1/J6 (`docs/SCOPE.md`) не нарушены; задача не расширяет скоуп — это точечный визуальный баг-фикс геометрии кладки, ранее принятой владельцем как дефект (#309 follow-up). +- **Решения владельца перенесены точно.** §1 ТЗ («полный mitre без лимита в узлах-двойках», «зубец срезается продолжением грани острия») буквально совпадает с текстом обоих комментариев владельца в issue — не переформулировано и не дополнено собственными догадками автора. +- **Технический контракт §2.1 проверен построчно по коду.** Утверждение «mitre-патч строится как до #309: `[node, pA, hit, pB]` без проверки `VISUAL_MITRE_LIMIT` и без `chamferApex`» точно описывает нынешний код (`wall-thickness.ts:1193-1197`) с одной заменой: убрать применение `VISUAL_MITRE_LIMIT`/`chamferApex` именно в этой ветке. Утверждение «`MITRE_LIMIT` в парной ветке не применяется» тоже подтверждено — `lineIntersect` (строка 1339) не использует `MITRE_LIMIT` вовсе, он и раньше не участвовал в парной ветке. +- **Технический контракт §2.2 (торцевой трим) обоснован реальной геометрией, а не выдумкой.** `linearWallBody` (строка 1062) действительно строит плоский прямоугольный торец, перпендикулярный оси стены — источник «зубца» у толстой стены при остром угле подтверждён кодом, а не является догадкой автора, выданной за факт. Границы «вдоль оси не дальше `2·halfDepth`(своей)» однозначны и проверяемы. +- **Сверка узлов фикстуры вручную.** Три узла `309-junction-teeth` — это `step` (4 луча), `spike` (2 луча) и `hump` (3 луча); заявление §0/§2.3 «step/hump не трогаются, потому что там узлы ≥3 лучей» подтверждено пересчётом координат, а не принято на веру. +- **Перечень потребителей парных патчей (§3 скоуп) полон.** `linearWallJoinPatches` имеет ровно три вызывающих места (превью открытого контура в `drawWallPreviewD`, `physicalBodyParts`, интерактивное превью протяжки в `houseplan-card.ts:19961`), а `physicalBodyParts(...).all` — единственный источник тела партиций для статического/полного 2D-рендера (`space-render.ts:236`), 2.5D-изо (`houseplan-card.ts:5330`), физической идентичности (`houseplan-card.ts:13990`) и барьеров света (`houseplan-card.ts:16258`). Заявление §4 «меняется форма кладки на всех поверхностях через общий структурный кэш» подтверждено — не общие слова. +- **Не-скоуп корректен.** `virtualJunctionPatches`/`unionJunctionPatches` (виртуальные Т-стыки контуров комнат у вырезов) и #249-механизм (`multiWallBevelCutsAt`, ≥3-лучевые узлы) — действительно отдельные, не связанные с узлом-парой механизмы; исключение их из скоупа обоснованно. +- **AC1, AC3, AC4, AC6, AC7 однозначны и имеют реальный, существующий способ доказательства** (проверены имена функций/тестов — `physicalBodySet`, golden-сцены `junction-309-step/hump-dark`, `junction-309-spike-dark`, матрица `demo/golden/matrix.mjs:620-627`, существующий тест-паттерн мутантов). AC2 приемлем при условии, что «проба» и пересъёмка golden-сцены вместе покрывают форму контура визуально и точечно. +- **Риск №1** (возврат длинных «хвостов» на почти встречных стенах) корректно квалифицирован как осознанный и уже исторически принятый (было так до #309); фолбэк для вырожденных пар не меняется и это подтверждено кодом (`lineIntersect` возвращает `null` только при параллельности). +- **Риск №3** (интерференция #271+#310 трима) адекватно закрыт отдельным юнитом «пара с коротким толстым саппортом» в §8 — обе техники применяются к взаимно исключающим степеням узла (#271 требует ≥3 луча, #310 — ровно 2), риск реален только когда один и тот же короткий толстый отрезок стены одним концом попадает в узел-пару, а другим — в ≥3-лучевой узел; тест-план это учитывает. +- **Release-артефакты, откат, UX/i18n/данные (§4, §6, §9)** — полны, корректны, ничего не упущено: нет миграции, нет новых полей, откат — чистый ревёрт геометрии. +- **Трейлеры коммита** (`Issue: #310`, `User-Visible: no`) на месте, класс изменения (C — документация) корректен, отдельный issue на инфраструктуру не нужен. + +## Чего не проверял + +- Полный обход всей `demo/golden/matrix.mjs` на предмет иных 2-лучевых острых пар за пределами трёх названных `junction-309-*`-сцен и владельческого репро — проверил вручную состав сцен `arms:`-семейства (все ≥3 луча) и `junction-owner-repro`/`junction-patch-resilience` (по имени и связи с #302, тоже ≥3-лучевые механизмы), но не гонял `npm run golden:verify` целиком — на этапе ТЗ код не менялся, гонять нечего; это ляжет на код-ревью после реализации. +- Не проверял фактическую работу `chamferApex`/геометрических формул на предмет численных краевых случаев (например, поведение при `sin` близком к нулю в самой парной ветке) — это код-ревью реализации, а не ревью контракта. +- Не запускал `npm run typecheck`/`npm test`/`npm run build` — на этой ветке нет изменений продуктового или тестового кода, гонять эти гейты нечего. +- Не проверял `npm run invariants` — задача не трогает модель данных (нет изменений `layout`, записей толщины, `marker.space`, `open_spans`), только вычисляемую геометрию рендера; инвариант о ключе решёточного ребра неприменим. + +## Итог + +Один блокирующий (High) недочёт: главный названный риск задачи («дыры на границе нового среза») не имеет рабочей проверки, потому что единственный названный для него гейт (`junctionContractHoles`) структурно не видит 2-лучевые узлы — а именно такой узел #310 и меняет. Остальное ТЗ технически точное, продуктовая рамка и решения владельца перенесены без искажений и без выданных за факт догадок. Возврат автору для правки AC5 (и, по желанию, снятого Low-замечания не требуется). Полный повторный разбор при r2 не обязателен, если правка ограничится AC5/§7 — это точечная правка внутри уже проверенного контракта.