Skip to content

fix: respect --allowedTools when building disallowed tools list #1033

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

Open
MaxwellCalkin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: respect --allowedTools when building disallowed tools list #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions src/modes/agent/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { mkdir, writeFile } from "fs/promises";
import { prepareMcpConfig } from "../../mcp/install-mcp-server";
import { parseAllowedTools } from "./parse-tools";
import { buildDisallowedToolsString } from "../../create-prompt";
import {
configureGitAuth,
setupSshSigning,
Expand Down Expand Up @@ -118,6 +119,14 @@ export async function prepareAgentMode({
claudeArgs = `--mcp-config '${escapedOurConfig}'`;
}

// Disable WebSearch and WebFetch by default for security, but respect
// user's --allowedTools from claude_args (e.g., --allowedTools WebSearch
// removes WebSearch from the disallowed list)
const disallowedTools = buildDisallowedToolsString([], allowedTools);
Copy link
Copy Markdown
Contributor

@claude claude Bot Mar 31, 2026

Choose a reason for hiding this comment

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

🔴 parseAllowedTools only captures the first value when users use space-separated syntax (--allowedTools "WebSearch" "WebFetch"), but the downstream parseClaudeArgsToExtraArgs correctly captures all values via its accumulating flag logic. This causes the missed tools to appear in both allowedTools and disallowedTools simultaneously, since buildDisallowedToolsString uses the incomplete parse to decide what to disallow. The comma-separated and repeated-flag syntaxes work correctly; only the space-separated multi-value format triggers this conflict.

Extended reasoning...

Bug Analysis

parseAllowedTools in parse-tools.ts uses regex patterns that each require the --allowedTools (or --allowed-tools) prefix immediately before each value. For the input --allowedTools "WebSearch" "WebFetch", the double-quoted regex matches only --allowedTools "WebSearch" because "WebFetch" stands alone without a preceding --allowedTools flag. The same issue affects unquoted space-separated values.

Downstream Conflict

The downstream parser parseClaudeArgsToExtraArgs in base-action/src/parse-sdk-options.ts has explicit support for this syntax. Lines 107-114 show that for ACCUMULATING_FLAGS (which includes allowedTools and disallowedTools), it consumes all consecutive non-flag values after the flag. So for the same input, it correctly captures both WebSearch and WebFetch.

Step-by-Step Proof

  1. User sets CLAUDE_ARGS='--allowedTools "WebSearch" "WebFetch"'
  2. parseAllowedTools returns ["WebSearch"] (misses WebFetch)
  3. buildDisallowedToolsString([], ["WebSearch"]) returns "WebFetch" (WebFetch stays disallowed since it was not detected as allowed)
  4. claudeArgs becomes: --disallowedTools "WebFetch" --allowedTools "WebSearch" "WebFetch"
  5. parseClaudeArgsToExtraArgs correctly parses the accumulating flags and produces disallowedTools: ["WebFetch"] AND allowedTools: ["WebSearch", "WebFetch"]
  6. At lines 241-244 of parse-sdk-options.ts, both mergedAllowedTools and mergedDisallowedTools are returned as-is with no conflict resolution, so WebFetch ends up in both arrays simultaneously

Impact

The behavior when a tool is in both allowedTools and disallowedTools is undefined and depends on the SDK implementation. The user explicitly asked for WebFetch to be allowed, but the system silently adds it to the disallowed list too, potentially overriding user intent. Before this PR, agent mode had no --disallowedTools at all, so this parsing discrepancy was harmless.

Fix

parseAllowedTools should be updated to also consume consecutive non-flag quoted or unquoted values after --allowedTools, matching the accumulating behavior of parseClaudeArgsToExtraArgs. Alternatively, the function could use the same shell-quote + iteration approach that parseClaudeArgsToExtraArgs uses. The PR tests only cover comma-separated and single-value syntaxes, so a test for space-separated multi-value should be added as well.

if (disallowedTools) {
claudeArgs = `${claudeArgs} --disallowedTools "${disallowedTools}"`;
}

// Append user's claude_args (which may have more --mcp-config flags)
claudeArgs = `${claudeArgs} ${userClaudeArgs}`.trim();

Expand Down
108 changes: 105 additions & 3 deletions test/modes/agent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,11 @@ describe("Agent Mode", () => {
githubToken: "test-token",
});

// Verify claude_args includes user args (no MCP config in agent mode without allowed tools)
expect(result.claudeArgs).toBe("--model claude-sonnet-4 --max-turns 10");
// Verify claude_args includes disallowed tools and user args
expect(result.claudeArgs).toContain("--disallowedTools");
expect(result.claudeArgs).toContain("WebSearch");
expect(result.claudeArgs).toContain("WebFetch");
expect(result.claudeArgs).toContain("--model claude-sonnet-4 --max-turns 10");
expect(result.claudeArgs).not.toContain("--mcp-config");

// Verify return structure - should use "main" as fallback when no env vars set
Expand All @@ -97,7 +100,7 @@ describe("Agent Mode", () => {
claudeBranch: undefined,
},
mcpConfig: expect.any(String),
claudeArgs: "--model claude-sonnet-4 --max-turns 10",
claudeArgs: expect.stringContaining("--disallowedTools"),
});

// Clean up
Expand Down Expand Up @@ -203,4 +206,103 @@ describe("Agent Mode", () => {
// Should be empty or just whitespace when no MCP servers are included
expect(result.claudeArgs).not.toContain("--mcp-config");
});


test("--allowedTools WebSearch removes WebSearch from disallowed list", async () => {
const context = createMockAutomationContext({
eventName: "workflow_dispatch",
});

const originalHeadRef = process.env.GITHUB_HEAD_REF;
const originalRefName = process.env.GITHUB_REF_NAME;
delete process.env.GITHUB_HEAD_REF;
delete process.env.GITHUB_REF_NAME;

// Set CLAUDE_ARGS with --allowedTools WebSearch
process.env.CLAUDE_ARGS = '--allowedTools "WebSearch"';

const mockOctokit = {
rest: {
users: {
getAuthenticated: mock(() =>
Promise.resolve({
data: { login: "test-user", id: 12345, type: "User" },
}),
),
getByUsername: mock(() =>
Promise.resolve({
data: { login: "test-user", id: 12345, type: "User" },
}),
),
},
},
} as any;

const result = await prepareAgentMode({
context,
octokit: mockOctokit,
githubToken: "test-token",
});

// WebSearch should NOT be in disallowed tools since user explicitly allowed it
// WebFetch should still be disallowed
expect(result.claudeArgs).toContain("--disallowedTools");
expect(result.claudeArgs).not.toMatch(/--disallowedTools[^"]*WebSearch/);
expect(result.claudeArgs).toContain("WebFetch");

// Clean up
Copy link
Copy Markdown
Contributor

@claude claude Bot Mar 31, 2026

Choose a reason for hiding this comment

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

🟡 The regex /--disallowedTools[^"]*WebSearch/ on line 253 is vacuous — [^"]* stops at the opening quote in --disallowedTools "WebFetch" and can never reach the tool name inside, so not.toMatch(...) always passes regardless of whether WebSearch was actually removed. Fix: use /--disallowedTools\s+"[^"]*WebSearch/ to match inside the quoted value.

Extended reasoning...

What the bug is

The test "--allowedTools WebSearch removes WebSearch from disallowed list" uses a regex assertion to verify that WebSearch is not present in the --disallowedTools value:

expect(result.claudeArgs).not.toMatch(/--disallowedTools[^"]*WebSearch/);

The regex /--disallowedTools[^"]*WebSearch/ matches the literal --disallowedTools, then [^"]* matches zero or more non-quote characters, then looks for WebSearch. However, the production code on line 127 of src/modes/agent/index.ts constructs the string as:

claudeArgs = `${claudeArgs} --disallowedTools "${disallowedTools}"`;

So the actual string looks like --disallowedTools "WebFetch". The [^"]* character class matches only the space between --disallowedTools and the opening ", then stops because " is excluded from the character class. It can never reach inside the quoted value.

Step-by-step proof

Consider the scenario where the code is broken and WebSearch is incorrectly included, producing: --disallowedTools "WebSearch,WebFetch".

  1. The regex engine matches --disallowedTools at the start.
  2. [^"]* starts matching — it matches the space (not a quote), then encounters " — stops.
  3. The engine now expects WebSearch but finds " — no match.
  4. not.toMatch(...) passes because the regex didn't match.

The test passes even though the implementation is wrong. The assertion provides zero verification of the test's stated purpose.

Impact

The test cannot catch regressions where buildDisallowedToolsString fails to remove WebSearch from the disallowed list. The other assertions in the test (toContain("--disallowedTools") and toContain("WebFetch")) provide partial coverage of the output format but do not verify the core behavior being tested — that WebSearch was excluded from the disallowed tools value.

Fix

Replace the regex with one that looks inside the quoted value:

expect(result.claudeArgs).not.toMatch(/--disallowedTools\s+"[^"]*WebSearch/);

This matches --disallowedTools, then whitespace, then the opening quote, then scans inside the quoted value for WebSearch.

delete process.env.CLAUDE_ARGS;
if (originalHeadRef !== undefined)
process.env.GITHUB_HEAD_REF = originalHeadRef;
if (originalRefName !== undefined)
process.env.GITHUB_REF_NAME = originalRefName;
});

test("--allowedTools WebSearch,WebFetch removes both from disallowed list", async () => {
const context = createMockAutomationContext({
eventName: "workflow_dispatch",
});

const originalHeadRef = process.env.GITHUB_HEAD_REF;
const originalRefName = process.env.GITHUB_REF_NAME;
delete process.env.GITHUB_HEAD_REF;
delete process.env.GITHUB_REF_NAME;

// Set CLAUDE_ARGS with both tools allowed
process.env.CLAUDE_ARGS = '--allowedTools "WebSearch,WebFetch"';

const mockOctokit = {
rest: {
users: {
getAuthenticated: mock(() =>
Promise.resolve({
data: { login: "test-user", id: 12345, type: "User" },
}),
),
getByUsername: mock(() =>
Promise.resolve({
data: { login: "test-user", id: 12345, type: "User" },
}),
),
},
},
} as any;

const result = await prepareAgentMode({
context,
octokit: mockOctokit,
githubToken: "test-token",
});

// Neither WebSearch nor WebFetch should be in disallowed tools
// So --disallowedTools should not be present at all
expect(result.claudeArgs).not.toContain("--disallowedTools");

// Clean up
delete process.env.CLAUDE_ARGS;
if (originalHeadRef !== undefined)
process.env.GITHUB_HEAD_REF = originalHeadRef;
if (originalRefName !== undefined)
process.env.GITHUB_REF_NAME = originalRefName;
});
});