[PR #1903] fix(adb): project actionable pm path refusal details #4895

New issue

Open

opened 2026-10-31 00:56:31 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:56:31 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1903
Author: @larinyo
Created: 10/11/2026
Status: 🔄 Open

Base: main ← Head: pr/adb-pull-filename-validation

📝 Commits (10+)

8cb18a3 fix(adb): reject unsafe pm path file names before pulling packages
d78a26a fix(adb): reject unsafe pm path entries before empty-output checks
aa4a00f fix(adb): reject case-folded and reserved host file names
7ac0b0b fix(adb): project unsafe device-path refusals and reject format controls
fc9290c fix(adb): reject default-ignorable characters and drop the unreachable normalization alias
6ffdd9e fix(adb): run pm path without a PTY and stop normalizing carriage returns
4c3a5cf Merge upstream main (51d6c87c) into pr/adb-pull-filename-validation
a53a80e fix(adb): preserve unsafe path and redaction diagnostics
fd4ab6b fix(adb): reconcile path refusal contracts with main
bd9dfe2 fix(adb): reconcile path refusal contracts with main

📊 Changes

12 files changed (+686 additions, -63 deletions)

View changed files

📝 docs/adb-device-analysis.md (+20 -0)
📝 src/adb/AdbDeviceOutput.test.ts (+31 -13)
📝 src/adb/AdbDeviceOutput.ts (+48 -6)
📝 src/adb/AdbHostFileName.ts (+3 -0)
📝 src/adb/AdbProvider.test.ts (+270 -4)
📝 src/adb/AdbProvider.ts (+153 -33)
📝 src/domain/analysisErrorCore.ts (+12 -1)
📝 src/domain/analysisErrorPresentation.ts (+6 -0)
📝 src/domain/analysisErrorProjection.test.ts (+40 -0)
📝 src/domain/analysisErrorProjection.ts (+7 -1)
📝 src/domain/explicitSensitiveFailure.ts (+5 -5)
📝 tests/boundary/mcp/adbMcp.test.ts (+91 -0)

📄 Description

Summary

pull_adb_package already refuses unsafe or colliding pm path entries on main (55b61485e, 1d4c4eb03), but every refusal reaches CLI and MCP callers as the generic "unreadable result / run rea doctor" message, because AnalysisOutputError messages are replaced during projection. A caller cannot see which device path was refused or why, even though the provider knows.

This PR makes those three expected refusals self-describing. It no longer changes which paths are accepted: name validation, collision detection and framing are left exactly as main implements them.

Behaviour change

For the three existing pm path refusals (unrepresentable host file name, duplicate local destination, destination reservation failure), the projected error now carries:

a stable details.reason: unsafe_device_path, duplicate_local_destination or local_destination_unavailable;
details.diagnostics with the raw refused device_path values, the derived file_name, a per-entry reason and, for reservation failures, the filesystem error code;
a specific user message naming the package and path, and remediation text pointing at details.diagnostics and details.captured_output instead of "retry / run rea doctor".

code stays unreadable_output, retryable stays false, and captured_output is retained. Device paths in the human-readable message are escaped (\u{202E} and similar) so bidi or invisible code points cannot reorder the rendered text; details keeps the raw values.

Change and scope

src/domain/analysisErrorCore.ts, analysisErrorProjection.ts, analysisErrorPresentation.ts: AnalysisOutputError accepts optional userMessage, remediationAction and diagnostics, following the existing pattern of ProviderAdapterError and AnalysisUnsupportedTargetError. The error schema is unchanged because details is an open record.
src/adb/AdbProvider.ts: the three refusals use those fields.
src/adb/AdbDeviceOutput.ts: displayDevicePath escaping helper.
docs/adb-device-analysis.md: documents the projected fields.

History note: earlier commits on this branch added their own name validation, case-fold and reserved-name checks, control-character rejection and adb shell -T framing. main has since solved the same problem with host-specific rules and destination reservations, so the merge of main drops all of that in favour of upstream's design. Thanks to @dandgabr and the Codex reviews for the findings along the way.

Validation

New domain projection tests for the AnalysisOutputError fields.
Provider tests assert the projected reason, diagnostics and messages for each refusal, with no pull and no host directory created.
tests/boundary/mcp/adbMcp.test.ts asserts the details reach MCP callers.
npx vitest run src/adb src/domain tests/boundary/mcp/adbMcp.test.ts tests/boundary/cli/adbCli.test.ts: 2213 tests pass. tsc, oxlint, module boundaries, format and docs:check are clean.

Limits

No real Android device was used.

_{🔄 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/1903 **Author:** [@larinyo](https://github.com/larinyo) **Created:** 10/11/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `pr/adb-pull-filename-validation` --- ### 📝 Commits (10+) - [`8cb18a3`](https://github.com/morluto/rea/commit/8cb18a33e519d9f0df894fd9a3c3aec13f3e4147) fix(adb): reject unsafe pm path file names before pulling packages - [`d78a26a`](https://github.com/morluto/rea/commit/d78a26ab7b390519722d2583191fc29c2e2194ed) fix(adb): reject unsafe pm path entries before empty-output checks - [`aa4a00f`](https://github.com/morluto/rea/commit/aa4a00ffcda34943c35892360f15d46805a011bc) fix(adb): reject case-folded and reserved host file names - [`7ac0b0b`](https://github.com/morluto/rea/commit/7ac0b0b6431e0b28784f0f49ace8334cdccf7c2f) fix(adb): project unsafe device-path refusals and reject format controls - [`fc9290c`](https://github.com/morluto/rea/commit/fc9290c4bb3a3068eab1ac6ffc254aecddb02248) fix(adb): reject default-ignorable characters and drop the unreachable normalization alias - [`6ffdd9e`](https://github.com/morluto/rea/commit/6ffdd9e49a11c12f2e669224daf78ab78efdbb9b) fix(adb): run pm path without a PTY and stop normalizing carriage returns - [`4c3a5cf`](https://github.com/morluto/rea/commit/4c3a5cf18f3893782da6f834377eca3b098b2818) Merge upstream main (51d6c87c) into pr/adb-pull-filename-validation - [`a53a80e`](https://github.com/morluto/rea/commit/a53a80e9a918caf2a00f63db0bf9838c34262b84) fix(adb): preserve unsafe path and redaction diagnostics - [`fd4ab6b`](https://github.com/morluto/rea/commit/fd4ab6b16249ed1228a1892dfa8dd1c580b2357a) fix(adb): reconcile path refusal contracts with main - [`bd9dfe2`](https://github.com/morluto/rea/commit/bd9dfe2383543d02f301f061ae79e7b0c61d970b) fix(adb): reconcile path refusal contracts with main ### 📊 Changes **12 files changed** (+686 additions, -63 deletions) <details> <summary>View changed files</summary> 📝 `docs/adb-device-analysis.md` (+20 -0) 📝 `src/adb/AdbDeviceOutput.test.ts` (+31 -13) 📝 `src/adb/AdbDeviceOutput.ts` (+48 -6) 📝 `src/adb/AdbHostFileName.ts` (+3 -0) 📝 `src/adb/AdbProvider.test.ts` (+270 -4) 📝 `src/adb/AdbProvider.ts` (+153 -33) 📝 `src/domain/analysisErrorCore.ts` (+12 -1) 📝 `src/domain/analysisErrorPresentation.ts` (+6 -0) 📝 `src/domain/analysisErrorProjection.test.ts` (+40 -0) 📝 `src/domain/analysisErrorProjection.ts` (+7 -1) 📝 `src/domain/explicitSensitiveFailure.ts` (+5 -5) 📝 `tests/boundary/mcp/adbMcp.test.ts` (+91 -0) </details> ### 📄 Description ## Summary `pull_adb_package` already refuses unsafe or colliding `pm path` entries on `main` (55b61485e, 1d4c4eb03), but every refusal reaches CLI and MCP callers as the generic "unreadable result / run `rea doctor`" message, because `AnalysisOutputError` messages are replaced during projection. A caller cannot see which device path was refused or why, even though the provider knows. This PR makes those three expected refusals self-describing. It no longer changes which paths are accepted: name validation, collision detection and framing are left exactly as `main` implements them. ## Behaviour change For the three existing `pm path` refusals (unrepresentable host file name, duplicate local destination, destination reservation failure), the projected error now carries: - a stable `details.reason`: `unsafe_device_path`, `duplicate_local_destination` or `local_destination_unavailable`; - `details.diagnostics` with the raw refused `device_path` values, the derived `file_name`, a per-entry reason and, for reservation failures, the filesystem error code; - a specific user message naming the package and path, and remediation text pointing at `details.diagnostics` and `details.captured_output` instead of "retry / run `rea doctor`". `code` stays `unreadable_output`, `retryable` stays `false`, and `captured_output` is retained. Device paths in the human-readable message are escaped (`\u{202E}` and similar) so bidi or invisible code points cannot reorder the rendered text; `details` keeps the raw values. ## Change and scope - `src/domain/analysisErrorCore.ts`, `analysisErrorProjection.ts`, `analysisErrorPresentation.ts`: `AnalysisOutputError` accepts optional `userMessage`, `remediationAction` and `diagnostics`, following the existing pattern of `ProviderAdapterError` and `AnalysisUnsupportedTargetError`. The error schema is unchanged because `details` is an open record. - `src/adb/AdbProvider.ts`: the three refusals use those fields. - `src/adb/AdbDeviceOutput.ts`: `displayDevicePath` escaping helper. - `docs/adb-device-analysis.md`: documents the projected fields. History note: earlier commits on this branch added their own name validation, case-fold and reserved-name checks, control-character rejection and `adb shell -T` framing. `main` has since solved the same problem with host-specific rules and destination reservations, so the merge of `main` drops all of that in favour of upstream's design. Thanks to @dandgabr and the Codex reviews for the findings along the way. ## Validation - New domain projection tests for the `AnalysisOutputError` fields. - Provider tests assert the projected reason, diagnostics and messages for each refusal, with no pull and no host directory created. - `tests/boundary/mcp/adbMcp.test.ts` asserts the details reach MCP callers. - `npx vitest run src/adb src/domain tests/boundary/mcp/adbMcp.test.ts tests/boundary/cli/adbCli.test.ts`: 2213 tests pass. `tsc`, `oxlint`, module boundaries, format and `docs:check` are clean. ## Limits No real Android device was used. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-31 00:56:31 +00:00

bsakaguchi referenced this issue

2026-11-01 21:42:10 +00:00

[PR #4899] [CLOSED] fix(setup): preserve unresolved launcher identity #6668