fix: propagate models.providers custom pricing to api-proxy defaultAiCreditsPricing#47571
Conversation
|
Hey Your checklist clearly maps out:
The approach is focused and tied to #47365. Once you’re ready to implement, the plan is ready to go. Note: This PR is currently a draft with 0 lines changed. Move to “Ready for review” once code changes are committed.
|
…CreditsPricing - Add AWFDefaultAiCreditsPricingConfig struct and DefaultAiCreditsPricing field on AWFAPIProxyConfig - Wire extractDefaultAiCreditsPricingFromModelCosts into BuildAWFConfigJSON so frontmatter models.providers pricing is included in apiProxy.defaultAiCreditsPricing in the AWF config - Convert per-token cost strings from frontmatter to per-million-token floats required by AWF schema - Copy ModelCosts from parent to buildThreatDetectionWorkflowData so detection sub-agent also gets the pricing fallback and does not 400 with unknown_model_ai_credits - Add unit tests: TestExtractDefaultAiCreditsPricingFromModelCosts, TestBuildAWFConfigJSON_DefaultAiCreditsPricingFromModelCosts, TestValidateAWFConfigJSON_AllowsDefaultAiCreditsPricing, TestBuildThreatDetectionWorkflowData_InheritsModelCosts - Update smoke-copilot golden file and recompile affected lock files Fixes #47365 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Propagates custom model pricing into AWF API-proxy configuration for main and threat-detection runs.
Changes:
- Converts per-token model costs into AWF fallback pricing.
- Propagates model costs to detection workflows.
- Adds tests and regenerates workflow snapshots.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/awf_config.go |
Adds pricing extraction and API-proxy configuration. |
pkg/workflow/awf_config_test.go |
Tests pricing conversion and schema validation. |
pkg/workflow/threat_detection_external.go |
Propagates costs to detection runs. |
pkg/workflow/threat_detection_test.go |
Tests detection cost inheritance. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Updates WASM golden output. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates smoke workflow output. |
.github/workflows/daily-model-inventory.lock.yml |
Regenerates inventory workflow output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/awf_config.go:984
- Valid integer-valued costs are rejected here. The frontmatter schema allows numbers, and goccy/go-yaml decodes unquoted YAML integers as
uint64, so a validinput: 0oroutput: 0makes the entire pricing entry disappear. Handle the integer types produced by YAML in addition tofloat64.
switch v := raw.(type) {
case float64:
perToken = v
case string:
parsed, err := strconv.ParseFloat(strings.TrimSpace(v), 64)
if err != nil {
return 0, false
}
perToken = parsed
default:
return 0, false
pkg/workflow/awf_config.go:986
ParseFloatacceptsNaN,+Inf, and negative strings, but these are not valid AWF prices: non-finite values makejson.Marshalfail, while negatives produce config rejected by the schema (minimum: 0). Since the frontmatter schema permits arbitrary numeric strings, reject non-finite or negative values here before multiplying.
parsed, err := strconv.ParseFloat(strings.TrimSpace(v), 64)
if err != nil {
return 0, false
}
perToken = parsed
default:
return 0, false
}
return perToken * 1e6, true
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Medium
|
|
||
| // First pass: find an exact match on workflowData.Model. | ||
| if targetModel != "" { | ||
| for _, pData := range providersMap { |
There was a problem hiding this comment.
Fixed in commit fix: scope exact-match pricing pass to resolved engine provider.
The first pass now extracts resolvedProvider from EngineConfig.LLMProvider (falling back to InlineProviderID) and checks that provider entry exclusively before falling through to the sorted all-providers search. A helper searchProviderForModel eliminates the duplicated inner loop. Three new sub-tests cover the new behaviour: resolved-provider-wins, InlineProviderID-used-when-LLMProvider-empty, and falls-back-when-resolved-provider-lacks-the-model.
| // Second pass: return the first parseable entry across all providers. | ||
| for _, pData := range providersMap { |
| // CachedInput is the price per 1M cached-read tokens, in USD. | ||
| // When omitted the proxy uses input × 0.1 as a default. | ||
| CachedInput float64 `json:"cachedInput,omitempty"` |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues and one test coverage gap.
📋 Key Themes & Highlights
Issues Found
- Non-deterministic fallback selection (awf_config.go lines 878–920): Both the first-pass (model match) and second-pass (first-entry fallback) iterate over
map[string]anydirectly. Go map iteration is random; with multiple providers or models the selected pricing is unpredictable and untestable. CachedInput float64vs*float64(awf_config.go line 288): An explicitly-configuredcache_read: "0"would be serialised as absent due toomitemptyon a non-pointer float.CacheWritecorrectly uses*float64;CachedInputshould match.- Detection sub-agent test gap (threat_detection_test.go): The new test only verifies shallow map inheritance, not that
defaultAiCreditsPricingactually appears in the detection run's rendered AWF config JSON — the exact failure mode fixed by this PR.
Positive Highlights
- ✅ Clean, well-commented implementation with clear selection-order documentation
- ✅ Comprehensive unit tests for
extractDefaultAiCreditsPricingFromModelCostsandparseCostFieldToPerMillioncovering nil, empty, string, float64, and missing-field cases - ✅ Schema validation tests for
defaultAiCreditsPricing(allow/reject paths) - ✅ Both the main agent and detection sub-agent are fixed in a single, coherent change
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 45.6 AIC · ⌖ 5.04 AIC · ⊞ 6.7K
Comment /matt to run again
| if !ok { | ||
| continue | ||
| } | ||
| for _, mData := range modelsMap { |
There was a problem hiding this comment.
[/diagnosing-bugs] Non-deterministic second-pass fallback: Go map iteration order is random, so when multiple providers or models are present, the pricing entry selected is unpredictable across runs. This could silently apply the wrong model's pricing.
💡 Suggested fix
Sort provider and model keys before iterating to guarantee a deterministic result (e.g. alphabetically first provider/model):
providerKeys := make([]string, 0, len(providersMap))
for k := range providersMap { providerKeys = append(providerKeys, k) }
sort.Strings(providerKeys)
for _, pName := range providerKeys {
pData := providersMap[pName]
// ... extract modelsMap ...
modelKeys := make([]string, 0, len(modelsMap))
for k := range modelsMap { modelKeys = append(modelKeys, k) }
sort.Strings(modelKeys)
for _, mName := range modelKeys {
if pricing := modelCostEntryToDefaultPricing(modelsMap[mName]); pricing != nil {
return pricing
}
}
}The same non-determinism exists in the first pass (lines 878-899).
@copilot please address this.
| Output float64 `json:"output"` | ||
| // CachedInput is the price per 1M cached-read tokens, in USD. | ||
| // When omitted the proxy uses input × 0.1 as a default. | ||
| CachedInput float64 `json:"cachedInput,omitempty"` |
There was a problem hiding this comment.
[/codebase-design] CachedInput float64 with omitempty will silently drop an explicitly-configured zero cache-read price. Unlike CacheWrite *float64, a value of 0.0 is indistinguishable from absent for a non-pointer float64.
💡 Suggested fix
Use *float64 (pointer) for CachedInput to match the treatment of CacheWrite, making the zero-vs-omitted distinction explicit:
CachedInput *float64 `json:"cachedInput,omitempty"`Then set it in modelCostEntryToDefaultPricing with &cachedInputPerM.
If the proxy truly treats zero and absent identically for cachedInput, add a comment explaining that invariant so future readers understand the asymmetry with CacheWrite.
@copilot please address this.
| if providers == nil { | ||
| t.Fatal("expected detection ModelCosts to contain providers") | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] The new test verifies that ModelCosts is shallow-copied to the detection WorkflowData, but doesn't verify that defaultAiCreditsPricing is actually emitted in the rendered AWF config JSON for the detection run. The regression path that this fixes (unknown_model_ai_credits on the detection sub-agent) remains untested at the integration level.
💡 Suggested addition
Mirror the pattern from TestBuildAWFConfigJSON_DefaultAiCreditsPricingFromModelCosts but invoke through BuildAWFConfigJSON with a detection-flavoured WorkflowData (i.e. IsDetectionRun: true):
func TestBuildDetectionAWFConfig_PropagatesDefaultAiCreditsPricing(t *testing.T) {
parent := &WorkflowData{
AI: "claude",
ModelCosts: customPricing, // same map as above
}
detected := buildThreatDetectionWorkflowData(parent, "claude")
cfg, err := BuildAWFConfigJSON(AWFCommandConfig{WorkflowData: detected, ...})
require.NoError(t, err)
assert.Contains(t, cfg, "defaultAiCreditsPricing")
}This regression test would have caught the original bug directly.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 6d28311. TestBuildDetectionAWFConfig_PropagatesDefaultAiCreditsPricing now calls BuildAWFConfigJSON with a detection WorkflowData built from a parent with custom ModelCosts and asserts that "defaultAiCreditsPricing" appears in the rendered JSON — directly testing the regression path.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (498 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 94/100 — Excellent
📊 Metrics (5 tests)
Verdict
|
|
/matt |
There was a problem hiding this comment.
REQUEST_CHANGES — Three correctness bugs must be fixed before merge; two affect the core pricing logic directly.
### Blocking issues (3)
-
Non-deterministic second-pass fallback (
awf_config.go~line 184) — the "first parseable entry across all providers" is Go-map-random per run. Multi-modelModelCostspayloads can produce different pricing on identical inputs, silently under/over-counting credits. -
Exact-match failure falls through to wrong-model pricing (
awf_config.go~line 162) — whenworkflowData.Modelmatches a key but that entry's pricing is malformed (e.g. missingoutput), the code falls to the second pass and silently prices the run against a different model. -
Modelnot forwarded to detectionWorkflowData(threat_detection_external.goline 566) — withoutModel, the detection run'sextractDefaultAiCreditsPricingFromModelCostsalways skips the exact-match pass and uses the non-deterministic fallback, potentially with different pricing than the main agent.
### Non-blocking but should be fixed (2)
-
omitemptyonCachedInput float64(awf_config.goline 77) — explicitcache_read: 0is silently dropped; use*float64consistent withCacheWrite. -
No test for provider-prefix vs bare key mismatch (
awf_config_test.go) —model: "openai/gpt-4o"with key"gpt-4o"silently falls to the wrong-model fallback; the behaviour is undefined and untested.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 88.4 AIC · ⌖ 5.34 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/awf_config.go:184
Non-deterministic second-pass fallback returns arbitrary pricing from a random model — Go map iteration is randomised per run, so the "first parseable entry across all providers" is not stable and can silently return different pricing on identical inputs.
<details>
<summary>💡 Details and fix</summary>
The second loop (lines ~167-185) iterates over providersMap and modelsMap, both map[string]any. Go randomises map iteration, meaning two identical invocations can return pricing from …
pkg/workflow/awf_config.go:162
Exact-match failure silently falls through to wrong-model pricing — when workflowData.Model is set and the key exists in the map but the entry has incomplete pricing (e.g. missing output), the function does not stop — it falls to the second pass and returns pricing from a completely different model.
<details>
<summary>💡 Details and fix</summary>
The first-pass loop (lines ~141-163):
for mName, mData := range modelsMap {
if !strings.EqualFold(mName, targetModel) {
co…
</details>
<details><summary>pkg/workflow/threat_detection_external.go:566</summary>
**`Model` not forwarded to detection `WorkflowData` — exact-match pricing hint is lost** — only `ModelCosts` is copied; without `Model`, the detection run always skips the exact-match first pass and falls to the non-deterministic second pass, potentially pricing it on a different model than the one actually running.
<details>
<summary>💡 Details and fix</summary>
In `extractDefaultAiCreditsPricingFromModelCosts`, the first pass (exact model match) is gated on `workflowData.Model != ""`. When …
</details>
<details><summary>pkg/workflow/awf_config.go:77</summary>
**`omitempty` on `CachedInput float64` silently drops an explicit zero** — if a model defines `cache_read: "0"`, the parsed value is `0.0`, which `omitempty` treats as absent, so the JSON omits the field entirely and the proxy applies its own default (`input × 0.1`) instead of the author's explicit zero cost.
<details>
<summary>💡 Details and fix</summary>
The struct fields for the two optional pricing values are inconsistent:
```go
CachedInput float64 `json:"cachedInput,omitempty"` // drops…
</details>
<details><summary>pkg/workflow/awf_config_test.go:373</summary>
**No test for model name with provider prefix vs bare key mismatch** — the exact-match test uses the same string in both `WorkflowData.Model` and the map key, but the real failure case is `model: "openai/gpt-4o"` when the map key is `"gpt-4o"`, which would silently fall to the non-deterministic second pass.
<details>
<summary>💡 Suggested test case</summary>
```go
t.Run("provider-prefix model name does not match bare key", func(t *testing.T) {
wd := &WorkflowData{
Model: "openai/g…
</details>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two small correctness gaps and one test gap.
📋 Key Themes & Highlights
Key Themes
- Non-deterministic fallback (correctness): second-pass map iteration is unordered in Go; pricing selection is non-deterministic when
model:is unset and multiple custom models are defined. - Incomplete required-field test parity (test coverage):
missing input → nilis tested butmissing output → nilis not. - Silent parse failure (debuggability): malformed cost strings are swallowed with no signal; a test and/or log would surface this.
- ADR is still Draft: update status to
Acceptedbefore merge.
Positive Highlights
- ✅ Root cause properly addressed — per-token pricing is now forwarded to both the main agent config and the threat-detection sub-agent.
- ✅ Clean three-function decomposition (
extract→entryToPricing→parseCostField) — each function is independently testable. - ✅ Comprehensive test suite: nil inputs, string/float64 values, exact-match preference, case-insensitivity, all covered.
- ✅ ADR documents the trade-offs including the known single-entry limitation and non-determinism — good transparency.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 67.7 AIC · ⌖ 7.77 AIC · ⊞ 6.7K
Comment /matt to run again
|
@copilot lint go |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the Unresolved review feedback (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30030451443
|
…ing through to second pass Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The two github-actions
Local validation: |
🤖 PR Triage
Score breakdown: Impact 35 + Urgency 22 + Quality 15 Rationale: Fixes a silent data-loss bug where custom/BYOK model pricing was dropped by api-proxy, breaking BYOK model billing. 30 files (mostly generated lock.yml). One APPROVED review alongside CHANGES_REQUESTED. High business impact. Needs expedited human review.
|
|
@copilot please run the Unresolved review feedback (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30035273314
|
…defaultAiCreditsPricing Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All blocking issues from both
Branch is now also updated from |
|
@copilot please run the Unresolved requested changes:
|
The two
These review states cannot be cleared by the agent — clearing them requires CI to re-run on the current HEAD so new reviews are generated, or a maintainer to dismiss the stale reviews manually. |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/30046537550
|
The two
Local validation status:
The |
models.providersfrontmatter pricing reached the agent-sideGH_AW_MODELS_JSON_PATHcatalog but was silently dropped fromawf-config.json → apiProxy, so the proxy still rejected custom/BYOK models withunknown_model_ai_creditsbefore the first turn. Detection sub-agent runs had the same issue sincebuildThreatDetectionWorkflowDatanever inheritedModelCosts.Changes
awf_config.go— addsAWFDefaultAiCreditsPricingConfigstruct andDefaultAiCreditsPricingfield onAWFAPIProxyConfig; wiresextractDefaultAiCreditsPricingFromModelCostsintoBuildAWFConfigJSONto populateapiProxy.defaultAiCreditsPricingfromWorkflowData.ModelCosts. Per-token cost strings (e.g."3e-07") are converted to per-million-token floats required by the AWF schema. Model selection prefers an exact case-insensitive match on the workflow'smodel:field; falls back to the first parseable entry across all providers.threat_detection_external.go— copiesModelCostsfrom parentWorkflowDataintobuildThreatDetectionWorkflowDataso the detection guardrail config also carries the pricing fallback.With this fix, a workflow like:
now produces
"apiProxy": { ..., "defaultAiCreditsPricing": {"input": 0.3, "output": 1.5, "cachedInput": 0.03, "cacheWrite": 0.375} }inawf-config.json, unblocking both the main agent and the detection sub-agent.Run: https://github.com/github/gh-aw/actions/runs/30035273314
Run: https://github.com/github/gh-aw/actions/runs/30042541462