diff --git a/.changeset/preserve-crlf-line-endings.md b/.changeset/preserve-crlf-line-endings.md new file mode 100644 index 0000000000..e42f4943a7 --- /dev/null +++ b/.changeset/preserve-crlf-line-endings.md @@ -0,0 +1,9 @@ +--- +"@fission-ai/openspec": patch +--- + +Preserve a file's existing line endings when rewriting it, so Windows users no longer get whole-file diffs. Applying a delta to a CRLF spec (the default on a Windows checkout with `core.autocrlf=true`) rewrote the file to LF, turning a one-requirement change into a diff that touched every line. `openspec archive` now writes the spec back with the convention it already used; a spec that does not exist yet is still written with LF. + +The same fix covers marker-managed files: installing or updating shell completions in a CRLF `.bashrc` or `.zshrc` no longer leaves the file with mixed endings, which `bash` reports as `$'\r': command not found`. + +`scripts/pack-version-check.mjs` now spawns `npm` through `cross-spawn`, so the release guard can run on Windows, where `npm` is `npm.cmd` and cannot be resolved by `execFile`. diff --git a/scripts/pack-version-check.mjs b/scripts/pack-version-check.mjs index 43cf8050eb..8aa8c75bb6 100644 --- a/scripts/pack-version-check.mjs +++ b/scripts/pack-version-check.mjs @@ -10,18 +10,34 @@ // `changeset publish` triggers `prepublishOnly` (also builds here). This // means an explicit build is not strictly necessary for the guard. -import { execFileSync } from 'child_process'; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs'; import { tmpdir } from 'os'; import path from 'path'; +import spawn from 'cross-spawn'; function log(msg) { if (process.env.CI) return; // keep CI logs quiet by default console.log(msg); } +// cross-spawn, not execFileSync: on Windows `npm` is npm.cmd, which execFile +// cannot resolve without a shell. Keeps the argv form, so no shell is involved. function run(cmd, args, opts = {}) { - return execFileSync(cmd, args, { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], ...opts }); + const result = spawn.sync(cmd, args, { + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + ...opts, + }); + + if (result.error) throw result.error; + if (result.status !== 0) { + const stderr = (result.stderr || '').trim(); + throw new Error( + `${cmd} ${args.join(' ')} exited with ${result.status}${stderr ? `: ${stderr}` : ''}` + ); + } + + return result.stdout; } function npmPack() { diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index ebf650278c..3e63294ea0 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -28,6 +28,7 @@ import { } from './validation/constants.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; import { FileSystemUtils } from '../utils/file-system.js'; +import { matchLineEnding } from '../utils/line-endings.js'; // ----------------------------------------------------------------------------- // Types @@ -1243,11 +1244,19 @@ export async function writeUpdatedSpec( // Create target directory if needed const targetDir = path.dirname(update.target); await fs.mkdir(targetDir, { recursive: true }); + + // The parsers normalize CRLF to LF on read, so `rebuilt` is always LF. Write + // it back with the convention the file already used, or a Windows checkout + // (core.autocrlf=true) sees every line of the spec change when one + // requirement moved. A spec that does not exist yet stays LF. + const previous = await fs.readFile(update.target, 'utf-8').catch(() => undefined); + const toWrite = previous === undefined ? rebuilt : matchLineEnding(rebuilt, previous); + await options.beforeMutate?.(); // Preserve the established in-place write semantics: symlink referents, // hard-linked specs, ACLs, extended attributes, and filesystems without hard // links must continue to behave as they did before capability retirement. - await fs.writeFile(update.target, rebuilt); + await fs.writeFile(update.target, toWrite); if (options.silent) return; const specName = update.id; diff --git a/src/utils/file-system.ts b/src/utils/file-system.ts index 5cf2ef8594..c9f901a4be 100644 --- a/src/utils/file-system.ts +++ b/src/utils/file-system.ts @@ -1,5 +1,6 @@ import * as nodeFs from 'fs'; import path from 'path'; +import { matchLineEnding } from './line-endings.js'; const fs = nodeFs.promises; const { constants: fsConstants } = nodeFs; @@ -325,10 +326,16 @@ export class FileSystemUtils { endMarker: string ): Promise { let existingContent = ''; - + // The managed block is composed with '\n', so splicing it into a CRLF file + // would leave mixed endings behind. bash reports a stray '\r' in .bashrc as + // "$'\r': command not found", so settle the whole file on the convention it + // already used. A file that does not exist yet stays LF. + let originalContent: string | undefined; + if (await this.fileExists(filePath)) { existingContent = await this.readFile(filePath); - + originalContent = existingContent; + const startIndex = findMarkerIndex(existingContent, startMarker); const endIndex = startIndex !== -1 ? findMarkerIndex(existingContent, endMarker, startIndex + startMarker.length) @@ -352,8 +359,13 @@ export class FileSystemUtils { } else { existingContent = startMarker + '\n' + content + '\n' + endMarker; } - - await this.writeFile(filePath, existingContent); + + await this.writeFile( + filePath, + originalContent === undefined + ? existingContent + : matchLineEnding(existingContent, originalContent) + ); } static async ensureWritePermissions(dirPath: string): Promise { diff --git a/src/utils/line-endings.ts b/src/utils/line-endings.ts new file mode 100644 index 0000000000..4ac2f970ef --- /dev/null +++ b/src/utils/line-endings.ts @@ -0,0 +1,50 @@ +/** + * Line-ending helpers. + * + * Every parser in this codebase normalizes CRLF to LF on the way in, so all + * serialization logic can assume '\n'. That leaves the write side responsible + * for restoring whatever convention the file already used: without it, editing + * one requirement in a CRLF spec rewrites every line of the file and buries the + * real change in the diff. + */ + +export type LineEnding = '\n' | '\r\n'; + +/** + * The dominant line ending in `content`, or undefined when it holds no line + * break to judge from. + * + * Mixed files resolve to whichever ending is more common, with CRLF winning a + * tie: a file that is mostly CRLF is a CRLF file that picked up a stray LF, and + * settling the whole file on one ending is what keeps later diffs small. + */ +export function detectLineEnding(content: string): LineEnding | undefined { + const crlf = content.match(/\r\n/g)?.length ?? 0; + // Count LFs not preceded by CR, so CRLF is never also counted as LF. + const lf = content.match(/(?= lf ? '\r\n' : '\n'; +} + +/** + * Re-apply `ending` to LF-normalized `content`. + * + * Normalizes to LF first, so the result is uniform even if the caller passed + * content that already contained CRLF. + */ +export function applyLineEnding(content: string, ending: LineEnding): string { + const normalized = content.replace(/\r\n/g, '\n'); + return ending === '\n' ? normalized : normalized.replace(/\n/g, '\r\n'); +} + +/** + * Rewrite `content` to match the convention of `original`. + * + * When `original` has no line break to learn from, `content` is left as LF — + * the portable default this project writes new files with. + */ +export function matchLineEnding(content: string, original: string): string { + const ending = detectLineEnding(original); + return ending === undefined ? content : applyLineEnding(content, ending); +} diff --git a/test/core/specs-apply.line-endings.test.ts b/test/core/specs-apply.line-endings.test.ts new file mode 100644 index 0000000000..f99b69a176 --- /dev/null +++ b/test/core/specs-apply.line-endings.test.ts @@ -0,0 +1,129 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { promises as fs } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { + buildUpdatedSpec, + findSpecUpdates, + writeUpdatedSpec, +} from '../../src/core/specs-apply.js'; + +/** + * A spec written by a Windows editor, or checked out with core.autocrlf=true, + * arrives with CRLF endings. Applying a delta must not silently convert the + * whole file to LF: that turns a one-requirement change into a diff touching + * every line, which is unreviewable. + */ + +const ORIGINAL_REQUIREMENT = [ + '### Requirement: Existing behavior', + 'The project SHALL expose the original behavior.', + '', + '#### Scenario: Existing path', + '- **WHEN** the behavior is exercised', + '- **THEN** it SHALL remain available', +].join('\n'); + +const UPDATED_REQUIREMENT = [ + '### Requirement: Existing behavior', + 'The project SHALL expose the updated behavior.', + '', + '#### Scenario: Existing path', + '- **WHEN** the behavior is exercised', + '- **THEN** it SHALL remain available', +].join('\n'); + +const BASE_SPEC = [ + '# demo Specification', + '', + '## Purpose', + 'Demonstrates line-ending preservation.', + '', + '## Requirements', + ORIGINAL_REQUIREMENT, + '', +].join('\n'); + +const DELTA_SPEC = ['## MODIFIED Requirements', '', UPDATED_REQUIREMENT, ''].join('\n'); + +function countEndings(content: string): { crlf: number; loneLf: number } { + const crlf = content.match(/\r\n/g)?.length ?? 0; + const loneLf = content.match(/(? { + let tempDir: string; + let changeDir: string; + let mainSpecsDir: string; + let source: string; + let target: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-spec-eol-')); + changeDir = path.join(tempDir, 'openspec', 'changes', 'eol-test'); + mainSpecsDir = path.join(tempDir, 'openspec', 'specs'); + source = path.join(changeDir, 'specs', 'demo', 'spec.md'); + target = path.join(mainSpecsDir, 'demo', 'spec.md'); + await fs.mkdir(path.dirname(source), { recursive: true }); + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.writeFile(source, DELTA_SPEC); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + async function applyTo(targetContent: string): Promise { + await fs.writeFile(target, targetContent); + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + const built = await buildUpdatedSpec(update, 'eol-test', { silent: true }); + await writeUpdatedSpec(update, built.rebuilt, built.counts, { silent: true }); + return fs.readFile(target, 'utf-8'); + } + + it('keeps a CRLF spec on CRLF', async () => { + const written = await applyTo(BASE_SPEC.replaceAll('\n', '\r\n')); + + expect(written).toContain('updated behavior.'); + const { crlf, loneLf } = countEndings(written); + expect(loneLf).toBe(0); + expect(crlf).toBeGreaterThan(0); + }); + + it('keeps an LF spec on LF', async () => { + const written = await applyTo(BASE_SPEC); + + expect(written).toContain('updated behavior.'); + const { crlf } = countEndings(written); + expect(crlf).toBe(0); + }); + + it('writes a brand-new spec with LF', async () => { + // No existing target to take a convention from; LF is the portable default. + // A new spec only accepts ADDED requirements. + await fs.writeFile( + source, + ['## ADDED Requirements', '', ORIGINAL_REQUIREMENT, ''].join('\n') + ); + await fs.rm(target, { force: true }); + + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + const built = await buildUpdatedSpec(update, 'eol-test', { silent: true }); + await writeUpdatedSpec(update, built.rebuilt, built.counts, { silent: true }); + + const written = await fs.readFile(target, 'utf-8'); + expect(countEndings(written).crlf).toBe(0); + }); + + it('normalizes a mixed-ending spec to its dominant ending', async () => { + const mixed = BASE_SPEC.replaceAll('\n', '\r\n').replace('## Purpose\r\n', '## Purpose\n'); + const written = await applyTo(mixed); + + // CRLF dominates the input, so the output should settle on CRLF throughout + // rather than preserving the stray LF. + const { crlf, loneLf } = countEndings(written); + expect(crlf).toBeGreaterThan(0); + expect(loneLf).toBe(0); + }); +}); diff --git a/test/utils/line-endings.test.ts b/test/utils/line-endings.test.ts new file mode 100644 index 0000000000..8ed03c4bdc --- /dev/null +++ b/test/utils/line-endings.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from 'vitest'; +import { + applyLineEnding, + detectLineEnding, + matchLineEnding, +} from '../../src/utils/line-endings.js'; + +describe('detectLineEnding', () => { + it('reports LF for an LF file', () => { + expect(detectLineEnding('a\nb\nc')).toBe('\n'); + }); + + it('reports CRLF for a CRLF file', () => { + expect(detectLineEnding('a\r\nb\r\nc')).toBe('\r\n'); + }); + + it('reports undefined when there is no line break', () => { + expect(detectLineEnding('single line')).toBeUndefined(); + expect(detectLineEnding('')).toBeUndefined(); + }); + + it('does not count a CRLF as an LF', () => { + // Two CRLF and no lone LF: a naive /\n/ count would see 2 of each and tie. + expect(detectLineEnding('a\r\nb\r\nc')).toBe('\r\n'); + }); + + it('picks the dominant ending in a mixed file', () => { + expect(detectLineEnding('a\r\nb\r\nc\r\nd\ne')).toBe('\r\n'); + expect(detectLineEnding('a\nb\nc\nd\r\ne')).toBe('\n'); + }); + + it('breaks a tie toward CRLF', () => { + expect(detectLineEnding('a\r\nb\nc')).toBe('\r\n'); + }); + + it('handles a lone CR without treating it as a line ending', () => { + // A bare CR is not a line break this project emits; it must not be + // mistaken for CRLF. + expect(detectLineEnding('a\rb')).toBeUndefined(); + }); +}); + +describe('applyLineEnding', () => { + it('converts LF to CRLF', () => { + expect(applyLineEnding('a\nb\n', '\r\n')).toBe('a\r\nb\r\n'); + }); + + it('leaves LF alone when LF is requested', () => { + expect(applyLineEnding('a\nb\n', '\n')).toBe('a\nb\n'); + }); + + it('is idempotent on already-CRLF content', () => { + expect(applyLineEnding('a\r\nb\r\n', '\r\n')).toBe('a\r\nb\r\n'); + }); + + it('collapses mixed content to the requested ending', () => { + expect(applyLineEnding('a\r\nb\nc', '\r\n')).toBe('a\r\nb\r\nc'); + expect(applyLineEnding('a\r\nb\nc', '\n')).toBe('a\nb\nc'); + }); +}); + +describe('matchLineEnding', () => { + it('restores CRLF from a CRLF original', () => { + expect(matchLineEnding('x\ny\n', 'a\r\nb\r\n')).toBe('x\r\ny\r\n'); + }); + + it('keeps LF from an LF original', () => { + expect(matchLineEnding('x\ny\n', 'a\nb\n')).toBe('x\ny\n'); + }); + + it('defaults to LF when the original has no line break', () => { + expect(matchLineEnding('x\ny\n', 'single line')).toBe('x\ny\n'); + expect(matchLineEnding('x\ny\n', '')).toBe('x\ny\n'); + }); +}); diff --git a/test/utils/marker-updates.test.ts b/test/utils/marker-updates.test.ts index 75476aef96..669db45dc8 100644 --- a/test/utils/marker-updates.test.ts +++ b/test/utils/marker-updates.test.ts @@ -283,6 +283,74 @@ ${END_MARKER} expect(secondResult).toBe(firstResult); }); }); + describe('line endings', () => { + const START = '# >>> openspec >>>'; + const END = '# <<< openspec <<<'; + + function countEndings(content: string): { crlf: number; loneLf: number } { + return { + crlf: content.match(/\r\n/g)?.length ?? 0, + loneLf: content.match(/(? { + // A .bashrc with CRLF endings must not come back mixed: bash chokes on a + // stray \r with "$'\r': command not found". + const filePath = path.join(testDir, '.bashrc'); + await fs.writeFile(filePath, '# user config\r\nexport EDITOR="vim"\r\n'); + + await FileSystemUtils.updateFileWithMarkers( + filePath, + 'alias openspec="npx openspec"', + START, + END + ); + + const result = await fs.readFile(filePath, 'utf-8'); + expect(result).toContain('alias openspec'); + expect(result).toContain('export EDITOR'); + expect(countEndings(result).loneLf).toBe(0); + }); + + it('keeps an LF rc file on LF', async () => { + const filePath = path.join(testDir, '.bashrc'); + await fs.writeFile(filePath, '# user config\nexport EDITOR="vim"\n'); + + await FileSystemUtils.updateFileWithMarkers( + filePath, + 'alias openspec="npx openspec"', + START, + END + ); + + const result = await fs.readFile(filePath, 'utf-8'); + expect(countEndings(result).crlf).toBe(0); + }); + + it('keeps a CRLF rc file on CRLF when replacing an existing block', async () => { + const filePath = path.join(testDir, '.bashrc'); + await fs.writeFile( + filePath, + `# user config\r\n${START}\r\nold content\r\n${END}\r\nexport EDITOR="vim"\r\n` + ); + + await FileSystemUtils.updateFileWithMarkers(filePath, 'new content', START, END); + + const result = await fs.readFile(filePath, 'utf-8'); + expect(result).toContain('new content'); + expect(result).not.toContain('old content'); + expect(countEndings(result).loneLf).toBe(0); + }); + + it('writes a new file with LF', async () => { + const filePath = path.join(testDir, 'brand-new'); + + await FileSystemUtils.updateFileWithMarkers(filePath, 'content', START, END); + + const result = await fs.readFile(filePath, 'utf-8'); + expect(countEndings(result).crlf).toBe(0); + }); }); describe('removeMarkerBlock', () => { @@ -444,4 +512,6 @@ export EDITOR="vim"`; expect(result).not.toContain(SHELL_START); }); }); + + }); }); diff --git a/test/utils/path-containment.test.ts b/test/utils/path-containment.test.ts new file mode 100644 index 0000000000..51e5216d9e --- /dev/null +++ b/test/utils/path-containment.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; +import path from 'node:path'; + +/** + * Windows filesystems are case-insensitive, so `C:\Repo` and `c:\repo` name the + * same directory. The containment guard in FileSystemUtils.assertPathWithin is + * built on path.relative, and these tests pin down the property it depends on: + * path.win32.relative already folds case, so a drive letter or directory that + * differs only in case is still recognized as inside the allowed root. + * + * They also pin the other half of that contract — a path genuinely outside the + * root, or on another drive, must still be rejected. Any future attempt to make + * the comparison case-insensitive by hand has to keep both halves true. + */ + +function isPathWithin( + allowedDirectory: string, + targetPath: string, + impl: path.PlatformPath +): boolean { + const relative = impl.relative(allowedDirectory, targetPath); + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${impl.sep}`) && + !impl.isAbsolute(relative)) + ); +} + +describe('path containment under Windows case variance', () => { + const root = 'C:\\Repo\\openspec'; + + it('accepts a target whose drive letter differs in case', () => { + expect(isPathWithin(root, 'c:\\Repo\\openspec\\specs\\x', path.win32)).toBe(true); + }); + + it('accepts a target whose directory differs in case', () => { + expect(isPathWithin(root, 'C:\\REPO\\openspec\\specs\\x', path.win32)).toBe(true); + }); + + it('accepts the root itself', () => { + expect(isPathWithin(root, root, path.win32)).toBe(true); + expect(isPathWithin(root, 'c:\\repo\\OPENSPEC', path.win32)).toBe(true); + }); + + it('still rejects a sibling directory outside the root', () => { + expect(isPathWithin(root, 'C:\\Repo\\other\\x', path.win32)).toBe(false); + }); + + it('still rejects a traversal escape', () => { + expect(isPathWithin(root, 'C:\\Repo\\openspec\\..\\other', path.win32)).toBe(false); + }); + + it('still rejects a path on another drive', () => { + expect(isPathWithin(root, 'D:\\Repo\\openspec\\specs', path.win32)).toBe(false); + }); + + it('keeps POSIX containment case-sensitive', () => { + // POSIX filesystems are case-sensitive, so /repo is genuinely not /Repo. + expect(isPathWithin('/Repo/openspec', '/Repo/openspec/specs', path.posix)).toBe(true); + expect(isPathWithin('/Repo/openspec', '/repo/openspec/specs', path.posix)).toBe(false); + }); +});