fix(catalog): synthesize incomplete combo members with context fallback (#1163) - #1305
Merged
Merged
Conversation
A combo member whose provider row is incomplete — missing a context window, or absent from the fetched catalog — was dropped from the generated Codex catalog rather than synthesized. `resolveComboCatalogMember` fills the gap from configuration, falling back to the provider's declared max input and then to 128k, so a combo stays selectable when one member's upstream row is thin. Republished from #1163 by 关俊江, whose branch was 366 commits behind dev. Two conflicts, both mechanical and both on a single line: `dev` renamed `augmentRoutedModelsWithJawcodeMetadata` to `augmentRoutedModelsWithMetadata` and added `CODEX_ACCOUNT_BOUND_CATALOG_KIND` plus a `catalog/parsing` import block, while this branch added `resolveComboCatalogMember` to the same export and import lines. Resolved by keeping every symbol from both sides; no behavior was re-decided. Co-authored-by: 关俊江 <each1024@qq.com>
7 tasks
Contributor
📝 WalkthroughWalkthroughCombo catalog resolution now synthesizes incomplete members from provider metadata, applies context caps and a 128,000-token fallback, and preserves explicit reasoning constraints. Documentation and tests cover eligibility, omission, and recovery behavior. ChangesCombo catalog recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CatalogAssembly
participant resolveComboCatalogMember
participant ProviderConfiguration
participant DiscoveredModels
CatalogAssembly->>resolveComboCatalogMember: Resolve each combo target
resolveComboCatalogMember->>DiscoveredModels: Read discovered member
resolveComboCatalogMember->>ProviderConfiguration: Read provider metadata and context caps
ProviderConfiguration-->>resolveComboCatalogMember: Return context and capability hints
resolveComboCatalogMember-->>CatalogAssembly: Return normalized member or omission reason
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 674-719: Rename usedFallback to reflect that it tracks whether
context was derived from a non-context-window value, including knownMaxInput,
rather than specifically the 128k fallback; update its use in fallbackCapped
while preserving the existing behavior and conditions.
In `@tests/codex-catalog.test.ts`:
- Around line 913-915: Update the comment above the hidden fixture to state that
the secret in the provider name must still be redacted in the omission warning,
matching the warningSentinel placement in targets[].provider while retaining the
unknown-provider behavior description.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 893c5a71-9589-43ca-a5c8-00443fcc3352
📒 Files selected for processing (6)
docs-site/src/content/docs/reference/configuration/routing.mddocs-site/src/content/docs/zh-cn/reference/configuration/routing.mdsrc/codex/catalog.tssrc/codex/catalog/aggregation.tssrc/codex/catalog/provider-fetch.tstests/codex-catalog.test.ts
Comment on lines
+674
to
+719
| const base: CatalogModel = existing ?? { | ||
| id: target.model, | ||
| provider: target.provider, | ||
| }; | ||
| const hinted = prov | ||
| ? applyProviderConfigHints(target.provider, prov, base, contextCap) | ||
| : base; | ||
| const hintedContext = typeof hinted.contextWindow === "number" && hinted.contextWindow > 0 | ||
| ? hinted.contextWindow | ||
| : undefined; | ||
| // Prefer a known positive maxInputTokens over inventing 128k when discovery | ||
| // advertised an input limit but no context window (common thin /models rows). | ||
| const knownMaxInput = typeof hinted.maxInputTokens === "number" && hinted.maxInputTokens > 0 | ||
| ? hinted.maxInputTokens | ||
| : (typeof base.maxInputTokens === "number" && base.maxInputTokens > 0 | ||
| ? base.maxInputTokens | ||
| : undefined); | ||
| const uncappedContext = hintedContext | ||
| ?? knownMaxInput | ||
| ?? (existing || prov ? COMBO_MEMBER_CONTEXT_FALLBACK : undefined); | ||
| if (uncappedContext === undefined) return undefined; | ||
| const usedFallback = hintedContext === undefined; | ||
| const cappedContext = applyProviderContextCap(uncappedContext, contextCap); | ||
| const contextWindow = cappedContext ?? uncappedContext; | ||
| const fallbackCapped = usedFallback | ||
| && contextCap !== undefined | ||
| && cappedContext !== undefined | ||
| && cappedContext !== uncappedContext; | ||
|
|
||
| const inputModalities = hinted.inputModalities ?? base.inputModalities ?? ["text"]; | ||
| const reasoningEfforts = hinted.reasoningEfforts | ||
| ?? (prov ? configuredReasoningEfforts(prov, target.model) : undefined) | ||
| ?? base.reasoningEfforts; | ||
| const maxInputTokens = knownMaxInput !== undefined | ||
| ? Math.min(knownMaxInput, contextWindow) | ||
| : contextWindow; | ||
|
|
||
| return { | ||
| ...hinted, | ||
| inputModalities, | ||
| ...(reasoningEfforts !== undefined ? { reasoningEfforts } : {}), | ||
| contextWindow, | ||
| maxInputTokens, | ||
| ...(fallbackCapped ? { contextCap, contextCapped: true as const } : {}), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider naming usedFallback for what it actually measures.
Line 695 sets usedFallback whenever hintedContext is undefined. That includes the knownMaxInput branch at line 692, which is not the 128,000-token fallback. The behavior is still correct, because fallbackCapped at lines 698-701 additionally requires that the cap changed the value. Only the name is misleading for the next reader who touches this block.
♻️ Suggested rename
- const usedFallback = hintedContext === undefined;
+ // True whenever the window came from maxInputTokens or the 128k fallback
+ // rather than from a hinted/discovered contextWindow.
+ const derivedContext = hintedContext === undefined;
const cappedContext = applyProviderContextCap(uncappedContext, contextCap);
const contextWindow = cappedContext ?? uncappedContext;
- const fallbackCapped = usedFallback
+ const fallbackCapped = derivedContext
&& contextCap !== undefined
&& cappedContext !== undefined
&& cappedContext !== uncappedContext;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const base: CatalogModel = existing ?? { | |
| id: target.model, | |
| provider: target.provider, | |
| }; | |
| const hinted = prov | |
| ? applyProviderConfigHints(target.provider, prov, base, contextCap) | |
| : base; | |
| const hintedContext = typeof hinted.contextWindow === "number" && hinted.contextWindow > 0 | |
| ? hinted.contextWindow | |
| : undefined; | |
| // Prefer a known positive maxInputTokens over inventing 128k when discovery | |
| // advertised an input limit but no context window (common thin /models rows). | |
| const knownMaxInput = typeof hinted.maxInputTokens === "number" && hinted.maxInputTokens > 0 | |
| ? hinted.maxInputTokens | |
| : (typeof base.maxInputTokens === "number" && base.maxInputTokens > 0 | |
| ? base.maxInputTokens | |
| : undefined); | |
| const uncappedContext = hintedContext | |
| ?? knownMaxInput | |
| ?? (existing || prov ? COMBO_MEMBER_CONTEXT_FALLBACK : undefined); | |
| if (uncappedContext === undefined) return undefined; | |
| const usedFallback = hintedContext === undefined; | |
| const cappedContext = applyProviderContextCap(uncappedContext, contextCap); | |
| const contextWindow = cappedContext ?? uncappedContext; | |
| const fallbackCapped = usedFallback | |
| && contextCap !== undefined | |
| && cappedContext !== undefined | |
| && cappedContext !== uncappedContext; | |
| const inputModalities = hinted.inputModalities ?? base.inputModalities ?? ["text"]; | |
| const reasoningEfforts = hinted.reasoningEfforts | |
| ?? (prov ? configuredReasoningEfforts(prov, target.model) : undefined) | |
| ?? base.reasoningEfforts; | |
| const maxInputTokens = knownMaxInput !== undefined | |
| ? Math.min(knownMaxInput, contextWindow) | |
| : contextWindow; | |
| return { | |
| ...hinted, | |
| inputModalities, | |
| ...(reasoningEfforts !== undefined ? { reasoningEfforts } : {}), | |
| contextWindow, | |
| maxInputTokens, | |
| ...(fallbackCapped ? { contextCap, contextCapped: true as const } : {}), | |
| }; | |
| } | |
| const base: CatalogModel = existing ?? { | |
| id: target.model, | |
| provider: target.provider, | |
| }; | |
| const hinted = prov | |
| ? applyProviderConfigHints(target.provider, prov, base, contextCap) | |
| : base; | |
| const hintedContext = typeof hinted.contextWindow === "number" && hinted.contextWindow > 0 | |
| ? hinted.contextWindow | |
| : undefined; | |
| // Prefer a known positive maxInputTokens over inventing 128k when discovery | |
| // advertised an input limit but no context window (common thin /models rows). | |
| const knownMaxInput = typeof hinted.maxInputTokens === "number" && hinted.maxInputTokens > 0 | |
| ? hinted.maxInputTokens | |
| : (typeof base.maxInputTokens === "number" && base.maxInputTokens > 0 | |
| ? base.maxInputTokens | |
| : undefined); | |
| const uncappedContext = hintedContext | |
| ?? knownMaxInput | |
| ?? (existing || prov ? COMBO_MEMBER_CONTEXT_FALLBACK : undefined); | |
| if (uncappedContext === undefined) return undefined; | |
| // True whenever the window came from maxInputTokens or the 128k fallback | |
| // rather than from a hinted/discovered contextWindow. | |
| const derivedContext = hintedContext === undefined; | |
| const cappedContext = applyProviderContextCap(uncappedContext, contextCap); | |
| const contextWindow = cappedContext ?? uncappedContext; | |
| const fallbackCapped = derivedContext | |
| && contextCap !== undefined | |
| && cappedContext !== undefined | |
| && cappedContext !== uncappedContext; | |
| const inputModalities = hinted.inputModalities ?? base.inputModalities ?? ["text"]; | |
| const reasoningEfforts = hinted.reasoningEfforts | |
| ?? (prov ? configuredReasoningEfforts(prov, target.model) : undefined) | |
| ?? base.reasoningEfforts; | |
| const maxInputTokens = knownMaxInput !== undefined | |
| ? Math.min(knownMaxInput, contextWindow) | |
| : contextWindow; | |
| return { | |
| ...hinted, | |
| inputModalities, | |
| ...(reasoningEfforts !== undefined ? { reasoningEfforts } : {}), | |
| contextWindow, | |
| maxInputTokens, | |
| ...(fallbackCapped ? { contextCap, contextCapped: true as const } : {}), | |
| }; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/catalog/provider-fetch.ts` around lines 674 - 719, Rename
usedFallback to reflect that it tracks whether context was derived from a
non-context-window value, including knownMaxInput, rather than specifically the
128k fallback; update its use in fallbackCapped while preserving the existing
behavior and conditions.
Comment on lines
+913
to
+915
| // Unknown provider (not just unlisted model) — synthesis cannot invent a member, | ||
| // and the secret in the model id must still be redacted in the omission warning. | ||
| hidden: { targets: [{ provider: warningSentinel, model: "m1" }] }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Update the comment to match the moved sentinel.
The fixture now puts warningSentinel in the provider name. The comment on line 914 still says "the secret in the model id must still be redacted". The model id is the literal "m1". A reader who trusts the comment will look for redaction of the wrong field.
📝 Suggested comment fix
// Unknown provider (not just unlisted model) — synthesis cannot invent a member,
- // and the secret in the model id must still be redacted in the omission warning.
+ // and the secret in the provider name must still be redacted in the omission warning.
hidden: { targets: [{ provider: warningSentinel, model: "m1" }] },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Unknown provider (not just unlisted model) — synthesis cannot invent a member, | |
| // and the secret in the model id must still be redacted in the omission warning. | |
| hidden: { targets: [{ provider: warningSentinel, model: "m1" }] }, | |
| // Unknown provider (not just unlisted model) — synthesis cannot invent a member, | |
| // and the secret in the provider name must still be redacted in the omission warning. | |
| hidden: { targets: [{ provider: warningSentinel, model: "m1" }] }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-catalog.test.ts` around lines 913 - 915, Update the comment above
the hidden fixture to state that the secret in the provider name must still be
redacted in the omission warning, matching the warningSentinel placement in
targets[].provider while retaining the unknown-provider behavior description.
![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: 9e5002c426
ℹ️ 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".
| if (!combo) continue; | ||
| const members = combo.targets | ||
| .map(target => memberByKey.get(targetKey(target))) | ||
| .map(target => resolveComboCatalogMember( |
There was a problem hiding this comment.
When a combo target was deliberately removed from all by the preceding shouldExposeRoutedModel filter—such as opencode-go/hy3-preview, which is explicitly excluded because the provider rejects it, or a standalone image/video-generation model—the empty lookup now causes resolveComboCatalogMember to synthesize that target from the enabled provider configuration. The resulting combo is advertised through /v1/models and can route requests to an uncallable or non-chat target, undoing the catalog's eligibility choke point; reject synthesized members that fail shouldExposeRoutedModel before deriving the combo.
Useful? React with 👍 / 👎.
This was referenced Aug 8, 2026
lidge-jun
added a commit
that referenced
this pull request
Aug 9, 2026
… closed (#1322) Two of the four contributor-held PRs resolved. @Wibias met every condition on #1244 -- rebase, Russian locale parity, two completed non-cancelled CI runs at the same SHA -- including resolving the conflict I created by merging #1305. Verified independently: two CI successes at d5e70a2 and a local full suite of 10120 pass / 0 fail on that head. Landed as c75e68e, 58 files. #241 closed with its chain named, since #1244 references #1056 rather than #241. #1301 rebased again after drifting 33 behind, then merged with --admin over a red test 1/4. The failure is a 5000ms timeout in tests/crash-guard.test.ts, which my one-file diff to tests/ci-workflows.test.ts cannot reach, and which passes 14/14 locally three times. Logged as MERGE-DESPITE-CI with the reason rather than as a clean green -- and recorded that this is the same shape of reasoning I criticised as 'rerun until green' earlier. #1272 hit a tenth #1302 occurrence and was rerun but NOT merged: it is still a draft and those boxes are the contributor's attestation. A broken CI is not a reason to tick them -- it is a reason not to, since they assert what CI can no longer confirm. Method note: rerun-failed-jobs overwrites the job log, so I destroyed the #1302 evidence by fetching it after the rerun. Capture first, then rerun.
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
1 participant
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
Republishes @eachann1024's #1163 on current
dev. Their branch was 366 commits behind andCONFLICTING, so this is a maintainer rebase; the commit carries aCo-authored-bytrailer and the logic is theirs.A combo member whose provider row is incomplete — missing a context window, or absent from the fetched catalog — was dropped from the generated Codex catalog rather than synthesized, which silently removes a combo the user configured.
resolveComboCatalogMemberfills the gap from configuration, falling back to the provider's declared max input and then to 128k, so the combo stays selectable when one member's upstream row is thin.The conflicts, and why the rebase is mechanical
git applyrefused the net diff, which usually signals a semantic rebase. It was not one — the merge produced exactly two conflicts, both a single line, both the same cause:devsidesrc/codex/catalog.tsaugmentRoutedModelsWithJawcodeMetadata→augmentRoutedModelsWithMetadataresolveComboCatalogMemberto the same export linetests/codex-catalog.test.tsCODEX_ACCOUNT_BOUND_CATALOG_KINDand acatalog/parsingimport blockresolveComboCatalogMemberto the same import lineResolved by keeping every symbol from both sides. No behavior was re-decided and no rename was reverted;
rg augmentRoutedModelsWithJawcodeMetadatareturns nothing.Supersedes #1163, which can be closed once this lands.
Verification
Rebased onto
14e948525and verified after the rebase, on the committed tree rather than a working copy:bun run test— 10013 pass / 7 skip / 0 fail across 626 filesbun test tests/codex-catalog.test.ts— 159 pass / 0 failbun run typecheck— cleanbun run privacy:scan— passedgit diff --cached --check— clean (staged-tree whitespace check, run before the commit; the suite results above were then run on the committed tree)The contributor's coverage survives the rebase and is what carries the feature:
tests/codex-catalog.test.tsexercises configuration synthesis, incomplete rows, max-input fallback, the 128k fallback, context caps, disabled providers, and hard-failure cases againstresolveComboCatalogMember(provider-fetch.ts:641, called for combo members at:1427).Checklist
Note on CI: Linux shards are currently hanging intermittently and reporting
cancelled(#1302). If a shard here comes back cancelled rather than failed, that is the tracked infrastructure problem and not this diff — I will rerun and record it rather than describing it as flake.Summary by CodeRabbit