From afebb57c07ea0b6934a7a1cc20ea479c6fae3ad1 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Sun, 13 Sep 2026 00:12:03 +0200 Subject: [PATCH 1/2] (panel): read-only Changes view from git status, same for local and remote sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #251. A pure parser for porcelain v2 + numstat, one runner interface with a local execFile and a remote ssh implementation (every token single-quoted, no backtick), two IPCs resolving the session's real cwd (recorded cwd locally, descriptor cwd remotely), and a Changes mode in the right-hand panel: files changed with counters, branch and ahead/behind, click for a read-only diff. Refresh on open, on the button and when the session goes idle — no polling. --- .ai/contexts/README.md | 3 +- .ai/contexts/changes-view.md | 68 +++++ .ai/contexts/ipc-bridge.md | 12 + .ai/contexts/viewer-panel.md | 8 + .ai/shared-guidelines.md | 1 + docs/README.md | 1 + docs/changes-view.md | 37 +++ eslint.config.js | 5 + git-changes-runner.js | 127 ++++++++++ git-changes-target.js | 32 +++ git-changes.js | 146 +++++++++++ main.js | 44 ++++ preload.js | 3 + public/file-panel.js | 374 +++++++++++++++++++++++++++- public/session-activity.js | 13 + public/style.css | 122 +++++++++ test/dom-file-panel-changes.test.js | 290 +++++++++++++++++++++ test/git-changes-runner.test.js | 226 +++++++++++++++++ test/git-changes-target.test.js | 119 +++++++++ test/git-changes.test.js | 220 ++++++++++++++++ 20 files changed, 1849 insertions(+), 2 deletions(-) create mode 100644 .ai/contexts/changes-view.md create mode 100644 docs/changes-view.md create mode 100644 git-changes-runner.js create mode 100644 git-changes-target.js create mode 100644 git-changes.js create mode 100644 test/dom-file-panel-changes.test.js create mode 100644 test/git-changes-runner.test.js create mode 100644 test/git-changes-target.test.js create mode 100644 test/git-changes.test.js 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..d38810d6 --- /dev/null +++ b/.ai/contexts/changes-view.md @@ -0,0 +1,68 @@ +# 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". Handles both rename spellings numstat can emit — `old => new` (keyed on the new path) and `prefix/{old => new}/suffix` (partial common-directory rename). +- `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. +- No `-z`/NUL-terminated output is used for either command, so a path needing `core.quotepath` escaping (embedded quotes, some non-ASCII) can, in principle, fail to correlate between the status and numstat outputs. Not hit by the acceptance criteria; not fixed here — flag it if it comes up. + +## 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`, `git diff --numstat`, `git diff --cached --numstat`) and merges them. `diff()` runs `git diff [--cached] -- `, capped at 512 KB 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 '' 'diff' '--' '' ..."`. Timeout 20s. This command string DOES run through a shell on the far end. + +### Quoting rule: denylist + single-quote escaping, not an allowlist + +`cwd` and the diff `path` are the two values that reach the remote command string. Both are wrapped with `shQuote()` — standard POSIX single-quote escaping (close the quote, insert a literal quote via `'\''`, reopen) — before they're interpolated. That one function is what actually makes the remote path 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 that, `isSafeShellArg`/`isSafeCwd`/`isSafeGitPath` are a **denylist** (reject control characters and, for the path, any `..` segment) rather than a positive character allowlist. The reasoning: 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. The character check exists only to catch the one thing quoting can't fix (NUL/newline) and, for paths, to keep `..` from walking outside the repo. 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. + +## cwd resolution (`git-changes-target.js`) + +`resolveGitChangesTarget(sessionId, deps)`, in order: + +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. + +`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 — fired once per `setActivity(sessionId, false, …)` call, for that `sessionId` only (`notifySessionIdle`, called at the end of `setActivity` when `active` is falsy). 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, on a duplicate idle signal with no new transition (swallowed upstream by `setActivity`'s own response-ready lock), or 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 transport (`remote-attach.js`'s `defaultRunRemoteCommand`) 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. Everything downstream of the transport — command shape, quoting, parsing, the IPC handlers, cwd resolution — is fully tested with injected fakes. 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..113b8e49 --- /dev/null +++ b/git-changes-runner.js @@ -0,0 +1,127 @@ +// 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; + +// 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); +} + +function isSafeGitPath(p) { + if (!isSafeShellArg(p)) return false; + if (p.includes('..')) return false; + return 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(' '); +} + +function defaultLocalExec(args, { cwd, timeoutMs }) { + return new Promise((resolve) => { + execFile('git', args, { cwd, 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) => defaultRunRemoteCommand(alias, command, { timeoutMs: effectiveTimeout })); + + function invoke(args) { + return kind === 'local' ? runExec(args) : runExec(buildRemoteGitCommand(cwd, args)); + } + + async function status() { + let results; + try { + results = await Promise.all([ + invoke(['status', '--porcelain=v2', '--branch']), + invoke(['diff', '--numstat']), + invoke(['diff', '--cached', '--numstat']), + ]); + } 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); + } catch (err) { + return { ok: false, error: err.message }; + } + if (result.code !== 0) return { ok: false, error: firstError(result) }; + + const content = result.stdout || ''; + const truncated = content.length > MAX_DIFF_BYTES; + return { ok: true, content: truncated ? content.slice(0, MAX_DIFF_BYTES) : content, truncated }; + } + + return { status, diff, kind, cwd, alias: alias || null }; +} + +module.exports = { + createGitChangesRunner, + buildRemoteGitCommand, + shQuote, + isSafeCwd, + isSafeGitPath, + MAX_DIFF_BYTES, +}; diff --git a/git-changes-target.js b/git-changes-target.js new file mode 100644 index 00000000..c2ee8ff4 --- /dev/null +++ b/git-changes-target.js @@ -0,0 +1,32 @@ +// git-changes-target.js — cwd resolution for the Changes panel IPCs — see .ai/contexts/changes-view.md + +'use strict'; + +// deps: {getCachedFolder, isRemoteFolder, parseFolderKey, getRemoteSessions, activeSessions, resolveSessionRealCwd, existsSync, projectsDir} +function resolveGitChangesTarget(sessionId, deps) { + const id = String(sessionId || ''); + if (!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 }; diff --git a/git-changes.js b/git-changes.js new file mode 100644 index 00000000..84952ca7 --- /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'; + +const ORDINARY_RE = /^1 (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?.+)$/; +const RENAME_RE = /^2 (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?.+)$/; +const UNMERGED_RE = /^u (?\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` output — see .ai/contexts/changes-view.md +function parseStatusPorcelainV2(text) { + const branch = { head: null, upstream: null, ahead: 0, behind: 0 }; + const files = []; + + for (const raw of String(text || '').split('\n')) { + if (!raw) continue; + + 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; + const rest = m.groups.rest; + const tabIdx = rest.indexOf('\t'); + const path = tabIdx >= 0 ? rest.slice(0, tabIdx) : rest; + const origPath = tabIdx >= 0 ? rest.slice(tabIdx + 1) : null; + files.push(makeOrdinaryFile(path, m.groups.xy, true, origPath)); + 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 }; +} + +// see .ai/contexts/changes-view.md (numstat rename spellings) +function resolveNumstatPath(raw) { + const braceMatch = /^(.*)\{.* => (.*)\}(.*)$/.exec(raw); + if (braceMatch) { + return (braceMatch[1] + braceMatch[2] + braceMatch[3]).replace(/\/{2,}/g, '/'); + } + const arrowMatch = /^(.*) => (.*)$/.exec(raw); + if (arrowMatch) return arrowMatch[2]; + return raw; +} + +// Parse `git diff --numstat` output — see .ai/contexts/changes-view.md +function parseNumstat(text) { + const result = {}; + for (const line of String(text || '').split('\n')) { + if (!line.trim()) continue; + const m = /^(\d+|-)\t(\d+|-)\t(.+)$/.exec(line); + if (!m) continue; + const added = m[1] === '-' ? null : parseInt(m[1], 10); + const deleted = m[2] === '-' ? null : parseInt(m[2], 10); + const path = resolveNumstatPath(m[3]); + result[path] = { 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..9a6b4235 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,7 @@ function setActivity(sessionId, active, via, opts) { } applyActivityClasses(sessionId); + if (!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/test/dom-file-panel-changes.test.js b/test/dom-file-panel-changes.test.js new file mode 100644 index 00000000..7087e3ec --- /dev/null +++ b/test/dom-file-panel-changes.test.js @@ -0,0 +1,290 @@ +'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(); } +}); + +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.test.js b/test/git-changes-runner.test.js new file mode 100644 index 00000000..943a7174 --- /dev/null +++ b/test/git-changes-runner.test.js @@ -0,0 +1,226 @@ +'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, + shQuote, + isSafeCwd, + isSafeGitPath, +} = 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'"); +}); + +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); +}); + +// --- 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() ------------------------------------------------ + +function localFakeExec(responses) { + const calls = []; + const exec = (args) => { + calls.push(args); + const key = args[0] + (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)', async () => { + const { exec, calls } = localFakeExec({ + status: { code: 0, stdout: '# branch.head main\n1 .M N... 100644 100644 100644 abc123 def456 foo.js\n', stderr: '' }, + diff: { code: 0, stdout: '1\t2\tfoo.js\n', 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'); +}); + +test('local runner .status(): a failing git call surfaces stderr as the error, not a throw', async () => { + const exec = (args) => Promise.resolve( + args[0] === '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, 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], ['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], ['diff', '--cached', '--', 'src/x.js']); +}); + +test('local runner .diff(): truncates content past 512 KB and flags it', async () => { + const big = 'a'.repeat(600 * 1024); + 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.equal(result.content.length, 512 * 1024); +}); + +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 ..." string', async () => { + const commands = []; + const exec = (command) => { + commands.push(command); + if (command.includes("'status'")) return Promise.resolve({ code: 0, stdout: '# branch.head main\n', 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' /); + assert.ok(!cmd.includes('`'), 'no backtick in any remote command string'); + } +}); + +test('remote runner .diff(): the built command 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' 'diff' '--cached' '--' 'src/weird file.js'"); +}); + +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..4684bee3 --- /dev/null +++ b/test/git-changes-target.test.js @@ -0,0 +1,119 @@ +'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 } = 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); +}); + +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..92ee20f7 --- /dev/null +++ b/test/git-changes.test.js @@ -0,0 +1,220 @@ +'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('\n'); + 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('\n')); + + 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('\n')); + 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: renamed:true and origPath carried through (mutation target: dropping rename handling)', () => { + const { files } = parseStatusPorcelainV2( + '2 R. N... 100644 100644 100644 abc1234 def5678 R100 new/path.js\told/path.js' + ); + 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\ta.js' + ).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\tsource.js' + ).files[0]; + assert.equal(f.renamed, true); + assert.equal(f.state, 'C'); + assert.equal(f.origPath, 'source.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\n? real.js'); + 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 -------------------------------------------------------- + +test('numstat: plain added/deleted counts keyed by path', () => { + const result = parseNumstat('3\t1\tfoo.js\n0\t5\tbar.js\n'); + assert.deepEqual(result, { 'foo.js': { added: 3, deleted: 1 }, 'bar.js': { added: 0, deleted: 5 } }); +}); + +test('numstat: binary file reports null, not 0 (mutation target: treating "-" as zero)', () => { + const result = parseNumstat('-\t-\timage.png\n'); + assert.deepEqual(result, { 'image.png': { added: null, deleted: null } }); +}); + +test('numstat: a full rename ("old => new") is keyed on the new path', () => { + const result = parseNumstat('2\t1\told/name.js => new/name.js\n'); + assert.deepEqual(result, { 'new/name.js': { added: 2, deleted: 1 } }); +}); + +test('numstat: a partial common-directory rename ("prefix/{old => new}/suffix") resolves to the new path', () => { + const result = parseNumstat('4\t0\tsrc/{old => new}/file.js\n'); + assert.deepEqual(result, { 'src/new/file.js': { added: 4, deleted: 0 } }); +}); + +test('numstat: blank lines and malformed lines are ignored, not throwing', () => { + assert.doesNotThrow(() => { + const result = parseNumstat('\n\nnot a numstat line\n2\t1\tok.js\n'); + 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 }); +}); From 1de3d1c28afde81c43811f1b537fc6d7b8328fa6 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Sun, 13 Sep 2026 01:04:35 +0200 Subject: [PATCH 2/2] =?UTF-8?q?(panel):=20harden=20the=20Changes=20view=20?= =?UTF-8?q?=E2=80=94=20literal=20pathspecs,=20-z=20parsing,=20stdout=20cap?= =?UTF-8?q?,=20id=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Every git invocation runs with --literal-pathspecs and a leading ':' is refused, so a file name cannot carry pathspec magic; status and numstat are read with -z, so a non-ASCII or spaced name round-trips to git diff unescaped; the ssh transport caps stdout in bytes and kills the child on overflow; sessionId is shape-checked before any lookup; the idle refresh fires on a busy-to-idle edge only; the diff cap is measured in bytes on a line boundary; the local runner drops inherited GIT_DIR-style variables so only the session's cwd decides the repository. A real-git test pins that an absolute pathspec outside the repo yields nothing. --- .ai/contexts/changes-view.md | 51 ++++++-- git-changes-runner.js | 68 ++++++++-- git-changes-target.js | 15 ++- git-changes.js | 56 ++++----- public/session-activity.js | 3 +- remote-attach.js | 28 ++++- test/dom-file-panel-changes.test.js | 46 +++++++ test/git-changes-runner-real-git.test.js | 112 +++++++++++++++++ test/git-changes-runner.test.js | 138 ++++++++++++++++++--- test/git-changes-target.test.js | 44 ++++++- test/git-changes.test.js | 60 ++++++--- test/remote-run-command-stdout-cap.test.js | 88 +++++++++++++ 12 files changed, 614 insertions(+), 95 deletions(-) create mode 100644 test/git-changes-runner-real-git.test.js create mode 100644 test/remote-run-command-stdout-cap.test.js diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index d38810d6..56b6d781 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -20,34 +20,53 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). ## 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". Handles both rename spellings numstat can emit — `old => new` (keyed on the new path) and `prefix/{old => new}/suffix` (partial common-directory rename). +- `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. -- No `-z`/NUL-terminated output is used for either command, so a path needing `core.quotepath` escaping (embedded quotes, some non-ASCII) can, in principle, fail to correlate between the status and numstat outputs. Not hit by the acceptance criteria; not fixed here — flag it if it comes up. +- **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`, `git diff --numstat`, `git diff --cached --numstat`) and merges them. `diff()` runs `git diff [--cached] -- `, capped at 512 KB with a `truncated` flag. +`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 '' 'diff' '--' '' ..."`. Timeout 20s. This command string DOES run through a shell on the far end. +- **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: denylist + single-quote escaping, not an allowlist +### Quoting rule: literal pathspecs, `-z`, a stdout cap, and single-quote escaping — not an allowlist -`cwd` and the diff `path` are the two values that reach the remote command string. Both are wrapped with `shQuote()` — standard POSIX single-quote escaping (close the quote, insert a literal quote via `'\''`, reopen) — before they're interpolated. That one function is what actually makes the remote path 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. +Four independent defenses, added after an adversarial review of the first cut of this panel (issue #251): -Given that, `isSafeShellArg`/`isSafeCwd`/`isSafeGitPath` are a **denylist** (reject control characters and, for the path, any `..` segment) rather than a positive character allowlist. The reasoning: 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. The character check exists only to catch the one thing quoting can't fix (NUL/newline) and, for paths, to keep `..` from walking outside the repo. 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. +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. +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). @@ -55,7 +74,7 @@ Extracted out of the two IPC handlers into its own module, fully dependency-inje `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 — fired once per `setActivity(sessionId, false, …)` call, for that `sessionId` only (`notifySessionIdle`, called at the end of `setActivity` when `active` is falsy). 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, on a duplicate idle signal with no new transition (swallowed upstream by `setActivity`'s own response-ready lock), or for an unrelated session's idle transition. +`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. @@ -65,4 +84,16 @@ Extracted out of the two IPC handlers into its own module, fully dependency-inje ## What's untested for remote -The real ssh transport (`remote-attach.js`'s `defaultRunRemoteCommand`) 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. Everything downstream of the transport — command shape, quoting, parsing, the IPC handlers, cwd resolution — is fully tested with injected fakes. +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/git-changes-runner.js b/git-changes-runner.js index 113b8e49..1b6f97af 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -10,6 +10,10 @@ 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) { @@ -20,12 +24,35 @@ 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, "'\\''") + "'"; @@ -35,9 +62,18 @@ 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, timeout: timeoutMs, maxBuffer: LOCAL_MAX_BUFFER, windowsHide: true }, + 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) }); @@ -68,19 +104,24 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { const runExec = exec || (kind === 'local' ? (args) => defaultLocalExec(args, { cwd, timeoutMs: effectiveTimeout }) - : (command) => defaultRunRemoteCommand(alias, command, { timeoutMs: effectiveTimeout })); - - function invoke(args) { - return kind === 'local' ? runExec(args) : runExec(buildRemoteGitCommand(cwd, args)); + : (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']), - invoke(['diff', '--numstat']), - invoke(['diff', '--cached', '--numstat']), + 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 }; @@ -103,15 +144,14 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { let result; try { - result = await invoke(args); + 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 = result.stdout || ''; - const truncated = content.length > MAX_DIFF_BYTES; - return { ok: true, content: truncated ? content.slice(0, MAX_DIFF_BYTES) : content, truncated }; + const { content, truncated } = truncateDiffContent(result.stdout || '', MAX_DIFF_BYTES); + return { ok: true, content, truncated }; } return { status, diff, kind, cwd, alias: alias || null }; @@ -120,8 +160,12 @@ function createGitChangesRunner({ kind, cwd, alias, exec, timeoutMs } = {}) { 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 index c2ee8ff4..9d5cb982 100644 --- a/git-changes-target.js +++ b/git-changes-target.js @@ -2,10 +2,21 @@ '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 (!id) return { ok: false, error: 'invalid session id' }; + if (!isValidChangesSessionId(id)) return { ok: false, error: 'invalid session id' }; let folder = null; try { folder = deps.getCachedFolder(id); } catch {} @@ -29,4 +40,4 @@ function resolveGitChangesTarget(sessionId, deps) { return { ok: false, error: 'could not resolve a working directory for this session' }; } -module.exports = { resolveGitChangesTarget }; +module.exports = { resolveGitChangesTarget, isValidChangesSessionId }; diff --git a/git-changes.js b/git-changes.js index 84952ca7..e753a8e6 100644 --- a/git-changes.js +++ b/git-changes.js @@ -2,9 +2,10 @@ 'use strict'; -const ORDINARY_RE = /^1 (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?.+)$/; -const RENAME_RE = /^2 (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?.+)$/; -const UNMERGED_RE = /^u (?\S\S) (?\S+) (?\S+) (?\S+) (?\S+) (?\S+) (?

\S+) (?

\S+) (?

\S+) (?.+)$/; +// 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]; @@ -20,13 +21,15 @@ function makeOrdinaryFile(path, xy, renamed, origPath) { }; } -// Parse `git status --porcelain=v2 --branch` output — see .ai/contexts/changes-view.md +// 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 (const raw of String(text || '').split('\n')) { - if (!raw) continue; + 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(); @@ -56,11 +59,10 @@ function parseStatusPorcelainV2(text) { if (raw.startsWith('2 ')) { const m = RENAME_RE.exec(raw); if (!m) continue; - const rest = m.groups.rest; - const tabIdx = rest.indexOf('\t'); - const path = tabIdx >= 0 ? rest.slice(0, tabIdx) : rest; - const origPath = tabIdx >= 0 ? rest.slice(tabIdx + 1) : null; - files.push(makeOrdinaryFile(path, m.groups.xy, true, origPath)); + // -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 ')) { @@ -86,28 +88,26 @@ function parseStatusPorcelainV2(text) { return { branch, files }; } -// see .ai/contexts/changes-view.md (numstat rename spellings) -function resolveNumstatPath(raw) { - const braceMatch = /^(.*)\{.* => (.*)\}(.*)$/.exec(raw); - if (braceMatch) { - return (braceMatch[1] + braceMatch[2] + braceMatch[3]).replace(/\/{2,}/g, '/'); - } - const arrowMatch = /^(.*) => (.*)$/.exec(raw); - if (arrowMatch) return arrowMatch[2]; - return raw; -} - -// Parse `git diff --numstat` output — see .ai/contexts/changes-view.md +// Parse `git diff --numstat -z` output — see .ai/contexts/changes-view.md ("Quoting rule: -z instead of core.quotepath") function parseNumstat(text) { const result = {}; - for (const line of String(text || '').split('\n')) { - if (!line.trim()) continue; - const m = /^(\d+|-)\t(\d+|-)\t(.+)$/.exec(line); + 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); - const path = resolveNumstatPath(m[3]); - result[path] = { added, deleted }; + 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; } diff --git a/public/session-activity.js b/public/session-activity.js index 9a6b4235..7d2c68e7 100644 --- a/public/session-activity.js +++ b/public/session-activity.js @@ -69,7 +69,8 @@ function setActivity(sessionId, active, via, opts) { } applyActivityClasses(sessionId); - if (!active) notifySessionIdle(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/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 index 7087e3ec..ec27376e 100644 --- a/test/dom-file-panel-changes.test.js +++ b/test/dom-file-panel-changes.test.js @@ -258,6 +258,52 @@ test('zero git invocations while a session stays busy, or idle with no new event } 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 { 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 index 943a7174..60012adc 100644 --- a/test/git-changes-runner.test.js +++ b/test/git-changes-runner.test.js @@ -10,9 +10,14 @@ 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 --------------------------------------- @@ -30,6 +35,18 @@ test('buildRemoteGitCommand: fixed shape, cwd and every arg individually quoted' 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 @@ -73,6 +90,40 @@ test('isSafeGitPath rejects NUL/newline, accepts spaces and unicode', () => { 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', () => { @@ -87,20 +138,22 @@ test('createGitChangesRunner requires an alias for a remote runner', () => { // --- 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[0] + (args.includes('--cached') ? ':cached' : ''); + 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)', async () => { +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\n1 .M N... 100644 100644 100644 abc123 def456 foo.js\n', stderr: '' }, - diff: { code: 0, stdout: '1\t2\tfoo.js\n', stderr: '' }, + 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 }); @@ -110,12 +163,16 @@ test('local runner .status(): three commands, no -C flag (cwd passed via execFil 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'); + 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[0] === 'status' ? { code: 128, stdout: '', stderr: 'fatal: not a git repository' } : { code: 0, stdout: '', stderr: '' } + 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(); @@ -132,7 +189,7 @@ test('local runner .status(): a thrown exec rejects gracefully', async () => { // --- local runner: .diff() -------------------------------------------------- -test('local runner .diff(): unstaged diff args, refuses an unsafe path before calling exec', async () => { +test('local runner .diff(): unstaged diff args carry --literal-pathspecs, refuses an unsafe path before calling exec', async () => { const calls = []; const exec = (args) => { calls.push(args); return Promise.resolve({ code: 0, stdout: 'diff --git a/x b/x\n+line\n', stderr: '' }); }; const runner = createGitChangesRunner({ kind: 'local', cwd: '/repo', exec }); @@ -143,7 +200,7 @@ test('local runner .diff(): unstaged diff args, refuses an unsafe path before ca const good = await runner.diff('src/x.js'); assert.equal(good.ok, true); - assert.deepEqual(calls[0], ['diff', '--', 'src/x.js']); + assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--', 'src/x.js']); }); test('local runner .diff({staged:true}): includes --cached', async () => { @@ -151,17 +208,32 @@ test('local runner .diff({staged:true}): includes --cached', async () => { 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], ['diff', '--cached', '--', 'src/x.js']); + assert.deepEqual(calls[0], ['--literal-pathspecs', 'diff', '--cached', '--', 'src/x.js']); }); -test('local runner .diff(): truncates content past 512 KB and flags it', async () => { - const big = 'a'.repeat(600 * 1024); +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.equal(result.content.length, 512 * 1024); + 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 () => { @@ -174,11 +246,11 @@ test('local runner .diff(): a diff under the cap is not marked truncated', async // --- remote runner: command shape ------------------------------------------- -test('remote runner .status(): three ssh calls, each a full "git -C ..." string', async () => { +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\n', stderr: '' }); + 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 }); @@ -187,19 +259,51 @@ test('remote runner .status(): three ssh calls, each a full "git -C ..." s assert.equal(result.ok, true); assert.equal(commands.length, 3); for (const cmd of commands) { - assert.match(cmd, /^git -C '\/srv\/app' /); + 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 quotes cwd and path, never a shell-interpreted concatenation', async () => { +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' 'diff' '--cached' '--' 'src/weird file.js'"); + 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 () => { diff --git a/test/git-changes-target.test.js b/test/git-changes-target.test.js index 4684bee3..63356a41 100644 --- a/test/git-changes-target.test.js +++ b/test/git-changes-target.test.js @@ -7,7 +7,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { resolveGitChangesTarget } = require('../git-changes-target'); +const { resolveGitChangesTarget, isValidChangesSessionId } = require('../git-changes-target'); function baseDeps(overrides = {}) { return { @@ -31,6 +31,48 @@ test('rejects an empty/missing session id without touching any dependency', () = 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', diff --git a/test/git-changes.test.js b/test/git-changes.test.js index 92ee20f7..b760f512 100644 --- a/test/git-changes.test.js +++ b/test/git-changes.test.js @@ -20,7 +20,7 @@ test('branch header: head, upstream, ahead/behind', () => { '# branch.head main', '# branch.upstream origin/main', '# branch.ab +2 -1', - ].join('\n'); + ].join('\0'); const { branch } = parseStatusPorcelainV2(text); assert.deepEqual(branch, { head: 'main', upstream: 'origin/main', ahead: 2, behind: 1 }); }); @@ -36,7 +36,7 @@ test('ordinary entry: staged and unstaged are independent booleans (mutation tar '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('\n')); + ].join('\0')); const byPath = Object.fromEntries(files.map(f => [f.path, f])); assert.deepEqual( @@ -58,7 +58,7 @@ test('ordinary entry: state reflects the staged code when present, else the unst const { files } = parseStatusPorcelainV2([ '1 A. N... 000000 100644 100644 0000000 abc1234 added.js', '1 .D N... 100644 100644 000000 abc1234 0000000 deleted.js', - ].join('\n')); + ].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'); @@ -77,9 +77,9 @@ test('ignored entries are dropped, not surfaced as files', () => { assert.deepEqual(files, []); }); -test('rename entry: renamed:true and origPath carried through (mutation target: dropping rename handling)', () => { +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\told/path.js' + ['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]; @@ -91,7 +91,7 @@ test('rename entry: renamed:true and origPath carried through (mutation target: 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\ta.js' + ['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' @@ -102,13 +102,28 @@ test('rename entry without renamed handling would collapse to a bare path with n test('copy entry (score C): renamed:true, state "C"', () => { const f = parseStatusPorcelainV2( - '2 C. N... 100644 100644 100644 abc1234 def5678 C90 copy.js\tsource.js' + ['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' @@ -120,7 +135,7 @@ test('unmerged entry: staged and unstaged both true, state carries a letter', () test('unknown/future record types are skipped without throwing', () => { assert.doesNotThrow(() => { - const { files } = parseStatusPorcelainV2('x SOMETHING new-record-type\n? real.js'); + const { files } = parseStatusPorcelainV2(['x SOMETHING new-record-type', '? real.js'].join('\0')); assert.equal(files.length, 1); assert.equal(files[0].path, 'real.js'); }); @@ -135,31 +150,36 @@ test('empty input produces an empty, well-formed result', () => { }); }); -// --- parseNumstat -------------------------------------------------------- +// --- parseNumstat (-z, NUL-separated) ------------------------------------- -test('numstat: plain added/deleted counts keyed by path', () => { - const result = parseNumstat('3\t1\tfoo.js\n0\t5\tbar.js\n'); +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: binary file reports null, not 0 (mutation target: treating "-" as zero)', () => { - const result = parseNumstat('-\t-\timage.png\n'); +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: a full rename ("old => new") is keyed on the new path', () => { - const result = parseNumstat('2\t1\told/name.js => new/name.js\n'); +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: a partial common-directory rename ("prefix/{old => new}/suffix") resolves to the new path', () => { - const result = parseNumstat('4\t0\tsrc/{old => new}/file.js\n'); - assert.deepEqual(result, { 'src/new/file.js': { added: 4, deleted: 0 } }); +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: blank lines and malformed lines are ignored, not throwing', () => { +test('numstat -z: blank/malformed tokens are ignored, not throwing', () => { assert.doesNotThrow(() => { - const result = parseNumstat('\n\nnot a numstat line\n2\t1\tok.js\n'); + const result = parseNumstat(['', 'not a numstat token', '2\t1\tok.js'].join('\0') + '\0'); assert.deepEqual(result, { 'ok.js': { added: 2, deleted: 1 } }); }); }); 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`)); +});