morluto/rea

Fork 0

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

[PR #5134] [CLOSED] fix(adb): admit empty string intent extras at the domain boundary #6780

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5134
Author: @chordpli
Created: 10/19/2026
Status: ❌ Closed

Base: main ← Head: fix/adb-empty-string-extra

📝 Commits (2)

2ca4fe7 fix(adb): admit empty string intent extras at the domain boundary
8eb1072 test(adb): pin MCP rejection of mistyped intent extras

📊 Changes

5 files changed (+83 additions, -58 deletions)

View changed files

📝 src/adb/AdbProvider.test.ts (+4 -16)
📝 src/adb/AdbProvider.ts (+0 -26)
📝 src/domain/adb/adbDeviceAnalysis.ts (+49 -14)
📝 tests/boundary/cli/adbCli.test.ts (+11 -1)
📝 tests/boundary/mcp/adbStartMcp.test.ts (+19 -1)

📄 Description

Summary

Fixes #5093.

start_adb_activity rejected a string extra with an empty value (--extra string:k:) as invalid_request before the adb provider ran. Android's am start --es accepts an empty string, so a caller could not send an intent that carries a present but empty extra. This change admits that value and keeps the type checks for boolean, int, long and float.

Problem and expected behavior

The shared schema put .min(1) on every extra value, whatever its type. The CLI parser already keeps an empty trailing field, so the schema was the only place that refused it.

While tracing this I found that the typed value forms (true|false, integers, decimals) were checked only inside AdbProvider.startActivity, after adb lookup. So int:k: was rejected at admission (only because of min(1)), while int:k:abc reached adb lookup first and returned capability_unavailable when adb was missing. The same kind of bad input got two different answers depending on the host.

Expected: an empty string extra reaches the provider and arrives on the device as "". A malformed typed value is rejected at admission as invalid_request, whether or not adb is installed.

Change and scope

src/domain/adb/adbDeviceAnalysis.ts: drop .min(1) from the extra value and check the typed value forms in the domain schema with superRefine (issue path ["value"]). The advertised value description now lists the accepted form for each type, because a refinement does not appear in the JSON Schema.
src/adb/AdbProvider.ts: remove the provider's duplicate value checks. Every production path reaches the provider through executeProviderOperation, which parses adbRequestSchema first (CLI via src/cli/adbCommands.ts, MCP via registerAdbTools).
The accepted forms are unchanged. The regular expressions match the removed provider checks character for character.

Not included:

executeProviderOperation builds AnalysisInputError without issues, so CLI callers get invalid_request without the field path or the per-type message. This was already true for the old provider checks and affects every single-request provider, so it belongs in a separate change. MCP callers are not affected by this gap; see the contract section below.
The extra type list ("string" | "boolean" | "int" | "long" | "float") is still written out separately in the domain, provider and CLI.
The float form is stricter than Android's Float.parseFloat (for example, 1e3 is rejected). I kept the existing behavior.

Contribution scope: an ordinary fix inside the existing adb boundary. A maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: the domain input schema for start_adb_activity, at shared request admission.
Contribution scope fit and linked maintainer decision: not applicable (fix within an existing tool).
CLI and MCP/tool-catalog contract: the advertised extras[].value loses minLength: 1 and gets a longer description. No properties, types or required fields change. The per-type forms are not expressible in the advertised JSON Schema (refinements do not convert), so they appear only in that description.
- CLI: int:k:abc, long:k:1.5, float:k:1e3 and boolean:k:yes now fail at admission as invalid_request instead of after adb lookup. The error keeps the same shape (code, category, message).
- MCP: the tool's inputSchema is this schema, so the SDK now rejects these values before the handler runs, the same path that every other schema violation (including the old min(1)) already took. The response changes from the projected invalid_request JSON to SDK text that names the field, for example Input validation error: Invalid arguments for tool start_adb_activity: extras.0.value: Boolean extra values must be true or false. A client that parsed error.code for this input now gets that text instead.
Provider, bridge, target-format, or platform compatibility: adb argv is unchanged except that an empty string is now possible, and the existing remote-shell quoting sends it as ''.
Evidence, artifact, provenance, or reconstruction contract: none. parameters.extras and normalized_result.extras keep the empty value as "".
Target selection, authority, upstream behavior, and cleanup details: unchanged. The tool still starts one activity on the selected device.
Generated metadata, package, or installation impact: none committed. npm run docs:check passes.

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

(The launch effect is the existing behavior of start_adb_activity. This change only widens the accepted input.)

Evidence and regression coverage

Tests added or updated:
- tests/boundary/cli/adbCli.test.ts: string:key: reaches provider admission. Valid typed values (int:key:-3, long:key:9007199254740993, float:key:-1.5, boolean:key:false) still do. Empty or malformed typed values (int:key:, int:key:abc, long:key:1.5, float:key:1e3, boolean:key:yes) return invalid_request with no adb binary present. On the base revision, string:key: returns invalid_request and the malformed typed values return capability_unavailable, so these cases fail there.
- tests/boundary/mcp/adbStartMcp.test.ts: the MCP call carries an empty string extra, and the simulated adb receives --es empty ''. A boolean extra of yes is rejected with the SDK input validation text above, and adb is not called.
- src/adb/AdbProvider.test.ts: the argv golden includes --es empty ''. The direct-provider test rejects mistyped intent extras before touching the device is removed, because the provider no longer owns that check. The CLI boundary cases above cover it.
Base reproduction or other evidence (executed): the issue's CLI probe on d1adc6079 printed string:k: → invalid_request. On this branch it prints capability_unavailable with a missing adb, which means the request reached provider admission.

Real-device E2E (executed), base origin/main d1adc6079 vs this branch (0182e0a67, identical patch to the rebased 2ca4fe777 per git range-diff). Both used the built runtime (node scripts/rea.mjs), adb 37.0.1 and a Pixel 8 AVD on Android 16 (API 36). The target was a throwaway test app, dev.rea.extraprobe, which logs each received extra with its Java type and length. Screenshots are in a comment below.

Case	Base	This branch
`--extra string:mode:probe --extra string:empty:`	`invalid_request`, activity not started	`reported_status: ok`; device log `empty = ""` / `String, length 0`
`--extra string:mode:probe` (control)	`ok`, `mode = "probe"`	`ok`, `mode = "probe"` (same)
`--extra int:level:`	`invalid_request`	`invalid_request` (same)
`--extra int:level:abc`, `REA_ADB_PATH=/nonexistent/adb`	`capability_unavailable`	`invalid_request`

Physical-device E2E (executed), base 95c8f57aa vs this branch (8eb1072f8) on a Samsung Galaxy Note10+ 5G (SM-N976N), Android 12 (API 31), adb 37.0.1, with the same probe app. The CLI used the built runtime. MCP used createServer from source over an in-memory transport, with the real adb and device behind it. No screenshots for this run.

Surface	Case	Base	This branch
CLI	`--extra string:mode:probe --extra string:empty:`	`invalid_request`, activity not started	`reported_status: ok`; device log `empty = ""` / `String, length 0`
CLI	`--extra string:mode:probe` (control)	`ok`, `mode = "probe"`	`ok`, `mode = "probe"` (same)
CLI	`--extra int:level:`	`invalid_request`	`invalid_request` (same)
CLI	`--extra int:level:abc`, `REA_ADB_PATH=/nonexistent/adb`	`capability_unavailable`	`invalid_request`
MCP	extras `mode="probe"`, `empty=""`	SDK text: `extras.1.value: Too small: expected string to have >=1 characters`	`reported_status: ok`; device log `empty = ""` / `String, length 0`
MCP	extra `flag` boolean `yes`	projected JSON `invalid_request`	SDK text: `extras.0.value: Boolean extra values must be true or false`

User-visible CLI/MCP output: see the table for the CLI. CLI error output keeps the same shape (code, category, message). The MCP error text for a malformed typed value is in the contract section above; I compared it against the base revision through an in-memory MCP client with a simulated adb, where the same call returned the projected invalid_request JSON.
Remaining proof gaps: MCP was not run through a stdio server process; the device run used the in-memory transport. Windows and Linux hosts were not run.

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

npm run test:local -- src/adb/AdbProvider.test.ts: 87 passed
npm run test:focused -- tests/boundary/cli/adbCli.test.ts tests/boundary/mcp/adbStartMcp.test.ts tests/boundary/mcp/adbMcp.test.ts tests/process-global/adbStartMcp.test.ts: 32 passed
npm run check:fast: typecheck and lint passed
npm run docs:check: passed
Real-device E2E above: macOS arm64, Node 24.19.0, adb 37.0.1, Pixel 8 AVD (Android 16, API 36)
Physical-device E2E above: macOS arm64, Node 24.19.0, adb 37.0.1, SM-N976N (Android 12, API 31)

The test and check commands above ran again after rebasing onto 95c8f57aa and again at 8eb1072f8. The emulator E2E ran on the pre-rebase patch noted above; the physical-device E2E ran at 8eb1072f8.

Compatibility, safety, and release

Breaking changes or migration steps: none. The only previously valid input that changes outcome is a malformed typed value with adb missing, which now returns invalid_request instead of capability_unavailable. It was rejected either way.
Real Hopper/Ghidra, browser, or OS coverage: real adb from macOS against an Android 16 emulator and an Android 12 physical device. Not run on Windows or Linux hosts.
Package or release metadata impact: none.
Security, privacy, process, or containment review: an empty operand goes through the existing remote-shell quoting as ''. No new argv shape or shell surface.

Author readiness

Human owner: @chordpli

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/5134 **Author:** [@chordpli](https://github.com/chordpli) **Created:** 10/19/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/adb-empty-string-extra` --- ### 📝 Commits (2) - [`2ca4fe7`](https://github.com/morluto/rea/commit/2ca4fe777993cbf490a7dd126f1521c1e5422962) fix(adb): admit empty string intent extras at the domain boundary - [`8eb1072`](https://github.com/morluto/rea/commit/8eb1072f8d11712f083de944d94e74585f6101c6) test(adb): pin MCP rejection of mistyped intent extras ### 📊 Changes **5 files changed** (+83 additions, -58 deletions) <details> <summary>View changed files</summary> 📝 `src/adb/AdbProvider.test.ts` (+4 -16) 📝 `src/adb/AdbProvider.ts` (+0 -26) 📝 `src/domain/adb/adbDeviceAnalysis.ts` (+49 -14) 📝 `tests/boundary/cli/adbCli.test.ts` (+11 -1) 📝 `tests/boundary/mcp/adbStartMcp.test.ts` (+19 -1) </details> ### 📄 Description ## Summary Fixes #5093. `start_adb_activity` rejected a string extra with an empty value (`--extra string:k:`) as `invalid_request` before the adb provider ran. Android's `am start --es` accepts an empty string, so a caller could not send an intent that carries a present but empty extra. This change admits that value and keeps the type checks for boolean, int, long and float. ## Problem and expected behavior The shared schema put `.min(1)` on every extra value, whatever its type. The CLI parser already keeps an empty trailing field, so the schema was the only place that refused it. While tracing this I found that the typed value forms (`true|false`, integers, decimals) were checked only inside `AdbProvider.startActivity`, after adb lookup. So `int:k:` was rejected at admission (only because of `min(1)`), while `int:k:abc` reached adb lookup first and returned `capability_unavailable` when adb was missing. The same kind of bad input got two different answers depending on the host. Expected: an empty `string` extra reaches the provider and arrives on the device as `""`. A malformed typed value is rejected at admission as `invalid_request`, whether or not adb is installed. ## Change and scope - `src/domain/adb/adbDeviceAnalysis.ts`: drop `.min(1)` from the extra value and check the typed value forms in the domain schema with `superRefine` (issue path `["value"]`). The advertised `value` description now lists the accepted form for each type, because a refinement does not appear in the JSON Schema. - `src/adb/AdbProvider.ts`: remove the provider's duplicate value checks. Every production path reaches the provider through `executeProviderOperation`, which parses `adbRequestSchema` first (CLI via `src/cli/adbCommands.ts`, MCP via `registerAdbTools`). - The accepted forms are unchanged. The regular expressions match the removed provider checks character for character. Not included: - `executeProviderOperation` builds `AnalysisInputError` without `issues`, so CLI callers get `invalid_request` without the field path or the per-type message. This was already true for the old provider checks and affects every single-request provider, so it belongs in a separate change. MCP callers are not affected by this gap; see the contract section below. - The extra type list (`"string" | "boolean" | "int" | "long" | "float"`) is still written out separately in the domain, provider and CLI. - The float form is stricter than Android's `Float.parseFloat` (for example, `1e3` is rejected). I kept the existing behavior. Contribution scope: an ordinary fix inside the existing adb boundary. A maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: the domain input schema for `start_adb_activity`, at shared request admission. - Contribution scope fit and linked maintainer decision: not applicable (fix within an existing tool). - CLI and MCP/tool-catalog contract: the advertised `extras[].value` loses `minLength: 1` and gets a longer description. No properties, types or required fields change. The per-type forms are not expressible in the advertised JSON Schema (refinements do not convert), so they appear only in that description. - CLI: `int:k:abc`, `long:k:1.5`, `float:k:1e3` and `boolean:k:yes` now fail at admission as `invalid_request` instead of after adb lookup. The error keeps the same shape (`code`, `category`, `message`). - MCP: the tool's `inputSchema` is this schema, so the SDK now rejects these values before the handler runs, the same path that every other schema violation (including the old `min(1)`) already took. The response changes from the projected `invalid_request` JSON to SDK text that names the field, for example `Input validation error: Invalid arguments for tool start_adb_activity: extras.0.value: Boolean extra values must be true or false`. A client that parsed `error.code` for this input now gets that text instead. - Provider, bridge, target-format, or platform compatibility: adb argv is unchanged except that an empty string is now possible, and the existing remote-shell quoting sends it as `''`. - Evidence, artifact, provenance, or reconstruction contract: none. `parameters.extras` and `normalized_result.extras` keep the empty value as `""`. - Target selection, authority, upstream behavior, and cleanup details: unchanged. The tool still starts one activity on the selected device. - Generated metadata, package, or installation impact: none committed. `npm run docs:check` passes. Expected effects: - [ ] Read existing files or captured data. - [x] 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. (The launch effect is the existing behavior of `start_adb_activity`. This change only widens the accepted input.) ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/cli/adbCli.test.ts`: `string:key:` reaches provider admission. Valid typed values (`int:key:-3`, `long:key:9007199254740993`, `float:key:-1.5`, `boolean:key:false`) still do. Empty or malformed typed values (`int:key:`, `int:key:abc`, `long:key:1.5`, `float:key:1e3`, `boolean:key:yes`) return `invalid_request` with no adb binary present. On the base revision, `string:key:` returns `invalid_request` and the malformed typed values return `capability_unavailable`, so these cases fail there. - `tests/boundary/mcp/adbStartMcp.test.ts`: the MCP call carries an empty string extra, and the simulated adb receives `--es empty ''`. A `boolean` extra of `yes` is rejected with the SDK input validation text above, and adb is not called. - `src/adb/AdbProvider.test.ts`: the argv golden includes `--es empty ''`. The direct-provider test `rejects mistyped intent extras before touching the device` is removed, because the provider no longer owns that check. The CLI boundary cases above cover it. - Base reproduction or other evidence (executed): the issue's CLI probe on `d1adc6079` printed `string:k:` → `invalid_request`. On this branch it prints `capability_unavailable` with a missing adb, which means the request reached provider admission. - Real-device E2E (executed), base `origin/main` `d1adc6079` vs this branch (`0182e0a67`, identical patch to the rebased `2ca4fe777` per `git range-diff`). Both used the built runtime (`node scripts/rea.mjs`), adb 37.0.1 and a Pixel 8 AVD on Android 16 (API 36). The target was a throwaway test app, `dev.rea.extraprobe`, which logs each received extra with its Java type and length. Screenshots are in a comment below. | Case | Base | This branch | |---|---|---| | `--extra string:mode:probe --extra string:empty:` | `invalid_request`, activity not started | `reported_status: ok`; device log `empty = ""` / `String, length 0` | | `--extra string:mode:probe` (control) | `ok`, `mode = "probe"` | `ok`, `mode = "probe"` (same) | | `--extra int:level:` | `invalid_request` | `invalid_request` (same) | | `--extra int:level:abc`, `REA_ADB_PATH=/nonexistent/adb` | `capability_unavailable` | `invalid_request` | - Physical-device E2E (executed), base `95c8f57aa` vs this branch (`8eb1072f8`) on a Samsung Galaxy Note10+ 5G (SM-N976N), Android 12 (API 31), adb 37.0.1, with the same probe app. The CLI used the built runtime. MCP used `createServer` from source over an in-memory transport, with the real adb and device behind it. No screenshots for this run. | Surface | Case | Base | This branch | |---|---|---|---| | CLI | `--extra string:mode:probe --extra string:empty:` | `invalid_request`, activity not started | `reported_status: ok`; device log `empty = ""` / `String, length 0` | | CLI | `--extra string:mode:probe` (control) | `ok`, `mode = "probe"` | `ok`, `mode = "probe"` (same) | | CLI | `--extra int:level:` | `invalid_request` | `invalid_request` (same) | | CLI | `--extra int:level:abc`, `REA_ADB_PATH=/nonexistent/adb` | `capability_unavailable` | `invalid_request` | | MCP | extras `mode="probe"`, `empty=""` | SDK text: `extras.1.value: Too small: expected string to have >=1 characters` | `reported_status: ok`; device log `empty = ""` / `String, length 0` | | MCP | extra `flag` boolean `yes` | projected JSON `invalid_request` | SDK text: `extras.0.value: Boolean extra values must be true or false` | - User-visible CLI/MCP output: see the table for the CLI. CLI error output keeps the same shape (`code`, `category`, `message`). The MCP error text for a malformed typed value is in the contract section above; I compared it against the base revision through an in-memory MCP client with a simulated adb, where the same call returned the projected `invalid_request` JSON. - Remaining proof gaps: MCP was not run through a stdio server process; the device run used the in-memory transport. Windows and Linux hosts were not run. 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 - `npm run test:local -- src/adb/AdbProvider.test.ts`: 87 passed - `npm run test:focused -- tests/boundary/cli/adbCli.test.ts tests/boundary/mcp/adbStartMcp.test.ts tests/boundary/mcp/adbMcp.test.ts tests/process-global/adbStartMcp.test.ts`: 32 passed - `npm run check:fast`: typecheck and lint passed - `npm run docs:check`: passed - Real-device E2E above: macOS arm64, Node 24.19.0, adb 37.0.1, Pixel 8 AVD (Android 16, API 36) - Physical-device E2E above: macOS arm64, Node 24.19.0, adb 37.0.1, SM-N976N (Android 12, API 31) The test and check commands above ran again after rebasing onto `95c8f57aa` and again at `8eb1072f8`. The emulator E2E ran on the pre-rebase patch noted above; the physical-device E2E ran at `8eb1072f8`. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The only previously valid input that changes outcome is a malformed typed value with adb missing, which now returns `invalid_request` instead of `capability_unavailable`. It was rejected either way. - Real Hopper/Ghidra, browser, or OS coverage: real adb from macOS against an Android 16 emulator and an Android 12 physical device. Not run on Windows or Linux hosts. - Package or release metadata impact: none. - Security, privacy, process, or containment review: an empty operand goes through the existing remote-shell quoting as `''`. No new argv shape or shell surface. ## Author readiness Human owner: @chordpli - [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 - [ ] 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>

bsakaguchi

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

closed this issue
added the
pull-request
label

bsakaguchi changed title from ~~[PR #5134] [CLOSED] fix(adb): admit empty string intent extras at the domain boundary~~ to [PR #5134] fix(adb): admit empty string intent extras at the domain boundary

2026-11-03 13:34:19 +00:00

bsakaguchi changed title from ~~[PR #5134] fix(adb): admit empty string intent extras at the domain boundary~~ to [PR #5134] [CLOSED] fix(adb): admit empty string intent extras at the domain boundary

2026-11-03 14:36:06 +00:00

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

No description provided.

Rows
Columns