chore(loop): re-open a loop PR's review when a new commit arrives

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 <missing>` printed a full Node stack
       trace instead of a one-line reason — the library throws (correct for the

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

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