fix(catalog): synthesize incomplete combo members with context fallback - #1163
fix(catalog): synthesize incomplete combo members with context fallback#1163eachann1024 wants to merge 2 commits into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCombo catalog resolution now synthesizes incomplete members from provider configuration, applies context fallbacks and caps, and excludes disabled or incompatible providers. Reasoning-effort intersections now treat undefined ladders as wildcards. ChangesCombo catalog resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComboDerivation
participant ProviderConfiguration
participant resolveComboCatalogMember
participant ComboCatalog
ComboDerivation->>ProviderConfiguration: Load enriched provider metadata
ComboDerivation->>resolveComboCatalogMember: Resolve combo targets
resolveComboCatalogMember->>ComboCatalog: Reuse or synthesize members
ComboDerivation->>ComboCatalog: Apply capabilities and omission tracking
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
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 `@docs-site/src/content/docs/reference/configuration/routing.md`:
- Around line 185-194: The combo eligibility documentation is missing the
advertised maxInputTokens source and incorrectly qualifies disabled providers by
discovery-row availability. Update
docs-site/src/content/docs/reference/configuration/routing.md lines 185-194 to
list maxInputTokens before the 128,000-token fallback, apply “no discovery row”
only to unknown providers, and state that disabled providers are always
excluded; make the equivalent corrections in
docs-site/src/content/docs/zh-cn/reference/configuration/routing.md lines 85-90,
adding 上游声明的 maxInputTokens before the 128,000 回退 and clarifying that 已禁用提供方 is
always excluded.
In `@tests/codex-catalog.test.ts`:
- Around line 843-852: Strengthen the assertion for the uncapped branch of
resolveComboCatalogMember by verifying that both contextCap and contextCapped
are absent, in addition to the existing contextWindow and maxInputTokens values.
Keep this coverage focused on the cappedContext !== uncappedContext behavior so
regressions that set cap flags without clamping fail the test.
🪄 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: 872e30d6-737b-411d-9402-5d3ba6fe47f5
📒 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
![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: bb96869edb
ℹ️ 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 (capabilityRecord?.vision === false) return ["text"]; | ||
| if (capabilityRecord?.vision === true || capabilities?.some(value => value === "vision" || value === "image-input")) { | ||
| return ["text", "image"]; |
There was a problem hiding this comment.
For providers that expose modality only through architecture.modality (the Nebius fixture has text+image->text), this now falls through to undefined unless a separate vision capability is present, so catalog rows are emitted as text-only and Codex blocks image attachments; the existing provider-model-discovery-contract and Nebius coverage still expect catalogHintsFromModelsApiItem to infer ['text','image'] from that bounded field. Please keep the enum filtering but restore the architecture parser before returning undefined.
AGENTS.md reference: AGENTS.md:L225-L227
Useful? React with 👍 / 👎.
| const uncappedContext = hintedContext | ||
| ?? knownMaxInput | ||
| ?? (existing || prov ? COMBO_MEMBER_CONTEXT_FALLBACK : undefined); |
There was a problem hiding this comment.
When a combo target is a row the catalog intentionally filtered out, such as opencode-go/hy3-preview from the exact compatibility exclusion or a media-generation id hidden by shouldExposeRoutedModel, memberByKey has no entry but prov is still present, so this fallback recreates the member with a 128k stub and the combo becomes picker-visible again. That bypasses the existing catalog exclusion path and lets users select combos whose target was already known to be uncallable or unsupported; check the same visibility/exclusion predicate before synthesizing a ghost member.
AGENTS.md reference: src/AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
Recover multi-target combos when discovery omits context windows by synthesizing members from provider config (or a 128k fallback clamped by providerContextCaps). Prefer known maxInputTokens over inventing 128k, skip disabled providers, and treat unknown effort ladders as wildcards in catalog derivation while empty ladders stay restrictive.
Rebuild resolveComboCatalogMember on current provider-fetch so the Nebius architecture.modality path is not regressed. Document maxInputTokens before the 128k fallback and that disabled providers never catalog. Pin contextCapped falsy when a high cap does not clamp.
6fec82a to
99c63db
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
|
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1374-1386: Update combo-member synthesis around
resolveComboCatalogMember to use a provider map containing every configured
provider, including disabled entries, while preferring the corresponding
enriched clone when available. Keep enrichedByName unchanged for custom-model
vision-sidecar inheritance, and ensure native OpenAI disabled status still
reflects the provider’s disabled flag so disabled-provider combo targets are
excluded.
🪄 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: 04405dbe-5e89-4f09-9864-c234d6a04655
📒 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
| // Enriched (registry-hydrated) provider clones — shared by combo member synthesis and | ||
| // custom-model vision-sidecar inheritance so both see the same merged registry view. | ||
| const enrichedByName = new Map(activeProviders.map(provider => [provider.name, provider.provider])); | ||
| for (const id of listComboIds(config)) { | ||
| const combo = getCombo(config, id); | ||
| if (!combo) continue; | ||
| const members = combo.targets | ||
| .map(target => memberByKey.get(targetKey(target))) | ||
| .map(target => resolveComboCatalogMember( | ||
| target, | ||
| memberByKey, | ||
| enrichedByName, | ||
| providerContextCap(config, target.provider), | ||
| )) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the disabled-provider signal reaching resolveComboCatalogMember.
set -euo pipefail
echo "=== disabledNativeSlugs definition (does it consider providers.openai.disabled?) ==="
ast-grep run --pattern 'export function disabledNativeSlugs($$$) { $$$ }' --lang typescript src
echo
echo "=== all call sites of resolveComboCatalogMember (which map is passed?) ==="
rg -nP -C 8 '\bresolveComboCatalogMember\s*\(' --type=ts
echo
echo "=== construction of capture.providers (confirm the disabled filter) ==="
rg -nP -C 4 'disabled\s*!==\s*true' --type=ts -g 'src/**'
echo
echo "=== any integration test that disables a provider AND configures a combo ==="
rg -nP -C 10 'disabled:\s*true' --type=ts -g 'tests/**' | rg -n -C 10 'combos'Repository: lidge-jun/opencodex
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== files containing resolveComboCatalogMember/disabledNativeSlugs ==="
rg -n 'resolveComboCatalogMember|disabledNativeSlugs|disabledNative|activeProviders|capture\.providers|disabledNativeSlugs' --type=ts -g 'src/**' -g 'tests/**'
echo
echo "=== provider-fetch relevant sections ==="
python3 - <<'PY'
from pathlib import Path
p = Path('src/codex/catalog/provider-fetch.ts')
lines = p.read_text().splitlines()
for start,end in [(1320,1405),(600,695)]:
print(f"\n--- {p}:{start}-{end} ---")
for i in range(start,end+1):
if i <= len(lines):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "=== tests likely relevant ==="
rg -n -C 12 'disabled:\s*true|openai|combos|resolveComboCatalogMember|catalog' tests --type=ts -g 'tests/**' | head -n 300Repository: lidge-jun/opencodex
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate provider-fetch file ==="
fd -a 'provider-fetch\.ts$' . || true
echo
echo "=== symbols across tracked TypeScript files ==="
git ls-files '*.ts' | xargs rg -n 'resolveComboCatalogMember|disabledNativeSlugs|activeProviders|capture\.providers|disabled\s*!==\s*true' || true
echo
echo "=== locate test files ==="
git ls-files 'tests/**/*.ts' | xargs rg -n -C 10 'disabled:\s*true|combos|catalog|resolveComboCatalogMember|enrichedByName|capture\.providers' || true
echo
echo "=== provider-fetch key ranges if present ==="
if [ -f src/codex/catalog/provider-fetch.ts ]; then
python3 - <<'PY'
from pathlib import Path
lines = Path('src/codex/catalog/provider-fetch.ts').read_text().splitlines()
for range in [(620,690),(1340,1400),(400,425)]:
print(f"\n--- ranges {range} ---")
for i in range(*range):
if i <= len(lines):
print(f"{i:5}: {lines[i-1]}")
PY
fiRepository: lidge-jun/opencodex
Length of output: 17252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== provider-fetch key ranges ==="
python3 - <<'PY'
from pathlib import Path
p = Path('src/codex/catalog/provider-fetch.ts')
lines = p.read_text().splitlines()
ranges = [(620,690),(1280,1415),(395,425)]
for start,end in ranges:
print(f"\n--- {p}:{start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "=== metadata definitions ==="
python3 - <<'PY'
from pathlib import Path
p = Path('src/codex/catalog/metadata.ts')
lines = p.read_text().splitlines()
ranges = [(90,170)]
for start,end in ranges:
print(f"\n--- {p}:{start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "=== unit tests around resolveComboCatalogMember ==="
python3 - <<'PY'
from pathlib import Path
p = Path('tests/codex-catalog.test.ts')
lines = p.read_text().splitlines()
ranges = [(740,835)]
for start,end in ranges:
print(f"\n--- {p}:{start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "=== native-model-toggle tests ==="
python3 - <<'PY'
from pathlib import Path
p = Path('tests/native-model-toggle.test.ts')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'disabledNativeSlugs' in line:
print(f"{i}: {line}")
PY
cat -n tests/native-model-toggle.test.ts | sed -n '1,90p'
echo
echo "=== exact resolveComboCatalogMember call sites ==="
rg -n -C 12 '\bresolveComboCatalogMember\s*\(' --type=ts -g 'src/codex/catalog/provider-fetch.ts' || true
echo
echo "=== exact activeProviders usage around catalog build ==="
rg -n -C 8 '\bactiveProviders\b' --type=ts -g 'src/codex/catalog/provider-fetch.ts' || true
echo
echo "=== deterministic behavior probe from source extraction ==="
python3 - <<'PY'
import re
from pathlib import Path
src = Path('src/codex/catalog/provider-fetch.ts')
txt = src.read_text()
m = re.search(r'buildCatalogEntries[\s\S]*?const enrichedByName = new Map\([\s\S]*?\);\s*for \(const id of listComboIds\([\s\S]*?resolveComboCatalogMember\([\s\S]*?\)\)\s*\]', txt)
print("buildCatalogEntries block extracted:", bool(m))
if m:
code = m.group(0)
print("includes activeProviders.map provider.disabled filter:", ".filter(([, provider]) => provider.disabled !== true)" in code or "provider.disabled !== true" in code.split("const activeProviders = capture.providers;")[-1].split("const enrichedByName")[0])
print("enrichedByName source:", [x.strip() for x in re.findall(r'const enrichedByName = new Map\([^;]+;\n', code)])
print("comboProviderView construction mentioned:", "comboProviderView" in code or "Object.entries(config.providers)" in code)
print("native injection call present:", "disabledNativeSlugs(config)" in code)
metadata = Path('src/codex/catalog/metadata.ts').read_text()
m2 = re.search(r'export function disabledNativeSlugs\([\s\S]*?\n\}', metadata)
print("\ndisabledNativeSlugs definition:")
print(m2.group(0) if m2 else "not found")
PYRepository: lidge-jun/opencodex
Length of output: 27361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== doc references ==="
rg -n -C 4 'disabled provider|disabled providers|catolog|catalog' docs-site/src/content/docs/reference/configuration/routing.md docs-site/src/content/docs/zh-cn/reference/configuration/routing.md 2>/dev/null || true
echo
echo "=== combo disable/route checks around providers.disabled ==="
rg -n -C 6 'target\.provider.*disabled|disabled.*target\.provider|combos.*disabled|disabled.*combos|disabledModels' src --type=ts -g 'src/combos/**' -g 'src/codex/**' -g 'src/router.ts' || true
echo
echo "=== unit tests: provider disabled with combos/native slugs ==="
python3 - <<'PY'
from pathlib import Path
text = Path('tests/codex-catalog.test.ts').read_text()
print('has disabled=true in provider map:', 'disabled: true' in text)
print('has openai provider in tests:', 'openai' in Path('tests/codex-catalog.test.ts').read_text())
print('has combos in tests:', 'combos' in text)
PYRepository: lidge-jun/opencodex
Length of output: 25622
Pass a combo provider view that keeps disabled providers visible.
src/codex/catalog/provider-fetch.ts:414-416 filters disabled providers before capture.providers is created, and enrichedByName is built from that filtered list at line 1376. In resolveComboCatalogMember, prov = providers.get(target.provider) can only be true-disabled when the input map contains the provider entry, so a disabled provider is treated as missing rather than explicitly disabled. Native OpenAI rows are still synthesized from disabledNativeSlugs(config) alone, which only checks config.disabledModels, so openai/<native-slug> can satisfy the complete-row branch despite providers.openai.disabled = true. This contradicts the documented behavior: docs-site/src/content/docs/reference/configuration/routing.md:191-192 removes combos when a target is on a disabled provider even with a complete discovery row.
Fix the combo-member lookup by passing the full configured provider map, preferring enriched clones where present, without changing the custom-model inheritance that needs enrichedByName.
🧰 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 1374 - 1386, Update
combo-member synthesis around resolveComboCatalogMember to use a provider map
containing every configured provider, including disabled entries, while
preferring the corresponding enriched clone when available. Keep enrichedByName
unchanged for custom-model vision-sidecar inheritance, and ensure native OpenAI
disabled status still reflects the provider’s disabled flag so disabled-provider
combo targets are excluded.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99c63dbfd6
ℹ️ 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".
| - a positive `contextWindow`, from live metadata, registry hints, provider | ||
| `modelContextWindows` / `contextWindow`, a known positive `maxInputTokens` on the member row, | ||
| or — when the provider is known and enabled but every source still omits a window — a | ||
| conservative 128,000-token fallback (clamped by `providerContextCaps` when set); and |
There was a problem hiding this comment.
This updates only the English and zh-cn routing references with the new combo catalog fallback, but the ru, ja, and ko versions of the same section still say combos are omitted when context metadata is absent (for example docs-site/src/content/docs/ru/reference/configuration/routing.md:93-100, .../ja/...:82-86, and .../ko/...:81-84). That leaves non-English users with the old eligibility rule and contradicts this new documented behavior, so the remaining locale pages should be updated or made non-contradictory.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| const capped = applyProviderContextCap(existing.contextWindow, contextCap); | ||
| if (capped === undefined || capped === existing.contextWindow) return existing; |
There was a problem hiding this comment.
When a combo target is the canonical openai provider and that provider is disabled, activeProviders omits the provider from enrichedByName, but the earlier native-slug injection still puts an openai/<slug> entry in memberByKey; this branch then returns that existing member because prov is undefined. In that configuration the combo is still derived and picker-visible even though this change is meant to omit targets on disabled providers, so pass disabled provider state into the resolver or skip native injection when providers.openai.disabled is true.
Useful? React with 👍 / 👎.
| .map(target => resolveComboCatalogMember( | ||
| target, | ||
| memberByKey, | ||
| enrichedByName, | ||
| providerContextCap(config, target.provider), |
There was a problem hiding this comment.
When a combo target is only registered through config.customModels (for example added with a smaller --context-window) and the provider does not also expose that model in discovery or provider hints, this loop resolves the combo before custom rows are merged and the resolver falls back to the hard-coded 128k text member. That makes the combo picker-visible with capabilities that contradict the custom model row and can let Codex send requests above the configured custom context, so include custom model metadata in member resolution before applying the 128k fallback.
Useful? React with 👍 / 👎.
1 participant
Summary
contextWindowby synthesizing members from provider config (or a conservative 128k fallback).providerContextCaps; prefer knownmaxInputTokensover inventing 128k.undefined) as wildcards in catalog derivation; empty ladders stay restrictive.Split from #1092 (maintainer asked for focused PRs after effort-picker work moved to #1151).
Change graph
Verification
bun test tests/codex-catalog.test.ts— 123 passbun run typecheck— exit 0git diff --check— cleanChecklist
Related: #1092, #1151
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
New Features
Improvements