Files
houseplan-card/docs/reviews/CODE-REVIEW-663-r2.md
T
2026-09-26 23:09:27 +00:00

22 KiB
Raw Blame History

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:

&& 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