diff --git a/pkg/workflow/mcp_benchmark_test.go b/pkg/workflow/mcp_benchmark_test.go index ad398b3451a..ef5535775ea 100644 --- a/pkg/workflow/mcp_benchmark_test.go +++ b/pkg/workflow/mcp_benchmark_test.go @@ -3,50 +3,9 @@ package workflow import ( - "strings" "testing" ) -// BenchmarkRenderPlaywrightMCPConfig benchmarks Playwright MCP config generation -func BenchmarkRenderPlaywrightMCPConfig(b *testing.B) { - playwrightTool := map[string]any{ - "container": "mcr.microsoft.com/playwright:v1.41.0", - "args": []any{"--debug"}, - } - playwrightConfig := parsePlaywrightTool(playwrightTool) - - for b.Loop() { - var yaml strings.Builder - renderPlaywrightMCPConfig(&yaml, playwrightConfig, true) - } -} - -// BenchmarkGeneratePlaywrightDockerArgs benchmarks Playwright args generation -func BenchmarkGeneratePlaywrightDockerArgs(b *testing.B) { - playwrightTool := map[string]any{ - "container": "mcr.microsoft.com/playwright:v1.41.0", - } - playwrightConfig := parsePlaywrightTool(playwrightTool) - - for b.Loop() { - _ = generatePlaywrightDockerArgs(playwrightConfig) - } -} - -// BenchmarkRenderPlaywrightMCPConfig_Complex benchmarks complex Playwright config -func BenchmarkRenderPlaywrightMCPConfig_Complex(b *testing.B) { - playwrightTool := map[string]any{ - "container": "mcr.microsoft.com/playwright:v1.41.0", - "args": []any{"--debug", "--timeout", "30000"}, - } - playwrightConfig := parsePlaywrightTool(playwrightTool) - - for b.Loop() { - var yaml strings.Builder - renderPlaywrightMCPConfig(&yaml, playwrightConfig, true) - } -} - // BenchmarkExtractExpressionsFromPlaywrightArgs benchmarks expression extraction func BenchmarkExtractExpressionsFromPlaywrightArgs(b *testing.B) { customArgs := []string{"--debug", "--timeout", "${{ github.event.inputs.timeout }}"} diff --git a/pkg/workflow/mcp_config_builtin.go b/pkg/workflow/mcp_config_builtin.go index 0b0bcac2673..3770f9c96bc 100644 --- a/pkg/workflow/mcp_config_builtin.go +++ b/pkg/workflow/mcp_config_builtin.go @@ -108,13 +108,6 @@ import ( var mcpBuiltinLog = logger.New("workflow:mcp-config-builtin") -// renderSafeOutputsMCPConfig generates the Safe Outputs MCP server configuration -// This is a shared function used by both Claude and Custom engines -func renderSafeOutputsMCPConfig(yaml *strings.Builder, isLast bool, workflowData *WorkflowData) { - mcpBuiltinLog.Print("Rendering Safe Outputs MCP configuration") - renderSafeOutputsMCPConfigWithOptions(yaml, isLast, false, workflowData) -} - // renderSafeOutputsMCPConfigWithOptions generates the Safe Outputs MCP server configuration with engine-specific options // Now uses HTTP transport instead of stdio, similar to safe-inputs // The server is started in a separate step before the agent job diff --git a/pkg/workflow/mcp_config_custom.go b/pkg/workflow/mcp_config_custom.go index 8f557bba610..319347aa271 100644 --- a/pkg/workflow/mcp_config_custom.go +++ b/pkg/workflow/mcp_config_custom.go @@ -16,32 +16,6 @@ import ( var mcpCustomLog = logger.New("workflow:mcp-config-custom") -// renderCustomMCPConfigWrapper generates custom MCP server configuration wrapper -// This is a shared function used by both Claude and Custom engines -func renderCustomMCPConfigWrapper(yaml *strings.Builder, toolName string, toolConfig map[string]any, isLast bool) error { - mcpCustomLog.Printf("Rendering custom MCP config wrapper for tool: %s", toolName) - fmt.Fprintf(yaml, " \"%s\": {\n", toolName) - - // Use the shared MCP config renderer with JSON format - renderer := MCPConfigRenderer{ - IndentLevel: " ", - Format: "json", - } - - err := renderSharedMCPConfig(yaml, toolName, toolConfig, renderer) - if err != nil { - return err - } - - if isLast { - yaml.WriteString(" }\n") - } else { - yaml.WriteString(" },\n") - } - - return nil -} - // renderCustomMCPConfigWrapperWithContext generates custom MCP server configuration wrapper with workflow context // This version includes workflowData to determine if localhost URLs should be rewritten func renderCustomMCPConfigWrapperWithContext(yaml *strings.Builder, toolName string, toolConfig map[string]any, isLast bool, workflowData *WorkflowData) error { diff --git a/pkg/workflow/mcp_config_playwright_renderer.go b/pkg/workflow/mcp_config_playwright_renderer.go index 4b8962cba38..9bec9c325c4 100644 --- a/pkg/workflow/mcp_config_playwright_renderer.go +++ b/pkg/workflow/mcp_config_playwright_renderer.go @@ -65,14 +65,6 @@ import ( var mcpPlaywrightLog = logger.New("workflow:mcp_config_playwright_renderer") -// renderPlaywrightMCPConfig generates the Playwright MCP server configuration -// Uses Docker container to launch Playwright MCP for consistent browser environment -// This is a shared function used by both Claude and Custom engines -func renderPlaywrightMCPConfig(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig, isLast bool) { - mcpPlaywrightLog.Print("Rendering Playwright MCP configuration") - renderPlaywrightMCPConfigWithOptions(yaml, playwrightConfig, isLast, false, false) -} - // renderPlaywrightMCPConfigWithOptions generates the Playwright MCP server configuration with engine-specific options // Per MCP Gateway Specification v1.0.0 section 3.2.1, stdio-based MCP servers MUST be containerized. // Uses MCP Gateway spec format: container, entrypointArgs, mounts, and args fields. diff --git a/pkg/workflow/mcp_config_shared_test.go b/pkg/workflow/mcp_config_shared_test.go index da981f151aa..866b5220292 100644 --- a/pkg/workflow/mcp_config_shared_test.go +++ b/pkg/workflow/mcp_config_shared_test.go @@ -7,144 +7,6 @@ import ( "testing" ) -// TestRenderSafeOutputsMCPConfigShared tests the shared renderSafeOutputsMCPConfig function -func TestRenderSafeOutputsMCPConfigShared(t *testing.T) { - tests := []struct { - name string - isLast bool - wantContains []string - wantEnding string - }{ - { - name: "safe outputs config not last", - isLast: false, - wantContains: []string{ - `"safeoutputs": {`, - `"type": "http"`, - `"url": "http://host.docker.internal:$GH_AW_SAFE_OUTPUTS_PORT"`, - `"headers": {`, - `"Authorization": "$GH_AW_SAFE_OUTPUTS_API_KEY"`, - }, - wantEnding: "},\n", - }, - { - name: "safe outputs config is last", - isLast: true, - wantContains: []string{ - `"safeoutputs": {`, - `"type": "http"`, - `"url": "http://host.docker.internal:$GH_AW_SAFE_OUTPUTS_PORT"`, - }, - wantEnding: "}\n", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var yaml strings.Builder - renderSafeOutputsMCPConfig(&yaml, tt.isLast, nil) - - result := yaml.String() - - // Check all required strings are present - for _, want := range tt.wantContains { - if !strings.Contains(result, want) { - t.Errorf("renderSafeOutputsMCPConfig() result missing %q\nGot:\n%s", want, result) - } - } - - // Check correct ending - if !strings.HasSuffix(result, tt.wantEnding) { - // Show last part of result for debugging, but handle short strings - endSnippet := result - if len(result) > 10 { - endSnippet = result[len(result)-10:] - } - t.Errorf("renderSafeOutputsMCPConfig() ending = %q, want suffix %q", endSnippet, tt.wantEnding) - } - }) - } -} - -// TestRenderCustomMCPConfigWrapperShared tests the shared renderCustomMCPConfigWrapper function -func TestRenderCustomMCPConfigWrapperShared(t *testing.T) { - tests := []struct { - name string - toolName string - toolConfig map[string]any - isLast bool - wantContains []string - wantEnding string - wantError bool - }{ - { - name: "custom MCP config not last", - toolName: "my-tool", - toolConfig: map[string]any{ - "command": "node", - "args": []string{"server.js"}, - }, - isLast: false, - wantContains: []string{ - `"my-tool": {`, - `"command": "node"`, - }, - wantEnding: "},\n", - wantError: false, - }, - { - name: "custom MCP config is last", - toolName: "another-tool", - toolConfig: map[string]any{ - "command": "python", - "args": []string{"-m", "server"}, - }, - isLast: true, - wantContains: []string{ - `"another-tool": {`, - `"command": "python"`, - }, - wantEnding: "}\n", - wantError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var yaml strings.Builder - err := renderCustomMCPConfigWrapper(&yaml, tt.toolName, tt.toolConfig, tt.isLast) - - if (err != nil) != tt.wantError { - t.Errorf("renderCustomMCPConfigWrapper() error = %v, wantError %v", err, tt.wantError) - return - } - - if tt.wantError { - return - } - - result := yaml.String() - - // Check all required strings are present - for _, want := range tt.wantContains { - if !strings.Contains(result, want) { - t.Errorf("renderCustomMCPConfigWrapper() result missing %q\nGot:\n%s", want, result) - } - } - - // Check correct ending - if !strings.HasSuffix(result, tt.wantEnding) { - // Show last part of result for debugging, but handle short strings - endSnippet := result - if len(result) > 10 { - endSnippet = result[len(result)-10:] - } - t.Errorf("renderCustomMCPConfigWrapper() ending = %q, want suffix %q", endSnippet, tt.wantEnding) - } - }) - } -} - // TestEngineMethodsDelegateToShared ensures engine methods properly delegate to shared functions func TestEngineMethodsDelegateToShared(t *testing.T) { t.Run("Claude engine Playwright delegation via unified renderer", func(t *testing.T) { diff --git a/pkg/workflow/mcp_config_utils_test.go b/pkg/workflow/mcp_config_utils_test.go index bf6e92c5af3..e2aabd44de8 100644 --- a/pkg/workflow/mcp_config_utils_test.go +++ b/pkg/workflow/mcp_config_utils_test.go @@ -7,113 +7,6 @@ import ( "testing" ) -func TestGetTypeString(t *testing.T) { - tests := []struct { - name string - value any - want string - }{ - { - name: "nil value", - value: nil, - want: "null", - }, - { - name: "int value", - value: 42, - want: "number", - }, - { - name: "int64 value", - value: int64(100), - want: "number", - }, - { - name: "float64 value", - value: 3.14, - want: "number", - }, - { - name: "float32 value", - value: float32(2.71), - want: "number", - }, - { - name: "boolean true", - value: true, - want: "boolean", - }, - { - name: "boolean false", - value: false, - want: "boolean", - }, - { - name: "string value", - value: "hello world", - want: "string", - }, - { - name: "empty string", - value: "", - want: "string", - }, - { - name: "object (map[string]any)", - value: map[string]any{ - "key": "value", - }, - want: "object", - }, - { - name: "empty object", - value: map[string]any{}, - want: "object", - }, - { - name: "array of strings", - value: []string{"a", "b", "c"}, - want: "array", - }, - { - name: "array of ints", - value: []int{1, 2, 3}, - want: "array", - }, - { - name: "array of any", - value: []any{"mixed", 123, true}, - want: "array", - }, - { - name: "empty array", - value: []string{}, - want: "array", - }, - { - name: "array of objects", - value: []map[string]any{{"key": "value"}}, - want: "array", - }, - { - name: "unknown type (struct)", - value: struct { - Name string - }{Name: "test"}, - want: "unknown", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := getTypeString(tt.value) - if got != tt.want { - t.Errorf("getTypeString(%v) = %v, want %v", tt.value, got, tt.want) - } - }) - } -} - func TestWriteArgsToYAMLInline(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/mcp_config_validation.go b/pkg/workflow/mcp_config_validation.go index 833b85e35e2..d8e03e69c5c 100644 --- a/pkg/workflow/mcp_config_validation.go +++ b/pkg/workflow/mcp_config_validation.go @@ -171,31 +171,6 @@ func getRawMCPConfig(toolConfig map[string]any) (map[string]any, error) { return result, nil } -// getTypeString returns a human-readable type name for error messages -func getTypeString(value any) string { - if value == nil { - return "null" - } - - switch value.(type) { - case int, int64, float64, float32: - return "number" - case bool: - return "boolean" - case map[string]any: - return "object" - case string: - return "string" - default: - // Check if it's any kind of slice/array by examining the type string - typeStr := fmt.Sprintf("%T", value) - if strings.HasPrefix(typeStr, "[]") { - return "array" - } - return "unknown" - } -} - // validateStringProperty validates that a property is a string and returns appropriate error message func validateStringProperty(toolName, propertyName string, value any, exists bool) error { if !exists { diff --git a/pkg/workflow/mcp_playwright_config.go b/pkg/workflow/mcp_playwright_config.go index 510fc3067a2..3193af55ef1 100644 --- a/pkg/workflow/mcp_playwright_config.go +++ b/pkg/workflow/mcp_playwright_config.go @@ -21,16 +21,6 @@ func getPlaywrightDockerImageVersion(playwrightConfig *PlaywrightToolConfig) str return playwrightDockerImageVersion } -// generatePlaywrightDockerArgs creates the common Docker arguments for Playwright MCP server -func generatePlaywrightDockerArgs(playwrightConfig *PlaywrightToolConfig) PlaywrightDockerArgs { - args := PlaywrightDockerArgs{ - ImageVersion: getPlaywrightDockerImageVersion(playwrightConfig), - MCPPackageVersion: string(constants.DefaultPlaywrightMCPVersion), - } - log.Printf("Playwright Docker args: image_version=%s, mcp_package_version=%s", args.ImageVersion, args.MCPPackageVersion) - return args -} - // extractExpressionsFromPlaywrightArgs extracts all GitHub Actions expressions from playwright arguments // Returns a map of environment variable names to their original expressions // Uses the same ExpressionExtractor as used for shell script security diff --git a/pkg/workflow/repo_memory.go b/pkg/workflow/repo_memory.go index f57ad29235f..6a41ff415b9 100644 --- a/pkg/workflow/repo_memory.go +++ b/pkg/workflow/repo_memory.go @@ -552,98 +552,6 @@ func generateRepoMemoryArtifactUpload(builder *strings.Builder, data *WorkflowDa } } -// generateRepoMemoryPushSteps generates steps to push changes back to the repo-memory branches -// This runs at the end of the workflow (always condition) to persist any changes made -func generateRepoMemoryPushSteps(builder *strings.Builder, data *WorkflowData) { - if data.RepoMemoryConfig == nil || len(data.RepoMemoryConfig.Memories) == 0 { - return - } - - repoMemoryLog.Printf("Generating repo-memory push steps for %d memories", len(data.RepoMemoryConfig.Memories)) - - builder.WriteString(" # Push repo memory changes back to git branches\n") - - for _, memory := range data.RepoMemoryConfig.Memories { - // Determine the target repository - targetRepo := memory.TargetRepo - if targetRepo == "" { - targetRepo = "${{ github.repository }}" - } - - // Determine the memory directory - memoryDir := "/tmp/gh-aw/repo-memory/" + memory.ID - - // Step: Push changes to repo-memory branch - if memory.Wiki { - fmt.Fprintf(builder, " - name: Push wiki-memory changes (%s)\n", memory.ID) - } else { - fmt.Fprintf(builder, " - name: Push repo-memory changes (%s)\n", memory.ID) - } - builder.WriteString(" if: always()\n") - builder.WriteString(" env:\n") - builder.WriteString(" GH_TOKEN: ${{ github.token }}\n") - builder.WriteString(" GITHUB_SERVER_URL: ${{ github.server_url }}\n") - builder.WriteString(" run: |\n") - builder.WriteString(" set -e\n") - fmt.Fprintf(builder, " cd \"%s\" || exit 0\n", memoryDir) - builder.WriteString(" \n") - builder.WriteString(" # Extract host from server URL (remove https:// prefix)\n") - builder.WriteString(" SERVER_HOST=\"${GITHUB_SERVER_URL#https://}\"\n") - builder.WriteString(" \n") - builder.WriteString(" # Check if we have any changes to commit\n") - builder.WriteString(" if [ -n \"$(git status --porcelain)\" ]; then\n") - builder.WriteString(" echo \"Changes detected in repo memory, committing and pushing...\"\n") - builder.WriteString(" \n") - - // Add file validation if constraints are specified - if len(memory.FileGlob) > 0 || memory.MaxFileSize > 0 || memory.MaxFileCount > 0 { - builder.WriteString(" # Validate files before committing\n") - - if memory.MaxFileSize > 0 { - fmt.Fprintf(builder, " # Check file sizes (max: %d bytes)\n", memory.MaxFileSize) - fmt.Fprintf(builder, " if find . -type f -size +%dc | grep -q .; then\n", memory.MaxFileSize) - builder.WriteString(" echo \"Error: Files exceed maximum size limit\"\n") - fmt.Fprintf(builder, " find . -type f -size +%dc -exec ls -lh {} \\;\n", memory.MaxFileSize) - builder.WriteString(" exit 1\n") - builder.WriteString(" fi\n") - builder.WriteString(" \n") - } - - if memory.MaxFileCount > 0 { - fmt.Fprintf(builder, " # Check file count (max: %d files)\n", memory.MaxFileCount) - builder.WriteString(" FILE_COUNT=$(git status --porcelain | wc -l)\n") - fmt.Fprintf(builder, " if [ \"$FILE_COUNT\" -gt %d ]; then\n", memory.MaxFileCount) - fmt.Fprintf(builder, " echo \"Error: Too many files to commit ($FILE_COUNT > %d)\"\n", memory.MaxFileCount) - builder.WriteString(" exit 1\n") - builder.WriteString(" fi\n") - builder.WriteString(" \n") - } - } - - builder.WriteString(" # Add all changes\n") - builder.WriteString(" git add -A\n") - builder.WriteString(" \n") - builder.WriteString(" # Commit changes\n") - builder.WriteString(" git commit -m \"Update memory from workflow run ${{ github.run_id }}\"\n") - builder.WriteString(" \n") - builder.WriteString(" # Pull with ours merge strategy (our changes win in conflicts)\n") - builder.WriteString(" set +e\n") - fmt.Fprintf(builder, " git pull --no-rebase -s recursive -X ours \"https://x-access-token:${GH_TOKEN}@${SERVER_HOST}/%s.git\" \"%s\" 2>&1\n", - targetRepo, memory.BranchName) - builder.WriteString(" PULL_EXIT_CODE=$?\n") - builder.WriteString(" set -e\n") - builder.WriteString(" \n") - builder.WriteString(" # Push changes (force push if needed due to conflict resolution)\n") - fmt.Fprintf(builder, " git push \"https://x-access-token:${GH_TOKEN}@${SERVER_HOST}/%s.git\" \"HEAD:%s\"\n", - targetRepo, memory.BranchName) - builder.WriteString(" \n") - builder.WriteString(" echo \"Successfully pushed changes to repo memory\"\n") - builder.WriteString(" else\n") - builder.WriteString(" echo \"No changes in repo memory, skipping push\"\n") - builder.WriteString(" fi\n") - } -} - // generateRepoMemorySteps generates git steps for the repo-memory configuration func generateRepoMemorySteps(builder *strings.Builder, data *WorkflowData) { if data.RepoMemoryConfig == nil || len(data.RepoMemoryConfig.Memories) == 0 { diff --git a/pkg/workflow/repo_memory_test.go b/pkg/workflow/repo_memory_test.go index bd4ddebcdc7..dddebc6291f 100644 --- a/pkg/workflow/repo_memory_test.go +++ b/pkg/workflow/repo_memory_test.go @@ -245,63 +245,6 @@ func TestRepoMemoryStepsGeneration(t *testing.T) { } } -// TestRepoMemoryPushStepsGeneration tests that push steps are generated correctly -func TestRepoMemoryPushStepsGeneration(t *testing.T) { - config := &RepoMemoryConfig{ - Memories: []RepoMemoryEntry{ - { - ID: "default", - BranchName: "memory/default", - MaxFileSize: 10240, - MaxFileCount: 100, - }, - }, - } - - data := &WorkflowData{ - RepoMemoryConfig: config, - } - - var builder strings.Builder - generateRepoMemoryPushSteps(&builder, data) - - output := builder.String() - - // Check for push step - if !strings.Contains(output, "Push repo-memory changes (default)") { - t.Error("Expected push step for repo-memory") - } - - // Check for if: always() - if !strings.Contains(output, "if: always()") { - t.Error("Expected always() condition") - } - - // Check for git commit - if !strings.Contains(output, "git commit") { - t.Error("Expected git commit command") - } - - // Check for git push - if !strings.Contains(output, "git push") { - t.Error("Expected git push command") - } - - // Check for merge strategy - if !strings.Contains(output, "-X ours") { - t.Error("Expected ours merge strategy") - } - - // Check for validation - if !strings.Contains(output, "Check file sizes") { - t.Error("Expected file size validation") - } - - if !strings.Contains(output, "Check file count") { - t.Error("Expected file count validation") - } -} - // TestRepoMemoryPromptGeneration tests that prompt section is generated correctly func TestRepoMemoryPromptGeneration(t *testing.T) { config := &RepoMemoryConfig{ diff --git a/pkg/workflow/safe_inputs_generator.go b/pkg/workflow/safe_inputs_generator.go index 5d89efad142..1b4b0c3efaf 100644 --- a/pkg/workflow/safe_inputs_generator.go +++ b/pkg/workflow/safe_inputs_generator.go @@ -391,9 +391,3 @@ func GenerateSafeInputShellToolScriptForInspector(toolConfig *SafeInputToolConfi func GenerateSafeInputPythonToolScriptForInspector(toolConfig *SafeInputToolConfig) string { return generateSafeInputPythonToolScript(toolConfig) } - -// GenerateSafeInputGoToolScriptForInspector generates a Go script tool handler -// This is a public wrapper for use by the CLI inspector command -func GenerateSafeInputGoToolScriptForInspector(toolConfig *SafeInputToolConfig) string { - return generateSafeInputGoToolScript(toolConfig) -} diff --git a/pkg/workflow/safe_inputs_parser.go b/pkg/workflow/safe_inputs_parser.go index 177e5cf64ee..037e32596e7 100644 --- a/pkg/workflow/safe_inputs_parser.go +++ b/pkg/workflow/safe_inputs_parser.go @@ -59,12 +59,6 @@ func HasSafeInputs(safeInputs *SafeInputsConfig) bool { return safeInputs != nil && len(safeInputs.Tools) > 0 } -// IsSafeInputsHTTPMode checks if safe-inputs is configured to use HTTP mode -// Note: All safe-inputs configurations now use HTTP mode exclusively -func IsSafeInputsHTTPMode(safeInputs *SafeInputsConfig) bool { - return safeInputs != nil -} - // IsSafeInputsEnabled checks if safe-inputs are configured. // Safe-inputs are enabled by default when configured in the workflow. // The workflowData parameter is kept for backward compatibility but is not used. @@ -206,26 +200,6 @@ func parseSafeInputsMap(safeInputsMap map[string]any) (*SafeInputsConfig, bool) return config, len(config.Tools) > 0 } -// ParseSafeInputs parses safe-inputs configuration from frontmatter (standalone function for testing) -func ParseSafeInputs(frontmatter map[string]any) *SafeInputsConfig { - if frontmatter == nil { - return nil - } - - safeInputs, exists := frontmatter["safe-inputs"] - if !exists { - return nil - } - - safeInputsMap, ok := safeInputs.(map[string]any) - if !ok { - return nil - } - - config, _ := parseSafeInputsMap(safeInputsMap) - return config -} - // extractSafeInputsConfig extracts safe-inputs configuration from frontmatter func (c *Compiler) extractSafeInputsConfig(frontmatter map[string]any) *SafeInputsConfig { safeInputsLog.Print("Extracting safe-inputs configuration from frontmatter") diff --git a/pkg/workflow/safe_inputs_parser_test.go b/pkg/workflow/safe_inputs_parser_test.go index fd55b2c6445..eefafb3fadf 100644 --- a/pkg/workflow/safe_inputs_parser_test.go +++ b/pkg/workflow/safe_inputs_parser_test.go @@ -6,155 +6,6 @@ import ( "testing" ) -func TestParseSafeInputs(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - expectedTools int - expectedNil bool - }{ - { - name: "nil frontmatter", - frontmatter: nil, - expectedNil: true, - }, - { - name: "empty frontmatter", - frontmatter: map[string]any{}, - expectedNil: true, - }, - { - name: "single javascript tool", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "search-issues": map[string]any{ - "description": "Search for issues", - "script": "return 'hello';", - "inputs": map[string]any{ - "query": map[string]any{ - "type": "string", - "description": "Search query", - "required": true, - }, - }, - }, - }, - }, - expectedTools: 1, - }, - { - name: "single shell tool", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "echo-message": map[string]any{ - "description": "Echo a message", - "run": "echo $INPUT_MESSAGE", - "inputs": map[string]any{ - "message": map[string]any{ - "type": "string", - "description": "Message to echo", - "default": "Hello", - }, - }, - }, - }, - }, - expectedTools: 1, - }, - { - name: "single python tool", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "analyze-data": map[string]any{ - "description": "Analyze data with Python", - "py": "import json\nprint(json.dumps({'result': 'success'}))", - "inputs": map[string]any{ - "data": map[string]any{ - "type": "string", - "description": "Data to analyze", - "required": true, - }, - }, - }, - }, - }, - expectedTools: 1, - }, - { - name: "single go tool", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "process-data": map[string]any{ - "description": "Process data with Go", - "go": "result := map[string]any{\"count\": len(inputs)}\njson.NewEncoder(os.Stdout).Encode(result)", - "inputs": map[string]any{ - "data": map[string]any{ - "type": "string", - "description": "Data to process", - "required": true, - }, - }, - }, - }, - }, - expectedTools: 1, - }, - { - name: "multiple tools", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "tool1": map[string]any{ - "description": "Tool 1", - "script": "return 1;", - }, - "tool2": map[string]any{ - "description": "Tool 2", - "run": "echo 2", - }, - }, - }, - expectedTools: 2, - }, - { - name: "tool with env secrets", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "api-call": map[string]any{ - "description": "Call API", - "script": "return fetch(url);", - "env": map[string]any{ - "API_KEY": "${{ secrets.API_KEY }}", - }, - }, - }, - }, - expectedTools: 1, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ParseSafeInputs(tt.frontmatter) - - if tt.expectedNil { - if result != nil { - t.Errorf("Expected nil, got %+v", result) - } - return - } - - if result == nil { - t.Error("Expected non-nil result") - return - } - - if len(result.Tools) != tt.expectedTools { - t.Errorf("Expected %d tools, got %d", tt.expectedTools, len(result.Tools)) - } - }) - } -} - func TestHasSafeInputs(t *testing.T) { tests := []struct { name string @@ -289,181 +140,3 @@ func TestIsSafeInputsEnabledWithEnv(t *testing.T) { // TestParseSafeInputsAndExtractSafeInputsConfigConsistency verifies that ParseSafeInputs // and extractSafeInputsConfig produce identical results for the same input. // This ensures both functions use the shared helper correctly. -func TestParseSafeInputsAndExtractSafeInputsConfigConsistency(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - }{ - { - name: "nil frontmatter", - frontmatter: nil, - }, - { - name: "empty frontmatter", - frontmatter: map[string]any{}, - }, - { - name: "single tool with all fields", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "search-issues": map[string]any{ - "description": "Search for issues", - "script": "return 'hello';", - "inputs": map[string]any{ - "query": map[string]any{ - "type": "string", - "description": "Search query", - "required": true, - "default": "test", - }, - }, - "env": map[string]any{ - "API_KEY": "${{ secrets.API_KEY }}", - }, - }, - }, - }, - }, - { - name: "multiple tools with different types", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "js-tool": map[string]any{ - "description": "JavaScript tool", - "script": "return 1;", - }, - "shell-tool": map[string]any{ - "description": "Shell tool", - "run": "echo hello", - }, - "python-tool": map[string]any{ - "description": "Python tool", - "py": "print('hello')", - }, - }, - }, - }, - { - name: "tool with complex inputs", - frontmatter: map[string]any{ - "safe-inputs": map[string]any{ - "complex-tool": map[string]any{ - "description": "Complex tool", - "script": "return inputs;", - "inputs": map[string]any{ - "string-param": map[string]any{ - "type": "string", - "description": "A string parameter", - }, - "number-param": map[string]any{ - "type": "number", - "description": "A number parameter", - "default": 42, - }, - "bool-param": map[string]any{ - "type": "boolean", - "description": "A boolean parameter", - "required": true, - }, - }, - }, - }, - }, - }, - } - - compiler := &Compiler{} - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result1 := ParseSafeInputs(tt.frontmatter) - result2 := compiler.extractSafeInputsConfig(tt.frontmatter) - - // Both should be nil or both non-nil - if (result1 == nil) != (result2 == nil) { - t.Errorf("Inconsistent nil results: ParseSafeInputs=%v, extractSafeInputsConfig=%v", result1 == nil, result2 == nil) - return - } - - if result1 == nil { - return - } - - // Compare number of tools - if len(result1.Tools) != len(result2.Tools) { - t.Errorf("Different number of tools: ParseSafeInputs=%d, extractSafeInputsConfig=%d", len(result1.Tools), len(result2.Tools)) - return - } - - // Compare each tool - for toolName, tool1 := range result1.Tools { - tool2, exists := result2.Tools[toolName] - if !exists { - t.Errorf("Tool %s not found in extractSafeInputsConfig result", toolName) - continue - } - - if tool1.Name != tool2.Name { - t.Errorf("Tool %s: Name mismatch: %s vs %s", toolName, tool1.Name, tool2.Name) - } - if tool1.Description != tool2.Description { - t.Errorf("Tool %s: Description mismatch: %s vs %s", toolName, tool1.Description, tool2.Description) - } - if tool1.Script != tool2.Script { - t.Errorf("Tool %s: Script mismatch: %s vs %s", toolName, tool1.Script, tool2.Script) - } - if tool1.Run != tool2.Run { - t.Errorf("Tool %s: Run mismatch: %s vs %s", toolName, tool1.Run, tool2.Run) - } - if tool1.Py != tool2.Py { - t.Errorf("Tool %s: Py mismatch: %s vs %s", toolName, tool1.Py, tool2.Py) - } - - // Compare inputs - if len(tool1.Inputs) != len(tool2.Inputs) { - t.Errorf("Tool %s: Different number of inputs: %d vs %d", toolName, len(tool1.Inputs), len(tool2.Inputs)) - continue - } - - for inputName, input1 := range tool1.Inputs { - input2, exists := tool2.Inputs[inputName] - if !exists { - t.Errorf("Tool %s: Input %s not found in extractSafeInputsConfig result", toolName, inputName) - continue - } - - if input1.Type != input2.Type { - t.Errorf("Tool %s, Input %s: Type mismatch: %s vs %s", toolName, inputName, input1.Type, input2.Type) - } - if input1.Description != input2.Description { - t.Errorf("Tool %s, Input %s: Description mismatch: %s vs %s", toolName, inputName, input1.Description, input2.Description) - } - if input1.Required != input2.Required { - t.Errorf("Tool %s, Input %s: Required mismatch: %v vs %v", toolName, inputName, input1.Required, input2.Required) - } - // Compare defaults (handle nil case) - if (input1.Default == nil) != (input2.Default == nil) { - t.Errorf("Tool %s, Input %s: Default nil mismatch: %v vs %v", toolName, inputName, input1.Default, input2.Default) - } - } - - // Compare env - if len(tool1.Env) != len(tool2.Env) { - t.Errorf("Tool %s: Different number of env vars: %d vs %d", toolName, len(tool1.Env), len(tool2.Env)) - continue - } - - for envName, envValue1 := range tool1.Env { - envValue2, exists := tool2.Env[envName] - if !exists { - t.Errorf("Tool %s: Env %s not found in extractSafeInputsConfig result", toolName, envName) - continue - } - if envValue1 != envValue2 { - t.Errorf("Tool %s, Env %s: Value mismatch: %s vs %s", toolName, envName, envValue1, envValue2) - } - } - } - }) - } -} diff --git a/pkg/workflow/safe_inputs_renderer.go b/pkg/workflow/safe_inputs_renderer.go index 255f9986749..5a4d2a97f8c 100644 --- a/pkg/workflow/safe_inputs_renderer.go +++ b/pkg/workflow/safe_inputs_renderer.go @@ -10,32 +10,6 @@ import ( var safeInputsRendererLog = logger.New("workflow:safe_inputs_renderer") -// getSafeInputsEnvVars returns the list of environment variables needed for safe-inputs -func getSafeInputsEnvVars(safeInputs *SafeInputsConfig) []string { - envVars := []string{} - seen := make(map[string]bool) - - if safeInputs == nil { - safeInputsRendererLog.Print("No safe-inputs configuration provided") - return envVars - } - - safeInputsRendererLog.Printf("Collecting environment variables from %d safe-inputs tools", len(safeInputs.Tools)) - - for _, toolConfig := range safeInputs.Tools { - for envName := range toolConfig.Env { - if !seen[envName] { - envVars = append(envVars, envName) - seen[envName] = true - } - } - } - - sort.Strings(envVars) - safeInputsRendererLog.Printf("Collected %d unique environment variables", len(envVars)) - return envVars -} - // collectSafeInputsSecrets collects all secrets from safe-inputs configuration func collectSafeInputsSecrets(safeInputs *SafeInputsConfig) map[string]string { secrets := make(map[string]string) diff --git a/pkg/workflow/safe_inputs_renderer_test.go b/pkg/workflow/safe_inputs_renderer_test.go index d557518a089..7f9d77186ea 100644 --- a/pkg/workflow/safe_inputs_renderer_test.go +++ b/pkg/workflow/safe_inputs_renderer_test.go @@ -3,75 +3,9 @@ package workflow import ( - "slices" "testing" ) -func TestGetSafeInputsEnvVars(t *testing.T) { - tests := []struct { - name string - config *SafeInputsConfig - expectedLen int - contains []string - }{ - { - name: "nil config", - config: nil, - expectedLen: 0, - }, - { - name: "tool with env", - config: &SafeInputsConfig{ - Tools: map[string]*SafeInputToolConfig{ - "test": { - Name: "test", - Env: map[string]string{ - "API_KEY": "${{ secrets.API_KEY }}", - "TOKEN": "${{ secrets.TOKEN }}", - }, - }, - }, - }, - expectedLen: 2, - contains: []string{"API_KEY", "TOKEN"}, - }, - { - name: "multiple tools with shared env", - config: &SafeInputsConfig{ - Tools: map[string]*SafeInputToolConfig{ - "tool1": { - Name: "tool1", - Env: map[string]string{"API_KEY": "key1"}, - }, - "tool2": { - Name: "tool2", - Env: map[string]string{"API_KEY": "key2"}, - }, - }, - }, - expectedLen: 1, // Should deduplicate - contains: []string{"API_KEY"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := getSafeInputsEnvVars(tt.config) - - if len(result) != tt.expectedLen { - t.Errorf("Expected %d env vars, got %d: %v", tt.expectedLen, len(result), result) - } - - for _, expected := range tt.contains { - found := slices.Contains(result, expected) - if !found { - t.Errorf("Expected to contain %s, got %v", expected, result) - } - } - }) - } -} - func TestCollectSafeInputsSecrets(t *testing.T) { tests := []struct { name string @@ -148,42 +82,3 @@ func TestCollectSafeInputsSecretsStability(t *testing.T) { } } } - -func TestGetSafeInputsEnvVarsStability(t *testing.T) { - config := &SafeInputsConfig{ - Tools: map[string]*SafeInputToolConfig{ - "zebra-tool": { - Name: "zebra-tool", - Env: map[string]string{ - "ZEBRA_SECRET": "${{ secrets.ZEBRA }}", - "ALPHA_SECRET": "${{ secrets.ALPHA }}", - }, - }, - "alpha-tool": { - Name: "alpha-tool", - Env: map[string]string{ - "BETA_SECRET": "${{ secrets.BETA }}", - }, - }, - }, - } - - // Test getSafeInputsEnvVars stability - iterations := 10 - envResults := make([][]string, iterations) - for i := range iterations { - envResults[i] = getSafeInputsEnvVars(config) - } - - for i := 1; i < iterations; i++ { - if len(envResults[i]) != len(envResults[0]) { - t.Errorf("getSafeInputsEnvVars produced different number of env vars on iteration %d", i+1) - } - for j := range envResults[0] { - if envResults[i][j] != envResults[0][j] { - t.Errorf("getSafeInputsEnvVars produced different value at position %d on iteration %d: expected %s, got %s", - j, i+1, envResults[0][j], envResults[i][j]) - } - } - } -} diff --git a/pkg/workflow/safe_inputs_timeout_test.go b/pkg/workflow/safe_inputs_timeout_test.go index b085d5d9066..98fe5905cdd 100644 --- a/pkg/workflow/safe_inputs_timeout_test.go +++ b/pkg/workflow/safe_inputs_timeout_test.go @@ -102,7 +102,7 @@ func TestSafeInputsTimeoutParsing(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - config := ParseSafeInputs(tt.frontmatter) + config := (&Compiler{}).extractSafeInputsConfig(tt.frontmatter) if config == nil { t.Fatalf("Expected config, got nil") }