fix(skills): load helper dependencies outside repo installs#500
fix(skills): load helper dependencies outside repo installs#500miguel-heygen merged 2 commits intomainfrom
Conversation
|
Hi, package-loader automatically runs 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:
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. |
|
Hi, When packages are missing, the loader installs bare package names (no versions), so the helper scripts may run against newer incompatible 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:
We noticed a couple of other issues in this PR as well - happy to share if helpful. Found by Qodo code review |
jrusso1020
left a comment
There was a problem hiding this comment.
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_ENVguard prevents infinite re-exec loops.- The custom
package.jsonparser only runs as a fallback afterrequire.resolvefails — 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–22chains the destructure inline;contrast-report.mjsfirst assigns topackagesthen destructures. The latter reads better — apply the same pattern toanimation-map.mjs:const packages = await importPackagesOrBootstrap([\"@hyperframes/producer\"]); const { createFileServer, ... } = packages[\"@hyperframes/producer\"];
packages.sharp.defaultis correct (sharp is CJS, so the namespace's.defaultis 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
resolvePackageEntryagainst a temp dir tree (projectnode_modules, env-var path, missing case). - Unit test for
exportEntrycovering 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 oxfmtare the project's tools (perCLAUDE.md); author confirms they ran. ✅- This fixes a runtime crash that hits every published
hyperframesconsumer outside the monorepo. Afix: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 rannpm installthemselves, 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.
What
Fix the HyperFrames skill helper scripts that ship inside the
hyperframesCLI 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.mjsskills/hyperframes/scripts/contrast-report.mjsskills/hyperframes/scripts/package-loader.mjsWhy
This does affect the CLI distribution, but the affected surface is the CLI-bundled skill helper scripts, not the core
hyperframes render,hyperframes lint, orhyperframes validatecommands.The published
hyperframespackage includes the skill helpers underdist/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 thehyperframespackage:animation-map.mjsimports@hyperframes/producercontrast-report.mjsimports@hyperframes/producerandsharpThat works inside the monorepo because workspace dev dependencies are available. It fails outside the monorepo because the published CLI package does not install
@hyperframes/produceras a dependency.Repro
From a clean install of the published CLI package:
Observed output:
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:
node_modules/.binparents.@hyperframes/producerto the bundled HyperFrames CLI/package version, and pinsharpfor the contrast helper.HYPERFRAMES_SKILL_BOOTSTRAP_DEPS=1is set; interactive runs ask before installing.--ignore-scripts, re-exec the same Node script with that temporarynode_modulespath, and clean it up after the helper exits.The helper dependency stays lazy: normal CLI installs do not pull
@hyperframes/producerorsharpunless the script actually needs them, and missing dependencies are no longer installed implicitly without consent.Verification
Local checks
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 2animation-map.jsonfrom 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 11because 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.mjsbunx oxlint skills/hyperframes/scripts/package-loader.mjs skills/hyperframes/scripts/animation-map.mjs skills/hyperframes/scripts/contrast-report.mjsbunx tsx scripts/lint-skills.tsReview follow-up checks
hyperframesPackageSpec("@hyperframes/producer")resolves to@hyperframes/producer@0.4.30in this checkout.HYPERFRAMES_SKILL_BOOTSTRAP_DEPS=1is set, and print the exactnpm install --ignore-scripts --no-save ...command.npmbootstrap simulation and confirmed the loader invokesnpm install --ignore-scripts --no-save --prefix <tmp> fake-pkg@1.2.3, then re-execs and imports from the temporarynode_modulespath.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/producerorsharpto the normalhyperframesCLI install path.