fix(cost): stop fabricating $0.00 prices for unpriced models - #956
Conversation
PriceBook.compute_cost() silently returned a $0.00 price for any provider/model without a configured price row, and that fabricated zero was indistinguishable from a real free price once recorded — the ledger asserted "measured, cost nothing" when the truth was "price unknown". This contradicted the repo's own honest-metrics convention and the two sibling honest-degradation patterns already in this codebase (model_discovery's unset Bytez per-token pricing, and orchestrator's separate unpriced_models reporting key). compute_cost() now returns a (cost_amount, currency_code, price_known) 3-tuple. UsageRecord gains a price_known: bool field, persisted through a new usage_price_knowledge satellite table joined the same way usage_measurements already is (including the conservative "unknown" default for rows migrated from the pre-price_known flattened schema). CostLedger.rollup()/.report()/.total() add an additive cost_amount_by_status/record_count_by_status breakdown (measured/estimated/unavailable) alongside the existing flat cost_amount total, so measured, estimated, and unavailable-priced spend are no longer opaquely blended into one authoritative-looking number. cost_amount's existing meaning and value are unchanged.
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthrough
Changes비용 계약과 원장 저장
배치 사용량 검증과 upstream 선택
라우팅 결과 비용 전파
회귀 검증과 변경 문서
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to When batch usage is marked invalid, cost estimation can omit the original prompt and understate charges for requests with large inputs. This bounded correctness issue should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant Client
participant CostRoutingCoordinator
participant CostLedger
participant PriceBook
Client->>CostRoutingCoordinator: 요청 완료 또는 사용량 결과
CostRoutingCoordinator->>CostLedger: 사용량 기록
CostLedger->>PriceBook: 비용과 price_known 계산
PriceBook-->>CostLedger: 비용 상태 반환
CostLedger-->>CostRoutingCoordinator: 원장 기록과 비용 상태
CostRoutingCoordinator-->>Client: cost_amount와 price_known 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 10 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Generated by Claude Code |
|
@opencode-agent please review this draft PR. Generated by Claude Code |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin review on #956: SqlLedgerStore._append_locked()'s satellite writes (usage_measurements, usage_price_knowledge, attribution) ran unconditionally even when the parent llm_usage_records insert was rejected as a duplicate usage_record_id. A retried append for a parent row that predates one of these satellite tables (e.g. an upgrade-migrated row with no usage_price_knowledge child, intentionally read as price-unknown) silently backfilled that child using the retry's current price/measurement state instead of what actually priced the original spend -- relabeling historical unknown-price provenance from an unrelated later call. The three satellite inserts now only run when the parent insert is actually accepted, so append() is a true no-op on a rejected duplicate. New regression test appends the same usage_record_id twice with different measurement_status/price_known values and asserts the second (rejected) append leaves the original row's provenance untouched.
|
Root cause is fixed in #979 (draft, fully validated), not yet merged so it doesn't help this specific re-run. Re-ran the failed job once (first occurrence of this cause on this PR); watching #979 to landing, which should let this self-heal. Generated by Claude Code |
PriceBook.compute_cost() treated a zero-token record the same as a missing price row: both returned price_known=False. That's wrong for zero usage specifically -- zero tokens cost zero regardless of whether the per-token price is known, since zero times any finite price is still zero, so there is no unknown quantity left to guess. This broke the required "Full unit and contract suite" check on 2b0b2f3 ("expose unknown price across usage surfaces"), which started surfacing price_known on complete()'s cost dict: a cache hit records its answer via a synthetic ("cache", "response") provider/model that never has a real price row, so every cache hit was newly (and wrongly) reported as an unpriced request with cost_amount=None instead of the honest $0.00 it actually is. compute_cost() now special-cases prompt_tokens == completion_tokens == 0 before ever consulting the price book, returning price_known=True with cost_amount=0.0 (using the price entry's own currency when one exists, the ledger's default currency otherwise). Updated the pre-existing cache-hit test to expect the now-exposed price_known: True field. Full suite: 2868 passed, 1 skipped, 1 known pre-existing fast_mlsirm gap (Python 3.11 sandbox vs the package's python_full_version >= 3.12 requirement).
|
Fixed in c37932f: Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit review on #956: retrieve_batch's invalid-usage estimation fallback passed a hardcoded [{"role": "user", "content": ""}] placeholder into _record_completion instead of the request actually submitted. When a provider marks a batch item's usage invalid, _record_completion estimates prompt tokens from that messages argument -- an empty placeholder always estimates near-zero prompt tokens regardless of the real prompt's size, silently understating batch cost for any large-input request whose usage came back invalid. submit_batch() now keeps the submitted BatchRequest list keyed by job_id (self._batch_requests, mirroring the existing self._embedding_requests pattern for embeddings batches). retrieve_batch() looks the original request up by custom_id and uses its real messages for estimation whenever usage_valid is False, falling back to the empty placeholder only when no matching original request is on record (e.g. a backend that never went through submit_batch). New regression test (test_invalid_batch_usage_estimates_prompt_tokens_from_original_request) submits a ~500-word prompt through a backend that reports invalid usage, and asserts the recorded prompt_tokens reflects the real prompt rather than the near-zero an empty placeholder would give. Full suite: 2870 passed, 1 skipped, 1 known pre-existing fast_mlsirm gap (Python 3.11 sandbox vs the package's python_full_version >= 3.12 requirement).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Updates the CHANGELOG entry to describe the actual shipped design from 2bfcaf6 (fix(cost_router): keep batch prompt fallback metadata safe) rather than the superseded intermediate approach: the prompt- token estimate lives on the durable BatchJob record itself (one publication write, no raw prompt content ever persisted), not a separate registry holding the original BatchRequest list.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…trievals Devin review on #956: _legacy_batch_requests() gated its lookup on `if job.prompt_token_estimates: return {}` -- meaning any non-empty dict short-circuited the legacy read, not just a fully-populated one. A legacy (pre-fix) job's estimates can be filled in incrementally across multiple retrieve_batch() calls (e.g. polling for still- processing items); once the first partial retrieval stored even one custom_id's estimate, every subsequent retrieval for that same job stopped consulting the legacy registry entirely, silently leaving any other still-unestimated custom_id's cost understated with no way to recover it later. retrieve_batch() now decides whether a legacy lookup is needed from the current retrieval's own items (does any item still lack a stored estimate), not from whether the job's dict happens to be non-empty. _legacy_batch_requests() itself is now a plain, ungated read -- for a normal post-fix job (fully estimated at submission time), the caller never invokes it at all, so there is no added cost to the common case. Extracted the usage-validity check the caller and the pre-scan both need into _batch_item_usage_valid() rather than duplicating it. New regression test drives a legacy job through two separate retrieve_batch() calls, each surfacing one of two legacy custom_ids, and asserts both get correctly estimated from the real submitted prompt -- reproducing the exact gap Devin flagged, which the prior gate would have left the second call's estimate at None. Targeted: tests/test_cost_router_boundaries.py + test_cost_router.py + test_batch_routing_boundaries.py + test_cost_ledger.py + test_cost_ledger_boundaries.py, 133 passed. Ruff passed; interrogate 100%. Full suite validation running in the background.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Root cause (confirmed via job log for this exact run, Not fixing it here — both fixes are already in flight elsewhere and this PR's own diff doesn't touch the review path. Will keep this PR watched and re-check once #974/#1415 land. Generated by Claude Code |
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Acknowledged. This PR's diff ( Generated by Claude Code |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
|
Generated by Claude Code |
…with PR 955 honesty precedence
| if prompt_tokens == 0 and completion_tokens == 0: | ||
| return 0.0, entry.currency_code if entry is not None else self.default_currency, True |
There was a problem hiding this comment.
🟡 Missing usage falsely confirms prices
When an unpriced call has unavailable usage, compute_cost converts its sentinel zero tokens into price_known=True. Streaming and race results claim known pricing without either input.
Prompt for agents
Distinguish authoritative zero usage from the zero-token sentinel used for measurement_status unavailable. PriceBook.compute_cost cannot infer that distinction from token counts alone. Move or override the zero-usage exception where measurement provenance is available, preserving price_known true for confirmed zero usage and cache hits while leaving unpriced unavailable stream and race records price-unknown. Add tests for both unpriced unavailable paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Both bugs (adversarial audit confirmed, both real)
Bug 1 —
PriceBook.compute_cost()fabricates a $0.00 price for any unpriced provider/model.contextual_orchestrator/cost_ledger.py. Previously:entry is None→return 0.0, self.default_currency. That flows intoCostLedger.record_usage()asUsageRecord.cost_amount, whilemeasurement_statusis computed independently — purely from whether token counts were reported (cost_router.py) — so a request with real, provider-reported usage against a model that simply has no price row yet was recorded ascost_amount=0.0, measurement_status="measured": "we measured this and it cost nothing," when the truth is "we don't know the price." This contradicts this repo's own Honest metrics convention (CLAUDE.md) and is inconsistent with two sibling honest-degradation patterns already in this codebase:model_discovery.pyleaves Bytez per-token pricing unset rather than invent one ("more honest than a misleading estimate"), andorchestrator.py's commercial-readiness report surfaces unpriced models under a separateunpriced_modelskey with a null cost instead of a fabricated zero.cost_ledger.pyhad no equivalent "price unknown" signal anywhere.The existing test
test_unpriced_model_costs_zero_and_still_recordsconfirmed the $0.0-on-unpriced behavior is deliberate (never fail recording on a missing price row) but only assertedcost_amount == 0.0, with no way to tell that zero apart from a real free price.Bug 2 (downstream of bug 1) —
rollup()/.report()/.total()blend measured, estimated, and unavailable-priced rows into one opaque total.All three summed
cost_amountfrom every stored row with zero regard formeasurement_status. The buyer-facingcost_report()API presented one authoritative-lookinggrand_total/per-dimension total that could silently mix fully measured dollars with rows that are $0 only because the price or the usage was unknown — with nothing in the output surfacing how many records, or how much of the total, fell into each bucket. The data (measurement_status) already existed per-row; it was just discarded during aggregation.The fix
PriceBook.compute_cost()now returns a(cost_amount, currency_code, price_known)3-tuple —price_known=Falseonly when there's no price row (entry is None),Trueotherwise. Updated all 3 call sites (cost_ledger.py::record_usage,model_discovery.py's discovery-ranking cost key,batch_routing.py::cheapest_upstream— the latter two discard the new third element since their existing logic doesn't need it).UsageRecordgainsprice_known: bool = True(defaultTrueso callers settingcost_amountexplicitly, bypassingPriceBook, are unaffected), captured fromcompute_cost()inrecord_usage(), included inas_dict().usage_price_knowledgesatellite table — same pattern this file already uses formeasurement_status/usage_measurements(aLEFT JOIN+COALESCE(..., 0)default, not anALTER TABLE), including in the legacy-flattened-schema migration path, where a migrated row's price-knownness is genuinely unknown and is marked0/unknown rather than assumed known.CostLedger.rollup()/.total()(and therefore.report()) add additive, backward-compatiblecost_amount_by_status/record_count_by_statusbreakdowns keyed bymeasurement_status(measured/estimated/unavailable) alongside the existing flatcost_amount— computed so the flat total is provably the sum of the breakdown (not double-computed and hoped to match).cost_amount's existing meaning and value are unchanged; every previously-passing test that asserts a flat total still passes unmodified.cost_router.py::cost_report()'s docstring updated to mention the new breakdown (no logic change — it just forwardsledger.report()'s output).Tests
test_unpriced_model_costs_zero_and_still_recordswithrecord.price_known is False; addedtest_priced_model_marks_price_known_true.test_compute_cost_returns_price_known_flag— asserts the 3-tuple shape/values for both priced and unpriced cases directly onPriceBook.test_rollup_report_total_break_down_cost_by_measurement_status— records onemeasured, oneestimated, oneunavailable(unpriced) row with distinct costs; assertscost_amount_by_status/record_count_by_statusontotal(),rollup(), andreport(), and that the flatcost_amounttotal is unchanged and equals the sum of the breakdown.test_sql_ledger_persists_price_known_flag— round-tripsprice_knownthroughSqlLedgerStore/sqlite.test_sql_ledger_migrates_flattened_usage_rows,test_orphaned_legacy_generation_is_adopted_and_dropped) to assert migrated rows getprice_known=False(unknown, not assumed known).test_ledger_table_names_follow_two_word_snake_caseto cover the new (and the previously-untestedusage_measurements) table names.compute_cost2-tuple unpackers intests/test_cost_ledger_boundaries.pyandtests/test_batch_routing_boundaries.py's_StaticPriceBookmock to the new 3-tuple contract.Validation
python -m pytest tests -q: 2835 passed, 1 skipped, 1 pre-existing unrelated failure (test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score—ModuleNotFoundError: No module named 'fast_mlsirm'; confirmed identical on unmodifiedorigin/mainviagit stash, so unrelated to this change).python -m interrogate -c pyproject.toml contextual_orchestrator/: PASSED (100.0%).cost_ledger.pyare pre-existing, untouched error-handling paths.origin/main(merged in045d17da— Bytezis_freefix, a clean auto-merge with no conflicts inmodel_discovery.py).cost_ledger.py'scompute_cost/rollup(checked before opening this one).Decimal/SQL already in use in this file).Do not merge without the required CI checks (Security workflow, OpenCode review) passing per this repo's governance.
Generated by Claude Code
Summary by CodeRabbit
$0.00으로 오인하지 않고 비용을null로 표시합니다.