fix(catalog): apply providerContextCaps.openai to native OpenAI rows - #1435
Conversation
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. CodeRabbit has 1 unresolved finding; the Codex/CodeRabbit findings box has been unticked. |
📝 WalkthroughWalkthroughNative OpenAI provider context caps now constrain native model metadata, catalog construction and synchronization, and routing capability evidence. GPT-5.6 uses shared API context metadata, with regression coverage for capped and uncapped behavior. ChangesNative OpenAI context-cap handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 |
|
I found one remaining Codex-facing path that still bypasses the OpenAI provider cap. In I reproduced this on the PR head with Please pass the cap through this call and add an integration test in |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Request changes based on a full review of the current head.
Merge blocker:
The PR claims providerContextCaps.openai is now a ceiling for native OpenAI rows, but the persisted-catalog normalizer still returns early unless the native slug has an entry in NATIVE_OPENAI_CONTEXT_OVERRIDES:
const override = NATIVE_OPENAI_CONTEXT_OVERRIDES[nativeSlug]; if (!override) return;
That means supported native rows without a hardcoded override can still bypass the cap. gpt-5.4-mini is in NATIVE_OPENAI_MODELS but has no NATIVE_OPENAI_CONTEXT_OVERRIDES entry, so a genuine/preserved native gpt-5.4-mini row can remain uncapped even though runtime/synthetic metadata paths use nativeOpenAiContextWindow(..., cap) and are capped. This leaves cross-surface catalog/routing disagreement.
Please decouple hardcoded native overrides from provider-cap application: first apply an override if one exists, then apply the OpenAI provider cap to the resulting known context_window / max_context_window values regardless of where those values came from, and recompute auto-compaction from the capped context where applicable.
Add a regression for a preserved genuine gpt-5.4-mini native row with an OpenAI cap lower than its existing context window, verifying context/max-context/auto-compact all reflect the cap.
The GPT-5.6 paths added by this PR otherwise look correct and I did not find a security issue. This PR also overlaps newer dev work in central catalog files, so after the fix it should be refreshed onto current dev and rerun with exact-head CI.
2e6099b to
16e6342
Compare
|
Thanks for the detailed review. Both points are addressed on the refreshed head 1. Hardcoded overrides and the provider cap are decoupled
2. Regression for preserved Added in 3. Refresh onto current dev and verification
No security issue was introduced; the GPT-5.6 paths are untouched by this revision. |
|
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/catalog/sync.ts (1)
363-377: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPass the OpenAI context cap to Codex discovery.
At
src/server/index.ts:914, passproviderContextCap(config, OPENAI_CODEX_PROVIDER_ID)as the final argument tobuildCatalogEntries. Otherwise,/v1/models?client_version=...returns uncapped nativecontext_windowandmax_context_windowvalues. Add a regression test intests/claude-models-discovery.test.tsfor the capped response.🤖 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/sync.ts` around lines 363 - 377, Update the Codex discovery call in src/server/index.ts around buildCatalogEntries to pass providerContextCap(config, OPENAI_CODEX_PROVIDER_ID) as its final argument. Ensure the capped value is reflected in native context_window and max_context_window responses, and add a regression test in claude-models-discovery.test.ts covering the capped response.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/codex/catalog/sync.ts`:
- Around line 363-377: Update the Codex discovery call in src/server/index.ts
around buildCatalogEntries to pass providerContextCap(config,
OPENAI_CODEX_PROVIDER_ID) as its final argument. Ensure the capped value is
reflected in native context_window and max_context_window responses, and add a
regression test in claude-models-discovery.test.ts covering the capped response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7523212-be04-47c7-a733-8833c94c7b92
📒 Files selected for processing (4)
src/codex/catalog/parsing.tssrc/codex/catalog/sync.tssrc/providers/registry.tstests/codex-catalog.test.ts
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The refreshed head fixes the earlier preserved-row blocker, but one merge blocker remains on exact head 16e6342b.
src/server/index.ts:914 serves the live Codex discovery route (GET /v1/models?client_version=...) by calling buildCatalogEntries(...) without the new OpenAI context-cap argument. With providerContextCaps.openai = 272000, that route can therefore still emit native GPT-5.6 rows at 372000 / 372000 / 334800, while the synchronized catalog and routing evidence are capped. This leaves the user-visible runtime surface inconsistent with the paths this PR fixed.
Please pass providerContextCap(config, OPENAI_CODEX_PROVIDER_ID) through this call and add an endpoint-level regression in tests/claude-models-discovery.test.ts (or the nearest existing server discovery suite) that asserts the live response is 272000 / 272000 / 244800 for a native GPT-5.6 row.
The branch is also five commits behind current dev (7779c057), so refresh it and run exact-head cross-platform CI after the endpoint fix. The rest of the revised cap propagation, including the preserved gpt-5.4-mini case, looks sound.
16e6342 to
1a31c7b
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changed files at PR head =='
git diff --name-status HEAD^ HEAD
printf '%s\n' '== src/server/index.ts outline =='
ast-grep outline src/server/index.ts --items all --match 'models|Catalog|providerContextCap|OPENAI_CODEX_PROVIDER_ID' || true
printf '%s\n' '== buildCatalogEntries call sites and cap-related imports =='
rg -n -C 5 '\bbuildCatalogEntries\s*\(|\bproviderContextCap\s*\(|\bOPENAI_CODEX_PROVIDER_ID\b' src/server/index.ts src tests
printf '%s\n' '== Discovery test outline and provider-cap references =='
ast-grep outline tests/claude-models-discovery.test.ts --items all || true
rg -n -C 6 'providerContextCaps|context_window|max_context_window|auto_compact_token_limit|/v1/models|client_version|gpt-5\.6' tests/claude-models-discovery.test.ts
printf '%s\n' '== Relevant PR diff =='
git diff HEAD^ HEAD -- src/server/index.ts tests/claude-models-discovery.test.tsLength of output: 169447
This covers the previously uncapped discovery path. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Native OpenAI catalog rows ignored providerContextCaps.openai: the fixed native context overrides in metadata.ts stayed at 372k while routed models were capped. Thread the openai provider cap through the catalog entry builders (finishUpstreamNativeEntry, deriveEntry, buildCatalogEntries, mergeCatalogEntriesForSync), the runtime native metadata accessors (nativeOpenAiContextWindow, nativeModelRows), and routing capability evidence so the catalog, management rows, and proxy routing agree. Preserved native rows without a hardcoded override (e.g. gpt-5.4-mini) are capped the same way, and auto-compaction follows the capped window. The 372k native value remains the default when no cap is configured. Closes lidge-jun#1430
1a31c7b to
e266f79
Compare
|
|
|
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/registry.ts (1)
322-323: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the Daybreak endpoint-support note.
At
src/providers/registry.ts:322-323, state that OpenAI lists bothv1/chat/completionsandv1/responsesfordaybreak-red-latestanddaybreak-blue-latest. If OpenCodex intentionally routes these aliases throughopenai-responses, describe that local routing policy instead of claiming that Chat Completions is unsupported.🤖 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/providers/registry.ts` around lines 322 - 323, Update the Daybreak endpoint-support note in the provider registry to state that OpenAI lists both v1/chat/completions and v1/responses for daybreak-red-latest and daybreak-blue-latest; if the aliases are locally routed through openai-responses, document that routing policy rather than claiming Chat Completions is unsupported.Sources: Path instructions, MCP tools
🤖 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.
Outside diff comments:
In `@src/providers/registry.ts`:
- Around line 322-323: Update the Daybreak endpoint-support note in the provider
registry to state that OpenAI lists both v1/chat/completions and v1/responses
for daybreak-red-latest and daybreak-blue-latest; if the aliases are locally
routed through openai-responses, document that routing policy rather than
claiming Chat Completions is unsupported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f567e087-7e1e-437b-ad8c-ce3ac5612371
📒 Files selected for processing (2)
src/providers/registry.tstests/codex-catalog.test.ts
Addressed on later commits: live Codex discovery (GET /v1/models?client_version=...) now threads providerContextCaps.openai through buildCatalogEntries, with regression coverage for capped discovery rows.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Approving. Cap application is decoupled from hardcoded native overrides, preserved rows without overrides are capped, live discovery matches catalog/routing metadata, and the no-cap 372k default is unchanged.
|
Thanks @Yuxin-Qiao — this was useful because Merging now. |
4 participants
Summary
providerContextCaps.openaiis now a ceiling for native OpenAI catalog rows, the live Codex discovery endpoint, management model rows, and routing capability evidence.context_window/max_context_windowvalues are capped regardless of origin. Auto-compaction is recomputed from the capped window.gpt-5.4-mini, now receive the same OpenAI cap. With no cap configured, the existing 372k native default is unchanged.Closes #1430
Verification
bun run typecheckpasses.bun run privacy:scanpasses;git diff --checkis clean.tests/codex-catalog.test.ts,tests/claude-models-discovery.test.ts,tests/native-model-toggle.test.ts,tests/route-explainability.test.ts,tests/grok-sync.test.ts,tests/claude-context-windows.test.ts, andtests/claude-desktop-native-context.test.ts.GET /v1/models?client_version=1.0.0and asserts native GPT-5.6 output of272000 / 272000 / 244800; catalog, preserved-row, runtime, and routing regressions remain covered.devheade2f7f2ba; the pushed PR head ise266f799.Checklist
providerContextCapsdocumentation covers this behavior).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.