morluto/rea

Fork 0

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

[PR #2344] [MERGED] fix(adb): keep the device's refusal when a package change exits nonzero #5178

New issue

Closed

opened 2026-10-31 01:04:34 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:34 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2344
Author: @huiq777
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/adb-install-failure-verdict

📝 Commits (2)

895d05f fix(adb): keep the device's refusal when a package change exits nonzero
355ff87 test(adb): assert retained package refusal reasons

📊 Changes

3 files changed (+105 additions, -4 deletions)

View changed files

📝 src/adb/AdbProvider.test.ts (+62 -0)
📝 src/adb/AdbProvider.ts (+36 -2)
📝 src/adb/AdbShellOutput.ts (+7 -2)

📄 Description

Summary

When a device refuses an install or uninstall, REA reports "unreadable output" and drops the device's reason. install_adb_package and uninstall_adb_package only keep the Failure [...] verdict when adb exits 0, which adb does not do for a refusal.

No issue is linked: I found this while reading how adb failures are classified, and it is an ordinary fix inside the existing ADB provider.

Problem and expected behavior

Smallest trigger: any refused package change, for example installing a package that is already present.

adb exits nonzero for a refusal. On AOSP android11-release:

adb install: adb/client/adb_install.cpp prints adb: failed to install <file>: <status line> to stderr and returns 1 whenever the status line is not Success.
adb uninstall: adb returns the status of cmd package uninstall, and PackageManagerShellCommand.runUninstall prints Failure [...] to stdout and returns 1.

runAdbCapture turns that nonzero exit into an AnalysisProtocolError, and both operations returned it before looking at the text:

if (!installed.ok) return installed;
const verdict = parsePackageChangeOutput(installed.value.stdout);

So parsePackageChangeOutput only ever saw zero-exit output. The existing tests cover that form (Failure [...] on stdout with exit 0), which is why this was not caught. docs/adb-device-analysis.md says installs "preserve the device's Failure [...] reason verbatim"; for a nonzero exit they did not.

Expected: the same unsupported_target refusal carrying the device's reason, whichever exit status adb used.

Change and scope

packageChangeFailureReason(output) in src/adb/AdbShellOutput.ts is now the one reader of the Failure [REASON] syntax. parsePackageChangeOutput uses it, with unchanged results.
packageChangeRefusal(...) in src/adb/AdbProvider.ts reads the verdict from a failed run's captured stdout and stderr. Install and uninstall call it before returning the run's error.
A nonzero exit without a Failure [...] verdict is left exactly as it was, so transport and protocol failures are not relabelled as device refusals. Device-state errors (offline, unauthorized, not found) are still classified earlier by runAdbCapture.

Not included: pull_adb_package's handling of a nonzero pm path, which open PR #1903 is already changing.

Follow the contribution scope.
Scope decision: not applicable. No new capability; this only changes how an existing operation reports a refusal.

Contract and boundary impact

Semantic owner and earliest changed stage: package-change output parsing in src/adb/AdbShellOutput.ts; the install and uninstall operations in src/adb/AdbProvider.ts.
Contribution scope fit and linked maintainer decision: fix within the existing ADB provider; not applicable.
CLI and MCP/tool-catalog contract: no schema change. A refused install or uninstall that exits nonzero now returns unsupported_target with details.reason instead of unreadable_output with no details.
Provider, bridge, target-format, or platform compatibility: ADB provider only. Zero-exit behavior is unchanged.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details (if applicable): unchanged. The same adb commands run; only the interpretation of their failure output changes.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

The two operations already install and remove packages on the selected device; this PR does not change what they run.

Evidence and regression coverage

Tests added or updated: three cases in src/adb/AdbProvider.test.ts using the existing adb stub: a refused install reported on stderr with exit 1, a refused uninstall reported on stdout with exit 1, and a nonzero install with no verdict, which must stay unreadable_output.
Base reproduction or other evidence (executed): without the fix the first two cases fail with unreadable_output where unsupported_target is expected. The adb and Package Manager exit behavior is source-derived from the two AOSP android11-release files linked above; I did not check other releases.

User-visible CLI/MCP output (executed with REA_ADB_PATH pointing at a stand-in adb that reports refusals as above):

Before, on fd95880, for both commands:

{
  "code": "unreadable_output",
  "message": "Analysis returned an unreadable result. Run `rea doctor` and review the reported details; repeating the unchanged request is not expected to succeed.",
  "details": null
}

After, rea install-adb-package emulator-5554 app.apk:

{
  "code": "unsupported_target",
  "message": "Unsupported target for install_adb_package at emulator-5554: INSTALL_FAILED_ALREADY_EXISTS: Attempt to re-install com.example without first uninstalling.",
  "details": {
    "operation": "install_adb_package",
    "path": "emulator-5554",
    "reason": "INSTALL_FAILED_ALREADY_EXISTS: Attempt to re-install com.example without first uninstalling."
  }
}

After, rea uninstall-adb-package emulator-5554 com.absent:

{
  "code": "unsupported_target",
  "message": "Unsupported target for uninstall_adb_package at emulator-5554: not installed for 0",
  "details": {
    "operation": "uninstall_adb_package",
    "path": "emulator-5554",
    "reason": "not installed for 0"
  }
}

Remaining proof gaps: not run against a real device or a real adb binary; the stand-in reproduces the stream and exit status that the AOSP sources describe.

For evidence-bearing changes:

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

macOS 15.6.1 arm64, Node 24.18.0, npm 11.16.0.

npm run test:local -- src/adb/AdbProvider.test.ts — 50 passed
npx vitest run src/adb — 5 files, 106 tests passed
npm run test:focused -- tests/boundary/cli/adbCli.test.ts tests/boundary/mcp/adbMcp.test.ts tests/boundary/mcp/adbStartMcp.test.ts tests/boundary/process/adbDirectoryListing.test.ts tests/boundary/providers/adb/adbBugreportPublication.test.ts — 5 files, 21 tests passed
npm run check — passed
npm run check:fast — passed

Not run: npm run check:pr, npm run docs:check (no contract or generated metadata changes), and no real-device lane.

Compatibility, safety, and release

Breaking changes or migration steps: none. Only the error code and details of an already-failing call change.
Real Hopper/Ghidra, browser, or OS coverage: not applicable.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the reason text comes from the device's own verdict and was already returned for zero-exit refusals; nothing new is read or executed.

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.

AI assistance (Claude Code, model Claude Opus 5.5) was used for investigation, implementation and running the tests locally.

🤖 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/2344 **Author:** [@huiq777](https://github.com/huiq777) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/adb-install-failure-verdict` --- ### 📝 Commits (2) - [`895d05f`](https://github.com/morluto/rea/commit/895d05f4ca31df6b3dab8944864d3cc8f4b63b1e) fix(adb): keep the device's refusal when a package change exits nonzero - [`355ff87`](https://github.com/morluto/rea/commit/355ff872ce5cc90e4e1df33837c4b829f3d02290) test(adb): assert retained package refusal reasons ### 📊 Changes **3 files changed** (+105 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/adb/AdbProvider.test.ts` (+62 -0) 📝 `src/adb/AdbProvider.ts` (+36 -2) 📝 `src/adb/AdbShellOutput.ts` (+7 -2) </details> ### 📄 Description ## Summary When a device refuses an install or uninstall, REA reports "unreadable output" and drops the device's reason. `install_adb_package` and `uninstall_adb_package` only keep the `Failure [...]` verdict when adb exits 0, which adb does not do for a refusal. No issue is linked: I found this while reading how adb failures are classified, and it is an ordinary fix inside the existing ADB provider. ## Problem and expected behavior Smallest trigger: any refused package change, for example installing a package that is already present. adb exits nonzero for a refusal. On AOSP `android11-release`: - `adb install`: [`adb/client/adb_install.cpp`](https://github.com/aosp-mirror/platform_system_core/blob/android11-release/adb/client/adb_install.cpp#L249-L254) prints `adb: failed to install <file>: <status line>` to **stderr** and returns 1 whenever the status line is not `Success`. - `adb uninstall`: adb returns the status of `cmd package uninstall`, and [`PackageManagerShellCommand.runUninstall`](https://github.com/aosp-mirror/platform_frameworks_base/blob/android11-release/services/core/java/com/android/server/pm/PackageManagerShellCommand.java#L2009-L2010) prints `Failure [...]` to **stdout** and returns 1. `runAdbCapture` turns that nonzero exit into an `AnalysisProtocolError`, and both operations returned it before looking at the text: ```ts if (!installed.ok) return installed; const verdict = parsePackageChangeOutput(installed.value.stdout); ``` So `parsePackageChangeOutput` only ever saw zero-exit output. The existing tests cover that form (`Failure [...]` on stdout with `exit 0`), which is why this was not caught. [docs/adb-device-analysis.md](https://github.com/morluto/rea/blob/main/docs/adb-device-analysis.md) says installs "preserve the device's `Failure [...]` reason verbatim"; for a nonzero exit they did not. Expected: the same `unsupported_target` refusal carrying the device's reason, whichever exit status adb used. ## Change and scope - `packageChangeFailureReason(output)` in `src/adb/AdbShellOutput.ts` is now the one reader of the `Failure [REASON]` syntax. `parsePackageChangeOutput` uses it, with unchanged results. - `packageChangeRefusal(...)` in `src/adb/AdbProvider.ts` reads the verdict from a failed run's captured stdout and stderr. Install and uninstall call it before returning the run's error. - A nonzero exit without a `Failure [...]` verdict is left exactly as it was, so transport and protocol failures are not relabelled as device refusals. Device-state errors (offline, unauthorized, not found) are still classified earlier by `runAdbCapture`. Not included: `pull_adb_package`'s handling of a nonzero `pm path`, which open PR #1903 is already changing. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Scope decision: not applicable. No new capability; this only changes how an existing operation reports a refusal. ## Contract and boundary impact - Semantic owner and earliest changed stage: package-change output parsing in `src/adb/AdbShellOutput.ts`; the install and uninstall operations in `src/adb/AdbProvider.ts`. - Contribution scope fit and linked maintainer decision: fix within the existing ADB provider; not applicable. - CLI and MCP/tool-catalog contract: no schema change. A refused install or uninstall that exits nonzero now returns `unsupported_target` with `details.reason` instead of `unreadable_output` with no details. - Provider, bridge, target-format, or platform compatibility: ADB provider only. Zero-exit behavior is unchanged. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): unchanged. The same adb commands run; only the interpretation of their failure output changes. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. The two operations already install and remove packages on the selected device; this PR does not change what they run. ## Evidence and regression coverage - Tests added or updated: three cases in `src/adb/AdbProvider.test.ts` using the existing adb stub: a refused install reported on stderr with exit 1, a refused uninstall reported on stdout with exit 1, and a nonzero install with no verdict, which must stay `unreadable_output`. - Base reproduction or other evidence (executed): without the fix the first two cases fail with `unreadable_output` where `unsupported_target` is expected. The adb and Package Manager exit behavior is source-derived from the two AOSP `android11-release` files linked above; I did not check other releases. - User-visible CLI/MCP output (executed with `REA_ADB_PATH` pointing at a stand-in adb that reports refusals as above): Before, on `fd95880`, for both commands: ```json { "code": "unreadable_output", "message": "Analysis returned an unreadable result. Run `rea doctor` and review the reported details; repeating the unchanged request is not expected to succeed.", "details": null } ``` After, `rea install-adb-package emulator-5554 app.apk`: ```json { "code": "unsupported_target", "message": "Unsupported target for install_adb_package at emulator-5554: INSTALL_FAILED_ALREADY_EXISTS: Attempt to re-install com.example without first uninstalling.", "details": { "operation": "install_adb_package", "path": "emulator-5554", "reason": "INSTALL_FAILED_ALREADY_EXISTS: Attempt to re-install com.example without first uninstalling." } } ``` After, `rea uninstall-adb-package emulator-5554 com.absent`: ```json { "code": "unsupported_target", "message": "Unsupported target for uninstall_adb_package at emulator-5554: not installed for 0", "details": { "operation": "uninstall_adb_package", "path": "emulator-5554", "reason": "not installed for 0" } } ``` - Remaining proof gaps: not run against a real device or a real adb binary; the stand-in reproduces the stream and exit status that the AOSP sources describe. For evidence-bearing changes: - [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 macOS 15.6.1 arm64, Node 24.18.0, npm 11.16.0. - `npm run test:local -- src/adb/AdbProvider.test.ts` — 50 passed - `npx vitest run src/adb` — 5 files, 106 tests passed - `npm run test:focused -- tests/boundary/cli/adbCli.test.ts tests/boundary/mcp/adbMcp.test.ts tests/boundary/mcp/adbStartMcp.test.ts tests/boundary/process/adbDirectoryListing.test.ts tests/boundary/providers/adb/adbBugreportPublication.test.ts` — 5 files, 21 tests passed - `npm run check` — passed - `npm run check:fast` — passed Not run: `npm run check:pr`, `npm run docs:check` (no contract or generated metadata changes), and no real-device lane. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only the error code and details of an already-failing call change. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the reason text comes from the device's own verdict and was already returned for zero-exit refusals; nothing new is read or executed. ## 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. AI assistance (Claude Code, model Claude Opus 5.5) was used for investigation, implementation and running the tests locally. 🤖 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:04:34 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #5179] [CLOSED] fix(browser): accept explicitly spelled default CDP ports #6803

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

No description provided.

Rows
Columns