Repository navigation
fix(combo): apply the combo default effort the target can actually reach - #3172
Merged
Merged
Conversation
A combo configured for max routed to a target whose ladder tops out lower sent no effort at all: concreteComboRequestBody tested literal membership and dropped the default on a miss, so the provider default applied and the turn ran at none. The catalog never agreed with that. effectiveComboDefault advertises the highest supported rung at or below the request, so the served catalog promised max while the request path silently sent nothing. Share one resolver instead. It moves to src/reasoning-effort.ts, a leaf whose only import is ./types and which already owns codexEffortRank, so the request path can use it without importing the catalog plane - aggregation.ts pulls node:child_process, oauth, model-cache and cursor live-models, and it already imports src/combos, so a direct import would also have closed a cycle. Unknown ladders stay fail-closed and an explicitly empty ladder still means no reasoning. Neither behavior changes. Closes #3108
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. |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds shared reasoning-effort resolution. Combo defaults now select the highest supported tier at or below the request and pass the resolved tier into target requests. Tests cover fallback, exact matches, caller overrides, and partial reasoning objects. ChangesCombo reasoning effort
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ComboCaller
participant concreteComboRequestBody
participant resolveEffortAtOrBelow
participant TargetModel
ComboCaller->>concreteComboRequestBody: submit request with optional effort
concreteComboRequestBody->>resolveEffortAtOrBelow: resolve combo default against target tiers
resolveEffortAtOrBelow-->>concreteComboRequestBody: supported effort
concreteComboRequestBody->>TargetModel: send reasoning effort
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
4 tasks done
tarunravi
pushed a commit
to tarunravi/opencodex
that referenced
this pull request
Sep 14, 2026
…ach (lidge-jun#3172) A combo configured for max routed to a target whose ladder tops out lower sent no effort at all: concreteComboRequestBody tested literal membership and dropped the default on a miss, so the provider default applied and the turn ran at none. The catalog never agreed with that. effectiveComboDefault advertises the highest supported rung at or below the request, so the served catalog promised max while the request path silently sent nothing. Share one resolver instead. It moves to src/reasoning-effort.ts, a leaf whose only import is ./types and which already owns codexEffortRank, so the request path can use it without importing the catalog plane - aggregation.ts pulls node:child_process, oauth, model-cache and cursor live-models, and it already imports src/combos, so a direct import would also have closed a cycle. Unknown ladders stay fail-closed and an explicitly empty ladder still means no reasoning. Neither behavior changes. Closes lidge-jun#3108 Co-authored-by: jun <jun@lidge.dev>
tarunravi
pushed a commit
to tarunravi/opencodex
that referenced
this pull request
Sep 14, 2026
…ated (lidge-jun#3175) The case expected the first target to receive no reasoning_effort, which is the behavior lidge-jun#3172 removed. That combo's members advertise low and low+high, so the catalog advertises low, and low is now what the first target receives. Refs lidge-jun#3108 Co-authored-by: jun <jun@lidge.dev>
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…ach (lidge-jun#3172) A combo configured for max routed to a target whose ladder tops out lower sent no effort at all: concreteComboRequestBody tested literal membership and dropped the default on a miss, so the provider default applied and the turn ran at none. The catalog never agreed with that. effectiveComboDefault advertises the highest supported rung at or below the request, so the served catalog promised max while the request path silently sent nothing. Share one resolver instead. It moves to src/reasoning-effort.ts, a leaf whose only import is ./types and which already owns codexEffortRank, so the request path can use it without importing the catalog plane - aggregation.ts pulls node:child_process, oauth, model-cache and cursor live-models, and it already imports src/combos, so a direct import would also have closed a cycle. Unknown ladders stay fail-closed and an explicitly empty ladder still means no reasoning. Neither behavior changes. Closes lidge-jun#3108 Co-authored-by: jun <jun@lidge.dev>
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…ated (lidge-jun#3175) The case expected the first target to receive no reasoning_effort, which is the behavior lidge-jun#3172 removed. That combo's members advertise low and low+high, so the catalog advertises low, and low is now what the first target receives. Refs lidge-jun#3108 Co-authored-by: jun <jun@lidge.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A combo with a default reasoning level of
maxrouted todeepseek-v4-prosentnone, while selecting the same model directly withmaxsentmax.concreteComboRequestBodytested literal membership —targetReasoningEfforts?.includes(defaultEffort)— and dropped the default on a miss. With nothing on the wire the provider's own default applied, which reads asnone.The catalog never agreed with that.
effectiveComboDefault(src/codex/catalog/aggregation.ts) resolves a combo's advertised default to the highest supported rung at or below the request rather than dropping it, so the served catalog promisedmaxfor a combo whose requests were silently carrying no effort at all.The fix is one shared resolver instead of two disagreeing rules.
resolveEffortAtOrBelowmoves intosrc/reasoning-effort.tsand both planes call it.Why that file. The obvious move — importing
effectiveComboDefaultstraight fromaggregation.ts— is not available.aggregation.tsalready importssrc/combos, so the reverse import closes a cycle; and it pullsnode:child_process,src/oauth,src/codex/model-cache, andsrc/adapters/cursor/live-models, all of which would land on the request path throughsrc/server/responses/core.ts → src/combos.reasoning-effort.tsis a genuine leaf (its only import is./types) and already ownscodexEffortRank, the exact primitive the resolver needs.Two behaviors deliberately do not change: an unknown ladder (
undefined) stays fail-closed, and an explicitly empty ladder still means the model takes no reasoning effort.Closes #3108.
Verification
Exact head
86dddcf5b:bun test tests/combos.test.ts— 44 pass, 0 fail, 229 expect() calls.bun test tests/core-lab-boundary.test.ts tests/combo-management-api.test.ts— 45 pass, 0 fail. The boundary suite is the one that would catch the import mistake described above.bun test tests/codex-catalog.test.ts— 255 pass, 0 fail, confirming the catalog plane behaves identically aftereffectiveComboDefaultbecame a delegation.includes()check turns the new case red (43 pass / 1 fail) and the fix returns it to green.One existing assertion changed rather than being added to:
concreteComboRequestBody(..., "high", ["low", "medium"])previously expectedundefined. That assertion was pinning the defect — it is the exact shape #3108 reports — so it now expects{ effort: "medium" }. The unset, empty-ladder, and unknown-ladder cases in that test are untouched.Full-suite and typecheck coverage is left to CI on this exact head.
Checklist
devtests/core-lab-boundary.test.tsgreenSummary by CodeRabbit
New Features
Bug Fixes