fix(compact): route combo compact requests through failover path - #3063
fix(compact): route combo compact requests through failover path#3063x3M3x wants to merge 3 commits into
Conversation
|
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe native ChangesCompact response routing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Combo-resolved compact requests now use the existing failover path, with no actionable merge-blocking risk remaining after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
리뷰 · 우선순위 64 / 80이 PR은 combo로 풀린 compact 요청이 고치는 방법은 가드에 다만 우회의 대가가 있습니다. 콤보 첫 타깃이 ChatGPT/공식 OpenAI여도 더 이상 진짜 현재 라인 559 - 가드에 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Pushed
Focused test command: |
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 0a7524b0240f8a79b7db4e6c996ceca831694f7b.
Routing combo compact through the existing failover loop is valuable, and the 429-to-backup case covers that direction. The new canonical-success test masks a real output-shape failure, however.
When a combo target succeeds through canonical ChatGPT Responses, its completed event carries a real opaque OpenAI compaction.encrypted_content, not an OpenCodex ocx1: envelope. The final branch preserves that native item only when accountGatedCompactWireModel is true. A normal combo such as combo/free -> openai/gpt-5.4 has that flag false, so the code calls decodeCompactionSummary() on the opaque OpenAI ciphertext and returns 502 invalid_response_error even though the canonical child succeeded.
The test currently avoids this by building the mocked canonical response with encodeCompactionSummary("compact summary"), which is the non-native adapter envelope and is not what the test's canonical endpoint would return. Change it to opaque native ciphertext and assert the exact compaction item is preserved. The implementation must decide from the actual successful combo attempt/output whether to return the native compaction item or decode an ocx1: synthetic summary; the initial route-level accountGatedCompactWireModel flag is insufficient once failover can select different target kinds.
Also make the focused test pass locally before enabling fork CI. The author notes it currently stops at a 401 before reaching the intercepted upstream path, so the new assertions are not yet validating this code. Fix the isolated auth/config harness rather than relying on hosted CI to make an unexecuted path green.
This head is 15 commits behind current dev. Rebase after the output-shape fix, then request exact-head compact/combo and cross-platform CI review.
When a compact request resolved through a combo, the native-compact fast path sent the request directly to the picked provider without failover. A 429 or 5xx from that target surfaced as an exhausted-retry error to the client instead of advancing to the next combo target. Skip native compact when route.combo is set so the request falls through to the synthetic compaction path, which dispatches through handleResponses → handleComboResponses with full combo failover (cooldown + advanceToNext).
0a7524b to
9582fc3
Compare
…oadmap (lidge-jun#3087) Rescans every open issue and bug-labelled PR against a written-down four-axis rubric, and plans the six targets that score >= 70 as one PABCD cycle each. Six enter the train: lidge-jun#3071 (73), lidge-jun#3032 (75), lidge-jun#3026 (75), lidge-jun#3029 (72), lidge-jun#3008 (71), lidge-jun#3019 (70). Sixteen below-bar items are recorded with components so the next scan does not re-litigate them, and lidge-jun#3068 is suppressed as a duplicate of lidge-jun#3071. The scan corrected several assumptions the titles suggested. lidge-jun#1527 and lidge-jun#3070 are already fixed on dev; lidge-jun#3059 asserts an unmount path the tree cannot produce; PRs lidge-jun#3040, lidge-jun#3041 and lidge-jun#3067 each found a real defect and proposed a worse remedy; PRs lidge-jun#3063 and lidge-jun#3038 claim regressions that pass against unfixed source. Eleven adversarial review rounds, all findings verified in-tree before amendment. Findings per round: 9, 5, 4, 4, 3, 2, 3, 3, 1, 0. Round 11 passed. Round 1 found nine holes in the plan; after that the defects were in the fixes, which is what 002-011 mostly record.
|
Implemented reviewer request in 9582fc3 (rebased onto current origin/dev).
Verified: bun test tests/server-combo-failover-e2e.test.ts -t 'combo compact' (2 pass), bun x tsc --noEmit, bun run privacy:scan. PR remains draft. |
|
Superseded by #3109, which is your three commits rebased onto current Worth recording: an earlier triage pass called this PR's regression vacuous. That reading was of your first commit alone, which is production-only. Commit Rebase check included in #3109: Triaged in the 2026-08-31 non-priority-70 bug round. |
Summary
When a compact request resolved through a combo, the native-compact fast path sent the request directly to the picked provider without failover. A 429 or 5xx from that target surfaced as an exhausted-retry error to the client instead of advancing to the next combo target.
Fix
Added && !route.combo to the native-compact guard in src/server/responses/compact.ts. When the route carries a combo pick, the request now falls through to the synthetic compaction path, which dispatches through handleResponses → handleComboResponses — that path already has the full combo failover loop (cooldown the failed target, advance to the next eligible one, retry).
Verification
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit