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);