diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 860052136..527524ee5 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -1,18 +1,21 @@ # Changelog ## [Unreleased] + ### Added - Added `CheckCredentialsOptions.completionProbe` (and `completionTimeoutMs`) so `AuthStorage.checkCredentials` can additionally exercise each credential against the provider's chat-completion endpoint after refresh-on-expiry. Result lands on `CredentialHealthResult.completion` ({ok, reason?, modelId?, latencyMs?}) without disturbing the usage `ok` field. Public types: `CompletionProbe`, `CompletionProbeInput`, `CompletionProbeCredential`, `CredentialCompletionResult`. The probe is invoked even when no `UsageProvider` is registered for the row, and is skipped when OAuth refresh fails (the stale bytes would only mask the upstream failure). ### Changed +- Changed OpenAI chat request parsing to accept `name` on `tool` messages and fall back to the matching assistant `tool_calls` name, so parsed tool results now carry a proper tool name when the wire omits it - Changed `checkCredentials` to skip running `completionProbe` when OAuth refresh fails, so stale bearer tokens are never probed and the refresh failure remains the returned `reason` - Changed completion reporting to return `completion: { ok: null, reason: ... }` when a credential has no usable bearer bytes instead of attempting the probe - Refactored `AuthStorage.checkCredentials` so OAuth refresh-on-expiry runs up-front and the refreshed credential is shared between the usage probe and the new completion probe; rows without a registered `UsageProvider` no longer short-circuit before the completion probe runs. ### Fixed +- Fixed auth-gateway error classification to extract embedded status codes and use word-boundary matching, so `GenerateContentRequest` and similar messages are no longer misreported as rate-limit errors - Fixed `checkCredentials` to handle `completionProbe` exceptions by recording the failure in `CredentialHealthResult.completion.reason` while still returning the usage probe result ## [15.5.7] - 2026-05-27 diff --git a/packages/ai/src/auth-gateway/server.ts b/packages/ai/src/auth-gateway/server.ts index 4292bfd21..2c1859ef1 100644 --- a/packages/ai/src/auth-gateway/server.ts +++ b/packages/ai/src/auth-gateway/server.ts @@ -191,49 +191,81 @@ function buildStreamOptions(parsed: ParsedFormatRequest, api: Api, signal: Abort /** * Classify an upstream / gateway-internal error into a status code and a - * provider-style error type tag. Used by `handleFormatEndpoint` / - * `handlePassthrough` to drive `route.module.formatError` so every wire - * format emits its native envelope shape. + * format-neutral type. The order is intentional: + * + * 1. Honour an explicit numeric `status` property on the thrown error. + * 2. Parse a status code embedded in the message string. Provider errors + * virtually always carry one (`Google API error (400): …`, `HTTP 429`, + * `status=503`) and the embedded value is authoritative. + * 3. Fall through to **word-boundaried** substring heuristics. The old + * `lower.includes("rate")` test famously matched + * `GenerateContentRequest`, surfacing every Google 400 as a 429 + * `rate_limit_error`. The patterns here all require boundaries so they + * don't collide with provider field names. */ -function classifyGatewayError(err: unknown): { status: number; type: string; message: string } { +export function classifyGatewayError(err: unknown): { status: number; type: string; message: string } { const message = err instanceof Error ? err.message : String(err); - const lower = message.toLowerCase(); - // Custom pi-ai errors may attach a numeric `status` property; honor it - // when present and pick the matching tag. + // 1. Custom pi-ai errors may attach a numeric `status` property. const statusProp = typeof err === "object" && err !== null && typeof (err as { status?: unknown }).status === "number" ? (err as { status: number }).status | 0 : undefined; - if (statusProp !== undefined) { - if (statusProp === 401 || statusProp === 403) - return { status: statusProp, type: "authentication_error", message }; - if (statusProp === 429) return { status: 429, type: "rate_limit_error", message }; - if (statusProp >= 400 && statusProp < 500) return { status: statusProp, type: "invalid_request_error", message }; - if (statusProp >= 500) return { status: statusProp, type: "upstream_error", message }; - } + if (statusProp !== undefined) return bucketStatus(statusProp, message); if (err instanceof Error && err.name === "AbortError") return { status: 499, type: "request_aborted", message }; - if (lower.includes("aborted") || lower.includes("abortsignal")) { + + // 2. Status code embedded in the message. Requires a contextual keyword + // (`HTTP`, `API error`, `status`, …) or a leading `(NNN)` token so we + // don't trip on incidental three-digit numbers ("took 200ms"). + const embedded = extractEmbeddedStatus(message); + if (embedded !== undefined) return bucketStatus(embedded, message); + + // 3. Word-boundaried substring heuristics. + if (/\baborted\b|\babort signal\b/i.test(message)) { return { status: 499, type: "request_aborted", message }; } - if ( - lower.includes("401") || - lower.includes("403") || - lower.includes("unauthorized") || - lower.includes("forbidden") - ) { + if (/\b(?:unauthorized|forbidden)\b/i.test(message)) { return { status: 401, type: "authentication_error", message }; } - if (lower.includes("429") || lower.includes("rate") || lower.includes("quota")) { + if ( + // Match rate-limit phrasings without colliding with + // `GenerateContentRequest`, `accelerate`, `iterate`, `deprecated`, etc. + /\brate[- _]?limit(?:s|ed|ing)?\b|\bquota(?:_exceeded| exceeded)?\b|\btoo[- _]many[- _]requests\b/i.test(message) + ) { return { status: 429, type: "rate_limit_error", message }; } - if (lower.includes("unsupported") || lower.includes("invalid")) { + if (/\b(?:unsupported|invalid_request|invalid request|bad request|malformed)\b/i.test(message)) { return { status: 400, type: "invalid_request_error", message }; } return { status: 502, type: "upstream_error", message }; } +function bucketStatus(status: number, message: string): { status: number; type: string; message: string } { + if (status === 401 || status === 403) return { status, type: "authentication_error", message }; + if (status === 429) return { status, type: "rate_limit_error", message }; + if (status >= 400 && status < 500) return { status, type: "invalid_request_error", message }; + if (status >= 500) return { status, type: "upstream_error", message }; + return { status: 502, type: "upstream_error", message }; +} + +/** + * Pull a status code from common error-message shapes. Returns undefined when + * no contextual keyword is present, so we never guess at incidental numbers. + */ +function extractEmbeddedStatus(message: string): number | undefined { + // `Google API error (400)`, `OpenAI API error (429): …`, `(503)` + // `HTTP 429: too many requests` + // `status: 503`, `status_code=429`, `status=400` + const re = /(?:\bHTTP\b|\bAPI error\b|\bstatus(?:[- _]?code)?\b)\s*[:=]?\s*\(?\s*(\d{3})\b|\((\d{3})\)/i; + const m = message.match(re); + if (!m) return undefined; + const raw = m[1] ?? m[2]; + if (!raw) return undefined; + const code = Number.parseInt(raw, 10); + return Number.isFinite(code) && code >= 100 && code < 600 ? code : undefined; +} + async function refreshGatewayApiKeyAfterAuthError( storage: AuthStorage, model: Model, diff --git a/packages/ai/src/providers/openai-chat-server-schema.ts b/packages/ai/src/providers/openai-chat-server-schema.ts index 727c1f833..4a2cef612 100644 --- a/packages/ai/src/providers/openai-chat-server-schema.ts +++ b/packages/ai/src/providers/openai-chat-server-schema.ts @@ -151,6 +151,15 @@ export const toolMessageSchema = z.object({ role: z.literal("tool"), content: baseContent.optional(), tool_call_id: z.string().optional(), + // OpenAI's wire spec omits `name` on `role:"tool"`, but in practice the + // official Python SDK and several wrappers do send it. Accept it so we can + // honour it downstream (Google's `functionResponse.name` is required and + // non-empty); empty strings are coerced to undefined so the back-resolve + // path runs. + name: z + .string() + .optional() + .transform(v => (v && v.length > 0 ? v : undefined)), }); /** diff --git a/packages/ai/src/providers/openai-chat-server.ts b/packages/ai/src/providers/openai-chat-server.ts index 49e5df61e..053789f46 100644 --- a/packages/ai/src/providers/openai-chat-server.ts +++ b/packages/ai/src/providers/openai-chat-server.ts @@ -59,6 +59,12 @@ export function parseRequest(body: unknown, headers?: Headers): ParsedRequest { const now = Date.now(); const systemParts: string[] = []; const messages: Message[] = []; + // Map of `tool_call_id` → function name, populated as we walk assistant + // turns. The OpenAI wire spec drops `name` from `role:"tool"` messages, + // but downstream providers (notably Google: `functionResponse.name` is + // required) need it. We back-resolve from the matching call. If the + // client did send a wire `name` we still prefer that (forward-compat). + const toolNamesById = new Map(); for (const m of data.messages as OpenAIChatMessage[]) { switch (m.role) { @@ -74,6 +80,13 @@ export function parseRequest(body: unknown, headers?: Headers): ParsedRequest { messages.push({ role: "user", content: parseUserLikeContent(m.content), timestamp: now }); break; case "assistant": + if (m.tool_calls) { + for (const raw of m.tool_calls) { + if (raw.type !== undefined && raw.type !== "function") continue; + const fn = (raw as { function?: { name?: string } }).function; + if (raw.id && fn?.name) toolNamesById.set(raw.id, fn.name); + } + } messages.push( buildAssistantMessage( (m.content ?? undefined) as string | OpenAIChatContentPart[] | undefined, @@ -83,9 +96,15 @@ export function parseRequest(body: unknown, headers?: Headers): ParsedRequest { ), ); break; - case "tool": - pushToolResultMessages(messages, m.content, m.tool_call_id, undefined, now); + case "tool": { + // Prefer the wire `name` when present; otherwise back-resolve from + // the assistant `tool_calls` map. Falls through to "" only when no + // prior call shares this id, which is the well-known broken case. + const wireName = (m as { name?: string }).name; + const resolvedName = wireName ?? (m.tool_call_id ? toolNamesById.get(m.tool_call_id) : undefined); + pushToolResultMessages(messages, m.content, m.tool_call_id, resolvedName, now); break; + } case "function": { // Legacy `function` role (pre-tools API): the message carries the tool's // name on `name` and its output on `content`. Translate to a canonical diff --git a/packages/ai/test/auth-gateway-classify-error.test.ts b/packages/ai/test/auth-gateway-classify-error.test.ts new file mode 100644 index 000000000..c3911f8c2 --- /dev/null +++ b/packages/ai/test/auth-gateway-classify-error.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from "bun:test"; +import { classifyGatewayError } from "../src/auth-gateway/server"; + +describe("auth-gateway classifyGatewayError", () => { + it("honours an explicit numeric `status` property on the error", () => { + const err = Object.assign(new Error("boom"), { status: 503 }); + const c = classifyGatewayError(err); + expect(c.status).toBe(503); + expect(c.type).toBe("upstream_error"); + }); + + it("maps 401/403 to authentication_error via status property", () => { + expect(classifyGatewayError(Object.assign(new Error(""), { status: 401 })).type).toBe("authentication_error"); + expect(classifyGatewayError(Object.assign(new Error(""), { status: 403 })).type).toBe("authentication_error"); + }); + + it("maps 429 to rate_limit_error via status property", () => { + expect(classifyGatewayError(Object.assign(new Error(""), { status: 429 })).type).toBe("rate_limit_error"); + }); + + it("does NOT misclassify `GenerateContentRequest` 400 as rate-limited (the original bug)", () => { + // Verbatim shape Google emits when functionResponse.name is missing. + const msg = + "Google API error (400): * GenerateContentRequest.contents[2].parts[0].function_response.name: Name cannot be empty."; + const c = classifyGatewayError(new Error(msg)); + expect(c.status).toBe(400); + expect(c.type).toBe("invalid_request_error"); + }); + + it("extracts embedded status codes from common message shapes", () => { + const cases: Array<[string, number, string]> = [ + ["OpenAI API error (429): too many requests", 429, "rate_limit_error"], + ["HTTP 503: upstream gone away", 503, "upstream_error"], + ["status: 401 unauthorized", 401, "authentication_error"], + ["status_code=400 — bad json", 400, "invalid_request_error"], + ["Anthropic API error (529): overloaded", 529, "upstream_error"], + ]; + for (const [msg, status, type] of cases) { + const c = classifyGatewayError(new Error(msg)); + expect({ msg, status: c.status, type: c.type }).toEqual({ msg, status, type }); + } + }); + + it("ignores incidental three-digit numbers without a status keyword", () => { + // "took 200ms" should not get classified as 2xx and short-circuit. + const c = classifyGatewayError(new Error("upstream took 200ms then timed out")); + // Falls through all heuristics → default upstream_error/502. + expect(c.status).toBe(502); + }); + + it("still recognizes rate-limit wording when no status is embedded", () => { + const c = classifyGatewayError(new Error("too many requests — back off")); + expect(c.status).toBe(429); + expect(c.type).toBe("rate_limit_error"); + }); + + it("does not match 'rate' inside camelCase or compound words", () => { + // `Generate`, `iterate`, `deprecated`, `accelerate` all contain `rate` as + // a substring and used to trip the classifier. + for (const msg of [ + "GenerateContentRequest validation failed", + "iterate over the candidate list", + "deprecated field on response", + "AccelerateProvider not registered", + ]) { + const c = classifyGatewayError(new Error(msg)); + expect({ msg, status: c.status }).not.toEqual({ msg, status: 429 }); + } + }); + + it("classifies AbortError instances as 499 request_aborted", () => { + const err = new Error("client gave up"); + err.name = "AbortError"; + const c = classifyGatewayError(err); + expect(c.status).toBe(499); + expect(c.type).toBe("request_aborted"); + }); + + it("classifies word-boundaried 'aborted' wording as 499", () => { + const c = classifyGatewayError(new Error("request aborted by caller")); + expect(c.status).toBe(499); + expect(c.type).toBe("request_aborted"); + }); + + it("falls through to 502 upstream_error when nothing matches", () => { + const c = classifyGatewayError(new Error("something inscrutable happened")); + expect(c.status).toBe(502); + expect(c.type).toBe("upstream_error"); + }); +}); diff --git a/packages/ai/test/auth-gateway-openai-chat.test.ts b/packages/ai/test/auth-gateway-openai-chat.test.ts index 31659ace9..29a50d0bd 100644 --- a/packages/ai/test/auth-gateway-openai-chat.test.ts +++ b/packages/ai/test/auth-gateway-openai-chat.test.ts @@ -124,7 +124,9 @@ describe("auth-gateway openai-chat: parseRequest", () => { expect(tool.role).toBe("toolResult"); if (tool.role !== "toolResult") throw new Error("unreachable"); expect(tool.toolCallId).toBe("call_1"); - expect(tool.toolName).toBe(""); + // Back-resolved from the prior assistant `tool_calls[].function.name`. + // Google's `functionResponse.name` is required to be non-empty. + expect(tool.toolName).toBe("lookup"); expect(tool.content).toEqual([{ type: "text", text: "result-text" }]); expect(parsed.context.tools).toHaveLength(1); @@ -150,6 +152,55 @@ describe("auth-gateway openai-chat: parseRequest", () => { expect(parsed.options.maxOutputTokens).toBe(256); expect(parsed.stream).toBe(false); }); + + it("honours an explicit wire `name` on a tool message over back-resolution", () => { + const parsed = parseRequest({ + model: "m", + messages: [ + { role: "user", content: "go" }, + { + role: "assistant", + tool_calls: [{ id: "c1", type: "function", function: { name: "lookup", arguments: "{}" } }], + }, + // SDK-supplied name; takes precedence over the map lookup. + { role: "tool", tool_call_id: "c1", name: "submit_move", content: "ok" }, + ], + }); + const tool = parsed.context.messages.find(m => m.role === "toolResult"); + if (!tool || tool.role !== "toolResult") throw new Error("expected toolResult"); + expect(tool.toolName).toBe("submit_move"); + }); + + it("treats an empty wire `name` as absent and falls back to map lookup", () => { + const parsed = parseRequest({ + model: "m", + messages: [ + { role: "user", content: "go" }, + { + role: "assistant", + tool_calls: [{ id: "c1", type: "function", function: { name: "submit_move", arguments: "{}" } }], + }, + { role: "tool", tool_call_id: "c1", name: "", content: "ok" }, + ], + }); + const tool = parsed.context.messages.find(m => m.role === "toolResult"); + if (!tool || tool.role !== "toolResult") throw new Error("expected toolResult"); + expect(tool.toolName).toBe("submit_move"); + }); + + it("leaves toolName empty when no matching tool_call_id and no wire name", () => { + const parsed = parseRequest({ + model: "m", + messages: [ + { role: "user", content: "go" }, + // orphan tool reply with no prior assistant tool_call + { role: "tool", tool_call_id: "orphan", content: "ok" }, + ], + }); + const tool = parsed.context.messages.find(m => m.role === "toolResult"); + if (!tool || tool.role !== "toolResult") throw new Error("expected toolResult"); + expect(tool.toolName).toBe(""); + }); }); describe("auth-gateway openai-chat: encodeResponse", () => {