docs: review document for #330

Issue: #330
User-Visible: no

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

@@ -0,0 +1,228 @@
+# SPEC-REVIEW-330-r1
+
+Issue: #330 · Этап: spec (ТЗ на ревью, PROCESS.md §2.4) · Заход r1 · блокирующих циклов 0/4
+
+Материал: `docs/specs/330-junction-limits-performance.md` на SHA `f1b7c237`
+(HEAD ветки `issue/330-junction-limits-performance` на момент ревью), тело issue
+#330 и оба комментария (S2-аналитика, объявление ТЗ).
+
+## Скоуп
+
+ТЗ описывает исключительно производительность существующей валидации
+ограничений стыков (#329): вынос цепочки валидаторов `ws_config_set`/
+`ws_plan_optimize` в executor, rev-кэш счётчиков `previous`, линеаризация П3
+(`checkSegmentLengths`/`check_segment_lengths`) через переиспользуемый
+byNode-индекс, и фронтовый кэш baseline на конфиг-эпоху. Явно заявлено: ни один
+вердикт П1–П5 не меняется. Трек — полный (перф-влияние исключает `small`),
+что верно по критериям §5 PROCESS.md.
+
+## Как проверялось
+
+Читкой, без исполнения. Против каждого пункта решения (§4) и каждого AC (§6)
+проверялся код, который они описывают, чтобы отделить утверждение от факта:
+
+- `custom_components/houseplan/websocket_api.py:1280-1398` (`ws_config_set`,
+  синхронная цепочка валидаторов под `write_lock`, MAX_CONFIG_BYTES) —
+  подтверждает описание проблемы дословно.
+- `custom_components/houseplan/junction_limits.py:150-317`
+  (`collinear_run_length_units`, `check_segment_lengths`,
+  `check_node_distances`, `validate_junction_limits`) — корень П3
+  (индекс строится заново на каждый сегмент) подтверждён; заодно проверена
+  стоимость П4.
+- `src/junction-limits.ts:121-169` и `src/houseplan-card.ts:7418-7473`
+  (`checkSegmentLengths`, `checkNodeDistances`, `_junctionLimitViolations`,
+  `_junctionLimitsIntroduced`) — та же проверка на фронте, плюс состав
+  «полной» функции, на которую ссылается перф-бюджет §5.
+- `demo/benchmark_safe_resize.mjs` (полностью) — существующий бенч, на
+  который опирается AC4.
+- `grep async_add_executor_job` по `custom_components/houseplan/*.py` —
+  подтверждение прецедента, на который ссылается §4.1.
+- `docs/SCOPE.md`, `PROCESS.md` §2.4/§2.5/§4/§5/§7.1 — рамка ревью и
+  обязательные разделы.
+
+Не проверялось (и не требовалось на этом этапе): исполнение кода, реальные
+замеры производительности — числа взяты из S2-аналитики автора и приняты как
+факт (это их роль на этом этапе; воспроизводить их — работа код-ревью).
+
+## Находки
+
+### H1 — Перф-бюджеты §5 недостижимы решением из §4: AC2 и AC6 непроверяемы как написаны
+
+`docs/specs/330-junction-limits-performance.md` §5, AC2, AC6.
+
+Собственные цифры автора (комментарий S2-аналитики и §1 ТЗ) на сетке
+12×12 / 576 атомов:
+
+| Компонент | Значение | Кем адресуется в §4? |
+|---|---|---|
+| Бэкенд, 1× миграция (`commit_wall_segment_model`) | 754 мс | rev-кэш (§4.2) убирает миграцию **только** стороны `previous`; сторона `candidate` мигрируется заново на КАЖДУЮ запись — это неустранимо текущей архитектурой и в §4 не тронуто. |
+| Бэкенд, П4 (`check_node_distances`) | 376 мс | Не адресуется никем. Ни §2 (корни), ни §4 не называют П4 квадратичным дефектом — и по коду (`junction_limits.py:211-245`) это не «баг переиспользуемого индекса», как П3, а架构рно O(n²) все-пары-узлов плюс O(n·m) узел-к-стене — линеаризовать так же, как П3, нельзя без отдельного решения (spatial index), которого в ТЗ нет. |
+| Фронт, П4 (`checkNodeDistances`) | 105 мс | Аналогично не адресуется. |
+
+Уже из одной только суммы «миграция candidate (754 мс) + П4 бэкенд (376 мс)»
+= **1130 мс** — при заявленном тёплом бюджете **≤ 150 мс** (§5) и AC6
+«красный при возврате O(n²)». Это не «близко к порогу» — это расхождение на
+порядок, и оно основано на цифрах, которые сам автор привёл как измеренные.
+То же для фронта: П4 (105 мс) сам по себе почти вдвое превышает бюджет
+«полный `_junctionLimitViolations` кандидата ≤ 60 мс», при том что П4 —
+только один из компонентов внутри `_junctionLimitViolations`
+(`src/houseplan-card.ts:7418-7441`, там же ещё `checkNodes` и цикл
+`checkRoomClearance`/`innerContourForRoom` по комнатам, чья стоимость нигде
+не измерена и не бюджетирована).
+
+**Почему это находка, а не придирка к цифре.** §4.1–4.4 перечисляют РОВНО
+четыре среза изменений (executor-вынос, rev-кэш previous, линейный П3,
+фронт-кэш baseline). Ни один не снижает стоимость (а) обязательной миграции
+`candidate` на каждую запись, (б) П4 в обеих реализациях. Значит бюджеты §5
+либо взяты без пересчёта относительно уже собственноручно измеренных
+компонентов (см. таблицу выше — цифры лежат в том же документе/треде), либо
+подразумевают пятое, неописанное изменение (например, пространственный
+индекс для П4, или отказ от полной миграции candidate). Ни то, ни другое не
+названо явно — а значит **автор выдал число за решённое, не проверив его
+арифметикой, которую сам же привёл строкой выше**. Это ровно то, что этап
+spec-review обязан ловить: не «согласиться», а найти, где ТЗ не проверяемо.
+
+Практическое следствие: реализовав буквально §4.1–4.4 и запустив
+`benchmark_junction_limits.mjs` из §5, разработчик получит красный
+перф-контракт независимо от качества работы — потому что AC2/AC6 требуют
+результата, которого выбранное решение физически не даёт. Это либо
+возвращает задачу в «В разработке» после несостоявшегося код-ревью (трата
+цикла впустую), либо толкает разработчика тихо расширить скоуп без ТЗ
+(находка П4 как quadratic bug, которую §3 явно не резервирует под #331 —
+#331 про точность ключей и 0°-дубль, не про производительность П4).
+
+**Что нужно поправить одним из двух способов:**
+1. пересчитать бюджеты §5 от реальных достижимых значений (учитывая
+   неустранимую миграцию candidate и нетронутую стоимость П4), с явным
+   указанием, откуда цифра взята; или
+2. добавить в §2/§4 пятый срез — оптимизацию П4 (обе реализации) и/или
+   устранение обязательной полной миграции candidate — со своим AC и
+   доказательством, раз она нужна для достижения заявленных 60/150 мс.
+
+Оба варианта — правка внутри этого же issue (Medium по объёму работы,
+но блокирует как High, потому что без неё AC2/AC6 недоказуемы в принципе,
+а не «доказуемы с оговоркой»).
+
+### H2 — AC4 ссылается на бенч, который не вызывает изменяемый код
+
+`docs/specs/330-junction-limits-performance.md` AC4:
+«…плюс существующий `benchmark_safe_resize` держит прежний pointer-бюджет
+на large-house».
+
+Прочитан `demo/benchmark_safe_resize.mjs` целиком: он меряет
+`clampEdgeDrag`/`resolveSafeResize`/`clampSafeResize`/`applySafeResize` из
+`test-build/resize.js` и `checkOptimizeGeometry` из
+`test-build/plan-geometry-preflight.js`. Ни один из этих путей не вызывает
+`_junctionLimitsIntroduced`, `checkSegmentLengths` или `checkNodeDistances` —
+это отдельная реализация ресайза (низкоуровневый safe-resize примитив), не
+тот код, который правит эта задача. Код, который реально меняется
+(`_junctionLimitsIntroduced` в `src/houseplan-card.ts:7452-7473`, вызывается
+из `_rszSpaceCandidateGeometry` на каждый шаг ресайза, `houseplan-card.ts:
+~9041`), в `benchmark_safe_resize.mjs` не участвует вовсе.
+
+Значит регресс именно в том коде, который §330 чинит, этим бенчем **не
+будет пойман** — ни до фикса (он и сейчас не показывает проблему, потому что
+не меряет её), ни после (он не докажет, что фикс сработал). AC4 как
+написан создаёт видимость покрытия там, где его нет: «существующий бенч
+держит бюджет» — верное, но нерелевантное утверждение, поданное как
+доказательство продуктового обещания «ресайз на large-house держит кадровый
+бюджет».
+
+**Что нужно поправить:** либо расширить/создать бенч, который вызывает
+`_junctionLimitsIntroduced`/`checkSegmentLengths` в контексте одного жеста
+ресайза (это и есть предмет AC4 — «в одном жесте baseline считается один
+раз»), либо снять ссылку на `benchmark_safe_resize` из доказательства AC4 и
+оставить только юнит на счётчик вызовов (который в AC4 уже есть и сам по
+себе корректен).
+
+### M1 — Отсутствуют обязательные разделы ТЗ по §7.1: i18n, touch, риски, release-артефакты
+
+`docs/specs/330-junction-limits-performance.md`, целиком.
+
+§7.1 PROCESS.md перечисляет обязательные разделы ТЗ: сценарий · что человек
+увидит до/после · проблема · скоуп/не-скоуп · контракт поведения · UX ·
+модель данных и миграция · i18n · AC с доказательством · план автотестов ·
+риски · откат · release-артефакты. В документе явно закрыты: сценарий (§1),
+проблема (§1), скоуп/не-скоуп (§3), контракт поведения и модель данных
+(§4 + §8 «откат» покрывает «формат данных не меняется»), AC (§6), план
+автотестов (§7), откат (§8).
+
+Полностью отсутствуют как отдельные утверждения:
+- **i18n** — ни слова; для этой задачи корректный ответ тривиален («нет
+  новых строк»), но он должен быть написан, а не подразумеваться —
+  DoR (§2.5) требует «ключи en+ru перечислены» именно как явный пункт;
+- **touch** — не сказано, что жест ресайза (в т.ч. на touch, best-effort по
+  `TOUCH-SUPPORT.md`) использует тот же код и получает тот же выигрыш; для
+  DoR это обязательный явный пункт, а не факт по умолчанию;
+- **риски** — нет отдельного раздела (в §2 «Корни» перечислены причины
+  медленности, но не риски решения: например, что кэш `_junction_baseline`
+  на runtime-объекте — единственный слот, и конкурентная запись в другое
+  пространство того же конфига инвалидирует его чаще, чем ожидается —
+  это не баг, но риск, который стоит явно взвесить в ТЗ, а не оставлять
+  ревьюеру код-ревью находить постфактум);
+- **release-артефакты** — не названы changelog RU+EN, документация,
+  golden/скриншоты (для чисто перф-задачи, вероятно, не нужны — но
+  правило §7.1 требует явного «нет», а не отсутствия раздела).
+
+Ничего из этого не блокирует техническую состоятельность решения — все
+четыре пункта, скорее всего, разрешаются одной строкой каждый («i18n: нет»,
+«touch: тот же код, ускорение применяется одинаково», «риск: единственный
+слот кэша инвалидируется по rev, race исключён write_lock», «release:
+changelog не нужен — не user-visible поведенчески, только perf»). Но без
+явной записи это дыра в DoR §2.5, которая иначе будет обнаружена только на
+входе в очередь «Готово к разработке» и вернёт задачу обратно.
+
+## Что проверено и корректно
+
+- Корень П3 (`checkSegmentLengths`/`check_segment_lengths` строит byNode-
+  индекс заново на каждый сегмент → O(n²)) подтверждён чтением обеих
+  реализаций — не догадка, реальный дефект, фикс (передать индекс третьим
+  опциональным параметром) технически корректен и не меняет публичную
+  сигнатуру для прямых вызовов.
+- Синхронность цепочки валидаторов `ws_config_set` под `write_lock` в event
+  loop подтверждена построчно (`websocket_api.py:1303-1334`); прецедент
+  `async_add_executor_job` в том же файле и соседних модулях (`__init__.py`,
+  `http_api.py`) подтверждён — решение §4.1 не изобретает новый паттерн.
+- Граница §3 («вердикты П1–П5 не меняются») сформулирована однозначно и
+  проверяема существующим паритет-тестом и тремя мутантами #329 — это не
+  голословное заявление, у него есть механизм проверки.
+- Откат (§8) корректен: кэши только в памяти процесса, формат данных не
+  меняется — ревертится действительно чисто.
+- AC1, AC3, AC5 однозначны и имеют названный способ доказательства
+  (backend-тест инструментированного времени; тест счётчика вызовов
+  `commit_wall_segment_model` через monkeypatch; существующие юниты/паритет-
+  тест/смоки/мутанты). Догадок, выданных за факт, в этих трёх AC не найдено.
+- Продуктовая рамка (§7.1 «сценарий» + «что человек увидит») по существу
+  присутствует и корректна: персона home admin на десктопе не названа явно,
+  но однозначно выводится из контекста (сохранение конфига и ресайз — только
+  редакторские действия) — это не отдельная находка, различие между
+  «выводится однозначно» и «отсутствует» здесь в пользу автора.
+- Открытых продуктовых вопросов, вынесенных владельцу как технические
+  (§7.1, запрещённый класс), в ТЗ и комментариях не найдено — оба
+  комментария автора чисто аналитические/технические и решены им самим,
+  что соответствует правилу «владельцу — только продуктовые вопросы».
+
+## Чего не проверял
+
+- Не запускал `npm test`/`pytest`/бенчи — на этапе spec-review это не
+  предусмотрено (кода изменения ещё нет, есть только текст ТЗ и уже
+  существующий, не тронутый этой веткой код).
+- Не проверял актуальность файла `AUDIT-2026-08-28.md`, упомянутого в теле
+  issue — файл не найден в дереве репозитория (`find` не дал результата);
+  это, вероятно, внешний документ или ещё не закоммитчен. Не блокирует
+  ревью ТЗ: цифры из него продублированы и подтверждены в S2-комментарии,
+  который я проверил по коду напрямую.
+- Не оценивал точную конструкцию будущего `_junction_baseline` кэша
+  (структура `counts`, инвалидация на несколько пространств одного
+  конфига) глубже, чем нужно для проверки конкурентной безопасности под
+  `write_lock` — это техническая деталь реализации, которую §7.1 явно
+  оставляет на усмотрение авторов кода, а не ТЗ.
+
+## Вывод
+
+Решение технически обосновано и корни (кроме недостающего П4) описаны
+доказательно, не как догадка. Но два High-находки делают документ
+непроверяемым в буквальном смысле процесса: AC2/AC6 (H1) и AC4 (H2) не могут
+быть подтверждены тем набором изменений и тем бенчем, которые ТЗ само же
+описывает. Это возврат автору на правку числа/скоупа решения, не отклонение
+идеи задачи.