diff --git a/.ai/contexts/README.md b/.ai/contexts/README.md index cc74e52f..c91cefb2 100644 --- a/.ai/contexts/README.md +++ b/.ai/contexts/README.md @@ -1,6 +1,6 @@ # Context engineering — Switchboard -Seven sub-system docs (76 to 526 lines as of 2026-09, most have grown well past +Eight sub-system docs (76 to 526 lines as of 2026-09, most have grown well past their original size), written for AI agents who need to make a focused change without re-reading `main.js`, now ~2600 LOC. @@ -16,6 +16,7 @@ without re-reading `main.js`, now ~2600 LOC. | File-trigger watcher, harness input injection, idle-wait | [trigger-watcher](trigger-watcher.md) | | Claude CLI state files, early subagent rescan, canary tests | [cli-session-state](cli-session-state.md) | | Busy/attention/response-ready state, the session-state domain module, the icon-slot projection | [session-state](session-state.md) | +| The Changes panel: git-status parser, local/remote runner, cwd resolution, no-polling refresh | [changes-view](changes-view.md) | ## Reading order for a new contributor (~30 min) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md new file mode 100644 index 00000000..56b6d781 --- /dev/null +++ b/.ai/contexts/changes-view.md @@ -0,0 +1,99 @@ +# Context: changes-view + +**Purpose**: A read-only, git-status-sourced view of a session's working +tree, in the same right-hand file panel IDE Emulation already uses — for +local and remote sessions alike. Issue #251. User-facing behavior: +`docs/changes-view.md`. IPC names and the path-guard table entry: +`.ai/contexts/ipc-bridge.md` ("Changes panel"). The panel's tab-type +integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). + +## Key files + +| File | Role | +|---|---| +| `git-changes.js` | Pure parser — no electron, no DOM, no fs. `require()`-d from `main.js` and from tests, same pattern as `remote-hosts.js` / `derive-project-path.js`. | +| `git-changes-runner.js` | Runs the git commands, local or remote, behind one interface. | +| `git-changes-target.js` | cwd resolution for the two IPCs, extracted out of `main.js` for testability (same rationale as `delete-session-target.js`). | +| `public/file-panel.js` | Renderer: the `'changes'` tab type, its rows, and the fallback diff-line renderer. | +| `public/session-activity.js` | `onSessionIdle()` — the no-polling refresh hook. | + +## Parser (`git-changes.js`) + +- `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. +- **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`. + - Both record regexes carry the `s` (dotAll) flag so a raw, unquoted embedded newline in a path — `-z` never quotes anything, unlike the default porcelain/numstat output — still falls inside `.` instead of truncating the match. + +## 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. + +- **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. +- **`invoke(args, remoteOpts)`** is the single choke point both `status()` and `diff()` go through: it prepends `--literal-pathspecs` (`buildGitArgs`, see "Quoting rule") to every argv/command, and threads `remoteOpts.maxStdoutBytes` to the remote transport only (the local path's `execFile` `maxBuffer` already bounds it). + +### Quoting rule: literal pathspecs, `-z`, a stdout cap, and single-quote escaping — not an allowlist + +Four independent defenses, added after an adversarial review of the first cut of this panel (issue #251): + +1. **`--literal-pathspecs` on every invocation** (`buildGitArgs`, prepended before the subcommand for both the local argv and the remote command string). Without it, a path starting with `:` (`:(exclude)x`, `:/`, `:(top)`) is interpreted by git as pathspec magic even after `--`. This flag disables all such magic globally, so a path is always taken literally as a path — belt-and-suspenders with the next point, not a replacement for it (a refactor that stops calling `buildGitArgs` should not silently reopen the hole). +2. **`isSafeGitPath` also rejects a leading `:`** directly, independent of the flag above. +3. **`-z` instead of relying on `core.quotepath`** — see "Parser" above. Git's *default* porcelain v2 / numstat output C-quotes a path with embedded quotes and octal-escapes non-ASCII bytes (`café.txt` → `"caf\303\251.txt"`) whenever `core.quotepath` is on (the git default) — a user's own git config, not something this app controls. That escaped spelling does not round-trip back into a working `git diff -- ` pathspec, and status/numstat could in principle escape the same path two different ways. `-z` never quotes anything — NUL-separated records instead of LF-terminated, quoted lines — so status and numstat always correlate on the exact same path bytes, and the path handed back to `diff()` is exactly the path git will accept. +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. + +`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. + +### 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). + +`opts.spawnFn` is dependency injection for tests only (`test/remote-run-command-stdout-cap.test.js`, a fake `child_process`-shaped `EventEmitter` with `stdout`/`stderr`/`kill`) — production code never passes it, and the lazy `require('child_process')` stays the real default. + +## cwd resolution (`git-changes-target.js`) + +`resolveGitChangesTarget(sessionId, deps)`, in order: + +0. **`isValidChangesSessionId(sessionId)`** — refused before any dependency runs, including `getCachedFolder`. Accepts a plain CLI-issued id (`/^[A-Za-z0-9._-]+$/`, excluding the bare `.`/`..` — same shape and rationale as `isValidSessionId` in `delete-session-target.js`, since a local id ultimately reaches `resolveSessionRealCwd` → `path.join(projectsDir, folder, sessionId + '.jsonl')`) or a remote descriptor-only placeholder id, `pid:` (`remote-index.js` `buildPlaceholderSession` — a live descriptor with no CLI-issued session id yet is keyed on its pid instead). Everything else — a `/` or `\`, a `..` segment, a bare `sub::` subagent id — is refused: `main.js` never routes a subagent id to either `git-changes-status` or `git-changes-diff` (subagents render as a read-only transcript, not a Changes-panel-bearing session), so that shape is out of scope rather than silently accepted. +1. **Remote folder** → the host's live descriptor list (`remoteIndexer.getRemoteSessions(alias)`), matched by `sessionId`. No PTY/attach required — issue #251's acceptance criteria is "works without attaching". +2. **Local, live in this app** → the session's own recorded `.cwd` (may be a worktree) — short-circuits the disk scan below. +3. **Local, not live here** → `resolveSessionRealCwd()`, the same disk scan `open-terminal`'s resume path uses ("For a Claude resume, spawn in the session's real recorded cwd…", `main.js`), so Changes and `claude --resume` never disagree about which directory a session's cwd really is. Refused if the resolved path no longer exists on disk. + +Extracted out of the two IPC handlers into its own module, fully dependency-injected, so this order is unit-tested without booting Electron (`test/git-changes-target.test.js`) — same rationale `delete-session-target.js` and `run-schedule-now-target.js` already document for their own handlers. Step 0's whole point is to be provably reachable *before* the disk-scanning fallback (step 3): `test/git-changes-target.test.js` asserts `resolveSessionRealCwd` is never called for `"../../x"`. + +`filePath` on `git-changes-diff` is a git pathspec relative to that cwd, not an absolute filesystem path, so `ipc-path-validator.js`'s allowlist/denylist helpers (which assume an absolute path under a known root) don't fit — it's validated by the runner's own `isSafeGitPath` instead (see "Quoting rule" above). + +## Refresh triggers (no polling) + +`refreshChanges(sessionId)` in `public/file-panel.js` runs only from three places: opening the tab, the panel's own Refresh button, and a subscriber registered with `onSessionIdle()` (`public/session-activity.js`) inside `initFilePanel()`. + +`onSessionIdle(cb)` is a plain callback registry — not a DOM class writer, so it's outside the four-class enforcement `.ai/contexts/session-state.md` documents. `setActivity()` fires it only on a genuine busy→idle **edge** — `wasActive && !active`, where `wasActive` is `sessionBusyState`'s value from before this call overwrote it — never on every call where `active` is merely falsy. That distinction matters for a caller that legitimately re-asserts idle more than once with nothing busy in between: a remote row's decay/detach path calls `setActivity(id, false, via, {armReady: false})` (`.ai/contexts/session-cache.md`, "Remote hosts — busy spinner") specifically so idle does *not* arm response-ready — but that opt-out also means the pre-existing response-ready-lock dedup (an idle call is dropped early when `responseReadySessions.has(sessionId)`) never engages for it either, since that lock is only armed when `armReady` is true. Before this fix, two such `armReady:false` idle calls in a row (no intervening busy) each independently called `notifySessionIdle`, double-firing the panel's refresh. The edge check subsumes both cases: an ordinary duplicate idle already covered by the response-ready lock still reads `wasActive:false` on the second call (harmless overlap, not a conflict), and the `armReady:false` case that the lock never covered is now caught by the same line. A session with no Changes tab open is a no-op lookup (`filePanelState.get(sessionId)` misses); a session whose tab is a different type is skipped by the `currentTab.type === 'changes'` check. Proven in `test/dom-file-panel-changes.test.js`: zero `gitChangesStatus` calls while busy, one refresh per busy→idle edge (including the `armReady:false` path), and none for an unrelated session's idle transition. + +`file-panel.js` references `onSessionIdle` even though `session-activity.js` loads *after* it in `index.html` — safe because the reference lives inside `initFilePanel()`'s body, which only runs once `app.js` (the last script) calls it, by which point every script has already evaluated. Same reasoning `.ai/contexts/session-state.md` documents for `session-activity-dom.js`'s own out-of-order cross-file references. + +## Renderer: why not `ViewerPanel` for the diff + +`public/file-panel.js`'s Changes mode is a third tab type (`'changes'`), alongside the pre-existing `'file'` and `'diff'` (MCP) types, on the same per-session `filePanelState` — opening one replaces whatever the other was showing. It does not route the diff through `ViewerPanel`'s CodeMirror editor or the MCP diff tab's merge-view: both expect an old/new content pair, and a `git diff` result is a unified-diff text blob. The bundled CodeMirror also has no diff/patch language mode to color it with. The fallback is deliberately plain: one `
` per line, classed by its `+`/`-`/`@@` prefix (`classifyDiffLine()`), set via `textContent` (no HTML injection risk from diff content, which can contain arbitrary user code). + +## What's untested for remote + +The real ssh child process (`remote-attach.js`'s `defaultRunRemoteCommand` actually calling `spawn('ssh', ...)`) is exercised only by construction in `git-changes-runner.test.js` (`kind: 'remote'` with no `exec` override — asserts `runner.kind`/`runner.alias`, makes no network call), consistent with the hard rule against ssh-ing to a real host from tests. `defaultRunRemoteCommand`'s own internal logic (byte counting, the overflow kill, the resolved shape) IS unit-tested, via `opts.spawnFn` injecting a fake `child_process`-shaped `EventEmitter` (`test/remote-run-command-stdout-cap.test.js`) rather than a real `ssh` process — still no network, no real host. Everything downstream of the transport — command shape, quoting, parsing, the IPC handlers, cwd resolution — is fully tested with injected fakes. + +## Local exec and inherited git environment + +`defaultLocalExec` strips the repo-location variables (`GIT_DIR`, `GIT_WORK_TREE`, +`GIT_INDEX_FILE`, `GIT_COMMON_DIR`, `GIT_OBJECT_DIRECTORY`, `GIT_PREFIX`, +`GIT_NAMESPACE`) from the child's environment: the session's cwd is the only +thing that decides which repository a Changes command reads. Measured +2026-09-13: with those inherited (the test suite running under the pre-commit +hook), the scratch-repo test wrote `tracked.txt` into the outer repository's +index and rewrote its local `user.email`; the test helper now drops `GIT_*` / +`HUSKY*` for the scratch repo and disables its hooks, and the runner no longer +trusts them either. diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 21e6b156..b1c9a04b 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -80,6 +80,17 @@ This file is the **canonical inventory** of the IPC surface. When you add a new | `read-file-for-panel` / `save-file-for-panel` | Arbitrary file IO inside the user's projects | | `watch-file` / `unwatch-file` | fs.watch wrapper, emits `file-changed` event | +### Changes panel (issue #251) + +Read-only git-status view in the same right-hand file panel, for local and +remote sessions alike. Full design (parser, runner, quoting, cwd resolution, +refresh triggers): `.ai/contexts/changes-view.md`. User-facing: `docs/changes-view.md`. + +| 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. | + ### Misc | IPC | Notes | @@ -145,6 +156,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 | ### Non-obvious behaviors diff --git a/.ai/contexts/viewer-panel.md b/.ai/contexts/viewer-panel.md index 06c07f85..065d71a3 100644 --- a/.ai/contexts/viewer-panel.md +++ b/.ai/contexts/viewer-panel.md @@ -73,6 +73,14 @@ The toolbar factory builds all configured buttons up front; `open()` toggles vis - `public/file-panel.js` — has its own `fpViewerPanel = new ViewerPanel(...)` for the file-diff side panel; might need same opt - If you add a new file-type-aware button, mirror the `_isJsonish()` / `_isMarkdown()` pattern with an `_isXyz()` helper rather than inlining the extension check +## Changes mode (issue #251) + +`public/file-panel.js`'s side panel gained a third tab type, `'changes'`, +alongside the pre-existing `'file'` and `'diff'` (MCP) types on the same +per-session `filePanelState`. Full design (why it skips `ViewerPanel`, the +entry point, the no-polling refresh trigger): `.ai/contexts/changes-view.md`. +User-facing behavior: `docs/changes-view.md`. + ## Gotchas - **CodeMirror state holds DOM references** — calling `destroy()` then immediately `open()` on the SAME container works because `_createEditor` rebuilds it, but if you reorder this, the editor can dangle. diff --git a/.ai/shared-guidelines.md b/.ai/shared-guidelines.md index dcc19e62..4975a9a9 100644 --- a/.ai/shared-guidelines.md +++ b/.ai/shared-guidelines.md @@ -16,6 +16,7 @@ Switchboard is an **Electron desktop app**: renderer + main-process, no Domain/A | Change busy/attention/response-ready state or the session-state domain module | [contexts/session-state.md](contexts/session-state.md) | | Read the Claude CLI's own session state files | [contexts/cli-session-state.md](contexts/cli-session-state.md) | | Change Memory/.work-files panels (CodeMirror) | [contexts/viewer-panel.md](contexts/viewer-panel.md) | +| Change the Changes panel (git-status parser, local/remote runner, cwd resolution) | [contexts/changes-view.md](contexts/changes-view.md) | | Change the renderer (sidebar, terminal, app.js) | `public/*.js` — entry is `app.js` | | Write a test | `test/*.test.js` — node:test + jsdom for renderer files | | Working practices for AI agents (HANDOFF format, shell pitfalls, review loop) | [agent-practices.md](agent-practices.md) | diff --git a/docs/README.md b/docs/README.md index 2b9744dd..1f99cfa7 100644 --- a/docs/README.md +++ b/docs/README.md @@ -10,6 +10,7 @@ This is the documentation for the [devsuitup/switchboard](https://github.com/dev - [Terminal](terminal.md) — built-in terminal, right-click menu, drag-and-drop, in-terminal find - [Grid Overview](grid-overview.md) — bird's-eye live grid of all open sessions - [IDE Emulation](ide-emulation.md) — file diffs in a side panel, inline and side-by-side, partial accept +- [Changes View](changes-view.md) — read-only git-status panel, same for local and remote sessions - [Subagents](subagents.md) — subagent index, hierarchy, live status, read-only transcript viewer - [Session Restore](session-restore.md) — persist open sessions and restore them on restart - [Keyboard Shortcuts](keyboard-shortcuts.md) — editor/terminal shortcuts and rebindable session-nav keys diff --git a/docs/changes-view.md b/docs/changes-view.md new file mode 100644 index 00000000..746267fa --- /dev/null +++ b/docs/changes-view.md @@ -0,0 +1,37 @@ +# Changes View + +**Changes** is a read-only, git-status-sourced view of a session's working tree, shown in the same right-hand side panel as [IDE Emulation](ide-emulation.md)'s file/diff tabs. It exists because IDE-mode sessions never get the CLI's own `/diff` pane — Switchboard impersonates the IDE, and the IDE protocol never pushes "these files changed", only per-file diffs at permission time. A remote session shows `/diff` inside its terminal, but that view scrolls away with the session and isn't clickable from Switchboard. Changes gives both kinds the same panel. + +## Opening it + +Click the **Changes** button in the terminal header, next to the stop button. Click it again to close. + +## What it shows + +- 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. +- A **Refresh** button for a manual pull. + +## What it doesn't do + +- No staging, committing, or reverting from the UI — this is a viewer, not a git client. +- It doesn't replace the CLI's `/diff` pane in a non-IDE session; the two coexist. +- IDE mode itself is not available for remote sessions (that's a separate, larger feature — an `ssh -R` tunnel plus a lock file on the host); Changes does not depend on it and works today for both local and remote sessions. + +## How it refreshes + +Changes does not poll. It reloads: + +- The moment you open it. +- When you click Refresh. +- The moment the session goes idle (finishes a turn) while the tab is open — for a remote session this costs one ssh round-trip, typically well under a second. + +## Local vs. remote + +The same parser and the same panel render both. Only the command runner differs: + +- **Local**: `git status`/`git diff` run directly against the session's real working directory (its worktree, if it has one — the same directory a `claude --resume` targets). +- **Remote**: the same commands run over the existing ssh connection to the host, against the directory recorded in that session's descriptor. No attach, no tmux — this works even for a session you've never opened a terminal tab for. + +Diffs are capped at 512 KB; a diff larger than that is truncated with a note at the bottom. diff --git a/eslint.config.js b/eslint.config.js index 72f90618..c6537ace 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -150,6 +150,8 @@ const rendererCrossFileGlobals = { forgetActivitySeq: 'readonly', purgeActivityFor: 'readonly', pruneRemoteActivityTimers: 'readonly', + // Changes panel no-polling refresh hook (issue #251, public/file-panel.js) + onSessionIdle: 'readonly', // public/session-state.js (pure domain, see .ai/contexts/session-state.md) createSessionState: 'readonly', renderSessionIcon: 'readonly', @@ -455,6 +457,9 @@ module.exports = [ 'read-session-file.js', 'derive-project-path.js', 'encode-project-path.js', + 'git-changes.js', + 'git-changes-runner.js', + 'git-changes-target.js', 'folder-index-state.js', 'pty-size.js', 'claude-auth.js', diff --git a/git-changes-runner.js b/git-changes-runner.js new file mode 100644 index 00000000..1b6f97af --- /dev/null +++ b/git-changes-runner.js @@ -0,0 +1,171 @@ +// git-changes-runner.js — runs the git commands, local or remote — see .ai/contexts/changes-view.md + +'use strict'; + +const { execFile } = require('child_process'); +const { defaultRunRemoteCommand } = require('./remote-attach'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges } = require('./git-changes'); + +const DEFAULT_LOCAL_TIMEOUT_MS = 10_000; +const DEFAULT_REMOTE_TIMEOUT_MS = 20_000; +const MAX_DIFF_BYTES = 512 * 1024; +const LOCAL_MAX_BUFFER = 20 * 1024 * 1024; +// Remote stdout caps — see .ai/contexts/changes-view.md ("Remote transport stdout cap"). +const STATUS_MAX_STDOUT_BYTES = 2 * 1024 * 1024; +const DIFF_STDOUT_SLACK_BYTES = 64 * 1024; +const DIFF_MAX_STDOUT_BYTES = MAX_DIFF_BYTES + DIFF_STDOUT_SLACK_BYTES; + +// Denylist, not allowlist — see .ai/contexts/changes-view.md ("Quoting rule") +function isSafeShellArg(s) { + return typeof s === 'string' && s.length > 0 && s.length <= 4096 && !/[\x00\n\r]/.test(s); +} + +function isSafeCwd(cwd) { + return isSafeShellArg(cwd); +} + +// 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 (p[0] === ':') return false; + return true; +} + +// --literal-pathspecs on every invocation — see .ai/contexts/changes-view.md ("Quoting rule"). +function buildGitArgs(args) { + return ['--literal-pathspecs', ...args]; +} + +// Cut on a line boundary at or under maxBytes, measured in UTF-8 bytes — see .ai/contexts/changes-view.md ("Runner interface") +function truncateDiffContent(content, maxBytes) { + if (Buffer.byteLength(content, 'utf8') <= maxBytes) return { content, truncated: false }; + const lines = content.split('\n'); + let acc = ''; + let accBytes = 0; + for (let i = 0; i < lines.length; i++) { + const chunk = i < lines.length - 1 ? lines[i] + '\n' : lines[i]; + const chunkBytes = Buffer.byteLength(chunk, 'utf8'); + if (accBytes + chunkBytes > maxBytes) break; + acc += chunk; + accBytes += chunkBytes; + } + return { content: acc, truncated: true }; +} + +// POSIX single-quote escaping — see .ai/contexts/changes-view.md ("Quoting rule") +function shQuote(s) { + return "'" + String(s).replace(/'/g, "'\\''") + "'"; +} + +function buildRemoteGitCommand(cwd, args) { + return ['git', '-C', shQuote(cwd), ...args.map(shQuote)].join(' '); +} + +// The session's cwd is authoritative: inherited repo-location vars must not redirect git — see .ai/contexts/changes-view.md +const GIT_LOCATION_ENV = ['GIT_DIR', 'GIT_WORK_TREE', 'GIT_INDEX_FILE', 'GIT_COMMON_DIR', 'GIT_OBJECT_DIRECTORY', 'GIT_PREFIX', 'GIT_NAMESPACE']; + +function localGitEnv() { + const env = { ...process.env }; + for (const k of GIT_LOCATION_ENV) delete env[k]; + return env; +} + +function defaultLocalExec(args, { cwd, timeoutMs }) { + return new Promise((resolve) => { + execFile('git', args, { cwd, env: localGitEnv(), timeout: timeoutMs, maxBuffer: LOCAL_MAX_BUFFER, windowsHide: true }, + (err, stdout, stderr) => { + if (err) { + resolve({ code: typeof err.code === 'number' ? err.code : -1, stdout: stdout || '', stderr: stderr || err.message || String(err) }); + return; + } + resolve({ code: 0, stdout: stdout || '', stderr: stderr || '' }); + }); + }); +} + +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 } = {}) { + if (kind !== 'local' && kind !== 'remote') { + throw new Error('createGitChangesRunner requires kind "local" or "remote"'); + } + if (!isSafeCwd(cwd)) { + throw new Error('createGitChangesRunner requires a valid cwd'); + } + if (kind === 'remote' && (typeof alias !== 'string' || !alias)) { + throw new Error('createGitChangesRunner requires an alias for a remote runner'); + } + + const effectiveTimeout = timeoutMs || (kind === 'local' ? DEFAULT_LOCAL_TIMEOUT_MS : DEFAULT_REMOTE_TIMEOUT_MS); + + const runExec = exec || (kind === 'local' + ? (args) => defaultLocalExec(args, { cwd, timeoutMs: effectiveTimeout }) + : (command, remoteOpts) => defaultRunRemoteCommand(alias, command, { + timeoutMs: effectiveTimeout, + maxStdoutBytes: remoteOpts && remoteOpts.maxStdoutBytes, + })); + + // remoteOpts (maxStdoutBytes) matter only for the remote transport — see .ai/contexts/changes-view.md ("Remote transport stdout cap") + function invoke(args, remoteOpts) { + const fullArgs = buildGitArgs(args); + return kind === 'local' ? runExec(fullArgs) : runExec(buildRemoteGitCommand(cwd, fullArgs), remoteOpts); + } + + async function status() { + let results; + try { + results = await Promise.all([ + invoke(['status', '--porcelain=v2', '--branch', '-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) }; + if (unstagedNum.code !== 0) return { ok: false, error: firstError(unstagedNum) }; + if (stagedNum.code !== 0) return { ok: false, error: firstError(stagedNum) }; + + const parsedStatus = parseStatusPorcelainV2(st.stdout); + const numstatUnstaged = parseNumstat(unstagedNum.stdout); + const numstatStaged = parseNumstat(stagedNum.stdout); + return { ok: true, ...mergeChanges(parsedStatus, numstatStaged, numstatUnstaged) }; + } + + async function diff(path, opts = {}) { + if (!isSafeGitPath(path)) return { ok: false, error: 'invalid path' }; + const staged = !!opts.staged; + const args = staged ? ['diff', '--cached', '--', path] : ['diff', '--', path]; + + let result; + try { + result = await invoke(args, { maxStdoutBytes: DIFF_MAX_STDOUT_BYTES }); + } catch (err) { + return { ok: false, error: err.message }; + } + if (result.code !== 0) return { ok: false, error: firstError(result) }; + + const { content, truncated } = truncateDiffContent(result.stdout || '', MAX_DIFF_BYTES); + return { ok: true, content, truncated }; + } + + return { status, diff, kind, cwd, alias: alias || null }; +} + +module.exports = { + createGitChangesRunner, + buildRemoteGitCommand, + buildGitArgs, + truncateDiffContent, + shQuote, + isSafeCwd, + isSafeGitPath, + MAX_DIFF_BYTES, + STATUS_MAX_STDOUT_BYTES, + DIFF_MAX_STDOUT_BYTES, +}; diff --git a/git-changes-target.js b/git-changes-target.js new file mode 100644 index 00000000..9d5cb982 --- /dev/null +++ b/git-changes-target.js @@ -0,0 +1,43 @@ +// git-changes-target.js — cwd resolution for the Changes panel IPCs — see .ai/contexts/changes-view.md + +'use strict'; + +// Accepted sessionId shapes — see .ai/contexts/changes-view.md ("cwd resolution"). +const PLAIN_SESSION_ID_RE = /^[A-Za-z0-9._-]+$/; +const PLACEHOLDER_SESSION_ID_RE = /^pid:[1-9][0-9]*$/; + +function isValidChangesSessionId(id) { + if (typeof id !== 'string' || id === '') return false; + if (id === '.' || id === '..') return false; + if (PLAIN_SESSION_ID_RE.test(id)) return true; + return PLACEHOLDER_SESSION_ID_RE.test(id); +} + +// deps: {getCachedFolder, isRemoteFolder, parseFolderKey, getRemoteSessions, activeSessions, resolveSessionRealCwd, existsSync, projectsDir} +function resolveGitChangesTarget(sessionId, deps) { + const id = String(sessionId || ''); + if (!isValidChangesSessionId(id)) return { ok: false, error: 'invalid session id' }; + + let folder = null; + try { folder = deps.getCachedFolder(id); } catch {} + + if (deps.isRemoteFolder(folder)) { + const { alias } = deps.parseFolderKey(folder); + const descriptor = deps.getRemoteSessions(alias).sessions.find((s) => s.sessionId === id); + const cwd = descriptor && typeof descriptor.cwd === 'string' ? descriptor.cwd : null; + if (!cwd) return { ok: false, error: 'remote session has no known working directory' }; + return { ok: true, kind: 'remote', alias, cwd }; + } + + const session = deps.activeSessions.get(id); + if (session && !session.exited && session.cwd) { + return { ok: true, kind: 'local', cwd: session.cwd }; + } + const realCwd = deps.resolveSessionRealCwd(deps.projectsDir, id, folder); + if (realCwd && deps.existsSync(realCwd)) { + return { ok: true, kind: 'local', cwd: realCwd }; + } + return { ok: false, error: 'could not resolve a working directory for this session' }; +} + +module.exports = { resolveGitChangesTarget, isValidChangesSessionId }; diff --git a/git-changes.js b/git-changes.js new file mode 100644 index 00000000..e753a8e6 --- /dev/null +++ b/git-changes.js @@ -0,0 +1,146 @@ +// git-changes.js — pure parser for the Changes panel — see .ai/contexts/changes-view.md + +'use strict'; + +// dotAll: a raw newline inside a -z path must still match — see .ai/contexts/changes-view.md +const ORDINARY_RE = /^1 (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?.+)$/s; +const RENAME_RE = /^2 (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?.+)$/s; +const UNMERGED_RE = /^u (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?

\S+) (?

\S+) (?

\S+) (?.+)$/s; + +function makeOrdinaryFile(path, xy, renamed, origPath) { + const X = xy[0]; + const Y = xy[1]; + return { + path, + origPath: origPath || null, + staged: X !== '.', + unstaged: Y !== '.', + untracked: false, + renamed: !!renamed, + state: X !== '.' ? X : Y, + }; +} + +// Parse `git status --porcelain=v2 --branch -z` output — see .ai/contexts/changes-view.md ("Quoting rule: -z instead of core.quotepath") +function parseStatusPorcelainV2(text) { + const branch = { head: null, upstream: null, ahead: 0, behind: 0 }; + const files = []; + const tokens = String(text || '').split('\0'); + + for (let i = 0; i < tokens.length; i++) { + const raw = tokens[i]; + if (!raw) continue; // trailing empty token after the final NUL, or empty input + + if (raw.startsWith('# branch.head ')) { + const v = raw.slice('# branch.head '.length).trim(); + branch.head = v === '(detached)' ? null : v; + continue; + } + if (raw.startsWith('# branch.upstream ')) { + branch.upstream = raw.slice('# branch.upstream '.length).trim(); + continue; + } + if (raw.startsWith('# branch.ab ')) { + const m = /\+(\d+)\s+-(\d+)/.exec(raw); + if (m) { + branch.ahead = parseInt(m[1], 10); + branch.behind = parseInt(m[2], 10); + } + continue; + } + if (raw.startsWith('#')) continue; // other header lines (branch.oid, etc.) — not modeled + + if (raw.startsWith('1 ')) { + const m = ORDINARY_RE.exec(raw); + if (!m) continue; + files.push(makeOrdinaryFile(m.groups.path, m.groups.xy, false, null)); + continue; + } + if (raw.startsWith('2 ')) { + const m = RENAME_RE.exec(raw); + if (!m) continue; + // -z rename layout: path then origPath as the next NUL token — see .ai/contexts/changes-view.md + const origPath = tokens[i + 1]; + i += 1; + files.push(makeOrdinaryFile(m.groups.path, m.groups.xy, true, typeof origPath === 'string' ? origPath : null)); + continue; + } + if (raw.startsWith('u ')) { + const m = UNMERGED_RE.exec(raw); + if (!m) continue; + files.push(makeOrdinaryFile(m.groups.path, m.groups.xy, false, null)); + continue; + } + if (raw.startsWith('? ')) { + files.push({ + path: raw.slice(2), + origPath: null, + staged: false, + unstaged: false, + untracked: true, + renamed: false, + state: '?', + }); + continue; + } + } + + return { branch, files }; +} + +// Parse `git diff --numstat -z` output — see .ai/contexts/changes-view.md ("Quoting rule: -z instead of core.quotepath") +function parseNumstat(text) { + const result = {}; + const tokens = String(text || '').split('\0'); + + for (let i = 0; i < tokens.length; i++) { + const tok = tokens[i]; + if (!tok) continue; // trailing empty token, or a blank/malformed line + const m = /^(\d+|-)\t(\d+|-)\t(.*)$/s.exec(tok); + if (!m) continue; + const added = m[1] === '-' ? null : parseInt(m[1], 10); + const deleted = m[2] === '-' ? null : parseInt(m[2], 10); + if (m[3] === '') { + // -z numstat rename: empty path field, then old and new paths — see .ai/contexts/changes-view.md + const newPath = tokens[i + 2]; + i += 2; + if (typeof newPath === 'string') result[newPath] = { added, deleted }; + continue; + } + result[m[3]] = { added, deleted }; + } + return result; +} + +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); + const deleted = (a ? a.deleted || 0 : 0) + (b ? b.deleted || 0 : 0); + return { added, deleted }; +} + +// Combine status + both numstat maps into the panel's model — see .ai/contexts/changes-view.md +function mergeChanges(status, numstatStaged, numstatUnstaged) { + const staged = numstatStaged || {}; + const unstaged = numstatUnstaged || {}; + const files = (status && status.files ? status.files : []).map((f) => { + if (f.untracked) return { ...f, added: null, deleted: null }; + const counts = combineCounts(staged[f.path], unstaged[f.path]); + return { ...f, added: counts.added, deleted: counts.deleted }; + }); + + let totalAdded = 0; + let totalDeleted = 0; + for (const f of files) { + if (typeof f.added === 'number') totalAdded += f.added; + if (typeof f.deleted === 'number') totalDeleted += f.deleted; + } + + return { + branch: (status && status.branch) || { head: null, upstream: null, ahead: 0, behind: 0 }, + files, + totals: { files: files.length, added: totalAdded, deleted: totalDeleted }, + }; +} + +module.exports = { parseStatusPorcelainV2, parseNumstat, mergeChanges }; diff --git a/main.js b/main.js index 0ecc8fbd..b6d94239 100644 --- a/main.js +++ b/main.js @@ -82,6 +82,8 @@ const { handleTerminalInput } = require('./terminal-input'); const { createTriggerContext } = require('./trigger-context'); const { createTmuxAttachAdapter } = require('./remote-attach'); const { createRemoteStopAdapter } = require('./remote-stop'); +const { createGitChangesRunner } = require('./git-changes-runner'); +const gitChangesTarget = require('./git-changes-target'); setPtyOpLogger(log); @@ -1690,6 +1692,48 @@ ipcMain.handle('remote-stop-session', async (_event, payload) => { return result; }); +// --- IPC: git-changes-status / git-changes-diff — see .ai/contexts/changes-view.md --- +function resolveGitChangesTarget(sessionId) { + return gitChangesTarget.resolveGitChangesTarget(sessionId, { + getCachedFolder, + isRemoteFolder, + parseFolderKey, + getRemoteSessions: (alias) => remoteIndexer.getRemoteSessions(alias), + activeSessions, + resolveSessionRealCwd, + existsSync: (p) => fs.existsSync(p), + projectsDir: PROJECTS_DIR, + }); +} + +function gitChangesRunnerFor(target) { + return target.kind === 'remote' + ? createGitChangesRunner({ kind: 'remote', cwd: target.cwd, alias: target.alias }) + : createGitChangesRunner({ kind: 'local', cwd: target.cwd }); +} + +ipcMain.handle('git-changes-status', async (_event, sessionId) => { + const target = resolveGitChangesTarget(sessionId); + if (!target.ok) return target; + try { + return await gitChangesRunnerFor(target).status(); + } catch (err) { + return { ok: false, error: err.message }; + } +}); + +// 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) => { + 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 }); + } catch (err) { + return { ok: false, error: err.message }; + } +}); + // --- IPC: toggle-star --- ipcMain.handle('toggle-star', (_event, sessionId) => { const starred = toggleStar(sessionId); diff --git a/preload.js b/preload.js index a2ce1e3f..1a1b9df7 100644 --- a/preload.js +++ b/preload.js @@ -33,6 +33,9 @@ contextBridge.exposeInMainWorld('api', { listSubagents: (parentSessionId) => ipcRenderer.invoke('list-subagents', parentSessionId), startSubagentWatch: (parentSessionId, agentId) => ipcRenderer.invoke('start-subagent-watch', parentSessionId, agentId), 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), // Settings getSetting: (key) => ipcRenderer.invoke('get-setting', key), diff --git a/public/file-panel.js b/public/file-panel.js index 8d174fcc..9e445926 100644 --- a/public/file-panel.js +++ b/public/file-panel.js @@ -33,6 +33,13 @@ let diffBodyEl = null; let diffActionsEl = null; let diffToggleBtn = null; +// Changes-specific DOM (issue #251) +let changesContainerEl = null; +let changesSummaryEl = null; +let changesListEl = null; +let changesDiffEl = null; +let changesToggleBtn = null; + const PANEL_WIDTH_KEY = 'filePanelWidth'; const DEFAULT_PANEL_WIDTH = parseInt(localStorage.getItem(PANEL_WIDTH_KEY), 10) || 450; const MIN_PANEL_WIDTH = 280; @@ -131,12 +138,78 @@ function initFilePanel() { diffActionsEl.style.display = 'none'; diffContainer.appendChild(diffActionsEl); + // ── Changes mode (issue #251, git-status-sourced, read-only) ── + changesContainerEl = document.createElement('div'); + changesContainerEl.id = 'file-panel-changes'; + changesContainerEl.style.display = 'none'; + filePanelContentEl.appendChild(changesContainerEl); + + const changesToolbarEl = document.createElement('div'); + changesToolbarEl.className = 'viewer-toolbar'; + + const changesInfo = document.createElement('div'); + changesInfo.className = 'viewer-toolbar-info'; + const changesTitleEl = document.createElement('span'); + changesTitleEl.className = 'viewer-toolbar-title'; + changesTitleEl.textContent = 'Changes'; + const changesBranchInfoEl = document.createElement('span'); + changesBranchInfoEl.className = 'viewer-toolbar-path'; + changesBranchInfoEl.id = 'changes-branch-info'; + changesInfo.appendChild(changesTitleEl); + changesInfo.appendChild(changesBranchInfoEl); + changesToolbarEl.appendChild(changesInfo); + + const changesControls = document.createElement('div'); + changesControls.className = 'viewer-toolbar-controls'; + + const changesRefreshBtn = document.createElement('button'); + changesRefreshBtn.className = 'fp-toolbar-btn'; + changesRefreshBtn.textContent = 'Refresh'; + changesRefreshBtn.addEventListener('click', () => { + if (currentPanelSessionId) refreshChanges(currentPanelSessionId); + }); + changesControls.appendChild(changesRefreshBtn); + + const changesCloseBtn = document.createElement('button'); + changesCloseBtn.className = 'fp-toolbar-btn fp-close-btn fp-icon-btn'; + changesCloseBtn.innerHTML = ''; + changesCloseBtn.title = 'Close panel'; + changesCloseBtn.addEventListener('click', handleClose); + changesControls.appendChild(changesCloseBtn); + + changesToolbarEl.appendChild(changesControls); + changesContainerEl.appendChild(changesToolbarEl); + + changesSummaryEl = document.createElement('div'); + changesSummaryEl.id = 'changes-summary'; + changesContainerEl.appendChild(changesSummaryEl); + + changesListEl = document.createElement('div'); + changesListEl.id = 'changes-list'; + changesContainerEl.appendChild(changesListEl); + + changesDiffEl = document.createElement('div'); + changesDiffEl.id = 'changes-diff-view'; + changesDiffEl.style.display = 'none'; + changesContainerEl.appendChild(changesDiffEl); + terminalSplitEl.appendChild(filePanelEl); terminalArea.appendChild(terminalSplitEl); wireIpcListeners(); setupPanelResizeHandle(); addMcpToggle(); + addChangesToggle(); + + // see .ai/contexts/changes-view.md ("Refresh triggers") + if (typeof onSessionIdle === 'function') { + onSessionIdle((sessionId) => { + const state = filePanelState.get(sessionId); + if (state && state.currentTab && state.currentTab.type === 'changes') { + refreshChanges(sessionId); + } + }); + } } // ── Handlers ──────────────────────────────────────────────────────── @@ -408,17 +481,25 @@ function renderTabContent(sessionId, tab) { if (!tab) { vpContainer.style.display = 'none'; diffContainer.style.display = 'none'; + changesContainerEl.style.display = 'none'; return; } if (tab.type === 'file') { // Use ViewerPanel diffContainer.style.display = 'none'; + changesContainerEl.style.display = 'none'; vpContainer.style.display = 'flex'; fpViewerPanel.open(tab.label, tab.filePath, tab.content); + } else if (tab.type === 'changes') { + vpContainer.style.display = 'none'; + diffContainer.style.display = 'none'; + changesContainerEl.style.display = 'flex'; + renderChangesContent(sessionId, tab); } else { - // Diff mode + // MCP diff mode vpContainer.style.display = 'none'; + changesContainerEl.style.display = 'none'; diffContainer.style.display = 'flex'; renderDiffContent(sessionId, tab); } @@ -507,6 +588,275 @@ function handleDiffAction(sessionId, tab, action) { diffActionsEl.style.display = 'none'; } +// ── Changes Mode — see .ai/contexts/changes-view.md ────────────────── + +function toggleChangesTab(sessionId) { + const state = getSessionState(sessionId); + if (state.currentTab && state.currentTab.type === 'changes') { + destroyCurrentTab(state); + state.currentTab = null; + state.panelVisible = false; + if (currentPanelSessionId === sessionId) hidePanel(); + return; + } + return openChangesTab(sessionId); +} + +function openChangesTab(sessionId) { + const state = getSessionState(sessionId); + destroyCurrentTab(state); + state.currentTab = { + type: 'changes', + label: 'Changes', + loading: true, + error: null, + data: null, + selectedFile: null, + diffLoading: false, + diffError: null, + diffContent: null, + diffTruncated: false, + diffUntracked: false, + }; + state.panelVisible = true; + + if (currentPanelSessionId === sessionId) { + showPanel(state); + renderPanel(sessionId); + } + return refreshChanges(sessionId); +} + +async function refreshChanges(sessionId) { + const state = filePanelState.get(sessionId); + if (!state || !state.currentTab || state.currentTab.type !== 'changes') return; + const tab = state.currentTab; + + tab.loading = true; + if (currentPanelSessionId === sessionId) renderPanel(sessionId); + + const result = await window.api.gitChangesStatus(sessionId); + + // tab may have been closed/replaced while the IPC round-trip was in flight + const stillState = filePanelState.get(sessionId); + if (!stillState || stillState.currentTab !== tab) return; + + tab.loading = false; + if (!result || result.ok === false) { + tab.error = (result && result.error) || 'failed to load changes'; + tab.data = null; + } else { + tab.error = null; + tab.data = result; + } + if (currentPanelSessionId === sessionId) renderPanel(sessionId); +} + +async function openChangesDiff(sessionId, file) { + const state = filePanelState.get(sessionId); + if (!state || !state.currentTab || state.currentTab.type !== 'changes') return; + const tab = state.currentTab; + + tab.selectedFile = file; + tab.diffError = null; + tab.diffContent = null; + tab.diffTruncated = false; + tab.diffUntracked = !!file.untracked; + + if (file.untracked) { + // git diff never reports an untracked file — nothing to fetch. + tab.diffLoading = false; + if (currentPanelSessionId === sessionId) renderPanel(sessionId); + return; + } + + tab.diffLoading = true; + if (currentPanelSessionId === sessionId) renderPanel(sessionId); + + const result = await window.api.gitChangesDiff(sessionId, file.path, file.staged); + + const stillState = filePanelState.get(sessionId); + if (!stillState || stillState.currentTab !== tab || tab.selectedFile !== file) return; + + tab.diffLoading = false; + if (!result || result.ok === false) { + tab.diffError = (result && result.error) || 'failed to load diff'; + } else { + tab.diffContent = result.content; + tab.diffTruncated = !!result.truncated; + } + if (currentPanelSessionId === sessionId) renderPanel(sessionId); +} + +function closeChangesDiff(sessionId) { + const state = filePanelState.get(sessionId); + if (!state || !state.currentTab || state.currentTab.type !== 'changes') return; + state.currentTab.selectedFile = null; + state.currentTab.diffContent = null; + state.currentTab.diffError = null; + if (currentPanelSessionId === sessionId) renderPanel(sessionId); +} + +function renderChangesContent(sessionId, tab) { + if (tab.selectedFile) { + changesSummaryEl.style.display = 'none'; + changesListEl.style.display = 'none'; + changesDiffEl.style.display = 'flex'; + renderChangesDiff(sessionId, tab); + return; + } + changesDiffEl.style.display = 'none'; + changesSummaryEl.style.display = 'block'; + changesListEl.style.display = 'block'; + + const branchInfoEl = document.getElementById('changes-branch-info'); + + if (tab.loading && !tab.data) { + changesSummaryEl.textContent = 'Loading changes…'; + changesListEl.innerHTML = ''; + if (branchInfoEl) branchInfoEl.textContent = ''; + return; + } + if (tab.error) { + changesSummaryEl.textContent = ''; + changesListEl.innerHTML = ''; + const err = document.createElement('div'); + err.className = 'changes-error'; + err.textContent = tab.error; + changesListEl.appendChild(err); + if (branchInfoEl) branchInfoEl.textContent = ''; + return; + } + + const data = tab.data; + if (!data) return; + const { branch, files, totals } = data; + + changesSummaryEl.textContent = totals.files === 0 + ? 'No changes' + : `${totals.files} file${totals.files === 1 ? '' : 's'} changed +${totals.added} −${totals.deleted}`; + + if (branchInfoEl) { + const parts = []; + if (branch.head) parts.push(branch.head); + if (branch.ahead) parts.push('↑' + branch.ahead); + if (branch.behind) parts.push('↓' + branch.behind); + branchInfoEl.textContent = parts.join(' '); + } + + changesListEl.innerHTML = ''; + for (const file of files) { + changesListEl.appendChild(buildChangesFileRow(sessionId, file)); + } +} + +function buildChangesFileRow(sessionId, file) { + const row = document.createElement('div'); + row.className = 'changes-file-row'; + row.dataset.path = file.path; + + const state = document.createElement('span'); + state.className = 'changes-file-state changes-state-' + (file.state || '?').toLowerCase(); + state.textContent = file.state || '?'; + row.appendChild(state); + + const pathEl = document.createElement('span'); + pathEl.className = 'changes-file-path'; + pathEl.textContent = (file.renamed && file.origPath) ? `${file.origPath} → ${file.path}` : file.path; + row.appendChild(pathEl); + + if (typeof file.added === 'number' || typeof file.deleted === 'number') { + const counts = document.createElement('span'); + counts.className = 'changes-file-counts'; + const added = document.createElement('span'); + added.className = 'changes-added'; + added.textContent = '+' + (file.added || 0); + const deleted = document.createElement('span'); + deleted.className = 'changes-deleted'; + deleted.textContent = '−' + (file.deleted || 0); + counts.appendChild(added); + counts.appendChild(deleted); + row.appendChild(counts); + } + + row.addEventListener('click', () => { + // prefer the unstaged (worktree) diff when a file has both + const staged = !!file.staged && !file.unstaged; + openChangesDiff(sessionId, { path: file.path, staged, untracked: !!file.untracked }); + }); + return row; +} + +function renderChangesDiff(sessionId, tab) { + changesDiffEl.innerHTML = ''; + + const header = document.createElement('div'); + header.className = 'viewer-toolbar'; + + const info = document.createElement('div'); + info.className = 'viewer-toolbar-info'; + const titleEl = document.createElement('span'); + titleEl.className = 'viewer-toolbar-title'; + titleEl.textContent = tab.selectedFile.path; + info.appendChild(titleEl); + header.appendChild(info); + + const controls = document.createElement('div'); + controls.className = 'viewer-toolbar-controls'; + const backBtn = document.createElement('button'); + backBtn.className = 'fp-toolbar-btn'; + backBtn.textContent = 'Back'; + backBtn.addEventListener('click', () => closeChangesDiff(sessionId)); + controls.appendChild(backBtn); + header.appendChild(controls); + + changesDiffEl.appendChild(header); + + const body = document.createElement('pre'); + body.className = 'changes-diff-body'; + + if (tab.diffLoading) { + body.textContent = 'Loading diff…'; + } 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 { + renderDiffLines(body, tab.diffContent); + } + changesDiffEl.appendChild(body); + + if (tab.diffTruncated) { + const note = document.createElement('div'); + note.className = 'changes-diff-truncated'; + note.textContent = 'Diff truncated at 512 KB.'; + changesDiffEl.appendChild(note); + } +} + +// see .ai/contexts/changes-view.md ("why not ViewerPanel for the diff") +function renderDiffLines(container, text) { + const frag = document.createDocumentFragment(); + for (const line of text.split('\n')) { + const div = document.createElement('div'); + div.className = 'changes-diff-line ' + classifyDiffLine(line); + div.textContent = line; + frag.appendChild(div); + } + container.appendChild(frag); +} + +function classifyDiffLine(line) { + if (line.startsWith('+++') || line.startsWith('---')) return 'changes-diff-file-header'; + if (line.startsWith('@@')) return 'changes-diff-hunk'; + if (line.startsWith('+')) return 'changes-diff-add'; + if (line.startsWith('-')) return 'changes-diff-del'; + return 'changes-diff-ctx'; +} + // ── IDE Emulation Indicator ───────────────────────────────────────── let mcpIndicatorEl = null; @@ -529,6 +879,28 @@ function addMcpToggle() { } } +// Terminal header entry point for Changes mode — see .ai/contexts/changes-view.md +function addChangesToggle() { + const controls = document.getElementById('terminal-header-controls'); + if (!controls) return; + + changesToggleBtn = document.createElement('button'); + changesToggleBtn.id = 'changes-toggle-btn'; + changesToggleBtn.className = 'fp-toolbar-btn'; + changesToggleBtn.textContent = 'Changes'; + changesToggleBtn.title = 'Show working tree changes for this session'; + changesToggleBtn.addEventListener('click', () => { + if (currentPanelSessionId) toggleChangesTab(currentPanelSessionId); + }); + + const stopBtn = document.getElementById('terminal-stop-btn'); + if (stopBtn) { + controls.insertBefore(changesToggleBtn, stopBtn); + } else { + controls.appendChild(changesToggleBtn); + } +} + // ── Resize Handle ─────────────────────────────────────────────────── function setupPanelResizeHandle() { diff --git a/public/session-activity.js b/public/session-activity.js index ec8e271c..7d2c68e7 100644 --- a/public/session-activity.js +++ b/public/session-activity.js @@ -7,6 +7,18 @@ const attentionSessions = new Set(); // sessions needing user action (OSC 9) const responseReadySessions = new Set(); // Claude finished, user hasn't looked (terminal state) const sessionBusyState = new Map(); // sessionId → boolean (currently active) +// see .ai/contexts/changes-view.md ("Refresh triggers") +const idleListeners = new Set(); +function onSessionIdle(cb) { + idleListeners.add(cb); + return () => idleListeners.delete(cb); +} +function notifySessionIdle(sessionId) { + for (const cb of idleListeners) { + try { cb(sessionId); } catch {} + } +} + // Monotonic transition counter, plus its value at each session's last change. let activitySeq = 0; const activitySeqBySession = new Map(); @@ -57,6 +69,8 @@ function setActivity(sessionId, active, via, opts) { } applyActivityClasses(sessionId); + // Fire only on a genuine busy->idle edge — see .ai/contexts/changes-view.md ("Refresh triggers"). + if (wasActive && !active) notifySessionIdle(sessionId); } function clearUnread(sessionId, via) { diff --git a/public/style.css b/public/style.css index a05f7690..71334b16 100644 --- a/public/style.css +++ b/public/style.css @@ -4469,6 +4469,128 @@ body { display: flex; flex-direction: column; } vertical-align: middle; } +/* --- Changes mode (issue #251) --- */ +#file-panel-changes { + flex: 1; + display: flex; + flex-direction: column; + min-height: 0; +} + +#changes-summary { + padding: 8px 12px; + font-size: 12px; + color: #c0c0d0; + border-bottom: 1px solid rgba(255,255,255,0.06); +} + +#changes-list { + flex: 1; + overflow-y: auto; +} + +.changes-file-row { + display: flex; + align-items: center; + gap: 8px; + padding: 5px 12px; + cursor: pointer; + font-size: 12px; + border-bottom: 1px solid rgba(255,255,255,0.03); +} + +.changes-file-row:hover { + background: rgba(120,130,255,0.06); +} + +.changes-file-state { + flex-shrink: 0; + width: 14px; + text-align: center; + font-weight: 700; + font-family: monospace; + color: #9090a8; +} + +.changes-state-a { color: #3ecf5a; } +.changes-state-m { color: #e0a030; } +.changes-state-d { color: #e05070; } +.changes-state-r, +.changes-state-c { color: #8088ff; } +.changes-state-\? { color: #9090a8; } + +.changes-file-path { + flex: 1; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + color: #d0d0e0; +} + +.changes-file-counts { + flex-shrink: 0; + display: flex; + gap: 6px; + font-family: monospace; + font-size: 11px; +} + +.changes-added { color: #3ecf5a; } +.changes-deleted { color: #e05070; } + +.changes-error { + padding: 12px; + color: #e05070; + font-size: 12px; +} + +#changes-diff-view { + flex: 1; + display: flex; + flex-direction: column; + min-height: 0; +} + +.changes-diff-body { + flex: 1; + overflow: auto; + margin: 0; + padding: 4px 0; + font-family: monospace; + font-size: 12px; + white-space: pre; +} + +.changes-diff-line { + padding: 0 12px; +} + +.changes-diff-add { + background: rgba(63, 185, 80, 0.12); + color: #3ecf5a; +} + +.changes-diff-del { + background: rgba(248, 81, 73, 0.12); + color: #e05070; +} + +.changes-diff-hunk { + color: #8088ff; +} + +.changes-diff-file-header { + color: #9090a8; +} + +.changes-diff-truncated { + padding: 6px 12px; + font-size: 11px; + color: #9090a8; + border-top: 1px solid rgba(255,255,255,0.06); +} + /* --- Diagnostics: activity trace --- */ #activity-trace-viewer { position: absolute; diff --git a/remote-attach.js b/remote-attach.js index 629d33bf..b0bf4ad9 100644 --- a/remote-attach.js +++ b/remote-attach.js @@ -173,9 +173,13 @@ function buildRemoteCommandArgs(alias, command) { return ['-o', 'BatchMode=yes', '-o', 'ConnectTimeout=5', '-n', alias, command]; } -// see .ai/contexts/session-cache.md ("Remote hosts — tmux attach") -function defaultRunRemoteCommand(alias, command, { timeoutMs } = {}) { - const { spawn } = require('child_process'); +// Default stdout cap for a single ssh exec — see .ai/contexts/changes-view.md ("Remote transport stdout cap"). +const DEFAULT_MAX_STDOUT_BYTES = 8 * 1024 * 1024; + +// see .ai/contexts/session-cache.md ("Remote hosts — tmux attach") and .ai/contexts/changes-view.md ("Remote transport stdout cap") +function defaultRunRemoteCommand(alias, command, { timeoutMs, maxStdoutBytes, spawnFn } = {}) { + const spawn = spawnFn || require('child_process').spawn; + const stdoutCap = typeof maxStdoutBytes === 'number' ? maxStdoutBytes : DEFAULT_MAX_STDOUT_BYTES; return new Promise((resolve) => { let child; try { @@ -187,16 +191,31 @@ function defaultRunRemoteCommand(alias, command, { timeoutMs } = {}) { return; } let stdout = ''; + let stdoutBytes = 0; let stderr = ''; let settled = false; + let overflowed = false; const timer = setTimeout(() => { try { child.kill('SIGKILL'); } catch {} }, timeoutMs || DEFAULT_PROBE_TIMEOUT_MS); const finish = (code) => { if (settled) return; settled = true; clearTimeout(timer); + if (overflowed) { + resolve({ code: -1, stdout: '', stderr: `stdout exceeded ${stdoutCap} bytes` }); + return; + } resolve({ code, stdout, stderr: stderr.slice(0, 4096) }); }; - if (child.stdout) child.stdout.on('data', (c) => { stdout += c; }); + if (child.stdout) child.stdout.on('data', (c) => { + if (overflowed) return; + stdoutBytes += Buffer.byteLength(c); + if (stdoutBytes > stdoutCap) { + overflowed = true; + try { child.kill('SIGKILL'); } catch {} + return; + } + stdout += c; + }); if (child.stderr) child.stderr.on('data', (c) => { if (stderr.length < 4096) stderr += c; }); child.on('error', (err) => { stderr += err.message; finish(-1); }); child.on('close', (code) => finish(code == null ? -1 : code)); @@ -354,4 +373,5 @@ module.exports = { isValidPid, buildProcCmdlineCheck, defaultRunRemoteCommand, + DEFAULT_MAX_STDOUT_BYTES, }; diff --git a/test/dom-file-panel-changes.test.js b/test/dom-file-panel-changes.test.js new file mode 100644 index 00000000..ec27376e --- /dev/null +++ b/test/dom-file-panel-changes.test.js @@ -0,0 +1,336 @@ +'use strict'; + +// Renderer tests for the Changes mode in public/file-panel.js (issue #251). +// Strategy mirrors test/dom-work-files-view.test.js: evaluate the real +// renderer files in a jsdom window, stub window.api and ViewerPanel, drive +// the public tab functions, and assert on the resulting DOM plus call counts. +// +// session-state.js + session-activity-dom.js + session-activity.js load +// alongside file-panel.js (same order as index.html) because the no-polling +// refresh hooks into session-activity.js's onSessionIdle — the very thing the +// zero-invocation test below is proving. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); +const { JSDOM } = require('jsdom'); + +const PUBLIC_DIR = path.join(__dirname, '..', 'public'); + +const INDEX_HTML = ` + + +
+
+
+ + +`; + +function evalInWindow(dom, file) { + vm.runInContext(fs.readFileSync(file, 'utf8'), dom.getInternalVMContext(), { filename: file }); +} + +function makeStatusResult(overrides = {}) { + return { + 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: false, unstaged: false, untracked: true, renamed: false, state: '?', added: null, deleted: null }, + ], + totals: { files: 2, added: 3, deleted: 1 }, + ...overrides, + }; +} + +function setupFilePanelDom({ statusImpl, diffImpl } = {}) { + const dom = new JSDOM(INDEX_HTML, { url: 'http://localhost/', runScripts: 'outside-only', pretendToBeVisual: true }); + const { window } = dom; + + const calls = { status: [], diff: [] }; + + window.api = { + onMcpOpenDiff: () => {}, + onMcpOpenFile: () => {}, + onMcpCloseAllDiffs: () => {}, + onMcpCloseTab: () => {}, + gitChangesStatus: (sessionId) => { + 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)); + }, + }; + + Object.defineProperty(window, 'ViewerPanel', { + value: function ViewerPanelStub() { return { open() {}, destroy() {} }; }, + writable: true, + configurable: true, + }); + Object.defineProperty(window, 'activeSessionId', { value: null, writable: true, configurable: true }); + + evalInWindow(dom, path.join(PUBLIC_DIR, 'session-state.js')); + evalInWindow(dom, path.join(PUBLIC_DIR, 'session-activity-dom.js')); + evalInWindow(dom, path.join(PUBLIC_DIR, 'session-activity.js')); + evalInWindow(dom, path.join(PUBLIC_DIR, 'file-panel.js')); + + window.initFilePanel(); + + const ctx = dom.getInternalVMContext(); + const read = (expr) => vm.runInContext(expr, ctx); + + return { + window, + document: window.document, + calls, + setActivity: read('setActivity'), + destroy: () => window.close(), + }; +} + +function flush() { + // Two microtask turns: one for the IPC promise, one for whatever chains off it. + return Promise.resolve().then(() => Promise.resolve()); +} + +// --- Rendering --------------------------------------------------------- + +test('openChangesTab loads status and renders the summary, branch info, and one row per file', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + const summary = ctx.document.getElementById('changes-summary'); + assert.match(summary.textContent, /2 files changed \+3/); + assert.match(summary.textContent, /−1/, 'unicode minus for deleted count'); + + const branchInfo = ctx.document.getElementById('changes-branch-info'); + assert.match(branchInfo.textContent, /main/); + assert.match(branchInfo.textContent, /↑1/, 'ahead-by-one arrow'); + + const rows = ctx.document.querySelectorAll('.changes-file-row'); + assert.equal(rows.length, 2); + assert.equal(rows[0].dataset.path, 'src/a.js'); + assert.equal(rows[1].dataset.path, 'new.txt'); + assert.equal(ctx.calls.status.length, 1); + assert.deepEqual(ctx.calls.status, ['s1']); + } finally { ctx.destroy(); } +}); + +test('an error from gitChangesStatus renders as an error message, not a crash', async () => { + const ctx = setupFilePanelDom({ statusImpl: () => ({ ok: false, error: 'not a git repository' }) }); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + const err = ctx.document.querySelector('.changes-error'); + assert.ok(err, 'an error element must be rendered'); + assert.match(err.textContent, /not a git repository/); + } finally { ctx.destroy(); } +}); + +test('clicking a file row opens a read-only diff colored by line prefix', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + const row = ctx.document.querySelector('.changes-file-row[data-path="src/a.js"]'); + row.dispatchEvent(new ctx.window.Event('click', { bubbles: true })); + await flush(); + + assert.equal(ctx.calls.diff.length, 1); + assert.deepEqual(ctx.calls.diff[0], { sessionId: 's1', filePath: 'src/a.js', staged: true }); + + const addLine = ctx.document.querySelector('.changes-diff-add'); + const delLine = ctx.document.querySelector('.changes-diff-del'); + const hunkLine = ctx.document.querySelector('.changes-diff-hunk'); + assert.ok(addLine && addLine.textContent === '+new'); + assert.ok(delLine && delLine.textContent === '-old'); + assert.ok(hunkLine && hunkLine.textContent.startsWith('@@')); + + // Back returns to the file list without another status call. + const backBtn = Array.from(ctx.document.querySelectorAll('#changes-diff-view button')).find(b => b.textContent === 'Back'); + backBtn.click(); + assert.equal(ctx.document.getElementById('changes-list').style.display, 'block'); + assert.equal(ctx.calls.status.length, 1, 'returning to the list must not re-fetch status'); + } finally { ctx.destroy(); } +}); + +test('clicking an untracked file shows a note instead of calling gitChangesDiff', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + + const row = ctx.document.querySelector('.changes-file-row[data-path="new.txt"]'); + 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'); + const body = ctx.document.querySelector('.changes-diff-body'); + assert.match(body.textContent, /Untracked file/); + } finally { ctx.destroy(); } +}); + +test('the Refresh button re-invokes gitChangesStatus', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + assert.equal(ctx.calls.status.length, 1); + + const refreshBtn = Array.from(ctx.document.querySelectorAll('#file-panel-changes button')).find(b => b.textContent === 'Refresh'); + refreshBtn.click(); + await flush(); + + assert.equal(ctx.calls.status.length, 2); + } finally { ctx.destroy(); } +}); + +// --- No-polling refresh trigger + zero-invocation proof ----------------- + +test('setActivity(id, false) refreshes an open Changes tab for that session only', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + assert.equal(ctx.calls.status.length, 1, 'the initial open'); + + // A different, unrelated session going idle must not trigger a fetch for s1's tab. + ctx.setActivity('s2', true); + ctx.setActivity('s2', false); + await flush(); + assert.equal(ctx.calls.status.length, 1, 'an unrelated session\'s idle transition must not refresh s1'); + + // s1 itself finishing a turn (busy -> idle) must refresh its own open tab. + ctx.setActivity('s1', true); + await flush(); + assert.equal(ctx.calls.status.length, 1, 'going busy must never trigger a git invocation'); + + ctx.setActivity('s1', false); + await flush(); + assert.equal(ctx.calls.status.length, 2, 'going idle must refresh the open Changes tab exactly once'); + } finally { ctx.destroy(); } +}); + +test('zero git invocations while a session stays busy, or idle with no new event', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + const baseline = ctx.calls.status.length; + assert.equal(baseline, 1); + + ctx.setActivity('s1', true); + await flush(); + ctx.setActivity('s1', true); // duplicate busy signal — no transition + await flush(); + assert.equal(ctx.calls.status.length, baseline, 'no invocation while busy'); + + ctx.setActivity('s1', false); + await flush(); + const afterFirstIdle = ctx.calls.status.length; + assert.equal(afterFirstIdle, baseline + 1); + + // Idle again with no new busy->idle transition in between: setActivity's + // own response-ready lock swallows the duplicate, so no second refresh. + ctx.setActivity('s1', false); + await flush(); + assert.equal(ctx.calls.status.length, afterFirstIdle, 'a duplicate idle signal with no new event must not re-invoke git'); + } finally { ctx.destroy(); } +}); + +// --- armReady:false duplicate idle (adversarial review, MINOR finding 5) --- +// A remote row's decay/detach path calls setActivity(id, false, ..., {armReady:false}) +// (see .ai/contexts/session-cache.md, "Remote hosts — busy spinner") — that +// opt-out means the response-ready lock never arms, so two such idle calls in +// a row with no busy in between must not double-fire the refresh on their own. + +test('two consecutive setActivity(id, false, ..., {armReady:false}) idles refresh only once (mutation target: firing notifySessionIdle on every !active call)', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + assert.equal(ctx.calls.status.length, 1, 'the initial open'); + + ctx.setActivity('s1', true, 'remote-seed'); + await flush(); + ctx.setActivity('s1', false, 'remote-decay', { armReady: false }); + await flush(); + assert.equal(ctx.calls.status.length, 2, 'the busy->idle edge must refresh once'); + + ctx.setActivity('s1', false, 'remote-decay', { armReady: false }); + await flush(); + assert.equal(ctx.calls.status.length, 2, 'a second armReady:false idle with no new busy edge must not refresh again'); + } finally { ctx.destroy(); } +}); + +test('busy, then idle, then busy again, then idle again: two edges, two refreshes — not swallowed by the armReady:false dedup', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + await ctx.window.openChangesTab('s1'); + await flush(); + assert.equal(ctx.calls.status.length, 1); + + ctx.setActivity('s1', true, 'remote-seed'); + ctx.setActivity('s1', false, 'remote-decay', { armReady: false }); + await flush(); + assert.equal(ctx.calls.status.length, 2, 'first busy->idle edge'); + + ctx.setActivity('s1', true, 'remote-seed'); + ctx.setActivity('s1', false, 'remote-decay', { armReady: false }); + await flush(); + assert.equal(ctx.calls.status.length, 3, 'second busy->idle edge must still refresh'); + } finally { ctx.destroy(); } +}); + +test('a session with no Changes tab open never calls gitChangesStatus, however often it goes idle', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.setActivity('s9', true); + ctx.setActivity('s9', false); + ctx.setActivity('s9', true); + ctx.setActivity('s9', false); + await flush(); + assert.equal(ctx.calls.status.length, 0); + } finally { ctx.destroy(); } +}); + +// --- Toggle button ------------------------------------------------------- + +test('the Changes header button opens and closes the tab for the active session', async () => { + const ctx = setupFilePanelDom(); + try { + ctx.window.switchPanel('s1'); + const btn = ctx.document.getElementById('changes-toggle-btn'); + assert.ok(btn, 'the Changes toggle button must be created in the terminal header'); + + btn.click(); + await flush(); + assert.equal(ctx.calls.status.length, 1); + assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), true); + + btn.click(); + assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), false); + } finally { ctx.destroy(); } +}); diff --git a/test/git-changes-runner-real-git.test.js b/test/git-changes-runner-real-git.test.js new file mode 100644 index 00000000..f66ae658 --- /dev/null +++ b/test/git-changes-runner-real-git.test.js @@ -0,0 +1,112 @@ +'use strict'; + +// A real, on-disk git repository — no injected `exec`, the local runner's own +// execFile('git', ...) path. Adversarial review (issue #251), SUGGESTION 7: +// document what an absolute path or a "~" pathspec does against real git, +// and reject it in isSafeGitPath if that turns out to be unsafe. Measured +// behavior (git 2.24+, --literal-pathspecs): git itself refuses an absolute +// pathspec that resolves outside the repository ("fatal: ... is outside +// repository", exit 128, no stdout) — no content ever leaks from outside the +// 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. + +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 { createGitChangesRunner } = require('../git-changes-runner'); + +function mkTmp() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'switchboard-gcr-real-')); + // Windows can hand back a short (8.3) TEMP path (e.g. "JEAN-B~1"); git's + // own containment check resolves the repo root through its long form and + // then rejects a short-form pathspec as "outside repository" even when it + // points at the same file — an environment quirk, not the thing under + // test. Canonicalize once so cwd and every pathspec built from `dir` agree. + return fs.realpathSync.native(dir); +} + +function cleanup(dir) { + fs.rmSync(dir, { recursive: true, force: true }); +} + +// Scratch repo only: drop the caller's GIT_* env (set when this suite runs under a hook) and its hooks. +function scratchGitEnv() { + const env = {}; + for (const [k, v] of Object.entries(process.env)) { + if (k.startsWith('GIT_') || k.startsWith('HUSKY')) continue; + env[k] = v; + } + return env; +} + +function git(cwd, args) { + return execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args], { cwd, encoding: 'utf8', env: scratchGitEnv() }); +} + +function initRepo(repoDir) { + fs.mkdirSync(repoDir, { recursive: true }); + git(repoDir, ['init', '-q']); + git(repoDir, ['config', 'user.email', 'a@a.com']); + git(repoDir, ['config', 'user.name', 'a']); + fs.writeFileSync(path.join(repoDir, 'tracked.txt'), 'line1\n'); + git(repoDir, ['add', 'tracked.txt']); + git(repoDir, ['commit', '-q', '-m', 'init']); + fs.writeFileSync(path.join(repoDir, 'tracked.txt'), 'line1\nline2\n'); +} + +test('real git: an absolute pathspec outside the repo is refused by git itself — no secret content ever surfaces', 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'); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.diff(secretPath); + if (result.ok) { + // Whatever git did, it must never have echoed the secret file's content. + assert.ok(!result.content.includes('SUPER_SECRET_OUTSIDE_THE_REPO'), 'no content from outside the repo may ever surface'); + } else { + assert.match(result.error, /outside repository/, 'git refuses an absolute pathspec outside the repo with this message'); + } + } finally { + cleanup(tmp); + } +}); + +test('real git: an absolute pathspec INSIDE the repo still works — the containment check is about the repo boundary, not "absolute" per se', async () => { + const tmp = mkTmp(); + try { + const repoDir = path.join(tmp, 'repo'); + initRepo(repoDir); + const absoluteInsidePath = path.join(repoDir, 'tracked.txt'); + + const runner = createGitChangesRunner({ kind: 'local', cwd: repoDir }); + const result = await runner.diff(absoluteInsidePath.replace(/\\/g, '/')); + assert.equal(result.ok, true); + assert.match(result.content, /\+line2/); + } finally { + cleanup(tmp); + } +}); + +test('real git: a literal "~/..." pathspec is never shell-expanded (no shell is invoked) and resolves to nothing, not the caller\'s home directory', 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('~/nonexistent-should-not-expand'); + assert.equal(result.ok, true, 'a nonexistent literal path is not an error, just an empty diff'); + assert.equal(result.content, '', 'git diff never reports a nonexistent/untracked path — and "~" was never expanded to $HOME'); + } finally { + cleanup(tmp); + } +}); diff --git a/test/git-changes-runner.test.js b/test/git-changes-runner.test.js new file mode 100644 index 00000000..60012adc --- /dev/null +++ b/test/git-changes-runner.test.js @@ -0,0 +1,330 @@ +'use strict'; + +// git-changes-runner.js — local (execFile, no shell) and remote (ssh, shell on +// the far end) command construction, fully injected: no real git, no ssh, no +// network. See .ai/contexts/ipc-bridge.md ("Changes panel") and issue #251. + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { + createGitChangesRunner, + buildRemoteGitCommand, + buildGitArgs, + truncateDiffContent, + shQuote, + isSafeCwd, + isSafeGitPath, + MAX_DIFF_BYTES, + STATUS_MAX_STDOUT_BYTES, + DIFF_MAX_STDOUT_BYTES, +} = require('../git-changes-runner'); + +// --- shQuote / buildRemoteGitCommand --------------------------------------- + +test('shQuote wraps a plain value in single quotes', () => { + assert.equal(shQuote('/home/dev/proj'), "'/home/dev/proj'"); +}); + +test('shQuote escapes an embedded single quote with the standard close-escape-reopen trick', () => { + assert.equal(shQuote("it's/here"), "'it'\\''s/here'"); +}); + +test('buildRemoteGitCommand: fixed shape, cwd and every arg individually quoted', () => { + const cmd = buildRemoteGitCommand('/srv/app', ['status', '--porcelain=v2', '--branch']); + assert.equal(cmd, "git -C '/srv/app' 'status' '--porcelain=v2' '--branch'"); +}); + +// --- buildGitArgs: --literal-pathspecs on every invocation ----------------- + +test('buildGitArgs: prepends --literal-pathspecs before the subcommand (mutation target: dropping the flag)', () => { + assert.deepEqual(buildGitArgs(['status', '--porcelain=v2', '--branch']), ['--literal-pathspecs', 'status', '--porcelain=v2', '--branch']); + assert.deepEqual(buildGitArgs(['diff', '--', 'x.js']), ['--literal-pathspecs', 'diff', '--', 'x.js']); +}); + +test('buildRemoteGitCommand carries --literal-pathspecs through as its own quoted token, still before the subcommand', () => { + const cmd = buildRemoteGitCommand('/srv/app', buildGitArgs(['status', '--porcelain=v2', '--branch'])); + assert.equal(cmd, "git -C '/srv/app' '--literal-pathspecs' 'status' '--porcelain=v2' '--branch'"); +}); + +test('no builder ever emits a backtick, even when cwd/path contain one', () => { + const withBacktick = buildRemoteGitCommand('/srv/`whoami`', ['diff', '--', 'a`b`.js']); + // The backtick is neutralized by single-quoting, not stripped — assert the + // structural property that matters: it never sits outside a quoted token + // where a shell would interpret it. Every quoted segment starts and ends + // with a single quote; nothing is concatenated unquoted around it. + assert.match(withBacktick, /^git -C '.*' 'diff' '--' '.*'$/s); +}); + +test('a cwd or path containing $(...) stays inside single quotes, never interpolated', () => { + const cmd = buildRemoteGitCommand('/srv/$(rm -rf /)', ['diff', '--', 'x']); + assert.equal(cmd, "git -C '/srv/$(rm -rf /)' 'diff' '--' 'x'"); +}); + +test('a path containing an embedded single quote is escaped, not left to break out of the quoting', () => { + const cmd = buildRemoteGitCommand('/srv/app', ['diff', '--', "weird'name.js"]); + assert.equal(cmd, "git -C '/srv/app' 'diff' '--' 'weird'\\''name.js'"); +}); + +// --- isSafeCwd / isSafeGitPath --------------------------------------------- + +test('isSafeCwd accepts a normal path, rejects empty/NUL/newline', () => { + assert.equal(isSafeCwd('/home/dev/proj'), true); + assert.equal(isSafeCwd(''), false); + assert.equal(isSafeCwd('/a\0b'), false); + assert.equal(isSafeCwd('/a\nb'), false); + assert.equal(isSafeCwd(null), false); + assert.equal(isSafeCwd(42), false); +}); + +test('isSafeGitPath rejects path traversal (mutation target: dropping the ".." check)', () => { + assert.equal(isSafeGitPath('../../etc/passwd'), false); + assert.equal(isSafeGitPath('src/../../../etc/passwd'), false); + assert.equal(isSafeGitPath('src/file.js'), true); +}); + +test('isSafeGitPath rejects NUL/newline, accepts spaces and unicode', () => { + assert.equal(isSafeGitPath('a\0b'), false); + assert.equal(isSafeGitPath('a\nb'), false); + assert.equal(isSafeGitPath('my file.js'), true); + assert.equal(isSafeGitPath('café/déjà-vu.js'), true); +}); + +test('isSafeGitPath rejects a leading ":" — git pathspec magic interpreted even after "--" (mutation target: dropping the check)', () => { + assert.equal(isSafeGitPath(':(exclude)x'), false); + assert.equal(isSafeGitPath(':/'), false); + assert.equal(isSafeGitPath(':(top)src/x.js'), false); + assert.equal(isSafeGitPath('src/:weird.js'), true, 'a colon not in the first position is not pathspec magic'); +}); + +// --- truncateDiffContent: byte cap, cut on a line boundary ----------------- + +test('truncateDiffContent: content at or under the cap is returned unchanged', () => { + assert.deepEqual(truncateDiffContent('small\n', 100), { content: 'small\n', truncated: false }); +}); + +test('truncateDiffContent: cuts on a line boundary, never mid-line (mutation target: a naive .slice(0, maxBytes))', () => { + const content = 'aaaaaaaaaa\nbbbbbbbbbb\ncccccccccc\n'; // 3 lines of 11 bytes each + const result = truncateDiffContent(content, 15); // fits exactly 1 line (11) but not 2 (22) + assert.equal(result.truncated, true); + assert.equal(result.content, 'aaaaaaaaaa\n'); +}); + +test('truncateDiffContent: measures UTF-8 bytes, not JS string length (mutation target: content.length instead of Buffer.byteLength)', () => { + const content = 'é'.repeat(10) + '\n'; // 10 chars => 20 bytes, plus 1-byte \n = 21 bytes, 11 chars + const result = truncateDiffContent(content, 20); // under 21 bytes but over 20 chars would wrongly pass a char-length check + assert.equal(result.truncated, true); + assert.equal(result.content, '', 'the only line exceeds the cap alone, so nothing whole fits'); +}); + +test('truncateDiffContent: never exceeds maxBytes even when the very first line alone does', () => { + const content = 'x'.repeat(50) + '\nshort\n'; + const result = truncateDiffContent(content, 10); + assert.equal(result.truncated, true); + assert.equal(result.content, ''); +}); + +// --- createGitChangesRunner: construction guards --------------------------- + +test('createGitChangesRunner throws on an invalid kind or cwd', () => { + assert.throws(() => createGitChangesRunner({ kind: 'bogus', cwd: '/a' })); + assert.throws(() => createGitChangesRunner({ kind: 'local', cwd: '' })); + assert.throws(() => createGitChangesRunner({ kind: 'local', cwd: '/a\0b' })); +}); + +test('createGitChangesRunner requires an alias for a remote runner', () => { + assert.throws(() => createGitChangesRunner({ kind: 'remote', cwd: '/a' })); +}); + +// --- local runner: .status() ------------------------------------------------ + +// Keyed on the subcommand (args[1]) — args[0] is always the prepended +// --literal-pathspecs flag (see buildGitArgs / .ai/contexts/changes-view.md). +function localFakeExec(responses) { + const calls = []; + const exec = (args) => { + calls.push(args); + const key = args[1] + (args.includes('--cached') ? ':cached' : ''); + return Promise.resolve(responses[key] || { code: 0, stdout: '', stderr: '' }); + }; + return { exec, calls }; +} + +test('local runner .status(): three commands, no -C flag (cwd passed via execFile options, not argv), --literal-pathspecs and -z on every one', async () => { + const { exec, calls } = localFakeExec({ + status: { code: 0, stdout: '# branch.head main\x001 .M N... 100644 100644 100644 abc123 def456 foo.js\x00', stderr: '' }, + 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 result = await runner.status(); + + assert.equal(result.ok, true); + assert.equal(result.branch.head, 'main'); + assert.equal(result.files.length, 1); + assert.equal(calls.length, 3); + for (const args of calls) { + assert.ok(!args.includes('-C'), 'the local runner must not pass -C — cwd is execFile\'s own option'); + assert.equal(args[0], '--literal-pathspecs', 'every invocation must lead with --literal-pathspecs'); + assert.ok(args.includes('-z'), 'status/numstat must run with -z'); + } +}); + +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 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 result = await runner.status(); + assert.equal(result.ok, false); + assert.match(result.error, /ENOENT/); +}); + +// --- local runner: .diff() -------------------------------------------------- + +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 bad = await runner.diff('../escape.js'); + assert.equal(bad.ok, false); + assert.equal(calls.length, 0, 'an unsafe path must never reach exec'); + + const good = await runner.diff('src/x.js'); + assert.equal(good.ok, true); + assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--', 'src/x.js']); +}); + +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 }); + await runner.diff('src/x.js', { staged: true }); + assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--cached', '--', 'src/x.js']); +}); + +test('local runner .diff(): truncates content past 512 KB, on a line boundary, measured in bytes', async () => { + 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 result = await runner.diff('x.js'); + assert.equal(result.ok, true); + assert.equal(result.truncated, true); + assert.ok(Buffer.byteLength(result.content, 'utf8') <= MAX_DIFF_BYTES, 'must never exceed the byte cap'); + assert.ok(result.content.endsWith('\n'), 'must cut on a line boundary, never mid-line'); + assert.equal(result.content.length % 101, 0, 'must consist of whole 101-byte lines only'); +}); + +test('local runner .diff(): the byte cap is measured in UTF-8 bytes, not JS string length (mutation target: using .length instead of Buffer.byteLength)', async () => { + // 'é' is 1 JS string char but 2 UTF-8 bytes — a char-length cap would let + // roughly twice MAX_DIFF_BYTES worth of such lines through uncaught. + 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 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'); +}); + +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 result = await runner.diff('x.js'); + assert.equal(result.truncated, false); + assert.equal(result.content, 'small diff\n'); +}); + +// --- remote runner: command shape ------------------------------------------- + +test('remote runner .status(): three ssh calls, each a full "git -C --literal-pathspecs ... -z" string', async () => { + const commands = []; + const exec = (command) => { + commands.push(command); + if (command.includes("'status'")) return Promise.resolve({ code: 0, stdout: '# branch.head main\x00', stderr: '' }); + return Promise.resolve({ code: 0, stdout: '', stderr: '' }); + }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.status(); + + assert.equal(result.ok, true); + assert.equal(commands.length, 3); + 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'); + assert.ok(!cmd.includes('`'), 'no backtick in any remote command string'); + } +}); + +test('remote runner .diff(): the built command carries --literal-pathspecs, quotes cwd and path, never a shell-interpreted concatenation', async () => { + const commands = []; + const exec = (command) => { commands.push(command); return Promise.resolve({ code: 0, stdout: 'diff text\n', stderr: '' }); }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + await runner.diff('src/weird file.js', { staged: true }); + + assert.equal(commands.length, 1); + assert.equal(commands[0], "git -C '/srv/app' '--literal-pathspecs' 'diff' '--cached' '--' 'src/weird file.js'"); +}); + +// --- 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 () => { + const seenOpts = []; + const exec = (command, opts) => { seenOpts.push(opts); return Promise.resolve({ code: 0, stdout: '', stderr: '' }); }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + await runner.status(); + + assert.equal(seenOpts.length, 3); + for (const opts of seenOpts) assert.equal(opts.maxStdoutBytes, STATUS_MAX_STDOUT_BYTES); +}); + +test('remote runner .diff(): passes MAX_DIFF_BYTES-plus-slack as the transport stdout cap', async () => { + const seenOpts = []; + const exec = (command, opts) => { seenOpts.push(opts); return Promise.resolve({ code: 0, stdout: '', stderr: '' }); }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + await runner.diff('x.js'); + + assert.equal(seenOpts.length, 1); + assert.equal(seenOpts[0].maxStdoutBytes, DIFF_MAX_STDOUT_BYTES); + assert.ok(DIFF_MAX_STDOUT_BYTES > MAX_DIFF_BYTES, 'the transport cap must have slack above the display cap'); +}); + +test('remote runner: a transport-level stdout-cap failure surfaces as ok:false with the cap message, not silently truncated', async () => { + const exec = () => Promise.resolve({ code: -1, stdout: '', stderr: 'stdout exceeded 2097152 bytes' }); + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.status(); + assert.equal(result.ok, false); + assert.match(result.error, /stdout exceeded 2097152 bytes/); +}); + +test('remote runner: an unsafe path is refused before any ssh call', async () => { + let calls = 0; + const exec = () => { calls++; return Promise.resolve({ code: 0, stdout: '', stderr: '' }); }; + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.diff('../../etc/passwd'); + assert.equal(result.ok, false); + assert.equal(calls, 0); +}); + +test('remote runner: an ssh-level failure (exit 255) surfaces without claiming success', async () => { + const exec = () => Promise.resolve({ code: 255, stdout: '', stderr: 'ssh: connection refused' }); + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps', exec }); + const result = await runner.status(); + assert.equal(result.ok, false); + assert.match(result.error, /connection refused/); +}); + +test('remote runner: uses the real defaultRunRemoteCommand transport when no exec is injected (construction only, no network call made in this test)', () => { + const runner = createGitChangesRunner({ kind: 'remote', cwd: '/srv/app', alias: 'vps' }); + assert.equal(runner.kind, 'remote'); + assert.equal(runner.alias, 'vps'); +}); diff --git a/test/git-changes-target.test.js b/test/git-changes-target.test.js new file mode 100644 index 00000000..63356a41 --- /dev/null +++ b/test/git-changes-target.test.js @@ -0,0 +1,161 @@ +'use strict'; + +// Cwd resolution for the Changes panel IPCs — extracted so the resolution +// order (remote descriptor / live local PTY / disk scan) can be exercised +// with fully injected dependencies, no Electron. See issue #251 and +// .ai/contexts/ipc-bridge.md ("Changes panel"). + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { resolveGitChangesTarget, isValidChangesSessionId } = require('../git-changes-target'); + +function baseDeps(overrides = {}) { + return { + getCachedFolder: () => null, + isRemoteFolder: () => false, + parseFolderKey: (folder) => ({ alias: null, folder }), + getRemoteSessions: () => ({ sessions: [] }), + activeSessions: new Map(), + resolveSessionRealCwd: () => null, + existsSync: () => false, + projectsDir: '/projects', + ...overrides, + }; +} + +test('rejects an empty/missing session id without touching any dependency', () => { + let touched = false; + const deps = baseDeps({ getCachedFolder: () => { touched = true; return null; } }); + const result = resolveGitChangesTarget('', deps); + assert.equal(result.ok, false); + assert.equal(touched, false); +}); + +// --- sessionId shape validation (adversarial review, MAJOR finding 4) ------ + +test('isValidChangesSessionId: accepts a plain CLI-issued id and a remote pid: placeholder', () => { + assert.equal(isValidChangesSessionId('a1b2c3d4-e5f6-7890-abcd-ef1234567890'), true); + assert.equal(isValidChangesSessionId('pid:4242'), true); +}); + +test('isValidChangesSessionId: rejects path traversal, separators, and the composite subagent shape (mutation target: dropping the shape check)', () => { + assert.equal(isValidChangesSessionId('../../x'), false); + assert.equal(isValidChangesSessionId('..'), false); + assert.equal(isValidChangesSessionId('.'), false); + assert.equal(isValidChangesSessionId('a/b'), false); + assert.equal(isValidChangesSessionId('a\\b'), false); + assert.equal(isValidChangesSessionId('sub:parent-id:agent-1'), false, 'main.js never routes a subagent id to this IPC'); + assert.equal(isValidChangesSessionId('pid:'), false, 'a placeholder id must carry a positive integer pid'); + assert.equal(isValidChangesSessionId('pid:0'), false); + assert.equal(isValidChangesSessionId('pid:-1'), false); + assert.equal(isValidChangesSessionId(''), false); + assert.equal(isValidChangesSessionId(null), false); +}); + +test('resolveGitChangesTarget: "../../x" is refused up front, without ever calling resolveSessionRealCwd (mutation target: validating after dispatch instead of before)', () => { + let realCwdCalled = false; + const deps = baseDeps({ resolveSessionRealCwd: () => { realCwdCalled = true; return '/should-not-be-reached'; } }); + const result = resolveGitChangesTarget('../../x', deps); + assert.equal(result.ok, false); + assert.equal(realCwdCalled, false, 'an invalid id must never reach the disk-scanning fallback'); +}); + +test('resolveGitChangesTarget: a well-formed remote pid: placeholder id is accepted and dispatched to the remote path', () => { + const deps = baseDeps({ + getCachedFolder: () => 'vps::-home-dev-proj', + isRemoteFolder: () => true, + parseFolderKey: () => ({ alias: 'vps' }), + getRemoteSessions: (alias) => ({ + sessions: alias === 'vps' ? [{ sessionId: 'pid:4242', cwd: '/srv/app' }] : [], + }), + }); + const result = resolveGitChangesTarget('pid:4242', deps); + assert.deepEqual(result, { ok: true, kind: 'remote', alias: 'vps', cwd: '/srv/app' }); +}); + +test('remote: resolves cwd from the host descriptor list, no PTY required', () => { + const deps = baseDeps({ + getCachedFolder: () => 'vps::-home-dev-proj', + isRemoteFolder: () => true, + parseFolderKey: () => ({ alias: 'vps' }), + getRemoteSessions: (alias) => ({ + sessions: alias === 'vps' ? [{ sessionId: 's1', cwd: '/srv/app' }] : [], + }), + }); + const result = resolveGitChangesTarget('s1', deps); + assert.deepEqual(result, { ok: true, kind: 'remote', alias: 'vps', cwd: '/srv/app' }); +}); + +test('remote: refuses when the descriptor is gone or carries no cwd (mutation target: falling back to a stale value)', () => { + const deps = baseDeps({ + getCachedFolder: () => 'vps::-home-dev-proj', + isRemoteFolder: () => true, + parseFolderKey: () => ({ alias: 'vps' }), + getRemoteSessions: () => ({ sessions: [] }), + }); + const result = resolveGitChangesTarget('s1', deps); + assert.equal(result.ok, false); + assert.match(result.error, /no known working directory/); +}); + +test('local: a live session in this app wins with its own recorded cwd, even without touching the disk scan', () => { + let scanCalled = false; + const deps = baseDeps({ + activeSessions: new Map([['s1', { exited: false, cwd: '/repo/.claude-worktrees/feature-x' }]]), + resolveSessionRealCwd: () => { scanCalled = true; return '/should-not-be-used'; }, + }); + const result = resolveGitChangesTarget('s1', deps); + assert.deepEqual(result, { ok: true, kind: 'local', cwd: '/repo/.claude-worktrees/feature-x' }); + assert.equal(scanCalled, false, 'a live session\'s own cwd must short-circuit the disk scan'); +}); + +test('local: an exited session in activeSessions is treated as not-live, falling through to the disk scan', () => { + const deps = baseDeps({ + activeSessions: new Map([['s1', { exited: true, cwd: '/stale/cwd' }]]), + resolveSessionRealCwd: () => '/repo/real-cwd', + existsSync: (p) => p === '/repo/real-cwd', + }); + const result = resolveGitChangesTarget('s1', deps); + assert.deepEqual(result, { ok: true, kind: 'local', cwd: '/repo/real-cwd' }); +}); + +test('local: not live in this app — falls back to resolveSessionRealCwd, same source as the resume path', () => { + const deps = baseDeps({ + getCachedFolder: () => '-home-dev-proj', + resolveSessionRealCwd: (projectsDir, sessionId, preferredFolder) => { + assert.equal(projectsDir, '/projects'); + assert.equal(sessionId, 's1'); + assert.equal(preferredFolder, '-home-dev-proj'); + return '/home/dev/proj'; + }, + existsSync: (p) => p === '/home/dev/proj', + }); + const result = resolveGitChangesTarget('s1', deps); + assert.deepEqual(result, { ok: true, kind: 'local', cwd: '/home/dev/proj' }); +}); + +test('local: a resolved cwd that no longer exists on disk is refused, not returned stale', () => { + const deps = baseDeps({ + resolveSessionRealCwd: () => '/deleted/worktree', + existsSync: () => false, + }); + const result = resolveGitChangesTarget('s1', deps); + assert.equal(result.ok, false); +}); + +test('local: nothing found anywhere resolves to a clear error, not a throw', () => { + const deps = baseDeps(); + const result = resolveGitChangesTarget('s1', deps); + assert.equal(result.ok, false); + assert.match(result.error, /could not resolve/); +}); + +test('a getCachedFolder throw is swallowed, resolution still proceeds as local', () => { + const deps = baseDeps({ + getCachedFolder: () => { throw new Error('db closed'); }, + resolveSessionRealCwd: () => '/home/dev/proj', + existsSync: () => true, + }); + const result = resolveGitChangesTarget('s1', deps); + assert.deepEqual(result, { ok: true, kind: 'local', cwd: '/home/dev/proj' }); +}); diff --git a/test/git-changes.test.js b/test/git-changes.test.js new file mode 100644 index 00000000..b760f512 --- /dev/null +++ b/test/git-changes.test.js @@ -0,0 +1,240 @@ +'use strict'; + +// Parser for `git status --porcelain=v2 --branch` + `git diff --numstat`, +// plus the merge into the Changes panel's model. See issue #251 and +// .ai/contexts/ipc-bridge.md ("Changes panel"). +// +// Each test below is written so that the specific distinction it names is +// load-bearing: collapsing staged/unstaged into one boolean, or treating a +// rename as a plain add+delete pair, turns the corresponding assertion red. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { parseStatusPorcelainV2, parseNumstat, mergeChanges } = require('../git-changes'); + +// --- parseStatusPorcelainV2 -------------------------------------------- + +test('branch header: head, upstream, ahead/behind', () => { + const text = [ + '# branch.oid abc123', + '# branch.head main', + '# branch.upstream origin/main', + '# branch.ab +2 -1', + ].join('\0'); + const { branch } = parseStatusPorcelainV2(text); + assert.deepEqual(branch, { head: 'main', upstream: 'origin/main', ahead: 2, behind: 1 }); +}); + +test('branch header: detached HEAD reports head:null, no upstream defaults to null/0', () => { + const text = '# branch.head (detached)'; + const { branch } = parseStatusPorcelainV2(text); + assert.deepEqual(branch, { head: null, upstream: null, ahead: 0, behind: 0 }); +}); + +test('ordinary entry: staged and unstaged are independent booleans (mutation target: collapsing XY to one flag)', () => { + const { files } = parseStatusPorcelainV2([ + '1 M. N... 100644 100644 100644 abc123 def456 staged-only.js', + '1 .M N... 100644 100644 100644 abc123 def456 unstaged-only.js', + '1 MM N... 100644 100644 100644 abc123 def456 both.js', + ].join('\0')); + + const byPath = Object.fromEntries(files.map(f => [f.path, f])); + assert.deepEqual( + { staged: byPath['staged-only.js'].staged, unstaged: byPath['staged-only.js'].unstaged }, + { staged: true, unstaged: false }, + ); + assert.deepEqual( + { staged: byPath['unstaged-only.js'].staged, unstaged: byPath['unstaged-only.js'].unstaged }, + { staged: false, unstaged: true }, + ); + assert.deepEqual( + { staged: byPath['both.js'].staged, unstaged: byPath['both.js'].unstaged }, + { staged: true, unstaged: true }, + 'a file modified in both the index and the worktree must report both flags true, not collapse to one', + ); +}); + +test('ordinary entry: state reflects the staged code when present, else the unstaged code', () => { + const { files } = parseStatusPorcelainV2([ + '1 A. N... 000000 100644 100644 0000000 abc1234 added.js', + '1 .D N... 100644 100644 000000 abc1234 0000000 deleted.js', + ].join('\0')); + const byPath = Object.fromEntries(files.map(f => [f.path, f])); + assert.equal(byPath['added.js'].state, 'A'); + assert.equal(byPath['deleted.js'].state, 'D'); +}); + +test('untracked entry: marked untracked, not staged/unstaged, state "?" (mutation target: dropping the untracked flag)', () => { + const { files } = parseStatusPorcelainV2('? new-file.js'); + assert.equal(files.length, 1); + assert.deepEqual(files[0], { + path: 'new-file.js', origPath: null, staged: false, unstaged: false, untracked: true, renamed: false, state: '?', + }); +}); + +test('ignored entries are dropped, not surfaced as files', () => { + const { files } = parseStatusPorcelainV2('! ignored/build-output.js'); + assert.deepEqual(files, []); +}); + +test('rename entry (-z): renamed:true and origPath carried through as the next NUL-terminated token, no tab embedded (mutation target: dropping rename handling)', () => { + const { files } = parseStatusPorcelainV2( + ['2 R. N... 100644 100644 100644 abc1234 def5678 R100 new/path.js', 'old/path.js'].join('\0') + ); + assert.equal(files.length, 1); + const f = files[0]; + assert.equal(f.renamed, true); + assert.equal(f.path, 'new/path.js'); + assert.equal(f.origPath, 'old/path.js'); + assert.equal(f.state, 'R'); +}); + +test('rename entry without renamed handling would collapse to a bare path with no origPath — pinned distinctly from an ordinary entry', () => { + const renamed = parseStatusPorcelainV2( + ['2 R. N... 100644 100644 100644 abc1234 def5678 R100 b.js', 'a.js'].join('\0') + ).files[0]; + const ordinary = parseStatusPorcelainV2( + '1 M. N... 100644 100644 100644 abc1234 def5678 b.js' + ).files[0]; + assert.notEqual(renamed.renamed, ordinary.renamed, 'a rename record must be distinguishable from an ordinary modify'); + assert.ok(renamed.origPath && !ordinary.origPath); +}); + +test('copy entry (score C): renamed:true, state "C"', () => { + const f = parseStatusPorcelainV2( + ['2 C. N... 100644 100644 100644 abc1234 def5678 C90 copy.js', 'source.js'].join('\0') + ).files[0]; + assert.equal(f.renamed, true); + assert.equal(f.state, 'C'); + assert.equal(f.origPath, 'source.js'); +}); + +test('rename entry (-z): a non-ASCII path round-trips byte-for-byte, no core.quotepath escaping to undo', () => { + const f = parseStatusPorcelainV2( + ['2 R. N... 100644 100644 100644 abc1234 def5678 R100 café-new.txt', 'café-old.txt'].join('\0') + ).files[0]; + assert.equal(f.path, 'café-new.txt'); + assert.equal(f.origPath, 'café-old.txt'); +}); + +test('ordinary entry (-z): a path with an embedded space is not quoted or truncated', () => { + const f = parseStatusPorcelainV2( + '1 .M N... 100644 100644 100644 abc123 def456 my file.js' + ).files[0]; + assert.equal(f.path, 'my file.js'); +}); + +test('unmerged entry: staged and unstaged both true, state carries a letter', () => { + const f = parseStatusPorcelainV2( + 'u UU N... 100644 100644 100644 100644 abc1 def2 ghi3 conflict.js' + ).files[0]; + assert.equal(f.staged, true); + assert.equal(f.unstaged, true); + assert.equal(f.state, 'U'); +}); + +test('unknown/future record types are skipped without throwing', () => { + assert.doesNotThrow(() => { + const { files } = parseStatusPorcelainV2(['x SOMETHING new-record-type', '? real.js'].join('\0')); + assert.equal(files.length, 1); + assert.equal(files[0].path, 'real.js'); + }); +}); + +test('empty input produces an empty, well-formed result', () => { + assert.deepEqual(parseStatusPorcelainV2(''), { + branch: { head: null, upstream: null, ahead: 0, behind: 0 }, files: [], + }); + assert.deepEqual(parseStatusPorcelainV2(null), { + branch: { head: null, upstream: null, ahead: 0, behind: 0 }, files: [], + }); +}); + +// --- parseNumstat (-z, NUL-separated) ------------------------------------- + +test('numstat -z: plain added/deleted counts keyed by path', () => { + const result = parseNumstat(['3\t1\tfoo.js', '0\t5\tbar.js'].join('\0') + '\0'); + assert.deepEqual(result, { 'foo.js': { added: 3, deleted: 1 }, 'bar.js': { added: 0, deleted: 5 } }); +}); + +test('numstat -z: binary file reports null, not 0 (mutation target: treating "-" as zero)', () => { + const result = parseNumstat('-\t-\timage.png\0'); + assert.deepEqual(result, { 'image.png': { added: null, deleted: null } }); +}); + +test('numstat -z: a rename is reported as an empty path field followed by two NUL-terminated tokens (old, new), keyed on the new path', () => { + const result = parseNumstat(['2\t1\t', 'old/name.js', 'new/name.js'].join('\0') + '\0'); + assert.deepEqual(result, { 'new/name.js': { added: 2, deleted: 1 } }); +}); + +test('numstat -z: a non-ASCII rename path round-trips byte-for-byte', () => { + const result = parseNumstat(['1\t0\t', 'café-old.txt', 'café-new.txt'].join('\0') + '\0'); + assert.deepEqual(result, { 'café-new.txt': { added: 1, deleted: 0 } }); +}); + +test('numstat -z: a path with an embedded space is not quoted or truncated', () => { + const result = parseNumstat('2\t0\tmy file.js\0'); + assert.deepEqual(result, { 'my file.js': { added: 2, deleted: 0 } }); +}); + +test('numstat -z: blank/malformed tokens are ignored, not throwing', () => { + assert.doesNotThrow(() => { + const result = parseNumstat(['', 'not a numstat token', '2\t1\tok.js'].join('\0') + '\0'); + assert.deepEqual(result, { 'ok.js': { added: 2, deleted: 1 } }); + }); +}); + +// --- mergeChanges --------------------------------------------------------- + +test('mergeChanges: a staged-only file gets its counts from the staged numstat map', () => { + const status = { branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, files: [ + { path: 'a.js', origPath: null, staged: true, unstaged: false, untracked: false, renamed: false, state: 'M' }, + ] }; + const merged = mergeChanges(status, { 'a.js': { added: 5, deleted: 2 } }, {}); + assert.deepEqual(merged.files[0], { path: 'a.js', origPath: null, staged: true, unstaged: false, untracked: false, renamed: false, state: 'M', added: 5, deleted: 2 }); + assert.deepEqual(merged.totals, { files: 1, added: 5, deleted: 2 }); +}); + +test('mergeChanges: a file modified in both index and worktree sums both numstat entries (mutation target: only reading one map)', () => { + const status = { branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, files: [ + { path: 'both.js', origPath: null, staged: true, unstaged: true, untracked: false, renamed: false, state: 'M' }, + ] }; + const merged = mergeChanges(status, { 'both.js': { added: 3, deleted: 1 } }, { 'both.js': { added: 2, deleted: 4 } }); + assert.equal(merged.files[0].added, 5, 'staged (3) + unstaged (2) added lines'); + 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', () => { + 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: '?' }, + ] }; + const merged = mergeChanges(status, {}, {}); + assert.equal(merged.files[0].added, null); + assert.equal(merged.files[0].deleted, null); + assert.deepEqual(merged.totals, { files: 1, added: 0, deleted: 0 }, 'totals only sum known counts'); +}); + +test('mergeChanges: a binary file (null in both maps) stays null after combining, not coerced to 0', () => { + const status = { branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, files: [ + { path: 'img.png', origPath: null, staged: false, unstaged: true, untracked: false, renamed: false, state: 'M' }, + ] }; + const merged = mergeChanges(status, {}, { 'img.png': { added: null, deleted: null } }); + assert.equal(merged.files[0].added, null); + assert.equal(merged.files[0].deleted, null); +}); + +test('mergeChanges: totals sum added/deleted across all files and count files', () => { + const status = { branch: { head: 'main', upstream: null, ahead: 0, behind: 0 }, files: [ + { path: 'a.js', origPath: null, staged: true, unstaged: false, untracked: false, renamed: false, state: 'M' }, + { path: 'b.js', origPath: null, staged: false, unstaged: true, untracked: false, renamed: false, state: 'M' }, + ] }; + const merged = mergeChanges(status, { 'a.js': { added: 1, deleted: 1 } }, { 'b.js': { added: 4, deleted: 0 } }); + assert.deepEqual(merged.totals, { files: 2, added: 5, deleted: 1 }); +}); + +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 }); + assert.deepEqual(merged.files, []); + assert.deepEqual(merged.totals, { files: 0, added: 0, deleted: 0 }); +}); diff --git a/test/remote-run-command-stdout-cap.test.js b/test/remote-run-command-stdout-cap.test.js new file mode 100644 index 00000000..c617f698 --- /dev/null +++ b/test/remote-run-command-stdout-cap.test.js @@ -0,0 +1,88 @@ +'use strict'; + +// defaultRunRemoteCommand's stdout cap — see .ai/contexts/changes-view.md +// ("Quoting rule") and issue #251 (adversarial review, CRITICAL finding 1). +// A fake child is injected via opts.spawnFn — no real ssh, no network. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { EventEmitter } = require('events'); + +const { defaultRunRemoteCommand, DEFAULT_MAX_STDOUT_BYTES } = require('../remote-attach'); + +function fakeChild() { + const child = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + child.killed = []; + child.kill = (sig) => { child.killed.push(sig); }; + return child; +} + +test('DEFAULT_MAX_STDOUT_BYTES is 8 MB', () => { + assert.equal(DEFAULT_MAX_STDOUT_BYTES, 8 * 1024 * 1024); +}); + +test('a fake child emitting more than maxStdoutBytes is killed and the promise resolves ok:false-shaped (code!==0, stderr names the cap)', async () => { + const child = fakeChild(); + const spawnFn = () => child; + + const resultPromise = defaultRunRemoteCommand('vps', 'git -C /repo status', { maxStdoutBytes: 10, spawnFn }); + + child.stdout.emit('data', Buffer.from('this chunk alone is already more than ten bytes')); + // A real child would emit 'close' only after the SIGKILL takes effect; the + // fake simulates that by closing right after the overflow is observed. + child.emit('close', null); + + const result = await resultPromise; + assert.equal(result.code, -1, 'an overflow must not report the process\'s own exit code as success'); + assert.match(result.stderr, /stdout exceeded 10 bytes/); + assert.equal(result.stdout, '', 'no partial/overflowing stdout must leak through'); + assert.deepEqual(child.killed, ['SIGKILL']); +}); + +test('a fake child under the cap resolves normally with full stdout, no kill', async () => { + const child = fakeChild(); + const spawnFn = () => child; + + const resultPromise = defaultRunRemoteCommand('vps', 'git -C /repo status', { maxStdoutBytes: 1024, spawnFn }); + + child.stdout.emit('data', Buffer.from('small output')); + child.emit('close', 0); + + const result = await resultPromise; + assert.equal(result.code, 0); + assert.equal(result.stdout, 'small output'); + assert.deepEqual(child.killed, []); +}); + +test('overflow across multiple small chunks is still caught (byte-counted cumulatively, not per-chunk)', async () => { + const child = fakeChild(); + const spawnFn = () => child; + + const resultPromise = defaultRunRemoteCommand('vps', 'git -C /repo diff', { maxStdoutBytes: 20, spawnFn }); + + child.stdout.emit('data', Buffer.from('12345678901')); // 11 bytes, under cap alone + child.stdout.emit('data', Buffer.from('12345678901')); // cumulative 22 bytes, over cap + child.emit('close', null); + + const result = await resultPromise; + assert.equal(result.code, -1); + assert.match(result.stderr, /stdout exceeded 20 bytes/); + assert.deepEqual(child.killed, ['SIGKILL']); +}); + +test('no maxStdoutBytes option falls back to the 8 MB default, not unbounded (RED before the fix: the old code had no cap at all)', async () => { + const child = fakeChild(); + const spawnFn = () => child; + + const resultPromise = defaultRunRemoteCommand('vps', 'git -C /repo diff', { spawnFn }); + + const over8mb = Buffer.alloc(DEFAULT_MAX_STDOUT_BYTES + 1, 97); // 'a' + child.stdout.emit('data', over8mb); + child.emit('close', null); + + const result = await resultPromise; + assert.equal(result.code, -1); + assert.match(result.stderr, new RegExp(`stdout exceeded ${DEFAULT_MAX_STDOUT_BYTES} bytes`)); +});