diff --git a/.changeset/patch-fix-safe-inputs-copilot-cli.md b/.changeset/patch-fix-safe-inputs-copilot-cli.md new file mode 100644 index 00000000000..530b3425488 --- /dev/null +++ b/.changeset/patch-fix-safe-inputs-copilot-cli.md @@ -0,0 +1,8 @@ +--- +"gh-aw": patch +--- + +Fix safe-inputs MCP config for Copilot CLI: convert `type: stdio` to `type: local` when generating Copilot fields; fix server startup JS to avoid calling `.catch()` on undefined; update tests to assert behavior for Copilot and Claude. + +This is a non-breaking bugfix that ensures the Copilot CLI receives a compatible MCP `type` and that the generated server entrypoint handles errors correctly. + diff --git a/.changeset/patch-fix-safe-inputs-copilot.md b/.changeset/patch-fix-safe-inputs-copilot.md new file mode 100644 index 00000000000..131e87a0ec0 --- /dev/null +++ b/.changeset/patch-fix-safe-inputs-copilot.md @@ -0,0 +1,6 @@ +--- +"gh-aw": patch +--- + +Fix safe-inputs MCP config for Copilot CLI: convert `type: stdio` to `type: local` when generating Copilot fields; fix server startup JS to avoid calling `.catch()` on undefined; update tests to assert behavior for Copilot and Claude. + diff --git a/.github/workflows/copilot-pr-merged-report.lock.yml b/.github/workflows/copilot-pr-merged-report.lock.yml index 81f0bb39244..665d3a07d05 100644 --- a/.github/workflows/copilot-pr-merged-report.lock.yml +++ b/.github/workflows/copilot-pr-merged-report.lock.yml @@ -2931,7 +2931,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { @@ -3230,12 +3232,15 @@ jobs: const path = require("path"); const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); - startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" - }).catch(error => { + try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); + } catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); - }); + } EOFSI chmod +x /tmp/gh-aw/safe-inputs/mcp-server.cjs @@ -3264,7 +3269,7 @@ jobs: { "mcpServers": { "safeinputs": { - "type": "stdio", + "type": "local", "command": "node", "args": ["/tmp/gh-aw/safe-inputs/mcp-server.cjs"], "tools": ["*"], diff --git a/.github/workflows/daily-performance-summary.lock.yml b/.github/workflows/daily-performance-summary.lock.yml index 8202e263732..794ae869034 100644 --- a/.github/workflows/daily-performance-summary.lock.yml +++ b/.github/workflows/daily-performance-summary.lock.yml @@ -3505,7 +3505,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { diff --git a/.github/workflows/dev.lock.yml b/.github/workflows/dev.lock.yml index e8861c6109b..fff63420b20 100644 --- a/.github/workflows/dev.lock.yml +++ b/.github/workflows/dev.lock.yml @@ -1432,7 +1432,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { @@ -1731,12 +1733,15 @@ jobs: const path = require("path"); const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); - startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" - }).catch(error => { + try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); + } catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); - }); + } EOFSI chmod +x /tmp/gh-aw/safe-inputs/mcp-server.cjs diff --git a/.github/workflows/smoke-copilot-no-firewall.lock.yml b/.github/workflows/smoke-copilot-no-firewall.lock.yml index b026a595f67..8b395c70661 100644 --- a/.github/workflows/smoke-copilot-no-firewall.lock.yml +++ b/.github/workflows/smoke-copilot-no-firewall.lock.yml @@ -4436,7 +4436,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { @@ -4735,12 +4737,15 @@ jobs: const path = require("path"); const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); - startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" - }).catch(error => { + try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); + } catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); - }); + } EOFSI chmod +x /tmp/gh-aw/safe-inputs/mcp-server.cjs @@ -4796,7 +4801,7 @@ jobs: "tools": ["*"] }, "safeinputs": { - "type": "stdio", + "type": "local", "command": "node", "args": ["/tmp/gh-aw/safe-inputs/mcp-server.cjs"], "tools": ["*"], diff --git a/.github/workflows/smoke-copilot-playwright.lock.yml b/.github/workflows/smoke-copilot-playwright.lock.yml index b3a4b1e4c8c..51ddf781532 100644 --- a/.github/workflows/smoke-copilot-playwright.lock.yml +++ b/.github/workflows/smoke-copilot-playwright.lock.yml @@ -4435,7 +4435,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { @@ -4734,12 +4736,15 @@ jobs: const path = require("path"); const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); - startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" - }).catch(error => { + try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); + } catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); - }); + } EOFSI chmod +x /tmp/gh-aw/safe-inputs/mcp-server.cjs @@ -4795,7 +4800,7 @@ jobs: "tools": ["*"] }, "safeinputs": { - "type": "stdio", + "type": "local", "command": "node", "args": ["/tmp/gh-aw/safe-inputs/mcp-server.cjs"], "tools": ["*"], diff --git a/.github/workflows/smoke-copilot-safe-inputs.lock.yml b/.github/workflows/smoke-copilot-safe-inputs.lock.yml index 408fd11fbd7..28bb2132877 100644 --- a/.github/workflows/smoke-copilot-safe-inputs.lock.yml +++ b/.github/workflows/smoke-copilot-safe-inputs.lock.yml @@ -4340,7 +4340,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { @@ -4639,12 +4641,15 @@ jobs: const path = require("path"); const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); - startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" - }).catch(error => { + try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); + } catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); - }); + } EOFSI chmod +x /tmp/gh-aw/safe-inputs/mcp-server.cjs @@ -4673,7 +4678,7 @@ jobs: { "mcpServers": { "safeinputs": { - "type": "stdio", + "type": "local", "command": "node", "args": ["/tmp/gh-aw/safe-inputs/mcp-server.cjs"], "tools": ["*"], diff --git a/.github/workflows/smoke-copilot.lock.yml b/.github/workflows/smoke-copilot.lock.yml index 5c50958cff8..1dff774bf33 100644 --- a/.github/workflows/smoke-copilot.lock.yml +++ b/.github/workflows/smoke-copilot.lock.yml @@ -4352,7 +4352,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { @@ -4651,12 +4653,15 @@ jobs: const path = require("path"); const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); - startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" - }).catch(error => { + try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); + } catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); - }); + } EOFSI chmod +x /tmp/gh-aw/safe-inputs/mcp-server.cjs @@ -4706,7 +4711,7 @@ jobs: } }, "safeinputs": { - "type": "stdio", + "type": "local", "command": "node", "args": ["/tmp/gh-aw/safe-inputs/mcp-server.cjs"], "tools": ["*"], diff --git a/.github/workflows/test-python-safe-input.lock.yml b/.github/workflows/test-python-safe-input.lock.yml index 99a6d3e6e42..f150cd4d80b 100644 --- a/.github/workflows/test-python-safe-input.lock.yml +++ b/.github/workflows/test-python-safe-input.lock.yml @@ -2692,7 +2692,9 @@ jobs: for (const tool of tools) { registerTool(server, tool); } - cleanupConfigFile(configPath, server); + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } start(server); } if (require.main === module) { diff --git a/pkg/workflow/js/safe_inputs_mcp_server.cjs b/pkg/workflow/js/safe_inputs_mcp_server.cjs index 2038eca3f80..208fda0847f 100644 --- a/pkg/workflow/js/safe_inputs_mcp_server.cjs +++ b/pkg/workflow/js/safe_inputs_mcp_server.cjs @@ -44,6 +44,7 @@ const { bootstrapSafeInputsServer, cleanupConfigFile } = require("./safe_inputs_ * @param {string} configPath - Path to the configuration JSON file * @param {Object} [options] - Additional options * @param {string} [options.logDir] - Override log directory from config + * @param {boolean} [options.skipCleanup] - Skip deletion of config file (useful for stdio mode with agent restarts) */ function startSafeInputsServer(configPath, options = {}) { // Create server first to have logger available @@ -67,8 +68,10 @@ function startSafeInputsServer(configPath, options = {}) { registerTool(server, tool); } - // Cleanup: delete the configuration file after loading - cleanupConfigFile(configPath, server); + // Cleanup: delete the configuration file after loading (unless skipCleanup is true) + if (!options.skipCleanup) { + cleanupConfigFile(configPath, server); + } // Start the server start(server); diff --git a/pkg/workflow/safe_inputs.go b/pkg/workflow/safe_inputs.go index 3bd759771e2..bcc5c48a0b5 100644 --- a/pkg/workflow/safe_inputs.go +++ b/pkg/workflow/safe_inputs.go @@ -407,12 +407,16 @@ const { startSafeInputsServer } = require("./safe_inputs_mcp_server.cjs"); const configPath = path.join(__dirname, "tools.json"); // Start the stdio server -startSafeInputsServer(configPath, { - logDir: "/tmp/gh-aw/safe-inputs/logs" -}).catch(error => { +// Note: skipCleanup is true for stdio mode to allow agent restarts +try { + startSafeInputsServer(configPath, { + logDir: "/tmp/gh-aw/safe-inputs/logs", + skipCleanup: true + }); +} catch (error) { console.error("Failed to start safe-inputs stdio server:", error); process.exit(1); -}); +} `) } else { // HTTP transport - server started in separate step @@ -641,7 +645,12 @@ func renderSafeInputsMCPConfigWithOptions(yaml *strings.Builder, safeInputs *Saf // Choose transport based on mode if IsSafeInputsStdioMode(safeInputs) { // Stdio transport configuration - server started by agent - yaml.WriteString(" \"type\": \"stdio\",\n") + // Use "local" for Copilot CLI, "stdio" for other engines + typeValue := "stdio" + if includeCopilotFields { + typeValue = "local" + } + yaml.WriteString(" \"type\": \"" + typeValue + "\",\n") yaml.WriteString(" \"command\": \"node\",\n") yaml.WriteString(" \"args\": [\"/tmp/gh-aw/safe-inputs/mcp-server.cjs\"],\n") diff --git a/pkg/workflow/safe_inputs_mode_test.go b/pkg/workflow/safe_inputs_mode_test.go index d321693a714..947095ad7d9 100644 --- a/pkg/workflow/safe_inputs_mode_test.go +++ b/pkg/workflow/safe_inputs_mode_test.go @@ -65,9 +65,9 @@ Test safe-inputs stdio mode t.Error("Safe-inputs MCP server config not found") } - // Should use stdio transport - if !strings.Contains(yamlStr, `"type": "stdio"`) { - t.Error("Expected type field set to 'stdio' in MCP config") + // Should use local transport for Copilot (stdio is converted to local for Copilot CLI compatibility) + if !strings.Contains(yamlStr, `"type": "local"`) { + t.Error("Expected type field set to 'local' in MCP config for Copilot engine") } if !strings.Contains(yamlStr, `"command": "node"`) { @@ -279,9 +279,9 @@ Test mode via import yamlStr := string(lockContent) - // Verify stdio mode is used from import - if !strings.Contains(yamlStr, `"type": "stdio"`) { - t.Error("Expected stdio mode from imported configuration") + // Verify local mode is used from import (converted from stdio for Copilot CLI) + if !strings.Contains(yamlStr, `"type": "local"`) { + t.Error("Expected local mode (converted from stdio) from imported configuration for Copilot engine") } // Verify HTTP server steps are NOT present @@ -292,6 +292,61 @@ Test mode via import t.Logf("✓ Mode correctly inherited from import") } +// TestSafeInputsStdioModeWithClaudeEngine verifies that stdio mode with Claude engine uses "stdio" not "local" +func TestSafeInputsStdioModeWithClaudeEngine(t *testing.T) { + // Create a temporary workflow file + tempDir := t.TempDir() + workflowPath := filepath.Join(tempDir, "test-workflow.md") + + workflowContent := `--- +on: workflow_dispatch +engine: claude +safe-inputs: + mode: stdio + test-tool: + description: Test tool + script: | + return { result: "test" }; +--- + +Test safe-inputs stdio mode with Claude engine +` + + err := os.WriteFile(workflowPath, []byte(workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + err = compiler.CompileWorkflow(workflowPath) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockPath := strings.TrimSuffix(workflowPath, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockPath) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + yamlStr := string(lockContent) + + // Verify that type field is "stdio" for Claude engine (not converted to "local") + if !strings.Contains(yamlStr, `"type": "stdio"`) { + t.Error("Expected type field set to 'stdio' in MCP config for Claude engine") + } + + // Verify "local" is NOT used + safeinputsConfig := extractSafeinputsConfigSection(yamlStr) + if strings.Contains(safeinputsConfig, `"type": "local"`) { + t.Error("Claude engine should use 'stdio' type, not 'local'") + } + + t.Logf("✓ Stdio mode correctly uses 'stdio' type for Claude engine") +} + // extractSafeinputsConfigSection extracts the safeinputs configuration section from the YAML func extractSafeinputsConfigSection(yamlStr string) string { start := strings.Index(yamlStr, `"safeinputs"`)