fix: decode shell skill output as UTF-8#6288
Open
VectorPeak wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
RunSkillScriptToolbuilds a generated Python wrapper when executing shell-based skill scripts. In the shell script path, that wrapper currently calls:without passing an explicit
encodingorerrorspolicy.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
bashor a shell script can fail whilesubprocessis 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
warningorerrorinstead observedsuccess.Changes
This PR makes the generated shell wrapper decode captured stdout/stderr with an explicit UTF-8 contract:
The change is intentionally narrow:
RunSkillScriptToolcapture_output=Trueandtext=Truebehaviorerrors="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:
and:
The 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
RunSkillScriptToolshell execution validation also passed throughUnsafeLocalCodeExecutorfor:The full
tests/unittestssuite 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, artifactfile://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:When 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:
encoding="utf-8"to the generatedsubprocess.run(...)errors="replace"so malformed bytes do not prevent the JSON result envelope from being emittedTesting 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:
Manual End-to-End (E2E) Tests:
Manually validated the real
RunSkillScriptToolshell execution path withUnsafeLocalCodeExecutorusing shell scripts that produce stdout, stderr, and non-zero exit status.Result summary:
Checklist
Additional context
Python's
subprocess.run()supports explicitencodinganderrorsparameters 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.