diff --git a/docs/reviews/CODE-REVIEW-475-r1.md b/docs/reviews/CODE-REVIEW-475-r1.md new file mode 100644 index 00000000..e12ee860 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-475-r1.md @@ -0,0 +1,187 @@ +# CODE-REVIEW-475-r1 + +Issue: #475 · «mutation-gate --check не отличает живого свидетеля от мёртвого» +Трек: `small` (лёгкий) · Этап: код-ревью · Заход: r1 · блокирующих циклов 0/2 +SHA материала: `c0207af40f25564eac9773d583cdcc7f5c56ea45` (`git rev-parse HEAD` перед выводом) +Ветка приведена к `dev` конвейером до ревью: поверх легло 4 коммит(ов) dev +(`9b41ac1c` → `c0207af4`). Диапазон `origin/dev..HEAD` после этого содержит +ровно один коммит задачи — разбор полный (§7.2), а не по дельте, как и +предписано при ребейзе на ушедший вперёд `dev`. + +## Скоуп + +Диапазон `git diff origin/dev...HEAD`, 4 файла, все класса B: + +- `.github/workflows/validate.yml` — новая job `changed_mutants`; +- `scripts/mutation-gate.mjs` — `guardFiles()`, расширенный `selectChangedMutants()`, 2 новых мутанта; +- `test/mutation-gate.test.mjs` — AC1–AC3, AC5–AC7; +- `test/validate-workflow.test.mjs` — контрактный тест на YAML (AC4). + +Класса A ни один файл не задет — трек `small`, ТЗ в теле issue, ревью ТЗ +прошло два захода (r1 жёлтый/Medium, r2 зелёный) и закрыто до кода. + +## Как проверялось + +Гейты, соразмерно классу B без изменений в `src/**` (issue #127, PROCESS.md §8): + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный, 7.2s | +| Юниты | `npm test` | **2083 pass / 0 fail / 1 skip**, 38.7s (совпадает с числом автора в хендоффе) | +| Сборка + синхрон бандла | `npm run build` затем `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | зелёный, байт-в-байт совпадение | +| Новый код не добавляет `any` | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | «Проверено добавленных строк в src/\*\*/\*.ts: 0» — диф не касается `src/**` | +| Отбор браузерных смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет (src/\*\*/\*.ts не тронут)» — смоки не выбраны, выбирать нечего | +| Реестр мутантов применим | `node scripts/mutation-gate.mjs --check` | 509 мутантов, все `ok`, включая 2 новых | +| Мутант `changed-selection-ignores-guard-files` | `node scripts/mutation-gate.mjs --id=changed-selection-ignores-guard-files` | «чистый прогон: ok», «тест покраснел, как обязан», поймано 1/1 | +| Мутант `changed-selection-matches-any-token` | `node scripts/mutation-gate.mjs --id=changed-selection-matches-any-token` | то же, поймано 1/1 | +| Самоприменение отбора | `node scripts/mutation-gate.mjs --changed=origin/dev..HEAD` | «файлов в диффе 4, мутантов затронуто 4 из 509» — независимо воспроизведено, состав описан ниже | +| AC7 напрямую | `node -e "...selectChangedMutants(MUTANTS, [...])..."` | все 4 бэкенд-мутанта `frontend_registration.py`/`test_ha_frontend_registration.py` отобраны и по гарду, и по патчу | + +**Не прогонялось и почему:** + +- `check-docs.mjs` — правило #127/§8 требует его при диффе по `src/**`; diff файлов `src/**` не содержит. Пропуск честный, не молчаливый. +- `python -m pytest tests_backend -q` — окружение ревью не содержит `.venv-backend` и `pytest` (`pip show pytest` → not found), это задокументированное ограничение (PROCESS.md «Backend» и AGENTS.md «Environments»), не результат этой задачи. При попытке независимо прогнать `--changed=origin/dev..HEAD` до конца скрипт сам упёрся в это же: мутант `typing-gate-stops-running` (не из #475, патчит `.github/workflows/validate.yml`) требует `pytest`, которого здесь нет. AC7 проверен напрямую через API функции (см. таблицу) вместо прогона pytest. +- `golden:verify`, model-invariants, performance-профили, single-source-numbers — diff не меняет визуал, геометрию, ссылки на неё или пользовательские числа; неприменимо. + +## Находки + +### Medium (в скоупе) — триггер job не покрывает третье условие контракта + +**Файл:** `.github/workflows/validate.yml:410` +**Что не так:** Контракт §2 принятого ТЗ (тело issue #475, редакция после фикса Medium из SPEC-REVIEW r1) требует: + +> job запускается при `frontend == 'true'` **или `backend == 'true'` или изменении `scripts/mutation-gate.mjs`** + +Реализовано: + +```yaml +if: needs.changes.outputs.frontend == 'true' || needs.changes.outputs.backend == 'true' +``` + +Третье условие отсутствует — ни в `if:`, ни в контрактном тесте +`test/validate-workflow.test.mjs:290`, который проверяет ровно эти же два +дизъюнкта и не более. + +**Воспроизведение (не гипотеза — проверено чтением классификатора):** +`changes` job классифицирует файлы двумя regex (`validate.yml:262-263`): + +``` +frontend: ^(src/|demo/|test/|dist/|custom_components/houseplan/frontend/|package(-lock)?\.json$|rollup\.config\.mjs$|tsconfig) +backend: ^(custom_components/.*\.py$|tests_backend/|scripts/support-relay/|pytest\.ini$) +``` + +`scripts/mutation-gate.mjs` не начинается ни с одного из перечисленных +префиксов и не совпадает ни с одним `$`-якорем — ни `frontend`, ни `backend` +не станут `true` от диффа, ограниченного этим файлом. Значит правка +`scripts/mutation-gate.mjs` (например, новый мутант, патчащий +`src/`/`custom_components/`-файл, без сопутствующей правки в `test/**`) +проходит без единого запуска `changed_mutants` — job попросту не +запланируется на этом пуше. + +Это ровно тот класс дефекта, ради которого заведён #475: рефакторинг файла, +отвечающего за отбор мутантов, остаётся невидимым до недельного полного +прогона (или до релиза) — тот же разрыв «неделя–релиз» из #466/#467, только +теперь применительно к самому гейту, а не к продуктовому коду. Contract §2 +называет это условие явно и с тем же обоснованием, каким объяснено условие +`backend` («бэкенд-мутанты патчат `.py`... дифф... даёт `backend=true` без +`frontend=true`») — то есть это не пропущенная деталь ТЗ, а нереализованная +часть уже принятого контракта. + +**Почему не High:** основной сценарий (правка продуктового/тестового кода, +меняющая `patch.file` или файл гарда любого мутанта) работает и подтверждён +и юнитами, и самоприменением. Пробел узкий — правка ограничена буквально +одним файлом `scripts/mutation-gate.mjs` без сопутствующих `test/**`/`src/**`/ +`custom_components/**.py` изменений — но контракт именно этот случай называет +по имени, поэтому строка контракта не выполнена. + +**Чем закрывается (в скоупе, без выхода за задачу):** добавить в `if:` +третий дизъюнкт, обнаруживающий изменение `scripts/mutation-gate.mjs` +(например, отдельным шагом `changes`/`classify` либо прямой проверкой пути +диапазона в самой job), и покрыть его тем же контрактным тестом. + +### Low — тест AC7 не покрывает четвёртый бэкенд-мутант, хотя код это делает + +**Файл:** `test/mutation-gate.test.mjs:259–267` +В реестре ровно 4 мутанта, патчащих `custom_components/houseplan/frontend_registration.py` +с гардом `tests_backend/test_ha_frontend_registration.py` (`scripts/mutation-gate.mjs:120,132,146,158`). +Тест AC7 фильтрует их через `id.startsWith('frontend-registration-')`, что +даёт только 3 — четвёртый называется `frontend-reload-notice-forgets-persisted-flag` +(другой префикс id, тот же патч-файл и гард) и в assert-цикл не попадает. + +Проверено напрямую (не через тест): `selectChangedMutants` реально отбирает +все 4 и по гарду, и по патчу — сама реализация не различает мутанты по имени, +поэтому регрессии здесь нет, только тест не проверяет ровно то, что заявляет +ТЗ («отбирает все четыре frontend-registration-\* мутанта»). Наименование в +ТЗ тоже неточно (не все четыре носят префикс `frontend-registration-`). +Снимаю без исправления: механизм общий, не завязан на имя мутанта, и +пропущенный четвёртый защищён тем же кодовым путём, что и проверенные три — +регрессия, ломающая только его и не ломающая остальные три, потребовала бы +отдельного условия по конкретному id, которого в реализации нет. + +## Проверка AC — таблица «чем доказан / чем краснеет» + +| AC | Доказано | Чем | Чем краснеет | +|---|---|---|---| +| AC1 (патч-файл в диффе → отбор) | unit | `test/mutation-gate.test.mjs:228` (`#475 AC1`) + ранее существовавший `#332` тест | защитный AC без мутанта не заявлен (простое сравнение выборки) | +| AC2 (файл гарда — тест/смок/pytest — в диффе → отбор) | unit + мутант | `test/mutation-gate.test.mjs:233` (`#475 AC2`) | мутант `changed-selection-ignores-guard-files`: гвард (`--test-name-pattern="#475 AC2"`) чист на исходном коде и красный на патче (проверено: `--id=changed-selection-ignores-guard-files` → «поймано 1 из 1») | +| AC3 (токены-нефайлы не считаются) | unit + мутант | `test/mutation-gate.test.mjs:241` (`#475 AC3`) | мутант `changed-selection-matches-any-token`: проверено аналогично, «поймано 1 из 1» | +| AC4 (job в validate.yml: триггер, база диапазона, python-зависимости, блокирующая) | частично unit, частично чтением | `test/validate-workflow.test.mjs:283` + чтение `validate.yml:407-459` | **не выполнено полностью** — триггер не покрывает третий дизъюнкт контракта (см. находку Medium); база диапазона, установка Python и отсутствие `continue-on-error` подтверждены и текстом job, и сверкой с `mutation-gate.yml`/`frontend`-job (byte-идентичный паттерн вычисления `base`) | +| AC5 (пустой отбор → зелёный выход без сборки бандла) | unit + чтение | `test/mutation-gate.test.mjs:250` (`#475 AC5`) для отбора; `guardNeedsBundle`/`buildBundle` вызывается только для отобранных мутантов с браузерным гвардом (`scripts/mutation-gate.mjs:6779,6801`) — проверено чтением, не исполнением полного CI-прогона | защита не заявлена как guard/limit — расположение раннего выхода, обычное сравнение | +| AC6 (репродукция #467: `src/wall-thickness.ts` → 3 мутанта) | unit | `test/mutation-gate.test.mjs:255` (`#475 AC6`), прогнан в составе `npm test` | простое сравнение множества id, мутант не требуется правилом (не защитный AC) | +| AC7 (репродукция ревью: `.py`-патч/гард → 4 бэкенд-мутанта) | unit (неполный, см. находку Low) + независимая проверка | `test/mutation-gate.test.mjs:259`; дополнено прямым вызовом `selectChangedMutants` в этом ревью, подтвердившим все 4 | простое сравнение множества id | + +## Что проверено и корректно + +- **`guardFiles()`** извлекает файлы без парсинга команды: только токены вида + `[\w./-]+\.(mjs|py)`, не начинающиеся с `-`, существующие в репозитории. + Проверено на реальных гардах (`--test-name-pattern="magnet presses|x.mjs"` + корректно исключает фрагмент шаблона, оставляя только `test/furniture.test.mjs`) + и на `bundle:sync && node demo/smoke_*.mjs` (составные команды с `&&`). +- **`selectChangedMutants`** обратно совместима: старые вызовы без третьего + параметра `exists` продолжают работать (дефолт — `existsSync`), старый тест + `#332` по-прежнему проходит без изменений сигнатуры вызова. +- **Мутанты #475 сгенерированы корректно**: якорь `changed-selection-ignores-guard-files` + собран из двух конкатенированных строк ровно затем, чтобы `--check` не находил + его дважды (в коде и в определении мутанта) — воспроизведено: `--check` даёт + `ok` на обоих новых мутантах. +- **Установка Python-зависимостей и Chromium** в `changed_mutants` дословно + повторяет шаги `mutation-gate.yml` (checkout → setup-node → npm ci → + setup-python 3.14 → `pip install -r tests_backend/requirements.txt` → кэш + Playwright → установка Chromium при промахе кэша) — сверено построчно. +- **База диапазона** (`PROVEN_BASE`/`BEFORE_SHA`/`merge-base` с фолбэком) + побайтово идентична блоку, уже проверенному в job `frontend` для гейта + «новый код не добавляет any» (#387/#388) — не новая, а переиспользованная + логика. +- **Трейлеры коммита**: `Issue: #475`, `User-Visible: no` — корректно для + инфраструктурной правки без пользовательского эффекта; changelog не тронут, + что верно при `User-Visible: no`. +- **Класс файлов**: все правки — класс B (`test/**`, `scripts/**`, + `.github/workflows/**`), issue переиспользован по правилу AGENTS.md. + +## Унаследовано из предыдущих раундов + +Не применимо — это первый заход код-ревью (`r1`). Ревью ТЗ (`SPEC-REVIEW-475-r1` +жёлтый, `SPEC-REVIEW-475-r2` зелёный) относится к отдельному этапу и не +экономит объём этого разбора; код проверен полностью, включая уже закрытую +там находку (расширение `.py`/`backend`), которая была сверена по факту в коде +(см. «Что проверено и корректно»), а не принята на слово. + +## Вердикт + +Жёлтый. Единственная блокирующая находка — Medium в скоупе задачи +(триггер `changed_mutants` не покрывает изменение `scripts/mutation-gate.mjs`, +как того явно требует Contract §2 принятого ТЗ). High-находок нет. Правится +в этом же issue без нового цикла ревью ТЗ; после фикса — обычный повторный +заход код-ревью по дельте (§2.10). + +--- + + + +## Материал раунда + +- Ветка: `issue/475-changed-mutants-on-push`, коммит `9b41ac1c7041` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `5483eee97c02fa1a40b6d6199b30741353263bf4` + ``` + git log --all --format='%H %T' | grep 5483eee97c02 + ```