fix(models): keep per-provider context caps independent - #1224
Conversation
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds independent provider context-cap values and optional global propagation. The management API, GUI, CLI, tests, and localized documentation now support this behavior. ChangesProvider context-cap management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ModelsPage
participant ProviderRoutes
participant ContextCap
Operator->>ModelsPage: Select provider cap or global cap
ModelsPage->>ProviderRoutes: Submit validated update
ProviderRoutes->>ContextCap: Apply provider value or setAll
ContextCap-->>ProviderRoutes: Update cap configuration
ProviderRoutes-->>ModelsPage: Return updated provider catalog
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/pages/Models.tsx (1)
420-425: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSend the next provider-cap state.
Line 420 sets
enabledto the current cap state. The request at Line 425 then re-applies the current state.When the cap is off, clicking the switch sends
enabled: false. When the cap is on, clicking the switch sendsenabled: true. The switch cannot enable or disable a provider cap.Proposed fix
- const enabled = contextCaps[provider] !== undefined; + const enabled = contextCaps[provider] === undefined;As per path instructions, GUI state must stay consistent with management API responses.
🤖 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 `@gui/src/pages/Models.tsx` around lines 420 - 425, Update the provider-cap toggle logic around the enabled state and the PUT request so it sends the next state, not the current state: invert whether contextCaps[provider] is defined before serializing enabled. Keep the UI state synchronized with the management API response by applying the returned provider-cap state after a successful request.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.
Inline comments:
In `@src/server/management/provider-routes.ts`:
- Around line 615-635: Validate optional provider context-cap fields before any
configuration mutation: in the provider-specific branch, reject a supplied value
unless it is a finite positive number, returning 400 instead of converting it to
undefined; in the global-cap branch, reject a supplied setAll unless it is
boolean, while preserving omitted-field behavior. Apply these checks before
setProviderContextCap, setGlobalContextCapValue, save, or related
reconciliation, using the existing request handler flow.
---
Outside diff comments:
In `@gui/src/pages/Models.tsx`:
- Around line 420-425: Update the provider-cap toggle logic around the enabled
state and the PUT request so it sends the next state, not the current state:
invert whether contextCaps[provider] is defined before serializing enabled. Keep
the UI state synchronized with the management API response by applying the
returned provider-cap state after a successful request.
🪄 Autofix
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: aa346184-6a7d-4847-b4be-9b9e0098beb5
📒 Files selected for processing (11)
docs-site/src/content/docs/guides/model-routing.mddocs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/guides/model-routing.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/guides/model-routing.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdgui/src/pages/Models.tsxsrc/providers/context-cap.tssrc/server/management/provider-routes.tstests/management-provider-validation.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8c3fdce06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SummaryAll review findings have been addressed in commit Fixes
Verification
All four review comment threads are resolved. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/pages/Models.tsx (1)
491-505: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the bulk-cap target set consistent with the GUI.
allCappedonly considers non-native routed groups with rows. The bulk actions send{ setAll: ... }, butsrc/server/management/provider-routes.tsLines 655-657 passes every configured provider tosetAllProviderContextCaps.src/providers/context-cap.tsLines 63-73 then writes an entry for every supplied name.This includes disabled providers and providers that the Models page excludes as native or non-routed. The toggle can persist caps that the GUI cannot display or manage.
Use one shared eligible-provider predicate for the GUI and server bulk path. At minimum, exclude disabled providers and non-routed providers. Add a regression test with disabled and native providers.
Suggested server-side target selection
- const names = Object.keys(config.providers); + const names = eligibleRoutedProviderNames(config);Based on the PR objectives, the toggle must apply only to enabled routed providers. As per path instructions, keep GUI state consistent with management API responses and shared routing/config layers.
Also applies to: 542-542
🤖 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 `@gui/src/pages/Models.tsx` around lines 491 - 505, Align the bulk-cap target selection used by Models.tsx allCapped and provider-routes.ts setAllProviderContextCaps through one shared eligibility predicate. The predicate must include only enabled, routed, non-native providers so setAll cannot persist caps for providers the GUI excludes; keep GUI state and management API responses consistent. Add regression coverage including disabled, native, and non-routed providers.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.
Inline comments:
In `@src/server/management/provider-routes.ts`:
- Around line 615-619: Update the validation preceding Branch 1 in the handler
to reject malformed or mixed payloads before branch selection: require provider
and enabled to be present together with their expected types, reject any payload
containing provider fields with setAll, and return 400 without modifying
configuration. Ensure numeric value validation remains intact, and add
regression tests covering invalid enabled with provider and setAll combined with
provider fields, verifying unchanged configuration.
---
Outside diff comments:
In `@gui/src/pages/Models.tsx`:
- Around line 491-505: Align the bulk-cap target selection used by Models.tsx
allCapped and provider-routes.ts setAllProviderContextCaps through one shared
eligibility predicate. The predicate must include only enabled, routed,
non-native providers so setAll cannot persist caps for providers the GUI
excludes; keep GUI state and management API responses consistent. Add regression
coverage including disabled, native, and non-routed providers.
🪄 Autofix
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: 217e85a8-7da8-4882-bed4-494442586029
📒 Files selected for processing (4)
docs-site/src/content/docs/ko/reference/configuration/providers.mdgui/src/pages/Models.tsxsrc/server/management/provider-routes.tstests/management-provider-validation.test.ts
Follow-up review round summaryA new CodeRabbit finding (Major — "Reject malformed and mixed payloads before branch selection") arrived after the first round and has been fixed in commit FixPayload validation now happens before branch selection (
Regression testsBoth cases are covered in Verification
All review comment threads (first round + this one) are resolved. The PR head is at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bd8b0828e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a26bace to
2245a43
Compare
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/server/management/provider-routes.ts`:
- Line 606: In the PUT provider context-cap validation flow, validate the parsed
body with isPlainRecord before accessing provider, enabled, value, or setAll.
Reject any non-object payload, such as {"provider": true}, with the route’s
consistent 400 response, then compute hasProviderFields and continue existing
validation for plain-record bodies.
- Around line 628-632: Normalize body.value with Math.floor before validation in
the provider-context-caps PUT handler, reject undefined or values whose
normalized result is less than 1, and reuse that normalized value for both
per-provider and global cap setters. Add a regression test covering value 0.5
that preserves the previous cap without mutating configuration.
🪄 Autofix
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: 88daedcf-c8ca-4424-9100-405be4cfadbc
⛔ Files ignored due to path filters (1)
devlog/_plan/260808_context_cap_per_provider/evidence/01-models-context-caps.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/server/management/provider-routes.tstests/management-provider-validation.test.ts
2 participants
Summary
The Models dashboard leaks a provider's context-cap setting into other providers: changing the shared "Context cap" value rewrote every enabled provider to the new value, even when "Apply the {value} context cap to every routed provider" was unchecked. Providers could only share one global value and could never hold different caps.
Fix
{value, setAll: true}); otherwise each provider keeps its own cap.valuethat touches only that provider.Screenshot
Each routed provider now keeps its own cap value (Alpha 128k, Beta 350k); the shared dashboard value only re-points providers when "apply to every routed provider" is checked.
Verification
bun run typecheckpassesbun run lint:guipassesbun run build:guipassesupstream/devwithout this change — pre-existing, unrelatedChecklist
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
Documentation