From 6e000afddcbc510f468b9a5e0eedced6259fea1e Mon Sep 17 00:00:00 2001 From: Alex Chao Date: Fri, 11 Sep 2026 03:17:50 +0800 Subject: [PATCH 1/3] chore(loop): move tasks to terminal states on GitHub gate events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D30. `handleMetrics` appended the acceptance row and returned, so a loop PR that merged was counted for the auto-acceptance rate while its task stayed in `waiting-merge` — contradicting the state machine ops.md defines (`waiting-merge -> accepted | rejected`). Sweep cannot repair it either: sweep reconciles by listing *open* GitHub items, which by construction cannot see a closure. A loop PR merged unattended would strand its task under "Awaiting human merge" permanently, and this was the next thing to happen in A1. `handleMetrics` now also applies the transition via `markTaskTerminal` (merged -> accepted, closed-unmerged -> rejected) and refreshes SUMMARY.md. The two effects are idempotent on independent footings — the row on `taskId+event+pr`, the task on its own status — so a crash between them is repaired by a repeated event instead of leaving a permanent inconsistency. An originating state other than `waiting-merge` is recorded as `(was ...)`, which makes an unexpected route visible rather than silent. D31. `renderSummary` named two different things: the shared library's exporter writes `state/SUMMARY.md`, while entry.mjs's local function renders the Actions Step Summary. Importing the former beside the latter was a SyntaxError at module load — the orchestrator would not start. The collision is a symptom, so both are renamed: `renderStateSummary` and `renderStepSummary` (paired with the existing `writeStepSummary`). The terminal-state list, previously written out inline in more than one place, is now the single exported `TERMINAL_STATUSES`. Verified: both transition directions, duplicate events, a non-waiting-merge origin, `released` leaving the status alone, a missing task not throwing, plus the standing regression (route decisions, CLI guards, orchestrator paths). --- scripts/loop/README.md | 28 +++++++++++++++- scripts/loop/entry.mjs | 39 ++++++++++++++++++++--- scripts/loop/run.mjs | 4 +-- scripts/loop/shared/state.mjs | 60 ++++++++++++++++++++++++++++++++--- 4 files changed, 120 insertions(+), 11 deletions(-) diff --git a/scripts/loop/README.md b/scripts/loop/README.md index 49dd4bb..ee329b7 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -88,7 +88,7 @@ GitHub event ─▶ loop.yml (concurrency group `loop` = platform-level single w | loop PR (`loop/-*` **and** `loop-task: #`) opened/synchronize | run(pr-review) | | 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 | +| `pull_request_target` closed (loop PR) | metrics: merged / closed-unmerged **and** the task moves to `accepted` / `rejected` | | `release` published | metrics: released | | `schedule` daily / weekly | system run: sweep / retro-scheduled | | `workflow_dispatch` | run per inputs (`task`/`stage`); `execute_writes=true` = L2 rehearsal | @@ -462,6 +462,32 @@ was *correct*, and whether new events produce proposals that match reality. awaiting merge). Verified across five scenarios: inbox and awaiting-merge are refused without touching the decision, `closed` + a genuinely reopened GitHub item is still let through, and `closed` + a closed item is not. +- [x] D30 (A1) Gate events recorded the acceptance row but never moved the task: + `handleMetrics` appended to `metrics/acceptance.jsonl` and returned, so a + loop PR that merged was counted for the auto-acceptance rate while its task + stayed in `waiting-merge`. That contradicts the state machine ops.md + defines (`waiting-merge → accepted | rejected`), and sweep cannot repair it + — sweep reconciles by listing *open* GitHub items, which by construction + cannot see a closure. Any loop PR merged unattended would have stranded its + task under "Awaiting human merge" permanently. `handleMetrics` now also + applies the transition via `markTaskTerminal` (merged → `accepted`, + closed-unmerged → `rejected`) and refreshes `SUMMARY.md`. The two effects + are idempotent independently — the row on `taskId+event+pr`, the task on + its own status — so a crash between them is repaired by a repeated event + rather than leaving a permanent inconsistency. Verified: both directions, + duplicate events, a non-`waiting-merge` originating state (recorded as + `(was …)` so an unexpected route is visible), `released` leaving the status + alone, and a missing task not throwing. +- [x] D31 (A1) `renderSummary` named two different things: the shared library's + exporter writes `state/SUMMARY.md`, while `entry.mjs`'s local function + renders the Actions Step Summary. Importing the former alongside the latter + was a `SyntaxError` at module load — the orchestrator would not start. The + real problem is the name, not the collision, so they are now + `renderStateSummary` (state layer) and `renderStepSummary` (Actions UI, + paired with the existing `writeStepSummary`). Terminal-state lists, which + had also been written out inline in more than one place, are now the single + exported `TERMINAL_STATUSES` — duplicated lists are this codebase's + recurring failure mode. ## Environment facts diff --git a/scripts/loop/entry.mjs b/scripts/loop/entry.mjs index 1f80f8e..1ff2c20 100644 --- a/scripts/loop/entry.mjs +++ b/scripts/loop/entry.mjs @@ -29,7 +29,8 @@ import { spawnSync } from 'node:child_process'; import { makeState, ensureDirs, readJson, readJsonl, writeJson, saveTask, listTasks, - appendAcceptance, beginRun, finishRun, lockExpired, + appendAcceptance, markTaskTerminal, GATE_TRANSITIONS, renderStateSummary, + beginRun, finishRun, lockExpired, nowIso, outcomeAllowed, allowedOutcomes, describeActions, } from './shared/state.mjs'; import { route, eventKey } from './shared/route.mjs'; @@ -137,6 +138,15 @@ async function handleInbox({ ctx, repo }) { /* ---------------------------------------------------------------- metrics */ +/** + * GitHub gate events (a loop PR merged / closed unmerged, a release) are the only + * place a task reaches a terminal state without a run. Two effects, each + * idempotent on its own footing: + * 1. append the acceptance row (deduplicated by taskId+event+pr); + * 2. move the task to its terminal state. + * They are deliberately not gated on each other — if the process dies between + * them, a repeated event still repairs the task instead of leaving it stranded. + */ async function handleMetrics({ decision }) { const row = { at: nowIso(), ...decision.metrics }; if (row.event === 'released' && !row.taskId) { @@ -149,7 +159,22 @@ async function handleMetrics({ decision }) { } } const wrote = await appendAcceptance(S, row); - return { state: 'metrics', wrote, row }; + + let transition = null; + const to = GATE_TRANSITIONS[row.event]; + if (to && row.taskId) { + transition = await markTaskTerminal(S, row.taskId, to, { + event: row.event, + detail: `${row.pr ? `PR #${row.pr} ` : ''}${row.event}`, + }); + if (transition.changed) { + log(`task #${row.taskId}: ${transition.from} → ${to} (${row.event})`); + await renderStateSummary(S); + } else { + log(`task #${row.taskId}: no transition needed (${transition.reason})`); + } + } + return { state: 'metrics', wrote, row, transition }; } /* ------------------------------------------------------------------ run */ @@ -258,7 +283,7 @@ async function readTextSafe(file) { try { return await fs.readFile(file, 'utf8'); } catch { return ''; } } -function renderSummary({ decision, result, writeLevel }) { +function renderStepSummary({ decision, result, writeLevel }) { const L = [`## Loop — ${decision.action}`, '']; L.push(`- event: \`${decision.reason}\``); if (result?.runId) { @@ -273,6 +298,12 @@ function renderSummary({ decision, result, writeLevel }) { } else if (result?.note) { L.push(`- ${result.state}: ${result.note}`); } + if (result?.transition) { + const tr = result.transition; + L.push(tr.changed + ? `- task #${tr.task.id}: **${tr.from} → ${tr.task.status}**` + : `- task: no transition needed (${tr.reason})`); + } if (result?.actions?.length) { const head = writeLevel === 'auto' ? '### GitHub actions executed' : '### Proposed GitHub actions (report boundary — NOT executed)'; L.push('', head, ''); @@ -365,7 +396,7 @@ async function main() { : 'metrics already present, skipped (idempotent)'); } - await writeStepSummary(renderSummary({ decision, result, writeLevel })); + await writeStepSummary(renderStepSummary({ decision, result, writeLevel })); return 0; // noop / inbox-only / metrics / a completed run all exit successfully } diff --git a/scripts/loop/run.mjs b/scripts/loop/run.mjs index 33bdbbc..6062d0e 100644 --- a/scripts/loop/run.mjs +++ b/scripts/loop/run.mjs @@ -44,7 +44,7 @@ import { spawnSync } from 'node:child_process'; import { makeState, ensureDirs, loadTask, saveTask, nextTaskId, listTasks, readRunLines, appendRunRow, readJson, writeJson, - nowIso, parseArgs, renderSummary, + nowIso, parseArgs, renderStateSummary, OUTCOME_MAP, lockExpired, describeActions, beginRun, finishRun, } from './shared/state.mjs'; @@ -307,7 +307,7 @@ write level: LOOP_WRITE_LEVEL=report|auto (default report)`); if (cmd === 'start') return await cmdStart(args); if (cmd === 'end') return await cmdEnd(args); if (cmd === 'checkpoint') return await cmdCheckpoint(args); - if (cmd === 'summary') return await renderSummary(S); + if (cmd === 'summary') return await renderStateSummary(S); if (cmd === 'view') return await cmdView(args); } catch (e) { fail(e.message); diff --git a/scripts/loop/shared/state.mjs b/scripts/loop/shared/state.mjs index 8a3d6ca..7cfa9eb 100644 --- a/scripts/loop/shared/state.mjs +++ b/scripts/loop/shared/state.mjs @@ -17,6 +17,13 @@ import { spawnSync } from 'node:child_process'; /** Five canonical role labels (triage-labels.md): mutually exclusive, never stacked. */ export const ROLE_LABELS = ['needs-triage', 'needs-info', 'ready-for-agent', 'ready-for-human', 'wontfix']; +/** + * Terminal task states (ops.md state machine). Single definition on purpose: + * this list used to be written out inline in several places, and duplicated + * lists are how this codebase drifts. + */ +export const TERMINAL_STATUSES = ['accepted', 'rejected', 'closed']; + /** outcome → task terminal status + label to apply (ops.md "Labels → task state"). */ export const OUTCOME_MAP = { 'triaged': { status: 'ready', label: 'ready-for-agent' }, @@ -212,9 +219,54 @@ export async function appendAcceptance(s, row) { return true; } +/** + * Move a task to a terminal state in response to a GitHub gate event. + * + * ops.md defines `waiting-merge → accepted (merged) / rejected (closed-unmerged)`, + * and GitHub is the source of truth for "a human accepted or rejected this". + * Nothing else in the host performed that transition: the router recorded the + * acceptance row and left the task sitting in `waiting-merge` forever, where it + * kept showing up under "Awaiting human merge" and no sweep could correct it + * (sweep reconciles by listing *open* GitHub items, which cannot see this + * direction). A loop PR merged with nobody watching would strand its task. + * + * Idempotent on the task's own state rather than on the acceptance row: if the + * process dies between writing the row and the transition, a repeated event + * still repairs the task. + * + * @returns {{task: object|null, changed: boolean, from?: string, reason?: string}} + */ +export async function markTaskTerminal(s, taskId, to, { event, detail, by = 'workflow' } = {}) { + if (!TERMINAL_STATUSES.includes(to)) { + throw new Error(`markTaskTerminal: ${to} is not terminal (expected one of ${TERMINAL_STATUSES.join(', ')})`); + } + const t = await readJson(s.taskFile(taskId)); + if (!t) return { task: null, changed: false, reason: `task #${taskId} does not exist` }; + if (TERMINAL_STATUSES.includes(t.status)) { + return { task: t, changed: false, reason: `already ${t.status}` }; + } + const from = t.status; + t.status = to; + t.labels = []; // terminal states carry no role label + t.timeline.push({ + at: nowIso(), + event, + by, + detail: `${detail}${from === 'waiting-merge' ? '' : ` (was ${from})`}`, + }); + await saveTask(s, t); + return { task: t, changed: true, from }; +} + +/** The router's gate events → the terminal state they imply. */ +export const GATE_TRANSITIONS = { + merged: 'accepted', + 'closed-unmerged': 'rejected', +}; + /* -------------------------------------------------------------- SUMMARY */ -export async function renderSummary(s) { +export async function renderStateSummary(s) { const tasks = await listTasks(s); const L = []; L.push(`# Loop state summary (updated ${nowIso()})`); @@ -257,7 +309,7 @@ export async function renderSummary(s) { return `#${t.id} (${kind}) — ${t.title} (${when(t)})`; })); - const terminal = tasks.filter((t) => ['closed', 'accepted', 'rejected'].includes(t.status)); + const terminal = tasks.filter((t) => TERMINAL_STATUSES.includes(t.status)); if (terminal.length) { const c = (st) => terminal.filter((t) => t.status === st).length; L.push('## Terminal counts', ''); @@ -451,7 +503,7 @@ export async function finishRun(s, { }); if (!t) { - await renderSummary(s); + await renderStateSummary(s); return { task: null, actions: [], applied: [] }; } @@ -477,7 +529,7 @@ export async function finishRun(s, { else await appendActionBlock(s, runId, actions); } - await renderSummary(s); + await renderStateSummary(s); return { task: t, actions, applied }; } From 4876568da3cc166fc66d4447bc7e20f9bfe500ea Mon Sep 17 00:00:00 2001 From: Alex Chao Date: Fri, 11 Sep 2026 03:18:18 +0800 Subject: [PATCH 2/3] docs(loop): flag an acceptance double-counting hazard (D32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sweep's mandate is to rewrite a missed closure "labelled `sweep-corrected`". Read literally that writes `event: "sweep-corrected"`, which changes the `taskId+event+pr` dedup key and appends a second row for the same closure — double-counting the PR in the auto-acceptance rate, the one number retro exists to produce. Not reachable today (no sweep implementation writes it yet), and recorded now because the ambiguous sentence lives in the norms layer, which is where whoever implements the backfill will read it. The fix is to keep the original `event` and use the existing `writer` field. --- scripts/loop/README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/scripts/loop/README.md b/scripts/loop/README.md index ee329b7..5cdf8ec 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -488,6 +488,17 @@ was *correct*, and whether new events produce proposals that match reality. had also been written out inline in more than one place, are now the single exported `TERMINAL_STATUSES` — duplicated lists are this codebase's recurring failure mode. +- [ ] D32 (A1, open — not yet reachable) The acceptance dedup key is + `taskId|event|pr`, but sweep's mandate (docs + `skills/sweep/SKILL.md`) is + to rewrite a *missed* event "labelled `sweep-corrected`". Read literally + that means writing `event: "sweep-corrected"`, which changes the key and + therefore appends a **second** row for the same closure — double-counting + the PR in the auto-acceptance rate, which is the one number retro exists + to produce. Not yet triggerable: no sweep implementation writes it today. + The fix when sweep lands is to keep the original `event` value and mark + 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. ## Environment facts From 7b2a60ea579689daaa9ac8d85ce5fcc57e849bbb Mon Sep 17 00:00:00 2001 From: Alex Chao Date: Fri, 11 Sep 2026 03:22:32 +0800 Subject: [PATCH 3/3] docs(loop): record the live PR-resynchronisation skip (D26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second push to this PR produced the first real sighting: event=pull_request.synchronize -> run (repo PR synchronize) skip: task #39 not claimable (status=waiting-human) - event handled idempotently So a PR task is never re-analysed after its first push, and neither is a `waiting-merge` loop PR after a new commit. This is a direct consequence of running at the report boundary: `flows` §2a has 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. D26 now covers both faces with the observed evidence. Still deliberately open: the fix is a design choice (re-open PR tasks on `synchronize`, or a bounded re-triage), and A1's week should quantify how often it bites first. The same run also re-confirmed D25 in the field: the skip reason is in the Actions log, which is exactly the line that would have been blank before. --- scripts/loop/README.md | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/scripts/loop/README.md b/scripts/loop/README.md index 5cdf8ec..e3552a2 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -426,13 +426,24 @@ was *correct*, and whether new events produce proposals that match reality. 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 task in `waiting-merge` cannot be re-claimed, so a loop - PR that receives a new commit (`synchronize`) is skipped instead of - re-reviewed, and `review_requested`-style re-entry has no path back. - Re-review after new commits is normal in a PR loop; the fix probably - belongs in the `pr-review` route (reopen `waiting-merge` → `ready` on - `synchronize` for loop PRs) rather than in claimability. Left open - deliberately: A1's week should show how often it actually bites. +- [ ] 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. + - `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. - [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