fix(selfhost): validate AI reviewer-provider configuration and add a failure circuit breaker

prometheus/rules/alerts.yml

@@ -369,3 +369,39 @@ groups:
           summary: "gittensory p95 request latency above 1s"
           description: "p95 HTTP request latency is {{ $value | printf \"%.2f\" }}s over the last 5m (sustained 10m), breaching the 1s SLO."
           runbook: "Check whether slowness is queue/DB/AI-bound: correlate with gittensory_queue_pending and Qdrant/AI latency. A rising p95 with flat error rate usually means a saturated dependency, not a bug."
+
+  # ── AI review provider health (#2540) ───────────────────────────────────────
+  - name: gittensory-ai
+    rules:
+      - alert: GittensoryAiProviderCircuitOpen
+        # A provider's per-provider circuit breaker (createChainAi) has been open (tripping repeated attempts,
+        # not just one) for a sustained window -- the provider is known-bad for this whole window, not a
+        # one-off blip. `increase(...) > 0` over the alert window plus `for: 15m` requires the breaker to
+        # still be actively tripping partway through, not a single trip that then recovered.
+        expr: increase(gittensory_ai_provider_circuit_total{result="tripped"}[15m]) > 0
+        for: 15m
+        labels:
+          severity: warning
+        annotations:
+          summary: "gittensory AI provider {{ $labels.provider }} circuit breaker open"
+          description: "The {{ $labels.provider }} AI provider has tripped its circuit breaker and stayed open for over 15m -- reviews are falling through to the next configured provider (or failing if none remain)."
+          runbook: "Check the provider's credentials/quota/reachability (API key, CLI auth, network egress). gittensory_ai_provider_circuit_total{result=\"skipped\"} shows how many reviews are bypassing it. Clears automatically once a call succeeds."
+
+      - alert: GittensoryAiReviewInconclusiveSpike
+        # A HELD (fail-closed, no usable AI output) verdict is not itself an error -- it's the SAFE outcome
+        # when review output can't be trusted. But a SUSTAINED spike means dual-AI review is providing
+        # materially less signal than configured, often correlated with a provider circuit trip above.
+        # `> 0` denominator guard avoids 0/0 = NaN when no block-mode reviews ran in the window.
+        expr: |
+          (
+            sum(rate(gittensory_ai_review_inconclusive_total[15m]))
+            /
+            sum(rate(gittensory_ai_requests_total[15m])) > 0
+          ) > 0.25
+        for: 15m
+        labels:
+          severity: warning
+        annotations:
+          summary: "gittensory AI review inconclusive-verdict ratio above 25%"
+          description: "{{ $value | humanizePercentage }} of AI review activity resulted in an inconclusive (held) verdict over the last 15m (sustained 15m)."
+          runbook: "Correlate with GittensoryAiProviderCircuitOpen and selfhost_ai_providers_exhausted logs. A spike usually means one or more configured reviewers are down; check credentials and provider status."

src/selfhost/ai.ts

@@ -634,21 +634,70 @@ export function markAiProviderUnhealthyAtBoot(): void {
   aiConsecutiveFailures = AI_UNHEALTHY_FAILURE_STREAK;
 }
 
-/** Try each provider in order until one returns; if all throw, rethrow the last error so the caller degrades
- *  (AI summary → "unavailable"; the review still runs deterministically). The fallback chain is what makes a
- *  BYOK setup robust — e.g. AI_PROVIDER="anthropic,ollama" uses the API first and a local model if it's down. */
+// Per-provider circuit breaker (#2540): the readiness streak above only tracks "the WHOLE chain exhausted" --
+// it says nothing about ONE provider within a multi-provider chain (e.g. AI_PROVIDER="anthropic,ollama") being
+// known-bad, so every single PR review still pays that provider's full failure latency (a slow API timeout or
+// CLI hang) before falling through to the next, every time, for the whole outage. Trip a provider's breaker
+// after a short run of CONSECUTIVE failures and skip it (no network/CLI call at all) for a cooldown window,
+// falling straight through to the next provider in the chain -- degrading review latency/cost gracefully
+// instead of repeating a doomed attempt on every review. In-process only (no persistence layer), matching this
+// module's existing aiConsecutiveFailures streak.
+const AI_PROVIDER_CIRCUIT_FAILURE_STREAK = 3;
+const AI_PROVIDER_CIRCUIT_COOLDOWN_MS = 5 * 60_000;
+type ProviderCircuitState = { consecutiveFailures: number; cooldownUntilMs: number };
+const providerCircuits = new Map<string, ProviderCircuitState>();
+
+function isProviderCircuitOpen(providerName: string, nowMs = Date.now()): boolean {
+  const state = providerCircuits.get(providerName);
+  return state !== undefined && state.cooldownUntilMs > nowMs;
+}
+
+function recordProviderCircuitSuccess(providerName: string): void {
+  if (providerCircuits.delete(providerName)) {
+    incr("gittensory_ai_provider_circuit_total", { provider: providerName, result: "recovered" });
+  }
+}
+
+function recordProviderCircuitFailure(providerName: string, nowMs = Date.now()): void {
+  const state = providerCircuits.get(providerName) ?? { consecutiveFailures: 0, cooldownUntilMs: 0 };
+  state.consecutiveFailures += 1;
+  if (state.consecutiveFailures >= AI_PROVIDER_CIRCUIT_FAILURE_STREAK) {
+    state.cooldownUntilMs = nowMs + AI_PROVIDER_CIRCUIT_COOLDOWN_MS;
+    incr("gittensory_ai_provider_circuit_total", { provider: providerName, result: "tripped" });
+  }
+  providerCircuits.set(providerName, state);
+}
+
+/** Test-only reset so circuit-breaker state from one test can't leak into the next (module-level Map). */
+export function resetAiProviderCircuitsForTest(): void {
+  providerCircuits.clear();
+}
+
+/** Try each provider in order until one returns; if all throw (or are circuit-open), rethrow the last real error
+ *  so the caller degrades (AI summary → "unavailable"; the review still runs deterministically). The fallback
+ *  chain is what makes a BYOK setup robust — e.g. AI_PROVIDER="anthropic,ollama" uses the API first and a local
+ *  model if it's down. */
 export function createChainAi(providers: Array<{ name: string; ai: SelfHostAi }>): SelfHostAi {
   return {
     async run(model, options) {
       let lastError: unknown = new Error("no_ai_providers");
+      let attempted = false;
       const failures: Array<{ provider: string; error: string }> = [];
       for (const p of providers) {
+        if (isProviderCircuitOpen(p.name)) {
+          failures.push({ provider: p.name, error: "circuit_open: skipped after repeated recent failures" });
+          incr("gittensory_ai_provider_circuit_total", { provider: p.name, result: "skipped" });
+          continue;
+        }
+        attempted = true;
         try {
           const result = await runProviderWithOtel(p, model, options);
           aiConsecutiveFailures = 0;
+          recordProviderCircuitSuccess(p.name);
           return result;
         } catch (error) {
           lastError = error;
+          recordProviderCircuitFailure(p.name);
           failures.push({ provider: p.name, error: errorMessage(error) });
           console.error(JSON.stringify({ level: "warn", event: "selfhost_ai_provider_failed_in_chain", provider: p.name, error: errorMessage(error) }));
         }
@@ -665,6 +714,9 @@ export function createChainAi(providers: Array<{ name: string; ai: SelfHostAi }>
           error: errorMessage(lastError),
         }),
       );
+      // Every provider was circuit-open (a real attempt never ran) -- surface a distinct, actionable error
+      // instead of the generic "no_ai_providers" default, which would misleadingly imply NOTHING was configured.
+      if (!attempted && providers.length > 0) throw new Error("all_ai_providers_circuit_open");
       throw lastError instanceof Error ? lastError : new Error("all_ai_providers_failed");
     },
   };
@@ -798,6 +850,18 @@ export function resolveAiReviewerPlan(
   const names = resolveProviderNames(env);
   if (names.length === 0) return undefined;
   if (names.length === 1) return { reviewers: [{ model: names[0] as string }], combine: "single", onMerge: undefined };
+  // #2540: a duplicate AI_PROVIDER entry feeding BOTH dual-review slots (e.g. "claude-code,claude-code" -- a
+  // copy-paste config mistake) would otherwise silently collapse "two independent reviewers reaching consensus"
+  // into "the same provider called twice", defeating dual-AI review entirely AND meaning a single provider's
+  // outage/auth failure takes down both slots at once. Fail loud at boot, mirroring
+  // assertNoLegacySharedAiEnv's pattern for other misconfigured env combinations, rather than silently
+  // degrading review quality. Only the first two names feed the two slots, so only THOSE need to differ --
+  // a harmless third duplicate further down the fallback chain (unused here) is not this problem.
+  if (names[0] === names[1]) {
+    throw new Error(
+      `duplicate_ai_reviewer_provider: AI_PROVIDER lists "${names[0]}" for both dual-review slots -- configure two DISTINCT providers (e.g. AI_PROVIDER="claude-code,codex") or a single provider (dual review needs two).`,
+    );
+  }
   const rawCombine = (env.AI_COMBINE ?? "").trim().toLowerCase() as CombineStrategy;
   const combine: CombineStrategy = COMBINE_STRATEGIES.has(rawCombine) ? rawCombine : "synthesis";
   const rawOnMerge = (env.AI_ON_MERGE ?? "").trim().toLowerCase() as OnMerge;

src/services/ai-review.ts

@@ -25,6 +25,7 @@ import { sanitizePublicComment } from "../queue-intelligence";
 import { defangReviewInput } from "../review/safety";
 import { convergedFeatureActive } from "../review/feature-activation";
 import { labelSelfHostReviewerModels, labelSelfHostReviewerNames, resolveConfiguredProviderNames } from "../selfhost/ai-config";
+import { incr } from "../selfhost/metrics";
 import { errorMessage } from "../utils/json";
 import type { ReviewProfile } from "../signals/focus-manifest";
 
@@ -1228,6 +1229,10 @@ export async function runGittensoryAiReview(
       reviewDiagnostics.some((diagnostic) => diagnostic.status === "unparseable_output"))
   )
     inconclusive = true;
+  // #2540: observability for a fail-closed HOLD (no gate finding either way, but no usable AI output to certify
+  // clean) -- a correlated spike here alongside gittensory_ai_provider_circuit_total{result="tripped"} is the
+  // "dual-AI review is silently degrading" signal the alert rule watches for.
+  if (inconclusive) incr("gittensory_ai_review_inconclusive_total", { mode: input.mode, dual: String(dual) });
   const advisoryNotes =
     reviewsForNotes.length > 0
       ? (composeAdvisoryNotes(reviewsForNotes) ?? composeFallbackAdvisoryNotes(fallbackNotes))

test/unit/ai-review.test.ts

@@ -6,6 +6,7 @@ import {
   type GittensoryAiReviewInput,
 } from "../../src/services/ai-review";
 import { createTestEnv } from "../helpers/d1";
+import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
 
 const {
   parseModelReview,
@@ -87,6 +88,7 @@ const baseInput: GittensoryAiReviewInput = {
 
 afterEach(() => {
   vi.unstubAllGlobals();
+  resetMetrics();
 });
 
 describe("runGittensoryAiReview gating", () => {
@@ -474,6 +476,9 @@ describe("runGittensoryAiReview block mode (consensus)", () => {
     expect(result.consensusDefect).toBeNull();
     expect(result.inconclusive).toBe(true); // FAIL-CLOSED: a missing second opinion holds the PR, never passes it
     expect(result.advisoryNotes).not.toBeNull(); // notes still come from the one parseable opinion
+    // #2540: an inconclusive HOLD is observable, so a sustained spike (often correlated with a provider outage)
+    // is alertable independent of any one PR's outcome.
+    expect(await renderMetrics()).toContain('gittensory_ai_review_inconclusive_total{dual="true",mode="block"} 1');
   });
 
   it("a clean dual review is NOT inconclusive (both models parsed, neither blocks → passes)", async () => {
@@ -486,6 +491,8 @@ describe("runGittensoryAiReview block mode (consensus)", () => {
     });
     expect(result.status === "ok" && result.consensusDefect).toBeNull();
     expect(result.status === "ok" && result.inconclusive).toBe(false);
+    // #2540: no inconclusive metric on the happy path — the counter must stay silent, not just correct-valued.
+    expect(await renderMetrics()).not.toContain("gittensory_ai_review_inconclusive_total");
   });
 
   it("block mode with BYOK: provider writes the advisory, the free Workers-AI pair drives consensus", async () => {