feat(pricing): tiered cost accounting, model-ID normalization, and live price feeds - #9
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe change adds normalized token accounting, estimated-usage status fields, canonical model pricing, cache and reasoning charges, and opt-in remote price sources with validation and caching. Adapter and pricing tests cover provider-specific accounting, model variants, fallbacks, outages, and compatibility. ChangesPricing and usage accounting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant UseRemote
participant PricingSource
participant PricingSnapshot
Application->>UseRemote: configure remote pricing
UseRemote->>PricingSource: fetch pricing catalogue
PricingSource-->>UseRemote: parsed model rates
UseRemote->>PricingSnapshot: validate and install snapshot
PricingSnapshot-->>Application: resolve model pricing
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Reviewer's GuideImplements tiered, normalized cost accounting across providers, adds model-ID canonicalization for robust price lookup, and introduces an opt-in remote pricing layer with caching and validation, while updating the adapter and public API to expose richer Usage metadata and keeping legacy pricing APIs working. Sequence diagram for normalized usage and tiered cost computationsequenceDiagram
actor User
participant Adapter as Adapter.Complete
participant Pricing as pricing
User->>Adapter: Complete(ctx, req)
Adapter->>Adapter: usage(ctx, model, folded, req, content)
Adapter->>Adapter: extractTokens(f.genInfo)
alt tokens missing
Adapter->>Adapter: estimateTokens(ctx, req, content)
Adapter->>Adapter: warnUnreported(provider, model)
end
Adapter->>Pricing: ComputeTokens(model, Tokens)
Pricing->>Pricing: Lookup(model)
Pricing->>Pricing: Canonical(model)
Pricing->>Pricing: longestPrefix(table, canon)
Pricing->>Pricing: cacheRates(model, Price)
Pricing->>Pricing: familyCacheMultipliers(model)
Pricing-->>Adapter: cost, priced
Adapter-->>User: Response{Usage, CostUSD, Priced}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hallelx2
changed the base branch from
halleluyaholudele/hal-524-adapter-correctness
to
main
August 2, 2026 12:44
…feeds The reported cost was wrong in four independent ways. All four are fixed here because they compound: each one alone still leaves the dollar figure untrustworthy. 1. Cache and reasoning tokens were dropped. langchaingo already surfaces CacheCreationInputTokens, CacheReadInputTokens, PromptCachedTokens and ReasoningTokens; llmgate read none of them. Anthropic reports cache tokens *outside* input_tokens, so a cached prompt — writes bill at 1.25x input — was charged nothing at all. OpenAI and Google fold cached tokens *into* the prompt count, so those were charged at the full rate instead of the 0.5x/0.25x they actually cost. Usage now carries CacheWrite, CacheRead and Reasoning, normalized to a disjoint form so the fields can be summed on any provider, and Price carries the matching rate tiers with family-specific fallbacks. 2. Lookup was an exact map hit. Every ID a real API returns missed it and priced at $0: dated snapshots (claude-sonnet-4-5-20250929), SDK prefixes (models/gemini-2.5-flash), Bedrock ARNs, and the gateway-qualified forms the aggregate feeds use for the model we actually run (z-ai/glm-4.6). Canonical strips prefixes, dates and revisions, then falls back to a longest-prefix match — longest, because claude-sonnet-4 also prefixes claude-sonnet-4-5-preview and the shorter key would misprice it. 3. Missing usage was reported as a priced $0 call. When GenerationInfo carried no counts, ComputeWithOK(model, 0, 0) returned (0, true) — Priced:true asserting a call that returned content was free. Usage gains TokensReported and Estimated, and the adapter now estimates from the tokenizer rather than reporting a zero. 4. The table was hand-maintained and drifted silently. UseRemote layers a refreshed snapshot from LiteLLM or OpenRouter over the embedded defaults, under any Register overrides. It is opt-in — importing the package does no network I/O — and fails open throughout: lookups never touch the network, a failed fetch keeps the last snapshot, a snapshot is persisted so restarts start warm, and one whose rates have moved more than 10x from the embedded values is rejected as a units error rather than adopted. Rates are keyed by model ID alone, never provider+model: vectorless runs GLM-4.6 through z.ai's Anthropic-compatible gateway, and a provider-scoped key would look up "anthropic/glm-4.6" and miss. BREAKING CHANGE: pricing.Compute and pricing.ComputeWithOK are deprecated in favour of ComputeTokens, which takes a Tokens breakdown. Both still work and are behaviour-compatible for callers with no cached tokens.
…ependent TestRegisterBeatsNormalization left a global override in place, so with -shuffle=on it could run before TestLookupResolvesVariants and make that test see the pinned rate instead of the real one. Overrides are process-global; every test that sets one has to undo it.
hallelx2
force-pushed
the
halleluyaholudele/hal-529-pricing
branch
from
August 2, 2026 12:46
29eb794 to
73000c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
pricing/remote_test.go (1)
69-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare converted rates with a tolerance, not
==.Lines 69, 72, 95, and 98 compare float64 results of
perMTokwith==. The fixture values are decimal fractions that float64 cannot represent exactly, so0.0000022 * 1_000_000 == 2.2depends on rounding landing exactly right.pricing/canonical_test.goalready defines anapproxhelper with a 1e-9 tolerance for this reason. Reuse that pattern here. Both test files are in packagepricing_test, soapproxis already visible.The same applies to Lines 129, 134, 180, and 279.
♻️ Example for the LiteLLM assertions
- if sonnet.InputPerMTok != 3.0 || sonnet.OutputPerMTok != 15.0 { - t.Errorf("sonnet rates = %+v, want 3.00/15.00 per Mtok", sonnet) - } - if sonnet.CacheWritePerMTok != 3.75 || sonnet.CacheReadPerMTok != 0.30 { - t.Errorf("sonnet cache rates = %+v, want 3.75/0.30", sonnet) - } + approx(t, sonnet.InputPerMTok, 3.0, "sonnet input per Mtok") + approx(t, sonnet.OutputPerMTok, 15.0, "sonnet output per Mtok") + approx(t, sonnet.CacheWritePerMTok, 3.75, "sonnet cache-write per Mtok") + approx(t, sonnet.CacheReadPerMTok, 0.30, "sonnet cache-read per Mtok")Also applies to: 95-100
🤖 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 `@pricing/remote_test.go` around lines 69 - 74, Update the float64 rate assertions in the remote pricing tests, including the checks around sonnet and the additional assertions at the referenced locations, to use the existing pricing_test approx helper with its 1e-9 tolerance instead of direct equality. Keep the expected rate values and failure messages unchanged, and apply the same comparison pattern to all listed perMTok and cache-rate checks.pricing/remote.go (1)
133-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument that
UseRemoteblocks on the first fetch.Line 133 runs
refresh()synchronously. With the default two sources and the default 30s timeout,UseRemotecan block the caller for about 60 seconds when both sources hang. The doc comment at Lines 67-78 does not state this. Either document the blocking first fetch, or run it in the background and let lookups use the embedded table until it lands.🤖 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 `@pricing/remote.go` at line 133, Document in the UseRemote comment that its initial refresh() runs synchronously and may block the caller while the configured sources time out; preserve the current blocking behavior unless changing refresh initialization to run asynchronously with lookups falling back to the embedded table.
🤖 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 `@internal/adapter/adapter_usage_test.go`:
- Around line 184-199: Update TestUnreportedUsageWarnsOncePerModel to remove the
anthropic/warn-once-test-model entry from unreportedSeen both before the test
assertion and during t.Cleanup, while retaining the existing UnreportedUsageFunc
reset.
In `@internal/adapter/adapter.go`:
- Around line 177-182: Update extractTokens to use a provider-specific rule
instead of comparing tk.Input and tk.CacheRead magnitudes. Pass a.provider into
extractTokens or identify the provider metadata keys, and subtract cache reads
only for providers whose prompt count includes them; preserve Anthropic’s
disjoint InputTokens and CacheReadInputTokens values. Add an Anthropic
regression case where InputTokens exceeds CacheReadInputTokens and verify totals
and charging remain correct.
In `@pricing/canonical_test.go`:
- Around line 87-88: Add test cleanup for every pricing override: in
pricing/canonical_test.go lines 87-88, register cleanup to unregister both
prefixtest-model-3 and prefixtest-model-3-5; in lines 211-213, add cleanup
inside the loop to unregister tc.model; and in lines 234-236, unregister
reasoning-rate-test via t.Cleanup.
In `@pricing/pricing.go`:
- Around line 144-153: Update the documentation for Register to remove the claim
that registered IDs match exactly before normalization. Describe that override
lookups use Canonical and longest-prefix matching, so a registered key can also
match dated, prefixed, and longer-suffixed model variants; leave the
implementation unchanged.
In `@pricing/remote.go`:
- Around line 260-276: Update loadCache to run the loaded cf.Prices through vet
before returning the cached snapshot. Preserve the existing empty and cacheTTL
checks, use vet’s validated prices in the returned snapshot, and rely on vet’s
idempotent behavior for the already-canonical keys.
- Around line 144-167: Make each UseRemote installation ownership-aware by
adding an owner token to snapshot and routing all snapshot writes through store,
including the initial install and ticker refresh. Update both stop functions to
use clear, which removes the global remote value only when its ownership token
still matches, and ensure store refuses writes after that installation is closed
so an in-flight refresh cannot resurrect it.
In `@pricing/sources.go`:
- Around line 76-84: Update the LiteLLM pricing filter around CacheCreationCost
and CacheReadCost to reject or clamp negative cache costs before constructing
the Price entry, matching the existing non-negative validation used by
OpenRouter’s parseRate. Preserve valid cache costs and the existing input/output
cost checks.
In `@README.md`:
- Around line 164-169: Update the fenced model-ID mapping block in README.md by
specifying the text language in its opening fence, changing it to a text-labeled
fence while preserving the mapping contents.
- Around line 181-186: Update the remote-pricing example around
pricing.UseRemote to be self-contained: add the required pricing and context
references in the example’s imports, replace the undefined ctx with
context.Background(), and handle the returned error directly without relying on
an undefined log symbol. Preserve the existing deferred stop behavior after
successful initialization.
---
Nitpick comments:
In `@pricing/remote_test.go`:
- Around line 69-74: Update the float64 rate assertions in the remote pricing
tests, including the checks around sonnet and the additional assertions at the
referenced locations, to use the existing pricing_test approx helper with its
1e-9 tolerance instead of direct equality. Keep the expected rate values and
failure messages unchanged, and apply the same comparison pattern to all listed
perMTok and cache-rate checks.
In `@pricing/remote.go`:
- Line 133: Document in the UseRemote comment that its initial refresh() runs
synchronously and may block the caller while the configured sources time out;
preserve the current blocking behavior unless changing refresh initialization to
run asynchronously with lookups falling back to the embedded table.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a11244e-79e2-4551-9dee-0b67a3cdc785
📒 Files selected for processing (11)
README.mdclient.gointernal/adapter/adapter.gointernal/adapter/adapter_tools_test.gointernal/adapter/adapter_usage_test.gopricing/canonical.gopricing/canonical_test.gopricing/pricing.gopricing/remote.gopricing/remote_test.gopricing/sources.go
Comment on lines
+184
to
+199
| func TestUnreportedUsageWarnsOncePerModel(t *testing.T) { | ||
| var seen []string | ||
| UnreportedUsageFunc = func(p llmgate.Provider, model string) { | ||
| seen = append(seen, string(p)+"/"+model) | ||
| } | ||
| t.Cleanup(func() { UnreportedUsageFunc = nil }) | ||
|
|
||
| const model = "warn-once-test-model" | ||
| for range 3 { | ||
| usageFor(t, model, llmgate.ProviderAnthropic, map[string]any{"nothing": true}) | ||
| } | ||
|
|
||
| if len(seen) != 1 { | ||
| t.Fatalf("warned %d times %v, want exactly once per provider/model", len(seen), seen) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reset the deduplication state in test cleanup.
Line 189 resets UnreportedUsageFunc but leaves anthropic/warn-once-test-model in unreportedSeen. A repeated run such as go test -count=2 can then receive no callback and fail the expectation. Delete this key before the assertion and in cleanup.
Proposed fix
func TestUnreportedUsageWarnsOncePerModel(t *testing.T) {
+ const model = "warn-once-test-model"
+ key := string(llmgate.ProviderAnthropic) + "/" + model
+ unreportedSeen.Delete(key)
+
var seen []string
UnreportedUsageFunc = func(p llmgate.Provider, model string) {
seen = append(seen, string(p)+"/"+model)
}
- t.Cleanup(func() { UnreportedUsageFunc = nil })
-
- const model = "warn-once-test-model"
+ t.Cleanup(func() {
+ UnreportedUsageFunc = nil
+ unreportedSeen.Delete(key)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestUnreportedUsageWarnsOncePerModel(t *testing.T) { | |
| var seen []string | |
| UnreportedUsageFunc = func(p llmgate.Provider, model string) { | |
| seen = append(seen, string(p)+"/"+model) | |
| } | |
| t.Cleanup(func() { UnreportedUsageFunc = nil }) | |
| const model = "warn-once-test-model" | |
| for range 3 { | |
| usageFor(t, model, llmgate.ProviderAnthropic, map[string]any{"nothing": true}) | |
| } | |
| if len(seen) != 1 { | |
| t.Fatalf("warned %d times %v, want exactly once per provider/model", len(seen), seen) | |
| } | |
| } | |
| func TestUnreportedUsageWarnsOncePerModel(t *testing.T) { | |
| const model = "warn-once-test-model" | |
| key := string(llmgate.ProviderAnthropic) + "/" + model | |
| unreportedSeen.Delete(key) | |
| var seen []string | |
| UnreportedUsageFunc = func(p llmgate.Provider, model string) { | |
| seen = append(seen, string(p)+"/"+model) | |
| } | |
| t.Cleanup(func() { | |
| UnreportedUsageFunc = nil | |
| unreportedSeen.Delete(key) | |
| }) | |
| for range 3 { | |
| usageFor(t, model, llmgate.ProviderAnthropic, map[string]any{"nothing": true}) | |
| } | |
| if len(seen) != 1 { | |
| t.Fatalf("warned %d times %v, want exactly once per provider/model", len(seen), seen) | |
| } | |
| } |
🤖 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 `@internal/adapter/adapter_usage_test.go` around lines 184 - 199, Update
TestUnreportedUsageWarnsOncePerModel to remove the
anthropic/warn-once-test-model entry from unreportedSeen both before the test
assertion and during t.Cleanup, while retaining the existing UnreportedUsageFunc
reset.
Comment on lines
+177
to
+182
| // OpenAI and Google fold cache reads into the prompt count, Anthropic | ||
| // does not. Detect which by asking whether subtracting would go | ||
| // negative — that only happens when the counts were already disjoint. | ||
| if tk.CacheRead > 0 && tk.Input >= tk.CacheRead { | ||
| tk.Input -= tk.CacheRead | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not infer Anthropic token semantics from token magnitudes.
Line 180 subtracts cache reads when Input >= CacheRead. Anthropic reports these fields as disjoint. A response with 10,000 uncached input tokens and 8,000 cache-read tokens becomes 2,000 uncached input tokens. This undercounts TotalTokens by 8,000 and undercharges the request.
Pass a.provider into extractTokens, or identify the provider-specific metadata keys, and subtract cache reads only for providers that include them in prompt tokens. Add an Anthropic regression case where InputTokens > CacheReadInputTokens.
Proposed fix
- tk, reported := extractTokens(f.genInfo)
+ tk, reported := extractTokens(a.provider, f.genInfo)
-func extractTokens(gi map[string]any) (pricing.Tokens, bool) {
+func extractTokens(provider llmgate.Provider, gi map[string]any) (pricing.Tokens, bool) {
...
- if tk.CacheRead > 0 && tk.Input >= tk.CacheRead {
+ if provider != llmgate.ProviderAnthropic && tk.CacheRead > 0 && tk.Input >= tk.CacheRead {
tk.Input -= tk.CacheRead
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // OpenAI and Google fold cache reads into the prompt count, Anthropic | |
| // does not. Detect which by asking whether subtracting would go | |
| // negative — that only happens when the counts were already disjoint. | |
| if tk.CacheRead > 0 && tk.Input >= tk.CacheRead { | |
| tk.Input -= tk.CacheRead | |
| } | |
| // OpenAI and Google fold cache reads into the prompt count, Anthropic | |
| // does not. Detect which by asking whether subtracting would go | |
| // negative — that only happens when the counts were already disjoint. | |
| if provider != llmgate.ProviderAnthropic && tk.CacheRead > 0 && tk.Input >= tk.CacheRead { | |
| tk.Input -= tk.CacheRead | |
| } |
🤖 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 `@internal/adapter/adapter.go` around lines 177 - 182, Update extractTokens to
use a provider-specific rule instead of comparing tk.Input and tk.CacheRead
magnitudes. Pass a.provider into extractTokens or identify the provider metadata
keys, and subtract cache reads only for providers whose prompt count includes
them; preserve Anthropic’s disjoint InputTokens and CacheReadInputTokens values.
Add an Anthropic regression case where InputTokens exceeds CacheReadInputTokens
and verify totals and charging remain correct.
Comment on lines
+87
to
+88
| pricing.Register("prefixtest-model-3", cheap) | ||
| pricing.Register("prefixtest-model-3-5", dear) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Three tests leak process-global pricing overrides. pricing.Register writes to the package-level overrides map, and that map outlives the test that wrote it. Every leaked key stays in the price book for the rest of the test binary and participates in the longestPrefix scan of later lookups. The PR already fixed this at Line 125 and in pricing/remote_test.go Line 142; these three sites were missed.
pricing/canonical_test.go#L87-L88: add at.Cleanupthat callspricing.Unregisterforprefixtest-model-3andprefixtest-model-3-5.pricing/canonical_test.go#L211-L213: add at.Cleanupinside the loop that callspricing.Unregister(tc.model).pricing/canonical_test.go#L234-L236: add at.Cleanupthat callspricing.Unregister("reasoning-rate-test").
📍 Affects 1 file
pricing/canonical_test.go#L87-L88(this comment)pricing/canonical_test.go#L211-L213pricing/canonical_test.go#L234-L236
🤖 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 `@pricing/canonical_test.go` around lines 87 - 88, Add test cleanup for every
pricing override: in pricing/canonical_test.go lines 87-88, register cleanup to
unregister both prefixtest-model-3 and prefixtest-model-3-5; in lines 211-213,
add cleanup inside the loop to unregister tc.model; and in lines 234-236,
unregister reasoning-rate-test via t.Cleanup.
Comment on lines
144
to
+153
| // Register overrides or adds a price. Safe for init() in callers. | ||
| // | ||
| // Registered IDs match exactly, before any normalization, and sit above | ||
| // both the remote snapshot and the embedded table — so this is how to pin | ||
| // a rate that would otherwise resolve elsewhere. | ||
| func Register(model string, p Price) { | ||
| priceMu.Lock() | ||
| defer priceMu.Unlock() | ||
| prices[model] = p | ||
| overrides[model] = p | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the Register doc: overrides are not matched exactly.
The comment states that registered IDs "match exactly, before any normalization". Lookup passes overrides to lookupIn at Line 108, and lookupIn applies Canonical and longestPrefix to that table as well. A registered key therefore also captures dated, prefixed, and longer-suffixed variants. Register("claude-sonnet-4-5", ...) also prices claude-sonnet-4-5-20250929 and claude-sonnet-4-5-preview.
Either update the doc to describe the three-step match, or make the overrides layer exact-match only if that was the intent.
📝 Proposed doc correction
-// Register overrides or adds a price. Safe for init() in callers.
-//
-// Registered IDs match exactly, before any normalization, and sit above
-// both the remote snapshot and the embedded table — so this is how to pin
-// a rate that would otherwise resolve elsewhere.
+// Register overrides or adds a price. Safe for init() in callers.
+//
+// The overrides layer sits above both the remote snapshot and the
+// embedded table, and it resolves like every other layer: exact ID
+// first, then the canonical form, then the longest matching prefix. A
+// registered base ID therefore also pins its dated and suffixed
+// variants.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Register overrides or adds a price. Safe for init() in callers. | |
| // | |
| // Registered IDs match exactly, before any normalization, and sit above | |
| // both the remote snapshot and the embedded table — so this is how to pin | |
| // a rate that would otherwise resolve elsewhere. | |
| func Register(model string, p Price) { | |
| priceMu.Lock() | |
| defer priceMu.Unlock() | |
| prices[model] = p | |
| overrides[model] = p | |
| } | |
| // Register overrides or adds a price. Safe for init() in callers. | |
| // | |
| // The overrides layer sits above both the remote snapshot and the | |
| // embedded table, and it resolves like every other layer: exact ID | |
| // first, then the canonical form, then the longest matching prefix. A | |
| // registered base ID therefore also pins its dated and suffixed | |
| // variants. | |
| func Register(model string, p Price) { | |
| priceMu.Lock() | |
| defer priceMu.Unlock() | |
| overrides[model] = p | |
| } |
🤖 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 `@pricing/pricing.go` around lines 144 - 153, Update the documentation for
Register to remove the claim that registered IDs match exactly before
normalization. Describe that override lookups use Canonical and longest-prefix
matching, so a registered key can also match dated, prefixed, and
longer-suffixed model variants; leave the implementation unchanged.
Comment on lines
+144
to
+167
| done := make(chan struct{}) | ||
| go func() { | ||
| t := time.NewTicker(interval) | ||
| defer t.Stop() | ||
| for { | ||
| select { | ||
| case <-t.C: | ||
| refresh() | ||
| case <-done: | ||
| return | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| var once sync.Once | ||
| return func() { | ||
| once.Do(func() { | ||
| close(done) | ||
| remote.Store(nil) | ||
| }) | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
stop() can be undone by an in-flight refresh, and it clears a snapshot it may not own.
remote is a package-level pointer shared by every UseRemote call. Two problems follow.
A refresh that is already running when stop() executes still reaches Line 120 and stores its snapshot. Line 163 closes done, but that only prevents the next tick. The layer that stop() removed at Line 164 therefore reappears, which contradicts the guarantee documented at Lines 76-78.
A second UseRemote call installs into the same global pointer. If the first caller stops afterwards, Line 164 clears the second caller's snapshot.
Give each installation an ownership token and clear the pointer only when the installed snapshot still belongs to that installation. Also stop storing once the installation is closed.
🔒 Sketch of an ownership-aware stop
+ // Each installation owns the snapshots it stores, so a stop never
+ // clears a layer another caller installed and a late refresh never
+ // resurrects a stopped one.
+ type owner struct{}
+ var (
+ ownMu sync.Mutex
+ stopped bool
+ )
+ store := func(snap *snapshot) {
+ ownMu.Lock()
+ defer ownMu.Unlock()
+ if stopped {
+ return
+ }
+ snap.owner = &ownMu
+ remote.Store(snap)
+ }
+ clear := func() {
+ ownMu.Lock()
+ defer ownMu.Unlock()
+ stopped = true
+ if cur := remote.Load(); cur != nil && cur.owner == &ownMu {
+ remote.Store(nil)
+ }
+ }This needs a matching owner any field on snapshot. Use store at Lines 95 and 120, and clear in both stop functions at Lines 137 and 164.
🤖 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 `@pricing/remote.go` around lines 144 - 167, Make each UseRemote installation
ownership-aware by adding an owner token to snapshot and routing all snapshot
writes through store, including the initial install and ticker refresh. Update
both stop functions to use clear, which removes the global remote value only
when its ownership token still matches, and ensure store refuses writes after
that installation is closed so an in-flight refresh cannot resurrect it.
Comment on lines
+260
to
+276
| func loadCache(dir string) (*snapshot, error) { | ||
| b, err := os.ReadFile(filepath.Join(dir, cacheName)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var cf cacheFile | ||
| if err := json.Unmarshal(b, &cf); err != nil { | ||
| return nil, err | ||
| } | ||
| if len(cf.Prices) == 0 { | ||
| return nil, fmt.Errorf("cached snapshot is empty") | ||
| } | ||
| if time.Since(cf.AsOf) > cacheTTL { | ||
| return nil, fmt.Errorf("cached snapshot from %s is too old", cf.AsOf.Format(time.RFC3339)) | ||
| } | ||
| return &snapshot{prices: cf.Prices, asOf: cf.AsOf, source: cf.Source + " (cached)"}, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the cached snapshot with vet before installing it.
loadCache checks only that the file parses, is non-empty, and is within cacheTTL. It skips vet, so the disk path bypasses the non-positive-rate filter at Lines 200-202 and the drift check at Lines 227-232. A cache written by an earlier build, or an edited file on disk, is adopted at startup without any sanity check. UseRemote installs it directly at Line 95.
Run the loaded prices through vet. The keys are already canonical, and vet is idempotent on canonical keys.
🛡️ Proposed fix
if time.Since(cf.AsOf) > cacheTTL {
return nil, fmt.Errorf("cached snapshot from %s is too old", cf.AsOf.Format(time.RFC3339))
}
- return &snapshot{prices: cf.Prices, asOf: cf.AsOf, source: cf.Source + " (cached)"}, nil
+ // A cached file is data from a previous process. Re-validate it
+ // against the current embedded table rather than trusting it.
+ cleaned, err := vet(cf.Prices)
+ if err != nil {
+ return nil, fmt.Errorf("cached snapshot rejected: %w", err)
+ }
+ return &snapshot{prices: cleaned, asOf: cf.AsOf, source: cf.Source + " (cached)"}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func loadCache(dir string) (*snapshot, error) { | |
| b, err := os.ReadFile(filepath.Join(dir, cacheName)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var cf cacheFile | |
| if err := json.Unmarshal(b, &cf); err != nil { | |
| return nil, err | |
| } | |
| if len(cf.Prices) == 0 { | |
| return nil, fmt.Errorf("cached snapshot is empty") | |
| } | |
| if time.Since(cf.AsOf) > cacheTTL { | |
| return nil, fmt.Errorf("cached snapshot from %s is too old", cf.AsOf.Format(time.RFC3339)) | |
| } | |
| return &snapshot{prices: cf.Prices, asOf: cf.AsOf, source: cf.Source + " (cached)"}, nil | |
| } | |
| func loadCache(dir string) (*snapshot, error) { | |
| b, err := os.ReadFile(filepath.Join(dir, cacheName)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var cf cacheFile | |
| if err := json.Unmarshal(b, &cf); err != nil { | |
| return nil, err | |
| } | |
| if len(cf.Prices) == 0 { | |
| return nil, fmt.Errorf("cached snapshot is empty") | |
| } | |
| if time.Since(cf.AsOf) > cacheTTL { | |
| return nil, fmt.Errorf("cached snapshot from %s is too old", cf.AsOf.Format(time.RFC3339)) | |
| } | |
| // A cached file is data from a previous process. Re-validate it | |
| // against the current embedded table rather than trusting it. | |
| cleaned, err := vet(cf.Prices) | |
| if err != nil { | |
| return nil, fmt.Errorf("cached snapshot rejected: %w", err) | |
| } | |
| return &snapshot{prices: cleaned, asOf: cf.AsOf, source: cf.Source + " (cached)"}, nil | |
| } |
🤖 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 `@pricing/remote.go` around lines 260 - 276, Update loadCache to run the loaded
cf.Prices through vet before returning the cached snapshot. Preserve the
existing empty and cacheTTL checks, use vet’s validated prices in the returned
snapshot, and rely on vet’s idempotent behavior for the already-canonical keys.
Comment on lines
+76
to
+84
| if e.InputCostPerToken <= 0 || e.OutputCostPerToken <= 0 { | ||
| continue | ||
| } | ||
| out[id] = Price{ | ||
| InputPerMTok: perMTok(e.InputCostPerToken), | ||
| OutputPerMTok: perMTok(e.OutputCostPerToken), | ||
| CacheWritePerMTok: perMTok(e.CacheCreationCost), | ||
| CacheReadPerMTok: perMTok(e.CacheReadCost), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clamp negative LiteLLM cache costs.
Lines 82 and 83 convert CacheCreationCost and CacheReadCost without a sign check. vet in pricing/remote.go Lines 200-202 validates only InputPerMTok and OutputPerMTok, so a negative cache cost reaches the snapshot and lowers the computed bill. The OpenRouter path already rejects negatives in parseRate at Line 166. Apply the same guard here.
🛡️ Proposed guard
+ cacheWrite, cacheRead := e.CacheCreationCost, e.CacheReadCost
+ if cacheWrite < 0 {
+ cacheWrite = 0
+ }
+ if cacheRead < 0 {
+ cacheRead = 0
+ }
out[id] = Price{
InputPerMTok: perMTok(e.InputCostPerToken),
OutputPerMTok: perMTok(e.OutputCostPerToken),
- CacheWritePerMTok: perMTok(e.CacheCreationCost),
- CacheReadPerMTok: perMTok(e.CacheReadCost),
+ CacheWritePerMTok: perMTok(cacheWrite),
+ CacheReadPerMTok: perMTok(cacheRead),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if e.InputCostPerToken <= 0 || e.OutputCostPerToken <= 0 { | |
| continue | |
| } | |
| out[id] = Price{ | |
| InputPerMTok: perMTok(e.InputCostPerToken), | |
| OutputPerMTok: perMTok(e.OutputCostPerToken), | |
| CacheWritePerMTok: perMTok(e.CacheCreationCost), | |
| CacheReadPerMTok: perMTok(e.CacheReadCost), | |
| } | |
| if e.InputCostPerToken <= 0 || e.OutputCostPerToken <= 0 { | |
| continue | |
| } | |
| cacheWrite, cacheRead := e.CacheCreationCost, e.CacheReadCost | |
| if cacheWrite < 0 { | |
| cacheWrite = 0 | |
| } | |
| if cacheRead < 0 { | |
| cacheRead = 0 | |
| } | |
| out[id] = Price{ | |
| InputPerMTok: perMTok(e.InputCostPerToken), | |
| OutputPerMTok: perMTok(e.OutputCostPerToken), | |
| CacheWritePerMTok: perMTok(cacheWrite), | |
| CacheReadPerMTok: perMTok(cacheRead), | |
| } |
🤖 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 `@pricing/sources.go` around lines 76 - 84, Update the LiteLLM pricing filter
around CacheCreationCost and CacheReadCost to reject or clamp negative cache
costs before constructing the Price entry, matching the existing non-negative
validation used by OpenRouter’s parseRate. Preserve valid cache costs and the
existing input/output cost checks.
Comment on lines
+164
to
+169
| ``` | ||
| claude-sonnet-4-5-20250929 -> claude-sonnet-4-5 | ||
| models/gemini-2.5-flash -> gemini-2.5-flash | ||
| us.anthropic.claude-opus-4-1-v1:0 -> claude-opus-4-1 | ||
| z-ai/glm-4.6 -> glm-4.6 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the fenced block.
markdownlint reports MD040 at Line 164. Add text to this model-ID mapping block.
-```
+```text
claude-sonnet-4-5-20250929 -> claude-sonnet-4-5
...
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.23.1)</summary>
[warning] 164-164: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @README.md around lines 164 - 169, Update the fenced model-ID mapping block
in README.md by specifying the text language in its opening fence, changing it
to a text-labeled fence while preserving the mapping contents.
</details>
<!-- fingerprinting:phantom:poseidon:terra -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:afd613c609ef0de8b95133b0 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+181
to
+186
| ```go | ||
| stop, err := pricing.UseRemote(ctx, pricing.RemoteConfig{ | ||
| CacheDir: "/var/cache/llmgate", // survive restarts | ||
| OnError: func(src string, err error) { log.Warn("price refresh", "src", src, "err", err) }, | ||
| }) | ||
| defer stop() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the remote-pricing example self-contained.
The sample uses undefined ctx and log, and its surrounding import list does not include pricing. Use context.Background() and handle the error directly, or show the required logger and imports.
🤖 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 `@README.md` around lines 181 - 186, Update the remote-pricing example around
pricing.UseRemote to be self-contained: add the required pricing and context
references in the example’s imports, replace the undefined ctx with
context.Background(), and handle the returned error directly without relying on
an undefined log symbol. Preserve the existing deferred stop behavior after
successful initialization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The reported cost was wrong in four independent ways. All four are fixed together because they compound: any one left in place still leaves the dollar figure untrustworthy.
1. Cache and reasoning tokens were dropped entirely
langchaingo already surfaces
CacheCreationInputTokens,CacheReadInputTokens,PromptCachedTokensandReasoningTokens. llmgate read none of them.input_tokens, so never countedprompt_tokensat full rateUsagenow carriesCacheWriteTokens,CacheReadTokensandReasoningTokens, normalized to a disjoint form soInput + CacheWrite + CacheReadis the whole prompt on every provider.Pricegains the matching tiers, with family-specific fallbacks when a table entry predates cache pricing (Anthropic 1.25x/0.1x, OpenAI 1.0x/0.5x, Google 1.0x/0.25x — one global default would be wrong for two of the three).2. Lookup was an exact map hit
Every ID a real API actually returns missed the table and priced at $0:
Canonicalstrips vendor/region prefixes, snapshot dates and revisions, then falls back to a longest-prefix match. Longest matters:claude-sonnet-4also prefixesclaude-sonnet-4-5-preview, and the shorter key would bill Sonnet 4.5 at Sonnet 4 rates. Prefix matches must also land on a segment boundary, soglm-4.5can't absorbglm-4.55.Rates are keyed by model ID alone, never provider+model — vectorless runs GLM-4.6 through z.ai's Anthropic-compatible gateway, and a provider-scoped key would look up
anthropic/glm-4.6and miss.3. Missing usage was reported as a priced $0 call
With no counts in
GenerationInfo,ComputeWithOK(model, 0, 0)returned(0, true)—Priced: truepositively asserting that a call which returned content was free. Worse than the unpriced case, which at least says "unknown".UsagegainsTokensReportedandEstimated; the adapter estimates from the tokenizer and labels it rather than reporting a zero.4. The table drifted silently
UseRemotelayers a refreshed snapshot from LiteLLM or OpenRouter over the embedded defaults, beneath anyRegisteroverrides.Opt-in and staying that way — importing
pricingdoes no network I/O. It fails open at every layer: lookups never touch the network, a failed fetch keeps the last snapshot, snapshots persist to disk so restarts start warm, and a snapshot whose rates moved >10x from the embedded values is rejected as a units error rather than adopted. That last gate is the one that matters — both feeds quote per-token where we work in per-Mtok, so a conversion slip is a factor of a million and invisible in a diff.perMTokis one named function with its own test for the same reason.Verification
go build,go vet,golangci-lint,staticcheck— clean-shuffle=onto prove no order dependence, after addingpricing.Unregisterso the layering test can undo its own overridego test— both feeds are exercised againsthttptestfixtures that mirror the real payloads, including OpenRouter's string-encoded rates and LiteLLM's non-modelsample_speckey-racestill not runnable locally (no gcc); CI covers itNotable tests:
TestAnthropicCacheWriteIsBilled(the $0 regression),TestOpenAICachedTokensSubtracted(the double-charge),TestLongestPrefixWins,TestMissingUsageIsEstimatedNotZero,TestRemoteRejectsImplausibleRates,TestGLMUsageThroughAnthropicGateway.Known gap
Gemini 2.5 thinking tokens land in the API's
thoughtsTokenCount, which langchaingo does not surface at all — they remain invisible and unbilled. That needs the native client work (HAL-539) and is tracked on HAL-529 so it isn't mistaken for a zero.Closes HAL-529
Closes HAL-530
Closes HAL-531
Closes HAL-532
Summary by Sourcery
Improve pricing accuracy and robustness by introducing tiered token cost accounting, canonicalized model ID resolution, and optional live price feeds, and by surfacing richer usage metadata in responses.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes