Add backup AI provider / failover (off by default)#78
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…coverage Soften circuit-breaker wording (it counts per-call, not per-review), add PRIMARY_AI_TIMEOUT_MS guidance, drop an unused test import, and cover the openai-compatible backup validation branch in config-backup.test.ts. No runtime behavior changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughAdded an opt-in backup AI provider failover so transient primary failures fall over to a configured secondary model instead of dead-lettering the review. Extracted shared provider construction, wired backup config with circuit-breaker knobs, and covered the behavior with unit tests while keeping the feature off by default. Changes
Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant FailoverProvider
participant Primary
participant Backup
Reviewer->>FailoverProvider: review(ctx)
FailoverProvider->>Primary: review(ctx) with short timeout
Primary-->>FailoverProvider: transient error
FailoverProvider->>Backup: review(ctx) with full timeout
Backup-->>FailoverProvider: ReviewResult
FailoverProvider-->>Reviewer: ReviewResult servedBy backup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested Labels
Risk AssessmentScore: 25/100 — 🟡 Moderate
Test Coverage Signal🟢 153 prod / 318 test lines added.
🧭 Description Drift
Compares PR description claims to the actual diff. Update the description (or the code) so they tell the same story. ✍️ Commit Message Coach1 of 10 commit messages could be stronger.
Tip: imperative mood ( 📊 Confidence breakdown10 findings — 🟢 5 high · 🟡 5 medium · 🔴 0 low. Treat the medium / low ones as hypotheses to verify. Linked Issues
🚥 Pre-merge checks | ✅ 5 | ❌ 0✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in your ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Generate docstrings (beta)
🧹 Simplify (beta)
🪄 Autofix unresolved comments (beta)
Comment |
![diffsentry[bot]](https://avatars.githubusercontent.com/in/3360270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Solid, well-scoped failover design: transient-only trigger, sequential primary→backup, short primary deadline only when a backup exists, and thorough unit coverage for the wrapper and config. One real correctness gap remains in the circuit-breaker half-open path—a failed probe does not re-arm the open window—so REQUEST_CHANGES until that is fixed. A few maintainability/test nits around non-positive PRIMARY_AI_TIMEOUT_MS and breaker half-open re-open coverage should also be cleaned up.
🔎 Issues not tied to a specific line (1)
These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.
Description claims .env.example was updated, but it is not in the diff
🤔 Medium confidence — verify against intent before acting.
The Changes section lists .env.example / README.md as operator docs, and the new README backup section tells operators to “See .env.example for the full list of BACKUP_ANTHROPIC_* / BACKUP_OPENAI_* / BACKUP_LOCAL_AI_* override vars.” No .env.example file appears among the changed files, so those override variables are not actually documented where the description and README claim.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. Description claims `.env.example` was updated, but it is not in the diff The Changes section lists `.env.example` / `README.md` as operator docs, and the new README backup section tells operators to “See `.env.example` for the full list of `BACKUP_ANTHROPIC_*` / `BACKUP_OPENAI_*` / `BACKUP_LOCAL_AI_*` override vars.” No `.env.example` file appears among the changed files, so those override variables are not actually documented where the description and README claim.
🧹 Nitpick comments (3)
src/config.ts (1)
Line 140: PRIMARY_AI_TIMEOUT_MS accepts non-positive values.
⚠️ Potential issue | 🟡 MinorPRIMARY_AI_TIMEOUT_MS accepts non-positive values.
primaryAiTimeoutMsonly clamps the upper bound toaiRequestTimeoutMs. A mis-setPRIMARY_AI_TIMEOUT_MS=0or negative value is accepted and becomes the primary provider deadline when a backup is configured, which effectively forces every primary call to time out immediately and fail over.Mirror the breaker-threshold style guard and fall back to
20_000(then clamp) for non-positive parses so a bad env value cannot silently disable the primary.🔧 Proposed fix
const parsedPrimaryTimeout = parseInt(process.env.PRIMARY_AI_TIMEOUT_MS || "", 10); let primaryAiTimeoutMs = Number.isFinite(parsedPrimaryTimeout) && parsedPrimaryTimeout > 0 ? parsedPrimaryTimeout : 20_000; if (aiRequestTimeoutMs > 0 && primaryAiTimeoutMs > aiRequestTimeoutMs) { primaryAiTimeoutMs = aiRequestTimeoutMs; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/config.ts around line 140, reject non-positive PRIMARY_AI_TIMEOUT_MS values. Require Number.isFinite(parsedPrimaryTimeout) && parsedPrimaryTimeout > 0 before using the parsed value; otherwise default to 20_000, then keep the existing clamp against aiRequestTimeoutMs. Add a config-backup unit test covering PRIMARY_AI_TIMEOUT_MS=0 or a negative value falling back to 20000.♻️ Suggested fix
const parsedPrimaryTimeout = parseInt(process.env.PRIMARY_AI_TIMEOUT_MS || "", 10); let primaryAiTimeoutMs = Number.isFinite(parsedPrimaryTimeout) && parsedPrimaryTimeout > 0 ? parsedPrimaryTimeout : 20_000; if (aiRequestTimeoutMs > 0 && primaryAiTimeoutMs > aiRequestTimeoutMs) { primaryAiTimeoutMs = aiRequestTimeoutMs; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/config.ts around line 140, reject non-positive PRIMARY_AI_TIMEOUT_MS values. Require Number.isFinite(parsedPrimaryTimeout) && parsedPrimaryTimeout > 0 before using the parsed value; otherwise default to 20_000, then keep the existing clamp against aiRequestTimeoutMs. Add a config-backup unit test covering PRIMARY_AI_TIMEOUT_MS=0 or a negative value falling back to 20000.tests/unit/failover.test.ts (1)
Line 110: Half-open re-open path is untested.🛠️ Refactor suggestion | 🟡 Minor
Half-open re-open path is untested.
The suite covers open-after-threshold and half-open success, but never asserts the half-open failure path. Given the current
recordPrimaryFailurebug, a test that advances the clock past cooldown, fails the primary probe, and expects the next call to skip primary would have caught the re-open gap immediately.Add that case next to the existing half-open success test so the breaker contract stays locked down.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/failover.test.ts around the half-open success test near line 110, add a sibling case: trip the breaker with three transient primary failures, advance the injectable clock past cooldown, make the next primary call fail transiently, then assert that one more call still skips primary (call count unchanged) until another full cooldown elapses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/failover.test.ts around the half-open success test near line 110, add a sibling case: trip the breaker with three transient primary failures, advance the injectable clock past cooldown, make the next primary call fail transiently, then assert that one more call still skips primary (call count unchanged) until another full cooldown elapses.src/ai/provider-factory.ts (1)
Line 28: Factory relies on non-null assertions for required credentials.🛠️ Refactor suggestion | 🟢 Trivial
Factory relies on non-null assertions for required credentials.
🤔 Medium confidence — verify against intent before acting.
buildProviderusesspec.anthropicApiKey!,spec.localAiBaseUrl!, andspec.openaiApiKey!. Boot-time config validation currently guarantees those for the configured provider, but the factory is now a shared construction path for both primary and backup. A future caller that builds a partialProviderSpecwill get a late runtime failure inside the provider constructor instead of a clear factory error.Prefer an explicit throw with the missing field named, matching the fail-fast style in
loadConfig.🔧 Proposed fix
export function buildProvider(spec: ProviderSpec): AIProvider { 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) { throw new Error("buildProvider(openai-compatible) requires localAiBaseUrl"); } 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); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts around line 28, replace the non-null assertions on anthropicApiKey, localAiBaseUrl, and openaiApiKey with explicit missing-field checks that throw descriptive Error messages before constructing each provider.♻️ Suggested fix
export function buildProvider(spec: ProviderSpec): AIProvider { 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) { throw new Error("buildProvider(openai-compatible) requires localAiBaseUrl"); } 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); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts around line 28, replace the non-null assertions on anthropicApiKey, localAiBaseUrl, and openaiApiKey with explicit missing-field checks that throw descriptive Error messages before constructing each provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/ai/failover.ts`:
- Line 75: In src/ai/failover.ts around line 75, fix recordPrimaryFailure so a failed half-open probe re-opens the breaker. When consecutiveFailures >= threshold, always set openedAt = this.now() even if openedAt was already non-null; optionally log a distinct warn for re-open vs first open. Then add a unit test in tests/unit/failover.test.ts that opens the breaker, advances the clock past cooldown, fails the primary probe, and asserts the next call still skips the primary until another full cooldown elapses.
In `src/config.ts`:
- Line 140: In src/config.ts around line 140, reject non-positive PRIMARY_AI_TIMEOUT_MS values. Require Number.isFinite(parsedPrimaryTimeout) && parsedPrimaryTimeout > 0 before using the parsed value; otherwise default to 20_000, then keep the existing clamp against aiRequestTimeoutMs. Add a config-backup unit test covering PRIMARY_AI_TIMEOUT_MS=0 or a negative value falling back to 20000.
In `tests/unit/failover.test.ts`:
- Line 110: In tests/unit/failover.test.ts around the half-open success test near line 110, add a sibling case: trip the breaker with three transient primary failures, advance the injectable clock past cooldown, make the next primary call fail transiently, then assert that one more call still skips primary (call count unchanged) until another full cooldown elapses.
In `src/ai/provider-factory.ts`:
- Line 28: In src/ai/provider-factory.ts around line 28, replace the non-null assertions on anthropicApiKey, localAiBaseUrl, and openaiApiKey with explicit missing-field checks that throw descriptive Error messages before constructing each provider.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: e5679bd6-183a-47fc-97da-d2736d3992a5
📥 Commits
Reviewing files at c1da271 (base SHA unavailable).
⛔ Files ignored due to path filters (1)
.env.exampleis excluded by!.env.example
📒 Files selected for processing (13)
README.mddocs/superpowers/plans/2026-07-09-backup-ai-provider-failover.mddocs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.mdsrc/ai/failover.tssrc/ai/provider-factory.tssrc/ai/transient.tssrc/config.tssrc/realtime/jobs.tssrc/reviewer.tssrc/types.tstests/unit/config-backup.test.tstests/unit/failover.test.tstests/unit/transient.test.ts
✂️ Files with diffs truncated to fit the review size budget (1)
docs/superpowers/plans/2026-07-09-backup-ai-provider-failover.md
📌 Status — last updated
|
| Risk score | 25/100 (Moderate) █▁▆ |
| Unresolved threads | 5 |
| Failing checks | 0 |
| Pending checks | 1 |
| Files reviewed | 4 (9 skipped) |
| Updated | 2026-07-09 22:51Z |
Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.
- Guard PRIMARY_AI_TIMEOUT_MS against non-positive values (would disable the primary deadline and defeat failover); fall back to 20000. (Minor) - buildProvider: throw named errors for missing required credentials instead of non-null assertions, since it's now a shared primary/backup construction path. (Trivial) - Add a half-open re-open coverage test for the circuit breaker. (Minor) Refuted: the half-open re-open 'Major' finding — recordPrimaryFailure already re-arms openedAt unconditionally; wasOpen gates only the log line. New test proves it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Solid, well-scoped opt-in failover design: transient-only sequential failover, short primary deadline, circuit breaker with injectable clock, reuse-with-override config, and strong unit coverage (328/328). The factory extraction and shared isTransientError keep the no-backup path clean. A few wiring/config edge cases and the acknowledged call-vs-review breaker limitation deserve attention before merge.
🔎 Issues not tied to a specific line (5)
These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.
Diff is incomplete relative to the PR description — core failover wiring is missing from the reviewable patch.
The description and DiffSentry summary claim changes to src/ai/failover.ts, src/ai/transient.ts, src/realtime/jobs.ts, src/reviewer.ts, src/types.ts, .env.example, README.md, and the design/plan docs, plus tests/unit/transient.test.ts. Only provider-factory.ts, the config parse block, and two test files appear in the provided diff.
Without FailoverProvider, isTransientError, the reviewer.ts wrap (short primary timeout, footnote, servedBy), and the Config/ReviewResult type extensions, this cannot be fully verified as matching the design (transient-only, sequential, breaker open/half-open, footnote). Please ensure those files are included in the PR/diff under review before treating the feature as merge-ready.
What is present (factory, config parse, failover/config unit tests) looks coherent with the stated design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Ensure the PR includes all files claimed in the description: src/ai/failover.ts, src/ai/transient.ts, src/realtime/jobs.ts (re-export), src/reviewer.ts (FailoverProvider wrap + footnote), src/types.ts (Config backup fields + ReviewResult.servedBy), .env.example, README.md, docs/superpowers specs/plans, and tests/unit/transient.test.ts. Re-open or amend the PR so the full wiring is reviewable.
📝 Documentation | 🟢 Trivial
Acknowledge call-level breaker vs review-call failure mode in operator docs (non-blocking).
The PR already calls out that the breaker counts primary calls, so a succeeding walkthrough can reset the counter while the large review call keeps timing out (the #77 pattern). Failover still happens every time; only the cost-saving skip is best-effort.
Keep that caveat visible in README / design doc (as noted in non-blocking follow-ups). A future per-operation breaker would be the right fix if this shows up in production metrics; no code change required for this PR if docs already say so.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In README.md and/or docs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.md, confirm there is an explicit note that the circuit breaker is per-call (not per-review) and that a succeeding short call can reset consecutive failures while the large review call still fails over every time. Add that sentence if missing.
🛠️ Refactor suggestion | 🟢 Trivial
Consider a boot-time warning when backup resolves identical to primary.
🤔 Medium confidence — verify against intent before acting.
Listed as a non-blocking follow-up: if BACKUP_AI_PROVIDER reuses the same provider type + endpoint + model + key as the primary, failover cannot help and will only double latency on transient errors. A single console.warn / logger warning at config load (or when building the FailoverProvider) would catch the common misconfig of setting BACKUP_AI_PROVIDER equal to AI_PROVIDER without overrides.
Optional for this PR; worth filing if not done here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Optionally in src/config.ts after resolving backup fields (or in src/reviewer.ts when constructing FailoverProvider), detect when the resolved backup provider type, model, base URL, and key match the primary and emit a one-line warning that failover will not diversify the backend. Do not fail boot.
.env.example claimed as updated but not present in the PR changes
🤔 Medium confidence — verify against intent before acting.
The Changes section says .env.example / README.md carry the operator docs, but the actual changed-files set includes README.md (plus the design/plan docs) and has no .env.example modification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. .env.example claimed as updated but not present in the PR changes The Changes section says `.env.example` / `README.md` carry the operator docs, but the actual changed-files set includes `README.md` (plus the design/plan docs) and has no `.env.example` modification.
Claimed test totals contradict the size of the new suites
🤔 Medium confidence — verify against intent before acting.
The description reports npm test 328/328 and an “unchanged 326-test baseline,” which implies only ~2 new tests, yet it also lists new suites tests/unit/transient.test.ts, tests/unit/failover.test.ts (many cases), and tests/unit/config-backup.test.ts (many cases) whose combined count far exceeds two tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. Claimed test totals contradict the size of the new suites The description reports `npm test` 328/328 and an “unchanged 326-test baseline,” which implies only ~2 new tests, yet it also lists new suites `tests/unit/transient.test.ts`, `tests/unit/failover.test.ts` (many cases), and `tests/unit/config-backup.test.ts` (many cases) whose combined count far exceeds two tests.
🧹 Nitpick comments (5)
src/config.ts (1)
Line 145: PRIMARY_AI_TIMEOUT_MS and breaker knobs are always parsed even when backup is off.🛠️ Refactor suggestion | 🟡 Minor
PRIMARY_AI_TIMEOUT_MS and breaker knobs are always parsed even when backup is off.
primaryAiTimeoutMs,backupCircuitThreshold, andbackupCircuitCooldownMsare computed and written ontoConfigregardless of whetherBACKUP_AI_PROVIDERis set. When backup is off these knobs are dead weight, and a mis-setPRIMARY_AI_TIMEOUT_MScan still clamp/override expectations for operators who never intended failover.Gate the entire primary-timeout + breaker block on
if (backupRaw)(orif (backupAiProvider)) so the fields stay at their type-level optionality and only resolve when failover is actually enabled. That matches the PR's "off by default / byte-for-byte unchanged" claim more tightly.🔧 Proposed fix
// Short primary deadline + breaker knobs only matter when a backup is configured. let primaryAiTimeoutMs = 20_000; let backupCircuitThreshold = 3; let backupCircuitCooldownMs = 60_000; if (backupAiProvider) { 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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/config.ts around the PRIMARY_AI_TIMEOUT_MS / BACKUP_CIRCUIT_* parsing block, gate computation of primaryAiTimeoutMs, backupCircuitThreshold, and backupCircuitCooldownMs so they only resolve when backupAiProvider is set. Keep sensible defaults for the Config fields when backup is off, or leave them undefined if types allow; do not let PRIMARY_AI_TIMEOUT_MS affect anything on the no-backup path.♻️ Suggested fix
// Short primary deadline + breaker knobs only matter when a backup is configured. let primaryAiTimeoutMs = 20_000; let backupCircuitThreshold = 3; let backupCircuitCooldownMs = 60_000; if (backupAiProvider) { 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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/config.ts around the PRIMARY_AI_TIMEOUT_MS / BACKUP_CIRCUIT_* parsing block, gate computation of primaryAiTimeoutMs, backupCircuitThreshold, and backupCircuitCooldownMs so they only resolve when backupAiProvider is set. Keep sensible defaults for the Config fields when backup is off, or leave them undefined if types allow; do not let PRIMARY_AI_TIMEOUT_MS affect anything on the no-backup path.src/ai/provider-factory.ts (2)
Line 37: openai-compatible branch does not require a model string at the factory.
⚠️ Potential issue | 🟡 Minoropenai-compatible branch does not require a model string at the factory.
buildProvidervalidateslocalAiBaseUrlforopenai-compatiblebut notlocalAiModel. Config already fails boot when both base URL and model are missing for a backup, and primary loadConfig similarly expects a model, but this factory is documented as the single construction path that should fail fast on a partial spec.A future caller that builds a primary/backup with an empty model will only fail deep inside the OpenAI-compatible SDK/request. Mirror the base-URL guard and reject an empty/missing
localAiModelhere.🔧 Proposed fix
if (spec.provider === "openai-compatible") { if (!spec.localAiBaseUrl) { throw new Error("buildProvider(openai-compatible) requires localAiBaseUrl"); } if (!spec.localAiModel) { throw new Error("buildProvider(openai-compatible) requires localAiModel"); } return new OpenAICompatibleProvider({ baseURL: spec.localAiBaseUrl, model: spec.localAiModel, apiKey: spec.localAiApiKey, jsonMode: spec.localAiJsonMode, timeoutMs: spec.timeoutMs, providerLabel: spec.label, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts inside the openai-compatible branch of buildProvider, add a guard that throws when spec.localAiModel is missing or empty, matching the existing localAiBaseUrl check and the config-level validation for BACKUP_AI_PROVIDER=openai-compatible.♻️ Suggested fix
if (spec.provider === "openai-compatible") { if (!spec.localAiBaseUrl) { throw new Error("buildProvider(openai-compatible) requires localAiBaseUrl"); } if (!spec.localAiModel) { throw new Error("buildProvider(openai-compatible) requires localAiModel"); } return new OpenAICompatibleProvider({ baseURL: spec.localAiBaseUrl, model: spec.localAiModel, apiKey: spec.localAiApiKey, jsonMode: spec.localAiJsonMode, timeoutMs: spec.timeoutMs, providerLabel: spec.label, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts inside the openai-compatible branch of buildProvider, add a guard that throws when spec.localAiModel is missing or empty, matching the existing localAiBaseUrl check and the config-level validation for BACKUP_AI_PROVIDER=openai-compatible.
Line 23: label override is only applied for openai-compatible providers.🛠️ Refactor suggestion | 🟡 Minor
label override is only applied for openai-compatible providers.
🤔 Medium confidence — verify against intent before acting.
ProviderSpec.labelis documented as overriding attribution "when a same-type backup would otherwise collide with the primary", but it is only forwarded asproviderLabelintoOpenAICompatibleProvider. Anthropic and OpenAI constructors take no label, so a same-type anthropic→anthropic or openai→openai backup will still share cost/log identity with the primary.Either extend the anthropic/openai providers (or their usage-recording path) to accept an optional label, or document that same-type attribution disambiguation is openai-compatible-only so operators don't expect
recordAiUsageto split costs for those pairs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts and the AnthropicProvider/OpenAIProvider constructors (or their usage-attribution helpers), either plumb ProviderSpec.label through for anthropic/openai the same way openai-compatible gets providerLabel, or update the ProviderSpec.label docs and README to state that same-type cost/log disambiguation currently only works for openai-compatible backups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts and the AnthropicProvider/OpenAIProvider constructors (or their usage-attribution helpers), either plumb ProviderSpec.label through for anthropic/openai the same way openai-compatible gets providerLabel, or update the ProviderSpec.label docs and README to state that same-type cost/log disambiguation currently only works for openai-compatible backups.tests/unit/failover.test.ts (1)
Line 78: Breaker open-path test does not assert servedBy on direct-to-backup calls.🛠️ Refactor suggestion | 🟢 Trivial
Breaker open-path test does not assert servedBy on direct-to-backup calls.
When the breaker is open the 4th
reviewskips primary entirely and should still tagservedBy: "backup"so the review footnote and any metrics stay accurate. The test only checks call counts.Add an assertion on the open-path result's
servedBy(and ideally one after the half-open re-open path) so a regression that drops the tag on the skip-primary branch is caught.🔧 Proposed fix
// 4th call: breaker open → primary skipped entirely. const openRes = await p.review(ctx()); expect(primaryReview).toHaveBeenCalledTimes(3); expect(backupReview).toHaveBeenCalledTimes(4); expect(openRes.servedBy).toBe("backup");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/failover.test.ts in the breaker-open test (and the re-open-after-failed-probe test), capture the result of the call that skips primary and assert res.servedBy === "backup" so direct-to-backup routing still tags the review result.♻️ Suggested fix
// 4th call: breaker open → primary skipped entirely. const openRes = await p.review(ctx()); expect(primaryReview).toHaveBeenCalledTimes(3); expect(backupReview).toHaveBeenCalledTimes(4); expect(openRes.servedBy).toBe("backup");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/failover.test.ts in the breaker-open test (and the re-open-after-failed-probe test), capture the result of the call that skips primary and assert res.servedBy === "backup" so direct-to-backup routing still tags the review result.tests/unit/config-backup.test.ts (1)
Line 60: Missing coverage for breaker threshold/cooldown parse edge cases.🧹 Nitpick | 🟢 Trivial
Missing coverage for breaker threshold/cooldown parse edge cases.
🤔 Medium confidence — verify against intent before acting.
The suite covers defaults and primary-timeout clamp/non-positive fallback, but not:
BACKUP_CIRCUIT_THRESHOLD=0/ negative / non-numeric → should fall back to 3BACKUP_CIRCUIT_COOLDOWN_MSnon-numeric → fall back to 60000 (0 is intentionally valid)- primary timeout when
AI_REQUEST_TIMEOUT_MSis unset/0 (no clamp path)Those branches are small but operator-facing; one compact table-driven case would lock the fail-soft parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/config-backup.test.ts, add cases for invalid BACKUP_CIRCUIT_THRESHOLD (0/negative/NaN → 3), invalid BACKUP_CIRCUIT_COOLDOWN_MS (non-numeric → 60000), and confirm cooldown 0 is accepted. Optionally assert primaryAiTimeoutMs is not clamped when AI_REQUEST_TIMEOUT_MS is 0/unset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/config-backup.test.ts, add cases for invalid BACKUP_CIRCUIT_THRESHOLD (0/negative/NaN → 3), invalid BACKUP_CIRCUIT_COOLDOWN_MS (non-numeric → 60000), and confirm cooldown 0 is accepted. Optionally assert primaryAiTimeoutMs is not clamped when AI_REQUEST_TIMEOUT_MS is 0/unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/config.ts`:
- Line 145: In src/config.ts around the PRIMARY_AI_TIMEOUT_MS / BACKUP_CIRCUIT_* parsing block, gate computation of primaryAiTimeoutMs, backupCircuitThreshold, and backupCircuitCooldownMs so they only resolve when backupAiProvider is set. Keep sensible defaults for the Config fields when backup is off, or leave them undefined if types allow; do not let PRIMARY_AI_TIMEOUT_MS affect anything on the no-backup path.
In `src/ai/provider-factory.ts`:
- Line 37: In src/ai/provider-factory.ts inside the openai-compatible branch of buildProvider, add a guard that throws when spec.localAiModel is missing or empty, matching the existing localAiBaseUrl check and the config-level validation for BACKUP_AI_PROVIDER=openai-compatible.
- Line 23: In src/ai/provider-factory.ts and the AnthropicProvider/OpenAIProvider constructors (or their usage-attribution helpers), either plumb ProviderSpec.label through for anthropic/openai the same way openai-compatible gets providerLabel, or update the ProviderSpec.label docs and README to state that same-type cost/log disambiguation currently only works for openai-compatible backups.
In `tests/unit/failover.test.ts`:
- Line 78: In tests/unit/failover.test.ts in the breaker-open test (and the re-open-after-failed-probe test), capture the result of the call that skips primary and assert res.servedBy === "backup" so direct-to-backup routing still tags the review result.
In `tests/unit/config-backup.test.ts`:
- Line 60: In tests/unit/config-backup.test.ts, add cases for invalid BACKUP_CIRCUIT_THRESHOLD (0/negative/NaN → 3), invalid BACKUP_CIRCUIT_COOLDOWN_MS (non-numeric → 60000), and confirm cooldown 0 is accepted. Optionally assert primaryAiTimeoutMs is not clamped when AI_REQUEST_TIMEOUT_MS is 0/unset.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 8357ba25-4cf5-4eb7-9adc-2ac63110308e
📥 Commits
Reviewing files that changed from c1da271 to be6e847. Previously-reviewed commits are not re-reviewed.
⛔ Files ignored due to path filters (1)
.env.exampleis excluded by!.env.example
📒 Files selected for processing (4)
src/ai/provider-factory.tssrc/config.tstests/unit/config-backup.test.tstests/unit/failover.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
README.mddocs/superpowers/plans/2026-07-09-backup-ai-provider-failover.mddocs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.mdsrc/ai/failover.tssrc/ai/transient.tssrc/realtime/jobs.tssrc/reviewer.tssrc/types.tstests/unit/transient.test.ts
- config: gate primary-timeout + breaker-knob env parsing on backupAiProvider so the off-by-default path never reads/applies them. (Minor) - buildProvider: also require localAiModel for the openai-compatible branch, matching the localAiBaseUrl guard. (Minor) - Clarify ProviderSpec.label doc: same-type attribution disambiguation is openai-compatible-only; anthropic/openai rely on provider+model distinctness. (Minor) - Tests: assert servedBy on the breaker-open (skip-primary) path; add breaker-knob fail-soft parse cases + a knobs-stay-default-when-off case. (Trivial) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Solid failover design: shared factory, fail-fast config, sequential primary→backup, and good unit coverage for the breaker. The off-by-default path is preserved. One major hole remains: non-review methods never set servedBy, so backup-served walkthrough/chat/complete traffic is invisible to the reviewer footnote and any downstream attribution. A few smaller maintainability cleanups would also help.
🔎 Issues not tied to a specific line (5)
These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.
servedBy is only defined on ReviewResult, so non-review backup traffic is invisible.
The PR description says failover covers all five AIProvider methods and that a subtle footnote is appended when the backup served the review. The unit tests confirm non-review methods fail over, but only review() can tag servedBy because that field lives on ReviewResult.
That leaves two gaps relative to the stated transparency goals:
- Backup-served walkthrough / chat / chatIssue / complete calls have no signal for operators or logs beyond whatever the provider itself emits.
- Any reviewer footnote logic keyed only on
ReviewResult.servedBywill correctly mark backup reviews, buttestAiProvider()/aiTarget()(already listed as a non-blocking follow-up) and any multi-call review pipeline cannot distinguish "primary walkthrough + backup review" from "fully backup-served".
If full transparency is in scope for this PR, plumb a small servedBy / last-used-provider side channel through the wrapper (e.g. a getter on FailoverProvider, or a shared call-result envelope) rather than overloading only ReviewResult. If review-body footnotes are intentionally review-only, document that non-review methods fail over silently so the design claim matches the code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Across src/ai/failover.ts, src/types.ts, and the reviewer wiring for this feature, decide how non-review methods report backup service. Either (a) add a FailoverProvider.lastServedBy getter (or similar) updated on every method and document that only review() gets a body footnote, or (b) extend the relevant return types / a shared envelope so walkthrough/chat/complete can also surface servedBy. Update tests/unit/failover.test.ts to assert the chosen signal on at least one non-review method.
Diff is incomplete relative to the PR description — core failover wiring is not in the changed files.
🤔 Medium confidence — verify against intent before acting.
The PR description and DiffSentry summary claim changes to src/ai/failover.ts, src/ai/transient.ts, src/types.ts, src/reviewer.ts, .env.example, and README.md, plus extraction of isTransientError and the backup footnote. The provided changed-files list only includes provider-factory.ts, the config parse, and two unit test files.
Without those files in the review surface, the most important behaviors cannot be verified here: transient classification, sequential await-before-backup, breaker state machine, ReviewResult.servedBy typing, primary timeout wiring, or the posted-review footnote. Either the diff is truncated for this review, or those files are missing from the PR — please confirm they land together so the feature is reviewable end-to-end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Verify that src/ai/failover.ts, src/ai/transient.ts, src/types.ts, src/reviewer.ts, .env.example, and README.md are included in this PR with the behaviors described in the PR body (FailoverProvider, isTransientError extraction, ReviewResult.servedBy, backup footnote, env docs). If any are missing from the branch, add them before merge so the feature is complete and reviewable.
Description claims core implementation files absent from the diff
🤔 Medium confidence — verify against intent before acting.
The PR description lists src/ai/failover.ts (FailoverProvider + circuit breaker), src/ai/transient.ts (isTransientError extraction), src/types.ts (ReviewResult.servedBy), and src/reviewer.ts (factory wiring + backup footnote) as changes, but none of those files appear in the actual changed-files set. Only provider-factory.ts, config.ts, and two test files are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. Description claims core implementation files absent from the diff The PR description lists src/ai/failover.ts (FailoverProvider + circuit breaker), src/ai/transient.ts (isTransientError extraction), src/types.ts (ReviewResult.servedBy), and src/reviewer.ts (factory wiring + backup footnote) as changes, but none of those files appear in the actual changed-files set. Only provider-factory.ts, config.ts, and two test files are present.
Operator docs and jobs.ts re-export claimed but not in diff
🤔 Medium confidence — verify against intent before acting.
Description says .env.example and README.md were updated for operator docs, and that jobs.ts re-exports isTransientError from the new transient module. Neither .env.example, README.md, nor any jobs.ts change appears in the provided diffs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. Operator docs and jobs.ts re-export claimed but not in diff Description says .env.example and README.md were updated for operator docs, and that jobs.ts re-exports isTransientError from the new transient module. Neither .env.example, README.md, nor any jobs.ts change appears in the provided diffs.
tests/unit/transient.test.ts claimed but missing from changes
🤔 Medium confidence — verify against intent before acting.
The Testing section states new unit coverage in tests/unit/transient.test.ts, but that file is not among the changed files. Only tests/unit/failover.test.ts and tests/unit/config-backup.test.ts are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. tests/unit/transient.test.ts claimed but missing from changes The Testing section states new unit coverage in tests/unit/transient.test.ts, but that file is not among the changed files. Only tests/unit/failover.test.ts and tests/unit/config-backup.test.ts are added.
🧹 Nitpick comments (5)
src/ai/provider-factory.ts (2)
Line 32: label is only applied for openai-compatible backups.🛠️ Refactor suggestion | 🟡 Minor
label is only applied for openai-compatible backups.
ProviderSpec.labelis documented as the cost/log disambiguator, but onlyOpenAICompatibleProviderreceives it. An anthropic/openai backup that reuses the same provider type and model as the primary will collapse into the primary inrecordAiUsage/ logs.That is called out as an unusual config and a future follow-up, but the current wiring makes the documented attribution guarantee incomplete for two of the three provider types. Either:
- extend
AnthropicProvider/OpenAIProviderconstructors to accept an optional label now, or- fail boot (or warn loudly) when the resolved backup is same-provider + same-model as the primary so operators cannot silently lose attribution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts around line 32, address the incomplete label attribution for anthropic/openai backups. Either pass an optional providerLabel into AnthropicProvider and OpenAIProvider constructors (mirroring OpenAICompatibleProvider) so same-type backups stay distinct in recordAiUsage, or add a boot-time check in loadConfig/reviewer that warns or rejects when the resolved backup is same provider + same model as the primary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts around line 32, address the incomplete label attribution for anthropic/openai backups. Either pass an optional providerLabel into AnthropicProvider and OpenAIProvider constructors (mirroring OpenAICompatibleProvider) so same-type backups stay distinct in recordAiUsage, or add a boot-time check in loadConfig/reviewer that warns or rejects when the resolved backup is same provider + same model as the primary.
Line 56: OpenAI branch is the implicit default with a less precise error path.🧹 Nitpick | 🟢 Trivial
OpenAI branch is the implicit default with a less precise error path.
After the anthropic and openai-compatible arms, any other
spec.providervalue falls into the OpenAI constructor. TodayProviderSpec.provideris a closed union so this is safe, but the error message only mentions openai, and an accidental widening of the union later would mis-construct rather than fail clearly.Prefer an explicit
if (spec.provider === "openai")+ exhaustiveneverfallback so the factory stays aligned with the three-way switch style used above.🔧 Proposed fix
if (spec.provider === "openai") { if (!spec.openaiApiKey) { throw new Error("buildProvider(openai) requires openaiApiKey"); } return new OpenAIProvider(spec.openaiApiKey, spec.openaiModel, spec.openaiBaseUrl, spec.timeoutMs); } const _exhaustive: never = spec.provider; throw new Error(`buildProvider: unsupported provider ${String(_exhaustive)}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts around line 56, replace the implicit openai fallback with an explicit if (spec.provider === "openai") branch and an exhaustive never default that throws, matching the explicit anthropic and openai-compatible arms above.♻️ Suggested fix
if (spec.provider === "openai") { if (!spec.openaiApiKey) { throw new Error("buildProvider(openai) requires openaiApiKey"); } return new OpenAIProvider(spec.openaiApiKey, spec.openaiModel, spec.openaiBaseUrl, spec.timeoutMs); } const _exhaustive: never = spec.provider; throw new Error(`buildProvider: unsupported provider ${String(_exhaustive)}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/provider-factory.ts around line 56, replace the implicit openai fallback with an explicit if (spec.provider === "openai") branch and an exhaustive never default that throws, matching the explicit anthropic and openai-compatible arms above.src/config.ts (1)
Line 147: primaryAiTimeoutMs and breaker knobs are always exported even when failover is off.🛠️ Refactor suggestion | 🟡 Minor
primaryAiTimeoutMs and breaker knobs are always exported even when failover is off.
🤔 Medium confidence — verify against intent before acting.
When
BACKUP_AI_PROVIDERis unset,primaryAiTimeoutMs/backupCircuitThreshold/backupCircuitCooldownMsstill land on the Config object with defaults. That is fine for the "off by default = unchanged runtime" claim only if every call site gates onbackupAiProviderbefore reading them.If any future consumer applies
primaryAiTimeoutMsunconditionally (e.g. wiringtimeoutMs: config.primaryAiTimeoutMsfor the primary), an operator who never opted into failover would silently get a 20s primary deadline instead ofAI_REQUEST_TIMEOUT_MS.Consider making these fields optional (
number | undefined) when failover is off, so misuse becomes a type error rather than a silent behavioral change.🔧 Proposed fix
let primaryAiTimeoutMs: number | undefined; let backupCircuitThreshold: number | undefined; let backupCircuitCooldownMs: number | undefined; if (backupAiProvider) { 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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/config.ts around line 147, change primaryAiTimeoutMs, backupCircuitThreshold, and backupCircuitCooldownMs to number | undefined and only assign them inside the if (backupAiProvider) branch. Update Config in src/types.ts accordingly so call sites that apply the short primary deadline must first prove a backup is configured.♻️ Suggested fix
let primaryAiTimeoutMs: number | undefined; let backupCircuitThreshold: number | undefined; let backupCircuitCooldownMs: number | undefined; if (backupAiProvider) { 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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/config.ts around line 147, change primaryAiTimeoutMs, backupCircuitThreshold, and backupCircuitCooldownMs to number | undefined and only assign them inside the if (backupAiProvider) branch. Update Config in src/types.ts accordingly so call sites that apply the short primary deadline must first prove a backup is configured.tests/unit/failover.test.ts (1)
Line 46: No assertion that a permanent non-401 error also skips failover.🛠️ Refactor suggestion | 🟡 Minor
No assertion that a permanent non-401 error also skips failover.
The suite covers 401 (no failover) and timeout (failover), but not other permanent 4xx classes the design promises to never mask — e.g. 400 bad request, 403, or a plain non-status
Error.Add one case for a 400/403 (and optionally a generic non-transient Error) so a future change to
isTransientErrorcannot silently start routing permanent primary failures to the backup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/failover.test.ts around the existing 401 case near line 46, add a companion test that rejects with status 400 (or 403) and asserts the backup is never called and the original error is rethrown. Optionally also cover a plain Error without status to lock the non-transient path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/failover.test.ts around the existing 401 case near line 46, add a companion test that rejects with status 400 (or 403) and asserts the backup is never called and the original error is rethrown. Optionally also cover a plain Error without status to lock the non-transient path.tests/unit/config-backup.test.ts (1)
Line 32: Reuse path is only covered for anthropic credentials.🧹 Nitpick | 🟢 Trivial
Reuse path is only covered for anthropic credentials.
reuses primary env when only BACKUP_AI_PROVIDER is setchecks anthropic key reuse, and a later test covers openai-compatible overrides, but there is no symmetric case for:
BACKUP_AI_PROVIDER=openaireusingOPENAI_API_KEY/ model / base URLBACKUP_AI_PROVIDER=openai-compatiblereusing primaryLOCAL_AI_*without anyBACKUP_LOCAL_AI_*Given the fail-fast validation branches differ per provider, those two paths are worth a couple of cheap asserts so a future edit to the reuse chain cannot regress silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/config-backup.test.ts after the anthropic reuse test around line 32, add two small cases: one with BACKUP_AI_PROVIDER=openai that only sets OPENAI_API_KEY and asserts backupOpenaiApiKey is reused, and one with BACKUP_AI_PROVIDER=openai-compatible that relies on BASE LOCAL_AI_* and asserts backupLocalAiBaseUrl/backupLocalAiModel are populated without BACKUP_LOCAL_AI_* overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In tests/unit/config-backup.test.ts after the anthropic reuse test around line 32, add two small cases: one with BACKUP_AI_PROVIDER=openai that only sets OPENAI_API_KEY and asserts backupOpenaiApiKey is reused, and one with BACKUP_AI_PROVIDER=openai-compatible that relies on BASE LOCAL_AI_* and asserts backupLocalAiBaseUrl/backupLocalAiModel are populated without BACKUP_LOCAL_AI_* overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/ai/provider-factory.ts`:
- Line 32: In src/ai/provider-factory.ts around line 32, address the incomplete label attribution for anthropic/openai backups. Either pass an optional providerLabel into AnthropicProvider and OpenAIProvider constructors (mirroring OpenAICompatibleProvider) so same-type backups stay distinct in recordAiUsage, or add a boot-time check in loadConfig/reviewer that warns or rejects when the resolved backup is same provider + same model as the primary.
- Line 56: In src/ai/provider-factory.ts around line 56, replace the implicit openai fallback with an explicit if (spec.provider === "openai") branch and an exhaustive never default that throws, matching the explicit anthropic and openai-compatible arms above.
In `src/config.ts`:
- Line 147: In src/config.ts around line 147, change primaryAiTimeoutMs, backupCircuitThreshold, and backupCircuitCooldownMs to number | undefined and only assign them inside the if (backupAiProvider) branch. Update Config in src/types.ts accordingly so call sites that apply the short primary deadline must first prove a backup is configured.
In `tests/unit/failover.test.ts`:
- Line 46: In tests/unit/failover.test.ts around the existing 401 case near line 46, add a companion test that rejects with status 400 (or 403) and asserts the backup is never called and the original error is rethrown. Optionally also cover a plain Error without status to lock the non-transient path.
In `tests/unit/config-backup.test.ts`:
- Line 32: In tests/unit/config-backup.test.ts after the anthropic reuse test around line 32, add two small cases: one with BACKUP_AI_PROVIDER=openai that only sets OPENAI_API_KEY and asserts backupOpenaiApiKey is reused, and one with BACKUP_AI_PROVIDER=openai-compatible that relies on BASE LOCAL_AI_* and asserts backupLocalAiBaseUrl/backupLocalAiModel are populated without BACKUP_LOCAL_AI_* overrides.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 5d4ef1e1-04b9-4950-8474-01a167a29dbd
📥 Commits
Reviewing files that changed from be6e847 to 9297926. Previously-reviewed commits are not re-reviewed.
⛔ Files ignored due to path filters (1)
.env.exampleis excluded by!.env.example
📒 Files selected for processing (4)
src/ai/provider-factory.tssrc/config.tstests/unit/config-backup.test.tstests/unit/failover.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
README.mddocs/superpowers/plans/2026-07-09-backup-ai-provider-failover.mddocs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.mdsrc/ai/failover.tssrc/ai/transient.tssrc/realtime/jobs.tssrc/reviewer.tssrc/types.tstests/unit/transient.test.ts
TL;DRThis PR adds an opt-in backup AI provider so that when the primary model fails transiently (timeouts, 5xx, 429, network blips), DiffSentry can fall over to a secondary and still post a review instead of dead-lettering it. It was motivated by PR #77, where Generated by DiffSentry on demand. Re-run with |
|
@diffsentry release-notes |
📣 Release Notes✨ What's new
🛠 Improvements
Marketing-speak version of this PR. Re-run with |
1 participant
What & why
When the primary review model fails transiently or hangs, DiffSentry now falls over to a configured backup AI provider instead of dead-lettering the review. Motivated by PR #77, where
grok-4.5(OpenAI-compatible) intermittently stalled the largereviewcall past the 60s timeout and all retries hit it, so no review was posted.Off by default — unset
BACKUP_AI_PROVIDER⇒ behavior is byte-for-byte unchanged (plain primary provider, fullAI_REQUEST_TIMEOUT_MS).Complementary to the operational mitigation (raising
AI_REQUEST_TIMEOUT_MS/ scoping retries), not a replacement.Design (agreed in brainstorm — see spec)
AiTimeoutError, 5xx, 429, network). Never on 4xx (401/403/400) — a bad primary key surfaces loudly instead of being masked by backup traffic.PRIMARY_AI_TIMEOUT_MS(default 20s, clamped ≤AI_REQUEST_TIMEOUT_MS) so a slow-hang switches fast; the backup gets the full 60s.AIProvidermethods, per-call, sequential (primary awaited to rejection before backup) — which also makes double-posting impossible.BACKUP_AI_PROVIDERselects a type and reuses that provider's existing env, with optionalBACKUP_*overrides.recordAiUsage.Spec:
docs/superpowers/specs/2026-07-09-backup-ai-provider-failover-design.mdPlan:
docs/superpowers/plans/2026-07-09-backup-ai-provider-failover.mdChanges
src/ai/transient.ts— extractedisTransientError(shared by the job runner and the new wrapper;jobs.tsre-exports it).src/ai/failover.ts—FailoverProvider implements AIProvider+ circuit breaker (injectable clock for deterministic tests).src/ai/provider-factory.ts—buildProvider(spec); unifies the anthropic/openai/openai-compatible construction switch.src/config.ts/src/types.ts— backup env parsing/validation (fail-fast at boot),primaryAiTimeoutMs+ breaker knobs,ReviewResult.servedBy.src/reviewer.ts— build primary via the factory, wrap inFailoverProviderwhen a backup is configured, append the backup footnote..env.example/README.md— operator docs.Testing
npx tsc --noEmitclean ·npm test328/328 ·npm run lintclean ·npm run buildclean.tests/unit/transient.test.ts,tests/unit/failover.test.ts(8 cases: success, transient failover, no-failover on 401, both-fail, breaker open/half-open/reset, all-five-method delegation),tests/unit/config-backup.test.ts(9 cases incl. reuse/override + validation branches).reviewer.reviewPR; the failover wrapper is tested in isolation, and the unchanged 326-test baseline staying green proves the no-backup path is untouched.Non-blocking follow-ups (from the final review)
testAiProvider()/aiTarget()report the primary only; a probe could be silently backup-served.🤖 Generated with Claude Code
Summary
Added an opt-in backup AI provider failover so transient primary failures fall over to a configured secondary model instead of dead-lettering the review. Extracted shared provider construction, wired backup config with circuit-breaker knobs, and covered the behavior with unit tests while keeping the feature off by default.
Changes
src/ai/provider-factory.tssrc/config.tstests/unit/config-backup.test.tstests/unit/failover.test.ts