fix(routing): define hard cost-cap behavior when cost evidence is unknown - #1344
fix(routing): define hard cost-cap behavior when cost evidence is unknown#1344abhisheksharma2411 wants to merge 135 commits into
Conversation
Implement the CL-00 protocol conformance runner with manifest loading, assertion DSL evaluation, shipped adapter/bridge execution paths, and eight negative controls for the initial five protocol suites.
Rebase onto dev CL-00 merge, remove Chat→Responses observation projection, apply source-protocol SSE [DONE] rules, synthetic provenance, MCP actions, and Chat tool_call_id selectors per final Protocol V1 authority.
Roadmap unit for replacing the vision sidecar's provider-name model list (every openai + every anthropic row) with a real image-input capability filter, an allowed-model list on the wire, and a compact delegation-style card. The load-bearing finding is that catalog inputModalities is not a truthful capability signal: applyProviderConfigHints deliberately adds "image" to a model listed in provider.noVisionModels, because Codex gates attachments client-side and a text-only entry would block the image before the sidecar could run. A blind model therefore advertises image input, so noVisionModels membership is a hard disqualifier checked before the modality list it rewrote. Two audit rounds with an independent reviewer are recorded in 002; the write gate rejects only models that can be PROVEN blind, never merely unknown ones, so an operator can still point at a model the catalog has never heard of. No production code in this commit.
The vision sidecar picker offered every model whose provider was openai or anthropic, which is both too wide and too narrow: it listed models that cannot see, while hiding capability behind a provider name. This adds the predicate that answers the real question. Two rules make it non-obvious, and both are load-bearing: - provider.noVisionModels marks models the proxy describes images FOR, and applyProviderConfigHints deliberately adds "image" to their advertised modalities so Codex does not block the attachment client-side. A blind model therefore advertises image input, so list membership is a hard disqualifier checked BEFORE the modality list it rewrote. - Catalog rows routinely omit inputModalities (live /api/models carries none for openai or anthropic rows), so the predicate is a tri-state and unknown stays eligible. Collapsing undefined into false would empty the picker. visionEligibleModelOptions is the suggestion list and is deliberately NOT the write gate: absence from it must never imply rejection. gpt-5.6-luna and claude-haiku-4-5 are guaranteed per enabled backend so a cold catalog still offers a usable describer. No caller yet; the management API and dashboard consume this in later layers. Plan: devlog/_plan/260809_vision_sidecar_model_filter/010
…regression test(omp): close final lidge-jun#1109 path-contract review gap
Stop the PR quality gate from marking review-ready when hygiene still fails (as on lidge-jun#1324), and re-run the gate when sponsorship or exception labels change.
Document the observed ~15-20 RPM burst ceiling on opencode-zen (and cross-link it on opencode-free), and enrich opaque Zen 429s with guidance plus a parseable Retry-After so Codex clients can back off.
CodeRabbit: Zen may omit Retry-After / X-RateLimit headers on generic 429s; synthetic backoff is only added when upstream omits Retry-After.
Keep rename sources in sponsored-surface checks, load hygiene scripts from the PR base SHA, and exempt sponsorship only for write-capable repository permissions.
Address review findings on lidge-jun#1205: - chatgpt-codex-connector P2: preserveReasoningContentModels only opts models into replaying reasoning that exists; MiniMax-M3 low effort maps to thinking disabled, so a fabricated placeholder could reach non-thinking histories. Add requiresReasoningPlaceholderModels (registry/derive/router/oauth/auth-cors plumbing, docs-site table) defaulting to the preserve list; minimax/minimax-cn seed [] to opt out. Custom preserve-only provider configs keep the lidge-jun#1193 fix via fallback. - CodeRabbit minor: treat a falsy cache hit as a miss in the orphan-repair path (defense-in-depth; the write path already rejects empty strings). Refs lidge-jun#1193
…outs durable Address the remaining review findings on lidge-jun#1205: - chatgpt-codex-connector P2: the orphan-repair fallback checked only requiresReasoningPlaceholderModels, so a requires-only custom entry could fabricate reasoning_content on a path the main assistant history would never emit it on. Gate the orphan placeholder on the preserve list too. - chatgpt-codex-connector P2: Zhipu BigModel GLM ids are thinking-toggle models (low maps to disabled) AND preserve-listed, so the placeholder default could fabricate reasoning for non-thinking histories. Seed requiresReasoningPlaceholderModels: [] for zhipu-bigmodel, matching the MiniMax opt-out. - chatgpt-codex-connector P2: OAuth reconcile deleted an explicit requiresReasoningPlaceholderModels: [] opt-out on every startup because no OAuth preset seeds the field. Keep the field out of OAUTH_RECONCILE_FIELDS; registry seeds still reach existing rows via enrichProviderFromRegistry. - CodeRabbit minor: fix Korean spelling in the providers table. Refs lidge-jun#1193
feat(lab): CL-02 immutable evidence ledger and projection
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. Its title has been prefixed with |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ 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:
📝 WalkthroughWalkthroughChangesUnknown cost cap handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RoutingProfile
participant RoutingEvaluator
participant Candidate
RoutingProfile->>RoutingEvaluator: Provide normalized cost limits
RoutingEvaluator->>Candidate: Read estimated cost
alt Known estimate exceeds cap
RoutingEvaluator->>Candidate: Exclude with cost-limit
else Unknown estimate and policy is exclude
RoutingEvaluator->>Candidate: Exclude with cost-limit-unknown
else Unknown estimate and policy is allow
RoutingEvaluator->>Candidate: Keep eligible
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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 `@tests/cost-cap-unknown-evidence.test.ts`:
- Around line 1-21: Update the documentation and test names in the cost-cap
behavior tests to reflect the evaluator’s current behavior: unknown cost
evidence is allowed by default, while exclusion is opt-in. Remove obsolete
reproduce-before-fixing text that says the explicit exclusion test fails until a
fix lands, including the header and the statement near the second test, and
rename both tests to clearly describe their default-allow and opt-in-exclusion
behavior.
🪄 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: 72e55f8e-291a-41d9-aa70-52a02f1ae7c9
📒 Files selected for processing (4)
src/routing/evaluator.tssrc/routing/profile.tssrc/types.tstests/cost-cap-unknown-evidence.test.ts
| /** | ||
| * Reproduction for issue #1181 — "Routing: define hard cost-cap behavior when | ||
| * runtime cost evidence is unknown". | ||
| * | ||
| * The hard ceiling `limits.maxEstimatedCostUsd` is documented as a hard | ||
| * per-request cap. In the live routing path it never fires, because | ||
| * `router.ts` assembles cost evidence WITHOUT usage: | ||
| * | ||
| * costEvidenceForCandidate({ provider, model, limitUsd }) // no `usage` | ||
| * | ||
| * `costEvidenceForCandidate` then returns `{ limitUsd, incomplete: true }` | ||
| * with no `estimatedUsd`, and the evaluator's cap check | ||
| * (evaluator.ts:307-310) requires `typeof estimatedCost === "number"`, so an | ||
| * unknown estimate silently passes a cap the operator configured as hard. | ||
| * | ||
| * The existing test in cost-scoring.test.ts only exercises the cap with | ||
| * `usage: USAGE` supplied — i.e. on a code path production never takes. | ||
| * | ||
| * Test 1 documents current (fail-open) behaviour and passes today. | ||
| * Test 2 asserts the behaviour the issue asks for and FAILS until the fix | ||
| * lands, per the reproduce-before-fixing rule. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the obsolete reproduce-before-fixing text.
Lines 19-21 and Line 105 state that the explicit exclusion test fails until a fix lands. The evaluator now implements this behavior, so the test passes. Rename the tests and update the header to describe default allow behavior and opt-in exclusion behavior.
Proposed documentation update
- * Reproduction for issue `#1181` — "Routing: define hard cost-cap behavior when
- * runtime cost evidence is unknown".
+ * Regression coverage for issue `#1181` — "Routing: define hard cost-cap behavior
+ * when runtime cost evidence is unknown".
...
- * Test 1 documents current (fail-open) behaviour and passes today.
- * Test 2 asserts the behaviour the issue asks for and FAILS until the fix
- * lands, per the reproduce-before-fixing rule.
+ * Test 1 verifies the default fail-open policy.
+ * Test 2 verifies opt-in fail-closed behavior.
...
- test("REPRO: live-path evidence carries no estimate, so the hard cap never fires", async () => {
+ test("default allow keeps an unknown-cost candidate eligible", async () => {
...
- test("EXPECTED (fails until fixed): fail-closed cap excludes unknown-cost candidates", async () => {
+ test("opt-in fail-closed cap excludes unknown-cost candidates", async () => {Also applies to: 73-73, 105-105
🤖 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/cost-cap-unknown-evidence.test.ts` around lines 1 - 21, Update the
documentation and test names in the cost-cap behavior tests to reflect the
evaluator’s current behavior: unknown cost evidence is allowed by default, while
exclusion is opt-in. Remove obsolete reproduce-before-fixing text that says the
explicit exclusion test fails until a fix lands, including the header and the
statement near the second test, and rename both tests to clearly describe their
default-allow and opt-in-exclusion behavior.
…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>
1862f1c to
c27cafb
Compare
|
Malfunctioned PR a swell as to broad. Please open a new smaller PR. |
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. No GUI changes, 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.