Skip to content

Commit f94ee5c

Browse files
committed
core: extract external directory validation to shared utility to reduce code duplication across tools
1 parent 76386f5 commit f94ee5c

10 files changed

Lines changed: 186 additions & 49 deletions

File tree

packages/opencode/src/tool/edit.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { FileTime } from "../file/time"
1515
import { Filesystem } from "../util/filesystem"
1616
import { Instance } from "../project/instance"
1717
import { Snapshot } from "@/snapshot"
18+
import { assertExternalDirectory } from "./external-directory"
1819

1920
const MAX_DIAGNOSTICS_PER_FILE = 20
2021

@@ -40,18 +41,7 @@ export const EditTool = Tool.define("edit", {
4041
}
4142

4243
const filePath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath)
43-
if (!Filesystem.contains(Instance.directory, filePath)) {
44-
const parentDir = path.dirname(filePath)
45-
await ctx.ask({
46-
permission: "external_directory",
47-
patterns: [parentDir, path.join(parentDir, "*")],
48-
always: [parentDir + "/*"],
49-
metadata: {
50-
filepath: filePath,
51-
parentDir,
52-
},
53-
})
54-
}
44+
await assertExternalDirectory(ctx, filePath)
5545

5646
let diff = ""
5747
let contentOld = ""
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import path from "path"
2+
import type { Tool } from "./tool"
3+
import { Filesystem } from "../util/filesystem"
4+
import { Instance } from "../project/instance"
5+
6+
type Kind = "file" | "directory"
7+
8+
type Options = {
9+
bypass?: boolean
10+
kind?: Kind
11+
}
12+
13+
export async function assertExternalDirectory(ctx: Tool.Context, target?: string, options?: Options) {
14+
if (!target) return
15+
16+
if (options?.bypass) return
17+
18+
if (Filesystem.contains(Instance.directory, target)) return
19+
20+
const kind = options?.kind ?? "file"
21+
const parentDir = kind === "directory" ? target : path.dirname(target)
22+
const glob = path.join(parentDir, "*")
23+
24+
await ctx.ask({
25+
permission: "external_directory",
26+
patterns: [glob],
27+
always: [glob],
28+
metadata: {
29+
filepath: target,
30+
parentDir,
31+
},
32+
})
33+
}

packages/opencode/src/tool/glob.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { Tool } from "./tool"
44
import DESCRIPTION from "./glob.txt"
55
import { Ripgrep } from "../file/ripgrep"
66
import { Instance } from "../project/instance"
7+
import { assertExternalDirectory } from "./external-directory"
78

89
export const GlobTool = Tool.define("glob", {
910
description: DESCRIPTION,
@@ -29,6 +30,7 @@ export const GlobTool = Tool.define("glob", {
2930

3031
let search = params.path ?? Instance.directory
3132
search = path.isAbsolute(search) ? search : path.resolve(Instance.directory, search)
33+
await assertExternalDirectory(ctx, search, { kind: "directory" })
3234

3335
const limit = 100
3436
const files = []

packages/opencode/src/tool/grep.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { Ripgrep } from "../file/ripgrep"
44

55
import DESCRIPTION from "./grep.txt"
66
import { Instance } from "../project/instance"
7+
import path from "path"
8+
import { assertExternalDirectory } from "./external-directory"
79

810
const MAX_LINE_LENGTH = 2000
911

@@ -30,7 +32,9 @@ export const GrepTool = Tool.define("grep", {
3032
},
3133
})
3234

33-
const searchPath = params.path || Instance.directory
35+
let searchPath = params.path ?? Instance.directory
36+
searchPath = path.isAbsolute(searchPath) ? searchPath : path.resolve(Instance.directory, searchPath)
37+
await assertExternalDirectory(ctx, searchPath, { kind: "directory" })
3438

3539
const rgPath = await Ripgrep.filepath()
3640
const args = ["-nH", "--hidden", "--follow", "--field-match-separator=|", "--regexp", params.pattern]

packages/opencode/src/tool/ls.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import * as path from "path"
44
import DESCRIPTION from "./ls.txt"
55
import { Instance } from "../project/instance"
66
import { Ripgrep } from "../file/ripgrep"
7+
import { assertExternalDirectory } from "./external-directory"
78

89
export const IGNORE_PATTERNS = [
910
"node_modules/",
@@ -42,6 +43,7 @@ export const ListTool = Tool.define("list", {
4243
}),
4344
async execute(params, ctx) {
4445
const searchPath = path.resolve(Instance.directory, params.path || ".")
46+
await assertExternalDirectory(ctx, searchPath, { kind: "directory" })
4547

4648
await ctx.ask({
4749
permission: "list",

packages/opencode/src/tool/lsp.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { LSP } from "../lsp"
55
import DESCRIPTION from "./lsp.txt"
66
import { Instance } from "../project/instance"
77
import { pathToFileURL } from "url"
8+
import { assertExternalDirectory } from "./external-directory"
89

910
const operations = [
1011
"goToDefinition",
@@ -27,14 +28,15 @@ export const LspTool = Tool.define("lsp", {
2728
character: z.number().int().min(1).describe("The character offset (1-based, as shown in editors)"),
2829
}),
2930
execute: async (args, ctx) => {
31+
const file = path.isAbsolute(args.filePath) ? args.filePath : path.join(Instance.directory, args.filePath)
32+
await assertExternalDirectory(ctx, file)
33+
3034
await ctx.ask({
3135
permission: "lsp",
3236
patterns: ["*"],
3337
always: ["*"],
3438
metadata: {},
3539
})
36-
37-
const file = path.isAbsolute(args.filePath) ? args.filePath : path.join(Instance.directory, args.filePath)
3840
const uri = pathToFileURL(file).href
3941
const position = {
4042
file,

packages/opencode/src/tool/patch.ts

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ import { Bus } from "../bus"
77
import { FileWatcher } from "../file/watcher"
88
import { Instance } from "../project/instance"
99
import { Patch } from "../patch"
10-
import { Filesystem } from "../util/filesystem"
1110
import { createTwoFilesPatch } from "diff"
11+
import { assertExternalDirectory } from "./external-directory"
1212

1313
const PatchParams = z.object({
1414
patchText: z.string().describe("The full patch text that describes all changes to be made"),
@@ -49,19 +49,7 @@ export const PatchTool = Tool.define("patch", {
4949

5050
for (const hunk of hunks) {
5151
const filePath = path.resolve(Instance.directory, hunk.path)
52-
53-
if (!Filesystem.contains(Instance.directory, filePath)) {
54-
const parentDir = path.dirname(filePath)
55-
await ctx.ask({
56-
permission: "external_directory",
57-
patterns: [parentDir, path.join(parentDir, "*")],
58-
always: [parentDir + "/*"],
59-
metadata: {
60-
filepath: filePath,
61-
parentDir,
62-
},
63-
})
64-
}
52+
await assertExternalDirectory(ctx, filePath)
6553

6654
switch (hunk.type) {
6755
case "add":
@@ -103,12 +91,15 @@ export const PatchTool = Tool.define("patch", {
10391

10492
const diff = createTwoFilesPatch(filePath, filePath, oldContent, newContent)
10593

94+
const movePath = hunk.move_path ? path.resolve(Instance.directory, hunk.move_path) : undefined
95+
await assertExternalDirectory(ctx, movePath)
96+
10697
fileChanges.push({
10798
filePath,
10899
oldContent,
109100
newContent,
110101
type: hunk.move_path ? "move" : "update",
111-
movePath: hunk.move_path ? path.resolve(Instance.directory, hunk.move_path) : undefined,
102+
movePath,
112103
})
113104

114105
totalDiff += diff + "\n"

packages/opencode/src/tool/read.ts

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ import { Tool } from "./tool"
55
import { LSP } from "../lsp"
66
import { FileTime } from "../file/time"
77
import DESCRIPTION from "./read.txt"
8-
import { Filesystem } from "../util/filesystem"
98
import { Instance } from "../project/instance"
109
import { Identifier } from "../id/id"
10+
import { assertExternalDirectory } from "./external-directory"
1111

1212
const DEFAULT_READ_LIMIT = 2000
1313
const MAX_LINE_LENGTH = 2000
@@ -27,18 +27,9 @@ export const ReadTool = Tool.define("read", {
2727
}
2828
const title = path.relative(Instance.worktree, filepath)
2929

30-
if (!ctx.extra?.["bypassCwdCheck"] && !Filesystem.contains(Instance.directory, filepath)) {
31-
const parentDir = path.dirname(filepath)
32-
await ctx.ask({
33-
permission: "external_directory",
34-
patterns: [parentDir],
35-
always: [parentDir + "/*"],
36-
metadata: {
37-
filepath,
38-
parentDir,
39-
},
40-
})
41-
}
30+
await assertExternalDirectory(ctx, filepath, {
31+
bypass: Boolean(ctx.extra?.["bypassCwdCheck"]),
32+
})
4233

4334
await ctx.ask({
4435
permission: "read",

packages/opencode/src/tool/write.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { FileTime } from "../file/time"
1010
import { Filesystem } from "../util/filesystem"
1111
import { Instance } from "../project/instance"
1212
import { trimDiff } from "./edit"
13+
import { assertExternalDirectory } from "./external-directory"
1314

1415
const MAX_DIAGNOSTICS_PER_FILE = 20
1516
const MAX_PROJECT_DIAGNOSTICS_FILES = 5
@@ -22,12 +23,7 @@ export const WriteTool = Tool.define("write", {
2223
}),
2324
async execute(params, ctx) {
2425
const filepath = path.isAbsolute(params.filePath) ? params.filePath : path.join(Instance.directory, params.filePath)
25-
/* TODO
26-
if (!Filesystem.contains(Instance.directory, filepath)) {
27-
const parentDir = path.dirname(filepath)
28-
...
29-
}
30-
*/
26+
await assertExternalDirectory(ctx, filepath)
3127

3228
const file = Bun.file(filepath)
3329
const exists = await file.exists()
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import { describe, expect, test } from "bun:test"
2+
import path from "path"
3+
import type { Tool } from "../../src/tool/tool"
4+
import { Instance } from "../../src/project/instance"
5+
import { assertExternalDirectory } from "../../src/tool/external-directory"
6+
import type { PermissionNext } from "../../src/permission/next"
7+
8+
const baseCtx: Omit<Tool.Context, "ask"> = {
9+
sessionID: "test",
10+
messageID: "",
11+
callID: "",
12+
agent: "build",
13+
abort: AbortSignal.any([]),
14+
metadata: () => {},
15+
}
16+
17+
describe("tool.assertExternalDirectory", () => {
18+
test("no-ops for empty target", async () => {
19+
const requests: Array<Omit<PermissionNext.Request, "id" | "sessionID" | "tool">> = []
20+
const ctx: Tool.Context = {
21+
...baseCtx,
22+
ask: async (req) => {
23+
requests.push(req)
24+
},
25+
}
26+
27+
await Instance.provide({
28+
directory: "/tmp",
29+
fn: async () => {
30+
await assertExternalDirectory(ctx)
31+
},
32+
})
33+
34+
expect(requests.length).toBe(0)
35+
})
36+
37+
test("no-ops for paths inside Instance.directory", async () => {
38+
const requests: Array<Omit<PermissionNext.Request, "id" | "sessionID" | "tool">> = []
39+
const ctx: Tool.Context = {
40+
...baseCtx,
41+
ask: async (req) => {
42+
requests.push(req)
43+
},
44+
}
45+
46+
await Instance.provide({
47+
directory: "/tmp/project",
48+
fn: async () => {
49+
await assertExternalDirectory(ctx, path.join("/tmp/project", "file.txt"))
50+
},
51+
})
52+
53+
expect(requests.length).toBe(0)
54+
})
55+
56+
test("asks with a single canonical glob", async () => {
57+
const requests: Array<Omit<PermissionNext.Request, "id" | "sessionID" | "tool">> = []
58+
const ctx: Tool.Context = {
59+
...baseCtx,
60+
ask: async (req) => {
61+
requests.push(req)
62+
},
63+
}
64+
65+
const directory = "/tmp/project"
66+
const target = "/tmp/outside/file.txt"
67+
const expected = path.join(path.dirname(target), "*")
68+
69+
await Instance.provide({
70+
directory,
71+
fn: async () => {
72+
await assertExternalDirectory(ctx, target)
73+
},
74+
})
75+
76+
const req = requests.find((r) => r.permission === "external_directory")
77+
expect(req).toBeDefined()
78+
expect(req!.patterns).toEqual([expected])
79+
expect(req!.always).toEqual([expected])
80+
})
81+
82+
test("uses target directory when kind=directory", async () => {
83+
const requests: Array<Omit<PermissionNext.Request, "id" | "sessionID" | "tool">> = []
84+
const ctx: Tool.Context = {
85+
...baseCtx,
86+
ask: async (req) => {
87+
requests.push(req)
88+
},
89+
}
90+
91+
const directory = "/tmp/project"
92+
const target = "/tmp/outside"
93+
const expected = path.join(target, "*")
94+
95+
await Instance.provide({
96+
directory,
97+
fn: async () => {
98+
await assertExternalDirectory(ctx, target, { kind: "directory" })
99+
},
100+
})
101+
102+
const req = requests.find((r) => r.permission === "external_directory")
103+
expect(req).toBeDefined()
104+
expect(req!.patterns).toEqual([expected])
105+
expect(req!.always).toEqual([expected])
106+
})
107+
108+
test("skips prompting when bypass=true", async () => {
109+
const requests: Array<Omit<PermissionNext.Request, "id" | "sessionID" | "tool">> = []
110+
const ctx: Tool.Context = {
111+
...baseCtx,
112+
ask: async (req) => {
113+
requests.push(req)
114+
},
115+
}
116+
117+
await Instance.provide({
118+
directory: "/tmp/project",
119+
fn: async () => {
120+
await assertExternalDirectory(ctx, "/tmp/outside/file.txt", { bypass: true })
121+
},
122+
})
123+
124+
expect(requests.length).toBe(0)
125+
})
126+
})

0 commit comments

Comments
 (0)