From c8775bb09d683d00261851013fdb6b03e6519da4 Mon Sep 17 00:00:00 2001 From: kevin9327 <5299031+kevin9327@users.noreply.github.com> Date: Sun, 20 Sep 2026 20:27:31 +0900 Subject: [PATCH 1/5] fix(archive): copy without staging when Windows EPERM blocks rename fs.rename of a non-leaf change directory fails with EPERM on Windows when a watcher holds a handle. The fallback required a staging rename of the same directory, so it never ran: specs were rolled back after printing success, and a newly created capability was left as an empty folder git cannot see. Copy from the original source when staging also fails with EPERM/EXDEV, and prune empty capability dirs on rollback. Closes #1895 AI-assisted (Grok) --- .../archive-eperm-copy-without-staging.md | 7 + src/core/archive.ts | 167 +++++++++++------- src/core/specs-apply.ts | 2 +- test/core/archive.test.ts | 114 +++++++++++- 4 files changed, 220 insertions(+), 70 deletions(-) create mode 100644 .changeset/archive-eperm-copy-without-staging.md diff --git a/.changeset/archive-eperm-copy-without-staging.md b/.changeset/archive-eperm-copy-without-staging.md new file mode 100644 index 0000000000..dc0ed7fd5e --- /dev/null +++ b/.changeset/archive-eperm-copy-without-staging.md @@ -0,0 +1,7 @@ +--- +"@fission-ai/openspec": patch +--- + +### Bug Fixes + +- **Archive** — When Windows `EPERM` blocks renaming a change directory that still has children, copy from the original source instead of requiring a staging rename that fails the same way. That lets archive finish instead of rolling back the spec write and leaving an empty capability directory git cannot see. A staging failure that is not `EPERM`/`EXDEV` still leaves the source untouched. Rollback of a newly created spec now also prunes the empty capability directory it created. diff --git a/src/core/archive.ts b/src/core/archive.ts index 62a12ef728..dbf9e53373 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -21,6 +21,7 @@ import { writeUpdatedSpec, retireSpec, finalizeRetiredSpec, + pruneEmptyDirs, type SpecUpdate, } from './specs-apply.js'; import { discoverSpecFiles, findUnreadDeltaFiles, hasAnyFileUnder } from '../utils/spec-discovery.js'; @@ -470,14 +471,94 @@ async function assertCopiedDirectoryUnchanged( /** * Move a directory from src to dest. On Windows, fs.rename() can fail with - * EPERM, and cross-device moves fail with EXDEV. When the source can first be - * renamed to a private sibling, fall back to a verified copy-then-remove. A - * source that cannot be staged is left untouched rather than copied and deleted - * through a path another process may still be editing. + * EPERM, and cross-device moves fail with EXDEV. Prefer renaming the source + * to a private sibling first, then copy-then-remove. When that staging rename + * also fails with EPERM/EXDEV — the usual Windows case for a directory that + * still has children, because a watcher holds a directory-enumeration handle — + * copy from the original source instead. Fingerprints still abort if the tree + * changes mid-copy. A staging failure that is not EPERM/EXDEV still leaves + * the source untouched rather than copying through a path we could not claim. */ class MoveDestinationRetainedError extends Error {} class RetirementBackupsRetainedError extends Error {} +function isFallbackRenameCode(code: string | undefined): boolean { + return code === 'EPERM' || code === 'EXDEV'; +} + +async function copyThenRemoveDirectory( + source: string, + dest: string, + options: { + verifyCopiedDestination?: (copiedSource: string) => Promise; + }, + restoreSource?: () => Promise +): Promise { + let destIsOurs = false; + let sourceFingerprint: string; + try { + sourceFingerprint = await fingerprintDirectoryContents(source); + await fs.mkdir(dest, { mode: 0o700 }); + destIsOurs = true; + await copyDirContents(source, dest); + await options.verifyCopiedDestination?.(source); + await assertCopiedDirectoryUnchanged(source, dest, sourceFingerprint); + } catch (copyError) { + if (destIsOurs) { + await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined); + } + if (restoreSource) { + try { + await restoreSource(); + } catch (restoreError) { + throw new Error( + `${copyError instanceof Error ? copyError.message : String(copyError)} ` + + `Could not restore the staged source at ${source} ` + + `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).` + ); + } + } + if ((copyError as NodeJS.ErrnoException).code === 'EEXIST') { + throw new ArchiveBlockedError( + 'archive_target_exists', + `Archive '${path.basename(dest)}' already exists.` + ); + } + throw copyError; + } + try { + await options.verifyCopiedDestination?.(source); + await assertCopiedDirectoryUnchanged(source, dest, sourceFingerprint); + } catch (verificationError) { + await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined); + if (restoreSource) { + try { + await restoreSource(); + } catch (restoreError) { + throw new Error( + `${verificationError instanceof Error ? verificationError.message : String(verificationError)} ` + + `Could not restore the staged source at ${source} ` + + `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).` + ); + } + } + throw verificationError; + } + try { + await fs.rm(source, { recursive: true, force: true }); + } catch (cleanupError) { + // Recursive removal may already have deleted part of the source. The + // destination is now the only complete copy, so never erase it while + // trying to make this failed move look atomic. + throw new MoveDestinationRetainedError( + `Copied ${source} to ${dest}, but could not remove the source at ` + + `${source} completely ` + + `(${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}). ` + + 'The complete destination was retained for recovery.' + ); + } +} + async function moveDirectory( src: string, dest: string, @@ -497,76 +578,25 @@ async function moveDirectory( `Archive '${path.basename(dest)}' already exists.` ); } - if (code === 'EPERM' || code === 'EXDEV') { + if (isFallbackRenameCode(code)) { const stagedSource = path.join(path.dirname(src), `.openspec-move-${randomUUID()}`); try { await fs.rename(src, stagedSource); } catch (stageError) { + const stageCode = (stageError as NodeJS.ErrnoException)?.code; + if (isFallbackRenameCode(stageCode)) { + await copyThenRemoveDirectory(src, dest, options); + return; + } throw new Error( `Could not safely stage ${src} before the fallback archive copy ` + `(${stageError instanceof Error ? stageError.message : String(stageError)}). ` + 'No fallback copy was attempted.' ); } - let destIsOurs = false; - let stagedFingerprint: string; - try { - stagedFingerprint = await fingerprintDirectoryContents(stagedSource); - await fs.mkdir(dest, { mode: 0o700 }); - destIsOurs = true; - await copyDirContents(stagedSource, dest); - await options.verifyCopiedDestination?.(stagedSource); - await assertCopiedDirectoryUnchanged(stagedSource, dest, stagedFingerprint); - } catch (copyError) { - if (destIsOurs) { - await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined); - } - try { - await fs.rename(stagedSource, src); - } catch (restoreError) { - throw new Error( - `${copyError instanceof Error ? copyError.message : String(copyError)} ` + - `Could not restore the staged source at ${stagedSource} ` + - `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).` - ); - } - if ((copyError as NodeJS.ErrnoException).code === 'EEXIST') { - throw new ArchiveBlockedError( - 'archive_target_exists', - `Archive '${path.basename(dest)}' already exists.` - ); - } - throw copyError; - } - try { - await options.verifyCopiedDestination?.(stagedSource); - await assertCopiedDirectoryUnchanged(stagedSource, dest, stagedFingerprint); - } catch (verificationError) { - await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined); - try { - await fs.rename(stagedSource, src); - } catch (restoreError) { - throw new Error( - `${verificationError instanceof Error ? verificationError.message : String(verificationError)} ` + - `Could not restore the staged source at ${stagedSource} ` + - `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).` - ); - } - throw verificationError; - } - try { - await fs.rm(stagedSource, { recursive: true, force: true }); - } catch (cleanupError) { - // Recursive removal may already have deleted part of the source. The - // destination is now the only complete copy, so never erase it while - // trying to make this failed move look atomic. - throw new MoveDestinationRetainedError( - `Copied ${src} to ${dest}, but could not remove the staged source at ` + - `${stagedSource} completely ` + - `(${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}). ` + - 'The complete destination was retained for recovery.' - ); - } + await copyThenRemoveDirectory(stagedSource, dest, options, async () => { + await fs.rename(stagedSource, src); + }); } else { throw err; } @@ -917,7 +947,10 @@ async function captureSpecSnapshots(mutations: SpecMutation[]): Promise { +async function restoreSpecSnapshots( + snapshots: SpecSnapshot[], + mainSpecsDir: string +): Promise { const errors: Error[] = []; for (const snapshot of [...snapshots].reverse()) { try { @@ -1008,6 +1041,7 @@ async function restoreSpecSnapshots(snapshots: SpecSnapshot[]): Promise { if (!snapshot.existed) { await fs.rm(snapshot.target, { force: true }); + await pruneEmptyDirs(path.dirname(snapshot.target), mainSpecsDir); continue; } if (snapshot.symlink !== undefined) { @@ -2028,7 +2062,8 @@ export class ArchiveCommand { const rollbackErrors: Error[] = []; try { await restoreSpecSnapshots( - specSnapshots.filter(({ target }) => mutationAttempts.has(target)) + specSnapshots.filter(({ target }) => mutationAttempts.has(target)), + mainSpecsDir ); } catch (rollbackError) { rollbackErrors.push( diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 1ca2fba52d..f612a03d2b 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -1190,7 +1190,7 @@ async function isInsideRealDir(realPath: string, dir: string): Promise * needs fd-relative syscalls Node does not expose, and it requires local write * access to `openspec/specs` during an archive. */ -async function pruneEmptyDirs(startDir: string, boundaryDir: string): Promise { +export async function pruneEmptyDirs(startDir: string, boundaryDir: string): Promise { let boundary: string; try { boundary = await fs.realpath(boundaryDir); diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index ff34e21a66..4777f360af 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -6769,11 +6769,14 @@ The system SHALL provide a replacement behavior. const realRename = fs.rename.bind(fs); onTestFinished(() => vi.restoreAllMocks()); vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + const src = String(source); + const dest = String(destination); if ( - String(source).endsWith( - `${path.sep}openspec${path.sep}changes${path.sep}${changeName}` - ) + src.endsWith(`${path.sep}openspec${path.sep}changes${path.sep}${changeName}`) ) { + if (dest.includes(`${path.sep}.openspec-move-`)) { + throw Object.assign(new Error('staging denied'), { code: 'EACCES' }); + } throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); } return realRename(source, destination); @@ -6807,6 +6810,111 @@ The system SHALL provide a replacement behavior. ).toBe(false); }); + it('does not leave an empty capability directory when a create is rolled back', async () => { + const changeName = 'eperm-create-rollback-prunes'; + const changeDir = await createChange( + changeName, + 'write-feedback', + `## ADDED Requirements + +### Requirement: Write feedback is captured +The system SHALL capture write feedback. + +#### Scenario: Feedback is stored +- **WHEN** write feedback arrives +- **THEN** it is stored +` + ); + const capabilityDir = path.join(tempDir, 'openspec', 'specs', 'write-feedback'); + + const realRename = fs.rename.bind(fs); + onTestFinished(() => vi.restoreAllMocks()); + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + const src = String(source); + const dest = String(destination); + if ( + src.endsWith(`${path.sep}openspec${path.sep}changes${path.sep}${changeName}`) + ) { + if (dest.includes(`${path.sep}.openspec-move-`)) { + throw Object.assign(new Error('staging denied'), { code: 'EACCES' }); + } + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + return realRename(source, destination); + }); + + await expect( + archiveCommand.execute(changeName, { yes: true }) + ).rejects.toThrow(/Could not safely stage/); + + await expect(fs.access(path.join(capabilityDir, 'spec.md'))).rejects.toThrow(); + await expect(fs.access(capabilityDir)).rejects.toThrow(); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + }); + + it('archives via copy when EPERM prevents both dest rename and staging', async () => { + const changeName = 'eperm-copy-without-staging'; + const changeDir = await createChange( + changeName, + 'write-feedback', + `## ADDED Requirements + +### Requirement: Write feedback is captured +The system SHALL capture write feedback. + +#### Scenario: Feedback is stored +- **WHEN** write feedback arrives +- **THEN** it is stored +` + ); + const target = path.join( + tempDir, + 'openspec', + 'specs', + 'write-feedback', + 'spec.md' + ); + + const realRename = fs.rename.bind(fs); + onTestFinished(() => vi.restoreAllMocks()); + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + if ( + String(source).endsWith( + `${path.sep}openspec${path.sep}changes${path.sep}${changeName}` + ) + ) { + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + return realRename(source, destination); + }); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(changeDir)).rejects.toThrow(); + await expect(fs.readFile(target, 'utf-8')).resolves.toContain( + '### Requirement: Write feedback is captured' + ); + await expect( + fs.access( + path.join( + tempDir, + 'openspec', + 'changes', + 'archive', + `${formatLocalDate()}-${changeName}`, + 'specs', + 'write-feedback', + 'spec.md' + ) + ) + ).resolves.not.toThrow(); + expect( + (await fs.readdir(path.dirname(path.dirname(changeDir)))).some((entry) => + entry.startsWith('.openspec-move-') + ) + ).toBe(false); + }); + it('keeps applied specs when fallback retains a complete archive copy', async () => { const changeName = 'retained-copy-keeps-specs'; const changeDir = await createChange( From 64e42473bbc9d9f1718c35fe829c190bbc0485cb Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 22 Sep 2026 15:26:36 -0500 Subject: [PATCH 2/5] fix(archive): bound the unstaged cleanup to what it verified Addresses both review findings on the copy fallback. The staging rename was what claimed the source before it was deleted. Falling back without it means copy-then-remove now runs against the live change directory, which the archive claim does not cover, and a recursive remove deletes whatever is there at that moment - including a file written after the final fingerprint, which never reached the destination. Cleanup now removes a named set: the entries listed after the last verification, deepest first. A later arrival is not in that set, so it is never deleted, and the rmdir of its parent fails with ENOTEMPTY, which the caller already reports as a retained destination. The move fails loudly rather than completing with data missing. Rollback of a created spec pruned the capability directory unconditionally, which also removed one the user already had, along with its mode and ACLs. The snapshot now records whether that parent existed, and only a directory this write created is pruned. Co-Authored-By: Claude Opus 5 --- .../archive-eperm-copy-without-staging.md | 6 +- src/core/archive.ts | 88 ++++++++++++- test/core/archive.test.ts | 123 ++++++++++++++++++ 3 files changed, 211 insertions(+), 6 deletions(-) diff --git a/.changeset/archive-eperm-copy-without-staging.md b/.changeset/archive-eperm-copy-without-staging.md index dc0ed7fd5e..f758e56b3c 100644 --- a/.changeset/archive-eperm-copy-without-staging.md +++ b/.changeset/archive-eperm-copy-without-staging.md @@ -4,4 +4,8 @@ ### Bug Fixes -- **Archive** — When Windows `EPERM` blocks renaming a change directory that still has children, copy from the original source instead of requiring a staging rename that fails the same way. That lets archive finish instead of rolling back the spec write and leaving an empty capability directory git cannot see. A staging failure that is not `EPERM`/`EXDEV` still leaves the source untouched. Rollback of a newly created spec now also prunes the empty capability directory it created. +- **Archive** — When Windows `EPERM` blocks renaming a change directory that still has children, copy from the original source instead of requiring a staging rename that fails the same way. That lets archive finish instead of rolling back the spec write and leaving an empty capability directory git cannot see. A staging failure that is not `EPERM`/`EXDEV` still leaves the source untouched. + + The source of that unstaged copy is still the live change directory, which the archive claim does not cover, so cleanup removes only the entries it copied and verified rather than whatever is present when it runs. A file written in that window is left alone and the complete destination is retained for recovery, instead of being deleted without ever reaching the archive. + + Rollback of a newly created spec now also prunes the capability directory it created — and only that one. An empty capability directory that was already there is left in place with its own permissions. diff --git a/src/core/archive.ts b/src/core/archive.ts index dbf9e53373..3d46e3e002 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -486,6 +486,59 @@ function isFallbackRenameCode(code: string | undefined): boolean { return code === 'EPERM' || code === 'EXDEV'; } +/** + * Every entry under `root`, deepest first, as paths relative to it. + * + * The listing is what bounds the removal below. Anything that appears after it + * is simply not in the set, so it cannot be deleted by the cleanup. + */ +async function listTreeEntriesDeepestFirst( + root: string +): Promise<{ relative: string; isDirectory: boolean }[]> { + const entries: { relative: string; isDirectory: boolean }[] = []; + const visit = async (dir: string, relativeDir: string): Promise => { + for (const entry of await fs.readdir(dir, { withFileTypes: true })) { + const relative = relativeDir === '' ? entry.name : path.join(relativeDir, entry.name); + if (entry.isDirectory()) { + await visit(path.join(dir, entry.name), relative); + entries.push({ relative, isDirectory: true }); + } else { + entries.push({ relative, isDirectory: false }); + } + } + }; + await visit(root, ''); + return entries; +} + +/** + * Remove exactly the entries that were copied and verified, deepest first. + * + * The move is only safe to finish by deleting the source, and the source of the + * unstaged fallback is still the live change directory: the archive claim + * covers the destination, not it. A recursive remove would delete whatever is + * there at that moment, including a file a concurrent writer added after the + * final fingerprint - data that never reached the destination. + * + * Removing a named set instead means a late arrival is never in it. It is left + * on disk, and the `rmdir` of its parent fails with ENOTEMPTY, which the caller + * reports as a retained destination. The move does not complete silently. + */ +async function removeVerifiedTree( + root: string, + entries: { relative: string; isDirectory: boolean }[] +): Promise { + for (const entry of entries) { + const target = path.join(root, entry.relative); + if (entry.isDirectory) { + await fs.rmdir(target); + } else { + await fs.rm(target, { force: true }); + } + } + await fs.rmdir(root); +} + async function copyThenRemoveDirectory( source: string, dest: string, @@ -545,11 +598,14 @@ async function copyThenRemoveDirectory( throw verificationError; } try { - await fs.rm(source, { recursive: true, force: true }); + // Bounded by a listing taken after the last verification, so only entries + // that were copied can be deleted. See removeVerifiedTree. + await removeVerifiedTree(source, await listTreeEntriesDeepestFirst(source)); } catch (cleanupError) { - // Recursive removal may already have deleted part of the source. The - // destination is now the only complete copy, so never erase it while - // trying to make this failed move look atomic. + // Removal may already have deleted part of the source, or stopped on an + // entry that appeared after verification. The destination is now the only + // complete copy, so never erase it while trying to make this failed move + // look atomic. throw new MoveDestinationRetainedError( `Copied ${source} to ${dest}, but could not remove the source at ` + `${source} completely ` + @@ -703,6 +759,12 @@ async function claimArchiveDestination( interface SpecSnapshot { target: string; existed: boolean; + /** + * Whether the target's parent directory existed before the mutation. An + * empty capability directory the user already had is not ours to delete on + * rollback, and removing it would drop its permissions and ACLs too. + */ + parentExisted?: boolean; outcome: 'write' | 'retire'; expectedContent?: Buffer; content?: Buffer; @@ -892,6 +954,17 @@ async function assertDistinctMutationTargets(mutations: SpecMutation[]): Promise } } +/** Whether `dir` is present, without distinguishing why it is not. */ +async function directoryExists(dir: string): Promise { + try { + await fs.lstat(dir); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; + throw error; + } +} + async function captureSpecSnapshots(mutations: SpecMutation[]): Promise { return Promise.all( mutations.map(async ({ update, outcome, rebuilt }) => { @@ -938,6 +1011,7 @@ async function captureSpecSnapshots(mutations: SpecMutation[]): Promise { + // The unstaged fallback copies from the live change directory: the + // archive claim covers the destination, not the source. Cleanup must + // therefore delete only the entries it verified, never whatever happens + // to be there when it runs. + const changeName = 'eperm-late-write-preserved'; + const changeDir = await createChange( + changeName, + 'write-feedback', + `## ADDED Requirements + +### Requirement: Write feedback is captured +The system SHALL capture write feedback. + +#### Scenario: Feedback is stored +- **WHEN** write feedback arrives +- **THEN** it is stored +` + ); + const lateFile = path.join(changeDir, 'late-arrival.md'); + + const realRename = fs.rename.bind(fs); + const realRmdir = fs.rmdir.bind(fs); + // archive works in realpaths, which on macOS carry a /private prefix the + // temp dir does not. + const resolvedChangeDir = await fs.realpath(changeDir); + onTestFinished(() => vi.restoreAllMocks()); + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + if ( + String(source).endsWith( + `${path.sep}openspec${path.sep}changes${path.sep}${changeName}` + ) + ) { + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + return realRename(source, destination); + }); + + // Land the write in the window the listing has already closed: the + // verified entries are being removed, so the copy and both fingerprint + // checks are already behind us. rmdir of a subdirectory only happens + // inside that removal. + let arrived = false; + vi.spyOn(fs, 'rmdir').mockImplementation(async (target, options) => { + const t = String(target); + if ( + !arrived && + (t === resolvedChangeDir || t.startsWith(resolvedChangeDir + path.sep)) + ) { + arrived = true; + await fs.writeFile(lateFile, 'Written while the move was finishing.\n'); + } + return realRmdir(target, options); + }); + + await expect(archiveCommand.execute(changeName, { yes: true })).rejects.toThrow( + /could not remove the source|retained for recovery/i + ); + + expect(arrived).toBe(true); + // The late write survives, and the complete copy is still there. + await expect(fs.readFile(lateFile, 'utf-8')).resolves.toContain( + 'Written while the move was finishing.' + ); + await expect( + fs.access( + path.join( + tempDir, + 'openspec', + 'changes', + 'archive', + `${formatLocalDate()}-${changeName}`, + 'specs', + 'write-feedback', + 'spec.md' + ) + ) + ).resolves.not.toThrow(); + }); + + it('keeps a capability directory that already existed when a create is rolled back', async () => { + // Pruning is only ever taking back a directory this write created. One + // the user already had carries their own mode and ACLs. + const changeName = 'eperm-create-rollback-keeps-existing-dir'; + await createChange( + changeName, + 'write-feedback', + `## ADDED Requirements + +### Requirement: Write feedback is captured +The system SHALL capture write feedback. + +#### Scenario: Feedback is stored +- **WHEN** write feedback arrives +- **THEN** it is stored +` + ); + const capabilityDir = path.join(tempDir, 'openspec', 'specs', 'write-feedback'); + await fs.mkdir(capabilityDir, { recursive: true }); + + const realRename = fs.rename.bind(fs); + onTestFinished(() => vi.restoreAllMocks()); + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + const src = String(source); + const dest = String(destination); + if (src.endsWith(`${path.sep}openspec${path.sep}changes${path.sep}${changeName}`)) { + if (dest.includes(`${path.sep}.openspec-move-`)) { + throw Object.assign(new Error('staging denied'), { code: 'EACCES' }); + } + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + return realRename(source, destination); + }); + + await expect(archiveCommand.execute(changeName, { yes: true })).rejects.toThrow( + /Could not safely stage/ + ); + + // The spec the rollback undid is gone; the directory the user had stays. + await expect(fs.access(path.join(capabilityDir, 'spec.md'))).rejects.toThrow(); + await expect(fs.access(capabilityDir)).resolves.not.toThrow(); + }); + it('archives via copy when EPERM prevents both dest rename and staging', async () => { const changeName = 'eperm-copy-without-staging'; const changeDir = await createChange( From 3b385da12224b84d68eafcd2b5d2ffc35cd31819 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 22 Sep 2026 16:06:35 -0500 Subject: [PATCH 3/5] fix(archive): close the listing window and the nested prune boundary Both follow-ups from CodeRabbit's second pass, and both are right. The removal listing was taken after the final fingerprint, which left the window it was meant to close: a file arriving between the fingerprint and the listing landed in the set and was deleted, having never reached the destination. Listing before the verification makes the two orderings exhaustive - an arrival either changes the fingerprint and aborts the move, or is absent from the set and survives. The one case this cannot cover, an edit to an already-listed file, is now stated in the comment. `parentExisted` only described the target's direct parent, so a nested capability id whose intermediate directory already existed still lost it: the prune walked to the specs root. The snapshot now records the deepest pre-existing ancestor and passes it as the prune boundary, which pruneEmptyDirs never removes. That one mechanism covers the flat case too. Co-Authored-By: Claude Opus 5 --- src/core/archive.ts | 75 +++++++++++++++++++++++++++------------ test/core/archive.test.ts | 45 +++++++++++++++++++++++ 2 files changed, 98 insertions(+), 22 deletions(-) diff --git a/src/core/archive.ts b/src/core/archive.ts index 3d46e3e002..fde3ee2b48 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -523,6 +523,13 @@ async function listTreeEntriesDeepestFirst( * Removing a named set instead means a late arrival is never in it. It is left * on disk, and the `rmdir` of its parent fails with ENOTEMPTY, which the caller * reports as a retained destination. The move does not complete silently. + * + * `entries` must be listed before the final fingerprint, so that an arrival is + * either caught by that fingerprint or absent from the set. What this cannot + * cover is an edit to a file that is already in the set: the copy holds the + * content as of the fingerprint, and the newer bytes go with the source. That + * window is the one the staging rename closes, and is why staging is still + * preferred whenever the rename is permitted at all. */ async function removeVerifiedTree( root: string, @@ -579,7 +586,13 @@ async function copyThenRemoveDirectory( } throw copyError; } + let verifiedEntries: { relative: string; isDirectory: boolean }[]; try { + // Listed before the verification, not after it. A file that arrives before + // the fingerprint changes it and aborts the move; one that arrives after is + // not in this set. Listing afterwards would leave a window in which an + // arrival is both unverified and deletable. + verifiedEntries = await listTreeEntriesDeepestFirst(source); await options.verifyCopiedDestination?.(source); await assertCopiedDirectoryUnchanged(source, dest, sourceFingerprint); } catch (verificationError) { @@ -598,9 +611,7 @@ async function copyThenRemoveDirectory( throw verificationError; } try { - // Bounded by a listing taken after the last verification, so only entries - // that were copied can be deleted. See removeVerifiedTree. - await removeVerifiedTree(source, await listTreeEntriesDeepestFirst(source)); + await removeVerifiedTree(source, verifiedEntries); } catch (cleanupError) { // Removal may already have deleted part of the source, or stopped on an // entry that appeared after verification. The destination is now the only @@ -760,11 +771,13 @@ interface SpecSnapshot { target: string; existed: boolean; /** - * Whether the target's parent directory existed before the mutation. An - * empty capability directory the user already had is not ours to delete on - * rollback, and removing it would drop its permissions and ACLs too. + * The deepest directory at or above the target's parent that already existed + * before the mutation. Rollback prunes up to but never past it, so a + * capability directory the user already had keeps its permissions and ACLs - + * including an intermediate one under a nested capability id, where only the + * leaf was created by this write. */ - parentExisted?: boolean; + pruneBoundary?: string; outcome: 'write' | 'retire'; expectedContent?: Buffer; content?: Buffer; @@ -954,18 +967,31 @@ async function assertDistinctMutationTargets(mutations: SpecMutation[]): Promise } } -/** Whether `dir` is present, without distinguishing why it is not. */ -async function directoryExists(dir: string): Promise { - try { - await fs.lstat(dir); - return true; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; - throw error; +/** + * The deepest directory at or above `dir` that exists, never going above + * `boundaryDir`. Used as the floor for a rollback prune: everything below it + * was created by the write being undone, and it was not. + */ +async function deepestExistingAncestor(dir: string, boundaryDir: string): Promise { + let current = dir; + for (;;) { + if (current === boundaryDir || !current.startsWith(boundaryDir + path.sep)) { + return boundaryDir; + } + try { + await fs.lstat(current); + return current; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + current = path.dirname(current); } } -async function captureSpecSnapshots(mutations: SpecMutation[]): Promise { +async function captureSpecSnapshots( + mutations: SpecMutation[], + mainSpecsDir: string +): Promise { return Promise.all( mutations.map(async ({ update, outcome, rebuilt }) => { try { @@ -1011,7 +1037,10 @@ async function captureSpecSnapshots(mutations: SpecMutation[]): Promise [snapshot.target, snapshot]) ); diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 44f787b3a5..3f136b3044 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -6975,6 +6975,51 @@ The system SHALL capture write feedback. await expect(fs.access(capabilityDir)).resolves.not.toThrow(); }); + it('keeps a pre-existing ancestor when a nested capability create is rolled back', async () => { + // `platform/` already existed and `platform/session-layout/` did not. + // Only the leaf is ours to take back; walking up to the specs root would + // delete the user's directory too. + const changeName = 'eperm-nested-rollback-keeps-ancestor'; + await createChange( + changeName, + 'platform/session-layout', + `## ADDED Requirements + +### Requirement: Session layout is described +The system SHALL describe the session layout. + +#### Scenario: Layout is read +- **WHEN** the layout is requested +- **THEN** it is returned +` + ); + const ancestorDir = path.join(tempDir, 'openspec', 'specs', 'platform'); + const capabilityDir = path.join(ancestorDir, 'session-layout'); + await fs.mkdir(ancestorDir, { recursive: true }); + + const realRename = fs.rename.bind(fs); + onTestFinished(() => vi.restoreAllMocks()); + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + const src = String(source); + const dest = String(destination); + if (src.endsWith(`${path.sep}openspec${path.sep}changes${path.sep}${changeName}`)) { + if (dest.includes(`${path.sep}.openspec-move-`)) { + throw Object.assign(new Error('staging denied'), { code: 'EACCES' }); + } + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + return realRename(source, destination); + }); + + await expect(archiveCommand.execute(changeName, { yes: true })).rejects.toThrow( + /Could not safely stage/ + ); + + // The leaf this write created is gone; the ancestor the user had stays. + await expect(fs.access(capabilityDir)).rejects.toThrow(); + await expect(fs.access(ancestorDir)).resolves.not.toThrow(); + }); + it('archives via copy when EPERM prevents both dest rename and staging', async () => { const changeName = 'eperm-copy-without-staging'; const changeDir = await createChange( From 5d830f2bc92dac1e268ce90746abe675fa2e133a Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 23 Sep 2026 08:47:28 -0500 Subject: [PATCH 4/5] fix(archive): claim each entry before removing it in the unstaged fallback An editor could rewrite an already-verified file between the final fingerprint and its removal. The destination held the older bytes, the newer ones were deleted with the source, and archive reported success. Cleanup now renames each entry to a private claim name before reading it. rename is atomic, so a rewrite that lands after the claim creates a new file at the original path, which is not in the verified set, is never deleted, and makes the parent rmdir fail. A rewrite that lands first is caught by comparing the claimed entry against the copy, which puts the file back and abandons the move with both trees intact. Symlinks are compared by their target rather than by reading them, since a link to a directory is not a directory entry and reading one is EISDIR. Co-Authored-By: Claude Opus 5 --- .../archive-eperm-copy-without-staging.md | 2 + src/core/archive.ts | 94 ++++++++++++++----- test/core/archive.test.ts | 73 ++++++++++++++ 3 files changed, 148 insertions(+), 21 deletions(-) diff --git a/.changeset/archive-eperm-copy-without-staging.md b/.changeset/archive-eperm-copy-without-staging.md index f758e56b3c..88f2d6483a 100644 --- a/.changeset/archive-eperm-copy-without-staging.md +++ b/.changeset/archive-eperm-copy-without-staging.md @@ -8,4 +8,6 @@ The source of that unstaged copy is still the live change directory, which the archive claim does not cover, so cleanup removes only the entries it copied and verified rather than whatever is present when it runs. A file written in that window is left alone and the complete destination is retained for recovery, instead of being deleted without ever reaching the archive. + An edit to a file that was already verified is covered too. Cleanup claims each entry with an atomic rename before reading it, then compares what it claimed against the copy. A rewrite that lands first is caught by that comparison and the file is put back; one that lands after creates a new file at the original path, which is never deleted. Either way the newer bytes stay on disk and archive reports the move as incomplete rather than succeeding with the older copy. + Rollback of a newly created spec now also prunes the capability directory it created — and only that one. An empty capability directory that was already there is left in place with its own permissions. diff --git a/src/core/archive.ts b/src/core/archive.ts index fde3ee2b48..d757766006 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -379,6 +379,17 @@ async function copyDirContents(src: string, dest: string): Promise { await fs.chmod(dest, sourceStat.mode & 0o7777); } +type TreeEntry = { relative: string; kind: 'directory' | 'file' | 'symlink' }; + +/** SHA-256 of one file's bytes, streamed so a large file is never held whole. */ +async function fingerprintFileContents(filePath: string): Promise { + const fileHash = createHash('sha256'); + for await (const chunk of createReadStream(filePath)) { + fileHash.update(chunk); + } + return fileHash.digest(); +} + async function fingerprintDirectoryContents(root: string): Promise { const hash = createHash('sha256'); const updateHashField = (label: string, value: string | Buffer): void => { @@ -391,13 +402,7 @@ async function fingerprintDirectoryContents(root: string): Promise { hash.update(labelBuffer); hash.update(valueBuffer); }; - const fingerprintFile = async (filePath: string): Promise => { - const fileHash = createHash('sha256'); - for await (const chunk of createReadStream(filePath)) { - fileHash.update(chunk); - } - return fileHash.digest(); - }; + const fingerprintFile = fingerprintFileContents; const visit = async (dir: string, relativeDir: string): Promise => { const before = await fs.lstat(dir, { bigint: true }); @@ -494,16 +499,19 @@ function isFallbackRenameCode(code: string | undefined): boolean { */ async function listTreeEntriesDeepestFirst( root: string -): Promise<{ relative: string; isDirectory: boolean }[]> { - const entries: { relative: string; isDirectory: boolean }[] = []; +): Promise { + const entries: TreeEntry[] = []; const visit = async (dir: string, relativeDir: string): Promise => { for (const entry of await fs.readdir(dir, { withFileTypes: true })) { const relative = relativeDir === '' ? entry.name : path.join(relativeDir, entry.name); if (entry.isDirectory()) { await visit(path.join(dir, entry.name), relative); - entries.push({ relative, isDirectory: true }); + entries.push({ relative, kind: 'directory' }); } else { - entries.push({ relative, isDirectory: false }); + // A symlink to a directory is not a directory here, and its content is + // its target, not bytes to read - reading one raises EISDIR. Record the + // kind so cleanup compares each entry the way the copy wrote it. + entries.push({ relative, kind: entry.isSymbolicLink() ? 'symlink' : 'file' }); } } }; @@ -525,23 +533,67 @@ async function listTreeEntriesDeepestFirst( * reports as a retained destination. The move does not complete silently. * * `entries` must be listed before the final fingerprint, so that an arrival is - * either caught by that fingerprint or absent from the set. What this cannot - * cover is an edit to a file that is already in the set: the copy holds the - * content as of the fingerprint, and the newer bytes go with the source. That - * window is the one the staging rename closes, and is why staging is still + * either caught by that fingerprint or absent from the set. + * + * An edit to a file that is already in the set is covered by claiming each file + * before reading it: `rename` is atomic, so once a file is under its claim name + * the bytes there are ours. A writer that rewrites the file by path after that + * point creates a new file at the original path, which is not in `entries`, is + * never deleted, and makes the parent `rmdir` fail with ENOTEMPTY - reported as + * a retained destination. A writer that got there first is caught by comparing + * the claimed bytes against the copy: on a mismatch the file is put back and + * the move is abandoned with both trees intact, because the destination holds + * the older content and deleting the source would lose the newer. + * + * What remains outside this, as for any copy, is a writer holding an open + * descriptor that writes through it after the comparison. Staging is still * preferred whenever the rename is permitted at all. */ +const CLEANUP_CLAIM_SUFFIX = '.openspec-claim'; + +/** What the entry holds now, for comparison against the copy. */ +async function readEntryIdentity( + entryPath: string, + kind: 'file' | 'symlink' +): Promise { + return kind === 'symlink' + ? `symlink:${await fs.readlink(entryPath)}` + : `file:${(await fingerprintFileContents(entryPath)).toString('hex')}`; +} + async function removeVerifiedTree( root: string, - entries: { relative: string; isDirectory: boolean }[] + entries: TreeEntry[], + destination: string ): Promise { for (const entry of entries) { const target = path.join(root, entry.relative); - if (entry.isDirectory) { + if (entry.kind === 'directory') { await fs.rmdir(target); - } else { - await fs.rm(target, { force: true }); + continue; + } + const claimed = target + CLEANUP_CLAIM_SUFFIX; + await fs.rename(target, claimed); + let claimedIdentity: string; + let copiedIdentity: string; + try { + claimedIdentity = await readEntryIdentity(claimed, entry.kind); + copiedIdentity = await readEntryIdentity( + path.join(destination, entry.relative), + entry.kind + ); + } catch (error) { + await fs.rename(claimed, target).catch(() => undefined); + throw error; + } + if (claimedIdentity !== copiedIdentity) { + await fs.rename(claimed, target).catch(() => undefined); + throw new Error( + `${target} changed after it was verified, so the copy at ${destination} ` + + 'does not hold its current content.' + ); } + await fs.rm(claimed, { force: true }); } await fs.rmdir(root); } @@ -586,7 +638,7 @@ async function copyThenRemoveDirectory( } throw copyError; } - let verifiedEntries: { relative: string; isDirectory: boolean }[]; + let verifiedEntries: TreeEntry[]; try { // Listed before the verification, not after it. A file that arrives before // the fingerprint changes it and aborts the move; one that arrives after is @@ -611,7 +663,7 @@ async function copyThenRemoveDirectory( throw verificationError; } try { - await removeVerifiedTree(source, verifiedEntries); + await removeVerifiedTree(source, verifiedEntries, dest); } catch (cleanupError) { // Removal may already have deleted part of the source, or stopped on an // entry that appeared after verification. The destination is now the only diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 3f136b3044..9338c57c15 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -6852,6 +6852,79 @@ The system SHALL capture write feedback. await expect(fs.access(changeDir)).resolves.not.toThrow(); }); + it('keeps an edit to an already-verified file, and retains the destination', async () => { + // The window alfred flagged: the copy and both fingerprints are behind + // us, and an editor rewrites a file that is already in the verified set. + // The destination holds the older bytes, so removing that file would + // delete the only copy of the newer ones and still report success. + // Cleanup claims each file by renaming it before reading, then compares + // the claimed bytes against the copy, so this aborts instead. + const changeName = 'eperm-late-edit-preserved'; + const changeDir = await createChange( + changeName, + 'edit-feedback', + `## ADDED Requirements + +### Requirement: Edit feedback is captured +The system SHALL capture edit feedback. + +#### Scenario: Feedback is stored +- **WHEN** edit feedback arrives +- **THEN** it is stored +` + ); + const deltaPath = path.join(changeDir, 'specs', 'edit-feedback', 'spec.md'); + const newBytes = '# Rewritten while the move was finishing.\n'; + + const realRename = fs.rename.bind(fs); + const realWriteFile = fs.writeFile.bind(fs); + onTestFinished(() => vi.restoreAllMocks()); + + let edited = false; + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + const from = String(source); + if ( + from.endsWith(`${path.sep}openspec${path.sep}changes${path.sep}${changeName}`) + ) { + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + // The claim rename for the delta: land the edit just before it, so the + // bytes we claim are the new ones and the copy still holds the old. + if (!edited && from.endsWith(`${path.sep}spec.md`) && from.includes(changeName)) { + edited = true; + await realWriteFile(from, newBytes); + } + return realRename(source, destination); + }); + + await expect(archiveCommand.execute(changeName, { yes: true })).rejects.toThrow( + /could not remove the source|retained for recovery/i + ); + + expect(edited).toBe(true); + // The newer bytes are still on disk, under their own path. + await expect(fs.readFile(deltaPath, 'utf-8')).resolves.toBe(newBytes); + // No claim file is left behind. + await expect( + fs.access(`${deltaPath}.openspec-claim`) + ).rejects.toThrow(); + // The complete copy is retained for recovery. + await expect( + fs.access( + path.join( + tempDir, + 'openspec', + 'changes', + 'archive', + `${formatLocalDate()}-${changeName}`, + 'specs', + 'edit-feedback', + 'spec.md' + ) + ) + ).resolves.not.toThrow(); + }); + it('keeps a file added after verification, and retains the destination', async () => { // The unstaged fallback copies from the live change directory: the // archive claim covers the destination, not the source. Cleanup must From 9f736b6a68e7f2f63058ae15970770d50b34d06e Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 23 Sep 2026 09:53:10 -0500 Subject: [PATCH 5/5] fix(archive): draw the cleanup claim suffix per move A fixed '.openspec-claim' suffix collided with a source file that legitimately ends in it: claiming 'collision' renamed it over a real 'collision.openspec-claim', and that file's own turn then failed with ENOENT after part of the live source had already been removed. A valid tree could not archive, and its source was damaged for nothing. The suffix is now drawn per move and checked against the entries being removed, so no claim of one entry can land on another. Co-Authored-By: Claude Opus 5 --- src/core/archive.ts | 21 +++++++++++-- test/core/archive.test.ts | 66 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/core/archive.ts b/src/core/archive.ts index d757766006..da965496db 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -549,7 +549,23 @@ async function listTreeEntriesDeepestFirst( * descriptor that writes through it after the comparison. Staging is still * preferred whenever the rename is permitted at all. */ -const CLEANUP_CLAIM_SUFFIX = '.openspec-claim'; +/** + * A claim suffix no entry in this move can already carry. + * + * A fixed suffix collides with a source file that legitimately ends in it: + * claiming `x` would rename it over a real `x.openspec-claim`, and that file's + * own turn would then fail with ENOENT after part of the live source had + * already been removed. So draw a fresh suffix per move and prove it against + * the very set being removed - if no entry ends with the suffix, no claim of + * one entry can land on another. + */ +function makeClaimSuffix(entries: TreeEntry[]): string { + const names = entries.map((entry) => entry.relative); + for (;;) { + const suffix = `.openspec-claim-${randomUUID()}`; + if (!names.some((name) => name.endsWith(suffix))) return suffix; + } +} /** What the entry holds now, for comparison against the copy. */ async function readEntryIdentity( @@ -566,13 +582,14 @@ async function removeVerifiedTree( entries: TreeEntry[], destination: string ): Promise { + const claimSuffix = makeClaimSuffix(entries); for (const entry of entries) { const target = path.join(root, entry.relative); if (entry.kind === 'directory') { await fs.rmdir(target); continue; } - const claimed = target + CLEANUP_CLAIM_SUFFIX; + const claimed = target + claimSuffix; await fs.rename(target, claimed); let claimedIdentity: string; let copiedIdentity: string; diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 9338c57c15..00eb907670 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -6852,6 +6852,66 @@ The system SHALL capture write feedback. await expect(fs.access(changeDir)).resolves.not.toThrow(); }); + it('archives a source that already contains a claim-suffixed filename', async () => { + // A fixed claim suffix collided with a real source file ending in it: + // claiming `collision` renamed it over `collision.openspec-claim`, and + // that file's own turn then failed with ENOENT after part of the live + // source was gone. The suffix is drawn per move and checked against the + // entries being removed, so a valid tree like this archives normally. + const changeName = 'eperm-claim-suffix-collision'; + const changeDir = await createChange( + changeName, + 'collision-feedback', + `## ADDED Requirements + +### Requirement: Collision feedback is captured +The system SHALL capture collision feedback. + +#### Scenario: Feedback is stored +- **WHEN** collision feedback arrives +- **THEN** it is stored +` + ); + await fs.writeFile(path.join(changeDir, 'collision'), 'plain entry\n'); + await fs.writeFile( + path.join(changeDir, 'collision.openspec-claim'), + 'entry that looks like a claim\n' + ); + + const realRename = fs.rename.bind(fs); + onTestFinished(() => vi.restoreAllMocks()); + vi.spyOn(fs, 'rename').mockImplementation(async (source, destination) => { + if ( + String(source).endsWith( + `${path.sep}openspec${path.sep}changes${path.sep}${changeName}` + ) + ) { + throw Object.assign(new Error('directory is busy'), { code: 'EPERM' }); + } + return realRename(source, destination); + }); + + await expect( + archiveCommand.execute(changeName, { yes: true }) + ).resolves.not.toThrow(); + + // The source is gone and both files made it into the archive intact. + await expect(fs.access(changeDir)).rejects.toThrow(); + const archived = path.join( + tempDir, + 'openspec', + 'changes', + 'archive', + `${formatLocalDate()}-${changeName}` + ); + await expect(fs.readFile(path.join(archived, 'collision'), 'utf-8')).resolves.toBe( + 'plain entry\n' + ); + await expect( + fs.readFile(path.join(archived, 'collision.openspec-claim'), 'utf-8') + ).resolves.toBe('entry that looks like a claim\n'); + }); + it('keeps an edit to an already-verified file, and retains the destination', async () => { // The window alfred flagged: the copy and both fingerprints are behind // us, and an editor rewrites a file that is already in the verified set. @@ -6904,10 +6964,10 @@ The system SHALL capture edit feedback. expect(edited).toBe(true); // The newer bytes are still on disk, under their own path. await expect(fs.readFile(deltaPath, 'utf-8')).resolves.toBe(newBytes); - // No claim file is left behind. + // No claim file is left behind, whatever suffix this move drew. await expect( - fs.access(`${deltaPath}.openspec-claim`) - ).rejects.toThrow(); + fs.readdir(path.dirname(deltaPath)) + ).resolves.toEqual(['spec.md']); // The complete copy is retained for recovery. await expect( fs.access(