fix(catalog): apply providerContextCaps.openai to native OpenAI rows - #1435
fix(catalog): apply providerContextCaps.openai to native OpenAI rows#1435Yuxin-Qiao wants to merge 2 commits into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
📝 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 reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
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
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. |
4 participants
Summary
providerContextCaps.openaiis now a ceiling for native OpenAI catalog rows, not just routed/provider-discovered models. The live Codex discovery endpoint (/v1/models?client_version=...) now receives the same cap, keeping endpoint responses consistent with synchronized catalog and routing metadata. Previously the fixed native overrides kept GPT-5.6 Sol/Terra/Luna at 372k even when the OpenAI provider cap was set to 272k (providerContextCaps.openai is ignored for native GPT-5.6 catalog rows #1430).finishUpstreamNativeEntry,deriveEntry,buildCatalogEntries,mergeCatalogEntriesForSync), the runtime native metadata accessors (nativeOpenAiContextWindow,nativeModelRows), and routing capability evidence (candidateCapabilityEvidence), so the on-disk catalog, management model rows, and proxy routing agree.NATIVE_OPENAI_CONTEXT_OVERRIDESentries are now decoupled from cap application.applyNativeOpenAiContextOverrideapplies an override first if one exists, then caps the resultingcontext_window/max_context_windowregardless of origin, so preserved native rows without an override (e.g.gpt-5.4-mini) cannot bypass the cap; auto-compaction is recomputed from the capped window.providerContextCaps.OPENAI_CODEX_GPT56_CONTEXT_WINDOW/OPENAI_GPT56_CONTEXT_WINDOWSregistry constants flagged in the issue suspected-cause section; they were dead code.Closes #1430
Verification
bun run typecheckpasses.tests/codex-catalog.test.ts,tests/native-model-toggle.test.ts,tests/route-explainability.test.ts,tests/claude-models-discovery.test.ts, plus grok/claude context-window suites (230 tests, 0 fail), including the live discovery regression for272000 / 272000 / 244800.gpt-5.4-minirow without a hardcoded override capped at 200000/200000/180000,nativeModelRowsandcandidateCapabilityEvidenceceiling behavior, and no-cap default unchanged at 372k.bun run privacy:scanpasses;git diff --checkclean.devhead0c170525c; the exact head1a31c7b0is green on the repository gates (enforce-target,label,hygiene,resolve-pr). Cross-platform CI and React Doctor require maintainer approval to run on this fork PR.devbaseline on the same files and is identical except onecrash-guardbreadcrumb test that passes on rerun (load flake during the full run). Pre-existingdevfailures (management-provider-validation,crash-guardtimeout, retained-root serialization seam) reproduce on the clean baseline and are unrelated.Checklist
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
Bug Fixes