Skip to content

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

Closed
JSONbored wants to merge 1 commit into
mainfrom
fix/selfhost-ai-provider-circuit-breaker
Closed

fix(selfhost): validate AI reviewer-provider configuration and add a failure circuit breaker#2604
JSONbored wants to merge 1 commit into
mainfrom
fix/selfhost-ai-provider-circuit-breaker

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Closes #2540.

Self-host's dual-AI review path resolves up to two configured reviewer "slots" from AI_PROVIDER with no deduplication or distinctness check. A config mistake (the same provider listed twice — a copy-paste artifact) silently collapses "two independent reviewers reaching consensus" into "the same provider called twice in parallel," defeating the point of dual-AI review and meaning a single provider's outage/auth failure takes down both reviewer slots at once.

Separately, there was no circuit breaker or failure-streak tracking per AI provider: every PR review independently retried a full attempt against a provider from a cold state, even seconds after that same provider failed the exact same way on the previous PR.

What changed

  • Duplicate-provider validation (src/selfhost/ai.ts, resolveAiReviewerPlan): throws a descriptive duplicate_ai_reviewer_provider error when the first two provider names (the ones that actually feed the two dual-review slots) are identical — a third duplicate further down an unused fallback chain is not this problem and is left alone. This mirrors assertNoLegacySharedAiEnv's existing fail-loud-at-boot pattern for other misconfigured env combinations, at the exact same unguarded boot call site (resolveAiReviewerPlan(process.env) in src/server.ts), so it crashes startup with a clear message the same way that pattern already does.
  • Per-provider circuit breaker (createChainAi): after 3 consecutive failures, a provider is skipped entirely (no network/CLI call) for a 5-minute cooldown, falling straight through to the next provider in the chain. In-process only (module-level Map), matching the existing global aiConsecutiveFailures streak pattern rather than introducing a new persistence layer. When every provider in the chain is circuit-open, the chain throws a distinct all_ai_providers_circuit_open error instead of the generic (and here misleading) no_ai_providers default.
  • Observability: gittensory_ai_provider_circuit_total{provider,result} (tripped/skipped/recovered) and gittensory_ai_review_inconclusive_total{mode,dual} (a fail-closed HELD verdict — not an error, but a spike is the "dual-AI review is silently degrading" signal), plus two new Prometheus alert rules (GittensoryAiProviderCircuitOpen, GittensoryAiReviewInconclusiveSpike) in a new gittensory-ai rule group, matching this repo's existing alert-rule conventions (severity/summary/description/runbook).

Correctness notes

  • The circuit breaker is keyed by provider name, not object identity — this is intentional (a cooldown that reset on every createChainAi(...) call would do nothing), but it did collide with one pre-existing test that reused the same provider name for a failing and a working provider within one test. Fixed that test to use a distinct name, since its actual intent (the global health streak resetting on any success) is provider-name-agnostic.
  • Mutation-tested all three new behaviors directly: disabling the dedup check, disabling the circuit-open skip, and removing the inconclusive metric each caused the corresponding new test(s) to fail as expected, then reverted.

Tests

  • test/unit/selfhost-ai.test.ts: duplicate-slot validation (throws, case/whitespace-insensitive, does NOT throw for a 3rd-position duplicate or genuinely distinct providers); circuit breaker (stays closed below the streak threshold, opens and skips after 3 failures, distinct all-open error, per-provider streak reset on success, cooldown expiry re-attempts the provider, metrics for skipped/tripped/recovered).
  • test/unit/ai-review.test.ts: the inconclusive-verdict metric fires on a fail-closed HOLD and stays silent on a clean pass.
  • test/unit/selfhost-grafana-dashboard.test.ts: both new alert rules are present with their exact expressions.

Validation

  • npm run typecheck
  • npx vitest run test/unit/selfhost-ai.test.ts test/unit/ai-review.test.ts test/unit/selfhost-grafana-dashboard.test.ts
  • npm run test:changed
  • npm run test:coverage (unsharded, full suite — 100% line and branch coverage on every changed line in src/** per coverage/lcov.info, cross-checked programmatically against the diff)
  • npm run db:migrations:check (no-op — no schema changes)
  • node -e "require('yaml').parse(...)" to confirm prometheus/rules/alerts.yml still parses after the new group
  • npm run test:ci (the full local gate, exit 0)
  • npm audit --audit-level=moderate
  • git diff --check
  • Manual mutation testing of all three new behaviors (see Correctness notes above)

Scope

  • Change is narrow and limited to the stated problem
  • No secrets, wallets, hotkeys, trust scores, or reward values added anywhere
  • No edits to site/, CNAME, **/lovable/**, or CHANGELOG.md

Safety

  • No auth/session/CORS surface touched
  • Byte-identical behavior for a correctly-configured single-provider or two-distinct-provider setup; the only behavior change for a MISCONFIGURED (duplicate) setup is failing loud at boot instead of silently degrading

…failure circuit breaker

Closes #2540.

Self-host's dual-AI review path resolved up to two reviewer slots from
AI_PROVIDER with no distinctness check, so a config mistake (the same
provider listed twice, e.g. a copy-paste artifact) silently collapsed "two
independent reviewers reaching consensus" into "the same provider called
twice" -- defeating dual-AI review and meaning one provider's outage/auth
failure took down both slots at once. resolveAiReviewerPlan now throws a
descriptive duplicate_ai_reviewer_provider error when the first two
(distinctness-relevant) names match, mirroring assertNoLegacySharedAiEnv's
existing fail-loud-at-boot pattern for other misconfigured env combinations.

createChainAi also gained a per-provider circuit breaker: after 3 consecutive
failures a provider is skipped (no network/CLI call at all) for a 5-minute
cooldown, falling straight through to the next provider in the chain instead
of repeating a doomed attempt on every single PR review during a sustained
outage. In-process only, matching the module's existing global failure-streak
pattern. Every provider circuit-open (no healthy fallback) throws a distinct
all_ai_providers_circuit_open error instead of the misleading generic
no_ai_providers default.

New metrics (gittensory_ai_provider_circuit_total{provider,result},
gittensory_ai_review_inconclusive_total{mode,dual}) and two Prometheus alert
rules give an operator visibility into a tripped provider and a spike in
fail-closed HELD verdicts, which is often the correlated downstream symptom
of the same outage.
@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 20:54:33 UTC

6 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · dirty

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The diff adds boot-time validation for duplicated dual-review providers and an in-process per-provider circuit breaker around the self-host AI provider chain, with tests covering threshold, skip, cooldown, recovery, and inconclusive-review metrics. The core fallback behavior is preserved: real provider errors are still retried until the breaker opens, successes reset provider-local state, and the all-open case now returns a distinct error. The notable weak spot is observability/query precision rather than runtime correctness.

Nits — 7 non-blocking
  • nit: `prometheus/rules/alerts.yml:GittensoryAiReviewInconclusiveSpike` uses `sum(rate(gittensory_ai_requests_total[15m]))` as the denominator without matching the new `mode`/`dual` labels, so the alert may be diluted by unrelated request volume and should either aggregate matching review dimensions or document why all AI requests are the intended base.
  • nit: `src/selfhost/ai.ts:createChainAi` increments `gittensory_ai_provider_circuit_total{result="tripped"}` on every failed half-open attempt after the threshold, which makes the metric count repeated trips rather than first-open transitions; rename/label it accordingly or only increment when `cooldownUntilMs` transitions from closed/expired to open.
  • nit: `prometheus/rules/alerts.yml:GittensoryAiProviderCircuitOpen` says the alert means the circuit stayed open for 15m, but the breaker cooldown is only 5m and the expression observes repeated `tripped` events, so align the annotation with the actual repeated-failure semantics.
  • In `prometheus/rules/alerts.yml`, make the inconclusive ratio denominator match the numerator’s operational scope, for example by filtering or grouping the request counter to the same review path if those labels exist.
  • In `src/selfhost/ai.ts:recordProviderCircuitFailure`, consider incrementing a separate `opened`/`reopened` result only on closed-to-open transitions and keeping failed half-open attempts as their own result for cleaner alerts.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2540
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:M; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 64 registered-repo PR(s), 55 merged, 526 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 64 PR(s), 526 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 64 PR(s), 526 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.03%. Comparing base (f8de661) to head (c665480).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2604   +/-   ##
=======================================
  Coverage   96.02%   96.03%           
=======================================
  Files         233      233           
  Lines       26077    26105   +28     
  Branches     9474     9482    +8     
=======================================
+ Hits        25041    25069   +28     
  Misses        425      425           
  Partials      611      611           
Files with missing lines Coverage Δ
src/selfhost/ai.ts 98.56% <100.00%> (+0.12%) ⬆️
src/services/ai-review.ts 95.17% <100.00%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored

Copy link
Copy Markdown
Owner Author

Closing as a duplicate — #2540 was already resolved by #2626 (merged), implemented independently in a parallel session. Confirmed via diff comparison: both PRs touch the exact same 5 files (prometheus/rules/alerts.yml, src/selfhost/ai.ts, src/services/ai-review.ts, test/unit/ai-review.test.ts, test/unit/selfhost-ai.test.ts) implementing the same duplicate-provider validation + per-provider circuit breaker + Prometheus alerting. #2626's circuit breaker is arguably more complete — it patches the shared runProviderWithOtel chokepoint used by both the fallback chain and the dual-review named-slot path, where this PR only patched the fallback chain in createChainAi. No unique work here worth preserving; #2626 is the PR of record for #2540.

@JSONbored JSONbored closed this Jul 2, 2026
@JSONbored
JSONbored deleted the fix/selfhost-ai-provider-circuit-breaker branch July 4, 2026 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

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

1 participant