Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-zero-arg-safeoutputs-tools.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 16 additions & 6 deletions actions/setup/js/mcp_cli_bridge.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>} 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 : [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treating an unresolved matchedTool (unknown tool name) the same as a zero-input schema means a genuinely-required-arg tool called with empty args can silently reach tools/call with {} whenever the local tool cache is stale or the name is mistyped.

💡 Details

shouldShowToolHelpForEmptyArgs defaults required to [] whenever matchedTool is null/undefined — which happens both for legitimately schema-less tools and for tools not found in the cached tools array at all (e.g. stale toolsFile, race with a newly-registered custom job, or a simple typo in toolName). In the "not found" case the caller previously got a fast, clear local help message; now it silently proceeds to a network round-trip that will likely fail server-side with a less actionable -32602, regressing exactly the wasteful-retry problem this function's docstring says it exists to prevent — just for a different trigger. Consider distinguishing "tool not found in cache" (show help / unknown command error, as showToolHelp already does) from "tool found with an empty/optional schema" (proceed to call).

return required.length > 0;
}

/**
Expand Down Expand Up @@ -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);
Expand Down
203 changes: 199 additions & 4 deletions actions/setup/js/mcp_cli_bridge.test.cjs
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -8,6 +9,7 @@ import {
formatResponse,
getToolCallTimeoutMs,
hasStdinJsonPayload,
main,
parseToolArgs,
readStdinSync,
shouldShowToolHelpForEmptyArgs,
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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<void>}
*/
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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setupMainCall inside the try block overwrites process.argv without the . sentinel, making the outer setupMainCall(zeroInputTools, ["dispatch_code_factory", "."]) dead code — hasStdinJsonPayload will not see . and the piped-stdin path is not exercised as intended.

Additionally, vi.spyOn(require("./mcp_cli_bridge.cjs"), "readStdinSync") is unlikely to intercept the binding captured inside main() (CJS closure), which is why the test uses mockRestore?.(). The test only exercises the fs.readSync mock, not true stdin routing.

Suggested fix: remove the duplicate setupMainCall inside try (keep only the one with ["dispatch_code_factory", "."] before the spy setup), or simplify the test to trust the bare-invocation case already covers zero-arg routing.

@copilot please address this.

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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] vi.spyOn on a CommonJS require() call here won't intercept the readStdinSync that main() already closed over at module-load time — so this spy is effectively dead code and the test is not actually verifying what the comment says it does.

💡 Context

In CJS modules, require() returns a cached module object. If readStdinSync is a local binding (not re-exported as an enumerable property), spying on the cached export does not affect the internal call inside main(). This means the readStdinSync used by main() will call through to the real implementation regardless of the spy.

The fsReadSyncSpy on fs.readSync does intercept the right level, which is why the test likely still passes — but the readStdinSyncSpy lines are misleading. Consider removing them or replacing this test with one that directly exercises readStdinSync independently, keeping the main()-level test simpler.

@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vi.spyOn on readStdinSync from a fresh require() never intercepts the call main() actually makes, so it's dead test scaffolding that misrepresents what's verified.

💡 Details

readStdinSync() is invoked directly inside main() as an internal CommonJS reference, not via module.exports.readStdinSync. Spying on the exported reference from a separate require("./mcp_cli_bridge.cjs") therefore cannot patch the call site main() uses — the spy is set up, never asserted on, and mockRestore?.() even hints the author suspected it might not be a real spy. Combined with the duplicate setupMainCall issue flagged above, none of this stdin-simulation machinery in the test actually does anything; it should either be removed or the module needs to expose stdin reading through an injectable seam (e.g. accept a reader function) so it can genuinely be mocked.

// 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"]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The .\ sentinel set on line 1295 is overwritten by a second setupMainCall call here (without the . argument), so hasStdinJsonPayload will receive ["dispatch_code_factory"] rather than ["."] — the piped-stdin branch is never actually exercised.

💡 Suggested fix

Remove the duplicate setupMainCall call inside the try block. The process.argv was already configured at line 1295 with the . sentinel; the second call silently resets it:

// Remove this line — it replaces argv that was set with ["."]
// setupMainCall(zeroInputTools, ["dispatch_code_factory"]);
await main();

Without the fix the piped-stdin assertion passes vacuously: the test reaches tools/call via the zero-arg (non-stdin) path, not via readStdinSync.

@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's second setupMainCall() call silently overwrites the first (with the . sentinel), so the test never actually exercises the piped-stdin path it claims to verify.

💡 Details

Line 1295 sets process.argv to include the . sentinel and mocks fs.readSync/isTTY to simulate piped stdin. But line 1315 calls setupMainCall(zeroInputTools, ["dispatch_code_factory"]) again — with no . — right before await main(), which overwrites process.argv and creates a brand-new toolsFile (leaking the first one, since only the last toolsFile value is cleaned up in afterEach). The test therefore passes for the exact same reason as the "bare invocation" test above it (empty args routed to a zero-required-fields schema), not because of stdin/sentinel handling. The elaborate fs.readSync mock and readStdinSyncSpy are dead code with respect to what the test title promises.

Fix: remove the duplicate setupMainCall call on line 1315 so the mocked stdin path from line 1295 is what actually runs through main().

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'"));
});
});
});
63 changes: 63 additions & 0 deletions pkg/workflow/safe_outputs_tools_meta_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Loading