Skip to content

fix(skills): load helper dependencies outside repo installs#500

Merged
miguel-heygen merged 2 commits intomainfrom
fix/animation-map-package-resolution
Apr 27, 2026
Merged

fix(skills): load helper dependencies outside repo installs#500
miguel-heygen merged 2 commits intomainfrom
fix/animation-map-package-resolution

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented Apr 26, 2026

What

Fix the HyperFrames skill helper scripts that ship inside the hyperframes CLI package so they can run from standalone composition projects, not only from the monorepo, while keeping helper-only dependency bootstrap explicit and pinned.

This changes:

  • skills/hyperframes/scripts/animation-map.mjs
  • skills/hyperframes/scripts/contrast-report.mjs
  • a shared skills/hyperframes/scripts/package-loader.mjs

Why

This does affect the CLI distribution, but the affected surface is the CLI-bundled skill helper scripts, not the core hyperframes render, hyperframes lint, or hyperframes validate commands.

The published hyperframes package includes the skill helpers under dist/skills/..., so agents can call them from an installed CLI or copied skill bundle. But those helpers used static top-level imports for packages that are not runtime dependencies of the hyperframes package:

  • animation-map.mjs imports @hyperframes/producer
  • contrast-report.mjs imports @hyperframes/producer and sharp

That works inside the monorepo because workspace dev dependencies are available. It fails outside the monorepo because the published CLI package does not install @hyperframes/producer as a dependency.

Repro

From a clean install of the published CLI package:

tmp=$(mktemp -d -t hf-cli-install-repro)
npm install --silent --no-audit --no-fund --prefix "$tmp" hyperframes@0.4.30

test -f "$tmp/node_modules/hyperframes/dist/skills/hyperframes/scripts/animation-map.mjs" \
  && echo "animation-map shipped in hyperframes package"

test ! -d "$tmp/node_modules/@hyperframes/producer" \
  && echo "@hyperframes/producer is not installed as a hyperframes dependency"

node "$tmp/node_modules/hyperframes/dist/skills/hyperframes/scripts/animation-map.mjs" \
  /Users/miguel07code/dev/yc-revenue-truth-video \
  --out "$tmp/anim-map" \
  --frames 1

Observed output:

animation-map shipped in hyperframes package
@hyperframes/producer is not installed as a hyperframes dependency
Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@hyperframes/producer' imported from .../node_modules/hyperframes/dist/skills/hyperframes/scripts/animation-map.mjs

So the failure is real for the installed CLI package shape: the helper script is shipped, but its dependency is not present.

How

Add a small shared package loader for skill scripts:

  • First try normal resolution from the current project, helper bundle, explicit bootstrap paths, and node_modules/.bin parents.
  • Require callers to pass pinned npm specs for helper-only dependency bootstrap.
  • Pin @hyperframes/producer to the bundled HyperFrames CLI/package version, and pin sharp for the contrast helper.
  • Refuse non-interactive bootstrap by default unless HYPERFRAMES_SKILL_BOOTSTRAP_DEPS=1 is set; interactive runs ask before installing.
  • If bootstrap is explicitly allowed, install into a temporary npm prefix with --ignore-scripts, re-exec the same Node script with that temporary node_modules path, and clean it up after the helper exits.

The helper dependency stays lazy: normal CLI installs do not pull @hyperframes/producer or sharp unless the script actually needs them, and missing dependencies are no longer installed implicitly without consent.

Verification

Local checks

  • Reproduced the published CLI-package failure above with hyperframes@0.4.30.
  • node skills/hyperframes/scripts/animation-map.mjs /Users/miguel07code/dev/yc-revenue-truth-video --out /tmp/hf-anim-map-pr-test --frames 2
    • Succeeded and wrote animation-map.json from the standalone composition project.
  • node skills/hyperframes/scripts/contrast-report.mjs /Users/miguel07code/dev/yc-revenue-truth-video --out /tmp/hf-contrast-pr-test --samples 1
    • Reached the normal contrast-audit result path and wrote report artifacts.
    • Exited 1 because that sample composition has expected WCAG AA failures, not because dependency loading failed.
  • node --check skills/hyperframes/scripts/package-loader.mjs && node --check skills/hyperframes/scripts/animation-map.mjs && node --check skills/hyperframes/scripts/contrast-report.mjs
  • bunx oxlint skills/hyperframes/scripts/package-loader.mjs skills/hyperframes/scripts/animation-map.mjs skills/hyperframes/scripts/contrast-report.mjs
  • bunx tsx scripts/lint-skills.ts

Review follow-up checks

  • Confirmed hyperframesPackageSpec("@hyperframes/producer") resolves to @hyperframes/producer@0.4.30 in this checkout.
  • Confirmed unpinned bootstrap specs are rejected before install.
  • Confirmed missing dependencies in non-interactive mode refuse to install unless HYPERFRAMES_SKILL_BOOTSTRAP_DEPS=1 is set, and print the exact npm install --ignore-scripts --no-save ... command.
  • Ran a fake-npm bootstrap simulation and confirmed the loader invokes npm install --ignore-scripts --no-save --prefix <tmp> fake-pkg@1.2.3, then re-execs and imports from the temporary node_modules path.

Browser verification

No browser verification is applicable for this PR because it only changes standalone Node helper scripts, not a user-facing Studio or player flow.

Notes

The dependency bootstrap remains a helper-script fallback only. It does not add @hyperframes/producer or sharp to the normal hyperframes CLI install path.

@miguel-heygen miguel-heygen marked this pull request as ready for review April 26, 2026 06:38
@qodo-ai-reviewer
Copy link
Copy Markdown

Hi, package-loader automatically runs npm install without --ignore-scripts, which will execute dependency lifecycle scripts (pre/postinstall) from remote packages as part of running these helper scripts.

Severity: action required | Category: security

How to fix: Add ignore-scripts and prompt

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

skills/hyperframes/scripts/package-loader.mjs bootstraps missing packages via npm install but allows lifecycle scripts to run, which can execute arbitrary code.

Issue Context

These scripts are shipped with the CLI under dist/skills/... (copied during build), so users running helper scripts may trigger implicit installs.

Fix Focus Areas

  • skills/hyperframes/scripts/package-loader.mjs[130-145]
    • Add --ignore-scripts to the npm arguments.
    • Consider also making bootstrapping opt-in (e.g., require an env var/flag) so running helpers never installs code implicitly unless explicitly requested.

We noticed a couple of other issues in this PR as well - happy to share if helpful.


Found by Qodo. Free code review for open-source maintainers.

@qodo-ai-reviewer
Copy link
Copy Markdown

Hi, When packages are missing, the loader installs bare package names (no versions), so the helper scripts may run against newer incompatible @hyperframes/producer/sharp APIs than the CLI/skill bundle was authored and tested with.

Severity: remediation recommended | Category: correctness

How to fix: Pin versions during bootstrap

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

Bootstrapping installs unversioned dependencies, which can introduce version skew between the shipped helper scripts and the installed runtime dependencies.

Issue Context

The helpers are bundled with a specific CLI release, but npm install <name> will resolve the latest version at execution time.

Fix Focus Areas

  • skills/hyperframes/scripts/package-loader.mjs[12-41]
  • skills/hyperframes/scripts/package-loader.mjs[130-145]
    • Accept version specs (e.g., @hyperframes/producer@<cliVersion>), or
    • Read/pin versions from a known source shipped with the CLI (e.g., CLI package.json), or
    • Allow callers (animation-map/contrast-report) to provide pinned options.npmPackages explicitly.

We noticed a couple of other issues in this PR as well - happy to share if helpful.


Found by Qodo code review

@miguel-heygen miguel-heygen merged commit 5397431 into main Apr 27, 2026
43 of 48 checks passed
@miguel-heygen miguel-heygen deleted the fix/animation-map-package-resolution branch April 27, 2026 20:12
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes a real, repro'd bug: the published hyperframes CLI ships skills/hyperframes/scripts/{animation-map,contrast-report}.mjs under dist/skills/... but their static import @hyperframes/producer / import sharp aren't runtime deps of the published package, so they crash with ERR_MODULE_NOT_FOUND outside the monorepo. The new package-loader.mjs resolves required packages from a list of candidate bases (cwd, script dir, HYPERFRAMES_SKILL_NODE_MODULES env, node_modules/.bin parents from PATH) and, on miss, runs npm install --no-save --prefix <tmp> and re-execs the script with the tmp node_modules as a hint. Direction is right — comments below are about hardening the loader.

Strengths

  • createRequire(join(base, "__hyperframes_skill_loader__.cjs")).resolve(name) is the right way to root resolution at an arbitrary directory.
  • BOOTSTRAP_ENV guard prevents infinite re-exec loops.
  • The custom package.json parser only runs as a fallback after require.resolve fails — keeps the simple path simple.
  • Resolution order (cwd → script dir → env hints → PATH-derived) is sensible.
  • Lazy install: regular CLI consumers don't pull producer/sharp unless a helper actually needs them.

Issues to address

Tmpdir leak on installResult.error (package-loader.mjs:135). The throw installResult.error happens before any rmSync. The non-zero-exit branch and the re-exec branch both clean up correctly — only the spawn-itself-failed path leaks. Wrap install + re-exec in a try { ... } finally { rmSync(installRoot, { recursive: true, force: true }); }.

Re-exec drops process.execArgv (package-loader.mjs:155). Args are [...process.argv.slice(1)], so flags like --inspect, --require=, --experimental-… that the user passed to the original Node process are silently stripped on re-exec. Suggest:

const args = [...process.execArgv, ...process.argv.slice(1)];

Bootstrap is fully silent. Cold installs of @hyperframes/producer + sharp can take 30+ seconds, but the spawn uses --silent and stdio: \"inherit\", so the user sees nothing — looks like the script is hanging. Print a one-line stderr notice before spawning ("Installing helper dependencies on first use…") and consider dropping --silent so npm's progress is visible.

No bootstrap caching. Every invocation that hits this path reinstalls into a fresh tmpdir. For helpers called from a loop (per-frame, batch contrast audits), users pay the install cost on every call. A small follow-up could cache to ~/.cache/hyperframes/skill-deps/<sha-of-(packages+versions)>. Not blocking; worth a TODO comment so it isn't forgotten.

exportEntry only handles a narrow package.json#exports shape — root entry, conditions import / default / node (and nested node). Returns null silently for arrays, subpath patterns, or other conditions. Fine for the two packages this targets today; risky as a general loader. A brief comment that this is intentionally minimal (require.resolve fallback only) would help future maintainers.

importPackagesOrBootstrap(packageNames, options) accepts options.npmPackages which becomes npm install arguments. Today both call sites are hardcoded literals ([\"@hyperframes/producer\"] / [\"@hyperframes/producer\", \"sharp\"]) so no injection. But the API shape invites it. JSDoc note: "Both packageNames and options.npmPackages are passed to npm install. Never derive these from untrusted input."

Nits

  • animation-map.mjs:13–22 chains the destructure inline; contrast-report.mjs first assigns to packages then destructures. The latter reads better — apply the same pattern to animation-map.mjs:
    const packages = await importPackagesOrBootstrap([\"@hyperframes/producer\"]);
    const { createFileServer, ... } = packages[\"@hyperframes/producer\"];
  • packages.sharp.default is correct (sharp is CJS, so the namespace's .default is the export), but a one-line comment explaining the asymmetry between the two destructures would prevent confusion.

Tests

No tests added. Manual verification via the CLI repro is fine for the happy path, but for a loader that does multi-base resolution + npm install --prefix + Node re-exec, that's thin. Suggested:

  • Unit test for resolvePackageEntry against a temp dir tree (project node_modules, env-var path, missing case).
  • Unit test for exportEntry covering string root, { \".\" : { import, default } }, and unsupported shapes.
  • Optional integration test gated behind HYPERFRAMES_TEST_BOOTSTRAP=1 (it costs network) that exercises the full bootstrap with a tiny known package.

The CLI repro from the PR body would already make a great integration test — it's a reproducible script.

Conventions / release

  • bunx oxlint + bunx oxfmt are the project's tools (per CLAUDE.md); author confirms they ran. ✅
  • This fixes a runtime crash that hits every published hyperframes consumer outside the monorepo. A fix: commit on its own should be enough for release-please if the repo is conventional-commits driven, but worth confirming a release will actually pick this up.

Security

  • Network is required for the bootstrap path; airgapped users will see registry timeouts. Worth a doc comment.
  • npm install --prefix <tmp> runs arbitrary install scripts (e.g. sharp's). Same trust model as if the user ran npm install themselves, so not a new vulnerability — but inheriting that trust on a tool's first run is worth a one-liner in the helper's docstring.

Verdict

Approve with comments. Sound fix for a real, user-facing bug. The improvements above (try/finally cleanup, execArgv pass-through, bootstrap progress message, JSDoc note on the install-arg API, a couple of unit tests) would meaningfully harden it but none are blocking. Caching can stay as a follow-up TODO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants