From aeb045c5bbd1e3ebb7c57de3c591463be38dd40d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 14:20:12 +0000 Subject: [PATCH 01/31] Add roadmap items --- context/drafts/agent-mode.md | 131 +++++++++++++++++++++++++++++++++++ context/roadmap.md | 17 +++++ 2 files changed, 148 insertions(+) create mode 100644 context/drafts/agent-mode.md diff --git a/context/drafts/agent-mode.md b/context/drafts/agent-mode.md new file mode 100644 index 0000000..3422dcf --- /dev/null +++ b/context/drafts/agent-mode.md @@ -0,0 +1,131 @@ +# agent-mode — supplied reference material + +Notes, not a design. Captured by `/roadmap` on **2026-09-05** from a brief written by the repository +maintainer. `/feature-plan` turns this into `plans/AGENT-MODE-PLAN.md`; nothing here is a decision. + +## Provenance + +- Source: maintainer brief pasted into `/roadmap`, 2026-09-05. +- External citation in the brief: — the maintainer's claim + about background-session worktree isolation is attributed to that page. **Not independently verified + during capture.** Docs move; re-check the page before the plan depends on the behaviour. +- The `claude agents --json` join (item 3) is marked "I verified this join works" by the maintainer. + Also unverified here — no Claude Code invocation was made while writing these notes. + +## The mechanic that makes it worth building + +Claude Code background sessions normally isolate themselves into `/.claude/worktrees/`, which +would fight this tool's `.worktrees/` layout. Per the brief, that isolation is **skipped when +the session's cwd is already inside a linked git worktree**. So if this CLI creates the worktree and +dispatches the agent with `cwd` set to it, the agent works inside our layout and no nested worktree is +created. + +There is a `worktree.location` setting in Claude Code, but per the brief its own schema says the CLI does +not read it yet — so dispatching into an existing worktree is the only way to control placement today. +That is the whole reason this belongs in *this* tool rather than in agent configuration. + +## Prerequisite — the shell interpolation bug + +`src/lib/base-command.ts:56` interpolates unquoted into a shell string: + +```ts +exec(`${codeEditor} ${path}`, (error) => { ... }) +``` + +Verified present. Any worktree under a directory containing a space is already broken today, independent of +this feature. The fix is `spawn`/`execFile` with an argv array, plus a regression test for a path with a +space. + +The maintainer's instruction is that this is fixed and tested **separately and first** — it stands on its +own. Per [`workflow.md`](../workflow.md)'s "feature or task?" rule it is commit-sized and would not earn a +`history.md` row, so it is an `/orchestrate` task, not part of this entry. It is recorded here because +agent mode makes it urgent: a prompt string is arbitrary user text full of quotes and apostrophes. + +## Wanted, in the order the brief gives + +1. **`worktree branch --agent ""`** — new config value `agent.command` (e.g. `claude --bg`, but + any CLI of that shape). `dispatchAgent(path, prompt)` on `BaseCommand` mirroring `openWorktreePath()`: + split `agent.command` into argv, append the prompt as one argument, `cwd` = worktree path, + `detached: true` + `unref()` so the CLI exits cleanly. Unset `agent.command` prints a message pointing + at `worktree config` rather than erroring. `--agent` / `-a` string flag on `branch`. + - **Ordering is load-bearing:** create worktree → copy env files → dispatch agent. Env files must land + before the agent starts. + - **Maintainer's preference:** `--agent` and the editor are independent; both can fire. Whichever way + the plan decides, it must be documented. + - End-to-end target: `worktree branch --github 47 --agent "implement the issue"`. + - Same flag on `checkout` **only if it falls out cheaply**. Explicitly skippable. + +2. **`worktree list --agents`** — add to `WorktreeListEntry`: + + ```ts + filesChanged?: number; + insertions?: number; + deletions?: number; + agent?: { name: string; pid: number }; + ``` + + - Churn: `git diff --shortstat HEAD` per worktree. + - Agent join: `claude agents --json` emits an array of live sessions each with a `cwd` and a `name`; + join `cwd` to the worktree `path`. Gate behind `agent.command` being configured. **Degrade silently** + if the command is missing or returns non-JSON — this must not become a hard dependency on Claude Code. + - Output stays a printed table, consistent with current `list` formatting. + +3. **Agent-aware `cleanup`** — `safeToRemove` does not know a live agent is mid-edit inside a worktree. + Removing a worktree out from under a running agent is called out as *the worst failure mode in the whole + flow*. Reuse the session join to mark such worktrees unsafe, and require an explicit override to remove + them anyway. + +## Constraints the brief states + +- **Runtime-neutral.** Never hardcode `claude`. The command is a config string so Codex, or anything else + with the same shape, works. **Tests must not depend on Claude Code being installed.** +- **No TUI, no monitor.** `claude agents` already is one. `list --agents` prints and exits. +- **No orchestration.** This tool decides *where* work happens, never *what* the work is. No task + assignment, no queue, no prompt templating. + +## What already holds in this repo + +Read, not recalled — checked 2026-09-05 on `feature/add-agent-mode`. + +| Claim | Status | +|---|---| +| Unquoted shell interpolation of the path in `openWorktreePath()` | confirmed, `src/lib/base-command.ts:51-66` | +| `CONFIG_NAMES` has `codeEditor`, no agent entry | confirmed, `src/lib/constants.ts:1-12` | +| `WorktreeListEntry` carries `ahead`/`behind`/`uncommittedChanges`/`safeToRemove` | confirmed, `src/lib/types.ts:12-19` | +| `branch.run()` already orders create → copy env → open editor | confirmed, `src/commands/branch.ts:182-184` | +| `safeToRemove` reasons only about remote / commits / uncommitted | confirmed, `isSafeToRemove()` at `src/lib/git.ts:153-166` | +| `cleanup` filters on `safeToRemove === true` and has only `--force` | confirmed, `src/commands/cleanup.ts:16-27` | +| `list` has no flags at all today | confirmed, `src/commands/list.ts:6-19` — `--agents` is the first | + +Consequences the brief does not spell out, found while checking: + +- **`codeEditor` is validated by `isValidCommand`** (`src/lib/validators.ts:57-71`, via `commandExists`). + `agent.command` is a *command line*, not a bare command — `claude --bg` would fail that validator as + written. The plan has to decide: validate only the argv head, or skip validation for this key. +- **`config.ts` gates `codeEditor` behind a `maybePrompt` confirm** (`src/commands/config.ts:211-224`). + Following "how `codeEditor` is handled" means an equivalent opt-in confirm for `agent.command`. +- **`gitGetWorktreeList()` already does per-worktree async work in a loop** (`src/lib/git.ts:168-211`). + Churn and the session join are two more calls per worktree on a path that is already serial — worth a + thought about cost when the plan is written. +- The merge-base for churn needs a source branch per worktree. `gitGetWorktreeList` tracks `remote` but + the entry has no record of what the worktree was *branched from*; `defaultSourceBranch` config + (`src/commands/branch.ts:81`) is the likely fallback. Unresolved. + +## Surfaces to update — all verified to exist + +- `docs/src/app/docs/commands/branch/page.mdx`, `list/page.mdx`, `cleanup/page.mdx` +- `docs/src/app/docs/configuration/page.mdx` — for `agent.command` +- `docs/src/app/docs/commands/_meta.ts` — **only** if a new command is added (the brief prefers not) +- `skills/core/SKILL.md` — frontmatter `description` enumerates every command and config value, and + `sources` lists derived-from files. Both need updating; `sources` already lists `src/commands/branch.ts`, + `src/lib/git.ts`, `src/lib/validators.ts`. +- `README.md` — if the feature list changes + +## Definition of done, per the brief + +- `pnpm verify` green (see [`verify.md`](../verify.md) — that file, not this one, names the command). +- A real manual run: create a worktree with `--agent`, confirm the agent starts in the right cwd, confirm + **no `.claude/worktrees/` directory appears inside the repo**. +- The brief's own instruction was "do not commit or push, leave the work in the tree and summarise". That + was addressed to a direct implementation run and is **superseded** by the roadmap flow — `/feature-implement` + commits per phase and updates the ledger in the same commit. diff --git a/context/roadmap.md b/context/roadmap.md index c558f17..c3413cb 100644 --- a/context/roadmap.md +++ b/context/roadmap.md @@ -27,3 +27,20 @@ this, appended under **Features** below: ## Features +### agent-mode — `pending` + +A worktree can be handed straight to a coding agent instead of, or as well as, an editor, and one command +shows what every running agent has changed. + +- **Size:** large — three surfaces (`branch`, `list`, `cleanup`), a new config value, and a runtime-neutral + session join that must degrade silently +- **Doc:** [`drafts/agent-mode.md`](drafts/agent-mode.md) — maintainer brief: the cwd/isolation mechanic, + the four work items, and what already holds in this repo + +### chat-input-multiline — `pending` + +The docs chatbot's message field is a single-line ``, so a longer question cannot contain newlines +and the text scrolls out of sight instead of the field growing. + +- **Size:** small — one component (`ChatInput`) plus its ref type and submit key handling in `ChatForm` +- **Doc:** none yet From a274292923cfca9cc4af71262672b0bcb6d498a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 16:40:46 +0000 Subject: [PATCH 02/31] docs(context): land agent-mode and cleanup-data-loss plans Planning output from /roadmap and /feature-plan, previously uncommitted in the working tree. - agent-mode: promote the maintainer brief in drafts/ to a 7-phase plan and repoint its roadmap Doc field; the draft it replaces is removed. - cleanup-data-loss: new roadmap entry plus a 5-phase plan for the isSafeToRemove data-loss path. - shell-argv-safety: new roadmap entry and draft covering the shell interpolation call-site inventory. All three entries stay `pending`; no feature is activated by this commit. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/drafts/agent-mode.md | 131 -------- context/drafts/shell-argv-safety.md | 123 +++++++ context/plans/AGENT-MODE-PLAN.md | 379 +++++++++++++++++++++ context/plans/CLEANUP-DATA-LOSS-PLAN.md | 426 ++++++++++++++++++++++++ context/roadmap.md | 19 +- 5 files changed, 945 insertions(+), 133 deletions(-) delete mode 100644 context/drafts/agent-mode.md create mode 100644 context/drafts/shell-argv-safety.md create mode 100644 context/plans/AGENT-MODE-PLAN.md create mode 100644 context/plans/CLEANUP-DATA-LOSS-PLAN.md diff --git a/context/drafts/agent-mode.md b/context/drafts/agent-mode.md deleted file mode 100644 index 3422dcf..0000000 --- a/context/drafts/agent-mode.md +++ /dev/null @@ -1,131 +0,0 @@ -# agent-mode — supplied reference material - -Notes, not a design. Captured by `/roadmap` on **2026-09-05** from a brief written by the repository -maintainer. `/feature-plan` turns this into `plans/AGENT-MODE-PLAN.md`; nothing here is a decision. - -## Provenance - -- Source: maintainer brief pasted into `/roadmap`, 2026-09-05. -- External citation in the brief: — the maintainer's claim - about background-session worktree isolation is attributed to that page. **Not independently verified - during capture.** Docs move; re-check the page before the plan depends on the behaviour. -- The `claude agents --json` join (item 3) is marked "I verified this join works" by the maintainer. - Also unverified here — no Claude Code invocation was made while writing these notes. - -## The mechanic that makes it worth building - -Claude Code background sessions normally isolate themselves into `/.claude/worktrees/`, which -would fight this tool's `.worktrees/` layout. Per the brief, that isolation is **skipped when -the session's cwd is already inside a linked git worktree**. So if this CLI creates the worktree and -dispatches the agent with `cwd` set to it, the agent works inside our layout and no nested worktree is -created. - -There is a `worktree.location` setting in Claude Code, but per the brief its own schema says the CLI does -not read it yet — so dispatching into an existing worktree is the only way to control placement today. -That is the whole reason this belongs in *this* tool rather than in agent configuration. - -## Prerequisite — the shell interpolation bug - -`src/lib/base-command.ts:56` interpolates unquoted into a shell string: - -```ts -exec(`${codeEditor} ${path}`, (error) => { ... }) -``` - -Verified present. Any worktree under a directory containing a space is already broken today, independent of -this feature. The fix is `spawn`/`execFile` with an argv array, plus a regression test for a path with a -space. - -The maintainer's instruction is that this is fixed and tested **separately and first** — it stands on its -own. Per [`workflow.md`](../workflow.md)'s "feature or task?" rule it is commit-sized and would not earn a -`history.md` row, so it is an `/orchestrate` task, not part of this entry. It is recorded here because -agent mode makes it urgent: a prompt string is arbitrary user text full of quotes and apostrophes. - -## Wanted, in the order the brief gives - -1. **`worktree branch --agent ""`** — new config value `agent.command` (e.g. `claude --bg`, but - any CLI of that shape). `dispatchAgent(path, prompt)` on `BaseCommand` mirroring `openWorktreePath()`: - split `agent.command` into argv, append the prompt as one argument, `cwd` = worktree path, - `detached: true` + `unref()` so the CLI exits cleanly. Unset `agent.command` prints a message pointing - at `worktree config` rather than erroring. `--agent` / `-a` string flag on `branch`. - - **Ordering is load-bearing:** create worktree → copy env files → dispatch agent. Env files must land - before the agent starts. - - **Maintainer's preference:** `--agent` and the editor are independent; both can fire. Whichever way - the plan decides, it must be documented. - - End-to-end target: `worktree branch --github 47 --agent "implement the issue"`. - - Same flag on `checkout` **only if it falls out cheaply**. Explicitly skippable. - -2. **`worktree list --agents`** — add to `WorktreeListEntry`: - - ```ts - filesChanged?: number; - insertions?: number; - deletions?: number; - agent?: { name: string; pid: number }; - ``` - - - Churn: `git diff --shortstat HEAD` per worktree. - - Agent join: `claude agents --json` emits an array of live sessions each with a `cwd` and a `name`; - join `cwd` to the worktree `path`. Gate behind `agent.command` being configured. **Degrade silently** - if the command is missing or returns non-JSON — this must not become a hard dependency on Claude Code. - - Output stays a printed table, consistent with current `list` formatting. - -3. **Agent-aware `cleanup`** — `safeToRemove` does not know a live agent is mid-edit inside a worktree. - Removing a worktree out from under a running agent is called out as *the worst failure mode in the whole - flow*. Reuse the session join to mark such worktrees unsafe, and require an explicit override to remove - them anyway. - -## Constraints the brief states - -- **Runtime-neutral.** Never hardcode `claude`. The command is a config string so Codex, or anything else - with the same shape, works. **Tests must not depend on Claude Code being installed.** -- **No TUI, no monitor.** `claude agents` already is one. `list --agents` prints and exits. -- **No orchestration.** This tool decides *where* work happens, never *what* the work is. No task - assignment, no queue, no prompt templating. - -## What already holds in this repo - -Read, not recalled — checked 2026-09-05 on `feature/add-agent-mode`. - -| Claim | Status | -|---|---| -| Unquoted shell interpolation of the path in `openWorktreePath()` | confirmed, `src/lib/base-command.ts:51-66` | -| `CONFIG_NAMES` has `codeEditor`, no agent entry | confirmed, `src/lib/constants.ts:1-12` | -| `WorktreeListEntry` carries `ahead`/`behind`/`uncommittedChanges`/`safeToRemove` | confirmed, `src/lib/types.ts:12-19` | -| `branch.run()` already orders create → copy env → open editor | confirmed, `src/commands/branch.ts:182-184` | -| `safeToRemove` reasons only about remote / commits / uncommitted | confirmed, `isSafeToRemove()` at `src/lib/git.ts:153-166` | -| `cleanup` filters on `safeToRemove === true` and has only `--force` | confirmed, `src/commands/cleanup.ts:16-27` | -| `list` has no flags at all today | confirmed, `src/commands/list.ts:6-19` — `--agents` is the first | - -Consequences the brief does not spell out, found while checking: - -- **`codeEditor` is validated by `isValidCommand`** (`src/lib/validators.ts:57-71`, via `commandExists`). - `agent.command` is a *command line*, not a bare command — `claude --bg` would fail that validator as - written. The plan has to decide: validate only the argv head, or skip validation for this key. -- **`config.ts` gates `codeEditor` behind a `maybePrompt` confirm** (`src/commands/config.ts:211-224`). - Following "how `codeEditor` is handled" means an equivalent opt-in confirm for `agent.command`. -- **`gitGetWorktreeList()` already does per-worktree async work in a loop** (`src/lib/git.ts:168-211`). - Churn and the session join are two more calls per worktree on a path that is already serial — worth a - thought about cost when the plan is written. -- The merge-base for churn needs a source branch per worktree. `gitGetWorktreeList` tracks `remote` but - the entry has no record of what the worktree was *branched from*; `defaultSourceBranch` config - (`src/commands/branch.ts:81`) is the likely fallback. Unresolved. - -## Surfaces to update — all verified to exist - -- `docs/src/app/docs/commands/branch/page.mdx`, `list/page.mdx`, `cleanup/page.mdx` -- `docs/src/app/docs/configuration/page.mdx` — for `agent.command` -- `docs/src/app/docs/commands/_meta.ts` — **only** if a new command is added (the brief prefers not) -- `skills/core/SKILL.md` — frontmatter `description` enumerates every command and config value, and - `sources` lists derived-from files. Both need updating; `sources` already lists `src/commands/branch.ts`, - `src/lib/git.ts`, `src/lib/validators.ts`. -- `README.md` — if the feature list changes - -## Definition of done, per the brief - -- `pnpm verify` green (see [`verify.md`](../verify.md) — that file, not this one, names the command). -- A real manual run: create a worktree with `--agent`, confirm the agent starts in the right cwd, confirm - **no `.claude/worktrees/` directory appears inside the repo**. -- The brief's own instruction was "do not commit or push, leave the work in the tree and summarise". That - was addressed to a direct implementation run and is **superseded** by the roadmap flow — `/feature-implement` - commits per phase and updates the ledger in the same commit. diff --git a/context/drafts/shell-argv-safety.md b/context/drafts/shell-argv-safety.md new file mode 100644 index 0000000..1aff437 --- /dev/null +++ b/context/drafts/shell-argv-safety.md @@ -0,0 +1,123 @@ +# shell-argv-safety — supplied reference material + +Notes, not a design. Captured by `/roadmap` on **2026-09-05**. `/feature-plan` turns this into +`plans/SHELL-ARGV-SAFETY-PLAN.md`; nothing here is a decision. + +## Provenance + +- Source: maintainer, pasted into `/roadmap` on 2026-09-05. Originated as analysis done while planning + `agent-mode`, where it is recorded as risk R1 in `plans/AGENT-MODE-PLAN.md`. +- The **original** maintainer brief for `agent-mode` called this a single-site, commit-sized `/orchestrate` + task citing only `src/lib/base-command.ts:56`. The inventory below is why it was filed as its own entry + instead. +- Every citation re-verified against the tree on 2026-09-05, branch `feature/add-agent-mode`. Two claims in + the supplied material were adjusted — see "Corrections". + +## The shape of the problem — one chokepoint, one bypass + +The supplied material framed this as "at least seven sites across two files". That count is right but the +framing understates how tractable it is. `grep -rn 'exec(\|execSync\|spawn(\|execFile' src/` over non-test +sources returns exactly **two** hits: + +| Site | What it is | +|---|---| +| `src/lib/cli.ts:17` | `exec(cmd, …)` inside `cmd()` — **the single shell boundary every git call funnels through** | +| `src/lib/base-command.ts:56` | `exec(\`${codeEditor} ${path}\`, …)` — the one call that **bypasses `cmd()`** and shells out directly | + +So this is not seven independent bugs. It is one helper with a string-shaped contract, one caller that +skipped the helper, and a set of call sites that interpolate into that contract. + +## Why the `cd` workarounds exist + +`CmdOptions` is `{ debug?: boolean }` (`src/lib/cli.ts:3-5`) — **there is no `cwd` option.** That absence is +the direct cause of five of the interpolation sites, which all shell out to `cd` to reach a worktree: + +``` +src/lib/git.ts:86 `cd ${branchPath} && git rev-list --count @{u}..HEAD` +src/lib/git.ts:95 `cd ${branchPath} && git rev-list --count HEAD..@{u}` +src/lib/git.ts:103 `cd ${branchPath} && git status -s` +src/lib/git.ts:231 `cd ${gitRootPath}` ─┐ composed at :241 into one chained +src/lib/git.ts:239 `cd ${currentPath}` ─┘ `${cdRoot} && ${gitFetch} && ${addWorktree} && ${gotoBack}` +``` + +**Adding a `cwd` option deletes these rather than escaping them.** That is the important consequence: the +fix is mostly subtraction, and `child_process.execFile` already takes `{ cwd }`. + +## Full inventory of `cmd()` call sites + +**18** non-test call sites (a naive grep finds 14 — four are formatted across lines and need +`grep -n '\bcmd('` to catch: `git.ts:85, 94, 108, 260`). + +**Ten pass a static string** and are already safe: `git.ts:28, 32, 36, 65, 71, 76, 108, 133` and +`integrations/github.ts:113, 147`. + +**Eight interpolate**, and are the work: + +| Site | Interpolates | Source of the value | +|---|---|---| +| `git.ts:17` | `${name}` | a `ConfigName` from a fixed union — low risk | +| `git.ts:24` | `${name}`, `${value}` into `git config … "${value}"` | **arbitrary user input**, double-quoted only | +| `git.ts:86` | `${branchPath}` | filesystem path | +| `git.ts:95` | `${branchPath}` | filesystem path | +| `git.ts:103` | `${branchPath}` | filesystem path | +| `git.ts:241` | four commands chained with `&&`, composed from `231`, `236-237`, `239` — carrying `${branchName}`, `${worktreePath}`, `${sourceBranch}`, `${gitRootPath}`, `${currentPath}` | branch name + filesystem paths | +| `git.ts:261-263` | `${branchName}` ×2 into `git worktree remove` / `git branch -D` | branch name | +| `cli.ts:34` | `${checkCommand} ${baseCommand}` | `commandExists`, already head-split | + +Plus `src/lib/base-command.ts:56` — the direct-`exec` bypass, which is not a `cmd()` call site at all. + +## Demonstrated, not asserted + +Run on 2026-09-05 against a directory whose path contains a space: + +``` +the pattern the CLI builds today: + exec(`cd ${branchPath} && git status -s`) + → FAILS: Command failed: cd /…/tmp/space demo && git status -s + +the same call with an argv array and a cwd option: + execFile("git", ["status", "-s"], { cwd: branchPath }) + → ok — no shell, no quoting +``` + +## Corrections to the supplied material + +- **`git.ts:94` should be `git.ts:95`.** Line 94 is `const countStr = await cmd(`; the interpolated string + is on the following line. Line 86 was cited correctly because that call is formatted differently. +- **"Every subprocess call … builds a shell string" is too broad.** Ten of the eighteen `cmd()` call sites + pass static strings with nothing interpolated. The defect is in the *contract* — `cmd()` accepts a string + and runs it through a shell — not in every caller. + +## What already holds in this repo + +| Claim | Status | +|---|---| +| `cmd()` is the only shell boundary for git calls | confirmed, `src/lib/cli.ts:7-25` | +| `base-command.ts:56` bypasses `cmd()` and calls `exec` directly | confirmed | +| `CmdOptions` has no `cwd` | confirmed, `src/lib/cli.ts:3-5` | +| Five interpolation sites exist only to work around that | confirmed, `git.ts:86, 95, 103, 231, 239` | +| `git.ts:241` chains four commands with `&&` in one shell string | confirmed | +| `commandExists` already splits on whitespace and checks only the head | confirmed, `src/lib/cli.ts:27-39` | +| No `src/lib/base-command.test.ts` exists | confirmed — a new file either way | +| `src/integrations/` contains no `exec`/`spawn` of its own | confirmed | +| A path containing a space fails today | **demonstrated above** | + +## Not decided here + +- Whether `cmd()` changes signature to `(file, args[], opts)`, gains an overload, or is replaced by a new + helper with the old one kept for static strings. +- What happens to the chained command at `git.ts:241` — four sequential `execFile` calls with `{ cwd }`, + or keep one shell call with proper quoting. The `cd`-back-afterwards half becomes unnecessary with `cwd`. +- Whether branch names need validation as well as escaping. `isValidBranchName` + (`src/lib/validators.ts:22-55`) already rejects spaces and several metacharacters, but it is not applied + on every path a branch name reaches `cmd()` by. +- Whether `debug: true` in `CmdOptions` still makes sense once commands are argv arrays. + +## Relationship to other entries + +- **`agent-mode`** records this as risk R1. Its Phase 1 stores `agent.command` — a value containing spaces — + through `gitSetConfigValue` (`git.ts:24`), and its Phase 2 edits `base-command.ts`. Doing this first means + agent mode is not built on the broken contract. Note that `agent-mode`'s own decision D2 already requires + `spawn` with an argv array for agent dispatch, so that one path is safe regardless. +- **`cleanup-data-loss`** touches `git.ts` too (`isSafeToRemove`, and `gitNukeWorktreeCmd` at `261-263` is + in this entry's table). The two overlap in that function; sequencing them avoids a conflict. diff --git a/context/plans/AGENT-MODE-PLAN.md b/context/plans/AGENT-MODE-PLAN.md new file mode 100644 index 0000000..6094635 --- /dev/null +++ b/context/plans/AGENT-MODE-PLAN.md @@ -0,0 +1,379 @@ +# agent-mode Plan + +Written 2026-09-05. Hands a freshly created worktree to a coding agent, and makes the other worktree +commands aware that an agent may be living inside one. The `agent-mode` entry in +[`../roadmap.md`](../roadmap.md) is where this feature's status lives. + +**Phase status lives in §6.1 of this document, and nowhere else.** + +Built on the maintainer brief captured by `/roadmap` on 2026-09-05, which this document replaces. That +brief's provenance is carried forward in §0 so nothing it recorded is lost. + +--- + +## 0. Provenance of the source material + +- **Source:** maintainer brief pasted into `/roadmap`, 2026-09-05. +- **External citation in the brief:** — the maintainer's claim + about background-session worktree isolation is attributed to that page. **Still not independently + verified.** See §8 Q1. +- **The `claude agents --json` join was marked "I verified this join works" by the maintainer.** That claim + **is now verified here** — see §1 and D4. +- The brief's closing instruction was "do not commit or push, leave the work in the tree and summarise." + That was addressed to a direct implementation run and is **superseded** by the roadmap flow: + `/feature-implement` commits per phase and updates the ledger in the same commit. + +## 1. Why + +Claude Code background sessions isolate themselves into `/.claude/worktrees/`, which fights this +tool's `.worktrees/` layout. Per the brief, that isolation is **skipped when the session's cwd +is already inside a linked git worktree** — so if this CLI creates the worktree and dispatches the agent +with `cwd` set to it, the agent works inside our layout and no nested worktree appears. There is a +`worktree.location` setting in Claude Code, but per the brief its own schema says the CLI does not read it +yet, so dispatching into an existing worktree is the only way to control placement today. **That is why this +belongs in this tool rather than in agent configuration.** + +Verified on 2026-09-05, on this machine, `claude` 2.1.261: + +``` +$ claude agents --json | head +[ + { "pid": 9187, "cwd": "/Users/baldur/Documents/Job seeker", "kind": "interactive", + "startedAt": 1788600791123, "sessionId": "53fac4da-…", "name": "job-seeker-1f" }, + … + { "pid": 33471, "id": "23f50fae", + "cwd": "/Users/baldur/Development/northguild/worktree/worktree.worktrees/feature/add-agent-mode", + "kind": "background", … } ] +``` + +`claude agents --help` documents `--json` as *"Print active sessions (interactive and background) as a JSON +array and exit (for scripting; does not require a TTY)"*, plus `--cwd ` and `--all`. The join the +brief wanted is real, and the last entry above is a background session whose `cwd` is a worktree in this +repo's own layout — with no `.claude/worktrees/` anywhere under +`/Users/baldur/Development/northguild/worktree` (`find … -name worktrees -path '*.claude*'` returns +nothing). + +**Three things the live JSON shows that the brief did not**, each of which forces a decision below: + +1. A `kind` field separating `"interactive"` from `"background"`. Interactive sessions include the user's + own terminal — a naive join reports "an agent is here" when a human simply has Claude open. (D5) +2. `status` and `state` fields on background sessions, e.g. `"status": "idle", "state": "done"`. A finished + session lingers in the default listing, so "block cleanup whenever a session matches" would wedge a + worktree permanently. (D6) +3. `pid`, `sessionId` and a short `id` are all present. The brief asked for `{ name, pid }`; that is + satisfiable exactly as written. + +## 2. Constraints + +From the brief, unchanged: + +- **Runtime-neutral.** Never hardcode `claude`. The command is a config string, so Codex or anything of the + same shape works. **Tests must not depend on Claude Code being installed.** +- **No TUI, no monitor.** `claude agents` already is one. `list --agents` prints and exits. +- **No orchestration.** This tool decides *where* work happens, never *what* the work is. No task + assignment, no queue, no prompt templating. +- **Degrade silently.** A missing agent command, a non-zero exit, or non-JSON output yields no agent data + and no error. This must never become a hard dependency on Claude Code. + +From this repository ([`../stack.md`](../stack.md)): + +- ESM throughout; relative imports keep the `.js` extension. `export default` only in `src/commands/*.ts`. +- Tests colocated as `*.test.ts`, vitest. +- Console output is chalk-styled and TTY-dependent; assertions on printed text rely on the `FORCE_COLOR: "0"` + pin in `vitest.config.ts`. +- Never add a file named `biome.json` or `biome.jsonc` anywhere in the tree. + +## 3. Decisions + +**D1. `agent.command` holds a full command line, validated on its argv head only.** A new +`isValidCommandLine` in `src/lib/validators.ts` splits on whitespace and checks the first token, wired into +`isValidConfigValue`'s switch. *Rejected:* reusing `isValidCommand` — `commandExists` (`src/lib/cli.ts:27-39`) +does already take `command.split(" ")[0]`, so `claude --bg` would in fact pass, but `isValidCommand`'s error +message quotes the whole string back (`Command not found: claude --bg`), which is a misleading thing to show +a user. *Rejected:* no validation at all — a typo would then fail silently at dispatch time, long after the +config command exited 0. + +**D2. Dispatch uses `spawn` with an argv array, never `exec`.** `dispatchAgent(path, prompt)` splits +`agent.command` into argv, appends the prompt as **one** argument, sets `cwd` to the worktree path, and uses +`detached: true` + `unref()` so the CLI exits cleanly. *Rejected:* mirroring `openWorktreePath`'s +`exec(\`${cmd} ${path}\`)` — a prompt is arbitrary user text full of quotes and apostrophes, and passing it +through a shell string is an injection hole, not merely a quoting bug. See §5 R1. + +**D3. `--agent` and the editor are independent; both fire.** Order in `run()` becomes: create worktree → +copy env files → dispatch agent → open editor. **The env-before-agent edge is load-bearing** (the brief says +so explicitly); editor-last simply preserves the existing final call and keeps `openWorktreePath`'s +`✔ Worktree created in …` as the last line when no editor is configured. *Rejected:* making `--agent` +suppress the editor — the maintainer's stated preference is independence. + +**D4. One `claude agents --json` invocation per command run, joined in-process on `cwd`.** *Rejected:* +`claude agents --cwd ` per worktree — `gitGetWorktreeList()` (`src/lib/git.ts:168-211`) is already a +serial loop doing 3 subprocess calls per worktree; a fourth spawn per iteration for data one call returns +whole is the wrong trade. + +**D5. The join keeps `kind`, and the two consumers use it differently.** `list --agents` reports background +sessions — the ones this tool dispatched. `cleanup` blocks on **any** session, interactive included: its job +is not to delete a directory a human is sitting in. *Rejected:* filtering to background everywhere — that +makes `cleanup` delete the worktree out from under an open editor session, which is the failure mode the +brief calls the worst in the whole flow. + +**D6. Liveness is `state !== "done"`, and an absent or unrecognised `state` counts as live.** Fail safe: a +session whose shape we do not recognise blocks removal rather than being ignored. *Rejected:* treating every +listed session as live — `"state": "done"` sessions persist in the listing, so that would wedge a worktree +until the user hunted down a finished PID. + +**D7. Churn's merge-base comes from `defaultSourceBranch`, falling back to `origin/main`.** A +`WorktreeListEntry` has no record of what it was branched from (`gitGetWorktreeList` tracks `remote`, not +origin-of-branch), and `branch.ts:81` already uses exactly this fallback chain. If `git merge-base` fails, +churn is **omitted**, not defaulted to zero — an absent number and a genuine zero are different facts. + +**D8. `list --agents` extends the existing bullet-list output, and does not become a table.** The brief says +"output stays a printed table, consistent with current `list` formatting" — **the brief is wrong about the +current formatting.** `src/commands/list.ts:16-18` prints `- ${worktreeListEntryToListName(wt)}`, a bullet +list, and `worktreeListEntryToListName` (`src/lib/utils.ts:24-49`) builds a parenthesised details string. +Churn and agent facts append to that same `details` array. **`worktreeListEntryToListName` is shared with +`src/commands/cleanup.ts:40`**, so it takes an options argument to control which details render; without one, +this change silently rewrites `cleanup`'s output too and breaks `cleanup.test.ts`. + +**D9. `checkout --agent` is in scope.** The brief marked it "only if it falls out cheaply — explicitly +skippable." It does fall out cheaply: `dispatchAgent` lives on `BaseCommand`, and `checkout.run()` +(`src/commands/checkout.ts:65-71`) already has the identical create → copy env → open sequence. The only +real cost is that `checkout` has no `static override flags` block today and needs one. + +## 4. Design + +**Config.** `agent.command` joins `CONFIG_NAMES` (`src/lib/constants.ts`). `config.ts` gates it behind a +`maybePrompt` confirm exactly as `codeEditor` is gated at `src/commands/config.ts:211-224`. Unset is a +normal state: `dispatchAgent` prints a line pointing at `worktree config` and returns, rather than erroring. + +**Dispatch.** `dispatchAgent(path, prompt)` on `BaseCommand`, sibling to `openWorktreePath`: + +``` +agent.command ──split──▶ [bin, ...args] + spawn(bin, [...args, prompt], { cwd: path, detached: true, stdio: "ignore" }) + .unref() +``` + +**Session join.** A new `src/lib/agent.ts` owns everything that knows an agent CLI exists: + +- `getAgentSessions()` — returns `AgentSession[]` (`{ name, pid, cwd, kind, state? }`), or `[]` on any + failure. Gated on `agent.command` being configured; parses stdout as JSON inside a `try`. +- `findSessionForPath(sessions, path)` — the `cwd` join. +- `isSessionLive(session)` — D6. + +Nothing outside this file knows the JSON shape, so swapping runtimes touches one module. + +**List.** `WorktreeListEntry` (`src/lib/types.ts:12-19`) gains the four optional fields the brief specifies: +`filesChanged?`, `insertions?`, `deletions?`, `agent?: { name: string; pid: number }`. `gitGetWorktreeList` +takes an options flag so the extra work happens only when `list --agents` asks for it — `list` today is not +paying for churn and must not start. + +**Cleanup.** `isSafeToRemove` (`src/lib/git.ts:153-166`) gains a live-agent clause. `cleanup` gets an +override flag; `--force` alone must **not** be it, since `--force` today means "skip the confirmation +prompt", not "override a safety verdict". + +## 5. Risks + +**R1 — the shell-interpolation prerequisite is wider than the brief says.** The brief cites +`src/lib/base-command.ts:56` and calls it an `/orchestrate` task to be fixed separately and first. Verified +present, and **it is not the only one**: `src/lib/git.ts` interpolates unquoted paths at lines 86, 94, 103, +231 and 239 (`cd ${branchPath} && …`), and `gitSetConfigValue` at `src/lib/git.ts:23-25` interpolates a +config *value* into `git config … "${value}"` — which `agent.command` will now flow through. Any worktree +under a path containing a space is already broken today. **This plan does not depend on that fix** (D2 keeps +dispatch off the shell entirely), but shipping `agent.command` through `gitSetConfigValue` adds a value with +spaces in it to a code path that quotes badly. See §8 Q4. + +**R2 — the isolation mechanic is load-bearing and second-hand.** If the cwd-inside-a-worktree exemption does +not hold, `--agent` produces a nested `.claude/worktrees/` and the feature's premise fails. It shows up +immediately as a directory appearing inside the repo. Response: this is the first thing Phase 2's manual +check looks for (§7), before any of Phases 3–7 build on it. + +**R3 — the agent JSON shape is undocumented.** `kind`, `state` and `status` appear in output but not in +`claude agents --help`. A future version could rename them. Response: D6 fails safe, `getAgentSessions` +returns `[]` on any parse failure, and every field the code reads is optional. + +**R4 — cost on the `list` path.** `gitGetWorktreeList` is serial and already runs 3 subprocess calls per +worktree. Churn adds a fourth. Response: gated behind `--agents` (§4), so default `list` is unchanged. + +**R5 — a stale PID.** A session's process can die between the JSON call and the removal. The window is +small and the consequence is a spurious block, not data loss. Accepted; not mitigated. + +## 6. Phases + +### 6.1 Status ledger + +| # | Phase | Status | Depends on | Note | +|---|---|---|---|---| +| 1 | `agent.command` config value | not started | — | | +| 2 | `dispatchAgent` + `--agent` on `branch` and `checkout` | not started | 1 | | +| 3 | Agent session join module | not started | 1 | | +| 4 | Churn stats on the worktree entry | not started | — | | +| 5 | `list --agents` | not started | 3, 4 | | +| 6 | Agent-aware `cleanup` | not started | 3 | | +| 7 | Generated-surface sweep | not started | 2, 5, 6 | | + +Status is one of `not started`, `in progress`, `blocked`, `done`. `done` only when committed and verified, +and whoever finishes a phase updates the row in the same commit. + +**Exactly one table in this document has these columns.** Do not add a second phase table — a +differently-shaped one nearby is a decoy that gets read by mistake. + +### 6.2 The phases + +#### Phase 1 — `agent.command` config value + +**Files:** `src/lib/constants.ts`, `src/lib/validators.ts`, `src/lib/validators.test.ts`, +`src/commands/config.ts`, `src/commands/config.test.ts`, `docs/src/app/docs/configuration/page.mdx` + +**Scope:** Add `agent.command` to `CONFIG_NAMES`. Add `isValidCommandLine` and wire it into +`isValidConfigValue`'s switch (D1). Add a `maybePrompt`-gated prompt in `renderInput`, following the +`codeEditor` block at `src/commands/config.ts:211-224`. Document the value. + +**Done when:** `worktree config agent.command "claude --bg"` stores it and `worktree config --list` shows it; +`worktree config agent.command "nope-not-a-binary"` is rejected; `config.test.ts` covers both. + +#### Phase 2 — `dispatchAgent` + `--agent` on `branch` and `checkout` + +**Files:** `src/lib/base-command.ts`, `src/commands/branch.ts`, `src/commands/branch.test.ts`, +`src/commands/checkout.ts`, `src/commands/checkout.test.ts`, +`docs/src/app/docs/commands/branch/page.mdx`, `docs/src/app/docs/commands/checkout/page.mdx` + +**Scope:** `dispatchAgent(path, prompt)` on `BaseCommand` per D2 — `spawn`, argv array, prompt as one +argument, `cwd` set, `detached` + `unref()`. Unset `agent.command` prints a pointer at `worktree config` and +returns. Add the `--agent` / `-a` string flag to `branch` and `checkout`, called after +`copyEnvFilesFromRootPath` and before `openWorktreePath` (D3). `checkout` needs a new `static override flags` +block (D9). **This phase adds `src/lib/base-command.test.ts`, which does not exist today.** + +**Done when:** `worktree branch --github 47 --agent "implement the issue"` creates the worktree, copies env +files, and starts the agent with cwd set to the worktree; tests assert the spawn argv — including a prompt +containing a single quote and a double quote — without invoking a real agent binary. + +#### Phase 3 — Agent session join module + +**Files:** `src/lib/agent.ts` (new), `src/lib/agent.test.ts` (new), `src/lib/types.ts` + +**Scope:** `getAgentSessions`, `findSessionForPath`, `isSessionLive` per §4 and D4/D5/D6. Add the +`AgentSession` type and the `agent?: { name: string; pid: number }` field to `WorktreeListEntry`. Every +failure path returns `[]`. + +**Done when:** tests cover a well-formed array, a non-zero exit, non-JSON stdout, an unset `agent.command`, +and a session with no `state` — all without Claude Code installed, per §2. + +#### Phase 4 — Churn stats on the worktree entry + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts`, `src/lib/types.ts` + +**Scope:** Add `filesChanged?`, `insertions?`, `deletions?` to `WorktreeListEntry`. Add a +`gitGetChurnStats(path, sourceBranch)` parsing `git diff --shortstat HEAD`, with the D7 +merge-base resolution and omit-on-failure behaviour. Extend `gitGetWorktreeList`'s options so the work is +opt-in (R4). + +**Done when:** `gitGetWorktreeList({ withChurn: true })` returns the three numbers for a worktree with +commits; a worktree whose merge-base cannot be resolved returns the entry with the fields absent; default +`gitGetWorktreeList()` issues no extra subprocess call. + +#### Phase 5 — `list --agents` + +**Files:** `src/commands/list.ts`, `src/commands/list.test.ts`, `src/lib/utils.ts`, +`src/lib/utils.test.ts`, `docs/src/app/docs/commands/list/page.mdx` + +**Scope:** Add the `--agents` flag — `list`'s first (`src/commands/list.ts:6-19` has no flags today). When +set, request churn and perform the session join, and render both in the existing bullet-list details string +per D8, via a new options argument to `worktreeListEntryToListName` so `cleanup`'s output is untouched. + +**Done when:** `worktree list --agents` prints churn and the agent name per worktree; `worktree list` +output is byte-identical to today's; `cleanup.test.ts` passes unmodified. + +#### Phase 6 — Agent-aware `cleanup` + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts`, `src/commands/cleanup.ts`, +`src/commands/cleanup.test.ts`, `docs/src/app/docs/commands/cleanup/page.mdx` + +**Scope:** Teach `isSafeToRemove` about a live agent (D5, D6) and add the explicit override flag to +`cleanup` — distinct from `--force` (§4). A worktree hosting a live session is excluded from the sweep and +named as skipped rather than silently dropped. + +**Done when:** a worktree with a live session in its `cwd` is excluded from `cleanup` and reported as +skipped; the override includes it; a session with `state: "done"` does not block; tests cover all three +without a real agent binary. + +#### Phase 7 — Generated-surface sweep + +**Files:** `skills/core/SKILL.md`, `README.md` + +**Scope:** Update `SKILL.md`'s frontmatter `description` (it enumerates every command and config value) and +its `sources` list, plus the body. Update `README.md` if the feature list changed. **No new command is +added, so `docs/src/app/docs/commands/_meta.ts` is not touched.** + +**Done when:** `SKILL.md` names `agent.command`, `--agent` and `list --agents`; `pnpm sync-version` leaves +no diff (`ci.yml` hard-fails on drift via `git diff --exit-code`). + +## 7. Verification + +[`../verify.md`](../verify.md) names the commands — this file does not repeat them. Beyond Gate 1: + +**The manual run, from the brief's own definition of done.** Required at Phase 2, before later phases build +on the premise (R2): + +1. `worktree branch --agent ""` in a repo with `agent.command` set. +2. Confirm the agent starts with cwd set to the new worktree — `claude agents --json` shows a session whose + `cwd` is that path. +3. **Confirm no `.claude/worktrees/` directory appears inside the repo.** This is the load-bearing check. + +**At Phase 6**, confirm by hand that a worktree with a live agent survives `cleanup` and is reported as +skipped. Removing a worktree out from under a running agent is the worst failure mode in this flow, and it +is not one to discover from a unit test alone. + +## 8. Open questions + +- **Q1 — the isolation mechanic is still second-hand.** was + not fetched while writing this plan. Everything in §1 that is verified was verified by running the CLI, + not by reading that page; the *rule* that isolation is skipped inside a linked worktree remains the + maintainer's claim. §7 step 3 is what would falsify it. +- **Q2 — should `list --agents` show interactive sessions?** D5 says no for `list`, yes for `cleanup`. That + asymmetry is defensible but it means `list --agents` will not show a worktree where the user has Claude + open interactively, while `cleanup` refuses to remove it. If that reads as inconsistent in use, the fix is + to show interactive sessions in `list` with a marker. +- **Q3 — is `state: "done"` a stable field?** It is absent from `claude agents --help`. D6 fails safe, so a + rename degrades to "everything blocks cleanup" rather than "nothing does" — annoying, not dangerous. +- **Q4 — does the `/orchestrate` shell fix land before or after this?** R1 found the problem is wider than + the brief's single citation, including `gitSetConfigValue`, through which `agent.command` will flow. This + plan does not block on it, but the phases and that task touch `src/lib/git.ts` and + `src/lib/base-command.ts` in overlapping places, so doing it first avoids a conflict. +- **Q5 — what should `--agent` with no `agent.command` configured do on a *scripted* run?** The brief says + print a message rather than error, which is right interactively. In CI, a silently-not-dispatched agent + looks like success. Not resolved; the phases implement the brief's stated behaviour. + +## 9. Surfaces to update — all verified to exist + +- `docs/src/app/docs/commands/branch/page.mdx`, `checkout/`, `list/`, `cleanup/` — all present. +- `docs/src/app/docs/configuration/page.mdx` — for `agent.command`. +- `docs/src/app/docs/commands/_meta.ts` — **not touched**; no new command is added. +- `skills/core/SKILL.md` — frontmatter `description` enumerates every command and config value; `sources` + already lists `src/commands/branch.ts`, `src/lib/git.ts`, `src/lib/validators.ts`. +- `README.md` — if the feature list changes. + +## 10. What already holds in this repo + +Read, not recalled — checked 2026-09-05 on `feature/add-agent-mode`. The first seven rows are the brief's +own table, re-verified; the rest were found while writing this plan. + +| Claim | Status | +|---|---| +| Unquoted shell interpolation of the path in `openWorktreePath()` | confirmed, `src/lib/base-command.ts:51-66` | +| `CONFIG_NAMES` has `codeEditor`, no agent entry | confirmed, `src/lib/constants.ts:1-12` | +| `WorktreeListEntry` carries `ahead`/`behind`/`uncommittedChanges`/`safeToRemove` | confirmed, `src/lib/types.ts:12-19` | +| `branch.run()` already orders create → copy env → open editor | confirmed, `src/commands/branch.ts:182-184` | +| `safeToRemove` reasons only about remote / commits / uncommitted | confirmed, `isSafeToRemove()` at `src/lib/git.ts:153-166` | +| `cleanup` filters on `safeToRemove === true` and has only `--force` | confirmed, `src/commands/cleanup.ts:16-27` | +| `list` has no flags at all today | confirmed, `src/commands/list.ts:6-19` — `--agents` is the first | +| `claude agents --json` exists and emits `cwd` + `name` per session | **confirmed by running it**, `claude` 2.1.261 — see §1 | +| `list` output is a bullet list, **not** a table as the brief states | confirmed, `src/commands/list.ts:16-18` + `src/lib/utils.ts:24-49` | +| `worktreeListEntryToListName` is shared by `list` and `cleanup` | confirmed, `src/commands/cleanup.ts:40` | +| `checkout` has no `flags` block at all | confirmed, `src/commands/checkout.ts:12-20` | +| `commandExists` already splits on whitespace and checks only the head | confirmed, `src/lib/cli.ts:27-39` | +| `config.ts` gates `codeEditor` behind a `maybePrompt` confirm | confirmed, `src/commands/config.ts:211-224` | +| `gitGetWorktreeList()` does 3 serial subprocess calls per worktree | confirmed, `src/lib/git.ts:168-211` | +| No `src/lib/base-command.test.ts` exists | confirmed, `ls src/lib/` — Phase 2 creates it | +| `gitSetConfigValue` interpolates the value into a shell string | confirmed, `src/lib/git.ts:23-25` — see R1 | +| Unquoted `cd ${branchPath}` in five more places | confirmed, `src/lib/git.ts:86,94,103,231,239` — see R1 | +| No `.claude/worktrees/` exists under this repo today | confirmed, `find` returned nothing — the §7 baseline | diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/plans/CLEANUP-DATA-LOSS-PLAN.md new file mode 100644 index 0000000..beb0126 --- /dev/null +++ b/context/plans/CLEANUP-DATA-LOSS-PLAN.md @@ -0,0 +1,426 @@ +# Cleanup Data Loss Plan + +Written 2026-09-05. Makes `safeToRemove` mean what its name says, so `worktree cleanup` stops force-removing +worktrees that hold uncommitted work. The `cleanup-data-loss` entry in [`../roadmap.md`](../roadmap.md) is +where this feature's status lives. + +**Phase status lives in §6.1 of this document, and nowhere else.** + +--- + +## 1. Why + +`isSafeToRemove` (`src/lib/git.ts:153-166`) has three branches and returns on the first that matches: + +```ts +if (!wt.pathExists) return true; // 154-157 +if (wt.remote && !wt.remoteExists) return true; // 158-161 ← returns before line 162 +if (!wt.remote && !wt.ahead && !wt.behind && wt.uncommittedChanges === 0) return true; // 162-165 +``` + +**The second branch returns before the third is ever reached, and the third is the only one that consults +`uncommittedChanges`.** A worktree tracking a deleted remote branch is therefore classified `safeToRemove` +no matter how much uncommitted work sits in it. + +Both removal surfaces then act on that verdict without re-checking: + +- `cleanup` filters on `safeToRemove === true` (`src/commands/cleanup.ts:27`) and removes every match via + `gitNukeWorktreeCmd(wt.branchName, { force: true })` (`src/lib/git.ts:354`). That `force: true` is + unconditional — it is not derived from the flag. +- `remove`'s multi-select path sorts the same field into an "Inactive branches (Safe to delete)" group + (`src/commands/remove.ts:36-48`) and skips its are-you-sure prompt entirely when every selection is + `safeToRemove` (`src/commands/remove.ts:81`), then removes through the same forced sweep. + +`git worktree remove --force` does not stage, stash or back anything up. The work is gone. + +**Reproduction** + +1. Create a worktree from a remote branch (`worktree checkout some-branch`). +2. Get its PR merged so the remote branch is deleted, or delete it on the remote by hand. +3. Keep editing in the worktree. Do not commit. +4. Run `worktree cleanup`. + +**Who is exposed.** `remote` is only non-empty when the branch has an upstream (`src/lib/git.ts:178` — +`tracking.find(...)?.remote ?? ""`). `branch` creates worktrees with `--no-track` (`src/lib/git.ts:237`), so +those have no `remote` and fall through to the safe third branch. `checkout` creates them with `--track` +(`src/lib/git.ts:236`). **Worktrees made by `worktree checkout` are the exposed ones** — exactly the ones +most likely to have had a PR merged and their remote branch deleted. This is a mainline path, not an exotic +one. + +**What the user sees today.** The information is not hidden, but it is easy to miss. In the default path +`cleanup.ts:39-41` prints each candidate through `worktreeListEntryToListName`, which appends an +uncommitted-changes count (`src/lib/utils.ts:38-42`): + +``` +- feature/thing (Remote removed, 3 uncommitted changes) +Are you sure you want to delete them? +``` + +That is one bulk yes/no covering every candidate at once, so a single worktree with work in it is easy to +miss in a list. `--force` skips the display entirely (`cleanup.ts:34,46-48`) — it takes the `else` branch +and never prints the candidates. + +**The deeper problem is the classification.** `safeToRemove` is the name of a safety verdict, and it returns +`true` for a worktree that is not safe to remove. Anything that trusts the field inherits the bug, which is +why the fix belongs in the predicate rather than in either caller. + +**There is no test coverage for any of this.** `src/lib/git.test.ts` contains four `describe` blocks — git +branch parsing, git config, git root path, git status and tracking helpers — and none of them exercise +`isSafeToRemove` or `gitGetWorktreeList`. `cleanup.test.ts` and `remove.test.ts` hand-write `safeToRemove` +into their fixtures (`cleanup.test.ts:44,56,68`), so they assert against a verdict they supply themselves +and never run the predicate. + +**The precedent for what correct looks like** is already in the file. `gitRemoveWorktree`'s interactive +single-worktree path (`src/lib/git.ts:303-326`) prompts specifically on `ahead`, then specifically on +`uncommittedChanges`, each defaulting to `false`, and only passes `force` once the user has confirmed +against that specific hazard. The two paths should agree about what "safe" means. + +## 2. Constraints + +- **`safeToRemove` has two consumers, not one** — `cleanup.ts:27` and `remove.ts:36-48,81`. Any fix must + leave both correct; a patch applied in `cleanup` alone would leave `remove` removing the same worktrees + without a prompt, and would put two copies of one safety rule in the tree. +- **`WorktreeListEntry.safeToRemove` is an exported interface member** (`src/lib/types.ts:18`, + `safeToRemove?: boolean`) in a published npm package. Changing its type is a breaking change to anything + importing it; see D4. +- **`ahead` and `behind` are only computed when `pathExists && remoteExists`** (`src/lib/git.ts:180-187`), + so for exactly the worktrees this plan is about, `ahead` is `undefined`. This is not an oversight to + route around: `gitGetCommitsAheadCount` compares against `@{u}` (`src/lib/git.ts:84-91`), which cannot + resolve once the upstream branch is deleted. Any check for unpushed commits on a deleted-remote branch + needs a different comparison base, which is why D3 scopes it out rather than folding it in. +- **Must not change what `branch` or `checkout` do at creation time.** The `--track` / `--no-track` + difference is the reason the exposure is shaped as it is, but it is correct behaviour and out of scope. +- **Repository conventions** apply as written in [`stack.md`](../stack.md): ESM with `.js` extensions on + relative imports, named exports outside `src/commands/`, tests colocated as `*.test.ts`, Biome owns + formatting. Verification commands come from [`verify.md`](../verify.md) and nowhere else. +- **Land this before `agent-mode` Phase 6.** That phase adds a live-agent clause to this same + `isSafeToRemove` and rewrites `cleanup`'s reporting, touching `src/lib/git.ts`, + `src/commands/cleanup.ts` and both test files. Running the two concurrently would conflict; running this + one second would mean Phase 6 had already encoded the current behaviour into `cleanup.test.ts` as if it + were intended. + +## 3. Decisions + +**D1.** **Fix the predicate, not the callers.** `isSafeToRemove` is corrected so that its verdict can be +trusted by anything that reads it. Rejected: re-checking `uncommittedChanges` inside `cleanup` before the +sweep, because `remove.ts:81` inherits the identical bug and would need the identical patch — two +independently-worded copies of one safety rule, which is the drift this repository's own workflow exists to +prevent. + +**D2.** **Uncommitted changes disqualify a worktree from `safeToRemove`, regardless of remote state.** The +uncommitted-changes test is hoisted above the remote-branch branch so it cannot be skipped. Rejected: +prompting per-worktree inside the sweep, because `cleanup` is the bulk path — per-item prompts defeat its +purpose, and `--force` would skip them anyway, leaving the destructive path exactly as destructive as it is +today. + +**D3.** **Unpushed commits on a deleted-remote branch are out of scope for this plan**, and recorded as an +open question (§8, Q1) rather than silently fixed. Branch 2 swallows that case too — a worktree that is +`ahead` with a deleted remote is also classified safe — but detecting it is not a predicate change: `ahead` +is `undefined` for these worktrees by construction (§2), and `gitGetCommitsAheadCount`'s `@{u}` cannot +resolve without an upstream. Fixing it means choosing a new comparison base and changing what +`gitGetWorktreeList` gathers. That is a larger, separately-reviewable change; folding it in here would grow +a `small` entry into a `medium` one and delay the data-loss fix behind an unsettled design question. +Rejected: doing both at once, for that reason. + +**D4.** **`safeToRemove` stays `boolean`.** A skipped worktree's *reason* is derived at the point of +reporting from the fields already on the entry (`uncommittedChanges`, `remote`, `remoteExists`), exactly as +`worktreeListEntryToListName` already does (`src/lib/utils.ts:29-42`). Rejected: turning `safeToRemove` into +a reason enum or a `{ safe, reason }` object. It is a published interface member (§2) with two consumers +that both treat it as a boolean — `remove.ts` groups on its truthiness — so the change would ripple through +both commands and their tests for no gain this plan needs. + +**D5.** **`isSafeToRemove` becomes a total function with an explicit return type.** It currently declares no +return type and falls off the end when no branch matches, so it returns `boolean | undefined` and +`safeToRemove` is `undefined` rather than `false` for every unsafe worktree. `cleanup.ts:27` compares +`=== true` and `remove.ts:36-37` tests truthiness, so both happen to behave — but the next caller to write +`!== false` inherits a trap. Adding `: boolean` and a final `return false` is a correctness fix in its own +right and makes the compiler enforce that every branch decides. + +**D6.** **`isSafeToRemove` is exported as a named export** so it can be unit-tested directly. It is a pure +function over a `WorktreeListEntry`, and testing it through `gitGetWorktreeList` would require mocking four +git calls to assert one predicate. Named export matches the `src/lib/` convention in +[`stack.md`](../stack.md). Rejected: testing it indirectly through `gitGetWorktreeList`, because the mock +scaffolding would exceed the code under test and would couple the predicate's tests to the list builder's +implementation. + +**D7.** **`cleanup` reports what it skipped**, naming each worktree it declined to remove and why, in both +the default and `--force` paths. Without this the fix is silent: a user whose worktree is no longer swept +gets no signal that anything changed, and `--force` prints nothing at all today. This is the reporting half +of the roadmap entry. + +**D8.** **`gitRemoveWorktreesWithProgress` keeps its unconditional `force: true`** (`src/lib/git.ts:354`). +Once D1/D2 land, everything `cleanup` sends it is genuinely safe, and `remove` sends user-selected +worktrees that have already passed a confirmation prompt, where `--force` is the point. Rejected: deriving +`force` per worktree from its hazards, because the sweep would then fail partway on precisely the +worktrees a user had just confirmed. Recorded as a residual risk (R3) rather than a change. + +## 4. Design + +### 4.1 The predicate + +`isSafeToRemove` is reordered so the hazard test cannot be bypassed, gains an explicit return type, and +becomes total: + +```ts +export function isSafeToRemove(wt: WorktreeListEntry): boolean { + // A worktree whose directory is gone holds nothing to lose. + if (!wt.pathExists) return true; + + // Uncommitted work disqualifies a worktree whatever its remote looks like. See plan §3 D2. + if (wt.uncommittedChanges) return false; + + // Tracking a remote branch that no longer exists. + if (wt.remote && !wt.remoteExists) return true; + + // No remote, and nothing pending. + if (!wt.remote && !wt.ahead && !wt.behind) return true; + + return false; +} +``` + +Three things to note about the shape: + +- **The `!wt.pathExists` branch stays first.** If the directory is gone there is nothing to lose, and + `uncommittedChanges` is hardcoded to `0` for that case anyway (`src/lib/git.ts:188-190`), so testing it + first would be meaningless as well as wrong. +- **The uncommitted test is `if (wt.uncommittedChanges)`, not `=== 0`.** The field is optional + (`src/lib/types.ts:17`), so `undefined` must not read as "has changes". Truthiness gives `undefined` and + `0` the same, correct answer. +- **The third branch loses its `wt.uncommittedChanges === 0` clause**, which is now redundant — the hoisted + test has already returned `false` for that case. Leaving it in would be a second copy of the same rule. + +Source comments cite `§3 D2` by section number, not line number, per +[`plan-template.notes.md`](../plan-template.notes.md). + +### 4.2 Reporting in `cleanup` + +`cleanup` currently derives its candidate list and discards everything else (`src/commands/cleanup.ts:27`). +It instead keeps both halves — the removable set and the worktrees it declined — and prints the declined +ones with the reason drawn from the entry's own fields, reusing `worktreeListEntryToListName` +(`src/lib/utils.ts:24-48`), which already renders "Remote removed" and "N uncommitted changes". + +The skipped list prints in **both** paths. Today `--force` takes the `else` branch and prints nothing +(`cleanup.ts:46-48`); after this change it still skips the *confirmation* but still says what it left +alone. `--force` means "do not ask me", not "do not tell me". + +The existing "No stale worktree branches found." early return (`cleanup.ts:29-32`) needs to account for the +case where there are no removable worktrees but there *are* skipped ones — reporting "none found" while +silently declining three worktrees with work in them would reintroduce the invisibility this fix exists to +remove. + +### 4.3 What `remove` needs + +Nothing, in the command itself. Once the predicate is correct, a deleted-remote worktree holding uncommitted +changes stops being sorted into the "Safe to delete" group (`remove.ts:39-47`) and starts tripping the +`selected.some((wt) => !wt.safeToRemove)` confirmation at `remove.ts:81`. The phase for `remove` is +regression coverage proving that, not a code change — and if it turns out a change is needed, that is the +finding the phase exists to surface. + +### 4.4 Documentation + +`docs/src/app/docs/commands/cleanup/page.mdx:16` lists "worktrees whose remote branch no longer exists" as a +cleanup target with no qualification, which documents the defect as if it were the design. It gains the +uncommitted-work exception. + +## 5. Risks + +**R1 — `cleanup` becomes less useful because `git status -s` counts untracked files.** +`gitGetUncommittedChangesCount` shells out to `git status -s` (`src/lib/git.ts:102-105`), whose short format +includes untracked entries (`??`) by default. A merged worktree holding only stray cruft — an unignored +`.env.local`, a scratch file, a stale build output — will now be declined rather than swept. **How it shows +up:** users report that `cleanup` stopped cleaning anything. **Response:** this is the correct default — +untracked files are unrecoverable in exactly the way this plan is about, and D7's skip reporting names the +worktree and its count so the user can act. If it proves too noisy in practice, the lever is a flag or a +`-uno` variant, recorded as Q2 rather than pre-emptively built. + +**R2 — the `agent-mode` collision.** `plans/AGENT-MODE-PLAN.md` Phase 6 edits the same function and the same +`cleanup` reporting. **How it shows up:** merge conflicts in `src/lib/git.ts` and `src/commands/cleanup.ts`, +or a silent revert of this fix if Phase 6 rewrites the predicate from its own plan text. **Response:** land +this first (§2), and have Phase 6 add its live-agent clause to the corrected predicate. + +**R3 — the unconditional `force: true` in the shared sweep remains** (`src/lib/git.ts:354`, D8). This fix +removes the way unsafe worktrees currently *reach* that sweep from `cleanup`, but the sweep itself stays +maximally destructive for whatever is handed to it. **How it shows up:** a future caller that builds its own +worktree list and calls `gitRemoveWorktreesWithProgress` gets force-removal with no prompt and no verdict +check. **Response:** accepted for this plan; the mitigation is that the one remaining unguarded caller path +(`remove`, after an explicit confirmation) is intentional. + +**R4 — characterization tests briefly assert the defect.** Phase 1 pins current behaviour, including the +wrong verdict, so that Phase 2's diff shows the behaviour change. **How it shows up:** someone reads the +Phase 1 commit in isolation and believes the project intends that behaviour. **Response:** the assertion +carries a comment naming this plan and the phase that overturns it, and Phase 2 is the immediately following +phase. + +## 6. Phases + +### 6.1 Status ledger + +| # | Phase | Status | Depends on | Note | +|---|---|---|---|---| +| 1 | Make `isSafeToRemove` testable and total | not started | — | No behaviour change | +| 2 | Uncommitted work disqualifies removal | not started | 1 | The fix | +| 3 | `cleanup` reports what it skipped | not started | 2 | | +| 4 | `remove` multi-select regression coverage | not started | 2 | | +| 5 | Document the exception | not started | 2 | Docs-only — Lint gate only | + +Status is one of `not started`, `in progress`, `blocked`, `done`. `done` only when committed and verified, +and whoever finishes a phase updates the row in the same commit. + +**Exactly one table in this document has these columns.** Do not add a second phase table — a +differently-shaped one nearby is a decoy that gets read by mistake. + +### 6.2 The phases + +#### Phase 1 — Make `isSafeToRemove` testable and total + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts` + +**Scope:** Export `isSafeToRemove` (D6), give it an explicit `: boolean` return type and a final +`return false` (D5). **No branch is reordered and no verdict changes.** Add a `describe("isSafeToRemove")` +block to `src/lib/git.test.ts` covering all four current outcomes: missing path, deleted remote, no-remote +clean, and the fall-through. The deleted-remote-with-uncommitted-changes case is asserted as `true` — the +current, wrong answer — with a comment citing this plan §1 and naming Phase 2 as the phase that overturns +it (R4). + +**Done when:** `isSafeToRemove` is exported with a `boolean` return type, `git.test.ts` has a +`describe("isSafeToRemove")` block whose cases include a deleted-remote worktree with non-zero +`uncommittedChanges`, and `pnpm test` exits 0 with no change to any existing assertion. + +#### Phase 2 — Uncommitted work disqualifies removal + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts` + +**Scope:** Hoist the uncommitted-changes test above the remote branch and drop the now-redundant +`uncommittedChanges === 0` clause from the third branch, per §4.1 (D2). Flip the Phase 1 characterization +assertion to `false` and remove its R4 comment. Add cases pinning that the fix does not over-reach: a +deleted-remote worktree with `uncommittedChanges: 0` is still `true`, and one with `uncommittedChanges: +undefined` is still `true`. + +**Done when:** `isSafeToRemove` returns `false` for `{ pathExists: true, remote: "origin/x", remoteExists: +false, uncommittedChanges: 3 }`, still returns `true` for the same entry with `uncommittedChanges` of `0` or +`undefined`, and `pnpm test` exits 0. + +#### Phase 3 — `cleanup` reports what it skipped + +**Files:** `src/commands/cleanup.ts`, `src/commands/cleanup.test.ts` + +**Scope:** Partition the list rather than filtering it, and print the declined worktrees through +`worktreeListEntryToListName` in both the default and `--force` paths (§4.2, D7). Handle the +nothing-removable-but-something-skipped case so it no longer reports "No stale worktree branches found." +while silently declining worktrees that hold work. Existing fixtures at `cleanup.test.ts:44,56,68` hand-set +`safeToRemove`; add one with `safeToRemove: false` and non-zero `uncommittedChanges` to drive the new +output. + +**Done when:** running `cleanup` against a list containing a `safeToRemove: false` worktree prints that +worktree and its reason, with and without `--force`; the no-candidates path distinguishes "nothing found" +from "everything was skipped"; `pnpm test` exits 0. + +**Note:** assert unstyled strings. `vitest.config.ts` pins `FORCE_COLOR: "0"` and this is load-bearing for +this exact file — see [`verify.md`](../verify.md). + +#### Phase 4 — `remove` multi-select regression coverage + +**Files:** `src/commands/remove.test.ts`, and `src/commands/remove.ts` only if the test proves a change is +needed + +**Scope:** Prove the claim in §4.3 — that a deleted-remote worktree with uncommitted changes now reaches the +`selected.some((wt) => !wt.safeToRemove)` confirmation at `remove.ts:81` instead of being grouped under +"Inactive branches (Safe to delete)" at `remove.ts:39-47`. If it does not, fix `remove.ts` and say so in the +ledger Note. + +**Done when:** `remove.test.ts` contains a case selecting such a worktree that asserts the confirmation is +requested and that declining it performs no removal; `pnpm test` exits 0. + +#### Phase 5 — Document the exception + +**Files:** `docs/src/app/docs/commands/cleanup/page.mdx` + +**Scope:** Qualify the "worktrees whose remote branch no longer exists" bullet (line 16) with the +uncommitted-work exception, and state that skipped worktrees are reported (§4.4). + +**Done when:** the page no longer claims deleted-remote worktrees are removed unconditionally, and `pnpm +check` exits 0. Docs-only, so per [`verify.md`](../verify.md) this phase runs the Lint gate plus a read of +the diff. + +## 7. Verification + +Beyond [`verify.md`](../verify.md) passing, the defect in §1 is proved gone by hand, against a real +repository — the unit tests use synthetic `WorktreeListEntry` objects and cannot prove that the real +`gitGetWorktreeList` produces the field values the predicate now depends on. + +1. In a scratch repository with a remote, `worktree checkout some-branch`. +2. Delete the remote branch (`git push origin --delete some-branch`), then `git fetch --prune`. +3. Write an uncommitted change in the worktree — do both a modification to a tracked file and an untracked + new file, since `git status -s` counts both (R1). +4. `worktree list` — the entry should show `Remote removed` and the uncommitted count. +5. `worktree cleanup` — the worktree must **not** appear in the removal candidates, and must appear in the + skipped report with its reason. +6. `worktree cleanup --force` — same: not removed, and still reported as skipped. +7. Commit the change, leaving the branch `ahead` with no remote. `worktree cleanup` **will** remove it — + this is Q1, the known remaining gap, and confirming it here is what keeps the open question honest + rather than forgotten. +8. `worktree remove`, select the worktree from step 3 — it must be listed outside the "Safe to delete" + group and must trigger the not-safe confirmation. + +## 8. Open questions + +- **Q1 — unpushed commits on a deleted-remote branch.** Scoped out by D3 and still a live data-loss path, + narrower than the one this plan closes: a worktree that is `ahead` with a deleted remote is classified + safe. It cannot be fixed in the predicate alone, because `ahead` is `undefined` for these worktrees by + construction and `gitGetCommitsAheadCount` relies on `@{u}`, which no longer resolves (§2). Settling it + means choosing a comparison base — the repository's default branch is the obvious candidate, but that is + a design decision with its own edge cases, not a detail. **Should this become its own roadmap entry once + this one lands?** +- **Q2 — should untracked files count?** R1's regression rests on `git status -s` including untracked + entries. Treating an unignored scratch file as work worth protecting is the safe default and this plan + adopts it, but it is a judgement call that will shape how `cleanup` feels day to day. No evidence either + way was available while planning; the honest answer is to ship the safe default and revisit if it + annoys. +- **Q3 — the `pathExists: false` branch is unverified.** It returns `true` immediately, and + `uncommittedChanges` is hardcoded to `0` for that case (`src/lib/git.ts:188-190`), so a worktree whose + directory was deleted out from under git is always swept. That is almost certainly right — there is + nothing left to lose — but it was not tested against a real repository while planning, only read. +- **Q4 — is `--force` printing skipped worktrees the right call?** D7 says `--force` should still report. + A user piping `cleanup --force` in a script gets new output on stdout. Nothing in the repository suggests + that path is scripted, but nothing rules it out either. +- **Q5 — sequencing against `agent-mode`.** This plan assumes it lands before `agent-mode` Phase 6 (§2, + R2). `agent-mode` is `pending` and holds no active slot, so there is no conflict today — but if + `agent-mode` is activated first, this plan's §4.1 and §4.2 need re-reading against whatever Phase 6 left + behind. + +## 9. Reference material carried from the draft + +Captured by `/roadmap` on 2026-09-05 from maintainer-supplied material, which originated as analysis done +while planning `agent-mode` in the same session — it was found by reading `isSafeToRemove` to work out what +a live-agent check would have to attach to. Retained here because it is the provenance of §1. + +### 9.1 Verified against the tree + +Read, not recalled. Every row checked on 2026-09-05 on branch `feature/add-agent-mode`, once when the draft +was written and again while writing this plan. + +| Claim | Status | +|---|---| +| `isSafeToRemove` returns at line 158 before reaching the `uncommittedChanges` branch at 162 | confirmed, `src/lib/git.ts:153-166` | +| `cleanup` filters on `safeToRemove === true` | confirmed, `src/commands/cleanup.ts:27` | +| The sweep removes with an unconditional `force: true` | confirmed, `src/lib/git.ts:354` | +| `cleanup --force` only skips the confirmation prompt | confirmed, `src/commands/cleanup.ts:34-48` | +| The candidate list already prints uncommitted counts | confirmed, `src/commands/cleanup.ts:40` → `src/lib/utils.ts:38-42` | +| The single-worktree path prompts separately on uncommitted changes | confirmed, `src/lib/git.ts:310-315` | +| `remote` is only non-empty when the branch has an upstream | confirmed, `src/lib/git.ts:178` | +| `remove` also consumes `safeToRemove`, to group choices and to gate its confirmation | confirmed, `src/commands/remove.ts:36-48,81` | +| `ahead`/`behind` are computed only when `pathExists && remoteExists` | confirmed, `src/lib/git.ts:180-187` | +| `gitGetCommitsAheadCount` compares against `@{u}` | confirmed, `src/lib/git.ts:84-91` | +| `gitGetUncommittedChangesCount` uses `git status -s`, which counts untracked files | confirmed, `src/lib/git.ts:102-105` | +| `isSafeToRemove` declares no return type and falls through to `undefined` | confirmed, `src/lib/git.ts:153-166` | +| No test exercises `isSafeToRemove` or `gitGetWorktreeList` | confirmed, `src/lib/git.test.ts` — four `describe` blocks, none covering either | +| The docs page lists deleted-remote worktrees as targets without qualification | confirmed, `docs/src/app/docs/commands/cleanup/page.mdx:16` | + +### 9.2 Correction the draft made to the supplied material + +The original brief said cleanup shows "only a bulk progress bar". That overstates it — the default path does +print each candidate with its uncommitted count. The accurate, narrower statement is carried in §1, and it +matters because it changes what the fix has to do: the information is displayed but bundled into one bulk +yes/no, `--force` skips the display entirely, and the real defect is the classification rather than the +reporting. diff --git a/context/roadmap.md b/context/roadmap.md index c3413cb..6269972 100644 --- a/context/roadmap.md +++ b/context/roadmap.md @@ -34,8 +34,7 @@ shows what every running agent has changed. - **Size:** large — three surfaces (`branch`, `list`, `cleanup`), a new config value, and a runtime-neutral session join that must degrade silently -- **Doc:** [`drafts/agent-mode.md`](drafts/agent-mode.md) — maintainer brief: the cwd/isolation mechanic, - the four work items, and what already holds in this repo +- **Doc:** [`plans/AGENT-MODE-PLAN.md`](plans/AGENT-MODE-PLAN.md) — 7 phases, 5 open questions ### chat-input-multiline — `pending` @@ -44,3 +43,19 @@ and the text scrolls out of sight instead of the field growing. - **Size:** small — one component (`ChatInput`) plus its ref type and submit key handling in `ChatForm` - **Doc:** none yet + +### cleanup-data-loss — `pending` + +`worktree cleanup` classifies a worktree whose remote branch was deleted as safe to remove without ever +checking for uncommitted changes, then force-removes it — destroying work in progress. + +- **Size:** small — one predicate in `isSafeToRemove`, plus how `cleanup` reports what it skipped +- **Doc:** [`plans/CLEANUP-DATA-LOSS-PLAN.md`](plans/CLEANUP-DATA-LOSS-PLAN.md) — 5 phases, 5 open questions + +### shell-argv-safety — `pending` + +`cmd()` runs every git call through a shell as an interpolated string, so a repo path containing a space +fails today and a config value containing a quote or backtick is an injection vector. + +- **Size:** medium — one helper contract, 8 interpolating call sites, and the `exec` that bypasses it +- **Doc:** [`drafts/shell-argv-safety.md`](drafts/shell-argv-safety.md) — the full call-site inventory, a demonstrated failure, and why adding `cwd` removes most of it From e0f4feb6db6959f6f76dfb48cb02e231b68da3f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 16:41:03 +0000 Subject: [PATCH 03/31] refactor(git): make isSafeToRemove testable and total MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cleanup-data-loss Phase 1. Preparation only — no verdict changes. isSafeToRemove had no return type and fell off the end when no branch matched, so it returned boolean | undefined and safeToRemove was undefined rather than false for every unsafe worktree. Add an explicit : boolean and a final return false so the compiler enforces that every branch decides (plan D5), and export it so the predicate can be unit tested directly instead of through four mocked git calls (D6). No branch is reordered and no condition is touched. The only verdict transition is undefined -> false; both consumers already treated those identically (cleanup.ts:27 compares === true, remove.ts:36-37,81 test truthiness), and nothing in the tree distinguishes them. Adds the first coverage for the predicate: the four current outcomes, plus a characterization test pinning the defect this plan exists to fix — a deleted-remote worktree holding uncommitted changes is still classified safe. That assertion is deliberately wrong and carries a comment naming Phase 2 as what overturns it (plan R4). Gate 1: check, typecheck, build, test, docs:test all exit 0 (186 src tests, up from 181; no existing assertion changed). Gate 2: PASS from the reviewer subagent, no blocking findings. F-001 (P3) raised against Phase 2 for an unpinned input whose verdict the Phase 2 rewrite will silently flip. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/findings.md | 17 +++++++ context/plans/CLEANUP-DATA-LOSS-PLAN.md | 4 +- context/roadmap.md | 2 +- src/lib/git.test.ts | 61 +++++++++++++++++++++++++ src/lib/git.ts | 3 +- 5 files changed, 83 insertions(+), 4 deletions(-) diff --git a/context/findings.md b/context/findings.md index bacb273..2b6f973 100644 --- a/context/findings.md +++ b/context/findings.md @@ -36,5 +36,22 @@ life of the project. ## Open +### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it + +**Tied to:** cleanup-data-loss Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) + +No test covers `{ pathExists: true, remote: "", uncommittedChanges: undefined }`. Today the third branch's +strict `wt.uncommittedChanges === 0` (`src/lib/git.ts:162`) makes `undefined` fail the clause, so the entry +falls through and `isSafeToRemove` returns `false`. Under the §4.1 rewrite that clause is dropped, and the +same entry becomes `true` — a silent verdict change in a phase whose stated scope is the deleted-remote +case. + +Not reachable from `gitGetWorktreeList`, which always assigns a number (`src/lib/git.ts:189-191`), so this +is a latent contract change rather than a live defect. Phase 2's **Done when** already requires cases +pinning that the fix does not over-reach; this is the case it does not currently name. + +**Closes when:** Phase 2's Gate 1 re-passes with a `git.test.ts` case asserting the verdict for a +no-remote entry whose `uncommittedChanges` is `undefined`, whichever verdict Phase 2 decides is correct. + ## Closed diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/plans/CLEANUP-DATA-LOSS-PLAN.md index beb0126..9f49eeb 100644 --- a/context/plans/CLEANUP-DATA-LOSS-PLAN.md +++ b/context/plans/CLEANUP-DATA-LOSS-PLAN.md @@ -258,11 +258,11 @@ phase. | # | Phase | Status | Depends on | Note | |---|---|---|---|---| -| 1 | Make `isSafeToRemove` testable and total | not started | — | No behaviour change | +| 1 | Make `isSafeToRemove` testable and total | done | — | Verdicts unchanged; only the fall-through moved `undefined` → `false`. F-001 (P3) raised against Phase 2 | | 2 | Uncommitted work disqualifies removal | not started | 1 | The fix | | 3 | `cleanup` reports what it skipped | not started | 2 | | | 4 | `remove` multi-select regression coverage | not started | 2 | | -| 5 | Document the exception | not started | 2 | Docs-only — Lint gate only | +| 5 | Document the exception | not started | 2 | Docs-only — Lint gate only. §4.4 cites `page.mdx:16`; the bullet is now at line 17 | Status is one of `not started`, `in progress`, `blocked`, `done`. `done` only when committed and verified, and whoever finishes a phase updates the row in the same commit. diff --git a/context/roadmap.md b/context/roadmap.md index 6269972..a1950d5 100644 --- a/context/roadmap.md +++ b/context/roadmap.md @@ -44,7 +44,7 @@ and the text scrolls out of sight instead of the field growing. - **Size:** small — one component (`ChatInput`) plus its ref type and submit key handling in `ChatForm` - **Doc:** none yet -### cleanup-data-loss — `pending` +### cleanup-data-loss — `active` `worktree cleanup` classifies a worktree whose remote branch was deleted as safe to remove without ever checking for uncommitted changes, then force-removes it — destroying work in progress. diff --git a/src/lib/git.test.ts b/src/lib/git.test.ts index 880d11b..bc081a1 100644 --- a/src/lib/git.test.ts +++ b/src/lib/git.test.ts @@ -12,7 +12,9 @@ import { gitGetRootPath, gitGetUncommittedChangesCount, gitSetConfigValue, + isSafeToRemove, } from "./git.js"; +import type { WorktreeListEntry } from "./types.js"; describe("git branch parsing", () => { beforeEach(() => { @@ -235,3 +237,62 @@ describe("git status and tracking helpers", () => { ]); }); }); + +describe("isSafeToRemove", () => { + function entry( + overrides: Partial = {}, + ): WorktreeListEntry { + return { + path: "/repo/project.worktrees/test", + branchName: "feature/test", + pathExists: true, + remote: "", + uncommittedChanges: 0, + ...overrides, + }; + } + + it("is safe when the worktree directory no longer exists", () => { + expect(isSafeToRemove(entry({ pathExists: false }))).toBe(true); + }); + + it("is safe when the tracked remote branch was deleted", () => { + expect( + isSafeToRemove( + entry({ remote: "origin/feature/test", remoteExists: false }), + ), + ).toBe(true); + }); + + it("is safe with no remote and nothing pending", () => { + expect(isSafeToRemove(entry())).toBe(true); + }); + + it("is not safe when no branch matches", () => { + expect( + isSafeToRemove( + entry({ + remote: "origin/feature/test", + remoteExists: true, + ahead: 2, + }), + ), + ).toBe(false); + }); + + // Characterization of the defect described in CLEANUP-DATA-LOSS-PLAN §1: the + // deleted-remote branch returns before the uncommitted-changes test is ever + // reached, so work in progress is classified safe to remove. Phase 2 of that + // plan overturns this assertion. + it("is safe when the remote was deleted, even holding uncommitted changes", () => { + expect( + isSafeToRemove( + entry({ + remote: "origin/feature/test", + remoteExists: false, + uncommittedChanges: 3, + }), + ), + ).toBe(true); + }); +}); diff --git a/src/lib/git.ts b/src/lib/git.ts index a431a06..b4aeb91 100644 --- a/src/lib/git.ts +++ b/src/lib/git.ts @@ -150,7 +150,7 @@ export async function gitGetWorktrees({ ); } -function isSafeToRemove(wt: WorktreeListEntry) { +export function isSafeToRemove(wt: WorktreeListEntry): boolean { if (!wt.pathExists) { // Worktree is defined but doesn't exist in the filesystem. return true; @@ -163,6 +163,7 @@ function isSafeToRemove(wt: WorktreeListEntry) { // Worktree has no changes and it not tracking any remote branch. return true; } + return false; } export async function gitGetWorktreeList({ From 7a3e67534a0d7741331cc4067c3d02bf05a5d3d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 17:04:26 +0000 Subject: [PATCH 04/31] fix(git): uncommitted work disqualifies a worktree from removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isSafeToRemove returned true for a worktree tracking a deleted remote branch no matter how much uncommitted work sat in it: the remote branch returned before the only clause that consulted uncommittedChanges was reached. Both cleanup and remove trusted that verdict and force-removed through it. Hoist the uncommitted-changes test above the remote branch so it cannot be bypassed, and drop the now-redundant `uncommittedChanges === 0` clause from the no-remote branch. The test is a truthiness check, not a comparison, because the field is optional and undefined must not read as "has changes". Two verdicts move, both intended. The fix itself, and the no-remote entry with an unknown count, which F-001 pre-registered against this phase and which the plan's §4.1 specifies; it is unreachable from gitGetWorktreeList, which always assigns a number. Verified against a real repository as well as in unit tests: a worktree whose remote branch was deleted while holding a tracked modification and an untracked file is no longer a candidate for `cleanup` or for `cleanup --force`, and the work survives both. CLEANUP-DATA-LOSS-PLAN Phase 2 -> done. F-001 closed, F-002 opened. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/findings.md | 40 +++++++++++++++++-------- context/plans/CLEANUP-DATA-LOSS-PLAN.md | 2 +- src/lib/git.test.ts | 36 ++++++++++++++++++---- src/lib/git.ts | 7 ++++- 4 files changed, 66 insertions(+), 19 deletions(-) diff --git a/context/findings.md b/context/findings.md index 2b6f973..a5f2867 100644 --- a/context/findings.md +++ b/context/findings.md @@ -36,22 +36,38 @@ life of the project. ## Open -### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it +### F-002 — P3 — the `pathExists` ordering rationale in §4.1 is not pinned by any test + +**Tied to:** cleanup-data-loss Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 2) -**Tied to:** cleanup-data-loss Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) +§4.1's first bullet argues that `!wt.pathExists` must stay the first branch — a worktree whose directory is +gone holds nothing to lose. No test enforces it. `src/lib/git.test.ts:255-257` covers `{ pathExists: false }` +with the `entry()` default of `uncommittedChanges: 0`, which returns `true` under either ordering, so a +future change hoisting the uncommitted test above the path test would flip +`{ pathExists: false, uncommittedChanges: 3 }` from `true` to `false` with no test failing. -No test covers `{ pathExists: true, remote: "", uncommittedChanges: undefined }`. Today the third branch's -strict `wt.uncommittedChanges === 0` (`src/lib/git.ts:162`) makes `undefined` fail the clause, so the entry -falls through and `isSafeToRemove` returns `false`. Under the §4.1 rewrite that clause is dropped, and the -same entry becomes `true` — a silent verdict change in a phase whose stated scope is the deleted-remote -case. +Latent, not live: `gitGetWorktreeList` hardcodes `uncommittedChanges` to `0` when the path is missing +(`src/lib/git.ts:194-196`), so the combination is unreachable from the list builder. Left open rather than +fixed because Phase 2's **Done when** names exactly two over-reach cases and this is not one of them — +adding it would have landed an unreviewed assertion after Gate 2 had already passed on the diff. -Not reachable from `gitGetWorktreeList`, which always assigns a number (`src/lib/git.ts:189-191`), so this -is a latent contract change rather than a live defect. Phase 2's **Done when** already requires cases -pinning that the fix does not over-reach; this is the case it does not currently name. +This matters sooner than it looks: `agent-mode` Phase 6 adds a live-agent clause to this same predicate +(§2, R2), and is the natural place to pin the ordering while the branches are being re-read anyway. -**Closes when:** Phase 2's Gate 1 re-passes with a `git.test.ts` case asserting the verdict for a -no-remote entry whose `uncommittedChanges` is `undefined`, whichever verdict Phase 2 decides is correct. +**Closes when:** a Gate 1 run passes with a `git.test.ts` case asserting the verdict for +`{ pathExists: false, uncommittedChanges: 3 }`. ## Closed +### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it + +**Tied to:** cleanup-data-loss Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) · +**Closed:** 2026-09-05 (Gate 1, Phase 2) + +Phase 2 dropped the `wt.uncommittedChanges === 0` clause per §4.1 and pinned the resulting verdict: the +no-remote entry with an unknown count is `true`, asserted at `src/lib/git.test.ts:321-323` against +`entry()`'s defaults of `pathExists: true, remote: ""` (`src/lib/git.test.ts:245-253`). Gate 1 re-passed on +that run — `pnpm check`, `pnpm typecheck`, `pnpm build`, `pnpm test` (12 files, 189 tests) and +`pnpm docs:test` (6 files, 49 tests) all exit 0. + + diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/plans/CLEANUP-DATA-LOSS-PLAN.md index 9f49eeb..b634553 100644 --- a/context/plans/CLEANUP-DATA-LOSS-PLAN.md +++ b/context/plans/CLEANUP-DATA-LOSS-PLAN.md @@ -259,7 +259,7 @@ phase. | # | Phase | Status | Depends on | Note | |---|---|---|---|---| | 1 | Make `isSafeToRemove` testable and total | done | — | Verdicts unchanged; only the fall-through moved `undefined` → `false`. F-001 (P3) raised against Phase 2 | -| 2 | Uncommitted work disqualifies removal | not started | 1 | The fix | +| 2 | Uncommitted work disqualifies removal | done | 1 | Two verdicts moved, both intended: the fix, and the no-remote `undefined` case F-001 pre-registered. F-001 closed. §4.1 cites `git.ts:188-190`; that assignment is now at `194-196` | | 3 | `cleanup` reports what it skipped | not started | 2 | | | 4 | `remove` multi-select regression coverage | not started | 2 | | | 5 | Document the exception | not started | 2 | Docs-only — Lint gate only. §4.4 cites `page.mdx:16`; the bullet is now at line 17 | diff --git a/src/lib/git.test.ts b/src/lib/git.test.ts index bc081a1..73ee3c6 100644 --- a/src/lib/git.test.ts +++ b/src/lib/git.test.ts @@ -280,11 +280,7 @@ describe("isSafeToRemove", () => { ).toBe(false); }); - // Characterization of the defect described in CLEANUP-DATA-LOSS-PLAN §1: the - // deleted-remote branch returns before the uncommitted-changes test is ever - // reached, so work in progress is classified safe to remove. Phase 2 of that - // plan overturns this assertion. - it("is safe when the remote was deleted, even holding uncommitted changes", () => { + it("is not safe when the remote was deleted but work is uncommitted", () => { expect( isSafeToRemove( entry({ @@ -293,6 +289,36 @@ describe("isSafeToRemove", () => { uncommittedChanges: 3, }), ), + ).toBe(false); + }); + + it("is still safe when the remote was deleted and nothing is uncommitted", () => { + expect( + isSafeToRemove( + entry({ + remote: "origin/feature/test", + remoteExists: false, + uncommittedChanges: 0, + }), + ), + ).toBe(true); + }); + + // An unknown count must not read as "has changes" — the field is optional, so + // the hoisted test is a truthiness check rather than a comparison against 0. + it("is still safe when the remote was deleted and the count is unknown", () => { + expect( + isSafeToRemove( + entry({ + remote: "origin/feature/test", + remoteExists: false, + uncommittedChanges: undefined, + }), + ), ).toBe(true); }); + + it("is safe with no remote and an unknown uncommitted count", () => { + expect(isSafeToRemove(entry({ uncommittedChanges: undefined }))).toBe(true); + }); }); diff --git a/src/lib/git.ts b/src/lib/git.ts index b4aeb91..6115f95 100644 --- a/src/lib/git.ts +++ b/src/lib/git.ts @@ -155,11 +155,16 @@ export function isSafeToRemove(wt: WorktreeListEntry): boolean { // Worktree is defined but doesn't exist in the filesystem. return true; } + if (wt.uncommittedChanges) { + // Uncommitted work disqualifies a worktree whatever its remote looks like. + // See CLEANUP-DATA-LOSS-PLAN §3 D2. + return false; + } if (wt.remote && !wt.remoteExists) { // Worktree is tracking a remote branch that no longer exists. return true; } - if (!wt.remote && !wt.ahead && !wt.behind && wt.uncommittedChanges === 0) { + if (!wt.remote && !wt.ahead && !wt.behind) { // Worktree has no changes and it not tracking any remote branch. return true; } From b84b02205ed98dd5e9062464f01c25f94302d6e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 17:17:31 +0000 Subject: [PATCH 05/31] feat(cleanup): report the worktrees left alone for holding work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 made `isSafeToRemove` decline a stale worktree that holds uncommitted changes, which silently shrank what `cleanup` sweeps. Nothing told the user why. `--force` printed nothing at all. `cleanup` now names the worktrees it declined, with the reason drawn from the entry's own fields via `worktreeListEntryToListName`, in both the default and the `--force` path: `--force` means "do not ask me", not "do not tell me". The no-candidates path distinguishes "nothing found" from "everything was skipped", so declining three worktrees with work in them can no longer read as a clean sweep. "Declined" is read narrowly: only worktrees that would have been swept but for their work. Taken literally, plan §4.2's "the worktrees it declined" is every entry that is not `safeToRemove`, which would print every active worktree in the repository on every run. The set is computed by asking `isSafeToRemove` about a zeroed copy rather than restating the rule, so the safety verdict stays in one place per §3 D1. cleanup-data-loss Phase 3. Gate 1: `pnpm check`, `pnpm typecheck`, `pnpm build`, `pnpm test` (12 files, 193 tests), `pnpm docs:test` (6 files, 49 tests) — all exit 0. Gate 2: PASS WITH NOTES, reviewer subagent, no blocking findings; F-003 (P3) records a latent double-listing. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/findings.md | 20 ++++ context/plans/CLEANUP-DATA-LOSS-PLAN.md | 2 +- src/commands/cleanup.test.ts | 129 ++++++++++++++++++++++++ src/commands/cleanup.ts | 49 ++++++++- 4 files changed, 194 insertions(+), 6 deletions(-) diff --git a/context/findings.md b/context/findings.md index a5f2867..0ebd764 100644 --- a/context/findings.md +++ b/context/findings.md @@ -57,6 +57,26 @@ This matters sooner than it looks: `agent-mode` Phase 6 adds a live-agent clause **Closes when:** a Gate 1 run passes with a `git.test.ts` case asserting the verdict for `{ pathExists: false, uncommittedChanges: 3 }`. +### F-003 — P3 — a path-less worktree with a non-zero change count would be listed as skipped *and* removed + +**Tied to:** cleanup-data-loss Phase 3 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 3) + +For `{ pathExists: false, uncommittedChanges: 3 }` both halves of `cleanup`'s split claim the entry: +`isSafeToRemove` returns `true` at `src/lib/git.ts:154-156`, so it lands in `worktrees` +(`src/commands/cleanup.ts:48`), and `isSkippedForUncommittedChanges` (`src/commands/cleanup.ts:17-21`) also +returns `true`, because its zeroed probe hits that same first branch. The command would print the worktree +as skipped and then remove it anyway. + +Latent, not live, and for the same reason as F-002: `gitGetWorktreeList` hardcodes `uncommittedChanges` +to `0` when the path is missing (`src/lib/git.ts:194-196`), and `cleanup` consumes no other source. The +one-line form is `wt.safeToRemove !== true &&` in front of the existing condition. Left open rather than +fixed because Gate 2 had already passed on the diff — the same reasoning F-002 records — and because both +findings are the `pathExists: false` ordering question that `agent-mode` Phase 6 will have this predicate +open for anyway. + +**Closes when:** a Gate 1 run passes with a `cleanup.test.ts` case proving that entry appears in at most one +of the two lists. + ## Closed ### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/plans/CLEANUP-DATA-LOSS-PLAN.md index b634553..1e929d7 100644 --- a/context/plans/CLEANUP-DATA-LOSS-PLAN.md +++ b/context/plans/CLEANUP-DATA-LOSS-PLAN.md @@ -260,7 +260,7 @@ phase. |---|---|---|---|---| | 1 | Make `isSafeToRemove` testable and total | done | — | Verdicts unchanged; only the fall-through moved `undefined` → `false`. F-001 (P3) raised against Phase 2 | | 2 | Uncommitted work disqualifies removal | done | 1 | Two verdicts moved, both intended: the fix, and the no-remote `undefined` case F-001 pre-registered. F-001 closed. §4.1 cites `git.ts:188-190`; that assignment is now at `194-196` | -| 3 | `cleanup` reports what it skipped | not started | 2 | | +| 3 | `cleanup` reports what it skipped | done | 2 | Skipped = held back *only* by uncommitted work, asked of `isSafeToRemove` on a zeroed copy (D1); §4.2's literal "declined" would print every active worktree. F-003 (P3) raised. `verify.md:63` cites `cleanup.ts:37` and `cleanup.test.ts:176-178`; now `69` and `188-190` | | 4 | `remove` multi-select regression coverage | not started | 2 | | | 5 | Document the exception | not started | 2 | Docs-only — Lint gate only. §4.4 cites `page.mdx:16`; the bullet is now at line 17 | diff --git a/src/commands/cleanup.test.ts b/src/commands/cleanup.test.ts index 4ea4e46..f2eb5fa 100644 --- a/src/commands/cleanup.test.ts +++ b/src/commands/cleanup.test.ts @@ -68,6 +68,18 @@ describe("cleanup command", () => { safeToRemove: false, }; + // A stale worktree — its remote branch is gone — that holds uncommitted work. + // Phase 2 made `isSafeToRemove` decline it; this is what cleanup must report. + const staleWorktreeWithChanges = { + path: "/path/to/project.worktrees/feature/stale-dirty", + branchName: "feature/stale-dirty", + remote: "origin/feature/stale-dirty", + remoteExists: false, + pathExists: true, + uncommittedChanges: 3, + safeToRemove: false, + }; + beforeEach(() => { vi.clearAllMocks(); const mockConfig = { @@ -214,4 +226,121 @@ describe("cleanup command", () => { expect(spinnerMocks.stop).toHaveBeenCalledTimes(1); expect(mockRemove).toHaveBeenCalledWith([safeWorktree]); }); + + it("reports worktrees skipped for uncommitted changes and removes the rest", async () => { + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([ + safeWorktree, + staleWorktreeWithChanges, + ]); + const logSpy = vi.spyOn(cleanup, "log").mockImplementation(() => {}); + mockConfirm.mockResolvedValue(true); + const mockRemove = vi + .spyOn(git, "gitRemoveWorktreesWithProgress") + .mockResolvedValue(undefined); + + (cleanup as any).parse = vi.fn().mockResolvedValue({ + flags: { force: false }, + }); + + await cleanup.run(); + + expect(logSpy).toHaveBeenCalledWith( + "Skipped 1 worktree branch that has uncommitted changes:", + ); + expect(logSpy).toHaveBeenCalledWith( + "- feature/stale-dirty (Remote removed, 3 uncommitted changes)", + ); + expect(mockRemove).toHaveBeenCalledWith([safeWorktree]); + }); + + it("reports skipped worktrees when --force is set", async () => { + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([ + safeWorktree, + staleWorktreeWithChanges, + ]); + const logSpy = vi.spyOn(cleanup, "log").mockImplementation(() => {}); + const mockRemove = vi + .spyOn(git, "gitRemoveWorktreesWithProgress") + .mockResolvedValue(undefined); + + (cleanup as any).parse = vi.fn().mockResolvedValue({ + flags: { force: true }, + }); + + await cleanup.run(); + + expect(mockConfirm).not.toHaveBeenCalled(); + expect(spinnerMocks.stop).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + "Skipped 1 worktree branch that has uncommitted changes:", + ); + expect(logSpy).toHaveBeenCalledWith( + "- feature/stale-dirty (Remote removed, 3 uncommitted changes)", + ); + expect(mockRemove).toHaveBeenCalledWith([safeWorktree]); + }); + + it("distinguishes everything-skipped from nothing-found", async () => { + const secondStaleWorktree = { + ...staleWorktreeWithChanges, + path: "/path/to/project.worktrees/feature/stale-dirty-two", + branchName: "feature/stale-dirty-two", + uncommittedChanges: 1, + }; + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([ + staleWorktreeWithChanges, + secondStaleWorktree, + ]); + const logSpy = vi.spyOn(cleanup, "log").mockImplementation(() => {}); + const mockRemove = vi + .spyOn(git, "gitRemoveWorktreesWithProgress") + .mockResolvedValue(undefined); + + (cleanup as any).parse = vi.fn().mockResolvedValue({ + flags: { force: false }, + }); + + await cleanup.run(); + + expect(spinnerMocks.succeed).not.toHaveBeenCalled(); + expect(spinnerMocks.info).toHaveBeenCalledWith( + "No stale worktree branches can be removed safely.", + ); + expect(logSpy).toHaveBeenCalledWith( + "Skipped 2 worktree branches that have uncommitted changes:", + ); + expect(logSpy).toHaveBeenCalledWith( + "- feature/stale-dirty (Remote removed, 3 uncommitted changes)", + ); + expect(logSpy).toHaveBeenCalledWith( + "- feature/stale-dirty-two (Remote removed, 1 uncommitted change)", + ); + expect(mockConfirm).not.toHaveBeenCalled(); + expect(mockRemove).not.toHaveBeenCalled(); + }); + + // Only worktrees cleanup would otherwise have swept are reported. An active + // branch that happens to be dirty was never a candidate, so naming it here + // would be noise. See CLEANUP-DATA-LOSS-PLAN §4.2. + it("does not report an active worktree that merely holds uncommitted changes", async () => { + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([ + safeWorktree, + unsafeWorktree, + ]); + const logSpy = vi.spyOn(cleanup, "log").mockImplementation(() => {}); + mockConfirm.mockResolvedValue(true); + vi.spyOn(git, "gitRemoveWorktreesWithProgress").mockResolvedValue( + undefined, + ); + + (cleanup as any).parse = vi.fn().mockResolvedValue({ + flags: { force: false }, + }); + + await cleanup.run(); + + expect( + logSpy.mock.calls.some((call) => (call[0] ?? "").startsWith("Skipped ")), + ).toBe(false); + }); }); diff --git a/src/commands/cleanup.ts b/src/commands/cleanup.ts index feb2fa6..379a01c 100644 --- a/src/commands/cleanup.ts +++ b/src/commands/cleanup.ts @@ -6,9 +6,20 @@ import { BaseCommand } from "../lib/base-command.js"; import { gitGetWorktreeList, gitRemoveWorktreesWithProgress, + isSafeToRemove, } from "../lib/git.js"; +import type { WorktreeListEntry } from "../lib/types.js"; import { worktreeListEntryToListName } from "../lib/utils.js"; +// A worktree cleanup would have swept but for the work sitting in it. Asking the +// predicate about a zeroed copy keeps the safety rule in one place rather than +// restating it here. See CLEANUP-DATA-LOSS-PLAN §3 D1 and D7. +function isSkippedForUncommittedChanges(wt: WorktreeListEntry): boolean { + return ( + !!wt.uncommittedChanges && isSafeToRemove({ ...wt, uncommittedChanges: 0 }) + ); +} + export default class Cleanup extends BaseCommand { static override description = "Cleanup worktree branches by removing stale ones"; @@ -20,31 +31,59 @@ export default class Cleanup extends BaseCommand { }), }; + private logSkipped(skipped: WorktreeListEntry[]) { + const count = skipped.length; + this.log( + `Skipped ${chalk.bold(count)} worktree ${count === 1 ? "branch that has" : "branches that have"} uncommitted changes:`, + ); + skipped.forEach((wt) => { + this.log(`- ${worktreeListEntryToListName(wt, "yellow")}`); + }); + } + public async run(): Promise { const { flags } = await this.parse(Cleanup); const spinner = ora("Gathering worktree branches").start(); const allWorktrees = await gitGetWorktreeList(); const worktrees = allWorktrees.filter((wt) => wt.safeToRemove === true); + const skipped = allWorktrees.filter(isSkippedForUncommittedChanges); - if (worktrees.length === 0) { + if (worktrees.length === 0 && skipped.length === 0) { spinner.succeed("No stale worktree branches found."); return; } - if (!flags.force) { - const count = worktrees.length; + // Nothing is removable, but something was held back. Reporting "none found" + // here would hide exactly the worktrees this check exists to protect. + if (worktrees.length === 0) { + spinner.info("No stale worktree branches can be removed safely."); + this.logSkipped(skipped); + return; + } + + const count = worktrees.length; + if (flags.force) { + spinner.stop(); + } else { spinner.info( `Found ${chalk.bold(count)} worktree ${count === 1 ? "branch that is" : "branches that are"} marked safe to remove.`, ); worktrees.forEach((wt) => { this.log(`- ${worktreeListEntryToListName(wt, "gray")}`); }); + } + + // Reported in both paths: --force means "do not ask me", not "do not tell + // me". See CLEANUP-DATA-LOSS-PLAN §4.2. + if (skipped.length > 0) { + this.logSkipped(skipped); + } + + if (!flags.force) { const message = `Are you sure you want to delete ${count === 1 ? "it" : "them"}?`; if (!(await confirm({ message, default: false }))) { return; } - } else { - spinner.stop(); } await gitRemoveWorktreesWithProgress(worktrees); From 3cc4b2ecaf04296ddde3940890c4130b0e78338f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 17:25:49 +0000 Subject: [PATCH 06/31] Add discord notification --- .github/workflows/publish.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 13dd520..a89be02 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -33,3 +33,13 @@ jobs: - name: Publish prerelease if: github.event.release.prerelease == true run: npm publish --tag next + + - name: Send Discord Notification + uses: Ilshidur/action-discord@0.3.2 + env: + DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} + with: + args: | + 🚀 **Version ${{ github.event.release.tag_name }}** is now live on npm! + 📑 **What's New:** + ${{ github.event.release.html_url }} From 2200c72b9fe73df63af3ba04b20577d9b591f156 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 17:35:18 +0000 Subject: [PATCH 07/31] test(remove): pin that uncommitted work reaches the confirmation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 4 of CLEANUP-DATA-LOSS-PLAN. §4.3 predicted `remove` needs no code change once the Phase 2 predicate fix landed: a deleted-remote worktree holding uncommitted work stops being grouped under "Inactive branches (Safe to delete)" and starts tripping the not-safe confirmation instead. The tests confirm it, so no source file moved. The fixture takes `safeToRemove` from the real `isSafeToRemove` rather than hand-setting it as every other fixture in the file does — otherwise these cases would keep passing even if the classification regressed. F-004 (P3) records two assertions that are looser than the claim they pin; both are non-blocking and Gate 2 passed on this diff. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- .github/workflows/publish.yml | 2 +- context/findings.md | 23 +++++++ context/plans/CLEANUP-DATA-LOSS-PLAN.md | 2 +- src/commands/remove.test.ts | 86 +++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 2 deletions(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index a89be02..0ef21ea 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -40,6 +40,6 @@ jobs: DISCORD_WEBHOOK: ${{ secrets.DISCORD_WEBHOOK }} with: args: | - 🚀 **Version ${{ github.event.release.tag_name }}** is now live on npm! + 🚀 Version ${{ github.event.release.tag_name }} of @northguild/worktree is now live on npm! 📑 **What's New:** ${{ github.event.release.html_url }} diff --git a/context/findings.md b/context/findings.md index 0ebd764..e1ca025 100644 --- a/context/findings.md +++ b/context/findings.md @@ -77,6 +77,29 @@ open for anyway. **Closes when:** a Gate 1 run passes with a `cleanup.test.ts` case proving that entry appears in at most one of the two lists. +### F-004 — P3 — Phase 4's regression cases are looser than the §4.3 claim they pin + +**Tied to:** cleanup-data-loss Phase 4 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 4) + +Two assertions in `src/commands/remove.test.ts` prove less than the plan's §4.3 claims, in ways a future +edit could exploit without failing a test: + +- The confirmation case selects a single worktree (`remove.test.ts:367-368`), where `some` and `every` are + equivalent — so mutating `selected.some((wt) => !wt.safeToRemove)` at `src/commands/remove.ts:81` to + `every` passes. §4.3 names `some` specifically, and a mixed selection of `[safeWorktree, mergedWithWork]` + is the multi-select shape the phase is titled after. The pre-existing case at `remove.test.ts:173-194` + has the identical gap, so this is not introduced here. +- The grouping case asserts position only (`remove.test.ts:363`, `index > activeGroupStart`), so swapping + the two groups emitted at `src/commands/remove.ts:40-45` would still pass. The test's name promises "not + Safe to delete" and never asserts that literal. + +Non-blocking: Gate 2 returned `PASS WITH NOTES` on the diff and Phase 4's **Done when** is met — the +confirmation is asserted and declining it performs no removal. Left open rather than fixed for the reason +F-002 and F-003 record: adding assertions after Gate 2 had already passed would land unreviewed test code. + +**Closes when:** a Gate 1 run passes with the confirmation case selecting a mixed `[safe, unsafe]` list and +the grouping case asserting the entry is absent from the "Safe to delete" group by name. + ## Closed ### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/plans/CLEANUP-DATA-LOSS-PLAN.md index 1e929d7..68ede98 100644 --- a/context/plans/CLEANUP-DATA-LOSS-PLAN.md +++ b/context/plans/CLEANUP-DATA-LOSS-PLAN.md @@ -261,7 +261,7 @@ phase. | 1 | Make `isSafeToRemove` testable and total | done | — | Verdicts unchanged; only the fall-through moved `undefined` → `false`. F-001 (P3) raised against Phase 2 | | 2 | Uncommitted work disqualifies removal | done | 1 | Two verdicts moved, both intended: the fix, and the no-remote `undefined` case F-001 pre-registered. F-001 closed. §4.1 cites `git.ts:188-190`; that assignment is now at `194-196` | | 3 | `cleanup` reports what it skipped | done | 2 | Skipped = held back *only* by uncommitted work, asked of `isSafeToRemove` on a zeroed copy (D1); §4.2's literal "declined" would print every active worktree. F-003 (P3) raised. `verify.md:63` cites `cleanup.ts:37` and `cleanup.test.ts:176-178`; now `69` and `188-190` | -| 4 | `remove` multi-select regression coverage | not started | 2 | | +| 4 | `remove` multi-select regression coverage | done | 2 | §4.3 confirmed — `remove.ts` needed no change. The fixture takes `safeToRemove` from the real predicate, unlike the hand-set ones at `remove.test.ts:37,49`. F-004 (P3) raised. `verify.md:68` says `pnpm test` covers 181 tests; now 197 | | 5 | Document the exception | not started | 2 | Docs-only — Lint gate only. §4.4 cites `page.mdx:16`; the bullet is now at line 17 | Status is one of `not started`, `in progress`, `blocked`, `done`. `done` only when committed and verified, diff --git a/src/commands/remove.test.ts b/src/commands/remove.test.ts index aaa7def..1433b97 100644 --- a/src/commands/remove.test.ts +++ b/src/commands/remove.test.ts @@ -309,4 +309,90 @@ describe("remove command", () => { expect(choices).toHaveLength(1); }); }); + + // Regression coverage for CLEANUP-DATA-LOSS-PLAN §4.3, which claims this + // command needed no change of its own once isSafeToRemove stopped calling a + // deleted-remote worktree safe while work sits in it. These cases pin that + // claim so a later edit to either side cannot quietly undo it. + describe("a worktree whose remote was deleted while work is uncommitted", () => { + const mergedWithWorkEntry = { + path: "/path/to/project.worktrees/feature/merged-with-work", + branchName: "feature/merged-with-work", + remote: "origin/feature/merged-with-work", + remoteExists: false, + pathExists: true, + uncommittedChanges: 3, + }; + // Taken from the real predicate rather than hand-set. Every other fixture + // in this file supplies its own verdict, so it would keep passing even if + // the classification regressed. + const mergedWithWork = { + ...mergedWithWorkEntry, + safeToRemove: git.isSafeToRemove(mergedWithWorkEntry), + }; + + beforeEach(() => { + (remove as any).parse = vi.fn().mockResolvedValue({ + args: {}, + flags: { force: false }, + }); + }); + + it("should be classified as not safe to remove", () => { + expect(mergedWithWork.safeToRemove).toBe(false); + }); + + it("should be grouped under Active branches, not Safe to delete", async () => { + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([ + safeWorktree, + mergedWithWork, + ]); + mockCheckbox.mockResolvedValue([]); + + await remove.run(); + + const choices = mockCheckbox.mock.calls[0][0].choices as any[]; + const activeGroupStart = choices.findIndex((choice) => + String(choice.separator ?? "").includes("Active branches"), + ); + const index = choices.findIndex( + (choice) => choice.value?.branchName === "feature/merged-with-work", + ); + + expect(activeGroupStart).toBeGreaterThan(-1); + expect(index).toBeGreaterThan(activeGroupStart); + }); + + it("should prompt for confirmation when it is selected", async () => { + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([mergedWithWork]); + mockCheckbox.mockResolvedValue([mergedWithWork]); + mockConfirm.mockResolvedValue(true); + const mockRemove = vi + .spyOn(git, "gitRemoveWorktreesWithProgress") + .mockResolvedValue(undefined); + + await remove.run(); + + expect(mockConfirm).toHaveBeenCalledWith({ + message: + "Some selected branches are not safe to delete. Are you sure you want to continue?", + default: false, + }); + expect(mockRemove).toHaveBeenCalledWith([mergedWithWork]); + }); + + it("should not remove it when the confirmation is declined", async () => { + vi.spyOn(git, "gitGetWorktreeList").mockResolvedValue([mergedWithWork]); + mockCheckbox.mockResolvedValue([mergedWithWork]); + mockConfirm.mockResolvedValue(false); + const mockRemove = vi + .spyOn(git, "gitRemoveWorktreesWithProgress") + .mockResolvedValue(undefined); + + await remove.run(); + + expect(mockConfirm).toHaveBeenCalled(); + expect(mockRemove).not.toHaveBeenCalled(); + }); + }); }); From e98461c1f9807315e6c4891e0a38e9137ef442c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 17:44:55 +0000 Subject: [PATCH 08/31] docs(cleanup): qualify the deleted-remote target with uncommitted work The cleanup page listed "worktrees whose remote branch no longer exists" as an unconditional target, which documented the defect this feature fixed as if it were the design. Qualify it, name `git status -s` as the source of the count so untracked files are not a surprise, and say that held-back worktrees are reported. Closes cleanup-data-loss Phase 5. Gate 1: `pnpm check` exit 0 (docs-only, so Typecheck/Build/Test skipped per context/verify.md). Gate 2: PASS WITH NOTES. F-005 (P3) raised for README.md:160, which carries the same unqualified claim outside this phase's Files. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/findings.md | 20 ++++++++++++++++++++ context/plans/CLEANUP-DATA-LOSS-PLAN.md | 2 +- docs/src/app/docs/commands/cleanup/page.mdx | 8 +++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/context/findings.md b/context/findings.md index e1ca025..cedc4aa 100644 --- a/context/findings.md +++ b/context/findings.md @@ -100,6 +100,26 @@ F-002 and F-003 record: adding assertions after Gate 2 had already passed would **Closes when:** a Gate 1 run passes with the confirmation case selecting a mixed `[safe, unsafe]` list and the grouping case asserting the entry is absent from the "Safe to delete" group by name. +### F-005 — P3 — `README.md` still describes cleanup's pre-fix behaviour + +**Tied to:** cleanup-data-loss Phase 5 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 5) + +`README.md:160` carries the same unqualified claim Phase 5 corrected on the docs page — cleanup "targets +worktrees that are considered safe to remove, for example branches whose remote no longer exists" — with no +uncommitted-work exception. It is now the only place in the repository that documents the defect as if it +were the design; `grep -rn "remote branch no longer exists" docs README.md skills` returns the corrected +`page.mdx:17` and nothing else. Left unfixed because Phase 5's **Files** names exactly one file, +`docs/src/app/docs/commands/cleanup/page.mdx`, and editing the README would have been scope the gate did +not review. + +Adjacent and smaller: neither page documents `cleanup --force`, so D7's design point — `--force` skips the +confirmation but still prints the skipped report (`src/commands/cleanup.ts:66,78-80`) — is written down +nowhere user-facing. §4.4 did not ask for it and Q4 leaves whether `--force` *should* print unsettled, so +this is a note, not a gap to close blindly. + +**Closes when:** a Lint gate run passes with `README.md:160` carrying the same uncommitted-work exception +as `page.mdx:17`. + ## Closed ### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/plans/CLEANUP-DATA-LOSS-PLAN.md index 68ede98..d7f4c2e 100644 --- a/context/plans/CLEANUP-DATA-LOSS-PLAN.md +++ b/context/plans/CLEANUP-DATA-LOSS-PLAN.md @@ -262,7 +262,7 @@ phase. | 2 | Uncommitted work disqualifies removal | done | 1 | Two verdicts moved, both intended: the fix, and the no-remote `undefined` case F-001 pre-registered. F-001 closed. §4.1 cites `git.ts:188-190`; that assignment is now at `194-196` | | 3 | `cleanup` reports what it skipped | done | 2 | Skipped = held back *only* by uncommitted work, asked of `isSafeToRemove` on a zeroed copy (D1); §4.2's literal "declined" would print every active worktree. F-003 (P3) raised. `verify.md:63` cites `cleanup.ts:37` and `cleanup.test.ts:176-178`; now `69` and `188-190` | | 4 | `remove` multi-select regression coverage | done | 2 | §4.3 confirmed — `remove.ts` needed no change. The fixture takes `safeToRemove` from the real predicate, unlike the hand-set ones at `remove.test.ts:37,49`. F-004 (P3) raised. `verify.md:68` says `pnpm test` covers 181 tests; now 197 | -| 5 | Document the exception | not started | 2 | Docs-only — Lint gate only. §4.4 cites `page.mdx:16`; the bullet is now at line 17 | +| 5 | Document the exception | done | 2 | Docs-only — Lint gate only. §4.4's `page.mdx:16` bullet was at 17. Gate 2 `PASS WITH NOTES`; its N1/N2 tightened the new prose before commit — "every worktree it skipped" was broader than `cleanup.ts:17-21`, and "instead of silently removed" repeated the overstatement §9.2 corrects. F-005 (P3) raised: `README.md:160` still carries the unqualified claim, outside this phase's **Files** | Status is one of `not started`, `in progress`, `blocked`, `done`. `done` only when committed and verified, and whoever finishes a phase updates the row in the same commit. diff --git a/docs/src/app/docs/commands/cleanup/page.mdx b/docs/src/app/docs/commands/cleanup/page.mdx index efd537a..8281b18 100644 --- a/docs/src/app/docs/commands/cleanup/page.mdx +++ b/docs/src/app/docs/commands/cleanup/page.mdx @@ -14,5 +14,11 @@ worktree cleanup Typical cleanup targets include: -- worktrees whose remote branch no longer exists +- worktrees whose remote branch no longer exists, as long as they hold no uncommitted changes - local worktrees with no tracked remote and no pending work + +Uncommitted changes disqualify a worktree from removal whatever its remote looks like. The +count comes from `git status -s`, so untracked files hold a worktree back too. + +`cleanup` reports every worktree it would otherwise have removed but held back for that +reason, along with its change count, so work in progress is named rather than swept up. From f6087a1a851ad78e1f77d65ae4f1e4af09b75d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 17:54:57 +0000 Subject: [PATCH 09/31] chore(context): retire cleanup-data-loss into the archive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five phases done and both gates passed on each, so the feature leaves roadmap.md for a history.md row and its plan moves to context/archive/ with git mv, keeping the history of how it was actually built. The plan's header is repointed at the history row rather than stamped with an outcome, and now names the five source comments that cite its sections by number — those citations carry no path, so they survived the move untouched and must not be rewritten into paths that would rot. The sweep found exactly one path reference, roadmap.md:53, which the entry removal deleted outright; no link needed rewriting, since plans/ and archive/ sit at the same depth from context/. F-001 (closed) moves into the archived plan's §10 log. F-002 through F-005 stay open in findings.md — all P3, none gating. The archive records them, and records that §7's by-hand verification was never run. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- .../CLEANUP-DATA-LOSS-PLAN.md | 53 +++++++++++++++++-- context/findings.md | 15 ++---- context/history.md | 1 + context/roadmap.md | 8 --- 4 files changed, 54 insertions(+), 23 deletions(-) rename context/{plans => archive}/CLEANUP-DATA-LOSS-PLAN.md (90%) diff --git a/context/plans/CLEANUP-DATA-LOSS-PLAN.md b/context/archive/CLEANUP-DATA-LOSS-PLAN.md similarity index 90% rename from context/plans/CLEANUP-DATA-LOSS-PLAN.md rename to context/archive/CLEANUP-DATA-LOSS-PLAN.md index d7f4c2e..33a8750 100644 --- a/context/plans/CLEANUP-DATA-LOSS-PLAN.md +++ b/context/archive/CLEANUP-DATA-LOSS-PLAN.md @@ -1,10 +1,14 @@ # Cleanup Data Loss Plan -Written 2026-09-05. Makes `safeToRemove` mean what its name says, so `worktree cleanup` stops force-removing -worktrees that hold uncommitted work. The `cleanup-data-loss` entry in [`../roadmap.md`](../roadmap.md) is -where this feature's status lives. +Retired — its outcome and date are in [`../history.md`](../history.md). -**Phase status lives in §6.1 of this document, and nowhere else.** +Made `safeToRemove` mean what its name says, so `worktree cleanup` stops force-removing worktrees that hold +uncommitted work. + +**Its section numbers are cited from source comments.** `src/lib/git.ts:160`, `src/commands/cleanup.ts:16,77`, +`src/commands/cleanup.test.ts:324` and `src/commands/remove.test.ts:313` reference `CLEANUP-DATA-LOSS-PLAN` +by section and without a path, so they survived this move untouched — but renumbering or deleting a section +below breaks them. --- @@ -424,3 +428,44 @@ print each candidate with its uncommitted count. The accurate, narrower statemen matters because it changes what the fix has to do: the information is displayed but bundled into one bulk yes/no, `--force` skips the display entirely, and the real defect is the classification rather than the reporting. + +## 10. Findings log + +Closed findings tied to this feature, moved here from [`../findings.md`](../findings.md) at +`/feature-close` so that file does not grow for the life of the project. + +### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it + +**Tied to:** Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) · +**Closed:** 2026-09-05 (Gate 1, Phase 2) + +Phase 2 dropped the `wt.uncommittedChanges === 0` clause per §4.1 and pinned the resulting verdict: the +no-remote entry with an unknown count is `true`, asserted at `src/lib/git.test.ts:321-323` against +`entry()`'s defaults of `pathExists: true, remote: ""` (`src/lib/git.test.ts:245-253`). Gate 1 re-passed on +that run — `pnpm check`, `pnpm typecheck`, `pnpm build`, `pnpm test` (12 files, 189 tests) and +`pnpm docs:test` (6 files, 49 tests) all exit 0. + +### Still open at retirement + +Four `P3` findings tied to this feature were open when it was retired, and **stay in +[`../findings.md`](../findings.md)** — only closed findings move here. None gated the close; `P3` blocks +nothing. They are recorded here so the archive does not read as if the feature retired clean: + +| Id | Phase | What it is | +|---|---|---| +| F-002 | 2 | §4.1's `pathExists`-first ordering is argued but unpinned by any test | +| F-003 | 3 | `{ pathExists: false, uncommittedChanges: 3 }` would be listed as skipped *and* removed | +| F-004 | 4 | the `remove` regression cases are looser than the §4.3 claim they pin | +| F-005 | 5 | `README.md:160` still carries the unqualified claim §4.4 corrected on the docs page | + +F-002 and F-003 are both the `pathExists: false` ordering question, latent because +`gitGetWorktreeList` hardcodes the count to `0` when the path is missing. `agent-mode` Phase 6 adds a +live-agent clause to this same predicate (§2, R2) and is the natural place to settle them. + +### §7 was not performed + +**The by-hand verification in §7 was never run.** Every phase passed both gates on unit tests over +synthetic `WorktreeListEntry` objects, which is exactly what §7 says is insufficient: no test in this +feature exercised `gitGetWorktreeList` against a real repository, so nothing proved that the real list +builder produces the field values the corrected predicate depends on. Step 7 of that walkthrough — which +confirms Q1's remaining gap rather than fixing it — is likewise unconfirmed. diff --git a/context/findings.md b/context/findings.md index cedc4aa..8ad7e31 100644 --- a/context/findings.md +++ b/context/findings.md @@ -122,15 +122,8 @@ as `page.mdx:17`. ## Closed -### F-001 — P3 — `uncommittedChanges: undefined` with no remote is unpinned, and Phase 2 flips it - -**Tied to:** cleanup-data-loss Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) · -**Closed:** 2026-09-05 (Gate 1, Phase 2) - -Phase 2 dropped the `wt.uncommittedChanges === 0` clause per §4.1 and pinned the resulting verdict: the -no-remote entry with an unknown count is `true`, asserted at `src/lib/git.test.ts:321-323` against -`entry()`'s defaults of `pathExists: true, remote: ""` (`src/lib/git.test.ts:245-253`). Gate 1 re-passed on -that run — `pnpm check`, `pnpm typecheck`, `pnpm build`, `pnpm test` (12 files, 189 tests) and -`pnpm docs:test` (6 files, 49 tests) all exit 0. - +None. Closed findings leave this file — a feature's at `/feature-close`, folded into the retiring plan's +own log; an `ad-hoc` one at the start of the next `/orchestrate`. +`cleanup-data-loss`'s F-001 moved to +[`archive/CLEANUP-DATA-LOSS-PLAN.md`](archive/CLEANUP-DATA-LOSS-PLAN.md) §10 on 2026-09-05. diff --git a/context/history.md b/context/history.md index dae968e..8e7c3a2 100644 --- a/context/history.md +++ b/context/history.md @@ -11,3 +11,4 @@ except git history, and this row is what stops it being re-proposed. | Date | Feature | Outcome | Why | Document | |---|---|---|---|---| +| 2026-09-05 | cleanup-data-loss | shipped | `isSafeToRemove` called a deleted-remote worktree safe while it held uncommitted work, and `cleanup` force-removed it; the hazard test is now hoisted above every remote branch and `cleanup` names what it held back | [`archive/CLEANUP-DATA-LOSS-PLAN.md`](archive/CLEANUP-DATA-LOSS-PLAN.md) | diff --git a/context/roadmap.md b/context/roadmap.md index a1950d5..97addeb 100644 --- a/context/roadmap.md +++ b/context/roadmap.md @@ -44,14 +44,6 @@ and the text scrolls out of sight instead of the field growing. - **Size:** small — one component (`ChatInput`) plus its ref type and submit key handling in `ChatForm` - **Doc:** none yet -### cleanup-data-loss — `active` - -`worktree cleanup` classifies a worktree whose remote branch was deleted as safe to remove without ever -checking for uncommitted changes, then force-removes it — destroying work in progress. - -- **Size:** small — one predicate in `isSafeToRemove`, plus how `cleanup` reports what it skipped -- **Doc:** [`plans/CLEANUP-DATA-LOSS-PLAN.md`](plans/CLEANUP-DATA-LOSS-PLAN.md) — 5 phases, 5 open questions - ### shell-argv-safety — `pending` `cmd()` runs every git call through a shell as an interpolated string, so a repo path containing a space From 4e23769ab2fd4eda7cd9b910297632737f6ec8d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 18:23:21 +0000 Subject: [PATCH 10/31] docs(context): activate shell-argv-safety and settle its sequencing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promotes the shell-argv-safety draft to a plan with a phase ledger and takes the one active-feature slot with it. It goes ahead of agent-mode because both plans' open questions argue for it from opposite sides: agent-mode Phase 1 pushes `agent.command` — a value that contains spaces — through the `gitSetConfigValue` interpolation that this plan's Phase 4 replaces with argv. Settles two of the four open questions: - Q1, sequencing: this plan holds the slot. Phase 6 therefore creates `src/lib/base-command.test.ts` and agent-mode Phase 2 merges into it. - Q4, the helper's name: `run`, confirmed against the tree. No `src/commands/*.ts` imports from `cli.js`, so the only file that sees both it and oclif's inherited method is `base-command.ts` after Phase 6, where the two read as `run()` and `this.run()`. Q2 and Q3 stay deferred — no phase touches either. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/drafts/shell-argv-safety.md | 123 -------- context/plans/SHELL-ARGV-SAFETY-PLAN.md | 392 ++++++++++++++++++++++++ context/roadmap.md | 4 +- 3 files changed, 394 insertions(+), 125 deletions(-) delete mode 100644 context/drafts/shell-argv-safety.md create mode 100644 context/plans/SHELL-ARGV-SAFETY-PLAN.md diff --git a/context/drafts/shell-argv-safety.md b/context/drafts/shell-argv-safety.md deleted file mode 100644 index 1aff437..0000000 --- a/context/drafts/shell-argv-safety.md +++ /dev/null @@ -1,123 +0,0 @@ -# shell-argv-safety — supplied reference material - -Notes, not a design. Captured by `/roadmap` on **2026-09-05**. `/feature-plan` turns this into -`plans/SHELL-ARGV-SAFETY-PLAN.md`; nothing here is a decision. - -## Provenance - -- Source: maintainer, pasted into `/roadmap` on 2026-09-05. Originated as analysis done while planning - `agent-mode`, where it is recorded as risk R1 in `plans/AGENT-MODE-PLAN.md`. -- The **original** maintainer brief for `agent-mode` called this a single-site, commit-sized `/orchestrate` - task citing only `src/lib/base-command.ts:56`. The inventory below is why it was filed as its own entry - instead. -- Every citation re-verified against the tree on 2026-09-05, branch `feature/add-agent-mode`. Two claims in - the supplied material were adjusted — see "Corrections". - -## The shape of the problem — one chokepoint, one bypass - -The supplied material framed this as "at least seven sites across two files". That count is right but the -framing understates how tractable it is. `grep -rn 'exec(\|execSync\|spawn(\|execFile' src/` over non-test -sources returns exactly **two** hits: - -| Site | What it is | -|---|---| -| `src/lib/cli.ts:17` | `exec(cmd, …)` inside `cmd()` — **the single shell boundary every git call funnels through** | -| `src/lib/base-command.ts:56` | `exec(\`${codeEditor} ${path}\`, …)` — the one call that **bypasses `cmd()`** and shells out directly | - -So this is not seven independent bugs. It is one helper with a string-shaped contract, one caller that -skipped the helper, and a set of call sites that interpolate into that contract. - -## Why the `cd` workarounds exist - -`CmdOptions` is `{ debug?: boolean }` (`src/lib/cli.ts:3-5`) — **there is no `cwd` option.** That absence is -the direct cause of five of the interpolation sites, which all shell out to `cd` to reach a worktree: - -``` -src/lib/git.ts:86 `cd ${branchPath} && git rev-list --count @{u}..HEAD` -src/lib/git.ts:95 `cd ${branchPath} && git rev-list --count HEAD..@{u}` -src/lib/git.ts:103 `cd ${branchPath} && git status -s` -src/lib/git.ts:231 `cd ${gitRootPath}` ─┐ composed at :241 into one chained -src/lib/git.ts:239 `cd ${currentPath}` ─┘ `${cdRoot} && ${gitFetch} && ${addWorktree} && ${gotoBack}` -``` - -**Adding a `cwd` option deletes these rather than escaping them.** That is the important consequence: the -fix is mostly subtraction, and `child_process.execFile` already takes `{ cwd }`. - -## Full inventory of `cmd()` call sites - -**18** non-test call sites (a naive grep finds 14 — four are formatted across lines and need -`grep -n '\bcmd('` to catch: `git.ts:85, 94, 108, 260`). - -**Ten pass a static string** and are already safe: `git.ts:28, 32, 36, 65, 71, 76, 108, 133` and -`integrations/github.ts:113, 147`. - -**Eight interpolate**, and are the work: - -| Site | Interpolates | Source of the value | -|---|---|---| -| `git.ts:17` | `${name}` | a `ConfigName` from a fixed union — low risk | -| `git.ts:24` | `${name}`, `${value}` into `git config … "${value}"` | **arbitrary user input**, double-quoted only | -| `git.ts:86` | `${branchPath}` | filesystem path | -| `git.ts:95` | `${branchPath}` | filesystem path | -| `git.ts:103` | `${branchPath}` | filesystem path | -| `git.ts:241` | four commands chained with `&&`, composed from `231`, `236-237`, `239` — carrying `${branchName}`, `${worktreePath}`, `${sourceBranch}`, `${gitRootPath}`, `${currentPath}` | branch name + filesystem paths | -| `git.ts:261-263` | `${branchName}` ×2 into `git worktree remove` / `git branch -D` | branch name | -| `cli.ts:34` | `${checkCommand} ${baseCommand}` | `commandExists`, already head-split | - -Plus `src/lib/base-command.ts:56` — the direct-`exec` bypass, which is not a `cmd()` call site at all. - -## Demonstrated, not asserted - -Run on 2026-09-05 against a directory whose path contains a space: - -``` -the pattern the CLI builds today: - exec(`cd ${branchPath} && git status -s`) - → FAILS: Command failed: cd /…/tmp/space demo && git status -s - -the same call with an argv array and a cwd option: - execFile("git", ["status", "-s"], { cwd: branchPath }) - → ok — no shell, no quoting -``` - -## Corrections to the supplied material - -- **`git.ts:94` should be `git.ts:95`.** Line 94 is `const countStr = await cmd(`; the interpolated string - is on the following line. Line 86 was cited correctly because that call is formatted differently. -- **"Every subprocess call … builds a shell string" is too broad.** Ten of the eighteen `cmd()` call sites - pass static strings with nothing interpolated. The defect is in the *contract* — `cmd()` accepts a string - and runs it through a shell — not in every caller. - -## What already holds in this repo - -| Claim | Status | -|---|---| -| `cmd()` is the only shell boundary for git calls | confirmed, `src/lib/cli.ts:7-25` | -| `base-command.ts:56` bypasses `cmd()` and calls `exec` directly | confirmed | -| `CmdOptions` has no `cwd` | confirmed, `src/lib/cli.ts:3-5` | -| Five interpolation sites exist only to work around that | confirmed, `git.ts:86, 95, 103, 231, 239` | -| `git.ts:241` chains four commands with `&&` in one shell string | confirmed | -| `commandExists` already splits on whitespace and checks only the head | confirmed, `src/lib/cli.ts:27-39` | -| No `src/lib/base-command.test.ts` exists | confirmed — a new file either way | -| `src/integrations/` contains no `exec`/`spawn` of its own | confirmed | -| A path containing a space fails today | **demonstrated above** | - -## Not decided here - -- Whether `cmd()` changes signature to `(file, args[], opts)`, gains an overload, or is replaced by a new - helper with the old one kept for static strings. -- What happens to the chained command at `git.ts:241` — four sequential `execFile` calls with `{ cwd }`, - or keep one shell call with proper quoting. The `cd`-back-afterwards half becomes unnecessary with `cwd`. -- Whether branch names need validation as well as escaping. `isValidBranchName` - (`src/lib/validators.ts:22-55`) already rejects spaces and several metacharacters, but it is not applied - on every path a branch name reaches `cmd()` by. -- Whether `debug: true` in `CmdOptions` still makes sense once commands are argv arrays. - -## Relationship to other entries - -- **`agent-mode`** records this as risk R1. Its Phase 1 stores `agent.command` — a value containing spaces — - through `gitSetConfigValue` (`git.ts:24`), and its Phase 2 edits `base-command.ts`. Doing this first means - agent mode is not built on the broken contract. Note that `agent-mode`'s own decision D2 already requires - `spawn` with an argv array for agent dispatch, so that one path is safe regardless. -- **`cleanup-data-loss`** touches `git.ts` too (`isSafeToRemove`, and `gitNukeWorktreeCmd` at `261-263` is - in this entry's table). The two overlap in that function; sequencing them avoids a conflict. diff --git a/context/plans/SHELL-ARGV-SAFETY-PLAN.md b/context/plans/SHELL-ARGV-SAFETY-PLAN.md new file mode 100644 index 0000000..61883d5 --- /dev/null +++ b/context/plans/SHELL-ARGV-SAFETY-PLAN.md @@ -0,0 +1,392 @@ +# shell-argv-safety Plan + +Written 2026-09-05. Removes the shell from this CLI's subprocess calls, replacing an interpolated +command-string contract with an argv-array one. The `shell-argv-safety` entry in +[`../roadmap.md`](../roadmap.md) is where this feature's status lives. + +**Phase status lives in §6.1 of this document, and nowhere else.** + +Built on the reference material captured by `/roadmap` on 2026-09-05, which this document replaces. That +material's provenance is carried forward in §0, and its inventory — re-verified and corrected — is §10. + +--- + +## 0. Provenance of the source material + +- **Source:** maintainer, pasted into `/roadmap` on 2026-09-05. Originated as analysis done while planning + `agent-mode`, where it is recorded as risk R1 in [`AGENT-MODE-PLAN.md`](AGENT-MODE-PLAN.md). +- The **original** `agent-mode` brief called this a single-site, commit-sized `/orchestrate` task citing + only `src/lib/base-command.ts:56`. The inventory is why it was filed as its own roadmap entry instead. +- The draft's two corrections to the supplied material (`git.ts:94` → `:95`, and "every subprocess call + builds a shell string" being too broad) **both still hold** and are carried into §10. +- **Every line number in the draft was re-verified against the tree on 2026-09-05 while writing this plan, + and four had drifted.** See §10's "Corrections to the draft" — the draft is now three commits stale, and + the phases below cite the current numbers. + +## 1. Why + +`cmd()` (`src/lib/cli.ts:7-25`) runs every git call through `child_process.exec`, which means a shell, +which means every value interpolated into it is parsed as shell syntax. Two consequences, one live today +and one latent: + +**A path containing a space fails.** Re-demonstrated first-hand on 2026-09-05, not inherited from the +draft — the exact pattern the CLI builds at `src/lib/git.ts:103`, against a directory whose path contains +a space: + +``` +today's pattern -> FAILS: Command failed: cd /…/tmp/space demo && git status -s +argv + cwd -> ok +``` + +The second line is `execFile("git", ["status", "-s"], { cwd })` — the same work, no shell, no quoting. + +**A config value is an injection vector.** `gitSetConfigValue` (`src/lib/git.ts:23-25`) interpolates an +arbitrary user-supplied value into `git config … "${value}"` with nothing but double quotes around it. A +value containing `"` closes the quote; a backtick or `$(…)` executes. + +The shape of the problem is better than the raw count suggests. +`grep -rn --include='*.ts' 'exec(\|execSync\|spawn(\|execFile' src/` over non-test sources returns exactly +**two** hits: + +| Site | What it is | +|---|---| +| `src/lib/cli.ts:17` | `exec(cmd, …)` inside `cmd()` — **the single shell boundary every git call funnels through** | +| `src/lib/base-command.ts:56` | `exec(\`${codeEditor} ${path}\`, …)` — the one call that **bypasses `cmd()`** | + +So this is not eighteen independent bugs. It is **one helper with a string-shaped contract, one caller that +skipped the helper, and eight call sites that interpolate into that contract.** Ten further call sites pass +static strings and are already safe. + +**Five of the eight interpolations exist only to work around a missing option.** `CmdOptions` is +`{ debug?: boolean }` (`src/lib/cli.ts:3-5`) — there is no `cwd`. So five sites shell out to `cd` to reach a +worktree: + +``` +src/lib/git.ts:86 `cd ${branchPath} && git rev-list --count @{u}..HEAD` +src/lib/git.ts:95 `cd ${branchPath} && git rev-list --count HEAD..@{u}` +src/lib/git.ts:103 `cd ${branchPath} && git status -s` +src/lib/git.ts:237 `cd ${gitRootPath}` ─┐ composed at :247 into one chained +src/lib/git.ts:245 `cd ${currentPath}` ─┘ `${cdRoot} && ${gitFetch} && ${addWorktree} && ${gotoBack}` +``` + +**Adding a `cwd` option deletes these rather than escaping them.** That is the load-bearing consequence for +how this plan is phased: the fix is mostly subtraction, and `execFile` already takes `{ cwd }`. + +## 2. Constraints + +- **No behaviour change that a user can see**, except the two that are the point: paths with spaces start + working, and hostile config values stop executing. Command output, error text and exit behaviour stay as + they are. +- **The suite must stay green at every phase boundary.** This is a refactor of a contract that 18 call + sites and 16 test assertions depend on; a phase that leaves the tree red is not commit-sized. +- **No new runtime dependency.** `node:child_process` already provides `execFile`. Adding a shell-quoting + library would be the wrong direction — it keeps the shell. +- From [`../stack.md`](../stack.md): ESM throughout, relative imports keep the `.js` extension; `export + default` only in `src/commands/*.ts`; tests colocated as `*.test.ts`, vitest. Never add a file named + `biome.json` or `biome.jsonc` anywhere in the tree. +- Console output is chalk-styled and TTY-dependent; assertions on printed text rely on the + `FORCE_COLOR: "0"` pin in `vitest.config.ts`. + +## 3. Decisions + +**D1. A new `run(file, args, opts)` lands alongside `cmd()`, call sites migrate in batches, and `cmd()` is +deleted when its last caller is gone.** *Rejected:* changing `cmd()`'s signature in place — that moves 18 +call sites, 16 assertions and the global mock in one commit, which is not a commit-sized unit and hands the +review gate one undifferentiated diff. *Rejected:* an overload on `cmd()` — a union signature keeps the +string path reachable and reviewable-as-normal forever, and the goal here is to **remove** that path, not to +park a safer one next to it. + +**D2. `run` is built on `execFile`, not `spawn`.** `execFile` buffers stdout and hands it back, which is +exactly what all 18 call sites want — every one of them parses stdout. `spawn` would push chunk +accumulation into each caller. *Note:* `agent-mode` D2 chooses `spawn` for agent dispatch, and that is not +in tension — it dispatches a detached, long-lived process whose output is deliberately not collected. + +**D3. `run` takes `{ cwd }`, and the five `cd` sites are deleted rather than quoted.** This is the whole +reason the change is mostly subtraction (§1). A per-call `cwd` also never mutates the process's own working +directory, which is what makes the `cd`-back halves unnecessary (D5). + +**D4. `CmdOptions.debug` is dropped, not carried onto `run`.** Verified dead: it is defined at +`src/lib/cli.ts:4,9,12` and **passed by no caller** — `grep -rn --include='*.ts' 'debug' src/` returns only +those three lines. *Rejected:* preserving it — it is a branch that logs, resolves `""` and skips execution +entirely, so setting it on a migrated call site would silently no-op the command rather than run it. + +**D5. Chained `&&` commands become sequential `await run(…)` calls.** Short-circuit semantics are preserved +exactly: an `await` that rejects stops the sequence, which is what `&&` did. The `cd`-back halves disappear +under D3. + +**D6. `openWorktreePath` splits `codeEditor` on whitespace — head as the file, tail as leading args.** This +is the contract `commandExists` (`src/lib/cli.ts:27-39`) **already** applies, since it does +`command.split(" ")[0]` and checks only the head. Today validation and execution disagree: `commandExists` +validates `code`, the shell then runs the whole string. After this, they agree. *Rejected:* a shell-quoting +library, per §2. + +**D7. `src/test-setup.ts` gains a `run` mock in the same commit that adds `run` to `cli.ts`.** The global +`vi.mock("./lib/cli.js", …)` factory returns an **explicit object** (`{ cmd, commandExists }`), so any export +missing from it is `undefined` at call time. A migrated call site would not fail with a useful assertion — +it would throw "run is not a function". This is a sequencing constraint, not a preference. + +**D8. Migration order is read-only sites first, filesystem-mutating sites last.** Ahead/behind/uncommitted +counts are pure reads (Phase 2). `gitCreateWorktree` and `gitNukeWorktree` create and destroy directories +(Phases 3, 4), and go after the pattern is established and covered. + +## 4. Design + +### 4.1 The helper + +```ts +// src/lib/cli.ts +interface RunOptions { cwd?: string } + +export function run(file: string, args: string[] = [], { cwd }: RunOptions = {}): Promise +``` + +`execFile(file, args, { cwd })`, resolving `stdout?.trim() ?? ""` and rejecting on error — the same +resolve/reject shape `cmd()` has today, so no caller's error handling changes. `cmd()` stays untouched +until Phase 5. + +### 4.2 What each call-site group becomes + +| Group | Today | After | +|---|---|---| +| three `cd ${branchPath}` reads | `cmd(\`cd ${p} && git status -s\`)` | `run("git", ["status", "-s"], { cwd: p })` | +| `gitCreateWorktree`'s chain | one `cmd()` with four `&&`-joined commands | two sequential `run`s, both `{ cwd: gitRootPath }` | +| `gitNukeWorktreeCmd`'s chain | one `cmd()` with three `&&`-joined commands | three sequential `run`s | +| config get/set | `cmd(\`git config … "${value}"\`)` | `run("git", ["config", name, value])` | +| ten static sites | `cmd("git --no-pager branch")` | `run("git", ["--no-pager", "branch"])` | +| `openWorktreePath` | `exec(\`${codeEditor} ${path}\`)` | argv-split head + `[...tail, path]` | + +### 4.3 What disappears + +- The `cd …` prefix at five sites, and the `&&` chaining at two. +- `const currentPath = process.env.PWD` (`src/lib/git.ts:230`) and the `gotoBack` it feeds — the only + `process.env.PWD` read in the codebase. +- `CmdOptions.debug` (D4), and eventually `cmd()` and the `exec` import in `cli.ts`. + +### 4.4 The test surface + +This is the part the draft does not cover, and it is what sizes the work. `src/test-setup.ts` mocks +`./lib/cli.js` **globally** and its `afterEach` maps `mockCmd.mock.calls` to `call[0]` — a command string. +Sixteen assertions across `src/lib/git.test.ts` (23 mock references), `src/integrations/github.test.ts` (7) +and `src/integrations/jira.test.ts` (2) assert on that string. Each migrated call site moves its assertion +from `toHaveBeenCalledWith("git …")` to `toHaveBeenCalledWith("git", […], { cwd })`, in the same commit as +the source change. + +## 5. Risks + +**R1 — `gitCreateWorktree` is the highest-consequence migration.** The comment at `src/lib/git.ts:235-236` +says the `cd` to the git root is deliberate: it makes `git worktree add` receive a **relative** +`worktreePath` so the layout survives the project being moved on disk. Getting `cwd` wrong there does not +error — it creates a worktree in the wrong place. Response: Phase 3 asserts both the `cwd` and the relative +path shape, and §7 case 2 checks the resulting `.git` file by hand. + +**R2 — a `codeEditor` value with quoted arguments regresses.** `code -n` splits correctly on whitespace. +`open -a "Visual Studio Code"` does not — today the shell parses those quotes, and after D6 it becomes three +argv entries. Narrow but real, and it is a behaviour change a user could see, against §2. Response: Phase 6 +documents the contract on the configuration page. See §8 Q2 — whether to reject such values at config time +is not settled here. + +**R3 — the global mock warns instead of failing.** `src/test-setup.ts`'s `afterEach` `console.warn`s on +unexpected `cmd` calls; it does not fail the test. So a migration that changes *which* commands are issued +can pass a green suite while printing a warning nobody reads. Response: each phase's **Done when** names the +assertion, not just the green run, and §7 case 4 greps the captured output for the warning. + +**R4 — overlap with two other roadmap entries.** `agent-mode` Phase 2 edits `src/lib/base-command.ts` and +creates `src/lib/base-command.test.ts` — the same file this plan's Phase 6 creates. `cleanup-data-loss` +touched `src/lib/git.ts`'s `isSafeToRemove`, and `gitNukeWorktreeCmd` is in this plan's Phase 4. Response: +this is a sequencing question, not a design one — see §8 Q1. + +**R5 — `execFile` has a default `maxBuffer`.** `exec` and `execFile` both default to 1 MB of stdout in +current Node. `git worktree list` or `git --no-pager branch -r` in a very large repository could exceed it, +and the failure mode is a rejected promise, not truncation. Unchanged from today — `exec` has the same +default — so this is not a regression, but it is now worth knowing. Not mitigated. + +## 6. Phases + +### 6.1 Status ledger + +| # | Phase | Status | Depends on | Note | +|---|---|---|---|---| +| 1 | `run()` helper, `cwd` support, and its global mock | not started | — | | +| 2 | The three read-only `cd` sites | not started | 1 | | +| 3 | `gitCreateWorktree`'s four-command chain | not started | 1 | | +| 4 | Config get/set and `gitNukeWorktreeCmd` | not started | 1 | | +| 5 | Static sites, `commandExists`, and deleting `cmd()` | not started | 2, 3, 4 | | +| 6 | `openWorktreePath` — the last `exec` | not started | 1 | | + +Status is one of `not started`, `in progress`, `blocked`, `done`. `done` only when committed and verified, +and whoever finishes a phase updates the row in the same commit. + +**Exactly one table in this document has these columns.** Do not add a second phase table — a +differently-shaped one nearby is a decoy that gets read by mistake. + +### 6.2 The phases + +#### Phase 1 — `run()` helper, `cwd` support, and its global mock + +**Files:** `src/lib/cli.ts`, `src/lib/cli.test.ts` (new), `src/test-setup.ts` + +**Scope:** Add `run(file, args, opts)` per §4.1 — `execFile`, `{ cwd }`, same resolve/reject shape as +`cmd()`. Add `run` to the `vi.mock` factory in `src/test-setup.ts` (D7). **No call site migrates in this +phase** and `cmd()` is untouched. `src/lib/cli.test.ts` does not exist today — this phase creates it. + +**Done when:** `run("git", ["status", "-s"], { cwd })` resolves trimmed stdout, rejects on a non-zero exit, +and passes `cwd` through, all covered in `cli.test.ts`; `pnpm test` is green with `cmd()` still present and +**every existing assertion unmodified**. + +#### Phase 2 — The three read-only `cd` sites + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts` + +**Scope:** Migrate `gitGetCommitsAheadCount` (`git.ts:85-87`), `gitGetCommitsBehindCount` (`git.ts:94-96`) +and `gitGetUncommittedChangesCount` (`git.ts:103`) to `run("git", […], { cwd: branchPath })`. The +`cd ${branchPath} &&` prefix is deleted, not quoted (D3). Move each function's assertion to the argv form +(§4.4). + +**Done when:** none of the three functions' bodies contain the string `cd `; their tests assert +`("git", [...], { cwd })`; a `branchPath` containing a space produces a correct call (§7 case 1). + +#### Phase 3 — `gitCreateWorktree`'s four-command chain + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts` + +**Scope:** Replace the `${cdRoot} && ${gitFetch} && ${addWorktree} && ${gotoBack}` chain +(`git.ts:237-247`) with two sequential `run` calls under `{ cwd: gitRootPath }` (D5). Delete `gotoBack` +and the `process.env.PWD` read at `git.ts:230`. **Preserve the relative `worktreePath`** and the comment +at `git.ts:235-236` explaining why it is relative (R1). + +**Done when:** `gitCreateWorktree` issues exactly two subprocess calls, both with `cwd` at the git root; +the `worktree add` argv still carries the **relative** path; both the `isCheckout` and non-`isCheckout` +branches are covered; `process.env.PWD` appears nowhere in `src/`. + +#### Phase 4 — Config get/set and `gitNukeWorktreeCmd` + +**Files:** `src/lib/git.ts`, `src/lib/git.test.ts` + +**Scope:** Migrate `gitGetConfigValue` (`git.ts:17`) and `gitSetConfigValue` (`git.ts:24`) — the +arbitrary-value site that is `agent-mode`'s R1 — plus `gitNukeWorktreeCmd`'s three-command chain +(`git.ts:266-270`) into sequential `run` calls (D5). + +**Done when:** a config value containing `"`, a backtick and `;` round-trips through set-then-get unchanged +and is asserted as a single argv element; `gitNukeWorktreeCmd` issues three sequential calls that stop at +the first rejection; the `force` branch still appends `--force`. + +#### Phase 5 — Static sites, `commandExists`, and deleting `cmd()` + +**Files:** `src/lib/cli.ts`, `src/lib/cli.test.ts`, `src/lib/git.ts`, `src/lib/git.test.ts`, +`src/integrations/github.ts`, `src/integrations/github.test.ts`, `src/integrations/jira.test.ts`, +`src/test-setup.ts` + +**Scope:** Migrate the ten static call sites (`git.ts:28, 32, 36, 65, 71, 76, 108, 133` and +`github.ts:113, 147`) and `commandExists` (`cli.ts:34`). Then delete `cmd()`, `CmdOptions.debug` (D4), the +`exec` import in `cli.ts`, and the `cmd` entry in the global mock factory. + +**Done when:** `grep -rn --include='*.ts' '\bcmd(' src/` returns nothing outside `*.test.ts` history; +`grep -rn --include='*.ts' 'exec(' src/` returns only `src/lib/base-command.ts`; `pnpm test` and +`pnpm typecheck` green. + +#### Phase 6 — `openWorktreePath` — the last `exec` + +**Files:** `src/lib/base-command.ts`, `src/lib/base-command.test.ts` (new), +`docs/src/app/docs/configuration/page.mdx` + +**Scope:** Replace `exec(\`${codeEditor} ${path}\`)` (`base-command.ts:56`) with the D6 argv split, keeping +the existing ora spinner success/fail behaviour exactly. Document on the configuration page that +`codeEditor` is a command line split on whitespace, and that quoted arguments are not supported (R2). +`src/lib/base-command.test.ts` does not exist today — this phase creates it. **See R4: `agent-mode` Phase 2 +creates the same file.** + +**Done when:** `grep -rn --include='*.ts' 'exec(' src/` returns nothing outside `*.test.ts`; a worktree path +containing a space opens; `base-command.test.ts` asserts the argv without launching a real editor; the +spinner still fails with the error message on a rejected call. + +## 7. Verification + +[`../verify.md`](../verify.md) names the commands — this file does not repeat them. Beyond Gate 1: + +1. **The space-path case, by hand, at Phase 2.** Create a worktree under a path containing a space and run + `worktree list`. Ahead/behind/uncommitted counts must be real numbers, not blanks. This is the §1 failure + reproduced against the real CLI rather than a scratch script. +2. **The relative-path check, by hand, at Phase 3.** After `worktree branch `, read the `.git` file in + the new worktree and confirm it points at a path of the same shape as before this change (R1). Then move + the repository directory and confirm the worktree still resolves — that is what the relative path buys. +3. **The hostile-value case, at Phase 4.** `worktree config codeEditor 'x"; touch /tmp/pwned; #'` must store + the literal string and create no file. +4. **Grep the test output for the R3 warning** at every phase: a run that prints + `Unexpected cmd calls detected` is a failure even when vitest is green. + +## 8. Open questions + +- **Q1 — sequencing against `agent-mode` and `cleanup-data-loss`. Settled 2026-09-05: this plan holds the + slot.** Activated ahead of `agent-mode` on the argument it and `agent-mode`'s §8 Q4 both make — that + `agent-mode` Phase 1 pushes `agent.command`, a value containing spaces, through the `gitSetConfigValue` + quoting this plan's Phase 4 fixes. The consequence for Phase 6 stands: it **creates** + `src/lib/base-command.test.ts`, and `agent-mode` Phase 2 merges into it rather than creating it. +- **Q2 — should a `codeEditor` value with quotes be rejected at config time?** R2 makes such a value + silently misbehave after D6. `isValidConfigValue` (`src/lib/validators.ts:57-70`) is where a check would + go, and `agent-mode` D1 proposes an `isValidCommandLine` for exactly this shape. Deferring: adding the + validator here would collide with that plan's Phase 1. +- **Q3 — do branch names need validation as well as argv-safety?** Carried from the draft, unresolved. + `isValidBranchName` (`src/lib/validators.ts:22-55`) already rejects spaces and several metacharacters, but + it is **not applied on every path a branch name reaches a subprocess call by** — `gitNukeWorktreeCmd` takes + whatever it is handed. Argv-safety makes this non-exploitable, so it is now a correctness question rather + than a security one. +- **Q4 — is `run` the right name? Settled 2026-09-05: yes, `run`.** Confirmed against the tree rather than + waved through: no `src/commands/*.ts` imports from `cli.js` today, so the only file that will see both + names is `src/lib/base-command.ts` after Phase 6, where the inherited oclif method is reached as + `this.run()` and the helper as `run()` — distinct to TypeScript and to a reader. `execCmd` was the + considered alternative and was rejected for costing a rename across §4.1, all six phases and §10 to buy + nothing. `sh` was rejected as actively misleading: the point of the change is that no shell is involved. + **Do not revisit.** + +## 9. Surfaces to update — all verified to exist + +- `docs/src/app/docs/configuration/page.mdx` — the `codeEditor` contract note (Phase 6). It documents the + value at lines 12 and 28 with no mention of arguments today. +- **No generated-surface sweep is needed, and this was checked rather than assumed.** `skills/core/SKILL.md` + lists `src/lib/git.ts` and `src/lib/validators.ts` in `sources`, and both paths survive; its frontmatter + `description` enumerates commands and config values, none of which change. No command is added, so + `docs/src/app/docs/commands/_meta.ts` is untouched. +- `docs/src/app/docs/changelog/page.mdx` — **not touched.** It explicitly records a "single latest-docs + strategy" with no per-release notes, so there is no entry to add. +- `README.md` — no feature-list change; nothing in it describes subprocess behaviour. + +## 10. What already holds in this repo + +Read, not recalled — checked 2026-09-05 on `feature/add-agent-mode`. The first nine rows are the draft's +own table, re-verified; the rest were found while writing this plan. + +| Claim | Status | +|---|---| +| `cmd()` is the only shell boundary for git calls | confirmed, `src/lib/cli.ts:7-25` | +| `base-command.ts:56` bypasses `cmd()` and calls `exec` directly | confirmed | +| `CmdOptions` has no `cwd` | confirmed, `src/lib/cli.ts:3-5` | +| Five interpolation sites exist only to work around that | confirmed, `git.ts:86, 95, 103, 237, 245` | +| The create-worktree call chains four commands with `&&` | confirmed, `git.ts:247` | +| `commandExists` splits on whitespace and checks only the head | confirmed, `src/lib/cli.ts:27-39` | +| No `src/lib/base-command.test.ts` exists | confirmed, `ls src/lib/` — Phase 6 creates it | +| `src/integrations/` contains no `exec`/`spawn` of its own | confirmed | +| A path containing a space fails today | **re-demonstrated 2026-09-05** — see §1 | +| Exactly 18 non-test `cmd()` call sites; 10 static, 8 interpolating | confirmed by `grep -rn --include='*.ts' '\bcmd(' src/` | +| `CmdOptions.debug` is passed by no caller | confirmed — `grep` returns only `cli.ts:4,9,12` | +| No `src/lib/cli.test.ts` exists | confirmed, `ls src/lib/` — Phase 1 creates it | +| `src/test-setup.ts` mocks `./lib/cli.js` globally with an explicit factory | confirmed, `src/test-setup.ts:5-8` — the D7 constraint | +| Its `afterEach` warns, and does not fail, on unexpected calls | confirmed, `src/test-setup.ts:19-31` — R3 | +| 16 assertions across 3 test files assert on the command string | confirmed, `git.test.ts` (23 mock refs), `github.test.ts` (7), `jira.test.ts` (2) | +| `process.env.PWD` is read exactly once in `src/` | confirmed, `git.ts:230` — deleted by Phase 3 | +| The relative worktree path is deliberate, with a comment saying why | confirmed, `git.ts:235-236` — R1 | +| The changelog page keeps no per-release notes | confirmed, `docs/src/app/docs/changelog/page.mdx` | + +### Corrections to the draft + +The draft's line numbers were correct when written and have since drifted. Four are restated here so the +phases are not read against stale citations: + +| Draft said | Actually | What it is | +|---|---|---| +| `git.ts:231` | `git.ts:237` | `const cdRoot = \`cd ${gitRootPath}\`` | +| `git.ts:239` | `git.ts:245` | `const gotoBack = \`cd ${currentPath}\`` | +| `git.ts:241` | `git.ts:247` | the four-command chained `cmd()` call | +| `git.ts:261-263` | `git.ts:266-270` | `gitNukeWorktreeCmd`'s three-command chain | + +The draft's own two corrections to the material it was given — `git.ts:94` → `:95`, and "every subprocess +call builds a shell string" being too broad — both still hold and are carried into §1 and the table above. diff --git a/context/roadmap.md b/context/roadmap.md index 97addeb..3c2244f 100644 --- a/context/roadmap.md +++ b/context/roadmap.md @@ -44,10 +44,10 @@ and the text scrolls out of sight instead of the field growing. - **Size:** small — one component (`ChatInput`) plus its ref type and submit key handling in `ChatForm` - **Doc:** none yet -### shell-argv-safety — `pending` +### shell-argv-safety — `active` `cmd()` runs every git call through a shell as an interpolated string, so a repo path containing a space fails today and a config value containing a quote or backtick is an injection vector. - **Size:** medium — one helper contract, 8 interpolating call sites, and the `exec` that bypasses it -- **Doc:** [`drafts/shell-argv-safety.md`](drafts/shell-argv-safety.md) — the full call-site inventory, a demonstrated failure, and why adding `cwd` removes most of it +- **Doc:** [`plans/SHELL-ARGV-SAFETY-PLAN.md`](plans/SHELL-ARGV-SAFETY-PLAN.md) — 6 phases, 4 open questions From acf077459f7adf0feee3f86aaa40a1e072519bb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Baldur=20P=C3=A1ll=20H=C3=B3lmgeirsson?= Date: Sat, 5 Sep 2026 18:35:46 +0000 Subject: [PATCH 11/31] feat(cli): add an argv-based run() alongside cmd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shell-argv-safety Phase 1. Adds `run(file, args, { cwd })` on top of `execFile`, so no value passed to a subprocess is parsed as shell syntax and `cwd` reaches the child directly instead of through a `cd` prefix. Nothing migrates in this phase. `cmd()` is untouched, all 18 of its call sites still use it, and no existing assertion changed — `run` is imported only by its own test. This is the additive half of D1: add alongside, migrate in batches, delete `cmd()` when the last caller is gone. D7 is a sequencing constraint, not a preference: the global `vi.mock` factory in `src/test-setup.ts` returns an explicit object, so an export missing from it is `undefined` at call time and a migrated call site would fail with "run is not a function" rather than a useful assertion. `run` goes in now, before the first caller. `src/lib/cli.test.ts` is new and covers the real helper — it opts out of that global mock with `vi.unmock`, and drives `process.execPath` so the suite needs no fixture binary. Two of its nine cases pin the reasons this work exists: a `cwd` whose path contains a space, and an argument carrying `"`, a backtick, `$(…)` and `;` that arrives intact while its embedded `touch` creates nothing. Gate 2 (reviewer subagent) returned PASS WITH NOTES with no blocking findings, and re-verified R5 by measurement rather than inference: `exec` and `execFile` both fail at 1048576 bytes with ERR_CHILD_PROCESS_STDIO_MAXBUFFER, so the buffer limit is genuinely unchanged. Its notes are recorded rather than left in the transcript: - F-006 (P2) — the `afterEach` guard watches `mockCmd` only, so R3's mitigation goes vacuous as call sites move to `run`. Tied to Phase 2, which migrates the first three, and named in that phase's scope so it cannot be missed. - F-007 (P3) — this suite's `afterAll` would mask a `beforeAll` failure. - R6 — `execFile` defaults to `shell: false` and cannot launch a Windows `.cmd`/`.bat` shim, which lands on Phase 6's `codeEditor`. Unverified from macOS and flagged as a question, not a defect. - R7 — two caller-observable error deltas: the message prefix shrinks at Phase 3, and `code` goes from numeric 127 to "ENOENT" for a missing file. Claude-Session: https://claude.ai/code/session_01Sjmawm7wirBuTSTAuKEx7g --- context/findings.md | 37 +++++++++ context/plans/SHELL-ARGV-SAFETY-PLAN.md | 39 ++++++++- src/lib/cli.test.ts | 101 ++++++++++++++++++++++++ src/lib/cli.ts | 26 +++++- src/test-setup.ts | 11 ++- 5 files changed, 206 insertions(+), 8 deletions(-) create mode 100644 src/lib/cli.test.ts diff --git a/context/findings.md b/context/findings.md index 8ad7e31..f09496a 100644 --- a/context/findings.md +++ b/context/findings.md @@ -120,6 +120,43 @@ this is a note, not a gap to close blindly. **Closes when:** a Lint gate run passes with `README.md:160` carrying the same uncommitted-work exception as `page.mdx:17`. +### F-006 — P2 — the unexpected-call guard watches `cmd` only, so it goes vacuous as call sites migrate + +**Tied to:** shell-argv-safety Phase 2 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) + +`src/test-setup.ts:23` builds its unexpected-call list from `mockCmd.mock.calls` alone. Phase 1 added +`mockRun` to the mock factory (`src/test-setup.ts:6,11,43`) but nothing watches it, so from Phase 2 onward +every call site that moves to `run` leaves that guard's field of view. The plan's R3 exists precisely +because this `afterEach` warns instead of failing, and §7 case 4 tells every phase to grep the test output +for `Unexpected cmd calls detected` — a grep that will keep coming back clean while covering steadily less. +The mitigation decays exactly as the migration proceeds. + +Not live in Phase 1: no call site migrated, so `mockRun` is never called and the guard's coverage is still +total. Left unfixed rather than folded into Phase 1 because the fix is not one targeted edit — +`expectedCommands` is a `string[]` and cannot hold a `(file, args, opts)` triple without a shape decision, +and that decision wants real call sites to validate it. Phase 2 migrates the first three. + +**Closes when:** a Gate 1 run passes with the `afterEach` in `src/test-setup.ts` reporting unexpected `run` +calls as well as `cmd` calls — or, if that shape is judged not worth having, when R3's mitigation and §7 +case 4 are explicitly retired in the plan, citing the run that made the call. + +### F-007 — P3 — `cli.test.ts`'s `afterAll` would mask a failure in its own `beforeAll` + +**Tied to:** shell-argv-safety Phase 1 · **Raised:** 2026-09-05 (Gate 2, reviewer subagent, Phase 1) + +`src/lib/cli.test.ts:35-37` calls `rmSync(tempPath, { recursive: true, force: true })` unconditionally. If +`mkdtempSync` at `:30` ever threw — a full or read-only temp filesystem — `tempPath` would still be +`undefined` and `rmSync` would throw `ERR_INVALID_ARG_TYPE` on top of the real error, so the reported +failure would name the cleanup rather than the cause. `force: true` does not help: it suppresses a missing +path, not an invalid argument type. + +Narrow, and it costs a one-line `if (tempPath)` guard. Left unfixed because Gate 2 had already returned +`PASS WITH NOTES` on this exact diff, and editing it afterwards would commit code no gate had seen — the +same reasoning F-002 through F-005 record. + +**Closes when:** a Gate 1 run passes with the `afterAll` in `src/lib/cli.test.ts` guarded against an +unset `tempPath`. + ## Closed None. Closed findings leave this file — a feature's at `/feature-close`, folded into the retiring plan's diff --git a/context/plans/SHELL-ARGV-SAFETY-PLAN.md b/context/plans/SHELL-ARGV-SAFETY-PLAN.md index 61883d5..d8acff4 100644 --- a/context/plans/SHELL-ARGV-SAFETY-PLAN.md +++ b/context/plans/SHELL-ARGV-SAFETY-PLAN.md @@ -198,7 +198,30 @@ this is a sequencing question, not a design one — see §8 Q1. **R5 — `execFile` has a default `maxBuffer`.** `exec` and `execFile` both default to 1 MB of stdout in current Node. `git worktree list` or `git --no-pager branch -r` in a very large repository could exceed it, and the failure mode is a rejected promise, not truncation. Unchanged from today — `exec` has the same -default — so this is not a regression, but it is now worth knowing. Not mitigated. +default — so this is not a regression, but it is now worth knowing. Not mitigated. **Re-verified at +Phase 1's Gate 2** by running both against a 1 MB stdout on Node 24: identical +`ERR_CHILD_PROCESS_STDIO_MAXBUFFER` at 1 048 576 bytes. The claim is measured, not inferred. + +**R6 — `execFile` cannot launch a Windows `.cmd` or `.bat` shim.** Found at Phase 1's Gate 2 and **not in +the original inventory.** `exec` always goes through a shell; `execFile` defaults to `shell: false`, and +since the Node 18.20/20.12 spawn hardening a `.cmd`/`.bat` shim on Windows needs `shell: true` to launch at +all. This barely touches git — `git.exe` is a real binary — but it lands squarely on **Phase 6**, where a +`codeEditor` of `code` is `code.cmd` on Windows, and on `commandExists` in Phase 5, which already branches +on `win32` (`src/lib/cli.ts:57`). **Unverified from macOS**: the restriction lives in libuv's Windows +`uv_spawn`, not in the JS layer, so this is a question Phase 6's design must answer rather than a +demonstrated defect. Do not close it by assertion. + +**R7 — two caller-observable error differences that §2 should acknowledge.** §4.1's "no caller's error +handling changes" is very slightly overstated, in two ways found by measurement at Phase 1's Gate 2: + +- **The message prefix shrinks.** `src/lib/git.ts:253` surfaces a rejection with + `spinner.fail(error.message)`. Both forms carry `Command failed: …` plus stderr, so nothing is lost, but + at **Phase 3** the prefix goes from `cd /x && git fetch && git worktree add …` to `git worktree add …`. + That is an improvement and still a user-visible text change. Phase 3 should state it deliberately rather + than let it happen. +- **`code` changes type when the file is missing.** `exec` rejects with a numeric `127` from the shell; + `execFile` rejects with the string `"ENOENT"`. Harmless today — `grep -rn 'error\.code|\.code ===' src/` + returns nothing, so no caller branches on it — but a future caller must not assume a number. ## 6. Phases @@ -206,8 +229,8 @@ default — so this is not a regression, but it is now worth knowing. Not mitiga | # | Phase | Status | Depends on | Note | |---|---|---|---|---| -| 1 | `run()` helper, `cwd` support, and its global mock | not started | — | | -| 2 | The three read-only `cd` sites | not started | 1 | | +| 1 | `run()` helper, `cwd` support, and its global mock | done | — | Gate 2 `PASS WITH NOTES`; R5 re-verified, not a regression | +| 2 | The three read-only `cd` sites | not started | 1 | Owns F-006 — see the scope note below | | 3 | `gitCreateWorktree`'s four-command chain | not started | 1 | | | 4 | Config get/set and `gitNukeWorktreeCmd` | not started | 1 | | | 5 | Static sites, `commandExists`, and deleting `cmd()` | not started | 2, 3, 4 | | @@ -242,8 +265,16 @@ and `gitGetUncommittedChangesCount` (`git.ts:103`) to `run("git", […], { cwd: `cd ${branchPath} &&` prefix is deleted, not quoted (D3). Move each function's assertion to the argv form (§4.4). +**Also in scope — F-006, and it is the reason this phase is where R3 lives or dies.** The `afterEach` +guard in `src/test-setup.ts` maps `mockCmd.mock.calls` only. These are the first call sites to move onto +`mockRun`, so from this phase on every migrated call is invisible to that guard and §7 case 4's grep goes +quietly vacuous — the opposite of what R3 asks for. Decide the shape here: either extend the guard to cover +`run` (`expectedCommands: string[]` cannot hold an argv triple unchanged, so this is a shape decision, not a +one-line edit) or retire R3's mitigation explicitly in this document. Do not leave it implicit. + **Done when:** none of the three functions' bodies contain the string `cd `; their tests assert -`("git", [...], { cwd })`; a `branchPath` containing a space produces a correct call (§7 case 1). +`("git", [...], { cwd })`; a `branchPath` containing a space produces a correct call (§7 case 1); F-006 is +either closed or consciously retired. #### Phase 3 — `gitCreateWorktree`'s four-command chain diff --git a/src/lib/cli.test.ts b/src/lib/cli.test.ts new file mode 100644 index 0000000..2a67e99 --- /dev/null +++ b/src/lib/cli.test.ts @@ -0,0 +1,101 @@ +import { + existsSync, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { run } from "./cli.js"; + +// src/test-setup.ts mocks ./lib/cli.js for every suite so command tests never +// execute anything. This file covers the real helper, so it opts back out. +vi.unmock("./cli.js"); + +// The node binary running this suite: always present, on every platform CI and +// contributors use, and reachable without a shell — which is the point here. +const node = process.execPath; + +// Print argv[1], which under `node -e