feat(qa): adb interaction tools for QA agents (screenshot/input/logcat)#423
feat(qa): adb interaction tools for QA agents (screenshot/input/logcat)#423OBenner wants to merge 1 commit into
Conversation
Second slice of the device & GUI verification roadmap (track A2) - the Android analog of the Electron MCP toolset: - android_screenshot: adb exec-out screencap, downscaled/JPEG-encoded via Pillow to fit the SDK's 1MB message buffer (PNG passthrough with a clear error fallback when Pillow is missing) - android_input: tap/swipe/text/keyevent via adb shell input; text is rejected when it carries device-shell metacharacters, keyevents are validated against KEYCODE_*/numeric form - android_logcat: recent lines with substring filtering Tools are registered only for Android Gradle projects (com.android in build files), keeping tool context clean elsewhere, and are available to qa_reviewer/qa_fixer. All adb calls use fixed argv (no shell) with timeouts. Pillow added to backend requirements. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
📝 WalkthroughWalkthroughAdds an adb-based Android device wrapper and three MCP tools for screenshots, input, and logcat. The tools register only for Android Gradle projects, are enabled for QA reviewer and fixer agents, and are covered by new tests. ChangesAndroid QA tooling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QAAgent
participant ToolRegistry
participant AndroidHarness
participant AndroidDevice
participant ADB
QAAgent->>ToolRegistry: request tools for project
ToolRegistry->>AndroidHarness: create_android_tools(project_dir)
AndroidHarness->>AndroidDevice: is_android_project(project_dir)
AndroidHarness-->>QAAgent: register Android MCP tools
QAAgent->>AndroidHarness: call screenshot, input, or logcat
AndroidHarness->>AndroidDevice: execute requested operation
AndroidDevice->>ADB: run adb command
ADB-->>AndroidDevice: device output
AndroidDevice-->>QAAgent: image or text result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
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 `@apps/backend/agents/tools_pkg/tools/android_harness.py`:
- Around line 64-67: Update the async handlers around take_screenshot and the
additional device-helper call sites to invoke blocking ADB helpers via await
asyncio.to_thread(...). Preserve their existing arguments, result handling, and
error behavior while ensuring subprocess.run does not execute on the MCP event
loop.
- Around line 157-171: Update the android_logcat handling around read_logcat to
require an app/package scope when no filter is provided, using the existing
package-scope configuration or symbol rather than allowing device-wide logs.
Before _text_result returns successful or error logcat output, redact sensitive
patterns such as identifiers, tokens, and user data; preserve the existing
empty-output behavior and ensure the security considerations are covered.
- Around line 120-128: Update the argument defaults in the swipe and related
line-handling paths to preserve explicit zero values instead of using
truthiness-based fallbacks. Use presence-aware defaulting for duration_ms and
lines so omitted arguments still receive their defaults while 0 reaches
send_swipe and the core line-clamping logic unchanged.
- Around line 52-59: Update the `@tool` schemas for android_screenshot,
android_input, and android_logcat so optional handler arguments are not required
by validation. Replace the dict-based schemas with full JSON Schema or TypedDict
definitions, keeping serial, x2/y2/duration_ms, text, key, and filter_text
optional and marking only fields required for the selected action.
In `@apps/backend/core/android_device.py`:
- Around line 117-129: The _fit_screenshot function currently accepts any JPEG
returned by _compress_with_pillow without validating its encoded size. Enforce
MAX_SCREENSHOT_BASE64_BYTES after compression, retrying with reduced dimensions
or quality until the JPEG fits, and return a clear size error if it cannot;
preserve the existing PNG fallback behavior and add a test covering an oversized
compressed image.
- Around line 33-60: Replace the literal build-file scan in is_android_project
with stack classification through context/project_analyzer.py, using its
established analyzer API to detect Android projects. Ensure version-catalog
plugin aliases and Android modules outside app/ are recognized, while preserving
the boolean result expected by Android registration callers.
- Line 52: Replace the direct Path-based path construction and subprocess.run
calls in the affected Android device operations with the path-joining and
execution utilities from the platform abstraction module. Route these operations
through the required hooks in core/security.py, preserving the existing
fixed-argument behavior and command results while ensuring portability and
centralized policy enforcement.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 546b2b7e-e7e2-4aff-84ea-fbb3dabd84f2
📒 Files selected for processing (7)
apps/backend/agents/tools_pkg/models.pyapps/backend/agents/tools_pkg/registry.pyapps/backend/agents/tools_pkg/tools/__init__.pyapps/backend/agents/tools_pkg/tools/android_harness.pyapps/backend/core/android_device.pyapps/backend/requirements.txttests/test_android_device.py
| @tool( | ||
| "android_screenshot", | ||
| "Take a screenshot of the Android emulator or attached device via " | ||
| "adb. Use it to visually verify UI state after launching the app or " | ||
| "interacting with it. Optionally pass a device serial when several " | ||
| "devices are connected.", | ||
| {"serial": str}, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
pip download --no-deps --no-binary=:all: "claude-agent-sdk==0.2.87" --dest "$tmpdir"
archive="$(fd -t f -e gz . "$tmpdir" | head -n 1)"
tar -xOf "$archive" --wildcards '*/src/claude_agent_sdk/__init__.py' |
rg -n -C3 '"required": list\(properties\.keys\(\)\)'Repository: OBenner/Auto-Coding
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby definitions.
git ls-files 'apps/backend/agents/tools_pkg/tools/android_harness.py' \
'apps/backend/**/claude_agent_sdk*' \
'pyproject.toml' 'uv.lock' 'poetry.lock' 'requirements*.txt' 2>/dev/null || true
echo '--- outline ---'
ast-grep outline apps/backend/agents/tools_pkg/tools/android_harness.py --view expanded || true
echo '--- focused reads ---'
sed -n '1,240p' apps/backend/agents/tools_pkg/tools/android_harness.py
echo '--- sdk schema search ---'
rg -n '"required":|properties\.keys\(\)|`@tool`\(' -S . \
-g '!**/.git/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'Repository: OBenner/Auto-Coding
Length of output: 23954
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tool-schema docs ---'
sed -n '1,220p' docs/architecture/tool-schema-translator.md
echo '--- Android harness surrounding lines ---'
sed -n '40,180p' apps/backend/agents/tools_pkg/tools/android_harness.py
echo '--- compare other `@tool` usages with optional params ---'
sed -n '1,220p' apps/backend/agents/tools_pkg/tools/cli_harness.py
sed -n '1,260p' apps/backend/agents/tools_pkg/tools/debugging.pyRepository: OBenner/Auto-Coding
Length of output: 26494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
path = Path("docs/architecture/tool-schema-translator.md")
text = path.read_text()
for needle in ["required", "optional", "`@tool`", "schema"]:
print(f"=== {needle} ===")
for i, line in enumerate(text.splitlines(), 1):
if needle.lower() in line.lower():
print(f"{i}: {line}")
PYRepository: OBenner/Auto-Coding
Length of output: 7356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tool-schema docs ---'
sed -n '1,240p' docs/architecture/tool-schema-translator.md
echo '--- android harness ---'
sed -n '40,180p' apps/backend/agents/tools_pkg/tools/android_harness.py
echo '--- nearby tool patterns ---'
sed -n '1,220p' apps/backend/agents/tools_pkg/tools/cli_harness.py
sed -n '1,260p' apps/backend/agents/tools_pkg/tools/debugging.pyRepository: OBenner/Auto-Coding
Length of output: 27099
🌐 Web query:
claude-agent-sdk @tool dict schema required properties optional JSON schema tool decorator
💡 Result:
In the Claude Agent SDK for Python, the @tool decorator uses the input_schema parameter to define the tool's arguments [1][2]. You have three ways to define this schema [2]: 1. Dictionary mapping (Simple): You provide a dictionary where keys are parameter names and values are Python types (e.g., {"name": str, "age": int}) [2][3]. In this mode, all parameters are treated as required [4]. 2. TypedDict (Complex): You can provide a TypedDict class for more structured, complex schemas [2]. 3. JSON Schema (Full Control): You can provide a full JSON Schema dictionary [1][2]. This is necessary if you need features like enums, specific ranges, nested objects, or explicit control over required/optional fields [5][4]. For JSON Schema definitions, the standard JSON schema structure applies, where you define your properties under the "properties" key and specify which are required using the "required" array [1][6]. To handle optional parameters in Python when using the simple dictionary mapping: Because the dictionary schema treats every key as required, the recommended pattern for optional parameters is to omit them from the schema entirely, mention them in the tool's description string, and access them using args.get within your handler function [4]. When using the full JSON Schema approach, you simply define your properties and omit the parameter name from the "required" list in the schema definition [1][6]. Top Results: [1][5][2][4]
Citations:
- 1: https://code.claude.com/docs/en/agent-sdk/python
- 2: https://anthropics-claude-agent-sdk-python-82.mintlify.app/api/tool
- 3: https://github.com/anthropics/claude-agent-sdk-python?tab=readme-ov-file
- 4: https://code.claude.com/docs/en/agent-sdk/custom-tools.md
- 5: https://code.claude.com/docs/en/agent-sdk/custom-tools
- 6: https://platform.claude.com/docs/en/agents-and-tools/tool-use/define-tools
Make the optional tool args optional. android_screenshot, android_input, and android_logcat currently use the dict-based @tool schema, which makes every listed property required. That forces serial, x2/y2/duration_ms, text, key, and filter_text to be passed even though the handlers already treat them as optional. Switch to full JSON Schema (or a TypedDict) and mark only the action-specific fields as required.
🤖 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 `@apps/backend/agents/tools_pkg/tools/android_harness.py` around lines 52 - 59,
Update the `@tool` schemas for android_screenshot, android_input, and
android_logcat so optional handler arguments are not required by validation.
Replace the dict-based schemas with full JSON Schema or TypedDict definitions,
keeping serial, x2/y2/duration_ms, text, key, and filter_text optional and
marking only fields required for the selected action.
| try: | ||
| image, mime_or_error = take_screenshot( | ||
| serial=(args.get("serial") or "").strip() or None | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Offload blocking ADB calls from the MCP event loop.
Each async handler synchronously waits in subprocess.run for up to 30 seconds, blocking other MCP work and cancellation. Invoke the device helper through await asyncio.to_thread(...).
As per path instructions, focus on proper “async patterns.”
Also applies to: 118-133, 157-162
🤖 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 `@apps/backend/agents/tools_pkg/tools/android_harness.py` around lines 64 - 67,
Update the async handlers around take_screenshot and the additional
device-helper call sites to invoke blocking ADB helpers via await
asyncio.to_thread(...). Preserve their existing arguments, result handling, and
error behavior while ensuring subprocess.run does not execute on the MCP event
loop.
Source: Path instructions
| result = send_tap(args.get("x") or 0, args.get("y") or 0, serial) | ||
| elif action == "swipe": | ||
| result = send_swipe( | ||
| args.get("x") or 0, | ||
| args.get("y") or 0, | ||
| args.get("x2") or 0, | ||
| args.get("y2") or 0, | ||
| args.get("duration_ms") or 300, | ||
| serial, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve explicit zero-valued inputs.
or converts duration_ms=0 to 300 and lines=0 to 200, bypassing the core layer’s intended line clamping.
Proposed fix
- args.get("duration_ms") or 300,
+ args.get("duration_ms", 300),
...
- lines=args.get("lines") or 200,
+ lines=args.get("lines", 200),Also applies to: 159-159
🤖 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 `@apps/backend/agents/tools_pkg/tools/android_harness.py` around lines 120 -
128, Update the argument defaults in the swipe and related line-handling paths
to preserve explicit zero values instead of using truthiness-based fallbacks.
Use presence-aware defaulting for duration_ms and lines so omitted arguments
still receive their defaults while 0 reaches send_swipe and the core
line-clamping logic unchanged.
| try: | ||
| result = read_logcat( | ||
| lines=args.get("lines") or 200, | ||
| filter_text=(args.get("filter_text") or "").strip() or None, | ||
| serial=(args.get("serial") or "").strip() or None, | ||
| ) | ||
| except Exception as e: | ||
| logging.exception("Error during android_logcat") | ||
| return _text_result(f"Error reading logcat: {e}") | ||
|
|
||
| if not result.ok: | ||
| return _text_result(f"logcat failed: {result.output}") | ||
|
|
||
| output = result.output[-LOGCAT_TAIL_CHARS:] | ||
| return _text_result(output if output.strip() else "(no matching log lines)") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not return unscoped device logs to the agent.
With no filter, this forwards raw device-wide logcat output into the agent context. Attached-device logs may contain identifiers, tokens, or other user data. Require an app/package scope by default and redact sensitive patterns before returning results.
As per path instructions, check for proper “security considerations.”
🤖 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 `@apps/backend/agents/tools_pkg/tools/android_harness.py` around lines 157 -
171, Update the android_logcat handling around read_logcat to require an
app/package scope when no filter is provided, using the existing package-scope
configuration or symbol rather than allowing device-wide logs. Before
_text_result returns successful or error logcat output, redact sensitive
patterns such as identifiers, tokens, and user data; preserve the existing
empty-output behavior and ensure the security considerations are covered.
Source: Path instructions
| _ANDROID_BUILD_FILES = ( | ||
| "build.gradle", | ||
| "build.gradle.kts", | ||
| "app/build.gradle", | ||
| "app/build.gradle.kts", | ||
| ) | ||
|
|
||
|
|
||
| @dataclass | ||
| class AdbResult: | ||
| """Outcome of an adb invocation.""" | ||
|
|
||
| ok: bool | ||
| output: str | ||
|
|
||
|
|
||
| def is_android_project(project_dir: Path) -> bool: | ||
| """Check if the project is an Android Gradle project.""" | ||
| for build_file in _ANDROID_BUILD_FILES: | ||
| path = Path(project_dir) / build_file | ||
| if not path.exists(): | ||
| continue | ||
| try: | ||
| if "com.android" in path.read_text(encoding="utf-8"): | ||
| return True | ||
| except (OSError, UnicodeDecodeError): | ||
| continue | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use the stack analyzer for Android registration.
This literal scan misses valid Android Gradle layouts using version-catalog plugin aliases or Android modules outside app/, so the QA tools are never registered. Delegate classification to context/project_analyzer.py and cover those layouts.
As per coding guidelines, “Dynamically detect project stack using context/project_analyzer.py to determine allowed commands.”
🤖 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 `@apps/backend/core/android_device.py` around lines 33 - 60, Replace the
literal build-file scan in is_android_project with stack classification through
context/project_analyzer.py, using its established analyzer API to detect
Android projects. Ensure version-catalog plugin aliases and Android modules
outside app/ are recognized, while preserving the boolean result expected by
Android registration callers.
Source: Coding guidelines
| def is_android_project(project_dir: Path) -> bool: | ||
| """Check if the project is an Android Gradle project.""" | ||
| for build_file in _ANDROID_BUILD_FILES: | ||
| path = Path(project_dir) / build_file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Route path and process operations through the platform abstractions.
These direct Path and subprocess.run calls bypass the centralized execution and security hooks. The fixed argv means this is not a host-shell injection path, but it still evades the required policy and portability layer.
As per coding guidelines, use “path joining utilities and execution utilities from the platform abstraction module” and “use core/security.py for security hooks.”
Also applies to: 74-80, 99-104
🤖 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 `@apps/backend/core/android_device.py` at line 52, Replace the direct
Path-based path construction and subprocess.run calls in the affected Android
device operations with the path-joining and execution utilities from the
platform abstraction module. Route these operations through the required hooks
in core/security.py, preserving the existing fixed-argument behavior and command
results while ensuring portability and centralized policy enforcement.
Sources: Coding guidelines, Linters/SAST tools
| def _fit_screenshot(png_bytes: bytes) -> tuple[bytes | None, str]: | ||
| """Compress the screenshot to fit the SDK message limit.""" | ||
| compressed = _compress_with_pillow(png_bytes) | ||
| if compressed is not None: | ||
| return compressed, "image/jpeg" | ||
|
|
||
| # Pillow unavailable: pass the PNG through if it is small enough | ||
| if len(base64.b64encode(png_bytes)) <= MAX_SCREENSHOT_BASE64_BYTES: | ||
| return png_bytes, "image/png" | ||
| return None, ( | ||
| "Screenshot too large for the SDK message limit and Pillow is not " | ||
| "installed - install Pillow to enable compression" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce the size cap after JPEG compression.
A fixed width and JPEG quality do not guarantee the encoded JPEG fits the cap; tall or noisy screenshots can still exceed it and cause the MCP result to exceed the SDK limit. Retry with lower dimensions/quality until it fits, or return a clear size error. Add an oversized-compressed-image test.
🤖 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 `@apps/backend/core/android_device.py` around lines 117 - 129, The
_fit_screenshot function currently accepts any JPEG returned by
_compress_with_pillow without validating its encoded size. Enforce
MAX_SCREENSHOT_BASE64_BYTES after compression, retrying with reduced dimensions
or quality until the JPEG fits, and return a clear size error if it cannot;
preserve the existing PNG fallback behavior and add a test covering an oversized
compressed image.
1 participant
Summary
Slice A2 of the Device & GUI Verification Roadmap (follows #422): the Android analog of the Electron MCP toolset, lifting Android verification from L2 to L3/L4 — the QA agent gets the same screenshot → inspect → interact → verify loop it already has for Electron apps.
android_screenshotadb exec-out screencap -p, downscaled to 1280px / JPEG q60 via Pillow to fit the SDK's 1MB message buffer (documented Electron MCP precedent); PNG passthrough + clear error when Pillow is absentandroid_inputtap/swipe/text/keyeventviaadb shell input; optionalserialfor multi-device setupsandroid_logcatread_electron_logs)Security/robustness:
com.androidin build files) — zero tool-context pollution for everything else; verified live viacreate_all_tools()input textis evaluated by the device shell, so shell metacharacters (;|&$quotes, parentheses) are rejected; keyevents validated againstKEYCODE_*/numeric formcore/android_device.py(pure, tested) + thin MCP wrappers inagents/tools_pkg/tools/android_harness.py— same split asrun_cli_session(feat(qa): CLI E2E harness tool for terminal applications #415)Pillow>=10.0.0added to backend requirementsTesting
tests/test_android_device.py: 21 tests (detection, argv construction, serial routing, missing adb/device, text/keyevent sanitization incl. injection attempts, logcat filtering and clamping, screenshot size handling with and without Pillow, registration gating, QA permissions).tests/test_agent_configs.py: 25 passed. Ruff check + format clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests