From e8fa6985bc63f1f177dc6e3f02a8d2cc7a8d6c88 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 14 Aug 2026 23:52:34 -0400 Subject: [PATCH] fix(core): unify patch path resolution (#42667) --- packages/core/src/location-mutation.ts | 8 +- packages/core/src/tool/plugin/patch.ts | 99 +++++---------- packages/core/test/location-mutation.test.ts | 14 ++ packages/core/test/tool-patch.test.ts | 127 +++++++++++++++++-- 4 files changed, 165 insertions(+), 83 deletions(-) diff --git a/packages/core/src/location-mutation.ts b/packages/core/src/location-mutation.ts index 546e405700..7dc1f1355a 100644 --- a/packages/core/src/location-mutation.ts +++ b/packages/core/src/location-mutation.ts @@ -76,9 +76,11 @@ const layer = Layer.effect( const type = input.kind === "directory" ? "Directory" - : (yield* fs - .stat(absolute) - .pipe(Effect.catchReason("PlatformError", "NotFound", () => Effect.succeed(undefined))))?.type + : input.kind === "file" + ? "File" + : (yield* fs + .stat(absolute) + .pipe(Effect.catchReason("PlatformError", "NotFound", () => Effect.succeed(undefined))))?.type const externalDirectory = type === "Directory" ? absolute : path.dirname(absolute) const externalResource = slash(path.join(externalDirectory, "*")) return { diff --git a/packages/core/src/tool/plugin/patch.ts b/packages/core/src/tool/plugin/patch.ts index 3dca1f003a..3ddca3573d 100644 --- a/packages/core/src/tool/plugin/patch.ts +++ b/packages/core/src/tool/plugin/patch.ts @@ -6,11 +6,11 @@ import { FileDiff } from "@opencode-ai/schema/file-diff" import { Effect, Result, Schema } from "effect" import path from "path" import { Bom } from "@opencode-ai/util/bom" -import { FSUtil } from "@opencode-ai/util/fs-util" import { Environment } from "../../environment/index.js" import { Formatter } from "../../formatter.js" import { FileMutation } from "../../file-mutation.js" import { Location } from "../../location.js" +import { LocationMutation } from "../../location-mutation.js" import { Patch } from "@opencode-ai/util/patch" import { Permission } from "../../permission.js" import DESCRIPTION from "../patch.txt" @@ -46,38 +46,30 @@ export const toModelOutput = (output: Output) => type Prepared = | (Extract & { - readonly target: Target + readonly target: LocationMutation.Target readonly content: string readonly before: string readonly after: string }) | (Extract & { - readonly target: Target + readonly target: LocationMutation.Target readonly before: string readonly after: string }) | (Extract & { - readonly target: Target + readonly target: LocationMutation.Target readonly content: string readonly before: string readonly after: string - readonly moveTarget?: Target + readonly moveTarget?: LocationMutation.Target }) -interface Target { - readonly absolute: string - readonly resource: string - readonly externalDirectory?: { - readonly directory: string - readonly resource: string - } -} - export const Plugin = { id: "opencode.tool.patch", effect: Effect.fn("PatchTool.Plugin")(function* (ctx: PluginContext) { const environment = yield* Environment.Service - const mutation = yield* FileMutation.Service + const mutation = yield* LocationMutation.Service + const fileMutation = yield* FileMutation.Service const formatter = yield* Formatter.Service const location = yield* Location.Service const permission = yield* Permission.Service @@ -119,26 +111,25 @@ export const Plugin = { return yield* new ToolFailure({ message: "patch rejected: empty patch" }) } const prepared: Prepared[] = [] - const targets: Target[] = [] const updates = new Map() + const resolveTarget = Effect.fnUntraced(function* (value: string) { + const target = yield* mutation.resolve({ path: value, kind: "file" }) + if (!target.externalDirectory) return target + yield* permission.assert({ + ...LocationMutation.externalDirectoryPermission(target.externalDirectory), + metadata: { + filepath: target.absolute, + parentDir: target.externalDirectory.directory, + }, + sessionID: context.sessionID, + agent: context.agent, + source, + }) + return target + }) for (const hunk of hunks) { yield* Effect.gen(function* () { - const target = resolveTarget(location, hunk.path) - targets.push(target) - if (target.externalDirectory) { - yield* permission.assert({ - action: "external_directory", - resources: [target.externalDirectory.resource], - save: [target.externalDirectory.resource], - metadata: { - filepath: target.absolute, - parentDir: target.externalDirectory.directory, - }, - sessionID: context.sessionID, - agent: context.agent, - source, - }) - } + const target = yield* resolveTarget(hunk.path) if (hunk.type === "add") { const content = hunk.contents.endsWith("\n") || hunk.contents === "" ? hunk.contents : `${hunk.contents}\n` @@ -182,22 +173,7 @@ export const Plugin = { try: () => Patch.derive(hunk.path, hunk.chunks, original), catch: (error) => new ToolFailure({ message: `patch verification failed: ${errorMessage(error)}` }), }) - const moveTarget = hunk.movePath ? resolveTarget(location, hunk.movePath) : undefined - if (moveTarget) targets.push(moveTarget) - if (moveTarget?.externalDirectory) { - yield* permission.assert({ - action: "external_directory", - resources: [moveTarget.externalDirectory.resource], - save: [moveTarget.externalDirectory.resource], - metadata: { - filepath: moveTarget.absolute, - parentDir: moveTarget.externalDirectory.directory, - }, - sessionID: context.sessionID, - agent: context.agent, - source, - }) - } + const moveTarget = hunk.movePath ? yield* resolveTarget(hunk.movePath) : undefined prepared.push({ ...hunk, target, @@ -217,6 +193,10 @@ export const Plugin = { } const patchFiles = prepared.map((change) => patchFile(change)) + const targets = prepared.flatMap((change) => [ + change.target, + ...(change.type === "update" && change.moveTarget ? [change.moveTarget] : []), + ]) yield* permission.assert({ action: "edit", resources: [...new Set(targets.map((target) => target.resource))], @@ -313,7 +293,7 @@ export const Plugin = { }) return { applied, files } }).pipe( - mutation.withLock(lockTargets), + fileMutation.withLock(lockTargets), Effect.map((output) => ({ output, content: toModelOutput(output), @@ -394,24 +374,3 @@ function trimDiff(diff: string) { }) .join("\n") } - -function resolveTarget(location: Location.Interface, value: string): Target { - const absolute = - process.platform === "win32" - ? FSUtil.normalizePath(path.resolve(location.directory, value)) - : path.resolve(location.directory, value) - const projectRoot = path.parse(location.project.directory).root - const external = - !FSUtil.contains(location.directory, absolute) && - (location.project.directory === projectRoot || !FSUtil.contains(location.project.directory, absolute)) - const directory = path.dirname(absolute) - const resource = - process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(directory, "*")) - : path.join(directory, "*").replaceAll("\\", "/") - return { - absolute, - resource: path.relative(location.project.directory, absolute).replaceAll("\\", "/") || ".", - externalDirectory: external ? { directory, resource } : undefined, - } -} diff --git a/packages/core/test/location-mutation.test.ts b/packages/core/test/location-mutation.test.ts index 01a4604ba9..faee4c479d 100644 --- a/packages/core/test/location-mutation.test.ts +++ b/packages/core/test/location-mutation.test.ts @@ -160,6 +160,20 @@ describe("LocationMutation", () => { ), ) + it.live("uses an explicit file kind without treating an existing directory as the target boundary", () => + withTmp((directory) => + withTmp((outside) => + Effect.gen(function* () { + const target = yield* (yield* LocationMutation.Service).resolve({ path: outside, kind: "file" }) + expect(target.externalDirectory).toMatchObject({ + directory: path.dirname(outside), + resource: path.join(path.dirname(outside), "*").replaceAll("\\", "/"), + }) + }).pipe(provide(directory)), + ), + ), + ) + it.live("authorizes prospective external descendants at their lexical parent", () => withTmp((directory) => withTmp((outside) => diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index 959ddd7a12..c4f8e672ac 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/packages/core/test/tool-patch.test.ts @@ -5,10 +5,10 @@ import { Effect, Exit, Layer, Schema } from "effect" import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder" import { LayerNode } from "@opencode-ai/util/effect/layer-node" import { Environment } from "@opencode-ai/core/environment/index" -import { FSUtil } from "@opencode-ai/util/fs-util" import { Formatter } from "@opencode-ai/core/formatter" import { FileMutation } from "@opencode-ai/core/file-mutation" import { Location } from "@opencode-ai/core/location" +import { LocationMutation } from "@opencode-ai/core/location-mutation" import { Permission } from "@opencode-ai/core/permission" import { AbsolutePath } from "@opencode-ai/core/schema" import { Session } from "@opencode-ai/core/session" @@ -25,7 +25,15 @@ import { toolIdentity, executeTool, registerToolPlugin, toolDefinitions } from " const patchToolNode = makeLocationNode({ name: "test/patch-tool-plugin", layer: Layer.effectDiscard(registerToolPlugin(PatchTool.Plugin)), - deps: [Tool.node, FileMutation.node, Environment.node, Formatter.node, Location.node, Permission.node], + deps: [ + Tool.node, + LocationMutation.node, + FileMutation.node, + Environment.node, + Formatter.node, + Location.node, + Permission.node, + ], }) const sessionID = Session.ID.make("ses_patch_tool_test") @@ -91,7 +99,7 @@ const withTool = ( return yield* body(yield* Tool.Service) }).pipe( Effect.provide( - AppNodeBuilder.build(LayerNode.group([Tool.node, FileMutation.node, patchToolNode]), [ + AppNodeBuilder.build(LayerNode.group([Tool.node, LocationMutation.node, FileMutation.node, patchToolNode]), [ [ Environment.node, transformEnvironmentFiles(activeLocation, (files) => ({ @@ -485,6 +493,42 @@ describe("PatchTool", () => { ), ) + it.live("uses Location-relative resources for move targets in a nested Location", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => { + reset() + const active = path.join(tmp.path, "nested", "location") + const source = path.join(active, "old.txt") + return Effect.promise(() => + fs.mkdir(active, { recursive: true }).then(() => fs.writeFile(source, "before\n")), + ).pipe( + Effect.andThen( + withTool( + active, + (registry) => + Effect.gen(function* () { + const settled = yield* executeTool( + registry, + call( + "*** Begin Patch\n*** Update File: old.txt\n*** Move to: moved.txt\n@@\n-before\n+after\n*** End Patch", + ), + ) + expect(settled).toMatchObject({ + status: "completed", + output: { applied: [{ resource: "moved.txt" }] }, + }) + expect(assertions).toMatchObject([{ action: "edit", resources: ["old.txt", "moved.txt"] }]) + }), + tmp.path, + ), + ), + ) + }, + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + it.live("inserts lines with an insert-only hunk", () => withTempTool((directory, registry) => Effect.gen(function* () { @@ -781,8 +825,15 @@ describe("PatchTool", () => { Effect.promise(() => Promise.all([tmpdir(), tmpdir()])), ([active, outside]) => { reset() - const target = path.join(outside.path, "external.txt") - return Effect.promise(() => fs.writeFile(target, "before\n")).pipe( + const repository = path.join(outside.path, "repository") + const directory = path.join(repository, "nested") + const target = path.join(directory, "external.txt") + return Effect.promise(() => + Promise.all([ + fs.mkdir(path.join(repository, ".git"), { recursive: true }), + fs.mkdir(directory, { recursive: true }).then(() => fs.writeFile(target, "before\n")), + ]), + ).pipe( Effect.andThen( withTool(active.path, (registry) => Effect.gen(function* () { @@ -793,6 +844,15 @@ describe("PatchTool", () => { ), ).toMatchObject({ status: "completed" }) expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"]) + expect(assertions[0]).toMatchObject({ + resources: [path.join(directory, "*").replaceAll("\\", "/")], + save: [path.join(repository, "*").replaceAll("\\", "/")], + metadata: { + filepath: target, + parentDir: directory, + }, + }) + expect(assertions[1]?.resources).toEqual([target.replaceAll("\\", "/")]) expect(readsBeforeEditApproval).toBe(1) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after\n") }), @@ -860,7 +920,7 @@ describe("PatchTool", () => { ), ) - it.live("treats a sibling path inside the project worktree as internal", () => + it.live("treats a sibling path inside the project worktree as external to the Location", () => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), (tmp) => { @@ -879,7 +939,9 @@ describe("PatchTool", () => { call("*** Begin Patch\n*** Update File: ../sibling.txt\n@@\n-before\n+after\n*** End Patch"), ), ).toMatchObject({ status: "completed" }) - expect(assertions.map((input) => input.action)).toEqual(["edit"]) + expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"]) + expect(assertions[0]?.resources).toEqual([path.join(tmp.path, "*").replaceAll("\\", "/")]) + expect(assertions[1]?.resources).toEqual([target.replaceAll("\\", "/")]) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after\n") }), tmp.path, @@ -956,6 +1018,53 @@ describe("PatchTool", () => { ), ) + it.live("uses canonical external permissions and resources for a move destination", () => + Effect.acquireUseRelease( + Effect.promise(() => Promise.all([tmpdir(), tmpdir()])), + ([active, outside]) => { + reset() + const source = path.join(active.path, "source.txt") + const destination = path.join(outside.path, "moved.txt") + return Effect.promise(() => fs.writeFile(source, "before\n")).pipe( + Effect.andThen( + withTool(active.path, (registry) => + Effect.gen(function* () { + const settled = yield* executeTool( + registry, + call( + `*** Begin Patch\n*** Update File: source.txt\n*** Move to: ${destination}\n@@\n-before\n+after\n*** End Patch`, + ), + ) + expect(settled).toMatchObject({ + status: "completed", + output: { applied: [{ resource: destination.replaceAll("\\", "/") }] }, + }) + expect(assertions).toMatchObject([ + { + action: "external_directory", + resources: [path.join(outside.path, "*").replaceAll("\\", "/")], + save: [path.join(outside.path, "*").replaceAll("\\", "/")], + metadata: { filepath: destination, parentDir: outside.path }, + }, + { + action: "edit", + resources: ["source.txt", destination.replaceAll("\\", "/")], + }, + ]) + expect(yield* exists(source)).toBe(false) + expect(yield* Effect.promise(() => fs.readFile(destination, "utf8"))).toBe("after\n") + }), + ), + ), + ) + }, + ([active, outside]) => + Effect.promise(() => + Promise.all([active[Symbol.asyncDispose](), outside[Symbol.asyncDispose]()]).then(() => undefined), + ), + ), + ) + it.live("approves each external file under the same parent", () => Effect.acquireUseRelease( Effect.promise(() => Promise.all([tmpdir(), tmpdir()])), @@ -983,9 +1092,7 @@ describe("PatchTool", () => { "edit", ]) expect(assertions[0]?.resources).toEqual([ - process.platform === "win32" - ? FSUtil.normalizePathPattern(path.join(outside.path, "*")) - : path.join(yield* Effect.promise(() => fs.realpath(outside.path)), "*").replaceAll("\\", "/"), + path.join(yield* Effect.promise(() => fs.realpath(outside.path)), "*").replaceAll("\\", "/"), ]) expect(assertions[1]?.resources).toEqual(assertions[0]?.resources) }),