Skip to content

fix(cli): measure answers the four mistakes agents make most - #7443

Merged
WaterrrForever merged 5 commits into
mainfrom
miao/rav-measure-ux
Nov 9, 2026
Merged

WaterrrForever merged 5 commits into
mainfrom
miao/rav-measure-ux

Conversation

@WaterrrForever

@WaterrrForever WaterrrForever commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Agents most often get hyperframes measure wrong in three ways:

  • they name no op (measure ref.mp4);
  • they pass a path it can't read;
  • they use a flag it doesn't take.

All three still happen after the unknown-command hint from #7247, and some agents never get past them to a successful measure.

What changes

  • measure <video> with no op, on its own or with --json, runs probe and says so on stderr, showing the path quoted. An unquoted path with spaces (measure ref one.mp4) is joined into the file it names. With other flags the op can't be guessed, so the unknown-command hint asks for one. The hint now also names a path that does not exist yet (clips/x.mp4).
  • When ffprobe can't read a file, the error carries ffprobe's most specific line and a matching fix. A truncated MP4 and a web page saved as .mp4 print the same two lines, so the file's header decides between them:
    • ftyp at bytes 4–8 means the file is cut short: download it again;
    • otherwise it isn't a video at all.
  • An unquoted path with spaces that a command rejects as extra words gets a hint to quote it, wherever it falls in argv, flag values included.
  • Window flags accept --from/--to, and folder flags accept -o.

Checked

  • vitest: measure, projectShortcut, didYouMean, reject-extra-positionals, commands/measure and cli. This includes both two-line ffprobe outputs, and probeVideo on a real half-truncated MP4.
  • By hand, with 2–3 s test MP4s:
    • a bare file probes; an unquoted spaced bare path probes the joined file;
    • look a.mp4 --compare <spaced path unquoted> prints the quote hint;
    • cuts --from 0.5 --to 2 measures that window;
    • an HTML file named .mp4 gets the "not a video" fix.

The usage errors agents hit most on `measure`: no op named, a path it
cannot read, and a flag it does not take. Some agents never get past
them to a successful measure.

- `measure <video>` with no op (alone or with --json) runs `probe` and
  says so on stderr. With other flags the op is a guess, so the
  unknown-command hint asks for one; it now names a path that does not
  exist yet too.
- When ffprobe cannot read a file, the error carries ffprobe's reason and
  a matching fix: a cut-short download, a page saved as .mp4, a
  permission.
- An unquoted path with spaces says to quote it, both where the extra
  words are rejected and where the cut path is missing.
- Window flags take --from/--to, and folder flags take -o.
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 267 (base branch 267), smooth 97 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

@jrusso1020 jrusso1020 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.

Reviewed at 391e10f8381e7590c957eff99df60395e7d4911f. No new blocker found.

measureProbeShortcutArgv preserves explicit operations, only rewrites an existing regular file with no trailing arguments beyond --json, and cli.ts announces the probe on stderr. Thus valid existing scripted operation forms are not reinterpreted; the newly accepted form previously failed. Missing paths, directories and ambiguous operation flags retain their error path. The shared output-folder aliases and window aliases preserve the original names.

Validation: gated Vitest projectShortcut/didYouMean tests: 37 passed. Two additional focused probes passed: real ffprobe rejects a spaced-path HTML file with the reason/fix, and runMeasure emits failure JSON while throwing exitCode 1 (unrelated UI/update dependencies mocked). Source traced ffprobe reason capture, bounded reason text, shell-safe execFile arguments and runtime exit mapping. Wider targeted suites could not collect against the borrowed dependency tree (missing current engine/core/parser subpath exports); this is not a passing full-CLI/build claim. No full build or live agent run performed.

Pre-existing output distinction: packages/cli/src/measure/command.ts:17–37 always returns measure failure data as JSON on stdout, even without --json, and throws a nonzero runtime result. Human stderr summaries are TTY-only; usage messages and the new shortcut notice use stderr. This PR preserves that existing JSON contract, so I am not claiming all errors print to stderr in unattended mode.

Need: yes · Size: ok · Reuse: existing project shortcut and shared measure argument/error helpers · Simpler: already minimal · Structure: fine (checked shared runMeasure/outArg and command registry; no split)
Risk: low — narrow additive CLI forms and clearer diagnostics.

Verdict: APPROVE
Reasoning: The default is explicit and restricted to previously invalid no-operation calls; valid operation calls and nonzero failures retain their contract.

— Rames / hf-review

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new help path is broadly sound, but an incomplete MP4 with a specific moov atom not found line followed by ffprobe's generic final summary gets the wrong repair instruction because the classifier sees only the last line. Please keep the specific diagnosis before the generic summary and add a composed regression. — Jerrai

Comment thread packages/cli/src/measure/media.ts Outdated
/** ffprobe's own last word on a file it could not read, without the path it repeats. */
export function ffprobeReason(err: unknown, path: string): string | undefined {
const stderr = (err as { stderr?: Buffer | string } | null)?.stderr?.toString() ?? "";
const line = stderr.trim().split(/\r?\n/).pop()?.replace(`${path}: `, "").trim();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When ffprobe reports a truncated MP4 it can emit moov atom not found followed by <path>: Invalid data found when processing input. Taking only the last line discards the specific diagnostic; unreadableFix(reason) then recommends “this is not a video” instead of “download it again.” Classify from the bounded full stderr (specific cause before generic summary) while retaining a safe concise displayed reason, and add a two-line truncation test through probeVideo or the composed helpers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Miguel's same-head ffprobe 7.0.2 probe narrows my suggested fix: a truncated MP4 and an HTML page named .mp4 can both emit moov atom not found followed by Invalid data found. Scanning full stderr alone would misclassify the HTML case. Please distinguish file content first (e.g. MP4 ftyp header versus HTML) and test both two-line stderr cases before choosing the download-again advice. The finding and change request stand; this replaces my earlier full-stderr-only suggestion.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e391c67: ffprobeReason now keeps the most specific line (moov atom not found over the generic Invalid data found last line). One wrinkle while reproducing: an HTML page saved as .mp4 gets the same two lines from ffprobe, so the fix is chosen from the line plus the file's own header (ftyp at bytes 4–8 → cut short, download again; no MP4 header → not a video). Added a two-line stderr test and a real half-truncated MP4 through probeVideo.

🤖 Addressed by Claude Code

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done in e391c67: the fix comes from the file's own header plus ffprobe's most specific line. ftyp at bytes 4–8 with moov atom not found → cut short, download again; no MP4 header → not a video. Tests feed both two-line outputs and run probeVideo on a real half-truncated MP4. Miguel's spaced-path points are in 8484f96: the unreachable missingFileFix branch is gone, measure ref one.mp4 probes the joined file, and the quote hint finds a split path anywhere in argv, including flag values.

🤖 Addressed by Claude Code

@miguel-heygen miguel-heygen 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.

Reviewed at 391e10f8381e7590c957eff99df60395e7d4911f. This adds to Jerrai's review; Jerrai's truncation finding is not repeated here, beyond why its suggested fix doesn't work on its own.

What works:

  • The probe shortcut is safe: it fires only for an existing file with nothing after it but --json (utils/projectShortcut.ts:38).
  • ffprobe's stderr is now captured instead of leaking to the terminal.
  • --from/--to are added on every op where they don't clash. face, extract and energy already use those names and are rightly skipped.

Blocker (with Jerrai): classifying from the full stderr isn't enough. I ran real ffprobe 7.0.2 on two files:

  • a truncated download (head -c 30000 a.mp4);
  • an HTML page saved as .mp4.

Both print the same two lines:

[mov,mp4,...] moov atom not found
x.mp4: Invalid data found when processing input

So scanning all of stderr for moov atom not found would only move the wrong advice to the web-page case. Telling the two apart needs the file's first bytes (media.ts:130-145):

  • ftyp at offset 4 plus a missing moov means the file is cut short: download it again;
  • a leading < or <!doctype means it's a web page.

Today, through the CLI, the truncated file gets "fix": "this is not a video: check what was saved (a web page saved as .mp4 reads like this)". The test at media.test.ts feeds one synthetic line, so it can't catch this. Please add a composed test that feeds both two-line outputs.

Important: part of the spaced-path hint can't be reached.

  • missingFileFix re-joins the words after the path (media.ts:116-125). But for every measure leaf, resolveExtraPositionals runs before run() (command-failure-tracking.ts:65), so the extra words are rejected first and probeVideo never sees a truncated path.
  • spacedPath (reject-extra-positionals.ts:49,70) only tries joins that start at the last positional. A spaced flag value therefore gets no hint: measure look "ref one.mp4" --compare my clip.mp4 prints Unexpected extra argument ...: clip.mp4 with no quote hint.
  • Either look for joins anywhere in words, or drop the missingFileFix branch that can't run.

Important: the bare form with a spaced path gets neither fix. measure ref one.mp4 (no op, unquoted) prints only Unknown command: ref: no probe and no quote hint. measureProbeShortcutArgv requires argv[at+1] to be a file, and the unknown-command hint needs a slash or an extension. The two headline mistakes overlap here, and it's likely the most common way they show up. Suggested fix: in the shortcut, if joining the following words gives an existing file, print the quote hint.

Verdict: REQUEST CHANGES
Reasoning: The fix offered for an unreadable file is wrong for the truncated-download case the PR names. Classifying from all of stderr would make it wrong for web pages instead, so it needs a file-header check. Part of the spaced-path guidance can't be reached.

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 8484f96cd00afd3697c2bd46fd3273f2aa761f12. The prior blocker is fixed: ffprobeReason retains the specific moov diagnostic, while the failed file's ftyp header distinguishes a cut-off MP4 from HTML saved as .mp4; the composed multiline and real half-truncated MP4 tests cover both. The bare spaced-path shortcut joins an existing file, explicit-operation hints inspect positionals and flag values, and the unreachable hint branch is gone. Valid explicit operation forms remain unchanged. Exact-head PR CI executed CLI shards successfully, including media.test.ts and shortcut tests; later review-event skipped jobs aren't test evidence. No new code blocker found. No merge or enqueue by reviewer.

— Jerrai

@jrusso1020 jrusso1020 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.

Reviewed at 8484f96cd00afd3697c2bd46fd3273f2aa761f12. The previous diagnostic and spaced-path findings are addressed.

measure/media.ts:136–190 selects the specific ffprobe cause and checks the actual file header before advising download versus wrong content; its descriptor closes in finally. utils/projectShortcut.ts:33–46 now joins an existing spaced file only in the no-op form, preserves explicit operations, and rejects unrelated flags. utils/reject-extra-positionals.ts:49–60 searches split paths across argv, including flag values; the dead probe-layer hint is removed. Source review addresses the spaced-path concerns raised by Jerrai and Miguel, while retaining existing measure JSON/nonzero error behavior.

Validation: Gated Vitest: 40 passed (projectShortcut/didYouMean plus an independent actual ffprobe half-MP4 versus HTML probe; only binary discovery mocked to system ffmpeg/ffprobe). A fresh independent source adversary found no additional blocker. The broader media/extra-positionals selection failed collection against borrowed dependencies missing current parser/core exports. No full build or live agent run claimed.

Need: yes · Size: ok · Reuse: existing shortcut, command dispatch and shared measure errors · Simpler: already minimal · Structure: fine (checked runMeasure, command registry and ffprobe diagnostic policy; no split)
Risk: low — additive CLI input forms and bounded diagnostics.

Verdict: APPROVE
Reasoning: The prior failures now have discriminating repairs, and valid existing operation calls retain their behavior.

— Rames / hf-review

@miguel-heygen miguel-heygen 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.

Re-review at 8484f96cd00afd3697c2bd46fd3273f2aa761f12, covering the delta since our CHANGES at 391e10f8 (e391c67, 8484f96).

Every point from the last round is resolved:

  • Cut-short MP4 vs an HTML page named .mp4. I re-ran last round's probe through the real CLI (ffprobe 7.0.2):

    • a half-truncated MP4 and a cut-short .mov → moov atom not found with "download it again";
    • the HTML page (page.mp4), a larger HTML page and a 0-byte file → the same line with "not a video";
    • an unreadable file → "check its permissions".

    The header sniff and the most-specific line (media.ts:126-171) together tell the cases apart. That is the fix Jerrai's follow-up asked for.

  • Tests. The composed two-line test (media.test.ts) feeds ffprobe's real two-line stderr through both functions for both header states. With that, the real truncated probeVideo test and the existing HTML probeVideo test, both header states are covered end to end.

  • measure ref one.mp4 now probes "ref one.mp4" and says so (projectShortcut.ts:34-43, cli.ts:209-212). --json still works.

  • The quote hint now fires for a split positional, a split flag value (--compare my clip.mp4) and a split nested path (--compare My Videos/clip one.mp4).

  • The unreachable missingFileFix branch is gone.

Tests: the changed and new tests pass. The only local failures are the ffmpeg "select of many frames" cases, which fail the same way on base on this machine. CI's full run at this head is green (Typecheck, Lint, Build, Test cli 1/2 and 2/2, CLI smoke). Its later all-skipped re-trigger run is what gh pr checks shows.

Nits (non-blocking):

  • reject-extra-positionals.ts:48-58: the comment says the hint takes the longest run that is a file, but the code returns the first (shortest) join that exists, including a directory. Probe: with a folder Final Cut/ beside Final Cut export.mp4, measure curve a.mp4 --compare Final Cut export.mp4 says 'Final Cut' is one path with spaces. Fix: from each start, keep the last join that statSync(...).isFile() accepts.
  • --compare=my clip.mp4 (the = form) gets no hint, because the joined word keeps its --compare= prefix. Stripping ^--[^=]+= before existsSync would cover it.

Verdict: APPROVE
Reasoning: Both two-line ffprobe cases now get the right fix, and both are verified against the real CLI. Every spaced-path case from the last round is handled, and CI is green.

@WaterrrForever
WaterrrForever added this pull request to the merge queue Nov 9, 2026
Merged via the queue into main with commit eb0a52c Nov 9, 2026
132 checks passed
@WaterrrForever
WaterrrForever deleted the miao/rav-measure-ux branch November 9, 2026 14:59
WaterrrForever added a commit that referenced this pull request Nov 9, 2026
…lue; telemetry docs name the reverse session id

Follow-ups from review on #7443 and #7444:
- The quote hint returned the first run of words that existed, so a
  "Final Cut/" folder hid "Final Cut export.mp4". It now names the longest.
- "--compare=my clip.mp4" got no hint; the value after = now starts a run.
- The telemetry section lists the skill a reverse or templates command ran
  for, and the HeyGen session id a recorded reverse run carries.
cursor Bot pushed a commit to AmroAlemam/hyperframes that referenced this pull request Nov 9, 2026
…lue; telemetry docs name the reverse session id (heygen-com#7452)

Follow-ups from review on heygen-com#7443 and heygen-com#7444:
- The quote hint returned the first run of words that existed, so a
  "Final Cut/" folder hid "Final Cut export.mp4". It now names the longest.
- "--compare=my clip.mp4" got no hint; the value after = now starts a run.
- The telemetry section lists the skill a reverse or templates command ran
  for, and the HeyGen session id a recorded reverse run carries.
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 9, 2026
…n-com#7110, heygen-com#7000; advance watermark

Eight commits, PR heygen-com#7452 and ten pin moves are recorded with their evidence
in the continued ledger, DECISIONS and UPSTREAM. Adopted: heygen-com#7249 (template
entries detected structurally), heygen-com#7110 (whisper nan/inf output), heygen-com#7000
(engine probe test on fake timers). Deferred: heygen-com#7444, heygen-com#7452. Not applicable:
heygen-com#7257, heygen-com#7443, heygen-com#7450. The watermark moves to c149730, PR heygen-com#7452, and the
nine moved pins plus heygen-com#7434 are re-pinned at their current heads.

Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
@miguel-heygen miguel-heygen mentioned this pull request Nov 9, 2026
3 of 4 tasks
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