-
Notifications
You must be signed in to change notification settings - Fork 418
Enforce sandbox-disable justification strings and surface new AWF import/safe-output constraints #38228
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
Enforce sandbox-disable justification strings and surface new AWF import/safe-output constraints #38228
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 |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
|
|
@@ -9,6 +10,8 @@ import ( | |
|
|
||
| var networkFirewallCodemodLog = logger.New("cli:codemod_network_firewall") | ||
|
|
||
| const migratedSandboxDisableJustification = "migrated from deprecated network.firewall disable setting" | ||
|
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 💡 Suggested cross-validation testfunc TestMigratedJustificationPassesValidation(t *testing.T) {
data := &workflow.WorkflowData{
Features: map[string]any{
"dangerously-disable-sandbox-agent": migratedSandboxDisableJustification,
},
SandboxConfig: &workflow.SandboxConfig{
Agent: &workflow.AgentSandboxConfig{Disabled: true},
},
}
// This would require exporting getSandboxDisableJustification or
// testing via the full compiler path:
require.NoError(t, compiler.CompileWorkflow(workflowWithMigratedJustification))
}Alternatively, a simple length assertion in a package test is enough to lock the contract. |
||
|
|
||
| // getNetworkFirewallCodemod creates a codemod for migrating network.firewall to sandbox.agent | ||
| func getNetworkFirewallCodemod() Codemod { | ||
| return newFieldRemovalCodemod(fieldRemovalCodemodConfig{ | ||
|
|
@@ -21,26 +24,44 @@ func getNetworkFirewallCodemod() Codemod { | |
| LogMsg: "Applied network.firewall migration (firewall now always enabled via sandbox.agent: awf default)", | ||
| Log: networkFirewallCodemodLog, | ||
| PostTransform: func(lines []string, frontmatter map[string]any, fieldValue any) []string { | ||
| requiresDisableFlag := requiresSandboxDisableFeatureFlag(fieldValue) | ||
| _, hasSandbox := frontmatter["sandbox"] | ||
|
|
||
| if !hasSandbox { | ||
| sandboxLines := sandboxAgentLinesFromFirewall(fieldValue) | ||
| if len(sandboxLines) > 0 { | ||
| lines = insertSandboxAfterNetworkBlock(lines, sandboxLines) | ||
| if requiresDisableFlag { | ||
| lines = ensureSandboxDisableFeatureFlag(lines) | ||
| } | ||
| networkFirewallCodemodLog.Print("Converted deprecated network.firewall to sandbox.agent") | ||
| } | ||
| return lines | ||
| } | ||
|
|
||
| lines, merged := mergeFirewallIntoExistingSandbox(lines, fieldValue) | ||
| if merged { | ||
| if requiresDisableFlag { | ||
| lines = ensureSandboxDisableFeatureFlag(lines) | ||
| } | ||
| networkFirewallCodemodLog.Print("Merged deprecated network.firewall into existing sandbox.agent") | ||
| } | ||
| return lines | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| func requiresSandboxDisableFeatureFlag(fieldValue any) bool { | ||
| switch value := fieldValue.(type) { | ||
| case bool: | ||
| return !value | ||
| case string: | ||
| return strings.EqualFold(strings.TrimSpace(value), "disable") | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func sandboxAgentLinesFromFirewall(fieldValue any) []string { | ||
| switch value := fieldValue.(type) { | ||
| case bool: | ||
|
|
@@ -260,3 +281,59 @@ func indentLines(lines []string, indent string) []string { | |
| } | ||
| return indented | ||
| } | ||
|
|
||
| func ensureSandboxDisableFeatureFlag(lines []string) []string { | ||
| featureKey := "dangerously-disable-sandbox-agent:" | ||
|
|
||
| featuresIdx := -1 | ||
| featuresIndent := "" | ||
| featuresEnd := len(lines) | ||
| for i, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| if isTopLevelKey(line) && strings.HasPrefix(trimmed, "features:") { | ||
| featuresIdx = i | ||
| featuresIndent = getIndentation(line) | ||
| for j := i + 1; j < len(lines); j++ { | ||
| if isTopLevelKey(lines[j]) { | ||
| featuresEnd = j | ||
| break | ||
| } | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if featuresIdx >= 0 { | ||
| featureIndent := featuresIndent + " " | ||
| for i := featuresIdx + 1; i < featuresEnd; i++ { | ||
| if strings.HasPrefix(strings.TrimSpace(lines[i]), featureKey) { | ||
| return lines | ||
| } | ||
|
Comment on lines
+308
to
+311
|
||
| } | ||
| featureLine := fmt.Sprintf("%s%s %s", featureIndent, featureKey, strconv.Quote(migratedSandboxDisableJustification)) | ||
| newLines := make([]string, 0, len(lines)+1) | ||
| newLines = append(newLines, lines[:featuresEnd]...) | ||
| newLines = append(newLines, featureLine) | ||
| newLines = append(newLines, lines[featuresEnd:]...) | ||
| return newLines | ||
| } | ||
|
|
||
| insertIndex := len(lines) | ||
| for i, line := range lines { | ||
| if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "sandbox:") { | ||
| insertIndex = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| featureLines := []string{ | ||
| "features:", | ||
| " " + featureKey + " " + strconv.Quote(migratedSandboxDisableJustification), | ||
| } | ||
|
|
||
| newLines := make([]string, 0, len(lines)+len(featureLines)) | ||
| newLines = append(newLines, lines[:insertIndex]...) | ||
| newLines = append(newLines, featureLines...) | ||
| newLines = append(newLines, lines[insertIndex:]...) | ||
| return newLines | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,7 @@ permissions: | |
| assert.NotContains(t, result, "firewall:", "Should remove firewall field") | ||
| assert.Contains(t, result, "sandbox:", "Should add sandbox block") | ||
| assert.Contains(t, result, "agent: false", "Should convert firewall false to sandbox.agent: false") | ||
| assert.Contains(t, result, `dangerously-disable-sandbox-agent: "migrated from deprecated network.firewall disable setting"`, "Should add required sandbox-disable justification feature") | ||
| } | ||
|
|
||
| func TestNetworkFirewallCodemod_NoNetworkField(t *testing.T) { | ||
|
|
@@ -208,6 +209,43 @@ sandbox: | |
| assert.Contains(t, result, "sandbox:", "Should preserve existing sandbox block") | ||
| assert.Contains(t, result, "mcp: true", "Should preserve existing sandbox settings") | ||
| assert.Contains(t, result, "agent: false", "Should migrate firewall false to sandbox.agent: false") | ||
| assert.Contains(t, result, `dangerously-disable-sandbox-agent: "migrated from deprecated network.firewall disable setting"`, "Should add required sandbox-disable justification feature") | ||
| } | ||
|
|
||
| func TestNetworkFirewallCodemod_PreservesExistingSandboxDisableJustification(t *testing.T) { | ||
| codemod := getNetworkFirewallCodemod() | ||
|
|
||
| content := `--- | ||
| on: workflow_dispatch | ||
| network: | ||
| firewall: false | ||
| features: | ||
| dangerously-disable-sandbox-agent: "already documented justification string with enough detail" | ||
| sandbox: | ||
| mcp: true | ||
| --- | ||
|
|
||
| # Test Workflow` | ||
|
|
||
| frontmatter := map[string]any{ | ||
| "on": "workflow_dispatch", | ||
| "network": map[string]any{ | ||
| "firewall": false, | ||
| }, | ||
| "features": map[string]any{ | ||
| "dangerously-disable-sandbox-agent": "already documented justification string with enough detail", | ||
| }, | ||
| "sandbox": map[string]any{ | ||
| "mcp": true, | ||
| }, | ||
| } | ||
|
|
||
| result, applied, err := codemod.Apply(content, frontmatter) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.True(t, applied) | ||
| assert.Contains(t, result, `dangerously-disable-sandbox-agent: "already documented justification string with enough detail"`) | ||
| assert.NotContains(t, result, migratedSandboxDisableJustification, "Should not overwrite existing justification") | ||
|
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] There is no test for when 💡 Suggested test scenarioAdd a test where the content includes: features:
some-other-flag: truebut not
Without this, a line-index off-by-one in |
||
| } | ||
|
|
||
| func TestNetworkFirewallCodemod_MigratesFirewallVersionIntoExistingSandbox(t *testing.T) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,6 +174,69 @@ This is a test workflow with multiple imports. | |
| } | ||
| } | ||
|
|
||
| func TestCompileWorkflowWithConditionalImport(t *testing.T) { | ||
| tempDir := testutil.TempDir(t, "test-*") | ||
|
|
||
| sharedPath := filepath.Join(tempDir, "shared-conditional.md") | ||
| sharedContent := `--- | ||
| steps: | ||
| - name: Conditional Imported Step | ||
| run: echo "from import" | ||
| --- | ||
|
|
||
| Imported conditional instructions. | ||
| ` | ||
| if err := os.WriteFile(sharedPath, []byte(sharedContent), 0644); err != nil { | ||
| t.Fatalf("Failed to write shared conditional file: %v", err) | ||
| } | ||
|
|
||
| workflowPath := filepath.Join(tempDir, "test-workflow.md") | ||
| workflowContent := `--- | ||
| on: issues | ||
| permissions: | ||
| contents: read | ||
| issues: read | ||
| pull-requests: read | ||
| engine: copilot | ||
| experiments: | ||
| strategy: [eager, lazy] | ||
| imports: | ||
| - path: shared-conditional.md | ||
| if: "experiments.strategy == 'eager'" | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| Main workflow body. | ||
| ` | ||
| if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { | ||
| t.Fatalf("Failed to write workflow file: %v", err) | ||
| } | ||
|
|
||
| compiler := workflow.NewCompiler() | ||
| if err := compiler.CompileWorkflow(workflowPath); err != nil { | ||
| t.Fatalf("CompileWorkflow failed: %v", err) | ||
| } | ||
|
|
||
| lockFilePath := stringutil.MarkdownToLockFile(workflowPath) | ||
| lockFileContent, err := os.ReadFile(lockFilePath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
|
|
||
| compiled := string(lockFileContent) | ||
| assertions := []string{ | ||
| `{{#if experiments.strategy == "eager"}}`, | ||
| "{{/if}}", | ||
| "needs.activation.outputs.strategy == 'eager'", | ||
| } | ||
|
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] These assertions verify that the template guard syntax is present in the compiled output, but not that the imported step content is wrapped inside the 💡 Stronger structural assertionifIdx := strings.Index(compiled, `{{#if experiments.strategy == "eager"}}`)
endifIdx := strings.Index(compiled, "{{/if}}")
stepIdx := strings.Index(compiled, `echo "from import"`)
require.Greater(t, ifIdx, -1, "{{#if}} guard should be present")
require.Greater(t, endifIdx, -1, "{{/if}} guard should be present")
require.Greater(t, stepIdx, ifIdx, "imported step should appear after {{#if}} guard")
require.Less(t, stepIdx, endifIdx, "imported step should appear before {{/if}} guard")This turns a structural smoke test into a real gating assertion. |
||
| for _, expected := range assertions { | ||
| if !strings.Contains(compiled, expected) { | ||
| t.Errorf("Expected compiled workflow to contain %q", expected) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestCompileWorkflowWithMCPServersImport(t *testing.T) { | ||
| // Create a temporary directory for test files | ||
| tempDir := testutil.TempDir(t, "test-*") | ||
|
|
||
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.
[/grill-with-docs] Wording inconsistency: "the dangerous-disable justification feature" does not match the actual feature key
dangerously-disable-sandbox-agent. A reader skimming this doc will not know what key to write.Suggest replacing with: "you must provide the
dangerously-disable-sandbox-agentjustification:"