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

prometheus/rules/alerts.yml

@@ -369,3 +369,36 @@ 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 reliability (dual-AI combiner + per-provider circuit breaker, #2540) ─
+  - name: gittensory-ai-review
+    rules:
+      - alert: GittensoryAiReviewInconclusiveSpike
+        # `inconclusive` means the AI review pipeline could not produce a usable verdict (every reviewer
+        # opinion missing/unparseable, or a required opinion never came back) -- the review still runs
+        # deterministically, but dual-AI review is repeatedly failing to add value. Absolute-increase
+        # threshold (matching GittensoryDeadLetterJobsGrowing above): there's no clean matching-cardinality
+        # denominator (total review attempts aren't broken out per-mode the same way), so a ratio query
+        # would need an unrelated series. > 5 in 30m tolerates the occasional one-off degrade.
+        expr: increase(gittensory_ai_review_inconclusive_total[30m]) > 5
+        for: 10m
+        labels:
+          severity: warning
+        annotations:
+          summary: "gittensory AI review is repeatedly inconclusive"
+          description: "{{ $value | printf \"%.0f\" }} AI review(s) came back inconclusive over the last 30m (sustained 10m). Dual-AI review is repeatedly failing to produce a usable verdict."
+          runbook: "Check provider health and circuit-breaker state (gittensory_ai_provider_failures_total / gittensory_ai_provider_circuit_open_total) and verify AI_PROVIDER credentials are still valid for every configured reviewer."
+
+      - alert: GittensoryAiProviderCircuitOpen
+        # A provider's circuit breaker opens after AI_PROVIDER_FAILURE_THRESHOLD consecutive failures and
+        # short-circuits further attempts for a cooldown window -- ANY circuit-open event in 15m means that
+        # provider has been failing repeatedly and calls are being skipped fast rather than retried at full
+        # cost. Same absolute-increase style as GittensoryDeadLetterJobsGrowing: any occurrence is worth a look.
+        expr: increase(gittensory_ai_provider_circuit_open_total[15m]) > 0
+        for: 5m
+        labels:
+          severity: warning
+        annotations:
+          summary: "gittensory AI provider {{ $labels.provider }} circuit breaker is open"
+          description: "Provider {{ $labels.provider }} has failed repeatedly and its circuit breaker is skipping calls fast during its cooldown (sustained 5m)."
+          runbook: "Check that provider's credentials/reachability (CLI auth for claude-code/codex, or the configured API key/base URL for HTTP providers) via gittensory_ai_provider_failures_total{provider=\"...\"} and recent selfhost_ai_provider_failed logs."

src/selfhost/ai.ts

@@ -604,6 +604,20 @@ export function resetAiProviderHealthForTest(): void {
   aiConsecutiveFailures = 0;
 }
 
+// Per-provider circuit breaker (#2540): a provider that is failing hard (bad credential, sustained outage)
+// otherwise pays the FULL cost of a fresh attempt (a real HTTP call, or a real CLI subprocess spawn) on
+// every single review during the outage. This is independent of `aiConsecutiveFailures` above -- that streak
+// tracks whole-CHAIN exhaustion for /ready; this tracks one PROVIDER's own reliability so a known-broken
+// provider can be skipped fast without affecting readiness semantics.
+const AI_PROVIDER_FAILURE_THRESHOLD = 3;
+const AI_PROVIDER_COOLDOWN_MS = 60_000;
+const aiProviderCircuits = new Map<string, { failures: number; cooldownUntil: number }>();
+
+/** Test-only reset so circuit state from one test can't leak into the next (module-level map). */
+export function resetAiProviderCircuitBreakerForTest(): void {
+  aiProviderCircuits.clear();
+}
+
 /** Whether a missing-CLI boot check should force /ready unhealthy: only when EVERY configured provider is
  *  among the missing-CLI set, i.e. the whole AI_PROVIDER chain has zero chance of working -- not just one
  *  provider within a chain that has a working fallback (another present CLI, or an HTTP-based provider,
@@ -674,16 +688,39 @@ function requestKind(options: AiRunOptions): "embedding" | "review" {
   return Array.isArray(options.text) ? "embedding" : "review";
 }
 
-function runProviderWithOtel(
+async function runProviderWithOtel(
   provider: { name: string; ai: SelfHostAi },
   model: string,
   options: AiRunOptions,
 ): Promise<AiResult> {
-  return withReviewSpan(
-    "selfhost.ai.provider",
-    { "ai.provider": provider.name, "ai.model": model || "default", "ai.request_kind": requestKind(options) },
-    () => provider.ai.run(model, options),
-  );
+  const circuit = aiProviderCircuits.get(provider.name);
+  if (circuit && circuit.cooldownUntil > Date.now()) {
+    incr("gittensory_ai_provider_circuit_open_total", { provider: provider.name });
+    throw new Error(
+      `circuit_open: provider "${provider.name}" is in cooldown after ${AI_PROVIDER_FAILURE_THRESHOLD} consecutive failures — skipping this attempt`,
+    );
+  }
+  try {
+    const result = await withReviewSpan(
+      "selfhost.ai.provider",
+      { "ai.provider": provider.name, "ai.model": model || "default", "ai.request_kind": requestKind(options) },
+      () => provider.ai.run(model, options),
+    );
+    aiProviderCircuits.delete(provider.name);
+    return result;
+  } catch (error) {
+    incr("gittensory_ai_provider_failures_total", { provider: provider.name });
+    // Re-read the map here rather than reusing the `circuit` captured above: that read happened BEFORE the
+    // `await` on the real provider call, so under concurrent same-provider calls it can be stale by the time
+    // this catch runs, and computing `failures` from it would clobber a sibling call's write (lost-update race)
+    // instead of accumulating. No `await` between this read and the `.set()` below, so it's race-free.
+    const failures = (aiProviderCircuits.get(provider.name)?.failures ?? 0) + 1;
+    aiProviderCircuits.set(provider.name, {
+      failures,
+      cooldownUntil: failures >= AI_PROVIDER_FAILURE_THRESHOLD ? Date.now() + AI_PROVIDER_COOLDOWN_MS : 0,
+    });
+    throw error;
+  }
 }
 
 /** Build one provider adapter by name. Provider config stays explicit so dual-provider setups cannot accidentally
@@ -798,6 +835,16 @@ 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 };
+  // Fail loud when the two SLOTS the dual-review plan actually uses (the first two names) are the same
+  // provider: routeProviders' `byName` map collapses duplicate provider names to one runtime instance, so
+  // "dual review" would silently become "one provider called twice" -- no independent second opinion, and
+  // that provider's outage takes down both slots. A THIRD+ duplicate further down the list is fine; only
+  // the first two matter because resolveAiReviewerPlan below caps reviewers at names.slice(0, 2).
+  if (names[0] === names[1]) {
+    throw new Error(
+      `ai_reviewer_providers_not_distinct: AI_PROVIDER lists "${names[0]}" for both dual-review reviewer slots — configure two distinct providers (e.g. AI_PROVIDER=claude-code,codex) for independent dual review, or a single provider (AI_PROVIDER=codex) for single-reviewer mode.`,
+    );
+  }
   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,11 @@ export async function runGittensoryAiReview(
       reviewDiagnostics.some((diagnostic) => diagnostic.status === "unparseable_output"))
   )
     inconclusive = true;
+  // Observability (#2540): the single canonical point where `inconclusive` reaches its final value for this
+  // review call -- increment exactly once here, never at the downstream consumers in queue/processors.ts that
+  // push an `ai_review_inconclusive` advisory finding off this same already-computed result (incrementing there
+  // too would double/triple-count one review).
+  if (inconclusive) incr("gittensory_ai_review_inconclusive_total", { mode: input.mode });
   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,8 @@ 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
+    // Observability (#2540): the single canonical increment fires once for this inconclusive review.
+    expect(await renderMetrics()).toContain('gittensory_ai_review_inconclusive_total{mode="block"} 1');
   });
 
   it("a clean dual review is NOT inconclusive (both models parsed, neither blocks → passes)", async () => {
@@ -486,6 +490,8 @@ describe("runGittensoryAiReview block mode (consensus)", () => {
     });
     expect(result.status === "ok" && result.consensusDefect).toBeNull();
     expect(result.status === "ok" && result.inconclusive).toBe(false);
+    // A non-inconclusive review must NOT increment the inconclusive counter.
+    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 () => {