fix(skills): consolidate animation-map capture reliability

[miguel-heygen]

Summary

• pass rational frame-rate values to animation-map and contrast capture helpers
• batch animation-map sampling into one browser evaluation to avoid per-sample timeout overhead
• add regression coverage for both capture contracts

Consolidates and replaces #2339 and #2329.

Verification

• node --test skills/hyperframes-animation/scripts/animation-map.test.mjs skills/hyperframes-animation/scripts/animation-map-sampling.test.mjs (3 passed)
• skills manifest regenerated and in sync
• git diff --check

[vanceingalls]

Lens: runtime-interop + cross-layer contract on the fps rational shape and the new sampling batch boundary. Rames has test + observability. Adversarial-user pass run in a fresh sub-agent (Rule 15 trigger: multi-component contract). Head 2d5114b.

Strengths

• animation-map-sampling.mjs:5-33 cleanly hoists the seek+bbox+style samples into one page.evaluate per tween. The missing-element shape ({ t, x:0, y:0, w:0, h:0, missing:true } — no opacity/visible) is preserved byte-identically from the removed measureTarget, so computeFlags, describeTween, and markCollisions see no divergence.
• animation-map.test.mjs:50 pins fps: {num, den} via a producer stub that throws with the options JSON — exactly the shape the #2329 bug turned into NaN at beginFrameIntervalMs, now covered by regression.
• Shape change reaches both consumers (animation-map.mjs:55, contrast-report.mjs:85) with no drift.

Blockers — none.

Important

• animation-map.mjs:44 and contrast-report.mjs:74 — CLI silently mis-shapes rational fps inputs. Number(args.fps ?? 30) maps --fps 30000/1001 → NaN → {num: NaN, den: 1} handed to the producer; --fps 29.97 → {num: 29.97, den: 1} (non-integer numerator). The producer's own canonical parser in packages/core/src/core.types.ts:86 (parseFps) rejects both cases explicitly (not-a-number and ambiguous-decimal) — this PR reinvents fps parsing at the CLI boundary and drops the guarantees the rational shape exists to give. The PR body says "pass Producer capture helpers the rational FPS shape required by the current engine contract"; the CLI parser only supports {num: integer, den: 1}. Minimum fix: gate with Number.isFinite(FPS) && Number.isInteger(FPS) && FPS > 0 and reject with a clear error. Ideal fix: bootstrap @hyperframes/core alongside @hyperframes/producer and use parseFps/toFps — the package-loader already supports arbitrary npm-pinned specs.

Nits / Notes

• animation-map.mjs:39-43 — same silent-NaN class for --frames, --min-duration, --width, --height. Pre-existing, but sampleTweenBboxes amplifies impact: Number("abc") = NaN → Array.from({length: NaN}, …) = [] → bboxes = [], and computeFlags's .every returns vacuously true, tagging every tween degenerate + invisible. Not a regression from this PR, but the new hot path makes the failure mode louder.
• animation-map-sampling.test.mjs only exercises window.__hf.seek; the window.__timelines fallback in animation-map-sampling.mjs:14-18 has no coverage. Since this file is new, a fair time to add the second-branch test.
• animation-map.mjs:143 — seekTo (still used by captureSnapshots, line 501) retains its await setTimeout(_, 100). The batching insight that motivated the removal ("GSAP/HyperFrames seeks update DOM state synchronously") applies here too — 500ms of pure wall clock across the 5 snapshots. Follow-up, not in-scope.
• contrast-report.mjs:275 — prepareTextElements sits outside the try/finally that calls restoreTextElements. If the page-side walker throws mid-hide, window.__contrastReportRestores holds the partial hide list but nobody restores. Pre-existing.

Absorbed-PR verification

• #2339 (batched sampling): new sampleTweenBboxes is called once per tween in animation-map.mjs:81; old inline seek+measure loop removed cleanly. Shape preserved. ✓
• #2329 (rational fps shape): fps: {num: FPS, den: 1} reaches both animation-map.mjs:55 and contrast-report.mjs:85. ✓
• Neither source PR had substantive review comments to carry forward (both closed with 0 reviews).

Audited: all 6 changed files end-to-end. Trusting: skills-manifest.json (regenerated).

Verdict: COMMENT
Reasoning: The consolidation preserves both source PRs' shapes and the sampling refactor is clean, but the CLI fps parser silently mis-shapes the exact rational inputs the rational-fps shape exists to enable. Not blocking (integer --fps 30 works), but the PR body's own thesis is only half-delivered.

— Via

[james-russo-rames-d-jusso]

Reviewed at 2d5114b2eaf5247c7366bd05f9bc151d6259e415 — consolidates #2339 (rational fps) + #2329 (batched sampling).

Two orthogonal wins on the animation-map capture path

1. Rational-fps contract compliance — createCaptureSession's CaptureOptions.fps requires Fps = { num, den } (see engine/src/types.ts:86-93, core/src/core.types.ts:18-21). Both animation-map.mjs:55 and contrast-report.mjs:85 were passing the scalar FPS; now both pass { num: FPS, den: 1 }. Skills aren't typechecked so this had silently gone wrong until #2339 surfaced the mis-shape downstream.
2. Batched tween sampling — sampleTweenBboxes(page, selector, times) in the new animation-map-sampling.mjs collapses N (seek+measure) CDP round trips + N × 100 ms sleeps into ONE page.evaluate that loops in the browser. The comment names the invariant: "GSAP/HyperFrames seeks update DOM state synchronously." The old seekTo at animation-map.mjs:130-145 had a hardcoded 100 ms settle sleep after every seek — a Ntimes latency multiplier on longer tween grids.

Verification

• sampleTweenBboxes behavior — animation-map-sampling.test.mjs:5-48 mocks a page whose evaluate records payloads, then asserts (a) the returned samples map each time to a distinct bbox, (b) seek was called in order for [1, 2, 3], (c) ONE payload was passed to evaluate (calls == [{selector, times: [1,2,3]}]). Load-bearing for the "one round trip, N samples" invariant.
• Rational-fps regression test — animation-map.test.mjs:12-59 spawns each helper (animation-map.mjs AND contrast-report.mjs) with a mocked @hyperframes/producer whose createCaptureSession throws with the JSON-encoded options in the error message. Asserts stderr matches /CAPTURE_OPTIONS=.*"fps":\{"num":24,"den":1\}/. Both scripts covered by the same parameterized test — good.
• Seek fallback chain — sampleTweenBboxes prefers window.__hf.seek(time), falls back to iterating Object.values(window.__timelines) and calling .seek(time) on each with a function-type guard. Matches the pre-PR seekTo semantics.
• skills-manifest.json — hashes rewritten for hyperframes-animation (files 99→102, matches +3 new .mjs files) and hyperframes-creative (hash-only). Consistent with the diff.

Adversarial pass

• 100 ms sleep removed with no replacement — old seekTo did await new Promise((r) => setTimeout(r, 100)) after each seek. The new batched code has no equivalent settle. Safe for GSAP (whose .seek() writes to element.style synchronously), but any compositions relying on Web Animations API, CSS transitions, or requestAnimationFrame-scheduled visual updates would sample pre-settle. Realism: low — animation-map is a diagnostic tool used against GSAP-authored compositions. Worth naming here so a future "why did my CSS-transition composition suddenly measure wrong?" ticket points here.
• page.evaluate payload serialization — { selector, times } is a plain object with string + number[]. Serializes via structured clone cleanly.
• Math.round on rect components — same as the pre-PR measureTarget; preserves 1px quantization behavior. Callers reading the tween grid should be tolerant of ±1px per sample.
• querySelector inside the browser loop — resolves once per sample. If the target is added/removed mid-tween (unusual), later samples would be {missing: true}. Old code had the same shape.
• opacity parsed via parseFloat(style.opacity) — old code used the same expression. Consistent.
• Empty times array — returns []. Both code paths handle it.

Verdict framing

Two well-scoped skill fixes, both covered by executable regression tests. LGTM from my side.

— Review by Rames D Jusso

[miguel-heygen]

Resolved Via’s rational-FPS parser finding at current head 49d8d8ea5.

Both animation-map.mjs and contrast-report.mjs now bootstrap @hyperframes/core alongside producer and call the canonical parseFps. Exact 30000/1001 reaches capture as {num:30000,den:1001}; ambiguous 29.97 fails before capture with a structured reason. The duplicated scalar Number(...) parser is removed.

TDD evidence: both helper regressions failed first with {num:null,den:1}. Verification now passes 9/9 animation/creative skill tests, oxfmt, oxlint, and manifest sync. CI is rerunning; please re-review current head once required checks settle.

[james-russo-rames-d-jusso]

Reviewed at 49d8d8ea54e9cb2a82b08fafefe63240542c8f2d — delta from R1 2d5114b2 resolves Via's rational-FPS finding.

R2 delta verified

• parseFps from @hyperframes/core now handles the FPS input in both animation-map.mjs:47-49 and contrast-report.mjs:82-84. The R1 shape { num: Number(args.fps), den: 1 } silently converted NTSC input 30000/1001 to {num: 30001001, den: 1} (via Number("30000/1001") → NaN) or 29.97 decimal to {num: 29.97, den: 1} — both wrong. R2 preserves exact rationals AND fails hard on ambiguous decimals before capture starts.
• importPackagesOrBootstrap now includes @hyperframes/core — added to the bootstrap manifest so the skill helper can import { parseFps } even from an unbootstrapped project.
• Bootstrap consolidated into a single call in animation-map.mjs:22-28 (previously producer-only, now both packages). Same shape in contrast-report.mjs:52-61.

Verification

• Happy path (NTSC rational) — test asserts "fps":{"num":30000,"den":1001} when spawned with --fps 30000/1001. Load-bearing.
• Rejection path (ambiguous decimal) — new invocation with --fps 29.97 asserts (a) exit != 0, (b) stderr matches /Invalid --fps "29\.97": ambiguous-decimal/, (c) CAPTURE_OPTIONS= marker never appears (session never opened). Both invariants covered.
• Both helpers covered — parameterized describe block runs the same suite over animation-map.mjs AND contrast-report.mjs at animation-map.test.mjs:14-16.
• parseFps mock in the test is a shape-shim: returns {ok, value} or {ok: false, reason} matching the real contract from @hyperframes/core. Real parseFps lives in the core package — I trust its published test coverage.

Adversarial pass

• args.fps ?? 30 — the default is still the integer 30 (which the real parseFps should parse as {num: 30, den: 1} — the same shape as the R1 hand-wrap for integer input). No regression for the common no-flag case.
• Error surface — die(\Invalid --fps ...`)fails beforecreateCaptureSessionopens, so no browser/server resources leak.die` semantic preserved.
• Test hash refresh in skills-manifest.json — both hyperframes-animation and hyperframes-creative bumped correctly to match the diff.

Verdict framing

Clean R2 — NTSC and other rationals now flow through unchanged, ambiguous decimals rejected loudly. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

R2 re-review at 49d8d8ea5 — targeted at my R1 rational-FPS finding. Rames covered the same delta in his R2; this is the runtime-interop + adversarial-user lens on top.

R1 finding resolution — verified

• Both sites now import parseFps from @hyperframes/core via the shared bootstrap and use it as the fps entry point: animation-map.mjs:22-31, 47-49, contrast-report.mjs:52-64, 82-84. Raw Number(args.fps) is gone at both sites; parse failure short-circuits with die(\Invalid --fps "…": `)beforecreateCaptureSession` opens.
• importPackagesOrBootstrap now carries @hyperframes/producer + @hyperframes/core (+ sharp for contrast-report), so global skill installs get both packages bootstrapped in one manifest — no partial-install shape where the CLI reaches for parseFps and misses it.
• Test coverage lives in animation-map.test.mjs, parameterized across both helpers via the HELPERS array (animation-map.test.mjs:10-16). Two invocations per helper: --fps 30000/1001 asserts CAPTURE_OPTIONS=.*"fps":\{"num":30000,"den":1001\}; --fps 29.97 asserts non-zero exit, stderr matches /Invalid --fps "29\.97": ambiguous-decimal/, and — load-bearing — CAPTURE_OPTIONS= marker never appears (session never opened on the ambiguous-decimal path).

Trace against packages/core/src/core.types.ts parseFps

Walked the canonical parser against the input classes I originally flagged, plus adversarial extensions:

• --fps 30000/1001 → integer/integer, den>0, decimal 29.97… in [1,240] → {num: 30000, den: 1001}. ✓
• --fps 24000/1001 → same shape, decimal 23.97… in range → {num: 24000, den: 1001}. ✓
• --fps 24|30|60 → integer-regex path → {num, den:1}. ✓
• --fps 29.97 → decimal-regex match → ambiguous-decimal. ✓
• --fps 23.976 → same → ambiguous-decimal. ✓
• --fps foo → neither regex matches → not-a-number. ✓
• --fps 30/ → split ["30",""]; Number("")===0, integer, but den<=0 → invalid-fraction. ✓
• --fps /1001 → ["", "1001"]; Number("")===0 → num<=0 → non-positive. ✓
• --fps 30000/0 → den<=0 → invalid-fraction. ✓
• --fps -30 → integer regex matches, n===-30, n<=0 → non-positive. ✓
• Bounds guard: --fps 480 → out-of-range. ✓ (defensive >240 cap on both integer and rational paths.)

Every one of these fails at the CLI boundary with a structured reason, before any server or session is opened. die() prints helper: Invalid --fps "<raw>": <reason> and exits 2 — actionable and consistent.

Adversarial pass

Fresh-lens pass on constructed slip-past scenarios against parseFps itself:

• --fps 30.0/1001 — Number("30.0")===30, Number.isInteger(30)===true; passes integer check with num=30, den=1001; decimal ≈0.03 fails the >=1 bound → out-of-range. Fails cleanly.
• --fps 30000/1001.0 — semantically identical to 30000/1001; both parts integer-valued, so passes. Not a bug.
• --fps 30000/1001/2 — split length !== 2 → invalid-fraction. ✓
• --fps 1e20/1e20 — both parts integer-valued (Number.isInteger(1e20)===true); decimal ratio is 1; passes as {num:1e20, den:1e20}. Constructed pathological input; downstream FFmpeg would emit -r 1e+20/1e+20 which won't parse. Not a realistic user path, and no upstream parseFps caller in this PR feeds this shape. Flagged only as an observation for whoever owns the core parser.
• --fps Infinity / --fps NaN (as string) → neither regex matches → not-a-number. ✓
• --fps with no value → parseArgs yields undefined, args.fps ?? 30 defaults to integer 30, which parses as {num:30, den:1}. No silent-NaN.
• Verified the parseFpsWithDefault behavior isn't relied on here — both helpers explicitly ?? 30 before calling parseFps, which is fine (30 is an integer literal, parses cleanly).

Couldn't construct a slip-past against parseFps that produces a silent-NaN or non-integer-numerator downstream. R1's important is fully resolved.

Adjacent gap — still open, nit severity

The sibling Number() boundaries I flagged as a nit at R1 are unchanged at R2:

• animation-map.mjs:34-38 — FRAMES, MIN_DUR, WIDTH, HEIGHT still Number(args.<name> ?? default). Concrete misuse: --frames foo → Array.from({length: NaN}, …) yields [], so sampleTweenBboxes never fires and the tween silently records bboxes: []; computeFlags's .every returns vacuously true and every tween gets tagged both degenerate and invisible. --width foo → NaN passed as viewport dimension to createCaptureSession.
• contrast-report.mjs:79-81 — same for SAMPLES, WIDTH, HEIGHT. --samples foo produces an empty times array, no samples are taken, and the JSON report writes entries: [] + summary.total: 0 with exit code 0 (misleading pass).

Pre-existing at both sites; not a regression from this PR. Same failure class as the R1 finding — worth a small follow-up that either routes these through Number.isFinite + Number.isInteger guards or a shared parsePositiveInt/parsePositiveNumber helper next to parseFps. Nit severity.

Notes / observations (not gating)

• animation-map.mjs:317 (seekTo used by captureSnapshots) still holds the 100 ms settle sleep. Batch removed the sleep from tween sampling; the snapshot loop kept it. Consistent with what Rames noted at R1 about GSAP synchronous seek — potential future cleanup, not in-scope.
• The parseFps mock in animation-map.test.mjs:38-45 is a shape-shim, not a fidelity clone. That's fine — the test's job is to pin the CLI plumbing (calls parseFps, gates on .ok, threads .value to createCaptureSession, fails-fast on .ok===false); the real parser's semantics are covered by @hyperframes/core's own tests. Load-bearing separation.

Audited: both .mjs files end-to-end at 49d8d8ea5, parseFps in core.types.ts:86, both test bodies, both bootstrap manifests. Trusting: skills-manifest.json hash rewrites (Rames confirmed file counts).

Verdict: COMMENT (post-merge: n/a — PR open)
Reasoning: R1's rational-FPS silent-NaN finding is fully resolved at both sites; adversarial couldn't slip past parseFps. Flagging the sibling Number() boundaries on --frames/--width/--height/--samples as a follow-up nit — same failure class, pre-existing, not a regression here.

— Via