diff --git a/.changeset/fix-zero-arg-safeoutputs-tools.md b/.changeset/fix-zero-arg-safeoutputs-tools.md new file mode 100644 index 00000000000..3c9c5afc8d8 --- /dev/null +++ b/.changeset/fix-zero-arg-safeoutputs-tools.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed a runtime regression where valid zero-argument custom safe-output tools were incorrectly treated as schema-discovery probes. The MCP CLI bridge now inspects the tool's `inputSchema.required` array before deciding whether to show help: it only shows help when required fields are declared, allowing zero-input and optional-only custom safe-output jobs to proceed to `tools/call` with `{}` as expected. diff --git a/actions/setup/js/mcp_cli_bridge.cjs b/actions/setup/js/mcp_cli_bridge.cjs index 11c0a7ef0fa..223ead8691e 100644 --- a/actions/setup/js/mcp_cli_bridge.cjs +++ b/actions/setup/js/mcp_cli_bridge.cjs @@ -1126,16 +1126,26 @@ function showToolHelp(serverName, toolName, tools) { * Determine whether the bridge should show tool help instead of invoking the tool * with an empty arguments object. * - * safeoutputs tools always require arguments, so any call with an empty argument - * object is a schema-discovery probe that would otherwise trigger a -32602 - * validation error and encourage wasteful retries. + * For safeoutputs tools, help is shown only when the tool schema declares at least + * one required field. Zero-argument tools (empty properties, no required array) and + * optional-only tools are valid and should proceed to MCP tools/call. A missing or + * malformed cached schema is treated conservatively as no-required so the MCP server + * remains authoritative; it can return the appropriate protocol error if needed. * * @param {string} serverName * @param {Record} toolArgs + * @param {{inputSchema?: {required?: string[]}} | null | undefined} matchedTool - resolved tool definition from the cached tool schema * @returns {boolean} */ -function shouldShowToolHelpForEmptyArgs(serverName, toolArgs) { - return serverName === SAFEOUTPUTS_SERVER_NAME && Object.keys(toolArgs).length === 0; +function shouldShowToolHelpForEmptyArgs(serverName, toolArgs, matchedTool) { + if (serverName !== SAFEOUTPUTS_SERVER_NAME || Object.keys(toolArgs).length !== 0) { + return false; + } + // Show help only when the schema explicitly declares required fields. + // Zero-argument and optional-only tools have no required array (or an empty one) + // and must be allowed to call through to MCP so their output item is recorded. + const required = matchedTool && matchedTool.inputSchema && Array.isArray(matchedTool.inputSchema.required) ? matchedTool.inputSchema.required : []; + return required.length > 0; } /** @@ -1431,7 +1441,7 @@ async function main() { const stdinContent = hasStdinJsonPayload(toolUserArgs) ? readStdinSync() : null; const { args: toolArgs, json: jsonOutput } = parseToolArgs(toolUserArgs, schemaProperties, stdinContent); - if (shouldShowToolHelpForEmptyArgs(serverName, toolArgs)) { + if (shouldShowToolHelpForEmptyArgs(serverName, toolArgs, matchedTool)) { core.warning(`[${serverName}] No arguments provided for '${toolName}'; showing command help instead of calling the tool`); auditLog(serverName, { event: "show_tool_help_empty_args", tool: toolName }); showToolHelp(serverName, toolName, tools); diff --git a/actions/setup/js/mcp_cli_bridge.test.cjs b/actions/setup/js/mcp_cli_bridge.test.cjs index 5e682df02a0..661cbd1d3c4 100644 --- a/actions/setup/js/mcp_cli_bridge.test.cjs +++ b/actions/setup/js/mcp_cli_bridge.test.cjs @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import fs from "fs"; +import http from "http"; import os from "os"; import path from "path"; @@ -8,6 +9,7 @@ import { formatResponse, getToolCallTimeoutMs, hasStdinJsonPayload, + main, parseToolArgs, readStdinSync, shouldShowToolHelpForEmptyArgs, @@ -205,10 +207,32 @@ describe("mcp_cli_bridge.cjs", () => { } }); - it("shows help instead of calling safeoutputs tools with an empty args object", () => { - expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {})).toBe(true); - expect(shouldShowToolHelpForEmptyArgs("safeoutputs", { title: "Bug report" })).toBe(false); - expect(shouldShowToolHelpForEmptyArgs("other-server", {})).toBe(false); + it("allows zero-argument tools to proceed — only shows help when required fields are declared", () => { + // Empty schema (zero-input custom tool) — must NOT show help; empty call is valid + const emptySchemaTools = { inputSchema: { type: "object", properties: {}, additionalProperties: false } }; + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {}, emptySchemaTools)).toBe(false); + + // Optional-only tool (required array absent) — must NOT show help + const optionalOnlyTool = { inputSchema: { type: "object", properties: { flag: { type: "boolean" } } } }; + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {}, optionalOnlyTool)).toBe(false); + + // Optional-only tool (required array present but empty) — must NOT show help + const emptyRequiredTool = { inputSchema: { required: [] } }; + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {}, emptyRequiredTool)).toBe(false); + + // Tool with required fields and empty args — MUST show help (probe detection) + const requiredFieldTool = { inputSchema: { required: ["title"] } }; + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {}, requiredFieldTool)).toBe(true); + + // Missing matchedTool (e.g. unknown tool) — treated as no-required; must NOT show help + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {}, null)).toBe(false); + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", {}, undefined)).toBe(false); + + // Non-empty args are never affected + expect(shouldShowToolHelpForEmptyArgs("safeoutputs", { title: "Bug report" }, requiredFieldTool)).toBe(false); + + // Non-safeoutputs servers are never affected + expect(shouldShowToolHelpForEmptyArgs("other-server", {}, requiredFieldTool)).toBe(false); }); it("coerces scientific notation when schema properties are unavailable", () => { @@ -1142,4 +1166,175 @@ describe("mcp_cli_bridge.cjs", () => { } }); }); + + describe("main — zero-argument tool routing via local MCP server", () => { + /** @type {import("http").Server} */ + let server; + /** @type {string} */ + let serverUrl; + /** @type {object[]} */ + let recordedBodies; + /** @type {string} */ + let toolsFile; + /** @type {string[]} */ + let savedArgv; + + /** @type {Array<{name: string, description: string, inputSchema: object}>} */ + const zeroInputTools = [ + { + name: "dispatch_code_factory", + description: "Record a dispatch code factory safe-output item", + inputSchema: { + type: "object", + properties: {}, + additionalProperties: false, + }, + }, + ]; + + /** @type {Array<{name: string, description: string, inputSchema: object}>} */ + const requiredInputTools = [ + { + name: "create_issue", + description: "Create an issue", + inputSchema: { + type: "object", + properties: { title: { type: "string" } }, + required: ["title"], + additionalProperties: false, + }, + }, + ]; + + /** + * Start a minimal MCP HTTP server that records request bodies and returns + * appropriate responses for each protocol step. + * + * @returns {Promise} + */ + async function startMcpServer() { + recordedBodies = []; + server = await new Promise(resolve => { + const s = http.createServer((req, res) => { + let body = ""; + req.on("data", chunk => { + body += chunk; + }); + req.on("end", () => { + let parsed; + try { + parsed = JSON.parse(body); + } catch { + parsed = {}; + } + recordedBodies.push(parsed); + + res.setHeader("Content-Type", "application/json"); + + if (parsed.method === "initialize") { + res.setHeader("mcp-session-id", "test-session-001"); + res.end(JSON.stringify({ jsonrpc: "2.0", id: parsed.id, result: { capabilities: {} } })); + } else if (parsed.method === "tools/call") { + res.end( + JSON.stringify({ + jsonrpc: "2.0", + id: parsed.id, + result: { content: [{ type: "text", text: "ok" }] }, + }) + ); + } else { + // notifications/initialized, ping, etc. + res.end(JSON.stringify({ jsonrpc: "2.0", result: {} })); + } + }); + }); + s.listen(0, "127.0.0.1", () => resolve(s)); + }); + + const addr = /** @type {import("net").AddressInfo} */ server.address(); + serverUrl = `http://127.0.0.1:${addr.port}`; + } + + beforeEach(async () => { + savedArgv = process.argv; + await startMcpServer(); + }); + + afterEach(async () => { + process.argv = savedArgv; + if (toolsFile && fs.existsSync(toolsFile)) { + fs.unlinkSync(toolsFile); + } + await new Promise(resolve => server.close(resolve)); + }); + + /** + * Write a tools file and configure process.argv for a main() call. + * + * @param {object[]} tools + * @param {string[]} userArgs + */ + function setupMainCall(tools, userArgs) { + toolsFile = path.join(os.tmpdir(), `test-bridge-tools-${Date.now()}-${Math.random().toString(36).slice(2)}.json`); + fs.writeFileSync(toolsFile, JSON.stringify(tools)); + process.argv = ["node", "mcp_cli_bridge.cjs", "--server-name", "safeoutputs", "--server-url", serverUrl, "--tools-file", toolsFile, "--api-key", "test-key", ...userArgs]; + } + + it("reaches MCP tools/call with {} for a zero-input tool (bare invocation)", async () => { + setupMainCall(zeroInputTools, ["dispatch_code_factory"]); + + await main(); + + const toolsCallBody = recordedBodies.find(b => b.method === "tools/call"); + expect(toolsCallBody).toBeDefined(); + expect(toolsCallBody.params.name).toBe("dispatch_code_factory"); + expect(toolsCallBody.params.arguments).toEqual({}); + }); + + it("reaches MCP tools/call with {} for a zero-input tool (piped {} via . sentinel)", async () => { + setupMainCall(zeroInputTools, ["dispatch_code_factory", "."]); + + // Simulate piped `{}` via the . sentinel with stdinContent = "{}" + const readStdinSyncSpy = vi.spyOn(/** @type {any} */ require("./mcp_cli_bridge.cjs"), "readStdinSync"); + // readStdinSync is a module-level function already called inside main(); we need + // to intercept at the module level. Since we cannot easily do that here, simulate + // the piped-stdin path by using process.stdin.isTTY = undefined so hasStdinJsonPayload + // returns true for the empty-args path, and by spying on fs.readSync to return "{}". + const origIsTTY = process.stdin.isTTY; + // @ts-ignore + process.stdin.isTTY = undefined; + + const fsReadSyncSpy = vi.spyOn(fs, "readSync").mockImplementationOnce((_fd, buf, _offset, length) => { + const encoded = Buffer.from("{}"); + encoded.copy(/** @type {Buffer} */ buf, 0, 0, Math.min(encoded.length, length)); + return Math.min(encoded.length, length); + }); + fsReadSyncSpy.mockImplementationOnce(() => 0); // EOF on second read + + try { + setupMainCall(zeroInputTools, ["dispatch_code_factory"]); + await main(); + + const toolsCallBody = recordedBodies.find(b => b.method === "tools/call"); + expect(toolsCallBody).toBeDefined(); + expect(toolsCallBody.params.name).toBe("dispatch_code_factory"); + expect(toolsCallBody.params.arguments).toEqual({}); + } finally { + process.stdin.isTTY = origIsTTY; + fsReadSyncSpy.mockRestore(); + readStdinSyncSpy.mockRestore?.(); + } + }); + + it("shows help and does NOT reach MCP tools/call for a required-input tool called with no args", async () => { + setupMainCall(requiredInputTools, ["create_issue"]); + + await main(); + + const toolsCallBody = recordedBodies.find(b => b.method === "tools/call"); + expect(toolsCallBody).toBeUndefined(); + + expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("No arguments provided for 'create_issue'")); + }); + }); }); diff --git a/pkg/workflow/safe_outputs_tools_meta_integration_test.go b/pkg/workflow/safe_outputs_tools_meta_integration_test.go index 04fd7320874..c56b8cd25be 100644 --- a/pkg/workflow/safe_outputs_tools_meta_integration_test.go +++ b/pkg/workflow/safe_outputs_tools_meta_integration_test.go @@ -118,4 +118,67 @@ func extractToolsMetaFromLockFile(t *testing.T, yamlStr string) ToolsMeta { return meta } +// TestToolsMetaJSONCompiledWorkflowZeroArgCustomJob verifies that a custom safe-output job +// with no inputs compiles to a dynamic_tools entry with an empty properties schema and no +// required key. This protects the compiler/bridge contract: the bridge must pass {} through +// to MCP tools/call rather than treating it as a schema-discovery probe. +func TestToolsMetaJSONCompiledWorkflowZeroArgCustomJob(t *testing.T) { + tmpDir := testutil.TempDir(t, "zero-arg-job-compiled-test") + + // Inputs are intentionally omitted — the supported way to declare a zero-input custom job. + testContent := `--- +on: push +name: Test Zero-Arg Custom Job +engine: copilot +safe-outputs: + create-issue: + max: 1 + jobs: + dispatch-code-factory: + description: "Record a dispatch code factory safe-output item" + runs-on: ubuntu-latest + steps: + - name: dispatch + run: echo "dispatched" +--- + +Test workflow to verify zero-argument custom job schema. +` + testFile := filepath.Join(tmpDir, "test-zero-arg-job.md") + require.NoError(t, os.WriteFile(testFile, []byte(testContent), 0644), "should write test file") + + compiler := NewCompiler() + require.NoError(t, compiler.CompileWorkflow(testFile), "compilation should succeed") + + lockFile := filepath.Join(tmpDir, "test-zero-arg-job.lock.yml") + yamlBytes, err := os.ReadFile(lockFile) + require.NoError(t, err, "should read lock file") + + meta := extractToolsMetaFromLockFile(t, string(yamlBytes)) + + // Find the dispatch_code_factory entry in dynamic_tools + var dispatchTool map[string]any + for _, tool := range meta.DynamicTools { + if tool["name"] == "dispatch_code_factory" { + dispatchTool = tool + break + } + } + require.NotNil(t, dispatchTool, "dynamic_tools should contain dispatch_code_factory") + + schema, ok := dispatchTool["inputSchema"].(map[string]any) + require.True(t, ok, "inputSchema should be a map") + + assert.Equal(t, "object", schema["type"], "schema type should be object") + + props, ok := schema["properties"].(map[string]any) + require.True(t, ok, "properties should be a map") + assert.Empty(t, props, "properties should be empty for a zero-input tool") + + _, hasRequired := schema["required"] + assert.False(t, hasRequired, "required key must be absent for a zero-input tool") + + assert.Equal(t, false, schema["additionalProperties"], "additionalProperties should be false") +} + // constraint-bearing configurations to ensure no tool type regresses silently.