From 802604e0c344752e9dc92ecb19c67a8baca5db7d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:01:35 +0000 Subject: [PATCH] docs: review document for #330 Issue: #330 User-Visible: no --- docs/reviews/SPEC-REVIEW-330-r1.md | 228 +++++++++++++++++++++++++++++ 1 file changed, 228 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-330-r1.md diff --git a/docs/reviews/SPEC-REVIEW-330-r1.md b/docs/reviews/SPEC-REVIEW-330-r1.md new file mode 100644 index 00000000..d38ead5a --- /dev/null +++ b/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) не могут +быть подтверждены тем набором изменений и тем бенчем, которые ТЗ само же +описывает. Это возврат автору на правку числа/скоупа решения, не отклонение +идеи задачи.