Repository navigation
fix(cli): measure cuts reads a zoom that speeds up as a move, not a cut - #7075
Conversation
Bug bash (Stripe "pay", 0.9.6): the app window's zoom-in at 0.2 s changes 3, 9, 12, 5 grey levels a frame, and the 12 crossed a cut's level, so measure cuts listed a cut there and diff failed the rebuild (pass: false at 0.93 similarity) for missing it. A single-frame cut is now a fast move when the frames either side move too (30% of its change) and the frame before, zoomed 0.8-1.25x and shifted a few pixels, comes within 40% of its change of the frame after. A punch-in also zooms, but out of a still picture, so it stays a cut.
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 9263515ed97af549aa4c138dba22c63c74d17f50.
Important — motion on only one side can hide a real punch-in/out edit (packages/cli/src/measure/cuts.ts:130). Math.max(preChange, postChange) allows the adjacent shot's movement to satisfy “already moving,” even when the picture before the edit was held still, or the picture after it is held still. This contradicts the promised preservation of punch-ins from stillness.
Using the PR's own window signature fixture, 13 held width-20 frames → width 25 → 16 held width-27 frames loses the abrupt boundary at frame 13 as fast (boundary changes: 0, 18.046875, 8.689236, 0). The mirrored sequence loses the punch-out at frame 17. Both independent assertions pass with parent production code and fail at this head. A lossless FFV1 encode/decode corroborates the first case. findCuts removes these fast candidates, so the effect reaches measure cuts, diff's reference cuts (diff.ts:485) and the generation cut guard (heygen/generate.ts:336).
Fix: require motion on both sides before treating geometric similarity as continuous movement, e.g. use Math.min for this neighbor threshold, or another guard that preserves both hard-cut edges. The one-line min control passes both witnesses plus all official cut tests (19/19), including the new Stripe zoom test. Keep both punch-in and punch-out regressions; requiring only prior motion would leave the mirrored case.
Validation: full workspace build succeeds. The five official cuts/diff/diff-segments/diff-people/generate suites → 91/91 passed; the new zoom regression fails with parent classifier (1 failed / 16 passed). Exact-head independent witnesses → 2 failed, parent → 2 passed, both-neighbor control → 19 passed. Root reviewer reproduced both exact-head failures. The 33-film benchmark and any live/paid generation were not rerun; synthetic signatures and an encoded control establish this finding.
Need: yes · Size: ok · Reuse: existing cut classifier and fixtures · Simpler: guard both sides of the existing geometric check · Structure: fine
Risk: medium — a missed cut also weakens the pre-generation cut guard.
Verdict: REQUEST CHANGES
Reasoning: The zoom false positive is fixed, but the one-sided motion test now suppresses genuine cut edges beside moving footage. Preserve both stationary-edge cases before approval.
— hf-review
Review (hf-review): motion on one side hid a real punch-in out of a still picture next to a moving shot, and the mirrored punch-out. isMove now needs both neighbours moving (min, not max). The 33-film check now changes only Stripe's 0.2 s zoom; league-promo 95.93 s and M1000 5.43 s are cuts again.
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 1d12321f26f55b39618249bb373667baf9f493d8.
Both concerns from review 5749399568 are resolved: isMove now requires motion on both sides (packages/cli/src/measure/cuts.ts:130), preserving the held→cut→moving punch-in and moving→cut→held punch-out. The new tests pin both edges (cuts.test.ts:148, cuts.test.ts:153); the Stripe-style continuous zoom still classifies as fast.
Validation: bun install and full root bun run build succeed. Five official cuts/diff/diff-segments/diff-people/generate suites plus five independent adversarial cases: 98/98 passed (93 official and 5 independent). The independent tests cover both FFV1 encode/decode witnesses through actual generate() refusal with zero fetches, continuous reverse zoom, and first/last-frame cut boundaries. The five independent controls were rerun by the root reviewer with bun run --cwd packages/cli test src/measure/review7075.test.ts (review scratch, not committed). Restoring Math.max makes four assertions fail: both committed edge regressions and both decoded witnesses; the exact tracked source was then restored. The paid path calls checkRequest before runJob, so preserved guide cuts are refused before submission. The 33-film corpus, original Stripe clip and live/paid generation were not rerun.
Need: yes · Size: ok · Reuse: existing cut classifier and fixtures · Simpler: existing two-sided guard is minimal · Structure: fine
Risk: medium — heuristic cut classification also feeds the pre-generation guide guard.
Verdict: APPROVE
Reasoning: Both prior cut-loss regressions are fixed at this head, with no further Important finding in the delta or the tested neighboring cases.
— hf-review
What
From the 11-02 bug bash (Miguel's "yours to fix" #1): on the Stripe "pay" video, the app window's zoom-in at 0.2 s changes 3, 9, 12, 5 grey levels a frame. The 12 crossed a cut's level, so
measure cutslisted a cut at 0.2 s, anddifffailed the rebuild (pass: falseat 0.93 similarity) because it had no cut there.A cut of up to 3 frames is now marked
fast(listed innot_cuts, left out ofdiff's reference cuts) when both hold:A punch-in also zooms, but out of a still picture, so it stays a cut.
Test plan
fails without the fix:
cuts.test.ts"calls a zoom that speeds up past a cut's level a fast move"src/measure+src/commandstests, typecheck, oxlint/oxfmt. The one failure,lambda/deploy.test.ts"warms it once by default", fails on main too.