feat(core): improve error messages in isCommandAllowed (#3349)

2 files changed, 169 insertions, 76 deletions

diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts
index 936c1f77..f33d3f03 100644
--- a/packages/core/src/tools/shell.test.ts
+++ b/packages/core/src/tools/shell.test.ts
@@ -15,8 +15,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => undefined,
     } as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('ls -l');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('ls -l');
+    expect(result.allowed).toBe(true);
   });
 
   it('should allow a command if it is in the allowed list', async () => {
@@ -25,8 +25,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => undefined,
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('ls -l');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('ls -l');
+    expect(result.allowed).toBe(true);
   });
 
   it('should block a command if it is not in the allowed list', async () => {
@@ -35,8 +35,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => undefined,
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is not in the allowed commands list",
+    );
   });
 
   it('should block a command if it is in the blocked list', async () => {
@@ -45,8 +48,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['ShellTool(rm -rf /)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should allow a command if it is not in the blocked list', async () => {
@@ -55,8 +61,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['ShellTool(rm -rf /)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('ls -l');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('ls -l');
+    expect(result.allowed).toBe(true);
   });
 
   it('should block a command if it is in both the allowed and blocked lists', async () => {
@@ -65,8 +71,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['ShellTool(rm -rf /)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should allow any command when ShellTool is in coreTools without specific commands', async () => {
@@ -75,8 +84,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('any command');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('any command');
+    expect(result.allowed).toBe(true);
   });
 
   it('should block any command when ShellTool is in excludeTools without specific commands', async () => {
@@ -85,8 +94,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['ShellTool'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('any command');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('any command');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      'Shell tool is globally disabled in configuration',
+    );
   });
 
   it('should allow a command if it is in the allowed list using the public-facing name', async () => {
@@ -95,8 +107,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => undefined,
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('ls -l');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('ls -l');
+    expect(result.allowed).toBe(true);
   });
 
   it('should block a command if it is in the blocked list using the public-facing name', async () => {
@@ -105,8 +117,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['run_shell_command(rm -rf /)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should block any command when ShellTool is in excludeTools using the public-facing name', async () => {
@@ -115,8 +130,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['run_shell_command'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('any command');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('any command');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      'Shell tool is globally disabled in configuration',
+    );
   });
 
   it('should block any command if coreTools contains an empty ShellTool command list using the public-facing name', async () => {
@@ -125,8 +143,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('any command');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('any command');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'any command' is not in the allowed commands list",
+    );
   });
 
   it('should block any command if coreTools contains an empty ShellTool command list', async () => {
@@ -135,8 +156,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('any command');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('any command');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'any command' is not in the allowed commands list",
+    );
   });
 
   it('should block a command with extra whitespace if it is in the blocked list', async () => {
@@ -145,8 +169,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['ShellTool(rm -rf /)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed(' rm  -rf  / ');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed(' rm  -rf  / ');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should allow any command when ShellTool is present with specific commands', async () => {
@@ -155,8 +182,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('any command');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('any command');
+    expect(result.allowed).toBe(true);
   });
 
   it('should block a command on the blocklist even with a wildcard allow', async () => {
@@ -165,8 +192,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['ShellTool(rm -rf /)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should allow a command that starts with an allowed command prefix', async () => {
@@ -175,10 +205,10 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed(
+    const result = shellTool.isCommandAllowed(
       'gh issue edit 1 --add-label "kind/feature"',
     );
-    expect(isAllowed).toBe(true);
+    expect(result.allowed).toBe(true);
   });
 
   it('should allow a command that starts with an allowed command prefix using the public-facing name', async () => {
@@ -187,10 +217,10 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed(
+    const result = shellTool.isCommandAllowed(
       'gh issue edit 1 --add-label "kind/feature"',
     );
-    expect(isAllowed).toBe(true);
+    expect(result.allowed).toBe(true);
   });
 
   it('should not allow a command that starts with an allowed command prefix but is chained with another command', async () => {
@@ -199,8 +229,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('gh issue edit&&rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('gh issue edit&&rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is not in the allowed commands list",
+    );
   });
 
   it('should not allow a command that is a prefix of an allowed command', async () => {
@@ -209,8 +242,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('gh issue');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('gh issue');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'gh issue' is not in the allowed commands list",
+    );
   });
 
   it('should not allow a command that is a prefix of a blocked command', async () => {
@@ -219,8 +255,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['run_shell_command(gh issue edit)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('gh issue');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('gh issue');
+    expect(result.allowed).toBe(true);
   });
 
   it('should not allow a command that is chained with a pipe', async () => {
@@ -229,8 +265,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('gh issue list | rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('gh issue list | rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is not in the allowed commands list",
+    );
   });
 
   it('should not allow a command that is chained with a semicolon', async () => {
@@ -239,8 +278,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('gh issue list; rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('gh issue list; rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is not in the allowed commands list",
+    );
   });
 
   it('should block a chained command if any part is blocked', async () => {
@@ -249,8 +291,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['run_shell_command(rm)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('echo "hello" && rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('echo "hello" && rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should block a command if its prefix is on the blocklist, even if the command itself is on the allowlist', async () => {
@@ -259,8 +304,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['run_shell_command(git)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('git push');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('git push');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'git push' is blocked by configuration",
+    );
   });
 
   it('should be case-sensitive in its matching', async () => {
@@ -269,8 +317,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('ECHO "hello"');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('ECHO "hello"');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      'Command \'ECHO "hello"\' is not in the allowed commands list',
+    );
   });
 
   it('should correctly handle commands with extra whitespace around chaining operators', async () => {
@@ -279,8 +330,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => ['run_shell_command(rm)'],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('ls -l  ;  rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('ls -l  ;  rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is blocked by configuration",
+    );
   });
 
   it('should allow a chained command if all parts are allowed', async () => {
@@ -292,8 +346,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('echo "hello" && ls -l');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('echo "hello" && ls -l');
+    expect(result.allowed).toBe(true);
   });
 
   it('should block a command with command substitution using backticks', async () => {
@@ -302,8 +356,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('echo `rm -rf /`');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('echo `rm -rf /`');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      'Command substitution using backticks is not allowed for security reasons',
+    );
   });
 
   it('should block a command with command substitution using $()', async () => {
@@ -312,8 +369,11 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('echo $(rm -rf /)');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('echo $(rm -rf /)');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      'Command substitution using $() is not allowed for security reasons',
+    );
   });
 
   it('should allow a command with I/O redirection', async () => {
@@ -322,8 +382,8 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('echo "hello" > file.txt');
-    expect(isAllowed).toBe(true);
+    const result = shellTool.isCommandAllowed('echo "hello" > file.txt');
+    expect(result.allowed).toBe(true);
   });
 
   it('should not allow a command that is chained with a double pipe', async () => {
@@ -332,7 +392,10 @@ describe('ShellTool', () => {
       getExcludeTools: () => [],
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const isAllowed = shellTool.isCommandAllowed('gh issue list || rm -rf /');
-    expect(isAllowed).toBe(false);
+    const result = shellTool.isCommandAllowed('gh issue list || rm -rf /');
+    expect(result.allowed).toBe(false);
+    expect(result.reason).toBe(
+      "Command 'rm -rf /' is not in the allowed commands list",
+    );
   });
 });
diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts
index f0a30a9c..4954e055 100644
--- a/packages/core/src/tools/shell.ts
+++ b/packages/core/src/tools/shell.ts
@@ -112,12 +112,23 @@ Process Group PGID: Process group started or \`(none)\``,
    * the tool's configuration including allowlists and blocklists.
    *
    * @param command The shell command string to validate
-   * @returns True if the command is allowed to execute, false otherwise
+   * @returns An object with 'allowed' boolean and optional 'reason' string if not allowed
    */
-  isCommandAllowed(command: string): boolean {
+  isCommandAllowed(command: string): { allowed: boolean; reason?: string } {
     // 0. Disallow command substitution
-    if (command.includes('$(') || command.includes('`')) {
-      return false;
+    if (command.includes('$(')) {
+      return {
+        allowed: false,
+        reason:
+          'Command substitution using $() is not allowed for security reasons',
+      };
+    }
+    if (command.includes('`')) {
+      return {
+        allowed: false,
+        reason:
+          'Command substitution using backticks is not allowed for security reasons',
+      };
     }
 
     const SHELL_TOOL_NAMES = [ShellTool.name, ShellTool.Name];
@@ -157,7 +168,10 @@ Process Group PGID: Process group started or \`(none)\``,
 
     // 1. Check if the shell tool is globally disabled.
     if (SHELL_TOOL_NAMES.some((name) => excludeTools.includes(name))) {
-      return false;
+      return {
+        allowed: false,
+        reason: 'Shell tool is globally disabled in configuration',
+      };
     }
 
     const blockedCommands = new Set(extractCommands(excludeTools));
@@ -170,35 +184,51 @@ Process Group PGID: Process group started or \`(none)\``,
 
     const commandsToValidate = command.split(/&&|\|\||\||;/).map(normalize);
 
+    const blockedCommandsArr = [...blockedCommands];
+
     for (const cmd of commandsToValidate) {
       // 2. Check if the command is on the blocklist.
-      const isBlocked = [...blockedCommands].some((blocked) =>
+      const isBlocked = blockedCommandsArr.some((blocked) =>
         isPrefixedBy(cmd, blocked),
       );
       if (isBlocked) {
-        return false;
+        return {
+          allowed: false,
+          reason: `Command '${cmd}' is blocked by configuration`,
+        };
       }
 
       // 3. If in strict allow-list mode, check if the command is permitted.
       const isStrictAllowlist =
         hasSpecificAllowedCommands && !isWildcardAllowed;
+      const allowedCommandsArr = [...allowedCommands];
       if (isStrictAllowlist) {
-        const isAllowed = [...allowedCommands].some((allowed) =>
+        const isAllowed = allowedCommandsArr.some((allowed) =>
           isPrefixedBy(cmd, allowed),
         );
         if (!isAllowed) {
-          return false;
+          return {
+            allowed: false,
+            reason: `Command '${cmd}' is not in the allowed commands list`,
+          };
         }
       }
     }
 
     // 4. If all checks pass, the command is allowed.
-    return true;
+    return { allowed: true };
   }
 
   validateToolParams(params: ShellToolParams): string | null {
-    if (!this.isCommandAllowed(params.command)) {
-      return `Command is not allowed: ${params.command}`;
+    const commandCheck = this.isCommandAllowed(params.command);
+    if (!commandCheck.allowed) {
+      if (!commandCheck.reason) {
+        console.error(
+          'Unexpected: isCommandAllowed returned false without a reason',
+        );
+        return `Command is not allowed: ${params.command}`;
+      }
+      return commandCheck.reason;
     }
     if (
       !SchemaValidator.validate(