Source-only review of 1158d6853c34929d0003395354b13d877577a2c3 against effective base ae02e5aceff4742e5d72c20f0859eb3e6b449035. Three introduced correctness failures need changes before approval. The witnesses below are source-derived traces, not executed reproductions.

Strengths

• packages/cli/src/cloud/offload.ts:710–720 limits text decoding to rewriteable text files, preserving large binary Buffers instead of turning them into V8 strings.
• packages/cli/src/commands/cloud/render.ts:605–613 measures the actual compressed archive after offloading and checks it before the project ZIP upload; packages/cli/src/cloud/offload.ts:624–635 processes media sequentially and cleans its temporary directories in finally.

Blockers — P2 correctness

1. Keep the ZIP copy when a dynamic reference cannot be rewritten. packages/cli/src/cloud/offload.ts:649–673 deletes an uploaded file unless a text file still contains its entire basename. Consider a valid ≥5 MiB media/clip.mp4 used by a timed video whose script sets v.src = "media/" + "clip" + ".mp4", with no contiguous clip.mp4 elsewhere in project text. rewriteStringLiterals (:766–778) cannot replace those fragments with the uploaded URL, and isStillMentioned finds no whole basename, so the ZIP loses the file while the script still requests its local path. BASE kept that staged file; the existing producer explicitly collects media sources assigned after JavaScript (packages/producer/src/services/htmlCompiler.ts:2268–2321). This contradicts the promised dynamically constructed-path fallback. The current dynamic-reference test retains a full filename in a comment, so it does not cover split-literal references without a contiguous basename. Retain the ZIP copy when dynamic references cannot be resolved/rewritten; basename absence is not sufficient deletion evidence.

2. Resolve entry GIFs against the selected entry before preparing them. packages/cli/src/cloud/offload.ts:585–591 passes unrebased entry HTML to prepareAnimatedGifInputs with the project root as projectDir. With --composition compositions/scene.html, <img src="logo.gif">, and different animated GIFs at both logo.gif and compositions/logo.gif, the helper resolves and prepares the root GIF, then replaces the nested entry's image with that wrong animation. outputSrcPrefix only fixes the generated output URL; it does not resolve the input. The unchanged render compiler first rebases the selected entry (packages/producer/src/services/htmlCompiler.ts:1954–1996), using the sibling-first resolver in packages/parsers/src/rewriteSubCompPaths.ts:72–86, before GIF preparation. Preserve that input-resolution contract here, for example through a correctly resolved source mapping.

3. Encode the same snapshot that supplies the cache key. packages/cli/src/cloud/offload.ts:444–452 returns the live project pathname whenever it exists, but :483–495 keys the result by the staged Buffer. A concrete interleaving: staging captures A bytes for a ≥5 MiB b.m4a; while an earlier media upload is awaited, an editor saves different valid B bytes to b.m4a. For an audio element without an authored bounded duration, the preset remains audio and the trim plan remains null. FFmpeg then reads B, and :555–570 persists B's URL under A's hash/settings/null-trim key. Restoring A and rendering again hits that same key; the HEAD/content-length check validates the stored B asset, not equality with A. This silently persists the wrong media beyond the concurrent save. BASE zipped the staged Buffers consistently. Materialize the staged bytes for encoding, or otherwise bind the encoded input and cache identity to one immutable snapshot.

Audited: two independent code passes read all 11 existing changed BASE files before all 14 changed HEAD files, including tests as source. Parent reconciled their delivered numbered read ranges against hash-verified blobs with no changed-body gaps, verified the tree/path union, and independently traced the three failures through staging, rewriting, GIF resolution and cache read/write boundaries. Supporting compiler inspection was targeted, not whole-file coverage.

Trusting / limits: no tests, builds, installs, encoders, browsers, uploads, renders or CI validation run. The reported real-cloud run, frame/audio equivalence, deployed upload caps and CDN lifetime/access behavior remain author-provided evidence, not independent runtime proof. No claim of bounded whole-project heap use or complete transitive dependency coverage. GIF preparation running without an upload callback is explicitly admitted by the function comment; I am not treating that behavior alone as an unintended defect.

Review effects: none of the 16 inspected checked-in workflows has a review-submission trigger. External merge-loop/queue/app policy remains unverified. A later merge/main push can run checks and catalog publication; the latter can update generated branches/PRs and dispatch check workflows. At the reviewed feature-branch ref, the inspected npm-release entry condition is not satisfied; that does not establish post-merge or external-app behavior. No explicit merge, workflow dispatch, build, package publication or deployment operation is being invoked by this review; downstream effects of review submission are not independently established.

— tai

Verdict: REQUEST CHANGES
Reasoning: Dynamic-path media can disappear, nested entries can render the wrong GIF, and concurrent saves can cache B’s uploaded URL under A’s content-derived key. These are source-level correctness findings, not a hold for CI or missing runtime validation.

Reviewed at 1158d685. Requesting changes for two --variables regressions. The trim itself is right: with real ffmpeg, every played frame and audio sample lands where it should. The problem is which clips and files it applies to. cloud render accepts --variables (render.ts:104-133), and both issues below break that path.

Blockers

1. A variable-bound clip is trimmed and its in-point shifted (offloadTrim.ts:40-49, verified). playedWindow excludes loop, data-automation and timed ancestors, but not data-var-src. Take <video data-var-src="clip" src="a.mp4" data-media-start="10" data-duration="2">. The clip is cut and its start rewritten to 0.5. A render with --variables '{"clip":"b.mp4"}' then plays b.mp4 from 0.5 s, not 10 s: the wrong frames, with no error. Fix: return null from playedWindow for any element with data-var-src. Its source isn't known until render, so neither the cut nor the shift is valid.
2. A ≥ 5 MB file that no text file names is uploaded and then removed from the zip (offload.ts:626 → dropReplacedFiles, verified). offloadHeavyMedia offloads every heavy file in fileMap. isStillMentioned keeps a zip copy only if a staged text file still names it. A file used only through --variables (say media/b.mp4) is therefore missing at render time. That unused footage also goes to a public asset URL for no reason. Fix: offload only files that a rewritten reference actually points at. Leave everything else in the zip as today.

Should-fix
3. Animated GIFs are transcoded locally at any size, dry run included (offload.ts:578-599). A 2 KB GIF in the entry became a webm in the zip, where before this PR the server did that step. Inferred: without ffmpeg, any project with an animated GIF now fails with "Media upload failed". Limit it to GIFs at or above the 5 MB cutoff, or leave the GIF alone when ffmpeg is missing.
4. The cache key isn't scoped to the account (offload.ts:483). It's apiBaseUrl + sha256 + variant. On a machine that switches HeyGen accounts, account B silently reuses account A's asset URL. That only stays valid while A keeps the asset, and it links the two accounts' renders. Add the account or space id to the key.
5. The tests mock the encoder and miss load-bearing logic. Each of these was removed with the suite still green:

• the cut segments in the cache key (a stale cut would be reused after the composition changes);
• the frame-grid snap (with real ffmpeg at 29.97 fps, removing it put 50 of 60 frames off by one);
• the nested-composition and parent in-point exclusion;
• the data-automation exclusion;
• the "other mentions in the entry HTML" check;
• the HEAD size check on cache hits.

A real-ffmpeg test for the 29.97 snap and a cache-key test would pin the risky parts.
6. Docs. docs/packages/cli.mdx and docs/deploy/cloud.mdx should say that offloaded media sits behind public, unsigned asset URLs embedded in the render's HTML. The zip has always gone by asset_id, so individual files being directly addressable is new.

Nits (verified)

• JSON keys and labels. The string-literal rewrite also replaces them: {"intro.mp4": …, "label": "intro.mp4"} gets both rewritten (offload.ts:767-779).
• Cuts past the source end. They aren't clamped to the source length. A clip that starts past the end uploads a 261-byte stream-less mp4 with no error, and next to other clips it holds an unrelated segment's last frame (offload.ts:309-328).
• Substring match. "Still referenced" is a substring test, so a.mp4 stays in the zip because banana.mp4 is named (offload.ts:661-674).
• Query strings and fragments. ?query and #fragment are dropped: clip.mp4#t=5,8 becomes the bare URL (offload.ts:724-728).
• Null cache file. A cache file containing null crashes the run (offload.ts:198-204).

Verified fine

• Trim accuracy. Frame-exact at 30 and 29.97 fps across 4 clips and 3 segments, including overlapping windows and rate 2. FLAC → WAV is sample-exact at both ends, with A/V sync within 0–3 samples.
• Rewriting. CSS url() resolves against the stylesheet, srcset descriptors are kept, URL-encoded and unicode names resolve, and remote and data: URLs are left alone.
• Cache safety. Cache writes are atomic, and corrupt JSON falls back to an empty cache.
• Dry run and limits. A dry run does no auth, upload, encode or HEAD check (except the GIF step in 3). The 200 MiB per-file stop names the file.
• Tests. The three PR test files pass 79/79.

— Somu

Re-reviewed at 1d8c3d12, which adds one commit on top of 1158d685. Both of my blockers are fixed, so I've dismissed my change request.

1. Variable-bound clips (fixed). playedWindow now returns null for data-var-src, so such a clip is neither cut nor shifted. The docs list it with the other exceptions.
2. Unreferenced files (fixed). offloadHeavyMedia now offloads only referencedFiles(...): the same rewriteReferences walk, run as a dry pass that collects the keys it would replace. A file used only through --variables therefore stays in the zip and is never uploaded.

Should-fixes and nits from my review

• Done:
  • Animated GIFs are transcoded locally only when an entry GIF is at least 5 MB (entryGifSources returns null otherwise, so no local ffmpeg is needed).
  • The docs now say the asset URLs are public and unsigned.
  • A cache file containing null reads as empty.
  • ?query and #fragment are kept on rewritten URLs, and a clip whose src carries a media fragment is not trimmed.
  • Encodes now read a snapshot of the staged bytes, so the cache key and the encoded bytes match.
• Still open, not blocking: the cache key isn't scoped to the account (apiBaseUrl + sha256 + variant). The test gaps I listed (frame-grid snap, segments in the cache key, the nested and automation exclusions) are partly covered by the new tests. I haven't re-run the mutations.

Not approving yet. terencecho's CHANGES_REQUESTED (at 1158d685) is still open. The Comments check is red at this head, and CI was still running (12 pass, 25 pending). Once those clear and the lane re-pins this head, I'll approve.

— Somu

Re-review at 0b062841. The delta since my COMMENT at 1d8c3d12 is: ed4ac226, the main merge 65d38696, 7855bbd3 (over 2 GiB), ad0e2c82 (one descriptor) and 0b062841 (split-strings test). Nothing from my side blocks. I'm not approving yet, because @terencecho's CHANGES_REQUESTED (5581308964, from 1158d685) is still open. Here's how I read its three blockers at this head:

1. Dynamic path loses its ZIP copy: addressed. Only files a rewritable reference points at are offloaded (referencedFiles). The new test shows a file built from split strings ("media/" + "clip" + ".mp4") is never uploaded and stays in the ZIP (upload not called, fileMap.has true). isStillMentioned still keeps a ZIP copy when the basename survives rewriting.
2. Entry GIF resolved against the root: addressed. prepareEntryGifs now passes sourceAssets from entryGifSources, which resolves each entry GIF the way the renderer rebases a nested entry. So compositions/scene.html's logo.gif maps to the sibling-first file, not the root one.
3. Encode vs cache-key snapshot: addressed. planFile hashes the staged file (stagedSha256), and snapshotPath makes the encode read those bytes: an in-memory buffer gets a temp copy, and a file left on disk is used in place and checked with assertUnchanged. ad0e2c82 makes the size/change check and the read go through one open descriptor, so a save mid-render can't pair B's encode with A's key.

My earlier open point, the cache key not being account-scoped, is now a recorded product decision (per machine). That's acceptable as decided: the key is the content hash, so another account on the same machine reuses an asset only for byte-identical media it already holds.

Validate docs is red but not required.

Once Terence re-reviews or dismisses his CR, I'll approve this head.

— Somu

Changes requested at 0b0628411008adb56dac38012d91548c505595d4 (effective base c8f322c25910ed224e29ca5d1a62a521e62544cc). Three P2 source-level correctness issues remain.

1. Preserve the ZIP copy when a statically referenced file also has a split-built runtime reference

packages/cli/src/cloud/offload.ts:710–733

For a >=5 MiB media/clip.mp4, a page can contain both <video id="v" src="media/clip.mp4"> and document.getElementById("v").src = "media/" + "clip" + ".mp4". The static source admits the file for offload and is rewritten to the asset URL. The split literals are not rewritten and contain no complete clip.mp4 basename. After stripping the inserted asset URL, isStillMentioned therefore returns false and deletes the original from the archive. The script still requests the now-absent local file, whereas the base archive retained it.

The new split-only regression is a useful control, but it avoids offloading altogether; it does not cover this mixed case. This is also the remaining case acknowledged in the author's earlier response, rather than an unanswered question about the split-only fix. Please retain the original when runtime references cannot be proven rewritten, and add the mixed static/split-built case.

2. Apply the staged-file identity guarantee to the disk-backed entry-GIF path

packages/cli/src/cloud/offload.ts:650–654 (also 459–460, 620–634, 714–715)

A >=5 MiB GIF is a DiskFile, so snapshotPath returns its live original path. entryGifSources passes that path through the awaited producer import/preparation without the unchanged checks used by ordinary offload. The real producer reads the GIF bytes for metadata/hash, then transcodes by opening the same path (animatedGifPrep.ts:394–415, 284–305). Once prepared, the original GIF can be deleted from fileMap, so the later archive readStaged check cannot catch the change either.

Consequently, an edit after staging but before preparation can be accepted as the GIF input without the changed staged input being rejected. On a fresh GIF-cache miss, an edit after the producer hashes version A but before ffmpeg opens the path can also cache version B's video under A's content-derived GIF key. Buffer-backed GIF snapshots and the ordinary-file post-encode check do not cover this branch. Please provide an immutable input for GIF hashing/encoding, or enforce identity checks before accepting/persisting the prepared output; cover the disk-backed path, not only buildPublishFileMap's all-Buffer fixtures. This is an interleaving identified from source, not an observed production cache incident.

3. Do not trim away media that supported page initialization selects by changing its in-point

packages/cli/src/cloud/offloadTrim.ts:113–120 (also 40–54; caller offload.ts:539–550)

The basename-occurrence check only rules out additional textual references to the file; it does not establish that the clips' timing is static. For example, use a >=5 MiB, >102-second media/music.flac with a two-second root composition, <audio id="a" src="media/music.flac" data-start="0" data-duration="2" data-media-start="10">, and initialization that runs const a = document.getElementById("a"); a.setAttribute("data-media-start", "100"); a.volume = 0.5;. There is still only one basename occurrence, so the planner cuts 9.5–12.5 seconds and rewrites the static in-point to 0.5, but leaves the script's assignment to 100 intact.

This is not dependent on a browser probe that never runs: .volume = causes hasScriptedAudioVolumeAutomation/probeRequiresBrowser to select the real probe (probeStage.ts:135–149, 331–345), and its runtime reconciliation adopts the changed media start (185–215, 642–646). The base retains the whole source containing the 100-second passage; the new trimmed asset no longer contains it. Please conservatively skip trimming when supported runtime initialization can change a clip's playback window, with a regression covering an ID-based timing mutation that does not repeat the filename.

Verification limits

These are source-derived input/state traces, not executed reproductions. One reviewer completed all 12 BASE bodies before all 15 HEAD bodies in the changed-source pass; I checked its actual source deliveries and adjudicated the cited paths and the real GIF/probe consumers directly. The primary source/conventions pass stopped at its metadata cap after 13/15 HEAD bodies and remains incomplete. Two additional supporting-file reads in the completed changed-source pass were partial. This is not a complete two-pass approval.

No render, ffmpeg, upload, cache, browser/native/cloud reproduction, tests, or CI were run. No complete transitive build/deploy or pixel/audio-equivalence claim is made. Some source-read reservations were not renewed after context interruptions, some historical API/copy accounting could not be reconstructed exactly, and the first authored draft was charged after creation. Those process gaps remain; verifying the delivered source does not establish full reservation/accounting compliance.

The nested-entry GIF source resolution, normal Buffer snapshot, and split-only reference fixes are acknowledged; the above findings concern the remaining paths, not a blanket rejection of those fixes. Shared-cache/public-URL policy remains an attributed product choice, not an additional approval requirement.

— tai

Re-review at 1b8cd794, covering the delta 0b062841..1b8cd794 (one commit) against terencecho's three points at 0b062841.

1. Static plus split-built reference: fixed.
   • dropReplacedFiles now keeps the zip copy whenever the page's script code holds the file's stem and its extension, after asset URLs are stripped. "Script code" means inline <script> text from HTML files plus .js, .mjs, .cjs and .json files.
   • I checked the one way that could go blind: stageFile keeps every TEXT_FILE_EXTS file (html|css|m?js|cjs|json|svg|xml|txt) in memory regardless of size. So a large local script is never a DiskFile, and textOf always sees it.
   • The check is deliberately over-inclusive (any script containing clip and mp4). That fails toward keeping the file.
2. Disk-staged entry GIF: fixed. entryGifSources now always uses stagedCopy. A disk-staged GIF is copyFileSync'd and then assertUnchanged against its staged size and mtime. The producer hashes and transcodes only the private copy, so an edit after the copy can't reach the GIF cache, and an edit before it fails the render.
3. Clips a script can retime: fixed. planTrim returns null when RETIMING_SCRIPT_RE matches any page script: the timing data-* names, dataset, setAttribute, currentTime, playbackRate, .src or .loop. Terence's ID-based setAttribute page and an external dataset.mediaStart write are now tests.

Non-blocking:

• The retiming check runs over every script in the project, not per clip. So a project that ships any library locally (a vendored gsap.min.js contains setAttribute) never gets trimming. That's correct, but the trim saving will rarely apply to real projects. A per-file sidecar metric (trim_skipped: "script") would show how often.
• scriptCode(ctx.fileMap) re-parses every HTML file once per trim candidate. Computing it once per publish would avoid that.

No blockers from me at this head. I can't approve while terencecho's CHANGES_REQUESTED (5592149963 at 0b062841) is open. Once Terence clears it, ping me and I'll approve this head.

— Somu

Approving at 1b8cd794, the same head as my re-review 5595319593. That review found all three of terencecho's points from 0b062841 fixed, with only non-blocking notes. A maintainer has since dismissed his CHANGES_REQUESTED, the head hasn't moved, and every required check is green.

— Somu