fix(ctx): let explicit overrides beat discovered context windows - #2082
kevincodex1 merged 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (4)Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny.⚙️ CodeRabbit configuration file Files:
Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.⚙️ CodeRabbit configuration file Files:
Review docs for accuracy against current code behavior.⚙️ CodeRabbit configuration file Files:
Apply the OpenClaude maintainer review rubric from AGENTS.md.⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughRuntime context-window resolution now applies prefix and settings overrides before discovery values. Tests cover cache isolation, explicit overrides, and 128k and 200k gateway limits. Documentation describes precedence and session-only or permanent configuration. ChangesRuntime context-window precedence
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to OpenAI-compatible gateway limits now preserve discovered values unless an explicit configured override applies, with matching documentation and regression coverage. No current merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Full details: Description checkExplanation The description clearly covers the motivation, implementation, tests, user impact, provider path, and known limitations. It does not use every template heading exactly, and it omits the explicit local-preflight checkbox, but the required information is substantially present. Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 5 files. (1 skipped: 1 unsupported.) Full details: Risk Surface DisclosedExplanation PASS. The diff changes runtime-limit precedence, documentation, help text, and test fixtures. It does not change route selection, discovery requests, authentication, permissions, or production config-home behavior. The test-only config-home changes use an existing testing override. The PR description identifies the OpenAI-compatible gateway risk: an advertised window can be a real deployment cap, while overriding it incorrectly can cause a mid-session API failure. It also states the user impact and that accurate gateway behavior does not change. No blocker is introduced by the changed risk surface. Full details: No Hidden Policy ChangeExplanation PASS — The only production behavior change is the documented
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/integrations/runtimeMetadata.ts`:
- Around line 492-506: Update preferDiscoveredOrKnownContextWindow and its
discovery call sites so descriptor rescue uses explicit provenance indicating
that the discovered context window came from the generic fallback, rather than
treating every 128,000 value as fallback. Preserve explicit provider limits of
128,000, and add coverage for both explicit 128k discovery metadata and generic
fallback discovery metadata.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 035631b0-cc00-48b2-8b16-2292c53b6a08
📒 Files selected for processing (6)
docs/advanced-setup.mdsrc/commands/set-context-window/set-context-window.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.tssrc/integrations/runtimeMetadata.tssrc/utils/model/openaiContextWindows.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/model/openaiContextWindows.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.tssrc/commands/set-context-window/set-context-window.tssrc/integrations/runtimeMetadata.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/model/openaiContextWindows.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.tsdocs/advanced-setup.mdsrc/commands/set-context-window/set-context-window.tssrc/integrations/runtimeMetadata.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/model/openaiContextWindows.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.tsdocs/advanced-setup.mdsrc/commands/set-context-window/set-context-window.tssrc/integrations/runtimeMetadata.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/openaiContextWindows.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.tssrc/integrations/runtimeMetadata.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
docs/advanced-setup.md
🔇 Additional comments (1)
src/integrations/runtimeMetadata.ts (1)
547-575: 🎯 Functional CorrectnessRun the required validation before merge.
No focused test or typecheck result was provided. Run the narrow runtime-metadata tests,
bun run typecheck, andbun run typecheck:type-testswhen that script exists.Sources: Coding guidelines, Path instructions
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve explicit 128k discovery limits
src/integrations/runtimeMetadata.ts:502
The new helper replaces every cached128_000value with a larger descriptor. That number is not a provenance signal: the custom discovery mapper copies an endpoint's explicitcontext_length/context_windowinto the same plainModelCatalogEntry.contextWindowfield. A gateway can legitimately expose a 128k deployment or tenant cap for a globally known larger model such asgpt-5.6-sol; after this change it is silently budgeted at 272k. The context resolver and OpenAI shim then delay compaction and retain/compress history past the endpoint's real limit, producing a late context-window API failure.Please address the root cause rather than inferring “generic” from the numeric value. Preserve a verifiable trust/provenance signal from discovery through the cache, or restrict any descriptor-rescue policy to a specifically identified gateway/response contract that can actually distinguish a synthetic default from an advertised cap. If the OpenAI-compatible response provides only an indistinguishable numeric field, it cannot safely support this global heuristic; retain discovery as authoritative and expose the documented per-model override instead. Add regression coverage for both an explicit 128k provider cap (which must remain 128k) and the identified generic-default path (which may use the descriptor). This is the unresolved CodeRabbit provenance request on the earlier head.
-
[P3] Isolate the newly added discovery-cache fixture
src/integrations/runtimeMetadata.modelLimits.test.ts:175
getClaudeConfigHomeDir()intentionally ignoresCLAUDE_CONFIG_DIRand only honorsOPENCLAUDE_CONFIG_DIR(or the test override), so this test writes its fake cache entry to the caller's real default OpenClaude config directory while deleting an unused temporary directory. The assertion can pass even when the cache is not read becausemodelLimitswins first, so it neither proves the claimed cache-precedence boundary nor reliably isolates its state; it can also leak fake metadata into later tests or a developer's config.Use
setClaudeConfigHomeDirForTesting(saving and restoring its previous value) or an explicitly restoredOPENCLAUDE_CONFIG_DIR, then clear the actual discovery cache/synchronous snapshot during cleanup. Make the test load-bearing by first establishing that the isolated cache is observable withoutmodelLimits, then asserting that the same cached value loses once the explicit settings override is enabled. That tests the intended ordering instead of allowing the result to pass without the fixture.
Addresses both review findings on Twigpine#2082. P1 — drop the generic-128k rescue from the previous commit. The custom gateway discovery mapper copies an endpoint's own context_length / context_window into ModelCatalogEntry.contextWindow, and nothing synthesizes 128k on our side, so the numeric value carries no provenance: a flat 128000 is indistinguishable from a real per-deployment or tenant cap. Budgeting a known-larger descriptor over that cap traded an early auto-compact for a late context-window API failure. Discovery is authoritative over the model descriptor again. What remains is the safe half of the fix: an env prefix override and settings.json modelLimits now sit above discovery, so a user on a gateway that advertises the wrong window can pin the real one. Docs and /set-context-window help point at that path. Tests cover a gateway-advertised 128k and 200k window staying put for a model the catalog knows is larger, plus env and modelLimits overrides beating it. P3 — isolate the discovery-cache fixtures. getClaudeConfigHomeDir() ignores CLAUDE_CONFIG_DIR by design, so both the new modelLimits test and the pre-existing withTempConfigDir helper were writing fixtures into the caller's real config dir. Both now use setClaudeConfigHomeDirForTesting with save and restore, and clear the discovery cache on teardown so the in-memory sync snapshot cannot leak into a later suite. The modelLimits test now asserts the isolated cache resolves on its own before asserting the settings pin wins. Refs Twigpine#2081
euxaristia
changed the title
|
@jatmn thanks, both findings addressed in 78d9ece. I retitled the PR and rewrote the description, since your P1 changes what this delivers. [P1] Preserve explicit 128k discovery limits You're right, and I dropped the heuristic entirely rather than trying to rescue it. I went looking for a provenance signal to preserve and there isn't one to preserve: So Regression coverage is as you asked: [P3] Isolate the newly added discovery-cache fixture Confirmed, and it was worse than the one test: The Checks Mutation-checked both guards before claiming this: putting the discovery cache back above |
|
Correcting one line in my previous comment: CI CI on 78d9ece:
|
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/integrations/runtimeMetadata.test.ts`:
- Around line 173-212: Add a regression test alongside the existing discovery
precedence cases that seeds discovery data for a model with both contextWindow
and maxOutputTokens, then sets CLAUDE_CODE_OPENAI_CONTEXT_WINDOWS and
CLAUDE_CODE_OPENAI_MAX_OUTPUT_TOKENS for the matching prefix key. Assert
resolveModelRuntimeLimits returns both prefix-provided values, confirming prefix
overrides take precedence over discovery.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e139d5c6-1737-4092-9126-54e90440abc9
📒 Files selected for processing (5)
docs/advanced-setup.mdsrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.tssrc/integrations/runtimeMetadata.tssrc/utils/model/openaiContextWindows.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/model/openaiContextWindows.tssrc/integrations/runtimeMetadata.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/model/openaiContextWindows.tsdocs/advanced-setup.mdsrc/integrations/runtimeMetadata.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/model/openaiContextWindows.tsdocs/advanced-setup.mdsrc/integrations/runtimeMetadata.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/openaiContextWindows.tssrc/integrations/runtimeMetadata.tssrc/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
docs/advanced-setup.md
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/integrations/runtimeMetadata.modelLimits.test.tssrc/integrations/runtimeMetadata.test.ts
🔇 Additional comments (6)
src/integrations/runtimeMetadata.ts (2)
513-545: LGTM!
513-545: 📐 Maintainability & Code QualityProvide validation results before merge.
The supplied context has no results for the required TypeScript checks. Run the focused runtime-metadata tests and
bun run typecheck. Runbun run typecheck:type-testsif that script applies. Report the exact commands and results in the PR.As per coding guidelines, “Run the relevant TypeScript validation checks for changed code, including
bun run typecheckand, when applicable,bun run typecheck:type-tests.” As per path instructions, “run narrow tests for the modified integration files plus relevant typecheck/build validation; report exact commands in the PR.”Sources: Coding guidelines, Path instructions
src/utils/model/openaiContextWindows.ts (1)
13-13: LGTM!src/integrations/runtimeMetadata.test.ts (1)
14-48: LGTM!docs/advanced-setup.md (1)
520-531: LGTM!src/integrations/runtimeMetadata.modelLimits.test.ts (1)
184-234: 🩺 Stability & AvailabilityNo change needed:
acquireSharedMutationLockalready serializes this fixture.
CodeRabbit on Twigpine#2082 asked for a regression where CLAUDE_CODE_OPENAI_* prefix keys beat a seeded discovery cache for both contextWindow and maxOutputTokens, not just exact keys and settings.modelLimits. Refs Twigpine#2081
|
Addressed the remaining CodeRabbit finding in 6182a43: Env prefix overrides vs discovery Added |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Make the permanent-override guidance work for catalogued models
src/commands/set-context-window/set-context-window.ts:20
The new help presentsmodelLimitsas a permanent counterpart to the session override, but the resolver evaluatescatalogEntry?.contextWindowandcatalogEntry?.maxOutputTokensbefore the corresponding settings values. For example, a user can run/set-context-window gpt-4o 200000, copy the displayedmodelLimitsrecipe intosettings.json, and then restart into the catalog limit instead of 200k. That silently reintroduces the compaction or endpoint-limit behavior the user was trying to prevent.The root cause is that the documentation presents two configuration mechanisms as equivalent despite their intentionally different precedence. Preserve the catalog-over-settings policy only if the help explicitly limits
modelLimitsto custom/discovered models and sends catalogued models to an exactCLAUDE_CODE_OPENAI_CONTEXT_WINDOWSentry; otherwise change the resolver and add a regression that proves amodelLimitsentry persists a catalogue-model override. The command help, advanced setup page, and tests should describe and enforce one consistent contract. -
[P2] Do not close the unresolved generic-discovery issue
PR description: Fixes #2081
#2081 explicitly requires known models such asgpt-5.6-solnot to remain on a generic discovered128000, while this head intentionally adds a test that preserves discovered 128k (and 200k) values. The PR correctly explains why the current cache cannot distinguish a gateway's synthetic default from a real tenant/deployment cap, but its safe manual-override path does not implement the issue's expected automatic behavior. Merging withFixes #2081would therefore close an issue whose primary failure case still reproduces until the user discovers and configures a pin.Address the root mismatch explicitly: either carry verifiable discovery provenance from the gateway response through the cache so a descriptor rescue is limited to a known synthetic-default contract, or narrow #2081 to the documented manual-override behavior and change this link to
Refs #2081. Do not represent a manual workaround as closing the unresolved automatic-detection requirement. -
[P3] Correct the stale precedence comment on the settings match
src/utils/model/openaiContextWindows.ts:29
This comment still says the resolver applies settings after the catalog/discovery cache, but the new resolver putsmodelLimitsabove discovery. The top-of-file comment was updated while this exported-field comment was not, leaving two incompatible descriptions next to the same match type. The next maintainer can reasonably restore or test the wrong ordering from this text. Update every precedence comment at the same time as precedence logic, and keep the wording aligned with the resolver's ordered??chain.
|
Addressed the findings from review 4836484670 in
Validation:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Please rebase on main and resolve conflicts.
64fb0b9 to
e2558a9
Compare
Addresses both review findings on Twigpine#2082. P1 — drop the generic-128k rescue from the previous commit. The custom gateway discovery mapper copies an endpoint's own context_length / context_window into ModelCatalogEntry.contextWindow, and nothing synthesizes 128k on our side, so the numeric value carries no provenance: a flat 128000 is indistinguishable from a real per-deployment or tenant cap. Budgeting a known-larger descriptor over that cap traded an early auto-compact for a late context-window API failure. Discovery is authoritative over the model descriptor again. What remains is the safe half of the fix: an env prefix override and settings.json modelLimits now sit above discovery, so a user on a gateway that advertises the wrong window can pin the real one. Docs and /set-context-window help point at that path. Tests cover a gateway-advertised 128k and 200k window staying put for a model the catalog knows is larger, plus env and modelLimits overrides beating it. P3 — isolate the discovery-cache fixtures. getClaudeConfigHomeDir() ignores CLAUDE_CONFIG_DIR by design, so both the new modelLimits test and the pre-existing withTempConfigDir helper were writing fixtures into the caller's real config dir. Both now use setClaudeConfigHomeDirForTesting with save and restore, and clear the discovery cache on teardown so the in-memory sync snapshot cannot leak into a later suite. The modelLimits test now asserts the isolated cache resolves on its own before asserting the settings pin wins. Refs Twigpine#2081
CodeRabbit on Twigpine#2082 asked for a regression where CLAUDE_CODE_OPENAI_* prefix keys beat a seeded discovery cache for both contextWindow and maxOutputTokens, not just exact keys and settings.modelLimits. Refs Twigpine#2081
|
@coderabbitai full review |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
Proxies such as OmniRoute often report context_length 128000 when per-model metadata is missing. That discovery value was beating known descriptors and settings.modelLimits, so 1M models auto-compacted against a 128k budget. Prefer larger known descriptors over the generic 128k discovery default, and let settings/env overrides beat discovery. Document the override path in /set-context-window help. Refs Twigpine#2081
The Discord report used GPT-5.6 Sol, not gpt-5.4. Regression now pins the catalog-less custom-route rescue to the 272k descriptor for gpt-5.6-sol. Refs Twigpine#2081
Addresses both review findings on Twigpine#2082. P1 — drop the generic-128k rescue from the previous commit. The custom gateway discovery mapper copies an endpoint's own context_length / context_window into ModelCatalogEntry.contextWindow, and nothing synthesizes 128k on our side, so the numeric value carries no provenance: a flat 128000 is indistinguishable from a real per-deployment or tenant cap. Budgeting a known-larger descriptor over that cap traded an early auto-compact for a late context-window API failure. Discovery is authoritative over the model descriptor again. What remains is the safe half of the fix: an env prefix override and settings.json modelLimits now sit above discovery, so a user on a gateway that advertises the wrong window can pin the real one. Docs and /set-context-window help point at that path. Tests cover a gateway-advertised 128k and 200k window staying put for a model the catalog knows is larger, plus env and modelLimits overrides beating it. P3 — isolate the discovery-cache fixtures. getClaudeConfigHomeDir() ignores CLAUDE_CONFIG_DIR by design, so both the new modelLimits test and the pre-existing withTempConfigDir helper were writing fixtures into the caller's real config dir. Both now use setClaudeConfigHomeDirForTesting with save and restore, and clear the discovery cache on teardown so the in-memory sync snapshot cannot leak into a later suite. The modelLimits test now asserts the isolated cache resolves on its own before asserting the settings pin wins. Refs Twigpine#2081
CodeRabbit on Twigpine#2082 asked for a regression where CLAUDE_CODE_OPENAI_* prefix keys beat a seeded discovery cache for both contextWindow and maxOutputTokens, not just exact keys and settings.modelLimits. Refs Twigpine#2081
Keep the documented workaround aligned with resolver precedence so users do not expect modelLimits to override built-in catalog entries. Refs Twigpine#2081
e2558a9 to
859d4c1
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. |
…gpine#2082) fork port: applied per-tier1 worktree (5 files, +115/-16): - docs/advanced-setup.md: precedence reordered (env > modelLimits > discovery) - src/commands/set-context-window/set-context-window.ts: HELP expanded - src/integrations/runtimeMetadata.modelLimits.test.ts: new test file (5 pass / 1 skip) - src/integrations/runtimeMetadata.ts: chain-ordering updated - src/utils/model/openaiContextWindows.ts: comment-only update Skipped per fork policy: - src/integrations/runtimeMetadata.test.ts (references xai/nvidia-nim/codex/openrouter-fires) Silenced 1 test in modelLimits (depends on dropped discovery-cache step). Typecheck: 31 errors all baseline drift (loadPluginCommands.ts / refresh.ts / Config.tsx etc.) — pre-existing on feff404, unrelated to this commit.
3 participants
Summary
OpenAI-compatible gateways can advertise a context window that does not match the model's real capability. A user on Discord (
#openclaude) hit this with OmniRoute + GPT-5.6 Sol (gpt-5.6-sol): the gateway reported 128k, so OpenClaude budgeted 128k and auto-compacted after a few prompts.This PR makes the documented per-model overrides win over discovery, so a user in that position can pin the real window and have it stick.
It deliberately does not try to detect a "wrong" discovery value automatically. An earlier revision of this branch rescued known models from a discovered
128000, and that was wrong: the custom-gateway discovery mapper copies the endpoint's owncontext_length/context_windowinto the same plainModelCatalogEntry.contextWindowfield, and nothing on our side ever synthesizes 128k. So the number carries no provenance, and a flat 128k is indistinguishable from a real per-deployment or tenant cap. Budgeting a known-larger descriptor over a real cap would trade an early auto-compact for a late context-window API failure, which is the worse outcome. Discovery stays authoritative over the model descriptor.Refs #2081
Changes
src/integrations/runtimeMetadata.tsresolveModelRuntimeLimitsprecedence to: exact env override -> built-in catalog -> env prefix override -> settingsmodelLimits-> discovery cache -> descriptor defaultmodelLimits, so a gateway's advertised window could not be overridden by the documented settings pathmaxOutputTokensfor consistencyTests
runtimeMetadata.test.ts: a gateway-advertised window (parameterized over 128k and 200k) is preserved forgpt-5.6-sol, a model the catalog knows is larger; an exact env override beats a discovered windowruntimeMetadata.modelLimits.test.ts: the isolated discovery cache resolves on its own first (128k / 8k), then themodelLimitspin wins (1M / 32k), so the assertion proves precedence rather than passing on a missing fixturesetClaudeConfigHomeDirForTesting(saved and restored) instead ofCLAUDE_CONFIG_DIR, whichgetClaudeConfigHomeDir()ignores by design, and clear the cache on teardown so the in-memory sync snapshot cannot leakDocs / UX
docs/advanced-setup.md: corrected precedence list, plus a note on what to do when a gateway advertises the wrong window/set-context-windowhelp: points at the permanentmodelLimits/ env override pathUser impact
modelLimitsorCLAUDE_CODE_OPENAI_CONTEXT_WINDOWS, or for one session with/set-context-window. Before this change the discovery cache silently outranked all three.Checks run
Mutation-checked both new guards: reordering discovery back above
modelLimitsfails the settings-precedence test, and re-adding the 128k rescue fails the advertised-window test. Restored after each.Notes
CONTRIBUTING.mdandAGENTS.mdSummary by CodeRabbit
New Features
/set-context-window.Bug Fixes
Documentation