diff --git a/scripts/loop/README.md b/scripts/loop/README.md index 7e68a2c..ff1af85 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -757,24 +757,31 @@ the first `auto` run is a verification step rather than a milestone. Summary, so the Actions log showed a route line and then nothing — a skip was indistinguishable from a silent failure. Every path (skip, inbox no-op, inbox-only, duplicate event, no-op route) now logs its reason. -- [ ] D26 (A1, open) A `pull_request.synchronize` on a PR task is skipped whenever - its state is not claimable, so a PR that gains a commit is never re-analysed - or re-reviewed. Two faces, both observed on real PRs: - - `waiting-merge`: a loop PR that receives a new commit is skipped instead of - re-reviewed, so `review_requested`-style re-entry has no path back. +- [x] D26 (A1) A `pull_request.synchronize` on a PR task was skipped whenever its + state was not claimable, so a PR that gained a commit was never re-analysed or + re-reviewed. Closed for the review path by `reviewReopenOf` (`shared/route.mjs`): + `synchronize` moves a `rejected` (request-changes) or `waiting-merge` (approved, + awaiting a human) task back to `ready` before the claim check, so a revised loop + PR is reviewed again — the only path back after the loop's own review, and the + one that let #82 receive fixes without a new task. The second face stays a + deliberate gap, recorded as D38. - `waiting-human`: **observed live on #39** — the first push opened the PR and triaged it to `waiting-human`; the second push logged `skip: task #39 not claimable (status=waiting-human)`. Every later push is skipped too, so the task's analysis is frozen at whatever the first - revision looked like. - - This is a real consequence of running at the report boundary: `flows` §2a - describes request-changes → `processing` → re-review on `synchronize`, but a - report-boundary `pr-review` cannot comment or relabel, so it can only land in - `waiting-human` — where nothing re-enters. Left open deliberately: A1's week - should quantify how often this bites (it will have hit every loop PR by then), - and the fix is a design choice — re-open the task on `synchronize` for PR - kinds, or add a bounded re-triage — not a one-line guard. + revision looked like. Unchanged on purpose: that state is the inbox, and a + push must not silently claim it (D29). + + The original note stands as the reason this took a design decision rather than a + guard exception: `flows` §2a describes request-changes → `processing` → + re-review on `synchronize`, but a report-boundary `pr-review` could only land in + a terminal state, where nothing re-entered. +- [ ] D38 (follow-up to D26) A PR task triaged into `waiting-human` still has no + re-entry: `synchronize` refuses it (D29), `issues.labeled` does not fire for + PRs, and `workflow_dispatch` hits the same claim check. Closing it needs a + deliberate human act *on the PR* — a label the router treats as a claim, or a + command comment — which is a design choice about the claim vocabulary, not + another exception in `claimability`. - [x] D27 (A1) `run.mjs`'s subcommands did not catch errors thrown by the shared library, so `pnpm loop start --task ` printed a full Node stack trace instead of a one-line reason — the library throws (correct for the diff --git a/scripts/loop/entry.mjs b/scripts/loop/entry.mjs index 303a35d..5155b6f 100644 --- a/scripts/loop/entry.mjs +++ b/scripts/loop/entry.mjs @@ -35,7 +35,9 @@ import { nowIso, outcomeAllowed, allowedOutcomes, describeActions, LOOP_BRANCH_RE, missingPrContractSections, } from './shared/state.mjs'; -import { route, eventKey, stageForIssue, humanClaimOf, CLAIM_LABEL } from './shared/route.mjs'; +import { + route, eventKey, stageForIssue, humanClaimOf, reviewReopenOf, CLAIM_LABEL, +} from './shared/route.mjs'; import { buildPrompt, runPi, parseEvents, summarize, classify } from './shared/agent.mjs'; import { runDoubleReview } from './shared/review.mjs'; @@ -306,6 +308,24 @@ async function doRun({ decision, ctx, repo, writeLevel }) { task.updatedAt = nowIso(); await saveTask(S, task); } + // A push on a loop PR re-opens the review (D26): both `rejected` and + // `waiting-merge` were terminal for `synchronize`, so a revised PR could never be + // reviewed again — the loop had no path back after its own review. Narrow: only + // those two states, only on a PR event. + const reopened = reviewReopenOf(ctx, task.status); + if (reopened) { + log(`review reopen: task #${task.id} ${task.status} → ready ` + + `(synchronize on ${task.kind} #${reopened})`); + task.timeline = [...(task.timeline ?? []), { + at: nowIso(), + event: 'reopened-for-review', + by: ctx.event.sender?.login ?? 'github', + detail: 'new commit on the loop PR: review may run again', + }]; + task.status = 'ready'; + task.updatedAt = nowIso(); + await saveTask(S, task); + } const claim = claimability(task); if (!claim.ok) { // A skip must be logged: during an event storm, a human must see at a glance in the diff --git a/scripts/loop/shared/route.mjs b/scripts/loop/shared/route.mjs index c16da18..8801129 100644 --- a/scripts/loop/shared/route.mjs +++ b/scripts/loop/shared/route.mjs @@ -98,6 +98,25 @@ export function humanClaimOf({ eventName, action, event = {} }, status) { return sender.login ?? null; } +/** + * A new commit on a loop PR re-opens its review (D26). `pr-review` leaves the task in + * `rejected` (request-changes) or `waiting-merge` (approved, awaiting a human), and + * both were terminal for the event: every later `synchronize` logged "not claimable", + * so a revised PR could never get a second look. That is the loop's only path back + * after a review, so it is not a nicety. + * + * Narrow on purpose — `pull_request.synchronize` only (opened is already claimable), + * and only those two states. `waiting-human` PR tasks keep D29's guard: they are in + * the inbox because a human has to decide, and a push must not silently claim them. + * + * @returns {number|null} the PR number whose task may be claimed for review. + */ +export function reviewReopenOf({ eventName, action, event = {} }, status) { + if (eventName !== 'pull_request' || action !== 'synchronize') return null; + if (!['rejected', 'waiting-merge'].includes(status)) return null; + return event.pull_request?.number ?? null; +} + /** * Idempotency key: skip an event whose key has already been consumed. * Prefer immutable identifiers (PR head sha / comment id), falling back to the