From e66e82fc86f72c51d2f5e47dcccee081ca7db139 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 13 Sep 2026 12:43:51 +0000 Subject: [PATCH] docs: review document for #555 Issue: #555 User-Visible: no --- docs/reviews/CODE-REVIEW-555-r1.md | 242 +++++++++++++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-555-r1.md diff --git a/docs/reviews/CODE-REVIEW-555-r1.md b/docs/reviews/CODE-REVIEW-555-r1.md new file mode 100644 index 00000000..2e3ace38 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-555-r1.md @@ -0,0 +1,242 @@ +# CODE-REVIEW #555 · заход r1 + +Материал: `e3bf893e3d76b3276a548ee9182f073c901a12e9` (HEAD, рабочая копия на нём). +Диапазон: `git diff origin/dev...HEAD` (один коммит, `ci: восстанавливать +потерянные запросы ревью (#555)`, `Issue: #555`, `User-Visible: no`). + +## Скоуп + +Инфраструктурная задача (`infra`, `process`, `tech-debt`, без продуктового S* +на входе — по AGENTS.md это ожидаемо). Из аудита 12.09 (§7.2, остаток I3): +конвейер `process.yml` (#499) убрал нерелевантные label-события, но не +гарантирует, что S4/S7 после `cancelled`/`timed_out`/отказа guard приходят в +понятное конечное состояние. Задача добавляет bounded reconciler — снимок +раз в 30 минут, не polling. + +Изменённые файлы: `.github/workflows/process-reconcile.yml` (новый workflow), +`.github/workflows/process.yml` (+1 строка `run-name` — стабильная идентичность +прогона по issue+label), `scripts/process-reconcile.mjs` (новый, 399 строк), +`scripts/mutation-gate.mjs` (+5 мутантов), `test/process-reconcile.test.mjs` +(новый, 8 тестов), `test/review-doc-guard.test.mjs` (+1 тест на согласованность +документации/workflow), `AGENTS.md` + `PROCESS.md` (описание механизма). +`src/**` не тронут — продукт и его контракты не затрагиваются. + +Проверка входа (Rule #1 неприменимо буквально — это infra-задача, вход по +AGENTS.md — явное решение владельца применить `S7-code-review`, что и стоит на +issue). Трейлеры корректны: `Issue: #555`, `User-Visible: no` — согласуется с +тем, что changelog не менялся и менять было незачем. + +## Как проверялось + +- Прочитан `scripts/process-reconcile.mjs` целиком построчно: `parseProcessRun`, + `latestReviewRequest`, `preparedEvidenceError`, `decideReconciliation`, + `reconciliationKey`/`markerFor`/`alreadyReported`, и — отдельно, так как это + единственный слой без юнит-тестов — императивная обвязка `gh()`, `relabel()`, + `addComment()`, `snapshot()`, `reconcileAll()`. +- Прочитан `.github/workflows/process-reconcile.yml` и изменение `run-name` в + `process.yml` целиком; сверено, что `RUN_TITLE`-регэксп в скрипте + соответствует формату `run-name` буквально (issue.number → label.name → + issue.title через ` · `). +- Прочитаны оба новых теста и добавленный тест в `review-doc-guard.test.mjs` + построчно, сопоставлены с кодом и текстом документов. +- Прогнан целевой `node --test test/process-reconcile.test.mjs` — 8/8 green + (см. «Гейты»). +- Сверены все 5 новых `MUTANT_DEFINITIONS` в `scripts/mutation-gate.mjs`: + каждый `find` дословно встречается ровно один раз в + `scripts/process-reconcile.mjs`, и для каждого патча прослежено по коду, что + соответствующий тест в `test/process-reconcile.test.mjs` обязан покраснеть + (см. разбор по AC ниже) — без пересборки мутационного гейта (дорогой, ночной, + §513, не гейт code review). +- Прочитаны тело issue #555 и единственный комментарий автора (перечень + проверок и SHA). + +## Находки + +### [High] `relabel()` может уронить весь прогон и никогда не эскалировать именно тот случай, ради которого задача заведена + +`scripts/process-reconcile.mjs:502-511`: + +```js +function relabel(repo, issue, label) { + gh(['issue', 'edit', String(issue.number), '--repo', repo, '--remove-label', label]); + const added = gh(['issue', 'edit', String(issue.number), '--repo', repo, '--add-label', label], { allowFailure: true }); + if (added.status !== 0) { + // Best-effort rollback: leaving an issue without its review state is worse + // than a loud failed reconciliation. + gh(['issue', 'edit', String(issue.number), '--repo', repo, '--add-label', label], { allowFailure: true }); + throw new Error(`could not restore ${label}: ${(added.stderr || '').trim()}`); + } +} +``` + +Второй вызов `gh(...)` (заявленный «best-effort rollback») результата не +проверяет: `throw` происходит безусловно на основании `added` — результата +ПЕРВОЙ попытки — даже если вторая попытка восстановила метку успешно. Это само +по себе неверно (лишний шум), но хуже цепочка последствий: + +- `relabel()` вызывается внутри `reconcileAll()` (`process-reconcile.mjs:354`) + без единого `try/catch` вокруг записи (проверено: `grep -n "try\|catch"` — + единственные `try` в файле относятся к `preparedEvidenceError`/ + `hydrateRunEvidence`, не к этому месту). +- Исключение всплывает из `reconcileAll()` в `.then()/.catch()` на + `process-reconcile.mjs:391-398`: `.then()` (который пишет + `--output=summary.json`) не выполняется, отрабатывает только `.catch()`, + `process.exitCode = 2`. +- В workflow `process-reconcile.yml` шаг `upload-artifact` идёт следующим шагом + без `if: always()` — при ненулевом коде шаг `run:` фейлится, `upload-artifact` + не выполняется вовсе. Итог прохода: ни machine-readable summary (AC5), ни + обработки остальных issue в очереди этого тика. +- Критично для AC "намеренно остановленная работа не возобновляется" и общей + цели issue: `addComment()` (запись дедуплицированного диагностического + комментария) стоит в коде ПОСЛЕ `relabel()` и в этой ветке не выполняется + никогда. Значит `alreadyReported()` не увидит маркер, и при следующем тике + (через 30 минут) `decideReconciliation` примет то же решение `retry` для того + же события — реконсилер наступит на тот же `relabel()` заново. Если причина + отказа `--add-label` не самоустранилась (метка удалена, право токена, + устойчивый rate-limit), это бесконечный цикл: workflow красный каждые 30 + минут, ни одного комментария в issue не появляется. +- Это ровно тот класс отказа, который задача обязана закрыть: «Failure guard, + … получают один дедуплицированный диагностический комментарий и эскалацию + человеку» (PROCESS.md, добавлено этим же коммитом) — для отказа самого + восстановления такой эскалации нет вообще. + +Тестами не покрыто: `relabel`, `addComment`, `snapshot`, `reconcileAll` не +экспортированы и не встречаются в `test/process-reconcile.test.mjs` (проверено +`grep -n "export function\|export async function" scripts/process-reconcile.mjs` +против списка импортов теста) — вся императивная запись проверена только +чтением кода, как и требует протокол этого этапа. Ручной прогон автора тоже не +покрывает эту ветку: «read-only CLI на живой очереди — green, records=0, +writes=0» — это прогон с `apply=false` (или без reconciliation-действий), +путь `relabel()` не исполнялся ни разу ни в тесте, ни вручную. + +Воспроизведение (по коду, не по запуску): вызвать `relabel(repo, issue, label)` +в ситуации, когда первый `gh issue edit --add-label` вернёт `status !== 0` +(транзиентная ошибка GitHub API/rate limit сразу после `--remove-label`, что в +CI не экзотика) — независимо от исхода второй попытки, функция бросает +исключение и завершает `reconcileAll()` без записи артефакта и без +диагностического комментария. + +**Это находка в скоупе задачи**: файл и функция — часть этого же diff, чинится +там же. Из-за High это блокирующий вердикт данного захода. + +Минимальное исправление (не мой мандат его вносить, только диагноз): проверять +результат именно повторной попытки, а не переиспользовать `added`; и/или +оборачивать применение решения на уровне `reconcileAll()` в `try/catch` на +issue, чтобы отказ одного issue не ронял снимок остальных и не блокировал запись +`summary.json`. + +## Проверено и корректно + +- **Матрица решений `decideReconciliation`** (AC1) — для всех перечисленных в + issue фикстур (cancellation, guard failure/`failure`, timeout, + `startup_failure`, потерянное событие, уже применённый verdict, ещё идущий + run) выбирается правильное действие; прочитано построчно и подтверждено + зелёным целевым прогоном (см. «Гейты»). +- **Дедупликация и запрет повторного расхода цикла** (AC2, частично) — чистая + часть: `reconciliationKey`/`markerFor`/`alreadyReported`, + `retryAlreadyIssued` (не более одного авто-relabel на потерянное событие) — + проверены тестом `#555 repeat reconciliation has a stable dedupe marker` и + чтением; логика корректна. Применение чужого stage/material отклоняется + `preparedEvidenceError` (`badIdentity`) — проверено тестом и подтверждено + соответствующим mutation-witness (`process-reconcile-accepts-foreign- + prepared-evidence`), прослежено по коду, что мутация переводит исход `retry` + вместо ожидаемого в тесте `escalate` — тест обязан покраснеть. +- **Гонка между чтением и записью** (AC3, частично) — `reconcileAll` перед + мутацией перечитывает issue/events/runs (`snapshot(repo, freshRuns, + first.issue)`) и сверяет `confirmed.action`/`confirmedKey` с исходным + решением; при расхождении действие не применяется (`record.action = 'noop'`). + Логика прочитана и корректна как таковая — сама операция записи (см. High) + ненадёжна, но защита от протухшего решения — да. +- **Здоровый running run не перезапускается** (AC4) — `age <= activeLimitMs` → + `wait`; подтверждено тестом и mutation-witness + (`process-reconcile-restarts-healthy-run`: мутация `wait`→`retry` красит тест + «running waits»). +- **`blocked`/`review-4` сильнее старого события** (AC3/AC4) — проверено тестом + и mutation-witness (`process-reconcile-ignores-owner-stop`): при `if (false + && …)` тест `issue(['S7-code-review','blocked'])` перестаёт быть `noop`. +- **Sealed model result не переигрывается** — `run.resultArtifact` → + `escalate`, тест + witness (`process-reconcile-reruns-sealed-model-result`) + подтверждают, что без проверки конклюзия ушла бы в `retry`. +- **Один авто-relabel, не бесконечный** — `retryAlreadyIssued` + witness + (`process-reconcile-retries-lost-event-forever`) подтверждают переход + `retry`→`escalate` после первого отмеченного повтора — сама детекция + корректна (описанная в High проблема — это отсутствие записи маркера в одном + конкретном отказном пути, а не дефект самой проверки). +- **Стабильная идентичность прогона** — `RUN_TITLE` + (`/^process #(\d+) · (S4-spec-review|S7-code-review)(?: ·|$)/`) сверена + буквально с новым `run-name` в `process.yml`; `on: issues: types: [labeled]` + — единственный триггер этого workflow (прочитан файл целиком), значит + `github.event.label.name` всегда заполнен и regex всегда применим к свежим + прогонам. Легаси-путь по совпадению `issue.title` (для прогонов до #555) + безопасно вырождается в `null` при неоднозначности (`matches.length !== 1`). +- **Workflow `process-reconcile.yml`** — `cron: '7,37 * * * *'` (раз в 30 минут, + не polling одинакового состояния на каждый тик, а один снимок и выход), + `concurrency: group: process-reconcile, cancel-in-progress: false` не даёt + двум прогонам работать параллельно, `timeout-minutes: 10` ограничивает + зависший прогон, `permissions:` минимальны для `GITHUB_TOKEN` (сам скрипт + везде использует `GH_TOKEN: secrets.HP_PROCESS_TOKEN`, как и остальной + `process.yml` — сверено по существующему паттерну). +- **Трейлеры и класс изменений** — `Issue: #555`, `User-Visible: no` корректны; + `src/**` не тронут, changelog не требовался. +- **Тест `test/review-doc-guard.test.mjs` (#555)** — построчно сверен с текущим + содержимым `PROCESS.md`/`AGENTS.md`/обоих workflow-файлов; все проверяемые + фрагменты присутствуют дословно. + +## Чего не проверял и почему + +- **`npx tsc --noEmit`, `npm test` (полностью), `npm run build`+сверка трёх + копий бандла** — не гонял повторно: Validate на этом же SHA `e3bf893e` + зелёный (https://github.com/Matysh/houseplan-card/actions/runs/34757318234), + код с тех пор не менялся (материал ревью = вершина без исполненного + ребейза). Прогнал только точечно `node --test test/process-reconcile.test.mjs` + (8/8 green) — дешёвая точечная проверка нового теста, не замена полного + набора. +- **`node scripts/check-docs.mjs`** — не гонял: диапазон не трогает `src/**`, + отпечаток скриншотов документации не мог устареть. +- **`npm run invariants -- --config …`** — не гонял: диапазон не трогает + геометрию, толщину стен, `layout`, `marker.space`, `open_spans` — модель + вообще не затронута. +- **Браузерные смоки `demo/smoke_*.mjs`, `npm run golden:verify`, + `python -m pytest tests_backend`, performance-профили** — не гонял и не + выбирал по `scripts/smoke-select.mjs`: diff не трогает `src/**`, `demo/**` + (кроме несуществующего — фактически не тронут), `custom_components/**/*.py` + не менялся, AC задачи ни один из этих гейтов не называет. Это гейты + визуального/HA-контракта продукта, а изменение — чисто процессный скрипт и + workflow. +- **Полный ночной `scripts/mutation-gate.mjs` прогон (пересборка бандла на + каждый мутант)** — не гонял: это дорогой, отдельно расписанный (§513) гейт + вне цикла ревью. Вместо него по каждому из 5 новых мутантов проверено по + коду точное совпадение `find`-паттерна и прослежена логика, почему + соответствующий тест обязан покраснеть — см. «Проверено и корректно». +- **Живой прогон `process-reconcile.mjs --apply=true` на реальной очереди + issues** — не выполнял (ручного тестирования в этом цикле нет, а + «доказательство» такого прогона потребовало бы мутировать реальные метки/ + комментарии чужих issue, что вне полномочий ревью). Именно отсутствие любого + исполнения (тестового или ручного) ветки `relabel()`/`addComment()`/ + `reconcileAll()` — основание High-находки выше. + +## Итог + +Один High, найден чтением непокрытого тестами кода записи (`relabel()` +безусловно бросает исключение независимо от исхода собственного «best-effort +rollback», что роняет весь прогон реконсилера и — что хуже — пропускает именно +тот единственный диагностический комментарий, который должен предотвратить +бесконечный тихий повтор одного и того же отказа). Находка в скоупе задачи и +не требует отдельного issue — правится в этой же ветке. Остальная (доминирующая +по объёму) логика — чистая функция `decideReconciliation` и её вспомогательные +функции — вычитана, протестирована точечным прогоном и подтверждена всеми +пятью новыми mutation witnesses. + +--- + + + +## Материал раунда + +- Ветка: `issue/555-process-reconciler`, коммит `e3bf893e3d76` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `9ddf11012bb0a1d9657e3801bb856e32a6cca73f` + ``` + git log --all --format='%H %T' | grep 9ddf11012bb0 + ``` +- Тело issue: `61feb887bc5d5cae5bac106b48e7f07161e697938ac473cc87c5e47a8ca8d4e6` +- Вердикт конвейера: `red` · High 1