Fix MCP CLI bridge arg coercion, audit error envelopes, and logs cache readability

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<string, {type?: string|string[]}>} [schemaProperties] - Tool input schema properties
  * @returns {{args: Record<string, unknown>, json: boolean}}
  */
-function parseToolArgs(args) {
+function parseToolArgs(args, schemaProperties = {}) {
   /** @type {Record<string, unknown>} */
   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: <command> [--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,
+};

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);
+  });
+});

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,27 +49,44 @@ 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))
 		}
 	} else {
 		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))
 	fileName := hex.EncodeToString(sum[:]) + ".json"
 	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)

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": []}`

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
 		}