diff --git a/docs/reviews/SPEC-REVIEW-275-r1.md b/docs/reviews/SPEC-REVIEW-275-r1.md new file mode 100644 index 00000000..ca2dc4dd --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-275-r1.md @@ -0,0 +1,191 @@ +# SPEC-REVIEW-275-r1 + +- 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`, коммит `762e9f4b323d9d00ae4b230e419b281cd3d82a9a` (ветка `issue/275-multiwall-strip-containment`) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (первый заход — полный разбор по PROCESS.md §2.9) +- Ревьюер: свежая сессия, без устных пояснений автора + +## Скоуп ревью + +Не «согласиться», а найти, где ТЗ невыполнимо или непроверяемо. Проверено: +соответствие `docs/SCOPE.md`, обязательные разделы §7.1 PROCESS.md, +однозначность и доказательность каждого AC, отсутствие догадок, выданных за +факт, и совпадение технических утверждений о причине дефекта с реальным +`src/wall-thickness.ts`. Диапазон изменений `git diff origin/dev...HEAD` +содержит один файл: `docs/specs/275-multiwall-strip-containment.md` (364 +строки, только добавление) — продуктовый код не тронут, что и ожидается на +этапе ревью ТЗ. + +## Как проверялось + +- Прочитаны `docs/SCOPE.md` целиком (J1/J6, персоны, lock-инвариант, + «никогда не удалять файл на догадке») и `AGENTS.md` (классы изменений, + трейлеры, окружения, лимит циклов, шаблон вердикта). +- Прочитан `PROCESS.md` §§1–14 целиком, включая §2.9 (объём повторного раунда + — не применим к r1) и §7.1/§7.2 (обязательные разделы ТЗ, формат вердикта). +- Прочитаны тело issue #275 (полный технический репорт с SHA-256 входов, + точными координатами узлов и числами potерянных sample) и комментарий + «ТЗ готово к ревью». +- Прочитан канонический `docs/WALL-THICKNESS.md` целиком и построчно сверен + с разделом 6 ТЗ (contract `R = 1.25 × H`, `MITRE_LIMIT = 4`, + `ray.support`/`halfDepth`, failure-isolation #197, opening association). +- Прочитан `src/wall-thickness.ts`: + - `MITRE_LIMIT = 4` (L46), `MULTI_WALL_JOIN_LIMIT = 1.25` (L49) — совпадают + с §6 ТЗ; + - `multiWallBevelCutsAt` (L2016–2095) — подтверждён механизм exterior-connector + из #272 (`connectToExterior`), который и создаёт «открытую белую выемку», + описанную в разделе 3 ТЗ; + - `bevelMultiWallBody` (L2125–2219) — подтверждена буквально: `local` строится + как union прямоугольников `ray.supports` (L2153–2174), затем + `local = difference(local, retainedCuts)` (L2183) — ровно операция, которую + ТЗ называет источником дефекта («восстанавливает union… и снова вычитает + pairwise cut уже из этого физического union»); + - геометрия `sqrt(hA²+hB²)` для прямого угла: для равных `H` + `√2·H ≈ 1.414·H > 1.25·H = R` — арифметика ТЗ (раздел 3) верна. +- `grep` по `test/wall-thickness.test.mjs`, `test/golden-matrix.test.mjs`, + `demo/golden/harness.mjs`, `demo/golden/matrix.mjs` — подтверждено: + `enclosedHoles === 0` действительно единственный golden-инвариант (L399, + 416–418 `golden-matrix.test.mjs`; L182, 567, 620 `harness.mjs`), + `retainedWedgeProbe`/`discardedWedgeProbe` из #249/#261 существуют + (L1549–1584 `wall-thickness.test.mjs`) — заявление ТЗ «hole inventory #272 + остаётся дополнительной, не достаточной проверкой» и характеристика тестов + #272 в разделе 3 ТЗ точны, не выдуманы. +- Подтверждено существование всех инструментов, которые ТЗ называет в + разделах 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` (`bug`, `P1`), + #270 — `OPEN` (`tests`/`infra`/`tech-debt`, без статусного `S*`, т.е. вне + процесса разработки). ТЗ ссылается на них только в списке «Связанные + задачи» и не искажает их состояние. +- Сравнена структура ТЗ (13 разделов, нумерация, порядок) с ранее принятым + ТЗ той же подсистемы `docs/specs/272-no-multiwall-holes.md` — идентичный + каркас, включая обязательную строку «**Доказательство:**» на **каждом** + AC1–AC8 без исключения. Предыдущий раунд той же подсистемы + (`SPEC-REVIEW-272-r1`, жёлтый) поймал ровно два дефекта: (а) отсутствие + строки «Доказательство:» на 6 из 8 AC и (б) неопределённый термин + «объявленный exterior sector» в контракте. Проверено прицельно: в #275 оба + класса отсутствуют — AC1–AC8 несут явную строку доказательства, а + разграничение «что можно резать» дано формулой + `effectiveCut = pairwiseCut − requiredStripUnion(node)` (раздел 6.3), а не + словесно неопределённым понятием. +- Гейты этого этапа не прогонялись: код не менялся (диапазон — один + документ), прогон `typecheck`/`test`/`build`/`check-docs` на ревью ТЗ + бессмыслен и не даёт сигнала. + +## Находки + +Ни одной High и ни одной Medium-находки, блокирующей задачу. + +### Low — 1: делегирование доказательства AC3 существующей конвенции хендоффа, не тексту ТЗ + +`docs/specs/275-multiwall-strip-containment.md`, AC3. Доказательство описано +как «локальный отчёт с SHA-256 входов, machine-readable inventory и +PNG-crops», но раздел не называет явно канал передачи этого отчёта ревьюеру +кода — ревьюер (свежая сессия в CI) не имеет доступа ни к `C:\Temp\4\*.json`, +ни к локальной машине автора. Формально это тот же класс вопроса, который +ревью ТЗ обязано ловить («где ТЗ непроверяемо»), но по существу канал уже +задан на уровне процесса, а не этой задачи: PROCESS.md §7.2 требует в +хендофф-комментарии строку «Гейты: `<команда → результат>`» для каждого +прогнанного гейта, и тот же паттерн (прогон на приватных/локальных данных, +отчёт — командой и результатом в issue) уже используется для WSL/Windows +Playwright-прогонов (AGENTS.md, «Running the app / smoke suite», +«`Verified` without a named command… is not evidence»). Отдельного +повторения этого правила в тексте ТЗ не требуется — снимаю без правки +документа, реализация обязана положить хеш/инвентарь/crops AC3 в +хендофф-комментарий по уже действующему шаблону §7.2, и это же придётся +неизбежно проверить на этапе код-ревью. + +### Low — 2: план автотестов не выделен отдельным заголовком + +Формально §7.1 PROCESS.md перечисляет «план автотестов» отдельным пунктом +обязательных разделов, а в ТЗ #275 он не оформлен отдельным заголовком — +содержание распределено между §8 (AC + «Доказательство:» на каждый) и §9 +(«Ожидаемые файлы» с точным списком test/fixture/smoke/golden/mutation +файлов). Содержательно это ровно план автотестов, и точно тот же способ +подачи использован в принятом ранее ТЗ той же подсистемы +(`docs/specs/272-no-multiwall-holes.md`, разделы 8–9) без замечаний по этому +пункту в его собственном ревью. Снимаю без правки: расхождение чисто +формальное, не создаёт непроверяемости ни одного AC. + +## Что проверено и корректно + +- **Полнота обязательных разделов** (§7.1 PROCESS.md): сценарий/персона + (§1), «что человек увидит до/после» (§4, и кратко в конце §1), проблема + (§2–3), scope/не-scope (§5), геометрический контракт (§6), + compatibility/UX/i18n/touch/security/performance (§7 — все явно закрыты + словом «не меняется»/«нет»), AC1–AC8 с «Доказательство:» на каждом (§8), + ожидаемые файлы = план тестов (§9), release-артефакты (§10), риски (§11), + откат (§12), явный блок принятых технических предположений (§13). +- **Технические утверждения о коде не догадка.** Причина дефекта (раздел 3) + и геометрический контракт (раздел 6) построчно сверены с + `src/wall-thickness.ts:46,49,2016–2219` и с `docs/WALL-THICKNESS.md` — + совпадают буквально, включая формулу `sqrt(hA²+hB²)` и её сравнение с + `R = 1.25H`. +- **Утверждения об устаревшем покрытии тестов #272 точны**, а не преуменьшены + или преувеличены: `enclosedHoles === 0` — реальный и единственный + golden-инвариант, подтверждено grep'ом по `golden-matrix.test.mjs` и + `demo/golden/harness.mjs`. +- **Числовые данные не искажены при переносе из issue в ТЗ**: координаты + проблемных узлов, `cell_cm`, число rooms/walls/degree-3+ nodes и число + потерянных samples в разделе 2 ТЗ совпадают посимвольно с телом issue #275. +- **Не-scope сдерживает расползание**: явно исключены изменение persisted + данных, новая эвристика Optimize, отмена finite-ray endpoints #271, + изменение `R`/коэффициента, новый UI/i18n/backend/миграция — ни одна + соседняя подсистема не втянута в задачу. +- **Продуктовых вопросов владельцу нет и не должно быть.** Единственный + продуктовый факт («сохранённая положительная полоса стены не может быть + вырезана renderer-bevel») уже зафиксирован самим владельцем в тексте issue; + все оставшиеся пункты §13 — технические предположения, оспоримые + ревьюером, а не владельцем, что и есть верное место для них по PROCESS.md + §7.1. +- **Приватность данных не нарушена ни на этапе ТЗ, ни в описанном плане**: + ТЗ явно требует anonymized/minimized fixtures в Git (AC1) и запрещает + коммит полных backups/layout/имён комнат (AC7, раздел 2); формулировка + сохраняет прецедент, уже использованный для fixture #249 + (`test/fixtures/249-multiwall-junction.json`). +- **AC6 (mutation) ссылается на реальный существующий инструмент** + `scripts/mutation-gate.mjs`, а не изобретённый для задачи механизм; + негативный мутант («безусловное вычитание pairwise cut из ray-strip union») + сформулирован так, что обязан уронить именно AC1/AC5, а не только + косвенный признак. +- **Структура ТЗ идентична ранее принятой конвенции той же подсистемы** + (`docs/specs/272-no-multiwall-holes.md`) и не повторяет два конкретных + дефекта, пойманных прошлым раундом той же линии задач (`SPEC-REVIEW-272-r1`: + отсутствие строки «Доказательство:», неопределённый термин в контракте). + +## Чего не проверял + +- Не запускал `npm run typecheck`/`npm test`/`npm run build`/ + `node scripts/check-docs.mjs` — на этапе ревью ТЗ продуктовый код не + менялся (диапазон `origin/dev...HEAD` — один файл спецификации), прогон + этих гейтов не даёт сигнала о качестве ТЗ. +- Не пытался воспроизвести или запросить приватные `1.json`/`2.json` — они + не коммитятся по условию задачи и по правилу `docs/SCOPE.md` (пользовательские + данные не публикуются); принял точные числа/координаты, приведённые + владельцем в теле issue, на тех же основаниях, на которых процесс уже + принимал их в ревью #272 (SPEC-REVIEW-272-r1, «Чего не проверял»). +- Не оценивал сложность самого geometric boolean алгоритма, который + напишет исполнитель, — ТЗ прямо и правильно оставляет конкретную реализацию + техническим решением (§13.2, «эквивалентный алгоритм допустим»), и это его + законное место, а не работа ревью ТЗ. +- Не проверял `docs/ARCHITECTURE.md`/`docs/USER-GUIDE.ru.md`/`docs/TESTING.md` + целиком — только подтвердил, что они существуют и что #275 не меняет их + текущий контракт, лишь дополнит тестовые/пользовательские артефакты, которые + эти файлы описывают на верхнем уровне. +- Не оценивал производительность предлагаемого алгоритма эмпирически — + раздел 7 ТЗ корректно называет риск и меру («cached local node work и + prerelease performance gate»), числовой бюджет уже существует вне этой + задачи и не является предметом ревью ТЗ. + +## Вердикт + +Зелёный. Обязательные разделы §7.1 присутствуют, каждый AC1–AC8 однозначен и +несёт явное доказательство, причина дефекта и геометрический контракт +построчно сверены с реальным кодом и каноническим `docs/WALL-THICKNESS.md` и +не являются догадкой, продуктовых вопросов владельцу нет и не должно быть, +scope/не-scope сдерживают задачу от расползания на соседние подсистемы. Обе +находки — Low, сняты решением ревьюера с записью (существующая конвенция +хендоффа §7.2 закрывает канал доказательства AC3; распределение плана +автотестов между §8/§9 — уже принятый прежде формат той же подсистемы).