From e43ee3dfa076a29dcee6cf59000f558282b89cfe Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:19:31 +0800 Subject: [PATCH 1/3] harden(guard-push): unlink the borrowed dependency tree before force-deleting its container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The format guard checks the pushed commit in a scratch worktree 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 tree, 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`. PR #2186 added a cleanup that removed the link first, but it had two gaps: - it gated on existsSync, which FOLLOWS the link, so a link whose target had gone away read as absent and was left in place for the force-deletes; - it swallowed an unlink failure and continued, so the one case where the link is still live is the case where both force-deletes still ran over it. Extract the teardown into unlinkDependencyLink() and cleanupFormatCheckout(). The first uses lstat, operates on the link itself (unlink, then rmdir for a Windows directory reparse point), refuses a real directory outright, and never uses a recursive delete on that path. The second runs it before either force-delete and skips both when it did not succeed — a leftover scratch directory is cheap, and `git worktree prune` clears its registration next push. The format guard is not weakened; SKIP_FORMAT_GUARD=1 remains the only escape hatch. The borrowing fallback is deliberately left ungated on donor idleness, with the reasoning recorded at findPrettierBin. This fixes a demonstrable hazard. It makes no claim about the cause of any past worktree loss. Co-Authored-By: Claude Opus 5 --- scripts/guard-push.mjs | 123 +++++++++++++++++++++++++++++++++--- tests/guard-push.test.ts | 132 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 245 insertions(+), 10 deletions(-) diff --git a/scripts/guard-push.mjs b/scripts/guard-push.mjs index 0beaa0a7c..24400c5c1 100755 --- a/scripts/guard-push.mjs +++ b/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"; @@ -486,6 +486,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"); @@ -626,15 +633,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) { diff --git a/tests/guard-push.test.ts b/tests/guard-push.test.ts index 79bbeada8..2baedfa79 100644 --- a/tests/guard-push.test.ts +++ b/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"; @@ -33,6 +42,8 @@ import { pushedBranchNames, pushedTipMatchesHead, staticGuard, + cleanupFormatCheckout, + unlinkDependencyLink, } from "../scripts/guard-push.mjs"; const ZERO = "0".repeat(40); @@ -529,3 +540,122 @@ 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 }); + 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 }); + 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" }), + 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"]) { + 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" }; + }, + removeWorktree: () => order.push("removeWorktree"), + removeDir: () => order.push("removeDir"), + log: () => {}, + }); + expect(order).toEqual(["unlink", "removeWorktree", "removeDir"]); + }); +}); From 5cb7fcf30b956273b767662cbea16e69c9f02eb7 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:53:31 +0800 Subject: [PATCH 2/3] fix(tests): satisfy the source typecheck for the injected cleanup seams The injected removeWorktree/removeDir returned Array.prototype.push's number where tryGit's signature is string | undefined, and the reason loop widened the literal union. Braces and `as const` instead. Co-Authored-By: Claude Opus 5 --- tests/guard-push.test.ts | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/tests/guard-push.test.ts b/tests/guard-push.test.ts index 2baedfa79..24830217d 100644 --- a/tests/guard-push.test.ts +++ b/tests/guard-push.test.ts @@ -624,21 +624,29 @@ describe("format-checkout cleanup never deletes through the linked dependency tr // cheap; that is not. const calls: string[] = []; cleanupFormatCheckout("D:/nonexistent-scratch", { - unlink: () => ({ removed: false, reason: "failed" }), - removeWorktree: () => calls.push("removeWorktree"), - removeDir: () => calls.push("removeDir"), + 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"]) { + 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"), + removeWorktree: () => { + calls.push("removeWorktree"); + }, + removeDir: () => { + calls.push("removeDir"); + }, log: () => {}, }); expect(calls).toEqual(["removeWorktree", "removeDir"]); @@ -650,10 +658,14 @@ describe("format-checkout cleanup never deletes through the linked dependency tr cleanupFormatCheckout("D:/nonexistent-scratch", { unlink: () => { order.push("unlink"); - return { removed: true, reason: "unlink" }; + return { removed: true, reason: "unlink" } as const; + }, + removeWorktree: () => { + order.push("removeWorktree"); + }, + removeDir: () => { + order.push("removeDir"); }, - removeWorktree: () => order.push("removeWorktree"), - removeDir: () => order.push("removeDir"), log: () => {}, }); expect(order).toEqual(["unlink", "removeWorktree", "removeDir"]); From e63d4e3eb6fa88cf41902067f8968551298226c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 16:10:31 +0000 Subject: [PATCH 3/3] fix(tests): add bounded retries to guard-push scratch-cleanup rmSync calls tests/test-runner-safety.test.ts's repo-wide static contract requires every recursive rmSync in a test file to carry a positive maxRetries so Windows transient handle errors (EBUSY/EPERM/ENOTEMPTY) never masquerade as a real assertion failure. The two new rmSync calls added by this PR's guard-push.test.ts fixtures ("removes the link itself and leaves the borrowed tree intact" and "removes a DANGLING link, which existsSync reports as absent") omitted maxRetries, tripping "requires bounded retries for every recursive test-fixture cleanup" in CI's Unit coverage job. Add maxRetries: 5, retryDelay: 100 to both, matching this file's existing afterEach cleanup convention. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015StJgDC2dfef8PXN9dfriw --- tests/guard-push.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/guard-push.test.ts b/tests/guard-push.test.ts index 959c9dd9d..9fccd4dd8 100644 --- a/tests/guard-push.test.ts +++ b/tests/guard-push.test.ts @@ -609,7 +609,7 @@ describe("format-checkout cleanup never deletes through the linked dependency tr expect(existsSync(canary)).toBe(true); // The force-delete that follows in checkPushedCommit can no longer reach it. - rmSync(container, { recursive: true, force: true }); + rmSync(container, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); expect(existsSync(canary)).toBe(true); }); @@ -619,7 +619,7 @@ describe("format-checkout cleanup never deletes through the linked dependency tr // 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 }); + rmSync(sentinel, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); expect(existsSync(link)).toBe(false); // the trap: it is still there unlinkDependencyLink(link);