feat(routing): rename policies in place, and stop them hiding candidate prices - #560
Conversation
…te prices
Two changes to the routing surface that were in flight together.
Rename: POST /v1/routing/policies takes `rename_from`, so an edit that both
renames a policy and re-targets it cannot land half applied, leaving the old
name serving the new spec. The new name is validated exactly as a fresh one is,
because a rename can walk a policy into every collision a create can. The
dashboard's Edit form exposes it, and the row moves rather than being copied.
Pricing: a policy no longer withholds its candidates from GET /v1/models. Every
selector of every policy in force was hidden, `on_failure` chains included, so
one failover policy could empty most of a catalogue, and a candidate priced by
the genai-prices fallback then disappeared from the dashboard together with its
rate. GET /v1/models/{key} never withheld them, so nothing was really being kept
off the wire. Alias targets are still withheld: an alias exists to stand in for
a target, a policy does not.
POST /v1/pricing now refuses a policy name the way it already refuses an alias
name, naming the candidates to price instead. Pricing, budgets, and usage key on
the model a request resolves to, so a row stored under the policy name was
written and never read.
The dashboard rebuilds rows the catalogue withheld from the discovery endpoint.
Those now report "rate unknown" instead of "not priced" when default pricing is
on, since the gateway does meter such a model; with the fallback off, unpriced
remains the truth.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughChangesThe PR adds scoped routing-policy renaming through the API and dashboard. It preserves policy rows and scopes, exposes policy candidates in model discovery, rejects policy names for pricing, and updates pricing displays, tests, and documentation. Routing policy behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
…me race Review follow-ups on the rename path, plus two dashboard notes. `rename_from` now keys the 404 on the field being sent rather than on the name having changed. Sending it asserts that policy is stored, so `rename_from` equal to `name` with no row behind it is answered instead of quietly becoming a create: an edit form whose row was deleted underneath it would otherwise resurrect the policy rather than report it gone. An unchanged name whose row does exist stays a plain update, which is what a form that always sends the field needs. The name check and the commit are not one atomic step, so a concurrent writer can take the target name in between and leave the unique constraint to catch it. That surfaced as "Database error". It now re-reads the name and answers with the same 409 the pre-check would have given, falling through to the 500 otherwise, because the same constraint class also covers the user foreign key and reporting a deleted user as a name clash would send the operator after the wrong thing. Dashboard: the unpriced label for a discovery-only row now waits for the settings answer instead of reading "not priced" while the flag is still undefined, and its comment covers the second way such a row arises, `model_discovery` being off, not only a withheld alias target. Also covers the pricing refusal for a user-scoped policy name, the one branch that cannot name the candidates, and frees the e2e rename target so a re-run against a warm database does not fail on its own leftovers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/gateway/api/routes/routing.py (1)
311-318: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd
limit(1)to the existence check.
_name_is_takenonly needs to know whether a row exists. The coding guidelines ask forlimit(1)on existence checks. It also removes any theoreticalMultipleResultsFoundrisk if a future migration relaxes the unique constraint.♻️ Proposed refactor
existing = ( await db.execute( - select(RoutingPolicy.id).where(RoutingPolicy.name == name, RoutingPolicy.user_id == user_id) + select(RoutingPolicy.id) + .where(RoutingPolicy.name == name, RoutingPolicy.user_id == user_id) + .limit(1) ) ).scalar_one_or_none() return existing is not NoneAs per coding guidelines: "Push filtering, sorting, counting, and aggregation into SQL; use
func.count()instead of loading rows and callinglen(), and uselimit(1)for existence checks."🤖 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 `@src/gateway/api/routes/routing.py` around lines 311 - 318, Update the query in _name_is_taken to apply limit(1) before execution, while preserving the existing name and user_id filters and scalar existence result.Source: Coding guidelines
web/src/pages/RoutingPage.tsx (1)
393-397: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider always sending
rename_fromwhen editing a policy.The form omits
rename_fromwhen the name is unchanged. The colocated test explains the reason: a plain spec edit should not read as a rename. That reasoning is sound, so this is a judgment call rather than a defect.The tradeoff is on the other side. The backend treats
rename_fromas an assertion that the row exists, including when it equalsname(seeset_policyinsrc/gateway/api/routes/routing.py, and the testtest_rename_from_never_falls_back_to_creating_the_policy). If another operator deletes the policy while this edit form is open, the current payload re-creates it instead of reporting a 404. Sendingrename_from: previousNamefor every policy edit would surface the deletion.If you keep the current behavior, no change is needed. The server log already distinguishes the two cases, because it records
renamed_from=-when the name did not change.♻️ Optional change
- save.mutate( - { name: name.trim(), spec, user_id: scope, ...(renaming ? { rename_from: previousName } : {}) }, - { onSuccess: onClose }, - ); + save.mutate( + { name: name.trim(), spec, user_id: scope, ...(editing ? { rename_from: previousName } : {}) }, + { onSuccess: onClose }, + );Also applies to: 495-498
🤖 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 `@web/src/pages/RoutingPage.tsx` around lines 393 - 397, Optional: update the policy edit payload construction around the renaming logic and its corresponding use near the second occurrence so every non-alias edit sends rename_from using previousName, including when the name is unchanged. Preserve aliases without rename_from and ensure policy edits assert the original row rather than falling back to creation.
🤖 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.
Nitpick comments:
In `@src/gateway/api/routes/routing.py`:
- Around line 311-318: Update the query in _name_is_taken to apply limit(1)
before execution, while preserving the existing name and user_id filters and
scalar existence result.
In `@web/src/pages/RoutingPage.tsx`:
- Around line 393-397: Optional: update the policy edit payload construction
around the renaming logic and its corresponding use near the second occurrence
so every non-alias edit sends rename_from using previousName, including when the
name is unchanged. Preserve aliases without rename_from and ensure policy edits
assert the original row rather than falling back to creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96d699bd-f189-40b5-a21f-f861cf7c703d
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (15)
docs/api-reference.mddocs/dashboard.mddocs/models.mddocs/public/otari.postman_collection.jsondocs/routing.mdsrc/gateway/api/routes/models.pysrc/gateway/api/routes/pricing.pysrc/gateway/api/routes/routing.pytests/integration/test_routing_policies.pyweb/e2e/dashboard.spec.tsweb/src/api/types.tsweb/src/pages/ModelsPage.test.tsxweb/src/pages/ModelsPage.tsxweb/src/pages/RoutingPage.test.tsxweb/src/pages/RoutingPage.tsx
njbrake
left a comment
njbrake
left a comment
There was a problem hiding this comment.
Note: this review was drafted by Claude Opus 5 via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is the model's.
Approving. Heads up that I pushed a second commit to your branch (7204cdd9) instead of sending it back for round trips on small things. Take a look and say so if you disagree with any of it.
Two of them were real, both on the rename path:
rename_fromequal tonamewith no stored row created the policy instead of 404ing, because the 404 sat behindrenaming. An edit form whose row was deleted underneath it would resurrect the policy rather than report it gone. The 404 now keys on the field being sent at all, and the unchanged-name upsert your test pins still works.- A rename that lost the race between
_name_is_takenand the commit surfaced asDatabase error. It now catchesIntegrityErrorand re-reads the name to answer with the same 409. Re-reading rather than mapping the exception straight to 409, because theuser_idforeign key raises the same class and calling a deleted user a name clash sends the operator after the wrong thing.
Both carry regression tests that fail without the fix. The rest is minor: a coverage test for the one pricing branch that cannot name candidates, the unpriced label waiting for the settings fetch instead of claiming "not priced" while the flag is still undefined, a comment, and freeing the e2e rename target so a re-run against a warm database does not trip on its own leftovers.
On the half that stops policies withholding candidates: I checked that GET /v1/models/{key} only ever consulted effective_aliases, so those candidates were served with their prices all along and the listing was withholding from the dashboard without protecting anything. The key allowlist still filters every listed row. Good change, and the docs carry the reasoning rather than just the new behavior.
4 participants
Description
Two changes to the routing surface that were in flight together.
Rename: POST /v1/routing/policies takes
rename_from, so an edit that bothrenames a policy and re-targets it cannot land half applied, leaving the old
name serving the new spec. The new name is validated exactly as a fresh one is,
because a rename can walk a policy into every collision a create can. The
dashboard's Edit form exposes it, and the row moves rather than being copied.
Pricing: a policy no longer withholds its candidates from GET /v1/models. Every
selector of every policy in force was hidden,
on_failurechains included, soone failover policy could empty most of a catalogue, and a candidate priced by
the genai-prices fallback then disappeared from the dashboard together with its
rate. GET /v1/models/{key} never withheld them, so nothing was really being kept
off the wire. Alias targets are still withheld: an alias exists to stand in for
a target, a policy does not.
POST /v1/pricing now refuses a policy name the way it already refuses an alias
name, naming the candidates to price instead. Pricing, budgets, and usage key on
the model a request resolves to, so a row stored under the policy name was
written and never read.
The dashboard rebuilds rows the catalogue withheld from the discovery endpoint.
Those now report "rate unknown" instead of "not priced" when default pricing is
on, since the gateway does meter such a model; with the fallback off, unpriced
remains the truth.
PR Type
Relevant issues
None.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).make lint,make typecheckand the fullpytest tests/unit tests/integrationrun are green (2718 passed, 10 skipped). The API contract changed, sodocs/public/openapi.jsonand the Postman collection were both regenerated;make openapi-checkandmake postman-checkpass. Dashboard side:tsc -b --noEmitclean and the full Vitest suite green (585 passed), plus an e2e spec for the rename flow.AI Usage
AI Model/Tool used:
Claude Opus 5, via Claude Code.
Any additional AI details you'd like to share:
The change was written and validated with Claude Code. It was previously part of a
longer branch of mine; that branch has been rebased onto current
mainand split soeach PR carries one self-contained change, reviewable on its own.
Summary