fix(samples): propagate handler isError to MCP response; add content-text defense-in-depth

actions/setup/js/apply_samples.cjs

@@ -392,6 +392,30 @@ function resolveMcpServerPath() {
   throw new Error(`${ERR_CONFIG}: apply_samples: could not locate safe_outputs_mcp_server.cjs. Looked in: ${candidates.join(", ")}`);
 }
 
+/**
+ * Return true when an MCP tool-call result carries an application-level error
+ * payload, regardless of whether the transport-level `isError` flag was set.
+ *
+ * Safe-output handlers encode errors as `{"result":"error", ...}` inside the
+ * first content text item.  This helper provides a defense-in-depth check so
+ * that the driver fails even when an older server version did not set `isError`.
+ *
+ * @param {any} result - The `result` object from a JSON-RPC tools/call response.
+ * @returns {boolean}
+ */
+function sampleResultIsError(result) {
+  if (!result) return false;
+  if (result.isError) return true;
+  const text = result.content && result.content[0] && result.content[0].text;
+  if (!text) return false;
+  try {
+    const parsed = JSON.parse(text);
+    return !!(parsed && parsed.result === "error");
+  } catch {
+    return false;
+  }
+}
+
 /**
  * Append a synthetic terminal_reason: completed marker to the engine stdio log
  * so downstream parsers / handle_agent_failure recognize the replay as a
@@ -489,8 +513,8 @@ async function main() {
         continue;
       }
       const result = callRsp.result;
-      if (result && result.isError) {
-        const text = result.content && result.content[0] && result.content[0].text;
+      if (sampleResultIsError(result)) {
+        const text = result && result.content && result.content[0] && result.content[0].text;
         failures.push(`sample[${i}] (tool=${sample.tool}): ${text || JSON.stringify(result)}`);
       } else {
         core.info(`apply_samples: sample[${i}] (tool=${sample.tool}) ok`);
@@ -543,4 +567,6 @@ module.exports = {
   // Exported for unit testing of the 3-tier PR head ref resolution logic.
   derivePrHeadRef,
   fetchPullRequestHeadRef,
+  // Exported for unit testing of the defense-in-depth error detection.
+  sampleResultIsError,
 };

actions/setup/js/apply_samples.test.cjs

@@ -489,3 +489,45 @@ describe("apply_samples.cjs preStagePatch (create_pull_request / push_to_pull_re
     expect(log).toEqual(["seed"]);
   });
 });
+
+describe("apply_samples.cjs sampleResultIsError", () => {
+  const { sampleResultIsError } = require("./apply_samples.cjs");
+
+  it("returns false for a successful result (no isError, no error content)", () => {
+    const result = { content: [{ type: "text", text: JSON.stringify({ result: "success", id: 1 }) }], isError: false };
+    expect(sampleResultIsError(result)).toBe(false);
+  });
+
+  it("returns true when isError is true on the result (flag takes precedence)", () => {
+    const result = {
+      // Content is a success payload — proves the early-return on the isError flag,
+      // not the content-text fallback path.
+      content: [{ type: "text", text: JSON.stringify({ result: "success", id: 1 }) }],
+      isError: true,
+    };
+    expect(sampleResultIsError(result)).toBe(true);
+  });
+
+  it("returns true (defense-in-depth) when content text has result:error but isError is false", () => {
+    // This simulates the bug: an older server that forgot to set isError:true.
+    const result = {
+      content: [{ type: "text", text: JSON.stringify({ result: "error", error: "patch failed", details: "no merge base" }) }],
+      isError: false,
+    };
+    expect(sampleResultIsError(result)).toBe(true);
+  });
+
+  it("returns false for non-JSON content text", () => {
+    const result = { content: [{ type: "text", text: "plain text, not JSON" }], isError: false };
+    expect(sampleResultIsError(result)).toBe(false);
+  });
+
+  it("returns false for null result", () => {
+    expect(sampleResultIsError(null)).toBe(false);
+  });
+
+  it("returns false for empty content array", () => {
+    const result = { content: [], isError: false };
+    expect(sampleResultIsError(result)).toBe(false);
+  });
+});

actions/setup/js/mcp_server_core.cjs

@@ -796,7 +796,9 @@ async function handleRequest(server, request, defaultHandler) {
       // Call handler and await the result (supports both sync and async handlers)
       const handlerResult = await Promise.resolve(handler(args));
       const content = handlerResult && handlerResult.content ? handlerResult.content : [];
-      result = { content, isError: false };
+      // Preserve isError from the handler result (e.g. safe-output handlers return isError:true on error)
+      const isError = !!(handlerResult && handlerResult.isError);
+      result = { content, isError };
     } else if (/^notifications\//.test(method)) {
       // Notifications don't need a response
       return null;
@@ -953,7 +955,9 @@ async function handleMessage(server, req, defaultHandler) {
       const result = await Promise.resolve(handler(args));
       server.debug(`Handler returned for tool: ${name}`);
       const content = result && result.content ? result.content : [];
-      server.replyResult(id, { content, isError: false });
+      // Preserve isError from the handler result (e.g. safe-output handlers return isError:true on error)
+      const isError = !!(result && result.isError);
+      server.replyResult(id, { content, isError });
     } else if (/^notifications\//.test(method)) {
       server.debug(`ignore ${method}`);
     } else {

actions/setup/js/mcp_server_core.test.cjs

@@ -187,6 +187,36 @@ describe("mcp_server_core.cjs", () => {
       expect(results[0].result.isError).toBe(false);
     });
 
+    it("should propagate isError:true from handler result to MCP response", async () => {
+      // Reuse the server, capture hooks, and stderr spy already set up in beforeEach.
+      const { registerTool, handleMessage } = await import("./mcp_server_core.cjs");
+
+      registerTool(server, {
+        name: "failing_tool",
+        description: "A tool that returns an application-level error",
+        inputSchema: {
+          type: "object",
+          properties: { input: { type: "string", description: "Input" } },
+          required: ["input"],
+        },
+        handler: () => ({
+          content: [{ type: "text", text: JSON.stringify({ result: "error", error: "something went wrong" }) }],
+          isError: true,
+        }),
+      });
+
+      await handleMessage(server, {
+        jsonrpc: "2.0",
+        id: 42,
+        method: "tools/call",
+        params: { name: "failing_tool", arguments: { input: "test" } },
+      });
+
+      expect(results).toHaveLength(1);
+      expect(results[0].result.isError).toBe(true);
+      expect(results[0].result.content[0].text).toContain('"result":"error"');
+    });
+
     it("should normalize tool arguments before required-field validation", async () => {
       const { createServer, registerTool, handleMessage } = await import("./mcp_server_core.cjs");
       results = [];
@@ -440,6 +470,72 @@ describe("mcp_server_core.cjs", () => {
     });
   });
 
+  describe("handleRequest", () => {
+    beforeEach(() => {
+      // Suppress stderr output during tests
+      vi.spyOn(process.stderr, "write").mockImplementation(() => true);
+    });
+
+    it("should propagate isError:true from handler result into the JSON-RPC response", async () => {
+      const { createServer, registerTool, handleRequest } = await import("./mcp_server_core.cjs");
+      const server = createServer({ name: "test-server", version: "1.0.0" });
+
+      registerTool(server, {
+        name: "failing_tool",
+        description: "A tool that returns an application-level error",
+        inputSchema: {
+          type: "object",
+          properties: { input: { type: "string", description: "Input" } },
+          required: ["input"],
+        },
+        handler: () => ({
+          content: [{ type: "text", text: JSON.stringify({ result: "error", error: "something went wrong" }) }],
+          isError: true,
+        }),
+      });
+
+      const response = await handleRequest(server, {
+        jsonrpc: "2.0",
+        id: 7,
+        method: "tools/call",
+        params: { name: "failing_tool", arguments: { input: "test" } },
+      });
+
+      expect(response.id).toBe(7);
+      expect(response.result.isError).toBe(true);
+      expect(response.result.content[0].text).toContain('"result":"error"');
+    });
+
+    it("should report isError:false for a successful handler result", async () => {
+      const { createServer, registerTool, handleRequest } = await import("./mcp_server_core.cjs");
+      const server = createServer({ name: "test-server", version: "1.0.0" });
+
+      registerTool(server, {
+        name: "ok_tool",
+        description: "A tool that succeeds",
+        inputSchema: {
+          type: "object",
+          properties: { input: { type: "string", description: "Input" } },
+          required: ["input"],
+        },
+        handler: args => ({
+          content: [{ type: "text", text: `received: ${args.input}` }],
+        }),
+      });
+
+      const response = await handleRequest(server, {
+        jsonrpc: "2.0",
+        id: 8,
+        method: "tools/call",
+        params: { name: "ok_tool", arguments: { input: "hello" } },
+      });
+
+      expect(response.id).toBe(8);
+      expect(response.result.isError).toBe(false);
+      expect(response.result.content[0].text).toBe("received: hello");
+    });
+  });
+
   describe("loadToolHandlers", () => {
     let server;
     const fs = require("fs");

actions/setup/js/safe_outputs_mcp_server_http.cjs

@@ -49,6 +49,20 @@ const { runHttpServer, logStartupError } = require("./mcp_http_server_runner.cjs
 const { normalizeSafeOutputToolArguments, stripInternalSafeOutputSchemaMetadata } = require("./safe_outputs_mcp_arguments.cjs");
 moduleLogger.debug("All modules loaded successfully");
 
+/**
+ * Normalize a handler result into the MCP tool-call response shape, preserving
+ * the handler-provided `isError` flag (e.g. safe-output handlers return
+ * `isError: true` on error). Hardcoding `isError: false` here would mask
+ * application-level errors from clients such as the samples replay driver.
+ * @param {any} result - Raw result returned by a tool handler.
+ * @returns {{ content: any[], isError: boolean }}
+ */
+function normalizeMcpToolResult(result) {
+  const content = result && result.content ? result.content : [];
+  const isError = !!(result && result.isError);
+  return { content, isError };
+}
+
 /**
  * Create and configure the MCP server with tools
  * @param {Object} [options] - Additional options
@@ -189,9 +203,8 @@ function createMCPServer(options = {}) {
       const result = await Promise.resolve(toolHandler(args));
       logger.debug(`Handler returned for tool: ${tool.name}`);
 
-      // Normalize result to MCP format
-      const content = result && result.content ? result.content : [];
-      return { content, isError: false };
+      // Normalize result to MCP format; preserve isError from the handler result
+      return normalizeMcpToolResult(result);
     });
 
     registeredCount++;
@@ -226,9 +239,8 @@ function createMCPServer(options = {}) {
         const result = await Promise.resolve(defaultHandler({ toolName, ...args }));
         logger.debug(`Handler returned for dynamic tool: ${toolName}`);
 
-        // Normalize result to MCP format
-        const content = result && result.content ? result.content : [];
-        return { content, isError: false };
+        // Normalize result to MCP format; preserve isError from the handler result
+        return normalizeMcpToolResult(result);
       });
 
       registeredCount++;
@@ -346,4 +358,5 @@ if (require.main === module) {
 module.exports = {
   startHttpServer,
   createMCPServer,
+  normalizeMcpToolResult,
 };

actions/setup/js/safe_outputs_mcp_server_http.test.cjs

@@ -0,0 +1,45 @@
+import { describe, it, expect } from "vitest";
+
+// normalizeMcpToolResult is the shared helper used by BOTH tool-registration
+// paths in safe_outputs_mcp_server_http.cjs (predefined tools and dynamic
+// safe-job tools). The HTTP server cannot be unit-tested without real
+// transport/config bootstrap, so the isError-preservation logic that both
+// `server.tool(...)` callbacks rely on is exercised here directly.
+const { normalizeMcpToolResult } = require("./safe_outputs_mcp_server_http.cjs");
+
+describe("safe_outputs_mcp_server_http.cjs normalizeMcpToolResult", () => {
+  it("preserves isError:true from the handler result", () => {
+    const result = {
+      content: [{ type: "text", text: JSON.stringify({ result: "error", error: "boom" }) }],
+      isError: true,
+    };
+    expect(normalizeMcpToolResult(result)).toEqual({
+      content: result.content,
+      isError: true,
+    });
+  });
+
+  it("reports isError:false for a successful handler result", () => {
+    const result = {
+      content: [{ type: "text", text: JSON.stringify({ result: "success", id: 1 }) }],
+    };
+    expect(normalizeMcpToolResult(result)).toEqual({
+      content: result.content,
+      isError: false,
+    });
+  });
+
+  it("coerces a truthy non-boolean isError to a boolean", () => {
+    const result = { content: [{ type: "text", text: "x" }], isError: "yes" };
+    expect(normalizeMcpToolResult(result).isError).toBe(true);
+  });
+
+  it("defaults content to an empty array when the handler returns no content", () => {
+    expect(normalizeMcpToolResult({ isError: true })).toEqual({ content: [], isError: true });
+  });
+
+  it("returns a safe shape for null/undefined results", () => {
+    expect(normalizeMcpToolResult(null)).toEqual({ content: [], isError: false });
+    expect(normalizeMcpToolResult(undefined)).toEqual({ content: [], isError: false });
+  });
+});