Repository navigation
fix(autorouter): compare historical and new savings consistently - #43348
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
|
@greptileai Please re-review 5a40335; the baseline-label issue is fixed, with a regression that fails when the guard is removed |
|
bugbot run Please review the latest commit, 5a40335, including eligible spend breakdowns and the fix for ambiguous baseline labels |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5a40335. Configure here.
…fixtures (#43958) * test(ci): repair stale request fakes, spend-log golden, auto-router labels, and Interactions spec lookups Request fakes now carry the scope a real Starlette request has, the GCS pub/sub spend-log golden gains the agent identity keys from #43722, the auto-router session tests follow the baseline_models contract from #43348, and the Interactions spec checks resolve the create body and resource paths from the live spec instead of hardcoded names * test(ci): move retired OpenAI text-completion fixtures to live vehicles OpenAI still serves native /v1/completions on the gpt-5.4 family, so the single-prompt cases move to text-completion-openai/gpt-5.4-nano. Multi-prompt batches and echo with logprobs now 500 on every OpenAI model, so those cases keep the same text-completion-openai transport pointed at Fireworks, which documents both. The optional-params test asserts the request body actually sent instead of a success callback whose assertions were swallowed * test(ci): use a serverless Fireworks model for the text-completion batch and echo cases gpt-oss-20b is on-demand only on Fireworks, so the CI key got 404 model not deployed; glm-5p3-flash is listed as serverless * test(ci): skip the ROI calculator repository listing in the security route sweep GET /roi-calculator/repositories (#43669) lists repositories from the configured GitHub API, api.github.com by default, so the S2 sweep's GET of every route made the owned proxy reach an external host and failed the egress check in 31 integration-security tests. It joins /get/latest_release_info in the deny list
TLDR
Problem this solves:
How it solves it:
Intentional product change: the savings card compares eligible requests across both estimation methods. Unknown estimates are excluded, the duplicate actual-spend row is removed, and LLM/classification subtotals use the same eligible requests. Partial sessions with multiple baseline models omit the ambiguous model label
User Flow
Before: historical savings disappear and the card mixes different requests
combined-savings-completeover the last 30 daysAfter: old and new savings, actual spend, baseline and percentage describe the same requests
combined-savings-completeover the last 30 daysValidation
The restored breakdown passes 41 endpoint tests, 37 PostgreSQL tests and 63 dashboard tests. Earlier validation passed 277 backend tests. Recovery covers positive, zero and negative savings, user scope, missing/corrupt logs, changed request counts and skipped classification. Mutating the classifier subtotal to include unknown requests fails six regressions; removing the partial-session baseline-label guard also fails its regression. Earlier history and unknown-spend mutations also failed
Scoped Python/dashboard lint, type and budget gates, and generated API-type synchronization pass on the committed tip. Live benchmark and session endpoints agree on $30 saved, $10 eligible actual and $40 baseline. The dashboard shows $9.90 LLM plus $0.10 classification and uses eligible requests for the classification rate
142 added test lines versus 318 production lines, excluding generated types (0.45:1). No migrations or spend-write changes
CI and patch coverage pass on 5a40335. Veria reports no security issues on that commit. The earlier Greptile finding is fixed; fresh Greptile and Bugbot reviews have been requested
Pre-Submission checklist
Screenshots / Proof of Fix
Real localhost proxies and browser, using the same synthetic PostgreSQL records on both revisions. Historical request: $9 actual including $0.10 classification, $29.50 recorded savings. New request: $1 actual, $0.50 saved. Unknown request: $99 actual with no estimate. A second identical session omits the retained historical log. This reads recorded charges, so provider inference does not apply
The requests below use an admin key for benchmarks and the owning virtual key for sessions
Before (40297e6)
Historical, new and unknown requests
PROXY=http://127.0.0.1:48282Missing historical log
combined-savings-missingAfter (5a40335)
Historical, new and unknown requests
PROXY=http://127.0.0.1:48284Missing historical log
combined-savings-missingType
Bug Fix
Caveats
Medium
Low
Final Attestation
Note
Medium Risk
Benchmark and session savings math now depends on bounded spend-log scans and reconciliation; incorrect recovery could misstate baselines, though mismatches suppress breakdown fields and preserve recorded savings totals.
Overview
Auto-router benchmarks and session APIs now keep recorded historical savings visible while aligning actual spend, baseline, classifier subtotals, and savings % to the same eligible requests—historical estimates plus newer tracked estimates, excluding turns with no savings estimate (including high-spend “unknown” requests that previously inflated actual spend).
When session rollups have partial estimate coverage, a new bounded spend-log reconciliation (
historical_session_comparisons) joins retainedLiteLLM_SpendLogsto rollups, verifies totals, and backfills comparison fields; failures degrade gracefully (savings dollars preserved, cost breakdown marked unavailable). Benchmark aggregation addssavings_estimated_classifier_costand asavings_comparison_completegate before exposing baseline math.baseline_modelnow derives frombaseline_modelsand is suppressed when partial coverage and multiple baselines would mislabel the counterfactual. The cost-optimization dashboard updates copy and layout: single “total estimated savings” hero, eligible-request spend rows, and a message when historical savings exist without matching cost details.Reviewed by Cursor Bugbot for commit 5a40335. Bugbot is set up for automated code reviews on this repo. Configure here.