Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added adb_vision/__init__.py
Empty file.
70 changes: 70 additions & 0 deletions adb_vision/screenshot.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import asyncio
import base64
import logging
import urllib.request
from typing import Callable, Awaitable

# Type alias for the async ADB runner
AdbRunFn = Callable[..., Awaitable[bytes]]

async def _capture_droidcast(*, adb_run: AdbRunFn, serial: str, adb_exe: str) -> str:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: You accept serial and adb_exe parameters like you're running a customizable operation, but then completely ignore them in favor of... whatever adb_run closure was passed in. These parameters are about as useful as a screen door on a submarine — they look like they do something, but they're just for show.

🩹 The Fix: Either use these parameters to construct the ADB command (like the MCP server does with ADB_EXECUTABLE and ADB_SERIAL), or remove them from the signature if the caller is expected to handle device targeting via adb_run.

📏 Severity: warning

"""Return base64-encoded PNG screenshot via DroidCast HTTP stream."""
port = "53516"
apk_path = "alas_wrapped/bin/DroidCast/DroidCast-debug-1.2.3.apk"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: Hardcoding a relative path to the APK? Bold strategy. This will work great until someone runs the code from literally any other directory. Then it's a game of 'Where's Waldo?' but with APK files and FileNotFound errors.

🩹 The Fix: Use Path(__file__).parent.parent / "alas_wrapped/bin/DroidCast/DroidCast-debug-1.2.3.apk" to make this path resolution independent of the working directory.

📏 Severity: warning


Comment on lines +12 to +14
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apk_path points to alas_wrapped/bin/DroidCast/DroidCast-debug-1.2.3.apk, but the repo only ships DroidCast_raw-release-1.0.apk under alas_wrapped/bin/DroidCast/. With the current path, install will always fail and first-time setup can’t work; update this to the actual APK path (and consider resolving it via Path(__file__) to avoid cwd-dependent failures).

Copilot uses AI. Check for mistakes.
# 1. Idempotency check: if port forward already exists, try to fetch first
try:
forward_list = await adb_run("forward", "--list", timeout=5.0)
if f"tcp:{port}" in forward_list.decode():
try:
return await _fetch_screenshot(port)
except Exception:
# Port forward exists but service might not be responding, continue to start
pass
except Exception:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: Ah yes, the classic except Exception: pass — the debugging equivalent of sweeping dirt under the rug. When this fails in production, you'll have absolutely no idea why. I've written this exact bug, and I'm still ashamed.

🩹 The Fix: At minimum, log the exception with logging.debug():

except Exception as e:
    logging.debug(f"Forward list check failed: {e}")

📏 Severity: warning

pass

# 2. Push/Install APK if needed
# (In a real scenario we'd check if installed, but following the plan's direct install -r)
try:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: You attempt to install the APK without first checking if the file even exists. That's like ordering pizza before checking if you have money — technically optimistic, but practically setting yourself up for disappointment.

🩹 The Fix: Add a pre-flight check:

apk_path_obj = Path(apk_path)
if not apk_path_obj.exists():
    raise FileNotFoundError(f"DroidCast APK not found: {apk_path}")

📏 Severity: suggestion

await adb_run("install", "-r", apk_path, timeout=30.0)
except Exception as e:
# If the file is missing in the environment, we log it but might proceed if it's already installed
logging.warning(f"DroidCast install failed: {e}")

Comment on lines +27 to +34
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The install step swallows all exceptions and continues, which can lead to confusing downstream failures (e.g., start/HTTP retries) when the APK is genuinely missing or install fails for other reasons. Prefer failing fast when the APK path is not present locally, or explicitly detect “already installed” and only ignore that specific case.

Copilot uses AI. Check for mistakes.
# 3. Start DroidCast service
await adb_run(
"shell", "am", "start-foreground-service",
"-n", "com.rayworks.droidcast/.Main",
"-a", "android.intent.action.MAIN",
timeout=10.0
)
Comment on lines +35 to +41
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This starts DroidCast via am start-foreground-service -n com.rayworks.droidcast/.Main, but the DroidCast_raw APK shipped in this repo is typically launched via app_process with CLASSPATH=/data/local/tmp/DroidCast_raw.apk (see ALAS’s existing DroidCast implementation). As-is, the service start is likely to fail or start the wrong component; align the start mechanism/component with the APK you install/push.

Copilot uses AI. Check for mistakes.

# 4. Forward the port
await adb_run("forward", f"tcp:{port}", f"tcp:{port}", timeout=5.0)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: You forward the port but never clean it up. This is the networking equivalent of leaving camping chairs at a festival — eventually you'll run out of ports and wonder why nothing works anymore.

🩹 The Fix: Add a cleanup step or document that callers must manage port lifecycle. Consider using contextlib.asynccontextmanager to ensure adb forward --remove tcp:{port} runs on exit.

📏 Severity: warning


# 5. Fetch and return
# Allow a small retry loop for the service to bind the port
for i in range(5):
try:
return await _fetch_screenshot(port)
except Exception:
if i == 4:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: Another except Exception — you're really committed to this 'silence all errors' aesthetic. When the 5th retry fails, you'll raise a generic exception with zero context about which of the 5 attempts actually happened or what went wrong.

🩹 The Fix: Capture and log the last exception before raising:

except Exception as last_err:
    if i == 4:
        raise RuntimeError(f"Failed to fetch screenshot after 5 retries: {last_err}") from last_err

📏 Severity: suggestion

raise
await asyncio.sleep(0.5)


async def _fetch_screenshot(port: str) -> str:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: Wait, the function returns just a string? I thought we established a Tool Contract in ROADMAP.md that requires {success, data, error, observed_state, expected_state}. This is like showing up to a black-tie event in sweatpants — technically you're dressed, but you're not meeting the dress code.

🩹 The Fix: Return the standard tool envelope as documented in docs/ROADMAP.md lines 36-42:

return {
    "success": True,
    "data": {"screenshot_b64": base64.b64encode(data).decode("ascii")},
    "error": None,
    "observed_state": "screenshot_captured",
    "expected_state": "screenshot_captured"
}

📏 Severity: critical

"""Internal helper to GET screenshot from localhost:port."""
url = f"http://localhost:{port}/screenshot?format=png"
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch URL uses /screenshot?format=png, but for the DroidCast_raw server the PNG endpoint is typically /preview (and /screenshot returns a raw RGB565 bitmap, not PNG). Using the wrong route will return 404 even when the server is healthy; update the endpoint to match the shipped DroidCast variant (or make it selectable).

Suggested change
url = f"http://localhost:{port}/screenshot?format=png"
# DroidCast_raw serves PNG frames on /preview; /screenshot returns raw RGB565.
url = f"http://localhost:{port}/preview"

Copilot uses AI. Check for mistakes.

def _get():
# Using urllib.request to avoid extra dependencies like aiohttp
with urllib.request.urlopen(url, timeout=5.0) as resp:
if resp.status != 200:
raise RuntimeError(f"DroidCast HTTP error: {resp.status}")
return resp.read()
Comment on lines +61 to +66
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urllib.request.urlopen() raises urllib.error.HTTPError for non-2xx responses, so resp.status != 200 will never run for 404/500 in real usage. Catch HTTPError (and possibly URLError) inside _get() and convert it into the intended RuntimeError so callers/tests get consistent behavior.

Copilot uses AI. Check for mistakes.

loop = asyncio.get_running_loop()
data = await loop.run_in_executor(None, _get)
return base64.b64encode(data).decode("ascii")
98 changes: 98 additions & 0 deletions adb_vision/test_droidcast.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import asyncio
import base64
import io
Comment on lines +1 to +3
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asyncio and io are imported but unused in this test module. If the repo runs an import/unused check (or if you add one later), this will fail; remove unused imports to keep the test file clean.

Suggested change
import asyncio
import base64
import io
import base64

Copilot uses AI. Check for mistakes.
import pytest
from unittest import mock
from adb_vision.screenshot import _capture_droidcast
Comment on lines +1 to +6
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests won’t be collected by the repo’s default pytest run because root pytest.ini restricts testpaths to agent_orchestrator/. If adb_vision tests are meant to run in CI, you’ll need to expand testpaths (or add a separate test invocation / move these tests under the collected suite).

Copilot uses AI. Check for mistakes.

def _fake_png() -> bytes:
"""Return a minimal valid PNG (1x1 white square)."""
return (
b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00"
b"\x01\x08\x02\x00\x00\x00\x90wS\xde\x00\x00\x00\x0cIDATx\x9cc\xf8"
b"\xff\xff\x3f\x00\x05\xfe\x02\xfe\x05\x1e\x35\xaf\x00\x00\x00\x00IEND\xaeB`\x82"
)

@pytest.mark.asyncio
async def test_capture_droidcast_happy_path():
"""Verify happy path: install -> start -> forward -> fetch."""
adb_run = mock.AsyncMock()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: Setting both return_value AND side_effect on the same mock? That's like setting your GPS destination AND telling it to take random turns. Once the side_effect list is exhausted, it'll fall back to return_value instead of raising StopIteration. Your test might pass by accident.

🩹 The Fix: Remove adb_run.return_value = b"" — the side_effect list already defines all expected return values.

📏 Severity: suggestion

adb_run.return_value = b"" # Default for all calls

# Mocking --list to return empty initially
adb_run.side_effect = [
b"", # forward --list
b"", # install
b"", # am start-foreground-service
b"", # forward tcp:53516 tcp:53516
]

png_data = _fake_png()

# Mock urllib.request.urlopen
with mock.patch("urllib.request.urlopen") as mock_url:
mock_resp = mock.MagicMock()
mock_resp.status = 200
mock_resp.read.return_value = png_data
mock_resp.__enter__.return_value = mock_resp
mock_url.return_value = mock_resp

result = await _capture_droidcast(adb_run=adb_run, serial="emulator-5554", adb_exe="adb")

assert result == base64.b64encode(png_data).decode("ascii")

# Verify calls
# Call 0: forward --list
# Call 1: install -r
# Call 2: am start-foreground-service
# Call 3: forward tcp:53516 tcp:53516
assert adb_run.call_count == 4
adb_run.assert_any_call("install", "-r", mock.ANY, timeout=30.0)
adb_run.assert_any_call("shell", "am", "start-foreground-service", "-n", "com.rayworks.droidcast/.Main", "-a", "android.intent.action.MAIN", timeout=10.0)
adb_run.assert_any_call("forward", "tcp:53516", "tcp:53516", timeout=5.0)

@pytest.mark.asyncio
async def test_capture_droidcast_idempotent():
"""Verify it skips install/start if port is already forwarded and responsive."""
adb_run = mock.AsyncMock()
adb_run.return_value = b"tcp:53516 tcp:53516" # forward --list says it exists

png_data = _fake_png()

with mock.patch("urllib.request.urlopen") as mock_url:
mock_resp = mock.MagicMock()
mock_resp.status = 200
mock_resp.read.return_value = png_data
mock_resp.__enter__.return_value = mock_resp
mock_url.return_value = mock_resp

result = await _capture_droidcast(adb_run=adb_run, serial="emulator-5554", adb_exe="adb")

assert result == base64.b64encode(png_data).decode("ascii")

# Should only have called forward --list
assert adb_run.call_count == 1
adb_run.assert_called_once_with("forward", "--list", timeout=5.0)

@pytest.mark.asyncio
async def test_capture_droidcast_http_failure():
"""Verify it raises RuntimeError on HTTP failure."""
adb_run = mock.AsyncMock()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: Same issue here — you set return_value = b"" but then immediately override the behavior with side_effect. Pick a lane! The mock is having an identity crisis.

🩹 The Fix: Remove line 81 (adb_run.return_value = b"") since you're using side_effect.

📏 Severity: nitpick

adb_run.return_value = b""

# Mocking --list to return empty initially
adb_run.side_effect = [
b"", # forward --list
b"", # install
b"", # am start-foreground-service
b"", # forward tcp:53516 tcp:53516
]

with mock.patch("urllib.request.urlopen") as mock_url:
mock_resp = mock.MagicMock()
mock_resp.status = 404 # Failure
mock_resp.__enter__.return_value = mock_resp
mock_url.return_value = mock_resp

with pytest.raises(RuntimeError, match="DroidCast HTTP error: 404"):
await _capture_droidcast(adb_run=adb_run, serial="emulator-5554", adb_exe="adb")
Comment on lines +91 to +98
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTP failure test mocks urlopen() returning a response with status=404, but real urllib.request.urlopen() raises urllib.error.HTTPError on 404 (no response object is returned). Update the test to reflect urllib’s actual behavior (and update _fetch_screenshot accordingly) so this failure mode is exercised realistically.

Copilot uses AI. Check for mistakes.