Skip to content

fix(engine): keep capture diagnostics on stderr - #5304

Open
user-github-me wants to merge 6 commits into
heygen-com:mainfrom
user-github-me:fix/engine-diagnostics-stderr
Open

user-github-me wants to merge 6 commits into
heygen-com:mainfrom
user-github-me:fix/engine-diagnostics-stderr

Conversation

@user-github-me

@user-github-me user-github-me commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Engine browser, capture and worker diagnostics now go to stderr, keeping stdout available for the JSON report from hyperframes benchmark --json. Diagnostic text, buffering and error classification are preserved.

The current-main integration also routes the newer first-frame and session-close motion-blur reports to stderr. Existing tests verify that each report is emitted once and stdout stays clear. The existing no-console lint guard now covers the three engine diagnostic emitters; the engine reference documents the stream contract.

Validation: the three motion-blur regression cases fail before the correction and pass afterward. All 266 tests across six affected engine suites, engine build, lint, formatting, reachability, normal commit hooks and both comment checks pass. Mint documentation validation and broken-link checks also pass.

@jrusso1020 jrusso1020 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.

The direction is right and matches the producer logger, which already sends every level to stderr. I checked the two in-repo consumers of these lines (the CI page.goto complete budget step and de-gatecheck.sh), and both capture with 2>&1, so moving the lines does not break them. The problem is that the branch is 385 commits behind main, and main gained two new console.log(blur) calls in frameCapture.ts in #5991 (captureFrameToBuffer after the first frame, and closeCaptureSession). GitHub reports the PR as cleanly mergeable, so a merge keeps both calls. Then the new no-console override added here fails lint on main, and the motion-blur notice still lands on stdout ahead of the benchmark --json payload, which is the bug this PR is fixing. Please rebase onto main and switch those two calls to console.error too. Need: yes · Size: ok · Reuse: follows the producer logger's stderr contract · Simpler: already minimal.

— Rames

Comment thread .oxlintrc.json
"packages/cli/src/telemetry/**/*.ts"
"packages/cli/src/telemetry/**/*.ts",
"packages/engine/src/services/browserManager.ts",
"packages/engine/src/services/frameCapture.ts",

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.

Current main has two more console.log calls in this file that this branch predates: if (blur) console.log(blur); in captureFrameToBuffer (first-frame motion-blur notice) and in closeCaptureSession, both added in #5991. GitHub shows a clean merge, so they survive it, and then this override fails lint on main and the blur notice still goes to stdout during benchmark --json. Please rebase and route both to console.error.

@mintlify

mintlify Bot commented Oct 24, 2026

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit 5baa25a.

Approve preview

@miguel-heygen miguel-heygen 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.

Reviewed at 5baa25a. Both points from the earlier change request are addressed:

  • The two motion-blur reports that came in with #5991 now use console.error, both after the first captured frame and in closeCaptureSession.
  • I merged this head onto current main and grepped the engine sources: no console.log is left outside tests, so the new no-console override for browserManager.ts, frameCapture.ts and parallelCoordinator.ts will hold after merge.

I also rechecked the readers of these lines on current main. The CI page.goto budget step reads a log written with 2>&1, and de-gatecheck.sh redirects both streams, so moving everything to stderr does not hide anything they grep for. Nothing in the CLI or producer reassigns console.log or console.error, so no line changes visibility in a normal render. CI is green at this head and the branch merges cleanly.

Verdict: APPROVE
Reasoning: All engine diagnostics now go to stderr, including the newer motion-blur reports, the lint guard holds on the merged tree, and every in-repo consumer still sees the lines.

This branch has not been deployed

No deployments
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