Skip to content

fix: preserve JavaScript errors and handle closed output pipes - #31

Open
254808127 wants to merge 1 commit into
mainfrom
fix/action-errors-and-broken-pipe
Open

254808127 wants to merge 1 commit into
mainfrom
fix/action-errors-and-broken-pipe

Conversation

@254808127

Copy link
Copy Markdown
Collaborator

Summary

Fix two CLI error-reporting defects without changing action semantics or the JSON envelope:

  • Preserve the JavaScript exception summary from Runtime.evaluate, rather than reducing every error to Uncaught. Include optional one-based line/column positions. Keep cdp_error and exit status 1.
  • Replace panic-prone stdout/stderr printing with fallible writes and an explicit flush. A closed stdout reader is benign only after the requested operation succeeded. Other output errors and all operation failures still exit 1, even when stderr is unavailable.

The exception summary is bounded to 1024 Unicode characters (plus a truncation marker). Stack traces, source URLs, evaluated expressions and remote object previews are not appended. Page-provided error text can itself contain sensitive information; the README documents this limitation.

Regression coverage

Before the production change, the new tests reproduced both defects: CLI/SDK evaluation returned only Uncaught, and closed stdout or stderr produced panic exit 101.

  • Deterministic CDP fixtures verify error details reach the CLI for eval, click and fill, with no retry and unchanged failure envelopes.
  • A gated local HTTP response ensures the output reader is already closed before the real CLI process emits a result; no race with a fast version command.
  • Writer tests cover write/flush BrokenPipe, other I/O failures, failed stderr reporting and JSON escaping.
  • Exception tests cover missing fields, primitive throws, numeric position bounds, Unicode truncation and omission of stacks/remote metadata.
  • An opt-in real headless-browser regression covers missing-selector click/fill, TypeError, SyntaxError, a thrown string and a rejected Promise. A normal evaluation succeeds after each error.

Local verification

On Windows, Rust 1.94.0:

  • cargo fmt --all -- --check: passed.
  • cargo test --all-targets --locked: 54 passed, 2 opt-in browser tests skipped in this default run.
  • cargo test --all-targets --locked --offline with deliberately unusable inherited HTTP/HTTPS/ALL proxies: 54 passed.
  • cargo test --locked --test page_targets_browser -- --ignored --nocapture: 2 passed, using a separate HeadlessChrome 145.0.7632.6 profile and loopback fixtures only.
  • cargo clippy --all-targets --locked -- -D warnings: passed.
  • git diff --check: passed.

Self-review completed before submission: checked failure exit codes, output compatibility, bounded error text, isolated fixtures and unchanged proxy/page-target behavior. No blocking findings remain.

Based on current main (b41e085, version 1.2.1). No dependency, version, release workflow or Skill changes. No cloud sessions or product test machines were used; no release or deployment is included.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-20T11:27:09.560697Z 2f23b0e PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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