From fe2746198c16886b28892463357aef83fac5a018 Mon Sep 17 00:00:00 2001 From: David Blass Date: Tue, 26 May 2026 16:53:18 +0000 Subject: [PATCH] fix: 6 unaddressed log-audit / run-audit findings (#840) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: 6 unaddressed log-audit / run-audit findings - #836 + #818 (clerk middleware SyntaxError on action-runtime endpoints): narrow proxy.ts matcher to exclude /api/repo///run-context, /api/runtime/*, /api/proxy-token. these are server-to-server with their own auth and have no Clerk session to evaluate, so clerkMiddleware's decodeJwt throws turn into 500s on every request. - #837 (npx EBADDEVENGINES on customer's package.json): change runCli's bootstrap cwd from $GITHUB_WORKSPACE to os.tmpdir() so npm v11+ doesn't enforce devEngines.packageManager from the customer's tree before our bootstrap can install pullfrog. CLI process.chdir's to payload.cwd internally, so the runtime work still happens in $GITHUB_WORKSPACE. - #838 (createWorkflowDispatch silently dropped 39 user runs on a 5xx spike): add bounded retry (3 attempts, ~750ms total) on Octokit 5xx and network errors inside dispatchReservedRun. preserves the existing 422 "Unexpected inputs" path. retry budget stays well under GitHub's 10s webhook redelivery window. - #833 (bail() redirect("/signout") propagated NEXT_REDIRECT into webhook handlers, 40+ 500s/24h): drop the redirect side-effect; bail now just classifies bad-credentials as non-retryable and propagates. UI flows that wanted auto-signout on revoked tokens can detect it themselves; the side-effect was wrong for any non-page caller. - #835 (BYOK provider billing-exhausted fell through to raw error renderer, 37 review-mode runs/24h with no PR-side signal): - extend providerErrors patterns with Anthropic "credit balance is too low" + extract isProviderBillingExhausted / extractProviderId helpers - add a renderer branch in runErrorRenderer.ts that names the provider (parsed from providerID=) and links to its billing dashboard - route handleAgentResult's !result.success path through the renderer + reportErrorToComment with createIfMissing, so review-mode and silent triggers get an actionable PR comment regardless of mode - #834 (post-hook ERR_MODULE_NOT_FOUND already fixed on main, hardening): - add a vitest invariant that walks the entryPost.ts import graph and refuses any non-relative / non-node: specifier — catches the next `@actions/core` slip-up before publish - add an analyze-logs classifier so future entryPost crashes surface as failure:post-hook-module-not-found instead of hiding inside failure:unknown / failure:git-lock-file Co-authored-by: Cursor * anneal: fix dead-code matcher, 404 url, silent-trigger gate, and grammar regression Round-1 anneal pass on the audit-fixes PR surfaced two critical issues + several majors that the original fixes shipped with: - proxy.ts matcher 1 (`(?!_next|[^?]*\.(...))`) still caught every /api/... route because matcher arrays are OR'd. The narrowing in matcher 2 was dead code → middleware still 500'd on /api/runtime/*, /api/proxy-token, /api/repo/.../run-context. Carve-out now lives in BOTH matchers. - opencode.ai/billing returns 404; canonical top-up surface is /zen. deepseek /usage is the consumption page, not the top-up flow (/top_up is correct). google /apikey is the keys list, not billing (/usage is the spend dashboard). All three URL strings updated. - handleAgentResult gated reportErrorToComment behind `if (!ctx.silent)`, contradicting the createIfMissing intent — silent IncrementalReview / pull_request_synchronize / auto-label still got zero PR signal on BYOK billing exhaustion, the exact failure mode #835 was meant to fix. Moved createIfMissing into finalizeSuccessRun's existing render-and- post block (single source of truth), reverted handleAgentResult to its prior shape. Side benefit: drops the double-PATCH that fired on every non-silent !success path with an existing progress comment. - Anthropic-direct error rendered "**Your your provider account is out of credit.**" because Anthropic SDK has no providerID= tag, so extractProviderId returned null and the headline composed "Your " + "your provider". Added detectProviderId Anthropic fallback (matches "Anthropic API" / "credit balance is too low") so the link is reachable AND made the headline conditional on whether a provider id was detected. - Reordered renderRunError classifier: BYOK billing-exhausted now runs BEFORE api-key auth detection. Providers commonly return 401 for billing exhaustion (DeepSeek, Gemini), and the OpenCode harness logs often include "API Error: 401" in the raw error body, which isApiKeyAuthError would otherwise match — surfacing "rotate your key" when the actual fix is "top up credits". - isTransientUpstreamError missed ENOTFOUND / ENETUNREACH / EHOSTUNREACH (undici DNS-class failures Octokit doesn't wrap with a status). Added to the prefix alternation. - Tightened "10s webhook redelivery budget" / "GitHub redelivers" wording in triggerWorkflow.ts and bail.ts JSDoc — GitHub's 10s is the response timeout (it doesn't auto-redeliver); upstream webhook proxy retries are what multiplied the failure. Co-authored-by: Cursor * anneal r2: unbreak proxy.ts matcher 2, extend carve-out, mkdtemp bootstrap Round-2 anneal (security + cross-cutting lenses) on top of the round-1 audit-fixes commit caught a critical regression + several majors: - proxy.ts matcher 2 from r1 (`/(api|trpc)(?!...)(.*)`) does NOT compile. path-to-regexp rejects a top-level `(?!` after `)` as "Pattern cannot start with '?'", and Next.js's SourceSchema runs the same validator at build time and aborts via `process.exit(1)`. PR #840's Vercel + preview deployments have been failing since 978eca26 for exactly this reason. Fixed by nesting the lookahead inside an outer parameter group: `/(api|trpc)((?!...).*)`. Same shape matcher 1 already uses, which is why m1 always compiled. Verified `pnpm next build` succeeds end-to-end. - proxy.ts carve-out was incomplete relative to its own justification. The "no Clerk session, decodeJwt 500" failure mode applies to ALL server-to-server action-runtime endpoints — five more share the exact shape: /api/repo/.../learnings, /api/repo/.../pr/.../summary-comment, /api/repo/.../issue/.../plan-comment, /api/workflow-run/, and /api/github/installation-token. Extended both matchers. /api/upload/ signed-url stays in (dual auth: Clerk session OR bearer JWT — needs middleware for the user path). - proxy.ts carve-outs were unanchored: a future /api/proxy-token-info, /api/proxy-tokens, or /api/repo/X/Y/run-context-foo would silently bypass Clerk. Added `(?:$|/)` for path-prefix carve-outs (allow exact match or sub-path), `$` for routes with browser-callable siblings (e.g. `learnings/history` is browser-Clerk, `learnings$` is action- bearer-JWT). Verified 30/31 routes via path-to-regexp test harness. - runCli.ts cwd flipped from $GITHUB_WORKSPACE to os.tmpdir() in r1 (#837 fix for npm v11 devEngines.packageManager EBADDEVENGINES). But $TMPDIR is overridable from a prior $GITHUB_ENV step — a customer- authored or compromised prior step can plant /atk/node_modules/ pullfrog/ and `echo "TMPDIR=/atk" >> $GITHUB_ENV`, and our npx --yes pullfrog@ bootstrap resolves the local install first, executing attacker code with full action env (provider keys, OIDC, installation token, CODEX_AUTH_JSON). Switched to mkdtempSync(join (tmpdir(), "pullfrog-bootstrap-")) — fresh per-invocation 0700 dir, not pre-writable by anything earlier in the job. - runLifecycle.ts: writeRunErrorOutputs (catch-path) didn't pass createIfMissing: true, contradicting the symmetric intent of the r1 finalizeSuccessRun fix. Silent triggers (IncrementalReview / pull_request_synchronize / auto-label) that throw past the success path still got zero PR signal — exact failure mode #835 was meant to close. Now both paths pass createIfMissing: true. - runErrorRenderer.ts detectProviderId regex `/Anthropic API|credit balance is too low/i` could mis-tag a non-Anthropic billing-exhausted error that mentioned "Anthropic API" in passing (fallback-chain agent prompt text, OpenCode harness logs). Tightened to /credit balance is too low/i — Anthropic-specific phrasing, sufficient for the direct- Anthropic SDK case the fallback exists to handle. - runErrorRenderer.ts JSDoc: r1's classifier reorder put hang at #6 in code but the JSDoc still listed it at #2. Reordered the doc to match dispatch order, with explicit note that hang is a sub-source for the api-key check (which is why hangBody is precomputed early). - analyze-logs.ts: ERR_MODULE_NOT_FOUND.*entryPost regex needs `s` flag so it survives Node v23+ stack-trace reformatting onto multiple lines. One-char fix to defend the #834 classification bucket. Co-authored-by: Cursor --------- Co-authored-by: Cursor --- entryPost.stdlibOnly.test.ts | 96 +++++++++++++++++++++++++++++ runCli.ts | 18 +++++- utils/providerErrors.test.ts | 52 ++++++++++++++++ utils/providerErrors.ts | 44 +++++++++++--- utils/run.ts | 5 ++ utils/runErrorRenderer.test.ts | 52 ++++++++++++++++ utils/runErrorRenderer.ts | 106 ++++++++++++++++++++++++++++++--- utils/runLifecycle.ts | 46 ++++++++++---- 8 files changed, 387 insertions(+), 32 deletions(-) create mode 100644 entryPost.stdlibOnly.test.ts diff --git a/entryPost.stdlibOnly.test.ts b/entryPost.stdlibOnly.test.ts new file mode 100644 index 0000000..d9770b5 --- /dev/null +++ b/entryPost.stdlibOnly.test.ts @@ -0,0 +1,96 @@ +import { readFileSync } from "node:fs"; +import { dirname, resolve } from "node:path"; +import { describe, expect, it } from "vitest"; + +// The GHA `post:` hook runs `node action/entryPost.ts` directly against the +// rsynced action checkout, which deliberately excludes `node_modules`. Any +// non-relative / non-`node:` import in entryPost.ts (or in its transitive +// imports) crashes the post-step with `ERR_MODULE_NOT_FOUND` AFTER the agent +// already exited 0, flipping the workflow to `failure`. see #834. +// +// This test parses the static-import graph rooted at entryPost.ts and refuses +// any specifier that isn't one of: +// - node:* (stdlib) +// - ./* or ../* (relative) +// +// Any other specifier (`@actions/core`, `pullfrog`, `zod`, etc.) means the +// post-hook will need a `node_modules` tree the rsync drops. + +const ENTRY_FILE = resolve(import.meta.dirname, "entryPost.ts"); + +const IMPORT_RE = /^\s*(?:import|export)(?:\s+(?:type\s+)?[\s\S]*?)?\s+from\s+["']([^"']+)["']/gm; +const SIDE_EFFECT_RE = /^\s*import\s+["']([^"']+)["']/gm; +// `import.meta.glob` and friends are not used in entryPost.ts; the simple +// regex above is sufficient here. expand if a transitive dep starts using +// dynamic imports for stdlib-only logic. + +function extractImports(filePath: string): string[] { + const source = readFileSync(filePath, "utf8"); + const specs: string[] = []; + for (const re of [IMPORT_RE, SIDE_EFFECT_RE]) { + re.lastIndex = 0; + for (const m of source.matchAll(re)) specs.push(m[1]); + } + return specs; +} + +function isAllowed(spec: string): boolean { + return spec.startsWith("node:") || spec.startsWith("./") || spec.startsWith("../"); +} + +type WalkResult = { + visited: Set; + violations: { file: string; spec: string }[]; +}; + +function walk(start: string): WalkResult { + const visited = new Set(); + const violations: WalkResult["violations"] = []; + const queue: string[] = [start]; + + while (queue.length > 0) { + const file = queue.shift()!; + if (visited.has(file)) continue; + visited.add(file); + + for (const spec of extractImports(file)) { + if (!isAllowed(spec)) { + violations.push({ file, spec }); + continue; + } + if (spec.startsWith("node:")) continue; + const resolved = resolve(dirname(file), spec); + const candidate = resolved.endsWith(".ts") ? resolved : `${resolved}.ts`; + try { + readFileSync(candidate, "utf8"); + queue.push(candidate); + } catch { + // non-.ts (e.g. JSON `with { type: "json" }`) — already classified + // as relative-allowed above. nothing further to walk. + } + } + } + return { visited, violations }; +} + +describe("entryPost.ts stdlib-only invariant (#834)", () => { + it("only imports node: builtins and relative siblings (no node_modules deps)", () => { + const result = walk(ENTRY_FILE); + expect(result.violations, JSON.stringify(result.violations, null, 2)).toEqual([]); + }); + + it("walks the full transitive graph (entryPost + 3 utils)", () => { + const result = walk(ENTRY_FILE); + expect(result.visited.size).toBeGreaterThanOrEqual(4); + }); + + it("matches the modules entryPost actually imports today", () => { + const direct = extractImports(ENTRY_FILE).sort(); + expect(direct).toEqual([ + "./utils/codexRefreshDetect.ts", + "./utils/ghaCore.ts", + "./utils/postApiFetch.ts", + "node:fs", + ]); + }); +}); diff --git a/runCli.ts b/runCli.ts index 2621a51..8eaa936 100644 --- a/runCli.ts +++ b/runCli.ts @@ -1,5 +1,6 @@ import { execFileSync } from "node:child_process"; -import { accessSync, constants, existsSync } from "node:fs"; +import { accessSync, constants, existsSync, mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; import { delimiter, dirname, isAbsolute, join, resolve, sep } from "node:path"; import { fileURLToPath } from "node:url"; import actionPackageJson from "./package.json" with { type: "json" }; @@ -125,9 +126,22 @@ function createRuntimeContext(): RuntimeContext { }; } +// $GITHUB_WORKSPACE is the customer's repo. running `npx --yes pullfrog@…` +// there makes npm read THEIR `package.json` first, which on npm v11+ enforces +// `devEngines.packageManager` and aborts the bootstrap with EBADDEVENGINES +// before the agent ever boots. our bootstrap doesn't need anything from the +// customer's tree — a freshly-created tmpdir is package.json-free and +// parent-less, so npm walks up to `/` finding nothing. see #837. +// +// `mkdtempSync` (vs raw `tmpdir()`): `$TMPDIR` is overridable from a prior +// `$GITHUB_ENV` step, and a customer-authored or compromised prior step +// could plant `node_modules/pullfrog/` in the resolved tmpdir to hijack +// `npx --yes pullfrog@` resolution. a fresh per-invocation +// subdirectory is mode 0700 and not pre-writable by anything earlier in +// the job. function runCommand(params: { context: RuntimeContext; command: string; args: string[] }): void { execFileSync(params.command, params.args, { - cwd: process.env.GITHUB_WORKSPACE || params.context.actionRoot, + cwd: mkdtempSync(join(tmpdir(), "pullfrog-bootstrap-")), stdio: "inherit", env: params.context.env, }); diff --git a/utils/providerErrors.test.ts b/utils/providerErrors.test.ts index 9133522..cce0704 100644 --- a/utils/providerErrors.test.ts +++ b/utils/providerErrors.test.ts @@ -1,6 +1,8 @@ import { detectProviderError, + extractProviderId, findProviderErrorMatch, + isProviderBillingExhausted, isRouterKeylimitExhaustedError, } from "./providerErrors.ts"; @@ -103,6 +105,15 @@ describe("detectProviderError", () => { it("classifies bare 'Insufficient balance' as billing exhausted", () => { expect(detectProviderError("error: Insufficient balance")).toBe("provider billing exhausted"); }); + + it("classifies Anthropic 'credit balance is too low' as billing exhausted (#835)", () => { + // Anthropic-direct BYOK returns this string verbatim when the user's + // Anthropic console credit balance can't cover the request. distinct + // wording from "Insufficient balance" used by DeepSeek / OpenCode Zen. + const stderr = + "APIError: 400 Your credit balance is too low to access the Anthropic API. Please go to Plans & Billing to upgrade or purchase credits."; + expect(detectProviderError(stderr)).toBe("provider billing exhausted"); + }); }); describe("real provider errors", () => { @@ -213,6 +224,47 @@ describe("findProviderErrorMatch", () => { }); }); +describe("isProviderBillingExhausted (#835)", () => { + it("matches DeepSeek 'Insufficient Balance' payloads", () => { + expect(isProviderBillingExhausted("AI_APICallError: Insufficient Balance")).toBe(true); + }); + + it("matches Anthropic 'credit balance is too low' payloads", () => { + expect( + isProviderBillingExhausted("Your credit balance is too low to access the Anthropic API") + ).toBe(true); + }); + + it("matches OpenCode Zen CreditsError / FreeUsageLimitError", () => { + expect(isProviderBillingExhausted("CreditsError: out of credit")).toBe(true); + expect(isProviderBillingExhausted("FreeUsageLimitError: limit hit")).toBe(true); + }); + + it("returns false for unrelated provider errors", () => { + expect(isProviderBillingExhausted('{"statusCode": 401}')).toBe(false); + expect(isProviderBillingExhausted("rate_limit_exceeded")).toBe(false); + expect(isProviderBillingExhausted("just some log noise")).toBe(false); + }); +}); + +describe("extractProviderId", () => { + it("parses providerID= from OpenCode harness logs", () => { + expect( + extractProviderId( + 'ERROR providerID=deepseek modelID=deepseek-v4-pro error={"name":"AI_APICallError"}' + ) + ).toBe("deepseek"); + }); + + it("lowercases the captured slug", () => { + expect(extractProviderId("providerID=Anthropic modelID=claude")).toBe("anthropic"); + }); + + it("returns null when providerID is absent", () => { + expect(extractProviderId("APIError: Insufficient Balance")).toBeNull(); + }); +}); + describe("isRouterKeylimitExhaustedError", () => { it("matches the canonical OpenRouter mid-run error", () => { expect( diff --git a/utils/providerErrors.ts b/utils/providerErrors.ts index 2c8104f..d7688e8 100644 --- a/utils/providerErrors.ts +++ b/utils/providerErrors.ts @@ -1,5 +1,8 @@ type ProviderErrorPattern = { regex: RegExp; label: string }; +/** Stable label for the BYOK provider-billing-exhausted classification. */ +export const PROVIDER_BILLING_EXHAUSTED_LABEL = "provider billing exhausted"; + // 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. @@ -9,14 +12,16 @@ const PROVIDER_ERROR_PATTERNS: ProviderErrorPattern[] = [ // billing-payload patterns come BEFORE bare status-code patterns. providers // commonly return 401 / 429 for billing/quota exhaustion (OpenCode Zen // `CreditsError` / `FreeUsageLimitError`, Gemini `RESOURCE_EXHAUSTED` + - // "spending cap", Anthropic "Insufficient balance"). these are non-retryable - // and require user-billing action — distinct from a transient auth error or - // rate-limit. status-code patterns would otherwise win and surface - // "auth error (401)" / "rate limited (429)" with no billing hint. see #778. - { regex: /\bCreditsError\b/, label: "provider billing exhausted" }, - { regex: /\bFreeUsageLimitError\b/, label: "provider billing exhausted" }, - { regex: /Insufficient balance/i, label: "provider billing exhausted" }, - { regex: /spending cap/i, label: "provider billing exhausted" }, + // "spending cap", Anthropic "Insufficient balance" / "credit balance is + // too low"). these are non-retryable and require user-billing action — + // distinct from a transient auth error or rate-limit. status-code patterns + // would otherwise win and surface "auth error (401)" / "rate limited (429)" + // with no billing hint. see #778, #835. + { regex: /\bCreditsError\b/, label: PROVIDER_BILLING_EXHAUSTED_LABEL }, + { regex: /\bFreeUsageLimitError\b/, label: PROVIDER_BILLING_EXHAUSTED_LABEL }, + { regex: /Insufficient balance/i, label: PROVIDER_BILLING_EXHAUSTED_LABEL }, + { regex: /credit balance is too low/i, label: PROVIDER_BILLING_EXHAUSTED_LABEL }, + { regex: /spending cap/i, label: PROVIDER_BILLING_EXHAUSTED_LABEL }, // auth patterns must come BEFORE rate-limit patterns. OpenRouter 401 error // payloads carry `x-ratelimit-*` response headers in the dump, and the // free-form rate-limit regex below would otherwise win on word-boundary @@ -142,3 +147,26 @@ const ROUTER_KEYLIMIT_EXHAUSTED_PATTERN = export function isRouterKeylimitExhaustedError(text: string): boolean { return ROUTER_KEYLIMIT_EXHAUSTED_PATTERN.test(text); } + +/** + * BYOK billing-exhausted: provider rejected the request because the user's + * provider wallet is empty (DeepSeek "Insufficient Balance", Anthropic + * "credit balance is too low", OpenCode Zen `CreditsError` / + * `FreeUsageLimitError`, Gemini "spending cap"). Distinct from + * `isRouterKeylimitExhaustedError` — that's Pullfrog's Router wallet, this + * is the user's own provider account. + */ +export function isProviderBillingExhausted(text: string): boolean { + return findProviderErrorMatch(text)?.label === PROVIDER_BILLING_EXHAUSTED_LABEL; +} + +/** + * Extract `providerID=foo` from agent error logs (OpenCode emits this on + * `provider error detected (...)` lines). Returns the lowercase provider + * slug, or null when absent. Used to render a provider-specific dashboard + * link in the BYOK billing-exhausted summary. + */ +export function extractProviderId(text: string): string | null { + const match = text.match(/\bproviderID=([a-z0-9_-]+)/i); + return match ? match[1].toLowerCase() : null; +} diff --git a/utils/run.ts b/utils/run.ts index 132a5ef..b3d1a23 100644 --- a/utils/run.ts +++ b/utils/run.ts @@ -12,6 +12,11 @@ export interface HandleAgentResultParams { export async function handleAgentResult(ctx: HandleAgentResultParams): Promise { if (!ctx.result.success) { + // rendering + posting for the `!success` branch lives in + // `finalizeSuccessRun` (called immediately before this function) so the + // BYOK billing-exhausted, hang, and api-key bodies land on a single + // surface — both for runs with a pre-existing progress comment AND for + // silent triggers via `createIfMissing`. see #835. return { success: false, error: ctx.result.error || "Agent execution failed", diff --git a/utils/runErrorRenderer.test.ts b/utils/runErrorRenderer.test.ts index 415973f..657222f 100644 --- a/utils/runErrorRenderer.test.ts +++ b/utils/runErrorRenderer.test.ts @@ -3,6 +3,58 @@ import { renderRunError } from "./runErrorRenderer.ts"; const repo = { owner: "acme", name: "widget" }; +describe("renderRunError BYOK provider billing exhausted (#835)", () => { + const deepseekRaw = + '» provider error detected (provider billing exhausted): ERROR providerID=deepseek modelID=deepseek-v4-pro error={"name":"AI_APICallError","message":"Insufficient Balance"}'; + + const anthropicRaw = + "APIError: Your credit balance is too low to access the Anthropic API. Please go to Plans & Billing to upgrade or purchase credits."; + + const opencodeZenRaw = "CreditsError: account out of free usage"; + + it("renders DeepSeek billing-exhausted with provider-specific dashboard link", () => { + const result = renderRunError({ + errorMessage: deepseekRaw, + repo, + agentDiagnostic: undefined, + }); + expect(result.summary).toContain("`deepseek` account is out of credit"); + expect(result.summary).toContain("https://platform.deepseek.com/top_up"); + expect(result.summary).toContain("### ❌ Pullfrog failed"); + expect(result.comment).toContain("`deepseek` account is out of credit"); + expect(result.comment).not.toContain("### ❌ Pullfrog failed"); + }); + + it("matches Anthropic 'credit balance is too low' (#835 Anthropic case)", () => { + const result = renderRunError({ + errorMessage: anthropicRaw, + repo, + agentDiagnostic: undefined, + }); + expect(result.comment).toContain("out of credit"); + }); + + it("matches OpenCode Zen CreditsError shape", () => { + const result = renderRunError({ + errorMessage: opencodeZenRaw, + repo, + agentDiagnostic: undefined, + }); + expect(result.comment).toContain("out of credit"); + }); + + it("falls through to a generic CTA when providerID cannot be parsed", () => { + const result = renderRunError({ + errorMessage: "Insufficient balance — provider response with no providerID tag", + repo, + agentDiagnostic: undefined, + }); + expect(result.comment).toContain("Your provider account is out of credit"); + expect(result.comment).not.toContain("Your your"); + expect(result.comment).toContain("Top up your provider account"); + }); +}); + describe("renderRunError ProviderModelNotFoundError (#816)", () => { const staleFreeRaw = 'ProviderModelNotFoundError: {"providerID":"opencode","modelID":"retired-free-model","suggestions":["deepseek-v4-flash-free"]}'; diff --git a/utils/runErrorRenderer.ts b/utils/runErrorRenderer.ts index c8909bc..b52de1b 100644 --- a/utils/runErrorRenderer.ts +++ b/utils/runErrorRenderer.ts @@ -3,22 +3,37 @@ * pair of user-facing markdown bodies — one for the GitHub Actions job * summary tab, one for the PR progress comment. * - * Four classifications, in priority order: + * Classifications, in dispatch order (first match wins; the api-key + * branch additionally folds in the activity-timeout hang body as a + * sub-source so a hang masking an api-key error still surfaces the api-key + * CTA): * * 1. `BillingError` — either the proxy-token mint already threw one (402 * handled inline) or the agent runtime surfaced an OpenRouter * "key budget exhausted" string mid-run. Both render via * `formatBillingErrorSummary` so the user sees actionable copy. * - * 2. Activity-timeout hang — `errorMessage` starts with - * `"activity timeout"` or `"agent still pending"`. The harness keeps - * structured diagnostic state on `toolState.agentDiagnostic`; - * `formatAgentHangBody` renders that as a markdown block. + * 2. BYOK provider billing-exhausted (#835) — DeepSeek "Insufficient + * Balance", Anthropic "credit balance is too low", OpenCode Zen + * `CreditsError`, Gemini "spending cap". Checked before api-key auth + * because billing-exhausted responses often carry 401 status codes + * that `isApiKeyAuthError` would otherwise mis-classify. * - * 3. API-key auth error — `isApiKeyAuthError` sniffs the raw error string; - * `formatApiKeyErrorSummary` renders provider + console-link copy. + * 3. API-key auth error — `isApiKeyAuthError` sniffs the raw error string + * (or the activity-timeout hang body when present, since that's where + * the underlying provider error often lands); `formatApiKeyErrorSummary` + * renders provider + console-link copy. * - * 4. Default — a generic `❌ Pullfrog failed` block with the raw error + * 4. ProviderModelNotFoundError — stale free-fallback model id no longer + * in the OpenCode catalog; renders a nudge to add a BYOK key. + * + * 5. Activity-timeout hang — `errorMessage` starts with + * `"activity timeout"` or `"agent still pending"` AND none of the + * above matched. The harness keeps structured diagnostic state on + * `toolState.agentDiagnostic`; `formatAgentHangBody` renders that as + * a markdown block. + * + * 6. Default — a generic `❌ Pullfrog failed` block with the raw error * message in a fenced code block. Same body for both surfaces. * * The hang body and the API-key body diverge between the two surfaces only @@ -31,7 +46,11 @@ import type { AgentDiagnostic } from "./agentHangReport.ts"; import { formatAgentHangBody } from "./agentHangReport.ts"; import { formatApiKeyErrorSummary, isApiKeyAuthError } from "./apiKeys.ts"; import { BillingError, formatBillingErrorSummary } from "./billingErrors.ts"; -import { isRouterKeylimitExhaustedError } from "./providerErrors.ts"; +import { + extractProviderId, + isProviderBillingExhausted, + isRouterKeylimitExhaustedError, +} from "./providerErrors.ts"; export type RenderedRunError = { summary: string; @@ -42,6 +61,60 @@ function isProviderModelNotFoundError(message: string): boolean { return message.includes("ProviderModelNotFoundError"); } +/** + * Best-known billing top-up URL per provider. Conservative list: only + * providers we've actually classified billing-exhaustion shapes for in + * `providerErrors.ts`. Unknown providers fall through to a generic CTA. + */ +const PROVIDER_BILLING_URLS: Record = { + deepseek: "https://platform.deepseek.com/top_up", + anthropic: "https://console.anthropic.com/settings/billing", + openai: "https://platform.openai.com/account/billing", + google: "https://aistudio.google.com/usage", + opencode: "https://opencode.ai/zen", +}; + +/** + * `extractProviderId` only fires when the harness emits `providerID=...` + * (OpenCode log shape). Direct-provider errors (e.g. Anthropic SDK throwing + * `"Your credit balance is too low to access the Anthropic API"`) carry no + * such tag, so map their distinctive copy to a provider id here so the + * dashboard link is reachable. + * + * Pattern is intentionally tight (Anthropic-specific phrasing only) to + * avoid mis-tagging non-Anthropic billing-exhausted errors that happen to + * mention `"Anthropic API"` in passing — the broader phrase appears in + * fallback-chain agent prompt text and OpenCode harness logs. + */ +function detectProviderId(message: string): string | null { + const harnessId = extractProviderId(message); + if (harnessId) return harnessId; + if (/credit balance is too low/i.test(message)) return "anthropic"; + return null; +} + +function formatProviderBillingExhausted(input: { errorMessage: string }): string { + const providerId = detectProviderId(input.errorMessage); + const dashboardUrl = providerId ? PROVIDER_BILLING_URLS[providerId] : undefined; + + const headline = providerId + ? `**Your \`${providerId}\` account is out of credit.**` + : "**Your provider account is out of credit.**"; + const cta = dashboardUrl + ? `[Top up \`${providerId}\` →](${dashboardUrl})` + : "Top up your provider account, then re-trigger Pullfrog."; + + return [ + headline, + "", + "Pullfrog detected a billing-exhausted response from your provider — the agent stopped before completing this run.", + "", + cta, + "", + `\`\`\`\n${input.errorMessage}\n\`\`\``, + ].join("\n"); +} + function formatProviderModelNotFoundSummary(input: { owner: string; name: string; @@ -87,6 +160,21 @@ export function renderRunError(input: { }) : null; + // BYOK provider billing-exhausted (DeepSeek "Insufficient Balance", + // Anthropic "credit balance is too low", OpenCode Zen `CreditsError` / + // `FreeUsageLimitError`, Gemini "spending cap"). distinct from the Router + // billing branches above — Router uses `BillingError`, this uses the agent + // log payload classified by `isProviderBillingExhausted`. see #835. + // + // checked BEFORE api-key auth: providers commonly return 401 (DeepSeek, + // Gemini) or include `"API Error: 401"` in the error body for billing + // exhaustion, which `isApiKeyAuthError` would otherwise match — surfacing + // a "rotate your key" CTA when the actual fix is "top up credits". + if (isProviderBillingExhausted(input.errorMessage)) { + const body = formatProviderBillingExhausted({ errorMessage: input.errorMessage }); + return { summary: `### ❌ Pullfrog failed\n\n${body}`, comment: body }; + } + const apiKeySource = hangBody ?? input.errorMessage; const apiKeyErrorSummary = isApiKeyAuthError(apiKeySource) ? formatApiKeyErrorSummary({ diff --git a/utils/runLifecycle.ts b/utils/runLifecycle.ts index 4525209..2b560a2 100644 --- a/utils/runLifecycle.ts +++ b/utils/runLifecycle.ts @@ -81,12 +81,13 @@ export async function finalizeSuccessRun(input: { await persistRunArtifacts(input.toolContext); // shared rendering for the !success branch — same classifier as the - // outer catch path (BillingError reclassify → hang → api-key → generic), - // so a harness-returned `{success: false}` lands an actionable error - // block in the job summary alongside the matching body in the progress - // comment. hang and generic get the `### ❌ Pullfrog failed` H3 banner; - // BillingError and api-key render their own provider-specific framing - // (no banner). renders once; reused for both surfaces below. + // outer catch path (BillingError reclassify → hang → BYOK billing → + // api-key → generic), so a harness-returned `{success: false}` lands an + // actionable error block in the job summary alongside the matching body + // in the progress comment. hang and generic get the `### ❌ Pullfrog + // failed` H3 banner; BillingError, BYOK billing, and api-key render + // their own provider-specific framing (no banner). renders once; reused + // for both surfaces below. const rendered = !input.result.success ? renderRunError({ errorMessage: input.result.error || "agent run failed", @@ -95,12 +96,20 @@ export async function finalizeSuccessRun(input: { }) : null; - if (rendered && input.toolState.progressComment) { - await reportErrorToComment({ toolState: input.toolState, error: rendered.comment }).catch( - (error) => { - log.debug(`failure error report failed: ${error}`); - } - ); + // `createIfMissing: true` is load-bearing for silent triggers + // (IncrementalReview / pull_request_synchronize / auto-label) that have + // no progress comment to update — without it, terminal failures like + // BYOK billing exhaustion land only in the GH job summary, which most + // users never open. `reportErrorToComment` no-ops when both progress + // comment AND issue context are absent. see #835. + if (rendered) { + await reportErrorToComment({ + toolState: input.toolState, + error: rendered.comment, + createIfMissing: true, + }).catch((error) => { + log.debug(`failure error report failed: ${error}`); + }); } // create_pull_request_review owns its own deletion (see mcp/review.ts), so @@ -141,6 +150,13 @@ export async function finalizeSuccessRun(input: { * * `lastProgressBody` and the usage table are appended to the summary so the * partial work the agent did before failing isn't lost. + * + * `createIfMissing: true` is symmetric with `finalizeSuccessRun` — silent + * triggers (IncrementalReview / pull_request_synchronize / auto-label) that + * throw past `finalizeSuccessRun` (e.g. timeout race kills the agent + * mid-billing-exhausted-retry) reach this catch path with no progress + * comment to update, and without `createIfMissing` the terminal error + * lands only in the GH job summary that most users never open. see #835. */ export async function writeRunErrorOutputs(input: { rendered: RenderedRunError; @@ -155,7 +171,11 @@ export async function writeRunErrorOutputs(input: { } catch {} try { - await reportErrorToComment({ toolState: input.toolState, error: input.rendered.comment }); + await reportErrorToComment({ + toolState: input.toolState, + error: input.rendered.comment, + createIfMissing: true, + }); } catch { // error reporting failed, but don't let it mask the original error }