fix(cli): add --vars refuses a value of the wrong type instead of writing it (#4369)

* fix(cli): add --vars refuses a value of the wrong type instead of writing it

* fix(cli): a refused --vars writes none of the item's dependencies either

* fix(cli): keep a file edited while add was still downloading the rest of the plan

* style(cli): shorten the --vars refusal comment

packages/cli/src/commands/add.test.ts

@@ -156,6 +156,8 @@ const ITEM_BY_NAME: Record<string, RegistryItem> = {
   "my-example": EXAMPLE_ITEM,
 };
 
+const DEP_BLOCK_HTML = `<div data-composition-variables='[{ "id": "maths", "type": "boolean", "label": "Maths", "default": false }]'></div>`;
+
 function mockFetch(): void {
   vi.stubGlobal(
     "fetch",
@@ -171,6 +173,9 @@ function mockFetch(): void {
       }
       // File fetch — match `/<type-dir>/<name>/<rest>` and serve synthetic content.
       const f = /\/(examples|blocks|components)\/([^/]+)\/(.+)$/.exec(url);
+      if (f?.[3] === "dep-block.html") {
+        return new Response(DEP_BLOCK_HTML, { status: 200 });
+      }
       if (f) {
         return new Response(`/* ${f[3]} */\n`, { status: 200 });
       }
@@ -357,6 +362,20 @@ describe("runAdd (integration, mocked registry)", () => {
     }
   });
 
+  it("refuses a wrong-typed --vars before writing any item in the plan", async () => {
+    const dir = tmp();
+    try {
+      writeRegistryConfig(dir);
+      await expect(
+        runAdd({ name: "dep-block", projectDir: dir, skipClipboard: true, vars: '{"maths":1}' }),
+      ).rejects.toThrow(/maths: expected boolean, got number/);
+      expect(existsSync(join(dir, "compositions/components/base-component"))).toBe(false);
+      expect(existsSync(join(dir, "hyperframes.lock.json"))).toBe(false);
+    } finally {
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
   it("installs transitive registryDependencies before the requested item", async () => {
     const dir = tmp();
     try {

packages/cli/src/commands/add.ts

@@ -15,8 +15,15 @@ import { existsSync } from "node:fs";
 import { resolve, relative } from "node:path";
 import { ITEM_TYPE_DIRS, type RegistryItem } from "@hyperframes/core";
 import { c } from "../ui/colors.js";
-import { DEFAULT_REGISTRY_URL, installItem, resolveItemsByTag } from "../registry/index.js";
+import {
+  DEFAULT_REGISTRY_URL,
+  prepareItem,
+  publishItem,
+  resolveItemsByTag,
+  type PreparedItem,
+} from "../registry/index.js";
 import { resolveItemWithDependencies } from "../registry/resolver.js";
+import { InvalidVariableValuesError } from "../registry/variableDefaults.js";
 import {
   gateRegistryItemsCompatibility,
   RegistryCompatibilityError,
@@ -194,8 +201,8 @@ function assertCompatibleOrThrow(items: RegistryItem[], cliVersion?: string): st
 }
 
 // Install a topologically-ordered plan (dependencies first, requested item
-// last). The installer validates every target before any write; a failure on
-// any item surfaces as an install-failed AddError. Returns all written paths.
+// last). Every item is fetched and checked before any is written, so a refusal
+// or failed fetch anywhere in the plan writes nothing. Returns all written paths.
 async function installAll(
   installPlan: RegistryItem[],
   destDir: string,
@@ -208,35 +215,39 @@ async function installAll(
   preserved: string[];
   variablesApplied: string[];
   variablesUnknown: string[];
-  variablesInvalid: { id: string; reason: string }[];
 }> {
   const written: string[] = [];
   const preserved: string[] = [];
   let variablesApplied: string[] = [];
   let variablesUnknown: string[] = [];
-  let variablesInvalid: { id: string; reason: string }[] = [];
   try {
+    const prepared: PreparedItem[] = [];
     for (const planItem of installPlan) {
-      const result = await installItem(planItem, {
-        destDir,
-        baseUrl,
-        force,
-        // Only the item the user named. A dependency dragged in behind it never
-        // declared these variables and must not be rewritten by them.
-        variableValues: planItem.name === requestedName ? variableValues : null,
-      });
+      prepared.push(
+        await prepareItem(planItem, {
+          destDir,
+          baseUrl,
+          force,
+          // Only the item the user named. A dependency dragged in behind it never
+          // declared these variables and must not be rewritten by them.
+          variableValues: planItem.name === requestedName ? variableValues : null,
+        }),
+      );
+    }
+    for (const [i, planItem] of installPlan.entries()) {
+      const result = publishItem(prepared[i]!);
       written.push(...result.written);
       preserved.push(...result.preserved);
       if (planItem.name === requestedName) {
         variablesApplied = result.variablesApplied;
         variablesUnknown = result.variablesUnknown;
-        variablesInvalid = result.variablesInvalid;
       }
     }
   } catch (err) {
+    if (err instanceof InvalidVariableValuesError) throw new AddError(err.message, "invalid-vars");
     throw new AddError(describeInstallFailure(err, baseUrl), "install-failed");
   }
-  return { written, preserved, variablesApplied, variablesUnknown, variablesInvalid };
+  return { written, preserved, variablesApplied, variablesUnknown };
 }
 
 /**
@@ -318,15 +329,14 @@ export async function runAdd(opts: RunAddArgs): Promise<RunAddResult> {
 
   // 5. Install — dependencies first, requested item last.
   const variableValues = parseVariableValues(opts.vars);
-  const { written, preserved, variablesApplied, variablesUnknown, variablesInvalid } =
-    await installAll(
-      installPlan,
-      projectDir,
-      config.registry,
-      opts.force ?? false,
-      item.name,
-      variableValues,
-    );
+  const { written, preserved, variablesApplied, variablesUnknown } = await installAll(
+    installPlan,
+    projectDir,
+    config.registry,
+    opts.force ?? false,
+    item.name,
+    variableValues,
+  );
 
   // 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
@@ -358,9 +368,6 @@ export async function runAdd(opts: RunAddArgs): Promise<RunAddResult> {
   const snippet = buildSnippet(item, snippetTargetRel, variableValues);
   const clipboardCopied = !opts.skipClipboard && snippet ? copyToClipboard(snippet) : false;
 
-  for (const { id, reason } of variablesInvalid) {
-    warnings.push(`--vars ${id} ignored: ${reason}`);
-  }
   if (variablesUnknown.length > 0) {
     warnings.push(`--vars ignored (not declared by ${item.name}): ${variablesUnknown.join(", ")}`);
   }
@@ -539,8 +546,9 @@ export default defineCommand({
             if (!json) console.log(`  ${c.warn("Warning:")} ${warning}`);
           }
           if (!json) console.log(`  ${c.success("✓")} ${result.name}`);
-        } catch {
-          if (!json) console.log(`  ${c.error("✗")} ${item.name} (skipped)`);
+        } catch (err) {
+          const reason = err instanceof Error ? err.message : String(err);
+          if (!json) console.log(`  ${c.error("✗")} ${item.name} (skipped: ${reason})`);
         }
       }
 

packages/cli/src/registry/index.ts

@@ -5,4 +5,4 @@ export { DEFAULT_REGISTRY_URL } from "./remote.js";
 
 export { listRegistryItems, loadAllItems, resolveItemsByTag } from "./resolver.js";
 
-export { installItem } from "./installer.js";
+export { installItem, prepareItem, publishItem, type PreparedItem } from "./installer.js";

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

@@ -21,7 +21,7 @@ vi.mock("./remote.js", () => ({
   fetchItemFile: vi.fn(async () => Buffer.from(remote.contents)),
 }));
 
-const { hasLocalEdits, installItem } = await import("./installer.js");
+const { hasLocalEdits, installItem, prepareItem, publishItem } = await import("./installer.js");
 
 function project(): string {
   return mkdtempSync(join(tmpdir(), "hf-installer-"));
@@ -148,6 +148,21 @@ describe("installItem", () => {
     expect(readFileSync(join(dir, target), "utf-8")).toBe("MY OWN COLOURS\n");
   });
 
+  it("keeps an edit saved while the rest of the plan was still downloading", async () => {
+    const dir = project();
+    await installItem(item, { destDir: dir });
+    remote.contents = "REGISTRY VERSION 2\n";
+    const prepared = await prepareItem(item, { destDir: dir });
+    remote.contents = "REGISTRY VERSION\n";
+    writeFileSync(join(dir, target), "MY OWN COLOURS\n", "utf-8");
+
+    const result = publishItem(prepared);
+
+    expect(result.written).toEqual([]);
+    expect(result.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 });
@@ -236,3 +251,42 @@ describe("installing several items, as a dependency plan does", () => {
     expect(readFileSync(join(dir, target), "utf-8")).toBe("EDITED DEPENDENCY\n");
   });
 });
+
+describe("installing with --vars the item cannot take", () => {
+  const declaration = `<div data-composition-id="demo" data-composition-variables='[{"id":"maths","type":"boolean","label":"Maths","default":false}]'></div>`;
+  const block = {
+    name: "demo-block",
+    title: "Demo block",
+    description: "Block",
+    dimensions: { width: 100, height: 100 },
+    duration: 1,
+    type: "hyperframes:block",
+    files: [
+      {
+        path: "demo-block.html",
+        target: "compositions/demo-block.html",
+        type: "hyperframes:composition",
+      },
+    ],
+  } as unknown as RegistryItem;
+
+  it.each([
+    ["block", block, "compositions/demo-block.html"],
+    ["component", item, target],
+  ])(
+    "refuses a wrong-typed value for a %s and writes nothing",
+    async (_kind, installable, file) => {
+      const dir = project();
+      remote.contents = declaration;
+      try {
+        await expect(
+          installItem(installable, { destDir: dir, variableValues: { maths: 1 } }),
+        ).rejects.toThrow(/maths: expected boolean, got number/);
+        expect(existsSync(join(dir, file))).toBe(false);
+      } finally {
+        remote.contents = "REGISTRY VERSION\n";
+        rmSync(dir, { recursive: true, force: true });
+      }
+    },
+  );
+});

packages/cli/src/registry/installer.ts

@@ -14,7 +14,11 @@ import { existsSync, readFileSync } 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";
-import { applyVariableDefaults, type ApplyResult } from "./variableDefaults.js";
+import {
+  applyVariableDefaults,
+  InvalidVariableValuesError,
+  type ApplyResult,
+} from "./variableDefaults.js";
 
 export interface InstallOptions {
   /** Project root where files land. Every target resolves relative to this. */
@@ -40,7 +44,6 @@ export interface InstallResult {
   variablesApplied: string[];
   /** Ids the item does not declare, and ids it declares but cannot accept. */
   variablesUnknown: string[];
-  variablesInvalid: { id: string; reason: string }[];
 }
 
 /**
@@ -100,6 +103,16 @@ export function hasLocalEdits(
   return installed !== digest(onDisk);
 }
 
+/** An install must leave this file alone: the project changed it, and `--force` was not given. */
+function keptByProject(
+  record: InstallRecord,
+  destPath: string,
+  target: string,
+  force: boolean | undefined,
+): boolean {
+  return !force && existsSync(destPath) && hasLocalEdits(record, target, readFileSync(destPath));
+}
+
 /**
  * Reject target paths that would escape `destDir`. Mirrors the pattern check
  * in `packages/core/schemas/registry-item.json#files.items.target`, but runs at
@@ -144,16 +157,37 @@ function addRegistryItemMarker(source: string, item: RegistryItem): string {
   return `<!-- hyperframes-registry-item: ${item.name} -->\n${source}`;
 }
 
-interface FileOutcome {
+export interface FileOutcome {
   destPath: string;
   target: string;
   preserved: boolean;
   hash: string | null;
   vars: ApplyResult | null;
+  /** What to write; null for a preserved file. */
+  bytes: Buffer | null;
 }
 
-/** Fetch, write and post-process one file. Extracted so installItem stays readable. */
-async function installOneFile(
+/**
+ * Check `--vars` against the file that declares them. A block's values ride on
+ * its mount, so only a component's own declaration is rewritten.
+ */
+function bakeVariables(
+  item: RegistryItem,
+  file: FileTarget,
+  bytes: Buffer,
+  values: Record<string, unknown> | null | undefined,
+): { bytes: Buffer; vars: ApplyResult | null } {
+  const component = isInstalledComponentSnippet(item, file);
+  if (!values || !(component || isInstalledRegistryBlockComposition(item, file))) {
+    return { bytes, vars: null };
+  }
+  const vars = applyVariableDefaults(bytes.toString("utf8"), values);
+  const rewrite = component && vars.applied.length > 0;
+  return { bytes: rewrite ? Buffer.from(vars.html) : bytes, vars };
+}
+
+/** Fetch and post-process one file; publishItem writes it once the whole plan is ready. */
+async function prepareOneFile(
   item: RegistryItem,
   file: FileTarget,
   destDir: string,
@@ -167,32 +201,26 @@ async function installOneFile(
   // 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, vars: null };
+  if (keptByProject(record, destPath, file.target, options.force)) {
+    return { destPath, target: file.target, preserved: true, hash: null, vars: null, bytes: null };
   }
 
   let bytes = await fetchItemFile(item, file, baseUrl, budget);
   if (isInstalledRegistryBlockComposition(item, file)) {
     bytes = Buffer.from(addRegistryItemMarker(bytes.toString("utf8"), item));
   }
-  let vars: ApplyResult | null = null;
-  if (options.variableValues && isInstalledComponentSnippet(item, file)) {
-    vars = applyVariableDefaults(bytes.toString("utf8"), options.variableValues);
-    if (vars.applied.length > 0) bytes = Buffer.from(vars.html);
-  }
-  publishRegistryFile(destDir, file.target, bytes);
-  // Hash what actually landed, marker and baked defaults included, or the
-  // next install reads its own output as the project's edit.
+  const baked = bakeVariables(item, file, bytes, options.variableValues);
+  bytes = baked.bytes;
+  const vars = baked.vars;
+  // Hash what will land, marker and baked defaults included, or the next
+  // install reads its own output as the project's edit.
   return {
     destPath,
     target: file.target,
     preserved: false,
     hash: digest(bytes),
     vars,
+    bytes,
   };
 }
 
@@ -204,6 +232,21 @@ export async function installItem(
   item: RegistryItem,
   options: InstallOptions,
 ): Promise<InstallResult> {
+  return publishItem(await prepareItem(item, options));
+}
+
+/** An item fetched and checked, with nothing written yet. */
+export interface PreparedItem {
+  root: string;
+  outcomes: FileOutcome[];
+  force: boolean;
+}
+
+/** Fetch and check every file of an item without writing, so a caller can refuse a whole plan. */
+export async function prepareItem(
+  item: RegistryItem,
+  options: InstallOptions,
+): Promise<PreparedItem> {
   if (!validRegistryItem(item, item.name, item.type)) throw new Error("Invalid registry item");
   const baseUrl = options.baseUrl ?? DEFAULT_REGISTRY_URL;
   const destDir = resolve(options.destDir);
@@ -219,8 +262,26 @@ export async function installItem(
   const budget = { remainingBytes: 512 * 1024 * 1024 };
 
   const outcomes = await installFileBatches(item.files, (file) =>
-    installOneFile(item, file, root, baseUrl, record, options, budget),
+    prepareOneFile(item, file, root, baseUrl, record, options, budget),
   );
+  const invalid = outcomes.flatMap((o) => o.vars?.invalid ?? []);
+  if (invalid.length > 0) throw new InvalidVariableValuesError(invalid);
+  return { root, outcomes, force: options.force ?? false };
+}
+
+/** Write a prepared item and record what landed. */
+export function publishItem({ root, outcomes: prepared, force }: PreparedItem): InstallResult {
+  // Read now, not at prepare time: another item published in between has recorded its own files.
+  const record = readInstallRecord(root);
+  // Asked again at write time: a save made while the rest of the plan downloaded is still an edit.
+  const outcomes = prepared.map((o) =>
+    o.bytes && keptByProject(record, o.destPath, o.target, force)
+      ? { ...o, preserved: true, hash: null, vars: null, bytes: null }
+      : o,
+  );
+  for (const outcome of outcomes) {
+    if (outcome.bytes) publishRegistryFile(root, outcome.target, outcome.bytes);
+  }
 
   const written = outcomes.filter((o) => !o.preserved).map((o) => o.destPath);
   const preserved = outcomes.filter((o) => o.preserved).map((o) => o.destPath);
@@ -245,7 +306,6 @@ export async function installItem(
           vars[0]!.unknown,
         )
       : [],
-    variablesInvalid: vars.flatMap((v) => v.invalid),
   };
 }
 
@@ -267,11 +327,11 @@ function validatePhysicalTargets(root: string, files: FileTarget[]): void {
 
 async function installFileBatches(
   files: FileTarget[],
-  install: (file: FileTarget) => Promise<FileOutcome>,
+  prepare: (file: FileTarget) => Promise<FileOutcome>,
 ): Promise<FileOutcome[]> {
   const outcomes: FileOutcome[] = [];
   for (let at = 0; at < files.length; at += 4) {
-    const batch = await Promise.allSettled(files.slice(at, at + 4).map(install));
+    const batch = await Promise.allSettled(files.slice(at, at + 4).map(prepare));
     for (const result of batch) {
       if (result.status === "rejected") throw result.reason;
       outcomes.push(result.value);

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

@@ -106,3 +106,30 @@ describe("applyVariableDefaults", () => {
     expect(applyVariableDefaults(COMPONENT, {}).html).toBe(COMPONENT);
   });
 });
+
+/** One variable of each type the CLI previously wrote without checking. */
+const TYPED = `<div
+  data-hf-ui-root
+  data-composition-variables='[
+    { "id": "maths", "type": "boolean", "label": "Maths", "default": false },
+    { "id": "accent", "type": "color", "label": "Accent", "default": "#7a4a2e" },
+    { "id": "title", "type": "string", "label": "Title", "default": "Hello" }
+  ]'
+></div>`;
+
+describe("applyVariableDefaults type checks", () => {
+  it("refuses a value whose type does not match the declaration", () => {
+    expect(applyVariableDefaults(TYPED, { maths: 1, accent: 5, title: {} }).invalid).toEqual([
+      { id: "maths", reason: "expected boolean, got number" },
+      { id: "accent", reason: "expected color, got number" },
+      { id: "title", reason: "expected string, got object" },
+    ]);
+  });
+
+  it("keeps a boolean a boolean, so the composition's own check still sees true", () => {
+    const r = applyVariableDefaults(TYPED, { maths: true, accent: "#ffffff" });
+    expect(r.invalid).toEqual([]);
+    expect(defaultOf(r.html, "maths")).toBe(true);
+    expect(defaultOf(r.html, "accent")).toBe("#ffffff");
+  });
+});

packages/cli/src/registry/variableDefaults.ts

@@ -16,7 +16,11 @@
  * silently discarded for every component in the catalog.
  */
 
-import { isCompositionVariable, type CompositionVariable } from "@hyperframes/core/variables";
+import {
+  isCompositionVariable,
+  validateVariables,
+  type CompositionVariable,
+} from "@hyperframes/core/variables";
 
 export interface ApplyResult {
   /** The source with defaults rewritten. Unchanged when nothing applied. */
@@ -29,6 +33,14 @@ export interface ApplyResult {
   invalid: { id: string; reason: string }[];
 }
 
+/** `--vars` values the item's declaration cannot take; the install writes nothing. */
+export class InvalidVariableValuesError extends Error {
+  constructor(readonly invalid: { id: string; reason: string }[]) {
+    super(`--vars refused: ${invalid.map(({ id, reason }) => `${id}: ${reason}`).join("; ")}`);
+    this.name = "InvalidVariableValuesError";
+  }
+}
+
 const ATTR = "data-composition-variables";
 
 /** Locate the attribute's quoted value, tolerating either delimiter. */
@@ -52,23 +64,6 @@ function encode(json: string, quote: string): string {
   return quote === "'" ? json.replace(/'/g, "'") : json.replace(/"/g, """);
 }
 
-function optionValues(decl: CompositionVariable): string[] | null {
-  return decl.type === "enum" ? decl.options.map((option) => option.value) : null;
-}
-
-/**
- * Reject a value the declaration cannot represent, rather than writing it.
- *
- * A bad enum falls back to the default at runtime and warns, so writing one
- * here would produce a file that renders as if the value had been ignored --
- * which is the exact failure this function exists to remove.
- */
-function rejectEnum(decl: CompositionVariable, value: unknown): string | null {
-  const options = optionValues(decl);
-  if (!options) return null;
-  return options.includes(String(value)) ? null : `not one of ${options.join(", ")}`;
-}
-
 function rejectNumber(decl: CompositionVariable, value: unknown): string | null {
   if (decl.type !== "number") return null;
   const n = typeof value === "number" ? value : Number(value);
@@ -78,8 +73,15 @@ function rejectNumber(decl: CompositionVariable, value: unknown): string | null
   return null;
 }
 
+// Refuse what the runtime would ignore. Numbers keep their own check (string input, min/max);
+// every other type goes through the runtime's validator.
 function reject(decl: CompositionVariable, value: unknown): string | null {
-  return rejectEnum(decl, value) ?? rejectNumber(decl, value);
+  if (decl.type === "number") return rejectNumber(decl, value);
+  const [issue] = validateVariables({ [decl.id]: value }, [decl]);
+  if (!issue) return null;
+  if (issue.kind === "enum-out-of-range") return `not one of ${issue.allowed.join(", ")}`;
+  if (issue.kind === "type-mismatch") return `expected ${issue.expected}, got ${issue.actual}`;
+  return null;
 }
 
 export function applyVariableDefaults(
@@ -116,7 +118,7 @@ export function applyVariableDefaults(
   const applied: string[] = [];
   const invalid: { id: string; reason: string }[] = [];
   const byId = new Map(declared.map((decl) => [decl.id, decl]));
-  const updated = new Map<string, string | number>();
+  const updated = new Map<string, unknown>();
 
   for (const [id, value] of Object.entries(values)) {
     const decl = byId.get(id);
@@ -129,7 +131,7 @@ export function applyVariableDefaults(
     // The declaration's own type decides how the value is stored. A number
     // written as the string "76" would trip the composition's guard and fall
     // back, which looks exactly like the value being ignored.
-    updated.set(id, decl.type === "number" ? Number(value) : String(value));
+    updated.set(id, decl.type === "number" ? Number(value) : value);
     applied.push(id);
   }