diff --git a/docs/reviews/SPEC-REVIEW-298-r1.md b/docs/reviews/SPEC-REVIEW-298-r1.md new file mode 100644 index 00000000..096d3e42 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-298-r1.md @@ -0,0 +1,198 @@ +# SPEC-REVIEW-298-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/298 +- **ТЗ:** `docs/specs/298-resize-wall-thickness-carrier.md` @ commit `45066631aee4997ac1f033dbe75918b5b2d439d4` +- **Этап:** ТЗ на ревью (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (лимит лёгкого трека не действует — issue без метки `small`) +- **Ревьюер:** Claude (сессия ревью ТЗ), автор ≠ ревьюер + +## Скоуп ревью + +Полный разбор — это первый заход, дельты нет. Читаю ТЗ состязательно: ищу, где +оно невыполнимо, непроверяемо, либо выдаёт догадку за решение продукта. + +## Как проверялось + +1. `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` — целиком, перед тем как судить. +2. Тело issue #298 и три комментария (аналитика, занятие, передача ТЗ) — через + `gh issue view 298 --json ...` (MCP-инструменты GitHub были недоступны без + подтверждения, поэтому использован `gh` CLI). +3. Полный текст `docs/specs/298-resize-wall-thickness-carrier.md`. +4. Канонические документы затронутой подсистемы: `docs/RESIZE.md`, + `docs/WALL-THICKNESS.md`, разделы `docs/TOUCH-SUPPORT.md` про safety floor + редакторов. +5. Существование артефактов, на которые ссылается ТЗ, проверено в дереве + репозитория, а не на слово автора: + - `test/fixtures/real-plan-second-floor.json`, + `test/fixtures/real-plan-first-floor.json` — существуют; + - `LATTICE_NOISE_STEPS` — существует, `src/coordinate-canonicalization.ts:11`, + значение `1e-4`, используется в `scripts/model-invariants.mjs` и + мутационном гейте; + - `rekeyWallsAfterMove()` — существует, `src/wall-thickness.ts:533`; прочитан + код: помеченная в issue пропорциональная проекция (`mapPoint`, строки + 574–582 — `t` считается от старого ребра и переносится на новое без учёта + реального соответствия вершин) реально в проде — корневая причина + подтверждена чтением, а не поверена на слово; + - `off_lattice_coordinate` / `wall_carrier` — существуют как виды нарушений + в `scripts/model-invariants.mjs` и таблице `KNOWN` `demo/smoke_edit_walk.mjs`; + текущая таблица `KNOWN` (строки 73–87) уже регистрирует ровно те находки, + которые AC6 требует убрать (`off_lattice_coordinate`, `wall_carrier` для + обеих фикстур), и отдельно `mixed_role_record` — долг #299, который ТЗ + прямо исключает из скоупа и просит не трогать в `KNOWN`. Согласуется. +6. Проверено существование и состояние всех связанных issue, на которые ссылается + ТЗ: #253, #277, #289, #291, #293, #297 — закрыты; #299 — открыт, как и + утверждает ТЗ (не дубликат, отдельный долг). Ни одна ссылка не битая и не + искажена. +7. Сверены формулировки с канонoм: раздел 3.3 (legacy fallback, запрет + proportional-midpoint) согласуется с `WALL-THICKNESS.md` («Legacy entries + without endpoints keep the unambiguous whole-key/midpoint fallback and are + never split by inventing a length»); раздел 8 (touch safety floor) согласуется + с разделом `TOUCH-SUPPORT.md` «Safety floor that still applies to touch + editors»; §5 (disabled handle до pointer capture) согласуется с существующим + текстом `RESIZE.md` про eligibility и disabled-handle. Ни одного места, где + автор выдаёт непроверяемую догадку за факт, не найдено — все нетривиальные + технические решения (§12 «Принятые предположения») либо выводятся из уже + принятого контракта #277, либо явно помечены как предположение. + +**Не проверялось** (стадия ТЗ этого не требует): выполнение гейтов +`typecheck`/`test`/`build` — кода к задаче ещё нет; сам код ещё не написан. + +## Находки + +### Medium (в скоупе задачи) — отсутствуют обязательные продуктовые разделы §7.1 + +`PROCESS.md` §7.1 требует в ТЗ два продуктовых раздела первыми: «какая персона +встречает изменение, на какой поверхности, в какой момент» и «что человек +увидит до и после — одной фразой, без терминов реализации». `AGENTS.md` +повторяет это как обязательное дополнение к §7.1. Ни то, ни другое в тексте +ТЗ не выделено. + +- §1 («Сценарий и подтверждённая причина») называет только техническую цепочку + (`rekeyWallsAfterMove()`, endpoint, topology boundary) — персона, поверхность + и момент («администратор дома, десктоп, Plan editor / Resize, после + нескольких правок за дни») были прямо названы в комментарии аналитики к + issue, но не перенесены в сам документ ТЗ. +- §2 («Пользовательский результат») — единственный кандидат на «одну фразу без + терминов реализации», но весь абзац написан в терминах реализации: «wall + endpoints», «topology vertex», «lossless correspondence», «config write», + «Undo entry». Читатель без контекста кода не поймёт по нему, что видит + пользователь. + +Дефект не про содержание решения — оно верное и подтверждено кодом — а про +форму ТЗ, которая обязана доказывать, что это изменение продукта, а не работа +над кодом (PROCESS.md §7.1: «ТЗ, которое не может ответить на эти два вопроса, +описывает работу, а не изменение продукта»). Правка дешёвая: одна-две фразы в +начале документа, без пересмотра контракта. + +**Предлагаемая правка** (пример, автор волен сформулировать иначе): +> Персона — администратор дома (`docs/SCOPE.md`), поверхность — desktop Plan +> editor, инструмент Resize; момент — любой ресайз стены, эффект копится +> незаметно и проявляется через дни на другой стене. +> Видимо: сегодня после серии обычных ресайзов случайная стена в другом месте +> плана вдруг рисуется другой толщиной или теряет рабочую ручку ресайза, хотя +> её никто не трогал. После исправления обычный ресайз либо проходит как +> раньше, либо (в редком неоднозначном случае) заканчивается тем же самым +> сообщением об ошибке без изменения плана — но никогда не портит стену, которую +> пользователь не двигал. + +Это Medium: без High-находок вердикт жёлтый, правка делается автором в этом же +issue, повторного полного разбора не требует — фактическая архитектура решения +ревью не оспаривает. + +### Low (снято ревьюером с записью) — способ доказательства не назван явно у двух AC + +DoR-чеклист (`PROCESS.md` §2.5) требует у каждого AC явного указания, чем он +доказывается (`unit`/`backend`/`smoke`/`golden`/«ревью кода»). У AC1, AC2, AC4, +AC6, AC8, AC9 это явно есть (или очевидно из перечисленных команд). У **AC5** +(carrier preflight) и **AC7** (preview/commit/Undo атомарны) отсутствует строка +«Доказательство: …» — способ проверки не назван словом, хотя из контекста +понятен: AC5 — чистая функция преflight из §4, доказывается unit-тестом; AC7 — +поведение pointer-жеста, доказывается production-bundle smoke по аналогии с уже +существующим `demo/smoke_room_resize.mjs` (см. `RESIZE.md` «Verification»). + +Снимаю без возврата на цикл: неоднозначности в том, *что* проверяется, нет — их +формулировки уже настолько конкретны (перечислены positive/negative кейсы, +атомарность записи/Undo), что тип теста восстанавливается однозначно. Автору +стоит добавить явную строку при реализации ради единообразия с остальными AC, +но это не блокирует переход в «Готово к разработке». + +### Low (снято ревьюером с записью) — разделы «UX» и «i18n» не выделены явно + +`PROCESS.md` §7.1 перечисляет UX и i18n как обязательные разделы. В документе +нет разделов с этими заголовками; содержание по факту размазано — UX-семантика +(disabled handle, единственное существующее сообщение об ошибке, отсутствие +нового UX) описана в §5 и подтверждена явным предположением №3 в §12; i18n +покрыт тем же предположением №3 («новый persisted reason или новый UX в этой +задаче не вводится» ⇒ новых ключей нет). + +Снимаю: по существу оба вопроса закрыты (нет нового текста, нет новых ключей), +и это прямо написано, просто не под ожидаемым заголовком. Chisto формальный +момент, не влияющий на проверяемость AC. + +## Что проверено и корректно + +- Корневая причина в §1 подтверждена чтением `rekeyWallsAfterMove()` — + реальный код делает ровно ту пропорциональную проекцию (`mapPoint`), о которой + говорит issue и ТЗ; это не пересказ чужих слов. +- Оба воспроизведения (AC1/AC2) содержат точные числа из issue + (`-85 → -82.457`, `59.538`, вершины `17/52/57/101`) и совпадают с текстом + issue буквально — не пересказаны с искажением. +- Скоуп «не входит» (§6) корректно исключает #289/#299 (смешанные роли), + ADR #282 (integer storage), eligibility/#277/#293 (topology) — все со ссылками + на реально существующие issue в правильном статусе. +- AC6 корректно ссылается на реальную структуру `KNOWN` в + `demo/smoke_edit_walk.mjs` и требует убрать только те два вида находок, которые + там сейчас действительно зарегистрированы для этой пары фикстур, не трогая + независимый долг #299 (`mixed_role_record`) — проверено построчно. +- Ни одна ссылка на связанные issue (#253, #277, #289, #291, #293, #297, #299) + не оказалась битой, дублирующей или искажающей состояние (закрыт/открыт). +- Технические решения раздела 3 (correspondence-таблица, конфликт нескольких + destinations, атомарное разбиение длинной записи) и раздела 4 (carrier + preflight по всему span, не только по концам/midpoint) не изобретают новое + поведение — они последовательно продолжают уже принятый контракт #277 и модель + данных `WALL-THICKNESS.md` («exact endpoints… independent of whichever room + topology later happens to split the same straight line»), а не выдают догадку + за факт. +- Раздел 3.3 (legacy fallback, fail-closed при неоднозначном whole-edge + соответствии) прямо согласован с уже задокументированным в `WALL-THICKNESS.md` + запретом «изобретать длину» для legacy-записей без `a/b`. +- Touch/safety floor в §8 корректно ссылается на существующий раздел + `TOUCH-SUPPORT.md` «Safety floor that still applies to touch editors», а не + придумывает новое правило. +- Откат (§10) реалистичен: чистый revert коммита, миграции нет, потому что схема + не меняется — согласуется с §8. +- Продуктовых вопросов владельцу в этом ТЗ нет, и по факту разбора это + оправданно: спорные места (§5 п.7 про предсказуемый конфликт до pointer + capture) читаются как следствие уже принятого принципа fail-closed + (`RESIZE.md`: «If neither step is safe, the handle remains… disabled»), а не + как новое расширение eligibility — само ТЗ явно исключает изменение eligibility + из скоупа (§6 «Не входит»), и текст §5 п.7 с этим не расходится при точном + чтении («если конфликт можно доказать в eligibility» — то есть уже + существующими проверками, а не новыми). +- Ни один найденный ранее класс дефектов (#253, #258/#259, #287/#289) не + игнорируется задним числом — ТЗ явно проговаривает отличие от каждого в + разделе «Почему это дорого» issue и в §6/§12 ТЗ. + +## Чего не проверял + +- Само исправление (кода ещё нет — задача в `S4-spec-review`, реализация не + началась). AC1–AC9 разбирались на выполнимость и проверяемость формулировки, + а не на то, будущий код им действительно удовлетворит — это работа + код-ревью. +- Полный текст `docs/CANVAS.md` — раздел на который ссылается ТЗ, но задача не + меняет canvas-рендер напрямую (только данные, которые он потребляет); беглая + проверка показала, что ссылка не противоречит модели данных `walls[*]`. +- Все шесть комбинаций `smoke_edit_walk` не прогонялись — на этой стадии нет + кода для прогона; сверка ограничилась статическим соответствием таблицы + `KNOWN` тому, что требует AC6. +- Производительность (AC9) — числовые бюджеты `RESIZE.md` не пересчитывались, + только сверено само требование «сохраняется p95 budget» с текстом канона. + +## Вывод + +Единственная содержательная находка — Medium, отсутствие обязательных +продуктовых разделов §7.1 (персона/поверхность/момент + «одна фраза без +терминов реализации»). Остальное — два Low, снятых с запиской. High-находок +нет: контракт технически выполним, каждый AC проверяем, ни одна ссылка не +искажена, ни одна догадка не выдана за решённый факт. Вердикт жёлтый — +формально из-за Medium, а не из-за сомнений в самом решении.