mirror of
https://bitbucket.org/siakitem/my-pi.git
synced 2026-10-26 10:45:22 +00:00
refactor(permission-system): simplify policy and autoreview flow
This commit is contained in:
@@ -14,8 +14,6 @@
|
||||
import {
|
||||
mkdirSync,
|
||||
mkdtempSync,
|
||||
readdirSync,
|
||||
readFileSync,
|
||||
rmSync,
|
||||
symlinkSync,
|
||||
writeFileSync,
|
||||
@@ -28,11 +26,6 @@ import {
|
||||
type ExtensionAPI,
|
||||
} from "@earendil-works/pi-coding-agent";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
createPermissionForwardingLocation,
|
||||
type ForwardedPermissionRequest,
|
||||
} from "#src/authority/permission-forwarding";
|
||||
import { getServingSessionRegistry } from "#src/authority/serving-registry";
|
||||
import { SUBAGENT_CHILD_SESSION_CREATED } from "#src/authority/subagent-lifecycle-events";
|
||||
import { getSubagentSessionRegistry } from "#src/authority/subagent-registry";
|
||||
import { getGlobalConfigPath } from "#src/config-paths";
|
||||
@@ -40,7 +33,6 @@ import { DEFAULT_EXTENSION_CONFIG } from "#src/extension-config";
|
||||
import piPermissionSystemExtension from "#src/index";
|
||||
import { PERMISSIONS_READY_CHANNEL } from "#src/permission-events";
|
||||
import { getPermissionsService } from "#src/service";
|
||||
import { publishServingHeartbeat } from "#test/helpers/forwarding-fixtures";
|
||||
import { makeFakePi } from "#test/helpers/make-fake-pi";
|
||||
|
||||
const SERVICE_KEY = Symbol.for("@gotgenes/pi-permission-system:service");
|
||||
@@ -171,8 +163,6 @@ function makeUiCtx(cwd: string, capturedTitles: string[]): { ctx: unknown } {
|
||||
return { ctx };
|
||||
}
|
||||
|
||||
const sleep = (ms: number): Promise<void> =>
|
||||
new Promise((resolve) => setTimeout(resolve, ms));
|
||||
|
||||
/** Drive the registered `session_start` handler with a ctx. */
|
||||
function fireSessionStart(
|
||||
@@ -182,53 +172,6 @@ function fireSessionStart(
|
||||
return pi.fire("session_start", { reason: "start" }, ctx);
|
||||
}
|
||||
|
||||
/**
|
||||
* Simulate the parent UI session responding to a forwarded permission request.
|
||||
*
|
||||
* Polls the parent's requests directory for the child's request file, then
|
||||
* writes an approval response so the child's forwarding poll resolves quickly
|
||||
* instead of waiting out the 10-minute timeout.
|
||||
*/
|
||||
async function approveForwardedRequest(
|
||||
forwardingDir: string,
|
||||
parentSessionId: string,
|
||||
): Promise<ForwardedPermissionRequest> {
|
||||
const location = createPermissionForwardingLocation(
|
||||
forwardingDir,
|
||||
parentSessionId,
|
||||
);
|
||||
const deadline = Date.now() + 2000;
|
||||
while (Date.now() < deadline) {
|
||||
let files: string[] = [];
|
||||
try {
|
||||
files = readdirSync(location.requestsDir).filter((f) =>
|
||||
f.endsWith(".json"),
|
||||
);
|
||||
} catch {
|
||||
files = [];
|
||||
}
|
||||
const requestFile = files[0];
|
||||
if (requestFile) {
|
||||
const request = JSON.parse(
|
||||
readFileSync(join(location.requestsDir, requestFile), "utf8"),
|
||||
) as ForwardedPermissionRequest;
|
||||
mkdirSync(location.responsesDir, { recursive: true });
|
||||
writeFileSync(
|
||||
join(location.responsesDir, `${request.id}.json`),
|
||||
JSON.stringify({
|
||||
approved: true,
|
||||
state: "approved",
|
||||
responderSessionId: parentSessionId,
|
||||
respondedAt: Date.now(),
|
||||
}),
|
||||
"utf8",
|
||||
);
|
||||
return request;
|
||||
}
|
||||
await sleep(5);
|
||||
}
|
||||
throw new Error("Timed out waiting for the forwarded permission request");
|
||||
}
|
||||
|
||||
describe("event-handler registration completeness", () => {
|
||||
it("registers a handler for every required event exactly once", () => {
|
||||
@@ -239,208 +182,7 @@ describe("event-handler registration completeness", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("subagent registry sharing across factory instances", () => {
|
||||
// The #296 regression class: two factory invocations on *different* event
|
||||
// buses must still resolve the same process-global SubagentSessionRegistry,
|
||||
// so a child registered via the parent's bus detects itself as a subagent and
|
||||
// forwards (rather than blocking) an external-directory `ask`.
|
||||
it("lets a child instance forward an ask it received via the parent's bus", async () => {
|
||||
writeGlobalConfig({
|
||||
permission: { "*": "allow", external_directory: "ask" },
|
||||
});
|
||||
|
||||
const childCwd = mkdtempSync(join(tmpdir(), "pi-perm-child-cwd-"));
|
||||
const externalDir = mkdtempSync(join(tmpdir(), "pi-perm-external-"));
|
||||
const forwardingDir = join(agentDir, "sessions", "permission-forwarding");
|
||||
const parentSessionId = "parent-session-1";
|
||||
const childSessionId = "child-session-1";
|
||||
|
||||
// Two factory instances, each wired to its own event bus (as in production:
|
||||
// every session's ResourceLoader creates a separate bus).
|
||||
const parentBus = createEventBus();
|
||||
const childBus = createEventBus();
|
||||
piPermissionSystemExtension(
|
||||
makeFakePi({ events: parentBus }) as unknown as ExtensionAPI,
|
||||
);
|
||||
const childPi = makeFakePi({
|
||||
events: childBus,
|
||||
toolNames: ["read"],
|
||||
});
|
||||
piPermissionSystemExtension(childPi as unknown as ExtensionAPI);
|
||||
|
||||
// The child session is announced on the *parent's* bus only; the parent's
|
||||
// lifecycle subscription writes it into the shared global registry.
|
||||
parentBus.emit(SUBAGENT_CHILD_SESSION_CREATED, {
|
||||
sessionId: childSessionId,
|
||||
parentSessionId,
|
||||
});
|
||||
|
||||
// This test answers the forwarded request by hand instead of running the
|
||||
// parent's poll timer, so state what that timer would have published:
|
||||
// the parent is draining its inbox. Without it the child correctly
|
||||
// abandons the request as unserved (#719).
|
||||
getServingSessionRegistry().markServing(parentSessionId);
|
||||
|
||||
// The child fires an external-directory read with no UI. With the shared
|
||||
// registry it detects itself as a subagent and forwards; the simulated
|
||||
// parent approves.
|
||||
const firePromise = childPi.fire(
|
||||
"tool_call",
|
||||
{
|
||||
toolName: "read",
|
||||
toolCallId: "child-external-read",
|
||||
input: { path: join(externalDir, "secret.txt") },
|
||||
},
|
||||
makeChildCtx(childCwd, childSessionId),
|
||||
);
|
||||
|
||||
const request = await approveForwardedRequest(
|
||||
forwardingDir,
|
||||
parentSessionId,
|
||||
);
|
||||
expect(request.targetSessionId).toBe(parentSessionId);
|
||||
expect(request.requesterSessionId).toBe(childSessionId);
|
||||
// The child persists the original display fields so the parent emits a
|
||||
// non-degraded `permissions:ui_prompt` event (forwarded non-degradation).
|
||||
expect(request.source).toBe("tool_call");
|
||||
expect(request.surface).toBe("read");
|
||||
expect(request.value).toBe(join(externalDir, "secret.txt"));
|
||||
|
||||
const result = (await firePromise) as { block?: true };
|
||||
expect(result.block).toBeUndefined();
|
||||
|
||||
rmSync(childCwd, { recursive: true, force: true });
|
||||
rmSync(externalDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// The #719 failure mode: the child forwards correctly, but nothing drains
|
||||
// the parent's inbox. Before the serving registry it waited out the full
|
||||
// ten-minute timeout and reported the block as a user denial.
|
||||
it("blocks promptly when no session is draining the parent's inbox", async () => {
|
||||
writeGlobalConfig({
|
||||
permission: { "*": "allow", external_directory: "ask" },
|
||||
});
|
||||
|
||||
const childCwd = mkdtempSync(join(tmpdir(), "pi-perm-child-cwd-"));
|
||||
const externalDir = mkdtempSync(join(tmpdir(), "pi-perm-external-"));
|
||||
const parentSessionId = "parent-session-2";
|
||||
const childSessionId = "child-session-2";
|
||||
|
||||
const parentBus = createEventBus();
|
||||
piPermissionSystemExtension(
|
||||
makeFakePi({ events: parentBus }) as unknown as ExtensionAPI,
|
||||
);
|
||||
const childPi = makeFakePi({
|
||||
events: createEventBus(),
|
||||
toolNames: ["read"],
|
||||
});
|
||||
piPermissionSystemExtension(childPi as unknown as ExtensionAPI);
|
||||
|
||||
parentBus.emit(SUBAGENT_CHILD_SESSION_CREATED, {
|
||||
sessionId: childSessionId,
|
||||
parentSessionId,
|
||||
});
|
||||
// Deliberately no markServing: nobody is polling the parent's inbox.
|
||||
|
||||
const result = (await childPi.fire(
|
||||
"tool_call",
|
||||
{
|
||||
toolName: "read",
|
||||
toolCallId: "child-external-read",
|
||||
input: { path: join(externalDir, "secret.txt") },
|
||||
},
|
||||
makeChildCtx(childCwd, childSessionId),
|
||||
)) as { block?: true; reason?: string };
|
||||
|
||||
expect(result.block).toBe(true);
|
||||
expect(result.reason).toContain("no interactive UI is available");
|
||||
expect(result.reason).toContain(
|
||||
`Session '${parentSessionId}' is not serving forwarded permission requests.`,
|
||||
);
|
||||
expect(result.reason).not.toContain("User denied");
|
||||
|
||||
rmSync(childCwd, { recursive: true, force: true });
|
||||
rmSync(externalDir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe("out-of-process forwarding liveness", () => {
|
||||
// A child spawned as its own `pi` process resolves its parent from the
|
||||
// environment and shares no `globalThis` with it, so the serving registry the
|
||||
// tests above rely on is invisible to it. The filesystem heartbeat is the
|
||||
// only signal it has (#721, #735 scenario 1).
|
||||
const externalAsk = {
|
||||
permission: { "*": "allow", external_directory: "ask" },
|
||||
};
|
||||
|
||||
function fireChildRead(childCwd: string, externalDir: string) {
|
||||
const childPi = makeFakePi({
|
||||
events: createEventBus(),
|
||||
toolNames: ["read"],
|
||||
});
|
||||
piPermissionSystemExtension(childPi as unknown as ExtensionAPI);
|
||||
return childPi.fire(
|
||||
"tool_call",
|
||||
{
|
||||
toolName: "read",
|
||||
toolCallId: "child-external-read",
|
||||
input: { path: join(externalDir, "secret.txt") },
|
||||
},
|
||||
makeChildCtx(childCwd, "child-session-oop"),
|
||||
);
|
||||
}
|
||||
|
||||
it("blocks promptly when the out-of-process parent published no heartbeat", async () => {
|
||||
writeGlobalConfig(externalAsk);
|
||||
const childCwd = mkdtempSync(join(tmpdir(), "pi-perm-child-cwd-"));
|
||||
const externalDir = mkdtempSync(join(tmpdir(), "pi-perm-external-"));
|
||||
const parentSessionId = "parent-session-oop-gone";
|
||||
// The env-var path a process-based subagent extension uses: the hint makes
|
||||
// the child detect itself, and the parent id names a session that exited.
|
||||
vi.stubEnv("PI_SUBAGENT_CHILD", "1");
|
||||
vi.stubEnv("PI_SUBAGENT_PARENT_SESSION", parentSessionId);
|
||||
|
||||
const result = (await fireChildRead(childCwd, externalDir)) as {
|
||||
block?: true;
|
||||
reason?: string;
|
||||
};
|
||||
|
||||
expect(result.block).toBe(true);
|
||||
expect(result.reason).toContain(
|
||||
`Session '${parentSessionId}' is not serving forwarded permission requests.`,
|
||||
);
|
||||
expect(result.reason).not.toContain("User denied");
|
||||
|
||||
rmSync(childCwd, { recursive: true, force: true });
|
||||
rmSync(externalDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("waits for an out-of-process parent whose heartbeat is fresh", async () => {
|
||||
writeGlobalConfig(externalAsk);
|
||||
const childCwd = mkdtempSync(join(tmpdir(), "pi-perm-child-cwd-"));
|
||||
const externalDir = mkdtempSync(join(tmpdir(), "pi-perm-external-"));
|
||||
const forwardingDir = join(agentDir, "sessions", "permission-forwarding");
|
||||
const parentSessionId = "parent-session-oop-live";
|
||||
vi.stubEnv("PI_SUBAGENT_CHILD", "1");
|
||||
vi.stubEnv("PI_SUBAGENT_PARENT_SESSION", parentSessionId);
|
||||
// What the parent's poll timer would have published; this test answers the
|
||||
// request by hand instead of running that timer.
|
||||
publishServingHeartbeat(forwardingDir, parentSessionId);
|
||||
|
||||
const firePromise = fireChildRead(childCwd, externalDir);
|
||||
const request = await approveForwardedRequest(
|
||||
forwardingDir,
|
||||
parentSessionId,
|
||||
);
|
||||
expect(request.targetSessionId).toBe(parentSessionId);
|
||||
|
||||
const result = (await firePromise) as { block?: true };
|
||||
expect(result.block).toBeUndefined();
|
||||
|
||||
rmSync(childCwd, { recursive: true, force: true });
|
||||
rmSync(externalDir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe("shutdown teardown chain", () => {
|
||||
it("unpublishes the service and unsubscribes the lifecycle on shutdown", async () => {
|
||||
@@ -727,7 +469,7 @@ describe("project trust gates project-scoped config (#644)", () => {
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("lets a trusted project's `bash: allow` override global `bash: deny`", async () => {
|
||||
it("ignores project policy even when the project is trusted", async () => {
|
||||
writeGlobalConfig({ permission: { "*": "ask", bash: "deny" } });
|
||||
const cwd = mkdtempSync(join(tmpdir(), "pi-perm-trusted-cwd-"));
|
||||
writeProjectConfig(cwd, { permission: { bash: "allow" } });
|
||||
@@ -739,10 +481,10 @@ describe("project trust gates project-scoped config (#644)", () => {
|
||||
makeBaseCtx(cwd, "trusted-session", { isProjectTrusted: true }),
|
||||
);
|
||||
|
||||
// The trusted project override applies (last-match-wins).
|
||||
// Project policy files are not part of the permission configuration.
|
||||
expect(
|
||||
getPermissionsService()!.checkPermission("bash", "echo hi").state,
|
||||
).toBe("allow");
|
||||
).toBe("deny");
|
||||
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
});
|
||||
@@ -883,65 +625,6 @@ describe("bash bare-token path gating (#509, #645)", () => {
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("denies a bare symlink escaping the working tree under external_directory deny (#645)", async () => {
|
||||
// The issue's headline repro:
|
||||
// printf 'test' > /tmp/…-secret ; ln -s /tmp/…-secret outside-link
|
||||
// cat outside-link (under a permissive `cat *` bash rule)
|
||||
const cwd = mkdtempSync(join(tmpdir(), "pi-perm-escape-cwd-"));
|
||||
const outside = mkdtempSync(join(tmpdir(), "pi-perm-escape-target-"));
|
||||
const secret = join(outside, "pi-permission-test-secret");
|
||||
writeFileSync(secret, "test");
|
||||
symlinkSync(secret, join(cwd, "outside-link"));
|
||||
writeGlobalConfig({
|
||||
permission: {
|
||||
"*": "allow",
|
||||
bash: { "cat *": "allow" },
|
||||
external_directory: "deny",
|
||||
},
|
||||
});
|
||||
|
||||
const pi = makeFakePi({ events: createEventBus() });
|
||||
piPermissionSystemExtension(pi as unknown as ExtensionAPI);
|
||||
const ctx = makeChildCtx(cwd, "bare-escape-session-deny");
|
||||
await fireSessionStart(pi, ctx);
|
||||
|
||||
const result = await fireBashToolCall(pi, ctx, "cat outside-link");
|
||||
expect(result.block).toBe(true);
|
||||
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
rmSync(outside, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("denies a path embedded in a long option under external_directory deny (#645)", async () => {
|
||||
// The issue's second repro: `grep --file=/tmp/…` under an allowing
|
||||
// `grep *` bash rule. The flag token never reached the path surfaces.
|
||||
const cwd = mkdtempSync(join(tmpdir(), "pi-perm-flagpath-cwd-"));
|
||||
const outside = mkdtempSync(join(tmpdir(), "pi-perm-flagpath-target-"));
|
||||
const patterns = join(outside, "pi-permission-patterns");
|
||||
writeFileSync(patterns, "secret\n");
|
||||
writeGlobalConfig({
|
||||
permission: {
|
||||
"*": "allow",
|
||||
bash: { "grep *": "allow" },
|
||||
external_directory: "deny",
|
||||
},
|
||||
});
|
||||
|
||||
const pi = makeFakePi({ events: createEventBus() });
|
||||
piPermissionSystemExtension(pi as unknown as ExtensionAPI);
|
||||
const ctx = makeChildCtx(cwd, "flag-path-session-deny");
|
||||
await fireSessionStart(pi, ctx);
|
||||
|
||||
const result = await fireBashToolCall(
|
||||
pi,
|
||||
ctx,
|
||||
`grep --file=${patterns} target`,
|
||||
);
|
||||
expect(result.block).toBe(true);
|
||||
|
||||
rmSync(cwd, { recursive: true, force: true });
|
||||
rmSync(outside, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe("multi-instance global service interplay", () => {
|
||||
|
||||
Reference in New Issue
Block a user