diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ac7fb41..90da6c5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -81,6 +81,7 @@ jobs: matrix: test: [ + byok-no-keys-fallback, git-permissions, githooks, pkg-json-scripts, diff --git a/main.ts b/main.ts index dbc66a1..d993ba6 100644 --- a/main.ts +++ b/main.ts @@ -17,6 +17,7 @@ import { import { resolveAgent, resolveModel } from "./utils/agent.ts"; import { validateAgentApiKey } from "./utils/apiKeys.ts"; import { resolveBody } from "./utils/body.ts"; +import { selectFallbackModelIfNeeded } from "./utils/byokFallback.ts"; import { log } from "./utils/cli.ts"; import { recordDiffReadFromToolUse } from "./utils/diffCoverage.ts"; import { onExitSignal } from "./utils/exitHandler.ts"; @@ -203,18 +204,46 @@ export async function main(): Promise { await using gitAuthServer = await startGitAuthServer(tmpdir); setGitAuthServer(gitAuthServer); - const resolvedModel = payload.proxyModel ? undefined : resolveModel({ slug: payload.model }); + const initialResolvedModel = payload.proxyModel + ? undefined + : resolveModel({ slug: payload.model }); + + // BYOK fallback: if the configured model needs a key the runner doesn't + // have, swap to a free OpenCode model so the run can still produce + // value. Without this, the agent launches with no key, the LLM provider + // 401s, and the run dies in seconds with a synthetic "Invalid API key" + // — exactly the silent-churn pattern that took out 15 accounts before + // this landed. Router/proxy runs are skipped (Pullfrog mints the key); + // see `selectFallbackModelIfNeeded` for the full skip set. + const fallback = selectFallbackModelIfNeeded({ + resolvedModel: initialResolvedModel, + proxyModel: payload.proxyModel, + }); + // when fallback engages we bypass `resolveModel` for the new slug — + // `PULLFROG_MODEL` has higher priority than the slug arg inside that + // helper and would otherwise re-override back to the unkeyed model. + // the free fallback slug is already a CLI-ready specifier, so using + // it verbatim is correct and avoids the override. + const effectiveSlug = fallback.fallback ? fallback.to : payload.model; + const resolvedModel = fallback.fallback ? fallback.to : initialResolvedModel; + if (fallback.fallback) { + log.warning( + `» fell back from ${fallback.from} to ${fallback.to} — no BYOK key present in runner env. add a provider key in repo secrets to use ${fallback.from} instead.` + ); + toolState.modelFallback = { from: fallback.from }; + } + const agent = resolveAgent({ model: resolvedModel }); // surface the effective model in comment/review footers. payload.model is // just the stored slug (often undefined for router/oss runs that derive // the target from proxyModel). matching priority with resolveModelForLog // so the "Using `…`" badge reflects what actually ran. - toolState.model = payload.proxyModel ?? resolvedModel ?? payload.model; + toolState.model = payload.proxyModel ?? resolvedModel ?? effectiveSlug; validateAgentApiKey({ agent, - model: payload.proxyModel ?? resolvedModel ?? payload.model, + model: payload.proxyModel ?? resolvedModel ?? effectiveSlug, owner: runContext.repo.owner, name: runContext.repo.name, }); diff --git a/mcp/comment.ts b/mcp/comment.ts index ad717d8..2ba0706 100644 --- a/mcp/comment.ts +++ b/mcp/comment.ts @@ -33,6 +33,7 @@ function buildCommentFooter(ctx: ToolContext, customParts?: string[]): string { : undefined, customParts, model: ctx.toolState.model, + fallbackFrom: ctx.toolState.modelFallback?.from, }); } diff --git a/mcp/pr.ts b/mcp/pr.ts index 1e96989..caba21b 100644 --- a/mcp/pr.ts +++ b/mcp/pr.ts @@ -23,6 +23,7 @@ function buildPrBodyWithFooter(ctx: ToolContext, body: string): string { ? { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId } : undefined, model: ctx.toolState.model, + fallbackFrom: ctx.toolState.modelFallback?.from, }); const bodyWithoutFooter = stripExistingFooter(fixDoubleEscapedString(body)); diff --git a/mcp/review.ts b/mcp/review.ts index e8931e8..050f505 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -871,6 +871,7 @@ async function createAndSubmitWithFooter( : undefined, customParts, model: ctx.toolState.model, + fallbackFrom: ctx.toolState.modelFallback?.from, }); return await ctx.octokit.rest.pulls.submitReview({ diff --git a/test/agnostic/byokFallback.ts b/test/agnostic/byokFallback.ts new file mode 100644 index 0000000..477a10f --- /dev/null +++ b/test/agnostic/byokFallback.ts @@ -0,0 +1,75 @@ +import type { AgentResult, TestRunnerOptions, ValidationCheck } from "../utils.ts"; +import { defineFixture, getAgentOutput } from "../utils.ts"; + +/** + * BYOK-no-keys fallback test — proves that an account configured for a + * BYOK model (here: `anthropic/claude-opus`) but with no provider API + * keys present in the runner env still gets a successful run by falling + * back to a free OpenCode model. + * + * This was the structural failure that took out 15 accounts post-launch + * before the fallback shipped: GH Actions secret references resolved to + * empty strings (because the secrets didn't exist), the action launched + * Claude Code with no key, the LLM provider 401'd, and the run died in + * 20s with a synthesized "Invalid API key" message. + * + * The env block below empty-strings every known provider key — that's + * exactly what GitHub Actions does when a `${{ secrets.X }}` reference + * resolves to a missing secret. We verify: + * 1. the run succeeded + * 2. the fallback log line was emitted (proves the swap happened) + */ +const fixture = defineFixture( + { + prompt: "Reply with exactly the single character: 4", + timeout: "5m", + }, + { localOnly: true } +); + +function validator(result: AgentResult): ValidationCheck[] { + const output = getAgentOutput(result); + const fellBack = /fell back from .* to opencode\/minimax-m2\.5-free/.test(output); + return [ + { name: "run_succeeded", passed: result.success }, + { name: "fallback_logged", passed: fellBack }, + ]; +} + +export const test: TestRunnerOptions = { + name: "byok-no-keys-fallback", + fixture, + validator, + env: { + // simulate every BYOK provider's secret being absent — same shape as + // a fresh-install account whose user never configured any keys. + ANTHROPIC_API_KEY: "", + CLAUDE_CODE_OAUTH_TOKEN: "", + OPENAI_API_KEY: "", + OPENROUTER_API_KEY: "", + GEMINI_API_KEY: "", + GOOGLE_GENERATIVE_AI_API_KEY: "", + XAI_API_KEY: "", + DEEPSEEK_API_KEY: "", + MOONSHOT_API_KEY: "", + OPENCODE_API_KEY: "", + AWS_BEARER_TOKEN_BEDROCK: "", + AWS_ACCESS_KEY_ID: "", + AWS_SECRET_ACCESS_KEY: "", + BEDROCK_MODEL_ID: "", + // configure a model that requires a BYOK key — the fallback only + // engages when there's a configured model whose provider key is + // absent, so we have to pin one. anthropic/claude-opus is the + // most common first-run choice (it's the catalog "preferred" for + // the anthropic provider). + PULLFROG_MODEL: "anthropic/claude-opus", + }, + tags: ["agnostic"], + coverage: [ + "action/utils/byokFallback.ts", + "action/utils/apiKeys.ts", + "action/utils/agent.ts", + "action/main.ts", + "action/models.ts", + ], +}; diff --git a/toolState.ts b/toolState.ts index 134885a..f73224a 100644 --- a/toolState.ts +++ b/toolState.ts @@ -156,6 +156,12 @@ export interface ToolState { output?: string; usageEntries: AgentUsage[]; model?: string | undefined; + // set by main.ts when the BYOK fallback engaged (configured model needed + // a provider key the runner didn't have). carried into PR-comment footers + // so users can see "Using (credentials for not + // configured)" rather than just being silently downgraded. literal record + // of an event that happened — matches the ToolState design rule. + modelFallback?: { from: string } | undefined; todoTracker?: TodoTracker | undefined; diffCoverage?: DiffCoverageState | undefined; // mutable handle the agent harness writes to as a run progresses (recent diff --git a/utils/buildPullfrogFooter.test.ts b/utils/buildPullfrogFooter.test.ts new file mode 100644 index 0000000..6a860a7 --- /dev/null +++ b/utils/buildPullfrogFooter.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; +import { buildPullfrogFooter } from "./buildPullfrogFooter.ts"; + +describe("buildPullfrogFooter — fallbackFrom annotation", () => { + it("renders the provider display name when fallbackFrom is set", () => { + const footer = buildPullfrogFooter({ + model: "opencode/minimax-m2.5-free", + fallbackFrom: "anthropic/claude-opus", + }); + expect(footer).toContain( + "Using `MiniMax M2.5` (free) (credentials for Anthropic not configured)" + ); + }); + + it("works for OpenAI's display name too", () => { + const footer = buildPullfrogFooter({ + model: "opencode/minimax-m2.5-free", + fallbackFrom: "openai/gpt", + }); + expect(footer).toContain("(credentials for OpenAI not configured)"); + }); + + it("falls back to the raw provider key when the slug provider is unknown to the catalog", () => { + const footer = buildPullfrogFooter({ + model: "opencode/minimax-m2.5-free", + fallbackFrom: "some-unknown/model", + }); + expect(footer).toContain("(credentials for some-unknown not configured)"); + }); + + it("omits the annotation when fallbackFrom is not set", () => { + const footer = buildPullfrogFooter({ + model: "anthropic/claude-opus", + }); + expect(footer).toContain("Using `Claude Opus`"); + expect(footer).not.toContain("not configured"); + }); +}); diff --git a/utils/buildPullfrogFooter.ts b/utils/buildPullfrogFooter.ts index 7ab7e53..7f3387a 100644 --- a/utils/buildPullfrogFooter.ts +++ b/utils/buildPullfrogFooter.ts @@ -1,4 +1,4 @@ -import { modelAliases, resolveDisplayAlias } from "../models.ts"; +import { getModelProvider, modelAliases, providers, resolveDisplayAlias } from "../models.ts"; export const PULLFROG_DIVIDER = ""; @@ -23,20 +23,41 @@ export interface BuildPullfrogFooterParams { customParts?: string[] | undefined; /** model slug from payload (e.g., "anthropic/claude-opus"). shown in footer as "Using `Model Name`" */ model?: string | undefined; + /** + * When the action engaged the BYOK fallback, this is the slug the user + * had configured (e.g. "anthropic/claude-opus") — the footer renders + * `Using (credentials for not configured)` + * so the substitution is visible in PR comments + reviews. + */ + fallbackFrom?: string | undefined; } -function formatModelLabel(slug: string): string { - // walk the fallback chain so a deprecated stored slug shows the model the - // run actually executed against (e.g. "GPT", not "GPT Codex"). +/** Provider display name (e.g. "Anthropic") for the slug, or the raw provider segment as a fallback. */ +function providerDisplayName(slug: string): string { + try { + const key = getModelProvider(slug); + const meta = providers[key as keyof typeof providers]; + return meta?.displayName ?? key; + } catch { + // raw IDs without a `/` (Bedrock model IDs) — never reach this function + // in practice because the BYOK fallback skips Bedrock, but defensively + // return the slug itself rather than throw if it ever does. + return slug; + } +} + +function formatModelLabel(params: { model: string; fallbackFrom?: string | undefined }): string { const alias = - resolveDisplayAlias(slug) ?? + resolveDisplayAlias(params.model) ?? // reverse-lookup: when the caller passes an effective model (proxy or // resolved target like "openrouter/anthropic/claude-opus-4.7") instead of // a stored alias slug, find the alias whose resolve target matches so we // still render a friendly display name. - modelAliases.find((a) => a.resolve === slug || a.openRouterResolve === slug); - if (!alias) return `\`${slug}\``; - return alias.isFree ? `\`${alias.displayName}\` (free)` : `\`${alias.displayName}\``; + modelAliases.find((a) => a.resolve === params.model || a.openRouterResolve === params.model); + const displayName = alias?.displayName ?? params.model; + const base = alias?.isFree ? `\`${displayName}\` (free)` : `\`${displayName}\``; + if (!params.fallbackFrom) return base; + return `${base} (credentials for ${providerDisplayName(params.fallbackFrom)} not configured)`; } /** @@ -64,7 +85,9 @@ export function buildPullfrogFooter(params: BuildPullfrogFooterParams): string { } if (params.model) { - parts.push(`Using ${formatModelLabel(params.model)}`); + parts.push( + `Using ${formatModelLabel({ model: params.model, fallbackFrom: params.fallbackFrom })}` + ); } const allParts = [...parts, "[𝕏](https://x.com/pullfrogai)"]; diff --git a/utils/byokFallback.test.ts b/utils/byokFallback.test.ts new file mode 100644 index 0000000..67dc18c --- /dev/null +++ b/utils/byokFallback.test.ts @@ -0,0 +1,102 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { FREE_FALLBACK_SLUG, selectFallbackModelIfNeeded } from "./byokFallback.ts"; + +describe("selectFallbackModelIfNeeded", () => { + const originalEnv = { ...process.env }; + const KEYS = [ + "ANTHROPIC_API_KEY", + "CLAUDE_CODE_OAUTH_TOKEN", + "OPENAI_API_KEY", + "OPENROUTER_API_KEY", + "GEMINI_API_KEY", + "GOOGLE_GENERATIVE_AI_API_KEY", + "XAI_API_KEY", + "DEEPSEEK_API_KEY", + "MOONSHOT_API_KEY", + "OPENCODE_API_KEY", + ] as const; + + beforeEach(() => { + for (const k of KEYS) delete process.env[k]; + }); + afterEach(() => { + for (const k of KEYS) { + if (originalEnv[k] === undefined) delete process.env[k]; + else process.env[k] = originalEnv[k]; + } + }); + + it("falls back when the resolved model needs a key that isn't set", () => { + const result = selectFallbackModelIfNeeded({ + resolvedModel: "anthropic/claude-opus-4-7", + proxyModel: undefined, + }); + expect(result).toEqual({ + fallback: true, + from: "anthropic/claude-opus-4-7", + to: FREE_FALLBACK_SLUG, + }); + }); + + it("does not fall back when the resolved model's key IS set", () => { + process.env.ANTHROPIC_API_KEY = "sk-test"; + const result = selectFallbackModelIfNeeded({ + resolvedModel: "anthropic/claude-opus-4-7", + proxyModel: undefined, + }); + expect(result.fallback).toBe(false); + }); + + it("does not fall back on Router runs (proxyModel set)", () => { + const result = selectFallbackModelIfNeeded({ + resolvedModel: undefined, + proxyModel: "openrouter/anthropic/claude-opus-4.7", + }); + expect(result.fallback).toBe(false); + }); + + it("does not fall back when no model is resolved (auto-select path)", () => { + const result = selectFallbackModelIfNeeded({ + resolvedModel: undefined, + proxyModel: undefined, + }); + expect(result.fallback).toBe(false); + }); + + it("does not fall back when the resolved model is itself the free fallback", () => { + const result = selectFallbackModelIfNeeded({ + resolvedModel: FREE_FALLBACK_SLUG, + proxyModel: undefined, + }); + expect(result.fallback).toBe(false); + }); + + it("does not fall back for Bedrock routing (raw model ID has no slash)", () => { + // resolveModel({slug:"bedrock/byok"}) returns the raw BEDROCK_MODEL_ID + // value (e.g. "us.anthropic.claude-opus-4-7"), which has no `/`. without + // a guard, hasProviderKey → parseModel would throw and crash the action + // before validateBedrockSetup can surface its tailored error. + const result = selectFallbackModelIfNeeded({ + resolvedModel: "us.anthropic.claude-opus-4-7", + proxyModel: undefined, + }); + expect(result.fallback).toBe(false); + }); + + it("does not fall back for free models that need no key", () => { + const result = selectFallbackModelIfNeeded({ + resolvedModel: "opencode/mimo-v2-pro-free", + proxyModel: undefined, + }); + expect(result.fallback).toBe(false); + }); + + it("treats empty-string env vars as missing (matches GH Actions secret-not-found behavior)", () => { + process.env.ANTHROPIC_API_KEY = ""; + const result = selectFallbackModelIfNeeded({ + resolvedModel: "anthropic/claude-opus-4-7", + proxyModel: undefined, + }); + expect(result.fallback).toBe(true); + }); +}); diff --git a/utils/byokFallback.ts b/utils/byokFallback.ts new file mode 100644 index 0000000..ef28fcc --- /dev/null +++ b/utils/byokFallback.ts @@ -0,0 +1,59 @@ +import { hasProviderKey } from "./apiKeys.ts"; + +/** + * Slug we fall back to when a BYOK-required model is configured but the + * runner has no provider key in env. Picked because it's free + * (`isFree: true`, `envVars: []` — see `action/models.ts`), stable, and + * currently the strongest free OpenCode model in the catalog. If a + * smarter free model is added later, update this single constant. + * + * The slug is intentionally hard-coded and not a config knob — the + * fallback is a safety net, not a user-facing preference, and adding a + * config surface here would just push the same "what to fall back to" + * decision into another setting that goes stale the same way. + */ +export const FREE_FALLBACK_SLUG = "opencode/minimax-m2.5-free"; + +export type FallbackDecision = { fallback: false } | { fallback: true; from: string; to: string }; + +/** + * If the resolved model requires a BYOK key but no provider key is + * available in env, return `fallback: true` with a free OpenCode slug + * so the run can still succeed. Caller is responsible for swapping the + * model state and surfacing the fallback (log line + run summary). + * + * Gates on `resolvedModel` directly (not the configured slug) so the + * decision matches both code paths that reach this point: payload-based + * config (`repo.model` from DB) and `PULLFROG_MODEL` env var. Both end + * up in `resolvedModel` after `resolveModel()` runs upstream. + * + * Skip cases: + * - Router / proxy runs (`proxyModel` set): Pullfrog mints the key, + * no BYOK in play — never fall back. + * - No resolved model: keeps the existing auto-select-with-throw + * behavior in `validateAgentApiKey` for the "neither model nor + * key" case (genuine misconfig the user should see). + * - Resolved model is itself the free fallback: avoid suggesting we + * fell back to the model we're already running. + * - Resolved model is a Bedrock raw ID (no `/`): Bedrock has its own + * auth shape (`AWS_BEARER_TOKEN_BEDROCK` + region + model ID), and + * `validateBedrockSetup` already surfaces a tailored error. Skipping + * here also avoids `parseModel`'s slash requirement crashing inside + * `hasProviderKey`. + * - Resolved model has its provider key present: no fallback needed. + */ +export function selectFallbackModelIfNeeded(input: { + resolvedModel: string | undefined; + proxyModel: string | undefined; +}): FallbackDecision { + if (input.proxyModel) return { fallback: false }; + if (!input.resolvedModel) return { fallback: false }; + if (input.resolvedModel === FREE_FALLBACK_SLUG) return { fallback: false }; + if (!input.resolvedModel.includes("/")) return { fallback: false }; + if (hasProviderKey(input.resolvedModel)) return { fallback: false }; + return { + fallback: true, + from: input.resolvedModel, + to: FREE_FALLBACK_SLUG, + }; +} diff --git a/utils/errorReport.ts b/utils/errorReport.ts index b86c198..4169108 100644 --- a/utils/errorReport.ts +++ b/utils/errorReport.ts @@ -38,6 +38,7 @@ export async function reportErrorToComment(ctx: ReportErrorParams): Promise