fix(producer,cli): surface every tried manifest path in the missing-manifest error (#3370) - #3387
Conversation
…anifest error (heygen-com#3370) When the hyperframe runtime manifest could not be located, the loader reported a single fallback path that was never searched for (`/usr/local/lib/core/dist/hyperframe.manifest.json`), so users looked in the wrong directory. * Hoist the candidate list to a single `MANIFEST_CANDIDATES` owner shared by the resolver and the error reporter. The missing-manifest message now lists every path actually checked plus the cwd. * Drop the byte-identical duplicate of `SIBLING_MANIFEST_PATH` inside what was mislabelled `CWD_RELATIVE_MANIFEST_PATHS`. * Replace the source-text regex test with a behaviour test that points `PRODUCER_HYPERFRAME_MANIFEST_PATH` at a missing file and asserts the thrown error names it. * Suppress the "Try --docker" hint inside the render container (Dockerfile.render sets `ENV CONTAINER=true`), so users already in the container don't get told to run the same fallback they're in.
miguel-heygen
left a comment
There was a problem hiding this comment.
The implementation is clean at 0e732786. MANIFEST_CANDIDATES preserves the old resolution order while removing the duplicate and gives the resolver/error reporter one source of truth (hyperframeRuntimeLoader.ts:9-56); the override path still short-circuits consistently. The CLI fix also reads the exact CONTAINER=true contract set by packages/cli/src/docker/Dockerfile.render:28, and errorBox suppresses the empty suggestion as intended.
Two non-blocking test notes:
- important:
hyperframeRuntimeLoader.test.ts:44-46now unconditionally asserts that some candidate exists. The advertised standalonebunx vitest run ...command fails 1/7 in a clean source worktree with no prebuiltpackages/core/dist/hyperframe.manifest.json; it passes in CI because the producer lane builds core first. Either mirror the next test's “skip when core is not built” posture or state/build that prerequisite in the targeted test command, so the test is independently runnable rather than environment-dependent. - important: the new
process.env.CONTAINER === "true"branch atrender.ts:904-912has no direct regression test. The loader half is well-pinned, but a typo/removal of the container suppression would leave every added test green. The existingrenderLocalharness can pin both directions: container omitsTry --docker, host retains it.
CI evidence at review time: Build, Typecheck, Lint, Producer unit/integration, CLI smoke, runtime contract, and preview parity are green; Windows/regression/CodeQL shards are still running. No required check is failing.
Verdict: APPROVE
Reasoning: the production change is behavior-preserving outside the two intended error-message branches and correctly fixes both misleading paths; remaining concerns are test-harness completeness, not a runtime blocker.
— Magi
santhiprakash
left a comment
There was a problem hiding this comment.
Thanks Magi — appreciate the careful read of the loader, the override short-circuit, and the CONTAINER contract.
Both notes are valid and worth addressing. To keep this approved PR stable I'm holding off on pushing a follow-up commit here, and I'll handle them in a follow-up PR:
- Test 1 (env-dependent assertion): the right shape is the "skip when core is not built" guard you described — i.e. move the
existsSync(MONOREPO_PATH)skip above theexistsSync(resolved)assertion rather than parallel to it. That makesbunx vitest run src/services/hyperframeRuntimeLoader.test.tspass in a clean checkout, and the test still proves the behaviour once core has been built. - Test 2 (CONTAINER suppression coverage): a
renderLocalcase that asserts the captured error omits"Try --docker"whenprocess.env.CONTAINER === "true"and retains it otherwise. ThesavedEnv/setEnvhelpers in the existing harness already support the env flip.
Also added Closes #3370 to the PR description so the issue auto-closes on merge.
|
@miguel-heygen thanks again for the careful review — this has been approved and fully green for about a day now. Is there anything else you'd like from our side before it merges? The two test improvements you suggested are queued for a follow-up PR, but I'm happy to fold them in here first if that's easier for you. |
Closes #3370
What
When
hyperframeRuntimeLoadercould not locatehyperframe.manifest.json, the loader reported a single fallback path that was never searched for (/usr/local/lib/core/dist/hyperframe.manifest.json). Inside a Docker render the user is then told to look at the wrong directory; the file that was actually missing (/usr/local/lib/node_modules/hyperframes/dist/hyperframe.manifest.json) was nowhere in the message.Why
resolveHyperframeManifestPath()built a 5-elementcandidatesarray, walked it withexistsSync, and on total miss returned the last candidate. The error then quoted that candidate verbatim. The reporter even shows the exact reproducing command from a published image.A second issue rode the same failure path:
packages/cli/src/commands/render.ts:902keeps attaching the hint"Try --docker for containerized rendering"to users who are already inside the container. The container setsENV CONTAINER=trueand nothing reads it.A third small thing came along:
CWD_RELATIVE_MANIFEST_PATHS[0]was a byte-identical duplicate ofSIBLING_MANIFEST_PATH— same path, two names.How
MANIFEST_CANDIDATESowner inhyperframeRuntimeLoader.tsand share it between the resolver and the error reporter. De-duplicate while doing it.triedManifestPaths()as a tiny export so callers (and tests) can see what was actually searched.const candidates = [...]with a behaviour test that pointsPRODUCER_HYPERFRAME_MANIFEST_PATHat a missing file and asserts the thrown error names it. Also exercise the no-override branch to confirm the sibling path is the first entry.render.ts, checkprocess.env.CONTAINER === "true"before attaching the--dockerhint. The chrome-launch and macos-old-chrome remediation branches already short-circuit before the hint, so an empty string is a safe value when the user is in the container.Test plan
bunx vitest run src/services/hyperframeRuntimeLoader.test.ts— 7/7 pass (hyperframeRuntimeLoader error path (#3370)describe covers the missing-manifest message and the tried-paths export).bunx tsc --noEmitinpackages/producerandpackages/cli— clean.bunx oxfmt --checkandbunx oxlinton the touched files — clean.bunx fallow audit --base origin/main— no new findings on the touched files.node scripts/run-test-lane.mjs unit— same 7 pre-existing failures asorigin/mainbefore the change (htmlCompiler.parity, audioPadTrim.integration); no regressions introduced.Files touched:
packages/producer/src/services/hyperframeRuntimeLoader.tspackages/producer/src/services/hyperframeRuntimeLoader.test.tspackages/cli/src/commands/render.ts