diff --git a/packages/cli/src/acp/permission.ts b/packages/cli/src/acp/permission.ts index cef7fc2cf4..0d45def396 100644 --- a/packages/cli/src/acp/permission.ts +++ b/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 { - 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 { - 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 => { - 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) { - if (previews.length > 1) return `${previews.length} files` +function permissionTitle(toolName: string, input: ToolInput, resources: ReadonlyArray) { + 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, cwd: string, - previews: ReadonlyArray, ): 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) } diff --git a/packages/cli/test/acp/permission-behavior.test.ts b/packages/cli/test/acp/permission-behavior.test.ts index 3dabbe5714..8b823ccc16 100644 --- a/packages/cli/test/acp/permission-behavior.test.ts +++ b/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 readonly metadata?: Record 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 } : {}), }) diff --git a/packages/core/src/tool/plugin/edit.ts b/packages/core/src/tool/plugin/edit.ts index c7162367aa..8dbedb0103 100644 --- a/packages/core/src/tool/plugin/edit.ts +++ b/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"})`, diff --git a/packages/core/src/tool/plugin/patch.ts b/packages/core/src/tool/plugin/patch.ts index 0c22b6bcbf..f0c873ecbe 100644 --- a/packages/core/src/tool/plugin/patch.ts +++ b/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 = [] 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() - 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() + 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() - 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() + 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), diff --git a/packages/core/src/tool/plugin/write.ts b/packages/core/src/tool/plugin/write.ts index 0eb0d4f5fe..7bb3ee6812 100644 --- a/packages/core/src/tool/plugin/write.ts +++ b/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)) { diff --git a/packages/core/test/tool-edit.test.ts b/packages/core/test/tool-edit.test.ts index e730ebf53d..d06262311f 100644 --- a/packages/core/test/tool-edit.test.ts +++ b/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 => Effect.void let afterRead = (_target: string, _content: Uint8Array): Effect.Effect => Effect.void let formatFile = (_target: string): Effect.Effect => 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() + const releaseFirst = yield* Deferred.make() + const secondApproved = yield* Deferred.make() + 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() + const releasePermission = yield* Deferred.make() + 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]()), ), diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index 5a08fc8dfc..a9094de68c 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/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 => Effect.void +let afterEditApproval = (_input: Permission.AssertInput): Effect.Effect => Effect.void +let afterRead = (_target: string, _content: Uint8Array): Effect.Effect => Effect.void let formatFile = (_target: string): Effect.Effect => 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 = ( 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() + const releaseFirst = yield* Deferred.make() + const secondApproved = yield* Deferred.make() + 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() + const releasePermission = yield* Deferred.make() + 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") }), ), diff --git a/packages/core/test/tool-write.test.ts b/packages/core/test/tool-write.test.ts index f43c6b42ef..80691b5f83 100644 --- a/packages/core/test/tool-write.test.ts +++ b/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 => Effect.succeed(false) let afterPermission = (_input: Permission.AssertInput): Effect.Effect => 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 = ( [ 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() + const releaseFirst = yield* Deferred.make() + 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]()), + ), + ) }) diff --git a/packages/tui/src/util/permission.ts b/packages/tui/src/util/permission.ts index 165943c413..4b1e4a12ee 100644 --- a/packages/tui/src/util/permission.ts +++ b/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, } } diff --git a/packages/tui/test/mini/permission.shared.test.ts b/packages/tui/test/mini/permission.shared.test.ts index 84a9ae3f47..97143505a4 100644 --- a/packages/tui/test/mini/permission.shared.test.ts +++ b/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, }) })