diff --git a/.changeset/fix-duplicate-generic-safe-output-tools.md b/.changeset/fix-duplicate-generic-safe-output-tools.md new file mode 100644 index 00000000000..e366b98d7db --- /dev/null +++ b/.changeset/fix-duplicate-generic-safe-output-tools.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed the safe-outputs MCP server exposing a duplicate, unwired generic tool alongside the real, workflow-named tool for renamed dynamic safe outputs. When `call_workflow` (or `dispatch_workflow`/`dispatch_repository`) targeted a workflow, the server registered both the properly-typed tool (e.g. `agent_sandbox_stack`) and a generic `call_workflow` tool whose handler wrote a malformed record and reported false success, because the dedup check only compared exact tool names. Dynamic tool synthesis now also recognises tools by their family metadata (`_call_workflow_name`, `_workflow_name`, `_dispatch_repository_tool`) and no longer synthesizes tools for handler-only or global config keys such as `create_report_incomplete_issue`, `mentions`, and `max_bot_mentions`. diff --git a/actions/setup/js/safe_outputs_tools_loader.cjs b/actions/setup/js/safe_outputs_tools_loader.cjs index 06990ff15dc..d932005d643 100644 --- a/actions/setup/js/safe_outputs_tools_loader.cjs +++ b/actions/setup/js/safe_outputs_tools_loader.cjs @@ -52,6 +52,44 @@ function sanitizeArgsBySchema(args, inputSchema, onUnknownKeysStripped) { return sanitizedArgs; } +/** + * Map from safe-outputs config key to the tool-definition metadata field that marks a + * dynamically generated tool belonging to that config key. Such tools are named after their + * target (e.g. a tool named `my_workflow` for the `call_workflow` config key), so they cover + * the config key even though their name differs from it. + * @type {Record} + */ +const DYNAMIC_TOOL_METADATA_BY_CONFIG_KEY = { + dispatch_workflow: "_workflow_name", + dispatch_repository: "_dispatch_repository_tool", + call_workflow: "_call_workflow_name", +}; + +/** + * Safe-outputs config keys that never correspond to an agent-facing tool: handler-only output + * types produced by other handlers, and global configuration knobs. + * @type {Set} + */ +const NON_TOOL_CONFIG_KEYS = new Set(["create_report_incomplete_issue", "create_missing_tool_issue", "create_missing_data_issue", "mentions", "max_bot_mentions"]); + +/** + * Check whether a config key is already covered by a dynamically generated tool that was + * renamed after its target workflow/tool (identified by tool metadata rather than by name). + * @param {Array} tools - Array of tool definitions + * @param {string} normalizedKey - Normalized config key + * @returns {boolean} True when a tool of the same family is already defined + */ +function isConfigKeyCoveredByDynamicTool(tools, normalizedKey) { + const metadataKey = DYNAMIC_TOOL_METADATA_BY_CONFIG_KEY[normalizedKey]; + if (!metadataKey) { + return false; + } + if (metadataKey === "_workflow_name" || metadataKey === "_call_workflow_name") { + return tools.some(tool => tool && hasValidWorkflowMetadataName(tool[metadataKey])); + } + return tools.some(tool => tool && tool[metadataKey]); +} + /** * Check whether workflow metadata name is a non-empty string after trimming. * @param {any} workflowName @@ -336,10 +374,22 @@ function registerDynamicTools(server, tools, config, outputFile, registerTool, n Object.keys(config).forEach(configKey => { const normalizedKey = normalizeTool(configKey); - // Skip if it's already a predefined tool + // Skip config keys that are never exposed as agent-facing tools (handler-only output + // types and global configuration knobs). + if (NON_TOOL_CONFIG_KEYS.has(normalizedKey)) { + server.debug(`Skipping generic tool for non-tool config key: ${configKey}`); + return; + } + + // Skip if it's already a predefined tool, or if a dynamically generated tool named after + // its target (identified by metadata) already covers this config key. if (server.tools[normalizedKey] || tools.find(t => t.name === normalizedKey)) { return; } + if (isConfigKeyCoveredByDynamicTool(tools, normalizedKey)) { + server.debug(`Skipping generic tool for '${configKey}': already covered by dynamically generated tool(s)`); + return; + } const jobConfig = config[configKey]; diff --git a/actions/setup/js/safe_outputs_tools_loader.test.cjs b/actions/setup/js/safe_outputs_tools_loader.test.cjs index 10c6a134bbb..cb658ebd49f 100644 --- a/actions/setup/js/safe_outputs_tools_loader.test.cjs +++ b/actions/setup/js/safe_outputs_tools_loader.test.cjs @@ -819,6 +819,101 @@ describe("safe_outputs_tools_loader", () => { expect(registerTool).not.toHaveBeenCalled(); }); + it("should not register a generic call_workflow tool when a renamed call_workflow tool exists", () => { + const tools = [{ name: "agent_sandbox_stack", description: "Call agent-sandbox-stack", _call_workflow_name: "agent-sandbox-stack" }]; + const config = { + call_workflow: { workflows: ["agent-sandbox-stack"] }, + }; + const outputFile = "/tmp/test-output.jsonl"; + const registerTool = vi.fn(); + const normalizeTool = name => name.replace(/-/g, "_"); + + registerDynamicTools(mockServer, tools, config, outputFile, registerTool, normalizeTool); + + expect(registerTool).not.toHaveBeenCalled(); + }); + + it("should register generic call_workflow tool when renamed call_workflow metadata is invalid", () => { + const tools = [{ name: "agent_sandbox_stack", description: "Call agent-sandbox-stack", _call_workflow_name: " " }]; + const config = { + call_workflow: { workflows: ["agent-sandbox-stack"] }, + }; + const outputFile = "/tmp/test-output.jsonl"; + const registerTool = vi.fn(); + const normalizeTool = name => name.replace(/-/g, "_"); + + registerDynamicTools(mockServer, tools, config, outputFile, registerTool, normalizeTool); + + expect(registerTool).toHaveBeenCalledTimes(1); + expect(registerTool.mock.calls[0][1].name).toBe("call_workflow"); + }); + + it("should not register generic dispatch_workflow or dispatch_repository tools when renamed tools exist", () => { + const tools = [ + { name: "my_workflow", description: "Dispatch my-workflow", _workflow_name: "my-workflow" }, + { name: "my_dispatch", description: "Dispatch repository event", _dispatch_repository_tool: "my_dispatch" }, + ]; + const config = { + dispatch_workflow: { workflows: ["my-workflow"] }, + dispatch_repository: { tools: ["my_dispatch"] }, + }; + const outputFile = "/tmp/test-output.jsonl"; + const registerTool = vi.fn(); + const normalizeTool = name => name.replace(/-/g, "_"); + + registerDynamicTools(mockServer, tools, config, outputFile, registerTool, normalizeTool); + + expect(registerTool).not.toHaveBeenCalled(); + }); + + it("should register generic dispatch_workflow tool when renamed dispatch_workflow metadata is invalid", () => { + const tools = [{ name: "my_workflow", description: "Dispatch my-workflow", _workflow_name: 123 }]; + const config = { + dispatch_workflow: { workflows: ["my-workflow"] }, + }; + const outputFile = "/tmp/test-output.jsonl"; + const registerTool = vi.fn(); + const normalizeTool = name => name.replace(/-/g, "_"); + + registerDynamicTools(mockServer, tools, config, outputFile, registerTool, normalizeTool); + + expect(registerTool).toHaveBeenCalledTimes(1); + expect(registerTool.mock.calls[0][1].name).toBe("dispatch_workflow"); + }); + + it("should not register generic tools for handler-only or global config keys", () => { + const tools = [{ name: "report_incomplete", description: "Report incomplete" }]; + const config = { + report_incomplete: {}, + create_report_incomplete_issue: {}, + mentions: { allowed: [] }, + max_bot_mentions: 3, + }; + const outputFile = "/tmp/test-output.jsonl"; + const registerTool = vi.fn(); + const normalizeTool = name => name.replace(/-/g, "_"); + + registerDynamicTools(mockServer, tools, config, outputFile, registerTool, normalizeTool); + + expect(registerTool).not.toHaveBeenCalled(); + }); + + it("should still register a generic tool for unrelated safe-job config keys", () => { + const tools = [{ name: "agent_sandbox_stack", description: "Call agent-sandbox-stack", _call_workflow_name: "agent-sandbox-stack" }]; + const config = { + call_workflow: { workflows: ["agent-sandbox-stack"] }, + custom_job: { description: "Custom job" }, + }; + const outputFile = "/tmp/test-output.jsonl"; + const registerTool = vi.fn(); + const normalizeTool = name => name.replace(/-/g, "_"); + + registerDynamicTools(mockServer, tools, config, outputFile, registerTool, normalizeTool); + + expect(registerTool).toHaveBeenCalledTimes(1); + expect(registerTool.mock.calls[0][1].name).toBe("custom_job"); + }); + it("should create dynamic tool with input schema", () => { const tools = []; const config = {