feat(routing): add cost-aware policy scoring and limits - #1015
Conversation
|
Warning Review limit reached
Next review available in: 18 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 (1)
📝 WalkthroughWalkthroughThe PR adds cost evidence and cost-weighted policy routing. It supports canonical price estimation, unknown-cost policies, maximum cost limits, trace propagation, bounded trace normalization, updated scoring tests, and routing documentation. ChangesCost-aware policy routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Router
participant CostEvidence
participant PolicyEvaluator
participant DecisionTrace
Router->>CostEvidence: calculate candidate cost evidence
CostEvidence->>Router: return estimate, source, completeness, and limit status
Router->>PolicyEvaluator: evaluate candidates with cost evidence
PolicyEvaluator->>PolicyEvaluator: apply cost policy and weighted score
PolicyEvaluator->>DecisionTrace: provide selected candidate and score components
DecisionTrace->>Router: return bounded route-decision trace
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66ae44aca4
ℹ️ 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".
| return input.some(part => { | ||
| if (!part || typeof part !== "object" || Array.isArray(part)) return false; | ||
| const record = part as Record<string, unknown>; | ||
| if (record.type === "image" || record.type === "input_image") return true; | ||
| if (record.image_url !== undefined || record.image !== undefined) return true; |
There was a problem hiding this comment.
When policy routes are invoked for normal Responses/Chat/Claude turns, images are usually nested under input message items, e.g. { type: "message", content: [{ type: "input_image", ... }] }, after translation. This shallow scan only checks the top-level input elements, so imageInputRequired remains false and a profile can select a text-only candidate for an image request unless the profile itself always requires imageInput. Recurse into content blocks before routing.
Useful? React with 👍 / 👎.
| return stored.sourceSize === Number(revision.size) | ||
| && stored.sourceMtimeMs === Number(revision.mtimeMs); |
There was a problem hiding this comment.
After any new usage row is appended, currentUsageLogRevision() reports a different size/mtime, so this check returns false and ensureSchemaAndIdentity() destroys and fully rebuilds the SQLite index before the tail-ingest path can run. That makes /api/request-history, /api/routing-analytics, and policy health evidence pay an O(size of usage.jsonl) rebuild after each append on large histories; compare stable file identity here and let indexedOffset handle appended bytes.
Useful? React with 👍 / 👎.
| const policyId = resolvePolicyProfileId(config, modelId); | ||
| if (policyId) { |
There was a problem hiding this comment.
policy account selectors
Because this new policy check runs before the existing Codex account namespace branch, any existing config with codexAccountNamespaces.policy = ... now turns policy/gpt-* into a profile lookup and fails instead of selecting that account; policy was previously a valid selector because config validation only reserved combo/provider namespaces. Please either reserve/reject/migrate that namespace at config load or let account selectors win for configured namespace IDs.
AGENTS.md reference: src/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| if (cost?.estimatedUsd !== undefined && cost.limitUsd !== undefined | ||
| && cost.estimatedUsd > cost.limitUsd) { |
There was a problem hiding this comment.
When dry-run or another evaluator caller supplies cost evidence as { estimatedUsd: ... } without also duplicating limitUsd, this check skips profile.limits.maxEstimatedCostUsd, so an estimate above the configured hard ceiling remains eligible. Compare against the profile limit here (and then stamp it into the trace) instead of relying on callers to echo the limit in every candidate evidence object.
Useful? React with 👍 / 👎.
| if (requestEvidence.toolsRequired === true) { | ||
| const tools = booleanRequirement("request-tools", true, capability?.tools); | ||
| if (tools) requirements.push(tools); | ||
| } | ||
| if (requestEvidence.imageInputRequired === true) { | ||
| const image = booleanRequirement("request-image-input", true, capability?.image); | ||
| if (image) requirements.push(image); | ||
| } |
There was a problem hiding this comment.
parseEvidence() and the CLI accept request-specific constraints such as contextWindow and structuredOutputRequired, but this evaluator only turns tools/images into requirements. A dry-run with --model-context or --structured-output can therefore select a candidate that lacks the requested context window or structured-output support unless the same constraint is duplicated in the profile's static require; add requirements for the rest of PolicyRequestEvidence.
Useful? React with 👍 / 👎.
| if (hasOwnProvider(config.providers, alias)) { | ||
| issues.push({ path: ["alias"], message: `alias "${alias}" collides with configured provider name "${alias}"` }); | ||
| } |
There was a problem hiding this comment.
For aliases containing /, this validation only compares the whole alias to provider names, so anthropic/claude-... or a/m1 passes even though routeModel() checks policy aliases before explicit provider namespaces. In that configuration, requests intended for an existing provider/model selector are silently rerouted through the policy profile; reject aliases whose first segment is a configured provider namespace.
Useful? React with 👍 / 👎.
| ...(input.score ? { score: input.score } : {}), | ||
| ...(input.capability ? { capability: input.capability } : {}), | ||
| ...(input.health ? { health: input.health } : {}), | ||
| ...(input.quota ? { quota: input.quota } : {}), | ||
| ...(input.cost ? { cost: input.cost } : {}), |
There was a problem hiding this comment.
These newly attached evidence objects are copied verbatim into the decision trace, but /api/routing-profiles/dry-run accepts caller-supplied candidate evidence and provider config can contain long capability strings. In those cases MAX_TRACE_STRING and MAX_TRACE_BYTES no longer hold (and extra fields can ride along), because only provider/model/exclusion strings are capped before JSON.stringify; normalize evidence through the existing parse helpers or explicitly whitelist/cap it before storing it in the trace.
Useful? React with 👍 / 👎.
| const length = size - fromOffset; | ||
| const buf = Buffer.allocUnsafe(length); |
There was a problem hiding this comment.
On the first request-history/analytics query, or after any rebuild, this allocates one Buffer for the entire unindexed tail of usage.jsonl. A user with a large existing ledger can therefore block the Bun server or exhaust memory just by opening those management views (and policy health calls use the same indexer); read and ingest bounded chunks instead of materializing the full tail at once.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
1374-1382: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
NoEligiblePolicyCandidateErrormaps to a 404, which tells the client the model does not exist.
src/router.tsLine 470 throwsNoEligiblePolicyCandidateErrorwhen every candidate of a routing profile is excluded. The catch at Lines 1377-1382 handles onlyNoAvailableComboTargetsErrorspecially and maps everything else to404 invalid_request_error. The client receives "No eligible candidates for policy profile: X" with a 404.The exclusion causes are transient by design: a live cooldown (
src/routing/evaluator.tshealth branch), exhausted quota, or a cost limit. A 404 signals a permanent model-resolution failure, so Codex-shaped and Claude-shaped clients treat it as fatal and do not retry. The correct signal is a 503 with aRetry-Afterhint, matching howcomboUnavailableResponsehandles the equivalent combo case at Line 1379.This is reachable today:
quotaEvidenceForCandidateis called without an account reference insrc/router.tsLines 458-461, so a profile withunknownEvidence.quota: "exclude"excludes every candidate on every request.🔧 Proposed fix
} catch (err) { if (err instanceof NoAvailableComboTargetsError) { return comboUnavailableResponse(err.message); } + if (err instanceof NoEligiblePolicyCandidateError) { + // Exclusions are transient (cooldown, quota, cost limit); a 404 would be read as + // a permanent model-resolution failure and never retried. + return formatErrorResponse(503, "upstream_error", err.message, { retryAfter: "5" }); + } return formatErrorResponse(404, "invalid_request_error", err instanceof Error ? err.message : String(err)); }Import
NoEligiblePolicyCandidateErrorfrom../../routeralongside the existingrouteModelimport.🤖 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/server/responses/core.ts` around lines 1374 - 1382, Update the routeModel error handling in the response path to import and recognize NoEligiblePolicyCandidateError alongside NoAvailableComboTargetsError, then map it to the same 503 response and Retry-After behavior used by comboUnavailableResponse instead of the generic 404 invalid_request_error path.
🤖 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/cli/observe.ts`:
- Around line 17-18: Update the usage examples in the observe CLI help block to
prefix both rebuild-index and index-status commands with observe, using ocx
observe logs rebuild-index and ocx observe logs index-status while preserving
the existing command descriptions and formatting.
- Around line 84-115: Route rebuildIndex and indexStatus through the management
API instead of importing and invoking rebuildRequestHistoryIndex or
requestHistoryIndexStatus directly. Add authenticated rebuild and status
endpoints in request-history-routes.ts, then call those endpoints via
runtimeRequest using deps and preserve the existing JSON and human-readable
output formatting from the returned metadata.
In `@src/cli/route-policy.ts`:
- Around line 81-89: The new route-policy CLI subcommands lack focused
regression coverage. Add a flat Bun test under tests/ exercising
handleRoutePolicyCommand with an injected fetchImpl, covering list with
empty-profile rendering, show reporting an unknown-id error, and dry-run sending
the expected request path, method, and body.
- Around line 41-43: Reject arguments beginning with "-" immediately after
extracting the profile id in both the show flow and the dryRun flow, alongside
the existing missing-id validation. Throw CliUsageError with the existing usage
text so misplaced flags such as "--json" produce usage guidance instead of being
treated as profile IDs or sent to the API.
In `@src/config.ts`:
- Around line 1271-1276: The alias validation around routingProfileIssues must
reject slash aliases whose first segment matches a configured provider,
preventing them from overriding explicit provider/model routing; update
aliasIssues using the configured providers passed to routingProfileIssues. In
docs-site/src/content/docs/reference/configuration/routing.md lines 93-96,
retain the provider-collision claim and keep it synchronized with this
validation behavior.
In `@src/router.ts`:
- Around line 446-474: Add a recursion guard to policy-profile routing around
resolvePolicyProfileId and routeModelInternal, tracking visited policy IDs or
enforcing a maximum depth across nested profile candidates. Reject or terminate
when a candidate resolves to an already-visited policy, including candidates
expressed as policy/<id> or another profile alias, and preserve normal routing
for non-recursive candidates.
- Around line 414-431: Update the eligibility calculation in the target-mapping
function around isSelected so the selected combo target remains eligible even
when it was chosen despite cooldown; preserve exclusion annotation via
selected-despite-cooldown. Keep non-selected targets’ existing enabled,
cooldown, and already-attempted eligibility rules unchanged, and do not alter
buildRouteDecisionTrace.
- Around line 458-461: Update the quotaEvidenceForCandidate call in the live
policy scoring path to pass the candidate’s quota account context required by
src/routing/quota.ts, enabling account-specific headroom evaluation. If that
context is unavailable in this flow, disable live quota optimization instead of
producing unknown quota evidence for every candidate; preserve valid candidate
scoring and the configured unknownEvidence behavior.
In `@src/routing/analytics.ts`:
- Around line 320-325: The estimatedCostUsdPerSuccessfulRequest calculations in
the bucket and global analytics paths use bucket.costRows, which represents only
priced successful rows, so rename these fields to accurately state the
priced-request denominator (for example, estimatedCostUsdPerPricedRequest).
Update all related API documentation and locale entries, including the global
field, while preserving the existing calculation and priceCoverage reporting.
- Around line 227-280: Compute the cost estimate once per successful row with
usage and reuse it for both global and bucket accumulators. In the analytics
loop, move the byKey bucket lookup/initialization before the existing global
cost block, retain a single estimateRequestCost call, and apply its result to
costTotalUsd/costCount and bucket.costUsdSum/costRows; remove the duplicate
bucket-side calculation.
- Around line 149-153: Update parseEntry to normalize persisted analytics data
before cooldown evaluation: validate that attempts is an array, discard
malformed attempt elements, and default missing or invalid recoveryKinds to an
empty array. Ensure cooldownTriggering receives only safe normalized attempts so
malformed history rows cannot throw or reject analytics computation.
In `@src/routing/capability.ts`:
- Around line 25-50: Memoize the result of cachedCatalogModels with a
short-lived process-level TTL cache so readCatalog(readCodexCatalogPath()) and
the catalog transformation execute only when the cache is expired. Preserve the
existing empty-array fallback for read or parse failures, and ensure
candidateCapabilityEvidence continues receiving the cached model list without
moving its call out of the candidate loop.
- Around line 105-108: Update the native branch of the tools capability
calculation near capabilities and tools to call the imported
nativeParallelToolCalls helper with the native model identifier instead of
hardcoding true for every native model. Preserve the existing provider
parallelToolCalls fallback for non-native models and retain the final undefined
behavior when no canonical evidence is available.
- Around line 121-134: Update the encryptedCodexTasks derivation around
isCanonicalOpenAiForwardProvider to return an omitted capability when provider
configuration is absent, rather than emitting false for an unknown provider. For
configured providers, mirror the router’s authMode backfill by treating an
omitted authMode as "forward" before performing the canonical check, while
preserving the existing canonical-provider conditions and output shape.
- Around line 59-70: Expand isPrivateHostname to classify link-local
169.254.0.0/16, CGNAT 100.64.0.0/10, and IPv6 unique-local fc00::/7 hostnames as
private, while preserving the existing private-range checks. Keep
localRemoteEvidence unchanged so these hosts produce localOnly evidence instead
of remoteAllowed.
In `@src/routing/cost.ts`:
- Around line 36-74: Update src/routing/cost.ts:36-74 and the policy route path
around costEvidenceForCandidate to accept and pass a normalized request-side
usage estimate before evaluating candidates, while preserving incomplete/unknown
results when no estimate exists and ensuring they are not presented as usage.
Update docs-site/src/content/docs/reference/configuration/routing.md:102-105 to
state that cost remains unknown without an estimate until runtime enforcement is
implemented.
- Around line 81-86: Update costScore to treat an estimatedUsd value of 0 as
valid evidence: check only for undefined before applying Number.isFinite, then
preserve the existing reference selection and score calculation.
In `@src/routing/evaluator.ts`:
- Around line 286-304: The evaluator must not silently treat
profile.optimize.latency as configured declaration priority. In
src/routing/evaluator.ts lines 286-304, either add deterministic latency
evidence and a latency component to the RouteScoreEvidence calculation, or
remove/reject the latency option; in
docs-site/src/content/docs/reference/configuration/routing.md line 103, document
only optimization weights supported by the current evaluator, with no direct
change required there if latency is implemented.
In `@src/routing/health.ts`:
- Around line 107-169: Update healthEvidenceForCandidate to cache the computed
aggregate by provider|model|accountRef with a short TTL, reusing valid cached
evidence before calling openRequestHistoryIndexSync and the synchronous query;
ensure the cache is keyed per candidate context and expires promptly. Replace
the silent catch with a rate-limited warn-level log that includes the caught
error and preserves the existing unknown-evidence fallback.
- Around line 116-121: Replace the unsafe HealthSample[] cast on the query
result with boundary validation that accepts only rows having finite numeric
status and durationMs values, preserving timestamp and closeReason fields as
appropriate. Store the validated samples, iterate those instead of raw rows in
the health calculation, and only add finite durationMs values to latencies so
classifySample and healthScore never receive NULL-derived values.
In `@src/routing/history/indexer.ts`:
- Around line 181-197: Update readCompleteTail and its ingestion flow to read
the unread ledger range in bounded chunks (for example, 4 MiB), rather than
allocating the entire range at once. Preserve partial lines between chunks,
ingest each completed newline-delimited portion incrementally through
ingestText, and ensure the final nextOffset reflects only fully consumed lines
while retaining incomplete trailing data for the next read.
- Around line 295-318: Update destroyAndRecreate to remove the SQLite WAL and
SHM sidecars alongside path before constructing the fresh Database. Apply the
existing retry behavior to all three files—path, path + "-wal", and path +
"-shm"—while preserving the current recreation and schema initialization flow.
- Around line 128-133: Update sourceIdentityMatches to compare source_path,
source_dev, source_ino, and source_birthtime_ms from
recordSourceMeta/sourceIdentity instead of source_size and source_mtime_ms,
while preserving the null-revision behavior and separate truncation handling.
Ensure ingestSourceTail refreshes identity metadata after successful tail
ingestion. Add a regression test in the request-history index tests that appends
one row, verifies indexedRows increases, and confirms lastError remains
unchanged.
In `@src/routing/history/schema.ts`:
- Around line 70-72: Update indexDbPath() in the indexer to return
historyIndexPath(dir) instead of constructing the database path inline with
HISTORY_DB_FILENAME. Reuse the existing helper’s normalization and single path
definition.
In `@src/routing/profile.ts`:
- Around line 154-164: Update the provider collision check in the alias
validation block to compare the first segment of a slashed alias against
configured providers, matching the existing codex namespace check. Preserve
whole-alias validation for unslashed aliases, and continue reporting the
provider collision through the existing issues entry.
In `@src/routing/quota.ts`:
- Around line 57-71: Unify quota exhaustion handling across the Anthropic and
Codex branches by extracting the shared threshold and headroom calculation into
a named helper near the quota logic. Update the Anthropic branch to use the
shared `EXHAUSTED_USAGE_PERCENT` threshold instead of hardcoded `100`, and
spread the helper result for `headroom` in both branches while preserving
existing behavior when no percentage is available.
In `@src/routing/request-evidence.ts`:
- Around line 12-22: Update inputContainsImage to inspect each message item's
nested content array in addition to any directly represented image fields.
Recursively or explicitly scan the content parts for type "image"/"input_image"
or image_url/image markers, while preserving false results for non-array and
unrelated inputs, so Responses-shaped bodies trigger imageInputRequired.
- Around line 24-33: Update evidenceFromBody to extract the top-level
reasoningEffort and serviceTier scalar values from the request body and include
them in the returned PolicyRequestEvidence, preserving the existing tools and
image detection. Use the PolicyRequestEvidence fields consumed by
evaluatePolicyProfile/requestRequirements and follow the corresponding
parsed.options value handling in responses core.
In `@src/routing/trace.ts`:
- Around line 335-336: Update the serialized trace size checks in the
surrounding trace serialization logic to measure UTF-8 bytes with
Buffer.byteLength(serialized, "utf-8"), replacing every serialized.length
comparison at the initial and subsequent checks. Keep MAX_TRACE_BYTES as the
threshold and preserve the existing truncation behavior.
- Around line 287-291: Update the requirement truncation path around
buildRequirement so it sets a requirements-specific flag instead of
truncated.candidates. Extend RouteDecisionTraceV1["truncated"] with that flag
and ensure normalizeRouteDecisionTrace initializes or preserves it consistently.
- Around line 490-494: Update normalizeRouteDecisionTrace to invoke each
evidence parser only once per candidate by storing each result in a local value
and reusing it for both the spread condition and property value. Apply this to
parseCapability, parseHealth, parseQuota, parseCost, and parseScore while
preserving the current omission of falsy results.
- Around line 345-349: Update the final candidate truncation in the
trace-building flow to always retain the selected candidate and remap
selected.candidateIndex to its new position after slicing, preserving valid
normalization for high indices. Add a regression test near the route-decision
trace tests that builds an oversized trace with a high candidateIndex and
asserts normalizeRouteDecisionTrace(buildRouteDecisionTrace(...)) is non-null.
In `@src/server/chat-completions.ts`:
- Around line 114-122: Route each request only once and pass the resolved route
into the replay path. In src/server/chat-completions.ts lines 114-122, reuse the
route result used for the pre-flight adapter decisions and preserve its
routeDecision in logCtx instead of allowing handleResponses to call routeModel
again. In src/server/claude-messages.ts lines 632-636, pass the same route into
the replay at line 719, including when replay configuration is built with
buildClaudeReplayConfig(config), so replay uses the route that shaped
sampling-parameter stripping.
In `@src/server/management/request-history-routes.ts`:
- Around line 92-96: Update the request-id parsing in the GET branch of the
request-history route to handle decodeURIComponent failures defensively. Treat
any malformed percent-escape as an unknown request and return the existing 404
response, while preserving the current empty-id and slash validation for
successfully decoded IDs.
In `@src/server/management/routing-analytics-routes.ts`:
- Around line 28-36: Update the route handler around computeRoutingAnalytics to
limit repeated scan-and-aggregate work: either pass an operator-tunable maxRows
value through the existing options argument, or add a short-lived cache keyed by
the provider, model, profileId, surface, from, and to filter tuple. Preserve the
existing response contract and JSON response behavior.
- Around line 12-16: Move parseOptionalInt from request-history-routes.ts into
shared.ts and export it, then remove the duplicate definition from
routing-analytics-routes.ts and import the shared helper in both route modules.
Preserve the existing validation and invalid_range behavior for the from and to
query parameters at routing-analytics-routes.ts:12-16 and
request-history-routes.ts:23-27.
In `@src/server/management/routing-profile-routes.ts`:
- Around line 31-32: Update parseEvidence to treat an absent or undefined raw
value as valid empty evidence, while continuing to return ok: false for present
non-object values. Preserve the existing invalid_evidence response for malformed
evidence and the normal evaluation flow using the resulting empty
PolicyRequestEvidence.
- Around line 58-64: Replace the structural casts in the evidence construction
near evaluatePolicyProfile with boundary validation for the numeric fields used
by scoring, especially cost.estimatedUsd and cost.limitUsd, and validate every
numeric scoring field in capability, health, quota, and cost as applicable.
Reject or omit malformed evidence rather than passing objects containing
non-numeric values into the returned candidates, while preserving valid evidence
and the existing trace normalization flow.
In `@src/server/request-log.ts`:
- Around line 255-267: Update requestLogEntryFromPersistedUsage to normalize
entry.routeDecision into a local nullable value and spread routeDecision only
when normalization succeeds; rejected persisted traces must be omitted rather
than falling back to the raw entry. Remove the now-unused
normalizeRouteDecisionTraceForLog helper while leaving other DTO fields
unchanged.
In `@src/types.ts`:
- Around line 829-837: The structuredOutput requirement is unsupported because
candidateCapabilityEvidence does not provide corresponding evidence. In
src/types.ts lines 829-837, either derive it from supported adapter or catalog
capability evidence or reject the requirement until such evidence exists; do not
infer it from model metadata. In
docs-site/src/content/docs/reference/configuration/routing.md lines 107-109,
remove or revise structuredOutput documentation so it is not presented as
supported until runtime evidence exists.
In `@tests/cost-scoring.test.ts`:
- Around line 105-133: Add focused regression coverage for the unknown-evidence
allow policy: in tests/cost-scoring.test.ts:105-133, configure
unknownEvidence.cost as "allow" with missing cost data and assert the candidate
is eligible, selected, and scored by priority without a cost penalty; in
tests/quota-scoring.test.ts:84-112, configure unknownEvidence.quota as "allow"
with unknown quota data and assert eligibility with no quota penalty. Use the
existing evaluatePolicyProfile test patterns and symbols.
In `@tests/health-scoring.test.ts`:
- Around line 216-221: Update the test “execution path includes health evidence
and can exclude on cooldown” to create live cooldown state for an OpenAI
candidate with a Codex account identifier, alongside an eligible fallback
candidate in the policy. Assert that routing selects the fallback and that the
route trace records a “cooldown” exclusion, ensuring routeModel preserves
execution-time cooldown evidence.
In `@tests/route-decision-trace.test.ts`:
- Around line 242-267: Update normalizeRouteDecisionTrace() to retain each
candidate’s original index while filtering and remap selected.candidateIndex
only when the selected raw candidate survives normalization; reject the trace
when it does not. Preserve the normalized candidate list and selected metadata,
and add a focused test in the existing normalizer tests covering an invalid
prefix candidate with a valid selected candidate.
- Around line 125-141: Update the test “combo candidates are capped and the
selected candidate survives truncation” to force failover selection of a target
whose original index is at least MAX_TRACE_CANDIDATES, rather than index 0.
Assert the retained candidate has the selected provider and model, and verify
selected.candidateIndex points to that retained candidate while preserving the
cap assertion and truncated.candidates flag.
In `@tests/routing-profile.test.ts`:
- Around line 112-137: Extend the alias collision validation test around
routingProfileIssues to include an alias of "work", matching the reserved
codexAccountNamespaces.work entry in the test configuration, and assert that the
returned issue reports the alias as reserved. Keep the existing provider, combo,
native-family, and sibling-profile collision cases unchanged.
---
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 1374-1382: Update the routeModel error handling in the response
path to import and recognize NoEligiblePolicyCandidateError alongside
NoAvailableComboTargetsError, then map it to the same 503 response and
Retry-After behavior used by comboUnavailableResponse instead of the generic 404
invalid_request_error path.
🪄 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: 63d2e91f-b2b1-4cc9-bb5c-041918b7c2b8
📒 Files selected for processing (40)
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/cost.tssrc/routing/evaluator.tssrc/routing/health.tssrc/routing/history/cursor.tssrc/routing/history/indexer.tssrc/routing/history/schema.tssrc/routing/profile.tssrc/routing/quota.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/cost-scoring.test.tstests/health-scoring.test.tstests/policy-execution.test.tstests/quota-scoring.test.tstests/request-history-index.test.tstests/route-decision-trace.test.tstests/routing-analytics.test.tstests/routing-profile.test.ts
| ocx logs rebuild-index | ||
| ocx logs index-status |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm that "logs" is a top-level CLI alias forwarding to handleObserveCommand.
set -euo pipefail
rg -n -C 6 'handleObserveCommand' src/cli/index.ts
rg -n -C 3 '"logs"' src/cli/index.tsRepository: lidge-jun/opencodex
Length of output: 1157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- observe usage and dispatch ---'
sed -n '1,45p;120,160p' src/cli/observe.ts
printf '%s\n' '--- command dispatch context ---'
sed -n '1005,1045p' src/cli/index.ts
printf '%s\n' '--- exact usage references ---'
rg -n -C 3 'rebuild-index|index-status|rejectArgs|CliUsageError' src/cli/observe.ts src/cli/index.ts
printf '%s\n' '--- read-only command-shape verifier ---'
python3 - <<'PY'
from pathlib import Path
text = Path("src/cli/index.ts").read_text()
observe = Path("src/cli/observe.ts").read_text()
assert 'case "logs":' in text
assert 'handleObserveCommand([command, ...args.slice(1)])' in text
assert 'sub === "logs"' in observe
print("top-level logs alias: forwards as handleObserveCommand([\"logs\", ...args])")
print("observe logs subcommand: dispatched by handleObserveCommand")
print("Both command shapes reach the logs branch.")
PYRepository: lidge-jun/opencodex
Length of output: 10198
Use the ocx observe prefix for the rebuild commands. Both spellings work, but ocx observe logs rebuild-index and ocx observe logs index-status keep this usage block consistent.
🤖 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/cli/observe.ts` around lines 17 - 18, Update the usage examples in the
observe CLI help block to prefix both rebuild-index and index-status commands
with observe, using ocx observe logs rebuild-index and ocx observe logs
index-status while preserving the existing command descriptions and formatting.
| async function rebuildIndex(argv: string[], deps: RuntimeApiDeps): Promise<void> { | ||
| const args = [...argv]; | ||
| const wantsJson = takeFlag(args, "--json"); | ||
| rejectArgs(args, USAGE); | ||
| const { rebuildRequestHistoryIndex } = await import("../routing/history/indexer"); | ||
| const meta = await rebuildRequestHistoryIndex(); | ||
| if (wantsJson) printData(meta, true); | ||
| else { | ||
| console.log(`Request-history index rebuilt (${meta.dbPath})`); | ||
| console.log(` schema version: ${meta.schemaVersion}`); | ||
| console.log(` indexed rows: ${meta.indexedRows}`); | ||
| console.log(` source size: ${meta.sourceSize} bytes`); | ||
| console.log(` last error: ${meta.lastError ?? "none"}`); | ||
| } | ||
| } | ||
|
|
||
| async function indexStatus(argv: string[], deps: RuntimeApiDeps): Promise<void> { | ||
| const args = [...argv]; | ||
| const wantsJson = takeFlag(args, "--json"); | ||
| rejectArgs(args, USAGE); | ||
| const { requestHistoryIndexStatus } = await import("../routing/history/indexer"); | ||
| const meta = await requestHistoryIndexStatus(); | ||
| if (wantsJson) printData(meta, true); | ||
| else { | ||
| console.log(`Request-history index (${meta.dbPath})`); | ||
| console.log(` schema version: ${meta.schemaVersion}`); | ||
| console.log(` indexed rows: ${meta.indexedRows}`); | ||
| console.log(` source size: ${meta.sourceSize} bytes`); | ||
| console.log(` indexed offset: ${meta.indexedOffset} bytes`); | ||
| console.log(` last error: ${meta.lastError ?? "none"}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether a management route already exposes request-history index rebuild/status,
# and whether the CLI has an offline mode contract for index commands.
set -euo pipefail
rg -n -C 4 'rebuildRequestHistoryIndex|requestHistoryIndexStatus' --type=ts
fd -t f -e ts . src/server/management --exec rg -n 'api/request-history|index-status|rebuild' {}Repository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files 'src/cli/observe.ts' 'src/routing/history/indexer.ts' 'src/server/management/**' 'src/**' | sed -n '1,160p'
printf '%s\n' '--- observe structure and relevant lines ---'
ast-grep outline src/cli/observe.ts
sed -n '1,190p' src/cli/observe.ts
printf '%s\n' '--- indexer structure and relevant lines ---'
ast-grep outline src/routing/history/indexer.ts
sed -n '1,125p' src/routing/history/indexer.ts
sed -n '270,335p' src/routing/history/indexer.ts
printf '%s\n' '--- management and routing references ---'
rg -n -C 5 'runtimeRequest|simple\("/api/storage"|request-history|history.*index|index.*history|rebuild-index|index-status' src --type=tsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- indexer lifecycle and public methods ---'
sed -n '240,470p' src/routing/history/indexer.ts
printf '%s\n' '--- request-history management route ---'
sed -n '1,150p' src/server/management/request-history-routes.ts
printf '%s\n' '--- management dispatcher and context ---'
sed -n '1,115p' src/server/management-api.ts
sed -n '1,120p' src/server/management/context.ts
printf '%s\n' '--- all management route paths near request-history ---'
rg -n 'handle[A-Za-z]+Routes|url\.pathname|req\.method' src/server/management-api.ts src/server/management --type=ts | rg 'request|history|index|handle|pathname|method' | sed -n '1,220p'
printf '%s\n' '--- runtime request behavior ---'
sed -n '1,115p' src/cli/runtime-api.ts
printf '%s\n' '--- indexer call sites ---'
rg -n -C 3 'openRequestHistoryIndex|rebuildRequestHistoryIndex|requestHistoryIndexStatus|requestHistoryDb|queryRequestHistory|requestHistoryRowById' src --type=tsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- indexer query/status implementations ---'
sed -n '445,570p' src/routing/history/indexer.ts
printf '%s\n' '--- management dispatch and auth boundary ---'
sed -n '120,225p' src/server/management-api.ts
printf '%s\n' '--- history-related tests and documentation ---'
rg -n -C 4 'rebuild-index|index-status|rebuildRequestHistoryIndex|requestHistoryIndexStatus|offline|Proxy is not running|request-history' test tests src README.md docs devlog 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- database package/runtime assumptions ---'
rg -n -C 3 'bun:sqlite|journal_mode = WAL|busy_timeout|routing-history.sqlite|HISTORY_DB_FILENAME' src package.json bun.lock 2>/dev/null | sed -n '1,220p'Repository: lidge-jun/opencodex
Length of output: 32344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
observe = Path("src/cli/observe.ts").read_text()
indexer = Path("src/routing/history/indexer.ts").read_text()
history_routes = Path("src/server/management/request-history-routes.ts").read_text()
adr = Path("docs/adr/0007-headless-cli-parity.md").read_text()
checks = {
"CLI imports rebuild implementation": 'import("../routing/history/indexer")' in observe and "rebuildRequestHistoryIndex" in observe,
"CLI imports status implementation": 'import("../routing/history/indexer")' in observe and "requestHistoryIndexStatus" in observe,
"CLI handlers accept deps": bool(re.search(r"function rebuildIndex\(argv: string\[], deps: RuntimeApiDeps\)", observe))
and bool(re.search(r"function indexStatus\(argv: string\[], deps: RuntimeApiDeps\)", observe)),
"CLI handlers do not pass deps to runtimeRequest": not re.search(
r"(rebuildIndex|indexStatus)[\s\S]*?runtimeRequest\(", observe
),
"server query opens shared index": "await openRequestHistoryIndex()" in indexer,
"manual rebuild mutates shared index": "fullRebuild(handle, \"manual rebuild requested\")" in indexer,
"rebuild can unlink database": "unlinkSync(path)" in indexer and "destroyAndRecreate" in indexer,
"request-history routes are read-only": "req.method === \"GET\"" in history_routes
and not re.search(r"req\.method === \"(POST|PUT|PATCH|DELETE)\"", history_routes),
"ADR requires live management commands": "Live management commands require a running proxy" in adr,
"ADR defines separate offline path": "offline config inspection/import remains available" in adr,
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: lidge-jun/opencodex
Length of output: 564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/cli/observe.ts").read_text()
def body(name):
match = re.search(
rf"async function {name}\([^)]*\)\s*:\s*Promise<void>\s*\{{",
source,
)
if not match:
raise SystemExit(f"missing {name}")
start = match.end()
depth = 1
i = start
while i < len(source) and depth:
if source[i] == "{":
depth += 1
elif source[i] == "}":
depth -= 1
i += 1
return source[start:i-1]
for name in ("rebuildIndex", "indexStatus"):
text = body(name)
print(f"{name}:")
print(f" deps references in body: {len(re.findall(r'\\bdeps\\b', text))}")
print(f" runtimeRequest calls in body: {len(re.findall(r'\\bruntimeRequest\\b', text))}")
print(f" direct index import: {'../routing/history/indexer' in text}")
assert all("deps" not in body(name) for name in ("rebuildIndex", "indexStatus"))
assert all("runtimeRequest" not in body(name) for name in ("rebuildIndex", "indexStatus"))Repository: lidge-jun/opencodex
Length of output: 495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/cli/observe.ts").read_text()
def body(name):
match = re.search(
rf"async function {name}\([^)]*\)\s*:\s*Promise<void>\s*\{{",
source,
)
if not match:
raise SystemExit(f"missing {name}")
start = match.end()
depth = 1
i = start
while i < len(source) and depth:
if source[i] == "{":
depth += 1
elif source[i] == "}":
depth -= 1
i += 1
return source[start:i-1]
deps_pattern = re.compile(r"\bdeps\b")
request_pattern = re.compile(r"\bruntimeRequest\b")
for name in ("rebuildIndex", "indexStatus"):
text = body(name)
deps_count = len(deps_pattern.findall(text))
request_count = len(request_pattern.findall(text))
print(f"{name}:")
print(f" deps references in body: {deps_count}")
print(f" runtimeRequest calls in body: {request_count}")
print(f" direct index import: {'../routing/history/indexer' in text}")
assert deps_count == 0
assert request_count == 0
assert "../routing/history/indexer" in text
PYRepository: lidge-jun/opencodex
Length of output: 366
Route index commands through the management API.
src/cli/observe.ts:84-105 opens and mutates routing-history.sqlite in a separate CLI process. The daemon retains its module-level index handle through src/routing/history/indexer.ts. Concurrent rebuilds can invalidate the daemon’s handle, and recovery can unlink the database while the daemon still references the deleted inode. The CLI can then report success while the daemon serves stale index data.
Add authenticated rebuild and status routes to src/server/management/request-history-routes.ts. Call them through runtimeRequest with deps.
🤖 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/cli/observe.ts` around lines 84 - 115, Route rebuildIndex and indexStatus
through the management API instead of importing and invoking
rebuildRequestHistoryIndex or requestHistoryIndexStatus directly. Add
authenticated rebuild and status endpoints in request-history-routes.ts, then
call those endpoints via runtimeRequest using deps and preserve the existing
JSON and human-readable output formatting from the returned metadata.
Source: Path instructions
| const id = args.shift(); | ||
| const wantsJson = takeFlag(args, "--json"); | ||
| if (!id) throw new CliUsageError("profile id is required", USAGE); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A flag written before the profile id is consumed as the id and produces a misleading error.
Line 41 shifts the first argument into id before line 42 scans for --json. For ocx route policy show --json my-profile, id becomes "--json", wantsJson stays false, and line 47 reports unknown routing profile: --json. The message blames a missing profile when the real fault is flag placement.
dryRun has the identical ordering at lines 53-55, and there the cost is higher: if (!id) on line 55 passes because "--json" is a non-empty string, so the command posts profile: "--json" to /api/routing-profiles/dry-run and waits for the server to answer 404.
The documented order in USAGE (lines 13-16) does place the flag after the id, so this is not a contract break. It is a diagnosability defect. Reject an id that begins with - and show USAGE instead.
🐛 Proposed fix: detect a misplaced flag in the id position
async function show(argv: string[], deps: RuntimeApiDeps): Promise<void> {
const args = [...argv];
const id = args.shift();
const wantsJson = takeFlag(args, "--json");
if (!id) throw new CliUsageError("profile id is required", USAGE);
+ if (id.startsWith("-")) throw new CliUsageError(`profile id must come before options, got "${id}"`, USAGE);
rejectArgs(args, USAGE);Apply the same guard in dryRun after line 55.
🤖 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/cli/route-policy.ts` around lines 41 - 43, Reject arguments beginning
with "-" immediately after extracting the profile id in both the show flow and
the dryRun flow, alongside the existing missing-id validation. Throw
CliUsageError with the existing usage text so misplaced flags such as "--json"
produce usage guidance instead of being treated as profile IDs or sent to the
API.
| export async function handleRoutePolicyCommand(argv: string[], deps: RuntimeApiDeps = {}): Promise<number> { | ||
| return runCliAction(async () => { | ||
| const [sub, ...rest] = argv; | ||
| if (sub === "list") await list(rest, deps); | ||
| else if (sub === "show") await show(rest, deps); | ||
| else if (sub === "dry-run") await dryRun(rest, deps); | ||
| else throw new CliUsageError(`unknown route policy command: ${sub ?? ""}`, USAGE); | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a focused regression test for the new CLI subcommands.
src/cli/route-policy.ts adds three user-facing subcommands with their own argument parsing, and the routing/persistence validation layer of this stack lists no test file for it. The listed tests — tests/routing-profile.test.ts, tests/request-history-index.test.ts, and tests/routing-analytics.test.ts — exercise handleManagementAPI, so the server routes are covered and the CLI is not.
The command is straightforward to test without a running proxy: runtimeRequest accepts an injectable fetchImpl through RuntimeApiDeps (src/cli/runtime-api.ts lines 61-88), so handleRoutePolicyCommand(argv, { fetchImpl }) can assert the request path, method, and body against a stub. Cover at least the list empty-profile rendering, the show unknown-id error, and the dry-run request body built on lines 66-74.
I can draft that test file. Do you want me to open an issue to track it?
As per path instructions: "Tests are flat Bun tests under tests/. A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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/cli/route-policy.ts` around lines 81 - 89, The new route-policy CLI
subcommands lack focused regression coverage. Add a flat Bun test under tests/
exercising handleRoutePolicyCommand with an injected fetchImpl, covering list
with empty-profile rendering, show reporting an unknown-id error, and dry-run
sending the expected request path, method, and body.
Source: Path instructions
| for (const issue of routingProfileIssues(id, raw, { | ||
| providers: config.providers, | ||
| combos: combos as Record<string, import("./types").OcxComboConfig> | undefined, | ||
| routingProfiles: routingProfiles as Record<string, import("./types").OcxRoutingProfileConfig>, | ||
| codexAccountNamespaces: accountNamespaces, | ||
| }, { excludeProfileId: id })) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject aliases in configured provider namespaces.
routingProfileIssues accepts anthropic/foo because it tests the complete alias against provider names. routeModelInternal resolves policy aliases before explicit <provider>/<model> routes. This alias therefore changes anthropic/foo from a direct provider request into policy routing.
src/config.ts#L1271-L1276: updatealiasIssuesto reject a slash alias when its first segment names a configured provider.docs-site/src/content/docs/reference/configuration/routing.md#L93-L96: retain the provider-collision claim only after validation rejects these aliases.
As per path instructions, user-facing docs must stay in sync with actual CLI/API behavior.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 2 files
src/config.ts#L1271-L1276(this comment)docs-site/src/content/docs/reference/configuration/routing.md#L93-L96
🤖 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/config.ts` around lines 1271 - 1276, The alias validation around
routingProfileIssues must reject slash aliases whose first segment matches a
configured provider, preventing them from overriding explicit provider/model
routing; update aliasIssues using the configured providers passed to
routingProfileIssues. In
docs-site/src/content/docs/reference/configuration/routing.md lines 93-96,
retain the provider-collision claim and keep it synchronized with this
validation behavior.
Source: Path instructions
| test("execution path includes health evidence and can exclude on cooldown", async () => { | ||
| for (let index = 0; index < 5; index++) appendUsageEntry(row(`ok-${index}`, 200, 800)); | ||
| const route = routeModel(config(), "policy/healthy"); | ||
| expect(route.routeKind).toBe("policy"); | ||
| expect(route.routeDecision!.candidates[0]!.health).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the execution-time cooldown exclusion.
This test does not create a cooldown. It routes provider a, which has no Codex account identifier. src/routing/health.ts, Lines 92-172, only reads live cooldown state for an OpenAI candidate with a Codex account identifier.
A regression that drops cooldown evidence in routeModel() will still pass. Create a policy with a cooled-down OpenAI account and an eligible fallback candidate. Assert that the fallback is selected and that the trace records the cooldown exclusion.
As per path instructions, a routing behavior change requires focused regression coverage.
🤖 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/health-scoring.test.ts` around lines 216 - 221, Update the test
“execution path includes health evidence and can exclude on cooldown” to create
live cooldown state for an OpenAI candidate with a Codex account identifier,
alongside an eligible fallback candidate in the policy. Assert that routing
selects the fallback and that the route trace records a “cooldown” exclusion,
ensuring routeModel preserves execution-time cooldown evidence.
Source: Path instructions
| test("combo candidates are capped and the selected candidate survives truncation", () => { | ||
| const targets = Array.from({ length: 12 }, (_, index) => ({ | ||
| provider: index % 2 === 0 ? "a" : "b", | ||
| model: `m${index}`, | ||
| })); | ||
| const config = baseConfig({ combos: { big: { strategy: "failover", targets } } }); | ||
| // Failover picks the first eligible target (index 0), so to exercise the | ||
| // selected-past-the-slice path we must force an early exclusion. | ||
| const route = routeModel(config, "combo/big"); | ||
| const trace = route.routeDecision!; | ||
| expect(trace.candidates.length).toBeLessThanOrEqual(MAX_TRACE_CANDIDATES); | ||
| expect(trace.truncated?.candidates).toBe(true); | ||
| expect(trace.candidates[trace.selected.candidateIndex]).toMatchObject({ | ||
| provider: "a", | ||
| model: "m0", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the selected-after-cap path.
The failover route selects original candidate index 0. This test does not exercise preservation of a selected candidate beyond MAX_TRACE_CANDIDATES.
Build a trace where the selected candidate has an original index at or beyond the cap. Assert that the retained candidate matches the selected provider and model and that selected.candidateIndex points to that retained candidate.
As per path instructions, a routing trace change requires focused regression coverage.
🤖 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/route-decision-trace.test.ts` around lines 125 - 141, Update the test
“combo candidates are capped and the selected candidate survives truncation” to
force failover selection of a target whose original index is at least
MAX_TRACE_CANDIDATES, rather than index 0. Assert the retained candidate has the
selected provider and model, and verify selected.candidateIndex points to that
retained candidate while preserving the cap assertion and truncated.candidates
flag.
Source: Path instructions
| test("normalizer bounds hand-edited oversized rows", () => { | ||
| const raw = { | ||
| version: 1, | ||
| decisionId: "abc", | ||
| createdAt: 1, | ||
| requestedModel: "y".repeat(400), | ||
| routeKind: "native", | ||
| requirements: [], | ||
| candidates: [{ | ||
| provider: "a", | ||
| model: "m1", | ||
| eligible: true, | ||
| exclusions: [], | ||
| capability: { contextWindow: 100, tools: true, junk: "drop-me" }, | ||
| health: { sampleCount: 3, junkField: true }, | ||
| score: { total: 1, components: { health: 1 } }, | ||
| }], | ||
| selected: { candidateIndex: 0, provider: "a", model: "m1", reason: "r" }, | ||
| }; | ||
| const trace = normalizeRouteDecisionTrace(raw)!; | ||
| expect(trace.requestedModel.length).toBe(MAX_TRACE_STRING); | ||
| expect(trace.candidates[0]!.capability).toEqual({ contextWindow: 100, tools: true }); | ||
| expect(trace.candidates[0]!.health).toEqual({ sampleCount: 3 }); | ||
| expect(trace.candidates[0]!.score).toEqual({ total: 1, components: { health: 1 } }); | ||
| expect(JSON.stringify(trace)).not.toContain("drop-me"); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject or remap a selected index after candidate filtering.
normalizeRouteDecisionTrace() in src/routing/trace.ts, Lines 503-580, filters invalid candidates before it validates selected.candidateIndex. If raw candidate 0 is invalid and raw candidate 1 is valid, a raw selected index of 0 can select the wrong retained candidate. A raw selected index of 1 instead drops the whole trace.
Retain original candidate indexes and remap the selected candidate only when that candidate survives normalization. Otherwise, reject the trace. Add an invalid-prefix-candidate test here.
As per path instructions, trace persistence behavior requires focused regression coverage.
🤖 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/route-decision-trace.test.ts` around lines 242 - 267, Update
normalizeRouteDecisionTrace() to retain each candidate’s original index while
filtering and remap selected.candidateIndex only when the selected raw candidate
survives normalization; reject the trace when it does not. Preserve the
normalized candidate list and selected metadata, and add a focused test in the
existing normalizer tests covering an invalid prefix candidate with a valid
selected candidate.
Source: Path instructions
| const isSelected = index === combo.targetIndex; | ||
| // The pick's `attempted` list includes the winner itself; only non-selected | ||
| // targets can be "already-attempted" (fallback picks exclude earlier tries). | ||
| const alreadyAttempted = !isSelected && combo.attempted.includes(key); | ||
| const exclusions: TraceCandidateInput["exclusions"] = []; | ||
| if (!configured) exclusions.push({ code: "unconfigured" }); | ||
| if (configured && !enabled) exclusions.push({ code: "disabled" }); | ||
| if (inCooldown) exclusions.push({ code: "cooldown" }); | ||
| if (isSelected && inCooldown) exclusions.push({ code: "selected-despite-cooldown" }); | ||
| if (!isSelected && alreadyAttempted && exclusions.length === 0) { | ||
| exclusions.push({ code: "already-attempted" }); | ||
| } | ||
| if (!isSelected && exclusions.length === 0) exclusions.push({ code: "not-selected" }); | ||
| return { | ||
| provider: target.provider, | ||
| model: target.model, | ||
| eligible: enabled && !inCooldown && !alreadyAttempted, | ||
| exclusions, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The selected combo target can be recorded as eligible: false, which contradicts the decision it describes.
Line 430 computes eligible: enabled && !inCooldown && !alreadyAttempted for every target, including the one the router already picked. When the pick was made despite a cooldown, the trace records the selected candidate with eligible: false. buildRouteDecisionTrace in src/routing/trace.ts Lines 256-331 keeps that candidate as the selected index without reconciling the flag, so a trace consumer reads a decision that selected an ineligible candidate.
The comment at Lines 398-400 states this function is purely observational and never re-selects. Line 422 already emits a dedicated selected-despite-cooldown code for exactly this case, which indicates the intent was to annotate the reason, not to invalidate the pick.
🔧 Proposed fix
return {
provider: target.provider,
model: target.model,
- eligible: enabled && !inCooldown && !alreadyAttempted,
+ // The selected target is eligible by definition: the pick already happened.
+ // Its cooldown is annotated via the `selected-despite-cooldown` exclusion above.
+ eligible: isSelected || (enabled && !inCooldown && !alreadyAttempted),
exclusions,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isSelected = index === combo.targetIndex; | |
| // The pick's `attempted` list includes the winner itself; only non-selected | |
| // targets can be "already-attempted" (fallback picks exclude earlier tries). | |
| const alreadyAttempted = !isSelected && combo.attempted.includes(key); | |
| const exclusions: TraceCandidateInput["exclusions"] = []; | |
| if (!configured) exclusions.push({ code: "unconfigured" }); | |
| if (configured && !enabled) exclusions.push({ code: "disabled" }); | |
| if (inCooldown) exclusions.push({ code: "cooldown" }); | |
| if (isSelected && inCooldown) exclusions.push({ code: "selected-despite-cooldown" }); | |
| if (!isSelected && alreadyAttempted && exclusions.length === 0) { | |
| exclusions.push({ code: "already-attempted" }); | |
| } | |
| if (!isSelected && exclusions.length === 0) exclusions.push({ code: "not-selected" }); | |
| return { | |
| provider: target.provider, | |
| model: target.model, | |
| eligible: enabled && !inCooldown && !alreadyAttempted, | |
| exclusions, | |
| const isSelected = index === combo.targetIndex; | |
| // The pick's `attempted` list includes the winner itself; only non-selected | |
| // targets can be "already-attempted" (fallback picks exclude earlier tries). | |
| const alreadyAttempted = !isSelected && combo.attempted.includes(key); | |
| const exclusions: TraceCandidateInput["exclusions"] = []; | |
| if (!configured) exclusions.push({ code: "unconfigured" }); | |
| if (configured && !enabled) exclusions.push({ code: "disabled" }); | |
| if (inCooldown) exclusions.push({ code: "cooldown" }); | |
| if (isSelected && inCooldown) exclusions.push({ code: "selected-despite-cooldown" }); | |
| if (!isSelected && alreadyAttempted && exclusions.length === 0) { | |
| exclusions.push({ code: "already-attempted" }); | |
| } | |
| if (!isSelected && exclusions.length === 0) exclusions.push({ code: "not-selected" }); | |
| return { | |
| provider: target.provider, | |
| model: target.model, | |
| // The selected target is eligible by definition: the pick already happened. | |
| // Its cooldown is annotated via the `selected-despite-cooldown` exclusion above. | |
| eligible: isSelected || (enabled && !inCooldown && !alreadyAttempted), | |
| exclusions, |
🤖 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/router.ts` around lines 414 - 431, Update the eligibility calculation in
the target-mapping function around isSelected so the selected combo target
remains eligible even when it was chosen despite cooldown; preserve exclusion
annotation via selected-despite-cooldown. Keep non-selected targets’ existing
enabled, cooldown, and already-attempted eligibility rules unchanged, and do not
alter buildRouteDecisionTrace.
| quota: quotaEvidenceForCandidate({ | ||
| provider: candidate.provider, | ||
| model: candidate.model, | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify quota evidence inputs and the unknownEvidence.quota default.
set -euo pipefail
echo "--- quotaEvidenceForCandidate call sites ---"
rg -nP --type=ts -C8 '\bquotaEvidenceForCandidate\s*\('
echo "--- healthEvidenceForCandidate call sites ---"
rg -nP --type=ts -C8 '\bhealthEvidenceForCandidate\s*\('
echo "--- unknownEvidence defaults in profile normalization ---"
rg -nP --type=ts -C10 '\bunknownEvidence\b' src/routing/profile.ts src/config.tsRepository: lidge-jun/opencodex
Length of output: 202
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(router|quota|health|evaluator|profile|config)(\.[^.]+)?$|src/' | head -200
printf '%s\n' '--- symbol occurrences ---'
grep -RInE 'quotaEvidenceForCandidate|healthEvidenceForCandidate|unknownEvidence|routeModelInternal|NoEligiblePolicyCandidateError' src --include='*.ts' --include='*.tsx' 2>/dev/null | head -300Repository: lidge-jun/opencodex
Length of output: 10843
🏁 Script executed:
set +e
printf '%s\n' '--- relevant tracked files ---'
git ls-files 'src/*' | grep -E '(^|/)(router|quota|health|evaluator|profile|config)([^/]*)\.(ts|tsx)$'
printf '%s\n' '--- relevant symbols ---'
git grep -n -E 'quotaEvidenceForCandidate|healthEvidenceForCandidate|unknownEvidence|routeModelInternal|NoEligiblePolicyCandidateError' -- 'src/**/*.ts' 'src/**/*.tsx' 2>/dev/null
exit 0Repository: lidge-jun/opencodex
Length of output: 3517
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- src/routing/quota.ts ---'
sed -n '1,120p' src/routing/quota.ts
printf '%s\n' '--- src/routing/health.ts ---'
sed -n '80,145p' src/routing/health.ts
printf '%s\n' '--- src/routing/evaluator.ts ---'
sed -n '215,305p' src/routing/evaluator.ts
printf '%s\n' '--- src/routing/profile.ts ---'
sed -n '335,365p' src/routing/profile.ts
printf '%s\n' '--- src/router.ts ---'
sed -n '420,490p' src/router.tsRepository: lidge-jun/opencodex
Length of output: 14916
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- quota evidence call sites ---'
git grep -n -C5 'quotaEvidenceForCandidate'
printf '%s\n' '--- default unknown evidence ---'
git grep -n -C8 'DEFAULT_UNKNOWN_EVIDENCE'
printf '%s\n' '--- candidate account fields and routing profile types ---'
git grep -n -E -C5 'accountRef|codexAccountId|interface .*Candidate|candidates:' -- src/routing src/router.ts
printf '%s\n' '--- quota-related tests and configuration ---'
git grep -n -E -C4 'unknownEvidence|optimize.*quota|quota.*exclude|quotaEvidence|accountRef|codexAccountId' -- '*test*' '*spec*' 'docs*' 'config*' 2>/dev/null || trueRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- quota scoring behavior tests ---'
sed -n '78,155p' tests/quota-scoring.test.ts
printf '%s\n' '--- policy execution tests around routing failures ---'
sed -n '95,155p' tests/policy-execution.test.ts
printf '%s\n' '--- read-only source invariant check ---'
python3 - <<'PY'
from pathlib import Path
router = Path("src/router.ts").read_text()
quota = Path("src/routing/quota.ts").read_text()
profile = Path("src/routing/profile.ts").read_text()
production_calls = router.count("quotaEvidenceForCandidate(")
call_block = router.split("quotaEvidenceForCandidate(", 1)[1].split("}),", 1)[0]
print("router quota call count:", production_calls)
print("router call block:", call_block.strip())
print("quota account gates:", "codexAccountId" in quota and "accountRef" in quota)
print("default quota mode:", profile.split('quota: "', 1)[1].split('"', 1)[0])
PYRepository: lidge-jun/opencodex
Length of output: 6508
Provide quota account context to live policy scoring, or disable live quota optimization. src/router.ts:458-461 passes only provider and model, so src/routing/quota.ts:33-55 returns unknown evidence for every live policy candidate. The evaluator then cannot use actual headroom, and optimize.quota is inert. With unknownEvidence.quota: "exclude", all candidates are rejected; the default "penalize" avoids that failure but still applies the same fixed penalty to every candidate.
🤖 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/router.ts` around lines 458 - 461, Update the quotaEvidenceForCandidate
call in the live policy scoring path to pass the candidate’s quota account
context required by src/routing/quota.ts, enabling account-specific headroom
evaluation. If that context is unavailable in this flow, disable live quota
optimization instead of producing unknown quota evidence for every candidate;
preserve valid candidate scoring and the configured unknownEvidence behavior.
| function isPrivateHostname(hostname: string): boolean { | ||
| return hostname.startsWith("10.") || hostname.startsWith("192.168.") | ||
| || /^172\.(1[6-9]|2\d|3[01])\./.test(hostname); | ||
| } | ||
|
|
||
| function localRemoteEvidence(baseUrl: string | undefined): Pick<RouteCapabilityEvidence, "localOnly" | "remoteAllowed"> { | ||
| if (typeof baseUrl !== "string" || baseUrl.length === 0) return {}; | ||
| try { | ||
| const hostname = new URL(baseUrl).hostname; | ||
| if (!hostname) return {}; | ||
| if (isLocalHostname(hostname) || isPrivateHostname(hostname)) return { localOnly: true }; | ||
| return { remoteAllowed: true }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isPrivateHostname misses link-local and IPv6 private ranges.
The predicate covers only 10., 192.168., and 172.16-31.. It does not cover 169.254.0.0/16 (link-local), 100.64.0.0/10 (CGNAT/Tailscale), or IPv6 unique-local fc00::/7. A candidate whose baseUrl names such a host is labelled remoteAllowed: true at Line 70 instead of localOnly: true. A profile that requires localOnly then excludes a genuinely local candidate.
This is evidence accuracy only; destination enforcement stays with assertProviderDestinationAllowed in src/router.ts Lines 250 and 296.
🔧 Proposed fix
-function isPrivateHostname(hostname: string): boolean {
- return hostname.startsWith("10.") || hostname.startsWith("192.168.")
- || /^172\.(1[6-9]|2\d|3[01])\./.test(hostname);
-}
+function isPrivateHostname(hostname: string): boolean {
+ const host = hostname.replace(/^\[|\]$/g, "").toLowerCase();
+ if (host.startsWith("10.") || host.startsWith("192.168.") || host.startsWith("169.254.")) return true;
+ if (/^172\.(1[6-9]|2\d|3[01])\./.test(host)) return true;
+ if (/^100\.(6[4-9]|[7-9]\d|1[01]\d|12[0-7])\./.test(host)) return true;
+ // IPv6 unique-local (fc00::/7) and link-local (fe80::/10).
+ return /^f[cd][0-9a-f]{2}:/.test(host) || /^fe[89ab][0-9a-f]:/.test(host);
+}🤖 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/routing/capability.ts` around lines 59 - 70, Expand isPrivateHostname to
classify link-local 169.254.0.0/16, CGNAT 100.64.0.0/10, and IPv6 unique-local
fc00::/7 hostnames as private, while preserving the existing private-range
checks. Keep localRemoteEvidence unchanged so these hosts produce localOnly
evidence instead of remoteAllowed.
| const localRemote = localRemoteEvidence(provider?.baseUrl); | ||
| const encryptedCodexTasks = isCanonicalOpenAiForwardProvider( | ||
| provider ?? { adapter: "", authMode: undefined, baseUrl: undefined }, | ||
| ); | ||
|
|
||
| return { | ||
| ...(typeof contextWindow === "number" ? { contextWindow } : {}), | ||
| ...(typeof image === "boolean" ? { image } : {}), | ||
| ...(typeof tools === "boolean" ? { tools } : {}), | ||
| ...(reasoningEfforts !== undefined && reasoningEfforts.length > 0 ? { reasoningEfforts } : {}), | ||
| ...(serviceTier !== "unknown" ? { serviceTier } : {}), | ||
| ...localRemote, | ||
| encryptedCodexTasks, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
encryptedCodexTasks reports a definitive false for unknown providers and ignores the authMode backfill.
Two defects sit on the same expression.
First, Line 123 fabricates { adapter: "", authMode: undefined, baseUrl: undefined } when config.providers[providerName] is absent. The canonical check returns false, and Line 133 emits that false unconditionally. Every other dimension omits its key when evidence is missing, per the doctrine at Lines 8-10. An unconfigured candidate therefore claims proven absence of encrypted-task support instead of unknown, and a penalize or allow unknown-evidence policy never applies to it.
Second, src/router.ts Lines 497-499 backfills an omitted authMode to "forward" before running the same canonical check, because registry routing applies that default. This file does not mirror the backfill. A configured built-in openai provider that omits authMode routes as canonical-forward but reports encryptedCodexTasks: false here. A profile requiring encryptedCodexTasks then excludes the only candidate that supports it.
🔧 Proposed fix
const localRemote = localRemoteEvidence(provider?.baseUrl);
- const encryptedCodexTasks = isCanonicalOpenAiForwardProvider(
- provider ?? { adapter: "", authMode: undefined, baseUrl: undefined },
- );
+ // Mirror the registry backfill applied in src/router.ts (routeModelInternal): an omitted
+ // authMode on the built-in OpenAI row routes as forward, so evidence must agree.
+ const encryptedCodexTasks = provider === undefined
+ ? undefined
+ : isCanonicalOpenAiForwardProvider(
+ provider.authMode === undefined ? { ...provider, authMode: "forward" as const } : provider,
+ );
return {
...(typeof contextWindow === "number" ? { contextWindow } : {}),
...(typeof image === "boolean" ? { image } : {}),
...(typeof tools === "boolean" ? { tools } : {}),
...(reasoningEfforts !== undefined && reasoningEfforts.length > 0 ? { reasoningEfforts } : {}),
...(serviceTier !== "unknown" ? { serviceTier } : {}),
...localRemote,
- encryptedCodexTasks,
+ ...(typeof encryptedCodexTasks === "boolean" ? { encryptedCodexTasks } : {}),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const localRemote = localRemoteEvidence(provider?.baseUrl); | |
| const encryptedCodexTasks = isCanonicalOpenAiForwardProvider( | |
| provider ?? { adapter: "", authMode: undefined, baseUrl: undefined }, | |
| ); | |
| return { | |
| ...(typeof contextWindow === "number" ? { contextWindow } : {}), | |
| ...(typeof image === "boolean" ? { image } : {}), | |
| ...(typeof tools === "boolean" ? { tools } : {}), | |
| ...(reasoningEfforts !== undefined && reasoningEfforts.length > 0 ? { reasoningEfforts } : {}), | |
| ...(serviceTier !== "unknown" ? { serviceTier } : {}), | |
| ...localRemote, | |
| encryptedCodexTasks, | |
| }; | |
| const localRemote = localRemoteEvidence(provider?.baseUrl); | |
| // Mirror the registry backfill applied in src/router.ts (routeModelInternal): an omitted | |
| // authMode on the built-in OpenAI row routes as forward, so evidence must agree. | |
| const encryptedCodexTasks = provider === undefined | |
| ? undefined | |
| : isCanonicalOpenAiForwardProvider( | |
| provider.authMode === undefined ? { ...provider, authMode: "forward" as const } : provider, | |
| ); | |
| return { | |
| ...(typeof contextWindow === "number" ? { contextWindow } : {}), | |
| ...(typeof image === "boolean" ? { image } : {}), | |
| ...(typeof tools === "boolean" ? { tools } : {}), | |
| ...(reasoningEfforts !== undefined && reasoningEfforts.length > 0 ? { reasoningEfforts } : {}), | |
| ...(serviceTier !== "unknown" ? { serviceTier } : {}), | |
| ...localRemote, | |
| ...(typeof encryptedCodexTasks === "boolean" ? { encryptedCodexTasks } : {}), | |
| }; |
🤖 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/routing/capability.ts` around lines 121 - 134, Update the
encryptedCodexTasks derivation around isCanonicalOpenAiForwardProvider to return
an omitted capability when provider configuration is absent, rather than
emitting false for an unknown provider. For configured providers, mirror the
router’s authMode backfill by treating an omitted authMode as "forward" before
performing the canonical check, while preserving the existing canonical-provider
conditions and output shape.
| const rows = handle.query( | ||
| `SELECT status, close_reason AS closeReason, terminal_status AS terminalStatus, | ||
| duration_ms AS durationMs, timestamp | ||
| FROM requests WHERE ${where.join(" AND ")} | ||
| ORDER BY timestamp DESC LIMIT ?`, | ||
| ).all(...values, HEALTH_MAX_SAMPLES) as HealthSample[]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The as HealthSample[] cast asserts non-null columns; a NULL duration_ms produces a perfect latency score.
Line 121 casts raw SQLite rows to HealthSample, whose durationMs is typed number. SQLite returns null for a NULL column, and no validation runs. Trace the effect:
- Line 144 pushes
nullintolatencies. - Line 162 sorts with
(a, b) => a - b;nullcomparisons yield0, so ordering is undefined. medianat Line 85 returnsnullor(null + x) / 2.- Line 164 assigns that to
evidence.recentLatencyMs. healthScoreLine 186 testsp50 === undefined, which isfalsefornull. Line 188 computes1 - null / 60_000→1, the maximum latency score.
A target whose rows carry NULL durations therefore scores best on the latency axis. status has the same exposure in classifySample: null >= 400 is false, so a NULL status classifies as "success".
Fix the cast rather than patching healthScore, so both consumers get validated values.
🔧 Proposed fix: validate at the boundary
const rows = handle.query(
`SELECT status, close_reason AS closeReason, terminal_status AS terminalStatus,
duration_ms AS durationMs, timestamp
FROM requests WHERE ${where.join(" AND ")}
ORDER BY timestamp DESC LIMIT ?`,
- ).all(...values, HEALTH_MAX_SAMPLES) as HealthSample[];
+ ).all(...values, HEALTH_MAX_SAMPLES) as Array<Record<string, unknown>>;
+ const samples: HealthSample[] = rows.flatMap(row => {
+ // A row without a usable status or timestamp cannot be classified or decayed.
+ if (typeof row.status !== "number" || typeof row.timestamp !== "number") return [];
+ return [{
+ status: row.status,
+ closeReason: typeof row.closeReason === "string" ? row.closeReason : null,
+ terminalStatus: typeof row.terminalStatus === "string" ? row.terminalStatus : null,
+ durationMs: typeof row.durationMs === "number" ? row.durationMs : Number.NaN,
+ timestamp: row.timestamp,
+ }];
+ });Then iterate samples instead of rows, and skip the latency push when the duration is not finite:
if (row.terminalStatus === "incomplete") incompleteStreams += 1;
- latencies.push(row.durationMs);
+ if (Number.isFinite(row.durationMs)) latencies.push(row.durationMs);🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 116-121: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🤖 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/routing/health.ts` around lines 116 - 121, Replace the unsafe
HealthSample[] cast on the query result with boundary validation that accepts
only rows having finite numeric status and durationMs values, preserving
timestamp and closeReason fields as appropriate. Store the validated samples,
iterate those instead of raw rows in the health calculation, and only add finite
durationMs values to latencies so classifySample and healthScore never receive
NULL-derived values.
…ced onto latest dev
… docs: cost is a score dimension
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66ae44aca4
ℹ️ 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".
| outcome: headroom >= require.minQuotaHeadroom ? "satisfied" : "unsatisfied", | ||
| }); | ||
| } else { | ||
| requirements.push({ id: "min-quota-headroom", expected: require.minQuotaHeadroom, outcome: "unknown" }); |
There was a problem hiding this comment.
When a profile has require.minQuotaHeadroom and quota headroom is unknown, this new requirement is returned as a generic unknown requirement, so the evaluator later applies unknownEvidence.capability and emits unknown-capability instead of honoring unknownEvidence.quota. In profiles that deliberately allow unknown quota but keep unknown capabilities excluded, an otherwise valid candidate is rejected despite the quota policy saying to allow it; classify this requirement as quota evidence or special-case the min-quota-headroom id in the unknown-handling path.
Useful? React with 👍 / 👎.
| const implementedWeight = healthWeight + quotaWeight + costWeight; | ||
| const priorityWeight = Math.max(0, 1 - implementedWeight); | ||
| const components: RouteScoreEvidence["components"] = { configuredPriority: priority }; | ||
| let total = priorityWeight * priority; |
There was a problem hiding this comment.
For profiles that set unknownEvidence.cost, quota, or health to "allow", the corresponding value stays null, but these lines still remove that dimension's weight from priority and never add a replacement component. A cost-only profile with unknownEvidence.cost: "allow" therefore gives an unknown-cost candidate a total of 0 and can prefer a later candidate with any tiny known cost, even though the documented policy says unknown evidence is allowed and never treated as zero; either reallocate allowed-unknown weights or use a neutral score.
Useful? React with 👍 / 👎.
| ...(maxPercent !== undefined | ||
| ? { headroom: Math.max(0, Math.min(1, 1 - maxPercent / 100)) } | ||
| : {}), | ||
| exhausted: isCodexQuotaExhausted(quota), |
There was a problem hiding this comment.
When Codex quota evidence is built for an account that has both weekly and monthly cached readings, this call omits the account plan, so Go/Free accounts are evaluated with the default weekly+monthly rule instead of the monthly-only rule used by the existing pool helpers. A Go/Free account with stale or secondary weeklyPercent: 100 and monthlyPercent still available is reported as exhausted (and scored with zero headroom), which can make quota-aware dry-runs or any account-specific policy evidence avoid a usable account; pass the plan through or share the same plan-aware usage/exhaustion helper.
Useful? React with 👍 / 👎.
…t evidence (RI-08)
66ae44a to
18c4c3d
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. |
|
✅ Deterministic PR hygiene checks passed. |
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 `@docs-site/src/content/docs/reference/configuration/routing.md`:
- Line 107: Update the `optimize?` configuration documentation to state that the
default configured-priority share is 0.55, matching the displayed health, quota,
and cost defaults and the residual calculation used by the evaluator; leave the
surrounding scoring description unchanged.
🪄 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: 82d721fd-81d0-49e1-a6ea-e19c361c5425
📒 Files selected for processing (9)
devlog/_plan/260804_router_intelligence/000_master_plan.mddocs-site/src/content/docs/reference/configuration/routing.mdsrc/router.tssrc/routing/cost.tssrc/routing/evaluator.tssrc/routing/trace.tstests/cost-scoring.test.tstests/policy-execution.test.tstests/routing-profile.test.ts
💤 Files with no reviewable changes (1)
- src/routing/trace.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
RI-08 of the Router Intelligence / Routing Control Plane programme. Adds
cost-aware policy scoring and hard cost limits, reusing the canonical
price/cost normalization (
src/usage/cost.ts).Estimated cost, price-source provenance, estimated-vs-authoritative
distinction, and cost-limit exclusions are all recorded in the route-decision
trace. Unknown prices stay unknown (never free).
Scope
src/routing/cost.ts:costEvidenceForCandidate()-estimateRequestCost-backed evidence:estimatedUsd,priceSource(jawcode / expected / unmatched),incomplete(estimated usage OR expected-price overlay),
limitUsd,excludedByLimit.costScore()- deterministic relative score (cheaper is better, reference=
COST_SCORE_REFERENCE_USD1.0 or the profile limit when set); unknownreturns null.
src/routing/evaluator.ts:estimatedUsd > limits.maxEstimatedCostUsd->cost-limitexclusion, ineligible;
unknownEvidence.cost):exclude->unknown-priceexclusion,
penalize-> deterministic 0.3 floor,allow-> priority only;components.cost.src/router.ts- execution assembles cost evidence per candidate (usage isunknown pre-dispatch, so execution-time cost is honestly unknown unless
evidence is supplied via dry-run/evaluate).
tests/cost-scoring.test.ts- 6 tests.Estimated vs authoritative
incompleteis set when usage was estimated OR the price came from theexpected-price overlay; authoritative jawcode prices with reported usage are
complete.
Privacy / security
per-request payloads beyond the bounded trace field.
bun run privacy:scanpasses.Compatibility
RI-04). RI-05..07 behavior preserved when
optimize.cost: 0andunknownEvidence.cost: allow.Dependency
Branch base:
feat/ri-07-quota-aware-routinghead9dbdce2ab.Non-goals
are enforced on evidence supplied via dry-run/evaluate; execution-time cost
stays unknown-until-proven).
Local verification (exact)
bun x tsc --noEmit-> PASSED (0 errors)bun run test tests/cost-scoring.test.ts-> 6/6 passbun run privacy:scan-> passedSummary by CodeRabbit
New Features
Documentation
Tests