diff --git a/phone_agent/adb/screenshot.py b/phone_agent/adb/screenshot.py index bdc5b092..e74c1bde 100644 --- a/phone_agent/adb/screenshot.py +++ b/phone_agent/adb/screenshot.py @@ -41,24 +41,25 @@ def get_screenshot(device_id: str | None = None, timeout: int = 10) -> Screensho adb_prefix = _get_adb_prefix(device_id) try: - # Execute screenshot command + # Execute screenshot command (use binary mode to avoid encoding issues) result = subprocess.run( adb_prefix + ["shell", "screencap", "-p", "/sdcard/tmp.png"], capture_output=True, - text=True, + text=False, timeout=timeout, ) # Check for screenshot failure (sensitive screen) - output = result.stdout + result.stderr + # Decode with error handling for binary output + output = (result.stdout + result.stderr).decode("utf-8", errors="replace") if "Status: -1" in output or "Failed" in output: return _create_fallback_screenshot(is_sensitive=True) - # Pull screenshot to local temp path + # Pull screenshot to local temp path (use binary mode for file transfer) subprocess.run( adb_prefix + ["pull", "/sdcard/tmp.png", temp_path], capture_output=True, - text=True, + text=False, timeout=5, ) diff --git a/tests/test_screenshot_encoding.py b/tests/test_screenshot_encoding.py new file mode 100644 index 00000000..0b7284c7 --- /dev/null +++ b/tests/test_screenshot_encoding.py @@ -0,0 +1,73 @@ +"""Tests for screenshot.py encoding handling. + +This test verifies that subprocess.run uses binary mode (text=False) to avoid +UTF-8 encoding errors when handling screenshot operations (Issue #224). +""" + +import os +import re +import unittest + + +class TestScreenshotEncodingFix(unittest.TestCase): + """Test that screenshot.py uses binary mode for subprocess calls.""" + + def test_screenshot_uses_binary_mode_for_screencap(self): + """Verify screencap command uses text=False.""" + screenshot_py_path = os.path.join( + os.path.dirname(os.path.dirname(__file__)), + 'phone_agent', 'adb', 'screenshot.py' + ) + + with open(screenshot_py_path, 'r', encoding='utf-8') as f: + content = f.read() + + # Find the screencap subprocess.run call + screencap_pattern = r'subprocess\.run\([^)]*screencap[^)]*\)' + match = re.search(screencap_pattern, content, re.DOTALL) + self.assertIsNotNone(match, "Could not find subprocess.run with screencap") + + subprocess_call = match.group(0) + + # Verify text=False is used (binary mode) + self.assertIn('text=False', subprocess_call, + "screencap should use text=False for binary mode") + + def test_screenshot_uses_binary_mode_for_pull(self): + """Verify pull command uses text=False.""" + screenshot_py_path = os.path.join( + os.path.dirname(os.path.dirname(__file__)), + 'phone_agent', 'adb', 'screenshot.py' + ) + + with open(screenshot_py_path, 'r', encoding='utf-8') as f: + content = f.read() + + # Find the pull subprocess.run call + pull_pattern = r'subprocess\.run\([^)]*"pull"[^)]*\)' + match = re.search(pull_pattern, content, re.DOTALL) + self.assertIsNotNone(match, "Could not find subprocess.run with pull") + + subprocess_call = match.group(0) + + # Verify text=False is used (binary mode) + self.assertIn('text=False', subprocess_call, + "pull should use text=False for binary mode") + + def test_screenshot_decodes_output_with_error_handling(self): + """Verify output is decoded with errors='replace'.""" + screenshot_py_path = os.path.join( + os.path.dirname(os.path.dirname(__file__)), + 'phone_agent', 'adb', 'screenshot.py' + ) + + with open(screenshot_py_path, 'r', encoding='utf-8') as f: + content = f.read() + + # Check for proper decoding with error handling + self.assertIn('decode("utf-8", errors="replace")', content, + "Output should be decoded with errors='replace' to handle surrogates") + + +if __name__ == '__main__': + unittest.main()