fix: honor parenthesized excludeTools and correct docs - #1
fix: honor parenthesized excludeTools and correct docs#1samanyugoyal2010 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
4 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/policy/legacy-tool-syntax.ts">
<violation number="1" location="packages/core/src/policy/legacy-tool-syntax.ts:74">
P2: For non-shell tools, a parenthesized entry like `read_file(/etc/passwd)` or `grep(build)` is turned into a whole-tool DENY, so the entire tool becomes unavailable instead of just the scoped argument being blocked. Because these parenthesized strings previously never matched any tool name, entries like `read_file(/etc/passwd)` in `settings.tools.exclude` or extension `excludeTools` were inert; after this change they silently disable the whole tool, which is broader than the author of the scoped entry intended. Since the args matcher is only meaningful for the shell tool, consider warning and ignoring non-shell parenthesized entries (or emitting a clearer message) instead of globally denying the tool, or document the whole-tool consequence explicitly.</violation>
<violation number="2" location="packages/core/src/policy/legacy-tool-syntax.ts:104">
P1: When a shell command uses a tab after the excluded prefix, the converted deny rule does not match and the command falls through to lower-priority policy rules. Update the argument-prefix pattern generation to recognize JSON-escaped shell whitespace so `run_shell_command(rm)` also denies `rm\t-rf /`.</violation>
</file>
<file name="packages/core/src/utils/extensionLoader.ts">
<violation number="1" location="packages/core/src/utils/extensionLoader.ts:110">
P2: When a parenthesized exclude maps to an unconditional deny, the rule is added after the tool refresh and removed after the refresh. The tool is therefore still advertised after dynamic load and remains hidden after unload; register/remove the rules before refreshing tools, then refresh once.</violation>
</file>
<file name="docs/extensions/reference.md">
<violation number="1" location="docs/extensions/reference.md:163">
P2: The documentation omits the still-supported legacy parenthesized form while describing all entries as whole-tool matches. Document that parenthesized entries are converted to command-deny rules, while recommending `policies/` for new extensions.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| continue; | ||
| } | ||
|
|
||
| const patterns = buildArgsPatterns(undefined, commandPrefix); |
There was a problem hiding this comment.
P1: When a shell command uses a tab after the excluded prefix, the converted deny rule does not match and the command falls through to lower-priority policy rules. Update the argument-prefix pattern generation to recognize JSON-escaped shell whitespace so run_shell_command(rm) also denies rm\t-rf /.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/policy/legacy-tool-syntax.ts, line 104:
<comment>When a shell command uses a tab after the excluded prefix, the converted deny rule does not match and the command falls through to lower-priority policy rules. Update the argument-prefix pattern generation to recognize JSON-escaped shell whitespace so `run_shell_command(rm)` also denies `rm\t-rf /`.</comment>
<file context>
@@ -0,0 +1,128 @@
+ continue;
+ }
+
+ const patterns = buildArgsPatterns(undefined, commandPrefix);
+ for (const pattern of patterns) {
+ if (pattern) {
</file context>
| extensionExcludeToolsPolicySource(extension.name), | ||
| ); | ||
| for (const rule of denyRules) { | ||
| policyEngine.addRule(rule); |
There was a problem hiding this comment.
P2: When a parenthesized exclude maps to an unconditional deny, the rule is added after the tool refresh and removed after the refresh. The tool is therefore still advertised after dynamic load and remains hidden after unload; register/remove the rules before refreshing tools, then refresh once.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/utils/extensionLoader.ts, line 110:
<comment>When a parenthesized exclude maps to an unconditional deny, the rule is added after the tool refresh and removed after the refresh. The tool is therefore still advertised after dynamic load and remains hidden after unload; register/remove the rules before refreshing tools, then refresh once.</comment>
<file context>
@@ -89,6 +95,22 @@ export abstract class ExtensionLoader {
+ extensionExcludeToolsPolicySource(extension.name),
+ );
+ for (const rule of denyRules) {
+ policyEngine.addRule(rule);
+ }
+ }
</file context>
| command. Note that this differs from the MCP server `excludeTools` | ||
| functionality, which can be listed in the MCP server config. | ||
| - `excludeTools`: An array of tool names to exclude from the model. Entries are | ||
| matched against whole tool names, so `"excludeTools": ["run_shell_command"]` |
There was a problem hiding this comment.
P2: The documentation omits the still-supported legacy parenthesized form while describing all entries as whole-tool matches. Document that parenthesized entries are converted to command-deny rules, while recommending policies/ for new extensions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/extensions/reference.md, line 163:
<comment>The documentation omits the still-supported legacy parenthesized form while describing all entries as whole-tool matches. Document that parenthesized entries are converted to command-deny rules, while recommending `policies/` for new extensions.</comment>
<file context>
@@ -159,12 +159,13 @@ The manifest file defines the extension's behavior and configuration.
- command. Note that this differs from the MCP server `excludeTools`
- functionality, which can be listed in the MCP server config.
+- `excludeTools`: An array of tool names to exclude from the model. Entries are
+ matched against whole tool names, so `"excludeTools": ["run_shell_command"]`
+ removes that tool entirely. To restrict individual commands instead of the
+ whole tool, define a rule in your extension's `policies/` directory; see
</file context>
| for (const tool of tools) { | ||
| const match = tool.match(LEGACY_TOOL_ARGS_PATTERN); | ||
| if (!match) { | ||
| rules.push({ |
There was a problem hiding this comment.
P2: For non-shell tools, a parenthesized entry like read_file(/etc/passwd) or grep(build) is turned into a whole-tool DENY, so the entire tool becomes unavailable instead of just the scoped argument being blocked. Because these parenthesized strings previously never matched any tool name, entries like read_file(/etc/passwd) in settings.tools.exclude or extension excludeTools were inert; after this change they silently disable the whole tool, which is broader than the author of the scoped entry intended. Since the args matcher is only meaningful for the shell tool, consider warning and ignoring non-shell parenthesized entries (or emitting a clearer message) instead of globally denying the tool, or document the whole-tool consequence explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/policy/legacy-tool-syntax.ts, line 74:
<comment>For non-shell tools, a parenthesized entry like `read_file(/etc/passwd)` or `grep(build)` is turned into a whole-tool DENY, so the entire tool becomes unavailable instead of just the scoped argument being blocked. Because these parenthesized strings previously never matched any tool name, entries like `read_file(/etc/passwd)` in `settings.tools.exclude` or extension `excludeTools` were inert; after this change they silently disable the whole tool, which is broader than the author of the scoped entry intended. Since the args matcher is only meaningful for the shell tool, consider warning and ignoring non-shell parenthesized entries (or emitting a clearer message) instead of globally denying the tool, or document the whole-tool consequence explicitly.</comment>
<file context>
@@ -0,0 +1,128 @@
+ for (const tool of tools) {
+ const match = tool.match(LEGACY_TOOL_ARGS_PATTERN);
+ if (!match) {
+ rules.push({
+ toolName: normalizeToolName(tool),
+ decision: PolicyDecision.DENY,
</file context>
ea12a7b to
9498dd0
Compare
Extension excludeTools entries are matched by exact tool name, so forms like run_shell_command(rm -rf *) never exclude anything. Update docs and the shipped example to use bare tool names, and point command-level blocking at the policy engine. Co-authored-by: samanyugoyal2010 <samanyugoyal2010@users.noreply.github.com>
tools.exclude and extension excludeTools compared whole tool names, so entries like run_shell_command(rm) were silently ignored. Convert that legacy form into Policy Engine deny rules with command-prefix matching, warn that authors should migrate to policies/, and update shell and enterprise docs that still taught the non-matching syntax. Fixes google-gemini#28962. Also addresses the matching gap reported in google-gemini#17728; command-level control remains the Policy Engine path introduced by google-gemini#18508. Co-authored-by: samanyugoyal2010 <samanyugoyal2010@users.noreply.github.com>
9498dd0 to
6db1316
Compare
Summary
Fixes the misleading extension
excludeToolsdocs reported in google-gemini/gemini-cli#28962.excludeToolsentries are matched by exact tool name equality. Documented forms likerun_shell_command(rm -rf *)never match a tool and are silently ignored, so the dangerous command is not blocked.Changes
docs/extensions/best-practices.md: show barerun_shell_commandfor full-tool exclusion; document apolicies/TOML rule for command-level denydocs/extensions/reference.md: clarify whole-name matching and point command restrictions at the policy enginepackages/cli/.../examples/exclude-tools/gemini-extension.json: use["run_shell_command"]instead of the non-matching parenthesized formNotes
Parenthesized
toolName(args)syntax belongs totools.core/tools.allowed(viamapToolsToRules), not extensionexcludeTools. Command-level blocking for extensions should use the policy engine (policies/directory).Related upstream PR for reference: google-gemini#28963
Summary by cubic
Honor parenthesized entries in extension
excludeToolsand settingstools.excludeby convertingtoolName(args)into Policy Engine deny rules. Previously these never matched and were ignored; now they create command-prefix deny rules and emit a warning, whileexcludeToolsstill matches only whole tool names.Changes
toolName(args)in settings and extensions into deny rules (argsPattern), filter them out of excluded-name sets, and register/unregister these rules via the extension loader under a stable source; add tests."excludeTools": ["run_shell_command"], deprecate command-level use oftools.exclude, direct command restrictions to the Policy Engine, and update the shipped example.Migration
excludeToolsortools.excludewith bare tool names to exclude a whole tool; use policy rules underpolicies/for command-level blocking. Legacy parenthesized entries still work via auto-converted deny rules but warn.Written for commit 6db1316. Summary will update on new commits.