From 8acbd333d899159dfc66ab33e0f31f8fdbc4e55b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:43:09 +0000 Subject: [PATCH] docs: review document for #510 Issue: #510 User-Visible: no --- docs/reviews/CODE-REVIEW-510-r2.md | 245 +++++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-510-r2.md diff --git a/docs/reviews/CODE-REVIEW-510-r2.md b/docs/reviews/CODE-REVIEW-510-r2.md new file mode 100644 index 00000000..9ba6d079 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-510-r2.md @@ -0,0 +1,245 @@ +# CODE-REVIEW-510-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/510 +- **Материал:** `00130e08aa247cd8796a99125deb7f9b5131bfa1` (рабочая копия на нём; `git rev-parse HEAD` сверен непосредственно перед выводом) +- **Заход:** r2 · блокирующих циклов израсходовано (после этого раунда) 2 из 4 +- **Ветка:** `issue/510-mutants-on-candidate-and-review-gate` +- **Предыдущий раунд:** CODE-REVIEW-510-r1 (жёлтый, Medium M1), материал `cbece6324f83b3400364fcd1a8eb1fa3b321a3ff` +- **ТЗ:** `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md` (r2 добавил одно предложение про `cancelled`), ревью ТЗ зелёное на r4 (`docs/reviews/SPEC-REVIEW-510-r4.md`) + +## Скоуп раунда + +`git log --oneline cbece632..00130e08`: + +``` +00130e08 ci: a cancelled Validate dispatch proves nothing to the review gate +f6e64ba9 docs: review document for #510 +``` + +Никакого ребейза между r1 и r2 не было: `00130e08` — прямой потомок `cbece632` +(два коммита сверху), дерево то же самое плюс точечный фикс M1. Это не смена +кода по содержанию (§7.2) — разбор ведётся по дельте. + +`git diff cbece632..00130e08 --stat`: + +``` +docs/reviews/CODE-REVIEW-510-r1.md | 191 +++++++++ +docs/specs/510-mutants-on-candidate-and-review-waits-validate.md | 2 +- +scripts/merge-candidate.mjs | 4 +- +scripts/mutation-gate.mjs | 11 ++ +scripts/validate-gate.mjs | 10 +- +test/validate-gate.test.mjs | 18 ++ +6 files changed, 233 insertions(+), 3 deletions(-) +``` + +Предмет дельты — ровно закрытие M1 из r1: «отменённый (`cancelled`) +dispatch-прогон читается как красный вместо того, чтобы игнорироваться», +найденное в двух местах (`scripts/validate-gate.mjs` и +`scripts/merge-candidate.mjs`). Разбор этого раунда сосредоточен на том, +чем и насколько полно M1 закрыт; остальные AC (1, 3, 4, 5, 6) delta не +задевает — унаследованы из r1 без повторной проверки (раздел ниже). + +## Как проверялось + +Зелёного Validate на `00130e08` не найдено (см. заголовок задачи) — гейты +прогнаны лично. + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | зелёный, без вывода | +| unit (полный набор) | `npm test` | 2421 тестов, 2420 pass, 1 skip (предсуществующий #281, не в этом диффе), 0 fail | +| build + бандл | `npm run build` затем `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | сборка зелёная, копии идентичны | +| check-docs / golden / смоки / инварианты / pytest / perf | не прогонялись | `src/**` и `custom_components/**/*.py` в дельте не тронуты — те же основания, что в r1 (AC6) | +| новый мутант протокола `review-returns-task-on-cancelled-dispatch` | `node scripts/mutation-gate.mjs --id=review-returns-task-on-cancelled-dispatch` | `поймано 1 из 1` (лично, не со слов хендоффа) | +| пять мутантов, подтверждённых в r1 (AC4) | не перепрогонялись | делта не трогает их патчи/guard-и (`mutation-gate.mjs` только добавил новую запись) — унаследовано | + +### Демонстрация: тест умеет падать — и её отсутствие там, где оно важно + +Для нового мутанта `review-returns-task-on-cancelled-dispatch` заявленный +раннер сам это доказывает («тест покраснел, как обязан»). Но M1 в r1 назвал +**два** места с одним и тем же паттерном — `scripts/validate-gate.mjs` и +`scripts/merge-candidate.mjs`. Проверил вторую половину лично: + +``` +cp scripts/merge-candidate.mjs /tmp/merge-candidate.mjs.bak +# вручную снял фильтр отменённых прогонов: +# const runs = all.filter((x) => (!event || x.event === event) && x.conclusion !== 'cancelled'); +# → const runs = all.filter((x) => (!event || x.event === event)); +node --test test/merge-candidate.test.mjs # 10/10 pass — ни одного упавшего +npm test # 2420/2420 pass — ни одного упавшего +cp /tmp/merge-candidate.mjs.bak scripts/merge-candidate.mjs # восстановлено, git diff пуст +``` + +Результат: снятие фикса в `merge-candidate.mjs` **не роняет ни одного теста +во всём проекте**. См. находку ниже. + +## Находки + +### Medium (в скоупе задачи — чинится в этой же задаче, без отдельного issue) + +**M1 (новая, r2). Закрытие M1(r1) асимметрично: половина фикса в +`scripts/merge-candidate.mjs` не защищена ни одним тестом или мутантом.** + +- Файл: `scripts/merge-candidate.mjs:145` — `waitValidate` внутри `realOps`: + ```js + const runs = all.filter((x) => (!event || x.event === event) && x.conclusion !== 'cancelled'); + ``` + Сам код корректен и делает ровно то, что требует обновлённый текст ТЗ + («то же в `waitValidate` слияния»): отменённый прогон исключается из + списка кандидатов, гейт слияния ждёт замену вместо того, чтобы красить + кандидата. +- **Но:** ни один тест не вызывает эту ветку кода. `test/merge-candidate.test.mjs` + проверяет `mergeCandidate()` только с полностью замоканным `ops.waitValidate` + (строки 83, 207) либо со «настоящим git», где `ops.waitValidate` тоже + подменён фейком (строка 207 того же теста) — реальная реализация + `waitValidate` внутри `realOps()` не вызывается **ни разу** ни в одном + тесте проекта. Соответственно ни один из пяти мутантов, привязанных к + `merge-candidate.mjs` (`merge-pushes-unvalidated-candidate`, + `merge-ignores-lease-rejection`, `merge-waits-push-run-without-mutants` и + др., `scripts/mutation-gate.mjs:3696,3707,8178`), не патчит и не может + поймать регресс именно этой строки — реестр про неё вообще не знает. +- **Воспроизведено лично** (см. блок выше): при полностью снятом фильтре + `x.conclusion !== 'cancelled'` весь набор (`npm test`, 2420 тестов) + остаётся зелёным. Это ровно тот класс дефекта, о котором предупреждает + сама дисциплина ревью этого проекта («зелёный тест без реальной защиты», + PROCESS.md §2.7, прецеденты #423/#430) — только тут защиты нет вообще, + а не «не та». +- **Почему это Medium, а не Low/наблюдение:** `scripts/validate-gate.mjs` + получил в этом же коммите два прицельных теста и мутант реестра для + ровно того же паттерна; `scripts/merge-candidate.mjs` — тот же паттерн, + та же формулировка ТЗ, но без единого свидетеля. Будущий рефакторинг + (например, унификация двух копий одной и той же логики фильтрации, + которую сам этот диф оставил задублированной) может тихо вернуть + «cancelled = red» именно в путь слияния кандидата — и ничего в CI об + этом не сообщит. Это не гипотетический сценарий, а прямое следствие + того, что issue #510 в целом решает задачу «не тратить впустую циклы + ревью/минуты раннера»: путь слияния (`merge-candidate.mjs`) — как раз + тот, где отменённый прогон при повторной попытке гейта слияния красит + готового к мержу кандидата. +- **Почему в скоупе:** M1 в r1 явно называла оба файла как один и тот же + дефект («тот же паттерн в `scripts/merge-candidate.mjs:147`»); закрытие + найдено только частичным — код исправлен, но не тестом, который умеет + упасть. Чинится добавлением теста, эксплуатирующего `realOps().waitValidate` + напрямую (с фейковым `sh`/`gh` или через существующий git-фикстурный тест + строка ~200, но с подставленным реальным `waitValidate` вместо фейка) плюс + записи в реестр мутантов. + +Других Medium/High не найдено. + +### Low + +Не найдено требующих записи. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** (`cancelled` dispatch читается как `red` вместо игнорирования) — половина 1: `scripts/validate-gate.mjs:32` | Код: `validate-gate.mjs:60-66` — ветка `run.conclusion === 'cancelled'` добавляет прогон в `ignored`, сбрасывает `tracked`, продолжает цикл (ждёт замену либо диспатчит свой). Тест: `test/validate-gate.test.mjs:109-125` — два новых сценария («замена уже есть» / «замены нет»), лично прогнаны, зелёные. Мутант: `scripts/mutation-gate.mjs` id `review-returns-task-on-cancelled-dispatch` — лично прогнан, `поймано 1 из 1` | `git diff cbece632..00130e08 -- scripts/validate-gate.mjs test/validate-gate.test.mjs scripts/mutation-gate.mjs`; вывод `node scripts/mutation-gate.mjs --id=review-returns-task-on-cancelled-dispatch` выше | +| **M1** — половина 2: `scripts/merge-candidate.mjs:147` (было) | Код исправлен: `merge-candidate.mjs:145` — тот же фильтр `x.conclusion !== 'cancelled'` внутри `waitValidate`. Тест/мутант — **отсутствуют**, регрессия не ловится (см. воспроизведение выше) | `git diff cbece632..00130e08 -- scripts/merge-candidate.mjs`; ручной откат фикса + `npm test` (2420/2420 pass) выше — новая находка M1(r2) | +| Low (r1: «не найдено требующих записи») | — | нечего закрывать, r1 не оставил Low | + +## Унаследовано из r1 + +Принято без повторной проверки в этом раунде — дельта (`docs/specs/*.md` +1 +строка, `scripts/{merge-candidate,mutation-gate,validate-gate}.mjs`, +`test/validate-gate.test.mjs`) их не задевает. Источник — CODE-REVIEW-510-r1.md +на материале `cbece6324f83b3400364fcd1a8eb1fa3b321a3ff`: + +- **AC1** (обычный push не запускает `changed_mutants`; dispatch/PR/schedule/кандидат + беты — запускают) — доказано `test/classify-changes.test.mjs` + + `test/validate-workflow.test.mjs`, мутант `mutants-run-on-every-push` + («поймано 1 из 1» в r1). Файлы дельтой не тронуты. +- **AC2** (гейт находит/запускает/красный/таймаут/несовпадение SHA) — доказано + `test/validate-gate.test.mjs` (исходные 9 сценариев r1) + `review-doc-guard.test.mjs`, + мутанты `review-starts-on-red-validate` / `review-trusts-push-run-without-mutants`. + `.github/workflows/process.yml` дельтой r1→r2 не тронут вовсе. +- **AC3** (слияние ждёт dispatch с мутантами, не push) — доказано + `test/merge-candidate.test.mjs`, мутанты `merge-waits-push-run-without-mutants` / + `merge-pushes-unvalidated-candidate`. Логика диспатча/ожидания, за исключением + фильтра `cancelled` (см. новую находку M1 выше), дельтой не изменена. +- **AC4** (мутанты протокола пойманы штатным раннером) — пять мутантов r1 + подтверждены лично в r1; дельта r2 добавляет к ним шестой + (`review-returns-task-on-cancelled-dispatch`, подтверждён в этом раунде). + Остальные пять не перепрогонялись — их патчи/guard-файлы не менялись. +- **AC5** (доки описывают место мутантов и правила хендоффа) — + `PROCESS.md`/`AGENTS.md`/`docs/TESTING.md`/`docs/specs/README.md` сверены + построчно в r1; в дельте r1→r2 из этого списка правок нет (только сам + ТЗ-файл получил одно уточняющее предложение — см. `git diff` выше, + прочитано, соответствует коду). +- **AC6** (`src/**` не тронут, perf/touch/UX не задеты) — верно и для этой + дельты: `git diff cbece632..00130e08 --stat` не содержит ни одного файла + `src/**`. +- Асимметрия дизайна между `validate-gate.mjs` (учитывает чужой push-прогон) + и `merge-candidate.mjs` (SHA кандидата всегда свежий) — признана + обоснованной в r1, дельта её не меняет. + +## Что проверено и корректно (в этом раунде) + +- `scripts/validate-gate.mjs`: ветка `cancelled` корректно встроена в + существующий цикл `ignored`/`tracked=null`/`continue` — не ломает уже + протестированные сценарии (chужой skip-dispatch, красный, таймаут, + «материал сменился», слежение за tracked-run — все 9 старых тестов плюс + 2 новых зелёные). +- `scripts/merge-candidate.mjs`: сама формула фильтра корректна и + синтаксически идентична логике `validate-gate.mjs` — по коду поведение + правильное (проверено чтением и ручным откатом-и-восстановлением, + описанным выше), проблема только в отсутствии автотеста. +- Правка ТЗ (`docs/specs/...md`, одна строка) точно описывает реализованное + поведение обоих мест, включая «то же в `waitValidate` слияния» — текст + не разошёлся с кодом. +- `scripts/mutation-gate.mjs`: новая запись `review-returns-task-on-cancelled-dispatch` + синтаксически корректна (`find`/`replace` совпадают один-в-один со строкой + кода), `guard` запускает верный тестовый файл, лично прогнана — красит + тест при внесении мутации и не красит при чистом дереве. + +## Чего не проверял и почему + +- `check-docs.mjs`, golden, смоки, инварианты модели, `pytest tests_backend`, + perf-профили — дельта не касается `src/**`, `custom_components/**/*.py` + или геометрии; основание не изменилось с r1 (AC6). +- Пять мутантов протокола, подтверждённых в r1 (`mutants-run-on-every-push`, + `review-starts-on-red-validate`, `review-trusts-push-run-without-mutants`, + `merge-waits-push-run-without-mutants`, `merge-pushes-unvalidated-candidate`) — + не перепрогонял: дельта не меняет ни их `patches`, ни `guard`-файлы + (`git diff` выше показывает только добавление новой записи в реестр). +- Реальный прогон `validate-gate.mjs`/`merge-candidate.mjs` против живого + GitHub Actions — не воспроизводил (та же причина, что в r1: `gh` в этом + окружении не аутентифицирован для целей ревью); логика проверена юнитами, + мутантами и, для найденного пробела, ручным откатом-фикса — это и есть + предмет код-ревью для скрипта без сети. +- Зеркалирование `process.yml`/`validate.yml` в `main` (AC5, §5.3 ТЗ) — шаг + публикации после слияния, не код-ревью. + +## Материал раунда + +- SHA: `00130e08aa247cd8796a99125deb7f9b5131bfa1` (сверено `git rev-parse HEAD` + перед выводом). +- Предыдущий материал (r1, код): `cbece6324f83b3400364fcd1a8eb1fa3b321a3ff`. +- Ребейза между раундами не было — прямые 2 коммита сверху, дельта локальна. + +## Вердикт + +Жёлтый: одна находка Medium в скоупе (M1(r2) — половина фикса M1(r1) в +`scripts/merge-candidate.mjs` не защищена тестом/мутантом, регресс +воспроизведён лично и не ловится ни одним из 2420 тестов проекта), High +нет. Остальное из r1 (AC1, AC3–AC6, половина M1 в `validate-gate.mjs`) +закрыто полностью и подтверждено. Возврат автору для добавления теста +(и, по возможности, мутанта) на `realOps().waitValidate` в +`scripts/merge-candidate.mjs`. + +--- + + + +## Материал раунда + +- Ветка: `issue/510-mutants-on-candidate-and-review-gate`, коммит `00130e08aa24` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `e53cf9a26b9c66fb7602e485b8dffd1c7d286212` + ``` + git log --all --format='%H %T' | grep e53cf9a26b9c + ``` +- ТЗ `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md`, блоб `d68ed7818b4c77535305d0aae7da9765a2ad6c61` + ``` + git log --all --find-object=d68ed7818b4c77535305d0aae7da9765a2ad6c61 -- docs/specs/510-mutants-on-candidate-and-review-waits-validate.md + ``` +- Вердикт конвейера: `yellow` · High 0