fix(check): keep auditing when a motion spec is partly unusable

[xuanruli]

Rebuilt onto today's main (the July branch shared no ancestry after the history rewrite) and consolidated with #2806, which touched the same two files.

1. Ambiguous selector no longer aborts the spec (was this PR)
An ambiguous selector is reported as a finding, the assertions that depend on it are skipped and named, and every other assertion still evaluates. assertionTargets is exhaustive over the assertion kinds, so a new kind cannot silently fall back to an arbitrary first match while its selector is also reported ambiguous. Its default branch keeps the compile-time never check but returns empty targets at runtime, so a future kind that slips past spec validation degrades instead of throwing.

The "N assertion(s) naming it were not evaluated" suffix is counted per ambiguous selector. An earlier draft appended the global skipped total to every message, so two ambiguous selectors with one assertion each both claimed "2".

2. Unparseable sidecar no longer ends the run (was #2806)
A motion sidecar that will not parse becomes a spec finding instead of an early return, so the composition is still audited. The invalid spec yields an empty sampling plan, so no motion evaluation runs and the finding still lands in errorCount — the gate keeps failing.

Tests: 190 pass on the four affected files (main: 182).

Not addressed here: hf layout's own runMotionPass still aborts the whole spec on an ambiguous selector and still exits on an unparseable sidecar, so the same sidecar now behaves differently under hf check and hf layout. Worth a follow-up that has layout.ts reuse assertionTargets the same way. Also unchanged: a keepsMoving scope is not preflighted for ambiguity, so it is neither reported nor counted.

Closes #2806.

[james-russo-rames-d-jusso]

:large_green_circle: 1f0e90f9f COMMENTED

Hey Xuanru — the shape is right (unusable parts stay findings, usable parts still audit, report.ok=false when either fires), and the narrow filter beats the previous early-return. Verified against the current single-commit force-push; the typecheck fix in this push landed exactly the type-mismatch I would have flagged pre-push, so nothing on the code side blocks.

Verified as OK

• Partly-unusable path is fail-closed at source — planMotionSampling in checkPipeline.ts computes preflightIssues first, filters motion.spec.assertions against ambiguous.has(selector), and returns both in MotionPlan. runAuditGrid seeds motionIssues = plan.preflightIssues and appends anchored evaluation results (motionIssues = [...motionIssues, ...(await driver.anchorMotionIssues(evaluated))]) — the ambiguous finding is never dropped. This is the exact bug shape the framing worried about; it doesn't fire here.
• Unusable part surfaces as its own severity=error finding — ambiguousIssue in motionAudit.ts sets severity: "error"; the invalid-sidecar path (runCheckPipeline) reuses findingAtRoot("motion_spec_invalid", "error", ...) and threads it through buildReport(..., specFindings, ...). The new still audits layout when the motion sidecar is invalid test locks report.ok === false for the invalid-sidecar case.
• Filter scope is narrow — no swallow — the "keep auditing" logic is a per-assertion .filter(a => !assertionTargets(a).selectors.some(s => ambiguous.has(s))), not a try/except wrapping the audit loop. assertionTargets in motionAudit.ts uses an exhaustive switch with a never-typed default that returns empty targets at runtime — a genuine bug in evaluation logic isn't absorbed anywhere.
• Tests discriminate the defect boundary — the keeps evaluating the unambiguous assertions when one selector is ambiguous and drops a before assertion when either side is ambiguous, keeping the rest cases assert codes.sort() === ["motion_appears_late", "motion_selector_ambiguous"], so a regression that either (a) silently swallows ambiguity or (b) short-circuits the usable audit shows up. does not sample when every assertion names an ambiguous selector covers the all-unusable case and pins samples: 0 + motion_selector_ambiguous present. The two per-selector-count tests pin 1 assertion(s) vs 2 assertion(s) separately, which a global-count variant would have failed.
• Typecheck fix landed correctly — check.test.ts:1192 at HEAD reads findAmbiguousSelectors: vi.fn(async () => [{ ...layoutIssue("error"), ...ambiguousIssue(".item") }]), which spreads the anchored fields (dataAttributes, sourceFile, bbox) over the LayoutIssue shape to satisfy the Promise<AnchoredLayoutIssue[]> return type on CheckAuditDriver.findAmbiguousSelectors. Typecheck at HEAD 1f0e90f9f is pass.
• Happy path preserved — reports a failing appearsBy sidecar as motion_appears_late still holds; the new plumbing doesn't disturb it.

Concerns (both acknowledged in the PR body, tracking here for the record)

• hf layout divergence — runMotionPass in hf layout still aborts the whole spec on an ambiguous selector and exits on an unparseable sidecar, so post-merge the same sidecar behaves differently under hf check vs hf layout. Worth a follow-up that reuses assertionTargets from motionAudit.ts the same way.
• keepsMoving scope not preflighted for ambiguity — assertionTargets returns scopes for keepsMoving in a separate bucket, and planMotionSampling only tests .selectors against ambiguous. An ambiguous withinSelector neither surfaces nor counts as skipped. Small follow-up.

What I didn't verify

• Runtime behavior of assertionTargets default branch when a spec somehow contains an unknown assertion kind at runtime (spec validation should preclude it; author says degradation-over-throw is intentional).
• Prior state of the PR pre-force-push — reviewed the current single-commit shape only. 1f0e90f9f's parent is master (7a2a6917, #3735 catalog fix), so the current commit is the entire PR content.

State at HEAD 1f0e90f9f: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. CI at HEAD: Typecheck pass, Build pass, Lint pass, Preview parity pass, Studio load smoke pass, CLI: npx shim (all three OSes) pass. Test / Producer: integration tests / Tests on windows-latest: studio-engine-cli still pending as of read; older run 34069796229 at prior push showed the pre-fix typecheck failure — that push was superseded, disregard.

Code LGTM from my side. Stamp goes through a HyperFrames-authorized reviewer — heygen-com/hyperframes runs dismiss_stale=false / require_last_push_approval=true, so pin the stamp to the exact SHA that lands.

— Review by Rames D Jusso

[jrusso1020]

APPROVE at 1f0e90f9faadfa2db9dc7943c72938cce04fb6d4. Read the full changed files plus checkBrowser.ts, checkTypes.ts, layoutAudit.ts, layout.ts, check.ts, motionSpec.ts and motion-sample.browser.js at that SHA. I did not run the repo suite (see the note at the end).

The core property holds: partly-unusable is fail-closed

Both mechanisms check out, and so does the invariant that connects them.

• planMotionSampling (checkPipeline.ts:184-196) drops an assertion only when one of its selectors is in ambiguous, which is built from preflightIssues. So drop implies a motion_selector_ambiguous error finding -- there is no silent-skip branch.
• runAuditGrid (checkPipeline.ts:1082-1090) appends: motionIssues = [...motionIssues, ...await driver.anchorMotionIssues(evaluated)]. That is the load-bearing line and it is pinned by a test.

The one way the invariant could have broken is the set key, so I checked it: anchorLayoutIssues can rewrite issue.selector to "[data-composition-id]" (checkBrowser.ts:733), but only when querySelector(selector) is null. A selector reaches this list because querySelectorAll(sel).length > 1 (:666), so querySelector is non-null by construction and the selector round-trips intact.

Downstream nothing launders a skipped element into a pass: PERSISTENCE_TIERED_CODES deliberately excludes motion_* so the error is never demoted to info, staticIssueKey includes the selector so per-selector findings do not collapse, limitLayoutIssues sorts errors first so truncation cannot drop the last one, and motion_spec_invalid is appended after limiting. errorCount feeds ok. Gate stays closed.

assertionTargets exhaustiveness -- verified rather than eyeballed. I rebuilt an isolated strict typecheck over the verbatim motionAudit.ts / motionSpec.ts / layoutAudit.ts from this SHA (clean baseline), then added a fifth kind to the union. It fails to compile at motionAudit.ts:252 (not assignable to type 'never') and again at :221, so a new kind is a compile error at both sites, not a runtime fallback. Claim confirmed.

Non-blocking, but the first is a real one

1. keepsMoving can now evaluate against a selector already known to be ambiguous. assertionTargets returns selectors: [] for keepsMoving (motionAudit.ts:249-250), so it is never filtered and never counted. Concrete case: spec is [{appearsBy ".card"}, {keepsMoving withinSelector ".card"}] and .card matches three elements. .card gets preflighted via the appearsBy and reported ambiguous, the appearsBy is dropped -- but the keepsMoving survives, assertions.length > 0 so sampling runs, and sampleLiveness does safeQuery(scope) -> document.querySelector -> first match only (motion-sample.browser.js:111). So that assertion can report pass against an arbitrary one of the three, while the message says "1 assertion(s) naming it were not evaluated."

This is narrow and the overall gate still fails on the ambiguity error, which is why it is not a blocker. But it is genuinely new: before this PR any ambiguity aborted the spec, so nothing could evaluate against an ambiguous selector. It is also distinct from the unpreflighted-scope case your PR body already discloses -- here the selector is known ambiguous and the assertion evaluates anyway. Cheapest fix is to have assertionTargets surface keepsMoving's withinSelector for the ambiguity check while keeping it out of the sampling selector set.

2. One PR-body claim does not hold. The body says the default branch "returns empty targets at runtime, so a future kind that slips past spec validation degrades instead of throwing." It would still throw: with empty targets the unknown assertion is unfilterable and reaches evaluateMotion, whose switch has no default (motionAudit.ts:221-236), so the callback returns undefined, flatMap yields [undefined], and anchorMotionIssues dereferences issue.selector (checkBrowser.ts:698) -> TypeError. The scenario is unreachable anyway (parseMotionSpec rejects unknown kinds against a closed VALIDATORS whitelist, motionSpec.ts:72-73). Harmless belt-and-braces; just do not count the degradation as a property you have.

3. Two of the seven new tests pass with the logic disabled. "does not sample motion for an invalid sidecar" and "does not sample when every assertion names an ambiguous selector" both hold trivially on revert -- the browser never runs, so the negative assertion is satisfied for the wrong reason. Worth keeping as guards, just not countable as regression coverage. The other five are real pins; "keeps evaluating the unambiguous assertions when one selector is ambiguous" is the one that pins append-vs-replace, since a replace drops the ambiguity error and the sort assertion fails.

4. The test-count claim I could not reconcile. The body says "190 pass on the four affected files (main: 182)." At this SHA check.test.ts goes 52 -> 59 blocks (+7) and motionAudit.test.ts, motionSpec.test.ts, checkBrowser.test.ts, layout.test.ts and both checkPipeline.* files are unchanged. No four-file subset sums 182 -> 190 and the +8 does not match the +7 actually added. Most likely a stale number from before the consolidation -- flagging rather than asserting, since I did not run the suite.

5. Smaller things. noteSkippedAssertions (checkPipeline.ts:199-206) mutates driver-returned objects in place via issue.message +=; safe today because those objects are built fresh per call, but a memoizing driver would accumulate the suffix. The motion section discards truncated / totalIssueCount at :1360 unlike shapeLayoutSection at :1381 (pre-existing, slightly more reachable now). And findMotionSpec throwing on multiple matching sidecars (motionSpec.ts:125-129) is called outside any try at :1152, so it still ends the run with no report at all -- the same shape this PR is fixing, one path over.

Telemetry note: trackCheckReport is not success-only, so a partial audit always shows motionErrors >= 1 -- good. But there is no assertionsSkipped dimension, so 1-of-10 skipped and 9-of-10 skipped are indistinguishable. Since partial audit is the behaviour this PR introduces, that is the thing you would want to measure.

What I did not do

I read the code at the SHA above; I did not run the repo typecheck or test suite (that needs a monorepo install I was not willing to do against the shared checkout). CI at this head is green on all eight required contexts, but I am approving on the code, not on CI.

-- Rames