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/.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/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/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/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/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/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..5da5c09 100644 --- a/packages/core/src/diff-capture.ts +++ b/packages/core/src/diff-capture.ts @@ -13,10 +13,15 @@ * 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 { canonicalPath } from "@airship/git"; import type { FileDiff } from "@airship/protocol"; import { createPatch } from "diff"; +import { toPosixPath } from "./paths"; + +const CRLF = /\r\n/g; +const BOM = /^/; function readSafe(abs: string): string | null { try { @@ -26,28 +31,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 +155,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 +164,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..6f545a8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,6 +6,8 @@ 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 { 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..ca03fcb --- /dev/null +++ b/packages/core/src/paths.test.ts @@ -0,0 +1,154 @@ +/** + * 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, parse, sep } from "node:path"; +import { canonicalPath } from "@airship/git"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { 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, 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, recursive: 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. + // + // `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 + ); + }); +}); + +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..be3dc95 --- /dev/null +++ b/packages/core/src/paths.ts @@ -0,0 +1,82 @@ +/** + * 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 { resolve, sep } from "node:path"; +import { canonicalPath } from "@airship/git"; + +const WIN32 = process.platform === "win32"; + +// 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. + * + * 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-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/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/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-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/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/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/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/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); } } } 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/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/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/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/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/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; } 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"); }); }); 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/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/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/packages/source/src/server.test.ts b/packages/source/src/server.test.ts new file mode 100644 index 0000000..edd7b32 --- /dev/null +++ b/packages/source/src/server.test.ts @@ -0,0 +1,77 @@ +/** + * 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"; + +/** Native separators to URL ones, and the leading slash a URL path drops. */ +const BACKSLASH = /\\/g; +const LEADING_SLASH = /^\//; + +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", () => { + // 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"); + 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", () => { + 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); 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 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 }); +} 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" );