From 46a83de1816b44b1010b806975431282a17ba740 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 10:57:41 +0000 Subject: [PATCH] docs: review document for #258 Issue: #258 User-Visible: no --- docs/reviews/SPEC-REVIEW-258-r1.md | 190 +++++++++++++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-258-r1.md diff --git a/docs/reviews/SPEC-REVIEW-258-r1.md b/docs/reviews/SPEC-REVIEW-258-r1.md new file mode 100644 index 00000000..a0346b9c --- /dev/null +++ b/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 коммита, миграции нет), +открытых продуктовых вопросов нет и не должно было быть. Задача готова к +статусу «Готово к разработке».