diff --git a/.specs/LESSONS.md b/.specs/LESSONS.md index 1c773bd..204f49b 100644 --- a/.specs/LESSONS.md +++ b/.specs/LESSONS.md @@ -14,6 +14,12 @@ Corroborated across multiple features. Safe to apply as guidance. - evidence: src/main/workflow-ctx.ts:82,106 (CtxDeps.agent / CtxRuntime.signal SPEC_DEVIATION) (workflow-ctx) (+1 more) - last seen: 2026-07-06T16:15:40Z +### L-005 — Before adding real-process or real-git tests, check whether existing suites already sit near the default per-test timeout: the extra parallel load alone can push them over it, turning a green gate red without any production change +- signal: `gate_fail` · recurrence: 2 feature(s) · scope: `testing` · harmful: 0 +- features: worktree-post-create-hook, worktree-removal-fault-tolerance +- evidence: validation.md round-2 gate section; tree.test.ts / worktree-manager.test.ts timeouts (testing) (+1 more) +- last seen: 2026-07-31T12:27:40Z + ## Candidates (under observation — do NOT load as guidance yet) Seen once or not yet corroborated. Tracked, not trusted. @@ -36,11 +42,17 @@ Seen once or not yet corroborated. Tracked, not trusted. - evidence: mutant R1/M7; post-create-hook.test.ts output-tail test (testing) - last seen: 2026-07-29T22:37:04Z -### L-005 — Before adding real-process or real-git tests, check whether existing suites already sit near the default per-test timeout: the extra parallel load alone can push them over it, turning a green gate red without any production change -- signal: `gate_fail` · recurrence: 1 feature(s) · scope: `testing` · harmful: 0 -- features: worktree-post-create-hook -- evidence: validation.md round-2 gate section; tree.test.ts / worktree-manager.test.ts timeouts (testing) -- last seen: 2026-07-29T22:37:06Z +### L-006 — Assert a returned payload field by its value, not by the value you handed an injected fake: a field that appears in the test only as a spy's input reads like coverage in review, but a mutation dropping it from the real return still passes +- signal: `surviving_mutant` · recurrence: 1 feature(s) · scope: `testing` · harmful: 0 +- features: worktree-removal-fault-tolerance +- evidence: round-1 mutant M6; worktree-manager.test.ts leftover: at :844/:867/:882 were spyDeleter inputs, not assertions - dropping the field from worktree-manager.ts:335-339 left all 80 tests green; closed by F1 124340c (testing) +- last seen: 2026-07-31T12:27:40Z + +### L-007 — When writing a test to kill a specific surviving mutant, check the fixture does not encode that mutant's own blind spot: pick one whose readings differ under every wrong implementation, not just the one you saw. A directories-only residue pinned the recursive count yet let a directories-only count survive +- signal: `surviving_mutant` · recurrence: 1 feature(s) · scope: `testing` · harmful: 0 +- features: worktree-removal-fault-tolerance +- evidence: round-2 mutant N3 survived the round-1 fix F2 (dir-remover.test.ts:328-348 fixture wt/keep/a/b was directories-only); closed by F3 1abe8aa with a mixed chain giving 3/2/1/1 for every-entry/dirs-only/files-only/top-level (testing) +- last seen: 2026-07-31T12:27:40Z ## Quarantined (failed when applied — ignore) diff --git a/.specs/STATE.md b/.specs/STATE.md index f04ce45..0e9891d 100644 --- a/.specs/STATE.md +++ b/.specs/STATE.md @@ -20,77 +20,76 @@ Handoff snapshot. | AD-009 | 2026-07-06 | **WF3 MERGED to `main` (PR #65).** Independent SDD eval (author≠judge, `spec-driven-eval`): **Final 0.98 — "Spec-complete"** (S=PASS, E recall/precision/justified ≈1.0, gates build/lint/unit green; live smoke owner-PASS 6/6). Two minor gaps merged as-is and **carried into WF4** (WF3-04 generic retry prompt; WF3-10 unasserted server reuse). **WF4 planning deferred to the next session.** | The two gaps are cheap polish on the same runner/`--resume` path WF4 already touches, so folding them into WF4 avoids a throwaway PR. Report: `.specs/features/workflows-agent-step/evaluations/P1-workflows-agent-step-20260706T141244Z.md`. | | AD-008 | 2026-07-03 | **WF3 (Structured agent step) scope pinned via 4 owner decisions:** (1) **Arm M (MCP) only** — one shared loopback HTTP MCP server, per-step bearer token = auth+routing, forced `emit_result`; Arm N (`--json-schema`) dropped. (2) **ajv** for payload validation (promotes `emit-result-schema` off the spike's minimal checker; `expect` stays a JSON Schema). (3) `ctx.agent()` returns the **full envelope** `{status,data?,question?,sessionId}`; `blocked` is returned **as-is** (no engine pause in WF3 — that's WF4). (4) Permission presets **read/write/bypass**, default **read** (read = read-only tools + `emit_result`, guaranteed non-mutating). | Findings recommended Arm M to keep the `blocked` terminal value + per-step routing first-class for WF4; ajv because the author declares a JSON Schema and the tool `inputSchema` is JSON Schema too; full-envelope return lets WF4 add the pause without breaking the happy path; the preset set is PRD-fixed (US 26). Spec: `.specs/features/workflows-agent-step/spec.md` (WF3-01..25). | | AD-013 | 2026-07-29 | **Worktree post-create hook (`worktree-post-create-hook`) scope pinned via 4 owner decisions + a decorator architecture:** (1) The command is declared **repo-locally** in a NEW `\.app\config.json` key `postCreateCommand` (mirrors the existing workspace-level `.app/config.json` reader one level down) — **not** in global settings, so it travels with the repo. (2) A failing hook **keeps the worktree**: `createWorktree` returns `ok:true` plus a `hook` failure payload (exit code + 4000-char output tail); no rollback. (3) The hook runs on **all three create paths** (New Worktree, Start Work, workflow `ctx.worktree.create`). (4) Feedback is **inline in the dialog on failure, silent on success**; the workflow run-timeline detail box is **P2/deferred**. **Architecture:** a `withPostCreateHook(create, deps)` **decorator** (Approach D) wraps `createWorktree` with an identical signature, wired **once** in `index.ts` and assigned to both the IPC handler and `ctxDeps.worktree.create` — so `worktree-manager.ts` (+ its ~40 real-git tests) and `workflow-ctx.ts` are **untouched**, and the run-iff-created rule (`ok && path`) is unit-testable against a fake create with no git and no spawn. The 120 s timeout's process kill stays in the hand-verified `index.ts` spawn seam; only its result *mapping* is unit-tested. | Repo-local won because the init script (`SetupSkills.cmd` in `m:\triade\source\Code`) is already checked in and resolves its own paths from `$PSScriptRoot` — the repo is what knows its init. Keeping the worktree matches the fact that `git worktree add` already succeeded; discarding a valid checkout (plus any base refresh / branch recut) over a fixable script error is the worse failure. All-three-paths because workflow-created worktrees for agents are the case that most needs the skills junctions. The decorator was chosen over a 7th positional param, a trailing options object, and a module-level setter because it is the only option that changes neither the real-git module nor the workflow ctx, and it avoids the parallel-test-hostile global state a setter would introduce. **Accepted trade-off, recorded not buried:** the command is repo content, so cloning an untrusted repo into a registered workspace means its `postCreateCommand` runs on the next create for that repo — no prompt, no allowlist in v1. Spec/design/tasks: `.specs/features/worktree-post-create-hook/` (WPC-01..24; 21 in the P1 slice, WPC-17..19 deferred). | +| AD-014 | 2026-07-30 | **Worktree removal is delete-first, project-wide.** The app deletes the worktree directory **itself** — `dir-remover.ts`'s `removeDirTree` (junction-safe, `maxRetries: 0` per attempt inside a 250 ms / 3000 ms deadline-bounded loop) — and only then calls `git worktree remove ` **purely to drop bookkeeping**. **No surface may use `git worktree remove --force` as a *deleter*** — not `WorktreeManager`, not `workflow-ctx`, not the deferred create-time cleanup. `force` keeps its FRWT meaning (**skip the dirty check only**) and never reaches git. The guard order is fixed at **primary → registered → locked → dirty → delete → bookkeeping**, and **every guard refuses before anything is deleted**; the registered check is also the anti-`rm -rf` guard and fails **closed** when git itself fails. **`git worktree lock` is checked by us**, from the porcelain `locked` line (a bare `locked` parses to `''`, which is still locked). **WRFT-07 (create-time leftover collision) is deferred to a follow-up PR** (owner decision at Tasks approval); this branch ships WRFT-01..06 plus the deleter and classification seams the follow-up lifts. | Two measured findings forced the inversion, one on each path. **(1) The success path destroyed data.** Git for Windows treats a directory junction as an ordinary directory and **recurses into it**, so `git worktree remove --force` emptied the shared *target* of AD-013's skills junctions and **reported success** — and because every hook-created worktree reads dirty (`?? .skills/`), the UI routed exactly those worktrees down the force path. Node's `fs.rm` lstats a junction as a link and **unlinks** it, leaving the target byte-identical (measured both ways). **(2) The failure path failed open.** Git deletes its bookkeeping even when the tree deletion fails — its own source comments *"continue on even if ret is non-zero, there's no going back from here"* — so one locked file left an **invisible orphan**: no `.git`, so `scanRepos` skips it, the row vanished on the next refresh, the folder later blocked recreating that worktree, and a retry answered `fatal: '' is not a working tree`. Delete-first inverts that failure mode: git is never invoked, the worktree stays **registered**, and the still-visible row *is* the retry handle — which is also why no pending-cleanup persistence was needed. The lock guard has to be ours precisely because git's own refusal would arrive **after** we had already deleted the tree. WRFT-07 was deferred as P2 that rides on this branch's seams rather than blocking it. Spec/design/tasks: `.specs/features/worktree-removal-fault-tolerance/` (WRFT-01..07). | ## Handoff -**Status (current, 2026-07-29):** **`worktree-post-create-hook` (AD-013) — EXECUTED + -VERIFIED (round 2 PASS) — NOT pushed, NO PR, visual/UAT pass OUTSTANDING.** Branch -`feature/worktree-post-create-hook`, 11 commits (`c846eb0..663e2d3`), **533 tests / 39 files -green**, typecheck + lint (18 pre-existing warnings) + `build` + `build:win` all clean. +**Status (current, 2026-07-30):** **`worktree-removal-fault-tolerance` (AD-014) — EXECUTED and +independently VERIFIED (round 3 PASS). NOT pushed, no PR, no GitHub issue yet; the owner's live smoke +run and visual pass are OUTSTANDING.** Branch `feature/worktree-removal-fault-tolerance`, 15 commits +(`16d2c2f..HEAD`), **566 tests / 40 files green**, typecheck clean, lint 0 errors / 18 pre-existing +warnings (unchanged count). WRFT-01..05 are **Verified**; WRFT-06 is **Unverified** (renderer — no +executed evidence); WRFT-07 is **Deferred** to a follow-up PR. -A repo declares `postCreateCommand` in its own NEW `\.app\config.json`; it runs with -cwd = the new worktree on **all three create paths**, via a `withPostCreateHook` decorator -wired once in `index.ts` and handed to both the IPC handler and `ctxDeps.worktree.create` -(so no caller can opt out). A failed hook keeps the worktree and reports exit code + a -4000-char output tail; the dialogs show an amber advisory. `worktree-manager.ts` and -`workflow-ctx.ts` were never touched. +Removal is now **delete-first**: the app deletes the worktree directory itself with a junction-safe, +deadline-bounded deleter and calls `git worktree remove` only to drop bookkeeping. A blocked deletion +returns before git runs, so the worktree stays **registered** and its row is the retry handle; the +Danger section names the blocked path and the remaining entry count. Guard order is primary → +registered → locked → dirty, all refusing before any deletion. `SessionManager.stop` now resolves on +the PTY's real exit (capped at 3000 ms), so removal no longer races the terminals it just killed. + +This also closed a **latent data-loss bug** found while probing: git for Windows treats a junction as +a directory and recurses into it, so `git worktree remove --force` was emptying the shared target of +AD-013's skills junctions **while reporting success**. Node's `fs.rm` unlinks junctions instead, so +delete-first fixes it as a side effect. Worth checking whether any real shared-skills folder was +already emptied by a past removal. **Commit map:** | Commit | Task | What | | ------ | ---- | ---- | -| c846eb0 | plan | spec (WPC-01..24) + design + tasks + AD-013 | -| 7732a89 | T1 | `repo-config.ts` — repo-local `postCreateCommand` reader (+10) | -| 4859446 | T2 | `post-create-hook.ts` — `runPostCreateHook` env/tail/timeout mapping + shared types (+12) | -| bce57f4 | T3 | `withPostCreateHook` run-iff-created decorator (+10) | -| dd3eeab | T4 | `index.ts` spawn seam + single wiring point | -| 0ce6177 | T5 | `HookFailureNotice` component + CSS | -| cd95f5a | T6 | both create dialogs surface hook failure | -| 0291a70 | F1 | **Verifier blocker** — shell settled on `close` only; extracted to `hook-shell.ts`, settles on `close` OR `exit`+grace (+7) | -| adff2bb | F2/F3 | pinned the 4000 literal (surviving mutant); real-git end-to-end for WPC-03's on-disk half (+3) | -| 98034eb | F4 | backdrop dismissal skipped the tree refresh | -| 663e2d3 | F5 | grace timer un-`unref`'d (paths were not independent); real-seam stderr + large-burst tests; shortened lingering pings (+2) | - -**Verifier (independent, author ≠ verifier) — round 1 FAIL → round 2 PASS, 4/4 findings -closed.** Round 1 caught a genuine blocker: resolving on `close` waits for stdio EOF, and -`spawn`'s timeout kills only `cmd.exe`, so a surviving grandchild held the pipes — measured -`exit` 1665 ms vs `close` 12969 ms, and 21000 ms with a detached grandchild. A hung script -would never settle: `worktrees:create` never resolved, dialog stuck on `busy`. Round 2 -re-probed the fixed seam with real processes: `pause` +152 ms, infinite loop +94 ms, -pipe-holding child **+119 ms**, detached grandchild **+467 ms**; output verified complete to a -1 MB single burst. Report: `.specs/features/worktree-post-create-hook/validation.md`. - -**Accepted mutation survivors (reasoned, not oversights):** `HOOK_FLUSH_GRACE_MS 250→0` and -removing the `close` handler are **equivalent mutants** — queued `data` events drain before the -timer callback either way, so the only observable difference is latency, and asserting -sub-250 ms latency on this contended box would be flaky. Verified empirically both ways. - -**NEXT STEP (nothing else outstanding in code):** owner **visual/UAT pass** — WPC-12..16 are -marked `Built †` in the spec, not Verified: the renderer has no unit tests by convention and -the dialogs have never been rendered. Then push + PR with `Closes #` once the feature -issue exists (the repo's issue = feature = PR pipeline). **Live gate to run:** create a worktree -for `m: riade\source\Code` with `.app\config.json` → `{"postCreateCommand": ".\SetupSkills.cmd"}` -and confirm `.claude\skills` + `.codex\skills` junctions appear; then the same via a workflow. +| 16d2c2f | plan | spec (WRFT-01..07) + design + tasks | +| 34f8970 | T0 | gate stabilization — `testTimeout`/`hookTimeout` 30000, one racing fixture window widened (added during Execute after two runs of untouched `main` came back red: `2 failed`, then `14 failed`) | +| b286a46 | T1 | `dir-remover.ts` — `removeDirTree` + `DELETE_RETRY_INTERVAL_MS`/`DELETE_RETRY_BUDGET_MS`, DI'd fs deps (+9) | +| bdc32fe | T2 | real-fs hazard tests — junction target survives, dangling junction, read-only + nested repo, real external-holder lock (+6) | +| 32eb539 | T3 | porcelain `locked` parsing — reason / `''` / `undefined` are distinguishable (+3) | +| dd7f31c | T4 | `removeWorktree` reordered to delete-then-deregister, 6-step guard table, deleter injected (+10) | +| f8a4af8 | T5 | `leftover` through `shared/worktrees.ts` → IPC → `WorktreeDetail` (producer + consumer together, L-001) | +| b090c6f | T6 | `SessionManager.stop` awaits the real PTY exit, capped at `SESSION_EXIT_WAIT_MS = 3000`; `killAll` stays fire-and-forget (+3) | +| ac71cfb | T7 | `smoke-remove.mjs` + seed extended with the WRFT-06 blocked-then-retry flow (**written, never run**) | +| dcc50dc | T8 | AD-014 + spec traceability + handoff | +| 124340c | F1 | **Verifier r1 gap** — assert `RemoveWorktreeResult.leftover` by value (it was only ever a spy *input*) | +| 5aafb90 | F2 | **Verifier r1 gap** — pin the recursive `remaining` count against real fs | +| 6f3af8a | — | spec precision: `remaining` is the **recursive** count; `leftover` is part of the returned contract; guard refusals carry none | +| 1abe8aa | F3 | **Verifier r2 gap** — mixed file+directory residue fixture (one locked chain, `pwsh` `FileShare.None` holder) so 3/2/1/1 separates four readings | +| 45c27d5 | F4 | **Verifier r2 gap** — `leftover` absence asserted on all four guard refusal paths, not just `locked` | -**Deferred (spec Out of Scope):** WPC-17..19 — the workflow run-timeline hook detail box (needs a -new `StepDetail` variant + a `RunDetail` branch); `result.hook` is already reachable by an author. -Also: multiple/ordered commands, other lifecycle hooks, Settings UI, trust prompt/allowlist, -process-tree kill, in-app re-run. +**Verification (independent, author ≠ verifier) — 3 rounds, ending PASS.** Round 1 FAIL (14/16 mutants +killed): `leftover` never asserted, recursive count unpinned. Round 2 FAIL (8/10): the round-1 fix's +fixture was directories-only and therefore blind to *what* it counted, and guard-refusal absence was +pinned on one guard of four. Round 3 **PASS** (12/15; 3 survivors, all non-blocking and recorded). +**Every fix was test-only** — `git diff --name-only dcc50dc..HEAD` touches no production file. +Report: `.specs/features/worktree-removal-fault-tolerance/validation.md`. -**Two environment findings (NOT code issues), worth acting on separately:** -1. **`npm test` is unreliable on this machine.** Real-git tests in `tree.test.ts` / - `worktree-manager.test.ts` intermittently exceed their **5000 ms default** timeout under load - (observed 5.1 / 6.1 / 8.4 / 44 s), then cascade to `EPERM` in `afterEach` because the - timed-out git child still holds the temp dir. The failing subset differs per run and both - files pass 71/71 in isolation. `--maxWorkers=2` is reliable AND faster (81–125 s vs 300 s) — - vitest oversubscribes this box. **Recommend a `testTimeout` bump and/or `maxWorkers` in - `vitest.config.ts`** (deliberately not changed here — out of feature scope). -2. **`NoDefaultCurrentDirectoryInExePath=1`** in the agent session env makes a bare - `SetupSkills.cmd` fail with code 1; it is not a persistent User/Machine variable. Harness - artifact, not a product bug — but it's why the README example uses `.\SetupSkills.cmd`. - (Same variable already noted for node-gyp builds.) +**Accepted non-blocking survivors (reasoned, not oversights):** a guard `leftover` conditioned on +`force: true` (contrived; every guard is pinned on its non-force path); `blockedPath` naming the first +rather than the last failing attempt (the spec leaves it open and a discriminating fixture needs two +holders releasing mid-loop — racy); and the two guard message literals, whose behavior is pinned while +their wording is not (the Verifier recommends **not** fixing this). -**Prior context:** Workflows epic (#56) DONE + CLOSED; WF1–WF5 + hifi merged (PR #67). Baseline -before this feature: 489 tests / 36 files on `main`. Pre-existing quirk: -`src/main/ado-gateway.ts` is UTF-16 (git treats it as binary). Open follow-ups: 3 transitive dev -advisories (esbuild/form-data/undici); App.tsx `useTasks`/`useConfig` extraction deferred -(AD-004). +**OUTSTANDING — owner tasks, in order:** +1. **Live smoke** (discharges WRFT-06): `node scripts/seed-smoke-remove.mjs`, then + `npm run dev -- -- --remote-debugging-port=9222`, then `node scripts/smoke-remove.mjs`. One-shot — + re-seed before each run. Note a blocked deletion still removes everything it can reach, so the retry + click may face either a direct remove or the confirm dialog; the script handles both. +2. **Visual pass** on the Danger section (WRFT-06 AC 4 — long-path wrapping; that markup has never + been rendered). +3. **Create the GitHub issue** for this feature (issue = feature = PR), then push and open the PR with + `Closes #` in the body. +4. **Lessons store has no writer — entries below were HAND-MAINTAINED.** `.specs/LESSONS.md` declares + itself machine-owned by `scripts/lessons.py`, which does not exist in this repo. With the owner's + approval `lessons.json` was edited directly and `LESSONS.md` re-rendered by hand in the script's + exact format: **L-005 promoted to `confirmed`** (recurred on a second feature, meeting + `promote_threshold=2`), and **L-006** (*payload asserted as fixture input only*) and **L-007** + (*a fixture shaped around the known mutation*) added as candidates. If `lessons.py` is ever + restored, verify its rendering still matches these blocks byte-for-byte. +5. **Follow-up PR** for WRFT-07 (T9–T11 are specified verbatim in `tasks.md`). diff --git a/.specs/features/worktree-removal-fault-tolerance/design.md b/.specs/features/worktree-removal-fault-tolerance/design.md new file mode 100644 index 0000000..3cccf45 --- /dev/null +++ b/.specs/features/worktree-removal-fault-tolerance/design.md @@ -0,0 +1,304 @@ +# Worktree Removal Fault Tolerance Design + +**Spec**: `.specs/features/worktree-removal-fault-tolerance/spec.md` +**Status**: Draft +**Owner decisions this design implements**: delete-first ordering; bounded auto-retry then inline leftover +report; terminate-and-await app-owned sessions; create-time collision as P2 — plus the three architecture +choices confirmed during Design (reorder inside `removeWorktree` + `dir-remover.ts`; awaitable +`SessionManager.stop`; a separate `worktrees:clean-path` channel keyed by repo+branch). + +**Active decisions checked (`.specs/STATE.md`)**: AD-005 (Windows-only, Windows-path assertions in tests — +conformed: new tests keep backslash/`realpathSync.native` discipline), AD-013 (`withPostCreateHook` +decorator; explicit refusal to widen `createWorktree`'s signature — conformed: the P2 cleanup is a separate +channel, not a 7th parameter; `worktree-manager.ts` gains no hook coupling), AD-004/AD-011 (renderer units +are not tested by convention — conformed: WRFT-06 is smoke + hand-verified). No decision is superseded. +**Confirmed lesson applied**: L-001 — `RemovalLeftover` crosses `shared` → main → renderer and is wired +producer-and-consumer in a single phase (see Tasks phasing note). + +--- + +## Architecture Overview + +One invariant drives the whole design: **git is never the deleter**. The app deletes the worktree tree +itself, and only calls `git worktree remove` once the directory is verifiably gone. Every failure therefore +lands on the safe side of the state machine — the worktree stays registered, visible and retryable. + +```mermaid +graph TD + A[WorktreeDetail: Remove] --> B{dirty or running agents?} + B -- no --> D[worktrees:remove] + B -- yes --> C[RemoveWorktreeConfirm] + C --> S[sessions:stop each] + S -- awaits real PTY exit, cap 3000ms --> D + D --> G1[guards: primary / registered / locked / dirty] + G1 -- refuse --> R1[ok:false, nothing deleted] + G1 -- pass --> RM[dir-remover.removeDirTree] + RM -- ok --> GIT[git worktree remove: bookkeeping only] + RM -- give up --> R2["ok:false + leftover{blockedPath, remaining}
git NEVER invoked, worktree stays registered"] + GIT -- ok --> R3[ok:true: folder gone AND entry gone] + GIT -- fail --> R4[ok:false: dir gone, entry stays, retry self-heals] +``` + +The removal state machine, and the one state this feature makes unreachable: + +```mermaid +stateDiagram-v2 + [*] --> Registered_Present + Registered_Present --> Registered_Absent: removeDirTree ok + Registered_Present --> Registered_Present: removeDirTree gave up (retryable) + Registered_Absent --> Unregistered_Absent: git worktree remove + Registered_Absent --> Registered_Absent: git failed (retry heals) + Unregistered_Present: Unregistered_Present
(today's orphan — invisible, unrecoverable) + note right of Unregistered_Present + UNREACHABLE by construction: + git is only called once the + directory is already gone. + end note +``` + +--- + +## Code Reuse Analysis + +### Existing components to leverage + +| Component | Location | How to use | +| --- | --- | --- | +| `removeWorktree` guards + `RemoveWorktreeResult` discipline | `src/main/worktree-manager.ts:263-289` | Extend in place: same signature, same messages, new ordering + two new guards | +| `parsePorcelainBlocks` | `worktree-manager.ts:316-339` | Extend the block type with `locked`; already consumed by `listWorktrees` and `worktreeHosting` | +| `samePath` | `worktree-manager.ts:292-295` | Reuse verbatim for the registered-worktree match (case/separator-insensitive) | +| `gitFailureLine` | `worktree-manager.ts:298-303` | Reuse for the bookkeeping-step failure message | +| `statusOf` | `worktree-manager.ts:341-351` | Unchanged; still the dirty pre-check | +| `worktreePathFor` | `src/shared/worktrees.ts:45-50` | The clean-path channel recomputes the target from repo+branch+template instead of trusting a path | +| DI-with-defaults convention | `SessionManagerDeps`, `withPostCreateHook(create, deps)` | Same shape for the injected deleter | +| `BranchExistsChoice` + dialog conflict state | `NewWorktreeDialog.tsx:44,113`, `StartWorkDialog.tsx:55,130` | Same pattern for the new `path-exists` choice | +| Real-temp-git-repo test fixtures | `worktree-manager.test.ts:30-46` | Reuse `realpathSync.native(mkdtempSync(...))` setup for the new guard/junction/lock tests | +| `RemoveWorktreeConfirm` | `src/renderer/src/components/` | Unchanged — the confirm dialog's contract does not move | + +### Integration points + +| System | Integration | +| --- | --- | +| IPC contract | `worktrees:remove` res gains `leftover`; new `worktrees:clean-path`; `worktrees:create` res gains `path-exists` conflict | +| `workflow-ctx` `worktree.remove` | Signature unchanged — the workflow path inherits the fix for free, no rewiring | +| `withPostCreateHook` | Untouched; the P2 cleanup runs *before* create, so the hook still fires exactly once on the real create | +| `SessionManager` | `stop` becomes awaitable; `sessions:stop` handler awaits it; renderer unchanged | + +--- + +## Components + +### `dir-remover.ts` (new) + +- **Purpose**: Delete a directory tree safely and bounded — junction-safe, retrying only lock-type errors, reporting what is left when it gives up. +- **Location**: `src/main/dir-remover.ts` +- **Interfaces**: + - `removeDirTree(path: string, deps?: DirRemoverDeps): Promise` + - `export const DELETE_RETRY_INTERVAL_MS = 250` + - `export const DELETE_RETRY_BUDGET_MS = 3000` +- **Dependencies**: `node:fs/promises` (`rm`, `readdir`), `node:fs` (`existsSync`) — all injectable via `DirRemoverDeps` for unit tests +- **Reuses**: nothing — deliberately standalone and pure enough to unit-test without git or Electron + +Algorithm (each numbered step maps to an AC): + +``` +if (!exists(path)) return { ok: true } // WRFT-02 AC 4 +start = now() +loop: + try { await rm(path, { recursive: true, force: true, maxRetries: 0 }) ; return { ok: true } } + catch (err): + if (!RETRYABLE.has(err.code)) return fail(err) // WRFT-04 AC 4 — immediate + if (now() - start >= DELETE_RETRY_BUDGET_MS) return fail(err) // WRFT-04 AC 3 + await sleep(DELETE_RETRY_INTERVAL_MS) // WRFT-04 AC 1 + +fail(err) = { ok: false, code: err.code, leftover: { blockedPath: err.path ?? path, + remaining: countEntries(path) } } +RETRYABLE = { EBUSY, EPERM, ENOTEMPTY, EACCES } +``` + +`maxRetries: 0` is load-bearing, not a default: Node retries at every level of the recursive walk, so +`maxRetries: 5` measured **21 599 ms** against a locked directory (spec finding F). The comment in the code +must say so, or a future reader will "helpfully" raise it. + +### `worktree-manager.ts` (modified) + +- **Purpose**: Own the removal ordering and every guard that must run before a byte is deleted. +- **Interfaces**: + - `removeWorktree(repoPath, worktreePath, opts?: { force?: boolean }, deps?: WorktreeRemoveDeps): Promise` — 3-arg call sites unchanged; `deps` defaults to `{ removeDirTree }` + - `cleanWorktreePath(repoPath, branch, worktreeTemplate?, deps?): Promise` (P2) + - `classifyTargetPath(target: string): Promise` (P2, internal + tested) + - `parsePorcelainBlocks` → `PorcelainBlock` gains `locked?: string` +- **Reuses**: `samePath`, `gitFailureLine`, `statusOf`, `parsePorcelainBlocks`, `worktreePathFor` + +New `removeWorktree` flow — the guard order is the contract: + +| # | Step | Failure behavior | AC | +| --- | --- | --- | --- | +| 1 | `samePath(repoPath, worktreePath)` → primary refusal | unchanged message, nothing deleted | WRFT-01 AC 4 | +| 2 | `git worktree list --porcelain`, match by `samePath` | git failure ⇒ **fail closed** (refuse, delete nothing) | WRFT-01 AC 2 | +| 3 | matched block has a `locked` line | refuse with git's lock reason | WRFT-01 AC 3 | +| 4 | `!force` → `statusOf` dirty check | unchanged message | WRFT-01 AC 5 | +| 5 | `deps.removeDirTree(worktreePath)` | **return before touching git**, carry `leftover` | WRFT-02 AC 1, WRFT-04 | +| 6 | `git worktree remove ` (bookkeeping) | `gitFailureLine`; retry self-heals | WRFT-02 AC 3 | + +Steps 1-4 all run before step 5, so no guard can be bypassed by the new ordering — the reason the locked +check is mandatory here is that git's own lock refusal (which needs `-f -f` to override) would otherwise +arrive *after* we deleted the files. + +### `session-manager.ts` (modified) + +- **Purpose**: Make "the agent is stopped" mean "its PTY has actually exited". +- **Interfaces**: `stop(id: string): Promise` (was `void`); `export const SESSION_EXIT_WAIT_MS = 3000` +- **Mechanics**: `#start` stores an `exited` promise on the `RunningSession`, resolved from the existing + `handle.onExit` callback. `stop()` captures it, kills, calls `#finalize` **immediately** (status flips to + stopped exactly as today), then awaits `exited` raced against a 3000 ms timer. The timer is cleared in a + `finally` and `unref`'d so a fake PTY that never exits cannot hold the event loop open in tests. +- **Unchanged on purpose**: `killAll()` stays fire-and-forget (`void this.stop(id)`). Awaiting it would add + up to 3 s per session to app quit — this **corrects the "bonus" I claimed when presenting the options**: + the quit path gets no new guarantee, only the removal path does. +- **Reuses**: the existing idempotent `#finalize` (its own comment already covers being called twice). + +### Renderer: `WorktreeDetail.tsx` + `WorktreeDetail.css` (modified) + +- **Purpose**: Show *what* is blocking, and keep the retry one click away. +- **Changes**: `removeError: string | null` gains a sibling `removeLeftover: RemovalLeftover | null`, both + set from the failed result and cleared on every new attempt. Below the existing + `.detail-danger-note.error` line, render the blocked path in monospace with `word-break: break-all` + (WRFT-06 AC 4) and the `N item(s) still on disk` count. +- **Already correct, deliberately unchanged**: `setRemoving(false)` on the failure branch already re-enables + the button (WRFT-06 AC 3, `WorktreeDetail.tsx:128`); the session-stop failure path + (`WorktreeDetail.tsx:173-179`) is untouched (WRFT-05 AC 4); `onRemoved` refresh/reselect is untouched. + +### P2 surfaces + +- **`cleanWorktreePath`** (main): recomputes the target with `worktreePathFor(repoPath, branch, template)`, + re-runs `classifyTargetPath`, refuses unless the state is `leftover`, then calls `removeDirTree`. + A raw path is never accepted over IPC — the handler derives it, so no caller can aim the recursive + deleter at an arbitrary directory. +- **`LeftoverPathChoice.tsx`** (new, ~40 LOC): mirrors `BranchExistsChoice` — states the path and entry + count, offers "Delete folder & create" (danger) and Cancel. +- **`NewWorktreeDialog` / `StartWorkDialog`**: widen the existing `conflict` state to + `'branch-exists' | 'path-exists'`, render the new choice, and on confirm call `worktrees:clean-path` + then re-invoke `worktrees:create` unchanged. + +--- + +## Data Models + +```typescript +// src/shared/worktrees.ts +/** What a failed deletion left behind (WRFT-04). */ +export interface RemovalLeftover { + /** The first path the deleter could not remove (absolute). */ + blockedPath: string + /** Entries still present under the worktree root after the failed attempt. */ + remaining: number +} + +export interface RemoveWorktreeResult { + ok: boolean + error?: string + /** Present only when a deletion attempt gave up; the worktree is still registered. */ + leftover?: RemovalLeftover +} + +export interface CreateWorktreeResult { + // …unchanged fields… + conflict?: 'branch-exists' | 'path-exists' + /** Set with conflict:'path-exists' — what is sitting at the target (WRFT-07 AC 1). */ + pathConflict?: { path: string; entries: number } +} + +// src/main/dir-remover.ts +export interface DirRemovalResult { + ok: boolean + /** Node error code of the last failing attempt (EBUSY, EPERM, …). */ + code?: string + leftover?: RemovalLeftover +} + +// P2 classification — the only state that may be auto-deleted is 'leftover' +export type TargetPathState = + | { kind: 'free' } // does not exist + | { kind: 'empty' } // exists, no entries → git accepts it + | { kind: 'leftover'; entries: number } // non-empty, no .git → cleanup offerable + | { kind: 'occupied' } // contains .git (repo or registered worktree) +``` + +IPC additions: + +```typescript +'worktrees:remove': { req: { repoPath, worktreePath, force? }; res: RemoveWorktreeResult } // res widened +'worktrees:clean-path': { req: { repoPath: string; branch: string; worktreeTemplate?: string } + res: RemoveWorktreeResult } // new +``` + +--- + +## Error Handling Strategy + +| Scenario | Handling | User impact | +| --- | --- | --- | +| Lock released within 3000 ms | Retry loop absorbs it | Nothing — removal just succeeds (~800 ms measured) | +| Lock persists past budget | `ok:false` + `leftover`; git never called | Inline error names the blocked path + count; row stays; retry works | +| Non-lock fs error (e.g. `EINVAL`) | Reported immediately, budget untouched | Same surface, no 3 s stall | +| Worktree `git worktree lock`ed | Refused at guard 3 with git's reason | "unlock it first" — nothing deleted | +| Path not a registered worktree | Refused at guard 2 | Nothing deleted (this is also the anti-`rm -rf` guard) | +| `git worktree list` itself fails | Fail closed: refuse, delete nothing | git's first stderr line inline | +| Deletion ok, bookkeeping fails | `ok:false` + git's line; dir gone, entry stays | Retry succeeds (git accepts removing a worktree whose dir is missing) | +| Directory already absent | Deletion no-ops, bookkeeping still runs | `ok:true` | +| Session never exits within 3000 ms | Proceed anyway; retry + leftover cover it | Worst case the normal blocked-path error | +| P2 cleanup deletion fails | Create aborts, leftover reported | Dialog shows the blocked path; no worktree created | +| P2 target holds a `.git` | Refused, cleanup **not** offered | "path already contains a repository or worktree" | + +--- + +## Risks & Concerns + +| Concern | Location | Impact | Mitigation | +| --- | --- | --- | --- | +| **Existing test asserts the behavior WRFT-07 changes** — and its fixture is an *empty* dir, which under AC 5 must now proceed | `worktree-manager.test.ts:428-438` | The "path guard wins over branch check" ordering assertion would silently change meaning | Intentional: update the fixture to a **non-empty** leftover dir to preserve the ordering assertion, and add a separate test pinning empty-dir passthrough. Called out in Tasks as an expected test edit, not an unexplained diff | +| Every removal now runs an extra `git worktree list --porcelain` | `worktree-manager.ts` step 2 | ~30 ms per removal; a git failure could block a legitimate removal | Accepted for the guard it buys; fails closed by design (refusing is the safe direction) | +| `stop()` becoming async with 7 existing sync call sites | `session-manager.test.ts:141,150,181,194,293,330,338`, `killAll` | Floating promises; a pending 3 s timer could outlive a test | Immediate `#finalize` keeps every existing assertion valid; timer cleared in `finally` and `unref`'d; `killAll` explicitly `void`s | +| Confirm dialog now waits for real exits before deleting | `WorktreeDetail.tsx:168` | Up to ~3 s of apparent hang on a stuck agent (parallel, so not per-session) | Button already disabled while removing; wait is capped and then proceeds | +| `remaining` count walks the tree on failure | `dir-remover.ts` | A `node_modules`-sized leftover costs a recursive `readdir` (~100-300 ms) | Failure path only, and it is what makes the message actionable. Not capped — a truncated count would mislead | +| Junction safety depends on Node's `fs.rm` treating junctions as links | `dir-remover.ts` | A Node behavior change would silently reintroduce data loss | Pinned by a real-fs test asserting the **target's** contents (spec finding D fixture) — the assertion that fails against today's implementation | +| Timer `unref` here vs candidate lesson L-003 | `session-manager.ts` | L-003's sibling fix (663e2d3) was caused by an `unref`'d grace timer skipping work | Different shape: this timer only races an awaited promise for a live caller, so `unref` cannot skip work — it only prevents holding the loop open. Noted so Execute does not "fix" it either way blindly | +| `path-exists` carries both `conflict` and `error`; `branch-exists` carries only `conflict` | `shared/worktrees.ts` | Mild asymmetry in the result contract | Deliberate: non-interactive callers (`ctx.worktree.create`) otherwise get `ok:false` with no message. `branch-exists` is left untouched rather than widening this feature's blast radius | +| Renderer has no unit tests (project convention AD-004/AD-011) | `WorktreeDetail.tsx` | WRFT-06 is not machine-verified | CDP smoke extension with a real external lock holder + hand-verified visual pass | +| Long paths (>260 chars) unprobed | `dir-remover.ts` | Unknown `fs.rm` behavior | Assumption logged in the spec; failure mode is a *named leftover*, never a silent orphan — safe either way | + +--- + +## Tech Decisions + +| Decision | Choice | Rationale | +| --- | --- | --- | +| Deleter injection | 4th `deps` param on `removeWorktree`, defaulted to the real `removeDirTree` | Matches the project's DI-with-defaults convention (`SessionManagerDeps`, `withPostCreateHook`); all 55 existing real-git tests keep their 3-arg calls; retry-policy tests get a deterministic fake without `vi.mock` | +| Per-attempt retry setting | `maxRetries: 0`, own outer loop | Measured: Node's ladder costs 21.6 s vs 786 ms for a self-managed loop (spec finding F) | +| Bookkeeping command | plain `git worktree remove ` | Verified exit 0 once the directory is gone. `--force` is unnecessary (nothing left to protect) and `prune` is repo-wide, so it could clear unrelated stale entries | +| Guard order | primary → registered → locked → dirty → delete | Cheapest/most-certain refusals first; every guard precedes deletion, which is the whole point of the reorder | +| Retry clock in tests | Vitest fake timers with the **real** constants, plus one test pinning the literals `250`/`3000` | Candidate lesson L-004: overriding the constants in tests would let a mutation of the constants survive | +| `stop()` semantics | Immediate finalize + awaited real exit | Keeps the instant UI status flip (and every existing test) while making the promise mean what the caller assumes | +| Clean-path addressing | `{repoPath, branch, worktreeTemplate}`, target recomputed main-side | Honors AD-013's refusal to widen `createWorktree`, and structurally prevents an arbitrary path reaching a recursive delete | +| Junction handling | Rely on `fs.rm`'s native symlink/junction semantics | Measured safe (spec finding D); hand-rolled reparse-point logic would be strictly more code and more risk | + +> **Project-level decision:** the delete-first invariant is a convention future features must follow (no +> future cleanup surface may use `git worktree remove --force` as a *deleter*). To be recorded as **AD-014** +> in `.specs/STATE.md` with the Tasks commit. + +--- + +## Test Strategy + +| Layer | What | Where | +| --- | --- | --- | +| Unit, fake deps | Retry cadence, budget exhaustion, non-retryable immediate fail, leftover payload, already-absent no-op | `dir-remover.test.ts` (new) — fake `rm` + fake timers, no fs | +| Unit, real fs | Junction target survives; read-only files and a nested repo's `0444` object store delete | `dir-remover.test.ts` (new) — explicit generous timeouts (L-005) | +| Unit, real git | Guard order (primary/registered/locked/dirty), delete-first ordering, bookkeeping-failure retry, already-absent path, P2 classification | `worktree-manager.test.ts` (extend) | +| Integration, real lock | One test: external holder (`spawn(process.execPath, ['-e','setTimeout…'], { cwd: })`) → assert `ok:false`, `leftover`, **and that `git worktree list` still lists the worktree**; release, retry, assert fully clean | `worktree-manager.test.ts` (extend), explicit timeout | +| Unit | `stop()` resolves only after the fake PTY's exit fires; resolves anyway after 3000 ms for a port that never exits | `session-manager.test.ts` (extend) | +| Smoke (CDP) | Blocked remove shows the path and keeps the row; retry after release removes it | `scripts/smoke-remove.mjs` (extend) | + +Baseline to preserve: **533 tests / 39 files green**, no deletions; the one intentional edit is the +`worktree-manager.test.ts:428` fixture described in Risks. diff --git a/.specs/features/worktree-removal-fault-tolerance/spec.md b/.specs/features/worktree-removal-fault-tolerance/spec.md new file mode 100644 index 0000000..3d42395 --- /dev/null +++ b/.specs/features/worktree-removal-fault-tolerance/spec.md @@ -0,0 +1,460 @@ +# Worktree Removal Fault Tolerance Specification + +**Milestone:** Post-v2 hardening (extends M2 _Delete Worktree (guarded)_ and _Force-Remove Worktree_) +**Sources of truth:** this conversation (user report + 4 owner decisions), measured probes against +git 2.49.0.windows.1 / Node 24.9.0 (§Verified behavior below), `delete-worktree/spec.md` (DLWT-01..04), +`force-remove-worktree/spec.md` (FRWT-01..04), `worktree-manager.ts` (`removeWorktree`), `repo-scanner.ts` +(why an orphan is invisible), `session-manager.ts` (`stop` does not await exit), AD-013 (the post-create +hook that creates the skills junctions this feature must stop destroying) +**Scope size:** Large — full spec + requirement IDs; `design.md` + `tasks.md` to follow +**Lessons applied:** L-001 (confirmed) — the new `leftover` field crosses main→shared→renderer; wire +producer and consumer in one phase rather than relaxing it to optional to keep an interim typecheck green + +## Problem Statement + +`git worktree remove` is not atomic, and on Windows it fails open. When any process holds a file inside +the worktree, git deletes **part** of the tree, fails, and then deletes its bookkeeping **anyway** — its +own source comments this as _"continue on even if ret is non-zero, there's no going back from here."_ +The worktree disappears from `git worktree list` while its files stay on disk. Because git also deletes +the worktree's `.git` file, the leftover folder has no `.git` at all, so `scanRepos` (`repo-scanner.ts:30-33`) +skips it and **the app cannot see it**: the user is shown "removal failed", the row vanishes on the next +refresh anyway, and a folder leaks silently — later blocking any attempt to recreate that same worktree +(`fatal: '' already exists`). Retrying is impossible, because git now answers `fatal: '' is not +a working tree`. + +The same investigation surfaced a second, worse defect on the **success** path. AD-013's post-create hook +creates skills **junctions** inside worktrees. Git for Windows treats a junction as an ordinary directory, +so `git worktree remove --force` **recurses into it and deletes the shared target's contents** — and +reports success. Every hook-created worktree is dirty (`?? .skills/`), so today's UI routes exactly those +worktrees to the force path. + +This feature inverts the order: **the app deletes the directory itself (junction-safe, with a bounded +retry), and only then asks git to drop the bookkeeping.** A deletion that cannot complete therefore leaves +the worktree fully registered — visible, retryable, self-healing — instead of an invisible orphan. + +## Goals + +- [ ] A worktree is **never** deregistered from git while its files are still on disk — the app's core + removal invariant +- [ ] A removal blocked by a locked file reports **which path is blocking and how many entries remain**, + keeps the row in the tree, and succeeds on a plain retry once the holder is gone +- [ ] Removal **never touches data outside the worktree** — junction/symlink targets survive intact, + closing the AD-013 skills data-loss path +- [ ] Transient locks (a just-terminated agent still releasing handles) resolve **automatically** within a + bounded retry, without the user clicking anything +- [ ] Terminating a worktree's agent sessions actually **waits for the processes to exit** before deletion + starts, instead of racing them +- [ ] Creating a worktree over a leftover folder offers a **clean-and-continue** path instead of git's raw + `fatal: already exists` (P2) +- [ ] Guards are preserved end-to-end: primary checkout, `git worktree lock`, and the dirty/force rules all + refuse **before** anything is deleted + +## Out of Scope + +| Feature | Reason | +| --- | --- | +| Identifying/naming the process holding the lock | Needs handle enumeration (Restart Manager / Sysinternals `handle64`); not installed, no Node binding. The error names the blocked *path* instead | +| Killing arbitrary (non-app) processes rooted in the worktree | Owner decision: terminate only sessions the app itself started. Killing a user's editor is not ours to do | +| Persisted pending-cleanup queue / auto-retry on app start | Unnecessary under delete-first: a failed removal leaves the worktree **registered**, so the existing row *is* the retry handle | +| Workspace-wide scan for abandoned worktree folders | Heuristic detection would false-positive on ordinary folders; owner chose the precise create-time collision path instead | +| Cleaning orphans already on disk, other than at create time (P2) | One-time manual cleanup; the app can no longer produce new ones after this feature | +| Deleting the branch with the worktree | Unchanged from DLWT/FRWT: removal is worktree-only | +| `git worktree prune` surface | Verified equivalent for bookkeeping, but repo-wide; the per-worktree `git worktree remove` is the narrower tool | +| Stopping junction-bearing worktrees from reading as dirty | Real annoyance (`?? .skills/` forces every hook worktree down the force path), but it is a hook/ignore concern, not removal | +| Auto-stash / preserving discarded work | Unchanged stance from FRWT | + +--- + +## Verified behavior (measured, not assumed) + +All on this machine: **git 2.49.0.windows.1**, **Node 24.9.0**, Windows 11. Probe scripts under the +session scratchpad; each result below was observed, not inferred. + +**A. The reported fault reproduces exactly.** External process holds `sub/deep.txt` with `FileShare.None`: + +``` +git worktree remove --force → exit 255 +error: failed to delete '': Invalid argument + files left on disk : sub/, sub/deep.txt, untracked.txt (partial deletion) + .git file : deleted by git + .git/worktrees/: DELETED ANYWAY → gone from `git worktree list` + retry : fatal: '' is not a working tree ← git can no longer help + branch : survives +``` + +**B. The orphan is invisible and blocking.** No `.git` remains, so `scanRepos` skips it. Recreating that +worktree later: `git worktree add` → `fatal: '' already exists` (a non-empty leftover). An **empty** +leftover directory is accepted by `git worktree add` (exit 0). + +**C. Delete-first works and inverts the failure mode.** + +``` +fs.rm(wt, {recursive:true, force:true}) → ok +git worktree remove (dir already gone) → exit 0, bookkeeping cleaned, no longer listed +partial fs.rm failure (locked file) → worktree STILL registered + listed; admin dir intact; + retry after the holder exits → exit 0, fully clean +``` + +**D. Junction data loss is real, and app-side deletion fixes it.** Identical fixture, shared folder +junctioned into the worktree as `.skills`: + +``` +git status --porcelain of the worktree → "?? .skills/" (⇒ dirty ⇒ UI routes to force) +git worktree remove --force → reports SUCCESS, shared target: nested/, nested/deep.txt, precious.txt → (EMPTY) +fs.rm recursive/force → shared target: nested/, nested/deep.txt, precious.txt (fully intact) +``` + +**E. `fs.rm` is safe on the content that worried us.** Read-only (`0444` + `attrib +R`) files: deleted. +A nested real git repo with its read-only object store: deleted. Node's own open file handles do **not** +block deletion (libuv sets `FILE_SHARE_DELETE`) — so a test fixture needs a genuine external holder. + +**F. Node's built-in retry ladder is unusable; a self-managed loop is not.** Directory locked by a child +process whose cwd is inside it (the real agent-terminal case), time to fail: + +| Attempt policy | Result | Wall time | +| --- | --- | --- | +| `maxRetries: 0` | EBUSY | **2 ms** | +| `maxRetries: 1, retryDelay: 100` | EBUSY | 326 ms | +| `maxRetries: 2, retryDelay: 100` | EBUSY | 1 239 ms | +| `maxRetries: 5, retryDelay: 200` | EBUSY | **21 599 ms** ⚠️ | +| own loop: 4 × (`maxRetries: 0`) spaced 250 ms | EBUSY | **786 ms** | +| own loop, holder exits at 600 ms | **OK** | 796 ms | +| happy path, 200 files, no lock | OK | 121 ms | + +Node retries at every level of the recursive walk, so its cost compounds. The policy must therefore be +`maxRetries: 0` per attempt inside our own deadline-bounded loop. + +**G. `git worktree lock` must be checked before deleting.** A locked worktree shows a `locked ` +line in `git worktree list --porcelain`, and git refuses removal even with a single `--force` +(`use 'remove -f -f' to override or unlock first`). Under delete-first, nothing else would enforce it. + +--- + +## Decisions (gray areas resolved during Specify) + +- **Delete-first ordering** _(owner-selected)_: `removeWorktree` deletes the worktree tree itself, then + calls `git worktree remove` purely to drop bookkeeping. Chosen over "recover after git's failure" and + "snapshot/restore the admin dir" because it is the only option that also closes the junction data-loss + path (finding D), and because its failure mode is the benign one — git never runs, so the worktree stays + registered (finding C). +- **Auto-retry, then an inline error naming the leftovers** _(owner-selected)_: a bounded retry absorbs the + common transient lock; on exhaustion the Danger section names the blocked path and the remaining entry + count. **No new persistence** — the still-registered worktree is itself the retry handle. +- **Terminate known sessions, wait for real exit, then retry** _(owner-selected)_: today `SessionManager.stop` + kills the PTY and finalizes synchronously (`session-manager.ts:118-123`), and `sessions:stop` resolves + immediately, so the renderer starts deleting while children may still hold handles (candidate lesson + L-003: killing a shell does not kill its children). The wait plus the retry loop is what makes it reliable. + Arbitrary process hunting stays out of scope. +- **Create-time leftover collision handled as P2** _(owner-selected)_: a create whose target exists, + is non-empty, and is not a registered worktree offers clean-and-continue rather than surfacing + `fatal: already exists`. A workspace-wide orphan scan was rejected as too heuristic. +- **Bookkeeping cleaner = `git worktree remove `** (agent default): verified exit 0 once the directory + is gone (finding C). Preferred over `git worktree prune`, which is repo-wide and could clear unrelated + stale entries. +- **`force` keeps its FRWT meaning — "skip the dirty check" only**: it no longer implies git's `--force` + deletion, because the app performs the deletion. The primary/registered/locked guards apply under force. + +--- + +## Assumptions & Open Questions + +| Assumption / decision | Chosen default | Rationale | Confirmed? | +| --- | --- | --- | --- | +| Removal strategy | Delete-first, git for bookkeeping | Owner-selected; measured findings C + D | y | +| Failure surface | Bounded auto-retry, then inline error naming leftovers; no persistence | Owner-selected | y | +| Lock handling | Terminate app-owned sessions, await exit, then retry | Owner-selected | y | +| Existing leftovers | Handled only at create time (P2) | Owner-selected | y | +| Retry interval / budget | 250 ms between attempts, 3000 ms total budget, `maxRetries: 0` per attempt | Derived from finding F: a failing attempt costs ~2 ms, so the budget is wall-clock honest, and a released lock self-heals in ~800 ms | n (agent default) | +| Session-exit wait | 3000 ms, then proceed anyway | A hung child must not block removal forever; the retry + leftover report covers the residue | n (agent default) | +| Paths > 260 chars | Rely on libuv's `\\?\` long-path handling; **not probed** | Node normalizes long Windows paths internally. If it fails, the leftover report surfaces it as a named failure rather than a silent orphan — the failure mode is safe either way | n (agent default) | +| Retryable error set | `EBUSY`, `EPERM`, `ENOTEMPTY`, `EACCES`; everything else reports immediately | The lock-type errors Windows raises for sharing violations (finding F yielded `EBUSY`); retrying e.g. `EINVAL` only wastes the budget | n (agent default) | +| Junction detection | Node's `fs.rm` native behavior (`lstat` reports junctions as symlinks → unlink, no recursion) | Finding D verified the target survives; no hand-rolled reparse-point handling needed | y (measured) | +| Removal remains a single IPC round-trip | No progress streaming for long deletions | A 200-file tree deletes in 121 ms; the bounded failure path reports in < 1 s | n (agent default) | + +**Open questions:** none — all resolved or logged above. + +--- + +## Implicit-requirement dimensions sweep + +| Dimension | Resolution | +| --- | --- | +| Input validation & bounds | WRFT-01 AC 2-4: the target must be a registered worktree of *this* repo, not the primary checkout, path-normalized — the app never recursively deletes an unvalidated path | +| Failure / partial-failure states | WRFT-02 (never deregister with files remaining; partial deletion stays registered) + WRFT-04 (leftover report) | +| Idempotency / retry / duplicate handling | WRFT-02 AC 2-4: retry after a partial failure self-heals; a second removal of an already-deleted directory still cleans bookkeeping and returns ok | +| Auth boundaries & rate limits | N/A because this is a single-user local desktop app with no auth surface and no remote callers | +| Concurrency / ordering | WRFT-05 (kill → observed exit → delete → deregister ordering) + Edge Cases (concurrent double-remove is idempotent; the in-flight button disable from DLWT-02 AC 5 is unchanged) | +| Data lifecycle / expiry | WRFT-03: data outside the worktree (junction targets) must survive removal. Leftovers have no TTL — they stay registered and user-driven, by decision | +| Observability | WRFT-04 AC 3 + WRFT-06 AC 1: the failure names the blocked path and the remaining count in the UI. Metrics/tracing N/A because the app has no logging infrastructure | +| External-dependency failure | WRFT-02 AC 3: git failing at the bookkeeping step is returned, never thrown, and self-heals on retry; the existing `gitFailureLine` discipline is unchanged | +| State-transition integrity | WRFT-02 AC 1 is the invariant: `registered+present → registered+absent → unregistered`; the app never reaches `unregistered+present` | + +--- + +## User Stories + +### P1: Delete-then-deregister with pre-flight guards ⭐ MVP + +**User Story**: As a developer, I want the app to delete the worktree folder itself before telling git to +forget it, so that a blocked deletion never leaves an invisible folder behind. + +**Acceptance Criteria**: + +1. WHEN `removeWorktree(repoPath, worktreePath)` is called on a clean, non-primary, unlocked, registered + worktree THEN the app SHALL delete the worktree directory itself **first**, then run + `git worktree remove ` for bookkeeping, and return `{ ok: true }` — with the folder gone + from disk **and** the entry gone from `git worktree list --porcelain` +2. WHEN `worktreePath` is not present as a `worktree` entry of `repoPath` in `git worktree list --porcelain` + THEN remove SHALL refuse with a message stating it is not a registered worktree of this repo, and SHALL + delete nothing from disk +3. WHEN the entry carries a `locked` line THEN remove SHALL refuse with a message including git's lock + reason, and SHALL delete nothing — including under `force: true` +4. WHEN `worktreePath` equals `repoPath` (primary checkout) THEN remove SHALL refuse with the unchanged + DLWT-01 message before any deletion, including under `force: true` +5. WHEN the worktree is dirty and `force` is absent/false THEN remove SHALL refuse with the unchanged + `"N uncommitted change(s) — commit or stash before removing."` message before any deletion +6. WHEN `force: true` THEN only the dirty pre-check SHALL be skipped; AC 2, 3 and 4 SHALL still refuse + +**Independent Test**: Vitest on real temp git repos — clean remove leaves neither folder nor listing entry; +an unregistered path refuses with the folder untouched; a `git worktree lock`ed worktree refuses (with the +reason) under both plain and `force` calls; primary refuses under force; dirty refuses without force. + +--- + +### P1: A worktree is never deregistered while its files remain ⭐ MVP + +**User Story**: As a developer, I want a failed deletion to leave the worktree fully registered, so that the +row stays visible and I can simply retry instead of hunting an invisible folder. + +**Acceptance Criteria**: + +1. WHEN the directory deletion does not complete THEN `git worktree remove` SHALL NOT be invoked at all, the + worktree SHALL still appear in `git worktree list --porcelain`, and the result SHALL be `{ ok: false }` +2. WHEN a removal that failed on a lock is retried after the holding process has ended THEN the retry SHALL + complete both steps and return `{ ok: true }` +3. WHEN the directory deletion succeeds but `git worktree remove` fails THEN the result SHALL be + `{ ok: false }` carrying git's first stderr line, and a subsequent retry SHALL return `{ ok: true }` + (git accepts removing a registered worktree whose directory is already gone — finding C) +4. WHEN the worktree directory is already absent (deleted outside the app) THEN the deletion step SHALL be a + no-op and the bookkeeping step SHALL still run, returning `{ ok: true }` + +**Independent Test**: Vitest — hold a lock inside a real temp worktree, call remove, assert `ok: false` **and** +that `git worktree list --porcelain` still contains the path; release the holder, call remove again, assert +`ok: true` and both the folder and the entry are gone. + +--- + +### P1: Removal never destroys data outside the worktree ⭐ MVP + +**User Story**: As a developer whose worktrees contain skills junctions (AD-013), I want removal to unlink +those junctions rather than delete through them, so that removing a worktree never empties my shared folder. + +**Acceptance Criteria**: + +1. WHEN the worktree contains a directory junction or symlink THEN removal SHALL unlink it without recursing + into it, and every file under the junction's target SHALL still exist afterwards with unchanged content +2. WHEN removal completes for such a worktree THEN the result SHALL be `{ ok: true }` and the worktree folder + (including the junction entry itself) SHALL be gone +3. WHEN the junction target is unreachable or already deleted THEN removal SHALL still succeed (a dangling + junction is unlinked like any other entry) + +**Independent Test**: Vitest on a real temp repo — junction a fixture folder containing `nested/deep.txt` and +`precious.txt` into the worktree, remove the worktree, assert both files still exist. This test fails against +today's implementation (finding D measured the target emptied), which is the point. + +--- + +### P1: Bounded retry with an actionable leftover report ⭐ MVP + +**User Story**: As a developer, I want a transient lock to resolve itself and a stubborn one to tell me +exactly what is blocking, so that I never have to guess why a removal failed. + +**Acceptance Criteria**: + +1. WHEN a deletion attempt fails with `EBUSY`, `EPERM`, `ENOTEMPTY` or `EACCES` THEN the app SHALL retry the + deletion every **250 ms** until a total budget of **3000 ms** is exhausted, each attempt using + `maxRetries: 0` so Node's own compounding retry ladder is never engaged (finding F: it costs 21 599 ms) +2. WHEN the lock is released within the budget THEN removal SHALL proceed to the bookkeeping step and return + `{ ok: true }` +3. WHEN the budget is exhausted THEN the result SHALL be `{ ok: false }` with a `leftover` payload carrying + `blockedPath` (the path of the entry that could not be deleted) and `remaining` (the **recursive** count + of every entry still present anywhere under the worktree root, not just its top level), and the `error` + message SHALL name `blockedPath`, state the remaining count, and say the worktree is still registered and + the removal can be retried. The `leftover` payload SHALL be present on the returned result itself — the + renderer branches on it (`WorktreeDetail.tsx`), so it is part of the contract, not an internal detail. + Guard refusals (primary / unregistered / locked / dirty) carry **no** `leftover`, since nothing was deleted +4. WHEN a deletion attempt fails with any other error code THEN the app SHALL report it immediately in the + same shape without consuming the retry budget +5. WHEN removal fails for any reason THEN it SHALL return within **5000 ms** of the call + +**Independent Test**: Vitest — with a fake deleter that fails N times then succeeds, assert the call succeeds +and that attempts were spaced by the interval; with a permanently failing fake, assert `ok: false`, the +`leftover` payload, and that the elapsed time respects the budget. One real-lock test (external holder +process) proves the fake matches reality. + +--- + +### P1: Terminated sessions are really gone before deletion starts ⭐ MVP + +**User Story**: As a developer removing a worktree with running agents, I want the app to wait for those +processes to actually exit before it deletes files, so that its own terminals stop being the thing that +blocks the removal. + +**Acceptance Criteria**: + +1. WHEN removal is confirmed for a worktree with running sessions THEN each session's PTY SHALL be observed + exited (its real exit event) before the directory deletion begins, or a **3000 ms** wait SHALL elapse first +2. WHEN a session's process does not exit within that wait THEN removal SHALL proceed anyway rather than + blocking indefinitely — the retry loop and leftover report cover the residue +3. WHEN a stopped session's handles are released shortly after its exit THEN the WRFT-04 retry loop SHALL + absorb the delay and the removal SHALL succeed without further user action +4. WHEN a session stop fails THEN the existing behavior SHALL be unchanged: removal is aborted and the error + surfaces inline (`WorktreeDetail.tsx:173-179`) + +**Independent Test**: Vitest against `SessionManager` with a fake PTY port whose exit is delayed — assert the +stop resolves only after the port's exit event fires, and that it resolves anyway once the wait elapses for a +port that never exits. + +--- + +### P1: The failure is visible in the UI and the row stays ⭐ MVP + +**User Story**: As a developer, I want a failed removal to leave the worktree in the sidebar with a clear +reason, so that "removal failed" and what I see actually agree. + +**Acceptance Criteria**: + +1. WHEN removal fails THEN the Danger section SHALL show the error including the blocked path and the + remaining entry count, and the worktree SHALL still be listed after a tree refresh (today it disappears) +2. WHEN the user ends the blocking process and clicks Remove again THEN the removal SHALL succeed, the row + SHALL disappear and the `"Removed "` toast SHALL show +3. WHEN removal fails THEN the Remove button SHALL return to its enabled state (no permanent busy) so the + retry needs no app restart +4. WHEN the blocked path is long THEN it SHALL wrap or scroll within the Danger section without pushing the + layout (mirror the existing inline-error treatment) + +**Independent Test**: CDP smoke (`scripts/smoke-remove.mjs` extension) — hold a lock inside a seeded +worktree, click Remove, assert the inline error names the path and the row is still present after a refresh; +release the holder, click Remove, assert the row disappears with the toast. + +--- + +### P2: Create over a leftover folder offers clean-and-continue + +**User Story**: As a developer recreating a worktree whose folder was orphaned by an earlier failure (or a +manual `git worktree remove`), I want the app to offer to clear it, so that I am not blocked by a raw git error. + +**Acceptance Criteria**: + +1. WHEN a create targets a path that exists, is non-empty, is not a registered worktree of any repo, and does + not contain a `.git` directory THEN the create SHALL return `conflict: 'path-exists'` with the entry count, + **upgrading the app's own existing flat refusal** — `createWorktree` already guards the target at + `worktree-manager.ts:87-89` with `Target path already exists: `, so git's + `fatal: '' already exists` is never actually reached and this AC replaces a dead-end app message, + not a raw git error *(wording corrected during T8: the original AC named the git error)* +2. WHEN the user confirms cleanup THEN the app SHALL delete the leftover using the same junction-safe bounded + deleter and then proceed with the create; the resulting worktree SHALL be created normally (post-create + hook included, per AD-013) +3. WHEN that cleanup deletion fails THEN the create SHALL abort with the WRFT-04 leftover report and no + worktree SHALL be created +4. WHEN the target path contains a `.git` directory or is a registered worktree THEN cleanup SHALL NOT be + offered and the create SHALL refuse with a message saying the path holds a repository or worktree +5. WHEN the target path exists but is empty THEN the create SHALL proceed unchanged (git accepts an empty + directory — finding B) + +**Independent Test**: Vitest on real temp repos — create onto a non-empty junk folder returns +`conflict: 'path-exists'` with the count and creates nothing; the confirmed path clears it and creates the +worktree; a folder containing a `.git` directory refuses without offering cleanup. + +--- + +## Edge Cases + +- WHEN two removals of the same worktree run concurrently THEN the second SHALL find the directory already + gone, clean bookkeeping (or find it already clean) and return `{ ok: true }` — never a hard error +- WHEN the worktree contains read-only files or a nested repository with a `0444` object store THEN deletion + SHALL still succeed (finding E) +- WHEN the worktree path contains spaces or non-ASCII characters THEN removal SHALL handle it (`execFile`, + no shell — unchanged discipline) +- WHEN the worktree is in detached-HEAD state THEN removal SHALL behave as for any non-primary worktree +- WHEN a path inside the worktree exceeds 260 characters and deletion fails THEN it SHALL surface as a named + leftover failure (never a silent orphan) — see Assumptions +- WHEN the repo itself is gone or git is unavailable THEN the registered-worktree pre-check SHALL fail closed: + refuse and delete nothing +- WHEN the dirty pre-check itself fails to run (unreadable worktree) THEN the existing `statusOf` stance + (report clean) is unchanged — the registered/primary/locked guards still apply + +--- + +## Requirement Traceability + +| Requirement ID | Story | Phase (tasks) | Status | +| --- | --- | --- | --- | +| WRFT-01 | P1: Delete-then-deregister with pre-flight guards | Phase 2 (T3, T4) | ✅ Verified (round 3) | +| WRFT-02 | P1: Never deregister while files remain | Phase 1–2 (T1, T4) | ✅ Verified (round 3) | +| WRFT-03 | P1: No data destroyed outside the worktree (junctions) | Phase 1 (T1, T2) | ✅ Verified (round 3) | +| WRFT-04 | P1: Bounded retry + actionable leftover report | Phase 1–2 (T1, T2, T4, T5) + F1–F4 | ✅ Verified (round 3, all five AC 3 clauses pinned) | +| WRFT-05 | P1: Sessions really exited before deletion starts | Phase 2 (T6) | ✅ Verified (round 3) | +| WRFT-06 | P1: Failure visible in the UI, row stays, retry works | Phase 2–3 (T5, T7) | ⏳ Unverified — awaits the owner's live smoke run + visual pass | +| WRFT-07 | P2: Create over a leftover folder offers clean-and-continue | Deferred — follow-up PR (T9–T11) | ⏸ Deferred by owner decision (AD-014) | + +**Status legend:** `✅ Verified` means the independent Verifier (author ≠ verifier) confirmed the AC against +`file:line` assertion evidence **and** the discrimination sensor killed mutations of that behavior. +`⏳ Unverified` means implemented with a green gate but with **no executed evidence** — WRFT-06 is renderer +behavior, which this project does not unit-test by convention (AD-004/AD-011), and its CDP smoke has been +written but never run. `⏸ Deferred` means specified but deliberately not built on this branch. + +**Verification history:** three rounds. Round 1 FAIL (14/16 mutants killed; `leftover` never asserted, the +recursive count unpinned), round 2 FAIL (8/10; the round-1 fix's fixture was directories-only and so blind +to *what* it counted, and guard-refusal absence was pinned on one guard of four), round 3 **PASS** (12/15, +3 survivors all non-blocking). Every fix was test-only: `git diff --name-only dcc50dc..HEAD` touches no +production file. Full evidence in `validation.md`. + +**Known non-blocking survivors** (recorded, deliberately not fixed): a guard `leftover` conditioned on +`force: true`; `blockedPath` naming the first rather than the last failing attempt (spec leaves it open, and +a discriminating fixture would be racy); and the two guard message literals, whose *behavior* is pinned +while their wording is not. + +**WRFT-07 pointer:** deferred to a follow-up PR at the owner's decision during Tasks approval, and recorded +in **AD-014**. Its tasks stay written verbatim as T9–T11 in `tasks.md` so the follow-up can lift them; they +build on the seams this branch creates (`removeDirTree`, and the `createWorktree` target guard at +`worktree-manager.ts:87-89` that `classifyTargetPath` replaces). + +**Coverage target:** 7 requirements. WRFT-01..05 and WRFT-07's backend half are unit-testable +(`worktree-manager.test.ts`, `session-manager.test.ts`); WRFT-06 and WRFT-07's dialog follow the project's +renderer convention (hand-verified + CDP smoke). WRFT-06's smoke (`scripts/smoke-remove.mjs`) is **written +but not yet run** — a CDP smoke needs a live desktop session and is hand-run by the owner, never automated. + +--- + +## Testing Notes + +- **Real-lock fixture**: Node's own handles do not block deletion (finding E), so the honest fixture is an + **external holder** — a child process whose cwd is inside the worktree (`spawn(process.execPath, ['-e', + 'setTimeout(...)'], { cwd: })`), which is also the real-world agent-terminal case. Measured + cost ≈ 400 ms spawn settle + ~800 ms failing loop. +- **Keep the slow path rare**: use a DI'd fake deleter for the retry-policy assertions (fast, deterministic) + and **one** real-lock test to prove the fake matches reality. Candidate lesson L-005 warns that + `worktree-manager.test.ts` already sits near the default per-test timeout — give the real-lock and + real-junction tests explicit generous timeouts rather than inheriting the default. +- **Assert literals, not constants** (candidate lesson L-004): pin `250`, `3000` and `5000` as literal + expectations so a mutation of the constants is caught. +- **L-001 (confirmed)**: `RemoveWorktreeResult.leftover` crosses `shared/worktrees.ts` → main → renderer. + Wire producer and consumer in the same phase; do not relax the field to keep an interim typecheck green. +- **Regression protection**: the existing DLWT/FRWT tests must stay green unchanged — the guard messages and + result shape are deliberately preserved. Anchor the expected-pass count to the current baseline (533 tests + / 39 files) with no deletions. +- **Junction test** must assert the *target's* contents, not just that the worktree is gone — asserting only + the worktree would pass against today's data-destroying implementation. +- Gate: `npm run typecheck && npm run lint && npm test`; `node scripts/smoke-remove.mjs` on a live session. + +## Success Criteria + +- [ ] With a process holding a file in a worktree: Remove reports the blocked path inline, the row is still + there after a refresh, and `git worktree list` still contains the worktree — no invisible folder exists +- [ ] Ending that process and clicking Remove again completes the removal (folder gone, row gone, toast) +- [ ] Removing a worktree that contains a skills junction leaves the shared source folder byte-identical +- [ ] A worktree whose agent sessions were just terminated removes on the first click, with no manual retry +- [ ] `git worktree lock`ed and primary worktrees still refuse, and refuse without deleting anything +- [ ] Creating a worktree over a leftover folder offers cleanup and then succeeds (P2) +- [ ] Full gate green with the existing DLWT/FRWT tests unchanged diff --git a/.specs/features/worktree-removal-fault-tolerance/tasks.md b/.specs/features/worktree-removal-fault-tolerance/tasks.md new file mode 100644 index 0000000..26f3ffa --- /dev/null +++ b/.specs/features/worktree-removal-fault-tolerance/tasks.md @@ -0,0 +1,800 @@ +# Worktree Removal Fault Tolerance Tasks + +## Execution Protocol (MANDATORY -- do not skip) + +Implement these tasks with the `tlc-spec-driven` skill: **activate it by name and follow its Execute flow and Critical Rules.** Do not search for skill files by filesystem path. The skill is the source of truth for the full flow (per-task cycle, sub-agent delegation, adequacy review, Verifier, discrimination sensor). + +**If the skill cannot be activated, STOP and tell the user — do not proceed without it.** + +--- + +**Design**: `.specs/features/worktree-removal-fault-tolerance/design.md` +**Status**: Draft + +--- + +## Test Coverage Matrix + +> Generated from codebase, project guidelines, and spec — confirm before Execute. Guidelines found: +> `.specs/codebase/TESTING.md` (authoritative), `vitest.config.ts` (coverage scoped to `src/main` + +> `src/shared`; renderer and thin shells intentionally excluded), `.github/workflows/ci.yml` (gate = +> `typecheck && lint && test`), `.specs/STATE.md` AD-003 (coverage is report-only), AD-004/AD-011 +> (renderer units not tested by convention), AD-005 (Windows-only; tests assert backslash paths). + +| Code Layer | Required Test Type | Coverage Expectation | Location Pattern | Run Command | +| --- | --- | --- | --- | --- | +| Main-process deep modules with logic (`dir-remover`, `WorktreeManager`, `SessionManager`) | **unit** | All branches; 1:1 to spec ACs; every listed edge case has a test | `src/main/.test.ts` | `npm test` | +| Extracted pure helpers (porcelain `locked` parsing, `classifyTargetPath`, leftover message) | **unit** | Input→output for every documented mapping, including the edge cases | co-located `src/main/*.test.ts` | `npm test` | +| Shared types (`src/shared/worktrees.ts`, `ipc-contract.ts`) | none — build gate only | — | — | `npm run typecheck` | +| Thin OS/Electron shells (`index.ts` IPC wiring) | none (hand-verified) | — | `src/main/index.ts` | `npm run typecheck` | +| Renderer React components (`WorktreeDetail`, dialogs, `LeftoverPathChoice`) | none (CDP smoke + visual pass) | — | — | `node scripts/smoke-remove.mjs` | +| Out-of-CI smoke scripts | manual only | — | `scripts/smoke-*.mjs` | `node scripts/smoke-remove.mjs` (live session) | + +**Deviation from TESTING.md worth stating:** TESTING.md says "no mocking library is used anywhere +(no `vi.mock`); fakes are hand-rolled and injected" — this feature conforms (the deleter is injected via a +defaulted `deps` param, never mocked). It also lists no fake-timer usage; `dir-remover.test.ts` introduces +`vi.useFakeTimers()` for the retry cadence. That is a **new pattern for this repo**, chosen because the +alternative (real 3 s waits) would add ~10 s to the suite and invite the flakiness lesson L-005 warns about. + +## Parallelism Assessment + +> Generated from codebase — confirm before Execute. + +| Test Type | Parallel-Safe? | Isolation Model | Evidence | +| --- | --- | --- | --- | +| Unit (pure) | **Yes** | No shared state; input→output | `tree.test.ts`, `shortcut-launcher.test.ts` | +| Unit (temp-dir, real git) | **Yes** | Per-test `realpathSync.native(mkdtempSync(...))` + `rmSync` teardown | `worktree-manager.test.ts:30-46` | +| Unit (injected fake) | **Yes** | Hand-rolled fakes per test, no globals | `task-board.test.ts`, `post-create-hook.test.ts` | +| Unit (fake timers) | **Yes** | `vi.useFakeTimers()` scoped per file, restored in `afterEach`; no real fs in those tests | new in `dir-remover.test.ts` | +| Unit (spawns a real holder process) | **Yes**, but slow | Own temp dir + own child process, killed in `afterEach` | new; mirrors `hook-shell.test.ts` real-process style | +| CDP smoke | **No** | Single live app on a fixed debug port + shared disk state | `scripts/smoke-*.mjs` | + +⚠️ **Lesson L-005 applies to T2 and T7**: `worktree-manager.test.ts` already raises its own timeouts +(`vi.setConfig({ testTimeout: 30000 })` at line 445) because real-git tests get starved under parallel +load. Every new real-process/real-git test must set an explicit generous timeout rather than inherit the +5 s default. + +## Gate Check Commands + +> Generated from codebase — confirm before Execute. + +| Gate Level | When to Use | Command | +| --- | --- | --- | +| **Quick** | After a task whose only tests are unit tests | `npm test` | +| **Full** | After a logic-bearing task / before PR | `npm run typecheck && npm run lint && npm test` | +| **Build** | After phase completion | `npm run build:win` | +| **Manual** | Renderer/user-facing behavior | `node scripts/smoke-remove.mjs` (live session) | + +**Baseline:** **533 tests / 39 files green** — established *after* T0; the two runs before it were red +(`2 failed`, then `14 failed`) purely from timeout starvation. Every task's expected count is +`533 + N` with **zero deletions**. Note TESTING.md's own header still cites the stale 125/11 figure — +anchor to the live run, not to that line. + +--- + +## Execution Plan + +### Phase 0: Make the gate trustworthy (Sequential) + +Added during Execute, before T1, after two baseline runs of untouched `main` came back red. + +``` +T0 +``` + +### Phase 1: The deleter (Sequential) + +``` +T1 → T2 +``` + +### Phase 2: Ordering, guards and the session wait (Parallel OK) + +``` + ┌→ T3 ─→ T4 ─→ T5 ─┐ +T2 ─────┤ ├──→ (Phase 3) + └→ T6 [P] ─────────┘ +``` + +### Phase 3: Surfacing it (Sequential) + +``` +T5, T6 → T7 → T8 +``` + +### Deferred to a follow-up PR (owner decision, during Tasks approval) + +T9–T11 (WRFT-07, the create-time leftover collision) **are not executed on this branch.** They stay +specified below so the follow-up feature can lift them verbatim. This branch ships WRFT-01..06; the +follow-up ships WRFT-07 on top of the deleter and classification seams this branch creates. + +``` +(follow-up PR) T9 → T10 → T11 +``` + +--- + +## Task Breakdown + +### T0: Stabilize the test gate (added during Execute) + +**What**: Raise Vitest's global test/hook timeouts and widen one racing fixture window, so the gate is +deterministic before this feature adds more real-git and real-process tests. +**Where**: `vitest.config.ts`, `src/main/hook-shell.test.ts` +**Depends on**: None +**Reuses**: the local precedent at `worktree-manager.test.ts:445` (`vi.setConfig({ testTimeout: 30000 })`) +**Requirement**: none — enabling work for every task's gate + +**Why it exists**: two full runs of untouched `main` failed — `2 failed | 531 passed`, then +`14 failed | 519 passed` across 5 files. Every failure was a duration overrun against the 5 s default +(11 430–15 557 ms), and `tree.test.ts` passed alone in 9.3 s. This is candidate lesson **L-005 recurring on +a second feature** (first seen in `worktree-post-create-hook`), which qualifies it for promotion to +confirmed. With a gate failing 2–14 random tests per run, no task's "gate passes" claim means anything. + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] `vitest.config.ts` sets `testTimeout: 30000` and `hookTimeout: 30000`, with a comment recording the + measurement that motivated it +- [x] `hook-shell.test.ts:96` passes `timeoutMs: 1500` instead of `500` (owner-approved fixture fix): under + load `ping` emitted nothing before the kill, so the output-tail assertion saw `''`. `ping -n 5` still + runs ~4 s, so the command is still killed mid-flight — **no assertion weakened, no production code + touched** +- [x] Full suite green: **533 passed / 39 files, 0 failed** (149.5 s) — typecheck clean, lint 0 errors / + 18 pre-existing warnings (unchanged count) +- [x] No test deleted, skipped, or weakened + +**Tests**: none (test infrastructure) +**Gate**: full — `npm run typecheck && npm run lint && npm test` +**Commit**: `test(infra): stop the real-git suites racing the default timeout` + +--- + +### T1: Create the junction-safe bounded deleter + +**What**: New `dir-remover.ts` exporting `removeDirTree(path, deps?)`, the two retry constants, and the +`DirRemovalResult`/`RemovalLeftover` shapes it returns. +**Where**: `src/main/dir-remover.ts` (new), `src/shared/worktrees.ts` (add `RemovalLeftover`) +**Depends on**: None +**Reuses**: DI-with-defaults convention (`SessionManagerDeps`, `withPostCreateHook`) +**Requirement**: WRFT-04 (AC 1, 2, 4), WRFT-02 (AC 4), WRFT-03 (mechanism) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] `removeDirTree` implements the design's loop: `maxRetries: 0` per attempt, retry only on + `EBUSY`/`EPERM`/`ENOTEMPTY`/`EACCES`, `DELETE_RETRY_INTERVAL_MS = 250` between attempts, giving up at + `DELETE_RETRY_BUDGET_MS = 3000` +- [x] A code comment states **why** `maxRetries: 0` is mandatory (measured 21 599 ms for `maxRetries: 5`) + so a future reader does not raise it +- [x] Returns `{ ok: true }` immediately when the path does not exist (WRFT-02 AC 4) +- [x] On give-up returns `{ ok: false, code, leftover: { blockedPath, remaining } }`; `remaining` counts + entries still under the root +- [x] Non-retryable codes return immediately without consuming the budget +- [x] `deps` (`rm`, `exists`, `readEntries`) default to the real fs functions; no `vi.mock` anywhere +- [x] Unit tests with fake deps + `vi.useFakeTimers()`: retry cadence (assert the **literal** 250 ms + spacing), budget exhaustion (**literal** 3000 ms), success after N transient failures, non-retryable + immediate return, missing-path no-op, leftover payload contents +- [x] One test pins the exported constants to their literal values (`250`, `3000`) — lesson L-004 +- [x] Gate check passes: `npm test` +- [x] Test count: baseline + 9 (no silent deletions) — **542 passed / 40 files** + +**Tests**: unit +**Gate**: quick +**Commit**: `feat(worktree): add junction-safe bounded directory remover` + +--- + +### T2: Prove the deleter against real filesystem hazards + +**What**: Real-fs tests for the three hazards that decide whether delete-first is safe at all: junction +targets, read-only content, and a genuinely locked directory. +**Where**: `src/main/dir-remover.test.ts` (extend) +**Depends on**: T1 +**Reuses**: real-temp-dir pattern (`TESTING.md` §2), real-process style from `hook-shell.test.ts` +**Requirement**: WRFT-03 (AC 1, 2, 3), WRFT-04 (AC 3, 5) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] **Junction test asserts the TARGET's contents survive** (`precious.txt` + `nested/deep.txt` still + readable after removal) — not merely that the worktree folder is gone. Written so it would FAIL + against a git-based deleter (it also reads through the junction *before* removing, so the assertion + cannot pass vacuously) +- [x] Dangling-junction test: target deleted first, removal still succeeds (WRFT-03 AC 3) +- [x] Read-only file (`chmod 0o444` + `attrib +R`) and a nested real git repo (`0444` object store) both + delete successfully +- [x] Real-lock test: a child process with `cwd` inside the tree (`spawn(process.execPath, ['-e', + 'setTimeout(…)'], { cwd })`) blocks deletion → asserts `ok: false`, the `leftover` payload, and that + the call returns within 5000 ms (WRFT-04 AC 5); after killing the holder a retry succeeds +- [x] Every test in this task sets an **explicit** timeout (lesson L-005) — 30000, matching T0's new global + so the ceiling is unchanged; the holder process is killed in `afterEach` even when the test fails +- [x] Gate check passes: `npm test` +- [x] Test count: baseline + 9 + 6 (no silent deletions) — **548 passed / 40 files** + +**Tests**: unit (real-fs) +**Gate**: quick +**Commit**: `test(worktree): pin junction safety, read-only and real-lock deletion` + +--- + +### T3: Parse the porcelain `locked` line + +**What**: Extend `PorcelainBlock` with `locked?: string` and teach `parsePorcelainBlocks` to read the +`locked [reason]` line. +**Where**: `src/main/worktree-manager.ts` +**Depends on**: T2 +**Reuses**: `parsePorcelainBlocks` (`worktree-manager.ts:316-339`) +**Requirement**: WRFT-01 (AC 3) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] `locked` with a reason yields the reason string; bare `locked` yields `''`; absent yields `undefined` + (the three cases are distinguishable — `''` must not read as "unlocked") +- [x] `listWorktrees` and `worktreeHosting` behavior is unchanged (additive field only) +- [x] Unit tests on a real temp repo using `git worktree lock --reason …` and a bare `git worktree lock` +- [x] Gate check passes: `npm test` +- [x] Test count: baseline + 15 + 3 (no silent deletions) — **551 passed / 40 files** + +**Tests**: unit +**Gate**: quick +**Commit**: `feat(worktree): parse the porcelain locked line` + +--- + +### T4: Reorder removeWorktree to delete-then-deregister + +**What**: Rewrite `removeWorktree`'s body to the design's 6-step guard table, with the deleter injected as a +defaulted 4th param. +**Where**: `src/main/worktree-manager.ts` +**Depends on**: T3 +**Reuses**: `samePath`, `gitFailureLine`, `statusOf`, T1's `removeDirTree` +**Requirement**: WRFT-01 (all), WRFT-02 (AC 1, 3, 4) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] Guard order is primary → registered → locked → dirty → delete → bookkeeping; **every** guard refuses + before any deletion +- [x] Unregistered path refuses and deletes nothing (the anti-`rm -rf` guard); `git worktree list` failure + fails **closed** +- [x] Locked worktree refuses with git's reason, under plain **and** `force: true` calls (plus a bare + `locked` line, whose reason parses to `''` and must still refuse) +- [x] Primary refuses under `force: true`; dirty refuses without force — both messages byte-identical to + today's (DLWT/FRWT regression) +- [x] Deletion failure returns `{ ok: false }` whose `error` names the blocked path + remaining count, and + `git worktree remove` is **never invoked** — asserted by checking the worktree is still in + `git worktree list --porcelain`. The **structured `leftover` field lands in T5** with its renderer + consumer, per lesson L-001 (do not ship the field with no consumer) +- [x] Bookkeeping runs only after the directory is gone; a bookkeeping failure returns git's first line and + a retry succeeds +- [x] 3-arg call sites (`index.ts`, `workflow-ctx`) compile unchanged +- [x] Unit tests on real temp repos for each guard + the ordering + the already-absent path; retry-policy + cases use an injected fake deleter +- [x] All existing `removeWorktree` tests still pass **unmodified** +- [x] Gate check passes: `npm run typecheck && npm run lint && npm test` +- [x] Test count: baseline + 18 + 10 (no silent deletions) — **561 passed / 40 files** + +**Tests**: unit +**Gate**: full +**Commit**: `fix(worktree): delete the worktree before deregistering it` + +--- + +### T5: Carry the leftover through IPC and render it + +**What**: Widen `RemoveWorktreeResult` with `leftover`, thread it through the contract, and render the +blocked path + count in the Danger section. **Producer and consumer land together (lesson L-001).** +**Where**: `src/shared/worktrees.ts`, `src/shared/ipc-contract.ts`, `src/renderer/src/components/WorktreeDetail.tsx`, `WorktreeDetail.css` +**Depends on**: T4 +**Reuses**: existing `.detail-danger-note.error` treatment; `setRemoving(false)` failure branch +**Requirement**: WRFT-04 (AC 3), WRFT-06 (all) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] `RemoveWorktreeResult.leftover?: RemovalLeftover`; `worktrees:remove` res widened; no `any` +- [x] The failure message names the blocked path, the remaining count, and says the worktree is still + registered and can be retried; count pluralizes (`1 item` / `N items`) +- [x] `WorktreeDetail` stores and clears `removeLeftover` on every new attempt, and renders the path in + monospace with `word-break: break-all` plus the count. **Deviation from design.md:** the structured + block *replaces* the flat error line rather than sitting below it — T4's main-side `error` is + self-contained (WRFT-04 AC 3 needs it for non-interactive callers), so rendering both would print + the long path twice. Every non-leftover failure still renders the flat line +- [x] Button returns to enabled after failure (verify the existing `setRemoving(false)` path — no + regression); the row is still present after a refresh +- [x] Typecheck passes across node + web projects +- [x] Gate check passes: `npm run typecheck && npm run lint && npm test` +- [x] Test count: baseline + 28 + 0 (renderer untested by convention; no deletions) — **561 passed / 40 files** + +**Tests**: none (renderer + shared types — matrix says build gate / smoke) +**Gate**: full +**Commit**: `feat(worktree): surface the blocked path when removal is left over` + +--- + +### T6: Make session stop await the real PTY exit [P] + +**What**: `SessionManager.stop` returns a promise resolving on the PTY's real exit, capped at +`SESSION_EXIT_WAIT_MS = 3000`, with `killAll` explicitly left fire-and-forget. +**Where**: `src/main/session-manager.ts`, `src/main/index.ts` (handler awaits) +**Depends on**: T2 +**Reuses**: existing idempotent `#finalize`, the `handle.onExit` registration in `#start` +**Requirement**: WRFT-05 (AC 1, 2, 3, 4) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] `#start` stores an `exited` promise resolved from the existing `onExit` callback; `stop` captures it + before `#finalize` drops the Map entry +- [x] `stop` still finalizes **immediately** (status flips to stopped synchronously — all 7 existing + `manager.stop(...)` call sites keep passing unmodified) +- [x] The wait is capped at 3000 ms; the timer is cleared in `finally` and `unref`'d, with a comment + distinguishing this from lesson L-003's grace timer (here the promise is awaited by a live caller, so + `unref` cannot skip work) +- [x] `killAll()` stays synchronous (`void this.stop(id)`) with a comment stating why (quit must not stall + up to 3 s per session) +- [x] Unit tests with a fake PTY port: resolves only after the fake's exit fires; resolves anyway after the + cap for a port that never exits (fake timers, real constant); existing session tests unmodified +- [x] Gate check passes: `npm test` +- [x] Test count: baseline + 28 + 3 (no silent deletions) — **564 passed / 40 files** + +**Note on `index.ts`**: no code change was needed — `handle('sessions:stop', ({ id }) => sessions.stop(id))` +already returns the promise and `ipcMain.handle` awaits it, so the channel resolves on the real exit the +moment `stop` became async. A comment was added at that line recording why the `return` is load-bearing. + +**Tests**: unit +**Gate**: quick +**Commit**: `fix(sessions): resolve stop only once the PTY has really exited` + +--- + +### T7: Extend the remove smoke with a real lock + +**What**: Extend `scripts/smoke-remove.mjs` with the blocked-then-retry flow against a live app. +**Where**: `scripts/smoke-remove.mjs`, `scripts/seed-smoke-remove.mjs` if seeding needs it +**Depends on**: T5, T6 +**Reuses**: existing CDP smoke structure and its check-count reporting +**Requirement**: WRFT-06 (AC 1, 2, 3) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] Smoke spawns a holder process inside a seeded worktree, clicks Remove, asserts the inline error names + the blocked path and that the row survives a tree refresh +- [x] Kills the holder, clicks Remove again, asserts the row disappears and the toast shows the branch +- [x] Holder process is killed even when the script fails (no leaked processes on a failed run) — the whole + WRFT-06 section sits in `try`/`finally` +- [x] Script documents that it needs a live session (never CI), per TESTING.md +- [ ] **OUTSTANDING (owner-run)**: `node scripts/smoke-remove.mjs` passes on a live session. Not run here — + a CDP smoke needs a live desktop session and a seeded workspace, and launching the app from an agent + would interfere with the owner's desktop. Verified instead by `node --check` on both scripts and by + reading them against the T5 renderer markup (`.detail-danger-leftover`, `.detail-danger-path`) +- [x] Test count: unchanged (smoke is not part of `npm test`) — **564 passed / 40 files** + +**Seeding change**: the two existing seeded worktrees are both removed by the earlier checks, so the seed +gained a third — `api-lock-me` (branch `lock/me`) with an empty `sub/`. Empty directories are invisible to +`git status`, so the worktree still reads clean and the first Remove click takes the direct path rather than +the confirm dialog. The holder parks its cwd in `sub/`, which makes `sub` the reported `blockedPath` and +leaves `remaining: 1` — mirroring the real-lock unit test at `dir-remover.test.ts:322`. + +**Known consequence, asserted around**: a blocked deletion still removes everything it *could* reach, +including the worktree's `.git` link file, so on the retry the row may read clean (direct remove) or dirty +(confirm dialog) depending on what survived. The retry step clicks `.dialog-btn-danger` optionally so either +shape passes — the requirement is that the retry succeeds, not which path it takes. + +**Tests**: none (manual smoke — matrix: renderer layer) +**Gate**: manual +**Commit**: `test(worktree): smoke the blocked-removal retry flow` + +--- + +### T8: Record AD-014 and update the spec's traceability + +**What**: Record the delete-first invariant as a project-level decision and mark WRFT-01..06 verified. +**Where**: `.specs/STATE.md`, `.specs/features/worktree-removal-fault-tolerance/spec.md` +**Depends on**: T7 +**Reuses**: existing AD-NNN format +**Requirement**: traceability for WRFT-01..06 + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [x] AD-014 states: worktree removal is delete-first; no surface may use `git worktree remove --force` as a + *deleter*; the junction rationale is recorded (git for Windows recurses into junctions); the guard + order (primary → registered → locked → dirty) and why the lock check must be ours (git's own refusal + would arrive *after* deletion); and records that **WRFT-07 is deferred to a follow-up PR** (owner + decision at Tasks approval) +- [x] Spec traceability rows for WRFT-01..06 move to their real status — **`⚙ Implemented — pending + Verifier`**, not Verified: the independent Verifier has not run. **WRFT-07 is marked Deferred** with a + pointer to the follow-up; the WRFT-07 AC 1 wording is corrected (the app's own `existsSync` guard at + `worktree-manager.ts:87-89` fires before git's `fatal`, so this upgrades an existing flat error rather + than replacing a git error) +- [x] Handoff section updated with the commit map — **section-scoped write**: only the `## Decisions` row + append and the `## Handoff` body changed; `git diff` confirms no existing AD row was touched +- [x] Gate check passes: `npm run typecheck && npm run lint && npm test` — **564 passed / 40 files**, 0 lint + errors / 18 pre-existing warnings + +**Tests**: none (docs) +**Gate**: quick +**Commit**: `docs(specs): record AD-014 delete-first worktree removal` + +--- + +## Deferred tasks (follow-up PR — not executed on this branch) + +### T9: Classify what sits at a create target ⏸ DEFERRED + +**What**: `classifyTargetPath(target)` returning `free | empty | leftover | occupied`, replacing the flat +`existsSync` guard in `createWorktree`. +**Where**: `src/main/worktree-manager.ts` +**Depends on**: T8 +**Reuses**: the existing guard site (`worktree-manager.ts:87-89`) +**Requirement**: WRFT-07 (AC 1, 4, 5) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [ ] `free` (absent) and `empty` both proceed to `git worktree add` (empty is accepted by git — verified) +- [ ] `occupied` (contains `.git`, file or directory) refuses with a "already contains a repository or + worktree" message and offers no cleanup +- [ ] `leftover` (non-empty, no `.git`) returns `{ ok: false, conflict: 'path-exists', pathConflict: { path, + entries }, error }` — `error` is set too, so non-interactive callers (`ctx.worktree.create`) get a + usable message +- [ ] **Expected test edit**: `worktree-manager.test.ts:428` ("short-circuits on target-path collision") + currently uses an **empty** dir, which must now proceed. Change the fixture to a non-empty leftover so + the ordering assertion survives, and add a separate test pinning empty-dir passthrough. This is the + one intentional edit to an existing test — call it out in the commit body +- [ ] Unit tests on real temp repos for all four classifications +- [ ] Gate check passes: `npm run typecheck && npm run lint && npm test` +- [ ] Test count: baseline + 31 + 5 (1 existing test modified, 0 deleted) + +**Tests**: unit +**Gate**: full +**Commit**: `feat(worktree): classify what occupies a create target` + +--- + +### T10: Add the clean-path channel ⏸ DEFERRED + +**What**: `cleanWorktreePath(repoPath, branch, worktreeTemplate?, deps?)` plus the +`worktrees:clean-path` IPC channel. +**Where**: `src/main/worktree-manager.ts`, `src/shared/ipc-contract.ts`, `src/main/index.ts` +**Depends on**: T9 +**Reuses**: `worktreePathFor`, `classifyTargetPath` (T9), `removeDirTree` (T1) +**Requirement**: WRFT-07 (AC 2, 3, 4) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [ ] The channel takes `{ repoPath, branch, worktreeTemplate? }` — **never a raw path**; the handler + recomputes the target with `worktreePathFor` +- [ ] Refuses unless the recomputed target classifies as `leftover` (so `occupied`/`free` can never be + deleted); returns `RemoveWorktreeResult` including `leftover` on failure +- [ ] Unit tests: cleans a leftover; refuses an `occupied` target; refuses when the recomputed target does + not exist; a deletion failure surfaces the leftover payload +- [ ] Gate check passes: `npm run typecheck && npm run lint && npm test` +- [ ] Test count: baseline + 36 + 4 (no silent deletions) + +**Tests**: unit +**Gate**: full +**Commit**: `feat(worktree): add the guarded clean-path channel` + +--- + +### T11: Offer clean-and-continue in both create dialogs ⏸ DEFERRED + +**What**: `LeftoverPathChoice` component + wiring in both create dialogs. +**Where**: `src/renderer/src/components/LeftoverPathChoice.tsx` (new) + `.css`, `NewWorktreeDialog.tsx`, `StartWorkDialog.tsx` +**Depends on**: T10 +**Reuses**: `BranchExistsChoice.tsx` structure; the dialogs' existing `conflict` state machine +**Requirement**: WRFT-07 (AC 1, 2, 3) + +**Tools**: MCP: NONE · Skill: NONE + +**Done when**: + +- [ ] `conflict` state widens to `'branch-exists' | 'path-exists'` in both dialogs +- [ ] The choice states the path and entry count, with a danger "Delete folder & create" primary and Cancel +- [ ] Confirm calls `worktrees:clean-path`, then re-invokes `worktrees:create` unchanged; a cleanup failure + shows the leftover inline and creates nothing +- [ ] Visual pass against `BranchExistsChoice`'s existing styling +- [ ] Gate check passes: `npm run typecheck && npm run lint && npm test`; smoke re-run for the create path +- [ ] Test count: baseline + 40 + 0 (renderer untested by convention) + +**Tests**: none (renderer — matrix: CDP smoke + visual) +**Gate**: full + manual +**Commit**: `feat(worktree): offer to clear a leftover folder before creating` + +--- + +## Parallel Execution Map + +``` +Phase 1 (Sequential): + T1 ──→ T2 + +Phase 2: + T2 complete, then: + T3 ──→ T4 ──→ T5 (sequential chain: same file, then its consumers) + T6 [P] (independent module: session-manager) + +Phase 3 (Sequential): + T5 + T6 complete, then: + T7 ──→ T8 + +Deferred (follow-up PR, not this branch): + T9 ──→ T10 ──→ T11 +``` + +**Phase count note:** with T9–T11 deferred this branch has **3** phases, which is the skill's inline +threshold. The owner nonetheless chose one sub-agent worker per phase during Tasks approval, so Execute +runs with three sequential phase workers plus the always-on independent Verifier. + +--- + +## Task Granularity Check + +| Task | Scope | Status | +| --- | --- | --- | +| T1: deleter module | 1 module + 1 type | ✅ Granular | +| T2: real-fs hazard tests | 1 test file | ✅ Granular | +| T3: porcelain `locked` | 1 function | ✅ Granular | +| T4: reorder `removeWorktree` | 1 function | ✅ Granular | +| T5: leftover through IPC + render | 1 type + 1 channel + 1 component (one cohesive slice, L-001) | ⚠️ OK — deliberately cohesive | +| T6: awaitable `stop` | 1 method | ✅ Granular | +| T7: smoke extension | 1 script | ✅ Granular | +| T8: AD-014 + traceability | docs only | ✅ Granular | +| T9: `classifyTargetPath` | 1 function | ✅ Granular | +| T10: clean-path channel | 1 function + 1 channel | ✅ Granular | +| T11: leftover choice UI | 1 component + 2 wirings | ⚠️ OK — same pattern as `BranchExistsChoice` | + +## Diagram-Definition Cross-Check + +| Task | Depends On (body) | Diagram Shows | Status | +| --- | --- | --- | --- | +| T1 | None | (root) | ✅ Match | +| T2 | T1 | T1 → T2 | ✅ Match | +| T3 | T2 | T2 → T3 | ✅ Match | +| T4 | T3 | T3 → T4 | ✅ Match | +| T5 | T4 | T4 → T5 | ✅ Match | +| T6 | T2 | T2 → T6 `[P]` | ✅ Match | +| T7 | T5, T6 | T5 + T6 → T7 | ✅ Match | +| T8 | T7 | T7 → T8 | ✅ Match | +| T9 | T8 | T8 → T9 | ✅ Match | +| T10 | T9 | T9 → T10 | ✅ Match | +| T11 | T10 | T10 → T11 | ✅ Match | + +T6 is the only `[P]` task; it shares no file and no state with T3/T4/T5 (`session-manager.ts` vs +`worktree-manager.ts`), and its tests are parallel-safe injected fakes. + +## Test Co-location Validation + +| Task | Code Layer Created/Modified | Matrix Requires | Task Says | Status | +| --- | --- | --- | --- | --- | +| T1 | Main-process deep module (`dir-remover`) | unit | unit | ✅ OK | +| T2 | Same module, real-fs hazards | unit | unit | ✅ OK | +| T3 | Extracted pure helper (porcelain parsing) | unit | unit | ✅ OK | +| T4 | Main-process deep module (`WorktreeManager`) | unit | unit | ✅ OK | +| T5 | Shared types + renderer component | none (build gate) + none (smoke) | none | ✅ OK | +| T6 | Main-process deep module (`SessionManager`) | unit | unit | ✅ OK | +| T7 | Smoke script | manual only | none (manual) | ✅ OK | +| T8 | Docs | — | none | ✅ OK | +| T9 | Extracted pure helper + `WorktreeManager` | unit | unit | ✅ OK | +| T10 | Main-process deep module + IPC wiring | unit (module) / none (wiring) | unit | ✅ OK | +| T11 | Renderer components | none (CDP smoke + visual) | none | ✅ OK | + +No violations. T5 and T11 carry `Tests: none` **only** because the matrix assigns "none" to shared types, +IPC wiring and renderer components — not because their tests were deferred; their behavior is covered by +T4/T10 unit tests below and by the T7 smoke above. + +**Scope of the three validation tables:** rows T9–T11 describe the deferred follow-up work and are kept for +that PR to lift verbatim. This branch's approval covers T1–T8 only. + +--- + +## Fix round 1 (from `validation.md`, Verifier round 1 — both gaps are test-strength, not defects) + +The Verifier returned FAIL with **2 surviving mutants** (M6, M13). Both survivors were *unasserted +outcomes*, not wrong behavior: the implementation is correct on every path examined, so **no production +code changed in this round**. Each fix is proved by re-running the exact mutation the Verifier used and +observing the new assertion fail. + +### F1: Assert the leftover payload the renderer branches on + +**Gap**: WRFT-04 AC 3c — `RemoveWorktreeResult.leftover` was never asserted. The three `leftover:` +occurrences in `worktree-manager.test.ts` (`:844`, `:867`, `:882`) are fixture **inputs** handed to +`spyDeleter`, never expectations on the value `removeWorktree` returns. Mutation **M6** — dropping +`leftover` from the failure result at `worktree-manager.ts:335-339` — left all **80 tests green**. This is +exactly the payload/conjunction trap: the value goes in and nothing checks it comes back out. +`WorktreeDetail.tsx:135` branches on `result.leftover`, so losing it silently degrades the UI from the +structured two-row block (WRFT-06 AC 1/AC 4) to the flat error line. + +**Where**: `src/main/worktree-manager.test.ts` (test-only; additions only, nothing weakened or removed) +**Requirement**: WRFT-04 AC 3 + +**Fix**: + +- `:852` — `expect(failed.leftover).toEqual({ blockedPath: blocked, remaining: 3 })` in the give-up test +- `:880-882` — `expect(result.leftover).toEqual({ blockedPath: blocked, remaining: 3 })` plus a per-field + check on `.blockedPath` and `.remaining` in the failure-message test (message **and** payload) +- `:936` — `expect(result.leftover).toBeUndefined()` on the locked-guard refusal: `shared/worktrees.ts:110-116` + documents "never on a guard refusal", and that half was unasserted too + +**Mutation evidence (M6 — `leftover: { blockedPath, remaining }` removed from the returned object)**: + +``` +FAIL src/main/worktree-manager.test.ts > removeWorktree > never invokes git and keeps the + worktree registered when deletion gives up +FAIL src/main/worktree-manager.test.ts > removeWorktree > names the blocked path, the remaining + count and the retry in the failure message +AssertionError: expected undefined to deeply equal { …(2) } + - Expected: { "blockedPath": "…\repo-feature-x\a.txt", "remaining": 3 } + + Received: undefined +Tests 2 failed | 18 passed | 60 skipped (80) +``` + +Before: `80 passed (80)` — **survived**. After: **2 failed — killed**. Production file restored from a +byte-identical backup (`git diff src/main/worktree-manager.ts` empty) before the gate and the commit. + +**Test count**: 564 → 564 (assertions added to existing tests; no new test cases, no deletions) +**Tests**: unit +**Gate**: full — `npm run typecheck && npm run lint && npm test` +**Commit**: `test(worktree): assert the leftover payload the renderer branches on` + +--- + +### F2: Pin the recursive leftover count against real fs + +**Gap**: WRFT-04 AC 3e — the real recursive `remaining` count was unpinned. The exact-count assertions at +`dir-remover.test.ts:148` / `:156` drive the **injected fake** `readEntries`, so they cannot see a change to +the real-fs wiring; `:323`'s `toBeGreaterThanOrEqual(1)` was the only real-filesystem check and is satisfied +by any non-zero count. Mutation **M13** — `readdir(path, { recursive: true })` → `readdir(path)` — left all +**15 tests green**, so a wrong-but-plausible number could reach the user's error message undetected. + +**Where**: `src/main/dir-remover.test.ts` (test-only; the `:323` assertion is kept, the new test sits +alongside it) +**Requirement**: WRFT-04 AC 3 + +**Fix**: a new real-fs test, `counts the leftovers recursively, not just the direct children of the root` +(`:328-348`), asserting `expect(result.leftover).toEqual({ blockedPath: held, remaining: 3 })`. + +The fixture is built so the two readings **cannot coincide**: the tree is directories only +(`wt/keep/a/b`), so a failed attempt deletes nothing and the residue is deterministic, and the external +holder's cwd sits three levels down. A recursive read reports **3** (`keep`, `keep\a`, `keep\a\b`) where a +non-recursive read of the root reports **1**. Verified out-of-band with a standalone probe before the +literal was written: `{ code: 'EBUSY', pathIsHeld: true, recCount: 3, topCount: 1 }`. + +**Mutation evidence (M13 — `readEntries: (path) => readdir(path)`)**: + +``` +FAIL src/main/dir-remover.test.ts > removeDirTree against the real filesystem > counts the + leftovers recursively, not just the direct children of the root +AssertionError: expected { …(2) } to deeply equal { …(2) } + { "blockedPath": "…\wt\keep\a\b", + - "remaining": 3, + + "remaining": 1, } +Tests 1 failed | 15 passed (16) +``` + +Before: `15 passed (15)` — **survived**. After: **1 failed — killed**. Production file restored from a +byte-identical backup (`git diff src/main/dir-remover.ts` empty) before the gate and the commit. + +**Test count**: 564 → 565 (+1 test, +0 files, zero deletions) +**Tests**: unit +**Gate**: full — `npm run typecheck && npm run lint && npm test` +**Commit**: `test(worktree): pin the recursive leftover count against real fs` + +--- + +**Not done in this round, deliberately:** Verifier Fix 3 (amend WRFT-04 AC 3 to say the count is +*recursive*, spec-precision gap P2) touches `spec.md`, which is outside this round's scope. F2's fixture +makes the recursive reading the only one that passes, so the ambiguity is now pinned by test even though +the prose still allows both readings. WRFT-06 remains blocked on the owner's live smoke run. + +--- + +## Fix round 2 (from `validation.md`, Verifier round 2 — again both gaps are test-strength) + +Round 1's two survivors were confirmed killed, but two **new** adversarial probes survived (N3, N6). Both +are the same family one step narrower: each fix had been shaped around the single mutation that was known, +so a neighbouring wrong reading still passed. No production code changed in this round either. + +### F3: Pin the leftover count against a mixed file and directory residue + +**Gap**: WRFT-04 AC 3b — the amended AC says `remaining` is the recursive count of **every entry**, but F2's +fixture (`wt/keep/a/b`) is *directories only* by construction, so a directories-only count reads the same 3 +and is indistinguishable from it. Mutation **N3** — `readEntries` filtered to `isDirectory()` — left all +**16 tests green**. In the spec's own headline residue (`sub/`, `sub/deep.txt`, `untracked.txt`) that reports +**1** instead of 3, in exactly the scenario the problem statement describes. + +**Where**: `src/main/dir-remover.test.ts` (test-only; the `:328-348` test and the `:323` `>= 1` assertion +are both kept, the new test sits alongside them) +**Requirement**: WRFT-04 AC 3 + +**Fix**: a new real-fs test, `counts every leftover entry, files as well as directories`, asserting +`expect(result.leftover).toEqual({ blockedPath: held, remaining: 3 })`. + +The fixture separates **four** readings at once rather than two. The tree contains nothing but the locked +chain `wt/keep/a/held.txt` — no sibling entry, so no traversal-order dependence — and the holder is an +*external* pwsh process opening `held.txt` with `FileShare.None` (a cwd holder does not protect the files +beside it, and Node's own handles do not block deletion at all: libuv sets `FILE_SHARE_DELETE`). The residue +is exactly `keep`, `keep\a`, `keep\a\held.txt`: + +| Reading of `remaining` | Value | +| --- | --- | +| recursive, every entry (**the spec's**) | **3** | +| recursive, directories only (N3) | 2 | +| recursive, files only | 1 | +| top level only (M13) | 1 | + +**Mutation evidence** — four variants run against the restored file, one at a time: + +``` +N3 readEntries filtered to isDirectory() → 1 failed | 16 passed (17) - 3 / + 2 + readEntries filtered to isFile() → 2 failed | 15 passed (17) - 3 / + 1 + readEntries + 1 phantom entry (off-by-one) → 2 failed | 15 passed (17) - 3 / + 4 + readdir(path) (non-recursive, M13) → 2 failed | 15 passed (17) - 3 / + 1 +``` + +N3 before: `16 passed (16)` — **survived**. After: **killed**, and so are the files-only, off-by-one and +non-recursive variants. `dir-remover.test.ts`'s `afterEach` cleanup gained `maxRetries`/`retryDelay` on its +`rmSync`, because Windows releases a killed holder's file handle asynchronously and the temp cleanup would +otherwise fail EPERM on the very file the test held. Production file restored from a byte-identical backup +(`git diff src/main/dir-remover.ts` empty) before the gate and the commit. + +**Test count**: 565 → 566 (+1 test, +0 files, zero deletions) +**Tests**: unit +**Gate**: full — `npm run typecheck && npm run lint && npm test` +**Commit**: `test(worktree): pin the leftover count against a mixed file and directory residue` + +--- + +### F4: Assert that no guard refusal reports a leftover + +**Gap**: WRFT-04 AC 3e — the amended AC names **four** guards (primary / unregistered / locked / dirty) that +must refuse without a `leftover`, but F1 added `expect(result.leftover).toBeUndefined()` to the *locked* +test only; `toBeUndefined` on `leftover` occurred exactly once in the file. Mutation **N6** — a +`leftover: { blockedPath, remaining: 0 }` on the primary-checkout guard's return — left all **80 tests +green**. It is not cosmetic: `WorktreeDetail.tsx:334` branches on `removeLeftover` to render the +structured "…still registered, so you can retry" block, so a spurious payload tells the user to retry a +refusal that can never succeed. + +**Where**: `src/main/worktree-manager.test.ts` (test-only, three added assertions) +**Requirement**: WRFT-04 AC 3 + +**Fix**: the same one-line assertion on the three unasserted guards — the dirty-without-force test, the +primary-checkout test and the unregistered-path test — leaving the locked one from F1 in place. All four +refusal paths now pin the absence. + +**Mutation evidence** — all three guard returns given a stray `leftover` in one run: + +``` +FAIL refuses a dirty worktree and leaves it intact +FAIL refuses the repo's primary checkout ← N6 +FAIL refuses a path that is not a registered worktree of this repo and deletes nothing +AssertionError: expected { …(2) } to be undefined (×3) +Tests 3 failed | 77 passed (80) +``` + +N6 alone, run first and on its own: `1 failed | 79 passed (80)` — before the fix it was `80 passed (80)`, +**survived**. Each of the three new assertions is therefore load-bearing, not just the one the Verifier +probed. Production file restored from a byte-identical backup (`git diff src/main/worktree-manager.ts` +empty) before the gate and the commit. + +**Test count**: 566 → 566 (assertions added to existing tests, so the count does not move; zero deletions) +**Tests**: unit +**Gate**: full — `npm run typecheck && npm run lint && npm test` +**Commit**: `test(worktree): assert no guard refusal reports a leftover` diff --git a/.specs/features/worktree-removal-fault-tolerance/validation.md b/.specs/features/worktree-removal-fault-tolerance/validation.md new file mode 100644 index 0000000..5beb45d --- /dev/null +++ b/.specs/features/worktree-removal-fault-tolerance/validation.md @@ -0,0 +1,457 @@ +# Worktree Removal Fault Tolerance Validation — Round 3 (final) + +**Date**: 2026-07-30 +**Spec**: `.specs/features/worktree-removal-fault-tolerance/spec.md` (as amended by `6f3af8a`) +**Diff range**: `54cf725..HEAD` (branch `feature/worktree-removal-fault-tolerance`, 15 commits) +**New since round 2**: `1abe8aa` (F3 — mixed file/directory leftover fixture), `45c27d5` (F4 — guard +`leftover` absence on all four refusal paths) +**Verifier**: independent sub-agent (author ≠ verifier), evidence-or-zero, re-derived from `spec.md`. +The Verifier wrote none of this code and re-ran every mutation itself; the fix worker's transcript was +not relied on anywhere in this report. +**Scope**: WRFT-01 … WRFT-06. **WRFT-07 is out of scope** — deferred to a follow-up PR by owner +decision (AD-014); its absence is not assessed as a gap. + +**Verdict**: ✅ **PASS** — both round-2 survivors (N3, N6) are genuinely killed, verified independently +and clause by clause. Three mutants survived this round; all three are **non-blocking** (one contrived, +one spec-undefined, one cosmetic-wording), and none corresponds to a plausible wrong implementation that +would change user-visible behaviour. **Nothing here blocks merging.** WRFT-06 remains unverified pending +the owner's live smoke — an unchanged caveat, not a finding. + +This was the third and final permitted iteration. The residual items below are reported for the owner's +decision, not looped back. + +--- + +## Findings across all three rounds — full history and resolution status + +| # | Raised | Finding | Status now | Evidence | +| --- | --- | --- | --- | --- | +| **M6** | R1 | `RemoveWorktreeResult.leftover` never asserted (WRFT-04 AC 3d) | ✅ **CLOSED** in R2 | R2 re-run: `2 failed \| 78 passed` on `:858`, `:886` | +| **M13** | R1 | real recursive `remaining` unpinned (WRFT-04 AC 3b) | ✅ **CLOSED** in R2 | R3 re-run of the non-recursive mutant: **2 failed \| 15 passed (17)**, `:370` and `:395` both `- 3 / + 1` | +| **P1** | R1 | WRFT-01 AC 4/5 quote message literals; tests pin distinctive fragments only | ⏳ **OPEN — non-blocking** (stance unchanged, now *evidenced*) | R3 probe: gutting both guard messages to bare fragments left **80 passed (80)**. Behaviour (refuse + delete nothing) is fully pinned; only wording is loose | +| **P2** | R1 | spec did not say whether `remaining` is recursive | ✅ **CLOSED** in R2 by `6f3af8a` | `spec.md:280-281` now says "the **recursive** count of every entry still present anywhere under the worktree root" | +| **P3** | R1 | WRFT-05 AC 3 has no observable outcome distinct from WRFT-04 AC 2 | ⏳ **OPEN — non-blocking** | `spec.md:309-310` unchanged. Coverage note C1 stands | +| **C1** | R1 | WRFT-05 AC 3 covered only via the fake-deleter retry test; spec finding F's "holder exits mid-loop" unreplicated | ⏳ **OPEN — non-blocking** | Unchanged | +| **C2** | R1 | WRFT-03 asserted one layer below `removeWorktree`, bridged by the default-deps test | ⏳ **OPEN — informational** | Bridge at `worktree-manager.test.ts:699-705` | +| **N3** | R2 | `remaining` counting only **directories** passed the whole suite (WRFT-04 AC 3b) | ✅ **CLOSED** by `1abe8aa` | R3 independent re-run: **1 failed \| 16 passed (17)** — `dir-remover.test.ts:395`, `- "remaining": 3 / + "remaining": 2` | +| **N6** | R2 | guard-refusal `leftover` absence asserted on 1 of 4 guards (WRFT-04 AC 3e) | ✅ **CLOSED** by `45c27d5` | R3: each guard mutated **separately**; all three new assertions are individually load-bearing (see sensor table) | +| **C3** | R2 | `dir-remover.test.ts:348`'s `toBeGreaterThanOrEqual(1)` "discriminates nothing beyond non-zero" | ✅ **SUPERSEDED** in R3 | Probe O3 shows `:348` *does* kill a `remaining: 0` regression (`expected 0 to be greater than or equal to 1`). It is weak, not inert | +| **O4 / P4** | **R3** | `blockedPath` may name the **first** failing attempt's entry rather than the **last**; the suite cannot tell, and the spec does not say which | ⏳ **OPEN — non-blocking (new)** | R3 probe: **17 passed (17)** — survived. See §Survivor analysis | +| **O5** | **R3** | a guard `leftover` conditioned on `force: true` slips past the three force-path guard tests | ⏳ **OPEN — non-blocking (new)** | R3 probe: **80 passed (80)** — survived. See §Survivor analysis | +| — | R1 | **WRFT-06 has no executed evidence** (CDP smoke needs the owner's live session) | ⏳ **UNCHANGED CAVEAT** — not a finding | See WRFT-06 section | + +**Summary: 12 findings raised across three rounds — 6 closed (M6, M13, N3, N6, P2, C3), 6 open +(P1, P3, C1, C2, O4/P4, O5), all six non-blocking — plus 1 unchanged caveat (WRFT-06).** + +--- + +## Task Completion + +| Task | Status | Notes | +| ---- | ------ | ----- | +| T0–T8 | ✅ Done | Unchanged from round 1 | +| F1 `124340c` | ✅ Done | Test-only, additions only | +| F2 `5aafb90` | ✅ Done | Test-only, one new real-fs test | +| Fix 3 `6f3af8a` | ✅ Done | Spec-only: WRFT-04 AC 3 amended | +| **Fix 4 `1abe8aa`** | ✅ **Done** | Test-only. `dir-remover.test.ts` **+52 / −1**; the single deletion is the `afterEach` `rmSync` line, replaced by the retrying form — no assertion removed | +| **Fix 5 `45c27d5`** | ✅ **Done** | Test-only. `worktree-manager.test.ts` **+8 / −0** — pure additions | +| T9–T11 | ⏸ Deferred | WRFT-07, follow-up PR (AD-014) — out of scope | + +**No production line changed across any of the four fixes** — independently confirmed: +`git diff --name-only dcc50dc..HEAD` touches only `*.test.ts`, `tasks.md` and `.specs/`. + +--- + +## Spec-Anchored Acceptance Criteria (re-derived against the current spec text) + +Evidence-or-zero: every criterion below carries a `file:line` + assertion expression, or it is counted +as NOT covered. + +### WRFT-01 — Delete-then-deregister with pre-flight guards (`spec.md:203-216`) + +| AC | Spec-defined outcome | `file:line` + assertion | Result | +| --- | --- | --- | --- | +| 1 | delete first, then `git worktree remove`; `{ ok: true }`, folder gone, entry gone | `worktree-manager.test.ts:839` — `expect(seen).toEqual({ registered: true, present: true })` (the deleter observes git as *not yet run*); `:840-842`; `:702-704` | ✅ PASS | +| 2 | unregistered path → refuse, delete nothing | `:913` `/not a registered worktree of this repo/i`; `:916` `expect(deleter.calls).toEqual([])`; `:917` `readFileSync(precious.txt) === 'keep me'` | ✅ PASS | +| 3 | `locked` line → refuse with git's reason, delete nothing, incl. under force | `:940-941`, `:945-947`; force: `:956-959`; bare-locked `''`: `:968-971`; parse: `:120`ff | ✅ PASS | +| 4 | primary checkout → unchanged DLWT-01 message, before any deletion, incl. force | `:735` `/primary checkout/i`, `:738`; casing/separators `:744-745`; force `:783-785` | ✅ PASS (precision note **P1**) | +| 5 | dirty + no force → unchanged `"N uncommitted change(s) …"` message, before any deletion | `:713` `.toContain('1 uncommitted change')`, `:718-719`; untracked counts as dirty `:727-728` | ✅ PASS (precision note **P1**) | +| 6 | `force` skips **only** the dirty check | `:753-754` (dirty force-removes); AC 2/3/4 still refuse under force: `:910`, `:954`, `:781` | ✅ PASS | + +### WRFT-02 — Never deregistered while files remain (`spec.md:231-239`) + +| AC | Spec-defined outcome | `file:line` + assertion | Result | +| --- | --- | --- | --- | +| 1 | git NOT invoked, still listed, `{ ok: false }` | `:855`, `:860` `expect(porcelainOf()).toContain(sibling…)`, `:861` `toHaveLength(2)`, `:859` folder present | ✅ PASS (**the central invariant** — mutation M1 re-killed) | +| 2 | retry after the holder ends → `{ ok: true }` | `:866-868`; real fs: `dir-remover.test.ts:412-413` | ✅ PASS | +| 3 | delete ok / git fails → `{ ok: false }` + git's first stderr line; retry heals | `:983-984` `/^fatal: /`, `:985`; retry `:989-991` | ✅ PASS | +| 4 | directory already absent → no-op delete, bookkeeping still runs, `{ ok: true }` | `:803-804`; unit: `dir-remover.test.ts:84-85` `expect(calls).toHaveLength(0)` | ✅ PASS | + +### WRFT-03 — Removal never destroys data outside the worktree (`spec.md:254-259`) + +| AC | Spec-defined outcome | `file:line` + assertion | Result | +| --- | --- | --- | --- | +| 1 | junction unlinked, target files intact with unchanged content | `dir-remover.test.ts:283-284` — `readFileSync(shared/precious.txt) === 'keep me'` and `nested/deep.txt === 'keep me too'`, after `:277` proves the junction was live | ✅ PASS (mutation M12 re-killed) | +| 2 | `{ ok: true }`, worktree folder incl. the junction entry gone | `:281-282` | ✅ PASS | +| 3 | dangling junction still unlinks | `:298-299` | ✅ PASS | +| — | reaches `removeWorktree` via default deps | bridged by `worktree-manager.test.ts:699-705` (note **C2**) | ⚠️ layer note | + +### WRFT-04 — Bounded retry with an actionable leftover report (`spec.md:274-288`) + +AC 3 carries five distinct clauses; each is assessed separately. + +| Clause | Spec-defined outcome | `file:line` + assertion | Result | +| --- | --- | --- | --- | +| **3a** `blockedPath` = the entry that could not be deleted | exact path, and specifically the **held entry**, not an ancestor | `dir-remover.test.ts:148`, `:156`, `:347`, `:370`, **`:395`** — `toEqual({ blockedPath: held, remaining: 3 })` where `held` is `wt\keep\a\held.txt`, a **file**; `worktree-manager.test.ts:858`, `:887` | ✅ **PASS** — probe **O2** (report the deepest *directory* instead) is killed by `:395` **alone** | +| **3b** `remaining` = the **recursive** count of **every entry** anywhere under the root | one number that separates all four readings | `dir-remover.test.ts:395` against a real-fs residue of exactly `keep`, `keep\a`, `keep\a\held.txt`. Measured this round: every-entry **3** ✅, directories-only **2** ❌, files-only **1** ❌, top-level-only **1** ❌, root-inclusive **4** ❌ | ✅ **PASS — round-2 gap N3 closed** | +| **3c** error message names `blockedPath`, states the count, says still-registered + retryable | four content assertions | `worktree-manager.test.ts:881-884`; singular form `:900-901` | ✅ PASS | +| **3d** `leftover` present **on the returned result itself** | the removal result carries it, not just the deleter | `:858` `toEqual({ blockedPath, remaining: 3 })`; `:886-888` `toEqual` + per-field | ✅ PASS (round-1 gap closed in R2) | +| **3e** guard refusals (primary / unregistered / locked / dirty) carry **no** `leftover` | absent on **all four** | dirty `:717`, primary `:737`, unregistered `:915`, locked `:944` — `expect(result.leftover).toBeUndefined()`. Each mutated **independently** this round; each kills exactly its own test | ✅ **PASS — round-2 gap N6 closed** (residual asymmetry **O5** on the *force* variants — non-blocking) | + +| Other WRFT-04 ACs | `file:line` | Result | +| --- | --- | --- | +| AC 1 — retryable set, 250 ms spacing, `maxRetries: 0` | `dir-remover.test.ts:97-98`, `:108` `toEqual([0, 250, 500])`, `:166-169`, `:176-177` | ✅ PASS | +| AC 2 — lock clears within budget → proceed | `:97` | ✅ PASS | +| AC 3 — budget literal 3000 ms | `:121-122` | ✅ PASS | +| AC 4 — non-retryable reported immediately, budget untouched | `:132-135` | ✅ PASS | +| AC 5 — any failure returns within 5000 ms | `:350` | ✅ PASS | + +### WRFT-05 — Terminated sessions really gone before deletion (`spec.md:305-312`) + +| AC | Spec-defined outcome | `file:line` + assertion | Result | +| --- | --- | --- | --- | +| 1 | PTY's real exit observed before deletion, or 3000 ms elapses | `session-manager.test.ts:374` `expect(settled).toBe(false)` at 2999 ms, `:376-378` resolves on `emitExit(0)` | ✅ PASS | +| 2 | no exit within the wait → proceed anyway | `:383` `expect(SESSION_EXIT_WAIT_MS).toBe(3000)` (literal, L-004), `:393-397` | ✅ PASS | +| 3 | late handle release absorbed by the WRFT-04 retry loop | no test with an outcome distinct from WRFT-04 AC 2 (`dir-remover.test.ts:97`) | ⚠️ **P3 / C1** — spec-precision, non-blocking | +| 4 | session-stop failure → removal aborted, error inline | `WorktreeDetail.tsx:173-179` — renderer convention-exempt (AD-004/AD-011) | ✅ convention-exempt | + +### WRFT-06 — The failure is visible in the UI and the row stays + +**Status unchanged from rounds 1 and 2 — a restatement, not a new finding.** The CDP smoke +(`scripts/smoke-remove.mjs:262-359`) is written, reads correctly and covers all four ACs by name, but +has **never been executed**: it needs the owner's live desktop session and is never run in CI. All four +ACs therefore have **no executed evidence**. The absence of *unit* tests here is convention-consistent +(`.specs/codebase/TESTING.md`, AD-004/AD-011) and correct. The producer→consumer wiring the smoke +exercises is statically present and pinned on the producer side (`shared/worktrees.ts:100-105`, +`:117`; `WorktreeDetail.tsx:135`, `:331-341`, `:320`). Discharged by the owner running +`node scripts/seed-smoke-remove.mjs` then `node scripts/smoke-remove.mjs`, plus the visual pass for +AC 4 — not by any change to this branch. + +| Requirement | ACs | Result | +| --- | --- | --- | +| WRFT-01 | 6/6 | ✅ PASS (precision note P1 on AC 4/5) | +| WRFT-02 | 4/4 | ✅ PASS | +| WRFT-03 | 3/3 | ✅ PASS (layer note C2) | +| WRFT-04 | 5/5 (AC 3: **5/5 clauses**) | ✅ **PASS — both round-2 clauses closed** | +| WRFT-05 | 4/4 | ✅ PASS (AC 4 convention-exempt; AC 3 thin — C1/P3) | +| WRFT-06 | 0/4 executed | ⏳ Unverified — owner's live smoke (unchanged caveat) | + +**22/22 assessed ACs (WRFT-01…05) match their spec-defined outcome; 4 unverified (WRFT-06); +3 spec-precision gaps open (P1, P3, P4) — all non-blocking.** + +--- + +## Payload / Conjunction Rule + +| Type | Field | Asserted on value/state? | +| --- | --- | --- | +| `DirRemovalResult` | `ok` / `code` | ✅ `:119-120`, `:132-133`, `:345-346` | +| `DirRemovalResult` | `leftover.blockedPath` | ✅ exact-value at `:148`, `:156`, `:347`, `:370`, `:395`; pinned to the **held file**, not an ancestor (probe O2) | +| `DirRemovalResult` | `leftover.remaining` | ✅ exact against the fake (`:148`, `:156`) **and** against real fs in two independent fixtures (`:370` dirs-only residue, `:395` mixed file+dir residue). Round-2 blind spot removed | +| `RemoveWorktreeResult` | `ok` / `error` | ✅ unchanged | +| `RemoveWorktreeResult` | **`leftover`** | ✅ asserted by value — `:858`, `:886-888` | +| `RemoveWorktreeResult` | `leftover` **absence** on refusal | ✅ **all four guards** — `:717`, `:737`, `:915`, `:944`; each independently load-bearing | + +--- + +## Discrimination Sensor — Round 3 + +Mutations were applied to **scratch state only**: each production file was copied to the session +scratchpad before any edit, mutated in place by an exact-single-occurrence patcher, exercised with a +targeted `npx vitest run`, then restored from the copy. `git diff --quiet` was asserted after **every** +restore (all 15 reported `RESTORED-CLEAN`). No test file was ever modified. + +### Re-run of round 2's two survivors — both genuinely killed + +| # | File:line | Mutation | R2 | R3 | Observed failure | +| --- | --- | --- | --- | --- | --- | +| **N3** | `dir-remover.ts:53` | `remaining` counts **only directories** (`withFileTypes` + `isDirectory()`) | ❌ Survived | ✅ **Killed** | `1 failed \| 16 passed (17)` — `dir-remover.test.ts:395`, `- "remaining": 3 / + "remaining": 2` | +| **N6a** | `worktree-manager.ts:292-294` | **primary-checkout** guard returns `leftover: { blockedPath, remaining: 0 }` | ❌ Survived | ✅ **Killed** | `1 failed \| 79 passed (80)` — `:737` `expected { …(2) } to be undefined` | + +### N3's sibling readings — the fixture separates all four, as claimed + +| # | Mutation of `dir-remover.ts:53` | Killed? | Observed | +| --- | --- | --- | --- | +| **N4** | files-only (`isFile()`) | ✅ Killed | `2 failed \| 15 passed (17)` — `:370` `- 3 / + 0`, `:395` `- 3 / + 1` | +| **N5 / O1** | `remaining` counts the **worktree root itself** (`+1`) | ✅ Killed | `2 failed \| 15 passed (17)` — `:370` and `:395` both `- 3 / + 4` | +| **M13** | non-recursive (`readdir(path)`) | ✅ Killed | `2 failed \| 15 passed (17)` — `:370` and `:395` both `- 3 / + 1` | + +Measured readings of the F3 fixture: **every-entry 3 / directories-only 2 / files-only 1 / +top-level-only 1 / root-inclusive 4** — four distinct wrong numbers, exactly as `1abe8aa` claimed. + +### N6's siblings — each guard mutated independently + +| # | Guard mutated (leftover attached to its refusal) | Killed? | Observed | +| --- | --- | --- | --- | +| **N6b** | **dirty** (`worktree-manager.ts:321-324`) | ✅ Killed | `1 failed \| 79 passed (80)` — `:717` | +| **N6c** | **unregistered** (`worktree-manager.ts:306`) | ✅ Killed | `1 failed \| 79 passed (80)` — `:915` | + +Each of F4's three new assertions kills **its own** test and nothing else — they are individually +load-bearing, not collectively lucky. + +### Regression re-check of earlier kills + +| # | File:line | Mutation | Result | Observed failure | +| --- | --- | --- | --- | --- | +| **M1** | `worktree-manager.ts:330` | deletion-failure path falls through and calls `git worktree remove` anyway (**the central invariant**) | ✅ Killed | `3 failed \| 77 passed (80)` — `:855` `expected true to be false`, plus `:881` and `:900` | +| **M12** | `dir-remover.ts:50-54` | real deleter **follows junctions** (`statSync` walk instead of `fs.rm`) | ✅ Killed | `2 failed \| 15 passed (17)` — `ENOENT … shared\precious.txt`: the shared target really was destroyed, the exact AD-013 loss | +| **GO** | `worktree-manager.ts:291` | **guard order** — deletion hoisted ahead of every guard | ✅ Killed | `10 failed \| 70 passed (80)` — `:717`, `:727`, `:737`, `:783`, `:839`, `:916`, `:929`, `:945`, `:958`, `:970`; `deleter.calls` non-empty on five refusal paths | + +### Fresh third-layer overfit probes (never run in rounds 1–2) + +| # | File:line | Mutation | Killed? | Observed | +| --- | --- | --- | --- | --- | +| **O2** | `dir-remover.ts:86` | `blockedPath` reported as the deepest **directory** instead of the held file — scoped so it fires *only* when the blocked entry really is an existing file, leaving the fake-deleter tests untouched | ✅ **Killed** | `1 failed \| 16 passed (17)` — `:395` only: `- ".../keep/a/held.txt" / + ".../keep/a"`. F3 pins the held **file** on its own terms | +| **O3** | `dir-remover.ts:53` | real `readEntries` unreadable → `countEntries`' catch fires → `leftover` **present but `remaining: 0`** | ✅ **Killed** | `3 failed \| 14 passed (17)` — `:348` `expected 0 to be greater than or equal to 1`, `:370` and `:395` `- 3 / + 0`. Supersedes note C3 | +| **O4** | `dir-remover.ts:70/82/88` | retry loop reports the **first** failing attempt's path instead of the **last** | ❌ **SURVIVED** | `17 passed (17)` | +| **O5** | `worktree-manager.ts:292-294` | primary guard leaks `leftover` **only when `force: true`** | ❌ **SURVIVED** | `80 passed (80)` | +| **P1-msg** | `worktree-manager.ts:293`, `:323` | both guard messages gutted to bare fragments (`'primary checkout'`; `"N uncommitted changes — sort it out."`) | ❌ **SURVIVED** | `80 passed (80)` | + +**Sensor depth**: P0-full. **Round-3 total: 15 mutations, 12 killed, 3 survived** — 2 re-runs of round-2 +survivors, 5 re-runs of earlier kills (N4, N5/O1, M13, M1, M12), 1 guard-order mutation, 2 new +per-guard isolations (N6b, N6c), and **5 fresh third-layer probes** (O2, O3, O4, O5, P1-msg). + +### Survivor analysis — all three non-blocking + +**O4 — first-vs-last failing path is unpinned, and the spec does not choose.** With `fs.rm`'s partial +progress, attempt *n+1* can get further than attempt *n*, so the first attempt's `err.path` may name an +entry that has since been deleted. The current implementation reports the **last** attempt's path, which +is the correct reading of AC 3 ("the path of the entry that could not be deleted"); the mutant reports +the first and no test notices, because in every fixture the same entry blocks every attempt. Killing it +would need a two-holder fixture releasing in sequence mid-loop — expensive and inherently racy on +Windows, the same reason C1 is open. Recorded as spec-precision gap **P4** (AC 3 does not say *which +attempt's* entry) plus a test-strength note. **Impact if it ever regressed:** a stale path in one error +message in a multi-lock scenario; the count, the registration invariant and the retry are unaffected. +**NON-BLOCKING.** + +**O5 — the three force-path guard tests carry no `leftover` assertion.** F4 correctly put +`expect(result.leftover).toBeUndefined()` on one test per guard, but the *force* variants — +`:780` primary-under-force, `:950` locked-under-force, `:962` bare-locked — assert only `ok`, `error` +and `deleter.calls`. (`:915` unregistered *does* run under force, so that guard is covered on both +paths.) A `leftover` conditioned on `opts.force` therefore slips through. WRFT-01 AC 6 says `force` +skips **only** the dirty check, so the two paths ought to be indistinguishable. No plausible +implementation branches a guard's payload on `force` — this is a contrived mutant, and the non-force +variant of every guard is pinned. **NON-BLOCKING**; a three-line fix if the owner wants symmetry. + +**P1-msg — the guard message literals are not pinned, only their distinctive fragments.** Now +demonstrated rather than assumed: replacing the primary message with the literal string +`'primary checkout'` and the dirty message's tail with `— sort it out.` leaves all 80 tests green. +The *behaviour* the ACs are about (refuse, delete nothing, before any deletion) is fully pinned by +`:738`, `:718-719`, `:916`, `:945`; only the wording is loose, and the spec itself only says "the +unchanged DLWT-01 message" for AC 4. **NON-BLOCKING** — unchanged stance from rounds 1 and 2, and the +right call: pinning full literals would trade a real regression signal for churn on every copy edit. + +--- + +## The `afterEach` change is inert with respect to assertions + +`1abe8aa` added `maxRetries: 10, retryDelay: 100` to the cleanup `rmSync` in `dir-remover.test.ts`'s +shared `afterEach` (`:198-206`). Verified inert on four independent grounds: + +1. **It is the commit's only deletion.** `git show --numstat 1abe8aa -- src/main/dir-remover.test.ts` + reports **+52 / −1**, and the single removed line is + `rmSync(root, { recursive: true, force: true })` — replaced by the retrying form. No assertion, no + test, no timeout was touched. +2. **The hook contains no assertion.** `afterEach` is exactly a `stopHolder` loop plus one `rmSync`; + there is no `expect()` in it, so it cannot pass or fail a criterion. +3. **It runs strictly after the test body.** It cannot alter state that an assertion already observed, + and `root` is a fresh `mkdtempSync` per test (`:194`), so there is no cross-test coupling it could + hide. +4. **Empirically demonstrated.** Eight mutations this round produced loud failures inside the very + tests whose residue this hook cleans (`:348`, `:370`, `:395`) — the hook demonstrably does not + swallow a failing expectation. Its only effect is the opposite one: it stops a *cleanup* EPERM, + caused by Windows releasing a killed holder's handle asynchronously, from turning a passing test + into a spurious failure. + +--- + +## Code Quality + +| Principle | Status | +| --- | --- | +| Minimum code | ✅ F3 and F4 are test-only; **no production line changed since round 1** (`dcc50dc..HEAD` = `*.test.ts` + `.specs/` only) | +| Surgical changes | ✅ F3 adds one test + a cleanup hardening; F4 adds three one-line assertions | +| No scope creep | ✅ WRFT-07 still correctly unbuilt | +| Only touched files required | ✅ two test files + `tasks.md`/`validation.md` | +| No test weakened or deleted | ✅ `+52/−1` and `+8/−0`; the one deletion is the `afterEach` cleanup line (see above). `:348`'s original `>= 1` retained alongside the two exact assertions — and probe O3 shows it still kills something | +| Matches existing patterns | ✅ real temp dirs, external holder processes, no `vi.mock`, explicit 30 s timeouts (L-005) | +| Tests map to ACs, non-shallow | ✅ both new tests carry their WRFT-04 AC 3 reference in a comment | +| Spec-anchored outcome check | ✅ 22/22 assessed ACs; 3 precision gaps flagged, not silently passed | +| Per-layer Coverage Expectation | ✅ domain logic 1:1 with ACs; renderer exempt by AD-004/AD-011 | +| No unclaimed tests | ✅ all 33 new tests map to a WRFT AC or a listed Edge Case | +| Documented guidelines followed | ✅ `.specs/codebase/TESTING.md`, AD-005, lessons L-001/L-004/L-005 | + +--- + +## Edge Cases (from `spec.md` §Edge Cases) + +- [x] Concurrent double-remove is idempotent — `dir-remover.test.ts:84-85`, `worktree-manager.test.ts:803-804` +- [x] Read-only files / nested repo with a `0444` object store — `dir-remover.test.ts:311-312`, `:330-331` +- [x] Repo gone or git unavailable → fail closed — `worktree-manager.test.ts:920-931` +- [x] Paths with spaces / non-ASCII — unchanged `execFile` discipline +- [ ] Detached-HEAD worktree removal — **no located test** (low behavioural risk, no evidence) +- [ ] Path > 260 chars — **no located test**; the spec marks it "not probed" under Assumptions, a knowing omission + +--- + +## Gate Check + +- **Gate command**: `npm run typecheck && npm run lint && npm test` (Full gate, `tasks.md` §Gate Check Commands) — **run by this Verifier**, exit code **0** +- **Result**: **566 passed, 0 failed, 0 skipped** — **40 test files** + - `typecheck`: clean (node + web projects) + - `lint`: **0 errors, 18 warnings** — all `prettier/prettier` in `scripts/fixtures/implement-ticket/workflow.ts`, `scripts/smoke-agent-config.mjs`, `scripts/smoke-agents.mjs`; none in any file this diff touches. Pre-existing + - `test`: 78.18 s wall (tests 132.32 s across workers) +- **Test count before feature**: 533 tests / 39 files (post-T0 baseline) +- **Round 1 → 2 → 3**: 564 → 565 → **566** tests / 40 files. F3 adds one real-fs test; F4 adds assertions to existing tests, so it moves no count +- **Delta vs. pre-feature**: **+33 tests, +1 file, zero deletions** — the DLWT/FRWT regression set is intact +- **Skipped tests**: none. **Failures**: none. +- **Manual gate**: `node scripts/smoke-remove.mjs` — **NOT RUN** (needs a live desktop session; owner-run) + +--- + +## Working Tree Check + +- Every one of the 15 mutations ran against a scratchpad copy + (`…/scratchpad/backup/{dir-remover,worktree-manager}.ts`), restored by `cp` immediately after the + run, with `git diff --quiet ` asserted after each restore — **all 15 reported `RESTORED-CLEAN`**. +- No test file was ever modified, by mutation or otherwise. +- Final `git status --short`: **`M .specs/features/worktree-removal-fault-tolerance/validation.md` + only** — the tree is clean apart from this report. + +--- + +## Fix Plans + +**No fix task is proposed.** All three surviving mutants are non-blocking (see §Survivor analysis), and +this was the final permitted iteration — they are handed to the owner as a decision, not looped back. +For completeness, the cheapest form of each, should the owner want it in a follow-up: + +### Optional 1 — `O5`: assert `leftover` absence on the force-path guard tests — Minor +Add `expect(result.leftover).toBeUndefined()` to `worktree-manager.test.ts:780` (primary under force), +`:950` (locked under force) and `:962` (bare-locked). Verify by re-running O5. + +### Optional 2 — `O4 / P4`: say which attempt's path `blockedPath` names — Minor +Amend `spec.md:279-280` to "the entry that blocked the **final** attempt", then either accept it as +spec-pinned-only or add a two-holder mid-loop fixture. The fixture is racy; the spec amendment alone is +probably the better trade. + +### Optional 3 — `P1`: pin the guard message literals — Cosmetic +Deliberately **not** recommended: full-literal assertions would churn on every copy edit while the +behavioural assertions already carry the regression signal. + +### Carried forward (not fix tasks) +- **P3 / C1** — WRFT-05 AC 3 has no observable outcome distinct from WRFT-04 AC 2; spec finding F's + "holder exits mid-loop" self-heal is unreplicated. Unchanged from rounds 1 and 2. +- **C2** — WRFT-03 asserted one layer below `removeWorktree`, bridged by the default-deps test. +- **WRFT-06** — owner's live smoke + visual pass. Unchanged caveat, not a finding. + +--- + +## Spec-Precision Gaps + +- **P1** — ⏳ open, **non-blocking**. WRFT-01 AC 4/5 name/quote the message literals; tests pin the + distinctive fragment. Now *evidenced* (P1-msg probe survived) rather than assumed, and the stance is + unchanged: the behaviour is pinned, only the wording is loose. +- **P2** — ✅ closed by `6f3af8a`. The amendment raised the bar to "every entry", and `1abe8aa` now + meets it. +- **P3** — ⏳ open, **non-blocking**. WRFT-05 AC 3 says the retry loop "SHALL absorb the delay" without + an observable outcome distinct from WRFT-04 AC 2. +- **P4** — ⏳ **new, non-blocking**. WRFT-04 AC 3 says `blockedPath` is "the path of the entry that + could not be deleted" without saying *which attempt's* entry, so first-vs-last is spec-undefined + (probe O4). + +## Coverage Notes + +- **C1** — unchanged: no test reproduces spec finding F's "holder exits at 600 ms → OK" row with a real + process exiting *mid-loop*. +- **C2** — unchanged: WRFT-03 is asserted one layer below `removeWorktree`, bridged by the default-deps + test at `worktree-manager.test.ts:699-705`. +- **C3** — ✅ **superseded**. Round 2 recorded `dir-remover.test.ts:348`'s `toBeGreaterThanOrEqual(1)` + as discriminating nothing; probe O3 shows it does kill a `remaining: 0` regression. It is weak, not + inert, and it is now redundant beside `:370`/`:395` rather than misleading. + +--- + +## Requirement Traceability Update + +| Requirement | R1 | R2 | R3 (final) | +| --- | --- | --- | --- | +| WRFT-01 | ✅ Verified | ✅ Verified | ✅ **Verified** (P1 non-blocking) | +| WRFT-02 | ✅ Verified | ✅ Verified | ✅ **Verified** — invariant re-confirmed (M1 killed) | +| WRFT-03 | ✅ Verified | ✅ Verified | ✅ **Verified** — junction safety re-confirmed (M12 killed) | +| WRFT-04 | ❌ Needs Fix (3c, 3e) | ❌ Needs Fix (3b, 3e) | ✅ **Verified** — all five clauses of AC 3 pinned | +| WRFT-05 | ✅ Verified | ✅ Verified | ✅ **Verified** (AC 4 convention-exempt; AC 3 thin — C1/P3) | +| WRFT-06 | ⏳ Unverified | ⏳ Unverified | ⏳ **Unverified** — blocked on the owner's live smoke + visual pass | +| WRFT-07 | ⏸ Deferred | ⏸ Deferred | ⏸ **Deferred** (AD-014) — out of scope, not assessed | + +--- + +## Summary + +**Overall**: ✅ **Ready to merge**, with WRFT-06 outstanding on the owner and three non-blocking +residuals recorded. + +**Findings**: 12 raised across three rounds — **6 closed** (M6, M13, N3, N6, P2, C3), **6 open** +(P1, P3, C1, C2, O4/P4, O5), **all six non-blocking**; 1 unchanged caveat (WRFT-06). +**Spec-anchored check**: **22/22** assessed ACs (WRFT-01…05) match their spec-defined outcome; +**3 spec-precision gaps** open (P1, P3, P4); **4 criteria unverified** pending the owner's smoke. +**Sensor**: **15 mutations, 12 killed, 3 survived** — including 5 fresh third-layer overfit probes. +**Gate**: 566 passed, 0 failed, 0 skipped; typecheck clean; lint 0 errors / 18 pre-existing warnings; exit 0. +**Working tree**: clean apart from this file. + +**What round 3 verified independently** — re-derived, not taken on the fix worker's word: +- **N3 is genuinely dead.** F3's fixture separates all four readings with real measured numbers, not a + claim: every-entry 3, directories-only 2, files-only 1, top-level 1, root-inclusive 4. Four distinct + mutations, four distinct failures. +- **N6 is genuinely dead, per guard.** Mutating the primary, dirty and unregistered refusals + *separately* fails exactly one test each — so all three of F4's assertions are load-bearing + individually, not collectively lucky. +- **The fixes bought more than they were asked for.** Probe O2 shows F3 also pins `blockedPath` to the + held **file** rather than its parent directory — a mutation nothing else in the suite catches — and + probe O3 shows the real `readdir` wiring is pinned against silently reporting zero. +- **Nothing regressed.** The central invariant (M1: never call git after a failed deletion), the + junction-safety guarantee (M12: the AD-013 data-loss path), and guard ordering (deletion hoisted above + the guards → 10 failures) are all still killed hard. +- **The `afterEach` hardening is inert.** One deleted line, no assertion in the hook, and eight + mutations this round still failed loudly inside the very tests it cleans up after. + +**What still survives** (ranked; none blocks merging): +1. **O5** — a guard `leftover` conditioned on `force: true` passes, because the three force-path guard + tests assert no `leftover`. Contrived mutant; every guard is pinned on its non-force path. + **NON-BLOCKING.** +2. **O4 / P4** — `blockedPath` naming the *first* rather than the *last* failing attempt passes; the + spec does not say which, and a discriminating fixture would be racy. **NON-BLOCKING.** +3. **P1** — the guard message literals can be gutted with the suite green; the behaviour they guard is + fully pinned. Unchanged stance, now evidenced. **NON-BLOCKING.** +4. **WRFT-06 still has no executed evidence** — unchanged caveat, not a finding, and dischargeable only + by the owner. + +**Next steps**: +1. Merge is not blocked by any item above. +2. Owner runs `node scripts/seed-smoke-remove.mjs` then `node scripts/smoke-remove.mjs` against a live + session to discharge WRFT-06, plus the visual pass for AC 4. Until then WRFT-06 stays ⏳ Unverified. +3. Optionally fold Optional 1 (three assertions) and Optional 2 (one spec sentence) into the WRFT-07 + follow-up PR. + +**Lesson distillation**: not performed by this Verifier — the round-3 assignment restricts its only +write to this file, and `scripts/lessons.py` mutates `lessons.json`/`LESSONS.md`. There **is** signal +worth recording, and the orchestrator should distill it: +- *A fixture built to kill one named mutant is blind to that mutant's siblings.* Rounds 1→2→3 walked + M13 → N3 → (O2, O4): each fix pinned exactly the reading it was shown and left the neighbouring + readings free. The durable countermeasure is the one F3 finally applied — **choose a fixture whose + single observed number separates every plausible reading at once**, and state the separation in the + test's comment so the next reader can check it. +- *A contract clause that names N call sites needs N assertions.* AC 3e named four guards; asserting one + looked like coverage for two rounds. Enumerate the clause's call sites and assert each. +- *Verify a "cleanup-only" test-harness change is inert before accepting it* — cheapest proof is that an + injected fault still fails loudly in the tests that hook serves. diff --git a/.specs/lessons.json b/.specs/lessons.json index 98e8178..0b09c3b 100644 --- a/.specs/lessons.json +++ b/.specs/lessons.json @@ -3,7 +3,7 @@ "promote_threshold": 2, "window_days": 45, "quarantine_threshold": 2, - "next_id": 6, + "next_id": 8, "lessons": [ { "id": "L-001", @@ -85,17 +85,55 @@ "text": "Before adding real-process or real-git tests, check whether existing suites already sit near the default per-test timeout: the extra parallel load alone can push them over it, turning a green gate red without any production change", "signal": "gate_fail", "scope": "testing", + "status": "confirmed", + "features": [ + "worktree-post-create-hook", + "worktree-removal-fault-tolerance" + ], + "recurrence": 2, + "harmful": 0, + "evidence": [ + "validation.md round-2 gate section; tree.test.ts / worktree-manager.test.ts timeouts (testing)", + "two full runs of untouched main red (2 failed, then 14 failed across 5 files), all duration overruns 11430-15557ms; fixed by vitest.config.ts testTimeout/hookTimeout 30000 in 34f8970 (testing)" + ], + "created": "2026-07-29T22:37:06Z", + "last_seen": "2026-07-31T12:27:40Z" + }, + { + "id": "L-006", + "key": "surviving_mutant::assert a returned payload field by its value not by the value you handed an injected fake a field that appears in the test only as a spy s input reads like coverage in review but a mutation dropping it from the real return still passes", + "text": "Assert a returned payload field by its value, not by the value you handed an injected fake: a field that appears in the test only as a spy's input reads like coverage in review, but a mutation dropping it from the real return still passes", + "signal": "surviving_mutant", + "scope": "testing", "status": "candidate", "features": [ - "worktree-post-create-hook" + "worktree-removal-fault-tolerance" ], "recurrence": 1, "harmful": 0, "evidence": [ - "validation.md round-2 gate section; tree.test.ts / worktree-manager.test.ts timeouts (testing)" + "round-1 mutant M6; worktree-manager.test.ts leftover: at :844/:867/:882 were spyDeleter inputs, not assertions - dropping the field from worktree-manager.ts:335-339 left all 80 tests green; closed by F1 124340c (testing)" ], - "created": "2026-07-29T22:37:06Z", - "last_seen": "2026-07-29T22:37:06Z" + "created": "2026-07-31T12:27:40Z", + "last_seen": "2026-07-31T12:27:40Z" + }, + { + "id": "L-007", + "key": "surviving_mutant::when writing a test to kill a specific surviving mutant check the fixture does not encode that mutant s own blind spot pick one whose readings differ under every wrong implementation not just the one you saw a directories only residue pinned the recursive count yet let a directories only count survive", + "text": "When writing a test to kill a specific surviving mutant, check the fixture does not encode that mutant's own blind spot: pick one whose readings differ under every wrong implementation, not just the one you saw. A directories-only residue pinned the recursive count yet let a directories-only count survive", + "signal": "surviving_mutant", + "scope": "testing", + "status": "candidate", + "features": [ + "worktree-removal-fault-tolerance" + ], + "recurrence": 1, + "harmful": 0, + "evidence": [ + "round-2 mutant N3 survived the round-1 fix F2 (dir-remover.test.ts:328-348 fixture wt/keep/a/b was directories-only); closed by F3 1abe8aa with a mixed chain giving 3/2/1/1 for every-entry/dirs-only/files-only/top-level (testing)" + ], + "created": "2026-07-31T12:27:40Z", + "last_seen": "2026-07-31T12:27:40Z" } ] } diff --git a/scripts/seed-smoke-remove.mjs b/scripts/seed-smoke-remove.mjs index 624d33c..bbdb38a 100644 --- a/scripts/seed-smoke-remove.mjs +++ b/scripts/seed-smoke-remove.mjs @@ -7,6 +7,10 @@ * api-feature-42/ clean worktree, branch feature/42 * api-chore-wip/ dirty worktree, branch chore/wip — mixed dirt: * a.txt modified, b.txt deleted, c.txt untracked + * api-lock-me/ clean worktree, branch lock/me, holding an empty sub/ + * for the WRFT-06 blocked-removal flow (the smoke parks + * a holder process's cwd there). Empty directories are + * invisible to git status, so the worktree stays clean. * * Usage: * node scripts/seed-smoke-remove.mjs [baseDir] @@ -32,6 +36,7 @@ const wsPath = join(base, 'wtm-smoke-seed') const repo = join(wsPath, 'api') const cleanWt = join(wsPath, 'api-feature-42') const dirtyWt = join(wsPath, 'api-chore-wip') +const lockWt = join(wsPath, 'api-lock-me') // Fresh on every run so the dirty state is deterministic. rmSync(wsPath, { recursive: true, force: true }) @@ -54,6 +59,14 @@ writeFileSync(join(dirtyWt, 'a.txt'), 'alpha edited\n') // modified rmSync(join(dirtyWt, 'b.txt')) // deleted writeFileSync(join(dirtyWt, 'c.txt'), 'scratch\n') // untracked +// Clean sibling worktree (lock/me) for the WRFT-06 blocked-removal flow. `sub` +// is the directory the smoke's holder process sits in: an external cwd is the +// only honest lock fixture (Node's own handles open with FILE_SHARE_DELETE and +// never block a delete). It stays empty so git still reports the worktree clean, +// which keeps the first Remove click on the direct path, not the confirm dialog. +git(repo, 'worktree', 'add', lockWt, '-b', 'lock/me') +mkdirSync(join(lockWt, 'sub')) + // Register the workspace folder in the app config. Mirrors WorkspaceRegistry.add: // id = lowercased absolute path, displayName = folder basename. const appData = process.env.APPDATA ?? join(homedir(), 'AppData', 'Roaming') @@ -83,6 +96,7 @@ console.log( dirtyWt, '(chore/wip — a.txt modified, b.txt deleted, c.txt untracked)' ) +console.log(' lock worktree: ', lockWt, '(lock/me — empty sub/ for the holder process)') console.log('Registered in: ', configPath) console.log('') console.log('Next: start the app with --remote-debugging-port=9222, then run') diff --git a/scripts/smoke-remove.mjs b/scripts/smoke-remove.mjs index f1d9885..2df3f06 100644 --- a/scripts/smoke-remove.mjs +++ b/scripts/smoke-remove.mjs @@ -1,16 +1,24 @@ /* CDP smoke for delete-worktree (DLWT-01..04) + force-remove-worktree - * (FRWT-01..04). Assumes the app is running with --remote-debugging-port=9222 + * (FRWT-01..04) + worktree-removal-fault-tolerance (WRFT-06). + * Assumes the app is running with --remote-debugging-port=9222 * and a seeded workspace named wtm-smoke-* containing repo `api` (branch main) - * plus a clean linked worktree `api-feature-42` (branch feature/42) and a dirty - * linked worktree `api-chore-wip` (branch chore/wip). For the fullest FRWT - * coverage, seed chore/wip with mixed dirt — a modified tracked file, an added - * untracked file, and a deleted tracked file — so the confirm dialog renders - * Modified/Added/Deleted rows; any non-empty dirt also passes. + * plus a clean linked worktree `api-feature-42` (branch feature/42), a dirty + * linked worktree `api-chore-wip` (branch chore/wip) and a clean linked + * worktree `api-lock-me` (branch lock/me) holding an empty `sub/`. For the + * fullest FRWT coverage, seed chore/wip with mixed dirt — a modified tracked + * file, an added untracked file, and a deleted tracked file — so the confirm + * dialog renders Modified/Added/Deleted rows; any non-empty dirt also passes. * Seed it with: node scripts/seed-smoke-remove.mjs (run before launching the app) * Run: node scripts/smoke-remove.mjs + * + * MANUAL ONLY — never CI (TESTING.md): this drives a live Electron app over CDP + * on a real desktop session, against real on-disk state that the run destroys. + * Every removal here is one-shot; re-seed before each run. */ +import { spawn } from 'node:child_process' import { existsSync } from 'fs' +import { join } from 'node:path' const PORT = 9222 @@ -69,6 +77,10 @@ const selectExpr = (branch) => `(async () => { } })()` +/* Git's porcelain paths and the path Node reports inside an fs error can differ + * in separators and case, so compare them the way the main process does. */ +const norm = (p) => p.replaceAll('/', '\\').replace(/\\+$/, '').toLowerCase() + const checks = [] function check(name, ok, detail = '') { checks.push({ name, ok }) @@ -247,6 +259,112 @@ check( ) check('dirty worktree folder gone from disk (FRWT-03)', !existsSync(dirtyWt.path)) +// WRFT-06: a removal blocked by a live lock names what blocked it, keeps the +// row (git is never asked to deregister anything), and the same button is a +// working retry once the holder is gone. The fixture is an external process +// whose cwd sits inside the worktree — the real agent-terminal case, and the +// only honest one, since Node's own handles never block a delete. +const lockWt = api.worktrees.find((w) => w.branch === 'lock/me') +check('seeded lock/me worktree present for the blocked-removal flow', Boolean(lockWt)) +const heldDir = join(lockWt.path, 'sub') + +const holder = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { + cwd: heldDir, + stdio: 'ignore' +}) +try { + await new Promise((r) => setTimeout(r, 400)) // measured settle before the lock is held + + // First Remove: the deleter exhausts its 3 s budget and gives up before git + // runs, so the Danger section gets the structured leftover block. + await evaluate(ws, selectExpr('lock/me')) + const blocked = await evaluate( + ws, + `(async () => { + document.querySelector('.detail-remove-btn').click() + await new Promise((r) => setTimeout(r, 6000)) // 3s retry budget + IPC round-trip + return { + note: document.querySelector('.detail-danger-leftover .detail-danger-note') + ?.textContent ?? null, + path: document.querySelector('.detail-danger-path')?.textContent ?? null, + disabled: document.querySelector('.detail-remove-btn')?.disabled ?? null + } + })()` + ) + check( + 'blocked removal names the blocked path inline (WRFT-06 AC 1)', + blocked.path !== null && norm(blocked.path) === norm(heldDir), + JSON.stringify({ shown: blocked.path, expected: heldDir }) + ) + check( + 'blocked removal reports what is left and that it stays registered (WRFT-06 AC 1)', + /\d+ items? still on disk/.test(blocked.note ?? '') && + /still registered/.test(blocked.note ?? ''), + JSON.stringify(blocked.note) + ) + check( + 'remove button is enabled again after the failure (WRFT-06 AC 3)', + blocked.disabled === false, + JSON.stringify(blocked.disabled) + ) + + // The row must survive a real tree refresh — the defect this feature fixes is + // that it used to vanish while the folder stayed on disk. + const survived = await evaluate( + ws, + `(async () => { + document.querySelector('.topbar-icon-btn').click() + await new Promise((r) => setTimeout(r, 1500)) + return [...document.querySelectorAll('.sidebar-worktree-branch')] + .some((b) => b.textContent === 'lock/me') + })()` + ) + check('blocked worktree is still listed after a tree refresh (WRFT-06 AC 1)', survived === true) + check('blocked worktree folder is still on disk (WRFT-02 AC 1)', existsSync(heldDir)) + + // Release the lock and retry from the same row — no restart, no cleanup. + await new Promise((resolve) => { + holder.once('exit', resolve) + holder.kill() + }) + await evaluate(ws, selectExpr('lock/me')) + const retried = await evaluate( + ws, + `(async () => { + document.querySelector('.detail-remove-btn').click() + await new Promise((r) => setTimeout(r, 1000)) + // The blocked attempt already deleted everything it could reach, the + // worktree's .git link included, so the row can read either clean (direct + // remove) or dirty (confirm dialog). Confirm it if it opened. + document.querySelector('.dialog-btn-danger')?.click() + await new Promise((r) => setTimeout(r, 2500)) + return { + rowGone: ![...document.querySelectorAll('.sidebar-worktree-branch')] + .some((b) => b.textContent === 'lock/me'), + toast: document.querySelector('.toast')?.textContent ?? null + } + })()` + ) + check( + 'retry after the holder exits removes the row (WRFT-06 AC 2)', + retried.rowGone === true, + JSON.stringify(retried) + ) + check( + 'retry toast names the branch (WRFT-06 AC 2)', + /Removed lock\/me/.test(retried.toast ?? ''), + JSON.stringify(retried.toast) + ) + check( + 'blocked worktree folder gone from disk after the retry (WRFT-06 AC 2)', + !existsSync(lockWt.path) + ) +} finally { + // A failed check above must not leave a node.exe parked in the worktree — + // it would block every later run and the seed's own rmSync. + holder.kill() +} + ws.close() const failed = checks.filter((c) => !c.ok).length console.log(`\n${checks.length - failed}/${checks.length} checks passed`) diff --git a/src/main/dir-remover.test.ts b/src/main/dir-remover.test.ts new file mode 100644 index 0000000..7ef483e --- /dev/null +++ b/src/main/dir-remover.test.ts @@ -0,0 +1,415 @@ +import { type ChildProcess, execFileSync, spawn } from 'node:child_process' +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + type RmOptions, + rmSync, + writeFileSync +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + DELETE_RETRY_BUDGET_MS, + DELETE_RETRY_INTERVAL_MS, + type DirRemoverDeps, + removeDirTree +} from './dir-remover' + +const ROOT = 'C:\\tmp\\wtm-repo-feature' + +/** A Node fs error the way `fs.rm` raises it: a `code` and the offending `path`. */ +function fsError(code: string, path?: string): NodeJS.ErrnoException { + const err = new Error(`${code}: operation not permitted`) as NodeJS.ErrnoException + err.code = code + if (path !== undefined) err.path = path + return err +} + +/** + * Hand-rolled fs seam (no `vi.mock`, per TESTING.md): `fail` decides what the + * n-th `rm` attempt throws, `entries` is what a recursive read of the root + * reports afterwards. + */ +function fakeFs(opts: { + fail?: (attempt: number) => NodeJS.ErrnoException | null + entries?: string[] + exists?: boolean +}): { + deps: DirRemoverDeps + attemptsAt: number[] + calls: Array<{ path: string; options: RmOptions }> +} { + const attemptsAt: number[] = [] + const calls: Array<{ path: string; options: RmOptions }> = [] + const deps: DirRemoverDeps = { + exists: () => opts.exists ?? true, + readEntries: async () => opts.entries ?? [], + rm: async (path, options) => { + attemptsAt.push(Date.now()) + calls.push({ path, options }) + const err = opts.fail?.(calls.length) ?? null + if (err !== null) throw err + } + } + return { deps, attemptsAt, calls } +} + +/** Drives the deleter's own sleeps on the fake clock and returns its result. */ +async function runWithTimers(promise: Promise): Promise { + await vi.runAllTimersAsync() + return promise +} + +describe('removeDirTree', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('reports success without deleting anything when the path does not exist', async () => { + // WRFT-02 AC 4: an already-absent directory is a no-op success, so the + // caller still goes on to clean git's bookkeeping. + const { deps, calls } = fakeFs({ exists: false }) + + const result = await runWithTimers(removeDirTree(ROOT, deps)) + + expect(result).toEqual({ ok: true }) + expect(calls).toHaveLength(0) + }) + + it('retries every lock-type error and succeeds once the lock clears', async () => { + // WRFT-04 AC 1 (the retryable set) + AC 2 (a transient lock resolves itself). + const lockCodes = ['EBUSY', 'EPERM', 'ENOTEMPTY', 'EACCES'] + const { deps, calls } = fakeFs({ + fail: (n) => (n <= lockCodes.length ? fsError(lockCodes[n - 1], ROOT) : null) + }) + + const result = await runWithTimers(removeDirTree(ROOT, deps)) + + expect(result).toEqual({ ok: true }) + expect(calls).toHaveLength(lockCodes.length + 1) + }) + + it('spaces retry attempts 250 ms apart', async () => { + // WRFT-04 AC 1 — literal, not the constant (lesson L-004). + const { deps, attemptsAt } = fakeFs({ fail: (n) => (n <= 2 ? fsError('EBUSY', ROOT) : null) }) + const startedAt = Date.now() + + await runWithTimers(removeDirTree(ROOT, deps)) + + expect(attemptsAt.map((at) => at - startedAt)).toEqual([0, 250, 500]) + }) + + it('gives up once the 3000 ms budget is exhausted', async () => { + // WRFT-04 AC 3 — literal budget; the last attempt sits on the deadline and + // nothing is attempted beyond it. + const { deps, attemptsAt } = fakeFs({ fail: () => fsError('EBUSY', ROOT) }) + const startedAt = Date.now() + + const result = await runWithTimers(removeDirTree(ROOT, deps)) + + expect(result.ok).toBe(false) + expect(result.code).toBe('EBUSY') + expect(Date.now() - startedAt).toBe(3000) + expect(attemptsAt.at(-1)! - startedAt).toBe(3000) + }) + + it('reports a non-retryable error immediately without consuming the budget', async () => { + // WRFT-04 AC 4: retrying e.g. EINVAL only burns the budget. + const { deps, attemptsAt } = fakeFs({ fail: () => fsError('EINVAL', ROOT) }) + const startedAt = Date.now() + + const result = await runWithTimers(removeDirTree(ROOT, deps)) + + expect(result.ok).toBe(false) + expect(result.code).toBe('EINVAL') + expect(attemptsAt).toHaveLength(1) + expect(Date.now() - startedAt).toBe(0) + }) + + it('names the blocking path and how many entries are still on disk', async () => { + // WRFT-04 AC 3: the leftover payload is what makes the failure actionable. + const blocked = `${ROOT}\\sub\\deep.txt` + const { deps } = fakeFs({ + fail: () => fsError('EBUSY', blocked), + entries: ['sub', 'sub\\deep.txt', 'untracked.txt'] + }) + + const result = await runWithTimers(removeDirTree(ROOT, deps)) + + expect(result.leftover).toEqual({ blockedPath: blocked, remaining: 3 }) + }) + + it('falls back to the removal root when the error carries no path', async () => { + const { deps } = fakeFs({ fail: () => fsError('EPERM'), entries: ['a.txt'] }) + + const result = await runWithTimers(removeDirTree(ROOT, deps)) + + expect(result.leftover).toEqual({ blockedPath: ROOT, remaining: 1 }) + }) + + it("deletes with maxRetries: 0 so Node's own retry ladder is never engaged", async () => { + // WRFT-04 AC 1: measured 21 599 ms for maxRetries: 5 against a locked + // directory, because Node retries at every level of the recursive walk. + const { deps, calls } = fakeFs({ fail: (n) => (n === 1 ? fsError('EBUSY', ROOT) : null) }) + + await runWithTimers(removeDirTree(ROOT, deps)) + + expect(calls).toEqual([ + { path: ROOT, options: { recursive: true, force: true, maxRetries: 0 } }, + { path: ROOT, options: { recursive: true, force: true, maxRetries: 0 } } + ]) + }) +}) + +describe('retry constants', () => { + it('are a 250 ms interval and a 3000 ms budget', () => { + // Pinned to literals so a mutation of either constant is caught (L-004). + expect(DELETE_RETRY_INTERVAL_MS).toBe(250) + expect(DELETE_RETRY_BUDGET_MS).toBe(3000) + }) +}) + +/** + * Real filesystem, real child processes, real timers — the hazards that decide + * whether deleting before deregistering is safe at all. These run against the + * default (real-fs) deps, so they also pin that those defaults are wired. + * Explicit per-test timeouts, per lesson L-005. + */ +describe('removeDirTree against the real filesystem', () => { + let root: string + let holders: ChildProcess[] + + beforeEach(() => { + // realpathSync.native so the paths compare byte-equal to what Node reports + // back in an error (tmpdir is a symlink/8.3 path on some machines). + root = realpathSync.native(mkdtempSync(join(tmpdir(), 'wtm-rm-'))) + holders = [] + }) + + afterEach(async () => { + // Kill first, and even when the test failed: a live child whose cwd sits + // inside the tree makes the cleanup below fail with EPERM on Windows. + for (const holder of holders) await stopHolder(holder) + // Windows releases a killed process's file handles asynchronously, so the + // very handle that made a test's residue survive can still be open here; + // retry rather than fail the cleanup with EPERM. + rmSync(root, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }) + }) + + const delay = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)) + + /** + * An external process holding `cwd` — the real agent-terminal case, and the + * only honest fixture: Node's own handles do not block deletion because libuv + * opens with FILE_SHARE_DELETE. + */ + async function holdCwd(cwd: string): Promise { + const holder = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { + cwd, + stdio: 'ignore' + }) + holders.push(holder) + await delay(400) // measured settle time before the lock is actually held + return holder + } + + /** + * An external process holding one *file* open with `FileShare.None`. A cwd + * holder protects only the directory it sits in — the files beside it are + * deleted — so this is the only fixture that leaves a surviving file behind, + * and it is the same shape the spec used to measure the original defect + * (finding A). + */ + async function holdFile(file: string): Promise { + const holder = spawn( + 'pwsh', + [ + '-NoProfile', + '-Command', + `$fs=[IO.File]::Open('${file}',[IO.FileMode]::Open,[IO.FileAccess]::Read,[IO.FileShare]::None); Start-Sleep 60` + ], + { stdio: 'ignore' } + ) + holders.push(holder) + await delay(2500) // measured: pwsh startup plus acquiring the handle + return holder + } + + function stopHolder(holder: ChildProcess): Promise { + if (holder.exitCode !== null || holder.signalCode !== null) return Promise.resolve() + return new Promise((resolve) => { + holder.once('exit', () => resolve()) + holder.kill() + }) + } + + function makeTree(...files: string[]): string { + const worktree = join(root, 'wt') + mkdirSync(worktree, { recursive: true }) + for (const file of files) { + mkdirSync(join(worktree, file, '..'), { recursive: true }) + writeFileSync(join(worktree, file), file, 'utf8') + } + return worktree + } + + it('unlinks a junction instead of deleting through it', async () => { + // WRFT-03 AC 1 + AC 2 — the AD-013 skills junction. This is the assertion + // that fails against a git-based deleter: `git worktree remove --force` + // recurses into the junction and empties the shared target, then reports + // success (measured, spec finding D). + const worktree = makeTree('a.txt') + const shared = join(root, 'shared') + mkdirSync(join(shared, 'nested'), { recursive: true }) + writeFileSync(join(shared, 'precious.txt'), 'keep me', 'utf8') + writeFileSync(join(shared, 'nested', 'deep.txt'), 'keep me too', 'utf8') + execFileSync('cmd', ['/c', 'mklink', '/J', join(worktree, '.skills'), shared]) + // The junction is live: a recursive walk really would reach the target. + expect(readFileSync(join(worktree, '.skills', 'precious.txt'), 'utf8')).toBe('keep me') + + const result = await removeDirTree(worktree) + + expect(result).toEqual({ ok: true }) + expect(existsSync(worktree)).toBe(false) + expect(readFileSync(join(shared, 'precious.txt'), 'utf8')).toBe('keep me') + expect(readFileSync(join(shared, 'nested', 'deep.txt'), 'utf8')).toBe('keep me too') + }, 30000) + + it('removes a worktree whose junction target is already gone', async () => { + // WRFT-03 AC 3: a dangling junction is unlinked like any other entry. + const worktree = makeTree('a.txt') + const shared = join(root, 'shared') + mkdirSync(shared) + writeFileSync(join(shared, 'doomed.txt'), 'bye', 'utf8') + execFileSync('cmd', ['/c', 'mklink', '/J', join(worktree, '.skills'), shared]) + rmSync(shared, { recursive: true, force: true }) + + const result = await removeDirTree(worktree) + + expect(result).toEqual({ ok: true }) + expect(existsSync(worktree)).toBe(false) + }, 30000) + + it('deletes read-only files', async () => { + // Spec Edge Cases: read-only content must not turn into a leftover. + const worktree = makeTree('sub/readonly.txt') + const readonly = join(worktree, 'sub', 'readonly.txt') + chmodSync(readonly, 0o444) + execFileSync('cmd', ['/c', 'attrib', '+R', readonly]) + + const result = await removeDirTree(worktree) + + expect(result).toEqual({ ok: true }) + expect(existsSync(worktree)).toBe(false) + }, 30000) + + it('deletes a nested repository with its read-only object store', async () => { + // Spec Edge Cases: git writes loose objects 0444, the classic rm-blocker. + const worktree = makeTree('a.txt') + const nested = join(worktree, 'vendor') + mkdirSync(nested) + const git = (...args: string[]): void => void execFileSync('git', args, { cwd: nested }) + git('init', '-b', 'main') + git('config', 'user.email', 'test@test.local') + git('config', 'user.name', 'Test') + writeFileSync(join(nested, 'v.txt'), 'vendored', 'utf8') + git('add', '.') + git('commit', '-m', 'init') + + const result = await removeDirTree(worktree) + + expect(result).toEqual({ ok: true }) + expect(existsSync(worktree)).toBe(false) + }, 30000) + + it('reports the blocked path when a live process holds a directory in the tree', async () => { + // WRFT-04 AC 3 + AC 5, and WRFT-02 AC 1: nothing is deregistered because the + // deletion never completes — the tree is still there to retry against. + const worktree = makeTree('sub/deep.txt', 'untracked.txt') + const held = join(worktree, 'sub') + await holdCwd(held) + + const startedAt = Date.now() + const result = await removeDirTree(worktree) + const elapsed = Date.now() - startedAt + + expect(result.ok).toBe(false) + expect(result.code).toBe('EBUSY') + expect(result.leftover?.blockedPath).toBe(held) + expect(result.leftover?.remaining).toBeGreaterThanOrEqual(1) + expect(existsSync(held)).toBe(true) + expect(elapsed).toBeLessThan(5000) + }, 30000) + + it('counts the leftovers recursively, not just the direct children of the root', async () => { + // WRFT-04 AC 3: `remaining` is the count of entries still present *under* the + // worktree root. The exact-count tests above drive the injected `readEntries` + // fake, so only a real-fs fixture can pin the real `readdir` wiring. + // + // The residue is deterministic by construction: the tree is directories only, + // so a failed attempt deletes nothing, and the holder's cwd is nested three + // levels down. A recursive read therefore reports 3 (`keep`, `keep\a`, + // `keep\a\b`) where a non-recursive read of the root would report 1. + const worktree = join(root, 'wt') + const held = join(worktree, 'keep', 'a', 'b') + mkdirSync(held, { recursive: true }) + await holdCwd(held) + + const result = await removeDirTree(worktree) + + expect(result.ok).toBe(false) + expect(result.leftover).toEqual({ blockedPath: held, remaining: 3 }) + // The residue really is nested — which is what makes 3 distinguishable from 1. + expect(existsSync(held)).toBe(true) + }, 30000) + + it('counts every leftover entry, files as well as directories', async () => { + // WRFT-04 AC 3: `remaining` is the recursive count of *every* entry still + // present under the worktree root — not only its directories. The test + // above cannot say so: its fixture is directories-only by construction, so + // a directories-only count reads the same 3. + // + // Here the tree is nothing but the locked chain, and the residue after the + // failed attempt is exactly `keep`, `keep\a`, `keep\a\held.txt`. That single + // number separates four readings at once: recursive-every-entry = 3, + // directories-only = 2, files-only = 1, top-level-only = 1. Nothing else + // exists under the root, so no sibling's deletion order can make it flaky. + const worktree = join(root, 'wt') + const held = join(worktree, 'keep', 'a', 'held.txt') + mkdirSync(join(worktree, 'keep', 'a'), { recursive: true }) + writeFileSync(held, 'held', 'utf8') + await holdFile(held) + + const result = await removeDirTree(worktree) + + expect(result.ok).toBe(false) + expect(result.leftover).toEqual({ blockedPath: held, remaining: 3 }) + // The surviving entry really is a file — which is what makes 3 distinct + // from the directories-only 2. + expect(existsSync(held)).toBe(true) + }, 30000) + + it('succeeds on a retry once the holding process is gone', async () => { + // WRFT-02 AC 2: the still-present tree is its own retry handle. + const worktree = makeTree('sub/deep.txt') + const holder = await holdCwd(join(worktree, 'sub')) + + const blocked = await removeDirTree(worktree) + expect(blocked.ok).toBe(false) + + await stopHolder(holder) + const retried = await removeDirTree(worktree) + + expect(retried).toEqual({ ok: true }) + expect(existsSync(worktree)).toBe(false) + }, 30000) +}) diff --git a/src/main/dir-remover.ts b/src/main/dir-remover.ts new file mode 100644 index 0000000..67f1db4 --- /dev/null +++ b/src/main/dir-remover.ts @@ -0,0 +1,105 @@ +import { existsSync, type RmOptions } from 'node:fs' +import { readdir, rm } from 'node:fs/promises' +import type { RemovalLeftover } from '../shared/worktrees' + +/** + * Junction-safe, deadline-bounded removal of a directory tree (WRFT-03, WRFT-04). + * + * Why the app deletes a worktree itself instead of letting `git worktree remove` + * do it: git for Windows treats a directory junction as an ordinary directory and + * recurses into it, emptying the junction's *target* while reporting success — + * which is exactly what the AD-013 skills junctions sit in. Node's `fs.rm` lstats + * the junction as a link and unlinks it, so the target survives untouched + * (measured; spec finding D). + */ + +/** Pause between two deletion attempts (WRFT-04 AC 1). */ +export const DELETE_RETRY_INTERVAL_MS = 250 + +/** Total wall-clock budget for waiting out a lock before giving up (WRFT-04 AC 3). */ +export const DELETE_RETRY_BUDGET_MS = 3000 + +/** + * The codes Windows raises while something still holds a handle inside the tree — + * the only ones worth waiting on. Anything else would fail the same way after the + * budget, so it is reported at once (WRFT-04 AC 4). + */ +const RETRYABLE_CODES = new Set(['EBUSY', 'EPERM', 'ENOTEMPTY', 'EACCES']) + +/** Outcome of a removal attempt; failures are returned, never thrown. */ +export interface DirRemovalResult { + ok: boolean + /** Node error code of the last failing attempt (EBUSY, EPERM, …). */ + code?: string + /** What the give-up left behind; absent when `ok`. */ + leftover?: RemovalLeftover +} + +/** + * The three filesystem touch points, injected with real-fs defaults so the retry + * policy is unit-testable without arranging a real lock (no `vi.mock` anywhere — + * TESTING.md). + */ +export interface DirRemoverDeps { + rm(path: string, options: RmOptions): Promise + exists(path: string): boolean + /** Every entry under the root, recursively — the count the failure reports. */ + readEntries(path: string): Promise +} + +const realFs: DirRemoverDeps = { + rm, + exists: existsSync, + readEntries: (path) => readdir(path, { recursive: true }) +} + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)) + +/** + * Deletes `path` and everything under it, retrying only lock-type failures and + * only until the budget runs out. An absent path is a success (WRFT-02 AC 4), so + * a caller can always follow up with its bookkeeping step. + */ +export async function removeDirTree( + path: string, + deps: DirRemoverDeps = realFs +): Promise { + if (!deps.exists(path)) return { ok: true } + + const startedAt = Date.now() + for (;;) { + try { + // `maxRetries: 0` is load-bearing, not a default — do not raise it. Node + // retries at *every* level of the recursive walk, so its ladder compounds: + // against a cwd-locked directory `maxRetries: 5, retryDelay: 200` measured + // 21 599 ms before failing, while a single attempt fails in 2 ms. All the + // waiting belongs to the loop below, where the budget is wall-clock honest. + await deps.rm(path, { recursive: true, force: true, maxRetries: 0 }) + return { ok: true } + } catch (err) { + const { code, path: blockedPath } = err as NodeJS.ErrnoException + const retryable = code !== undefined && RETRYABLE_CODES.has(code) + if (!retryable || Date.now() - startedAt >= DELETE_RETRY_BUDGET_MS) { + return { + ok: false, + code, + leftover: { blockedPath: blockedPath ?? path, remaining: await countEntries(path, deps) } + } + } + await sleep(DELETE_RETRY_INTERVAL_MS) + } + } +} + +/** + * How much is still there, for the leftover report. The root can be unreadable + * for the very reason the deletion failed, and a report is not worth throwing + * over, so an unreadable root counts as zero. + */ +async function countEntries(path: string, deps: DirRemoverDeps): Promise { + try { + return (await deps.readEntries(path)).length + } catch { + return 0 + } +} diff --git a/src/main/hook-shell.test.ts b/src/main/hook-shell.test.ts index c429dd2..e8c24f8 100644 --- a/src/main/hook-shell.test.ts +++ b/src/main/hook-shell.test.ts @@ -97,7 +97,11 @@ describe('runHookShell', () => { const result = await runHookShell('ping -n 5 127.0.0.1', { cwd: tmpdir(), env, - timeoutMs: 500 + // 1500ms, not 500ms: under full-suite load ping had not emitted its first + // line before the kill, so the output-tail assertion below saw ''. `ping + // -n 5` still runs ~4s, so the command is still killed mid-flight — this + // widens the fixture's margin without weakening what is asserted. + timeoutMs: 1500 }) expect(result.timedOut).toBe(true) diff --git a/src/main/index.ts b/src/main/index.ts index 58d31b0..cd8c813 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -253,6 +253,10 @@ app.whenReady().then(() => { handle('sessions:spawn', ({ agentName, cwd, adhocCommand }) => sessions.spawn(agentName, cwd, adhocCommand) ) + // Returning the promise is load-bearing: ipcMain.handle awaits it, so the + // renderer's `sessions:stop` only resolves once the PTY has really exited + // (WRFT-05) — which is what lets a worktree removal start without racing + // handles the agent's children still hold. handle('sessions:stop', ({ id }) => sessions.stop(id)) handle('sessions:respawn', ({ id }) => sessions.respawn(id)) handle('sessions:rename', ({ id, title }) => sessions.rename(id, title)) diff --git a/src/main/session-manager.test.ts b/src/main/session-manager.test.ts index 4d8aaac..1414585 100644 --- a/src/main/session-manager.test.ts +++ b/src/main/session-manager.test.ts @@ -1,13 +1,13 @@ import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { SEEDED_AGENTS } from '../shared/agents' import type { PersistedSession } from '../shared/config' import { ConfigStore } from './config-store' import type { PtyHandle, PtyPort } from './pty-port' import type { SpawnPlan } from './spawn-plan' -import { SessionManager, type EmitFn } from './session-manager' +import { SessionManager, SESSION_EXIT_WAIT_MS, type EmitFn } from './session-manager' interface FakeHandle extends PtyHandle { plan: SpawnPlan @@ -80,6 +80,12 @@ afterEach(() => { dirs.length = 0 }) +// Only the stop-wait tests below install fake timers; restoring here keeps every +// other test on the real clock. +afterEach(() => { + vi.useRealTimers() +}) + function makeManager(opts: { fsExists?: (p: string) => boolean; seed?: PersistedSession[] } = {}): { manager: SessionManager config: ConfigStore @@ -346,4 +352,66 @@ describe('SessionManager', () => { const restored = makeManager({ seed }) expect(restored.manager.list()[0].lastOutput).toBeUndefined() }) + + // --- WRFT-05: stop resolves on the PTY's real exit --- + + it('stop finalizes at once but resolves only after the PTY has really exited', async () => { + vi.useFakeTimers() + const { manager, config, port } = makeManager() + const view = manager.spawn('Claude', CWD) + + let settled = false + const stopped = manager.stop(view.id).then(() => { + settled = true + }) + + // The status flip stays synchronous — every existing caller keeps working. + expect(port.handles[0].killed).toBe(true) + expect(manager.list()[0].status).toBe('stopped') + expect(config.get().sessions[0].status).toBe('stopped') + + await vi.advanceTimersByTimeAsync(2999) + expect(settled).toBe(false) // the kill alone is not "really gone" + + port.handles[0].emitExit(0) + await stopped + expect(settled).toBe(true) + }) + + it('stop resolves anyway once the 3000 ms wait elapses for a PTY that never exits', async () => { + vi.useFakeTimers() + expect(SESSION_EXIT_WAIT_MS).toBe(3000) // pin the literal, not the constant + const { manager, port } = makeManager() + const view = manager.spawn('Claude', CWD) + + let settled = false + const stopped = manager.stop(view.id).then(() => { + settled = true + }) + + await vi.advanceTimersByTimeAsync(2999) + expect(settled).toBe(false) + + await vi.advanceTimersByTimeAsync(1) + await stopped + expect(settled).toBe(true) + expect(port.handles[0].killed).toBe(true) // proceeded without an exit event + }) + + it('killAll stays synchronous so quit never stalls on a PTY that never exits', async () => { + vi.useFakeTimers() + const { manager, config, port } = makeManager() + manager.spawn('Claude', CWD) + manager.spawn('Codex', 'C:\\work\\other') + + // void, not a promise: awaiting it would add up to 3 s per session to quit. + expect(manager.killAll()).toBeUndefined() + + expect(port.handles.every((h) => h.killed)).toBe(true) + expect(manager.list().every((s) => s.status === 'stopped')).toBe(true) + expect(config.get().sessions.every((s) => s.status === 'stopped')).toBe(true) + + // Drain the two pending waits so nothing outlives the test. + await vi.advanceTimersByTimeAsync(SESSION_EXIT_WAIT_MS) + }) }) diff --git a/src/main/session-manager.ts b/src/main/session-manager.ts index ef82fea..0e8f8bb 100644 --- a/src/main/session-manager.ts +++ b/src/main/session-manager.ts @@ -21,11 +21,21 @@ export interface SessionManagerDeps { /** Stored on ad-hoc sessions in place of a registry agent name. */ const ADHOC_AGENT = 'Ad-hoc' +/** + * How long `stop` waits for the PTY's *real* exit before giving up and letting + * the caller proceed (WRFT-05 AC 1/2). A wedged child must not block a worktree + * removal forever — the deleter's own retry loop and leftover report cover the + * residue. + */ +export const SESSION_EXIT_WAIT_MS = 3000 + /** A session with a live PTY. Stopped/restored sessions live only in config. */ interface RunningSession { meta: PersistedSession handle: PtyHandle buffer: SessionRingBuffer + /** Resolves when the PTY's own onExit fires — what `stop` actually waits on. */ + exited: Promise } /** @@ -115,11 +125,40 @@ export class SessionManager { return this.#toView(meta) } - stop(id: string): void { + /** + * Kill the PTY and resolve once it has **really exited** (WRFT-05). Killing a + * shell does not kill its children, so a caller that deletes files the moment + * `stop` returns used to race handles that were still open — the removal then + * failed on a lock the app itself was holding. + * + * The status flip stays synchronous: `#finalize` runs before the first await, + * so every existing caller that reads `list()` right after `stop` still sees + * `stopped`. Only the returned promise is new, and it means "really gone". + */ + async stop(id: string): Promise { const session = this.#running.get(id) if (!session) return + // Captured before #finalize drops the Map entry. + const exited = session.exited session.handle.kill() this.#finalize(id) + let timer: ReturnType | undefined + try { + await Promise.race([ + exited, + new Promise((resolve) => { + timer = setTimeout(resolve, SESSION_EXIT_WAIT_MS) + // Unlike lesson L-003's grace timer — whose unref let real work be + // skipped when the process was free to exit — this timer only races a + // promise a live caller is already awaiting, so unref cannot skip + // anything. It just stops a PTY that never exits from pinning the + // event loop open. + timer.unref?.() + }) + ]) + } finally { + clearTimeout(timer) + } } respawn(id: string): SessionView { @@ -170,7 +209,13 @@ export class SessionManager { // stop() each session (not Map.clear()) so every status is finalized, // persisted, and emitted — otherwise config stays stale until the next // restart, observable on macOS where closing the last window doesn't quit. - for (const id of [...this.#running.keys()]) this.stop(id) + // + // Deliberately fire-and-forget: stop() now waits up to SESSION_EXIT_WAIT_MS + // for a real exit, and quit must not stall 3 s per session. Nothing is lost + // by not awaiting — the kill is issued and #finalize has already persisted + // every status synchronously before stop() suspends. Only the *removal* + // path needs the exit guarantee; quit does not. + for (const id of [...this.#running.keys()]) void this.stop(id) this.#activeId = null } @@ -192,8 +237,15 @@ export class SessionManager { buffer.append(data) if (this.#activeId === meta.id) this.deps.emit('session:data', { id: meta.id, data }) }) - handle.onExit(({ exitCode }) => this.#finalize(meta.id, exitCode)) - this.#running.set(meta.id, { meta: { ...meta, status: 'running' }, handle, buffer }) + let markExited = (): void => {} + const exited = new Promise((resolve) => { + markExited = resolve + }) + handle.onExit(({ exitCode }) => { + markExited() + this.#finalize(meta.id, exitCode) + }) + this.#running.set(meta.id, { meta: { ...meta, status: 'running' }, handle, buffer, exited }) } /** Idempotent transition to stopped: drop the Map entry, persist, push status. */ diff --git a/src/main/worktree-manager.test.ts b/src/main/worktree-manager.test.ts index 7555076..f2b8057 100644 --- a/src/main/worktree-manager.test.ts +++ b/src/main/worktree-manager.test.ts @@ -12,12 +12,14 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { sanitizeBranch, worktreeNameFor, worktreePathFor } from '../shared/worktrees' +import type { DirRemovalResult } from './dir-remover' import { changedFilesOf, createWorktree, GitError, listWorktrees, parseChangedFiles, + parsePorcelainBlocks, removeWorktree } from './worktree-manager' @@ -115,6 +117,66 @@ describe('listWorktrees', () => { }) }) +describe('parsePorcelainBlocks — locked line (WRFT-01 AC 3)', () => { + let root: string + let repo: string + let locked: string + let unlocked: string + + beforeEach(() => { + root = realpathSync.native(mkdtempSync(join(tmpdir(), 'wtm-lock-'))) + repo = join(root, 'repo') + mkdirSync(repo) + git(repo, 'init', '-b', 'main') + git(repo, 'config', 'user.email', 'test@test.local') + git(repo, 'config', 'user.name', 'Test') + writeFileSync(join(repo, 'a.txt'), 'one', 'utf8') + git(repo, 'add', '.') + git(repo, 'commit', '-m', 'init') + locked = join(root, 'repo-locked') + unlocked = join(root, 'repo-unlocked') + git(repo, 'worktree', 'add', locked, '-b', 'feature/locked') + git(repo, 'worktree', 'add', unlocked, '-b', 'feature/unlocked') + }) + + afterEach(() => { + // A git lock is bookkeeping only — it holds no OS handle, so the tree removes + // without unlocking first. + rmSync(root, { recursive: true, force: true }) + }) + + const porcelain = (): string => git(repo, 'worktree', 'list', '--porcelain') + + it("yields git's lock reason for a worktree locked with --reason", async () => { + git(repo, 'worktree', 'lock', '--reason', 'held for review', locked) + + const block = parsePorcelainBlocks(porcelain()).find((b) => b.path === locked) + + expect(block?.locked).toBe('held for review') + // Additive only: the fields listWorktrees reads are untouched (Done-when 2). + const listed = (await listWorktrees(repo)).find((w) => w.path === locked) + expect(listed).toMatchObject({ branch: 'feature/locked', isDefault: false }) + }) + + it('yields an empty reason for a bare locked line — not undefined', () => { + git(repo, 'worktree', 'lock', locked) + + const block = parsePorcelainBlocks(porcelain()).find((b) => b.path === locked) + + expect(block?.locked).toBe('') + expect(block?.locked).not.toBeUndefined() + }) + + it('leaves locked undefined for a worktree that is not locked', () => { + git(repo, 'worktree', 'lock', '--reason', 'held for review', locked) + + const blocks = parsePorcelainBlocks(porcelain()) + + expect(blocks.find((b) => b.path === unlocked)?.locked).toBeUndefined() + expect(blocks.find((b) => b.path === repo)?.locked).toBeUndefined() + }) +}) + describe('sanitizeBranch', () => { it.each([ ['feature/123', 'feature-123'], @@ -649,6 +711,10 @@ describe('removeWorktree', () => { expect(result.ok).toBe(false) expect(result.error).toContain('1 uncommitted change') + // WRFT-04 AC 3: a guard refusal carries no leftover — nothing was deleted, + // and WorktreeDetail branches on the field to offer a retry that this + // refusal can never satisfy. + expect(result.leftover).toBeUndefined() expect(existsSync(sibling)).toBe(true) expect(await listWorktrees(repo)).toHaveLength(2) }) @@ -667,6 +733,8 @@ describe('removeWorktree', () => { expect(result.ok).toBe(false) expect(result.error).toMatch(/primary checkout/i) + // WRFT-04 AC 3: a guard refusal carries no leftover (see the dirty case). + expect(result.leftover).toBeUndefined() expect(existsSync(repo)).toBe(true) }) @@ -735,4 +803,191 @@ describe('removeWorktree', () => { expect(result).toEqual({ ok: true }) expect(await listWorktrees(repo)).toHaveLength(1) }) + + // --- WRFT: delete-first ordering, pre-deletion guards, leftover reporting --- + + /** A deleter that records every call and never touches the disk. */ + const spyDeleter = ( + result: DirRemovalResult = { ok: true } + ): { removeDirTree: (p: string) => Promise; calls: string[] } => { + const calls: string[] = [] + return { + calls, + removeDirTree: async (p: string) => { + calls.push(p) + return result + } + } + } + + const porcelainOf = (): string => git(repo, 'worktree', 'list', '--porcelain') + + it('deletes the directory itself before git drops the bookkeeping', async () => { + // The deleter observes the world at deletion time: git must not have run yet. + const seen: { registered?: boolean; present?: boolean } = {} + const deleter = { + removeDirTree: async (p: string): Promise => { + seen.registered = porcelainOf().includes(sibling.replaceAll('\\', '/')) + seen.present = existsSync(p) + rmSync(p, { recursive: true, force: true }) + return { ok: true } + } + } + + const result = await removeWorktree(repo, sibling, {}, deleter) + + expect(seen).toEqual({ registered: true, present: true }) + expect(result).toEqual({ ok: true }) + expect(existsSync(sibling)).toBe(false) + expect(await listWorktrees(repo)).toHaveLength(1) + }) + + it('never invokes git and keeps the worktree registered when deletion gives up', async () => { + const blocked = join(sibling, 'a.txt') + const stuck = spyDeleter({ + ok: false, + code: 'EBUSY', + leftover: { blockedPath: blocked, remaining: 3 } + }) + + const failed = await removeWorktree(repo, sibling, {}, stuck) + + expect(failed.ok).toBe(false) + // WRFT-04 AC 3: the structured payload must come back out of removeWorktree, + // not merely go into the deleter — it is what WorktreeDetail branches on. + expect(failed.leftover).toEqual({ blockedPath: blocked, remaining: 3 }) + expect(existsSync(sibling)).toBe(true) + expect(porcelainOf()).toContain(sibling.replaceAll('\\', '/')) + expect(await listWorktrees(repo)).toHaveLength(2) + + // …and the still-registered worktree is itself the retry handle (WRFT-02 AC 2). + const retried = await removeWorktree(repo, sibling) + + expect(retried).toEqual({ ok: true }) + expect(existsSync(sibling)).toBe(false) + expect(await listWorktrees(repo)).toHaveLength(1) + }) + + it('names the blocked path, the remaining count and the retry in the failure message', async () => { + const blocked = join(sibling, 'sub', 'deep.txt') + const stuck = spyDeleter({ + ok: false, + code: 'EBUSY', + leftover: { blockedPath: blocked, remaining: 3 } + }) + + const result = await removeWorktree(repo, sibling, {}, stuck) + + expect(result.error).toContain(blocked) + expect(result.error).toContain('3 items still on disk') + expect(result.error).toMatch(/still registered/i) + expect(result.error).toMatch(/retry/i) + // WRFT-04 AC 3: message *and* payload — the renderer needs both fields by value. + expect(result.leftover).toEqual({ blockedPath: blocked, remaining: 3 }) + expect(result.leftover?.blockedPath).toBe(blocked) + expect(result.leftover?.remaining).toBe(3) + }) + + it('pluralizes a single leftover entry as "1 item"', async () => { + const stuck = spyDeleter({ + ok: false, + code: 'EPERM', + leftover: { blockedPath: sibling, remaining: 1 } + }) + + const result = await removeWorktree(repo, sibling, {}, stuck) + + expect(result.error).toContain('1 item still on disk') + expect(result.error).not.toContain('1 items') + }) + + it('refuses a path that is not a registered worktree of this repo and deletes nothing', async () => { + const stranger = join(root, 'not-a-worktree') + mkdirSync(stranger) + writeFileSync(join(stranger, 'precious.txt'), 'keep me', 'utf8') + const deleter = spyDeleter() + + const result = await removeWorktree(repo, stranger, { force: true }, deleter) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/not a registered worktree of this repo/i) + // WRFT-04 AC 3: a guard refusal carries no leftover (see the dirty case). + expect(result.leftover).toBeUndefined() + expect(deleter.calls).toEqual([]) + expect(readFileSync(join(stranger, 'precious.txt'), 'utf8')).toBe('keep me') + }) + + it('fails closed when git worktree list itself fails', async () => { + const notARepo = join(root, 'plain-folder') + mkdirSync(notARepo) + const deleter = spyDeleter() + + const result = await removeWorktree(notARepo, sibling, { force: true }, deleter) + + expect(result.ok).toBe(false) + expect(result.error).toBeTruthy() + expect(deleter.calls).toEqual([]) + expect(existsSync(sibling)).toBe(true) + }) + + it("refuses a locked worktree with git's lock reason and deletes nothing", async () => { + git(repo, 'worktree', 'lock', '--reason', 'held for review', sibling) + const deleter = spyDeleter() + + const result = await removeWorktree(repo, sibling, {}, deleter) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/locked/i) + expect(result.error).toContain('held for review') + // A guard refusal never carries a leftover: nothing was deleted, so there is + // nothing left over, and the renderer must fall back to the flat error line. + expect(result.leftover).toBeUndefined() + expect(deleter.calls).toEqual([]) + expect(existsSync(sibling)).toBe(true) + expect(await listWorktrees(repo)).toHaveLength(2) + }) + + it('refuses a locked worktree under force too', async () => { + git(repo, 'worktree', 'lock', '--reason', 'held for review', sibling) + const deleter = spyDeleter() + + const result = await removeWorktree(repo, sibling, { force: true }, deleter) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/locked/i) + expect(deleter.calls).toEqual([]) + expect(existsSync(sibling)).toBe(true) + }) + + it('refuses a bare-locked worktree, whose reason parses to an empty string', async () => { + git(repo, 'worktree', 'lock', sibling) + const deleter = spyDeleter() + + const result = await removeWorktree(repo, sibling, {}, deleter) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/locked/i) + expect(deleter.calls).toEqual([]) + expect(existsSync(sibling)).toBe(true) + }) + + it("returns git's first line when the bookkeeping step fails, and a retry heals it", async () => { + // A deleter that reports success without deleting is the deterministic stand-in + // for git's own bookkeeping failure: git then still sees a populated, dirty + // worktree and refuses. The branch under test is "deletion ok, git failed". + writeFileSync(join(sibling, 'untracked.txt'), 'wip', 'utf8') + const liar = spyDeleter({ ok: true }) + + const failed = await removeWorktree(repo, sibling, { force: true }, liar) + + expect(failed.ok).toBe(false) + expect(failed.error).toMatch(/^fatal: /) + expect(await listWorktrees(repo)).toHaveLength(2) + + const retried = await removeWorktree(repo, sibling, { force: true }) + + expect(retried).toEqual({ ok: true }) + expect(existsSync(sibling)).toBe(false) + expect(await listWorktrees(repo)).toHaveLength(1) + }) }) diff --git a/src/main/worktree-manager.ts b/src/main/worktree-manager.ts index 70716b8..78c524d 100644 --- a/src/main/worktree-manager.ts +++ b/src/main/worktree-manager.ts @@ -9,6 +9,7 @@ import type { RemoveWorktreeResult } from '../shared/worktrees' import { worktreeNameFor, worktreePathFor } from '../shared/worktrees' +import { removeDirTree, type DirRemovalResult } from './dir-remover' const run = promisify(execFile) @@ -254,20 +255,66 @@ function ffFailureLine(err: unknown, baseBranch: string, upstream: string): stri return gitFailureLine(err) } +/** The deleter, injected with the real implementation as the default. */ +export interface WorktreeRemoveDeps { + removeDirTree(path: string): Promise +} + +const realRemoveDeps: WorktreeRemoveDeps = { removeDirTree } + /** - * `git worktree remove` with the PRD guards (DLWT-01): refuses the repo's - * primary checkout, and refuses a dirty worktree unless force — dirtiness is - * re-checked fresh here, not trusted from the renderer's tree snapshot. + * Delete-then-deregister removal (WRFT-01, WRFT-02). **The app deletes the + * worktree directory itself and only then asks git to drop the bookkeeping** — + * git is never the deleter. Two defects drove the inversion: `git worktree + * remove` deletes its admin dir even when its own deletion failed ("no going + * back from here"), leaving a folder on disk that no longer belongs to any + * worktree and that `scanRepos` cannot see; and git for Windows recurses into + * directory junctions, emptying the AD-013 skills targets while reporting + * success. Under this order a failed deletion leaves the worktree fully + * registered — visible, and retryable by simply clicking Remove again. + * + * The guard order is the contract: primary → registered → locked → dirty → + * delete → bookkeeping. **Every guard refuses before a single byte is deleted**, + * because after the reorder nothing downstream can veto the deletion: git's own + * lock refusal would arrive only after the files were gone, and the registered + * check is what stops an unvalidated path reaching a recursive delete. + * `force` keeps its FRWT meaning — skip the dirty check, nothing else. + * * Failures (guards included) are returned, never thrown. */ export async function removeWorktree( repoPath: string, worktreePath: string, - opts: { force?: boolean } = {} + opts: { force?: boolean } = {}, + deps: WorktreeRemoveDeps = realRemoveDeps ): Promise { + // 1. Primary checkout (DLWT-01) — message unchanged. if (samePath(repoPath, worktreePath)) { return { ok: false, error: "This is the repo's primary checkout — it can't be removed here." } } + // 2. Registered worktree of *this* repo. Also the anti-`rm -rf` guard, so a + // git failure here refuses rather than guessing: fail closed. + let blocks: PorcelainBlock[] + try { + const { stdout } = await git(repoPath, ['worktree', 'list', '--porcelain']) + blocks = parsePorcelainBlocks(stdout) + } catch (err) { + return { ok: false, error: gitFailureLine(err) } + } + const entry = blocks.find((block) => samePath(block.path, worktreePath)) + if (!entry) { + return { ok: false, error: `${worktreePath} is not a registered worktree of this repo.` } + } + // 3. `git worktree lock` (WRFT-01 AC 3). Presence of the line is the lock — + // a bare `locked` parses to '', which is still locked. + if (entry.locked !== undefined) { + const reason = entry.locked === '' ? '' : `: ${entry.locked}` + return { + ok: false, + error: `This worktree is locked${reason} — unlock it before removing (git worktree unlock).` + } + } + // 4. Dirty (FRWT) — message unchanged; the only check `force` skips. if (!opts.force) { const { dirty, changes } = await statusOf(worktreePath) if (dirty) { @@ -277,17 +324,37 @@ export async function removeWorktree( } } } - const args = opts.force - ? ['worktree', 'remove', '--force', worktreePath] - : ['worktree', 'remove', worktreePath] + // 5. Delete. On give-up we return *before touching git*, which is what keeps + // the worktree registered and the removal retryable (WRFT-02 AC 1). + const removal = await deps.removeDirTree(worktreePath) + if (!removal.ok) { + const { blockedPath, remaining } = removal.leftover ?? { + blockedPath: worktreePath, + remaining: 0 + } + return { + ok: false, + error: leftoverMessage(blockedPath, remaining), + leftover: { blockedPath, remaining } + } + } + // 6. Bookkeeping only — the directory is already gone, so plain `remove` + // suffices (`--force` would protect nothing) and a failure self-heals on + // retry, since git accepts removing a worktree whose directory is missing. try { - await git(repoPath, args) + await git(repoPath, ['worktree', 'remove', worktreePath]) return { ok: true } } catch (err) { return { ok: false, error: gitFailureLine(err) } } } +/** What the user needs to act on: what blocked it, how much is left, and that retrying works. */ +function leftoverMessage(blockedPath: string, remaining: number): string { + const items = `${remaining} item${remaining === 1 ? '' : 's'}` + return `Couldn't delete ${blockedPath} — ${items} still on disk. The worktree is still registered, so you can retry the removal once nothing is using it.` +} + /** Paths from the tree snapshot and the registry may differ in case/separators. */ function samePath(a: string, b: string): boolean { const norm = (p: string): string => p.replaceAll('/', '\\').replace(/\\+$/, '').toLowerCase() @@ -302,9 +369,15 @@ function gitFailureLine(err: unknown): string { return err instanceof Error ? err.message.split('\n')[0] : String(err) } -interface PorcelainBlock { +export interface PorcelainBlock { path: string branch: string + /** + * Git's `git worktree lock` reason, or `''` for a bare `locked` line. Absent + * (`undefined`) means the worktree is not locked at all — the three cases must + * stay distinguishable, because `''` is a *locked* worktree (WRFT-01 AC 3). + */ + locked?: string } /** @@ -312,8 +385,12 @@ interface PorcelainBlock { * worktree * HEAD * branch refs/heads/ (or `detached`, or `bare`) + * locked [reason] (only when the worktree is locked) + * + * Exported for unit tests (same stance as `parseChangedFiles`): the parse is the + * pure half of the locked guard, testable against real porcelain without a remove. */ -function parsePorcelainBlocks(stdout: string): PorcelainBlock[] { +export function parsePorcelainBlocks(stdout: string): PorcelainBlock[] { const blocks: PorcelainBlock[] = [] for (const raw of stdout.split(/\r?\n\r?\n/)) { const lines = raw.split(/\r?\n/).filter(Boolean) @@ -333,7 +410,14 @@ function parsePorcelainBlocks(stdout: string): PorcelainBlock[] { } else { branch = `(detached ${head ? head.slice(0, 7) : '?'})` } - blocks.push({ path, branch }) + // `locked` alone and `locked ` are both lock markers; only the + // absence of the line means unlocked. + const lockedLine = lines.find((l) => l === 'locked' || l.startsWith('locked ')) + blocks.push({ + path, + branch, + ...(lockedLine === undefined ? {} : { locked: lockedLine.slice('locked'.length).trim() }) + }) } return blocks } diff --git a/src/renderer/src/components/WorktreeDetail.css b/src/renderer/src/components/WorktreeDetail.css index 6ff8c23..238ab88 100644 --- a/src/renderer/src/components/WorktreeDetail.css +++ b/src/renderer/src/components/WorktreeDetail.css @@ -358,6 +358,25 @@ color: var(--red); } +/* Blocked-removal report (WRFT-06): the reason, then the offending path on its + own row. min-width:0 lets the flex item shrink so break-all can act. */ +.detail-danger-leftover { + display: flex; + flex-direction: column; + gap: 4px; + flex: 1 1 260px; + min-width: 0; +} + +.detail-danger-path { + font-family: var(--font-mono); + font-size: 11.5px; + color: var(--text-faint); + /* A worktree path can be far longer than the section — wrap it mid-token + rather than letting it push the Danger row's layout (WRFT-06 AC 4). */ + word-break: break-all; +} + /* AGENTS section (AGSN-06): spawn button + existing-session chips. */ .detail-agents { display: flex; diff --git a/src/renderer/src/components/WorktreeDetail.tsx b/src/renderer/src/components/WorktreeDetail.tsx index 0d5dfe7..f5c10c7 100644 --- a/src/renderer/src/components/WorktreeDetail.tsx +++ b/src/renderer/src/components/WorktreeDetail.tsx @@ -4,7 +4,7 @@ import type { SessionView } from '../../../shared/config' import type { ShortcutTool } from '../../../shared/shortcuts' import type { PinnedTaskView } from '../../../shared/tasks' import type { WorktreeNode } from '../../../shared/tree' -import type { ChangedFile } from '../../../shared/worktrees' +import type { ChangedFile, RemovalLeftover } from '../../../shared/worktrees' import { api } from '../lib/api' import { stateClass, typeClass } from '../lib/task-pills' import { Icon } from './Icon' @@ -84,6 +84,10 @@ export function WorktreeDetail({ const [copied, setCopied] = useState(false) const [removing, setRemoving] = useState(false) const [removeError, setRemoveError] = useState(null) + /** What a blocked deletion left on disk (WRFT-06) — set only when the removal + * itself was blocked, so the worktree is still registered and this row is the + * retry handle. Cleared on every new attempt alongside removeError. */ + const [removeLeftover, setRemoveLeftover] = useState(null) /** When set, the removal confirmation is open — agents and/or dirty (AGCF-05, FRWT-03). */ const [confirmOpen, setConfirmOpen] = useState(false) /** Fresh changed-file list for the confirm dialog when the worktree is dirty (FRWT-03). */ @@ -118,6 +122,7 @@ export function WorktreeDetail({ const doRemove = (): void => { setRemoving(true) setRemoveError(null) + setRemoveLeftover(null) api .invoke('worktrees:remove', { repoPath, worktreePath: worktree.path, force: worktree.dirty }) .then((result) => { @@ -127,6 +132,7 @@ export function WorktreeDetail({ } else { setRemoving(false) setRemoveError(result.error ?? 'Removal failed') + setRemoveLeftover(result.leftover ?? null) } }) .catch((err) => { @@ -165,6 +171,7 @@ export function WorktreeDetail({ const confirmRemove = (): void => { setRemoving(true) setRemoveError(null) + setRemoveLeftover(null) Promise.all(runningSessions.map((s) => api.invoke('sessions:stop', { id: s.id }))) .then(() => { setConfirmOpen(false) @@ -317,7 +324,23 @@ export function WorktreeDetail({ Remove worktree {guardNote && {guardNote}} - {removeError && {removeError}} + {/* A blocked deletion gets the structured treatment instead of the flat + line: the same facts, but with the blocked path on its own row so a + long path wraps inside the section instead of stretching it (WRFT-06 + AC 4). Any other failure keeps the plain error line. */} + {removeError && !removeLeftover && ( + {removeError} + )} + {removeLeftover && ( + + + Couldn’t delete this worktree — {removeLeftover.remaining} item + {removeLeftover.remaining === 1 ? '' : 's'} still on disk. It’s still registered, so + you can retry once nothing is using it. + + {removeLeftover.blockedPath} + + )} {confirmOpen && ( diff --git a/src/shared/ipc-contract.ts b/src/shared/ipc-contract.ts index c7e03d5..fc3bc0a 100644 --- a/src/shared/ipc-contract.ts +++ b/src/shared/ipc-contract.ts @@ -55,12 +55,16 @@ export interface IpcContract { } res: CreateWorktreeResult } - /** git worktree remove with dirty/primary guards; failures are returned, never thrown. */ + /** + * Delete-first worktree removal (WRFT-01): the app deletes the directory, then + * git drops the bookkeeping. Failures are returned, never thrown — and a `res` + * carrying `leftover` means nothing was deregistered, so the same call retries. + */ 'worktrees:remove': { req: { repoPath: string worktreePath: string - /** Override the dirty guard with `git worktree remove --force` (FRWT-01); absent = off. */ + /** Skip the dirty guard (FRWT-01); absent = off. Never skips primary/registered/locked. */ force?: boolean } res: RemoveWorktreeResult diff --git a/src/shared/worktrees.ts b/src/shared/worktrees.ts index 55d1abb..620c460 100644 --- a/src/shared/worktrees.ts +++ b/src/shared/worktrees.ts @@ -92,11 +92,29 @@ export interface CreateWorktreeResult { conflict?: 'branch-exists' } +/** + * What a deletion that gave up left behind (WRFT-04 AC 3). Its presence means + * the worktree is still registered with git, so the removal can simply be + * retried once whatever holds the path lets go. + */ +export interface RemovalLeftover { + /** The path the deleter could not remove (absolute). */ + blockedPath: string + /** Entries still present under the removal root after the failed attempt. */ + remaining: number +} + /** Result of worktrees:remove — failures (guards included) are returned, never thrown. */ export interface RemoveWorktreeResult { ok: boolean /** Human-readable refusal/failure message, present when ok is false. */ error?: string + /** + * Present only when the *deletion* gave up (WRFT-04 AC 3) — never on a guard + * refusal or a bookkeeping failure. Its presence is the renderer's signal that + * the worktree is still registered and the Remove button is a working retry. + */ + leftover?: RemovalLeftover } /** diff --git a/vitest.config.ts b/vitest.config.ts index 5877442..3a2d2c3 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -3,6 +3,15 @@ import { defineConfig } from 'vitest/config' export default defineConfig({ test: { include: ['src/**/*.test.ts', 'scripts/**/*.test.ts'], + // Real-git and real-process suites (worktree-manager, tree, post-create-hook, + // hook-shell, workflow-loader) routinely take 5-15s per test under parallel + // load — measured 14 failures in one full run, all duration overruns, on an + // otherwise untouched tree. A timeout is a ceiling, not a delay: passing + // tests are unaffected, and this only stops the runner killing work that is + // merely starved. worktree-manager.test.ts already raised these locally for + // its base-refresh block; this lifts the same fix to every suite. + testTimeout: 30000, + hookTimeout: 30000, // Report-only (AD-003): no thresholds, so `test:coverage` never fails the // build — it just surfaces which logic modules are under-tested. Scoped to // the layers that carry unit tests; renderer components and thin OS/Electron