mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,86 @@
|
||||
# SPEC-REVIEW-282-r1
|
||||
|
||||
- **Issue:** [#282 — Геометрия стен: сменить представление, а не чинить последствия](https://github.com/Matysh/houseplan-card/issues/282)
|
||||
- **Этап:** spec (PROCESS.md §2.4)
|
||||
- **Заход:** r1 (первый; дельта-режим §2.10 не применяется)
|
||||
- **Артефакт ТЗ:** [`docs/specs/282-stable-wall-segment-identity.md`](../specs/282-stable-wall-segment-identity.md)
|
||||
- **SHA ТЗ на момент ревью:** `8856fbda` (`docs: specify stable wall segment identity`, единственный коммит в ветке поверх `dev`)
|
||||
- **Нормативный документ:** [`docs/adr/282-wall-geometry-representation.md`](../adr/282-wall-geometry-representation.md), статус: Stage 0 принят и реализован (#283), Stage 1 — предмет этого ТЗ
|
||||
- **Поставляемый этап:** ADR Stage 1 — stored identity сегментов contour walls
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Issue #282 — зонтичная задача о смене модели геометрии стен, реализуемая стадиями (ADR). Текущий рабочий цикл поставляет только **Stage 1**: стабильный persisted `id` для атомарных contour-сегментов, ссылки комнат на эти id, миграция v7→v8, compatibility-проекция для старого рендерера и старых клиентов. Никакого визуального/UX-изменения не заявлено, кроме одного нового отказного состояния (не удалось безопасно смигрировать план).
|
||||
|
||||
Ревью не покрывает продуктовое качество ADR Stage 0-4 в целом (ADR принят владельцем 2026-08-24 как направление) — только ТЗ Stage 1: выполнимость, однозначность, доказуемость AC1…AC17 и соответствие `docs/SCOPE.md`.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком.
|
||||
2. Прочитано тело issue #282 и все 4 комментария (включая измерение владельца по Stage 0: 65–79% координат его живой инсталляции — «шум» у узла решётки, и признание, что тестовые фикстуры проекта этот класс дефектов не воспроизводят).
|
||||
3. Прочитан ADR `docs/adr/282-wall-geometry-representation.md` целиком.
|
||||
4. Прочитан ТЗ `docs/specs/282-stable-wall-segment-identity.md` целиком (645 строк, §1–§19).
|
||||
5. Прочитаны канонические документы затронутой подсистемы — `docs/WALL-THICKNESS.md`, `docs/CONFIG-COMPATIBILITY.md` — целиком, для проверки, что ТЗ описывает существующую модель точно, а не по памяти/догадке.
|
||||
6. Факт-чек утверждений ТЗ по исходному коду (не запуская тесты — на этом этапе продуктового кода для #282 ещё нет, разбор чтением):
|
||||
- `PLAN_MODEL_VERSION = 7` в `src/plan-optimizer.ts:39` и `custom_components/houseplan/const.py:54` — подтверждает заявленный переход v7→v8 (§0, §7.3, §10) как факт, а не предположение;
|
||||
- `MAX_ROOMS = 400` (`custom_components/houseplan/validation.py:909`, `src/houseplan-card.ts:648`) и `MAX_POLY_POINTS = 500` (`validation.py:930`) дают `200 000`, что дословно совпадает с лимитом §13;
|
||||
- структура `host?: { kind: 'partition'; id: string; t: number }` (`src/opening-placement.ts:46`) подтверждает исходное состояние, которое §6.4 расширяет до tagged union `'partition' | 'wall'`;
|
||||
- `wallKey`, `rekeyWallsAfterMove`, `exactCoveringWall`, `edgeKinds`, `sharedSegsOf`, `atomicPolyForRoom` — все функции существуют в `src/wall-thickness.ts` под теми же именами, что и в §3/ADR;
|
||||
- `src/coordinate-canonicalization.ts` подтверждает, что write-barrier для lattice-координат (issue #291) уже реализован (`canonicalizeConfigGeometry`, `LATTICE_NOISE_STEPS = 1e-4`) — это отдельный от предлагаемого §9-барьера механизм (см. находку M1);
|
||||
- `scripts/config-field-registry.mjs`, `scripts/model-invariants.mjs`, `scripts/mutation-gate.mjs`, `scripts/smoke-select.mjs` существуют — гейты и инструменты, на которые ссылается ТЗ (§10.4, §17), реальны;
|
||||
- i18n-префиксы `toast.*` и `gs.*`, использованные в §12, — существующая конвенция (`src/i18n/en.json`).
|
||||
7. Сверены обязательные разделы ТЗ (PROCESS.md §7.1) против фактической структуры документа и против двух недавних крупных принятых ТЗ той же подсистемы (#291, #302) для калибровки, какой уровень детализации разделов принят как норма в проекте.
|
||||
|
||||
## Находки
|
||||
|
||||
### M1 (Medium, в скоупе). Не специфицирован порядок атомизации Stage 1 относительно уже существующего lattice-барьера #291 — именно там, где живёт заявленная причина задачи
|
||||
|
||||
**Где:** §7.1 «Breakpoints» и §7.2 «Толщина» (`docs/specs/282-stable-wall-segment-identity.md:196–222`), см. также §9 «Writers и единый identity barrier» (`:314–344`).
|
||||
|
||||
**В чём проблема.** §7.1 требует разбивать boundary «в точках… **exact** `walls[].a/b`», §7.2 — «**exact** matching `walls[].a/b`». Проблема, которую решает вся задача (см. ADR и подтверждающий комментарий владельца в issue: 65–79% координат его живой инсталляции лежат «почти на узле, но не на нём», худшее отклонение — 8·10⁻⁸ шага), — это именно то, что «exact» совпадение координат в хранимых данных ненадёжно до канонизации. В проекте уже есть отдельный, независимо реализованный барьер именно для этого (issue #291, `src/coordinate-canonicalization.ts`, `canonicalizeConfigGeometry`, порог `LATTICE_NOISE_STEPS = 1e-4`), а в `docs/WALL-THICKNESS.md` для сопоставления `wallKey` используется **другой** порог, `max(pitch·10⁻⁶, 10⁻⁹)` — то есть в кодовой базе уже сосуществуют два разных допуска для «эта же точка» в разных местах.
|
||||
|
||||
ТЗ Stage 1 нигде не говорит:
|
||||
- должна ли атомизация/breakpoint-matching запускаться **после** прогона существующей lattice-канонизации (#291) в той же транзакции миграции, или на сырых persisted-координатах;
|
||||
- если «exact» — это буквально бит в бит, атомизация того самого плана владельца, который стал поводом для задачи, будет либо создавать лишние микро-сегменты на каждом зашумлённом узле, либо (что хуже) детерминированно проваливать миграцию по «conflicting thickness»/«zero-length» блокировкам из §7.1, оставляя план в вечном «честном отказе» (§2);
|
||||
- какой из двух существующих допусков (`1e-4` или `max(pitch·10⁻⁶,10⁻⁹)`) применяется к breakpoint-сопоставлению, либо что нужен третий.
|
||||
|
||||
Это не гипотетический краевой случай: это ровно тот датасет (собственная инсталляция владельца), который стал причиной задачи и явно процитирован в issue.
|
||||
|
||||
**Почему это Medium, а не High.** AC1–AC17 остаются осмысленными и не противоречат друг другу — вопрос решается одним предложением («атомизация переиспользует существующий lattice-канонизационный допуск #291 и запускается после него в той же commit-транзакции» или явным альтернативным решением) без изменения границ скоупа/не-скоупа и без продуктового вопроса владельцу: это техническое решение, которое ТЗ вправе принять само (PROCESS.md §7.1: «всё, чего пользователь не наблюдает, агенты решают сами»). Но без этого предложения AC1 («дважды дают byte-equivalent v8 candidate», «lossless») не имеет однозначного критерия выполнения на данных с реальным шумом, а разработчик и код-ревьюер не смогут договориться, что именно «exact» означает при коде-ревью.
|
||||
|
||||
**Что делать:** добавить в §7.1/§9 (или в §19 как явное принятое предположение) одно явное утверждение о порядке/допуске: атомизация запускается на уже канонизированных (через #291-барьер) координатах, использует тот же порог `1e-4`/шаг, либо явно назвать другой порог и почему.
|
||||
|
||||
### M2 (Medium, в скоупе). Заявленный UX отказа («предлагает Optimize либо исправление») не совпадает с фактическим текстом тоста в §12
|
||||
|
||||
**Где:** §2 «Что человек увидит до и после» (`:30–34`) против §12 «i18n» (`:395–405`) и §11 (`:390–391`).
|
||||
|
||||
**В чём проблема.** §2 обещает: «диалог/тост называет причину **и предлагает Optimize либо исправление конфликтной геометрии**». §11 повторяет контракт: «failure UI… только локализованный класс blocker **и действие пользователя**». Но единственный определённый в §12 ключ для этого случая —
|
||||
|
||||
`toast.wall_model_migration_blocked` → «Не удалось обновить модель стен: {reason}. План не изменён.» / «The wall model could not be updated: {reason}. The plan was not changed.»
|
||||
|
||||
— не содержит ни предложения запустить Optimize, ни какого-либо другого «действия пользователя». Для сравнения, второй ключ (`toast.wall_model_client_outdated`) действие называет прямо: «Обновите карточку и перезагрузите страницу…». Несоответствие между §2/§11 (обещан совет) и §12 (текста совета нет) — это ровно тот класс дефекта, который правило «одно число — один источник» (PROCESS.md §8) просит ловить для любой величины, видимой пользователю дважды: здесь дважды описан один и тот же UI-элемент двумя разными источниками текста, и они расходятся. Разработчик не может однозначно закрыть AC8 (атомарный отказ, «reason называет причину») по этому ключу — неясно, входит ли в её текст указание действия, или §2/§11 нужно привести в соответствие с §12.
|
||||
|
||||
**Почему Medium, не High:** решается либо правкой §2/§11 (убрать обещание конкретного совета — «называет причину» без «предлагает Optimize»), либо добавлением текста действия в i18n-таблицу; выбор — вкус автора ТЗ, не требует владельца.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Соответствие SCOPE.md/J6.** Задача явно закрывает J6 «Keep the plan true as the home evolves» и не расширяется на новую функциональность — §5 «Не-скоуп» корректно исключает Stage 2–4, новый UI, `cm:0`-функциональность #306 (который приостановлен и явно не открывается этим ТЗ).
|
||||
- **Продуктовые разделы (§7.1 первых два пункта).** §1 «Сценарий» называет персону из SCOPE.md (администратор дома) и момент; §2 формулирует «до/после» в одном абзаце без терминов реализации — обе секции присутствуют и различимы, как того требует PROCESS.md §7.1.
|
||||
- **Отсутствие незаявленных догадок.** Ключевые фактические утверждения ТЗ (текущая модель v7, `MAX_ROOMS/MAX_POLY_POINTS`, существующая структура opening host, существование `wallKey`/`rekeyWallsAfterMove`/`exactCoveringWall` и т. д.) проверены построчно против кода — расхождений не найдено. Технические решения (алгоритм детерминированного ID §7.3, tie-break правила split/merge §8.2–8.3, single-writer barrier module) корректно помечены как «assumed, change freely» в §19 и не эскалированы владельцу, хотя они не банальны — соответствует PROCESS.md («смешанный вопрос делится», «владельцу — только продуктовые»).
|
||||
- **AC1–AC17 в целом однозначны и проверяемы.** Каждый AC называет конкретный наблюдаемый инвариант (unique ID/owner count 1-2, идемпотентность миграции, barrier bypass mutant, read-only не пишет model_version, fail-closed старого клиента, лимиты и performance-бюджеты с конкретными числами) и способ доказательства (unit/backend/smoke/golden/source-guard), что удовлетворяет требованию §2.5 «у каждого AC указано, чем он доказывается» — форма («Доказательство: …» под каждым AC, без отдельного заголовка «План автотестов») совпадает с недавно принятой практикой того же поднабора ТЗ (#291, #302), так что это не дефект структуры.
|
||||
- **Не-скоуп корректно ограничивает риск.** Явно исключены integer lattice (Stage 2), отказ от `rooms[].poly` (Stage 3), closed-form junction (Stage 4), удаление legacy reader — держит риск задачи (10/10 по аналитике) в границах, которые действительно можно проверить одним циклом ревью.
|
||||
- **Откат и release-артефакты (§17–18) реалистичны**: до первой v8-записи откат — чистый revert кода; после — только штатный backup/Undo, без автоматического downgrade v8→v7 (обоснованно запрещён, т.к. теряет hosted identity).
|
||||
- **Ссылки на существующий тулинг и гейты (§10.4, §17) верны**: все названные скрипты (`config-field-registry.mjs`, `model-invariants.mjs`, `mutation-gate.mjs`, `smoke-select.mjs`) существуют в `scripts/`.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не читал построчно `docs/ARCHITECTURE.md`, `docs/CANVAS.md`, `docs/UX-MODES.md`, `docs/TOUCH-SUPPORT.md` целиком — ограничился тем, что уже известно из `docs/SCOPE.md`/`AGENTS.md` и прямыми grep-проверками (renderer/junction-код в ТЗ явно не переписывается, что подтверждается прочитанным `WALL-THICKNESS.md`). Для Stage 1, который сам заявляет «renderer не меняется», это достаточно; при код-ревью реализации у ревьюера кода будет диапазон коммитов, где эти документы стоит перечитать целиком, если изменения затронут рендер сильнее, чем заявлено.
|
||||
- Не проверял историю issues #244/#248/#249/#252/#253/#258/#261/#271–#280/#288–#302, упомянутых как регрессионный пол AC12/AC1, на предмет того, что у каждого из них действительно есть тест в текущем `npm test` — на этапе ТЗ формулировка AC («все тесты … остаются зелёными») достаточна; фактическую зелёность гоняет код-ревью.
|
||||
- Не запускал `npm run typecheck`/`npm test`/`npm run build` — на этапе spec-review для #282 продуктового кода ещё нет (только текстовый ТЗ-файл), гонять гейты нечего.
|
||||
- Не оценивал реалистичность SHA-хэш-коллизионной схемы §7.3 математически (вероятность коллизии base32(sha256)[0:20]) — оценил как явно избыточно низкую для практических объёмов (`MAX_ROOMS×MAX_POLY_POINTS = 200 000`) и не требующую отдельного доказательства на этапе ТЗ.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Документ методологически сильный: почти каждое утверждение о существующей модели подтверждено кодом, AC однозначны и проверяемы, скоуп/не-скоуп жёстко держат границу Stage 1. Две находки M1 и M2 — обе в скоупе задачи, обе решаются правкой текста ТЗ без изменения архитектуры или обращения к владельцу. Без High-находок это жёлтый вердикт.
|
||||
|
||||
`Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче · Документ: docs/reviews/SPEC-REVIEW-282-r1.md`
|
||||
Reference in New Issue
Block a user