1364f8c1cd3435a5c41f997dda74453df1c22896
- Author
- TheEdgeOfRage <git@theedgeofrage.com>
- Committer
- TheEdgeOfRage <git@theedgeofrage.com>
- Date
Message
Improve Bash policy diagnostics
Diff
This diff is truncated to protect this page.
1diff --git a/README.md b/README.md
2index 81d7a0f6cc6fc7cdc473803c2ce25524d9e36014..285a47ffe078cc76a04a706409f37aa958398063 100644
3--- a/README.md
4+++ b/README.md
5@@ -93,7 +93,7 @@ Pi uses a root-level `tools` map and `externalDirectories` list. Any tool name c
6
7diff --git a/src/core/deterministic.ts b/src/core/deterministic.ts
8index d6032f4cfe40476de565855f92a74edbd2c68839..6b5904ac796ec9f4c3a34a05d25bc3154c5297ad 100644
9--- a/src/core/deterministic.ts
10+++ b/src/core/deterministic.ts
11@@ -92,10 +92,17 @@ function variableCharacter(character: string | undefined): boolean {
12 return character !== undefined && /[A-Za-z0-9_]/.test(character);
13 }
14
15-function hasUnsafeWordSyntax(command: string): boolean {
16+function isStaticOptionAssignment(prefix: string): boolean {
17+ return /^--[A-Za-z0-9][A-Za-z0-9_-]*=$/.test(prefix);
18+}
19+
20+function hasUnsafeWordSyntax(command: string, allowStaticGlobs = false): boolean {
21 let quote: "'" | '"' | undefined;
22 let wordStarted = false;
23 let quotedWord = false;
24+ let wordStart = 0;
25+ let quotedOptionValue = false;
26+ let optionValueQuoteClosed = false;
27
28 for (let index = 0; index < command.length; index += 1) {
29 const character = command[index];
30@@ -104,6 +111,7 @@ function hasUnsafeWordSyntax(command: string): boolean {
31 if (quote === "'") {
32 if (character === "'") {
33 quote = undefined;
34+ if (quotedOptionValue) optionValueQuoteClosed = true;
35 continue;
36 }
37 if (character === "$" || character === "~") return true;
38@@ -113,6 +121,7 @@ function hasUnsafeWordSyntax(command: string): boolean {
39 if (quote === '"') {
40 if (character === '"') {
41 quote = undefined;
42+ if (quotedOptionValue) optionValueQuoteClosed = true;
43 continue;
44 }
45 if (character === "\\") {
46@@ -124,6 +133,10 @@ function hasUnsafeWordSyntax(command: string): boolean {
47 }
48 if (character === "`") return true;
49 if (character === "$") {
50+ if (nextCharacter === "?") {
51+ index += 1;
52+ continue;
53+ }
54 if (command.slice(index + 1, index + 5) !== "HOME" || variableCharacter(command[index + 5])) return true;
55 index += 4;
56 continue;
57@@ -136,14 +149,20 @@ function hasUnsafeWordSyntax(command: string): boolean {
58 quote = undefined;
59 wordStarted = false;
60 quotedWord = false;
61+ quotedOptionValue = false;
62+ optionValueQuoteClosed = false;
63 continue;
64 }
65- if (quotedWord) return true;
66+ if (optionValueQuoteClosed || quotedWord) return true;
67 if (character === "'" || character === '"') {
68- if (wordStarted) return true;
69+ const wordPrefix = command.slice(wordStart, index);
70+ const isOptionValue = wordStarted && isStaticOptionAssignment(wordPrefix);
71+ if (wordStarted && !isOptionValue) return true;
72 quote = character;
73+ if (!wordStarted) wordStart = index;
74 wordStarted = true;
75- quotedWord = true;
76+ quotedOptionValue = isOptionValue;
77+ quotedWord = !quotedOptionValue;
78 continue;
79 }
80 if (character === "\\") {
81@@ -154,19 +173,23 @@ function hasUnsafeWordSyntax(command: string): boolean {
82 }
83 if (character === "`" || SHELL_CONTROL_RE.test(character) || ["{", "}", "(", ")"].includes(character)) return true;
84 if (character === "$") {
85+ if (nextCharacter === "?") {
86+ index += 1;
87+ wordStarted = true;
88+ continue;
89+ }
90 if (command.slice(index + 1, index + 5) !== "HOME" || variableCharacter(command[index + 5])) return true;
91 index += 4;
92 wordStarted = true;
93 continue;
94 }
95- if (character === "*") return true;
96- if (character === "?") return true;
97- if (character === "[") return true;
98+ if (!allowStaticGlobs && (character === "*" || character === "?" || character === "[")) return true;
99 if (character === "~") {
100 if (wordStarted || (nextCharacter !== undefined && nextCharacter !== "/" && !/\s/.test(nextCharacter))) {
101 return true;
102 }
103 }
104+ if (!wordStarted) wordStart = index;
105 wordStarted = true;
106 }
107
108@@ -222,8 +245,14 @@ function positionalWords(words: string[], valueOptions: ReadonlySet<string> = ne
109 function hasSecretOptionValue(arguments_: string[], optionNames: ReadonlySet<string>): boolean {
110 for (let index = 0; index < arguments_.length; index += 1) {
111diff --git a/src/core/review.ts b/src/core/review.ts
112index 66acdb1732e00008133243f452969635aa6c4d3f..bd8ec8bebec7c457babe112254d4613c25b6ffb1 100644
113--- a/src/core/review.ts
114+++ b/src/core/review.ts
115@@ -72,7 +72,8 @@ export function createPolicyReviewer(
116 return {
117 decision: {
118 decision: "ask",
119- reason: "LLM reviews are disabled",
120+ reason:
121+ "Request fell through static permissions, deterministic rules, and the decision cache; LLM reviews are disabled",
122 category: "uncertain",
123 },
124 };
125diff --git a/src/pi/bash-split.ts b/src/pi/bash-split.ts
126index de0e73f73c298b275b21856767c11ce3ddbb94b0..111aa1b9248f4ca9d71cde0d78c3d59919570f6c 100644
127--- a/src/pi/bash-split.ts
128+++ b/src/pi/bash-split.ts
129@@ -10,6 +10,7 @@ export type BashSplitCommand = {
130 export type BashSplitResult = {
131 commands: BashSplitCommand[];
132 parsed: boolean;
133+ parserUnavailable?: boolean;
134 };
135
136 type BashParser = {
137@@ -95,12 +96,22 @@ function raw(command: string): BashSplitResult {
138 return { commands: [{ source: command, redirects: [] }], parsed: false };
139 }
140
141+function parserUnavailable(command: string): BashSplitResult {
142+ return { ...raw(command), parserUnavailable: true };
143+}
144+
145 export async function splitBashCommand(
146 command: string,
147 loadParser: BashParserLoader = loadBashParser,
148 ): Promise<BashSplitResult> {
149+ let parser: BashParser;
150+ try {
151+ parser = await loadParser();
152+ } catch {
153+ return parserUnavailable(command);
154+ }
155+
156 try {
157- const parser = await loadParser();
158 const tree = parser.parse(command);
159 if (!tree || tree.rootNode.hasError) return raw(command);
160 try {
161diff --git a/src/pi/index.ts b/src/pi/index.ts
162index f5d91cb42317e712c33c1128f132aa3ff7ef7165..f1c2cea8df1d330af0dab48ed003b2a2027d35e4 100644
163--- a/src/pi/index.ts
164+++ b/src/pi/index.ts
165@@ -161,6 +161,17 @@ export async function evaluatePiToolCall(
166 }
167
168 const split = await splitBash(input.command);
169+ if (split.parserUnavailable) {
170+ return {
171+ decision: {
172+ decision: "deny",
173+ reason: "Bash parser is unavailable",
174+ category: "bash",
175+ },
176+ source: "deterministic",
177+ };
178+ }
179+
180 const requests = split.commands.map((splitCommand) => {
181 const commandInput = { ...input, command: splitCommand.source };
182 bashRedirects.set(commandInput, splitCommand.redirects);
183diff --git a/test/core/deterministic.test.ts b/test/core/deterministic.test.ts
184index 44799577c4f9fa4fc7a056705db74146a845edbc..ff8465775814ecac2306a97e46d53968aa1e95ac 100644
185--- a/test/core/deterministic.test.ts
186+++ b/test/core/deterministic.test.ts
187@@ -75,6 +75,42 @@ test("allows recognized deterministic command grammars", () => {
188 }
189 });
190
191+test("allows static pathname globs only for ls", () => {
192+ expect(checkDeterministic("bash", { command: "ls internal/*/" })).toMatchObject({ decision: "allow" });
193+ expect(checkDeterministic("bash", { command: "ls internal/[ab]?/" })).toMatchObject({ decision: "allow" });
194+ expect(checkDeterministic("bash", { command: "ls */.env" })).toMatchObject({ decision: "ask" });
195+
196+ for (const command of ["cat *", "echo *", "rg *"]) {
197+ expect(checkDeterministic("bash", { command })?.decision).not.toBe("allow");
198+ }
199+});
200+
201+test("allows static quoted option values while rejecting mixed quoted paths", () => {
202+ for (const command of [
203+ 'grep -rn "NewOrchestrator\\|ScratchModel\\|scratch" --include="*.go"',
204+ "grep -rn 'NewOrchestrator\\|ScratchModel\\|scratch' --include='*.go'",
205+ 'grep --include="*.go"',
206+ ]) {
207+ expect(checkDeterministic("bash", { command })).toMatchObject({ decision: "allow" });
208+ }
209+
210+ expect(checkDeterministic("bash", { command: 'grep -rn scratch --include=".env"' })).toMatchObject({
211+ decision: "ask",
212+ });
213+
214+ for (const command of ['cat .e"nv"', 'echo > /e"tc"/x', 'rm -rf "$(echo -n /)"']) {
215+ expect(checkDeterministic("bash", { command })?.decision).not.toBe("allow");
216+ }
217+});
218+
219+test("allows fixed shell status expansion but not dynamic expansions", () => {
220+ expect(checkDeterministic("bash", { command: 'echo "EXIT: $?"' })).toMatchObject({ decision: "allow" });
221+
222+ for (const command of ['echo "$TARGET"', 'echo "${TARGET}"', 'echo "$1"', 'echo "$(pwd)"']) {
223+ expect(checkDeterministic("bash", { command })?.decision).not.toBe("allow");
224+ }
225+});
226+
227 test("checks parsed Bash redirects", () => {
228 expect(checkParsedBash("echo value", [{ path: "output.txt", writes: true, dynamic: false }])).toMatchObject({
229 decision: "allow",
230diff --git a/test/core/review.test.ts b/test/core/review.test.ts
231new file mode 100644
232index 0000000000000000000000000000000000000000..003ffb363341a566b60beefd6e68945350ab2849
233--- /dev/null
234+++ b/test/core/review.test.ts
235@@ -0,0 +1,14 @@
236+import { expect, test } from "bun:test";
237+import { createPolicyReviewer } from "../../src/core/review";
238+
239+test("explains why a request reaches the disabled reviewer", async () => {
240+ const reviewer = createPolicyReviewer({ kind: "none" }, async () => "");
241+ const result = await reviewer.evaluate({ toolName: "bash", input: { command: "echo ok" } }, {});
242+
243+ expect(result.decision).toEqual({
244+ decision: "ask",
245+ reason:
246+ "Request fell through static permissions, deterministic rules, and the decision cache; LLM reviews are disabled",
247+ category: "uncertain",
248+ });
249+});
250diff --git a/test/pi/bash-split.test.ts b/test/pi/bash-split.test.ts
251index 03743a417cfc34b46313265a23a5959b9f064853..1fdb72603f0e12b4f80457afc405d68bc09c4759 100644
252--- a/test/pi/bash-split.test.ts
253+++ b/test/pi/bash-split.test.ts
254@@ -9,6 +9,23 @@ test("falls back to the raw command when valid Bash has no command nodes", async
255 });
256 });
257
258+test("distinguishes Bash parse errors from parser unavailability", async () => {
259+ await expect(splitBashCommand("if")).resolves.toEqual({
260+ commands: [{ source: "if", redirects: [] }],
261+ parsed: false,
262+ });
263+
264+ const unavailable = await splitBashCommand("echo ok", async () => {
265+ throw new Error("tree-sitter asset details");
266+ });
267+ expect(unavailable).toEqual({
268+ commands: [{ source: "echo ok", redirects: [] }],
269+ parsed: false,
270+ parserUnavailable: true,
271+ });
272+ expect(JSON.stringify(unavailable)).not.toContain("asset details");
273+});
274+
275 test("keeps duplicate command nodes and their enclosing redirects", async () => {
276 const result = await splitBashCommand("(echo x) > /tmp/out; (echo x) > /etc/policy-engine");
277
278diff --git a/test/pi/index.test.ts b/test/pi/index.test.ts
279new file mode 100644
280index 0000000000000000000000000000000000000000..550c218e5604b10191bd191f9d384068f536b893
281--- /dev/null
282+++ b/test/pi/index.test.ts
283@@ -0,0 +1,40 @@
284+import { expect, test } from "bun:test";
285+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
286+import { tmpdir } from "node:os";
287+import { join } from "node:path";
288+import type { ExtensionContext } from "@earendil-works/pi-coding-agent";
289+import { evaluatePiToolCall } from "../../src/pi/index";
290+
291+test("blocks Bash without consulting the cache or reviewer when the parser is unavailable", async () => {
292+ const cwd = mkdtempSync(join(tmpdir(), "policy-engine-cwd-"));
293+ try {
294+ mkdirSync(join(cwd, ".pi"));
295+ writeFileSync(
296+ join(cwd, ".pi", "policy-engine.json"),
297+ JSON.stringify({ reviewer: { kind: "none" }, tools: { bash: "check" } }),
298+ );
299+
300+ const result = await evaluatePiToolCall(
301+ "bash",
302+ { command: "echo ok" },
303+ { cwd },
304+ undefined,
305+ async () => ({
306+ commands: [{ source: "echo ok", redirects: [] }],
307+ parsed: false,
308+ parserUnavailable: true,
309+ }),
310+ false,
311+ {} as ExtensionContext,
312+ );
313+
314+ expect(result).toMatchObject({
315+ decision: {
316+ decision: "deny",
317+ reason: "Bash parser is unavailable",
318+ },
319+ });
320+ } finally {
321+ rmSync(cwd, { recursive: true, force: true });
322+ }
323+});