-
Notifications
You must be signed in to change notification settings - Fork 479
Add AWF token steering frontmatter opt-out #50122
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
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 |
|---|---|---|
|
|
@@ -2110,6 +2110,11 @@ sandbox: | |
| # Format 2: GitHub Actions expression that resolves to a boolean at runtime | ||
| model-fallback: "example-value" | ||
|
|
||
| # Enable or disable API proxy token steering. Set to false to preserve the | ||
| # explicitly configured provider and model. | ||
| # (optional) | ||
| token-steering: true | ||
|
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. [/grill-with-docs] The reference doc shows the default as Suggest showing the opt-out value in the example, matching the usage example in token-steering: false@copilot please address this. |
||
|
|
||
| # Container runtime for the agent container. Use 'gvisor' to run the agent under | ||
| # gVisor's runsc runtime for additional kernel-level isolation. Use 'docker-sbx' | ||
| # to run the agent inside a Docker sbx microVM with KVM hypervisor-level isolation | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3515,6 +3515,11 @@ | |
| "description": "Enable or disable model fallback for unresolved model selections. Set to false for BYOK Azure OpenAI deployments to prevent deployment-name rewriting. Supports literal boolean or GitHub Actions expression.", | ||
| "examples": [false, "${{ inputs.model-fallback }}"] | ||
| }, | ||
| "token-steering": { | ||
| "type": "boolean", | ||
| "description": "Enable or disable API proxy token steering. Set to false to preserve the explicitly configured provider and model.", | ||
| "examples": [false] | ||
| }, | ||
|
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. [/grill-with-docs] Consider using the same @copilot please address this. |
||
| "runtime": { | ||
| "type": "string", | ||
| "description": "Container runtime for the agent container. Use 'gvisor' to run the agent under gVisor's runsc runtime for additional kernel-level isolation. Use 'docker-sbx' to run the agent inside a Docker sbx microVM with KVM hypervisor-level isolation \u2014 requires sandbox.agent.sudo: true, DOCKER_PAT and DOCKER_USERNAME secrets, and a KVM-capable runner. Incompatible with runner.topology: arc-dind.", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,7 +279,7 @@ type AWFAPIProxyConfig struct { | |
| Enabled bool `json:"enabled"` | ||
|
|
||
| // EnableTokenSteering enables budget-warning system message injection near ET budget exhaustion. | ||
| EnableTokenSteering bool `json:"enableTokenSteering,omitempty"` | ||
| EnableTokenSteering *bool `json:"enableTokenSteering,omitempty"` | ||
|
|
||
| // MaxRuns is the maximum number of LLM invocations allowed for a run. | ||
| MaxRuns int `json:"maxRuns,omitempty"` | ||
|
|
@@ -553,21 +553,28 @@ func BuildAWFConfigJSON(config AWFCommandConfig) (string, error) { | |
| // value (-1) omits that budget from the AWF config and disables token steering. | ||
| // When maxAICredits is 0 (runtime default), token steering stays enabled here. | ||
| enableTokenSteering := maxAICredits >= 0 | ||
| if config.WorkflowData != nil && config.WorkflowData.SandboxConfig != nil && config.WorkflowData.SandboxConfig.Agent != nil && config.WorkflowData.SandboxConfig.Agent.TokenSteering != nil { | ||
|
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. Explicit 💡 DetailsBefore this PR, enableTokenSteering := maxAICredits >= 0
if ...SandboxConfig.Agent.TokenSteering != nil {
enableTokenSteering = *config.WorkflowData.SandboxConfig.Agent.TokenSteering
}
if maxAICredits < 0 {
maxAICredits = 0 // omitted from config
}If a workflow sets Suggested fix: either ignore/reject the override when |
||
| enableTokenSteering = *config.WorkflowData.SandboxConfig.Agent.TokenSteering | ||
| } | ||
|
Comment on lines
+556
to
+558
|
||
| if maxAICredits < 0 { | ||
| // Negative signals "disabled" — omit the budget from the AWF config. | ||
| maxAICredits = 0 | ||
| } | ||
| var tokenSteeringEnabled *bool | ||
| if awfSupportsTokenSteering(firewallConfig) && (enableTokenSteering || (config.WorkflowData != nil && config.WorkflowData.SandboxConfig != nil && config.WorkflowData.SandboxConfig.Agent != nil && config.WorkflowData.SandboxConfig.Agent.TokenSteering != nil)) { | ||
|
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. [/codebase-design] The nil-guard chain on line 564 is a verbatim copy of the one on line 556, making the emit logic hard to follow. A shared local variable would eliminate the duplication and clarify intent. 💡 Suggested refactorAfter line 557, capture the explicit-override flag once: agent := func() *AgentSandboxConfig {
if config.WorkflowData != nil && config.WorkflowData.SandboxConfig != nil {
return config.WorkflowData.SandboxConfig.Agent
}
return nil
}()
explicitlySet := agent != nil && agent.TokenSteering != nilThen lines 556–564 simplify to: if explicitlySet {
enableTokenSteering = *agent.TokenSteering
}
var tokenSteeringEnabled *bool
if awfSupportsTokenSteering(firewallConfig) && (enableTokenSteering || explicitlySet) {
tokenSteeringEnabled = &enableTokenSteering
}@copilot please address this.
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. The 4-level nil-check chain for 💡 Detailsif config.WorkflowData != nil && config.WorkflowData.SandboxConfig != nil && config.WorkflowData.SandboxConfig.Agent != nil && config.WorkflowData.SandboxConfig.Agent.TokenSteering != nil {This exact expression (or a near-duplicate) appears at line 556 and twice more within the func tokenSteeringOverride(wd *WorkflowData) *bool {
if wd == nil || wd.SandboxConfig == nil || wd.SandboxConfig.Agent == nil {
return nil
}
return wd.SandboxConfig.Agent.TokenSteering
}and reuse |
||
| tokenSteeringEnabled = &enableTokenSteering | ||
| } | ||
|
|
||
| apiProxy := &AWFAPIProxyConfig{ | ||
| Enabled: true, | ||
| MaxRuns: maxRuns, | ||
| MaxTurnCacheMisses: maxTurnCacheMisses, | ||
| MaxAICredits: maxAICredits, | ||
| EnableTokenSteering: enableTokenSteering && awfSupportsTokenSteering(firewallConfig), | ||
| EnableTokenSteering: tokenSteeringEnabled, | ||
| } | ||
|
|
||
| if !enableTokenSteering { | ||
|
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. [/diagnosing-bugs] The log message on line 577 says "Disabling" but that condition also fires when the sandbox override explicitly sets 💡 SuggestionDifferentiate the two disablement paths in the log: if !enableTokenSteering {
if explicitlySet {
awfConfigLog.Print("Disabling apiProxy.enableTokenSteering: sandbox.agent.token-steering=false")
} else {
awfConfigLog.Print("Disabling apiProxy.enableTokenSteering: max-ai-credits is negative")
}
}This keeps operational diagnostics actionable when users report unexpected behaviour. @copilot please address this. |
||
| awfConfigLog.Printf("Skipping apiProxy.enableTokenSteering: max-ai-credits is negative (disabled)") | ||
| awfConfigLog.Print("Disabling apiProxy.enableTokenSteering") | ||
| } else if !awfSupportsTokenSteering(firewallConfig) { | ||
| awfConfigLog.Printf("Skipping apiProxy.enableTokenSteering: AWF version %q requires at least %s", getAWFImageTag(firewallConfig), constants.AWFTokenSteeringMinVersion) | ||
|
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. When Consider emitting a warning when the user set the field explicitly but the version does not support it: } else if !awfSupportsTokenSteering(firewallConfig) {
if config.WorkflowData != nil && ... tokenSteering != nil {
awfConfigLog.Printf("Warning: sandbox.agent.token-steering is set but AWF version %q requires at least %s; setting ignored",
getAWFImageTag(firewallConfig), constants.AWFTokenSteeringMinVersion)
} else {
awfConfigLog.Printf("Skipping apiProxy.enableTokenSteering: ...")
}
}@copilot please address this. |
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -436,6 +436,27 @@ func TestBuildAWFConfigJSON(t *testing.T) { | |
| assert.Contains(t, jsonStr, `"enableTokenSteering":true`, "apiProxy should emit enableTokenSteering by default") | ||
| }) | ||
|
|
||
| t.Run("token steering can be disabled in the sandbox config", func(t *testing.T) { | ||
| disabled := false | ||
| config := AWFCommandConfig{ | ||
| EngineName: "copilot", | ||
| AllowedDomains: "github.com", | ||
| WorkflowData: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ID: "copilot"}, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{Enabled: true}, | ||
| }, | ||
| SandboxConfig: &SandboxConfig{ | ||
| Agent: &AgentSandboxConfig{TokenSteering: &disabled}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| jsonStr, err := BuildAWFConfigJSON(config) | ||
| require.NoError(t, err) | ||
|
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] The new test verifies the happy path (sandbox disables steering), but misses the interaction case: when 💡 Suggested additional test caset.Run("token steering sandbox override wins over positive max-ai-credits", func(t *testing.T) {
disabled := false
config := AWFCommandConfig{
EngineName: "copilot",
AllowedDomains: "github.com",
WorkflowData: &WorkflowData{
EngineConfig: &EngineConfig{ID: "copilot", MaxAICredits: 500},
NetworkPermissions: &NetworkPermissions{
Firewall: &FirewallConfig{Enabled: true},
},
SandboxConfig: &SandboxConfig{
Agent: &AgentSandboxConfig{TokenSteering: &disabled},
},
},
}
jsonStr, err := BuildAWFConfigJSON(config)
require.NoError(t, err)
assert.Contains(t, jsonStr, `"enableTokenSteering":false`)
})@copilot please address this. |
||
| assert.Contains(t, jsonStr, `"enableTokenSteering":false`, "apiProxy should emit the sandbox token-steering override") | ||
| }) | ||
|
|
||
| t.Run("token steering is disabled when max-ai-credits is negative", func(t *testing.T) { | ||
| config := AWFCommandConfig{ | ||
| EngineName: "copilot", | ||
|
|
||
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.
The example value should be
false, nottrue. The entire purpose of this field is to opt out of token steering —trueis already the default and does not need to be set explicitly. Showingtruehere will mislead readers.@copilot please address this.