diff --git a/docs/reviews/CODE-REVIEW-198-r2.md b/docs/reviews/CODE-REVIEW-198-r2.md new file mode 100644 index 00000000..4e518565 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-198-r2.md @@ -0,0 +1,248 @@ +# Код-ревью issue #198 — очистка изолированного микро-интервала толщины через Optimize (цикл r2) + +- Цикл: r2/4 +- Вердикт: **жёлтый** · High: 0 · Medium: 1 (в скоупе задачи) +- Диапазон: `git log --oneline origin/dev..HEAD` = `9d6cd8b` (ТЗ), `8c9241b` + (ревью ТЗ r1), `0df8051` (реализация), `f4ab0b3` (ревью-документ r1), + `1c9575a` (follow-up тесты на замечания r1), `0625b6f` (обновление + provenance скриншотов после изменения `src/`) +- Диапазон диффа: `git diff origin/dev...HEAD` +- ТЗ: [`docs/specs/198-optimize-micro-interval.md`](../specs/198-optimize-micro-interval.md) + (ревью ТЗ зелёное, r1) +- Предыдущий цикл: [`docs/reviews/CODE-REVIEW-198-r1.md`](CODE-REVIEW-198-r1.md) + (жёлтый · Medium-1 AC5, Medium-2 AC8 — обе «доказательство обещанным + способом отсутствует», не дефект поведения) + +## Скоуп + +Тот же, что в r1: явное действие «Общие настройки → Оптимизировать планы» +получает строго ограниченный lossy-шаг — схлопывание одиночного +изолированного интервала толщины стены короче половины шага сетки между +двумя одинаковыми коллинеарными соседями, при отсутствии room-vertex/ +opening-узла на его концах. Runtime и редактор остаются lossless. + +Этот цикл — не новая функциональность, а закрытие двух Medium-находок r1: +- **AC5**: добавлен прямой unit `collapseIsolatedWallThicknessIslands()` на + реверс endpoints, перестановку `walls`, перестановку/реверс winding + `rooms`, неизменность входов (`test/plan-optimizer.test.mjs`). +- **AC8**: добавлен прямой regression unit для `normalizeWallIntervals()` и + `degradeWalls()` на изолированном `22→15→22`-профиле вне Optimize + (`test/wall-thickness.test.mjs`, ранее не тронут диапазоном). + +Дополнительно найден и учтён коммит `0625b6f` — он появился на ветке уже +после старта этого цикла ревью (см. «Как проверялось»): обновляет +`docs/images/screenshots.json` + один PNG (`06-device-editor.png`, +отличие на 1 байт) вслед за изменением `sourceFingerprint` `src/`, класс C, +`User-Visible: no`, не относится к продуктовому поведению #198. + +Продуктовый код (`src/plan-optimizer.ts`) в этом цикле не менялся — +`git diff 0df8051..HEAD -- src/` пуст. Проверка была нужна только для +двух добавленных тестовых файлов и одного docs-коммита. + +## Как проверялось + +Материал — актуальный tip ветки. Важно: сессия ревью изначально была +развёрнута на коммите `1c9575a`, но `git fetch` показал, что ветка +`issue/198-optimize-micro-interval` к моменту ревью уже продвинулась на +`0625b6f` (owner исправил red `docs`-job). Переключился на +`origin/issue/198-optimize-micro-interval` перед тем, как выносить +вердикт, — иначе ревью судило бы устаревший срез. + +| Гейт | Результат | Команда | +|---|---|---| +| typecheck | green | `npx tsc --noEmit` | +| unit | green, 917/917 (было 915/915 в r1, +2 новых теста) | `npm test` | +| build + 3 копии бандла | green, побайтово идентичны | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js && cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | +| targeted smoke (AC7) | green, все 10 подпроверок | `node demo/smoke_optimize_micro_interval.mjs` | +| mutation-gate, полный self-check | green, 36/36 | `node scripts/mutation-gate.mjs --check` | +| mutation-gate, целевой мутант (AC9) | мутант пойман 1/1 | `node scripts/mutation-gate.mjs --id=optimizer-micro-interval-cleanup-disabled` | +| docs-gate (затронут `0625b6f`) | green | `node scripts/check-docs.mjs --external` | +| whitespace | чисто | `git diff --check origin/dev...HEAD` | +| **адверсариальная проверка новых AC5/AC8-тестов мутацией кода** (см. находку ниже) | AC8-тест ловит регрессию; AC5-тест — нет для одного из сценариев | см. «Находки» | + +**Не прогонялось и почему (без изменений относительно r1 — диапазон с r1 +по продуктовому коду и UI-поверхности не менялся):** +- `npm run golden:verify` и смежные `smoke_grid_snap`/`smoke_align_guides` — + этот цикл не трогает `src/plan-optimizer.ts`, рендер, геометрию, + стили или UI-пайплайн Optimize; изменились только `test/*.mjs` и + provenance двух docs-артефактов. Повторный прогон той же UI-поверхности + без изменений в ней добавил бы время без нового сигнала — уже + перепрогнан и подтверждён зелёным в r1 против того же кода. +- `python -m pytest tests_backend -q` — ни один файл + `custom_components/**/*.py` не входит в диапазон ни в r1, ни в r2. +- performance-профили — не названы в AC; изменения этого цикла — только + тесты и docs-provenance, нет тронутых perf-путей. +- Полный набор из 127 browser-смоков — диапазон r2 не касается ни одной + новой UI-поверхности; единственный релевантный (`smoke_optimize_micro_interval`) + прогнан. + +### Проверка дисциплины «тест должен уметь падать» для двух новых юнитов + +Не ограничился чтением — реально мутировал код и убедился в результате, +затем вернул рабочее дерево в чистое состояние (`git diff` пуст, `npm +test` 917/917 после отката). + +**AC8-тест (`test/wall-thickness.test.mjs`, «lossless wall helpers preserve +an isolated sub-half-step thickness island outside Optimize») — +подтверждённо способен падать.** Внёс в `degradeWalls()` (`src/wall-thickness.ts`) +однострочную мутацию входа (`walls[0].cm = 999`) сразу после guard `if +(!walls?.length) return [];`. Прогон `npm test` дал 5 красных тестов, +включая именно новый (`not ok 843 - lossless wall helpers preserve an +isolated sub-half-step thickness island outside Optimize`), плюс два +существующих соседних `degradeWalls`-теста и оба уже покраснели бы сами +по себе — то есть новый тест не единственная защита, но он сам по себе +дискриминативен для входной мутации. Откатил правку (`git checkout -- +src/wall-thickness.ts`), `npm test` снова 917/917. + +**AC5-тест (`test/plan-optimizer.test.mjs`, «micro-interval cleanup is +endpoint, input-order and room-order invariant without mutation») — не +ловит именно тот сценарий регрессии, который Medium-1 из r1 назвал +целевым.** Подробности — находка ниже. + +## Находки + +### Medium-1 (в скоупе) — новый AC5-тест не хермитичен к сценарию входной мутации, который он должен доказывать + +**Файл:** `test/plan-optimizer.test.mjs`, тест «micro-interval cleanup is +endpoint, input-order and room-order invariant without mutation» (строки +127–159 в HEAD) + +**Суть:** тест сперва вызывает `baseline = collapseIsolatedWallThicknessIslands( +[fixture.rooms[0], detached], fixture.walls, ...)` **на непроклонированном** +`fixture.walls`/`fixture.rooms`, а затем строит `reversedMiddle` и итоговый +`walls`, **читая** `fixture.walls[1]` уже **после** этого первого вызова +(`{...fixture.walls[1], a: [...fixture.walls[1].b], b: [...fixture.walls[1].a]}`). +Если бы `collapseIsolatedWallThicknessIslands` регрессировала ровно так, +как предупреждал Medium-1 в r1 — начала мутировать входной массив `walls` +на месте (`wall.cm = target` вместо иммутабельного `{...wall, cm: +target}` внутри `out.map(...)` на строках 178–182 `src/plan-optimizer.ts`) — +то `baseline`-вызов сам испортил бы `fixture.walls[1].cm` (15 → 22) ещё +**до** того, как из него строится `reversedMiddle`/`walls` для проверки +инвариантности. К моменту, когда тест снимает `beforeWalls = +structuredClone(walls)`, входной центральный интервал уже был бы виден +как `cm: 22`, кандидат схлопывания отфильтровался бы условием `centreCm +=== leftCm` (строка 138 `src/plan-optimizer.ts` — уже «канонично», сливать +нечего), второй вызов не тронул бы объект вообще, и +`assert.deepEqual(walls, beforeWalls)` прошла бы тривиально, потому что +мутировать было уже нечего. + +**Проверено исполнением, не только чтением.** Ввёл в +`src/plan-optimizer.ts` минимальную адверсариальную мутацию — заменил +`out = out.map((wall) => {... return wall.cm === target ? wall : {...wall, +cm: target}; })` на цикл `for (const wall of out) { ...; wall.cm = +target; }` (мутация того же самого объекта, на который ссылаются и +`out`, и исходный `walls`, поскольку `out = walls.slice()` — мелкое +копирование массива, не элементов). Прогнал `npm test`: **все 917 тестов +остались зелёными**, включая AC5-тест. Отдельным изолированным +node-скриптом против пересобранного `test-build/plan-optimizer.js` +подтвердил механизм: `fixture.walls[1].cm` меняется с `15` на `22` сразу +после `baseline`-вызова (для немутированного, реального кода — +остаётся `15`, для мутированного — становится `22`). Значит именно +описанный сценарий воспроизводится, а не является гипотетическим. + +**Сценарий отказа, который прошёл бы незамеченным:** будущий рефакторинг +`collapseIsolatedWallThicknessIslands`, меняющий иммутабельный `.map` на +мутацию на месте (естественная логическая ошибка при, например, +попытке «оптимизировать» цикл), не будет пойман этим тестом — именно +тем автотестом, который ТЗ и r1-ревью называют доказательством AC5. +`test/wall-thickness.test.mjs`-тест того же follow-up-коммита для AC8 +устроен иначе (сравнивает итоговую переменную `walls` с её собственным +`before`-снимком, снятым до **любых** вызовов) и корректно ловит тот же +класс регрессии — см. проверку выше. Это подтверждает, что дефект +специфичен для одного теста, а не общая недостижимая планка. + +**Почему Medium, а не Low:** это не стилистика — тест был написан +специально в ответ на Medium-1 из r1 как «Reversal/permutation/ +immutability unit» для строгого продуктового инварианта неизменности +персистентных данных (`docs/WALL-THICKNESS.md`), и для сценария, прямо +названного в тексте самого Medium-1 r1 («если бы будущая правка начала +мутировать переданный `walls`-массив на месте»). Регрессия именно этого +класса останется незамеченной автотестами задачи. + +**Что нужно:** в начале теста один раз снять `structuredClone` со всего, +что дальше используется как источник и для `baseline`, и для проверяемого +вызова (например, клонировать `fixture` целиком перед первым вызовом, +либо строить `reversedMiddle`/`walls`/`rooms` из независимой копии +fixture, а не из объекта, уже прошедшего через `baseline`-вызов). После +правки повторить именно ту адверсариальную мутацию, которой я проверял +эту находку (иммутабельный `.map` → мутирующий `for`), и убедиться, что +тест в таком варианте краснеет. + +### Medium-2 — нет (закрыта) + +Assertion из r1 подтверждена исполнением как реально дискриминативная +(см. «Проверка дисциплины „тест должен уметь падать“» выше) — новых +находок по AC8 нет. + +### Low — нет + +Находок уровня Low не выявлено. + +## Что проверено и корректно + +- **AC8 закрыт полностью и доказан заявленным способом.** Новый unit в + `test/wall-thickness.test.mjs` вызывает `normalizeWallIntervals()` и + `degradeWalls()` напрямую (без `collapseIsolatedWallThicknessIslands`, + то есть вне Optimize) на изолированном `22→15→22`-профиле короче + половины шага и подтверждает: обе lossless-функции не трогают его, вход + не мутируется. Дискриминативность подтверждена адверсариальной + мутацией (см. выше) — тест реально краснеет на регрессии. +- **Продуктовый код не менялся с r1.** `git diff 0df8051..HEAD -- src/` + пуст — контракт §6 ТЗ, non-cascading snapshot, разрешение + неоднозначности через `targets.size === 1` и защиту topology-узлов из + r1 не требовалось перепроверять заново по существу; перепроверены + только заново пересобранные гейты (typecheck/test/build/mutation-gate/ + targeted smoke), все зелёные на актуальном коде. +- **Docs-provenance коммит `0625b6f` корректен и не расширяет скоуп.** + `sourceFingerprint` в `docs/images/screenshots.json` актуализирован под + изменившийся `src/` (тестовые правки этого же issue меняют fingerprint, + т.к. он покрывает весь `src/` + build inputs), `node scripts/check-docs.mjs + --external` зелёный, класс C, `User-Visible: no`, никакой продуктовой + логики не касается. 1-байтовое отличие PNG `06-device-editor.png` + проверено на невоспроизводимость по байтам: два последовательных + локальных прогона `npm run build && node demo/docs/capture.mjs` + дают разные SHA-256 даже для одного и того же коммита — это известная + нестрогая детерминированность самого capture-пайплайна (не связана с + #198), а `check-docs.mjs` проверяет только внутреннюю согласованность + манифеста и файла, а не побитовую воспроизводимость между запусками, + так что это не дефект. +- **Idempotenность и mutation guard.** `node scripts/mutation-gate.mjs + --check` (36/36) и целевой `--id=optimizer-micro-interval-cleanup-disabled` + (1/1) зелёные на актуальном HEAD. +- **Трейлеры.** Все пять новых/изменённых коммитов диапазона несут + `Issue: #198`; `User-Visible: no` на всех — корректно, ни один из них + (тесты, ревью-документы, docs-provenance) не меняет видимое поведение; + changelog-и не тронуты этим циклом и не должны были быть (правки + r1 уже внесли оба changelog в `0df8051`, `User-Visible: yes`, + подтверждено в r1 и повторно проверено — диапазон не менялся). +- **Три копии бандла синхронны.** `cmp` дал совпадение бит-в-бит для + `dist/`, `custom_components/houseplan/frontend/` и `demo/srv/assets/` + после `npm run build` на актуальном HEAD. + +## Чего не проверял + +- Полный HA backend harness (`tests_backend` под настоящим Home Assistant) — + диапазон не содержит правок в `custom_components/**/*.py`. +- `npm run golden:verify` и полный browser-smoke набор (127 файлов) — + диапазон r2 не трогает рендер/геометрию/UI-пайплайн; обоснование то же, + что в r1, и продуктовый код не менялся. +- Производительность/бенчмарки — не названы в AC, изменения цикла — только + тесты и docs-provenance. +- Повторный ручной прогон adversarial-мутаций на самом `exactMatches` + (реверс endpoints без общей мутации входа) — этот сценарий был + исполнен в r1 (см. `CODE-REVIEW-198-r1.md`, Medium-1) и один раз + дополнительно в этом цикле при диагностике находки выше; отдельно + третий раз не повторял, так как вывод не менялся между попытками. + +## Вывод + +High-находок нет. Одна Medium-находка — строго в скоупе этой же задачи +(это тест, добавленный именно как ответ на Medium-1 из r1 для доказательства +AC5, и находка о том, что он не доказывает заявленное для конкретного +класса регрессии), поэтому решение не заводит отдельный issue, а +возвращает задачу автору жёлтым вердиктом. Продуктовый код и AC8 +подтверждены; исправить нужно только конструкцию AC5-теста (устранить +разделяемую немутированную ссылку на `fixture` между `baseline`-вызовом +и построением проверяемых `walls`/`rooms`) и повторно прогнать `npm +test` + адверсариальную мутацию `.map`→`for`, которой было +продемонстрировано отсутствие защиты.