From 1675d28ec8ef22eb3355ea09c53fc43050ef5109 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 18 Aug 2026 12:32:48 -0500 Subject: [PATCH 1/7] RT-43: pnpm implicit install defaults to plain install (side-effects cache starves out-of-tree generators) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pnpm's side-effects cache replays a dependency's recorded postinstall effects rather than re-running the script, and it only ever captured files written inside node_modules. A dep whose postinstall writes outside the package dir (prisma generating into apps/backend/generated/) is silently skipped on a fresh tree: the install exits 0 and the worktree is missing generated code. The flag remains available as a declared ready step for repos verified free of out-of-tree generators — it is just no longer the blind default. Co-Authored-By: Claude Fable 5 --- lib/worktree/__tests__/config.test.ts | 8 ++++---- lib/worktree/config.ts | 12 +++++++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/worktree/__tests__/config.test.ts b/lib/worktree/__tests__/config.test.ts index ee0fd019..ec86794a 100644 --- a/lib/worktree/__tests__/config.test.ts +++ b/lib/worktree/__tests__/config.test.ts @@ -101,7 +101,7 @@ describe("worktree config", () => { JSON.stringify({ name: "x", packageManager: "pnpm@9.1.0" }) ); expect(resolveImplicitInstall(repoPath)).toEqual({ - run: "pnpm install --side-effects-cache", + run: "pnpm install", when: "changed:pnpm-lock.yaml", }); }); @@ -131,7 +131,7 @@ describe("worktree config", () => { writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); expect(resolveImplicitInstall(repoPath)).toEqual({ - run: "pnpm install --side-effects-cache", + run: "pnpm install", when: "changed:pnpm-lock.yaml", }); }); @@ -168,7 +168,7 @@ describe("worktree config", () => { ready: [{ run: "node scripts/gen-types.js", when: "changed:db/schema/**" }], }; expect(resolveReadySteps(cfg, repoPath)).toEqual([ - { run: "pnpm install --side-effects-cache", when: "changed:pnpm-lock.yaml" }, + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, { run: "node scripts/gen-types.js", when: "changed:db/schema/**" }, ]); }); @@ -200,7 +200,7 @@ describe("worktree config", () => { ready: [{ run: "pnpm lint" }], }; expect(resolveReadySteps(cfg, repoPath)).toEqual([ - { run: "pnpm install --side-effects-cache", when: "changed:pnpm-lock.yaml" }, + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, { run: "pnpm lint" }, ]); }); diff --git a/lib/worktree/config.ts b/lib/worktree/config.ts index 3bb9d7a0..d8b04703 100644 --- a/lib/worktree/config.ts +++ b/lib/worktree/config.ts @@ -79,8 +79,18 @@ export function loadWorktreeRepoConfig(repoName: string, repoPath: string): Work type Manager = "pnpm" | "bun" | "yarn" | "npm"; +/** + * pnpm's default is deliberately a PLAIN install, not `--side-effects-cache`: + * that cache replays a dependency's recorded postinstall effects instead of + * re-running it, and it only ever captured files written inside node_modules. + * A dep whose postinstall writes outside the package dir (prisma generating + * into apps/backend/generated/, say) is therefore silently skipped on a fresh + * tree — the install "succeeds" and the tree is missing generated code. The + * flag stays available as a declared ready step for repos verified free of + * out-of-tree generators; it is not safe as a blind default. + */ const MANAGER_STEP: Record = { - pnpm: { run: "pnpm install --side-effects-cache", when: "changed:pnpm-lock.yaml" }, + pnpm: { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, bun: { run: "bun install", when: "changed:bun.lock*" }, yarn: { run: "yarn install", when: "changed:yarn.lock" }, npm: { run: "npm install", when: "changed:package-lock.json" }, From 07d419434361a24a74d01d6cda207fbd998df65a Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 18 Aug 2026 12:33:45 -0500 Subject: [PATCH 2/7] RT-42: install dedup sees through env-var prefixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The implicit install is suppressed when a declared ready step already runs " install". The prefix test was applied to the raw run, so a step declared as `SKIP_GEN_TYPES=1 pnpm install --side-effects-cache` did not match and the tree installed twice — once implicitly, once declared. stripEnvPrefix() drops leading `VAR=value` assignments and an optional `env` word before the test. Recognition only: the step still executes verbatim, env prefix included. Co-Authored-By: Claude Fable 5 --- lib/worktree/__tests__/config.test.ts | 70 +++++++++++++++++++++++++++ lib/worktree/config.ts | 27 ++++++++++- 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/lib/worktree/__tests__/config.test.ts b/lib/worktree/__tests__/config.test.ts index ec86794a..ecf033a2 100644 --- a/lib/worktree/__tests__/config.test.ts +++ b/lib/worktree/__tests__/config.test.ts @@ -8,6 +8,7 @@ import { loadWorktreeRepoConfig, resolveImplicitInstall, resolveReadySteps, + stripEnvPrefix, loadWorktreeAppConfig, type WorktreeRepoConfig, } from "../config.ts"; @@ -215,6 +216,75 @@ describe("worktree config", () => { }; expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); }); + + test("an env-var prefix on the declared install still suppresses the implicit one", () => { + const repoPath = tmpRepoPath("rtcfg-resolve5-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "SKIP_GEN_TYPES=1 pnpm install --side-effects-cache" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); + }); + + test("an `env VAR=value` prefix on the declared install still suppresses the implicit one", () => { + const repoPath = tmpRepoPath("rtcfg-resolve6-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "env FOO=bar pnpm install" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); + }); + + test("an env-var prefix on a NON-install step does not suppress the implicit install", () => { + const repoPath = tmpRepoPath("rtcfg-resolve7-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "SKIP_X=1 pnpm lint" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual([ + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, + { run: "SKIP_X=1 pnpm lint" }, + ]); + }); + }); + + describe("stripEnvPrefix", () => { + test("leaves a bare command alone", () => { + expect(stripEnvPrefix("pnpm install")).toBe("pnpm install"); + }); + + test("strips one or many leading assignments", () => { + expect(stripEnvPrefix("A=1 pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("A=1 B=2 pnpm install --frozen-lockfile")).toBe( + "pnpm install --frozen-lockfile", + ); + }); + + test("strips a leading `env`, with or without assignments after it", () => { + expect(stripEnvPrefix("env pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("env FOO=bar pnpm install")).toBe("pnpm install"); + }); + + test("tolerates quoted assignment values containing spaces", () => { + expect(stripEnvPrefix('FOO="a b" pnpm install')).toBe("pnpm install"); + }); + + test("never eats the command itself", () => { + expect(stripEnvPrefix("A=1")).toBe("A=1"); + expect(stripEnvPrefix("env")).toBe("env"); + }); }); describe("loadWorktreeAppConfig", () => { diff --git a/lib/worktree/config.ts b/lib/worktree/config.ts index d8b04703..a800e35c 100644 --- a/lib/worktree/config.ts +++ b/lib/worktree/config.ts @@ -130,19 +130,44 @@ export function resolveImplicitInstall(repoPath: string): ReadyStep | null { return manager ? MANAGER_STEP[manager] : null; } +/** One leading `env` word, or one `VAR=value` assignment (value optionally quoted). */ +const ENV_PREFIX_TOKEN = /^(?:env|[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|\S*))\s+/; + +/** + * Drop a shell env prefix from a declared run, leaving the command word first: + * `SKIP_GEN_TYPES=1 pnpm install` → `pnpm install`, `env FOO=bar pnpm install` + * → `pnpm install`. Purely for recognising WHICH command a step runs (the + * install-dedup test below); the step itself is always executed verbatim, env + * prefix included. Requires trailing whitespace, so a run that is nothing but + * an assignment (`A=1`) is left alone rather than emptied. + */ +export function stripEnvPrefix(run: string): string { + let rest = run.trimStart(); + while (ENV_PREFIX_TOKEN.test(rest)) { + rest = rest.replace(ENV_PREFIX_TOKEN, ""); + } + return rest; +} + /** * Implicit install first UNLESS cfg.ready already declares its own install * step for the detected manager (run starts with " install", e.g. * "pnpm install --side-effects-cache") — only an install step replaces the * implicit one; any other declared command for that manager (e.g. "pnpm * lint") does not suppress it. Otherwise cfg.ready in order. + * + * The prefix test runs against the env-stripped run: declaring + * `SKIP_GEN_TYPES=1 pnpm install` is still declaring the install, and letting + * an env prefix hide it from the dedup made the tree install twice. */ export function resolveReadySteps(cfg: WorktreeRepoConfig, repoPath: string): ReadyStep[] { const manager = detectManager(repoPath); if (!manager) return cfg.ready; const installPrefix = `${manager} install`; - const alreadyDeclared = cfg.ready.some((step) => step.run.startsWith(installPrefix)); + const alreadyDeclared = cfg.ready.some((step) => + stripEnvPrefix(step.run).startsWith(installPrefix), + ); if (alreadyDeclared) return cfg.ready; return [MANAGER_STEP[manager], ...cfg.ready]; From 20556f251608c436f3255f0b56a4976a68ce3552 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 18 Aug 2026 12:45:12 -0500 Subject: [PATCH 3/7] =?UTF-8?q?RT-41:=20dispose=20renames=20to=20trash=20a?= =?UTF-8?q?nd=20reaps=20async=20=E2=80=94=20verb=20latency=20independent?= =?UTF-8?q?=20of=20tree=20size?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git worktree remove --force` unlinks a worktree file by file. On a pnpm-scale node_modules that ran for minutes inside the verb, and the 5-minute timeout that bounded it killed the unlink mid-flight, leaving a half-deleted directory that was neither a worktree nor gone. Disposal now renames the tree to a sibling `.trash--` — same volume, atomic, instant — and everything after it (worktree prune, branch -D, registry prune, event) is fast, so the verb returns in seconds however large the tree is. A rename that fails keeps the old "remove-failed" refusal: the contract was always "is the tree still at rec.path", and it still is. The actual delete is a detached `rm -rf` with no timeout that nobody awaits. If it dies (or the daemon does), the leftover is a `.trash-*` directory, which the reconciler's new reap duty sweeps from every worktree root on a later pass — gated on the app `enabled` flag like every other mutating duty. A crash costs disk, never correctness. scrapTree shares the same helper, so a mid-create scrap also returns instantly and no longer depends on git being willing to remove the tree. Co-Authored-By: Claude Fable 5 --- .../__tests__/worktree-reconciler.test.ts | 74 +++++++--- lib/daemon/worktree-reconciler.ts | 27 ++++ lib/worktree/__tests__/create.test.ts | 57 +++++++- lib/worktree/__tests__/dispose.test.ts | 69 ++++++++-- lib/worktree/__tests__/trash.test.ts | 127 ++++++++++++++++++ lib/worktree/create.ts | 34 +++-- lib/worktree/dispose.ts | 45 ++++--- lib/worktree/trash.ts | 119 ++++++++++++++++ 8 files changed, 496 insertions(+), 56 deletions(-) create mode 100644 lib/worktree/__tests__/trash.test.ts create mode 100644 lib/worktree/trash.ts diff --git a/lib/daemon/__tests__/worktree-reconciler.test.ts b/lib/daemon/__tests__/worktree-reconciler.test.ts index 82df31db..9aa83c2c 100644 --- a/lib/daemon/__tests__/worktree-reconciler.test.ts +++ b/lib/daemon/__tests__/worktree-reconciler.test.ts @@ -1,6 +1,6 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { execSync } from "child_process"; -import { existsSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { basename, join } from "path"; import type { Logger } from "pino"; @@ -326,6 +326,36 @@ describe("createWorktreeReconciler", () => { expect(loadRegistry(repoName).length).toBe(1); // just main, adopted once }); + test("runOnce reaps .trash-* leftovers in the default and configured roots", async () => { + // What a daemon crash mid-reap leaves behind: the dispose renamed the tree + // but its detached `rm -rf` never finished (or never started). + const configuredRoot = realpathSync(mkdtempSync(join(tmpdir(), "rtrecon-root-"))); + writeJson(join(repoDataDir(repoName), "config.json"), { + worktrees: { root: configuredRoot }, + }); + + const defaultRootTrash = join(repo, ".worktrees", ".trash-hotel-1700000000000"); + const configuredTrash = join(configuredRoot, ".trash-india-1700000000001"); + const live = join(configuredRoot, "juliet"); + for (const dir of [defaultRootTrash, configuredTrash, live]) { + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "file.txt"), "x\n"); + } + + const reconciler = createWorktreeReconciler({ + cache: { entries: {} }, + repoIndex: () => ({ [repoName]: repo }), + emit: () => {}, + log: fakeLog(), + }); + + await reconciler.runOnce(); + + expect(existsSync(defaultRootTrash)).toBe(false); + expect(existsSync(configuredTrash)).toBe(false); + expect(existsSync(live)).toBe(true); + }); + test("runOnce with the app disabled still syncs the registry, but skips reactor/freshen/replenish", async () => { // A dedicated repoName (not the shared "acme" the other tests in this // describe use): the prior test's `kick()` is deliberately unawaited by @@ -350,6 +380,9 @@ describe("createWorktreeReconciler", () => { sh(`git add -A && git ${GIT_ID} commit -m feat`, clone); sh(`git push -q origin main`, clone); + const seededTrash = join(repo, ".worktrees", ".trash-kilo-1700000000002"); + mkdirSync(seededTrash, { recursive: true }); + const beforeSha = await headSha(repo); const events: Array<{ type: string; data: any }> = []; const reconciler = createWorktreeReconciler({ @@ -372,6 +405,9 @@ describe("createWorktreeReconciler", () => { // Reactor skipped: it never even opened/wrote its state file. expect(existsSync(__test__.reactorStatePath())).toBe(false); expect(events.length).toBe(0); + // Reap skipped: it deletes directories, so it is gated like every other + // mutating duty. + expect(existsSync(seededTrash)).toBe(true); }); }); @@ -587,21 +623,26 @@ describe("merge reactor (detectTransitions)", () => { test("a failed worktree removal is retried, never advertised as disposable", async () => { const rec = ephemeralTree("hotel", "feat-hotel"); - // A locked worktree makes `git worktree remove --force` refuse (it wants - // --force twice) without touching the directory: a mechanical, transient - // removal failure, which must NOT be reported to the user as "disposable". - sh(`git -C ${repo} worktree lock ${rec.path}`); + // Disposal renames the tree into a sibling trash dir, so the mechanical, + // transient failure to reproduce is a root nothing can be renamed within. + // The tree survives untouched, and that must NOT be reported to the user + // as "disposable". + const root = join(repo, ".worktrees"); + chmodSync(root, 0o555); + + try { + await detect(mrCache("feat-hotel", "opened")); + await detect(mrCache("feat-hotel", "merged")); + + expect(existsSync(rec.path)).toBe(true); + expect(tracked(rec.path)!.state).toBe("claimed"); + expect(tracked(rec.path)!.disposableReason).toBeUndefined(); + expect(events.some((e) => e.type === "worktree:disposable")).toBe(false); + expect(reactorState().mrState[`${repoName}:feat-hotel`]).toBe("opened"); + } finally { + chmodSync(root, 0o755); + } - await detect(mrCache("feat-hotel", "opened")); - await detect(mrCache("feat-hotel", "merged")); - - expect(existsSync(rec.path)).toBe(true); - expect(tracked(rec.path)!.state).toBe("claimed"); - expect(tracked(rec.path)!.disposableReason).toBeUndefined(); - expect(events.some((e) => e.type === "worktree:disposable")).toBe(false); - expect(reactorState().mrState[`${repoName}:feat-hotel`]).toBe("opened"); - - sh(`git -C ${repo} worktree unlock ${rec.path}`); await detect(mrCache("feat-hotel", "merged")); expect(existsSync(rec.path)).toBe(false); @@ -882,6 +923,9 @@ describe("freshen", () => { { name: basename(repo), path: repo, kind: "main", branch: "main", createdAt: new Date().toISOString() }, ]); + const seededTrash = join(repo, ".worktrees", ".trash-kilo-1700000000002"); + mkdirSync(seededTrash, { recursive: true }); + const beforeSha = await headSha(repo); const events: Array<{ type: string; data: any }> = []; await __test__.freshenRepo({ diff --git a/lib/daemon/worktree-reconciler.ts b/lib/daemon/worktree-reconciler.ts index b70fa38e..046d53dd 100644 --- a/lib/daemon/worktree-reconciler.ts +++ b/lib/daemon/worktree-reconciler.ts @@ -45,6 +45,7 @@ import { type WorktreeAppConfig, } from "../worktree/config.ts"; import { killWorktreeProcesses } from "./worktree-process-kill.ts"; +import { reapTrashInRoots } from "../worktree/trash.ts"; export interface ReconcilerDeps { cache: { entries: Record }; @@ -958,6 +959,26 @@ async function replenishAndShrink( } } +/** + * Reap duty: delete any `.trash-*` directory sitting in a worktree root. + * + * Disposal renames a tree to trash and fires a detached `rm -rf` at it (see + * worktree/trash.ts). That process can die — a daemon crash, a reboot mid + * delete — and what it leaves behind is a directory nobody will ever look at + * again. This is the sweep that collects them, so a crash costs disk and + * nothing else. + * + * Both roots are swept: the repo's default `.worktrees` and whatever root the + * repo config declares, because a root that changed after a disposal still has + * the old root's leftovers in it. + */ +async function reapRepoTrash(deps: { repoName: string; repoPath: string; log: Logger }): Promise { + const { repoName, repoPath, log } = deps; + const cfg = loadWorktreeRepoConfig(repoName, repoPath); + const reaped = await reapTrashInRoots([join(repoPath, ".worktrees"), cfg.root], log); + if (reaped > 0) log.info({ repo: repoName, count: reaped }, "worktree trash reaped"); +} + /** Whether a repo has any worktree state worth reconciling: registry entries or a declared "worktrees" config. */ function repoHasWorktreeActivity(repoName: string): boolean { if (loadRegistry(repoName).length > 0) return true; @@ -1030,6 +1051,11 @@ export function createWorktreeReconciler(deps: ReconcilerDeps): { } catch (err) { deps.log.warn({ err, repo: repoName }, "worktree reconciler: replenish/shrink pass failed"); } + try { + await reapRepoTrash({ repoName, repoPath, log: deps.log }); + } catch (err) { + deps.log.warn({ err, repo: repoName }, "worktree reconciler: trash reap pass failed"); + } } } @@ -1058,6 +1084,7 @@ export function createWorktreeReconciler(deps: ReconcilerDeps): { export const __test__ = { detectTransitions, + reapRepoTrash, reactorStatePath, freshenRepo, replenishAndShrink, diff --git a/lib/worktree/__tests__/create.test.ts b/lib/worktree/__tests__/create.test.ts index 0f880de2..835dc2ed 100644 --- a/lib/worktree/__tests__/create.test.ts +++ b/lib/worktree/__tests__/create.test.ts @@ -5,9 +5,9 @@ import { tmpdir } from "os"; import { join } from "path"; import { writeJson } from "../../json-store.ts"; import { repoDataDir } from "../../rt-paths.ts"; -import { loadRegistry } from "../registry.ts"; +import { loadRegistry, saveRegistry, type TreeRecord } from "../registry.ts"; import { branchExistsLocalAsync, listWorktreesAsync } from "../git-async.ts"; -import { createTree, type CreateDeps } from "../create.ts"; +import { createTree, scrapTree, type CreateDeps } from "../create.ts"; function makeRepo(): string { // realpathSync: git canonicalizes /var -> /private/var on macOS (Global Constraints) @@ -115,3 +115,56 @@ describe("createTree", () => { expect(registry.length).toBe(0); }); }); + +describe("scrapTree", () => { + let repo: string; + let repoName: string; + let events: Array<{ type: string; data: unknown }>; + + beforeEach(() => { + process.env.HOME = realpathSync(mkdtempSync(join(tmpdir(), "rtcreate-home-"))); + repo = makeRepo(); + addBareOrigin(repo); + repoName = "acme"; + events = []; + }); + + test("tolerates a record whose worktree and branch never came to exist", async () => { + const rec: TreeRecord = { + name: "ghost", + path: join(repo, ".worktrees", "ghost"), + kind: "ephemeral", + state: "creating", + branch: "on-deck/ghost", + createdAt: new Date().toISOString(), + }; + saveRegistry(repoName, [rec]); + + await scrapTree(makeDeps(repoName, repo, events), rec); + + expect(loadRegistry(repoName).length).toBe(0); + }); + + test("scraps a tree git itself would refuse to remove", async () => { + // A locked worktree makes `git worktree remove --force` refuse (it wants + // --force twice). A rename does not care: a mid-create scrap must always + // get the half-built tree out of the way, whatever state git is in. + const path = join(repo, ".worktrees", "locked"); + execSync(`git -C ${repo} worktree add -b on-deck/locked ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + execSync(`git -C ${repo} worktree lock ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + const rec: TreeRecord = { + name: "locked", + path, + kind: "ephemeral", + state: "creating", + branch: "on-deck/locked", + createdAt: new Date().toISOString(), + }; + saveRegistry(repoName, [rec]); + + await scrapTree(makeDeps(repoName, repo, events), rec); + + expect(existsSync(path)).toBe(false); + expect(loadRegistry(repoName).length).toBe(0); + }); +}); diff --git a/lib/worktree/__tests__/dispose.test.ts b/lib/worktree/__tests__/dispose.test.ts index 761030af..ac8f30f2 100644 --- a/lib/worktree/__tests__/dispose.test.ts +++ b/lib/worktree/__tests__/dispose.test.ts @@ -1,8 +1,8 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { execSync } from "child_process"; -import { existsSync, mkdirSync, mkdtempSync, writeFileSync, realpathSync } from "fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, writeFileSync, realpathSync } from "fs"; import { tmpdir } from "os"; -import { join } from "path"; +import { basename, dirname, join } from "path"; import { repoDataDir } from "../../rt-paths.ts"; import { saveSyncConfig } from "../../sync-config.ts"; import { loadRegistry, saveRegistry, type TreeRecord } from "../registry.ts"; @@ -50,6 +50,15 @@ function addTree(repo: string, name: string, branch: string, base = "origin/main return path; } +/** Poll until `cond` holds — for the detached reaper, which nobody awaits. */ +async function waitFor(cond: () => boolean, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + while (!cond()) { + if (Date.now() > deadline) throw new Error("timed out waiting for condition"); + await new Promise((r) => setTimeout(r, 20)); + } +} + function commitIn(worktree: string, file: string, content: string): void { writeFileSync(join(worktree, file), content); execSync(`git add ${file} && git ${GIT_ID} commit -m change`, { @@ -566,17 +575,57 @@ describe("disposeTree", () => { expect(loadRegistry(repoName).length).toBe(1); }); - test("a worktree git refuses to remove refuses \"remove-failed\" and keeps the registry row", async () => { + test("a tree that cannot be renamed refuses \"remove-failed\" and keeps the registry row", async () => { + const path = addTree(repo, "tree-a", "feature-a"); + const rec = register(repoName, ephemeral("tree-a", path, "feature-a")); + // A rename needs write permission on the PARENT: a read-only .worktrees is + // the mechanical, transient failure that stands in for a busy/held tree. + const root = join(repo, ".worktrees"); + chmodSync(root, 0o555); + + try { + const result = await disposeTree(makeDeps(), rec, { force: true }); + expect(result).toEqual({ disposed: false, refusal: "remove-failed" }); + // Nothing downstream of the rename ran: the tree, its branch, and its + // registry row are all still there for the retry. + expect(existsSync(path)).toBe(true); + expect(loadRegistry(repoName).length).toBe(1); + expect(await branchExistsLocalAsync(repo, "feature-a")).toBe(true); + expect((await listWorktreesAsync(repo))!.some((w) => w.path === path)).toBe(true); + expect(events.length).toBe(0); + } finally { + chmodSync(root, 0o755); + } + }); + + test("disposal renames the tree to a .trash-* sibling and reaps it in the background", async () => { const path = addTree(repo, "tree-a", "feature-a"); const rec = register(repoName, ephemeral("tree-a", path, "feature-a")); - // A locked worktree needs --force twice; one --force fails and leaves the tree. - execSync(`git -C ${repo} worktree lock ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + const root = join(repo, ".worktrees"); - const result = await disposeTree(makeDeps(), rec, { force: true }); - expect(result).toEqual({ disposed: false, refusal: "remove-failed" }); - expect(existsSync(path)).toBe(true); - expect(loadRegistry(repoName).length).toBe(1); - expect(await branchExistsLocalAsync(repo, "feature-a")).toBe(true); + const infos: Array> = []; + const deps = makeDeps({ + log: { info: (fields: unknown) => infos.push(fields as Record), warn: () => {} }, + }); + const result = await disposeTree(deps, rec, { force: true }); + expect(result).toEqual({ disposed: true }); + + // The tree is gone from its own path the moment dispose returns, moved to + // a trash sibling the log names (the reaper may already have eaten it). + expect(existsSync(path)).toBe(false); + const trash = infos.find((f) => typeof f.trash === "string")!.trash as string; + expect(dirname(trash)).toBe(root); + expect(basename(trash).startsWith(".trash-tree-a-")).toBe(true); + expect(readdirSync(root).every((e) => e.startsWith(".trash-tree-a-"))).toBe(true); + + // The registration, branch, and registry row all went with it. + expect((await listWorktreesAsync(repo))!.some((w) => w.path === path)).toBe(false); + expect(await branchExistsLocalAsync(repo, "feature-a")).toBe(false); + expect(loadRegistry(repoName).length).toBe(0); + expect(events.some((e) => e.type === "worktree:disposed")).toBe(true); + + // …and the detached reaper finishes the delete without anyone awaiting it. + await waitFor(() => readdirSync(root).length === 0); }); test("guard order: dirty is reported before unpushed", async () => { diff --git a/lib/worktree/__tests__/trash.test.ts b/lib/worktree/__tests__/trash.test.ts new file mode 100644 index 00000000..2b737a73 --- /dev/null +++ b/lib/worktree/__tests__/trash.test.ts @@ -0,0 +1,127 @@ +import { describe, test, expect, beforeEach } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { basename, dirname, join } from "path"; +import { + TRASH_PREFIX, + reapTrashDir, + reapTrashInRoots, + trashPathFor, + trashTree, +} from "../trash.ts"; + +function capturingLog(): { log: { warn: (...args: unknown[]) => void }; warns: unknown[][] } { + const warns: unknown[][] = []; + return { log: { warn: (...args: unknown[]) => warns.push(args) }, warns }; +} + +/** A directory standing in for a worktree, with one file inside it. */ +function makeTree(root: string, name: string): string { + const path = join(root, name); + mkdirSync(path, { recursive: true }); + writeFileSync(join(path, "file.txt"), "content\n"); + return path; +} + +describe("worktree trash", () => { + let root: string; + + beforeEach(() => { + // realpathSync: /var -> /private/var on macOS (Global Constraints) + root = realpathSync(mkdtempSync(join(tmpdir(), "rttrash-"))); + }); + + describe("trashPathFor", () => { + test("is a sibling of the tree, prefixed and stamped", () => { + const path = trashPathFor(join(root, "hotel"), "hotel", 1_700_000_000_000); + expect(dirname(path)).toBe(root); + expect(basename(path)).toBe(`${TRASH_PREFIX}hotel-1700000000000`); + }); + + test("two disposals of the same name never collide", () => { + const a = trashPathFor(join(root, "hotel"), "hotel", 1); + const b = trashPathFor(join(root, "hotel"), "hotel", 2); + expect(a).not.toBe(b); + }); + }); + + describe("trashTree", () => { + test("moves the tree out of the way, contents intact", async () => { + const tree = makeTree(root, "hotel"); + + const result = await trashTree(tree, "hotel"); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("expected ok"); + + expect(existsSync(tree)).toBe(false); + expect(existsSync(result.trashPath)).toBe(true); + expect(readFileSync(join(result.trashPath, "file.txt"), "utf8")).toBe("content\n"); + expect(basename(result.trashPath).startsWith(TRASH_PREFIX)).toBe(true); + }); + + test("a path that cannot be renamed reports the failure instead of throwing", async () => { + const result = await trashTree(join(root, "never-existed"), "never-existed"); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected failure"); + expect((result.err as NodeJS.ErrnoException).code).toBe("ENOENT"); + }); + }); + + describe("reapTrashDir", () => { + test("deletes the trash directory and everything under it", async () => { + const tree = makeTree(root, "hotel"); + mkdirSync(join(tree, "node_modules", "dep"), { recursive: true }); + writeFileSync(join(tree, "node_modules", "dep", "index.js"), "//\n"); + const result = await trashTree(tree, "hotel"); + if (!result.ok) throw new Error("expected ok"); + + const { log, warns } = capturingLog(); + await reapTrashDir(result.trashPath, log); + + expect(existsSync(result.trashPath)).toBe(false); + expect(warns.length).toBe(0); + }); + + test("refuses a path that is not a trash directory", async () => { + const tree = makeTree(root, "hotel"); + const { log, warns } = capturingLog(); + + await reapTrashDir(tree, log); + + expect(existsSync(tree)).toBe(true); + expect(warns.length).toBe(1); + }); + }); + + describe("reapTrashInRoots", () => { + test("reaps every trash dir across roots and leaves real trees alone", async () => { + const other = realpathSync(mkdtempSync(join(tmpdir(), "rttrash-other-"))); + const live = makeTree(root, "hotel"); + const stale1 = makeTree(root, `${TRASH_PREFIX}india-1`); + const stale2 = makeTree(root, `${TRASH_PREFIX}juliet-2`); + const stale3 = makeTree(other, `${TRASH_PREFIX}kilo-3`); + + const { log, warns } = capturingLog(); + const reaped = await reapTrashInRoots([root, other], log); + + expect(reaped).toBe(3); + expect(existsSync(stale1)).toBe(false); + expect(existsSync(stale2)).toBe(false); + expect(existsSync(stale3)).toBe(false); + expect(existsSync(live)).toBe(true); + expect(warns.length).toBe(0); + }); + + test("a root that does not exist is not an error", async () => { + const { log, warns } = capturingLog(); + expect(await reapTrashInRoots([join(root, "nope")], log)).toBe(0); + expect(warns.length).toBe(0); + }); + + test("the same root listed twice is swept once", async () => { + makeTree(root, `${TRASH_PREFIX}lima-1`); + const { log } = capturingLog(); + expect(await reapTrashInRoots([root, root], log)).toBe(1); + }); + }); +}); diff --git a/lib/worktree/create.ts b/lib/worktree/create.ts index 79e39e1a..3927f489 100644 --- a/lib/worktree/create.ts +++ b/lib/worktree/create.ts @@ -9,6 +9,7 @@ * (worktree + on-deck branch + registry row) and reports typed detail. */ +import { existsSync } from "fs"; import { join } from "path"; import { loadRegistry, @@ -27,14 +28,11 @@ import { pickName } from "./names.ts"; import { loadWorktreeRepoConfig, resolveReadySteps, type WorktreeRepoConfig } from "./config.ts"; import { runReadySteps } from "./ready.ts"; import { withTreeLock } from "./locks.ts"; +import { reapTrashDir, trashTree } from "./trash.ts"; import { reconcileForRepo } from "../daemon/doppler-sync.ts"; const CREATE_TIMEOUT_MS = 5 * 60_000; -/** `git worktree remove` deletes node_modules by hand; a pnpm-scale tree on - * APFS routinely outruns runGit's 60s default. */ -const REMOVE_TIMEOUT_MS = 5 * 60_000; - /** Longest slice of a failed step's output carried into the log line. */ const MAX_LOGGED_OUTPUT = 2000; @@ -160,15 +158,29 @@ async function runCreate( } /** - * Force-remove the worktree, delete its on-deck/ branch, and prune the - * registry entry. Tolerant of partial existence: the worktree may not exist - * yet (git worktree add never ran or failed before creating it), and the - * branch may not exist either. + * Get rid of a half-built tree: rename it into trash (see trash.ts) with a + * detached reap behind it, prune the registration, delete its on-deck/ + * branch, and drop the registry entry. Tolerant of partial existence — the + * worktree may not exist yet (git worktree add never ran or failed before + * creating it), and the branch may not exist either — and, since it renames + * rather than asking git to unlink, it returns instantly however far the + * install got before the create failed. */ export async function scrapTree(deps: CreateDeps, rec: TreeRecord): Promise { - await runGit(deps.repoPath, ["worktree", "remove", "--force", rec.path], { - timeoutMs: REMOVE_TIMEOUT_MS, - }); + const trashed = await trashTree(rec.path, rec.name); + if (trashed.ok) { + void reapTrashDir(trashed.trashPath, deps.log); + } else if (existsSync(rec.path)) { + // Absent is the normal case here (`git worktree add` may never have run); + // present-and-unrenameable is the one worth a line. + deps.log.warn( + { repo: deps.repoName, tree: rec.name, path: rec.path, err: trashed.err }, + "worktree scrap: trash rename failed", + ); + } + // Collects the registration whose directory just went missing. Unconditional: + // scrap is the tolerant path, and every step below runs on best effort. + await runGit(deps.repoPath, ["worktree", "prune"]); if (rec.branch) { await runGit(deps.repoPath, ["branch", "-D", rec.branch]); } diff --git a/lib/worktree/dispose.ts b/lib/worktree/dispose.ts index e69391a0..1dd3857b 100644 --- a/lib/worktree/dispose.ts +++ b/lib/worktree/dispose.ts @@ -18,16 +18,13 @@ import { hasFreshAttendantLease } from "./lease.ts"; import { loadSyncConfig, matchRule } from "../sync-config.ts"; import { repoDataDir } from "../rt-paths.ts"; import { killWorktreeProcesses } from "../daemon/worktree-process-kill.ts"; +import { reapTrashDir, trashTree } from "./trash.ts"; /** Merge-reactor disposals ignore claims younger than this (stale-event protection). */ const GRACE_MS = 10 * 60_000; const FETCH_TIMEOUT_MS = 2 * 60_000; -/** `git worktree remove` deletes node_modules by hand; a pnpm-scale tree on - * APFS routinely outruns runGit's 60s default. */ -const REMOVE_TIMEOUT_MS = 5 * 60_000; - // ─── Dirty classification (harvested from parking-lot.ts, execSync → runGit) ── /** One entry from `git status --porcelain`. */ @@ -252,26 +249,26 @@ export async function disposeTree( } } - // --force here is plumbing, not a safety statement: `git worktree remove` - // refuses on any untracked file, and the guard above (or the caller's - // explicit force) already made the decision. - const removal = await runGit(repoPath, ["worktree", "remove", "--force", rec.path], { - timeoutMs: REMOVE_TIMEOUT_MS, - }); - if (removal.exitCode !== 0) { - const output = (removal.stdout + removal.stderr).trim(); + // One atomic rename, not a recursive unlink: see trash.ts. Everything below + // is fast, so the verb returns in seconds however large the tree was. + const trashed = await trashTree(rec.path, rec.name); + if (!trashed.ok) { log.warn( - { repo: repoName, tree: rec.name, path: rec.path, output }, - "git worktree remove failed during dispose", + { repo: repoName, tree: rec.name, path: rec.path, err: trashed.err }, + "worktree trash rename failed during dispose", ); // A directory that is already gone is the expected failure and disposal // continues (the registry row is the thing left to clean up). A tree still - // on disk means removal genuinely failed — locked file, permissions — and - // pruning the registry there would orphan a real worktree with its - // metadata lost. Refuse instead; the caller retries or forces. + // at rec.path means the rename genuinely failed — held directory, + // permissions — and pruning the registry there would orphan a real + // worktree with its metadata lost. Refuse instead; the caller retries. if (existsSync(rec.path)) return refuse("remove-failed"); } + // The registration now points at a path that no longer exists, which is + // exactly what prune collects. + await runGit(repoPath, ["worktree", "prune"]); + if (rec.branch) { // Already-gone branches are expected (the reactor disposes after a merge // that may have deleted it), so a failure here is not worth a warning. @@ -287,7 +284,19 @@ export async function disposeTree( branch: rec.branch, discarded, }); - log.info({ repo: repoName, tree: rec.name, path: rec.path }, "worktree disposed"); + log.info( + { + repo: repoName, + tree: rec.name, + path: rec.path, + ...(trashed.ok ? { trash: trashed.trashPath } : {}), + }, + "worktree disposed", + ); + + // Fire-and-forget: the bytes go away on their own time, and a trash dir this + // process never gets to finish is swept by the reconciler's reap duty. + if (trashed.ok) void reapTrashDir(trashed.trashPath, log); return { disposed: true }; } diff --git a/lib/worktree/trash.ts b/lib/worktree/trash.ts new file mode 100644 index 00000000..3f51545d --- /dev/null +++ b/lib/worktree/trash.ts @@ -0,0 +1,119 @@ +/** + * Trash-and-reap: how rt makes a worktree directory go away. + * + * `git worktree remove` unlinks the tree file by file. On a pnpm-scale + * node_modules that is minutes of syscalls, and it runs inline in whatever + * verb asked for the disposal — so `rt worktree dispose` blocked for as long + * as the tree was big, and the timeout that bounded it killed the unlink + * mid-flight, leaving a half-deleted directory that was neither a worktree nor + * gone. + * + * So disposal renames instead: one same-volume rename into a sibling + * `.trash--` directory. That is atomic and instant regardless of + * tree size, and it either happened or it didn't — never half. Everything + * after it (prune, branch -D, registry) is fast and keys off the same fact: + * the tree dir is no longer at rec.path. Verb latency becomes O(seconds). + * + * The real unlink is a detached `rm -rf` nobody waits on and nothing times + * out. If that process (or the whole daemon) dies mid-delete, the leftover is + * a `.trash-*` directory, which is exactly what the reconciler's reap duty + * sweeps on a later pass — a crash costs disk, never correctness. + */ + +import { readdir, rename } from "fs/promises"; +import { basename, dirname, join } from "path"; + +/** Marks a directory as rt's to delete. Nothing without this prefix is ever reaped. */ +export const TRASH_PREFIX = ".trash-"; + +/** Minimal log surface: pino's, and the plain object the CLI paths pass. */ +export interface TrashLog { + warn: (...args: unknown[]) => void; +} + +export type TrashResult = { ok: true; trashPath: string } | { ok: false; err: unknown }; + +/** + * The sibling directory `path` gets renamed to. A sibling (not a shared trash + * root) so the rename never crosses a filesystem, which is what makes it + * atomic and instant; the epoch stamp keeps repeat disposals of the same tree + * name from colliding. + */ +export function trashPathFor(path: string, name: string, now: number = Date.now()): string { + return join(dirname(path), `${TRASH_PREFIX}${name}-${now}`); +} + +/** + * Rename the tree out of the way. Never throws: a busy or unwritable directory + * comes back as `{ ok: false }` and the caller decides — dispose refuses + * ("remove-failed"), scrap shrugs, because mid-create the tree may not exist + * at all. + */ +export async function trashTree(path: string, name: string): Promise { + const trashPath = trashPathFor(path, name); + try { + await rename(path, trashPath); + return { ok: true, trashPath }; + } catch (err) { + return { ok: false, err }; + } +} + +/** + * Delete a trash directory in a detached `rm -rf` with no timeout: nothing + * upstream waits for it, and a kill mid-unlink is the very failure mode this + * design exists to remove. The returned promise settles when the child does, + * for the reconciler's one-at-a-time sweep (and for tests); callers on the + * dispose path drop it. + * + * Refuses any path not named `.trash-*` — the one guard between this and an + * `rm -rf` of a live worktree. + */ +export async function reapTrashDir(trashPath: string, log: TrashLog): Promise { + if (!basename(trashPath).startsWith(TRASH_PREFIX)) { + log.warn({ path: trashPath }, "worktree reap refused: not a trash directory"); + return; + } + + try { + const proc = Bun.spawn(["rm", "-rf", "--", trashPath], { + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + // Survives the CLI process that started it: the point is that no caller + // has to stay alive for the delete to finish. + detached: true, + }); + proc.unref(); + const exitCode = await proc.exited; + if (exitCode !== 0) { + log.warn({ path: trashPath, exitCode }, "worktree trash reap failed"); + } + } catch (err) { + log.warn({ err, path: trashPath }, "worktree trash reap could not be spawned"); + } +} + +/** + * Sweep every `.trash-*` directory out of `roots`, one at a time (a reap is + * IO-bound and there is never a hurry — this is the crash-leftover path, not + * the dispose path). A root that does not exist is normal, not an error: a + * repo may simply never have had a worktree. Returns how many were reaped. + */ +export async function reapTrashInRoots(roots: string[], log: TrashLog): Promise { + let reaped = 0; + for (const root of new Set(roots)) { + let entries: string[]; + try { + entries = await readdir(root); + } catch { + continue; // no such root yet + } + for (const entry of entries) { + if (!entry.startsWith(TRASH_PREFIX)) continue; + await reapTrashDir(join(root, entry), log); + reaped += 1; + } + } + return reaped; +} From d9e3d0510ca73aa5fd2e3cae6639d8d5c45b766e Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 18 Aug 2026 12:55:18 -0500 Subject: [PATCH 4/7] RT-41: reject dot-leading name-pool entries; drop dead test fixture The reconciler's reap duty is the only rm -rf in the codebase, and it fires at anything named `.trash-*` in a worktree root. A repo config declaring a namePool entry like ".trash-x" would build a tree at exactly that name and the next reconciler pass would delete it. loadWorktreeRepoConfig now filters dot-leading entries out of the pool, closing the one door into the reaper. Also drops a `.trash-kilo-*` seeding that a bad copy-paste left in the "dirty non-idle main is left untouched" freshen test, which never asserted on it. Co-Authored-By: Claude Fable 5 --- lib/daemon/__tests__/worktree-reconciler.test.ts | 3 --- lib/worktree/__tests__/config.test.ts | 12 ++++++++++++ lib/worktree/config.ts | 4 +++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/daemon/__tests__/worktree-reconciler.test.ts b/lib/daemon/__tests__/worktree-reconciler.test.ts index 9aa83c2c..ba46c90f 100644 --- a/lib/daemon/__tests__/worktree-reconciler.test.ts +++ b/lib/daemon/__tests__/worktree-reconciler.test.ts @@ -923,9 +923,6 @@ describe("freshen", () => { { name: basename(repo), path: repo, kind: "main", branch: "main", createdAt: new Date().toISOString() }, ]); - const seededTrash = join(repo, ".worktrees", ".trash-kilo-1700000000002"); - mkdirSync(seededTrash, { recursive: true }); - const beforeSha = await headSha(repo); const events: Array<{ type: string; data: any }> = []; await __test__.freshenRepo({ diff --git a/lib/worktree/__tests__/config.test.ts b/lib/worktree/__tests__/config.test.ts index ecf033a2..387ce7b2 100644 --- a/lib/worktree/__tests__/config.test.ts +++ b/lib/worktree/__tests__/config.test.ts @@ -79,6 +79,18 @@ describe("worktree config", () => { expect(cfg.root).toBe(join(process.env.HOME!, "wt-root")); }); + test("drops dot-leading namePool entries", () => { + // A pool entry named ".trash-x" would build a tree the reconciler's reap + // duty then deletes as a leftover. The reaper is the only rm -rf in the + // codebase; this is the door it can come through. + const repoPath = tmpRepoPath("rtcfg-repo-"); + writeJson(join(repoDataDir("myrepo"), "config.json"), { + worktrees: { namePool: [".trash-x", "luna"] }, + }); + const cfg = loadWorktreeRepoConfig("myrepo", repoPath); + expect(cfg.namePool).toEqual(["luna"]); + }); + test("leaves an absolute root unchanged", () => { const repoPath = tmpRepoPath("rtcfg-repo-"); writeJson(join(repoDataDir("myrepo"), "config.json"), { diff --git a/lib/worktree/config.ts b/lib/worktree/config.ts index a800e35c..85b5dfc2 100644 --- a/lib/worktree/config.ts +++ b/lib/worktree/config.ts @@ -71,7 +71,9 @@ export function loadWorktreeRepoConfig(repoName: string, repoPath: string): Work branchFormat: declared.branchFormat ?? "-", ready: declared.ready ?? [], }; - if (declared.namePool) cfg.namePool = declared.namePool; + // A dot-leading name would build a tree the reconciler's reap duty then + // deletes as a `.trash-*` leftover, so the pool never gets to declare one. + if (declared.namePool) cfg.namePool = declared.namePool.filter((n) => !n.startsWith(".")); return cfg; } From c4590296ad9cafcfcffacc89b9b51a4d1e65c894 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 18 Aug 2026 13:10:26 -0500 Subject: [PATCH 5/7] =?UTF-8?q?RT-41/42:=20review=20fixes=20=E2=80=94=20en?= =?UTF-8?q?v=20options=20in=20install=20dedup,=20trash=20name=20validation?= =?UTF-8?q?,=20sweep=20warns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - stripEnvPrefix consumes env(1) options (-i, -u NAME, --long) so `env -i ... pnpm install` still suppresses the implicit install, and the dedup matches " install" on a token boundary so `pnpm installer` no longer counts as an install. - trashPathFor rejects names containing path separators; trashTree keeps its non-throwing contract by validating inside the try. - reapTrashInRoots only swallows ENOENT; other readdir failures warn with the root path. Co-Authored-By: Claude Fable 5 --- lib/worktree/__tests__/config.test.ts | 40 +++++++++++++++++++++ lib/worktree/__tests__/trash.test.ts | 24 +++++++++++++ lib/worktree/config.ts | 51 +++++++++++++++++++-------- lib/worktree/trash.ts | 17 ++++++--- 4 files changed, 114 insertions(+), 18 deletions(-) diff --git a/lib/worktree/__tests__/config.test.ts b/lib/worktree/__tests__/config.test.ts index 387ce7b2..3adcbeed 100644 --- a/lib/worktree/__tests__/config.test.ts +++ b/lib/worktree/__tests__/config.test.ts @@ -255,6 +255,35 @@ describe("worktree config", () => { expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); }); + test("an `env -i` prefix on the declared install still suppresses the implicit one", () => { + const repoPath = tmpRepoPath("rtcfg-resolve8-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "env -i PATH=/usr/bin pnpm install" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual(cfg.ready); + }); + + test("a command that merely starts with the letters `install` does not suppress it", () => { + const repoPath = tmpRepoPath("rtcfg-resolve9-"); + writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(repoPath, "pnpm-lock.yaml"), ""); + const cfg: WorktreeRepoConfig = { + onDeck: 0, + root: join(repoPath, ".worktrees"), + branchFormat: "-", + ready: [{ run: "pnpm installer" }], + }; + expect(resolveReadySteps(cfg, repoPath)).toEqual([ + { run: "pnpm install", when: "changed:pnpm-lock.yaml" }, + { run: "pnpm installer" }, + ]); + }); + test("an env-var prefix on a NON-install step does not suppress the implicit install", () => { const repoPath = tmpRepoPath("rtcfg-resolve7-"); writeFileSync(join(repoPath, "package.json"), JSON.stringify({ name: "x" })); @@ -289,6 +318,17 @@ describe("worktree config", () => { expect(stripEnvPrefix("env FOO=bar pnpm install")).toBe("pnpm install"); }); + test("strips env's own options, including ones that take an argument", () => { + expect(stripEnvPrefix("env -i PATH=/usr/bin pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("env --ignore-environment pnpm install")).toBe("pnpm install"); + expect(stripEnvPrefix("env -u FOO pnpm install")).toBe("pnpm install"); + }); + + test("never eats a command's own flags — options only strip after `env`", () => { + expect(stripEnvPrefix("pnpm -r install")).toBe("pnpm -r install"); + expect(stripEnvPrefix("A=1 pnpm --filter x install")).toBe("pnpm --filter x install"); + }); + test("tolerates quoted assignment values containing spaces", () => { expect(stripEnvPrefix('FOO="a b" pnpm install')).toBe("pnpm install"); }); diff --git a/lib/worktree/__tests__/trash.test.ts b/lib/worktree/__tests__/trash.test.ts index 2b737a73..0bfa6d91 100644 --- a/lib/worktree/__tests__/trash.test.ts +++ b/lib/worktree/__tests__/trash.test.ts @@ -38,6 +38,14 @@ describe("worktree trash", () => { expect(basename(path)).toBe(`${TRASH_PREFIX}hotel-1700000000000`); }); + test("rejects a name containing a path separator", () => { + expect(() => trashPathFor(join(root, "x"), "x/../../outside")).toThrow( + /single path component/, + ); + expect(() => trashPathFor(join(root, "x"), "x\\y")).toThrow(/single path component/); + expect(() => trashPathFor(join(root, "x"), "")).toThrow(/single path component/); + }); + test("two disposals of the same name never collide", () => { const a = trashPathFor(join(root, "hotel"), "hotel", 1); const b = trashPathFor(join(root, "hotel"), "hotel", 2); @@ -59,6 +67,13 @@ describe("worktree trash", () => { expect(basename(result.trashPath).startsWith(TRASH_PREFIX)).toBe(true); }); + test("a separator-containing name reports the failure without renaming", async () => { + const tree = makeTree(root, "sierra"); + const result = await trashTree(tree, "sierra/../escape"); + expect(result.ok).toBe(false); + expect(existsSync(tree)).toBe(true); + }); + test("a path that cannot be renamed reports the failure instead of throwing", async () => { const result = await trashTree(join(root, "never-existed"), "never-existed"); expect(result.ok).toBe(false); @@ -118,6 +133,15 @@ describe("worktree trash", () => { expect(warns.length).toBe(0); }); + test("a root that cannot be read (not just missing) is warned about", async () => { + const file = join(root, "not-a-dir"); + writeFileSync(file, ""); + const { log, warns } = capturingLog(); + expect(await reapTrashInRoots([file], log)).toBe(0); + expect(warns.length).toBe(1); + expect(warns[0]?.[0]).toMatchObject({ root: file }); + }); + test("the same root listed twice is swept once", async () => { makeTree(root, `${TRASH_PREFIX}lima-1`); const { log } = capturingLog(); diff --git a/lib/worktree/config.ts b/lib/worktree/config.ts index 85b5dfc2..b5ef5901 100644 --- a/lib/worktree/config.ts +++ b/lib/worktree/config.ts @@ -132,23 +132,44 @@ export function resolveImplicitInstall(repoPath: string): ReadyStep | null { return manager ? MANAGER_STEP[manager] : null; } -/** One leading `env` word, or one `VAR=value` assignment (value optionally quoted). */ -const ENV_PREFIX_TOKEN = /^(?:env|[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|\S*))\s+/; +/** One leading `VAR=value` assignment (value optionally quoted). */ +const ASSIGNMENT_TOKEN = /^[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|\S*)\s+/; +const ENV_WORD = /^env\s+/; +const OPTION_TOKEN = /^(-\S*)\s+/; +/** env(1) short options whose argument is the NEXT token (`-u NAME` etc.). */ +const ENV_ARG_OPTIONS = new Set(["-u", "-C", "-P", "-S"]); /** * Drop a shell env prefix from a declared run, leaving the command word first: - * `SKIP_GEN_TYPES=1 pnpm install` → `pnpm install`, `env FOO=bar pnpm install` - * → `pnpm install`. Purely for recognising WHICH command a step runs (the - * install-dedup test below); the step itself is always executed verbatim, env - * prefix included. Requires trailing whitespace, so a run that is nothing but - * an assignment (`A=1`) is left alone rather than emptied. + * `SKIP_GEN_TYPES=1 pnpm install` → `pnpm install`, `env -i FOO=bar pnpm + * install` → `pnpm install`. Purely for recognising WHICH command a step runs + * (the install-dedup test below); the step itself is always executed verbatim, + * env prefix included. Option words are only consumed after an `env`, so a + * command's own flags are never eaten. Every token match requires trailing + * whitespace, so a run that is nothing but a prefix (`A=1`, `env`) is left + * alone rather than emptied. */ export function stripEnvPrefix(run: string): string { let rest = run.trimStart(); - while (ENV_PREFIX_TOKEN.test(rest)) { - rest = rest.replace(ENV_PREFIX_TOKEN, ""); + let inEnv = false; + for (;;) { + if (ASSIGNMENT_TOKEN.test(rest)) { + rest = rest.replace(ASSIGNMENT_TOKEN, ""); + continue; + } + if (ENV_WORD.test(rest)) { + rest = rest.replace(ENV_WORD, ""); + inEnv = true; + continue; + } + const option = inEnv ? rest.match(OPTION_TOKEN) : null; + if (option) { + rest = rest.slice(option[0].length); + if (ENV_ARG_OPTIONS.has(option[1] ?? "")) rest = rest.replace(/^\S+\s+/, ""); + continue; + } + return rest; } - return rest; } /** @@ -166,10 +187,12 @@ export function resolveReadySteps(cfg: WorktreeRepoConfig, repoPath: string): Re const manager = detectManager(repoPath); if (!manager) return cfg.ready; - const installPrefix = `${manager} install`; - const alreadyDeclared = cfg.ready.some((step) => - stripEnvPrefix(step.run).startsWith(installPrefix), - ); + const installWords = `${manager} install`; + const alreadyDeclared = cfg.ready.some((step) => { + const command = stripEnvPrefix(step.run); + // Whole-token match: `pnpm install --flag` counts, `pnpm installer` does not. + return command === installWords || command.startsWith(`${installWords} `); + }); if (alreadyDeclared) return cfg.ready; return [MANAGER_STEP[manager], ...cfg.ready]; diff --git a/lib/worktree/trash.ts b/lib/worktree/trash.ts index 3f51545d..6a2ba373 100644 --- a/lib/worktree/trash.ts +++ b/lib/worktree/trash.ts @@ -37,9 +37,14 @@ export type TrashResult = { ok: true; trashPath: string } | { ok: false; err: un * The sibling directory `path` gets renamed to. A sibling (not a shared trash * root) so the rename never crosses a filesystem, which is what makes it * atomic and instant; the epoch stamp keeps repeat disposals of the same tree - * name from colliding. + * name from colliding. A name with a path separator could steer the rename + * outside the root (and past the basename guard in reapTrashDir), so it is + * rejected outright. */ export function trashPathFor(path: string, name: string, now: number = Date.now()): string { + if (!name || name.includes("/") || name.includes("\\")) { + throw new Error(`worktree trash name must be a single path component: ${JSON.stringify(name)}`); + } return join(dirname(path), `${TRASH_PREFIX}${name}-${now}`); } @@ -50,8 +55,8 @@ export function trashPathFor(path: string, name: string, now: number = Date.now( * at all. */ export async function trashTree(path: string, name: string): Promise { - const trashPath = trashPathFor(path, name); try { + const trashPath = trashPathFor(path, name); await rename(path, trashPath); return { ok: true, trashPath }; } catch (err) { @@ -106,8 +111,12 @@ export async function reapTrashInRoots(roots: string[], log: TrashLog): Promise< let entries: string[]; try { entries = await readdir(root); - } catch { - continue; // no such root yet + } catch (err) { + // A root that never existed is normal; anything else hides stale trash. + if ((err as NodeJS.ErrnoException)?.code !== "ENOENT") { + log.warn({ err, root }, "worktree trash sweep could not read root"); + } + continue; } for (const entry of entries) { if (!entry.startsWith(TRASH_PREFIX)) continue; From 9052d5b066f8ea2c99cc55675e242168e13eaa3b Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Tue, 18 Aug 2026 13:14:59 -0500 Subject: [PATCH 6/7] RT-41: scrap keeps branch and record when the trash rename fails Dropping the registry row while the tree was still on disk stranded a live directory as unmanaged. Returning after the warn leaves the creating record in place, and the reconciler's orphaned-creating pass retries the scrap until the rename goes through. Co-Authored-By: Claude Fable 5 --- lib/daemon/worktree-reconciler.ts | 4 +++- lib/worktree/__tests__/create.test.ts | 30 ++++++++++++++++++++++++++- lib/worktree/create.ts | 6 +++++- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/lib/daemon/worktree-reconciler.ts b/lib/daemon/worktree-reconciler.ts index 046d53dd..d85fe741 100644 --- a/lib/daemon/worktree-reconciler.ts +++ b/lib/daemon/worktree-reconciler.ts @@ -163,7 +163,9 @@ async function reconcilePass(deps: ReconcileDeps, attempt: number): Promise { expect(loadRegistry(repoName).length).toBe(0); }); + test("a rename that fails with the tree still on disk keeps branch and record for retry", async () => { + const path = join(repo, ".worktrees", "stuck"); + execSync(`git -C ${repo} worktree add -b on-deck/stuck ${path}`, { shell: "/bin/zsh", stdio: "pipe" }); + const rec: TreeRecord = { + name: "stuck", + path, + kind: "ephemeral", + state: "creating", + branch: "on-deck/stuck", + createdAt: new Date().toISOString(), + }; + saveRegistry(repoName, [rec]); + + // A read-only parent makes the trash rename fail while the tree remains. + const worktreesRoot = join(repo, ".worktrees"); + chmodSync(worktreesRoot, 0o555); + try { + await scrapTree(makeDeps(repoName, repo, events), rec); + } finally { + chmodSync(worktreesRoot, 0o755); + } + + expect(existsSync(path)).toBe(true); + expect(loadRegistry(repoName)).toEqual([rec]); + const branches = execSync(`git -C ${repo} branch --list on-deck/stuck`, { shell: "/bin/zsh" }).toString(); + expect(branches).toContain("on-deck/stuck"); + }); + test("scraps a tree git itself would refuse to remove", async () => { // A locked worktree makes `git worktree remove --force` refuse (it wants // --force twice). A rename does not care: a mid-create scrap must always diff --git a/lib/worktree/create.ts b/lib/worktree/create.ts index 3927f489..99e7c773 100644 --- a/lib/worktree/create.ts +++ b/lib/worktree/create.ts @@ -172,11 +172,15 @@ export async function scrapTree(deps: CreateDeps, rec: TreeRecord): Promise Date: Tue, 18 Aug 2026 13:21:41 -0500 Subject: [PATCH 7/7] RT-41: deterministic rename failure in the scrap-retry test A pre-created non-empty trash destination (frozen Date.now) fails the rename with ENOTEMPTY regardless of uid, where the read-only parent approach silently passes under root. Co-Authored-By: Claude Fable 5 --- lib/worktree/__tests__/create.test.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/worktree/__tests__/create.test.ts b/lib/worktree/__tests__/create.test.ts index 4868d6d1..70b7440d 100644 --- a/lib/worktree/__tests__/create.test.ts +++ b/lib/worktree/__tests__/create.test.ts @@ -1,6 +1,6 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { execSync } from "child_process"; -import { chmodSync, existsSync, mkdtempSync, readFileSync, realpathSync } from "fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync } from "fs"; import { tmpdir } from "os"; import { join } from "path"; import { writeJson } from "../../json-store.ts"; @@ -158,13 +158,17 @@ describe("scrapTree", () => { }; saveRegistry(repoName, [rec]); - // A read-only parent makes the trash rename fail while the tree remains. - const worktreesRoot = join(repo, ".worktrees"); - chmodSync(worktreesRoot, 0o555); + // A non-empty directory already at the trash destination makes the rename + // fail (ENOTEMPTY) while the tree remains — deterministic even as root, + // unlike a read-only parent. Freeze Date.now so the destination is known. + const stamp = 1234567890; + const realNow = Date.now; + Date.now = () => stamp; + mkdirSync(join(repo, ".worktrees", `.trash-stuck-${stamp}`, "occupied"), { recursive: true }); try { await scrapTree(makeDeps(repoName, repo, events), rec); } finally { - chmodSync(worktreesRoot, 0o755); + Date.now = realNow; } expect(existsSync(path)).toBe(true);