From 1f6c50db0d09cc840b3b2770022be2bc2e2e16b3 Mon Sep 17 00:00:00 2001 From: "judagent[bot]" <331042234+judagent[bot]@users.noreply.github.com> Date: Wed, 23 Sep 2026 01:14:35 +0000 Subject: [PATCH] fix: name the serving route on advisor errors and failed fallbacks Generation failures now carry the serving route, so error rows record advisorProvider instead of looking like primary-route failures. A failed fallback reports both reasons with the primary first (error classification unchanged), and the fallback check is guarded so its own failure never masks the primary error. Adds a regression test proving the shared advisor session re-pins back to anthropic once the primary recovers. --- src/advisorGate.test.ts | 106 ++++++++++++++++++++++++++++- src/ocAdvisor.ts | 145 ++++++++++++++++++++++++++-------------- 2 files changed, 199 insertions(+), 52 deletions(-) diff --git a/src/advisorGate.test.ts b/src/advisorGate.test.ts index ce1b4cd..5554c02 100644 --- a/src/advisorGate.test.ts +++ b/src/advisorGate.test.ts @@ -751,10 +751,14 @@ describe("runAdvisor provider fallback", () => { question: "Fallback check", config: baseConfig(), }), - ).rejects.toThrow("Model unavailable: anthropic/claude-opus-5-5"); + ).rejects.toThrow( + "Model unavailable: anthropic/claude-opus-5-5 (fallback opencode: Model unavailable: opencode/claude-opus-5-5)", + ); expect(generated.length).toBe(0); expect(metricRecords()[0].outcome).toBe("error"); expect(metricRecords()[0].errorType).toBe("model_unavailable"); + // No route was chosen, so the row names none. + expect(metricRecords()[0].advisorProvider).toBeUndefined(); }); test("falls back on the V2 discovery path with effort selection on", async () => { @@ -802,6 +806,106 @@ describe("runAdvisor provider fallback", () => { }); expect(metricRecords()[0].advisorProvider).toBe("opencode"); }); + + test("names the fallback route when generation fails after it", async () => { + const { client } = stubGate(gateResponse(0.9)); + const { runtime, generated } = runtimeWith({ + gateClient: client, + generate: async () => { + throw new Error("boom"); + }, + }); + const catalog = runtime.catalog!; + catalog.provider!.get = async (args: { providerID?: string }) => { + if (args?.providerID === "anthropic") + throw new Error("provider disabled"); + return { activation: "enabled" }; + }; + catalog.model!.list = async () => [ + { + providerID: "opencode", + id: "claude-opus-5-5", + enabled: true, + variants: [{ id: "xhigh" }], + }, + ]; + await expect( + runAdvisor({ + runtime, + sessionId: "ses_fixture", + mode: "general", + question: "Failure check", + config: baseConfig(), + }), + ).rejects.toThrow("advisor failed (api_error): boom"); + expect(generated.length).toBe(1); + expect(metricRecords()[0].outcome).toBe("error"); + expect(metricRecords()[0].advisorProvider).toBe("opencode"); + }); + + test("re-pins the shared session when the primary recovers", async () => { + const { client } = stubGate(gateResponse(0.9)); + const { runtime } = runtimeWith({ gateClient: client }); + let anthropicDown = true; + const catalog = runtime.catalog!; + catalog.provider!.get = async (args: { providerID?: string }) => { + if (anthropicDown && args?.providerID === "anthropic") { + throw new Error("provider disabled"); + } + return { activation: "enabled" }; + }; + const anthropicModel = { + providerID: "anthropic", + id: "claude-opus-5-5", + enabled: true, + variants: [{ id: "xhigh" }, { id: "max" }], + }; + const opencodeModel = { + providerID: "opencode", + id: "claude-opus-5-5", + enabled: true, + variants: [{ id: "xhigh" }, { id: "max" }], + }; + catalog.model!.list = async () => + anthropicDown ? [opencodeModel] : [anthropicModel, opencodeModel]; + let pinned = { + providerID: "anthropic", + id: "claude-opus-5-5", + variant: "xhigh", + }; + const switches: Array> = []; + runtime.session!.get = async () => ({ + id: "ses_advisor_fixture", + model: { ...pinned }, + }); + runtime.session!.switchModel = async (args: Record) => { + switches.push(args); + pinned = { ...(args.model as typeof pinned) }; + }; + const config = baseConfig(); + await runAdvisor({ + runtime, + sessionId: "ses_fixture", + mode: "general", + question: "First", + config, + }); + anthropicDown = false; + await runAdvisor({ + runtime, + sessionId: "ses_fixture", + mode: "general", + question: "Second", + config, + }); + expect( + switches.map((args) => (args.model as { providerID: string }).providerID), + ).toEqual(["opencode", "anthropic"]); + expect(metricRecords().map((row) => row.advisorProvider)).toEqual([ + "opencode", + "anthropic", + ]); + }); }); describe("runAdvisor resolves model profiles", () => { diff --git a/src/ocAdvisor.ts b/src/ocAdvisor.ts index c1ed1e7..d2d5e89 100644 --- a/src/ocAdvisor.ts +++ b/src/ocAdvisor.ts @@ -494,9 +494,10 @@ interface AdvisorMetrics { via: string; gate?: GateRecord; benchmarks?: BenchmarkMetricsSummary; - // Provider route that served the generation (`opencode` when the Anthropic - // fallback fired). Present only on generated responses; older rows and - // non-generating outcomes omit it. + // Provider route that served (or attempted) the generation (`opencode` + // when the Anthropic fallback fired). Present on generated responses and + // on failures past the support check; older rows and earlier outcomes + // omit it. advisorProvider?: string; } @@ -824,6 +825,23 @@ function classifyAdvisorError(message: string): string { return "api_error"; } +// Tags a generation failure with the serving route so error rows name it. +// Returns the same error for rethrowing; non-Error values pass through. +function withAdvisorProvider(err: unknown, provider: string): unknown { + if (err instanceof Error) { + (err as Error & { advisorProvider?: string }).advisorProvider = provider; + } + return err; +} + +function errorAdvisorProvider(err: unknown): string | undefined { + if (err instanceof Error) { + const provider = (err as { advisorProvider?: unknown }).advisorProvider; + if (typeof provider === "string" && provider) return provider; + } + return undefined; +} + function callerLabel(model: SessionModel | null): string | null { if (!model) return null; const provider = model.providerID || model.provider || "unknown"; @@ -1844,10 +1862,10 @@ async function callAdvisor(opts: { // Anthropic first, opencode on retry: when the configured Anthropic // advisor is unavailable (provider off, model missing, no connection), one - // attempt through the opencode gateway keeps consultations working. Any - // other failure — including a failed fallback — reports the primary reason - // so the caller sees why the configured route did not work. Gate evidence - // was built from the primary config; the anthropic/ and opencode/ bindings + // attempt through the opencode gateway keeps consultations working. A + // failed fallback reports both reasons with the primary first, so error + // classification still matches the configured route. Gate evidence was + // built from the primary config; the anthropic/ and opencode/ bindings // point at the same Artificial Analysis records, so the scores still apply. let activeConfig = config; let support = await checkAdvisorSupport(runtime, config, opts.effort); @@ -1855,18 +1873,36 @@ async function callAdvisor(opts: { !support.supported && config.provider.toLowerCase() === ADVISOR_PROVIDER ) { + const primaryReason = support.reason; const fallbackConfig = { ...config, provider: ADVISOR_FALLBACK_PROVIDER }; - const fallback = await checkAdvisorSupport( - runtime, - fallbackConfig, - opts.effort, - ); + let fallback: AdvisorSupport; + try { + fallback = await checkAdvisorSupport( + runtime, + fallbackConfig, + opts.effort, + ); + } catch (err) { + fallback = { + supported: false, + reason: `fallback check failed: ${err instanceof Error ? err.message : String(err)}`, + }; + } if (fallback.supported) { console.log( - `[advisor] anthropic route unavailable (${support.reason}); using ${fallbackConfig.provider}/${fallbackConfig.model}`, + `[advisor] anthropic route unavailable (${primaryReason}); using ${fallbackConfig.provider}/${fallbackConfig.model}`, ); activeConfig = fallbackConfig; support = fallback; + } else { + console.log( + `[advisor] anthropic route unavailable (${primaryReason}); fallback ${fallbackConfig.provider} also failed (${fallback.reason})`, + ); + const combined: AdvisorSupport = { + supported: false, + reason: `${primaryReason} (fallback ${fallbackConfig.provider}: ${fallback.reason})`, + }; + support = combined; } } if (!support.supported) { @@ -1882,45 +1918,51 @@ async function callAdvisor(opts: { const generate = runtime.session.generate.bind(runtime.session); // The timeout wraps only the generation, not the time spent waiting in the // queue behind other advisor calls — otherwise a backlog guarantees timeouts. - const text = await enqueueAdvisor(() => - withTimeout( - (async () => { - const sessionId = await ensureAdvisorSession( - runtime, - support.variant, - activeConfig, - ); - const request = { sessionID: sessionId, prompt }; - const startedAt = Date.now(); - const run = async () => { - const result = opts.signal - ? await generate(request, { signal: opts.signal }) - : await generate(request); - return extractGeneratedText(result); - }; - // A generation can come back without text (transient provider - // behavior, e.g. a reasoning-only response with adaptive thinking). - // Retry once while most of the timeout budget remains; never retry - // after a caller cancellation. - let output = await run(); - if (!output?.trim() && !opts.signal?.aborted) { - const elapsed = Date.now() - startedAt; - if (elapsed < activeConfig.timeoutMs / 2) { - console.log( - `[advisor] empty generation; retrying once (session=${sessionId} elapsedMs=${elapsed})`, - ); - output = await run(); + // Failures past this point carry the serving route so error rows name it. + let text: string; + try { + text = await enqueueAdvisor(() => + withTimeout( + (async () => { + const sessionId = await ensureAdvisorSession( + runtime, + support.variant, + activeConfig, + ); + const request = { sessionID: sessionId, prompt }; + const startedAt = Date.now(); + const run = async () => { + const result = opts.signal + ? await generate(request, { signal: opts.signal }) + : await generate(request); + return extractGeneratedText(result); + }; + // A generation can come back without text (transient provider + // behavior, e.g. a reasoning-only response with adaptive thinking). + // Retry once while most of the timeout budget remains; never retry + // after a caller cancellation. + let output = await run(); + if (!output?.trim() && !opts.signal?.aborted) { + const elapsed = Date.now() - startedAt; + if (elapsed < activeConfig.timeoutMs / 2) { + console.log( + `[advisor] empty generation; retrying once (session=${sessionId} elapsedMs=${elapsed})`, + ); + output = await run(); + } } - } - if (!output?.trim()) { - throw new Error("Advisor returned an empty response."); - } - return output; - })(), - activeConfig.timeoutMs, - "Advisor generation", - ), - ); + if (!output?.trim()) { + throw new Error("Advisor returned an empty response."); + } + return output; + })(), + activeConfig.timeoutMs, + "Advisor generation", + ), + ); + } catch (err) { + throw withAdvisorProvider(err, activeConfig.provider); + } const modelLabel = support.variant ? `${activeConfig.provider}/${activeConfig.model} (effort=${support.variant})` @@ -2379,6 +2421,7 @@ async function runAdvisor(opts: { benchmarkLoaded, effectiveEffort ?? requestedEffort ?? null, ), + advisorProvider: errorAdvisorProvider(err), }); console.log( `[advisor] session=${sessionId} mode=${mode} trigger=${trigger} outcome=error errorType=${errorType} latencyMs=${latencyMs}`,