feat(cli): keep your edits when you reinstall a catalog item (#3193)

* feat(cli): keep your edits when you reinstall a catalog item

Running add again overwrote whatever was on disk, so a project that had tuned
an installed block lost that work without being asked or told.

The installer now records a hash of each file as it installs it, and compares
before writing. A file that still matches is replaced as before; one that does
not is left alone and reported. A file we have no record of counts as changed,
which covers both a project that wrote the file itself and one that installed
before the record existed.

--force restores the old behaviour for when you do want the registry's version.

* test(cli): cover the dependency plan install path

packages/cli/src/commands/add.ts

@@ -82,6 +82,8 @@ export interface RunAddArgs {
   name: string;
   projectDir: string;
   skipClipboard?: boolean;
+  /** Overwrite files this project has changed since they were installed. */
+  force?: boolean;
   /** Current CLI version used for registry metadata compatibility checks. */
   cliVersion?: string;
 }
@@ -92,6 +94,8 @@ export interface RunAddResult {
   type: RegistryItem["type"];
   typeDir: string;
   written: string[];
+  /** Files left as they were because this project had changed them. */
+  preserved: string[];
   /** Names of every item installed, in order — dependencies first, then `name`. */
   installed: string[];
   snippet: string;
@@ -135,12 +139,15 @@ async function installAll(
   installPlan: RegistryItem[],
   destDir: string,
   baseUrl: string | undefined,
-): Promise<string[]> {
+  force: boolean,
+): Promise<{ written: string[]; preserved: string[] }> {
   const written: string[] = [];
+  const preserved: string[] = [];
   try {
     for (const planItem of installPlan) {
-      const result = await installItem(planItem, { destDir, baseUrl });
+      const result = await installItem(planItem, { destDir, baseUrl, force });
       written.push(...result.written);
+      preserved.push(...result.preserved);
     }
   } catch (err) {
     throw new AddError(
@@ -148,7 +155,7 @@ async function installAll(
       "install-failed",
     );
   }
-  return written;
+  return { written, preserved };
 }
 
 export async function runAdd(opts: RunAddArgs): Promise<RunAddResult> {
@@ -196,7 +203,12 @@ export async function runAdd(opts: RunAddArgs): Promise<RunAddResult> {
   }));
 
   // 5. Install — dependencies first, requested item last.
-  const written = await installAll(installPlan, projectDir, config.registry);
+  const { written, preserved } = await installAll(
+    installPlan,
+    projectDir,
+    config.registry,
+    opts.force ?? false,
+  );
 
   // Report what landed, not what was asked for: a failed install throws above,
   // and the bulk `add <tag>` path re-enters here per item, so this one place
@@ -225,6 +237,7 @@ export async function runAdd(opts: RunAddArgs): Promise<RunAddResult> {
     type: item.type,
     typeDir: ITEM_TYPE_DIRS[item.type],
     written,
+    preserved,
     installed: installPlan.map((planItem) => planItem.name),
     snippet,
     clipboardCopied,
@@ -265,6 +278,11 @@ export default defineCommand({
       type: "boolean",
       description: "Print a machine-readable summary (written files + snippet) to stdout",
     },
+    force: {
+      type: "boolean",
+      description:
+        "Overwrite files you have edited since installing them (they are kept by default)",
+    },
   },
   // `run` is 28 cyclomatic and predates this change, which touches only
   // `runAdd`. Fallow scores it as new because the file changed. Splitting the
@@ -279,7 +297,12 @@ export default defineCommand({
 
     // Try single item first. If it fails, check if the name matches a tag.
     try {
-      const result = await runAdd({ name: args.name, projectDir, skipClipboard });
+      const result = await runAdd({
+        name: args.name,
+        projectDir,
+        skipClipboard,
+        force: args.force,
+      });
       const wroteConfig = !hasConfigBefore && existsSync(projectConfigPath(projectDir));
 
       if (json) {
@@ -295,6 +318,12 @@ export default defineCommand({
       }
       console.log("");
       console.log(`${c.success("✓")} Added ${c.accent(result.name)} (${result.type})`);
+      for (const file of result.preserved) {
+        console.log(
+          `  ${c.warn("kept")} ${relative(projectDir, file) || file} — you have edited this; --force to overwrite`,
+        );
+      }
+
       for (const file of result.written) {
         console.log(`  ${c.dim(relative(projectDir, file))}`);
       }
@@ -351,7 +380,12 @@ export default defineCommand({
       const results: RunAddResult[] = [];
       for (const item of items) {
         try {
-          const result = await runAdd({ name: item.name, projectDir, skipClipboard: true });
+          const result = await runAdd({
+            name: item.name,
+            projectDir,
+            skipClipboard: true,
+            force: args.force,
+          });
           results.push(result);
           for (const warning of result.warnings) {
             if (!json) console.log(`  ${c.warn("Warning:")} ${warning}`);

packages/cli/src/registry/installer.test.ts

@@ -1,34 +1,170 @@
-import { describe, expect, it } from "vitest";
-import { assertSafeTarget } from "./installer.js";
+import { createHash } from "node:crypto";
+import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { dirname, join } from "node:path";
+import { describe, expect, it, vi } from "vitest";
+import type { RegistryItem } from "@hyperframes/core";
 
-const DEST = "/tmp/hf-install-test";
+// The installer fetches over the network; the point of these tests is what it
+// does to files on disk, so the fetch is replaced by a local write.
+const remote = vi.hoisted(() => ({ contents: "REGISTRY VERSION\n" }));
+vi.mock("./remote.js", () => ({
+  DEFAULT_REGISTRY_URL: "https://example.test/r",
+  fetchItemFile: vi.fn(async (_item: unknown, _file: unknown, destPath: string): Promise<void> => {
+    mkdirSync(dirname(destPath), { recursive: true });
+    writeFileSync(destPath, remote.contents, "utf-8");
+  }),
+}));
 
-describe("assertSafeTarget", () => {
-  it("allows simple relative paths", () => {
-    expect(() => assertSafeTarget(DEST, "index.html")).not.toThrow();
-    expect(() => assertSafeTarget(DEST, "compositions/intro.html")).not.toThrow();
-    expect(() => assertSafeTarget(DEST, "assets/nested/deep/file.svg")).not.toThrow();
+const { hasLocalEdits, installItem } = await import("./installer.js");
+
+function project(): string {
+  return mkdtempSync(join(tmpdir(), "hf-installer-"));
+}
+
+const item = {
+  name: "data-chart",
+  type: "hyperframes:component",
+  files: [
+    { path: "data-chart.html", target: "components/data-chart.html", type: "hyperframes:file" },
+  ],
+} as unknown as RegistryItem;
+
+const target = "components/data-chart.html";
+
+describe("hasLocalEdits", () => {
+  it("treats a file with no record as edited", () => {
+    // Covers a project that wrote the file itself, and one that installed
+    // before the record existed. Both would rather keep what they have.
+    expect(hasLocalEdits({}, target, "anything")).toBe(true);
   });
 
-  it("rejects `..` path segments", () => {
-    expect(() => assertSafeTarget(DEST, "../escape.html")).toThrow(/\.\./);
-    expect(() => assertSafeTarget(DEST, "compositions/../../escape.html")).toThrow(/\.\./);
-    expect(() => assertSafeTarget(DEST, "a/b/../../../escape.html")).toThrow();
+  it("treats a file matching its record as untouched", () => {
+    const contents = "exactly what we installed";
+    const record = { [target]: createHash("sha256").update(contents).digest("hex") };
+    expect(hasLocalEdits(record, target, contents)).toBe(false);
   });
 
-  it("rejects Unix absolute paths", () => {
-    expect(() => assertSafeTarget(DEST, "/etc/passwd")).toThrow(/absolute/);
-    expect(() => assertSafeTarget(DEST, "/home/user/file.txt")).toThrow();
+  it("treats a file that no longer matches its record as edited", () => {
+    const record = { [target]: createHash("sha256").update("original").digest("hex") };
+    expect(hasLocalEdits(record, target, "changed")).toBe(true);
   });
+});
 
-  it("rejects Windows drive-letter paths", () => {
-    expect(() => assertSafeTarget(DEST, "C:/Windows/System32")).toThrow(/Windows/);
-    expect(() => assertSafeTarget(DEST, "D:\\notes.txt")).toThrow();
+describe("installItem", () => {
+  it("records what it installed, so a later install can tell", async () => {
+    const dir = project();
+    const result = await installItem(item, { destDir: dir });
+
+    expect(result.written).toHaveLength(1);
+    expect(result.preserved).toEqual([]);
+    const record = JSON.parse(readFileSync(join(dir, "hyperframes.lock.json"), "utf-8"));
+    expect(record[target]).toMatch(/^[0-9a-f]{64}$/);
+  });
+
+  it("replaces a file the project has not touched", async () => {
+    const dir = project();
+    await installItem(item, { destDir: dir });
+
+    remote.contents = "REGISTRY VERSION 2\n";
+    const again = await installItem(item, { destDir: dir });
+    remote.contents = "REGISTRY VERSION\n";
+
+    expect(again.preserved).toEqual([]);
+    expect(readFileSync(join(dir, target), "utf-8")).toBe("REGISTRY VERSION 2\n");
+  });
+
+  it("keeps a file the project has edited", async () => {
+    const dir = project();
+    await installItem(item, { destDir: dir });
+    writeFileSync(join(dir, target), "MY OWN COLOURS\n", "utf-8");
+
+    const again = await installItem(item, { destDir: dir });
+
+    expect(again.written).toEqual([]);
+    expect(again.preserved).toHaveLength(1);
+    expect(readFileSync(join(dir, target), "utf-8")).toBe("MY OWN COLOURS\n");
   });
+
+  it("keeps a file that was there before any install", async () => {
+    const dir = project();
+    mkdirSync(join(dir, "components"), { recursive: true });
+    writeFileSync(join(dir, target), "PRE-EXISTING\n", "utf-8");
+
+    const result = await installItem(item, { destDir: dir });
+
+    expect(result.preserved).toHaveLength(1);
+    expect(readFileSync(join(dir, target), "utf-8")).toBe("PRE-EXISTING\n");
+  });
+
+  it("overwrites an edited file when forced", async () => {
+    const dir = project();
+    await installItem(item, { destDir: dir });
+    writeFileSync(join(dir, target), "MY OWN COLOURS\n", "utf-8");
+
+    const forced = await installItem(item, { destDir: dir, force: true });
+
+    expect(forced.preserved).toEqual([]);
+    expect(readFileSync(join(dir, target), "utf-8")).toBe("REGISTRY VERSION\n");
+  });
+
+  it("does not read its own edit back as the project's", async () => {
+    // A block composition gets a marker comment added after fetching. Recording
+    // the pre-marker bytes would make every reinstall look like an edit.
+    const block = {
+      name: "hero",
+      type: "hyperframes:block",
+      files: [{ path: "hero.html", target: "blocks/hero.html", type: "hyperframes:composition" }],
+    } as unknown as RegistryItem;
+
+    const dir = project();
+    await installItem(block, { destDir: dir });
+    const second = await installItem(block, { destDir: dir });
+
+    expect(second.preserved).toEqual([]);
+    expect(second.written).toHaveLength(1);
+  });
+});
+
+describe("installing several items, as a dependency plan does", () => {
+  const other = {
+    name: "shared-caption",
+    type: "hyperframes:component",
+    files: [
+      {
+        path: "shared-caption.html",
+        target: "components/shared-caption.html",
+        type: "hyperframes:file",
+      },
+    ],
+  } as unknown as RegistryItem;
+
+  const otherTarget = "components/shared-caption.html";
+
+  it("keeps a record for every item, not just the last one installed", () => {
+    // `add` installs dependencies first and the requested item last. A record
+    // rewritten per item rather than merged would forget the dependency, and
+    // the next install would read it as edited and refuse to update it.
+    const dir = project();
+    return installItem(item, { destDir: dir })
+      .then(() => installItem(other, { destDir: dir }))
+      .then(() => {
+        const record = JSON.parse(readFileSync(join(dir, "hyperframes.lock.json"), "utf-8"));
+        expect(Object.keys(record).sort()).toEqual([otherTarget, target].sort());
+      });
+  });
+
+  it("preserves an edit to one item while updating another", async () => {
+    const dir = project();
+    await installItem(item, { destDir: dir });
+    await installItem(other, { destDir: dir });
+    writeFileSync(join(dir, target), "EDITED DEPENDENCY\n", "utf-8");
+
+    const edited = await installItem(item, { destDir: dir });
+    const untouched = await installItem(other, { destDir: dir });
 
-  it("allows `.` segments (no-op) and dotfile-like names", () => {
-    expect(() => assertSafeTarget(DEST, ".hidden")).not.toThrow();
-    expect(() => assertSafeTarget(DEST, "./file.html")).not.toThrow();
-    expect(() => assertSafeTarget(DEST, "a..b/file.html")).not.toThrow();
+    expect(edited.preserved).toHaveLength(1);
+    expect(untouched.written).toHaveLength(1);
+    expect(readFileSync(join(dir, target), "utf-8")).toBe("EDITED DEPENDENCY\n");
   });
 });

packages/cli/src/registry/installer.ts

@@ -6,7 +6,8 @@
  * runtime to reject traversal even if the registry JSON schema was bypassed.
  */
 
-import { readFileSync, writeFileSync } from "node:fs";
+import { createHash } from "node:crypto";
+import { existsSync, readFileSync, writeFileSync } from "node:fs";
 import { resolve, relative, isAbsolute } from "node:path";
 import type { FileTarget, RegistryItem } from "@hyperframes/core";
 import { fetchItemFile, DEFAULT_REGISTRY_URL } from "./remote.js";
@@ -16,11 +17,72 @@ export interface InstallOptions {
   destDir: string;
   /** Base URL of the registry. Defaults to the official public registry. */
   baseUrl?: string;
+  /** Overwrite files the project has changed since they were installed. */
+  force?: boolean;
 }
 
 export interface InstallResult {
   /** Absolute paths of files actually written. */
   written: string[];
+  /** Absolute paths left alone because the project had changed them. */
+  preserved: string[];
+}
+
+/**
+ * What each installed file looked like when we installed it.
+ *
+ * Reinstalling an item used to overwrite whatever was on disk, so a project
+ * that had tuned a block's colours lost that work to the next `add`. Comparing
+ * the file against the hash we recorded is what tells an untouched file, which
+ * is safe to replace, apart from an edited one, which is not.
+ */
+const INSTALL_RECORD = "hyperframes.lock.json";
+
+type InstallRecord = Record<string, string>;
+
+function digest(contents: Buffer | string): string {
+  return createHash("sha256").update(contents).digest("hex");
+}
+
+function readInstallRecord(destDir: string): InstallRecord {
+  const path = resolve(destDir, INSTALL_RECORD);
+  if (!existsSync(path)) return {};
+  try {
+    const parsed: unknown = JSON.parse(readFileSync(path, "utf-8"));
+    // A hand-edited or truncated record must not take the project's files with
+    // it: an unreadable record means "provenance unknown", which is the
+    // cautious answer everywhere it is used.
+    if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return {};
+    const record: InstallRecord = {};
+    for (const [target, hash] of Object.entries(parsed as Record<string, unknown>)) {
+      if (typeof hash === "string") record[target] = hash;
+    }
+    return record;
+  } catch {
+    return {};
+  }
+}
+
+function writeInstallRecord(destDir: string, record: InstallRecord): void {
+  const sorted = Object.fromEntries(Object.entries(record).sort(([a], [b]) => a.localeCompare(b)));
+  writeFileSync(resolve(destDir, INSTALL_RECORD), `${JSON.stringify(sorted, null, 2)}\n`, "utf-8");
+}
+
+/**
+ * Has the project changed this file since we installed it?
+ *
+ * A file we have no record of counts as changed. That covers the project that
+ * wrote the file itself, and the one that installed before this record existed;
+ * both would rather keep their file than have it silently replaced.
+ */
+export function hasLocalEdits(
+  record: InstallRecord,
+  target: string,
+  onDisk: Buffer | string,
+): boolean {
+  const installed = record[target];
+  if (!installed) return true;
+  return installed !== digest(onDisk);
 }
 
 /**
@@ -78,17 +140,48 @@ export async function installItem(
     assertSafeTarget(destDir, file.target);
   }
 
-  const written = await Promise.all(
+  const record = readInstallRecord(destDir);
+
+  const outcomes = await Promise.all(
     item.files.map(async (file: FileTarget) => {
       const destPath = resolve(destDir, file.target);
+
+      // Decided before fetching rather than after: a file we are going to keep
+      // should never be overwritten and then put back, because a crash in
+      // between would lose it for real.
+      if (
+        !options.force &&
+        existsSync(destPath) &&
+        hasLocalEdits(record, file.target, readFileSync(destPath))
+      ) {
+        return { destPath, target: file.target, preserved: true, hash: null };
+      }
+
       await fetchItemFile(item, file, destPath, baseUrl);
       if (isInstalledRegistryBlockComposition(item, file)) {
         const source = readFileSync(destPath, "utf-8");
         writeFileSync(destPath, addRegistryItemMarker(source, item), "utf-8");
       }
-      return destPath;
+      // Hash what actually landed, marker included, or the next install reads
+      // its own marker as the project's edit.
+      return {
+        destPath,
+        target: file.target,
+        preserved: false,
+        hash: digest(readFileSync(destPath)),
+      };
     }),
   );
 
-  return { written };
+  const written = outcomes.filter((o) => !o.preserved).map((o) => o.destPath);
+  const preserved = outcomes.filter((o) => o.preserved).map((o) => o.destPath);
+
+  if (written.length > 0) {
+    for (const outcome of outcomes) {
+      if (outcome.hash) record[outcome.target] = outcome.hash;
+    }
+    writeInstallRecord(destDir, record);
+  }
+
+  return { written, preserved };
 }