mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
committed by
Sergey Matyunin
Sergey Matyunin
parent
e333f0b2f1
commit
6f67d53665
@@ -0,0 +1,196 @@
|
||||
# SPEC-REVIEW-288-r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/288
|
||||
- **Этап:** spec (PROCESS.md §2.4)
|
||||
- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (правило #227 —
|
||||
зелёный вердикт бюджет не тратит, но этот вердикт не зелёный)
|
||||
- **Документ ТЗ:** `docs/specs/288-bounded-multiwall-corridor.md`
|
||||
- **SHA на момент ревью:** `cb0f9e7351e919701666dfd05b53f0e218e672a5`
|
||||
(коммит `docs: specify bounded multiwall corridor`)
|
||||
- **Первый заход** — раздел «объём по дельте» (§2.10) не применяется, разбор
|
||||
полный.
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Проверено: тело issue #288 и оба комментария (аналитика + «ТЗ готово»),
|
||||
`docs/specs/288-bounded-multiwall-corridor.md` целиком, `docs/SCOPE.md`,
|
||||
`docs/WALL-THICKNESS.md` (канонический документ подсистемы), фрагменты
|
||||
`src/wall-thickness.ts` (константы `MITRE_LIMIT`/`MULTI_WALL_JOIN_LIMIT`,
|
||||
типы `MultiWallNodeRay`/`MultiWallNodeRaySupport`, функции
|
||||
`multiWallBevelCutsAt`, `multiWallRayStripGeometry`, `bevelMultiWallBody`),
|
||||
`demo/smoke_real_plan_masonry.mjs`, `test/fixtures/real-plan-second-floor.json`
|
||||
(структура), `scripts/model-invariants.mjs` (наличие `--config`), и для
|
||||
сравнения структуры — специфи́кации `docs/specs/275-multiwall-strip-containment.md`
|
||||
и `docs/specs/278-wall-union-isolation.md` (та же подсистема, сопоставимый
|
||||
риск/сложность — использованы как прецедент требуемых разделов).
|
||||
|
||||
Как проверялось: ревью текстовое — сверка формулировок ТЗ с телом issue
|
||||
(числа/причина совпадают буквально), с идентификаторами в реальном коде
|
||||
(`MITRE_LIMIT`, `node.halfDepth`, `MultiWallNodeRay.supports` — все существуют
|
||||
и используются именно так, как описывает контракт §3.1), с формулировками
|
||||
`docs/WALL-THICKNESS.md` (ссылки на #249/#261/#271/#272/#275/#278/#279 в AC4
|
||||
и в тексте документа совпадают с уже задокументированным поведением, не
|
||||
изобретены), и структурное — сверка присутствия обязательных разделов §7.1
|
||||
с прецедентными спеками той же подсистемы. Продуктового кода в диапазоне нет
|
||||
(diff ограничен `docs/specs/**`, класс C) — гейты §8 к этапу spec не относятся
|
||||
и не запускались; это гейты этапа code (§2.7).
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе задачи — чинится в этом же ТЗ)
|
||||
|
||||
**M1. Отсутствуют обязательные разделы «Риски» и «Откат» (§7.1, DoR §2.5).**
|
||||
|
||||
`docs/specs/288-bounded-multiwall-corridor.md` не содержит ни одного раздела
|
||||
с анализом рисков, ни раздела откат/rollback. Единственное упоминание слова
|
||||
«риск» — оценочная цифра в шапке (`риск 8/10`), без содержания. Проверено
|
||||
`grep -n -i "риск\|rollback\|откат"` по всему файлу — единственное совпадение
|
||||
строка 7.
|
||||
|
||||
PROCESS.md §7.1 перечисляет обязательные разделы ТЗ явно: «…план автотестов ·
|
||||
риски · откат · release-артефакты» — оба отсутствующих раздела в списке есть.
|
||||
DoR §2.5 повторяет то же как отдельные обязательные пункты очереди: «влияние
|
||||
на производительность… названо», «**откат**: как выключить или вернуть
|
||||
назад», «риски перечислены». Пока эти пункты не выполнены, статус не может
|
||||
стать «Готово к разработке» буквально по тексту §2.5: «Если хоть один пункт
|
||||
не выполнен — статус не «Готово к разработке», как бы ни хотелось начать».
|
||||
|
||||
Это не абстрактная формальность именно для данной задачи: сам автор оценил
|
||||
риск как 8/10 и сложность 7/10 (P1, обычный трек — не `small`), а фикс трогает
|
||||
общий для всех T/L/multi-wall стыков код (`bevelMultiWallBody`,
|
||||
`multiWallBevelCutsAt`), от которого зависят уже закрытые контракты #249,
|
||||
#261, #271, #272, #275, #278, #279 — то есть реальный риск регрессии высок и
|
||||
уже отражён самим объёмом AC4, но нигде не назван явно как риск с мерой
|
||||
(«риск: регрессия одного из семи прежних контрактов → мера: AC4 перегоняет
|
||||
весь их набор»). Откат для правки без миграции и без флага — скорее всего
|
||||
«просто revert коммита», но это должно быть сказано, а не подразумеваться:
|
||||
раздел 6 («Совместимость…») описывает только *отсутствие* новых
|
||||
compatibility-полей, а не механику отката самой правки.
|
||||
|
||||
Прецедент: спеки той же подсистемы и сопоставимого риска — `#275`
|
||||
(`docs/specs/275-multiwall-strip-containment.md`, разделы «11. Риски и меры» и
|
||||
«12. Rollback») и `#278` (`docs/specs/278-wall-union-isolation.md`, разделы
|
||||
«12.1. Риски и меры» и «17. Release-артефакты и rollback») — оба раздела
|
||||
присутствуют. #288 того же класса дефекта и не легче ни по одной из этих
|
||||
двух метрик, но раздела не имеет.
|
||||
|
||||
**Чем закрывается:** добавить в ТЗ раздел «Риски и меры» (минимум: риск
|
||||
регрессии multi-wall контрактов #249/#261/#271/#272/#275/#278/#279 → мера
|
||||
AC4; риск undershooting/overshooting отсечения у соседней стены → мера AC3 +
|
||||
AC7 mutation) и раздел «Откат» (одна фраза: чистый revert коммита, миграции и
|
||||
флага не требуется, поскольку модель данных не меняется). Это не требует
|
||||
пересмотра геометрического контракта — правка текстовая.
|
||||
|
||||
### Low
|
||||
|
||||
**L1. «Связано» в шапке не включает #279, хотя AC4 явно на него ссылается.**
|
||||
|
||||
Шапка документа (строка 11) перечисляет «#249, #261, #271, #272, #275, #278,
|
||||
#284–#286», но AC4 (строка 127) прямо требует «near-orthogonal #279 остаётся
|
||||
защищённым». #279 — тот же класс контрактов, задействован в проверке
|
||||
регрессии, и разумно ожидать его в списке связанных issue. Не блокирует
|
||||
проверяемость ни одного AC и не влияет на выполнимость — чисто
|
||||
трассируемость документа. Решение оставляю на автора: либо дополнить строку
|
||||
11, либо снять как не влияющее (в этом случае — с записью в документе, а не
|
||||
молчанием, по требованию §12 «Оставили в тексте ревью не считается
|
||||
закрытием» — здесь это Low, а не Medium, поэтому достаточно осознанного
|
||||
снятия).
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Раздел «Сценарий» и диагноз причины (п.1).** Числа `349/120/5` шагов,
|
||||
толщины `30/30/30` см, соседняя стена `20` см, радиус
|
||||
`MITRE_LIMIT × H = 4 × 15 = 60`, вырез `60 − 15 = 45` — воспроизводят тело
|
||||
issue буквально, без искажений и без добавления недоказанных деталей.
|
||||
Диагноз в issue помечен владельцем как «Подтверждённая причина» (совпадение
|
||||
вычисленной величины с измеренной), так что повторение его в ТЗ как факта —
|
||||
не догадка, выданная за решение, а корректное наследование уже
|
||||
подтверждённого вывода.
|
||||
- **Идентификаторы контракта существуют в коде.** `MITRE_LIMIT` (`src/wall-thickness.ts:78`),
|
||||
`MultiWallNodeRay.supports`/`halfDepth` (там же, интерфейсы объявления и
|
||||
использование в `multiWallRayStripGeometry`/`bevelMultiWallBody`) — контракт
|
||||
§3.1 ссылается на реально существующие структуры, а не на вымышленные.
|
||||
- **AC1/AC2 привязаны к существующему воспроизводимому инструменту.**
|
||||
`demo/smoke_real_plan_masonry.mjs` уже в дереве (класс B, коммит
|
||||
`c654e0ec`/`d5659478`), считает разрывы по каждому ребру каждой комнаты
|
||||
(не только по четырём известным точкам), различает объявленный `open_span`
|
||||
от дефекта. Требование AC1 «gapCount: 0, totalGapSteps: 0 плюс обновление
|
||||
чисел `PLANS` в том же коммите» и AC2 «first-floor остаётся на нуле» —
|
||||
однозначны и проверяются одной командой.
|
||||
- **AC3 — table-driven unit, синтетическая, но осознанно упрощённая
|
||||
конфигурация** (три луча `349/120/5` при `30/30/20` см вместо реальных
|
||||
четырёх стен узла) корректна как проверка контракта §3.1 в отрыве от
|
||||
конкретной топологии реального плана: контракт формулируется через
|
||||
собственный half-depth соседнего ray, а не через воспроизведение точной
|
||||
геометрии дома. AC1 остаётся источником истины для реального дефекта,
|
||||
AC3 — независимая unit-гарантия того же правила на разных `cell_cm` и
|
||||
порядках лучей.
|
||||
- **AC4** — список регрессионных контрактов (#249 дискардед wedge,
|
||||
#261 retained wedge, #271 finite rays, #272 enclosed holes, #275 protected
|
||||
strips, #278 union failure isolation, #279 near-orthogonal) совпадает слово
|
||||
в слово с формулировками §3 `docs/WALL-THICKNESS.md`, не выдуман.
|
||||
- **AC7 (мутант)** — явно требует, чтобы порча корридора до зависимости
|
||||
только от `MITRE_LIMIT × node.halfDepth` валила AC1 или AC3; это тест на
|
||||
«умеет падать», а не декларация покрытия.
|
||||
- **Раздел 4 «Scope»**, особенно «Не входит» (никакого переписывания
|
||||
persisted-данных, никакого изменения глобальных констант, никакой новой
|
||||
модели стен из ADR #282) корректно ограничивает скоуп и явно исключает
|
||||
соседние задачи #289/#290, не позволяя дефекту раздуться в рефакторинг.
|
||||
- **Раздел 8 «Порядок интеграции»** относительно ветки `#260`
|
||||
(`issue/260-fixture-wall-keys`) корректно и без искажений повторяет
|
||||
требование самого issue («влить до пересъёмки скриншотов») и явно
|
||||
оговаривает, что #260 не входит в продуктовый скоуп #288 — не является
|
||||
попыткой расширить скоуп чужой задачей.
|
||||
- **«Принятые технические предположения» (п.9)** — все четыре пункта
|
||||
действительно технические (верхняя граница ущерба, конкретный алгоритм
|
||||
connector-а, объём фикстур, touch-политика), ни один не является
|
||||
продуктовым вопросом, который следовало бы адресовать владельцу; блок
|
||||
оформлен явно как предположение, что соответствует требованию §7.1
|
||||
(«принято предположительно, поменять свободно»), а не выдаёт догадку за
|
||||
факт.
|
||||
- **Совместимость/touch/performance (п.6)** — «модель данных не меняется»
|
||||
подтверждается отсутствием любых изменений схемы/`layout`/`Optimize` во
|
||||
всём документе; touch-формулировка («View на touch и kiosk получает тот же
|
||||
canonical result») соответствует терминологии `docs/TOUCH-SUPPORT.md`
|
||||
(View — гарантированная поверхность, редактор — best effort), не изобретает
|
||||
новый термин.
|
||||
- **Продуктовых вопросов владельцу нет** — и это корректно: единственная
|
||||
видимая пользователем перемена — «стены рисуются без разрыва», без нового
|
||||
UX-контракта, без выбора между персонами. Открытых развилок, требующих
|
||||
продуктового решения владельца, в тексте не найдено.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не запускал `npx tsc --noEmit` / `npm test` / `npm run build` /
|
||||
`node scripts/check-docs.mjs` — diff ограничен `docs/specs/288-*.md`
|
||||
(класс C), продуктовый код и тесты не менялись; эти гейты относятся к
|
||||
этапу code-review (§2.7) и будут обязательны там.
|
||||
- Не выполнял `node demo/smoke_real_plan_masonry.mjs` живьём — на этапе spec
|
||||
еще нет реализации; текущий (добавленный ранее, в другом issue) результат
|
||||
`gapCount: 4, totalGapSteps: 181` уже зафиксирован в самом smoke-файле и
|
||||
подтверждён текстом issue, повторный прогон здесь ничего нового не даёт.
|
||||
- Не проверял, действительно ли предложенная в AC3 синтетическая
|
||||
конфигурация (3 луча, включая «соседнюю» стену как один из лучей) технически
|
||||
достаточна для воспроизведения именно того случая, где четвёртая
|
||||
(не инцидентная узлу) стена теряет материал — если в реальности дефект
|
||||
затрагивает стену, geometрически не входящую в `node.rays` того же узла,
|
||||
синтетический repro AC3 может не поймать в точности этот путь кода. Это не
|
||||
повод для находки: AC1 (реальный smoke) остаётся источником истины
|
||||
независимо от того, что покажет AC3, и авторская реализация обязана
|
||||
удовлетворить AC1 в любом случае. Точную топологию дефектного пути стоит
|
||||
перепроверить на этапе code-review по фактическому диффу.
|
||||
- Не проверял golden baseline и её текущее состояние (`artifacts/golden/`) —
|
||||
AC6 описывает будущую работу (targeted golden для реального плана ещё не
|
||||
существует), нечего сверять до реализации.
|
||||
|
||||
## Вердикт
|
||||
|
||||
High-находок нет. Одна Medium-находка (M1) — в скоупе задачи, не требует
|
||||
отдельного issue (#202) и чинится добавлением двух коротких разделов в тот же
|
||||
файл ТЗ. Одна Low-находка (L1) — на решение автора (дополнить или снять с
|
||||
записью).
|
||||
|
||||
```
|
||||
Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 1 → в задаче
|
||||
Документ: docs/reviews/SPEC-REVIEW-288-r1.md
|
||||
```
|
||||
Reference in New Issue
Block a user