harden(guard-push): never force-delete a scratch checkout that still holds a borrowed node_modules link

scripts/guard-push.mjs

@@ -60,7 +60,7 @@
  */
 import { execFileSync } from "node:child_process";
 import { createHash } from "node:crypto";
-import { existsSync, mkdtempSync, readFileSync, rmSync, symlinkSync } from "node:fs";
+import { existsSync, lstatSync, mkdtempSync, readFileSync, rmdirSync, rmSync, symlinkSync, unlinkSync } from "node:fs";
 import { tmpdir } from "node:os";
 import path from "node:path";
 import { fileURLToPath, pathToFileURL } from "node:url";
@@ -502,6 +502,13 @@ function worktreeRoots() {
  * guard must work before that junction exists, but accepting an arbitrary
  * sibling would let a stale Prettier version disagree with CI. Byte-identical
  * lockfiles plus the installed package version make the fallback deterministic.
+ *
+ * The borrow is deliberately NOT gated on whether the donor worktree is idle.
+ * "Actively in use" is not cheaply detectable — an in-flight `npm ci` in another
+ * process leaves nothing this can read — and a heuristic that misfires would
+ * silently disable the format guard. The borrow is read-only; the deletion risk
+ * lived entirely in the scratch checkout's teardown, which unlinkDependencyLink
+ * and cleanupFormatCheckout now handle without following the link.
  */
 export function findPrettierBin(projectRoot, candidateRoots) {
   const lockPath = path.join(projectRoot, "package-lock.json");
@@ -642,15 +649,113 @@ function checkPushedCommit(prettierBin, sha, files) {
     }
     return { verdict: "formatted" };
   } finally {
-    const modulesPath = path.join(dir, "node_modules");
-    try {
-      if (existsSync(modulesPath)) rmSync(modulesPath, { recursive: false, force: true });
-    } catch {
-      // Continue cleanup even if unlinking junction throws
-    }
-    tryGit(["worktree", "remove", "--force", dir]);
-    rmSync(dir, { recursive: true, force: true });
+    cleanupFormatCheckout(dir);
+  }
+}
+
+/**
+ * Remove the linked dependency tree from a scratch checkout WITHOUT following it.
+ *
+ * That link is not necessarily this worktree's own `node_modules`. When the
+ * pushing worktree has none, findPrettierBin deliberately borrows ANOTHER
+ * worktree's real tree, and on Windows the borrow is linked in as a junction.
+ * Everything cleanupFormatCheckout runs afterwards — `git worktree remove
+ * --force` and a recursive rmSync — is a force-delete over the directory holding
+ * that link, and neither respects `git worktree lock`. So the link is removed
+ * first, by a call that operates on the link itself rather than on what it points
+ * at.
+ *
+ * lstat, not existsSync: existsSync FOLLOWS the link, so once the borrowed tree
+ * has gone away the link reads as absent and an existsSync-gated cleanup skips
+ * it — leaving it in place for the force-deletes to interpret instead. lstat
+ * sees the link whether or not it still resolves.
+ *
+ * unlink removes a junction and a POSIX directory symlink alike, and Windows can
+ * want rmdir for a directory reparse point; rmdir on a junction also removes the
+ * link, never its target. Neither call descends, and rmSync({recursive:true}) is
+ * never used on this path under any branch.
+ *
+ * A real directory here is refused outright. Nothing in guard-push creates one,
+ * so one means something unexpected — and recursively deleting an unexpected
+ * directory is precisely the outcome this function exists to prevent.
+ *
+ * @returns {{removed: boolean, reason: "unlink"|"rmdir"|"absent"|"not-a-link"|"unreadable"|"failed", detail?: string}}
+ */
+export function unlinkDependencyLink(linkPath) {
+  let stats;
+  try {
+    stats = lstatSync(linkPath);
+  } catch (error) {
+    if (error?.code === "ENOENT") return { removed: false, reason: "absent" };
+    return { removed: false, reason: "unreadable", detail: describeError(error) };
+  }
+  if (!stats.isSymbolicLink()) return { removed: false, reason: "not-a-link" };
+  try {
+    unlinkSync(linkPath);
+    return { removed: true, reason: "unlink" };
+  } catch {
+    // A directory reparse point can refuse unlink on Windows; rmdir removes the
+    // link itself in that case, and still never touches the target.
+  }
+  try {
+    rmdirSync(linkPath);
+    return { removed: true, reason: "rmdir" };
+  } catch (error) {
+    return { removed: false, reason: "failed", detail: describeError(error) };
+  }
+}
+
+function describeError(error) {
+  return error instanceof Error ? error.message : String(error);
+}
+
+/**
+ * Tear down a format-check scratch checkout, link first.
+ *
+ * The ordering is the point: the link is gone before `git worktree remove
+ * --force` and before the recursive delete, so whether either of those can
+ * traverse a junction never has to be relied upon.
+ *
+ * When the link could NOT be removed, neither force-delete runs. Swallowing that
+ * failure and continuing is the single case where a force-delete would run over
+ * a directory still holding a live link into another worktree's node_modules. A
+ * leftover scratch directory is cheap and the `git worktree prune` at the top of
+ * checkPushedCommit clears its registration on the next push; the alternative is
+ * not cheap.
+ *
+ * Dependencies are injectable so the ordering and the skip are unit-testable
+ * without creating and destroying real worktrees.
+ */
+export function cleanupFormatCheckout(
+  dir,
+  {
+    unlink = unlinkDependencyLink,
+    removeWorktree = (target) => tryGit(["worktree", "remove", "--force", target]),
+    removeDir = (target) => rmSync(target, { recursive: true, force: true }),
+    log = console.error,
+  } = {},
+) {
+  const linkPath = path.join(dir, "node_modules");
+  const result = unlink(linkPath);
+  if (!result.removed && result.reason !== "absent") {
+    log(
+      "[guard-push] left " +
+        dir +
+        " in place: could not remove the linked dependency tree at " +
+        linkPath +
+        " (" +
+        result.reason +
+        (result.detail ? ": " + result.detail : "") +
+        ").\n" +
+        "  Skipping the force worktree removal and the recursive delete — neither may run over a " +
+        "directory that still holds a live link into another worktree's node_modules.\n" +
+        "  Remove it by hand once nothing is using it.",
+    );
+    return result;
   }
+  removeWorktree(dir);
+  removeDir(dir);
+  return result;
 }
 
 function chunk(items, size) {

tests/guard-push.test.ts

@@ -1,5 +1,14 @@
 import { execFileSync } from "node:child_process";
-import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import {
+  existsSync,
+  lstatSync,
+  mkdtempSync,
+  mkdirSync,
+  readFileSync,
+  rmSync,
+  symlinkSync,
+  writeFileSync,
+} from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { afterEach, describe, expect, it } from "vitest";
@@ -34,6 +43,8 @@ import {
   pushedBranchNames,
   pushedTipMatchesHead,
   staticGuard,
+  cleanupFormatCheckout,
+  unlinkDependencyLink,
 } from "../scripts/guard-push.mjs";
 
 const ZERO = "0".repeat(40);
@@ -562,3 +573,134 @@ describe("in-flight CI push guard (#HSSHRG)", () => {
     expect(Number(capturedArgs[limitIndex + 1])).toBeGreaterThan(10);
   });
 });
+
+describe("format-checkout cleanup never deletes through the linked dependency tree", () => {
+  // checkPushedCommit checks out the pushed commit into a scratch directory and
+  // links a node_modules tree in so a dynamic prettier config can resolve its
+  // plugins. When the pushing worktree has no dependencies of its own,
+  // findPrettierBin borrows ANOTHER worktree's real node_modules, and on Windows
+  // that borrow is a junction. The scratch directory is then torn down with
+  // `git worktree remove --force` plus a recursive rmSync, neither of which
+  // respects `git worktree lock`. These tests pin the invariant that the link is
+  // unlinked-not-followed first, and that the force-deletes are skipped entirely
+  // when it could not be.
+  //
+  // Platform note: the link below is a junction on win32 and a directory symlink
+  // everywhere else, so CI (Linux) proves the symlink case and a Windows run
+  // proves the junction case. Both are exercised by the same assertions.
+  function linkFixture() {
+    const sentinel = mkdtempSync(join(tmpdir(), "guard-push-sentinel-"));
+    created.push(sentinel);
+    mkdirSync(join(sentinel, "prettier", "bin"), { recursive: true });
+    writeFileSync(join(sentinel, "prettier", "package.json"), '{"version":"3.9.6"}');
+    const container = mkdtempSync(join(tmpdir(), "guard-push-container-"));
+    created.push(container);
+    const link = join(container, "node_modules");
+    symlinkSync(sentinel, link, process.platform === "win32" ? "junction" : "dir");
+    return { sentinel, container, link, canary: join(sentinel, "prettier", "package.json") };
+  }
+
+  it("removes the link itself and leaves the borrowed tree intact", () => {
+    const { container, link, canary } = linkFixture();
+    expect(existsSync(canary)).toBe(true);
+
+    unlinkDependencyLink(link);
+    expect(existsSync(link)).toBe(false);
+    expect(existsSync(canary)).toBe(true);
+
+    // The force-delete that follows in checkPushedCommit can no longer reach it.
+    rmSync(container, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
+    expect(existsSync(canary)).toBe(true);
+  });
+
+  it("removes a DANGLING link, which existsSync reports as absent", () => {
+    // Regression guard. existsSync follows the link, so once the borrowed tree
+    // is gone the link reads as absent and an existsSync-gated cleanup leaves it
+    // in place — for `git worktree remove --force` and a recursive rmSync to
+    // interpret instead. lstat sees the link whether or not it resolves.
+    const { sentinel, link } = linkFixture();
+    rmSync(sentinel, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
+    expect(existsSync(link)).toBe(false); // the trap: it is still there
+
+    unlinkDependencyLink(link);
+    expect(() => lstatSync(link)).toThrow(/ENOENT/);
+  });
+
+  it("is tolerant of the link already being gone", () => {
+    const container = mkdtempSync(join(tmpdir(), "guard-push-container-"));
+    created.push(container);
+    const result = unlinkDependencyLink(join(container, "node_modules"));
+    expect(result.removed).toBe(false);
+    expect(result.reason).toBe("absent");
+  });
+
+  it("refuses a real directory at the link path instead of deleting its contents", () => {
+    // Nothing in guard-push creates a real directory here, so one means something
+    // unexpected — and recursively deleting an unexpected directory is the exact
+    // hazard these tests exist to prevent.
+    const container = mkdtempSync(join(tmpdir(), "guard-push-container-"));
+    created.push(container);
+    const real = join(container, "node_modules");
+    mkdirSync(join(real, "prettier"), { recursive: true });
+    writeFileSync(join(real, "prettier", "package.json"), '{"version":"3.9.6"}');
+
+    const result = unlinkDependencyLink(real);
+    expect(result.removed).toBe(false);
+    expect(result.reason).toBe("not-a-link");
+    expect(existsSync(join(real, "prettier", "package.json"))).toBe(true);
+  });
+
+  it("SKIPS both force-deletes when the link could not be removed", () => {
+    // Regression guard. Swallowing the unlink failure and continuing is the one
+    // case where a force-delete runs over a directory that still holds a live
+    // link into another worktree's node_modules. A leftover scratch directory is
+    // cheap; that is not.
+    const calls: string[] = [];
+    cleanupFormatCheckout("D:/nonexistent-scratch", {
+      unlink: () => ({ removed: false, reason: "failed" }) as const,
+      removeWorktree: () => {
+        calls.push("removeWorktree");
+      },
+      removeDir: () => {
+        calls.push("removeDir");
+      },
+      log: () => {},
+    });
+    expect(calls).toEqual([]);
+  });
+
+  it("still tears down the checkout when the link was removed or was never there", () => {
+    for (const reason of ["unlink", "absent"] as const) {
+      const calls: string[] = [];
+      cleanupFormatCheckout("D:/nonexistent-scratch", {
+        unlink: () => ({ removed: reason === "unlink", reason }),
+        removeWorktree: () => {
+          calls.push("removeWorktree");
+        },
+        removeDir: () => {
+          calls.push("removeDir");
+        },
+        log: () => {},
+      });
+      expect(calls).toEqual(["removeWorktree", "removeDir"]);
+    }
+  });
+
+  it("unlinks BEFORE either force-delete, so traversal behaviour cannot matter", () => {
+    const order: string[] = [];
+    cleanupFormatCheckout("D:/nonexistent-scratch", {
+      unlink: () => {
+        order.push("unlink");
+        return { removed: true, reason: "unlink" } as const;
+      },
+      removeWorktree: () => {
+        order.push("removeWorktree");
+      },
+      removeDir: () => {
+        order.push("removeDir");
+      },
+      log: () => {},
+    });
+    expect(order).toEqual(["unlink", "removeWorktree", "removeDir"]);
+  });
+});