diff --git a/src/engine/retry-fallback.ts b/src/engine/retry-fallback.ts index d875fa3..3c82d96 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 && first.error !== second.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..9caf60f 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, }); } 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; 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}`); +});