Repository navigation
fix(cli): reverse start makes its --dir project, and init takes the video already in it - #7089
Conversation
…ideo already in it Bug bash: production has no reverse/start route yet, so every run came back unrecorded, and reverse start then made no folder and no hyperframes.json. The agent downloaded the video into the folder it named, and init refused it as not empty, so agents moved files around. reverse start now makes --dir and stamps it with the skill when no session is recorded, and init accepts a folder whose only other files are its --video or --audio.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at fb236d0caddc12ac287f549406b54788a510a19d.
The unrecorded-session branch reuses seedProjectAuthoringSkill and preserves existing owners, custom metadata, and resumable sessions. Independent tests exercised these contracts.
Important — accepted in-place source files can be overwritten by the scaffold (packages/cli/src/commands/init.ts:284-287; both callers at 875 and 1057). The new whitelist admits every explicitly selected file without checking whether the scaffold writes that name. A valid H.264 MP4 container named p/index.html, p/meta.json, or p/AGENTS.md, passed as --video with --keep-original, now exits 0 and replaces the only original with template HTML, generated metadata, or shared instructions. All three are refused without mutation by the base implementation. Fix admission before any writes: reserve generated files, shared context files, and the selected template's output paths, or safely give imported media a separate destination and update the media reference. Add byte-preservation/refusal tests for these three cases. Skipping copyFileSync onto itself alone cannot protect against the later scaffold writes.
Important — the newly admitted in-folder .mp4 path cannot auto-transcode (packages/cli/src/commands/init.ts:875, interacting with handleVideoFile at 505-515). With a valid HEVC p/original.mp4 and no --keep-original, init p --video p/original.mp4 --skip-transcribe --skip-skills --non-interactive uses that same path for ffmpeg's input and output. It prints Transcode failed — copying original file, exits 0, and leaves the referenced media HEVC. Known-good controls using the same source outside p, or a .mov name inside p, both become H.264. Choose a distinct output name when input and transcode destination identify the same file (for example original.h264.mp4), keep the original intact, and point the scaffold at the converted file. Test an actual non-web codec in an in-folder .mp4 without --keep-original.
Nit (packages/cli/src/commands/reverse/start.ts:70): existence alone reports saved_to even when hyperframes.json is corrupt or is a directory and the seed wrote nothing. Return a path only after confirming a valid stamped config, or make the seeding helper report success. Both cases are independently witnessed; no metadata was overwritten.
Evidence: bun install --frozen-lockfile and bun run build passed. The focused init/reverse run passed 73 committed tests plus six independent persistence controls (79 total). node review-artifacts/7089/independent-init-controls.mjs ran 13 real CLI/ffmpeg scenarios, which I inspected and independently reran with the same outcomes. independent-init-results.json records each exit code, input codec, and SHA-256 before/after; independent-base-init-results.json repeats them with the base init implementation on head dependencies and includes a successful external-media control.
Limits: no live reverse backend or browser playback was exercised. Codec results come from ffprobe. Interactive paths share the guard/helper and their committed tests pass; no live terminal interaction was performed. Relative/absolute H.264 and WAV sources retain bytes; unrelated hidden files and an external same-name source are refused. An explicitly selected in-project symlink works; addressing a symlinked project through an alias while naming the source canonically still refuses (lexical identity limitation).
Need: yes · Size: ok · Reuse: projectConfig seed/session helpers and existing scaffold pipeline · Simpler: already minimal · Structure: fine
Risk: medium — broadened directory admission can now destroy an explicitly selected original.
Verdict: REQUEST CHANGES
Reasoning: The primary workflow works for ordinary in-folder media, but the new admission policy also reaches destructive filename collisions and a reproducible same-file transcode failure for in-folder .mp4 inputs requiring conversion. Preserve the source across every scaffold write and use a distinct transcode output before merging.
— hf-review
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed #7089 at fb236d0caddc12ac287f549406b54788a510a19d. The ordinary recorded:false --dir and in-folder H.264 --keep-original paths are implemented and the new tests exercise the commands. Changes requested. Rames's exact-head review already includes independent media reproductions of two findings, which I also traced at source rather than duplicating inline: permitting a supplied video named index.html/meta.json/AGENTS.md lets scaffolding overwrite that source, and an incompatible MP4 already at the desired output path is passed as both FFmpeg input and output, then init succeeds with the unconverted original. My distinct inline finding is a false saved_to receipt: the new unrecorded path treats existence of a malformed or unwritable hyperframes.json as successful skill seeding even when seedProjectAuthoringSkill left it untouched. The guard still rejects unrelated hidden files in non-interactive mode; a matching relative path is normalized lexically, not by inode, so a symlink alias may be conservatively refused. Exact-head pull_request CLI test shard 1 executed 4,017 passing tests including the new source-in-folder test; build/lint/typecheck checks ran. Production reverse/start route is a separate EF prerequisite, not verified live by this source review. No local test, project mutation, merge, or release action.
— Jerrai
| return undefined; | ||
| } | ||
| seedProjectAuthoringSkill(dir, SKILL); | ||
| return existsSync(projectConfigPath(dir)) ? projectConfigPath(dir) : undefined; |
There was a problem hiding this comment.
P2 — saved_to can claim a project config was saved when seeding did nothing. seedProjectAuthoringSkill is best-effort and returns void: for an existing malformed/unreadable/unwritable hyperframes.json (or a directory at that name) it leaves the entry alone. This new branch checks only existsSync, then returns the path as reverse.saved_to even though no valid authoringSkill: reverse-any-video was persisted. The recorded-session branch uses recordProjectReverseSession's actual success/undefined outcome; the unrecorded branch needs the same honest receipt, without clobbering the bad existing file. Add an existing corrupt/read-only config regression and assert saved_to:null when seeding fails.
…s a stamped config - Only an audio or video file in the folder counts as the source; a file named index.html, meta.json or AGENTS.md is refused, so the scaffold never writes over it. - An in-folder .mp4 that needs transcoding goes to <name>.h264.mp4 beside it, instead of being both ffmpeg's input and output. - reverse start names saved_to only when hyperframes.json now holds the skill; a broken config is left as it was and saved_to is null.
|
Thanks @jrusso1020 and Jerrai, all three are fixed in 3ca3629.
New tests ( |
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 3ca36295cd650091a56df7d97e85963f020c2c11, including the delta from fb236d0caddc12ac287f549406b54788a510a19d and the three earlier findings from Rames and Jerrai. All three are resolved; no new Important finding.
- Source preservation:
init.ts:280-294rejects scaffold filenames before writes. Actual CLI runs refused valid video containers namedindex.html,meta.json,AGENTS.md,CLAUDE.md, orpackage.json; their hashes stayed unchanged. An unrelated.notesfile and an existing generated output (regular file or symlink) are also refused unchanged. - In-folder conversion:
init.ts:510-518selects a separate H.264 output when the normal output equals the source. Real HEVC files became H.264, the original hashes remained unchanged, and generated HTML referenced the H.264 file. Absolute, relative, normalized relative, selected-source symlink, hidden-media,.mov, uppercase.MP4, and external-source controls were exercised. - Honest receipt:
reverse/start.ts:70now reads the persisted skill through the existing config reader. Independent command tests cover corrupt/scalar/array/null configs, a directory at the config path, an unwritable folder, an existing different owner and resumable session, BOM/indentation/custom metadata, and successful folder creation.
Evidence: bun install --frozen-lockfile and bun run build passed. The focused init/reverse/projectConfig command passed 137 tests: 128 committed tests plus nine independent receipt tests. node review-artifacts/7089/r2/adversarial-cli.mjs exercised 21 actual built-CLI scenarios: ten successful scaffolds, ten unchanged refusals, and one documented existing project-symlink failure. Against the previous init implementation on head dependencies, the same controls reproduced overwritten sources and unconverted HEVC; the old receipt implementation failed six of the nine independent controls. I inspected the independent scripts and reran all 21 built-CLI scenarios with matching outcomes. Temporary source substitutions were restored byte-for-byte; the worktree is clean.
Limits: local execution was Linux-only. A symlink used as the project directory still fails in the existing copyDirSync path (utils/copyDir.ts:12) when both project/source use the alias; the previously reviewed head reproduces the same copy error (pre-existing between review rounds), and source bytes remain intact. A canonical source addressed through a different project alias is conservatively refused. No live reverse backend, browser playback, or live interactive terminal was exercised; interactive callers share the audited guard/media helper and their tests passed.
Need: yes · Size: ok · Reuse: existing projectConfig reader/seed/session helpers and scaffold pipeline · Simpler: already minimal · Structure: fine
Risk: low — narrow guards and output-name correction, with byte-preservation and failure controls verified.
Verdict: APPROVE
Reasoning: The previous source-overwrite, same-file transcode, and false saved receipt findings are resolved at this head. Independent real-media and persistence controls found no new blocking regression.
— hf-review
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed #7089 at 3ca36295cd650091a56df7d97e85963f020c2c11. The three ordinary predecessors are fixed: source files named index.html/meta.json/AGENTS.md no longer enter the non-interactive folder allowlist; lowercase in-folder HEVC .mp4 converts beside the unchanged source as .h264.mp4; and reverse.saved_to is returned only when a valid reread of hyperframes.json contains reverse-any-video. Rames approved those cases on their source/media tests. I am requesting changes on two additional exact-head defects, detailed inline: uppercase/mixed-case .MP4 still aliases input/output on case-insensitive filesystems because resolve() equality is case-sensitive, and the new codecOf test omits the required -- before ffprobe's input. The latter is not hypothetical: exact-head hosted run 41675276046 failed Producer: unit tests on ffprobeArgvContract.test.ts (1 failure, 1,153 passed); aggregate Test failed too. CLI shard 1 passed 4,017 tests, but that is not all required CI. The source-path alias is a static cross-platform counterexample, not a separately executed macOS/Windows repro. The backend reverse/start route remains independent. No local tests, project writes, merge or release by me.
— Jerrai
| const mp4Name = localVideoName.replace(/\.[^.]+$/, ".mp4"); | ||
| const sameName = localVideoName.replace(/\.[^.]+$/, ".mp4"); | ||
| const mp4Name = | ||
| resolve(destDir, sameName) === resolve(videoPath) |
There was a problem hiding this comment.
P2 — case-variant MP4 still aliases FFmpeg input/output on default macOS/Windows filesystems. An existing in-folder HEVC p/original.MP4 is admitted by the case-insensitive media extension allowlist. sameName becomes original.mp4, but resolve(p/original.mp4) !== resolve(p/original.MP4) lexically, so this branch does not choose .h264.mp4. On a case-insensitive filesystem those two paths refer to the same file; transcodeToMp4 passes them as FFmpeg input and -y output, fails in-place conversion, and the fallback keeps the original incompatible codec while init succeeds. Please choose a distinct output independent of case-sensitive path equality (or compare actual file identity) and add a case-insensitive/uppercase-extension regression; preserve the good lowercase .mp4 behavior.
| const codecOf = (file: string) => | ||
| execFileSync("ffprobe", [ | ||
| ...["-v", "error", "-select_streams", "v:0", "-show_entries", "stream=codec_name"], | ||
| ...["-of", "csv=p=0", file], |
There was a problem hiding this comment.
Required CI is red on this new test. The exact-head pull_request run 41675276046 fails Producer: unit tests in src/utils/ffprobeArgvContract.test.ts: it statically scans this codecOf call and requires "--" immediately before the input file after -of csv=p=0. The aggregate Test job consequently fails, despite the CLI shard passing. Add the separator before file and re-run hosted checks on the successor head. This is a real diff-introduced test-contract failure, not a skipped or unrelated job.
…lder # Conflicts: # docs/packages/cli.mdx
…tive disks too original.MP4 and original.mp4 are one file on macOS and Windows, so the same-file check now ignores case. The test's ffprobe call gets its --.
|
Thanks @jrusso1020 for the approval, and Jerrai for the two catches, both fixed in e6ccab9 (on top of a merge of main, 43aa627).
The merge with main kept both sides of the |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
The earlier in-place .MP4 transcode alias and ffprobe -- failures are fixed at e6ccab9, with executed hosted CLI, build/lint/typecheck/smoke checks. One related case-only filename alias remains in the newly added existing-folder/source guard and copy helper on case-insensitive volumes; see the concrete inline sequence. The prior tests do not use a case-only input spelling. No local media test or merge by this reviewer. — Jerrai
| ); | ||
| return ( | ||
| existsSync(dir) && | ||
| readdirSync(dir).some((name) => name !== "hyperframes.json" && !kept.has(resolve(dir, name))) |
There was a problem hiding this comment.
This new in-folder source exemption still compares path spelling, unlike the transcode alias guard below. On a case-insensitive macOS/Windows volume, if the only file in the project folder is Original.MP4 and the person passes --video original.mp4, existsSync(videoPath) succeeds, but readdirSync returns Original.MP4 and kept.has(resolve(dir, name)) is false; noninteractive init refuses the otherwise valid source as “directory not empty.” On the interactive branch, accepting overwrite can reach copyIntoProjectUnlessAlreadyThere, whose case-sensitive resolve(src) !== resolve(dest) can attempt to copy that file onto itself. Please compare filesystem identity/canonical names (including at the copy site), and cover a case-only alias on a case-insensitive filesystem; the current .MP4 transcode test uses the same spelling and does not exercise this.
…elling On a case-blind disk --video original.mp4 names Original.MP4, so the folder check and the copy now compare device and inode; a source named in another case is accepted and never copied onto itself.
|
Thanks Jerrai, fixed in db29935. The folder check and the copy now identify the file on disk (
New test: |
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at db29935554cdb2864c7cbf1d3829e42ff14713c4, incremental review from my 3ca36295c and the subsequent case/identity fixes.
The earlier scaffold-name guard, honest saved_to receipt, uppercase-MP4 handling and ffprobe separator are present. One Important source-preservation regression is introduced by the new inode-based folder admission (packages/cli/src/commands/init.ts:297–305, interacting with :524–532).
Accepted aliases can become the transcode output and overwrite the original. The folder guard now accepts every media entry matching the source's dev/ino, but output selection still compares only lowercased path spelling. Reproduced with the built CLI and real FFmpeg on Linux:
- HEVC
original.mov, withoriginal.mp4hardlinked or symlinked to it: init chooses original.mp4 and rewrites the original inode. - HEVC
original.mp4, withoriginal.h264.mp4linked to it: the sidecar fallback rewrites the original inode too. - A source reached through a symlinked parent while the project uses its canonical path also bypasses the spelling check.
All five alias cases exit 0, create index.html, and change the source SHA and codec from HEVC to H.264. A normal .mov control keeps its source unchanged. Replacing only the new folder-admission guard with its prior version refuses all five aliases with exit 1, leaves source bytes unchanged and writes no index. I inspected that control and independently reran the exact-head built-CLI cases.
Fix: apply file identity to every proposed transcode destination, including the .h264.mp4 fallback. Allocate a distinct unused output (or refuse before writing) rather than overwriting an alias. Merely adding identity to the first ternary is insufficient when the fallback is itself an alias. Add hardlink/symlink regressions for both destination names plus a symlinked-parent input, asserting unchanged source bytes.
Validation: full root build passed; focused init/reverse tests 120 passed, 1 platform-specific skip; ffprobe argv-contract checks 122 passed. Reproducer on this devbox: node /home/ubuntu/workspaces/rames-jusso/hf-review/review-artifacts/7089/r4-20261103/agent-probes/alias-control.mjs /home/ubuntu/workspaces/hf-review-7089-r4-20261103. Root results are in adjacent root-alias.jsonl; prior guard controls are alias-prior-guard.jsonl. All probe projects are temporary fixtures; no live backend or native macOS/Windows run claimed. No source mutation retained.
Need: yes · Size: bounded re-review delta · Reuse: existing fileOnDisk identity helper at output allocation · Simpler: choose a genuinely distinct unused output · Structure: fine
Risk: high — successful initialization can destroy the person's original source bytes.
Verdict: REQUEST CHANGES
Reasoning: Identity-based admission must be paired with identity-safe output allocation; otherwise source aliases newly accepted by this change are overwritten during transcoding.
— hf-review
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed db29935554cdb2864c7cbf1d3829e42ff14713c4. The dev:ino check closes my earlier case-only source alias refusal/self-copy, and .MP4 conversion, ffprobe --, and the saved-skill receipt remain intact. One new source-preservation blocker: the broadened folder guard now accepts source aliases by file identity, but handleVideoFile chooses both its normal .mp4 and .h264.mp4 transcode destinations by lowercased pathname alone. A hardlink/symlink at either proposed destination can point to the selected source. FFmpeg is called with -y and overwrites that original while the command succeeds. Rames's separate exact-head review reproduces five real HEVC source changes and prior-guard controls; I independently checked the admission→destination→FFmpeg path at this SHA. Check file identity on every candidate output, including the fallback, and allocate a truly distinct name or refuse before writing. Required hosted checks are green but do not cover alias collisions. No local media run or merge from me.
— Jerrai
…lder # Conflicts: # packages/cli/src/commands/reverse/start.ts
…ked alias is never the output The folder guard admits every alias of the source by file identity, but the transcode output was chosen by path spelling. A hard link or symlink at name.mp4 or name.h264.mp4, or a source named through a linked folder, made FFmpeg (-y) rewrite the original. The output is now the first of name.mp4, name.h264.mp4, name.h264-2.mp4, ... with nothing at its path (lstat, so a link counts), which also covers a case-only spelling on a case-blind disk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HebQXN7wsMK6GqT3MNtqXw
|
Fixed at The change: New tests in
Each test asserts the source bytes are unchanged, the source is still HEVC, and the H.264 output is a separate file. Updated test: the existing Merge with main: it brought in the async Local checks: the |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 936d83a1f4e4f2cca0212f1e45ee2d023fac1108, against my prior linked-alias change request at db299355, also corroborated by Jerrai. That blocker is resolved; no new blocking or Important finding.
packages/cli/src/commands/init.ts:310-328 now uses lstat for every proposed .mp4, .h264.mp4, and numbered sidecar path before transcoding. Existing hardlinks, symlinks, and the source's own pathname are skipped. Both command modes share this allocator (init.ts:544). The earlier scaffold-name guard, file-identity copy guard, ffprobe separator, and honest saved_to receipt remain present.
Validation: frozen install and full root build passed. The init/initMedia/reverse/projectConfig suites passed 160 tests, with one case-blind-filesystem skip. The producer unit Vitest lane passed 1,170 tests, one skipped, including the ffprobe argv contract. All five added real-HEVC alias regressions fail at the source-byte assertion with the full prior db299355 init implementation on head dependencies, and pass at this head. Eight separate built-CLI/FFmpeg controls preserved source SHA-256 and HEVC codec, created separate H.264 output, and referenced it in generated HTML; these cover hardlink and symlink collisions at both normal .mp4 and .h264.mp4 destinations, a symlinked-parent input, repeated occupied numbered sidecars, the source itself as a candidate, and an ordinary .mov control. I independently replayed all eight built-CLI controls and the allocator controls. An allocator control also skipped a dangling symlink and directory in the candidate chain.
A first parent built-CLI attempt ran before build-copy finished and could not find the from-file template; after the completed build, the same eight scenarios passed.
Limits: Linux only; no native macOS/Windows execution, browser playback, live interactive terminal, or live reverse backend. Allocation remains a check followed by FFmpeg opening the path, so this review does not claim protection against another local process concurrently creating or replacing a candidate, including simultaneous CLI runs. No checked source changes retained.
Need: yes · Size: bounded fix · Reuse: shared media helper · Simpler: first unoccupied candidate · Structure: fine
Risk: low — tested existing aliases are preserved; allocation is not atomic against concurrent local writers.
Verdict: APPROVE
Reasoning: The prior source-destruction cases are resolved at this head, with regression tests that demonstrably fail the old implementation and independent real-media preservation controls.
— Rames (hf-review)
Superseded by exact-head 936d83a review: output names now use lstat and avoid every existing hard/symlink alias; five real-HEVC tests and hosted CLI checks cover the formerly destructive case. Not a merge action.
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved at the unchanged current head. freeTranscodeName checks each candidate with lstat, so a hard link, live or dangling symlink, or source reached through a linked folder cannot be selected as ffmpeg’s -y output; the five real-HEVC alias regressions ran in hosted CLI CI and assert unchanged source bytes/codec with a separate H.264 file. The main-merge async reverseUpgradeUrl callers are awaited. Rames independently approved the same head. Some Windows jobs were still pending at stamp time; independent CI remains the merge gate. Concurrent external creation between the path check and ffmpeg open is not an atomic guarantee, but no such writer was shown on this path. I did not merge or change the author’s existing auto-merge setting.
— Jerrai
…eview Adopts or partially adopts 8 groups: capture-failure diagnostics and frame-capture retry in engine, ffprobe fixes, renderOrchestrator updates, studio-server render/files/source-mutation routes, CLI screenshot retry and init/snapshot/timeline fixes, and the motion-blur land-then-undo path (fork seek recovery kept). Left out with ledger reasons: heygen-com#7211 render diagnostic CLI, heygen-com#7089 reverse start; heygen-com#7220 deferred. Core packed budget grows by the measured 1065 bytes since upstream heygen-com#7191 changed no budget. Watermark advances to 659ef56. Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-authored-by: WaterrrForever <miao.yang@heygen.com> Co-authored-by: Vance Ingalls <vance@heygen.com> Co-authored-by: James Russo <james.russo@heygen.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two bug bash items from Jake (#3 and #4 in his synopsis), reproduced on devbox before the fix.
What was wrong
reverse startalways returnsrecorded: false. Production answersPOST /v1/hyperframes/reverse/startwith 404: the route is still in the open backend PR heygen-com/experiment-framework#63633. With nothing recorded,reverse start --dir pmade no folder and nohyperframes.json.initrefused the project folder. Agents download the video into the folder they named, then runinit p --video p/original.mp4asstart.mdsays.initstopped with "Directory already exists and is not empty", so both of Jake's agents had to move files around.Fix
reverse start --dirwith no recorded session: still makes the folder, stampshyperframes.jsonwithauthoringSkill: reverse-any-video, and prints it assaved_to. A recorded session saves as before.init: a folder holding onlyhyperframes.jsonand the--video/--audioit was given is not "not empty". A source already at its place in the project is not copied onto itself. Any other file still makesinitrefuse.reversesection ofdocs/packages/cli.mdxsays the above.Tests
init.sourceInFolder.test.ts(new):hyperframes.jsonkeeps the skill.reverse/start.test.ts: unrecorded with--dirnow makes the folder with the skill and no session id.Devbox (API key, CLI packed from this branch)
reverse start stripe.mp4 --dir p:recorded: false, andp/hyperframes.jsonnames the skill.p, theninit p --video p/original.mp4 --keep-original --skip-transcribe --non-interactive: scaffolded, with the video untouched (cmpequal).Before this branch, step 2 failed with "Directory already exists and is not empty".
Recording the session itself needs the backend route to ship.
🤖 Generated with Claude Code