Skip to content

feat(check): name the saved report file and print one stderr line per check step - #7344

Merged
miguel-heygen merged 7 commits into
mainfrom
fix/check-progress-every-step
Nov 7, 2026
Merged

miguel-heygen merged 7 commits into
mainfrom
fix/check-progress-every-step

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

What

check now names the file it saves its report to on stderr (at start, and again after a --json run), and prints one line on stderr when it enters each step (starting Chrome, loading the page, waiting for the page to be ready, ...) and repeats still <step> (N s in this step) every 30 s while a step runs. Each step prints once, however often it is re-entered. stdout stays pure JSON. The check reference doc now says to run npx hyperframes check, never through npm run.

Why

A check --json run printed nothing for minutes. The existing check: samples 3/10 lines only start once sampling begins; loading the page and waiting for it to be ready (the 45 s render-ready wait and more) print nothing, so a slow scene looks hung to a person or to an agent with a tool timeout.

Why the report file

Under npm run check, a tool timeout's SIGTERM makes npm exit 143 at once with only its own header on stdout, while check goes on and writes its report to a pipe nobody reads. The report was already saved under .hyperframes/artifacts/checks/, but nobody was told where. Measured on a real run: TERM at 40 s, npm exit 143 with 109 bytes on stdout, and the saved file was complete JSON.

How

CheckDeadline.enter is the one place every step passes through, so it prints the line; start() runs a 30 s unref'd interval that close() and expire() clear. The copy step is left out: startCopyHeartbeat in checkBrowser.ts already reports its own percent (searched heartbeat under packages/cli/src/utils; runningCli.ts is an unrelated process heartbeat).

Size

A small change in one class, the check command, their tests and one doc line, separate from the word-count fix because it has a different root. No open PR touches checkDeadline.ts.

Test plan

  • Unit tests added in checkDeadline.test.ts (fake timers; one line per step re-entered in a loop) and check.test.ts (exit line names the saved file).
  • TERM under npm run check on a real WebGL fixture leaves a complete report file; its path was printed on stderr at start and exit.
  • fails without the fix: the new test fails on main's checkDeadline.ts.
  • checkDeadline, checkDeadline.process, checkPipeline.reportingProgress, checkBrowser and commands/check test files: 258 passed, 3 runs in a row.
  • oxlint and oxfmt clean on both files; comment share of checkDeadline.ts unchanged at 0.46%.

@heygengenesis heygengenesis Bot 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.

Request changes at 49d530fe.

Blocker: it prints a line on every enter() call, not once per step. enter() is called as a progress ping inside loops, so stderr gets flooded:

  • checkPipeline.ts:574-576: two calls per layout sample (enter("samples", "layout sample at …s"), then enter("samples")). The existing check: samples n/N line also prints for each sample.
  • checkPipeline.ts:1536: reportingProgress calls enter("analysis", name) on every driver method call.
  • checkPipeline.ts:615: once per overlap detail. checkBrowser.ts:740/747: once per frame and once per inspect target.

I measured it on the head with the same call pattern (start, settle, 40 samples, 5 driver calls): 87 stderr lines, alternating checking samples (layout sample at 0s) / checking samples …. A real check with hundreds of samples and driver calls prints far more. That's the opposite of "one line per step", and it's a lot of noise for an agent reading stderr under a token budget.

Suggested fix: in enter(), announce only when the phase changes. For example, read const changed = this.work?.phase !== phase before reassigning this.work, then if (changed) this.announce(…). The 30 s heartbeat can still show work.detail so a long sample stays identifiable. Add a test that calls enter("samples", …) repeatedly and expects one line.

Verified otherwise:

  • The 5 named suites (checkDeadline, checkDeadline.process, checkPipeline.reportingProgress, checkBrowser, commands/check) at head: 256 passed.
  • fails without the fix: holds. With main's checkDeadline.ts, "names each step and repeats it every 30 s on stderr…" fails.
  • The interval is unref'd and cleared in both close() and expire(), stdout stays JSON, and leaving out copies is consistent with startCopyHeartbeat.

@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 267 (base branch 267), smooth 94 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

Unstable (2)

  • seqplainkeys-none-pct-r0-nested-z100: tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo false, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true
  • seqab-none-pct-r0-root-z100: tracking 0.05, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

@miguel-heygen miguel-heygen changed the title feat(check): every check step prints a line on stderr and repeats it while it runs long feat(check): name the saved report file and print one stderr line per check step Nov 7, 2026

@heygengenesis heygengenesis Bot 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.

Approve at 04cc554b.

My earlier blocker is fixed. enter() now prints only when the phase changes, so per-sample, per-driver-call and per-frame re-entries are silent. "names a step once when it is entered again in a loop" pins this.

I also reviewed the scope added since 49d530fe:

  • The report id is now created before the run, so the path is printed at start. saveCheckReport writes to that same id (wx). Under --json, the saved path is printed on stderr and stdout stays pure JSON.
  • The npx hyperframes check doc line is consistent with that.

Verified at this head:

  • checkDeadline, checkDeadline.process, checkPipeline.reportingProgress, checkBrowser, commands/check: 258 passed.
  • fails without the fix: holds. With main's checkDeadline.ts and check.ts, all three new tests fail.

Non-blocking:

  • The start line says the report "is saved … when the check ends", but when captureArtifactRevision or the save fails, no file is written. The Check report was not recorded warning still goes to stderr, so the reader is told. Softening the wording ("is saved to … unless a 'not recorded' warning follows") would close the gap.
  • The test plan's "256 passed" is now 258, and "Size: one class plus one test" now covers check.ts, its test and a doc too.

@heygengenesis heygengenesis Bot 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.

Approve at 92210d97.

Since my approval at 04cc554b, two changes:

  • check.ts: the start line now reads … when the check ends, unless a "not recorded" warning follows. This addresses my non-blocking nit: it matches warnNotRecorded's Check report was not recorded on stderr.
  • skills-manifest.json: new hyperframes-cli hash for the doc line. Running bun packages/cli/scripts/gen-skills-manifest.ts at this head leaves the tree clean, so the hash is correct.

Verified at this head:

  • checkDeadline.test.ts + commands/check.test.ts: 178 passed. No test asserts the start-line text.
  • The fails without the fix: result from my last review still holds, since no test or logic changed.

@heygengenesis heygengenesis Bot 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.

Approve at d25add63.

Since my approval at 92210d97, one line changed: the start line now ends unless a warning says it could not be saved. That's needed. artifactCapture.test.ts:106 asserts that a successful run's stderr does not contain not recorded, so the previous wording would have tripped it on every run.

Verified at this head:

  • CI for d25add63 per the Actions API: all 8 workflows completed successfully (CI, CodeQL, regression, preview-regression, Windows render verification, Player perf, PR captures, Comments).
  • Locally, checkDeadline, commands/check and checkArtifacts: all pass. artifactCapture's spawned-CLI case also fails on main in my sandbox (the run never starts, so stderr is undefined), so that failure is my environment, not this PR. CI runs it green.
  • No logic or test changed, so the fails without the fix: result from my earlier reviews still holds.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 7, 2026
@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 7, 2026
Merged via the queue into main with commit 9ac2a23 Nov 7, 2026
84 checks passed
@miguel-heygen
miguel-heygen deleted the fix/check-progress-every-step branch November 7, 2026 07:04
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 7, 2026
…-com#7336-heygen-com#7358

Adopt heygen-com#7304 (without CLI renderDiagnostic, needs heygen-com#6725), heygen-com#7331, heygen-com#7332, heygen-com#7334,
heygen-com#7337, heygen-com#7336, heygen-com#7339, heygen-com#7289, heygen-com#5594, heygen-com#6900, heygen-com#7322, heygen-com#7342, heygen-com#7343, heygen-com#7350 (onProgress
only), heygen-com#7352, heygen-com#7348, heygen-com#7344, heygen-com#7353, heygen-com#7356 and heygen-com#7355. Defer heygen-com#5708 (needs heygen-com#6911) and
heygen-com#7345 (READ_VERSION 4 vs 7); heygen-com#7307 not applicable. Re-read 14 moved pins, pin 6
new PRs (monitored 126). Make checkReportFile module-private and suppress the
cross-package closeTestChrome duplicate for fallow.

Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com>
Co-authored-by: SophiaHuangCc <129631224+SophiaHuangCc@users.noreply.github.com>
Co-authored-by: Dante <duanjl.china@gmail.com>
Co-authored-by: Muzzy5150 <muzzy5150yt@gmail.com>
Co-authored-by: ump45nose <52391318+ump45nose@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant