Repository navigation
refactor(pr_review): run subprocesses through tool_utils.run_command - #1559
Merged
VLukiantsev merged 2 commits intoNov 3, 2026
Merged
VLukiantsev merged 2 commits into
VLukiantsev merged 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors several PR review tools, including github_utils, reply_pr_comment, and wait_for_ci, to replace raw subprocess calls with the shared run_command utility from tools.common.tool_utils. The corresponding unit tests have been updated to mock and assert on run_command instead of subprocess. No review comments were provided, and the changes appear correct and consistent.
VLukiantsev
force-pushed
the
refactor/pr-review-run-command
branch
from
November 3, 2026 11:40
b6467d0 to
edacaef
Compare
github_utils, reply_pr_comment and wait_for_ci called subprocess directly with hand-rolled capture, timeout and decoding options. Route them through tool_utils.run_command, which now supports input_text, timeout and errors. All calls pass log_cmd=False and log_on_error=False so stdout stays JSON-only and stderr is unchanged. No handled_instruction is set, so the raw CalledProcessError/TimeoutExpired/OSError still reach the existing handlers: github_utils' credential fallback, reply_pr_comment's "unknown" commit fallback, and wait_for_ci's transient-retry loop with its 60s per-call timeout. wait_for_ci's two gh calls share a small _run_gh helper. TAG=agy CONV=2937a1a9-78d5-4d4f-806c-262c963bb857
VLukiantsev
force-pushed
the
refactor/pr-review-run-command
branch
from
November 3, 2026 12:09
edacaef to
0cc403a
Compare
VLukiantsev
marked this pull request as ready for review
November 3, 2026 14:40
janzak-dev
previously approved these changes
Nov 3, 2026
test_call_github_api_gh_cli_sends_payload_on_stdin only checked the argv tail and input_text, so a changed method or endpoint, or dropped log_cmd/log_on_error flags (which keep tool stdout JSON-only), would still pass. Assert the complete run_command call and the return value, matching the GET test. TAG=agy CONV=2937a1a9-78d5-4d4f-806c-262c963bb857
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (0 lines, excluding 12 test file lines) are within the 50-line grace delta.
Preserving prior approval by @janzak-dev on commit 0cc403a.
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.
Summary
Moves the remaining direct
subprocesscalls intools/pr_review(github_utils.py,reply_pr_comment.py,wait_for_ci.py) ontotool_utils.run_command, which #1532 hardened withinput_text,timeoutanderrors.Motivation & Context
Each of these tools set its own capture, timeout and decoding options on
subprocess.runorcheck_output. Sending them through the shared wrapper gives one place to harden process handling. CLI behavior and exit codes stay the same.Key Changes
github_utils._call_api_with_gh:run_command(..., input_text=payload_json). ACalledProcessErrorstill falls through to theGITHUB_TOKENand unauthenticated fallbacks.reply_pr_comment: the SKILL.md commit lookup usesrun_command(capture_output=True). stderr is captured instead of sent toDEVNULL, and any failure still falls back to"unknown".wait_for_ci: both gh calls go through a small_run_gh(cmd, *, check)helper. It keeps the 60s per-call timeout anderrors="replace", and rawCalledProcessError,TimeoutExpiredandOSErrorstill reach the existing transient-retry loop and the log-tail placeholders.log_cmd=Falseandlog_on_error=False, so stdout stays JSON-only and stderr is unchanged. Nohandled_instructionis set, so callers see the same exceptions as before.github_utilsnow depends on//tools/common:tool_utilsinBUILD.bazel.<module>.run_commandinstead ofsubprocess.*, and assert the exact kwargs.Reused:
tool_utils.run_command(keyword-onlyinput_text,timeout,errorsfrom #1532).Assumptions
run_commandpasses a copy ofos.environasenv=. This matches inheriting the environment, so it has no effect.github_utils._call_api_with_ghsets no timeout on the gh call, and anOSError(gh on PATH but not executable) is not caught by itsCalledProcessError/JSONDecodeErrorfallback.Verification & Testing
bazel test //tools/pr_review/...(11/11 pass),./.githooks/pre-commit. Live run:wait_for_ci.py 1535 --repo google/xmcprinted only JSON on stdout and exited 0.--input -withinput_text) and the quiet kwargs of the git lookup.Related Issues / Bugs
N/A
TAG=agy
CONV=2937a1a9-78d5-4d4f-806c-262c963bb857