diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 56b6d781..559f06fe 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -21,7 +21,9 @@ 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. +- `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. - **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 +31,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, 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. @@ -45,12 +47,254 @@ 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. -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. +One operand does not get this treatment: the filesystem path handed to +`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, 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. **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`, 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 `-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 +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 +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`. **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 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 +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, 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 + 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. 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. `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 +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` +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 + +`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 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`) `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..8a989bde 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, 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` (`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` + 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/docs/changes-view.md b/docs/changes-view.md index 746267fa..8f8a28ae 100644 --- a/docs/changes-view.md +++ b/docs/changes-view.md @@ -10,9 +10,33 @@ 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. +### 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 +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..b74ae247 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -3,8 +3,10 @@ 'use strict'; const { execFile } = require('child_process'); +const fs = require('fs'); +const path = require('path'); const { defaultRunRemoteCommand } = require('./remote-attach'); -const { parseStatusPorcelainV2, parseNumstat, mergeChanges } = 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; @@ -24,14 +26,81 @@ 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; } +// `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; +} + +// 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), + stat: (p) => fs.statSync(p), +}; + +function isInsideRoot(root, candidate, pathOps) { + if (candidate === root) return true; + 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") +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, pathOps = path) { + if (!isSafeNoIndexPath(filePath)) return null; + + try { + const root = fsOps.realpath(cwd); + const absolute = pathOps.resolve(root, filePath); + const parent = fsOps.realpath(pathOps.dirname(absolute)); + if (!root || !parent || !isInsideRoot(root, parent, pathOps)) return null; + + 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 = toGitPath(pathOps.relative(root, resolved), pathOps); + 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]; @@ -88,8 +157,14 @@ 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 } = {}) { +// 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') { throw new Error('createGitChangesRunner requires kind "local" or "remote"'); } @@ -119,28 +194,86 @@ 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 }), ]); } 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) { + 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 }); + } 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(filePath) { + const contained = await resolveUntrackedOperand(filePath); + if (!contained.ok) return { ok: false, error: contained.error }; + + let result; + try { + 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); + return { ok: true, content, truncated, added, deleted: added === null ? null : 0 }; } - async function diff(path, opts = {}) { - 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 { @@ -165,6 +298,8 @@ module.exports = { shQuote, isSafeCwd, isSafeGitPath, + isSafeNoIndexPath, + resolveLocalNoIndexOperand, MAX_DIFF_BYTES, STATUS_MAX_STDOUT_BYTES, DIFF_MAX_STDOUT_BYTES, diff --git a/git-changes.js b/git-changes.js index e753a8e6..ece3e0f7 100644 --- a/git-changes.js +++ b/git-changes.js @@ -112,6 +112,46 @@ 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; +} + +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); @@ -143,4 +183,4 @@ function mergeChanges(status, numstatStaged, numstatUnstaged) { }; } -module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges }; +module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions, diffHeaderNamesPath }; 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..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; @@ -616,7 +619,6 @@ function openChangesTab(sessionId) { diffError: null, diffContent: null, diffTruncated: false, - diffUntracked: false, }; state.panelVisible = true; @@ -661,19 +663,12 @@ 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; - } + const dataAtRequest = tab.data; 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 +679,30 @@ async function openChangesDiff(sessionId, file) { } else { tab.diffContent = result.content; tab.diffTruncated = !!result.truncated; + 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, expectedData, filePath, added, deleted) { + if (typeof added !== 'number') 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; + + 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; @@ -744,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) { @@ -820,8 +849,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/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 ec27376e..b98d18b7 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,181 @@ 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, /Untracked file/); + 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 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 { + 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, /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 f66ae658..9c19d378 100644 --- a/test/git-changes-runner-real-git.test.js +++ b/test/git-changes-runner-real-git.test.js @@ -10,15 +10,27 @@ // repo, so isSafeGitPath does not need its own absolute-path check on top of // that. This test pins that measurement so a future git/behavior change is // caught here rather than assumed. +// +// The untracked tests below pin the three measurements the untracked support +// rests on: -uall descends into a wholly-untracked directory, `git diff +// --no-index` exits 1 on a difference (success, not failure), and --no-index +// carries NO repository-containment check — see .ai/contexts/changes-view.md +// ("Untracked files"). const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const os = require('os'); const path = require('path'); -const { execFileSync } = require('child_process'); +const { execFileSync, spawnSync } = require('child_process'); -const { createGitChangesRunner } = require('../git-changes-runner'); +const { createGitChangesRunner, isSafeNoIndexPath } = require('../git-changes-runner'); + +// git translates its diagnostics; the assertions below match its English text. +// Set on this process so both the scratch-repo helper and the runner's own +// execFile child (which inherits process.env) speak the same language. +process.env.LC_ALL = 'C'; +process.env.LANGUAGE = 'C'; function mkTmp() { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'switchboard-gcr-real-')); @@ -59,6 +71,244 @@ function initRepo(repoDir) { fs.writeFileSync(path.join(repoDir, 'tracked.txt'), 'line1\nline2\n'); } +// A whole untracked tree plus a binary file, none of it ever added to the index. +function addUntrackedTree(repoDir) { + fs.mkdirSync(path.join(repoDir, 'newdir', 'sub'), { recursive: true }); + fs.writeFileSync(path.join(repoDir, 'newdir', 'a.txt'), 'a1\na2\na3\n'); + fs.writeFileSync(path.join(repoDir, 'newdir', 'sub', 'b.txt'), 'b1\n'); + fs.writeFileSync(path.join(repoDir, 'bin.dat'), Buffer.from([0, 1, 2, 0, 3, 255])); +} + +test('real git: status descends into a wholly-untracked directory — one row per file, never a single directory row (mutation target: dropping -uall)', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + addUntrackedTree(repoDir); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.status(); + assert.equal(result.ok, true); + + const paths = result.files.map((f) => f.path).sort(); + assert.deepEqual(paths, ['bin.dat', 'newdir/a.txt', 'newdir/sub/b.txt', 'tracked.txt']); + for (const f of result.files) { + assert.ok(!f.path.endsWith('/'), `no row may be a directory: ${f.path}`); + } + assert.ok(!paths.includes('newdir/'), 'git\'s default --untracked-files=normal would collapse the tree to "newdir/"'); + } finally { + cleanup(tmp); + } +}); + +test('real git: an untracked file yields a new-file diff and its added-line count — --no-index exits 1 on a difference, which is success here', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + addUntrackedTree(repoDir); + + // The exit code real git actually returns here, pinned: 1, with the diff on stdout. + const raw = spawnSync('git', ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'newdir/a.txt'], + { cwd: repoDir, encoding: 'utf8', env: scratchGitEnv() }); + assert.equal(raw.status, 1, 'git diff --no-index exits 1 when the two inputs differ'); + assert.match(raw.stdout, /\+a1/); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.diff('newdir/a.txt', { untracked: true }); + + assert.equal(result.ok, true, 'exit 1 must not be reported as a failure'); + assert.match(result.content, /^\+a1$/m); + assert.match(result.content, /^\+a3$/m); + assert.equal(result.added, 3); + assert.equal(result.deleted, 0); + assert.equal(result.truncated, false); + } finally { + cleanup(tmp); + } +}); + +test('real git: an untracked binary file shows git\'s own note and no line counts, never raw bytes', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + addUntrackedTree(repoDir); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.diff('bin.dat', { untracked: true }); + + assert.equal(result.ok, true); + assert.match(result.content, /Binary files .*differ/); + assert.ok(!result.content.includes('\x00'), 'no raw binary content may reach the renderer'); + assert.equal(result.added, null); + assert.equal(result.deleted, null); + } finally { + cleanup(tmp); + } +}); + +test('real git: --no-index has NO repository-containment check — the operand guard, not git, is what keeps an untracked diff inside the working directory', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + const secretPath = path.join(tmp, 'outside-secret.txt'); + fs.writeFileSync(secretPath, 'SUPER_SECRET_OUTSIDE_THE_REPO\n'); + + // Measured, and the whole reason isSafeNoIndexPath is stricter than + // isSafeGitPath: git happily reads an out-of-repo file under --no-index, + // where the same path as a pathspec is refused with "outside repository". + const raw = spawnSync('git', ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', secretPath], + { cwd: repoDir, encoding: 'utf8', env: scratchGitEnv() }); + assert.match(raw.stdout, /SUPER_SECRET_OUTSIDE_THE_REPO/, 'raw git --no-index reads outside the repo'); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + for (const bad of [secretPath.replace(/\\/g, '/'), '../outside-secret.txt']) { + const result = await runner.diff(bad, { untracked: true }); + assert.equal(result.ok, false, `${bad} must never reach git`); + assert.equal(result.error, 'invalid path'); + } + } finally { + cleanup(tmp); + } +}); + +test('real git: a symlinked directory inside the repo does not open a way out — the operand is syntactically innocent and still refused', async (t) => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + const secretDir = path.join(tmp, 'secrets'); + fs.mkdirSync(secretDir, { recursive: true }); + fs.writeFileSync(path.join(secretDir, 'outside-secret.txt'), 'SUPER_SECRET_OUTSIDE_THE_REPO\n'); + try { + fs.symlinkSync(secretDir, path.join(repoDir, 'link-to-dir'), 'dir'); + } catch { + t.skip('this platform does not allow creating a directory symlink unprivileged'); + return; + } + + const operand = 'link-to-dir/outside-secret.txt'; + assert.equal(isSafeNoIndexPath(operand), true, 'no "..", not absolute, no leading dash: the syntactic guard accepts it'); + + const raw = spawnSync('git', ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', operand], + { cwd: repoDir, encoding: 'utf8', env: scratchGitEnv() }); + assert.match(raw.stdout, /SUPER_SECRET_OUTSIDE_THE_REPO/, 'raw git follows the symlink and reads the file'); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.diff(operand, { untracked: true }); + assert.equal(result.ok, false, 'the resolved parent is outside the working directory'); + assert.equal(result.error, 'invalid path'); + } finally { + cleanup(tmp); + } +}); + +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'); + initRepo(repoDir); + const secretPath = path.join(tmp, 'outside-secret.txt'); + fs.writeFileSync(secretPath, 'SUPER_SECRET_OUTSIDE_THE_REPO\n'); + try { + fs.symlinkSync(secretPath, path.join(repoDir, 'link-to-file')); + } catch { + t.skip('this platform does not allow creating a symlink unprivileged'); + return; + } + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const status = await runner.status(); + assert.ok(status.files.some((f) => f.path === 'link-to-file'), 'git lists the symlink as an untracked row'); + + const result = await runner.diff('link-to-file', { untracked: true }); + assert.equal(result.ok, true, 'a row git lists must stay openable'); + assert.match(result.content, /new file mode 120000/); + assert.ok(!result.content.includes('SUPER_SECRET_OUTSIDE_THE_REPO'), 'the target\'s content never surfaces'); + } finally { + cleanup(tmp); + } +}); + +test('real git: a file whose name contains ".." is listed and opens — ".." is only a traversal as a whole path segment', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + fs.writeFileSync(path.join(repoDir, 'has..dots.txt'), 'one\ntwo\n'); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const status = await runner.status(); + assert.ok(status.files.some((f) => f.path === 'has..dots.txt'), 'git lists it'); + + const result = await runner.diff('has..dots.txt', { untracked: true }); + assert.equal(result.ok, true, 'a listed row must not refuse to open'); + assert.equal(result.added, 2); + } finally { + cleanup(tmp); + } +}); + +test('real git: an untracked path that no longer exists is an error, not an empty success', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.diff('gone.txt', { untracked: true }); + + assert.equal(result.ok, false, 'a row whose file is gone must not read as an empty diff'); + assert.equal(result.error, 'invalid path'); + + // What raw git does with the same operand, pinned: exit 1 with nothing on + // stdout — the shape the runner must not mistake for a difference. + const raw = spawnSync('git', ['--literal-pathspecs', 'diff', '--no-index', '--', '/dev/null', 'gone.txt'], + { cwd: repoDir, encoding: 'utf8', env: scratchGitEnv() }); + assert.equal(raw.status, 1); + assert.equal(raw.stdout, ''); + assert.match(raw.stderr, /Could not access/); + } finally { + cleanup(tmp); + } +}); + test('real git: an absolute pathspec outside the repo is refused by git itself — no secret content ever surfaces', async () => { const tmp = mkTmp(); try { diff --git a/test/git-changes-runner.test.js b/test/git-changes-runner.test.js index 60012adc..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, @@ -15,11 +16,23 @@ const { shQuote, isSafeCwd, isSafeGitPath, + isSafeNoIndexPath, + resolveLocalNoIndexOperand, MAX_DIFF_BYTES, STATUS_MAX_STDOUT_BYTES, 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', () => { @@ -83,6 +96,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); @@ -97,6 +129,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', () => { @@ -156,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); @@ -170,18 +226,30 @@ 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: '' } ); - 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/); @@ -192,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); @@ -206,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']); }); @@ -215,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); @@ -230,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'); @@ -238,12 +306,357 @@ 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'); }); +// --- 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. +// `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', + '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, 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"'); + 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, 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']); + 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, fsOps: fakeFsOps() }); + 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, fsOps: fakeFsOps() }); + 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, fsOps: fakeFsOps() }); + 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, fsOps: fakeFsOps() }); + 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, fsOps: fakeFsOps() }); + 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, 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 }); + 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 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: { [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'); + 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: 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. + 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); + 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 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 }); + 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}): 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: '' }); }; + 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 ----------- + +// 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); + + const escaping = fakeFsOps({ links: { [inRepo('link')]: OUTSIDE } }); + assert.equal(resolveLocalNoIndexOperand(REPO, 'link/secret.txt', escaping), null); + + 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 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 +671,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 +692,156 @@ 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 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: untrackedDiffFor(hostilePath), stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff(hostilePath, { untracked: true }); + + 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' '-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'); + } + 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 -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' } + : { 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) --- 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..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 } = require('../git-changes'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges, countNewFileDiffAdditions, diffHeaderNamesPath } = 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,98 @@ 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); +}); + +// --- 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 });