Repository navigation
fix(profiles): apply context limit to all profile models - #2201
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details🧰 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:
📝 WalkthroughWalkthroughThe change maps ChangesContext window mapping
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Provider profiles now apply their context limit to all configured and restored supported models, including query-bearing selections. The change is covered across application, alignment, rehydration, and startup paths, with no concrete current-head merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR centralizes serialization of profile context-window overrides and expands each override across comma- or semicolon-separated model lists.
Confidence Score: 4/5The saved-model-override regression should be fixed before merging because it silently applies the wrong context limit to the running model. The expanded serializer correctly covers configured model lists, but runtime and startup paths can select a valid saved model outside that list, leaving the active model absent from the generated context-window map. Files Needing Attention: src/utils/providerProfiles.ts, src/utils/providerProfiles.test.ts
|
| Filename | Overview |
|---|---|
| src/utils/providerProfiles.ts | Centralizes multi-model context-window serialization, but drops the selected saved model override when it is outside the configured model list. |
| src/utils/providerProfiles.test.ts | Adds useful multi-model runtime and startup coverage but does not cover maxContextLength combined with an out-of-list saved model override. |
Reviews (1): Last reviewed commit: "fix(profiles): apply context limit to al..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one code issue that needs to be addressed before this is ready. It is a single invariant break across application and alignment, not a request to broaden this PR into the pre-existing provider-specific startup paths.
Merge readiness
- [P2] Rebase onto current
mainwith the fix
src/utils/providerProfiles.ts:676
This head is two commits behindmain, and the intervening context-limit work in #2082 changed both files touched here. GitHub's current three-way merge is clean, so this is not a conflict report. However, the repository requires branches to be synchronized before follow-up pushes. Please rebase while preparing the fix, then validate the repaired profile behavior against the live resolver and tests rather than validating it only on the stale base.
Finding
-
[P1] Build the context-window map from both configured and effective models
src/utils/providerProfiles.ts:1204
The newserializeProfileContextWindows(profile.model, ...)helper expands the profile's configured comma/semicolon list, but this call site has a second authoritative input:primaryModel. During rehydration,applyActiveProviderProfileFromConfig()obtains a supported saved/modelchoice withgetSavedModelOverrideForProfile()and passes it throughoptions.primaryModel; that effective model is intentionally allowed to differ from every literal entry inprofile.model. The existing Hicap test demonstrates the supported case by restoringgpt-5.4for a profile whose configured model remainsglm-5.2.At this head, that lifecycle produces
OPENAI_MODEL=gpt-5.4andCLAUDE_CODE_OPENAI_CONTEXT_WINDOWS={"glm-5.2":200000}. Because the running model has no exact map entry, runtime resolution falls through to the catalog and returns 1,050,000 instead of the profile's 200,000 cap. That changes metering and auto-compaction budgeting and can let the client exceed the provider's actual limit. The merge-base and live-target implementations key the override with the effectiveprimaryModel, so this regression is caused by replacing that key with configured-list-only serialization while adding secondary models.Please fix the source-of-truth mismatch rather than special-casing Hicap or copying an extra assignment into this one branch. The shared context-map construction should accept the configured model field and an optional already-resolved effective model, form their union while preserving the existing model spellings, and assign the profile limit to every resulting key. Application and alignment should pass
primaryModel; generic startup serialization can continue using the configured list because no restored selection has been resolved there. Use that same expected-map construction in bothapplyProviderProfileToProcessEnv()andisProcessEnvAlignedWithProfile()so alignment cannot accept an environment that application would consider incomplete. Preserve the current separation between selection and configuration: do not insert the saved choice intoprofile.model, and continue relying ongetSavedModelOverrideForProfile()to decide whether a saved choice is valid.Add one composition-level regression test with a valid saved model outside the configured list and a
maxContextLength. It should verify together that (1)OPENAI_MODELis the saved effective model, (2) the serialized map contains that model plus every configured list member at the same limit, (3) runtime resolution returns the configured limit for both the effective model and a configured secondary model, (4) a second rehydration/alignment pass preserves the same complete map, and (5) the persistedprofile.modelremains unchanged. This closes the uncovered lifecycle boundary without expanding the PR into provider-specific routed persistence.
Overall guidance
The repeated review rounds appear to come from testing the two relevant behaviors in isolation: the new tests prove that list serialization covers configured secondary models, while older tests prove that saved /model choices survive rehydration. Neither suite asserted their composition at the environment-to-runtime boundary. As a result, the serializer was made correct for the static profile list while application continued selecting the running model from separate saved state.
For this fix, define and test the invariant at the handoff: whenever a profile supplies maxContextLength, the emitted context-window map must cover the effective OPENAI_MODEL and all configured profile models, and alignment must compare against that identical map. Centralizing that invariant in one helper used by application and alignment is the important part; provider names, credential routing, and unrelated startup builders should remain untouched. A focused lifecycle test as described above should prevent another round of symptom-by-symptom feedback.
I specifically rechecked the provider-specific persisted-startup omission raised earlier. It is real pre-existing debt, but it is unchanged on the merge base, this head, and the live target, and it is outside #2182's named generic custom-profile call sites. It is therefore intentionally excluded from this review and should not be folded into this PR.
|
@jatmn The P1 invariant fix and P2 rebase are now pushed in 9bed13b, rebased onto upstream main at 0ea8eef. The shared serializer forms the union of the configured model list and the resolved effective model, preserving model spellings. Both application and alignment pass The composition regression uses The required local preflight was rerun after the rebase. Profile tests pass (178/178), as do the full check, typechecks, launchers, recommendation tests, and security scan. The provider suite still has the one documented watchdog failure reproduced on the same upstream base (1628 pass, 1 fail); details are in the PR description. Could you please re-review the updated head? Thank you for identifying the missing composition-level coverage. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one remaining issue that needs to be addressed before this is ready.
This is one invariant failure across profile serialization and runtime
consumption, not a new set of unrelated provider-profile findings.
Findings
-
[P1] Cover the query-stripped runtime key in generated context limits
src/utils/providerProfiles.ts:694
serializeProfileContextWindows()enumerates the right sources now—the full
configured list plus the effective saved selection—but it serializes each
source's raw spelling. Runtime does not use that spelling as its limit key:
resolveModelRuntimeLimits()removes?reasoning=/?thinking=before
consultingCLAUDE_CODE_OPENAI_CONTEXT_WINDOWS. These are supported
production values, not malformed input:/modelsaves the selected string
verbatim, andprofileSupportsModel()deliberately removes the query when it
validates that saved selection against the profile or provider catalog.The resulting failure is concrete. A Hicap profile configured as
glm-5.2; gpt-5.2, capped at 200,000, with saved
gpt-5.4?reasoning=highemits:OPENAI_MODEL=gpt-5.4?reasoning=high CLAUDE_CODE_OPENAI_CONTEXT_WINDOWS={"glm-5.2":200000,"gpt-5.2":200000,"gpt-5.4?reasoning=high":200000}Runtime then looks up
gpt-5.4, misses the generated key, and resolves the
catalog's 1,050,000-token window instead of the profile's 200,000-token cap.
Query-bearing configured secondary models miss for the same reason. That can
delay metering and auto-compaction until the provider rejects an oversized
request, so the all-configured/all-effective invariant promised by this PR
remains incomplete.The root cause is that this lifecycle currently uses the raw
request/display spelling at serialization but a query-stripped spelling at
runtime lookup. The patch centralized model enumeration, but the producer
and consumer still disagree at that boundary. Please make each generated
context map cover the exact query-stripped model value that
resolveModelRuntimeLimits()passes to the existing lookup, for both
configured models and the validated effective model. Keep the raw
OPENAI_MODEL, storedprofile.model, and query options unchanged; the
correction belongs in context-limit key construction, not model selection.
This finding does not establish a need to copy the broader
normalizeProfileModelLookupKey()behavior: changing case,[1m], aliases,
or documented exact/prefix matching would be separate work and is not
requested here.Add a composition regression that includes both a query-bearing configured
secondary and a valid query-bearing saved selection outside the configured
list. In one lifecycle, verify the raw effectiveOPENAI_MODEL, runtime
resolution of the profile cap for the configured and saved base identities,
a repeated rehydration/alignment pass, and an unchanged stored model list.
Existing keyed/keyless startup, route, credential, and cleanup behavior
should remain unchanged.
Overall guidance
The review churn is coming from exercising the same invariant one dimension at
a time. The first change covered configured list expansion; the next covered
composition with an out-of-list saved model; the remaining gap is that the map
producer and runtime consumer disagree about model identity. The current tests
use plain model strings and assert the serialized JSON, so they prove what the
producer wrote but do not falsify the consumer's normalization behavior.
Please close the invariant at the handoff rather than adding another
route-specific assignment or special-casing Hicap: for any supported
query-bearing model spelling selected under an OpenAI-compatible profile with
maxContextLength, the generated map must cover the query-stripped key that
runtime actually consumes. Use the same expected-map construction for
application, alignment, and generic startup serialization, and test the
environment-to-resolveModelRuntimeLimits() result as one composition.
This guidance intentionally does not ask this PR to change documented
exact/prefix override precedence, provider-specific startup builders, model
selection persistence, profile editing, credentials, or unrelated provider
routing. Those adjacent surfaces were checked and are not additional findings.
|
@jatmn Fixed in 6cfafbe, rebased onto 1afeb4b. Context-limit keys now match runtime’s query-stripped identity while preserving raw model selections and options. The composition tests cover configured/saved models, runtime limits, repeated rehydration, and old-map repair; all 283 related tests pass. Full validation and failures reproduced on the same upstream base are documented in the PR description. Could you please re-review? |
…) — DOC-ONLY fork port: full upstream code change is fork-policy-conflict — the 3way merge with theirs introduces CLINE_API_KEY / NEARAI_API_KEY / FIREWORKS_API_KEY / LONGCAT_API_KEY / CLOUDFLARE_API_TOKEN / ANTHROPIC_BASE_URL / Gemini/Mistral/Bedrock/Vertex/Foundry/NVIDIA-NIM/XAI profile types, all of which the fork removed per AGENTS.md Provider Policy (anthropic/ollama/openai-compatible only). The 20+ typecheck errors and the 7.3k-line diff on test fixtures are not partial-portable in a meaningful way — same situation as upstream 69aca78 effort.ts already documented in docs/sync-upstream.md (r5 deferred section). Functional behavior gap is left in place: fork's applyProviderProfileToProcessEnv only writes the primaryModel to CLAUDE_CODE_OPENAI_CONTEXT_WINDOWS, so the saved /model selection can fall back to a different context limit. Tracked as TODO; resume in a follow-up session that ports the serializeProfileContextWindows helper as a 3-provider-only carve-out. This commit ports the docs/advanced-setup.md paragraph describing the behavior — kept verbatim because it's accurate documentation of the gap, not the fix. Users reading the docs learn the intended behavior even though fork's runtime hasn't caught up.
…igpine#2185) DOC-ONLY + model entry partial port of upstream aceacf0 (Twigpine#2185). Skipped (per fork scope / AGENTS.md Provider Policy): - src/integrations/routeMetadata.ts (runtime interface change — conflicts with fork's `minimax-anthropic` route; requires fork-specific design decision, deferred per r3 sync convention) - src/integrations/runtimeMetadata.ts (resolvedRouteId field add + findModelDescriptorForApiName + xai/aimlapi/discoveryCache integration — multi-file runtime refactor, out of scope for DOC-ONLY path) - src/services/api/openaiShim/requestPreparation.ts (field rename) - All *.test.ts (fork Message-type drift avoidance; reasoning-effort test files depend on xhigh EFFORT_LEVELS that fork has not ported) - src/utils/providerProfiles.ts (already DOC-ONLY via r5 Twigpine#2201) - README.md (3way conflict on provider list baseline, rejected per rule #2) Files ported (5 → 7 with field parity): - .env.example (+5) — glm-5.3-flash env example - README.md — REJECTED (3way conflict) - docs/integrations/reasoning-effort.md (+63, new file) - src/integrations/brands/glm.ts (+1) — add `glm-5.3-flash` to modelIds - src/integrations/models/glm.ts (+14 → +15) — new glm-5.3-flash entry - src/integrations/vendors/zai.ts (+17/-1) — catalog entry; deletes `matchBaseUrlHosts: ['api.z.ai']` (3way auto-merged per the rule of accepting upstream's host-boundary simplification) - src/integrations/descriptors.ts (+12) — declare `runtimeMetadataScope` on ModelDescriptor for upstream parity. Runtime logic NOT ported (see below). New model: glm-5.3-flash (1M context, 131K output, vision+reasoning+coding). Fork note — runtimeMetadataScope field zombie: The `runtimeMetadataScope?: 'global' | 'catalog'` field on ModelDescriptor is declared for type-level parity with upstream Twigpine#2185, but the runtime logic that honors it (`inferredModelDescriptor?.runtimeMetadataScope === 'catalog' ? null : inferredModelDescriptor` in `resolveModelRuntimeLimits`) is NOT ported. That logic depends on upstream's `findModelDescriptorForApiName` + `resolveRouteOpenAIShimConfig` + xai/aimlapi/discoveryCache integration which is multi-file and requires fork-specific design decisions (fork lacks xai/aimlapi providers per AGENTS.md Provider Policy; discoveryCache is upstream-only infrastructure). The field is inert at runtime — fork behavior is identical to before this commit — but type-correct. Resume path: when fork runtime reconciles with upstream main (separate session), port `findModelDescriptorForApiName` and the catalog-scope branching, then add tests. Verification (5-phase): - Phase 1 build: ✓ Built opencc v0.27.0 → dist/cli.mjs rebuilt - Phase 2 typecheck: ✓ 0 errors - Phase 3 test: 5511 pass / 220 skip / 0 fail (no delta vs baseline) - Phase 4 TUI smoke: node bin/opencc -p "say 'ok' and stop" --model glm-5.3-flash → "ok" (model entry loads, CLI emits API request) - Phase 5 debug log scan: no new anomaly class (catalog + brand + vendor entries all load without warnings) Not pushed — awaiting user decision on integration into main-opencc.
3 participants
Summary
maxContextLengthto every configured model and its supported saved/modelselection.resolveModelRuntimeLimits(). For a Hicap profile capped at 200,000, configuredglm-5.2; gpt-5.2?reasoning=highand savedgpt-5.4?reasoning=highall resolve the profile cap instead of falling back to catalog/default limits.[1m]tags and stored profile model list.Fixes #2182
Impact
Testing
6cfafbe9278da52390ab710b51c1d02f2779dd70, rebased onto upstreammainat1afeb4b1e7a892b6456c6b86f6fd6683222b2005. Node 22.22.1, Bun 1.3.14, macOS arm64.bun install --frozen-lockfile,bun run typecheck,bun run typecheck:type-tests,node bin/openclaude --version,NODE_DISABLE_COMPILE_CACHE=1 node bin/openclaude --version, and exact changed-scope lint.bun test ./src/utils/providerProfiles.test.ts ./src/integrations/runtimeMetadata.test.ts ./src/utils/model/openaiContextWindows.test.ts: 283 passed, 0 failed.npm run test:provider-recommendation: 160 passed, 0 failed.git fetch https://github.com/Gitlawb/openclaude.git mainfollowed bybun run security:pr-scan -- --base FETCH_HEAD --head HEAD: passed on the committed candidate.bun run check: build, smoke and deadcode complete, but its unit-test log contains 57 failures despite exit code 0 and ends atsrc/cli/bgRegistry.test.tswithout an aggregate summary. The separate conversation-arc test passes. This is not a clean/full unit-suite pass.1afeb4b1, the samebun run checkproduces exactly the same 57 named failures and the same early end; comparison found no candidate-only failures. Affected groups include fact extraction, persistence, attribution, pricing, model output limits and fast mode. Example:deepseek-v4-flash uses the gateway-safe output cap by defaultexpects 65,536 output tokens but receives 8,000 on both trees. These failures also occur before the provider-profile tests in both runs; the PR does not modify their causal surfaces.bun run test:provider: 1641 passed, 1 failed:Claude stream watchdog > falls back when the top-level stream iterator never settles, withENOENTreadinginterruption-trace.jsonl. The same failure is reproduced on upstream1afeb4b1, including an isolated run ofbun test --feature=UNATTENDED_RETRY ./src/services/api/claude.streamWatchdog.test.ts --test-name-pattern 'falls back when the top-level stream iterator never settles'. The watchdog/trace implementation is unchanged by this PR.Notes
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Documentation