fix(skills): make hyperframes package version resolution non-fatal and overridable

[miguel-heygen]

Problem

hyperframes-animation/scripts/animation-map.mjs (and hyperframes-creative/scripts/contrast-report.mjs) fail for users who installed the skills globally:

Could not determine the bundled HyperFrames version for @hyperframes/producer

This happens even when @hyperframes/producer is already installed and no bootstrap install is needed.

Root cause

hyperframesPackageSpec() in package-loader.mjs calls readBundledHyperframesVersion(), which only walks the ancestor chain of the script location and cwd for a hyperframes package.json. A global skill install (~/.claude/skills, ~/.agents/skills) has no such ancestor, so it returns null and the function throws.

The call sites evaluate the spec eagerly as an argument to importPackagesOrBootstrap([...], { npmPackages: [hyperframesPackageSpec(...)] }), so it throws up-front, before the loader ever checks whether the package is already resolvable. Result: it throws even when nothing needs installing.

Reproduce

Run the loader from a temp dir whose ancestor chain (and cwd) has no hyperframes package.json:

• Before: hyperframesPackageSpec("@hyperframes/producer") throws "Could not determine ...".
• In-repo control: pins the bundled version (e.g. @hyperframes/producer@0.7.22).

Fix

Version resolution is now non-fatal and overridable, with order:

1. HYPERFRAMES_SKILL_PKG_VERSION env override (honored first) so users/CI can pin the version explicitly.
2. The bundled/in-repo version (unchanged behavior when resolvable, same pinned spec as before).
3. A non-throwing @latest fallback that prints a one-line stderr warning naming the env override. @latest satisfies the existing pinned-spec guard, so a bootstrap install still proceeds and already-installed packages import fine.

Because the fallback never throws, the eager argument evaluation is now harmless, so no signature change to importPackagesOrBootstrap was needed. Applied to both identical package-loader.mjs copies (animation + creative) and documented the env var in the two call-site scripts' usage headers. Regenerated skills-manifest.json.

Verification

• Repro no longer throws: warns + falls back to @latest.
• Env override set -> uses that version.
• In-repo resolution still pins the correct bundled version.
• New focused test (skills/hyperframes-animation/scripts/package-loader.test.mjs): (a) env override wins, (b) resolvable in-repo pin, (c) unresolvable -> @latest fallback with no throw.
• All skills tests pass (node --test skills/**/*.test.mjs); oxfmt + oxlint clean; skills-manifest in sync; commit passed all pre-commit gates.

[james-russo-rames-d-jusso]

Reviewed at HEAD cbc16d06. Read-only pass on the two package-loader.mjs copies, the two usage-header docs, the new test, and the manifest. Cross-checked against packages/cli/scripts/build-copy.mjs (skills are bundled into dist/skills/... in the published @hyperframes/cli, which is the global-install shape this PR targets — so the fix propagates via the CLI publish pipeline automatically).

🟢 Strengths

• Correct diagnosis. PR body walks readBundledHyperframesVersion() → eager-arg-eval → throw-before-import-check cleanly. Verified the throw was in fact eager: both animation-map.mjs:29 and contrast-report.mjs:28 build npmPackages: [hyperframesPackageSpec(...)] as an argument literal, so any throw inside hyperframesPackageSpec short-circuits before importPackagesOrBootstrap gets to try resolvePackageEntry on the already-installed package. Removing the throw lets the resolver do its job.
• Symmetric fix across both loaders. Diff'd hyperframes-animation/scripts/package-loader.mjs and hyperframes-creative/scripts/package-loader.mjs at HEAD — byte-identical. Two copies is the pre-existing shape (not this PR's problem), and the PR keeps them in sync.
• Fallback observability is on the failure path. The @latest fallback emits a two-line stderr message naming the env var override BEFORE returning. This is exactly the "warn on band-aid path" shape — no silent swallow.
• @latest satisfies assertPinnedPackageSpecs. Verified hasVersionSpec("@hyperframes/producer@latest") returns true via the L182-188 check (second @ after the slash), so the pinned-spec guard downstream still passes without a signature change. Nice reuse of the existing invariant; PR body's claim holds.
• Test isolation is thoughtful. Test (c) uses spawnSync into a temp-dir subprocess so the ancestor-package.json lookup can't accidentally resolve the repo's own hyperframes — that's the right way to prove "no resolvable version" behavior without touching the parent process's cwd.

🟡 Questions / follow-ups

• Test file exists only for the animation copy. skills/hyperframes-animation/scripts/package-loader.test.mjs is new; skills/hyperframes-creative/scripts/ has no sibling. The loaders are byte-identical today, so functional coverage is fine, but nothing prevents the two copies from silently drifting in a future PR (the "two loader copies kept in sync manually" shape is fragile). Cheapest guard: a one-line assertion in the animation test that reads both files and assert.equals their contents — or, better long-term, symlink one to the other, or hoist to a shared skills/_shared/package-loader.mjs. Not blocking; worth surfacing before the shape ossifies.
• HYPERFRAMES_SKILL_PKG_VERSION isn't documented in SKILL.md for either skill. The env var lives in the usage headers of animation-map.mjs (L11-15) and contrast-report.mjs (L15-19), which is the right place for CLI users. But the skill-agent-visible skills/hyperframes-animation/SKILL.md and skills/hyperframes-creative/SKILL.md don't mention it. If an agent hits the warn and wants to know how to pin, SKILL.md is where they'll look. Low-effort follow-up (one sentence per SKILL.md).
• @latest version-skew risk in the global-install fallback path. When a globally-installed user with no ancestor package.json triggers the fallback, they get the newest published @hyperframes/producer — which may drift from the CLI's own bundled version over months. The warning + env-var escape hatch is a reasonable answer, but I'd note this loader ships INSIDE the published @hyperframes/cli package (per packages/cli/scripts/build-copy.mjs:89 copying skills/<pkg> → dist/skills/<pkg>). That means at bundle-time you do know the CLI's own version — the same package.json that tsup reads at packages/cli/tsup.config.ts:5. A future refinement would be to snapshot the CLI's version at build time (baked into a generated constant in the copied loader) and fall back to ^<major>.<minor>.x instead of @latest. Not blocking for this fix; wanted to flag while the shape is fresh.
• Warn fires per-call, not per-process. Only matters if a script calls hyperframesPackageSpec() multiple times — currently 1 call in each of the two callers, so no spam today. If future callers loop, consider a Set<packageName>-keyed once-per-process gate. Nit.
• Env var value passes through unvalidated. .trim() handles whitespace, but HYPERFRAMES_SKILL_PKG_VERSION="not-a-version" gets embedded verbatim into the spec string. Since the install path uses argv-array spawnSync with --ignore-scripts --no-save, it's not shell-interpolated (safe), and npm rejects invalid semver — so worst case is a confusing npm error rather than an early "invalid version" one. Nit; would be a one-liner to /^[@\w./~^*<>=|,\s-]+$/-guard and emit a clearer message.
• Test coverage gap: env-var precedence-over-resolvable-in-repo. Test (a) proves env wins in the no-ancestor case (via async-import from within the repo but with env set — verified by inspection, but the test doesn't isolate cwd, so it's implicitly relying on the module-level import's ancestor-scan being suppressed by the early env check). Test (b) proves in-repo wins when env is unset. Neither test isolates "env set AND in-repo package.json reachable → env wins." Given the code shape is trivial (early return on env before readBundledHyperframesVersion), risk is low; a one-line assertion in test (a) that the returned version equals "9.9.9" (which it already does) is sufficient — that IS the precedence proof, so this is actually already covered. Withdrawing on second read; leaving here as note for anyone else reading.

Ship criteria

CI green (all required checks passing at HEAD), no peer reviews yet, mergeable: true / mergeStateStatus: BLOCKED on the standard reviewer-required gate. Fix is contained, well-tested, and observable. Leaving as COMMENT — stamp routes through the OG Rames Jusso lane.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

R2 @ 71c398c5 — verifying against my R1 at cbc16d06.

Findings verification

• ✅ R1 🟡 (1) Test asymmetry — resolved. skills/hyperframes-creative/scripts/package-loader.test.mjs exists at HEAD, and both test files are byte-identical (git blob 7b36a3ceb3 for both). Sibling loaders also byte-identical (063bf4fe0c). All 3 test cases (env-override wins, in-repo resolves, @latest fallback via isolated-tmpdir spawnSync) present on both sides. Coverage propagates; drift risk is now caught by whichever side runs first in CI.

  • Follow-up nit (non-blocking, deferred is fine): two byte-identical test files is a smaller version of the same "manually kept in sync" shape as the loaders themselves. If a future PR hoists the loader to a shared module, the tests should follow. Not this PR's job.

• ✅ R1 🟡 (2) Env-var doc-visibility gap — resolved. Both skills/hyperframes-animation/SKILL.md (L75+) and skills/hyperframes-creative/SKILL.md (L52+) now document HYPERFRAMES_SKILL_PKG_VERSION. Sentences are symmetric (only the entrypoint filename differs: animation-map.mjs vs contrast-report.mjs). Doc is placed in the discoverable "Scripts" section next to the script listing, not buried at bottom. Explains purpose ("bootstrap the bundled HyperFrames package version"), when to set it ("running the skill outside the bundled CLI/skill install"), and the precedence intent ("pin that bootstrap version explicitly").

  • The two .mjs script usage headers (animation-map.mjs:11-15, contrast-report.mjs:15-19) now also mention the env var, with a symmetric block that names the fallback semantics ("falls back to @latest with a warning otherwise"). Nice — CLI-users and skill-agents both have a reachable pointer.

• ↩️ R1 🟡 nit (@latest version-skew) — acknowledged-and-deferred. Not addressed in R2; explicitly flagged as future refinement in R1. Fine.

Adjacent-file coherence

• Loader body change is symmetric across both copies (same 18-line delta at hyperframesPackageSpec — override check first, then readBundledHyperframesVersion, then stderr-warn @latest fallback). Both blobs at 063bf4fe0c — verified via gh api contents?ref=71c398c5.
• skills-manifest.json regenerated: hyperframes-animation hash 57458f4308708e21 → f789d7f95d9ad2fe, files 115 → 116; hyperframes-creative hash bb248c1bc5cc28b5 → 8573b34712e14ab1, files 67 → 68. Both bumps == +1 file (the new test), consistent with the diff.
• No divergence introduced between animation and creative sides — the "byte-identical siblings" invariant that made R1(1) worth flagging is preserved across all four affected files (loader, loader-test, SKILL.md addition, script-header addition).

Ship state

• HEAD: 71c398c5 (stable through review, verified via gh pr view --json headRefOid).
• Mergeable: MERGEABLE / BLOCKED (standard reviewer-required gate; not a merge conflict).
• Draft: no.
• CI: Test: skills ✅, Skills: manifest in sync ✅, all required checks green or SKIPPED (path-filter). CodeQL still IN_PROGRESS but that's advisory/NEUTRAL historically on this repo.
• Peer reviews since R1: none. No inline comments.

Verdict

All R1 substantive findings resolved cleanly. Fix is symmetric, tested on both sides, documented on both surfaces. Leaving as COMMENT — stamp lane routes through the OG Rames Jusso.

— Review by Rames D Jusso