From 72423ab328b3fa99346ed33756ec67503e8eff6b Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 15:39:48 -0500 Subject: [PATCH 1/9] fix(schema): preserve YAML formatting when forking a schema Rename a forked schema via yaml's Document API (parseDocument + doc.set) instead of round-tripping through parseSchema/stringifyYaml, so block scalars, comments, and key order in the source schema.yaml survive the fork. Keep the structural parseSchema validation before the document mutation so an invalid source is still rejected (addresses PR #1130 review). Adds fork-level regression coverage for both formatting preservation and invalid-source rejection. Co-Authored-By: JinzeLin Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 15 +- test/commands/schema-fork-fidelity.test.ts | 175 +++++++++++++++++++++ 2 files changed, 186 insertions(+), 4 deletions(-) create mode 100644 test/commands/schema-fork-fidelity.test.ts diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 2aa9d2f700..052eb9a9d1 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -2,7 +2,7 @@ import { Command } from 'commander'; import * as fs from 'node:fs'; import * as path from 'node:path'; import ora from 'ora'; -import { stringify as stringifyYaml } from 'yaml'; +import { stringify as stringifyYaml, parseDocument } from 'yaml'; import { getSchemaDir, getProjectSchemasDir, @@ -706,10 +706,17 @@ export function registerSchemaCommand(program: Command): void { // Update name in schema.yaml const destSchemaPath = path.join(destinationDir, 'schema.yaml'); const schemaContent = fs.readFileSync(destSchemaPath, 'utf-8'); - const schema = parseSchema(schemaContent); - schema.name = destinationName; - fs.writeFileSync(destSchemaPath, stringifyYaml(schema)); + // Validate the structure before mutating, so an invalid source is + // rejected here just as the pre-Document-API path did. + parseSchema(schemaContent); + + // Rename via yaml's Document API instead of re-serializing the parsed + // object, so block scalars, comments, and key order in the source + // schema.yaml survive the fork. + const doc = parseDocument(schemaContent); + doc.set('name', destinationName); + fs.writeFileSync(destSchemaPath, doc.toString()); if (spinner) spinner.succeed(`Forked '${source}' to '${destinationName}'`); diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts new file mode 100644 index 0000000000..fdfeddef2a --- /dev/null +++ b/test/commands/schema-fork-fidelity.test.ts @@ -0,0 +1,175 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { Command } from 'commander'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; + +// Regression coverage for PR #1130: `schema fork` must preserve the source +// schema.yaml verbatim (block scalars, comments, key order) except for the +// updated top-level `name`. The prior implementation round-tripped through +// parseSchema -> stringifyYaml, which dropped comments and could rewrite +// block-scalar style; the new implementation edits a yaml Document in place. + +async function runSchemaCommand(args: string[]): Promise { + const { registerSchemaCommand } = await import('../../src/commands/schema.js'); + const program = new Command(); + registerSchemaCommand(program); + await program.parseAsync(['node', 'openspec', 'schema', ...args]); +} + +describe('schema fork fidelity (PR #1130)', () => { + let tempDir: string; + let originalCwd: string; + let originalEnv: NodeJS.ProcessEnv; + let originalExitCode: typeof process.exitCode; + let consoleLogSpy: ReturnType; + let consoleErrorSpy: ReturnType; + + // A valid source schema.yaml that intentionally carries: + // - a leading banner comment + // - an inline comment above a field + // - a literal block scalar (`instruction: |`) with multiple lines + const SOURCE_SCHEMA = [ + '# banner comment that must survive the fork', + 'name: src-schema', + 'version: 1', + 'description: source schema', + 'artifacts:', + ' - id: proposal', + ' generates: proposal.md', + ' description: The proposal', + ' template: proposal.md', + ' # instruction is authored as a literal block scalar', + ' instruction: |', + ' First line of guidance', + ' Second line of guidance', + ' requires: []', + '', + ].join('\n'); + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-fork-fidelity-')); + fs.mkdirSync(path.join(tempDir, 'openspec', 'schemas'), { recursive: true }); + + originalCwd = process.cwd(); + originalEnv = { ...process.env }; + originalExitCode = process.exitCode; + process.exitCode = undefined; + + process.chdir(tempDir); + process.env.XDG_DATA_HOME = path.join(tempDir, 'xdg-data'); + process.env.XDG_CONFIG_HOME = path.join(tempDir, 'xdg-config'); + + // Author the source schema as a project-local schema. + const srcDir = path.join(tempDir, 'openspec', 'schemas', 'src-schema'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.writeFileSync(path.join(srcDir, 'schema.yaml'), SOURCE_SCHEMA); + + consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + process.chdir(originalCwd); + process.env = originalEnv; + process.exitCode = originalExitCode; + fs.rmSync(tempDir, { recursive: true, force: true }); + consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + vi.resetModules(); + }); + + it('preserves block scalars and comments while updating name', async () => { + await runSchemaCommand(['fork', 'src-schema', 'forked-schema', '--json']); + + expect(process.exitCode).toBeFalsy(); + + const destPath = path.join( + tempDir, + 'openspec', + 'schemas', + 'forked-schema', + 'schema.yaml' + ); + expect(fs.existsSync(destPath)).toBe(true); + + const forked = fs.readFileSync(destPath, 'utf-8'); + + // 1. name was updated to the destination name. + expect(forked).toMatch(/^name: forked-schema$/m); + expect(forked).not.toMatch(/^name: src-schema$/m); + + // 2. The literal block scalar is preserved in `|` form, NOT flattened to + // a single line (the old parseSchema->stringify round trip is what this + // PR replaces). Both content lines remain on their own indented lines. + expect(forked).toMatch(/instruction: \|/); + expect(forked).toContain(' First line of guidance'); + expect(forked).toContain(' Second line of guidance'); + expect(forked).not.toContain('First line of guidance Second line of guidance'); + + // 3. Comments survive the fork (the old object round trip dropped them). + expect(forked).toContain('# banner comment that must survive the fork'); + expect(forked).toContain( + '# instruction is authored as a literal block scalar' + ); + + // 4. Everything else is byte-identical: the only change vs. the source is + // the name line. Proven by reconstructing the source from the fork. + const roundTripToSource = forked.replace( + /^name: forked-schema$/m, + 'name: src-schema' + ); + expect(roundTripToSource).toBe(SOURCE_SCHEMA); + }); + + it('rejects an invalid source schema instead of forking it', async () => { + // Regression for the review on PR #1130: switching to the Document API must + // NOT drop the structural validation the old parseSchema path performed. + // This source is valid YAML but structurally invalid (its single artifact + // is missing the required generates/description/template fields), so the + // fork must fail rather than serialize a broken schema. + const invalidDir = path.join( + tempDir, + 'openspec', + 'schemas', + 'invalid-schema' + ); + fs.mkdirSync(invalidDir, { recursive: true }); + fs.writeFileSync( + path.join(invalidDir, 'schema.yaml'), + ['name: invalid-schema', 'version: 1', 'artifacts:', ' - id: proposal', ''].join('\n') + ); + + await runSchemaCommand([ + 'fork', + 'invalid-schema', + 'forked-invalid', + '--json', + ]); + + // The command reports failure (non-zero exit) and the JSON payload marks + // the fork as not performed with an error message. + expect(process.exitCode).toBeTruthy(); + const output = consoleLogSpy.mock.calls + .map((call) => String(call[0])) + .join('\n'); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/Invalid schema/i); + }); + + it('demonstrates the pre-#1130 object round trip dropped comments', async () => { + // This asserts the OLD behavior that motivated the fix: parsing to a plain + // object and re-stringifying discards comments (and does not carry the + // authored block-scalar comment through). Kept as executable documentation + // of what regressed before the parseDocument-based fix. + const { parse, stringify } = await import('yaml'); + const obj = parse(SOURCE_SCHEMA) as Record; + obj.name = 'forked-schema'; + const oldOutput = stringify(obj); + + expect(oldOutput).not.toContain('# banner comment that must survive the fork'); + expect(oldOutput).not.toContain( + '# instruction is authored as a literal block scalar' + ); + }); +}); From 74240434f42067a119810d2fb358b1f2b31bc299 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 16:26:49 -0500 Subject: [PATCH 2/9] harden(schema): clean up partial fork when validation fails If the source schema is structurally invalid, parseSchema throws after copyDirRecursive has already created the destination directory, leaving a broken half-schema on disk that made the next fork report "already exists". Wrap the read/validate/rename in a try/catch that removes the just-created destination on any failure and rethrows so the original error still drives the JSON/exit-code reporting. The cleanup can only ever delete a directory this run created: the no-force existing-dest path returns before the copy, and the --force path removes the prior directory first. This also closes a mid-write truncation window for free. Adds regression coverage: cleanup + retryability on invalid source, the pre-existing-destination-is-never-touched invariant, and a lock-in that YAML-ambiguous names (true/false/null/off) round-trip as strings. Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 35 +++++++---- test/commands/schema-fork-fidelity.test.ts | 73 ++++++++++++++++++++++ 2 files changed, 96 insertions(+), 12 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 052eb9a9d1..cbf2bcbd60 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -705,18 +705,29 @@ export function registerSchemaCommand(program: Command): void { // Update name in schema.yaml const destSchemaPath = path.join(destinationDir, 'schema.yaml'); - const schemaContent = fs.readFileSync(destSchemaPath, 'utf-8'); - - // Validate the structure before mutating, so an invalid source is - // rejected here just as the pre-Document-API path did. - parseSchema(schemaContent); - - // Rename via yaml's Document API instead of re-serializing the parsed - // object, so block scalars, comments, and key order in the source - // schema.yaml survive the fork. - const doc = parseDocument(schemaContent); - doc.set('name', destinationName); - fs.writeFileSync(destSchemaPath, doc.toString()); + try { + const schemaContent = fs.readFileSync(destSchemaPath, 'utf-8'); + + // Validate the structure before mutating, so an invalid source is + // rejected here just as the pre-Document-API path did. + parseSchema(schemaContent); + + // Rename via yaml's Document API instead of re-serializing the parsed + // object, so block scalars, comments, and key order in the source + // schema.yaml survive the fork. + const doc = parseDocument(schemaContent); + doc.set('name', destinationName); + fs.writeFileSync(destSchemaPath, doc.toString()); + } catch (error) { + // copyDirRecursive created destinationDir fresh this run (the + // existing-destination path without --force returns before the copy, + // and the --force path removes the prior directory first), so removing + // it here can only delete the partial fork we just made — never a + // pre-existing user directory. Rethrow so the original error still + // reaches the outer handler and drives the JSON/exit-code reporting. + fs.rmSync(destinationDir, { recursive: true, force: true }); + throw error; + } if (spinner) spinner.succeed(`Forked '${source}' to '${destinationName}'`); diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index fdfeddef2a..13bfa23591 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -155,6 +155,79 @@ describe('schema fork fidelity (PR #1130)', () => { .join('\n'); expect(output).toContain('"forked": false'); expect(output).toMatch(/Invalid schema/i); + + // Hardening: a failed fork must not litter a partial destination. The + // freshly-copied dir is removed, so a corrected retry is not blocked by a + // spurious "already exists". + const destDir = path.join( + tempDir, + 'openspec', + 'schemas', + 'forked-invalid' + ); + expect(fs.existsSync(destDir)).toBe(false); + + // A second fork of the same (still invalid) source fails for the RIGHT + // reason — invalid schema — not because a leftover directory exists. + process.exitCode = undefined; + consoleLogSpy.mockClear(); + await runSchemaCommand(['fork', 'invalid-schema', 'forked-invalid', '--json']); + const retry = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(retry).toMatch(/Invalid schema/i); + expect(retry).not.toMatch(/already exists/i); + }); + + it('never removes a pre-existing destination when --force is absent', async () => { + // Guards the safety invariant of the failure cleanup: it may only delete a + // directory this run created. Without --force an existing destination is + // rejected BEFORE any copy, so a user's directory (and its files) must be + // left completely untouched. + const invalidDir = path.join(tempDir, 'openspec', 'schemas', 'invalid-schema'); + fs.mkdirSync(invalidDir, { recursive: true }); + fs.writeFileSync( + path.join(invalidDir, 'schema.yaml'), + ['name: invalid-schema', 'version: 1', 'artifacts:', ' - id: proposal', ''].join('\n') + ); + + const destDir = path.join(tempDir, 'openspec', 'schemas', 'forked-invalid'); + fs.mkdirSync(destDir, { recursive: true }); + const sentinel = path.join(destDir, 'sentinel.txt'); + fs.writeFileSync(sentinel, 'do not delete me'); + + await runSchemaCommand(['fork', 'invalid-schema', 'forked-invalid', '--json']); + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toMatch(/already exists/i); + // The pre-existing directory and its contents survive intact. + expect(fs.existsSync(sentinel)).toBe(true); + expect(fs.readFileSync(sentinel, 'utf-8')).toBe('do not delete me'); + }); + + it('writes YAML-ambiguous names as strings, not booleans/null', async () => { + // Lock-in for the Document-API rename: forking to a kebab-valid but + // YAML-ambiguous name (true/false/null/off) must round-trip as a STRING, so + // the forked schema still loads. Guards against a future yaml core-schema + // change that would emit these unquoted. + const { parse } = await import('yaml'); + for (const name of ['true', 'false', 'null', 'off']) { + process.exitCode = undefined; + await runSchemaCommand(['fork', 'src-schema', name, '--json']); + expect(process.exitCode).toBeFalsy(); + + const destPath = path.join( + tempDir, + 'openspec', + 'schemas', + name, + 'schema.yaml' + ); + const parsed = parse(fs.readFileSync(destPath, 'utf-8')) as { + name: unknown; + }; + expect(parsed.name).toBe(name); + expect(typeof parsed.name).toBe('string'); + } }); it('demonstrates the pre-#1130 object round trip dropped comments', async () => { From 6498ef52c56054c8838d3e60ff326da3b0d106b7 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 16:47:13 -0500 Subject: [PATCH 3/9] harden(schema): validate fork source up front; never mask fork errors Second hardening pass on the fork command, from an adversarial review of the previously-added cleanup. 1. Atomicity: validate the source's schema.yaml up front, immediately after assertSchemaTreeCanBeCopied and BEFORE the --force removal of an existing destination. Previously the source was validated only after the copy, so `fork --force ` destroyed the existing destination and then failed, leaving nothing. This matches `schema init`, which already validates before it overwrites. Behavior is unchanged for valid sources, and the redundant post-copy validation is dropped. 2. Never mask the real error: the failure-cleanup rmSync is now wrapped in its own try/catch. fs.rmSync's `force` only suppresses ENOENT, not EPERM/EBUSY/ENOTEMPTY (e.g. a locked file on Windows or a concurrent process), so a failed cleanup could previously replace the real "Invalid schema" diagnostic with a confusing filesystem error. The original error is now always rethrown. Adds regression coverage: --force with an invalid source leaves a valid destination intact; the pre-existing-destination test now uses a valid source so it exercises the no-force "already exists" guard directly. Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 31 +++++++++----- test/commands/schema-fork-fidelity.test.ts | 49 ++++++++++++++++++---- 2 files changed, 62 insertions(+), 18 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index cbf2bcbd60..31962446fc 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -675,6 +675,15 @@ export function registerSchemaCommand(program: Command): void { const trustedSourceDir = fs.realpathSync(sourceDir); assertSchemaTreeCanBeCopied(trustedSourceDir); + // Validate the source's schema content up front too, so a structurally + // invalid source is rejected before the --force path can remove an + // existing destination. This keeps `fork --force` atomic — an unusable + // source never destroys a valid destination — matching `schema init`, + // which likewise validates before it overwrites. + parseSchema( + fs.readFileSync(path.join(trustedSourceDir, 'schema.yaml'), 'utf-8') + ); + // Check destination const destinationDir = path.join(getProjectSchemasDir(projectRoot), destinationName); @@ -706,15 +715,11 @@ export function registerSchemaCommand(program: Command): void { // Update name in schema.yaml const destSchemaPath = path.join(destinationDir, 'schema.yaml'); try { - const schemaContent = fs.readFileSync(destSchemaPath, 'utf-8'); - - // Validate the structure before mutating, so an invalid source is - // rejected here just as the pre-Document-API path did. - parseSchema(schemaContent); - // Rename via yaml's Document API instead of re-serializing the parsed // object, so block scalars, comments, and key order in the source - // schema.yaml survive the fork. + // schema.yaml survive the fork. (The source was already validated + // above, before any files were copied.) + const schemaContent = fs.readFileSync(destSchemaPath, 'utf-8'); const doc = parseDocument(schemaContent); doc.set('name', destinationName); fs.writeFileSync(destSchemaPath, doc.toString()); @@ -723,9 +728,15 @@ export function registerSchemaCommand(program: Command): void { // existing-destination path without --force returns before the copy, // and the --force path removes the prior directory first), so removing // it here can only delete the partial fork we just made — never a - // pre-existing user directory. Rethrow so the original error still - // reaches the outer handler and drives the JSON/exit-code reporting. - fs.rmSync(destinationDir, { recursive: true, force: true }); + // pre-existing user directory. Guard the cleanup in its own try/catch + // so a failed removal (e.g. a locked file on Windows) can never mask + // the original error, then rethrow so the real failure still drives + // the JSON/exit-code reporting. + try { + fs.rmSync(destinationDir, { recursive: true, force: true }); + } catch { + // Best-effort cleanup; the original error below is what matters. + } throw error; } diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index 13bfa23591..afe16e5b89 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -182,6 +182,26 @@ describe('schema fork fidelity (PR #1130)', () => { // directory this run created. Without --force an existing destination is // rejected BEFORE any copy, so a user's directory (and its files) must be // left completely untouched. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'my-dest'); + fs.mkdirSync(destDir, { recursive: true }); + const sentinel = path.join(destDir, 'sentinel.txt'); + fs.writeFileSync(sentinel, 'do not delete me'); + + await runSchemaCommand(['fork', 'src-schema', 'my-dest', '--json']); + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toMatch(/already exists/i); + // The pre-existing directory and its contents survive intact. + expect(fs.existsSync(sentinel)).toBe(true); + expect(fs.readFileSync(sentinel, 'utf-8')).toBe('do not delete me'); + }); + + it('does not destroy a valid destination when --force forks an invalid source', async () => { + // Atomicity: `fork --force` must validate the source BEFORE removing the + // existing destination, so an unusable source can never leave the user with + // nothing. The source is validated up front (before the --force removal), + // so the prior destination survives untouched. const invalidDir = path.join(tempDir, 'openspec', 'schemas', 'invalid-schema'); fs.mkdirSync(invalidDir, { recursive: true }); fs.writeFileSync( @@ -189,19 +209,32 @@ describe('schema fork fidelity (PR #1130)', () => { ['name: invalid-schema', 'version: 1', 'artifacts:', ' - id: proposal', ''].join('\n') ); - const destDir = path.join(tempDir, 'openspec', 'schemas', 'forked-invalid'); + // A valid, pre-existing destination the user does not want to lose. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); fs.mkdirSync(destDir, { recursive: true }); - const sentinel = path.join(destDir, 'sentinel.txt'); - fs.writeFileSync(sentinel, 'do not delete me'); + const existing = path.join(destDir, 'schema.yaml'); + const existingContent = [ + 'name: keep-me', + 'version: 3', + 'artifacts:', + ' - id: proposal', + ' generates: proposal.md', + ' description: Keep this', + ' template: proposal.md', + ' requires: []', + '', + ].join('\n'); + fs.writeFileSync(existing, existingContent); - await runSchemaCommand(['fork', 'invalid-schema', 'forked-invalid', '--json']); + await runSchemaCommand(['fork', 'invalid-schema', 'keep-me', '--force', '--json']); const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); expect(process.exitCode).toBeTruthy(); - expect(output).toMatch(/already exists/i); - // The pre-existing directory and its contents survive intact. - expect(fs.existsSync(sentinel)).toBe(true); - expect(fs.readFileSync(sentinel, 'utf-8')).toBe('do not delete me'); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/Invalid schema/i); + // The valid destination was NOT destroyed by the --force removal. + expect(fs.existsSync(existing)).toBe(true); + expect(fs.readFileSync(existing, 'utf-8')).toBe(existingContent); }); it('writes YAML-ambiguous names as strings, not booleans/null', async () => { From 87d5da4f80ba143f7ff4682cfdd111d43a7317a9 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 16:03:18 -0500 Subject: [PATCH 4/9] harden(schema): reject self-fork and stage fork before replacing destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two data-loss defects in `schema fork --force` (per alfred-openspec review): 1. Self-fork: forking a schema onto itself removed the destination (which IS the source) before the copy, so the copy then read a directory it had just deleted — destroying the only copy. Now rejected up front by comparing the real (symlink-resolved) source and destination paths before any removal. 2. Non-atomic replacement: an existing destination was removed before the new fork was fully copied and name-updated, so a mid-copy failure left the user with nothing. The fork is now staged in a temporary sibling directory and only swapped into place once complete; any failure while staging leaves both the source and the existing destination untouched. Adds regressions: self-fork is rejected with the source intact; a forced fork whose copy fails leaves the existing destination byte-identical with no staging leftovers. Co-Authored-By: JinzeLin Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 100 +++++++++++++-------- test/commands/schema-fork-fidelity.test.ts | 91 +++++++++++++++++++ 2 files changed, 152 insertions(+), 39 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 31962446fc..4954883130 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -685,55 +685,77 @@ export function registerSchemaCommand(program: Command): void { ); // Check destination - const destinationDir = path.join(getProjectSchemasDir(projectRoot), destinationName); + const schemasDir = getProjectSchemasDir(projectRoot); + const destinationDir = path.join(schemasDir, destinationName); + + // Reject a self-fork. Forking a schema onto itself with --force would + // otherwise remove the source at the replacement step below and then + // fail the copy, destroying the only copy of the schema. Resolve both + // sides to their real paths (realpathSync follows symlinks; path.resolve + // is a fallback only for a destination that does not exist yet) so a + // symlink or a `.`/`..` spelling of the same directory is still caught. + const resolvedDestination = fs.existsSync(destinationDir) + ? fs.realpathSync(destinationDir) + : path.resolve(destinationDir); + if (resolvedDestination === trustedSourceDir) { + throw new Error( + `Cannot fork schema '${source}' onto itself; choose a different destination name` + ); + } - if (fs.existsSync(destinationDir)) { - if (!options?.force) { - if (options?.json) { - console.log(JSON.stringify({ - forked: false, - error: `Schema '${destinationName}' already exists`, - suggestion: 'Use --force to overwrite', - }, null, 2)); - } else { - console.error(`Error: Schema '${destinationName}' already exists at ${destinationDir}`); - console.error('Use --force to overwrite'); - } - process.exitCode = 1; - return; + const destinationExists = fs.existsSync(destinationDir); + if (destinationExists && !options?.force) { + if (options?.json) { + console.log(JSON.stringify({ + forked: false, + error: `Schema '${destinationName}' already exists`, + suggestion: 'Use --force to overwrite', + }, null, 2)); + } else { + console.error(`Error: Schema '${destinationName}' already exists at ${destinationDir}`); + console.error('Use --force to overwrite'); } - - // Remove existing - if (spinner) spinner.start(`Removing existing schema '${destinationName}'...`); - fs.rmSync(destinationDir, { recursive: true }); + process.exitCode = 1; + return; } - // Copy schema + // Stage the complete fork in a temporary sibling directory first, then + // swap it into place. This keeps `fork --force` atomic: an existing + // destination is only removed once the new fork has been fully copied, + // name-updated, and (via the up-front parseSchema above) validated. Any + // failure while staging leaves both the source and the existing + // destination exactly as they were. if (spinner) spinner.start(`Forking '${source}' to '${destinationName}'...`); - copyDirRecursive(trustedSourceDir, destinationDir); - - // Update name in schema.yaml - const destSchemaPath = path.join(destinationDir, 'schema.yaml'); + fs.mkdirSync(schemasDir, { recursive: true }); + const stagingDir = fs.mkdtempSync(path.join(schemasDir, '.fork-staging-')); try { - // Rename via yaml's Document API instead of re-serializing the parsed - // object, so block scalars, comments, and key order in the source - // schema.yaml survive the fork. (The source was already validated - // above, before any files were copied.) - const schemaContent = fs.readFileSync(destSchemaPath, 'utf-8'); + copyDirRecursive(trustedSourceDir, stagingDir); + + // Update name in the staged schema.yaml via yaml's Document API + // instead of re-serializing the parsed object, so block scalars, + // comments, and key order in the source schema.yaml survive the fork. + const stagedSchemaPath = path.join(stagingDir, 'schema.yaml'); + const schemaContent = fs.readFileSync(stagedSchemaPath, 'utf-8'); const doc = parseDocument(schemaContent); doc.set('name', destinationName); - fs.writeFileSync(destSchemaPath, doc.toString()); + fs.writeFileSync(stagedSchemaPath, doc.toString()); + + // Swap the staged fork into place. Only now — with a complete, valid + // fork ready — is any existing destination removed, so a failure above + // can never leave the user without their original. + if (destinationExists) { + if (spinner) spinner.text = `Replacing existing schema '${destinationName}'...`; + fs.rmSync(destinationDir, { recursive: true, force: true }); + } + fs.renameSync(stagingDir, destinationDir); } catch (error) { - // copyDirRecursive created destinationDir fresh this run (the - // existing-destination path without --force returns before the copy, - // and the --force path removes the prior directory first), so removing - // it here can only delete the partial fork we just made — never a - // pre-existing user directory. Guard the cleanup in its own try/catch - // so a failed removal (e.g. a locked file on Windows) can never mask - // the original error, then rethrow so the real failure still drives - // the JSON/exit-code reporting. + // Remove only the staging directory we created this run; the source + // and any existing destination are left exactly as we found them. + // Guard the cleanup in its own try/catch so a failed removal (e.g. a + // locked file on Windows) can never mask the original error, then + // rethrow so the real failure still drives the JSON/exit-code report. try { - fs.rmSync(destinationDir, { recursive: true, force: true }); + fs.rmSync(stagingDir, { recursive: true, force: true }); } catch { // Best-effort cleanup; the original error below is what matters. } diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index afe16e5b89..e021f48fde 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -4,6 +4,26 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; +// Deterministic, cross-platform hook to fail the fork's file copy without +// relying on filesystem permissions or symlink support (ESM forbids spying on +// node:fs's namespace exports directly). Only copyFileSync is wrapped; every +// other fs call passes straight through to the real implementation. +const fsControl = vi.hoisted(() => ({ failCopyFileSync: false })); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: actual, + copyFileSync: (...args: Parameters) => { + if (fsControl.failCopyFileSync) { + throw new Error('simulated copy failure'); + } + return actual.copyFileSync(...args); + }, + }; +}); + // Regression coverage for PR #1130: `schema fork` must preserve the source // schema.yaml verbatim (block scalars, comments, key order) except for the // updated top-level `name`. The prior implementation round-tripped through @@ -237,6 +257,77 @@ describe('schema fork fidelity (PR #1130)', () => { expect(fs.readFileSync(existing, 'utf-8')).toBe(existingContent); }); + it('rejects a self-fork and leaves the source intact', async () => { + // Data-loss guard: forking a schema onto itself with --force must be + // rejected UP FRONT. The old flow removed the destination (which is the + // source) before copying, so the copy then read from a directory it had + // just deleted — destroying the only copy of the schema. Nothing may be + // removed, and the source schema.yaml must be byte-identical afterward. + await runSchemaCommand(['fork', 'src-schema', 'src-schema', '--force', '--json']); + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/onto itself/i); + + // The source survives untouched, comments and block scalars included. + const srcPath = path.join( + tempDir, + 'openspec', + 'schemas', + 'src-schema', + 'schema.yaml' + ); + expect(fs.existsSync(srcPath)).toBe(true); + expect(fs.readFileSync(srcPath, 'utf-8')).toBe(SOURCE_SCHEMA); + }); + + it('preserves an existing --force destination when the copy fails', async () => { + // Atomicity for a mid-copy failure: the fork is staged in a temporary + // sibling directory and only swapped into place once complete, so a failure + // WHILE copying must never remove the existing destination. Here the source + // is valid (passes the up-front parseSchema), but the file copy itself is + // forced to fail; the pre-existing destination must be left fully intact. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); + fs.mkdirSync(destDir, { recursive: true }); + const existing = path.join(destDir, 'schema.yaml'); + const existingContent = [ + 'name: keep-me', + 'version: 3', + 'artifacts:', + ' - id: proposal', + ' generates: proposal.md', + ' description: Keep this', + ' template: proposal.md', + ' requires: []', + '', + ].join('\n'); + fs.writeFileSync(existing, existingContent); + + fsControl.failCopyFileSync = true; + try { + await runSchemaCommand(['fork', 'src-schema', 'keep-me', '--force', '--json']); + } finally { + fsControl.failCopyFileSync = false; + } + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/simulated copy failure/i); + + // The existing destination was never removed — the copy failed while still + // staging, before any replacement. + expect(fs.existsSync(existing)).toBe(true); + expect(fs.readFileSync(existing, 'utf-8')).toBe(existingContent); + + // And no staging leftovers linger in the schemas directory. + const leftovers = fs + .readdirSync(path.join(tempDir, 'openspec', 'schemas')) + .filter((entry) => entry.startsWith('.fork-staging-')); + expect(leftovers).toEqual([]); + }); + it('writes YAML-ambiguous names as strings, not booleans/null', async () => { // Lock-in for the Document-API rename: forking to a kebab-valid but // YAML-ambiguous name (true/false/null/off) must round-trip as a STRING, so From 2c20d4c7a02b112864adb491d2eaf28dcd567596 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 16:18:40 -0500 Subject: [PATCH 5/9] harden(schema): back up destination before installing fork so a failed final move restores The stage-then-swap still removed the destination and then renamed staging into place; if that final rename failed (e.g. a Windows lock) the destination was gone with no restore. Now, when a destination exists, `fork --force` moves it to a sibling backup, installs the staged fork, and only then discards the backup. If the install rename throws, the backup is moved back so the original destination is never lost. Non-existing destinations keep the simple staging rename. Adds a regression: forcing the final staging->destination move to fail leaves the pre-existing destination byte-identical with no staging/backup leftovers. Co-Authored-By: JinzeLin Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 27 ++++++-- test/commands/schema-fork-fidelity.test.ts | 74 ++++++++++++++++++++-- 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 4954883130..70a0b89be9 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -740,14 +740,31 @@ export function registerSchemaCommand(program: Command): void { doc.set('name', destinationName); fs.writeFileSync(stagedSchemaPath, doc.toString()); - // Swap the staged fork into place. Only now — with a complete, valid - // fork ready — is any existing destination removed, so a failure above - // can never leave the user without their original. + // Swap the staged fork into place. When a destination already exists, + // move it aside to a sibling backup FIRST, then install the staged + // fork; only once the install succeeds is the backup discarded. If the + // install rename itself fails (e.g. a Windows lock), the backup is + // moved back so the user's original destination is never lost. if (destinationExists) { if (spinner) spinner.text = `Replacing existing schema '${destinationName}'...`; - fs.rmSync(destinationDir, { recursive: true, force: true }); + const backupDir = `${destinationDir}.fork-backup-${process.pid}-${Date.now()}`; + fs.renameSync(destinationDir, backupDir); + try { + fs.renameSync(stagingDir, destinationDir); + } catch (installError) { + // Restore the original destination, then rethrow. Guard the + // restore so a failed rename-back cannot mask the real error. + try { + fs.renameSync(backupDir, destinationDir); + } catch { + // Best-effort restore; the original error below is what matters. + } + throw installError; + } + fs.rmSync(backupDir, { recursive: true, force: true }); + } else { + fs.renameSync(stagingDir, destinationDir); } - fs.renameSync(stagingDir, destinationDir); } catch (error) { // Remove only the staging directory we created this run; the source // and any existing destination are left exactly as we found them. diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index e021f48fde..9d83fc27dc 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -4,11 +4,14 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; -// Deterministic, cross-platform hook to fail the fork's file copy without -// relying on filesystem permissions or symlink support (ESM forbids spying on -// node:fs's namespace exports directly). Only copyFileSync is wrapped; every -// other fs call passes straight through to the real implementation. -const fsControl = vi.hoisted(() => ({ failCopyFileSync: false })); +// Deterministic, cross-platform hooks to fail specific fork filesystem steps +// without relying on permissions or symlink support (ESM forbids spying on +// node:fs's namespace exports directly). Only the wrapped calls are affected; +// every other fs call passes straight through to the real implementation. +const fsControl = vi.hoisted(() => ({ + failCopyFileSync: false, + failStagingInstall: false, +})); vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal(); @@ -21,6 +24,18 @@ vi.mock('node:fs', async (importOriginal) => { } return actual.copyFileSync(...args); }, + renameSync: (...args: Parameters) => { + // Fail ONLY the final staging->destination install move, leaving the + // earlier destination->backup move and the backup->destination restore + // to run for real. + if ( + fsControl.failStagingInstall && + String(args[0]).includes('.fork-staging-') + ) { + throw new Error('simulated install rename failure'); + } + return actual.renameSync(...args); + }, }; }); @@ -328,6 +343,55 @@ describe('schema fork fidelity (PR #1130)', () => { expect(leftovers).toEqual([]); }); + it('restores the destination when the final install move fails', async () => { + // Atomicity for the swap itself: `fork --force` moves the existing + // destination to a sibling backup, installs the staged fork, and only then + // discards the backup. If the install move fails (e.g. a Windows lock), the + // backup must be moved back so the original destination is never lost. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); + fs.mkdirSync(destDir, { recursive: true }); + const existing = path.join(destDir, 'schema.yaml'); + const existingContent = [ + 'name: keep-me', + 'version: 3', + 'artifacts:', + ' - id: proposal', + ' generates: proposal.md', + ' description: Keep this', + ' template: proposal.md', + ' requires: []', + '', + ].join('\n'); + fs.writeFileSync(existing, existingContent); + + fsControl.failStagingInstall = true; + try { + await runSchemaCommand(['fork', 'src-schema', 'keep-me', '--force', '--json']); + } finally { + fsControl.failStagingInstall = false; + } + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/simulated install rename failure/i); + + // The original destination was moved to backup, the install failed, and the + // backup was moved back — so the destination is byte-identical. + expect(fs.existsSync(existing)).toBe(true); + expect(fs.readFileSync(existing, 'utf-8')).toBe(existingContent); + + // No staging or backup leftovers linger in the schemas directory. + const leftovers = fs + .readdirSync(path.join(tempDir, 'openspec', 'schemas')) + .filter( + (entry) => + entry.startsWith('.fork-staging-') || + entry.includes('.fork-backup-') + ); + expect(leftovers).toEqual([]); + }); + it('writes YAML-ambiguous names as strings, not booleans/null', async () => { // Lock-in for the Document-API rename: forking to a kebab-valid but // YAML-ambiguous name (true/false/null/off) must round-trip as a STRING, so From b6c2206b404701c287a6224574502cefb71d3226 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 17:19:52 -0500 Subject: [PATCH 6/9] harden(schema): surface unrecoverable fork restore + hide fork temp dirs from discovery Two more edge cases from alfred's review: 1. A failed backup->destination restore was silently swallowed, so if the final install AND the restore both failed the user lost the destination with no clue the backup existed. Now that case throws an error naming the backup directory and how to move it back, with the original install error attached as cause. 2. The transient `.fork-staging-*` / `.fork-backup-*` directories live inside the schemas dir, so a concurrent scan could surface them as real schemas. isSchemaDir (the single discovery chokepoint) now excludes them; real schema names are kebab-case (no dots) so this can never hide a schema. Adds regressions: an unrecoverable restore surfaces the backup path (and the rescued content is really there); fork temp dirs are excluded from listSchemas and listSchemasWithInfo. Co-Authored-By: JinzeLin Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 16 +++-- src/core/artifact-graph/resolver.ts | 15 +++++ test/commands/schema-fork-fidelity.test.ts | 75 ++++++++++++++++++++++ 3 files changed, 102 insertions(+), 4 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 70a0b89be9..2af90e166e 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -752,12 +752,20 @@ export function registerSchemaCommand(program: Command): void { try { fs.renameSync(stagingDir, destinationDir); } catch (installError) { - // Restore the original destination, then rethrow. Guard the - // restore so a failed rename-back cannot mask the real error. + // Install failed after the original was moved aside. Try to move + // it back. If that restore ALSO fails, the original is stranded in + // the backup dir — surface an error naming both the backup and the + // destination so the user can recover manually, and attach the + // original install error as the cause. Never swallow this. try { fs.renameSync(backupDir, destinationDir); - } catch { - // Best-effort restore; the original error below is what matters. + } catch (restoreError) { + throw new Error( + `Failed to install the forked schema and could not restore the previous '${destinationName}'. ` + + `Your previous schema is preserved at ${backupDir}; move it back to ${destinationDir} to restore. ` + + `Restore error: ${(restoreError as Error).message}`, + { cause: installError } + ); } throw installError; } diff --git a/src/core/artifact-graph/resolver.ts b/src/core/artifact-graph/resolver.ts index 3c9ec80e71..f7d84d140c 100644 --- a/src/core/artifact-graph/resolver.ts +++ b/src/core/artifact-graph/resolver.ts @@ -58,7 +58,22 @@ export function getProjectSchemasDir(projectRoot: string): string { * @param parentDir - The directory containing the entry * @param entry - The directory entry from `fs.readdirSync(..., { withFileTypes: true })` */ +/** + * Directories `schema fork` creates transiently while swapping a fork into + * place: a staging copy (`.fork-staging-`, created via mkdtemp) and a + * backup of the previous destination (`.fork-backup--`). Either + * can briefly coexist with real schemas in the schemas dir, so discovery must + * never surface them. Real schema names are kebab-case (no dots), so excluding + * these dot-bearing temp names can never hide a legitimate schema. + */ +function isOwnedForkTempDir(name: string): boolean { + return name.startsWith('.fork-staging-') || name.includes('.fork-backup-'); +} + export function isSchemaDir(parentDir: string, entry: fs.Dirent): boolean { + if (isOwnedForkTempDir(entry.name)) { + return false; + } if (entry.isDirectory()) { return true; } diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index 9d83fc27dc..2620046037 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -11,6 +11,7 @@ import * as os from 'node:os'; const fsControl = vi.hoisted(() => ({ failCopyFileSync: false, failStagingInstall: false, + failBackupRestore: false, })); vi.mock('node:fs', async (importOriginal) => { @@ -34,6 +35,13 @@ vi.mock('node:fs', async (importOriginal) => { ) { throw new Error('simulated install rename failure'); } + // Fail the backup->destination restore move (source is the backup dir). + if ( + fsControl.failBackupRestore && + String(args[0]).includes('.fork-backup-') + ) { + throw new Error('simulated restore rename failure'); + } return actual.renameSync(...args); }, }; @@ -392,6 +400,73 @@ describe('schema fork fidelity (PR #1130)', () => { expect(leftovers).toEqual([]); }); + it('surfaces the backup location when a failed install cannot be restored', async () => { + // Worst case: the install move fails AND the restore move back also fails. + // The original destination is now stranded in the backup dir. The command + // must NOT silently swallow this — it must throw an error naming the backup + // path so the user can recover manually, with the install error attached. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); + fs.mkdirSync(destDir, { recursive: true }); + const existing = path.join(destDir, 'schema.yaml'); + fs.writeFileSync(existing, 'name: keep-me\nversion: 3\n'); + + fsControl.failStagingInstall = true; + fsControl.failBackupRestore = true; + try { + await runSchemaCommand(['fork', 'src-schema', 'keep-me', '--force', '--json']); + } finally { + fsControl.failStagingInstall = false; + fsControl.failBackupRestore = false; + } + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toContain('"forked": false'); + // The destination loss is surfaced, not silent: the error names a + // `.fork-backup-` directory and how to restore it. + expect(output).toMatch(/\.fork-backup-/); + expect(output).toMatch(/preserved at/i); + expect(output).toMatch(/could not restore/i); + + // The original content really is still on disk in the backup dir the error + // points at (recovery is genuinely possible). + const backupDir = fs + .readdirSync(path.join(tempDir, 'openspec', 'schemas')) + .find((entry) => entry.includes('.fork-backup-')); + expect(backupDir).toBeTruthy(); + const rescued = fs.readFileSync( + path.join(tempDir, 'openspec', 'schemas', backupDir!, 'schema.yaml'), + 'utf-8' + ); + expect(rescued).toBe('name: keep-me\nversion: 3\n'); + }); + + it('excludes fork staging/backup temp dirs from schema discovery', async () => { + // The transient dirs `schema fork` creates live inside the schemas dir, so a + // concurrent `openspec schema list`/validate scan must never treat them as + // real schemas. Simulate both a staging and a backup dir mid-fork. + const schemasDir = path.join(tempDir, 'openspec', 'schemas'); + for (const tempName of ['.fork-staging-abc123', 'keep-me.fork-backup-999-1700000000000']) { + const dir = path.join(schemasDir, tempName); + fs.mkdirSync(dir, { recursive: true }); + // Give them a valid-looking schema.yaml so only the name filter can + // exclude them (not a missing file). + fs.writeFileSync(path.join(dir, 'schema.yaml'), SOURCE_SCHEMA); + } + + const { listSchemas, listSchemasWithInfo } = await import( + '../../src/core/artifact-graph/resolver.js' + ); + + const names = listSchemas(tempDir); + expect(names).toContain('src-schema'); + expect(names.some((n) => n.includes('.fork-'))).toBe(false); + + const infoNames = listSchemasWithInfo(tempDir).map((s) => s.name); + expect(infoNames).toContain('src-schema'); + expect(infoNames.some((n) => n.includes('.fork-'))).toBe(false); + }); + it('writes YAML-ambiguous names as strings, not booleans/null', async () => { // Lock-in for the Document-API rename: forking to a kebab-valid but // YAML-ambiguous name (true/false/null/off) must round-trip as a STRING, so From c59d025056fca4631485f9f4aecae4f64f0e63e6 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 17:47:18 -0500 Subject: [PATCH 7/9] harden(schema): fingerprint fork destination to abort on concurrent edits A concurrent process could edit an existing fork destination between the moment --force authorized the overwrite and the moment the destructive swap ran, and those edits were silently destroyed (reproduced by alfred: mutate destination schema.yaml during copy; --force completed and deleted the newer content). Now, when overwriting an existing destination, the fork: - fingerprints the authorized destination (SHA-256 over every file's relative path and bytes) BEFORE staging; - re-fingerprints and compares immediately before moving the destination aside; on mismatch it ABORTS without touching the destination, preserving the concurrent changes and telling the user to re-run; - re-fingerprints the backup before discarding it on the success path; if it changed during the install window it is kept, not deleted, and its location is surfaced. All prior guarantees remain: self-fork rejection, stage-then-swap, backup/restore on failed install with the backup path surfaced, and the temp-dir discovery filter. Adds regressions: a destination edited concurrently during staging aborts the fork and preserves the edit; a backup modified during the install window is kept and its location surfaced. Co-Authored-By: JinzeLin Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 83 +++++++++++++- test/commands/schema-fork-fidelity.test.ts | 125 ++++++++++++++++++++- 2 files changed, 205 insertions(+), 3 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 2af90e166e..5b5fe094ad 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -1,6 +1,7 @@ import { Command } from 'commander'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import { createHash } from 'node:crypto'; import ora from 'ora'; import { stringify as stringifyYaml, parseDocument } from 'yaml'; import { @@ -324,6 +325,49 @@ function assertSchemaTreeCanBeCopied( } } +/** + * Produces a stable content fingerprint of a directory: a SHA-256 over every + * file's relative path AND its bytes (plus directory paths), walked in sorted + * order. Two directories with byte-identical trees produce the same digest, and + * ANY change to a file's contents, size, or the set of paths changes it. Used to + * detect a concurrent modification of a fork destination between the moment the + * overwrite is authorized and the moment it is actually moved/deleted, so those + * changes are never silently destroyed. + */ +function fingerprintDir(dir: string): string { + const hash = createHash('sha256'); + const walk = (current: string, rel: string): void => { + const entries = fs + .readdirSync(current, { withFileTypes: true }) + .sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); + for (const entry of entries) { + const abs = path.join(current, entry.name); + const relPath = rel ? `${rel}/${entry.name}` : entry.name; + const stats = fs.lstatSync(abs); + if (stats.isDirectory()) { + hash.update(`D:${relPath}\n`); + walk(abs, relPath); + } else if (stats.isFile()) { + hash.update(`F:${relPath}:${stats.size}:`); + hash.update(fs.readFileSync(abs)); + hash.update('\n'); + } else { + // Symlinks / other entry types: record the type + path (and the link + // target when readable) so a swap of one for another is still detected. + let target = ''; + try { + target = fs.readlinkSync(abs); + } catch { + // Non-symlink or unreadable target; the type marker below suffices. + } + hash.update(`O:${relPath}:${target}\n`); + } + } + }; + walk(dir, ''); + return hash.digest('hex'); +} + /** * Default artifacts with descriptions for schema init. */ @@ -719,6 +763,14 @@ export function registerSchemaCommand(program: Command): void { return; } + // Fingerprint the destination the user authorized us to overwrite, BEFORE + // we spend time staging. Staging can take a while, and a concurrent + // process may edit the destination in that window; the fingerprint lets + // us detect such a change and abort rather than clobber it. + const authorizedDestinationFingerprint = destinationExists + ? fingerprintDir(destinationDir) + : null; + // Stage the complete fork in a temporary sibling directory first, then // swap it into place. This keeps `fork --force` atomic: an existing // destination is only removed once the new fork has been fully copied, @@ -747,6 +799,22 @@ export function registerSchemaCommand(program: Command): void { // moved back so the user's original destination is never lost. if (destinationExists) { if (spinner) spinner.text = `Replacing existing schema '${destinationName}'...`; + + // Revalidate immediately before the destructive move: if the + // destination changed on disk while we were staging (or was removed), + // its fingerprint no longer matches what the user authorized. Abort + // WITHOUT touching it, so the concurrent changes are preserved. The + // outer catch cleans up staging. + const currentFingerprint = fs.existsSync(destinationDir) + ? fingerprintDir(destinationDir) + : null; + if (currentFingerprint !== authorizedDestinationFingerprint) { + throw new Error( + `Schema '${destinationName}' at ${destinationDir} changed on disk while the fork was being prepared. ` + + `Aborted to preserve those concurrent changes; nothing was overwritten. Re-run the fork to overwrite the current contents.` + ); + } + const backupDir = `${destinationDir}.fork-backup-${process.pid}-${Date.now()}`; fs.renameSync(destinationDir, backupDir); try { @@ -769,7 +837,20 @@ export function registerSchemaCommand(program: Command): void { } throw installError; } - fs.rmSync(backupDir, { recursive: true, force: true }); + + // Revalidate before discarding the backup: only delete it if it is + // still byte-for-byte the original destination we moved aside. If it + // changed during the install window (a concurrent write to the + // moved-aside directory), do NOT delete it — leave it in place and + // surface where it is so nothing is lost. + if (fingerprintDir(backupDir) === authorizedDestinationFingerprint) { + fs.rmSync(backupDir, { recursive: true, force: true }); + } else { + console.error( + `Warning: the previous '${destinationName}' changed during the fork and was NOT deleted; ` + + `its pre-fork copy is preserved at ${backupDir}.` + ); + } } else { fs.renameSync(stagingDir, destinationDir); } diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index 2620046037..7f5d63a113 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -12,10 +12,18 @@ const fsControl = vi.hoisted(() => ({ failCopyFileSync: false, failStagingInstall: false, failBackupRestore: false, + // When set, simulate a concurrent process editing the fork destination: after + // the next real file copy during staging, overwrite `path` with `content`. + mutateOnCopy: null as null | { path: string; content: string }, + // When set, simulate a concurrent write to the backup dir: after the + // destination is moved aside (dest -> backup), overwrite the backup's + // schema.yaml with `content`. + mutateBackupContent: null as null | string, })); vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal(); + const nodePath = await import('node:path'); return { ...actual, default: actual, @@ -23,7 +31,13 @@ vi.mock('node:fs', async (importOriginal) => { if (fsControl.failCopyFileSync) { throw new Error('simulated copy failure'); } - return actual.copyFileSync(...args); + const result = actual.copyFileSync(...args); + if (fsControl.mutateOnCopy) { + const { path: target, content } = fsControl.mutateOnCopy; + fsControl.mutateOnCopy = null; // one-shot + actual.writeFileSync(target, content); + } + return result; }, renameSync: (...args: Parameters) => { // Fail ONLY the final staging->destination install move, leaving the @@ -42,7 +56,21 @@ vi.mock('node:fs', async (importOriginal) => { ) { throw new Error('simulated restore rename failure'); } - return actual.renameSync(...args); + const result = actual.renameSync(...args); + // After the destination is moved aside to the backup dir, simulate a + // concurrent write into that backup before it is (potentially) deleted. + if ( + fsControl.mutateBackupContent !== null && + String(args[1]).includes('.fork-backup-') + ) { + const content = fsControl.mutateBackupContent; + fsControl.mutateBackupContent = null; // one-shot + actual.writeFileSync( + nodePath.join(String(args[1]), 'schema.yaml'), + content + ); + } + return result; }, }; }); @@ -467,6 +495,99 @@ describe('schema fork fidelity (PR #1130)', () => { expect(infoNames.some((n) => n.includes('.fork-'))).toBe(false); }); + it('aborts and preserves a destination edited concurrently during staging', async () => { + // The race alfred reproduced: between authorizing the --force overwrite and + // the destructive swap, another process edits the destination. The fork must + // fingerprint the authorized destination, re-check it before moving it aside, + // and ABORT if it changed — never clobbering the concurrent edit. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); + fs.mkdirSync(destDir, { recursive: true }); + const existing = path.join(destDir, 'schema.yaml'); + const originalContent = [ + 'name: keep-me', + 'version: 3', + 'artifacts:', + ' - id: proposal', + ' generates: proposal.md', + ' description: Original', + ' template: proposal.md', + ' requires: []', + '', + ].join('\n'); + fs.writeFileSync(existing, originalContent); + + // Simulate the concurrent edit landing DURING the staging copy (after the + // destination fingerprint was captured, before the destructive move). + const concurrentContent = originalContent.replace( + 'description: Original', + 'description: Edited by a concurrent process' + ); + fsControl.mutateOnCopy = { path: existing, content: concurrentContent }; + try { + await runSchemaCommand(['fork', 'src-schema', 'keep-me', '--force', '--json']); + } finally { + fsControl.mutateOnCopy = null; + } + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/changed on disk|concurrent|aborted/i); + + // The concurrent edit is preserved — NOT overwritten by the fork. The + // destination still has the concurrent content, and is not the fork's copy + // (which would carry src-schema's `description: The proposal`). + expect(fs.existsSync(existing)).toBe(true); + expect(fs.readFileSync(existing, 'utf-8')).toBe(concurrentContent); + + // No staging or backup leftovers remain — the abort happened before any move. + const leftovers = fs + .readdirSync(path.join(tempDir, 'openspec', 'schemas')) + .filter( + (entry) => + entry.startsWith('.fork-staging-') || entry.includes('.fork-backup-') + ); + expect(leftovers).toEqual([]); + }); + + it('keeps the backup when it is modified during the install window', async () => { + // Second window: after the original is moved aside to the backup, a + // concurrent write lands in the backup before it is discarded. The fork must + // re-fingerprint the backup before deleting it and, on mismatch, keep it and + // surface its location rather than silently deleting changed content. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); + fs.mkdirSync(destDir, { recursive: true }); + const existing = path.join(destDir, 'schema.yaml'); + fs.writeFileSync(existing, 'name: keep-me\nversion: 3\n'); + + fsControl.mutateBackupContent = 'name: keep-me\nversion: 4-touched-in-backup\n'; + try { + await runSchemaCommand(['fork', 'src-schema', 'keep-me', '--force', '--json']); + } finally { + fsControl.mutateBackupContent = null; + } + + // The fork itself succeeds (the install path was untouched by the race). + const errOutput = consoleErrorSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeFalsy(); + // The install landed the fork at the destination. + expect(fs.readFileSync(existing, 'utf-8')).toMatch(/^name: keep-me$/m); + + // The changed backup was NOT deleted, and its location is surfaced. + expect(errOutput).toMatch(/was NOT deleted/i); + expect(errOutput).toMatch(/\.fork-backup-/); + const backupDir = fs + .readdirSync(path.join(tempDir, 'openspec', 'schemas')) + .find((entry) => entry.includes('.fork-backup-')); + expect(backupDir).toBeTruthy(); + expect( + fs.readFileSync( + path.join(tempDir, 'openspec', 'schemas', backupDir!, 'schema.yaml'), + 'utf-8' + ) + ).toBe('name: keep-me\nversion: 4-touched-in-backup\n'); + }); + it('writes YAML-ambiguous names as strings, not booleans/null', async () => { // Lock-in for the Document-API rename: forking to a kebab-valid but // YAML-ambiguous name (true/false/null/off) must round-trip as a STRING, so From ce9039e0b8492a16cfdf0e6cf6eda2d618dd8504 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 17:59:05 -0500 Subject: [PATCH 8/9] fix(schema): avoid stat-then-read in fork fingerprint (CodeQL js/file-system-race) fingerprintDir called fs.lstatSync then fs.readFileSync on the same path, which CodeQL flags as a file-system race (the file may change between the check and the read). Use the Dirent type already returned by readdirSync ({ withFileTypes: true }) instead of a separate lstat, and read files directly, deriving the size from the bytes read. Behavior is unchanged (13/13 fork-fidelity tests, incl. the concurrent-edit race regressions, still pass); one fewer syscall per entry. Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 5b5fe094ad..6f7c7e6422 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -343,13 +343,16 @@ function fingerprintDir(dir: string): string { for (const entry of entries) { const abs = path.join(current, entry.name); const relPath = rel ? `${rel}/${entry.name}` : entry.name; - const stats = fs.lstatSync(abs); - if (stats.isDirectory()) { + // Use the entry type from readdir (no separate lstat), then read the file + // directly — avoiding a stat-then-read check/use gap. Size is derived from + // the bytes actually read, so the digest still covers content and length. + if (entry.isDirectory()) { hash.update(`D:${relPath}\n`); walk(abs, relPath); - } else if (stats.isFile()) { - hash.update(`F:${relPath}:${stats.size}:`); - hash.update(fs.readFileSync(abs)); + } else if (entry.isFile()) { + const contents = fs.readFileSync(abs); + hash.update(`F:${relPath}:${contents.length}:`); + hash.update(contents); hash.update('\n'); } else { // Symlinks / other entry types: record the type + path (and the link From 2d10a1c3230bb5d7727ba2f85ed1ada77fbda439 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 18:15:05 -0500 Subject: [PATCH 9/9] harden(schema): validate the completed staged fork before any destructive step The up-front parseSchema only checks the SOURCE, but copyDirRecursive reads source files that can change mid-copy, so the staged result can be invalid even though the source was valid at the pre-check (reproduced by alfred: mutate source schema.yaml to invalid inside copyFileSync; --force installed the invalid fork and deleted the valid destination). Now, after copying and the Document-API name edit, the fork validates the COMPLETED staged schema.yaml (the exact bytes about to be installed) with parseSchema BEFORE any destination displacement. On failure it aborts, cleans up staging, and rethrows a clear error ("the staged fork of '' is not a valid schema ...; aborted, '' was not modified") chaining the parse error. The up-front source parseSchema stays as a fail-fast; this is the authoritative gate. Order before the swap: validate staged -> fingerprint-revalidate dest -> rename dest->backup -> rename staging->dest -> revalidate+rm backup. Adds a regression: a source that becomes structurally invalid during staging aborts the fork and leaves the valid destination byte-identical, no leftovers. Co-Authored-By: JinzeLin Co-Authored-By: Claude Opus 4.8 --- src/commands/schema.ts | 16 +++++ test/commands/schema-fork-fidelity.test.ts | 73 ++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 6f7c7e6422..5ec8172be7 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -795,6 +795,22 @@ export function registerSchemaCommand(program: Command): void { doc.set('name', destinationName); fs.writeFileSync(stagedSchemaPath, doc.toString()); + // Authoritative gate: validate the COMPLETED staged schema — the exact + // bytes we are about to install — not just the source at the pre-check. + // The source files copyDirRecursive reads can change mid-copy, so a + // source that was valid up front can still produce an invalid staged + // fork. Validating here, before ANY destructive step, guarantees we + // never install an invalid fork or delete a valid destination for one. + try { + parseSchema(fs.readFileSync(stagedSchemaPath, 'utf-8')); + } catch (validationError) { + throw new Error( + `The staged fork of '${source}' is not a valid schema (the source may have changed during copy); ` + + `aborted, '${destinationName}' was not modified.`, + { cause: validationError } + ); + } + // Swap the staged fork into place. When a destination already exists, // move it aside to a sibling backup FIRST, then install the staged // fork; only once the install succeeds is the backup discarded. If the diff --git a/test/commands/schema-fork-fidelity.test.ts b/test/commands/schema-fork-fidelity.test.ts index 7f5d63a113..d28703ed0a 100644 --- a/test/commands/schema-fork-fidelity.test.ts +++ b/test/commands/schema-fork-fidelity.test.ts @@ -19,6 +19,10 @@ const fsControl = vi.hoisted(() => ({ // destination is moved aside (dest -> backup), overwrite the backup's // schema.yaml with `content`. mutateBackupContent: null as null | string, + // When set, simulate the source changing mid-copy so the STAGED schema.yaml + // ends up invalid: after it is copied into staging, overwrite it with + // `content` (structurally invalid). + corruptStagedSchema: null as null | string, })); vi.mock('node:fs', async (importOriginal) => { @@ -37,6 +41,17 @@ vi.mock('node:fs', async (importOriginal) => { fsControl.mutateOnCopy = null; // one-shot actual.writeFileSync(target, content); } + // Corrupt the staged schema.yaml right after it is copied into staging, to + // simulate the source having changed to invalid content during the copy. + if ( + fsControl.corruptStagedSchema !== null && + String(args[1]).includes('.fork-staging-') && + String(args[1]).endsWith('schema.yaml') + ) { + const content = fsControl.corruptStagedSchema; + fsControl.corruptStagedSchema = null; // one-shot + actual.writeFileSync(String(args[1]), content); + } return result; }, renameSync: (...args: Parameters) => { @@ -588,6 +603,64 @@ describe('schema fork fidelity (PR #1130)', () => { ).toBe('name: keep-me\nversion: 4-touched-in-backup\n'); }); + it('aborts and preserves the destination when the source becomes invalid during staging', async () => { + // The gap alfred reproduced: the up-front parseSchema checks the SOURCE, but + // copyDirRecursive reads source files that can change mid-copy, so the STAGED + // result can be invalid even though the source was valid at the pre-check. + // The completed staged schema must be validated before any destructive step; + // an invalid staged fork must abort and leave a valid destination untouched. + const destDir = path.join(tempDir, 'openspec', 'schemas', 'keep-me'); + fs.mkdirSync(destDir, { recursive: true }); + const existing = path.join(destDir, 'schema.yaml'); + const existingContent = [ + 'name: keep-me', + 'version: 3', + 'artifacts:', + ' - id: proposal', + ' generates: proposal.md', + ' description: Keep this valid schema', + ' template: proposal.md', + ' requires: []', + '', + ].join('\n'); + fs.writeFileSync(existing, existingContent); + + // Structurally invalid but valid YAML (artifact missing required fields), so + // parseDocument + doc.set succeed but parseSchema rejects the staged result. + const invalidStaged = [ + 'name: keep-me', + 'version: 1', + 'artifacts:', + ' - id: proposal', + '', + ].join('\n'); + fsControl.corruptStagedSchema = invalidStaged; + try { + await runSchemaCommand(['fork', 'src-schema', 'keep-me', '--force', '--json']); + } finally { + fsControl.corruptStagedSchema = null; + } + + const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(process.exitCode).toBeTruthy(); + expect(output).toContain('"forked": false'); + expect(output).toMatch(/not a valid schema|aborted/i); + + // The valid destination is preserved byte-identical — never overwritten by + // the invalid staged fork nor deleted for it. + expect(fs.existsSync(existing)).toBe(true); + expect(fs.readFileSync(existing, 'utf-8')).toBe(existingContent); + + // No staging or backup leftovers remain — the abort happened before any move. + const leftovers = fs + .readdirSync(path.join(tempDir, 'openspec', 'schemas')) + .filter( + (entry) => + entry.startsWith('.fork-staging-') || entry.includes('.fork-backup-') + ); + expect(leftovers).toEqual([]); + }); + it('writes YAML-ambiguous names as strings, not booleans/null', async () => { // Lock-in for the Document-API rename: forking to a kebab-valid but // YAML-ambiguous name (true/false/null/off) must round-trip as a STRING, so