From a31058d76cfd4514a62b9f83b54104f814197c0a Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Wed, 12 Aug 2026 21:09:52 -0400 Subject: [PATCH] fix(core): skip shell parsing when permissions allow all (#42203) --- packages/core/src/permission.ts | 25 ++++++- packages/core/src/tool/plugin/shell.ts | 67 +++++++++++-------- packages/core/test/permission.test.ts | 25 +++++++ .../core/test/session-instructions.test.ts | 1 + .../core/test/session-runner-recorded.test.ts | 1 + packages/core/test/session-runner.test.ts | 1 + packages/core/test/tool-edit.test.ts | 1 + packages/core/test/tool-patch.test.ts | 1 + packages/core/test/tool-question.test.ts | 1 + packages/core/test/tool-read.test.ts | 1 + packages/core/test/tool-search.test.ts | 1 + packages/core/test/tool-shell.test.ts | 27 ++++++++ packages/core/test/tool-skill.test.ts | 1 + packages/core/test/tool-webfetch.test.ts | 1 + packages/core/test/tool-websearch.test.ts | 1 + packages/core/test/tool-write.test.ts | 1 + 16 files changed, 128 insertions(+), 28 deletions(-) diff --git a/packages/core/src/permission.ts b/packages/core/src/permission.ts index 22e8d6e395a..d7c6bf34ea9 100644 --- a/packages/core/src/permission.ts +++ b/packages/core/src/permission.ts @@ -99,6 +99,11 @@ export function merge(...rulesets: Permission.Ruleset[]): Permission.Ruleset { } export interface Interface { + readonly allowsAll: (input: { + readonly sessionID: SessionSchema.ID + readonly action: string + readonly agent?: Agent.ID + }) => Effect.Effect readonly ask: (input: AssertInput) => Effect.Effect readonly assert: (input: AssertInput) => Effect.Effect readonly reply: (input: ReplyInput) => Effect.Effect @@ -154,6 +159,24 @@ const layer = Layer.effect( return agent?.permissions ?? missingAgentPermissions }) + const allowsAll = Effect.fn("Permission.allowsAll")(function* (input: { + readonly sessionID: SessionSchema.ID + readonly action: string + readonly agent?: Agent.ID + }) { + const rules = yield* configured(input.sessionID, input.agent) + const relevant = rules.filter((rule) => Wildcard.match(input.action, rule.action)) + for (let index = relevant.length - 1; index >= 0; index--) { + const rule = relevant[index] + if (rule.resource !== "*") { + if (rule.effect !== "allow") return false + continue + } + return rule.effect === "allow" + } + return false + }) + function denied(input: AssertInput, rules: Permission.Ruleset) { return input.resources.some((resource) => evaluate(input.action, resource, rules).effect === "deny") } @@ -315,7 +338,7 @@ const layer = Layer.effect( return Array.from(pending.values(), (item) => item.request).filter((request) => request.sessionID === sessionID) }) - return Service.of({ ask, assert, reply, get, forSession, list }) + return Service.of({ allowsAll, ask, assert, reply, get, forSession, list }) }), ) diff --git a/packages/core/src/tool/plugin/shell.ts b/packages/core/src/tool/plugin/shell.ts index 367e9eee51e..4e65df61eca 100644 --- a/packages/core/src/tool/plugin/shell.ts +++ b/packages/core/src/tool/plugin/shell.ts @@ -149,36 +149,49 @@ export const Plugin = { (invocation) => Effect.gen(function* () { const target = yield* mutation.resolve({ path: invocation.cwd, kind: "directory" }) - const parsed = yield* ShellParse.scan(invocation.command, invocation.shell, target.absolute) - const directories = yield* Effect.forEach(parsed.directories, (directory) => - mutation.resolve({ path: path.resolve(target.absolute, directory), kind: "directory" }), - ) + const unrestricted = + (yield* permission.allowsAll({ + sessionID: context.sessionID, + action: name, + agent: context.agent, + })) && + (yield* permission.allowsAll({ + sessionID: context.sessionID, + action: "external_directory", + agent: context.agent, + })) invocation.cwd = target.absolute finalTimeout = invocation.timeout - const external = [target, ...directories] - .map((item) => item.externalDirectory) - .filter((item) => item !== undefined) - .filter( - (item, index, items) => items.findIndex((other) => other.resource === item.resource) === index, + if (!unrestricted) { + const parsed = yield* ShellParse.scan(invocation.command, invocation.shell, target.absolute) + const directories = yield* Effect.forEach(parsed.directories, (directory) => + mutation.resolve({ path: path.resolve(target.absolute, directory), kind: "directory" }), ) - if (external.length > 0) - yield* permission.assert({ - action: "external_directory", - resources: external.map((item) => item.resource), - save: external.map((item) => item.save), - sessionID: context.sessionID, - agent: context.agent, - source, - }) - if (parsed.commands.length > 0) - yield* permission.assert({ - action: name, - resources: parsed.commands.map((command) => command.resource), - save: parsed.commands.map((command) => command.save), - sessionID: context.sessionID, - agent: context.agent, - source, - }) + const external = [target, ...directories] + .map((item) => item.externalDirectory) + .filter((item) => item !== undefined) + .filter( + (item, index, items) => items.findIndex((other) => other.resource === item.resource) === index, + ) + if (external.length > 0) + yield* permission.assert({ + action: "external_directory", + resources: external.map((item) => item.resource), + save: external.map((item) => item.save), + sessionID: context.sessionID, + agent: context.agent, + source, + }) + if (parsed.commands.length > 0) + yield* permission.assert({ + action: name, + resources: parsed.commands.map((command) => command.resource), + save: parsed.commands.map((command) => command.save), + sessionID: context.sessionID, + agent: context.agent, + source, + }) + } const workdir = yield* Environment.typeFollowing(environment.files, target.absolute).pipe( Effect.catchTag("Environment.NotFound", () => Effect.fail(new Error(`Working directory does not exist: ${target.absolute}`)), diff --git a/packages/core/test/permission.test.ts b/packages/core/test/permission.test.ts index e8565abe238..ac83bf7f28e 100644 --- a/packages/core/test/permission.test.ts +++ b/packages/core/test/permission.test.ts @@ -112,6 +112,31 @@ describe("Permission", () => { }), ) + it.effect("proves only unconditional configured allows", () => + Effect.gen(function* () { + const service = yield* Permission.Service + const input = { sessionID: Session.ID.make("ses_test"), action: "shell" } + + yield* setup([{ action: "shell", resource: "*", effect: "allow" }]) + expect(yield* service.allowsAll(input)).toBe(true) + + yield* setRules([ + { action: "shell", resource: "*", effect: "allow" }, + { action: "shell", resource: "rm *", effect: "deny" }, + ]) + expect(yield* service.allowsAll(input)).toBe(false) + + yield* setRules([{ action: "shell", resource: "git *", effect: "allow" }]) + expect(yield* service.allowsAll(input)).toBe(false) + + yield* setRules([ + { action: "shell", resource: "rm *", effect: "deny" }, + { action: "shell", resource: "*", effect: "allow" }, + ]) + expect(yield* service.allowsAll(input)).toBe(true) + }), + ) + it.effect("evaluates against an explicit provider-turn agent", () => Effect.gen(function* () { yield* setup([{ action: "read", resource: "*", effect: "allow" }]) diff --git a/packages/core/test/session-instructions.test.ts b/packages/core/test/session-instructions.test.ts index a738499a609..8e20c8df78d 100644 --- a/packages/core/test/session-instructions.test.ts +++ b/packages/core/test/session-instructions.test.ts @@ -61,6 +61,7 @@ const projects = Layer.succeed( const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: () => Effect.void, ask: () => Effect.die("unused"), reply: () => Effect.die("unused"), diff --git a/packages/core/test/session-runner-recorded.test.ts b/packages/core/test/session-runner-recorded.test.ts index 74613a505fb..95a8fea11e0 100644 --- a/packages/core/test/session-runner-recorded.test.ts +++ b/packages/core/test/session-runner-recorded.test.ts @@ -58,6 +58,7 @@ const client = LLMClient.layer.pipe(Layer.provide(executor)) const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: () => Effect.die("unused"), ask: () => Effect.die("unused"), reply: () => Effect.die("unused"), diff --git a/packages/core/test/session-runner.test.ts b/packages/core/test/session-runner.test.ts index 592ac806fb9..667d4db2841 100644 --- a/packages/core/test/session-runner.test.ts +++ b/packages/core/test/session-runner.test.ts @@ -221,6 +221,7 @@ const permissionFail = { const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: () => Effect.die("unused"), ask: () => Effect.die("unused"), reply: () => Effect.die("unused"), diff --git a/packages/core/test/tool-edit.test.ts b/packages/core/test/tool-edit.test.ts index 10bb94f8179..da66c895231 100644 --- a/packages/core/test/tool-edit.test.ts +++ b/packages/core/test/tool-edit.test.ts @@ -46,6 +46,7 @@ let formatFile = (_target: string): Effect.Effect => Effect.succeed(fal const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => assertions.push(input)).pipe( Effect.andThen( diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index 9c1cc590905..95d1064bf36 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/packages/core/test/tool-patch.test.ts @@ -41,6 +41,7 @@ let formatFile = (_target: string): Effect.Effect => Effect.succeed(fal const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => { assertions.push(input) diff --git a/packages/core/test/tool-question.test.ts b/packages/core/test/tool-question.test.ts index 8fe3d6ab017..01efc6d2d69 100644 --- a/packages/core/test/tool-question.test.ts +++ b/packages/core/test/tool-question.test.ts @@ -31,6 +31,7 @@ const questionInput = { const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => assertions.push(input)).pipe( Effect.andThen( diff --git a/packages/core/test/tool-read.test.ts b/packages/core/test/tool-read.test.ts index de490f57eb8..81874257154 100644 --- a/packages/core/test/tool-read.test.ts +++ b/packages/core/test/tool-read.test.ts @@ -73,6 +73,7 @@ let allow = true const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => { assertions.push(input) diff --git a/packages/core/test/tool-search.test.ts b/packages/core/test/tool-search.test.ts index a87c45cd883..9d5d5bfc1fd 100644 --- a/packages/core/test/tool-search.test.ts +++ b/packages/core/test/tool-search.test.ts @@ -52,6 +52,7 @@ const withTools = ( Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => { assertions?.push(input) diff --git a/packages/core/test/tool-shell.test.ts b/packages/core/test/tool-shell.test.ts index 0e6ac46cb2e..a90e72bd604 100644 --- a/packages/core/test/tool-shell.test.ts +++ b/packages/core/test/tool-shell.test.ts @@ -44,12 +44,14 @@ import { toolIdentity, executeTool, registerToolPlugin, toolDefinitions } from " const sessionID = Session.ID.make("ses_shell_tool_test") const sessionModel = Model.Ref.make({ id: Model.ID.make("test"), providerID: Provider.ID.make("test") }) const assertions: Permission.AssertInput[] = [] +const allowedActions = new Set() let denyAction: string | undefined let afterPermission = (_input: Permission.AssertInput): Effect.Effect => Effect.void const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: (input) => Effect.succeed(allowedActions.has(input.action)), assert: (input) => Effect.sync(() => assertions.push(input)).pipe( Effect.andThen(Effect.suspend(() => afterPermission(input))), @@ -75,6 +77,7 @@ const permission = Layer.succeed( const reset = () => { assertions.length = 0 + allowedActions.clear() denyAction = undefined afterPermission = () => Effect.void } @@ -337,6 +340,30 @@ describe("ShellTool", () => { { timeout: 15_000 }, ) + it.live( + "skips command decomposition when shell and external directories are unrestricted", + () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => { + reset() + allowedActions.add("shell") + allowedActions.add("external_directory") + return withSession(tmp.path, (registry) => + executeTool(registry, call({ command: "printf one && printf two" }, "call-unrestricted")), + ).pipe( + Effect.andThen( + Effect.sync(() => { + expect(assertions).toEqual([]) + }), + ), + ) + }, + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]().then(() => undefined)), + ), + { timeout: 15_000 }, + ) + it.live( "captures stderr-only and mixed stdout/stderr output", () => diff --git a/packages/core/test/tool-skill.test.ts b/packages/core/test/tool-skill.test.ts index 7872d0fef1c..174c11a540c 100644 --- a/packages/core/test/tool-skill.test.ts +++ b/packages/core/test/tool-skill.test.ts @@ -55,6 +55,7 @@ describe("SkillTool", () => { const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => assertions.push(input)).pipe( Effect.andThen( diff --git a/packages/core/test/tool-webfetch.test.ts b/packages/core/test/tool-webfetch.test.ts index 315272d42da..d94b698a8a7 100644 --- a/packages/core/test/tool-webfetch.test.ts +++ b/packages/core/test/tool-webfetch.test.ts @@ -39,6 +39,7 @@ const http = Layer.succeed( const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => assertions.push(input)), ask: () => Effect.die("unused"), reply: () => Effect.die("unused"), diff --git a/packages/core/test/tool-websearch.test.ts b/packages/core/test/tool-websearch.test.ts index 8275a628263..8316d142f2c 100644 --- a/packages/core/test/tool-websearch.test.ts +++ b/packages/core/test/tool-websearch.test.ts @@ -69,6 +69,7 @@ beforeEach(() => { const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => assertions.push(input)), ask: () => Effect.die("unused"), reply: () => Effect.die("unused"), diff --git a/packages/core/test/tool-write.test.ts b/packages/core/test/tool-write.test.ts index 46d2ea6fab4..68f45d53d00 100644 --- a/packages/core/test/tool-write.test.ts +++ b/packages/core/test/tool-write.test.ts @@ -36,6 +36,7 @@ let denyAction: string | undefined const permission = Layer.succeed( Permission.Service, Permission.Service.of({ + allowsAll: () => Effect.succeed(false), assert: (input) => Effect.sync(() => assertions.push(input)).pipe( Effect.andThen(