From f31006b1cd2b889cd2816651b695b4b50240204e Mon Sep 17 00:00:00 2001 From: Dax Raad Date: Wed, 31 Dec 2025 18:46:07 -0500 Subject: [PATCH] core: migrate permission system to new PermissionNext API Replaces legacy Permission.respond with PermissionNext.reply for better async handling and updates server endpoints to use the new permission system. Improves error handling in session processor to work with both old and new permission rejection types. --- packages/opencode/src/permission/index.ts | 1 - packages/opencode/src/permission/next.ts | 4 + packages/opencode/src/server/server.ts | 16 +- packages/opencode/src/session/processor.ts | 11 +- packages/opencode/test/config/config.test.ts | 2 +- packages/opencode/test/tool/bash.test.ts | 503 +++++-------------- packages/opencode/test/tool/patch.test.ts | 5 +- packages/opencode/test/tool/read.test.ts | 104 ++-- 8 files changed, 191 insertions(+), 455 deletions(-) diff --git a/packages/opencode/src/permission/index.ts b/packages/opencode/src/permission/index.ts index 5fbd621ae0..f1cd43fdbe 100644 --- a/packages/opencode/src/permission/index.ts +++ b/packages/opencode/src/permission/index.ts @@ -6,7 +6,6 @@ import { Identifier } from "../id/id" import { Plugin } from "../plugin" import { Instance } from "../project/instance" import { Wildcard } from "../util/wildcard" -import { Config } from "@/config/config" export namespace Permission { const log = Log.create({ service: "permission" }) diff --git a/packages/opencode/src/permission/next.ts b/packages/opencode/src/permission/next.ts index 5102a22fdc..83e98a70fa 100644 --- a/packages/opencode/src/permission/next.ts +++ b/packages/opencode/src/permission/next.ts @@ -246,4 +246,8 @@ export namespace PermissionNext { ) } } + + export async function list() { + return state().then((x) => Object.values(x.pending).map((x) => x.info)) + } } diff --git a/packages/opencode/src/server/server.ts b/packages/opencode/src/server/server.ts index 35ebcda643..9d75308c1c 100644 --- a/packages/opencode/src/server/server.ts +++ b/packages/opencode/src/server/server.ts @@ -1525,6 +1525,7 @@ export namespace Server { "/session/:sessionID/permissions/:permissionID", describeRoute({ summary: "Respond to permission", + deprecated: true, description: "Approve or deny a permission request from the AI assistant.", operationId: "permission.respond", responses: { @@ -1546,15 +1547,12 @@ export namespace Server { permissionID: z.string(), }), ), - validator("json", z.object({ response: Permission.Response })), + validator("json", z.object({ response: PermissionNext.Reply })), async (c) => { const params = c.req.valid("param") - const sessionID = params.sessionID - const permissionID = params.permissionID - Permission.respond({ - sessionID, - permissionID, - response: c.req.valid("json").response, + PermissionNext.reply({ + requestID: params.permissionID, + reply: c.req.valid("json").response, }) return c.json(true) }, @@ -1605,14 +1603,14 @@ export namespace Server { description: "List of pending permissions", content: { "application/json": { - schema: resolver(Permission.Info.array()), + schema: resolver(PermissionNext.Request.array()), }, }, }, }, }), async (c) => { - const permissions = Permission.list() + const permissions = await PermissionNext.list() return c.json(permissions) }, ) diff --git a/packages/opencode/src/session/processor.ts b/packages/opencode/src/session/processor.ts index c8771345bf..227ca64bb9 100644 --- a/packages/opencode/src/session/processor.ts +++ b/packages/opencode/src/session/processor.ts @@ -3,7 +3,6 @@ import { Log } from "@/util/log" import { Identifier } from "@/id/id" import { Session } from "." import { Agent } from "@/agent/agent" -import { Permission } from "@/permission" import { Snapshot } from "@/snapshot" import { SessionSummary } from "./summary" import { Bus } from "@/bus" @@ -202,11 +201,6 @@ export namespace SessionProcessor { status: "error", input: value.input, error: (value.error as any).toString(), - metadata: - value.error instanceof Permission.RejectedError || - value.error instanceof Permission.RejectedError - ? value.error.metadata - : undefined, time: { start: match.state.time.start, end: Date.now(), @@ -214,10 +208,7 @@ export namespace SessionProcessor { }, }) - if ( - value.error instanceof Permission.RejectedError || - value.error instanceof PermissionNext.RejectedError - ) { + if (value.error instanceof PermissionNext.RejectedError) { blocked = shouldBreak } delete toolcalls[value.toolCallId] diff --git a/packages/opencode/test/config/config.test.ts b/packages/opencode/test/config/config.test.ts index 9288bbbdaa..c2ed3abe8f 100644 --- a/packages/opencode/test/config/config.test.ts +++ b/packages/opencode/test/config/config.test.ts @@ -478,7 +478,7 @@ Helper subagent prompt`, directory: tmp.path, fn: async () => { const config = await Config.get() - expect(config.agent?.["helper"]).toEqual({ + expect(config.agent?.["helper"]).toMatchObject({ name: "helper", model: "test/model", mode: "subagent", diff --git a/packages/opencode/test/tool/bash.test.ts b/packages/opencode/test/tool/bash.test.ts index 31875d556b..ee82813fb5 100644 --- a/packages/opencode/test/tool/bash.test.ts +++ b/packages/opencode/test/tool/bash.test.ts @@ -2,8 +2,8 @@ import { describe, expect, test } from "bun:test" import path from "path" import { BashTool } from "../../src/tool/bash" import { Instance } from "../../src/project/instance" -import { Permission } from "../../src/permission" import { tmpdir } from "../fixture/fixture" +import type { PermissionNext } from "../../src/permission/next" const ctx = { sessionID: "test", @@ -38,397 +38,164 @@ describe("tool.bash", () => { }) describe("tool.bash permissions", () => { - test("allows command matching allow pattern", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "echo *": "allow", - "*": "deny", - }, - }, - }), - ) - }, - }) + test("asks for bash permission with correct pattern", async () => { + await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, fn: async () => { const bash = await BashTool.init() - const result = await bash.execute( + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await bash.execute( { command: "echo hello", description: "Echo hello", }, - ctx, + testCtx, ) - expect(result.metadata.exit).toBe(0) - expect(result.metadata.output).toContain("hello") + expect(requests.length).toBe(1) + expect(requests[0].permission).toBe("bash") + expect(requests[0].patterns).toContain("echo hello") }, }) }) - test("denies command matching deny pattern", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "curl *": "deny", - "*": "allow", - }, - }, - }), - ) - }, - }) + test("asks for bash permission with multiple commands", async () => { + await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, fn: async () => { const bash = await BashTool.init() - await expect( - bash.execute( - { - command: "curl https://example.com", - description: "Fetch URL", - }, - ctx, - ), - ).rejects.toThrow("restricted") - }, - }) - }) - - test("denies all commands with wildcard deny", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "*": "deny", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - await expect( - bash.execute( - { - command: "ls", - description: "List files", - }, - ctx, - ), - ).rejects.toThrow("restricted") - }, - }) - }) - - test("more specific pattern overrides general pattern", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "*": "deny", - "ls *": "allow", - "pwd*": "allow", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - // ls should be allowed - const result = await bash.execute( - { - command: "ls -la", - description: "List files", + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) }, - ctx, - ) - expect(result.metadata.exit).toBe(0) - - // pwd should be allowed - const pwd = await bash.execute( - { - command: "pwd", - description: "Print working directory", - }, - ctx, - ) - expect(pwd.metadata.exit).toBe(0) - - // cat should be denied - await expect( - bash.execute( - { - command: "cat /etc/passwd", - description: "Read file", - }, - ctx, - ), - ).rejects.toThrow("restricted") - }, - }) - }) - - test("denies dangerous subcommands while allowing safe ones", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "find *": "allow", - "find * -delete*": "deny", - "find * -exec*": "deny", - "*": "deny", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - // Basic find should work - const result = await bash.execute( - { - command: "find . -name '*.ts'", - description: "Find typescript files", - }, - ctx, - ) - expect(result.metadata.exit).toBe(0) - - // find -delete should be denied - await expect( - bash.execute( - { - command: "find . -name '*.tmp' -delete", - description: "Delete temp files", - }, - ctx, - ), - ).rejects.toThrow("restricted") - - // find -exec should be denied - await expect( - bash.execute( - { - command: "find . -name '*.ts' -exec cat {} \\;", - description: "Find and cat files", - }, - ctx, - ), - ).rejects.toThrow("restricted") - }, - }) - }) - - test("allows git read commands while denying writes", async () => { - await using tmp = await tmpdir({ - git: true, - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "git status*": "allow", - "git log*": "allow", - "git diff*": "allow", - "git branch": "allow", - "git commit *": "deny", - "git push *": "deny", - "*": "deny", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - // git status should work - const status = await bash.execute( - { - command: "git status", - description: "Git status", - }, - ctx, - ) - expect(status.metadata.exit).toBe(0) - - // git log should work - const log = await bash.execute( - { - command: "git log --oneline -5", - description: "Git log", - }, - ctx, - ) - expect(log.metadata.exit).toBe(0) - - // git commit should be denied - await expect( - bash.execute( - { - command: "git commit -m 'test'", - description: "Git commit", - }, - ctx, - ), - ).rejects.toThrow("restricted") - - // git push should be denied - await expect( - bash.execute( - { - command: "git push origin main", - description: "Git push", - }, - ctx, - ), - ).rejects.toThrow("restricted") - }, - }) - }) - - test("denies external directory access when permission is deny", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "deny", - bash: { - "*": "allow", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - // Should deny cd to parent directory (cd is checked for external paths) - await expect( - bash.execute( - { - command: "cd ../", - description: "Change to parent directory", - }, - ctx, - ), - ).rejects.toThrow() - }, - }) - }) - - test("denies workdir outside project when external_directory is deny", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "deny", - bash: { - "*": "allow", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - await expect( - bash.execute( - { - command: "ls", - workdir: "/tmp", - description: "List /tmp", - }, - ctx, - ), - ).rejects.toThrow() - }, - }) - }) - - test("handles multiple commands in sequence", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - bash: { - "echo *": "allow", - "curl *": "deny", - "*": "deny", - }, - }, - }), - ) - }, - }) - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const bash = await BashTool.init() - // echo && echo should work - const result = await bash.execute( + } + await bash.execute( { command: "echo foo && echo bar", description: "Echo twice", }, - ctx, + testCtx, ) - expect(result.metadata.output).toContain("foo") - expect(result.metadata.output).toContain("bar") + expect(requests.length).toBe(1) + expect(requests[0].permission).toBe("bash") + expect(requests[0].patterns).toContain("echo foo") + expect(requests[0].patterns).toContain("echo bar") + }, + }) + }) - // echo && curl should fail (curl is denied) - await expect( - bash.execute( - { - command: "echo hi && curl https://example.com", - description: "Echo then curl", - }, - ctx, - ), - ).rejects.toThrow("restricted") + test("asks for external_directory permission when cd to parent", async () => { + await using tmp = await tmpdir({ git: true }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const bash = await BashTool.init() + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await bash.execute( + { + command: "cd ../", + description: "Change to parent directory", + }, + testCtx, + ) + const extDirReq = requests.find((r) => r.permission === "external_directory") + expect(extDirReq).toBeDefined() + }, + }) + }) + + test("asks for external_directory permission when workdir is outside project", async () => { + await using tmp = await tmpdir({ git: true }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const bash = await BashTool.init() + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await bash.execute( + { + command: "ls", + workdir: "/tmp", + description: "List /tmp", + }, + testCtx, + ) + const extDirReq = requests.find((r) => r.permission === "external_directory") + expect(extDirReq).toBeDefined() + expect(extDirReq!.patterns).toContain("/tmp") + }, + }) + }) + + test("includes always patterns for auto-approval", async () => { + await using tmp = await tmpdir({ git: true }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const bash = await BashTool.init() + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await bash.execute( + { + command: "git log --oneline -5", + description: "Git log", + }, + testCtx, + ) + expect(requests.length).toBe(1) + expect(requests[0].always.length).toBeGreaterThan(0) + expect(requests[0].always.some((p) => p.endsWith("*"))).toBe(true) + }, + }) + }) + + test("does not ask for bash permission when command is cd only", async () => { + await using tmp = await tmpdir({ git: true }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const bash = await BashTool.init() + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await bash.execute( + { + command: "cd .", + description: "Stay in current directory", + }, + testCtx, + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeUndefined() }, }) }) diff --git a/packages/opencode/test/tool/patch.test.ts b/packages/opencode/test/tool/patch.test.ts index 2a0df48f98..3d3ec574e6 100644 --- a/packages/opencode/test/tool/patch.test.ts +++ b/packages/opencode/test/tool/patch.test.ts @@ -3,7 +3,7 @@ import path from "path" import { PatchTool } from "../../src/tool/patch" import { Instance } from "../../src/project/instance" import { tmpdir } from "../fixture/fixture" -import { Permission } from "../../src/permission" +import { PermissionNext } from "../../src/permission/next" import * as fs from "fs/promises" const ctx = { @@ -60,7 +60,8 @@ describe("tool.patch", () => { patchTool.execute({ patchText: maliciousPatch }, ctx) // TODO: this sucks await new Promise((resolve) => setTimeout(resolve, 1000)) - expect(Permission.pending()[ctx.sessionID]).toBeDefined() + const pending = await PermissionNext.list() + expect(pending.find((p) => p.sessionID === ctx.sessionID)).toBeDefined() }, }) }) diff --git a/packages/opencode/test/tool/read.test.ts b/packages/opencode/test/tool/read.test.ts index a808070a6d..1093a17fea 100644 --- a/packages/opencode/test/tool/read.test.ts +++ b/packages/opencode/test/tool/read.test.ts @@ -3,6 +3,7 @@ import path from "path" import { ReadTool } from "../../src/tool/read" import { Instance } from "../../src/project/instance" import { tmpdir } from "../fixture/fixture" +import type { PermissionNext } from "../../src/permission/next" const ctx = { sessionID: "test", @@ -19,14 +20,6 @@ describe("tool.read external_directory permission", () => { await using tmp = await tmpdir({ init: async (dir) => { await Bun.write(path.join(dir, "test.txt"), "hello world") - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "deny", - }, - }), - ) }, }) await Instance.provide({ @@ -43,14 +36,6 @@ describe("tool.read external_directory permission", () => { await using tmp = await tmpdir({ init: async (dir) => { await Bun.write(path.join(dir, "subdir", "test.txt"), "nested content") - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "deny", - }, - }), - ) }, }) await Instance.provide({ @@ -63,83 +48,74 @@ describe("tool.read external_directory permission", () => { }) }) - test("denies reading absolute path outside project directory", async () => { + test("asks for external_directory permission when reading absolute path outside project", async () => { await using outerTmp = await tmpdir({ init: async (dir) => { await Bun.write(path.join(dir, "secret.txt"), "secret data") }, }) - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "deny", - }, - }), - ) - }, - }) + await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, fn: async () => { const read = await ReadTool.init() - await expect(read.execute({ filePath: path.join(outerTmp.path, "secret.txt") }, ctx)).rejects.toThrow( - "not in the current working directory", - ) + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await read.execute({ filePath: path.join(outerTmp.path, "secret.txt") }, testCtx) + const extDirReq = requests.find((r) => r.permission === "external_directory") + expect(extDirReq).toBeDefined() + expect(extDirReq!.patterns.some((p) => p.includes(outerTmp.path))).toBe(true) }, }) }) - test("denies reading relative path that traverses outside project directory", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "deny", - }, - }), - ) - }, - }) + test("asks for external_directory permission when reading relative path outside project", async () => { + await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, fn: async () => { const read = await ReadTool.init() - await expect(read.execute({ filePath: "../../../etc/passwd" }, ctx)).rejects.toThrow( - "not in the current working directory", - ) + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + // This will fail because file doesn't exist, but we can check if permission was asked + await read.execute({ filePath: "../outside.txt" }, testCtx).catch(() => {}) + const extDirReq = requests.find((r) => r.permission === "external_directory") + expect(extDirReq).toBeDefined() }, }) }) - test("allows reading outside project directory when external_directory is allow", async () => { - await using outerTmp = await tmpdir({ - init: async (dir) => { - await Bun.write(path.join(dir, "external.txt"), "external content") - }, - }) + test("does not ask for external_directory permission when reading inside project", async () => { await using tmp = await tmpdir({ + git: true, init: async (dir) => { - await Bun.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - permission: { - external_directory: "allow", - }, - }), - ) + await Bun.write(path.join(dir, "internal.txt"), "internal content") }, }) await Instance.provide({ directory: tmp.path, fn: async () => { const read = await ReadTool.init() - const result = await read.execute({ filePath: path.join(outerTmp.path, "external.txt") }, ctx) - expect(result.output).toContain("external content") + const requests: Array> = [] + const testCtx = { + ...ctx, + ask: async (req: Omit) => { + requests.push(req) + }, + } + await read.execute({ filePath: path.join(tmp.path, "internal.txt") }, testCtx) + const extDirReq = requests.find((r) => r.permission === "external_directory") + expect(extDirReq).toBeUndefined() }, }) })