fix(ai): resolved auth-gateway classification by status-code precedence

- Added optional `name` support to tool message schema, coercing blank values to undefined.
- Tracked assistant tool-call IDs to resolve `tool` message names from prior calls when wire names are missing.
- Updated auth-gateway classification to prefer explicit/embedded status codes over text heuristics.
- Added word-boundary matching, expanded parse/error tests, and updated changelog entries.
This commit is contained in:
can1357
2026-05-28 01:45:32 +02:00
parent 7c64576524
commit 230ca08409
6 changed files with 230 additions and 26 deletions
+3
View File
@@ -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
+55 -23
View File
@@ -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<Api>,
@@ -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)),
});
/**
@@ -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<string, string>();
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
@@ -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");
});
});
@@ -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", () => {