Skip to content

fix(cli): distinguish a skipped browser check from a genuinely clean one - #3901

Open
miga-heygen wants to merge 1 commit into
mainfrom
fix/check-lint-error-skips-browser-session
Open

fix(cli): distinguish a skipped browser check from a genuinely clean one#3901
miga-heygen wants to merge 1 commit into
mainfrom
fix/check-lint-error-skips-browser-session

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Problem

hyperframes check's runtime/layout/motion/contrast sections in the JSON report claim ok:true/zero findings identically whether the browser session actually ran and found nothing, or never ran at all. Three distinct situations skip the browser entirely and fall through to the same empty-result shape:

  1. A blocking lint error short-circuits the run before any browser launches.
  2. The lint step itself throws.
  3. The browser check throws before producing an audit result.

In all three cases, the top-level ok field is correctly false, but a caller (human or agent) that only inspects the individual sections sees a false "all clear" — the only tell was an undocumented layout.duration === 0.

Fix

Add a skipped: boolean field to CheckBrowserResult, set at the only two places one is ever constructed: false in the real audit result (runAuditGrid), true in the empty-result fallback (emptyBrowserResult(), used by all three skip paths above). Mirror it to a new top-level browserSkipped: boolean on CheckReport, set in buildReport directly from browser.skipped.

  • check --json output now includes browserSkipped — the explicit signal a caller needs.
  • The human-readable report prints a warning line when the browser never ran, worded to be accurate across all three trigger paths (two of them still leave a real diagnostic finding in the Runtime section, so the warning doesn't claim Runtime itself is empty — only layout/motion/contrast always are).
  • Updated the CLI reference doc to mention browserSkipped in the envelope shape and call out the false-clean risk explicitly.

Tests

packages/cli/src/commands/check.test.ts:

  • Updated the existing lint-block test to also assert browserSkipped: true and that layout/motion/contrast report clean/empty despite never having run.
  • Added a test asserting browserSkipped: false on a normal completed run.
  • Added a test asserting browserSkipped: true when the browser check throws before producing results, and that runtime (unlike the other sections) carries the real diagnostic finding in that case.
  • Extended the existing JSON-envelope test to check browserSkipped: false.

Verified the fix is load-bearing via a real revert: stashed the four production-code files (kept the test changes), reran — the four new/updated assertions failed exactly as expected (browserSkipped read undefined); restored, reran — 61/61 green again.

Verification

  • bunx tsc --noEmit -p packages/cli — clean
  • bunx oxlint / bunx oxfmt --check on all touched files — clean
  • bunx vitest run packages/cli/src/commands/check.test.ts — 61/61 passed
  • bunx vitest run packages/cli (full package) — pre-existing failures only, unrelated to this diff (real-Chromium launch tests, macOS-only vm_stat parsing, host telemetry env leakage, PID/socket ownership checks) — confirmed identical on an unmodified checkout before making any change.

`hyperframes check`'s runtime/layout/motion/contrast JSON sections reported
the exact same ok:true/zero-findings shape whether the browser session
actually ran and found nothing, or never ran at all (a blocking lint error,
a lint-step crash, or an exception thrown before the audit could start all
took the same "empty result" fallback). Only the top-level ok field and an
undocumented layout.duration === 0 tell separated the two cases apart.

Add a skipped boolean to CheckBrowserResult, set at the only two places one
is constructed (false in the real audit result, true in the empty-result
fallback), and mirror it to a new top-level browserSkipped field on
CheckReport. The human-readable report now prints a warning when the
browser never ran; the JSON envelope carries the same signal for callers
that only check individual sections.

Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Independent review at 5e74e2cf (manual pass — Codex spend-capped this session, so direct source verification, not a codex run). Verdict: correct, well-designed, no blockers. One minor completeness note.

Verified at source — the design is the right one

  • skipped/browserSkipped are required, not optional (checkTypes.ts). That's what makes this airtight: the compiler forces every CheckBrowserResult construction to set skipped, so "cover every site" is a type obligation rather than a reviewer's grep. (It's why the test helper reportWithFindings had to add browserSkipped: false to compile.)
  • Construction sites are complete. skipped is assigned a literal in exactly two places — runAuditGridfalse (checkPipeline.ts:1123, a session genuinely ran) and emptyBrowserResult()true (:1504). Every browser value reaching buildReport flows from one of those two, and buildReport mirrors it verbatim (browserSkipped: browser.skipped, :1379). Traced all four paths:
    • lint blocking error → emptyBrowserResult() (:1150) → skipped ✓
    • lint step itself throws → failureReport (:1140:1530) → skipped ✓
    • browser check throws → catch → emptyBrowserResult() (:1170) → skipped ✓
    • real audit → runAuditGrid → not skipped ✓
  • Warning wording is carefully scoped. The human-report line (check.ts) names only layout/motion/contrast as empty placeholders and deliberately excludes runtime, because the throw/lint-crash triggers leave a real diagnostic finding there. That nuance is correct and matches the type doc-comment.

Non-blocking: one of the three skip triggers is untested

The tests pin the lint-block and browser-throw triggers (browserSkipped: true), plus the normal (false) and JSON-envelope cases. The third trigger — the linter itself crashing (lintProject throws → failureReport, :1140) — has no committed test asserting browserSkipped: true. It's structurally safe (shares emptyBrowserResult(); the required field can't be undefined), so this is genuine test-debt rather than a latent bug — a one-line test with a throwing lintProject dep would close the last of the three documented paths. (Same shape, lower stakes: the printed warning line in printHumanReport is also unasserted — cosmetic.)

I confirmed the assertions are load-bearing by hand: hardcoding browserSkipped: false in buildReport fails both the lint-block and throw tests, so they bind the field (not mock-echo). I did not execute the suite locally (repo not provisioned in this sandbox; per your notes check.test.ts runs 61/61 for you and the full-package failures are pre-existing/unrelated) — this rests on static trace + your revert run.

Minor

  • browserSkipped is whole-session granularity — a --no-contrast run still reports browserSkipped: false with an empty contrast section. That's correct (the browser did run, and the skip was user-initiated), just worth knowing the field answers "did the session run," not "was every audit exercised." No change needed.

Not approving here — posting as a comment (I hold review, not stamp authority on this channel trigger). Clean on my pass; the note is test debt, your call whether to close it in this PR.

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.

2 participants