morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #5172] fix(adb): reuse shared PNG header validation #6799

New issue

Open

opened 2026-11-01 21:43:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:43:06 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5172
Author: @Radiummm
Created: 10/19/2026
Status: 🔄 Open

Base: main ← Head: fix/adb-shared-png-header

📝 Commits (1)

5d5535a fix(adb): reuse shared PNG header validation

📊 Changes

3 files changed (+42 additions, -26 deletions)

View changed files

📝 src/adb/AdbProvider.test.ts (+39 -0)
📝 src/adb/AdbProvider.ts (+3 -3)
📝 src/adb/AdbShellOutput.ts (+0 -23)

📄 Description

Summary

ADB screenshots now use the shared PNG header reader already used by browser and native-UI screenshots. Invalid IHDR types/lengths and zero or out-of-range dimensions return unreadable_output before screen.png is published.

Problem and expected behavior

The ADB-specific parser checked only the PNG signature and a 24-byte minimum. A signature followed by a JUNK chunk was accepted as a successful 4×3 screenshot. Zero dimensions and dimensions above PNG’s 2³¹−1 limit were also accepted. No issue is linked; this was found by source review and reproduced with the provider’s subprocess-backed ADB test stub.

Change and scope

Call readPngHeaderDimensions at the screenshot acquisition boundary and delete the single-caller duplicate parsePngDimensions. Existing output schemas and error projection are preserved. This remains header validation, without image decoding, CRC verification, or full PNG completeness claims. Ordinary fix within existing ADB authority; advance maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner: src/domain/pngHeader.ts; ADB delegates header interpretation to it.
CLI/MCP contracts and generated/package metadata: unchanged.
Existing workflow still runs adb exec-out screencap -p and publishes accepted bytes with their digest. Rejected headers produce no screenshot file. Target selection, cancellation, exclusive publication, and cleanup paths are unchanged.
Expected existing workflow effects: process launch, device communication, reading capture bytes, and writing accepted screenshots. No additional effects are introduced.

Evidence and regression coverage

Added seven provider-level regressions: wrong IHDR type, ASCII-folded type, wrong IHDR length, zero width/height, and oversized width/height. Each asserts unreadable_output and absence of screen.png.
Red: all seven regressions failed on the base implementation because the provider returned success.
Green: all 126 tests across AdbProvider.test.ts, AdbShellOutput.test.ts, and pngHeader.test.ts passed, including existing successful capture, cancellation, and existing-file preservation tests.
Real Android behavior is unverified: adb devices -l found no connected device, so npm run verify:adb was not run. The subprocess stub establishes the provider boundary behavior, not real-device compatibility.

Validation performed

macOS arm64, pinned Node.js 24.18.0 and npm 11.16.0:

npm run test:focused -- src/adb/AdbProvider.test.ts src/adb/AdbShellOutput.test.ts src/domain/pngHeader.test.ts — 3 files, 126 tests passed.
npm run check — passed (typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards).
git diff --check — passed.

Compatibility, safety, and release

No schema, dependency, installer, or generated metadata changes. Malformed screenshot headers previously reported as successful are now rejected using the existing protocol-error path. No full-image validation is claimed; accepted screenshot bytes and digests are preserved.

Author readiness

Human owner: @Radiummm

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem and has one focused outcome.
Ordinary fix within existing boundaries; no new capability or authority.
Regression coverage demonstrates the original failure and corrected behavior.
Human owner reviews the change and validation gaps before requesting maintainer review.

_{🔄 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/5172 **Author:** [@Radiummm](https://github.com/Radiummm) **Created:** 10/19/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/adb-shared-png-header` --- ### 📝 Commits (1) - [`5d5535a`](https://github.com/morluto/rea/commit/5d5535ab5dbd10f76356f687dad635de25855234) fix(adb): reuse shared PNG header validation ### 📊 Changes **3 files changed** (+42 additions, -26 deletions) <details> <summary>View changed files</summary> 📝 `src/adb/AdbProvider.test.ts` (+39 -0) 📝 `src/adb/AdbProvider.ts` (+3 -3) 📝 `src/adb/AdbShellOutput.ts` (+0 -23) </details> ### 📄 Description ## Summary ADB screenshots now use the shared PNG header reader already used by browser and native-UI screenshots. Invalid IHDR types/lengths and zero or out-of-range dimensions return `unreadable_output` before `screen.png` is published. ## Problem and expected behavior The ADB-specific parser checked only the PNG signature and a 24-byte minimum. A signature followed by a `JUNK` chunk was accepted as a successful 4×3 screenshot. Zero dimensions and dimensions above PNG’s 2³¹−1 limit were also accepted. No issue is linked; this was found by source review and reproduced with the provider’s subprocess-backed ADB test stub. ## Change and scope Call `readPngHeaderDimensions` at the screenshot acquisition boundary and delete the single-caller duplicate `parsePngDimensions`. Existing output schemas and error projection are preserved. This remains header validation, without image decoding, CRC verification, or full PNG completeness claims. Ordinary fix within existing ADB authority; advance maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner: `src/domain/pngHeader.ts`; ADB delegates header interpretation to it. - CLI/MCP contracts and generated/package metadata: unchanged. - Existing workflow still runs `adb exec-out screencap -p` and publishes accepted bytes with their digest. Rejected headers produce no screenshot file. Target selection, cancellation, exclusive publication, and cleanup paths are unchanged. - Expected existing workflow effects: process launch, device communication, reading capture bytes, and writing accepted screenshots. No additional effects are introduced. ## Evidence and regression coverage - Added seven provider-level regressions: wrong IHDR type, ASCII-folded type, wrong IHDR length, zero width/height, and oversized width/height. Each asserts `unreadable_output` and absence of `screen.png`. - Red: all seven regressions failed on the base implementation because the provider returned success. - Green: all 126 tests across `AdbProvider.test.ts`, `AdbShellOutput.test.ts`, and `pngHeader.test.ts` passed, including existing successful capture, cancellation, and existing-file preservation tests. - Real Android behavior is unverified: `adb devices -l` found no connected device, so `npm run verify:adb` was not run. The subprocess stub establishes the provider boundary behavior, not real-device compatibility. ## Validation performed macOS arm64, pinned Node.js 24.18.0 and npm 11.16.0: - `npm run test:focused -- src/adb/AdbProvider.test.ts src/adb/AdbShellOutput.test.ts src/domain/pngHeader.test.ts` — 3 files, 126 tests passed. - `npm run check` — passed (typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards). - `git diff --check` — passed. ## Compatibility, safety, and release No schema, dependency, installer, or generated metadata changes. Malformed screenshot headers previously reported as successful are now rejected using the existing protocol-error path. No full-image validation is claimed; accepted screenshot bytes and digests are preserved. ## Author readiness Human owner: @Radiummm - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem and has one focused outcome. - [x] Ordinary fix within existing boundaries; no new capability or authority. - [x] Regression coverage demonstrates the original failure and corrected behavior. - [ ] Human owner reviews the change and validation gaps before requesting maintainer review. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-11-01 21:43:06 +00:00

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#6799

No description provided.

Rows
Columns