fix: stop double-charging cache-read tokens for github-copilot provider#43048
Conversation
…(§3.5) Cache-read tokens are bundled inside the reported input_tokens for github-copilot (and its aliases: github, copilot, github_models), just as they are for anthropic, openai, and azure-openai. The previous code did not list github-copilot in providerIncludesCacheReadsInInput, so cache reads were charged twice: once at the full input price (included in input_tokens) and again at the cache_read price – inflating every per-request ΔAI Credits and the running total. Changes: - pkg/cli/effective_tokens.go: add github-copilot, github, copilot, github_models to the bundled-semantics provider list - actions/setup/js/model_costs.cjs: add github-copilot (post-normalise) to the same list in the JS implementation - pkg/cli/model_costs_test.go: new sub-test verifying §3.5 deduction for all four provider variants - actions/setup/js/model_costs.test.cjs: new test for the same fix - docs/…/ai-credits-specification.md: expand §3.5 with the canonical provider table that now includes github-copilot Closes #43034 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43048 does not have the 'implementation' label and has only 31 new lines of code in business logic directories (threshold: 100). |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR fixes AI Credits (AIC) accounting for the github-copilot provider (and its wire aliases) by ensuring cache-read tokens that are already bundled into input_tokens are deducted before applying input pricing, preventing double-charging when cache hits occur.
Changes:
- Treat
github-copilot(and Go-level raw aliasesgithub,copilot,github_models) as “cache reads included in input” providers for cost/effective-token calculations. - Update the JS cost calculator to apply the same bundled-cache-read semantics for
github-copilot(with aliases handled by provider normalization). - Expand the AIC specification (§3.5) with an explicit provider table and add unit tests (Go + JS) asserting the exact expected AIC for a cache-read scenario across all aliases.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/effective_tokens.go | Adds github-copilot and aliases to bundled cache-read provider handling to prevent double-charging. |
| pkg/cli/model_costs_test.go | Adds regression tests asserting correct AIC for cache-read deduction across github-copilot aliases. |
| actions/setup/js/model_costs.cjs | Marks github-copilot as a bundled cache-read provider (aliases covered via normalization). |
| actions/setup/js/model_costs.test.cjs | Adds a JS regression test verifying cache-read deduction behavior across provider aliases. |
| docs/src/content/docs/specs/ai-credits-specification.md | Documents bundled-cache-read providers explicitly in §3.5, including github-copilot and aliases. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Low
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
📏 Inflation Note (informational — not a quality defect)Both test files exceed the 2:1 line-added ratio vs their production counterparts:
This is expected: the production fix is a single Verdict
Both new tests are spec-anchored (§3.5), table-driven across all four Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. 0% implementation tests (threshold: 30%). No violations. Both new tests are spec-anchored (§3.5), table-driven across all four provider aliases, and enforce the behavioral contract against double-charging of cache-read tokens.
There was a problem hiding this comment.
Review: fix double-charging cache-read tokens for github-copilot
This is a clean, well-reasoned bug fix. The root cause is clear, the changes are precise, the math is verified, and the new tests are comprehensive.
Correctness analysis
Go (effective_tokens.go): The providerIncludesCacheReadsInInput function receives the raw (pre-normalization) lowercased provider string from its call site in model_costs.go:170. Adding all four variants — github-copilot, github, copilot, github_models — is necessary and correct.
JS (model_costs.cjs): The switch wraps its argument with normalizeProvider(), which collapses all aliases to "github-copilot" before matching. Adding only "github-copilot" is sufficient and correct.
Math verification
| Value | |
|---|---|
| net input | 1000 − 400 = 600 tokens |
| input cost | 600 × 3e-6 = $0.0018 |
| output cost | 200 × 1.5e-5 = $0.0030 |
| cache-read cost | 400 × 3e-7 = $0.00012 |
| total | $0.00492 |
| AIC | 0.00492 / 0.01 = 0.492 ✓ |
The embedded catalog (pkg/cli/data/models.json) confirms the same pricing for github-copilot/claude-sonnet-4.6, so the Go test exercises real embedded data.
Test coverage
Both new tests (Go TestComputeModelInferenceAICGitHubCopilotCacheReadDeduction and JS equivalent) exercise all four provider aliases with the exact expected AIC — high confidence in the fix.
Documentation
The §3.5 table in ai-credits-specification.md now explicitly enumerates github-copilot and its aliases as bundled-semantics providers.
No issues found. Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 57.5 AIC · ⌖ 5.84 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking changes required, but flagging two improvements worth addressing.
📋 Key Themes & Highlights
Key Themes
- Alias divergence risk (
/diagnosing-bugs): Go normalizes aliases in two separate places (normalizeCatalogProviderand theproviderIncludesCacheReadsInInputswitch), while JS normalizes once innormalizeProviderbefore the switch. This asymmetry means a future alias added to Go'snormalizeCatalogProviderwould still miss deduction — the root cause pattern is partially preserved. - Boundary test gaps (
/tdd): Tests correctly cover the happy path (400 cache reads < 1000 input), but do not assert thecacheReadTokens > inputTokensclamp or thecacheReadTokens = 0identity case.
Positive Highlights
- ✅ Excellent PR description with worked-out pricing math and a before/after code block — makes the fix auditable
- ✅ Both Go and JS implementations fixed and tested in the same PR — no drift between runtimes
- ✅ Tests are well-named and structured (Arrange / Act / Assert) with inline math derivations in comments
- ✅ Spec §3.5 updated with an explicit provider table — prevents the same omission for future providers
- ✅ The
max(input - cacheRead, 0)guard is present in both implementations to avoid negative costs
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 103.9 AIC · ⌖ 8.08 AIC · ⊞ 6.6K
Comment /matt to run again
| switch normalizedProvider { | ||
| case "", "anthropic", "openai", "azure-openai", "azure_openai": | ||
| case "", "anthropic", "openai", "azure-openai", "azure_openai", | ||
| "github-copilot", "github", "copilot", "github_models": |
There was a problem hiding this comment.
[/diagnosing-bugs] Aliases are duplicated across two locations: normalizeCatalogProvider in model_costs.go and this switch, because the call site at model_costs.go:170 uses strings.ToLower(strings.TrimSpace(provider)) rather than normalizeCatalogProvider. A future alias added only to normalizeCatalogProvider would silently skip deduction here and re-introduce the double-charge bug.
💡 Suggested fix: normalize at the call site
Change model_costs.go:170 to pass the already-normalized provider:
if cacheRead > 0 && providerIncludesCacheReadsInInput(normalizeCatalogProvider(provider)) {Then this switch needs only the canonical form, mirroring the JS implementation:
case "", "anthropic", "openai", "azure-openai", "azure_openai", "github-copilot":
return true@copilot please address this.
| // AIC = 0.00492 / 0.01 = 0.492 | ||
| const wantAIC = 0.492 | ||
|
|
||
| for _, provider := range []string{"github-copilot", "github_models", "github", "copilot"} { |
There was a problem hiding this comment.
[/tdd] The test iterates over all four provider aliases in a single it block in the JS test, but in Go the loop is inside TestComputeModelInferenceAICGitHubCopilotCacheReadDeduction which is good. However, the existing tests TestComputeModelInferenceAICGitHubModels and TestComputeModelInferenceAICCopilotAlias (lines 49–64) exercise the alias normalization path without cache reads. They do not cover the combination of alias + cache-read deduction — the new test catches that gap.
Consider also adding a negative test: verify that with cacheReadTokens=0, the AIC matches plain input pricing (no unintended subtraction) to prevent future off-by-one regressions at the zero boundary.
💡 Suggested negative test skeleton
func 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.
| cacheReadTokens: 400, | ||
| cacheWriteTokens: 0, | ||
| }); | ||
| expect(aic).toBeCloseTo(0.492, 6); |
There was a problem hiding this comment.
[/tdd] The JS test covers all four alias inputs (line 166) which is excellent. One missing boundary: cacheReadTokens > inputTokens. The Go implementation uses max(inputTokens-cacheReadTokens, 0) to clamp negative values, but the JS implementation uses Math.max(input - cacheRead, 0) (line 204 of model_costs.cjs). A test with cacheReadTokens: 1200 (> inputTokens: 1000) would confirm the clamp is working and prevent silent negative-cost bugs.
💡 Suggested boundary test
it('clamps net input to 0 when cache_read exceeds input (§3.5 boundary)', async () => {
// ... same fixture setup ...
const aic = computeInferenceAIC({
provider: 'github-copilot',
model: 'claude-sonnet-4.6',
inputTokens: 1000,
outputTokens: 200,
cacheReadTokens: 1200, // exceeds input
cacheWriteTokens: 0,
});
// net input = max(1000-1200, 0) = 0
// cost = 0 * 0.000003 + 200 * 0.000015 + 1200 * 0.0000003 = 0.003 + 0.00036 = 0.00336
expect(aic).toBeCloseTo(0.336, 6);
});@copilot please address this.
|
@copilot run pr-finisher |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the follow-up in |
Triage Result
Rationale: Billing accuracy fix — cache-read tokens double-counted for github-copilot provider. Small targeted diff (+84/-1, 5 files), non-draft, clean implementation. Ready for expedited review.
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/28632917404
|
…credits-calculation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
Models served via
github-copilot(and its wire aliasesgithub,copilot,github_models) report cache-read tokens bundled inside the total input token count — identical semantics toanthropic/openai/azure-openai. Per §3.5, cache reads must be subtracted from input before applying the input price. Without this, every turn with cache hits was charged at full input price and again at the cache-read price, compounding across every turn and producing ~3–4× inflated ΔAI Credits totals (visible in the screenshot: turn 2 reports 7.8 AIC instead of the correct ~2.1 AIC).Changes
pkg/cli/effective_tokens.go— addgithub-copilot,github,copilot,github_modelstoproviderIncludesCacheReadsInInputswitch; the Go call site passes the raw lowercase provider string so all four variants are neededactions/setup/js/model_costs.cjs— add"github-copilot"to the same switch (JS normalizes aliases before the check, so one entry covers all)docs/…/ai-credits-specification.md— expand §3.5 with an explicit provider table listinggithub-copilotand its aliases as bundled-semantics providerspkg/cli/model_costs_test.go/actions/setup/js/model_costs.test.cjs— new tests asserting exact AIC (0.492 for a 1000-input/200-output/400-cache-read request) for all four provider variants