-
Notifications
You must be signed in to change notification settings - Fork 479
Guard default AI credits pricing for unsupported AWF pins #50041
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
2f2d149
4d42eee
6411354
5815b66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,19 @@ | ||
| package workflow | ||
|
|
||
| import "fmt" | ||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/github/gh-aw/pkg/constants" | ||
| ) | ||
|
|
||
| // validateDefaultAiCreditsPricing returns an error when the workflow's | ||
| // models.default-ai-credits-pricing frontmatter is present and any price field | ||
| // has a non-positive value. Absent pricing (nil) is allowed; the check is only | ||
| // enforced once a value is explicitly configured. | ||
| // models.default-ai-credits-pricing frontmatter is present and either: | ||
| // - the effective AWF version is older than AWFDefaultAiCreditsPricingMinVersion | ||
| // (the field is silently dropped during config resolution in older versions), or | ||
| // - any price field has a non-positive value. | ||
| // | ||
| // Absent pricing (nil) is allowed; both checks are only enforced once a value | ||
| // is explicitly configured. | ||
| // | ||
| // The AWF api-proxy rejects zero rates as "not configured", so requiring | ||
| // positive values here prevents silent runtime failures for self-hosted models. | ||
|
|
@@ -14,6 +22,16 @@ func validateDefaultAiCreditsPricing(workflowData *WorkflowData) error { | |
| if p == nil { | ||
| return nil | ||
| } | ||
| firewallConfig := getFirewallConfig(workflowData) | ||
| if !awfSupportsDefaultAiCreditsPricing(firewallConfig) { | ||
|
Comment on lines
+25
to
+26
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. Blocks harmless configs: this check only inspects the AWF version, not whether the firewall/sandbox is actually enabled. 💡 Details
Since 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. |
||
| awfTag := getAWFImageTag(firewallConfig) | ||
| return NewValidationError( | ||
| "models.default-ai-credits-pricing", | ||
| awfTag, | ||
| fmt.Sprintf("requires AWF %s or newer; pinned version %q drops apiProxy.defaultAiCreditsPricing during config resolution", constants.AWFDefaultAiCreditsPricingMinVersion, awfTag), | ||
| fmt.Sprintf("Set network.firewall.version or sandbox.agent.version to %s or newer:\n\nnetwork:\n firewall:\n version: %s", constants.AWFDefaultAiCreditsPricingMinVersion, constants.AWFDefaultAiCreditsPricingMinVersion), | ||
| ) | ||
| } | ||
| if p.Input <= 0 { | ||
| return fmt.Errorf("models.default-ai-credits-pricing: input must be a positive value (got %g); use a small positive rate such as 0.000001 for effectively-free self-hosted models", p.Input) | ||
| } | ||
|
|
||
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.
[/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.