Repository navigation
test(proxy): cover the auto-fallback attempt regression in the continuous suite - #1270
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe test suite adds Claude proxy regression coverage for auto-fallback, retry attempts, terminal-error finalization, and usage statistics. The exhaustion test mock enables auto-fallback to exercise the two-attempt path. ChangesProxy fallback accounting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR moves coverage for a proxy auto-fallback behavior regression into the repository’s actual continuous test runner, ensuring the opt-in auto-fallback change (introduced in 33bdd141) can’t silently change upstream attempt accounting again.
Changes:
- Adds a continuous-suite regression test that exercises both auto-fallback enabled and default-disabled paths for an exhausted translated Claude request, resetting global usage stats between sub-cases.
- Updates the existing (orphaned) Vitest proxy test router stub to opt into auto-fallback so its
totalAttempts: 2assertion remains accurate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/proxyTerminalErrorAccounting.test.ts |
Updates the test’s router stub to explicitly enable auto-fallback so the test continues to reach the second-attempt path it asserts. |
test/continuous-test-suite-bugfixes.ts |
Adds continuous-suite coverage for the auto-fallback opt-in behavior affecting translated request upstream attempt counts. |
Suppressed comments (1)
test/continuous-test-suite-bugfixes.ts:9061
- The new continuous regression test asserts ProxyStats totals, but it doesn’t verify the proxy terminal-error journal (
getTerminalErrors()), which is the “finalized exactly once (not twice)” behavior described in the PR. Without checkinggetTerminalErrors().totalErrors, a regression could reintroduce double-finalization in terminal error accounting while still passing these assertions.
const result = (await messagesRoute.handler(ctx)) as {
type?: string;
};
return { result, stats: getStats() };
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review SummaryDecision: APPROVED ✅ Changes OverviewThis PR fixes a regression introduced in commit Files Changed
Test Coverage AnalysisThe new test (
The test properly resets usage stats between scenarios using Impact on Existing Code
Verification Performed✅ Verified import paths match existing patterns in the codebase ConclusionThe PR addresses a real regression with proper test coverage. Implementation is sound, follows project conventions, and introduces no breaking changes. Ready to merge. |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR fixes a regression in proxy auto-fallback behavior introduced in commit 33bdd141. The fix adds proper coverage to the continuous test suite and updates an orphaned Vitest file.
Findings
2 issues found:
- 🔒 CRITICAL (line 126): Missing
getTerminalErrors()import needed for regression test validation - blocks the test from properly validating terminal error finalization ⚠️ MAJOR (line 9057): Same missing import issue affects another location in the same file
Impact Assessment
- Changed files: 2 (test/continuous-test-suite-bugfixes.ts, test/proxyTerminalErrorAccounting.test.ts)
- Risk level: Medium - affects proxy fallback behavior which impacts reliability
- Test coverage: Improved by adding to continuous suite
- Breaking changes: None
Decision
CHANGES_REQUESTED - The missing import at line 126 is critical as it prevents the regression test from properly validating terminal error finalization. This needs to be fixed before merge.
Recommendations
- Add
getTerminalErrorsto the imports fromsrc/lib/mcp/aiCoreServer.jsalongsideusageStats - Verify the fix resolves the regression test failure
- Consider addressing the other orphaned test files mentioned in the PR description
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR fixes a regression in proxy auto-fallback behavior introduced in commit 33bdd141. The fix adds proper coverage to the continuous test suite and updates an orphaned Vitest file.
Findings
2 issues found:
- 🔒 CRITICAL (line 126): Missing
getTerminalErrors()import needed for regression test validation - blocks the test from properly validating terminal error finalization ⚠️ MAJOR (line 9057): Same missing import issue affects another location in the same file
Impact Assessment
- Changed files: 2 (test/continuous-test-suite-bugfixes.ts, test/proxyTerminalErrorAccounting.test.ts)
- Risk level: Medium - affects proxy fallback behavior which impacts reliability
- Test coverage: Improved by adding to continuous suite
- Breaking changes: None
Decision
CHANGES_REQUESTED - The missing import at line 126 is critical as it prevents the regression test from properly validating terminal error finalization. This needs to be fixed before merge.
Recommendations
- Add
getTerminalErrorsto the imports fromsrc/lib/mcp/aiCoreServer.jsalongsideusageStats - Verify the fix resolves the regression test failure
- Consider addressing the other orphaned test files mentioned in the PR description
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · 🤖 Yama Review Summary
No verified findings were accepted by the review gate. The verdict reported findings but carried no detail — treat it as unverified and re-run the review or inspect manually. |
116e8d5 to
213229a
Compare
Re: MAJOR "only 1 attempt happens regardless of the autoFallback flag"This one is testable, and it does not hold. The test asserts both directions and I verified it by patching the source in each direction:
If the flag made no difference with an empty The reason is that the two mechanisms are different. if (!loopState.sawRateLimit && modelRouter?.isAutoFallbackEnabled?.()) {No MINOR: "mocks getFallbackChain() to return an empty array — doesn't test the fallback mechanism"Deliberate, and the empty chain is the point. With entries present the request would fall back through configured providers and the auto-fallback flag would be masked. Emptying it isolates the one mechanism under test. Coverage of the configured chain belongs with the routing tests, not here. Rebased onto |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
213229a to
a33fe4d
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Correction to my earlier thread replies: the current remote head is |
|
Current-head verification: |
a33fe4d to
846c0ab
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Yama Review AnalysisI've thoroughly reviewed this PR and found no issues. The code is correct: Verification Summary
No Issues FoundThis PR successfully adds regression test coverage for the auto-fallback behavior change. All changes are well-written and follow the project's testing patterns. Decision: APPROVED |
|
Rebased onto the current This push re-triggers Yama on the current head. The previous CHANGES_REQUESTED verdict was submitted against For the record, the |
846c0ab to
fa73cbc
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
…uous suite 33bdd14 made translation-layer auto-fallback opt-in, which changed how many upstream attempts an exhausted translated request makes. Nothing caught it: the only coverage lived in test/proxyTerminalErrorAccounting.test.ts, which no package.json script runs, so its "two attempts" assertion has been failing on release ever since. Bisected — passes at 65b27b7, fails from 33bdd14 onward. Adds the coverage to continuous-test-suite-bugfixes.ts, where 33bdd14 itself put its proxy tests and which test:unit already runs, asserting both directions of the flag: opted in makes a second attempt, the default makes one, and either way the terminal error is finalized exactly once. Also opts the stale vitest assertion into auto-fallback so that file no longer states something untrue about the current behaviour.
fa73cbc to
1d5e568
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Review SummaryThis PR fixes test coverage for an auto-fallback regression that was silently breaking behavior. Changes Reviewed (File-by-File)test/continuous-test-suite-bugfixes.ts ✅
test/proxyTerminalErrorAccounting.test.ts ✅
Impact on Existing Code
Verdict: APPROVED ✅This is a well-crafted test-only PR that adds critical missing coverage for a regression. Both files show excellent attention to detail:
The PR correctly addresses the issue identified by bisecting from commit 65b27b7 (passing) through 33bdd14 (breaking) to today's release (still broken). Review scope: Test coverage for proxy auto-fallback regression |
|
🎉 This PR is included in version 10.8.19 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Replaces #1267, which took the wrong approach (it wired Vitest files into
test:unitand added a CI check enforcing that). The finding underneath is real and runner-independent, so it lands here in the form the repo actually uses.The regression
33bdd141 fix(proxy): bound account admission and fallback routingmade translation-layer auto-fallback opt-in. That changed how many upstream attempts an exhausted translated request makes — two before, one now.Nothing caught it. The only coverage was in
test/proxyTerminalErrorAccounting.test.ts, which nopackage.jsonscript runs, so itstotalAttempts: 2assertion has been failing onreleaseever since and no one saw it.33bdd141updated the three orphaned proxy suites its author knew about and missed this one.Bisected rather than assumed: passes at
65b27b75, fails from33bdd141onward, still fails on today'srelease.The fix
Coverage goes into
test/continuous-test-suite-bugfixes.ts— where33bdd141itself put its proxy tests, and whichtest:unitalready runs. PerCLAUDE.md:190("all suites run via tsx; there is no vitest runner") and:258.The test asserts both directions of the flag in one case, resetting the global usage stats between them so it doesn't depend on suite ordering:
It also opts the stale Vitest assertion into auto-fallback, so that file stops stating something untrue. It is not wired into any script — no
package.jsonchange in this PR.Verification
Two-sided negative control, since a one-sided one would pass on a test that only checks a single branch:
33bdd141)pnpm run check: 0 errors / 4,838 files.pnpm run lint: 0 errors.Still open, deliberately not bundled
Eleven other
test/*.test.tsfiles remain unreferenced by any script, and.github/workflows/ci.ymlruns no test step at all (format:check,eslint,validate:all,build,build:cli). Both need a direction from you on the intended end state for those 28 Vitest files — migrate to continuous suites, or delete — rather than a guess from me.Summary by CodeRabbit