mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 12:18:51 +00:00
claude[bot]
@@ -0,0 +1,259 @@
|
||||
# SPEC-REVIEW-197-r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/197
|
||||
- **ТЗ под ревью:** [`docs/specs/197-junction-patch-fail-dark.md`](https://github.com/Matysh/houseplan-card/blob/issue/197-junction-patch-fail-dark/docs/specs/197-junction-patch-fail-dark.md)
|
||||
(коммит `fe7b28f`), обычный трек — не `small`/`trivial`
|
||||
- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review`
|
||||
- **Трек:** обычный, лимит циклов ревью ТЗ — 4 (§4 PROCESS.md)
|
||||
- **Цикл:** r1/4
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
ТЗ #197 предлагает исправление: один вырожденный `virtualJunctionPatches()`
|
||||
patch (шум IEEE-754 в координатах, из-за которого `union()` в `polyclip-ts`
|
||||
бросает исключение) обнуляет весь результат `wallBodiesGeometry()`, поскольку
|
||||
цикл добавления junction-патчей (`src/wall-thickness.ts:1760-1761`) — в отличие
|
||||
от соседних циклов room-ring и wall-edge — не изолирует отказ одного элемента.
|
||||
Заявленное решение: численная стабилизация координат patch + per-patch
|
||||
try/catch, без изменения persisted-схемы, i18n, touch-контракта, оптимизатора
|
||||
и без правки #198/#199 (сознательно вынесены отдельно).
|
||||
|
||||
Не в скоупе ревью: продуктовый код. На ветке `issue/197-junction-patch-fail-dark`
|
||||
лежит один коммит (`fe7b28f`, `Issue: #197 · User-Visible: no`) — только
|
||||
ТЗ и правка `docs/specs/README.md`, реализации нет. Гейты
|
||||
(`typecheck`/`test`/`build`) не прогонялись — на этапе ревью ТЗ продуктового
|
||||
кода не существует, что вне скоупа этапа (PROCESS.md §2.4/§8). Вместо этого
|
||||
проверялась исполнимость **фактических утверждений** ТЗ о текущем поведении
|
||||
`src/wall-thickness.ts` на `origin/dev`, поскольку именно они — единственное
|
||||
обоснование, почему решение верно и почему AC1/AC2 будут доказывать то, что
|
||||
заявлено.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитаны целиком `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (действующая
|
||||
редакция, §1, §2.2, §2.4, §5, §7.1, §8) и `docs/WALL-THICKNESS.md`.
|
||||
2. Прочитано тело issue #197 и все три комментария (аналитика владельца —
|
||||
ценность 9/9, сложность 6, риск 8, `P2`, `bug`, обычный трек; занятие;
|
||||
хендофф на ревью).
|
||||
3. Прочитан весь текст ТЗ (`docs/specs/197-junction-patch-fail-dark.md`,
|
||||
409 строк) и сверены обязательные разделы §7.1 PROCESS.md — присутствуют
|
||||
все: сценарий и персона (§1), что человек увидит до/после (§2), проблема и
|
||||
причина (§3), скоуп/не-скоуп (§5/§6), контракт поведения (§7), UX/i18n/touch
|
||||
(§10), модель данных и миграция (§9), AC1…AC11 с доказательством (§11), план
|
||||
автотестов (§12), риски (§14), откат (§15), release-артефакты (§16). Плюс
|
||||
продуктовые разделы (персона/поверхность/момент; фраза «что увидит» без
|
||||
терминов реализации) — на месте.
|
||||
4. Прочитан код `src/wall-thickness.ts` (`wallBodiesGeometry`,
|
||||
`virtualJunctionPatches`, `wallIntervals`, `openEps`) и подтверждено
|
||||
построчно: room-ring loop (`:1730-1739`) и wall-edge loop (`:1744-1756`)
|
||||
имеют per-piece `try/catch`; junction-patch loop (`:1760-1761`) — нет, и
|
||||
любое исключение из его `union()` долетает до общего `catch` на `:1790`,
|
||||
обнуляя весь `wallBodiesGeometry()`. **Совпадает с диагнозом ТЗ дословно.**
|
||||
5. Прочитан `docs/specs/141-wall-junctions.md` (`git fetch` тело issue #141
|
||||
через `gh issue view` показало не то, что ожидалось — актуальный заголовок
|
||||
issue #141 на GitHub («Стыки перегородок под углом остаются с зубцами») не
|
||||
про virtual-junction fail-dark; проверка ушла на уровень спецификации,
|
||||
которая шире исходного тикета). Раздел 7.7 «Ошибки вычисления» этой
|
||||
спецификации действительно фиксирует общий архитектурный контракт: «Malformed
|
||||
legacy input не должен превращать видимую стену в прозрачность… Boolean
|
||||
failure не разрешается маскировать исчезновением кладки». Ссылка ТЗ #197 на
|
||||
#141 как источник продуктового контракта **обоснована** — это не догадка,
|
||||
выданная за решение, а корректная опора на уже принятый архитектурный
|
||||
принцип, просто зафиксированный в спеке, а не в теле issue.
|
||||
6. **Ключевая проверка — самостоятельное исполнение заявленной причинно-следственной
|
||||
цепочки** (см. «Находки», High-1). Собран `test-build/` (`npx tsc -p
|
||||
tsconfig.test.json && node scripts/fix-test-build.mjs`), написан скрипт,
|
||||
воспроизводящий ровно вызов, который ТЗ приводит в §3: «комнаты ×NORM_W,
|
||||
затем `wallBodiesGeometry(rooms, walls, openCuts, [], GRID_STEP_N, 5,
|
||||
GRID_PITCH, NORM_W, [])`» — с полным fixture из issue (проверено: 8 комнат,
|
||||
25 walls, 3 open_spans, совпадает с заявленным). Результат разошёлся с
|
||||
заявленным; подробности и повторяемая команда — в находке.
|
||||
7. Проверена структурная целостность ссылок ТЗ: #198 и #199 существуют,
|
||||
открыты, не дубликаты (`gh issue view 198/199`), в `S1-new`, что
|
||||
соответствует «смежные находки, аналитика ещё впереди» — согласуется с
|
||||
текстом ТЗ, который явно выносит их из скоупа.
|
||||
8. AC1–AC11 прочитаны на однозначность и способ доказательства: у каждого
|
||||
указан метод (`unit`/`smoke`/`golden`/`ревью кода`/сочетание), формулировки
|
||||
не допускают двух прочтений, кроме зависимости AC1/AC2 от fixture, разобранной
|
||||
в находке High-1.
|
||||
|
||||
## Находки
|
||||
|
||||
### High-1 — центральное фактическое утверждение ТЗ (единственный патч →
|
||||
крах `union()` → `null`) не воспроизводится точным сценарием, который сам ТЗ
|
||||
описывает как «подтверждено исполнением»
|
||||
|
||||
**Файл:** `docs/specs/197-junction-patch-fail-dark.md`, §3 «Подтверждённая
|
||||
проблема и причина» (и производные от неё AC1, AC2 в §11).
|
||||
|
||||
**Что заявлено.** §3 ТЗ и аналитический комментарий владельца в issue
|
||||
утверждают: на `origin/dev` `19e92e0`, с анонимизированным fixture из issue и
|
||||
вызовом `wallBodiesGeometry(rooms, walls, openCuts, [], GRID_STEP_N, 5,
|
||||
GRID_PITCH, NORM_W, [])` (комнаты предварительно умножены на `NORM_W = 1000`),
|
||||
`virtualJunctionPatches()` создаёт **ровно один** patch, а
|
||||
`wallBodiesGeometry(...)` возвращает `null` — то есть дефект «подтверждён
|
||||
исполнением», а не выведен логически. На этом стоят AC1 («тест красный на
|
||||
исходном dev, где результат равен `null`») и AC2 («fixture создаёт ровно один
|
||||
ожидаемый virtual-junction patch»).
|
||||
|
||||
**Что получено при независимом повторении.** HEAD этой ветки (`fe7b28f`) не
|
||||
содержит изменений в `src/` относительно `19e92e0`
|
||||
(`git diff 19e92e0 fe7b28f --stat -- src/` — пусто), то есть повторение идёт
|
||||
на том же коде, который тестировал автор. Собран `test-build/` тем же
|
||||
способом, каким его собирает `npm test` (`npx tsc -p tsconfig.test.json &&
|
||||
node scripts/fix-test-build.mjs`). Взят фикстур **дословно** из тела issue
|
||||
#197 (проверено программно: 8 `rooms`, 25 `walls`, 3 `open_spans` — совпадает
|
||||
с заявленным), выполнено ровно то преобразование и тот вызов, которые
|
||||
описывает §3 ТЗ:
|
||||
|
||||
```js
|
||||
import { wallBodiesGeometry, virtualJunctionPatches } from './test-build/wall-thickness.js';
|
||||
import { GRID_STEP_N, GRID_PITCH, NORM_W } from './test-build/space-geometry.js';
|
||||
|
||||
const coordScale = 1000;
|
||||
const rooms = fixture.rooms.map(r => ({ ...r, poly: r.poly.map(([x, y]) => [x * coordScale, y * coordScale]) }));
|
||||
const walls = fixture.walls.map(w => ({ ...w, a: [w.a[0] * coordScale, w.a[1] * coordScale], b: [w.b[0] * coordScale, w.b[1] * coordScale] }));
|
||||
const openCuts = fixture.open_spans.map(s => [s.a[0] * coordScale, s.a[1] * coordScale, s.b[0] * coordScale, s.b[1] * coordScale]);
|
||||
|
||||
virtualJunctionPatches(rooms, walls, openCuts, GRID_STEP_N, fixture.cell_cm, GRID_PITCH, coordScale);
|
||||
// -> [] (0 патчей, не 1)
|
||||
|
||||
wallBodiesGeometry(rooms, walls, openCuts, [], GRID_STEP_N, fixture.cell_cm, GRID_PITCH, coordScale, []);
|
||||
// -> { geom, paperGeom, depthUnits, openingIndex } — НЕ null, area(geom) ≈ 99518.06
|
||||
```
|
||||
|
||||
Результат воспроизводится детерминированно (запускался многократно, всегда
|
||||
одинаково): **ноль** virtual-junction patches вместо заявленного одного, и
|
||||
**валидная непустая geometry** вместо `null`. Проверены дополнительные
|
||||
вариации масштаба (координаты в конфиг-пространстве `coordScale = 1`, разные
|
||||
сочетания `pitch`/`gridPitch`) — ни один вариант, соответствующий описанным в
|
||||
ТЗ или в коде конвенциям масштабирования (комментарий `src/wall-thickness.ts:115-116`
|
||||
«`coordScale = NORM_W` with `pitch = GRID_STEP_N`… config-space edges use
|
||||
`coordScale = 1`»), не воспроизводит `null`.
|
||||
|
||||
**Расследование, почему patch не строится.** У узла `[620.833…, 550]` (реальный
|
||||
T между стенами 20 см — тот самый, что описан в issue как место дефекта)
|
||||
`virtualJunctionPatches()` требует ⩾2 «касающихся» интервалов с `half > 0`.
|
||||
Прямая проверка `thicknessCmAt()`/`lookupWall()` на этом fixture показала: сама
|
||||
стена `{"key":"0.754167,0.550000@0.0000","cm":20,"a":[0.6208…,0.55],
|
||||
"b":[0.8875,0.55]}` корректно разрешается `lookupWall()` на **весь** пролёт, но
|
||||
`thicknessCmAt()` в его середине после атомарного разбиения на подынтервалы
|
||||
(разбиение возникает из-за частичного перекрытия room_6 и room_8 вдоль этого
|
||||
же `y = 0.55`) возвращает **0**, а не 20/22. Из-за этого у узла остаётся только
|
||||
один интервал с `half > 0` (вертикальная стена), патч не формируется вовсе —
|
||||
крах `union()`, который заявлен как причина, физически не может произойти,
|
||||
потому что до него код не доходит. Это отдельный, самостоятельно
|
||||
воспроизводимый дефект (заведён как #201, `bug`/`P2`/`S1-new`, со ссылкой на
|
||||
#197) — сам по себе вне скоупа #197, но именно он «съедает» патч, который ТЗ
|
||||
считает существующим и падающим.
|
||||
|
||||
**Почему это блокирует, а не просто интересное наблюдение.** AC1 и AC2 — не
|
||||
частности, они физически формируют regression-тест, который должен «уметь
|
||||
падать» на незащищённом коде (обязательное условие §12 ТЗ и §2.7 PROCESS.md).
|
||||
Если фикстур, как описано в §3, не даёт ни patch, ни `null`, разработчик не
|
||||
сможет написать red-тест по AC1/AC2 буквально по тексту ТЗ — а раз этот тест не
|
||||
может упасть на исходном коде, он не доказывает то, ради чего задача
|
||||
существует (PROCESS.md §2.7: «ревьюер убедился, что тест умеет падать»).
|
||||
Дальше есть три равно вероятных объяснения, и без ответа на них разработка
|
||||
рискует чинить не тот код:
|
||||
|
||||
1. reproduction procedure в ТЗ (деление исходных production-координат на
|
||||
`NORM_W` при анонимизации → обратное умножение на `NORM_W` в тесте) не
|
||||
гарантирует битовое совпадение с исходными числами: деление и умножение на
|
||||
1000 не обязаны быть точным round-trip для произвольного `double`, а весь
|
||||
сюжет issue — именно про чувствительность `union()` к разнице в 1 ULP.
|
||||
Тогда нужен **другой** способ передать fixture в тест (например, без
|
||||
промежуточного деления/анонимизации через `NORM_W`, а с координатами,
|
||||
зафиксированными на исходном масштабе);
|
||||
2. #201 (тихое обнуление толщины на частичном перекрытии трёх комнат) —
|
||||
самостоятельная причина, из-за которой на ЭТОМ конкретном fixture
|
||||
заявленный патч не возникает; после её исправления сценарий ТЗ может
|
||||
воспроизвестись как заявлено — но тогда порядок работ («что делаем
|
||||
первым») из продуктового становится техническим вопросом, который ТЗ #197
|
||||
не рассматривает вовсе;
|
||||
3. диагноз §3 ТЗ в целом требует уточнения на заново подтверждённом fixture
|
||||
(не обязательно неверен по существу — код действительно не изолирует
|
||||
`union()` одного patch, это подтверждено чтением кода независимо от
|
||||
fixture, — но опора именно на «дефект воспроизведён исполнением» с этими
|
||||
конкретными числами недостоверна).
|
||||
|
||||
**Рекомендация.** Прежде чем issue уходит в `S5-ready`, автор обязан:
|
||||
заново подтвердить `null`/один-patch на fixture так, чтобы это мог повторить
|
||||
кто угодно (например, приложив ровно тот код воспроизведения, который
|
||||
использовался, а не только текстовое описание шагов), либо скорректировать
|
||||
§3/AC1/AC2 под fixture, который действительно падает на `origin/dev`.
|
||||
Технический характер причины (round-trip координат) не делает находку
|
||||
продуктовым вопросом владельцу — это ровно тот случай, который ревью решает
|
||||
сам (PROCESS.md §7.1): «Всё, чего пользователь не наблюдает, агенты решают
|
||||
сами». Продуктовый контракт §4/§7 ТЗ (patch fail не должен гасить кладку) не
|
||||
оспаривается и, по прочтению кода независимо от fixture, обоснован верно —
|
||||
речь только о доказательной базе AC1/AC2 на этом конкретном наборе данных.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Диагноз механизма отказа (без привязки к конкретному fixture).**
|
||||
Отсутствие per-piece `try/catch` вокруг junction-patch loop
|
||||
(`src/wall-thickness.ts:1760-1761`) в отличие от двух соседних циклов —
|
||||
подтверждено чтением кода дословно, независимо от вопроса, воспроизводит ли
|
||||
именно этот fixture крах. Это реальный, объективно существующий разрыв в
|
||||
изоляции отказа, и предложенное исправление (§7.2 ТЗ: transactional
|
||||
per-patch fallback) адресует именно его.
|
||||
- **Продуктовый контракт.** Ссылка на `docs/specs/141-wall-junctions.md` §7.6/§7.7
|
||||
как источник уже принятого архитектурного решения («boolean failure не
|
||||
маскирует исчезновение кладки») подтверждена чтением этого документа —
|
||||
не выдумана и не выдана вслепую за факт.
|
||||
- **Полнота обязательных разделов ТЗ.** Все разделы §7.1 PROCESS.md на месте,
|
||||
однозначны, у каждого AC указан способ доказательства.
|
||||
- **Скоуп/не-скоуп.** Границы с #198 (вне-сеточный micro-interval) и #199
|
||||
(self-check Optimize) проведены чётко и не пересекаются с #197.
|
||||
- **Трек.** Обычный трек обоснован верно (сложность 6, риск 8, несколько
|
||||
поверхностей) — `small`/`trivial` были бы нарушением критериев §5/§5.1
|
||||
PROCESS.md.
|
||||
- **Технические предположения (§17).** Помечены явно как «assumed, change
|
||||
freely» в соответствии с PROCESS.md §7.1 — никакой техническое решение не
|
||||
выдано за продуктовый факт без пометки.
|
||||
- **Модель данных, i18n, touch, миграция, откат, release-артефакты** —
|
||||
корректно отражают «без изменений», согласуется с характером задачи (чистая
|
||||
геометрическая правка без нового UX).
|
||||
- **Открытых продуктовых вопросов к владельцу не осталось** — единственный
|
||||
найденный пробел (High-1) технический, а не продуктовый: ни «что видит
|
||||
пользователь», ни «объём видимых изменений» здесь не решаются, значит, по
|
||||
PROCESS.md §7.1 он не эскалируется владельцу, а решается автором и ревью.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- **Численная стабилизация (§7.1 контракт, §17.1 tolerance).** Не проверялось
|
||||
экспериментально, действительно ли округление patch-координат с точностью
|
||||
`coordScale × 10⁻¹²` (или альтернатива) устраняет крах `union()` **в
|
||||
реальном крашащем сценарии** — поскольку у меня на fixture из issue крах не
|
||||
наступил вовсе (High-1), проверить «лечит ли предложенный numeric step
|
||||
именно эту причину» было нечем. Требует повторной проверки после разбора
|
||||
находки High-1.
|
||||
- **AC3–AC8, AC10, AC11 на уровне реального прогона** — не запускал `npm
|
||||
test`/`npm run build`/browser smoke: на этапе ревью ТЗ продуктового кода нет
|
||||
(только два doc-файла в коммите), прогон гейтов реализации вне скоупа этапа
|
||||
(PROCESS.md §2.4/§8). Формулировки этих AC оценивались только на
|
||||
однозначность текста и адекватность заявленного метода доказательства.
|
||||
- **Полный набор regression-фикстур #123/#141/#150/#172** (AC6) — не
|
||||
прогонялся; полагался на то, что они уже зелёные на `dev` (не тронуты этим
|
||||
ТЗ) и что их перечисление в AC6 корректно охватывает риск регрессии.
|
||||
- **golden/визуальная сцена (AC9)** — не создавалась и не оценивалась
|
||||
визуально: на этапе ТЗ сцены не существует.
|
||||
- **Полный causal chain #201** — заведённый по итогам этого ревью issue
|
||||
зафиксирован на уровне voспроизведённого факта (`thicknessCmAt` возвращает
|
||||
0 для корректно объявленного интервала при частичном перекрытии трёх
|
||||
комнат), но не диагностирован до конкретной строки/функции-виновника;
|
||||
это аналитика #201, не эта работа.
|
||||
|
||||
## Вердикт
|
||||
|
||||
High: 1 (центральное фактическое обоснование AC1/AC2 не подтверждено
|
||||
независимым исполнением точно описанного в ТЗ сценария) · Medium: 0 (found
|
||||
issue #201 filed separately, tracked on its own, not blocking #197's spec as
|
||||
a Medium gap in the text) · Low: 0.
|
||||
|
||||
**Красный.** ТЗ возвращается автору для повторной проверки причинно-следственной
|
||||
цепочки на исполняемом fixture (см. рекомендацию в High-1) и корректировки
|
||||
§3/AC1/AC2 по результату — либо доказательством, что первоначальный fixture
|
||||
всё же воспроизводит `null` иным путём вызова, либо заменой fixture/подхода на
|
||||
тот, что действительно падает на `origin/dev`.
|
||||
Reference in New Issue
Block a user