docs: review document for #258

Issue: #258
User-Visible: no

docs/reviews/SPEC-REVIEW-258-r1.md

@@ -0,0 +1,190 @@
+# SPEC-REVIEW-258-r1
+
+- Issue: [#258](https://github.com/Matysh/houseplan-card/issues/258) — «На 1.67.0-beta.4 после «Оптимизировать» появились белые клинья в местах схода стен»
+- ТЗ: `docs/specs/258-wall-key-storage-roundtrip.md`, ветка `issue/258-wall-key-storage-roundtrip`, SHA `a830184`
+- Этап: spec (PROCESS.md §2.4), трек — обычный (не `small`/`trivial`, метки `bug`/`P1`/`S4-spec-review`)
+- Заход: r1 · блокирующих циклов израсходовано 0 из 4
+- Вердикт: **зелёный**
+
+## Скоуп
+
+Регресс после «Оптимизировать»: `wallKey()` квантует середину стены через
+`Math.round`, и для стены нечётной длины в шагах сетки середина попадает точно
+на границу округления. Точное узловое представление вершины и её
+девятизнаковое persisted-представление (#224) дают два разных результата
+округления → два разных строковых `key` для одного и того же физического
+ребра. `lookupWall()` не находит запись по несовпавшему ключу и не спасается
+терпимым запасом (`tol` = ровно полшага = ровно величина ошибки), тогда как
+`thicknessCmAt()` находит её же по точным `a/b` через `exactCoveringWall()`.
+Расхождение потребителей даёт белый клин в T-стыке. ТЗ фиксирует это как
+регресс J1/J6 (`docs/SCOPE.md`) и предлагает: стабилизировать генерацию key
+near-grid нормализацией endpoint'ов, добавить строгий same-span lookup по
+точным `a/b` как средний шаг между exact-key и legacy midpoint fallback, и
+новый точный (без допусков) модельный инвариант `wall_key_mismatch`.
+
+## Как проверялось
+
+Это исключительно техническая задача (persisted-представление одного и того
+же числа, лишённая пользовательского выбора), поэтому основная работа ревью —
+проверить, что утверждения ТЗ о коде верны, а не гадать о продуктовой
+неоднозначности, которой здесь почти нет.
+
+1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` §2.4/§7.1/§7.2, `AGENTS.md`,
+   `docs/USER-GUIDE.ru.md` (раздел «Что делает оптимизация» и текст диалога
+   Optimize), канонический `docs/WALL-THICKNESS.md` и
+   `docs/CONFIG-COMPATIBILITY.md` (раздел «Каноническая геометрия на запись
+   (#224)»).
+2. Прочитано тело issue #258 целиком и оба комментария (аналитика владельца +
+   ссылка на ТЗ). Открытых продуктовых вопросов не оставлено — подтверждено
+   и автором, и содержанием ТЗ (§15, шесть явных технических допущений).
+3. Вытянута ветка `issue/258-wall-key-storage-roundtrip` (SHA `a830184`),
+   `git diff` от `dev` показывает только `docs/specs/258-*.md` и одну строку
+   в `docs/specs/README.md` — продуктовый код не тронут, что ожидаемо для
+   стадии spec.
+4. Каждое техническое утверждение ТЗ сверено с реальным `src/wall-thickness.ts`
+   (3382 строки), `src/plan-optimizer.ts`, `src/coordinate-canonicalization.ts`,
+   `src/space-geometry.ts`, `scripts/model-invariants.mjs` на этой ветке:
+   - `wallKey()`/`q()`/`lookupWall()`/`exactCoveringWall()`/`cmsForPoly()`/
+     `edgeKinds()`/`rekeyWallsAfterMove()` существуют ровно с тем поведением,
+     которое им приписывает ТЗ (файл `wall-thickness.ts:147-489, 1299-1360`).
+   - `GRID_N = 240`, `GRID_STEP_N = 1/240` — `space-geometry.ts:201,205`.
+   - `canonicalizeNumber()` округляет до 9 знаков (`COORDINATE_DECIMALS = 9`) —
+     `coordinate-canonicalization.ts:8,23-28`, `canonicalizeConfigGeometry()`
+     существует и используется в `plan-optimizer.ts:541` и
+     `houseplan-card.ts:7042`.
+   - `rekeyWallsAfterMove()` уже использует `exactEps = pitch * scale * 1e-6`
+     (мин. `1e-9`) — это ровно та величина, которую ТЗ §6.1 предлагает как
+     key-epsilon, и ТЗ прямо говорит, что она совпадает с уже используемой
+     точностью. Подтверждено чтением (`wall-thickness.ts:449`).
+   - `scripts/model-invariants.mjs` не импортирует `src/**`, читает сырой JSON
+     — соответствует требованию §8 ТЗ. Существующий `checkReferences()`
+     проверяет только «конец записи лежит на ребре комнаты» с допуском
+     `EDGE_TOLERANCE = 0.004` (`model-invariants.mjs:23,74-135`) и не
+     сравнивает `key` с `wallKey(a,b)` — подтверждает, что предлагаемый
+     `wall_key_mismatch` действительно новая проверка, а не дубликат.
+   - `test-build/wall-thickness.js` — устоявшийся паттерн проекта
+     (`package.json` script `test`, `demo/benchmark_optimize_geometry_preflight.mjs`,
+     несколько прошлых `CODE-REVIEW-*.md`), так что AC4's «parity guard с
+     `test-build/wall-thickness.js`» — не выдумка, а существующий механизм.
+   - Партиции (`space.partitions`) хранят `{a,b,cm}` без производного
+     строкового `key` (`houseplan-card.ts:12945`, `partition-openings.ts`) —
+     этот класс дефекта их не касается, и ТЗ корректно не включает их в
+     scope/non-scope отдельной строкой.
+   - В backend (`custom_components/houseplan/**/*.py`) `wallKey`/`wall_key` не
+     встречается — допущение §15.6 «backend не меняется» подтверждено.
+5. Численно пересчитан пример из issue в Node (`Math.round`, `GRID_STEP_N`):
+   середина стены 1 по точному узлу `83/240` даёt `47.500000000 → 48 →
+   0.200000`, по девятизнаковому `0.345833333` — `47.499999960 → 47 →
+   0.195833`. Совпадает с issue буквально до шестого знака.
+6. Проверено, что предложенная в §6.1 near-grid нормализация endpoint'ов
+   (`eps = max(pitch·1e-6, 1e-9) ≈ 4.1667e-9`) действительно устраняет тай-брейк:
+   после снапа `0.345833333` к узлу `83/240` (расхождение `3.33e-10 < eps`)
+   середина обеих версий совпадает и даёт один и тот же `key` (`0.200000`).
+   Это не «предположение, которое звучит правдоподобно» — я исполнил формулу.
+7. Проверен `docs/USER-GUIDE.ru.md:1374-1414` (диалог Optimize) на предмет
+   утверждения ТЗ §7.1 «исправление key считается технической канонизацией
+   стен в существующем отчёте» — категории отчёта («обновлённое представление
+   стен/связей» и «устранённый вычислительный шум») действительно существуют
+   и правдоподобно покрывают этот случай; см. «Находки» Low-1 ниже.
+8. Проверено структурное соответствие §7.1 PROCESS.md: сценарий/персона,
+   «что человек увидит», проблема (роль играет §3 «Подтверждённая причина»),
+   скоуп/не-скоуп, контракт поведения, UX/touch/perf/security, критерии
+   приёмки с доказательством, план тестов (раздел 10 таблица + раздел 11
+   шаги 4-6), риски, откат, release-артефакты — все разделы присутствуют по
+   содержанию.
+9. Проверено на «догадку, выданную за факт» (§7.1): раздел 15 явно выделяет
+   шесть принятых технических допущений вместо того, чтобы включить их
+   безадресно в контракт; ни одно из них не является скрытым продуктовым
+   решением — все технические (где резолвится repair, накопление в отчёте,
+   формат compatibility). Ни одно не требовало эскалации владельцу, что
+   совпадает с его собственным «Открытых продуктовых вопросов нет».
+
+Гейты `typecheck`/`test`/`build`/`check-docs` не прогонялись: диапазон diff —
+только `docs/specs/**`, продуктовый код (`src/**`) не тронут, стадия spec, а
+не code review. Смоки/golden/invariants аналогично не прогонялись — они
+проверяют поведение кода, которого в этой ветке ещё нет; они относятся к
+будущему code-review циклу этой же задачи.
+
+## Находки
+
+### Low-1 — «Что человек увидит» смешивает продуктовую и техническую лексику
+
+`docs/specs/258-wall-key-storage-roundtrip.md`, раздел 2. Формулировка «До»
+начинается с «Optimize способен создать либо закрепить пару `key` и `a/b`,
+полученную из разных floating-point представлений одного ребра» — это
+описание механизма, а не то, что видит человек. PROCESS.md §7.1 требует эту
+секцию «одной фразой, без терминов реализации»; здесь встречаются `key`,
+`a/b`, «floating-point representations», «midpoint-key».
+
+**Почему не блокирует:** несмотря на терминологию, ответ на оба обязательных
+вопроса — какая персона, где, что видно до/после — читается однозначно из
+того же раздела (белый клин в T-стыке, не лечится reload; после — непрерывная
+кладка независимо от того, какая версия ключа была сохранена). Раздел 1
+(«Сценарий») отдельно и чисто формулирует персону/поверхность/момент. AC5/AC6
+проверяют ровно видимый результат, а не строковый key, так что и приёмка не
+зависит от формулировки этого абзаца.
+
+**Решение ревьюера:** снимаю как Low с записью, автор может (не обязан)
+переформулировать первую фразу «До» в следующей редакции без блокировки
+текущего захода.
+
+## Что проверено и корректно
+
+- **Причина бага и её локализация** — подтверждены и чтением кода, и
+  численным пересчётом; ни один из числовых примеров ТЗ не «на глаз».
+- **Контракт key (§6.1–6.3)** — численно проверено, что предложенная
+  нормализация endpoint'ов действительно устраняет tie-break на нечётных
+  длинах, не трогая формат строки и не расширяя допуск (`tol` остаётся
+  `pitch/2`, новый epsilon на пять порядков меньше него).
+- **AC1–AC8** — каждый однозначен, у каждого назван способ доказательства
+  (`unit`/`smoke`/`golden`/составной gate) в формате, достаточном для DoR
+  (§2.5); дисциплина «mutant, убирающий фикс» присутствует в AC1–AC4, что
+  соответствует требованию «тест умеет падать» уже на этапе постановки.
+- **Scope/non-scope** — явно исключены смежные форматы/миграции (#224,
+  `GRID_N`, bevel-геометрия #249, partial-resize контракт #253, auto-Apply,
+  backend/schema/API), что не даёт задаче расползтись на соседние issue.
+- **Совместимость** — legacy `{key, cm}` без `a/b` не трогается; несовпадающий
+  key не делает import невалидным и не позволяет удалить запись — это прямое
+  и корректное применение принципа `docs/SCOPE.md` «никогда не удалять
+  данные пользователя по догадке» к записям толщины.
+- **Инвариант (§8)** — предлагаемая проверка `key === wallKey(a,b,1/240)`
+  действительно новая: существующий `checkReferences()` её не делает
+  (проверено чтением `model-invariants.mjs`), и она ловит оба случая из
+  issue без допусков, в отличие от уже существующей carrier-проверки,
+  которая, как верно замечено в issue, стоит ровно на той же границе допуска
+  и потому её не ловит.
+- **Технические допущения (§15)** — все шесть действительно технические, не
+  подменяют продуктовое решение и не требовали вопроса владельцу; это
+  соответствует правилу «смешанный вопрос делится, а не эскалируется целиком»
+  и корректно отражено как «открытых продуктовых вопросов нет» в комментарии
+  автора.
+- **Артефакты и трассируемость** — issue ↔ ТЗ ссылки в обе стороны на месте
+  (issue-комментарий → blob ТЗ; ТЗ → issue в шапке), `docs/specs/README.md`
+  обновлён той же веткой.
+
+## Чего не проверял
+
+- Продуктовый код не существует ещё на этой ветке — реализация, её тесты,
+  golden/smoke-сценарии и производительность будут предметом code-review
+  этой же задачи, не этого захода.
+- Не проверял вручную UI диалога Optimize (нет кода, нечего запускать);
+  утверждение о категории отчёта (Low-1 контекст) проверено только по тексту
+  `USER-GUIDE.ru.md`, не по факту работы кода.
+- Не проверял состояние второго пространства владельца (`smt2ntdrc`) — в
+  issue это числа без файла, экспортов у меня нет; для ревью ТЗ это не нужно,
+  для code-review стоит убедиться, что общий unit/mutation-набор AC1–AC2
+  покрывает обе формы (30/20/20/30 см из второго пространства), а не только
+  числа первого.
+- Не проверял ссылки на `AUD-159B6-01` и `#201` в таблице рисков — они
+  использованы как исторический контекст, не как проверяемое утверждение
+  этого ТЗ, и не влияют на выполнимость или проверяемость AC.
+
+## Вывод
+
+ТЗ технически безупречно для такой узкой числовой задачи: все нетривиальные
+утверждения о коде, формулах и константах подтверждаются чтением исходников
+на этой же ветке и, где это осмысленно, прямым численным пересчётом. Единственная
+находка — Low, стилистическая, снята с запиской. AC проверяемы и однозначны,
+scope/non-scope чёткие, откат тривиален (только revert коммита, миграции нет),
+открытых продуктовых вопросов нет и не должно было быть. Задача готова к
+статусу «Готово к разработке».