From 8e0c91c5dc58cccd7253ac06356fb23e1d150d7f Mon Sep 17 00:00:00 2001 From: Nayan Date: Sun, 16 Aug 2026 12:15:31 +0530 Subject: [PATCH] fix(git): carry every failure's reason, and stop undo deleting files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit packages/git funnelled every invocation through `catch { return null }`, so a missing commit identity, a stale index.lock, a pathspec that matched nothing and a git that was not installed all reached the user as "commit failed" — and a git that could not run at all was reported as "not a git repository", which is what a Windows contributor hit after installing Git for Git Bash only. That null had a second meaning. `fileAtHead` returned it both for "this file was not in HEAD" and for "we could not ask", so with git unavailable every edited file was recorded `isNew` and undo deletes what an edit created: on the codex and opencode paths, Revert deleted pre-existing tracked source files. `HeadRead` splits the two, `FileDiff.noBaseline` carries the difference, and `restoreFiles` checks it before `isNew` and refuses. One `run()` now keeps exit code, stderr and spawn errno, and `failureText` turns them into the one line a toast has room for. `stdout` is half its input, not a fallback: `git commit` with nothing staged exits 1 and says so on stdout. Also here, each its own small correctness fix: - `cat-file --filters` instead of `show`, so `before` is the working-tree form. The stored blob is LF whatever the tree is, and undo was writing that back over CRLF files, rewriting every line ending in them. - `:(literal)` pathspecs. `git add -- 'a[1].ts'` also stages an unrelated `a1.ts`, so one edited file could sweep untouched work into the auto-commit. - A binary blob is `unavailable` rather than decoded through U+FFFD and then written back corrupted. - `restoreFiles` cannot throw. `undo` is called straight out of the WebSocket message handler, so an EACCES there took the whole daemon down. - `push` and `gh pr create` get a timeout, a closed stdin and `GIT_TERMINAL_PROMPT=0`. Git Credential Manager opens a GUI behind the browser on Windows, and the awaiting handler never replied. - `LC_ALL=C`, so classifying "absent from HEAD" does not depend on git's messages being untranslated. - One 64MB buffer ceiling for every call, not just the blob read. `gitStatus()` is the answer that can be shown to someone: not installed, not a work tree, bare, no commits yet, no identity. `ghStatus` gains the same precision and a cwd, since `gh auth status` resolves its host from the remote. The package had no tests at all, which is how the data-loss bug shipped. It has 34 now, including the reported scenario driven with PATH scrubbed of git. --- packages/core/src/diff-capture.test.ts | 54 +- packages/core/src/diff-capture.ts | 40 +- packages/core/src/runner.ts | 36 +- packages/git/package.json | 4 +- packages/git/src/index.test.ts | 493 ++++++++++++ packages/git/src/index.ts | 700 ++++++++++++++++-- packages/protocol/src/index.ts | 29 + packages/server/src/git-health.test.ts | 83 +++ packages/server/src/index.ts | 118 ++- .../server/src/server.integration.test.ts | 11 + pnpm-lock.yaml | 3 + 11 files changed, 1447 insertions(+), 124 deletions(-) create mode 100644 packages/git/src/index.test.ts create mode 100644 packages/server/src/git-health.test.ts diff --git a/packages/core/src/diff-capture.test.ts b/packages/core/src/diff-capture.test.ts index 80239f8..8c83a05 100644 --- a/packages/core/src/diff-capture.test.ts +++ b/packages/core/src/diff-capture.test.ts @@ -1,9 +1,15 @@ /** - * Covers the three ways the Codex path can arrive at a `before` side. + * Covers the ways the Codex path can arrive at a `before` side. * - * Worth testing specifically because a wrong `before` fails quietly rather than - * loudly: `restoreFiles` skips any diff whose `before` is not a string, so the - * only symptom is undo reporting "restored 0/1 files" long after the fact. + * Worth testing specifically because a wrong `before` picks a different branch + * of `restoreFiles`, and one of them is destructive: a string is rewritten, + * `isNew` is *deleted*, and `noBaseline` is refused. The dangerous confusion is + * between the last two — a baseline that could not be read looks exactly like a + * file that never existed, and undo then deletes a tracked file instead of + * restoring it. + * + * This header used to claim `restoreFiles` "skips any diff whose `before` is + * not a string", which was only ever true of the non-`isNew` branch. */ import { execFileSync } from "node:child_process"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; @@ -90,6 +96,46 @@ describe("DiffCapture on the Codex path", () => { expect(diff.file).toBe("fresh.txt"); expect(diff.before).toBeNull(); expect(diff.isNew).toBe(true); + expect(diff.noBaseline).toBeFalsy(); + }); + + it("keeps a primed baseline even when the head reader is unavailable", () => { + /* + * The two are independent, and `runner.ts` gates them separately for this + * reason. `git status` answers perfectly well in a repository with no + * commits, so priming still yields a real on-disk baseline there — and in a + * fresh repo that is every file, since nothing is tracked yet. Only reading + * a blob out of HEAD needs a HEAD. + * + * If this ever regressed to one gate, undo in a fresh repository would + * refuse every file instead of restoring it. + */ + write("dirty.txt", "before the turn\n"); + const dc = new DiffCapture(repo, () => ({ kind: "unavailable" })); + dc.prime([join(repo, "dirty.txt")]); + + write("dirty.txt", "after the turn\n"); + dc.recordAfterTheFact("dirty.txt"); + + const [diff] = dc.finalize(); + expect(diff.before).toBe("before the turn\n"); + expect(diff.noBaseline).toBeFalsy(); + expect(diff.isNew).toBe(false); + }); + + it("marks a file whose baseline could not be read, rather than calling it new", () => { + // The distinction the whole `HeadRead` union exists for. With git + // unavailable every read comes back like this, and calling them all `isNew` + // is what made undo delete files it had never created. + const dc = new DiffCapture(repo, () => ({ kind: "unavailable" })); + + write("committed.txt", "edited by the agent\n"); + dc.recordAfterTheFact("committed.txt"); + + const [diff] = dc.finalize(); + expect(diff.noBaseline).toBe(true); + expect(diff.isNew).toBe(false); + expect(diff.before).toBeNull(); }); it("leaves a primed file out of the diff unless something writes to it", () => { diff --git a/packages/core/src/diff-capture.ts b/packages/core/src/diff-capture.ts index 5da5c09..f6a4cf9 100644 --- a/packages/core/src/diff-capture.ts +++ b/packages/core/src/diff-capture.ts @@ -15,7 +15,7 @@ */ import { existsSync, readFileSync } from "node:fs"; import { relative, resolve } from "node:path"; -import { canonicalPath } from "@airship/git"; +import { canonicalPath, type HeadRead } from "@airship/git"; import type { FileDiff } from "@airship/protocol"; import { createPatch } from "diff"; import { toPosixPath } from "./paths"; @@ -35,15 +35,18 @@ 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. + * working tree is CRLF while every agent's edit tool writes LF. Comparing a + * file's pre-edit CRLF form against its post-edit LF form makes every line + * 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. + * verbatim and undo has to round-trip exactly. That holds on the git-baseline + * path too, but only because `fileAtHead` reads through `cat-file --filters` + * rather than `show`: the stored blob is LF whatever the working tree is, and + * writing that back over a CRLF file rewrote every line ending in it. * * 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 @@ -79,10 +82,19 @@ function canonical(cwd: string, path: string): string { } /** Reads a path's content as of git HEAD. Injected so core need not know how. */ -export type HeadReader = (absPath: string) => string | null; +export type HeadReader = (absPath: string) => HeadRead; export class DiffCapture { private readonly before = new Map(); + /** + * Paths whose `before` side could not be read at all, as opposed to files + * that genuinely did not exist at HEAD. Kept beside `before` rather than + * folded into it: the map's `null` already means "did not exist", and undo + * deletes what did not exist. Two meanings on one value is what let a machine + * with no git on PATH turn every edit into a creation and every undo into a + * delete. + */ + private readonly unavailable = new Set(); private readonly touched = new Set(); /** Keys are canonical, so the root they are made relative to must be too. */ private readonly root: string; @@ -136,7 +148,11 @@ export class DiffCapture { recordAfterTheFact(filePath: string): void { const abs = canonical(this.cwd, filePath); if (!this.before.has(abs)) { - this.before.set(abs, this.readHead?.(abs) ?? null); + const head = this.readHead?.(abs) ?? { kind: "unavailable" as const }; + if (head.kind === "unavailable") { + this.unavailable.add(abs); + } + this.before.set(abs, head.kind === "content" ? head.text : null); } this.touched.add(abs); } @@ -174,6 +190,7 @@ export class DiffCapture { const rel = toPosixPath(relative(this.root, abs)); const patch = createPatch(rel, forDiff(before), forDiff(after)); const { additions, deletions } = countChanges(patch); + const noBaseline = this.unavailable.has(abs); diffs.push({ additions, after, @@ -181,7 +198,10 @@ export class DiffCapture { deletions, file: rel, isDeleted: after === null, - isNew: before === null, + // A file we could not read a baseline for is not a file the agent + // created, and calling it one is what makes undo delete it. + isNew: before === null && !noBaseline, + ...(noBaseline ? { noBaseline: true } : {}), patch, }); } diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 41bbd07..a628de8 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -6,7 +6,7 @@ * result. Provider-specific work lives behind `AgentAdapter` in `./providers`. */ -import { dirtyFiles, fileAtHead, isGitRepo } from "@airship/git"; +import { dirtyFiles, fileAtHead, gitStatus } from "@airship/git"; import type { AgentKind, AttrEditTarget, @@ -117,14 +117,40 @@ export async function runEdit( // and the pre-turn dirty set is primed as a baseline. Both halves belong // together and neither is provider-specific, so they live here rather than // being copied into every adapter that needs them. + // + // The per-run check is what makes the common failure legible: when git cannot + // run at all, every file would otherwise fail its own read for a reason + // nobody sees. Saying "unavailable" once, up front, marks every diff in the + // turn `noBaseline` so undo refuses cleanly instead of deleting them. The + // per-file answer still matters for the cases that genuinely differ — a path + // outside the work tree, a blob that is not text. + const git = adapter.needsGitBaseline ? gitStatus(input.cwd) : undefined; + /* + * Two gates, not one. `git status` answers perfectly well in a repository + * with no commits, so priming still gets a real on-disk baseline there, and + * in a fresh repo that is every file it holds. Only reading a blob out of + * HEAD needs a HEAD to read from. + * + * What the no-HEAD case still costs: a file the agent *creates* mid-turn was + * not in the pre-turn dirty set and has no HEAD to be absent from, so it is + * recorded as `unavailable` and undo declines to remove it. That is the + * conservative answer and also the correct one — `status -uall` omits + * gitignored files, so "absent from the primed set" does not actually prove + * the file is new, and guessing wrong here deletes something the user had. + */ + const canPrime = Boolean(git?.workTree); + const canReadHead = Boolean(git?.workTree && git.hasCommits); const diffCapture = new DiffCapture( input.cwd, - adapter.needsGitBaseline ? (abs) => fileAtHead(input.cwd, abs) : undefined + adapter.needsGitBaseline + ? (abs) => + canReadHead + ? fileAtHead(input.cwd, abs) + : { kind: "unavailable" as const } + : undefined ); if (adapter.needsGitBaseline) { - diffCapture.prime( - isGitRepo(input.cwd) ? dirtyFiles(input.cwd) : new Set() - ); + diffCapture.prime(canPrime ? dirtyFiles(input.cwd) : new Set()); } // One recorder owns both the persisted array and the live sink, so the diff --git a/packages/git/package.json b/packages/git/package.json index e6a9b77..306ecb1 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -15,12 +15,14 @@ "build": "tsup src/index.ts --format esm --dts --sourcemap --clean", "clean": "node ../../scripts/clean.mjs dist .turbo", "dev": "tsup src/index.ts --format esm --dts --sourcemap --watch", + "test": "vitest run", "typecheck": "tsc --noEmit" }, "dependencies": { "@airship/protocol": "workspace:*" }, "devDependencies": { - "@types/node": "^26.2.0" + "@types/node": "^26.2.0", + "vitest": "^4.1.10" } } diff --git a/packages/git/src/index.test.ts b/packages/git/src/index.test.ts new file mode 100644 index 0000000..efb659c --- /dev/null +++ b/packages/git/src/index.test.ts @@ -0,0 +1,493 @@ +/** + * The package had no tests at all, which is how it shipped a data-loss bug. + * + * Two things are pinned here that nothing else can pin. The first is that a + * failure keeps its reason: every helper used to funnel through `catch { return + * null }`, so a missing commit identity, a stale index.lock, a pathspec that + * matched nothing and a git that was not installed all reached the user as + * "commit failed". The second is the distinction between "this file was not in + * HEAD" and "we could not ask": collapsing those made undo delete tracked + * source files on any machine where git could not run. + * + * Everything runs against a real temp repo and a real git binary. There is no + * mock — the whole subject is what the real one does on the day it goes wrong. + */ +import { execFileSync } from "node:child_process"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { FileDiff } from "@airship/protocol"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + canonicalPath, + commitEdit, + createBranch, + dirtyFiles, + failureText, + fileAtHead, + type GitFailure, + gitStatus, + isGitRepo, + onGitFailure, + restoreFiles, +} from "./index"; + +const VERSION = /^\d+\.\d+/; +const SHA = /^[0-9a-f]{40}$/; +/** git words this several ways across versions, so match the subject. */ +const IDENTITY = /email|identity/i; + +let repo: string; +/** An empty directory, used as a PATH with no git in it. */ +let empty: string; + +function git(...args: string[]): void { + execFileSync("git", args, { cwd: repo, stdio: "ignore" }); +} + +function write(rel: string, body: string): void { + const path = join(repo, rel); + mkdirSync(join(path, ".."), { recursive: true }); + writeFileSync(path, body); +} + +/** A `FileDiff` with only the fields the restore path reads. */ +function diff(over: Partial & { file: string }): FileDiff { + return { + additions: 0, + deletions: 0, + isDeleted: false, + isNew: false, + patch: "", + ...over, + }; +} + +beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), "airship-git-test-")); + empty = mkdtempSync(join(tmpdir(), "airship-git-nopath-")); + // `-b main` keeps `init.defaultBranch` advice off stderr, which would + // otherwise be the first line `failureText` picks out of an unrelated failure. + git("init", "-q", "-b", "main"); + // The developer's own global config must not decide what these assert — the + // identity case in particular passes vacuously on a machine that has one. + // `GIT_CONFIG_GLOBAL=/dev/null` is not portable; an empty file is. + writeFileSync(join(repo, ".gitconfig-empty"), ""); + process.env.GIT_CONFIG_GLOBAL = join(repo, ".gitconfig-empty"); + process.env.GIT_CONFIG_SYSTEM = join(repo, ".gitconfig-empty"); + git("config", "user.email", "test@example.com"); + git("config", "user.name", "Test"); + git("config", "commit.gpgsign", "false"); + // Pinned, so a runner or developer with `core.autocrlf=true` does not turn + // the non-EOL cases into a coin flip. The CRLF case sets its own rule. + git("config", "core.autocrlf", "false"); + write("committed.txt", "one\ntwo\nthree\n"); + git("add", "-A"); + git("commit", "-q", "-m", "initial"); +}); + +afterEach(() => { + onGitFailure(undefined); + // `delete`, not `= undefined`: assigning to `process.env` coerces, so that + // would leave the literal string "undefined" behind as a config path. + delete process.env.GIT_CONFIG_GLOBAL; + delete process.env.GIT_CONFIG_SYSTEM; + for (const dir of [repo, empty]) { + // maxRetries: on Windows a git process can still hold a handle inside .git + // for a moment after it exits, which surfaces as EBUSY. + rmSync(dir, { force: true, maxRetries: 3, recursive: true }); + } +}); + +describe("gitStatus", () => { + it("reports a healthy repository", () => { + const status = gitStatus(repo); + expect(status.installed).toBe(true); + expect(status.version).toMatch(VERSION); + expect(status.workTree).toBe(true); + expect(status.hasCommits).toBe(true); + expect(status.identity).toBe(true); + expect(status.error).toBeUndefined(); + }); + + it("still lists dirty files in a repository with no commits", () => { + // `runner.ts` gates priming on `workTree` and reading HEAD on `hasCommits`, + // separately, because of this: status answers without a HEAD, so a fresh + // repository still gets a real on-disk baseline for every file it holds. + const fresh = mkdtempSync(join(tmpdir(), "airship-git-nohead-")); + execFileSync("git", ["init", "-q"], { cwd: fresh, stdio: "ignore" }); + writeFileSync(join(fresh, "a.txt"), "hi\n"); + const dirty = dirtyFiles(fresh); + expect(dirty.size).toBe(1); + expect([...dirty][0].endsWith("a.txt")).toBe(true); + rmSync(fresh, { force: true, maxRetries: 3, recursive: true }); + }); + + it("separates a repository with no commits from a broken one", () => { + const fresh = mkdtempSync(join(tmpdir(), "airship-git-fresh-")); + execFileSync("git", ["init", "-q"], { cwd: fresh, stdio: "ignore" }); + const status = gitStatus(fresh); + expect(status.workTree).toBe(true); + expect(status.hasCommits).toBe(false); + expect(status.error).toContain("no commits"); + rmSync(fresh, { force: true, maxRetries: 3, recursive: true }); + }); + + it("names a bare repository rather than calling it not-a-repository", () => { + // The regression this pins: `isGitRepo` answers "false" for a bare repo and + // null for a hard failure, and both used to render the same sentence. + const bare = mkdtempSync(join(tmpdir(), "airship-git-bare-")); + execFileSync("git", ["init", "-q", "--bare"], { + cwd: bare, + stdio: "ignore", + }); + const status = gitStatus(bare); + expect(status.installed).toBe(true); + expect(status.workTree).toBe(false); + expect(status.error).toContain("bare"); + rmSync(bare, { force: true, maxRetries: 3, recursive: true }); + }); + + it("reports a directory that is not a repository", () => { + const plain = mkdtempSync(join(tmpdir(), "airship-git-plain-")); + expect(gitStatus(plain).error).toBe("not a git repository"); + rmSync(plain, { force: true, maxRetries: 3, recursive: true }); + }); + + it("reports a missing commit identity", () => { + git("config", "--unset", "user.email"); + const status = gitStatus(repo); + expect(status.identity).toBe(false); + expect(status.error).toContain("identity"); + expect(status.hint).toContain("user.email"); + }); + + it("does not report a nonexistent cwd as a missing git", () => { + // Spawning into a directory that does not exist throws ENOENT, which is + // byte-identical to git not being installed. A mistyped --cwd must not be + // reported as "install git". + const status = gitStatus(join(repo, "no-such-directory")); + expect(status.installed).toBe(true); + expect(status.error).toBe("not a git repository"); + }); + + it("keeps every reason inside the overlay's one-line tooltip budget", () => { + // `GitStatus.error` is rendered in the turn menu's tooltip, where the house + // rule is 44 characters. A path in the message would blow that, which is + // why the banner adds the directory and this does not. + const plain = mkdtempSync(join(tmpdir(), "airship-git-copy-")); + for (const status of [gitStatus(plain), gitStatus(repo)]) { + expect((status.error ?? "").length).toBeLessThanOrEqual(44); + expect(status.error ?? "").not.toContain("—"); + } + rmSync(plain, { force: true, maxRetries: 3, recursive: true }); + }); +}); + +describe("commitEdit", () => { + it("returns the new sha", () => { + write("committed.txt", "one\ntwo\nfour\n"); + const result = commitEdit(repo, ["committed.txt"], "change three to four"); + expect(result.ok).toBe(true); + expect(result.sha).toMatch(SHA); + expect(result.error).toBeUndefined(); + }); + + it("surfaces a pathspec that matched nothing, without leaking our magic", () => { + const result = commitEdit(repo, ["nowhere.txt"], "x"); + expect(result.ok).toBe(false); + expect(result.error).toContain("did not match any files"); + expect(result.error).toContain("nowhere.txt"); + // `:(literal)` is ours, not something the user typed. Leaving it in reads + // as a bug in airship rather than as a missing file. + expect(result.error).not.toContain("literal"); + }); + + it("surfaces 'nothing to commit', which git prints on stdout", () => { + // The case a stderr-only formatter gets wrong. `git commit` exits 1 here and + // says nothing on stderr at all, so the message has to come off stdout. + const result = commitEdit(repo, ["committed.txt"], "no change"); + expect(result.ok).toBe(false); + expect(result.error).toBe("nothing to commit, working tree clean"); + // "On branch main" rides along on stdout and explains nothing; leading with + // it buries the half that does. + expect(result.error).not.toContain("On branch"); + }); + + it("surfaces a missing commit identity", () => { + git("config", "--unset", "user.email"); + git("config", "--unset", "user.name"); + // Without this git invents an identity from the hostname and the commit + // succeeds, so the interesting case would never run on a machine that has + // a resolvable one. `useConfigOnly` is exactly the "do not guess" switch. + git("config", "user.useConfigOnly", "true"); + write("committed.txt", "one\ntwo\nfive\n"); + const result = commitEdit(repo, ["committed.txt"], "x"); + expect(result.ok).toBe(false); + // git words this several ways across versions ("no email was given and + // auto-detection is disabled", "unable to auto-detect email address", + // "Author identity unknown"), so match the subject rather than a sentence. + expect(result.error).toMatch(IDENTITY); + }); + + it("refuses outside a repository, and says which problem it is", () => { + const plain = mkdtempSync(join(tmpdir(), "airship-git-nc-")); + expect(commitEdit(plain, ["a.txt"], "x").error).toBe( + "not a git repository" + ); + rmSync(plain, { force: true, maxRetries: 3, recursive: true }); + }); + + it("treats a filename with glob characters literally", () => { + // Without `:(literal)`, `git add -- 'a[1].ts'` also stages `a1.ts` — so one + // edited file could sweep unrelated work into airship's commit. + write("a[1].ts", "bracketed\n"); + write("a1.ts", "decoy, must not be committed\n"); + const result = commitEdit(repo, ["a[1].ts"], "literal pathspec"); + expect(result.ok).toBe(true); + const staged = execFileSync( + "git", + ["show", "--name-only", "--format=", "HEAD"], + { + cwd: repo, + encoding: "utf8", + } + ); + expect(staged).toContain("a[1].ts"); + expect(staged).not.toContain("a1.ts\n"); + }); +}); + +describe("createBranch", () => { + it("reports why a branch could not be created", () => { + expect(createBranch(repo, "feature").ok).toBe(true); + const again = createBranch(repo, "feature"); + expect(again.ok).toBe(false); + expect(again.error).toContain("already exists"); + }); +}); + +describe("fileAtHead", () => { + it("reads a tracked file", () => { + const read = fileAtHead(repo, join(repo, "committed.txt")); + expect(read).toEqual({ kind: "content", text: "one\ntwo\nthree\n" }); + }); + + it("reports an untracked file as absent, not unavailable", () => { + write("fresh.txt", "new\n"); + expect(fileAtHead(repo, join(repo, "fresh.txt"))).toEqual({ + kind: "absent", + }); + }); + + it("reports a path outside the work tree as unavailable, not absent", () => { + // A file we cannot even ask about is not a file that did not exist. Calling + // it absent marks the diff `isNew`, and undo deletes what an edit created. + expect(fileAtHead(repo, join(repo, "..", "outside.txt"))).toEqual({ + kind: "unavailable", + }); + }); + + it("reports a binary blob as unavailable rather than decoding it", () => { + writeFileSync( + join(repo, "logo.bin"), + Buffer.from([0x00, 0xff, 0xfe, 0x41]) + ); + git("add", "-A"); + git("commit", "-q", "-m", "binary"); + // Decoded as UTF-8 it would come back full of U+FFFD, and `restoreFiles` + // would write that corruption over the user's file. + expect(fileAtHead(repo, join(repo, "logo.bin"))).toEqual({ + kind: "unavailable", + }); + }); + + it("returns the working-tree form, not the stored blob", () => { + // `.gitattributes` rather than `core.autocrlf`, so this is the same test on + // Linux and Windows. The blob is LF; a checkout of it is CRLF; `before` has + // to be the second, because `restoreFiles` writes it straight back to disk. + write(".gitattributes", "*.txt text eol=crlf\n"); + write("crlf.txt", "alpha\nbeta\n"); + git("add", "-A"); + git("commit", "-q", "-m", "crlf"); + const read = fileAtHead(repo, join(repo, "crlf.txt")); + expect(read).toEqual({ kind: "content", text: "alpha\r\nbeta\r\n" }); + }); +}); + +describe("restoreFiles", () => { + it("rewrites a file from its before content", () => { + write("committed.txt", "edited\n"); + const result = restoreFiles(repo, [ + diff({ before: "one\ntwo\nthree\n", file: "committed.txt" }), + ]); + expect(result.restored).toEqual(["committed.txt"]); + expect(result.skipped).toEqual([]); + expect(fileAtHead(repo, join(repo, "committed.txt"))).toEqual({ + kind: "content", + text: "one\ntwo\nthree\n", + }); + }); + + it("deletes a file the edit created", () => { + write("added.txt", "new\n"); + const result = restoreFiles(repo, [ + diff({ file: "added.txt", isNew: true }), + ]); + expect(result.restored).toEqual(["added.txt"]); + expect(existsSync(join(repo, "added.txt"))).toBe(false); + }); + + it("refuses a file with no baseline instead of deleting it", () => { + // The headline regression. `noBaseline` is checked before `isNew`, because a + // baseline that could not be read used to be indistinguishable from a file + // that never existed, and undo deleted the difference. + write("committed.txt", "edited by the agent\n"); + const result = restoreFiles(repo, [ + diff({ file: "committed.txt", isNew: true, noBaseline: true }), + ]); + expect(result.restored).toEqual([]); + expect(result.skipped).toEqual([ + { + file: "committed.txt", + reason: "no baseline was captured, so there is nothing to restore", + }, + ]); + // Still on disk, and still holding the agent's edit: refusing is the point. + expect(readFileSync(join(repo, "committed.txt"), "utf8")).toBe( + "edited by the agent\n" + ); + }); + + it("reports a write failure instead of throwing", () => { + // `undo` is called straight out of the WebSocket message handler with no + // try/catch of its own, so a throw here takes the whole daemon down with it. + // + // A directory standing where the file should be, rather than a read-only + // bit: the POSIX permission bits do not mean the same thing on NTFS, and + // this failure is identical on both. + mkdirSync(join(repo, "blocked.txt"), { recursive: true }); + const result = restoreFiles(repo, [ + diff({ before: "original\n", file: "blocked.txt" }), + ]); + expect(result.restored).toEqual([]); + expect(result.skipped).toHaveLength(1); + expect(result.skipped[0].file).toBe("blocked.txt"); + }); +}); + +describe("failureText", () => { + const base: GitFailure = { + args: ["commit"], + bin: "git", + code: 1, + stderr: "", + stdout: "", + }; + + it("names a missing binary", () => { + expect(failureText({ ...base, code: null, errno: "ENOENT" })).toBe( + "git is not installed or not on PATH" + ); + }); + + it("prefers git's own marked lines over its progress output", () => { + expect( + failureText({ + ...base, + stderr: "Enumerating objects: 5, done.\nfatal: repository not found", + }) + ).toBe("fatal: repository not found"); + }); + + it("falls back to a description when there is no output at all", () => { + expect(failureText(base)).toBe("git commit exited 1"); + }); + + it("stays on one line", () => { + const text = failureText({ + ...base, + stderr: `fatal: ${"x".repeat(400)}`, + }); + expect(text).not.toContain("\n"); + expect(text.length).toBeLessThanOrEqual(300); + }); +}); + +describe("with git not on PATH", () => { + /* + * The reported scenario, and the one that used to end in deleted files. + * + * Git for Windows' "Git from Git Bash only" install option leaves git.exe off + * the system PATH, so every call from the daemon fails while `git` works + * perfectly in the contributor's own terminal. + * + * PATH is set to a real empty directory rather than to "", which behaves + * differently across platforms. On Windows `process.env` lookup is + * case-insensitive, so assigning `PATH` also overrides an existing `Path`; + * CreateProcess additionally searches the application directory and the cwd, + * neither of which holds a git. + */ + let saved: string | undefined; + + beforeEach(() => { + saved = process.env.PATH; + process.env.PATH = empty; + }); + + afterEach(() => { + process.env.PATH = saved; + }); + + it("reports git as missing rather than the directory as not-a-repository", () => { + const status = gitStatus(repo); + expect(status.installed).toBe(false); + expect(status.error).toBe("git is not installed or not on PATH"); + expect(status.hint).toContain("PATH"); + }); + + it("makes fileAtHead unavailable, never absent", () => { + // The single most important assertion in this file. `absent` here would + // mark every edited file `isNew`, and undo deletes what an edit created. + expect(fileAtHead(repo, join(repo, "committed.txt"))).toEqual({ + kind: "unavailable", + }); + }); + + it("makes commitEdit say so", () => { + const result = commitEdit(repo, ["committed.txt"], "x"); + expect(result.ok).toBe(false); + expect(result.error).toBe("git is not installed or not on PATH"); + }); + + it("still answers the yes/no probes without throwing", () => { + expect(isGitRepo(repo)).toBe(false); + expect(dirtyFiles(repo).size).toBe(0); + }); + + it("hands every failure to the diagnostic sink", () => { + const seen: GitFailure[] = []; + onGitFailure((failure) => seen.push(failure)); + isGitRepo(repo); + expect(seen).toHaveLength(1); + expect(seen[0].bin).toBe("git"); + expect(seen[0].errno).toBe("ENOENT"); + expect(seen[0].args).toEqual(["rev-parse", "--is-inside-work-tree"]); + }); +}); + +describe("canonicalPath", () => { + it("resolves a path that does not exist via its parent", () => { + // The create case: `DiffCapture` keys on a file before it is written. + expect(canonicalPath(join(repo, "not-yet.txt"))).toBe( + join(canonicalPath(repo), "not-yet.txt") + ); + }); +}); diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index d637532..ba340b1 100644 --- a/packages/git/src/index.ts +++ b/packages/git/src/index.ts @@ -2,6 +2,13 @@ * @airship/git — minimal git helpers for the daemon: optional auto-commit of an * accepted edit (with a Conventional-Commits message) and a content-restore * undo that rewrites files to their pre-edit state. + * + * Every invocation goes through `run`, which never throws and never discards + * what went wrong. That is not incidental: this module used to be a wall of + * `catch { return null }`, so a missing git, a missing commit identity, a stale + * index.lock and a pathspec that matched nothing all reached the user as the + * same three words — "commit failed" — and a git that could not run at all was + * reported as "not a git repository". */ import { execFile, execFileSync } from "node:child_process"; import { @@ -17,38 +24,279 @@ import type { FileDiff } from "@airship/protocol"; const execFileAsync = promisify(execFile); -function git(cwd: string, args: string[]): string { - return execFileSync("git", args, { - cwd, - encoding: "utf8", - stdio: ["ignore", "pipe", "pipe"], - }).trim(); +/** + * A single blob can exceed Node's 1MB default and would otherwise throw — and + * the throw would be indistinguishable from every other failure. Applied to + * every invocation rather than only the blob read, because `git push` against a + * chatty remote can outrun 1MB of progress output just as easily. + */ +const MAX_BUFFER = 64 * 1024 * 1024; + +/** How long a network call may block before it is killed. See `tryExec`. */ +const NETWORK_TIMEOUT_MS = 120_000; + +/** + * The environment every child gets. + * + * `LC_ALL`/`LANG` because git's messages are translated when locales are + * installed, and `fileAtHead` classifies "absent from HEAD" by matching git's + * own wording — a French checkout would otherwise be read as an unavailable + * baseline. It also keeps what the user is shown stable and searchable. + * + * The prompt-suppressing half matters most on Windows, where Git Credential + * Manager opens a *GUI* behind the browser: the daemon is also proxying the + * user's dev server, so a push that sits waiting on an invisible dialog hangs + * the WebSocket handler with nothing to cancel it. + */ +const GIT_ENV = { + GCM_INTERACTIVE: "never", + GH_PROMPT_DISABLED: "1", + GIT_TERMINAL_PROMPT: "0", + LANG: "C", + LC_ALL: "C", + SSH_ASKPASS_REQUIRE: "never", +} as const; + +/** Everything a failed invocation knows about itself. */ +export interface GitFailure { + /** The argv after `bin`, so a message can say which command failed. */ + args: string[]; + /** The executable — "git", or "gh" on the pull-request path. */ + bin: string; + /** Exit status, or null when the process never started. */ + code: number | null; + cwd?: string; + /** `ENOENT` when the binary is not on PATH, `EACCES` when not executable. */ + errno?: string; + stderr: string; + /** Not a fallback for `stderr` — see `failureText`. */ + stdout: string; } -function tryGit(cwd: string, args: string[]): string | null { - try { - return git(cwd, args); - } catch { - return null; +type GitRun = { ok: true; stdout: T } | { failure: GitFailure; ok: false }; + +/** + * A diagnostic tap, for `--debug`. Deliberately *not* how error text reaches + * the user: the daemon serves several sockets at once and a module-level sink + * delivers a failure out of band from the call that produced it, so two clients + * committing at the same time would cross wires and toast each other's error. + * Text that must be correlated to a call travels on that call's return value. + */ +let sink: ((failure: GitFailure) => void) | undefined; + +export function onGitFailure( + fn: ((failure: GitFailure) => void) | undefined +): void { + sink = fn; +} + +function text(value: Buffer | string | undefined): string { + if (value === undefined) { + return ""; } + return typeof value === "string" ? value : value.toString("utf8"); +} + +function toFailure( + bin: string, + args: string[], + cwd: string | undefined, + err: unknown +): GitFailure { + // execFileSync throws an object whose `status` is the exit code, and whose + // `code` is a spawn errno string only when the process never started. + const e = err as { + code?: number | string; + status?: number | null; + stderr?: Buffer | string; + stdout?: Buffer | string; + }; + return { + args, + bin, + code: typeof e.status === "number" ? e.status : null, + cwd, + errno: typeof e.code === "string" ? e.code : undefined, + stderr: text(e.stderr), + stdout: text(e.stdout), + }; +} + +function fail(failure: GitFailure): GitRun { + sink?.(failure); + return { failure, ok: false }; } /** - * Untrimmed variant. File contents and NUL-delimited porcelain output both - * carry significant leading/trailing bytes, so they cannot go through `git()`. + * `cwd` is checked before spawning because a directory that does not exist + * makes spawn throw `ENOENT` — byte-identical to git not being installed. A + * mistyped `--cwd` would otherwise be reported as a missing git, which is the + * exact class of wrong answer this module exists to stop giving. + */ +function missingCwd(bin: string, args: string[], cwd: string): GitFailure { + return { + args, + bin, + code: null, + cwd, + errno: "ENOTDIR", + stderr: `${cwd} does not exist`, + stdout: "", + }; +} + +function runBin( + bin: string, + cwd: string | undefined, + args: string[] +): GitRun { + if (cwd !== undefined && !existsSync(cwd)) { + return fail(missingCwd(bin, args, cwd)); + } + try { + return { + ok: true, + stdout: execFileSync(bin, args, { + cwd, + encoding: "utf8", + env: { ...process.env, ...GIT_ENV }, + maxBuffer: MAX_BUFFER, + // stdin closed: a synchronous git must never be able to block on a + // prompt. stdout/stderr piped so a failure carries git's own words. + stdio: ["ignore", "pipe", "pipe"], + windowsHide: true, + }), + }; + } catch (err) { + return fail(toFailure(bin, args, cwd, err)); + } +} + +function run(cwd: string | undefined, args: string[]): GitRun { + return runBin("git", cwd, args); +} + +/** + * The byte-exact variant, for content that must not be decoded on trust. + * `fileAtHead` needs it: a blob that is not valid UTF-8 comes back through + * `encoding: "utf8"` full of U+FFFD, and `restoreFiles` would then write that + * corruption over the user's file. + */ +function runBytes(cwd: string, args: string[]): GitRun { + if (!existsSync(cwd)) { + return fail(missingCwd("git", args, cwd)); + } + try { + return { + ok: true, + stdout: execFileSync("git", args, { + cwd, + env: { ...process.env, ...GIT_ENV }, + maxBuffer: MAX_BUFFER, + stdio: ["ignore", "pipe", "pipe"], + windowsHide: true, + }), + }; + } catch (err) { + return fail(toFailure("git", args, cwd, err)); + } +} + +/** git's own markers for the line that actually explains a failure. */ +const GIT_MARKER = /^(?:fatal|error|remote|!)\b/; +/** + * Status preamble that rides along on stdout and never explains anything. `git + * commit` with nothing staged prints "On branch main" before the line that + * matters, and leading with it buries the answer. + */ +const GIT_PREAMBLE = /^(?:On branch |Your branch |HEAD detached)/; +/** Both terminators: git output reaching us on Windows carries CRLF. */ +const NEWLINE = /\r?\n/; +/** + * Our own pathspec magic, stripped back out of anything a user reads. + * + * `commitEdit` wraps every path in `:(literal)` so a filename holding a glob + * character is matched literally. That is an implementation detail, and leaving + * it in `pathspec ':(literal)src/app.ts' did not match` reads as a bug in + * airship rather than as a missing file. + */ +const LITERAL_MAGIC = /:\(literal\)/g; +const MAX_FAILURE_LINES = 3; +const MAX_FAILURE_CHARS = 300; + +/** + * The lines most likely to explain the failure. + * + * Marked lines first, then anything that is not status preamble, then whatever + * there is. Each step only applies when it leaves something to say, so a + * failure whose entire output is one unmarked preamble line still reports it + * rather than reporting nothing. + */ +function explanatory(lines: string[]): string[] { + const marked = lines.filter((line) => GIT_MARKER.test(line)); + if (marked.length > 0) { + return marked; + } + const plain = lines.filter((line) => !GIT_PREAMBLE.test(line)); + return plain.length > 0 ? plain : lines; +} + +/** + * The one line worth showing a user: git's own words, or ours when it has none. + * + * `stdout` is not a fallback for `stderr`, it is half the input. `git commit` + * with nothing staged exits 1 and prints "nothing to commit, working tree + * clean" on *stdout* — the single commonest commit failure, which a + * stderr-only formatter renders as the useless "git commit exited 1". + * + * Capped and joined onto one line because `toast()` in the overlay takes a + * plain string and renders a single message. The full text is what `--debug` + * is for. + */ +export function failureText(failure: GitFailure): string { + if (failure.errno === "ENOTDIR") { + return failure.stderr; + } + if (failure.errno === "ENOENT") { + return `${failure.bin} is not installed or not on PATH`; + } + if (failure.errno === "EACCES") { + return `${failure.bin} is not executable`; + } + // Windows only. Node has refused to spawn a .bat/.cmd without a shell since + // 18.20/20.12, and a bare EINVAL here reads as a bug in airship rather than + // as the fixable packaging problem it is. `gh` installed through npm or a + // scoop shim is the way to reach this; the official gh.exe is not. + if (failure.errno === "EINVAL") { + return `${failure.bin} resolves to a .cmd or .bat shim, which cannot be launched directly`; + } + const lines = `${failure.stderr}\n${failure.stdout}` + .split(NEWLINE) + .map((line) => line.trim().replace(LITERAL_MAGIC, "")) + .filter(Boolean); + const chosen = explanatory(lines).slice(0, MAX_FAILURE_LINES); + if (chosen.length === 0) { + return `${failure.bin} ${failure.args[0] ?? "command"} exited ${failure.code ?? "abnormally"}`; + } + const joined = chosen.join(" · "); + return joined.length > MAX_FAILURE_CHARS + ? `${joined.slice(0, MAX_FAILURE_CHARS - 1)}…` + : joined; +} + +/** Lossy by design: for probes whose only question is yes/no. */ +function tryGit(cwd: string, args: string[]): string | null { + const result = run(cwd, args); + return result.ok ? result.stdout.trim() : null; +} + +/** + * Untrimmed variant. NUL-delimited porcelain output carries significant + * trailing bytes, so it cannot go through `tryGit`. */ function tryGitRaw(cwd: string, args: string[]): string | null { - try { - return execFileSync("git", args, { - cwd, - encoding: "utf8", - // A single blob can exceed Node's 1MB default and would otherwise throw. - maxBuffer: 64 * 1024 * 1024, - stdio: ["ignore", "pipe", "pipe"], - }); - } catch { - return null; - } + const result = run(cwd, args); + return result.ok ? result.stdout : null; } /** @@ -58,25 +306,68 @@ function tryGitRaw(cwd: string, args: string[]): string | null { * microseconds and the call sites read better for it. `push` does not: it can * block for seconds on a slow remote, and this process is also proxying the * user's dev server, so a synchronous push would freeze their app mid-edit. + * + * The timeout is the other half of that. Without it a push waiting on + * credentials never returns, the awaiting socket handler never replies, and no + * child handle is kept anywhere that could kill it. */ async function tryExec( bin: string, args: string[], - cwd: string + cwd: string, + timeoutMs: number = NETWORK_TIMEOUT_MS ): Promise<{ error?: string; ok: boolean; stdout: string }> { - try { - const { stdout } = await execFileAsync(bin, args, { cwd }); - return { ok: true, stdout: stdout.trim() }; - } catch (err) { - const e = err as { stderr?: string; message?: string }; + if (!existsSync(cwd)) { return { - error: (e.stderr || e.message || "command failed").trim(), + error: failureText(missingCwd(bin, args, cwd)), ok: false, stdout: "", }; } + const pending = execFileAsync(bin, args, { + cwd, + env: { ...process.env, ...GIT_ENV }, + killSignal: "SIGKILL", + maxBuffer: MAX_BUFFER, + timeout: timeoutMs, + windowsHide: true, + }); + // Nobody is going to write to it, and git will happily sit reading a pipe. + pending.child.stdin?.end(); + try { + const { stdout } = await pending; + return { ok: true, stdout: stdout.trim() }; + } catch (err) { + const e = err as { killed?: boolean }; + if (e.killed) { + return { + error: `${bin} timed out after ${Math.round(timeoutMs / 1000)}s, most likely waiting on credentials`, + ok: false, + stdout: "", + }; + } + const failure = toFailure(bin, args, cwd, err); + // execFile reports the exit status on `code`, where execFileSync uses + // `status`; a numeric `code` is therefore an exit, not a spawn errno. + const exit = (err as { code?: number | string }).code; + if (typeof exit === "number") { + failure.code = exit; + failure.errno = undefined; + } + sink?.(failure); + return { error: failureText(failure), ok: false, stdout: "" }; + } } +/** + * Is `cwd` inside a work tree? + * + * The cheap yes/no, and deliberately nothing more: one `rev-parse`, no reason + * attached. It answers `false` for a bare repository, a directory that is not a + * repository, and a git that will not run, which is why it is no longer what + * anything user-facing asks. `gitStatus` is the answer that can be shown to + * someone; this is the guard `commitEdit` takes before paying for one. + */ export function isGitRepo(cwd: string): boolean { return tryGit(cwd, ["rev-parse", "--is-inside-work-tree"]) === "true"; } @@ -109,9 +400,137 @@ export function hasRemote(cwd: string, name = "origin"): boolean { return Boolean(tryGit(cwd, ["remote", "get-url", name])); } +export interface GitStatus { + /** + * The first thing that is wrong, phrased for a user. + * + * Short, and carrying no path: it is rendered in the overlay's tooltip box, + * where the house budget is one line of about 44 characters. Callers with + * more room, like the launch banner, add the directory themselves. + */ + error?: string; + /** HEAD resolves. A fresh `git init` has no commits and so no baseline. */ + hasCommits: boolean; + /** The command that fixes `error`. */ + hint?: string; + /** Both halves of the commit identity are configured. */ + identity: boolean; + installed: boolean; + /** e.g. "2.43.0". Present whenever `installed`. */ + version?: string; + /** `cwd` is inside a work tree — not bare, not outside a repo. */ + workTree: boolean; +} + +const GIT_VERSION = /(\d+\.\d+\.\d+)/; + +const NOT_INSTALLED: GitStatus = { + hasCommits: false, + identity: false, + installed: false, + workTree: false, +}; + /** - * Content of a file as of HEAD, or null when it does not exist there (untracked, - * newly added, or outside the repo). + * Why git will not work here, in the order the answers depend on each other. + * + * The same shape as `ghStatus`, and for the same reason its comment gives: + * "installed but unusable" is the common case and fails much later with a + * confusing message. Up to five spawns, so this is a preflight — `doctor`, the + * launch banner, the PR gate and once per turn. Never per edit. + */ +export function gitStatus(cwd: string): GitStatus { + // No `cwd`: "is git installed" has to be answerable when cwd is garbage. + const version = run(undefined, ["--version"]); + if (!version.ok) { + return { + ...NOT_INSTALLED, + error: failureText(version.failure), + hint: 'Install Git and make sure `git` is on PATH. On Windows, re-run the installer and choose "Git from the command line".', + }; + } + const base = { + installed: true, + version: GIT_VERSION.exec(version.stdout)?.[1], + }; + const inside = run(cwd, ["rev-parse", "--is-inside-work-tree"]); + if (!inside.ok) { + return { + ...base, + error: "not a git repository", + hasCommits: false, + hint: "Run `git init` there, or point --cwd at a repository.", + identity: false, + workTree: false, + }; + } + // "false" rather than a failure: a bare repo answers the question and is + // still unusable here. These were the same answer until this function existed. + if (inside.stdout.trim() !== "true") { + return { + ...base, + error: "this is a bare git repository", + hasCommits: false, + hint: "Airship edits a working tree; open a normal clone instead.", + identity: false, + workTree: false, + }; + } + const hasCommits = run(cwd, ["rev-parse", "--verify", "--quiet", "HEAD"]).ok; + const identity = + Boolean(tryGit(cwd, ["config", "--get", "user.email"])) && + Boolean(tryGit(cwd, ["config", "--get", "user.name"])); + if (!hasCommits) { + return { + ...base, + error: "this repository has no commits yet", + hasCommits, + hint: "Make one commit. Without HEAD there is no baseline to diff or undo against.", + identity, + workTree: true, + }; + } + if (!identity) { + return { + ...base, + error: "git has no commit identity", + hasCommits, + hint: 'Run `git config --global user.email you@example.com` and `git config --global user.name "Your Name"`.', + identity, + workTree: true, + }; + } + return { ...base, hasCommits, identity, workTree: true }; +} + +/** + * What a file looked like at HEAD. + * + * Three answers, not two. "absent" is a fact about HEAD; "unavailable" is a + * fact about us — and collapsing them into one null is what let undo delete + * tracked files on a machine with no git on PATH: every read failed, every file + * was therefore recorded as newly created, and undo deletes what an edit + * created. See `restoreFiles`. + */ +export type HeadRead = + | { kind: "absent" } + | { kind: "content"; text: string } + | { kind: "unavailable" }; + +const ABSENT_FROM_HEAD = /does not exist in|exists on disk, but not in/; +const UNKNOWN_OPTION = /unknown option|error: unknown switch/; + +/** + * Whether this git understands `cat-file --filters`, memoized. + * + * Deliberately not a version probe: parsing `git --version` costs a spawn and + * distro builds spell it in ways worth not depending on ("2.39.5 (Apple + * Git-154)"). One wasted invocation on a pre-2.11 git, once per process. + */ +let filtersSupported: boolean | undefined; + +/** + * Content of a file as of HEAD. * * This is how the Codex path reconstructs a diff's `before` side. That backend * has no pre-tool hook, so unlike the Claude path there is no opportunity to @@ -119,23 +538,63 @@ export function hasRemote(cwd: string, name = "origin"): boolean { * changed, the new content is already on disk. For a file that was clean when * the turn started, its HEAD blob is exactly the pre-turn content. * + * `cat-file --filters` rather than `show`, because `before` is written straight + * back to disk by `restoreFiles` and therefore has to be the *working tree* + * form, not the stored blob. `core.autocrlf=true` is the Git-for-Windows + * default, so `show` returns LF for a file that is CRLF on disk and undo + * silently rewrote every line ending in the file. `--filters` applies the same + * conversion a checkout would, including `.gitattributes` and LFS smudge. + * * `path` may be absolute or relative to `cwd`; `HEAD:./…` resolves against the * working directory rather than the repo root, so a project root that sits in a * subdirectory of the repo still works. */ -export function fileAtHead(cwd: string, path: string): string | null { +export function fileAtHead(cwd: string, path: string): HeadRead { // Both sides are canonicalized before being compared. Callers hand us paths // 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 = 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. + // `..` means the path really does escape the working directory. Unavailable + // rather than absent: a file outside the tree may or may not have existed, + // and guessing "it did not" is a licence to delete it. if (!rel || rel.startsWith("..")) { - return null; + return { kind: "unavailable" }; } - return tryGitRaw(cwd, ["show", `HEAD:./${rel.split(sep).join("/")}`]); + const spec = `HEAD:./${rel.split(sep).join("/")}`; + const result = readBlob(cwd, spec); + if (!result.ok) { + return ABSENT_FROM_HEAD.test(result.failure.stderr) + ? { kind: "absent" } + : { kind: "unavailable" }; + } + try { + // `fatal` is the precise test for "is this losslessly text". A blob that is + // not — a PNG, a latin-1 source file — must not be decoded on trust and + // then written back over the original. + return { + kind: "content", + text: new TextDecoder("utf8", { fatal: true }).decode(result.stdout), + }; + } catch { + return { kind: "unavailable" }; + } +} + +function readBlob(cwd: string, spec: string): GitRun { + if (filtersSupported !== false) { + const filtered = runBytes(cwd, ["cat-file", "--filters", spec]); + if (filtered.ok) { + filtersSupported = true; + return filtered; + } + if (!UNKNOWN_OPTION.test(filtered.failure.stderr)) { + return filtered; + } + filtersSupported = false; + } + return runBytes(cwd, ["show", spec]); } /** @@ -146,7 +605,7 @@ export function fileAtHead(cwd: string, path: string): string | null { * 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 + * above 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. * @@ -187,6 +646,9 @@ export function dirtyFiles(cwd: string): Set { // NUL-delimited records: `XY `. A rename or copy spends a second field // on its origin path, which must be consumed as data rather than parsed as // the next status record — hence a cursor rather than a for-of. + // + // `-z` is also what makes `core.quotepath` a non-issue: with it git emits raw + // bytes and never applies C-style quoting. const fields = raw.split("\0"); let i = 0; while (i < fields.length) { @@ -208,8 +670,14 @@ export function dirtyFiles(cwd: string): Set { return out; } -export function createBranch(cwd: string, name: string): boolean { - return tryGit(cwd, ["checkout", "-b", name]) !== null; +export function createBranch( + cwd: string, + name: string +): { error?: string; ok: boolean } { + const result = run(cwd, ["checkout", "-b", name]); + return result.ok + ? { ok: true } + : { error: failureText(result.failure), ok: false }; } export interface GhStatus { @@ -218,24 +686,34 @@ export interface GhStatus { installed: boolean; } -/** Whether `gh` is usable. Both halves matter — installed but unauthenticated - * is the common case and fails much later with a confusing message. */ -export function ghStatus(): GhStatus { - try { - execFileSync("gh", ["--version"], { stdio: "ignore" }); - } catch { - return { authed: false, error: "gh is not installed", installed: false }; - } - try { - execFileSync("gh", ["auth", "status"], { stdio: "ignore" }); - return { authed: true, installed: true }; - } catch { +/** + * Whether `gh` is usable. Both halves matter — installed but unauthenticated is + * the common case and fails much later with a confusing message. + * + * `cwd` is passed to `auth status` and not to `--version`: the first resolves + * which host to check from the repository's own remote, the second must be + * answerable anywhere. + */ +export function ghStatus(cwd?: string): GhStatus { + const version = runBin("gh", undefined, ["--version"]); + if (!version.ok) { return { authed: false, - error: "gh is installed but not authenticated (`gh auth login`)", - installed: true, + error: failureText(version.failure), + installed: false, }; } + const auth = runBin("gh", cwd, ["auth", "status"]); + if (auth.ok) { + return { authed: true, installed: true }; + } + // gh's own text, rather than a guess: an expired token, an unconfigured host + // and a network failure are three different problems with three fixes. + return { + authed: false, + error: `gh is installed but not usable: ${failureText(auth.failure)}`, + installed: true, + }; } /** Push a branch and set upstream. Async — see `tryExec`. */ @@ -279,8 +757,9 @@ export async function createPr( if (!result.ok) { return { error: result.error, ok: false }; } - // `gh` prints the PR URL as its last line. - const url = result.stdout.split("\n").filter(Boolean).pop(); + // `gh` prints the PR URL as its last line. Split on both terminators: the + // trim above only reaches the last one. + const url = result.stdout.split(NEWLINE).filter(Boolean).pop(); return { ok: true, url }; } @@ -288,52 +767,115 @@ export function currentSha(cwd: string): string | null { return tryGit(cwd, ["rev-parse", "HEAD"]); } +export interface CommitResult { + error?: string; + ok: boolean; + sha?: string; +} + /** * Stage and commit exactly the given files with a Conventional-Commits message. - * Returns the new commit sha, or null if nothing was committed. + * + * Every path is wrapped in `:(literal)` because the arguments after `--` are + * pathspecs, not filenames: without it `git add -- 'a[1].ts'` stages both + * `a[1].ts` and any file the glob happens to match, so a single edited file can + * sweep unrelated work into airship's commit. */ export function commitEdit( cwd: string, files: string[], summary: string -): string | null { - if (files.length === 0 || !isGitRepo(cwd)) { - return null; +): CommitResult { + if (files.length === 0) { + return { error: "nothing to commit", ok: false }; + } + // The cheap predicate first, and the five-spawn diagnosis only once it has + // already failed. `isGitRepo` alone is what used to report a git that is not + // installed as "not a git repository"; paying for the real answer on the + // unhappy path costs nothing anybody waits for. + if (!isGitRepo(cwd)) { + return { error: gitStatus(cwd).error ?? "git is unavailable", ok: false }; } const subject = `chore(airship): ${summary.trim() || "visual edit"}`.slice( 0, 100 ); - try { - git(cwd, ["add", "--", ...files]); - git(cwd, ["commit", "--no-verify", "-m", subject, "--", ...files]); - return currentSha(cwd); - } catch { - return null; + const specs = files.map((file) => `:(literal)${file}`); + const staged = run(cwd, ["add", "--", ...specs]); + if (!staged.ok) { + return { error: failureText(staged.failure), ok: false }; } + const committed = run(cwd, [ + "commit", + "--no-verify", + "-m", + subject, + "--", + ...specs, + ]); + if (!committed.ok) { + return { error: failureText(committed.failure), ok: false }; + } + return { ok: true, sha: currentSha(cwd) ?? undefined }; +} + +export interface RestoreResult { + /** Repo-relative paths that were rewritten or deleted. */ + restored: string[]; + /** Paths left alone, and why. */ + skipped: { file: string; reason: string }[]; } /** * Content-restore undo: rewrite each touched file back to its pre-edit content * (deleting files the edit created). Pure filesystem — works whether or not the - * project is a git repo. Returns the repo-relative paths that were restored. + * project is a git repo. + * + * `noBaseline` is checked *before* `isNew`, and that order is the whole point. + * A baseline we could not read used to look exactly like a file that never + * existed, so undo deleted tracked source files on any machine where git could + * not run. Refusing and saying so is the only safe answer. + * + * Nothing here may throw: `undo` is called straight out of the WebSocket + * message handler, so an EACCES on one read-only file would otherwise take the + * whole daemon down with it. */ -export function restoreFiles(cwd: string, diffs: FileDiff[]): string[] { +export function restoreFiles(cwd: string, diffs: FileDiff[]): RestoreResult { const restored: string[] = []; + const skipped: { file: string; reason: string }[] = []; for (const d of diffs) { const abs = resolve(cwd, d.file); - if (d.isNew) { - if (existsSync(abs)) { - rmSync(abs, { force: true }); - } - restored.push(d.file); + if (d.noBaseline) { + skipped.push({ + file: d.file, + reason: "no baseline was captured, so there is nothing to restore", + }); continue; } - if (typeof d.before === "string") { - mkdirSync(dirname(abs), { recursive: true }); - writeFileSync(abs, d.before, "utf8"); - restored.push(d.file); + try { + if (d.isNew) { + if (existsSync(abs)) { + rmSync(abs, { force: true }); + } + restored.push(d.file); + continue; + } + if (typeof d.before === "string") { + mkdirSync(dirname(abs), { recursive: true }); + writeFileSync(abs, d.before, "utf8"); + restored.push(d.file); + continue; + } + skipped.push({ + file: d.file, + reason: "no previous content was recorded", + }); + } catch (err) { + skipped.push({ + file: d.file, + reason: err instanceof Error ? err.message : String(err), + }); } } - return restored; + return { restored, skipped }; } diff --git a/packages/protocol/src/index.ts b/packages/protocol/src/index.ts index 7a8deb9..61a85cb 100644 --- a/packages/protocol/src/index.ts +++ b/packages/protocol/src/index.ts @@ -422,6 +422,18 @@ export const FileDiffSchema = z.object({ file: z.string(), isDeleted: z.boolean(), isNew: z.boolean(), + /** + * No `before` side could be captured — git could not answer, not "the file + * did not exist". + * + * Named rather than left implicit, because the two states used to share one + * `before: null` and undo read the wrong one: a baseline it could not read + * looked exactly like a file the edit had created, so it deleted tracked + * source files on any machine where git could not run. `restoreFiles` checks + * this before `isNew` and refuses. Optional, so bundles written before this + * field existed still parse. + */ + noBaseline: z.boolean().optional(), patch: z.string(), }); export type FileDiff = z.infer; @@ -654,10 +666,27 @@ export interface JobSnapshot { // WebSocket protocol: server → client // --------------------------------------------------------------------------- +/** + * Whether git-backed actions can work at all, and why not when they cannot. + * + * Sent so the overlay can grey out Revert, Commit and Create pull request with + * the reason attached, instead of offering them and failing after the click. + */ +export interface GitHealth { + /** What to do about it. Shown under `reason` in the same tooltip. */ + hint?: string; + ok: boolean; + /** Absent when `ok`. A sentence, already phrased for a user. */ + reason?: string; +} + export type ServerEvent = /** `defaultAgent` is the daemon's `--agent` setting, so the composer's picker * can render the right backend on first paint instead of guessing. */ | { type: "hello"; jobs: JobSnapshot[]; defaultAgent: AgentKind } + /** Pushed on connect and again after every turn, since a repo can gain its + * first commit — or lose its git — while the overlay is open. */ + | { type: "git:health"; health: GitHealth } | { type: "job:created"; job: JobSnapshot } /** Coarse one-line status ("Reading foo.tsx"). Self-overwriting by design — * it drives the turn's live status pill, not the transcript body. */ diff --git a/packages/server/src/git-health.test.ts b/packages/server/src/git-health.test.ts new file mode 100644 index 0000000..ac7de53 --- /dev/null +++ b/packages/server/src/git-health.test.ts @@ -0,0 +1,83 @@ +/** + * Which git problems reach the overlay as "these buttons will not work". + * + * The distinction that matters is between a git that cannot serve the verbs at + * all and a repository that simply has no HEAD yet. The second one greys + * nothing: committing is what gives a fresh repository its first commit, so a + * Commit button disabled for want of a HEAD disables the cure. + */ +import type { GitStatus } from "@airship/git"; +import { describe, expect, it } from "vitest"; +import { healthOf } from "./index"; + +const HEALTHY: GitStatus = { + hasCommits: true, + identity: true, + installed: true, + version: "2.43.0", + workTree: true, +}; + +describe("healthOf", () => { + it("is happy with a working repository", () => { + expect(healthOf(HEALTHY)).toEqual({ ok: true }); + }); + + it("stays happy in a repository with no commits yet", () => { + // The regression this exists for. `git commit` works here; it is the diff + // baseline that does not, and that gates Revert through `noBaseline`. + expect( + healthOf({ + ...HEALTHY, + error: "this repository has no commits yet", + hasCommits: false, + }) + ).toEqual({ ok: true }); + }); + + it("carries the reason and the fix when git is missing", () => { + expect( + healthOf({ + error: "git is not installed or not on PATH", + hasCommits: false, + hint: "Install Git and make sure `git` is on PATH.", + identity: false, + installed: false, + workTree: false, + }) + ).toEqual({ + hint: "Install Git and make sure `git` is on PATH.", + ok: false, + reason: "git is not installed or not on PATH", + }); + }); + + it("carries the reason when the directory is not a work tree", () => { + const health = healthOf({ + ...HEALTHY, + error: "this is a bare git repository", + hasCommits: false, + workTree: false, + }); + expect(health.ok).toBe(false); + expect(health.reason).toBe("this is a bare git repository"); + }); + + it("keeps the reason inside the tooltip's one-line budget", () => { + // It is rendered as a `data-tip`, where the house rule is 44 characters + // and `tooltip.copy.test.ts` in the overlay enforces it for literals. + for (const status of [ + { ...HEALTHY, error: "not a git repository", workTree: false }, + { ...HEALTHY, error: "this is a bare git repository", workTree: false }, + { + ...HEALTHY, + error: "git is not installed or not on PATH", + installed: false, + }, + ]) { + const { reason } = healthOf(status); + expect((reason ?? "").length).toBeLessThanOrEqual(44); + expect(reason ?? "").not.toContain("—"); + } + }); +}); diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index cce73bf..7ca50c3 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -21,9 +21,10 @@ import { createPr, currentBranch, defaultBranch, + type GitStatus, ghStatus, + gitStatus, hasRemote, - isGitRepo, pushBranch, restoreFiles, } from "@airship/git"; @@ -36,6 +37,7 @@ import { type CreateJobRequest, type Effort, type ElementContext, + type GitHealth, type JobDiffBundle, type JobStatus, type ModelCatalogue, @@ -63,7 +65,8 @@ export type { } from "@airship/core"; /** Re-exported so the CLI depends only on @airship/server. */ export { checkAuth, listModels } from "@airship/core"; -export { isGitRepo } from "@airship/git"; +export type { GitFailure, GitStatus } from "@airship/git"; +export { gitStatus, isGitRepo, onGitFailure } from "@airship/git"; export type { AgentKind, AirshipSurface, Effort } from "@airship/protocol"; const WS_PATH = "/__airship/ws"; @@ -202,6 +205,19 @@ export async function startServer(opts: ServerOptions): Promise { } } + /** + * Can the overlay offer its git verbs at all? + * + * Uncached on purpose. It is a handful of `rev-parse` calls, it runs on + * connect and after a turn rather than per edit, and a repo can gain its + * first commit — or have its git uninstalled — while a tab stays open. A + * stale "yes" here is a click that fails for a reason the user was already + * told about and can no longer see. + */ + function gitHealth(): GitHealth { + return healthOf(gitStatus(cwd)); + } + wss.on("connection", (ws: WebSocket) => { clients.add(ws); send(ws, { @@ -209,6 +225,11 @@ export async function startServer(opts: ServerOptions): Promise { jobs: jobs.snapshots(), type: "hello", }); + // Beside the handshake rather than deferred: the transcript can paint a + // finished turn's action menu immediately, and a menu that offers Commit + // for half a second before greying it out is worse than one that never + // offered it. + send(ws, { health: gitHealth(), type: "git:health" }); // Push the token scan without being asked. The inspector needs it to render // its very first selection, and a request/response round trip would leave // the badges missing for the first element the user clicks. Deferred off the @@ -417,19 +438,23 @@ export async function startServer(opts: ServerOptions): Promise { status, type: "job:status", }); + // Before `job:done`, not after. That event is what paints the finished + // turn, and the turn menu captures the health it was rendered with — so + // sending it afterwards would leave the newest turn a beat behind. + broadcast({ health: gitHealth(), type: "git:health" }); broadcast({ bundle, jobId: rec.jobId, type: "job:done" }); broadcast({ entries: listHistory(cwd), type: "history" }); if (opts.autoCommit && status === "done" && result.diffs.length) { - const sha = commitEdit( + const auto = commitEdit( cwd, result.diffs.map((d) => d.file), bundle.summary || displayPrompt ); broadcast({ - error: sha ? undefined : "auto-commit failed", - ok: Boolean(sha), - sha: sha ?? undefined, + error: auto.ok ? undefined : `auto-commit failed: ${auto.error}`, + ok: auto.ok, + sha: auto.sha, type: "commit:result", }); } @@ -468,12 +493,15 @@ export async function startServer(opts: ServerOptions): Promise { } // Content-restore from the before-state captured by the SDK PreToolUse // hooks — instant and reliable. (SDK `rewindEdit` is the alternative.) - const restored = restoreFiles(cwd, bundle.diffs); - const ok = restored.length === bundle.diffs.length; + const { restored, skipped } = restoreFiles(cwd, bundle.diffs); + const ok = skipped.length === 0; broadcast({ + // The count alone was the whole message, which told the user a number and + // nothing they could act on. The first reason is the useful half; the + // rest are almost always the same one. error: ok ? undefined - : `restored ${restored.length}/${bundle.diffs.length} files`, + : `restored ${restored.length}/${bundle.diffs.length} files — ${skipped[0].file}: ${skipped[0].reason}`, jobId, ok, type: "undo:result", @@ -496,18 +524,13 @@ export async function startServer(opts: ServerOptions): Promise { }); return; } - const sha = commitEdit( + const { error, ok, sha } = commitEdit( cwd, bundle.diffs.map((d) => d.file), message || bundle.summary || bundle.prompt ); - if (!(sha && push)) { - send(ws, { - error: sha ? undefined : "commit failed", - ok: Boolean(sha), - sha: sha ?? undefined, - type: "commit:result", - }); + if (!(ok && push)) { + send(ws, { error, ok, sha, type: "commit:result" }); return; } const branch = currentBranch(cwd); @@ -521,6 +544,18 @@ export async function startServer(opts: ServerOptions): Promise { }); return; } + // Checked here as well as in `prPreflight`: pushing to a remote that does + // not exist otherwise spends the network timeout before saying so. + if (!hasRemote(cwd)) { + send(ws, { + error: "committed, but there is no `origin` remote to push to", + ok: true, + pushed: false, + sha, + type: "commit:result", + }); + return; + } const pushed = await pushBranch(cwd, branch); send(ws, { error: pushed.ok @@ -570,13 +605,13 @@ export async function startServer(opts: ServerOptions): Promise { const { head } = branched; const summary = bundle.summary || bundle.prompt; - const sha = commitEdit( + const committed = commitEdit( cwd, bundle.diffs.map((d) => d.file), summary ); - if (!sha) { - fail("commit", "commit failed"); + if (!committed.ok) { + fail("commit", committed.error ?? "commit failed"); return; } const pushed = await pushBranch(cwd, head); @@ -666,6 +701,29 @@ export async function startServer(opts: ServerOptions): Promise { }; } +/** + * Which git problems stop the overlay's git verbs, and which do not. + * + * The verbs are commit, push and open a pull request. Deliberately not gated on + * `hasCommits`: the first commit in a fresh repository is exactly the thing + * that works there, so greying Commit out for want of a HEAD would block the + * action that creates one. Having no HEAD costs the diff *baseline*, which is a + * per-turn fact carried on `FileDiff.noBaseline` and gates Revert instead. + * + * Exported for its own test. `gitStatus` short-circuits in the order these are + * read, so `error` always describes the field that actually failed here. + */ +export function healthOf(status: GitStatus): GitHealth { + if (status.installed && status.workTree) { + return { ok: true }; + } + return { + hint: status.hint, + ok: false, + reason: status.error ?? "git is unavailable", + }; +} + function preview(prompt: string): string { return prompt.length > 120 ? `${prompt.slice(0, 117)}…` : prompt; } @@ -676,13 +734,19 @@ function prPreflight(cwd: string, bundle: JobDiffBundle | null): string | null { if (!bundle?.diffs.length) { return "nothing to open a pull request for"; } - if (!isGitRepo(cwd)) { - return "not a git repository"; + // `gitStatus` rather than `isGitRepo`, because this message is the one the + // user acts on: a git that is not installed, a bare repo and a directory that + // is not a repository all used to arrive here as "not a git repository". + const git: GitStatus = gitStatus(cwd); + if (!(git.workTree && git.hasCommits)) { + return git.error ?? "git is unavailable"; } if (!hasRemote(cwd)) { return "no `origin` remote"; } - const gh = ghStatus(); + // Scoped to the repo: `gh auth status` resolves which host to check from the + // remote, so asking without a cwd can pass while the push still fails. + const gh = ghStatus(cwd); if (!gh.authed) { return gh.error ?? "gh is unavailable"; } @@ -707,8 +771,12 @@ function ensureFeatureBranch( return { head }; } const name = requested || `airship/${jobId.slice(0, 8)}`; - if (!createBranch(cwd, name)) { - return { error: `could not create branch ${name}`, stage: "branch" }; + const branched = createBranch(cwd, name); + if (!branched.ok) { + return { + error: `could not create branch ${name}: ${branched.error}`, + stage: "branch", + }; } return { head: name }; } diff --git a/packages/server/src/server.integration.test.ts b/packages/server/src/server.integration.test.ts index 1e16ce3..3783f7f 100644 --- a/packages/server/src/server.integration.test.ts +++ b/packages/server/src/server.integration.test.ts @@ -174,6 +174,17 @@ describe("the editor server, over raw sockets", () => { expect(reply.startsWith("HTTP/1.1 101 ")).toBe(true); expect(reply).toContain('"type":"hello"'); }); + + it("reports whether git works, beside the handshake", async () => { + // Beside `hello` rather than deferred: the transcript paints a finished + // turn's action menu immediately, and a menu that offers Commit for half a + // second before greying it out is worse than one that never offered it. + const reply = await exchange( + port, + upgradeRequest("/__airship/ws", { host: `localhost:${port}` }) + ); + expect(reply).toContain('"type":"git:health"'); + }); }); describe("the Host gate", () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2a06f04..44e7849 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -210,6 +210,9 @@ importers: '@types/node': specifier: ^26.2.0 version: 26.2.0 + vitest: + specifier: ^4.1.10 + version: 4.1.10(@types/node@26.2.0)(@vitest/browser-playwright@4.1.10)(happy-dom@20.11.2)(vite@8.2.1(@types/node@26.2.0)(esbuild@0.28.2)(jiti@2.7.0)(yaml@2.9.0)) packages/overlay: dependencies: