From cdfc6e59dcf80f5b0a924775b95b31e79248744b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 16:44:24 +0000 Subject: [PATCH] docs: review document for #775 Issue: #775 User-Visible: no --- docs/reviews/CODE-REVIEW-775-r1.md | 205 +++++++++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-775-r1.md diff --git a/docs/reviews/CODE-REVIEW-775-r1.md b/docs/reviews/CODE-REVIEW-775-r1.md new file mode 100644 index 00000000..94c8b4ab --- /dev/null +++ b/docs/reviews/CODE-REVIEW-775-r1.md @@ -0,0 +1,205 @@ +# CODE-REVIEW-775-r1 + +Issue: #775 — «Конвейер: "Разбудить раунд" после Validate отвечает noop — задачи в S7 зависают без ревью» +Трек: show · этап: code · заход: r1 · блокирующих циклов: 0/2 +Материал: `6bece2517939542be56c73bf8ee0703aebb2635c` (= HEAD, = origin/dev + 1 коммит) +Класс: B (gates/tooling) + C (PROCESS.md) — инфраструктурная задача, без файлов класса A. + +## Скоуп + +01.10 три задачи (#740, #744, #748) застряли в `S7-code-review`: Validate на +материале завершился, а `process-resume.mjs` ответил `noop`, ревью не +запустилось, обход — ручная перестановка метки. Issue называет три гипотезы +(окно в 200 прогонов `processRuns`, перепутанный «последний» прогон после +снятия/установки метки в одну секунду, маркер прошлого раунда после ребейза) и +требует: resume находит именно прогон с маркером, а не просто последний; окно +не зависит от чужой разметки; красный Validate всегда возвращает задачу в S6 с +комментарием. Свидетели — юниты `decideResume`/`processRuns` на этих трёх +историях. + +Единственный коммит на ветке правит `scripts/process-reconcile.mjs`, +`scripts/process-resume.mjs`, `.github/workflows/_process-resume.yml`, +`scripts/mutation-registry.mjs`, `PROCESS.md` и добавляет +`test/process-pending-round.test.mjs`; правит `test/process-resume.test.mjs` +под новую сигнатуру. Продуктовый код (`src/**`, `custom_components/**`) не +тронут. Трейлеры коммита: `Issue: #775`, `User-Visible: no` — верно: правка не +меняет ничего, что видит пользователь карточки, changelog не нужен. + +**Применимость критериев §5 (route):** +- complexity — умеренная (переписана выборка истории, идентичность маркера, + двойная проверка перед записью), риск смягчён 32 регрессионными тестами, + три из них — прямой повтор трёх реальных инцидентов по их идентичностям + (id прогонов, SHA, время). ≤3, проходит. +- surfaces — один модуль: пара resume/reconcile процесса ревью. Проходит. +- migration — пайплайн уже писал `branch`/`validate_run_id`/`run_attempt`/ + `stage` в `pending.json` до этой задачи (`.github/workflows/_process.yml:792-811`, + файл не менялся в этом диффе); правка лишь ужесточает проверку уже + существующих полей. Новых полей конфигурации или совместимости нет. Проходит. +- ux-contract / perf-touch — не применимо, пайплайн не рендерит карточку и не + трогает ввод. Проходит. +- undocumented — поведение зафиксировано в самом PROCESS.md тем же коммитом + (раздел «Поиск ожидающего раунда (#775)», после «весь реестр проверяет + ночь…») и в теле issue («Что нужно»). Проходит. + +Все критерии §5 пройдены → `route: fix`. + +## Как проверялось + +Дешёвые гейты на `6bece251` уже зелёные в Validate +(https://github.com/Matysh/houseplan-card/actions/runs/36892663617) — `npx tsc +--noEmit`, `npm test`, `npm run build` + сверка бандла не перегонялись +повторно (#343). + +Прогнано самим ревьюером (дополнительно к Validate, т.к. задача про сам +пайплайн ревью — проверил своими руками, а не только по отчёту автора): + +| Гейт | Команда | Результат | +|---|---|---| +| Целевые юниты по issue | `node --test test/process-pending-round.test.mjs test/process-resume.test.mjs` | 19/19 зелёных | +| Юниты reconcile (не изменена сигнатура публичных функций, но логика общая) | `node --test test/process-reconcile.test.mjs` | 13/13 зелёных | +| Консистентность дайджеста ревьюера с PROCESS.md (правка в этом же файле) | `node --test test/process-digests.test.mjs` | 6/6 зелёных | +| Статическая целостность реестра мутантов (новые 4 патча накладываются на реальный текст) | `node --test test/mutation-gate.test.mjs` | 69/69 зелёных | +| Выбор смоков по диффу | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет» — смоки не выбираются, диф не трогает `src/**` | + +Итого 19+13+6+69 = 107 юнит-тестов, все зелёные; независимо подтверждает +заявленные автором «32 целевых теста» (19 в process-pending-round/process-resume ++ 13 в process-reconcile = 32). + +Дополнительно ручной разбор: +- Прочитан полный `scripts/process-reconcile.mjs` и `scripts/process-resume.mjs` + целиком (не только дифф) — проверено чтением, не исполнением, как + изменённые функции сочетаются с неизменными (`decideReconciliation`, + `applyReconciliationDecision`). +- Сверено текстом, что четыре новых мутанта в `scripts/mutation-registry.mjs` + (`pending-history-latest-200-only`, `pending-revives-previous-request`, + `pending-resumes-another-validate`, `pending-no-fresh-snapshot-before-write`) + действительно патчат уникальные строки текущего (постдиффового) кода — все + четыре `find`-строки найдены ровно один раз в целевых файлах (grep). +- Прочитан нетронутый этим диффом участок `.github/workflows/_process.yml` + (шаги «Validate на материале» / «Validate идёт — раунд продолжит событие» / + «Validate красный — вернуть автору без ревью», строки 748–857) — подтверждён + заявленный в комментарии автора факт: red-path в S6 с комментарием уже + существовал ДО этой задачи и не требовал правки; баг был именно в том, что + resume не давал этому пути шанс сработать повторно. + +## AC и способ доказательства + +Issue не содержит формального раздела `## ТЗ` (инфраструктурная задача, трек +show, спецификация — текст issue), но называет три проверяемых требования и +явно требует юнит-свидетелей на трёх реальных историях. + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| Resume находит прогон с маркером, не просто «последний» | `test('#775 all three incident markers resume…')` реплицирует точные идентичности (issue, run id, validate id, sha, время, исход) трёх зависших задач и утверждает `action === 'resume'`; разобрано чтением — `reviewRunsForRequest` теперь ограничивает выборку `at(run.createdAt) >= at(request.at)`, что устраняет оба сценария из гипотез issue (события `unlabeled` не порождают `request`, т.к. `latestReviewRequest` фильтрует только `event === 'labeled'`; ребейзный «старый» прогон до переразметки не попадает в окно) | Мутант `pending-revives-previous-request` (сдвигает границу на −120с, реестр `scripts/mutation-registry.mjs`) | +| Окно истории не зависит от чужой разметки (было: 200 последних прогонов `process.yml`) | `test('#775 history is paged to the request, not capped at 200 or filtered-search 1000 runs')` — 1100 посторонних прогонов для issue #999 не мешают найти нужный; `calls.length === 12` страниц (постранично до `oldest`), `url` без `event=` (без серверного фильтра с потолком 1000) | Мутант `pending-history-latest-200-only` (возвращает предел в 2 страницы) | +| Красный Validate всегда возвращает задачу в S6 с комментарием | Код шага «Validate красный — вернуть автору без ревью» (`_process.yml:826-857`) не изменён этим диффом и безусловен по `steps.gate.outputs.proceed == 'false'` — проверено чтением, не исполнением. `decideResume` не фильтрует по `conclusion` Validate (только `event`/`status`/`headSha`), и инцидент #744 (`conclusion: 'failure'`) воспроизведён в том же параметризованном тесте с ожиданием `resume` | Явного мутанта на «resume не должен отказывать по conclusion» в реестре нет — проверка идёт через отсутствие фильтра в коде, подтверждена только прочтением; граница ответственности этого диффа — довести до relabel, а не до интерпретации red/green (это не менялось) | +| Не терять событие при временно неполной выдаче (до 3 снимков, паузы 5с) | `test('#775 transient missing/old/no-marker snapshots recover…')`: `waits = [5000]`; `test('#775 absent evidence gets a bounded grace…')`: `reads = 3`, `waits = [5000, 5000]` — числа точно совпадают с текстом PROCESS.md-правки («не более трёх снимков с паузами по 5 секунд») | Нет отдельного мутанта на счётчик попыток; граница проверена только тестом, не реестром | +| Перед перестановкой метки состояние перечитывается (TOCTOU) | `test('#775 write path rechecks current request, stop labels, head and runs; duplicates cannot relabel twice')` — 4 варианта изменения состояния между чтениями все дают `applied: false, writes: 0`; второй прогон той же задачи после успешного relabel тоже `applied: false` | Мутант `pending-no-fresh-snapshot-before-write` (вторая проверка подменяется первой) | + +Третья строка таблицы (красный Validate → S6) формально не закрыта +собственным мутантом в реестре этой задачи — это не недоработка диффа: сам +red-path не входит в его изменяемую поверхность (код не трогался), а то, что +diff проверяет (`decideResume` не фильтрует по conclusion), доказано чтением +кода и тестом-инцидентом #744. Не считаю это пустой третьей колонкой в +смысле §2.7, т.к. объект защиты («resume не блокирует red Validate») — не то +же самое, что объект мутации («red Validate переводит в S6») — последний не +менялся и не нуждается в новом мутанте. + +## Находки + +Нет High. Нет Medium (ни в скоупе, ни вне скоупа). + +Два наблюдения Low, не блокируют и не открывают цикл (track show, +«бухгалтерия» по REVIEWER.md): + +1. `decideResume` в ветке `!sha || headSha !== sha` (scripts/process-resume.mjs:61) + возвращает терминальный `noop` без `recheck: true`, в отличие от соседних + веток «не нашли прогон/маркер». Если чтение текущего SHA ветки через + `git/ref/heads/` окажется на мгновение неконсистентным (крайне + маловероятно для простого ref-запроса), событие потеряется до планового + `process-reconcile`. Не блокирует: reconcile — штатная страховка именно на + этот случай, а ref-API не входит в число источников, для которых авторы + уже наблюдали задержку (это были списки прогонов и артефакты). +2. `processRuns` (scripts/process-reconcile.mjs:240-267) использует offset- + пагинацию (`page=N`) без курсора; при параллельном создании новых прогонов + `process.yml` во время обхода возможен классический дрейф страниц (пропуск + или дублирование элемента на границе). Дубликаты дедуплицируются по + `id/attempt`; пропуск по границе проявился бы как «process history + pagination made no progress» — громкий отказ, а не тихая потеря. Это не + регрессия: офсетная пагинация тем же способом уже использовалась в коде до + #775 (`[1,2].flatMap(...)`), просто без ограничения в 2 страницы. Не + блокирует. + +## Что проверено и корректно + +- Корень всех трёх гипотез issue закрыт структурно, а не патчем под частный + случай: `latestReviewRequest` берёт только `labeled`-события (гипотеза 2 про + `unlabeled`-прогон снята на уровне типа события, не на уровне эвристики); + `reviewRunsForRequest` режет историю по времени текущего запроса метки, что + одновременно решает «перепутанный latest после ребейза» (гипотеза 3) и + убирает нужду в ограничении размера окна, раз оно больше не читает чужие + issue вообще (гипотеза 1 снята инженерно, не обходом). +- `pendingEvidenceError` ужесточает проверку маркера (issue/run_id/run_attempt + /stage/branch-префикс/формат sha и validate_run_id) — все поля уже писались + пайплайном до этой задачи (`_process.yml:792-811`, не изменён), значит это + не миграция формата, а более строгая проверка существующих данных; разрыва + совместимости с уже лежащими в Actions старыми `pending.json` быть не + должно, т.к. они писались тем же неизменным шагом. +- `resume()` перед записью перечитывает состояние целиком (`io.state()` + + `io.runs()`), сверяет id запроса, id/attempt прогона — honest TOCTOU-защита, + а не формальная галочка (тест явно ловит смену запроса/прогона между + чтениями). +- `pendingOf` в реальных `ops` отдельно отказывает, если уже существует + `review-result-*` — защита от двойной траты модели даже если бы + `decideResume` её не поймал. +- CLI (`process-resume.mjs` as main) и вызывающий workflow + (`_process-resume.yml`) согласованы: новый обязательный `--run-id` появился + в обоих местах одним коммитом, тест `test/process-resume.test.mjs` сверяет + текст workflow-файла регэкспом на оба изменения. +- `User-Visible: no` обоснован: диффне меняет ничего, что видит пользователь + карточки; «одно число — один источник» (§8) не применимо — в диффе нет + пользовательских величин вообще. +- Трек show подтверждён на issue (`track:show` label), все критерии §5 + пройдены (см. «Скоуп»), эскалация/reclassify не нужны. + +## Чего не проверял + +- Не воспроизводил вживую реальное событие `workflow_run` / доставку webhook + GitHub — это прямо исключено и самим автором («живую доставку будущего + события до слияния этого исправления доказать нельзя»); единственная + доступная проверка — юнит-уровень на точных идентичностях инцидентов, она + сделана. +- Не гонял мутационные прогоны (ни старые, ни новые четыре) — на треке show в + разработке это не гейт ревью (§2.7, #709); проверил статически, что все + четыре `find`-строки патчей существуют и уникальны в целевых файлах, и что + `test/mutation-gate.test.mjs` (реестр целостности) зелёный. +- Не проверял нагрузочно офсетную пагинацию `processRuns` на реальном + GitHub API при высокой конкурентной активности (много параллельных меток в + репозитории) — см. Low-наблюдение 2; сочтено приемлемым остаточным риском, + унаследованным от уже существующего подхода, а не внесённым этой задачей. +- Golden/визуальные гейты, инварианты модели, pytest backend, performance — + не применимы: diff не трогает `src/**`, рендер, геометрию или Python. +- Не читал историю Actions-запусков трёх реальных инцидентов (#740/#744/#748) + напрямую через `gh api` (не было доступа запрашивать живые GitHub Actions + логи сверх уже предоставленных в issue ссылок) — доверился идентичностям + (run id, validate id, sha, время), процитированным в issue и воспроизведённым + в фикстуре теста; они взаимно согласованы (issue-таблица ↔ тест ↔ код). + +## Вердикт + +Зелёный. High: 0, Medium: 0. Route: fix (все критерии §5 пройдены). + +--- + + + +## Материал раунда + +- Ветка: `issue/775-resume-pending-round`, коммит `6bece2517939` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `5a1dba2cd87b12c5fa82dbc7342a1e8fa0777d47` + ``` + git log --all --format='%H %T' | grep 5a1dba2cd87b + ``` +- Тело issue: `bf0fc310f0258fb8df339bab23938f022ff6b529c332fc038bf8698c8c7b6b0f` +- Вердикт конвейера: `green` · High 0 · маршрут `fix` +