Skip to content

Show safeoutputs command help on empty arguments #43566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Jul 5, 2026
Merged

Show safeoutputs command help on empty arguments #43566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9689d9e
Initial plan
Copilot Jul 5, 2026
f374f84
Show safeoutputs help on empty args
Copilot Jul 5, 2026
d7c3412
Simplify shouldShowToolHelpForEmptyArgs to use serverName check inste…
Copilot Jul 5, 2026
1db4fcb
Merge branch 'main' into copilot/aw-failures-bound-cli-retry-loop
github-actions[bot] Jul 5, 2026
e6ff13e
Fix hint pollution in core.error/auditLog; add tests for no-toolName …
Copilot Jul 5, 2026
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
35 changes: 33 additions & 2 deletions actions/setup/js/mcp_cli_bridge.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,22 @@ function showToolHelp(serverName, toolName, tools) {
process.stdout.write(lines.join("\n") + "\n");
}

/**
* 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.
*
* @param {string} serverName
* @param {Record<string, unknown>} toolArgs
* @returns {boolean}
*/
function shouldShowToolHelpForEmptyArgs(serverName, toolArgs) {
return serverName === SAFEOUTPUTS_SERVER_NAME && Object.keys(toolArgs).length === 0;
}

/**
* Collapse whitespace and trim long help text for compact output.
*
Expand Down Expand Up @@ -1109,9 +1125,10 @@ function isResultMessage(message) {
*
* @param {unknown} responseBody - Parsed JSON-RPC response body
* @param {string} serverName - Server name (for logging)
* @param {string} [toolName] - Tool name, when known
* @returns {Promise<void>}
*/
async function formatResponse(responseBody, serverName) {
async function formatResponse(responseBody, serverName, toolName = "") {
const core = global.core;
const messages = extractJSONRPCMessages(responseBody);
renderProgressMessages(messages);
Expand All @@ -1123,11 +1140,17 @@ async function formatResponse(responseBody, serverName) {
const errRecord = resp.error;
const message = "message" in errRecord ? String(errRecord.message || "Unknown error") : "Unknown error";
const code = "code" in errRecord && errRecord.code != null ? String(errRecord.code) : "";
const isSafeOutputsEmptyArgs = serverName === SAFEOUTPUTS_SERVER_NAME && code === "-32602" && /Empty arguments are not allowed/i.test(message);
const hint =
isSafeOutputsEmptyArgs && toolName
? `Hint: do not retry '${serverName} ${toolName}' with empty arguments. Run '${serverName} ${toolName} --help' to inspect the required options, or call 'noop' with a message if no action is needed.`
: "";
const errText = code ? `Error [${code}]: ${message}` : `Error: ${message}`;
process.stderr.write(errText + "\n");
core.error(`[${serverName}] Tool call error: ${errText}`);

@github-actions github-actions Bot Jul 5, 2026

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.

Multi-line hint injected into errText pollutes core.error output and the structured audit log: the hint string contains embedded newlines and is concatenated directly into errText, which is then passed to both core.error() and auditLog(), making the audit log harder to parse programmatically and the CI error annotation span multiple unexpected lines.

💡 Suggested fix

Log the base error separately from the hint:

const baseErrText = code ? `Error [${code}]: ${message}` : `Error: ${message}`;
process.stderr.write(baseErrText + '\n');
core.error(`[${serverName}] Tool call error: ${baseErrText}`);
auditLog(serverName, { event: 'tool_error', error: baseErrText });
process.exitCode = 1;
if (hint) process.stderr.write(hint + '\n');
return;

This keeps the audit log field as a single-line string, keeps core.error() annotations clean, and still surfaces the hint to the human reader on stderr.

auditLog(serverName, { event: "tool_error", error: errText });
process.exitCode = 1;
if (hint) process.stderr.write(hint + "\n");
return;
}

Expand Down Expand Up @@ -1231,6 +1254,13 @@ async function main() {
const stdinContent = hasStdinJsonPayload(toolUserArgs) ? readStdinSync() : null;
const { args: toolArgs, json: jsonOutput } = parseToolArgs(toolUserArgs, schemaProperties, stdinContent);

@pelikhan pelikhan Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot safeoutputs always require arguments so simplify detection without a regex

Copilot AI Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — shouldShowToolHelpForEmptyArgs now takes serverName and simply checks serverName === SAFEOUTPUTS_SERVER_NAME && Object.keys(toolArgs).length === 0, removing the inputSchema.required inspection entirely. Tests updated to match the new signature.


if (shouldShowToolHelpForEmptyArgs(serverName, toolArgs)) {
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);
return;
}

core.info(`[${serverName}] Calling tool '${toolName}' with args: ${JSON.stringify(toolArgs)}${jsonOutput ? " (--json)" : ""}`);
auditLog(serverName, { event: "call_start", tool: toolName, arguments: toolArgs });

Expand Down Expand Up @@ -1263,7 +1293,7 @@ async function main() {
// --json: print the raw JSON-RPC response body
await writeStdoutAndFlush(JSON.stringify(resp.body, null, 2) + "\n");
} else {
await formatResponse(resp.body, serverName);
await formatResponse(resp.body, serverName, toolName);
}
} catch (err) {
const totalMs = Date.now() - callStartMs;
Expand Down Expand Up @@ -1300,6 +1330,7 @@ module.exports = {
writeStdoutAndFlush,
showHelp,
showToolHelp,
shouldShowToolHelpForEmptyArgs,
hasStdinJsonPayload,
readStdinSync,
ensureSafeOutputsTools,
Expand Down
64 changes: 63 additions & 1 deletion actions/setup/js/mcp_cli_bridge.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import fs from "fs";
import os from "os";
import path from "path";

import { ensureSafeOutputsTools, formatResponse, hasStdinJsonPayload, parseToolArgs, readStdinSync, showHelp, showToolHelp, writeStdoutAndFlush } from "./mcp_cli_bridge.cjs";
import { ensureSafeOutputsTools, formatResponse, hasStdinJsonPayload, parseToolArgs, readStdinSync, shouldShowToolHelpForEmptyArgs, showHelp, showToolHelp, writeStdoutAndFlush } from "./mcp_cli_bridge.cjs";

describe("mcp_cli_bridge.cjs", () => {
let originalCore;
Expand Down Expand Up @@ -190,6 +190,12 @@ 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("coerces scientific notation when schema properties are unavailable", () => {
const { args } = parseToolArgs(["--max_tokens", "1e3", "--threshold", "-2E-4"], {});

Expand Down Expand Up @@ -248,6 +254,62 @@ describe("mcp_cli_bridge.cjs", () => {
expect(process.exitCode).toBe(0);
});

it("adds a non-retry hint for safeoutputs empty-argument rejections", async () => {

@github-actions github-actions Bot Jul 5, 2026

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 formatResponse hint test only covers the case where toolName is provided. A second assertion for the no-toolName path (toolName = "") would pin the intended silent behaviour.\n\n

\n💡 Suggested addition\n\njs\nit('omits non-retry hint when toolName is absent', async () => {\n await formatResponse(\n { error: { code: -32602, message: 'Empty arguments are not allowed — ...' } },\n 'safeoutputs'\n // toolName omitted → defaults to ""\n );\n const stderr = stderrChunks.join('');\n expect(stderr).not.toContain('do not retry');\n});\n\n\nWithout this, a future change that makes the hint unconditional would still pass all tests.\n\n
\n\n@copilot please address this.

Copilot AI Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the no-toolName test in the commit $(git rev-parse --short HEAD). When toolName is omitted (defaults to ""), the hint is suppressed and only the base error line appears on stderr.

await formatResponse(
{
error: {
code: -32602,
message: "Empty arguments are not allowed — this tool is write-once, not a discovery probe.",
},
},
"safeoutputs",
"create_issue"
);

const stderr = stderrChunks.join("");
expect(stderr).toContain("Error [-32602]: Empty arguments are not allowed");
expect(stderr).toContain("do not retry 'safeoutputs create_issue' with empty arguments");
expect(stderr).toContain("safeoutputs create_issue --help");
expect(process.exitCode).toBe(1);
});

it("omits non-retry hint when toolName is absent", async () => {
await formatResponse(
{
error: {
code: -32602,
message: "Empty arguments are not allowed — this tool is write-once, not a discovery probe.",
},
},
"safeoutputs"
// toolName omitted → defaults to ""
);

const stderr = stderrChunks.join("");
expect(stderr).toContain("Error [-32602]");
expect(stderr).not.toContain("do not retry");
expect(process.exitCode).toBe(1);
});

it("does not add a non-retry hint for -32602 errors from non-safeoutputs servers", async () => {
await formatResponse(
{
error: {
code: -32602,
message: "Empty arguments are not allowed — this tool is write-once, not a discovery probe.",
},
},
"agenticworkflows",
"some_tool"
);

const stderr = stderrChunks.join("");
expect(stderr).toContain("Error [-32602]");
expect(stderr).not.toContain("do not retry");
expect(stderr).not.toContain("--help");
expect(process.exitCode).toBe(1);
});

it("keeps top-level help compact for many commands", () => {
const tools = Array.from({ length: 25 }, (_, i) => ({
name: `tool_${i + 1}`,

@github-actions github-actions Bot Jul 5, 2026

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.

No test asserts that the hint is absent for non-safeoutputs servers: the new test proves the hint appears for safeoutputs with a matching -32602 message, but nothing prevents a future change that accidentally widens the condition to all servers.

💡 Suggested addition

Add a negative assertion immediately after the existing test:

it('does NOT add a non-retry hint for -32602 errors from non-safeoutputs servers', async () => {
  await formatResponse(
    {
      error: {
        code: -32602,
        message: 'Empty arguments are not allowed — this tool is write-once, not a discovery probe.',
      },
    },
    'agenticworkflows', // different server
    'some_tool'
  );

  const stderr = stderrChunks.join('');
  expect(stderr).toContain('Error [-32602]');
  expect(stderr).not.toContain('do not retry');
  expect(stderr).not.toContain('--help');
  expect(process.exitCode).toBe(1);
});

Without this, a regression that broadens hint injection to all servers goes undetected by the test suite.

Expand Down
Loading