feat(adb): add device auto-connect and connect/disconnect methods - #977
Open
bennyz wants to merge 1 commit into
Open
feat(adb): add device auto-connect and connect/disconnect methods#977bennyz wants to merge 1 commit into
bennyz wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesADB device connection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AdbClient
participant AdbServer
participant adb_command
AdbClient->>AdbServer: connect_device(device)
AdbServer->>adb_command: Run adb connect device
adb_command-->>AdbServer: Return output or error
AdbClient->>AdbServer: disconnect_device(device)
AdbServer->>adb_command: Run adb disconnect device
adb_command-->>AdbServer: Return output or error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py`:
- Around line 116-123: Add a finite timeout to the subprocess.run calls used by
connect_device and disconnect_device, catch subprocess.TimeoutExpired, and
preserve the driver’s existing failure behavior when ADB hangs. Add
driver_test.py coverage verifying the timeout path for these ADB connection
commands.
🪄 Autofix
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: 453af8db-ba36-4479-9d27-31b68c23790d
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/client.pypython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py
Comment on lines
+116
to
+123
| result = subprocess.run( | ||
| [self.adb_path, "connect", device], | ||
| check=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| env=self._adb_env(), | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'python/packages/jumpstarter-driver-adb|driver_test.py$|driver.py$' | sed -n '1,120p'
echo "== outline driver =="
ast-grep outline python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py --view expanded || true
echo "== outline driver_test =="
ast-grep outline python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/*test*.py --view expanded || true
echo "== relevant driver lines =="
cat -n python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py | sed -n '1,230p'
echo "== relevant test lines =="
for f in $(git ls-files | rg 'python/packages/jumpstarter-driver-adb/.+_test\.py$|python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb.*test.*'); do
echo "--- $f"
cat -n "$f" | sed -n '1,260p'
done
echo "== subprocess.run timeout usages in adb package =="
rg -n "subprocess\.run|TimeoutExpired|connect|disconnect" python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb -SRepository: jumpstarter-dev/jumpstarter
Length of output: 1940
Add a timeout for ADB connection commands.
subprocess.run has no timeout and can block exporter initialization or connect_device/disconnect_device if adb hangs. Add a finite timeout to the ADB commands around Lines 116-123 and 141-148, handle subprocess.TimeoutExpired, and add coverage for the timeout path in driver_test.py.
🤖 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 `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py`
around lines 116 - 123, Add a finite timeout to the subprocess.run calls used by
connect_device and disconnect_device, catch subprocess.TimeoutExpired, and
preserve the driver’s existing failure behavior when ADB hangs. Add
driver_test.py coverage verifying the timeout path for these ADB connection
commands.
Source: Coding guidelines
bennyz
force-pushed
the
adb-device-connect
branch
from
August 7, 2026 12:50
eb0be97 to
ff2c6e3
Compare
Allow ADB driver to automatically connect to a remote device on startup via the `device` config field, enabling use cases where the exporter is not co-located with the Android device, like with cuttlefish targets Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
adb-device-connect
branch
from
August 8, 2026 13:08
ff2c6e3 to
de76136
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py`:
- Around line 95-106: Update test_connect_device to clear mock_run.side_effect
after reset_mock() before assigning mock_run.return_value, then assert the exact
stripped response for 10.0.0.2:6520 rather than only checking for “connected”.
In `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py`:
- Around line 109-130: The AdbServer startup path must auto-connect configured
remote devices. In
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py lines
109-130, update AdbServer.__post_init__ to read the optional device
configuration and call _connect_device after starting the server when
configured; leave startup unchanged when no device is set. In
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py
lines 86-90, add coverage for configured-device startup expecting the adb
connect subprocess call, while retaining the two-call assertion only for
no-device configuration.
🪄 Autofix
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: e103f067-80e7-4db1-b887-fb8a0c2fc856
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.pypython/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py
Comment on lines
+95
to
+106
| def test_connect_device(mock_run, _): | ||
| mock_run.side_effect = [ | ||
| _mock_adb_ok(), # version check | ||
| _mock_adb_ok(), # start-server | ||
| MagicMock(stdout="connected to 10.0.0.1:6520\n", stderr="", returncode=0), | ||
| ] | ||
| server = AdbServer() | ||
| mock_run.reset_mock() | ||
| mock_run.return_value = MagicMock(stdout="connected to 10.0.0.2:6520\n", stderr="", returncode=0) | ||
| result = server.connect_device("10.0.0.2:6520") | ||
| assert "connected" in result | ||
| assert mock_run.call_args[0][0] == ["/usr/bin/adb", "connect", "10.0.0.2:6520"] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test\.py|driver\.py' || true
echo "== test block =="
if [ -f python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py ]; then
sed -n '1,150p' python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py | nl -ba
fi
echo "== mock reset behavior probe =="
python3 - <<'PY'
from unittest.mock import MagicMock, Mock
m = Mock(side_effect=["one", "two", "three"], return_value="default")
for i, val in enumerate([m(), m(), m(), m()], start=1):
print(f"before reset call {i}: {val!r}")
m.reset_mock()
for x in [m(), m(), m("x")]:
print(f"after reset: {x!r}")
print("after reset side_effect:", m.side_effect)
print("after reset return_value:", m.return_value)
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 4332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test block =="
sed -n '1,150p' python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py
echo "== mock reset behavior probe =="
python3 - <<'PY'
from unittest.mock import MagicMock, Mock
m = Mock(side_effect=["one", "two", "three"], return_value="default")
for i, val in enumerate([m(), m(), m(), m()], start=1):
print(f"before reset call {i}: {val!r}")
m.reset_mock()
for x in [m(), m(), m("x")]:
print(f"after reset: {x!r}")
print("after reset side_effect:", m.side_effect)
print("after reset return_value:", m.return_value)
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 5619
Clear side_effect before setting return_value.
reset_mock() does not clear side_effect, so the call for 10.0.0.2:6520 uses the remaining 10.0.0.1:6520 response instead of the response configured on the next line. Clear side_effect before assigning return_value, and assert the exact stripped response for the new address.
🤖 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 `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py`
around lines 95 - 106, Update test_connect_device to clear mock_run.side_effect
after reset_mock() before assigning mock_run.return_value, then assert the exact
stripped response for 10.0.0.2:6520 rather than only checking for “connected”.
Source: Coding guidelines
Comment on lines
+109
to
+130
| def _connect_device(self, device: str) -> str: | ||
| self.logger.info(f"Connecting to device {device}") | ||
| try: | ||
| result = subprocess.run( | ||
| [self.adb_path, "connect", device], | ||
| check=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| env=self._adb_env(), | ||
| ) | ||
| output = result.stdout.strip() | ||
| self.logger.info(output) | ||
| return output | ||
| except subprocess.CalledProcessError as e: | ||
| self.logger.error(f"Failed to connect to device {device}: {e}") | ||
| return f"Error: {e}" | ||
|
|
||
| @export | ||
| def connect_device(self, device: str) -> str: | ||
| """Connect to an ADB device by address (host:port).""" | ||
| return self._connect_device(device) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement the required startup auto-connect.
AdbServer.__post_init__ starts the ADB server but never calls _connect_device. The new method is only reachable through an explicit RPC call. The initialization test also requires exactly two subprocess calls. A configured remote device will not connect at startup.
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py#L109-L130: add the optional device configuration and invoke_connect_deviceafter server startup when it is configured.python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py#L86-L90: add coverage for configured-device startup and expect theadb connectcommand; retain the two-call assertion only for no-device configuration.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 111-118: Command coming from incoming request
Context: subprocess.run(
[self.adb_path, "connect", device],
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
env=self._adb_env(),
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
📍 Affects 2 files
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py#L109-L130(this comment)python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py#L86-L90
🤖 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 `@python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py`
around lines 109 - 130, The AdbServer startup path must auto-connect configured
remote devices. In
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver.py lines
109-130, update AdbServer.__post_init__ to read the optional device
configuration and call _connect_device after starting the server when
configured; leave startup unchanged when no device is set. In
python/packages/jumpstarter-driver-adb/jumpstarter_driver_adb/driver_test.py
lines 86-90, add coverage for configured-device startup expecting the adb
connect subprocess call, while retaining the two-call assertion only for
no-device configuration.
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.
Allow ADB driver to automatically connect to a remote device on startup via the
deviceconfig field, enabling use cases where the exporter is not co-located with the Android device, like with cuttlefish targets