fix: media-use bug-bash fixes (codex gate, id race, provider/reuse/adopt guards) + CLI unknown-flag rejection (#2033)

* fix(media-use): codex gate misfires as 'not logged in' when piped codexUnavailableReason() gated generation on parsing `codex login status` stdout, but that command prints 'Logged in using ChatGPT' to stderr and exits 0 — so the piped stdout media-use captures (execFileSync returns stdout only on success) was empty, and the gate falsely reported 'not logged in'. Every headless / CI / agent run was blocked from codex image gen even when fully authed. Gate on the durable credentials file ($CODEX_HOME/auth.json) instead of the TTY/stderr-only human text. Token validity is still proven by the exec, which fails cleanly on a stale login. The stdout `features list` capability check is unchanged. Verified: reproduced the false 'not logged in' block, then after the fix generated end-to-end via `resolve -t image --provider codex` (valid 1254x1254 PNG, source=generated, provider=codex.image_gen). * fix(media-use): bug-bash fixes — id race, provider/reuse/adopt guards From the bug-bash against main: - MU-23 (HIGH): concurrent resolves raced on nextId (read-max-then-append, non-atomic), so parallel agents got duplicate ids and clobbered each other's files. Add allocateId(): a coarse per-project lock (.media/.lock, 15s stale-steal) around id allocation that scans the manifest AND the type dir for reserved ids, then O_EXCL-creates a placeholder file so the slow download between allocate and append can't collide. 5 parallel resolves now yield 5 distinct ids + files. - X4: --reuse imported across a type mismatch (bgm asset under images/). Apply typesMatch on the --reuse path; reject mismatches (icon<->image still interchangeable). - X5: --provider silently overrode --local-only and made a network call. --local-only is now a hard guard: network providers are skipped even under a forced provider; the miss message explains the conflict. - BUG-2: --provider ignored the exact-cache floor and could hand back an asset from a different provider. A forced --provider now bypasses all reuse rungs (regenerate with THIS provider); the unforced floor is intact. - MU-26/X6: 0-byte assets accepted. --adopt skips 0-byte files (loud); ingest refuses a 0-byte local file (freezeUrl already rejects empty responses). - BUG-4: unknown/unavailable --provider now errors with the available list instead of a generic 'no provider could resolve' (typo != catalog miss). - BUG-5: --reuse "" gave the wrong 'type and intent required' error; it now routes to a clear empty-sha message. - BUG-3: voice duration leaked an unrounded float into index.md; round all durations to 0.1s centrally at record build (matches probe). - Nits: whitespace-only --intent is rejected; nudge grammar (exists/exist). Tests: allocateId reservation + registry local-only-wins added; full media-use suite green. All fixes verified e2e. * fix(cli): reject unknown flags instead of silently ignoring them citty is permissive: an unrecognized flag was dropped, not rejected — so `render . --out x` (the flag is --output/-o) silently ignored --out and rendered to the default renders/<name>.mp4 path. A mistyped flag read as a render/catalog miss. Add assertKnownFlags(): validate every dash-prefixed token against the command's declared args + aliases + the global set (help/version/json) before the command runs, in the shared trackCommandFailures run-wrapper so every leaf command is covered. Handles --flag=value, --no-<bool> negation, camelCase<->kebab arg names, and combined shorts; stops at --; positionals and flag values pass through. Verified: `render . --out x` -> 'Error: Unknown flag: --out'; --output/-o/ --json/--help still accepted. Unit tests added. * docs(skills): install with --full-depth so agents get current main The documented `npx skills add heygen-com/hyperframes` fetched the skills.sh registry blob, which lags GitHub main by hours — so users following the docs got a stale skill (e.g. media-use v1: no --candidates, voice stubbed). The CLI's own `hyperframes skills` command already forces a full clone via --full-depth to bypass this; the docs didn't pass it. Add --full-depth to every documented install command (README, CLAUDE.md, docs/guides/skills.mdx) with a one-line note on the lag. Addresses the user-facing half of the publish/registry lag (#2034). * chore(media-use): collapse resolve.mjs import to satisfy oxfmt --check * fix(cli): extract longFlagName to keep flag validator under complexity gate Also regenerate skills-manifest.json (resolve.mjs formatting change re-hashed the media-use skill). Fixes the Fallow audit + skills-manifest-in-sync CI gates.
2026-09-03 04:38:33 +00:00 · 2026-07-07 19:19:28 -04:00
parent 924727a0b4
commit 401dd1d27f
14 changed files with 353 additions and 45 deletions
@@ -54,6 +54,12 @@ export function scanExistingAssets(projectDir) {
    if (!type) continue;
    const fullPath = join(assetsDir, rel);
    const stat = statSync(fullPath);
+    if (stat.size === 0) {
+      // A 0-byte asset would register clean but fail at render — skip it loudly
+      // rather than adopt a broken file.
+      console.error(`media-use: skipping 0-byte asset assets/${rel}`);
+      continue;
+    }
    const meta = probe(fullPath);
    found.push({
      relativePath: `assets/${rel}`,
@@ -64,8 +64,15 @@ function codexUnavailableReason() {
  } catch {
    return "codex CLI not on PATH";
  }
-  const login = codexRun(["login", "status"]);
-  if (!login || !/logged in/i.test(login)) return "codex not logged in (run: codex login)";
+  // Auth marker: presence of the credentials file, NOT `codex login status`.
+  // That command prints "Logged in using ChatGPT" only to a human stream
+  // (stderr / TTY) and exits 0, so its piped stdout — how media-use spawns it —
+  // is empty, and the gate falsely reported "not logged in", blocking codex
+  // image gen in every headless / CI / agent run even when fully authed.
+  // auth.json is the durable, TTY-independent signal; token validity is proven
+  // by the exec itself, which fails cleanly if the login is stale.
+  const authPath = join(process.env.CODEX_HOME || join(homedir(), ".codex"), "auth.json");
+  if (!existsSync(authPath)) return "codex not logged in (run: codex login)";
  const feats = codexRun(["features", "list"]);
  if (feats == null) return "could not read `codex features list`";
  if (!/\bimage_generation\b/.test(feats)) return "codex image_generation feature unavailable";
@@ -1,4 +1,15 @@
-import { readFileSync, appendFileSync, mkdirSync, existsSync } from "node:fs";
+import {
+  readFileSync,
+  appendFileSync,
+  mkdirSync,
+  existsSync,
+  readdirSync,
+  openSync,
+  closeSync,
+  writeFileSync,
+  rmSync,
+  statSync,
+} from "node:fs";
 import { join } from "node:path";

 const MANIFEST_FILE = "manifest.jsonl";
@@ -103,3 +114,75 @@ export function nextId(projectDir, type) {
  }
  return `${prefix}_${String(max + 1).padStart(3, "0")}`;
 }
+
+// Sync sleep (no busy-spin) for the allocation lock retry.
+function sleepMs(ms) {
+  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
+}
+
+// Coarse per-project lock so concurrent resolves don't race on id allocation.
+// ponytail: one lock file with a 15s stale-steal (a crashed holder can't wedge
+// the project); fine for agent-scale concurrency — revisit if throughput needs
+// finer locking. Date.now() is available here (a normal Node CLI, not a
+// workflow DSL), so mtime-based staleness is safe.
+const LOCK_STALE_MS = 15000;
+const LOCK_TIMEOUT_MS = 20000;
+
+function withLock(dir, fn) {
+  const lock = join(dir, ".lock");
+  const start = Date.now();
+  for (;;) {
+    try {
+      closeSync(openSync(lock, "wx")); // O_EXCL: atomic acquire
+      break;
+    } catch (err) {
+      if (err.code !== "EEXIST") throw err;
+      try {
+        if (Date.now() - statSync(lock).mtimeMs > LOCK_STALE_MS) {
+          rmSync(lock, { force: true }); // steal a stale lock from a dead holder
+          continue;
+        }
+      } catch {
+        continue; // lock vanished between check and stat — retry the acquire
+      }
+      if (Date.now() - start > LOCK_TIMEOUT_MS) {
+        throw new Error("media-use: timed out acquiring .media/.lock");
+      }
+      sleepMs(25);
+    }
+  }
+  try {
+    return fn();
+  } finally {
+    rmSync(lock, { force: true });
+  }
+}
+
+// Atomically allocate the next free id for `type` AND reserve its file, so a
+// slow download/copy between allocation and appendRecord can't let a concurrent
+// caller grab the same id (the MU-23 clobber). Under the lock we take the max id
+// across BOTH the manifest and any already-reserved files in the type dir, then
+// O_EXCL-create an empty placeholder at the target path; freeze/copy overwrites
+// it. Returns { id, localPath }.
+export function allocateId(projectDir, type, ext) {
+  mkdirSync(mediaDir(projectDir), { recursive: true });
+  const typeDir = typeDirPath(projectDir, type);
+  mkdirSync(typeDir, { recursive: true });
+  return withLock(mediaDir(projectDir), () => {
+    const re = new RegExp(`^${type}_(\\d+)`);
+    let max = 0;
+    for (const r of readManifest(projectDir)) {
+      if (r.type !== type) continue;
+      const m = r.id?.match(re);
+      if (m) max = Math.max(max, parseInt(m[1], 10));
+    }
+    for (const f of readdirSync(typeDir)) {
+      const m = f.match(re);
+      if (m) max = Math.max(max, parseInt(m[1], 10)); // skip ids reserved but not yet appended
+    }
+    const id = `${type}_${String(max + 1).padStart(3, "0")}`;
+    const localPath = `.media/${typeSubdir(type)}/${id}${ext}`;
+    writeFileSync(join(projectDir, localPath), "", { flag: "wx" }); // durable reservation
+    return { id, localPath };
+  });
+}
@@ -8,6 +8,7 @@ import {
  findByPrompt,
  findByEntity,
  nextId,
+  allocateId,
  normalizePrompt,
  manifestPath,
  mediaDir,
@@ -129,6 +130,28 @@ function runTests() {
    assert.equal(normalizePrompt(null), "");
  });

+  test("allocateId reserves the id on disk so a pre-append caller can't reuse it (MU-23)", () => {
+    setup();
+    const a = allocateId(tmp, "bgm", ".wav");
+    assert.equal(a.id, "bgm_001");
+    assert.ok(existsSync(join(tmp, a.localPath)), "placeholder reserved on disk");
+    // Second allocation BEFORE any manifest append (the download window) must not
+    // hand back bgm_001 again, even with a different extension.
+    const b = allocateId(tmp, "bgm", ".mp3");
+    assert.equal(b.id, "bgm_002");
+    assert.notEqual(a.localPath, b.localPath);
+    // Lock file is released (not left behind).
+    assert.ok(!existsSync(join(tmp, ".media", ".lock")), "lock released");
+    cleanup();
+  });
+
+  test("allocateId continues past the highest manifest id", () => {
+    setup();
+    appendRecord(tmp, makeRecord({ id: "bgm_005" }));
+    assert.equal(allocateId(tmp, "bgm", ".wav").id, "bgm_006");
+    cleanup();
+  });
+
  test("findByEntity matches case-insensitively", () => {
    setup();
    appendRecord(tmp, makeRecord({ entity: "GitHub", type: "icon" }));
@@ -79,6 +79,20 @@ export function listTypes() {
  return Object.keys(REGISTRY);
 }

+/** Provider names available for a type, in cascade order (for --provider validation). */
+export function providerNamesFor(type) {
+  return listFor(type).map((p) => p.name);
+}
+
+/**
+ * Does an override token (full name like "codex.image_gen" or a prefix like
+ * "codex") match any provider declared for the type? Same match rule as
+ * runProviders, so validation and dispatch never disagree.
+ */
+export function providerMatches(type, want) {
+  return providerNamesFor(type).some((n) => n === want || n.startsWith(`${want}.`));
+}
+
 /**
 * Back-compat shim for the v1 single-provider API. Returns the first declared
 * provider for the type (tagged with `type`); throws for an unknown type.
@@ -94,17 +108,21 @@ export function getProvider(type) {
 * order, returns the first non-null result, skips providers that don't expose
 * the capability. Pure over its input — the unit-testable core of the cascade.
 *
- * Offline guard: a `network` provider is skipped when `ctx.localOnly` is set.
+ * Offline guard: a `network` provider is skipped when `ctx.localOnly` is set —
+ * unconditionally, even under a `ctx.provider` override. --local-only is a hard
+ * safety flag: it must never make a network call. Forcing a network provider
+ * while offline yields a clean miss (the caller explains the conflict), never a
+ * silent network request.
 * Provider override: `ctx.provider` (a full name like "codex.image_gen" or a
 * prefix like "codex") pins resolution to matching providers only — this is how
 * a user "make an image WITH codex" forces the upsell instead of taking the
- * free-first default. An override to a `network` provider ignores --local-only.
+ * free-first default.
 */
 export async function runProviders(providers, capability, intent, ctx) {
  const want = ctx?.provider;
  for (const p of providers) {
    if (want && p.name !== want && !p.name.startsWith(`${want}.`)) continue;
-    if (p.network && ctx?.localOnly && !want) continue; // --local-only: cache + local only
+    if (p.network && ctx?.localOnly) continue; // --local-only wins, even over --provider
    const fn = p[capability];
    if (typeof fn !== "function") continue;
    const res = await fn(intent, ctx);
@@ -68,10 +68,16 @@ test("ctx.provider forces one generator (e.g. 'make an image WITH codex')", asyn
    await runProviders(providers, "generate", "x", { provider: "codex.image_gen" }),
    { hit: "codex" },
  );
-  // forcing a network provider ignores --local-only (explicit user intent)
-  assert.deepEqual(
+  // --local-only wins even over a forced network provider: no network call,
+  // clean miss (the caller surfaces the conflict). A forced LOCAL provider under
+  // --local-only still runs.
+  assert.equal(
    await runProviders(providers, "generate", "x", { provider: "codex", localOnly: true }),
-    { hit: "codex" },
+    null,
+  );
+  assert.deepEqual(
+    await runProviders(providers, "generate", "x", { provider: "mflux", localOnly: true }),
+    { hit: "local" },
  );
 });

@@ -1,12 +1,12 @@
 #!/usr/bin/env node

-import { existsSync } from "node:fs";
+import { existsSync, statSync } from "node:fs";
 import { resolve, join, extname, basename } from "node:path";
 import { parseArgs } from "node:util";
-import { appendRecord, findByPrompt, findByEntity, nextId, typeSubdir } from "./lib/manifest.mjs";
+import { appendRecord, findByPrompt, findByEntity, nextId, allocateId } from "./lib/manifest.mjs";
 import { regenerateIndex } from "./lib/index-gen.mjs";
 import { cacheGet, cacheGetByEntity, importFromCache, cachePut } from "./lib/cache.mjs";
-import { runCapability, listTypes } from "./lib/registry.mjs";
+import { runCapability, listTypes, providerMatches, providerNamesFor } from "./lib/registry.mjs";
 import { freezeUrl, freezeLocalFile, isDirectMediaUrl } from "./lib/freeze.mjs";
 import { findExistingAsset } from "./lib/adopt.mjs";
 import { track } from "./lib/telemetry.mjs";
@@ -81,8 +81,10 @@ if (args.candidates || args["dry-run"]) {
 }

 // Reuse: import a specific global-cache asset (by content sha/prefix, taken
-// from --candidates) into this project.
-if (args.reuse) {
+// from --candidates) into this project. `!== undefined` so an empty --reuse ""
+// still routes here (and gets a clear empty-sha error) instead of falling
+// through to the misleading "--type and --intent are required".
+if (args.reuse !== undefined) {
  await reuseGlobal(args.reuse);
  process.exit(0);
 }
@@ -93,8 +95,8 @@ if (args.from) {
  process.exit(0);
 }

-if (!args.type || !args.intent) {
-  console.error("error: --type and --intent are required");
+if (!args.type || !args.intent || !args.intent.trim()) {
+  console.error("error: --type and a non-empty --intent are required");
  process.exit(2);
 }

@@ -103,14 +105,30 @@ if (!listTypes().includes(args.type)) {
  process.exit(2);
 }

+// Forced-provider validation: reject an unknown/unavailable provider name up
+// front so a typo reads as a typo, not a catalog miss (`no provider could
+// resolve`). Match rule mirrors runProviders (full name or dotted prefix).
+if (args.provider && !providerMatches(args.type, args.provider)) {
+  console.error(
+    `error: unknown provider "${args.provider}" for type ${args.type} (available: ${providerNamesFor(args.type).join(", ")})`,
+  );
+  process.exit(2);
+}
+
 const projectDir = resolve(args.project);
 const type = args.type;
 const intent = args.intent;
 const entity = args.entity || null;

 async function run() {
+  // A forced --provider means "(re)generate with THIS provider" — it bypasses
+  // every reuse rung (project/entity/assets/global cache) so it can't silently
+  // hand back an asset from a different provider. The floor only applies to the
+  // default (unforced) cascade.
+  const forced = !!args.provider;
+
  // 1. project manifest — exact-prompt match
-  const projectHit = findByPrompt(projectDir, intent, type);
+  const projectHit = forced ? null : findByPrompt(projectDir, intent, type);
  if (projectHit && existsSync(join(projectDir, projectHit.path))) {
    return result(projectHit, "cached");
  }
@@ -118,7 +136,7 @@ async function run() {
  // 1b. entity match in project. icon and image are interchangeable for
  // entity hits — both live in images/, and figma-imported brand marks are
  // always recorded as type image while agents ask for logos as type icon.
-  if (entity) {
+  if (!forced && entity) {
    const entityHit = findByEntity(projectDir, entity);
    if (
      entityHit &&
@@ -130,7 +148,7 @@ async function run() {
  }

  // 1c. scan existing assets/ directory for unregistered matches
-  const existingAsset = findExistingAsset(projectDir, intent, type);
+  const existingAsset = forced ? null : findExistingAsset(projectDir, intent, type);
  if (existingAsset) {
    const id = nextId(projectDir, type);
    const record = {
@@ -147,11 +165,10 @@ async function run() {
  }

  // 2. global cache — exact-prompt or entity match
-  const cacheHit = cacheGet(intent, type);
+  const cacheHit = forced ? null : cacheGet(intent, type);
  if (cacheHit) {
-    const id = nextId(projectDir, type);
    const ext = extname(cacheHit.cached_path);
-    const localPath = `.media/${typeSubdir(type)}/${id}${ext}`;
+    const { id, localPath } = allocateId(projectDir, type, ext);
    const imported = importFromCache(cacheHit, projectDir, id, localPath);
    if (imported) {
      appendRecord(projectDir, imported);
@@ -160,12 +177,11 @@ async function run() {
    }
  }

-  if (entity) {
+  if (!forced && entity) {
    const entityCacheHit = cacheGetByEntity(entity);
    if (entityCacheHit && typesMatch(entityCacheHit.type, type)) {
-      const id = nextId(projectDir, type);
      const ext = extname(entityCacheHit.cached_path);
-      const localPath = `.media/${typeSubdir(type)}/${id}${ext}`;
+      const { id, localPath } = allocateId(projectDir, type, ext);
      const imported = importFromCache(entityCacheHit, projectDir, id, localPath);
      if (imported) {
        appendRecord(projectDir, imported);
@@ -189,7 +205,7 @@ async function run() {
    const { similar } = listCandidates({ projectDir, type, intent, cap: CANDIDATE_CAP });
    if (similar > 0) {
      console.error(
-        `media-use: ${similar} similar cached asset${similar === 1 ? "" : "s"} already exist — run \`resolve --candidates --type ${type} --intent "${intent}"\` to review and reuse instead of fetching.`,
+        `media-use: ${similar} similar cached asset${similar === 1 ? "" : "s"} already ${similar === 1 ? "exists" : "exist"} — run \`resolve --candidates --type ${type} --intent "${intent}"\` to review and reuse instead of fetching.`,
      );
    }
  } catch {
@@ -224,7 +240,9 @@ async function run() {
    const msg =
      type === "brand"
        ? "no brand spec found — add a frame.md or design.md (colors/font/logo) to this project. Run the HyperFrames design flow to create one; brand tokens are read locally for deterministic rendering."
-        : `no provider could resolve ${type}: "${intent}"`;
+        : args.provider
+          ? `provider "${args.provider}" could not resolve ${type}: "${intent}"${localOnly ? " (--local-only skips network providers; drop it or the --provider override)" : ""}`
+          : `no provider could resolve ${type}: "${intent}"`;
    if (args.json) {
      console.log(JSON.stringify({ ok: false, error: msg }));
    } else {
@@ -233,10 +251,10 @@ async function run() {
    process.exit(1);
  }

-  // 5. freeze + register
-  const id = nextId(projectDir, type);
+  // 5. freeze + register (atomic id+file reservation so concurrent resolves
+  // can't collide on an id during the download — MU-23)
  const ext = searchResult.ext || extFromUrl(searchResult.url || "") || defaultExt(type);
-  const localPath = `.media/${typeSubdir(type)}/${id}${ext}`;
+  const { id, localPath } = allocateId(projectDir, type, ext);
  const fullPath = join(projectDir, localPath);

  if (searchResult.localPath) {
@@ -254,7 +272,9 @@ async function run() {
    path: localPath,
    source: searchResult.source || "search",
    description: searchResult.metadata?.description || intent,
-    ...(searchResult.metadata?.duration != null && { duration: searchResult.metadata.duration }),
+    ...(searchResult.metadata?.duration != null && {
+      duration: Math.round(searchResult.metadata.duration * 10) / 10, // round to 0.1s like probe (voice bypassed it)
+    }),
    ...(searchResult.metadata?.width != null && { width: searchResult.metadata.width }),
    ...(searchResult.metadata?.height != null && { height: searchResult.metadata.height }),
    ...(searchResult.metadata?.transparent != null && {
@@ -301,9 +321,14 @@ async function ingest(src) {
    console.error(`error: file not found: ${src}`);
    process.exit(2);
  }
-  const id = nextId(projectDir, type);
+  // Refuse 0-byte input: an empty asset would register clean but fail at render
+  // (freezeUrl already rejects empty responses; this covers local files).
+  if (!isUrl && statSync(resolve(src)).size === 0) {
+    console.error(`error: refusing to ingest a 0-byte file: ${src}`);
+    process.exit(2);
+  }
  const ext = extname(isUrl ? new URL(src).pathname : src) || defaultExt(type);
-  const localPath = `.media/${typeSubdir(type)}/${id}${ext}`;
+  const { id, localPath } = allocateId(projectDir, type, ext);
  const fullPath = join(projectDir, localPath);
  if (isUrl) await freezeUrl(src, fullPath);
  else freezeLocalFile(resolve(src), fullPath);
@@ -359,6 +384,10 @@ async function reuseGlobal(shaArg) {
    console.error(`error: --reuse requires --type (one of: ${listTypes().join(", ")})`);
    process.exit(2);
  }
+  if (!shaArg || !shaArg.trim()) {
+    console.error("error: --reuse needs a content sha/prefix (from `resolve --candidates`)");
+    process.exit(2);
+  }
  const rec = findGlobalBySha(shaArg);
  if (rec && rec.ambiguous) {
    console.error(
@@ -370,9 +399,14 @@ async function reuseGlobal(shaArg) {
    console.error(`error: no reusable global asset matches sha "${shaArg}"`);
    process.exit(1);
  }
-  const id = nextId(projectDir, type);
+  // Type guard: don't import a bgm asset as an image (audio under images/).
+  // icon<->image are interchangeable; everything else must match --type.
+  if (!typesMatch(rec.type, type)) {
+    console.error(`error: sha "${shaArg}" is a ${rec.type} asset, not ${type}`);
+    process.exit(2);
+  }
  const ext = extname(rec.cached_path || "") || defaultExt(type);
-  const localPath = `.media/${typeSubdir(type)}/${id}${ext}`;
+  const { id, localPath } = allocateId(projectDir, type, ext);
  const imported = importFromCache(rec, projectDir, id, localPath);
  if (!imported) {
    console.error(`error: cache entry for "${shaArg}" is incomplete or missing on disk`);