diff --git a/docs/reviews/SPEC-REVIEW-275-r2.md b/docs/reviews/SPEC-REVIEW-275-r2.md new file mode 100644 index 00000000..30034066 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-275-r2.md @@ -0,0 +1,96 @@ +# SPEC-REVIEW-275-r2 + +- Issue: [#275](https://github.com/Matysh/houseplan-card/issues/275) — «Multi-wall bevel beta.6 вырезает реальные полосы стен: белые уголки, потеря толщины и крупные провалы» +- Этап: ТЗ на ревью (PROCESS.md §2.4) +- Артефакт ТЗ: `docs/specs/275-multiwall-strip-containment.md`, коммит `62dd02d8234a8d5b9b629e2fd77b1c09f00932ab` (ветка `issue/275-multiwall-strip-containment`) +- Предыдущий заход: r1, `docs/reviews/SPEC-REVIEW-275-r1.md`, вердикт **зелёный**, на коммите `762e9f4b323d9d00ae4b230e419b281cd3d82a9a` +- Заход: r2 · блокирующих циклов израсходовано 0 из 4 (зелёный вердикт r1 не образует цикла, PROCESS.md §4/#227) +- Ревьюер: свежая сессия, без устных пояснений автора + +## Почему это r2, хотя r1 был зелёным + +r1 одобрил формулу `requiredStripUnion(node) − roomGeom = ∅` как единый инвариант для **всех** multi-wall nodes. Сразу после зелёного вердикта автор сам исполнил её буквально на неортогональном fixture #249 и получил контрпример: точка `discardedWedgeProbe (330.3808442725, 148.8560107825)` — уже утверждённо пустая по #249 — обязана вернуться в masonry по формуле r1, потому что лежит внутри union двух raw incident rectangles. Автор без ожидания вердикта вернул issue в `S3-spec`, переписал контракт и снова запросил независимое ревью. Это не отменяет r1 (там не было пропущенной ошибки при тогдашнем объёме проверки — формула была корректно сверена с кодом и работала для всех репортнутых случаев), это новое знание, полученное после него. + +Дельта контракта — не редакционная, а замена **инвариант для всех узлов** → **инвариант только для rectilinear узлов, неортогональные остаются на контракте #249**. Это смена контракта поведения в смысле PROCESS.md §2.10 («разбор остаётся полным, если... смена контракта поведения»), поэтому я провёл полный технический разбор нового раздела 6 и связанных AC, а не только сверку «что изменилось в тексте». Разделы, которых дельта не касается по существу (персона/сценарий §1, compatibility/i18n/security §7 кроме одной переформулированной строки, список файлов §9, release-артефакты §10, rollback §12), не разбирались повторно — см. «Унаследовано из r1». + +## Дельта (`git diff 762e9f4..62dd02d`) + +Только `docs/specs/275-multiwall-strip-containment.md` (140 строк: +93/−47) плюс публикация `docs/reviews/SPEC-REVIEW-275-r1.md` (191 строк, добавление автоматическим шагом, не автором). Изменены: §Статус, §3 (причина — добавлен абзац о контрпримере и rectilinear-классе), §4 (что видит человек), §5 (Входит), §6.1–6.7 (весь геометрический контракт: новый §6.1 классификация, §6.2/6.3 переименованы и сужены до rectilinear, §6.4 переписан как «неортогональный bounded bevel» вместо общей формулы, §6.5–6.7 перенумерованы без изменения смысла), §7 (одна строка переформулирована, смысл не изменён), AC1/AC2/AC3/AC5/AC6/AC7 (сужены до rectilinear/non-rectilinear), §11 (первая строка риска), §13 (пункт 2 переписан). Продуктовый код не тронут: `git diff origin/dev...HEAD` за весь issue — два файла в `docs/`, оба документного класса C. + +## Как проверялось + +- Перечитан диапазон `762e9f4..62dd02d` построчно (см. выше) и весь текущий `docs/specs/275-multiwall-strip-containment.md`. +- Перечитан `docs/reviews/SPEC-REVIEW-275-r1.md` целиком, включая его находки и то, что он уже сверил с кодом. +- Комментарии issue #275 прочитаны целиком: хендофф r1, зелёный вердикт r1, «Авторский контрпример», хендофф r2. +- Заново, от первых принципов, проверена геометрия нового §6.1–6.4 против `src/wall-thickness.ts:2016–2219` (`multiWallBevelCutsAt`, `bevelMultiWallBody`), а не только против текста r1: + - для точно перпендикулярной пары лучей с полутолщинами `hA=hB=H` пересечение офсетных линий (`hit`) лежит в точке `(H, H)` относительно узла — на **границе** объединения двух strip-прямоугольников, а не за её пределами. Треугольник `(pA, pB, hit)`, который безусловно вычитается сейчас, — это ровно тот угол, который у настоящего прямоугольного T/X обязан быть заполнен настоящей кладкой, а не «избыточный mitre-spike» в смысле #249. Отсюда и системный характер бага: `sqrt(hA²+hB²) = √2·H ≈ 1.414H > R = 1.25H` для любых двух равных или сопоставимых толщин — обычный T теряет материал всегда, независимо от конкретных чисел из репорта. + - для «настоящего» избыточного mitre-spike в смысле #249 (острые/тупые углы, не 90°/0°/180°) расстояние до `hit` растёт неограниченно по мере уменьшения угла между лучами и не привязано к сумме полутолщин — это структурно другой случай, для которого bounded bevel #249 остаётся необходимым. Формально: `multiWallBevelCutsAt` пропускает пары с `gap ≈ 0` или `gap ≈ π` (строка `if (!(gap > 1e-9) || gap >= Math.PI - 1e-9) continue;`), но не пары с `gap ≈ π/2`, что и создаёт дефект. + - вывод: разделение «rectilinear — никогда не режем» / «non-rectilinear — режем как раньше» технически обосновано для случаев, которые я могу проверить по формуле, а не является произвольной подгонкой под два репортнутых файла. +- Проверен реальный fixture `test/fixtures/249-multiwall-junction.json`: три инцидентных луча из узла `[0.329166667, 0.141666667]` имеют направления с углами приблизительно `102.3°`, `45°` и `−27.7°` (вычислено из координат `a`/`b` трёх стен) — ни одна пара не параллельна/перпендикулярна в пределах разумного эпсилон. AC2 корректно утверждает «Fixture #249 классифицируется как non-rectilinear» — это не догадка, а проверяемый факт о существующем фикстуре. +- Сверено, что формулировка issue #275 («белые треугольные вырезы в **обычных T-стыках**») уже до ревью характеризует затронутые узлы как ортогональные — новая классификация не придумывает продуктовое требование, а называет техническим термином то, что репортёр (владелец) описал словами. +- Проверено, что список файлов §9, release-артефакты §10, rollback §12, поверхности §7 (кроме перефразированной строки про кэш) не изменились по существу — сверка с r1 не переделывалась. +- Гейты `typecheck`/`test`/`build`/`check-docs` не запускались: диапазон `origin/dev...HEAD` за весь issue содержит только два файла в `docs/reviews/` и `docs/specs/` (документный класс C), продуктовый код не менялся ни в r1, ни в r2 — прогон не даёт сигнала о качестве ТЗ, то же основание, что в r1. + +## Закрытие раунда r1 + +r1 был зелёным без High/Medium — в строгом смысле §2.10 «находок предыдущего раунда» для закрытия нет. Но раунд объявил контрпример к собственному одобренному инварианту, и это единственное содержательное изменение, которое требует явного прослеживания: + +| Что было в r1 | Чем закрыто в r2 | Где это видно | +|---|---|---| +| Одобрен единый инвариант `requiredStripUnion(node) − roomGeom = ∅` для **всех** multi-wall nodes (r1, §6.2–6.3) | Инвариант сужен до **rectilinear** nodes; неортогональные явно исключены и сохраняют контракт #249 | `docs/specs/275-...md` §6.1 (классификация), §6.3 (заголовок «Rectilinear containment»), §6.4 («Неортогональный bounded bevel») | +| AC2 проверял только «area вне strips и bound R» безусловно | AC2 теперь явно требует классификации fixture #249 как non-rectilinear и сохранения его вероятностей/геометрии | AC2, строки 279–285 | +| AC6-мутант отключал «безусловное вычитание pairwise cut из ray-strip union» | Мутант теперь конкретно отключает rectilinear guard | AC6, строки 320–327 | +| Low-1 (канал доказательства AC3) и Low-2 (план тестов не отдельным заголовком), обе снял ревьюер без правки документа | Не затронуты дельтой — обе снятые находки остаются в силе без изменений | `docs/reviews/SPEC-REVIEW-275-r1.md`, разделы «Low — 1/2» | + +## Унаследовано из r1 + +Принято без повторной проверки — не задевается дельтой, проверено на `762e9f4b323d9d00ae4b230e419b281cd3d82a9a` в `docs/reviews/SPEC-REVIEW-275-r1.md`: + +- полнота обязательных разделов §7.1 PROCESS.md кроме изменённых §3/§4/§5/§6/AC1-2-3-5-6-7/§11/§13 (персона/сценарий §1, i18n/compat/security/touch §7, план тестов §8+9, release-артефакты §10, риски кроме первой строки §11, rollback §12); +- существование инструментов, названных в §8–9: `scripts/mutation-gate.mjs`, `demo/smoke_multiwall_junction.mjs`, `scripts/smoke-select.mjs`, `demo/golden/harness.mjs`, `demo/golden/matrix.mjs` — ни один не выдуман; +- статусы связанных issue (#271/#272/#273 CLOSED, #270 OPEN вне процесса) и точность их упоминания; +- точность переноса числовых данных из тела issue в раздел 2 ТЗ (координаты, `cell_cm`, число rooms/walls/nodes/samples) — не менялся дельтой; +- приватность данных: анонимизированные fixtures в Git, полные backups вне репозитория — тот же контракт, AC7 лишь добавил один негативный кейс; +- Low-1 и Low-2 из r1 (см. таблицу выше) — сняты ревьюером r1, дельта их не касается, повторно не поднимаю. + +## Находки + +### Medium — 1: контракт не определяет узлы, где ray-пары смешивают rectilinear и non-rectilinear + +`docs/specs/275-multiwall-strip-containment.md`, §6.1/§6.3/§6.4. Классификация — свойство **узла целиком**: rectilinear только если **каждая** пара лучей параллельна/перпендикулярна (§6.1: «Node является rectilinear, если для **каждой** пары направлений...»). Если хотя бы одна пара не проходит тест, узел целиком становится non-rectilinear и получает **весь** старый bounded-bevel #249 — включая пары, которые сами по себе перпендикулярны. + +**Сценарий отказа:** 4-лучевой узел — прямая внутренняя перегородка (север 0°, восток 90°) плюс диагональная стена под 45° (эркер, лестничный марш, скошенный угол). Пары `(север, восток)` = 90° (rectilinear-пара), `(север, диагональ)` = 45°, `(восток, диагональ)` = 45° — обе non-rectilinear. По §6.1 весь узел классифицируется non-rectilinear → пара `(север, восток)` получает старый pairwise cut → для равных полутолщин `sqrt(hA²+hB²) ≈ 1.414H > R = 1.25H` → ровно тот же белый треугольник/потеря толщины, из-за которого заведён #275, воспроизводится на этом узле, хотя весь остальной фикс формально «работает». + +Это не гипотетическая придирка к формуле: `docs/WALL-THICKNESS.md` явно допускает узлы «с тремя или более различными инцидентными лучами» без ограничения на то, что все они должны быть либо все ортогональны, либо все под общим углом — контракт файла не запрещает смешанные узлы, а значит их не запрещает и продукт. AC1 покрывает только чисто rectilinear T/X, AC2/AC3 — только целиком non-rectilinear fixture #249 и репортнутые узлы (которые issue называет «обычными T-стыками», то есть предположительно чисто rectilinear). Ни один AC не называет смешанный узел, а §5 «Не входит» не исключает его явно. + +**Почему в скоупе, а не отдельный issue:** это тот же контракт §6, который сейчас переписывается — расширить классификацию до попарной (exempt только rectilinear-пары, а не весь узел) или явно исключить смешанные узлы с обоснованием (например, «не встречаются в репортнутых backups, доказано по факту N узлов и их углам») — техническое решение автора, а не соседняя подсистема. + +**Что нужно:** либо (a) сузить проверку §6.3 до уровня пары, а не узла — «pairwise cut не строится для пары, которая сама parallel/perpendicular, независимо от классификации остальных пар того же узла», с соответствующей правкой AC1/AC6/AC7, либо (b) явно исключить смешанные узлы в §5/§13 с доказательством по фактическим backups (AC3 может это подтвердить: «ни один из 12 репортнутых узлов не смешанный» — при условии, что это действительно так, и добавлением негативного теста на смешанный синтетический fixture, документирующего оставшееся поведение как известное ограничение). + +Без High-находок это делает вердикт жёлтым: находка в скоупе задачи, чинится в текущем issue (PROCESS.md §2.4/§4), отдельный issue не заводится. + +### Low — 1 (снимаю без правки документа): величина angle-epsilon в §6.1 не названа числом + +§6.1 вводит «единый angle-epsilon» для теста `|dot| ≈ 1`/`|dot| ≈ 0`, не называя конкретное значение или формулу (в отличие от именованных констант кода — `MITRE_LIMIT = 4`, `MULTI_WALL_JOIN_LIMIT = 1.25`, или уже существующего допуска openings «`max(4% × grid pitch, 1e-9)`» в `docs/WALL-THICKNESS.md`). Технически это решение реализации (§13.5 ТЗ прямо оставляет технические детали за исполнителем), и AC7 прямо требует «негативную angle-boundary matrix», которая обязана уронить тест при слишком большом эпсилон — то есть непроверяемости здесь нет, только отложенное числовое решение. Снимаю без правки текста: то же основание, на котором r1 принял оставление алгоритма бевела как «эквивалентная реализация допустима». + +## Что проверено и корректно + +- Новая rectilinear-классификация (§6.1) технически обоснована не только текстом ТЗ, но и независимым пересчётом геометрии для перпендикулярного случая (точка `hit` лежит на границе union strip-прямоугольников, а не за её пределами) и для острого/тупого случая (расстояние до `hit` растёт неограниченно с уменьшением угла и структурно отличается от rectilinear случая) — это не подгонка под два репортнутых файла, а разделение двух разных геометрических режимов существующей формулы. +- AC2 корректно и проверяемо утверждает, что fixture #249 — non-rectilinear: три инцидентных луча этого фикстура образуют углы, ни один из которых не близок к 0/90/180° (вычислено из фактических координат стен в `test/fixtures/249-multiwall-junction.json`), это не догадка. +- §6.7 закрывает конкретную дыру, которую наивная реализация могла бы оставить: failure-fallback не может тихо вернуть rectilinear-узел на старый pairwise-cut путь — если бы этой строки не было, unit-тест на failure isolation не имел бы, что проверять именно для этого случая. +- §6.3 явно запрещает частичное применение классификации внутри уже rectilinear узла («недостаточно пропустить только одну 90° пару и оставить соседний cut того же T/X») — устраняет один класс наивной реализации, отличный от Medium-находки выше (та — про узлы, которые НЕ прошли классификацию целиком, эта — про узлы, которые прошли). +- Продуктовых вопросов владельцу нет и не должно быть: единственный продуктовый факт (сохранённая стена не может быть вырезана renderer-bevel) зафиксирован владельцем в теле issue, различение rectilinear/non-rectilinear — техническое уточнение уже одобренного продуктового требования, а не новый вопрос ему. +- Дельта не расширяет и не сужает scope искусственно: «Не входит» (§5) не изменился по смыслу, никакая соседняя подсистема (i18n, backend, миграция, UI) не затронута новым текстом. +- Числа/координаты, перенесённые из issue в разделы 2–3, не искажены и не изменены дельтой r1→r2 (сверено построчно). + +## Чего не проверял + +- Не запускал `npm run typecheck`/`npm test`/`npm run build`/`node scripts/check-docs.mjs` — весь диапазон `origin/dev...HEAD` за issue #275 (r1 и r2) состоит из двух файлов `docs/**`, продуктовый код не менялся; прогон этих гейтов не даёт сигнала о качестве ТЗ. То же основание, что в r1. +- Не запрашивал и не пытался воспроизвести приватные `1.json`/`2.json` — они не коммитятся по условию задачи; принял утверждение «все 12 измеренных потерь лежат в rectilinear T/X nodes» на тех же основаниях, на которых процесс уже принимал числовые данные владельца в #272/#275-r1 («Чего не проверял», SPEC-REVIEW-275-r1). Если это утверждение неточно (среди 12 узлов есть смешанные), Medium-находка выше становится не гипотетической, а актуальной для самих репортнутых файлов — но это не меняет того, что находка должна быть закрыта независимо от того, встречается она в этих двух конкретных файлах или нет. +- Не оценивал сложность или производительность конкретного алгоритма классификации, который напишет исполнитель — §13.5 прямо и правильно оставляет это реализации, это её законное место. +- Не проверял повторно `docs/ARCHITECTURE.md`/`docs/USER-GUIDE.ru.md`/`docs/TESTING.md` — не тронуты дельтой, r1 уже подтвердил, что #275 не меняет их текущий контракт. +- Не проверял `docs/reviews/SPEC-REVIEW-275-r1.md` как предмет ревью (это не артефакт этого раунда, а отчёт предыдущего) — только использовал его как источник для «Унаследовано» и «Закрытие r1». + +## Вердикт + +Жёлтый. High-находок нет. Одна Medium-находка в скоупе задачи (контракт §6 не определяет поведение для multi-wall узлов, у которых часть ray-пар rectilinear, а часть — нет, и молча возвращает такие узлы на старый баг-путь для их ортогональных пар) — чинится в текущем issue без отдельного issue (PROCESS.md §2.4/§4/#202). Одна Low-находка снята решением ревьюера без правки документа (эпсилон — техническая деталь, защищённая требуемым AC7 негативным тестом). Новая rectilinear/non-rectilinear классификация в остальном технически обоснована и проверена как против кода (`src/wall-thickness.ts`), так и от первых принципов геометрии, а не является повторением прежней догадки под новым именем.