Skip to content

feat(cli): hyperframes check — the single-session verification gate#2138

Merged
miguel-heygen merged 16 commits into
mainfrom
feat/check-command
Jul 10, 2026
Merged

feat(cli): hyperframes check — the single-session verification gate#2138
miguel-heygen merged 16 commits into
mainfrom
feat/check-command

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

hyperframes check: one command, one Chrome boot, one seek pass covering everything the old validateinspectsnapshot loop did across three browser sessions:

  • Lint gate first — browser never launches on lint errors
  • Runtime capture — console errors, page errors, failed requests (listeners wired before navigation)
  • Layout audit — overflow, clipping, held overlaps, occlusion (with covered fraction)
  • Motion verification*.motion.json sidecar assertions on the same seeked timeline
  • WCAG contrast — now a gating error, and every finding carries the sampled fg/bg colors, measured vs required ratio, and a suggested compliant color, so an agent can fix contrast without ever taking a screenshot
  • --snapshots — overview frames annotated with labeled finding boxes, plus a finding-NN-<code>.png crop per error finding
  • --caption-zone / --frame-check — opt-in band/bounds gates for render pipelines

Measured on registry/examples/kinetic-type: 5.6s vs 23.0s for the sequential three-command flow — and one tool call instead of three, which is the real win for agent loops.

Every finding is agent-actionable: selector, data-* identity, composition source file, bbox, and sample time.

Reliability semantics

  • Persistence tiers: a defect observed at a single grid sample (entrance/exit transient) demotes to info/warning; defects held across samples gate the exit code. This also re-promotes held content_overlap to an error, resolving a long-standing TODO.
  • sweep_static: a 3s+ composition showing zero geometry change across all samples fails loudly — a frozen timeline makes every green verdict unreliable.
  • Contrast judges readable content only: data-layout-ignore set dressing and text that has left the canvas are excluded.

Also in this PR

  • snapshot --zoom <selector|x,y,w,h> + --zoom-scale: high-density crops via raised deviceScaleFactor (layout untouched, deterministic); sliver frames from off-canvas targets are skipped with a note
  • validate, inspect, layout are deprecated (stderr notice + _meta.deprecated: true), still fully functional; npm run check in scaffolded projects now invokes the single command
  • HYPERFRAMES_RUN_ID env correlates a verify loop's CLI invocations in telemetry; check emits a per-gate breakdown event
  • Producer info/debug logs route to stderr (they were corrupting --json stdout for piped consumers)
  • Unified seek/settle and Chrome-launch plumbing across all browser commands (three divergent implementations become one)
  • Docs, skills, and templates all teach check as the canonical gate
  • Registry examples now pass the gate; the sweep surfaced real defects (decision-tree rendered a 5.5s blank white tail; nyt-graph's subtitle was 1.41:1)

Credit

The persistence-tier, frozen-sweep, and occlusion-coverage detection mechanics are adapted from @Adam-Rosler's open-sourced visual-linter design — thank you! Our elementFromPoint paint model, opt-out attributes, and single-audit architecture are unchanged.

Verification

  • 1550/1552 tests (2 pre-existing whisper-env failures, identical on main)
  • Real-Chrome E2E on clean and broken fixtures: contrast gate exits 1 with actionable payloads; caption band gates fire only when configured; zoom crops at true pixel density
  • All registry examples pass check except product-promo's 4 pre-existing GSAP/CSS lint conflicts (separate example-maintenance pass)

…mands

seekCompositionTimeline becomes the single seek implementation with
per-caller settle options (rAF mode, font wait, settle sleep), replacing
the divergent local seekTo copies in validate and layout. All three
launch paths now build args via the engine's buildChromeArgs; screenshot
paths keep the engine's software-GPU default for deterministic output.

inspect gains one transient content_overlap warning on product-promo
(t=12.22s): the gsap.ticker.tick flush samples timeline state the old
layout seek missed.
One command, one Chrome boot: in-process lint gate (browser skipped on
lint errors), passive runtime capture wired before navigation, layout +
motion + contrast audits over one seek grid, optional --snapshots
persisting the contrast-pass screenshots. Aggregated --json envelope
{ok, lint, runtime, layout, motion, contrast, snapshots}; findings carry
selector/data-*/source-file/bbox/time anchors, contrast findings include
fg/bg, measured vs required ratio, and a compliant color suggestion.
Contrast AA failures gate the exit code (they were warning-only in
validate); --strict gates warnings.

Contrast candidates round-trip verbatim between __contrastAuditPrepare
and __contrastAuditFinish: the page script owns their shape (bbox w/h),
and normalizing them Node-side made every sample rect NaN — the audit
reported zero checked elements as green. Regression-pinned in
check.test.ts; E2E on a low-contrast fixture now exits 1 with 8 findings.

Measured on kinetic-type: check 5.6s vs 23.0s for sequential
validate + inspect + snapshot.
Ports the EF bridge's captionZone and frameCheck semantics as opt-in
flags so the bespoke bridge can be retired: --caption-zone takes
fractional band geometry (x0;y0;x1;y1) with optional severity routing
and seek points, defaults matching the bridge (caption seek [1], frame
seek [0.5], 2px tolerance, 0.05 opacity floor, 4px minimum size, 0.95
full-frame exclusion, center-in-band comparison, tag|text dedup).
--frame-check adds media bounds detection (img/svg/video/canvas) the
always-on text canvas_overflow never covered, reusing overflowFor.
Breach floor: max(120px, 6% of min canvas dimension). Band math derives
from the composition's own canvas, portrait included. Both gates off by
default; plain check output unchanged.
HYPERFRAMES_RUN_ID (trimmed, 128-char cap) attaches as run_id to the
generic cli_command / cli_command_result events, absent when unset, so
an orchestrator setting it per design element can group a verify loop's
invocations in analytics. check additionally emits one check_report
event per invocation (including lint-short-circuited and failing runs):
gate booleans, per-class error/warning counts, launch/seek/contrast
phase timings, sample counts, ok and exit code. Timings stay internal;
no command output changes.
One stderr notice per invocation and _meta.deprecated: true in JSON mode
(shared helper next to withMeta; layout owns both inspect and layout via
createInspectCommand). Help descriptions gain the pointer. No behavior
change; removal ships separately once migration telemetry says usage
has decayed.

fix(producer): route info/debug logs to stderr — the compiler's
'Localized remote media' line was landing on stdout ahead of validate's
--json payload, breaking every piped consumer. Diagnostics now share
stderr with warn/error; render progress uses its own channel.
snapshot --zoom <selector|x,y,w,h> + --zoom-scale (default 3) crops via
Puppeteer clip at raised deviceScaleFactor — density changes, layout
never does. Selector resolves per frame with 24px padding; no match is
a loud error, and a frame whose clamped region is a sliver (element
collapsed or animated off-canvas) is skipped with a stderr note rather
than written as a useless few-pixel image.

check --snapshots additionally writes finding-NN-<code>.png crops for
error findings with bboxes (cap 12, deterministic re-seek in a second
session) and draws labeled annotation boxes on overview frames via a
transient overlay injected only after audits complete. Skill reference
gains the zoom workflow: check reports a finding, zoom into it, fix,
re-check.
… coverage

Layout findings now distinguish held defects from entrance/exit
transients: a dynamic issue seen at a single grid sample demotes to
info, while content_overlap held across two-plus samples (or 500ms+)
promotes to error, resolving the long-standing re-promotion TODO. Static
compositions keep their severity. check gains a sweep_static error when
a 3s+ composition shows zero geometry change across every sample (a
frozen timeline makes every green verdict unreliable); skipped when the
motion sidecar already reported motion_frozen. text_occluded findings
carry a coveredFraction; atomic labels (short, no whitespace) flag on
any cover while prose needs 15%, since partial cover changes what a
short label reads as.

Deprecation-test scaffolding consolidates into deprecationTestHarness;
tier logic and logger tests restructured under the complexity gate
without suppression markers.

Detection mechanics adapted from Adam Rosler's open-sourced
visual-linter design (github.com/Adam-Rosler/hyperframes-visual-linter-design);
the elementFromPoint paint model, opt-out attributes, and single-audit
architecture are unchanged.
…tional

The persistence-tier promotion surfaced two held content_overlap errors
on the hero heading; zooming in shows the collaborative-cursor demo
(cursor badge + typed comment bubble) hovering by design. Opt out with
data-layout-allow-overlap per the finding's own fixHint. The example's 4
pre-existing gsap_css_transform_conflict lint errors stay for a separate
example-maintenance pass.
Scaffolded projects' npm run check now invokes the single check command
instead of chaining lint, validate, and inspect (three Chrome boots
become one). The CLI skill, its correctness reference, the entry skill's
capability map, README/docs catalog rows, the Mintlify CLI page (new
check section, deprecation banner on inspect), template CLAUDE/AGENTS
(byte-identical), root CLAUDE/AGENTS, and every creation-workflow skill
that taught the old sequence all point at check. snapshot keeps its
standalone sections; validate/inspect stay documented as deprecated
aliases with their check equivalents.
Every duplicated decision gets one owner: rectToBbox lives in checkTypes
(was verbatim in pipeline and browser layers); the audit seek tuning is
one exported AUDIT_SEEK_OPTIONS consumed by check and the deprecated
inspect path; zoom padding/scale defaults export from the capture module
instead of re-literalized in three files; the optional run_id property
is built by one helper across all three telemetry events; check's
--max-transition-samples parsing reuses its own positiveInteger helper;
validate drops a leftover re-export and redundant explicit-default args
go away.

Tests: the contrast candidate round-trip gains a real integration
anchor (the actual browser script eval'd in-page, a wrapper asserting
finish receives the page-script bbox shape) replacing regex-over-source
as the primary guard; the redundant geometry source-golden and a
duplicated deprecation-envelope assertion are dropped.
…heck

Three filters keep the escalated contrast gate honest, each surfaced by
running check across the registry examples:
- data-layout-ignore (the layout audit's existing decorative opt-out)
  now also excludes set-dressing text from the contrast audit — one
  vocabulary for 'not copy a viewer must read'.
- Text that has (nearly) left the canvas is skipped: sampling a clamped
  off-canvas box reads border pixels and produced the classic false
  white-on-white (a cursor exiting the frame).
- Contrast failures follow the same persistence rule as layout findings:
  observed at a single sample of a multi-sample sweep demotes to
  warning; held failures gate.

Example fixes the sweep exposed: motion-blur's 21 deliberately-dim rail
labels are marked decorative (its vivid labels pass on their own);
nyt-graph's subtitle and source-note adopt the gate's suggested
compliant gray; decision-tree's declared duration drops 15s to 10s —
its content ends at ~9.5s and every render shipped a blank white tail.
@mintlify

mintlify Bot commented Jul 10, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview Jul 10, 2026, 6:13 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

…n-blur

Running the CLI from source (tsx dev script, as CI's smoke job does)
transpiles with keepNames, which rewrites named inner functions in
serialized page closures into __name(...) calls — a helper that exists
in the Node bundle but not in the browser realm. The unified seek
closure was the first validate-path page function with named inner
functions, so 'hyperframes validate' threw '__name is not defined' in
CI while the dist build worked. Every session opener now installs a
no-op __name shim via evaluateOnNewDocument before any page script
runs, immunizing all serialized closures regardless of build mode.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 96cb5e39. Cross-repo pair with heygen-com/experiment-framework#41829 (the zephyr-side consumer, reviewed separately).

The consolidation is well-shaped: single browser session, single seek loop, deterministic seek + settle, persistence-tier semantics that also close the long-standing content_overlap TODO, and a top-level envelope contract that EF's _parse_check_result can pin against. process.exitCode in check.ts (vs. validate's process.exit) is the right call — lets finallys drain the browser/server. Rollout mechanics look correct: validate/inspect/layout still functional with printDeprecationNotice + _meta.deprecated: true, and the pinned CLI version + default-off flag on the EF side keep the rollback path byte-for-byte.

One functional concern (possible silent-drop) and two observability nits inline. Nothing blocking-blocking — all three flag things worth a look before the release cut that EF's HYPERFRAMES_VERSION bumps to.

Also verified (no findings):

  • The runtime listener (checkBrowser.ts:174-208) is a strict superset of validate's console/pageerror/requestfailed capture, including the Unexpected token '<' CDN-error suppression and shouldIgnoreRequestFailure's media net::ERR_ABORTED tolerance.
  • The missing_or_empty_sub_composition lint code that validate.extractCompositionErrorsFromLint pulled out is caught natively by buildLintSection — it flows through as a lint.error, no bypass needed.
  • _parse_check_result in the EF side (utils.py:178-233) requires every one of lint/runtime/layout/motion/contrast to be present as a dict with a findings array; every severity must be error/warning/info; per-section counts must match the finding list — this is the anti-silent-drop invariant. check.ts / checkPipeline.ts both honor that shape (buildReport line 720-771 unconditionally builds all five sections, plus snapshots and _meta via withMeta).
  • sweep_static (detectSweepStatic, line 430-456) skips <3s compositions, single-sample runs, and runs where motion_frozen already reported the same symptom — the double-report guard reads correctly.
  • contrastFailureHeld (line 641-655) reproduces the persistence-tier logic for contrast findings the same way layout uses collapseStaticLayoutIssues. Single-sample sweeps skip the demotion — expected.

Review by Rames D Jusso

renderReadyWarningSuffix: "checking the current page state",
browserGpuMode: resolveCliChromeGpuMode(),
beforeNavigate: (page) => wireRuntimeListeners(page, drafts, () => currentTime),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Possible silent-drop from validate consolidation: auditClipDurations. The old validate command runs a per-media-element audit (packages/cli/src/commands/validate.ts:135-230) that reads each <video>/<audio>'s intrinsic .duration (via page.evaluate), compares it to the data-duration slot, and warns "Audio is 22.10s but its slot (data-duration) is 30.00s — the slot is shortened to the media length when rendered." This is a real, user-visible render defect that neither lint (can't see intrinsic media duration) nor check's runtime-listener path catches — a silently-shortened slot loads with no console.error/pageerror/requestfailed.

I didn't find analyzeClipMediaFit / auditClipDurations / data-media-start anywhere in the new pipeline (grep -rn across checkBrowser.ts, checkPipeline.ts, checkTypes.ts). If this is deliberately deferred, worth calling out in the PR description under "Deferred" so the deprecation notice on validate doesn't lead users to a lossy migration. If it's an oversight, porting auditClipDurations into wireRuntimeListeners (or as a driver step alongside getDuration) would close it — the browser session is already open, so the marginal cost is minimal.

— Rames D Jusso

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — this was an oversight, not a deferral. Ported in 67cdf0f: the clip audit now runs in check's existing session right after settle (one extra evaluate), surfacing as clip_media_fit findings in the runtime section with the same warning text. Unit-pinned in checkBrowser.test.ts.

Comment thread packages/cli/src/utils/checkPipeline.ts Outdated
try {
lintResult = await dependencies.lintProject(project.dir);
} catch (error) {
return failureReport(options, runtimeFailure(error));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Lint-execution failure is misclassified as a runtime failure. If dependencies.lintProject(project.dir) itself throws (linter crash, unreadable file, etc. — distinct from lint findings), failureReport(options, runtimeFailure(error)) produces a report with lint.filesScanned = 0 and the failure surfaces as a runtime finding with code check_runtime_failure. From an agent-consumer standpoint that mis-locates the fault (they'll look for a runtime script issue when the linter itself broke).

Cheap fix: pass an explicit code, e.g. failureReport(options, runtimeFailure(error, "check_lint_failure"))runtimeFailure already accepts a code override on line 876. Even better would be surfacing it under lint.findings so section counts add up, but that requires threading a lint-shaped failure through buildReport; the code-override alone is the minimal defensible fix.

— Rames D Jusso

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid. Took the minimal fix in 67cdf0f: a linter crash now reports as check_lint_failure via the existing code override. Agreed a lint-shaped failure through buildReport would be nicer; left as-is to keep section counting untouched.

const findingFiles = await dependencies.captureFindingCrops(project, options, cropRequests);
return { ...report, snapshots: { ...report.snapshots, findingFiles } };
} catch {
return report;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Finding-crop failures are silently swallowed with no observability trace. The catch { return report; } is defensible design ("crops are bonus evidence") but there's currently no way to measure how often the second Chrome launch fails or times out — telemetry-wise this is a blind spot next to trackCheckReport on line 772.

Suggest a one-line trackCommand-style event (or a field on the existing trackCheckReport payload) so a rollout has visibility into finding-crop failure rate without the run being marked failed. Purely observability, non-gating.

— Rames D Jusso

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid. In 67cdf0f the catch stays non-gating but now emits a stderr note plus a telemetry error event (command: check-finding-crops), so the second-session failure rate is measurable during rollout without failing runs.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM with concurrence on Rames's findings. Read the check pipeline end-to-end at 96cb5e3 against my split lenses (session lifecycle, listener-wiring order, deprecated-command shape parity, persistence tiers, sweep_static false-positives, contrast payload, producer log routing, run-id plumbing). Rames landed first with three substantive inline findings I want to explicitly concur with — one of them worth another look before the release cut.

Concurrence with Rames

  • 🟠 auditClipDurations silent-drop under consolidation — this is a real feature regression, not a nit. validate.ts:135-230 runs a per-media-element intrinsic .duration audit ("Audio is 22.10s but its slot is 30.00s — the slot is shortened to the media length when rendered") that check's lint / runtime / layout / motion / contrast passes do NOT cover. Grepped the new pipeline for auditClipDurations / analyzeClipMediaFit / data-media-start / any intrinsic-media-duration probe; nothing. Since validate stays functional under the deprecation, existing agents keep the warning today. But the "migrate to check" story implies parity, and this is the biggest gap in that story. The three routes I see:

    1. Port the audit into the new pipeline as a new check-section (media) or fold into layout.
    2. Move auditClipDurations into checkBrowser.ts and expose findings on the layout section (simplest — media already lives inside the DOM the pipeline is auditing).
    3. Keep validate un-deprecated indefinitely for callers that rely on this specific warning; downgrade the deprecation notice to "check is the preferred path for most callers" and re-classify.

    My preference is (2): the audit's cost is low (already runs inside the same page load), the finding shape maps cleanly onto CheckFinding (has selector, source file, bbox — the <video>/<audio> element), and it avoids a bifurcated migration story. Not blocking the merge given validate still works, but I'd want this filed as a follow-up before flipping any EF cohort to use_hyperframes_check: true.

  • 🟡 Lint-execution failure misclassified as check_runtime_failure (checkPipeline.ts:517-522). Concur — cheap fix on the same line via the existing code override on runtimeFailure. failureReport(options, runtimeFailure(error, "check_lint_failure")). Agent consumers reading the code field will otherwise look for a runtime script defect when the linter itself broke.

  • 🟡 Finding-crop failures silently swallowed with no observability (checkPipeline.ts:592-594). Concur — catch { return report; } is the right SHAPE (crops are bonus), but the rollout will want visibility into how often the second Chrome launch fails. Adding a field to the existing trackCheckReport payload (checkPipeline.ts:772-796, e.g. findingCropFailed: boolean) is the minimum-friction change.

My independent lens read (no new findings, receipts here for the record)

Rames's "also verified" block covered runtime-listener parity, missing_or_empty_sub_composition lint routing, _parse_check_result envelope shape, sweep_static guard shape, and contrastFailureHeld. Rather than duplicate, here's what I found the same on the lenses my split emphasized:

  • Session lifecycle — clean. checkBrowser.ts:96-124 uses chromeBrowser?.close() in an outer try/finally. The launch path itself has its OWN inner catch at captureCompositionFrame.ts:158-179 that closes the browser on any of puppeteer.launch → newPage → setViewport → beforeNavigate → goto → waitForCompositionSettle throwing. So the "launched-but-lost-reference" leak I went looking for isn't there — inner catch owns cleanup on the pre-return path, outer finally owns cleanup on the post-return path. captureFindingCrops (checkBrowser.ts:142-172) mirrors the shape.
  • Listener wiring before page.goto — verified in code. openSettledCompositionPage orders as: launch → newPageinstallPageFunctionGuardsetViewportbeforeNavigate (wires listeners) → page.goto (captureCompositionFrame.ts:160-174). runBrowserCheck passes wireRuntimeListeners in via beforeNavigate at checkBrowser.ts:104. Early runtime errors captured.
  • Deprecated-command shape parity — preserved. validate --json still emits { ok, errors, warnings, contrast, contrastFailures, _meta } (validate.ts:517-530). inspect --json / layout --json still emit { schemaVersion, ok, issues, errorCount, warningCount, infoCount, issueCount, _meta } (layout.ts:389-403). Wrapping via withMeta(..., { deprecated: true }) at utils/updateCheck.ts:140-146 is additive — consumers reading by key presence, not equality, stay green.
  • Contrast payload — every promised field on the wire. buildContrastResults at checkPipeline.ts:657-690 emits per finding: fg, bg, ratio, requiredRatio, suggestedColor, selector, text, bbox, time, sourceFile, dataAttributes, large. suggestedColor runs suggestCompliantForegroundColor (line 692-700) to produce a real actionable rgb(r,g,b) string, not a placeholder. This alone justifies the rewrite.
  • sweep_static false-positive shape worth naming. The guard at checkPipeline.ts:430-456 correctly skips <3s compositions, single-sample runs, and motion_frozen-covered runs. But a legitimate 10-second static title card (a hero card held for read time, no animation) would trip sweep_static and get "Timeline did not advance under seek." The fix hint asks the author to "confirm the composition seeks a paused GSAP/CSS timeline," which won't apply. Not blocking (the intended defect this catches is far worse than the false positive on an intentional static hold, and subtle opacity/cursor animation on a real title card changes the fingerprint), just naming it for the first agent that hits it in a real composition.
  • Producer logs → stderr — verified. packages/producer/src/logger.ts:67-91: all four levels route to console.error / console.warn, both of which go to stderr in Node. Nothing corrupts --json stdout.
  • HYPERFRAMES_RUN_ID plumbing — correct, one behavior note. runId.ts:1-12 reads the env var, trims to 128 chars, memoizes on first call; runIdField at events.ts:8-9 omits run_id from telemetry when undefined. So presence of run_id deterministically indicates orchestration. Cleaner than a synthesized fallback. Slight prose slip in the PR body ("CLI generates one" — it doesn't); the omission behavior is better than a fake, so I'd leave the code and edit the body if anything. The memoization is process-local: fine for a shell invocation, worth a comment if anyone ever embeds check as a library.

One nit that could become a time-bomb (non-blocking, cross-repo)

Not this repo's bug, but I'll note here since these two PRs are cross-linked: on the EF side, _frame_check_args at hyperframes_validate/utils.py:92-95 currently returns ["--frame-check"] and drops the passed severity / seek values because the HF flag is boolean (check.ts:111-116). Today's only EF caller uses severity="warning" and seek=(0.5,), which happen to match HF's defaults, so the two paths agree by coincidence. If any future EF caller ever tunes frame_check_options(severity="error") to route into a scene-retry, the bridge honors it; the CLI silently drops it. Consider promoting --frame-check to a value-form (--frame-check "severity=error;seek=.5") mirroring --caption-zone's spec at check.ts:106-116 when this contract firms up — the comment on the EF helper already anticipates this.

On the receipts

The 5.6s vs 23.0s round-trip savings claim is architecturally plausible (one Chrome, one navigation, one seek grid, one contrast pass vs three separate lifecycles) and the internal telemetry (launchSettleMs, seekLoopMs, contrastMs) is now wired to measure it live. The two registry-example defects cited in the PR body (decision-tree 5.5s blank tail, nyt-graph 1.41:1 subtitle) are trust-but-note — verifying either requires running the CLI, out of scope for this review. The tests that DID land here (1315 lines of check.test.ts, per-section shape locks, sweep_static edge cases, run-id memoization) cover this reviewer's lenses well.

R1 by Via

…sing, crop observability

Port validate's per-media-element clip audit into check's session
(clip_media_fit findings): an intrinsic duration meaningfully shorter
than the data-duration slot silently shortens the slot at render time,
and neither lint nor the runtime listeners can see it. A linter crash
now reports as check_lint_failure instead of masquerading as a runtime
failure. Finding-crop capture failures stay non-gating but emit a
stderr note and a telemetry error event so rollouts can measure the
second-session failure rate.
The pipeline already carried FrameCheckOptions; only the flag was
boolean, which meant a pipeline caller tuning severity or seek points
would have them silently dropped — the two sides only agreed because
today's caller happens to match the defaults. Bare --frame-check keeps
the defaults; the value form mirrors --caption-zone's grammar, freezing
the contract before a release pins it.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Addressed the three concurred findings in 67cdf0f (clip audit ported into check's session as clip_media_fit findings; linter crash now reports check_lint_failure; crop failures emit stderr + a telemetry error event).

On your independent notes:

  • --frame-check value-form: implemented in 659cb6a — bare flag keeps defaults, and --frame-check "severity=error;seek=.25,.75;tol=4" mirrors --caption-zone's grammar, so the pipeline options stop being coincidentally-aligned with the bridge caller. Contract is now frozen ahead of the release pin.
  • sweep_static on intentional long static holds: agreed it's a real false-positive shape. Leaving as-is for now (the frozen-timeline defect it catches is much worse, and --at single-sample runs skip it); if real compositions hit it, the plan is an explicit opt-out rather than loosening the guard. Noting it for the docs follow-up.
  • run-ID body prose: the body reads "env correlates a verify loop's CLI invocations" — caller-supplied, no generation claim, so I've left it. The memoization comment for library embedding is a fair future note.

Thanks for the deep lens pass — the session-lifecycle and shape-parity receipts are exactly what the deprecation story needed.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 verify at 659cb6a2. Cross-repo pair with heygen-com/experiment-framework#41829 c1e34039 (reviewed separately). Per-finding disposition across the two address-review commits (67cdf0fd, 659cb6a2) below.

Verdict: 🟢 LGTM — all four concurred findings resolved with verifiable code delta.

The trail my magnifying glass followed:

  1. Clip-duration parity gap — ✅ resolved. auditClipDurations is now exported from validate.ts:135 (the single +1/-1 line — precisely the "reference shared implementation" I hypothesised, no drift path). check's session now invokes it at checkBrowser.ts:112-119 after resolveRootAnchor and before the driver spins up — unconditional, no flag guard, exactly one extra page.evaluate. The finding shape {code: "clip_media_fit", severity, message, time: 0} matches the existing RuntimeFinding schema (severity is "error"|"warning" per the entry.level union). Test coverage in checkBrowser.test.ts:216-247 uses a partial mock so the audit's mocked result flows through into result.runtimeFindings — the assertion is on the finding-shape, not on the audit internals. Clean port; the two implementations no longer drift.

  2. Lint-execution failure misclassified — ✅ resolved. checkPipeline.ts:521 now passes "check_lint_failure" as the second arg to runtimeFailure() when lintProject throws, distinguishing linter-crash from a script runtime crash. Comment explicitly calls out the divergence ("distinct from lint findings; a runtime-failure code would send the agent hunting for a script problem that doesn't exist") — telemetry consumers can cohort the two failure modes.

  3. Finding-crop failures silently swallowed — ✅ resolved. checkPipeline.ts:595-600 — catch stays non-gating (still return report), but now emits console.error(" finding crops skipped: ...") and calls trackCommandFailure("check-finding-crops", error). Exit code path unchanged (crop failures don't gate a rollout). The rollout cohort can now see the crop-failure rate through the telemetry funnel without the audit lying about clean runs. The comment "Still non-gating, but observable" is the right invariant.

  4. --frame-check spec grammar — ✅ resolved (this is the load-bearing one for the cross-repo pair). check.ts:107-111 flips the flag type from boolean to string. The parser parseFrameCheck at check.ts:174-193 handles four shapes: undefined|null|false → undefined, true|"" → {} (bare-flag default preserved), string → parsed severity/seek/tol via the same parseCaptionField grammar as --caption-zone (so the mental model is shared), invalid → throw with an actionable message. Downstream consumers all read the parsed fields:

    • options.frameCheck.seek at checkPipeline.ts:143-144 (gateSampleTimes(duration, options.frameCheck.seek, 0.5) — falls through to 0.5 default when absent, honours the caller's seek when present)
    • options.frameCheck.tol at checkPipeline.ts:207 via configuredTolerance
    • options.frameCheck.severity at checkPipeline.ts:302 (options.frameCheck.severity === "error" ? "error" : "warning")
    • options.frameCheck !== undefined at checkPipeline.ts:783 still gates the whole feature

    End-to-end propagation is real — the parsed values are consumed at every downstream site, not just accepted-then-discarded. The parseFrameCheck unit test at check.test.ts:1115-1128 exercises all four input shapes plus a bad-key and a negative-tolerance rejection. And the EF-side cross-repo consumer at _frame_check_args in experiment-framework#41829 now emits severity=warning;seek=0.5;tol=2 unconditionally (verified in the sibling review) — the cross-repo time-bomb from R1 is defused.

Small elegant details worth noting:

  • The FRAME_CHECK_FIELDS whitelist rejects unknown keys with the same idiom --caption-zone uses — no silent-accept surface. Attempting --frame-check "bogus=1" throws Invalid --frame-check: use bare --frame-check or "severity=warning|error;seek=.25,.75;tol=4" (all fields optional).
  • parseFrameCheckTolerance rejects NaN and negatives (defensive; Number.isFinite(tol) || tol < 0 is the right guard for a pixel tolerance).

Ship it. Both commits are in the "reference implementation" shape — one path per concern, no drift surface, tests that would catch a future regression. This is the pattern I like to see in address-review commits: don't refactor around the finding, port the missing behaviour and prove it with a test. The EF-side observability commit has one live bug flagged separately in that review, but nothing on the CLI side blocks the merge.

R2 by Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 at 659cb6a23605 (delta from R1 96cb5e39 → 6 files, +132/-22).

All three R1 findings cleanly addressed:

  • 🟠 auditClipDurations silent-dropauditClipDurations imported from validate.js and called on the shared session at checkBrowser.ts:114, emitting clip_media_fit findings. Zero-extra-Chrome cost as suggested.
  • 🟡 lint-execution failure misclassificationruntimeFailure(error, "check_lint_failure") at checkPipeline.ts:524, distinct from check_runtime_failure. Agents / dashboards can now distinguish linter-crash from render-crash.
  • 🟡 finding-crop silent-swallow → both a console.error and trackCommandFailure("check-finding-crops", error) at checkPipeline.ts:597. Rollouts get the crop-failure rate.

Bonus win: --frame-check now accepts the severity/seek/tol spec grammar mirroring --caption-zone, resolving Via's latent cross-repo divergence finding (_frame_check_args was passing values that HF would silently ignore).

LGTM from my side. Stamp is James's call.

Review by Rames D Jusso

@miguel-heygen miguel-heygen merged commit 00d059b into main Jul 10, 2026
55 of 57 checks passed
@miguel-heygen miguel-heygen deleted the feat/check-command branch July 10, 2026 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants