fix(responses): let a combo shadow-call target enter the failover loop - #4157
Conversation
A shadowCallIntercept whose replacement names a combo ran exactly one attempt
and never entered the failover loop, so a 429 or 5xx from the first target
returned to the caller instead of hopping to the next one.
Two cooperating causes. The combo gate reads the UN-rewritten body:
comboIdFromRawBody sees only body.model, which is still the bare helper slug
(gpt-5.6-luna) at that point, so handleComboResponses never runs and neither
does advanceComboAfterFailure. The rewrite happened later, after parse, where
resolveRoute("combo/shadow") goes through routeModel -> tryPickComboModel and
collapses the combo to ONE target while still tagging routeKind "combo". That
collapsed pick is the reported "combo route, one attempt".
There is a second path through the same site. shouldInterceptShadowCall is
isShadowSourceModel && !shadowCallTargetsIntersect, so when the collapsed first
pick happens to be openai/gpt-5.6-luna the intersect check is true, the
intercept is skipped outright, and the request leaves as a plain native route
with no marker. Swapping the two blocks does not close that path.
Rewrite the selector before comboIdFromRawBody reads it instead, and identify
the combo with resolveComboId - a pure config lookup that performs no routing
and therefore cannot collapse the table. A combo selector is routing policy,
not the identity of its first pick. The existing combo gate takes it from there
and handleComboResponses runs its ordinary loop.
shouldInterceptShadowCall is left alone: it still suppresses direct
same-provider replacements (#2706). The late intercept site does not re-enter
handleComboResponses, which would double-run expandPreviousResponseInput and
onRequestBodyRead. The marker records the operator-configured prefix through
sanitizeLogMetadataString exactly as the late site does, so no caller-controlled
string is persisted.
Closes #4129.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughShadow-call interception now rewrites eligible source models before combo detection. Configured combo targets enter ChangesShadow combo interception
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Configured combo shadow replacements now retain combo failover behavior after 429 or server failures, including source-overlap cases, while ordinary non-combo interception behavior remains unchanged. Sequence Diagram(s)sequenceDiagram
participant HelperRequest
participant handleResponsesInner
participant handleComboResponses
participant UpstreamProviders
HelperRequest->>handleResponsesInner: Submit shadow source model
handleResponsesInner->>handleResponsesInner: Rewrite model to configured combo
handleResponsesInner->>handleComboResponses: Dispatch combo
handleComboResponses->>UpstreamProviders: Try targets in failover order
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
리뷰 · 우선순위 77 / 80이 PR은 지금 원인은 둘이 같이 맞물린다. 첫째, 콤보 게이트( 고침은 콤보 게이트 앞에서, 설정만 보고 콤보인지 판별한 뒤 selector를 먼저 고쳐 쓰는 것이다. 라인 단위로 보면 고침 위치는 맞고, 일부러 남겨 둔 표면이 두 군데다. src/server/responses/core.ts (early rewrite, comboIdFromRawBody 직전) - 늦은 intercept가 켜는 src/server/management/shadow-call-validation.ts 의 tests/responses/responses-shadow-intercept.test.ts - 회귀 세 칸은 의도(홉 / self-intersect / non-combo 보존)가 분명하다. 다만 이 라운드는 로컬 suite를 안 돌리기로 했으니, exact-head 원격 CI(test 1–4 / macos)가 초록인지만 게이트로 보면 된다. 지금 일부 shard는 아직 pending이다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79fec5d6cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && isShadowSourceModel(rawShadowModel, shadowIntercept.sourceModels)) { | ||
| const shadowComboId = resolveComboId(config, shadowIntercept.model); | ||
| if (shadowComboId && Object.hasOwn(config.combos ?? {}, shadowComboId)) { | ||
| (body as Record<string, unknown>).model = shadowIntercept.model; |
There was a problem hiding this comment.
When this rewrite dispatches a shadow call to a combo whose selected target uses the Cursor adapter, it returns through handleComboResponses before the late interceptor can set parsed._cursorIsolateConversation = true. Combo children preserve x-codex-parent-thread-id, so Cursor derives the parent's conversation ID and may reuse or update its checkpoint, allowing title/commit helper traffic to contaminate the main conversation; this regresses the previous single-target path, which did set the isolation flag. Carry a shadow-isolation bit into every combo child and add a Cursor-target regression test; structure/04_transports-and-sidecars.md:1116-1117 explicitly requires isolated helper/shadow turns never to join parent or sibling conversations.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
…openai provider
The first CI run proved the fix itself: the combo loop was entered and hopped
("[combo] shadow: xai/grok-4.5 failed with 429", then the second target). Both
new cases still failed, because the fixture used a provider literally named
"openai", whose endpoint is pinned to https://chatgpt.com/backend-api/codex.
The configured helper.example baseUrl was ignored with a warning and the second
target answered 401 instead of the mocked 200.
Move both cases onto ordinary key providers. The self-target case now uses the
same shape as the existing #2706 no-op test: a custom sourceModels prefix whose
resolved provider is also the combo's first target, so shadowCallTargetsIntersect
is genuinely true for the collapsed one-candidate pick. That is the condition
that used to suppress the intercept, and it is now reproduced without depending
on a reserved provider id.
1 participant
Summary
A
shadowCallInterceptwhose replacement names a combo ran exactly one attempt and never entered the failover loop, so a 429 or 5xx from the first target came back to the caller instead of hopping to the next one. The operator saw a log line claiming a combo route with a single attempt, which is exactly what it was.Two cooperating causes, not one.
The combo gate reads the un-rewritten body.
comboIdFromRawBodylooks only atbody.model, and at that point the model is still the bare helper slug (gpt-5.6-luna), which is not a combo id.handleComboResponsesis therefore never called, itswhile (pick)loop never runs,advanceComboAfterFailurenever runs, and the 429/5xx hop decisions — which exist only inside that loop — are unreachable.The rewrite happened later, after parse, where
resolveRoute("combo/shadow")goes throughrouteModel→tryPickComboModel. That collapses the combo table to one target while still taggingrouteKind: "combo". The result looks like a combo route and behaves like a single native call.There is a second path through the same site.
shouldInterceptShadowCallisisShadowSourceModel && !shadowCallTargetsIntersect, so when the collapsed first pick happens to beopenai/gpt-5.6-lunathe intersect check is true, the intercept is skipped outright,shadowCallRewrittenFromstays unset, and the request leaves as a plain native route. Simply swapping the order of the two blocks does not close that path.The fix rewrites the selector before
comboIdFromRawBodyreads it, and identifies the combo withresolveComboId— a pure config lookup that performs no routing and therefore cannot collapse the table. A combo selector is routing policy, not the identity of its first pick. The existing combo gate takes it from there andhandleComboResponsesruns its ordinary loop.What is deliberately unchanged:
shouldInterceptShadowCallstill suppresses direct same-provider replacements ([Bug]: Shadow Call Intercept forces effort low on every gpt-5.6-luna request when luna is the target (or main model) — max turns silently downgraded #2706). Only the combo case bypasses it, because a combo has no single identity to intersect against.handleComboResponses. Doing so would double-runexpandPreviousResponseInputandonRequestBodyRead.shadowSourceModelPrefix+sanitizeLogMetadataString, exactly as the late site does, so no caller-controlled model string is ever persisted tousage.jsonlor/api/logs.Object.hasOwn(config.combos, id)mirrors the existing gate, so acombo/<id>that parses but is not configured falls through to the ordinary intercept unchanged.Known follow-ups, deliberately left out of this PR:
parsed._cursorIsolateConversationis not propagated to combo children. Plumbing a newHandleResponsesOptionsbit is a separate change and only matters when a Cursor target sits inside the combo.shadowCallTargetErrorin the management API collapses the same way, so a dashboard PUT naming a Luna-firstcombo/shadowcan still return 400 even though the same file config now works. That is a management-surface fix and belongs in its own change.Verification
Remote CI at this PR's exact head SHA is the gate for this change.
Local checks: NOT RUN.
bun test,bun run test:changed,bun run typecheck,bun install,bun run build:gui,bun run lint:gui, andbun run privacy:scanwere all skipped by explicit maintainer instruction for this delivery round, which overrides the PR-ready gate inAGENTS.md. Nothing in this description claims a local check passed.Independent review that was done: a read-only reviewer audited the diff against the plan and confirmed the placement sits before
comboIdFromRawBodyinside!options.comboAttempt; thatresolveComboIdperforms no routing; thatsrc/lib/shadow-call.tsis untouched; thatchildLognever carriesshadowCallRewrittenFrom, so the successObject.assigninhandleComboResponsescannot clobber the marker; thatcomboFailureDecisionreturnshopfor 429 and 5xx; and that the extended test file is already explicit inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json, so no layout entry is added.Regression coverage added to
tests/responses/responses-shadow-intercept.test.ts:provider: "combo",routeKind: "combo", marker set, two attempt rows. Red before this change: one attempt.openai/gpt-5.6-lunastill routes as a combo and still records the marker, with exactly one upstream call. Red before this change:routeKind: "native"and no marker, because the collapsed pick read as a self-target.The existing self-target, prefix-log, and
gpt-5.6-terracases in that file are untouched and must stay green.Checklist
routeMayChangeCredentialDomainis already true for every combo attempt.Closes #4129.
Summary by CodeRabbit
New Features
Bug Fixes