mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 12:49:56 +00:00
claude[bot]
@@ -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 — это точечная правка внутри уже проверенного контракта.
|
||||
Reference in New Issue
Block a user