diff --git a/.changeset/archive-eperm-copy-without-staging.md b/.changeset/archive-eperm-copy-without-staging.md new file mode 100644 index 0000000000..f758e56b3c --- /dev/null +++ b/.changeset/archive-eperm-copy-without-staging.md @@ -0,0 +1,11 @@ +--- +"@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. + + 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 62a12ef728..fde3ee2b48 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,161 @@ 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'; +} + +/** + * 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. + * + * `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, + 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, + 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; + } + 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) { + 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 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 + // 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 +645,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; } @@ -673,6 +770,14 @@ async function claimArchiveDestination( interface SpecSnapshot { target: string; existed: boolean; + /** + * 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. + */ + pruneBoundary?: string; outcome: 'write' | 'retire'; expectedContent?: Buffer; content?: Buffer; @@ -862,7 +967,31 @@ async function assertDistinctMutationTargets(mutations: SpecMutation[]): Promise } } -async function captureSpecSnapshots(mutations: SpecMutation[]): Promise { +/** + * 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[], + mainSpecsDir: string +): Promise { return Promise.all( mutations.map(async ({ update, outcome, rebuilt }) => { try { @@ -908,6 +1037,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 +1144,13 @@ async function restoreSpecSnapshots(snapshots: SpecSnapshot[]): Promise { if (!snapshot.existed) { await fs.rm(snapshot.target, { force: true }); + // Only a capability directory this write created is ours to take back. + // One the user already had stays, empty or not, with its own mode - + // pruneEmptyDirs never removes its boundary. + await pruneEmptyDirs( + path.dirname(snapshot.target), + snapshot.pruneBoundary ?? mainSpecsDir + ); continue; } if (snapshot.symlink !== undefined) { @@ -1756,7 +1899,7 @@ export class ArchiveCommand { ); } } - const specSnapshots = await captureSpecSnapshots(mutations); + const specSnapshots = await captureSpecSnapshots(mutations, mainSpecsDir); const specSnapshotsByTarget = new Map( specSnapshots.map((snapshot) => [snapshot.target, snapshot]) ); @@ -2028,7 +2171,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..3f136b3044 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,279 @@ 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('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 + // 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('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( + 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(