From 4fa3a52950292848ccb3d656e625d27ef17e814b Mon Sep 17 00:00:00 2001 From: RECTOR Date: Sat, 29 Aug 2026 11:20:34 +0700 Subject: [PATCH 1/2] fix(subagent): surface primary error when fallback retry masks it + journal failure reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #59 dogfood finding: a failed fallback retry returned ONLY the fallback's error, masking why the primary (e.g. an explicit model string) failed at all. Investigation showed explicit vs inherited model resolution are the same code path (both strings -> getModel(provider, id)); the observed asymmetry was transient provider failure + this masking, so the fix is diagnosability: - tools/subagent.ts: when the retry also fails, compose the surfaced error with both attempts (primary '': ; fallback: ). - engine/retry-fallback.ts: same composed-error contract for the lifecycle/bg spawn wrapper (withModelFallbackRetry). - engine/spawnSubagent.ts finishRun: journal the failure reason on the run:ended event (archived failing runs had empty resultSummary and no error field — post-hoc diagnosis from the journal was impossible). Tests: composed-error on both retry paths; run:ended carries error. --- src/engine/retry-fallback.ts | 9 +++++++- src/engine/spawnSubagent.ts | 3 +++ src/tools/subagent.ts | 9 ++++++++ test/retry-fallback.test.mts | 15 ++++++++++++ test/spawn-subagent-runlog.test.mts | 24 ++++++++++++++++++- test/subagent-tool.test.mts | 36 +++++++++++++++++++++++++++++ 6 files changed, 94 insertions(+), 2 deletions(-) diff --git a/src/engine/retry-fallback.ts b/src/engine/retry-fallback.ts index d875fa3..1dd7841 100644 --- a/src/engine/retry-fallback.ts +++ b/src/engine/retry-fallback.ts @@ -19,7 +19,14 @@ export function withModelFallbackRetry(spawn: SpawnFn, fallback: string | undefi return async (opts) => { const first = await spawn(opts); if (first.status === "failed" && first.retryable && fallback !== first.model && !signal?.aborted) { - return spawn({ ...opts, model: fallback }); + const second = await spawn({ ...opts, model: fallback }); + // #59: when the fallback also fails, compose the primary's failure into the surfaced error + // (same contract as the direct-foreground path in tools/subagent.ts) — the fallback's error + // alone masks why the primary failed. + if (second.status === "failed" && first.error) { + second.error = `primary '${first.model}' failed: ${first.error}; fallback '${fallback}' failed: ${second.error ?? second.status}`; + } + return second; } return first; }; diff --git a/src/engine/spawnSubagent.ts b/src/engine/spawnSubagent.ts index 9b8565d..c0f37ff 100644 --- a/src/engine/spawnSubagent.ts +++ b/src/engine/spawnSubagent.ts @@ -574,6 +574,9 @@ async function finishRun( type: "run:ended", runId, status, endedAt, resultSummary: finalText.slice(0, 120), tokenTotal, costTotal, contextTokens, resumedFrom: opts.resumeLink, forkedFrom: opts.forkLink, + // #59: journal the failure reason — the archived failing runs had run:ended with an empty + // resultSummary and no error field, making post-hoc diagnosis from the journal impossible. + ...(error ? { error } : {}), }); } catch { /* best-effort: journal is the index, not the product */ } // SPEC-4: lifecycle phase children skip the per-run todo reconciliation — the lifecycle diff --git a/src/tools/subagent.ts b/src/tools/subagent.ts index 4b1ee79..766d815 100644 --- a/src/tools/subagent.ts +++ b/src/tools/subagent.ts @@ -231,6 +231,15 @@ export function createSubagentTool(deps: SubagentToolDeps) { }); retriedWithModel = fallback; } + // #59: when the fallback retry ALSO fails, surface the PRIMARY's failure too — returning + // only the fallback's error masked why the primary (e.g. an explicit model string) failed + // at all, making provider diagnosis impossible from the controller's seat. + if (retriedWithModel && finalRes.status === "failed" && res.error && res.error !== finalRes.error) { + finalRes = { + ...finalRes, + error: `primary '${res.model}' failed: ${res.error}; fallback '${retriedWithModel}' failed: ${finalRes.error ?? finalRes.status}`, + }; + } const isError = finalRes.status === "failed" || finalRes.status === "aborted"; return { content: [{ type: "text" as const, text: isError ? (finalRes.error ?? finalRes.status) : finalRes.finalText }], diff --git a/test/retry-fallback.test.mts b/test/retry-fallback.test.mts index cf98a22..ed447bb 100644 --- a/test/retry-fallback.test.mts +++ b/test/retry-fallback.test.mts @@ -81,3 +81,18 @@ test("#39 wrapper: signal already aborted → NO retry (even on retryable failur assert.equal(out.status, "failed", "first (failed) result returned"); assert.deepEqual(calls, ["(none)"], "no retry when signal aborted"); }); + +test("#59: both attempts fail → returned error includes the PRIMARY's failure (no masking)", async () => { + const { fn, calls } = fakeSpawn([ + res({ retryable: true, model: "Ollama/glm", error: "rate limited" }), + res({ model: "openrouter/glm", error: "also rate limited" }), + ]); + const wrapped = withModelFallbackRetry(fn, "openrouter/glm"); + const out = await wrapped(baseOpts); + assert.equal(out.status, "failed"); + assert.deepEqual(calls, ["(none)", "openrouter/glm"], "retried once on the fallback"); + assert.ok(out.error!.includes("Ollama/glm"), `names the primary model: ${out.error}`); + assert.ok(out.error!.includes("rate limited"), "includes the primary's error text"); + assert.ok(out.error!.includes("openrouter/glm"), "names the fallback model"); + assert.ok(out.error!.includes("also rate limited"), "includes the fallback's error text"); +}); diff --git a/test/spawn-subagent-runlog.test.mts b/test/spawn-subagent-runlog.test.mts index a1f59bf..fc8c0fd 100644 --- a/test/spawn-subagent-runlog.test.mts +++ b/test/spawn-subagent-runlog.test.mts @@ -128,4 +128,26 @@ test("tool_error result is kept in full in the journal", async () => { const toolEvent = log.replay(res.runId).find((e) => e.type === "tool") as any; strictEqual(toolEvent.isError, true); ok(toolEvent.result.length > 20, "error result not excerpted"); -}); \ No newline at end of file +}); +test("#59: run:ended carries the failure reason (error) on failed runs", async () => { + // The archived #59 failing runs had run:ended with empty resultSummary and NO error field — + // post-hoc diagnosis from the journal was impossible. The failure reason must be journaled. + const handlers: Array<(e: any) => void> = []; + const errChild: ChildSession = { + prompt: async () => { for (const h of handlers) h({ type: "message_end", message: { role: "assistant", stopReason: "error", content: [{ type: "text", text: "quota exhausted" }] } }); }, + subscribe: (h: any) => { handlers.push(h); return () => {}; }, + abort: async () => {}, dispose: () => {}, + }; + const factory: ChildSessionFactory = { create: async () => ({ session: errChild, model: "m" }) }; + const log = new RunLog(logDir); + const h = harness(factory, log); + const res = await spawnSubagent({ + agent: "g", task: "t", track: true, runLog: log, + registry: h.registry, todoSync: h.todoSync, runRegistry: h.runRegistry, lock: h.lock, backendRegistry: h.backendRegistry, + parentModel: PARENT, parentCwd: tmpDir, + }); + strictEqual(res.status, "failed"); + const ended = log.replay(res.runId).at(-1) as any; + strictEqual(ended.type, "run:ended"); + ok(typeof ended.error === "string" && ended.error.includes("quota exhausted"), `run:ended.error present + meaningful: ${ended.error}`); +}); diff --git a/test/subagent-tool.test.mts b/test/subagent-tool.test.mts index d3ea572..5f7985f 100644 --- a/test/subagent-tool.test.mts +++ b/test/subagent-tool.test.mts @@ -451,3 +451,39 @@ test("SPEC-6-5: same-cwd dispatch does NOT fire onNotify", async () => { test("SPEC-6-5: subagentParams schema includes cwd", () => { ok("cwd" in subagentParams.properties, "cwd field in the schema"); }); + +test("#59: when the fallback retry also fails, the surfaced error includes the PRIMARY's failure", async () => { + // Regression: the #39 retry returned only the FALLBACK's error — the primary's actual failure + // (e.g. why an explicit model string failed at all) was masked entirely (#59 dogfood finding: + // the surfaced error named 'openrouter/z-ai/glm-5.2' while the primary 'Ollama/glm-5.2:cloud' + // failure was invisible). + let createCalls = 0; + const makeErrorChild = (text: string) => { + const hs: Array<(e: any) => void> = []; + return { + prompt: async () => { for (const h of hs) h({ type: "message_end", message: { role: "assistant", stopReason: "error", content: [{ type: "text", text }] } }); }, + subscribe: (h: any) => { hs.push(h); return () => {}; }, abort: async () => {}, dispose: () => {}, + }; + }; + const factory: ChildSessionFactory = { + create: async () => { + createCalls += 1; + return createCalls === 1 + ? { session: makeErrorChild("primary rate limited"), model: "Ollama/glm-5.2:cloud" } + : { session: makeErrorChild("fallback rate limited too"), model: "openrouter/z-ai/glm-5.2" }; + }, + }; + const deps = makeDeps(); + const reg = new BackendRegistry(); + reg.register({ id: "pi", factory, available: () => true, versionInfo: () => null, hookParity: PI_HOOK_PARITY }); + deps.backendRegistry = reg; + const tool = createSubagentTool(deps); + const out = await tool.execute!("c", { agent: "g", task: "x", model: "Ollama/glm-5.2:cloud", modelFallback: "openrouter/z-ai/glm-5.2" } as any, new AbortController().signal, () => {}, {} as any); + strictEqual(createCalls, 2, "factory called twice (primary failed retryable → retried on fallback)"); + strictEqual(out.isError, true, "both primary and fallback failed"); + const text = (out.content as any)[0].text as string; + ok(text.includes("Ollama/glm-5.2:cloud"), `error names the PRIMARY model: ${text}`); + ok(text.includes("primary rate limited"), `error includes the PRIMARY's failure text: ${text}`); + ok(text.includes("openrouter/z-ai/glm-5.2"), `error names the FALLBACK model: ${text}`); + ok(text.includes("fallback rate limited too"), `error includes the FALLBACK's failure text: ${text}`); +}); From cf71cd899c2c1f5fd51778f9bb1fb1804a79e15c Mon Sep 17 00:00:00 2001 From: RECTOR Date: Sat, 29 Aug 2026 11:28:07 +0700 Subject: [PATCH 2/2] review: RunEndedEvent gains error? (journal contract) + mirror dedup guard in retry wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on PR #69: the journaled run:ended.error rode on a TS spread quirk (excess-property bypass) — declare it on the RunEndedEvent interface so the journal schema is the contract; and mirror the direct path's identical-error dedup guard in withModelFallbackRetry so both composition sites share one contract. --- src/engine/retry-fallback.ts | 2 +- src/engine/spawnSubagent.ts | 2 +- src/runtime/run-log.ts | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/engine/retry-fallback.ts b/src/engine/retry-fallback.ts index 1dd7841..3c82d96 100644 --- a/src/engine/retry-fallback.ts +++ b/src/engine/retry-fallback.ts @@ -23,7 +23,7 @@ export function withModelFallbackRetry(spawn: SpawnFn, fallback: string | undefi // #59: when the fallback also fails, compose the primary's failure into the surfaced error // (same contract as the direct-foreground path in tools/subagent.ts) — the fallback's error // alone masks why the primary failed. - if (second.status === "failed" && first.error) { + if (second.status === "failed" && first.error && first.error !== second.error) { second.error = `primary '${first.model}' failed: ${first.error}; fallback '${fallback}' failed: ${second.error ?? second.status}`; } return second; diff --git a/src/engine/spawnSubagent.ts b/src/engine/spawnSubagent.ts index c0f37ff..9caf60f 100644 --- a/src/engine/spawnSubagent.ts +++ b/src/engine/spawnSubagent.ts @@ -576,7 +576,7 @@ async function finishRun( resumedFrom: opts.resumeLink, forkedFrom: opts.forkLink, // #59: journal the failure reason — the archived failing runs had run:ended with an empty // resultSummary and no error field, making post-hoc diagnosis from the journal impossible. - ...(error ? { error } : {}), + error, }); } catch { /* best-effort: journal is the index, not the product */ } // SPEC-4: lifecycle phase children skip the per-run todo reconciliation — the lifecycle diff --git a/src/runtime/run-log.ts b/src/runtime/run-log.ts index ba99a6e..9c46ea5 100644 --- a/src/runtime/run-log.ts +++ b/src/runtime/run-log.ts @@ -33,6 +33,8 @@ export interface RunEndedEvent { costTotal?: number; /** SPEC-6-1: latest context-token snapshot at run end. */ contextTokens?: number; + /** #59: the failure reason on failed runs (post-hoc diagnosability from the journal). */ + error?: string; } export type RunLogEvent = RunMetaEvent | MessageEvent | ToolEvent | RunEndedEvent;