fix(persistence): tighten LockfileRepository follow-ups from #1298 review

[stack72]

Summary

Three follow-up fixes to LockfileRepository raised in review of #1298 (the W2 prequel). All small, all tested.

Changes

1. Lock-contention throws UserError (clean CLI message), not plain Error

Before #1298, acquireLock in pull.ts threw UserError. The top-level error handler at error_output.ts:64-68 keys off instanceof UserError to emit a clean Error: <message> line; plain Error falls through to a stack trace.

When #1298 consolidated the duplicate acquireLock helpers from pull.ts and rm.ts into the new repository, it picked up rm.ts's plain-Error shape. Result: a real lock-exhaustion failure during swamp extension pull (two concurrent users, the loser retries 10× and gives up) now shows a stack trace instead of the clean message it used to.

This restores the pull.ts UX (the better of the two pre-prequel implementations). New test pins the instanceof UserError contract so a future refactor can't quietly break it again.

2. removeEntry now mkdir's parent dir, symmetric with writeEntry

Currently unreachable in practice — rm.ts only calls removeEntry after extensionRmPreview confirmed the entry exists, which implies the lockfile was readable. But the asymmetry would surface as an unhelpful NotFound from Deno.open if a future caller path ever hit a missing-parent-dir state. Defensive consistency fix; test constructs against a non-existent parent dir and verifies removeEntry creates it.

3. getAllEntries returns a deep copy via structuredClone

The previous shallow copy { ...this.cache } only protected against top-level key deletion. A caller doing entries[\"@x/y\"].files!.push(\"injected\") could mutate the internal cache through the shared UpstreamExtensionEntry reference.

All current callers in this repo are read-only so this was theoretical, but the original regression test only verified top-level deletion — nested array mutation would have slipped through. Test now also verifies files[] and include[] array mutation cannot reach the cache.

Test plan

• deno check clean
• deno lint clean
• deno fmt clean
• deno run test — 5399 passed, 0 failed (3 new tests: UserError contract, removeEntry mkdir symmetry, deep-copy nested mutation)

Out of scope

This is a tightening pass, not a behavioral change. Lockfile JSON format unchanged; no migration; trivial revert.

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-scoped tightening pass. All three fixes are correct, well-tested, and the DDD layering is sound (infrastructure → domain import for UserError is the right direction).

Blocking Issues

None.

Suggestions

1. Inline comments reference callers and task history that will rot — The removeEntry mkdir comment (lines 167–172) names rm.ts as a caller and the UserError comment (lines 203–206) references "pre-W2-prequel behavior in pull.ts." Per CLAUDE.md, caller references and task-specific history belong in the PR description, not inline. A one-liner like // Defensive: ensure parent dir exists before lock acquisition would preserve the WHY without the rot risk.

2. getEntry returns a direct cache reference — The PR hardens getAllEntries with structuredClone but getEntry still returns this.cache[name] directly. All current callers are read-only (confirmed by grep), so this isn't a bug today, but the asymmetry means a future caller doing repo.getEntry("@x/y")!.files!.push(...) would corrupt the cache. Worth considering for consistency — or at minimum a JSDoc note on getEntry that the returned object is shared.

3. Redundant instanceof assertion in lock-exhaustion test — Line 274 (assertEquals(error instanceof UserError, true)) is already guaranteed by the assertRejects(..., UserError, ...) call on line 266–270 which both checks the type and returns the typed error. The extra assertion doesn't add coverage.

4. getAllEntries JSDoc mentions implementation details — The note about structuredClone being "in scope on Deno via the global" is an implementation detail that doesn't help callers. The contract ("returns a deep copy; safe to mutate without affecting the cache") is the valuable part.

[github-actions]

Adversarial Review

Critical / High

None.

Medium

1. getEntry returns a direct mutable reference to the cache (lockfile_repository.ts:98-99).

   The PR correctly upgrades getAllEntries from a shallow copy to structuredClone, preventing callers from corrupting the cache via nested array mutation. However, getEntry still returns the raw cached object:

   getEntry(name: string): UpstreamExtensionEntry | null {
     return this.cache[name] ?? null;
   }

   A caller doing repo.getEntry("@scope/foo")!.files!.push("injected") would mutate the internal cache through the shared reference — the exact class of bug getAllEntries was just hardened against. All current callers are read-only so this is not exploitable today, but the asymmetry is worth noting: the PR closes one door and leaves the adjacent one open.

   Not a regression — this was pre-existing. But if the goal is defensive consistency, getEntry should also return a clone (or the JSDoc should document that it returns a live reference).

Low

1. Dead code after the for loop in acquireLock (lockfile_repository.ts:214-216). The throw new UserError(...) after the for loop is unreachable: every iteration either returns (success), throws UserError (last-retry AlreadyExists), or re-throws the original error (non-AlreadyExists). The loop can never exit normally. Pre-existing, not introduced by this PR — just changed from Error to UserError for consistency. Harmless but worth a future cleanup.

Verdict

PASS. Three well-scoped, well-tested defensive fixes. The structuredClone upgrade, UserError consolidation, and removeEntry mkdir symmetry are all correct. The getEntry gap is pre-existing and theoretical. No blocking issues.