diff --git a/docs/reviews/SPEC-REVIEW-172-r1.md b/docs/reviews/SPEC-REVIEW-172-r1.md new file mode 100644 index 00000000..09aeb3ab --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-172-r1.md @@ -0,0 +1,272 @@ +# SPEC-REVIEW-172-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/172 +- **ТЗ под ревью:** `docs/specs/172-zero-divider-taper.md` (коммит `4582628`, + ветка `issue/172-zero-divider-taper`) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный (не `small`/`trivial`) — сложность/риск 6/7 из 10, задача + задевает более одной поверхности (Plan, View/kiosk/static, hidden Iso, + clean-floor/room fills, Glow/sun) и физическую геометрию, потребляемую всем + рендером; критерии лёгкого трека (§5 PROCESS.md: одна поверхность, риск ≤3) + не выполняются ни по одному пункту — полный трек и файл ТЗ выбраны верно. +- **Цикл:** r1/4 + +## Скоуп ревью + +Проверялось соответствие ТЗ: + +- `docs/SCOPE.md` — попадание в Core user jobs (J4/J6), отсутствие расширения + скоупа за пределы описанного дефекта; +- `PROCESS.md` §2.4/§2.5 (DoR), §7.1 (обязательные разделы ТЗ), §5 (критерии + лёгкого трека), §3/§12 (запреты, включая «догадка вместо решения»); +- `AGENTS.md` — классы файлов, имя ветки, трейлеры коммита ТЗ; +- каноническому документу подсистемы `docs/WALL-THICKNESS.md` (модель + толщины, growth ±½, mitre/bevel-контракт, единый источник геометрии для + всех потребителей); +- `docs/USER-GUIDE.ru.md` — терминология инструмента «Split»; +- фактическому коду `src/wall-thickness.ts` (`insetContour()`, + `outsetContour()`, `MITRE_LIMIT`) — чтобы диагноз причины в ТЗ не оказался + непроверенной догадкой, выданной за факт; +- полному треду issue #172 — аналитика Codex, вопросы Q1–Q4 с default'ами, + решение владельца, финальный комментарий автора со ссылкой на ТЗ. + +## Как проверялось + +1. Прочитан весь тред issue #172: исходный баг-репорт пользователя (Г-образная + комната, Split из внутреннего угла с отклонением 0,5–1° от нормали, один + конец разделителя — толщина 0, другой — толщина примыкающей стены), + аналитика Codex (воспроизведение на `origin/dev` `a05aa5d` при углах `0°`, + `0,477°`, `0,955°`, `1,909°`, `9,462°`, `26,565°`; при точном `0°` дефекта + нет), явные вопросы Q1–Q4 с предложенными default'ами, ответ владельца + «принимаю все defaults» и финальная публикация ТЗ. +2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — таблица ниже. +3. Прочитан код `src/wall-thickness.ts` и построчно сверен диагноз §3 ТЗ: + - `insetContour()` (:773-832): при `collinearJoint(uA, uB)` (:808) — + специальная ветка, которая кладёт обе точки `pa`/`pb` (offset-точка И + исходная вершина нулевой грани при коллинеарном стыке) — совпадает с + утверждением ТЗ §7.2 «строго коллинеарный переход сохраняет ступень»; + - при **не**-коллинеарном стыке (реальный fixture отклонён на доли/единицы + градуса) код идёт в mitre/bevel-ветку (:817-829); при удалённом mitre + (`dist > MITRE_LIMIT × maxO`, :821) срабатывает bevel (:827-829): + `if (oA > 0) out.push(...)`; `if (oB > 0) out.push(...)`. Если один из + offset'ов равен нулю (наш случай: положительная наружная стена ↔ + нулевой разделитель), в вывод попадает **только одна** точка — офсетная + точка толстой грани; исходная вершина нулевой грани не добавляется + нигде. Это ровно механизм, который ТЗ §3/§4 (комментарий-анализ) + описывает как причину клина: «bevel-ветка сохраняет только смещённую + точку толстой грани и теряет исходную вершину нулевой грани». Диагноз + точен, не является догадкой. + - `outsetContour()` (:1912-1967) зеркально воспроизводит ту же структуру + (:1962-1963) — подтверждает утверждение ТЗ §8.2 о необходимости + симметричного исправления в inset и outset. + - Геометрически прослежен путь клина: в узле стыка (толстая стена → нулевой + разделитель) кольцо после bevel соединяет офсетную точку толстой грани + напрямую со следующей вершиной вдоль нулевого разделителя (у которой оба + соседних offset = 0, значит она остаётся исходной вершиной), образуя + прямую от «почти нулевого» смещения до нуля на другом конце — то самое + сечение «растёт от 0 до полной глубины стены», описанное в баг-репорте и + §3 ТЗ. +4. Прочитан `docs/WALL-THICKNESS.md` целиком: подтверждён контракт growth ±½, + union колец по комнатам, единый источник геометрии для + full/static/hidden-isometric и light occlusion (раздел 2–4) — ТЗ §7.4/§8.6 + продолжает существующую модель, а не изобретает новую. Раздел 8 документа + («Independent partitions… same joined set used by Glow, sun and source + placement») также согласуется с требованием ТЗ единого физического тела + для всех потребителей. +5. Проверено, что #172 не дублирует #123 (наружный фасад/выход толщины через + вершину при Split) и #150 (breakpoint между коллинеарными внешними + интервалами разной толщины): прочитан `docs/specs/123-corner-split-wall.md` + — там баг про экстерьерный bbox и наружный зуб от острого митра, здесь — + про внутреннюю нулевую границу и bevel, теряющий вершину. Разные механизмы, + разные условия срабатывания (там — вершина исходной комнаты, здесь — + стык offset>0 / offset=0 в bevel-ветке). Не дубликат. +6. Проверена терминология: «Split» в ТЗ совпадает с `docs/USER-GUIDE.ru.md:340` + («Split | Делит комнату путём от одной стены до другой»); термины «masonry», + «mitre», «bevel», «cap» — это уже принятая в `docs/WALL-THICKNESS.md` + английская терминология подсистемы, не изобретены автором ТЗ. +7. Проверен явно фактический фикстур §3: полигон + `[100,100]–[900,100]–[900,800]–[600,800]–[600,400]–[100,400]`, Split + `[600,400]→[900,402.5]` даёт `atan(2.5/300) ≈ 0,477°`, а `[900,405]` даёт + `atan(5/300) ≈ 0,955°` — числа в ТЗ внутренне согласованы, не выдуманы. +8. Проверено соответствие `docs/CONFIG-COMPATIBILITY.md`: задача не создаёт + нового compatibility-случая (не меняет persisted-представление `RoomCfg`/ + `WallEntry`), что подтверждено и содержанием реестра (нет полей, + относящихся к разделителям/толщине, требующих отдельной миграции). +9. Проверен явный технический блок §16 «Принятые технические предположения»: + все пять пунктов — про место реализации, эпсилон в тестах, отсутствие + отдельной post-render маски, переиспользование fixture #123 и границу с + возможным отдельным багом boolean-библиотеки — технические, не продуктовые, + корректно не эскалированы владельцу (PROCESS.md §7.1: «владельцу — только + продуктовые вопросы»). +10. Проверена трассируемость: `docs/specs/README.md:94` обновлён тем же + коммитом `4582628`; ссылка issue → ТЗ и ТЗ → issue двусторонняя. + `git diff --stat origin/dev...HEAD` показывает только + `docs/specs/172-zero-divider-taper.md` и `docs/specs/README.md` (класс C, + ни одного файла класса A — правило №1 не нарушено на этапе ТЗ). Коммит + несёт `Issue: #172`, `User-Visible: no` — верно для документа ТЗ, который + сам не меняет поведение продукта. +11. Проверено существование файлов, которые ТЗ называет предполагаемыми: + `test/wall-thickness.test.mjs`, `demo/smoke_split_corner_wall.mjs`, + `demo/smoke_wall_junctions.mjs`, `demo/smoke_wall_thickness.mjs` — все + существуют; новый `demo/smoke_zero_divider_taper.mjs` не пересекается по + смыслу с существующим `demo/smoke_split_nonsnap.mjs` (тот проверяет + Split на не-grid-aligned полигоне, а не переход толщины на нулевой + границе) — не дублирует существующее покрытие. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | §1 — администратор дома, desktop Plan editor, момент завершения Split почти вдоль плеча угла | +| Что человек увидит до/после | ✅ | §2, «До:»/«После:» — см. Low-1 | +| Проблема (с подтверждённой причиной) | ✅ | §3, причина проверена построчно по коду (см. «Как проверялось» п.3) | +| Скоуп / не-скоуп | ✅ | §5 / §6, явные границы (без snap, без нового UX, без изменения `MITRE_LIMIT` для двух положительных толщин, без миграции) | +| Контракт поведения | ✅ | §7 (геометрия) + §8 (архитектурные ограничения реализации) | +| Модель данных и миграция | ✅ | §9 — явное «форматы не меняются», «читается исправленно, без записи» | +| UX, i18n, accessibility, touch | ✅ | §10, явно `Touch editor: best effort / intentionally degraded` — буквальная канон-метка `docs/TOUCH-SUPPORT.md` присутствует | +| AC1…ACn с доказательством | ✅ | §11, 11 штук, каждый помечен `unit`/`smoke`/`golden`/«ревью кода» | +| План автотестов | ✅ | §12, разбит на unit / browser smoke / golden и pre-release / implementation loop | +| Риски | ✅ | §13, таблица риск → мера, плюс performance/security | +| Откат | ✅ | §14 | +| Release-артефакты | ✅ | §15, конкретный список: оба changelog, `WALL-THICKNESS.md`, тесты, golden, три копии бандла | + +Все обязательные разделы присутствуют и содержательны. Дополнительно есть +раздел решений владельца (§4, фиксирует принятые Q1–Q4) и явный блок принятых +технических предположений (§16) — соответствует требованию PROCESS.md §7.1 +отделять продуктовое решение от технического и не выдавать догадку за факт. + +## Находки + +Находок уровня **High** и **Medium** нет — новых issue не требуется. + +### Low-1 — «что человек увидит» длиннее одной фразы + +**Файл:** `docs/specs/172-zero-divider-taper.md:26-34` (§2) + +PROCESS.md §7.1 требует «одной фразой, без терминов реализации». Раздел +написан двумя-тремя предложениями на «До:»/«После:» (например, «После:» +содержит два предложения). По существу требование выполнено — язык +исключительно визуальный («клин», «толщина», «стык»), без имён функций или +внутренних терминов, — но формально это не «одна фраза». Тот же класс +находки уже фиксировался как Low и не блокировал приёмку в +`SPEC-REVIEW-141-r1` (Low-2) и `SPEC-REVIEW-137-r1`. + +**Решение ревьюера:** Low, не блокирует. Косметическая правка на усмотрение +автора при следующей редакции. + +### Low-2 — эпсилон/half-depth границы локального cap не формализованы числом + +**Файл:** `docs/specs/172-zero-divider-taper.md:124-125, 338-339` (§7.1, §16.2) + +Контракт требует, чтобы локальная область примыкания «ограничена физической +half-depth примыкающей стены и геометрическим epsilon» и «не может расти +пропорционально длине D», но конкретная формула эпсилон (как, например, +`max(4% × grid pitch, 1e-9)` для «effectively collinear» в +`docs/WALL-THICKNESS.md`) не приведена — §16.2 явно оставляет её тестовой +стратегии автора кода. В `src/wall-thickness.ts` уже есть несколько +устоявшихся эпсилон-констант (`openEps(pitch, coordScale)`, +`pitch * coordScale * 0.02`, `1e-9`), так что это не белое пятно, а +осознанно оставленная техническая свобода, корректно помеченная как +предположение, которое ревьюер вправе оспорить, но переносить в +продуктовый вопрос владельцу нет оснований — граница «не растёт +пропорционально длине» уже достаточна как проверяемый инвариант для AC2. + +**Решение ревьюера:** Low, не блокирует. Код-ревью должно убедиться, что +выбранная константа действительно не масштабируется с длиной грани (что уже +явно требует AC2), а не что она равна конкретному числу. + +## Что проверено и корректно + +- **Соответствие `docs/SCOPE.md`:** задача закрывает **J4** («от нуля до + плана без Inkscape/YAML» — Split не должен молча создавать кладку, которую + пользователь не задавал) и **J6** («план остаётся правдивым по мере + развития» — сохранённые планы должны отображаться корректно без ручной + переработки). Обе строки Closed, это исправление дефекта внутри принятой + функциональности, а не расширение продукта. Общая физическая геометрия + также поддерживает согласованность J1/J2/J3 между View и Plan, как верно + указано в §1 ТЗ. +- **Легитимность полного трека:** сложность/риск 6/7 из 10, минимум пять + затронутых поверхностей (Plan, View/kiosk/static, hidden Iso, floor/room + fills, Glow/sun barriers) — критерии `small` (§5 PROCESS.md, одна + поверхность, риск ≤3) не выполняются; полный трек и отдельный файл ТЗ + выбраны верно, лёгкий трек владелец и автор корректно не применили. +- **Продуктовые вопросы закрыты по процессу:** Q1–Q4 заданы одним пакетным + комментарием, каждый с предлагаемым default, задача корректно ушла в + `blocked`+`S3-spec` до ответа и вышла из `blocked` сразу после решения + владельца (все четыре ответа — «да»). Ни один технический вопрос не был + ошибочно вынесен владельцу — §16 явно и полностью перечисляет технически + свободные решения. +- **Технический диагноз не голословен.** Причина клина (bevel-ветка + `insetContour()`/`outsetContour()` теряет исходную вершину нулевой грани + при близком, но не точном коллинеарном стыке) построчно проверена по + исходному коду `src/wall-thickness.ts:773-832, 1912-1967` и совпадает с + описанием в ТЗ — см. «Как проверялось» п.3. Числовые фикстуры (углы + `0,477°`/`0,955°`) внутренне согласованы с геометрией §3. +- **Не дубликат #123/#150:** механизм и условие срабатывания различны, + проверено по `docs/specs/123-corner-split-wall.md`; ТЗ корректно + разграничивает три задачи в одном абзаце §3. +- **Не-скоуп (§6) корректно отсекает соседние соблазны:** snap почти + перпендикулярного/коллинеарного Split, изменение допустимости или диалога + Split, автоназначение толщины, переработка модели данных, изменение + `MITRE_LIMIT` для пар двух положительных offset'ов, публикация изометрии + как отдельной фичи — все явно исключены с указанием причины. +- **Регрессионные гарантии сформулированы явно:** AC3/AC9 поимённо защищают + точный коллинеарный/ортогональный переход, пары `1↔15`/`15↔100`, corner + Split, wall junctions, wall thickness, opening tunnels — то есть именно те + сценарии, которые уже используют ту же `insetContour()`/`outsetContour()` и + могли бы негласно пострадать от исправления. +- **Модель данных и миграция (§9):** корректно заявлено «форматы `RoomCfg` и + `WallEntry` не меняются», «чтение и рендер не записывают конфигурацию», без + прямой/обратной миграции — сверено с `docs/CONFIG-COMPATIBILITY.md`, задача + не создаёт нового compatibility-случая. +- **UX/touch (§10):** буквально использует канон-формулировку + `docs/TOUCH-SUPPORT.md` («Touch editor: best effort / intentionally + degraded») и отдельно фиксирует safety floor: View/kiosk/static — + блокирующие поверхности, сохранённая геометрия не зависит от типа + указателя. +- **Release-артефакты (§15)** перечисляют оба changelog в одном + implementation-коммите, `docs/WALL-THICKNESS.md`, конкретные + unit/smoke/golden-файлы и три синхронные копии бандла — соответствует + §7.1/правилу 11 PROCESS.md. Golden корректно ограничен только + `npm run golden:accept -- --reviewed` по полному Linux-артефакту на + предрелизном этапе (§12, согласуется с PROCESS.md §8/§11.4). +- **Дисциплина «тест должен уметь падать»:** AC1 прямо требует, чтобы фикстура + §3 краснела на исходном `dev` из-за taper-клина, и явно передаёт эту + проверку на код-ревью («Ревьюер фиксирует эту проверку в code review») — + соответствует требованию PROCESS.md §2.7/§18. +- **Трассируемость:** `docs/specs/README.md:94` обновлён тем же коммитом + `4582628`; ветка `issue/172-zero-divider-taper` и трейлеры (`Issue: #172`, + `User-Visible: no`) корректны для документа класса C, который сам не меняет + поведение. `git diff --stat origin/dev...HEAD` не содержит ни одного файла + класса A — продуктовый код не тронут до `S5-ready` (правило №1). + +## Чего не проверял + +- Не проверял реализуемость конкретного алгоритма из §8.2 («сохранить обе + точки в детерминированном порядке») как единственно возможного решения — + это техническая свобода автора кода (§16), а не предмет ревью ТЗ; код-ревью + должно будет проверить сам факт отсутствия taper, а не конкретный порядок + вставки точек. +- Не запускал автотесты, `golden`, browser-смоки или performance-профили — на + этапе `spec` это не требуется; существование названных в ТЗ файлов + (`test/wall-thickness.test.mjs`, `demo/smoke_split_corner_wall.mjs`, + `demo/smoke_wall_junctions.mjs`, `demo/smoke_wall_thickness.mjs`) и + структура `insetContour()`/`outsetContour()` проверены чтением кода, а не + исполнением. +- Не проверял корректность конкретных числовых оценок аналитики (8/10 · 6/10 · + 7/10 · P2) по существу — это поле владельца (PROCESS.md §2.2), уже принято + явным решением владельца до написания ТЗ. +- Не проверял связанные issue #123/#150 по существу за пределами того, что + понадобилось для верификации отсутствия дублирования (различие механизма и + условий срабатывания) — они не входят в предмет этого ревью. +- Не проверял, обнаружится ли в ходе реализации отдельный дефект + boolean-библиотеки (упомянутый как риск в §16.5) — это явно вынесено в + будущий отдельный issue, если случится, и не влияет на приёмку ТЗ сейчас. + +## Вердикт + +Зелёный. High: 0, Medium: 0. Две находки Low (раздел «что человек увидит» +длиннее одной фразы; численная граница локального cap оставлена технической +свободой без явной формулы) — ни одна не блокирует приёмку, обе либо +правятся косметически при следующей редакции, либо снимаются этой записью без +нового цикла.