feat(router): learned kNN router as a routing-policy backend - #188
Merged
Conversation
njbrake
temporarily deployed
to
integration-tests
June 19, 2026 17:29 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (44)
WalkthroughAdds learned kNN routing with configurable candidate pools, per-user routing memory, backend decision orchestration, management APIs, dashboard controls, persistence, tests, and documentation. Generated dashboard assets are refreshed to include the routing and documentation pages. ChangesLearned kNN Router
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
dpoulopoulos
self-requested a review
June 22, 2026 14:54
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 18:36 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 18:52 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 18:58 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 19:04 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 20:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 21:04 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 21:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 21:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 22:15 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 23:01 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 22, 2026 23:36 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 23, 2026 01:45 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 23, 2026 10:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 23, 2026 10:53 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
force-pushed
the
feat/router-knn
branch
3 times, most recently
from
June 23, 2026 13:38
11e226c to
90a3def
Compare
njbrake
temporarily deployed
to
integration-tests
June 23, 2026 15:20 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 23, 2026 15:30 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
temporarily deployed
to
integration-tests
June 23, 2026 17:02 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
njbrake
marked this pull request as ready for review
June 23, 2026 17:27
njbrake
commented
Jun 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
docs/routing.md (1)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language markers to fenced code blocks showing example headers.
Lines 138 and 168 have code fences without a language specifier. To satisfy markdown linting (and to make the intent clear to readers), add a language marker. Since these are example HTTP headers, use
`text`or`bash`:# Line 138: - `Otari-Router: off` serves... Instead of: +Otari-Conversation-Id: 4f9c2b10-... Use: + ``` text +Otari-Conversation-Id: 4f9c2b10-... +```This keeps the visual clarity of code blocks while satisfying the linter and readers.
Also applies to: 168-168
🤖 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/routing.md` at line 138, The fenced code blocks at lines 138 and 168 in the routing.md file are missing language specifiers on their opening code fence markers. Add a language marker to each opening fence by changing the bare triple backticks (```) to include a language identifier such as `text` or `bash` (for example, ```text or ```bash). This satisfies markdown linting requirements and provides clarity to readers about the content type of the code blocks containing example HTTP headers.Source: Linters/SAST tools
demo/router/generate_demo_dataset.py (1)
251-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository logger instead of
These progress/status lines should use the project logger pattern so output is structured and consistent with repo logging conventions.
As per coding guidelines,
**/*.py: use module logger fromgateway.log_configwith structured/contextual log messages using%sformatting placeholders.🤖 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 `@demo/router/generate_demo_dataset.py` around lines 251 - 269, Replace all print statements in the main function with structured logging using the project's logger from gateway.log_config. The three print statements that display progress output (showing candidate index and spread information, divergence summary, and file write confirmation) should be converted to logger calls using percent-style formatting placeholders (%s) instead of f-strings. Import the logger module at the top of the file and use appropriate log levels (info or debug) for these status messages to maintain consistency with repository logging conventions.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 `@demo/router/src/components/StepWalkthrough.tsx`:
- Around line 33-40: The global keyboard event listener in the useEffect hook
unconditionally triggers step navigation for ArrowLeft and ArrowRight keys,
which interferes with interactive controls like sliders that also use arrow
keys. Inside the onKey handler function, add a check to verify that the event
target is not an interactive element (such as an input, slider, or other form
control) before calling next() or prev(). You can check the event target's type
or role to determine if it is an interactive control, and only proceed with
navigation if the event originated from a non-interactive element.
In `@demo/router/src/components/TeachWalkthrough.tsx`:
- Around line 24-38: The code assumes demo.items is non-empty, causing crashes
when it has zero items (accessing demo.items[0] on line 24 and reducing an empty
array on line 38). Add an early empty-state guard clause at the beginning of the
TeachWalkthrough component that checks if demo.items.length is zero and handles
this case before deriving the item variable and initializing the useState hooks
that depend on having data. This ensures the component gracefully handles empty
datasets before attempting to access array indices or perform reduce operations.
In `@demo/router/src/main.tsx`:
- Around line 2-7: The import statement in main.tsx is using a default import
for ReactDOM from react-dom/client, but React 19 exports createRoot as a named
export only. Change the import statement from `import ReactDOM from
"react-dom/client"` to `import { createRoot } from "react-dom/client"`, then
update the ReactDOM.createRoot call to use createRoot directly, replacing
`ReactDOM.createRoot(document.getElementById("root") as
HTMLElement).render(...)` with `createRoot(document.getElementById("root") as
HTMLElement).render(...)`.
In `@demo/router/src/router-sim.ts`:
- Around line 63-91: When no eligible neighbors exist in a task partition, the
neighborItems array becomes empty, causing the meanQuality calculation to
default to 0 for all candidates, which then selects the cheapest model instead
of preserving the fallback behavior. After the neighborItems assignment and
before the pool.map call that creates the candidates array, add an explicit
check: if neighborItems.length is 0, set winner to the fallback value
(strongest) and either skip the candidate scoring loop or handle it as a special
case to preserve the documented fallback behavior.
In `@src/gateway/api/routes/router.py`:
- Around line 8-10: The module docstring for the POST
/v1/router/preferences/rank endpoint currently describes the old behavior of
writing one routing-memory record per scored model, but the implementation now
writes a single routing-memory example with a score map instead. Update the
docstring (lines 8-10) to accurately reflect the current storage model where one
submission creates one routing-memory example containing a score map for all
models, plus one audit row, rather than describing multiple records per model.
- Around line 51-52: The `models` field in the schema has no maximum length
constraint, allowing users to submit arbitrarily large arrays that trigger
unbounded concurrent provider calls at line 145, creating excessive upstream
load. Add a `max_length` parameter to the Field definition for the `models` list
to enforce a schema-level hard cap on the number of models allowed.
Additionally, implement a semaphore or similar concurrency limiting mechanism
around line 145 where the provider calls are launched asynchronously to ensure
that only a small, bounded number of concurrent calls execute regardless of the
input array size.
- Around line 141-143: The ModelResponse being returned in the exception handler
is exposing raw exception details via str(exc) directly to clients, which can
leak sensitive upstream provider information. Replace the str(exc) value passed
to the error parameter in the ModelResponse with a generic, user-friendly error
message that does not reveal internal details. The actual error details are
already being captured server-side in the logger.warning call, so keep those
specifics there and only return a safe generic message to the client.
In `@src/gateway/models/entities.py`:
- Around line 310-313: The docstring for the RouterPreference class incorrectly
describes the storage model by stating that rank submissions write one
RoutingMemory row per scored model. Update this docstring to accurately reflect
the new storage model where one row is written per example with per-model
qualities stored together, rather than separate rows per model. This ensures the
documentation matches the actual implementation and prevents confusion during
future maintenance.
In `@src/gateway/services/knn_router.py`:
- Around line 235-247: The database write operation in the RoutingMemory
creation block does not have proper error handling for commit failures. Wrap the
await db.commit() call in a try/except block that catches SQLAlchemyError, calls
db.rollback() in the except clause on any database error, and re-raises an
appropriate mapped API or domain error. This same transaction handling pattern
must also be applied to the other database write operation mentioned at lines
374-399 to ensure consistent error handling across all router DB writes and
prevent sessions from remaining in failed transaction states.
- Around line 216-248: The record_preference() method opens and commits its own
database session independently, causing a race condition where the
RouterPreference row can be persisted before this method is called, but if the
embedding or insert fails here, the audit row exists without the corresponding
routing-memory example. Modify record_preference() to accept an optional
AsyncSession parameter and use that session directly instead of creating a new
one with async with create_session(). This allows the calling code in the route
handler to manage both the RouterPreference and RoutingMemory writes as a single
atomic transaction, ensuring both succeed or fail together.
- Around line 193-195: In the exception handler block where embedding fails (in
the except Exception clause with logger.warning), change the logging to avoid
logging the raw exception text which may contain user-derived content or
sensitive data from provider exceptions. Instead, log only the exception class
name or type using type(exc).__name__ rather than logging the full exc object
with the %s placeholder. This prevents privacy leaks from user payloads or
provider-specific error messages in logs while still providing debugging
context.
In `@tests/integration/test_config_env_loading.py`:
- Around line 150-166: The test_router_knob_defaults function only clears
OTARI_ROUTER_* environment variables but GatewayConfig also accepts legacy
GATEWAY_ROUTER_* fallback environment variables, making the assertions
environment-dependent. Add the legacy GATEWAY_ROUTER_* environment variables
(such as GATEWAY_ROUTER_ALPHA, GATEWAY_ROUTER_K, GATEWAY_ROUTER_SEED_COUNT,
GATEWAY_ROUTER_GRANULARITY, GATEWAY_ROUTER_CANDIDATES, and
GATEWAY_ROUTER_EMBEDDING_MODEL) to the loop in monkeypatch.delenv calls so both
namespaces are cleared before instantiating GatewayConfig().
In `@tests/integration/test_router_preferences.py`:
- Around line 87-105: The _build_client function hardcodes "test-key" as the
OpenAI API key in the providers configuration, which causes issues when running
in live mode where the OPENAI_API_KEY environment variable is available. Modify
the _build_client function to check if the OPENAI_API_KEY environment variable
exists and use it instead of the hardcoded "test-key" value in the providers
dict. Apply this same fix to all other occurrences of this pattern in the file
(at lines 131-135 and 463-467 as indicated) to ensure consistency across all
test client builders.
---
Nitpick comments:
In `@demo/router/generate_demo_dataset.py`:
- Around line 251-269: Replace all print statements in the main function with
structured logging using the project's logger from gateway.log_config. The three
print statements that display progress output (showing candidate index and
spread information, divergence summary, and file write confirmation) should be
converted to logger calls using percent-style formatting placeholders (%s)
instead of f-strings. Import the logger module at the top of the file and use
appropriate log levels (info or debug) for these status messages to maintain
consistency with repository logging conventions.
In `@docs/routing.md`:
- Line 138: The fenced code blocks at lines 138 and 168 in the routing.md file
are missing language specifiers on their opening code fence markers. Add a
language marker to each opening fence by changing the bare triple backticks
(```) to include a language identifier such as `text` or `bash` (for example,
```text or ```bash). This satisfies markdown linting requirements and provides
clarity to readers about the content type of the code blocks containing example
HTTP headers.
🪄 Autofix (Beta)
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
Run ID: a240f8a6-be5d-4911-8049-619de44f244b
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (45)
alembic/versions/f0a1b2c3d4e5_add_router_tables.pydemo/router/.gitignoredemo/router/README.mddemo/router/generate_demo_dataset.pydemo/router/index.htmldemo/router/package.jsondemo/router/src/App.tsxdemo/router/src/components/CodeJson.tsxdemo/router/src/components/KnnGraph.tsxdemo/router/src/components/StepWalkthrough.tsxdemo/router/src/components/TeachWalkthrough.tsxdemo/router/src/components/Walkthrough.tsxdemo/router/src/components/ui/button.tsxdemo/router/src/components/ui/card.tsxdemo/router/src/components/ui/index.tsdemo/router/src/demo-config.tsdemo/router/src/demo_prompts.jsondemo/router/src/format.tsdemo/router/src/globals.cssdemo/router/src/main.tsxdemo/router/src/router-sim.tsdemo/router/src/styles/brand-tokens.cssdemo/router/src/types.tsdemo/router/tsconfig.jsondemo/router/vite.config.tsdocs/api-reference.mddocs/configuration.mddocs/index.mddocs/routing-scaling.mddocs/routing.mdsrc/gateway/api/deps.pysrc/gateway/api/main.pysrc/gateway/api/routes/chat.pysrc/gateway/api/routes/router.pysrc/gateway/core/config.pysrc/gateway/main.pysrc/gateway/models/entities.pysrc/gateway/services/knn_router.pysrc/gateway/services/router_backend.pytests/integration/test_config_env_loading.pytests/integration/test_router_preferences.pytests/integration/test_router_seam_e2e.pytests/unit/test_knn_router.pytests/unit/test_router_backend.pytests/unit/test_router_header.py
njbrake
changed the title
njbrake
marked this pull request as ready for review
August 6, 2026 14:46
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
njbrake
temporarily deployed
to
integration-tests
August 6, 2026 14:46 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
web/src/components/RouterReadiness.tsx (2)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a colocated test file for this component.
RoutingPage.test.tsxexercises this panel through the page, which is good integration coverage. The repository standard also asks for a colocated test next to the component, soweb/src/components/RouterReadiness.test.tsxwould cover the parts that are hard to reach from the page:seed === 0inWarmth,recordsaboveseed, and the error branch on line 108.As per coding guidelines: "Add colocated Vitest tests for changed behavior (
Foo.tsx→Foo.test.tsx)".🤖 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/components/RouterReadiness.tsx` at line 46, Add a colocated Vitest test file for the RouterReadiness component, targeting the exported RouterReadiness flow and its internal Warmth behavior. Cover Warmth with seed === 0, records greater than seed, and the error branch near the component’s error handling, while following existing repository test conventions.Source: Coding guidelines
116-131: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHoist
status.databefore reading it in themapcallback.
status.datanarrows outside the callback, but TypeScript 5.6 resets that narrowing inside the nestedstatus.data.tasks.map(...), sostatus.data.seed_countis still possiblyundefinedthere. Capture aconst data = status.databefore the JSX and usedataeverywhere inside the branch.🤖 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/components/RouterReadiness.tsx` around lines 116 - 131, Capture status.data in a local data constant at the start of the truthy branch, then use data for the default pool, tasks map, and seed_count accesses, including inside the map callback, so TypeScript preserves the narrowing.Source: Coding guidelines
web/src/pages/RoutingPage.test.tsx (1)
577-585: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe last assertion in this test cannot fail.
Line 584 checks that no POST happened, but the test never presses "Create policy", and the fifth candidate row is still empty, so
canSubmitis alreadyfalse. The two assertions that carry the intent are on lines 582 and 583. Consider dropping line 584, or press "Create policy" first so the assertion actually proves that submission is blocked.🤖 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 577 - 585, The final POST assertion in the candidate-cap test is not meaningful because submission is never attempted while the fifth row is empty. Update the test around the “+ Another model” loop to either remove the calls.some POST assertion or complete the fifth candidate and click “Create policy” so it verifies submission is blocked; preserve the existing cap and explanatory-text assertions.web/src/pages/RoutingPage.tsx (1)
522-530: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive each fallback radio a distinct accessible name.
Every radio in the pool exposes the same accessible name, "Serves when unsure". A sighted operator reads the name against the adjacent
Model Nfield, but a screen-reader user hears the identical name two to five times with no indication of which model each radio marks. Adding the candidate to the name, and wrapping the group in afieldset, keeps the visible copy short while making the choice unambiguous.♿ Proposed refactor
<label className="flex items-center gap-2 pb-2 text-xs text-[var(--otari-ink)]"> <input type="radio" name="router-safe-choice" + aria-label={`Serves when unsure: ${entry.trim() === "" ? `model ${index + 1}` : entry}`} checked={safeIndex === index} onChange={() => setSafeIndex(index)} /> Serves when unsure </label>Note that
RoutingPage.test.tsxqueries these radios by the name/serves when unsure/i(lines 404 and 417), so the suggested prefix keeps those queries matching.🤖 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 522 - 530, Update the fallback radio group around the safe-choice controls in RoutingPage to use a fieldset with an accessible group label, and give each radio a distinct accessible name by prefixing “Serves when unsure” with its associated candidate/model identifier. Preserve the existing visible label text and ensure the accessible name still matches queries for /serves when unsure/i.tests/unit/test_routing_decide.py (1)
207-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test proves the opt-out path, not the cold-pool path its comment describes.
The
recorderfixture replaces the backend with one that always ranks the pool, andopted_out=Truemakesdecide_orderingreturn before any backend call. So the "cold pool" case in the comment is never reached, and Line 271 already covers the opt-out. A backend that returns an emptyRoutingDecisionwould make the test match its own description.Small thing, and the intent is good: the silence-on-decline guarantee is worth locking down, because a warning per request on a cold router is exactly the log flood you are avoiding.
♻️ One way to cover the decline case
`@pytest.mark.asyncio` async def test_a_decline_is_not_warned_about( - config: GatewayConfig, recorder: _Recorder, router_warnings: Callable[[], list[str]] + config: GatewayConfig, monkeypatch: pytest.MonkeyPatch, router_warnings: Callable[[], list[str]] ) -> None: # A cold pool or an opted-out caller is normal operation, and warning on it # would log a line for every request a cold router serves, which is all of them # until someone teaches it. + class _ColdBackend: + async def rank(self, ctx: RoutingContext) -> RoutingDecision: + return RoutingDecision.decline("cold pool: 0/20 records for this user") + + monkeypatch.setattr( + "gateway.services.routing.decide.get_router_backend", lambda config, name: _ColdBackend() + ) await decide_ordering( - config, _spec(), policy_name="smart", user_id="u", allowlist=None, signal=_signal(opted_out=True) + config, _spec(), policy_name="smart", user_id="u", allowlist=None, signal=_signal() ) assert router_warnings() == []🤖 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_routing_decide.py` around lines 207 - 217, Update test_a_decline_is_not_warned_about to exercise the cold-pool decline path instead of the opted-out early return: configure the recorder/backend to return an empty RoutingDecision, invoke decide_ordering with an eligible signal, and retain the assertion that router_warnings() is empty. Keep the existing opt-out coverage separate and preserve the test’s silence-on-decline guarantee.tests/unit/test_knn_router.py (1)
86-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
totalis never passed by any test, and its comment describes behavior nothing exercises.No call site in this file uses
total=. The seed gate and the sparse gate are separated instead by tuningrouter_seed_countandrouter_k(Lines 128, 148). The comment therefore points a future reader at a mechanism that is not in use. Either drop the parameter or add the test that needed it.♻️ Proposed cleanup
def _wire( backend: KnnRoutingMemory, records: list[RoutingMemory], *, prices: dict[str, float] | None = None, query: tuple[float, ...] = (1.0, 0.0), - total: int | None = None, ) -> None: async def _embed(text: str) -> list[float]: return list(query) async def _load(user_id: str, task_id: str | None) -> list[RoutingMemory]: - # `total` pads the record count without inventing neighbors, which is how - # the seed gate and the sparse-neighborhood gate are tested separately. - padding = [] if total is None else [_both_good()] * max(0, total - len(records)) - return [*records, *padding] + return list(records)🤖 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_knn_router.py` around lines 86 - 95, Remove the unused total parameter and its padding logic from the test helper, along with the comment describing unexercised behavior. Keep the existing seed and sparse-neighborhood test coverage driven by router_seed_count and router_k.
🤖 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/f0a1b2c3d4e5_add_router_tables.py`:
- Around line 40-41: Update the upgrade migration near the existing
routing_memory indexes to add a composite index on user_id, embedding_model, and
task_id, and remove the corresponding index in downgrade().
In `@src/gateway/api/routes/responses.py`:
- Around line 428-430: Update the routing_signal construction to include
request_body.instructions before _responses_input_text(request_body.input),
preserving provider prompt order so routing decisions and sticky trace
identities reflect both instructions and input.
In `@src/gateway/api/routes/routing_memory.py`:
- Around line 65-80: Normalize inputs in the request model or handler before
persistence: reject whitespace-only values for prompt despite its current
min_length constraint, and trim task_id while converting blank results to None.
Update the flow around record_preference() and RouterPreference creation so
invalid prompts are rejected before any audit row is written and normalized task
IDs match request-header handling.
- Around line 304-355: Replace the per-example write/commit flow around
record_preference and the RouterPreference db.add calls with a batch service
method that embeds all examples, writes both routing-memory and audit records
using one session, and commits once after successful processing. Run eviction
once after the batch transaction commits, and preserve an explicit
partial-failure contract so examples completed before an embedding failure
remain correctly reported and handled.
In `@src/gateway/core/config.py`:
- Around line 367-374: Update the description for router_max_records_per_user to
explicitly document that 0 disables eviction and allows unlimited stored
routing-memory records; retain the existing cap and oldest-record eviction
behavior for positive values.
In `@src/gateway/services/routing/decide.py`:
- Around line 118-131: The backend ranking call in decide_ordering must be
bounded and fail open to the policy default. Add asyncio and a positive
router_decision_timeout_seconds setting (or an equivalent module constant), wrap
backend.rank in asyncio.wait_for, and catch timeout or ranking exceptions so the
decline path returns the existing default decision instead of propagating an
error.
In `@src/gateway/services/routing/knn.py`:
- Around line 350-352: Update _candidate_prices to avoid awaiting _input_price
once per pool member; batch the pricing lookup for all candidate models in a
single query keyed by the resolved instance and model values. Reuse or add a
batched pricing helper that returns the existing dict[str, float] shape,
preserving the current behavior for the supplied pool.
- Around line 290-305: Bound _load_records independently of eviction by ordering
matching RoutingMemory rows newest-first and limiting the query to
self.scan_limit. Initialize self.scan_limit in __init__ as self.max_records or
_DEFAULT_SCAN_LIMIT so max_records=0 still applies a finite cap, while
preserving the existing user, embedding-model, and task_id filters.
- Around line 312-346: Update record_preference() so _evict_if_needed() runs
within the same database session and transaction as the preference insert,
committing only after both succeed. Refactor _evict_if_needed() to delete
records using a NOT EXISTS or subquery-based condition that retains the newest
max_records rows, avoiding materializing keep_ids or binding one parameter per
retained id. Preserve rollback and error propagation on transaction failure.
In `@web/src/pages/RoutingPage.tsx`:
- Around line 770-776: Update the hint text in the candidates guidance near the
routing page results to refer to the existing “Examples” row action instead of
“Router,” while preserving the surrounding instructions and formatting.
---
Nitpick comments:
In `@tests/unit/test_knn_router.py`:
- Around line 86-95: Remove the unused total parameter and its padding logic
from the test helper, along with the comment describing unexercised behavior.
Keep the existing seed and sparse-neighborhood test coverage driven by
router_seed_count and router_k.
In `@tests/unit/test_routing_decide.py`:
- Around line 207-217: Update test_a_decline_is_not_warned_about to exercise the
cold-pool decline path instead of the opted-out early return: configure the
recorder/backend to return an empty RoutingDecision, invoke decide_ordering with
an eligible signal, and retain the assertion that router_warnings() is empty.
Keep the existing opt-out coverage separate and preserve the test’s
silence-on-decline guarantee.
In `@web/src/components/RouterReadiness.tsx`:
- Line 46: Add a colocated Vitest test file for the RouterReadiness component,
targeting the exported RouterReadiness flow and its internal Warmth behavior.
Cover Warmth with seed === 0, records greater than seed, and the error branch
near the component’s error handling, while following existing repository test
conventions.
- Around line 116-131: Capture status.data in a local data constant at the start
of the truthy branch, then use data for the default pool, tasks map, and
seed_count accesses, including inside the map callback, so TypeScript preserves
the narrowing.
In `@web/src/pages/RoutingPage.test.tsx`:
- Around line 577-585: The final POST assertion in the candidate-cap test is not
meaningful because submission is never attempted while the fifth row is empty.
Update the test around the “+ Another model” loop to either remove the
calls.some POST assertion or complete the fifth candidate and click “Create
policy” so it verifies submission is blocked; preserve the existing cap and
explanatory-text assertions.
In `@web/src/pages/RoutingPage.tsx`:
- Around line 522-530: Update the fallback radio group around the safe-choice
controls in RoutingPage to use a fieldset with an accessible group label, and
give each radio a distinct accessible name by prefixing “Serves when unsure”
with its associated candidate/model identifier. Preserve the existing visible
label text and ensure the accessible name still matches queries for /serves when
unsure/i.
🪄 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: f52fc820-74e9-4e71-9e2c-0ae534daf23e
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (62)
alembic/versions/f0a1b2c3d4e5_add_router_tables.pydocs/api-reference.mddocs/configuration.mddocs/dashboard.mddocs/index.mddocs/public/otari.postman_collection.jsondocs/routing-scaling.mddocs/routing.mdscripts/seed_routing_demo.pysrc/gateway/AGENTS.mdsrc/gateway/api/deps.pysrc/gateway/api/main.pysrc/gateway/api/routes/_helpers.pysrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/chat.pysrc/gateway/api/routes/messages.pysrc/gateway/api/routes/responses.pysrc/gateway/api/routes/routing.pysrc/gateway/api/routes/routing_memory.pysrc/gateway/cli.pysrc/gateway/core/config.pysrc/gateway/main.pysrc/gateway/models/entities.pysrc/gateway/models/routing.pysrc/gateway/services/pricing_init_service.pysrc/gateway/services/routing/__init__.pysrc/gateway/services/routing/backends.pysrc/gateway/services/routing/compiler.pysrc/gateway/services/routing/decide.pysrc/gateway/services/routing/knn.pysrc/gateway/static/dashboard/assets/ActivityPage-C5ofX9qk.jssrc/gateway/static/dashboard/assets/BudgetsPage--eVatwDr.jssrc/gateway/static/dashboard/assets/ConfirmDialog-CXWJ7SUm.jssrc/gateway/static/dashboard/assets/DocsPage-s_xvezWK.jssrc/gateway/static/dashboard/assets/KeysPage-BG-GJfIc.jssrc/gateway/static/dashboard/assets/ModelScopeControl-DyHkbW6e.jssrc/gateway/static/dashboard/assets/ModelsPage-DNqcJtqR.jssrc/gateway/static/dashboard/assets/ModelsPage-uwVSUUQm.jssrc/gateway/static/dashboard/assets/OverviewPage-Dl956LT-.jssrc/gateway/static/dashboard/assets/ProvidersPage-TstjvtEF.jssrc/gateway/static/dashboard/assets/RoutingPage-CZeNudr_.jssrc/gateway/static/dashboard/assets/RoutingPage-D1os8M2m.jssrc/gateway/static/dashboard/assets/SettingsPage-DnyaGTEO.jssrc/gateway/static/dashboard/assets/TablePagination-B55cx-co.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-B0d257Kp.jssrc/gateway/static/dashboard/assets/UsagePage-3rqO5_Gv.jssrc/gateway/static/dashboard/assets/UsersPage-ij9p_J3R.jssrc/gateway/static/dashboard/assets/index-2gdkqFtD.jssrc/gateway/static/dashboard/assets/index-B9YHHu7Y.csssrc/gateway/static/dashboard/assets/index-D-R1nuKP.jssrc/gateway/static/dashboard/index.htmltests/integration/test_config_env_loading.pytests/integration/test_routing_learned.pytests/unit/test_knn_router.pytests/unit/test_routing_compiler.pytests/unit/test_routing_decide.pytests/unit/test_routing_signal.pyweb/src/api/hooks.tsweb/src/api/types.tsweb/src/components/RouterReadiness.tsxweb/src/pages/RoutingPage.test.tsxweb/src/pages/RoutingPage.tsx
💤 Files with no reviewable changes (3)
- src/gateway/static/dashboard/assets/RoutingPage-D1os8M2m.js
- src/gateway/static/dashboard/assets/index-D-R1nuKP.js
- src/gateway/static/dashboard/assets/ModelsPage-uwVSUUQm.js
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/index.md
- src/gateway/api/deps.py
- src/gateway/models/entities.py
- docs/routing-scaling.md
A router is an optimization, so it must never be the reason a request cannot be served. Backends already decline on the failures they can name (a cold pool, an embedding error, a candidate with no pricing), but ranking also reads the database, so a failure there surfaced as a 500 for a request the policy's default target could have served. decide_ordering now wraps the backend call: any exception warns and declines, so the default target serves. One guard at the seam covers every backend and every failure rather than the ones each backend anticipated. Found in review of #188 by @khaledosman, whose budget-leak finding this is the tail of. The leak itself is structurally gone (the router runs before the reservation is taken, not after), but the 500 was still reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 6, 2026 15:03 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Validation canonicalized a score key to decide whether to accept it, while record_preference stored the raw string and knn._score matched candidates by exact string. So `openai/gpt-3.5-turbo` against a policy naming `openai:gpt-3.5-turbo` was accepted with a 200 and then never matched: the candidate vanished from the score map and the *expensive* model won unopposed at confidence 1.0, with a rationale that reads like the router working. The pool reported warm, no route could delete the records, and docs/routing.md promises a 400 for keys that do not name candidates, so a 200 reads as "usable". Validation now returns the mapping from the key as sent to the spelling its policy uses, and rank rewrites each example before storing. Two spellings of one candidate in one example is a 400 rather than one score silently discarded. With no learned policy for the user, keys are stored as sent, as before. Known residual: two learned policies for one user that spell the same model differently normalize to one spelling, so the other policy's lookup still misses. Storing the canonical form and canonicalizing the pool at lookup would close the class; tracked on #187. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate was `spec.router_backend is not None`, which is true for the whole policy rather than for this request. Select entries are evaluated in order, so a `when` entry ahead of the router wins outright and the ranking is discarded, after the request has already paid for an embedding call and a scan of the user's stored examples. It also emitted the router's decision log line naming a model that did not serve, which is worse than the wasted work: it makes the log lie. `selection_consults_router` walks `select` exactly as `_select_head` does and lives beside it in the compiler, so the two cannot disagree about which entry wins. Pure and synchronous like the rest of that module. The dashboard emits `[conditions, router, default]` for a policy with both, so this is the shape the form produces, not a hypothetical one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three problems in one area, all raised by CodeRabbit against the current code. Eviction deleted with `id NOT IN (<kept ids>)`, binding one host parameter per kept row. That is up to `router_max_records_per_user` parameters (5000 by default) in a single statement, and SQLite caps them at 999 on builds before 3.32, so the query that keeps the store bounded would itself fail on the default configuration. It now deletes rows strictly older than the oldest row being kept. Rows sharing that timestamp survive, so a batch written in one tick is never half evicted and the count can sit slightly above the cap until the next write. The per-request read had no limit and no ordering. Eviction is enforced lazily on write and only over a user's whole set, so nothing stopped a task partition from growing past the cap, and one decision would load and cosine-score every row it found. It now takes the newest `router_max_records_per_user` rows, which is the bound the operator already configured, with the same newest-first rule eviction uses. `0` means eviction is off, which is not a licence for an unbounded select, so the read falls back to the default bound. The field's description and docs/configuration.md now say that, since `0` otherwise reads as a hard cap. The migration and the ORM both gain `(user_id, embedding_model, task_id)`. A task-scoped read and the seed gate that counts the same partition both filter on all three; without it they walk every record the user has for that embedding model. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`task_id` was stored exactly as submitted while the request side trims `Otari-Router-Task` and treats blank as absent, so a label written as " support " created a partition that `Otari-Router-Task: support` could never reach, and `/status` listed it as a real pool with real records. A whitespace-only prompt passed `min_length=1`, and `record_preference` then returned 0 for it: an audit row was written, no routing-memory row was, and the `recorded` count in the response agreed with neither. Both are now field validators on the example, so they apply per example in a batch and the error names the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard added in b3473ef covers a backend that raises, not one that hangs. The work behind a ranking is an outbound embedding call plus a read of the stored examples, neither with a deadline of its own, so a hung embedding provider held the request open for as long as its own client allowed. That breaks the same premise the guard was for: a router is an optimization, so it can be neither the reason a request fails nor the reason it hangs. Five seconds, then decline to the policy's default target. Expiring costs the caller the cheaper model, not the request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`instructions` is part of the task, not decoration: the same `input` under "answer in one word" and "write a rigorous proof" are different jobs with different quality bars. The signal embedded only `input`, so both got the same kNN decision and, under trace-sticky granularity, the same conversation identity. Guardrails keep reading `input` alone, which is correct for them: they screen what the user sent. Routing has to read what the model was actually asked to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The policy form told the operator to open **Router** on the row, which is what the row action was called before it became **Examples**. Regenerated bundle and specs, the latter because the rank endpoint's docstrings changed in this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 6, 2026 15:23 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Only generated artifacts conflicted: docs/public/openapi.json, and 33 files in the committed dashboard bundle where both sides had rebuilt it under different content hashes (rename/rename and modify/delete churn). Neither is hand-authored, so both were regenerated from the merged source rather than hand-merged: the bundle by deleting it and running the build (emptyOutDir prunes stale hashes), the spec and Postman collection by regenerating from the merged app. No source file conflicted. main's work landed in streaming keepalives, usage filters, SDK codegen, and the dashboard's usage views, none of which touch where the routing code sits. One semantic conflict the merge could not show: main added tests/unit/test_sdk_endpoint_coverage.py, which requires every endpoint in the spec to be classified in scripts/sdk_codegen/sdk-endpoints.txt. This branch adds two, so they are now listed as not yet wrapped, alongside the routing-policy routes they belong with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
August 7, 2026 14:02 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
|
@besaleli and I chatted and aligned on project direction, agreeing that this PR was the appropriate first step to set the groundwork for future routing work and behavior 🚀 |
This was referenced Aug 7, 2026
This was referenced Aug 10, 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.
Note: this PR description was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose and code are Claude's.
Description
Fills the
router:seamPolicySpecalready reserved. A policy entry names a backend that ranks candidates per request, and the ranking becomes the plan the attempt walker already executes, so a learned policy inherits failover, absorbed usage rows, allow-list filtering, and guardrail mandates rather than being a second routing system beside them.The
knnbackend embeds the prompt, votes over the nearest scored examples, and scores each candidate aspredicted_quality - alpha * normalized_cost. Every uncertain case declines anddefaultserves: a cold pool, a sparse neighborhood, sub-floor confidence, tools present, an unpriced candidate, a failed embedding, an unknown backend,Otari-Router: off. So a learned policy is never worse than the failover policy it was written from, and a routed request that fails falls over to the router's second choice before reachingon_failure.selection_reasonon the usage row isrouter:knnwhen it chose,defaultwhen it declined.compile_policystays pure and synchronous; ranking is async, so it runs in the pipeline and arrives as a value, which is whyexplainshows the decline path.Teaching is
POST /v1/routing/preferences/rank, batched to 100 because a pool needs 20 examples before it routes.GET /v1/routing/statusreports warmth per pool. Both master-key gated, withuser_idnaming whose memory it is: memory is per user even for a global policy, since the records hold that user's prompts. The dashboard authors these policies and shows whether one can act yet. Standalone only, like every policy.Not in this PR, deliberately
compareendpoint. Seeing what each candidate answers is whatPOST /v1/chat/completionsalready does, on the path that reserves budget and logs usage.rankrefuses a score key no learned policy could route to: such records are unmatchable, and the pool would report warm while never routing.What a reviewer should push on
trace_stickylimits the damage.docs/routing-scaling.md.PR Type
Relevant issues
A v1 of #187, which stays open: cache-aware cost, passive learning, latency tracking, an ANN index past a few thousand records per user, a capability registry, a list/delete route for examples, and hybrid-mode routing memory all remain there.
Checklist
make lint,make typecheck,make test,npm test,make openapi-check,make postman-check).Two integration tests fail in the author's sandbox on any branch,
mainincluded, because they make a real outbound provider call:test_error_detail_leakageandtest_streaming_error_event. Everything else passes.AI Usage
AI Model/Tool used: Claude Code (Opus 5)
Any additional AI details you'd like to share: Code, tests, docs, and the dashboard work were generated by Claude through iterative back-and-forth with @njbrake, who directed the design decisions and reviewed the output.
Summary
knnrouting with confidence-based fallback and failover support.Technical notes