| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Hi @VectorPeak , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share. |
Sorry, something went wrong.
|
Hi @wyf7107 , can you please review this. LGTM |
Sorry, something went wrong.
Merge #6288 ## Summary - Decode shell skill output as UTF-8 to keep it independent from host locale. - Preserve shell exit code with stderr output. Closes #6289 ## Testing - Activated virtual environment in open_source_workspace. - Ran `pytest tests/unittests/tools/test_skill_toolset.py`. - 113 tests passed. Co-authored-by: Kathy Wu <wukathy@google.com> PiperOrigin-RevId: 948464268
|
Thank you @VectorPeak for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit b7ad76a. Closing this PR as the changes are now in the main branch. |
Sorry, something went wrong.
Count google/adk-python#6288 as a closed-but-landed upstream PR.
Merge google#6288 ## Summary - Decode shell skill output as UTF-8 to keep it independent from host locale. - Preserve shell exit code with stderr output. Closes google#6289 ## Testing - Activated virtual environment in open_source_workspace. - Ran `pytest tests/unittests/tools/test_skill_toolset.py`. - 113 tests passed. Co-authored-by: Kathy Wu <wukathy@google.com> PiperOrigin-RevId: 948464268
| Back | FazBrowse Home | New Git URL |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
What Problem This Solves
RunSkillScriptTool builds a generated Python wrapper when executing shell-based skill scripts. In the shell script path, that wrapper currently calls:
without passing an explicit encoding or errors policy.
Because the wrapper requests text-mode captured output, Python decodes the child process stdout and stderr before the wrapper prints the JSON shell-result envelope back to ADK. When no explicit encoding is supplied, that decode path can depend on the platform default text encoding. On Windows environments using a non-UTF-8 default encoding such as cp936/gbk, UTF-8 bytes emitted by bash or a shell script can fail while subprocess is reading the captured stream.
In the local Windows full unit-test log, this appeared as a reader-thread failure inside Python's subprocess implementation:
That decode failure caused the generated wrapper to lose the captured stderr path. As a result, existing shell integration tests that expected stderr to map to warning or error instead observed success.
Changes
This PR makes the generated shell wrapper decode captured stdout/stderr with an explicit UTF-8 contract:
The change is intentionally narrow:
errors="replace" prevents a single undecodable byte from making the wrapper fail before it can return stdout, stderr, and the command return code to the caller.
Evidence
The previous local Windows full unit-test run contained two focused failures in the shell skill path:
test_integration_shell_stdout_and_stderr AssertionError: assert 'success' == 'warning' Exception in thread Thread-2 (_readerthread): File "...\\Lib\\subprocess.py", line 1599, in _readerthread buffer.append(fh.read()) UnicodeDecodeError: 'gbk' codec can't decode byte 0xff in position 47: illegal multibyte sequenceand:
test_integration_shell_stderr_only AssertionError: assert 'success' == 'error' Exception in thread Thread-2 (_readerthread): File "...\\Lib\\subprocess.py", line 1599, in _readerthread buffer.append(fh.read()) UnicodeDecodeError: 'gbk' codec can't decode byte 0xff in position 47: illegal multibyte sequenceThe PR also updates the generated-wrapper unit coverage to assert that shell wrappers include explicit UTF-8 decoding and replacement error handling:
Focused local validation passed on Windows:
Result:
Focused WSL/Linux validation also passed for the PR-relevant shell wrapper tests:
/home/zxy/.local/bin/uv run --extra test pytest tests/unittests/tools/test_skill_toolset.py::test_execute_script_shell_success tests/unittests/tools/test_skill_toolset.py::test_integration_shell_stdout_and_stderr tests/unittests/tools/test_skill_toolset.py::test_integration_shell_stderr_only tests/unittests/tools/test_skill_toolset.py::test_integration_shell_nonzero_exit -qResult:
Manual RunSkillScriptTool shell execution validation also passed through UnsafeLocalCodeExecutor for:
The full tests/unittests suite is not claimed as passing here. A full local Windows run still had unrelated failures outside this PR:
Those failures include existing Windows path/temporary-directory cleanup, load_web_page, artifact file:// URI, edit-file newline, telemetry, sample, and evaluation-manager failures unrelated to this shell wrapper change.
Possible call chain / impact
The affected path is shell skill execution through RunSkillScriptTool:
RunSkillScriptTool.run_async() -> _SkillScriptCodeExecutor.execute_script_async() -> generated Python wrapper -> shell script branch for .sh / .bash files -> subprocess.run(..., capture_output=True, text=True) -> JSON shell result envelope -> RunSkillScriptTool parses stdout/stderr/returncode -> result status becomes success / warning / errorWhen stderr and return codes are preserved correctly, the existing status mapping can work as intended:
Without explicit decoding, a Windows locale mismatch can make the subprocess reader fail before the wrapper returns the captured stderr content. That can hide stderr from the ADK tool result and make shell script failures or warnings look like successful executions.
This PR does not attempt to preserve arbitrary legacy-encoded shell output byte-for-byte. If a shell script intentionally emits non-UTF-8 bytes, invalid byte sequences are represented with replacement characters so the wrapper can still return the shell result envelope instead of dropping the result.
Solution:
Decode generated shell wrapper output explicitly as UTF-8 and tolerate undecodable bytes with replacement characters:
Testing Plan
Unit Tests:
Focused unit tests passed locally on Windows:
Result:
Focused WSL/Linux validation also passed for the PR-relevant shell wrapper tests:
/home/zxy/.local/bin/uv run --extra test pytest tests/unittests/tools/test_skill_toolset.py::test_execute_script_shell_success tests/unittests/tools/test_skill_toolset.py::test_integration_shell_stdout_and_stderr tests/unittests/tools/test_skill_toolset.py::test_integration_shell_stderr_only tests/unittests/tools/test_skill_toolset.py::test_integration_shell_nonzero_exit -qResult:
Full unit suites were attempted in both Windows and WSL/Linux environments, but this PR does not claim the full suite as passing because both runs still have failures outside this change.
Windows full unit suite:
Result:
WSL/Linux full unit suite:
/home/zxy/.local/bin/uv run --extra test pytest tests/unittests -qResult:
The remaining WSL/Linux failure is stable when rerun by itself and is outside this PR's touched area:
tests/unittests/workflow/test_workflow_parallel_worker.py::test_parallel_worker_failure_propagates_and_cancels_others AssertionError: tracker included {'task-3': True} when the test expected only {'task-1': True}Manual End-to-End (E2E) Tests:
Manually validated the real RunSkillScriptTool shell execution path with UnsafeLocalCodeExecutor using shell scripts that produce stdout, stderr, and non-zero exit status.
Result summary:
Checklist
Additional context
Python's subprocess.run() supports explicit encoding and errors parameters for text-mode captured streams. This PR uses those parameters so generated shell wrappers do not depend on the host locale when decoding captured stdout and stderr.
No dependent downstream changes are required for this PR.