From 942ba1edde69a8f1f8faa4b6fffc3a747eb370ce Mon Sep 17 00:00:00 2001 From: asdfqwerzxcc Date: Mon, 14 Sep 2026 00:21:59 +0900 Subject: [PATCH 1/5] fix(coding-agent): reserve canonical mutation queues Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai --- .../coding-agent/src/core/tools/changes.md | 26 +++++++++++++++++++ .../src/core/tools/file-mutation-queue.ts | 21 +++++++++------ .../test/file-mutation-queue.test.ts | 17 +++++++++++- 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/src/core/tools/changes.md b/packages/coding-agent/src/core/tools/changes.md index 52700e364b..c5430ed6f9 100644 --- a/packages/coding-agent/src/core/tools/changes.md +++ b/packages/coding-agent/src/core/tools/changes.md @@ -1,5 +1,31 @@ # core/tools changes +## Multi-file mutation queue reservations (2026-09-13) + +### What changed + +- `packages/coding-agent/src/core/tools/file-mutation-queue.ts` adds + `withFileMutationQueues()`, which resolves every path to the existing canonical + identity, deduplicates aliases, and reserves all identities before running one + mutation transaction. + +### Why + +- Transactional `apply_patch` rollback must keep every affected path isolated until + restoration finishes. Nesting the single-file helper deadlocks when two patch + paths are case or symlink aliases of the same file. + +### Why an extension could not handle it + +- Edit, write, and builtin extension mutations share this core queue. Only the queue + owner can atomically reserve canonical identities without duplicating its bounded + realpath and case-folding contract. + +### Expected merge conflict zones + +- LOW: queue registration and release in + `packages/coding-agent/src/core/tools/file-mutation-queue.ts`. + ## Session cwd and goal-store environment keys (2026-09-13) ### What changed diff --git a/packages/coding-agent/src/core/tools/file-mutation-queue.ts b/packages/coding-agent/src/core/tools/file-mutation-queue.ts index 369d26a3ec..85d69e2134 100644 --- a/packages/coding-agent/src/core/tools/file-mutation-queue.ts +++ b/packages/coding-agent/src/core/tools/file-mutation-queue.ts @@ -22,7 +22,7 @@ let registrationQueue = Promise.resolve(); async function getMutationQueueKey(filePath: string): Promise { const resolvedPath = resolve(filePath); const canonicalPath = await withResolutionDeadline(realpath(resolvedPath)).catch((error: unknown) => { - if (isMissingPathError(error)) return resolvedPath; + if (isMissingPathError(error)) return realpathWithoutOpenStrict(resolvedPath); throw error; }); const identity = canonicalPath === RESOLUTION_TIMED_OUT ? realpathWithoutOpenStrict(resolvedPath) : canonicalPath; @@ -34,32 +34,37 @@ async function getMutationQueueKey(filePath: string): Promise { * Operations for different files still run in parallel. */ export async function withFileMutationQueue(filePath: string, fn: () => Promise): Promise { + return withFileMutationQueues([filePath], fn); +} + +/** Reserve all canonical identities together, without nesting queues for aliases of one file. */ +export async function withFileMutationQueues(filePaths: readonly string[], fn: () => Promise): Promise { const registration = registrationQueue.then(async () => { - const key = await getMutationQueueKey(filePath); - const currentQueue = fileMutationQueues.get(key) ?? Promise.resolve(); + const keys = [...new Set(await Promise.all(filePaths.map(getMutationQueueKey)))]; + const currentQueue = Promise.all(keys.map((key) => fileMutationQueues.get(key))).then(() => undefined); let releaseNext!: () => void; const nextQueue = new Promise((resolveQueue) => { releaseNext = resolveQueue; }); const chainedQueue = currentQueue.then(() => nextQueue); - fileMutationQueues.set(key, chainedQueue); + for (const key of keys) fileMutationQueues.set(key, chainedQueue); - return { key, currentQueue, chainedQueue, releaseNext }; + return { keys, currentQueue, chainedQueue, releaseNext }; }); registrationQueue = registration.then( () => undefined, () => undefined, ); - const { key, currentQueue, chainedQueue, releaseNext } = await registration; + const { keys, currentQueue, chainedQueue, releaseNext } = await registration; await currentQueue; try { return await fn(); } finally { releaseNext(); - if (fileMutationQueues.get(key) === chainedQueue) { - fileMutationQueues.delete(key); + for (const key of keys) { + if (fileMutationQueues.get(key) === chainedQueue) fileMutationQueues.delete(key); } } } diff --git a/packages/coding-agent/test/file-mutation-queue.test.ts b/packages/coding-agent/test/file-mutation-queue.test.ts index 4250210ed3..7889b73891 100644 --- a/packages/coding-agent/test/file-mutation-queue.test.ts +++ b/packages/coding-agent/test/file-mutation-queue.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { createEditTool } from "../src/core/tools/edit.ts"; -import { withFileMutationQueue } from "../src/core/tools/file-mutation-queue.ts"; +import { withFileMutationQueue, withFileMutationQueues } from "../src/core/tools/file-mutation-queue.ts"; import { createWriteTool } from "../src/core/tools/write.ts"; function delay(ms: number): Promise { @@ -107,6 +107,21 @@ describe("withFileMutationQueue", () => { expect(order).toEqual(["target:start", "target:end", "alias:start", "alias:end"]); }); + + it("deduplicates symlink aliases within one multi-file reservation", async () => { + const dir = await createTempDir(); + const targetPath = join(dir, "target.txt"); + const symlinkPath = join(dir, "alias.txt"); + await writeFile(targetPath, "hello\n", "utf8"); + await symlink(targetPath, symlinkPath); + + let calls = 0; + await withFileMutationQueues([targetPath, symlinkPath], async () => { + calls += 1; + }); + + expect(calls).toBe(1); + }); }); describe("built-in edit and write tools", () => { From 43b0f898777a7f336d03e5f695cc88fe8a7d6c34 Mon Sep 17 00:00:00 2001 From: asdfqwerzxcc Date: Mon, 14 Sep 2026 00:22:10 +0900 Subject: [PATCH 2/5] fix(coding-agent): rollback cancelled patch transactions Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai --- .../gpt-apply-patch/apply-operation.ts | 146 ++++++++++ .../builtin/gpt-apply-patch/apply.ts | 261 ++++++------------ .../builtin/gpt-apply-patch/transaction.ts | 95 +++++++ 3 files changed, 323 insertions(+), 179 deletions(-) create mode 100644 packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply-operation.ts create mode 100644 packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/transaction.ts diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply-operation.ts b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply-operation.ts new file mode 100644 index 0000000000..fd79f9905d --- /dev/null +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply-operation.ts @@ -0,0 +1,146 @@ +import { mkdir, rename, rm, unlink, writeFile } from "node:fs/promises"; +import path from "node:path"; +import { withFileMutationQueues } from "../../../tools/file-mutation-queue.ts"; +import { replaceChunks } from "./patch-replace.ts"; +import { buildPatchPreviewFile, readPatchFileSnapshot } from "./preview.ts"; +import type { AtomicWriteOperations, ParsedPatch } from "./types.ts"; +import { resolvePatchPath } from "./workspace.ts"; + +const ATOMIC_WRITE_OPERATIONS: AtomicWriteOperations = { writeFile, rename, unlink }; + +function hasErrorCode(error: unknown, code: string): boolean { + return Boolean(error && typeof error === "object" && "code" in error && error.code === code); +} + +async function writeFileAtomic( + absPath: string, + content: string, + operations: AtomicWriteOperations = ATOMIC_WRITE_OPERATIONS, +): Promise { + const tempPath = `${absPath}.tmp.${process.pid}.${Math.random().toString(16).slice(2)}`; + await operations.writeFile(tempPath, content, "utf-8"); + try { + await operations.rename(tempPath, absPath); + } catch (error) { + if (!hasErrorCode(error, "EEXIST")) throw error; + await operations.unlink(absPath); + await operations.rename(tempPath, absPath); + } +} + +async function writeBinaryFileAtomic(absPath: string, content: Uint8Array): Promise { + const tempPath = `${absPath}.tmp.${process.pid}.${Math.random().toString(16).slice(2)}`; + await writeFile(tempPath, content); + try { + await rename(tempPath, absPath); + } catch (error) { + if (!hasErrorCode(error, "EEXIST")) throw error; + await unlink(absPath); + await rename(tempPath, absPath); + } +} + +export async function __testWriteFileAtomic( + absPath: string, + content: string, + operations: AtomicWriteOperations, +): Promise { + await writeFileAtomic(absPath, content, operations); +} + +export function patchMutationPaths(cwd: string, hunk: ParsedPatch): string[] { + return hunk.type === "update" && hunk.movePath + ? [resolvePatchPath(cwd, hunk.filePath), resolvePatchPath(cwd, hunk.movePath)] + : [resolvePatchPath(cwd, hunk.filePath)]; +} + +async function applySingleHunkUnlocked( + cwd: string, + hunk: ParsedPatch, +): Promise<{ + readonly summary: string; + readonly appliedFile: string; + readonly fuzz: number; + readonly preview: ReturnType; +}> { + const absolutePath = resolvePatchPath(cwd, hunk.filePath); + if (hunk.type === "add") { + const source = await readPatchFileSnapshot(absolutePath); + const preview = buildPatchPreviewFile({ hunk, source, newContent: hunk.content }); + await mkdir(path.dirname(absolutePath), { recursive: true }); + await writeFileAtomic(absolutePath, hunk.content); + return { summary: `add: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: 0, preview }; + } + + if (hunk.type === "delete") { + const source = await readPatchFileSnapshot(absolutePath); + const preview = buildPatchPreviewFile({ hunk, source, newContent: "" }); + await rm(absolutePath); + return { summary: `delete: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: 0, preview }; + } + + const source = await readPatchFileSnapshot(absolutePath); + if (!source.exists) { + const error = new Error(`ENOENT: no such file or directory, open '${absolutePath}'`) as NodeJS.ErrnoException; + error.code = "ENOENT"; + throw error; + } + const absoluteMovePath = hunk.movePath ? resolvePatchPath(cwd, hunk.movePath) : undefined; + const moveDestination = + absoluteMovePath && absoluteMovePath !== absolutePath ? await readPatchFileSnapshot(absoluteMovePath) : undefined; + if (source.binary) { + if (hunk.chunks.length > 0) { + throw new Error(`apply_patch cannot apply text hunks to binary file: ${hunk.filePath}`); + } + if (!hunk.movePath || !absoluteMovePath || !source.bytes) { + throw new Error(`apply_patch cannot update binary file without a move destination: ${hunk.filePath}`); + } + const preview = buildPatchPreviewFile({ + hunk, + source, + newContent: "", + ...(moveDestination ? { moveDestination } : {}), + }); + await mkdir(path.dirname(absoluteMovePath), { recursive: true }); + await writeBinaryFileAtomic(absoluteMovePath, source.bytes); + if (absoluteMovePath !== absolutePath) await rm(absolutePath); + return { + summary: `move: ${hunk.filePath} -> ${hunk.movePath}`, + appliedFile: hunk.movePath, + fuzz: 0, + preview, + }; + } + + const chunkResult = + hunk.chunks.length === 0 + ? { content: source.content, fuzz: 0 } + : replaceChunks(source.content, hunk.filePath, hunk.chunks); + + if (hunk.movePath && absoluteMovePath) { + const preview = buildPatchPreviewFile({ + hunk, + source, + newContent: chunkResult.content, + ...(moveDestination ? { moveDestination } : {}), + }); + await mkdir(path.dirname(absoluteMovePath), { recursive: true }); + await writeFileAtomic(absoluteMovePath, chunkResult.content); + if (absoluteMovePath !== absolutePath) await rm(absolutePath); + return { + summary: `move: ${hunk.filePath} -> ${hunk.movePath}`, + appliedFile: hunk.movePath, + fuzz: chunkResult.fuzz, + preview, + }; + } + + const preview = buildPatchPreviewFile({ hunk, source, newContent: chunkResult.content }); + await writeFileAtomic(absolutePath, chunkResult.content); + return { summary: `update: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: chunkResult.fuzz, preview }; +} + +export async function applySingleHunk(cwd: string, hunk: ParsedPatch, queuesHeld = false) { + if (queuesHeld) return applySingleHunkUnlocked(cwd, hunk); + return withFileMutationQueues(patchMutationPaths(cwd, hunk), () => applySingleHunkUnlocked(cwd, hunk)); +} diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply.ts b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply.ts index d485693d86..eb8738e0c4 100644 --- a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply.ts +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply.ts @@ -1,26 +1,20 @@ -import { mkdir, rename, rm, unlink, writeFile } from "node:fs/promises"; -import path from "node:path"; -import { withFileMutationQueue } from "../../../tools/file-mutation-queue.ts"; +import { __testWriteFileAtomic, applySingleHunk, patchMutationPaths } from "./apply-operation.ts"; import { ApplyPatchError } from "./errors.ts"; import { parsePatch } from "./parser.ts"; -import { replaceChunks } from "./patch-replace.ts"; -import { buildPatchPreviewFile, readPatchFileSnapshot } from "./preview.ts"; import { createRecoveryInstructions } from "./recovery.ts"; export { buildPartialFailureText } from "./recovery.ts"; +export { __testWriteFileAtomic }; import { normalizePatchText } from "./text.ts"; +import { type PatchTransaction, runPatchTransaction } from "./transaction.ts"; import type { AppliedPatchOperation, ApplyPatchFailure, ApplyPatchProgressCallback, ApplyPatchResult, - AtomicWriteOperations, ParsedPatch, } from "./types.ts"; -import { resolvePatchPath } from "./workspace.ts"; - -const ATOMIC_WRITE_OPERATIONS: AtomicWriteOperations = { writeFile, rename, unlink }; async function notifyApplyPatchProgress( onProgress: ApplyPatchProgressCallback | undefined, @@ -33,10 +27,6 @@ async function notifyApplyPatchProgress( } } -function hasErrorCode(error: unknown, code: string): boolean { - return Boolean(error && typeof error === "object" && "code" in error && error.code === code); -} - function extractErrorCode(error: unknown): string | undefined { if (error && typeof error === "object" && "code" in error && typeof error.code === "string") { return error.code; @@ -57,152 +47,6 @@ export function compactApplyPatchResult(result: ApplyPatchResult): ApplyPatchRes }; } -async function writeFileAtomic( - absPath: string, - content: string, - operations: AtomicWriteOperations = ATOMIC_WRITE_OPERATIONS, -): Promise { - const tempPath = `${absPath}.tmp.${process.pid}.${Math.random().toString(16).slice(2)}`; - await operations.writeFile(tempPath, content, "utf-8"); - try { - await operations.rename(tempPath, absPath); - } catch (error) { - if (!hasErrorCode(error, "EEXIST")) throw error; - await operations.unlink(absPath); - await operations.rename(tempPath, absPath); - } -} - -async function writeBinaryFileAtomic(absPath: string, content: Uint8Array): Promise { - const tempPath = `${absPath}.tmp.${process.pid}.${Math.random().toString(16).slice(2)}`; - await writeFile(tempPath, content); - try { - await rename(tempPath, absPath); - } catch (error) { - if (!hasErrorCode(error, "EEXIST")) throw error; - await unlink(absPath); - await rename(tempPath, absPath); - } -} - -export async function __testWriteFileAtomic( - absPath: string, - content: string, - operations: AtomicWriteOperations, -): Promise { - await writeFileAtomic(absPath, content, operations); -} - -async function applySingleHunk( - cwd: string, - hunk: ParsedPatch, -): Promise<{ - readonly summary: string; - readonly appliedFile: string; - readonly fuzz: number; - readonly preview: ReturnType; -}> { - const absolutePath = resolvePatchPath(cwd, hunk.filePath); - const mutationPaths = - hunk.type === "update" && hunk.movePath ? [absolutePath, resolvePatchPath(cwd, hunk.movePath)] : [absolutePath]; - return withPatchMutationQueues(mutationPaths, async () => { - if (hunk.type === "add") { - const source = await readPatchFileSnapshot(absolutePath); - const preview = buildPatchPreviewFile({ hunk, source, newContent: hunk.content }); - await mkdir(path.dirname(absolutePath), { recursive: true }); - await writeFileAtomic(absolutePath, hunk.content); - return { summary: `add: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: 0, preview }; - } - - if (hunk.type === "delete") { - const source = await readPatchFileSnapshot(absolutePath); - const preview = buildPatchPreviewFile({ - hunk, - source, - newContent: "", - }); - await rm(absolutePath); - return { summary: `delete: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: 0, preview }; - } - - const source = await readPatchFileSnapshot(absolutePath); - if (!source.exists) { - const error = new Error(`ENOENT: no such file or directory, open '${absolutePath}'`) as NodeJS.ErrnoException; - error.code = "ENOENT"; - throw error; - } - const absoluteMovePath = hunk.movePath ? resolvePatchPath(cwd, hunk.movePath) : undefined; - const moveDestination = - absoluteMovePath && absoluteMovePath !== absolutePath - ? await readPatchFileSnapshot(absoluteMovePath) - : undefined; - if (source.binary) { - if (hunk.chunks.length > 0) { - throw new Error(`apply_patch cannot apply text hunks to binary file: ${hunk.filePath}`); - } - if (!hunk.movePath || !absoluteMovePath || !source.bytes) { - throw new Error(`apply_patch cannot update binary file without a move destination: ${hunk.filePath}`); - } - const preview = buildPatchPreviewFile({ - hunk, - source, - newContent: "", - ...(moveDestination ? { moveDestination } : {}), - }); - await mkdir(path.dirname(absoluteMovePath), { recursive: true }); - await writeBinaryFileAtomic(absoluteMovePath, source.bytes); - if (absoluteMovePath !== absolutePath) await rm(absolutePath); - return { - summary: `move: ${hunk.filePath} -> ${hunk.movePath}`, - appliedFile: hunk.movePath, - fuzz: 0, - preview, - }; - } - - const chunkResult = - hunk.chunks.length === 0 - ? { content: source.content, fuzz: 0 } - : replaceChunks(source.content, hunk.filePath, hunk.chunks); - - if (hunk.movePath && absoluteMovePath) { - const preview = buildPatchPreviewFile({ - hunk, - source, - newContent: chunkResult.content, - ...(moveDestination ? { moveDestination } : {}), - }); - await mkdir(path.dirname(absoluteMovePath), { recursive: true }); - await writeFileAtomic(absoluteMovePath, chunkResult.content); - if (absoluteMovePath !== absolutePath) await rm(absolutePath); - return { - summary: `move: ${hunk.filePath} -> ${hunk.movePath}`, - appliedFile: hunk.movePath, - fuzz: chunkResult.fuzz, - preview, - }; - } - - const preview = buildPatchPreviewFile({ - hunk, - source, - newContent: chunkResult.content, - }); - await writeFileAtomic(absolutePath, chunkResult.content); - return { summary: `update: ${hunk.filePath}`, appliedFile: hunk.filePath, fuzz: chunkResult.fuzz, preview }; - }); -} - -async function withPatchMutationQueues(filePaths: readonly string[], operation: () => Promise): Promise { - const sortedPaths = [...new Set(filePaths)].sort((left, right) => left.localeCompare(right)); - const runQueued = (index: number): Promise => { - const filePath = sortedPaths[index]; - if (filePath === undefined) return operation(); - return withFileMutationQueue(filePath, () => runQueued(index + 1)); - }; - return runQueued(0); -} - function parseNonEmptyPatch(patchText: string): ParsedPatch[] { const hunks = parsePatch(patchText); if (hunks.length === 0) { @@ -213,47 +57,106 @@ function parseNonEmptyPatch(patchText: string): ParsedPatch[] { return hunks; } -export async function applyPatchDetailed( - cwd: string, - patchText: string, - onProgress?: ApplyPatchProgressCallback, -): Promise { - const hunks = parseNonEmptyPatch(patchText); +type DetailedResultInput = { + readonly summaries: string[]; + readonly appliedFiles: string[]; + readonly failures: ApplyPatchFailure[]; + readonly fuzz: number; + readonly appliedOperations: AppliedPatchOperation[]; +}; + +function createDetailedResult(input: DetailedResultInput): ApplyPatchResult { + const result: ApplyPatchResult = { + summaries: input.summaries, + appliedFiles: input.appliedFiles, + failures: input.failures, + hasPartialSuccess: input.appliedFiles.length > 0 && input.failures.length > 0, + recoveryInstructions: { mustReadFiles: [], mustNotReadFiles: [], failedFiles: [] }, + details: { fuzz: input.fuzz, appliedOperations: input.appliedOperations }, + }; + result.recoveryInstructions = createRecoveryInstructions(result); + return result; +} + +function createCancellationResult(hunk: ParsedPatch, operationIndex: number, error: unknown): ApplyPatchResult { + const failures: ApplyPatchFailure[] = [ + { + operationIndex, + filePath: hunk.filePath, + operation: hunk.type, + message: error instanceof Error ? error.message : String(error), + code: extractErrorCode(error), + }, + ]; + return createDetailedResult({ summaries: [], appliedFiles: [], failures, fuzz: 0, appliedOperations: [] }); +} + +type ApplyPatchDetailedHunksInput = { + readonly cwd: string; + readonly hunks: readonly ParsedPatch[]; + readonly onProgress?: ApplyPatchProgressCallback; + readonly transaction?: PatchTransaction; +}; + +async function applyPatchDetailedHunks(input: ApplyPatchDetailedHunksInput): Promise { const summaries: string[] = []; const appliedFiles: string[] = []; const appliedOperations: AppliedPatchOperation[] = []; const failures: ApplyPatchFailure[] = []; let fuzz = 0; - for (const [operationIndex, hunk] of hunks.entries()) { + for (const [operationIndex, hunk] of input.hunks.entries()) { + if (input.transaction) input.transaction.operationIndex = operationIndex; + input.transaction?.checkCancelled(); try { - const applied = await applySingleHunk(cwd, hunk); + const applied = await applySingleHunk(input.cwd, hunk, input.transaction !== undefined); summaries.push(applied.summary); appliedFiles.push(applied.appliedFile); appliedOperations.push({ operationIndex, preview: applied.preview }); fuzz += applied.fuzz; } catch (error) { + if (input.transaction) input.transaction.checkCancelled(); const message = error instanceof Error ? error.message : String(error); const code = extractErrorCode(error); failures.push({ operationIndex, filePath: hunk.filePath, operation: hunk.type, message, code }); } - await notifyApplyPatchProgress(onProgress, { + await notifyApplyPatchProgress(input.onProgress, { applied: appliedFiles.length, failed: failures.length, - total: hunks.length, + total: input.hunks.length, }); + input.transaction?.checkCancelled(); } - const result: ApplyPatchResult = { - summaries, - appliedFiles, - failures, - hasPartialSuccess: appliedFiles.length > 0 && failures.length > 0, - recoveryInstructions: { mustReadFiles: [], mustNotReadFiles: [], failedFiles: [] }, - details: { fuzz, appliedOperations }, - }; - result.recoveryInstructions = createRecoveryInstructions(result); - return result; + return createDetailedResult({ summaries, appliedFiles, failures, fuzz, appliedOperations }); +} + +export async function applyPatchDetailed( + cwd: string, + patchText: string, + onProgress?: ApplyPatchProgressCallback, + signal?: AbortSignal, +): Promise { + const hunks = parseNonEmptyPatch(patchText); + if (!signal) return applyPatchDetailedHunks({ cwd, hunks, onProgress }); + if (signal.aborted) { + const error = new Error("Operation aborted") as NodeJS.ErrnoException; + error.code = "ABORT_ERR"; + const firstHunk = hunks[0]; + if (!firstHunk) throw error; + return createCancellationResult(firstHunk, 0, error); + } + + return runPatchTransaction({ + filePaths: hunks.flatMap((hunk) => patchMutationPaths(cwd, hunk)), + signal, + run: (transaction) => applyPatchDetailedHunks({ cwd, hunks, onProgress, transaction }), + onAbort: (error, transaction) => { + const failedHunk = hunks[transaction.operationIndex]; + if (!failedHunk) throw error; + return createCancellationResult(failedHunk, transaction.operationIndex, error); + }, + }); } export async function applyPatch(cwd: string, patchText: string): Promise { diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/transaction.ts b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/transaction.ts new file mode 100644 index 0000000000..bf1f946cf9 --- /dev/null +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/transaction.ts @@ -0,0 +1,95 @@ +import { lstat, mkdir, readFile, readlink, rm, symlink, writeFile } from "node:fs/promises"; +import path from "node:path"; +import { withFileMutationQueues } from "../../../tools/file-mutation-queue.ts"; + +export type PatchTransaction = { + operationIndex: number; + checkCancelled: () => void; +}; + +type PatchPathSnapshot = + | { readonly kind: "missing"; readonly filePath: string } + | { readonly kind: "file"; readonly filePath: string; readonly bytes: Uint8Array } + | { readonly kind: "symlink"; readonly filePath: string; readonly target: string }; + +type PatchTransactionInput = { + readonly filePaths: readonly string[]; + readonly signal: AbortSignal; + readonly run: (transaction: PatchTransaction) => Promise; + readonly onAbort: (error: unknown, transaction: PatchTransaction) => T; +}; + +function hasErrorCode(error: unknown, code: string): boolean { + return Boolean(error && typeof error === "object" && "code" in error && error.code === code); +} + +function throwIfCancelled(signal: AbortSignal): void { + if (!signal.aborted) return; + const error = new Error("Operation aborted") as NodeJS.ErrnoException; + error.code = "ABORT_ERR"; + throw error; +} + +async function readSnapshot(filePath: string): Promise { + try { + const stat = await lstat(filePath); + if (stat.isSymbolicLink()) return { kind: "symlink", filePath, target: await readlink(filePath) }; + if (!stat.isFile()) throw new Error(`apply_patch cannot transactionally mutate non-file path: ${filePath}`); + return { kind: "file", filePath, bytes: await readFile(filePath) }; + } catch (error) { + if (hasErrorCode(error, "ENOENT")) return { kind: "missing", filePath }; + throw error; + } +} + +async function restoreSnapshots(snapshots: readonly PatchPathSnapshot[]): Promise { + const failures: string[] = []; + for (const snapshot of [...snapshots].reverse()) { + try { + if (snapshot.kind === "missing") { + await rm(snapshot.filePath, { force: true }); + continue; + } + await mkdir(path.dirname(snapshot.filePath), { recursive: true }); + if (snapshot.kind === "file") { + await writeFile(snapshot.filePath, snapshot.bytes); + continue; + } + await rm(snapshot.filePath, { force: true }); + await symlink(snapshot.target, snapshot.filePath); + } catch (error) { + failures.push(`${snapshot.filePath}: ${error instanceof Error ? error.message : String(error)}`); + } + } + if (failures.length > 0) { + throw new Error(`apply_patch rollback failed; workspace state is uncertain:\n${failures.join("\n")}`); + } +} + +export async function runPatchTransaction(input: PatchTransactionInput): Promise { + const uniquePaths = [...new Set(input.filePaths)]; + return withFileMutationQueues(uniquePaths, async () => { + throwIfCancelled(input.signal); + const snapshots = await Promise.all(uniquePaths.map(readSnapshot)); + throwIfCancelled(input.signal); + const transaction: PatchTransaction = { + operationIndex: 0, + checkCancelled: () => throwIfCancelled(input.signal), + }; + try { + return await input.run(transaction); + } catch (error) { + try { + await restoreSnapshots(snapshots); + } catch (rollbackError) { + throw new Error( + `${rollbackError instanceof Error ? rollbackError.message : String(rollbackError)}\nOriginal abort: ${ + error instanceof Error ? error.message : String(error) + }`, + { cause: error }, + ); + } + return input.onAbort(error, transaction); + } + }); +} From 70782d57a197f4725d741997b245be8c81ba9cd8 Mon Sep 17 00:00:00 2001 From: asdfqwerzxcc Date: Mon, 14 Sep 2026 00:22:20 +0900 Subject: [PATCH 3/5] fix(coding-agent): await apply_patch abort rollback Fixes code-yeongyu/oh-my-openagent#8246. Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai --- .../builtin/gpt-apply-patch/extension.ts | 33 +++- .../builtin/gpt-apply-patch/tool.ts | 37 +++- ...ssue-8246-apply-patch-atomic-abort.test.ts | 162 ++++++++++++++++++ 3 files changed, 220 insertions(+), 12 deletions(-) create mode 100644 packages/coding-agent/test/suite/regressions/issue-8246-apply-patch-atomic-abort.test.ts diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/extension.ts b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/extension.ts index 50a2bd9627..303d0daa9b 100644 --- a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/extension.ts +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/extension.ts @@ -1,5 +1,5 @@ import type { Api, Model } from "@earendil-works/pi-ai"; -import { createApplyPatchTool } from "./tool.ts"; +import { type ApplyPatchExecutions, createApplyPatchTool } from "./tool.ts"; import type { ApplyPatchExtensionAPI, ApplyPatchToolsetState, @@ -109,18 +109,39 @@ export function registerApplyPatchExtension(pi: ApplyPatchExtensionAPI): void { activeVariant?: ApplyPatchToolVariant; wireMode: ApplyPatchWireMode; } = { removedEditToolNames: [], wireMode: "none" }; + const executions: ApplyPatchExecutions = new Map(); const variants = { - freeform: createApplyPatchTool("freeform"), - json: createApplyPatchTool("json"), + freeform: createApplyPatchTool("freeform", executions), + json: createApplyPatchTool("json", executions), } as const; state.activeVariant = "freeform"; pi.registerTool(variants.freeform); registerApplyPatchLazyActivator(pi, state); pi.on("tool_result", async (event) => { - if (event.toolName !== APPLY_PATCH_NAME || event.isError || !hasApplyPatchFailures(event.details)) { - return undefined; + if (event.toolName !== APPLY_PATCH_NAME) return undefined; + try { + const execution = executions.get(event.toolCallId); + const genericAbort = + event.isError && + event.content.length === 1 && + event.content[0]?.type === "text" && + event.content[0].text === "Tool execution aborted"; + if (execution && genericAbort) { + try { + // Do not publish the abort until all in-flight mutations and rollback settle. + await execution; + } catch (error) { + return { + content: [{ type: "text", text: error instanceof Error ? error.message : String(error) }], + isError: true, + }; + } + } + if (event.isError || !hasApplyPatchFailures(event.details)) return undefined; + return { isError: true }; + } finally { + executions.delete(event.toolCallId); } - return { isError: true }; }); pi.on("session_start", async (_event, ctx) => { syncToolset(pi, ctx.model, state, variants); diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/tool.ts b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/tool.ts index 7742f48d73..bd17dad058 100644 --- a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/tool.ts +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/tool.ts @@ -117,7 +117,12 @@ function renderFailureBox( return component; } -export function createApplyPatchTool(variant: "freeform" | "json" = "freeform"): ApplyPatchToolDefinition { +export type ApplyPatchExecutions = Map>; + +export function createApplyPatchTool( + variant: "freeform" | "json" = "freeform", + executions?: ApplyPatchExecutions, +): ApplyPatchToolDefinition { const tool = defineTool({ name: "apply_patch", label: "ApplyPatch", @@ -132,7 +137,7 @@ export function createApplyPatchTool(variant: "freeform" | "json" = "freeform"): async execute( _toolCallId, params, - _signal, + signal, onUpdate, ctx, ): Promise> { @@ -148,10 +153,20 @@ export function createApplyPatchTool(variant: "freeform" | "json" = "freeform"): const pendingUpdate = await createPendingPatchUpdate(ctx.cwd, normalizedParams.input, initialProgress); onUpdate?.({ content: [{ type: "text", text: pendingUpdate.text }], details: pendingUpdate.details }); const preview = pendingUpdate.details?.preview; - const result = await applyPatchDetailed(ctx.cwd, normalizedParams.input, async (progress) => { - const progressUpdate = await createPendingPatchUpdate(ctx.cwd, normalizedParams.input, progress, preview); - onUpdate?.({ content: [{ type: "text", text: progressUpdate.text }], details: progressUpdate.details }); - }); + const result = await applyPatchDetailed( + ctx.cwd, + normalizedParams.input, + async (progress) => { + const progressUpdate = await createPendingPatchUpdate( + ctx.cwd, + normalizedParams.input, + progress, + preview, + ); + onUpdate?.({ content: [{ type: "text", text: progressUpdate.text }], details: progressUpdate.details }); + }, + signal, + ); const resultPreview = appliedPreview(result); const persistedResult = compactApplyPatchResult(result); if (result.failures.length > 0) { @@ -211,6 +226,16 @@ export function createApplyPatchTool(variant: "freeform" | "json" = "freeform"): }, }); + if (executions) { + const execute = tool.execute; + tool.execute = (...args) => { + const execution = execute(...args); + // The abort race may finish first; retain settlement until tool_result consumes it. + executions.set(args[0], execution); + return execution; + }; + } + if (variant === "json") return tool; return Object.assign(tool, { freeform: { type: "grammar", syntax: "lark", definition: APPLY_PATCH_LARK_GRAMMAR } satisfies FreeformToolFormat, diff --git a/packages/coding-agent/test/suite/regressions/issue-8246-apply-patch-atomic-abort.test.ts b/packages/coding-agent/test/suite/regressions/issue-8246-apply-patch-atomic-abort.test.ts new file mode 100644 index 0000000000..413624ed92 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/issue-8246-apply-patch-atomic-abort.test.ts @@ -0,0 +1,162 @@ +import { readFileSync } from "node:fs"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { fauxAssistantMessage, fauxToolCall } from "@earendil-works/pi-ai"; +import { afterEach, describe, expect, it } from "vitest"; +import { registerApplyPatchExtension } from "../../../src/core/extensions/builtin/gpt-apply-patch/extension.ts"; +import { createApplyPatchTool } from "../../../src/core/extensions/builtin/gpt-apply-patch/tool.ts"; +import type { ApplyPatchToolDetails } from "../../../src/core/extensions/builtin/gpt-apply-patch/types.ts"; +import { createHarness, type Harness } from "../harness.ts"; + +const originals = { + "update.txt": Buffer.from("u\r\n"), + "delete.txt": Buffer.from("d\r\n"), + "tail.txt": Buffer.from("t\r\n"), +}; +const patch = `*** Begin Patch +*** Update File: update.txt +@@ +-u ++U +*** Delete File: delete.txt +*** Update File: tail.txt +@@ +-t ++T +*** End Patch`; +const tempDirs: string[] = []; +const harnesses: Harness[] = []; + +async function seedFiles(cwd: string): Promise { + await Promise.all(Object.entries(originals).map(([name, bytes]) => writeFile(path.join(cwd, name), bytes))); +} + +function snapshot(cwd: string): Record { + return Object.fromEntries( + Object.keys(originals).map((name) => { + try { + return [name, readFileSync(path.join(cwd, name))]; + } catch (error) { + if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") { + return [name, null]; + } + throw error; + } + }), + ); +} + +afterEach(async () => { + for (const harness of harnesses.splice(0)) harness.cleanup(); + await Promise.all(tempDirs.splice(0).map((cwd) => rm(cwd, { recursive: true, force: true }))); +}); + +// https://github.com/code-yeongyu/oh-my-openagent/issues/8246 +describe("apply_patch cancellation is byte-exact and atomic", () => { + it("restores CRLF updates and deletions when aborted after two applied operations", async () => { + const cwd = await mkdtemp(path.join(tmpdir(), "senpi-8246-")); + tempDirs.push(cwd); + await seedFiles(cwd); + const controller = new AbortController(); + const tool = createApplyPatchTool(); + const applied: number[] = []; + + await tool.execute( + "abort-after-delete", + { input: patch }, + controller.signal, + (update) => { + const progress = update.details?.progress; + if (!progress) return; + applied.push(progress.applied); + if (progress.applied === 2) controller.abort(); + }, + { cwd } as Parameters[4], + ); + + expect(controller.signal.aborted).toBe(true); + expect(applied).toContain(2); + // Compare raw buffers: restoring LF text is not restoring the original CRLF bytes. + expect(snapshot(cwd)).toEqual(originals); + }); + + it("does not mutate files or emit applied progress for a pre-aborted signal", async () => { + const cwd = await mkdtemp(path.join(tmpdir(), "senpi-8246-pre-aborted-")); + tempDirs.push(cwd); + await seedFiles(cwd); + const controller = new AbortController(); + controller.abort(); + const tool = createApplyPatchTool(); + const applied: number[] = []; + + await tool.execute( + "pre-aborted", + { input: patch }, + controller.signal, + (update) => { + const count = update.details?.progress?.applied; + if (count !== undefined && count > 0) applied.push(count); + }, + { cwd } as Parameters[4], + ); + + expect(snapshot(cwd)).toEqual(originals); + expect(applied).toEqual([]); + }); + + it("restores original bytes before the agent-loop-visible final tool result", async () => { + const executions: ReturnType["execute"]>[] = []; + const harness = await createHarness({ + extensionFactories: [ + (pi) => { + registerApplyPatchExtension({ + ...pi, + registerTool(tool) { + pi.registerTool({ + ...tool, + execute(...args) { + const execution = tool.execute(...args); + executions.push(execution); + return execution; + }, + }); + }, + }); + }, + ], + }); + harnesses.push(harness); + await harness.session.bindExtensions({}); + harness.session.setActiveToolsByName(["apply_patch"]); + await seedFiles(harness.tempDir); + harness.setResponses([ + fauxAssistantMessage(fauxToolCall("apply_patch", { input: patch }), { stopReason: "toolUse" }), + ]); + let aborted = false; + let finalSnapshot: ReturnType | undefined; + // Subscribe before execution and snapshot synchronously at the observable result boundary. + const unsubscribe = harness.session.subscribe((event) => { + if (event.type === "tool_execution_update" && event.toolName === "apply_patch") { + const details = event.partialResult.details as ApplyPatchToolDetails | undefined; + if (details?.progress?.applied === 2 && !aborted) { + aborted = true; + harness.agent.abort(); + } + } + if (event.type === "tool_execution_end" && event.toolName === "apply_patch") { + finalSnapshot = snapshot(harness.tempDir); + } + }); + try { + await harness.session.prompt("Apply the patch"); + expect(aborted).toBe(true); + expect(harness.eventsOfType("tool_execution_end")).toHaveLength(1); + expect(finalSnapshot).toEqual(originals); + } finally { + unsubscribe(); + // The pre-fix agent loop abandons execute on abort; drain it before removing fixtures. + await Promise.all(executions); + } + }, 15_000); +}); From f49493bea450dca2008558f987a9659ee3d9e098 Mon Sep 17 00:00:00 2001 From: asdfqwerzxcc Date: Mon, 14 Sep 2026 00:22:30 +0900 Subject: [PATCH 4/5] docs(coding-agent): record patch abort rollback Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai --- .../builtin/gpt-apply-patch/AGENTS.md | 5 ++- .../builtin/gpt-apply-patch/changes.md | 34 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/AGENTS.md b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/AGENTS.md index a6d8fa26e0..9163e192f5 100644 --- a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/AGENTS.md +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/AGENTS.md @@ -17,7 +17,9 @@ gpt-apply-patch/ ├── patch-diff.ts # Diff/hunk math on top of the npm `diff` package ├── patch-replace.ts # Replace algorithms (anchor matching, seek fallback) ├── seek-sequence.ts # Strict context-line seek with N-line tolerance -├── apply.ts # Apply parsed patch to workspace +├── apply.ts # Parse/orchestrate patch results and cancellation +├── apply-operation.ts # Apply one add/update/delete/move operation +├── transaction.ts # Multi-path byte snapshots and abort rollback ├── workspace.ts # File I/O + path normalization for patches ├── preview.ts # Preview before apply (used by permission-system parser) ├── preview-format.ts # Render preview as TUI nodes (opencode-style diff) @@ -33,6 +35,7 @@ gpt-apply-patch/ |------|------| | Fix a parse error from a real GPT output | `parser.ts` — add a regression test in `test/suite/gpt-apply-patch-extension.test.ts` | | Improve strict-seek tolerance | `seek-sequence.ts` | +| Change cancellation or rollback | `transaction.ts` + `apply.ts` | | Change render | `preview-format.ts` + `streaming-render.ts` | | Add a new file op (e.g. `*** Rename File:`) | `types.ts` + `parser.ts` + `apply.ts` | | Adjust which models opt in | `extension.ts` — `APPLY_PATCH_FREEFORM_APIS` + `gpt-` id prefix in `isOpenAIGptModel()` | diff --git a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/changes.md b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/changes.md index 96c1c7a28b..3e5baba606 100644 --- a/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/changes.md +++ b/packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/changes.md @@ -1,5 +1,39 @@ # changes +## Transactional cancellation rollback (2026-09-13) + +### What changed + +- `packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/apply.ts`, + `apply-operation.ts`, and `transaction.ts` run registered-tool cancellations + under one affected-path reservation and restore every original file or symlink + from a byte-exact snapshot before the result can settle. +- `packages/coding-agent/src/core/extensions/builtin/gpt-apply-patch/tool.ts` and + `extension.ts` track each registered execution until the awaited `tool_result` + hook observes settlement, so the agent loop's generic abort race cannot publish + a final result while rollback is still running. +- Direct `applyPatchDetailed()` calls without an `AbortSignal` retain the existing + partial-failure contract. + +### Why + +- A cancelled multi-file patch could return `Tool execution aborted` after earlier + updates and deletions had already changed the workspace. The abandoned execution + then continued applying later operations, and CRLF source bytes were not recoverable + from normalized text. + +### Why an extension could not handle it + +- The builtin owns patch parsing, mutation ordering, previews, and the final + `tool_result` hook. Rollback must hold the same core mutation queues as edit and + write operations; an external extension only sees the result after those writes. + +### Expected merge conflict zones + +- MEDIUM: operation orchestration in `apply.ts` and the execution wrapper in `tool.ts`. +- LOW: the instance-local settlement registry in `extension.ts`. +- NONE: new fork-owned `apply-operation.ts` and `transaction.ts`. + ## Binary-safe patch previews (2026-08-05) ### What changed From 18838161f9c2025da8e9722bcd259456ee567195 Mon Sep 17 00:00:00 2001 From: asdfqwerzxcc Date: Mon, 14 Sep 2026 00:22:38 +0900 Subject: [PATCH 5/5] docs(coding-agent): note atomic patch aborts Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai --- packages/coding-agent/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 948d69b084..93af8834d6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -50,6 +50,8 @@ ### Fixed +- Fixed cancelled multi-file `apply_patch` calls returning before byte-exact rollback completed, which could leave updates and deletions applied behind a generic abort result ([oh-my-openagent #8246](https://github.com/code-yeongyu/oh-my-openagent/issues/8246)). + - Fixed the goal monitor parking on the ask-user idle-timeout setting instead of the earliest pending question deadline, so a shorter request no longer waits for a longer one; typing in an answer now extends that park without adding continuation prompts ([#1645](https://github.com/code-yeongyu/senpi/issues/1645)). - Fixed shared RPC hosts expiring an old idle window after a short readiness connection, which could remove the Windows named pipe before the client attached (part of #1290).