fix: bash interceptor regex bugs and add unit tests
- Fix cat rule to exclude heredoc syntax (cat <<EOF) via negative lookahead - Fix write rule: exclude >> append and digit-prefixed fd redirects (2>) using lookbehind (?<![|>\d])>(?!>) - Add compileInterceptor() — pre-compiles rules once at construction time instead of on every bash call; export CompiledInterceptor type - Update createBashTool to use pre-compiled interceptor instance - Add 33 unit tests covering all rules, edge cases, and pass-throughs
This commit is contained in:
parent
d0f84d9a38
commit
e55b6dd994
5 changed files with 249 additions and 25 deletions
198
packages/pi-coding-agent/src/core/tools/bash-interceptor.test.ts
Normal file
198
packages/pi-coding-agent/src/core/tools/bash-interceptor.test.ts
Normal file
|
|
@ -0,0 +1,198 @@
|
|||
import { describe, it } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import {
|
||||
checkBashInterception,
|
||||
compileInterceptor,
|
||||
DEFAULT_BASH_INTERCEPTOR_RULES,
|
||||
type BashInterceptorRule,
|
||||
} from "./bash-interceptor.js";
|
||||
|
||||
const ALL_TOOLS = ["read", "grep", "find", "edit", "write"];
|
||||
const NO_TOOLS: string[] = [];
|
||||
|
||||
describe("checkBashInterception", () => {
|
||||
describe("read rule (cat/head/tail/less/more)", () => {
|
||||
it("blocks cat with a file argument", () => {
|
||||
const r = checkBashInterception("cat README.md", ALL_TOOLS);
|
||||
assert.equal(r.block, true);
|
||||
assert.equal(r.suggestedTool, "read");
|
||||
});
|
||||
|
||||
it("blocks head and tail", () => {
|
||||
assert.equal(checkBashInterception("head -n 20 file.ts", ALL_TOOLS).block, true);
|
||||
assert.equal(checkBashInterception("tail -f app.log", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("does NOT block cat used as heredoc (cat <<EOF)", () => {
|
||||
const r = checkBashInterception("cat <<EOF > file.txt", ALL_TOOLS);
|
||||
assert.notEqual(r.suggestedTool, "read");
|
||||
});
|
||||
|
||||
it("does NOT block when read tool is absent", () => {
|
||||
assert.equal(checkBashInterception("cat README.md", NO_TOOLS).block, false);
|
||||
assert.equal(checkBashInterception("cat README.md", ["grep"]).block, false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("grep rule", () => {
|
||||
it("blocks grep and rg", () => {
|
||||
assert.equal(checkBashInterception("grep foo bar.ts", ALL_TOOLS).block, true);
|
||||
assert.equal(checkBashInterception("rg -r pattern .", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("blocks grep with leading whitespace", () => {
|
||||
assert.equal(checkBashInterception(" grep -r foo .", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("does NOT block when grep tool is absent", () => {
|
||||
assert.equal(checkBashInterception("grep foo bar", ["read", "edit"]).block, false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("find rule", () => {
|
||||
it("blocks find with -name flag", () => {
|
||||
assert.equal(checkBashInterception('find . -name "*.ts"', ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("blocks find with -type flag", () => {
|
||||
assert.equal(checkBashInterception("find /tmp -maxdepth 1 -type f", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("does NOT block find without name/type flags", () => {
|
||||
assert.equal(checkBashInterception("find /tmp -maxdepth 1", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("does NOT block when find tool is absent", () => {
|
||||
assert.equal(checkBashInterception('find . -name "*.ts"', ["read", "grep"]).block, false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("edit rule (sed/perl/awk)", () => {
|
||||
it("blocks sed -i", () => {
|
||||
assert.equal(checkBashInterception("sed -i 's/foo/bar/' file.ts", ALL_TOOLS).block, true);
|
||||
assert.equal(checkBashInterception("sed --in-place 's/x/y/' f", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("does NOT block sed without -i (read-only)", () => {
|
||||
assert.equal(checkBashInterception("sed 's/foo/bar/' file.ts", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("blocks perl -pi and perl -p -i", () => {
|
||||
assert.equal(checkBashInterception("perl -pi -e 's/foo/bar/' file", ALL_TOOLS).block, true);
|
||||
assert.equal(checkBashInterception("perl -p -i -e 's/x/y/' f", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("blocks awk -i inplace", () => {
|
||||
assert.equal(checkBashInterception("awk -i inplace '{print}' file", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("does NOT block when edit tool is absent", () => {
|
||||
assert.equal(checkBashInterception("sed -i 's/a/b/' f", ["read", "grep"]).block, false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("write rule (echo/printf/heredoc redirect)", () => {
|
||||
it("blocks echo with > redirect", () => {
|
||||
assert.equal(checkBashInterception("echo hello > file.txt", ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("blocks printf with > redirect", () => {
|
||||
assert.equal(checkBashInterception('printf "%s" content > out.txt', ALL_TOOLS).block, true);
|
||||
});
|
||||
|
||||
it("does NOT block echo without redirect", () => {
|
||||
assert.equal(checkBashInterception("echo hello", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("does NOT block >> append redirect (write tool does not support appending)", () => {
|
||||
assert.equal(checkBashInterception("echo hello >> file.txt", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("does NOT block stderr redirect (2>)", () => {
|
||||
assert.equal(checkBashInterception("echo test 2> /dev/null", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("does NOT block pipe (echo foo | grep bar)", () => {
|
||||
assert.equal(checkBashInterception("echo foo | grep bar", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("does NOT block when write tool is absent", () => {
|
||||
assert.equal(checkBashInterception("echo hello > file.txt", ["read", "grep"]).block, false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("pass-through commands", () => {
|
||||
it("passes npm install", () => {
|
||||
assert.equal(checkBashInterception("npm install", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("passes ls > output.txt (not an echo/printf/cat)", () => {
|
||||
assert.equal(checkBashInterception("ls > output.txt", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("passes tee file.txt", () => {
|
||||
assert.equal(checkBashInterception("tee file.txt", ALL_TOOLS).block, false);
|
||||
});
|
||||
|
||||
it("passes git log", () => {
|
||||
assert.equal(checkBashInterception("git log --oneline", ALL_TOOLS).block, false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("block message content", () => {
|
||||
it("includes the original command in the block message", () => {
|
||||
const r = checkBashInterception("cat README.md", ALL_TOOLS);
|
||||
assert.ok(r.message?.includes("cat README.md"), "message should contain original command");
|
||||
});
|
||||
|
||||
it("returns block:false with no message when not blocked", () => {
|
||||
const r = checkBashInterception("npm install", ALL_TOOLS);
|
||||
assert.equal(r.block, false);
|
||||
assert.equal(r.message, undefined);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("compileInterceptor", () => {
|
||||
it("produces same results as checkBashInterception", () => {
|
||||
const interceptor = compileInterceptor(DEFAULT_BASH_INTERCEPTOR_RULES);
|
||||
const cases: [string, string[], boolean][] = [
|
||||
["cat README.md", ALL_TOOLS, true],
|
||||
["npm install", ALL_TOOLS, false],
|
||||
["grep foo bar", ALL_TOOLS, true],
|
||||
["echo hello >> file", ALL_TOOLS, false],
|
||||
["echo test 2> /dev/null", ALL_TOOLS, false],
|
||||
];
|
||||
for (const [cmd, tools, expected] of cases) {
|
||||
assert.equal(
|
||||
interceptor.check(cmd, tools).block,
|
||||
expected,
|
||||
`pre-compiled: "${cmd}" expected block=${expected}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it("silently skips rules with invalid regex patterns", () => {
|
||||
const rules: BashInterceptorRule[] = [
|
||||
{ pattern: "[invalid(", tool: "read", message: "broken" },
|
||||
{ pattern: "^\\s*cat\\s+", tool: "read", message: "valid" },
|
||||
];
|
||||
const interceptor = compileInterceptor(rules);
|
||||
assert.equal(interceptor.check("cat file.txt", ["read"]).block, true);
|
||||
});
|
||||
|
||||
it("returns block:false when available tools list is empty", () => {
|
||||
const interceptor = compileInterceptor(DEFAULT_BASH_INTERCEPTOR_RULES);
|
||||
assert.equal(interceptor.check("cat README.md", []).block, false);
|
||||
});
|
||||
|
||||
it("allows custom rule override", () => {
|
||||
const customRules: BashInterceptorRule[] = [
|
||||
{ pattern: "^\\s*curl\\s+", tool: "fetch", message: "Use fetch tool instead." },
|
||||
];
|
||||
const interceptor = compileInterceptor(customRules);
|
||||
assert.equal(interceptor.check("curl https://example.com", ["fetch"]).block, true);
|
||||
// default rules not active
|
||||
assert.equal(interceptor.check("cat file.txt", ["read"]).block, false);
|
||||
});
|
||||
});
|
||||
|
|
@ -14,7 +14,8 @@ export interface BashInterceptorRule {
|
|||
|
||||
export const DEFAULT_BASH_INTERCEPTOR_RULES: BashInterceptorRule[] = [
|
||||
{
|
||||
pattern: "^\\s*(cat|head|tail|less|more)\\s+",
|
||||
// cat/head/tail for file viewing — excludes heredoc syntax (cat <<)
|
||||
pattern: "^\\s*(cat(?!\\s*<<)|head|tail|less|more)\\s+",
|
||||
tool: "read",
|
||||
message: "Use the read tool to view file contents instead of shell commands.",
|
||||
},
|
||||
|
|
@ -44,7 +45,9 @@ export const DEFAULT_BASH_INTERCEPTOR_RULES: BashInterceptorRule[] = [
|
|||
message: "Use the edit tool for in-place file modifications instead of awk.",
|
||||
},
|
||||
{
|
||||
pattern: "^\\s*(echo|printf|cat\\s*<<)\\s+.*[^|]>\\s*\\S",
|
||||
// echo/printf/heredoc writing to a file via > (not >> append, not 2> stderr redirect)
|
||||
// Matches a single > not preceded by |, >, or a digit (fd redirect like 2>)
|
||||
pattern: "^\\s*(echo|printf|cat\\s*<<)\\s+.*(?<![|>\\d])>(?!>)\\s*\\S",
|
||||
tool: "write",
|
||||
message: "Use the write tool to create/overwrite files instead of shell redirects.",
|
||||
},
|
||||
|
|
@ -56,22 +59,48 @@ export interface InterceptionResult {
|
|||
suggestedTool?: string;
|
||||
}
|
||||
|
||||
export interface CompiledInterceptor {
|
||||
check: (command: string, availableTools: string[]) => InterceptionResult;
|
||||
}
|
||||
|
||||
/**
|
||||
* Compile rules into regex objects, silently skipping invalid patterns.
|
||||
* Compile rules into an interceptor with pre-built regex objects.
|
||||
* Silently skips rules with invalid patterns.
|
||||
*
|
||||
* Pre-compiling at construction time avoids repeated `new RegExp()` calls
|
||||
* on every bash command invocation.
|
||||
*/
|
||||
function compileRules(rules: BashInterceptorRule[]): Array<{ regex: RegExp; rule: BashInterceptorRule }> {
|
||||
return rules.flatMap((rule) => {
|
||||
export function compileInterceptor(rules: BashInterceptorRule[]): CompiledInterceptor {
|
||||
const compiled = rules.flatMap((rule) => {
|
||||
try {
|
||||
return [{ regex: new RegExp(rule.pattern, rule.flags), rule }];
|
||||
} catch {
|
||||
return []; // skip invalid regex
|
||||
}
|
||||
});
|
||||
|
||||
return {
|
||||
check(command: string, availableTools: string[]): InterceptionResult {
|
||||
const trimmed = command.trim();
|
||||
for (const { regex, rule } of compiled) {
|
||||
if (regex.test(trimmed) && availableTools.includes(rule.tool)) {
|
||||
return {
|
||||
block: true,
|
||||
message: `Blocked: ${rule.message}\n\nOriginal command: ${command}`,
|
||||
suggestedTool: rule.tool,
|
||||
};
|
||||
}
|
||||
}
|
||||
return { block: false };
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether a bash command should be intercepted.
|
||||
*
|
||||
* Compiles rules on each call — prefer `compileInterceptor()` for repeated use.
|
||||
*
|
||||
* @param command - The shell command to check
|
||||
* @param availableTools - Tool names present in the current session
|
||||
* @param rules - Override the default rule set (optional)
|
||||
|
|
@ -82,18 +111,5 @@ export function checkBashInterception(
|
|||
rules?: BashInterceptorRule[],
|
||||
): InterceptionResult {
|
||||
const effectiveRules = rules ?? DEFAULT_BASH_INTERCEPTOR_RULES;
|
||||
const compiled = compileRules(effectiveRules);
|
||||
const trimmed = command.trim();
|
||||
|
||||
for (const { regex, rule } of compiled) {
|
||||
if (regex.test(trimmed) && availableTools.includes(rule.tool)) {
|
||||
return {
|
||||
block: true,
|
||||
message: `Blocked: ${rule.message}\n\nOriginal command: ${command}`,
|
||||
suggestedTool: rule.tool,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
return { block: false };
|
||||
return compileInterceptor(effectiveRules).check(command, availableTools);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ import type { AgentTool } from "@gsd/pi-agent-core";
|
|||
import { type Static, Type } from "@sinclair/typebox";
|
||||
import { spawn } from "child_process";
|
||||
import { getShellConfig, getShellEnv, killProcessTree, sanitizeCommand } from "../../utils/shell.js";
|
||||
import { type BashInterceptorRule, checkBashInterception } from "./bash-interceptor.js";
|
||||
import { type BashInterceptorRule, compileInterceptor, DEFAULT_BASH_INTERCEPTOR_RULES } from "./bash-interceptor.js";
|
||||
import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, type TruncationResult, truncateTail } from "./truncate.js";
|
||||
import type { ArtifactManager } from "../artifact-manager.js";
|
||||
|
||||
|
|
@ -207,6 +207,12 @@ export function createBashTool(cwd: string, options?: BashToolOptions): AgentToo
|
|||
const spawnHook = options?.spawnHook;
|
||||
const artifactManager = options?.artifactManager;
|
||||
|
||||
// Pre-compile interceptor rules once at construction time
|
||||
const interceptorInstance =
|
||||
options?.interceptor?.enabled
|
||||
? compileInterceptor(options.interceptor.rules ?? DEFAULT_BASH_INTERCEPTOR_RULES)
|
||||
: null;
|
||||
|
||||
return {
|
||||
name: "bash",
|
||||
label: "bash",
|
||||
|
|
@ -219,12 +225,12 @@ export function createBashTool(cwd: string, options?: BashToolOptions): AgentToo
|
|||
onUpdate?,
|
||||
) => {
|
||||
// Check bash interceptor — block commands that duplicate dedicated tools
|
||||
if (options?.interceptor?.enabled) {
|
||||
if (interceptorInstance) {
|
||||
const toolNames =
|
||||
typeof options.availableToolNames === "function"
|
||||
? options.availableToolNames()
|
||||
: options.availableToolNames ?? [];
|
||||
const interception = checkBashInterception(command, toolNames, options.interceptor.rules);
|
||||
typeof options!.availableToolNames === "function"
|
||||
? options!.availableToolNames()
|
||||
: options!.availableToolNames ?? [];
|
||||
const interception = interceptorInstance.check(command, toolNames);
|
||||
if (interception.block) {
|
||||
return {
|
||||
content: [{ type: "text" as const, text: interception.message ?? "Command blocked by interceptor" }],
|
||||
|
|
|
|||
|
|
@ -11,6 +11,8 @@ export {
|
|||
export {
|
||||
type BashInterceptorRule,
|
||||
checkBashInterception,
|
||||
type CompiledInterceptor,
|
||||
compileInterceptor,
|
||||
DEFAULT_BASH_INTERCEPTOR_RULES,
|
||||
type InterceptionResult,
|
||||
} from "./bash-interceptor.js";
|
||||
|
|
|
|||
|
|
@ -230,6 +230,8 @@ export {
|
|||
type BashToolOptions,
|
||||
bashTool,
|
||||
checkBashInterception,
|
||||
type CompiledInterceptor,
|
||||
compileInterceptor,
|
||||
DEFAULT_BASH_INTERCEPTOR_RULES,
|
||||
codingTools,
|
||||
DEFAULT_MAX_BYTES,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue