From 83b1ab0520f41ba7e54a7b87d787ac7bcb9a3394 Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:10:43 +0530 Subject: [PATCH 1/8] fix(build): make a fresh clone build on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fresh clone could not build on Windows at all. Three independent causes, reported by @kevin101681 in #11: - check-css.mjs resolved the styles directory with `new URL(...).pathname`, which yields `/C:/Users/...`; readdirSync resolves that against the current drive and looks for `C:\C:\Users\...`. fileURLToPath is also the fix for a latent bug everywhere else: `.pathname` stays percent-encoded, so a checkout under a directory with a space in it fails on macOS and Linux too. - vendor-assets.mjs derived asset names with `from.split("/").pop()`, a no-op on the backslash paths require.resolve returns, so the whole absolute source path was appended to the destination. path.basename handles both separators. - The front-matter regexes in the three gen.mjs scripts were anchored to `^---\n`, and with core.autocrlf=true — the Git-for-Windows default — the markdown specs check out as CRLF, so they reported a missing front-matter block rather than a wrong one. .gitattributes pins every checkout to LF, which prevents the third from recurring. The regexes take `\r?\n` anyway: .gitattributes only applies on checkout, so it does nothing for a clone made before it existed, a zip download, a patch, or an editor configured to write CRLF. sync-readme.mjs had the same defect one step further on, and it was the worse one — it splits on "\n" but rejoins the banner with LF, so on a CRLF tree the --check comparison could never match the file on disk and a Windows contributor had no way to make that CI gate pass. --- .gitattributes | 4 ++++ apps/cli/scripts/vendor-assets.mjs | 9 ++++++--- packages/editor-icons/scripts/gen.mjs | 4 +++- packages/editor-tokens/scripts/gen.mjs | 4 +++- packages/overlay/scripts/check-css.mjs | 8 +++++++- packages/site-tokens/scripts/gen.mjs | 4 +++- scripts/sync-readme.mjs | 12 ++++++++++-- 7 files changed, 36 insertions(+), 9 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..f9c77dd --- /dev/null +++ b/.gitattributes @@ -0,0 +1,4 @@ +# Check out everything with LF, even on Windows (core.autocrlf=true would +# otherwise write CRLF, which breaks the front-matter regexes in the +# editor-tokens/editor-icons/site-tokens gen scripts). +* text=auto eol=lf diff --git a/apps/cli/scripts/vendor-assets.mjs b/apps/cli/scripts/vendor-assets.mjs index 924db13..66ee13d 100644 --- a/apps/cli/scripts/vendor-assets.mjs +++ b/apps/cli/scripts/vendor-assets.mjs @@ -17,7 +17,7 @@ import { copyFileSync, existsSync, mkdirSync } from "node:fs"; import { createRequire } from "node:module"; -import { dirname, join } from "node:path"; +import { basename, join } from "node:path"; import { fileURLToPath } from "node:url"; const require = createRequire(import.meta.url); @@ -50,7 +50,10 @@ function resolve(specifier, what) { // serves `${bundle}.map` derived from whatever path it resolved, so the map has // to land beside its bundle here too. function copyWithMap(from, toDir) { - const name = from.split("/").pop(); + // basename, not split("/"): require.resolve returns a native path, so on + // Windows there is no "/" to split on and `name` would come back as the whole + // absolute path — which join() then appends to the destination wholesale. + const name = basename(from); copyFileSync(from, join(toDir, name)); if (existsSync(`${from}.map`)) { copyFileSync(`${from}.map`, join(toDir, `${name}.map`)); @@ -87,5 +90,5 @@ for (const name of expected) { } console.log( - `vendor-assets: ${expected.length} bundles + ${FONTS.length} fonts -> ${dirname(vendorDir)}/vendor` + `vendor-assets: ${expected.length} bundles + ${FONTS.length} fonts -> ${vendorDir}` ); diff --git a/packages/editor-icons/scripts/gen.mjs b/packages/editor-icons/scripts/gen.mjs index 9499373..5e01b42 100644 --- a/packages/editor-icons/scripts/gen.mjs +++ b/packages/editor-icons/scripts/gen.mjs @@ -39,7 +39,9 @@ if (!existsSync(specPath)) { } const raw = readFileSync(specPath, "utf8"); -const match = raw.match(/^---\n([\s\S]*?)\n---/); +// \r? because a Windows checkout with core.autocrlf=true hands us CRLF, and a +// front-matter block that opens `---\r\n` would otherwise read as absent. +const match = raw.match(/^---\r?\n([\s\S]*?)\r?\n---/); if (!match) { throw new Error("gen: ICONS.md is missing its YAML front-matter block"); } diff --git a/packages/editor-tokens/scripts/gen.mjs b/packages/editor-tokens/scripts/gen.mjs index 2313715..ad2a8b7 100644 --- a/packages/editor-tokens/scripts/gen.mjs +++ b/packages/editor-tokens/scripts/gen.mjs @@ -16,7 +16,9 @@ if (!existsSync(specPath)) { } const raw = readFileSync(specPath, "utf8"); -const match = raw.match(/^---\n([\s\S]*?)\n---/); +// \r? because a Windows checkout with core.autocrlf=true hands us CRLF, and a +// front-matter block that opens `---\r\n` would otherwise read as absent. +const match = raw.match(/^---\r?\n([\s\S]*?)\r?\n---/); if (!match) { throw new Error("gen: EDITOR.md is missing its YAML front-matter block"); } diff --git a/packages/overlay/scripts/check-css.mjs b/packages/overlay/scripts/check-css.mjs index d03950c..a06acd4 100644 --- a/packages/overlay/scripts/check-css.mjs +++ b/packages/overlay/scripts/check-css.mjs @@ -16,8 +16,14 @@ */ import { readdirSync, readFileSync } from "node:fs"; import { join } from "node:path"; +import { fileURLToPath } from "node:url"; -const DIR = new URL("../src/styles/", import.meta.url).pathname; +// fileURLToPath, not `.pathname`. `.pathname` is a URL component, not a path: +// on Windows it yields `/C:/…`, which readdirSync resolves against the current +// drive as `C:\C:\…`, and on every platform it stays percent-encoded, so a +// checkout under a directory with a space in it fails too. `join(DIR, file)` +// below needs a string, so this cannot stay a URL. +const DIR = fileURLToPath(new URL("../src/styles/", import.meta.url)); const START = /export const css\s*=\s*`/; const problems = []; diff --git a/packages/site-tokens/scripts/gen.mjs b/packages/site-tokens/scripts/gen.mjs index aa173dc..3e28a2c 100644 --- a/packages/site-tokens/scripts/gen.mjs +++ b/packages/site-tokens/scripts/gen.mjs @@ -16,7 +16,9 @@ if (!existsSync(designPath)) { } const raw = readFileSync(designPath, "utf8"); -const match = raw.match(/^---\n([\s\S]*?)\n---/); +// \r? because a Windows checkout with core.autocrlf=true hands us CRLF, and a +// front-matter block that opens `---\r\n` would otherwise read as absent. +const match = raw.match(/^---\r?\n([\s\S]*?)\r?\n---/); if (!match) { throw new Error("gen: DESIGN.md is missing its YAML front-matter block"); } diff --git a/scripts/sync-readme.mjs b/scripts/sync-readme.mjs index 0bf56b5..3b28b23 100644 --- a/scripts/sync-readme.mjs +++ b/scripts/sync-readme.mjs @@ -33,6 +33,10 @@ const LINK = /(!?)\[([^\]]*)\]\(([^)\s]+)\)/g; /** ``` or ~~~ opening or closing a fenced block, which is never rewritten. */ const FENCE = /^\s*(?:```|~~~)/; /** A scheme, a protocol-relative host, or an anchor — already resolvable. */ +// Matches CRLF as readily as LF: a Windows working tree would otherwise leave a +// trailing \r on every line, and the rejoin below emits LF, so the --check +// comparison could never match the file on disk. +const LINE_BREAK = /\r?\n/; const ABSOLUTE = /^(?:[a-z][a-z0-9+.-]*:|\/\/|#)/i; const LEADING_DOT_SLASH = /^\.\//; const REPO_URL = /github\.com[/:]([^/]+)\/([^/.]+)/; @@ -94,7 +98,7 @@ function render(source, slug) { return `${bang}[${label}](${absolutize(target, bang === "!", slug)})`; }); - for (const line of source.split("\n")) { + for (const line of source.split(LINE_BREAK)) { if (FENCE.test(line)) { inFence = !inFence; out.push(line); @@ -115,7 +119,11 @@ if (process.argv.includes("--check")) { } catch { die("apps/cli/README.md is missing — run `node scripts/sync-readme.mjs`"); } - if (current !== text) { + // Compare on content, not line endings. .gitattributes pins a fresh checkout + // to LF, but a file that arrived some other way — an existing clone, a zip, a + // patch, an editor configured for CRLF — would otherwise report as eternally + // stale with no way for the contributor to make it pass. + if (current.replace(/\r\n/g, "\n") !== text) { die( "apps/cli/README.md is stale — run `node scripts/sync-readme.mjs` and commit the result" ); From ff818c114545ea89263e1a2a4b98353310f19fe4 Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:11:04 +0530 Subject: [PATCH 2/8] build: replace the rm -rf clean scripts with a cross-platform runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every workspace `clean` was `rm -rf`, and pnpm runs lifecycle scripts through the platform shell — on Windows that is cmd.exe, which has no `rm`. The root `clean` fans out to all eleven packages through turbo, so `pnpm clean` failed eleven times over. A shared script rather than a `rimraf` dependency: it needs no install to work, nothing to keep pinned against the release-age gate in pnpm-workspace.yaml, and no argument that has to survive two different shells' quoting rules. Node accepts forward slashes on Windows, so `node ../../scripts/clean.mjs dist .turbo` is the same string in every package. It deletes recursively and by force, so it refuses any target that is not strictly inside the calling package — a typo in a package.json should not be able to reach the workspace root. --- apps/cli/package.json | 2 +- apps/web/package.json | 2 +- packages/core/package.json | 2 +- packages/editor-icons/package.json | 2 +- packages/editor-tokens/package.json | 2 +- packages/git/package.json | 2 +- packages/overlay/package.json | 2 +- packages/protocol/package.json | 2 +- packages/server/package.json | 2 +- packages/site-tokens/package.json | 2 +- packages/source/package.json | 2 +- scripts/clean.mjs | 43 +++++++++++++++++++++++++++++ 12 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 scripts/clean.mjs diff --git a/apps/cli/package.json b/apps/cli/package.json index 221afd0..c810737 100644 --- a/apps/cli/package.json +++ b/apps/cli/package.json @@ -31,7 +31,7 @@ ], "scripts": { "build": "tsup && node scripts/vendor-assets.mjs", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup --watch", "test": "vitest run", "typecheck": "tsc --noEmit" diff --git a/apps/web/package.json b/apps/web/package.json index 67844fb..5176516 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -8,7 +8,7 @@ }, "scripts": { "build": "vite build", - "clean": "rm -rf dist .output .nitro .tanstack node_modules/.vite .turbo", + "clean": "node ../../scripts/clean.mjs dist .output .nitro .tanstack node_modules/.vite .turbo", "dev": "vite dev", "og": "node scripts/og.mjs", "routes": "tsr generate", diff --git a/packages/core/package.json b/packages/core/package.json index 770df56..171662e 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -13,7 +13,7 @@ "types": "./dist/index.d.ts", "scripts": { "build": "tsup src/index.ts --format esm --dts --sourcemap --clean", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup src/index.ts --format esm --dts --sourcemap --watch", "test": "vitest run", "typecheck": "tsc --noEmit" diff --git a/packages/editor-icons/package.json b/packages/editor-icons/package.json index 9c714f4..060d0bb 100644 --- a/packages/editor-icons/package.json +++ b/packages/editor-icons/package.json @@ -16,7 +16,7 @@ ], "scripts": { "build": "node scripts/gen.mjs && tsup src/index.ts --format esm --dts --sourcemap --clean", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "node scripts/gen.mjs && tsup src/index.ts --format esm --dts --sourcemap --watch", "gen": "node scripts/gen.mjs", "typecheck": "tsc --noEmit" diff --git a/packages/editor-tokens/package.json b/packages/editor-tokens/package.json index 56bb7ef..273453f 100644 --- a/packages/editor-tokens/package.json +++ b/packages/editor-tokens/package.json @@ -19,7 +19,7 @@ ], "scripts": { "build": "node scripts/gen.mjs && tsup src/index.ts --format esm --dts --sourcemap --clean && node scripts/postbuild.mjs", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "node scripts/gen.mjs && tsup src/index.ts --format esm --dts --sourcemap --watch", "gen": "node scripts/gen.mjs", "typecheck": "tsc --noEmit" diff --git a/packages/git/package.json b/packages/git/package.json index 5b441ac..e6a9b77 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -13,7 +13,7 @@ "types": "./dist/index.d.ts", "scripts": { "build": "tsup src/index.ts --format esm --dts --sourcemap --clean", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup src/index.ts --format esm --dts --sourcemap --watch", "typecheck": "tsc --noEmit" }, diff --git a/packages/overlay/package.json b/packages/overlay/package.json index 11f87cc..e0b1be3 100644 --- a/packages/overlay/package.json +++ b/packages/overlay/package.json @@ -11,7 +11,7 @@ "build": "node scripts/check-css.mjs && tsup", "build-storybook": "storybook build", "check:css": "node scripts/check-css.mjs", - "clean": "rm -rf dist .turbo storybook-static", + "clean": "node ../../scripts/clean.mjs dist .turbo storybook-static", "dev": "tsup --watch", "storybook": "storybook dev -p 6006 --no-open", "test": "vitest run", diff --git a/packages/protocol/package.json b/packages/protocol/package.json index c845f4e..044d0e5 100644 --- a/packages/protocol/package.json +++ b/packages/protocol/package.json @@ -17,7 +17,7 @@ "types": "./dist/index.d.ts", "scripts": { "build": "tsup src/index.ts src/tokens.ts --format esm --dts --sourcemap --clean", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup src/index.ts src/tokens.ts --format esm --dts --sourcemap --watch", "test": "vitest run", "typecheck": "tsc --noEmit" diff --git a/packages/server/package.json b/packages/server/package.json index 6088e44..9450f58 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -13,7 +13,7 @@ "types": "./dist/index.d.ts", "scripts": { "build": "tsup src/index.ts --format esm --dts --sourcemap --clean", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup src/index.ts --format esm --dts --sourcemap --watch", "test": "vitest run", "typecheck": "tsc --noEmit" diff --git a/packages/site-tokens/package.json b/packages/site-tokens/package.json index ba74653..50d3c28 100644 --- a/packages/site-tokens/package.json +++ b/packages/site-tokens/package.json @@ -20,7 +20,7 @@ ], "scripts": { "build": "node scripts/gen.mjs && tsup src/index.ts --format esm --dts --sourcemap --clean && node scripts/postbuild.mjs", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "gen": "node scripts/gen.mjs", "typecheck": "tsc --noEmit" }, diff --git a/packages/source/package.json b/packages/source/package.json index 75542b3..0ff4d8f 100644 --- a/packages/source/package.json +++ b/packages/source/package.json @@ -19,7 +19,7 @@ }, "scripts": { "build": "tsup src/browser.ts src/server.ts src/tokens.ts --format esm --dts --sourcemap --clean", - "clean": "rm -rf dist .turbo", + "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup src/browser.ts src/server.ts src/tokens.ts --format esm --dts --sourcemap --watch", "test": "vitest run", "typecheck": "tsc --noEmit" diff --git a/scripts/clean.mjs b/scripts/clean.mjs new file mode 100644 index 0000000..f321226 --- /dev/null +++ b/scripts/clean.mjs @@ -0,0 +1,43 @@ +// Removes build output for the package that invokes it. +// +// Why this exists: every workspace `clean` script used to be `rm -rf dist +// .turbo`, and `rm` does not exist on Windows. pnpm runs lifecycle scripts +// through the platform shell, so on Windows that is cmd.exe and all eleven of +// them died with "'rm' is not recognized". `turbo run clean` fans out to every +// package, so `pnpm clean` failed eleven times over. +// +// A shared script rather than a `rimraf` dependency: this needs no install to +// work, nothing to keep pinned against the release-age gate in +// pnpm-workspace.yaml, and no argument that has to survive two different +// shells' quoting rules. Node resolves forward slashes on Windows, so +// `node ../../scripts/clean.mjs dist .turbo` is literally the same string +// everywhere. +// +// Usage, from a package directory: node ../../scripts/clean.mjs ... + +import { rmSync } from "node:fs"; +import { isAbsolute, relative, resolve, sep } from "node:path"; + +const targets = process.argv.slice(2); + +if (targets.length === 0) { + process.stderr.write("clean: nothing to remove (pass one or more paths)\n"); + process.exit(1); +} + +const cwd = process.cwd(); + +for (const target of targets) { + // This deletes recursively and by force, so it refuses anything that is not + // strictly inside the calling package. A typo in a package.json should not be + // able to reach the workspace root. + const abs = resolve(cwd, target); + const rel = relative(cwd, abs); + if (isAbsolute(target) || rel === "" || rel.startsWith(`..${sep}`)) { + process.stderr.write( + `clean: refusing to remove "${target}" — it escapes ${cwd}\n` + ); + process.exit(1); + } + rmSync(abs, { force: true, recursive: true }); +} From b60eabfc7c94f8e7117cfa435583ff2ca66b8447 Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:11:29 +0530 Subject: [PATCH 3/8] fix(core): give paths and line endings one identity across platforms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four features key on a path — `--safe` containment, DiffCapture's before/after map, the history store's per-repo directory, and the editor handler's containment check — and each canonicalized differently, or not at all. They now share ./paths. `realpathSync` was never enough on Windows: it resolves links while preserving whatever spelling the caller passed, and Windows filesystems are case-insensitive, so `C:\Proj` and `c:\proj` compared unequal. Drive-letter case alone triggers it — process.cwd() upper-cases it while `--cwd c:/proj` does not. `realpathSync.native` canonicalizes case through GetFinalPathNameByHandle. The failures were all quiet: `--safe` refusing edits inside the project, DiffCapture attributing the user's own uncommitted work to the agent and undoing it with the turn. isPathInside also appended a separator to a root that already ended in one, so a project at a filesystem root matched nothing at all. repoDir keeps a read-only fallback to the pre-canonicalization directory, or existing history would vanish on upgrade for anyone whose project is reached through a symlink. Line endings get the same treatment in DiffCapture. With core.autocrlf=true the working tree is CRLF while every agent's edit tool writes LF — and the HEAD blob the Codex path reads back is LF too — so a one-line edit diffed as a whole-file rewrite with garbage counts and review comments anchored to the wrong lines. A leading BOM, which Visual Studio writes and edit tools drop, did the same to the first line. Both are flattened for comparison and diffing only; FileDiff.before/after keep the bytes on disk, because restoreFiles writes `before` back verbatim and undo has to round-trip exactly. Paths leaving the filesystem are now forward-slashed: a git pathspec (where backslash is wildmatch's escape character), the JSON an MCP tool returns (where each backslash arrives doubled), and the overlay's display labels. The `--safe` command screen was POSIX-shell shaped throughout, so on Windows it was a no-op while the launch banner still said it was on. Each added pattern names the flags the real command takes rather than matching the verb loosely — a bare /\bdel\s+\// fires on `sed -i 's/del /x/'`, and a false deny is not free. --- packages/core/src/diff-capture.test.ts | 56 +++++++++ packages/core/src/diff-capture.ts | 69 ++++++++---- packages/core/src/index.ts | 7 ++ packages/core/src/paths.test.ts | 144 ++++++++++++++++++++++++ packages/core/src/paths.ts | 100 ++++++++++++++++ packages/core/src/providers/claude.ts | 3 +- packages/core/src/providers/opencode.ts | 19 +++- packages/core/src/sandbox.test.ts | 125 ++++++++++---------- packages/core/src/sandbox.ts | 59 ++++------ packages/overlay/src/app.ts | 7 +- packages/overlay/src/dom.ts | 16 +++ packages/overlay/src/inspector/panel.ts | 10 +- packages/server/src/history.ts | 29 ++++- packages/source/src/server.test.ts | 70 ++++++++++++ packages/source/src/server.ts | 54 +++++++-- packages/source/src/tokens.ts | 4 +- packages/source/src/walk.ts | 14 ++- 17 files changed, 634 insertions(+), 152 deletions(-) create mode 100644 packages/core/src/paths.test.ts create mode 100644 packages/core/src/paths.ts create mode 100644 packages/source/src/server.test.ts diff --git a/packages/core/src/diff-capture.test.ts b/packages/core/src/diff-capture.test.ts index b3566a3..80239f8 100644 --- a/packages/core/src/diff-capture.test.ts +++ b/packages/core/src/diff-capture.test.ts @@ -125,3 +125,59 @@ describe("DiffCapture on the Codex path", () => { expect(diff.before).toBe("one\ntwo\nthree\n"); }); }); + +/** + * The Windows shape, reproducible anywhere: `core.autocrlf=true` leaves a CRLF + * working tree, every agent's edit tool writes LF, and the HEAD blob the Codex + * path reads back is LF too. Comparing those byte-for-byte makes every line + * differ by its terminator, so a one-line edit reports as a whole-file rewrite. + */ +describe("DiffCapture with CRLF line endings", () => { + it("diffs only the line that changed, not every line", () => { + write("crlf.txt", "one\r\ntwo\r\nthree\r\n"); + const dc = new DiffCapture(repo); + dc.recordBefore("crlf.txt"); + // The agent rewrites the file with LF, changing exactly one line. + write("crlf.txt", "one\nTWO\nthree\n"); + + const [diff] = dc.finalize(); + expect(diff.additions).toBe(1); + expect(diff.deletions).toBe(1); + }); + + it("keeps the bytes on disk in before/after so undo round-trips exactly", () => { + // The patch is normalized for display; `restoreFiles` writes `before` back + // verbatim, so it has to carry the CRLF the file actually had. + write("crlf.txt", "one\r\ntwo\r\n"); + const dc = new DiffCapture(repo); + dc.recordBefore("crlf.txt"); + write("crlf.txt", "one\nCHANGED\n"); + + const [diff] = dc.finalize(); + expect(diff.before).toBe("one\r\ntwo\r\n"); + expect(diff.after).toBe("one\nCHANGED\n"); + }); + + it("does not report a first-line change when only the BOM was dropped", () => { + // Visual Studio writes a BOM; agent edit tools generally do not put it + // back, and readFileSync surfaces it as a real character. + write("bom.txt", "one\ntwo\n"); + const dc = new DiffCapture(repo); + dc.recordBefore("bom.txt"); + write("bom.txt", "one\nCHANGED\n"); + + const [diff] = dc.finalize(); + expect(diff.additions).toBe(1); + expect(diff.deletions).toBe(1); + }); + + it("reports no change when only the line endings were rewritten", () => { + write("crlf.txt", "one\r\ntwo\r\n"); + const dc = new DiffCapture(repo); + dc.recordBefore("crlf.txt"); + write("crlf.txt", "one\ntwo\n"); + + expect(dc.finalize()).toEqual([]); + expect(dc.pairFor("crlf.txt")).toBeNull(); + }); +}); diff --git a/packages/core/src/diff-capture.ts b/packages/core/src/diff-capture.ts index 0678f3c..c7599fa 100644 --- a/packages/core/src/diff-capture.ts +++ b/packages/core/src/diff-capture.ts @@ -13,10 +13,14 @@ * everything else from the file's HEAD blob. See the two methods for why that * covers every case. */ -import { existsSync, readFileSync, realpathSync } from "node:fs"; -import { basename, dirname, relative, resolve } from "node:path"; +import { existsSync, readFileSync } from "node:fs"; +import { relative, resolve } from "node:path"; import type { FileDiff } from "@airship/protocol"; import { createPatch } from "diff"; +import { canonicalPath, toPosixPath } from "./paths"; + +const CRLF = /\r\n/g; +const BOM = /^/; function readSafe(abs: string): string | null { try { @@ -26,28 +30,51 @@ function readSafe(abs: string): string | null { } } +/** + * Line endings, flattened for comparison and diffing only. + * + * On Windows `core.autocrlf=true` is the Git-for-Windows default, so the + * working tree is CRLF while every agent's edit tool writes LF — and the HEAD + * blob `fileAtHead` returns for the Codex path is LF too. Comparing those + * directly makes every line of every file differ by its terminator, so a + * one-line edit renders as a whole-file rewrite with garbage `+N −M` counts and + * review comments that anchor to the wrong lines. + * + * Only the comparison and the patch are normalized. `FileDiff.before`/`after` + * keep the bytes actually on disk, because `restoreFiles` writes `before` back + * verbatim and undo has to round-trip exactly. + * + * The leading BOM goes the same way and for the same reason. Visual Studio and + * several Windows editors write one; agent edit tools generally do not preserve + * it, and `readFileSync(…, "utf8")` hands it back as a real `` character + * rather than stripping it — so dropping it would otherwise show up as the + * first line having changed when its text is identical. + * + * One consequence worth knowing: an edit that changes *nothing but* line + * endings or the BOM now reads as no change at all, so it is neither shown nor + * committed. That is the intended trade — it is noise, not an edit. + */ +function forDiff(text: string | null): string { + return text === null ? "" : text.replace(BOM, "").replace(CRLF, "\n"); +} + /** * Canonicalize a path so the same file always produces the same map key. * * Without this the two `before` sources silently fail to meet: git reports * paths through `rev-parse --show-toplevel`, which resolves symlinks, while * `cwd` arrives however the user typed it. On macOS that is the common case, - * not an edge one — `/tmp` and `/var` are both symlinks. A mismatch here does - * not throw; it just means `prime` records a baseline nobody ever reads, so the - * user's own uncommitted edits get attributed to the agent and undone with it. + * not an edge one — `/tmp` and `/var` are both symlinks; on Windows it is any + * two spellings that differ in case, including the drive letter. A mismatch + * here does not throw; it just means `prime` records a baseline nobody ever + * reads, so the user's own uncommitted edits get attributed to the agent and + * undone with it. * - * Resolves the directory rather than the file, because the file frequently does - * not exist yet — that is precisely the create case. + * `canonicalPath` falls back to canonicalizing the containing directory when + * the file itself does not exist, which is precisely the create case. */ function canonical(cwd: string, path: string): string { - const abs = resolve(cwd, path); - try { - return resolve(realpathSync(dirname(abs)), basename(abs)); - } catch { - // A path whose parent does not exist yet cannot be canonicalized; the raw - // resolution is still a consistent key for it. - return abs; - } + return canonicalPath(resolve(cwd, path)); } /** Reads a path's content as of git HEAD. Injected so core need not know how. */ @@ -127,7 +154,7 @@ export class DiffCapture { } const before = this.before.get(abs) ?? null; const after = readSafe(abs); - return (before ?? "") === (after ?? "") ? null : { after, before }; + return forDiff(before) === forDiff(after) ? null : { after, before }; } /** Net before→after diff for every file the agent touched. */ @@ -136,11 +163,15 @@ export class DiffCapture { for (const abs of this.touched) { const before = this.before.get(abs) ?? null; const after = readSafe(abs); - if ((before ?? "") === (after ?? "")) { + if (forDiff(before) === forDiff(after)) { continue; } - const rel = relative(this.root, abs); - const patch = createPatch(rel, before ?? "", after ?? ""); + // Forward slashes from here on. This value is a git pathspec in + // `commitEdit` (where backslash is wildmatch's escape character), a key + // the browser splits on "/", and a string embedded in JSON headed for the + // model, where every backslash arrives doubled. + const rel = toPosixPath(relative(this.root, abs)); + const patch = createPatch(rel, forDiff(before), forDiff(after)); const { additions, deletions } = countChanges(patch); diffs.push({ additions, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 57f24ff..1e3e62e 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,6 +6,13 @@ export type { } from "./agent"; export { getAdapter } from "./agent"; export { DiffCapture } from "./diff-capture"; +/** Path identity, shared so containment, diff keys and history keys agree. */ +export { + canonicalPath, + isPathInside, + pathKey, + toPosixPath, +} from "./paths"; export type { EditPromptInput } from "./prompt"; export { buildEditPrompt, PIKA_SYSTEM_PROMPT, systemPrompt } from "./prompt"; export type { CodexConfigValue, CodexSettings } from "./providers/codex"; diff --git a/packages/core/src/paths.test.ts b/packages/core/src/paths.test.ts new file mode 100644 index 0000000..f9c2568 --- /dev/null +++ b/packages/core/src/paths.test.ts @@ -0,0 +1,144 @@ +/** + * Path identity, which four separate features key on — `--safe` containment, + * `DiffCapture`'s before/after map, the history store's per-repo directory, and + * the editor handler's containment check. + * + * The containment cases moved here wholesale when `isPathInside` moved out of + * `sandbox.ts`; the guard's job has not changed. It must deny what is genuinely + * outside the project — and, just as importantly, allow everything inside it. A + * false deny is not a safe failure: the model burns turns probing why it was + * refused and then routes around the guard, which costs money and produces a + * worse edit. + * + * Some of what these helpers exist for is Windows-only and cannot be exercised + * from a POSIX runner: `realpathSync.native` case-folding and `toPosixPath` + * rewriting separators both no-op here. The `windows-latest` leg in checks.yml + * is what covers those. + */ +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join, sep } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { canonicalPath, isPathInside, pathKey, toPosixPath } from "./paths"; + +let root: string; + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), "airship-paths-test-")); + mkdirSync(join(root, "src")); + writeFileSync(join(root, "src", "app.ts"), "x"); +}); + +afterEach(() => { + rmSync(root, { force: true, recursive: true }); +}); + +describe("isPathInside", () => { + it("allows a relative path in the project", () => { + expect(isPathInside(root, "src/app.ts")).toBe(true); + }); + + it("allows an absolute path in the project", () => { + expect(isPathInside(root, join(root, "src", "app.ts"))).toBe(true); + }); + + it("allows the root itself", () => { + expect(isPathInside(root, root)).toBe(true); + }); + + it("allows a file that does not exist yet", () => { + expect(isPathInside(root, join(root, "src", "new.ts"))).toBe(true); + }); + + it("allows the symlink-resolved spelling of an in-project path", () => { + // On macOS `mkdtemp` hands back `/var/...` while everything that resolves + // the path reports `/private/var/...`. Both name the same file, and a guard + // that denies one of them fires on an ordinary project. + const viaRealpath = join(realpathSync(root), "src", "app.ts"); + expect(isPathInside(root, viaRealpath)).toBe(true); + }); + + it("allows an in-project path reached through a symlinked root", () => { + const link = join(tmpdir(), `airship-paths-link-${process.pid}`); + rmSync(link, { force: true }); + symlinkSync(root, link); + try { + expect(isPathInside(link, join(root, "src", "app.ts"))).toBe(true); + expect(isPathInside(root, join(link, "src", "app.ts"))).toBe(true); + } finally { + rmSync(link, { force: true }); + } + }); + + it("denies a sibling directory that shares the project's name prefix", () => { + expect(isPathInside(root, `${root}-evil/secret.txt`)).toBe(false); + }); + + it("denies a traversal out of the project", () => { + expect(isPathInside(root, "../../etc/hosts")).toBe(false); + }); + + it("denies an unrelated absolute path", () => { + expect(isPathInside(root, "/etc/hosts")).toBe(false); + }); + + it("handles a root that already ends in a separator", () => { + // Documentation rather than regression: `resolve` strips a trailing + // separator, so this case was never the broken one. + expect(isPathInside(`${root}${sep}`, join(root, "src", "app.ts"))).toBe( + true + ); + }); + + it("handles the filesystem root, which is nothing but a separator", () => { + // This is the regression. `resolve` cannot strip the separator here — it is + // the whole path — so the old `absRoot + sep` built the prefix `//`, which + // no real path starts with, and every file read as outside the root. The + // same shape is far more reachable on Windows, where a project checked out + // at `C:\` is ordinary. + expect(isPathInside(sep, join(root, "src", "app.ts"))).toBe(true); + }); +}); + +describe("pathKey", () => { + it("agrees with itself across spellings of one path", () => { + const viaJoin = pathKey(join(root, "src", "app.ts")); + const viaTraversal = pathKey(join(root, "src", "..", "src", "app.ts")); + expect(viaJoin).toBe(viaTraversal); + }); + + it("resolves a symlinked root to the same key as the real one", () => { + // This is what stops the history store splitting in two for one project. + expect(pathKey(root)).toBe(pathKey(realpathSync(root))); + }); +}); + +describe("canonicalPath", () => { + it("falls back to the parent directory for a file that does not exist", () => { + // The create case: the file is absent but its directory is real, so the + // result still has to be the canonical location it will occupy. + expect(canonicalPath(join(root, "src", "absent.ts"))).toBe( + join(canonicalPath(join(root, "src")), "absent.ts") + ); + }); + + it("returns an absolute path even when nothing on the way exists", () => { + const result = canonicalPath(join(root, "no", "such", "dir", "x.ts")); + expect(result.endsWith(join("no", "such", "dir", "x.ts"))).toBe(true); + }); +}); + +describe("toPosixPath", () => { + it("leaves an already-POSIX path alone", () => { + expect(toPosixPath("src/components/Button.tsx")).toBe( + "src/components/Button.tsx" + ); + }); +}); diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts new file mode 100644 index 0000000..4c4d594 --- /dev/null +++ b/packages/core/src/paths.ts @@ -0,0 +1,100 @@ +/** + * Path canonicalization, shared by everything that uses a path as an identity. + * + * Four places key on paths — the `--safe` containment check, `DiffCapture`'s + * before/after map, the history store's per-repo directory, and the editor + * handler's containment check — and all four were comparing paths that Windows + * considers equal but JavaScript does not. + * + * Two distinct hazards, and they need different tools: + * + * 1. Symlinks. `/tmp` and `/var` are symlinks on macOS, so `cwd` arrives as + * `/var/folders/…` while the agent reports `/private/var/folders/…`. + * `realpathSync` handles this and always did. + * + * 2. Case. Windows filesystems are case-insensitive, so `C:\Proj` and + * `c:\proj` are the same directory — but `===` and `startsWith` say + * otherwise, and `realpathSync` does NOT fix it: the JS implementation + * resolves links while preserving whatever spelling the caller passed. + * `realpathSync.native` does canonicalize case on Windows, because it goes + * through `GetFinalPathNameByHandle`. Drive-letter case alone is enough to + * trigger this: `process.cwd()` upper-cases it, but `--cwd c:/proj` and an + * `airship.config.json` value both pass through untouched. + * + * The failures are quiet rather than loud. `--safe` refuses edits inside the + * project, `DiffCapture` attributes the user's own uncommitted work to the agent + * and undoes it with the turn, and the history store splits in two so undo and + * PR-from-session silently find nothing. + */ +import { realpathSync } from "node:fs"; +import { basename, dirname, resolve, sep } from "node:path"; + +const WIN32 = process.platform === "win32"; + +/** + * `.native` on Windows for the case-folding above. Elsewhere the JS version is + * the better choice — it is faster and does not go through the OS handle API. + */ +const realpath = WIN32 ? realpathSync.native : realpathSync; + +/** + * Resolve symlinks (and, on Windows, case) so two spellings of one path compare + * equal. + * + * Falls back to the containing directory for a file that does not exist yet — + * that is the create case, and it is the common one — and to the raw resolved + * path when even the parent is missing. + */ +export function canonicalPath(path: string): string { + try { + return realpath(path); + } catch { + try { + return resolve(realpath(dirname(path)), basename(path)); + } catch { + return resolve(path); + } + } +} + +/** + * A stable comparison key for a path. + * + * Lower-cased on Windows, where the filesystem is case-insensitive. Use this to + * compare or to key a Map; keep the original string for anything that touches + * the filesystem, so error messages and editor targets stay in the user's own + * spelling. + */ +export function pathKey(path: string): string { + const canon = canonicalPath(path); + return WIN32 ? canon.toLowerCase() : canon; +} + +/** + * True if `target` resolves to a path at or under `root`. + * + * The `+ sep` is what stops `/proj2` matching root `/proj`, so it cannot be + * dropped — but it has to be applied to a root that does not already end in a + * separator, or a drive root (`C:\`, `/`) becomes `C:\\` and matches nothing. + */ +export function isPathInside(root: string, target: string): boolean { + const absRoot = pathKey(resolve(root)); + const abs = pathKey(resolve(resolve(root), target)); + if (abs === absRoot) { + return true; + } + const prefix = absRoot.endsWith(sep) ? absRoot : absRoot + sep; + return abs.startsWith(prefix); +} + +/** + * Rewrite a native path to forward slashes. + * + * For values that leave the filesystem: git pathspecs (backslash is git's + * wildmatch escape character), anything crossing the wire to the browser, and + * anything embedded in JSON headed for the model, where every `\` is doubled. + * A no-op off Windows. + */ +export function toPosixPath(path: string): string { + return WIN32 ? path.split(sep).join("/") : path; +} diff --git a/packages/core/src/providers/claude.ts b/packages/core/src/providers/claude.ts index 9edfe61..4b37f53 100644 --- a/packages/core/src/providers/claude.ts +++ b/packages/core/src/providers/claude.ts @@ -30,8 +30,9 @@ import { type AgentRunOutcome, failureText, } from "../agent"; +import { isPathInside } from "../paths"; import { systemPrompt } from "../prompt"; -import { EDIT_TOOLS, isPathInside, makeSandboxHook } from "../sandbox"; +import { EDIT_TOOLS, makeSandboxHook } from "../sandbox"; import type { TimelineRecorder } from "../timeline"; import { describeTool } from "../tool-summary"; import { buildAirshipMcpServer } from "../tools"; diff --git a/packages/core/src/providers/opencode.ts b/packages/core/src/providers/opencode.ts index 91ce30f..3802a69 100644 --- a/packages/core/src/providers/opencode.ts +++ b/packages/core/src/providers/opencode.ts @@ -50,8 +50,9 @@ import { type AgentRunOutcome, failureText, } from "../agent"; +import { isPathInside } from "../paths"; import { systemPrompt } from "../prompt"; -import { isPathInside, screenBash, screenEdit } from "../sandbox"; +import { screenBash, screenEdit } from "../sandbox"; import { modelRefFor, sessionIdOf } from "./opencode-events"; import { finishBlocks, @@ -579,11 +580,27 @@ const PROVIDER_ENV = [ "AWS_ACCESS_KEY_ID", ]; +/** + * Where opencode keeps its state, per platform. + * + * Windows gets `%LOCALAPPDATA%` / `%APPDATA%` ahead of the XDG fallbacks: the + * XDG variables are almost never set there, so the fallback resolved to a + * `~/.local/share` that opencode has no reason to use. Both callers are the + * credential heuristic below, so getting this wrong only costs a spurious "no + * provider credentials" warning at launch — the run still proceeds — but that + * warning is indistinguishable from the real thing. + */ function dataHome(): string { + if (process.platform === "win32" && process.env.LOCALAPPDATA) { + return process.env.LOCALAPPDATA; + } return process.env.XDG_DATA_HOME || join(homedir(), ".local", "share"); } function configHome(): string { + if (process.platform === "win32" && process.env.APPDATA) { + return process.env.APPDATA; + } return process.env.XDG_CONFIG_HOME || join(homedir(), ".config"); } diff --git a/packages/core/src/sandbox.test.ts b/packages/core/src/sandbox.test.ts index f96f2bc..043ca10 100644 --- a/packages/core/src/sandbox.test.ts +++ b/packages/core/src/sandbox.test.ts @@ -1,76 +1,69 @@ /** - * The path guard's job is to deny what is genuinely outside the project — and, - * just as importantly, to allow everything inside it. A false deny is not a - * safe failure: the model burns turns probing why it was refused and then - * routes around the guard, which costs money and produces a worse edit. + * The command screen, which `--safe` promises in the launch banner. + * + * Two directions matter equally. It has to block what would actually destroy + * the user's machine — and it has to leave ordinary commands alone, because a + * false deny is not a safe failure: the model cannot see the reason, so it + * burns turns working around a guard that should never have fired. + * + * The Windows half exists because the original list was entirely POSIX-shell + * shaped, so on Windows — where Codex and OpenCode run commands through cmd.exe + * or PowerShell — the screen was a no-op while the banner still said it was on. + * (Path containment lives in ./paths.test.ts.) */ -import { - mkdirSync, - mkdtempSync, - realpathSync, - rmSync, - symlinkSync, - writeFileSync, -} from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { isPathInside } from "./sandbox"; +import { describe, expect, it } from "vitest"; +import { screenBash } from "./sandbox"; -let root: string; +const blocked = (command: string) => screenBash(command).allowed === false; -beforeEach(() => { - root = mkdtempSync(join(tmpdir(), "airship-sandbox-test-")); - mkdirSync(join(root, "src")); - writeFileSync(join(root, "src", "app.ts"), "x"); +describe("screenBash blocks destructive POSIX commands", () => { + it.each([ + "rm -rf /", + "rm -f important.txt", + "git push origin main", + "git reset --hard HEAD~5", + "sudo rm something", + "dd if=/dev/zero of=/dev/sda", + "mkfs.ext4 /dev/sda1", + ])("blocks %j", (command) => { + expect(blocked(command)).toBe(true); + }); }); -afterEach(() => { - rmSync(root, { force: true, recursive: true }); +describe("screenBash blocks destructive Windows commands", () => { + it.each([ + "del /f /s /q C:\\Users\\me\\project", + "del /q important.txt", + "rd /s /q build", + "rmdir /s /q node_modules", + "Remove-Item -Recurse -Force .\\dist", + "Remove-Item .\\dist -Recurse", + "format c:", + "diskpart", + "Clear-Disk -Number 0", + "runas /user:Administrator cmd.exe", + "Start-Process powershell -Verb RunAs", + ])("blocks %j", (command) => { + expect(blocked(command)).toBe(true); + }); }); - -describe("isPathInside", () => { - it("allows a relative path in the project", () => { - expect(isPathInside(root, "src/app.ts")).toBe(true); - }); - - it("allows an absolute path in the project", () => { - expect(isPathInside(root, join(root, "src", "app.ts"))).toBe(true); - }); - - it("allows a file that does not exist yet", () => { - expect(isPathInside(root, join(root, "src", "new.ts"))).toBe(true); - }); - - it("allows the symlink-resolved spelling of an in-project path", () => { - // On macOS `mkdtemp` hands back `/var/...` while everything that resolves - // the path reports `/private/var/...`. Both name the same file, and a guard - // that denies one of them fires on an ordinary project. - const viaRealpath = join(realpathSync(root), "src", "app.ts"); - expect(isPathInside(root, viaRealpath)).toBe(true); - }); - - it("allows an in-project path reached through a symlinked root", () => { - const link = join(tmpdir(), `airship-sandbox-link-${process.pid}`); - rmSync(link, { force: true }); - symlinkSync(root, link); - try { - expect(isPathInside(link, join(root, "src", "app.ts"))).toBe(true); - expect(isPathInside(root, join(link, "src", "app.ts"))).toBe(true); - } finally { - rmSync(link, { force: true }); - } - }); - - it("denies a sibling directory that shares the project's name prefix", () => { - expect(isPathInside(root, `${root}-evil/secret.txt`)).toBe(false); - }); - - it("denies a traversal out of the project", () => { - expect(isPathInside(root, "../../etc/hosts")).toBe(false); - }); - it("denies an unrelated absolute path", () => { - expect(isPathInside(root, "/etc/hosts")).toBe(false); +describe("screenBash allows ordinary commands", () => { + it.each([ + // The reason the Windows patterns name their flags rather than the verb: + // all of these contain `del`, `rd` or `format` as a substring or a word. + "sed -i 's/del /x/g' notes.txt", + "grep -rn 'delete' src/", + "npm run build && npm test", + "git status", + "git log --format=oneline", + "prettier --write .", + "node scripts/format.mjs", + "cargo build --release", + "echo 'runas is a windows command'", + "ls -la", + "pnpm dlx ultracite fix", + ])("allows %j", (command) => { + expect(blocked(command)).toBe(false); }); }); diff --git a/packages/core/src/sandbox.ts b/packages/core/src/sandbox.ts index c0dedb5..db99cb8 100644 --- a/packages/core/src/sandbox.ts +++ b/packages/core/src/sandbox.ts @@ -12,9 +12,9 @@ * functions. Keeping the policy separate from the hook is what lets one set of * rules — and one set of tests — cover both. */ -import { realpathSync } from "node:fs"; -import { basename, dirname, resolve, sep } from "node:path"; +import { resolve } from "node:path"; import type { HookCallback } from "@anthropic-ai/claude-agent-sdk"; +import { isPathInside } from "./paths"; export const EDIT_TOOLS = new Set([ "Write", @@ -32,6 +32,26 @@ const DESTRUCTIVE = [ />\s*\/dev\/(sd|disk)/, /\bsudo\b/, /:\(\)\s*\{/, + // The Windows half. Every pattern above except the two `git` ones is + // POSIX-shell shaped, and on Windows the backends run commands through + // cmd.exe or PowerShell — so without these the command screen was a no-op + // there while the launch banner still told the user it was on. + // + // Each names the flags the real command takes rather than matching the verb + // loosely. These run against every command on every platform, and `del` and + // `runas` are short enough to appear inside ordinary POSIX ones — a bare + // /\bdel\s+\// fires on `sed -i 's/del /x/'`. A false deny is not free: the + // model cannot see why it was refused, so it burns turns working around a + // guard that should not have fired. + /\bdel\s+\/[fsqap]\b/i, + /\brd\s+\/s\b/i, + /\brmdir\s+\/s\b/i, + /\bRemove-Item\b[^\n]*\s-(?:Recurse|Force)\b/i, + /\bformat\s+[a-z]:/i, + /\bdiskpart\b/i, + /\bClear-Disk\b/i, + /\brunas\s+\/user:/i, + /\bStart-Process\b[^\n]*\s-Verb\s+RunAs\b/i, ]; function deny(reason: string) { @@ -44,41 +64,6 @@ function deny(reason: string) { }; } -/** - * Resolve symlinks so two spellings of the same path compare equal. - * - * Falls back to the containing directory for a file that does not exist yet, - * which is the create case, and to the raw path when even that is missing. - */ -function canonical(path: string): string { - try { - return realpathSync(path); - } catch { - try { - return resolve(realpathSync(dirname(path)), basename(path)); - } catch { - return path; - } - } -} - -/** - * True if `target` resolves to a path at or under `root`. - * - * Both sides are canonicalized first. Comparing raw strings denies perfectly - * legitimate in-project edits whenever the project sits under a symlink — on - * macOS `/tmp` and `/var` both are, so `cwd` arrives as `/var/folders/…` while - * the agent reports the file as `/private/var/folders/…`. The failure is - * expensive rather than loud: the edit is refused, the model burns turns - * probing with `pwd -P` and `realpath` to work out why, and eventually routes - * around a guard that should never have fired. - */ -export function isPathInside(root: string, target: string): boolean { - const absRoot = canonical(resolve(root)); - const abs = canonical(resolve(resolve(root), target)); - return abs === absRoot || abs.startsWith(absRoot + sep); -} - /** The verdict shape both screens return. `reason` is user-facing. */ export interface ScreenResult { allowed: boolean; diff --git a/packages/overlay/src/app.ts b/packages/overlay/src/app.ts index ccf8fd7..e52a6d9 100644 --- a/packages/overlay/src/app.ts +++ b/packages/overlay/src/app.ts @@ -44,7 +44,7 @@ import { FEEDBACK, manager, } from "./dnd/manager"; -import { clear, cls, el, PREFIX } from "./dom"; +import { basename, clear, cls, el, PREFIX } from "./dom"; import { emptyState } from "./empty"; import { History } from "./history"; import { createOpApplier } from "./history-ops"; @@ -3242,11 +3242,6 @@ function chipLabel(e: ElementContext): string { return e.displayName || `<${e.tagName}>`; } -/** Last path segment — a chip has no room for `src/components/ui/Button.tsx`. */ -function basename(path: string): string { - return path.split("/").pop() || path; -} - function changeSummary( styleCount: number, moveCount: number, diff --git a/packages/overlay/src/dom.ts b/packages/overlay/src/dom.ts index e40f9f3..17726e7 100644 --- a/packages/overlay/src/dom.ts +++ b/packages/overlay/src/dom.ts @@ -50,6 +50,22 @@ export function clear(node: HTMLElement): void { * (first two classes). Shared by the selection/hover badges and the tree/DOM * views so they read identically. */ +/** + * Last segment of a source path — a chip has no room for + * `src/components/ui/Button.tsx`. + * + * Handles both separators. Diff paths arrive forward-slashed, but source + * locations come from the framework's own metadata, which on Windows is + * backslashed — and a `split("/")` on one of those returns the whole path, so + * the chip renders the full `src\components\ui\Button.tsx` it was meant to + * shorten. + */ +export function basename(path: string): string { + return path.slice( + Math.max(path.lastIndexOf("/"), path.lastIndexOf("\\")) + 1 + ); +} + export function elementLabel(node: Element): string { const tag = node.tagName.toLowerCase(); const classes = Array.from(node.classList) diff --git a/packages/overlay/src/inspector/panel.ts b/packages/overlay/src/inspector/panel.ts index 39a9f44..0907a18 100644 --- a/packages/overlay/src/inspector/panel.ts +++ b/packages/overlay/src/inspector/panel.ts @@ -25,7 +25,7 @@ import { hit, manager, } from "../dnd/manager"; -import { clear, cls, el, elementLabel } from "../dom"; +import { basename, clear, cls, el, elementLabel } from "../dom"; import { isEditorNode } from "../edit-guard"; import { emptyState } from "../empty"; import type { History } from "../history"; @@ -166,14 +166,6 @@ function isNonVisual(node: Element): boolean { return NON_VISUAL.has(node.tagName.toLowerCase()); } -/** Last segment of a source path. Handles both separators — the path comes from - * the framework's own metadata, which on Windows is backslashed. */ -function basename(path: string): string { - return path.slice( - Math.max(path.lastIndexOf("/"), path.lastIndexOf("\\")) + 1 - ); -} - /** * The Source heading's summary — `App.tsx:31`, full path on hover. * diff --git a/packages/server/src/history.ts b/packages/server/src/history.ts index e40705c..c2c3411 100644 --- a/packages/server/src/history.ts +++ b/packages/server/src/history.ts @@ -13,13 +13,38 @@ import { } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; +import { pathKey } from "@airship/core"; import type { JobDiffBundle, JobHistorySummary } from "@airship/protocol"; -function repoDir(cwd: string): string { - const hash = createHash("sha1").update(cwd).digest("hex").slice(0, 12); +function hashDir(key: string): string { + const hash = createHash("sha1").update(key).digest("hex").slice(0, 12); return join(homedir(), ".airship", "history", hash); } +/** + * One directory per project, keyed by a hash of its canonical path. + * + * `pathKey` rather than the raw `cwd`: the string the user typed is not a + * stable identity for a directory. A symlinked project (macOS `/tmp`, `/var`) + * or two spellings that differ only in case (Windows, drive letter included) + * hash to different directories, and the whole store — history, undo, + * PR-from-session — silently comes back empty for the same project. + * + * The legacy fallback exists because that hash used to be taken over the raw + * `cwd`, so canonicalizing it moves the directory for any project reached + * through a symlink. Without this their existing history would simply vanish on + * upgrade. Only read from: once anything is written under the canonical key, + * that is the one directory in play. + */ +function repoDir(cwd: string): string { + const canonical = hashDir(pathKey(cwd)); + if (existsSync(canonical)) { + return canonical; + } + const legacy = hashDir(cwd); + return existsSync(legacy) ? legacy : canonical; +} + export function writeBundle(cwd: string, bundle: JobDiffBundle): void { const dir = repoDir(cwd); mkdirSync(dir, { recursive: true }); diff --git a/packages/source/src/server.test.ts b/packages/source/src/server.test.ts new file mode 100644 index 0000000..c83be37 --- /dev/null +++ b/packages/source/src/server.test.ts @@ -0,0 +1,70 @@ +/** + * Turning what the browser reports into a path on disk. + * + * The input is a dev-server URL path, not a filesystem path, and the two look + * alike enough to be confused: `/src/App.tsx` is rooted as far as `resolve` is + * concerned. Getting it wrong is quiet — the agent is handed context from the + * wrong file, or none at all — so the cases are pinned here. + */ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { resolveServerSource } from "./server"; + +let cwd: string; + +beforeEach(() => { + cwd = mkdtempSync(join(tmpdir(), "airship-source-test-")); + mkdirSync(join(cwd, "src"), { recursive: true }); + writeFileSync(join(cwd, "src", "App.tsx"), "const App = () => null;\n"); +}); + +afterEach(() => { + rmSync(cwd, { force: true, recursive: true }); +}); + +const resolveFile = (file: string, line = 1) => + resolveServerSource(cwd, { source: { file, line } })?.file; + +describe("resolveServerSource", () => { + it("resolves a dev-server URL path against the project root", () => { + expect(resolveFile("/src/App.tsx")).toBe("src/App.tsx"); + }); + + it("resolves a project-relative path", () => { + expect(resolveFile("src/App.tsx")).toBe("src/App.tsx"); + }); + + it("resolves a genuinely absolute path inside the project", () => { + expect(resolveFile(join(cwd, "src", "App.tsx"))).toBe("src/App.tsx"); + }); + + it("resolves a /@fs/ path, which Vite uses for files outside its root", () => { + // Vite collapses `/@fs/` + `/abs/path` into `/@fs/abs/path`, so the + // remainder has lost its leading slash and has to get it back. + const abs = join(cwd, "src", "App.tsx"); + expect(resolveFile(`/@fs${abs}`)).toBe("src/App.tsx"); + }); + + it("reports the path unchanged when the file cannot be located", () => { + expect(resolveFile("/src/Missing.tsx")).toBe("/src/Missing.tsx"); + }); + + it("attaches surrounding source as context once it locates the file", () => { + const resolved = resolveServerSource(cwd, { + source: { file: "/src/App.tsx", line: 1 }, + }); + expect(resolved?.context).toContain("const App"); + }); + + it("emits forward slashes for a nested path", () => { + // This value goes into the edit prompt, into the JSON an MCP tool returns + // (where a backslash arrives doubled) and into the overlay as a label. + mkdirSync(join(cwd, "src", "components"), { recursive: true }); + writeFileSync(join(cwd, "src", "components", "Button.tsx"), "x\n"); + const file = resolveFile("/src/components/Button.tsx"); + expect(file).toBe("src/components/Button.tsx"); + expect(file).not.toContain("\\"); + }); +}); diff --git a/packages/source/src/server.ts b/packages/source/src/server.ts index e766327..9aef6b0 100644 --- a/packages/source/src/server.ts +++ b/packages/source/src/server.ts @@ -7,7 +7,7 @@ import { existsSync, readFileSync } from "node:fs"; import { relative, resolve } from "node:path"; import type { ElementContext, SourceLocation } from "@airship/protocol"; -import { readCapped, walkFiles } from "./walk"; +import { readCapped, toPosix, walkFiles } from "./walk"; export interface ResolveInput { element?: ElementContext; @@ -33,6 +33,11 @@ const LEADING_SLASHES = /^\/+/; /** Route-ish and component-ish directories, on either path separator. */ const ROUTE_DIR = /[\\/](pages|app|routes)[\\/]/; const COMPONENT_DIR = /[\\/]components?[\\/]/; +/** Vite serves files outside its root under `/@fs/`. */ +const VITE_FS_PREFIX = /^\/@fs\/(.*)$/; +/** `C:` — a path already rooted at a Windows drive. */ +const WIN32_DRIVE = /^[a-zA-Z]:/; +const WIN32 = process.platform === "win32"; export function resolveServerSource( cwd: string, @@ -42,7 +47,10 @@ export function resolveServerSource( const abs = resolveExistingSource(cwd, input.source.file); // Normalize to a project-relative path so the agent gets a path it can // open; fall back to the reported path if we can't locate the file. - const file = abs ? relative(cwd, abs) : input.source.file; + // Forward slashes on the way out — this string is rendered into the prompt + // and into the JSON the MCP tool returns, where a Windows separator arrives + // doubled, and the overlay uses it as a display label. + const file = abs ? toPosix(relative(cwd, abs)) : input.source.file; const context = abs ? readContext(abs, input.source.line) : undefined; return { ...input.source, context: context ?? input.source.context, file }; } @@ -57,19 +65,49 @@ export function resolveServerSource( * would treat the leading slash as absolute and miss the file. Try the reported * path first, then a cwd-relative form. Returns the absolute path that exists, * or null. + * + * The ordering flips on Windows, and it matters. There `resolve(cwd, "/src/ + * App.tsx")` does not fail — it resolves against cwd's *drive*, yielding + * `C:\src\App.tsx`. `C:\src` is an ordinary directory that may well exist, and + * if it does the wrong file is read, handed to the agent as context, and opened + * in the editor. On Windows a leading slash with no drive letter is never an + * absolute path, so the project-relative reading is the only correct one. */ function resolveExistingSource(cwd: string, file: string): string | null { - const direct = resolve(cwd, file); - if (existsSync(direct)) { - return direct; + const rooted = viteFsPath(file); + if (rooted) { + return existsSync(rooted) ? rooted : null; } - if (file.startsWith("/")) { + const urlPath = file.startsWith("/"); + if (urlPath) { const stripped = resolve(cwd, file.replace(LEADING_SLASHES, "")); if (existsSync(stripped)) { return stripped; } + if (WIN32) { + return null; + } } - return null; + const direct = resolve(cwd, file); + return existsSync(direct) ? direct : null; +} + +/** + * The absolute path behind a `/@fs/` URL, or null if this is not one. + * + * Vite serves anything outside its root this way. Without handling it here the + * whole `/@fs/…` string fell through as an unresolvable path and went to the + * agent verbatim — and on Windows `/@fs/C:/Users/…` would resolve to + * `C:\C:\Users\…`, which cannot exist. + */ +function viteFsPath(file: string): string | null { + const match = file.match(VITE_FS_PREFIX); + if (!match?.[1]) { + return null; + } + // POSIX needs back the slash the capture dropped; a Windows path already + // starts with its drive and must not be given one. + return WIN32_DRIVE.test(match[1]) ? match[1] : `/${match[1]}`; } function readContext(absPath: string, line: number): string | undefined { @@ -161,7 +199,7 @@ function searchByElement( } return { context: readContext(best.file, best.line), - file: relative(cwd, best.file), + file: toPosix(relative(cwd, best.file)), line: best.line, }; } diff --git a/packages/source/src/tokens.ts b/packages/source/src/tokens.ts index ae054e7..a06ec82 100644 --- a/packages/source/src/tokens.ts +++ b/packages/source/src/tokens.ts @@ -29,7 +29,7 @@ import { isTokenizableValue, type TokenScanResult, } from "@airship/protocol/tokens"; -import { readCapped, walkFiles } from "./walk"; +import { readCapped, toPosix, walkFiles } from "./walk"; const CSS_EXT: ReadonlySet = new Set([ ".css", @@ -300,7 +300,7 @@ function scanUncached(cwd: string): TokenScanResult { const lines = lineIndex(text); // Relative to the scan root, which is what the agent's own cwd-relative // paths are resolved against. - const rel = relative(root, file) || file; + const rel = toPosix(relative(root, file)) || file; scanFile(text, lines, rel, { customProperties, usage, utilities }); } diff --git a/packages/source/src/walk.ts b/packages/source/src/walk.ts index 2990aca..5360a91 100644 --- a/packages/source/src/walk.ts +++ b/packages/source/src/walk.ts @@ -9,7 +9,7 @@ * daemon that appears to hang on startup. */ import { readdirSync, readFileSync } from "node:fs"; -import { join } from "node:path"; +import { join, sep } from "node:path"; export const IGNORE_DIRS: ReadonlySet = new Set([ "node_modules", @@ -35,6 +35,18 @@ export function safeReaddir(dir: string) { } } +/** + * Rewrite a native path to forward slashes. + * + * Every path this package hands out crosses a boundary that has no notion of a + * Windows separator: the edit prompt, the JSON an MCP tool returns (where each + * `\` arrives doubled), and the overlay, which uses these as display labels and + * map keys. A no-op off Windows. + */ +export function toPosix(path: string): string { + return sep === "/" ? path : path.split(sep).join("/"); +} + export function extOf(name: string): string { const dot = name.lastIndexOf("."); return dot === -1 ? "" : name.slice(dot); From 36facc6ba455f927f4e16b784a1d58519eab51fc Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:11:52 +0530 Subject: [PATCH 4/8] fix(cli): repair editor launch, process cleanup and port selection on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Open-in-editor was broken outright, and took the daemon with it. All four editors ship as `.cmd` shims on Windows and libuv's PATH search only tries `.com` and `.exe` — so `where code` found the shim, onPath said yes, and the spawn failed with ENOENT. spawn reports that asynchronously, so the try/catch never saw it, and an `error` event with no listener throws: one click on a file it could not open exited the server. The URL-scheme fallback was no better, since it embedded a Windows path with backslashes, which are not legal in a URI path. Both are fixed, and every detached spawn here now has an error listener. `--exec` orphaned the dev server. The child is spawned with a shell, so on Windows it is cmd.exe and the real tree is cmd.exe -> pnpm -> node -> Vite; child.kill() maps to TerminateProcess on cmd.exe alone and left that tree holding the port, outliving airship and blocking the next launch. taskkill /T takes the descendants with it. Since that is already a forced kill, the SIGTERM-then-grace-then-SIGKILL escalation is skipped there. firstFreePort connect-probed, which answers "is anything accepting" rather than "can I bind" — and the two diverge on Windows, where Hyper-V, WSL2 and Docker Desktop reserve whole port ranges that accept nothing yet refuse to be bound. It now binds and lets go. The residual case is handled properly too: listen() had no error listener, so EADDRINUSE/EACCES became an uncaughtException that escaped the caller's try/catch, leaving the dev server we started unstopped. Also: opencode resolves through PATHEXT, because `npm i -g opencode-ai` — which our own error message recommends — writes opencode.cmd, never opencode.exe; its credential probe reads %LOCALAPPDATA%/%APPDATA% rather than XDG paths that are never set on Windows; SIGBREAK is registered, since Windows never delivers SIGTERM; and openInBrowser uses `cmd /c start ""` rather than a shell, where Node does no argument escaping and a URL carrying `&` would be split in two. --- apps/cli/src/commands/serve.ts | 31 ++++- apps/cli/src/lib/detect.ts | 21 ++- apps/cli/src/lib/exec.ts | 51 ++++++-- .../core/src/providers/opencode-server.ts | 41 +++++- packages/server/src/index.ts | 13 +- packages/server/src/open-editor.ts | 122 ++++++++++++++---- 6 files changed, 235 insertions(+), 44 deletions(-) diff --git a/apps/cli/src/commands/serve.ts b/apps/cli/src/commands/serve.ts index b858474..c418742 100644 --- a/apps/cli/src/commands/serve.ts +++ b/apps/cli/src/commands/serve.ts @@ -204,6 +204,28 @@ async function assertFree(port: number): Promise { } } +/** + * Turn a failure to bind the overlay's port into something actionable. + * + * EACCES is the Windows-specific one: Hyper-V, WSL2 and Docker Desktop reserve + * whole ranges of dynamic ports, and a port inside one accepts no connection + * yet refuses to be bound — so it looks free right up until it isn't. + */ +function asBindError(err: unknown, port: number): unknown { + const code = (err as NodeJS.ErrnoException | null)?.code; + if (code === "EADDRINUSE") { + return new CliError(`Port ${port} is already in use`, { + hint: "Pass --port with a free one.", + }); + } + if (code === "EACCES") { + return new CliError(`Not allowed to bind port ${port}`, { + hint: "On Windows this port may sit in a reserved range (see `netsh interface ipv4 show excludedportrange tcp`). Pass --port with one outside it.", + }); + } + return err; +} + export const serve = defineCommand({ args: argsFor(SERVE_FLAGS), meta: { @@ -274,7 +296,7 @@ export const serve = defineCommand({ // We started the dev server; if the proxy cannot come up it is ours to // clean up, or the user is left with a stray process holding the port. await dev?.stop(); - throw err; + throw asBindError(err, port); } if (opts.json) { @@ -332,5 +354,12 @@ export const serve = defineCommand({ }; process.on("SIGINT", onSignal); process.on("SIGTERM", onSignal); + // Windows never delivers SIGTERM — registering it is legal, it just never + // fires — so without SIGBREAK the only clean exit there is Ctrl-C. Ctrl-Break + // and a console close would otherwise skip shutdown entirely and strand the + // `opencode serve` child and any dev server we started. + if (process.platform === "win32") { + process.on("SIGBREAK", onSignal); + } }, }); diff --git a/apps/cli/src/lib/detect.ts b/apps/cli/src/lib/detect.ts index 666847d..b10f59f 100644 --- a/apps/cli/src/lib/detect.ts +++ b/apps/cli/src/lib/detect.ts @@ -172,6 +172,25 @@ export async function detectTarget( return first ? { ...first, listening: false } : undefined; } +/** + * Whether we can actually take this port, by taking it and letting go. + * + * `isListening` answers a different question — "is something accepting here" — + * and the two diverge on Windows, where Hyper-V, WSL2 and Docker Desktop + * reserve whole ranges of dynamic ports (`netsh interface ipv4 show + * excludedportrange tcp`). Nothing accepts on a reserved port, so a connect + * probe calls it free, and the bind then fails with EACCES. + */ +function canBind(port: number, host: string): Promise { + return new Promise((resolvePromise) => { + const probe = net.createServer(); + probe.once("error", () => resolvePromise(false)); + probe.listen({ host, port }, () => { + probe.close(() => resolvePromise(true)); + }); + }); +} + /** A port nothing is bound to, so the proxy does not collide on startup. */ export async function firstFreePort( start: number, @@ -182,7 +201,7 @@ export async function firstFreePort( // Also ordered: "the first free port at or after `start`" is the answer, so // the probes cannot be collapsed into one parallel batch. // biome-ignore lint/performance/noAwaitInLoops: ordered first-match search - if (!(await isListening(port, host))) { + if (await canBind(port, host)) { return port; } } diff --git a/apps/cli/src/lib/exec.ts b/apps/cli/src/lib/exec.ts index b315872..7eb206f 100644 --- a/apps/cli/src/lib/exec.ts +++ b/apps/cli/src/lib/exec.ts @@ -10,11 +10,13 @@ * first, and our own shutdown would be racing a corpse. */ -import { type ChildProcess, spawn } from "node:child_process"; +import { type ChildProcess, spawn, spawnSync } from "node:child_process"; import { isListening } from "./detect"; import { CliError } from "./errors"; import { style } from "./terminal"; +const WIN32 = process.platform === "win32"; + const READY_TIMEOUT_MS = 90_000; const POLL_INTERVAL_MS = 250; const STOP_GRACE_MS = 3000; @@ -33,9 +35,16 @@ function killTree(child: ChildProcess, signal: NodeJS.Signals): void { return; } try { - if (process.platform === "win32") { - // No process groups to negate; Node maps this to TerminateProcess. - child.kill(signal); + if (WIN32) { + // `child` here is cmd.exe, not the dev server — we spawn with `shell`, + // so the real tree is cmd.exe -> pnpm -> node -> Vite. `child.kill()` + // maps to TerminateProcess on cmd.exe alone and leaves that tree running, + // still holding the port, outliving airship and blocking the next launch. + // taskkill /T is the only way to take the descendants with it. + spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { + stdio: "ignore", + windowsHide: true, + }); return; } // Negative pid = the whole group, which is what actually stops a dev server @@ -87,12 +96,25 @@ export async function startDevServer(opts: { child.once("exit", (code, signal) => { exited = { code, signal }; }); + // A shell that will not start emits `error` and never `exit`. Without this + // the readiness loop below would wait the full 90s for a process that was + // never running — and an unheard `error` event throws. + child.once("error", () => { + exited ??= { code: null, signal: null }; + }); const stop = async (): Promise => { if (exited) { return; } killTree(child, "SIGTERM"); + // Windows has no graceful signal — killTree already went straight to + // `taskkill /F`, so there is nothing to escalate to and nothing to wait + // for. Polling for three seconds before repeating the same forced kill + // would only delay shutdown. + if (WIN32) { + return; + } for (let waited = 0; waited < STOP_GRACE_MS; waited += POLL_INTERVAL_MS) { if (exited) { return; @@ -134,26 +156,29 @@ export async function startDevServer(opts: { } } -const BROWSER_OPENERS: Record = { - darwin: "open", - win32: "start", -}; - /** Open a URL in the default browser, best-effort. */ export function openInBrowser(url: string): void { - const command = BROWSER_OPENERS[process.platform] ?? "xdg-open"; + // `cmd /c start "" ` on Windows rather than `shell: true` + `start`. + // With a shell Node does no argument escaping, so a URL carrying `&` would be + // split by cmd.exe into two commands; and `start` reads its first quoted + // argument as a window title, which is what the empty string absorbs. Same + // form as openUrl in @airship/server's open-editor. + const [command, args] = + process.platform === "win32" + ? (["cmd", ["/c", "start", "", url]] as const) + : ([process.platform === "darwin" ? "open" : "xdg-open", [url]] as const); try { // Detached and fully ignored: the browser outliving airship is the point, // and its stdio would otherwise pollute the terminal. - const child = spawn(command, [url], { + const child = spawn(command, [...args], { detached: true, - shell: process.platform === "win32", stdio: "ignore", + windowsHide: true, }); - child.unref(); child.on("error", () => { // No browser, no display, no `open` — the URL is printed either way. }); + child.unref(); } catch { // Same: --open is a convenience, never a reason to fail a launch. } diff --git a/packages/core/src/providers/opencode-server.ts b/packages/core/src/providers/opencode-server.ts index 92ea9ab..7825691 100644 --- a/packages/core/src/providers/opencode-server.ts +++ b/packages/core/src/providers/opencode-server.ts @@ -96,7 +96,29 @@ export interface OpencodeHandle { url: string; } -const BINARY = process.platform === "win32" ? "opencode.exe" : "opencode"; +/** + * The names `opencode` can go by on disk, most-specific first. + * + * Hardcoding `opencode.exe` on Windows found nothing for the install we + * ourselves recommend: `npm i -g opencode-ai` writes `opencode.cmd` and + * `opencode.ps1` shims, never a `.exe`. So a perfectly good opencode reported + * "No `opencode` binary found on PATH" and every edit through that backend + * failed. PATHEXT is the OS's own answer to which extensions are executable; + * the fallback matches its default. + */ +const BINARY_NAMES: string[] = + process.platform === "win32" + ? [ + ...(process.env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD") + .split(";") + .filter(Boolean) + .map((ext) => `opencode${ext.toLowerCase()}`), + // Extension-less last, so a real launcher always wins — but still + // present, because dropping it would be its own regression for anyone + // whose opencode is a bare binary on PATH. + "opencode", + ] + : ["opencode"]; /** A cold first launch on a slow machine must not read as a broken install. */ const START_TIMEOUT_MS = 30_000; @@ -115,9 +137,11 @@ export function resolveOpencodeBinary(override?: string): string | null { if (!dir) { continue; } - const candidate = join(dir, BINARY); - if (existsSync(candidate)) { - return candidate; + for (const name of BINARY_NAMES) { + const candidate = join(dir, name); + if (existsSync(candidate)) { + return candidate; + } } } return null; @@ -255,9 +279,16 @@ function registerExitHooks(): void { return; } hooksRegistered = true; - for (const signal of ["exit", "SIGINT", "SIGTERM"] as const) { + // SIGBREAK on Windows, where SIGTERM is accepted by `process.once` but never + // delivered — Ctrl-Break would otherwise leave this child holding its port. + // "exit" remains the backstop that catches everything else. + const signals = ["exit", "SIGINT", "SIGTERM"] as const; + for (const signal of signals) { process.once(signal, () => shutdownServer()); } + if (process.platform === "win32") { + process.once("SIGBREAK", () => shutdownServer()); + } } /** diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index 072749e..aad0591 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -524,8 +524,17 @@ export async function startServer(opts: ServerOptions): Promise { socket.on("close", () => tunnelled.delete(socket as Socket)); }); - await new Promise((resolve) => { - server.listen(opts.port, () => resolve()); + // The `error` listener is what makes this rejectable. Without it an + // EADDRINUSE — or, on Windows, the EACCES you get from a port inside a + // Hyper-V/WSL2 reserved range — is emitted with nobody listening, becomes an + // uncaughtException, and escapes the caller's try/catch, so the dev server + // airship started with `--exec` is never stopped either. + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(opts.port, () => { + server.removeListener("error", reject); + resolve(); + }); }); const addr = server.address() as AddressInfo | null; diff --git a/packages/server/src/open-editor.ts b/packages/server/src/open-editor.ts index 091c9e9..ab5b022 100644 --- a/packages/server/src/open-editor.ts +++ b/packages/server/src/open-editor.ts @@ -8,12 +8,15 @@ * a column and are awkward to detect. So: try the binary, fall back to the * scheme. */ -import { spawn, spawnSync } from "node:child_process"; +import { type ChildProcess, spawn, spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { platform } from "node:os"; -import { resolve, sep } from "node:path"; +import { resolve } from "node:path"; +import { isPathInside, toPosixPath } from "@airship/core"; import type { Editor } from "@airship/protocol"; +const WIN32 = platform() === "win32"; + export interface OpenRequest { column?: number; editor?: Editor; @@ -48,8 +51,10 @@ const LAUNCHERS: Record = { const PREFERENCE: Editor[] = ["vscode", "cursor", "windsurf", "zed"]; /** Vite serves out-of-root files under `/@fs/`. */ -const VITE_FS_PREFIX = /^\/@fs(\/.*)$/; +const VITE_FS_PREFIX = /^\/@fs\/(.*)$/; const LEADING_SLASHES = /^\/+/; +/** `C:` — a Windows path that is already rooted at a drive. */ +const WIN32_DRIVE = /^[a-zA-Z]:/; export function openInEditor(cwd: string, req: OpenRequest): OpenResult { const root = resolve(cwd); @@ -57,7 +62,12 @@ export function openInEditor(cwd: string, req: OpenRequest): OpenResult { // This socket is unauthenticated and local, and this handler spawns // processes — without the containment check any page the browser loads could // ask the daemon to open arbitrary files on disk. - if (abs !== root && !abs.startsWith(root + sep)) { + // + // isPathInside rather than a bare startsWith: the two sides reach here by + // different routes and can disagree on symlinks (macOS `/tmp`, `/var`) or on + // case (Windows, including the drive letter), and a false "outside the + // project" on the user's own file is indistinguishable from a real refusal. + if (!isPathInside(root, abs)) { return { error: "path is outside the project", ok: false }; } if (!existsSync(abs)) { @@ -73,22 +83,13 @@ export function openInEditor(cwd: string, req: OpenRequest): OpenResult { if (!onPath(launcher.bin)) { continue; } - try { - // Detached and unref'd: the editor outlives the daemon, and inheriting - // stdio would wire its output into ours. Never `shell: true` — argv - // arrays mean a path with a space or a quote is just a path. - spawn(launcher.bin, launcher.args(target), { - detached: true, - stdio: "ignore", - }).unref(); + if (launch(launcher.bin, launcher.args(target))) { return { editor, ok: true }; - } catch { - // Fall through to the next candidate, then to the URL scheme. } } const wanted = req.editor ?? PREFERENCE[0]; - if (openUrl(`${LAUNCHERS[wanted].scheme}://file/${abs}:${line}:${column}`)) { + if (openUrl(editorUrl(LAUNCHERS[wanted].scheme, abs, line, column))) { return { editor: wanted, ok: true }; } return { @@ -111,16 +112,83 @@ export function openInEditor(cwd: string, req: OpenRequest): OpenResult { function projectPath(file: string): string { const fs = file.match(VITE_FS_PREFIX); if (fs?.[1]) { - return fs[1]; + // The remainder is already absolute. On POSIX it needs the slash the + // capture dropped; on Windows it opens with a drive letter and must NOT + // get one, because `resolve` reads a leading slash as rooted-but- + // deviceless and inherits the device from cwd — turning + // `/@fs/C:/Users/me/x.tsx` into `C:\C:\Users\me\x.tsx`. + return WIN32_DRIVE.test(fs[1]) ? fs[1] : `/${fs[1]}`; } // A real absolute path exists on disk; a URL path like `/src/App.tsx` does - // not, and is meant to be read relative to the project root. - if (file.startsWith("/") && !existsSync(file)) { + // not, and is meant to be read relative to the project root. On Windows the + // probe is skipped: a leading slash with no drive letter is never an absolute + // path there, so `existsSync` would only ever be answering about a different + // file — `:\src\App.tsx`, which routinely exists. + if (file.startsWith("/") && (WIN32 || !existsSync(file))) { return file.replace(LEADING_SLASHES, ""); } return file; } +/** + * Start a detached child, or report that it could not start. + * + * `shell` on Windows because every one of these editors ships as a `.cmd` shim + * (`code.cmd`, `cursor.cmd`, …) and libuv's PATH search only ever tries `.com` + * and `.exe`. `where code` finds the shim, so `onPath` says yes, and the bare + * spawn then fails with ENOENT — which is how this managed to be 100% broken on + * Windows while looking fine. Node applies cmd-specific argument escaping when + * `shell` is set, so a path with a space in it survives. + * + * The `error` listener is not optional. spawn reports a failure to start + * asynchronously, so the `try` here never sees it; an EventEmitter that emits + * `error` with nobody listening throws, and this runs in the daemon, so one + * click on a file it cannot open took the whole server down. + * + * `pid` is undefined when the spawn failed outright, which is the only + * synchronous signal available — and it is what lets the caller fall through to + * the URL scheme instead of reporting a success that never happened. + */ +function launch(bin: string, args: string[]): boolean { + try { + const child = spawn(bin, args, { + detached: true, + shell: WIN32, + stdio: "ignore", + windowsHide: true, + }); + detach(child); + return child.pid !== undefined; + } catch { + return false; + } +} + +/** Let the child outlive us, and never let its `error` reach the top level. */ +function detach(child: ChildProcess): void { + child.on("error", () => { + // No editor, no PATH entry, no display. The caller reports it; the daemon + // must not die of it. + }); + child.unref(); +} + +/** + * `vscode://file/...` for the fallback path. + * + * Backslashes are not legal in a URI path, so a Windows path has to be + * rewritten or the scheme handler simply ignores it. POSIX paths already open + * with `/`, which is why there is only one after `file` in the template. + */ +function editorUrl( + scheme: string, + abs: string, + line: number, + column: number +): string { + return `${scheme}://file/${toPosixPath(abs)}:${line}:${column}`; +} + function candidates(explicit?: Editor): Editor[] { if (explicit) { return [explicit]; @@ -140,10 +208,12 @@ function onPath(bin: string): boolean { if (hit !== undefined) { return hit; } - const probe = platform() === "win32" ? "where" : "which"; + const probe = WIN32 ? "where" : "which"; let found = false; try { - found = spawnSync(probe, [bin], { stdio: "ignore" }).status === 0; + found = + spawnSync(probe, [bin], { stdio: "ignore", windowsHide: true }).status === + 0; } catch { found = false; } @@ -158,12 +228,20 @@ function openUrl(url: string): boolean { if (os === "darwin") { bin = "open"; } else if (os === "win32") { + // `cmd /c start "" ` rather than `shell: true`: `start` takes a window + // title as its first quoted argument, so the empty string is what stops it + // swallowing the URL as one. cmd.exe is a real .exe, so no shell needed. bin = "cmd"; args = ["/c", "start", "", url]; } try { - spawn(bin, args, { detached: true, stdio: "ignore" }).unref(); - return true; + const child = spawn(bin, args, { + detached: true, + stdio: "ignore", + windowsHide: true, + }); + detach(child); + return child.pid !== undefined; } catch { return false; } From 5349607a380347fc0020216a66900e186b0d6d45 Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:12:06 +0530 Subject: [PATCH 5/8] ci: add a windows-latest leg and document Windows support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit None of this was caught because nothing ever ran on Windows: every job in every workflow was ubuntu-latest, and checks.yml did not even run on the PR that reported it — only Vercel, which failed on fork authorization. The check job now runs on both, fail-fast off so a Windows-only break still reports the Linux result. `shell: bash` on the multi-line steps, since the default shell there is pwsh, which shares none of that syntax; Git Bash ships on the runner, so nothing needs rewriting. The two drift checks stay Linux-only — they verify that a committed generated file matches its generator, which is a property of the repo, not of the platform. The new build step is unconditional and unscoped on purpose. `--affected` is exactly what let these through: they lived in build scripts, so a PR touching no affected package never ran them. A `pnpm clean` step guards the lane that was broken in all eleven packages. pnpm-workspace.yaml's release-age exclusions had drifted almost across the board — turbo pinned at 2.10.1 against 2.10.9 in the lockfile, the Claude SDK at 0.3.196 against 0.3.226, both codex packages a minor behind. A stale pin does not fail loudly; it simply stops excluding anything, and the package falls back under the gate. That is the silent optional-dep drop the file's own esbuild comment documents, and every one of these ships per-platform packages including win32. --- .github/workflows/checks.yml | 43 +++++++++++++++++++++++++++--- CONTRIBUTING.md | 37 ++++++++++++++++++++++++++ README.md | 2 ++ apps/cli/README.md | 2 ++ pnpm-workspace.yaml | 51 +++++++++++++++++++++--------------- 5 files changed, 111 insertions(+), 24 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 1bcd5f6..f75fe9f 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -21,7 +21,16 @@ concurrency: jobs: check: if: github.event_name == 'workflow_dispatch' || github.event.pull_request.draft == false - runs-on: ubuntu-latest + # Windows is a first-class target — the CLI installs from npm onto it — but + # it went untested until a contributor reported that a fresh clone could not + # build there at all. Three separate path/line-ending bugs had shipped + # invisibly because every lane ran on Linux only. fail-fast is off so a + # Windows-only break still reports the Linux result, and vice versa. + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 with: @@ -33,7 +42,12 @@ jobs: - name: Lint (Biome via Ultracite) run: pnpm lint + # `shell: bash` on every multi-line block below: the default shell on + # windows-latest is pwsh, which shares none of this syntax — no `[ -n … ]`, + # no `if ! cmd`, and `${VAR}` is not env expansion. Git Bash ships on the + # Windows runner, so pinning the shell is enough; nothing needs rewriting. - name: Typecheck + test (affected) + shell: bash env: TURBO_SCM_BASE: ${{ github.event.pull_request.base.sha }} run: | @@ -43,10 +57,25 @@ jobs: pnpm turbo run typecheck test fi + # Unconditional and unscoped, unlike the step above. `--affected` is what + # let the Windows build bugs through: they lived in build scripts + # (check-css.mjs, vendor-assets.mjs, the gen.mjs front-matter readers), and + # a PR that touched none of the affected packages never ran them. This is + # the step that actually proves a clean checkout builds on both platforms. + - name: Build (every package) + run: pnpm build + # apps/cli/README.md is generated from the root README.md, and it is what # npmjs.com renders for @airshiplabs/cli. Committed rather than built on # demand so a clean checkout can publish without running the generator. + # + # Linux only, here and below: both of these check that a COMMITTED + # generated file matches what the generator emits. That is a property of + # the repo, not of the platform, so running it twice doubles the runtime + # and the flake surface for no extra signal. - name: README is not stale + if: matrix.os == 'ubuntu-latest' + shell: bash run: | if ! node scripts/sync-readme.mjs --check; then echo "::error file=apps/cli/README.md::The CLI README is stale. Run 'make readme' and commit the result." @@ -57,16 +86,24 @@ jobs: # Regenerated by BUILDING, not by `tsr generate`: two things write this # file and they disagree — the router CLI emits the tree alone, while the # Start Vite plugin appends the `declare module` block that registers the - # router type for SSR. The build's output is the committed one. + # router type for SSR. The build's output is the committed one. The build + # step above already wrote it; this only reads the result. - name: Route tree is not stale + if: matrix.os == 'ubuntu-latest' + shell: bash run: | - pnpm turbo run build --filter=@airship/web if ! git diff --quiet -- apps/web/src/routeTree.gen.ts; then git diff --stat -- apps/web/src/routeTree.gen.ts echo "::error file=apps/web/src/routeTree.gen.ts::The route tree is stale. Run 'make web:build' and commit the result." exit 1 fi + # Last, because it deletes what everything above produced. Every workspace + # `clean` was `rm -rf` until this lane existed to catch it — a command that + # does not exist on Windows, so `pnpm clean` failed in all eleven packages. + - name: Clean (removes what the build produced) + run: pnpm clean + commitlint: if: github.event_name == 'pull_request' && github.event.pull_request.draft == false runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 49a7e16..2fe5cc5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -30,6 +30,43 @@ Open and you are looking at Airship, with Airship's own inside it. Pick the hero's button, ask for a change, and the diff lands in `apps/web/src/`. `make run:solo` does both in one terminal via `--exec`. +### On Windows + +Everything builds, tests and runs on Windows — `checks.yml` gates every PR on a +`windows-latest` leg alongside Linux, so a break there fails the PR. + +`make` is the one thing that does not carry over: the Makefile declares +`SHELL := /bin/bash` and a handful of targets genuinely need it (`help` is an `awk` +program, `preflight` a shell conditional, `release` a bash script). It is a thin +wrapper either way — every recipe is one `pnpm` or `node` call — so use those directly: + +| Instead of | Run | +| --------------- | ---------------------------------------------------------- | +| `make demo` | `pnpm install && pnpm build` | +| `make web:dev` | `pnpm dev:web` | +| `make run` | `node apps/cli/dist/index.js --target 5173 --cwd apps/web` | +| `make run:solo` | `node apps/cli/dist/index.js --cwd apps/web --exec "pnpm dev:web"` | +| `make doctor` | `node apps/cli/dist/index.js doctor --cwd apps/web` | +| `make check` | `pnpm lint && pnpm typecheck && pnpm test` | +| `make readme` | `node scripts/sync-readme.mjs` | +| `make storybook`| `pnpm turbo run storybook --filter=@airship/overlay` | + +`pnpm dev:web` rather than a bare `vite dev`: the site cannot start until +`@airship/site-tokens` has emitted `dist/tokens.css`, and only turbo knows that. + +Two things worth setting up once: + +- **Git Bash**, which ships with Git for Windows, runs the Husky hooks and the release + scripts. Without a POSIX `sh` on PATH the pre-commit formatter silently does not run. +- **Developer Mode** (Settings → System → For developers), so pnpm can create the + symlinks its `node_modules` layout depends on without elevation. + +Line endings are pinned to LF by [`.gitattributes`](.gitattributes) — do not override it +with `core.autocrlf`. Several generators parse their input with anchored regexes, and a +CRLF checkout makes them report a missing front-matter block rather than a wrong one. +If you cloned before that file existed, renormalize once with +`git rm --cached -r . && git reset --hard`. + ## Repo layout | Package | Role | diff --git a/README.md b/README.md index 5bb3ea1..1e1f50c 100644 --- a/README.md +++ b/README.md @@ -428,6 +428,8 @@ and provider it would use from your terminal. Node 22.13 or later, and one of Claude Code, OpenAI Codex or OpenCode. +macOS, Linux and Windows. Every PR is built and tested on Linux and Windows. + ## Links - [airship.design](https://airship.design) diff --git a/apps/cli/README.md b/apps/cli/README.md index 1b6de36..8aaf37e 100644 --- a/apps/cli/README.md +++ b/apps/cli/README.md @@ -430,6 +430,8 @@ and provider it would use from your terminal. Node 22.13 or later, and one of Claude Code, OpenAI Codex or OpenCode. +macOS, Linux and Windows. Every PR is built and tested on Linux and Windows. + ## Links - [airship.design](https://airship.design) diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 14f46b3..6fffefd 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -24,28 +24,37 @@ minimumReleaseAgeExclude: # a name pattern carrying a version union and enumerating all 25 for every # bump is not maintainable. They only ever publish in lockstep with the # version above, so the unpinned glob follows whatever it is pinned to. + # + # EVERY VERSION BELOW MUST MATCH THE LOCKFILE. A pin left behind at an older + # version does not fail loudly — it simply stops excluding anything, and the + # package silently falls back under the release-age gate, which is the + # esbuild failure above all over again. All of these but esbuild had drifted + # at once (turbo 2.10.1 vs 2.10.9, the Claude SDK 0.3.196 vs 0.3.226, both + # codex packages a minor behind), and every one of them ships per-platform + # optional deps including win32 — so the package most likely to go missing is + # the one for whichever platform is least represented in the team's machines. + # `pnpm why ` after a bump, and update the pin in the same commit. - 'esbuild@0.28.2' - '@esbuild/*' - - '@anthropic-ai/claude-agent-sdk-darwin-arm64@0.3.196' - - '@anthropic-ai/claude-agent-sdk-darwin-x64@0.3.196' - - '@anthropic-ai/claude-agent-sdk-linux-arm64-musl@0.3.196' - - '@anthropic-ai/claude-agent-sdk-linux-arm64@0.3.196' - - '@anthropic-ai/claude-agent-sdk-linux-x64-musl@0.3.196' - - '@anthropic-ai/claude-agent-sdk-linux-x64@0.3.196' - - '@anthropic-ai/claude-agent-sdk-win32-arm64@0.3.196' - - '@anthropic-ai/claude-agent-sdk-win32-x64@0.3.196' - - '@anthropic-ai/claude-agent-sdk@0.3.196' - - '@openai/codex-sdk@0.146.0' + - '@anthropic-ai/claude-agent-sdk-darwin-arm64@0.3.226' + - '@anthropic-ai/claude-agent-sdk-darwin-x64@0.3.226' + - '@anthropic-ai/claude-agent-sdk-linux-arm64-musl@0.3.226' + - '@anthropic-ai/claude-agent-sdk-linux-arm64@0.3.226' + - '@anthropic-ai/claude-agent-sdk-linux-x64-musl@0.3.226' + - '@anthropic-ai/claude-agent-sdk-linux-x64@0.3.226' + - '@anthropic-ai/claude-agent-sdk-win32-arm64@0.3.226' + - '@anthropic-ai/claude-agent-sdk-win32-x64@0.3.226' + - '@anthropic-ai/claude-agent-sdk@0.3.226' + - '@openai/codex-sdk@0.147.0' # Unlike the other two SDKs this bundles no binary — the `opencode` CLI is a # separate install, detected on PATH at runtime. See providers/opencode.ts. - - '@opencode-ai/sdk@1.18.13' - - '@openai/codex@0.146.0-darwin-arm64 || 0.146.0-darwin-x64 || 0.146.0-linux-arm64 || 0.146.0-linux-x64 || 0.146.0-win32-arm64 || 0.146.0-win32-x64 || 0.146.0' - - '@turbo/darwin-64@2.10.1' - - '@turbo/darwin-arm64@2.10.1' - - '@turbo/linux-64@2.10.1' - - '@turbo/linux-arm64@2.10.1' - - '@turbo/windows-64@2.10.1' - - '@turbo/windows-arm64@2.10.1' - - turbo@2.10.1 - - ultracite@7.10.0 - - '@opencode-ai/sdk@1.18.13' + - '@opencode-ai/sdk@1.18.15' + - '@openai/codex@0.147.0-darwin-arm64 || 0.147.0-darwin-x64 || 0.147.0-linux-arm64 || 0.147.0-linux-x64 || 0.147.0-win32-arm64 || 0.147.0-win32-x64 || 0.147.0' + - '@turbo/darwin-64@2.10.9' + - '@turbo/darwin-arm64@2.10.9' + - '@turbo/linux-64@2.10.9' + - '@turbo/linux-arm64@2.10.9' + - '@turbo/windows-64@2.10.9' + - '@turbo/windows-arm64@2.10.9' + - turbo@2.10.9 + - ultracite@7.10.2 From ff9472776b6ec988e63352b095cab0ba5e3c27c2 Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:19:28 +0530 Subject: [PATCH 6/8] test: make the new path tests portable to Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The windows-latest leg caught these on its first run — all three are the tests assuming POSIX, not the code under test. - The `/@fs/` case built its input as `/@fs` + an absolute path, which only produces a separator when that path opens with one. On Windows it yielded `/@fsC:\Users\…`, a URL Vite would never serve. Vite writes the path in URL form after `/@fs/`: forward slashes, no leading slash of its own. - The filesystem-root case passed a bare `sep`, which on Windows resolves against the *current* drive. CI checks out on D: while the temp directory lives on C:, so the root and the file under test were genuinely unrelated and the guard was right to deny. `parse(root).root` names the root that actually contains it. - The symlinked-root case used Node's default link type, which is "file" and does not work for a directory on Windows. A junction does, and unlike a real directory symlink it needs neither elevation nor Developer Mode. --- packages/core/src/paths.test.ts | 19 ++++++++++++++----- packages/source/src/server.test.ts | 13 ++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/packages/core/src/paths.test.ts b/packages/core/src/paths.test.ts index f9c2568..70dfcc2 100644 --- a/packages/core/src/paths.test.ts +++ b/packages/core/src/paths.test.ts @@ -24,7 +24,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { join, sep } from "node:path"; +import { join, parse, sep } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { canonicalPath, isPathInside, pathKey, toPosixPath } from "./paths"; @@ -67,13 +67,16 @@ describe("isPathInside", () => { it("allows an in-project path reached through a symlinked root", () => { const link = join(tmpdir(), `airship-paths-link-${process.pid}`); - rmSync(link, { force: true }); - symlinkSync(root, link); + rmSync(link, { force: true, recursive: true }); + // "junction" on Windows: Node's default link type is "file", which does not + // work for a directory, and unlike a real directory symlink a junction + // needs neither elevation nor Developer Mode. + symlinkSync(root, link, process.platform === "win32" ? "junction" : "dir"); try { expect(isPathInside(link, join(root, "src", "app.ts"))).toBe(true); expect(isPathInside(root, join(link, "src", "app.ts"))).toBe(true); } finally { - rmSync(link, { force: true }); + rmSync(link, { force: true, recursive: true }); } }); @@ -103,7 +106,13 @@ describe("isPathInside", () => { // no real path starts with, and every file read as outside the root. The // same shape is far more reachable on Windows, where a project checked out // at `C:\` is ordinary. - expect(isPathInside(sep, join(root, "src", "app.ts"))).toBe(true); + // + // `parse(root).root` rather than a bare `sep`: on Windows that resolves + // against the *current* drive, and CI runs the checkout on D: while the + // temp directory lives on C:, so the two would be genuinely unrelated. + expect(isPathInside(parse(root).root, join(root, "src", "app.ts"))).toBe( + true + ); }); }); diff --git a/packages/source/src/server.test.ts b/packages/source/src/server.test.ts index c83be37..edd7b32 100644 --- a/packages/source/src/server.test.ts +++ b/packages/source/src/server.test.ts @@ -12,6 +12,10 @@ import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { resolveServerSource } from "./server"; +/** Native separators to URL ones, and the leading slash a URL path drops. */ +const BACKSLASH = /\\/g; +const LEADING_SLASH = /^\//; + let cwd: string; beforeEach(() => { @@ -41,10 +45,13 @@ describe("resolveServerSource", () => { }); it("resolves a /@fs/ path, which Vite uses for files outside its root", () => { - // Vite collapses `/@fs/` + `/abs/path` into `/@fs/abs/path`, so the - // remainder has lost its leading slash and has to get it back. + // Built the way Vite builds it — `/@fs/` followed by the absolute path in + // URL form: forward slashes, and no leading slash of its own. On POSIX that + // reads `/@fs/Users/…`, on Windows `/@fs/C:/Users/…`, so the remainder has + // a leading slash to restore in one case and a drive letter in the other. const abs = join(cwd, "src", "App.tsx"); - expect(resolveFile(`/@fs${abs}`)).toBe("src/App.tsx"); + const url = `/@fs/${abs.replace(BACKSLASH, "/").replace(LEADING_SLASH, "")}`; + expect(resolveFile(url)).toBe("src/App.tsx"); }); it("reports the path unchanged when the file cannot be located", () => { From 3f193ac4f2182cd519f995d234655de67b77035b Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:27:58 +0530 Subject: [PATCH 7/8] fix(git): canonicalize paths the same way @airship/core does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The windows-latest leg failed the two Codex-path tests with a null baseline, and the cause was a second implementation of the same rule. @airship/git had its own realpathSafe built on the JS realpathSync, while core's canonicalPath used realpathSync.native for the Windows case-folding. Those disagree on more than case: the JS version leaves an 8.3 short name alone, so a repo under `C:\Users\RUNNER~1\…` stayed short here while DiffCapture's keys came back as `C:\Users\runneradmin\…`. fileAtHead computes `relative(root, path)` from the two, got a `..` path, and returned null for every file — which on the Codex path means no baseline at all, so each edit would render as a whole-file diff against nothing. This is exactly the drift the ./paths module was added to end; it unified core's four copies and left this fifth one. The rule now lives here, in the lowest package that touches the filesystem, and core imports it. Deliberately not re-exported from ./paths — one implementation, one import path, so there is nowhere for a second copy to reappear. --- packages/core/src/diff-capture.ts | 3 ++- packages/core/src/index.ts | 7 +----- packages/core/src/paths.test.ts | 3 ++- packages/core/src/paths.ts | 36 ++++++++----------------------- packages/git/src/index.ts | 34 +++++++++++++++++++++++------ 5 files changed, 41 insertions(+), 42 deletions(-) diff --git a/packages/core/src/diff-capture.ts b/packages/core/src/diff-capture.ts index c7599fa..5da5c09 100644 --- a/packages/core/src/diff-capture.ts +++ b/packages/core/src/diff-capture.ts @@ -15,9 +15,10 @@ */ import { existsSync, readFileSync } from "node:fs"; import { relative, resolve } from "node:path"; +import { canonicalPath } from "@airship/git"; import type { FileDiff } from "@airship/protocol"; import { createPatch } from "diff"; -import { canonicalPath, toPosixPath } from "./paths"; +import { toPosixPath } from "./paths"; const CRLF = /\r\n/g; const BOM = /^/; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1e3e62e..6f545a8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -7,12 +7,7 @@ export type { export { getAdapter } from "./agent"; export { DiffCapture } from "./diff-capture"; /** Path identity, shared so containment, diff keys and history keys agree. */ -export { - canonicalPath, - isPathInside, - pathKey, - toPosixPath, -} from "./paths"; +export { isPathInside, pathKey, toPosixPath } from "./paths"; export type { EditPromptInput } from "./prompt"; export { buildEditPrompt, PIKA_SYSTEM_PROMPT, systemPrompt } from "./prompt"; export type { CodexConfigValue, CodexSettings } from "./providers/codex"; diff --git a/packages/core/src/paths.test.ts b/packages/core/src/paths.test.ts index 70dfcc2..ca03fcb 100644 --- a/packages/core/src/paths.test.ts +++ b/packages/core/src/paths.test.ts @@ -25,8 +25,9 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join, parse, sep } from "node:path"; +import { canonicalPath } from "@airship/git"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { canonicalPath, isPathInside, pathKey, toPosixPath } from "./paths"; +import { isPathInside, pathKey, toPosixPath } from "./paths"; let root: string; diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts index 4c4d594..be3dc95 100644 --- a/packages/core/src/paths.ts +++ b/packages/core/src/paths.ts @@ -26,36 +26,18 @@ * and undoes it with the turn, and the history store splits in two so undo and * PR-from-session silently find nothing. */ -import { realpathSync } from "node:fs"; -import { basename, dirname, resolve, sep } from "node:path"; +import { resolve, sep } from "node:path"; +import { canonicalPath } from "@airship/git"; const WIN32 = process.platform === "win32"; -/** - * `.native` on Windows for the case-folding above. Elsewhere the JS version is - * the better choice — it is faster and does not go through the OS handle API. - */ -const realpath = WIN32 ? realpathSync.native : realpathSync; - -/** - * Resolve symlinks (and, on Windows, case) so two spellings of one path compare - * equal. - * - * Falls back to the containing directory for a file that does not exist yet — - * that is the create case, and it is the common one — and to the raw resolved - * path when even the parent is missing. - */ -export function canonicalPath(path: string): string { - try { - return realpath(path); - } catch { - try { - return resolve(realpath(dirname(path)), basename(path)); - } catch { - return resolve(path); - } - } -} +// canonicalPath comes from @airship/git and is deliberately NOT re-exported +// here: one implementation, one import path. That package needs the identical +// rule for `fileAtHead`, which derives a repo-relative path from keys this +// module produced, and when a second copy lived here the two drifted on +// Windows — the JS realpathSync leaves an 8.3 short name (`RUNNER~1`) alone +// while the native one expands it (`runneradmin`) — so `relative` returned a +// `..` path and every file read as absent from HEAD. /** * A stable comparison key for a path. diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index 3601ab6..d637532 100644 --- a/packages/git/src/index.ts +++ b/packages/git/src/index.ts @@ -128,8 +128,8 @@ export function fileAtHead(cwd: string, path: string): string | null { // from mixed sources — some resolved through git, some as the user typed // them — and on macOS `/tmp` and `/var` are symlinks, so an uncanonicalized // comparison reads a perfectly normal path as escaping the project. - const root = realpathSafe(cwd); - const rel = relative(root, realpathSafe(resolve(cwd, path))); + const root = canonicalPath(cwd); + const rel = relative(root, canonicalPath(resolve(cwd, path))); // `..` means the path really does escape the working directory; `HEAD:./..` // is not valid git syntax and there is nothing sensible to return. if (!rel || rel.startsWith("..")) { @@ -138,15 +138,35 @@ export function fileAtHead(cwd: string, path: string): string | null { return tryGitRaw(cwd, ["show", `HEAD:./${rel.split(sep).join("/")}`]); } -/** Resolve symlinks where possible, falling back for paths that don't exist. */ -function realpathSafe(path: string): string { +/** + * Resolve symlinks — and, on Windows, case and 8.3 short names — where + * possible, falling back for paths that don't exist. + * + * `.native` on Windows is not a detail. The JS implementation resolves links + * while preserving whatever spelling the caller passed, so `C:\Users\RUNNER~1` + * stays short while anything that went through `GetFinalPathNameByHandle` + * reads `C:\Users\runneradmin`. Two spellings of one directory make `relative` + * below return a `..` path, and `fileAtHead` then reports every file as absent + * from HEAD — which on the Codex path silently turns each edit into a + * whole-file diff with no baseline. + * + * Exported, and re-exported by @airship/core's ./paths rather than + * reimplemented there: core canonicalizes DiffCapture's map keys and hands the + * results straight to `fileAtHead`, so a second implementation is a second + * chance for the two to disagree. This package is the lowest one that touches + * the filesystem, which makes it the place that owns the rule. + */ +const realpath = + process.platform === "win32" ? realpathSync.native : realpathSync; + +export function canonicalPath(path: string): string { try { - return realpathSync(path); + return realpath(path); } catch { try { - return resolve(realpathSync(dirname(path)), basename(path)); + return resolve(realpath(dirname(path)), basename(path)); } catch { - return path; + return resolve(path); } } } From effe3b52b891bda4097411a876bec4bdd747fdbb Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:32:38 +0530 Subject: [PATCH 8/8] test(server): assert the separator-independent source path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SourceLocation.file` is normalized to forward slashes now — it goes into the edit prompt, into the JSON an MCP tool returns where each backslash arrives doubled, and into the overlay as a display label. The assertion still built its expectation with `join`, so it demanded a native path and failed on Windows against the value the resolver is now meant to produce. --- packages/server/src/prompt-input.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/server/src/prompt-input.test.ts b/packages/server/src/prompt-input.test.ts index 5ed90a5..0d2fa9e 100644 --- a/packages/server/src/prompt-input.test.ts +++ b/packages/server/src/prompt-input.test.ts @@ -163,7 +163,11 @@ describe("preparePromptInput — source backfill", () => { source: { file: "/src/App.tsx", line: 4 }, }) ); - expect(input.source?.file).toBe(join("src", "App.tsx")); + // Forward slashes, not `join`: this value is separator-independent by + // design. It goes into the edit prompt, into the JSON an MCP tool returns + // (where a backslash arrives doubled), and into the overlay as a label, so + // the resolver normalizes it rather than emitting a native path. + expect(input.source?.file).toBe("src/App.tsx"); expect(input.source?.context).toContain("Get Started"); }); });