Repository navigation
fix(cli): measure answers the four mistakes agents make most - #7443
Conversation
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.
jrusso1020
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
| /** 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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/--toare added on every op where they don't clash.face,extractandenergyalready 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):
ftypat offset 4 plus a missing moov means the file is cut short: download it again;- a leading
<or<!doctypemeans 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.
missingFileFixre-joins the words after the path (media.ts:116-125). But for every measure leaf,resolveExtraPositionalsruns beforerun()(command-failure-tracking.ts:65), so the extra words are rejected first andprobeVideonever 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.mp4printsUnexpected extra argument ...: clip.mp4with no quote hint.- Either look for joins anywhere in
words, or drop themissingFileFixbranch 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.
…r and ffprobe's specific line
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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 foundwith "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. - a half-truncated MP4 and a cut-short .mov →
-
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 truncatedprobeVideotest and the existing HTMLprobeVideotest, both header states are covered end to end. -
measure ref one.mp4now probes"ref one.mp4"and says so (projectShortcut.ts:34-43,cli.ts:209-212).--jsonstill 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
missingFileFixbranch 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 folderFinal Cut/besideFinal Cut export.mp4,measure curve a.mp4 --compare Final Cut export.mp4says'Final Cut' is one path with spaces. Fix: from each start, keep the last join thatstatSync(...).isFile()accepts.--compare=my clip.mp4(the=form) gets no hint, because the joined word keeps its--compare=prefix. Stripping^--[^=]+=beforeexistsSyncwould 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.
…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.
…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.
…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>
Why
Agents most often get
hyperframes measurewrong in three ways:measure ref.mp4);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, runsprobeand 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)..mp4print the same two lines, so the file's header decides between them:ftypat bytes 4–8 means the file is cut short: download it again;--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, andprobeVideoon a real half-truncated MP4.look a.mp4 --compare <spaced path unquoted>prints the quote hint;cuts --from 0.5 --to 2measures that window;.mp4gets the "not a video" fix.