From 8176f1190f1c788de5e0f594e0f91e0c558d757c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:09:27 +0000 Subject: [PATCH] docs: review document for #663 Issue: #663 User-Visible: no --- docs/reviews/CODE-REVIEW-663-r2.md | 221 +++++++++++++++++++++++++++++ 1 file changed, 221 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-663-r2.md diff --git a/docs/reviews/CODE-REVIEW-663-r2.md b/docs/reviews/CODE-REVIEW-663-r2.md new file mode 100644 index 00000000..8dba9237 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-663-r2.md @@ -0,0 +1,221 @@ +# CODE-REVIEW-663-r2 + +Материал раунда: `d64338224da8606bf2b346906b8f6f2a9bd0a15e` (ветка `issue/663-stairs`, +рабочая копия уже на нём). Предыдущий материал (r1): `59a60d526db73974a81e57b739458f59c18cfe7f`. +Дельта: `git diff 59a60d52..d6433822` — 2 коммита (`3f8aae83` — публикация +`CODE-REVIEW-663-r1.md`, класс C, не код; `d6433822` — сам фикс), 5 +содержательных файлов: `src/stairs.ts` (+7/-3), `test/stairs.test.mjs` (+25), +`scripts/mutation-registry.mjs` (+39), `docs/CHANGELOG.md` (+2), +`docs/CHANGELOG.ru.md` (+6/-2). + +Заход: r2 (код-ревью), блокирующих циклов израсходовано 1/4. + +## Скоуп + +Дельта закрывает ровно три Medium-находки `CODE-REVIEW-663-r1.md`: направление +разметки ступеней для `direction: 'backward'` (AC3), mutation-witness для +защитного AC7 (битая цель) и mutation-witness для защитного AC11 +(backward-compat старого конфига + remap ссылки при полном импорте). Общий +скоуп задачи (#663) — прежний, см. `CODE-REVIEW-663-r1.md`, раздел «Скоуп»; +здесь не повторяется. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium-1 — разметка ступеней неверна для `direction:'backward'` | `src/stairs.ts:151-157`: якорь отсчёта и условие останова теперь зависят от `forward`; полные 30-см интервалы для `backward` считаются от `+length/2` (физический низ при развороте стрелки), остаток — у `-length/2` (физический верх) | `test/stairs.test.mjs:78-85` (новый блок в существующем тесте) — `backward.treads[0].a[0] === 595` (мимо форвардного `405`, зеркально), `backward.treads.at(-1).a[0] === 395` (мимо `605`); лично реверсировал `src/stairs.ts` к r1-версии и убедился, что этот же тест краснеет (`25 !== -25`) — тест не тавтологичен | +| Medium-2 — защитный AC7 без mutation witness | Новый `stairs-broken-targets-become-active` (`scripts/mutation-registry.mjs`), патчит `src/stairs-editor-model.ts:101-103` (`stairTargetState`) на константу `'active'`, guard `node demo/smoke_stairs.mjs` | Лично прочитан diff мутанта — find-блок совпадает с реальным кодом строка-в-строку; в CI Validate `36276064818` (job «Мутанты по диффу (6/6)») лог: `ok stairs-broken-targets-become-active: заявленный тест покраснел на мутанте` — прочитан лично | +| Medium-3 — защитный AC11 без mutation witness | Два новых мутанта: `stairs-legacy-config-materializes-empty-collection` (`src/plan-optimizer.ts`, guard — юнит `test/stairs.test.mjs` по имени) и `stairs-import-skips-target-space-remap` (`custom_components/houseplan/import_export.py`, guard — `backend-test-guard.mjs` на существующем backend-тесте) | Job «Мутанты по диффу (4/6)»: `ok stairs-legacy-config-materializes-empty-collection: …`; job «Мутанты по диффу (3/6)»: `ok stairs-import-skips-target-space-remap: …` — оба лога прочитаны лично. Юнит-тест `#663 legacy-no-stairs-config …` (`test/stairs.test.mjs:196-208`) лично пересобран и прогнан (`node --test`, 1/1 OK) | + +Все три находки r1 закрыты доказательно (не только заявлением автора): для +Medium-1 я лично воспроизвёл регресс на пред-фикс версии кода, для Medium-2/3 +прочитал реальные логи CI-джобов текущего SHA, где заявленный тест-свидетель +покраснел на мутанте (не поверил хендоффу на слово). + +## Унаследовано из r1 без повторной проверки + +Дельта не касается ни одного из следующих файлов/областей — приняты по +`CODE-REVIEW-663-r1.md` (материал `59a60d52`, вердикт жёлтый, High 0) без +повторного разбора: continuous/мебельный transform-контракт (кроме +затронутого магнитного свидетеля, см. находку ниже), backend-схема +(`validation.py`, лимит 250), repair ссылки при удалении пространства, +i18n (4 языка), 2D/2.5D через общий floor-projection слой, батчинг вычитания +площади по кадрам (`ea1f19c3`), navigation/gesture guard'ы, документация +(`STAIRS.md`, `CANVAS.md`, `UX-MODES.md` и др.). + +## Как проверялось + +Ссылка на подтверждённый зелёный Validate точного SHA материала: +https://github.com/Matysh/houseplan-card/actions/runs/36276064818 — +job-список проверен построчно (`gh run view … --json jobs`): зелёные — +предпролёт, классификация, **frontend (types+unit+build)**, **мутанты по +диффу (6/6 шардов)**, «доказательство выполненных проверок»; **skipped** — +hacs, hassfest, geometry_parity, backend (pytest), смоки в браузере, golden, +performance_smoke (ожидаемо: heavy-гейты не запускаются на обычном пуше). + +| Гейт | Прогнано | Результат | +|---|---|---| +| `npx tsc --noEmit` / `npm run build` | зачтено по зелёному Validate (frontend job) | — | +| `npm test` | зачтено по зелёному Validate (frontend job) | лог job `108498979810`: `# tests 3143`, `# pass 3142`, `# fail 0` (1 skipped) — включает все 11 тестов `test/stairs.test.mjs`, включая новые | +| Мутанты по диффу (registry, 6/6 шардов) | зачтено по зелёному Validate + лично прочитаны логи 3 новых ID | `stairs-broken-targets-become-active`, `stairs-legacy-config-materializes-empty-collection`, `stairs-import-skips-target-space-remap` — все три «заявленный тест покраснел на мутанте» | +| `node scripts/check-docs.mjs --screenshots=warn` (режим CI на обычном пуше, сверено с `validate.yml:76-90`) | прогнано лично | `Documentation checks passed (7 files, 12 external links)`, 1 ожидаемый `WARN` про устаревший отпечаток скриншотов (routine-следствие правки `src/**`, не блокирует до кандидата беты, #479) | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнано лично | 101 прямое совпадение, 3 зарегистрированные связи (upload-guard, к лестницам не относится) — как и в r1, широкий список из-за общих символов диффа в 18 файлах `src/**` | +| `node demo/smoke_stairs.mjs` (AC1/AC2/AC3/AC5/AC6/AC7/AC8/AC10) | прогнано лично, свежий `npm run bundle:sync`, **~13 последовательных запусков** | 10/13 — все 29 проверок `OK`; **3/13 — `wallMagnetUsesPhysicalFace: false`** (см. находку Medium-2 ниже) | +| `npm run golden:verify` (полная матрица, 179 сцен) | прогнано лично на свежем бандле этого SHA | **175 passed, 4 different**: `stairs-flat-normal-light`, `stairs-flat-hover-dark`, `stairs-flat-selected-light`, `stairs-isometric-dark` (см. находку Medium-1 ниже) | +| `python -m pytest tests_backend -q` | не прогнано | в этой ревью-среде не установлен `pytest`/`homeassistant` (как и в r1); дельта не трогает backend-продакшен-код, только добавляет mutation-registry запись поверх уже существующего и уже проверенного в r1 backend-теста | +| `npm run invariants -- --config …` | не прогнано | дельта не меняет модель/геометрию хранения, только рендер-производную математику (позиции линий ступеней) — не тот класс риска, под который заточен этот гейт | + +После проверки бандл возвращён к закоммиченному состоянию (`npm run +bundle:clean`), рабочая копия чистая (`git status` — пусто). + +## Находки + +### Medium-1 (в скоупе задачи) — AC12: golden-эталон устарел для 4 сцен с лестницами, фикс AC3 не сопровождён обновлением baseline + +`demo/golden/matrix.mjs:206-212` заводит `stairLayerFixture` с прямой малой +лестницей `direction: 'backward'` (используется в 4 сценах +`stairs-flat-normal-light`/`stairs-flat-hover-dark`/ +`stairs-flat-selected-light`/`stairs-isometric-dark`, помеченных в коде как +`#663 AC3/AC10/AC12`). Фикс Medium-1 из r1 (`src/stairs.ts`, эта дельта) +корректно меняет позицию ступеней именно для `backward` — то есть *обязан* +изменить пиксели этих 4 сцен. Committed baseline (`demo/golden/baselines/**`) +не обновлён: `git diff 59a60d52..d6433822 -- demo/golden/` — пусто. + +**Воспроизведение:** `npm run bundle:sync && npm run golden:verify` на этом +точном SHA → `175 passed, 4 different` (список выше). Открыл +`artifacts/golden/diff/stairs-flat-normal-light.png` — расхождение (пурпурные +линии) сосредоточено ровно на малой развёрнутой на 45° `backward`-лестнице +справа сверху; все остальные объекты сцены (прямая forward, обе спиральные, +стены, проёмы) идентичны принятому эталону пиксель-в-пиксель. Это подтверждает +точную причину: расхождение — прямое и единственное следствие фикса AC3, а не +шум окружения (иначе разошлись бы и другие 175 сцен). + +Not High: сам фикс корректен (расхождение — ожидаемое улучшение, не +регрессия), продукт ничего не ломает. Но AC12 («light/dark, оба типа, оба +направления… не имеют clipping, мерцания и нечитаемой стрелки») сейчас +**не доказан** на этом SHA: заявленный в r1 «179 passed, 0 расхождений» +(тогда — на пред-фикс материале, где не было расхождения) для текущего +материала неверен, и новый baseline никем не принят. Прецедент в этой же +ветке показывает штатный путь закрытия — коммиты `93876d75`/`ac6f4bfe` +(`chore(golden): закрепить …` + `docs: обновить отпечаток …`, оба с +`Release:`/`Baseline-Reviewed-Local:` трейлерами) делали ровно это после +похожей правки рендера (`ea1f19c3`). Для этой находки такого коммита нет. +Чинится в этой же задаче: `npm run golden:capture`/`npm run docs:capture` → +визуальный осмотр 4 diff → `npm run golden:accept -- --reviewed` с +`Release:`/`Baseline-Reviewed*` трейлерами. + +### Medium-2 (в скоупе задачи) — AC2 smoke-свидетель `wallMagnetUsesPhysicalFace` статистически ненадёжен из-за бага в собственной формуле сравнения углов + +`demo/smoke_stairs.mjs:87`: + +```js +&& closeTo(((straight.angle % 180) + 180) % 180, 0); +``` + +Продукт после wall-magnet snap иногда отдаёт `angle` как чистый `0`, а иногда +как `-2.2737367544323206e-13` (нормальный floating-point шум тригонометрии +snap — сам по себе не баг: обе величины физически означают «строго +горизонтально»). Формула нормализации `((x % 180) + 180) % 180` для +отрицательного `x`, близкого к нулю **снизу**, корректно заворачивает его к +`≈180`, а не к `≈0` — и тест сравнивает результат только с `0` без учёта +цикличности. `closeTo(179.999999999999773, 0, 1e-5)` — `false`, хотя физически +угол равен нулю с точностью до `2×10⁻¹³`. Правильный паттерн для «расстояние +до 0 по модулю 180» уже есть в этом же кодовом дереве и делает именно так: +`src/junction-limits.ts:200-204` (`axisDegrees`/`collinear`) использует +`Math.min(delta, 180 - delta)`, а не сырое сравнение с `0`. Баг — только в +тестовой формуле, не в продукте. + +**Воспроизведение:** 13 последовательных запусков `node demo/smoke_stairs.mjs` +на этом SHA (свежий `npm run bundle:sync`) → 3 красных (`false`), 10 зелёных +(`true`). Изолировал причину инструментированной копией скрипта: на красных +прогонах `straight.angle === -2.2737367544323206e-13` (на зелёных — ровно +`0`); подставил оба значения в `node -e` — воспроизводится детерминированно +(`norm = 179.99999999999977`, `closeTo0? false`). Проверил историю: строка +существует с самого первого коммита фичи (`d03a68b8`), не тронута +тест-стабилизацией `59a60d52` из материала r1 — то есть это не новый регресс +дельты r1→r2, а ранее не пойманная (видимо, ловилась в r1 везением: 5/5 при +эмпирической частоте отказа ~23% — `0.77⁵≈0.27`, шанс не так уж мал) +нестабильность собственного AC2-свидетеля, обнаруженная сейчас при +обязательном прогоне названного в AC смока. + +Not High: продуктовый код корректен (подтверждено кодом-аналогом в этом же +дереве и математикой), поведение magnet не меняется. Но это защитный +AC2-свидетель со ссылкой в самом ТЗ (`unit + smoke`), и его ненадёжность +~1 раз из 4 создаёт риск ложных возвратов в `S6-in-progress` на будущих +переприменениях этого же смока (mutation-gate, nightly, merge-candidate +Validate) — фиксируется в этой же задаче: заменить сравнение на +`Math.min(norm, 180 - norm)` либо на прямую проверку `Math.abs(angle) < eps +|| Math.abs(Math.abs(angle) - 180) < eps`. + +Итого: **High: 0, Medium: 2 (обе в скоупе задачи)**. Без High — жёлтый +вердикт; находки чинятся в этой же задаче, повторный раунд по дельте, +отдельный issue не заводится (#202). + +## Что проверено и корректно + +- **Все три Medium-находки r1 закрыты доказательно** — см. таблицу + «Закрытие раунда r1» выше; проверено исполнением (не только чтением кода): + личный реверт+перезапуск теста для Medium-1, чтение фактических CI-логов + красного-на-мутанте для Medium-2/3. +- **Unit-контракт** — 3143 юнит-теста зелёные (3142 pass + 1 skip), включая + все 11 тестов `test/stairs.test.mjs` (было 9 до дельты). +- **Мутанты по диффу** — все 6 шардов Validate зелёные на этом точном SHA; + прочитаны логи для всех 3 новых записей реестра — каждая корректно нацелена + на реальный код (find-блок совпадает построчно) и подтверждённо краснеет + заявленный тест. +- **Docs** — `check-docs.mjs` в режиме, реально используемом Validate на + обычном пуше (`--screenshots=warn`, сверено с `validate.yml:76-90`), зелёный; + единственный warn (устаревший отпечаток скриншотов) — ожидаемое следствие + правки `src/**`, не блокирует до кандидата беты. +- **Golden — 175 из 179 сцен идентичны эталону**; 4 расхождения локализованы + и объяснены (Medium-1 выше), не свидетельствуют о постороннем регрессе. +- Рассмотрен и **отклонён** как находка кандидат: `angle: 2.2737367544323206e-13` + (положительный, а не отрицательный эпсилон) на одном из зелёных прогонов — + проверил формулу отдельно, для положительного знака `((x%180)+180)%180` + корректно даёт `≈0`, баг асимметричен и проявляется только для + отрицательного эпсилон — согласуется с наблюдаемой частотой отказа (не + каждый второй, а по факту сторона эпсилон зависит от суб-пиксельного джиттера + клика). + +## Чего не проверял + +- **`python -m pytest tests_backend`** — не прогнан (нет `pytest`/ + `homeassistant` в этой среде, как и в r1); дельта не меняет backend-код, + только добавляет mutation-registry запись поверх уже принятого в r1 + backend-теста. Риск закрыт чтением: find/replace мутанта + `stairs-import-skips-target-space-remap` совпадает с реальным кодом + `import_export.py:1144-1151` построчно, и сам мутант подтверждён красным в + логе CI (см. таблицу выше) — то есть выполнен, просто не мной лично на этой + машине. +- **`npm run invariants`** — не прогнан, как и в r1 (нет готового экспорта + конфигурации с лестницами под рукой); дельта не меняет модель хранения. +- **Полная browser-smoke матрица** (278 смоков за пределами 101 прямого + совпадения) — не гонял; `smoke_stairs` (профильный) прогнан 13 раз лично + (см. находку Medium-2), остальные направленные смоки r1 + (`smoke_room_resize`, `smoke_summary_first_paint`) дельтой не задеты и не + перепрогонялись. +- **`performance_smoke`/Full Performance** — heavy-гейт skipped на материале, + дельта не трогает perf-путь (`ea1f19c3` вне дельты r1→r2), не перепрогонял. +- **HACS/hassfest/geometry_parity** — skipped на материале, дельта не трогает + манифесты/geometry-модель, не прогонял отдельно. +- **Golden-эталоны других 175 сцен** — свежий локальный прогон подтвердил их + идентичность принятому baseline, но саму пересъёмку/приёмку новых 4 не + выполнял (не моя роль — это работа автора с `--reviewed`). + +--- + + + +--- + + + +## Материал раунда + +- Ветка: `issue/663-stairs`, коммит `d64338224da8` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `9d9f9c3524f9fcf632e09f3e177754e7579a2be6` + ``` + git log --all --format='%H %T' | grep 9d9f9c3524f9 + ``` +- Тело issue: `f3f0ee8408eb76ccec4f3ec50a9048ff1e29b2967259f0d4c4d333d8c68157e4` +- Вердикт конвейера: `yellow` · High 0