feat(router): support default params and expose optional checks in Admin UI - #33144
Open
krrish-berri-2 wants to merge 20 commits into
Open
feat(router): support default params and expose optional checks in Admin UI#33144krrish-berri-2 wants to merge 20 commits into
krrish-berri-2 wants to merge 20 commits into
Conversation
…cks in Admin UI Router.update_settings() silently dropped default_litellm_params and optional_pre_call_checks (not in the allow-list, and optional_pre_call_checks was never even stored as a readable attribute), so the Admin UI's Router Settings page could not display or persist either setting - e.g. enabling cache_control_injection_points or prompt_caching pre-call routing required editing config.yaml directly. Router now persists optional_pre_call_checks and returns both fields from get_settings(). update_settings() merges default_litellm_params instead of replacing it (a full replace would drop the timeout/max_retries/metadata defaults Router.__init__ sets), and diffs optional_pre_call_checks against what's already applied before calling add_optional_pre_call_checks(), since that method has no dedup guard for prompt_caching/enforce_model_rate_limits and would otherwise register a duplicate callback on every re-save.
|
|
![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: 498aa2997d
ℹ️ 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".
Contributor
Greptile SummaryThis PR exposes
Confidence Score: 5/5Safe to merge; backend changes are well-guarded and the UI correctly round-trips both new fields. The core router changes are correctly implemented and well-tested. Both findings are non-blocking quality observations that do not affect the correctness of the stated fix. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/router.py | Adds optional_pre_call_checks persistence and update/remove handlers; wires both new fields into get_settings()/update_settings(). Affinity callback removal only disables flags, does not unregister the callback object. |
| litellm/proxy/proxy_server.py | Adds _apply_router_settings_role_gate to mask sensitive values in default_litellm_params for non-admin callers. Correct implementation. |
| litellm/types/management_endpoints/router_settings_endpoints.py | Adds optional_pre_call_checks to ROUTER_SETTINGS_FIELDS with matching options; updates default_litellm_params description and link. |
| ui/litellm-dashboard/src/components/router_settings/DefaultLitellmParamsSection.tsx | New component for free-form params + cache control toggle. Local otherParamsText state not synced with value prop changes after mount. |
| ui/litellm-dashboard/src/components/router_settings/OptionalPreCallChecksSelector.tsx | New multi-select component for optional pre-call checks driven by server metadata. |
| ui/litellm-dashboard/src/components/shared/cache_control_injection_points_editor.tsx | New shared fully-controlled cache control injection points editor extracted from cache_control_settings.tsx. |
| ui/litellm-dashboard/src/components/add_model/cache_control_settings.tsx | Delegates to shared editor; fixes pre-existing bug where role/index edits read wrong form field name. |
| ui/litellm-dashboard/src/components/router_settings/RouterSettingsForm.tsx | Adds both new sections with key-presence guard for default_litellm_params. |
| tests/test_litellm/proxy/test_proxy_server.py | Adds mock-based tests for role-gated masking of default_litellm_params. |
| tests/test_litellm/test_router.py | Formatting-only changes plus new tests for update_settings round-trip. No assertions weakened. |
| ui/litellm-dashboard/src/lib/http/schema.d.ts | Adds default_litellm_params and optional_pre_call_checks to UpdateRouterConfig schema type. |
Reviews (6): Last reviewed commit: "fix(ui): surface a visible error when de..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
PR overviewThis PR adds support for default router parameters and surfaces optional check controls in the Admin UI. It also changes router callback handling related to model rate limits and budget limiting. There is one open security issue remaining after two have been addressed. The remaining concern is that disabling certain controls on one router can remove shared callback enforcement for other routers in the same process, allowing their rate or budget limits to be bypassed under multi-router deployments. The risk is conditional on shared-process router usage and control toggling behavior, but it affects enforcement boundaries between routers. Open issues (1)
Fixed/addressed: 2 · PR risk: 4/10 |
Contributor
…multi-select The Router Settings page rendered optional_pre_call_checks as free-text JSON, requiring admins to know and correctly type the exact valid check names. Add a dedicated multi-select populated from the field's known options (already returned by /router/fields), matching how routing_strategy already gets its own selector instead of a raw text field. The value now flows through React state (like routing_strategy/enable_tag_filtering) instead of the page's DOM-querySelector-based save mechanism, since an antd Select doesn't produce a plain named <input> for that mechanism to read. default_litellm_params keeps the raw-JSON editor since it has no fixed set of keys to offer as options.
… of /get/config/callbacks default_litellm_params is merged into every completion call's kwargs, so an operator can put a shared api_key or an Authorization header under extra_headers there. Router.get_settings() now returns it (needed so the Admin UI's Router Settings page can display/edit it), but /get/config/callbacks forwarded router_settings verbatim regardless of caller role - unlike the callback and alerting env vars on the same response, which already redact for non-full-admin callers (e.g. PROXY_ADMIN_VIEW_ONLY). That let a read-only admin read another admin's upstream provider credentials. Add the same role gate used for callback/alerting env vars, scoped to default_litellm_params via the existing SensitiveDataMasker so any key/secret/token/auth-shaped field is masked for non-full-admin callers while full admins keep seeing the real value.
…s/optional_pre_call_checks
_add_router_settings_from_db_config merges config.yaml router_settings with
the DB router_settings row and calls update_settings(**combined) directly,
without going through UpdateRouterConfig's exclude_none filtering. An
explicit `default_litellm_params: null` or `optional_pre_call_checks: null`
in either source therefore reached the new elif branches verbatim:
`{**dict, **None}` and iterating `None` both raise TypeError, crashing
proxy startup / config sync.
Contributor
Author
|
@greptileai review |
Contributor
Author
|
@Veria review |
…omplexity, prettier - schema.d.ts was stale after adding default_litellm_params/optional_pre_call_checks to UpdateRouterConfig; applied the exact diff CI's schema-vs-spec check expects. - update_settings's two new elif branches pushed its cyclomatic complexity from 14 to 16, crossing ruff-strict.toml's max-complexity=15 budget. Replaced both branches with a single `var in _CUSTOM_UPDATE_SETTINGS_HANDLERS` dispatch (one branch instead of two) so adding a custom-handled setting doesn't grow this function's branch count per field; also switched the two new helper signatures to `X | None` per UP045. - prettier --write on the two test files flagged by frontend-lint.
…sted router_code_coverage.py detects test coverage via a static AST scan for literal `.method_name(` calls in test files, not real coverage instrumentation - it flagged _merge_default_litellm_params_setting and _apply_optional_pre_call_checks_setting as untested even though they're exercised through update_settings(default_litellm_params=...) / (optional_pre_call_checks=...) in test_router.py, matching the existing _merge_tools_from_deployment / _invalidate_access_groups_cache precedent for private helpers only called indirectly.
update_settings(optional_pre_call_checks=...) only ever unioned incoming checks into self.optional_pre_call_checks - it never removed anything absent from the incoming list. Unchecking a check in the Admin UI's new multi-select and clicking Save silently did nothing live: the DB got the smaller list, but the router kept the old value and its registered callback (e.g. PromptCachingDeploymentCheck, RouterBudgetLimiting) active until a restart, diverging from what the UI showed as saved. _remove_optional_pre_call_checks mirrors add_optional_pre_call_checks for the removal direction: clears the relevant flag on the shared DeploymentAffinityCheck / EncryptedContentAffinityCheck instance for the affinity-based checks, and unregisters the dedicated callback (via the existing logging_callback_manager.remove_callbacks_by_type) for prompt_caching, enforce_model_rate_limits, and router_budget_limiting. optional_pre_call_checks is now set to exactly the incoming list rather than a strictly-growing union.
Extend the removal regression tests to exercise enforce_model_rate_limits (alongside prompt_caching/router_budget_limiting), all three DeploymentAffinityCheck flags including the loop's skip-non-matching-callback path (a mixed optional_callbacks list with prompt_caching present), and the separate EncryptedContentAffinityCheck flag. Also drop the dead optional_callbacks-is-None guard in _remove_optional_pre_call_checks: it can never be None by the time removed_checks is non-empty, since anything in self.optional_pre_call_checks only got there via add_optional_pre_call_checks, which always initializes the list first.
…nfigured Router.__init__ auto-enables router_budget_limiting whenever a deployment has max_budget/budget_duration set or provider_budget_config is configured (RouterBudgetLimiting.should_init_router_budget_limiter), independent of what optional_pre_call_checks explicitly lists. _remove_optional_pre_call_checks didn't account for that: a save (via the Admin UI's new multi-select, or a config-sync payload) that simply omitted "router_budget_limiting" from the list would unregister the RouterBudgetLimiting callback and null out router_budget_logger, silently letting deployments keep serving requests after their configured budget is exhausted. _remove_optional_pre_call_checks now checks should_init_router_budget_limiter before actually removing the callback for this one check, and returns the checks it kept active despite being in removed_checks so _apply_optional_pre_call_checks_setting can fold them back into the tracked optional_pre_call_checks list - keeping the UI's displayed state honest about what's still enforced.
Contributor
Author
|
@greptileai review |
…he UI It's a literal in the OptionalPreCallChecks type union, but add_optional_pre_call_checks has no handler for it - selecting it from the Admin UI's new multi-select would save successfully and show as enabled while the router does nothing with it. Drop it from optional_pre_call_checks' exposed options until it's actually implemented.
…a merge
update_settings(default_litellm_params=...) merged the incoming dict into the
existing one ({**old, **new}), which can only add or overwrite keys, never
remove one. Since the Admin UI reads and re-submits the entire
default_litellm_params object as a single JSON blob, a merge meant clearing a
field (e.g. removing cache_control_injection_points) by editing it out of the
UI's textarea and saving had no effect: the old key survived the merge and
stayed visible from /get/config/callbacks and active on the live router.
Replace wholesale instead, matching how every other dict-shaped router
setting (e.g. model_group_alias) is already handled via the generic setattr
path - callers are expected to submit the complete desired object, which the
UI already does by round-tripping the full current value.
Also drop explanatory comments/docstrings added earlier in this branch that
weren't requested, per this repo's no-unrequested-comments convention.
Contributor
Author
|
@greptileai review |
Contributor
Author
|
@Veria review |
default_litellm_params.cache_control_injection_points was only editable as
raw JSON on the Router Settings page, requiring an admin to hand-write
[{"location": "message", "role": "system"}] to enable prompt cache routing -
clunky next to the structured Switch + row editor already used for the same
field on the Add Model page.
Extracted the row editor (location/role/index inputs, add/remove) out of
add_model/cache_control_settings.tsx into a form-agnostic shared component,
CacheControlInjectionPointsEditor, driven by plain value/onChange props
instead of antd Form bindings. cache_control_settings.tsx now delegates to it
(and picks up a real fix along the way: its role/index onChange handlers read
form.getFieldValue("cache_control_points"), a field that was never
registered under that name, so those edits silently never synced into
litellm_extra_params - now reads the correct "cache_control_injection_points"
field via Form.useWatch).
New DefaultLitellmParamsSection renders that same editor for Router Settings
plus a JSON textarea for the remaining default_litellm_params keys
(timeout, max_retries, metadata, ...). Fully controlled through React state
(matching the optional_pre_call_checks pattern) rather than the page's
generic DOM-read save path, and excluded from ReliabilityRetriesSection's
raw-JSON rendering so it isn't shown twice.
Could not visually verify in a live browser this session (a chrome-extension
tooling conflict blocked screenshots/typing); verified via 174 passing
frontend tests covering both components and their Add Model / Router
Settings integrations, plus a clean tsc typecheck.
Contributor
Author
|
@greptileai review |
Contributor
Author
|
@Veria review |
…invalid Invalid JSON typed into the Default LiteLLM Params textarea was silently swallowed on blur (console.error only, no on-screen indication), so an admin could think their edit was saved when it was actually discarded. Now flags the field with an error state and shows a warning notification, and clears both once the field is edited again.
Contributor
Author
|
@greptileai review |
Contributor
Author
|
@Veria review |
Contributor
|
QA update for commit Result: passed The DB-backed proxy returned an existing Router Settings rendered Cache Control Injection Points from frontend-owned definitions while keeping the raw Default LiteLLM Params input absent. Optional Pre-call Checks and Reliability & Retries remained visible I changed row 1 to Role Postgres retained the hidden marker, JSON-null timeout, and
Full QA report with screenshots Focused Vitest passed 42 tests. Focused ESLint had 0 errors. Production The remaining OSV failure reports |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| retained_checks.append(check) | ||
| continue | ||
| litellm.logging_callback_manager.remove_callbacks_by_type(optional_callbacks, callback_type) | ||
| litellm.logging_callback_manager.remove_callbacks_by_type(litellm.callbacks, callback_type) |
Contributor
There was a problem hiding this comment.
Medium: Cross-router security controls are removed
remove_callbacks_by_type removes all matching instances from litellm.callbacks, including callbacks owned by other Router objects. If one router disables enforce_model_rate_limits or router_budget_limiting, users of another router in the same process can exceed that router's configured limits; capture this router's matching callback objects and remove only those exact objects from the global list.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review)
Screenshots / Proof of Fix
Tested commit
d418752267through the Admin UI against a DB-backed local proxy and PostgresThe backend precondition contained
default_litellm_params.qa_marker = "pr33144_index_fixed_e1e233f"and two cache-control rows. Open Router Settings as a full Admin and confirm Cache Control Injection Points renders those rows while the raw Default LiteLLM Params input remains absentChange the first row to Role
Assistant, type Index-4, increment it to-3, then add a third row with RoleUserand Index0. Click Save Changes, expect the success notification, then hard reload and confirm the exact three rows persistAfter save, Postgres retained the hidden marker, JSON-null timeout, and
max_retries: 0while persisting the three edited cache-control rows. Optional Pre-call Checks retainedprompt_caching, and Reliability & Retries remained renderedThe annotated recording and screenshots are in the QA comment
Type
New Feature
Bug Fix
Changes
Router.get_settings()andRouter.update_settings()now round-tripdefault_litellm_paramsandoptional_pre_call_checksthrough DB-backed configuration. Default parameters use replacement semantics, removed optional checks are unregistered, automatic budget enforcement remains active when required, and non-admin config responses mask sensitive default valuesThe Router Settings UI owns the Optional Pre-call Checks contract instead of querying backend metadata to decide whether it renders. Its choices, labels, and documentation link are frontend-defined
The raw Default LiteLLM Params JSON input is intentionally not exposed, so visiting the page cannot prefill or accidentally configure arbitrary defaults. Cache Control Injection Points remains rendered through a dedicated frontend-owned component with frontend-defined roles, structure, copy, and validation
Untouched
default_litellm_paramsis omitted from unrelated saves. Explicit cache-control edits send the updated object, while explicit disabling removes onlycache_control_injection_pointsand preserves unrelated default parametersThe shared cache-control editor remains available to Add Model. Its Ant Design
InputNumbersupports direct integer typing, negative values, increment and decrement controls, responsive row wrapping, and accessible removalFocused frontend regressions verify cache-control rendering without the raw JSON input, no mutation on initial render, hidden-value preservation, explicit enable and disable saves, and frontend-owned Optional Pre-call Checks. Backend coverage verifies update and removal behavior, budget enforcement, masking, and config responses
Final Attestation
Link to Devin session: https://app.devin.ai/sessions/7c06b4a26d2e42fe84440cc5538b1d51
Requested by: @krrish-berri-2