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

[BigSimmo]

Summary

• scripts/guard-push.mjs — the format guard's scratch-checkout teardown no longer has any path
  that can force-delete a directory still holding a live link into another worktree's node_modules.
  The link is removed by lstat + unlink/rmdir (never followed, never recursively deleted), and
  when that does not succeed, git worktree remove --force and the recursive rmSync are both
  skipped rather than run anyway.
• tests/guard-push.test.ts — seven tests pinning that behaviour, four of which fail against the
  code as it stands on main.

No clinical, RAG, retrieval, ingestion, auth, or privacy surface is touched, so no Clinical
Governance Preflight applies. No provider-backed gate was run.

The hazard

checkPushedCommit checks out the pushed commit into a scratch worktree under %TEMP% 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 deliberately borrows another worktree's
real node_modules — gated on a byte-identical package-lock.json and a matching installed
Prettier version. On Windows that borrow is linked in as a junction. The scratch directory is
then torn down with git worktree remove --force followed by rmSync(recursive: true, force: true),
neither of which respects git worktree lock.

PR #2186 added a cleanup that removed the link before the worktree removal. Two gaps remained:

1. existsSync follows the link. Once the borrowed tree is gone, the link reads as absent, so
   the cleanup skips it — and the two force-deletes then run over it anyway. This is not theoretical:
   a live dangling junction of exactly this kind was found on the development machine while preparing
   this change (details below).
2. A failed unlink was swallowed and the teardown continued (// Continue cleanup even if unlinking junction throws). The one case where the link is still live is therefore the one case
   where both force-deletes still ran over it.

What this changes

unlinkDependencyLink(linkPath) — uses lstat, so a dangling link is still seen; unlink, falling
back to rmdir for a Windows directory reparse point, both of which remove the link and never its
target; refuses a real directory outright; and never uses a recursive delete on that path under any
branch.

cleanupFormatCheckout(dir, deps) — runs the unlink before either force-delete, and returns
without running either when the link is still there, logging the path and reason. A leftover scratch
directory is cheap, and the git worktree prune at the top of checkPushedCommit clears its
registration on the next push. Dependencies are injectable so ordering and the skip are unit-testable
without creating and destroying real worktrees.

The format guard is not weakened. It still fails closed — being unable to check is not evidence a
push is clean — and SKIP_FORMAT_GUARD=1 remains the only escape hatch.

Considered and deliberately not done: gating the borrow on whether the donor worktree is idle.
"Actively in use" is not cheaply detectable — an in-flight npm ci in another process leaves nothing
guard-push 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 teardown. Reasoning is recorded at
findPrettierBin rather than half-built.

What the tests do and do not prove

Not all seven are regression guards, and it matters which are which:

Test	Fails on main?
removes a DANGLING link, which existsSync reports as absent	yes
is tolerant of the link already being gone	yes
refuses a real directory at the link path instead of deleting its contents	yes
SKIPS both force-deletes when the link could not be removed	yes
removes the link itself and leaves the borrowed tree intact	no — see below
still tears down the checkout when the link was removed or was never there	no
unlinks BEFORE either force-delete	no

The sentinel test (link a directory in, delete the container, assert the target survived) is a
guarantee test, not a regression test: Node's recursive rmSync lstats children and unlinks
links rather than descending them, so it passes with or without this change. It is kept because it
pins the invariant in the place a future reader will look for it, but the four marked yes above
are what actually prove the fix.

Platform coverage — stated precisely

The link is a junction on win32 and a directory symlink everywhere else; the same assertions
cover both.

• The junction path was executed, not merely reasoned about: the red and green runs below were
  run on Windows 11 with Node 24.19.0, creating real junctions via symlinkSync(..., "junction").
• CI is Linux and therefore executes the POSIX directory-symlink path. CI does not enforce the
  junction case — that evidence is local only.

Evidence

Before the fix (the main teardown logic, extracted verbatim behind the new seams so the failures
are real assertions rather than an import error):

 FAIL  tests/guard-push.test.ts > ... > removes a DANGLING link, which existsSync reports as absent
AssertionError: expected [Function] to throw an error
 ❯ tests/guard-push.test.ts:593:35
     expect(() => lstatSync(link)).toThrow(/ENOENT/);

 Test Files  1 failed (1)
      Tests  4 failed | 45 passed (49)

After:

 Test Files  1 passed (1)
      Tests  49 passed (49)

npm run typecheck — exit 0, [gate-receipts] recorded a pass for "typecheck:internal" (3916 input files).
npm run lint — exit 0, [gate-receipts] recorded a pass for "lint:internal" (3916 input files).
prettier --check . (whole tree, not per-file) — exit 0, All matched files use Prettier code style!

Live corroboration found while preparing this

On the development machine, %TEMP% held several abandoned guard-push-format-* scratch
directories, three of which still carried a live Directory, ReparsePoint junction into a different
worktree's real node_modules:

K5MYiH → D:\Repos\Database\.claude\worktrees\phase-4-index-restoration-b0f4ea\node_modules
WEIhK8 → D:\Worktrees\Database\claude-cloud-parity\node_modules
Y6kYzj → D:\Repos\Database\.claude\worktrees\dev-hub-stocktake\node_modules

The first target no longer exists — a dangling junction, i.e. gap (1) above occurring in the wild.
All three were cleaned up using the new unlinkDependencyLink, and both surviving targets were
verified byte-count-stable at 528 entries before and after.

What this does not claim

This fixes a demonstrable hazard: the code links a foreign worktree's real node_modules into a
directory it then force-deletes. It makes no claim that this caused any past worktree loss. That
was not established and is not asserted here, in the commits, or in the code comments.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 14 minutes

Limit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6124701e-3cca-4dfc-99ae-9fb5f3b72415

📥 Commits

Reviewing files that changed from the base of the PR and between 8a5f71b and 6972bcc.

📒 Files selected for processing (2)

• scripts/guard-push.mjs
• tests/guard-push.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[supabase]

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[github-actions]

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

• Unit coverage — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
• PR required — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #12949 (success).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.