From db66119676fa4d5908266913d4c7597b8cbef3d0 Mon Sep 17 00:00:00 2001 From: jinglun010-cpu Date: Sun, 19 Jul 2026 22:18:49 +0800 Subject: [PATCH] fix: extend UTF-8 encoding to _op_version probe (#53428) Hermes-sweeper review on #60741 flagged that _op_version (the paired op probe used by the same setup/status CLI flow at lines 127 and 205) still ran text=True without explicit encoding/errors. Add encoding='utf-8', errors='replace' to match _op_whoami and the production op read path at agent/secret_sources/onepassword.py:271-278. Also extend the regression test to cover _op_version alongside _op_whoami, and update the module docstring to reflect the widened scope. Test sensitivity verified: reverting the source change makes test_op_version_passes_utf8_encoding fail with encoding=None. --- hermes_cli/onepassword_secrets_cli.py | 2 ++ tests/tools/test_subprocess_utf8_encoding.py | 22 +++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/hermes_cli/onepassword_secrets_cli.py b/hermes_cli/onepassword_secrets_cli.py index 4811adf7f160..8b17c3e1eb39 100644 --- a/hermes_cli/onepassword_secrets_cli.py +++ b/hermes_cli/onepassword_secrets_cli.py @@ -487,6 +487,8 @@ def _op_version(binary: Path) -> str: [str(binary), "--version"], capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=5, ) if res.returncode == 0: diff --git a/tests/tools/test_subprocess_utf8_encoding.py b/tests/tools/test_subprocess_utf8_encoding.py index 6c2717c7cfa3..72e2f8c52e36 100644 --- a/tests/tools/test_subprocess_utf8_encoding.py +++ b/tests/tools/test_subprocess_utf8_encoding.py @@ -4,9 +4,9 @@ explicit ``encoding=`` triggers ``UnicodeDecodeError`` on Chinese Windows PR #55339 covers 21 call sites in ``agent/``, ``gateway/``, ``cli.py``, ``cron/``, plus 5 more in ``tools/`` and ``hermes_cli/`` (main.py, -setup.py, tts_tool.py, transcription_tools.py). The one call site it -misses — ``hermes_cli/onepassword_secrets_cli.py::_op_whoami`` — is -guarded here. +setup.py, tts_tool.py, transcription_tools.py). The two call sites it +misses — ``hermes_cli/onepassword_secrets_cli.py::_op_whoami`` and +``_op_version`` — are guarded here. Without ``encoding=``, ``text=True`` decodes child output with ``locale.getpreferredencoding(False)`` — cp936 on Chinese Windows — @@ -50,3 +50,19 @@ def test_op_whoami_passes_utf8_encoding(tmp_path): ) op_cli._op_whoami(fake_binary, account="") _assert_utf8_kwargs(mock_run) + + +def test_op_version_passes_utf8_encoding(tmp_path): + """_op_version must pass encoding='utf-8', errors='replace' so op CLI + output containing non-ASCII bytes doesn't crash on cp936. Pairs with + _op_whoami — both run in the same setup/status CLI flow.""" + from hermes_cli import onepassword_secrets_cli as op_cli + + fake_binary = tmp_path / "op" + fake_binary.write_bytes(b"") + with patch.object(op_cli.subprocess, "run") as mock_run: + mock_run.return_value = MagicMock( + returncode=0, stdout="2.24.0", stderr="" + ) + op_cli._op_version(fake_binary) + _assert_utf8_kwargs(mock_run)