fix(windows): preserve a file's existing line endings on rewrite

.changeset/preserve-crlf-line-endings.md

@@ -0,0 +1,11 @@
+---
+"@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`.
+
+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`.

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() {

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,24 @@ 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.
+  // 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?.();
   // 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;

src/utils/file-system.ts

@@ -1,5 +1,6 @@
 import * as nodeFs from 'fs';
 import path from 'path';
+import { detectLineEnding, matchLineEnding } from './line-endings.js';
 
 const fs = nodeFs.promises;
 const { constants: fsConstants } = nodeFs;
@@ -325,10 +326,16 @@ export class FileSystemUtils {
     endMarker: string
   ): Promise<void> {
     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<boolean> {
@@ -439,14 +451,23 @@ 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".
+  //
+  // 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;
-  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;
 }

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(/(?<!\r)\n/g)?.length ?? 0;
+
+  if (crlf === 0 && lf === 0) return undefined;
+  return crlf >= 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);
+}

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(/(?<!\r)\n/g)?.length ?? 0;
+  return { crlf, loneLf };
+}
+
+describe('spec line-ending preservation', () => {
+  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<string> {
+    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);
+  });
+});