feat(routing): execute capability-aware policy profiles - #1012
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRouting profiles now execute live policy routing for explicit IDs and aliases. Request bodies provide tool and image evidence. Candidate capability evidence supports selection. Ineligible policies return traced 404 errors. Configuration validation, tests, and routing documentation cover the new behavior. ChangesLive policy routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RequestHandler
participant evidenceFromBody
participant routeModel
participant PolicyEvaluator
participant ConcreteModelRoute
participant RequestLog
RequestHandler->>evidenceFromBody: parsed request body
evidenceFromBody-->>RequestHandler: tool and image evidence
RequestHandler->>routeModel: model id and request evidence
routeModel->>PolicyEvaluator: profile and candidate capability evidence
PolicyEvaluator-->>routeModel: selected candidate and evaluation trace
routeModel->>ConcreteModelRoute: route selected concrete model
ConcreteModelRoute-->>RequestHandler: route result
RequestHandler->>RequestLog: route trace or 404 status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56f17f45c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 36
🤖 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-site/src/content/docs/reference/configuration/routing.md`:
- Around line 134-136: Update the CLI command summary in the routing reference
to include the complete flags from the route-policy CLI surface: add [--json] to
list and show, and add [--image], [--structured-output], and [--json] to dry-run
while retaining its existing optional flags.
- Around line 138-148: Add the routingProfiles table entry and the
policy-profile documentation section to the Japanese, Korean, Russian, and
Simplified Chinese localized routing reference pages, matching the corresponding
content and structure in the canonical routing.md page while preserving each
page’s localization.
- Around line 102-132: Update the routing configuration documentation to avoid
presenting optimize, limits.maxEstimatedCostUsd, and health/quota/cost
unknownEvidence as implemented, either labeling them as future RI-05–RI-08
behavior or removing them from the example. Preserve the supported require and
capability unknownEvidence documentation. Add the routingProfiles section to the
Japanese, Korean, Russian, and Simplified Chinese routing pages alongside their
existing combo-routing content.
In `@src/cli/index.ts`:
- Around line 1010-1021: Add subprocess regression coverage near the existing
CLI tests for the route dispatch in src/cli/index.ts. Invoke “ocx route policy
dry-run <id> ...” through the CLI entrypoint and verify the resulting request
demonstrates that handleRoutePolicyCommand receives args.slice(2); also assert
invalid route commands exit with code 2.
In `@src/cli/observe.ts`:
- Around line 17-18: Update the usage entries for rebuild-index and index-status
in the observe CLI help to use the canonical `ocx observe logs ...` spelling
consistently with the surrounding entry, without changing command dispatch
behavior.
- Around line 84-115: Route rebuildIndex and indexStatus through the management
API instead of importing history indexer functions directly. Add dedicated
management endpoints in the request-history management routes for rebuild and
status, then update both CLI functions to call them via runtimeRequest(...,
deps), preserving JSON and human-readable output while using the live proxy’s
baseUrl and fetchImpl.
In `@src/cli/route-policy.ts`:
- Around line 39-60: Extract all flags and options before calling args.shift()
in both show and dryRun. In show, move takeFlag(args, "--json") before reading
the profile id; in dryRun, move --json, --model-context, --tools, --image, and
--structured-output parsing before shifting the remaining positional id, then
retain the existing validation and request behavior.
In `@src/router.ts`:
- Around line 433-465: Add a bypassPolicy parameter to routeModelInternal and
guard the policy-resolution branch so it is skipped when that flag is set. Pass
the flag on the recursive call that routes the selected concrete candidate,
preventing self-alias and cyclic profile resolution from recursing; preserve
normal policy resolution for top-level requests. Add regression coverage in
policy-execution.test.ts for self-alias and cyclic profiles.
In `@src/routing/analytics.ts`:
- Around line 338-343: The analytics response currently exposes scanned sample
size as both totalRequests and scannedRows. Update the result construction in
the routing analytics function to remove totalRequests, or compute it from a
separate COUNT query using the same filters while retaining scannedRows for the
capped sample; ensure handleRoutingAnalyticsRoutes forwards the corrected
semantics.
- Around line 227-228: The conditional parsing in cooldownFailures calculation
skips attempt-level recovery kinds for non-success rows with status below 400.
In src/routing/analytics.ts lines 227-228, update the entry parsing used by
cooldownTriggering so every relevant row has its parsed entry and attempts
available. In tests/routing-analytics.test.ts lines 56-72, add coverage for
status 200 with terminalStatus "incomplete" and an attempt recoveryKinds
containing "rate-limit-429", asserting cooldownTriggeringFailures equals 1.
- Around line 184-195: Update the request-history query in the rows retrieval
flow to order results by timestamp descending and request_id descending,
matching the ordering used by the history indexer. Preserve the existing maxRows
+ 1 fetch and truncation logic so boundary selection is deterministic.
- Around line 230-280: In the row-processing logic, compute the request cost
estimate once and reuse it for both the global cost totals and the per-key
bucket accounting. Refactor the duplicated estimateRequestCost calls and shared
success/usage condition so serviceTierContext(entry) and pricing run only once,
while preserving the existing costTotalUsd, costCount, bucket.costUsdSum, and
bucket.costRows updates.
In `@src/routing/capability.ts`:
- Around line 25-50: Memoize the catalog rows used by cachedCatalogModels() so
each evaluation avoids repeated synchronous reads, parses, and mappings across
candidateCapabilityEvidence calls. Import statSync, track the catalog path and
modification time alongside the mapped rows, reuse the cache when both match,
and refresh catalogCache when either changes while preserving the empty-array
fallback on read or parse errors.
- Around line 52-74: Update isLocalHostname and isPrivateHostname to recognize
loopback ranges beyond 127.0.0.1, IPv4 link-local addresses, IPv6 unique-local
addresses, and IPv4-mapped local addresses, accounting for URL.hostname’s
bracketed IPv6 format. Update localRemoteEvidence so hosts that are neither
positively local nor positively remote remain unknown instead of returning
remoteAllowed.
- Around line 105-108: The tools capability inference in the capability
construction must not use provider.parallelToolCalls as evidence of tool
support. Update the tools assignment near capabilities and isNative to derive
only from the catalog/registry capability evidence, preserving native handling
if it is established local evidence, and leave tools undefined when no such
evidence exists.
In `@src/routing/evaluator.ts`:
- Around line 212-215: Update the comment above excludedByUnknown to identify
the future release that will introduce the capability score, while stating that
RI-05 still treats capability “penalize” the same as “allow” because
configuredPriorityScore is the only component. Also update the documented
penalize option in routing configuration documentation to describe this no-op if
it is user-facing there.
- Around line 243-255: Update the trace construction around
buildRouteDecisionTrace so requirements come from the selected candidate, or the
shared profile-level evaluation when applicable, rather than flattening all
candidates into one list. Replace the literal 16 with the exported
MAX_REQUIREMENTS constant, preserving candidate attribution and allowing the
trace builder to manage truncation.
In `@src/routing/history/indexer.ts`:
- Around line 181-197: Bound readCompleteTail to a fixed-size chunk and preserve
incomplete trailing lines across chunk boundaries while returning the correct
nextOffset and decoded complete lines. Update the ingest flow used by
fullRebuild/ingestSourceTail accordingly, and wrap ingestion in refreshLocked so
unexpected failures are recorded in HISTORY_META_KEYS.lastError rather than
rejecting openRequestHistoryIndex.
- Around line 124-133: Update sourceIdentityMatches to compare all persisted
file-identity fields from readIndexedMeta—sourcePath, sourceDev, sourceIno, and
sourceBirthtimeMs—against the corresponding fields on the UsageLogRevision, in
addition to sourceSize and sourceMtimeMs. Preserve the existing null-revision
handling and ensure identity fields remain unchanged during append refreshes.
- Around line 523-528: Update the history query and pagination logic around
queryRequestHistory so cursor data is carried from the indexed timestamp and
request_id columns, rather than parsing last.row_json. Build nextCursor from
those typed column values and remove the unguarded JSON.parse dependency,
preserving hydrateRow’s behavior of skipping damaged rows without allowing
pagination to return a 500.
In `@src/routing/profile.ts`:
- Around line 151-168: Update aliasIssues() to use a shared selector-conflict
check covering physical provider/model, account, combo, native-family,
configured-model, and pattern routes, so aliases cannot shadow existing
non-policy selectors such as “a/m1” or configured bare model names. Reuse the
router’s existing resolution logic where possible, preserve all current
non-policy routing behavior, and add regression coverage for both conflict
cases.
- Around line 363-381: The normalizeRoutingProfile weight calculation currently
permits an all-zero optimize map to produce normalized weights that do not sum
to one. Update validation to reject a zero total, or make
normalizeRoutingProfile replace an all-zero map with DEFAULT_PROFILE_WEIGHTS
before normalization, and add a regression test covering all four optimize
values set to zero.
In `@src/routing/request-evidence.ts`:
- Around line 12-22: Update inputContainsImage to inspect each array item's
nested content array, recursively or directly checking its parts for image type
or image fields while preserving existing top-level detection. Add a routing
regression test using a Responses-shaped body with a nested input_image part and
assert that evidenceFromBody returns imageInputRequired: true.
In `@src/routing/trace.ts`:
- Around line 277-281: In the requirements truncation branch of the trace
builder, replace the incorrect truncated.candidates assignment with a dedicated
requirements truncation flag. Update normalizeRouteDecisionTrace to preserve and
normalize this new flag alongside the existing truncation keys, while leaving
candidate truncation behavior unchanged.
- Around line 479-483: Update the evidence construction around parseCapability,
parseHealth, parseQuota, parseCost, and parseScore so each parser is invoked
once per candidate, storing its result before conditionally adding the
corresponding property. Preserve the current behavior of omitting properties
when a parsed result is falsy while reusing the stored values instead of
repeating validation and allocations.
- Around line 324-334: Update enforceByteBudget to measure serialized trace size
using the UTF-8 encoded byte length rather than JSON.stringify(...).length.
Apply this byte-based measurement to both the initial trace and the slimmed
trace checks against MAX_TRACE_BYTES, preserving the existing truncation flow
and return behavior.
In `@src/server/claude-messages.ts`:
- Around line 632-636: Add a focused Claude Messages regression test near the
existing subsystem tests, exercising a policy request whose tool or image input
is translated through anthropicToResponsesTranslation() and evidenceFromBody().
Assert that an ineligible candidate is not selected and that the persisted route
decision matches the evaluated result.
In `@src/server/management/request-history-routes.ts`:
- Around line 23-27: Update parseOptionalInt in request-history-routes.ts to
distinguish absent query values from malformed numeric values, then return 400
for invalid status, limit, from, and to inputs; apply the same behavior to from
and to in routing-analytics-routes.ts. In the request-history fallback handling,
accept only exactly true or false and reject any other supplied value with 400.
Add endpoint tests covering malformed limit, status, from, and fallback query
strings.
- Around line 92-95: Wrap the decodeURIComponent call in the GET request-history
route around requestId with URIError handling; return a 400 JSON response using
error.code "invalid_request_id" when decoding fails, while preserving the
existing 404 response for empty or slash-containing IDs. Add a regression test
covering GET /api/request-history/%.
In `@src/server/request-log.ts`:
- Around line 255-267: Update normalizeRouteDecisionTraceForLog to omit rejected
traces instead of falling back to the raw entry: return an optional/undefined
result when normalizeRouteDecisionTrace returns null, and adjust
requestLogEntryFromPersistedUsage to exclude routeDecision in that case. Add a
hydration test using a rejected trace that verifies
requestLogEntryFromPersistedUsage produces an undefined routeDecision.
In `@src/server/responses/compact.ts`:
- Line 275: Update the initial routeModel call in the compact response flow to
pass evidenceFromBody(raw), importing that helper so image evidence is available
during the first policy evaluation. Add a compact-policy test covering image
input with mixed image-capable and text-only candidates, verifying the selected
route respects provider and adapter capability contracts and does not bypass
required routing.
In `@tests/policy-execution.test.ts`:
- Around line 79-89: Update buildRouteDecisionTrace() in
src/routing/evaluator.ts to include each candidate’s evaluated capability
evidence in the persisted trace mapping, alongside provider, model, eligibility,
exclusions, and score. In tests/policy-execution.test.ts, add assertions for
capability evidence on both the selected eligible candidate and the excluded
candidate, covering the evidence used for the policy decision.
- Around line 106-116: Update NoEligiblePolicyCandidateError and the routeModel
failure path to carry evaluation.trace when no candidates are eligible, then
have the request route-error finalization persist that trace in
logCtx.routeDecision. Add an integration test confirming a failed policy request
log includes candidate exclusions and the no-eligible-candidate reason.
In `@tests/route-decision-trace.test.ts`:
- Around line 125-141: Update the test around routeModel/buildRouteDecisionTrace
so it forces the selected candidate beyond the retained slice and verifies
selected.candidateIndex remaps to the correct candidate, removing the misleading
comment if the direct path is used. Add focused regression coverage for the
requirements-overflow truncated flag and the request-log hydration path when
normalization rejects a row, using the existing subsystem test patterns.
In `@tests/routing-analytics.test.ts`:
- Around line 191-200: Add a test in the analytics endpoint suite exercising
handleManagementAPI with a GET request containing ?from=5000&to=1000, then parse
the response body and assert status 400 and error.code equals "invalid_range",
while preserving the existing success-path test.
- Around line 56-72: Add a focused regression row to the existing “classifies
success, failure, cancellation and incomplete streams” test with status 200,
terminalStatus “incomplete”, and an attempt whose recoveryKinds includes
“rate-limit-429”; update the expected cooldownTriggeringFailures count to
include this case while preserving the other 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: ASSERTIVE
Plan: Pro Plus
Run ID: c86a27a1-55bf-4178-8686-9d205dfed8d3
📒 Files selected for processing (34)
devlog/_plan/260804_router_intelligence/000_master_plan.mddevlog/_plan/260804_router_intelligence/001_pr_stack_status.mddocs-site/src/content/docs/reference/configuration/routing.mdsrc/cli/index.tssrc/cli/observe.tssrc/cli/route-policy.tssrc/config.tssrc/router.tssrc/routing/analytics.tssrc/routing/capability.tssrc/routing/evaluator.tssrc/routing/history/cursor.tssrc/routing/history/indexer.tssrc/routing/history/schema.tssrc/routing/profile.tssrc/routing/request-evidence.tssrc/routing/trace.tssrc/server/chat-completions.tssrc/server/claude-messages.tssrc/server/management-api.tssrc/server/management/request-history-routes.tssrc/server/management/routing-analytics-routes.tssrc/server/management/routing-profile-routes.tssrc/server/request-log.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/server/search.tssrc/types.tssrc/usage/log.tstests/policy-execution.test.tstests/request-history-index.test.tstests/route-decision-trace.test.tstests/routing-analytics.test.tstests/routing-profile.test.ts
Land RI-05 on current dev: explicit policy/<id> (or configured alias) requests execute the capability evaluator with canonical local evidence and dispatch to the selected candidate. Rebased from 56f17f4 onto dev (parents RI-01..04 already merged; branch carried stale pre-review copies). Conflict resolutions: evaluator keeps dev's requestRequirementFor; responses/core.ts combines dev's logCtx.routeDecision wiring with the new evidenceFromBody call; stack-status ledger follows dev.
- evidenceFromBody: recurse into input/messages content arrays so nested image parts (Responses input_image, chat image_url, Claude image) are detected on live paths; image-aware routing previously never fired for real bodies. - cachedCatalogModels: memoize catalog rows by path+mtime so policy evaluation does not re-read/parse the whole catalog per candidate on the request path. - classifyHostname: widen local/private detection (127/8, IPv6 loopback/ULA/link-local, IPv4-mapped) and keep unrecognized hosts unknown instead of asserting remoteAllowed; emit definitive localOnly/remoteAllowed booleans once classified.
…amespaces - NoEligiblePolicyCandidateError carries evaluation.trace; responses/chat/claude request paths persist it via logCtx.routeDecision so failed policy requests record candidate exclusions. - responses/compact.ts passes evidenceFromBody(raw) to the first policy evaluation (compact requests now apply tools/image requirements). - config: reserve policy/combo as provider/account-namespace names so a provider literally named policy cannot be silently shadowed by the policy/<id> branch. - evaluator: use MAX_REQUIREMENTS and correct the stale penalize comment (capability score still future, RI-06+).
Real-body evidence shapes (Responses nested input_image, chat image_url), compact-style dispatch with evidence, no-eligible trace propagation, and reserved policy/combo provider names.
Profiles now execute on explicit policy/<id> or alias requests; note the live-path evidence surface (tools/image), the capability penalize no-op until a score dimension ships (RI-06+), and the full CLI flag set.
56f17f4 to
043e654
Compare
|
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: 3
🤖 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-site/src/content/docs/reference/configuration/routing.md`:
- Around line 90-93: Add policy routing as the first step in the documented
model-resolution order, before account selectors and combo routing, matching the
behavior described for src/router.ts. Update the surrounding ordering text so
policy/<id> requests and configured profile aliases are explicitly validated and
resolved before later selectors.
In `@src/config.ts`:
- Around line 606-615: Extend the reserved account-selector namespace set used
by config validation to include "policy", matching the existing
RESERVED_PROVIDER_NAMES protection and routeModelInternal precedence. Add a
configuration-validation test proving that codexAccountNamespaces.policy is
rejected as a collision.
In `@src/server/responses/compact.ts`:
- Around line 276-279: Update the catch block surrounding routeModel in the
compact response handler to detect NoEligiblePolicyCandidateError and assign its
trace to logCtx.routeDecision before returning the existing 404 response. Follow
the handling used by other request handlers and add a compact-handler regression
test asserting the failed request log includes the policy trace.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b4ee6cf-63ad-4d36-a73f-4d20f0ca98cf
📒 Files selected for processing (11)
docs-site/src/content/docs/reference/configuration/routing.mdsrc/config.tssrc/router.tssrc/routing/capability.tssrc/routing/evaluator.tssrc/routing/request-evidence.tssrc/server/chat-completions.tssrc/server/claude-messages.tssrc/server/responses/compact.tssrc/server/responses/core.tstests/policy-execution.test.ts
A physical provider named \combo\ is a supported pattern (combo aliases hosted on the combo provider) exercised by model-visibility-management-api tests; reserving it broke config load for those setups. The policy namespace stays reserved.
…on order - compact handler catch now assigns NoEligiblePolicyCandidateError.trace to logCtx.routeDecision, matching responses/chat/claude; regression test asserts the failed compact request log carries the no-eligible trace. - routing.md model-resolution order now lists explicit policy/<id> and profile aliases as step 1, before account selectors and combos.
[GD] MergingMerging Required CI is green on |
Summary
RI-05 of the Router Intelligence / Routing Control Plane programme. Makes
policy profiles executable: an explicitly requested
policy/<id>(orconfigured alias) now routes through the capability evaluator, backed by
canonical capability evidence, and dispatches to the selected candidate.
Only explicit policy requests activate this path - every existing selector
(account namespace, provider/model, combo, native ids, default-provider)
behaves byte-for-byte as before.
Scope
src/routing/capability.ts-candidateCapabilityEvidence(): canonical,local-only evidence per
provider/model(no network at routing time):modelContextWindows/contextWindow,registry hints, cached Codex catalog row, native metadata
modelInputModalities, registry, catalog, native metadatacapabilities(supports_tools etc.), nativemodels,
parallelToolCallssupportsServiceTierbaseUrlhost classificationsrc/routing/request-evidence.ts-evidenceFromBody(): cheap proof fromthe request body (tools present, image parts present). Context size stays
unknown at routing time (documented limitation; the dry-run API/CLI is the
context-inspection surface).
src/routing/evaluator.ts- request-side requirements: a request thatprovably needs tools or images imposes
request-tools/request-image-inputrequirements on every candidate.src/router.ts-routeModel(config, modelId, policyEvidence?)executesthe policy branch (system-reserved
policy/namespace, before combos):evaluates, throws
NoEligiblePolicyCandidateErrorwhen nothing qualifies,routes the selected concrete target, and attaches the full RI-01 policy
trace (profile + revision + candidate scores) as
routeDecision./v1/responses,/v1/chat/completions,/v1/messages)pass body-derived evidence into
routeModel.tests/policy-execution.test.ts- 8 tests.Unknown is not zero
Capability dimensions without canonical evidence stay
undefined; theprofile's
unknownEvidence.capability(excludedefault,penalize,allow) decides eligibility. No dimension defaults to "supported".Privacy / security
canonical capability numbers.
bun run privacy:scanpasses.Compatibility
routeModel's optional third argument is additive; existing callers areunaffected.
Dependency
feat/ri-04-policy-profile-corehead00e1c4ae5.Non-goals
Local verification (exact)
bun x tsc --noEmit-> PASSED (0 errors)bun run test tests/policy-execution.test.ts-> 8/8 passand codex-routing)
bun run privacy:scan-> passedNotes for reviewers
routeModelreturns early when a policy trace is already attached, so thegeneric single-candidate trace builder never overwrites the evaluation.
policy/namespace is checked before account namespaces: it issystem-reserved, and profile aliases are validated against account
namespaces at config load.
Summary by CodeRabbit