RT-41/42/43: rename-then-reap dispose, env-prefix install dedup, plain-install default

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}`);
-
- 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");
+ // 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);
+ }
 
- sh(`git -C ${repo} worktree unlock ${rec.path}`);
  await detect(mrCache("feat-hotel", "merged"));
 
  expect(existsSync(rec.path)).toBe(false);

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<string, any> };
@@ -162,7 +163,9 @@ async function reconcilePass(deps: ReconcileDeps, attempt: number): Promise<Pass
 
  if (scrapped) {
  // scrapTree persists its own removal (fresh-load → filter → save), so the
- // scrap is already on disk and has already bumped the epoch. Re-read from
+ // scrap is already on disk and has already bumped the epoch. (A scrap
+ // whose rename failed keeps its record for retry and writes nothing; the
+ // re-read below is correct either way.) Re-read from
  // that write instead of carrying the pre-scrap snapshot forward: anything
  // another writer landed during the scrap's git awaits is in the file now,
  // and re-capturing the epoch here is what keeps our own intentional write
@@ -958,6 +961,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<void> {
+ 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 +1053,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 +1086,7 @@ export function createWorktreeReconciler(deps: ReconcilerDeps): {
 
 export const __test__ = {
  detectTransitions,
+ reapRepoTrash,
  reactorStatePath,
  freshenRepo,
  replenishAndShrink,

lib/worktree/__tests__/config.test.ts

@@ -8,6 +8,7 @@ import {
  loadWorktreeRepoConfig,
  resolveImplicitInstall,
  resolveReadySteps,
+ stripEnvPrefix,
  loadWorktreeAppConfig,
  type WorktreeRepoConfig,
 } from "../config.ts";
@@ -78,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"), {
@@ -101,7 +114,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 +144,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 +181,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 +213,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" },
  ]);
  });
@@ -215,6 +228,115 @@ 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: "<ticket>-<slug>",
+ 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: "<ticket>-<slug>",
+ ready: [{ run: "env FOO=bar pnpm install" }],
+ };
+ 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: "<ticket>-<slug>",
+ 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: "<ticket>-<slug>",
+ 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" }));
+ writeFileSync(join(repoPath, "pnpm-lock.yaml"), "");
+ const cfg: WorktreeRepoConfig = {
+ onDeck: 0,
+ root: join(repoPath, ".worktrees"),
+ branchFormat: "<ticket>-<slug>",
+ 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("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");
+ });
+
+ test("never eats the command itself", () => {
+ expect(stripEnvPrefix("A=1")).toBe("A=1");
+ expect(stripEnvPrefix("env")).toBe("env");
+ });
  });
 
  describe("loadWorktreeAppConfig", () => {