diff --git a/actions/setup/js/mcp_cli_bridge.cjs b/actions/setup/js/mcp_cli_bridge.cjs index e887c470195..2bc43e30e32 100644 --- a/actions/setup/js/mcp_cli_bridge.cjs +++ b/actions/setup/js/mcp_cli_bridge.cjs @@ -436,9 +436,10 @@ function parseBridgeArgs(argv) { * Boolean flags (--key without a value) are set to true. * * @param {string[]} args - User arguments after the tool name + * @param {Record} [schemaProperties] - Tool input schema properties * @returns {{args: Record, json: boolean}} */ -function parseToolArgs(args) { +function parseToolArgs(args, schemaProperties = {}) { /** @type {Record} */ const result = {}; let jsonOutput = false; @@ -453,12 +454,12 @@ function parseToolArgs(args) { if (key === "json") { jsonOutput = true; } else { - result[key] = raw.slice(eqIdx + 1); + result[key] = coerceToolArgValue(key, raw.slice(eqIdx + 1), schemaProperties[key], result[key]); } } else if (raw === "json") { jsonOutput = true; } else if (i + 1 < args.length && !args[i + 1].startsWith("--")) { - result[raw] = args[i + 1]; + result[raw] = coerceToolArgValue(raw, args[i + 1], schemaProperties[raw], result[raw]); i++; } else { result[raw] = true; @@ -470,6 +471,87 @@ function parseToolArgs(args) { return { args: result, json: jsonOutput }; } +/** + * Parse and coerce a CLI argument value based on the MCP tool schema property type. + * + * @param {string} key - Argument key name + * @param {string} rawValue - Raw CLI value + * @param {{type?: string|string[]}|undefined} schemaProperty - JSON schema property + * @param {unknown} existingValue - Existing value (for repeated flags) + * @returns {unknown} + */ +function coerceToolArgValue(key, rawValue, schemaProperty, existingValue) { + /** @type {string[]} */ + const types = []; + if (schemaProperty && typeof schemaProperty === "object" && "type" in schemaProperty && schemaProperty.type != null) { + if (Array.isArray(schemaProperty.type)) { + for (const t of schemaProperty.type) { + if (typeof t === "string") { + types.push(t); + } + } + } else if (typeof schemaProperty.type === "string") { + types.push(schemaProperty.type); + } + } + + if (types.includes("array")) { + /** @type {unknown[]} */ + let values; + const trimmed = rawValue.trim(); + if (trimmed.startsWith("[")) { + try { + const parsed = JSON.parse(trimmed); + if (Array.isArray(parsed)) { + values = parsed; + } else { + values = [rawValue]; + } + } catch { + values = [rawValue]; + } + } else if (rawValue.includes(",")) { + values = rawValue + .split(",") + .map(v => v.trim()) + .filter(v => v.length > 0); + } else { + values = [rawValue]; + } + + if (Array.isArray(existingValue)) { + return [...existingValue, ...values]; + } + return values; + } + + if (types.includes("integer") && /^-?\d+$/.test(rawValue)) { + const parsed = Number.parseInt(rawValue, 10); + if (Number.isSafeInteger(parsed)) { + return parsed; + } + } + + if (types.includes("number")) { + const parsed = Number(rawValue); + if (!Number.isNaN(parsed) && Number.isFinite(parsed)) { + return parsed; + } + } + + if (types.includes("boolean")) { + const normalized = rawValue.trim().toLowerCase(); + if (normalized === "true" || normalized === "1") { + return true; + } + if (normalized === "false" || normalized === "0") { + return false; + } + } + + return rawValue; +} + // --------------------------------------------------------------------------- // Tool information / help // --------------------------------------------------------------------------- @@ -583,6 +665,7 @@ function formatResponse(responseBody, serverName) { // Extract result content if (resp && typeof resp === "object" && "result" in resp && resp.result && typeof resp.result === "object") { const result = resp.result; + const isErrorResult = "isError" in result && result.isError === true; if ("content" in result && Array.isArray(result.content)) { const outputParts = []; for (const item of result.content) { @@ -596,13 +679,27 @@ function formatResponse(responseBody, serverName) { } } const output = outputParts.join("\n"); - process.stdout.write(output + "\n"); - core.info(`[${serverName}] Tool output: ${output.length} chars`); + if (isErrorResult) { + process.stderr.write(output + "\n"); + core.error(`[${serverName}] Tool returned isError=true: ${output.length} chars`); + auditLog(serverName, { event: "tool_error", error: output }); + process.exitCode = 1; + } else { + process.stdout.write(output + "\n"); + core.info(`[${serverName}] Tool output: ${output.length} chars`); + } return; } // Fallback: print raw result const resultStr = typeof result === "string" ? result : JSON.stringify(result); - process.stdout.write(resultStr + "\n"); + if (isErrorResult) { + process.stderr.write(resultStr + "\n"); + core.error(`[${serverName}] Tool returned isError=true`); + auditLog(serverName, { event: "tool_error", error: resultStr }); + process.exitCode = 1; + } else { + process.stdout.write(resultStr + "\n"); + } return; } @@ -658,7 +755,9 @@ async function main() { } // Route: [--param value ...] → call tool via MCP - const { args: toolArgs, json: jsonOutput } = parseToolArgs(toolUserArgs); + const matchedTool = tools.find(tool => tool && typeof tool === "object" && tool.name === toolName); + const schemaProperties = matchedTool && matchedTool.inputSchema && matchedTool.inputSchema.properties ? matchedTool.inputSchema.properties : {}; + const { args: toolArgs, json: jsonOutput } = parseToolArgs(toolUserArgs, schemaProperties); core.info(`[${serverName}] Calling tool '${toolName}' with args: ${JSON.stringify(toolArgs)}${jsonOutput ? " (--json)" : ""}`); auditLog(serverName, { event: "call_start", tool: toolName, arguments: toolArgs }); @@ -701,9 +800,18 @@ async function main() { } } -main().catch(err => { - const core = global.core; - const message = err instanceof Error ? err.stack || err.message : String(err); - core.error(`mcp_cli_bridge fatal: ${message}`); - process.exitCode = 1; -}); +if (require.main === module) { + main().catch(err => { + const core = global.core; + const message = err instanceof Error ? err.stack || err.message : String(err); + core.error(`mcp_cli_bridge fatal: ${message}`); + process.exitCode = 1; + }); +} + +module.exports = { + parseToolArgs, + coerceToolArgValue, + formatResponse, + main, +}; diff --git a/actions/setup/js/mcp_cli_bridge.test.cjs b/actions/setup/js/mcp_cli_bridge.test.cjs new file mode 100644 index 00000000000..ce4561444ad --- /dev/null +++ b/actions/setup/js/mcp_cli_bridge.test.cjs @@ -0,0 +1,71 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { formatResponse, parseToolArgs } from "./mcp_cli_bridge.cjs"; + +describe("mcp_cli_bridge.cjs", () => { + let originalCore; + let stdoutSpy; + let stderrSpy; + /** @type {string[]} */ + let stdoutChunks; + /** @type {string[]} */ + let stderrChunks; + + beforeEach(() => { + originalCore = global.core; + global.core = { + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + setFailed: vi.fn(), + }; + process.exitCode = 0; + stdoutChunks = []; + stderrChunks = []; + stdoutSpy = vi.spyOn(process.stdout, "write").mockImplementation(chunk => { + stdoutChunks.push(String(chunk)); + return true; + }); + stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(chunk => { + stderrChunks.push(String(chunk)); + return true; + }); + }); + + afterEach(() => { + stdoutSpy.mockRestore(); + stderrSpy.mockRestore(); + global.core = originalCore; + process.exitCode = 0; + }); + + it("coerces integer and array arguments based on tool schema", () => { + const schemaProperties = { + count: { type: "integer" }, + workflows: { type: ["null", "array"] }, + }; + + const { args } = parseToolArgs(["--count", "3", "--workflows", "daily-issues-report"], schemaProperties); + + expect(args).toEqual({ + count: 3, + workflows: ["daily-issues-report"], + }); + }); + + it("treats MCP result envelopes with isError=true as errors", () => { + formatResponse( + { + result: { + isError: true, + content: [{ type: "text", text: '{"error":"failed to audit workflow run"}' }], + }, + }, + "agenticworkflows" + ); + + expect(stdoutChunks.join("")).toBe(""); + expect(stderrChunks.join("")).toContain("failed to audit workflow run"); + expect(process.exitCode).toBe(1); + }); +}); diff --git a/pkg/cli/mcp_logs_guardrail.go b/pkg/cli/mcp_logs_guardrail.go index 7977bb0e474..792ed0e9d8e 100644 --- a/pkg/cli/mcp_logs_guardrail.go +++ b/pkg/cli/mcp_logs_guardrail.go @@ -23,6 +23,9 @@ const ( // is separate from the artifact download directory (/tmp/gh-aw/aw-mcp/logs) // so that these JSON summary files are not included in artifact uploads. mcpLogsCacheDir = "/tmp/gh-aw/logs-cache" + + mcpLogsCacheDirPerm = 0o755 + mcpLogsCacheFilePerm = 0o644 ) // MCPLogsGuardrailResponse represents the response returned by the logs tool. @@ -46,8 +49,11 @@ func buildLogsFileResponse(outputStr string) string { if info.Mode()&os.ModeSymlink != 0 { return buildLogsFileErrorResponse(fmt.Sprintf("logs cache path %q is a symlink; refusing to use it", mcpLogsCacheDir)) } + if !info.IsDir() { + return buildLogsFileErrorResponse(fmt.Sprintf("logs cache path %q is not a directory", mcpLogsCacheDir)) + } } else if os.IsNotExist(err) { - if mkErr := os.MkdirAll(mcpLogsCacheDir, 0700); mkErr != nil && !os.IsExist(mkErr) { + if mkErr := os.MkdirAll(mcpLogsCacheDir, mcpLogsCacheDirPerm); mkErr != nil && !os.IsExist(mkErr) { mcpLogsGuardrailLog.Printf("Failed to create logs cache directory: %v", mkErr) return buildLogsFileErrorResponse(fmt.Sprintf("failed to create logs cache directory: %v", mkErr)) } @@ -55,6 +61,10 @@ func buildLogsFileResponse(outputStr string) string { mcpLogsGuardrailLog.Printf("Failed to stat logs cache directory: %v", err) return buildLogsFileErrorResponse(fmt.Sprintf("failed to access logs cache directory: %v", err)) } + if chmodErr := os.Chmod(mcpLogsCacheDir, mcpLogsCacheDirPerm); chmodErr != nil { + mcpLogsGuardrailLog.Printf("Failed to set logs cache directory permissions: %v", chmodErr) + return buildLogsFileErrorResponse(fmt.Sprintf("failed to set logs cache directory permissions: %v", chmodErr)) + } // Use SHA256 of content as filename for content-addressed deduplication. sum := sha256.Sum256([]byte(outputStr)) @@ -62,11 +72,21 @@ func buildLogsFileResponse(outputStr string) string { filePath := filepath.Join(mcpLogsCacheDir, fileName) // Skip writing if a file with identical content already exists. - if _, err := os.Lstat(filePath); err == nil { + if fileInfo, err := os.Lstat(filePath); err == nil { + if fileInfo.Mode()&os.ModeSymlink != 0 { + return buildLogsFileErrorResponse(fmt.Sprintf("logs cache file path %q is a symlink; refusing to use it", filePath)) + } + if !fileInfo.Mode().IsRegular() { + return buildLogsFileErrorResponse(fmt.Sprintf("logs cache file path %q is not a regular file", filePath)) + } + if chmodErr := os.Chmod(filePath, mcpLogsCacheFilePerm); chmodErr != nil { + mcpLogsGuardrailLog.Printf("Failed to update logs cache file permissions: %v", chmodErr) + return buildLogsFileErrorResponse(fmt.Sprintf("failed to set logs cache file permissions: %v", chmodErr)) + } mcpLogsGuardrailLog.Printf("Logs data already cached at: %s", filePath) } else if os.IsNotExist(err) { // Write with O_EXCL to avoid following symlinks or races. - f, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + f, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mcpLogsCacheFilePerm) if err != nil { mcpLogsGuardrailLog.Printf("Failed to create logs cache file: %v", err) return buildLogsFileErrorResponse(fmt.Sprintf("failed to create logs cache file: %v", err)) @@ -82,6 +102,11 @@ func buildLogsFileResponse(outputStr string) string { mcpLogsGuardrailLog.Printf("Failed to write logs data to file: %v", errMsg) return buildLogsFileErrorResponse(fmt.Sprintf("failed to write logs data to file: %v", errMsg)) } + if chmodErr := os.Chmod(filePath, mcpLogsCacheFilePerm); chmodErr != nil { + _ = os.Remove(filePath) + mcpLogsGuardrailLog.Printf("Failed to set logs cache file permissions: %v", chmodErr) + return buildLogsFileErrorResponse(fmt.Sprintf("failed to set logs cache file permissions: %v", chmodErr)) + } mcpLogsGuardrailLog.Printf("Logs data written to file: %s (%d bytes)", filePath, len(outputStr)) } else { mcpLogsGuardrailLog.Printf("Failed to stat logs cache file: %v", err) diff --git a/pkg/cli/mcp_logs_guardrail_test.go b/pkg/cli/mcp_logs_guardrail_test.go index a915ef5da74..0632a2c3201 100644 --- a/pkg/cli/mcp_logs_guardrail_test.go +++ b/pkg/cli/mcp_logs_guardrail_test.go @@ -49,6 +49,39 @@ func TestBuildLogsFileResponse_WritesFile(t *testing.T) { _ = os.Remove(response.FilePath) } +func TestBuildLogsFileResponse_SetsReadablePermissions(t *testing.T) { + output := `{"summary":{"total_runs":1}}` + result := buildLogsFileResponse(output) + + var response MCPLogsGuardrailResponse + if err := json.Unmarshal([]byte(result), &response); err != nil { + t.Fatalf("Response should be valid JSON: %v", err) + } + if response.FilePath == "" { + t.Fatal("Response should include file_path") + } + + cacheInfo, err := os.Stat(mcpLogsCacheDir) + if err != nil { + t.Fatalf("Cache directory should exist: %v", err) + } + cachePerm := cacheInfo.Mode().Perm() + if cachePerm&0o005 != 0o005 { + t.Errorf("Cache directory should be accessible to other users (expected other r-x bits set), got mode %o", cachePerm) + } + + fileInfo, err := os.Stat(response.FilePath) + if err != nil { + t.Fatalf("Cache file should exist: %v", err) + } + filePerm := fileInfo.Mode().Perm() + if filePerm&0o004 != 0o004 { + t.Errorf("Cache file should be world-readable (expected other read bit set), got mode %o", filePerm) + } + + _ = os.Remove(response.FilePath) +} + func TestBuildLogsFileResponse_ContentDeduplication(t *testing.T) { // Same content should yield the same file path (content-addressed) output := `{"summary": {"total_runs": 5}, "runs": []}` diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index cb9f0759ee3..2efe8abda81 100644 --- a/pkg/cli/mcp_tools_privileged.go +++ b/pkg/cli/mcp_tools_privileged.go @@ -359,6 +359,7 @@ Returns JSON with the following structure: return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to audit workflow run: "+mainMsg, nil) } return &mcp.CallToolResult{ + IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: string(jsonBytes)}}, }, nil, nil } @@ -461,6 +462,7 @@ Returns JSON describing the differences between the base run and each comparison return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to diff workflow runs: "+mainMsg, nil) } return &mcp.CallToolResult{ + IsError: true, Content: []mcp.Content{&mcp.TextContent{Text: string(jsonBytes)}}, }, nil, nil } diff --git a/pkg/cli/mcp_tools_privileged_test.go b/pkg/cli/mcp_tools_privileged_test.go index c9fbca10913..a4d0a801845 100644 --- a/pkg/cli/mcp_tools_privileged_test.go +++ b/pkg/cli/mcp_tools_privileged_test.go @@ -4,6 +4,9 @@ package cli import ( "context" + "encoding/json" + "fmt" + "os" "os/exec" "testing" @@ -251,3 +254,92 @@ func TestAuditToolPassesGithubRepositoryAsRepoFlag(t *testing.T) { }) } } + +// TestAuditToolErrorEnvelopeSetsIsErrorTrue verifies that audit command failures +// returned as JSON envelopes are marked with IsError=true in the MCP response. +func TestAuditToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) { + mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestAuditToolErrorEnvelopeSetsIsErrorTrueHelperProcess") + cmd.Env = append(os.Environ(), "GH_AW_AUDIT_HELPER_PROCESS=1") + return cmd + } + + server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil) + err := registerAuditTool(server, mockExecCmd, "", false) + require.NoError(t, err, "registerAuditTool should succeed") + + session := connectInMemory(t, server) + + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "audit", + Arguments: map[string]any{"run_id_or_url": "9999999999"}, + }) + require.NoError(t, err, "audit tool should return result envelope without protocol error") + require.NotNil(t, result, "result should not be nil") + assert.True(t, result.IsError, "audit error envelope should set IsError=true") + require.NotEmpty(t, result.Content, "result should contain text content") + + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected text content in audit error response") + + var envelope map[string]any + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &envelope), "error response should be valid JSON") + assert.Equal(t, "9999999999", envelope["run_id_or_url"], "error envelope should include original run ID") + errorMessage, ok := envelope["error"].(string) + require.True(t, ok, "error envelope should include string error field") + assert.Contains(t, errorMessage, "failed to audit workflow run", "error envelope should include contextual prefix") +} + +func TestAuditToolErrorEnvelopeSetsIsErrorTrueHelperProcess(t *testing.T) { + if os.Getenv("GH_AW_AUDIT_HELPER_PROCESS") != "1" { + return + } + + _, _ = fmt.Fprintln(os.Stderr, "✗ failed to fetch run metadata") + os.Exit(1) +} + +func TestAuditDiffToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) { + mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestAuditDiffToolErrorEnvelopeSetsIsErrorTrueHelperProcess") + cmd.Env = append(os.Environ(), "GH_AW_AUDIT_DIFF_HELPER_PROCESS=1") + return cmd + } + + server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0"}, nil) + err := registerAuditDiffTool(server, mockExecCmd, "", false) + require.NoError(t, err, "registerAuditDiffTool should succeed") + + session := connectInMemory(t, server) + + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "audit-diff", + Arguments: map[string]any{ + "base_run_id": "100", + "compare_run_ids": []string{"200"}, + }, + }) + require.NoError(t, err, "audit-diff tool should return result envelope without protocol error") + require.NotNil(t, result, "result should not be nil") + assert.True(t, result.IsError, "audit-diff error envelope should set IsError=true") + require.NotEmpty(t, result.Content, "result should contain text content") + + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected text content in audit-diff error response") + + var envelope map[string]any + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &envelope), "error response should be valid JSON") + assert.Equal(t, "100", envelope["base_run_id"], "error envelope should include base run ID") + errorMessage, ok := envelope["error"].(string) + require.True(t, ok, "error envelope should include string error field") + assert.Contains(t, errorMessage, "failed to diff workflow runs", "error envelope should include contextual prefix") +} + +func TestAuditDiffToolErrorEnvelopeSetsIsErrorTrueHelperProcess(t *testing.T) { + if os.Getenv("GH_AW_AUDIT_DIFF_HELPER_PROCESS") != "1" { + return + } + + _, _ = fmt.Fprintln(os.Stderr, "✗ failed to diff workflow runs") + os.Exit(1) +}