fix(routing): define hard cost-cap behavior when cost evidence is unknown - #1345
Conversation
…nown `limits.maxEstimatedCostUsd` is documented as a hard per-request ceiling, but it never fires on the live routing path. `evaluatePolicyProfile` only excludes a candidate when the estimate is a finite number (evaluator.ts), while `routeModel` assembles cost evidence without usage (router.ts), so `estimatedUsd` is always `undefined` live and any candidate silently passes a cap the operator configured as hard. The existing coverage passed only because it supplied `usage` directly, exercising a path production does not take. Fail-closed unconditionally is not safe either: with usage unwired, it would reject every live candidate whenever a cap is set, and it would change the documented dry-run contract. This adds an explicit, opt-in policy instead: limits.onUnknownCost: "allow" | "exclude" (default "allow") - "allow" preserves today's behavior and the documented contract exactly. - "exclude" makes the ceiling genuinely hard: a candidate whose cost cannot be proven under the cap is ineligible. Unknown-cost exclusions emit a distinct `cost-limit-unknown` code so a trace distinguishes "known above the cap" from "cost is unknown", which is the operator-facing distinction lidge-jun#1181 asks for. Kept separate from `unknownEvidence.cost`, which governs how an unknown-cost candidate is *scored* rather than whether the *ceiling* applies. The two mechanisms now emit distinct codes and are covered by a test asserting they stay distinguishable. Fixes lidge-jun#1181 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe routing profile API now supports an explicit policy for unknown cost estimates. The evaluator preserves finite cost-cap exclusions and optionally excludes unknown costs with ChangesUnknown cost cap handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
I will run an independent review of PR
|
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes on the current head (a8f004f014a8b3084d9037391a58c2bcde160bd3).
The fail-closed path is implemented cleanly, but #1181's operator-facing contract is still incomplete.
- High — default
allowstill leaves the cap outcome ambiguous.
#1181 explicitly requires operators to distinguish a candidate that is known below the cap from one whose cost is unknown and therefore cannot be evaluated against the cap. The PR currently emits cost-limit-unknown only when onUnknownCost: "exclude" blocks the candidate. With the default/omitted "allow", the candidate remains eligible but the trace gains no stable indication that the cap was not actually proven satisfied.
Please preserve fail-open eligibility for the default, but emit a stable non-excluding policy outcome for the unknown-cost case (for example a structured cap status or a distinct cost-limit-unknown-allowed-style reason) and carry that through the operator-visible dry-run/live trace/log surfaces. Do not represent it as an exclusion if the candidate stays eligible.
- Medium — operator docs and integration coverage are incomplete.
There is already a public Routing Profile Editor/config guide that documents limits.maxEstimatedCostUsd. onUnknownCost, its default, and the allow-vs-fail-closed semantics should be documented there; JSDoc/validation text alone is not sufficient for a new operator-facing policy field.
The new tests currently call costEvidenceForCandidate() and evaluatePolicyProfile() directly, which validates evaluator behavior but only simulates the live-path evidence shape. Please add acceptance coverage that proves the policy outcome is consistent through the actual dry-run/live routing trace/log path, including:
- unknown cost + default/
allow=> eligible with an explicit unknown-cap outcome; - unknown cost +
exclude=> ineligible withcost-limit-unknown; - the two remain distinguishable from
unknownEvidence.costhandling.
- Merge gate — rebase onto current
devand rerun CI.
This head was based on 025c3791; current dev has advanced substantially since then. Rebase/update the branch after the contract fixes and rerun exact-head CI before merge.
The evaluator's known-over-cap behavior, the opt-in fail-closed logic, and the separation from unknownEvidence.cost otherwise look coherent.
|
Superseded by #1404, which completes the operator contract from my review feedback (non-excluding @Wibias — confirming the open item on your test plan: happy for this to be closed once #1404 lands, no cherry-pick needed. I've left a review there. Thanks for the detailed review; the fail-open ambiguity under the default was a genuine gap in what I had. |
|
Superseded by #1404. Thanks @abhisheksharma2411 — your PR was the right foundation for #1181. It correctly diagnosed that live routing builds cost evidence without usage, added the opt-in #1404 keeps that core, rebases onto current Closing this draft in favor of #1404 so the work can land on tip. |
Fixes #1181.
Summary
limits.maxEstimatedCostUsdis documented as a hard per-request ceiling, but it never fires on the live routing path.evaluatePolicyProfileonly excludes a candidate when the estimate is a finite number (src/routing/evaluator.ts):…while
routeModelassembles cost evidence without usage (src/router.ts):costEvidenceForCandidatecorrectly returns{ limitUsd, incomplete: true }with noestimatedUsd, so on the live path the estimate is always unknown and any candidate passes a cap the operator configured as hard. The existing coverage passes only because it suppliesusagedirectly — exercising a path production does not take.As #1181 notes, failing closed unconditionally is not safe either: with usage unwired it would reject every live candidate whenever a cap is set, and it would silently change the documented dry-run contract.
This adds an explicit, opt-in policy instead:
{ "limits": { "maxEstimatedCostUsd": 0.25, "onUnknownCost": "exclude" // "allow" (default) | "exclude" } }"allow"(default) — preserves current behaviour and the documented contract exactly. No existing deployment changes on upgrade."exclude"— the ceiling becomes genuinely hard: a candidate whose cost cannot be proven under the cap is ineligible.Unknown-cost exclusions emit a distinct
cost-limit-unknowncode, so a trace distinguishes known above the cap from cost is unknown — the operator-facing distinction the issue asks for.Kept deliberately separate from
unknownEvidence.cost, which governs how an unknown-cost candidate is scored rather than whether the ceiling applies. A test asserts the two mechanisms stay distinguishable.src/types.tsOcxRoutingUnknownCostCapMode;onUnknownCostonOcxRoutingProfileLimitssrc/routing/profile.ts"allow"/"exclude") + normalizationsrc/routing/evaluator.tscost-limit-unknownexclusiontests/cost-cap-unknown-evidence.test.tsVerification
The defect was reproduced with a failing test before the fix: with a
$0.000001cap and live-path evidence (no usage), the candidate was still eligible and selected.Commands run against the
devbase:New test cases:
onUnknownCost: "exclude"excludes the candidate and emitscost-limit-unknownonUnknownCost, behaviour is identical to todayunknownEvidence.cost: "exclude"emitsunknown-price, notcost-limit-unknownonUnknownCosthas no effect whenmaxEstimatedCostUsdis unsetCoverage on touched files:
src/routing/profile.ts89.8% lines; every added line insrc/routing/evaluator.tscovered. This change is server-side routing logic only; no front-end surface is touched, so no screenshot applies.Checklist
OcxRoutingProfileLimitsand its validation message. Happy to add a docs entry if you point me at the right file."allow"so an upgrade cannot silently make every live candidate ineligible; the stricter behaviour is opt-in.Notes for review
onUnknownCost, or fold it intounknownEvidence, if you would prefer a single mechanism. I kept them separate because they answer different questions.Supersedes #1344, which was opened against
mainby mistake and closed. This one targetsdev, is rebased onto the currentdevhead (025c3791), and addresses the CodeRabbit finding from that PR — the test docblock and two test names no longer describe the exclusion case as "fails until fixed", since the evaluator change ships in this PR.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Bug Fixes
Tests