align GPT-5 model routing with current OpenAI defaults#309
Conversation
📝 WalkthroughWalkthroughThe changes introduce a centralized model mapping and profiling system that replaces scattered model normalization logic with canonical model profiles, reasoning effort defaults, and tool capability metadata. New functions Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes key review vectors: the model profile system in 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 1937-1938: Normalize the model alias once before any live-probe
paths and reuse that normalized value everywhere instead of passing raw
options.model into fetchCodexQuotaSnapshot; update the code that currently
creates probeModel/inspectRequestedModel at lines like const probeModel =
options.model?.trim() || "gpt-5-codex" and ensure that the same normalized
variable is used for all calls to fetchCodexQuotaSnapshot (symbols: probeModel,
modelInspection, fetchCodexQuotaSnapshot) across the other live paths
(previously at the spots calling fetchCodexQuotaSnapshot) so routing is
consistent; add a vitest that takes a single alias and invokes the CLI handlers
for check, report, forecast, best, and fix, asserting the same resolved backend
model is used for each.
In `@lib/prompts/codex.ts`:
- Around line 94-95: The prewarm path is currently firing per-model targets
(e.g., both "gpt-5.4" and "gpt-5.2") which map to the same prompt family via
getModelFamily; update the prewarm logic (the function that builds prewarm
targets around the code referenced at lines ~372-379) to deduplicate targets by
prompt family using getModelFamily so only one request per promptFamily is
issued, add retry/backoff handling for EBUSY/429 in that queue, and ensure no
sensitive tokens/emails are logged; then add a deterministic vitest that covers:
multiple models mapping to the same family produce a single prewarm request,
retries on simulated EBUSY/429, and that gpt-5.1 still gets prewarmed when
present to prevent coverage loss.
In `@lib/request/helpers/model-map.ts`:
- Around line 183-184: Replace the direct addAlias calls for the chat-latest
entries with addReasoningAliases so wildcard legacy variants (e.g.,
"gpt-5-chat-latest-high") map to the correct base model and reasoning defaults;
specifically update the lines that call addAlias("gpt-5.1-chat-latest",
"gpt-5.1") and addAlias("gpt-5-chat-latest", "gpt-5") to use addReasoningAliases
instead, and add a vitest regression that asserts variants like
"gpt-5-chat-latest-high" and "gpt-5.1-chat-latest-foo" resolve to the intended
model IDs and reasoning settings (matching the behavior of addReasoningAliases).
In `@test/codex-prompts.test.ts`:
- Around line 467-486: The direct-family tests for codex variants only assert
the mocked response body and miss verifying the raw GitHub fetch path; update
the two earlier tests that call getCodexInstructions for the direct families
(the tests around the 'codex' and 'codex-max' cases that currently only check
the mocked body) to inspect mockFetch.mock.calls, find the
raw.githubusercontent.com call (like in the gpt-5.4 test), and add an assertion
that the fetched URL contains the expected raw prompt filename (e.g.,
"gpt_5_codex_prompt.md" for codex and "gpt_5_codex_max_prompt.md" for codex-max)
so regressions that collapse families into the wrong prompt file will fail.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 183f50c9-5a91-43e8-91a0-f5088cfe0930
📒 Files selected for processing (12)
lib/capability-policy.tslib/codex-manager.tslib/prompts/codex.tslib/request/helpers/model-map.tslib/request/request-transformer.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/codex.test.tstest/config.test.tstest/model-map.test.tstest/property/transformer.property.test.tstest/request-transformer.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/capability-policy.tslib/request/request-transformer.tslib/codex-manager.tslib/prompts/codex.tslib/request/helpers/model-map.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/property/transformer.property.test.tstest/config.test.tstest/codex-prompts.test.tstest/codex-manager-cli.test.tstest/codex.test.tstest/request-transformer.test.tstest/model-map.test.ts
🔇 Additional comments (5)
test/property/transformer.property.test.ts (1)
48-56: good property coverage for the new defaults.
test/property/transformer.property.test.ts:48-56andtest/property/transformer.property.test.ts:206-217pin thegpt-5.4default and the xhigh downgrade path cleanly.Also applies to: 206-217
test/codex-prompts.test.ts (1)
89-97: good family-routing regression coverage.
test/codex-prompts.test.ts:89-97pins the gpt-5.4-era general models to thegpt-5.2prompt family cleanly.test/codex-manager-cli.test.ts (1)
5705-5776: good json report coverage for model diagnostics.
test/codex-manager-cli.test.ts:5705-5776locks the newmodelSelectionpayload for both the no-remap and remap cases.test/config.test.ts (1)
93-118: good reasoning-tier regression coverage.
test/config.test.ts:93-118andtest/config.test.ts:191-196cover the new lightweight defaults, thegpt-5.4none default, and thegpt-5.4-proclamp.Also applies to: 175-196
test/codex.test.ts (1)
14-30: good prompt-family routing coverage.
test/codex.test.ts:14-30pins the gpt-5.4-era routing and the default-family fallback in one place.
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
@greptile @cubic review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
✅ Actions performedFull review triggered. |
All review threads are resolved and follow-up commits addressed this stale automated change request.
Summary
Stack
main