From 0c805e4bf7201f3687b1c9294ac6aa301feaab4e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 11 Oct 2026 19:13:38 -0700 Subject: [PATCH] refactor(permission): drop dead params and duplicate helpers --- src/agent/tool-aliases.test.ts | 14 ++++++------ src/agent/tools.ts | 2 +- src/permission/authz-grants.ts | 15 ++++--------- src/permission/gate.test.ts | 18 +++++++-------- src/permission/gate.ts | 4 ++-- src/permission/grant-scope.test.ts | 31 +++++++++++++------------- src/permission/path-restriction.ts | 27 ++++++++-------------- src/permission/permission.test.ts | 30 ++++++++++++------------- src/permission/queue.test.ts | 4 ++-- src/permission/queue.ts | 2 +- src/permission/worktree-roots.ts | 30 ++++++++++++------------- src/plugins/path-escape-plugin.ts | 8 +++---- src/plugins/shell-guard-plugin.test.ts | 8 +++---- src/shell/background-shell.test.ts | 6 ++--- src/shell/background-shell.ts | 5 ++--- src/shell/persistent-shell-cwd.ts | 8 +------ src/subagent/run-persist-close.test.ts | 12 +++++----- src/subagent/run.ts | 6 ++--- 18 files changed, 102 insertions(+), 128 deletions(-) diff --git a/src/agent/tool-aliases.test.ts b/src/agent/tool-aliases.test.ts index 9b9db54de..7c962ee95 100644 --- a/src/agent/tool-aliases.test.ts +++ b/src/agent/tool-aliases.test.ts @@ -13,7 +13,7 @@ import { withAuthzParityDefinitions, WIRE_TO_ENGINE, } from "./tool-aliases.js"; -import { evaluateApprovals } from "../permission/authz-grants.js"; +import { approvalCoversSubject } from "../permission/authz-grants.js"; const noWorkspace = { resolvedCwd: "/repo", roots: ["/repo"] }; @@ -55,7 +55,7 @@ describe("one advertised posix set", () => { describe("grant aliases", () => { test("a grant stored as run_shell covers bash", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "bash", subject: "npm test", approvals: [{ tool: "run_shell", pattern: "npm *" }], @@ -66,7 +66,7 @@ describe("grant aliases", () => { test("a grant stored as bash matches a run_shell request after canonicalize", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "npm test", approvals: [{ tool: "bash", pattern: "npm *" }], @@ -77,7 +77,7 @@ describe("grant aliases", () => { test("pure renames stay bidirectional: read covers read_file and vice versa", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "read_file", subject: "src/a.ts", approvals: [{ tool: "read", pattern: "src/*" }], @@ -85,7 +85,7 @@ describe("grant aliases", () => { }), ).toBe(true); expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "read", subject: "src/a.ts", approvals: [{ tool: "read_file", pattern: "src/*" }], @@ -102,7 +102,7 @@ describe("grant aliases", () => { // update_plan keys, so that replay path is unreachable. test("a stored update_plan grant does not cover manage_tasks", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "manage_tasks", subject: "manage_tasks", approvals: [{ tool: "update_plan", pattern: "*" }], @@ -113,7 +113,7 @@ describe("grant aliases", () => { test("a stored manage_tasks grant covers manage_tasks", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "manage_tasks", subject: "manage_tasks", approvals: [{ tool: "manage_tasks", pattern: "*" }], diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 89c0cc251..c9db63114 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -1527,7 +1527,7 @@ export async function createAgentToolset( const failures: unknown[] = []; // Kill live background process groups before the posix teardown so // reload/interrupt cannot leave orphans behind. - backgroundShells.disposeAll("session closed"); + backgroundShells.disposeAll(); try { await posixTools.dispose(); } catch (err: unknown) { diff --git a/src/permission/authz-grants.ts b/src/permission/authz-grants.ts index f8eab5377..55c2298a0 100644 --- a/src/permission/authz-grants.ts +++ b/src/permission/authz-grants.ts @@ -7,7 +7,7 @@ import { } from "../agent/canonical-tool-name.js"; import type { Approval } from "./types.js"; import { directoryGrantAllows, matchesPattern } from "./matcher.js"; -import { realpathOr } from "./worktree-roots.js"; +import { realpathOrOriginal } from "./worktree-roots.js"; // Exact-escaped patterns (backslash before metacharacters) cannot round-trip // through @intx/authz matchPattern; they skip the package call and fall to @@ -65,7 +65,7 @@ export function cwdMatchesGrant( if (requestCwd === undefined) return false; if (grantCwd !== workspace.resolvedCwd) return false; if (grantCwd === requestCwd) return true; - return workspace.roots.includes(realpathOr(requestCwd)); + return workspace.roots.includes(realpathOrOriginal(requestCwd)); } // Seeded grants enter in native key space: pure renames collapse onto the @@ -84,8 +84,8 @@ export function normalizeSeededApprovals( } // Single owner for grant tool/providerModel/cwd scope coverage, independent -// of pattern matching. Both live call sites (evaluateApprovals, -// isRequestCoveredByGrant) delegate here so a scoping-dimension change lands +// of pattern matching. Both live call sites (approvalCoversSubject, +// isRequestCoveredByApprovals) delegate here so a scoping-dimension change lands // in one place. export function grantScopeMatches( approval: Approval, @@ -165,10 +165,3 @@ export async function approvalCoversSubject( const decision = await evaluateGrants(grants, subject, action); return decision.effect === "allow"; } - -// Grant-store evaluation entry point; delegates to approvalCoversSubject. -export async function evaluateApprovals( - input: EvaluateApprovalsInput, -): Promise { - return approvalCoversSubject(input); -} diff --git a/src/permission/gate.test.ts b/src/permission/gate.test.ts index 3a21b1f62..a682d1808 100644 --- a/src/permission/gate.test.ts +++ b/src/permission/gate.test.ts @@ -12,7 +12,7 @@ import { join } from "node:path"; import type { ToolCall } from "@intx/types/runtime"; import { createPermissionGate, - isRequestCoveredByGrant, + isRequestCoveredByApproval, preGrantGuardReason, } from "./gate.js"; import { createPathRestriction } from "./path-restriction.js"; @@ -27,7 +27,7 @@ const shellCall = (command: string): ToolCall => ({ }); // Every pre-grant guard, keyed to a command that trips it. -// isRequestCoveredByGrant must refuse to cover each of these even when a +// isRequestCoveredByApproval must refuse to cover each of these even when a // grant would otherwise match verbatim. The secret/restricted-path cases // queue and could be auto-approved by reconciliation once a broad grant // lands; the shell-authz hard-denies are unreachable today (evaluate() @@ -44,7 +44,7 @@ const GUARD_CASES: { name: string; command: string }[] = [ { name: "restricted path target", command: "cat /etc/passwd" }, ]; -describe("preGrantGuardReason / isRequestCoveredByGrant guard parity", () => { +describe("preGrantGuardReason / isRequestCoveredByApproval guard parity", () => { const cwd = mkdtempSync(join(tmpdir(), "gate-guard-")); const isRestricted = createPathRestriction( cwd, @@ -63,7 +63,7 @@ describe("preGrantGuardReason / isRequestCoveredByGrant guard parity", () => { expect(preGrantGuardReason(request, isRestricted)).not.toBeUndefined(); }); - test(`${name}: isRequestCoveredByGrant refuses an otherwise-matching grant`, () => { + test(`${name}: isRequestCoveredByApproval refuses an otherwise-matching grant`, () => { const request: PermissionRequest = { tool: "run_shell", action: "Run", @@ -73,7 +73,7 @@ describe("preGrantGuardReason / isRequestCoveredByGrant guard parity", () => { }; const grant: Approval = { tool: "run_shell", pattern: command }; expect( - isRequestCoveredByGrant(request, grant, undefined, isRestricted, { + isRequestCoveredByApproval(request, grant, undefined, isRestricted, { resolvedCwd: cwd, roots: [], }), @@ -104,7 +104,7 @@ describe("preGrantGuardReason / isRequestCoveredByGrant guard parity", () => { expect(preGrantGuardReason(request, isRestricted)).toBeUndefined(); const grant: Approval = { tool: "run_shell", pattern: "npm test" }; expect( - isRequestCoveredByGrant(request, grant, undefined, isRestricted, { + isRequestCoveredByApproval(request, grant, undefined, isRestricted, { resolvedCwd: cwd, roots: [], }), @@ -265,7 +265,7 @@ describe("lone-& bypass at the gate (CL-7781)", () => { const workspace = { resolvedCwd: cwd, roots: [] as string[] }; const grant: Approval = { tool: "run_shell", pattern: "bun test *" }; const covered = (subject: string): boolean => - isRequestCoveredByGrant( + isRequestCoveredByApproval( { tool: "run_shell", action: "Run", subject, scopes: [], cwd }, grant, undefined, @@ -319,7 +319,7 @@ describe("grant coverage rebinds relative paths to the request process cwd", () }; const grant: Approval = { tool: "run_shell", pattern: "cat *" }; expect( - isRequestCoveredByGrant(request, grant, undefined, sessionRestricted, { + isRequestCoveredByApproval(request, grant, undefined, sessionRestricted, { resolvedCwd: sessionCwd, roots: [], }), @@ -337,7 +337,7 @@ describe("grant coverage rebinds relative paths to the request process cwd", () }; const grant: Approval = { tool: "run_shell", pattern: "cat *" }; expect( - isRequestCoveredByGrant(request, grant, undefined, sessionRestricted, { + isRequestCoveredByApproval(request, grant, undefined, sessionRestricted, { resolvedCwd: sessionCwd, roots: [], }), diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 0105281c8..413ced3f1 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -206,7 +206,7 @@ function bindRestrictedToProcessCwd( // Hard-deny and forced-ask checks a run_shell request must clear before grant // matching; no grant bypasses them. Shared by evaluate() and -// isRequestCoveredByGrant (via segmentGuard), so a guard added here applies to +// isRequestCoveredByApprovals (via segmentGuard), so a guard added here applies to // fresh requests and reconciliation alike. export function preGrantGuardReason( request: PermissionRequest, @@ -252,7 +252,7 @@ export function preGrantGuardReason( // preGrantGuardReason sequence — so reconciliation never auto-approves // something evaluate() would still ask for or hard-deny. run_shell coverage // is per-segment; whole-string chain patterns never cover. -export function isRequestCoveredByGrant( +export function isRequestCoveredByApproval( request: PermissionRequest, approval: Approval, activeProviderModel: string | undefined, diff --git a/src/permission/grant-scope.test.ts b/src/permission/grant-scope.test.ts index c58dcd802..36f520f3f 100644 --- a/src/permission/grant-scope.test.ts +++ b/src/permission/grant-scope.test.ts @@ -3,20 +3,19 @@ import type { ToolCall } from "@intx/types/runtime"; import type { Approval, PermissionRequest } from "./types.js"; import { approvalCoversSubject, - evaluateApprovals, grantScopeMatches, normalizeSeededApprovals, cwdMatchesGrant, type GrantWorkspace, } from "./authz-grants.js"; -import { createPermissionGate, isRequestCoveredByGrant } from "./gate.js"; +import { createPermissionGate, isRequestCoveredByApproval } from "./gate.js"; import { createPermissionRequestQueue } from "./queue.js"; import { buildRequests } from "./classify.js"; -// approvalCoversSubject is the single grant-evaluation owner; evaluateApprovals -// delegates to it and isRequestCoveredByGrant must agree on scope. Driving -// the same grant+request pairs through all three catches a call site that -// reimplements the check with subtly different semantics. +// approvalCoversSubject is the single grant-evaluation owner; +// isRequestCoveredByApproval must agree on scope. Driving the same grant+request +// pairs through both catches a call site that reimplements the check with +// subtly different semantics. describe("grant tool/providerModel/cwd scoping agrees across call sites", () => { const workspace: GrantWorkspace = { resolvedCwd: "/proj", roots: ["/proj"] }; const noopRestricted = () => false; @@ -70,7 +69,7 @@ describe("grant tool/providerModel/cwd scoping agrees across call sites", () => scopes: [], ...(req.cwd !== undefined ? { cwd: req.cwd } : {}), }; - const viaGate = isRequestCoveredByGrant( + const viaGate = isRequestCoveredByApproval( request, grant, req.activeProviderModel, @@ -151,7 +150,7 @@ describe("normalizeSeededApprovals", () => { // mintGrant decomposes a chain scope into one grant per real segment; a grant // patterned on the full chain is legacy shape and never replays, because -// evaluate() and isRequestCoveredByGrant both match per segment only. +// evaluate() and isRequestCoveredByApproval both match per segment only. describe("a scope-mismatched grant never replays a multi-segment chain", () => { const full = "npm i && curl x"; const shellCall = (command: string): ToolCall => ({ @@ -247,7 +246,7 @@ describe("legacy whole-string chain grants are explicitly rejected", () => { expect(asked).toBe(1); }); - test("isRequestCoveredByGrant refuses a legacy full-chain pattern", () => { + test("isRequestCoveredByApproval refuses a legacy full-chain pattern", () => { const request: PermissionRequest = { tool: "run_shell", action: "Run", @@ -256,7 +255,7 @@ describe("legacy whole-string chain grants are explicitly rejected", () => { cwd: "/proj", }; expect( - isRequestCoveredByGrant( + isRequestCoveredByApproval( request, { tool: "run_shell", pattern: full }, undefined, @@ -275,7 +274,7 @@ describe("legacy whole-string chain grants are explicitly rejected", () => { cwd: "/proj", }; expect( - isRequestCoveredByGrant( + isRequestCoveredByApproval( request, { tool: "run_shell", pattern: "npm i" }, undefined, @@ -429,9 +428,9 @@ describe("foreign grant cwd matching request cwd under a different workspace is }); test("both live call sites refuse a foreign grant replaying into /proj's workspace", async () => { - // evaluateApprovals + // approvalCoversSubject expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "npm test", approvals: [foreignGrant], @@ -440,7 +439,7 @@ describe("foreign grant cwd matching request cwd under a different workspace is }), ).toBe(false); - // isRequestCoveredByGrant (request cwd coincides with the foreign grant's cwd) + // isRequestCoveredByApproval (request cwd coincides with the foreign grant's cwd) const request: PermissionRequest = { tool: "run_shell", action: "Run", @@ -449,7 +448,7 @@ describe("foreign grant cwd matching request cwd under a different workspace is cwd: "/foreign", }; expect( - isRequestCoveredByGrant( + isRequestCoveredByApproval( request, foreignGrant, undefined, @@ -461,7 +460,7 @@ describe("foreign grant cwd matching request cwd under a different workspace is test("the same grant with its own workspace still replays the session root", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "npm test", approvals: [ownGrant], diff --git a/src/permission/path-restriction.ts b/src/permission/path-restriction.ts index cbe2da6d2..94ea9aabf 100644 --- a/src/permission/path-restriction.ts +++ b/src/permission/path-restriction.ts @@ -1,7 +1,7 @@ import { lstatSync, readlinkSync, realpathSync } from "node:fs"; import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; -import type { RootsProvider } from "./worktree-roots.js"; +import { realpathOrOriginal, type RootsProvider } from "./worktree-roots.js"; import { projectSessionsRoot } from "../session/project-key.js"; // Paths the agent must not touch without explicit operator approval, even @@ -26,14 +26,6 @@ export interface PathRestriction { const LEGACY_STATE_DIR = ".agent-state"; -function realpathOr(path: string): string { - try { - return realpathSync(path); - } catch { - return path; - } -} - // Sentinel from realpathNearestOr for a path that exists but cannot be // resolved (dangling symlink or loop). Contains a NUL byte, which can never // appear in a real path, so it cannot collide with a genuine result. @@ -154,10 +146,9 @@ function isResolvedPathInWorkspace( rootsProvider: RootsProvider, ): boolean { if (real === UNRESOLVABLE) return false; - const realCwd = realpathOr(resolve(cwd)); + const realCwd = realpathOrOriginal(resolve(cwd)); if (real === realCwd || real.startsWith(realCwd + sep)) return true; if (inKnownRoots(real, rootsProvider())) return true; - if (inKnownRoots(real, rootsProvider(true))) return true; return false; } @@ -180,14 +171,14 @@ export function isPermittedSiblingWorktreePath( if (path.startsWith("/") || /^[A-Za-z]:[\\/]/.test(path)) return false; const abs = resolve(cwd, path); - const realParent = realpathOr(dirname(abs)); - const realHome = realpathOr(resolve(home)); + const realParent = realpathOrOriginal(dirname(abs)); + const realHome = realpathOrOriginal(resolve(home)); if (realParent === realHome) return false; - const knownRoots = [...rootsProvider(), ...rootsProvider(true)]; + const knownRoots = rootsProvider(); const trustedParents = new Set([ - realpathOr(resolve(cwd, "..")), - ...knownRoots.map((root) => realpathOr(dirname(root))), + realpathOrOriginal(resolve(cwd, "..")), + ...knownRoots.map((root) => realpathOrOriginal(dirname(root))), ]); return trustedParents.has(realParent); } @@ -226,8 +217,8 @@ export function createPathRestriction( isRestricted: (path: string, isWrite: boolean): boolean => { const abs = resolve(cwd, path); const cacheKey = `${isWrite ? "w" : "r"}:${abs}`; - // Use realpathNearestOr rather than realpathOr: for a not-yet-created - // target, realpathOr returns the raw path unchanged — the same cache key + // Use realpathNearestOr rather than realpathOrOriginal: for a not-yet-created + // target, realpathOrOriginal returns the raw path unchanged — the same cache key // before and after a symlink retarget. realpathNearestOr resolves the // nearest existing ancestor, so the key changes when a symlink flips. const currentRealpath = realpathNearestOr(abs); diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index 80b2fbc24..45c98661c 100644 --- a/src/permission/permission.test.ts +++ b/src/permission/permission.test.ts @@ -19,7 +19,7 @@ import { stripCommentLines, } from "./command.js"; import { matchesPattern, escapeGlobLiteral } from "./matcher.js"; -import { evaluateApprovals } from "./authz-grants.js"; +import { approvalCoversSubject } from "./authz-grants.js"; import { classifyTool, buildRequests, @@ -360,7 +360,7 @@ describe("matchesPattern directory-grant traversal (CL-8989)", () => { }); }); -describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { +describe("approvalCoversSubject (@intx/authz evaluateGrants)", () => { const approvals: Approval[] = [ { tool: "run_shell", pattern: "npm *" }, { tool: "write_file", pattern: "src/*" }, @@ -374,7 +374,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { test("allows package-compatible wildcard grants", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "npm test", approvals, @@ -382,7 +382,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { }), ).toBe(true); expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "curl x", approvals, @@ -390,7 +390,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { }), ).toBe(false); expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "write_file", subject: "src/a.ts", approvals, @@ -407,7 +407,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { ]; const workspace = { resolvedCwd: "/proj", roots: [] as string[] }; expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "write_file", subject: "/proj/sub/nested/file.txt", approvals: dirApprovals, @@ -420,7 +420,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { "/proj/sub//../evil", ]) { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "write_file", subject, approvals: dirApprovals, @@ -432,7 +432,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { test("a grant for read_file covers default.read_file", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "default.read_file", subject: "src/a.ts", approvals: [{ tool: "read_file", pattern: "src/*" }], @@ -443,7 +443,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { test("a grant for an MCP tool covers the default. prefixed name", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "default.mcp__linear__save_issue", subject: "mcp__linear__save_issue", approvals: [ @@ -459,7 +459,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { test("allows exact-escaped grants without treating * as a wildcard", async () => { expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "rm -rf build/*", approvals, @@ -467,7 +467,7 @@ describe("evaluateApprovals (@intx/authz evaluateGrants)", () => { }), ).toBe(true); expect( - await evaluateApprovals({ + await approvalCoversSubject({ tool: "run_shell", subject: "rm -rf build/../../etc", approvals, @@ -2643,7 +2643,7 @@ describe("createWorktreeRootsProvider lazy re-discovery", () => { ).toBe(true); } // One call seeds the initial (empty) roots; the debounce window suppresses - // every forced refresh inside it. + // every later refresh inside it. expect(listCalls).toBe(1); }); @@ -2659,8 +2659,8 @@ describe("createWorktreeRootsProvider lazy re-discovery", () => { const outside = mkdtempSync(join(tmpdir(), "corbits-window-")); expect(restriction.isRestricted(join(outside, "a.ts"), false)).toBe(true); expect(restriction.isRestricted(join(outside, "b.ts"), false)).toBe(true); - // A zero-width window makes the initial listing plus one forced refresh - // per check both eligible. + // A zero-width window makes the initial listing plus one refresh per check + // both eligible. expect(listCalls).toBeGreaterThan(1); }); @@ -2675,7 +2675,7 @@ describe("createWorktreeRootsProvider lazy re-discovery", () => { }; const provider = createWorktreeRootsProvider(repo, () => lister(), 0); expect(provider()).toEqual([removed]); - expect(provider(true)).toEqual([]); + expect(provider()).toEqual([]); }); }); diff --git a/src/permission/queue.test.ts b/src/permission/queue.test.ts index 9ec28e151..d29431edc 100644 --- a/src/permission/queue.test.ts +++ b/src/permission/queue.test.ts @@ -4,7 +4,7 @@ import { createPermissionRequestQueue, wirePermissionGrantReconciliation, } from "./queue.js"; -import { isRequestCoveredByGrant } from "./gate.js"; +import { isRequestCoveredByApproval } from "./gate.js"; import { createPathRestriction } from "./path-restriction.js"; import { createWorktreeRootsProvider } from "./worktree-roots.js"; import type { Approval, ApprovalOutcome, PermissionRequest } from "./types.js"; @@ -33,7 +33,7 @@ function coversFor( const isRestricted = createPathRestriction(cwd, rootsProvider).isRestricted; const workspace = { resolvedCwd: cwd, roots: rootsProvider() }; return (r) => - isRequestCoveredByGrant( + isRequestCoveredByApproval( r, approval, activeProviderModel, diff --git a/src/permission/queue.ts b/src/permission/queue.ts index bea083ad7..146cbdcda 100644 --- a/src/permission/queue.ts +++ b/src/permission/queue.ts @@ -1,7 +1,7 @@ /** * Headless queued-approval reconciliation. When a grant widens mid-run, the * permission layer decides which already-queued requests it now covers and - * settles them without a prompt (see isRequestCoveredByGrant in gate.ts, + * settles them without a prompt (see isRequestCoveredByApprovals in gate.ts, * which supplies the `covers` predicate this module drains against). A * rendering surface only enqueues its pending requests and dispatches * whatever settle calls come back — it never decides coverage itself. diff --git a/src/permission/worktree-roots.ts b/src/permission/worktree-roots.ts index fce9e4f7f..43fe5752f 100644 --- a/src/permission/worktree-roots.ts +++ b/src/permission/worktree-roots.ts @@ -30,7 +30,7 @@ function gitWorktreeList(cwd: string): Promise { // compares like with like. Exported so other permission-layer code comparing // a path against these roots (e.g. grant cwd matching in authz-grants.ts) // normalizes through the same function rather than reimplementing it. -export function realpathOr(path: string): string { +export function realpathOrOriginal(path: string): string { try { return realpathSync(path); } catch { @@ -42,9 +42,11 @@ function parseWorktreePorcelain(stdout: string, cwd: string): string[] { const roots: string[] = []; for (const line of stdout.split("\n")) { if (line.startsWith("worktree ")) - roots.push(realpathOr(resolve(line.slice("worktree ".length).trim()))); + roots.push( + realpathOrOriginal(resolve(line.slice("worktree ".length).trim())), + ); } - const self = realpathOr(resolve(cwd)); + const self = realpathOrOriginal(resolve(cwd)); return roots.filter((root) => root !== self); } @@ -78,12 +80,10 @@ export function listWorktreeRootsSync(cwd: string): string[] { } } -// Called with no argument to read the current best-known roots, or with -// `true` to ask for a refresh before reading. A refresh request only actually -// re-lists when the debounce window has elapsed since the last one — so a -// burst of refresh requests (e.g. several foreign-path checks in a row) -// shells out to git at most once per window. -export type RootsProvider = (forceRefresh?: boolean) => readonly string[]; +// Called to read the current best-known roots; re-lists only when the debounce +// window has elapsed since the last read, so a burst of checks (e.g. several +// foreign-path checks in a row) shells out to git at most once per window. +export type RootsProvider = () => readonly string[]; const DEFAULT_DEBOUNCE_MS = 3000; @@ -99,7 +99,7 @@ export function createWorktreeRootsProvider( let roots: string[] | undefined; let lastRefreshAt = -Infinity; - return (forceRefresh = false): readonly string[] => { + return (): readonly string[] => { if (roots === undefined) { roots = lister(cwd); lastRefreshAt = Date.now(); @@ -107,12 +107,10 @@ export function createWorktreeRootsProvider( } const now = Date.now(); const due = now - lastRefreshAt >= debounceMs; - if (due || forceRefresh) { - if (due) { - lastRefreshAt = now; - // Replace the cache wholesale so roots git no longer lists are evicted. - roots = lister(cwd); - } + if (due) { + lastRefreshAt = now; + // Replace the cache wholesale so roots git no longer lists are evicted. + roots = lister(cwd); } return roots; }; diff --git a/src/plugins/path-escape-plugin.ts b/src/plugins/path-escape-plugin.ts index e51b35b1c..decf051f1 100644 --- a/src/plugins/path-escape-plugin.ts +++ b/src/plugins/path-escape-plugin.ts @@ -4,7 +4,7 @@ import { isToolOutputLike } from "../util/tool-output-uri.js"; import { isArchiveLike } from "../session/compaction-archive.js"; import { resolveWorkspacePath } from "../permission/path-restriction.js"; import { - realpathOr, + realpathOrOriginal, type RootsProvider, } from "../permission/worktree-roots.js"; import { canonicalToolName } from "../agent/canonical-tool-name.js"; @@ -47,9 +47,9 @@ function rootsForEscape( ) { return rootsProvider; } - return (refresh?: boolean) => [ - ...rootsProvider(refresh), - ...trustedPluginRoots(refresh).map(realpathOr), + return () => [ + ...rootsProvider(), + ...trustedPluginRoots().map(realpathOrOriginal), ]; } diff --git a/src/plugins/shell-guard-plugin.test.ts b/src/plugins/shell-guard-plugin.test.ts index 325b5d957..4f46324ac 100644 --- a/src/plugins/shell-guard-plugin.test.ts +++ b/src/plugins/shell-guard-plugin.test.ts @@ -288,7 +288,7 @@ describe("background run_shell (shellGuardPlugin)", () => { // The START call resolved while the child was still sleeping. expect(Date.now() - start).toBeLessThan(400); await recorder.waitFor(parsed.shell_id); - registry.disposeAll("test done"); + registry.disposeAll(); }); test("without a registry wired, background fails closed and spawns nothing", async () => { @@ -320,7 +320,7 @@ describe("background run_shell (shellGuardPlugin)", () => { neverAbort(), ); expect(String(after.content).trim()).toBe(process.cwd()); - registry.disposeAll("test done"); + registry.disposeAll(); }); test("foreground run_shell is unchanged when background is unset", async () => { @@ -333,7 +333,7 @@ describe("background run_shell (shellGuardPlugin)", () => { expect(result.isError).toBeUndefined(); expect(String(result.content)).toContain("direct"); expect(registry.runningCount()).toBe(0); - registry.disposeAll("test done"); + registry.disposeAll(); }); test("background omitted timeout skips the foreground default", async () => { @@ -362,7 +362,7 @@ describe("background run_shell (shellGuardPlugin)", () => { const exit = await recorder.waitFor(parsed.shell_id); expect(exit.timedOut).toBe(false); expect(exit.output).toContain("survived"); - registry.disposeAll("test done"); + registry.disposeAll(); }); test("an unwired shell-output feed spawns fine and paints no tail", async () => { diff --git a/src/shell/background-shell.test.ts b/src/shell/background-shell.test.ts index 060a00224..36efeadd3 100644 --- a/src/shell/background-shell.test.ts +++ b/src/shell/background-shell.test.ts @@ -86,7 +86,7 @@ describe("background shell registry", () => { } const over = registry.start({ command: "sleep 30", cwd: tmpCwd }); expect("error" in over).toBe(true); - registry.disposeAll("test done"); + registry.disposeAll(); }); test("disposeAll kills running children without delivering an exit", async () => { @@ -100,7 +100,7 @@ describe("background shell registry", () => { cwd: tmpCwd, }); if ("error" in started) throw new Error(started.error); - registry.disposeAll("session closed"); + registry.disposeAll(); await waitUntilGone(token); expect(registry.runningCount()).toBe(0); expect(exits).toHaveLength(0); @@ -126,7 +126,7 @@ describe("background shell registry", () => { expect(Date.now() - t0).toBeLessThan(2_000); expect(exit.exitCode).toBe(0); } finally { - registry.disposeAll("test done"); + registry.disposeAll(); spawnSync("pkill", ["-f", token]); await waitUntilGone(token); } diff --git a/src/shell/background-shell.ts b/src/shell/background-shell.ts index cc524db0a..6c3d793b7 100644 --- a/src/shell/background-shell.ts +++ b/src/shell/background-shell.ts @@ -60,7 +60,7 @@ export interface BackgroundShellExit { export interface BackgroundShellRegistry { start: (args: StartBackgroundShellArgs) => { id: string } | { error: string }; cancel: (id: string) => boolean; - disposeAll: (reason: string) => void; + disposeAll: () => void; runningCount: () => number; } @@ -147,12 +147,11 @@ export function createBackgroundShellRegistry( return true; }; - const disposeAll = (reason: string): void => { + const disposeAll = (): void => { for (const child of running.values()) killProcessTree(child); running.clear(); // No onExit for disposed shells: the session is gone, so there is no later // turn to deliver to. - void reason; }; return { diff --git a/src/shell/persistent-shell-cwd.ts b/src/shell/persistent-shell-cwd.ts index 146611787..b84b7dee9 100644 --- a/src/shell/persistent-shell-cwd.ts +++ b/src/shell/persistent-shell-cwd.ts @@ -108,13 +108,7 @@ export function assertShellCwdUsable(cwd: string): void { if (!st.isDirectory()) { throw new Error(missingShellCwdMessage(cwd)); } - } catch (err) { - if ( - err instanceof Error && - err.message.startsWith("Shell working directory") - ) { - throw err; - } + } catch { throw new Error(missingShellCwdMessage(cwd)); } } diff --git a/src/subagent/run-persist-close.test.ts b/src/subagent/run-persist-close.test.ts index cb05d37b6..ea1ecb0cf 100644 --- a/src/subagent/run-persist-close.test.ts +++ b/src/subagent/run-persist-close.test.ts @@ -153,7 +153,7 @@ describe("worker persist reaps leftover registry children", () => { const cwd = await tmpSubAgentCwd("corbits-worker-persist-reap-"); const token = `ic_worker_persist_${randomUUID()}`; let registry: BackgroundShellRegistry | undefined; - const disposeReasons: string[] = []; + let disposeAllCalls = 0; await withMockedModuleDuring( import.meta.resolve("../shell/background-shell.js"), @@ -165,9 +165,9 @@ describe("worker persist reaps leftover registry children", () => { const inner = real.createBackgroundShellRegistry(opts); const wrapped: BackgroundShellRegistry = { ...inner, - disposeAll: (reason: string) => { - disposeReasons.push(reason); - inner.disposeAll(reason); + disposeAll: () => { + disposeAllCalls += 1; + inner.disposeAll(); }, }; registry = wrapped; @@ -215,14 +215,14 @@ describe("worker persist reaps leftover registry children", () => { }), ); expect(result.agentRetained).toBe(true); - expect(disposeReasons).toEqual(["sub-agent closed"]); + expect(disposeAllCalls).toBe(1); const captured = defined(registry); expect(captured.runningCount()).toBe(0); if (process.platform !== "win32") { await waitUntilGone(token); } } finally { - registry?.disposeAll("test done"); + registry?.disposeAll(); await handles .peek() ?.close(1000) diff --git a/src/subagent/run.ts b/src/subagent/run.ts index f0a943a54..3dcd6c212 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -1564,7 +1564,7 @@ async function runSubAgentInner( closeOnAbort = (): void => { // interrupt_agent is the keep-alive path: it never reaches here, so its // background children stay. - backgroundShells.disposeAll("parent abort"); + backgroundShells.disposeAll(); void (async () => { try { await agent?.close(); @@ -1592,7 +1592,7 @@ async function runSubAgentInner( ): Promise => { if (!runController.signal.aborted) runController.abort(new Error("closed by close_agent")); - backgroundShells.disposeAll("closed by close_agent"); + backgroundShells.disposeAll(); try { await awaitBoundedTeardown( disposeSubAgentSession({ @@ -1896,7 +1896,7 @@ async function runSubAgentInner( // A persisted, cleanly-completed session skips teardown — it stays open // until close_agent (or a later failed/aborted run) tears it down. if (!persisting || !backgroundShellsMounted) { - backgroundShells.disposeAll("sub-agent closed"); + backgroundShells.disposeAll(); } if (!persisting) { // Bounded like close_agent: teardown wedged by a live shell descendant