[PR #5256] [MERGED] fix(scripts): remove the ADB verifier's pushed probe file from the device #6842

Closed
opened 2026-11-01 21:43:27 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5256
Author: @musaraza004
Created: 10/19/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: fix/adb-verifier-probe-cleanup


📝 Commits (2)

  • 0df2071 fix(scripts): remove the ADB verifier's pushed probe file from the device
  • 0524ad5 test(scripts): cover ADB probe cleanup after pull failure and leftover probes

📊 Changes

2 files changed (+216 additions, -36 deletions)

View changed files

📝 scripts/verify-adb.mjs (+83 -24)
📝 tests/boundary/process/adbVerifierCleanup.test.ts (+133 -12)

📄 Description

Summary

Fixes #5190.

npm run verify:adb pushed its own transfer probe to /data/local/tmp/rea-verify-<timestamp>.txt and never deleted it. Each successful run reported all adb device checks passed and left another probe on the device. The verifier now removes that exact file before it can report success.

Problem and expected behavior

  • Trigger: any verifier run where the transfer push succeeds.
  • Before: the only cleanup in the transfer phase removed host temp directories. pull_adb_file keeps the device original by design, and provider.close() doesn't own device paths. Nothing removed the device file.
  • Expected: once the verifier has pushed the probe itself, it deletes it, confirms it's gone, and shows any cleanup failure as a failed check.

Change and scope

  • The push/pull roundtrip now lives in a verifyTransferRoundtrip helper. After a successful overwrite: false push, it always calls removeDeviceProbe in a finally, so the probe is removed even if the pull fails or throws. A failed push removes nothing: with overwrite: false, the verifier can't prove it owns a file that already existed.
  • removeDeviceProbe runs <adb> -s <serial> shell rm -- <exact path> with no shell on the host. <adb> is the client path that inspect_adb_client reported, i.e. the same caller-selected binary the provider uses. It then confirms the file is absent with list_adb_directory. A missing client path, a failed rm, or a probe still listed is reported as [FAIL] verifier transfer probe removed, so the run exits non-zero.
  • Not changed: provider and service contracts. I didn't add a device-delete operation, because that would be a new mutation capability needing a maintainer scope decision. pull_adb_file still keeps device originals. Scope decision: not applicable (this cleans up a file the developer verifier created itself, using the adb client the verifier already uses).

Contract and boundary impact

  • Semantic owner and earliest changed stage: the verifier owns its probe; the transfer phase of scripts/verify-adb.mjs.
  • Contribution scope fit and linked maintainer decision: ordinary fix, not applicable.
  • CLI and MCP/tool-catalog contract: none.
  • Provider, bridge, target-format, or platform compatibility: none to the provider. The verifier now also runs adb shell rm on its own probe path.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details: removal only targets the exact timestamped path from a successful no-overwrite push. Wildcards are never used. A cleanup failure leaves the file in place and fails the run.
  • Generated metadata, package, or installation impact: none.

Expected effects:

  • 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. (Deletes the verifier's own probe file on the device.)
  • 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.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/process/adbVerifierCleanup.test.ts still runs the real verifier script against a stubbed service and provider. The stub now uses a simulated device directory and a fake POSIX adb that logs its argv.
    • New: success removes exactly one probe (-s test shell rm -- /data/local/tmp/rea-verify-<digits>.txt) and leaves the device directory empty.
    • New: a failed removal exits 1 with [FAIL] verifier transfer probe removed — <path>: …, never prints the success line, keeps the probe, and still closes the provider.
    • New (from review): a pull error and a pull throw both still run the exact rm and leave the device empty. A rm that exits 0 but leaves the probe listed fails the run.
    • The fake adb is a shell script, so the success-path cases skip on Windows. The throw/close-failure cases still run there. This file isn't in the Windows CI lane's explicit list.
  • Base reproduction (executed): with the new tests against unmodified main, 2 of 7 failed. The success case made no rm call, and the failure case exited 0.
  • Remaining proof gaps: I didn't run against a real adb client, device or emulator. Real adb shell exit-status propagation and /data/local/tmp permissions are unverified here.

Validation performed

All run in a node:24 Linux container with npm ci --ignore-scripts:

  • npx vitest run --project process-boundary tests/boundary/process/adbVerifierCleanup.test.ts: on base main, 2 failed and 5 passed. With the fix and the review cases, 10 passed. Two mutants each fail exactly one test: dropping the finally fails the pull-throw case, and dropping the listing check fails the leftover-probe case.
  • npx oxfmt --check on both files: clean.
  • npx oxlint on both files: 0 errors and 11 max-depth warnings, down from 12 on base verify-adb.mjs alone.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. A verifier run whose probe removal fails now exits non-zero.
  • Real Hopper/Ghidra, browser, or OS coverage: none. No real adb or device was used.
  • Package or release metadata impact: none. The verifier is a development script.
  • Security, privacy, process, or containment review: execFile passes argv directly with no host shell. The device path is the verifier-generated /data/local/tmp/rea-verify-<digits>.txt.

Author readiness

Human owner: @musaraza004

  • 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 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/5256 **Author:** [@musaraza004](https://github.com/musaraza004) **Created:** 10/19/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/adb-verifier-probe-cleanup` --- ### 📝 Commits (2) - [`0df2071`](https://github.com/morluto/rea/commit/0df207179fe7d793adec9aff9e80857823da045e) fix(scripts): remove the ADB verifier's pushed probe file from the device - [`0524ad5`](https://github.com/morluto/rea/commit/0524ad578b430e3535646b71776c2372b1b79485) test(scripts): cover ADB probe cleanup after pull failure and leftover probes ### 📊 Changes **2 files changed** (+216 additions, -36 deletions) <details> <summary>View changed files</summary> 📝 `scripts/verify-adb.mjs` (+83 -24) 📝 `tests/boundary/process/adbVerifierCleanup.test.ts` (+133 -12) </details> ### 📄 Description ## Summary Fixes #5190. `npm run verify:adb` pushed its own transfer probe to `/data/local/tmp/rea-verify-<timestamp>.txt` and never deleted it. Each successful run reported `all adb device checks passed` and left another probe on the device. The verifier now removes that exact file before it can report success. ## Problem and expected behavior - Trigger: any verifier run where the transfer push succeeds. - Before: the only cleanup in the transfer phase removed host temp directories. `pull_adb_file` keeps the device original by design, and `provider.close()` doesn't own device paths. Nothing removed the device file. - Expected: once the verifier has pushed the probe itself, it deletes it, confirms it's gone, and shows any cleanup failure as a failed check. ## Change and scope - The push/pull roundtrip now lives in a `verifyTransferRoundtrip` helper. After a successful `overwrite: false` push, it always calls `removeDeviceProbe` in a `finally`, so the probe is removed even if the pull fails or throws. A failed push removes nothing: with `overwrite: false`, the verifier can't prove it owns a file that already existed. - `removeDeviceProbe` runs `<adb> -s <serial> shell rm -- <exact path>` with no shell on the host. `<adb>` is the client path that `inspect_adb_client` reported, i.e. the same caller-selected binary the provider uses. It then confirms the file is absent with `list_adb_directory`. A missing client path, a failed `rm`, or a probe still listed is reported as `[FAIL] verifier transfer probe removed`, so the run exits non-zero. - Not changed: provider and service contracts. I didn't add a device-delete operation, because that would be a new mutation capability needing a maintainer scope decision. `pull_adb_file` still keeps device originals. Scope decision: not applicable (this cleans up a file the developer verifier created itself, using the adb client the verifier already uses). ## Contract and boundary impact - Semantic owner and earliest changed stage: the verifier owns its probe; the transfer phase of `scripts/verify-adb.mjs`. - Contribution scope fit and linked maintainer decision: ordinary fix, not applicable. - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none to the provider. The verifier now also runs `adb shell rm` on its own probe path. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: removal only targets the exact timestamped path from a successful no-overwrite push. Wildcards are never used. A cleanup failure leaves the file in place and fails the run. - Generated metadata, package, or installation impact: none. Expected effects: - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [x] Change process memory, registers, files, or configuration. (Deletes the verifier's own probe file on the device.) - [ ] 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. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/process/adbVerifierCleanup.test.ts` still runs the real verifier script against a stubbed service and provider. The stub now uses a simulated device directory and a fake POSIX `adb` that logs its argv. - New: success removes exactly one probe (`-s test shell rm -- /data/local/tmp/rea-verify-<digits>.txt`) and leaves the device directory empty. - New: a failed removal exits 1 with `[FAIL] verifier transfer probe removed — <path>: …`, never prints the success line, keeps the probe, and still closes the provider. - New (from review): a pull error and a pull throw both still run the exact `rm` and leave the device empty. A `rm` that exits 0 but leaves the probe listed fails the run. - The fake `adb` is a shell script, so the success-path cases skip on Windows. The `throw`/close-failure cases still run there. This file isn't in the Windows CI lane's explicit list. - Base reproduction (executed): with the new tests against unmodified `main`, 2 of 7 failed. The success case made no `rm` call, and the failure case exited 0. - Remaining proof gaps: I didn't run against a real adb client, device or emulator. Real `adb shell` exit-status propagation and `/data/local/tmp` permissions are unverified here. ## Validation performed All run in a `node:24` Linux container with `npm ci --ignore-scripts`: - `npx vitest run --project process-boundary tests/boundary/process/adbVerifierCleanup.test.ts`: on base `main`, 2 failed and 5 passed. With the fix and the review cases, 10 passed. Two mutants each fail exactly one test: dropping the `finally` fails the pull-throw case, and dropping the listing check fails the leftover-probe case. - `npx oxfmt --check` on both files: clean. - `npx oxlint` on both files: 0 errors and 11 `max-depth` warnings, down from 12 on base `verify-adb.mjs` alone. ## Compatibility, safety, and release - Breaking changes or migration steps: none. A verifier run whose probe removal fails now exits non-zero. - Real Hopper/Ghidra, browser, or OS coverage: none. No real adb or device was used. - Package or release metadata impact: none. The verifier is a development script. - Security, privacy, process, or containment review: `execFile` passes argv directly with no host shell. The device path is the verifier-generated `/data/local/tmp/rea-verify-<digits>.txt`. ## Author readiness Human owner: @musaraza004 - [x] 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. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## 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-11-01 21:43:27 +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#6842
No description provided.