Conversation
…, and enforced guardrails Turns an alias into a policy: a model name callers send, which decides which real model serves the request, what is tried after a retryable failure, and which guardrails always run. An alias is the one-target case, so `aliases:` keeps working as its shorthand. Closes the gap where a standalone gateway had no failover at all: any provider blip was a 502. Two axes, kept separate on purpose. `select` decides where the plan starts (a static target, a condition, or a router); `on_failure` is what is tried after a failure. Collapsing them would make "did this entry not apply, or did it fail?" ambiguous in every log line. The fallthrough is an explicit `default` rather than a positional last entry, so a misordered policy is refused instead of silently having dead rules. What is here: * A credential-agnostic attempt walker (`api/routes/_attempts.py`) plus an `Attempt` type in `gateway/types/`. It carries `instance` and `provider` separately, because pricing, budgets, and the usage log key on the instance while any-llm is dispatched against the implementation; collapsing them would silently re-key `usage_logs.provider` and break pricing for named instances. `kwargs` is an opaque dict rather than a required `api_key`, since a locally resolved attempt may have no API key at all (Vertex AI). * Config and stored policies, with the same scoping and precedence as aliases (`user-scoped > config.yml > global stored`), a `routing_policies` table, and `/v1/routing/policies` CRUD plus an `explain` dry run. Every schema model is `extra="forbid"`: GatewayConfig is `extra="ignore"`, so a typo'd key inside a policy would otherwise vanish and the policy would quietly not do what it says. * Budget-conditional selection, evaluated before the reservation. A threshold at or above 100 is refused at load: the budget gate rejects the request before selection, so such a rule could never fire. * Policy guardrails, with `mode` required (the request-level field defaults to monitor, so an omitted mode would look like a mandate and behave as shadow mode) and a new `on_unavailable` escape hatch for the fail-closed behavior. * Attribution on usage rows (`policy_name`, `selection_reason`, `attempt_position`, `attempt_count`, `request_group_id`). A recovered attempt is recorded as `status="absorbed"`, never `error`, and is excluded from `request_count` as well: every error metric counts `status == "error"` exactly, so a working fallback chain must not read as an outage. * A reservation top-up before each later candidate, so a chain that falls over to a pricier model cannot take spend past a cap the gate already approved. A refused top-up stops the chain rather than overshooting. * 401/403 are terminal for locally credentialed attempts, unlike hybrid mode: the operator owns these keys, so failing over would move traffic and spend to another provider and hide the misconfiguration. * A Routing page in the dashboard, `otari routing explain`, and docs/routing.md. Verified against a live gateway and real OpenAI traffic, not only tests: a 404 head fails over to a working model and writes two correlated rows (absorbed + success), an exhausted chain writes both rows and a 502, conditional selection picks per user, and the counts stay honest (12 rows, 2 absorbed, 10 requests, 5 errors). Deliberately not included: the kNN router (#187) is accepted by the schema and currently warns and falls through to the default; policy-shaped allow-list grants; output-direction guardrails. Hybrid mode refuses a policy name with a 400, since the platform resolves the model there. Refs #463 Refs #401 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflicts were confined to the committed dashboard bundle, which is build output: resolved by deleting it and rebuilding from the merged sources, so it matches web/src rather than either side's stale copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 4, 2026 23:58 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
the
missing-template
|
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:
WalkthroughChangesRouting policy feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (14)
src/gateway/api/routes/routing.py (2)
238-248: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a pagination bound to
list_policies.
select(RoutingPolicy)fetches every row with no limit. In practice this table stays small because only an operator writes to it, so this is unlikely to bite soon. The house rule is unconditional for list endpoints, though, and a deployment that scopes a policy per user can grow this table faster than expected.Adding
limit/offsetquery parameters with a default and a maximum, matching the other management list routes, keeps this endpoint predictable.As per coding guidelines: "Every list endpoint must have a sane default and maximum pagination bound; never select an unbounded growing table such as
UsageLogorModelPricing."🤖 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 238 - 248, Update the list_policies endpoint to accept limit and offset query parameters using the same sane defaults and maximum bounds as other management list routes. Apply both bounds to the RoutingPolicy query before execution while preserving its existing name ordering and response behavior.Source: Coding guidelines
371-376: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
explain_policynever usesdb.The handler declares
db: Annotated[AsyncSession, Depends(get_db)]but resolves everything fromconfigand the policy cache. FastAPI will still open and close a session for every call. Removing the parameter drops that cost and makes the "no dispatch, no database" contract in the docstring visible in the signature.If a future revision needs the session for budget lookups, adding it back is a one-line change.
🤖 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 371 - 376, Remove the unused db dependency parameter from explain_policy and its associated AsyncSession/get_db imports only if they become unused; retain config injection and all existing response behavior. This makes the endpoint avoid creating a database session when explaining policies.src/gateway/services/routing/compiler.py (2)
162-164: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer an explicit guard over
assertfor the schema invariant.
assertstatements are removed when Python runs with-O. If that happens and awhenentry ever reaches here without a target,Noneflows intoresolve_provider_selectorinstead of failing loudly. The schema does enforce the invariant today, so this is defensive rather than an active bug.♻️ Proposed fix
- if entry.when is not None and _matches(entry.when, user_id=user_id, key_id=key_id, budget=budget): - assert entry.target is not None # schema: a `when` entry always carries a target - return entry.target, f"condition:{','.join(entry.when.conditions())}" + if ( + entry.when is not None + and entry.target is not None + and _matches(entry.when, user_id=user_id, key_id=key_id, budget=budget) + ): + return entry.target, f"condition:{','.join(entry.when.conditions())}"🤖 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/services/routing/compiler.py` around lines 162 - 164, Replace the assert in the conditional-entry branch of the routing compiler with an explicit runtime guard that verifies entry.target is not None before returning it. Preserve the existing condition label and return behavior for valid targets, while failing loudly when a matching when entry lacks a target, including when Python runs with optimizations.
153-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rate-limiting the router warning.
compile_policyruns per request, so a policy that names a router logs a WARNING on every single request that uses it. The routing behavior is correct and the fallthrough is the right choice. The volume is the concern: a busy policy will bury the rest of the log, and the message tells the operator nothing new after the first time.A one-time warning at config validation or policy load, plus a debug-level line here, keeps the signal without the flood.
🤖 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/services/routing/compiler.py` around lines 153 - 161, Reduce repeated WARNING logs in compile_policy for unavailable routers: move the operator-facing warning to configuration validation or policy loading so it is emitted once, and change the per-request logger.warning in the entry.router branch to a debug-level message while preserving the existing fallthrough behavior.src/gateway/services/policy_store.py (1)
44-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
all_policy_namesto__all__.
all_policy_namesis imported bysrc/gateway/api/routes/routing.py(line 34), so it is part of this module's public surface. The__all__list omits it, which makes the declared surface disagree with real usage.♻️ Proposed fix
__all__ = [ "POLICY_CACHE_TTL_SECONDS", + "all_policy_names", "cached_policies", "effective_policies",🤖 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/services/policy_store.py` around lines 44 - 54, Add the existing all_policy_names symbol to the __all__ list in the policy store module, alongside the other exported policy helpers, so its declared public surface matches its import from the routing module.scripts/check_architecture.py (1)
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment claims more than the rule enforces.
The comment states that shared types "may not import any other gateway layer", but
gateway.modelsis absent fromforbidden, so a module undergateway/typescan import it and the check stays green. Either addgateway.modelsto the list, or narrow the comment to name the layers that are actually enforced. Right now a reader trusts the comment and a future type quietly grows a pydantic dependency.No behavior change is needed today; both current type modules import only the standard library and
any_llm.🤖 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 `@scripts/check_architecture.py` around lines 68 - 77, The gateway/types architecture rule’s comment claims all gateway layers are forbidden, but its forbidden list omits gateway.models. Update the “gateway/types” entry so the documented restriction matches enforcement by adding gateway.models to forbidden, without changing unrelated rules.src/gateway/api/routes/models.py (1)
298-309: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRead the policy map once, then derive both values from it.
The comment on line 413 already states the rule for aliases: read once, so the withheld set and the listed names agree even if a write lands between the two reads.
_policy_target_keysbreaks that rule for policies, because it callseffective_policiesa second time. If a policy is created or deleted between line 419 and line 427, the catalog can list a policy whose candidates were not withheld, or withhold candidates for a policy it no longer lists.The cache makes this cheap and rare, so this is about the invariant rather than cost. Passing the already-resolved mapping in also removes the duplicate lookup.
♻️ Suggested shape
-def _policy_target_keys(config: GatewayConfig, caller_user_id: str | None) -> set[str]: +def _policy_target_keys(config: GatewayConfig, policies: dict[str, PolicySpec]) -> set[str]: """Canonical pricing keys of every selector any policy in force can reach. Withheld from the listing for the same reason alias targets are: a policy exists partly so the provider/model behind it stays private, and that has to hold for its fallback candidates too, not just its default. """ return { normalize_pricing_key(config, selector) - for spec in effective_policies(config, caller_user_id).values() + for spec in policies.values() for selector in spec.static_selectors() }
_policy_catalog_entrieswould return the resolved mapping alongside the split, or the caller resolveseffective_policiesonce and passes it to both helpers.Also applies to: 419-427
🤖 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/models.py` around lines 298 - 309, Resolve effective_policies once in the policy catalog flow and reuse that mapping for both catalog entries and _policy_target_keys. Update the relevant helper signatures and caller, including _policy_catalog_entries if needed, so policy listing and withheld target derivation operate on the same snapshot.src/gateway/services/guardrails.py (1)
206-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the docstring to match the new two-field decision.
The logic is right, and keeping
block/blockas the default preserves the old behavior. The function docstring above still states thatblockguardrails always fail closed and thatGuardrailsNotReachableErroris raised for anyblock-mode guardrail. Withon_unavailable="monitor"ablockguardrail now fails open, so theRaises:section is no longer accurate. A future reader debugging a served request during a guardrails outage will read the docstring first.The
Failure handlingbullets and theRaises:line need theon_unavailablecondition added.🤖 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/services/guardrails.py` around lines 206 - 214, Update the docstring for the function containing the GuardrailsNotReachableError handler to describe fail-closed behavior only when both cfg.mode and cfg.on_unavailable are "block"; revise the Failure handling bullets and Raises section accordingly, while documenting that other on_unavailable settings fail open.web/src/pages/RoutingPage.test.tsx (1)
84-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for scoped delete, and let the mock honor
user_id.The DELETE branch resolves the name from the path and filters
listby name alone, so theuser_idquery parameter is ignored. No test currently deletes anything, which means the mock cannot catch the subtlest rule inuseDeleteRoutingPolicy: only a null or absentuserIdmeans global scope, because""is a legal user id. That rule is exactly the kind that a later refactor turns into a truthiness check, and then a global policy gets deleted instead of a user-scoped one.A short test that renders a user-scoped row, presses Delete, and asserts the request URL carries
?user_id=alicewould pin the behavior down.♻️ Proposed mock change so scope is observable
if (method === "DELETE") { - const name = decodeURIComponent((url.split("?")[0].split("/").pop() ?? "")); - list = list.filter((item) => item.name !== name); + const [path, query] = url.split("?"); + const name = decodeURIComponent(path.split("/").pop() ?? ""); + const scoped = new URLSearchParams(query ?? "").get("user_id"); + list = list.filter((item) => item.name !== name || item.user_id !== scoped); return new Response(null, { status: 204 }); }🤖 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.test.tsx` around lines 84 - 96, Update the DELETE branch of the routing-policy mock to read and honor the user_id query parameter, distinguishing null/absent global scope from any provided value, including an empty string, when filtering by policy name and scope. Add a test covering deletion of a user-scoped policy that renders the row, presses Delete, and verifies the request URL includes ?user_id=alice.src/gateway/static/dashboard/assets/index-BmxAZYrI.css (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialIgnore the generated dashboard bundle where lint runs
The build output under
src/gateway/static/dashboardis meant to be committed, and machine-generated Tailwind/HeroUI CSS should not fail CI on every rebuild. If this project configures a stylelint check, addsrc/gateway/static/dashboard/**to.stylelintignoresoweb/srcCSS stays the signal.🤖 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/static/dashboard/assets/index-BmxAZYrI.css` at line 1, Add src/gateway/static/dashboard/** to the project’s .stylelintignore configuration so generated Tailwind/HeroUI assets such as the dashboard CSS bundle are excluded from stylelint while web/src styles continue to be checked.Source: Linters/SAST tools
tests/unit/test_attempt_walker.py (1)
48-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo new walker parameters have no unit coverage.
The harness never exercises
on_absorbedorbuild_kwargs, and both carry real meaning.on_absorbeddecides which failures becomestatus="absorbed"audit rows, and the walker deliberately skips it for the last attempt.build_kwargsis how the responses format rebuilds per-candidate kwargs; a regression there would send one provider's arguments to another. The fast tier is the right place to pin both.💚 Suggested tests
`@pytest.mark.asyncio` async def test_absorbed_is_called_for_recovered_attempts_only() -> None: """The terminal failure is the request's own outcome; the caller logs that one.""" absorbed: list[int] = [] async def run_attempt(attempt: Attempt, call_kwargs: dict[str, Any], mark_locked_in: Any) -> Any: raise _http_error(503) async def on_absorbed(attempt: Attempt, exc: BaseException, total: int) -> None: absorbed.append(attempt.position) with pytest.raises(HTTPException): await walk_attempts( attempts=[_attempt(1, "a"), _attempt(2, "b")], base_request_fields={}, run_attempt=run_attempt, max_tool_iterations=10, on_absorbed=on_absorbed, ) assert absorbed == [1] `@pytest.mark.asyncio` async def test_build_kwargs_runs_per_candidate() -> None: """The transformation must apply to the candidate being tried, not the one that failed.""" seen: list[str] = [] def build_kwargs(attempt: Attempt, fields: dict[str, Any]) -> dict[str, Any]: return {**fields, "provider": attempt.instance, "model": attempt.model} async def run_attempt(attempt: Attempt, call_kwargs: dict[str, Any], mark_locked_in: Any) -> Any: seen.append(call_kwargs["model"]) if attempt.position == 1: raise _http_error(503) return "ok" await walk_attempts( attempts=[_attempt(1, "a"), _attempt(2, "b", instance="anthropic")], base_request_fields={}, run_attempt=run_attempt, max_tool_iterations=10, build_kwargs=build_kwargs, ) assert seen == ["a", "b"]🤖 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 `@tests/unit/test_attempt_walker.py` around lines 48 - 70, Add fast-tier unit tests covering both new walker hooks: verify on_absorbed is called only for recovered failures and skipped for the terminal attempt, and verify build_kwargs is invoked per candidate so each attempt’s provider/model arguments reach run_attempt after fallback.tests/integration/test_routing_policies.py (1)
770-784: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test passes for the wrong reason on one nearby case.
Here every candidate returns 503, so the walk really does reach position 2 and
attempt_position == 2is correct. The gap is the terminal-failure case: a 400 or 401 on candidate 1 stops the walk immediately, but the error row is still attributed toattempts[-1](see the_failure_attributioncomment onsrc/gateway/api/routes/_pipeline.py). A second test would catch that, and would fail today:💚 Suggested additional test
def test_a_terminal_failure_is_attributed_to_the_candidate_that_failed(client: TestClient) -> None: """A 401 stops the chain at candidate 1, so the row must name candidate 1.""" _create_user(client) with patch("gateway.api.routes.chat.acompletion", new=AsyncMock(side_effect=_http_error(401))): _chat(client, "fast") errors = [r for r in _usage_rows(client) if r["status"] == "error"] assert len(errors) == 1 assert errors[0]["attempt_position"] == 1 assert errors[0]["model"] == "gpt-5-mini" assert errors[0]["provider"] == "openai"🤖 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 `@tests/integration/test_routing_policies.py` around lines 770 - 784, The existing test does not cover terminal failures that stop routing at the first candidate. Add a test alongside test_a_failed_request_still_says_which_policy_it_went_through that mocks a 401 response, verifies one error usage row, and asserts attempt_position is 1 with the expected model and provider, so _failure_attribution handles the candidate that actually failed rather than the final configured attempt.tests/unit/test_pipeline_settlement.py (1)
75-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider letting
_ctxbuild a plan-shaped context.This block is the fast tier for settlement, and its own header says the merged executor must preserve these behaviors. Right now no test here builds a
RequestContextwith aplan, so the failover branch and the settlement rules that matter most for routing are covered only in the Postgres-backed integration tier:
- exactly one reconcile for a request that fell over,
- exactly one refund when the plan is exhausted,
- a refused reservation top-up stopping the chain instead of serving the pricier candidate.
Adding
plan: CompiledPlan | None = Noneandrequest_group_idto_ctxwould make those reachable here, where they run in milliseconds.🤖 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 `@tests/unit/test_pipeline_settlement.py` around lines 75 - 95, Extend the test helper _ctx to accept optional plan and request_group_id parameters and pass them into RequestContext, using the existing CompiledPlan type and appropriate default. Add fast-tier unit coverage for failover settlement: exactly one reconcile, one refund when the plan is exhausted, and stopping the chain when reservation top-up is refused.web/src/pages/ActivityPage.tsx (1)
879-902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Routing and absorbed-status coverage to
ActivityPage.test.tsx.
Routingis a new column, andstatus: "absorbed"is a new status pill, but the existing tests still do not assert either. Add two small row cases that mock usage rows, then query the rendered text or roles: one unrouted row must have no routing text, and one routed row must show theattempt 2/2attribution.🤖 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/ActivityPage.tsx` around lines 879 - 902, Add focused cases in ActivityPage.test.tsx covering the new Routing column and absorbed status pill: mock an unrouted usage row and assert it has no routing text, then mock a routed row with status "absorbed" and assert the rendered UI exposes the absorbed status and “attempt 2/2” attribution. Use existing row-rendering helpers and query text or roles rather than implementation details.Source: Coding guidelines
🤖 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 `@docs/routing.md`:
- Around line 82-85: Update the routing documentation to remove the master-key
case from the “no budget configured” exception. Keep the explanation for callers
without a budget and unlimited budgets, while documenting that master-key
requests use the supplied user_id and may match a policy branch based on that
user’s budget.
In `@otari.db-shm`:
- Line 1: Remove the committed SQLite runtime artifacts otari.db-shm and
otari.db-wal from version control, and broaden the existing root .gitignore rule
for otari.db so these companion files are ignored as well. Keep otari.db itself
ignored and ensure future local runs cannot reintroduce any of the three
database files.
In `@scripts/seed_routing_demo.py`:
- Around line 4-6: Update the descriptive output in the seed script to say
“failover” and avoid guaranteeing an absorbed attempt row; state that fallback
and absorbed activity may appear only when the seeded traffic triggers a
retryable primary-model failure. Apply the same wording correction to the
corresponding description around the other affected output block.
In `@src/gateway/api/routes/_attempts.py`:
- Around line 189-206: Add import asyncio and update the exception handling
around the retry flow so asyncio.CancelledError is explicitly re-raised before
the broad BaseException classification and provider-error mapping in the visible
catch block. Keep timeout handling unchanged, relying on the existing Python
3.13+ TimeoutError behavior.
In `@src/gateway/api/routes/_pipeline.py`:
- Around line 2626-2668: Update the exhausted-plan flow so it records the actual
attempt where walk_attempts stopped, rather than always using
ctx.plan.attempts[-1]. Propagate that attempt through both exception-catching
call sites into log_exhausted_plan, and use it for _failure_attribution plus the
logged model and provider; preserve tail attribution only when the walker truly
reaches the final candidate.
In `@src/gateway/api/routes/messages.py`:
- Line 535: The streaming Messages request path is missing the base request
fields needed for failover. Update the streaming call near the non-streaming
request handling to pass base_request_fields=request_fields, matching the
existing non-streaming path and chat.py behavior so run_single_attempt_stream
can apply the on_failure chain.
In `@src/gateway/api/routes/models.py`:
- Around line 254-270: The ModelsPage pricing-source mapping must preserve
"dynamic" instead of collapsing it to "none" when pricing is null. Update the
model-to-PriceSource logic in ModelsPage.tsx to explicitly map or permit
model.pricing_source === "dynamic", and ensure the resulting PriceSource
rendering handles that value without assigning an alias label.
In `@src/gateway/models/routing.py`:
- Around line 133-152: Update _budget_thresholds_stay_under_the_cap to apply the
unreachable-threshold validation only when budget_used_pct uses the upward gte
or gt comparator and its value is at least 100. Allow lt and lte thresholds at
or above 100 to pass validation, while preserving the existing error and
behavior for unreachable upward thresholds.
In `@src/gateway/static/dashboard/assets/RoutingPage-DKobULws.js`:
- Line 1: The edit form in D currently rebuilds spec.select through se,
preserving only budget thresholds and the default while dropping router entries.
Update the select-state initialization and serialization around se and the $
useMemo so existing router entries remain intact when saving, while retaining
the current budget-threshold editing behavior; alternatively, prevent D from
editing policies containing router-backed select entries.
In `@web/src/pages/ActivityPage.tsx`:
- Around line 82-91: The Status filter UI should expose an “Absorbed” option
alongside All, Success, and Error, using the existing status-filter value and
query flow so it requests status=absorbed. Update the status select
definition/rendering in ActivityPage without changing activity row styling or
other filter behavior.
In `@web/src/pages/RoutingPage.tsx`:
- Around line 614-615: Make the adding and editing state transitions in
RoutingPage mutually exclusive: when opening a new PolicyForm, clear editing,
and when opening an existing policy for editing, clear adding. Update the
handlers used by the “New policy” action and the table’s Edit button, while
preserving the existing PolicyForm close behavior.
- Around line 201-214: Update the policy editor state and submit logic around
the PolicySpec useMemo so editing preserves all loaded spec fields, including
spec_version, limits, and select entries the form does not model. On save,
replace only the form-owned conditions/default target while carrying through
unrecognized select rules and existing metadata; ensure the Edit action for
stored policies cannot silently discard data.
---
Nitpick comments:
In `@scripts/check_architecture.py`:
- Around line 68-77: The gateway/types architecture rule’s comment claims all
gateway layers are forbidden, but its forbidden list omits gateway.models.
Update the “gateway/types” entry so the documented restriction matches
enforcement by adding gateway.models to forbidden, without changing unrelated
rules.
In `@src/gateway/api/routes/models.py`:
- Around line 298-309: Resolve effective_policies once in the policy catalog
flow and reuse that mapping for both catalog entries and _policy_target_keys.
Update the relevant helper signatures and caller, including
_policy_catalog_entries if needed, so policy listing and withheld target
derivation operate on the same snapshot.
In `@src/gateway/api/routes/routing.py`:
- Around line 238-248: Update the list_policies endpoint to accept limit and
offset query parameters using the same sane defaults and maximum bounds as other
management list routes. Apply both bounds to the RoutingPolicy query before
execution while preserving its existing name ordering and response behavior.
- Around line 371-376: Remove the unused db dependency parameter from
explain_policy and its associated AsyncSession/get_db imports only if they
become unused; retain config injection and all existing response behavior. This
makes the endpoint avoid creating a database session when explaining policies.
In `@src/gateway/services/guardrails.py`:
- Around line 206-214: Update the docstring for the function containing the
GuardrailsNotReachableError handler to describe fail-closed behavior only when
both cfg.mode and cfg.on_unavailable are "block"; revise the Failure handling
bullets and Raises section accordingly, while documenting that other
on_unavailable settings fail open.
In `@src/gateway/services/policy_store.py`:
- Around line 44-54: Add the existing all_policy_names symbol to the __all__
list in the policy store module, alongside the other exported policy helpers, so
its declared public surface matches its import from the routing module.
In `@src/gateway/services/routing/compiler.py`:
- Around line 162-164: Replace the assert in the conditional-entry branch of the
routing compiler with an explicit runtime guard that verifies entry.target is
not None before returning it. Preserve the existing condition label and return
behavior for valid targets, while failing loudly when a matching when entry
lacks a target, including when Python runs with optimizations.
- Around line 153-161: Reduce repeated WARNING logs in compile_policy for
unavailable routers: move the operator-facing warning to configuration
validation or policy loading so it is emitted once, and change the per-request
logger.warning in the entry.router branch to a debug-level message while
preserving the existing fallthrough behavior.
In `@src/gateway/static/dashboard/assets/index-BmxAZYrI.css`:
- Line 1: Add src/gateway/static/dashboard/** to the project’s .stylelintignore
configuration so generated Tailwind/HeroUI assets such as the dashboard CSS
bundle are excluded from stylelint while web/src styles continue to be checked.
In `@tests/integration/test_routing_policies.py`:
- Around line 770-784: The existing test does not cover terminal failures that
stop routing at the first candidate. Add a test alongside
test_a_failed_request_still_says_which_policy_it_went_through that mocks a 401
response, verifies one error usage row, and asserts attempt_position is 1 with
the expected model and provider, so _failure_attribution handles the candidate
that actually failed rather than the final configured attempt.
In `@tests/unit/test_attempt_walker.py`:
- Around line 48-70: Add fast-tier unit tests covering both new walker hooks:
verify on_absorbed is called only for recovered failures and skipped for the
terminal attempt, and verify build_kwargs is invoked per candidate so each
attempt’s provider/model arguments reach run_attempt after fallback.
In `@tests/unit/test_pipeline_settlement.py`:
- Around line 75-95: Extend the test helper _ctx to accept optional plan and
request_group_id parameters and pass them into RequestContext, using the
existing CompiledPlan type and appropriate default. Add fast-tier unit coverage
for failover settlement: exactly one reconcile, one refund when the plan is
exhausted, and stopping the chain when reservation top-up is refused.
In `@web/src/pages/ActivityPage.tsx`:
- Around line 879-902: Add focused cases in ActivityPage.test.tsx covering the
new Routing column and absorbed status pill: mock an unrouted usage row and
assert it has no routing text, then mock a routed row with status "absorbed" and
assert the rendered UI exposes the absorbed status and “attempt 2/2”
attribution. Use existing row-rendering helpers and query text or roles rather
than implementation details.
In `@web/src/pages/RoutingPage.test.tsx`:
- Around line 84-96: Update the DELETE branch of the routing-policy mock to read
and honor the user_id query parameter, distinguishing null/absent global scope
from any provided value, including an empty string, when filtering by policy
name and scope. Add a test covering deletion of a user-scoped policy that
renders the row, presses Delete, and verifies the request URL includes
?user_id=alice.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 21ecf7ea-8c50-4a00-9827-28882ff33829
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (77)
alembic/versions/e8b1d3f5a7c9_add_routing_policies.pyalembic/versions/f4c6a8b0d2e5_add_usage_logs_routing_attribution.pyconfig.example.ymldocs/configuration.mddocs/dashboard.mddocs/index.mddocs/models.mddocs/public/otari.postman_collection.jsondocs/routing.mdotari.db-shmotari.db-walscripts/check_architecture.pyscripts/seed_routing_demo.pysrc/gateway/api/main.pysrc/gateway/api/routes/_attempts.pysrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/chat.pysrc/gateway/api/routes/messages.pysrc/gateway/api/routes/models.pysrc/gateway/api/routes/responses.pysrc/gateway/api/routes/routing.pysrc/gateway/api/routes/usage.pysrc/gateway/cli.pysrc/gateway/core/config.pysrc/gateway/main.pysrc/gateway/models/entities.pysrc/gateway/models/guardrails.pysrc/gateway/models/routing.pysrc/gateway/services/budget_service.pysrc/gateway/services/guardrails.pysrc/gateway/services/policy_store.pysrc/gateway/services/provider_kwargs.pysrc/gateway/services/routing/__init__.pysrc/gateway/services/routing/compiler.pysrc/gateway/static/dashboard/assets/ActivityPage-ChQ9nFtF.jssrc/gateway/static/dashboard/assets/ActivityPage-D3zW3w7G.jssrc/gateway/static/dashboard/assets/AliasesPage-BQwHhTMQ.jssrc/gateway/static/dashboard/assets/AliasesPage-Gb_wsTGI.jssrc/gateway/static/dashboard/assets/BudgetsPage-DPpY7c4J.jssrc/gateway/static/dashboard/assets/ConfirmDialog-Ug1xmzTh.jssrc/gateway/static/dashboard/assets/DocsPage-CFMmMs_h.jssrc/gateway/static/dashboard/assets/KeysPage-CmiGNdVE.jssrc/gateway/static/dashboard/assets/ModelComboBox-Bb0dsqaN.jssrc/gateway/static/dashboard/assets/ModelScopeControl-BSC5r_qF.jssrc/gateway/static/dashboard/assets/ModelScopeControl-DOAM-IRn.jssrc/gateway/static/dashboard/assets/ModelsPage-CRJ_Wl2U.jssrc/gateway/static/dashboard/assets/ModelsPage-qNMhih0k.jssrc/gateway/static/dashboard/assets/OverviewPage-B28JIhAq.jssrc/gateway/static/dashboard/assets/OverviewPage-CTW9HJc-.jssrc/gateway/static/dashboard/assets/ProvidersPage-CkydSD4T.jssrc/gateway/static/dashboard/assets/ProvidersPage-V9eY6Y_V.jssrc/gateway/static/dashboard/assets/RoutingPage-DKobULws.jssrc/gateway/static/dashboard/assets/SettingsPage-0CTHb8Ij.jssrc/gateway/static/dashboard/assets/SettingsPage-Cg7H88vI.jssrc/gateway/static/dashboard/assets/TablePagination-ZKa66bAr.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-CQmrtqXw.jssrc/gateway/static/dashboard/assets/UsagePage-4BFMRtYx.jssrc/gateway/static/dashboard/assets/UsersPage-CGPsxV6u.jssrc/gateway/static/dashboard/assets/index-BmxAZYrI.csssrc/gateway/static/dashboard/assets/index-CW8dw4O4.csssrc/gateway/static/dashboard/assets/index-CvNLvggU.jssrc/gateway/static/dashboard/assets/index-DGvD8Pfl.jssrc/gateway/static/dashboard/index.htmlsrc/gateway/types/attempt.pysrc/gateway/types/budget_state.pytests/integration/test_routing_policies.pytests/integration/test_usage_endpoint.pytests/unit/test_attempt_walker.pytests/unit/test_check_architecture.pytests/unit/test_pipeline_settlement.pyweb/src/App.tsxweb/src/api/hooks.tsweb/src/api/types.tsweb/src/components/AppShell.tsxweb/src/pages/ActivityPage.tsxweb/src/pages/RoutingPage.test.tsxweb/src/pages/RoutingPage.tsx
💤 Files with no reviewable changes (8)
- src/gateway/static/dashboard/assets/AliasesPage-BQwHhTMQ.js
- src/gateway/static/dashboard/assets/ModelsPage-CRJ_Wl2U.js
- src/gateway/static/dashboard/assets/ActivityPage-ChQ9nFtF.js
- src/gateway/static/dashboard/assets/SettingsPage-Cg7H88vI.js
- src/gateway/static/dashboard/assets/ProvidersPage-V9eY6Y_V.js
- src/gateway/static/dashboard/assets/OverviewPage-B28JIhAq.js
- src/gateway/static/dashboard/assets/ModelScopeControl-DOAM-IRn.js
- src/gateway/static/dashboard/assets/index-DGvD8Pfl.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🧹 Nitpick comments (14)
src/gateway/api/routes/routing.py (2)
238-248: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a pagination bound to
list_policies.
select(RoutingPolicy)fetches every row with no limit. In practice this table stays small because only an operator writes to it, so this is unlikely to bite soon. The house rule is unconditional for list endpoints, though, and a deployment that scopes a policy per user can grow this table faster than expected.Adding
limit/offsetquery parameters with a default and a maximum, matching the other management list routes, keeps this endpoint predictable.As per coding guidelines: "Every list endpoint must have a sane default and maximum pagination bound; never select an unbounded growing table such as
UsageLogorModelPricing."🤖 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 238 - 248, Update the list_policies endpoint to accept limit and offset query parameters using the same sane defaults and maximum bounds as other management list routes. Apply both bounds to the RoutingPolicy query before execution while preserving its existing name ordering and response behavior.Source: Coding guidelines
371-376: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
explain_policynever usesdb.The handler declares
db: Annotated[AsyncSession, Depends(get_db)]but resolves everything fromconfigand the policy cache. FastAPI will still open and close a session for every call. Removing the parameter drops that cost and makes the "no dispatch, no database" contract in the docstring visible in the signature.If a future revision needs the session for budget lookups, adding it back is a one-line change.
🤖 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 371 - 376, Remove the unused db dependency parameter from explain_policy and its associated AsyncSession/get_db imports only if they become unused; retain config injection and all existing response behavior. This makes the endpoint avoid creating a database session when explaining policies.src/gateway/services/routing/compiler.py (2)
162-164: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer an explicit guard over
assertfor the schema invariant.
assertstatements are removed when Python runs with-O. If that happens and awhenentry ever reaches here without a target,Noneflows intoresolve_provider_selectorinstead of failing loudly. The schema does enforce the invariant today, so this is defensive rather than an active bug.♻️ Proposed fix
- if entry.when is not None and _matches(entry.when, user_id=user_id, key_id=key_id, budget=budget): - assert entry.target is not None # schema: a `when` entry always carries a target - return entry.target, f"condition:{','.join(entry.when.conditions())}" + if ( + entry.when is not None + and entry.target is not None + and _matches(entry.when, user_id=user_id, key_id=key_id, budget=budget) + ): + return entry.target, f"condition:{','.join(entry.when.conditions())}"🤖 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/services/routing/compiler.py` around lines 162 - 164, Replace the assert in the conditional-entry branch of the routing compiler with an explicit runtime guard that verifies entry.target is not None before returning it. Preserve the existing condition label and return behavior for valid targets, while failing loudly when a matching when entry lacks a target, including when Python runs with optimizations.
153-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rate-limiting the router warning.
compile_policyruns per request, so a policy that names a router logs a WARNING on every single request that uses it. The routing behavior is correct and the fallthrough is the right choice. The volume is the concern: a busy policy will bury the rest of the log, and the message tells the operator nothing new after the first time.A one-time warning at config validation or policy load, plus a debug-level line here, keeps the signal without the flood.
🤖 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/services/routing/compiler.py` around lines 153 - 161, Reduce repeated WARNING logs in compile_policy for unavailable routers: move the operator-facing warning to configuration validation or policy loading so it is emitted once, and change the per-request logger.warning in the entry.router branch to a debug-level message while preserving the existing fallthrough behavior.src/gateway/services/policy_store.py (1)
44-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
all_policy_namesto__all__.
all_policy_namesis imported bysrc/gateway/api/routes/routing.py(line 34), so it is part of this module's public surface. The__all__list omits it, which makes the declared surface disagree with real usage.♻️ Proposed fix
__all__ = [ "POLICY_CACHE_TTL_SECONDS", + "all_policy_names", "cached_policies", "effective_policies",🤖 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/services/policy_store.py` around lines 44 - 54, Add the existing all_policy_names symbol to the __all__ list in the policy store module, alongside the other exported policy helpers, so its declared public surface matches its import from the routing module.scripts/check_architecture.py (1)
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment claims more than the rule enforces.
The comment states that shared types "may not import any other gateway layer", but
gateway.modelsis absent fromforbidden, so a module undergateway/typescan import it and the check stays green. Either addgateway.modelsto the list, or narrow the comment to name the layers that are actually enforced. Right now a reader trusts the comment and a future type quietly grows a pydantic dependency.No behavior change is needed today; both current type modules import only the standard library and
any_llm.🤖 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 `@scripts/check_architecture.py` around lines 68 - 77, The gateway/types architecture rule’s comment claims all gateway layers are forbidden, but its forbidden list omits gateway.models. Update the “gateway/types” entry so the documented restriction matches enforcement by adding gateway.models to forbidden, without changing unrelated rules.src/gateway/api/routes/models.py (1)
298-309: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRead the policy map once, then derive both values from it.
The comment on line 413 already states the rule for aliases: read once, so the withheld set and the listed names agree even if a write lands between the two reads.
_policy_target_keysbreaks that rule for policies, because it callseffective_policiesa second time. If a policy is created or deleted between line 419 and line 427, the catalog can list a policy whose candidates were not withheld, or withhold candidates for a policy it no longer lists.The cache makes this cheap and rare, so this is about the invariant rather than cost. Passing the already-resolved mapping in also removes the duplicate lookup.
♻️ Suggested shape
-def _policy_target_keys(config: GatewayConfig, caller_user_id: str | None) -> set[str]: +def _policy_target_keys(config: GatewayConfig, policies: dict[str, PolicySpec]) -> set[str]: """Canonical pricing keys of every selector any policy in force can reach. Withheld from the listing for the same reason alias targets are: a policy exists partly so the provider/model behind it stays private, and that has to hold for its fallback candidates too, not just its default. """ return { normalize_pricing_key(config, selector) - for spec in effective_policies(config, caller_user_id).values() + for spec in policies.values() for selector in spec.static_selectors() }
_policy_catalog_entrieswould return the resolved mapping alongside the split, or the caller resolveseffective_policiesonce and passes it to both helpers.Also applies to: 419-427
🤖 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/models.py` around lines 298 - 309, Resolve effective_policies once in the policy catalog flow and reuse that mapping for both catalog entries and _policy_target_keys. Update the relevant helper signatures and caller, including _policy_catalog_entries if needed, so policy listing and withheld target derivation operate on the same snapshot.src/gateway/services/guardrails.py (1)
206-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the docstring to match the new two-field decision.
The logic is right, and keeping
block/blockas the default preserves the old behavior. The function docstring above still states thatblockguardrails always fail closed and thatGuardrailsNotReachableErroris raised for anyblock-mode guardrail. Withon_unavailable="monitor"ablockguardrail now fails open, so theRaises:section is no longer accurate. A future reader debugging a served request during a guardrails outage will read the docstring first.The
Failure handlingbullets and theRaises:line need theon_unavailablecondition added.🤖 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/services/guardrails.py` around lines 206 - 214, Update the docstring for the function containing the GuardrailsNotReachableError handler to describe fail-closed behavior only when both cfg.mode and cfg.on_unavailable are "block"; revise the Failure handling bullets and Raises section accordingly, while documenting that other on_unavailable settings fail open.web/src/pages/RoutingPage.test.tsx (1)
84-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for scoped delete, and let the mock honor
user_id.The DELETE branch resolves the name from the path and filters
listby name alone, so theuser_idquery parameter is ignored. No test currently deletes anything, which means the mock cannot catch the subtlest rule inuseDeleteRoutingPolicy: only a null or absentuserIdmeans global scope, because""is a legal user id. That rule is exactly the kind that a later refactor turns into a truthiness check, and then a global policy gets deleted instead of a user-scoped one.A short test that renders a user-scoped row, presses Delete, and asserts the request URL carries
?user_id=alicewould pin the behavior down.♻️ Proposed mock change so scope is observable
if (method === "DELETE") { - const name = decodeURIComponent((url.split("?")[0].split("/").pop() ?? "")); - list = list.filter((item) => item.name !== name); + const [path, query] = url.split("?"); + const name = decodeURIComponent(path.split("/").pop() ?? ""); + const scoped = new URLSearchParams(query ?? "").get("user_id"); + list = list.filter((item) => item.name !== name || item.user_id !== scoped); return new Response(null, { status: 204 }); }🤖 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.test.tsx` around lines 84 - 96, Update the DELETE branch of the routing-policy mock to read and honor the user_id query parameter, distinguishing null/absent global scope from any provided value, including an empty string, when filtering by policy name and scope. Add a test covering deletion of a user-scoped policy that renders the row, presses Delete, and verifies the request URL includes ?user_id=alice.src/gateway/static/dashboard/assets/index-BmxAZYrI.css (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialIgnore the generated dashboard bundle where lint runs
The build output under
src/gateway/static/dashboardis meant to be committed, and machine-generated Tailwind/HeroUI CSS should not fail CI on every rebuild. If this project configures a stylelint check, addsrc/gateway/static/dashboard/**to.stylelintignoresoweb/srcCSS stays the signal.🤖 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/static/dashboard/assets/index-BmxAZYrI.css` at line 1, Add src/gateway/static/dashboard/** to the project’s .stylelintignore configuration so generated Tailwind/HeroUI assets such as the dashboard CSS bundle are excluded from stylelint while web/src styles continue to be checked.Source: Linters/SAST tools
tests/unit/test_attempt_walker.py (1)
48-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo new walker parameters have no unit coverage.
The harness never exercises
on_absorbedorbuild_kwargs, and both carry real meaning.on_absorbeddecides which failures becomestatus="absorbed"audit rows, and the walker deliberately skips it for the last attempt.build_kwargsis how the responses format rebuilds per-candidate kwargs; a regression there would send one provider's arguments to another. The fast tier is the right place to pin both.💚 Suggested tests
`@pytest.mark.asyncio` async def test_absorbed_is_called_for_recovered_attempts_only() -> None: """The terminal failure is the request's own outcome; the caller logs that one.""" absorbed: list[int] = [] async def run_attempt(attempt: Attempt, call_kwargs: dict[str, Any], mark_locked_in: Any) -> Any: raise _http_error(503) async def on_absorbed(attempt: Attempt, exc: BaseException, total: int) -> None: absorbed.append(attempt.position) with pytest.raises(HTTPException): await walk_attempts( attempts=[_attempt(1, "a"), _attempt(2, "b")], base_request_fields={}, run_attempt=run_attempt, max_tool_iterations=10, on_absorbed=on_absorbed, ) assert absorbed == [1] `@pytest.mark.asyncio` async def test_build_kwargs_runs_per_candidate() -> None: """The transformation must apply to the candidate being tried, not the one that failed.""" seen: list[str] = [] def build_kwargs(attempt: Attempt, fields: dict[str, Any]) -> dict[str, Any]: return {**fields, "provider": attempt.instance, "model": attempt.model} async def run_attempt(attempt: Attempt, call_kwargs: dict[str, Any], mark_locked_in: Any) -> Any: seen.append(call_kwargs["model"]) if attempt.position == 1: raise _http_error(503) return "ok" await walk_attempts( attempts=[_attempt(1, "a"), _attempt(2, "b", instance="anthropic")], base_request_fields={}, run_attempt=run_attempt, max_tool_iterations=10, build_kwargs=build_kwargs, ) assert seen == ["a", "b"]🤖 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 `@tests/unit/test_attempt_walker.py` around lines 48 - 70, Add fast-tier unit tests covering both new walker hooks: verify on_absorbed is called only for recovered failures and skipped for the terminal attempt, and verify build_kwargs is invoked per candidate so each attempt’s provider/model arguments reach run_attempt after fallback.tests/integration/test_routing_policies.py (1)
770-784: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test passes for the wrong reason on one nearby case.
Here every candidate returns 503, so the walk really does reach position 2 and
attempt_position == 2is correct. The gap is the terminal-failure case: a 400 or 401 on candidate 1 stops the walk immediately, but the error row is still attributed toattempts[-1](see the_failure_attributioncomment onsrc/gateway/api/routes/_pipeline.py). A second test would catch that, and would fail today:💚 Suggested additional test
def test_a_terminal_failure_is_attributed_to_the_candidate_that_failed(client: TestClient) -> None: """A 401 stops the chain at candidate 1, so the row must name candidate 1.""" _create_user(client) with patch("gateway.api.routes.chat.acompletion", new=AsyncMock(side_effect=_http_error(401))): _chat(client, "fast") errors = [r for r in _usage_rows(client) if r["status"] == "error"] assert len(errors) == 1 assert errors[0]["attempt_position"] == 1 assert errors[0]["model"] == "gpt-5-mini" assert errors[0]["provider"] == "openai"🤖 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 `@tests/integration/test_routing_policies.py` around lines 770 - 784, The existing test does not cover terminal failures that stop routing at the first candidate. Add a test alongside test_a_failed_request_still_says_which_policy_it_went_through that mocks a 401 response, verifies one error usage row, and asserts attempt_position is 1 with the expected model and provider, so _failure_attribution handles the candidate that actually failed rather than the final configured attempt.tests/unit/test_pipeline_settlement.py (1)
75-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider letting
_ctxbuild a plan-shaped context.This block is the fast tier for settlement, and its own header says the merged executor must preserve these behaviors. Right now no test here builds a
RequestContextwith aplan, so the failover branch and the settlement rules that matter most for routing are covered only in the Postgres-backed integration tier:
- exactly one reconcile for a request that fell over,
- exactly one refund when the plan is exhausted,
- a refused reservation top-up stopping the chain instead of serving the pricier candidate.
Adding
plan: CompiledPlan | None = Noneandrequest_group_idto_ctxwould make those reachable here, where they run in milliseconds.🤖 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 `@tests/unit/test_pipeline_settlement.py` around lines 75 - 95, Extend the test helper _ctx to accept optional plan and request_group_id parameters and pass them into RequestContext, using the existing CompiledPlan type and appropriate default. Add fast-tier unit coverage for failover settlement: exactly one reconcile, one refund when the plan is exhausted, and stopping the chain when reservation top-up is refused.web/src/pages/ActivityPage.tsx (1)
879-902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Routing and absorbed-status coverage to
ActivityPage.test.tsx.
Routingis a new column, andstatus: "absorbed"is a new status pill, but the existing tests still do not assert either. Add two small row cases that mock usage rows, then query the rendered text or roles: one unrouted row must have no routing text, and one routed row must show theattempt 2/2attribution.🤖 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/ActivityPage.tsx` around lines 879 - 902, Add focused cases in ActivityPage.test.tsx covering the new Routing column and absorbed status pill: mock an unrouted usage row and assert it has no routing text, then mock a routed row with status "absorbed" and assert the rendered UI exposes the absorbed status and “attempt 2/2” attribution. Use existing row-rendering helpers and query text or roles rather than implementation details.Source: Coding guidelines
🤖 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 `@docs/routing.md`:
- Around line 82-85: Update the routing documentation to remove the master-key
case from the “no budget configured” exception. Keep the explanation for callers
without a budget and unlimited budgets, while documenting that master-key
requests use the supplied user_id and may match a policy branch based on that
user’s budget.
In `@otari.db-shm`:
- Line 1: Remove the committed SQLite runtime artifacts otari.db-shm and
otari.db-wal from version control, and broaden the existing root .gitignore rule
for otari.db so these companion files are ignored as well. Keep otari.db itself
ignored and ensure future local runs cannot reintroduce any of the three
database files.
In `@scripts/seed_routing_demo.py`:
- Around line 4-6: Update the descriptive output in the seed script to say
“failover” and avoid guaranteeing an absorbed attempt row; state that fallback
and absorbed activity may appear only when the seeded traffic triggers a
retryable primary-model failure. Apply the same wording correction to the
corresponding description around the other affected output block.
In `@src/gateway/api/routes/_attempts.py`:
- Around line 189-206: Add import asyncio and update the exception handling
around the retry flow so asyncio.CancelledError is explicitly re-raised before
the broad BaseException classification and provider-error mapping in the visible
catch block. Keep timeout handling unchanged, relying on the existing Python
3.13+ TimeoutError behavior.
In `@src/gateway/api/routes/_pipeline.py`:
- Around line 2626-2668: Update the exhausted-plan flow so it records the actual
attempt where walk_attempts stopped, rather than always using
ctx.plan.attempts[-1]. Propagate that attempt through both exception-catching
call sites into log_exhausted_plan, and use it for _failure_attribution plus the
logged model and provider; preserve tail attribution only when the walker truly
reaches the final candidate.
In `@src/gateway/api/routes/messages.py`:
- Line 535: The streaming Messages request path is missing the base request
fields needed for failover. Update the streaming call near the non-streaming
request handling to pass base_request_fields=request_fields, matching the
existing non-streaming path and chat.py behavior so run_single_attempt_stream
can apply the on_failure chain.
In `@src/gateway/api/routes/models.py`:
- Around line 254-270: The ModelsPage pricing-source mapping must preserve
"dynamic" instead of collapsing it to "none" when pricing is null. Update the
model-to-PriceSource logic in ModelsPage.tsx to explicitly map or permit
model.pricing_source === "dynamic", and ensure the resulting PriceSource
rendering handles that value without assigning an alias label.
In `@src/gateway/models/routing.py`:
- Around line 133-152: Update _budget_thresholds_stay_under_the_cap to apply the
unreachable-threshold validation only when budget_used_pct uses the upward gte
or gt comparator and its value is at least 100. Allow lt and lte thresholds at
or above 100 to pass validation, while preserving the existing error and
behavior for unreachable upward thresholds.
In `@src/gateway/static/dashboard/assets/RoutingPage-DKobULws.js`:
- Line 1: The edit form in D currently rebuilds spec.select through se,
preserving only budget thresholds and the default while dropping router entries.
Update the select-state initialization and serialization around se and the $
useMemo so existing router entries remain intact when saving, while retaining
the current budget-threshold editing behavior; alternatively, prevent D from
editing policies containing router-backed select entries.
In `@web/src/pages/ActivityPage.tsx`:
- Around line 82-91: The Status filter UI should expose an “Absorbed” option
alongside All, Success, and Error, using the existing status-filter value and
query flow so it requests status=absorbed. Update the status select
definition/rendering in ActivityPage without changing activity row styling or
other filter behavior.
In `@web/src/pages/RoutingPage.tsx`:
- Around line 614-615: Make the adding and editing state transitions in
RoutingPage mutually exclusive: when opening a new PolicyForm, clear editing,
and when opening an existing policy for editing, clear adding. Update the
handlers used by the “New policy” action and the table’s Edit button, while
preserving the existing PolicyForm close behavior.
- Around line 201-214: Update the policy editor state and submit logic around
the PolicySpec useMemo so editing preserves all loaded spec fields, including
spec_version, limits, and select entries the form does not model. On save,
replace only the form-owned conditions/default target while carrying through
unrecognized select rules and existing metadata; ensure the Edit action for
stored policies cannot silently discard data.
---
Nitpick comments:
In `@scripts/check_architecture.py`:
- Around line 68-77: The gateway/types architecture rule’s comment claims all
gateway layers are forbidden, but its forbidden list omits gateway.models.
Update the “gateway/types” entry so the documented restriction matches
enforcement by adding gateway.models to forbidden, without changing unrelated
rules.
In `@src/gateway/api/routes/models.py`:
- Around line 298-309: Resolve effective_policies once in the policy catalog
flow and reuse that mapping for both catalog entries and _policy_target_keys.
Update the relevant helper signatures and caller, including
_policy_catalog_entries if needed, so policy listing and withheld target
derivation operate on the same snapshot.
In `@src/gateway/api/routes/routing.py`:
- Around line 238-248: Update the list_policies endpoint to accept limit and
offset query parameters using the same sane defaults and maximum bounds as other
management list routes. Apply both bounds to the RoutingPolicy query before
execution while preserving its existing name ordering and response behavior.
- Around line 371-376: Remove the unused db dependency parameter from
explain_policy and its associated AsyncSession/get_db imports only if they
become unused; retain config injection and all existing response behavior. This
makes the endpoint avoid creating a database session when explaining policies.
In `@src/gateway/services/guardrails.py`:
- Around line 206-214: Update the docstring for the function containing the
GuardrailsNotReachableError handler to describe fail-closed behavior only when
both cfg.mode and cfg.on_unavailable are "block"; revise the Failure handling
bullets and Raises section accordingly, while documenting that other
on_unavailable settings fail open.
In `@src/gateway/services/policy_store.py`:
- Around line 44-54: Add the existing all_policy_names symbol to the __all__
list in the policy store module, alongside the other exported policy helpers, so
its declared public surface matches its import from the routing module.
In `@src/gateway/services/routing/compiler.py`:
- Around line 162-164: Replace the assert in the conditional-entry branch of the
routing compiler with an explicit runtime guard that verifies entry.target is
not None before returning it. Preserve the existing condition label and return
behavior for valid targets, while failing loudly when a matching when entry
lacks a target, including when Python runs with optimizations.
- Around line 153-161: Reduce repeated WARNING logs in compile_policy for
unavailable routers: move the operator-facing warning to configuration
validation or policy loading so it is emitted once, and change the per-request
logger.warning in the entry.router branch to a debug-level message while
preserving the existing fallthrough behavior.
In `@src/gateway/static/dashboard/assets/index-BmxAZYrI.css`:
- Line 1: Add src/gateway/static/dashboard/** to the project’s .stylelintignore
configuration so generated Tailwind/HeroUI assets such as the dashboard CSS
bundle are excluded from stylelint while web/src styles continue to be checked.
In `@tests/integration/test_routing_policies.py`:
- Around line 770-784: The existing test does not cover terminal failures that
stop routing at the first candidate. Add a test alongside
test_a_failed_request_still_says_which_policy_it_went_through that mocks a 401
response, verifies one error usage row, and asserts attempt_position is 1 with
the expected model and provider, so _failure_attribution handles the candidate
that actually failed rather than the final configured attempt.
In `@tests/unit/test_attempt_walker.py`:
- Around line 48-70: Add fast-tier unit tests covering both new walker hooks:
verify on_absorbed is called only for recovered failures and skipped for the
terminal attempt, and verify build_kwargs is invoked per candidate so each
attempt’s provider/model arguments reach run_attempt after fallback.
In `@tests/unit/test_pipeline_settlement.py`:
- Around line 75-95: Extend the test helper _ctx to accept optional plan and
request_group_id parameters and pass them into RequestContext, using the
existing CompiledPlan type and appropriate default. Add fast-tier unit coverage
for failover settlement: exactly one reconcile, one refund when the plan is
exhausted, and stopping the chain when reservation top-up is refused.
In `@web/src/pages/ActivityPage.tsx`:
- Around line 879-902: Add focused cases in ActivityPage.test.tsx covering the
new Routing column and absorbed status pill: mock an unrouted usage row and
assert it has no routing text, then mock a routed row with status "absorbed" and
assert the rendered UI exposes the absorbed status and “attempt 2/2”
attribution. Use existing row-rendering helpers and query text or roles rather
than implementation details.
In `@web/src/pages/RoutingPage.test.tsx`:
- Around line 84-96: Update the DELETE branch of the routing-policy mock to read
and honor the user_id query parameter, distinguishing null/absent global scope
from any provided value, including an empty string, when filtering by policy
name and scope. Add a test covering deletion of a user-scoped policy that
renders the row, presses Delete, and verifies the request URL includes
?user_id=alice.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 21ecf7ea-8c50-4a00-9827-28882ff33829
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (77)
alembic/versions/e8b1d3f5a7c9_add_routing_policies.pyalembic/versions/f4c6a8b0d2e5_add_usage_logs_routing_attribution.pyconfig.example.ymldocs/configuration.mddocs/dashboard.mddocs/index.mddocs/models.mddocs/public/otari.postman_collection.jsondocs/routing.mdotari.db-shmotari.db-walscripts/check_architecture.pyscripts/seed_routing_demo.pysrc/gateway/api/main.pysrc/gateway/api/routes/_attempts.pysrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/chat.pysrc/gateway/api/routes/messages.pysrc/gateway/api/routes/models.pysrc/gateway/api/routes/responses.pysrc/gateway/api/routes/routing.pysrc/gateway/api/routes/usage.pysrc/gateway/cli.pysrc/gateway/core/config.pysrc/gateway/main.pysrc/gateway/models/entities.pysrc/gateway/models/guardrails.pysrc/gateway/models/routing.pysrc/gateway/services/budget_service.pysrc/gateway/services/guardrails.pysrc/gateway/services/policy_store.pysrc/gateway/services/provider_kwargs.pysrc/gateway/services/routing/__init__.pysrc/gateway/services/routing/compiler.pysrc/gateway/static/dashboard/assets/ActivityPage-ChQ9nFtF.jssrc/gateway/static/dashboard/assets/ActivityPage-D3zW3w7G.jssrc/gateway/static/dashboard/assets/AliasesPage-BQwHhTMQ.jssrc/gateway/static/dashboard/assets/AliasesPage-Gb_wsTGI.jssrc/gateway/static/dashboard/assets/BudgetsPage-DPpY7c4J.jssrc/gateway/static/dashboard/assets/ConfirmDialog-Ug1xmzTh.jssrc/gateway/static/dashboard/assets/DocsPage-CFMmMs_h.jssrc/gateway/static/dashboard/assets/KeysPage-CmiGNdVE.jssrc/gateway/static/dashboard/assets/ModelComboBox-Bb0dsqaN.jssrc/gateway/static/dashboard/assets/ModelScopeControl-BSC5r_qF.jssrc/gateway/static/dashboard/assets/ModelScopeControl-DOAM-IRn.jssrc/gateway/static/dashboard/assets/ModelsPage-CRJ_Wl2U.jssrc/gateway/static/dashboard/assets/ModelsPage-qNMhih0k.jssrc/gateway/static/dashboard/assets/OverviewPage-B28JIhAq.jssrc/gateway/static/dashboard/assets/OverviewPage-CTW9HJc-.jssrc/gateway/static/dashboard/assets/ProvidersPage-CkydSD4T.jssrc/gateway/static/dashboard/assets/ProvidersPage-V9eY6Y_V.jssrc/gateway/static/dashboard/assets/RoutingPage-DKobULws.jssrc/gateway/static/dashboard/assets/SettingsPage-0CTHb8Ij.jssrc/gateway/static/dashboard/assets/SettingsPage-Cg7H88vI.jssrc/gateway/static/dashboard/assets/TablePagination-ZKa66bAr.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-CQmrtqXw.jssrc/gateway/static/dashboard/assets/UsagePage-4BFMRtYx.jssrc/gateway/static/dashboard/assets/UsersPage-CGPsxV6u.jssrc/gateway/static/dashboard/assets/index-BmxAZYrI.csssrc/gateway/static/dashboard/assets/index-CW8dw4O4.csssrc/gateway/static/dashboard/assets/index-CvNLvggU.jssrc/gateway/static/dashboard/assets/index-DGvD8Pfl.jssrc/gateway/static/dashboard/index.htmlsrc/gateway/types/attempt.pysrc/gateway/types/budget_state.pytests/integration/test_routing_policies.pytests/integration/test_usage_endpoint.pytests/unit/test_attempt_walker.pytests/unit/test_check_architecture.pytests/unit/test_pipeline_settlement.pyweb/src/App.tsxweb/src/api/hooks.tsweb/src/api/types.tsweb/src/components/AppShell.tsxweb/src/pages/ActivityPage.tsxweb/src/pages/RoutingPage.test.tsxweb/src/pages/RoutingPage.tsx
💤 Files with no reviewable changes (8)
- src/gateway/static/dashboard/assets/AliasesPage-BQwHhTMQ.js
- src/gateway/static/dashboard/assets/ModelsPage-CRJ_Wl2U.js
- src/gateway/static/dashboard/assets/ActivityPage-ChQ9nFtF.js
- src/gateway/static/dashboard/assets/SettingsPage-Cg7H88vI.js
- src/gateway/static/dashboard/assets/ProvidersPage-V9eY6Y_V.js
- src/gateway/static/dashboard/assets/OverviewPage-B28JIhAq.js
- src/gateway/static/dashboard/assets/ModelScopeControl-DOAM-IRn.js
- src/gateway/static/dashboard/assets/index-DGvD8Pfl.js
🛑 Comments failed to post (1)
otari.db-shm (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Which SQLite artifacts are tracked, and are they ignored? git ls-files | rg -n 'otari\.db|\.sqlite|\.db-wal|\.db-shm' || echo "no tracked sqlite artifacts" fd -H -t f '^\.gitignore$' | while IFS= read -r f; do echo "== $f"; rg -n 'db|sqlite' "$f" || echo "(no db patterns)" doneRepository: mozilla-ai/otari
Length of output: 571
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Tracked SQLite artifacts:" git ls-files | rg -n 'otari\.db|\.sqlite|\.db-wal|\.db-shm' || echo "no tracked sqlite artifacts" echo echo "Relevant .gitignore entries:" for f in $(fd -H -t f '^\.gitignore$'); do echo "== $f" rg -n -i 'db|sqlite|\.db(-wal|-shm)?' "$f" || echo "(no matching db patterns)" done echo echo "SQLite artifact files by type:" git ls-files -z | xargs -0 file | rg 'otari\.db|\.sqlite|\.db-wal|\.db-shm' || trueRepository: mozilla-ai/otari
Length of output: 725
Remove the committed SQLite runtime artifacts from the repository.
otari.db-shmandotari.db-walare SQLite runtime files created alongside the database..gitignorealready ignoresotari.db, but not the-shmand-walartifacts, so the next local run can add them back unless the root ignore rule is broadened for this database name. Delete both committed artifacts from the index and cover them in.gitignore; the database file itself can contain local dev state that should not be reviewed or merged.🤖 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 `@otari.db-shm` at line 1, Remove the committed SQLite runtime artifacts otari.db-shm and otari.db-wal from version control, and broaden the existing root .gitignore rule for otari.db so these companion files are ignored as well. Keep otari.db itself ignored and ensure future local runs cannot reintroduce any of the three database files.
Review of #492 found that the headline feature did not work and that several claims the docs made were untrue. Each fix comes with the test that would have caught it, because every one of these was invisible to a green build and to a request returning 200. * **Policy guardrails were never enforced.** The compiler built the list and no route read it, so the schema, the API, the CLI output, the dashboard editor, and the docs all described enforcement that never happened. Merged in `prepare_gateway_tools`, which all three completion routes already call, so no route can forget it. Union by profile, stricter setting wins on both `mode` and `on_unavailable`: a caller may add a guardrail or tighten one, never weaken a mandate. * **Failover only reached `/v1/chat/completions`.** `messages.py` passed the base request fields on its non-streaming call only and `responses.py` on neither, so those runners never walked the chain and `_ResponsesAdapter.local_attempt_kwargs` was dead code. All three endpoints now fail over, streaming and not. * **Streamed requests recorded no attribution.** `build_streaming_response` never received it, so the serving row of a streamed fallover carried no `request_group_id` and the absorbed attempt it belonged to was an orphan, which is the one thing that column exists to prevent. * **A terminal failure named the wrong candidate.** Attribution defaulted to the end of the plan, but a non-retryable status or a tool-loop lock-in stops the walk early, so a 401 on candidate 1 was logged against candidate 2 and the by-provider breakdown blamed a provider that was never called. The walker now reports which candidate it stopped on. * **`require_pricing` was bypassable through a fallback.** The gate prices only the head candidate, so an unpriced model that 402s when named directly would serve, and log `cost=null`, by being reached as an `on_failure` entry. The gate now applies to every candidate. * **`limits` was a dead knob.** The streaming first-chunk deadline it would override is applied solely by the hybrid walker, and policies are standalone-only, so the field validated, stored, and did nothing. Removed rather than shipped, with a note on what has to exist before it returns. Also: `absorbed` is filterable in the activity log (it was rendered and styled but not in the filter's options); mean latency excludes absorbed rows, so a policy that recovers quickly no longer looks slower than one that never fails; the dashboard hides Edit for a policy whose shape the form cannot represent, since a lossy save is worse than no button; a policy-name error no longer talks about aliases; and two SQLite WAL sidecars that should never have been committed are removed and gitignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 00:28 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
removed
the
missing-template
The review noted that `docs/routing.md` explained the object and the config block but never mentioned `/v1/routing/policies`, so the half of the feature that does not need a file edit or a restart was undiscoverable from its own page. Adds a "Managing policies at runtime" section with the create, list, explain, and delete calls, and says plainly that every verb needs the master key, `explain` included, because its response enumerates the targets a policy exists to hide. `docs/api-reference.md` gains sections for both routing policies and aliases. Aliases were already missing there, which contradicted the page's claim to document every endpoint, so both land together where a reader can compare them. The availability matrix lists them too. Every curl in the new sections was run verbatim against a live gateway, since a previous alias example in these docs shipped without a Content-Type header and would have answered 422. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 00:34 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/public/otari.postman_collection.json (1)
1958-2083: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd master-key auth credentials to the policy management requests.
The routes are master-key gated, but PolicyRequest field validation is not affected by the auth mode, and
Delete Policyalready preserves theuser_idquery scope.🤖 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 `@docs/public/otari.postman_collection.json` around lines 1958 - 2083, Add the collection’s established master-key authentication credentials to the List Policies, Set Policy, Explain Policy, and Delete Policy requests, ensuring each request can access the master-key-gated routes. Preserve the existing request bodies, methods, paths, and Delete Policy user_id query scope.web/src/pages/RoutingPage.test.tsx (1)
109-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRun the link assertion with
HashRouter.
MemoryRouterrenders thisLinkas/tools; the deployed dashboard usesHashRouter, so this route should expose#/tools. Update the assertion to useHashRouter, or add a hash-router render helper, so this test matches the deployed routing contract.🤖 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.test.tsx` around lines 109 - 115, Update the renderPage helper in RoutingPage.test.tsx to use HashRouter instead of MemoryRouter, then run the link assertion against the hash-based route so it verifies the deployed contract and expects `#/tools`.Source: Coding guidelines
🧹 Nitpick comments (4)
tests/unit/test_attempt_walker.py (1)
373-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the operator-owned guardrail URL.
The merge now copies the mandated
GuardrailConfigand only updatesmodeandon_unavailable, but the guardrail URL ownership case is only documented. Add a test with differenturlvalues for the mandated and caller entries and assert the mandated URL is preserved.🤖 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 `@tests/unit/test_attempt_walker.py` around lines 373 - 383, The test test_a_caller_cannot_weaken_a_mandated_guardrail should also cover guardrail URL ownership: give the mandated and caller _guardrail entries different url values, then assert merged[0].url retains the mandated URL while the existing mode and on_unavailable assertions remain unchanged.web/src/pages/RoutingPage.test.tsx (3)
144-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that config-defined policies hide both controls.
The test title says the UI suppresses Edit and Delete, but it checks only Delete. A regression that exposes a lossy Edit action would pass. Add a negative assertion for the Edit button.
Suggested assertion
expect(within(autoRow).queryByRole("button", { name: "Delete" })).not.toBeInTheDocument(); + expect(within(autoRow).queryByRole("button", { name: "Edit" })).not.toBeInTheDocument();🤖 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.test.tsx` around lines 144 - 151, Update the test case “does not offer to edit or delete a policy that lives in config.yml” to assert that the config-defined policy row also lacks the Edit button, alongside the existing Delete assertion. Use the same autoRow and role-based query pattern to verify both controls are hidden.
51-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock the
apiFetchboundary instead ofglobalThis.fetch.This test currently exercises the real
apiFetchimplementation and intercepts the lower-level transport. That couples page tests to request serialization and transport details. Mock the exportedapiFetchfunction while keeping the real hooks and providers.As per coding guidelines, mock only the
apiFetchnetwork boundary, notglobalThis.fetch.🤖 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.test.tsx` around lines 51 - 58, Update the mockApi setup in RoutingPage.test.tsx to mock the exported apiFetch function rather than spying on globalThis.fetch. Keep the existing policy and guardrail response behavior and call tracking at the apiFetch boundary, while leaving the real hooks and providers unchanged.Source: Coding guidelines
104-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail on unexpected API requests.
The fallback returns
200 []for every unmodeled URL. A hook can call a wrong or new endpoint and these tests can still pass with empty data. Throw for unknown routes, then add explicit responses only when a test needs them.Suggested strict fallback
- return jsonResponse([]); + throw new Error(`Unexpected API request: ${method} ${url}`);🤖 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.test.tsx` at line 104, Update the request-mocking fallback in the RoutingPage tests to throw for any unmodeled URL instead of returning jsonResponse([]). Add explicit route responses for each endpoint required by existing tests, preserving their expected behavior while ensuring unexpected API requests fail immediately.
🤖 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/gateway/static/dashboard/assets/OverviewPage-CTPTo-KN.js`:
- Line 1: Update OverviewIndex and Rt so the stored-provider query error is
propagated into the Overview error state: pass the query error alongside
needsSetup or include it in Rt’s aggregated et value. Ensure a provider-query
failure is surfaced by the existing V error display and does not incorrectly
trigger the setup state.
In `@src/gateway/static/dashboard/assets/ProvidersPage-dKyb-Stc.js`:
- Line 1: Update the tutorial link rendered by Ce to use the hash router,
replacing the raw href="/welcome" navigation with the router Link component
targeting "/welcome" or an equivalent "/#/welcome" URL. Keep the existing link
styling and label unchanged, then rebuild the generated dashboard asset.
In `@src/gateway/static/dashboard/assets/SettingsPage-ByleVDJK.js`:
- Line 1: Update the dialog close handler m in X so every close path clears the
generated master-key state n and resets the regeneration mutation, including
closes after successful generation. Ensure reopening the dialog cannot reuse a
previously generated key, while preserving the existing open behavior and state
reset.
---
Outside diff comments:
In `@docs/public/otari.postman_collection.json`:
- Around line 1958-2083: Add the collection’s established master-key
authentication credentials to the List Policies, Set Policy, Explain Policy, and
Delete Policy requests, ensuring each request can access the master-key-gated
routes. Preserve the existing request bodies, methods, paths, and Delete Policy
user_id query scope.
In `@web/src/pages/RoutingPage.test.tsx`:
- Around line 109-115: Update the renderPage helper in RoutingPage.test.tsx to
use HashRouter instead of MemoryRouter, then run the link assertion against the
hash-based route so it verifies the deployed contract and expects `#/tools`.
---
Nitpick comments:
In `@tests/unit/test_attempt_walker.py`:
- Around line 373-383: The test test_a_caller_cannot_weaken_a_mandated_guardrail
should also cover guardrail URL ownership: give the mandated and caller
_guardrail entries different url values, then assert merged[0].url retains the
mandated URL while the existing mode and on_unavailable assertions remain
unchanged.
In `@web/src/pages/RoutingPage.test.tsx`:
- Around line 144-151: Update the test case “does not offer to edit or delete a
policy that lives in config.yml” to assert that the config-defined policy row
also lacks the Edit button, alongside the existing Delete assertion. Use the
same autoRow and role-based query pattern to verify both controls are hidden.
- Around line 51-58: Update the mockApi setup in RoutingPage.test.tsx to mock
the exported apiFetch function rather than spying on globalThis.fetch. Keep the
existing policy and guardrail response behavior and call tracking at the
apiFetch boundary, while leaving the real hooks and providers unchanged.
- Line 104: Update the request-mocking fallback in the RoutingPage tests to
throw for any unmodeled URL instead of returning jsonResponse([]). Add explicit
route responses for each endpoint required by existing tests, preserving their
expected behavior while ensuring unexpected API requests fail immediately.
🪄 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: CHILL
Plan: Pro Plus
Run ID: a6b51f67-cb66-42df-b397-f2950cf007c6
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (36)
.gitignoreconfig.example.ymldocs/public/otari.postman_collection.jsondocs/routing.mdsrc/gateway/api/routes/_attempts.pysrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/messages.pysrc/gateway/api/routes/responses.pysrc/gateway/api/routes/routing.pysrc/gateway/api/routes/usage.pysrc/gateway/models/routing.pysrc/gateway/static/dashboard/assets/ActivityPage-C4pUjPLK.jssrc/gateway/static/dashboard/assets/AliasesPage-SLdCGrYC.jssrc/gateway/static/dashboard/assets/BudgetsPage-Dob3Jbrv.jssrc/gateway/static/dashboard/assets/ConfirmDialog-BI7FL6ES.jssrc/gateway/static/dashboard/assets/DocsPage-Dt20I1DV.jssrc/gateway/static/dashboard/assets/KeysPage-BsMi6pWI.jssrc/gateway/static/dashboard/assets/ModelComboBox-BhChLCAf.jssrc/gateway/static/dashboard/assets/ModelScopeControl-0f1Cnnou.jssrc/gateway/static/dashboard/assets/ModelsPage-KERPudvl.jssrc/gateway/static/dashboard/assets/OverviewPage-CTPTo-KN.jssrc/gateway/static/dashboard/assets/ProvidersPage-dKyb-Stc.jssrc/gateway/static/dashboard/assets/RoutingPage-ByMeUo5k.jssrc/gateway/static/dashboard/assets/SettingsPage-ByleVDJK.jssrc/gateway/static/dashboard/assets/TablePagination-DCrGjxFF.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-CIrs_9py.jssrc/gateway/static/dashboard/assets/UsagePage-CDQ43iIU.jssrc/gateway/static/dashboard/assets/UsersPage-BAdJkiiM.jssrc/gateway/static/dashboard/assets/index-BvuEgn2W.jssrc/gateway/static/dashboard/index.htmltests/integration/test_routing_policies.pytests/unit/test_attempt_walker.pyweb/src/api/types.tsweb/src/pages/ActivityPage.tsxweb/src/pages/RoutingPage.test.tsxweb/src/pages/RoutingPage.tsx
💤 Files with no reviewable changes (2)
- web/src/api/types.ts
- config.example.yml
🚧 Files skipped from review as they are similar to previous changes (9)
- src/gateway/static/dashboard/index.html
- src/gateway/api/routes/responses.py
- src/gateway/api/routes/messages.py
- web/src/pages/ActivityPage.tsx
- web/src/pages/RoutingPage.tsx
- docs/routing.md
- src/gateway/api/routes/_attempts.py
- src/gateway/api/routes/routing.py
- src/gateway/api/routes/usage.py
…ones
A policy is the general form of an alias, so two tables, two endpoints, and two
dashboard pages for one concept was one too many of each.
Migration `b5d7f9a1c3e6` moves every `model_aliases` row into `routing_policies`
as `{select: [{default: <target>}]}`, carrying scope and timestamps across. Rows
are moved rather than copied: leaving them behind would put the same name in both
stores, and alias resolution runs first, so the stale alias would win and silently
shadow every later edit made through the policy API. The downgrade moves back
every policy an alias can represent and leaves the rest in place rather than
flattening them, so it is reversible in both directions.
The Aliases page is deleted. Routing lists whatever aliases remain (the ones in
`config.yml`, plus any created through `/v1/aliases`, which stays as the
one-target API) tagged `alias`, and routes their edits and deletes to the alias
endpoint, because they are still rows in `model_aliases` and writing them back as
policies would leave the originals behind. `/#/aliases` redirects to `/#/routing`
so bookmarks keep working, and the Models page's "Make an alias" link now opens
Routing with the target prefilled.
Growing an alias a chain, a condition, or a guardrail is refused in the form with
an explanation rather than attempted: an alias holds one target, and saving it as
a policy under the same name is the collision the API already rejects.
Also closes the reverse footgun the split allowed: creating an alias named after
an existing policy now 400s. Alias resolution runs first, so it would otherwise
have silently stopped that policy taking effect.
Verified in a browser: the tab is gone, `/#/aliases` lands on `/#/routing`, and a
policy and an alias appear side by side with the alias tagged and deletable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 01:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/pages/RoutingPage.tsx (1)
78-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve every supported policy field on save.
isEditableInFormaccepts a policy withlimitsorspec_version, butspecomits both fields. Saving the form therefore submits a truncated policy document and can remove policy limits.Preserve these fields in
spec, or mark such policies read-only before offering Edit. Add coverage for saving a policy withlimitsand an explicitspec_version.Proposed preservation of non-form metadata
const spec: PolicySpec = useMemo( () => ({ + ...(existing?.spec.spec_version === undefined ? {} : { spec_version: existing.spec.spec_version }), + ...(existing?.spec.limits === undefined ? {} : { limits: existing.spec.limits }), select: [ ...conditions.map((condition) => ({ when: { budget_used_pct: { gte: condition.threshold } }, target: condition.target.trim(), })), { default: target.trim() }, ], ...(chain.length > 0 ? { on_failure: chain.map((entry) => entry.trim()) } : {}), ...(guardrails.length > 0 ? { guardrails } : {}), }), - [conditions, target, chain, guardrails], + [existing, conditions, target, chain, guardrails], );Also applies to: 254-265
🤖 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 78 - 86, Update the policy edit/save flow around isEditableInForm and the spec construction to preserve supported non-form metadata, specifically limits and an explicit spec_version, instead of submitting a truncated policy. Ensure policies containing these fields remain editable only if saving retains them, otherwise mark them read-only before showing Edit. Add coverage for saving policies with limits and an explicit spec_version.
🧹 Nitpick comments (1)
web/src/pages/ModelsPage.tsx (1)
1590-1590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the Models-to-Routing deep link.
web/src/pages/ModelsPage.test.tsxdoes not click “Make an alias” or assert the selected model carriestargetto/routing. Since this changed frontend behavior needs colocated Vitest coverage, add that regression test.🤖 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/ModelsPage.tsx` at line 1590, Add a colocated Vitest regression test in ModelsPage.test.tsx that clicks “Make an alias” and verifies navigation to /routing includes the selected model’s target query parameter. Reuse the existing ModelsPage test setup and navigation mock/assertion patterns, covering the onMakeAlias behavior in ModelsPage.Source: Coding guidelines
🤖 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 `@alembic/versions/b5d7f9a1c3e6_move_stored_aliases_into_routing_policies.py`:
- Around line 63-87: Update both upgrade() and downgrade() collision handling so
same-scope alias/policy name conflicts abort the migration instead of silently
skipping one representation and deleting it. Allow continuation only when the
alias target and policy spec are proven equivalent, reusing _spec_for where
applicable; otherwise raise an explicit migration error requiring manual
resolution while preserving reversible downgrade behavior.
- Around line 104-135: Track provenance for each policy migrated by upgrade(),
recording which routing policy produced each alias. Update downgrade() to
convert or delete only policies present in that alias-origin mapping, leaving
pre-existing or independently created policies intact. Ensure policies with
additional fields such as limits are preserved or explicitly rejected rather
than converted lossy, while maintaining a reversible migration.
In `@web/src/pages/RoutingPage.tsx`:
- Around line 707-710: Ensure the policy editor states are mutually exclusive in
the handlers that open each editor: clear editing when starting a new policy,
and clear adding when selecting an existing policy for editing. Keep the
conditional rendering around PolicyForm unchanged so only one instance can be
open at a time.
---
Outside diff comments:
In `@web/src/pages/RoutingPage.tsx`:
- Around line 78-86: Update the policy edit/save flow around isEditableInForm
and the spec construction to preserve supported non-form metadata, specifically
limits and an explicit spec_version, instead of submitting a truncated policy.
Ensure policies containing these fields remain editable only if saving retains
them, otherwise mark them read-only before showing Edit. Add coverage for saving
policies with limits and an explicit spec_version.
---
Nitpick comments:
In `@web/src/pages/ModelsPage.tsx`:
- Line 1590: Add a colocated Vitest regression test in ModelsPage.test.tsx that
clicks “Make an alias” and verifies navigation to /routing includes the selected
model’s target query parameter. Reuse the existing ModelsPage test setup and
navigation mock/assertion patterns, covering the onMakeAlias behavior in
ModelsPage.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b4bf820c-33cb-4ae0-b78a-7fc38c4f5406
📒 Files selected for processing (31)
alembic/versions/b5d7f9a1c3e6_move_stored_aliases_into_routing_policies.pydocs/api-reference.mddocs/dashboard.mddocs/routing.mdsrc/gateway/api/routes/aliases.pysrc/gateway/static/dashboard/assets/ActivityPage-DvM1Qq47.jssrc/gateway/static/dashboard/assets/BudgetsPage-BwbBPut6.jssrc/gateway/static/dashboard/assets/ConfirmDialog-BMAUR-Vt.jssrc/gateway/static/dashboard/assets/DocsPage-BoOWI6AJ.jssrc/gateway/static/dashboard/assets/KeysPage-CJtqAEHt.jssrc/gateway/static/dashboard/assets/ModelScopeControl-BoQJEWO0.jssrc/gateway/static/dashboard/assets/ModelsPage-DoBSThuy.jssrc/gateway/static/dashboard/assets/OverviewPage-BdYEUAon.jssrc/gateway/static/dashboard/assets/ProvidersPage-BWtXuXHa.jssrc/gateway/static/dashboard/assets/RoutingPage-D6RB-Kzs.jssrc/gateway/static/dashboard/assets/SettingsPage-CxHdvm0U.jssrc/gateway/static/dashboard/assets/TablePagination-CeoyZYzl.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-DCkaJoq3.jssrc/gateway/static/dashboard/assets/UsagePage-BHBeFxGf.jssrc/gateway/static/dashboard/assets/UsersPage-BpaQskrG.jssrc/gateway/static/dashboard/assets/index-BNcwLMM7.jssrc/gateway/static/dashboard/assets/index-Bz4ocl69.csssrc/gateway/static/dashboard/index.htmlweb/AGENTS.mdweb/src/App.tsxweb/src/components/AppShell.tsxweb/src/pages/AliasesPage.test.tsxweb/src/pages/AliasesPage.tsxweb/src/pages/ModelsPage.tsxweb/src/pages/RoutingPage.test.tsxweb/src/pages/RoutingPage.tsx
💤 Files with no reviewable changes (3)
- web/src/pages/AliasesPage.tsx
- web/src/pages/AliasesPage.test.tsx
- web/src/components/AppShell.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/App.tsx
- docs/dashboard.md
- src/gateway/static/dashboard/index.html
- docs/routing.md
The Playwright suite navigated to the deleted Aliases page, so removing the tab broke it. It is a hand-maintained list of nav items outside the diff, which is the class of thing a code change silently invalidates, and I did not grep for it. The nav walk now visits Routing, "create an alias" becomes "create a routing policy", and a second case covers growing one a fallback chain, which also pins that the chain section stays hidden until asked for. The combobox is matched on a substring rather than an anchored name, matching the alias test this replaces: the accessible name carries the required marker, so an anchored match never hits. Role-scoping is what separates the "Serves" field from the "Serves" column header. Run locally this time (8 passed) rather than left to CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 01:40 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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 `@web/e2e/dashboard.spec.ts`:
- Around line 118-126: Update the dashboard tests around the created policy rows
so assertions for the model and fallback are scoped within the specific “fast”
and “chained” row containers rather than matching page-wide elements. Add test
isolation by resetting or deleting existing routing-policy state between runs,
ensuring stale policies cannot satisfy these assertions.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 9ff01a29-33fc-4bcb-afe3-3decaedeb0c6
📒 Files selected for processing (1)
web/e2e/dashboard.spec.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class named routing policies to otari (standalone mode) so a caller-provided model name can drive conditional selection, retryable-failure failover, and enforced guardrails, while also persisting routing attribution to usage logs and surfacing it in the dashboard and APIs.
Changes:
- Introduces routing policy schema, compilation (pure compiler), and a local multi-attempt executor (
walk_attempts) plus supporting shared types. - Adds stored policy cache + refresher, config validation, CLI explain command, and new DB tables/migrations for routing policies and usage attribution.
- Updates dashboard navigation/pages (Aliases → Routing), usage/activity UI to represent absorbed attempts, docs, Postman collection, and rebuilds the committed dashboard bundle.
Reviewed changes
Copilot reviewed 79 out of 83 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/ModelsPage.tsx | Routes “Make alias” action to the new Routing page. |
| web/src/pages/AliasesPage.tsx | Removes the old Aliases page implementation. |
| web/src/pages/ActivityPage.tsx | Adds absorbed-row styling and a Routing attribution column. |
| web/src/components/AppShell.tsx | Sidebar nav: Aliases → Routing (icon + path). |
| web/src/App.tsx | Adds /routing route and redirects legacy /aliases to /routing. |
| web/src/api/types.ts | Adds routing policy request/response types + usage attribution fields. |
| web/src/api/hooks.ts | Adds TanStack Query hooks for routing policy CRUD + explain. |
| web/e2e/dashboard.spec.ts | Updates E2E flow to create policies and add fallback chains. |
| web/AGENTS.md | Updates dashboard docs to reflect Routing page absorbing Aliases. |
| tests/unit/test_pipeline_settlement.py | Adds characterization tests for non-streaming settlement behavior. |
| tests/unit/test_check_architecture.py | Adds architecture rule coverage for shared gateway/types leaf layer. |
| tests/integration/test_usage_endpoint.py | Extends expected usage row shape with routing attribution nulls. |
| src/gateway/types/budget_state.py | Adds leaf BudgetState type for routing budget conditions. |
| src/gateway/types/attempt.py | Adds leaf Attempt type shared by services and API execution. |
| src/gateway/static/dashboard/index.html | Updates bundle asset hashes after dashboard rebuild. |
| src/gateway/static/dashboard/assets/TablePagination-CeoyZYzl.js | Rebuilt dashboard asset. |
| src/gateway/static/dashboard/assets/OverviewPage-BdYEUAon.js | Rebuilt/new dashboard asset. |
| src/gateway/static/dashboard/assets/OverviewPage-B28JIhAq.js | Removed old dashboard asset after rebuild. |
| src/gateway/static/dashboard/assets/ModelScopeControl-DOAM-IRn.js | Removed old dashboard asset after rebuild. |
| src/gateway/static/dashboard/assets/ModelScopeControl-BoQJEWO0.js | Rebuilt/new dashboard asset. |
| src/gateway/static/dashboard/assets/ConfirmDialog-BMAUR-Vt.js | Rebuilt dashboard asset. |
| src/gateway/static/dashboard/assets/AliasesPage-BQwHhTMQ.js | Removed old Aliases bundle after page removal. |
| src/gateway/services/routing/compiler.py | New compiler: PolicySpec → ordered Attempt plan + guardrails + drops. |
| src/gateway/services/routing/init.py | Exposes routing compiler API from gateway.services.routing. |
| src/gateway/services/provider_kwargs.py | Resolves static policy targets like aliases for non-completion surfaces. |
| src/gateway/services/policy_store.py | Adds stored routing policy cache + TTL refresher + startup loader. |
| src/gateway/services/guardrails.py | Adds on_unavailable behavior when guardrails service is unreachable. |
| src/gateway/services/budget_service.py | Adds get_budget_state() for policy budget conditions. |
| src/gateway/models/routing.py | New routing policy schema (extra=forbid) + validation invariants. |
| src/gateway/models/guardrails.py | Adds on_unavailable to GuardrailConfig. |
| src/gateway/models/entities.py | Adds routing_policies entity + usage log attribution columns. |
| src/gateway/main.py | Loads policies at startup and runs policy refresher task in standalone mode. |
| src/gateway/core/config.py | Adds routing config block + startup validation for routing policies. |
| src/gateway/cli.py | Adds otari routing explain command for offline compilation inspection. |
| src/gateway/api/routes/usage.py | Persists/exposes routing attribution; counts requests excluding absorbed rows. |
| src/gateway/api/routes/responses.py | Adds local attempt kwargs wiring for multi-attempt execution. |
| src/gateway/api/routes/models.py | Lists routing policies in /v1/models (dynamic policies as pricing_source=dynamic). |
| src/gateway/api/routes/messages.py | Adds local attempt kwargs wiring for multi-attempt execution. |
| src/gateway/api/routes/chat.py | Adds local attempt kwargs wiring for multi-attempt execution. |
| src/gateway/api/routes/aliases.py | Adds validation to prevent alias names shadowing routing policy names. |
| src/gateway/api/routes/_attempts.py | New local multi-attempt walker with absorbed/terminal hooks and retry classification. |
| src/gateway/api/main.py | Registers the new routing router. |
| scripts/seed_routing_demo.py | Adds demo seeding script for policies + traffic generation. |
| scripts/check_architecture.py | Adds new layer rule for shared leaf types under gateway/types. |
| docs/routing.md | New routing policies documentation. |
| docs/public/otari.postman_collection.json | Adds routing endpoints + updates usage status filter docs. |
| docs/models.md | Cross-links aliases to routing policies; fixes missing Content-Type in curl examples. |
| docs/index.md | Adds routing policies doc link. |
| docs/dashboard.md | Updates dashboard guide for Routing page + absorbed semantics. |
| docs/configuration.md | Adds routing to config reference and env override notes. |
| docs/api-reference.md | Adds routing policy endpoints to management API reference. |
| config.example.yml | Adds example routing: block with commentary. |
| alembic/versions/f4c6a8b0d2e5_add_usage_logs_routing_attribution.py | Adds usage_logs routing attribution columns + indexes. |
| alembic/versions/e8b1d3f5a7c9_add_routing_policies.py | Adds routing_policies table + indexes/constraints. |
| alembic/versions/b5d7f9a1c3e6_move_stored_aliases_into_routing_policies.py | Migrates stored aliases from model_aliases → routing_policies. |
| .gitignore | Ignores SQLite WAL sidecars. |
Comment on lines
+102
to
+106
| if name in all_policy_names(config): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=( | ||
| f"'{name}' is already a routing policy. An alias resolves before a policy, so this would " |
Member
Author
There was a problem hiding this comment.
Note: this reply was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's.
Right on both halves, and this is the sharpest framing of it: the migration drained the second store rather than retiring it, and POST /v1/aliases refills it.
Doing the second of your two options here, and the first as a follow-up decision:
docs/routing.mdnow carries the break as an explicit callout: for a pre-upgrade alias,GETstops listing it,DELETE404s, and re-POST400s. It also notes the dashboard is unaffected because it reads both stores, and that a code-only rollback leaves the moved rows unreadable.- Decide whether /v1/aliases should write routing policies instead of model_aliases #503 tracks whether
/v1/aliasesshould be rerouted atrouting_policies, deprecated, or documented as a permanent second store, with the tradeoffs written out. Recommendation there is deprecate then reroute, so an existing endpoint does not silently change what it writes mid-release.
Worth noting for anyone reading this thread: the two write paths already refuse each other's names (aliases.py rejects a name that is a policy, and the policy route rejects a name that is an alias), so the stores cannot shadow each other today. The cost is conceptual plus the upgrade break, not a live resolution bug.
khaledosman
reviewed
Aug 5, 2026
khaledosman
left a comment
khaledosman
left a comment
Member
There was a problem hiding this comment.
Reviewed the full diff (backend, migrations, dashboard, docs). The routing design holds up and the usage-accounting side is unusually careful: _row_status, _request_count_expr and _breakdown's residual_requests all agree, so the other row still reconciles with the tiles once a request can write more than one row. That is the part of an "extra rows per request" change that normally breaks silently, and it didn't.
One confirmed correctness bug (attribution on a refused reservation top-up), one wrong status/message pairing, and a test gap on the only code path that stops a fallover from overshooting a budget cap. Details inline, most substantive first.
Also worth adding to the PR description and the release notes: migration b5d7f9a1c3e6 is a breaking change to /v1/aliases for existing rows. It is documented in docs/routing.md:16, but the PR body doesn't mention it at all.
🤖 Generated with Claude Code
…tempts Four defects from review. A gateway-side refusal for a candidate (a refused reservation top-up, an unpriced fallback under `require_pricing`) and the tool-iteration cap both raised out of the attempt walker without reporting which candidate they happened on, so the exhausted-plan log fell back to the last attempt in the plan and blamed a provider that was never called. The `budget_used_pct` reachability check rejected any threshold at 100 or above, including `lt`/`lte`. "Still under the cap" is a usable rule, and the check runs on load, so a stored policy using it failed startup. The Overview page collapsed every non-error status to "ok". That preview is unfiltered, so an attempt a policy recovered from read as a served request. The Routing page kept the table mounted while the create form was open, so Edit stayed reachable from it and two forms could stack with neither closing except on cancel. Also corrects stale text: a `limits` field the policy spec never had, the `models.md` pointer to the deleted Aliases page, and the `gte`/`gt` wording in config.example.yml. Adds a guardrails doc section on fail-closed behavior. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 10:56 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/test_routing_policies.py (1)
670-687: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for
lte: 100.
tests/integration/test_routing_policies.pynow covers thegte: 100unreachable branch andlt: 100acceptance, but no test coverslte: 100; the PR objective includeslteas a usable boundary. Add a small policy creation test for{"lte": 100}so future changes cannot regress this case.🤖 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 `@tests/integration/test_routing_policies.py` around lines 670 - 687, Add a regression test alongside test_still_under_the_cap_is_a_usable_threshold that creates a routing policy using the budget_used_pct condition {"lte": 100} and asserts the policy creation succeeds with HTTP 200. Reuse the existing client, HEADERS, policy structure, and target values, changing only the operator needed to cover the lte boundary.
🤖 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 `@tests/integration/test_routing_policies.py`:
- Around line 670-687: Add a regression test alongside
test_still_under_the_cap_is_a_usable_threshold that creates a routing policy
using the budget_used_pct condition {"lte": 100} and asserts the policy creation
succeeds with HTTP 200. Reuse the existing client, HEADERS, policy structure,
and target values, changing only the operator needed to cover the lte boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f204ae6-427d-4588-9712-0d832fd739fe
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (32)
config.example.ymldocs/guardrails.mddocs/models.mddocs/routing.mdscripts/seed_routing_demo.pysrc/gateway/api/routes/_attempts.pysrc/gateway/api/routes/_helpers.pysrc/gateway/api/routes/routing.pysrc/gateway/models/entities.pysrc/gateway/models/routing.pysrc/gateway/services/guardrails.pysrc/gateway/static/dashboard/assets/ActivityPage-DFRQg69h.jssrc/gateway/static/dashboard/assets/BudgetsPage-C89NkGTn.jssrc/gateway/static/dashboard/assets/ConfirmDialog-DW0UT0p7.jssrc/gateway/static/dashboard/assets/DocsPage-DcNuuBLg.jssrc/gateway/static/dashboard/assets/KeysPage-x5gg-uxE.jssrc/gateway/static/dashboard/assets/ModelScopeControl-9Jv41cUf.jssrc/gateway/static/dashboard/assets/ModelsPage-Cz-0AzRW.jssrc/gateway/static/dashboard/assets/OverviewPage-D-7Ey9G1.jssrc/gateway/static/dashboard/assets/ProvidersPage-p62THQ55.jssrc/gateway/static/dashboard/assets/RoutingPage-BYyTue4O.jssrc/gateway/static/dashboard/assets/SettingsPage-Bg4TDn9K.jssrc/gateway/static/dashboard/assets/TablePagination-BqqBwgF3.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-BjHVZO3Y.jssrc/gateway/static/dashboard/assets/UsagePage-D3-FUt4w.jssrc/gateway/static/dashboard/assets/UsersPage-BQKXSoJI.jssrc/gateway/static/dashboard/assets/index-C0Mahcvr.jssrc/gateway/static/dashboard/index.htmltests/integration/test_routing_policies.pytests/unit/test_attempt_walker.pyweb/src/pages/OverviewPage.tsxweb/src/pages/RoutingPage.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- src/gateway/static/dashboard/index.html
- config.example.yml
- src/gateway/services/guardrails.py
- docs/models.md
- web/src/pages/RoutingPage.tsx
- src/gateway/models/routing.py
- src/gateway/api/routes/_attempts.py
- docs/routing.md
- src/gateway/models/entities.py
- src/gateway/api/routes/routing.py
Four fixes from the CodeRabbit pass on #492. A cancelled request no longer becomes a provider failure. The broad `BaseException` catch in the attempt walker exists so a provider client raising outside the `Exception` hierarchy still falls through to the next candidate, but it also caught `asyncio.CancelledError` from a disconnected caller: that recorded an abandoned attempt against a provider which had answered fine, and folded the cancellation into a 502 the server was waiting to unwind. Cancellation is now re-raised ahead of the catch. The alias move refuses a name that exists in both stores instead of picking a winner. Alias resolution runs first today, so completing the move would have deleted the alias and quietly handed the name to a policy that may serve a different model. The downgrade had the sharper version of the same bug: on a clash it deleted the policy without writing the alias, dropping the target entirely. Both directions now abort naming the conflict, which is a database that already needs a human. The routing docs claimed a budget condition never matches for the master key. It does: a master-key request has to name the billed user, and conditions are evaluated against that user's budget, so a master-key request can take a tier-down branch. The e2e fallback assertion was page-wide, so a `chained` policy saved without its fallback would still have passed. It is scoped to its own row. Adds tests/unit/test_alias_to_policy_migration.py, which drives the migration against SQLite so the data behavior (move, scope, timestamps, both clash refusals, and the chained-policy downgrade skip) is pinned without a Postgres fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 12:02 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This was referenced Aug 5, 2026
Addresses the review findings from @khaledosman and Copilot on #492. `/v1/usage/summary?status=absorbed` reported `request_count: 0` beside non-zero cost and tokens. `request_count` excludes absorbed rows so a recovered chain counts as one request; filtering *to* absorbed made every row in scope an excluded one. Filtering to the attempts makes them the unit being asked about, so those queries count rows. The Activity page offers that filter, so an operator arriving at Usage through it saw tiles that looked broken. An empty compiled plan no longer blames model access for every cause. `NoEligibleCandidatesError` derived its message from nothing and its status from a hardcoded 403, so an operator who had deleted a provider instance was told to audit their allow-lists. The status is now derived: 403 when access rules filtered the candidates, 502 when none of them resolve to a configured provider, with wording to match. Targets stay off the caller-facing string. An unavailable router warns once per policy and router rather than once per request. The condition is static config compiled on every request through the policy, so the unconditional warning was a log line per request forever. The message now also names the policy. The Activity routing column no longer renders `attempt 1/2 · ` with a dangling separator when a row carries a position but no selection reason. `otari routing explain` says that it reads config only, so an operator whose policies were all created through the dashboard is pointed at `POST /v1/routing/policies/explain` instead of being told, on a gateway with several policies, that none are configured. Documents two things the code did without saying so: the migration is a breaking change for `/v1/aliases` callers (pre-existing rows stop listing, `DELETE` 404s, re-`POST` 400s), and a routing policy withholds every one of its targets from `GET /v1/models`, so a directly callable model can vanish from the catalogue because an unrelated policy lists it as a fallback. The downgrade docstring no longer implies a multi-target policy survives a full downgrade; the next revision drops the table either way. Adds tests/unit/test_pipeline_failover_topup.py, covering the mid-failover reservation top-up that had no test at all despite being the only thing stopping a fallover to a pricier candidate from spending past an approved cap, and tests/unit/test_routing_compiler.py for the empty-plan statuses and the warn-once behavior. Readability: the absorbed-attempt guard reads `attempt.position < len(attempts)` rather than an identity comparison against the tail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 5, 2026 12:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This was referenced Aug 5, 2026
njbrake
added a commit
that referenced
this pull request
Aug 6, 2026
A disconnected caller raises asyncio.CancelledError, which derives from BaseException, so the broad catch meant to let non-Exception provider clients fall through to the next candidate instead classified the cancellation as an "unknown" upstream error, recorded an abandoned attempt against a provider that answered fine, and converted it into a provider-failure HTTP exception that suppressed the cancellation. Guard the broad catch with a re-raise, mirroring the standalone walker fix in #492. Add a unit test asserting the cancellation propagates and no abandoned-attempt row is recorded. Fixes #501
This was referenced Aug 6, 2026
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
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.
Description
Closes #463. Also delivers the observability half of #401.
An alias maps one name to one model. This turns that into a routing policy: a
model name your callers send, which decides which real model serves the request,
what is tried after a retryable failure, and which guardrails always run. An alias
is the one-target case, so
aliases:keeps working as its shorthand and nothingexisting changes behavior.
The gap this closes: a standalone gateway had no failover at all. The
multi-attempt walker existed but was hybrid-only, driven by the otari.ai resolve
payload, so any provider blip was a 502.
Policies are also fully manageable from the dashboard (new Routing page) and
over
/v1/routing/policies, so nothing here requires editing a file andrestarting.
Design decisions worth reviewing
Two axes, deliberately separate.
selectdecides where the plan starts;on_failureis what runs after a failure. One combined list would make "did thisentry not apply, or did it fail?" ambiguous in every log line. The fallthrough is
an explicit
defaultrather than a positional last entry, so a misordered policyis refused at load instead of silently carrying dead rules.
instanceandproviderare carried separately through the walker. Pricing,budgets, and the usage log key on the instance, while any-llm dispatches against
the implementation. Collapsing them would silently re-key
usage_logs.providerand break pricing for every named-instance deployment, and it would ship green
because the existing test does not cover the fallback path.
absorbedis a third outcome, not an error. A recovered attempt writes its ownusage row correlated by
request_group_id, withstatus="absorbed". Every errormetric in the product counts
status == "error"exactly, andrequest_countexcludes absorbed rows, so a working fallback chain cannot read as an outage. This
is why
lib/overview.ts's 2% amber threshold and the activity timeline stay honest.The reservation tops up before each later candidate. A chain that falls over to
a pricier model would otherwise run against the cheaper model's reservation and
take spend past a cap the gate already approved. A refused top-up stops the chain
rather than overshooting.
401/403 are terminal locally, unlike hybrid mode, which retries them because a
workspace key can be rotated upstream. A standalone operator owns both keys, so
failing over would move traffic and spend to another provider and hide the
misconfiguration.
Every schema model is
extra="forbid".GatewayConfigisextra="ignore", soa typo'd key inside a policy would otherwise vanish and the policy would quietly
not do what it says.
A budget threshold at or above 100 is refused at load. The budget gate rejects
the request before selection, so such a rule could never fire; an operator writing
one believes they have configured "keep serving on a cheaper model after the budget
runs out" when they have configured nothing.
Verified against a live gateway, not only tests
Real OpenAI traffic through a real gateway, because several of these claims are
only checkable end to end:
absorbedattempt 1/2 +successattempt 2/2, same group idrequest_count: 10,error_count: 5reason=condition:user_idpricing_source: "dynamic"with no priceThat exercise caught three real bugs that the test suite had missed, including
stored policies being absent from
/v1/models(the existing test covered onlyconfig policies) and an exhausted chain writing no usage row at all.
Not included
select: [{router: knn}]validates, and currently warnsand falls through to the default rather than pretending.
fast" is notyet expressible.
than accepting as a silent no-op.
there, so a local policy name would reach it as an unknown model.
PR Type
Relevant issues
Closes #463. Delivers the observability half of #401.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).Checks
make lint,make typecheck,make openapi-check,make postman-check, 1222unit, 964 integration, 442 dashboard tests, and the committed bundle rebuilt.
Migrations:
e8b1d3f5a7c9(routing_policies) andf4c6a8b0d2e5(usageattribution), both verified up and down against PostgreSQL.
Review round
An independent review found six real defects, all now fixed in
84d62d3cwiththe tests that would have caught them. The most serious: policy guardrails were
never enforced, so the third item in this PR's title was a no-op with a full
supporting cast of schema, API, CLI output, dashboard editor, and documentation
asserting otherwise. Also fixed: failover reached only
/v1/chat/completions;streamed requests recorded no attribution; a terminal failure was attributed to a
candidate that was never called;
require_pricingwas bypassable through afallback; and
limitswas a knob that could not take effect, so it was removedrather than shipped.
AI Usage
AI Model/Tool used:
Claude Opus 5 via Claude Code.
Any additional AI details you'd like to share:
Written by Claude through back-and-forth with @njbrake, who made the design
calls: value-first sequencing, the
routing policiesname, noRoutingPortfornow, one usage row per attempt with a distinct
absorbedoutcome, refusing policynames in hybrid mode, and the
on_failure/ explicitdefaultkey naming.The implementation was reviewed by a separate agent with no knowledge of how it
was built, which is what caught the six defects above; the verification was also
run against a live gateway with real provider traffic, not only against tests.
Summary
absorbedinstead of successful requests.Technical notes
on_unavailable: monitorenables monitoring behavior.