-
Notifications
You must be signed in to change notification settings - Fork 449
fix: stop double-charging cache-read tokens for github-copilot provider #43048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ea763da
bd3d634
393c8f1
261e402
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,41 @@ func TestComputeModelInferenceAICCopilotAlias(t *testing.T) { | |
| assert.InDelta(t, aicViaGitHubCopilot, aicViaCopilot, 1e-9, "copilot and github-copilot should yield identical AIC") | ||
| } | ||
|
|
||
| func TestComputeModelInferenceAICGitHubCopilotCacheReadDeduction(t *testing.T) { | ||
| // github-copilot (and its aliases) proxies OpenAI and Anthropic models, which bundle | ||
| // cache-read tokens inside the reported input total (§3.5). Cache reads MUST be | ||
| // subtracted from input_tokens before applying the input price so that they are not | ||
| // double-charged. | ||
| // | ||
| // Pricing for github-copilot/claude-sonnet-4.6: | ||
| // input: $0.000003/token | ||
| // output: $0.000015/token | ||
| // cache_read: $0.0000003/token | ||
| // | ||
| // With 1000 input, 200 output, 400 cache_read (no cache_write, no reasoning): | ||
| // net input = 1000 − 400 = 600 | ||
| // cost = 600×0.000003 + 200×0.000015 + 400×0.0000003 = 0.0018 + 0.003 + 0.00012 = 0.00492 | ||
| // AIC = 0.00492 / 0.01 = 0.492 | ||
| const wantAIC = 0.492 | ||
|
|
||
| for _, provider := range []string{"github-copilot", "github_models", "github", "copilot"} { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test iterates over all four provider aliases in a single Consider also adding a negative test: verify that with 💡 Suggested negative test skeletonfunc TestComputeModelInferenceAICGitHubCopilotNoCacheRead(t *testing.T) {
// With no cache reads, net input == inputTokens; no subtraction should occur.
aicNone := computeModelInferenceAIC("github-copilot", "claude-sonnet-4.6", 1000, 200, 0, 0, 0)
aicAnthropic := computeModelInferenceAIC("anthropic", "claude-sonnet-4.6", 1000, 200, 0, 0, 0)
assert.InDelta(t, aicAnthropic, aicNone, 1e-9, "zero cache reads must not alter input token count")
}@copilot please address this. |
||
| t.Run(provider, func(t *testing.T) { | ||
| aic := computeModelInferenceAIC(provider, "claude-sonnet-4.6", 1000, 200, 400, 0, 0) | ||
| assert.InDelta(t, wantAIC, aic, 1e-9, | ||
| "provider=%q: cache reads must not be double-charged (§3.5)", provider) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestComputeModelInferenceAICGitHubCopilotNoCacheRead(t *testing.T) { | ||
| // With no cache reads, github-copilot pricing should match anthropic exactly: | ||
| // net input remains inputTokens, so no subtraction is applied. | ||
| aicViaGitHubCopilot := computeModelInferenceAIC("github-copilot", "claude-sonnet-4.6", 1000, 200, 0, 0, 0) | ||
| aicViaAnthropic := computeModelInferenceAIC("anthropic", "claude-sonnet-4.6", 1000, 200, 0, 0, 0) | ||
| assert.InDelta(t, aicViaAnthropic, aicViaGitHubCopilot, 1e-9, | ||
| "zero cache reads must not alter the charged input token count") | ||
| } | ||
|
|
||
| func TestFindOrFetchModelPricing_EmbeddedModelReturnsNil(t *testing.T) { | ||
| // claude-sonnet-4.6 is in the embedded catalog; FindOrFetchModelPricing should return | ||
| // (nil, false) so the lock.yml overlay does not duplicate what models.json already has. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The JS test covers all four alias inputs (line 166) which is excellent. One missing boundary:
cacheReadTokens > inputTokens. The Go implementation usesmax(inputTokens-cacheReadTokens, 0)to clamp negative values, but the JS implementation usesMath.max(input - cacheRead, 0)(line 204 ofmodel_costs.cjs). A test withcacheReadTokens: 1200(> inputTokens: 1000) would confirm the clamp is working and prevent silent negative-cost bugs.💡 Suggested boundary test
@copilot please address this.