Sfoglia il codice sorgente

fix(core): authorize mutations before locking

Kit Langton 2 giorni fa
parent
commit
f5291d5ff5

+ 6 - 65
packages/cli/src/acp/permission.ts

@@ -1,7 +1,5 @@
-import type { AgentSideConnection, PermissionOption, ToolCallContent, ToolCallLocation } from "@agentclientprotocol/sdk"
+import type { AgentSideConnection, PermissionOption, ToolCallLocation } from "@agentclientprotocol/sdk"
 import type { EventSubscribeOutput, OpenCodeClient } from "@opencode-ai/client/promise"
-import { Patch } from "@opencode-ai/util/patch"
-import { Result } from "effect"
 import { isAbsolute, resolve } from "node:path"
 import { pendingToolCall, stringValue, toLocations, toToolKind, type ToolInput } from "./tool"
 
@@ -28,9 +26,8 @@ export async function replyPermission(input: {
 }) {
   const toolName = input.tool?.name ?? input.event.data.action
   const toolInput = { ...input.event.data.metadata, ...input.tool?.input }
-  const previews = await permissionPreviews(toolName, toolInput, input.cwd)
   const toolCallID = input.event.data.source?.id ?? input.event.data.id
-  const title = permissionTitle(toolName, toolInput, previews)
+  const title = permissionTitle(toolName, toolInput, input.event.data.resources)
   const result = await input.connection
     .requestPermission({
       sessionId: input.clientSessionID ?? input.sessionID,
@@ -44,8 +41,7 @@ export async function replyPermission(input: {
           },
           cwd: input.cwd,
         }),
-        locations: permissionLocations(toolName, toolInput, input.event.data.resources, input.cwd, previews),
-        ...(previews.length > 0 ? { content: previews } : {}),
+        locations: permissionLocations(toolName, toolInput, input.event.data.resources, input.cwd),
       },
       options,
     })
@@ -94,54 +90,8 @@ export async function syncEditedFiles(input: {
   )
 }
 
-async function permissionPreviews(toolName: string, input: ToolInput, cwd: string): Promise<ToolCallContent[]> {
-  const tool = toolName.toLocaleLowerCase()
-  if (tool === "patch" || tool === "apply_patch") return patchPreviews(input, cwd)
-  const path = filePath(input)
-  if (!path) return []
-  const oldText = await readText(path, cwd)
-  if (tool === "write") {
-    const content = stringValue(input.content)
-    return content === undefined ? [] : [{ type: "diff", path, oldText, newText: content }]
-  }
-  if (tool !== "edit") return []
-  const oldString = stringValue(input.oldString)
-  const newString = stringValue(input.newString)
-  if (oldString === undefined || newString === undefined) return []
-  const newText =
-    input.replaceAll === true ? oldText.replaceAll(oldString, newString) : oldText.replace(oldString, newString)
-  return [{ type: "diff", path, oldText, newText }]
-}
-
-async function patchPreviews(input: ToolInput, cwd: string): Promise<ToolCallContent[]> {
-  const patchText = stringValue(input.patchText)
-  if (!patchText) return []
-  try {
-    const parsed = Patch.parse(patchText)
-    if (Result.isFailure(parsed)) return []
-    return await Promise.all(
-      parsed.success.map(async (hunk): Promise<ToolCallContent> => {
-        const oldText = hunk.type === "add" ? "" : await readText(hunk.path, cwd)
-        if (hunk.type === "add") {
-          const newText = hunk.contents.endsWith("\n") || hunk.contents === "" ? hunk.contents : `${hunk.contents}\n`
-          return { type: "diff", path: hunk.path, oldText, newText }
-        }
-        if (hunk.type === "delete") return { type: "diff", path: hunk.path, oldText, newText: "" }
-        return {
-          type: "diff",
-          path: hunk.movePath ?? hunk.path,
-          oldText,
-          newText: Patch.derive(hunk.path, hunk.chunks, oldText).content,
-        }
-      }),
-    )
-  } catch {
-    return []
-  }
-}
-
-function permissionTitle(toolName: string, input: ToolInput, previews: ReadonlyArray<ToolCallContent>) {
-  if (previews.length > 1) return `${previews.length} files`
+function permissionTitle(toolName: string, input: ToolInput, resources: ReadonlyArray<string>) {
+  if (toToolKind(toolName) === "edit" && resources.length > 1) return `${resources.length} files`
   switch (toolName.toLocaleLowerCase()) {
     case "external_directory":
       return stringValue(input.description) ?? stringValue(input.command) ?? stringValue(input.parentDir)
@@ -157,7 +107,7 @@ function permissionTitle(toolName: string, input: ToolInput, previews: ReadonlyA
     case "write":
     case "patch":
     case "apply_patch":
-      return filePath(input) ?? (previews[0]?.type === "diff" ? previews[0].path : undefined)
+      return filePath(input)
     default:
       return undefined
   }
@@ -168,21 +118,12 @@ function permissionLocations(
   input: ToolInput,
   resources: ReadonlyArray<string>,
   cwd: string,
-  previews: ReadonlyArray<ToolCallContent>,
 ): ToolCallLocation[] {
-  const paths = previews.flatMap((preview) => (preview.type === "diff" ? [preview.path] : []))
-  if (paths.length > 0) return [...new Set(paths)].map((path) => ({ path }))
   const locations = toLocations(toolName, input, cwd)
   if (locations.length > 0) return locations
   return resources.filter((resource) => resource !== "*").map((path) => ({ path }))
 }
 
-function readText(path: string, cwd: string) {
-  return Bun.file(resolvePath(path, cwd))
-    .text()
-    .catch(() => "")
-}
-
 function filePath(input: ToolInput) {
   return stringValue(input.path) ?? stringValue(input.filePath) ?? stringValue(input.filepath)
 }

+ 8 - 8
packages/cli/test/acp/permission-behavior.test.ts

@@ -211,7 +211,7 @@ describe("acp permission behavior", () => {
     }
   })
 
-  test("previews edits during approval and syncs the completed file", async () => {
+  test("authorizes edit resources and syncs the completed file", async () => {
     const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "opencode-acp-permission-"))
     const file = path.join(cwd, "file.ts")
     await fs.writeFile(file, "before")
@@ -240,6 +240,7 @@ describe("acp permission behavior", () => {
         send(
           permissionAsked("ses_edit", "perm_edit", {
             action: "edit",
+            resources: ["file.ts"],
             source: { type: "tool", messageID: "msg_edit", id: "call_edit" },
           }),
         )
@@ -278,8 +279,8 @@ describe("acp permission behavior", () => {
         title: "file.ts",
         kind: "edit",
         locations: [{ path: "file.ts" }],
-        content: [{ type: "diff", path: "file.ts", oldText: "before", newText: "after" }],
       })
+      expect(permissionRequests[0]?.toolCall.content).toBeUndefined()
       expect(writes).toEqual([{ sessionId: "ses_edit", path: file, content: "after" }])
     } finally {
       await fixture.stop()
@@ -287,7 +288,7 @@ describe("acp permission behavior", () => {
     }
   })
 
-  test("previews and syncs each file in a patch", async () => {
+  test("authorizes and syncs each file in a patch", async () => {
     const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "opencode-acp-patch-permission-"))
     await Promise.all([
       fs.writeFile(path.join(cwd, "first.ts"), "one\n"),
@@ -330,6 +331,7 @@ describe("acp permission behavior", () => {
         send(
           permissionAsked("ses_patch", "perm_patch", {
             action: "edit",
+            resources: ["first.ts", "second.ts"],
             source: { type: "tool", messageID: "msg_patch", id: "call_patch" },
           }),
         )
@@ -371,11 +373,8 @@ describe("acp permission behavior", () => {
         title: "2 files",
         kind: "edit",
         locations: [{ path: "first.ts" }, { path: "second.ts" }],
-        content: [
-          { type: "diff", path: "first.ts", oldText: "one\n", newText: "two\n" },
-          { type: "diff", path: "second.ts", oldText: "alpha\n", newText: "beta\n" },
-        ],
       })
+      expect(permissionRequests[0]?.toolCall.content).toBeUndefined()
       expect(writes.toSorted((a, b) => a.path.localeCompare(b.path))).toEqual([
         { sessionId: "ses_patch", path: path.join(cwd, "first.ts"), content: "two\n" },
         { sessionId: "ses_patch", path: path.join(cwd, "second.ts"), content: "beta\n" },
@@ -556,6 +555,7 @@ function permissionAsked(
   id: string,
   input: {
     readonly action?: string
+    readonly resources?: ReadonlyArray<string>
     readonly metadata?: Record<string, unknown>
     readonly source?: { readonly type: "tool"; readonly messageID: string; readonly id: string }
   } = {},
@@ -564,7 +564,7 @@ function permissionAsked(
     id,
     sessionID,
     action: input.action ?? "shell",
-    resources: ["*"],
+    resources: [...(input.resources ?? ["*"])],
     metadata: input.metadata ?? { command: "printf hello" },
     ...(input.source ? { source: input.source } : {}),
   })

+ 55 - 61
packages/core/src/tool/plugin/edit.ts

@@ -11,11 +11,9 @@ import { ToolFailure } from "@opencode-ai/ai"
 import { FileDiff } from "@opencode-ai/schema/file-diff"
 import { Bom } from "@opencode-ai/util/bom"
 import { Effect, Schema } from "effect"
-import path from "path"
 import { Environment } from "../../environment"
 import { FileMutation } from "../../file-mutation"
 import { Formatter } from "../../formatter"
-import { Location } from "../../location"
 import { LocationMutation } from "../../location-mutation"
 import { Permission } from "../../permission"
 import { fileDiff } from "./file-diff"
@@ -87,7 +85,7 @@ const findLineOccurrences = (content: string, search: string) => {
     if (
       !actual.every(
         (item, lineIndex) =>
-          normalizeForMatch(item.text.trimEnd()) === normalizeForMatch(expected[lineIndex]!.trimEnd()),
+          normalizeForMatch(item.text.trimEnd()) === normalizeForMatch(expected[lineIndex].trimEnd()),
       )
     )
       return []
@@ -114,7 +112,6 @@ export const Plugin = {
     const fileMutation = yield* FileMutation.Service
     const environment = yield* Environment.Service
     const formatter = yield* Formatter.Service
-    const location = yield* Location.Service
     const permission = yield* Permission.Service
 
     yield* ctx.tool
@@ -154,72 +151,69 @@ export const Plugin = {
                   source: permissionSource,
                 })
               }
-
-              const original = yield* FileMutation.readText(environment.files, target.absolute).pipe(
-                Effect.catchTag("Environment.NotFound", () =>
-                  Effect.fail(new ToolFailure({ message: `File not found: ${input.path}` })),
-                ),
-                Effect.catchTag("Environment.WrongKind", (error) =>
-                  error.actual === "directory"
-                    ? Effect.fail(new ToolFailure({ message: `Path is a directory, not a file: ${input.path}` }))
-                    : Effect.fail(new ToolFailure({ message: `Unable to edit ${input.path}`, error })),
-                ),
-              )
-              const source = original.text
-              const ending = source.includes(crlf) ? crlf : "\n"
-              const oldString = input.oldString.replaceAll(crlf, "\n").replaceAll("\n", ending)
-              const newString = input.newString.replaceAll(crlf, "\n").replaceAll("\n", ending)
-              const exact = findOccurrences(source, oldString)
-              // These one-to-one mappings preserve offsets into the original source.
-              const unicode =
-                exact.length > 0 ? [] : findOccurrences(normalizeForMatch(source), normalizeForMatch(oldString))
-              const trailing = exact.length > 0 || unicode.length > 0 ? [] : findLineOccurrences(source, oldString)
-              const matches = exact.length > 0 ? exact : unicode.length > 0 ? unicode : trailing
-              const replacements = matches.length
-              const replaced = (input.replaceAll === true ? matches : matches.slice(0, 1))
-                .toReversed()
-                .reduce(
-                  (content, match) => `${content.slice(0, match.start)}${newString}${content.slice(match.end)}`,
-                  source,
-                )
-              const preview =
-                replacements > 0 && (replacements === 1 || input.replaceAll === true)
-                  ? fileDiff(target.resource, source, replaced)
-                  : undefined
               yield* permission.assert({
                 action: "edit",
                 resources: [target.resource],
                 save: ["*"],
-                metadata: preview ? { files: [preview] } : undefined,
                 sessionID: context.sessionID,
                 agent: context.agent,
                 source: permissionSource,
               })
-              if (replacements === 0) {
-                return yield* new ToolFailure({
-                  message: `Could not find oldString in ${input.path}. It must match exactly, including whitespace and indentation.`,
-                })
-              }
-              if (replacements > 1 && input.replaceAll !== true) {
-                return yield* new ToolFailure({
-                  message: `Found ${replacements} matches for oldString, but expected exactly one. Add more surrounding context to make oldString unique, or set replaceAll to true to replace every occurrence.`,
-                })
-              }
-              const replacementBom = replaced.startsWith("\uFEFF")
-              const result = yield* fileMutation.write({
-                target,
-                content: Bom.join(replaced, original.bom || replacementBom),
-              })
-              const bom = original.bom || replacementBom
-              const formatted = (yield* formatter.file(target.absolute))
-                ? yield* FileMutation.syncTextBom(environment.files, target.absolute, bom)
-                : (yield* FileMutation.readText(environment.files, target.absolute)).text
-              return {
-                files: [fileDiff(result.resource, source, formatted)],
-                replacements,
-              } satisfies Output
+              return yield* fileMutation.withLock([target.absolute])(
+                Effect.gen(function* () {
+                  const original = yield* FileMutation.readText(environment.files, target.absolute).pipe(
+                    Effect.catchTag("Environment.NotFound", () =>
+                      Effect.fail(new ToolFailure({ message: `File not found: ${input.path}` })),
+                    ),
+                    Effect.catchTag("Environment.WrongKind", (error) =>
+                      error.actual === "directory"
+                        ? Effect.fail(new ToolFailure({ message: `Path is a directory, not a file: ${input.path}` }))
+                        : Effect.fail(new ToolFailure({ message: `Unable to edit ${input.path}`, error })),
+                    ),
+                  )
+                  const source = original.text
+                  const ending = source.includes(crlf) ? crlf : "\n"
+                  const oldString = input.oldString.replaceAll(crlf, "\n").replaceAll("\n", ending)
+                  const newString = input.newString.replaceAll(crlf, "\n").replaceAll("\n", ending)
+                  const exact = findOccurrences(source, oldString)
+                  // These one-to-one mappings preserve offsets into the original source.
+                  const unicode =
+                    exact.length > 0 ? [] : findOccurrences(normalizeForMatch(source), normalizeForMatch(oldString))
+                  const trailing = exact.length > 0 || unicode.length > 0 ? [] : findLineOccurrences(source, oldString)
+                  const matches = exact.length > 0 ? exact : unicode.length > 0 ? unicode : trailing
+                  const replacements = matches.length
+                  const replaced = (input.replaceAll === true ? matches : matches.slice(0, 1))
+                    .toReversed()
+                    .reduce(
+                      (content, match) => `${content.slice(0, match.start)}${newString}${content.slice(match.end)}`,
+                      source,
+                    )
+                  if (replacements === 0) {
+                    return yield* new ToolFailure({
+                      message: `Could not find oldString in ${input.path}. It must match exactly, including whitespace and indentation.`,
+                    })
+                  }
+                  if (replacements > 1 && input.replaceAll !== true) {
+                    return yield* new ToolFailure({
+                      message: `Found ${replacements} matches for oldString, but expected exactly one. Add more surrounding context to make oldString unique, or set replaceAll to true to replace every occurrence.`,
+                    })
+                  }
+                  const replacementBom = replaced.startsWith("\uFEFF")
+                  const result = yield* fileMutation.write({
+                    target,
+                    content: Bom.join(replaced, original.bom || replacementBom),
+                  })
+                  const bom = original.bom || replacementBom
+                  const formatted = (yield* formatter.file(target.absolute))
+                    ? yield* FileMutation.syncTextBom(environment.files, target.absolute, bom)
+                    : (yield* FileMutation.readText(environment.files, target.absolute)).text
+                  return {
+                    files: [fileDiff(result.resource, source, formatted)],
+                    replacements,
+                  } satisfies Output
+                }),
+              )
             }).pipe(
-              fileMutation.withLock([path.resolve(location.directory, input.path)]),
               Effect.map((output) => ({
                 output,
                 content: `Edited ${output.files[0]?.file} (${output.replacements} replacement${output.replacements === 1 ? "" : "s"})`,

+ 174 - 186
packages/core/src/tool/plugin/patch.ts

@@ -3,7 +3,7 @@ export * as PatchTool from "./patch"
 import type { Context as PluginContext } from "@opencode-ai/plugin/effect/plugin"
 import { ToolFailure } from "@opencode-ai/ai"
 import { FileDiff } from "@opencode-ai/schema/file-diff"
-import { Effect, Result, Schema } from "effect"
+import { Effect, Schema } from "effect"
 import path from "path"
 import { Bom } from "@opencode-ai/util/bom"
 import { FSUtil } from "@opencode-ai/util/fs-util"
@@ -93,12 +93,6 @@ export const Plugin = {
           execute: (input, context) => {
             const applied: Array<typeof Applied.Type> = []
             const parsed = Patch.parse(input.patchText)
-            const lockTargets = Result.isSuccess(parsed)
-              ? parsed.success.flatMap((hunk) => [
-                  path.resolve(location.directory, hunk.path),
-                  ...(hunk.type === "update" && hunk.movePath ? [path.resolve(location.directory, hunk.movePath)] : []),
-                ])
-              : []
             const fail = (operation: string, error: unknown) => {
               const completed = applied.map((item) => item.resource).join(", ")
               return new ToolFailure({
@@ -118,202 +112,196 @@ export const Plugin = {
               if (hunks.length === 0) {
                 return yield* new ToolFailure({ message: "patch rejected: empty patch" })
               }
-              const prepared: Prepared[] = []
-              const targets: Target[] = []
-              const updates = new Map<string, string>()
-              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,
-                    })
-                  }
-                  if (hunk.type === "add") {
-                    const content =
-                      hunk.contents.endsWith("\n") || hunk.contents === "" ? hunk.contents : `${hunk.contents}\n`
-                    prepared.push({
-                      ...hunk,
-                      target,
-                      content,
-                      before: "",
-                      after: Bom.split(content).text,
-                    })
-                    return
-                  }
-                  if (hunk.type === "delete") {
-                    const content = yield* FileMutation.readText(environment.files, target.absolute).pipe(
-                      Effect.mapError(
-                        (error) =>
-                          new ToolFailure({
-                            message: `patch verification failed: Failed to delete ${target.resource}: ${errorMessage(error)}`,
-                          }),
-                      ),
-                    )
-                    prepared.push({ ...hunk, target, before: content.text, after: "" })
-                    return
-                  }
-                  const previous = updates.get(target.absolute)
-                  const original =
-                    previous ??
-                    (yield* Effect.gen(function* () {
-                      const content = yield* FileMutation.readText(environment.files, target.absolute).pipe(
-                        Effect.mapError(
-                          (error) =>
-                            new ToolFailure({
-                              message: `patch verification failed: Failed to read file to update ${target.absolute}: ${errorMessage(error)}`,
-                            }),
-                        ),
-                      )
-                      return Bom.join(content.text, content.bom)
-                    }))
-                  const before = Bom.split(original).text
-                  const update = yield* Effect.try({
-                    try: () => Patch.derive(hunk.path, hunk.chunks, original),
-                    catch: (error) => new ToolFailure({ message: `patch verification failed: ${errorMessage(error)}` }),
+              const plans = hunks.map((hunk) => ({
+                hunk,
+                target: resolveTarget(location, hunk.path),
+                moveTarget:
+                  hunk.type === "update" && hunk.movePath ? resolveTarget(location, hunk.movePath) : undefined,
+              }))
+              const targets = plans.flatMap((plan) => [plan.target, ...(plan.moveTarget ? [plan.moveTarget] : [])])
+              for (const target of targets) {
+                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 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,
-                    })
-                  }
-                  prepared.push({
-                    ...hunk,
-                    target,
-                    content: Patch.joinBom(update.content, update.bom),
-                    before,
-                    after: update.content,
-                    moveTarget,
-                  })
-                  if (!moveTarget) updates.set(target.absolute, Patch.joinBom(update.content, update.bom))
-                }).pipe(
-                  Effect.mapError((error) =>
-                    error instanceof ToolFailure
-                      ? error
-                      : new ToolFailure({ message: `Unable to prepare patch at ${hunk.path}`, error }),
-                  ),
-                )
+                }
               }
-
-              const patchFiles = prepared.map((change) => patchFile(change))
               yield* permission.assert({
                 action: "edit",
                 resources: [...new Set(targets.map((target) => target.resource))],
                 save: ["*"],
-                metadata: {
-                  filepath: targets.map((target) => target.resource).join(", "),
-                  diff: patchFiles.map((file) => `${file.patch}\n`).join(""),
-                  files: patchFiles,
-                },
                 sessionID: context.sessionID,
                 agent: context.agent,
                 source,
               })
 
-              yield* Effect.forEach(
-                prepared,
-                (change) =>
-                  Effect.gen(function* () {
-                    if (change.type === "add") {
-                      yield* environment.files
-                        .write(change.target.absolute, new TextEncoder().encode(change.content))
-                        .pipe(Effect.mapError((error) => fail(`Failed to write ${change.target.resource}`, error)))
-                      applied.push({
-                        type: change.type,
-                        resource: change.target.resource,
-                        target: change.target.absolute,
-                      })
-                      return
-                    }
-                    if (change.type === "delete") {
-                      yield* environment.files
-                        .remove(change.target.absolute)
-                        .pipe(Effect.mapError((error) => fail(`Failed to delete ${change.target.resource}`, error)))
-                      applied.push({
-                        type: change.type,
-                        resource: change.target.resource,
-                        target: change.target.absolute,
-                      })
-                      return
-                    }
-                    if (change.moveTarget) {
-                      const moveTarget = change.moveTarget
-                      yield* environment.files
-                        .write(moveTarget.absolute, new TextEncoder().encode(change.content))
-                        .pipe(Effect.mapError((error) => fail(`Failed to write ${moveTarget.resource}`, error)))
-                      yield* environment.files
-                        .remove(change.target.absolute)
-                        .pipe(
-                          Effect.mapError((error) =>
-                            fail(`Wrote ${moveTarget.resource} but failed to remove ${change.target.resource}`, error),
+              return yield* mutation.withLock(targets.map((target) => target.absolute))(
+                Effect.gen(function* () {
+                  const prepared: Prepared[] = []
+                  const updates = new Map<string, string>()
+                  for (const plan of plans) {
+                    const hunk = plan.hunk
+                    const target = plan.target
+                    yield* Effect.gen(function* () {
+                      if (hunk.type === "add") {
+                        const content =
+                          hunk.contents.endsWith("\n") || hunk.contents === "" ? hunk.contents : `${hunk.contents}\n`
+                        prepared.push({
+                          ...hunk,
+                          target,
+                          content,
+                          before: "",
+                          after: Bom.split(content).text,
+                        })
+                        return
+                      }
+                      if (hunk.type === "delete") {
+                        const content = yield* FileMutation.readText(environment.files, target.absolute).pipe(
+                          Effect.mapError(
+                            (error) =>
+                              new ToolFailure({
+                                message: `patch verification failed: Failed to delete ${target.resource}: ${errorMessage(error)}`,
+                              }),
                           ),
                         )
-                      applied.push({
-                        type: change.type,
-                        resource: change.moveTarget.resource,
-                        target: change.moveTarget.absolute,
-                      })
-                      return
-                    }
-                    yield* environment.files
-                      .write(change.target.absolute, new TextEncoder().encode(change.content))
-                      .pipe(Effect.mapError((error) => fail(`Failed to write ${change.target.resource}`, error)))
-                    applied.push({
-                      type: change.type,
-                      resource: change.target.resource,
-                      target: change.target.absolute,
-                    })
-                  }),
-                { discard: true },
-              )
-              const formatted = new Map<string, string>()
-              yield* Effect.forEach(
-                [...new Set(applied.filter((item) => item.type !== "delete").map((item) => item.target))],
-                (target) =>
-                  Effect.gen(function* () {
-                    const current = yield* FileMutation.readText(environment.files, target).pipe(
-                      Effect.mapError((error) => fail(`Failed to read ${target}`, error)),
-                    )
-                    formatted.set(
-                      target,
-                      (yield* formatter.file(target))
-                        ? yield* FileMutation.syncTextBom(environment.files, target, current.bom).pipe(
-                            Effect.mapError((error) => fail(`Failed to sync ${target}`, error)),
+                        prepared.push({ ...hunk, target, before: content.text, after: "" })
+                        return
+                      }
+                      const previous = updates.get(target.absolute)
+                      const original =
+                        previous ??
+                        (yield* Effect.gen(function* () {
+                          const content = yield* FileMutation.readText(environment.files, target.absolute).pipe(
+                            Effect.mapError(
+                              (error) =>
+                                new ToolFailure({
+                                  message: `patch verification failed: Failed to read file to update ${target.absolute}: ${errorMessage(error)}`,
+                                }),
+                            ),
                           )
-                        : current.text,
+                          return Bom.join(content.text, content.bom)
+                        }))
+                      const before = Bom.split(original).text
+                      const update = yield* Effect.try({
+                        try: () => Patch.derive(hunk.path, hunk.chunks, original),
+                        catch: (error) =>
+                          new ToolFailure({ message: `patch verification failed: ${errorMessage(error)}` }),
+                      })
+                      const moveTarget = plan.moveTarget
+                      prepared.push({
+                        ...hunk,
+                        target,
+                        content: Patch.joinBom(update.content, update.bom),
+                        before,
+                        after: update.content,
+                        moveTarget,
+                      })
+                      if (!moveTarget) updates.set(target.absolute, Patch.joinBom(update.content, update.bom))
+                    }).pipe(
+                      Effect.mapError((error) =>
+                        error instanceof ToolFailure
+                          ? error
+                          : new ToolFailure({ message: `Unable to prepare patch at ${hunk.path}`, error }),
+                      ),
                     )
-                  }),
-                { discard: true },
+                  }
+
+                  yield* Effect.forEach(
+                    prepared,
+                    (change) =>
+                      Effect.gen(function* () {
+                        if (change.type === "add") {
+                          yield* environment.files
+                            .write(change.target.absolute, new TextEncoder().encode(change.content))
+                            .pipe(Effect.mapError((error) => fail(`Failed to write ${change.target.resource}`, error)))
+                          applied.push({
+                            type: change.type,
+                            resource: change.target.resource,
+                            target: change.target.absolute,
+                          })
+                          return
+                        }
+                        if (change.type === "delete") {
+                          yield* environment.files
+                            .remove(change.target.absolute)
+                            .pipe(Effect.mapError((error) => fail(`Failed to delete ${change.target.resource}`, error)))
+                          applied.push({
+                            type: change.type,
+                            resource: change.target.resource,
+                            target: change.target.absolute,
+                          })
+                          return
+                        }
+                        if (change.moveTarget) {
+                          const moveTarget = change.moveTarget
+                          yield* environment.files
+                            .write(moveTarget.absolute, new TextEncoder().encode(change.content))
+                            .pipe(Effect.mapError((error) => fail(`Failed to write ${moveTarget.resource}`, error)))
+                          yield* environment.files
+                            .remove(change.target.absolute)
+                            .pipe(
+                              Effect.mapError((error) =>
+                                fail(
+                                  `Wrote ${moveTarget.resource} but failed to remove ${change.target.resource}`,
+                                  error,
+                                ),
+                              ),
+                            )
+                          applied.push({
+                            type: change.type,
+                            resource: change.moveTarget.resource,
+                            target: change.moveTarget.absolute,
+                          })
+                          return
+                        }
+                        yield* environment.files
+                          .write(change.target.absolute, new TextEncoder().encode(change.content))
+                          .pipe(Effect.mapError((error) => fail(`Failed to write ${change.target.resource}`, error)))
+                        applied.push({
+                          type: change.type,
+                          resource: change.target.resource,
+                          target: change.target.absolute,
+                        })
+                      }),
+                    { discard: true },
+                  )
+                  const formatted = new Map<string, string>()
+                  yield* Effect.forEach(
+                    [...new Set(applied.filter((item) => item.type !== "delete").map((item) => item.target))],
+                    (target) =>
+                      Effect.gen(function* () {
+                        const current = yield* FileMutation.readText(environment.files, target).pipe(
+                          Effect.mapError((error) => fail(`Failed to read ${target}`, error)),
+                        )
+                        formatted.set(
+                          target,
+                          (yield* formatter.file(target))
+                            ? yield* FileMutation.syncTextBom(environment.files, target, current.bom).pipe(
+                                Effect.mapError((error) => fail(`Failed to sync ${target}`, error)),
+                              )
+                            : current.text,
+                        )
+                      }),
+                    { discard: true },
+                  )
+                  const files = yield* Effect.forEach(prepared, (change) => {
+                    if (change.type === "delete") return Effect.succeed(patchFile(change))
+                    const target = change.type === "update" && change.moveTarget ? change.moveTarget : change.target
+                    return Effect.succeed(patchFile(change, formatted.get(target.absolute)))
+                  })
+                  return { applied, files }
+                }),
               )
-              const files = yield* Effect.forEach(prepared, (change) => {
-                if (change.type === "delete") return Effect.succeed(patchFile(change))
-                const target = change.type === "update" && change.moveTarget ? change.moveTarget : change.target
-                return Effect.succeed(patchFile(change, formatted.get(target.absolute)))
-              })
-              return { applied, files }
             }).pipe(
-              mutation.withLock(lockTargets),
               Effect.map((output) => ({
                 output,
                 content: toModelOutput(output),

+ 16 - 29
packages/core/src/tool/plugin/write.ts

@@ -9,13 +9,11 @@ export * as WriteTool from "./write"
 import type { Context as PluginContext } from "@opencode-ai/plugin/effect/plugin"
 import { ToolFailure } from "@opencode-ai/ai"
 import { Effect, Schema } from "effect"
-import { Bom } from "@opencode-ai/util/bom"
 import { Environment } from "../../environment"
 import { FileMutation } from "../../file-mutation"
 import { Formatter } from "../../formatter"
 import { LocationMutation } from "../../location-mutation"
 import { Permission } from "../../permission"
-import { fileDiff } from "./file-diff"
 
 export const name = "write"
 
@@ -69,35 +67,24 @@ export const Plugin = {
                 id: context.id,
               }
               const target = yield* mutation.resolve({ path: input.path, kind: "file" })
+              const external = target.externalDirectory
+              if (external)
+                yield* permission.assert({
+                  ...LocationMutation.externalDirectoryPermission(external),
+                  sessionID: context.sessionID,
+                  agent: context.agent,
+                  source,
+                })
+              yield* permission.assert({
+                action: "edit",
+                resources: [target.resource],
+                save: ["*"],
+                sessionID: context.sessionID,
+                agent: context.agent,
+                source,
+              })
               return yield* fileMutation.withLock([target.absolute])(
                 Effect.gen(function* () {
-                  const external = target.externalDirectory
-                  if (external)
-                    yield* permission.assert({
-                      ...LocationMutation.externalDirectoryPermission(external),
-                      sessionID: context.sessionID,
-                      agent: context.agent,
-                      source,
-                    })
-                  const current = yield* FileMutation.readText(environment.files, target.absolute).pipe(
-                    Effect.catchTag("Environment.NotFound", () => Effect.succeed(undefined)),
-                  )
-                  const next = Bom.split(input.content)
-                  const preview = fileDiff(
-                    target.resource,
-                    current?.text ?? "",
-                    next.text,
-                    current ? "modified" : "added",
-                  )
-                  yield* permission.assert({
-                    action: "edit",
-                    resources: [target.resource],
-                    save: ["*"],
-                    metadata: { files: [preview] },
-                    sessionID: context.sessionID,
-                    agent: context.agent,
-                    source,
-                  })
                   const result = yield* fileMutation.writeTextPreservingBom({ target, content: input.content })
                   const bom = (yield* FileMutation.readText(environment.files, target.absolute)).bom
                   if (yield* formatter.file(target.absolute)) {

+ 67 - 55
packages/core/test/tool-edit.test.ts

@@ -1,7 +1,7 @@
 import fs from "fs/promises"
 import path from "path"
 import { describe, expect } from "bun:test"
-import { Effect, Layer } from "effect"
+import { Deferred, Effect, Fiber, Layer } 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"
@@ -40,6 +40,7 @@ const assertions: Permission.AssertInput[] = []
 const writes: string[] = []
 let reads = 0
 let denyAction: string | undefined
+let afterPermission = (_input: Permission.AssertInput): Effect.Effect<void> => Effect.void
 let afterRead = (_target: string, _content: Uint8Array): Effect.Effect<void> => Effect.void
 let formatFile = (_target: string): Effect.Effect<boolean> => Effect.succeed(false)
 
@@ -48,6 +49,7 @@ const permission = Layer.succeed(
   Permission.Service.of({
     assert: (input) =>
       Effect.sync(() => assertions.push(input)).pipe(
+        Effect.andThen(Effect.suspend(() => afterPermission(input))),
         Effect.andThen(
           input.action === denyAction
             ? Effect.fail(
@@ -77,6 +79,7 @@ const reset = () => {
   writes.length = 0
   reads = 0
   denyAction = undefined
+  afterPermission = () => Effect.void
   afterRead = () => Effect.void
   formatFile = () => Effect.succeed(false)
 }
@@ -174,17 +177,7 @@ describe("EditTool", () => {
                 })
                 expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after\nrest\n")
                 expect(assertions).toMatchObject([{ sessionID, action: "edit", resources: ["hello.txt"], save: ["*"] }])
-                expect(assertions[0]?.metadata).toMatchObject({
-                  files: [
-                    {
-                      file: "hello.txt",
-                      status: "modified",
-                      additions: 1,
-                      deletions: 1,
-                      patch: expect.stringContaining("-before\n+after"),
-                    },
-                  ],
-                })
+                expect(assertions[0]?.metadata).toBeUndefined()
                 expect(writes).toEqual([yield* Effect.promise(() => fs.realpath(target))])
               }),
             ),
@@ -349,7 +342,7 @@ describe("EditTool", () => {
             error: { type: "permission.rejected", message: "Permission denied: edit" },
           })
           expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"])
-          expect(reads).toBe(1)
+          expect(reads).toBe(0)
           expect(writes).toEqual([])
           expect(yield* Effect.promise(() => fs.readFile(external, "utf8"))).toBe("before")
         }),
@@ -386,7 +379,7 @@ describe("EditTool", () => {
                 })
                 expect(missing).toEqual(matching)
                 expect(assertions.map((input) => input.action)).toEqual(["edit", "edit"])
-                expect(reads).toBe(2)
+                expect(reads).toBe(0)
                 expect(writes).toEqual([])
               }),
             ),
@@ -643,58 +636,77 @@ describe("EditTool", () => {
       (tmp) => {
         reset()
         const target = path.join(tmp.path, "concurrent.txt")
-        afterRead = () => (reads === 1 ? Effect.sleep("50 millis") : Effect.void)
-        return Effect.promise(() => fs.writeFile(target, "one\ntwo\n")).pipe(
-          Effect.andThen(
-            withTool(tmp.path, (registry) =>
-              Effect.all(
-                [
-                  executeTool(
-                    registry,
-                    call({ path: "concurrent.txt", oldString: "one", newString: "ONE" }, "call-edit-one"),
-                  ),
-                  executeTool(
-                    registry,
-                    call({ path: "concurrent.txt", oldString: "two", newString: "TWO" }, "call-edit-two"),
-                  ),
-                ],
-                { concurrency: "unbounded" },
-              ),
+        return Effect.gen(function* () {
+          yield* Effect.promise(() => fs.writeFile(target, "one\ntwo\n"))
+          const firstRead = yield* Deferred.make<void>()
+          const releaseFirst = yield* Deferred.make<void>()
+          const secondApproved = yield* Deferred.make<void>()
+          afterRead = () =>
+            reads === 1
+              ? Deferred.succeed(firstRead, undefined).pipe(Effect.andThen(Deferred.await(releaseFirst)))
+              : Effect.void
+          afterPermission = (input) =>
+            input.source?.id === "call-edit-two"
+              ? Deferred.succeed(secondApproved, undefined).pipe(Effect.asVoid)
+              : Effect.void
+
+          const first = yield* withTool(tmp.path, (registry) =>
+            executeTool(
+              registry,
+              call({ path: "concurrent.txt", oldString: "one", newString: "ONE" }, "call-edit-one"),
             ),
-          ),
-          Effect.andThen((results) =>
-            Effect.gen(function* () {
-              expect(results.map((result) => result.status)).toEqual(["completed", "completed"])
-              expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("ONE\nTWO\n")
-            }),
-          ),
-        )
+          ).pipe(Effect.forkChild)
+          yield* Deferred.await(firstRead)
+          const second = yield* withTool(tmp.path, (registry) =>
+            executeTool(
+              registry,
+              call({ path: "concurrent.txt", oldString: "two", newString: "TWO" }, "call-edit-two"),
+            ),
+          ).pipe(Effect.forkChild)
+          yield* Deferred.await(secondApproved)
+          expect(reads).toBe(1)
+
+          yield* Deferred.succeed(releaseFirst, undefined)
+          expect((yield* Fiber.join(first)).status).toBe("completed")
+          expect((yield* Fiber.join(second)).status).toBe("completed")
+          expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("ONE\nTWO\n")
+        })
       },
       (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()),
     ),
   )
 
-  it.live("applies the edit when content changes after matching", () =>
+  it.live("validates current content after permission succeeds", () =>
     Effect.acquireUseRelease(
       Effect.promise(() => tmpdir()),
       (tmp) => {
         reset()
         const target = path.join(tmp.path, "concurrent.txt")
-        afterRead = () => (reads === 1 ? Effect.promise(() => fs.writeFile(target, "newer\n")) : Effect.void)
-        return Effect.promise(() => fs.writeFile(target, "before\n")).pipe(
-          Effect.andThen(
-            withTool(tmp.path, (registry) =>
-              executeTool(registry, call({ path: "concurrent.txt", oldString: "before", newString: "after" })),
-            ),
-          ),
-          Effect.andThen((result) =>
-            Effect.gen(function* () {
-              expect(result).toMatchObject({ status: "completed", output: { replacements: 1 } })
-              expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after\n")
-              expect(writes).toEqual([target])
-            }),
-          ),
-        )
+        return Effect.gen(function* () {
+          yield* Effect.promise(() => fs.writeFile(target, "before\n"))
+          const permissionReached = yield* Deferred.make<void>()
+          const releasePermission = yield* Deferred.make<void>()
+          afterPermission = (input) =>
+            input.action === "edit"
+              ? Deferred.succeed(permissionReached, undefined).pipe(Effect.andThen(Deferred.await(releasePermission)))
+              : Effect.void
+
+          const edit = yield* withTool(tmp.path, (registry) =>
+            executeTool(registry, call({ path: "concurrent.txt", oldString: "before", newString: "after" })),
+          ).pipe(Effect.forkChild)
+          yield* Deferred.await(permissionReached)
+          expect(reads).toBe(0)
+          yield* Effect.promise(() => fs.writeFile(target, "newer\n"))
+          yield* Deferred.succeed(releasePermission, undefined)
+
+          expect(yield* Fiber.join(edit)).toMatchObject({
+            status: "error",
+            error: { message: expect.stringContaining("Could not find oldString") },
+          })
+          expect(reads).toBe(1)
+          expect(writes).toEqual([])
+          expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("newer\n")
+        })
       },
       (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()),
     ),

+ 78 - 45
packages/core/test/tool-patch.test.ts

@@ -1,7 +1,7 @@
 import fs from "fs/promises"
 import path from "path"
 import { describe, expect } from "bun:test"
-import { Effect, Exit, Layer, Schema } from "effect"
+import { Deferred, Effect, Exit, Fiber, 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"
@@ -33,9 +33,11 @@ let denyAction: string | undefined
 let failRemoveTarget: string | undefined
 let failRemoveErrorTarget: string | undefined
 let failWriteTarget: string | undefined
+let reads = 0
 let readsBeforeEditApproval = 0
 let editApproved = false
-let afterEditApproval = (): Effect.Effect<void> => Effect.void
+let afterEditApproval = (_input: Permission.AssertInput): Effect.Effect<void> => Effect.void
+let afterRead = (_target: string, _content: Uint8Array): Effect.Effect<void> => Effect.void
 let formatFile = (_target: string): Effect.Effect<boolean> => Effect.succeed(false)
 
 const permission = Layer.succeed(
@@ -46,7 +48,7 @@ const permission = Layer.succeed(
         assertions.push(input)
         if (input.action === "edit") editApproved = true
       }).pipe(
-        Effect.andThen(input.action === "edit" ? Effect.suspend(afterEditApproval) : Effect.void),
+        Effect.andThen(input.action === "edit" ? Effect.suspend(() => afterEditApproval(input)) : Effect.void),
         Effect.andThen(
           input.action === denyAction
             ? Effect.fail(
@@ -77,9 +79,11 @@ const reset = () => {
   failRemoveTarget = undefined
   failRemoveErrorTarget = undefined
   failWriteTarget = undefined
+  reads = 0
   readsBeforeEditApproval = 0
   editApproved = false
   afterEditApproval = () => Effect.void
+  afterRead = () => Effect.void
   formatFile = () => Effect.succeed(false)
 }
 
@@ -104,8 +108,12 @@ const withTool = <A, E, R>(
           transformEnvironmentFiles(activeLocation, (files) => ({
             read: (target, range) =>
               Effect.sync(() => {
+                reads++
                 if (!editApproved) readsBeforeEditApproval++
-              }).pipe(Effect.andThen(files.read(target, range))),
+              }).pipe(
+                Effect.andThen(files.read(target, range)),
+                Effect.tap((result) => Effect.suspend(() => afterRead(target, result.bytes))),
+              ),
             remove: (target) => {
               if (failRemoveTarget && path.basename(target) === failRemoveTarget)
                 return Effect.die("forced remove failure")
@@ -219,14 +227,10 @@ describe("PatchTool", () => {
                     action: "edit",
                     resources: ["nested/new.txt", "update.txt", "remove.txt"],
                     save: ["*"],
-                    metadata: {
-                      filepath: "nested/new.txt, update.txt, remove.txt",
-                      diff: expect.stringContaining("Index:"),
-                      files: expect.any(Array),
-                    },
                   },
                 ])
-                expect(readsBeforeEditApproval).toBe(2)
+                expect(assertions[0]?.metadata).toBeUndefined()
+                expect(readsBeforeEditApproval).toBe(0)
                 expect(yield* Effect.promise(() => fs.readFile(path.join(tmp.path, "nested/new.txt"), "utf8"))).toBe(
                   "created\n",
                 )
@@ -267,40 +271,69 @@ describe("PatchTool", () => {
   it.live("serializes concurrent patch transactions", () =>
     withTempTool((directory, registry) => {
       const target = path.join(directory, "concurrent.txt")
-      afterEditApproval = () =>
-        assertions.filter((input) => input.action === "edit").length === 1 ? Effect.sleep("50 millis") : Effect.void
-      return Effect.promise(() => fs.writeFile(target, "one\ntwo\n")).pipe(
-        Effect.andThen(
-          Effect.all(
-            [
-              executeTool(
-                registry,
-                call(
-                  "*** Begin Patch\n*** Update File: concurrent.txt\n@@\n-one\n+ONE\n*** End Patch",
-                  "call-patch-one",
-                ),
-              ),
-              executeTool(
-                registry,
-                call(
-                  "*** Begin Patch\n*** Update File: concurrent.txt\n@@\n-two\n+TWO\n*** End Patch",
-                  "call-patch-two",
-                ),
-              ),
-            ],
-            { concurrency: "unbounded" },
-          ),
-        ),
-        Effect.andThen((results) =>
-          Effect.gen(function* () {
-            expect(results.map((result) => result.status)).toEqual(["completed", "completed"])
-            expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("ONE\nTWO\n")
-          }),
-        ),
-      )
+      return Effect.gen(function* () {
+        yield* Effect.promise(() => fs.writeFile(target, "one\ntwo\n"))
+        const firstRead = yield* Deferred.make<void>()
+        const releaseFirst = yield* Deferred.make<void>()
+        const secondApproved = yield* Deferred.make<void>()
+        afterRead = () =>
+          reads === 1
+            ? Deferred.succeed(firstRead, undefined).pipe(Effect.andThen(Deferred.await(releaseFirst)))
+            : Effect.void
+        afterEditApproval = (input) =>
+          input.source?.id === "call-patch-two"
+            ? Deferred.succeed(secondApproved, undefined).pipe(Effect.asVoid)
+            : Effect.void
+
+        const first = yield* executeTool(
+          registry,
+          call("*** Begin Patch\n*** Update File: concurrent.txt\n@@\n-one\n+ONE\n*** End Patch", "call-patch-one"),
+        ).pipe(Effect.forkChild)
+        yield* Deferred.await(firstRead)
+        const second = yield* executeTool(
+          registry,
+          call("*** Begin Patch\n*** Update File: concurrent.txt\n@@\n-two\n+TWO\n*** End Patch", "call-patch-two"),
+        ).pipe(Effect.forkChild)
+        yield* Deferred.await(secondApproved)
+        expect(reads).toBe(1)
+
+        yield* Deferred.succeed(releaseFirst, undefined)
+        expect((yield* Fiber.join(first)).status).toBe("completed")
+        expect((yield* Fiber.join(second)).status).toBe("completed")
+        expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("ONE\nTWO\n")
+      })
     }),
   )
 
+  it.live("validates patch context after permission succeeds", () =>
+    withTempTool((directory, registry) =>
+      Effect.gen(function* () {
+        const target = path.join(directory, "current.txt")
+        yield* Effect.promise(() => fs.writeFile(target, "before\n"))
+        const permissionReached = yield* Deferred.make<void>()
+        const releasePermission = yield* Deferred.make<void>()
+        afterEditApproval = () =>
+          Deferred.succeed(permissionReached, undefined).pipe(Effect.andThen(Deferred.await(releasePermission)))
+
+        const patch = yield* executeTool(
+          registry,
+          call("*** Begin Patch\n*** Update File: current.txt\n@@\n-before\n+after\n*** End Patch"),
+        ).pipe(Effect.forkChild)
+        yield* Deferred.await(permissionReached)
+        expect(reads).toBe(0)
+        yield* Effect.promise(() => fs.writeFile(target, "newer\n"))
+        yield* Deferred.succeed(releasePermission, undefined)
+
+        expect(yield* Fiber.join(patch)).toMatchObject({
+          status: "error",
+          error: { message: expect.stringContaining("Failed to find expected lines") },
+        })
+        expect(reads).toBe(1)
+        expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("newer\n")
+      }),
+    ),
+  )
+
   it.live("returns file diffs for final formatted content", () =>
     withTempTool((directory, registry) => {
       const target = path.join(directory, "formatted.txt")
@@ -783,7 +816,7 @@ describe("PatchTool", () => {
     ),
   )
 
-  it.live("approves an external directory before reading and requests edit permission afterward", () =>
+  it.live("approves external-directory and edit access before reading", () =>
     Effect.acquireUseRelease(
       Effect.promise(() => Promise.all([tmpdir(), tmpdir()])),
       ([active, outside]) => {
@@ -800,7 +833,7 @@ describe("PatchTool", () => {
                   ),
                 ).toMatchObject({ status: "completed" })
                 expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"])
-                expect(readsBeforeEditApproval).toBe(1)
+                expect(readsBeforeEditApproval).toBe(0)
                 expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after\n")
               }),
             ),
@@ -931,7 +964,7 @@ describe("PatchTool", () => {
     ),
   )
 
-  it.live("approves a relative external target before reading and requests edit permission afterward", () =>
+  it.live("approves a relative external target before reading", () =>
     Effect.acquireUseRelease(
       Effect.promise(() => Promise.all([tmpdir(), tmpdir()])),
       ([active, outside]) => {
@@ -949,7 +982,7 @@ describe("PatchTool", () => {
                   ),
                 ).toMatchObject({ status: "completed" })
                 expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"])
-                expect(readsBeforeEditApproval).toBe(1)
+                expect(readsBeforeEditApproval).toBe(0)
                 expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after\n")
               }),
             ),

+ 43 - 26
packages/core/test/tool-write.test.ts

@@ -45,6 +45,7 @@ const editToolNode = makeLocationNode({
 const sessionID = Session.ID.make("ses_write_tool_test")
 const assertions: Permission.AssertInput[] = []
 const writes: string[] = []
+let reads = 0
 let formatFile = (_target: string): Effect.Effect<boolean> => Effect.succeed(false)
 let afterPermission = (_input: Permission.AssertInput): Effect.Effect<void> => Effect.void
 let denyAction: string | undefined
@@ -82,6 +83,7 @@ const formatter = Layer.mock(Formatter.Service, {
 const reset = () => {
   assertions.length = 0
   writes.length = 0
+  reads = 0
   formatFile = () => Effect.succeed(false)
   afterPermission = () => Effect.void
   denyAction = undefined
@@ -112,6 +114,7 @@ const withTool = <A, E, R>(
           [
             Environment.node,
             transformEnvironmentFiles(activeLocation, (files) => ({
+              read: (target, range) => Effect.sync(() => reads++).pipe(Effect.andThen(files.read(target, range))),
               write: (target, content) =>
                 Effect.sync(() => writes.push(target)).pipe(Effect.andThen(files.write(target, content))),
             })),
@@ -163,17 +166,7 @@ describe("WriteTool", () => {
               "created",
             )
             expect(assertions).toMatchObject([{ sessionID, action: "edit", resources: ["src/new.txt"], save: ["*"] }])
-            expect(assertions[0]?.metadata).toMatchObject({
-              files: [
-                {
-                  file: "src/new.txt",
-                  status: "added",
-                  additions: 1,
-                  deletions: 0,
-                  patch: expect.stringContaining("+created"),
-                },
-              ],
-            })
+            expect(assertions[0]?.metadata).toBeUndefined()
             expect(writes).toEqual([path.join(yield* Effect.promise(() => fs.realpath(tmp.path)), "src", "new.txt")])
           }),
         )
@@ -221,17 +214,7 @@ describe("WriteTool", () => {
               if (settled.status !== "completed") return
               expect(settled.content).toEqual([{ type: "text", text: "Wrote file successfully: existing.txt" }])
               expect(settled.output).toMatchObject({ resource: "existing.txt", existed: true })
-              expect(assertions[0]?.metadata).toMatchObject({
-                files: [
-                  {
-                    file: "existing.txt",
-                    status: "modified",
-                    additions: 1,
-                    deletions: 1,
-                    patch: expect.stringMatching(/-before[\s\S]*\+after/),
-                  },
-                ],
-              })
+              expect(assertions[0]?.metadata).toBeUndefined()
               expect(yield* Effect.promise(() => fs.readFile(path.join(tmp.path, "existing.txt"), "utf8"))).toBe(
                 "after",
               )
@@ -447,7 +430,7 @@ describe("WriteTool", () => {
     ),
   )
 
-  it.live("serializes write and edit transactions across Location service instances", () =>
+  it.live("authorizes an edit while a write holds the same-path execution lock", () =>
     Effect.acquireUseRelease(
       Effect.promise(() => tmpdir()),
       (tmp) => {
@@ -487,13 +470,12 @@ describe("WriteTool", () => {
               ),
             { edit: true },
           ).pipe(Effect.forkChild)
-          yield* Effect.yieldNow
-          expect(yield* Deferred.isDone(editApproved)).toBe(false)
+          yield* Deferred.await(editApproved)
+          expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("before")
 
           yield* Deferred.succeed(releaseFormatting, undefined)
           expect((yield* Fiber.join(write)).status).toBe("completed")
           expect((yield* Fiber.join(edit)).status).toBe("completed")
-          expect(yield* Deferred.isDone(editApproved)).toBe(true)
           expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("after")
         })
       },
@@ -501,4 +483,39 @@ describe("WriteTool", () => {
     ),
   )
 
+  it.live("does not hold the execution lock while waiting for permission", () =>
+    Effect.acquireUseRelease(
+      Effect.promise(() => tmpdir()),
+      (tmp) => {
+        reset()
+        const target = path.join(tmp.path, "shared.txt")
+        return Effect.gen(function* () {
+          yield* Effect.promise(() => fs.writeFile(target, "initial"))
+          const firstAsked = yield* Deferred.make<void>()
+          const releaseFirst = yield* Deferred.make<void>()
+          afterPermission = (input) =>
+            input.source?.id === "call-waiting-write" && input.action === "edit"
+              ? Deferred.succeed(firstAsked, undefined).pipe(Effect.andThen(Deferred.await(releaseFirst)))
+              : Effect.void
+
+          const first = yield* withTool(tmp.path, (registry) =>
+            executeTool(registry, call({ path: "shared.txt", content: "first" }, "call-waiting-write")),
+          ).pipe(Effect.forkChild)
+          yield* Deferred.await(firstAsked)
+          expect(reads).toBe(0)
+
+          const second = yield* withTool(tmp.path, (registry) =>
+            executeTool(registry, call({ path: "shared.txt", content: "second" }, "call-approved-write")),
+          )
+          expect(second.status).toBe("completed")
+          expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("second")
+
+          yield* Deferred.succeed(releaseFirst, undefined)
+          expect((yield* Fiber.join(first)).status).toBe("completed")
+          expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("first")
+        })
+      },
+      (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()),
+    ),
+  )
 })

+ 0 - 1
packages/tui/src/util/permission.ts

@@ -38,7 +38,6 @@ export function permissionPresentation(
       title: `Edit ${formatPath(file)}`,
       lines: [],
       diff,
-      patch: diff ? undefined : text(input.patchText) || undefined,
       file,
     }
   }

+ 2 - 4
packages/tui/test/mini/permission.shared.test.ts

@@ -152,7 +152,7 @@ describe("run permission shared", () => {
     })
   })
 
-  test("uses source patch text when an edit has no generated diff", () => {
+  test("uses the resource display when an edit has no generated diff", () => {
     const patch = '*** Begin Patch\n*** Update File: src/index.ts\n@@\n-old\n+const arrow = "→"\n*** End Patch'
     const request = req({
       action: "edit",
@@ -171,13 +171,11 @@ describe("run permission shared", () => {
     expect(permissionInfo(request)).toMatchObject({
       title: "Edit src/index.ts",
       diff: undefined,
-      patch,
     })
     expect(permissionInfo(request, undefined, true)).toMatchObject({
       title: "Edit src/index.ts",
-      lines: [patch],
+      lines: [],
       diff: undefined,
-      patch: undefined,
     })
   })