From 3e2de35e3adf87b9e5890501f6bb3436baeb20f7 Mon Sep 17 00:00:00 2001 From: Jack McIntyre Date: Wed, 24 Jun 2026 16:00:22 +1000 Subject: [PATCH] feat(01KVW2GD): retry a thrown courier hiccup on a read-only setup seam A thrown courier error (the courier emitted its StructuredOutput call as literal text and never returned a structured result -> agent() throws) used to hit the seam() catch and re-throw on attempt #1, bypassing the retryable seam's attempts=3 loop. A read-only preflight seam (preflight:standards, retryable=true) therefore died on the first hiccup, aborting the whole run before any story was claimed. Treat a thrown courier error like a garbled relay: for a retryable seam with attempts remaining, record the sentinel and retry; only propagate after attempts are exhausted. SAFE because retryable is set only on read-only / idempotent seams. A non-retryable (mutating) seam has attempts=1, so it still re-throws on the FIRST failure with no retry (no double-apply). Extends the run-fault-injection harness with a transient-throw knob (throwSeamTimes) and two cases: AC1 a transient throw at the read-only preflight seam is retried + recovers (run proceeds to claim + build); AC2 the same hiccup at the mutating verdict seam stops loudly on the first failure, invoked exactly once. Claude-Session: https://claude.ai/code/session_01EE2ncqkqh3u5af93Uf3bDA --- .../__tests__/run-fault-injection.test.ts | 106 ++++++++++++++++++ .../flow/workflows/internal/run.workflow.js | 23 +++- 2 files changed, 125 insertions(+), 4 deletions(-) diff --git a/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts index ceb132ce..20b26f04 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts @@ -98,6 +98,16 @@ interface RunOpts { * (AC3) and the observability hard-fail (AC5). */ throwSeam?: { ref?: string; prefix: string }; + /** + * TRANSIENT hard-throw: throw the underlying courier call for the seam whose + * label starts with `prefix` (and matches `ref` when set) the FIRST `times` + * times it is invoked, then let it succeed (return the normal stubbed result) + * on every subsequent call. Models a one-off courier hiccup that the retry + * loop can RECOVER from — distinct from `throwSeam`, which throws forever. + * Used by AC1 (a transient throw at a retryable read-only preflight seam must + * be retried and recovered so the run proceeds to claim + build). + */ + throwSeamTimes?: { ref?: string; prefix: string; times: number }; /** Refs that should drive the needs-human-decision path, → their verbatim question. */ needsHuman?: Record; /** @@ -160,6 +170,10 @@ async function runRun(opts: RunOpts = {}): Promise<{ const calls: AgentCall[] = []; const notifications: NotifyPayload[] = []; + // How many times the transient-throw seam (throwSeamTimes) has already thrown, + // so it can throw the first `times` invocations then recover. + let transientThrowCount = 0; + // Map each claim index to the story it hands out (in queue order); past the // queue length the claim seam reports the genuine full-run outcome. const storyForClaimIdx = (idx: number): unknown => { @@ -265,6 +279,21 @@ async function runRun(opts: RunOpts = {}): Promise<{ throw new Error(`courier hard-failed for ${label}`); } + // TRANSIENT hard rejection (throwSeamTimes): throw the FIRST `times` + // invocations of the matching seam, then fall through so the seam succeeds. + // Models a one-off courier hiccup the retry loop recovers from. + if ( + opts.throwSeamTimes && + label.startsWith(opts.throwSeamTimes.prefix) && + labelForRef(label, opts.throwSeamTimes.ref) && + transientThrowCount < opts.throwSeamTimes.times + ) { + transientThrowCount += 1; + throw new Error( + `courier hiccup (transient ${transientThrowCount}/${opts.throwSeamTimes.times}) for ${label}`, + ); + } + // A SEAM call carries `schema`; it must return { stdout: }. if (agentOpts.schema) { // GARBLE a chosen seam: return a non-JSON line so seam()'s _parseError @@ -743,4 +772,81 @@ describe("run fault-injection — honest reporting under misbehaving seams (nati // REF_A still landed in exactly one bucket (blocked, from the garbled verdict). expect(result.blocked.some((b: any) => b.ref === REF_A)).toBe(true); }, 5_000); + + // ── native:01KVW2GDTB0FC64RJFSH34F47P — a one-off courier hiccup on a read-only ── + // ── setup step is RETRIED so the run no longer dies at the starting line ────────── + // + // The seam() helper advertises 3 attempts for a retryable seam, but a THROWN + // courier error (the courier emitted its StructuredOutput call as literal text + // → agent() throws) used to bypass the retry loop and re-throw on attempt #1. + // A read-only preflight seam (preflight:standards, retryable=true) therefore + // died on the first hiccup, aborting the WHOLE run before any story was claimed. + // The fix treats a thrown courier error like a garbled relay: retry the + // remaining attempts for a retryable seam; a NON-retryable (mutating) seam + // still throws on the FIRST failure (no double-apply). + + it("AC1: a TRANSIENT courier hiccup at a read-only preflight check is retried and recovers — the run proceeds to claim + build, never aborts pre-claim", async () => { + const queue = [{ ref: REF_A, title: "Story A (built after preflight recovers)" }]; + // The retryable read-only preflight:standards seam throws ONCE then succeeds. + // Old code: re-throws on attempt #1 → the whole run aborts at startup before + // any story is claimed. New code: retries, the second attempt returns the + // stubbed standards result, and the run carries on to claim + build REF_A. + const { result, thrown, logs, calls } = await runRun({ + queue, + maxConcurrency: 1, + throwSeamTimes: { prefix: "preflight:standards", times: 1 }, + }); + + // The run did NOT abort pre-claim: it finished on its own with a run reason. + expect(thrown).toBeUndefined(); + expect(result.runReason).toBe("queue-emptied"); + expect(result.queueEmptied).toBe(true); + + // The story was actually CLAIMED and processed — the run reached past the + // preflight gate (a claim seam ran, and the story landed in a terminal bucket). + expect(calls.some((c) => (c.opts.label ?? "").startsWith("claim:"))).toBe(true); + const reached = + result.completed.includes(REF_A) || + result.merged.some((m: any) => m.ref === REF_A) || + result.pausedForHuman.some((p: any) => p.ref === REF_A) || + result.blocked.some((b: any) => b.ref === REF_A); + expect(reached, "REF_A should have reached a terminal outcome (run did not abort pre-claim)").toBe(true); + + // The preflight courier was invoked TWICE (the throw + the recovering retry) + // and the retry was narrated — proving the retry loop, not a single attempt. + const preflightCalls = calls.filter((c) => (c.opts.label ?? "") === "preflight:standards"); + expect(preflightCalls.length).toBe(2); + expect(logs.some((l) => /seam preflight:standards courier threw \(attempt 1\/3\) — retrying/.test(l))).toBe(true); + + assertHonestyInvariant(result, thrown, [REF_A]); + }); + + it("AC2: the SAME hiccup at a STATE-CHANGING step (the mutating verdict seam) stops loudly on the FIRST failure — the seam is invoked exactly once, never retried", async () => { + const queue = [{ ref: REF_A, title: "Story A (mutating verdict throws once)" }]; + // The mutating verdict seam (retryable=false → attempts=1) throws. Even a + // transient (throws-once) hiccup must NOT be retried on a state-changing step, + // so a mutation is never repeated. seam() re-throws on the FIRST failure, + // processStory propagates, and runWorker buckets it as worker-threw. + const { result, thrown, calls } = await runRun({ + queue, + maxConcurrency: 1, + throwSeamTimes: { ref: REF_A, prefix: "verdict:", times: 1 }, + }); + + expect(thrown).toBeUndefined(); + + // REF_A is blocked with worker-threw on the FIRST failure — no silent retry. + const a = result.blocked.find((b: any) => b.ref === REF_A); + expect(a, "story A should be blocked").toBeDefined(); + expect(a.blocked_by).toBe("worker-threw"); + expect(result.completed).not.toContain(REF_A); + + // The verdict courier was invoked EXACTLY ONCE — the no-double-apply invariant: + // a state-changing step is never re-run after a thrown failure. + const verdictCalls = calls.filter((c) => (c.opts.label ?? "").startsWith("verdict:")); + expect(verdictCalls.length).toBe(1); + + expect(result.runReason).toBe("queue-emptied"); + assertHonestyInvariant(result, thrown, [REF_A]); + }); }); diff --git a/plugins/flow/workflows/internal/run.workflow.js b/plugins/flow/workflows/internal/run.workflow.js index d50f20ed..080fa7ad 100644 --- a/plugins/flow/workflows/internal/run.workflow.js +++ b/plugins/flow/workflows/internal/run.workflow.js @@ -194,11 +194,26 @@ const seam = async (cmd, label, retryable = false, swallow = false, modelOverrid { schema: RawSchema, label, phase: 'run', model: modelOverride || (retryable ? 'haiku' : 'sonnet') }, ) } catch (e) { - // HARD rejection of the courier call. For an observability seam we degrade - // exactly as for a garble (no line, keep going); for any other (mutating) - // seam we re-throw so the failure stays loud and reaches its bucket. - if (!swallow) throw e + // HARD rejection of the courier call (e.g. the courier emitted its + // StructuredOutput call as literal text and never returned a structured + // result → agent() throws "subagent completed without calling + // StructuredOutput"). Treat this exactly like a garbled-but-returned relay: + // for a RETRYABLE seam with attempts remaining, record the sentinel and + // RETRY the remaining attempts before giving up. This is SAFE because + // `retryable` is set ONLY on read-only / idempotent seams (e.g. the + // preflight getStatus check) — re-invoking one cannot double-apply a + // mutation. A NON-retryable (mutating) seam has attempts=1, so the + // `a < attempts - 1` guard is false on the first attempt and it re-throws + // on the FIRST failure with NO retry — the no-double-apply invariant. An + // observability seam (swallow=true) degrades to no line as before once + // attempts are exhausted. parsed = { _parseError: `seam-threw: ${String(e)}` } + if (a < attempts - 1) { + log(`seam ${label} courier threw (attempt ${a + 1}/${attempts}) — retrying`) + continue + } + // Attempts exhausted (or attempts=1 for a NON-retryable/mutating seam → no retry). + if (!swallow) throw e log(`seam ${label} hard-failed (observability, swallowed) — no progress line, continuing`) return parsed }