Guard default AI credits pricing for unsupported AWF pins - #50041
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (72 additions detected). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds an AWF version guard so unsupported pins cannot silently drop default AI-credit pricing.
Changes:
- Defines v0.27.43 as the minimum supported AWF version.
- Validates firewall and sandbox-agent version pins.
- Adds regression tests and a patch changeset.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/model_costs_pricing_validation.go |
Enforces the AWF minimum version. |
pkg/workflow/model_costs_pricing_validation_test.go |
Tests unsupported and minimum versions. |
pkg/workflow/awf_helpers.go |
Adds the feature-support helper. |
pkg/constants/version_constants.go |
Defines the minimum version constant. |
.changeset/patch-awf-default-ai-credits-pricing-version-guard.md |
Documents the compatibility guard. |
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: 2
- Review effort level: Balanced
| } | ||
| firewallConfig := getFirewallConfig(workflowData) | ||
| if !awfSupportsDefaultAiCreditsPricing(firewallConfig) { | ||
| return fmt.Errorf("models.default-ai-credits-pricing requires AWF %s or newer; pinned AWF version %q drops apiProxy.defaultAiCreditsPricing during config resolution", constants.AWFDefaultAiCreditsPricingMinVersion, getAWFImageTag(firewallConfig)) |
| firewallConfig := getFirewallConfig(workflowData) | ||
| if !awfSupportsDefaultAiCreditsPricing(firewallConfig) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation looks correct and complete.
getFirewallConfigalready handlessandbox.agent.versionoverride, so both firewall-pinned and sandbox-only version paths are correctly guarded.- Error message is clear and actionable, including the minimum required version and the dropped field name.
- Tests cover the known-broken
v0.27.42pin, the sandbox-only override path, and the minimum supported version.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.9 AIC · ⌖ 10.4 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one minor finding on error message fidelity; overall the change is solid.
📋 Key Themes & Highlights
Finding
- Error message version format:
getAWFImageTagstrips thevprefix, so the error message prints0.27.42when users writev0.27.42in their YAML. See inline comment on line 21 ofmodel_costs_pricing_validation.go.
Positive Highlights
- ✅ Version constant placed consistently alongside all other AWF feature-gate constants
- ✅
awfSupportsDefaultAiCreditsPricingfollows the established helper pattern exactly - ✅ Three regression tests cover the broken pin, sandbox agent override path, and minimum supported version
- ✅
DefaultFirewallVersionwas already bumped tov0.27.43, so the no-pin path is safe by default - ✅ Changeset entry is clear and accurate
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.1 AIC · ⌖ 7.95 AIC · ⊞ 7.1K
Comment /matt to run again
| if p == nil { | ||
| return nil | ||
| } | ||
| firewallConfig := getFirewallConfig(workflowData) |
There was a problem hiding this comment.
[/diagnosing-bugs] getAWFImageTag strips the v prefix (returns 0.27.42 not v0.27.42), so the error message pinned-version quote won't match what users typed in their YAML. Consider using firewallConfig.Version directly in the error string to preserve fidelity.
The second regression test already asserts Contains(err.Error(), "0.27.42") without v, so it passes either way — but users seeing the CLI error will be confused if their YAML says v0.27.42 and the message echoes 0.27.42.
@copilot please address this.
There was a problem hiding this comment.
Correctness bug: the new version guard checks the AWF version but ignores whether the firewall/api-proxy is even enabled, so it can reject harmless configs where the feature is a no-op.
💡 Themes
- The overall approach (version gate + constant + helper, consistent with existing
awfSupports*patterns) is sound and well tested for the enabled-firewall case. - The one blocking issue:
validateDefaultAiCreditsPricingshould also checkisFirewallEnabled(workflowData)before applying the version gate, mirroring howBuildAWFConfigJSON/extractDefaultAiCreditsPricingonly wire this value into the api-proxy config when the firewall path is active. As written, a workflow withsandbox.agent: false(or firewall otherwise disabled) but an old pinnedsandbox.agent.versionstring would be spuriously rejected at compile time even though the buggy AWF config-resolution path is never exercised. - No regression test covers the firewall-disabled case, so this gap is not caught by the new tests added in this PR.
🔎 Code quality review by PR Code Quality Reviewer · auto · 73.9 AIC · ⌖ 4.99 AIC · ⊞ 7.9K
Comment /review to run again
| return nil | ||
| } | ||
| firewallConfig := getFirewallConfig(workflowData) | ||
| if !awfSupportsDefaultAiCreditsPricing(firewallConfig) { |
There was a problem hiding this comment.
Blocks harmless configs: this check only inspects the AWF version, not whether the firewall/sandbox is actually enabled.
💡 Details
getFirewallConfig can return FirewallConfig{Enabled: false, Version: agentVersion} when the firewall is disabled via sandbox.agent: false (or network.firewall.enabled: false) but a version is still recorded (carried over from sandbox.agent.version). awfSupportsDefaultAiCreditsPricing only calls awfVersionAtLeast, ignoring firewallConfig.Enabled entirely.
Since DefaultAiCreditsPricing is only wired into the AWF api-proxy config inside BuildAWFConfigJSON, which is only invoked when isFirewallEnabled(workflowData) is true, this new compile-time check will reject models.default-ai-credits-pricing for workflows where the firewall/api-proxy is disabled — even though the feature is a no-op there and the config-resolution bug this PR fixes cannot occur.
Suggested fix:
firewallConfig := getFirewallConfig(workflowData)
if isFirewallEnabled(workflowData) && !awfSupportsDefaultAiCreditsPricing(firewallConfig) {
return fmt.Errorf(...)
}No test in this PR covers the "firewall disabled + old pinned version" case, so this gap goes unverified.
|
@copilot sous-chef triage: This PR still needs another pass before maintainers can investigate quickly.
|
…el_costs_pricing_validation.go Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the blocking review feedback in commit
All tests pass and |
PR Triage: #50041
Guards default AI-credits pricing for unsupported AWF pins.
|
|
🎉 This pull request is included in a new release. Release: |
models.default-ai-credits-pricingcompiled intoawf-config.json, but AWF v0.27.42 droppedapiProxy.defaultAiCreditsPricingduring config resolution, so the API proxy never receivedAWF_DEFAULT_AI_CREDITS_PRICINGand rejected unknown models.Version compatibility
AWFDefaultAiCreditsPricingMinVersionatv0.27.43, the first AWF version where the field survives resolution into the API proxy environment.awfSupportsDefaultAiCreditsPricinghelper alongside existing AWF feature gates.Compiler validation
models.default-ai-credits-pricingwhen the effective AWF version is too old, including both:network.firewall.versionsandbox.agent.versionRegression coverage
v0.27.42pin and the supported minimum version.Example rejected config: