From 09eacadccdc529f3cd439ab211dfae95e5d5b455 Mon Sep 17 00:00:00 2001 From: Travis James Date: Mon, 21 Sep 2026 05:24:14 -0500 Subject: [PATCH 1/3] fix(windows): preserve a file's existing line endings on rewrite The parsers normalize CRLF to LF on read, but nothing restored it on write. On a Windows checkout (core.autocrlf=true) that turned every rewrite into a whole-file change: applying a delta that added one requirement produced a diff of 21 insertions and 14 deletions, burying the real change. Archiving the same spec now writes 7 insertions and 0 deletions. - specs-apply: write an updated spec back with the convention the file already used; a spec that does not exist yet stays LF. - file-system: same fix for updateFileWithMarkers, so installing shell completions into a CRLF .bashrc/.zshrc no longer leaves mixed endings, which bash reports as "$'\r': command not found". - pack-version-check: spawn npm through cross-spawn, since execFile cannot resolve npm.cmd on Windows. Adds src/utils/line-endings.ts for the detect/restore pair, plus tests pinning the CRLF round trip through the real write paths. Also adds regression tests for path containment under Windows case variance: path.win32.relative already folds case, and those tests pin both halves of the contract so a future "case-insensitive" change cannot quietly loosen the traversal guard. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/preserve-crlf-line-endings.md | 9 ++ scripts/pack-version-check.mjs | 20 +++- src/core/specs-apply.ts | 11 +- src/utils/file-system.ts | 20 +++- src/utils/line-endings.ts | 50 ++++++++ test/core/specs-apply.line-endings.test.ts | 129 +++++++++++++++++++++ test/utils/line-endings.test.ts | 75 ++++++++++++ test/utils/marker-updates.test.ts | 70 +++++++++++ test/utils/path-containment.test.ts | 63 ++++++++++ 9 files changed, 440 insertions(+), 7 deletions(-) create mode 100644 .changeset/preserve-crlf-line-endings.md create mode 100644 src/utils/line-endings.ts create mode 100644 test/core/specs-apply.line-endings.test.ts create mode 100644 test/utils/line-endings.test.ts create mode 100644 test/utils/path-containment.test.ts 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 1ca2fba52d..893a702a48 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -29,6 +29,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 @@ -1244,11 +1245,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); + }); +}); From da1265fb0e55b3eda3bb88b08ea82b717ef4e8b5 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 22 Sep 2026 15:20:19 -0500 Subject: [PATCH 2/3] fix(windows): keep removeMarkerBlock on the file's own newline Addresses the two review points and one more instance of the same bug. `removeMarkerBlock` collapses a run of blank lines, and rebuilt the separator as a bare '\n' regardless of the file it came from. Removing a managed block from a CRLF CLAUDE.md or rc file therefore left a lone LF behind - the mixed ending this PR exists to prevent. It now uses the newline it already detects for the trailing ending. Test fixes: - `marker-updates.test.ts`: close `describe('line endings')` so `removeMarkerBlock` is no longer nested inside `updateFileWithMarkers`. - `path-containment.test.ts`: exercise `FileSystemUtils.assertPathWithin` and `resolveProjectArtifactPath` instead of a private copy of the containment logic, which passed whatever the production guard did. The guard had no coverage at all; a prefix-comparison regression now fails the sibling case. Co-Authored-By: Claude Opus 5 --- .changeset/preserve-crlf-line-endings.md | 2 + src/utils/file-system.ts | 9 +- test/utils/marker-updates.test.ts | 51 ++++++++- test/utils/path-containment.test.ts | 126 +++++++++++++++-------- 4 files changed, 143 insertions(+), 45 deletions(-) diff --git a/.changeset/preserve-crlf-line-endings.md b/.changeset/preserve-crlf-line-endings.md index e42f4943a7..55edaa7a2d 100644 --- a/.changeset/preserve-crlf-line-endings.md +++ b/.changeset/preserve-crlf-line-endings.md @@ -6,4 +6,6 @@ Preserve a file's existing line endings when rewriting it, so Windows users no l 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`. +Removing a managed block is fixed the same way: the blank-line collapse in `removeMarkerBlock` rebuilt its separator as a bare LF, so cleaning up legacy artifacts left a lone LF inside an otherwise-CRLF `CLAUDE.md` or rc file. + `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/src/utils/file-system.ts b/src/utils/file-system.ts index c9f901a4be..0d5fce0586 100644 --- a/src/utils/file-system.ts +++ b/src/utils/file-system.ts @@ -451,14 +451,19 @@ export function removeMarkerBlock( const before = content.substring(0, lineStart); const after = content.substring(lineEnd); + // The file's own newline, used for every ending this function writes. The + // blank-line collapse below rebuilds the separator it matched, so spelling it + // '\n' would leave a CRLF file with a mixed pair wherever a run was collapsed + // - the stray '\r' that bash reports as "$'\r': command not found". + const newline = content.includes('\r\n') ? '\r\n' : '\n'; + // Clean up double blank lines (handle both Unix \n and Windows \r\n) let result = before + after; - result = result.replace(/(\r?\n){3,}/g, '\n\n'); + result = result.replace(/(\r?\n){3,}/g, newline + newline); // Trim trailing whitespace but preserve leading whitespace and original newline style if (result.trimEnd() === '') { return ''; } - const newline = content.includes('\r\n') ? '\r\n' : '\n'; return result.trimEnd() + newline; } diff --git a/test/utils/marker-updates.test.ts b/test/utils/marker-updates.test.ts index 669db45dc8..9a78e3909c 100644 --- a/test/utils/marker-updates.test.ts +++ b/test/utils/marker-updates.test.ts @@ -351,6 +351,7 @@ ${END_MARKER} const result = await fs.readFile(filePath, 'utf-8'); expect(countEndings(result).crlf).toBe(0); }); + }); }); describe('removeMarkerBlock', () => { @@ -490,6 +491,54 @@ After block content`; }); }); + describe('line endings', () => { + const MD_START = ''; + const MD_END = ''; + + it('collapses a blank-line run without leaving a lone LF in a CRLF file', () => { + // The collapse rebuilds the separator it matched. Spelling that '\n' + // puts a lone LF into an otherwise-CRLF file, which is the mixed ending + // bash reports as "$'\r': command not found" in a .bashrc. + const content = [ + '# User config', + '', + '', + MD_START, + 'managed', + MD_END, + '', + '', + '# More user config', + ].join('\r\n'); + + const result = removeMarkerBlock(content, MD_START, MD_END); + + expect(result.match(/(? { + const content = [ + '# User config', + '', + '', + MD_START, + 'managed', + MD_END, + '', + '', + '# More user config', + ].join('\n'); + + const result = removeMarkerBlock(content, MD_START, MD_END); + + expect(result).not.toContain('\r'); + expect(result).toContain('# More user config'); + }); + }); + describe('shell markers', () => { const SHELL_START = '# OPENSPEC:START'; const SHELL_END = '# OPENSPEC:END'; @@ -512,6 +561,4 @@ 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 index 51e5216d9e..75968a420e 100644 --- a/test/utils/path-containment.test.ts +++ b/test/utils/path-containment.test.ts @@ -1,63 +1,107 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import path from 'node:path'; +import { FileSystemUtils } from '../../src/utils/file-system.js'; /** - * 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. + * Coverage for the containment guard itself, rather than for a copy of it. * - * 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. + * `assertPathWithin` is what keeps every managed write inside the project, so + * the contract worth pinning is the guard's own: what it accepts, what it + * throws on, and that it reads a path as path segments rather than as a string + * prefix. `openspec-evil` starts with `openspec` and must still be rejected. + * + * The cases run through real directories because the guard canonicalizes + * before deciding, so a purely notional path would not exercise it. */ +describe('FileSystemUtils.assertPathWithin', () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(path.join(tmpdir(), 'openspec-containment-')); + }); -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)) - ); -} + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); -describe('path containment under Windows case variance', () => { - const root = 'C:\\Repo\\openspec'; + it('accepts a path inside the allowed directory', () => { + const inside = path.join(root, 'specs', 'widgets', 'spec.md'); + expect(() => FileSystemUtils.assertPathWithin(root, inside)).not.toThrow(); + }); + + it('accepts the allowed directory itself', () => { + expect(() => FileSystemUtils.assertPathWithin(root, root)).not.toThrow(); + }); - it('accepts a target whose drive letter differs in case', () => { - expect(isPathWithin(root, 'c:\\Repo\\openspec\\specs\\x', path.win32)).toBe(true); + it('rejects a sibling that merely shares the root as a string prefix', () => { + // `${root}-evil` starts with `${root}`, so a prefix comparison would let it + // through. The guard compares path segments, so it must not. + const sibling = `${root}-evil`; + mkdirSync(sibling, { recursive: true }); + try { + expect(() => FileSystemUtils.assertPathWithin(root, sibling)).toThrow( + /outside the allowed directory/ + ); + } finally { + rmSync(sibling, { recursive: true, force: true }); + } }); - it('accepts a target whose directory differs in case', () => { - expect(isPathWithin(root, 'C:\\REPO\\openspec\\specs\\x', path.win32)).toBe(true); + it('rejects a traversal escape', () => { + const escape = path.join(root, '..', 'elsewhere'); + expect(() => FileSystemUtils.assertPathWithin(root, escape)).toThrow( + /outside the allowed directory/ + ); + }); + + it('rejects the parent of the allowed directory', () => { + expect(() => FileSystemUtils.assertPathWithin(root, path.dirname(root))).toThrow( + /outside the allowed directory/ + ); + }); +}); + +describe('FileSystemUtils.resolveProjectArtifactPath', () => { + let project: string; + + beforeEach(() => { + project = mkdtempSync(path.join(tmpdir(), 'openspec-artifact-')); }); - it('accepts the root itself', () => { - expect(isPathWithin(root, root, path.win32)).toBe(true); - expect(isPathWithin(root, 'c:\\repo\\OPENSPEC', path.win32)).toBe(true); + afterEach(() => { + rmSync(project, { recursive: true, force: true }); }); - it('still rejects a sibling directory outside the root', () => { - expect(isPathWithin(root, 'C:\\Repo\\other\\x', path.win32)).toBe(false); + it('resolves a relative artifact path inside the project', () => { + const resolved = FileSystemUtils.resolveProjectArtifactPath( + project, + path.join('openspec', 'project.md') + ); + expect(resolved).toBe(path.join(project, 'openspec', 'project.md')); }); - it('still rejects a traversal escape', () => { - expect(isPathWithin(root, 'C:\\Repo\\openspec\\..\\other', path.win32)).toBe(false); + it('accepts a separator-joined artifact path on this platform', () => { + // Artifact paths are composed with path.join, so the separator the guard + // sees is the platform's own. Both halves must survive the round trip. + const resolved = FileSystemUtils.resolveProjectArtifactPath( + project, + path.join('openspec', 'changes', 'add-widgets', 'tasks.md') + ); + expect(resolved.startsWith(project + path.sep)).toBe(true); + expect(resolved.endsWith(path.join('add-widgets', 'tasks.md'))).toBe(true); }); - it('still rejects a path on another drive', () => { - expect(isPathWithin(root, 'D:\\Repo\\openspec\\specs', path.win32)).toBe(false); + it('refuses an absolute artifact path', () => { + expect(() => + FileSystemUtils.resolveProjectArtifactPath(project, path.resolve(project, 'openspec')) + ).toThrow(/Refusing to manage an artifact outside the project/); }); - 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); + it('refuses an artifact path that climbs out of the project', () => { + expect(() => + FileSystemUtils.resolveProjectArtifactPath(project, path.join('..', 'escape.md')) + ).toThrow(/outside the allowed directory/); }); }); From 32feb04cdbe40dff27e7363cd79c71af311908e2 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 22 Sep 2026 16:10:01 -0500 Subject: [PATCH 3/3] fix(windows): read the file's convention consistently, and only ENOENT as absent Three follow-ups from CodeRabbit's pass on the superseding PR. `writeUpdatedSpec` turned every read error into "no previous file", so an existing but unreadable spec was treated as absent and rewritten as LF. Only ENOENT means absent now; everything else propagates. `removeMarkerBlock` chose CRLF whenever the content held one anywhere, so a single stray CRLF in an otherwise-LF file pulled the whole rewrite to CRLF. It now uses detectLineEnding, the same dominant-ending reading matchLineEnding uses, so both write paths agree. Added the alias-path case the containment suite was missing: a directory link inside the root that resolves outside it. That exercises the canonicalization half of the guard, which a lexical check cannot do - the link's own path looks contained. Skipped where creating a directory link needs a privilege the runner lacks. Co-Authored-By: Claude Opus 5 --- .changeset/preserve-crlf-line-endings.md | 2 +- src/core/specs-apply.ts | 7 +++++- src/utils/file-system.ts | 8 +++++-- test/utils/marker-updates.test.ts | 15 +++++++++++++ test/utils/path-containment.test.ts | 27 +++++++++++++++++++++++- 5 files changed, 54 insertions(+), 5 deletions(-) diff --git a/.changeset/preserve-crlf-line-endings.md b/.changeset/preserve-crlf-line-endings.md index 55edaa7a2d..786266c77e 100644 --- a/.changeset/preserve-crlf-line-endings.md +++ b/.changeset/preserve-crlf-line-endings.md @@ -6,6 +6,6 @@ Preserve a file's existing line endings when rewriting it, so Windows users no l 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`. -Removing a managed block is fixed the same way: the blank-line collapse in `removeMarkerBlock` rebuilt its separator as a bare LF, so cleaning up legacy artifacts left a lone LF inside an otherwise-CRLF `CLAUDE.md` or rc file. +Removing a managed block is fixed the same way: the blank-line collapse in `removeMarkerBlock` rebuilt its separator as a bare LF, so cleaning up legacy artifacts left a lone LF inside an otherwise-CRLF `CLAUDE.md` or rc file. Both write paths now read the file the same way, by dominant ending, so one stray CRLF in an otherwise-LF file no longer pulls the whole rewrite to CRLF. `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/src/core/specs-apply.ts b/src/core/specs-apply.ts index 893a702a48..eec89db486 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -1250,7 +1250,12 @@ export async function writeUpdatedSpec( // 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); + // Only a missing file means "no convention to match". Swallowing every error + // would read an existing but unreadable spec as absent and rewrite it as LF. + const previous = await fs.readFile(update.target, 'utf-8').catch((error) => { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw error; + }); const toWrite = previous === undefined ? rebuilt : matchLineEnding(rebuilt, previous); await options.beforeMutate?.(); diff --git a/src/utils/file-system.ts b/src/utils/file-system.ts index 0d5fce0586..a64270b6f1 100644 --- a/src/utils/file-system.ts +++ b/src/utils/file-system.ts @@ -1,6 +1,6 @@ import * as nodeFs from 'fs'; import path from 'path'; -import { matchLineEnding } from './line-endings.js'; +import { detectLineEnding, matchLineEnding } from './line-endings.js'; const fs = nodeFs.promises; const { constants: fsConstants } = nodeFs; @@ -455,7 +455,11 @@ export function removeMarkerBlock( // blank-line collapse below rebuilds the separator it matched, so spelling it // '\n' would leave a CRLF file with a mixed pair wherever a run was collapsed // - the stray '\r' that bash reports as "$'\r': command not found". - const newline = content.includes('\r\n') ? '\r\n' : '\n'; + // + // Dominant rather than "contains a CRLF anywhere", so that one stray CRLF in + // an otherwise-LF file does not pull the whole rewrite to CRLF. This is the + // same reading matchLineEnding uses, so both write paths agree. + const newline = detectLineEnding(content) ?? '\n'; // Clean up double blank lines (handle both Unix \n and Windows \r\n) let result = before + after; diff --git a/test/utils/marker-updates.test.ts b/test/utils/marker-updates.test.ts index 9a78e3909c..2535172ab8 100644 --- a/test/utils/marker-updates.test.ts +++ b/test/utils/marker-updates.test.ts @@ -519,6 +519,21 @@ After block content`; expect(result).not.toContain('managed'); }); + it('follows the dominant ending, not a single stray CRLF', () => { + // One stray CRLF in an otherwise-LF file must not pull the rewrite to + // CRLF. This is the reading matchLineEnding uses, so both write paths + // agree on what the file's convention is. + const content = + '# User config\r\n' + + ['', '', MD_START, 'managed', MD_END, '', '', '# More user config'].join('\n'); + + const result = removeMarkerBlock(content, MD_START, MD_END); + + expect(result.endsWith('\n')).toBe(true); + expect(result.endsWith('\r\n')).toBe(false); + expect(result).toContain('# More user config'); + }); + it('leaves an LF file on LF when collapsing the same run', () => { const content = [ '# User config', diff --git a/test/utils/path-containment.test.ts b/test/utils/path-containment.test.ts index 75968a420e..010ddbf75b 100644 --- a/test/utils/path-containment.test.ts +++ b/test/utils/path-containment.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, mkdirSync, rmSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, rmSync, symlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { FileSystemUtils } from '../../src/utils/file-system.js'; @@ -49,6 +49,31 @@ describe('FileSystemUtils.assertPathWithin', () => { } }); + it('rejects a directory link inside the root that resolves outside it', () => { + // The guard canonicalizes before deciding, which is the half that a + // lexical containment check cannot do: the link's own path looks inside. + const outside = mkdtempSync(path.join(tmpdir(), 'openspec-outside-')); + const link = path.join(root, 'linked'); + try { + symlinkSync(outside, link, 'junction'); + } catch { + // Creating a directory link needs a privilege the runner may not have. + rmSync(outside, { recursive: true, force: true }); + return; + } + try { + expect(() => FileSystemUtils.assertPathWithin(root, link)).toThrow( + /outside the allowed directory/ + ); + expect(() => + FileSystemUtils.assertPathWithin(root, path.join(link, 'spec.md')) + ).toThrow(/outside the allowed directory/); + } finally { + rmSync(link, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }); + it('rejects a traversal escape', () => { const escape = path.join(root, '..', 'elsewhere'); expect(() => FileSystemUtils.assertPathWithin(root, escape)).toThrow(