[PR #7074] fix(verify): clean up failed ADB probes #7625

Open
opened 2026-11-03 13:39:11 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/7074
Author: @abdul-rahman-1
Created: 10/25/2026
Status: 🔄 Open

Base: main ← Head: fix/adb-verifier-failed-push-cleanup


📝 Commits (1)

  • acdbe88 fix(verify): clean up failed ADB probes

📊 Changes

2 files changed (+168 additions, -46 deletions)

View changed files

📝 scripts/verify-adb.mjs (+82 -40)
📝 tests/boundary/process/adbVerifierCleanup.test.ts (+86 -6)

📄 Description

Summary

Follow-up to #5256, which fixed the successful-push cleanup path for #5190. This PR addresses remaining cases where the verifier can leave its own ADB transfer probe on the device.

Problem and expected behavior

On current main, a push can complete and then fail its device-digest check. push_adb_file returns an AnalysisArtifactChangedError with a partial observation, but the verifier turns the failed result into null and returns before entering the cleanup finally. The device probe is left behind. The timestamp-based name can also collide for concurrent verifier runs, and a transient removal failure has no retained cleanup attempt during finalization.

Expected behavior: clean up only the exact probe the verifier owns, including when the provider reports a post-write failure; do not remove a path after a push refused before writing; retry a failed removal once at finalization and fail visibly if the probe still remains.

Change and scope

  • Treat a successful push or a matching push_adb_file partial observation (operation, serial, and exact device path) as evidence that this run may clean up the path. Pre-write refusals remain untouched.
  • Use a per-run UUID for the no-overwrite probe path. The concrete race is concurrent runs in the same millisecond receiving the same Date.now() name; the change costs one built-in crypto.randomUUID() import and path substitution, as requested for the review follow-up.
  • If immediate removal or its directory-listing confirmation fails, retain that exact cleanup task and retry it during verifier finalization, before closing the provider. A final failure remains visible and fails verification.
  • Preserve main's existing provider-reported ADB binary, exact rm argv, and directory-listing confirmation. No provider capability or CLI/MCP contract is added.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

  • Semantic owner and earliest changed stage: scripts/verify-adb.mjs owns the verifier probe and uses the provider's exact partial-observation identity.
  • Contribution scope fit and linked maintainer decision: focused verifier fix within the existing ADB workflow; no new provider/capability, maintainer decision not applicable.
  • CLI and MCP/tool-catalog contract: none; only npm run verify:adb output/check behavior changes.
  • Provider, bridge, target-format, or platform compatibility: provider contract unchanged; uses the existing caller-selected ADB binary and path.
  • Evidence, artifact, provenance, or reconstruction contract: no contract change; cleanup requires successful push or matching partial-observation evidence.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): selected serial is unchanged; deletion is limited to the exact UUID path and remains verified by directory listing. No cleanup is attempted for an explicit pre-write refusal.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): 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. (Remove only the verifier-created device probe.)
  • 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: simulated boundary cases for post-write digest error, thrown failure with matching partial observation, pre-write refusal, UUID uniqueness, and cleanup retry during finalization.
  • Base reproduction or other evidence: source-derived from current main: verifyTransferRoundtrip returns when run("push_adb_file", ...) yields null, before its cleanup finally; the provider's digest-mismatch error carries a partial observation. No baseline runtime reproduction was performed.
  • User-visible CLI/MCP output (if applicable): the verifier reports a deferred cleanup retry and a final [ok]/[FAIL] verifier transfer probe removed result.
  • Remaining proof gaps: the POSIX-shell cases (including the new cleanup regressions) are skipped on this Windows host; no real ADB device or emulator was available. CI results are pending.

For boundary fixes, identify the applicable boundary regressions
and lifecycle regressions
covered by the evidence above, and explain relevant gaps.

The tests cover the process-boundary verifier lifecycle and deferred final cleanup with simulated ADB. They do not establish real-device behavior or run the POSIX shell fixture on this Windows host.

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

  • corepack.cmd npm run test:focused -- tests/boundary/process/adbVerifierCleanup.test.ts — passed: 4 tests passed; 11 POSIX-shell cases skipped on Windows.
  • corepack.cmd npm run check:fast — passed.
  • corepack.cmd npm exec -- oxfmt --check scripts/verify-adb.mjs tests/boundary/process/adbVerifierCleanup.test.ts — passed.
  • git diff --check — passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: no real ADB device or emulator was used; POSIX-shell verifier tests were not run locally.
  • Package or release metadata impact: none; verifier-only change.
  • Security, privacy, process, or containment review: no new capability. Cleanup uses direct argv with the existing selected ADB binary and targets only the verifier's exact no-overwrite path; unresolved cleanup remains a failed check.

Author readiness

Human owner: @abdul-rahman-1

  • 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.

🔄 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/7074 **Author:** [@abdul-rahman-1](https://github.com/abdul-rahman-1) **Created:** 10/25/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/adb-verifier-failed-push-cleanup` --- ### 📝 Commits (1) - [`acdbe88`](https://github.com/morluto/rea/commit/acdbe8898e46f6648345fcf5614de07c8256e73f) fix(verify): clean up failed ADB probes ### 📊 Changes **2 files changed** (+168 additions, -46 deletions) <details> <summary>View changed files</summary> 📝 `scripts/verify-adb.mjs` (+82 -40) 📝 `tests/boundary/process/adbVerifierCleanup.test.ts` (+86 -6) </details> ### 📄 Description <!-- PR title: type(optional-scope): imperative outcome The title becomes the squash-merge commit subject and generated changelog entry. --> ## Summary <!-- What user, agent, or maintainer problem does this solve? Link the issue with "Fixes #123" when applicable. Keep the prior behavior, expected contract, and new behavior understandable without private context. --> Follow-up to #5256, which fixed the successful-push cleanup path for #5190. This PR addresses remaining cases where the verifier can leave its own ADB transfer probe on the device. ## Problem and expected behavior <!-- For fixes, give the smallest trigger and violated invariant. For features, describe the use case and observable outcome. --> On current `main`, a push can complete and then fail its device-digest check. `push_adb_file` returns an `AnalysisArtifactChangedError` with a partial observation, but the verifier turns the failed result into `null` and returns before entering the cleanup `finally`. The device probe is left behind. The timestamp-based name can also collide for concurrent verifier runs, and a transient removal failure has no retained cleanup attempt during finalization. Expected behavior: clean up only the exact probe the verifier owns, including when the provider reports a post-write failure; do not remove a path after a push refused before writing; retry a failed removal once at finalization and fail visibly if the probe still remains. ## Change and scope <!-- Explain the chosen approach, why it fits REA's architecture, and what is intentionally not included. If adding complexity for a speculative or vanishingly unlikely race or security edge case, record the concrete failure mode, likelihood, complexity cost, and the user's buy-in obtained before implementation. Do not silently expand scope for theoretical robustness. --> - Treat a successful push or a matching `push_adb_file` partial observation (operation, serial, and exact device path) as evidence that this run may clean up the path. Pre-write refusals remain untouched. - Use a per-run UUID for the no-overwrite probe path. The concrete race is concurrent runs in the same millisecond receiving the same `Date.now()` name; the change costs one built-in `crypto.randomUUID()` import and path substitution, as requested for the review follow-up. - If immediate removal or its directory-listing confirmation fails, retain that exact cleanup task and retry it during verifier finalization, before closing the provider. A final failure remains visible and fails verification. - Preserve `main`'s existing provider-reported ADB binary, exact `rm` argv, and directory-listing confirmation. No provider capability or CLI/MCP contract is added. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact <!-- Short answers are welcome. Complete applicable lines; mark unresolved facts as "unknown". Use "none" or "not applicable" only when established. --> - Semantic owner and earliest changed stage: `scripts/verify-adb.mjs` owns the verifier probe and uses the provider's exact partial-observation identity. <!-- Name the producer of parsed facts or resources and its affected consumers. Summarize superseded paths removed and any distinct policy, lifecycle, lazy-loading, or compatibility responsibility retained. --> - Contribution scope fit and linked maintainer decision: focused verifier fix within the existing ADB workflow; no new provider/capability, maintainer decision not applicable. - CLI and MCP/tool-catalog contract: none; only `npm run verify:adb` output/check behavior changes. - Provider, bridge, target-format, or platform compatibility: provider contract unchanged; uses the existing caller-selected ADB binary and path. - Evidence, artifact, provenance, or reconstruction contract: no contract change; cleanup requires successful push or matching partial-observation evidence. - Target selection, authority, upstream behavior, and cleanup details (if applicable): selected serial is unchanged; deletion is limited to the exact UUID path and remains verified by directory listing. No cleanup is attempted for an explicit pre-write refusal. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): 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. - [x] Change process memory, registers, files, or configuration. (Remove only the verifier-created device probe.) - [ ] 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 <!-- State whether evidence is executed, source-derived, or proposed. For fixes, show the original failure and explain how the regression would fail on the affected base revision when practical. Include the most useful proof inline; link long logs or artifacts, or put them in optional details. Documentation-only changes can use source, link, or rendered-document checks. --> - Tests added or updated: simulated boundary cases for post-write digest error, thrown failure with matching partial observation, pre-write refusal, UUID uniqueness, and cleanup retry during finalization. - Base reproduction or other evidence: source-derived from current `main`: `verifyTransferRoundtrip` returns when `run("push_adb_file", ...)` yields `null`, before its cleanup `finally`; the provider's digest-mismatch error carries a partial observation. No baseline runtime reproduction was performed. - User-visible CLI/MCP output (if applicable): the verifier reports a deferred cleanup retry and a final `[ok]`/`[FAIL] verifier transfer probe removed` result. - Remaining proof gaps: the POSIX-shell cases (including the new cleanup regressions) are skipped on this Windows host; no real ADB device or emulator was available. CI results are pending. For boundary fixes, identify the applicable [boundary regressions](https://github.com/morluto/rea/blob/main/docs/testing.md#boundary-regressions) and [lifecycle regressions](https://github.com/morluto/rea/blob/main/docs/testing.md#lifecycle-regressions) covered by the evidence above, and explain relevant gaps. The tests cover the process-boundary verifier lifecycle and deferred final cleanup with simulated ADB. They do not establish real-device behavior or run the POSIX shell fixture on this Windows host. 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 <!-- List only commands that actually ran, with observed results. Include the relevant real-provider or platform workflow, or explain why it was unavailable. Keep these results current after changes; a command without its result is incomplete. --> - `corepack.cmd npm run test:focused -- tests/boundary/process/adbVerifierCleanup.test.ts` — passed: 4 tests passed; 11 POSIX-shell cases skipped on Windows. - `corepack.cmd npm run check:fast` — passed. - `corepack.cmd npm exec -- oxfmt --check scripts/verify-adb.mjs tests/boundary/process/adbVerifierCleanup.test.ts` — passed. - `git diff --check` — passed. ## Compatibility, safety, and release <!-- Call out breaking changes, migrations, package effects, platform/provider limits, security/privacy/containment implications, and meaningful proof gaps. --> - Breaking changes or migration steps: none. <!-- For ! / BREAKING CHANGE: show a previously valid call/configuration, its new behavior and migration, and why compatibility/deprecation cannot preserve it. Every release increments minor, including breaking changes; keep breaking markers and migration notes so callers can assess compatibility. See docs/releasing.md. --> - Real Hopper/Ghidra, browser, or OS coverage: no real ADB device or emulator was used; POSIX-shell verifier tests were not run locally. - Package or release metadata impact: none; verifier-only change. - Security, privacy, process, or containment review: no new capability. Cleanup uses direct argv with the existing selected ADB binary and targets only the verifier's exact no-overwrite path; unresolved cleanup remains a failed check. ## Author readiness <!-- The human owner completes these declarations before requesting review. The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. AI agents must leave these declarations to a human to dictate to them or there could be catastrophic consequences for many users of REA. Draft PRs may leave them unchecked. These declarations record initial readiness when opening the PR for review; later pushes do not require a new human sign-off. Keep the description and validation evidence current as work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: @abdul-rahman-1 - [ ] 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 <!-- Draft PRs may leave pending items unchecked. Resolve material scope and authority questions before integration. --> - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
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#7625
No description provided.