From 5b5dd4327da7de35e0807036fb8059bb4b27a9e9 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 15:52:41 +0200 Subject: [PATCH 1/5] feat(changes): list every untracked file and give it a real diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git status ran with git's default --untracked-files=normal, so a brand-new directory arrived as a single row whose path was a directory: unopenable, and hiding every file under it. -uall lists them one by one. An untracked row also had no diff and no counts — git diff --numstat never reports an untracked file. Clicking one now runs git diff --no-index against /dev/null, which yields the new-file diff and, from that same output, the added-line count the status pass cannot know. The count lands on the row and in the header total at no extra process cost; computing it up front would mean one git invocation per untracked file on every refresh, and one ssh round-trip each on a remote session. --no-index offers neither of the guarantees the pathspec path relies on: it exits 1 whenever the two inputs differ (success here, not failure), and it has no repository-boundary check, so its operand gets a guard that also rejects absolute paths and a leading dash. The real-git test file now pins LC_ALL=C — its assertions match git's English diagnostics, which a localized machine does not produce. --- .ai/contexts/changes-view.md | 110 ++++++++++++++- .ai/contexts/ipc-bridge.md | 6 +- docs/changes-view.md | 14 +- git-changes-runner.js | 36 ++++- git-changes.js | 18 ++- main.js | 6 +- preload.js | 2 +- public/file-panel.js | 33 +++-- test/dom-file-panel-changes.test.js | 100 ++++++++++++-- test/git-changes-runner-real-git.test.js | Bin 4710 -> 10469 bytes test/git-changes-runner.test.js | 163 +++++++++++++++++++++++ test/git-changes.test.js | 63 ++++++++- 12 files changed, 516 insertions(+), 35 deletions(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 56b6d781..24b9afc6 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -21,7 +21,8 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). - `parseStatusPorcelainV2(text)` → `{branch:{head,upstream,ahead,behind}, files:[{path,origPath,staged,unstaged,untracked,renamed,state}]}`. Record types `1` (ordinary), `2` (rename/copy — `origPath` and `renamed:true`), `u` (unmerged), `?` (untracked, `state:'?'`). Type `!` (ignored) and any future/unrecognized record type are dropped rather than thrown on. - `parseNumstat(text)` → `{[path]: {added, deleted}}`. Binary files report `-` in git's own output; that becomes `null` here, not `0`, so a caller can tell "no lines changed" apart from "line count unknown". -- `mergeChanges(status, numstatStaged, numstatUnstaged)` → the panel's model: each file gets `added`/`deleted` summed across whichever of the two numstat maps have an entry for it (a file modified in both the index and the worktree has two independent diffs; a file already staged and now edited again is a real, common case, not an edge case). An untracked file's counts stay `null` — `git diff` never reports untracked files at all. `totals` sums only the known (non-null) counts. +- `mergeChanges(status, numstatStaged, numstatUnstaged)` → the panel's model: each file gets `added`/`deleted` summed across whichever of the two numstat maps have an entry for it (a file modified in both the index and the worktree has two independent diffs; a file already staged and now edited again is a real, common case, not an edge case). An untracked file's counts stay `null` at this stage — `git diff --numstat` never reports untracked files at all, and status makes no per-file call to find out (see "Untracked files" below). `totals` sums only the known (non-null) counts. +- `countNewFileDiffAdditions(text)` → the added-line count of a new-file unified diff, or `null` when the diff is binary (`Binary files … differ`). It counts only lines *after* the first `@@` hunk header, so a file whose own content starts with `+++ ` or `@@ ` is counted like any other line — a plain "starts with `+` but not `+++`" test miscounts exactly there. - **Both parsers consume `-z` (NUL-separated) output — see "Quoting rule" below.** They walk an explicit index into `String(text).split('\0')` rather than a plain `for...of` over lines, because a rename/copy record spans TWO tokens instead of one: - **Status** (`2 ... \0\0`): the origPath is the very next token — no tab embedded in the first one the way non-`-z` porcelain v2 does it. - **Numstat** (`\t\t\0\0\0`): an EMPTY path field (immediately followed by NUL) signals a rename; the actual paths are the next two tokens, old then new — never the `old => new` / `dir/{old => new}/suffix` arrow spellings numstat emits without `-z`. @@ -29,7 +30,7 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). ## Runner interface (`git-changes-runner.js`) -`createGitChangesRunner({kind, cwd, alias, exec, timeoutMs})` → `{status(), diff(path, {staged})}`. `status()` runs three commands in parallel (`git status --porcelain=v2 --branch -z`, `git diff --numstat -z`, `git diff --cached --numstat -z`) and merges them. `diff()` runs `git diff [--cached] -- `, capped at 512 KB (`MAX_DIFF_BYTES`) measured in UTF-8 bytes and cut on a line boundary, with a `truncated` flag. +`createGitChangesRunner({kind, cwd, alias, exec, timeoutMs})` → `{status(), diff(path, {staged, untracked})}`. `status()` runs three commands in parallel (`git status --porcelain=v2 --branch -uall -z`, `git diff --numstat -z`, `git diff --cached --numstat -z`) and merges them. `diff()` runs `git diff [--cached] -- ` — or, with `untracked: true`, `git diff --no-index -- /dev/null ` (see "Untracked files") — capped at 512 KB (`MAX_DIFF_BYTES`) measured in UTF-8 bytes and cut on a line boundary, with a `truncated` flag. - **Local** (`kind: 'local'`): `child_process.execFile('git', args, {cwd, timeout, maxBuffer})` — cwd is `execFile`'s own option, never a `-C` argument. No shell is invoked, so argument content cannot be interpreted as a command regardless of what it contains; timeout 10s. - **Remote** (`kind: 'remote'`): the same ssh transport `remote-attach.js` already uses for the tmux probe/restore calls (`buildRemoteCommandArgs`, `defaultRunRemoteCommand`) — `ssh -o BatchMode=yes -o ConnectTimeout=5 -n "git -C '' '--literal-pathspecs' 'diff' '--' '' ..."`. Timeout 20s. This command string DOES run through a shell on the far end. @@ -45,12 +46,117 @@ Four independent defenses, added after an adversarial review of the first cut of 4. **A capped stdout on the remote transport** (`defaultRunRemoteCommand`'s `maxStdoutBytes`, default 8 MB) — see "Remote transport stdout cap" below. Independent of the pathspec-safety points above; this one bounds memory/time on a huge or runaway response instead of trusting stderr's existing 4096-byte cap to also apply to stdout (it never did). 5. **`shQuote()`** (unchanged from the original design) — standard POSIX single-quote escaping (close the quote, insert a literal quote via `'\''`, reopen) around `cwd` and every arg before they're interpolated into the remote command string. This is what actually makes the remote command injection-safe: a correctly single-quoted string cannot be broken out of by any byte sequence except an embedded NUL, and NUL can't appear in a shell token or a JS string used as one to begin with. +One operand does not get this treatment: the filesystem path handed to +`git diff --no-index` for an untracked file, guarded by `isSafeNoIndexPath` — +strictly narrower, because `--no-index` drops git's own repository-boundary +check. See "Untracked files" below. + Given (5), `isSafeShellArg`/`isSafeCwd`/`isSafeGitPath` stay a **denylist** (control characters, any `..` segment, and now a leading `:`) rather than a positive character allowlist. Git paths and cwds legitimately contain almost any byte — spaces, unicode, punctuation, even a literal backtick or `$` in a filename — and an allowlist narrow enough to catch every shell metacharacter would also reject a lot of real filenames for no safety gain, since the metacharacters are already neutralized by the quoting, not by the character check. This mirrors the `open-terminal` `preLaunchCmd` guard's own documented lesson (`.ai/contexts/ipc-bridge.md`, "IPC path-guard inventory"): a denylist proved incomplete there because that string is deliberately raw shell; here the string is never raw shell in the first place, so closing by quoting is available and preferred over closing by enumeration. `buildRemoteGitCommand`/`shQuote` never emit a backtick for any input, proven in `test/git-changes-runner.test.js` including adversarial cwd/path values containing backtick, `$(...)`, and an embedded single quote. **Measured, not assumed** (`test/git-changes-runner-real-git.test.js`, a real `git init`-ed temp repo, no injected `exec`): an absolute pathspec that resolves outside the repository is refused by git itself (`fatal: ... is outside repository`, exit 128, empty stdout) — with or without `--literal-pathspecs` — so no code here needs its own absolute-path rejection on top of that. An absolute pathspec *inside* the repo still works normally. A `~`-prefixed pathspec is never shell-expanded (no shell in the local path; a shell exists on the remote path but the value sits inside single quotes, and tilde expansion does not apply inside single quotes either way) — it just resolves to a literal, almost-certainly-nonexistent relative path. +### Untracked files + +An untracked file is a first-class row: one row per file, a real diff on click, +and line counts that reach the header total. Four decisions hold that up, each +measured against real git (git 2.53, `test/git-changes-runner-real-git.test.js`). + +**1. `status` runs with `-uall`.** Git's default `--untracked-files=normal` +reports a wholly-untracked directory as a single entry (`? newdir/`) and never +descends, so a brand-new directory collapsed to one row whose `path` was a +directory — unopenable, uncountable. `-uall` lists every file individually +(`? newdir/a.txt`, `? newdir/sub/b.txt`), which is what makes "no row's `path` +is ever a directory" an invariant rather than a hope. The cost is a longer +status output on a repo with a large unignored tree; it is bounded by the +existing `STATUS_MAX_STDOUT_BYTES` (2 MB) cap on the remote transport, and by +`execFile`'s `maxBuffer` locally — a repo that overruns it surfaces the cap +message instead of a wrong answer. + +**2. The empty side of the diff is the literal string `/dev/null`.** +`git diff --no-index -- /dev/null ` produces exactly the new-file diff +the panel wants, with no index mutation. The portability question — this repo is +also checked out on Windows, where `/dev/null` does not exist — resolves in +git's favour: git does not `stat()` that operand, it compares the string. +Measured: `git diff --no-index -- /dev/null /dev/zero` fails with +`unsupported file type` (git *did* stat `/dev/zero`, an existing character +device), while `/dev/null` on the same side succeeds; and `nul`, git's Windows +spelling for the same thing, is refused on Linux. Both observations match git's +own `diff-no-index.c`, where the `/dev/null` string is special-cased +unconditionally and `nul` only under `GIT_WINDOWS_NATIVE`. So `/dev/null` is +the portable spelling, and the two alternatives are worse: creating an empty +temp file means writing into a repository under test (and cleaning it up on +every error path, remote included), and `git add -N` mutates the index of a +repository the user is actively working in, which a read-only viewer must never +do. + +**3. Exit code 1 is success here.** `git diff --no-index` exits 1 whenever the +two inputs differ — i.e. on every successful untracked diff. The shared +`result.code !== 0` check that every other command in this file uses would turn +every untracked row into a red error row, so this call has its own rule: +**0 and 1 are both success, anything else is an error**. Exit 1 is also how +`--no-index` reports an inaccessible operand (`error: Could not access 'x'`, +exit 1, empty stdout) — distinguished by the second half of the rule: empty +stdout plus a message on stderr is an error. A successful `--no-index` against +`/dev/null` always writes at least a `diff --git`/`new file mode` header, even +for an empty file. + +**4. A `--no-index` operand is a filesystem path, and gets a stricter guard +than a pathspec** (`isSafeNoIndexPath`, not `isSafeGitPath`). The pathspec guard +can afford to be a denylist because git itself enforces the repository boundary: +an absolute pathspec outside the repo is refused with `fatal: … is outside +repository` (measured; see below). **`--no-index` has no such containment check +at all** — measured: raw `git diff --no-index -- /dev/null /tmp/…/outside-secret.txt` +from inside the repo prints the file. Nothing downstream would stop it, so the +guard is what keeps an untracked diff inside the working directory: +`isSafeNoIndexPath` = `isSafeGitPath` (control characters, any `..`, leading +`:`) **plus** no absolute path (leading `/`, leading `\`, `X:` drive prefix) and +no leading `-` (the operand sits where git parses options, and `--` separation +is belt-and-braces, not the only defense). A repo-root-level file whose name +starts with `-` is therefore not diffable from the panel — an accepted, narrow +loss against an operand that could otherwise be read as a git option. + +Nothing changes for the remote transport: the untracked call goes through the +same `invoke()` → `buildRemoteGitCommand`/`shQuote` path as every other command, +so `/dev/null` and the path are each their own single-quoted token and the +"never emits a backtick outside a quoted token" property is unchanged +(asserted in `test/git-changes-runner.test.js` with a path containing a +backtick and `$(…)`). + +#### Why the counts arrive on click, not with status + +`git diff --numstat` genuinely never reports an untracked file, and there is no +single git invocation that yields line counts for *all* untracked files: +`--no-index` takes exactly two operands, and pointing it at a directory does not +help (measured: `git diff --no-index -- /dev/null ` errors with +`Could not access '/null'` — git pairs the operands by basename rather than +walking the tree). The options were therefore one invocation per untracked file +during `status()` — unacceptable on a repo with hundreds of untracked files, +and multiplied by an ssh round-trip on a remote session — or no counts at all. + +Neither is needed, because the click already fetches the whole diff: the runner +counts additions from the stdout it has just read (`countNewFileDiffAdditions`), +at zero extra process cost, and returns `{added, deleted: 0}` alongside the +content. The renderer writes them onto that file's record in the open tab and +re-derives the header totals (`applyUntrackedCounts` in `public/file-panel.js`), +so an untracked row looks exactly like a tracked one from the moment its diff +has been opened once, and the totals grow as rows are visited. A refresh +re-reads status and the counts go back to unknown — correct, since the file may +have changed. Counts are deliberately `null`, never `0`, for a binary file and +for a diff truncated at the 512 KB cap: both are "unknown", and `mergeChanges`'s +totals only sum known counts. Counting locally from the filesystem was rejected +for the same reason the whole runner exists — it would not work for a remote +session, and local and remote must not disagree about what the panel shows. + +A `--no-index` diff's file-header lines are `--- /dev/null` and `+++ b/` +(not the `a/ b/` pair a tracked diff carries). `classifyDiffLine()` +keys on the `---`/`+++`/`@@`/`+`/`-` prefixes only, so both land on +`changes-diff-file-header` exactly as they do for a tracked diff — no renderer +change was needed, and `test/dom-file-panel-changes.test.js` pins it so a future +"classify by `a/`…`b/` pair" refactor cannot silently render `--- /dev/null` as +a deleted line. + ### Remote transport stdout cap (`remote-attach.js` `defaultRunRemoteCommand`) `defaultRunRemoteCommand(alias, command, {timeoutMs, maxStdoutBytes, spawnFn})` counts accumulated stdout in UTF-8 bytes as each chunk arrives (`Buffer.byteLength`, works for both a real Buffer chunk and a test's plain-string chunk). Crossing `maxStdoutBytes` (default `DEFAULT_MAX_STDOUT_BYTES` = 8 MB when the caller doesn't pass one) SIGKILLs the child and resolves `{code: -1, stdout: '', stderr: 'stdout exceeded bytes'}` — the same `{code, stdout, stderr}` shape every other path already returns, so `git-changes-runner.js`'s existing `firstError()`/`ok:false` handling surfaces it as `{ok: false, error: 'stdout exceeded bytes'}` with no special-casing. `git-changes-runner.js` passes an explicit cap on every call — `STATUS_MAX_STDOUT_BYTES` (2 MB) for each of the three `status()` commands, `DIFF_MAX_STDOUT_BYTES` (`MAX_DIFF_BYTES` + 64 KB slack) for `diff()`, so a diff just over the panel's own display cap still arrives whole and gets truncated locally instead of being killed by the transport first. The tmux probe/restore calls in `remote-attach.js` and `remote-stop.js`'s kill command never pass `maxStdoutBytes` and fall back to the 8 MB default — their own output is a handful of bytes, nowhere near either cap (verified: `test/remote-attach.test.js` and `test/remote-stop.test.js` pass unmodified). diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index fc25f504..ac0f418a 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -88,8 +88,8 @@ refresh triggers): `.ai/contexts/changes-view.md`. User-facing: `docs/changes-vi | IPC | Args | Returns | Notes | |---|---|---|---| -| `git-changes-status` | `(sessionId)` | `{ok, branch, files, totals} \| {ok:false, error}` | `git status --porcelain=v2 --branch` + `git diff --numstat` + `git diff --cached --numstat`, merged by `git-changes.js`'s `mergeChanges()`. | -| `git-changes-diff` | `(sessionId, filePath, staged)` | `{ok, content, truncated} \| {ok:false, error}` | `git diff [--cached] -- `, capped at 512 KB. | +| `git-changes-status` | `(sessionId)` | `{ok, branch, files, totals} \| {ok:false, error}` | `git status --porcelain=v2 --branch -uall` + `git diff --numstat` + `git diff --cached --numstat`, merged by `git-changes.js`'s `mergeChanges()`. | +| `git-changes-diff` | `(sessionId, filePath, staged, untracked)` | `{ok, content, truncated, added, deleted} \| {ok:false, error}` | `git diff [--cached] -- `, or `git diff --no-index -- /dev/null ` when `untracked`; capped at 512 KB. `added`/`deleted` are filled for an untracked file only — see `.ai/contexts/changes-view.md` ("Untracked files"). | ### Misc @@ -157,7 +157,7 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | `add-project` / `remap-project` | none on the probe (`fs.statSync`/`fs.existsSync`/`fs.lstatSync`); the actual write is confined through `encodeProjectPath` | existence/type oracle only — inherent to the feature (both accept an arbitrary disk location by design), not cheaply fixable without breaking it | | `open-terminal` (`preLaunchCmd`) | `validatePreLaunchCmd` (`pre-launch-cmd-guard.js`) | not a path guard — a character allowlist on a raw-shell-by-design string (the documented prefix's character set plus its analogues: `env VAR=val`, `doas`, an absolute binary path); a denylist here proved incomplete (process substitution `<(...)`/`>(...)` needed none of the blocked characters), so this is closed by construction instead of by enumeration. Known cost: bare `$VAR` expansion and quoted arguments, both previously accepted, are now refused | | `read-session-jsonl` / `read-subagent-jsonl` / `start-subagent-watch` / `create-schedule-session` | none directly — path is derived from a SQLite key or built via `encodeProjectPath`, not taken verbatim from the renderer | out of scope for a path guard; flag if a renderer-controlled string is ever found reaching the derivation unencoded | -| `git-changes-diff` | `isSafeGitPath` (`git-changes-runner.js`) | not a filesystem path — a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist | +| `git-changes-diff` | `isSafeGitPath`, or `isSafeNoIndexPath` when `untracked` (`git-changes-runner.js`) | not a filesystem path — a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist. The untracked variant is a real filesystem operand of `git diff --no-index`, which has no repository-boundary check of its own, so its guard additionally rejects absolute paths and a leading `-` — see "Untracked files" in the same doc | ### Non-obvious behaviors diff --git a/docs/changes-view.md b/docs/changes-view.md index 746267fa..2848f7e5 100644 --- a/docs/changes-view.md +++ b/docs/changes-view.md @@ -10,9 +10,21 @@ Click the **Changes** button in the terminal header, next to the stop button. Cl - A header line: `N files changed +A −B`, plus the current branch and how far it is ahead/behind its upstream. - One row per changed file: a state letter (`M` modified, `A` added, `D` deleted, `R`/`C` renamed/copied, `?` untracked), its path, and its own `+added −deleted` line counts. -- Clicking a row opens a read-only diff for that file. Untracked files show a note instead of a diff — `git diff` never reports them. +- Clicking a row opens a read-only diff for that file, including an untracked one — a brand-new file shows up as an all-additions diff. A binary file shows a one-line note instead of its bytes. +- A brand-new directory is listed file by file, not as a single folder row. - A **Refresh** button for a manual pull. +### Counts for new files + +Git reports line counts for tracked files only, so an untracked file's row +starts without any, and the header's `+A −B` does not include it yet. Click the +row once: its diff is fetched, the row gets its `+added −0`, and the header +total grows by the same amount. This is deliberate — counting every new file up +front would mean running one extra git command per untracked file on every +refresh (and one ssh round-trip each, for a remote session), which a repo with a +large untracked tree would feel. Refreshing resets them, since the files may +have changed since. + ## What it doesn't do - No staging, committing, or reverting from the UI — this is a viewer, not a git client. diff --git a/git-changes-runner.js b/git-changes-runner.js index 1b6f97af..bcf1855f 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -4,7 +4,7 @@ const { execFile } = require('child_process'); const { defaultRunRemoteCommand } = require('./remote-attach'); -const { parseStatusPorcelainV2, parseNumstat, mergeChanges } = require('./git-changes'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions } = require('./git-changes'); const DEFAULT_LOCAL_TIMEOUT_MS = 10_000; const DEFAULT_REMOTE_TIMEOUT_MS = 20_000; @@ -32,6 +32,17 @@ function isSafeGitPath(p) { return true; } +// `git diff --no-index` operands are filesystem paths, not pathspecs — see .ai/contexts/changes-view.md ("Untracked files") +const NO_INDEX_EMPTY_SIDE = '/dev/null'; + +function isSafeNoIndexPath(p) { + if (!isSafeGitPath(p)) return false; + if (p[0] === '-') return false; + if (p[0] === '/' || p[0] === '\\') return false; + if (/^[A-Za-z]:/.test(p)) return false; + return true; +} + // --literal-pathspecs on every invocation — see .ai/contexts/changes-view.md ("Quoting rule"). function buildGitArgs(args) { return ['--literal-pathspecs', ...args]; @@ -119,7 +130,7 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { let results; try { results = await Promise.all([ - invoke(['status', '--porcelain=v2', '--branch', '-z'], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }), + invoke(['status', '--porcelain=v2', '--branch', '-uall', '-z'], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }), invoke(['diff', '--numstat', '-z'], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }), invoke(['diff', '--cached', '--numstat', '-z'], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }), ]); @@ -137,7 +148,27 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { return { ok: true, ...mergeChanges(parsedStatus, numstatStaged, numstatUnstaged) }; } + // `--no-index` exits 1 on a difference — see .ai/contexts/changes-view.md ("Untracked files") + async function untrackedDiff(path) { + if (!isSafeNoIndexPath(path)) return { ok: false, error: 'invalid path' }; + + let result; + try { + result = await invoke(['diff', '--no-index', '--', NO_INDEX_EMPTY_SIDE, path], { maxStdoutBytes: DIFF_MAX_STDOUT_BYTES }); + } catch (err) { + return { ok: false, error: err.message }; + } + if (result.code !== 0 && result.code !== 1) return { ok: false, error: firstError(result) }; + const stdout = result.stdout || ''; + if (!stdout && (result.stderr || '').trim()) return { ok: false, error: firstError(result) }; + + const { content, truncated } = truncateDiffContent(stdout, MAX_DIFF_BYTES); + const added = truncated ? null : countNewFileDiffAdditions(content); + return { ok: true, content, truncated, added, deleted: added === null ? null : 0 }; + } + async function diff(path, opts = {}) { + if (opts.untracked) return untrackedDiff(path); if (!isSafeGitPath(path)) return { ok: false, error: 'invalid path' }; const staged = !!opts.staged; const args = staged ? ['diff', '--cached', '--', path] : ['diff', '--', path]; @@ -165,6 +196,7 @@ module.exports = { shQuote, isSafeCwd, isSafeGitPath, + isSafeNoIndexPath, MAX_DIFF_BYTES, STATUS_MAX_STDOUT_BYTES, DIFF_MAX_STDOUT_BYTES, diff --git a/git-changes.js b/git-changes.js index e753a8e6..d40ce201 100644 --- a/git-changes.js +++ b/git-changes.js @@ -112,6 +112,22 @@ function parseNumstat(text) { return result; } +// Count additions in a new-file unified diff — see .ai/contexts/changes-view.md ("Untracked files") +function countNewFileDiffAdditions(text) { + const content = String(text || ''); + if (/^Binary files /m.test(content)) return null; + let inHunk = false; + let added = 0; + for (const line of content.split('\n')) { + if (!inHunk) { + if (line.startsWith('@@')) inHunk = true; + continue; + } + if (line.startsWith('+')) added += 1; + } + return added; +} + function combineCounts(a, b) { if ((a && a.added === null) || (b && b.added === null)) return { added: null, deleted: null }; const added = (a ? a.added || 0 : 0) + (b ? b.added || 0 : 0); @@ -143,4 +159,4 @@ function mergeChanges(status, numstatStaged, numstatUnstaged) { }; } -module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges }; +module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions }; diff --git a/main.js b/main.js index 987e361a..303c8e18 100644 --- a/main.js +++ b/main.js @@ -1759,13 +1759,13 @@ ipcMain.handle('git-changes-status', async (_event, sessionId) => { } }); -// filePath is a git pathspec, not a filesystem path — see .ai/contexts/changes-view.md -ipcMain.handle('git-changes-diff', async (_event, sessionId, filePath, staged) => { +// filePath is a git pathspec, or an untracked file's --no-index operand — see .ai/contexts/changes-view.md +ipcMain.handle('git-changes-diff', async (_event, sessionId, filePath, staged, untracked) => { if (typeof filePath !== 'string' || !filePath) return { ok: false, error: 'invalid path' }; const target = resolveGitChangesTarget(sessionId); if (!target.ok) return target; try { - return await gitChangesRunnerFor(target).diff(filePath, { staged: !!staged }); + return await gitChangesRunnerFor(target).diff(filePath, { staged: !!staged, untracked: !!untracked }); } catch (err) { return { ok: false, error: err.message }; } diff --git a/preload.js b/preload.js index 1a1b9df7..03a9ea8e 100644 --- a/preload.js +++ b/preload.js @@ -35,7 +35,7 @@ contextBridge.exposeInMainWorld('api', { stopSubagentWatch: (watchId) => ipcRenderer.invoke('stop-subagent-watch', watchId), // see .ai/contexts/changes-view.md gitChangesStatus: (sessionId) => ipcRenderer.invoke('git-changes-status', sessionId), - gitChangesDiff: (sessionId, filePath, staged) => ipcRenderer.invoke('git-changes-diff', sessionId, filePath, staged), + gitChangesDiff: (sessionId, filePath, staged, untracked) => ipcRenderer.invoke('git-changes-diff', sessionId, filePath, staged, untracked), // Settings getSetting: (key) => ipcRenderer.invoke('get-setting', key), diff --git a/public/file-panel.js b/public/file-panel.js index 9e445926..e3c9f2fb 100644 --- a/public/file-panel.js +++ b/public/file-panel.js @@ -616,7 +616,6 @@ function openChangesTab(sessionId) { diffError: null, diffContent: null, diffTruncated: false, - diffUntracked: false, }; state.panelVisible = true; @@ -661,19 +660,11 @@ async function openChangesDiff(sessionId, file) { tab.diffError = null; tab.diffContent = null; tab.diffTruncated = false; - tab.diffUntracked = !!file.untracked; - - if (file.untracked) { - // git diff never reports an untracked file — nothing to fetch. - tab.diffLoading = false; - if (currentPanelSessionId === sessionId) renderPanel(sessionId); - return; - } tab.diffLoading = true; if (currentPanelSessionId === sessionId) renderPanel(sessionId); - const result = await window.api.gitChangesDiff(sessionId, file.path, file.staged); + const result = await window.api.gitChangesDiff(sessionId, file.path, file.staged, file.untracked); const stillState = filePanelState.get(sessionId); if (!stillState || stillState.currentTab !== tab || tab.selectedFile !== file) return; @@ -684,10 +675,30 @@ async function openChangesDiff(sessionId, file) { } else { tab.diffContent = result.content; tab.diffTruncated = !!result.truncated; + if (file.untracked) applyUntrackedCounts(tab, file.path, result.added, result.deleted); } if (currentPanelSessionId === sessionId) renderPanel(sessionId); } +// Untracked counts arrive with the diff, not with status — see .ai/contexts/changes-view.md +function applyUntrackedCounts(tab, filePath, added, deleted) { + if (typeof added !== 'number') return; + if (!tab.data || !Array.isArray(tab.data.files)) return; + const record = tab.data.files.find((f) => f.path === filePath); + if (!record) return; + + record.added = added; + record.deleted = typeof deleted === 'number' ? deleted : 0; + + let totalAdded = 0; + let totalDeleted = 0; + for (const f of tab.data.files) { + if (typeof f.added === 'number') totalAdded += f.added; + if (typeof f.deleted === 'number') totalDeleted += f.deleted; + } + tab.data.totals = { ...tab.data.totals, added: totalAdded, deleted: totalDeleted }; +} + function closeChangesDiff(sessionId) { const state = filePanelState.get(sessionId); if (!state || !state.currentTab || state.currentTab.type !== 'changes') return; @@ -820,8 +831,6 @@ function renderChangesDiff(sessionId, tab) { } else if (tab.diffError) { body.textContent = tab.diffError; body.classList.add('changes-error'); - } else if (tab.diffUntracked) { - body.textContent = 'Untracked file — nothing to diff yet.'; } else if (!tab.diffContent) { body.textContent = 'No differences.'; } else { diff --git a/test/dom-file-panel-changes.test.js b/test/dom-file-panel-changes.test.js index ec27376e..3e9552b1 100644 --- a/test/dom-file-panel-changes.test.js +++ b/test/dom-file-panel-changes.test.js @@ -65,9 +65,9 @@ function setupFilePanelDom({ statusImpl, diffImpl } = {}) { calls.status.push(sessionId); return Promise.resolve((statusImpl || (() => makeStatusResult()))(sessionId)); }, - gitChangesDiff: (sessionId, filePath, staged) => { - calls.diff.push({ sessionId, filePath, staged }); - return Promise.resolve((diffImpl || (() => ({ ok: true, content: '@@ -1 +1 @@\n-old\n+new\n context\n', truncated: false })))(sessionId, filePath, staged)); + gitChangesDiff: (sessionId, filePath, staged, untracked) => { + calls.diff.push({ sessionId, filePath, staged, untracked }); + return Promise.resolve((diffImpl || (() => ({ ok: true, content: '@@ -1 +1 @@\n-old\n+new\n context\n', truncated: false })))(sessionId, filePath, staged, untracked)); }, }; @@ -153,7 +153,7 @@ test('clicking a file row opens a read-only diff colored by line prefix', async await flush(); assert.equal(ctx.calls.diff.length, 1); - assert.deepEqual(ctx.calls.diff[0], { sessionId: 's1', filePath: 'src/a.js', staged: true }); + assert.deepEqual(ctx.calls.diff[0], { sessionId: 's1', filePath: 'src/a.js', staged: true, untracked: false }); const addLine = ctx.document.querySelector('.changes-diff-add'); const delLine = ctx.document.querySelector('.changes-diff-del'); @@ -170,8 +170,16 @@ test('clicking a file row opens a read-only diff colored by line prefix', async } finally { ctx.destroy(); } }); -test('clicking an untracked file shows a note instead of calling gitChangesDiff', async () => { - const ctx = setupFilePanelDom(); +const UNTRACKED_DIFF_RESULT = { + ok: true, + content: 'diff --git a/new.txt b/new.txt\nnew file mode 100644\n--- /dev/null\n+++ b/new.txt\n@@ -0,0 +1,2 @@\n+first\n+second\n', + truncated: false, + added: 2, + deleted: 0, +}; + +test('clicking an untracked file fetches its diff like any other row, flagged untracked (mutation target: the old short-circuit)', async () => { + const ctx = setupFilePanelDom({ diffImpl: () => UNTRACKED_DIFF_RESULT }); try { ctx.window.switchPanel('s1'); await ctx.window.openChangesTab('s1'); @@ -181,9 +189,85 @@ test('clicking an untracked file shows a note instead of calling gitChangesDiff' row.dispatchEvent(new ctx.window.Event('click', { bubbles: true })); await flush(); - assert.equal(ctx.calls.diff.length, 0, 'an untracked file has no git diff to fetch'); + assert.equal(ctx.calls.diff.length, 1, 'an untracked file is fetched, not short-circuited'); + assert.deepEqual(ctx.calls.diff[0], { sessionId: 's1', filePath: 'new.txt', staged: false, untracked: true }); + + const body = ctx.document.querySelector('.changes-diff-body'); + assert.ok(!/nothing to diff/.test(body.textContent), 'the old placeholder note is gone'); + const addLines = Array.from(ctx.document.querySelectorAll('.changes-diff-add')).map((el) => el.textContent); + assert.deepEqual(addLines, ['+first', '+second']); + const headerLines = Array.from(ctx.document.querySelectorAll('.changes-diff-file-header')).map((el) => el.textContent); + assert.deepEqual(headerLines, ['--- /dev/null', '+++ b/new.txt'], '--no-index header paths are classified as headers, not as a deletion and an addition'); + } finally { ctx.destroy(); } +}); + +test('an untracked file\'s counts and the header totals pick up the additions its diff reported', async () => { + const ctx = setupFilePanelDom({ diffImpl: () => UNTRACKED_DIFF_RESULT }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + const before = ctx.document.querySelector('.changes-file-row[data-path="new.txt"] .changes-file-counts'); + assert.equal(before, null, 'status alone cannot know an untracked file\'s line count'); + + ctx.document.querySelector('.changes-file-row[data-path="new.txt"]') + .dispatchEvent(new ctx.window.Event('click', { bubbles: true })); + await flush(); + + const backBtn = Array.from(ctx.document.querySelectorAll('#changes-diff-view button')).find(b => b.textContent === 'Back'); + backBtn.click(); + + const counts = ctx.document.querySelector('.changes-file-row[data-path="new.txt"] .changes-file-counts'); + assert.ok(counts, 'the row now renders counts like any other row'); + assert.equal(counts.textContent, '+2−0'); + + const summary = ctx.document.getElementById('changes-summary'); + assert.match(summary.textContent, /2 files changed \+5 −1/, 'the untracked additions (2) join the tracked ones (3) in the header total'); + assert.equal(ctx.calls.status.length, 1, 'no extra status fetch — the counts came with the diff'); + } finally { ctx.destroy(); } +}); + +test('an untracked binary file keeps null counts — the row stays countless and the totals do not move', async () => { + const binary = { ok: true, content: 'diff --git a/new.txt b/new.txt\nBinary files /dev/null and b/new.txt differ\n', truncated: false, added: null, deleted: null }; + const ctx = setupFilePanelDom({ diffImpl: () => binary }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + ctx.document.querySelector('.changes-file-row[data-path="new.txt"]') + .dispatchEvent(new ctx.window.Event('click', { bubbles: true })); + await flush(); + + const body = ctx.document.querySelector('.changes-diff-body'); + assert.match(body.textContent, /Binary files .* differ/); + + const backBtn = Array.from(ctx.document.querySelectorAll('#changes-diff-view button')).find(b => b.textContent === 'Back'); + backBtn.click(); + + assert.equal(ctx.document.querySelector('.changes-file-row[data-path="new.txt"] .changes-file-counts'), null); + assert.match(ctx.document.getElementById('changes-summary').textContent, /\+3 −1/, 'unknown counts must not be folded in as zero'); + } finally { ctx.destroy(); } +}); + +test('a failed untracked diff surfaces the error and leaves the counts alone', async () => { + const ctx = setupFilePanelDom({ diffImpl: () => ({ ok: false, error: 'fatal: bad thing' }) }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + ctx.document.querySelector('.changes-file-row[data-path="new.txt"]') + .dispatchEvent(new ctx.window.Event('click', { bubbles: true })); + await flush(); + const body = ctx.document.querySelector('.changes-diff-body'); - assert.match(body.textContent, /Untracked file/); + assert.match(body.textContent, /fatal: bad thing/); + + const backBtn = Array.from(ctx.document.querySelectorAll('#changes-diff-view button')).find(b => b.textContent === 'Back'); + backBtn.click(); + assert.equal(ctx.document.querySelector('.changes-file-row[data-path="new.txt"] .changes-file-counts'), null); } finally { ctx.destroy(); } }); diff --git a/test/git-changes-runner-real-git.test.js b/test/git-changes-runner-real-git.test.js index f66ae658e8858cd1e6065a2b65a2552c5f70d1b2..80e1458182a935161223fe401d98c2dff59b3ad7 100644 GIT binary patch literal 10469 zcmd^F?Q+}3742_5#a8tU!Gr`WyP0O>ai@->I&Kr&W66{0*siq%79=8Y!C(Q=s-jFE zqEFZ-={a{7AG9P#b=e1buo!Ly?SC!GlM3r`$ z=I%r-@*3Z)OPAMWb*BFHkAEs#s@#5OlG>!|*t{{x@kG^2qlz-og{m5BO*L|=+*nS? z0e{L16OAy{Xrkgc9;>ykm$AB^o|?+(D(7S7G&dU+<<2!m{pR*(d*g{ZeE#s^!Qs)P zCy&+d_c>WwCe6y&T5XoPR@y2(cV*GkMrKrHrL@}paaTUMwMp=RaY`?AZXM5og|LeW zr_wgX7d(c-5p%0@cc?S-Ft49tb1KhpCah66mBm>a{8g&CQH^yN6{~NIc1>kcPBk}6 zeVSwOh}OCt-~QdF6E&R{d2K3POnF#t#*O#IoO$g`kzw%+*2nJpQ*g3&v|qk5INvEo zVFxZxjTn01k7zg3wJ!E)R-B+6#){&{bUs>*?v$4>?Tn9{|F0kNrf1_RmQu(8f(0)RKQn|Z6c zDMK+{3AUr%=bgzigt6Nl$2&WC0zMPR-NQQHH#l1~vSn)a`Q>PQHw=LbOjWD52l=mO zfqA;onH!GGTt82_^-DSUh4C_cil6)E&O81&JhKVlyVe`aKh?$Xp=6mC>Fae>(#U)| zZ2w#(AdQ-=@IVmfo(a8eKOM(2ICR=tFEAiy0Z!;@V9vS%^8g(HO`7Y4EnS@_?ydmL znpYho+WDV&sGkGbraJ%s{ZZ1R;%I41NW@0@)+}8oe;_&R|sh^@G>^jX@D2VB|JAjyo@Pl};B)g>!>)M`KuDDVXb9L{_#N=^Z3qK9UXl0)F+V0#EV$%{`uhkG&%2?>&q zZPxOOz=2P$#QPXLQRkR3X)1@O_YqT#nb4?7_@FNQbc_)n0~ii(l(snAS80XR!xQ6B zh3J!qkB(k{Lfe2{5mRDNn8PX8AaOHRoHq>IvnnGBynI*w~)-) z@2T?(f)QI)kq7t-c;NIEGNF1hCjt;*&|t zKm4G!zyIp_;a~m==nF`5+ZZmZc*GYcudt6(e!l1(gCDjX0IWywGV)5baDM&Z!&b$K zGlG(PF`6b2bfcuKOe{8e3K!p-rRH>Io1*Yyk5j#(?_^?RvoZzb_SLAVv(GUEo7itj ztFvHmWycY@&Fg1yW<-yE3E;Hw07K-JY%2V61xf@Mihg1m!I?>b)1x|8$DzZiSkSs4LonatcH46Hr-_%TX$Z8hN}!~%?4oM!Cf<_ zX)p0fK+27b(a)wkS84c3!xZ{X>#zTCDsBw>n|CwLJI~)}CfGJkwczH9Cd+_Z@^bXz zw-dEDQMV@wf84M;zcMUZHCTqpAFQ@8wWnvO@P!xCV;Dg@sx+|ay*~`30TMrmEJy|hL=7mNM&&O)h`_0D^9iR?>RNz7$_fEQ>adpIGIh{vu(L&s3rx&Y+)KwKh&Fau)9U}rqC7s@h-o`8tb z>nO^^j#5gw3y-kVRE|^caUj)smP_(S#AoHlNyEfMmB>}XIUpggelQuK;$JQ z9+f!kYr*SE$qBewL_T$n)Ra;;|BnV7lF1P?n{+QA96&Omg`;#*v+O>@H@-!~>VLvY z-Wo(3*ba4puZYBK;2PidpfK&CPj3)|Do(@A?D-d z)kKz5UITSZKCW0RU1_?j2H&|fcx$5)$VkiE^KA`DO<}NE zr0;hOLOQSsdJGtr>B2EJK<$43HW~;v)~P6_18cG#hCM;ZsH;KIJsGi4B?16gQu8y2 z41~!4b4K@w?lbCpf_dBhAza_a+!wvTb}IgaOPh)@JRlL~+K`L`vIhzBS5kK{=hAzA zwlGRkFi03}BIZDJC(${$#3Nha4ftk7xV?w1Q60p)aqkJ;0sk!#+&Afi@+2jsD`AYL zR)q*6O#tN+_SEIB;RN~!R58rbG%V<}Ys+uS3c_#Ei<6IpdAJHh=L}kx{x0LP77lHt zsUPI44UdmYWkx>_nDp@Z(}QQP4-XzZJ2-m%cRaIBBD3^2Nn=l zWchdcbq2?i2`DiJ(FGQO@!JNaX9HhhiRU~DeL(|}b#)*8hPBe5bvD;23yBxKbH8pJ>M?Cgs!m^djy>PbvhmFRb(L34^0Y$~N}D^6J6yeBnov6moUt~i;w(V#}y zK`8N33lV^!VH0FRep;>Tv#w;|?a=@VkQ-&EHh}~xagpD6SQ^Zo`u+%%_`%WmlfW4* z&<_z(y$(OJr|ata0H0R2iXnsf<>FTS1CByj8icFQSHl~_7A@Pt-YbRR*x3WAX~n-% z2r|wG%5H_LFkJ8mFE>?&2uuB}?~lH%+j_a5f15sr>^nab%0b7S*X7~O10VO&;qLCN zaS_k$eJqXnKag)fB-#EXynE^mdX6^~U0oEBm?@ge( zPnIQ{zRbxYTBL=Dt_*gmZ75wRgQ_Yt z8@t_XY4A3(T2!;j^e@hY*b2%*`1JMu_C#2nWA-s@-?BaMA@-d1)kLY$Yk3_MHF~$}jxP}3m)lYM%gau9`)Y<) zkp^#I0VKW^;P5g+F*R@2D61ZJ~)2G|9Y<0B=Ljw2DXk|0wE40H}V diff --git a/test/git-changes-runner.test.js b/test/git-changes-runner.test.js index 60012adc..462a5acf 100644 --- a/test/git-changes-runner.test.js +++ b/test/git-changes-runner.test.js @@ -15,6 +15,7 @@ const { shQuote, isSafeCwd, isSafeGitPath, + isSafeNoIndexPath, MAX_DIFF_BYTES, STATUS_MAX_STDOUT_BYTES, DIFF_MAX_STDOUT_BYTES, @@ -97,6 +98,30 @@ test('isSafeGitPath rejects a leading ":" — git pathspec magic interpreted eve assert.equal(isSafeGitPath('src/:weird.js'), true, 'a colon not in the first position is not pathspec magic'); }); +// --- isSafeNoIndexPath: the guard for a --no-index filesystem operand ------- + +test('isSafeNoIndexPath rejects an absolute path — --no-index has no repository-containment check of its own (mutation target: reusing isSafeGitPath here)', () => { + assert.equal(isSafeNoIndexPath('/etc/passwd'), false); + assert.equal(isSafeNoIndexPath('\\\\host\\share\\secret'), false); + assert.equal(isSafeNoIndexPath('C:/Users/dev/.ssh/id_rsa'), false); + assert.equal(isSafeNoIndexPath('c:\\Users\\dev\\.ssh\\id_rsa'), false); + assert.equal(isSafeGitPath('/etc/passwd'), true, 'the pathspec guard accepts it — git itself refuses it there, but --no-index would not'); +}); + +test('isSafeNoIndexPath rejects a leading "-" — a --no-index operand sits where git parses options', () => { + assert.equal(isSafeNoIndexPath('-R'), false); + assert.equal(isSafeNoIndexPath('--output=/tmp/x'), false); + assert.equal(isSafeNoIndexPath('src/-dash.txt'), true, 'a dash that is not in the first position is an ordinary filename'); +}); + +test('isSafeNoIndexPath keeps the pathspec guard\'s rejections and accepts an ordinary relative path', () => { + assert.equal(isSafeNoIndexPath('../escape.txt'), false); + assert.equal(isSafeNoIndexPath('a\0b'), false); + assert.equal(isSafeNoIndexPath(':(exclude)x'), false); + assert.equal(isSafeNoIndexPath('newdir/sub/b.txt'), true); + assert.equal(isSafeNoIndexPath('café/déjà vu.txt'), true); +}); + // --- truncateDiffContent: byte cap, cut on a line boundary ----------------- test('truncateDiffContent: content at or under the cap is returned unchanged', () => { @@ -170,6 +195,18 @@ test('local runner .status(): three commands, no -C flag (cwd passed via execFil } }); +test('local runner .status(): status runs with -uall so a wholly-untracked directory is listed file by file, never as one directory row (mutation target: dropping -uall)', async () => { + const { exec, calls } = localFakeExec({}); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + await runner.status(); + + const statusArgs = calls.find((args) => args[1] === 'status'); + assert.deepEqual(statusArgs, ['--literal-pathspecs', 'status', '--porcelain=v2', '--branch', '-uall', '-z']); + for (const args of calls) { + if (args[1] === 'diff') assert.ok(!args.includes('-uall'), '-uall belongs to status only, it is not a diff option'); + } +}); + test('local runner .status(): a failing git call surfaces stderr as the error, not a throw', async () => { const exec = (args) => Promise.resolve( args[1] === 'status' ? { code: 128, stdout: '', stderr: 'fatal: not a git repository' } : { code: 0, stdout: '', stderr: '' } @@ -244,6 +281,110 @@ test('local runner .diff(): a diff under the cap is not marked truncated', async assert.equal(result.content, 'small diff\n'); }); +// --- local runner: .diff({untracked:true}) ---------------------------------- + +const UNTRACKED_DIFF = [ + 'diff --git a/new.txt b/new.txt', + 'new file mode 100644', + 'index 0000000..2cdcdb0', + '--- /dev/null', + '+++ b/new.txt', + '@@ -0,0 +1,2 @@', + '+a1', + '+a2', + '', +].join('\n'); + +test('local runner .diff({untracked:true}): exit code 1 with a diff on stdout is SUCCESS — git diff --no-index exits 1 whenever the two inputs differ (mutation target: the usual code !== 0 check)', async () => { + const exec = () => Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.diff('new.txt', { untracked: true }); + + assert.equal(result.ok, true, 'exit 1 from --no-index means "they differ", not "it failed"'); + assert.equal(result.content, UNTRACKED_DIFF); + assert.equal(result.truncated, false); +}); + +test('local runner .diff({untracked:true}): builds a --no-index invocation against /dev/null, with -- before the two operands', async () => { + const calls = []; + const exec = (args) => { calls.push(args); return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + await runner.diff('new.txt', { untracked: true }); + + assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'new.txt']); + assert.ok(!calls[0].includes('--cached'), 'an untracked file has nothing in the index'); +}); + +test('local runner .diff({untracked:true}): returns the added-line count the status pass could not know, with deleted 0', async () => { + const exec = () => Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.diff('new.txt', { untracked: true }); + + assert.equal(result.added, 2); + assert.equal(result.deleted, 0); +}); + +test('local runner .diff({untracked:true}): a binary file reports null counts and git\'s own note, not garbage', async () => { + const binary = 'diff --git a/bin.dat b/bin.dat\nnew file mode 100644\nBinary files /dev/null and b/bin.dat differ\n'; + const exec = () => Promise.resolve({ code: 1, stdout: binary, stderr: '' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.diff('bin.dat', { untracked: true }); + + assert.equal(result.ok, true); + assert.match(result.content, /Binary files .* differ/); + assert.equal(result.added, null, 'a binary file has no line count — null, not 0'); + assert.equal(result.deleted, null); +}); + +test('local runner .diff({untracked:true}): a truncated diff reports null counts — a partial diff cannot be counted', async () => { + const head = 'diff --git a/big.txt b/big.txt\n--- /dev/null\n+++ b/big.txt\n@@ -0,0 +1,6000 @@\n'; + const exec = () => Promise.resolve({ code: 1, stdout: head + ('+' + 'a'.repeat(100) + '\n').repeat(6000), stderr: '' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.diff('big.txt', { untracked: true }); + + assert.equal(result.truncated, true); + assert.equal(result.added, null); + assert.equal(result.deleted, null); +}); + +test('local runner .diff({untracked:true}): any exit code other than 0 or 1 is still an error', async () => { + const exec = () => Promise.resolve({ code: 128, stdout: '', stderr: 'fatal: not a git repository' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.diff('new.txt', { untracked: true }); + + assert.equal(result.ok, false); + assert.match(result.error, /not a git repository/); +}); + +test('local runner .diff({untracked:true}): exit 1 with NO stdout and a message on stderr is an error — that is how --no-index reports an inaccessible operand', async () => { + const exec = () => Promise.resolve({ code: 1, stdout: '', stderr: "error: Could not access 'gone.txt'" }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.diff('gone.txt', { untracked: true }); + + assert.equal(result.ok, false); + assert.match(result.error, /Could not access/); +}); + +test('local runner .diff({untracked:true}): an operand outside the working directory never reaches exec — --no-index would happily read it', async () => { + let calls = 0; + const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: 'SECRET', stderr: '' }); }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + + for (const bad of ['/etc/passwd', '../../etc/passwd', 'C:/Users/dev/.ssh/id_rsa', '--output=/tmp/pwn']) { + const result = await runner.diff(bad, { untracked: true }); + assert.equal(result.ok, false, `${bad} must be refused`); + assert.equal(result.error, 'invalid path'); + } + assert.equal(calls, 0, 'no unsafe operand may ever reach exec'); +}); + +test('local runner .diff({untracked:true}): a thrown exec rejects gracefully', async () => { + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec: () => { throw new Error('ENOENT'); } }); + const result = await runner.diff('new.txt', { untracked: true }); + assert.equal(result.ok, false); + assert.match(result.error, /ENOENT/); +}); + // --- remote runner: command shape ------------------------------------------- test('remote runner .status(): three ssh calls, each a full "git -C --literal-pathspecs ... -z" string', async () => { @@ -258,6 +399,10 @@ test('remote runner .status(): three ssh calls, each a full "git -C --lite assert.equal(result.ok, true); assert.equal(commands.length, 3); + assert.equal( + commands.find((c) => c.includes("'status'")), + "git -C '/srv/app' '--literal-pathspecs' 'status' '--porcelain=v2' '--branch' '-uall' '-z'" + ); for (const cmd of commands) { assert.match(cmd, /^git -C '\/srv\/app' '--literal-pathspecs' /); assert.match(cmd, /'-z'$/, 'must end in a quoted -z token'); @@ -275,6 +420,24 @@ test('remote runner .diff(): the built command carries --literal-pathspecs, quot assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'diff' '--cached' '--' 'src/weird file.js'"); }); +test('remote runner .diff({untracked:true}): every token of the --no-index command is individually quoted, and no backtick is ever emitted', async () => { + const commands = []; + const seenOpts = []; + const exec = (command, opts) => { + commands.push(command); + seenOpts.push(opts); + return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff("weird `name`$(x).txt", { untracked: true }); + + assert.equal(result.ok, true); + assert.equal(commands.length, 1); + assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'diff' '--no-index' '--' '/dev/null' 'weird `name`$(x).txt'"); + assert.match(commands[0], /^git -C '.*' '--literal-pathspecs' 'diff' '--no-index' '--' '\/dev\/null' '.*'$/s, 'the backtick never sits outside a quoted token'); + assert.equal(seenOpts[0].maxStdoutBytes, DIFF_MAX_STDOUT_BYTES); +}); + // --- remote runner: stdout cap wiring (adversarial review, CRITICAL finding 1) --- test('remote runner .status(): passes an explicit maxStdoutBytes cap to the transport for each of the three commands', async () => { diff --git a/test/git-changes.test.js b/test/git-changes.test.js index b760f512..a08fcf02 100644 --- a/test/git-changes.test.js +++ b/test/git-changes.test.js @@ -10,7 +10,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { parseStatusPorcelainV2, parseNumstat, mergeChanges } = require('../git-changes'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions } = require('../git-changes'); // --- parseStatusPorcelainV2 -------------------------------------------- @@ -204,7 +204,7 @@ test('mergeChanges: a file modified in both index and worktree sums both numstat assert.equal(merged.files[0].deleted, 5, 'staged (1) + unstaged (4) deleted lines'); }); -test('mergeChanges: an untracked file carries null counts, not zero — git diff never reports it', () => { +test('mergeChanges: an untracked file carries null counts, not zero — its counts only exist once its diff is fetched', () => { const status = { branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, files: [ { path: 'new.js', origPath: null, staged: false, unstaged: false, untracked: true, renamed: false, state: '?' }, ] }; @@ -232,6 +232,65 @@ test('mergeChanges: totals sum added/deleted across all files and count files', assert.deepEqual(merged.totals, { files: 2, added: 5, deleted: 1 }); }); +// --- countNewFileDiffAdditions ----------------------------------------- + +test('countNewFileDiffAdditions: counts only the lines inside the hunk, never the "+++ b/..." file header (mutation target: counting every line starting with "+")', () => { + const diff = [ + 'diff --git a/new.txt b/new.txt', + 'new file mode 100644', + 'index 0000000..2cdcdb0', + '--- /dev/null', + '+++ b/new.txt', + '@@ -0,0 +1,3 @@', + '+a1', + '+a2', + '+a3', + '', + ].join('\n'); + assert.equal(countNewFileDiffAdditions(diff), 3); +}); + +test('countNewFileDiffAdditions: an added line that itself looks like a diff header or a hunk marker still counts exactly once (mutation target: a prefix test instead of hunk tracking)', () => { + const diff = [ + 'diff --git a/patch.txt b/patch.txt', + 'new file mode 100644', + '--- /dev/null', + '+++ b/patch.txt', + '@@ -0,0 +1,3 @@', + '++++ b/inner', // the file's own content is "+++ b/inner" + '+@@ -1 +1 @@', // ... and "@@ -1 +1 @@" + '+plain', + '', + ].join('\n'); + assert.equal(countNewFileDiffAdditions(diff), 3); +}); + +test('countNewFileDiffAdditions: a binary diff reports null, not 0 — "unknown" is not "no lines"', () => { + const diff = [ + 'diff --git a/bin.dat b/bin.dat', + 'new file mode 100644', + 'index 0000000..c94be36', + 'Binary files /dev/null and b/bin.dat differ', + '', + ].join('\n'); + assert.equal(countNewFileDiffAdditions(diff), null); +}); + +test('countNewFileDiffAdditions: an empty new file has a header but no hunk — 0 additions, not null', () => { + const diff = 'diff --git a/empty.txt b/empty.txt\nnew file mode 100644\nindex 0000000..e69de29\n'; + assert.equal(countNewFileDiffAdditions(diff), 0); +}); + +test('countNewFileDiffAdditions: a missing-final-newline marker is not an addition', () => { + const diff = '--- /dev/null\n+++ b/x\n@@ -0,0 +1 @@\n+only\n\\ No newline at end of file\n'; + assert.equal(countNewFileDiffAdditions(diff), 1); +}); + +test('countNewFileDiffAdditions: empty/missing input is 0', () => { + assert.equal(countNewFileDiffAdditions(''), 0); + assert.equal(countNewFileDiffAdditions(null), 0); +}); + test('mergeChanges: branch pass-through defaults when status is missing', () => { const merged = mergeChanges(null, {}, {}); assert.deepEqual(merged.branch, { head: null, upstream: null, ahead: 0, behind: 0 }); From d8d8d4bb904b7cfc17bc8215af7c8b58ea8e1c33 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 16:24:10 +0200 Subject: [PATCH 2/5] fix(changes): resolve the untracked diff operand instead of trusting its shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A syntactic guard cannot enforce containment. A symlink inside the repository pointing at a directory outside it produces an operand with no "..", not absolute, no leading dash — and git diff --no-index, which has no repository-boundary check of its own, then reads whatever is under it. The operand is now resolved before git sees it, and what git receives is the guard's own value. Locally the cwd and the operand's parent directory are resolved with realpath and the parent must lie inside the resolved root; the parent and not the leaf, so a symlink git itself lists stays openable and still diffs as its target string rather than the target's content. An lstat then requires a regular file or a symlink, which also keeps a FIFO — where --no-index blocks until the timeout — away from git. Remotely there is no filesystem to resolve against, so git's own listing is the oracle: ls-files --others must return the path before any diff is sent. -uall now has a floor and a ceiling. Its output is a hard error when it overruns the transport's stdout cap, around 25-35k untracked files, which would blank a panel that used to work; a failing -uall status is retried once with git's default untracked mode and comes back flagged, so tracked changes still render. The renderer caps the list at 500 rows with a note for the remainder: every row is a DOM node plus a listener, rebuilt on every busy-to-idle edge. Two more defects fixed. A count computed against one status result could be written onto a later one, replacing git's authoritative numstat with a stale "+N -0" when a refresh landed while the diff was in flight; the write now requires the status result it was computed from. And the ".." guard was a substring test, so a file legitimately named "has..dots.txt" was listed and then refused on click — it now tests whole path segments. The real-git test file held a literal NUL byte where the escape belonged, which made git store it as a binary blob: no diff on review, no text search, no blame. --- .ai/contexts/changes-view.md | 151 ++++++++++---- .ai/contexts/ipc-bridge.md | 4 +- docs/changes-view.md | 12 ++ git-changes-runner.js | 101 ++++++++-- public/file-panel.js | 26 ++- public/style.css | 12 ++ test/dom-file-panel-changes.test.js | 96 +++++++++ test/git-changes-runner-real-git.test.js | Bin 10469 -> 14210 bytes test/git-changes-runner.test.js | 243 +++++++++++++++++++++-- 9 files changed, 570 insertions(+), 75 deletions(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 24b9afc6..641727fa 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -30,7 +30,7 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). ## Runner interface (`git-changes-runner.js`) -`createGitChangesRunner({kind, cwd, alias, exec, timeoutMs})` → `{status(), diff(path, {staged, untracked})}`. `status()` runs three commands in parallel (`git status --porcelain=v2 --branch -uall -z`, `git diff --numstat -z`, `git diff --cached --numstat -z`) and merges them. `diff()` runs `git diff [--cached] -- ` — or, with `untracked: true`, `git diff --no-index -- /dev/null ` (see "Untracked files") — capped at 512 KB (`MAX_DIFF_BYTES`) measured in UTF-8 bytes and cut on a line boundary, with a `truncated` flag. +`createGitChangesRunner({kind, cwd, alias, exec, timeoutMs, fsOps})` → `{status(), diff(path, {staged, untracked})}`. `status()` runs three commands in parallel (`git status --porcelain=v2 --branch -uall -z`, `git diff --numstat -z`, `git diff --cached --numstat -z`), merges them, and reports `untrackedCollapsed` (see "Untracked files"). `diff()` runs `git diff [--cached] -- ` — or, with `untracked: true`, `git diff --no-index -- /dev/null ` (see "Untracked files") — capped at 512 KB (`MAX_DIFF_BYTES`) measured in UTF-8 bytes and cut on a line boundary, with a `truncated` flag. - **Local** (`kind: 'local'`): `child_process.execFile('git', args, {cwd, timeout, maxBuffer})` — cwd is `execFile`'s own option, never a `-C` argument. No shell is invoked, so argument content cannot be interpreted as a command regardless of what it contains; timeout 10s. - **Remote** (`kind: 'remote'`): the same ssh transport `remote-attach.js` already uses for the tmux probe/restore calls (`buildRemoteCommandArgs`, `defaultRunRemoteCommand`) — `ssh -o BatchMode=yes -o ConnectTimeout=5 -n "git -C '' '--literal-pathspecs' 'diff' '--' '' ..."`. Timeout 20s. This command string DOES run through a shell on the far end. @@ -47,11 +47,12 @@ Four independent defenses, added after an adversarial review of the first cut of 5. **`shQuote()`** (unchanged from the original design) — standard POSIX single-quote escaping (close the quote, insert a literal quote via `'\''`, reopen) around `cwd` and every arg before they're interpolated into the remote command string. This is what actually makes the remote command injection-safe: a correctly single-quoted string cannot be broken out of by any byte sequence except an embedded NUL, and NUL can't appear in a shell token or a JS string used as one to begin with. One operand does not get this treatment: the filesystem path handed to -`git diff --no-index` for an untracked file, guarded by `isSafeNoIndexPath` — -strictly narrower, because `--no-index` drops git's own repository-boundary -check. See "Untracked files" below. +`git diff --no-index` for an untracked file. `--no-index` drops git's own +repository-boundary check, so that operand is guarded by `isSafeNoIndexPath` +*and* resolved on disk (local) or checked against git's own untracked listing +(remote) before it is used. See "Untracked files" below. -Given (5), `isSafeShellArg`/`isSafeCwd`/`isSafeGitPath` stay a **denylist** (control characters, any `..` segment, and now a leading `:`) rather than a positive character allowlist. Git paths and cwds legitimately contain almost any byte — spaces, unicode, punctuation, even a literal backtick or `$` in a filename — and an allowlist narrow enough to catch every shell metacharacter would also reject a lot of real filenames for no safety gain, since the metacharacters are already neutralized by the quoting, not by the character check. This mirrors the `open-terminal` `preLaunchCmd` guard's own documented lesson (`.ai/contexts/ipc-bridge.md`, "IPC path-guard inventory"): a denylist proved incomplete there because that string is deliberately raw shell; here the string is never raw shell in the first place, so closing by quoting is available and preferred over closing by enumeration. +Given (5), `isSafeShellArg`/`isSafeCwd`/`isSafeGitPath` stay a **denylist** (control characters, a `..` path segment, a leading `:`) rather than a positive character allowlist. The `..` test is on segments, split on both separators, not on the raw string: `..` is a traversal only as a whole segment, and a substring test refuses ordinary filenames — `has..dots.txt`, `v1..v2.diff`, `archive..2024.tar` — that git lists and the panel must therefore be able to open. Git paths and cwds legitimately contain almost any byte — spaces, unicode, punctuation, even a literal backtick or `$` in a filename — and an allowlist narrow enough to catch every shell metacharacter would also reject a lot of real filenames for no safety gain, since the metacharacters are already neutralized by the quoting, not by the character check. This mirrors the `open-terminal` `preLaunchCmd` guard's own documented lesson (`.ai/contexts/ipc-bridge.md`, "IPC path-guard inventory"): a denylist proved incomplete there because that string is deliberately raw shell; here the string is never raw shell in the first place, so closing by quoting is available and preferred over closing by enumeration. `buildRemoteGitCommand`/`shQuote` never emit a backtick for any input, proven in `test/git-changes-runner.test.js` including adversarial cwd/path values containing backtick, `$(...)`, and an embedded single quote. @@ -63,16 +64,40 @@ An untracked file is a first-class row: one row per file, a real diff on click, and line counts that reach the header total. Four decisions hold that up, each measured against real git (git 2.53, `test/git-changes-runner-real-git.test.js`). -**1. `status` runs with `-uall`.** Git's default `--untracked-files=normal` -reports a wholly-untracked directory as a single entry (`? newdir/`) and never -descends, so a brand-new directory collapsed to one row whose `path` was a -directory — unopenable, uncountable. `-uall` lists every file individually -(`? newdir/a.txt`, `? newdir/sub/b.txt`), which is what makes "no row's `path` -is ever a directory" an invariant rather than a hope. The cost is a longer -status output on a repo with a large unignored tree; it is bounded by the -existing `STATUS_MAX_STDOUT_BYTES` (2 MB) cap on the remote transport, and by -`execFile`'s `maxBuffer` locally — a repo that overruns it surfaces the cap -message instead of a wrong answer. +**1. `status` runs with `-uall`, with a floor under it.** Git's default +`--untracked-files=normal` reports a wholly-untracked directory as a single +entry (`? newdir/`) and never descends, so a brand-new directory is one row +whose `path` is a directory — unopenable, uncountable. `-uall` lists every file +individually (`? newdir/a.txt`, `? newdir/sub/b.txt`), which is what makes "no +row's `path` is ever a directory" an invariant rather than a hope. + +The cost is volume, and it is bounded at both ends. Measured on a synthetic +repo, 20 000 untracked files: ~570 KB of porcelain and ~70 ms (against ~8 ms +and 81 bytes for the same repo without `-uall`) — time is a non-issue even on +every busy→idle edge, volume is not. At ~28 bytes per row for short paths, and +2–3× that for realistic ones, the remote transport's +`STATUS_MAX_STDOUT_BYTES` (2 MiB) cap is reached somewhere around 25 000–35 000 +untracked files. That cap is a **hard error** (`stdout exceeded …`, empty +stdout), so taking it at face value would blank the whole panel — including the +tracked changes, which cost nothing and are usually the reason the panel is +open. So a failing `-uall` status is retried once with git's default untracked +mode; if that succeeds the result comes back `untrackedCollapsed: true` and the +panel renders the tracked rows, the collapsed `? dir/` rows, and a note saying +the untracked listing is coarse. If the retry fails too, the original error is +returned unchanged — a broken repository is still an error, not a degraded +listing. + +The renderer holds the other end: `MAX_CHANGES_ROWS` (500) in +`public/file-panel.js` caps how many rows are built, with a `+N more files not +shown` note for the remainder. Every row is a DOM node plus its own click +listener, rebuilt from scratch on every refresh, and `refreshChanges` runs on +every busy→idle edge — an unbounded list would put tens of thousands of node +constructions on the Electron UI thread at exactly the moment a turn ends. 500 +is a reading limit, not a memory one: the panel is a viewer, and a list longer +than that is not scanned, it is searched — which this panel does not offer. +Porcelain v2 emits the ordinary/rename/unmerged records before the untracked +ones (measured), so the rows dropped by the cap are untracked ones first; the +header total keeps counting every file, capped or not. **2. The empty side of the diff is the literal string `/dev/null`.** `git diff --no-index -- /dev/null ` produces exactly the new-file diff @@ -84,8 +109,12 @@ Measured: `git diff --no-index -- /dev/null /dev/zero` fails with device), while `/dev/null` on the same side succeeds; and `nul`, git's Windows spelling for the same thing, is refused on Linux. Both observations match git's own `diff-no-index.c`, where the `/dev/null` string is special-cased -unconditionally and `nul` only under `GIT_WINDOWS_NATIVE`. So `/dev/null` is -the portable spelling, and the two alternatives are worse: creating an empty +unconditionally and `nul` only under `GIT_WINDOWS_NATIVE`. **The Windows half of +that reasoning has never been executed** — every measurement here is from Linux; +the `windows-2022` CI leg running `test/git-changes-runner-real-git.test.js` is +the evidence, and it is worth reading before merging anything that touches this +operand. So `/dev/null` is the portable spelling, and the two alternatives are +worse: creating an empty temp file means writing into a repository under test (and cleaning it up on every error path, remote included), and `git add -N` mutates the index of a repository the user is actively working in, which a read-only viewer must never @@ -102,27 +131,56 @@ stdout plus a message on stderr is an error. A successful `--no-index` against `/dev/null` always writes at least a `diff --git`/`new file mode` header, even for an empty file. -**4. A `--no-index` operand is a filesystem path, and gets a stricter guard -than a pathspec** (`isSafeNoIndexPath`, not `isSafeGitPath`). The pathspec guard -can afford to be a denylist because git itself enforces the repository boundary: -an absolute pathspec outside the repo is refused with `fatal: … is outside -repository` (measured; see below). **`--no-index` has no such containment check -at all** — measured: raw `git diff --no-index -- /dev/null /tmp/…/outside-secret.txt` -from inside the repo prints the file. Nothing downstream would stop it, so the -guard is what keeps an untracked diff inside the working directory: -`isSafeNoIndexPath` = `isSafeGitPath` (control characters, any `..`, leading -`:`) **plus** no absolute path (leading `/`, leading `\`, `X:` drive prefix) and -no leading `-` (the operand sits where git parses options, and `--` separation -is belt-and-braces, not the only defense). A repo-root-level file whose name -starts with `-` is therefore not diffable from the panel — an accepted, narrow -loss against an operand that could otherwise be read as a git option. - -Nothing changes for the remote transport: the untracked call goes through the -same `invoke()` → `buildRemoteGitCommand`/`shQuote` path as every other command, -so `/dev/null` and the path are each their own single-quoted token and the -"never emits a backtick outside a quoted token" property is unchanged -(asserted in `test/git-changes-runner.test.js` with a path containing a -backtick and `$(…)`). +**4. A `--no-index` operand is a filesystem path, so containment is resolved, +never inferred from the string.** The pathspec guard (`isSafeGitPath`) can +afford to be a denylist because git itself enforces the repository boundary: an +absolute pathspec outside the repo is refused with `fatal: … is outside +repository` (measured; see below). **`--no-index` has no containment check at +all** — measured: raw `git diff --no-index -- /dev/null /tmp/…/outside-secret.txt` +from inside the repo prints the file. Nothing downstream stops it, so the runner +does, in two layers: + +- **Syntactic** (`isSafeNoIndexPath`): `isSafeGitPath` (control characters, a + `..` path segment, a leading `:`) **plus** no absolute path (leading `/`, + leading `\`, `X:` drive prefix) and no leading `-` (the operand sits where git + parses options; `--` separation is belt-and-braces, not the only defense). A + repo-root-level file whose name starts with `-` is therefore not diffable from + the panel — an accepted, narrow loss. +- **Resolved** — the layer that actually enforces containment, because a + syntactic check cannot: a symlink *inside* the repo pointing at a directory + *outside* it yields an operand with no `..`, not absolute, that reads anything + under that directory (measured: raw git prints the out-of-repo file for + `link-to-dir/outside-secret.txt`). What each transport can do about it differs: + - **Local** (`resolveLocalNoIndexOperand`): `fs.realpathSync.native` on the + cwd and on the operand's **parent directory**, and the parent must be the + resolved root or below it (`path.relative`, not a string prefix — a sibling + named `/repo-evil` shares the prefix but is not inside `/repo`). The parent, + not the leaf: git `lstat`s the operand itself, so a leaf symlink diffs as + `new file mode 120000` plus the link target *string* and leaks no content — + resolving the leaf would instead refuse a row git legitimately lists. An + `lstat` on the leaf then requires a regular file or a symlink, which also + keeps a FIFO (where `git diff --no-index` blocks until the timeout) away + from git. This is the `resolveOnDisk` + realpath-containment shape + `ipc-path-validator.js` documents, including its TOCTOU rule: what git + receives is the **guard's** operand (relative to the resolved root), never + the caller's string. + - **Remote**: there is no local filesystem to resolve against, so git's own + view of the repository is the oracle — + `git ls-files --others --exclude-standard -z -- ` must return exactly + that path before any diff is sent. Measured: it lists a genuine untracked + file, and returns nothing for a path behind a symlinked directory (git's + traversal does not descend symlinks), for a tracked file, or for a FIFO. + Cost: one extra ssh round-trip per untracked row click, sequential (running + it alongside the diff would mean the far host had already read the file). + - `fsOps` (`{realpath, lstat}`) is dependency injection for tests only, the + same seam `remote-attach.js` uses for `spawnFn`; production always takes the + real fs. + +The untracked calls go through the same `invoke()` → +`buildRemoteGitCommand`/`shQuote` path as every other command, so `/dev/null` +and the path are each their own single-quoted token and the "never emits a +backtick outside a quoted token" property holds for both of them (asserted in +`test/git-changes-runner.test.js` with a path containing a backtick and `$(…)`). #### Why the counts arrive on click, not with status @@ -152,10 +210,19 @@ session, and local and remote must not disagree about what the panel shows. A `--no-index` diff's file-header lines are `--- /dev/null` and `+++ b/` (not the `a/ b/` pair a tracked diff carries). `classifyDiffLine()` keys on the `---`/`+++`/`@@`/`+`/`-` prefixes only, so both land on -`changes-diff-file-header` exactly as they do for a tracked diff — no renderer -change was needed, and `test/dom-file-panel-changes.test.js` pins it so a future -"classify by `a/`…`b/` pair" refactor cannot silently render `--- /dev/null` as -a deleted line. +`changes-diff-file-header` exactly as a tracked diff's do. +`test/dom-file-panel-changes.test.js` pins that, so a "classify by the +`a/`…`b/` pair" refactor cannot silently render `--- /dev/null` as a deleted +line. + +A count is only ever written back onto the status result it was computed +against: `applyUntrackedCounts` takes that result and returns early unless +`tab.data` is still the same object. `refreshChanges` replaces `tab.data` but +leaves `tab.selectedFile` alone, so the in-flight guard on the diff response +(`currentTab`/`selectedFile` identity) does not catch a refresh that landed +mid-flight — and the row a stale count would be stamped on is re-found by path, +which may by then be a *tracked* file carrying git's own authoritative numstat +counts. ### Remote transport stdout cap (`remote-attach.js` `defaultRunRemoteCommand`) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index ac0f418a..06ef38ce 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -88,7 +88,7 @@ refresh triggers): `.ai/contexts/changes-view.md`. User-facing: `docs/changes-vi | IPC | Args | Returns | Notes | |---|---|---|---| -| `git-changes-status` | `(sessionId)` | `{ok, branch, files, totals} \| {ok:false, error}` | `git status --porcelain=v2 --branch -uall` + `git diff --numstat` + `git diff --cached --numstat`, merged by `git-changes.js`'s `mergeChanges()`. | +| `git-changes-status` | `(sessionId)` | `{ok, branch, files, totals, untrackedCollapsed} \| {ok:false, error}` | `git status --porcelain=v2 --branch -uall` + `git diff --numstat` + `git diff --cached --numstat`, merged by `git-changes.js`'s `mergeChanges()`. A `-uall` run too large for the transport falls back to git's default untracked mode and reports `untrackedCollapsed: true`. | | `git-changes-diff` | `(sessionId, filePath, staged, untracked)` | `{ok, content, truncated, added, deleted} \| {ok:false, error}` | `git diff [--cached] -- `, or `git diff --no-index -- /dev/null ` when `untracked`; capped at 512 KB. `added`/`deleted` are filled for an untracked file only — see `.ai/contexts/changes-view.md` ("Untracked files"). | ### Misc @@ -157,7 +157,7 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | `add-project` / `remap-project` | none on the probe (`fs.statSync`/`fs.existsSync`/`fs.lstatSync`); the actual write is confined through `encodeProjectPath` | existence/type oracle only — inherent to the feature (both accept an arbitrary disk location by design), not cheaply fixable without breaking it | | `open-terminal` (`preLaunchCmd`) | `validatePreLaunchCmd` (`pre-launch-cmd-guard.js`) | not a path guard — a character allowlist on a raw-shell-by-design string (the documented prefix's character set plus its analogues: `env VAR=val`, `doas`, an absolute binary path); a denylist here proved incomplete (process substitution `<(...)`/`>(...)` needed none of the blocked characters), so this is closed by construction instead of by enumeration. Known cost: bare `$VAR` expansion and quoted arguments, both previously accepted, are now refused | | `read-session-jsonl` / `read-subagent-jsonl` / `start-subagent-watch` / `create-schedule-session` | none directly — path is derived from a SQLite key or built via `encodeProjectPath`, not taken verbatim from the renderer | out of scope for a path guard; flag if a renderer-controlled string is ever found reaching the derivation unencoded | -| `git-changes-diff` | `isSafeGitPath`, or `isSafeNoIndexPath` when `untracked` (`git-changes-runner.js`) | not a filesystem path — a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist. The untracked variant is a real filesystem operand of `git diff --no-index`, which has no repository-boundary check of its own, so its guard additionally rejects absolute paths and a leading `-` — see "Untracked files" in the same doc | +| `git-changes-diff` | `isSafeGitPath`, or `isSafeNoIndexPath` + containment when `untracked` (`git-changes-runner.js`) | a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist. The untracked variant is a real filesystem operand of `git diff --no-index`, which has no repository-boundary check of its own: on top of the syntactic guard it is resolved with `realpath` against the resolved cwd (local) or checked against `git ls-files --others` (remote), and git receives the guard's operand, not the caller's — see "Untracked files" in the same doc | ### Non-obvious behaviors diff --git a/docs/changes-view.md b/docs/changes-view.md index 2848f7e5..8f8a28ae 100644 --- a/docs/changes-view.md +++ b/docs/changes-view.md @@ -14,6 +14,18 @@ Click the **Changes** button in the terminal header, next to the stop button. Cl - A brand-new directory is listed file by file, not as a single folder row. - A **Refresh** button for a manual pull. +### Very large working trees + +The list shows at most 500 rows, then a `+N more files not shown` line; the +header keeps counting every changed file. Changed tracked files come first, so +what the cap drops is untracked files. + +A working tree with tens of thousands of untracked files — an unignored +`node_modules`, a vendored or build directory — can be more than the panel can +fetch file by file, especially over ssh. Changes then falls back to listing +untracked entries by directory, the way `git status` does by default, and says +so under the header. Your tracked changes are unaffected. + ### Counts for new files Git reports line counts for tracked files only, so an untracked file's row diff --git a/git-changes-runner.js b/git-changes-runner.js index bcf1855f..14347b99 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -3,6 +3,8 @@ 'use strict'; const { execFile } = require('child_process'); +const fs = require('fs'); +const path = require('path'); const { defaultRunRemoteCommand } = require('./remote-attach'); const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions } = require('./git-changes'); @@ -24,10 +26,14 @@ function isSafeCwd(cwd) { return isSafeShellArg(cwd); } +function hasDotDotSegment(p) { + return p.split(/[/\\]/).includes('..'); +} + // Denylist plus a leading-':' shape check — see .ai/contexts/changes-view.md ("Quoting rule"). function isSafeGitPath(p) { if (!isSafeShellArg(p)) return false; - if (p.includes('..')) return false; + if (hasDotDotSegment(p)) return false; if (p[0] === ':') return false; return true; } @@ -43,6 +49,39 @@ function isSafeNoIndexPath(p) { return true; } +// fs seam — injected in tests, real fs in production (same pattern as remote-attach.js's spawnFn) +const DEFAULT_FS_OPS = { + realpath: (p) => fs.realpathSync.native(p), + lstat: (p) => fs.lstatSync(p), +}; + +function isInsideRoot(root, candidate) { + if (candidate === root) return true; + const rel = path.relative(root, candidate); + return rel !== '' && rel !== '..' && !rel.startsWith('..' + path.sep) && !path.isAbsolute(rel); +} + +// Containment for a --no-index operand — see .ai/contexts/changes-view.md ("Untracked files") +function resolveLocalNoIndexOperand(cwd, filePath, fsOps = DEFAULT_FS_OPS) { + if (!isSafeNoIndexPath(filePath)) return null; + + try { + const root = fsOps.realpath(cwd); + const absolute = path.resolve(root, filePath); + const parent = fsOps.realpath(path.dirname(absolute)); + if (!root || !parent || !isInsideRoot(root, parent)) return null; + + const resolved = path.join(parent, path.basename(absolute)); + const stat = fsOps.lstat(resolved); + if (!stat.isFile() && !stat.isSymbolicLink()) return null; + + const operand = path.relative(root, resolved); + return isSafeNoIndexPath(operand) ? operand : null; + } catch { + return null; + } +} + // --literal-pathspecs on every invocation — see .ai/contexts/changes-view.md ("Quoting rule"). function buildGitArgs(args) { return ['--literal-pathspecs', ...args]; @@ -99,8 +138,8 @@ function firstError(result) { return (result.stderr || '').trim() || `git exited with code ${result.code}`; } -// {kind, cwd, alias, exec, timeoutMs} — see .ai/contexts/changes-view.md ("Runner interface") -function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { +// {kind, cwd, alias, exec, timeoutMs, fsOps} — see .ai/contexts/changes-view.md ("Runner interface") +function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs, fsOps } = {}) { if (kind !== 'local' && kind !== 'remote') { throw new Error('createGitChangesRunner requires kind "local" or "remote"'); } @@ -137,24 +176,59 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { } catch (err) { return { ok: false, error: err.message }; } - const [st, unstagedNum, stagedNum] = results; - if (st.code !== 0) return { ok: false, error: firstError(st) }; + let [st] = results; + const [, unstagedNum, stagedNum] = results; if (unstagedNum.code !== 0) return { ok: false, error: firstError(unstagedNum) }; if (stagedNum.code !== 0) return { ok: false, error: firstError(stagedNum) }; + // A repo too large for -uall falls back to git's collapsed listing — see .ai/contexts/changes-view.md ("Untracked files") + let untrackedCollapsed = false; + if (st.code !== 0) { + let fallback; + try { + fallback = await invoke(['status', '--porcelain=v2', '--branch', '-z'], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }); + } catch { + return { ok: false, error: firstError(st) }; + } + if (fallback.code !== 0) return { ok: false, error: firstError(st) }; + st = fallback; + untrackedCollapsed = true; + } + const parsedStatus = parseStatusPorcelainV2(st.stdout); const numstatUnstaged = parseNumstat(unstagedNum.stdout); const numstatStaged = parseNumstat(stagedNum.stdout); - return { ok: true, ...mergeChanges(parsedStatus, numstatStaged, numstatUnstaged) }; + return { ok: true, ...mergeChanges(parsedStatus, numstatStaged, numstatUnstaged), untrackedCollapsed }; + } + + // The operand git receives is the guard's own, never the caller's — see .ai/contexts/changes-view.md ("Untracked files") + async function resolveUntrackedOperand(filePath) { + if (!isSafeNoIndexPath(filePath)) return { ok: false, error: 'invalid path' }; + + if (kind === 'local') { + const operand = resolveLocalNoIndexOperand(cwd, filePath, fsOps || DEFAULT_FS_OPS); + return operand ? { ok: true, operand } : { ok: false, error: 'invalid path' }; + } + + let listed; + try { + listed = await invoke(['ls-files', '--others', '--exclude-standard', '-z', '--', filePath], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }); + } catch (err) { + return { ok: false, error: err.message }; + } + if (listed.code !== 0) return { ok: false, error: firstError(listed) }; + const operand = String(listed.stdout || '').split('\0')[0]; + return operand === filePath ? { ok: true, operand } : { ok: false, error: 'invalid path' }; } // `--no-index` exits 1 on a difference — see .ai/contexts/changes-view.md ("Untracked files") - async function untrackedDiff(path) { - if (!isSafeNoIndexPath(path)) return { ok: false, error: 'invalid path' }; + async function untrackedDiff(filePath) { + const contained = await resolveUntrackedOperand(filePath); + if (!contained.ok) return { ok: false, error: contained.error }; let result; try { - result = await invoke(['diff', '--no-index', '--', NO_INDEX_EMPTY_SIDE, path], { maxStdoutBytes: DIFF_MAX_STDOUT_BYTES }); + result = await invoke(['diff', '--no-index', '--', NO_INDEX_EMPTY_SIDE, contained.operand], { maxStdoutBytes: DIFF_MAX_STDOUT_BYTES }); } catch (err) { return { ok: false, error: err.message }; } @@ -167,11 +241,11 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { return { ok: true, content, truncated, added, deleted: added === null ? null : 0 }; } - async function diff(path, opts = {}) { - if (opts.untracked) return untrackedDiff(path); - if (!isSafeGitPath(path)) return { ok: false, error: 'invalid path' }; + async function diff(filePath, opts = {}) { + if (opts.untracked) return untrackedDiff(filePath); + if (!isSafeGitPath(filePath)) return { ok: false, error: 'invalid path' }; const staged = !!opts.staged; - const args = staged ? ['diff', '--cached', '--', path] : ['diff', '--', path]; + const args = staged ? ['diff', '--cached', '--', filePath] : ['diff', '--', filePath]; let result; try { @@ -197,6 +271,7 @@ module.exports = { isSafeCwd, isSafeGitPath, isSafeNoIndexPath, + resolveLocalNoIndexOperand, MAX_DIFF_BYTES, STATUS_MAX_STDOUT_BYTES, DIFF_MAX_STDOUT_BYTES, diff --git a/public/file-panel.js b/public/file-panel.js index e3c9f2fb..ed5b9deb 100644 --- a/public/file-panel.js +++ b/public/file-panel.js @@ -40,6 +40,9 @@ let changesListEl = null; let changesDiffEl = null; let changesToggleBtn = null; +// Row ceiling for the Changes list — see .ai/contexts/changes-view.md ("Untracked files") +const MAX_CHANGES_ROWS = 500; + const PANEL_WIDTH_KEY = 'filePanelWidth'; const DEFAULT_PANEL_WIDTH = parseInt(localStorage.getItem(PANEL_WIDTH_KEY), 10) || 450; const MIN_PANEL_WIDTH = 280; @@ -660,6 +663,7 @@ async function openChangesDiff(sessionId, file) { tab.diffError = null; tab.diffContent = null; tab.diffTruncated = false; + const dataAtRequest = tab.data; tab.diffLoading = true; if (currentPanelSessionId === sessionId) renderPanel(sessionId); @@ -675,15 +679,15 @@ async function openChangesDiff(sessionId, file) { } else { tab.diffContent = result.content; tab.diffTruncated = !!result.truncated; - if (file.untracked) applyUntrackedCounts(tab, file.path, result.added, result.deleted); + if (file.untracked) applyUntrackedCounts(tab, dataAtRequest, file.path, result.added, result.deleted); } if (currentPanelSessionId === sessionId) renderPanel(sessionId); } // Untracked counts arrive with the diff, not with status — see .ai/contexts/changes-view.md -function applyUntrackedCounts(tab, filePath, added, deleted) { +function applyUntrackedCounts(tab, expectedData, filePath, added, deleted) { if (typeof added !== 'number') return; - if (!tab.data || !Array.isArray(tab.data.files)) return; + if (!tab.data || tab.data !== expectedData || !Array.isArray(tab.data.files)) return; const record = tab.data.files.find((f) => f.path === filePath); if (!record) return; @@ -755,10 +759,24 @@ function renderChangesContent(sessionId, tab) { branchInfoEl.textContent = parts.join(' '); } + if (data.untrackedCollapsed) { + const note = document.createElement('div'); + note.className = 'changes-degraded-note'; + note.textContent = 'Too many untracked files to list — untracked entries are collapsed into their directories.'; + changesSummaryEl.appendChild(note); + } + changesListEl.innerHTML = ''; - for (const file of files) { + const shown = files.length > MAX_CHANGES_ROWS ? files.slice(0, MAX_CHANGES_ROWS) : files; + for (const file of shown) { changesListEl.appendChild(buildChangesFileRow(sessionId, file)); } + if (shown.length < files.length) { + const more = document.createElement('div'); + more.className = 'changes-more-note'; + more.textContent = `+${files.length - shown.length} more files not shown`; + changesListEl.appendChild(more); + } } function buildChangesFileRow(sessionId, file) { diff --git a/public/style.css b/public/style.css index c45b4fe2..44db6efc 100644 --- a/public/style.css +++ b/public/style.css @@ -4548,6 +4548,18 @@ body { display: flex; flex-direction: column; } font-size: 12px; } +.changes-degraded-note { + margin-top: 4px; + font-size: 11px; + color: #c8a24a; +} + +.changes-more-note { + padding: 6px 12px; + font-size: 11px; + color: #9090a8; +} + #changes-diff-view { flex: 1; display: flex; diff --git a/test/dom-file-panel-changes.test.js b/test/dom-file-panel-changes.test.js index 3e9552b1..b98d18b7 100644 --- a/test/dom-file-panel-changes.test.js +++ b/test/dom-file-panel-changes.test.js @@ -251,6 +251,102 @@ test('an untracked binary file keeps null counts — the row stays countless and } finally { ctx.destroy(); } }); +test('a count computed against one status result is never applied to a later one (mutation target: dropping the identity check)', async () => { + // v2 is what git says after the user staged and trimmed new.txt while the + // untracked diff of v1 was still in flight: the file is tracked now, with + // authoritative numstat counts that must not be overwritten. + const v2 = { + ok: true, + branch: { head: 'main', upstream: 'origin/main', ahead: 1, behind: 0 }, + files: [ + { path: 'src/a.js', origPath: null, staged: true, unstaged: false, untracked: false, renamed: false, state: 'M', added: 3, deleted: 1 }, + { path: 'new.txt', origPath: null, staged: true, unstaged: false, untracked: false, renamed: false, state: 'A', added: 2, deleted: 7 }, + ], + totals: { files: 2, added: 5, deleted: 8 }, + }; + let statusCall = 0; + let releaseDiff; + const ctx = setupFilePanelDom({ + statusImpl: () => (statusCall++ === 0 ? makeStatusResult() : v2), + diffImpl: () => new Promise((resolve) => { releaseDiff = () => resolve(UNTRACKED_DIFF_RESULT); }), + }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + ctx.document.querySelector('.changes-file-row[data-path="new.txt"]') + .dispatchEvent(new ctx.window.Event('click', { bubbles: true })); + await flush(); + + // A busy→idle edge lands while the diff is still in flight. + ctx.setActivity('s1', true); + ctx.setActivity('s1', false); + await flush(); + assert.equal(ctx.calls.status.length, 2, 'the refresh happened'); + + releaseDiff(); + await flush(); + + const backBtn = Array.from(ctx.document.querySelectorAll('#changes-diff-view button')).find(b => b.textContent === 'Back'); + backBtn.click(); + + const counts = ctx.document.querySelector('.changes-file-row[data-path="new.txt"] .changes-file-counts'); + assert.equal(counts.textContent, '+2−7', 'git\'s own counts must survive the stale diff'); + assert.match(ctx.document.getElementById('changes-summary').textContent, /2 files changed \+5 −8/); + } finally { ctx.destroy(); } +}); + +test('an overrun -uall listing degrades instead of blanking the panel: tracked rows render, with a note', async () => { + const collapsed = { + ok: true, + branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, + files: [ + { path: 'src/a.js', origPath: null, staged: true, unstaged: false, untracked: false, renamed: false, state: 'M', added: 3, deleted: 1 }, + { path: 'vendor/', origPath: null, staged: false, unstaged: false, untracked: true, renamed: false, state: '?', added: null, deleted: null }, + ], + totals: { files: 2, added: 3, deleted: 1 }, + untrackedCollapsed: true, + }; + const ctx = setupFilePanelDom({ statusImpl: () => collapsed }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + assert.equal(ctx.document.querySelectorAll('.changes-file-row').length, 2, 'tracked changes still render'); + const note = ctx.document.querySelector('.changes-degraded-note'); + assert.ok(note, 'the panel says why the untracked listing is coarse'); + assert.match(note.textContent, /collapsed/); + assert.equal(ctx.document.querySelector('.changes-error'), null, 'this is a degraded listing, not an error'); + } finally { ctx.destroy(); } +}); + +test('the row list is capped, with a note for the remainder (mutation target: rendering one node per file unbounded)', async () => { + const many = { + ok: true, + branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, + files: Array.from({ length: 1200 }, (_, i) => ({ + path: `f${i}.txt`, origPath: null, staged: false, unstaged: false, untracked: true, renamed: false, state: '?', added: null, deleted: null, + })), + totals: { files: 1200, added: 0, deleted: 0 }, + }; + const ctx = setupFilePanelDom({ statusImpl: () => many }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + const rows = ctx.document.querySelectorAll('.changes-file-row'); + assert.ok(rows.length < 1200, 'the list must not build one node per file without a bound'); + assert.equal(rows.length, 500); + const more = ctx.document.querySelector('.changes-more-note'); + assert.ok(more, 'the user must be told rows are missing'); + assert.equal(more.textContent, '+700 more files not shown'); + assert.match(ctx.document.getElementById('changes-summary').textContent, /1200 files changed/, 'the header still counts every file'); + } finally { ctx.destroy(); } +}); + test('a failed untracked diff surfaces the error and leaves the counts alone', async () => { const ctx = setupFilePanelDom({ diffImpl: () => ({ ok: false, error: 'fatal: bad thing' }) }); try { diff --git a/test/git-changes-runner-real-git.test.js b/test/git-changes-runner-real-git.test.js index 80e1458182a935161223fe401d98c2dff59b3ad7..4e76933ee2dcd47eba15f7e38291a1dafe74aba8 100644 GIT binary patch delta 2005 zcmbtV&x;&I6ef~g!{EjryPHki%jTk3Vm#s1sOriVLAKR$DMcfa31+&KTk(hSUWoM&<*IPgr1*eX4R zOqz@fu&Dqo3I$vV15#N~g^<8t!ls}~3qODOso#Xlnn{kQA)K@QZ2N zQb9Y4+Mzd`-8CvNEw1BoE*K}?oSF2RH~!dd7?w;Hwwg`%&HCEQY7|1kazmV}q$1Tl z%yGdqMGhUPRq*H&rAMCpT=KwuwZ7>-Yd-5Jon?)3GBV3a?HXGub56D4v4EYm}QskRzltW>3E)$(p&DE0O%GOk-<-VyHT z!h_4?{l5m4`udTidd<3Z)BN9z{&O<_8q8Do-HqU#Ws;;c?xQC!C2G{E{zXK|L`IE8 zr;~JHa1)ZKK;AGIEJU{=j)r<|GlSBSR&xr>pcU96Rqh8~+U@60 z+D18(Vozu=rA}BZjO)S8q=J~F$_S8bEWE`Gr>gHPN=J@dumBp1lrrN3MQ!I}D{*u( z^qnEHVL47p8oH@MUS2aJhGQXJ(Xx*L;PeZcDKk;Tl{NEW7bq!qrJX1oc-TGfF1Yf> ze|6;~=tEno4kzW$KDx~JX>T`V(vtRk#@WgSCqrLgV$-dqtHwuf{LVQ)7CEefav!pWlV7 AX8-^I delta 222 zcmZq5e;T;Kf_1Yu>pdYxhRwPXH&`dn6Xcm}r)^`U@Tg&mg05~}zHVk-N@|5dVoq_s zLTW{3NwI>VLPZwRc*BR09=* zRi)+@lvF|$C}b8ZBq|i8Cgv!lC1&Q7f{X&10u)QhOiKfroR>UVK~#3~8$%8w=ls%~ z6o_nca%yq0zK()=dS*$Cda*)&d7eTpP$)4y6=Y&@X;E5Ya%#%t1VM+*Jo;rE0Qu5S AX#fBK diff --git a/test/git-changes-runner.test.js b/test/git-changes-runner.test.js index 462a5acf..ce92d8b6 100644 --- a/test/git-changes-runner.test.js +++ b/test/git-changes-runner.test.js @@ -16,6 +16,7 @@ const { isSafeCwd, isSafeGitPath, isSafeNoIndexPath, + resolveLocalNoIndexOperand, MAX_DIFF_BYTES, STATUS_MAX_STDOUT_BYTES, DIFF_MAX_STDOUT_BYTES, @@ -84,6 +85,25 @@ test('isSafeGitPath rejects path traversal (mutation target: dropping the ".." c assert.equal(isSafeGitPath('src/file.js'), true); }); +test('isSafeGitPath rejects ".." as a whole path segment, in either separator (mutation target: only checking the first segment)', () => { + assert.equal(isSafeGitPath('..'), false); + assert.equal(isSafeGitPath('../x'), false); + assert.equal(isSafeGitPath('a/..'), false); + assert.equal(isSafeGitPath('a/../../b'), false); + assert.equal(isSafeGitPath('a/../b'), false); + assert.equal(isSafeGitPath('..\\windows\\x'), false); + assert.equal(isSafeGitPath('a\\..\\b'), false); +}); + +test('isSafeGitPath accepts ".." inside a filename — it is a name, not a traversal (mutation target: a substring test)', () => { + assert.equal(isSafeGitPath('has..dots.txt'), true); + assert.equal(isSafeGitPath('v1..v2.diff'), true); + assert.equal(isSafeGitPath('..leading.txt'), true); + assert.equal(isSafeGitPath('trailing..'), true); + assert.equal(isSafeGitPath('src/archive..2024.tar'), true); + assert.equal(isSafeNoIndexPath('has..dots.txt'), true, 'the untracked operand guard inherits the same rule'); +}); + test('isSafeGitPath rejects NUL/newline, accepts spaces and unicode', () => { assert.equal(isSafeGitPath('a\0b'), false); assert.equal(isSafeGitPath('a\nb'), false); @@ -283,6 +303,19 @@ test('local runner .diff(): a diff under the cap is not marked truncated', async // --- local runner: .diff({untracked:true}) ---------------------------------- +// The fs seam the local containment check goes through. By default every path +// is its own real path and every leaf is a regular file; `links` redirects a +// single path the way a symlink would. +function fakeFsOps({ links = {}, type = 'file' } = {}) { + return { + realpath: (p) => { + if (Object.prototype.hasOwnProperty.call(links, p)) return links[p]; + return p; + }, + lstat: () => ({ isFile: () => type === 'file', isSymbolicLink: () => type === 'symlink' }), + }; +} + const UNTRACKED_DIFF = [ 'diff --git a/new.txt b/new.txt', 'new file mode 100644', @@ -297,7 +330,7 @@ const UNTRACKED_DIFF = [ test('local runner .diff({untracked:true}): exit code 1 with a diff on stdout is SUCCESS — git diff --no-index exits 1 whenever the two inputs differ (mutation target: the usual code !== 0 check)', async () => { const exec = () => Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.ok, true, 'exit 1 from --no-index means "they differ", not "it failed"'); @@ -308,7 +341,7 @@ test('local runner .diff({untracked:true}): exit code 1 with a diff on stdout is test('local runner .diff({untracked:true}): builds a --no-index invocation against /dev/null, with -- before the two operands', async () => { const calls = []; const exec = (args) => { calls.push(args); return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); await runner.diff('new.txt', { untracked: true }); assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'new.txt']); @@ -317,7 +350,7 @@ test('local runner .diff({untracked:true}): builds a --no-index invocation again test('local runner .diff({untracked:true}): returns the added-line count the status pass could not know, with deleted 0', async () => { const exec = () => Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.added, 2); @@ -327,7 +360,7 @@ test('local runner .diff({untracked:true}): returns the added-line count the sta test('local runner .diff({untracked:true}): a binary file reports null counts and git\'s own note, not garbage', async () => { const binary = 'diff --git a/bin.dat b/bin.dat\nnew file mode 100644\nBinary files /dev/null and b/bin.dat differ\n'; const exec = () => Promise.resolve({ code: 1, stdout: binary, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); const result = await runner.diff('bin.dat', { untracked: true }); assert.equal(result.ok, true); @@ -339,7 +372,7 @@ test('local runner .diff({untracked:true}): a binary file reports null counts an test('local runner .diff({untracked:true}): a truncated diff reports null counts — a partial diff cannot be counted', async () => { const head = 'diff --git a/big.txt b/big.txt\n--- /dev/null\n+++ b/big.txt\n@@ -0,0 +1,6000 @@\n'; const exec = () => Promise.resolve({ code: 1, stdout: head + ('+' + 'a'.repeat(100) + '\n').repeat(6000), stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); const result = await runner.diff('big.txt', { untracked: true }); assert.equal(result.truncated, true); @@ -349,7 +382,7 @@ test('local runner .diff({untracked:true}): a truncated diff reports null counts test('local runner .diff({untracked:true}): any exit code other than 0 or 1 is still an error', async () => { const exec = () => Promise.resolve({ code: 128, stdout: '', stderr: 'fatal: not a git repository' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.ok, false); @@ -358,7 +391,7 @@ test('local runner .diff({untracked:true}): any exit code other than 0 or 1 is s test('local runner .diff({untracked:true}): exit 1 with NO stdout and a message on stderr is an error — that is how --no-index reports an inaccessible operand', async () => { const exec = () => Promise.resolve({ code: 1, stdout: '', stderr: "error: Could not access 'gone.txt'" }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); const result = await runner.diff('gone.txt', { untracked: true }); assert.equal(result.ok, false); @@ -368,7 +401,7 @@ test('local runner .diff({untracked:true}): exit 1 with NO stdout and a message test('local runner .diff({untracked:true}): an operand outside the working directory never reaches exec — --no-index would happily read it', async () => { let calls = 0; const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: 'SECRET', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); for (const bad of ['/etc/passwd', '../../etc/passwd', 'C:/Users/dev/.ssh/id_rsa', '--output=/tmp/pwn']) { const result = await runner.diff(bad, { untracked: true }); @@ -378,8 +411,88 @@ test('local runner .diff({untracked:true}): an operand outside the working direc assert.equal(calls, 0, 'no unsafe operand may ever reach exec'); }); +test('local runner .diff({untracked:true}): a symlinked directory inside the repo cannot be used to read outside it (mutation target: a syntax-only guard)', async () => { + let calls = 0; + const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: 'SUPER_SECRET_OUTSIDE_THE_REPO', stderr: '' }); }; + // repo/link-to-dir is a symlink to /elsewhere: the operand has no "..", is not + // absolute, and every syntactic check passes. + const fsOps = fakeFsOps({ links: { '/repo/link-to-dir': '/elsewhere' } }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('link-to-dir/outside-secret.txt', { untracked: true }); + assert.equal(isSafeNoIndexPath('link-to-dir/outside-secret.txt'), true, 'the syntactic guard alone accepts this operand'); + assert.equal(result.ok, false, 'containment must be resolved on disk, not inferred from the string'); + assert.equal(result.error, 'invalid path'); + assert.equal(calls, 0, 'git must never be handed an operand that resolves outside the working directory'); +}); + +test('local runner .diff({untracked:true}): git receives the guard\'s resolved operand, not the caller\'s string', async () => { + const calls = []; + const exec = (args) => { calls.push(args); return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); }; + // The repo is reached through a symlinked ancestor: cwd and the operand's real + // parent are spelled differently, and the operand must come out relative to the + // resolved root. + const fsOps = fakeFsOps({ links: { '/repo': '/real/repo', '/real/repo/sub': '/real/repo/sub' } }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('sub/new.txt', { untracked: true }); + assert.equal(result.ok, true); + assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'sub/new.txt']); +}); + +test('local runner .diff({untracked:true}): a leaf symlink is still diffable (git lstats it — the link target string, never the target\'s content)', async () => { + const exec = () => Promise.resolve({ code: 1, stdout: 'diff --git a/l b/l\nnew file mode 120000\n@@ -0,0 +1 @@\n+/etc/passwd\n', stderr: '' }); + const fsOps = fakeFsOps({ type: 'symlink' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('link-to-file', { untracked: true }); + assert.equal(result.ok, true, 'a symlink row that git lists must stay openable'); + assert.match(result.content, /new file mode 120000/); +}); + +test('local runner .diff({untracked:true}): an operand that is neither a file nor a symlink (fifo, device, directory) never reaches git', async () => { + let calls = 0; + const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: '', stderr: '' }); }; + const fsOps = fakeFsOps({ type: 'fifo' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('afifo', { untracked: true }); + assert.equal(result.ok, false); + assert.equal(calls, 0, 'git would block on a fifo until the timeout'); +}); + +test('local runner .diff({untracked:true}): a vanished operand is refused before git runs', async () => { + let calls = 0; + const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: '', stderr: '' }); }; + const fsOps = { + realpath: (p) => p, + lstat: () => { throw new Error('ENOENT: no such file or directory'); }, + }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('gone.txt', { untracked: true }); + assert.equal(result.ok, false); + assert.equal(calls, 0); +}); + +// --- resolveLocalNoIndexOperand: the containment helper on its own ----------- + +test('resolveLocalNoIndexOperand: returns the operand relative to the resolved root, or null when it escapes', () => { + const plain = fakeFsOps(); + assert.equal(resolveLocalNoIndexOperand('/repo', 'newdir/a.txt', plain), 'newdir/a.txt'); + assert.equal(resolveLocalNoIndexOperand('/repo', 'a.txt', plain), 'a.txt'); + assert.equal(resolveLocalNoIndexOperand('/repo', '../a.txt', plain), null); + assert.equal(resolveLocalNoIndexOperand('/repo', '/etc/passwd', plain), null); + + const escaping = fakeFsOps({ links: { '/repo/link': '/elsewhere' } }); + assert.equal(resolveLocalNoIndexOperand('/repo', 'link/secret.txt', escaping), null); + + const sibling = fakeFsOps({ links: { '/repo/link': '/repository-evil' } }); + assert.equal(resolveLocalNoIndexOperand('/repo', 'link/x.txt', sibling), null, 'a sibling sharing the root as a string prefix is not inside it'); +}); + test('local runner .diff({untracked:true}): a thrown exec rejects gracefully', async () => { - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec: () => { throw new Error('ENOENT'); } }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec: () => { throw new Error('ENOENT'); }, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.ok, false); assert.match(result.error, /ENOENT/); @@ -423,19 +536,121 @@ test('remote runner .diff(): the built command carries --literal-pathspecs, quot test('remote runner .diff({untracked:true}): every token of the --no-index command is individually quoted, and no backtick is ever emitted', async () => { const commands = []; const seenOpts = []; + const hostilePath = "weird `name`$(x).txt"; const exec = (command, opts) => { commands.push(command); seenOpts.push(opts); + if (command.includes("'ls-files'")) return Promise.resolve({ code: 0, stdout: hostilePath + '\0', stderr: '' }); return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); }; const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); - const result = await runner.diff("weird `name`$(x).txt", { untracked: true }); + const result = await runner.diff(hostilePath, { untracked: true }); assert.equal(result.ok, true); - assert.equal(commands.length, 1); - assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'diff' '--no-index' '--' '/dev/null' 'weird `name`$(x).txt'"); - assert.match(commands[0], /^git -C '.*' '--literal-pathspecs' 'diff' '--no-index' '--' '\/dev\/null' '.*'$/s, 'the backtick never sits outside a quoted token'); - assert.equal(seenOpts[0].maxStdoutBytes, DIFF_MAX_STDOUT_BYTES); + assert.equal(commands.length, 2); + assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'ls-files' '--others' '--exclude-standard' '-z' '--' 'weird `name`$(x).txt'"); + assert.equal(commands[1], "git -C '/srv/app' '--literal-pathspecs' 'diff' '--no-index' '--' '/dev/null' 'weird `name`$(x).txt'"); + for (const cmd of commands) { + assert.match(cmd, /^git -C '[^']*' ('[^']*' )*'[^']*'$/s, 'the backtick never sits outside a quoted token'); + } + assert.equal(seenOpts[1].maxStdoutBytes, DIFF_MAX_STDOUT_BYTES); +}); + +test('remote runner .diff({untracked:true}): git itself must list the path as untracked before any diff runs — no local filesystem to resolve against', async () => { + const commands = []; + const exec = (command) => { + commands.push(command); + if (command.includes("'ls-files'")) return Promise.resolve({ code: 0, stdout: 'new.txt\0', stderr: '' }); + return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff('new.txt', { untracked: true }); + + assert.equal(result.ok, true); + assert.equal(commands.length, 2, 'the listing check runs before the diff, not alongside it'); + assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'ls-files' '--others' '--exclude-standard' '-z' '--' 'new.txt'"); + assert.ok(commands[1].includes("'--no-index'")); +}); + +test('remote runner .diff({untracked:true}): a path git does not list as untracked never reaches the diff (mutation target: skipping the listing check)', async () => { + const commands = []; + const exec = (command) => { + commands.push(command); + // A path reached through a symlinked directory: git's own traversal never + // lists it, so the listing comes back empty. + if (command.includes("'ls-files'")) return Promise.resolve({ code: 0, stdout: '', stderr: '' }); + return Promise.resolve({ code: 1, stdout: 'SUPER_SECRET_OUTSIDE_THE_REPO', stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff('link-to-dir/outside-secret.txt', { untracked: true }); + + assert.equal(result.ok, false); + assert.equal(result.error, 'invalid path'); + assert.equal(commands.length, 1, 'no diff command may be sent for an unlisted path'); +}); + +test('remote runner .diff({untracked:true}): a prefix match is not a match — the listed path must be the requested one', async () => { + const exec = (command) => (command.includes("'ls-files'") + ? Promise.resolve({ code: 0, stdout: 'new.txt.bak\0', stderr: '' }) + : Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' })); + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff('new.txt', { untracked: true }); + assert.equal(result.ok, false); +}); + +test('remote runner .diff({untracked:true}): a failing listing surfaces git\'s error instead of running the diff', async () => { + const exec = (command) => (command.includes("'ls-files'") + ? Promise.resolve({ code: 128, stdout: '', stderr: 'fatal: not a git repository' }) + : Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' })); + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff('new.txt', { untracked: true }); + assert.equal(result.ok, false); + assert.match(result.error, /not a git repository/); +}); + +// --- status(): the -uall fallback ------------------------------------------ + +test('status(): when -uall overruns the transport cap, tracked changes still render, flagged as a collapsed untracked listing (mutation target: returning ok:false)', async () => { + const calls = []; + const exec = (args) => { + calls.push(args); + if (args[1] !== 'status') return Promise.resolve({ code: 0, stdout: '1\t2\tfoo.js\0', stderr: '' }); + if (args.includes('-uall')) return Promise.resolve({ code: -1, stdout: '', stderr: 'stdout exceeded 2097152 bytes' }); + return Promise.resolve({ code: 0, stdout: '# branch.head main\x001 .M N... 100644 100644 100644 abc123 def456 foo.js\x00? vendor/\x00', stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.status(); + + assert.equal(result.ok, true, 'the panel must not go dark because there are too many untracked files'); + assert.equal(result.untrackedCollapsed, true); + assert.equal(result.files.length, 2); + assert.equal(result.files[0].path, 'foo.js', 'the tracked change is still there'); + assert.equal(calls.filter((a) => a[1] === 'status').length, 2, 'exactly one retry, with git\'s default untracked mode'); +}); + +test('status(): a healthy -uall run reports untrackedCollapsed false and never retries', async () => { + const calls = []; + const exec = (args) => { + calls.push(args); + return Promise.resolve({ code: 0, stdout: args[1] === 'status' ? '# branch.head main\x00' : '', stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.status(); + + assert.equal(result.ok, true); + assert.equal(result.untrackedCollapsed, false); + assert.equal(calls.filter((a) => a[1] === 'status').length, 1); +}); + +test('status(): a genuine status failure is still an error — the fallback must not swallow it', async () => { + const exec = (args) => Promise.resolve(args[1] === 'status' + ? { code: 128, stdout: '', stderr: 'fatal: not a git repository' } + : { code: 0, stdout: '', stderr: '' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.status(); + + assert.equal(result.ok, false); + assert.match(result.error, /not a git repository/); }); // --- remote runner: stdout cap wiring (adversarial review, CRITICAL finding 1) --- From 06577eefc88ce91c5205c1fd107d02d4834978b8 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 16:47:10 +0200 Subject: [PATCH 3/5] fix(changes): close the directory-symlink path out of the working tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolving the operand's parent is not enough. Git lstats a symlink whose target is a file — that one diffs as its target string and leaks nothing — but it follows a symlink whose target is a directory, and git diff --no-index then pairs the two operands by basename: /dev/null against /null. A symlink in the working tree pointing at any directory holding a file called null reads that file, and git status -uall lists such a symlink as a row of its own, so it takes a click and no crafted path. The claim in the code, the docs and a test name was true only for a symlink to a file, and now says so. Two layers close it. The leaf gets a stat() as well as an lstat(), and a symlink resolving to anything but a regular file is refused; a dangling symlink still opens, since there is nothing to follow. And the diff itself must name the file that was asked for — every --no-index diff opens with "diff --git a/

b/

", including a binary one with no +++ line and an empty new file with no hunk, while the paired case says a/dirlink/null. That check needs no filesystem, which is what makes it work for a remote session, where ls-files legitimately lists the symlink itself. The call runs under core.quotepath=false so the comparison is against two spellings, verbatim and C-quoted, rather than against every escaping git might choose. The -uall retry is gated on the failure it was written for. It fired on any non-zero exit and always reported a collapsed listing, so a repository that could not be read told the user it had too many untracked files, and the real error was discarded. It now retries only on a stdout-cap overrun, ours or execFile's, and returns every other failure untouched. --- .ai/contexts/changes-view.md | 88 ++++++++++---- .ai/contexts/ipc-bridge.md | 2 +- git-changes-runner.js | 26 ++++- git-changes.js | 26 ++++- test/git-changes-runner-real-git.test.js | 36 +++++- test/git-changes-runner.test.js | 139 +++++++++++++++++++++-- test/git-changes.test.js | 35 +++++- 7 files changed, 312 insertions(+), 40 deletions(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 641727fa..091a5df8 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -22,6 +22,7 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). - `parseStatusPorcelainV2(text)` → `{branch:{head,upstream,ahead,behind}, files:[{path,origPath,staged,unstaged,untracked,renamed,state}]}`. Record types `1` (ordinary), `2` (rename/copy — `origPath` and `renamed:true`), `u` (unmerged), `?` (untracked, `state:'?'`). Type `!` (ignored) and any future/unrecognized record type are dropped rather than thrown on. - `parseNumstat(text)` → `{[path]: {added, deleted}}`. Binary files report `-` in git's own output; that becomes `null` here, not `0`, so a caller can tell "no lines changed" apart from "line count unknown". - `mergeChanges(status, numstatStaged, numstatUnstaged)` → the panel's model: each file gets `added`/`deleted` summed across whichever of the two numstat maps have an entry for it (a file modified in both the index and the worktree has two independent diffs; a file already staged and now edited again is a real, common case, not an edge case). An untracked file's counts stay `null` at this stage — `git diff --numstat` never reports untracked files at all, and status makes no per-file call to find out (see "Untracked files" below). `totals` sums only the known (non-null) counts. +- `diffHeaderNamesPath(content, path)` → whether a diff's `diff --git a/

b/

` first line names exactly `path`, in either the verbatim or the C-quoted spelling. The containment layer that needs no filesystem — see "Untracked files". - `countNewFileDiffAdditions(text)` → the added-line count of a new-file unified diff, or `null` when the diff is binary (`Binary files … differ`). It counts only lines *after* the first `@@` hunk header, so a file whose own content starts with `+++ ` or `@@ ` is counted like any other line — a plain "starts with `+` but not `+++`" test miscounts exactly there. - **Both parsers consume `-z` (NUL-separated) output — see "Quoting rule" below.** They walk an explicit index into `String(text).split('\0')` rather than a plain `for...of` over lines, because a rename/copy record spans TWO tokens instead of one: - **Status** (`2 ... \0\0`): the origPath is the very next token — no tab embedded in the first one the way non-`-z` porcelain v2 does it. @@ -80,12 +81,20 @@ every busy→idle edge, volume is not. At ~28 bytes per row for short paths, and untracked files. That cap is a **hard error** (`stdout exceeded …`, empty stdout), so taking it at face value would blank the whole panel — including the tracked changes, which cost nothing and are usually the reason the panel is -open. So a failing `-uall` status is retried once with git's default untracked -mode; if that succeeds the result comes back `untrackedCollapsed: true` and the -panel renders the tracked rows, the collapsed `? dir/` rows, and a note saying -the untracked listing is coarse. If the retry fails too, the original error is -returned unchanged — a broken repository is still an error, not a degraded -listing. +open. So a `-uall` status that fails **that specific way** is retried once with +git's default untracked mode; if the retry succeeds the result comes back +`untrackedCollapsed: true` and the panel renders the tracked rows, the collapsed +`? dir/` rows, and a note saying the untracked listing is coarse. + +The retry is gated on the failure signature (`isStdoutCapFailure`: the remote +transport's own `stdout exceeded bytes`, or `execFile`'s +`stdout maxBuffer length exceeded` — both non-localized, one ours and one +Node's). Any other failure returns its own error untouched: retrying on every +non-zero exit would tell a user whose repository is unreadable +(`could not read directory: Permission denied`) that they have too many +untracked files, and discard the real message on the way. If the retry itself +fails, the original error is returned unchanged — a broken repository is still +an error, not a degraded listing. The renderer holds the other end: `MAX_CHANGES_ROWS` (500) in `public/file-panel.js` caps how many rows are built, with a `+N more files not @@ -154,27 +163,56 @@ does, in two layers: - **Local** (`resolveLocalNoIndexOperand`): `fs.realpathSync.native` on the cwd and on the operand's **parent directory**, and the parent must be the resolved root or below it (`path.relative`, not a string prefix — a sibling - named `/repo-evil` shares the prefix but is not inside `/repo`). The parent, - not the leaf: git `lstat`s the operand itself, so a leaf symlink diffs as - `new file mode 120000` plus the link target *string* and leaks no content — - resolving the leaf would instead refuse a row git legitimately lists. An - `lstat` on the leaf then requires a regular file or a symlink, which also - keeps a FIFO (where `git diff --no-index` blocks until the timeout) away - from git. This is the `resolveOnDisk` + realpath-containment shape - `ipc-path-validator.js` documents, including its TOCTOU rule: what git - receives is the **guard's** operand (relative to the resolved root), never - the caller's string. - - **Remote**: there is no local filesystem to resolve against, so git's own - view of the repository is the oracle — + named `/repo-evil` shares the prefix but is not inside `/repo`). The parent + rather than the leaf, because git treats the leaf differently depending on + what it is (see the next bullet). An `lstat` on the leaf then requires a + regular file or a symlink, which also keeps a FIFO — where + `git diff --no-index` blocks until the timeout — away from git. This is the + `resolveOnDisk` + realpath-containment shape `ipc-path-validator.js` + documents, including its TOCTOU rule: what git receives is the **guard's** + operand (relative to the resolved root), never the caller's string. + - **A leaf symlink is only safe when it points at a file.** Measured: git + `lstat`s a symlink to a *file*, so the diff is `new file mode 120000` plus + the link target *string* — the target's content never appears, and the row + stays openable. Git **follows** a symlink to a *directory*, and + `--no-index` then pairs the two operands by basename, so `/dev/null` ↔ + `/null`: a symlink pointing anywhere with a file called `null` in + it reads that file. `git status -uall` lists such a symlink as a row of its + own, so this needs no crafted path — only a click. The leaf therefore gets a + `stat()` as well as an `lstat()`, and a symlink whose target is not a + regular file is refused. A dangling symlink is allowed: there is nothing for + git to follow, and it renders as its target string like any other link. + - **The diff must name the file that was asked for** (`diffHeaderNamesPath` in + `git-changes.js`) — the layer that does not need a filesystem, and therefore + the one that covers the remote transport. Every `--no-index` diff opens with + `diff --git a/ b/`, including a binary one (which has no `+++` + line at all) and an empty new file (which has neither `+++` nor a hunk). The + operand-pairing case says `a/dirlink/null b/dirlink/null` for a requested + `dirlink`, so comparing that line against the requested path catches it + wherever it happens. The call runs under `-c core.quotepath=false`, which + leaves non-ASCII verbatim, so the comparison is against two candidate + spellings — the verbatim one and git's C-quoted one (`gitQuotePath`, for a + name containing a quote, a backslash or a control character). A line + matching neither is a refusal, not an empty diff. + - **Remote**: with no local filesystem to resolve against, git's own view of + the repository is the first oracle — `git ls-files --others --exclude-standard -z -- ` must return exactly that path before any diff is sent. Measured: it lists a genuine untracked - file, and returns nothing for a path behind a symlinked directory (git's - traversal does not descend symlinks), for a tracked file, or for a FIFO. - Cost: one extra ssh round-trip per untracked row click, sequential (running - it alongside the diff would mean the far host had already read the file). - - `fsOps` (`{realpath, lstat}`) is dependency injection for tests only, the - same seam `remote-attach.js` uses for `spawnFn`; production always takes the - real fs. + file, and returns nothing for a path *behind* a symlinked directory (git's + traversal does not descend symlinks), for a tracked file, or for a FIFO. It + does list a symlink *itself*, so on this transport the header check above is + what closes the directory-symlink case. Cost: one extra ssh round-trip per + untracked row click, sequential (running it alongside the diff would mean + the far host had already read the file). + - `fsOps` (`{realpath, lstat, stat}`) is dependency injection for tests only, + the same seam `remote-attach.js` uses for `spawnFn`; production always takes + the real fs. + +A symlink row is diffed, not rendered as a `symbolic link → target` widget of +its own: `new file mode 120000` plus the target as the single added line is +git's own rendering of a symlink, this panel is a git viewer, and the two +guards above mean the only symlinks that reach git are the ones for which that +rendering is the whole truth. The untracked calls go through the same `invoke()` → `buildRemoteGitCommand`/`shQuote` path as every other command, so `/dev/null` diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 06ef38ce..8a989bde 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -157,7 +157,7 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | `add-project` / `remap-project` | none on the probe (`fs.statSync`/`fs.existsSync`/`fs.lstatSync`); the actual write is confined through `encodeProjectPath` | existence/type oracle only — inherent to the feature (both accept an arbitrary disk location by design), not cheaply fixable without breaking it | | `open-terminal` (`preLaunchCmd`) | `validatePreLaunchCmd` (`pre-launch-cmd-guard.js`) | not a path guard — a character allowlist on a raw-shell-by-design string (the documented prefix's character set plus its analogues: `env VAR=val`, `doas`, an absolute binary path); a denylist here proved incomplete (process substitution `<(...)`/`>(...)` needed none of the blocked characters), so this is closed by construction instead of by enumeration. Known cost: bare `$VAR` expansion and quoted arguments, both previously accepted, are now refused | | `read-session-jsonl` / `read-subagent-jsonl` / `start-subagent-watch` / `create-schedule-session` | none directly — path is derived from a SQLite key or built via `encodeProjectPath`, not taken verbatim from the renderer | out of scope for a path guard; flag if a renderer-controlled string is ever found reaching the derivation unencoded | -| `git-changes-diff` | `isSafeGitPath`, or `isSafeNoIndexPath` + containment when `untracked` (`git-changes-runner.js`) | a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist. The untracked variant is a real filesystem operand of `git diff --no-index`, which has no repository-boundary check of its own: on top of the syntactic guard it is resolved with `realpath` against the resolved cwd (local) or checked against `git ls-files --others` (remote), and git receives the guard's operand, not the caller's — see "Untracked files" in the same doc | +| `git-changes-diff` | `isSafeGitPath`, or `isSafeNoIndexPath` + containment when `untracked` (`git-changes-runner.js`) | a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist. The untracked variant is a real filesystem operand of `git diff --no-index`, which has no repository-boundary check of its own: on top of the syntactic guard it is resolved with `realpath`/`stat` against the resolved cwd (local) or checked against `git ls-files --others` (remote), git receives the guard's operand rather than the caller's, and the returned diff must name that same path in its `diff --git` line — see "Untracked files" in the same doc | ### Non-obvious behaviors diff --git a/git-changes-runner.js b/git-changes-runner.js index 14347b99..3039fdc4 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -6,7 +6,7 @@ const { execFile } = require('child_process'); const fs = require('fs'); const path = require('path'); const { defaultRunRemoteCommand } = require('./remote-attach'); -const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions } = require('./git-changes'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions, diffHeaderNamesPath } = require('./git-changes'); const DEFAULT_LOCAL_TIMEOUT_MS = 10_000; const DEFAULT_REMOTE_TIMEOUT_MS = 20_000; @@ -53,6 +53,7 @@ function isSafeNoIndexPath(p) { const DEFAULT_FS_OPS = { realpath: (p) => fs.realpathSync.native(p), lstat: (p) => fs.lstatSync(p), + stat: (p) => fs.statSync(p), }; function isInsideRoot(root, candidate) { @@ -61,6 +62,17 @@ function isInsideRoot(root, candidate) { return rel !== '' && rel !== '..' && !rel.startsWith('..' + path.sep) && !path.isAbsolute(rel); } +// git follows a symlink to a directory — see .ai/contexts/changes-view.md ("Untracked files") +function leafSymlinkIsDiffable(resolved, fsOps) { + let target; + try { + target = fsOps.stat(resolved); + } catch { + return true; + } + return target.isFile(); +} + // Containment for a --no-index operand — see .ai/contexts/changes-view.md ("Untracked files") function resolveLocalNoIndexOperand(cwd, filePath, fsOps = DEFAULT_FS_OPS) { if (!isSafeNoIndexPath(filePath)) return null; @@ -74,6 +86,7 @@ function resolveLocalNoIndexOperand(cwd, filePath, fsOps = DEFAULT_FS_OPS) { const resolved = path.join(parent, path.basename(absolute)); const stat = fsOps.lstat(resolved); if (!stat.isFile() && !stat.isSymbolicLink()) return null; + if (stat.isSymbolicLink() && !leafSymlinkIsDiffable(resolved, fsOps)) return null; const operand = path.relative(root, resolved); return isSafeNoIndexPath(operand) ? operand : null; @@ -138,6 +151,12 @@ function firstError(result) { return (result.stderr || '').trim() || `git exited with code ${result.code}`; } +// The two stdout-cap overruns: the remote transport's own, and execFile's maxBuffer — see .ai/contexts/changes-view.md ("Untracked files") +function isStdoutCapFailure(result) { + const stderr = (result && result.stderr) || ''; + return /stdout exceeded \d+ bytes/.test(stderr) || /maxBuffer length exceeded/i.test(stderr); +} + // {kind, cwd, alias, exec, timeoutMs, fsOps} — see .ai/contexts/changes-view.md ("Runner interface") function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs, fsOps } = {}) { if (kind !== 'local' && kind !== 'remote') { @@ -184,6 +203,7 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs, fsOps } = { // A repo too large for -uall falls back to git's collapsed listing — see .ai/contexts/changes-view.md ("Untracked files") let untrackedCollapsed = false; if (st.code !== 0) { + if (!isStdoutCapFailure(st)) return { ok: false, error: firstError(st) }; let fallback; try { fallback = await invoke(['status', '--porcelain=v2', '--branch', '-z'], { maxStdoutBytes: STATUS_MAX_STDOUT_BYTES }); @@ -228,13 +248,15 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs, fsOps } = { let result; try { - result = await invoke(['diff', '--no-index', '--', NO_INDEX_EMPTY_SIDE, contained.operand], { maxStdoutBytes: DIFF_MAX_STDOUT_BYTES }); + result = await invoke(['-c', 'core.quotepath=false', 'diff', '--no-index', '--', NO_INDEX_EMPTY_SIDE, contained.operand], + { maxStdoutBytes: DIFF_MAX_STDOUT_BYTES }); } catch (err) { return { ok: false, error: err.message }; } if (result.code !== 0 && result.code !== 1) return { ok: false, error: firstError(result) }; const stdout = result.stdout || ''; if (!stdout && (result.stderr || '').trim()) return { ok: false, error: firstError(result) }; + if (!diffHeaderNamesPath(stdout, contained.operand)) return { ok: false, error: 'invalid path' }; const { content, truncated } = truncateDiffContent(stdout, MAX_DIFF_BYTES); const added = truncated ? null : countNewFileDiffAdditions(content); diff --git a/git-changes.js b/git-changes.js index d40ce201..ece3e0f7 100644 --- a/git-changes.js +++ b/git-changes.js @@ -128,6 +128,30 @@ function countNewFileDiffAdditions(text) { return added; } +const C_QUOTE_ESCAPES = { 7: 'a', 8: 'b', 9: 't', 10: 'n', 11: 'v', 12: 'f', 13: 'r', 34: '"', 92: '\\' }; + +// git's C-style path quoting, as emitted under core.quotepath=false — see .ai/contexts/changes-view.md ("Untracked files") +function gitQuotePath(p) { + let out = '"'; + for (const ch of String(p)) { + const code = ch.codePointAt(0); + if (Object.prototype.hasOwnProperty.call(C_QUOTE_ESCAPES, code)) out += '\\' + C_QUOTE_ESCAPES[code]; + else if (code < 0x20 || code === 0x7f) out += '\\' + code.toString(8).padStart(3, '0'); + else out += ch; + } + return out + '"'; +} + +// A diff's first line names its file twice — see .ai/contexts/changes-view.md ("Untracked files") +function diffHeaderNamesPath(content, filePath) { + if (typeof filePath !== 'string' || !filePath) return false; + const first = String(content || '').split('\n', 1)[0]; + if (!first.startsWith('diff --git ')) return false; + const operands = first.slice('diff --git '.length); + return operands === `a/${filePath} b/${filePath}` + || operands === `${gitQuotePath('a/' + filePath)} ${gitQuotePath('b/' + filePath)}`; +} + function combineCounts(a, b) { if ((a && a.added === null) || (b && b.added === null)) return { added: null, deleted: null }; const added = (a ? a.added || 0 : 0) + (b ? b.added || 0 : 0); @@ -159,4 +183,4 @@ function mergeChanges(status, numstatStaged, numstatUnstaged) { }; } -module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions }; +module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions, diffHeaderNamesPath }; diff --git a/test/git-changes-runner-real-git.test.js b/test/git-changes-runner-real-git.test.js index 4e76933e..9c19d378 100644 --- a/test/git-changes-runner-real-git.test.js +++ b/test/git-changes-runner-real-git.test.js @@ -205,7 +205,41 @@ test('real git: a symlinked directory inside the repo does not open a way out } }); -test('real git: a leaf symlink stays openable and leaks nothing — git lstats it, so the diff is the link target string, not the target\'s content', async (t) => { +test('real git: a leaf symlink to a DIRECTORY leaks a file named "null" unless the guard stops it — git follows that one and pairs the --no-index operands by basename', async (t) => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + const outsideDir = path.join(tmp, 'outside'); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, 'null'), 'PAIRED_SECRET_VIA_NULL_BASENAME\n'); + try { + fs.symlinkSync(outsideDir, path.join(repoDir, 'dirlink'), 'dir'); + } catch { + t.skip('this platform does not allow creating a directory symlink unprivileged'); + return; + } + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const status = await runner.status(); + assert.ok(status.files.some((f) => f.path === 'dirlink'), 'git lists the symlink as a row of its own — this needs no crafted path, just a click'); + + // What raw git does with that row's own path, pinned. + const raw = spawnSync('git', ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'dirlink'], + { cwd: repoDir, encoding: 'utf8', env: scratchGitEnv() }); + assert.match(raw.stdout, /PAIRED_SECRET_VIA_NULL_BASENAME/, 'raw git follows the directory symlink and diffs /null'); + assert.match(raw.stdout, /^\+\+\+ b\/dirlink\/null$/m, 'and says so in the header: the path it diffed is not the path it was given'); + + const result = await runner.diff('dirlink', { untracked: true }); + assert.equal(result.ok, false); + assert.equal(result.error, 'invalid path'); + assert.ok(!String(result.content || '').includes('PAIRED_SECRET_VIA_NULL_BASENAME')); + } finally { + cleanup(tmp); + } +}); + +test('real git: a leaf symlink to a FILE stays openable and leaks nothing — git lstats that one, so the diff is the link target string, not the target\'s content', async (t) => { const tmp = mkTmp(); try { const repoDir = path.join(tmp, 'repo'); diff --git a/test/git-changes-runner.test.js b/test/git-changes-runner.test.js index ce92d8b6..778b2123 100644 --- a/test/git-changes-runner.test.js +++ b/test/git-changes-runner.test.js @@ -306,16 +306,27 @@ test('local runner .diff(): a diff under the cap is not marked truncated', async // The fs seam the local containment check goes through. By default every path // is its own real path and every leaf is a regular file; `links` redirects a // single path the way a symlink would. -function fakeFsOps({ links = {}, type = 'file' } = {}) { +// `type` is what lstat() sees; `target` is what stat() sees through a symlink +// ('missing' throws, the way a dangling link does). +function fakeFsOps({ links = {}, type = 'file', target = 'file' } = {}) { return { realpath: (p) => { if (Object.prototype.hasOwnProperty.call(links, p)) return links[p]; return p; }, lstat: () => ({ isFile: () => type === 'file', isSymbolicLink: () => type === 'symlink' }), + stat: () => { + if (target === 'missing') throw new Error('ENOENT: no such file or directory'); + return { isFile: () => target === 'file', isDirectory: () => target === 'dir' }; + }, }; } +// A --no-index diff whose header names `p`, the way real git spells it. +function untrackedDiffFor(p) { + return `diff --git a/${p} b/${p}\nnew file mode 100644\nindex 0000000..2cdcdb0\n--- /dev/null\n+++ b/${p}\n@@ -0,0 +1,2 @@\n+a1\n+a2\n`; +} + const UNTRACKED_DIFF = [ 'diff --git a/new.txt b/new.txt', 'new file mode 100644', @@ -344,7 +355,7 @@ test('local runner .diff({untracked:true}): builds a --no-index invocation again const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); await runner.diff('new.txt', { untracked: true }); - assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'new.txt']); + assert.deepEqual(calls[0], ['--literal-pathspecs', '-c', 'core.quotepath=false', 'diff', '--no-index', '--', '/dev/null', 'new.txt']); assert.ok(!calls[0].includes('--cached'), 'an untracked file has nothing in the index'); }); @@ -428,7 +439,7 @@ test('local runner .diff({untracked:true}): a symlinked directory inside the rep test('local runner .diff({untracked:true}): git receives the guard\'s resolved operand, not the caller\'s string', async () => { const calls = []; - const exec = (args) => { calls.push(args); return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); }; + const exec = (args) => { calls.push(args); return Promise.resolve({ code: 1, stdout: untrackedDiffFor('sub/new.txt'), stderr: '' }); }; // The repo is reached through a symlinked ancestor: cwd and the operand's real // parent are spelled differently, and the operand must come out relative to the // resolved root. @@ -437,12 +448,12 @@ test('local runner .diff({untracked:true}): git receives the guard\'s resolved o const result = await runner.diff('sub/new.txt', { untracked: true }); assert.equal(result.ok, true); - assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'sub/new.txt']); + assert.deepEqual(calls[0], ['--literal-pathspecs', '-c', 'core.quotepath=false', 'diff', '--no-index', '--', '/dev/null', 'sub/new.txt']); }); -test('local runner .diff({untracked:true}): a leaf symlink is still diffable (git lstats it — the link target string, never the target\'s content)', async () => { - const exec = () => Promise.resolve({ code: 1, stdout: 'diff --git a/l b/l\nnew file mode 120000\n@@ -0,0 +1 @@\n+/etc/passwd\n', stderr: '' }); - const fsOps = fakeFsOps({ type: 'symlink' }); +test('local runner .diff({untracked:true}): a leaf symlink to a FILE is still diffable (git lstats that one — the link target string, never the target\'s content)', async () => { + const exec = () => Promise.resolve({ code: 1, stdout: 'diff --git a/link-to-file b/link-to-file\nnew file mode 120000\n--- /dev/null\n+++ b/link-to-file\n@@ -0,0 +1 @@\n+/etc/passwd\n', stderr: '' }); + const fsOps = fakeFsOps({ type: 'symlink', target: 'file' }); const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); const result = await runner.diff('link-to-file', { untracked: true }); @@ -450,6 +461,86 @@ test('local runner .diff({untracked:true}): a leaf symlink is still diffable (gi assert.match(result.content, /new file mode 120000/); }); +test('local runner .diff({untracked:true}): a leaf symlink to a DIRECTORY never reaches git — it follows that one and pairs the operands by basename (mutation target: lstat alone)', async () => { + let calls = 0; + const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: untrackedDiffFor('dirlink/null'), stderr: '' }); }; + const fsOps = fakeFsOps({ type: 'symlink', target: 'dir' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('dirlink', { untracked: true }); + assert.equal(result.ok, false, 'git would diff /null, a file outside the repository'); + assert.equal(result.error, 'invalid path'); + assert.equal(calls, 0); +}); + +test('local runner .diff({untracked:true}): a dangling leaf symlink stays diffable — there is nothing for git to follow', async () => { + const exec = () => Promise.resolve({ code: 1, stdout: 'diff --git a/dangling b/dangling\nnew file mode 120000\n--- /dev/null\n+++ b/dangling\n@@ -0,0 +1 @@\n+/gone\n', stderr: '' }); + const fsOps = fakeFsOps({ type: 'symlink', target: 'missing' }); + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + + const result = await runner.diff('dangling', { untracked: true }); + assert.equal(result.ok, true); +}); + +// --- the diff header must name the file that was asked for ------------------ + +test('.diff({untracked:true}): a diff naming a path other than the one requested is refused, on either transport (mutation target: dropping the header check)', async () => { + const leak = untrackedDiffFor('dirlink/null'); + + // Local: the fs seam is made to lie about the leaf, so only the header check + // can catch the basename pairing. + const local = createGitChangesRunner({ + kind: 'local', + cwd: '/repo', + exec: () => Promise.resolve({ code: 1, stdout: leak, stderr: '' }), + fsOps: fakeFsOps({ type: 'file' }), + }); + const localResult = await local.diff('dirlink', { untracked: true }); + assert.equal(localResult.ok, false); + assert.equal(localResult.error, 'invalid path'); + + // Remote: git's own listing returns the symlink itself, so the oracle passes + // and the header check is the only thing left. + const remote = createGitChangesRunner({ + kind: 'remote', + cwd: '/srv/app', + alias: 'vps', + exec: (command) => (command.includes("'ls-files'") + ? Promise.resolve({ code: 0, stdout: 'dirlink\0', stderr: '' }) + : Promise.resolve({ code: 1, stdout: leak, stderr: '' })), + }); + const remoteResult = await remote.diff('dirlink', { untracked: true }); + assert.equal(remoteResult.ok, false, 'the remote transport has no filesystem to resolve against — the header is the check'); + assert.equal(remoteResult.error, 'invalid path'); + assert.ok(!String(remoteResult.content || '').includes('a1'), 'no content from the wrong file is returned'); +}); + +test('.diff({untracked:true}): a quoted header (a name git cannot print verbatim) still matches its own path', async () => { + const quoted = 'diff --git "a/quote\\".txt" "b/quote\\".txt"\nnew file mode 100644\n--- /dev/null\n+++ "b/quote\\".txt"\n@@ -0,0 +1 @@\n+x\n'; + const runner = createGitChangesRunner({ + kind: 'local', + cwd: '/repo', + exec: () => Promise.resolve({ code: 1, stdout: quoted, stderr: '' }), + fsOps: fakeFsOps(), + }); + const result = await runner.diff('quote".txt', { untracked: true }); + assert.equal(result.ok, true, 'a legitimately named file must not be refused by the header check'); + assert.equal(result.added, 1); +}); + +test('.diff({untracked:true}): a binary diff has no "+++" line at all and is still matched, by its "diff --git" line', async () => { + const binary = 'diff --git a/bin.dat b/bin.dat\nnew file mode 100644\nindex 0000000..c94be36\nBinary files /dev/null and b/bin.dat differ\n'; + const runner = createGitChangesRunner({ + kind: 'local', + cwd: '/repo', + exec: () => Promise.resolve({ code: 1, stdout: binary, stderr: '' }), + fsOps: fakeFsOps(), + }); + const result = await runner.diff('bin.dat', { untracked: true }); + assert.equal(result.ok, true); + assert.equal(result.added, null); +}); + test('local runner .diff({untracked:true}): an operand that is neither a file nor a symlink (fifo, device, directory) never reaches git', async () => { let calls = 0; const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: '', stderr: '' }); }; @@ -541,7 +632,7 @@ test('remote runner .diff({untracked:true}): every token of the --no-index comma commands.push(command); seenOpts.push(opts); if (command.includes("'ls-files'")) return Promise.resolve({ code: 0, stdout: hostilePath + '\0', stderr: '' }); - return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); + return Promise.resolve({ code: 1, stdout: untrackedDiffFor(hostilePath), stderr: '' }); }; const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); const result = await runner.diff(hostilePath, { untracked: true }); @@ -549,7 +640,7 @@ test('remote runner .diff({untracked:true}): every token of the --no-index comma assert.equal(result.ok, true); assert.equal(commands.length, 2); assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'ls-files' '--others' '--exclude-standard' '-z' '--' 'weird `name`$(x).txt'"); - assert.equal(commands[1], "git -C '/srv/app' '--literal-pathspecs' 'diff' '--no-index' '--' '/dev/null' 'weird `name`$(x).txt'"); + assert.equal(commands[1], "git -C '/srv/app' '--literal-pathspecs' '-c' 'core.quotepath=false' 'diff' '--no-index' '--' '/dev/null' 'weird `name`$(x).txt'"); for (const cmd of commands) { assert.match(cmd, /^git -C '[^']*' ('[^']*' )*'[^']*'$/s, 'the backtick never sits outside a quoted token'); } @@ -642,6 +733,36 @@ test('status(): a healthy -uall run reports untrackedCollapsed false and never r assert.equal(calls.filter((a) => a[1] === 'status').length, 1); }); +test('status(): a -uall failure that is not a stdout-cap overrun is reported, never relabelled as "too many untracked files" (mutation target: retrying on any non-zero exit)', async () => { + const calls = []; + const exec = (args) => { + calls.push(args); + if (args[1] !== 'status') return Promise.resolve({ code: 0, stdout: '', stderr: '' }); + if (args.includes('-uall')) return Promise.resolve({ code: 128, stdout: '', stderr: 'fatal: could not read directory: Permission denied' }); + return Promise.resolve({ code: 0, stdout: '# branch.head main\x00', stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.status(); + + assert.equal(result.ok, false, 'a permission error is not a volume problem'); + assert.match(result.error, /Permission denied/, 'the real error must not be discarded'); + assert.notEqual(result.untrackedCollapsed, true); + assert.equal(calls.filter((a) => a[1] === 'status').length, 1, 'no retry for a failure the fallback cannot help with'); +}); + +test('status(): the local maxBuffer overrun is recognised as a stdout-cap failure too, not only the remote transport\'s own message', async () => { + const exec = (args) => { + if (args[1] !== 'status') return Promise.resolve({ code: 0, stdout: '', stderr: '' }); + if (args.includes('-uall')) return Promise.resolve({ code: -1, stdout: '', stderr: 'stdout maxBuffer length exceeded' }); + return Promise.resolve({ code: 0, stdout: '# branch.head main\x00', stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const result = await runner.status(); + + assert.equal(result.ok, true); + assert.equal(result.untrackedCollapsed, true); +}); + test('status(): a genuine status failure is still an error — the fallback must not swallow it', async () => { const exec = (args) => Promise.resolve(args[1] === 'status' ? { code: 128, stdout: '', stderr: 'fatal: not a git repository' } diff --git a/test/git-changes.test.js b/test/git-changes.test.js index a08fcf02..28f75912 100644 --- a/test/git-changes.test.js +++ b/test/git-changes.test.js @@ -10,7 +10,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions } = require('../git-changes'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions, diffHeaderNamesPath } = require('../git-changes'); // --- parseStatusPorcelainV2 -------------------------------------------- @@ -291,6 +291,39 @@ test('countNewFileDiffAdditions: empty/missing input is 0', () => { assert.equal(countNewFileDiffAdditions(null), 0); }); +// --- diffHeaderNamesPath ----------------------------------------------- + +test('diffHeaderNamesPath: the first line must name the requested path, twice (mutation target: accepting any "diff --git" line)', () => { + assert.equal(diffHeaderNamesPath('diff --git a/new.txt b/new.txt\n+++ b/new.txt\n', 'new.txt'), true); + assert.equal(diffHeaderNamesPath('diff --git a/sub/new.txt b/sub/new.txt\n', 'sub/new.txt'), true); + assert.equal(diffHeaderNamesPath('diff --git a/new.txt b/other.txt\n', 'new.txt'), false); +}); + +test('diffHeaderNamesPath: a path git appended a basename to is not the path that was asked for — this is the operand-pairing leak', () => { + assert.equal(diffHeaderNamesPath('diff --git a/dirlink/null b/dirlink/null\n+++ b/dirlink/null\n', 'dirlink'), false); + assert.equal(diffHeaderNamesPath('diff --git a/dirlink/null b/dirlink/null\n', 'dirlink/null'), true, 'a file genuinely named null is still its own path'); +}); + +test('diffHeaderNamesPath: a name git prints verbatim — spaces, unicode under core.quotepath=false — matches', () => { + assert.equal(diffHeaderNamesPath('diff --git a/a space.txt b/a space.txt\n', 'a space.txt'), true); + assert.equal(diffHeaderNamesPath('diff --git a/café.txt b/café.txt\n', 'café.txt'), true); + assert.equal(diffHeaderNamesPath('diff --git a/has..dots.txt b/has..dots.txt\n', 'has..dots.txt'), true); +}); + +test('diffHeaderNamesPath: a name git C-quotes matches its quoted spelling (mutation target: comparing only the verbatim form)', () => { + assert.equal(diffHeaderNamesPath('diff --git "a/quote\\".txt" "b/quote\\".txt"\n', 'quote".txt'), true); + assert.equal(diffHeaderNamesPath('diff --git "a/tab\\there.txt" "b/tab\\there.txt"\n', 'tab\there.txt'), true); + assert.equal(diffHeaderNamesPath('diff --git "a/back\\\\slash.txt" "b/back\\\\slash.txt"\n', 'back\\slash.txt'), true); + assert.equal(diffHeaderNamesPath('diff --git "a/quote\\".txt" "b/other\\".txt"\n', 'quote".txt'), false); +}); + +test('diffHeaderNamesPath: anything that is not a diff header is a refusal, including empty output', () => { + assert.equal(diffHeaderNamesPath('', 'new.txt'), false); + assert.equal(diffHeaderNamesPath(null, 'new.txt'), false); + assert.equal(diffHeaderNamesPath('error: Could not access \'new.txt\'\n', 'new.txt'), false); + assert.equal(diffHeaderNamesPath('diff --git a/new.txt b/new.txt\n', ''), false); +}); + test('mergeChanges: branch pass-through defaults when status is missing', () => { const merged = mergeChanges(null, {}, {}); assert.deepEqual(merged.branch, { head: null, upstream: null, ahead: 0, behind: 0 }); From 16ff111f24d336c0d92c46fa6746d83b966b6fb8 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 17:11:25 +0200 Subject: [PATCH 4/5] fix(changes): make the untracked operand guard correct under Windows path rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The containment check is path arithmetic, and on Windows that arithmetic has two outcomes the POSIX-only reading missed. path.relative returns the empty string for two spellings of one directory, and the guard read that as "outside": a drive-less root like /repo against the \repo that path.resolve derives from it, a trailing separator, a different case. Every untracked file sitting directly in the repository root was refused before git was ever invoked — which is what the fifteen red tests on the windows-2022 legs were saying, all of them failing with the injected exec never called. An empty relative path now means the same directory, which is what it means. The operand handed to git was also spelled with backslashes there, while git writes forward slashes in the diff header the second containment layer compares against. The guard converts on the way out, so the requested path, the operand and the header agree on one spelling on every platform. The fixtures were wrong in the same way: "/repo" is drive-relative on Windows, so a fake realpath returning it verbatim described a directory the operand never resolved into, and the tests exercised nothing. They build their roots with path.resolve now. path.win32 and path.posix exist everywhere, so the guard takes an injectable path implementation and both flavours are asserted from whichever machine runs the suite — a legitimate path resolving, traversal and an out-of-tree symlink refused, a sibling sharing the root's prefix refused, and the leaf type rules — rather than only the one the runner happens to be on. --- .ai/contexts/changes-view.md | 47 ++++++++-- git-changes-runner.js | 24 +++-- test/git-changes-runner.test.js | 154 +++++++++++++++++++++++--------- 3 files changed, 166 insertions(+), 59 deletions(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 091a5df8..f1625531 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -118,12 +118,13 @@ Measured: `git diff --no-index -- /dev/null /dev/zero` fails with device), while `/dev/null` on the same side succeeds; and `nul`, git's Windows spelling for the same thing, is refused on Linux. Both observations match git's own `diff-no-index.c`, where the `/dev/null` string is special-cased -unconditionally and `nul` only under `GIT_WINDOWS_NATIVE`. **The Windows half of -that reasoning has never been executed** — every measurement here is from Linux; -the `windows-2022` CI leg running `test/git-changes-runner-real-git.test.js` is -the evidence, and it is worth reading before merging anything that touches this -operand. So `/dev/null` is the portable spelling, and the two alternatives are -worse: creating an empty +unconditionally and `nul` only under `GIT_WINDOWS_NATIVE`. **Git for Windows +accepts it**: `test/git-changes-runner-real-git.test.js` drives real +`git diff --no-index -- /dev/null ` invocations, it runs on the +`windows-2022` CI leg alongside Linux and macOS, and its untracked cases pass +there — so this is executed evidence on the platform in question, not an +argument from git's source. So `/dev/null` is the portable spelling, and the two +alternatives are worse: creating an empty temp file means writing into a repository under test (and cleaning it up on every error path, remote included), and `git add -N` mutates the index of a repository the user is actively working in, which a read-only viewer must never @@ -206,7 +207,39 @@ does, in two layers: the far host had already read the file). - `fsOps` (`{realpath, lstat, stat}`) is dependency injection for tests only, the same seam `remote-attach.js` uses for `spawnFn`; production always takes - the real fs. + the real fs. `resolveLocalNoIndexOperand` takes a fourth `pathOps` argument + for the same reason — see "Path arithmetic across platforms" below. + +#### Path arithmetic across platforms + +The local containment check is `path` arithmetic, and `path` means win32 rules +on the machine whose primary checkout is Windows. Three points decide whether +it works there: + +- **The operand handed to git is git-spelled.** `path.relative` returns + `newdir\a.txt` on Windows; git writes `newdir/a.txt` in every diff header it + emits, and the header check compares against it. The guard converts on the way + out (`toGitPath`), so the operand, the requested path and the header all agree + on one spelling whatever the platform. +- **An empty `path.relative` means "the same directory", not "outside".** Two + spellings of one directory — a drive-less root like `/repo` against the + `\repo` that `path.resolve` produces from it, a trailing separator, a + different case — compare unequal as strings while `path.relative` correctly + returns `''`. Reading that as an escape refuses every untracked diff whose + file sits directly in the repository root. +- **A test fixture path is platform-specific.** `/repo` is drive-relative on + Windows, so a fake `realpath` returning it verbatim describes a directory the + operand never resolves into, and the guard refuses — the tests then pass or + fail for reasons that have nothing to do with what they assert. The fixtures + build their roots with `path.resolve('/repo')`, which is `/repo` on POSIX and + `:\repo` on Windows. + +`path.win32` and `path.posix` exist on every platform, so both flavours are +injected through `pathOps` and asserted from whichever machine runs the suite +(`test/git-changes-runner.test.js`, the `PATH_FLAVOURS` loop): a legitimate +path resolves and comes back forward-slashed, traversal and an out-of-tree +symlink (another drive, on Windows) are refused, a sibling sharing the root's +string prefix is refused, and the leaf type rules hold under either separator. A symlink row is diffed, not rendered as a `symbolic link → target` widget of its own: `new file mode 120000` plus the target as the single added line is diff --git a/git-changes-runner.js b/git-changes-runner.js index 3039fdc4..b74ae247 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -56,10 +56,16 @@ const DEFAULT_FS_OPS = { stat: (p) => fs.statSync(p), }; -function isInsideRoot(root, candidate) { +function isInsideRoot(root, candidate, pathOps) { if (candidate === root) return true; - const rel = path.relative(root, candidate); - return rel !== '' && rel !== '..' && !rel.startsWith('..' + path.sep) && !path.isAbsolute(rel); + const rel = pathOps.relative(root, candidate); + if (rel === '') return true; + return rel !== '..' && !rel.startsWith('..' + pathOps.sep) && !pathOps.isAbsolute(rel); +} + +// git spells every path with forward slashes — see .ai/contexts/changes-view.md ("Untracked files") +function toGitPath(p, pathOps) { + return pathOps.sep === '/' ? p : p.split(pathOps.sep).join('/'); } // git follows a symlink to a directory — see .ai/contexts/changes-view.md ("Untracked files") @@ -74,21 +80,21 @@ function leafSymlinkIsDiffable(resolved, fsOps) { } // Containment for a --no-index operand — see .ai/contexts/changes-view.md ("Untracked files") -function resolveLocalNoIndexOperand(cwd, filePath, fsOps = DEFAULT_FS_OPS) { +function resolveLocalNoIndexOperand(cwd, filePath, fsOps = DEFAULT_FS_OPS, pathOps = path) { if (!isSafeNoIndexPath(filePath)) return null; try { const root = fsOps.realpath(cwd); - const absolute = path.resolve(root, filePath); - const parent = fsOps.realpath(path.dirname(absolute)); - if (!root || !parent || !isInsideRoot(root, parent)) return null; + const absolute = pathOps.resolve(root, filePath); + const parent = fsOps.realpath(pathOps.dirname(absolute)); + if (!root || !parent || !isInsideRoot(root, parent, pathOps)) return null; - const resolved = path.join(parent, path.basename(absolute)); + const resolved = pathOps.join(parent, pathOps.basename(absolute)); const stat = fsOps.lstat(resolved); if (!stat.isFile() && !stat.isSymbolicLink()) return null; if (stat.isSymbolicLink() && !leafSymlinkIsDiffable(resolved, fsOps)) return null; - const operand = path.relative(root, resolved); + const operand = toGitPath(pathOps.relative(root, resolved), pathOps); return isSafeNoIndexPath(operand) ? operand : null; } catch { return null; diff --git a/test/git-changes-runner.test.js b/test/git-changes-runner.test.js index 778b2123..47cf07c8 100644 --- a/test/git-changes-runner.test.js +++ b/test/git-changes-runner.test.js @@ -6,6 +6,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); +const path = require('node:path'); const { createGitChangesRunner, @@ -22,6 +23,16 @@ const { DIFF_MAX_STDOUT_BYTES, } = require('../git-changes-runner'); +// The guard resolves against the running platform's path rules, so a fixture +// cwd must be absolute FOR THAT PLATFORM: "/repo" is a drive-relative path on +// Windows, which the guard rightly refuses. REPO is "/repo" on POSIX and +// ":\repo" on Windows. +const REPO = path.resolve('/repo'); +const REAL_REPO = path.resolve('/real/repo'); +const OUTSIDE = path.resolve('/elsewhere'); +const SIBLING = path.resolve('/repository-evil'); +const inRepo = (...segments) => path.join(REPO, ...segments); + // --- shQuote / buildRemoteGitCommand --------------------------------------- test('shQuote wraps a plain value in single quotes', () => { @@ -201,7 +212,7 @@ test('local runner .status(): three commands, no -C flag (cwd passed via execFil diff: { code: 0, stdout: '1\t2\tfoo.js\x00', stderr: '' }, 'diff:cached': { code: 0, stdout: '', stderr: '' }, }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, true); @@ -217,7 +228,7 @@ test('local runner .status(): three commands, no -C flag (cwd passed via execFil test('local runner .status(): status runs with -uall so a wholly-untracked directory is listed file by file, never as one directory row (mutation target: dropping -uall)', async () => { const { exec, calls } = localFakeExec({}); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); await runner.status(); const statusArgs = calls.find((args) => args[1] === 'status'); @@ -231,14 +242,14 @@ test('local runner .status(): a failing git call surfaces stderr as the error, n const exec = (args) => Promise.resolve( args[1] === 'status' ? { code: 128, stdout: '', stderr: 'fatal: not a git repository' } : { code: 0, stdout: '', stderr: '' } ); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, false); assert.match(result.error, /not a git repository/); }); test('local runner .status(): a thrown exec rejects gracefully', async () => { - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec: () => { throw new Error('ENOENT'); } }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec: () => { throw new Error('ENOENT'); } }); const result = await runner.status(); assert.equal(result.ok, false); assert.match(result.error, /ENOENT/); @@ -249,7 +260,7 @@ test('local runner .status(): a thrown exec rejects gracefully', async () => { test('local runner .diff(): unstaged diff args carry --literal-pathspecs, refuses an unsafe path before calling exec', async () => { const calls = []; const exec = (args) => { calls.push(args); return Promise.resolve({ code: 0, stdout: 'diff --git a/x b/x\n+line\n', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const bad = await runner.diff('../escape.js'); assert.equal(bad.ok, false); @@ -263,7 +274,7 @@ test('local runner .diff(): unstaged diff args carry --literal-pathspecs, refuse test('local runner .diff({staged:true}): includes --cached', async () => { const calls = []; const exec = (args) => { calls.push(args); return Promise.resolve({ code: 0, stdout: '', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); await runner.diff('src/x.js', { staged: true }); assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--cached', '--', 'src/x.js']); }); @@ -272,7 +283,7 @@ test('local runner .diff(): truncates content past 512 KB, on a line boundary, m const line = 'a'.repeat(100) + '\n'; // 101 bytes/line, ASCII const big = line.repeat(6000); // ~600 KB, well past the 512 KB cap const exec = () => Promise.resolve({ code: 0, stdout: big, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.diff('x.js'); assert.equal(result.ok, true); assert.equal(result.truncated, true); @@ -287,7 +298,7 @@ test('local runner .diff(): the byte cap is measured in UTF-8 bytes, not JS stri const line = 'é'.repeat(100) + '\n'; // 100 chars => 201 bytes/line const big = line.repeat(4000); // ~400,000 chars / ~804,000 bytes const exec = () => Promise.resolve({ code: 0, stdout: big, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.diff('x.js'); assert.equal(result.truncated, true); assert.ok(Buffer.byteLength(result.content, 'utf8') <= MAX_DIFF_BYTES, 'a char-length cap would overshoot the byte cap here'); @@ -295,7 +306,7 @@ test('local runner .diff(): the byte cap is measured in UTF-8 bytes, not JS stri test('local runner .diff(): a diff under the cap is not marked truncated', async () => { const exec = () => Promise.resolve({ code: 0, stdout: 'small diff\n', stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.diff('x.js'); assert.equal(result.truncated, false); assert.equal(result.content, 'small diff\n'); @@ -341,7 +352,7 @@ const UNTRACKED_DIFF = [ test('local runner .diff({untracked:true}): exit code 1 with a diff on stdout is SUCCESS — git diff --no-index exits 1 whenever the two inputs differ (mutation target: the usual code !== 0 check)', async () => { const exec = () => Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.ok, true, 'exit 1 from --no-index means "they differ", not "it failed"'); @@ -352,7 +363,7 @@ test('local runner .diff({untracked:true}): exit code 1 with a diff on stdout is test('local runner .diff({untracked:true}): builds a --no-index invocation against /dev/null, with -- before the two operands', async () => { const calls = []; const exec = (args) => { calls.push(args); return Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); await runner.diff('new.txt', { untracked: true }); assert.deepEqual(calls[0], ['--literal-pathspecs', '-c', 'core.quotepath=false', 'diff', '--no-index', '--', '/dev/null', 'new.txt']); @@ -361,7 +372,7 @@ test('local runner .diff({untracked:true}): builds a --no-index invocation again test('local runner .diff({untracked:true}): returns the added-line count the status pass could not know, with deleted 0', async () => { const exec = () => Promise.resolve({ code: 1, stdout: UNTRACKED_DIFF, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.added, 2); @@ -371,7 +382,7 @@ test('local runner .diff({untracked:true}): returns the added-line count the sta test('local runner .diff({untracked:true}): a binary file reports null counts and git\'s own note, not garbage', async () => { const binary = 'diff --git a/bin.dat b/bin.dat\nnew file mode 100644\nBinary files /dev/null and b/bin.dat differ\n'; const exec = () => Promise.resolve({ code: 1, stdout: binary, stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); const result = await runner.diff('bin.dat', { untracked: true }); assert.equal(result.ok, true); @@ -383,7 +394,7 @@ test('local runner .diff({untracked:true}): a binary file reports null counts an test('local runner .diff({untracked:true}): a truncated diff reports null counts — a partial diff cannot be counted', async () => { const head = 'diff --git a/big.txt b/big.txt\n--- /dev/null\n+++ b/big.txt\n@@ -0,0 +1,6000 @@\n'; const exec = () => Promise.resolve({ code: 1, stdout: head + ('+' + 'a'.repeat(100) + '\n').repeat(6000), stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); const result = await runner.diff('big.txt', { untracked: true }); assert.equal(result.truncated, true); @@ -393,7 +404,7 @@ test('local runner .diff({untracked:true}): a truncated diff reports null counts test('local runner .diff({untracked:true}): any exit code other than 0 or 1 is still an error', async () => { const exec = () => Promise.resolve({ code: 128, stdout: '', stderr: 'fatal: not a git repository' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.ok, false); @@ -402,7 +413,7 @@ test('local runner .diff({untracked:true}): any exit code other than 0 or 1 is s test('local runner .diff({untracked:true}): exit 1 with NO stdout and a message on stderr is an error — that is how --no-index reports an inaccessible operand', async () => { const exec = () => Promise.resolve({ code: 1, stdout: '', stderr: "error: Could not access 'gone.txt'" }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); const result = await runner.diff('gone.txt', { untracked: true }); assert.equal(result.ok, false); @@ -412,7 +423,7 @@ test('local runner .diff({untracked:true}): exit 1 with NO stdout and a message test('local runner .diff({untracked:true}): an operand outside the working directory never reaches exec — --no-index would happily read it', async () => { let calls = 0; const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: 'SECRET', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps: fakeFsOps() }); for (const bad of ['/etc/passwd', '../../etc/passwd', 'C:/Users/dev/.ssh/id_rsa', '--output=/tmp/pwn']) { const result = await runner.diff(bad, { untracked: true }); @@ -427,8 +438,8 @@ test('local runner .diff({untracked:true}): a symlinked directory inside the rep const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: 'SUPER_SECRET_OUTSIDE_THE_REPO', stderr: '' }); }; // repo/link-to-dir is a symlink to /elsewhere: the operand has no "..", is not // absolute, and every syntactic check passes. - const fsOps = fakeFsOps({ links: { '/repo/link-to-dir': '/elsewhere' } }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const fsOps = fakeFsOps({ links: { [inRepo('link-to-dir')]: OUTSIDE } }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('link-to-dir/outside-secret.txt', { untracked: true }); assert.equal(isSafeNoIndexPath('link-to-dir/outside-secret.txt'), true, 'the syntactic guard alone accepts this operand'); @@ -443,8 +454,8 @@ test('local runner .diff({untracked:true}): git receives the guard\'s resolved o // The repo is reached through a symlinked ancestor: cwd and the operand's real // parent are spelled differently, and the operand must come out relative to the // resolved root. - const fsOps = fakeFsOps({ links: { '/repo': '/real/repo', '/real/repo/sub': '/real/repo/sub' } }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const fsOps = fakeFsOps({ links: { [REPO]: REAL_REPO } }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('sub/new.txt', { untracked: true }); assert.equal(result.ok, true); @@ -454,7 +465,7 @@ test('local runner .diff({untracked:true}): git receives the guard\'s resolved o test('local runner .diff({untracked:true}): a leaf symlink to a FILE is still diffable (git lstats that one — the link target string, never the target\'s content)', async () => { const exec = () => Promise.resolve({ code: 1, stdout: 'diff --git a/link-to-file b/link-to-file\nnew file mode 120000\n--- /dev/null\n+++ b/link-to-file\n@@ -0,0 +1 @@\n+/etc/passwd\n', stderr: '' }); const fsOps = fakeFsOps({ type: 'symlink', target: 'file' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('link-to-file', { untracked: true }); assert.equal(result.ok, true, 'a symlink row that git lists must stay openable'); @@ -465,7 +476,7 @@ test('local runner .diff({untracked:true}): a leaf symlink to a DIRECTORY never let calls = 0; const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: untrackedDiffFor('dirlink/null'), stderr: '' }); }; const fsOps = fakeFsOps({ type: 'symlink', target: 'dir' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('dirlink', { untracked: true }); assert.equal(result.ok, false, 'git would diff /null, a file outside the repository'); @@ -476,7 +487,7 @@ test('local runner .diff({untracked:true}): a leaf symlink to a DIRECTORY never test('local runner .diff({untracked:true}): a dangling leaf symlink stays diffable — there is nothing for git to follow', async () => { const exec = () => Promise.resolve({ code: 1, stdout: 'diff --git a/dangling b/dangling\nnew file mode 120000\n--- /dev/null\n+++ b/dangling\n@@ -0,0 +1 @@\n+/gone\n', stderr: '' }); const fsOps = fakeFsOps({ type: 'symlink', target: 'missing' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('dangling', { untracked: true }); assert.equal(result.ok, true); @@ -491,7 +502,7 @@ test('.diff({untracked:true}): a diff naming a path other than the one requested // can catch the basename pairing. const local = createGitChangesRunner({ kind: 'local', - cwd: '/repo', + cwd: REPO, exec: () => Promise.resolve({ code: 1, stdout: leak, stderr: '' }), fsOps: fakeFsOps({ type: 'file' }), }); @@ -519,7 +530,7 @@ test('.diff({untracked:true}): a quoted header (a name git cannot print verbatim const quoted = 'diff --git "a/quote\\".txt" "b/quote\\".txt"\nnew file mode 100644\n--- /dev/null\n+++ "b/quote\\".txt"\n@@ -0,0 +1 @@\n+x\n'; const runner = createGitChangesRunner({ kind: 'local', - cwd: '/repo', + cwd: REPO, exec: () => Promise.resolve({ code: 1, stdout: quoted, stderr: '' }), fsOps: fakeFsOps(), }); @@ -532,7 +543,7 @@ test('.diff({untracked:true}): a binary diff has no "+++" line at all and is sti const binary = 'diff --git a/bin.dat b/bin.dat\nnew file mode 100644\nindex 0000000..c94be36\nBinary files /dev/null and b/bin.dat differ\n'; const runner = createGitChangesRunner({ kind: 'local', - cwd: '/repo', + cwd: REPO, exec: () => Promise.resolve({ code: 1, stdout: binary, stderr: '' }), fsOps: fakeFsOps(), }); @@ -545,7 +556,7 @@ test('local runner .diff({untracked:true}): an operand that is neither a file no let calls = 0; const exec = () => { calls++; return Promise.resolve({ code: 1, stdout: '', stderr: '' }); }; const fsOps = fakeFsOps({ type: 'fifo' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('afifo', { untracked: true }); assert.equal(result.ok, false); @@ -559,7 +570,7 @@ test('local runner .diff({untracked:true}): a vanished operand is refused before realpath: (p) => p, lstat: () => { throw new Error('ENOENT: no such file or directory'); }, }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec, fsOps }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec, fsOps }); const result = await runner.diff('gone.txt', { untracked: true }); assert.equal(result.ok, false); @@ -568,22 +579,79 @@ test('local runner .diff({untracked:true}): a vanished operand is refused before // --- resolveLocalNoIndexOperand: the containment helper on its own ----------- +// Both path flavours run on both platforms: `path.win32` and `path.posix` are +// available everywhere, so the Windows drive/separator arithmetic is exercised +// from a POSIX machine and vice versa — the failure that shipped was visible +// only on Windows CI. +const PATH_FLAVOURS = [ + { name: 'win32', pathOps: path.win32, root: 'C:\\Serveur\\repo', sep: '\\', outside: 'D:\\secrets', sibling: 'C:\\Serveur\\repo-evil' }, + { name: 'posix', pathOps: path.posix, root: '/srv/repo', sep: '/', outside: '/secrets', sibling: '/srv/repo-evil' }, +]; + +for (const flavour of PATH_FLAVOURS) { + const { name, pathOps, root, sep, outside, sibling } = flavour; + + test(`resolveLocalNoIndexOperand [${name}]: a legitimate path resolves to a git-spelled operand, always forward-slashed (mutation target: returning the native separator)`, () => { + assert.equal(resolveLocalNoIndexOperand(root, 'a.txt', fakeFsOps(), pathOps), 'a.txt'); + assert.equal(resolveLocalNoIndexOperand(root, 'newdir/a.txt', fakeFsOps(), pathOps), 'newdir/a.txt'); + assert.equal(resolveLocalNoIndexOperand(root, 'deep/sub/dir/a.txt', fakeFsOps(), pathOps), 'deep/sub/dir/a.txt'); + assert.equal(resolveLocalNoIndexOperand(root, 'has..dots.txt', fakeFsOps(), pathOps), 'has..dots.txt'); + }); + + test(`resolveLocalNoIndexOperand [${name}]: traversal and escapes are refused`, () => { + assert.equal(resolveLocalNoIndexOperand(root, '../x.txt', fakeFsOps(), pathOps), null); + assert.equal(resolveLocalNoIndexOperand(root, 'sub/../../x.txt', fakeFsOps(), pathOps), null); + assert.equal(resolveLocalNoIndexOperand(root, '/etc/passwd', fakeFsOps(), pathOps), null); + + const escaping = fakeFsOps({ links: { [root + sep + 'link']: outside } }); + assert.equal(resolveLocalNoIndexOperand(root, 'link/secret.txt', escaping, pathOps), null, + 'a symlinked directory pointing out of the tree (another drive, on Windows) must not resolve inside it'); + + const nextDoor = fakeFsOps({ links: { [root + sep + 'link']: sibling } }); + assert.equal(resolveLocalNoIndexOperand(root, 'link/x.txt', nextDoor, pathOps), null, + 'a sibling sharing the root as a string prefix is not inside it'); + }); + + test(`resolveLocalNoIndexOperand [${name}]: a root that realpath spells differently from the cwd still resolves`, () => { + // The leaf's parent IS the root, reached by another spelling — path.relative + // returns '' for that, which is "the same directory", not "outside". + const spelled = fakeFsOps({ links: { [root]: root + sep + '.' } }); + assert.equal(resolveLocalNoIndexOperand(root, 'a.txt', spelled, pathOps), 'a.txt'); + }); + + test(`resolveLocalNoIndexOperand [${name}]: the leaf type rules hold whatever the separator`, () => { + assert.equal(resolveLocalNoIndexOperand(root, 'link', fakeFsOps({ type: 'symlink', target: 'file' }), pathOps), 'link'); + assert.equal(resolveLocalNoIndexOperand(root, 'dirlink', fakeFsOps({ type: 'symlink', target: 'dir' }), pathOps), null); + assert.equal(resolveLocalNoIndexOperand(root, 'afifo', fakeFsOps({ type: 'fifo' }), pathOps), null); + }); +} + +test('resolveLocalNoIndexOperand [win32]: the same directory spelled two ways is inside itself, not outside (mutation target: treating an empty path.relative as "not inside")', () => { + // The Windows CI failure in one line: "/repo" and the parent of its own + // resolved child are the same directory under two spellings, and + // path.relative says so by returning the empty string. + const parentOfChild = path.win32.dirname(path.win32.resolve('/repo', 'new.txt')); + assert.notEqual(parentOfChild, '/repo', 'win32 resolves a drive-less root to a different spelling'); + assert.equal(path.win32.relative('/repo', parentOfChild), ''); + assert.equal(resolveLocalNoIndexOperand('/repo', 'new.txt', fakeFsOps(), path.win32), 'new.txt'); +}); + test('resolveLocalNoIndexOperand: returns the operand relative to the resolved root, or null when it escapes', () => { const plain = fakeFsOps(); - assert.equal(resolveLocalNoIndexOperand('/repo', 'newdir/a.txt', plain), 'newdir/a.txt'); - assert.equal(resolveLocalNoIndexOperand('/repo', 'a.txt', plain), 'a.txt'); - assert.equal(resolveLocalNoIndexOperand('/repo', '../a.txt', plain), null); - assert.equal(resolveLocalNoIndexOperand('/repo', '/etc/passwd', plain), null); + assert.equal(resolveLocalNoIndexOperand(REPO, 'newdir/a.txt', plain), 'newdir/a.txt'); + assert.equal(resolveLocalNoIndexOperand(REPO, 'a.txt', plain), 'a.txt'); + assert.equal(resolveLocalNoIndexOperand(REPO, '../a.txt', plain), null); + assert.equal(resolveLocalNoIndexOperand(REPO, '/etc/passwd', plain), null); - const escaping = fakeFsOps({ links: { '/repo/link': '/elsewhere' } }); - assert.equal(resolveLocalNoIndexOperand('/repo', 'link/secret.txt', escaping), null); + const escaping = fakeFsOps({ links: { [inRepo('link')]: OUTSIDE } }); + assert.equal(resolveLocalNoIndexOperand(REPO, 'link/secret.txt', escaping), null); - const sibling = fakeFsOps({ links: { '/repo/link': '/repository-evil' } }); - assert.equal(resolveLocalNoIndexOperand('/repo', 'link/x.txt', sibling), null, 'a sibling sharing the root as a string prefix is not inside it'); + const sibling = fakeFsOps({ links: { [inRepo('link')]: SIBLING } }); + assert.equal(resolveLocalNoIndexOperand(REPO, 'link/x.txt', sibling), null, 'a sibling sharing the root as a string prefix is not inside it'); }); test('local runner .diff({untracked:true}): a thrown exec rejects gracefully', async () => { - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec: () => { throw new Error('ENOENT'); }, fsOps: fakeFsOps() }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec: () => { throw new Error('ENOENT'); }, fsOps: fakeFsOps() }); const result = await runner.diff('new.txt', { untracked: true }); assert.equal(result.ok, false); assert.match(result.error, /ENOENT/); @@ -709,7 +777,7 @@ test('status(): when -uall overruns the transport cap, tracked changes still ren if (args.includes('-uall')) return Promise.resolve({ code: -1, stdout: '', stderr: 'stdout exceeded 2097152 bytes' }); return Promise.resolve({ code: 0, stdout: '# branch.head main\x001 .M N... 100644 100644 100644 abc123 def456 foo.js\x00? vendor/\x00', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, true, 'the panel must not go dark because there are too many untracked files'); @@ -725,7 +793,7 @@ test('status(): a healthy -uall run reports untrackedCollapsed false and never r calls.push(args); return Promise.resolve({ code: 0, stdout: args[1] === 'status' ? '# branch.head main\x00' : '', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, true); @@ -741,7 +809,7 @@ test('status(): a -uall failure that is not a stdout-cap overrun is reported, ne if (args.includes('-uall')) return Promise.resolve({ code: 128, stdout: '', stderr: 'fatal: could not read directory: Permission denied' }); return Promise.resolve({ code: 0, stdout: '# branch.head main\x00', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, false, 'a permission error is not a volume problem'); @@ -756,7 +824,7 @@ test('status(): the local maxBuffer overrun is recognised as a stdout-cap failur if (args.includes('-uall')) return Promise.resolve({ code: -1, stdout: '', stderr: 'stdout maxBuffer length exceeded' }); return Promise.resolve({ code: 0, stdout: '# branch.head main\x00', stderr: '' }); }; - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, true); @@ -767,7 +835,7 @@ test('status(): a genuine status failure is still an error — the fallback must const exec = (args) => Promise.resolve(args[1] === 'status' ? { code: 128, stdout: '', stderr: 'fatal: not a git repository' } : { code: 0, stdout: '', stderr: '' }); - const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); + const runner = createGitChangesRunner({ kind: 'local', cwd: REPO, exec }); const result = await runner.status(); assert.equal(result.ok, false); From b14a585603fd3810954ed3fd4681d24593586248 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 17:26:55 +0200 Subject: [PATCH 5/5] docs(changes): name only the CI legs that run the suite The test matrix is ubuntu-latest and windows-2022; macos-14 appears only in build.yml, which never invokes the suite. The paragraph exists to separate executed evidence from inference, so an unsupported leg in it is the one kind of claim it must not make. --- .ai/contexts/changes-view.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index f1625531..559f06fe 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -121,7 +121,7 @@ own `diff-no-index.c`, where the `/dev/null` string is special-cased unconditionally and `nul` only under `GIT_WINDOWS_NATIVE`. **Git for Windows accepts it**: `test/git-changes-runner-real-git.test.js` drives real `git diff --no-index -- /dev/null ` invocations, it runs on the -`windows-2022` CI leg alongside Linux and macOS, and its untracked cases pass +`windows-2022` CI leg alongside Linux, and its untracked cases pass there — so this is executed evidence on the platform in question, not an argument from git's source. So `/dev/null` is the portable spelling, and the two alternatives are worse: creating an empty