Merge branch 'main' into feat/miner-ci-poller-2323

Author: Helios531

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/github/backfill.ts

@@ -3216,6 +3216,12 @@ async function syncLabels(
   }
 }
 
+/**
+ * `limit` is a page-boundary threshold, not a strict maximum: page consumption is atomic (see the loop body),
+ * so the crawl always finishes the page it's on once `items.length` reaches `limit`, which can return up to
+ * `perPage - 1` more items than requested. Callers that need an exact cap must trim the returned `items`
+ * themselves; `fetchedCount`/`items.length` always reflect the true (possibly over-`limit`) count.
+ */
 async function githubPaged<T>(
   env: Env,
   repo: RepositoryRecord,
@@ -3258,18 +3264,17 @@ async function githubPaged<T>(
       lastModified = result.lastModified ?? lastModified;
       lastCursor = String(page);
       pageCount += 1;
-      const remaining = limit - items.length;
-      // A page can only overrun `remaining` once we have already consumed at least one full page (so `page`
-      // has advanced past the crawl's start): resume from THIS page to pick up its unconsumed tail. On the
-      // first page `remaining >= perPage >= result.data.length`, so this is false and the cursor advances,
-      // which is why a small-`limit` (per_page = limit) crawl can never stall on its own start page.
-      const truncatedPage = result.data.length > remaining;
-      items.push(...result.data.slice(0, remaining));
+      // Resume cursors only have page precision, so keep page consumption atomic: once we request a
+      // page, process the whole response before advancing the cursor. Slicing a mid-page cap would make a
+      // later resume replay the already-consumed prefix of that same page and inflate fetched counts.
+      items.push(...result.data);
       const hasNext = hasNextPage(result.link);
-      if (items.length >= limit && (hasNext || truncatedPage)) {
-        nextCursor = String(truncatedPage ? page : page + 1);
+      if (items.length >= limit && hasNext) {
+        nextCursor = String(page + 1);
         status = "capped";
-        warnings.push(`GitHub sync reached local cap of ${limit} item(s) for ${path}; next page cursor is ${nextCursor}.`);
+        // `items.length` (not `limit`) is the actual count: page consumption is atomic, so a whole final page
+        // can overrun the requested `limit` — `limit` is a page-boundary threshold, not a strict maximum.
+        warnings.push(`GitHub sync reached local cap of ${limit} item(s) for ${path} (fetched ${items.length} after completing page ${page}); next page cursor is ${nextCursor}.`);
         break;
       }
       if (result.data.length < perPage || !hasNext) break;

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/selfhost/pg-queue.ts

@@ -253,6 +253,27 @@ export function createPgQueue(
     return revived;
   }
 
+  /** Wraps reviveDeadLetterJobs() for the setInterval callback below, which has no rejection handler of its
+   *  own -- a transient pool/driver/metric failure here would otherwise surface as an unhandled promise
+   *  rejection and can terminate the process (fatal when SENTRY_DSN is unset, since server.ts only installs
+   *  the handler when Sentry is configured), exactly the failure mode pump()'s own try/catch above guards
+   *  against for the main poll loop. A failed revive tick just waits for the next interval, same as a failed
+   *  poll tick waits for the next poll. */
+  async function reviveDeadLetterJobsSafely(): Promise<void> {
+    try {
+      await reviveDeadLetterJobs();
+    } catch (error) {
+      console.error(
+        JSON.stringify({
+          level: "error",
+          event: "selfhost_queue_dead_letter_revive_crashed",
+          error: errorMessageWithCause(error),
+        }),
+      );
+      captureError(error, { kind: "queue_dead_letter_revive_crashed" });
+    }
+  }
+
   async function spreadDueJobsOnStartup(): Promise<number> {
     const now = Date.now();
     const res = await pool.query(
@@ -666,7 +687,7 @@ export function createPgQueue(
       // Separate, much slower interval than the poll tick above -- reviving a dead job every second would
       // recreate the retry storm this feature exists to bound. The interval itself is the cooldown between
       // auto-retry rounds for any one job.
-      deadLetterReviveTimer = setInterval(() => void reviveDeadLetterJobs(), queueDeadLetterReviveIntervalMs());
+      deadLetterReviveTimer = setInterval(() => void reviveDeadLetterJobsSafely(), queueDeadLetterReviveIntervalMs());
     },
     async stop() {
       running = false;

src/selfhost/sqlite-queue.ts

@@ -187,6 +187,26 @@ export function createSqliteQueue(
     return revived;
   }
 
+  /** Wraps reviveDeadLetterJobs() for the setInterval callback below, which has no error handler of its own --
+   *  a transient driver/metric failure here would otherwise surface as an uncaught exception and can terminate
+   *  the process (fatal when SENTRY_DSN is unset, since server.ts only installs the handler when Sentry is
+   *  configured), exactly the failure mode pump()'s own try/catch above guards against for the main poll loop.
+   *  A failed revive tick just waits for the next interval, same as a failed poll tick waits for the next poll. */
+  function reviveDeadLetterJobsSafely(): void {
+    try {
+      reviveDeadLetterJobs();
+    } catch (error) {
+      console.error(
+        JSON.stringify({
+          level: "error",
+          event: "selfhost_queue_dead_letter_revive_crashed",
+          error: errorMessageWithCause(error),
+        }),
+      );
+      captureError(error, { kind: "queue_dead_letter_revive_crashed" });
+    }
+  }
+
   function enqueue(message: JobMessage, delaySeconds: number): void {
     const now = Date.now();
     const payload = JSON.stringify(message);
@@ -576,7 +596,7 @@ export function createSqliteQueue(
       // Separate, much slower interval than the poll tick above -- reviving a dead job every second would
       // recreate the retry storm this feature exists to bound. The interval itself is the cooldown between
       // auto-retry rounds for any one job.
-      deadLetterReviveTimer = setInterval(reviveDeadLetterJobs, queueDeadLetterReviveIntervalMs());
+      deadLetterReviveTimer = setInterval(reviveDeadLetterJobsSafely, queueDeadLetterReviveIntervalMs());
     },
     async stop() {
       running = false;

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 () => {