fix(core): publish runtime inline artifact#1787
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict — LGTM-with-nits ✅
The published-tarball mystery left a deductive trail almost too vulgar to record: the files manifest was screaming a negation no honest reader could overlook, and the import in dist/index.js carried on as if the negation had never been written. The remedy delivered here removes the lie, then arms a verifier that — had it been awake on the night of 0.7.15 — would have placed a hand upon the publisher's shoulder before the deed was done. The root cause is unambiguously addressed; the verifier is materially improved. I record one reachability concern (🟠) and three opportunities for further hardening (🟡) before the case is closed.
Issue #1786 — acceptance
dist/generated/runtime-inline.jswas excluded from the tarball at0.7.15+ whiledist/index.jscontinued to import it. Confirmed at HEAD: the!dist/generated/runtime-inline.jsnegation has been removed frompackages/core/package.json'sfilesarray. The sibling exclusion!dist/hyperframe.runtime.mjslegitimately remains (that artifact is mirrored via the IIFE on the producer path and is not imported bydist/index.js).- The author's repro —
pnpm pack+ tempnpm install+import('@hyperframes/core')→import ok function 230004— exactly mirrors the failure shape in the issue. Acceptance: satisfied.
Snapshot
- HEAD
82c35b65c0cc6aeabb3a50ff27f19a5ba7464ab3(single commit, author Miguel). - Status: all required checks green at time of review (
Build,Test,Test: skills,CLI smoke,Test: runtime contract,Studio: load smoke,Smoke: global installall SUCCESS). Regression shards still IN_PROGRESS but historically green on a packaging-only change. - Prior reviewers: none at read-time.
Part 1 — files exclusion fix ✅
packages/core/package.jsonat HEAD now lists!dist/hyperframe.runtime.mjsas the only negation; the offending!dist/generated/runtime-inline.jsis gone. Confirmed clean.- Sibling-package audit — grepped every
packages/*/package.jsonfilesarray at HEAD for!negations (cli,engine,gcp-cloud-run,aws-lambda,lint,parsers,player,producer,sdk,sdk-playground,shader-transitions,studio,studio-server,core). The ONLY remaining negation in the entire monorepo is!dist/hyperframe.runtime.mjsincore— and that one is legitimate (it is the IIFE-mirrored producer artifact, intentionally re-shipped viadist/hyperframe.runtime.iife.jsrather than the.mjs; nothing indist/index.jsimports it). No further stale-exclusion follow-ups warranted.
Part 2 — verifier extension
What it now does (read at HEAD):
listRelativeImportSpecifiers(source)
.flatMap(({ index, specifier }) => {
if (!hasExplicitRuntimeExtension(specifier)) return `… imports ${specifier}`; // existing — Node-incompatible extensionless
const target = resolvePackedRelativeImport(file, specifier);
if (!packedFiles.has(target)) return `… imports missing ${specifier}`; // NEW — bytes-present check
return [];
});The two patterns inside listRelativeImportSpecifiers:
/^\s*import\s+["'](\.\.?\/[^"']+)["']/gm // side-effect import "./x.js"
/^\s*(?:import|export)\s+[^;]*?\s+from\s+["'](\.\.?\/[^"']+)["']/gm // import/export … from "./x.js"What's verified clean:
- ✅
import "./missing.js"(side-effect) — covered by pattern 1, asserted by the new test. - ✅
import X from "./missing.js"— covered by pattern 2. - ✅
export * from "./missing.js"andexport { x } from "./missing.js"— covered by pattern 2 (the(?:import|export)alternation). - ✅ Nested-directory targets (
./generated/runtime-inline.js) —resolvePackedRelativeImportusesposix.normalize(posix.join(posix.dirname(fromFile), …)), which gives the correctdist/generated/runtime-inline.jskey intopackedFiles(set is built fromtar -tfwithpackage/stripped, same path shape). - ✅ Query-string / hash strip —
stripSpecifierQueryis reused, so./x.js?workerresolves to./x.js. - ✅ Bare specifiers (
react,@hyperframes/core) —listRelativeImportSpecifiersonly matches\.\.?\/prefixes, so npm dependencies are correctly out of scope. - ✅ The
if (process.argv[1] === fileURLToPath(import.meta.url))guard at the bottom is the canonical ESM equivalent ofif __name__ == "__main__"— needed because the file is now also imported by the test. Correct. - ✅ The
dist/hyperframe.runtime.mjslegitimate exclusion is unaffected becauseverifyPackedJavaScriptImportsfilters withfile.endsWith(".js")—.mjsfiles are out of scope, but no.jsfile imports./hyperframe.runtime.mjs, so no false positive.
🟠 should-fix — verifier-runs-on-PR reachability gap. The verifier guards bun run verify:packed-manifests only in .github/workflows/publish.yml (line 98), which fires on workflow_dispatch / tag-push to publish. There is no invocation in .github/workflows/ci.yml. That means a future PR that re-introduces a files negation for a published artifact (or, e.g., changes tsconfig outDir or a build step that drops a generated .js) ships green through code review and only trips at the Publish step — exactly the failure mode #1786 described. The verifier itself is good; the publish-only invocation is the same shape of "decorative gate" as #418/#1555 in my notebook — read-and-decide guard placed at the late edge of the dispatch chain. Suggested follow-up (does not block this PR): add a Verify packed manifests job in ci.yml that runs bun run verify:packed-manifests on PRs touching packages/**/package.json, scripts/verify-packed-manifests.mjs, or build-output scripts. Even a paths: filter would catch the regression class for ~zero cost on unaffected PRs.
🟡 nit — unhandled import shapes. The regex pair does not match these forms; a future stale build artifact using them would slip past the new check:
import("./x.js")— dynamic imports. (Used inside the codebase at multiple call sites; not implausible to appear in bundler output.)require("./x.js")— CommonJS. (coreis"type": "module", but the verifier walks every workspace, and at least the producer'sdist/ships some CJS interop in places.)- Multi-line import statements where the
from "./x.js"is on a wrapped line ([^;]*?is non-greedy but still constrained to a single line by^+mflag — bundler output is usually single-line, but tsc with--module preservecan split).
None of these were the cause of #1786, and Esbuild/tsc output for core/dist today is one-line side-effect / from imports — so this is a forward-looking robustness note, not a regression risk. Worth a one-line follow-up: extend the patterns or replace with a proper AST walk via acorn (already a transitive dep) when ambition strikes.
🟡 nit — test surface is thin. verify-packed-manifests.test.mjs covers exactly one case: a side-effect import "./missing.js" in index.js against a tarball missing that target. Worth at least asserting the happy path (target present → empty result), export * from (because that's the form dist/index.js re-exports use), and a nested-directory case (because that's literally the bug we're guarding against — ./generated/runtime-inline.js). The deductive value of a regression test that mirrors the original crime would be considerable.
🟡 nit — target is a non-empty array result vs. flat-map empty. Cosmetic, but the flatMap returning a string from one branch and [] from the other relies on flatMap's auto-spread of strings being treated as array elements (which works because the outer .flatMap flattens one level — a string return is wrapped). Reads as intentional; mention only because a future contributor refactoring to .map(…).filter(Boolean) could subtly change semantics if specifier is ever falsy. Self-correcting in practice. 💭
Cross-package audit ✅
Already covered above — only packages/core had any files negation, and the one remaining (!dist/hyperframe.runtime.mjs) is correct. No follow-up issue worth filing.
Summary of severities
| Severity | Item |
|---|---|
| ✅ | Root-cause fix (Part 1) is correct and minimal. |
| ✅ | Verifier extension (Part 2) correctly detects the #1786 regression class. |
| 🟠 | Verifier is not invoked in ci.yml — only at publish time. Suggest wiring bun run verify:packed-manifests into CI on PRs touching packages/**/package.json or the script itself. |
| 🟡 | Verifier regex misses dynamic import(), require(), and multi-line from — not the cause of #1786, but worth a forward-looking pass. |
| 🟡 | New test covers a single failure case — add happy path, export * from, and a nested-directory case. |
| 💭 | Cosmetic: flatMap returning a bare string relies on auto-spread; works correctly, just brittle to refactor. |
The fix is sound. Approve at author's discretion; CI-integration follow-up is the highest-leverage hardening and would convert this verifier from publish-time safety net into a true PR gate.
Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict — LGTM-with-followups (COMMENT)
Layering on Via's review above. I converged on the same three core findings (CI invocation gap, regex coverage of dynamic/CJS imports, thin test surface) — recording the convergence and adding a few orthogonal notes that didn't surface in his pass.
Convergence with Via ✅
- Root-cause fix is correct — confirmed
!dist/generated/runtime-inline.jswas added incdf9c817(parsers extraction #1755) alongside the IIFE/.mjsexclusions, and thegenerated/runtime-inline.{ts,js}artifact is referenced by static ESM imports atpackages/core/src/index.ts:192andpackages/core/src/compiler/htmlBundler.ts:18. The exclusion was incorrect from the moment it was added; removing it is the minimal fix. !dist/hyperframe.runtime.mjslegitimately remains — that artifact is fetched via CDN (hyperframe.runtime.iife.jsat runtime), not statically imported fromdist/index.js. Same conclusion as Via.- 🟠 CI invocation gap — confirmed:
verify:packed-manifestsonly runs in.github/workflows/publish.yml:98(tag-push / merge / dispatch). Not wired intoci.yml. The unit test (scripts/verify-packed-manifests.test.mjs) IS pulled intotest:scriptsand runs on PR viaci.yml:229, so the logic is regression-tested per-PR, but the integration (actually packing + verifying real workspace tarballs) is publish-only. The next time afilesnegation slips in via PR, the unit test still passes — only the publish job catches it, after merge. - 🟡 Regex misses dynamic
import()/require()/ multi-linefrom— same conclusion. Not the cause of #1786 (static side-effect import), but a known forward-looking gap; AST viaacorn(already a transitive dep) is the obvious upgrade if this trips again. - 🟡 Test surface is thin — agreed; negative-case (target present → empty result) is the most load-bearing addition. A test asserting the exact #1786 shape (
import { x } from "./generated/runtime-inline.js"resolving via nested-dirname + present-in-packedFiles) would be the canonical bug-pin.
Adding beyond Via 🆕
-
🟡
.cjs/.mjspacked files are entirely unchecked.listPackedJavaScriptImportIssuesfiltersfile.endsWith(".js")atscripts/verify-packed-manifests.mjs:187. Any packed.cjsor.mjsfile is not walked at all — its imports are invisible to the verifier.RUNTIME_IMPORT_EXTENSIONSat line 11 includes.mjs/.cjsas valid extensions on import specifiers, so the asymmetry is: an.mjsfile importing./x.cjsis unchecked, but a.jsfile importing./x.cjsis checked.@hyperframes/coreships"type": "module"ESM-only today, so this is a forward-looking gap. If any package starts emitting CJS or.mjsdist artifacts later, the verifier won't see them. Cheap one-line widen:.filter((file) => /\.(?:js|mjs|cjs)$/.test(file)). -
🟢 Consumer blast radius is contained.
@hyperframes/core@0.7.18(the broken version) is currently pinned in two places I can see:hyperframes-internal/packages/producer-internalandhyperframes-internal/packages/demo-next— both pinned to0.6.73, well before the 0.7.x parsers-extraction window. No external consumer inrepos/is on a 0.7.x version. Recovery via the nextset-versionbump (whatever release-please picks up — likely a patch) should be sufficient; no coordinated multi-repo upgrade needed. -
🟡 Recovery posture — this PR does not bump the version.
packages/core/package.json:3still reads"0.7.18". The npm tarball at0.7.18is broken and will remain broken; recovery happens only when the next publish (viascripts/set-version→ tag → publish.yml) ships a new version with this fix included. If anyone is onnpm install @hyperframes/core@0.7.18(or a^0.7.17range resolving to it) they stay broken until they upgrade. Worth a one-line callout in the merge message or a0.7.18deprecation on npm (npm deprecate @hyperframes/core@0.7.18 "use ^0.7.19") once the patch is published, so future installs of0.7.18get a clear pointer. -
🟢 Process-guard
if (process.argv[1] === fileURLToPath(import.meta.url)) main()is correct. Necessary now that the script also exportslistPackedJavaScriptImportIssuesfor the test to import — without it the test would re-pack every workspace as a side effect.
Followup priority (if author or @miguel-heygen wants to take any of this on)
- 🟠 Wire
verify:packed-manifestsintoci.ymlwith apaths:filter onpackages/**/package.json,scripts/verify-packed-manifests.*, and build scripts. Converts this from a post-merge net to a pre-merge gate. Highest leverage. - 🟡 Beef up the test: happy path (target present), nested-directory case mirroring
./generated/runtime-inline.jsexactly,export * from "./x.js"shape, and a verifier-rejects-extensionless case so both branches stay covered. - 🟡 Widen file-extension filter to
.js|.mjs|.cjsand consider anacornAST walk to catchrequire()/ dynamicimport()while you're in there. - 💭 Optional:
npm deprecate @hyperframes/core@0.7.18after the fix publishes.
None blocking this PR — it's a clean root-cause fix with a real (if narrowly-tested) regression guard.
Reviewed at SHA 82c35b65c0cc6aeabb3a50ff27f19a5ba7464ab3.
— Rames D Jusso
82c35b6 to
94d62e7
Compare
94d62e7 to
7064359
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
✅ Approved at 70643595.
Root-cause fix + R2 hardening both verified at source (additive to RDJ/Via):
- The stale
!dist/generated/runtime-inline.jsexclusion is removed frompackages/core/package.json— that file is generated + statically imported bydist/index.js, so excluding it from the tarball was the #1786 install crash. - The new PR-time gate is the key win:
bun run verify:packed-manifestsruns in the Build job afterbun run build(ci.yml:82-87), and it's GREEN at this SHA — i.e. the packed@hyperframes/coretarball now includes the artifact + all relative imports resolve. Direct proof the fix works, not just a unit test. - Verifier hardened to scan
.js/.mjs/.cjs+ catch static / export-from / dynamicimport()/require()/ side-effect / extensionless imports; tests 1→5 incl the #1786 missing-target regression guard; flatMap-string nit fixed.
CI: zero failures across 39 completed checks; CLI smoke (required) + verify:packed-manifests green. Still running: the long rendering regression-shards + windows + Smoke: global install — none affected by a packaging change (the install smoke is belt-and-suspenders on top of the green verify:packed-manifests).
Post-merge note: npm @hyperframes/core@0.7.18 stays broken until a set-version + republish — the merge fixes source, not the published package.
— Rames
Summary
Top-level
@hyperframes/coreimports no longer crash after install from npm. The package was still generatingdist/generated/runtime-inline.js, butpackages/core/package.jsonexplicitly excluded that file from the tarball whiledist/index.jsimported it.This removes the stale exclusion and strengthens the publish-safety verifier so future packed packages fail CI if any packed JavaScript file imports a relative target that is missing from the tarball.
Fixes #1786.
Verification
node --test scripts/verify-packed-manifests.test.mjsbun run test:scriptsbun run buildnode scripts/verify-packed-manifests.mjspnpm --dir packages/core pack ..., then tempnpm install+import('@hyperframes/core')→import ok function 230004bun run format:checkgit diff --check