Skip to content

fix(compact): route combo compact requests through failover path - #3063

Draft
x3M3x wants to merge 3 commits into
lidge-jun:devfrom
x3M3x:codex/compact-combo-failover
Draft

fix(compact): route combo compact requests through failover path#3063
x3M3x wants to merge 3 commits into
lidge-jun:devfrom
x3M3x:codex/compact-combo-failover

Conversation

@x3M3x

@x3M3x x3M3x commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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

  • �un run typecheck — clean
  • �un test tests/combos.test.ts — 43/43 pass
  • �un test tests/server-combo-failover-e2e.test.ts — all combo-specific tests pass (pre-existing Windows temp-dir failures are unrelated)

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

  • Bug Fixes
    • Improved compact response routing for combined model configurations.
    • Added more reliable failover when a selected model returns temporary errors or rate limits.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.

@github-actions github-actions Bot added the bug Something isn't working label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ 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.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft August 31, 2026 09:15
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 508e0c53-339a-4327-9b5f-ed747ebf7871

📥 Commits

Reviewing files that changed from the base of the PR and between 93b7ee8 and b9e9899.

📒 Files selected for processing (1)
  • src/server/responses/compact.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The native /responses/compact forwarding guard now excludes combo-resolved routes. These routes use the routed summarizer path, which dispatches through handleResponses and handleComboResponses for combo target failover.

Changes

Compact response routing

Layer / File(s) Summary
Combo routing guard
src/server/responses/compact.ts:556-562
The native compact endpoint condition now requires !route.combo. Combo-resolved targets fall through to the routed summarizer path and can advance through combo targets after 429 or 5xx responses.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to b9e98

Combo-resolved compact requests now use the existing failover path, with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: lidge-j

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: routing combo compact requests through the failover path. It matches the update in src/server/responses/compact.ts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

이 PR은 combo로 풀린 compact 요청이 src/server/responses/compact.ts의 네이티브 /responses/compact 빠른 길로 들어가면서, 콤보 페일오버 루프를 통째로 건너뛰는 구멍을 막습니다. 지금 dev HEAD(93b7ee80a)에서 handleResponsesCompactrouteModel으로 모델을 고른 뒤, supportsNativeResponsesCompactEndpoint(route.providerName, route.provider) && !accountGatedCompactWireModel이면 바로 ChatGPT/공식 OpenAI 컴팩트 엔드포인트로 보냅니다. 이 네이티브 가지는 풀 계정 교체(#913 계열), previous-model handoff(#2858), 호스트 서킷 같은 같은 프로바이더 안의 재시도는 있어도, handleComboResponses가 하는 cooldown → advanceToNext → 다음 타깃 재디스패치는 없습니다. 그래서 combo 타깃 목록의 첫 칸이 ChatGPT나 openai-apikey처럼 네이티브 컴팩트를 지원하는 모델이고 그 타깃이 429/5xx를 내면, 클라이언트에는 재시도 소진 오류만 보이고 두 번째 타깃으로 넘어가지 않습니다. 저자가 말한 증상과 HEAD 코드가 맞습니다.

고치는 방법은 가드에 && !route.combo를 하나 더 붙이는 것입니다. RouteResult.combosrc/router.ts에서 콤보 pick이 붙을 때만 채워지므로, 일반 네이티브 모델 compact와 account-gated(Daybreak 등) synthetic 분기는 그대로 둡니다. 콤보일 때만 아래 ROUTED 합성 컴팩션으로 떨어지고, 그 경로는 compaction_trigger를 붙인 내부 요청을 handleResponses에 다시 넣습니다. handleResponsesInner는 body에서 combo id를 읽으면 handleComboResponses로 보내고, 그 안의 while (pick) 루프가 이미 429/모델-lifecycle 실패에 대해 쿨다운과 다음 타깃 전진을 합니다. 즉 새 페일오버 엔진을 만들지 않고, 이미 검증된 콤보 루프를 compact에도 태우는 값싼 우회입니다. types.ts/config.ts 분할에 걸려 닫을 PR도 아니고, 같은 주제로 열려 중복 닫을 PR도 보이지 않습니다.

다만 우회의 대가가 있습니다. 콤보 첫 타깃이 ChatGPT/공식 OpenAI여도 더 이상 진짜 /responses/compact를 쓰지 않고, COMPACT_PROMPT + compaction_trigger 합성 턴으로 요약을 만듭니다. 와이어·품질·비용이 네이티브 컴팩트와 다릅니다. 더 민감한 점은 stream 플래그입니다. 합성 경로의 streamaccountGatedCompactWireModel ? true : false입니다. 주석에도 Canonical ChatGPT Responses는 non-streaming 턴을 거절한다고 적혀 있고, 그래서 Daybreak 같은 account-gated 모델만 지금 SSE로 돌립니다. 이 PR 이후에는 route.combo만으로 네이티브를 건너뛰므로, 콤보→ChatGPT(게이트 아닌 gpt 계열) 합성 턴이 stream: false로 나갈 수 있습니다. 예전에는 그 경우 네이티브 compact가 성공했을 수 있는데, 지금은 페일오버를 얻으려다 ChatGPT 자식에서 non-stream 거절이 날 위험이 있습니다. 회귀 테스트가 없는 상태에서 이 조합을 놓치기 쉽고, 실제로 hygiene 봇이 missing_regression_test로 draft를 유지 중입니다.

현재 dev 우선순위(#3048 Aside Integrations, #3054 entitlement refresh, shutdown spill)와 겹치지는 않지만, 콤보·컴팩트 열차(#2858 handoff, #2922 compact 402, #2850 combo encrypted recovery)와는 같은 축입니다. 한 줄 가드라 머지 비용은 싸지만, 테스트와 stream 가드 없이 넣으면 콤보→ChatGPT compact가 조용히 깨질 수 있어서 지금은 64점입니다.

라인 559 - 가드에 && !route.combo를 추가한 방향은 맞다. 콤보 pick이 붙은 라우트만 synthetic으로 보낸다
라인 556-558 - 주석 들여쓰기가 한 칸 빠졌다. 의미 변경은 아니지만 HEAD 스타일과 어긋난다. 같이 맞추라
라인 998 - stream: accountGatedCompactWireModel ? true : false — 콤보가 ChatGPT 자식을 고르면 account-gated가 아닌데도 non-stream이 될 수 있다. 콤보 우회 시 ChatGPT/공식 OpenAI 자식은 stream true가 필요한지 검증하라
경로 handleResponses → handleComboResponses - 합성 경로가 콤보 페일오버를 타는 연결은 HEAD에서 이미 있다. 이 PR이 새로 만든 루프는 없다
경로 tests/ - src 동작이 바뀌었는데 테스트 파일이 없다. hygiene missing_regression_test와 draft 유지가 맞다. 최소 한 케이스는 combo 첫 타깃 429 후 둘째 타깃으로 compact가 넘어가는 보여야 한다
경로 tests/server-combo-failover-e2e.test.ts · tests/responses-compaction-routing.test.ts - 콤보 e2e와 compact 라우팅 테스트는 이미 있다. 여기 compact+combo 교차 케이스를 붙이는 편이 새 하네스를 만드는 것보다 싸다

메인테이너의 판단이 필요한 지점

  • 콤보 compact에서 네이티브 /responses/compact를 포기하고 항상 합성 요약으로 가도 되는지, 아니면 네이티브 경로 안에 콤보 페일오버를 심는 쪽이 맞는지
  • 콤보→ChatGPT 합성 턴의 stream 기본값을 true로 올릴지, 아니면 account-gated일 때만 SSE로 둘지
  • hygiene 예외(test-exception-approved)로 한 줄 가드만 먼저 넣을지, 회귀 테스트 통과 전까지 draft로 둘지

너의 추천
방향은 맞고 버그도 진짜다. 머지 전에 (1) 주석 들여쓰기 복구, (2) combo 첫 타깃 429 → 둘째 타깃 compact 성공 회귀 테스트 추가, (3) 콤보→ChatGPT(비 account-gated)에서 synthetic stream이 거절되지 않는지 한 케이스 확인. stream이 깨지면 route.combo일 때 ChatGPT/공식 OpenAI 자식은 stream true로 보내거나, 네이티브 compact를 유지한 채 콤보 루프만 감싸는 쪽으로 수정하라. 체크리스트 0/4·draft·hygiene 블록 상태에서는 머지하지 말고, 위 세 가지 채운 뒤 ready로 올려라.

이 댓글은 grok-bot이 작성했습니다

@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 31, 2026
@x3M3x

x3M3x commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 0a7524b02 to the PR branch.

  • Fixed the review's comment indentation.
  • Added combo compact regression coverage: native-capable first target returning 429 advances to the backup target.
  • Forced combo synthetic compaction turns to SSE so canonical children that reject non-streaming turns remain usable.

Focused test command: bun test tests/server-combo-failover-e2e.test.ts -t "combo compact failover". The new cases currently hit the repository's existing Windows auth/config test-environment behavior (401 before the intercepted upstream path); the source and assertions are pushed for hosted CI validation.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

x3M3x added 3 commits August 31, 2026 20:19
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).
@x3M3x
x3M3x force-pushed the codex/compact-combo-failover branch from 0a7524b to 9582fc3 Compare August 31, 2026 16:19
x3M3x pushed a commit to x3M3x/opencodex that referenced this pull request Aug 31, 2026
…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.
@x3M3x

x3M3x commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Implemented reviewer request in 9582fc3 (rebased onto current origin/dev).

  • Preserves opaque native compaction ciphertext through the Responses adapter and bridge instead of re-encoding it as ocx1.
  • Keeps synthetic ocx1 summaries decoded into v1 history.
  • Adds a regression using opaque ciphertext and an API-key OpenAI mock, with local 429 failover coverage.

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.

@lidge-jun

Copy link
Copy Markdown
Owner

Superseded by #3109, which is your three commits rebased onto current dev — it now contains the #3071 web-search fix (a0d386b49), which rewrote the same two files hours ago.

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 78855ed06 adds tests that drive the real handleResponsesCompact with a ChatGPT 429 and an openai-chat backup. Mutation: dropping && !route.combo gives 74 pass / 2 fail. Judging a PR by one commit is how a correct change gets thrown away.

Rebase check included in #3109: tests/bridge.test.ts and tests/openai-responses-passthrough.test.ts stay green on the merged result.

Triaged in the 2026-08-31 non-priority-70 bug round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants