From 8b8b9ed90d533ab3ffd179d6653726ab3a05eb62 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:55:30 +0000 Subject: [PATCH] docs: review document for #201 Issue: #201 User-Visible: no --- docs/reviews/SPEC-REVIEW-201-r1.md | 202 +++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-201-r1.md diff --git a/docs/reviews/SPEC-REVIEW-201-r1.md b/docs/reviews/SPEC-REVIEW-201-r1.md new file mode 100644 index 00000000..6c15faba --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-201-r1.md @@ -0,0 +1,202 @@ +# Ревью ТЗ — issue #201, цикл r1 + +- Этап: `S4-spec-review` (PROCESS.md §2.4) +- Артефакт ТЗ: [`docs/specs/201-atomic-thickness-lookup.md`](../specs/201-atomic-thickness-lookup.md), + коммит `7b759f3` на ветке `issue/201-atomic-thickness-lookup` +- Issue: [#201](https://github.com/Matysh/houseplan-card/issues/201) +- Ревьюер: Claude (роль «ревьюер ТЗ», отдельная сессия от аналитика/автора) +- Трек: обычный (не `small`) — верно и совпадает с метками issue (`bug`, `P2`, + `S4-spec-review`, без `small`); сложность оценена аналитикой в 4/10, что ниже + порога `small` (≤3) уже само по себе, плюс задача трогает два модуля + (`wall-thickness.ts` и `open-spans.ts`), что дополнительно исключает лёгкий + трек по критерию «одна поверхность» (§5 PROCESS.md) + +## Скоуп ревью + +Оценивалось ТЗ `docs/specs/201-atomic-thickness-lookup.md` целиком: наличие +обязательных разделов §7.1 PROCESS.md, однозначность и доказуемость AC1–AC11, +отсутствие догадок, выданных за решённый факт, соответствие `docs/SCOPE.md` +(job J4/J6), `docs/WALL-THICKNESS.md`, `docs/TESTING.md` и текущему коду +(`src/wall-thickness.ts`, `src/open-spans.ts`, `test/wall-thickness.test.mjs`). +Продуктовый код не менялся и не мог быть изменён (задача на этапе ТЗ). + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +2. Прочитано тело issue #201 и оба комментария: аналитика владельца (с полным + подтверждением дефекта реальным исполнением на fixture #197) и хендофф + автора ТЗ. +3. Прочитан весь текст ТЗ построчно, сверен с §7.1 (обязательные разделы) и + §2.5 (DoR-чеклист). +4. Прочитан канонический `docs/WALL-THICKNESS.md` целиком — модель `walls`, + контракт «one physical stretch has one thickness», принцип «exact endpoints + make a thickness boundary independent of… room topology», существующий + testing-каталог §8. +5. Проверена заявленная причина дефекта чтением продуктового кода: + `src/wall-thickness.ts:189-219` (`lookupWall`/`thicknessCmAt` — только direct + key либо tolerant midpoint-in-half-pitch fallback, без поиска покрывающего + exact span), `src/wall-thickness.ts:1008-1090` (`cmsForPoly` — уже содержит + ровно тот алгоритм, который ТЗ просит добавить в `thicknessCmAt`: сначала + узкий covering-span поиск по `entrySpan`/`distToSeg` с минимизацией + `spanLen - childLen`, затем by-parent fallback). Подтверждено: + `cmsForPoly()` действительно уже умеет то, что заявляет §3 ТЗ, и это + отдельный, не переиспользуемый код-путь от `thicknessCmAt()`. +6. Проверен единственный продуктовый потребитель `thicknessCmAt()`: + `grep -rn "thicknessCmAt" src/` даёт ровно один вызов вне самого модуля — + `src/open-spans.ts:523` внутри `thicknessOnClose()`. Заявление ТЗ + («Единственный продуктовый consumer… — `thicknessOnClose()`») подтверждено, + не является догадкой. +7. Прочитан `src/open-spans.ts:507-545` (`thicknessOnClose`/ + `applyThicknessOnClose`) — подтверждено: при `cm === 0` от соседа функция + молча переходит к следующему кандидату и в итоге возвращает + `fallbackCm = DRAW_WALL_DEFAULT_CM`, ровно как описывает §3/§6.3 ТЗ. +8. Проверены существующие негативные unit-тесты, которые могли бы + конфликтовать с новым контрактом (`test/wall-thickness.test.mjs:146,249-250, + 1275`) — все три завязаны на отсутствие/иное происхождение записи, а не на + частичное покрытие exact span; новый fallback их не затрагивает. +9. Проверено соответствие терминологии `docs/WALL-THICKNESS.md` §1 («atomic + collinear spans», «exact endpoints… independent of… room topology») — + ТЗ использует ту же терминологию, не изобретает новую модель. +10. Проверено, что файл ТЗ и запись в `docs/specs/README.md` добавлены одним + коммитом `7b759f3` с трейлерами `Issue: #201` / `User-Visible: no` — + корректно для чистой документации без изменения поведения на этой стадии. + +Код не менялся, чтения было достаточно: вопрос ревью ТЗ — «выполнимо и +проверяемо ли», а не «работает ли реализация». Дополнительно проверено, что +предложенный контракт (§6) не является чистой гипотезой: он копирует уже +работающий и протестированный алгоритм `cmsForPoly()`, что заметно снижает +технический риск задачи. + +## Проверено и корректно + +- **Все обязательные разделы §7.1 присутствуют**: сценарий и персона (§1), что + человек увидит до/после без терминов реализации (§2), проблема и + подтверждённая причина (§3), scope/non-scope (§4–5), контракт поведения (§6), + данные/миграция (§7), UX/i18n/touch (§8), AC1–AC11 с доказательством (§9), + план автотестов (§10), риски (§11), rollback (§12), release-артефакты (§13), + явный блок принятых предположений (§14). +- **Причина дефекта подтверждена чтением кода, а не заявлена на веру** — см. + «Как проверялось» пп. 5–8. Контраст «`cmsForPoly()` уже умеет находить + covering exact span, `thicknessCmAt()` — нет» дословно совпадает с + устройством кода. +- **ТЗ корректно опровергает исходную гипотезу автора issue.** В issue + предполагался разрыв в общем render-path; аналитика владельца и это ТЗ верно + сузили дефект до одного потребителя (`thicknessOnClose`), что подтверждено + фактическим исполнением (аналитика: `wallIntervals()` уже возвращает 20 см + на fixture #197 на текущем `dev`). Разница между исходной гипотезой репорта + и уточнённым фактом явно проговорена в §3, а не молча подменена. +- **Все AC пронумерованы, однозначны и несут явный способ доказательства** + (unit/matrix/existing regression/browser smoke/mutation gate/gates) — + выполняется требование DoR (§2.5 PROCESS.md). AC1–AC5 задают точную числовую + и структурную проверку (containment, оба scale, direction reversal, + permutation, legacy fallback), AC10 явно требует падающего мутанта. +- **Не-скоуп сформулирован точно и обоснованно** (§5): explicitly исключены + `wallIntervals()`/`cmsForPoly()`/рендер/geometry (не трогать работающий код), + очистка уже сохранённых данных (#198, чужой issue), выбор ближайшего соседа + в `thicknessOnClose()`, default 15 см, UI Close, snapping, ownership, + schema/backend/migration/Optimize. Каждый пункт — реальная граница, а не + формальность; ни один не пересекается с AC. +- **Защита от утечки толщины (AUD-159B6-01) явно сохранена** (§6.2) и + проверяется отдельным AC3 с негативной матрицей (частичный span, параллельный + offset, перпендикулярный сосед, malformed endpoints) — контракт не ослабляет + существующий инвариант, ради которого `lookupWall()` в своё время сузили. +- **Legacy-путь не расширяется** (§6.2, AC5): key-only записи без exact + endpoints продолжают жить по старому direct/midpoint контракту, что + предотвращает недоказуемое поведение на данных, для которых покрытие нельзя + подтвердить. +- **Мутационный гейт усиливает доказуемость AC1/AC6** — заявленная запись + `atomic-child-thickness-parent-fallback` отключает именно новый fallback и + обязана дать red на unit `wall-thickness` + `open-spans`, что соответствует + правилу 4 `docs/TESTING.md` («тест, охраняющий механизм, сопровождается + мутантом»). +- **Данные/миграция/откат корректны** (§7, §12): формат `walls` не меняется, + миграции нет, downgrade не портит уже сохранённые планы — только ухудшает + наследование при следующем Close, что не является потерей данных. +- **UX/i18n/touch раздел корректно пуст** (§8): исправление меняет только + внутреннее разрешение значения уже существующего действия Close, не + добавляет узлов, строк или новых touch-путей — это согласуется с + `docs/TOUCH-SUPPORT.md` (нет новой поверхности — нет нарушения контракта). +- **Продуктовых вопросов владельцу нет, и это оправданно.** Дефект уже + диагностирован исполнением до постановки в ТЗ (аналитика владельца), ожидаемое + поведение Close «наследовать соседнюю толщину, иначе default» уже + зафиксировано существующим кодом/контрактом `thicknessOnClose()` + (`docs/WALL-THICKNESS.md` не описывает его иначе) — решать на уровне продукта + действительно нечего, что соответствует «принято: открытых продуктовых + вопросов нет» из аналитики. +- **Технические решения корректно помечены как предположения** (§14): + расположение fallback внутри `thicknessCmAt()`, переиспользование текущего + scale-relative tolerance, конкретный файл targeted-смока. Ревьюер эти + предположения не оспаривает — они разумны, не влияют на AC и явно оставлены + свободными для реализации. +- **Release-артефакты названы явно** (§13): оба changelog при `User-Visible: + yes` в implementation-коммите, `docs/TESTING.md`, unit/smoke/mutation; + golden/performance/migration корректно исключены с объяснением почему + (числовая browser-проверка уже покрывает форму, общий renderer не меняется). + +## Находки + +### Low — release-артефакты не называют обновление `docs/WALL-THICKNESS.md` §8 + +**Файл:** `docs/specs/201-atomic-thickness-lookup.md`, раздел 13 +(«Release-артефакты»). + +**Суть:** канонический `docs/WALL-THICKNESS.md` §8 ведёт содержательный +prose-каталог тестовых сценариев подсистемы и по факту обновлялся каждым +предыдущим fix'ом этой же зоны кода — вплоть до последней строки про #197 +(«the complete #197 fixture keeps the same non-empty canonical path…»). ТЗ #201 +называет в качестве «тестового контракта» только `docs/TESTING.md` (AC9, §13), +не упоминая `docs/WALL-THICKNESS.md` §8 вовсе, хотя новый unit/smoke-контур +(exact-parent fallback для атомарного ребёнка, наследование в +`thicknessOnClose()`) — ровно тот уровень детализации, который каталог §8 +исторически фиксирует. + +**Почему не блокирует:** `docs/TESTING.md` — процессно корректный и +достаточный «тестовый контракт» (AC9 named и проверяем); `WALL-THICKNESS.md` +§8 не требуется процессом как отдельный обязательный артефакт, а сам контракт +раздела 1 этого документа («exact endpoints make a thickness boundary +independent of… room topology») уже описывает целевое поведение в общем виде и +не станет неточным без правки — фикс приводит `thicknessCmAt()` в соответствие +с уже написанным инвариантом, а не меняет его. AC и корректность приёмки от +этого не зависят. + +**Решение ревьюера:** снимается без правки ТЗ, с записью в этом документе +(разрешено §2.4/§3 PROCESS.md: «Low либо правится, либо снимается решением +ревьюера с записью»). Рекомендация для реализации: по аналогии с #197 добавить +в `docs/WALL-THICKNESS.md` §8 одну строку про новый сценарий — дёшево и +поддерживает историческую полноту каталога, но отдельного цикла ревью это не +требует и не является условием зелёного вердикта. + +Других находок — Low, Medium или High — не выявлено. + +## Чего не проверял + +- Не проверялась реализация — на этапе ТЗ её не существует; код-ревью будет + отдельным циклом (`S7-code-review`) после написания кода. +- Не запускались `npm test`/`npm run build`/смоки: код не менялся, гейты ТЗ не + требуют их прогона на этом этапе (документация — класс C, продуктовый код не + тронут). +- Не проверялась предложенная реализация tie-break для AC4 «наиболее узкий + span, независимо от порядка» на конкретных крайних случаях (например, + два candidate exact spans с буквально идентичной длиной покрытия) — ТЗ + корректно требует детерминизма как свойства, а не диктует алгоритм; выбор + конкретного правила — техническое решение реализации и предмет код-ревью. +- Не оценивалась реальная browser-сцена AC7/AC10 (файл смока не создан на + этапе ТЗ) — план автотестов (§10.2) описывает сценарий содержательно и + оставляет выбор конкретного файла как явное предположение (§14 п.4), этого + достаточно для стадии ревью ТЗ. +- Не оценивалась производительность на реальном большом плане — риск в §11 + обоснованно сведён к «линейный проход, вне render tick», и это утверждение + проверяется код-ревью, а не спецификацией. + +## Вердикт + +Все обязательные разделы на месте, все AC однозначны и снабжены способом +доказательства, причина дефекта подтверждена чтением кода (включая то, что +предлагаемый алгоритм уже существует и работает в `cmsForPoly()`, а значит не +является непроверенной гипотезой), защита AUD-159B6-01 явно сохранена и +покрыта негативным AC, non-scope точен, mutation-gate усиливает доказуемость +ключевых AC. Открытых продуктовых вопросов нет и это оправданно — решать +здесь действительно нечего. Единственная находка — Low, не влияющая на +корректность контракта, снята с записью. + +**Вердикт: зелёный · цикл r1/4 · High: 0 · Medium: 0 → в задаче**