Skip to content

feat(media-use): color grading — grade/lut resolve, smart-grade, grade-compare CLI#2041

Merged
miguel-heygen merged 11 commits into
mainfrom
feat/media-use-color-grading
Jul 9, 2026
Merged

feat(media-use): color grading — grade/lut resolve, smart-grade, grade-compare CLI#2041
miguel-heygen merged 11 commits into
mainfrom
feat/media-use-color-grading

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

📚 Stack (bottom): #2041main. #2065 (heygen onboarding) is stacked on top.

What

Adds color grading to media-use as first-class resolve types, plus a faithful comparison command for agents. All local, offline, deterministic — no model, no GPU.

resolve -t grade / -t lut

  • grade returns a paste-ready data-color-grading block; lut freezes a reusable .cube.
  • Look cascade (free-first): core preset (no file) → bundled .cube libraryparametric buildCube.
  • Every emitted .cube is Rec.709 and validated against core's colorLuts constraints (LUT_3D_SIZE ≤ 64) before it's frozen — invalid LUTs are rejected, never written.

Smart grade — grade --for <media>

  • Reads the actual footage (ffmpeg signalstats) and fills adjust (exposure / contrast / white balance) in core's keys + ranges.
  • Surfaces the measured evidence on stderr as a starting point; never auto-applies (a gray-world "correction" would ruin an intentional sunset/neon look). stdout stays valid JSON.

hyperframes grade-compare

  • Renders N candidate grades onto a reference frame through the real runtime shader into one labeled comparison PNG — so an agent picks a look without opening Studio, and what it sees matches render.
  • Prepends an "original" baseline cell by default (--no-baseline to omit).

hyperframes compare (generalized primitive)

  • The "render N variants → agent sees → picks" loop, generalized beyond color: hand it N composition variants (font/layout/motion/anything) → one labeled comparison sheet + JSON. grade-compare is its first color-specific specialization; both share the capture/captureCompositionFrame helper (no duplication).

Proactive guidance

  • media-use/SKILL.md gains a "media opportunity pass": a grounded signal→offer table (TTS, icons, images/upscale, SFX, BGM, grade), ask once, surface-don't-mutate.

Why

Color grading previously needed a human in Studio to judge every LUT — the agent could emit a grade but couldn't see whether it worked. This closes that loop agent-natively: resolve a look → compare faithfully → commit. Same "surface, don't silently act" philosophy as the reuse --candidates work.

Verification

  • media-use suite 131/131; cli command tests 28/28 (grade-compare / compare / snapshot / capture / contact-sheet).
  • oxlint + oxfmt --check clean; full bun run build green; fallow pre-commit gate green (duplication + complexity).
  • End-to-end: grade-compare renders a 4-cell sheet (baseline default) and 3-cell (--no-baseline); grades visibly distinct on real footage.

Notes / scope

  • Not a new shader: a 3D LUT + adjust is color-complete; the agent authors look data, not GLSL.
  • Out of scope (follow-ups): neural models (NILUT/Zeng/pix2pix), reference-image→LUT provider, HDR/ACES, and wiring the already-catalogued Real-ESRGAN upscaler into resolve.

@mintlify

mintlify Bot commented Jul 8, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview Jul 8, 2026, 12:24 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Comment thread packages/cli/src/commands/grade-compare.ts Fixed
miguel-heygen and others added 5 commits July 8, 2026 14:12
…e-compare CLI

Add color grading to media-use as first-class resolve types plus a faithful
comparison command. All local, offline, deterministic — no model, no GPU.

- resolve -t grade / -t lut: produce a data-color-grading block (or a frozen
  .cube). Look cascade: core preset (no file) -> bundled .cube library ->
  parametric buildCube. Emitted .cube is Rec.709 and validated against core's
  colorLuts constraints (LUT_3D_SIZE <= 64) before it is frozen.
- smart grade (grade --for <media>): ffmpeg signalstats -> adjust suggestion
  (exposure / contrast / white balance), surfaced with the measured evidence on
  stderr as a starting point; never auto-applied.
- hyperframes grade-compare: renders N candidate grades onto a reference frame
  through the real runtime shader into one labeled comparison PNG, so an agent
  picks a look without opening Studio. Prepends an "original" baseline cell by
  default (--no-baseline to omit). Shares the headless-capture pipeline with
  snapshot via capture/captureCompositionFrame.
- media-use SKILL: proactive "media opportunity pass" guidance (grounded
  signal -> offer, ask once, surface don't mutate).

Verified: media-use 116/116, grade-compare 7/7, snapshot 9/9, lint + format
clean, full build green, comparison renders end to end.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…grading

Assert the whole cell via toEqual instead of reaching into .grading.preset /
.grading.lut on the unknown-typed field, keeping the test typecheck-clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…ube; never-read-.cube guardrail

- resolve -t lut / -t grade --params '<json>': build a parametric .cube from
  explicit params (bypassing the intent cascade), validate, and freeze in one
  step. --intent becomes the optional description. Lets an agent commit a look
  it computed itself.
- --from <file.cube> now validates the ingested LUT for lut/grade types and
  rejects an invalid/oversized cube (no partial write) — the escape hatch for a
  LUT the agent generated with its own code.
- SKILL.md: hard rule to never read a .cube body into context (~size^3 lines,
  zero legible signal) — inspect via grade-compare (see it) or cube-validate
  (ok/size), read the manifest description for meaning; plus both authoring
  paths and the parametric-vs-film-stock ceiling note.

Verified: media-use 116/116, lint + format clean; smokes — --params builds a
valid frozen cube, grade --params returns a lut block, bad JSON and an oversized
--from cube are both rejected with no stray file.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…ls, caps candidates

Bug-bash follow-ups — grade-compare silently accepted bad input:

- Validate LUT *content*, not just existence: each referenced .cube is parsed
  with core's parseCubeLut (now exported from @hyperframes/core) and rejected
  with a per-cell error ("LUT for \"<label>\" is not a valid .cube: ..."). A
  file that exists but isn't a valid cube no longer renders a silent no-op cell.
- Warn on inactive cells: a grading that normalizes to inactive (e.g. a
  malformed {lut:12345}) emits a stderr warning naming the cell; the
  auto-prepended "original" baseline is intentionally inactive and stays silent.
  stdout remains valid JSON.
- Cap candidates at 16 (excluding baseline): over-cap input renders the first N
  and reports {truncated:true, total:M} on stdout + a stderr note — no silent
  drop, no unbounded giant sheet.

Verified: grade-compare 10/10; non-cube LUT → clear error; {lut:12345} → warning
+ ok; 20 cells → cells=17 truncated total=20; valid runs unchanged. Lint/format
clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
Generalize grade-compare's "render N variants → one labeled sheet → the agent
looks and picks" loop into a standalone command that works on ANY variation
(font, layout, motion, grade, whole compositions) — the tool never needs to
know what differs.

- `hyperframes compare <path...> [--at <sec>] [--labels a,b,c] [--out] [--cols]
  [--json]`: renders each agent-authored composition variant through the real
  runtime (captureCompositionFrame) and stitches one labeled comparison sheet +
  JSON ({ok, sheet, rendered, variants, truncated?/total?}). 2+ paths required;
  caps at 16 with loud truncation. It presents, it does not judge — choosing is
  the caller's job.
- Factored the shared "render a labeled set → contact sheet" path so compare,
  grade-compare, and snapshot all sit on it (no duplication). grade-compare is
  now the first color-specific specialization of this primitive.
- New pathArgs util + contactSheet test; hyperframes-cli SKILL documents compare
  as the agent's "see your own renders and choose" primitive.

Verified: 26/26 across compare + grade-compare + snapshot + contactSheet (no
regressions); compare renders 3 variants into one visibly-distinct labeled
sheet; 2+-path error path clean; lint/format clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv

Copy link
Copy Markdown
Collaborator Author

miguel-heygen and others added 5 commits July 8, 2026 15:21
…arkdown

The "Test: skills" CI job runs bare `node --test` with no ffmpeg on PATH (by
design — skills tests are meant to be node-builtin-only). The grade-analyzer +
smart-grade tests shell to ffmpeg and were failing there with ENOENT. Guard
them to skip when ffmpeg isn't on PATH; they still run locally / where it is.

Also oxfmt README.md + hyperframes/media-use SKILL.md (the whole-repo
`oxfmt --check .` Format job caught markdown left unformatted by the rebase
conflict resolution).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
The "Test: skills" CI job installs no deps, so the normalizeHfColorGrading
conformance test (which imports core's TS via `node --import tsx`) failed there.
Guard it to skip when tsx can't resolve; runs locally / in the deps-installed
Test job. Completes the skills-CI greening (the ffmpeg guards handled the rest).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…ws-safe compare test

- grade-compare built `<img src="...">` (double-quoted) with the single-quote
  escaper, leaving `"` unescaped — a `"` in the frame path could break out
  (CodeQL: incomplete HTML attribute sanitization). Use escapeXml for src.
- compare label test hard-coded POSIX paths that can't match on Windows; assert
  the derived labels (the subject); path resolution is covered elsewhere.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
… .cube files)

The 3 bundled .cube files were 733 lines each (2,199 total) and were themselves
buildCube output — pure repo bloat. Replace with compact per-look params in
luts/index.json, generated on resolve; add an optional `url` for future scanned
LUTs to be CDN-hosted + downloaded on demand (freezeUrl) instead of committed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
….ai/luts), params fallback

Looks now carry a CDN `url` (hosted at s3://heygen-public/luts → static.heygen.ai/luts/<id>.cube);
resolve downloads + validates + freezes on demand, like bgm/image. `params` stays
as the deterministic offline fallback (--local-only, or if the download fails), so
resolution is never blocked on the network. Provider prefers url, falls back to params.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at bf1df97.

Nice, self-contained feature — the layered guardrails (freezeUrl size cap → CDN → params-fallback → validated .cube before write) and the "surface, don't mutate" smart-grade posture are load-bearing choices that made this diff readable at 3945 lines. Registration + failure-tracking wiring for the two new subcommands looks clean (packages/cli/src/cli.ts:142-159 wraps every loader symmetrically), so the historical asymmetric-handler footgun does not apply here. Test:code ratio on the net-new files is comfortably >0.5, and the CDN endpoints listed in skills/media-use/luts/index.json return live, distinct 200s.

A few things worth surfacing, all non-blocking:

Concerns

  • provenance.via is captured but never emitted as telemetry — CDN→params fallback is invisible in metrics. freezeLibraryLut stamps via: "url" | "params-fallback" | "params" onto the record (skills/media-use/scripts/lib/lut-preset-provider.mjs:191,217), but track("media_use_resolve", …) in resolve.mjs:755-761 only sends type/source/provider/local_only/provider_override. If static.heygen.ai/luts/*.cube starts 404ing (S3 object accidentally deleted, cache miss, DNS regression), users silently downgrade from the curated Rec.709 LUT to a parametric synthesis of the same look — same field name, subtly different pixels, zero signal to catch it. Since the PR body advertises "faithful comparison through the real runtime shader," a silent shift to the parametric approximation is exactly the kind of drift you'd want to see in a dashboard. Adding via (and maybe a boolean library_fallback) to the media_use_resolve payload closes this. Complements #2065's --doctor — doctor answers "can I reach the CDN right now?" but only telemetry answers "is the CDN regressing for real users?"

  • No adoption/cost event on grade-compare / compare. Both commands stand up a full headless Chrome, render up to 16 cells (grade-compare in one composition, compare per-variant), and screenshot. The outer _trackCommandResult in cli.ts captures success + duration, but there's no signal for cell count, truncation-cap hits, or how often the 5-second renderReadyTimeoutMs fires the "may be inaccurate" warning. For a new CPU-intensive path, that's the observability you want before you can safely add more cells or lift the cap. capCandidateCells and capCompareVariants already carry a truncated/total result — surfacing that as a property on a per-command event is a two-line change.

  • renderReadyTimeoutMs = 5000 in captureGradeCompareSheet is hardcoded and shared across 1..16 cells. grade-compare.ts:648 passes 5000 regardless of cell count; with a full 16-cell grid all animating on one timeline, the runtime may miss __renderReady and emit the "grade comparison may be inaccurate" warning while still writing the PNG. Result: a subtly-wrong comparison the agent can't tell is wrong. snapshot exposes --timeout for this same reason; grade-compare should too, or scale the timeout with cell count.

Nits

  • compare.ts stages an HTML variant by cpSync(dirname(variant.inputPath), stagedDir, { recursive: true }) (line 196) — the entire sibling directory. Users running hyperframes compare ./variants/a.html ./variants/b.html where ./variants/ sits next to node_modules or a raw-footage/ folder will silently copy those into tmp. snapshot.ts already does this, so it's not a new class of bug; worth a .hyperframes-ignore or size preflight later. (nit)
  • luts/index.json's notes field embeds s3://heygen-public/luts/ — operator-facing hostinfo in an agent-facing config. Consider moving to a LUT-authoring README so the agent-consumed JSON stays purely descriptive. (nit)

Questions

  • Parity: is the WebGL grading path exercised by grade-compare byte-identical to what render bakes into produced video? The PR positions the comparison as faithful; a pinned parity test between a grade-compare cell and a single-frame render at the same time + grading would enshrine that invariant so a future shader refactor can't silently break the "what the agent sees matches render" contract.

What I didn't verify

  • I didn't run the CLI locally against the live CDN; the review is static + PR-diff-only.
  • I didn't trace the smart-grade signalstats numbers through to the actual pixel result — the clamp/round pipeline in statsToAdjust looked reasonable but I didn't verify the chromaWarmth formula against known footage.
  • I didn't audit the shader/runtime path in packages/core for the WebGL parity question (Q1); flagging it as a question rather than a finding for that reason.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 — hyperframes #2041 at bf1df973

🟢 LGTM on the feature core. Substantial addition (36 files, +3945/-200, 10 commits with a visible bug-bash trail) landing color grading as a first-class media-use resolve type plus a runtime-faithful grade-compare CLI. Cross-checked the load-bearing safety claims — LUT_3D_SIZE ≤ 64 bound, "never written" freeze pattern, subprocess safety, truncation cap, HTML escaping. One P1 on atomicity, a handful of P2 observations, none merge-blocking.

Transparency note before findings: I ran an audit sub-agent to help scope the diff. Roughly half its P1s didn't survive independent verification (fabricated line numbers, one directly contradicted by an in-function guard). What follows is only what I confirmed on-disk against the actual file content at bf1df973.

Verified

  • LUT_3D_SIZE ≤ 64 enforced at generation entry. cube-build.mjs:116-118: if (!Number.isInteger(size) || size < 2 || size > MAX_SIZE) throw — guard fires before iteration, closing the "oversized never generated" claim. cube-validate.mjs also enforces on ingest. Both boundaries covered. ✅
  • Subprocess safety. All ffmpeg/ffprobe invocations (grade-analyzer.mjs :135-153, and by inspection the other CLI paths) use execFileSync with argv arrays + explicit timeouts (ffprobe: 5000ms, ffmpeg signalstats: 15000ms). No shell strings, no template interpolation into command lines. Media paths ride as the arg after -i; injection surface is closed. ✅
  • signalstats classifier is over documented output. grade-analyzer.mjs:69 regex /lavfi\.signalstats\.([A-Z]+)=([+-]?(?:\d+(?:\.\d+)?|\.\d+))/ matches ffmpeg's stable signalstats field format (YMIN/YMAX/YAVG/UAVG/VAVG). Parser requires all five to be Number.isFinite before including a frame, throws on zero valid frames. Unlike my [[verify-classifier-empirical-premise]] miss on HF #2044, the empirical premise here is well-documented in ffmpeg and the filter fields are stable lavfi API. ✅
  • Baseline default + --no-baseline. grade-compare.ts:597-600 declares the citty baseline: { type: boolean, default: true } shape; :626-627 guards if (args.baseline !== false) before prepending. Both branches are covered.
  • Truncation cap uniformity. Both capCandidateCells (grade-compare.ts:148) and capCompareVariants (compare.ts:147) enforce MAX_*=16 with {truncated: true, total} on stdout + stderr warning line. Payload builders preserve the reporting shape. Tests pin both cases.
  • --from invalid cube rejection. assertValidCubeFile gate rejects with a per-cell error; PR body's "no partial write" for the ingest path holds because ingest validates the user's existing file and passes-through only after it clears.
  • HTML escaping surface. grade-compare.ts:113-128: escapeXml covers &<>"' for double-quoted / attribute contexts; escapeSingleQuotedAttr covers &<>' for the data-hf-color-grading='...' single-quoted attribute. Both used at insertion sites (:323-331). The stale GHAS "incomplete HTML attribute sanitization" comment at 00:29Z on this PR is pre-fix — the fix(cli): escape grade-compare src double-quotes (CodeQL XSS) commit corrected it (visible via escapeXml(options.frameSrc) in the current head). GHAS just didn't re-run to auto-dismiss.
  • One-validator-many-writers pattern. cube-build.mjs (build) sits next to cube-validate.mjs (validate); ingest (resolve.mjs --from), generation (parametric buildCube), AND download (freezeUrl → assertValidCubeFile) all funnel through the same validator. That's the shape my [[verify-full-dispatch-chain]] discipline wants to see — one validator, multiple write paths, all gated by it.

P1 — should address before merge

P1a — "Never written" is validate-after-write, not write-to-temp-then-rename. lut-preset-provider.mjs:188-193:

await freezeUrl(match.url, fullPath);            // downloads to FINAL path
assertValidCubeFile(fullPath, ...);              // throws if invalid; rmSync in catch

and :207-210:

writeFileSync(fullPath, cube);                   // writes to FINAL path
assertValidCubeFile(fullPath, ...);              // throws if invalid; rmSync in catch

If the process is SIGKILL'd / OOM'd / crashes in an unrelated code path between the writeFileSync/freezeUrl completing and assertValidCubeFile throwing, an invalid .cube orphans at the final path. The catch-block rmSync never fires on crash — only on validation-throws.

Practical exposure is narrow: (a) freezeUrl returning means the download completed, so what's on disk is at least the full bytes; (b) allocateId presumably increments per-request so subsequent resolves miss this fullPath. Impact is a disk orphan, not a serving of poisoned bytes. But the PR body's "invalid LUTs are rejected, never written" reads stronger than reality — the correct pattern is write-to-.tmp, validate, atomic rename to .cube. Cheap, textbook, and lets the claim be exactly true. Not a security bug, but a factual gap between doc and code that a follow-up bug-bash will find.

P2 — observations (non-blocking)

O1 — mergeSmartAdjust doesn't gate args.for on file existence. resolve.mjs:362-365:

function mergeSmartAdjust(block) {
  if (!args.for) return block;
  const mediaPath = resolve(args.for);
  const analysis = analyzeMediaGrade(mediaPath);

If the user typos the --for path, ffmpeg surfaces its own "No such file" through the caught path. Functionally fine (the error IS surfaced), but a two-line if (!existsSync(mediaPath)) throw new Error("--for file not found: " + mediaPath) upfront produces a cleaner error than an ffmpeg stack. Taste-call.

O2 — Rec.709 luma coefficients undocumented. cube-build.mjs uses 0.2126 / 0.7152 / 0.0722 (Rec.709 luma weightings) and 0.28 / 0.08 for temperature adjust. Both are correct for the color-space the PR body cites, but neither carries a // Rec.709 luma comment. Future readers doing color math will thank you.

O3 — signalstats field-name uppercase assumption. The regex assumes uppercase-only field names, which matches current ffmpeg. If a future ffmpeg release added a mixed-case field, the parser silently skips. A test that invokes real ffmpeg on a known frame and asserts frames > 0 would pin this empirically. Not required; smart-grade IS gated on "no signalstats frames found" throw, so a total regression at least fails loud.

O4 — Subprocess timeouts hardcoded. 5s ffprobe, 15s ffmpeg-signalstats. Consistent with the rest of the tree. A long HD MP4 on slow storage could exceed 15s in signalstats; if support ever reports flake, HYPERFRAMES_ANALYZE_TIMEOUT_MS is a two-line follow-up. Not a today problem.

O5 — mediaPath echoed verbatim in measured-note. The analyzer's stderr note echoes the resolved absolute path. On CI logs / shared systems this is a minor infosec nit — basename(mediaPath) or a cwd-relative form would be tighter. Trivially fixable.

Positive-pattern notes

  • Runtime-faithful compare. captureCompositionFrame sharing between snapshot / grade-compare / compare means "the agent sees what render produces" holds by construction rather than convention. Nice consolidation.
  • Bug-bash commit trail visible. 10 commits including fix(cli): grade-compare validates referenced LUTs, warns on no-op cells, caps candidates, fix(cli): escape grade-compare src double-quotes (CodeQL XSS), fix(ci): skip core-conformance test when tsx is unavailable — this is a PR that got its own bug-bash before shipping, and it shows in the "no P0" outcome.

CI status

Not fetching status this turn — Miguel's PR body reports media-use suite 131/131, cli command tests 28/28, oxlint + oxfmt clean, bun run build green, fallow gate green. If any of those flipped red since the PR body was written, worth a re-check before merge.

R1 by Via

…emetry, follow-ups

- Atomic .cube writes: library provider (url + params) and the parametric
  generator now write to a .tmp path, validate, then rename, so a crash can
  never orphan an invalid .cube at the final path (was validate-after-write).
- track("media_use_resolve") now emits provenance.via (url/params-fallback/params).
- grade-compare + compare: --timeout flag (was hardcoded 5000) and a
  media_use_compare event (cells, truncated, total, render_ready_timed_out);
  openSettledCompositionPage now surfaces the render-ready timeout.
- compare staging skips node_modules/.git; --for gets an upfront existence check.
- Rec.709 luma comment; HYPERFRAMES_ANALYZE_TIMEOUT_MS override; measured note
  uses basename; LUT s3 hosting moved from index.json into luts/README.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 — hyperframes #2041 at 4be372f7

🟢 Cleared. Verified all 6 R1 findings + Rames D Jusso's Concerns against the R2 commit. Miguel's commit message maps 1:1 to the findings, and each grep-verifies clean at 4be372f7.

Verified fixes (my R1)

  • P1a atomic writes ✅lut-preset-provider.mjs:
    • CDN path (:188-198): tmpPath = ${fullPath}.tmpfreezeUrl(url, tmpPath)assertValidCubeFile(tmpPath)renameSync(tmpPath, fullPath). On catch: rmSync(tmpPath).
    • Params path (:209-217): same shape (writeFileSync(tmpPath, cube) → validate → rename).
    • renameSync newly imported at :1. Comment at :190-191 names the guarantee: "crash can never orphan an invalid .cube at the final path". The PR body's "invalid LUTs are rejected, never written" is now honestly true.
  • O1 --for upfront existsSync ✅resolve.mjs:365-366: // Clear upfront error beats an ffmpeg "No such file" stack on a typo'd path. + if (!existsSync(mediaPath)) throw new Error(\--for file not found: ${mediaPath}`);`. Matches suggested wording verbatim.
  • O2 Rec.709 luma comment ✅cube-build.mjs:17-19: // Rec.709 luma weightings (matches the color space the grading runtime uses). on the luma helper.
  • O3 signalstats uppercase-field test — not addressed. That was an "empirical test suggestion," not a fix ask. Regex /[A-Z]+/ unchanged; still correct for current ffmpeg. Fine.
  • O4 env-var timeout ✅grade-analyzer.mjs:7-8: const SIGNALSTATS_TIMEOUT_MS = Number(process.env.HYPERFRAMES_ANALYZE_TIMEOUT_MS) || 15000; with // override without a code change comment. ffprobe stays at hardcoded 5000 — reasonable, probe is quick metadata.
  • O5 basename in measured note ✅grade-analyzer.mjs:164: formatMeasuredNote uses basename(mediaPath); basename imported at :2.

Verified fixes (Rames D Jusso's Concerns — reviewing as neighbor)

  • provenance.via emitted ✅ — new packages/cli/src/telemetry/events.ts (+19), and resolve.mjs's track("media_use_resolve", ...) payload now carries via (url / params-fallback / params). CDN 404 → parametric drift is now observable end-to-end.
  • media_use_compare event ✅events.ts:501-507: trackEvent("media_use_compare", { command, cells, truncated, total, render_ready_timed_out, ... }) — the exact shape Rames asked for. Both grade-compare and compare emit through it.
  • --timeout flag on grade-compare + compare ✅compare.ts:378 and grade-compare.ts:608 add the citty timeout option; default 5000, renderReadyTimeoutMs: opts.timeoutMs plumbed to openSettledCompositionPage. Different shape than "scale with cell count" (Rames's alternative) but a cleaner API — caller decides per-invocation. Also: openSettledCompositionPage now surfaces the timeout back to the outer event so render_ready_timed_out is a truthful signal.
  • Compare staging skips node_modules/.git ✅compare.ts:203-208: return base !== "node_modules" && base !== ".git"; filter in the recursive cpSync callback. Comment names the failure mode Rames described.
  • luts/index.json operator-hostinfo split ✅ — new luts/README.md (+28) carries the s3:// hosting info; luts/index.json -1/+1 removes the operator-facing note from the agent-consumed JSON.

CI

Not fetching this turn — assumption is the R2 push landed on the same green tree the R1 did. If any of Test, Fallow, Typecheck have flipped red since, worth a re-check before merge.

R2 by Via — R1 P1 resolved; all raised observations either addressed or explicitly acknowledged as intentional.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 4be372f7 — R2.

Clean pass on the three R1 🟠 items — one commit (4be372f7) covers all three plus the nits and adds unrequested atomic-write hardening on the .cube materialization. Test:code balance for the fix commit is zero (no test changes), but CI is green and the emits are shape-simple enough that visual review + typecheck cover them. Approval-wise: 🟢 LGTM.

R1 items → R2 status

R1 🟠 (1): provenance.via is captured but never emitted as telemetry — CDN→params fallback is invisible in metrics. freezeLibraryLut stamps via: "url" | "params-fallback" | "params" onto the record but track("media_use_resolve", …) only sends type/source/provider/local_only/provider_override. Adding via (and maybe a boolean library_fallback) to the media_use_resolve payload closes this.

RESOLVED. 4be372f7 adds via: record.provenance?.via to the media_use_resolve payload at skills/media-use/scripts/resolve.mjs:768, with a comment naming exactly the scenario the R1 concern raised (silent CDN→params downgrade in prod, complements --doctor). Verified libraryRecord sets via on all three library paths (lut-preset-provider.mjs:196 "url", :226 "params-fallback" / "params"), so a dashboard filter on via="params-fallback" will now light up if static.heygen.ai/luts/*.cube starts 404ing. Non-library resolutions leave via undefined, which PostHog handles cleanly.

R1 🟠 (2): No adoption/cost event on grade-compare / compare. Both commands stand up a full headless Chrome, render up to 16 cells, screenshot. Outer _trackCommandResult captures success + duration, but there's no signal for cell count, truncation-cap hits, or how often the 5-second renderReadyTimeoutMs fires the "may be inaccurate" warning. capCandidateCells and capCompareVariants already carry a truncated/total result — surfacing that as a property on a per-command event is a two-line change.

RESOLVED. New trackCompareSheet helper at packages/cli/src/telemetry/events.ts:492-506 emits a media_use_compare event with the full requested tag set: command, cells, truncated, total, render_ready_timed_out. Wired at both call sites (grade-compare.ts:670-676, compare.ts:333-339). The renderReadyTimedOut signal is plumbed properly through openSettledCompositionPage (captureCompositionFrame.ts:20-22, 115), keyed off whether compositionRuntimeReadyInBrowser ever resolved — a truthful signal, not a proxy. One note: the emit only fires on success paths; a mid-run variant throw skips it, so failure-mode cell-count distributions come only from the outer command result. That's the right split for now, but worth remembering if you later want "how big are the sheets that error out."

R1 🟠 (3): renderReadyTimeoutMs = 5000 in captureGradeCompareSheet is hardcoded and shared across 1..16 cells. With a full 16-cell grid all animating on one timeline, the runtime may miss __renderReady and emit the "grade comparison may be inaccurate" warning while still writing the PNG. snapshot exposes --timeout for this same reason; grade-compare should too, or scale the timeout with cell count.

RESOLVED. grade-compare.ts:608-611 and compare.ts:378-381 both expose --timeout (string, default 5000ms) mirroring snapshot --timeout. Parsed at grade-compare.ts:288 and compare.ts:147-149 with a safe Number.parseInt || fallback idiom, so --timeout=<garbage> won't crash. The 5000 default is preserved (no behavior change for existing callers), so agents driving 16-cell grids can bump --timeout 15000 when they know the composition is heavy, and the render_ready_timed_out telemetry tells them (and you) when to bump. Not scale-with-cell-count, but flag > auto since a chatty on-timeline composition can time out on 4 cells and a lightweight one won't on 16.

R1 nits → R2 status

  • compare.ts stages an HTML variant by cpSync(dirname(variant.inputPath), stagedDir, { recursive: true }) — the entire sibling directory. Users running hyperframes compare ./variants/a.html ./variants/b.html where ./variants/ sits next to node_modules or a raw-footage/ folder will silently copy those into tmp.
    RESOLVED. compare.ts:203-208 now passes a filter that skips node_modules and .git. Full mitigation would need .hyperframes-ignore or a preflight size check as the R1 note flagged, but skipping the two biggest offenders covers the common case cleanly.
  • luts/index.json's notes field embeds s3://heygen-public/luts/ — operator-facing hostinfo in an agent-facing config.
    RESOLVED. skills/media-use/luts/README.md (new file) picks up the operator-facing s3 hosting doc; index.json:notes is now purely descriptive.

R1 question → R2 status

  • Parity: is the WebGL grading path exercised by grade-compare byte-identical to what render bakes into produced video?
    STILL OPEN as a question, not blocking. No parity test was added in this cycle. That's fine — the question was flagged as a _would enshrine the invariant_ future-proofing, not a defect claim. Worth filing as a follow-up if the shader is ever touched.

Cross-PR carry (#2065 --doctor)

#2065 is still open (unmerged, this PR does not depend on it), so the R1 note that --doctor doesn't yet probe static.heygen.ai reachability applies to that PR, not this one. The via tag added here means we now have prod-observability for the CDN regression, which was the R1 rationale — --doctor is the pre-run "can I reach the CDN?" check, via="params-fallback" is the post-run "is the CDN regressing for real users?" signal. Independent, both useful.

R2-NEW findings

None. Miguel also snuck in an unrequested atomic-write hardening (write to .tmp → validate → renameSync) at lut-preset-provider.mjs:186-217 and resolve.mjs:387-395 that closes a race where a SIGKILL between writeFileSync and validateCubeFile would leave a corrupt .cube at the final path. Nice — that was going to be a follow-up eventually and it's better off inline with the rest of the LUT-write work.

What I didn't verify

  • I didn't run the CLI locally against the live CDN, still static + PR-diff-only.
  • I didn't verify that the new media_use_compare and via fields land in PostHog with the shape the code emits — visual review says yes, live emission would confirm. Standard telemetry-emit trust for a shape-simple trackEvent call.
  • Regression-shard CI checks are still pending at review time (long-running perf/regression suites). The required checks that gate merge are all green.

Review by Rames D Jusso

@miguel-heygen miguel-heygen merged commit 57b3c78 into main Jul 9, 2026
54 checks passed
@miguel-heygen miguel-heygen deleted the feat/media-use-color-grading branch July 9, 2026 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants