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

src/infrastructure/persistence/lockfile_repository.ts

@@ -18,6 +18,7 @@
 // along with Swamp.  If not, see <https://www.gnu.org/licenses/>.
 
 import { dirname } from "@std/path";
+import { UserError } from "../../domain/errors.ts";
 import { atomicWriteTextFile } from "./atomic_write.ts";
 import {
   readUpstreamExtensions,
@@ -99,11 +100,14 @@ export class LockfileRepository {
   }
 
   /**
-   * Returns the cached entry map. Callers receive a defensive shallow
-   * copy so external mutation cannot corrupt the cache.
+   * Returns the cached entry map. Callers receive a defensive deep copy
+   * so external mutation — including pushing into nested `files[]` /
+   * `include[]` arrays — cannot corrupt the cache. `structuredClone`
+   * (in scope on Deno via the global) covers every value shape the
+   * lockfile carries today (strings, arrays of strings, booleans).
    */
   getAllEntries(): UpstreamExtensionsMap {
-    return { ...this.cache };
+    return structuredClone(this.cache);
   }
 
   /**
@@ -160,6 +164,13 @@ export class LockfileRepository {
    * the result, releases the lock, and updates this instance's cache.
    */
   async removeEntry(name: string): Promise<void> {
+    // Symmetric with writeEntry — defensive against callers who removed
+    // an extension in this process and the parent dir was cleaned up
+    // between then and now. In practice unreachable today (rm.ts only
+    // calls this after extensionRmPreview confirmed the entry exists,
+    // which read the lockfile successfully) but the asymmetry would
+    // surface as an unhelpful NotFound from acquireLock if it ever did.
+    await Deno.mkdir(dirname(this.lockfilePath), { recursive: true });
     const lockFile = await this.acquireLock();
     try {
       const current = await readUpstreamExtensions(this.lockfilePath);
@@ -189,14 +200,18 @@ export class LockfileRepository {
             await new Promise((r) => setTimeout(r, LOCK_RETRY_DELAY_MS));
             continue;
           }
-          throw new Error(
+          // UserError so the top-level handler renders the clean
+          // message instead of a stack trace — matches the pre-W2-prequel
+          // behavior in pull.ts (rm.ts threw plain Error; this
+          // consolidates on the better UX).
+          throw new UserError(
             "Could not acquire lock on upstream_extensions.json. Another operation may be in progress. Please retry.",
           );
         }
         throw error;
       }
     }
-    throw new Error(
+    throw new UserError(
       "Could not acquire lock on upstream_extensions.json.",
     );
   }

src/infrastructure/persistence/lockfile_repository_test.ts

@@ -17,10 +17,11 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with Swamp.  If not, see <https://www.gnu.org/licenses/>.
 
-import { assertEquals, assertNotEquals } from "@std/assert";
+import { assertEquals, assertNotEquals, assertRejects } from "@std/assert";
 import { join } from "@std/path";
 import { LockfileRepository } from "./lockfile_repository.ts";
 import { atomicWriteTextFile } from "./atomic_write.ts";
+import { UserError } from "../../domain/errors.ts";
 
 async function withTempDir(
   fn: (dir: string) => Promise<void>,
@@ -172,6 +173,23 @@ Deno.test("LockfileRepository.removeEntry: deletes key and persists", async () =
   });
 });
 
+Deno.test("LockfileRepository.removeEntry: creates parent dir (symmetric with writeEntry)", async () => {
+  await withTempDir(async (dir) => {
+    // Construct against a path whose parent dir does NOT exist yet.
+    // removeEntry must create it before acquireLock — otherwise
+    // Deno.open hits NotFound and propagates an unhelpful error.
+    const path = join(dir, "nested", "subdir", "upstream_extensions.json");
+    const repo = new LockfileRepository(path, {});
+
+    // Should not throw.
+    await repo.removeEntry("@scope/never-existed");
+
+    // The empty lockfile is now persisted and the parent dir exists.
+    const onDisk = JSON.parse(await Deno.readTextFile(path));
+    assertEquals(Object.keys(onDisk).length, 0);
+  });
+});
+
 Deno.test("LockfileRepository.removeEntry: missing key is a no-op", async () => {
   await withTempDir(async (dir) => {
     const path = join(dir, "upstream_extensions.json");
@@ -185,17 +203,28 @@ Deno.test("LockfileRepository.removeEntry: missing key is a no-op", async () =>
   });
 });
 
-Deno.test("LockfileRepository: getAllEntries returns a defensive copy", async () => {
+Deno.test("LockfileRepository: getAllEntries returns a defensive deep copy", async () => {
   await withTempDir(async (dir) => {
     const path = join(dir, "upstream_extensions.json");
     const repo = await LockfileRepository.create(path);
-    await repo.writeEntry("@scope/foo", "1.0.0", []);
+    await repo.writeEntry("@scope/foo", "1.0.0", ["models/foo.ts"], {
+      include: ["dep.ts"],
+    });
 
     const map = repo.getAllEntries();
-    delete map["@scope/foo"];
 
-    // Repo's internal cache must not be affected by external mutation.
+    // Top-level key deletion must not affect the cache.
+    delete map["@scope/foo"];
     assertNotEquals(repo.getEntry("@scope/foo"), null);
+
+    // Nested array mutation must not affect the cache either —
+    // deep copy guards against `entries["@x/y"].files!.push(...)`
+    // patterns that a shallow copy would propagate.
+    const fresh = repo.getAllEntries();
+    fresh["@scope/foo"].files!.push("INJECTED");
+    fresh["@scope/foo"].include!.push("INJECTED");
+    assertEquals(repo.getEntry("@scope/foo")?.files, ["models/foo.ts"]);
+    assertEquals(repo.getEntry("@scope/foo")?.include, ["dep.ts"]);
   });
 });
 
@@ -224,6 +253,30 @@ Deno.test("LockfileRepository: concurrent writers all complete via acquireLock r
   });
 });
 
+Deno.test("LockfileRepository: lock-acquisition exhaustion throws UserError (clean CLI message, not stack trace)", async () => {
+  await withTempDir(async (dir) => {
+    const path = join(dir, "upstream_extensions.json");
+    const lockPath = `${path}.lock`;
+
+    // Pre-create the lock file so every retry sees AlreadyExists.
+    // 10 retries × 100ms = ~1s before the writer gives up.
+    await Deno.writeTextFile(lockPath, "");
+
+    const repo = await LockfileRepository.create(path);
+    const error = await assertRejects(
+      () => repo.writeEntry("@scope/foo", "1.0.0", []),
+      UserError,
+      "Could not acquire lock on upstream_extensions.json",
+    );
+    // Top-level error_output renderer keys off `instanceof UserError` to
+    // emit a clean message rather than a stack trace; this assertion
+    // pins the contract.
+    assertEquals(error instanceof UserError, true);
+
+    await Deno.remove(lockPath);
+  });
+});
+
 Deno.test("LockfileRepository: cleans up .lock file on success path", async () => {
   await withTempDir(async (dir) => {
     const path = join(dir, "upstream_extensions.json");