fix(cli): defer tool exclusions to policy engine in non-interactive mode (#20639)

Co-authored-by: Bryan Morgan <bryanmorgan@google.com>
This commit is contained in:
Eric Rahm
2026-03-04 17:01:52 -08:00
committed by GitHub
parent 205d69eb07
commit c72cfad92c
6 changed files with 221 additions and 100 deletions
+20 -27
View File
@@ -953,12 +953,6 @@ describe('mergeMcpServers', () => {
});
describe('mergeExcludeTools', () => {
const defaultExcludes = new Set([
SHELL_TOOL_NAME,
EDIT_TOOL_NAME,
WRITE_FILE_TOOL_NAME,
WEB_FETCH_TOOL_NAME,
]);
const originalIsTTY = process.stdin.isTTY;
beforeEach(() => {
@@ -1080,9 +1074,7 @@ describe('mergeExcludeTools', () => {
process.argv = ['node', 'script.js', '-p', 'test'];
const argv = await parseArguments(createTestMergedSettings());
const config = await loadCliConfig(settings, 'test-session', argv);
expect(config.getExcludeTools()).toEqual(
new Set([...defaultExcludes, ASK_USER_TOOL_NAME]),
);
expect(config.getExcludeTools()).toEqual(new Set([ASK_USER_TOOL_NAME]));
});
it('should handle settings with excludeTools but no extensions', async () => {
@@ -1163,9 +1155,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, 'test-session', argv);
const excludedTools = config.getExcludeTools();
expect(excludedTools).toContain(SHELL_TOOL_NAME);
expect(excludedTools).toContain(EDIT_TOOL_NAME);
expect(excludedTools).toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).not.toContain(SHELL_TOOL_NAME);
expect(excludedTools).not.toContain(EDIT_TOOL_NAME);
expect(excludedTools).not.toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).toContain(ASK_USER_TOOL_NAME);
});
@@ -1184,9 +1176,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, 'test-session', argv);
const excludedTools = config.getExcludeTools();
expect(excludedTools).toContain(SHELL_TOOL_NAME);
expect(excludedTools).toContain(EDIT_TOOL_NAME);
expect(excludedTools).toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).not.toContain(SHELL_TOOL_NAME);
expect(excludedTools).not.toContain(EDIT_TOOL_NAME);
expect(excludedTools).not.toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).toContain(ASK_USER_TOOL_NAME);
});
@@ -1205,7 +1197,7 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, 'test-session', argv);
const excludedTools = config.getExcludeTools();
expect(excludedTools).toContain(SHELL_TOOL_NAME);
expect(excludedTools).not.toContain(SHELL_TOOL_NAME);
expect(excludedTools).not.toContain(EDIT_TOOL_NAME);
expect(excludedTools).not.toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).toContain(ASK_USER_TOOL_NAME);
@@ -1251,9 +1243,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, 'test-session', argv);
const excludedTools = config.getExcludeTools();
expect(excludedTools).toContain(SHELL_TOOL_NAME);
expect(excludedTools).toContain(EDIT_TOOL_NAME);
expect(excludedTools).toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).not.toContain(SHELL_TOOL_NAME);
expect(excludedTools).not.toContain(EDIT_TOOL_NAME);
expect(excludedTools).not.toContain(WRITE_FILE_TOOL_NAME);
expect(excludedTools).toContain(ASK_USER_TOOL_NAME);
});
@@ -1315,9 +1307,10 @@ describe('Approval mode tool exclusion logic', () => {
const excludedTools = config.getExcludeTools();
expect(excludedTools).toContain('custom_tool'); // From settings
expect(excludedTools).toContain(SHELL_TOOL_NAME); // From approval mode
expect(excludedTools).not.toContain(SHELL_TOOL_NAME); // No longer from approval mode
expect(excludedTools).not.toContain(EDIT_TOOL_NAME); // Should be allowed in auto_edit
expect(excludedTools).not.toContain(WRITE_FILE_TOOL_NAME); // Should be allowed in auto_edit
expect(excludedTools).toContain(ASK_USER_TOOL_NAME);
});
it('should throw an error if YOLO mode is attempted when disableYoloMode is true', async () => {
@@ -2164,9 +2157,9 @@ describe('loadCliConfig tool exclusions', () => {
'test-session',
argv,
);
expect(config.getExcludeTools()).toContain('run_shell_command');
expect(config.getExcludeTools()).toContain('replace');
expect(config.getExcludeTools()).toContain('write_file');
expect(config.getExcludeTools()).not.toContain('run_shell_command');
expect(config.getExcludeTools()).not.toContain('replace');
expect(config.getExcludeTools()).not.toContain('write_file');
expect(config.getExcludeTools()).toContain('ask_user');
});
@@ -2204,7 +2197,7 @@ describe('loadCliConfig tool exclusions', () => {
expect(config.getExcludeTools()).not.toContain(SHELL_TOOL_NAME);
});
it('should exclude web-fetch in non-interactive mode when not allowed', async () => {
it('should not exclude web-fetch in non-interactive mode at config level', async () => {
process.stdin.isTTY = false;
process.argv = ['node', 'script.js', '-p', 'test'];
const argv = await parseArguments(createTestMergedSettings());
@@ -2213,7 +2206,7 @@ describe('loadCliConfig tool exclusions', () => {
'test-session',
argv,
);
expect(config.getExcludeTools()).toContain(WEB_FETCH_TOOL_NAME);
expect(config.getExcludeTools()).not.toContain(WEB_FETCH_TOOL_NAME);
});
it('should not exclude web-fetch in non-interactive mode when allowed', async () => {
@@ -3326,11 +3319,11 @@ describe('Policy Engine Integration in loadCliConfig', () => {
await loadCliConfig(settings, 'test-session', argv);
// In non-interactive mode, ShellTool, etc. are excluded
// In non-interactive mode, only ask_user is excluded by default
expect(ServerConfig.createPolicyEngineConfig).toHaveBeenCalledWith(
expect.objectContaining({
tools: expect.objectContaining({
exclude: expect.arrayContaining([SHELL_TOOL_NAME]),
exclude: expect.arrayContaining([ASK_USER_TOOL_NAME]),
}),
}),
expect.anything(),