Repository navigation
fix(tools): harden run_command and adopt it in tools/lint - #1532
Conversation
Extend tools/common/tool_utils.run_command with keyword-only options instead of adding a parallel subprocess helper: - input_text: text written to the command's stdin. - timeout: kill the command and raise subprocess.TimeoutExpired. - errors: decoding error handler (e.g. "replace"); None keeps strict decoding, so existing callers are unchanged. - handled_instruction: opt-in conversion of OSError (missing or non-executable binary), TimeoutExpired, and CalledProcessError (check=True) into HandledError carrying the remediation hint, so run_tool exits 1 with JSON instead of exiting 2 with a traceback. Conversion is opt-in because existing callers (e.g. tools/dev_env) catch OSError and CalledProcessError from run_command. Migrate check_legacy_imports.py, check_agent_doc_paths.py, and precommit_autostage.py to run_command. check_legacy_imports no longer crashes on non-UTF-8 tracked paths; precommit_autostage turns git failures into handled errors and round-trips non-UTF-8 paths with surrogateescape. TAG=agy CONV=0640f971-1742-419e-afd7-3309df06d861
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the run_command utility in tools/common/tool_utils.py by adding support for stdin input, timeouts, custom decoding error handlers, and wrapping subprocess exceptions into a user-friendly HandledError. Several linting tools are refactored to use this updated helper, and comprehensive unit tests are added. The feedback suggests using shlex.join to safely format commands with spaces and adding a guard to prevent an IndexError in _command_error if the command list is empty.
There was a problem hiding this comment.
Code Review
This pull request enhances the run_command helper in tools/common/tool_utils.py to support input text, timeouts, custom decoding error 8000 handling, and structured HandledError wrapping with remediation instructions. Various linting tools (check_agent_doc_paths.py, check_legacy_imports.py, and precommit_autostage.py) are refactored to utilize this updated helper, improving their robustness against git errors and non-UTF-8 file paths. The review feedback suggests using shlex.join(cmd) instead of " ".join(cmd) in _command_error to ensure command strings are formatted correctly when arguments contain spaces or quotes.
Use shlex.join when formatting the command in HandledError messages so arguments containing spaces or quotes stay unambiguous. Existing "Running:" and "Command failed:" log lines are unchanged. TAG=agy CONV=0640f971-1742-419e-afd7-3309df06d861
Summary
Hardens the existing shared subprocess helper
tools/common/tool_utils.run_commandwith stdin input, an optional timeout, a configurable decoding error handler, and opt-in conversion of launch/timeout/exit failures intoHandledError. Migrates the threetools/lintscripts that calledsubprocessdirectly to use it.Motivation & Context
Review feedback on #1474, #1475, #1476, and #1477 repeatedly flagged subprocess calls that crashed with
UnicodeDecodeErroron non-UTF-8 output, could hang without a timeout, or turned a missing binary or git failure into an exit-2 traceback instead of a handled error. Each tool fixed this locally. This PR moves the fix into the existing shared helper instead of adding another one.Design choices:
input_text=None,timeout=None,errors=None,handled_instruction=None) produce exactly the samesubprocess.runbehavior as before for all existing callers.HandledErrorconversion is opt-in (handled_instruction=...), not the default. Existing callers catch the raw exceptions (for example,tools/dev_env/deploy_dev_env.pycatchessubprocess.CalledProcessErrorandOSErrorfromverify_helm_min_version, which callsrun_command), andHandledErroris not anOSError. Converting by default would bypass those handlers. A required hint string also means every converted error carries a remediation instruction.errors="replace"(lint path listings) orerrors="surrogateescape"(precommit_autostage, which writes paths back to git over stdin, wherereplacewould corrupt non-UTF-8 names).Key Changes
tools/common/tool_utils.py:run_commandgainsinput_text,timeout,errors,handled_instruction. A new private_command_errorbuilds theHandledErrormessage: binary name plus OS error, the timeout, or the exit code plus captured stderr.tools/lint/check_legacy_imports.py: usesrun_command. It no longer crashes on non-UTF-8 tracked paths (it used to call strict.decode()), and git failures are now handled errors (exit 1) instead of tracebacks.tools/lint/check_agent_doc_paths.py: replaces its localtry/exceptwrapper withrun_command(..., handled_instruction=...). The instruction is unchanged. The message text changes fromgit ls-files failed in <root>: <exc>to the shared format.tools/lint/precommit_autostage.py:_gitusesrun_commandwith stdin input. Git failures now produce a handled error that points atXMC_PRECOMMIT_NO_AUTOSTAGE=1instead of a traceback.TestRunCommandHardeningcovering stdin,errors="replace", strict default, raw exceptions without the opt-in (backward compatibility), handled missing binary, timeout, non-zero exit with stderr,check=False, and logging. Lint tests cover a missing git binary (patch target moved totools.common.tool_utils.subprocess.run), running outside a repo, non-UTF-8 tracked paths, and non-UTF-8 restaging.Assumptions
timeout: they are local, non-interactive git commands (ls-files,diff,hash-object,add) that don't block on network or credentials. If they did hang, the hook would stall as it does today. Adding one later is a single keyword argument.check_agent_doc_pathsgit failures changes (exit code and instruction unchanged). Nothing parses that message; the pre-commit hook only checks the exit status.subprocesscall sites:tools/pr_review/github_utils.py:100,tools/pr_review/reply_pr_comment.py:89,tools/pr_review/wait_for_ci.py:170,190. These are left alone because a parallel change edits these files.tools/release/git_ops.py:32,48,80,154,189,tools/release/github_release_client.py:92.Verification & Testing
./.githooks/pre-commit(full run, no bypass: formatters, linters,run_affected_tests.py --skip-emulator-check, 293/293 Bazel tests passed, including everytools/dev_env,tools/release, anddeploytarget that depends onrun_command);./tools/analyze_diff_size.py --base-ref origin/main...(75 prod / 94 test lines).run_command,_command_error, and the three lint call sites is covered. File totals:tool_utils.py92%,check_agent_doc_paths.py98%,check_legacy_imports.py94%,precommit_autostage.py92%. The uncovered lines are pre-existingmain()entrypoints and venv bootstrap.Related Issues / Bugs
N/A
TAG=agy
CONV=0640f971-1742-419e-afd7-3309df06d861