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-duplicate-generic-safe-output-tools.md

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

52 changes: 51 additions & 1 deletion actions/setup/js/safe_outputs_tools_loader.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>}
*/
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<string>}
*/
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) {

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.

L82-88: yagni: isConfigKeyCoveredByDynamicTool has exactly one caller (L386). Inline the tools.some(tool => tool && tool[metadataKey]) check at the call site.

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
Expand Down Expand Up @@ -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];

Expand Down
95 changes: 95 additions & 0 deletions actions/setup/js/safe_outputs_tools_loader.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
Loading