morluto/rea

Fork 0

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

[PR #2354] [MERGED] fix(adb): report the device state when a push cannot probe the path #5182

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/adb-push-unknown-device-path

📝 Commits (1)

3d2b799 fix(adb): report the device state when a push cannot probe the path

📊 Changes

2 files changed (+38 additions, -10 deletions)

View changed files

📝 src/adb/AdbProvider.test.ts (+30 -0)
📝 src/adb/AdbProvider.ts (+8 -10)

📄 Description

Summary

push_adb_file tells the caller "the device path already exists" when the device is offline or unauthorized. With overwrite: true it goes on to push even though its existence probe established nothing.

No issue is linked: I found this while reading the push path, and it is an ordinary fix inside the existing ADB provider.

Problem and expected behavior

Before pushing, the provider runs adb shell test -e <device_path>. The outcome was classified like this:

if (existsProbe.ok) deviceExists = true;
else if (existsProbe.error instanceof AnalysisProtocolError) deviceExists = false;
else if (/* timeout, cancelled, resource */) return existsProbe;
else deviceExists = true; // authorization/refusal states surface on push instead
if (deviceExists && !input.overwrite) return err(/* already exists */);

Smallest trigger: the probe fails with error: device offline (or device unauthorized, or device '…' not found). runAdbCapture classifies those as AnalysisUnsupportedTargetError, which lands in the last branch.

With overwrite: false (the default) the comment's push never runs. The caller gets unreadable_output with "The device path already exists and overwrite was not requested", which is false and suggests retrying with overwrite: true.
With overwrite: true the push does run, after a probe that did not determine whether the path exists, and a success would report overwritten: true without having observed that.

Expected: existence is unknown, so the probe's own refusal is returned and nothing is pushed. This is what inspect_adb_device already returns for the same stderr.

Change and scope

The probe now has two understood outcomes and everything else is reported as is:

exit 0: the path exists;
a nonzero exit with no adb diagnosis (AnalysisProtocolError): test -e says the path is absent, unchanged;
any other failure: return the probe's error.

deviceExists is then simply existsProbe.ok. Timeout, cancellation and resource failures were already returned this way; the device-state refusals now join them, which removes four branches.

Not included: an unclassified nonzero probe exit that does carry stderr text is still read as "absent", as before. Tightening that to an empty-stderr exit 1 would be a separate decision.

Follow the contribution scope.
Scope decision: not applicable. This makes an existing mutation run in fewer cases.

Contract and boundary impact

Semantic owner and earliest changed stage: the push operation's existence probe 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 push whose probe is refused by adb now returns unsupported_target with adb's diagnosis instead of the "already exists" unreadable_output.
Provider, bridge, target-format, or platform compatibility: ADB provider only.
Evidence, artifact, provenance, or reconstruction contract: overwritten is now only ever reported from an observed probe result.
Target selection, authority, upstream behavior, and cleanup details (if applicable): the push no longer runs when the probe could not establish whether the path exists.
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.

push_adb_file already writes a file on the selected device; this PR only removes one case in which it did so.

Evidence and regression coverage

Tests added or updated: a table test in src/adb/AdbProvider.test.ts using the existing adb stub, for an offline and an unauthorized device without overwrite and an offline device with overwrite. Each expects unsupported_target carrying the device state, no "already exists" text, and exactly one adb device call (the probe), so no push ran.
Base reproduction or other evidence (executed): without the fix all three rows fail. The two without overwrite return unreadable_output; the one with overwrite records a push call after the failed probe.

User-visible CLI/MCP output (executed with REA_ADB_PATH pointing at a stand-in adb that answers every device command with error: device offline), for rea push-adb-file emulator-5554 payload.bin /data/local/tmp/payload.bin:

Before, on 4dd44dd:

{
  "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": {
    "operation": "push_adb_file",
    "reason": "The device path already exists and overwrite was not requested: /data/local/tmp/payload.bin"
  }
}

After:

{
  "code": "unsupported_target",
  "message": "Unsupported target for push_adb_file at /data/local/tmp/payload.bin: error: device offline",
  "details": {
    "operation": "push_adb_file",
    "path": "/data/local/tmp/payload.bin",
    "reason": "error: device offline"
  }
}

Remaining proof gaps: not run against a real device or a real adb binary.

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 to schemas. A caller that passed overwrite: true to a device adb refuses now gets the probe's refusal instead of the push's; both carry adb's diagnosis.
Real Hopper/Ghidra, browser, or OS coverage: not applicable.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the device is written to in fewer cases than before.

This touches the same two files as #2344 in different places; whichever lands second may need a trivial rebase, which I will do.

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/2354 **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-push-unknown-device-path` --- ### 📝 Commits (1) - [`3d2b799`](https://github.com/morluto/rea/commit/3d2b7991a7f42c7dd3acc4cc8b3bab8717c5fdd2) fix(adb): report the device state when a push cannot probe the path ### 📊 Changes **2 files changed** (+38 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `src/adb/AdbProvider.test.ts` (+30 -0) 📝 `src/adb/AdbProvider.ts` (+8 -10) </details> ### 📄 Description ## Summary `push_adb_file` tells the caller "the device path already exists" when the device is offline or unauthorized. With `overwrite: true` it goes on to push even though its existence probe established nothing. No issue is linked: I found this while reading the push path, and it is an ordinary fix inside the existing ADB provider. ## Problem and expected behavior Before pushing, the provider runs `adb shell test -e <device_path>`. The outcome was classified like this: ```ts if (existsProbe.ok) deviceExists = true; else if (existsProbe.error instanceof AnalysisProtocolError) deviceExists = false; else if (/* timeout, cancelled, resource */) return existsProbe; else deviceExists = true; // authorization/refusal states surface on push instead if (deviceExists && !input.overwrite) return err(/* already exists */); ``` Smallest trigger: the probe fails with `error: device offline` (or `device unauthorized`, or `device '…' not found`). `runAdbCapture` classifies those as `AnalysisUnsupportedTargetError`, which lands in the last branch. - With `overwrite: false` (the default) the comment's push never runs. The caller gets `unreadable_output` with "The device path already exists and overwrite was not requested", which is false and suggests retrying with `overwrite: true`. - With `overwrite: true` the push does run, after a probe that did not determine whether the path exists, and a success would report `overwritten: true` without having observed that. Expected: existence is unknown, so the probe's own refusal is returned and nothing is pushed. This is what `inspect_adb_device` already returns for the same stderr. ## Change and scope The probe now has two understood outcomes and everything else is reported as is: - exit 0: the path exists; - a nonzero exit with no adb diagnosis (`AnalysisProtocolError`): `test -e` says the path is absent, unchanged; - any other failure: return the probe's error. `deviceExists` is then simply `existsProbe.ok`. Timeout, cancellation and resource failures were already returned this way; the device-state refusals now join them, which removes four branches. Not included: an unclassified nonzero probe exit that does carry stderr text is still read as "absent", as before. Tightening that to an empty-stderr exit 1 would be a separate decision. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Scope decision: not applicable. This makes an existing mutation run in fewer cases. ## Contract and boundary impact - Semantic owner and earliest changed stage: the push operation's existence probe 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 push whose probe is refused by adb now returns `unsupported_target` with adb's diagnosis instead of the "already exists" `unreadable_output`. - Provider, bridge, target-format, or platform compatibility: ADB provider only. - Evidence, artifact, provenance, or reconstruction contract: `overwritten` is now only ever reported from an observed probe result. - Target selection, authority, upstream behavior, and cleanup details (if applicable): the push no longer runs when the probe could not establish whether the path exists. - 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. `push_adb_file` already writes a file on the selected device; this PR only removes one case in which it did so. ## Evidence and regression coverage - Tests added or updated: a table test in `src/adb/AdbProvider.test.ts` using the existing adb stub, for an offline and an unauthorized device without overwrite and an offline device with overwrite. Each expects `unsupported_target` carrying the device state, no "already exists" text, and exactly one adb device call (the probe), so no push ran. - Base reproduction or other evidence (executed): without the fix all three rows fail. The two without overwrite return `unreadable_output`; the one with overwrite records a `push` call after the failed probe. - User-visible CLI/MCP output (executed with `REA_ADB_PATH` pointing at a stand-in adb that answers every device command with `error: device offline`), for `rea push-adb-file emulator-5554 payload.bin /data/local/tmp/payload.bin`: Before, on `4dd44dd`: ```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": { "operation": "push_adb_file", "reason": "The device path already exists and overwrite was not requested: /data/local/tmp/payload.bin" } } ``` After: ```json { "code": "unsupported_target", "message": "Unsupported target for push_adb_file at /data/local/tmp/payload.bin: error: device offline", "details": { "operation": "push_adb_file", "path": "/data/local/tmp/payload.bin", "reason": "error: device offline" } } ``` - Remaining proof gaps: not run against a real device or a real adb binary. 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 to schemas. A caller that passed `overwrite: true` to a device adb refuses now gets the probe's refusal instead of the push's; both carry adb's diagnosis. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the device is written to in fewer cases than before. This touches the same two files as #2344 in different places; whichever lands second may need a trivial rebase, which I will do. ## 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:37 +00:00