feat(cli): declarative motion verification in inspect (#1437)#1459
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
1de866f to
6f0a607
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verified at HEAD 6f0a607.
Reviewed against the canonical engineering rubric (CLI ergonomics, code health, test coverage, schema/forward-compat, security/output stability). HF runtime-interop semantics (__hf / __player / __timelines lookups, seek-pause contract, sampler's interaction with the rendered timeline) are explicitly deferred to Vai's parallel pass.
Net read: well-shaped feature, clean lane separation (browser samples / Node decides), good test coverage on the pure evaluator. Two real concerns worth addressing before merge, plus a handful of smaller items.
Concerns
1. Multi-match selectors silently sample only the first element. packages/cli/src/commands/motion-sample.browser.js:100 does document.querySelector(selector) and the inline comment ("first match only; assertions name single elements by design") asserts a single-element contract — but docs/packages/cli.mdx:594 advertises .card (class selector) as an example assertion target, and staysInFrame(".card") reads naturally as "no .card drifts off-frame." With the current impl, if there are 3 .cards and only the second drifts off-canvas, the run silently passes. Two reasonable fixes — pick one and document it: either (a) reject multi-match selectors at parse time / sample time with a clear motion_selector_ambiguous finding so the author has to disambiguate (.card:nth-of-type(2)), or (b) iterate all matches and fail if any match violates the assertion. Today's "first match wins" silently under-reports and is the trap class this PR is specifically trying to close.
2. No version field on the motion sidecar — strict unknown assertion kind rejection means a forward-evolving spec breaks old CLIs hard. packages/cli/src/utils/motionSpec.ts:70 rejects unknown kinds outright. That's the right default (typo'd appearBy shouldn't silently no-op), but with no version field on the sidecar and the PR body already listing future assertion kinds as deferred, a v2 spec that adds onBeat will hard-fail on every pre-rollout CLI. Suggest adding an optional version: 1 field now (validated when present, defaulted when absent) so future authors can opt their spec into "best-effort, warn on unknown kinds for kinds added after the CLI's version." Cheap to add now, painful to bolt on later. The JSON envelope's schemaVersion: 1 (packages/cli/src/commands/layout.ts:27) is a separate concept from the sidecar's authoring schema — both deserve to exist.
Nits
packages/cli/src/utils/motionAudit.ts:128—staysInFrameconsiders a sample "visible" ifsample.visibleis true, butisVisibleElementin the browser sampler accepts opacity down to 0.2, whileappearsByuses the stricterAPPEAR_OPACITY = 0.5gate. So an element canappearByat t=0.5s (opacity 0.6) then drop to opacity 0.3 at t=1.5s while drifting off-canvas, andstaysInFramewill still fire. Defensible (the box really is off-screen and partially visible), but worth either gatingstaysInFrameon the sameAPPEAR_OPACITYthreshold or documenting "once visible" as opacity ≥ 0.2 to match the implementation. (nit)packages/cli/src/commands/motion-sample.browser.js:79—root.querySelectorAll("*")per liveness frame is O(DOM size) × 300 samples in the worst case. Plausibly fine for typical compositions; flagging in case anyone wires this into a heavy-DOM stress test. Could cap withroot.querySelectorAll("*[data-hf-track], *[id], *[class]")or similar if it ever bites. (nit)packages/cli/src/utils/motionSpec.ts:78—durationvalidation rejects<= 0butparseMotionSpecacceptsNaNif it slips pasttypeof === "number"... actuallyNaN <= 0isfalse, soNaNwould pass the duration check. Tiny edge but worth addingNumber.isFinitelike theisPositivehelper already does formaxStaticSec. (nit)packages/cli/src/utils/motionAudit.ts:181—lastTime = frame.timereassigned every loop iteration; pulling it outside the loop tail (or just usingframes[frames.length - 1].time) is microscopically clearer. (nit)findMotionSpecties tiebreak to "first matching html basename" then "first alphabetical" — solid for single-composition projects, but a project with two compositions (hero.html+landing.html) plus two sidecars (hero.motion.json+landing.motion.json) will pick whichever sorts first alphabetically. The deferred--assert <path>flag mentioned in the PR body handles this; just worth being explicit in the docs that "one composition per project for motion verification today." (nit)
Questions
- The JSON envelope adds
motionSpec(path) andmotionSamples(count) fields conditionally without bumpingschemaVersion. Additive is generally safe for consumers using property existence, but if anything downstream (CI parser, agent loop) does strict schema validation, this is a quiet break. Intentional to keepschemaVersion: 1? If so, worth a comment near the constant about "additive-only" policy. - Tests cover the four evaluator kinds and the in-page sampler via happy-dom, but I didn't spot a test for
resolveMotionSpec's exit-1-with-clean-error path on an invalid sidecar — the PR body claims it's e2e-verified, just confirming this isn't unit-tested intentionally (since itprocess.exits). keepsMovinguses"*"as the magic "whole canvas" scope key. If a user writes a literal CSS universal-selector assertion (unlikely, butkeepsMoving({ withinSelector: "*" })), the parse accepts it and it'll collide with the default scope. Probably fine to leave but worth knowing.
What I didn't verify
- HF runtime-interop semantics — whether the sampler's
seekTo+ paused-timeline contract correctly reflects what the renderer sees, whether the liveness signature matches the renderer's notion of "moving," and whether__timelines/__player/__hffallback chain is the right one for current HF versions. Deferred to Vai's pass. - I did not run the full CLI test suite locally; PR body claims 835/835 green and the fallow gate passes.
- Did not exercise against a real composition end-to-end; reviewed semantics only from the diff.
Review by Rames D Jusso
6f0a607 to
83fddbb
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verified at HEAD 83fddbb.
R2 verification against R1 at 6f0a607. Rebased onto main (R1 commit replaced wholesale by a single squashed commit at the new tip), but the substantive code changes are clearly authored against the R1 feedback. Net: approve-with-concerns → approve. Both concerns resolved, three of four nits resolved, one question genuinely resolved by design choice. CLI-rubric lane only (Vai owns the HF runtime-interop pass).
R1 verification table
| R1 item | Status | Evidence |
|---|---|---|
| Concern 1 — multi-match selector silently first-match-only | ✅ resolved | packages/cli/src/commands/layout.ts:358-376 — findAmbiguousSelectors runs before sampling, calls document.querySelectorAll(sel).length > 1, returns motion_selector_ambiguous findings via packages/cli/src/utils/motionAudit.ts:35-46 with an actionable fixHint ("use #id or :nth-child()"). Picked option (a) from the R1 suggestion (reject) rather than (b) (iterate); reasonable for a "claim names one element" contract. Pass aborts before frame collection when ambiguous selectors are present, so no wasted seek work. |
Concern 2 — no version field, unknown-kind hard reject breaks old CLIs |
✅ resolved (alternative) | packages/cli/src/utils/motionSpec.ts:18 adds version?: number; packages/cli/src/utils/motionSpec.ts:85-89 rejects unsupported versions with a clear "spec version N is not supported — upgrade the hyperframes CLI" message. Resolution is "fail loudly with an actionable upgrade message" rather than the "warn-on-unknown-kinds tolerance" I suggested — but the operational goal (old CLI doesn't silently misread future specs, and the failure mode now has a clear remediation) is met. Per feedback_open_item_alternative_resolution, crediting as resolved-differently. Tested at packages/cli/src/utils/motionSpec.test.ts:79-86. |
Nit 1 — appearsBy 0.5 vs staysInFrame 0.2 opacity threshold |
packages/cli/src/utils/motionAudit.ts:5 defines APPEAR_OPACITY = 0.5 and uses it in firstAppear. staysInFrame at packages/cli/src/utils/motionAudit.ts:145-148 still gates on sample.visible (the sampler's 0.2-threshold field), not the 0.5 appear threshold. Code is unchanged from R1; docs at docs/packages/cli.mdx:603 still describe staysInFrame as "once visible" without disambiguating the threshold. Defensible (the box really is partially visible) but the R1 ask was either align thresholds or document — neither was done. Low-stakes, drop if intentional. |
|
Nit 2 — querySelectorAll("*") per-frame cost |
❌ still-open | packages/cli/src/commands/motion-sample.browser.js:78-86 still iterates root.querySelectorAll("*") per liveness frame. Acknowledged as low-priority in R1; safe to defer. Worth a comment near the loop ("O(DOM) × MOTION_MAX_SAMPLES; revisit if heavy-DOM compositions hit it") so future maintainers know the cost shape. |
Nit 3 — NaN slipping past duration check |
✅ resolved | packages/cli/src/utils/motionSpec.ts:94-95 now guards with !Number.isFinite(raw.duration). Test at packages/cli/src/utils/motionSpec.test.ts:88-95 exercises duration: NaN. |
| Nit 4 — multi-composition alphabetical tiebreak ambiguity | findMotionSpec logic at packages/cli/src/utils/motionSpec.ts:115-126 unchanged — still falls back to first-alphabetical when no html basename matches. Docs at docs/packages/cli.mdx:582-606 don't mention the "one composition per project" caveat. The new test at packages/cli/src/utils/motionSpec.test.ts:113-121 exercises the html-basename-match path but not the alphabetical-tiebreak path. Minor; the deferred --assert <path> will subsume this. |
|
Question 1 — schemaVersion not bumped for additive fields |
✅ resolved (by design) | packages/cli/src/commands/layout.ts:31 keeps INSPECT_SCHEMA_VERSION = 1; new motionSpec/motionSamples fields added conditionally at packages/cli/src/commands/layout.ts:581-582 (only emitted when a spec is present). Treating as resolved-by-intent (additive-only is a defensible JSON-envelope policy), but no explanatory comment was added near INSPECT_SCHEMA_VERSION documenting the policy — a one-line "// additive-only; bump on breaking changes" would future-proof against the next contributor. |
Question 2 — no test for resolveMotionSpec exit-1 path |
❌ still-open | resolveMotionSpec at packages/cli/src/commands/layout.ts:411-437 still process.exit(1)s on parse failure; not unit-tested. Defensible (process.exit is hostile to vitest) — the PR body claims end-to-end coverage. Worth refactoring to a "render error envelope" pure function + a thin process.exit wrapper, but a non-blocking R3 follow-up. |
Question 3 — "*" magic scope collision with literal universal selector |
✅ resolved | packages/cli/src/utils/motionSpec.ts:65-66 now explicitly rejects withinSelector: "*" at parse time with a clear error pointing the user at "omit it for whole-composition liveness." Tested at packages/cli/src/utils/motionSpec.test.ts:97-103. Clean fix. |
New since R1
findAmbiguousSelectors(packages/cli/src/commands/layout.ts:343-359) runsevaluatewith a try/catch swallow for invalid selectors. Not new behavior —safeQueryin the browser sampler has the same try/catch — but means a malformed selector (e.g.:where(:not(mid-string) silently skips the ambiguity check rather than raising. Low-stakes: it'd flow through tosafeQuery→ null →motion_selector_missing, so the user still sees a finding. Not a blocker.- The
runMotionPassshort-circuits onambiguous.length > 0and returnssampleCount: 0(packages/cli/src/commands/layout.ts:447). Means the JSON envelope reportsmotionSamples: 0when any selector is ambiguous, which is honest — no samples were taken. Fine. _examples.tsand the inspect/layout description copy mention "motion verification" — good discoverability.
Verdict
approve at HEAD 83fddbb. Both R1 concerns are resolved (concern 2 in spirit if not in shape, which is the right call). Remaining items are minor — nit 1 opacity-threshold consistency and the schemaVersion policy comment are the highest-value drop-ins if you want a tidy R3, but nothing here blocks merge.
Review by Rames D Jusso
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verified delta at HEAD 83fddbb, R2 baseline was 83fddbb — no new commits pushed since R2. Re-verifying the four R2-open items at the same SHA per Miguel's "review again."
R2-item status
-
Nit 1 —
staysInFramegates on 0.2-thresholdsample.visiblevsappearsBy's 0.5 — ❌ still-openpackages/cli/src/utils/motionAudit.ts:5definesAPPEAR_OPACITY = 0.5;appearsBy(L61) usessample.visible && sample.opacity >= APPEAR_OPACITY.staysInFrame(L142) still gates on!sample.visiblealone — uses the 0.2 visibility threshold rather thanAPPEAR_OPACITY.- Mismatch unchanged. An element can be
visible: trueat opacity 0.3 (sostaysInFrameevaluates it) whileappearsByconsiders it not-appeared. R3-tidy.
-
Nit 4 — alphabetical tiebreak + missing "one composition" doc caveat — ❌ still-open
packages/cli/src/utils/motionSpec.ts:116stillentries.filter(... endsWith(".motion.json")).sort()— first alphabetically when no basename match.- No doc clarification added on multi-composition behavior. R3-tidy.
-
Nit 2 —
querySelectorAll("*")per liveness frame — ❌ still-openpackages/cli/src/commands/motion-sample.browser.js:79stillroot.querySelectorAll("*"). R1-acknowledged, low-stakes, no change.
-
Q2 —
resolveMotionSpecexit-1 path untested — ❌ still-openpackages/cli/src/utils/motionSpec.test.tshas noprocess.exitcoverage. Test gap unchanged.
Delta verdict
No code changes since R2 approval at 83fddbb. The four open items are all R3-tidy or low-stakes — none are merge blockers. R2 verdict (approve) stands; these are follow-up cleanups, not gates.
If "review again" was expecting a new HEAD, there isn't one — happy to re-verify on push.
Review by Rames D Jusso
83fddbb to
0f834fa
Compare
Extend `inspect` to verify motion intent against the same seeked timeline
the renderer uses, catching render-≠-preview bugs that layout sampling can't:
entrance reveals the seek skips, broken stagger order, off-frame drift, and
frozen shots.
A `*.motion.json` sidecar next to the composition opts in (auto-discovered,
no flag, no authoring-framework changes); without one, inspect is unchanged.
inspect seeks a dense grid over the asserted selectors, builds an
element × time matrix of {rect, opacity, visible} plus per-scope liveness
signatures, and evaluates four assertions in Node:
appearsBy -> motion_appears_late
before -> motion_out_of_order
staysInFrame -> motion_off_frame
keepsMoving -> motion_frozen
A selector matching nothing is reported as motion_selector_missing rather
than silently passing. Findings reuse the LayoutIssue shape and flow through
the existing dedupe/collapse/limit/format pipeline and JSON envelope; they
are errors by default, so a failed assertion fails the run.
The motion pass runs in the same Chrome session as the layout audit (no extra
launch) and only when a sidecar is present.
0f834fa to
1d1d00d
Compare
Closes #1437.
Why this matters
The hardest class of agent-authored bug is render ≠ preview: animation that looks right while scrubbing the studio but is wrong in the rendered MP4, because the renderer seeks a paused timeline instead of playing it forward. A forward-only entrance reveal the seek lands past, a broken stagger, an element drifting off-frame mid-tween, a shot that silently freezes —
lintandinspect's layout sweep can't see any of it, and the only feedback today is "render the MP4 and watch it." That's exactly the step that doesn't scale for agents (cf. #911, #1174, #1387 — all preview-vs-render divergences).This adds the missing lever: state the motion intent, and have it checked against the same seeked timeline the renderer uses — no extra Chrome launch, no render, no guesswork.
What it does
Drop a
*.motion.jsonsidecar next to a composition.inspectdiscovers it automatically (no flag), seeks a dense time grid over the asserted selectors, and verifies four framework-agnostic assertions:{ "duration": 6, "assertions": [ { "kind": "appearsBy", "selector": "#headline", "bySec": 0.5 }, { "kind": "before", "a": "#headline", "b": "#cta" }, { "kind": "staysInFrame", "selector": ".card" }, { "kind": "keepsMoving", "withinSelector": ".scene" } ] }appearsBy(selector, bySec)bySec— the seek skipped a reveal (motion_appears_late)before(a, b)adoesn't first appear strictly beforeb— broken stagger order (motion_out_of_order)staysInFrame(selector)motion_off_frame)keepsMoving(withinSelector?)maxStaticSec(default 2s) — frozen shot (motion_frozen)A selector matching nothing is reported as
motion_selector_missingrather than silently passing, so a typo'd selector fails loudly.duration,withinSelector, andmaxStaticSecare optional.This implements the maintainer's scoping decisions on the RFC: (1) extend
inspectrather than add averifyverb, (2) sidecar JSON with no authoring-framework changes, (3)errorseverity by default.How an agent/human uses it (the feedback loop)
lint→validate→ author a*.motion.jsonstating what the motion should do →inspect(auto-discovers it, errors on divergence, non-zero exit) → fix → repeat. Findings appear in both the human output and--json, in the same shape and envelope as layout findings.How it works
motion-sample.browser.js) reads{rect, opacity, visible}per selector at the current seek, plus a per-scope liveness signature (visible elements' positions bucketed to 2px / opacity to 0.08) forkeepsMoving. All assertion logic lives inevaluateMotion, a pure function over the collectedelement × timematrix — fully unit-testable without a browser.LayoutIssues with four newmotion_*codes; dedupe, static-collapse, limit, the JSON envelope, the human formatter, and exit-code logic all work unchanged.inspectoutput is byte-identical to before. Sidecar present → the pass runs in the layout audit's existing Chrome session (no extra launch).Builds directly on the layout-audit work in #1435 / #1436 — same seek path, same finding shape, same JSON envelope.
Scope
All changes are in
packages/cli— no engine, runtime, or authoring-framework changes.Deferred (one-liners for later, if wanted):
--assert <path>for a non-sidecar location,--motion-fps/--motion-samplestuning, and assertion kinds beyond the four. Beat/music-relative timing (onBeat) is out of scope per the RFC.Docs
docs/packages/cli.mdx— Motion verification section underinspect.skills/hyperframes-cli/— the dev-loop skill now documents the sidecar spec and the loop, so agents reach for it instead of eyeballing renders.Testing
motion_selector_missing+ exit 1; invalid spec → clean error + exit 1.