morluto/rea

Fork 0

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

[PR #2221] [MERGED] fix(adb): preserve existing bugreport archives #5091

New issue

Closed

opened 2026-10-31 01:03:51 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:03:51 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/adb-bugreport-preserve

📝 Commits (2)

8f2e633 fix(adb): preserve existing bugreport archives
b7fe8b9 fix(adb): retain bugreports after publication failure

📊 Changes

5 files changed (+507 additions, -98 deletions)

View changed files

📝 docs/adb-device-analysis.md (+5 -0)
📝 src/adb/AdbProvider.test.ts (+4 -4)
📝 src/adb/AdbProvider.ts (+220 -84)
📝 src/domain/adb/adbDeviceAnalysis.ts (+12 -10)
➕ tests/boundary/providers/adb/adbBugreportPublication.test.ts (+266 -0)

📄 Description

Summary

Fixes #2220.

Collecting a bugreport into a directory that already contains a same-name ZIP could silently replace the earlier archive. This change isolates ADB's collection output and publishes the resulting ZIP exclusively, selecting a numeric suffix when a destination is occupied. Existing files retain their bytes, and a successful request still returns one newly created archive in the caller's chosen directory with its byte count and SHA-256 digest.

For example, if bugreport test.zip and bugreport test (1).zip already exist, the new collection is published as bugreport test (2).zip. The response names that final file in local_path and preserves ADB's original completion path in adb_reported_path.

Problem and expected behavior

The smallest trigger is a filename collision: the caller chooses an output directory, and adb bugreport selects a ZIP name that is already present there. A collision can also appear while collection is running. Repeated collection does not guarantee a collision on every device, but the provider must preserve existing files whenever one occurs.

The affected implementation passed the caller's directory directly to ADB and only inspected the reported file after ADB finished writing it. At that point, hashing the new archive could establish its current bytes but could not prevent or reveal the earlier file's destruction. The output_directory contract promises one new ZIP, and the request offers no permission to overwrite existing host evidence.

The expected behavior is to preserve every occupied destination while acquiring a new archive. A preflight existence check would leave a race during a potentially long collection, so the preservation guarantee must be enforced by the final filesystem publication operation.

Change and scope

The correction lives in AdbProvider.collectBugreport, shared by the CLI and MCP workflows:

Create a unique staging directory under the caller-selected directory with mkdtemp and record its filesystem identity before starting ADB.
Pass that staging directory to adb bugreport. Parse the completion path and require it to identify a regular, non-symlink file directly in the owned staging directory. A report pointing elsewhere is a protocol refusal, rather than permission to acquire an arbitrary existing file.
Publish the archive with copyFile(..., COPYFILE_EXCL). Try the reported basename first; on EEXIST, select (1), (2), and subsequent suffixes before the original extension. Other publication failures remain output errors.
Hash the published file and return its final path, byte count, and digest together with the exact ADB-reported path.
Remove the owned staging directory. Failed removal remains visible and stays owned for a later retry through close() or the next admitted operation.

The existing package-directory cleanup owner has been generalized to cover both package pulls and bugreport staging, retaining the originating operation for each resource. Aggregate cleanup failures use the shared close operation and record the owning operations. This avoids introducing a second cleanup mechanism for the same filesystem responsibility.

Staging allocation failures return the tagged Result error shape. Cleanup failures preserve the primary error rather than replacing it. If the archive has already been published, a digest cancellation retains its final path and producer path with an explicitly unknown digest. If hashing completed before cleanup failed, the error retains the completed acquisition, including its digest. The internal acquired-file partial-observation type accepts those completed bugreport facts.

Contract and boundary impact

Semantic owner and earliest changed stage: the ADB provider's host artifact acquisition boundary, before the external command receives an output path. CLI and MCP adapters continue to delegate to the same owner.
CLI and MCP/tool-catalog contract: tool name, CLI command, input fields, and advertised successful output schema are unchanged. Callers should use the returned local_path, which can now include a collision suffix. No overwrite flag is introduced.
Provider, bridge, target-format, or platform compatibility: ADB still receives an output directory for bugreport; the directory is now an isolated staging destination. No Android configuration or archive format change is introduced. Host filename selection retains the original extension.
Evidence, artifact, provenance, or reconstruction contract: a successful operation still leaves one new ZIP directly in the selected directory. The digest describes the final published artifact, and adb_reported_path retains the producer's exact reported value even though staging is subsequently removed. The archive's device-generated contents and existing limitations are unchanged. Internal partial observations now retain completed bugreport facts when later cleanup fails.
Process execution, authorization, cleanup, or containment impact: the existing supervised ADB invocation and its timeout/output budget remain in use. ADB writes into a unique owned directory; final publication uses exclusive creation. Cleanup verifies directory identity and retains ownership on failure. No additional device mutation authority is requested.
Generated metadata, package, or installation impact: none. No advertised schema, package dependency, installation behavior, authored skill, or tracked generated catalog changes. Build and skill outputs remain ignored.

Evidence and regression coverage

The new filesystem boundary regression executes on the Windows filesystem with simulated ADB output. The process replies model the archive write and ADB completion line; real filesystem operations perform publication and preservation checks. Targeted failure injection exercises staging allocation, digest cancellation, and cleanup failure.

The two collision scenarios verify that both the original basename and an already occupied first suffix keep their original bytes, while the new ZIP is published at the next available name with the expected bytes and digest. One creates the conflict before collection; the other creates it during collection. The affected implementation passed the caller directory to the fixture, so these assertions would expose its overwriting or incorrect artifact/digest selection. The affected base was reviewed in source; these new tests were executed on the fixed branch, not represented as a separate real-device or base-revision run.

The other regressions confirm that:

a cancellation after publication retains the final local path and original producer path, leaves the acquired ZIP available, and removes staging;
a staging allocation failure returns an output error without invoking device collection;
a cleanup failure after successful publication retains the completed acquisition and its digest, reports the owned resource, and succeeds on a subsequent close() retry;
a reported archive outside the staging directory is refused without changing an existing caller file;
a malformed completion combined with failed cleanup preserves the original protocol failure in the error chain, while making cleanup uncertainty visible and retryable.

The provider regression fixture now writes into the directory supplied to ADB and asserts the final artifact separately from the producer path.

Representative successful fields, when the original name and first suffix are occupied:

{
  "local_path": "C:\\reports\\bugreport test (2).zip",
  "bytes": 15,
  "sha256": "<SHA-256 of the newly published archive>",
  "adb_reported_path": "C:\\reports\\.rea-bugreport-<unique>\\bugreport test.zip"
}

This is a field-level illustration of the existing successful result shape; the CLI/MCP Evidence wrapper, client identity, and device serial remain present in the complete response. A removed staging path remains source provenance rather than the location callers should open.

Observed filesystem results, source-derived behavior, and real-device proof gaps remain distinguishable.
Final artifact identity, original ADB provenance, and meaningful failure information remain preserved.
Unsupported completion paths, cancelled digest verification, and incomplete cleanup remain visible.

Validation performed

The final branch is based on e4d3e1399d9cd4932653e7015a1bf8589e56029d. Local commands used the pinned Node 24.18.0 and npm 11.16.0 toolchain on Windows.

npm run test:focused -- tests/boundary/providers/adb/adbBugreportPublication.test.ts src/adb/AdbProvider.test.ts — passed: 7 tests. This command completed against the rebased final implementation.
npm run check — passed on the rebased final implementation: all 6 Turbo tasks completed successfully, including typecheck, lint, formatting, unused-code analysis, and metadata checks. Resolved module boundaries passed across 1,598 source files; architecture guards passed across 1,210 source files.
`npm run check:fast` — passed: typecheck and lint; the final full check also passed.
npm run verify:test-discovery — passed: all 931 test files were discovered exactly once.
git diff --check — passed.

Compatibility, safety, and release

Breaking changes or migration steps: none. Existing requests remain valid. A collision changes the new artifact's host filename, and callers can continue using the already advertised local_path field to locate it.
Real Hopper/Ghidra, browser, or OS coverage: Windows filesystem behavior was exercised with simulated ADB output. Real-device verification is outside local coverage. Hopper, Ghidra, and browser workflows are not affected by this change.
Package or release metadata impact: none; this is a focused bug fix with one Conventional Commit. No binaries, dist/, generated skill bundle, provider projects, or local diagnostic artifacts are committed.
Security, privacy, process, or containment review: the fix reduces authority over caller-owned files by isolating ADB's destination and enforcing exclusive final publication. Reported paths are checked against the owned staging directory, and recursive cleanup requires the recorded directory identity. Existing device configuration and command supervision semantics are preserved.

Review checklist

This PR addresses a concrete data-preservation bug.
The PR has one focused outcome and a Conventional Commit title.
The related issue is linked with Fixes above.
Tests cover changed observable behavior and meaningful failure paths.
The internal partial-observation contract is updated; advertised schemas and generated metadata do not require changes.
Representative user-visible result fields are included above.
The final diff was checked for secrets, unrelated cleanup, and unsupported verification 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/2221 **Author:** [@dakjdakd](https://github.com/dakjdakd) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/12/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/adb-bugreport-preserve` --- ### 📝 Commits (2) - [`8f2e633`](https://github.com/morluto/rea/commit/8f2e633dff2ee8eec672d4d7dc1491c2b8e6bc5e) fix(adb): preserve existing bugreport archives - [`b7fe8b9`](https://github.com/morluto/rea/commit/b7fe8b938dce0230343b9ddb81ed136ce06ce2dd) fix(adb): retain bugreports after publication failure ### 📊 Changes **5 files changed** (+507 additions, -98 deletions) <details> <summary>View changed files</summary> 📝 `docs/adb-device-analysis.md` (+5 -0) 📝 `src/adb/AdbProvider.test.ts` (+4 -4) 📝 `src/adb/AdbProvider.ts` (+220 -84) 📝 `src/domain/adb/adbDeviceAnalysis.ts` (+12 -10) ➕ `tests/boundary/providers/adb/adbBugreportPublication.test.ts` (+266 -0) </details> ### 📄 Description ## Summary Fixes #2220. Collecting a bugreport into a directory that already contains a same-name ZIP could silently replace the earlier archive. This change isolates ADB's collection output and publishes the resulting ZIP exclusively, selecting a numeric suffix when a destination is occupied. Existing files retain their bytes, and a successful request still returns one newly created archive in the caller's chosen directory with its byte count and SHA-256 digest. For example, if `bugreport test.zip` and `bugreport test (1).zip` already exist, the new collection is published as `bugreport test (2).zip`. The response names that final file in `local_path` and preserves ADB's original completion path in `adb_reported_path`. ## Problem and expected behavior The smallest trigger is a filename collision: the caller chooses an output directory, and `adb bugreport` selects a ZIP name that is already present there. A collision can also appear while collection is running. Repeated collection does not guarantee a collision on every device, but the provider must preserve existing files whenever one occurs. The [affected implementation](https://github.com/morluto/rea/blob/58fb37207/src/adb/AdbProvider.ts#L2074-L2151) passed the caller's directory directly to ADB and only inspected the reported file after ADB finished writing it. At that point, hashing the new archive could establish its current bytes but could not prevent or reveal the earlier file's destruction. The [`output_directory` contract](https://github.com/morluto/rea/blob/58fb37207/src/domain/adb/adbDeviceAnalysis.ts#L187-L194) promises one new ZIP, and the request offers no permission to overwrite existing host evidence. The expected behavior is to preserve every occupied destination while acquiring a new archive. A preflight existence check would leave a race during a potentially long collection, so the preservation guarantee must be enforced by the final filesystem publication operation. ## Change and scope The correction lives in [`AdbProvider.collectBugreport`](https://github.com/dakjdakd/rea/blob/8f2e633dff2ee8eec672d4d7dc1491c2b8e6bc5e/src/adb/AdbProvider.ts#L2109), shared by the CLI and MCP workflows: 1. Create a unique staging directory under the caller-selected directory with `mkdtemp` and record its filesystem identity before starting ADB. 2. Pass that staging directory to `adb bugreport`. Parse the completion path and require it to identify a regular, non-symlink file directly in the owned staging directory. A report pointing elsewhere is a protocol refusal, rather than permission to acquire an arbitrary existing file. 3. Publish the archive with [`copyFile(..., COPYFILE_EXCL)`](https://github.com/dakjdakd/rea/blob/8f2e633dff2ee8eec672d4d7dc1491c2b8e6bc5e/src/adb/AdbProvider.ts#L314). Try the reported basename first; on `EEXIST`, select ` (1)`, ` (2)`, and subsequent suffixes before the original extension. Other publication failures remain output errors. 4. Hash the published file and return its final path, byte count, and digest together with the exact ADB-reported path. 5. Remove the owned staging directory. Failed removal remains visible and stays owned for a later retry through `close()` or the next admitted operation. The existing package-directory cleanup owner has been generalized to cover both package pulls and bugreport staging, retaining the originating operation for each resource. Aggregate cleanup failures use the shared close operation and record the owning operations. This avoids introducing a second cleanup mechanism for the same filesystem responsibility. Staging allocation failures return the tagged `Result` error shape. Cleanup failures preserve the primary error rather than replacing it. If the archive has already been published, a digest cancellation retains its final path and producer path with an explicitly unknown digest. If hashing completed before cleanup failed, the error retains the completed acquisition, including its digest. The internal acquired-file partial-observation type accepts those completed bugreport facts. ## Contract and boundary impact - **Semantic owner and earliest changed stage:** the ADB provider's host artifact acquisition boundary, before the external command receives an output path. CLI and MCP adapters continue to delegate to the same owner. - **CLI and MCP/tool-catalog contract:** tool name, CLI command, input fields, and advertised successful output schema are unchanged. Callers should use the returned `local_path`, which can now include a collision suffix. No overwrite flag is introduced. - **Provider, bridge, target-format, or platform compatibility:** ADB still receives an output directory for `bugreport`; the directory is now an isolated staging destination. No Android configuration or archive format change is introduced. Host filename selection retains the original extension. - **Evidence, artifact, provenance, or reconstruction contract:** a successful operation still leaves one new ZIP directly in the selected directory. The digest describes the final published artifact, and `adb_reported_path` retains the producer's exact reported value even though staging is subsequently removed. The archive's device-generated contents and existing limitations are unchanged. Internal partial observations now retain completed bugreport facts when later cleanup fails. - **Process execution, authorization, cleanup, or containment impact:** the existing supervised ADB invocation and its timeout/output budget remain in use. ADB writes into a unique owned directory; final publication uses exclusive creation. Cleanup verifies directory identity and retains ownership on failure. No additional device mutation authority is requested. - **Generated metadata, package, or installation impact:** none. No advertised schema, package dependency, installation behavior, authored skill, or tracked generated catalog changes. Build and skill outputs remain ignored. ## Evidence and regression coverage The [new filesystem boundary regression](https://github.com/dakjdakd/rea/blob/8f2e633dff2ee8eec672d4d7dc1491c2b8e6bc5e/tests/boundary/providers/adb/adbBugreportPublication.test.ts) executes on the Windows filesystem with simulated ADB output. The process replies model the archive write and ADB completion line; real filesystem operations perform publication and preservation checks. Targeted failure injection exercises staging allocation, digest cancellation, and cleanup failure. The two collision scenarios verify that both the original basename and an already occupied first suffix keep their original bytes, while the new ZIP is published at the next available name with the expected bytes and digest. One creates the conflict before collection; the other creates it during collection. The affected implementation passed the caller directory to the fixture, so these assertions would expose its overwriting or incorrect artifact/digest selection. The affected base was reviewed in source; these new tests were executed on the fixed branch, not represented as a separate real-device or base-revision run. The other regressions confirm that: - a cancellation after publication retains the final local path and original producer path, leaves the acquired ZIP available, and removes staging; - a staging allocation failure returns an output error without invoking device collection; - a cleanup failure after successful publication retains the completed acquisition and its digest, reports the owned resource, and succeeds on a subsequent `close()` retry; - a reported archive outside the staging directory is refused without changing an existing caller file; - a malformed completion combined with failed cleanup preserves the original protocol failure in the error chain, while making cleanup uncertainty visible and retryable. The provider regression fixture now writes into the directory supplied to ADB and asserts the final artifact separately from the producer path. Representative successful fields, when the original name and first suffix are occupied: ```json { "local_path": "C:\\reports\\bugreport test (2).zip", "bytes": 15, "sha256": "<SHA-256 of the newly published archive>", "adb_reported_path": "C:\\reports\\.rea-bugreport-<unique>\\bugreport test.zip" } ``` This is a field-level illustration of the existing successful result shape; the CLI/MCP Evidence wrapper, client identity, and device serial remain present in the complete response. A removed staging path remains source provenance rather than the location callers should open. - [x] Observed filesystem results, source-derived behavior, and real-device proof gaps remain distinguishable. - [x] Final artifact identity, original ADB provenance, and meaningful failure information remain preserved. - [x] Unsupported completion paths, cancelled digest verification, and incomplete cleanup remain visible. ## Validation performed The final branch is based on `e4d3e1399d9cd4932653e7015a1bf8589e56029d`. Local commands used the pinned Node 24.18.0 and npm 11.16.0 toolchain on Windows. - `npm run test:focused -- tests/boundary/providers/adb/adbBugreportPublication.test.ts src/adb/AdbProvider.test.ts` — **passed: 7 tests**. This command completed against the rebased final implementation. - `npm run check` — passed on the rebased final implementation: all 6 Turbo tasks completed successfully, including typecheck, lint, formatting, unused-code analysis, and metadata checks. Resolved module boundaries passed across 1,598 source files; architecture guards passed across 1,210 source files. - \`npm run check:fast\` — passed: typecheck and lint; the final full check also passed. - `npm run verify:test-discovery` — passed: all 931 test files were discovered exactly once. - `git diff --check` — passed. ## Compatibility, safety, and release - **Breaking changes or migration steps:** none. Existing requests remain valid. A collision changes the new artifact's host filename, and callers can continue using the already advertised `local_path` field to locate it. - **Real Hopper/Ghidra, browser, or OS coverage:** Windows filesystem behavior was exercised with simulated ADB output. Real-device verification is outside local coverage. Hopper, Ghidra, and browser workflows are not affected by this change. - **Package or release metadata impact:** none; this is a focused bug fix with one Conventional Commit. No binaries, `dist/`, generated skill bundle, provider projects, or local diagnostic artifacts are committed. - **Security, privacy, process, or containment review:** the fix reduces authority over caller-owned files by isolating ADB's destination and enforcing exclusive final publication. Reported paths are checked against the owned staging directory, and recursive cleanup requires the recorded directory identity. Existing device configuration and command supervision semantics are preserved. ## Review checklist - [x] This PR addresses a concrete data-preservation bug. - [x] The PR has one focused outcome and a Conventional Commit title. - [x] The related issue is linked with `Fixes` above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] The internal partial-observation contract is updated; advertised schemas and generated metadata do not require changes. - [x] Representative user-visible result fields are included above. - [x] The final diff was checked for secrets, unrelated cleanup, and unsupported verification claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:03:51 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #5246] [MERGED] fix(rizin): bound recent history frames by UTF-8 byte budget #6834

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

No description provided.

Rows
Columns