diff --git a/.changeset/fail-closed-missing-provider-key.md b/.changeset/fail-closed-missing-provider-key.md new file mode 100644 index 0000000000..4f1146b316 --- /dev/null +++ b/.changeset/fail-closed-missing-provider-key.md @@ -0,0 +1,19 @@ +--- +"@agent-native/core": patch +--- + +Fail closed when an `ai-sdk:*` engine has no provider key, instead of sending an +unauthenticated request. The provider factory was previously built with no +`apiKey`, so the SDK omitted the Authorization header and the gateway's 401 came +back as `http_401` "Missing Authentication header" — a transport error naming the +wrong cause, which a scheduled job then retried on every tick forever. It now +reports `missing_credentials` and names the env var it wants, matching what +`builder-engine` and `anthropic-engine` already did. + +Also stop reaping in-process background automations (scheduler and trigger runs) +at the tight 45s post-claim stale window. That window exists to reach a durable +successor sooner, but these runs carry no `dispatch_payload` and have no +successor to reach, so an early reap killed still-working jobs that nothing could +recover. They now get the 90s background window, and the recovery path reports +`not_redispatchable` rather than `payload_missing`, which read as data loss for +the one case where nothing was ever lost. diff --git a/packages/core/src/agent/engine/ai-sdk-engine.spec.ts b/packages/core/src/agent/engine/ai-sdk-engine.spec.ts index 735e8993c8..48694d4008 100644 --- a/packages/core/src/agent/engine/ai-sdk-engine.spec.ts +++ b/packages/core/src/agent/engine/ai-sdk-engine.spec.ts @@ -472,17 +472,26 @@ describe("AISDKEngine OpenAI model selection", () => { ); }); - it("passes an empty apiKey when env fallback is disabled", async () => { + it("never reaches the deploy key when env fallback is disabled", async () => { vi.stubEnv("OPENAI_API_KEY", "sk-deploy"); - mockAiSdk(); + const { streamText } = mockAiSdk(); const { createOpenAI } = mockOpenAIProvider(); const { createAISDKEngine } = await import("./ai-sdk-engine.js"); const engine = createAISDKEngine("openai", { allowEnvFallback: false }); - await drain(engine.stream(BASE_STREAM_OPTIONS)); + const events: any[] = []; + for await (const e of engine.stream(BASE_STREAM_OPTIONS)) events.push(e); - expect(createOpenAI).toHaveBeenCalledWith({ apiKey: "" }); + // Previously this constructed the provider with `apiKey: ""` so the AI SDK + // could not read the ambient deploy key itself. That kept the deploy key + // out, but shipped a guaranteed-401 unauthenticated request. Failing closed + // keeps the deploy key out just as firmly and reports the real cause. + expect(createOpenAI).not.toHaveBeenCalled(); + expect(streamText).not.toHaveBeenCalled(); + expect(events.find((e) => e.type === "stop")?.errorCode).toBe( + "missing_credentials", + ); }); it("keeps Chat Completions for custom OpenAI-compatible base URLs", async () => { @@ -772,3 +781,73 @@ describe("AISDKEngine streamed tool-input reconciliation", () => { expect(events.some((e) => e.type === "tool-call-error")).toBe(false); }); }); + +describe("AISDKEngine missing-credential fail-closed", () => { + beforeEach(() => { + vi.resetModules(); + vi.unstubAllEnvs(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + // Production: the clips app ran `ai-sdk:openrouter` with no OPENROUTER_API_KEY. + // The provider factory was built with no apiKey, the SDK sent no Authorization + // header, and the gateway's 401 "Missing Authentication header" was classified + // `http_401` — a transport error naming the wrong cause, retried every 30 + // minutes forever. 18/18 scheduled runs failed this way. + it("fails closed with missing_credentials instead of sending an unauthenticated request", async () => { + const { streamText } = mockAiSdk(); + const createOpenRouter = vi.fn(); + vi.doMock("@openrouter/ai-sdk-provider", () => ({ createOpenRouter })); + + const { createAISDKEngine } = await import("./ai-sdk-engine.js"); + const engine = createAISDKEngine("openrouter", { allowEnvFallback: false }); + + const events: any[] = []; + for await (const e of engine.stream(BASE_STREAM_OPTIONS as any)) { + events.push(e); + } + + const stop = events.find((e) => e.type === "stop"); + expect(stop?.reason).toBe("error"); + expect(stop?.errorCode).toBe("missing_credentials"); + expect(stop?.error).toContain("OPENROUTER_API_KEY"); + // The whole point: no request was ever built or sent. + expect(createOpenRouter).not.toHaveBeenCalled(); + expect(streamText).not.toHaveBeenCalled(); + }); + + it("still runs when a key is present", async () => { + const { streamText } = mockAiSdk(); + const provider = vi.fn().mockReturnValue({ id: "m" }); + vi.doMock("@openrouter/ai-sdk-provider", () => ({ + createOpenRouter: vi.fn().mockReturnValue(provider), + })); + + const { createAISDKEngine } = await import("./ai-sdk-engine.js"); + const engine = createAISDKEngine("openrouter", { apiKey: "sk-or-test" }); + await drain(engine.stream(BASE_STREAM_OPTIONS as any)); + + expect(streamText).toHaveBeenCalled(); + }); + + // A self-hosted or local gateway may legitimately accept no credential. + it("allows a keyless provider when a baseUrl is configured", async () => { + const { streamText } = mockAiSdk(); + const provider = vi.fn().mockReturnValue({ id: "m" }); + vi.doMock("@openrouter/ai-sdk-provider", () => ({ + createOpenRouter: vi.fn().mockReturnValue(provider), + })); + + const { createAISDKEngine } = await import("./ai-sdk-engine.js"); + const engine = createAISDKEngine("openrouter", { + allowEnvFallback: false, + baseUrl: "http://localhost:4000/v1", + }); + await drain(engine.stream(BASE_STREAM_OPTIONS as any)); + + expect(streamText).toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/agent/engine/ai-sdk-engine.ts b/packages/core/src/agent/engine/ai-sdk-engine.ts index 8b330e6043..85cf07ea02 100644 --- a/packages/core/src/agent/engine/ai-sdk-engine.ts +++ b/packages/core/src/agent/engine/ai-sdk-engine.ts @@ -23,6 +23,10 @@ import { supportsClaudeAdaptiveThinking, } from "../../shared/reasoning-effort.js"; import { AI_SDK_MODEL_CONFIG, type AISDKProvider } from "../model-config.js"; +import { + LLM_MISSING_CREDENTIALS_ERROR_CODE, + LLM_MISSING_CREDENTIALS_MESSAGE, +} from "./credential-errors.js"; import { classifyProviderError, describeErrorWithCauses, @@ -228,6 +232,8 @@ class AISDKEngine implements AgentEngine { private readonly provider: AISDKProvider; private readonly apiKey?: string; private readonly baseUrl?: string; + /** Empty for providers that need no key (ollama). */ + private readonly requiredEnvVars: readonly string[]; private readonly appName?: string; private readonly appUrl?: string; @@ -243,12 +249,34 @@ class AISDKEngine implements AgentEngine { this.apiKey = config.apiKey ?? (config.allowEnvFallback === false ? "" : getProviderApiKey(provider)); + this.requiredEnvVars = PROVIDER_ENV_VARS[provider]; this.baseUrl = config.baseUrl; this.appName = config.appName; this.appUrl = config.appUrl; } async *stream(opts: EngineStreamOptions): AsyncIterable { + // An absent key is not an anonymous request. Without this the provider + // factory is constructed with no `apiKey`, the SDK omits the Authorization + // header entirely, and the gateway's 401 comes back as + // "Missing Authentication header" — which `classifyProviderError` codes + // `http_401`, a transport failure naming the wrong cause. A scheduled job + // then repeats that doomed unauthenticated request on every tick forever. + // `builder-engine` and `anthropic-engine` already fail closed here; this + // engine was the only one that did not. + // + // A configured `baseUrl` is exempt: a self-hosted or local gateway may + // legitimately accept unauthenticated requests. + if (!this.apiKey && !this.baseUrl && this.requiredEnvVars.length > 0) { + yield { + type: "stop", + reason: "error", + error: `${LLM_MISSING_CREDENTIALS_MESSAGE} (engine "${this.name}" has no ${this.requiredEnvVars.join(" or ")})`, + errorCode: LLM_MISSING_CREDENTIALS_ERROR_CODE, + }; + return; + } + let aiModule: any; try { aiModule = await import("ai"); diff --git a/packages/core/src/agent/run-store.stale-run-recovery.spec.ts b/packages/core/src/agent/run-store.stale-run-recovery.spec.ts index b7ebf0d8ab..c052169ff6 100644 --- a/packages/core/src/agent/run-store.stale-run-recovery.spec.ts +++ b/packages/core/src/agent/run-store.stale-run-recovery.spec.ts @@ -85,6 +85,7 @@ const { markTurnAborted, reapIfStale, reapAllStaleRuns, + BACKGROUND_PROCESSING_RUN_STALE_MS, __resetNoRunningRunsProbeForTests, } = await import("./run-store.js"); @@ -310,8 +311,8 @@ describe("FIX 3 — stale-run reaper server-owned recovery (reapIfStale)", () => it("does NOT create a successor when the dying run has no dispatch_payload to carry over", async () => { currentClient = makeRawClient(true); const { runId, thread, turn } = ids(); - // No dispatchPayload — e.g. a row whose payload was already cleared, or - // one that predates this fix. + // No dispatchPayload — an in-process background automation, which was never + // HTTP-dispatched and so has no request body to rehydrate. await insertRun(runId, thread, turn, { dispatchMode: "background" }); await claimBackgroundRun(runId); setStaleLiveness(runId, Date.now() - STALE_PAST_MS); @@ -319,10 +320,44 @@ describe("FIX 3 — stale-run reaper server-owned recovery (reapIfStale)", () => const reaped = await reapIfStale(runId); expect(reaped).toBe(true); expect(readRow(runId)?.status).toBe("errored"); - expect(readRow(runId)?.diag_stage).toContain("payload_missing"); + // Named for what it is. "payload_missing" read as data loss in production + // forensics for the one case where nothing was ever lost. + expect(readRow(runId)?.diag_stage).toContain("not_redispatchable"); expect(rowsForTurn(turn)).toHaveLength(1); // no successor inserted }); + it("gives a claimed run with no redispatch path the wider background stale window", async () => { + currentClient = makeRawClient(true); + // A payload-less claimed run is an in-process automation: reaping it at the + // 45s post-claim window kills a job that is still working and that nothing + // can recover. It must survive to the 90s background window instead. + const noPayload = ids(); + await insertRun(noPayload.runId, noPayload.thread, noPayload.turn, { + dispatchMode: "background", + }); + await claimBackgroundRun(noPayload.runId); + setStaleLiveness( + noPayload.runId, + Date.now() - (BACKGROUND_PROCESSING_RUN_STALE_MS + 5_000), + ); + expect(await reapIfStale(noPayload.runId)).toBe(false); + expect(readRow(noPayload.runId)?.status).toBe("running"); + + // A genuine HTTP worker (payload present) keeps the tight window, because + // for it an early reap does buy a durable successor. + const withPayload = ids(); + await insertRun(withPayload.runId, withPayload.thread, withPayload.turn, { + dispatchMode: "background", + dispatchPayload: JSON.stringify({ threadId: withPayload.thread }), + }); + await claimBackgroundRun(withPayload.runId); + setStaleLiveness( + withPayload.runId, + Date.now() - (BACKGROUND_PROCESSING_RUN_STALE_MS + 5_000), + ); + expect(await reapIfStale(withPayload.runId)).toBe(true); + }); + it("does NOT create a successor when a newer run already exists for the same turn", async () => { currentClient = makeRawClient(true); const { runId, thread, turn } = ids(); diff --git a/packages/core/src/agent/run-store.ts b/packages/core/src/agent/run-store.ts index aca9967d2c..c339d183db 100644 --- a/packages/core/src/agent/run-store.ts +++ b/packages/core/src/agent/run-store.ts @@ -736,7 +736,13 @@ function backgroundAwareStaleCutoffSql(): string { // `CAST(? AS BIGINT)` is required: without it Postgres infers the param as // int4 from the int4 window literals, so the bound `Date.now()` ms epoch // overflows int4. The cast keeps the subtraction 64-bit; a no-op on SQLite. - return `(CAST(? AS BIGINT) - CASE WHEN dispatch_mode = 'background-processing' THEN ${BACKGROUND_PROCESSING_RUN_STALE_MS} WHEN dispatch_mode LIKE 'background%' THEN ${BACKGROUND_RUN_STALE_MS} ELSE ${RUN_STALE_MS} END)`; + // The tight post-claim window buys ONE thing: reaching the durable successor + // sooner (see BACKGROUND_PROCESSING_RUN_STALE_MS). A row with no + // `dispatch_payload` has no successor to reach — `attemptStaleRunRecovery` + // declines it as `not_redispatchable` — so reaping it early is pure loss: it + // kills an in-process background automation (scheduler/trigger) that is still + // working, and nothing recovers it. Those get the wider background window. + return `(CAST(? AS BIGINT) - CASE WHEN dispatch_mode = 'background-processing' AND dispatch_payload IS NOT NULL THEN ${BACKGROUND_PROCESSING_RUN_STALE_MS} WHEN dispatch_mode LIKE 'background%' THEN ${BACKGROUND_RUN_STALE_MS} ELSE ${RUN_STALE_MS} END)`; } function terminalRunEventExclusionSql(runIdColumn = "id"): string { @@ -1546,7 +1552,7 @@ interface StaleRunRecoverySuccessor { type StaleRunRecoveryOutcome = | ({ outcome: "recovered" } & StaleRunRecoverySuccessor) | { outcome: "not_background" } - | { outcome: "payload_missing" } + | { outcome: "not_redispatchable" } | { outcome: "newer_run_exists" } | { outcome: "budget_exhausted" } | { outcome: "repeated_no_progress" }; @@ -1596,7 +1602,9 @@ function staleRecoveryDispatchPayload(payload: string): string { * - the row is a background chat-turn dispatch (`dispatch_mode` starting * with "background") — a foreground/foreground-self-chain run has a * connected client to recover it via its own `auto_continue` re-POST. - * - its `dispatch_payload` is still present — without it there is nothing + * - its `dispatch_payload` is still present (an in-process automation run + * never had one and is declined as `not_redispatchable`, not as a loss) + * — without it there is nothing * to rehydrate the successor's request body from. Read HERE, before the * caller's terminal write (which NULLs `dispatch_payload` on every other * path), so it survives long enough to carry over. @@ -1633,7 +1641,14 @@ async function attemptStaleRunRecovery( } const payload = row.dispatch_payload; if (typeof payload !== "string" || payload.length === 0) { - return { outcome: "payload_missing" }; + // Not an anomaly, and specifically NOT a lost payload: every HTTP-dispatched + // background run writes `dispatch_payload` in the same INSERT that creates + // the row, so a payload-less row is one that was never HTTP-dispatched at + // all — an in-process automation from `runBackgroundAutomation`. It has no + // request body to rehydrate because there was never a request. Naming this + // `payload_missing` read as "we are losing payloads" in production + // forensics and sent a reader hunting a bug that does not exist. + return { outcome: "not_redispatchable" }; } const threadId = row.thread_id; const turnId = row.turn_id ?? runId;