diff --git a/docs/reviews/CODE-REVIEW-546-r1.md b/docs/reviews/CODE-REVIEW-546-r1.md new file mode 100644 index 00000000..c484106c --- /dev/null +++ b/docs/reviews/CODE-REVIEW-546-r1.md @@ -0,0 +1,140 @@ +# CODE-REVIEW-546-r1 + +Issue: #546 · Заход: r1 · Материал: `5fc596c748c59e53d87297085a6b053c7f149c6e` (HEAD ветки `issue/546-wait-round`, `git diff origin/dev...HEAD`) + +## Скоуп + +Инфраструктурная задача (класс A файлов не затронут — трек §1 корректен, без S1…S6, +сразу на `S7-code-review`). Правит `scripts/wait-verdict.mjs`: waiter больше не +считает *любой* последний pipeline-комментарий issue новым событием текущего +раунда. Вводится якорь раунда — последнее фактическое применение метки +`S4-spec-review`/`S7-code-review`, взятое из timeline issue +(`GET /repos/{repo}/issues/{number}/events`, постранично). Комментарии старше +этого якоря — baseline и не завершают ожидание; комментарии не раньше якоря +(включая уже опубликованные до старта waiter) доставляются как раньше. + +Изменённые файлы: `scripts/wait-verdict.mjs`, `scripts/mutation-gate.mjs`, +`test/wait-verdict.test.mjs`, `AGENTS.md`, `PROCESS.md`. Один коммит, +трейлеры `Issue: #546` / `User-Visible: no` на месте — корректно: правится +только внутренний dev-tooling, пользовательского поведения нет, changelog не +требуется. + +## Как проверялось + +Validate на этом SHA зелёный ([run 34745280749](https://github.com/Matysh/houseplan-card/actions/runs/34745280749)), +поэтому `npx tsc --noEmit` / `npm test` / `npm run build` со сверкой копий +бандла не перегонялись — взят как подтверждение дешёвых гейтов. Diff не +трогает `src/**`, геометрию, `custom_components/**/*.py` и визуальный +результат, поэтому `check-docs.mjs`, `model-invariants.mjs`, `golden:verify`, +`pytest tests_backend` и браузерные смоки не применимы (смок-выбор не +запускал — diff вне `demo/**`/`src/**`, `smoke-select.mjs` не даёт кандидатов +по такому диффу). + +Прогнано мной в этом раунде (код изменился со времени прогона в хендоффе, +плюс это ровно предмет находок предыдущих раундов #546-подобных задач — +дёшево и целевое): + +| Команда | Результат | +|---|---| +| `node --test test/wait-verdict.test.mjs` | 8/8 green | +| `node --test --test-name-pattern="#546" test/wait-verdict.test.mjs` | 4/4 green (целевые тесты задачи) | +| `node scripts/mutation-gate.mjs --check` | зелёный, весь реестр, включая новый мутант | +| `node scripts/process-gate.mjs` | «гейт пройден, предупреждений 0» | + +Не прогонял: `npm run gate:small` целиком, `npm run toolchain:check`, +performance-профили — не относятся к диффу (нет продуктового/фронтенд кода, +нет perf-чувствительных путей), и Validate на этом SHA их уже подтвердил там, +где они применимы. + +## AC — доказательство и «чем краснеет» + +| AC (из тела issue) | Чем доказан | Чем краснеет | +|---|---|---| +| Старый failure + новый running review: ожидание продолжается без ложного выхода | `test/wait-verdict.test.mjs`: «старый failure до нового запроса ревью — baseline, ожидание продолжается (#546)» | мутант `wait-verdict-reuses-historical-failure` (`scripts/mutation-gate.mjs`): снимает фильтр `eventBelongsToReview` — `node scripts/mutation-gate.mjs --check` ловит (строка `ok wait-verdict-reuses-historical-failure`; проверил также прямым запуском guard-команды `node --test --test-name-pattern="#546" test/wait-verdict.test.mjs` с патчем, гейт-скрипт сам применяет и откатывает патч и подтверждает провал под мутацией) | +| Текущий failure уже опубликован до начала wait: он обнаруживается сразу | «failure текущего раунда, опубликованный до запуска waiter, виден на первом poll (#546)» | тот же мутант — фильтр общий для обеих веток (baseline и current) | +| Новый verdict, отмена/смена material и owner-blocker различаются; не игнорировать все первоначальные comments | «новые outcome и owner blocker текущего раунда не скрываются baseline-фильтром (#546)» — прямое сравнение ожидаемого/фактического, не требует мутанта (ветвление не защитное, а обычная маршрутизация событий, уже покрытая исходным #496-набором тестов) | н/п — не защитный AC в терминах §2.7 (правило прямо выводит из-под мутанта «расположение/текст/формат», сюда же относится «какой code возвращается по какому типу события»); свидетель — сравнение | +| Тесты работают детерминированно без реального длительного sleep и без модели | проверено чтением: `sleep` — инъецируемая функция-счётчик, `readSnapshot` — синтетический замыкающий объект без сети; исполнением подтверждено временем прогона (`duration_ms` < 100 мс на всю группу #546) | н/п, не защитный AC | + +Якорь раунда (`reviewRequestFromEvents`) проверен отдельно тестом «якорь +раунда — последнее применение любой review-метки (#546)»: смешанный поток +`labeled S4 → labeled P1 → unlabeled S4 → labeled S7` корректно даёт +последним именно `S7`-событие. Мутанта на этот отбор нет, но неверный выбор +анкера транзитивно ломает оба #546-теста через общий путь `stateOf` → +`eventBelongsToReview`, поэтому и здесь есть живой свидетель регрессии. + +## Что проверено и корректно + +- `ghSnapshotReader` использует `gh api --paginate --slurp + repos/{repo}/issues/{number}/events?per_page=100` и правильно разворачивает + результат: `--slurp` оборачивает каждую страницу (сама по себе массив) во + внешний массив, поэтому `pages` — массив массивов, и `pages.flat()` + корректен для одной и для многих страниц; пустая история (`pages = [[]]` + или `[]`) сводится к `timelineEvents = []`, а `reviewRequestFromEvents([])` + возвращает `null` → `eventBelongsToReview` откатывается к старому + поведению «всё принадлежит раунду» (совместимость со снимками без + timeline, в том числе с прямыми вызовами `snap()` в тестах 1–4, не + передающими `reviewRequest`) — прочитано и подтверждено прогоном тех же + тестов (не регрессировали). +- Формат события REST `/issues/{number}/events` (`event: 'labeled'`, + `created_at`, `label: {name}`) соответствует и коду, и фикстурам тестов — + прочитано, совпадает с задокументированной схемой GitHub REST API. +- `blocked` и смена метки-вердикта не проходят через + `eventBelongsToReview` вовсе (читаются из `labels`, не из `comments`), + поэтому сужение к текущему раунду не может случайно скрыть их — прочитано + в `decide`/`stateOf`, подтверждено тестом «новые outcome и owner blocker…». +- Документация (`AGENTS.md`, `PROCESS.md`) обновлена в том же коммите и + точно описывает новое поведение (baseline до последнего `S4`/`S7`, немедленная + доставка уже случившегося исхода текущего раунда) — соответствует §2.6/§11 + DoD. +- Трейлеры коммита корректны (`Issue: #546`, `User-Visible: no`); ветка + `issue/546-wait-round` соответствует конвенции; `process-gate.mjs` зелёный + локально. + +## Чего не проверял + +- Реальный вызов `gh api .../events` против живого GitHub (сетевой путь + `ghSnapshotReader`) — не покрыт автотестом (как и раньше не был, для + соседнего блока `gh run list` в этом же файле — не новый пробел, а + существующий паттерн). Автор заявляет ручной read-only прогон на issue с + длинной историей; принимаю как «проверено чтением + разовая ручная + проверка автора», не как автотест. Риск низкий: контракт REST-эндпоинта + стабилен, фикстуры тестов повторяют его форму дословно. +- Тай-брейк `id.localeCompare` в `reviewRequestFromEvents` при **совпадающих + секундных** таймстемпах двух labeled-событий сравнивает числовые id как + строки (лексикографически, не численно) — теоretически может выбрать не + тот из двух событий при коллизии. Не тестировалось и не мутировалось. + Вероятность на практике пренебрежимо мала (два применения `S4`/`S7` в один + и тот же issue в одну и ту же секунду) и не относится к заявленным AC — + фиксирую как Low, не блокирует, правку не считаю обязательной. +- Полный `npm run gate:small`, perf-профили, golden, backend pytest — не + прогонялись, diff их не касается (см. «Как проверялось»). + +## Находки + +Нет High и Medium в скоупе. Одна Low-наблюдение (тай-брейк по id при +одинаковых секундных таймстемпах, см. выше) — не блокирует зелёный вердикт, +оставляю как запись без правки: цена коллизии ничтожна и вне AC задачи. + +## Вывод + +Все четыре AC доказаны детерминированными тестами; защитный AC (baseline +против ложного немедленного выхода / против маскировки текущего отказа) +дополнительно закрыт мутантом `wait-verdict-reuses-historical-failure`, +который я лично прогнал и подтвердил, что он ловит снятие фильтра. Гейты, +относящиеся к диффу, зелёные. Документация и трейлеры в порядке. + +**Вердикт: зелёный** + +--- + + + +## Материал раунда + +- Ветка: `issue/546-wait-round`, коммит `5fc596c748c5` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `2350e7169f6f98b97900e4aabaebd5ea47dfe492` + ``` + git log --all --format='%H %T' | grep 2350e7169f6f + ``` +- Тело issue: `0968fbd2f409d44f80911b445838422e0563d955aa08ab72109bdec5253ca3d5` +- Вердикт конвейера: `green` · High 0