fix: decode local python output from bytes#7702
fix: decode local python output from bytes#7702bugkeep wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- _normalize_python_output unconditionally converts lone '\r' characters to '\n', which changes behavior for outputs that intentionally use carriage returns (e.g., progress bars); consider limiting normalization to '\r\n' or documenting this change in behavior.
- Since both stdout and stderr now share the same decode-and-normalize path, consider reusing or generalizing _normalize_python_output/_decode_shell_output so shell and Python components share a single, consistent normalization function rather than splitting this between two helpers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- _normalize_python_output unconditionally converts lone '\r' characters to '\n', which changes behavior for outputs that intentionally use carriage returns (e.g., progress bars); consider limiting normalization to '\r\n' or documenting this change in behavior.
- Since both stdout and stderr now share the same decode-and-normalize path, consider reusing or generalizing _normalize_python_output/_decode_shell_output so shell and Python components share a single, consistent normalization function rather than splitting this between two helpers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request updates the local Python execution logic to handle output decoding and normalization more robustly by disabling automatic text mode in subprocess calls and applying a custom normalization function. A new test case is added to verify the handling of non-UTF-8 (GBK) output. Review feedback recommends always capturing stderr to include diagnostic warnings even when the process succeeds and suggests mocking the operating system name in tests to ensure consistent behavior across different environments.
| stderr = ( | ||
| _normalize_python_output(_decode_shell_output(result.stderr)) | ||
| if result.returncode != 0 | ||
| else "" | ||
| ) |
There was a problem hiding this comment.
Currently, stderr is only captured and processed if the return code is non-zero. This differs from the behavior in LocalShellComponent.exec, which always returns the decoded stderr. Capturing stderr even on success (return code 0) is beneficial for surfacing warnings or diagnostic information that doesn't necessarily trigger a process failure. Since _decode_shell_output handles empty input gracefully, you can simplify this logic to always process result.stderr.
stderr = _normalize_python_output(_decode_shell_output(result.stderr))| with patch("astrbot.core.computer.booters.local.subprocess.run", fake_run): | ||
| result = await python.exec("print('中文输出')") |
There was a problem hiding this comment.
The new test test_exec_decodes_non_utf8_stdout_with_fallback uses GBK encoding for the mocked stdout. However, the _decode_bytes_with_fallback function only explicitly attempts GBK/CP936 decoding when os.name == 'nt'. On non-Windows environments (like many Linux-based CI systems), this test might fail if the default locale is UTF-8, as it will fall back to replacement characters. To ensure the test is platform-independent and correctly exercises the fallback logic, consider patching os.name to 'nt' within the test context.
| with patch("astrbot.core.computer.booters.local.subprocess.run", fake_run): | |
| result = await python.exec("print('中文输出')") | |
| with patch("astrbot.core.computer.booters.local.subprocess.run", fake_run), \ | |
| patch("astrbot.core.computer.booters.local.os.name", "nt"): | |
| result = await python.exec("print('中文输出')") |
Summary
Verification
Fixes #7695.
Summary by Sourcery
Decode LocalPythonComponent subprocess output from bytes while preserving previous text behavior and ensure non-UTF-8 stdout is handled via the existing fallback decoder.
Bug Fixes:
Tests: