From f9b827832f0cd59636023c701618646ba24f3055 Mon Sep 17 00:00:00 2001 From: Alex Chao Date: Fri, 11 Sep 2026 03:30:43 +0800 Subject: [PATCH] chore(loop): terminalise the task of a non-loop PR when it closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D33. `pull_request_target` returned `noop` for any PR without `loop-task:`, which is right about the **metric** (not loop-produced ⇒ never in the acceptance rate) but wrong about the **task**: ops.md maps "(none, merged) -> accepted" and "(none, closed unmerged) -> rejected" regardless of provenance. Every owner or dependabot PR gets a task from `pull_request` triage, so each merged PR left a zombie in `waiting-human` — observed live on #39, the very PR that fixed D30. It shows up under "Inbox" in SUMMARY.md forever, and sweep cannot repair it: sweep lists *open* GitHub items, so it cannot see a closure. The router now emits a `terminal` decision for that case, and entry handles it through the same `markTaskTerminal` as the metrics path — one definition of "what terminal means" — while still never writing an acceptance row for a non-loop PR. Verified: owner PR merged -> accepted and owner PR closed-unmerged -> rejected, neither writing a metric; a loop PR still does both (new row + transition); duplicate events add no row and re-transition nothing; a PR with no task does not throw; plus the 19-case route table and the CLI/orchestrator suite. --- scripts/loop/README.md | 14 ++++++++++++ scripts/loop/entry.mjs | 26 +++++++++++++++++++++- scripts/loop/shared/route.mjs | 42 ++++++++++++++++++++++++++--------- 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/scripts/loop/README.md b/scripts/loop/README.md index e3552a2..502e16c 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -89,6 +89,7 @@ GitHub event ─▶ loop.yml (concurrency group `loop` = platform-level single w | same-repo PR, other branches | run(triage) — PRs are a triage surface | | `pull_request_review` on a loop PR | run(pr-review) | | `pull_request_target` closed (loop PR) | metrics: merged / closed-unmerged **and** the task moves to `accepted` / `rejected` | +| `pull_request_target` closed (other PR) | task → `accepted` / `rejected` only; **no metric** (not loop-produced) | | `release` published | metrics: released | | `schedule` daily / weekly | system run: sweep / retro-scheduled | | `workflow_dispatch` | run per inputs (`task`/`stage`); `execute_writes=true` = L2 rehearsal | @@ -510,6 +511,19 @@ was *correct*, and whether new events produce proposals that match reality. the row through the existing `writer` field (`writer: "sweep-corrected"`), so dedup still matches. Recorded now because the ambiguity is in the norms layer, and whoever implements the backfill will read that sentence first. +- [x] D33 (A1) A non-loop PR closing left its task stranded. `pull_request_target` + returned `noop` for any PR without `loop-task:`, which is right about the + **metric** (not loop-produced ⇒ never in the acceptance rate) but wrong + about the **task**: ops.md maps "(none, merged) → accepted" and "(none, + closed unmerged) → rejected" regardless of provenance. Every owner or + dependabot PR gets a task from `pull_request` triage, so each merged PR left + a zombie in `waiting-human` — the task for the very PR that fixed D30 (#39) + demonstrated it live — visible forever under "Inbox" in SUMMARY.md and + uncorrectable by sweep, which lists *open* GitHub items and so cannot see a + closure. The router now emits a `terminal` decision for that case; the + entry handles it through the same `markTaskTerminal` as the metrics path, so + "what terminal means" has one definition, and the acceptance row is still + never written for a non-loop PR. ## Environment facts diff --git a/scripts/loop/entry.mjs b/scripts/loop/entry.mjs index 1ff2c20..4d73726 100644 --- a/scripts/loop/entry.mjs +++ b/scripts/loop/entry.mjs @@ -177,6 +177,28 @@ async function handleMetrics({ decision }) { return { state: 'metrics', wrote, row, transition }; } +/* --------------------------------------------------------------- terminal */ + +/** + * Terminalise a task with no metric attached — a PR that closed was not + * loop-produced, so it must not enter the acceptance rate, but its task still has + * to finish. Shares `markTaskTerminal` with the metrics path so there is one + * definition of "what terminal means". + */ +async function handleTaskTerminal({ decision }) { + const transition = await markTaskTerminal(S, decision.taskId, decision.to, { + event: decision.to === 'accepted' ? 'merged' : 'closed-unmerged', + detail: `${decision.pr ? `PR #${decision.pr} ` : ''}${decision.reason}`, + }); + if (transition.changed) { + log(`task #${decision.taskId}: ${transition.from} → ${decision.to} (${decision.reason})`); + await renderStateSummary(S); + } else { + log(`task #${decision.taskId}: no transition needed (${transition.reason})`); + } + return { state: 'terminal', transition, note: decision.reason }; +} + /* ------------------------------------------------------------------ run */ async function doRun({ decision, ctx, repo, writeLevel }) { @@ -394,10 +416,12 @@ async function main() { log(result.wrote ? `metrics written: ${JSON.stringify(result.row)}` : 'metrics already present, skipped (idempotent)'); + } else if (decision.action === 'terminal') { + result = await handleTaskTerminal({ decision }); } await writeStepSummary(renderStepSummary({ decision, result, writeLevel })); - return 0; // noop / inbox-only / metrics / a completed run all exit successfully + return 0; // noop / inbox-only / metrics / terminal / a completed run all exit successfully } main() diff --git a/scripts/loop/shared/route.mjs b/scripts/loop/shared/route.mjs index 9e761c6..de023c0 100644 --- a/scripts/loop/shared/route.mjs +++ b/scripts/loop/shared/route.mjs @@ -22,6 +22,11 @@ const noop = (reason) => ({ action: 'noop', reason }); const run = (o) => ({ action: 'run', ...o }); const metrics = (o) => ({ action: 'metrics', ...o }); const inbox = (o) => ({ action: 'inbox', ...o }); +/** + * Move a task to a terminal state without recording metrics — for closures that + * must not count toward the acceptance rate because the PR was not loop-produced. + */ +const terminal = (o) => ({ action: 'terminal', ...o }); /** * Loop PR test: head branch `loop/-*` and body containing `loop-task: #` @@ -121,18 +126,33 @@ export function route({ eventName, action, event = {} }) { case 'pull_request_target': { const pr = event.pull_request ?? {}; if (action !== 'closed') return noop(`pull_request_target.${action} has no Loop action`); - const loopTask = loopTaskOf(pr); - if (!loopTask) return noop('closing a non-loop PR is not counted for auto-acceptance'); const merged = Boolean(pr.merged); - return metrics({ - reason: `loop PR ${merged ? 'merged' : 'closed-unmerged'}`, - metrics: { - event: merged ? 'merged' : 'closed-unmerged', - taskId: loopTask, - pr: pr.number, - accepted: merged, - writer: 'workflow', - }, + const loopTask = loopTaskOf(pr); + if (loopTask) { + return metrics({ + reason: `loop PR ${merged ? 'merged' : 'closed-unmerged'}`, + metrics: { + event: merged ? 'merged' : 'closed-unmerged', + taskId: loopTask, + pr: pr.number, + accepted: merged, + writer: 'workflow', + }, + }); + } + // Not loop-produced, so it must not enter the acceptance metric — but its own + // task still has to reach a terminal state, or it sits in the inbox forever + // (ops.md maps "(none, merged) -> accepted", "(none, closed unmerged) -> + // rejected"). Every owner/dependabot PR gets a task from `pull_request` + // triage, so without this each one strands a zombie in the state layer, and + // sweep cannot repair it: sweep lists *open* GitHub items, which cannot see + // a closure. + return terminal({ + reason: `non-loop PR ${merged ? 'merged' : 'closed-unmerged'} ` + + '(not counted for auto-acceptance)', + taskId: pr.number, + to: merged ? 'accepted' : 'rejected', + pr: pr.number, }); }