From 327c35f606e07da3aac16849f5d979c9fb7e2d63 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:35:29 +0000 Subject: [PATCH] docs: spec review for #197 Issue: #197 User-Visible: no --- docs/reviews/SPEC-REVIEW-197-r1.md | 259 +++++++++++++++++++++++++++++ 1 file changed, 259 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-197-r1.md diff --git a/docs/reviews/SPEC-REVIEW-197-r1.md b/docs/reviews/SPEC-REVIEW-197-r1.md new file mode 100644 index 00000000..3af8da5b --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-197-r1.md @@ -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`.