Repository navigation
Add top-level max-turn-cache-misses support with env-managed default
#40388
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 2 commits
88972d5
059f695
27cdb10
8d7d74c
ab1f56a
8612d2e
71fb6b4
5150b8c
add6512
b560e5f
d515fec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,7 @@ import ( | |
| "github.com/github/gh-aw/pkg/constants" | ||
| "github.com/github/gh-aw/pkg/jsonutil" | ||
| "github.com/github/gh-aw/pkg/logger" | ||
| "github.com/github/gh-aw/pkg/workflow/compilerenv" | ||
| "github.com/santhosh-tekuri/jsonschema/v6" | ||
| ) | ||
|
|
||
|
|
@@ -197,6 +198,9 @@ type AWFAPIProxyConfig struct { | |
| // MaxRuns is the maximum number of LLM invocations allowed for a run. | ||
| MaxRuns int `json:"maxRuns,omitempty"` | ||
|
|
||
| // MaxCacheMisses is the maximum number of consecutive cache misses allowed for a run. | ||
| MaxCacheMisses int `json:"maxCacheMisses,omitempty"` | ||
|
|
||
| // MaxAICredits is the explicit per-run AI credits budget enforced by the API proxy. | ||
| MaxAICredits int64 `json:"maxAiCredits,omitempty"` | ||
|
|
||
|
|
@@ -354,11 +358,13 @@ func BuildAWFConfigJSON(config AWFCommandConfig) (string, error) { | |
| // BuildAWFCommand (see injectMaxAICreditsExpression in awf_helpers.go). | ||
| maxAICredits := int64(0) | ||
| maxRuns := constants.DefaultMaxRuns | ||
| maxCacheMisses := compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses) | ||
|
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. [/improve-codebase-architecture] 💡 SimplificationReplace the pre-call + conditional block with a single nil-safe call: var ec *EngineConfig
if config.WorkflowData != nil {
ec = config.WorkflowData.EngineConfig
}
maxCacheMisses := ec.GetMaxCacheMisses() // handles nil, env override, and built-in defaultThis matches the single-source-of-truth intent of
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. Redundant
Compare 💡 Suggested fix// Before:
maxRuns := constants.DefaultMaxRuns
maxCacheMisses := compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses)
if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil {
maxRuns = config.WorkflowData.EngineConfig.GetMaxRuns()
maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses()
}
// After — GetMaxCacheMisses already handles the nil-receiver and fallback:
maxRuns := constants.DefaultMaxRuns
maxCacheMisses := 0
if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil {
maxRuns = config.WorkflowData.EngineConfig.GetMaxRuns()
maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses()
} else {
maxCacheMisses = compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses)
}Alternatively, since maxCacheMisses := (*EngineConfig)(nil).GetMaxCacheMisses() // resolves env/built-in default via nil-safe method
if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil {
maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses()
} |
||
| if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil { | ||
| if config.WorkflowData.EngineConfig.MaxAICredits != 0 { | ||
| maxAICredits = config.WorkflowData.EngineConfig.MaxAICredits | ||
| } | ||
| maxRuns = config.WorkflowData.EngineConfig.GetMaxRuns() | ||
| maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses() | ||
| } | ||
|
|
||
| // Token steering is enabled by default. Setting max-ai-credits to a negative | ||
|
|
@@ -373,6 +379,7 @@ func BuildAWFConfigJSON(config AWFCommandConfig) (string, error) { | |
| apiProxy := &AWFAPIProxyConfig{ | ||
| Enabled: true, | ||
| MaxRuns: maxRuns, | ||
| MaxCacheMisses: maxCacheMisses, | ||
| MaxAICredits: maxAICredits, | ||
| EnableTokenSteering: enableTokenSteering && awfSupportsTokenSteering(firewallConfig), | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ func TestBuildAWFConfigJSON(t *testing.T) { | |
| assert.Contains(t, jsonStr, `"apiProxy"`, "should include apiProxy section") | ||
| assert.Contains(t, jsonStr, `"enabled":true`, "apiProxy should be enabled") | ||
| assert.Contains(t, jsonStr, fmt.Sprintf(`"maxRuns":%d`, constants.DefaultMaxRuns), "apiProxy should emit default maxRuns") | ||
| assert.Contains(t, jsonStr, fmt.Sprintf(`"maxCacheMisses":%d`, constants.DefaultMaxCacheMisses), "apiProxy should emit default maxCacheMisses") | ||
| assert.NotContains(t, jsonStr, `"maxEffectiveTokens"`, "apiProxy should omit maxEffectiveTokens when unset") | ||
|
|
||
| // container.imageTag | ||
|
|
@@ -161,6 +162,66 @@ func TestBuildAWFConfigJSON(t *testing.T) { | |
| assert.Contains(t, jsonStr, `"maxAiCredits":333`, "apiProxy should bake in frontmatter maxAiCredits (skipping runtime expression)") | ||
| }) | ||
|
|
||
| t.Run("default max-cache-misses uses built-in default when unset", func(t *testing.T) { | ||
|
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. Environment-sensitive test will produce a false failure when 💡 Suggested fixAdd at the top of this subtest (and at the top of the base fixture subtest): t.Setenv(compilerenv.DefaultMaxCacheMisses, "") // guarantee the built-in default pathThe sibling test "enterprise default max-cache-misses env var overrides built-in default" already calls |
||
| config := AWFCommandConfig{ | ||
| EngineName: "copilot", | ||
| AllowedDomains: "github.com", | ||
| WorkflowData: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "copilot", | ||
| }, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{Enabled: true}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| jsonStr, err := BuildAWFConfigJSON(config) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, jsonStr, `"maxCacheMisses":5`, "apiProxy should emit built-in maxCacheMisses default when unset") | ||
| }) | ||
|
|
||
| t.Run("enterprise default max-cache-misses env var overrides built-in default", func(t *testing.T) { | ||
|
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. [/tdd] All three new 💡 Suggested testt.Run("env default max-cache-misses applied when WorkflowData is nil", func(t *testing.T) {
t.Setenv(compilerenv.DefaultMaxCacheMisses, "11")
config := AWFCommandConfig{
EngineName: "copilot",
AllowedDomains: "github.com",
WorkflowData: nil,
}
jsonStr, err := BuildAWFConfigJSON(config)
require.NoError(t, err)
assert.Contains(t, jsonStr, `"maxCacheMisses":11`, "should fall back to env default when WorkflowData is nil")
})This pins the |
||
| t.Setenv(compilerenv.DefaultMaxCacheMisses, "9") | ||
| config := AWFCommandConfig{ | ||
| EngineName: "copilot", | ||
| AllowedDomains: "github.com", | ||
| WorkflowData: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "copilot", | ||
| }, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{Enabled: true}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| jsonStr, err := BuildAWFConfigJSON(config) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, jsonStr, `"maxCacheMisses":9`, "apiProxy should emit env-managed default maxCacheMisses when frontmatter is unset") | ||
| }) | ||
|
|
||
| t.Run("frontmatter max-cache-misses takes precedence over env default", func(t *testing.T) { | ||
| t.Setenv(compilerenv.DefaultMaxCacheMisses, "9") | ||
| config := AWFCommandConfig{ | ||
| EngineName: "copilot", | ||
| AllowedDomains: "github.com", | ||
| WorkflowData: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "copilot", | ||
| MaxCacheMisses: 3, | ||
| }, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{Enabled: true}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| jsonStr, err := BuildAWFConfigJSON(config) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, jsonStr, `"maxCacheMisses":3`, "apiProxy should emit frontmatter maxCacheMisses ahead of env default") | ||
| }) | ||
|
|
||
| // T-AIC-PR-007: Imported workflow max-ai-credits baked into AWF config JSON when no main | ||
| // frontmatter value is present; imported value takes precedence over the runtime default. | ||
| // The compiler_orchestrator_engine.go applies MergedMaxAICredits to EngineConfig when | ||
|
|
||
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.
[/tdd] Tests cover
max-cache-misses: 0(rejected) andmax-cache-misses: 5(accepted), but not negative values. The schema definesminimum: 1, which should also reject-1, and that boundary isn't currently verified.💡 Suggested test
Completing the three-point boundary coverage (negative / zero / positive) is a spec-readable guarantee that
minimum: 1is wired correctly.