aca4645d71d61d5bdb257f108d5251c79ed32d34
- Author
- TheEdgeOfRage <git@theedgeofrage.com>
- Committer
- TheEdgeOfRage <git@theedgeofrage.com>
- Date
Message
Refine policy decisions
Diff
This diff is truncated to protect this page.
1diff --git a/AGENTS.md b/AGENTS.md
2index cd996c6ae85fb84f51ad12e31ce196619ea40e9f..00381bc161a3b240b309d1a9847ae1fbf2c64a55 100644
3--- a/AGENTS.md
4+++ b/AGENTS.md
5@@ -24,7 +24,7 @@ MCP tools bypass the Pi adapter policy boundary.
6
7 ## Testing
8
9-Test policy behavior only: static permissions, deterministic rules, and LLM decisions. Do not add tests for configuration, providers, or other plumbing unless the user asks.
10diff --git a/src/core/deterministic.ts b/src/core/deterministic.ts
11index 9ed19c526c8c0cd4eb6d8fcbc803ad943b32001f..48b0b8897c011585c818eb99a2142fef06b9dfbb 100644
12--- a/src/core/deterministic.ts
13+++ b/src/core/deterministic.ts
14@@ -157,6 +157,7 @@ function hasUnsafeWordSyntax(command: string, allowStaticGlobs = false, program?
15 index += 1;
16 continue;
17 }
18+ if (nextCharacter === '"') continue;
19 if (command.slice(index + 1, index + 5) !== "HOME" || variableCharacter(command[index + 5])) return true;
20 index += 4;
21 continue;
22diff --git a/src/core/pipeline.ts b/src/core/pipeline.ts
23index dbd45fa5643f8cf0dd4dd9a458542a5d9a7603e6..ffc9893f16f4be3168c6f70777aae122bc8d4752 100644
24--- a/src/core/pipeline.ts
25+++ b/src/core/pipeline.ts
26@@ -115,13 +115,14 @@ export function createPolicyPipeline(options: PolicyPipelineOptions) {
27 }
28
29 const reviewed = await options.reviewer.evaluate(request, context);
30+ const source = reviewed.source ?? "llm";
31 const result: PolicyEvaluation = {
32 decision: reviewed.decision,
33- source: "llm",
34+ source,
35 rawResponse: reviewed.rawResponse,
36 error: reviewed.error,
37 };
38- if (!reviewed.error) {
39+ if (!reviewed.error && source === "llm") {
40 try {
41 await options.cache.write({
42 key,
43diff --git a/src/core/review.ts b/src/core/review.ts
44index bd8ec8bebec7c457babe112254d4613c25b6ffb1..f997b9676e7a177c01ea21df57779756934cf5d9 100644
45--- a/src/core/review.ts
46+++ b/src/core/review.ts
47@@ -76,6 +76,7 @@ export function createPolicyReviewer(
48 "Request fell through static permissions, deterministic rules, and the decision cache; LLM reviews are disabled",
49 category: "uncertain",
50 },
51+ source: "none",
52 };
53 }
54
55diff --git a/src/core/rules.ts b/src/core/rules.ts
56index efe11b72effc94546924c24e818aeef4696f70f6..385f3720a69952278e48016d76eebc046ca2fffe 100644
57--- a/src/core/rules.ts
58+++ b/src/core/rules.ts
59@@ -1,7 +1,7 @@
60 import { DECISION_CATEGORIES } from "./types";
61
62 // Bump to invalidate all cached decisions when rules change.
63-export const POLICY_VERSION = 37;
64+export const POLICY_VERSION = 38;
65
66 // Trailing stderr redirections that are safe to strip before pattern matching.
67 // `2>&1` and `2>/dev/null` have no security implication but would otherwise
68diff --git a/src/core/types.ts b/src/core/types.ts
69index 4a08e046b3c9f5c8c1526cb21316a009012f4598..7a19f7999dd44b8475aa71c7e32a9aa0ad4e0801 100644
70--- a/src/core/types.ts
71+++ b/src/core/types.ts
72@@ -39,6 +39,7 @@ export type Decision = {
73
74 export type LLMEvaluationResult = {
75 decision: Decision;
76+ source?: "llm" | "none";
77 rawResponse?: string;
78 error?: string;
79 };
80@@ -80,7 +81,7 @@ export type DecisionCache = {
81 write(entry: CacheEntry): Promise<void>;
82 };
83
84-export type DecisionSource = "session" | "static" | "deterministic" | "cache" | "llm";
85+export type DecisionSource = "session" | "static" | "deterministic" | "cache" | "llm" | "none";
86
87 export type AuditEntry = {
88 toolName: string;
89@@ -116,6 +117,6 @@ export type PolicyContext = {
90 };
91
92 export type PolicyPrecheck = {
93- source: Exclude<DecisionSource, "llm" | "cache" | "deterministic">;
94+ source: Exclude<DecisionSource, "llm" | "cache" | "deterministic" | "none">;
95 decide(request: PolicyRequest, context: PolicyContext): Promise<Decision | undefined>;
96 };
97diff --git a/test/core/deterministic.test.ts b/test/core/deterministic.test.ts
98index 4c7af4f3990f727694bd133325c8806bed3b2337..ec323aa0d550afd30b2f94cbbdac8f088aa04a2c 100644
99--- a/test/core/deterministic.test.ts
100+++ b/test/core/deterministic.test.ts
101@@ -22,6 +22,7 @@ test("allows recognized deterministic command grammars", () => {
102 "command -v bun node",
103 "cd ~/.ssh",
104 'grep -rn "namespace\\|Namespace" ~/go/pkg/mod/github.com/jessevdk/go-flags@v1.6.1/option.go',
105+ 'grep -iE "golangci|\\.go$"',
106 "echo 'a|b'",
107 "git add src/core/rules.ts",
108 "git checkout main",
109diff --git a/test/core/review.test.ts b/test/core/review.test.ts
110deleted file mode 100644
111index 003ffb363341a566b60beefd6e68945350ab2849..0000000000000000000000000000000000000000
112--- a/test/core/review.test.ts
113+++ /dev/null
114@@ -1,14 +0,0 @@
115-import { expect, test } from "bun:test";
116-import { createPolicyReviewer } from "../../src/core/review";
117-
118-test("explains why a request reaches the disabled reviewer", async () => {
119- const reviewer = createPolicyReviewer({ kind: "none" }, async () => "");
120- const result = await reviewer.evaluate({ toolName: "bash", input: { command: "echo ok" } }, {});
121-
122- expect(result.decision).toEqual({
123- decision: "ask",
124- reason:
125- "Request fell through static permissions, deterministic rules, and the decision cache; LLM reviews are disabled",
126- category: "uncertain",
127- });
128-});
129diff --git a/test/pi/bash-split.test.ts b/test/pi/bash-split.test.ts
130deleted file mode 100644
131index 1fdb72603f0e12b4f80457afc405d68bc09c4759..0000000000000000000000000000000000000000
132--- a/test/pi/bash-split.test.ts
133+++ /dev/null
134@@ -1,64 +0,0 @@
135-import { expect, test } from "bun:test";
136-import { checkParsedBash } from "../../src/core/deterministic";
137-import { splitBashCommand } from "../../src/pi/bash-split";
138-
139-test("falls back to the raw command when valid Bash has no command nodes", async () => {
140- await expect(splitBashCommand("> /etc/policy-engine")).resolves.toEqual({
141- commands: [{ source: "> /etc/policy-engine", redirects: [] }],
142- parsed: false,
143- });
144-});
145-
146-test("distinguishes Bash parse errors from parser unavailability", async () => {
147- await expect(splitBashCommand("if")).resolves.toEqual({
148- commands: [{ source: "if", redirects: [] }],
149- parsed: false,
150- });
151-
152- const unavailable = await splitBashCommand("echo ok", async () => {
153- throw new Error("tree-sitter asset details");
154- });
155- expect(unavailable).toEqual({
156- commands: [{ source: "echo ok", redirects: [] }],
157- parsed: false,
158- parserUnavailable: true,
159- });
160- expect(JSON.stringify(unavailable)).not.toContain("asset details");
161-});
162-
163-test("keeps duplicate command nodes and their enclosing redirects", async () => {
164- const result = await splitBashCommand("(echo x) > /tmp/out; (echo x) > /etc/policy-engine");
165-
166- expect(result.parsed).toBe(true);
167- expect(result.commands).toHaveLength(2);
168- expect(result.commands.map((command) => command.source)).toEqual(["echo x", "echo x"]);
169- expect(result.commands.map((command) => command.redirects.map((redirect) => redirect.path))).toEqual([
170- ["/tmp/out"],
171- ["/etc/policy-engine"],
172- ]);
173- expect(checkParsedBash(result.commands[0]!.source, result.commands[0]!.redirects)).toMatchObject({
174- decision: "allow",
175- });
176- expect(checkParsedBash(result.commands[1]!.source, result.commands[1]!.redirects)).toMatchObject({ decision: "ask" });
177-});
178-
179-test("falls back for unresolved words and redirect paths in parsed Bash", async () => {
180- for (const command of ['cat .e"nv"', 'rm -rf "$(echo -n /)"']) {
181- const result = await splitBashCommand(command);
182- expect(result.parsed).toBe(true);
183- expect(checkParsedBash(result.commands[0]!.source, result.commands[0]!.redirects)?.decision).not.toBe("allow");
184- }
185-
186- const redirectResult = await splitBashCommand('echo x > /e"tc"/x');
187- expect(redirectResult.parsed).toBe(true);
188- expect(redirectResult.commands[0]!.redirects[0]).toMatchObject({ dynamic: true });
189- expect(checkParsedBash(redirectResult.commands[0]!.source, redirectResult.commands[0]!.redirects)?.decision).not.toBe(
190- "allow",
191- );
192-
193- const homeRedirect = await splitBashCommand('echo x > "$HOME/output.txt"');
194- expect(homeRedirect.commands[0]!.redirects[0]).toMatchObject({ path: "$HOME/output.txt", dynamic: false });
195- expect(checkParsedBash(homeRedirect.commands[0]!.source, homeRedirect.commands[0]!.redirects)).toMatchObject({
196- decision: "allow",
197- });
198-});
199diff --git a/test/pi/index.test.ts b/test/pi/index.test.ts
200deleted file mode 100644
201index 3bd62e4581985ec74408e03fff59fb5e43563125..0000000000000000000000000000000000000000
202--- a/test/pi/index.test.ts
203+++ /dev/null
204@@ -1,84 +0,0 @@
205-import { expect, test } from "bun:test";
206-import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
207-import { tmpdir } from "node:os";
208-import { join } from "node:path";
209-import type { ExtensionAPI, ExtensionContext } from "@earendil-works/pi-coding-agent";
210-import policyEngine, { evaluatePiToolCall } from "../../src/pi/index";
211-
212-test("adds rejection feedback to an interactive approval block", async () => {
213- const cwd = mkdtempSync(join(tmpdir(), "policy-engine-cwd-"));
214- try {
215- mkdirSync(join(cwd, ".pi"));
216- writeFileSync(
217- join(cwd, ".pi", "policy-engine.json"),
218- JSON.stringify({ reviewer: { kind: "none" }, tools: { unknown: "check" } }),
219- );
220-
221- type ToolCallHandler = (
222- event: { toolName: string; input: Record<string, unknown> },
223- ctx: ExtensionContext,
224- ) => Promise<unknown>;
225- let toolCall: ToolCallHandler | undefined;
226- policyEngine({
227- on(event: string, handler: unknown) {
228- if (event === "tool_call") toolCall = handler as ToolCallHandler;
229- },
230- registerCommand() {},
231- appendEntry() {},
232- events: { emit() {} },
233- } as unknown as ExtensionAPI);
234-
235- const confirm = async () => false;
236- const input = async (title: string, placeholder?: string) => {
237- expect(title).toBe("Why was this rejected?");
238- expect(placeholder).toBe("Tell the agent why it was rejected and what to do differently");
239- return "Use the dry-run command first";
240- };
241- const result = await toolCall!({ toolName: "unknown", input: {} }, {
242- hasUI: true,
243- mode: "rpc",
244- cwd,
245- modelRegistry: {},
246- sessionManager: { getSessionId: () => "test-session" },
247- ui: { confirm, input },
248- } as unknown as ExtensionContext);
249-
250- expect(result).toEqual({ block: true, reason: "Blocked by user: Use the dry-run command first" });
251- } finally {
252- rmSync(cwd, { recursive: true, force: true });
253- }
254-});
255-
256-test("blocks Bash without consulting the cache or reviewer when the parser is unavailable", async () => {
257- const cwd = mkdtempSync(join(tmpdir(), "policy-engine-cwd-"));
258- try {
259- mkdirSync(join(cwd, ".pi"));
260- writeFileSync(
261- join(cwd, ".pi", "policy-engine.json"),
262- JSON.stringify({ reviewer: { kind: "none" }, tools: { bash: "check" } }),
263- );
264-
265- const result = await evaluatePiToolCall(
266- "bash",
267- { command: "echo ok" },
268- { cwd },
269- undefined,
270- async () => ({
271- commands: [{ source: "echo ok", redirects: [] }],
272- parsed: false,
273- parserUnavailable: true,
274- }),
275- false,
276- {} as ExtensionContext,
277- );
278-
279- expect(result).toMatchObject({
280- decision: {
281- decision: "deny",
282- reason: "Bash parser is unavailable",
283- },
284- });
285- } finally {
286- rmSync(cwd, { recursive: true, force: true });
287- }
288-});
289diff --git a/test/pi/static-permissions.test.ts b/test/pi/static-permissions.test.ts
290index a9d4c961f98876a088dd0c448f3167c3107066be..eaec305ef3d7995f327a7d083bdba3bb74143a1f 100644
291--- a/test/pi/static-permissions.test.ts
292+++ b/test/pi/static-permissions.test.ts
293@@ -2,7 +2,7 @@ import { expect, test } from "bun:test";
294 import { mkdtempSync, rmSync } from "node:fs";
295 import { tmpdir } from "node:os";
296 import { join } from "node:path";
297-import { checkStaticPermission, matchesExternalDirectory } from "../../src/pi/static-permissions";
298+import { checkStaticPermission } from "../../src/pi/static-permissions";
299 import type { PiPolicyEngineConfig } from "../../src/pi/config";
300
301 test("explicit deny wins before external path handling", async () => {
302@@ -53,7 +53,7 @@ test("asks before changing a policy engine configuration", async () => {
303 }
304 });
305
306-test("matches external directories by path components", async () => {
307+test("allows external paths only inside configured directories", async () => {
308 const root = mkdtempSync(join(tmpdir(), "policy-engine-external-"));
309 const cwd = join(root, "cwd");
310 const external = join(root, "shared");
311@@ -65,9 +65,6 @@ test("matches external directories by path components", async () => {
312 decision: "allow",
313 });
314 await expect(checkStaticPermission("read", { path: sibling }, cwd, config)).resolves.toBeUndefined();
315- expect(matchesExternalDirectory([external], `${external}-sibling`)).toBe(false);
316- expect(matchesExternalDirectory([external], nested)).toBe(true);
317- expect(matchesExternalDirectory([""], nested)).toBe(false);
318 } finally {
319 rmSync(root, { recursive: true, force: true });
320 }