feat(android): bootstrap rust android connection#2
Open
skulidropek wants to merge 3 commits into
Open
Conversation
Member
Author
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe crate is renamed to ChangesAndroid connection bootstrap
Sequence Diagram(s)Lifecycle command flow sequenceDiagram
participant User
participant Run as run()
participant Args as docker_run_args()
participant Docker
User->>Run: lifecycle command
Run->>Args: build Docker argv
Run->>Docker: execute Docker command
Docker-->>Run: stdout / exit status
Run-->>User: lifecycle JSON
Bridge server flow sequenceDiagram
participant UI as WebUSB Android bridge UI
participant Web as web()
participant Router as handle_web_client
participant State as BridgeState
UI->>Web: connect / poll
Web->>Router: HTTP request
Router->>State: enqueue / fetch commands
State-->>Router: session state
Router-->>UI: bridge JSON / static assets
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)tests/unit/android_connection.rs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.39][ERROR]: unable to find a config; path 🔧 Stylelint (17.13.0)src/web/styles.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' 🔧 markdownlint-cli2 (0.22.1)README.mdmarkdownlint-cli2 wrapper config was not available before execution changelog.d/20260624_000000_bootstrap_android_connection.mdmarkdownlint-cli2 wrapper config was not available before execution Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skulidropek
force-pushed
the
release/bootstrap-android-connection
branch
from
2a204e7 to
7fd2c8f
Compare
Member
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
tests/integration/cli.rs (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse CLI status output as JSON instead of substring matching.
This makes the test resilient to formatting changes and validates structure, not just text fragments.
Suggested patch
assert!(output.status.success()); let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("\"project_id\": \"dg-test\"")); - assert!(stdout.contains("\"android_container_name\": \"dg-test-android\"")); + let value: serde_json::Value = + serde_json::from_str(&stdout).expect("status output should be valid JSON"); + assert_eq!(value["project_id"], "dg-test"); + assert_eq!(value["android_container_name"], "dg-test-android");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/cli.rs` around lines 17 - 19, The CLI integration test currently checks status output with substring matching, which is brittle. Update the test in the CLI status assertion block to parse stdout as JSON and assert on the structured fields instead of raw text fragments, using the existing output handling around stdout and the project/android container keys to locate the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 60-95: The smoke-test pipeline in the README is broken because the
second `python3 - <<'PY'` consumes stdin for its inline script instead of
reading from `android-connection`, so the response parser never sees the pipe
output. Update the example so the request generator and response reader are
separated correctly, using the existing `android-connection` handshake flow and
the inline Python blocks as distinct producer/consumer steps.
In `@src/lib.rs`:
- Around line 98-119: The endpoint check in is_safe_adb_endpoint() is too loose
because it only enforces a colon and a character whitelist, so malformed values
like missing host/port, nonnumeric ports, or multiple colons still pass through
validate_adb_endpoint() and later fail in run_adb_raw(). Tighten the validation
by parsing the value into exactly one host and one numeric port (and reject
empty parts or extra separators) before returning Ok in validate_adb_endpoint(),
keeping the existing EndpointError path for invalid shapes. Add regression tests
for android:, :5555, android:abc, and a:b:c alongside the current endpoint test
coverage.
In `@src/mcp.rs`:
- Around line 445-462: The adb subprocess invocations in the command handling
flow can block forever because they use plain .output() without any timeout.
Update the adb connect path and the later Command::new("adb").args(args)
execution in the same mcp.rs flow to run with a bounded timeout and terminate
the child process if it expires, returning a McpToolError::CommandFailed that
reflects the timeout condition.
- Around line 530-543: The current workspace_path helper only rejects absolute
paths and parent-dir segments, so symlinked subpaths can still escape the
workspace. Update workspace_path to resolve and validate the canonicalized
workspace plus the target or parent directory before any fs::write or APK
install path use, and ensure the resulting path stays under the workspace root.
Use the existing workspace_path function as the main guard and apply the same
resolved-path check at the call sites that write files or install APKs.
- Around line 129-134: The request routing in handle_request only treats id-less
methods starting with notifications/ as notifications, which still allows
id-less tools/list, tools/call, or unknown methods to emit a response with id:
null. Update handle_request so any request with request.id.is_none() is handled
as a notification and returns None before building the id, while keeping the
existing notification-specific behavior in the method dispatch.
- Around line 80-83: Cap the peer-controlled Content-Length before allocating
the payload in the MCP request path. In `read_message`, validate the parsed
length from `parse_content_length` against a reasonable maximum and return an
error before creating the `payload` Vec or calling `read_exact` if it is too
large. Keep the guard close to the `if let Some(length)` branch so the safety
check is enforced before any allocation or blocking read.
- Around line 445-461: The ADB invocation in run_adb_raw is only using
state.spec.adb_endpoint for the initial connect, but the follow-up
Command::new("adb") call still runs unpinned and can target the wrong device.
Update the shared ADB execution path in run_adb_raw so every adb command
includes the same state.spec.adb_endpoint selection, likely by threading it
through the helper that builds the command arguments and ensuring the final adb
call uses -s with the endpoint as well.
- Around line 302-378: Escape or strictly validate all user-controlled values
before passing them into adb shell commands in android_type_text,
android_launch_app, and android_open_url. Replace the current ad hoc space
substitution in android_type_text with proper shell-safe encoding/escaping for
free-form text, and validate package/activity as Android component identifiers
before building the -n target. Ensure url is encoded or shell-escaped before
being forwarded to am start.
---
Nitpick comments:
In `@tests/integration/cli.rs`:
- Around line 17-19: The CLI integration test currently checks status output
with substring matching, which is brittle. Update the test in the CLI status
assertion block to parse stdout as JSON and assert on the structured fields
instead of raw text fragments, using the existing output handling around stdout
and the project/android container keys to locate the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82cd0695-7ca3-4e9d-85be-0ce42de93168
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlLICENSEREADME.mdchangelog.d/20260624_000000_bootstrap_android_connection.mdexamples/basic_usage.rssrc/bin/android-connection.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/sum.rstests/integration/cli.rstests/integration/mod.rstests/integration/sum.rstests/unit/android_connection.rstests/unit/mod.rstests/unit/sum.rs
💤 Files with no reviewable changes (3)
- src/sum.rs
- tests/integration/sum.rs
- tests/unit/sum.rs
Comment on lines
+98
to
+119
| #[must_use] | ||
| pub fn is_safe_adb_endpoint(value: &str) -> bool { | ||
| !value.is_empty() | ||
| && value.len() <= 255 | ||
| && value.contains(':') | ||
| && value.bytes().all(|byte| { | ||
| matches!( | ||
| byte, | ||
| b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'.' | b'-' | b'_' | b':' | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| pub fn validate_adb_endpoint(value: &str) -> Result<String, EndpointError> { | ||
| if is_safe_adb_endpoint(value) { | ||
| Ok(value.to_string()) | ||
| } else { | ||
| Err(EndpointError { | ||
| value: value.to_string(), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tighten endpoint validation beyond the character whitelist.
is_safe_adb_endpoint() still accepts malformed values like android:, :5555, android:abc, and a:b:c because it only checks for “contains :” plus allowed characters. android_spec() then blesses those values, and run_adb_raw() later forwards them to adb connect, so the promised upfront validation is bypassed and the failure moves to runtime.
Suggested fix
pub fn is_safe_adb_endpoint(value: &str) -> bool {
- !value.is_empty()
- && value.len() <= 255
- && value.contains(':')
- && value.bytes().all(|byte| {
- matches!(
- byte,
- b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'.' | b'-' | b'_' | b':'
- )
- })
+ if value.is_empty() || value.len() > 255 {
+ return false;
+ }
+
+ let (host, port) = match value.split_once(':') {
+ Some((host, port)) if !host.is_empty() && !port.is_empty() && !port.contains(':') => {
+ (host, port)
+ }
+ _ => return false,
+ };
+
+ host.bytes().all(|byte| {
+ matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'.' | b'-' | b'_')
+ }) && port.bytes().all(|byte| byte.is_ascii_digit())
}Please add regression cases for the malformed shapes above alongside the existing endpoint tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[must_use] | |
| pub fn is_safe_adb_endpoint(value: &str) -> bool { | |
| !value.is_empty() | |
| && value.len() <= 255 | |
| && value.contains(':') | |
| && value.bytes().all(|byte| { | |
| matches!( | |
| byte, | |
| b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'.' | b'-' | b'_' | b':' | |
| ) | |
| }) | |
| } | |
| pub fn validate_adb_endpoint(value: &str) -> Result<String, EndpointError> { | |
| if is_safe_adb_endpoint(value) { | |
| Ok(value.to_string()) | |
| } else { | |
| Err(EndpointError { | |
| value: value.to_string(), | |
| }) | |
| } | |
| } | |
| #[must_use] | |
| pub fn is_safe_adb_endpoint(value: &str) -> bool { | |
| if value.is_empty() || value.len() > 255 { | |
| return false; | |
| } | |
| let (host, port) = match value.split_once(':') { | |
| Some((host, port)) if !host.is_empty() && !port.is_empty() && !port.contains(':') => { | |
| (host, port) | |
| } | |
| _ => return false, | |
| }; | |
| host.bytes().all(|byte| { | |
| matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'.' | b'-' | b'_') | |
| }) && port.bytes().all(|byte| byte.is_ascii_digit()) | |
| } | |
| pub fn validate_adb_endpoint(value: &str) -> Result<String, EndpointError> { | |
| if is_safe_adb_endpoint(value) { | |
| Ok(value.to_string()) | |
| } else { | |
| Err(EndpointError { | |
| value: value.to_string(), | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 98 - 119, The endpoint check in
is_safe_adb_endpoint() is too loose because it only enforces a colon and a
character whitelist, so malformed values like missing host/port, nonnumeric
ports, or multiple colons still pass through validate_adb_endpoint() and later
fail in run_adb_raw(). Tighten the validation by parsing the value into exactly
one host and one numeric port (and reject empty parts or extra separators)
before returning Ok in validate_adb_endpoint(), keeping the existing
EndpointError path for invalid shapes. Add regression tests for android:, :5555,
android:abc, and a:b:c alongside the current endpoint test coverage.
Comment on lines
+445
to
+461
| let connect_output = Command::new("adb") | ||
| .arg("connect") | ||
| .arg(&state.spec.adb_endpoint) | ||
| .output() | ||
| .map_err(|error| { | ||
| McpToolError::CommandFailed(format!("failed to execute adb connect: {error}")) | ||
| })?; | ||
| if !connect_output.status.success() { | ||
| return Err(McpToolError::CommandFailed(command_failure( | ||
| "adb connect", | ||
| &connect_output, | ||
| ))); | ||
| } | ||
|
|
||
| Command::new("adb") | ||
| .args(args) | ||
| .output() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '400,520p' src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 4107
🏁 Script executed:
sed -n '400,520p' src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 4107
🏁 Script executed:
rg -n "run_adb_raw|adb_endpoint|adb connect|Command::new\\(\"adb\"\\)" src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 756
🏁 Script executed:
sed -n '1,220p' src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 6616
🏁 Script executed:
rg -n "run_adb_raw|adb_endpoint|adb connect|Command::new\\(\"adb\"\\)" src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 756
🏁 Script executed:
rg -n "run_adb_raw|adb_endpoint|adb connect|Command::new\\(\"adb\"\\)" src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 756
🏁 Script executed:
sed -n '1,240p' src/mcp.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 7534
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
text = Path("src/mcp.rs").read_text()
for needle in ["run_adb_raw", "adb_endpoint", "Command::new(\"adb\")", "adb connect"]:
print(f"--- {needle} ---")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-5)
end = min(len(text.splitlines()), i+10)
for j in range(start, end+1):
print(f"{j}: {text.splitlines()[j-1]}")
break
PYRepository: ProverCoderAI/rust-android-connection
Length of output: 2827
🏁 Script executed:
rg -n "adb_endpoint|allow_install|adb_probe|McpState|connect" -S .Repository: ProverCoderAI/rust-android-connection
Length of output: 7637
🏁 Script executed:
rg -n "struct McpState|adb_probe|adb_endpoint|spec:" src -SRepository: ProverCoderAI/rust-android-connection
Length of output: 1690
Pin all ADB commands to state.spec.adb_endpoint.
run_adb_raw connects first, but then executes plain adb ...; in multi-device setups that can select the wrong target or fail. Pass -s <endpoint> through the shared helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp.rs` around lines 445 - 461, The ADB invocation in run_adb_raw is only
using state.spec.adb_endpoint for the initial connect, but the follow-up
Command::new("adb") call still runs unpinned and can target the wrong device.
Update the shared ADB execution path in run_adb_raw so every adb command
includes the same state.spec.adb_endpoint selection, likely by threading it
through the helper that builds the command arguments and ensuring the final adb
call uses -s with the endpoint as well.
Comment on lines
+445
to
+462
| let connect_output = Command::new("adb") | ||
| .arg("connect") | ||
| .arg(&state.spec.adb_endpoint) | ||
| .output() | ||
| .map_err(|error| { | ||
| McpToolError::CommandFailed(format!("failed to execute adb connect: {error}")) | ||
| })?; | ||
| if !connect_output.status.success() { | ||
| return Err(McpToolError::CommandFailed(command_failure( | ||
| "adb connect", | ||
| &connect_output, | ||
| ))); | ||
| } | ||
|
|
||
| Command::new("adb") | ||
| .args(args) | ||
| .output() | ||
| .map_err(|error| McpToolError::CommandFailed(format!("failed to execute adb: {error}"))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add timeouts around ADB subprocesses.
adb connect and tool commands use blocking .output() calls; if ADB hangs, the single stdio request loop is stuck indefinitely. Wrap these subprocesses with a bounded timeout and kill on expiry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp.rs` around lines 445 - 462, The adb subprocess invocations in the
command handling flow can block forever because they use plain .output() without
any timeout. Update the adb connect path and the later
Command::new("adb").args(args) execution in the same mcp.rs flow to run with a
bounded timeout and terminate the child process if it expires, returning a
McpToolError::CommandFailed that reflects the timeout condition.
Member
Author
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 307-308: The default ADB endpoint is mismatched with the Docker
DNS name used by the Android container, so `DEFAULT_ADB_ENDPOINT` should resolve
to the container actually joined by `docker_run_args()`. Update the Docker setup
in `docker_run_args()` / `spec.android_container_name` handling so the Android
container is reachable as `android` (for example via a network alias), or change
the default endpoint derivation to use the generated container name consistently
across the `DEFAULT_ADB_ENDPOINT` and Docker networking logic.
In `@src/main.rs`:
- Around line 352-360: The copy_to_stderr helper currently holds the stderr lock
for the entire io::copy call, which can block the other Docker pipe reader and
cause hangs. Update copy_to_stderr so it reads from the provided Read in chunks
first, then acquires io::stderr().lock() only long enough to write each chunk
and flush as needed, keeping the lock scope minimal within the thread::spawn
closure.
In `@tests/integration/cli.rs`:
- Around line 4-48: The lifecycle CLI `status` path still shells out to Docker
to resolve `noVncUrl`, which makes the `lifecycle_cli_renders_status_json` and
`lifecycle_cli_can_disable_no_vnc_publication` tests depend on Docker. Update
the `status` implementation to avoid calling `docker port` during status
rendering, or add an explicit opt-out/override for test runs so
`parse_stdout_json` can validate `noVncUrl` and `noVncPublished` offline. Focus
the change in the `status` command handling and the `noVncUrl` resolution logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c3bc73e-4d26-42f3-87d3-4837f21b61a3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlLICENSEREADME.mdchangelog.d/20260624_000000_bootstrap_android_connection.mdexamples/basic_usage.rssrc/bin/android-connection.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/sum.rstests/integration/cli.rstests/integration/mod.rstests/integration/sum.rstests/unit/android_connection.rstests/unit/mod.rstests/unit/sum.rs
💤 Files with no reviewable changes (3)
- tests/unit/sum.rs
- tests/integration/sum.rs
- src/sum.rs
✅ Files skipped from review due to trivial changes (5)
- changelog.d/20260624_000000_bootstrap_android_connection.md
- tests/integration/mod.rs
- LICENSE
- tests/unit/mod.rs
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/basic_usage.rs
- src/bin/android-connection.rs
- src/mcp.rs
- Cargo.toml
Comment on lines
+307
to
+308
| "--network".to_string(), | ||
| spec.docker_network.clone(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files | rg '^(src/lib\.rs|src/|README|.*adb.*|.*docker.*)$' || true
printf '\n== search symbols ==\n'
rg -n "DEFAULT_ADB_ENDPOINT|adb_endpoint|docker_run_args|docker_network|network-alias|android:5555|5555" src README.md
printf '\n== src/lib.rs excerpt ==\n'
sed -n '250,360p' src/lib.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 5761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files | rg '^(src/lib\.rs|src/|README|.*adb.*|.*docker.*)$' || true
printf '\n== search symbols ==\n'
rg -n "DEFAULT_ADB_ENDPOINT|adb_endpoint|docker_run_args|docker_network|network-alias|android:5555|5555" src README.md
printf '\n== src/lib.rs excerpt ==\n'
sed -n '250,360p' src/lib.rsRepository: ProverCoderAI/rust-android-connection
Length of output: 5761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/lib.rs
printf '\n== main.rs excerpt ==\n'
sed -n '1,120p' src/main.rs
printf '\n== README excerpt ==\n'
sed -n '35,60p' README.mdRepository: ProverCoderAI/rust-android-connection
Length of output: 12023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/lib.rs
printf '\n== main.rs excerpt ==\n'
sed -n '1,120p' src/main.rs
printf '\n== README excerpt ==\n'
sed -n '35,60p' README.mdRepository: ProverCoderAI/rust-android-connection
Length of output: 12023
Align the default ADB host with the Docker container name.
DEFAULT_ADB_ENDPOINT is android:5555, but docker_run_args() only joins the network under spec.android_container_name (for example dg-my-project-android), so the default endpoint does not resolve on the Docker network. Add a --network-alias android or derive the default endpoint from the generated Android container name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 307 - 308, The default ADB endpoint is mismatched
with the Docker DNS name used by the Android container, so
`DEFAULT_ADB_ENDPOINT` should resolve to the container actually joined by
`docker_run_args()`. Update the Docker setup in `docker_run_args()` /
`spec.android_container_name` handling so the Android container is reachable as
`android` (for example via a network alias), or change the default endpoint
derivation to use the generated container name consistently across the
`DEFAULT_ADB_ENDPOINT` and Docker networking logic.
Comment on lines
+352
to
+360
| fn copy_to_stderr<R>(mut reader: R) -> thread::JoinHandle<io::Result<()>> | ||
| where | ||
| R: Read + Send + 'static, | ||
| { | ||
| thread::spawn(move || { | ||
| let mut stderr = io::stderr().lock(); | ||
| io::copy(&mut reader, &mut stderr)?; | ||
| stderr.flush() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid holding stderr locked while draining Docker pipes.
Each copy thread locks stderr for the full io::copy; if one stream holds the lock while reading and Docker fills the other pipe, docker pull can hang. Read chunks first, then lock only for each write.
Suggested fix
fn copy_to_stderr<R>(mut reader: R) -> thread::JoinHandle<io::Result<()>>
where
R: Read + Send + 'static,
{
thread::spawn(move || {
- let mut stderr = io::stderr().lock();
- io::copy(&mut reader, &mut stderr)?;
- stderr.flush()
+ let mut buffer = [0_u8; 8192];
+ loop {
+ let bytes_read = reader.read(&mut buffer)?;
+ if bytes_read == 0 {
+ return Ok(());
+ }
+
+ let mut stderr = io::stderr().lock();
+ stderr.write_all(&buffer[..bytes_read])?;
+ stderr.flush()?;
+ }
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn copy_to_stderr<R>(mut reader: R) -> thread::JoinHandle<io::Result<()>> | |
| where | |
| R: Read + Send + 'static, | |
| { | |
| thread::spawn(move || { | |
| let mut stderr = io::stderr().lock(); | |
| io::copy(&mut reader, &mut stderr)?; | |
| stderr.flush() | |
| }) | |
| fn copy_to_stderr<R>(mut reader: R) -> thread::JoinHandle<io::Result<()>> | |
| where | |
| R: Read + Send + 'static, | |
| { | |
| thread::spawn(move || { | |
| let mut buffer = [0_u8; 8192]; | |
| loop { | |
| let bytes_read = reader.read(&mut buffer)?; | |
| if bytes_read == 0 { | |
| return Ok(()); | |
| } | |
| let mut stderr = io::stderr().lock(); | |
| stderr.write_all(&buffer[..bytes_read])?; | |
| stderr.flush()?; | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 352 - 360, The copy_to_stderr helper currently
holds the stderr lock for the entire io::copy call, which can block the other
Docker pipe reader and cause hangs. Update copy_to_stderr so it reads from the
provided Read in chunks first, then acquires io::stderr().lock() only long
enough to write each chunk and flush as needed, keeping the lock scope minimal
within the thread::spawn closure.
- Rename CLI package/binary to rust-android-connection - Add container ADB proxy, phone WebUSB bridge, active browser lease, and browser-side screenshots - Remove MCP server entrypoint in favor of CLI/WebUSB commands - Validate with cargo fmt -- --check, node --check src/web/app.js, and cargo test --locked
Member
Author
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 315-319: The default interactive profile is reusing the app-test
emulator spec, so update the interactive startup flow to stop pulling in
APP_TEST_EMULATOR_CONFIG_* from interactive_runtime_options() and
docker_startup_command(). Use a separate full/default emulator config for the
interactive profile while keeping the app-test config only for the app-test
path, and make sure the symbols interactive_runtime_options,
docker_startup_command, and APP_TEST_EMULATOR_CONFIG_LINES no longer force the
cut-down device settings into the default runtime.
In `@src/main.rs`:
- Around line 1344-1354: The file-attachment authorization in main request
handling is using the client-controlled Host header, which allows remote clients
to spoof localhost. Update the checks in the request flow around the file
attachment gate and the file URL/download path to validate the peer’s actual
remote address instead of http_header_value(..., "host"), using the existing
request/connection metadata in the relevant handler methods. Keep the localhost
restriction, but base it on the socket peer address so both the enqueue path and
the download path enforce the same trust boundary.
- Around line 1426-1444: `BridgeState.results` is never cleared after a command
result is delivered, so the GET handler for `/bridge/commands/result/` keeps old
stdout/stderr/base64 payloads in memory indefinitely. Update the result
retrieval path in `main` to evict the entry from `bridge.results` after it is
read, using the existing `result.id`/request `id` flow in the bridge command
result handler, while preserving the current response behavior.
- Around line 421-435: The bridge request flow in main currently blocks
indefinitely on TcpStream::connect and read_to_end, so PHONE_BRIDGE_TIMEOUT is
not enforced for stalled peers. Update the HTTP call path to apply a
connection/read timeout around the bridge request in the same request-handling
logic, and cap how much response data is accepted before parse_http_response is
called. Make the timeout/boundary behavior explicit in the bridge client code
that builds the request and reads the response.
- Around line 447-452: The bridge URL parsing in the authority handling logic
currently hides invalid ports by falling back to DEFAULT_WEB_PORT, which can
misdirect requests. Update the parsing in the authority/port extraction branch
to reject malformed ports explicitly instead of using
unwrap_or(DEFAULT_WEB_PORT), and surface an error when `port.parse::<u16>()`
fails. Keep the fix localized to the authority split/parsing path in `main.rs`
so valid host:port inputs still work while malformed bridge URLs are rejected.
In `@src/web/app.js`:
- Around line 1249-1255: The postBridgeResult flow in app.js treats every
fetch() resolution as success, so stale-client or server failures from
/bridge/commands/result can incorrectly mark the command complete. Update
postBridgeResult to inspect the response status, mirror the poll-side 409
handling, and throw or surface an error for any non-2xx response before the
activity is marked done; apply the same change in the other referenced
submission path as well.
- Around line 88-95: The client logging path is leaking stable device
identifiers: update postClientLog and logBackendDetails so they do not emit raw
serials, client IDs, or full location URLs by default. Redact or hash the serial
values before building the payload, strip query strings from
window.location.href unless verbose diagnostics are enabled, and ensure any
bridge-server stderr logging of the client-log payload uses the sanitized
version. Use the existing postClientLog and logBackendDetails symbols to apply
the fix consistently wherever these fields are collected or printed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5055ad09-9e82-4363-b806-eeb8c95c4a88
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mdchangelog.d/20260624_000000_bootstrap_android_connection.mdexamples/basic_usage.rssrc/lib.rssrc/main.rssrc/web/app.jssrc/web/index.htmlsrc/web/styles.csstests/integration/cli.rstests/unit/android_connection.rs
✅ Files skipped from review due to trivial changes (2)
- src/web/index.html
- changelog.d/20260624_000000_bootstrap_android_connection.md
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/basic_usage.rs
Comment on lines
+315
to
+319
| emulator_config_path: Some(APP_TEST_EMULATOR_CONFIG_PATH.to_string()), | ||
| emulator_config_lines: APP_TEST_EMULATOR_CONFIG_LINES | ||
| .iter() | ||
| .map(|line| (*line).to_string()) | ||
| .collect(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't apply the app-test emulator spec to the default interactive profile.
interactive_runtime_options() reuses APP_TEST_EMULATOR_CONFIG_*, and docker_startup_command() always writes those lines before launch. That makes the default interactive profile inherit the cut-down app-test device config (for example GSM/audio/sensor changes), which contradicts the README's "full visual" profile and changes the default runtime behavior.
Proposed fix
pub fn interactive_runtime_options() -> AndroidRuntimeOptions {
AndroidRuntimeOptions {
profile: DEFAULT_ANDROID_RUNTIME_PROFILE.to_string(),
emulator_headless: RuntimeSwitch::Disabled,
appium_enabled: RuntimeSwitch::Disabled,
web_log_enabled: RuntimeSwitch::Disabled,
web_vnc_enabled: RuntimeSwitch::Enabled,
emulator_no_skin: RuntimeSwitch::Disabled,
emulator_device: "Nexus 5".to_string(),
emulator_data_partition: "2g".to_string(),
emulator_additional_args: "-no-audio -no-boot-anim".to_string(),
- emulator_config_path: Some(APP_TEST_EMULATOR_CONFIG_PATH.to_string()),
- emulator_config_lines: APP_TEST_EMULATOR_CONFIG_LINES
- .iter()
- .map(|line| (*line).to_string())
- .collect(),
+ emulator_config_path: None,
+ emulator_config_lines: Vec::new(),
}
}Also applies to: 461-466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 315 - 319, The default interactive profile is
reusing the app-test emulator spec, so update the interactive startup flow to
stop pulling in APP_TEST_EMULATOR_CONFIG_* from interactive_runtime_options()
and docker_startup_command(). Use a separate full/default emulator config for
the interactive profile while keeping the app-test config only for the app-test
path, and make sure the symbols interactive_runtime_options,
docker_startup_command, and APP_TEST_EMULATOR_CONFIG_LINES no longer force the
cut-down device settings into the default runtime.
Comment on lines
+421
to
+435
| let mut stream = TcpStream::connect((parsed.host.as_str(), parsed.port))?; | ||
| let body = body.unwrap_or(""); | ||
| let request = format!( | ||
| "{method} {} HTTP/1.1\r\nHost: {}\r\nContent-Type: application/json\r\nAccept: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | ||
| parsed.path, | ||
| parsed.host, | ||
| body.len(), | ||
| body | ||
| ); | ||
| stream.write_all(request.as_bytes())?; | ||
| stream.flush()?; | ||
|
|
||
| let mut response = Vec::new(); | ||
| stream.read_to_end(&mut response)?; | ||
| parse_http_response(&response) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound bridge HTTP calls so the phone CLI cannot hang indefinitely.
PHONE_BRIDGE_TIMEOUT does not apply while connect/read_to_end is blocked. A bridge peer that accepts the socket and stalls can hang the command forever; also cap the response body.
Suggested fix
+const PHONE_BRIDGE_IO_TIMEOUT: Duration = Duration::from_secs(30);
+const MAX_PHONE_BRIDGE_RESPONSE_BYTES: u64 = 33 * 1024 * 1024;
+
fn http_request_json(
method: &str,
url: &str,
body: Option<&str>,
) -> Result<(u16, String), Box<dyn std::error::Error>> {
let parsed = parse_http_url(url)?;
let mut stream = TcpStream::connect((parsed.host.as_str(), parsed.port))?;
+ stream.set_read_timeout(Some(PHONE_BRIDGE_IO_TIMEOUT))?;
+ stream.set_write_timeout(Some(PHONE_BRIDGE_IO_TIMEOUT))?;
let body = body.unwrap_or("");
@@
- let mut response = Vec::new();
- stream.read_to_end(&mut response)?;
+ let mut response = Vec::new();
+ stream
+ .take(MAX_PHONE_BRIDGE_RESPONSE_BYTES + 1)
+ .read_to_end(&mut response)?;
+ if response.len() as u64 > MAX_PHONE_BRIDGE_RESPONSE_BYTES {
+ return Err("bridge response exceeded maximum size".into());
+ }
parse_http_response(&response)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut stream = TcpStream::connect((parsed.host.as_str(), parsed.port))?; | |
| let body = body.unwrap_or(""); | |
| let request = format!( | |
| "{method} {} HTTP/1.1\r\nHost: {}\r\nContent-Type: application/json\r\nAccept: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | |
| parsed.path, | |
| parsed.host, | |
| body.len(), | |
| body | |
| ); | |
| stream.write_all(request.as_bytes())?; | |
| stream.flush()?; | |
| let mut response = Vec::new(); | |
| stream.read_to_end(&mut response)?; | |
| parse_http_response(&response) | |
| const PHONE_BRIDGE_IO_TIMEOUT: Duration = Duration::from_secs(30); | |
| const MAX_PHONE_BRIDGE_RESPONSE_BYTES: u64 = 33 * 1024 * 1024; | |
| let mut stream = TcpStream::connect((parsed.host.as_str(), parsed.port))?; | |
| stream.set_read_timeout(Some(PHONE_BRIDGE_IO_TIMEOUT))?; | |
| stream.set_write_timeout(Some(PHONE_BRIDGE_IO_TIMEOUT))?; | |
| let body = body.unwrap_or(""); | |
| let request = format!( | |
| "{method} {} HTTP/1.1\r\nHost: {}\r\nContent-Type: application/json\r\nAccept: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | |
| parsed.path, | |
| parsed.host, | |
| body.len(), | |
| body | |
| ); | |
| stream.write_all(request.as_bytes())?; | |
| stream.flush()?; | |
| let mut response = Vec::new(); | |
| stream | |
| .take(MAX_PHONE_BRIDGE_RESPONSE_BYTES + 1) | |
| .read_to_end(&mut response)?; | |
| if response.len() as u64 > MAX_PHONE_BRIDGE_RESPONSE_BYTES { | |
| return Err("bridge response exceeded maximum size".into()); | |
| } | |
| parse_http_response(&response) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 421 - 435, The bridge request flow in main
currently blocks indefinitely on TcpStream::connect and read_to_end, so
PHONE_BRIDGE_TIMEOUT is not enforced for stalled peers. Update the HTTP call
path to apply a connection/read timeout around the bridge request in the same
request-handling logic, and cap how much response data is accepted before
parse_http_response is called. Make the timeout/boundary behavior explicit in
the bridge client code that builds the request and reads the response.
Comment on lines
+447
to
+452
| let (host, port) = authority.rsplit_once(':').map_or_else( | ||
| || (authority.to_string(), DEFAULT_WEB_PORT), | ||
| |(host, port)| { | ||
| let parsed_port = port.parse::<u16>().unwrap_or(DEFAULT_WEB_PORT); | ||
| (host.to_string(), parsed_port) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed bridge URL ports instead of silently using 8080.
http://127.0.0.1:notaport currently falls back to DEFAULT_WEB_PORT, so a typo can target the wrong bridge.
Suggested fix
- let (host, port) = authority.rsplit_once(':').map_or_else(
- || (authority.to_string(), DEFAULT_WEB_PORT),
- |(host, port)| {
- let parsed_port = port.parse::<u16>().unwrap_or(DEFAULT_WEB_PORT);
- (host.to_string(), parsed_port)
- },
- );
+ let (host, port) = if let Some((host, port)) = authority.rsplit_once(':') {
+ let parsed_port = port
+ .parse::<u16>()
+ .map_err(|_| format!("invalid phone bridge URL port: {port}"))?;
+ (host.to_string(), parsed_port)
+ } else {
+ (authority.to_string(), DEFAULT_WEB_PORT)
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (host, port) = authority.rsplit_once(':').map_or_else( | |
| || (authority.to_string(), DEFAULT_WEB_PORT), | |
| |(host, port)| { | |
| let parsed_port = port.parse::<u16>().unwrap_or(DEFAULT_WEB_PORT); | |
| (host.to_string(), parsed_port) | |
| }, | |
| let (host, port) = if let Some((host, port)) = authority.rsplit_once(':') { | |
| let parsed_port = port | |
| .parse::<u16>() | |
| .map_err(|_| format!("invalid phone bridge URL port: {port}"))?; | |
| (host.to_string(), parsed_port) | |
| } else { | |
| (authority.to_string(), DEFAULT_WEB_PORT) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 447 - 452, The bridge URL parsing in the authority
handling logic currently hides invalid ports by falling back to
DEFAULT_WEB_PORT, which can misdirect requests. Update the parsing in the
authority/port extraction branch to reject malformed ports explicitly instead of
using unwrap_or(DEFAULT_WEB_PORT), and surface an error when
`port.parse::<u16>()` fails. Keep the fix localized to the authority
split/parsing path in `main.rs` so valid host:port inputs still work while
malformed bridge URLs are rejected.
Comment on lines
+1344
to
+1354
| if request.file_path.is_some() | ||
| && !is_local_http_host(http_header_value(initial, "host").as_deref()) | ||
| { | ||
| return write_http_response( | ||
| stream, | ||
| "403 Forbidden", | ||
| "text/plain; charset=utf-8", | ||
| "file attachments must be enqueued from localhost\n", | ||
| false, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Authorize file attachments by peer address, not Host.
Host is client-controlled. With web --bind 0.0.0.0, a remote client can send Host: localhost, enqueue a local filePath, then download it through the returned file URL.
Suggested fix
- if request.file_path.is_some()
- && !is_local_http_host(http_header_value(initial, "host").as_deref())
- {
+ if request.file_path.is_some() && !is_loopback_peer(stream) {
return write_http_response(
stream,
"403 Forbidden",
@@
}
}
+
+fn is_loopback_peer(stream: &TcpStream) -> bool {
+ stream
+ .peer_addr()
+ .is_ok_and(|address| address.ip().is_loopback())
+}Also applies to: 1485-1490
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 1344 - 1354, The file-attachment authorization in
main request handling is using the client-controlled Host header, which allows
remote clients to spoof localhost. Update the checks in the request flow around
the file attachment gate and the file URL/download path to validate the peer’s
actual remote address instead of http_header_value(..., "host"), using the
existing request/connection metadata in the relevant handler methods. Keep the
localhost restriction, but base it on the socket peer address so both the
enqueue path and the download path enforce the same trust boundary.
Comment on lines
+1426
to
+1444
| bridge.files.remove(&result.id); | ||
| bridge.results.insert(result.id.clone(), result); | ||
| } | ||
| write_http_response( | ||
| stream, | ||
| "204 No Content", | ||
| "text/plain; charset=utf-8", | ||
| "", | ||
| false, | ||
| ) | ||
| } | ||
| ("GET", path) if path.starts_with("/bridge/commands/result/") => { | ||
| let id = path.trim_start_matches("/bridge/commands/result/"); | ||
| let result = { | ||
| let bridge = bridge | ||
| .lock() | ||
| .map_err(|_| io::Error::new(io::ErrorKind::Other, "bridge state poisoned"))?; | ||
| bridge.results.get(id).cloned() | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Evict bridge results after delivery.
Command stdout/stderr/base64 results are retained in BridgeState.results forever. Repeated commands can grow memory and keep command output available longer than needed.
Suggested fix
let result = {
- let bridge = bridge
+ let mut bridge = bridge
.lock()
.map_err(|_| io::Error::new(io::ErrorKind::Other, "bridge state poisoned"))?;
- bridge.results.get(id).cloned()
+ bridge.results.remove(id)
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bridge.files.remove(&result.id); | |
| bridge.results.insert(result.id.clone(), result); | |
| } | |
| write_http_response( | |
| stream, | |
| "204 No Content", | |
| "text/plain; charset=utf-8", | |
| "", | |
| false, | |
| ) | |
| } | |
| ("GET", path) if path.starts_with("/bridge/commands/result/") => { | |
| let id = path.trim_start_matches("/bridge/commands/result/"); | |
| let result = { | |
| let bridge = bridge | |
| .lock() | |
| .map_err(|_| io::Error::new(io::ErrorKind::Other, "bridge state poisoned"))?; | |
| bridge.results.get(id).cloned() | |
| }; | |
| bridge.files.remove(&result.id); | |
| bridge.results.insert(result.id.clone(), result); | |
| } | |
| write_http_response( | |
| stream, | |
| "204 No Content", | |
| "text/plain; charset=utf-8", | |
| "", | |
| false, | |
| ) | |
| } | |
| ("GET", path) if path.starts_with("/bridge/commands/result/") => { | |
| let id = path.trim_start_matches("/bridge/commands/result/"); | |
| let result = { | |
| let mut bridge = bridge | |
| .lock() | |
| .map_err(|_| io::Error::new(io::ErrorKind::Other, "bridge state poisoned"))?; | |
| bridge.results.remove(id) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 1426 - 1444, `BridgeState.results` is never cleared
after a command result is delivered, so the GET handler for
`/bridge/commands/result/` keeps old stdout/stderr/base64 payloads in memory
indefinitely. Update the result retrieval path in `main` to evict the entry from
`bridge.results` after it is read, using the existing `result.id`/request `id`
flow in the bridge command result handler, while preserving the current response
behavior.
Comment on lines
+88
to
+95
| const postClientLog = (entry) => { | ||
| const payload = JSON.stringify({ | ||
| ...entry, | ||
| location: window.location.href, | ||
| clientId: state.bridgeClientId || "-", | ||
| serial: elements.deviceSerial?.textContent || "-", | ||
| state: elements.devicePill?.textContent || "-", | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact stable device identifiers in client logs.
postClientLog includes location, clientId, and the current serial, while logBackendDetails logs the raw USB serial; the bridge server prints non-raw client-log payloads to stderr. Please redact/hash serials and strip query strings unless verbose diagnostics are explicitly enabled.
Also applies to: 261-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/web/app.js` around lines 88 - 95, The client logging path is leaking
stable device identifiers: update postClientLog and logBackendDetails so they do
not emit raw serials, client IDs, or full location URLs by default. Redact or
hash the serial values before building the payload, strip query strings from
window.location.href unless verbose diagnostics are enabled, and ensure any
bridge-server stderr logging of the client-log payload uses the sanitized
version. Use the existing postClientLog and logBackendDetails symbols to apply
the fix consistently wherever these fields are collected or printed.
Comment on lines
+1249
to
+1255
| const postBridgeResult = async (result) => { | ||
| await fetch("/bridge/commands/result", { | ||
| method: "POST", | ||
| headers: bridgeHeaders({ "Content-Type": "application/json" }), | ||
| body: JSON.stringify(result), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check bridge result submission failures.
fetch() resolves for HTTP 409/500, so a stale-client rejection from /bridge/commands/result is currently treated as a successful command completion. Mirror the poll-side 409 handling and throw on other non-2xx responses before updating the activity as done.
Proposed fix
const postBridgeResult = async (result) => {
- await fetch("/bridge/commands/result", {
+ const response = await fetch("/bridge/commands/result", {
method: "POST",
headers: bridgeHeaders({ "Content-Type": "application/json" }),
body: JSON.stringify(result),
});
+ if (response.status === 409) {
+ state.bridgePollingEnabled = false;
+ state.bridgeClientId = null;
+ await disconnectDevice();
+ throw new Error("Bridge client session lost while submitting result");
+ }
+ if (!response.ok) {
+ throw new Error(`bridge result failed: HTTP ${response.status}`);
+ }
};Also applies to: 1297-1308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/web/app.js` around lines 1249 - 1255, The postBridgeResult flow in app.js
treats every fetch() resolution as success, so stale-client or server failures
from /bridge/commands/result can incorrectly mark the command complete. Update
postBridgeResult to inspect the response status, mirror the poll-side 409
handling, and throw or surface an error for any non-2xx response before the
activity is marked done; apply the same change in the other referenced
submission path as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bootstrap Android connection crate
This PR moves the docker-git Android connection codebase into the standalone
rust-android-connectionrepository.What changed
android-connectioncompatibility binary.Mathematical guarantees
Cargo.lockare committed, so binary installation with--lockedis reproducible for this revision.Proof of fix
Verification
Docker-git integration PR: ProverCoderAI/docker-git#437