diff --git a/scripts/loop/README.md b/scripts/loop/README.md index 49dd4bb..e3552a2 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 | @@ -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 @@ -462,6 +473,43 @@ 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. +- [ ] 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 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 }; }