[PR #2839] [CLOSED] fix(browser): classify malformed screenshot payloads as protocol errors #5429

Closed
opened 2026-10-31 01:06:33 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2839
Author: @yasshmittal
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/browser-screenshot-malformed-reason


📝 Commits (6)

  • c4142ec fix(browser): classify malformed screenshot payloads as protocol errors
  • ee0256c fix(ci): skip report merge after failed shards
  • c7fc4bb fix(ci): merge failed shard diagnostics
  • fce1816 Merge branch 'main' into fix/browser-screenshot-malformed-reason
  • 608aa06 Merge branch 'main' into fix/browser-screenshot-malformed-reason
  • eed8d3c Merge branch 'main' into fix/browser-screenshot-malformed-reason

📊 Changes

2 files changed (+29 additions, -1 deletions)

View changed files

📝 src/browser/CdpScreenshot.ts (+1 -1)
📝 tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts (+28 -0)

📄 Description

Summary

Fixes #2595.

A Page.captureScreenshot reply without image data, or with base64 that does not decode canonically, was reported as payload_limit, the reason used for bounded transport truncation. Callers could not distinguish unusable producer output from an actual size limit.

Problem and expected behavior

src/browser/CdpScreenshot.ts threw payload_limit when data was missing or rejected by decodeCanonicalBase64. That decoder applies no size constraint; real size limits are enforced by the CDP transport (CdpConnection), which reports its own payload_limit. A malformed reply should be protocol_error, as the adjacent invalid-PNG-header check already reports.

Change and scope

Missing, non-string, invalid and non-canonical screenshot data now report protocol_error. Transport size limits keep payload_limit.

Contribution scope: an ordinary fix within existing boundaries; no maintainer scope decision applies.

Contract and boundary impact

  • Semantic owner and earliest changed stage: screenshot payload decoding in src/browser/CdpScreenshot.ts.

  • Contribution scope fit and linked maintainer decision: not applicable (bug fix).

  • CLI and MCP/tool-catalog contract: failure reason changes from payload_limit to protocol_error for malformed replies only.

  • Provider, bridge, target-format, or platform compatibility: none.

  • Evidence, artifact, provenance, or reconstruction contract: unchanged; no partial screenshot is returned in either case.

  • Target selection, authority, upstream behavior, and cleanup details: unchanged.

  • Generated metadata, package, or installation impact: none.

  • Read existing files or captured data.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts. Through the fake CDP browser and CdpBrowserProvider.captureScreenshot, four replies each produce protocol_error:

    • missing data
    • non-string data
    • invalid base64
    • non-canonical base64
  • Base reproduction or other evidence: all four fail on the base revision with payload_limit.

  • User-visible CLI/MCP output: reason: "protocol_error" for malformed screenshot replies.

  • Remaining proof gaps: no real Chrome run; the change concerns classification of CDP replies.

  • Observed, derived, and inferred claims remain distinguishable.

  • Artifact identity, source provenance, and failed attempts remain preserved.

  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Host: Windows 11, Node.js 24.21.0.

  • npx vitest run tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts: 9 passed (4 new cases fail on the base revision).
  • npx vitest run tests/boundary/browser/cdpBrowserProvider.document_script.test.ts src/browser: 345 passed.
  • npm run check: passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: Windows only; fake CDP browser.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: classification change only.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code


🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2839 **Author:** [@yasshmittal](https://github.com/yasshmittal) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/browser-screenshot-malformed-reason` --- ### 📝 Commits (6) - [`c4142ec`](https://github.com/morluto/rea/commit/c4142ec8730ad896c4f0b1b7808bdf54c4e3a10b) fix(browser): classify malformed screenshot payloads as protocol errors - [`ee0256c`](https://github.com/morluto/rea/commit/ee0256c740c9be18baae6c56d90285df8069f5b4) fix(ci): skip report merge after failed shards - [`c7fc4bb`](https://github.com/morluto/rea/commit/c7fc4bba9f6c372db55fb0f776d0ed331a50b4e5) fix(ci): merge failed shard diagnostics - [`fce1816`](https://github.com/morluto/rea/commit/fce181682bb1d88990df97524f044116952ad547) Merge branch 'main' into fix/browser-screenshot-malformed-reason - [`608aa06`](https://github.com/morluto/rea/commit/608aa06c83c8b7b05b001f8bdedbc5e603b803e5) Merge branch 'main' into fix/browser-screenshot-malformed-reason - [`eed8d3c`](https://github.com/morluto/rea/commit/eed8d3cd41e74162a0d7dddbe3fbe08ba4db6719) Merge branch 'main' into fix/browser-screenshot-malformed-reason ### 📊 Changes **2 files changed** (+29 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/CdpScreenshot.ts` (+1 -1) 📝 `tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts` (+28 -0) </details> ### 📄 Description ## Summary Fixes #2595. A `Page.captureScreenshot` reply without image data, or with base64 that does not decode canonically, was reported as `payload_limit`, the reason used for bounded transport truncation. Callers could not distinguish unusable producer output from an actual size limit. ## Problem and expected behavior `src/browser/CdpScreenshot.ts` threw `payload_limit` when `data` was missing or rejected by `decodeCanonicalBase64`. That decoder applies no size constraint; real size limits are enforced by the CDP transport (`CdpConnection`), which reports its own `payload_limit`. A malformed reply should be `protocol_error`, as the adjacent invalid-PNG-header check already reports. ## Change and scope Missing, non-string, invalid and non-canonical screenshot data now report `protocol_error`. Transport size limits keep `payload_limit`. Contribution scope: an ordinary fix within existing boundaries; no maintainer scope decision applies. ## Contract and boundary impact - Semantic owner and earliest changed stage: screenshot payload decoding in `src/browser/CdpScreenshot.ts`. - Contribution scope fit and linked maintainer decision: not applicable (bug fix). - CLI and MCP/tool-catalog contract: failure reason changes from `payload_limit` to `protocol_error` for malformed replies only. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: unchanged; no partial screenshot is returned in either case. - Target selection, authority, upstream behavior, and cleanup details: unchanged. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts`. Through the fake CDP browser and `CdpBrowserProvider.captureScreenshot`, four replies each produce `protocol_error`: - missing `data` - non-string `data` - invalid base64 - non-canonical base64 - Base reproduction or other evidence: all four fail on the base revision with `payload_limit`. - User-visible CLI/MCP output: `reason: "protocol_error"` for malformed screenshot replies. - Remaining proof gaps: no real Chrome run; the change concerns classification of CDP replies. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Host: Windows 11, Node.js 24.21.0. - `npx vitest run tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts`: 9 passed (4 new cases fail on the base revision). - `npx vitest run tests/boundary/browser/cdpBrowserProvider.document_script.test.ts src/browser`: 345 passed. - `npm run check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Windows only; fake CDP browser. - Package or release metadata impact: none. - Security, privacy, process, or containment review: classification change only. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:06:33 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#5429
No description provided.