From 6f76a6a9da8695d517258ed8ddedf9e8cebc847d Mon Sep 17 00:00:00 2001 From: "pullfrog[bot]" <226033991+pullfrog[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 16:31:05 +0000 Subject: [PATCH] fix(action): tighten provider error detection and propagate agent error events (#580) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(action): tighten provider error detection and propagate agent error events Both bugs from #562: 1. detectProviderError used substring matches against "429", "rate limit", etc. — false-positives on commit SHAs containing 429 and on x-ratelimit-* response headers in dumped 401 error JSON. rewrote with anchored regexes: numeric status codes only match adjacent to a recognised status key, and `\brate[_ ]limit(?=[_ ]|\b)` rejects ratelimit-* headers (no separator). word-boundary anchors on INTERNAL / UNAVAILABLE / quota / limit:0 reject INTERNAL_SERVER_ERROR / time_limit:0 substrings. added 11-case regression test. 2. opencode 401s slipped through `eventCount === 0 && lastProviderError` because opencode's own type=error event increments eventCount before the guard runs. added an explicit `error:` handler that captures the event and propagates it to a non-success AgentResult. opencode emits the message under `error.data.message`, not the top level. mirror fix in claude.ts: error_max_turns / error_during_execution / any error* subtype on the result event now flips success: false. * fix(action): match quota inside identifiers like insufficient_quota \bquota\b missed insufficient_quota / quota_exceeded / quotaExceeded because _ is a word character and camelCase has no boundary. quota is specific enough to be matched as a plain substring. * fix(action): match `rate limited` and `rate limits exceeded` Drop the trailing `(?=[_ ]|\b)` lookahead from the rate-limit regex. The lookahead failed when `limit` was followed by another word character (`limited`, `limits`), so `rate limited` and `rate limits exceeded` were slipping past detection. The leading `\b` plus `[_ ]` separator already rejects `x-ratelimit-*` / `anthropic-ratelimit-*` headers without it. --------- Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass --- agents/claude.ts | 16 ++++++++ agents/opencode.ts | 32 ++++++++++++++- utils/providerErrors.test.ts | 80 ++++++++++++++++++++++++++++++++++++ utils/providerErrors.ts | 42 ++++++++++++++----- 4 files changed, 158 insertions(+), 12 deletions(-) create mode 100644 utils/providerErrors.test.ts diff --git a/agents/claude.ts b/agents/claude.ts index 8eb6e62..53ddc45 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -210,6 +210,7 @@ async function runClaude(params: RunParams): Promise { let finalOutput = ""; let sessionId: string | undefined; + let resultErrorSubtype: string | null = null; let accumulatedTokens = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }; // Claude CLI reports a single end-of-run `total_cost_usd` on the result // event. per-message events don't carry cost, so there's nothing to sum — @@ -367,9 +368,14 @@ async function runClaude(params: RunParams): Promise { tokensLogged = true; } } else if (subtype === "error_max_turns") { + resultErrorSubtype = subtype; log.info(`» ${params.label} max turns reached: ${JSON.stringify(event)}`); } else if (subtype === "error_during_execution") { + resultErrorSubtype = subtype; log.info(`» ${params.label} execution error: ${JSON.stringify(event)}`); + } else if (subtype.startsWith("error")) { + resultErrorSubtype = subtype; + log.info(`» ${params.label} result: subtype=${subtype}, data=${JSON.stringify(event)}`); } else { log.info(`» ${params.label} result: subtype=${subtype}, data=${JSON.stringify(event)}`); } @@ -527,6 +533,16 @@ async function runClaude(params: RunParams): Promise { }; } + if (resultErrorSubtype) { + return { + success: false, + output: finalOutput || output, + error: `result subtype: ${resultErrorSubtype}`, + usage, + sessionId, + }; + } + return { success: true, output: finalOutput || output, usage, sessionId }; } catch (error) { params.todoTracker?.cancel(); diff --git a/agents/opencode.ts b/agents/opencode.ts index ad5ca37..3ffed68 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -254,7 +254,13 @@ interface OpenCodeErrorEvent { type: "error"; timestamp?: string; sessionID?: string; - error?: { name?: string; message?: string; data?: unknown; [key: string]: unknown }; + // opencode emits the error message under `error.data.message`, not at the + // top level. see anomalyco/opencode packages/opencode/src/cli/cmd/run.ts. + error?: { + name?: string; + data?: { message?: string; [key: string]: unknown }; + [key: string]: unknown; + }; [key: string]: unknown; } @@ -630,6 +636,16 @@ async function runOpenCode(params: RunParams): Promise { log.debug(withLabel(label, `tool output: ${outputStr}`)); } }, + error: (event: OpenCodeErrorEvent) => { + // opencode emits a `type=error` event when a provider call fails (e.g. + // 401 Invalid authentication credentials). the underlying CLI still + // exits 0 because the error was returned cleanly by the LLM SDK, so + // unless we capture this event the run is reported as success. + agentErrorEvent = event; + const errorName = event.error?.name || "unknown"; + const errorMessage = event.error?.data?.message || event.error?.name || JSON.stringify(event); + log.info(`» ${params.label} error event: ${errorName}: ${errorMessage}`); + }, result: async (event: OpenCodeResultEvent) => { const status = event.status || "unknown"; const duration = event.stats?.duration_ms || 0; @@ -663,6 +679,7 @@ async function runOpenCode(params: RunParams): Promise { const recentStderr: string[] = []; let lastProviderError: string | null = null; + let agentErrorEvent: OpenCodeErrorEvent | null = null; let output = ""; let stdoutBuffer = ""; @@ -824,6 +841,19 @@ async function runOpenCode(params: RunParams): Promise { }; } + if (agentErrorEvent) { + const errorEvent: OpenCodeErrorEvent = agentErrorEvent; + const errorName = errorEvent.error?.name || "agent error"; + const errorMessage = + errorEvent.error?.data?.message || errorEvent.error?.name || JSON.stringify(errorEvent); + return { + success: false, + output: finalOutput || output, + error: `${errorName}: ${errorMessage}`, + usage, + }; + } + return { success: true, output: finalOutput || output, usage }; } catch (error) { params.todoTracker?.cancel(); diff --git a/utils/providerErrors.test.ts b/utils/providerErrors.test.ts new file mode 100644 index 0000000..3bc3ea5 --- /dev/null +++ b/utils/providerErrors.test.ts @@ -0,0 +1,80 @@ +import { detectProviderError } from "./providerErrors.ts"; + +describe("detectProviderError", () => { + describe("false positives previously seen in production", () => { + it("returns null for commit SHAs containing 429", () => { + expect(detectProviderError("hash=7a46d89f505b36df49b4f54429daffa1a9459b11")).toBeNull(); + expect(detectProviderError("commit f609cc89e84596ab125d60dac568bfb2ef398396 429")).toBeNull(); + }); + + it("returns null for x-ratelimit-* response headers in 401 error JSON", () => { + const stderr = JSON.stringify({ + error: { name: "APIError", statusCode: 401, message: "Invalid authentication credentials" }, + headers: { + "x-ratelimit-limit-requests": 50, + "x-ratelimit-remaining-requests": 49, + "x-ratelimit-reset-tokens": "2025-01-01T00:00:00Z", + }, + }); + expect(detectProviderError(stderr)).toBeNull(); + }); + + it("returns null for INTERNAL_SERVER_ERROR substring", () => { + expect(detectProviderError("HTTP/1.1 500 INTERNAL_SERVER_ERROR")).toBeNull(); + expect(detectProviderError("expected: not INTERNAL_SERVER_ERROR")).toBeNull(); + }); + + it("returns null for INTERNALS substring", () => { + expect(detectProviderError("debugging INTERNALS of the parser")).toBeNull(); + }); + }); + + describe("real provider errors", () => { + it("detects 429 only when adjacent to a status key", () => { + expect(detectProviderError('{"statusCode": 429}')).toBe("rate limited (429)"); + expect(detectProviderError('{"status_code": 429, "message": "..."}')).toBe( + "rate limited (429)" + ); + expect(detectProviderError("http_status: 429")).toBe("rate limited (429)"); + expect(detectProviderError("status=429")).toBe("rate limited (429)"); + }); + + it("detects rate_limit_error and rate_limit_exceeded", () => { + expect(detectProviderError('{"type":"rate_limit_error"}')).toBe("rate limited"); + expect(detectProviderError("rate_limit_exceeded")).toBe("rate limited"); + expect(detectProviderError("plain rate limit reached")).toBe("rate limited"); + }); + + it("detects rate-limit phrasing with trailing inflection", () => { + expect(detectProviderError("Error: rate limited by provider")).toBe("rate limited"); + expect(detectProviderError("rate limits exceeded for this key")).toBe("rate limited"); + }); + + it("detects RESOURCE_EXHAUSTED", () => { + expect(detectProviderError('"status": "RESOURCE_EXHAUSTED"')).toBe("quota exhausted"); + }); + + it("detects gRPC INTERNAL status as a whole word", () => { + expect(detectProviderError('"status": "INTERNAL"')).toBe("provider internal error"); + }); + + it("detects UNAVAILABLE as a whole word", () => { + expect(detectProviderError('"status": "UNAVAILABLE"')).toBe("provider unavailable"); + }); + + it("detects 500 / 503 only when adjacent to a status key", () => { + expect(detectProviderError('"statusCode": 500')).toBe("provider 500 error"); + expect(detectProviderError('"statusCode": 503')).toBe("provider unavailable (503)"); + expect(detectProviderError("v1.503.0 release notes")).toBeNull(); + }); + + it("detects quota and zero-quota responses", () => { + expect(detectProviderError('"message": "quota exceeded"')).toBe("quota error"); + expect(detectProviderError('{"code":"insufficient_quota"}')).toBe("quota error"); + expect(detectProviderError('"error":"quota_exceeded"')).toBe("quota error"); + expect(detectProviderError('{"reason":"quotaExceeded"}')).toBe("quota error"); + expect(detectProviderError('{"limit": 0, "remaining": 0}')).toBe("zero quota"); + expect(detectProviderError('"time_limit": 0')).toBeNull(); + }); + }); +}); diff --git a/utils/providerErrors.ts b/utils/providerErrors.ts index 65a5380..aece9ca 100644 --- a/utils/providerErrors.ts +++ b/utils/providerErrors.ts @@ -1,18 +1,38 @@ -const PROVIDER_ERROR_PATTERNS = [ - { pattern: "429", label: "rate limited (429)" }, - { pattern: "RESOURCE_EXHAUSTED", label: "quota exhausted" }, - { pattern: "quota", label: "quota error" }, - { pattern: "status: 500", label: "provider 500 error" }, - { pattern: "INTERNAL", label: "provider internal error" }, - { pattern: "status: 503", label: "provider unavailable (503)" }, - { pattern: "UNAVAILABLE", label: "provider unavailable" }, - { pattern: "rate limit", label: "rate limited" }, - { pattern: "limit: 0", label: "zero quota" }, +type ProviderErrorPattern = { regex: RegExp; label: string }; + +// status codes are only treated as provider errors when they are adjacent to +// a recognised status key. this rejects commit SHAs that happen to contain +// "429", version strings, file hashes, etc. +const statusKey = `\\b(?:status[_ ]?code|http[_ ]?status|status)["']?\\s*[:=]\\s*["']?`; + +const PROVIDER_ERROR_PATTERNS: ProviderErrorPattern[] = [ + { regex: new RegExp(`${statusKey}429\\b`, "i"), label: "rate limited (429)" }, + { regex: new RegExp(`${statusKey}500\\b`, "i"), label: "provider 500 error" }, + { regex: new RegExp(`${statusKey}503\\b`, "i"), label: "provider unavailable (503)" }, + // matches `rate limit`, `rate limited`, `rate limits exceeded`, + // `rate_limit_error`, `rate_limit_exceeded`. the leading `\b` + `[_ ]` + // separator rejects `x-ratelimit-*` / `anthropic-ratelimit-*` response + // headers (no separator between "rate" and "limit") which routinely + // appear in dumped 401 / 4xx error JSON. + { regex: /\brate[_ ]limit/i, label: "rate limited" }, + { regex: /\bRESOURCE_EXHAUSTED\b/, label: "quota exhausted" }, + // Google gRPC `INTERNAL` status. word-boundary anchors reject + // `INTERNAL_SERVER_ERROR` (HTTP 500 message that may appear in unrelated + // log lines) and identifiers like `INTERNALS`. + { regex: /\bINTERNAL\b/, label: "provider internal error" }, + { regex: /\bUNAVAILABLE\b/, label: "provider unavailable" }, + // matches `quota`, `insufficient_quota`, `quota_exceeded`, `quotaExceeded`. + // word-character lookarounds would reject `_quota` / `quotaX`; `quota` is + // specific enough that a plain substring match is safe. + { regex: /quota/i, label: "quota error" }, + // explicit zero-quota response, e.g. `{"limit": 0}`. the `\b` anchor + // around `limit` rejects keys like `time_limit` or `field_limit`. + { regex: /["']?\blimit\b["']?\s*:\s*0\b/, label: "zero quota" }, ]; export function detectProviderError(text: string): string | null { for (const entry of PROVIDER_ERROR_PATTERNS) { - if (text.includes(entry.pattern)) return entry.label; + if (entry.regex.test(text)) return entry.label; } return null; }