fix(cloud): reject unknown-priced token inference (#11635) - #11649
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
lalalune
left a comment
There was a problem hiding this comment.
Reviewed the #11635 fix in a sparse checkout at 96aacaad5c.
Local evidence:
bun test packages/cloud/shared/src/lib/services/ai-pricing/lookup-fallback-pricing.test.ts packages/cloud/shared/src/lib/services/ai-pricing/lookup-missing-pricing.test.ts-> 10 pass / 0 fail.bunx @biomejs/biome@2.5.2 check packages/cloud/shared/src/lib/services/ai-pricing/lookup.ts packages/cloud/shared/src/lib/services/ai-pricing/lookup-fallback-pricing.test.ts packages/cloud/shared/src/lib/services/ai-pricing/lookup-missing-pricing.test.ts-> clean.git diff --check origin/develop...HEAD-> clean.
The important bit for me is that the missing-price fallback now returns an explicit non-zero last_resort_floor, so the warning logs match the amount actually billed and both reserve/settle go through the same non-zero path. I also like the AI_PRICING_FALLBACK_*=0 regression coverage; that closes the quiet config footgun.
I attempted bun run --cwd packages/cloud/shared typecheck locally, but this sparse checkout is missing unrelated workspace packages/types (@elizaos/core, @elizaos/security/kms, pg typings, etc.), so CI should remain the source of truth for full package typecheck.
96aacaa to
bed9901
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Rebased and narrowed this after #11647 landed. What changed from the original #11649 branch:
Fresh verification in Manual log review from the focused test: both |
bed9901 to
7abdfef
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Fail closed when a non-zero token side has no exact catalog row, no provider-max fallback, and no positive AI_PRICING_FALLBACK_* env default. This removes the hardcoded floor approach: we should not sell inference we do not know how to price. Zero-token sides still cost zero without requiring a price, provider-max fallback still covers priced providers, and explicit positive env defaults still work. Env fallback values <= 0 are treated as invalid.
7abdfef to
8ae3267
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
lalalune
left a comment
There was a problem hiding this comment.
Re-reviewed after the policy change: this no longer guesses a hardcoded floor. Unknown non-zero token pricing now fails closed unless there is an exact catalog row, provider-max fallback, or a positive env fallback; zero-token sides still do not require pricing. Verified after rebasing onto current develop: focused pricing suites 10 pass, cloud-shared typecheck pass, Biome touched files clean, git diff --check clean.
|
Agreed — this reject-policy is the right one; it should supersede my #11647. @lalalune you're right on both counts. "Do not sell inference we don't know how to price" (throw / fail-closed) is a cleaner policy than my #11647's "bill a conservative frontier-max rate," and keeping the decision inside Reconciliation (my #11647 already merged to develop, so this needs it): #11649 branched before #11647 merged, so it now conflicts on I caused the conflict, so I'm happy to rebase #11649 onto develop and resolve it toward this PR's policy — say the word (or if the author is mid-work, I'll stay clear and just leave this as the merge plan). Either way: this supersedes #11647; my merge only bought the interim "never literally $0" guarantee until this lands. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Fixes #11635.
Policy
Do not sell inference we do not know how to price.
The previous last-resort behavior could bill an uncatalogued, non-zero token side at $0. A later floor-based variant still guessed a price. This PR now fails closed instead:
AI_PRICING_FALLBACK_INPUT_USD_PER_M/AI_PRICING_FALLBACK_OUTPUT_USD_PER_Mstill workAI_PRICING_FALLBACK_*=0, negative, and non-numeric values are rejected as invalidcalculateTextCostFromCatalogthrowsPricing unavailable ... refusing to bill unknown-priced inferenceEvidence
bun test src/lib/services/ai-pricing/lookup-missing-pricing.test.ts src/lib/services/ai-pricing/lookup-fallback-pricing.test.ts-> 10 passbun run --cwd packages/cloud/shared typecheck-> passbunx @biomejs/biome@2.5.2 check packages/cloud/shared/src/lib/services/ai-pricing/lookup.ts packages/cloud/shared/src/lib/services/ai-pricing/lookup-missing-pricing.test.ts packages/cloud/shared/src/lib/services/ai-pricing/lookup-fallback-pricing.test.ts-> cleangit diff --check origin/develop...HEADandgit diff --check-> cleanMoney lane; not self-merged.