Pythonのsubprocess呼び出しでshellとtimeout漏れをどうレビューするか

Pythonで外部コマンドを実行するとき、subprocess.run() は強力な道具になる。
ただしレビューでは、動くかどうかよりも、入力、停止、失敗情報の扱いを先に確認したい。

特に危ないのは次のような実装だ。

  • ユーザー入力を文字列結合して shell=True で実行している
  • timeout がなく、外部コマンドが停止しない可能性がある
  • check=True がなく、失敗しても成功扱いになる
  • 標準出力や標準エラーの扱いが曖昧
  • コマンド実行の責務が業務ロジックに埋め込まれている

この記事では subprocess の使い方紹介ではなく、
外部プロセス実行をレビューでどこまで止めるべきかを整理する。

まず止めたい実装

shell=Trueとtimeout漏れの実装 
import subprocess


def convert_image(input_path: str, output_path: str) -> None:
    command = f"convert {input_path} -resize 800x800 {output_path}"
    subprocess.run(command, shell=True)
Comment 
@Reviewer: `input_path` と `output_path` を文字列結合したうえで `shell=True` に渡しており、意図しないシェル解釈が起きます。引数はリストで渡し、shellを不要にしてください。
Comment 
@Reviewer: 外部コマンドにtimeoutがないため、convertがハングした場合に処理全体が停止します。呼び出し元のSLAに合わせてtimeoutを設定してください。

このコードは、正常なファイル名だけなら動く。
しかしレビューで見るべきなのは、正常系ではない。

外部プロセスは、入力に依存し、環境に依存し、停止しないこともある。
その前提で、実行境界を設計する必要がある。

なぜ危ないのか

shell=True は、文字列をシェルに解釈させる。
そのため、ファイル名やオプションに想定外の文字が入った場合、コマンドの意味が変わる可能性がある。

さらに timeout がないと、外部コマンドが終了しないときにPython側も待ち続ける。

実務では次のような事故につながる。

  • ファイル名に空白が入り、コマンドが壊れる
  • ユーザー入力がシェル構文として解釈される
  • 外部ツールのハングでワーカーが枯渇する
  • 失敗終了しているのに後続処理が成功扱いになる
  • stderrを捨てて原因調査ができない

レビューでは、subprocess を使っている箇所を
OS境界をまたぐ危険なI/Oとして扱うのが安全である。

レビューで見たい3つの判断線

1. shell=True が本当に必要か

多くのケースでは、shell=True は不要である。
コマンドと引数をリストで渡せば、シェル解釈を避けられる。

shellを使わない実装 
subprocess.run(
    ["convert", input_path, "-resize", "800x800", output_path],
    check=True,
    timeout=30,
)
Comment 
@Reviewer: シェル機能を使っていないため `shell=True` は不要です。引数リストで渡し、入力値がシェル構文として解釈されないようにしてください。

パイプやリダイレクトが必要な場合でも、Python側で stdout=subprocess.PIPE などを使って分けられないか確認したい。

2. timeoutが業務上の待ち時間と一致しているか

timeoutは単に付ければよいわけではない。
呼び出し元が待てる時間、ジョブの再試行方針、外部ツールの平均実行時間と接続している必要がある。

Comment 
@Reviewer: 外部コマンドにtimeoutがありません。処理が停止した場合にワーカーを占有するため、業務上許容できる待ち時間を決めて `timeout` を設定してください。

timeout値がハードコードされている場合も、設定値として切り出すか、処理責務に近い場所で名前を付けたい。

3. 失敗時の情報が残るか

check=True がないと、コマンドが非ゼロ終了しても例外にならない。
また、stderrを残さないと調査が難しくなる。

Comment 
@Reviewer: `check=True` がないため、コマンド失敗後も成功扱いで後続処理へ進む可能性があります。失敗を例外として扱い、stderrをログやエラー文脈に残してください。

改善例

コマンド実行を小さな関数に閉じ込めると、レビューしやすくなる。

安全側に寄せたsubprocess実行 
import subprocess


class CommandError(RuntimeError):
    pass


def convert_image(input_path: str, output_path: str, timeout_seconds: int = 30) -> None:
    try:
        subprocess.run(
            ["convert", input_path, "-resize", "800x800", output_path],
            check=True,
            timeout=timeout_seconds,
            capture_output=True,
            text=True,
        )
    except subprocess.TimeoutExpired as exc:
        raise CommandError(f"image conversion timed out: {input_path}") from exc
    except subprocess.CalledProcessError as exc:
        raise CommandError(
            f"image conversion failed: returncode={exc.returncode}, stderr={exc.stderr}"
        ) from exc

この形なら、レビューアーは次を確認しやすい。

  • shellを使っていない
  • 引数がリストで分離されている
  • timeoutがある
  • 失敗終了が例外になる
  • stderrが調査情報として残る
  • 業務ロジックからコマンド実行責務を切り出している

shellが必要なケースでも境界を狭くする

どうしてもシェル機能が必要な場合もある。
その場合は、入力値を直接埋め込まないこと、実行箇所を限定すること、レビューコメントで理由を残すことが重要になる。

shellが必要な場合でも入力を混ぜない 
subprocess.run(
    "set -o pipefail; df -h | awk '{print $5}'",
    shell=True,
    executable="/bin/bash",
    check=True,
    timeout=10,
)
Comment 
@Reviewer: この `shell=True` はパイプとshell optionに依存しているため理由はあります。ただし外部入力を混ぜないこと、timeoutとcheckを維持することをコメントで明示してください。

shell=True は禁止ワードではない。
ただし、使う理由をレビューで説明できないなら止めるべきである。

レビュー観点チェックリスト

subprocessレビューの確認項目
  • shell=True が不要に使われていないか
  • ユーザー入力やファイル名を文字列結合していないか
  • 引数をリストで渡しているか
  • timeout が設定され、値の根拠があるか
  • check=True で非ゼロ終了を検出しているか
  • stdout / stderr の扱いが調査可能になっているか
  • コマンド実行責務が業務ロジックから分離されているか

まとめ

subprocess は、PythonコードからOS境界を越えるための強いAPIである。
レビューでは、動作確認だけで通すのではなく、入力の解釈、停止条件、失敗時情報を必ず見る必要がある。

特に shell=True とtimeout漏れは、正常系では目立たない。
だからこそ、レビューアーが構造上のリスクとして早めに止めたい。