Propagate models.providers pricing into AWF apiProxy (including threat-detection runs)#48107
models.providers pricing into AWF apiProxy (including threat-detection runs)#48107Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
models.providers pricing into AWF apiProxy (including threat-detection runs)
There was a problem hiding this comment.
Pull request overview
Propagates custom model pricing into AWF API-proxy configuration for primary and threat-detection runs.
Changes:
- Adds
apiProxy.providersgeneration and schema support. - Propagates model costs into threat-detection data.
- Adds regression tests and updates workflow skill references.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/awf_config.go |
Emits provider pricing overlays. |
pkg/workflow/awf_config_test.go |
Tests provider emission. |
pkg/workflow/schemas/awf-config.schema.json |
Allows the providers field. |
pkg/workflow/threat_detection_external.go |
Propagates detection pricing data. |
pkg/workflow/threat_detection_test.go |
Tests detection propagation. |
.github/skills/agentic-workflows/SKILL.md |
Adds release workflow reference. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Medium
| if providers := extractModelCostProviders(config.WorkflowData); len(providers) > 0 { | ||
| apiProxy.Providers = providers | ||
| awfConfigLog.Printf("API proxy: %d model-cost provider override(s) configured", len(providers)) |
There was a problem hiding this comment.
Fixed in 1fc2916: apiProxy.providers is now version-gated behind AWFAPIProxyProvidersMinVersion so older AWF versions (including current default) do not emit an unsupported field. I also preserved firewall version propagation into detection/evals workflow data so supported versions still receive the overlay.
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review: Propagate models.providers pricing into AWF apiProxy
The implementation is well-structured with good test coverage. One existing inline comment flags the key issue (version compatibility of apiProxy.providers vs DefaultFirewallVersion v0.27.41) — that should be resolved before merging.
No additional blocking issues found. The extractModelCostProviders logic, schema extension, and threat-detection propagation are all correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.9 AIC · ⌖ 4.5 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
|
… into AWF apiProxy config
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (109 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 Matter
ADRs 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting (not blocking) on three targeted hardening opportunities.
📋 Key Themes & Highlights
Key Themes
- Version compatibility — already flagged in the existing Copilot comment at line 570: the
DefaultFirewallVersion(v0.27.41) predates theapiProxy.providersfield. This is the highest-priority issue and already under review. - Silent failure mode —
extractModelCostProviderstype-asserts without logging the!okcase, so a type mismatch would silently reproduce the original bug. - Untyped
map[string]anyfield —Providersbreaks the struct's otherwise-typed pattern (AWFAPITargetConfig,AiCreditsPricingConfig), making the shape opaque. - Shallow test assertions —
assert.Contains(jsonStr, "providers")doesn't verify the key lands underapiProxy.
Positive Highlights
- ✅ Minimal, focused fix: 110 additions, 0 deletions
- ✅ Both main and detection paths covered
- ✅ Schema updated to prevent validation stripping
- ✅ Good regression test structure overall
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.2 AIC · ⌖ 4.91 AIC · ⊞ 6.7K
Comment /matt to run again
| if workflowData == nil || len(workflowData.ModelCosts) == 0 { | ||
| return nil | ||
| } | ||
| providers, ok := workflowData.ModelCosts["providers"].(map[string]any) |
There was a problem hiding this comment.
[/diagnosing-bugs] The type assertion workflowData.ModelCosts["providers"].(map[string]any) silently discards the value if the concrete type differs from map[string]any (e.g. after JSON round-trip). A mismatch produces silent no-op behaviour identical to the original bug — no log, no error.
💡 Suggested hardening
Add a log line in the !ok branch, consistent with the pattern used elsewhere in this file:
providers, ok := workflowData.ModelCosts["providers"].(map[string]any)
if !ok {
awfConfigLog.Printf("API proxy: models.providers has unexpected type %T; skipping provider overlay",
workflowData.ModelCosts["providers"])
return nil
}This surfaces future type mismatches at runtime instead of masking them.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 1fc2916: extractModelCostProviders now logs an explicit message when models.providers has an unexpected type and skips the overlay safely.
| // AI-credits guardrails for models not present in the built-in pricing table. | ||
| // Structure matches models.json provider format: | ||
| // providers.<provider>.models.<model>.cost.{input,output,cache_read,cache_write,reasoning} | ||
| Providers map[string]any `json:"providers,omitempty"` |
There was a problem hiding this comment.
[/diagnosing-bugs] Providers is typed map[string]any, which gives no compile-time safety and makes it easy for callers to pass structurally wrong data that only fails at AWF runtime. Consider defining a typed struct (e.g. AWFProviderConfig) mirroring the models.json provider shape.
💡 Why this matters
Other fields in this struct use concrete types (AWFAPITargetConfig, AiCreditsPricingConfig, etc.). Using map[string]any here is inconsistent and makes the shape opaque to future readers. A typed struct:
- provides schema documentation in code
- enables Go's type checker to catch structural errors before they reach the AWF binary
- is consistent with how
AWFAPITargetConfighandles targets
If the shape needs to remain flexible, at minimum add a doc comment listing the expected keys.
@copilot please address this.
There was a problem hiding this comment.
Kept Providers as map[string]any intentionally in 1fc2916 because this field forwards the upstream AWF provider catalog shape as-is and that shape is intentionally open-ended across providers/models. I added stricter behavioral guardrails instead (version gating, type-check logging, and extraction hardening) so unsupported or malformed input fails safely.
| jsonStr, err := BuildAWFConfigJSON(config) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, jsonStr, `"providers"`, "apiProxy should emit providers when model cost overlay is configured") | ||
| assert.Contains(t, jsonStr, `"anthropic"`, "apiProxy.providers should include anthropic provider key") |
There was a problem hiding this comment.
[/tdd] The test asserts that "providers" appears as a JSON substring, but does not verify the key appears specifically under apiProxy. A misconfigured serialisation that emits providers at the top level would pass this check.
💡 Tighter assertion
Unmarshal the JSON and walk to apiProxy.providers directly:
var parsed map[string]any
require.NoError(t, json.Unmarshal([]byte(jsonStr), &parsed))
apiProxy, ok := parsed["apiProxy"].(map[string]any)
require.True(t, ok, "expected apiProxy object")
_, hasProviders := apiProxy["providers"]
assert.True(t, hasProviders, "apiProxy should contain providers")This matches how other tests in this file verify nested structure.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 1fc2916: the test now unmarshals the generated JSON and asserts apiProxy.providers structurally (including nested provider/model/cost keys) instead of relying on substring checks.
There was a problem hiding this comment.
Review: non-blocking with one fixable concern
The fix is correct and well-scoped: propagating ModelCosts into both the main AWF config and the threat-detection path closes a real gap. Tests are meaningful and regression-targeted.
One medium issue flagged inline; the existing comment about version-gating is also worth tracking.
Findings summary
Medium — map alias in extractModelCostProviders (see inline comment): The providers sub-map is returned by direct type-assertion reference. Any mutation of apiProxy.Providers post-assignment silently writes back into WorkflowData.ModelCosts. A shallow clone breaks the alias safely.
Pre-existing (already commented) — apiProxy.providers emitted against AWF binary v0.27.41 which may not support the field. The version-gate pattern used elsewhere in this file (awfSupports* checks) should guard this new field too.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 70 AIC · ⌖ 4.72 AIC · ⊞ 5.7K
Comment /review to run again
| if !ok || len(providers) == 0 { | ||
| return nil | ||
| } | ||
| return providers |
There was a problem hiding this comment.
Aliased map reference returned directly — mutations to apiProxy.Providers will silently corrupt WorkflowData.ModelCosts.
💡 Suggested fix
The type assertion on line 841 returns the live sub-map from workflowData.ModelCosts["providers"]. Assigning it to apiProxy.Providers creates an alias — any downstream mutation (normalization, schema validation, or future serialization helpers) will corrupt the originating WorkflowData in place.
func extractModelCostProviders(workflowData *WorkflowData) map[string]any {
if workflowData == nil || len(workflowData.ModelCosts) == 0 {
return nil
}
providers, ok := workflowData.ModelCosts["providers"].(map[string]any)
if !ok || len(providers) == 0 {
return nil
}
// Break alias so AWFAPIProxyConfig.Providers cannot corrupt WorkflowData
clone := make(map[string]any, len(providers))
for k, v := range providers {
clone[k] = v
}
return clone
}This is especially relevant given buildThreatDetectionWorkflowData now shares the same ModelCosts map reference — a mutation via either path reaches the same underlying map.
There was a problem hiding this comment.
Fixed in 1fc2916: extractModelCostProviders now returns a cloned map (using maps.Copy) so apiProxy.Providers no longer aliases WorkflowData.ModelCosts["providers"].
|
Great work! 🎯 This PR looks ready for review. The fix properly propagates What stands out:
The implementation looks solid with appropriate coverage for regression protection. Ready for maintainer review! 🚀
|
🤖 PR Triage — §30192186368
Why: Next: Verify CI passes, then fast-track merge.
|
|
@copilot also propagate to evals job |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the Unresolved review feedback:
Maintainer follow-up:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…gation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the Unresolved review feedback:
Maintainer follow-up:
|
Done. The maintainer follow-up to propagate pricing to evals is already implemented in Remaining blocker: the review threads are still marked unresolved in GitHub UI and need maintainer/reviewer resolution plus CI re-trigger on this HEAD. |
models.providersoverrides were reaching only the agent-side catalog, while the API proxy guardrail evaluated a config that lacked provider pricing; custom/BYOK models still failed at turn 1 withunknown_model_ai_credits. The same gap affected threat-detection runs, which use their own guarded AWF config.AWF config now carries provider pricing overlays
apiProxy.providersto the generated AWF config shape and emit path.WorkflowData.ModelCosts.providersintoapiProxy.providersso proxy-side AI-credit pricing can resolve custom models.Schema alignment for emitted config
pkg/workflow/schemas/awf-config.schema.jsonto allowapiProxy.providers, preventing validation-time rejection/stripping of emitted provider catalogs.Threat-detection parity with main agent path
ModelCostsinto threat-detection workflow data construction so detection AWF config receives the same pricing overrides as the primary agent run.Focused coverage for regression protection
BuildAWFConfigJSONincludesapiProxy.providerswith custom model cost entries.awf-config.json.With this change, those entries are emitted into
apiProxy.providers(main + detection), where the AI-credits guardrail performs pricing lookup.