From ff993f1bb009bc0505b9a2465e1966ef4f481625 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 18:31:58 +0000 Subject: [PATCH 1/3] Initial plan From ac55a0f8698728f3b643b105be21101ae789743b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 18:51:30 +0000 Subject: [PATCH 2/3] refactor: extract shared mount validation, relocate copilot engine functions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/copilot_engine.go | 11 ------- pkg/workflow/copilot_engine_execution.go | 38 ++++++++++++++++++++++- pkg/workflow/copilot_logs.go | 27 ---------------- pkg/workflow/mcp_config_validation.go | 11 +++---- pkg/workflow/sandbox_validation.go | 39 +++++++++--------------- pkg/workflow/validation_helpers.go | 19 ++++++++++++ 6 files changed, 76 insertions(+), 69 deletions(-) diff --git a/pkg/workflow/copilot_engine.go b/pkg/workflow/copilot_engine.go index c5972ae7655..8d9da7f8025 100644 --- a/pkg/workflow/copilot_engine.go +++ b/pkg/workflow/copilot_engine.go @@ -115,17 +115,6 @@ func (e *CopilotEngine) GetDeclaredOutputFiles() []string { return []string{logsFolder} } -// extractAddDirPaths extracts all directory paths from copilot args that follow --add-dir flags -func extractAddDirPaths(args []string) []string { - var dirs []string - for i := range len(args) - 1 { - if args[i] == "--add-dir" { - dirs = append(dirs, args[i+1]) - } - } - return dirs -} - // GetExecutionSteps is implemented in copilot_engine_execution.go // RenderMCPConfig is implemented in copilot_mcp.go diff --git a/pkg/workflow/copilot_engine_execution.go b/pkg/workflow/copilot_engine_execution.go index 9255530274e..5028ecaa951 100644 --- a/pkg/workflow/copilot_engine_execution.go +++ b/pkg/workflow/copilot_engine_execution.go @@ -366,4 +366,40 @@ COPILOT_CLI_INSTRUCTION="$(cat /tmp/gh-aw/aw-prompts/prompt.txt)" return steps } -// GetFirewallLogsCollectionStep returns the step for collecting firewall logs (before secret redaction) +// extractAddDirPaths extracts all directory paths from copilot args that follow --add-dir flags +func extractAddDirPaths(args []string) []string { + var dirs []string + for i := range len(args) - 1 { + if args[i] == "--add-dir" { + dirs = append(dirs, args[i+1]) + } + } + return dirs +} + +// generateCopilotSessionFileCopyStep generates a step to copy Copilot session state files +// from ~/.copilot/session-state/ to /tmp/gh-aw/sandbox/agent/logs/ +// This ensures session files are in /tmp/gh-aw/ where secret redaction can scan them +func generateCopilotSessionFileCopyStep() GitHubActionStep { + var step []string + + step = append(step, " - name: Copy Copilot session state files to logs") + step = append(step, " if: always()") + step = append(step, " continue-on-error: true") + step = append(step, " run: |") + step = append(step, " # Copy Copilot session state files to logs folder for artifact collection") + step = append(step, " # This ensures they are in /tmp/gh-aw/ where secret redaction can scan them") + step = append(step, " SESSION_STATE_DIR=\"$HOME/.copilot/session-state\"") + step = append(step, " LOGS_DIR=\"/tmp/gh-aw/sandbox/agent/logs\"") + step = append(step, " ") + step = append(step, " if [ -d \"$SESSION_STATE_DIR\" ]; then") + step = append(step, " echo \"Copying Copilot session state files from $SESSION_STATE_DIR to $LOGS_DIR\"") + step = append(step, " mkdir -p \"$LOGS_DIR\"") + step = append(step, " cp -v \"$SESSION_STATE_DIR\"/*.jsonl \"$LOGS_DIR/\" 2>/dev/null || true") + step = append(step, " echo \"Session state files copied successfully\"") + step = append(step, " else") + step = append(step, " echo \"No session-state directory found at $SESSION_STATE_DIR\"") + step = append(step, " fi") + + return GitHubActionStep(step) +} diff --git a/pkg/workflow/copilot_logs.go b/pkg/workflow/copilot_logs.go index 791de7ef2be..4c574d395de 100644 --- a/pkg/workflow/copilot_logs.go +++ b/pkg/workflow/copilot_logs.go @@ -472,30 +472,3 @@ func (e *CopilotEngine) GetCleanupStep(workflowData *WorkflowData) GitHubActionS // Return empty step - cleanup steps have been removed return GitHubActionStep([]string{}) } - -// generateCopilotSessionFileCopyStep generates a step to copy Copilot session state files -// from ~/.copilot/session-state/ to /tmp/gh-aw/sandbox/agent/logs/ -// This ensures session files are in /tmp/gh-aw/ where secret redaction can scan them -func generateCopilotSessionFileCopyStep() GitHubActionStep { - var step []string - - step = append(step, " - name: Copy Copilot session state files to logs") - step = append(step, " if: always()") - step = append(step, " continue-on-error: true") - step = append(step, " run: |") - step = append(step, " # Copy Copilot session state files to logs folder for artifact collection") - step = append(step, " # This ensures they are in /tmp/gh-aw/ where secret redaction can scan them") - step = append(step, " SESSION_STATE_DIR=\"$HOME/.copilot/session-state\"") - step = append(step, " LOGS_DIR=\"/tmp/gh-aw/sandbox/agent/logs\"") - step = append(step, " ") - step = append(step, " if [ -d \"$SESSION_STATE_DIR\" ]; then") - step = append(step, " echo \"Copying Copilot session state files from $SESSION_STATE_DIR to $LOGS_DIR\"") - step = append(step, " mkdir -p \"$LOGS_DIR\"") - step = append(step, " cp -v \"$SESSION_STATE_DIR\"/*.jsonl \"$LOGS_DIR/\" 2>/dev/null || true") - step = append(step, " echo \"Session state files copied successfully\"") - step = append(step, " else") - step = append(step, " echo \"No session-state directory found at $SESSION_STATE_DIR\"") - step = append(step, " fi") - - return GitHubActionStep(step) -} diff --git a/pkg/workflow/mcp_config_validation.go b/pkg/workflow/mcp_config_validation.go index 48d66dfcef4..04f375bcaca 100644 --- a/pkg/workflow/mcp_config_validation.go +++ b/pkg/workflow/mcp_config_validation.go @@ -313,12 +313,11 @@ func validateMCPMountsSyntax(toolName string, mountsRaw any) error { } for i, mount := range mounts { - parts := strings.Split(mount, ":") - if len(parts) != 3 { - return fmt.Errorf("tool '%s' mcp configuration mounts[%d] must follow 'source:destination:mode' format, got: %q.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, i, mount, toolName, constants.DocsToolsURL) - } - mode := parts[2] - if mode != "ro" && mode != "rw" { + source, dest, mode, err := validateMountStringFormat(mount) + if err != nil { + if source == "" && dest == "" && mode == "" { + return fmt.Errorf("tool '%s' mcp configuration mounts[%d] must follow 'source:destination:mode' format, got: %q.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, i, mount, toolName, constants.DocsToolsURL) + } return fmt.Errorf("tool '%s' mcp configuration mounts[%d] mode must be 'ro' or 'rw', got: %q.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\" # read-only\n - \"/host/path:/container/path:rw\" # read-write\n\nSee: %s", toolName, i, mode, toolName, constants.DocsToolsURL) } } diff --git a/pkg/workflow/sandbox_validation.go b/pkg/workflow/sandbox_validation.go index 360ce0b1d12..9ae96c95c5f 100644 --- a/pkg/workflow/sandbox_validation.go +++ b/pkg/workflow/sandbox_validation.go @@ -12,7 +12,6 @@ package workflow import ( "fmt" - "strings" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" @@ -24,23 +23,25 @@ var sandboxValidationLog = logger.New("workflow:sandbox_validation") // Expected format: "source:destination:mode" where mode is either "ro" or "rw" func validateMountsSyntax(mounts []string) error { for i, mount := range mounts { - // Split the mount string by colons - parts := strings.Split(mount, ":") - - // Must have exactly 3 parts: source, destination, mode - if len(parts) != 3 { + source, dest, mode, err := validateMountStringFormat(mount) + if err != nil { + // Distinguish format error (3-parts) from mode error + if source == "" && dest == "" && mode == "" { + return NewValidationError( + fmt.Sprintf("sandbox.mounts[%d]", i), + mount, + "mount syntax must follow 'source:destination:mode' format with exactly 3 colon-separated parts", + fmt.Sprintf("Use the format 'source:destination:mode'.\n\nExample:\nsandbox:\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", constants.DocsSandboxURL), + ) + } return NewValidationError( - fmt.Sprintf("sandbox.mounts[%d]", i), - mount, - "mount syntax must follow 'source:destination:mode' format with exactly 3 colon-separated parts", - fmt.Sprintf("Use the format 'source:destination:mode'.\n\nExample:\nsandbox:\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", constants.DocsSandboxURL), + fmt.Sprintf("sandbox.mounts[%d].mode", i), + mode, + "mount mode must be 'ro' (read-only) or 'rw' (read-write)", + fmt.Sprintf("Change the mount mode to either 'ro' or 'rw'.\n\nExample:\nsandbox:\n mounts:\n - \"/host/path:/container/path:ro\" # read-only\n - \"/host/path:/container/path:rw\" # read-write\n\nSee: %s", constants.DocsSandboxURL), ) } - source := parts[0] - dest := parts[1] - mode := parts[2] - // Validate that source and destination are not empty if source == "" { return NewValidationError( @@ -59,16 +60,6 @@ func validateMountsSyntax(mounts []string) error { ) } - // Validate mode is either "ro" or "rw" - if mode != "ro" && mode != "rw" { - return NewValidationError( - fmt.Sprintf("sandbox.mounts[%d].mode", i), - mode, - "mount mode must be 'ro' (read-only) or 'rw' (read-write)", - fmt.Sprintf("Change the mount mode to either 'ro' or 'rw'.\n\nExample:\nsandbox:\n mounts:\n - \"/host/path:/container/path:ro\" # read-only\n - \"/host/path:/container/path:rw\" # read-write\n\nSee: %s", constants.DocsSandboxURL), - ) - } - sandboxValidationLog.Printf("Validated mount %d: source=%s, dest=%s, mode=%s", i, source, dest, mode) } diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 740d55800fd..a31cbe0442f 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -13,6 +13,7 @@ // - ValidateInList() - Validates that a value is in an allowed list // - ValidatePositiveInt() - Validates that a value is a positive integer // - ValidateNonNegativeInt() - Validates that a value is a non-negative integer +// - validateMountStringFormat() - Parses and validates a "source:dest:mode" mount string // // # Design Rationale // @@ -28,6 +29,7 @@ package workflow import ( + "errors" "fmt" "slices" "strconv" @@ -146,3 +148,20 @@ func ValidateNonNegativeInt(field string, value int) error { } return nil } + +// validateMountStringFormat parses a mount string and validates its basic format. +// Expected format: "source:destination:mode" where mode is "ro" or "rw". +// Returns (source, dest, mode, nil) on success, or ("", "", "", error) on failure. +// The error message describes which aspect of the format is invalid. +// Callers are responsible for wrapping the error with context-appropriate error types. +func validateMountStringFormat(mount string) (source, dest, mode string, err error) { + parts := strings.Split(mount, ":") + if len(parts) != 3 { + return "", "", "", errors.New("must follow 'source:destination:mode' format with exactly 3 colon-separated parts") + } + mode = parts[2] + if mode != "ro" && mode != "rw" { + return parts[0], parts[1], parts[2], fmt.Errorf("mode must be 'ro' or 'rw', got %q", mode) + } + return parts[0], parts[1], parts[2], nil +} From 5ae971399867c4de730a4428c22692644d5e9337 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 19:02:00 +0000 Subject: [PATCH 3/3] test: add tests for validateMountStringFormat, validateMCPMountsSyntax, generateCopilotSessionFileCopyStep Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/copilot_engine_test.go | 22 ++++ pkg/workflow/mcp_config_validation_test.go | 126 +++++++++++++++++++++ pkg/workflow/validation_helpers_test.go | 80 +++++++++++++ 3 files changed, 228 insertions(+) create mode 100644 pkg/workflow/mcp_config_validation_test.go diff --git a/pkg/workflow/copilot_engine_test.go b/pkg/workflow/copilot_engine_test.go index 638366d9c56..553851931f4 100644 --- a/pkg/workflow/copilot_engine_test.go +++ b/pkg/workflow/copilot_engine_test.go @@ -1483,3 +1483,25 @@ func TestCopilotEnginePluginDiscoveryWithSRT(t *testing.T) { t.Errorf("Expected step to contain '--add-dir /home/runner/.copilot/' when plugins are declared with SRT enabled, but it was missing:\n%s", stepContent) } } + +// TestGenerateCopilotSessionFileCopyStep verifies the generated step copies session state files. +func TestGenerateCopilotSessionFileCopyStep(t *testing.T) { + step := generateCopilotSessionFileCopyStep() + content := strings.Join([]string(step), "\n") + + if !strings.Contains(content, "Copy Copilot session state files to logs") { + t.Error("Step should have a descriptive name") + } + if !strings.Contains(content, "always()") { + t.Error("Step should run always()") + } + if !strings.Contains(content, ".copilot/session-state") { + t.Error("Step should reference the Copilot session-state directory") + } + if !strings.Contains(content, "/tmp/gh-aw/sandbox/agent/logs") { + t.Error("Step should copy files into the gh-aw logs directory") + } + if !strings.Contains(content, "continue-on-error: true") { + t.Error("Step should be marked continue-on-error") + } +} diff --git a/pkg/workflow/mcp_config_validation_test.go b/pkg/workflow/mcp_config_validation_test.go new file mode 100644 index 00000000000..377291590b4 --- /dev/null +++ b/pkg/workflow/mcp_config_validation_test.go @@ -0,0 +1,126 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidateMCPMountsSyntax tests the MCP mount syntax validation function. +func TestValidateMCPMountsSyntax(t *testing.T) { + tests := []struct { + name string + toolName string + mountsRaw any + wantErr bool + errMsg string + }{ + { + name: "valid []string - ro mount", + toolName: "my-tool", + mountsRaw: []string{"/host/data:/data:ro"}, + wantErr: false, + }, + { + name: "valid []string - rw mount", + toolName: "my-tool", + mountsRaw: []string{"/host/data:/data:rw"}, + wantErr: false, + }, + { + name: "valid []any with string items", + toolName: "my-tool", + mountsRaw: []any{ + "/host/data:/data:ro", + "/usr/bin/tool:/usr/bin/tool:ro", + }, + wantErr: false, + }, + { + name: "empty []string", + toolName: "my-tool", + mountsRaw: []string{}, + wantErr: false, + }, + { + name: "invalid type — neither []any nor []string", + toolName: "my-tool", + mountsRaw: "not-an-array", + wantErr: true, + errMsg: "must be an array of strings", + }, + { + name: "invalid format — too few parts", + toolName: "my-tool", + mountsRaw: []string{"/host/path:/container/path"}, + wantErr: true, + errMsg: "must follow 'source:destination:mode' format", + }, + { + name: "invalid format — too many parts", + toolName: "my-tool", + mountsRaw: []string{"/host/path:/container/path:ro:extra"}, + wantErr: true, + errMsg: "must follow 'source:destination:mode' format", + }, + { + name: "invalid mode value", + toolName: "my-tool", + mountsRaw: []string{"/host/path:/container/path:invalid"}, + wantErr: true, + errMsg: "mode must be 'ro' or 'rw'", + }, + { + name: "invalid mode uppercase — case sensitive", + toolName: "my-tool", + mountsRaw: []string{"/host/path:/container/path:RO"}, + wantErr: true, + errMsg: "mode must be 'ro' or 'rw'", + }, + { + name: "error message includes tool name", + toolName: "special-tool", + mountsRaw: []string{"/host/path:/container/path"}, + wantErr: true, + errMsg: "special-tool", + }, + { + name: "error message includes mount index", + toolName: "my-tool", + mountsRaw: []string{ + "/host/data:/data:ro", + "/invalid/mount", + }, + wantErr: true, + errMsg: "mounts[1]", + }, + { + name: "[]any with non-string items are silently skipped", + toolName: "my-tool", + mountsRaw: []any{ + 123, // non-string, skipped + "/host/data:/data:ro", // valid string + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateMCPMountsSyntax(tt.toolName, tt.mountsRaw) + + if tt.wantErr { + require.Error(t, err, "expected an error") + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg, + "error message should contain %q", tt.errMsg) + } + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index c408704cece..b6ea6df8cb9 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -746,3 +746,83 @@ func TestDirExists(t *testing.T) { assert.True(t, result, "parent directory should return true") }) } + +// TestValidateMountStringFormat tests the shared mount format validation primitive. +func TestValidateMountStringFormat(t *testing.T) { + tests := []struct { + name string + mount string + wantErr bool + wantSrc string + wantDest string + wantMode string + allEmpty bool // true when format error (all three return values are empty) + }{ + { + name: "valid ro mount", + mount: "/host/data:/data:ro", + wantSrc: "/host/data", + wantDest: "/data", + wantMode: "ro", + }, + { + name: "valid rw mount", + mount: "/host/data:/data:rw", + wantSrc: "/host/data", + wantDest: "/data", + wantMode: "rw", + }, + { + name: "too few parts — format error, all values empty", + mount: "/host/path:/container/path", + wantErr: true, + allEmpty: true, + }, + { + name: "too many parts — format error, all values empty", + mount: "/host/path:/container/path:ro:extra", + wantErr: true, + allEmpty: true, + }, + { + name: "invalid mode — source and dest returned, mode returned", + mount: "/host/path:/container/path:xyz", + wantErr: true, + wantSrc: "/host/path", + wantDest: "/container/path", + wantMode: "xyz", + }, + { + name: "empty mode — source and dest returned, mode empty string", + mount: "/host/path:/container/path:", + wantErr: true, + wantSrc: "/host/path", + wantDest: "/container/path", + wantMode: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + src, dest, mode, err := validateMountStringFormat(tt.mount) + + if tt.wantErr { + require.Error(t, err, "expected an error for mount %q", tt.mount) + if tt.allEmpty { + assert.Empty(t, src, "source should be empty on format error") + assert.Empty(t, dest, "dest should be empty on format error") + assert.Empty(t, mode, "mode should be empty on format error") + } else { + assert.Equal(t, tt.wantSrc, src, "source mismatch") + assert.Equal(t, tt.wantDest, dest, "dest mismatch") + assert.Equal(t, tt.wantMode, mode, "mode mismatch") + } + } else { + require.NoError(t, err, "unexpected error for mount %q", tt.mount) + assert.Equal(t, tt.wantSrc, src, "source mismatch") + assert.Equal(t, tt.wantDest, dest, "dest mismatch") + assert.Equal(t, tt.wantMode, mode, "mode mismatch") + } + }) + } +}