diff --git a/.env.example b/.env.example index 4bd8bf1..95caab5 100644 --- a/.env.example +++ b/.env.example @@ -46,6 +46,38 @@ MAX_FILES_PER_REVIEW=50 # Per-request timeout (ms) for every AI provider call. A hung/slow model call # is aborted at this deadline and the review fails visibly instead of stalling. AI_REQUEST_TIMEOUT_MS=60000 + +# ─── Backup AI provider / failover (optional; off unless BACKUP_AI_PROVIDER set) ─── +# When the primary provider fails transiently (timeout, 5xx, 429, network), the +# review falls over to this backup so a review is still posted. Peer-quality +# backup recommended. Does NOT fail over on 401/403 (surfaces misconfiguration). +# BACKUP_AI_PROVIDER=anthropic # "anthropic" | "openai" | "openai-compatible" +# Backup creds REUSE the matching primary vars unless a BACKUP_* override is set: +# BACKUP_ANTHROPIC_API_KEY=sk-ant-... # else reuses ANTHROPIC_API_KEY +# BACKUP_ANTHROPIC_MODEL=claude-opus-4-8 # else reuses ANTHROPIC_MODEL +# BACKUP_ANTHROPIC_BASE_URL= # else reuses ANTHROPIC_BASE_URL +# BACKUP_OPENAI_API_KEY= # else reuses OPENAI_API_KEY +# BACKUP_OPENAI_MODEL= # else reuses OPENAI_MODEL +# BACKUP_OPENAI_BASE_URL= # else reuses OPENAI_BASE_URL +# BACKUP_LOCAL_AI_BASE_URL= # else reuses LOCAL_AI_BASE_URL +# BACKUP_LOCAL_AI_MODEL= # else reuses LOCAL_AI_MODEL +# BACKUP_LOCAL_AI_API_KEY= # else reuses LOCAL_AI_API_KEY +# BACKUP_LOCAL_AI_JSON_MODE= # else reuses LOCAL_AI_JSON_MODE +# +# Short deadline given to the PRIMARY when a backup is configured, so a slow-hang +# fails over quickly (clamped to <= AI_REQUEST_TIMEOUT_MS). Default 20000. Set +# this above your primary's observed p95 `review` latency — too low and a +# healthy-but-slow review will routinely time out and fail over, roughly +# doubling latency and cost for that review. +# PRIMARY_AI_TIMEOUT_MS=20000 +# Best-effort circuit breaker: after roughly N consecutive failed primary AI +# *calls* it routes straight to the backup for a cooldown, sparing the +# primary's stall during a clear outage. It counts individual provider calls +# (a review makes several, some concurrent), so it's a cost/latency guard, not +# a precise per-review guarantee. +# BACKUP_CIRCUIT_THRESHOLD=3 +# BACKUP_CIRCUIT_COOLDOWN_MS=60000 + # Comma-separated glob patterns to skip IGNORED_PATTERNS= diff --git a/README.md b/README.md index bdc3953..5db1dc3 100644 --- a/README.md +++ b/README.md @@ -722,6 +722,37 @@ GitHub webhook minified assets (`*.min.js`, `*.min.css`), sourcemaps (`*.map`), build output (`dist/**`, `build/**`, `.next/**`). +### Backup provider / failover + +Off by default. Set `BACKUP_AI_PROVIDER` (`anthropic`, `openai`, or +`openai-compatible`) to enable a second provider that the review falls over to +when the primary fails *transiently* (timeout, 5xx, 429, network) — it does +**not** fail over on 401/403, since those indicate misconfiguration rather +than an outage. Backup credentials reuse the matching primary vars +(`ANTHROPIC_*`, `OPENAI_*`, `LOCAL_AI_*`) unless a `BACKUP_*` override is set. +When a backup is configured, the primary is given a short deadline +(`PRIMARY_AI_TIMEOUT_MS`, default 20000ms) so a slow-hang fails over quickly +instead of eating the full `AI_REQUEST_TIMEOUT_MS`. Set it above your +primary's observed p95 `review` latency — too low and a healthy-but-slow +review will routinely time out and fail over, roughly doubling latency and +cost for that review. A best-effort circuit breaker +(`BACKUP_CIRCUIT_THRESHOLD` / `BACKUP_CIRCUIT_COOLDOWN_MS`) routes straight to +the backup for a cooldown after roughly N consecutive failed primary AI +*calls*, sparing the primary's stall during a clear outage. It counts +individual provider calls (a review makes several, some concurrent, and a +succeeding one can reset the counter), so it's a cost/latency guard, not a +precise per-review guarantee. + +| Variable | Required | Default | Description | +|---|---|---|---| +| `BACKUP_AI_PROVIDER` | No | | `anthropic`, `openai`, or `openai-compatible`. Unset = failover disabled. | +| `PRIMARY_AI_TIMEOUT_MS` | No | `20000` | Deadline given to the primary before failing over (clamped to `AI_REQUEST_TIMEOUT_MS`). | +| `BACKUP_CIRCUIT_THRESHOLD` | No | `3` | Roughly this many consecutive failed primary AI *calls* before the circuit breaker skips it (best-effort, not a precise per-review count). | +| `BACKUP_CIRCUIT_COOLDOWN_MS` | No | `60000` | How long the circuit breaker stays open before retrying the primary. | + +See `.env.example` for the full list of `BACKUP_ANTHROPIC_*` / `BACKUP_OPENAI_*` / +`BACKUP_LOCAL_AI_*` override vars. + ## Using local models (Ollama, LM Studio, vLLM, llama.cpp, LocalAI) DiffSentry can talk to any server that exposes an **OpenAI-compatible diff --git a/docs/superpowers/plans/2026-07-09-backup-ai-provider-failover.md b/docs/superpowers/plans/2026-07-09-backup-ai-provider-failover.md new file mode 100644 index 0000000..fa62ab9 --- /dev/null +++ b/docs/superpowers/plans/2026-07-09-backup-ai-provider-failover.md @@ -0,0 +1,1045 @@ +# Backup AI Provider / Failover Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When the primary review model fails transiently or hangs, DiffSentry falls back to a configured secondary provider and still posts a review, instead of dead-lettering it. + +**Architecture:** A new `FailoverProvider implements AIProvider` wraps a primary and a backup provider; each of the five interface methods tries the primary (on a deliberately short deadline) and, on a transient error, falls over to the backup (on the normal deadline). An in-memory circuit breaker skips a persistently-down primary. The wrapper is built in the `Reviewer` constructor only when `BACKUP_AI_PROVIDER` is set — feature off by default. + +**Tech Stack:** TypeScript (NodeNext, `.js`-suffixed relative imports mapping to `.ts`), Vitest (`tests/unit/**/*.test.ts`), Express app. + +## Global Constraints + +- Feature is **off by default**: unset `BACKUP_AI_PROVIDER` ⇒ `Reviewer` uses the plain primary provider with the full `AI_REQUEST_TIMEOUT_MS` (byte-for-byte current behavior). +- Relative imports MUST carry a `.js` extension (e.g. `import { x } from "./transient.js"`), per the repo's NodeNext style. +- Test files live under `tests/unit/`; import source as `../../src/...js`. +- Failover fires **only** on transient errors (`AiTimeoutError`, transient network codes, HTTP `>= 500`, `429`). It MUST NOT fire on 4xx (401/403/400). The circuit breaker MUST NOT count non-failover errors. +- Failover is **sequential** (primary awaited to completion/rejection before backup). Never run both concurrently. +- Commands: build `npm run build`, test `npm test`, lint `npm run lint`, typecheck `npx tsc --noEmit`. +- End every commit message with: + `Co-Authored-By: Claude Opus 4.8 (1M context) ` + +--- + +### Task 1: Extract the transient-error predicate into a shared module + +Move `isTransientError` (and its private helpers) out of `src/realtime/jobs.ts` into a neutral `src/ai/transient.ts` so both the job runner and the failover wrapper share one definition. Behavior is unchanged. + +**Files:** +- Create: `src/ai/transient.ts` +- Create: `tests/unit/transient.test.ts` +- Modify: `src/realtime/jobs.ts` (remove the local copy; import + re-export from the new module) + +**Interfaces:** +- Produces: `export function isTransientError(err: unknown): boolean` + +- [ ] **Step 1: Write the failing test** + +Create `tests/unit/transient.test.ts`: + +```ts +import { describe, it, expect } from "vitest"; +import { isTransientError } from "../../src/ai/transient.js"; +import { AiTimeoutError } from "../../src/ai/timeout.js"; + +describe("isTransientError", () => { + it("treats an AiTimeoutError as transient", () => { + expect(isTransientError(new AiTimeoutError("openai-compatible", "review", 20000))).toBe(true); + }); + + it("treats transient network codes as transient", () => { + expect(isTransientError({ code: "ECONNRESET" })).toBe(true); + expect(isTransientError({ code: "ETIMEDOUT" })).toBe(true); + }); + + it("treats HTTP 5xx and 429 as transient", () => { + expect(isTransientError({ status: 503 })).toBe(true); + expect(isTransientError({ status: 500 })).toBe(true); + expect(isTransientError({ status: 429 })).toBe(true); + }); + + it("does NOT treat auth/4xx as transient", () => { + expect(isTransientError({ status: 401 })).toBe(false); + expect(isTransientError({ status: 403 })).toBe(false); + expect(isTransientError({ status: 400 })).toBe(false); + }); + + it("treats AbortError / TimeoutError names as transient", () => { + expect(isTransientError({ name: "AbortError" })).toBe(true); + expect(isTransientError({ name: "TimeoutError" })).toBe(true); + }); + + it("matches transient message hints", () => { + expect(isTransientError(new Error("socket hang up"))).toBe(true); + expect(isTransientError(new Error("service unavailable"))).toBe(true); + }); + + it("treats an ordinary error as non-transient", () => { + expect(isTransientError(new Error("bad request: invalid model"))).toBe(false); + }); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx vitest run tests/unit/transient.test.ts` +Expected: FAIL — cannot resolve `../../src/ai/transient.js`. + +- [ ] **Step 3: Create the shared module** + +Create `src/ai/transient.ts` by moving the block from `src/realtime/jobs.ts` verbatim (currently `TRANSIENT_CODES`, `TRANSIENT_MESSAGE_HINTS`, `statusOf`, and `isTransientError`, roughly `jobs.ts:68–122`): + +```ts +// ───────────────────────────────────────────────────────────────────────────── +// Shared classification of "transient" AI/network errors — worth a retry or a +// failover, as opposed to a deterministic 4xx that will just fail again. +// +// Used by the job runner (bounded retry / dead-letter) AND the FailoverProvider +// (primary → backup). Keeping one definition prevents the two from drifting. +// ───────────────────────────────────────────────────────────────────────────── + +/** Network-layer error codes that warrant a retry / failover. */ +const TRANSIENT_CODES = new Set([ + "ECONNRESET", + "ETIMEDOUT", + "ECONNREFUSED", + "ENOTFOUND", + "EAI_AGAIN", + "EPIPE", + "ENETUNREACH", + "UND_ERR_CONNECT_TIMEOUT", + "UND_ERR_HEADERS_TIMEOUT", + "UND_ERR_SOCKET", +]); + +const TRANSIENT_MESSAGE_HINTS = [ + "timed out", + "timeout", + "etimedout", + "econnreset", + "socket hang up", + "network", + "fetch failed", + "temporarily unavailable", + "service unavailable", + "rate limit", + "too many requests", +]; + +/** Read an HTTP-ish status off an error (Octokit RequestError, fetch wrappers). */ +function statusOf(err: unknown): number | undefined { + const s = (err as { status?: unknown; statusCode?: unknown })?.status ?? (err as { statusCode?: unknown })?.statusCode; + return typeof s === "number" ? s : undefined; +} + +/** + * Classify an error as transient (worth retrying / failing over) or permanent + * (fail fast). Transient = network blips, GitHub/AI 5xx + 429, request timeouts, + * and AbortError raised by an AI client's own timeout. NOTE: a cancel/abort from + * our own cancel path never reaches these callers, so a thrown AbortError here is + * an upstream timeout, not a cancellation. + */ +export function isTransientError(err: unknown): boolean { + const code = (err as { code?: unknown })?.code; + if (typeof code === "string" && TRANSIENT_CODES.has(code)) return true; + + const status = statusOf(err); + if (typeof status === "number" && (status >= 500 || status === 429)) return true; + + const name = (err as { name?: unknown })?.name; + if (name === "AbortError" || name === "TimeoutError") return true; + + const msg = (err instanceof Error ? err.message : String(err)).toLowerCase(); + return TRANSIENT_MESSAGE_HINTS.some((hint) => msg.includes(hint)); +} +``` + +- [ ] **Step 4: Update `jobs.ts` to import from the shared module** + +In `src/realtime/jobs.ts`, delete the moved block (`TRANSIENT_CODES`, `TRANSIENT_MESSAGE_HINTS`, `statusOf`, and the `isTransientError` function + its doc comment). Add at the top with the other imports: + +```ts +import { isTransientError } from "../ai/transient.js"; +``` + +Then re-export it so any current/future importer of `jobs.ts` keeps working: + +```ts +export { isTransientError } from "../ai/transient.js"; +``` + +Leave the rest of `jobs.ts` (retry loop, `errorMessage`, `sleep`, etc.) unchanged. + +- [ ] **Step 5: Run tests + typecheck to verify pass** + +Run: `npx vitest run tests/unit/transient.test.ts && npx tsc --noEmit` +Expected: PASS; no type errors. + +- [ ] **Step 6: Commit** + +```bash +git add src/ai/transient.ts src/realtime/jobs.ts tests/unit/transient.test.ts +git commit -m "Extract isTransientError into shared src/ai/transient.ts + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +### Task 2: `FailoverProvider` + circuit breaker + +The wrapper. Implements all five `AIProvider` methods; tries primary then backup on transient errors; opens a breaker after N consecutive failover-eligible primary failures. + +**Files:** +- Create: `src/ai/failover.ts` +- Create: `tests/unit/failover.test.ts` +- Modify: `src/types.ts` (add `ReviewResult.servedBy?`) + +**Interfaces:** +- Consumes: `isTransientError` (Task 1); `AIProvider`, `ReviewResult`, `WalkthroughResult`, `PRContext`, `IssueContext`, `RepoConfig`, `Learning` from `../types.js`. +- Produces: + ```ts + export interface FailoverOptions { circuitThreshold: number; circuitCooldownMs: number; now?: () => number; } + export class FailoverProvider implements AIProvider { constructor(primary: AIProvider, backup: AIProvider, opts: FailoverOptions); /* five methods */ } + ``` +- Produces (types): `ReviewResult.servedBy?: "primary" | "backup"`. + +- [ ] **Step 1: Add the `servedBy` field to `ReviewResult`** + +In `src/types.ts`, inside `export interface ReviewResult` (after `fanInByFile?`), add: + +```ts + /** Which provider produced this review. Set to "backup" by FailoverProvider + * when the primary failed over; absent/"primary" otherwise. Drives the + * subtle "reviewed by backup provider" footnote in the posted body. */ + servedBy?: "primary" | "backup"; +``` + +- [ ] **Step 2: Write the failing test** + +Create `tests/unit/failover.test.ts`: + +```ts +import { describe, it, expect, vi } from "vitest"; +import { FailoverProvider } from "../../src/ai/failover.js"; +import { AiTimeoutError } from "../../src/ai/timeout.js"; +import type { AIProvider, ReviewResult, PRContext, RepoConfig } from "../../src/types.js"; + +function ctx(): PRContext { + return { + owner: "o", repo: "r", pullNumber: 1, title: "t", description: "", + baseBranch: "main", headBranch: "feat", headSha: "sha", files: [], diff: "", + } as unknown as PRContext; +} + +function review(summary: string): ReviewResult { + return { summary, comments: [], approval: "COMMENT" }; +} + +/** Minimal fake provider; only the methods a test exercises are stubbed. */ +function fakeProvider(over: Partial): AIProvider { + const notImpl = () => { throw new Error("not stubbed"); }; + return { + review: over.review ?? (notImpl as AIProvider["review"]), + generateWalkthrough: over.generateWalkthrough ?? (notImpl as AIProvider["generateWalkthrough"]), + chat: over.chat ?? (notImpl as AIProvider["chat"]), + chatIssue: over.chatIssue ?? (notImpl as AIProvider["chatIssue"]), + complete: over.complete ?? (notImpl as AIProvider["complete"]), + }; +} + +const OPTS = { circuitThreshold: 3, circuitCooldownMs: 60_000 }; + +describe("FailoverProvider", () => { + it("returns the primary result and never calls the backup on success", async () => { + const backupReview = vi.fn(); + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockResolvedValue(review("primary")) }), + fakeProvider({ review: backupReview }), + OPTS, + ); + const res = await p.review(ctx()); + expect(res.summary).toBe("primary"); + expect(res.servedBy).toBeUndefined(); + expect(backupReview).not.toHaveBeenCalled(); + }); + + it("fails over to the backup on a transient primary error and tags servedBy", async () => { + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)) }), + fakeProvider({ review: vi.fn().mockResolvedValue(review("backup")) }), + OPTS, + ); + const res = await p.review(ctx()); + expect(res.summary).toBe("backup"); + expect(res.servedBy).toBe("backup"); + }); + + it("does NOT fail over on a 401 and rethrows", async () => { + const backupReview = vi.fn(); + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockRejectedValue(Object.assign(new Error("unauthorized"), { status: 401 })) }), + fakeProvider({ review: backupReview }), + OPTS, + ); + await expect(p.review(ctx())).rejects.toThrow("unauthorized"); + expect(backupReview).not.toHaveBeenCalled(); + }); + + it("rethrows the backup error when both fail", async () => { + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)) }), + fakeProvider({ review: vi.fn().mockRejectedValue(new Error("backup down")) }), + OPTS, + ); + await expect(p.review(ctx())).rejects.toThrow("backup down"); + }); + + it("opens the breaker after threshold consecutive transient failures, then routes straight to backup", async () => { + const primaryReview = vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)); + const backupReview = vi.fn().mockResolvedValue(review("backup")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: backupReview }), + OPTS, + ); + // 3 failing-then-failover calls trip the breaker. + await p.review(ctx()); + await p.review(ctx()); + await p.review(ctx()); + expect(primaryReview).toHaveBeenCalledTimes(3); + // 4th call: breaker open → primary skipped entirely. + await p.review(ctx()); + expect(primaryReview).toHaveBeenCalledTimes(3); + expect(backupReview).toHaveBeenCalledTimes(4); + }); + + it("half-opens after cooldown; a primary success closes the breaker", async () => { + let clock = 1_000; + const now = () => clock; + const primaryReview = vi + .fn() + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockResolvedValue(review("primary-recovered")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: vi.fn().mockResolvedValue(review("backup")) }), + { ...OPTS, now }, + ); + await p.review(ctx()); await p.review(ctx()); await p.review(ctx()); // breaker opens + clock += 60_001; // cooldown elapsed → half-open probe hits primary + const res = await p.review(ctx()); + expect(res.summary).toBe("primary-recovered"); + expect(res.servedBy).toBeUndefined(); // primary served + }); + + it("resets consecutive failures on a primary success", async () => { + const primaryReview = vi + .fn() + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockResolvedValueOnce(review("ok")) // resets counter + .mockRejectedValue(new AiTimeoutError("primary", "review", 20000)); + const backupReview = vi.fn().mockResolvedValue(review("backup")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: backupReview }), + OPTS, + ); + await p.review(ctx()); await p.review(ctx()); // 2 failures + await p.review(ctx()); // success → reset + await p.review(ctx()); // 1 failure (breaker still closed) + expect(primaryReview).toHaveBeenCalledTimes(4); // primary always attempted (never skipped) + }); + + it("delegates the non-review methods and fails them over too", async () => { + const p = new FailoverProvider( + fakeProvider({ + generateWalkthrough: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "walkthrough", 20000)), + chat: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "chat", 20000)), + chatIssue: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "issue_chat", 20000)), + complete: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "complete", 20000)), + }), + fakeProvider({ + generateWalkthrough: vi.fn().mockResolvedValue({ summary: "wt", fileDescriptions: [] }), + chat: vi.fn().mockResolvedValue("chat-backup"), + chatIssue: vi.fn().mockResolvedValue("issue-backup"), + complete: vi.fn().mockResolvedValue("complete-backup"), + }), + OPTS, + ); + expect((await p.generateWalkthrough(ctx())).summary).toBe("wt"); + expect(await p.chat(ctx(), "hi")).toBe("chat-backup"); + expect(await p.chatIssue({} as never, "hi")).toBe("issue-backup"); + expect(await p.complete("sys", "usr")).toBe("complete-backup"); + }); +}); +``` + +- [ ] **Step 3: Run test to verify it fails** + +Run: `npx vitest run tests/unit/failover.test.ts` +Expected: FAIL — cannot resolve `../../src/ai/failover.js`. + +- [ ] **Step 4: Implement `FailoverProvider`** + +Create `src/ai/failover.ts`: + +```ts +import { + AIProvider, + PRContext, + ReviewResult, + WalkthroughResult, + RepoConfig, + Learning, + IssueContext, +} from "../types.js"; +import { isTransientError } from "./transient.js"; +import { logger } from "../logger.js"; + +export interface FailoverOptions { + /** Consecutive failover-eligible primary failures before the breaker opens. */ + circuitThreshold: number; + /** How long (ms) the breaker stays open, routing straight to the backup. */ + circuitCooldownMs: number; + /** Injectable clock for deterministic tests. Defaults to Date.now. */ + now?: () => number; +} + +// ───────────────────────────────────────────────────────────────────────────── +// Sequential try-primary-then-backup wrapper around two AIProviders. +// +// - Fails over ONLY on transient errors (isTransientError): timeouts, 5xx, +// 429, network blips. A 4xx (auth / bad request) rethrows without touching +// the backup or the breaker — a bad primary key must surface, not silently +// route all traffic to the backup. +// - The primary is constructed with a SHORT deadline and the backup with the +// normal one (see reviewer.ts), so a slow-hang on the primary switches fast. +// - A circuit breaker skips a persistently-down primary for a cooldown so we +// don't pay the primary's stall on every single review during an outage. +// +// State is in-memory on the instance (the Reviewer is effectively a singleton), +// so it persists across reviews within a process and resets on restart — it is a +// cost/latency guard, not a correctness mechanism. +// ───────────────────────────────────────────────────────────────────────────── +export class FailoverProvider implements AIProvider { + private consecutiveFailures = 0; + private openedAt: number | null = null; + private readonly threshold: number; + private readonly cooldownMs: number; + private readonly now: () => number; + + constructor( + private readonly primary: AIProvider, + private readonly backup: AIProvider, + opts: FailoverOptions, + ) { + this.threshold = opts.circuitThreshold; + this.cooldownMs = opts.circuitCooldownMs; + this.now = opts.now ?? Date.now; + } + + /** True while the breaker is open and the cooldown has not yet elapsed. Once + * the cooldown passes we return false so the next call probes the primary + * (half-open). */ + private breakerOpen(): boolean { + if (this.openedAt === null) return false; + return this.now() - this.openedAt < this.cooldownMs; + } + + private recordPrimarySuccess(): void { + if (this.openedAt !== null || this.consecutiveFailures > 0) { + logger.info({ mode: "failover" }, "Primary AI provider recovered — closing circuit breaker"); + } + this.consecutiveFailures = 0; + this.openedAt = null; + } + + private recordPrimaryFailure(): void { + this.consecutiveFailures += 1; + if (this.consecutiveFailures >= this.threshold) { + const wasOpen = this.openedAt !== null; + this.openedAt = this.now(); + if (!wasOpen) { + logger.warn( + { mode: "failover", consecutiveFailures: this.consecutiveFailures }, + "Primary AI provider failing repeatedly — opening circuit breaker (routing to backup)", + ); + } + } + } + + /** + * Run one operation with failover. Returns the result plus which provider + * served it. Never runs primary and backup concurrently. + */ + private async run( + operation: string, + primaryCall: () => Promise, + backupCall: () => Promise, + ): Promise<{ result: T; servedBy: "primary" | "backup" }> { + if (this.breakerOpen()) { + logger.warn({ mode: "failover", operation }, "Circuit breaker open — routing directly to backup AI provider"); + return { result: await backupCall(), servedBy: "backup" }; + } + + const startedAt = this.now(); + try { + const result = await primaryCall(); + this.recordPrimarySuccess(); + return { result, servedBy: "primary" }; + } catch (err) { + if (!isTransientError(err)) { + // Deterministic failure (e.g. 401/403/400) — do NOT fail over and do + // NOT trip the breaker; surface it so misconfiguration is visible. + throw err; + } + this.recordPrimaryFailure(); + logger.warn( + { mode: "failover", operation, primaryLatencyMs: this.now() - startedAt, err }, + "Primary AI provider failed transiently — failing over to backup", + ); + return { result: await backupCall(), servedBy: "backup" }; + } + } + + async review(context: PRContext, repoConfig?: RepoConfig, learnings?: Learning[]): Promise { + const { result, servedBy } = await this.run( + "review", + () => this.primary.review(context, repoConfig, learnings), + () => this.backup.review(context, repoConfig, learnings), + ); + return servedBy === "backup" ? { ...result, servedBy } : result; + } + + async generateWalkthrough(context: PRContext, repoConfig?: RepoConfig): Promise { + const { result } = await this.run( + "walkthrough", + () => this.primary.generateWalkthrough(context, repoConfig), + () => this.backup.generateWalkthrough(context, repoConfig), + ); + return result; + } + + async chat(context: PRContext, userMessage: string, repoConfig?: RepoConfig): Promise { + const { result } = await this.run( + "chat", + () => this.primary.chat(context, userMessage, repoConfig), + () => this.backup.chat(context, userMessage, repoConfig), + ); + return result; + } + + async chatIssue(context: IssueContext, userMessage: string, repoConfig?: RepoConfig): Promise { + const { result } = await this.run( + "issue_chat", + () => this.primary.chatIssue(context, userMessage, repoConfig), + () => this.backup.chatIssue(context, userMessage, repoConfig), + ); + return result; + } + + async complete(system: string, user: string, opts?: { maxTokens?: number; json?: boolean }): Promise { + const { result } = await this.run( + "complete", + () => this.primary.complete(system, user, opts), + () => this.backup.complete(system, user, opts), + ); + return result; + } +} +``` + +- [ ] **Step 5: Run tests + typecheck to verify pass** + +Run: `npx vitest run tests/unit/failover.test.ts && npx tsc --noEmit` +Expected: PASS; no type errors. (If `PRContext`/`IssueContext` fields in the test's `ctx()` cast cause a type error, keep the `as unknown as PRContext` cast — the wrapper never inspects the context.) + +- [ ] **Step 6: Commit** + +```bash +git add src/ai/failover.ts tests/unit/failover.test.ts src/types.ts +git commit -m "Add FailoverProvider with circuit breaker (primary → backup AI) + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +### Task 3: Provider factory + backup config parsing/validation + +Extract provider construction into a reusable `buildProvider(spec)`, and teach `loadConfig` to parse + validate the backup provider (reuse-with-override) plus the short primary deadline and breaker knobs. + +**Files:** +- Create: `src/ai/provider-factory.ts` +- Modify: `src/types.ts` (add `Config` fields) +- Modify: `src/config.ts` (parse/validate + resolve) +- Create: `tests/unit/config-backup.test.ts` + +**Interfaces:** +- Produces: + ```ts + export interface ProviderSpec { + provider: "anthropic" | "openai" | "openai-compatible"; + anthropicApiKey?: string; anthropicModel: string; anthropicBaseUrl?: string; + openaiApiKey?: string; openaiModel: string; openaiBaseUrl?: string; + localAiBaseUrl?: string; localAiApiKey?: string; localAiModel: string; localAiJsonMode: boolean; + timeoutMs: number; label?: string; + } + export function buildProvider(spec: ProviderSpec): AIProvider; + ``` +- Produces (Config additions): `backupAiProvider?`, resolved `backup*` fields, `primaryAiTimeoutMs: number`, `backupCircuitThreshold: number`, `backupCircuitCooldownMs: number`. + +- [ ] **Step 1: Create the provider factory** + +Create `src/ai/provider-factory.ts`: + +```ts +import { AIProvider } from "../types.js"; +import { AnthropicProvider } from "./anthropic.js"; +import { OpenAIProvider } from "./openai.js"; +import { OpenAICompatibleProvider } from "./openai-compatible.js"; + +/** A fully-resolved recipe for one provider. Both the primary and the backup + * are built from one of these, so construction lives in exactly one place. */ +export interface ProviderSpec { + provider: "anthropic" | "openai" | "openai-compatible"; + anthropicApiKey?: string; + anthropicModel: string; + anthropicBaseUrl?: string; + openaiApiKey?: string; + openaiModel: string; + openaiBaseUrl?: string; + localAiBaseUrl?: string; + localAiApiKey?: string; + localAiModel: string; + localAiJsonMode: boolean; + timeoutMs: number; + /** Overrides the openai-compatible provider label (for cost/log attribution + * when a same-type backup would otherwise collide with the primary). */ + label?: string; +} + +export function buildProvider(spec: ProviderSpec): AIProvider { + if (spec.provider === "anthropic") { + return new AnthropicProvider(spec.anthropicApiKey!, spec.anthropicModel, spec.anthropicBaseUrl, spec.timeoutMs); + } + if (spec.provider === "openai-compatible") { + return new OpenAICompatibleProvider({ + baseURL: spec.localAiBaseUrl!, + model: spec.localAiModel, + apiKey: spec.localAiApiKey, + jsonMode: spec.localAiJsonMode, + timeoutMs: spec.timeoutMs, + providerLabel: spec.label, + }); + } + return new OpenAIProvider(spec.openaiApiKey!, spec.openaiModel, spec.openaiBaseUrl, spec.timeoutMs); +} +``` + +- [ ] **Step 2: Add the new `Config` fields** + +In `src/types.ts`, inside `export interface Config` near the existing AI fields (`aiRequestTimeoutMs: number;`), add: + +```ts + // ─── Backup AI provider / failover (all optional; off unless backupAiProvider set) ─── + /** When set, wrap the primary provider in a FailoverProvider with this backup. */ + backupAiProvider?: "anthropic" | "openai" | "openai-compatible"; + backupAnthropicApiKey?: string; + backupAnthropicModel?: string; + backupAnthropicBaseUrl?: string; + backupOpenaiApiKey?: string; + backupOpenaiModel?: string; + backupOpenaiBaseUrl?: string; + backupLocalAiBaseUrl?: string; + backupLocalAiApiKey?: string; + backupLocalAiModel?: string; + backupLocalAiJsonMode?: boolean; + /** Short per-op deadline for the PRIMARY when a backup is configured, so a + * slow-hang fails over quickly. Ignored (primary uses aiRequestTimeoutMs) + * when no backup is set. */ + primaryAiTimeoutMs: number; + /** Consecutive primary failures before the failover breaker opens. */ + backupCircuitThreshold: number; + /** How long (ms) the failover breaker stays open. */ + backupCircuitCooldownMs: number; +``` + +- [ ] **Step 3: Write the failing config test** + +Create `tests/unit/config-backup.test.ts`: + +```ts +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { loadConfig } from "../../src/config.js"; + +// loadConfig requires the core GitHub vars; set a minimal valid baseline and +// vary only the AI/backup vars per test. We snapshot + restore process.env. +const BASE: Record = { + GITHUB_APP_ID: "1", + GITHUB_PRIVATE_KEY: "key", + GITHUB_WEBHOOK_SECRET: "secret", + AI_PROVIDER: "openai-compatible", + LOCAL_AI_BASE_URL: "http://localhost:1234/v1", + LOCAL_AI_MODEL: "grok-4.5", +}; + +let saved: NodeJS.ProcessEnv; +beforeEach(() => { + saved = process.env; + // Fresh env containing only what each test sets (plus BASE). + process.env = { ...BASE } as NodeJS.ProcessEnv; +}); +afterEach(() => { + process.env = saved; +}); + +describe("backup provider config", () => { + it("is off by default (no BACKUP_AI_PROVIDER)", () => { + const cfg = loadConfig(); + expect(cfg.backupAiProvider).toBeUndefined(); + }); + + it("reuses primary env when only BACKUP_AI_PROVIDER is set", () => { + process.env.ANTHROPIC_API_KEY = "sk-ant-reused"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + const cfg = loadConfig(); + expect(cfg.backupAiProvider).toBe("anthropic"); + expect(cfg.backupAnthropicApiKey).toBe("sk-ant-reused"); + }); + + it("prefers BACKUP_* overrides over primary env", () => { + process.env.ANTHROPIC_API_KEY = "sk-ant-primary"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + process.env.BACKUP_ANTHROPIC_API_KEY = "sk-ant-backup"; + process.env.BACKUP_ANTHROPIC_MODEL = "claude-opus-4-8"; + const cfg = loadConfig(); + expect(cfg.backupAnthropicApiKey).toBe("sk-ant-backup"); + expect(cfg.backupAnthropicModel).toBe("claude-opus-4-8"); + }); + + it("throws when the backup credential is missing", () => { + process.env.BACKUP_AI_PROVIDER = "anthropic"; // no ANTHROPIC key anywhere + expect(() => loadConfig()).toThrow(/BACKUP_AI_PROVIDER=anthropic/); + }); + + it("rejects an unknown BACKUP_AI_PROVIDER", () => { + process.env.BACKUP_AI_PROVIDER = "claude"; + expect(() => loadConfig()).toThrow(/BACKUP_AI_PROVIDER/); + }); + + it("defaults the short primary timeout and breaker knobs", () => { + process.env.ANTHROPIC_API_KEY = "sk"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + const cfg = loadConfig(); + expect(cfg.primaryAiTimeoutMs).toBe(20_000); + expect(cfg.backupCircuitThreshold).toBe(3); + expect(cfg.backupCircuitCooldownMs).toBe(60_000); + }); + + it("clamps the primary timeout to at most AI_REQUEST_TIMEOUT_MS", () => { + process.env.ANTHROPIC_API_KEY = "sk"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + process.env.AI_REQUEST_TIMEOUT_MS = "15000"; + process.env.PRIMARY_AI_TIMEOUT_MS = "20000"; + const cfg = loadConfig(); + expect(cfg.primaryAiTimeoutMs).toBe(15_000); + }); +}); +``` + +- [ ] **Step 4: Run test to verify it fails** + +Run: `npx vitest run tests/unit/config-backup.test.ts` +Expected: FAIL — `backupAiProvider`/`primaryAiTimeoutMs` undefined and no validation. + +- [ ] **Step 5: Implement config parsing/validation** + +In `src/config.ts`, after the `aiRequestTimeoutMs` block (around line 83) and before the `ignoredPatterns` block, add: + +```ts + // ─── Backup AI provider (failover) — off unless BACKUP_AI_PROVIDER is set ─── + const backupRaw = process.env.BACKUP_AI_PROVIDER; + let backupAiProvider: Config["backupAiProvider"]; + let backupAnthropicApiKey: string | undefined; + let backupAnthropicModel: string | undefined; + let backupAnthropicBaseUrl: string | undefined; + let backupOpenaiApiKey: string | undefined; + let backupOpenaiModel: string | undefined; + let backupOpenaiBaseUrl: string | undefined; + let backupLocalAiBaseUrl: string | undefined; + let backupLocalAiApiKey: string | undefined; + let backupLocalAiModel: string | undefined; + let backupLocalAiJsonMode: boolean | undefined; + + if (backupRaw) { + if (!isAiProvider(backupRaw)) { + throw new Error( + `BACKUP_AI_PROVIDER must be one of: ${AI_PROVIDERS.join(", ")} (got: ${backupRaw})` + ); + } + backupAiProvider = backupRaw; + + // Reuse-with-override: BACKUP_* wins, else fall back to the primary's env. + backupAnthropicApiKey = process.env.BACKUP_ANTHROPIC_API_KEY || process.env.ANTHROPIC_API_KEY; + backupAnthropicModel = process.env.BACKUP_ANTHROPIC_MODEL || process.env.ANTHROPIC_MODEL || DEFAULT_ANTHROPIC_MODEL; + backupAnthropicBaseUrl = process.env.BACKUP_ANTHROPIC_BASE_URL || process.env.ANTHROPIC_BASE_URL; + backupOpenaiApiKey = process.env.BACKUP_OPENAI_API_KEY || process.env.OPENAI_API_KEY; + backupOpenaiModel = process.env.BACKUP_OPENAI_MODEL || process.env.OPENAI_MODEL || DEFAULT_OPENAI_MODEL; + backupOpenaiBaseUrl = process.env.BACKUP_OPENAI_BASE_URL || process.env.OPENAI_BASE_URL; + backupLocalAiBaseUrl = process.env.BACKUP_LOCAL_AI_BASE_URL || process.env.LOCAL_AI_BASE_URL; + backupLocalAiApiKey = process.env.BACKUP_LOCAL_AI_API_KEY || process.env.LOCAL_AI_API_KEY; + backupLocalAiModel = process.env.BACKUP_LOCAL_AI_MODEL || process.env.LOCAL_AI_MODEL || ""; + backupLocalAiJsonMode = + (process.env.BACKUP_LOCAL_AI_JSON_MODE || process.env.LOCAL_AI_JSON_MODE || "true").toLowerCase() !== "false"; + + // Fail fast if the resolved backup can't actually be constructed. + if (backupAiProvider === "anthropic" && !backupAnthropicApiKey) { + throw new Error("BACKUP_AI_PROVIDER=anthropic requires BACKUP_ANTHROPIC_API_KEY or ANTHROPIC_API_KEY"); + } + if (backupAiProvider === "openai" && !backupOpenaiApiKey) { + throw new Error("BACKUP_AI_PROVIDER=openai requires BACKUP_OPENAI_API_KEY or OPENAI_API_KEY"); + } + if (backupAiProvider === "openai-compatible" && (!backupLocalAiBaseUrl || !backupLocalAiModel)) { + throw new Error( + "BACKUP_AI_PROVIDER=openai-compatible requires BACKUP_LOCAL_AI_BASE_URL and BACKUP_LOCAL_AI_MODEL " + + "(or the primary LOCAL_AI_BASE_URL / LOCAL_AI_MODEL to reuse)" + ); + } + } + + // Short primary deadline (only meaningful when a backup is configured). Clamp + // to at most the normal bound so the primary is never given LONGER than the + // overall per-op budget. + const parsedPrimaryTimeout = parseInt(process.env.PRIMARY_AI_TIMEOUT_MS || "", 10); + let primaryAiTimeoutMs = Number.isFinite(parsedPrimaryTimeout) ? parsedPrimaryTimeout : 20_000; + if (aiRequestTimeoutMs > 0 && primaryAiTimeoutMs > aiRequestTimeoutMs) { + primaryAiTimeoutMs = aiRequestTimeoutMs; + } + + const parsedThreshold = parseInt(process.env.BACKUP_CIRCUIT_THRESHOLD || "", 10); + const backupCircuitThreshold = Number.isFinite(parsedThreshold) && parsedThreshold >= 1 ? parsedThreshold : 3; + + const parsedCooldown = parseInt(process.env.BACKUP_CIRCUIT_COOLDOWN_MS || "", 10); + const backupCircuitCooldownMs = Number.isFinite(parsedCooldown) && parsedCooldown >= 0 ? parsedCooldown : 60_000; +``` + +Then add these to the returned `Config` object literal (after `aiRequestTimeoutMs,`): + +```ts + backupAiProvider, + backupAnthropicApiKey, + backupAnthropicModel, + backupAnthropicBaseUrl, + backupOpenaiApiKey, + backupOpenaiModel, + backupOpenaiBaseUrl, + backupLocalAiBaseUrl, + backupLocalAiApiKey, + backupLocalAiModel, + backupLocalAiJsonMode, + primaryAiTimeoutMs, + backupCircuitThreshold, + backupCircuitCooldownMs, +``` + +- [ ] **Step 6: Run tests + typecheck to verify pass** + +Run: `npx vitest run tests/unit/config-backup.test.ts && npx tsc --noEmit` +Expected: PASS; no type errors. + +- [ ] **Step 7: Commit** + +```bash +git add src/ai/provider-factory.ts src/config.ts src/types.ts tests/unit/config-backup.test.ts +git commit -m "Add provider factory and backup-provider config (off by default) + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +### Task 4: Wire failover into the Reviewer + backup footnote + +Use `buildProvider` in the `Reviewer` constructor, wrap in `FailoverProvider` when a backup is configured, and append the subtle "reviewed by backup" footnote to the posted body. + +**Files:** +- Modify: `src/reviewer.ts` (constructor `211–223`; body assembly near `1372`; imports) + +**Interfaces:** +- Consumes: `buildProvider`, `ProviderSpec` (Task 3); `FailoverProvider` (Task 2); `Config` backup fields (Task 3). + +- [ ] **Step 1: Add imports** + +At the top of `src/reviewer.ts`, alongside the existing provider imports, add: + +```ts +import { buildProvider, ProviderSpec } from "./ai/provider-factory.js"; +import { FailoverProvider } from "./ai/failover.js"; +``` + +(Keep the existing `AnthropicProvider`/`OpenAIProvider`/`OpenAICompatibleProvider` imports only if still referenced elsewhere; if the constructor was their sole use, remove them to satisfy `noUnusedLocals`. Verify with `npx tsc --noEmit`.) + +- [ ] **Step 2: Replace the constructor provider block** + +In `src/reviewer.ts`, replace the `if (config.aiProvider === "anthropic") { … } else { … }` block (currently lines ~211–223) with: + +```ts + const primarySpec: ProviderSpec = { + provider: config.aiProvider, + anthropicApiKey: config.anthropicApiKey, + anthropicModel: config.anthropicModel, + anthropicBaseUrl: config.anthropicBaseUrl, + openaiApiKey: config.openaiApiKey, + openaiModel: config.openaiModel, + openaiBaseUrl: config.openaiBaseUrl, + localAiBaseUrl: config.localAiBaseUrl, + localAiApiKey: config.localAiApiKey, + localAiModel: config.localAiModel, + localAiJsonMode: config.localAiJsonMode, + // Short deadline ONLY when there's a backup to fail over to; otherwise the + // primary keeps the full budget (unchanged behavior). + timeoutMs: config.backupAiProvider ? config.primaryAiTimeoutMs : config.aiRequestTimeoutMs, + }; + const primary = buildProvider(primarySpec); + + if (config.backupAiProvider) { + const backupSpec: ProviderSpec = { + provider: config.backupAiProvider, + anthropicApiKey: config.backupAnthropicApiKey, + anthropicModel: config.backupAnthropicModel ?? config.anthropicModel, + anthropicBaseUrl: config.backupAnthropicBaseUrl, + openaiApiKey: config.backupOpenaiApiKey, + openaiModel: config.backupOpenaiModel ?? config.openaiModel, + openaiBaseUrl: config.backupOpenaiBaseUrl, + localAiBaseUrl: config.backupLocalAiBaseUrl, + localAiApiKey: config.backupLocalAiApiKey, + localAiModel: config.backupLocalAiModel ?? "", + localAiJsonMode: config.backupLocalAiJsonMode ?? true, + timeoutMs: config.aiRequestTimeoutMs, + // Distinguish a same-type backup in cost/log attribution. + label: config.backupAiProvider === "openai-compatible" ? "openai-compatible-backup" : undefined, + }; + const backup = buildProvider(backupSpec); + this.ai = new FailoverProvider(primary, backup, { + circuitThreshold: config.backupCircuitThreshold, + circuitCooldownMs: config.backupCircuitCooldownMs, + }); + logger.info( + { primary: config.aiProvider, backup: config.backupAiProvider }, + "AI failover enabled (primary → backup)", + ); + } else { + this.ai = primary; + } +``` + +(If `logger` is not already imported in `reviewer.ts`, use the module's existing logging import; verify by grep. If none exists, add `import { logger } from "./logger.js";`.) + +- [ ] **Step 3: Append the backup footnote to the review body** + +In `src/reviewer.ts`, immediately after the statement that assigns +`reviewResult.summary = formatReviewBody(reviewResult, { … });` (ends around line 1395), add: + +```ts + if (reviewResult.servedBy === "backup") { + reviewResult.summary += + "\n\nℹ️ The primary review model was unavailable; this review was generated by the configured backup provider."; + } +``` + +- [ ] **Step 4: Typecheck + full test suite + lint** + +Run: `npx tsc --noEmit && npm test && npm run lint` +Expected: PASS. In particular the pre-existing suite is green (no-backup path unchanged). + +- [ ] **Step 5: Build to confirm the server compiles** + +Run: `npm run build` +Expected: clean build. + +- [ ] **Step 6: Commit** + +```bash +git add src/reviewer.ts +git commit -m "Wire FailoverProvider into Reviewer; note backup-served reviews + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +### Task 5: Documentation + +Document the new env vars so operators can turn the feature on. + +**Files:** +- Modify: `.env.example` +- Modify: `README.md` (or the docs page covering AI provider config — grep for `AI_PROVIDER`) + +- [ ] **Step 1: Add the backup block to `.env.example`** + +After the existing AI provider / `AI_REQUEST_TIMEOUT_MS` section, add: + +```bash +# ─── Backup AI provider / failover (optional; off unless BACKUP_AI_PROVIDER set) ─── +# When the primary provider fails transiently (timeout, 5xx, 429, network), the +# review falls over to this backup so a review is still posted. Peer-quality +# backup recommended. Does NOT fail over on 401/403 (surfaces misconfiguration). +# BACKUP_AI_PROVIDER=anthropic # "anthropic" | "openai" | "openai-compatible" +# Backup creds REUSE the matching primary vars unless a BACKUP_* override is set: +# BACKUP_ANTHROPIC_API_KEY=sk-ant-... # else reuses ANTHROPIC_API_KEY +# BACKUP_ANTHROPIC_MODEL=claude-opus-4-8 # else reuses ANTHROPIC_MODEL +# BACKUP_ANTHROPIC_BASE_URL= # else reuses ANTHROPIC_BASE_URL +# BACKUP_OPENAI_API_KEY= # else reuses OPENAI_API_KEY +# BACKUP_OPENAI_MODEL= # else reuses OPENAI_MODEL +# BACKUP_OPENAI_BASE_URL= # else reuses OPENAI_BASE_URL +# BACKUP_LOCAL_AI_BASE_URL= # else reuses LOCAL_AI_BASE_URL +# BACKUP_LOCAL_AI_MODEL= # else reuses LOCAL_AI_MODEL +# BACKUP_LOCAL_AI_API_KEY= # else reuses LOCAL_AI_API_KEY +# BACKUP_LOCAL_AI_JSON_MODE= # else reuses LOCAL_AI_JSON_MODE +# +# Short deadline given to the PRIMARY when a backup is configured, so a slow-hang +# fails over quickly (clamped to <= AI_REQUEST_TIMEOUT_MS). Default 20000. +# PRIMARY_AI_TIMEOUT_MS=20000 +# Circuit breaker: after N consecutive primary failures, skip the primary for a +# cooldown and go straight to the backup. +# BACKUP_CIRCUIT_THRESHOLD=3 +# BACKUP_CIRCUIT_COOLDOWN_MS=60000 +``` + +- [ ] **Step 2: Add a short README section** + +Find the provider-config docs: `grep -rn "AI_PROVIDER" README.md docs/`. In the AI-provider section, add a short "Backup provider / failover" subsection summarizing: off by default; set `BACKUP_AI_PROVIDER` to enable; reuse-with-override; fails over on transient errors only (not auth); primary gets a short deadline; circuit breaker guards a down primary. Keep it to a short paragraph + the key vars. + +- [ ] **Step 3: Commit** + +```bash +git add .env.example README.md docs/ +git commit -m "Document backup AI provider / failover env vars + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +## Self-Review + +**Spec coverage:** +- Trigger policy (transient-only + short primary deadline) → Task 1 (predicate), Task 2 (`run`), Task 3 (`primaryAiTimeoutMs`), Task 4 (wiring). ✓ +- All-five-methods per-call sequential failover → Task 2. ✓ +- Peer-quality / output parity (fall through on backup failure) → Task 2 (`run` rethrows backup error). ✓ +- Config reuse-with-override, off by default → Task 3. ✓ +- Idempotency (sequential) → inherent in Task 2 `run`; no code needed. ✓ +- Observability (structured logs, per-provider cost) → Task 2 logs + Task 4 `label`. ✓ +- Circuit breaker → Task 2. ✓ +- Review-body transparency note → Task 2 (`servedBy` field) + Task 4 (footnote). ✓ +- Docs → Task 5. ✓ + +**Placeholder scan:** No TBD/TODO; every code step shows full code. Task 5 Step 2 references a grep because README structure isn't pinned, but gives the exact content to add. ✓ + +**Type consistency:** `ProviderSpec` fields identical across Task 3 (definition) and Task 4 (construction). `FailoverOptions` (`circuitThreshold`, `circuitCooldownMs`, `now?`) consistent across Task 2 definition, tests, and Task 4 call site. `ReviewResult.servedBy` defined in Task 2, set in Task 2, read in Task 4. `isTransientError` signature consistent Task 1 ↔ Task 2. ✓ diff --git a/docs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.md b/docs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.md new file mode 100644 index 0000000..e5c5567 --- /dev/null +++ b/docs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.md @@ -0,0 +1,317 @@ +# Backup AI Provider / Failover — Design + +**Date:** 2026-07-09 +**Status:** Approved (brainstorm), pending implementation +**Author:** Claude + Luke + +## Problem + +On PR #77, DiffSentry reviews intermittently failed with +`AiTimeoutError: AI request timed out after 60000ms (openai-compatible review)`. +On one push all retries hit the 60s timeout and the review was **dead-lettered** +(no review posted). Model: `grok-4.5` via an OpenAI-compatible endpoint. + +Diagnosis established the dominant failure mode was **slow-hang / latency +variance on the large `review` call**, not a fast error or a confirmed outage: +the smaller `walkthrough` call (same endpoint/auth) succeeded in ~3s on every +failed attempt; the same `review` call finished in ~9s on other runs. + +Two levers came out of that: +1. **Operational (out of scope here):** raise `AI_REQUEST_TIMEOUT_MS` / scope + retries. Immediate mitigation, may already be applied on the deployment. +2. **This feature:** a real resilience feature — a secondary provider to fall + back to so a degraded primary still yields a posted review. + +This feature is **complementary** to lever 1, not a replacement. + +## Ground truth (verified against the code) + +- `AIProvider` (`src/types.ts:425`) has **five** methods: `review`, + `generateWalkthrough`, `chat`, `chatIssue`, `complete`. A wrapper must + implement all five. +- Three implementations conform: `AnthropicProvider` (`src/ai/anthropic.ts`), + `OpenAIProvider` (`src/ai/openai.ts`), `OpenAICompatibleProvider` + (`src/ai/openai-compatible.ts`). + - `AnthropicProvider(apiKey, model, baseURL?, timeoutMs?)` — provider label + hardcoded `"anthropic"`. + - `OpenAIProvider(apiKey, model, baseURL?, timeoutMs?)` — label hardcoded + `"openai"`. + - `OpenAICompatibleProvider({ baseURL, model, apiKey?, jsonMode?, + providerLabel?, timeoutMs? })` — accepts a `providerLabel`. +- The provider is constructed **once** in the `Reviewer` constructor + (`src/reviewer.ts:211–223`) from flat `config` fields. +- Every model call is bounded by `withAiTimeout` (`src/ai/timeout.ts`) with a + single per-provider `timeoutMs` = `config.aiRequestTimeoutMs` + (`AI_REQUEST_TIMEOUT_MS`, default `DEFAULT_AI_REQUEST_TIMEOUT_MS = 60_000`). + On timeout it rejects with typed `AiTimeoutError` (`isAiTimeoutError` helper) + **before** cost is recorded, so a timed-out call writes no cost event. +- **Retry / dead-letter is NOT in `reviewer.ts`.** It lives in + `src/realtime/jobs.ts::runReviewJob`: bounded retry (default 3 attempts, + `REVIEW_RETRY_MAX_ATTEMPTS`) with exponential backoff, then dead-letter, only + on `isTransientError`. It re-runs the **entire** `handlePullRequest` + (walkthrough + review + verify + posting). The reviewer's own catch + (`reviewer.ts:1558`) posts a failure status comment and **rethrows**. +- `isTransientError` (`jobs.ts:110`) classifies transient = transient network + `code`s, HTTP status `>= 500` or `429`, `name` `AbortError`/`TimeoutError`, + or a message matching hint substrings. `AiTimeoutError` is matched via the + `"timed out"` message hint (its `.name` is `"AiTimeoutError"`). +- Cost attribution (`recordAiUsage`, `src/ai/cost.ts`) keys on **provider + + model**. A backup of a different provider type, or the same type with a + different model, is therefore already distinct in cost accounting. + +## Design decisions (agreed in brainstorm) + +| # | Decision | Choice | +|---|----------|--------| +| 1 | Trigger policy | Fast-fail on transient errors **+ short primary deadline** so slow-hang switches quickly. Never fail over on 4xx (auth/bad-request). | +| 2 | Failover scope | All five `AIProvider` methods, per-call, **sequential** (primary then backup). | +| 3 | Backup role | Peer-quality strong model. Output parity is realistic. | +| 4 | Output parity | Rely on existing `parseReviewResponse` + `verify.ts`. Backup failure falls through to today's failure/retry/dead-letter path (no regression). | +| 5 | Config surface | Env only (secrets), off by default: `BACKUP_AI_PROVIDER` selects type and **reuses** that provider's existing env, with optional `BACKUP_*` overrides. | +| 6 | Idempotency | Free — sequential per-call failover means the primary is aborted before the backup runs; no double-post window. No machinery. | +| 7 | Observability | Structured logs on failover + breaker transitions. Per-provider cost already attributed. Plus a subtle review-body "reviewed by backup" note. | +| 8 | Cost/abuse guardrail | In-memory circuit breaker on the wrapper: after N consecutive primary failures, skip primary for a cooldown. | + +## Architecture + +### `FailoverProvider implements AIProvider` — `src/ai/failover.ts` + +Wraps a `primary` and `backup` `AIProvider`. Each of the five methods follows +the same shape: + +``` +async (...args) { + return this.run("", () => primary.(...args), + () => backup.(...args)); +} +``` + +`run(operation, primaryCall, backupCall)`: +1. If the circuit breaker is **open** (and cooldown not elapsed), skip the + primary and call the backup directly (record `servedBy: "backup"`, reason + `"circuit-open"`). +2. Otherwise call the primary. + - On success: record a primary success (resets the breaker's consecutive- + failure count / closes a half-open breaker). Return the result. + - On error: + - If **not** `shouldFailover(err)` (e.g. a 401/403, a 400, or any non- + transient error): record a primary failure for breaker accounting **only + if it is a connection/5xx-class failure** (auth/4xx must not trip the + breaker — see below), then **rethrow** (no backup attempt). + - If `shouldFailover(err)`: record a primary failure, log the failover with + structured fields, then call the backup. + - Backup success: return the backup result (annotated `servedBy: + "backup"` where the result type carries it). + - Backup failure: rethrow the **backup** error (the more recent / + actionable one) so the job runner's retry/dead-letter path takes over + exactly as today. + +The wrapper adds **no** timeout of its own; it relies on each inner provider's +own `withAiTimeout`. The primary instance is constructed with the short +`primaryAiTimeoutMs`; the backup with the normal `aiRequestTimeoutMs`. + +### Trigger predicate — shared, extracted + +`shouldFailover(err)` must use the **same** rules as `jobs.ts::isTransientError` +to avoid drift. Extract `isTransientError` (and its `TRANSIENT_CODES`, +`TRANSIENT_MESSAGE_HINTS`, `statusOf`) into a new neutral module +`src/ai/transient.ts`. `jobs.ts` imports it from there (no behavior change); +`failover.ts` reuses it as the failover predicate. + +- Fails over on: `AiTimeoutError`, transient network codes, HTTP `>= 500`, `429`. +- Does **not** fail over on: 401/403 (auth) and other 4xx — surfaced as-is so a + bad primary key isn't silently masked by backup traffic. + +**Breaker vs. failover distinction:** the breaker counts *primary reachability* +failures. A 4xx that doesn't fail over also should **not** trip the breaker +(it's deterministic, not a reachability problem). So the breaker increments only +when `shouldFailover(err)` is true; a non-failover error rethrows without +touching breaker state. + +### Circuit breaker + +In-memory state on the `FailoverProvider` instance (the `Reviewer` is +effectively a singleton, so state persists across reviews within a process): + +- `consecutiveFailures: number` +- `openedAt: number | null` (epoch ms; `Date.now()`) + +Behavior: +- Increment `consecutiveFailures` on each failover-eligible primary failure. +- When `consecutiveFailures >= threshold` (default 3), set `openedAt = now` + → breaker **open**. +- While open and `now - openedAt < cooldownMs` (default 60_000): skip primary, + go straight to backup. +- After cooldown: **half-open** — allow one primary probe. Success closes the + breaker (`consecutiveFailures = 0`, `openedAt = null`); failure re-opens + (`openedAt = now`). +- Any primary success closes the breaker. + +Log `warn` on open, `info` on close, with counts. + +### Provider construction — `buildProvider(spec)` helper + +Extract the anthropic/openai/openai-compatible switch out of the `Reviewer` +constructor into a reusable factory (`src/ai/provider-factory.ts`, or a local +private function in `reviewer.ts`). Signature: + +```ts +interface ProviderSpec { + provider: "anthropic" | "openai" | "openai-compatible"; + anthropicApiKey?: string; anthropicModel: string; anthropicBaseUrl?: string; + openaiApiKey?: string; openaiModel: string; openaiBaseUrl?: string; + localAiBaseUrl?: string; localAiApiKey?: string; localAiModel: string; + localAiJsonMode: boolean; + timeoutMs: number; + label?: string; // openai-compatible providerLabel override +} +function buildProvider(spec: ProviderSpec): AIProvider; +``` + +`Reviewer` constructor: +1. Build the **primary** provider from the existing flat config fields, with + `timeoutMs = config.backupAiProvider ? config.primaryAiTimeoutMs : config.aiRequestTimeoutMs`. + (When no backup is configured, the primary keeps the full 60s — the short + deadline only makes sense when there's somewhere to fail over to.) +2. If `config.backupAiProvider` is set, build the **backup** provider from the + resolved backup fields with `timeoutMs = config.aiRequestTimeoutMs`, then set + `this.ai = new FailoverProvider(primary, backup, { threshold, cooldownMs })`. +3. Else `this.ai = primary` (unchanged behavior). + +### Review-body transparency note (#7) + +Add an optional `servedBy?: "primary" | "backup"` field to `ReviewResult` +(`src/types.ts`). The `FailoverProvider.review` sets it to `"backup"` when the +backup served the review (leaves it unset/`"primary"` otherwise). `reviewer.ts`, +when rendering the review body, appends a small footnote when +`reviewResult.servedBy === "backup"`, e.g.: + +> ℹ️ Primary review model was unavailable; this review was generated by the +> configured backup provider. + +Keep it subtle and additive — it must not alter approval/verification logic. +Only the `review` result carries the note (the substantive posted artifact); the +walkthrough's served-by is captured in logs only. + +## Config surface (env, off by default) + +New env vars (all optional; absence of `BACKUP_AI_PROVIDER` = feature off): + +| Var | Meaning | Default | +|-----|---------|---------| +| `BACKUP_AI_PROVIDER` | `anthropic` \| `openai` \| `openai-compatible`. Unset ⇒ failover disabled. | (unset) | +| `PRIMARY_AI_TIMEOUT_MS` | Short primary deadline used only when a backup is configured. | `20000`, clamped to `≤ AI_REQUEST_TIMEOUT_MS` | +| `BACKUP_ANTHROPIC_API_KEY` / `BACKUP_ANTHROPIC_MODEL` / `BACKUP_ANTHROPIC_BASE_URL` | Overrides; reuse `ANTHROPIC_*` when unset. | reuse primary `ANTHROPIC_*` | +| `BACKUP_OPENAI_API_KEY` / `BACKUP_OPENAI_MODEL` / `BACKUP_OPENAI_BASE_URL` | Overrides; reuse `OPENAI_*` when unset. | reuse primary `OPENAI_*` | +| `BACKUP_LOCAL_AI_BASE_URL` / `BACKUP_LOCAL_AI_API_KEY` / `BACKUP_LOCAL_AI_MODEL` / `BACKUP_LOCAL_AI_JSON_MODE` | Overrides; reuse `LOCAL_AI_*` when unset. | reuse primary `LOCAL_AI_*` | +| `BACKUP_CIRCUIT_THRESHOLD` | Consecutive primary failures before opening the breaker. | `3` | +| `BACKUP_CIRCUIT_COOLDOWN_MS` | How long the breaker stays open. | `60000` | + +**Reuse-with-override resolution:** for `BACKUP_AI_PROVIDER=X`, the backup's +credentials/model come from `BACKUP_X_*` if set, else fall back to the primary's +`X_*` env. This makes "primary grok, backup Anthropic" a two-line config +(`BACKUP_AI_PROVIDER=anthropic` + an existing `ANTHROPIC_API_KEY`), while still +supporting a distinct model/key/endpoint of the same type as the primary. + +**Validation (fail fast at boot, mirroring the primary checks):** when +`BACKUP_AI_PROVIDER` is set, require the resolved credentials for that type: +- `anthropic` ⇒ resolved Anthropic API key present. +- `openai` ⇒ resolved OpenAI API key present. +- `openai-compatible` ⇒ resolved base URL **and** model present. + +`Config` (`src/types.ts`) gains: `backupAiProvider?`, the resolved backup +fields, `primaryAiTimeoutMs: number`, `backupCircuitThreshold: number`, +`backupCircuitCooldownMs: number`. + +## Interaction with the job runner + +Per-call failover nests **inside** the existing job-level retry. If the backup +also fails transiently, `review()` throws transient → `handlePullRequest` +rethrows → `runReviewJob` retries the whole pipeline (attempt 2), which again +tries primary→backup. The circuit breaker makes those retries efficient: an open +breaker sends the retry straight to the backup instead of re-eating the primary +stall. No change to `jobs.ts` retry logic is required beyond the +`isTransientError` import move. + +## Failure modes & edge cases + +- **Backup itself times out / errors:** rethrow the backup error → existing + retry/dead-letter path. No regression vs. today. +- **Backup output fails to parse:** `parseReviewResponse` already tolerant; + a genuine parse failure surfaces via the existing parse-failure banner path, + same as a primary parse failure. +- **Auth misconfig on primary (401/403):** does not fail over, does not trip the + breaker; surfaces loudly (so ops notices the bad key) exactly as today. +- **No backup configured:** `this.ai` is the plain primary with the full 60s + timeout — byte-for-byte current behavior. +- **Same-type, same-model backup:** cost accounting collapses them (identical + provider+model) — harmless; logs still show `servedBy`. +- **Breaker state is per-process, in-memory:** resets on restart. Acceptable — + it's a cost/latency guard, not a correctness mechanism. + +## Testing strategy + +No unit harness exists around `reviewer.reviewPR`; test the wrapper in isolation. + +**`src/ai/failover.test.ts`** — fake `AIProvider` stubs (spies on all five +methods): +- Primary success ⇒ backup never called; result returned. +- Primary transient error (`AiTimeoutError`, 503, `ECONNRESET`) ⇒ backup called; + backup result returned; `servedBy === "backup"` on `review`. +- Primary 401/403 ⇒ backup **not** called; error rethrown; breaker untouched. +- Both fail ⇒ throws the **backup** error. +- Breaker opens after `threshold` consecutive failover-eligible failures ⇒ + subsequent call skips primary, hits backup directly. +- Breaker half-open probe after cooldown: primary success closes it; failure + re-opens. (Inject a clock so cooldown is testable without real time.) +- Primary success resets `consecutiveFailures`. +- All five methods delegate to the correct inner method with the same args. + +**`src/ai/transient.test.ts`** — the extracted predicate keeps its current +classifications (port the relevant assertions from any existing `jobs`/ +`isTransientError` coverage; add cases for `AiTimeoutError`, 500, 429, 401→false, +400→false). + +**Config tests** — `BACKUP_AI_PROVIDER` unset ⇒ no backup fields / failover off; +set with reuse ⇒ resolves primary env; set with overrides ⇒ resolves overrides; +missing required backup credential ⇒ throws at `loadConfig`. + +**Clock injection:** `FailoverProvider` takes an optional `now: () => number` +(default `Date.now`) so breaker timing is deterministic in tests. + +## Files to touch + +| File | Change | +|------|--------| +| `src/ai/transient.ts` | **New.** Extract `isTransientError` + helpers. | +| `src/realtime/jobs.ts` | Import `isTransientError` from `../ai/transient.js` (drop local copy). Re-export if any test imports it from here. | +| `src/ai/failover.ts` | **New.** `FailoverProvider` + circuit breaker. | +| `src/ai/provider-factory.ts` | **New** (or private in reviewer). `buildProvider(spec)`. | +| `src/config.ts` | Parse + validate backup env; resolve reuse/override; `primaryAiTimeoutMs`, breaker knobs. | +| `src/types.ts` | `Config` backup fields; `ReviewResult.servedBy?`. | +| `src/reviewer.ts` | Use `buildProvider`; conditionally wrap in `FailoverProvider`; append backup footnote when `servedBy === "backup"`. | +| `src/ai/failover.test.ts` | **New.** Wrapper unit tests. | +| `src/ai/transient.test.ts` | **New.** Predicate tests. | +| `test/…config…` | Backup config parse/validate tests (match existing config test location). | +| `.env.example` | Document the new `BACKUP_*` / `PRIMARY_AI_TIMEOUT_MS` vars. | +| `README` / relevant docs | Short "backup provider / failover" section. | + +## Out of scope + +- Changing the operational mitigation (raising `AI_REQUEST_TIMEOUT_MS`, retry + scoping) — that is lever 1, tracked separately. +- Concurrent/hedged requests (running primary and backup in parallel). Sequential + only, by decision #2/#6. +- Persisting breaker state across restarts. +- A third+ provider chain. Two-provider (primary + one backup) only. + +## Success criteria + +- With `BACKUP_AI_PROVIDER` unset, behavior is unchanged (all existing tests + pass; primary keeps the 60s deadline). +- With a backup configured, a primary transient failure (incl. a primary + short-timeout) results in a backup-served review rather than a dead-letter. +- A primary 401/403 surfaces without silently routing to the backup. +- A persistently-down primary opens the breaker and stops paying the primary + stall on every review. +- `tsc --noEmit` clean, `npm test` green, `npm run lint` clean. diff --git a/src/ai/failover.ts b/src/ai/failover.ts new file mode 100644 index 0000000..0db97f3 --- /dev/null +++ b/src/ai/failover.ts @@ -0,0 +1,163 @@ +import { + AIProvider, + PRContext, + ReviewResult, + WalkthroughResult, + RepoConfig, + Learning, + IssueContext, +} from "../types.js"; +import { isTransientError } from "./transient.js"; +import { logger } from "../logger.js"; + +export interface FailoverOptions { + /** Consecutive failover-eligible primary failures before the breaker opens. */ + circuitThreshold: number; + /** How long (ms) the breaker stays open, routing straight to the backup. */ + circuitCooldownMs: number; + /** Injectable clock for deterministic tests. Defaults to Date.now. */ + now?: () => number; +} + +// ───────────────────────────────────────────────────────────────────────────── +// Sequential try-primary-then-backup wrapper around two AIProviders. +// +// - Fails over ONLY on transient errors (isTransientError): timeouts, 5xx, +// 429, network blips. A 4xx (auth / bad request) rethrows without touching +// the backup or the breaker — a bad primary key must surface, not silently +// route all traffic to the backup. +// - The primary is constructed with a SHORT deadline and the backup with the +// normal one (see reviewer.ts), so a slow-hang on the primary switches fast. +// - A circuit breaker skips a persistently-down primary for a cooldown so we +// don't pay the primary's stall on every single review during an outage. +// +// State is in-memory on the instance (the Reviewer is effectively a singleton), +// so it persists across reviews within a process and resets on restart — it is a +// cost/latency guard, not a correctness mechanism. +// ───────────────────────────────────────────────────────────────────────────── +export class FailoverProvider implements AIProvider { + private consecutiveFailures = 0; + private openedAt: number | null = null; + private readonly threshold: number; + private readonly cooldownMs: number; + private readonly now: () => number; + + constructor( + private readonly primary: AIProvider, + private readonly backup: AIProvider, + opts: FailoverOptions, + ) { + this.threshold = opts.circuitThreshold; + this.cooldownMs = opts.circuitCooldownMs; + this.now = opts.now ?? Date.now; + } + + /** True while the breaker is open and the cooldown has not yet elapsed. Once + * the cooldown passes we return false so the next call probes the primary + * (half-open). */ + private breakerOpen(): boolean { + if (this.openedAt === null) return false; + return this.now() - this.openedAt < this.cooldownMs; + } + + private recordPrimarySuccess(): void { + if (this.openedAt !== null || this.consecutiveFailures > 0) { + logger.info({ mode: "failover" }, "Primary AI provider recovered — closing circuit breaker"); + } + this.consecutiveFailures = 0; + this.openedAt = null; + } + + private recordPrimaryFailure(): void { + this.consecutiveFailures += 1; + if (this.consecutiveFailures >= this.threshold) { + const wasOpen = this.openedAt !== null; + this.openedAt = this.now(); + if (!wasOpen) { + logger.warn( + { mode: "failover", consecutiveFailures: this.consecutiveFailures }, + "Primary AI provider failing repeatedly — opening circuit breaker (routing to backup)", + ); + } + } + } + + /** + * Run one operation with failover. Returns the result plus which provider + * served it. Never runs primary and backup concurrently. + */ + private async run( + operation: string, + primaryCall: () => Promise, + backupCall: () => Promise, + ): Promise<{ result: T; servedBy: "primary" | "backup" }> { + if (this.breakerOpen()) { + logger.warn({ mode: "failover", operation }, "Circuit breaker open — routing directly to backup AI provider"); + return { result: await backupCall(), servedBy: "backup" }; + } + + const startedAt = this.now(); + try { + const result = await primaryCall(); + this.recordPrimarySuccess(); + return { result, servedBy: "primary" }; + } catch (err) { + if (!isTransientError(err)) { + // Deterministic failure (e.g. 401/403/400) — do NOT fail over and do + // NOT trip the breaker; surface it so misconfiguration is visible. + throw err; + } + this.recordPrimaryFailure(); + logger.warn( + { mode: "failover", operation, primaryLatencyMs: this.now() - startedAt, err }, + "Primary AI provider failed transiently — failing over to backup", + ); + return { result: await backupCall(), servedBy: "backup" }; + } + } + + async review(context: PRContext, repoConfig?: RepoConfig, learnings?: Learning[]): Promise { + const { result, servedBy } = await this.run( + "review", + () => this.primary.review(context, repoConfig, learnings), + () => this.backup.review(context, repoConfig, learnings), + ); + return servedBy === "backup" ? { ...result, servedBy } : result; + } + + async generateWalkthrough(context: PRContext, repoConfig?: RepoConfig): Promise { + const { result } = await this.run( + "walkthrough", + () => this.primary.generateWalkthrough(context, repoConfig), + () => this.backup.generateWalkthrough(context, repoConfig), + ); + return result; + } + + async chat(context: PRContext, userMessage: string, repoConfig?: RepoConfig): Promise { + const { result } = await this.run( + "chat", + () => this.primary.chat(context, userMessage, repoConfig), + () => this.backup.chat(context, userMessage, repoConfig), + ); + return result; + } + + async chatIssue(context: IssueContext, userMessage: string, repoConfig?: RepoConfig): Promise { + const { result } = await this.run( + "issue_chat", + () => this.primary.chatIssue(context, userMessage, repoConfig), + () => this.backup.chatIssue(context, userMessage, repoConfig), + ); + return result; + } + + async complete(system: string, user: string, opts?: { maxTokens?: number; json?: boolean }): Promise { + const { result } = await this.run( + "complete", + () => this.primary.complete(system, user, opts), + () => this.backup.complete(system, user, opts), + ); + return result; + } +} diff --git a/src/ai/provider-factory.ts b/src/ai/provider-factory.ts new file mode 100644 index 0000000..97390d1 --- /dev/null +++ b/src/ai/provider-factory.ts @@ -0,0 +1,60 @@ +import { AIProvider } from "../types.js"; +import { AnthropicProvider } from "./anthropic.js"; +import { OpenAIProvider } from "./openai.js"; +import { OpenAICompatibleProvider } from "./openai-compatible.js"; + +/** A fully-resolved recipe for one provider. Both the primary and the backup + * are built from one of these, so construction lives in exactly one place. */ +export interface ProviderSpec { + provider: "anthropic" | "openai" | "openai-compatible"; + anthropicApiKey?: string; + anthropicModel: string; + anthropicBaseUrl?: string; + openaiApiKey?: string; + openaiModel: string; + openaiBaseUrl?: string; + localAiBaseUrl?: string; + localAiApiKey?: string; + localAiModel: string; + localAiJsonMode: boolean; + timeoutMs: number; + /** Overrides the openai-compatible provider label for cost/log attribution. + * Only `OpenAICompatibleProvider` accepts a label, so this disambiguates a + * same-type openai-compatible backup from the primary. For anthropic/openai, + * attribution relies on provider+model distinctness (`recordAiUsage` keys on + * both): a backup with a different model is already distinct; a same-provider + * AND same-model backup collapses into the primary in cost/logs (an unusual + * config). Extending the anthropic/openai constructors to take a label would + * be the follow-up if that pairing ever needs splitting. */ + label?: string; +} + +export function buildProvider(spec: ProviderSpec): AIProvider { + // loadConfig validates the configured provider's credentials at boot, but this + // factory is a shared construction path for both primary and backup — throw a + // named error here so a future partial spec fails fast at the factory rather + // than with an opaque failure deep inside a provider SDK constructor. + if (spec.provider === "anthropic") { + if (!spec.anthropicApiKey) { + throw new Error("buildProvider(anthropic) requires anthropicApiKey"); + } + return new AnthropicProvider(spec.anthropicApiKey, spec.anthropicModel, spec.anthropicBaseUrl, spec.timeoutMs); + } + if (spec.provider === "openai-compatible") { + if (!spec.localAiBaseUrl || !spec.localAiModel) { + throw new Error("buildProvider(openai-compatible) requires localAiBaseUrl and localAiModel"); + } + return new OpenAICompatibleProvider({ + baseURL: spec.localAiBaseUrl, + model: spec.localAiModel, + apiKey: spec.localAiApiKey, + jsonMode: spec.localAiJsonMode, + timeoutMs: spec.timeoutMs, + providerLabel: spec.label, + }); + } + if (!spec.openaiApiKey) { + throw new Error("buildProvider(openai) requires openaiApiKey"); + } + return new OpenAIProvider(spec.openaiApiKey, spec.openaiModel, spec.openaiBaseUrl, spec.timeoutMs); +} diff --git a/src/ai/transient.ts b/src/ai/transient.ts new file mode 100644 index 0000000..466dfbf --- /dev/null +++ b/src/ai/transient.ts @@ -0,0 +1,62 @@ +// ───────────────────────────────────────────────────────────────────────────── +// Shared classification of "transient" AI/network errors — worth a retry or a +// failover, as opposed to a deterministic 4xx that will just fail again. +// +// Used by the job runner (bounded retry / dead-letter) AND the FailoverProvider +// (primary → backup). Keeping one definition prevents the two from drifting. +// ───────────────────────────────────────────────────────────────────────────── + +/** Network-layer error codes that warrant a retry / failover. */ +const TRANSIENT_CODES = new Set([ + "ECONNRESET", + "ETIMEDOUT", + "ECONNREFUSED", + "ENOTFOUND", + "EAI_AGAIN", + "EPIPE", + "ENETUNREACH", + "UND_ERR_CONNECT_TIMEOUT", + "UND_ERR_HEADERS_TIMEOUT", + "UND_ERR_SOCKET", +]); + +const TRANSIENT_MESSAGE_HINTS = [ + "timed out", + "timeout", + "etimedout", + "econnreset", + "socket hang up", + "network", + "fetch failed", + "temporarily unavailable", + "service unavailable", + "rate limit", + "too many requests", +]; + +/** Read an HTTP-ish status off an error (Octokit RequestError, fetch wrappers). */ +function statusOf(err: unknown): number | undefined { + const s = (err as { status?: unknown; statusCode?: unknown })?.status ?? (err as { statusCode?: unknown })?.statusCode; + return typeof s === "number" ? s : undefined; +} + +/** + * Classify an error as transient (worth retrying / failing over) or permanent + * (fail fast). Transient = network blips, GitHub/AI 5xx + 429, request timeouts, + * and AbortError raised by an AI client's own timeout. NOTE: a cancel/abort from + * our own cancel path never reaches these callers, so a thrown AbortError here is + * an upstream timeout, not a cancellation. + */ +export function isTransientError(err: unknown): boolean { + const code = (err as { code?: unknown })?.code; + if (typeof code === "string" && TRANSIENT_CODES.has(code)) return true; + + const status = statusOf(err); + if (typeof status === "number" && (status >= 500 || status === 429)) return true; + + const name = (err as { name?: unknown })?.name; + if (name === "AbortError" || name === "TimeoutError") return true; + + const msg = (err instanceof Error ? err.message : String(err)).toLowerCase(); + return TRANSIENT_MESSAGE_HINTS.some((hint) => msg.includes(hint)); +} diff --git a/src/config.ts b/src/config.ts index 83b6da7..761742a 100644 --- a/src/config.ts +++ b/src/config.ts @@ -82,6 +82,85 @@ export function loadConfig(): Config { ? parsedTimeout : DEFAULT_AI_REQUEST_TIMEOUT_MS; + // ─── Backup AI provider (failover) — off unless BACKUP_AI_PROVIDER is set ─── + const backupRaw = process.env.BACKUP_AI_PROVIDER; + let backupAiProvider: Config["backupAiProvider"]; + let backupAnthropicApiKey: string | undefined; + let backupAnthropicModel: string | undefined; + let backupAnthropicBaseUrl: string | undefined; + let backupOpenaiApiKey: string | undefined; + let backupOpenaiModel: string | undefined; + let backupOpenaiBaseUrl: string | undefined; + let backupLocalAiBaseUrl: string | undefined; + let backupLocalAiApiKey: string | undefined; + let backupLocalAiModel: string | undefined; + let backupLocalAiJsonMode: boolean | undefined; + + if (backupRaw) { + if (!isAiProvider(backupRaw)) { + throw new Error( + `BACKUP_AI_PROVIDER must be one of: ${AI_PROVIDERS.join(", ")} (got: ${backupRaw})` + ); + } + backupAiProvider = backupRaw; + + // Reuse-with-override: BACKUP_* wins, else fall back to the primary's env. + backupAnthropicApiKey = process.env.BACKUP_ANTHROPIC_API_KEY || process.env.ANTHROPIC_API_KEY; + backupAnthropicModel = process.env.BACKUP_ANTHROPIC_MODEL || process.env.ANTHROPIC_MODEL || DEFAULT_ANTHROPIC_MODEL; + backupAnthropicBaseUrl = process.env.BACKUP_ANTHROPIC_BASE_URL || process.env.ANTHROPIC_BASE_URL; + backupOpenaiApiKey = process.env.BACKUP_OPENAI_API_KEY || process.env.OPENAI_API_KEY; + backupOpenaiModel = process.env.BACKUP_OPENAI_MODEL || process.env.OPENAI_MODEL || DEFAULT_OPENAI_MODEL; + backupOpenaiBaseUrl = process.env.BACKUP_OPENAI_BASE_URL || process.env.OPENAI_BASE_URL; + backupLocalAiBaseUrl = process.env.BACKUP_LOCAL_AI_BASE_URL || process.env.LOCAL_AI_BASE_URL; + backupLocalAiApiKey = process.env.BACKUP_LOCAL_AI_API_KEY || process.env.LOCAL_AI_API_KEY; + backupLocalAiModel = process.env.BACKUP_LOCAL_AI_MODEL || process.env.LOCAL_AI_MODEL || ""; + backupLocalAiJsonMode = + (process.env.BACKUP_LOCAL_AI_JSON_MODE || process.env.LOCAL_AI_JSON_MODE || "true").toLowerCase() !== "false"; + + // Fail fast if the resolved backup can't actually be constructed. + if (backupAiProvider === "anthropic" && !backupAnthropicApiKey) { + throw new Error("BACKUP_AI_PROVIDER=anthropic requires BACKUP_ANTHROPIC_API_KEY or ANTHROPIC_API_KEY"); + } + if (backupAiProvider === "openai" && !backupOpenaiApiKey) { + throw new Error("BACKUP_AI_PROVIDER=openai requires BACKUP_OPENAI_API_KEY or OPENAI_API_KEY"); + } + if (backupAiProvider === "openai-compatible" && (!backupLocalAiBaseUrl || !backupLocalAiModel)) { + throw new Error( + "BACKUP_AI_PROVIDER=openai-compatible requires BACKUP_LOCAL_AI_BASE_URL and BACKUP_LOCAL_AI_MODEL " + + "(or the primary LOCAL_AI_BASE_URL / LOCAL_AI_MODEL to reuse)" + ); + } + } + + // Short primary deadline (only meaningful when a backup is configured). Clamp + // to at most the normal bound so the primary is never given LONGER than the + // overall per-op budget. + // The short primary deadline and the breaker knobs only matter when a backup + // is configured; keep them at their defaults (and don't even read their env) + // when failover is off, so a mis-set PRIMARY_AI_TIMEOUT_MS can't affect an + // operator who never opted into failover. Preserves the "off by default = + // unchanged" invariant. + let primaryAiTimeoutMs = 20_000; + let backupCircuitThreshold = 3; + let backupCircuitCooldownMs = 60_000; + if (backupAiProvider) { + // A non-positive value would disable the primary's bound (withAiTimeout + // treats timeoutMs <= 0 as "no deadline"), leaving the primary un-bounded and + // defeating fast failover. Reject it and fall back to the 20s default. + const parsedPrimaryTimeout = parseInt(process.env.PRIMARY_AI_TIMEOUT_MS || "", 10); + primaryAiTimeoutMs = + Number.isFinite(parsedPrimaryTimeout) && parsedPrimaryTimeout > 0 ? parsedPrimaryTimeout : 20_000; + if (aiRequestTimeoutMs > 0 && primaryAiTimeoutMs > aiRequestTimeoutMs) { + primaryAiTimeoutMs = aiRequestTimeoutMs; + } + + const parsedThreshold = parseInt(process.env.BACKUP_CIRCUIT_THRESHOLD || "", 10); + backupCircuitThreshold = Number.isFinite(parsedThreshold) && parsedThreshold >= 1 ? parsedThreshold : 3; + + const parsedCooldown = parseInt(process.env.BACKUP_CIRCUIT_COOLDOWN_MS || "", 10); + backupCircuitCooldownMs = Number.isFinite(parsedCooldown) && parsedCooldown >= 0 ? parsedCooldown : 60_000; + } + const ignoredPatterns = (process.env.IGNORED_PATTERNS || "") .split(",") .map((p) => p.trim()) @@ -117,6 +196,20 @@ export function loadConfig(): Config { localAiModel: process.env.LOCAL_AI_MODEL || "", localAiJsonMode: (process.env.LOCAL_AI_JSON_MODE || "true").toLowerCase() !== "false", aiRequestTimeoutMs, + backupAiProvider, + backupAnthropicApiKey, + backupAnthropicModel, + backupAnthropicBaseUrl, + backupOpenaiApiKey, + backupOpenaiModel, + backupOpenaiBaseUrl, + backupLocalAiBaseUrl, + backupLocalAiApiKey, + backupLocalAiModel, + backupLocalAiJsonMode, + primaryAiTimeoutMs, + backupCircuitThreshold, + backupCircuitCooldownMs, maxFilesPerReview: parseInt(process.env.MAX_FILES_PER_REVIEW || "50", 10), ignoredPatterns: [...defaultIgnored, ...ignoredPatterns], botName: process.env.BOT_NAME || "diffsentry", diff --git a/src/realtime/jobs.ts b/src/realtime/jobs.ts index ce48727..fbde600 100644 --- a/src/realtime/jobs.ts +++ b/src/realtime/jobs.ts @@ -7,6 +7,9 @@ import { listInFlightReviewJobs, } from "../storage/dao.js"; import { isShuttingDown } from "../shutdown.js"; +import { isTransientError } from "../ai/transient.js"; + +export { isTransientError } from "../ai/transient.js"; // ───────────────────────────────────────────────────────────────────────────── // Durable review job-runner — the resilience layer around a single review. @@ -65,62 +68,6 @@ function baseBackoffMs(): number { return Number.isFinite(raw) && raw >= 0 ? raw : DEFAULT_BASE_BACKOFF_MS; } -/** Network-layer error codes that warrant a retry. */ -const TRANSIENT_CODES = new Set([ - "ECONNRESET", - "ETIMEDOUT", - "ECONNREFUSED", - "ENOTFOUND", - "EAI_AGAIN", - "EPIPE", - "ENETUNREACH", - "UND_ERR_CONNECT_TIMEOUT", - "UND_ERR_HEADERS_TIMEOUT", - "UND_ERR_SOCKET", -]); - -const TRANSIENT_MESSAGE_HINTS = [ - "timed out", - "timeout", - "etimedout", - "econnreset", - "socket hang up", - "network", - "fetch failed", - "temporarily unavailable", - "service unavailable", - "rate limit", - "too many requests", -]; - -/** Read an HTTP-ish status off an error (Octokit RequestError, fetch wrappers). */ -function statusOf(err: unknown): number | undefined { - const s = (err as { status?: unknown; statusCode?: unknown })?.status ?? (err as { statusCode?: unknown })?.statusCode; - return typeof s === "number" ? s : undefined; -} - -/** - * Classify an error thrown by a review attempt as transient (worth retrying) or - * permanent (fail fast). Transient = network blips, GitHub/AI 5xx + 429, request - * timeouts, and AbortError raised by an AI client's own timeout. NOTE: our cancel - * path never reaches here — handlePullRequest swallows an operator/superseded - * abort and returns without throwing — so a thrown AbortError is an upstream - * timeout, not a cancellation. - */ -export function isTransientError(err: unknown): boolean { - const code = (err as { code?: unknown })?.code; - if (typeof code === "string" && TRANSIENT_CODES.has(code)) return true; - - const status = statusOf(err); - if (typeof status === "number" && (status >= 500 || status === 429)) return true; - - const name = (err as { name?: unknown })?.name; - if (name === "AbortError" || name === "TimeoutError") return true; - - const msg = (err instanceof Error ? err.message : String(err)).toLowerCase(); - return TRANSIENT_MESSAGE_HINTS.some((hint) => msg.includes(hint)); -} - function errorMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } diff --git a/src/reviewer.ts b/src/reviewer.ts index c3e8095..0c8f439 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -1,8 +1,7 @@ import { randomUUID } from "node:crypto"; import { Config, AIProvider, PRContext, RepoConfig, ReviewComment } from "./types.js"; -import { AnthropicProvider } from "./ai/anthropic.js"; -import { OpenAIProvider } from "./ai/openai.js"; -import { OpenAICompatibleProvider } from "./ai/openai-compatible.js"; +import { buildProvider, ProviderSpec } from "./ai/provider-factory.js"; +import { FailoverProvider } from "./ai/failover.js"; import { isAiTimeoutError } from "./ai/timeout.js"; import { GitHubClient } from "./github.js"; import { loadRepoConfig, mergeWithDefaults, shouldReviewPR, isPathIncluded } from "./repo-config.js"; @@ -208,18 +207,52 @@ export class Reviewer { this.github = new GitHubClient(config); this.learnings = new LearningsStore(config.learningsDir); - if (config.aiProvider === "anthropic") { - this.ai = new AnthropicProvider(config.anthropicApiKey!, config.anthropicModel, config.anthropicBaseUrl, config.aiRequestTimeoutMs); - } else if (config.aiProvider === "openai-compatible") { - this.ai = new OpenAICompatibleProvider({ - baseURL: config.localAiBaseUrl!, - model: config.localAiModel, - apiKey: config.localAiApiKey, - jsonMode: config.localAiJsonMode, + const primarySpec: ProviderSpec = { + provider: config.aiProvider, + anthropicApiKey: config.anthropicApiKey, + anthropicModel: config.anthropicModel, + anthropicBaseUrl: config.anthropicBaseUrl, + openaiApiKey: config.openaiApiKey, + openaiModel: config.openaiModel, + openaiBaseUrl: config.openaiBaseUrl, + localAiBaseUrl: config.localAiBaseUrl, + localAiApiKey: config.localAiApiKey, + localAiModel: config.localAiModel, + localAiJsonMode: config.localAiJsonMode, + // Short deadline ONLY when there's a backup to fail over to; otherwise the + // primary keeps the full budget (unchanged behavior). + timeoutMs: config.backupAiProvider ? config.primaryAiTimeoutMs : config.aiRequestTimeoutMs, + }; + const primary = buildProvider(primarySpec); + + if (config.backupAiProvider) { + const backupSpec: ProviderSpec = { + provider: config.backupAiProvider, + anthropicApiKey: config.backupAnthropicApiKey, + anthropicModel: config.backupAnthropicModel ?? config.anthropicModel, + anthropicBaseUrl: config.backupAnthropicBaseUrl, + openaiApiKey: config.backupOpenaiApiKey, + openaiModel: config.backupOpenaiModel ?? config.openaiModel, + openaiBaseUrl: config.backupOpenaiBaseUrl, + localAiBaseUrl: config.backupLocalAiBaseUrl, + localAiApiKey: config.backupLocalAiApiKey, + localAiModel: config.backupLocalAiModel ?? "", + localAiJsonMode: config.backupLocalAiJsonMode ?? true, timeoutMs: config.aiRequestTimeoutMs, + // Distinguish a same-type backup in cost/log attribution. + label: config.backupAiProvider === "openai-compatible" ? "openai-compatible-backup" : undefined, + }; + const backup = buildProvider(backupSpec); + this.ai = new FailoverProvider(primary, backup, { + circuitThreshold: config.backupCircuitThreshold, + circuitCooldownMs: config.backupCircuitCooldownMs, }); + logger.info( + { primary: config.aiProvider, backup: config.backupAiProvider }, + "AI failover enabled (primary → backup)", + ); } else { - this.ai = new OpenAIProvider(config.openaiApiKey!, config.openaiModel, config.openaiBaseUrl, config.aiRequestTimeoutMs); + this.ai = primary; } } @@ -1393,6 +1426,11 @@ export class Reviewer { botName: this.config.botName, }); + if (reviewResult.servedBy === "backup") { + reviewResult.summary += + "\n\nℹ️ The primary review model was unavailable; this review was generated by the configured backup provider."; + } + // Persist this review to SQLite (best-effort; no-op if DB disabled). // Done before GitHub submission so an API failure doesn't lose the data. recordRepo({ owner, repo, installationId }); diff --git a/src/types.ts b/src/types.ts index b7658f8..9feb6b9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -26,6 +26,27 @@ export interface Config { * as a "review failed (AI timeout)" outcome instead of stalling indefinitely. */ aiRequestTimeoutMs: number; + // ─── Backup AI provider / failover (all optional; off unless backupAiProvider set) ─── + /** When set, wrap the primary provider in a FailoverProvider with this backup. */ + backupAiProvider?: "anthropic" | "openai" | "openai-compatible"; + backupAnthropicApiKey?: string; + backupAnthropicModel?: string; + backupAnthropicBaseUrl?: string; + backupOpenaiApiKey?: string; + backupOpenaiModel?: string; + backupOpenaiBaseUrl?: string; + backupLocalAiBaseUrl?: string; + backupLocalAiApiKey?: string; + backupLocalAiModel?: string; + backupLocalAiJsonMode?: boolean; + /** Short per-op deadline for the PRIMARY when a backup is configured, so a + * slow-hang fails over quickly. Ignored (primary uses aiRequestTimeoutMs) + * when no backup is set. */ + primaryAiTimeoutMs: number; + /** Consecutive primary failures before the failover breaker opens. */ + backupCircuitThreshold: number; + /** How long (ms) the failover breaker stays open. */ + backupCircuitCooldownMs: number; maxFilesPerReview: number; ignoredPatterns: string[]; botName: string; @@ -315,6 +336,10 @@ export interface ReviewResult { * by downstream passes (e.g. severity calibration). Absent when the graph * was unavailable. */ fanInByFile?: Record; + /** Which provider produced this review. Set to "backup" by FailoverProvider + * when the primary failed over; absent/"primary" otherwise. Drives the + * subtle "reviewed by backup provider" footnote in the posted body. */ + servedBy?: "primary" | "backup"; } // ─── Walkthrough Result ──────────────────────────────────────── diff --git a/tests/unit/config-backup.test.ts b/tests/unit/config-backup.test.ts new file mode 100644 index 0000000..bba6fe2 --- /dev/null +++ b/tests/unit/config-backup.test.ts @@ -0,0 +1,136 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { loadConfig } from "../../src/config.js"; + +// loadConfig requires the core GitHub vars; set a minimal valid baseline and +// vary only the AI/backup vars per test. We snapshot + restore process.env. +const BASE: Record = { + GITHUB_APP_ID: "1", + GITHUB_PRIVATE_KEY: "key", + GITHUB_WEBHOOK_SECRET: "secret", + AI_PROVIDER: "openai-compatible", + LOCAL_AI_BASE_URL: "http://localhost:1234/v1", + LOCAL_AI_MODEL: "grok-4.5", +}; + +let saved: NodeJS.ProcessEnv; +beforeEach(() => { + saved = process.env; + // Fresh env containing only what each test sets (plus BASE). + process.env = { ...BASE } as NodeJS.ProcessEnv; +}); +afterEach(() => { + process.env = saved; +}); + +describe("backup provider config", () => { + it("is off by default (no BACKUP_AI_PROVIDER)", () => { + const cfg = loadConfig(); + expect(cfg.backupAiProvider).toBeUndefined(); + }); + + it("reuses primary env when only BACKUP_AI_PROVIDER is set", () => { + process.env.ANTHROPIC_API_KEY = "sk-ant-reused"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + const cfg = loadConfig(); + expect(cfg.backupAiProvider).toBe("anthropic"); + expect(cfg.backupAnthropicApiKey).toBe("sk-ant-reused"); + }); + + it("prefers BACKUP_* overrides over primary env", () => { + process.env.ANTHROPIC_API_KEY = "sk-ant-primary"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + process.env.BACKUP_ANTHROPIC_API_KEY = "sk-ant-backup"; + process.env.BACKUP_ANTHROPIC_MODEL = "claude-opus-4-8"; + const cfg = loadConfig(); + expect(cfg.backupAnthropicApiKey).toBe("sk-ant-backup"); + expect(cfg.backupAnthropicModel).toBe("claude-opus-4-8"); + }); + + it("throws when the backup credential is missing", () => { + process.env.BACKUP_AI_PROVIDER = "anthropic"; // no ANTHROPIC key anywhere + expect(() => loadConfig()).toThrow(/BACKUP_AI_PROVIDER=anthropic/); + }); + + it("rejects an unknown BACKUP_AI_PROVIDER", () => { + process.env.BACKUP_AI_PROVIDER = "claude"; + expect(() => loadConfig()).toThrow(/BACKUP_AI_PROVIDER/); + }); + + it("defaults the short primary timeout and breaker knobs", () => { + process.env.ANTHROPIC_API_KEY = "sk"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + const cfg = loadConfig(); + expect(cfg.primaryAiTimeoutMs).toBe(20_000); + expect(cfg.backupCircuitThreshold).toBe(3); + expect(cfg.backupCircuitCooldownMs).toBe(60_000); + }); + + it("clamps the primary timeout to at most AI_REQUEST_TIMEOUT_MS", () => { + process.env.ANTHROPIC_API_KEY = "sk"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + process.env.AI_REQUEST_TIMEOUT_MS = "15000"; + process.env.PRIMARY_AI_TIMEOUT_MS = "20000"; + const cfg = loadConfig(); + expect(cfg.primaryAiTimeoutMs).toBe(15_000); + }); + + it("falls back to 20000 for a non-positive PRIMARY_AI_TIMEOUT_MS", () => { + // 0 or negative would disable the primary's deadline (withAiTimeout treats + // <= 0 as no bound) and defeat fast failover — must not be accepted. + process.env.ANTHROPIC_API_KEY = "sk"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + process.env.PRIMARY_AI_TIMEOUT_MS = "0"; + expect(loadConfig().primaryAiTimeoutMs).toBe(20_000); + process.env.PRIMARY_AI_TIMEOUT_MS = "-5"; + expect(loadConfig().primaryAiTimeoutMs).toBe(20_000); + }); + + it("fail-soft parses the breaker knobs (invalid → defaults; cooldown 0 kept)", () => { + process.env.ANTHROPIC_API_KEY = "sk"; + process.env.BACKUP_AI_PROVIDER = "anthropic"; + // threshold: 0 / negative / non-numeric all fall back to 3 (min 1). + for (const bad of ["0", "-2", "abc"]) { + process.env.BACKUP_CIRCUIT_THRESHOLD = bad; + expect(loadConfig().backupCircuitThreshold).toBe(3); + } + delete process.env.BACKUP_CIRCUIT_THRESHOLD; + // cooldown: non-numeric → 60000, but an explicit 0 is a valid "no cooldown". + process.env.BACKUP_CIRCUIT_COOLDOWN_MS = "nope"; + expect(loadConfig().backupCircuitCooldownMs).toBe(60_000); + process.env.BACKUP_CIRCUIT_COOLDOWN_MS = "0"; + expect(loadConfig().backupCircuitCooldownMs).toBe(0); + }); + + it("leaves breaker/primary-timeout knobs at defaults when failover is off", () => { + // With no BACKUP_AI_PROVIDER, the knob env is not even read. + process.env.PRIMARY_AI_TIMEOUT_MS = "5"; + process.env.BACKUP_CIRCUIT_THRESHOLD = "99"; + const cfg = loadConfig(); + expect(cfg.backupAiProvider).toBeUndefined(); + expect(cfg.primaryAiTimeoutMs).toBe(20_000); + expect(cfg.backupCircuitThreshold).toBe(3); + expect(cfg.backupCircuitCooldownMs).toBe(60_000); + }); + + it("throws when openai-compatible backup has no base URL or model to resolve", () => { + process.env.AI_PROVIDER = "anthropic"; + process.env.ANTHROPIC_API_KEY = "sk-ant-primary"; + process.env.BACKUP_AI_PROVIDER = "openai-compatible"; + delete process.env.LOCAL_AI_BASE_URL; + delete process.env.LOCAL_AI_MODEL; + expect(() => loadConfig()).toThrow(/BACKUP_AI_PROVIDER=openai-compatible/); + }); + + it("resolves BACKUP_LOCAL_AI_* overrides for an openai-compatible backup", () => { + process.env.AI_PROVIDER = "anthropic"; + process.env.ANTHROPIC_API_KEY = "sk-ant-primary"; + process.env.BACKUP_AI_PROVIDER = "openai-compatible"; + delete process.env.LOCAL_AI_BASE_URL; + delete process.env.LOCAL_AI_MODEL; + process.env.BACKUP_LOCAL_AI_BASE_URL = "http://localhost:9999/v1"; + process.env.BACKUP_LOCAL_AI_MODEL = "backup-model"; + const cfg = loadConfig(); + expect(cfg.backupLocalAiBaseUrl).toBe("http://localhost:9999/v1"); + expect(cfg.backupLocalAiModel).toBe("backup-model"); + }); +}); diff --git a/tests/unit/failover.test.ts b/tests/unit/failover.test.ts new file mode 100644 index 0000000..20c6e4d --- /dev/null +++ b/tests/unit/failover.test.ts @@ -0,0 +1,182 @@ +import { describe, it, expect, vi } from "vitest"; +import { FailoverProvider } from "../../src/ai/failover.js"; +import { AiTimeoutError } from "../../src/ai/timeout.js"; +import type { AIProvider, ReviewResult, PRContext } from "../../src/types.js"; + +function ctx(): PRContext { + return { + owner: "o", repo: "r", pullNumber: 1, title: "t", description: "", + baseBranch: "main", headBranch: "feat", headSha: "sha", files: [], diff: "", + } as unknown as PRContext; +} + +function review(summary: string): ReviewResult { + return { summary, comments: [], approval: "COMMENT" }; +} + +/** Minimal fake provider; only the methods a test exercises are stubbed. */ +function fakeProvider(over: Partial): AIProvider { + const notImpl = () => { throw new Error("not stubbed"); }; + return { + review: over.review ?? (notImpl as AIProvider["review"]), + generateWalkthrough: over.generateWalkthrough ?? (notImpl as AIProvider["generateWalkthrough"]), + chat: over.chat ?? (notImpl as AIProvider["chat"]), + chatIssue: over.chatIssue ?? (notImpl as AIProvider["chatIssue"]), + complete: over.complete ?? (notImpl as AIProvider["complete"]), + }; +} + +const OPTS = { circuitThreshold: 3, circuitCooldownMs: 60_000 }; + +describe("FailoverProvider", () => { + it("returns the primary result and never calls the backup on success", async () => { + const backupReview = vi.fn(); + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockResolvedValue(review("primary")) }), + fakeProvider({ review: backupReview }), + OPTS, + ); + const res = await p.review(ctx()); + expect(res.summary).toBe("primary"); + expect(res.servedBy).toBeUndefined(); + expect(backupReview).not.toHaveBeenCalled(); + }); + + it("fails over to the backup on a transient primary error and tags servedBy", async () => { + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)) }), + fakeProvider({ review: vi.fn().mockResolvedValue(review("backup")) }), + OPTS, + ); + const res = await p.review(ctx()); + expect(res.summary).toBe("backup"); + expect(res.servedBy).toBe("backup"); + }); + + it("does NOT fail over on a 401 and rethrows", async () => { + const backupReview = vi.fn(); + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockRejectedValue(Object.assign(new Error("unauthorized"), { status: 401 })) }), + fakeProvider({ review: backupReview }), + OPTS, + ); + await expect(p.review(ctx())).rejects.toThrow("unauthorized"); + expect(backupReview).not.toHaveBeenCalled(); + }); + + it("rethrows the backup error when both fail", async () => { + const p = new FailoverProvider( + fakeProvider({ review: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)) }), + fakeProvider({ review: vi.fn().mockRejectedValue(new Error("backup down")) }), + OPTS, + ); + await expect(p.review(ctx())).rejects.toThrow("backup down"); + }); + + it("opens the breaker after threshold consecutive transient failures, then routes straight to backup", async () => { + const primaryReview = vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)); + const backupReview = vi.fn().mockResolvedValue(review("backup")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: backupReview }), + OPTS, + ); + // 3 failing-then-failover calls trip the breaker. + await p.review(ctx()); + await p.review(ctx()); + await p.review(ctx()); + expect(primaryReview).toHaveBeenCalledTimes(3); + // 4th call: breaker open → primary skipped entirely, still tagged servedBy. + const openRes = await p.review(ctx()); + expect(primaryReview).toHaveBeenCalledTimes(3); + expect(backupReview).toHaveBeenCalledTimes(4); + expect(openRes.servedBy).toBe("backup"); + }); + + it("half-opens after cooldown; a primary success closes the breaker", async () => { + let clock = 1_000; + const now = () => clock; + const primaryReview = vi + .fn() + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockResolvedValue(review("primary-recovered")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: vi.fn().mockResolvedValue(review("backup")) }), + { ...OPTS, now }, + ); + await p.review(ctx()); await p.review(ctx()); await p.review(ctx()); // breaker opens + clock += 60_001; // cooldown elapsed → half-open probe hits primary + const res = await p.review(ctx()); + expect(res.summary).toBe("primary-recovered"); + expect(res.servedBy).toBeUndefined(); // primary served + }); + + it("re-opens the breaker for a fresh cooldown when the half-open probe fails", async () => { + let clock = 1_000; + const now = () => clock; + // Primary fails every time; backup always serves. + const primaryReview = vi.fn().mockRejectedValue(new AiTimeoutError("primary", "review", 20000)); + const backupReview = vi.fn().mockResolvedValue(review("backup")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: backupReview }), + { ...OPTS, now }, + ); + await p.review(ctx()); await p.review(ctx()); await p.review(ctx()); // breaker opens (3 probes) + expect(primaryReview).toHaveBeenCalledTimes(3); + + clock += 60_001; // cooldown elapsed → next call is a half-open probe on the primary + await p.review(ctx()); + expect(primaryReview).toHaveBeenCalledTimes(4); // probe hit the primary... + + // ...and its failure must re-arm the cooldown. The immediately-following call + // stays on the backup instead of re-probing the just-failed primary. + await p.review(ctx()); + expect(primaryReview).toHaveBeenCalledTimes(4); // primary skipped — breaker re-opened + expect(backupReview).toHaveBeenCalledTimes(5); + }); + + it("resets consecutive failures on a primary success", async () => { + const primaryReview = vi + .fn() + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockRejectedValueOnce(new AiTimeoutError("primary", "review", 20000)) + .mockResolvedValueOnce(review("ok")) // resets counter + .mockRejectedValue(new AiTimeoutError("primary", "review", 20000)); + const backupReview = vi.fn().mockResolvedValue(review("backup")); + const p = new FailoverProvider( + fakeProvider({ review: primaryReview }), + fakeProvider({ review: backupReview }), + OPTS, + ); + await p.review(ctx()); await p.review(ctx()); // 2 failures + await p.review(ctx()); // success → reset + await p.review(ctx()); // 1 failure (breaker still closed) + expect(primaryReview).toHaveBeenCalledTimes(4); // primary always attempted (never skipped) + }); + + it("delegates the non-review methods and fails them over too", async () => { + const p = new FailoverProvider( + fakeProvider({ + generateWalkthrough: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "walkthrough", 20000)), + chat: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "chat", 20000)), + chatIssue: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "issue_chat", 20000)), + complete: vi.fn().mockRejectedValue(new AiTimeoutError("primary", "complete", 20000)), + }), + fakeProvider({ + generateWalkthrough: vi.fn().mockResolvedValue({ summary: "wt", fileDescriptions: [] }), + chat: vi.fn().mockResolvedValue("chat-backup"), + chatIssue: vi.fn().mockResolvedValue("issue-backup"), + complete: vi.fn().mockResolvedValue("complete-backup"), + }), + OPTS, + ); + expect((await p.generateWalkthrough(ctx())).summary).toBe("wt"); + expect(await p.chat(ctx(), "hi")).toBe("chat-backup"); + expect(await p.chatIssue({} as never, "hi")).toBe("issue-backup"); + expect(await p.complete("sys", "usr")).toBe("complete-backup"); + }); +}); diff --git a/tests/unit/transient.test.ts b/tests/unit/transient.test.ts new file mode 100644 index 0000000..3842ac7 --- /dev/null +++ b/tests/unit/transient.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect } from "vitest"; +import { isTransientError } from "../../src/ai/transient.js"; +import { AiTimeoutError } from "../../src/ai/timeout.js"; + +describe("isTransientError", () => { + it("treats an AiTimeoutError as transient", () => { + expect(isTransientError(new AiTimeoutError("openai-compatible", "review", 20000))).toBe(true); + }); + + it("treats transient network codes as transient", () => { + expect(isTransientError({ code: "ECONNRESET" })).toBe(true); + expect(isTransientError({ code: "ETIMEDOUT" })).toBe(true); + }); + + it("treats HTTP 5xx and 429 as transient", () => { + expect(isTransientError({ status: 503 })).toBe(true); + expect(isTransientError({ status: 500 })).toBe(true); + expect(isTransientError({ status: 429 })).toBe(true); + }); + + it("does NOT treat auth/4xx as transient", () => { + expect(isTransientError({ status: 401 })).toBe(false); + expect(isTransientError({ status: 403 })).toBe(false); + expect(isTransientError({ status: 400 })).toBe(false); + }); + + it("treats AbortError / TimeoutError names as transient", () => { + expect(isTransientError({ name: "AbortError" })).toBe(true); + expect(isTransientError({ name: "TimeoutError" })).toBe(true); + }); + + it("matches transient message hints", () => { + expect(isTransientError(new Error("socket hang up"))).toBe(true); + expect(isTransientError(new Error("service unavailable"))).toBe(true); + }); + + it("treats an ordinary error as non-transient", () => { + expect(isTransientError(new Error("bad request: invalid model"))).toBe(false); + }); +});