diff --git a/docs/reviews/SPEC-REVIEW-299-r1.md b/docs/reviews/SPEC-REVIEW-299-r1.md new file mode 100644 index 00000000..5b5d12ff --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-299-r1.md @@ -0,0 +1,125 @@ +# SPEC-REVIEW-299-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/299 +- **Этап:** ревью ТЗ (PROCESS.md §2.4) +- **Документ ТЗ:** `docs/specs/299-mixed-role-wall-records.md` +- **SHA на котором получен вердикт:** `10f000dc47c391e5d6c18ba6e9b5b46ed4df67a3` + (ветка `issue/299-mixed-role-thickness`, коммит «docs: specify role-aware + wall compaction») +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (первый заход, §4 + бюджет ещё не тратился) + +## Скоуп проверки + +Первый раунд — разбор полный (PROCESS.md §2.10 применяется только со второго +раунда). Проверено: + +1. `docs/SCOPE.md` — задача закрывает J6 («Keep the plan true as the home + evolves»); ADR #282 явно оставляет открытые P1 на живой репрезентации + (строка 135: «None of this cancels an open P1»), так что точечная починка + в обход стадий ADR легитимна, а не самовольное расширение скоупа. +2. `AGENTS.md`, `PROCESS.md` §2.4/§2.10/§7.1/§5 — трек не `small` (сложность + 8/10, риск 9/10, две продуктовые операции, real-plan regression), поэтому + файл ТЗ обязателен и он есть; комментарий аналитика подтверждает трек + «обычный» и что продуктовых вопросов нет. +3. Тело issue #299 и три комментария (аналитика → занятие → «ТЗ готово»). +4. `docs/USER-GUIDE.ru.md` не проверялся отдельно текстом, т.к. ТЗ прямо + заявляет «новых кнопок, диалогов, предупреждений и терминов UI нет» (§2) — + термины Optimize/«Удалить комнату, оставить стены» уже существующие и не + переименовываются. +5. Канонический документ подсистемы: `docs/WALL-THICKNESS.md` — прочитан + целиком, модель, компакция и контракт удаления комнаты сверены построчно. +6. Связанные закрытые issue #287 (диагностика `checkMixedRoleRecords`, есть в + репозитории) и #289 (safe Resize eligibility) — оба `CLOSED`, содержимое их + спек (`docs/specs/289-no-mixed-role-resize.md`) явно исключает «исправление + уже сохранённых mixed-role records через Optimize» из своего скоупа, что + подтверждает отсутствие дублирования с #299. #298 сейчас в `S4-spec-review` + (параллельно) — ТЗ #299 корректно называет эту зависимость и требует, чтобы + автор перед кодом ребейзился и держал контракт независимым от порядка + слияния (принятое предположение №5, §15). + +## Как проверялось (техническая верификация утверждений ТЗ) + +ТЗ делает несколько фактических утверждений о текущем поведении кода — +согласно инструкции ревью, догадка, выданная за факт, является находкой. Все +проверены чтением исходников на SHA выше, не исполнением: + +| Утверждение ТЗ | Где проверено | Результат | +|---|---|---| +| §3: `normalizeWallIntervals()` группирует соседние атомы только по `cm` и «сплошной», не сравнивая владельцев | `src/wall-thickness.ts:1927-2005`, ключевая проверка `pr.cms[next] !== cm` на строке 1966 — владелец/роль нигде не сравнивается | подтверждено | +| §3: `checkMixedRoleRecords()` — существующий диагностический инвариант из #287, не product-фикс | `scripts/model-invariants.mjs:518-543` | подтверждено, вызывается только в `main()` отчёта, не в write-пути | +| §4.4 / #228: exclusive positive-solid intervals удаляемой комнаты → partitions, shared/virtual/zero не материализуются | `src/room-deletion.ts:73-112`, фильтр `interval.kind === 'outer' && !interval.open && interval.cm > 0` | подтверждено | +| §4.4: Keep walls и Optimize сходятся в одном канонизаторе | `src/houseplan-card.ts:11456` (`this._normalizeWalls(materializedWalls, …)` после удаления комнаты) и `src/plan-optimizer.ts:580` (`normalizeWallIntervals(...)`) — оба вызывают одну и ту же функцию `src/wall-thickness.ts:1927` | подтверждено, один enforcement point действительно один | +| §15 предположение №1: роль выводится из `WallInterval.roomId`/`kind` ('shared'\|'outer'), пары владельцев ещё не готовы как поле | `src/wall-thickness.ts:1535-1545` (`WallKind = 'shared'\|'outer'`, без id второго владельца) | подтверждено техническое допущение реалистично: пару владельцев для `shared(A,B)` придётся строить геометрическим сопоставлением reversed-копий, а не читать готовое поле — это именно то, что ТЗ явно отдаёт на усмотрение автора/ревьюера в §15.1, а не выдаёт за уже существующее | +| ADR #282 исключение для P1 | `docs/adr/282-wall-geometry-representation.md:135-137` | подтверждено дословно | +| Комментарий кода `KNOWN` (`demo/smoke_edit_walk.mjs`) действительно содержит долг `mixed_role_record` на `real-plan-first-floor.json` seed 1 и 3 | `demo/smoke_edit_walk.mjs:82,86` | подтверждено, ссылка «Класс #287/#289» соответствует истории | +| Фикстура/тестовые файлы, названные в AC, существуют | `test/fixtures/real-plan-first-floor.json`, `test/wall-thickness.test.mjs`, `test/plan-optimizer.test.mjs`, `test/room-deletion.test.mjs` | все существуют | +| Инфраструктура для AC8 (targeted mutation) реально существует | `scripts/mutation-gate.mjs` (issue #85) | подтверждено, не изобретённый инструмент | + +Ни одно проверенное утверждение не оказалось догадкой, выданной за факт. + +## §7.1: обязательные разделы + +Все обязательные разделы присутствуют и в правильном порядке: сценарий (§1) · +что человек увидит (§2) · проблема/причина (§3) · контракт поведения (§4) · +scope и не-scope (§5) · модель данных/миграция (§6) · UX/i18n/touch/security +(§7) · AC1…AC9 с доказательством (§8) · план автотестов (§9) · производительность +(§10) · риски (§11) · откат (§12) · ожидаемые файлы (§13) · release-артефакты +(§14) · явный блок принятых технических предположений (§15). Дополнительные +продуктовые разделы AGENTS.md (персона/поверхность/момент; что человек видит +одной фразой без терминов реализации) — оба на месте и написаны без терминов +реализации. + +## Находки + +Блокирующих (High) находок нет. Medium в скоупе или вне скоупа — нет. + +Отмечено, но не является находкой (Low, снято без правки): §15 сознательно +оставляет способ построения пары владельцев `shared(A,B)` на усмотрение +реализации («может быть заменено ревьюером на эквивалентное без изменения +UX») — это ровно тот класс технического решения, который PROCESS.md §7.1 +явно не выносит на продуктовый вопрос владельцу. Снимаю без правки: граница +между «непроверяемая догадка» и «явно объявленное техническое допущение» +соблюдена. + +## Продуктовые вопросы владельцу + +Отсутствуют, и это корректно: единственный продуктовый вопрос («ожидает ли +пользователь, что кнопка Optimize/Keep walls чинит толщину, а не портит её») +уже отвечен самим фактом происхождения задачи — она заведена как P1-баг +против уже согласованного поведения (J6), а не как новый UX-контракт. Ни один +технический вопрос не был замаскирован под продуктовый и не был переадресован +владельцу — оба спорных технических места (owner-signature representation, +порядок интеграции с #298) явно решены в §15 как «принято предположительно, +поменять свободно», что и требуется вместо вопроса. + +## AC — проверяемость и доказательство + +AC1–AC9 однозначны, у каждого назван способ доказательства (unit table-driven, +real-plan regression, production-bundle smoke, targeted mutation, локальные +гейты). AC8 (мутационный тест на сам guard) — сильная защита от неполной +реализации, убивающая как «слишком мягкий» (не сравнивает владельцев), так и +«слишком строгий» (blanket-disable всей компакции) уклон; вместе с +позитивными assertions AC2 это заранее закрывает риск того, что фикс превратит +нормализацию в отключение слияния вообще (см. риск §11 и `AC2`). + +## Что не проверялось + +- Само исполнение тестов/гейтов — на этапе спек-ревью кода ещё нет, это + предмет код-ревью (§2.7); проверка ограничена чтением ТЗ и сверкой с + текущим состоянием репозитория. +- Полнота geometry-инвариантов (`npm run invariants`) не прогонялась — нет + ещё изменения геометрии для прогона; будет предметом код-ревью. +- Не проверялся сам ADR #282 целиком построчно за пределами цитируемого + исключения — прочитан фрагмент, относящийся к заявленному основанию. +- Актуальность `docs/USER-GUIDE.ru.md`/`CANVAS.md`/`UX-MODES.md` не сверялась + построчно, так как ТЗ не меняет видимую поверхность/термины (§2, §7); при + расхождении это всплывёт на код-ревью, когда будет реальный diff + документации. + +## Вердикт + +Зелёный. ТЗ полно, каждый AC проверяем и привязан к способу доказательства, +технические предположения явно помечены и не выданы за факт, продуктовых +вопросов нет и не должно быть, скоуп и не-скоуп чётко разграничены и не +пересекаются с закрытыми #287/#289 и параллельным #298.