refactor(lib): wait_with_timeout を 2 variant で lib-subprocess に extract (順位 173b)

[aloekun]

Summary

• 順位 173b: wait_with_timeout を polling 系 2 variant (_safe / _basic) として lib-subprocess に extract
• 173 (subprocess utils 統合) 挙動保存型 sub-PR 分割の 2 番目。173a (combine_output 抽出、PR refactor(lib): combine_output を lib-subprocess に extract (順位 173a) #205 land 済) で確立した lib-subprocess 基盤の上に新 variant を追加
• 2 variant 並立は 挙動保存原則 によるもので、callsite ごとにこれまでと同じ動作を選ぶ形を維持。variant 統一の可否は 173e (dogfood 後判断) に分離

2 variant の意味

variant	Err 経路 (try_wait 失敗) の動作	callsite
wait_with_timeout_safe	child.kill() + child.wait() 後に Err 返却	cli-pr-monitor stages/push_jj_bookmark.rs (1 件)
wait_with_timeout_basic	child 操作なしで Err 返却	cli-push-runner runner.rs 内 1 件 + stages/{scratch_file_warning, push_jj_bookmark, pr_size_check, lint_screen, bookmark_check}.rs 計 6 件

timeout 経路の挙動は両 variant 同じ (kill+wait 実施)。

scope 調整 (実装時に判明)

当初計画では「cli-pr-monitor classifier_runner.rs (2 callsites)」も _basic を共用する想定だったが、実コード確認の結果、classifier_runner.rs の wait_with_timeout は:

• signature: fn(Child, Duration) -> Option<Output> (polling 系の fn(&str, &mut Child, u64) -> Result<Option<ExitStatus>, String> と非互換)
• 設計: wait_with_output() を spawn thread 内で呼び mpsc::channel で結果を受ける channel-based。Child を thread に move するため kill 不可
• 意図: pipe buffer overflow (~64 KB) デッドロック対策の意図的構造

…と 完全に別系統の設計 であることが判明。本 sub では touch せず、docs/todo11.md 順位 173b section と lib-subprocess/src/lib.rs module doc に scope outside の明示を追記。必要なら 173e で channel-based variant の評価を行う。

内訳 (12 files changed, +175 / -93)

区分	内容
lib-subprocess/src/lib.rs	2 variant + 4 test 追加 (各 variant の success / timeout)
cli-pr-monitor/Cargo.toml	lib-subprocess dep 復活 (173a で combine_output 不使用のため remove していた)
cli-pr-monitor/src/runner.rs	wait_with_timeout impl + use ExitStatus 削除
cli-pr-monitor/src/stages/push_jj_bookmark.rs	callsite 1 件を lib_subprocess::wait_with_timeout_safe に
cli-push-runner/src/runner.rs	wait_with_timeout impl + 1 test + use ExitStatus/Instant 削除、内部 callsite 1 件を _basic に
cli-push-runner/src/stages/*.rs 5 ファイル	callsite 5 件を lib_subprocess::wait_with_timeout_basic に (lint_screen.rs は use import 経由、他 4 は直接修飾)
docs/todo11.md	173b の work plan 完了マーク + classifier_runner.rs scope outside の調整を inline 記述

Test plan

• cargo build --workspace pass
• cargo test --workspace pass (lib-subprocess 9 test 含む workspace 全体 pass)
• cargo clippy --workspace -- -D warnings clean (push-runner-config.toml の gate と同等)
• pre-push review APPROVED (1 iteration, 6m 47s, F-1 は non-blocking record only)

pre-push reviewer 所見への対応

F-1 (non-blocking): _basic の doc コメントで「try_wait 失敗時の child cleanup は呼び出し側が制御する設計」と記載したが、6 callsite いずれも明示的な cleanup を行っていない。

• reviewer の正当性: 実装と doc が完全一致していない
• 本 PR での扱い: current impact: none (try_wait() の Err 経路は子プロセス spawn 成功後には実質 unreachable な path)、173e で variant merge 判断時に doc を実態ベース ("try_wait 失敗は spawn 成功後には unreachable のため child を release せず Err を返す") に書き直す予定
• 本 PR では fix-up commit を avoid (commit 履歴が乱れるため) し、follow-up として位置付け

関連

• 既マージ: PR refactor(lib): combine_output を lib-subprocess に extract (順位 173a) #205 (順位 173a / combine_output extract)
• docs/todo11.md 順位 173 entry — 173c (drain_pipe) / 173d (run_cmd) / 173e (variant merge 判断) の計画
• ADR-024 (lib-jj-helpers) — 異なる semantics を別 helper として export する先例
• ADR-026 (Cargo workspace) / ADR-012 (lib-* naming) — 設計根拠

Summary by CodeRabbit

• リファクタリング

  • サブプロセス管理機能を共有ライブラリに統合し、複数ツール間での実装の一貫性を向上

• テスト

  • プロセス待機およびタイムアウト処理のユニットテストスイートを拡充

[coderabbitai]

📝 Walkthrough

Walkthrough

lib-subprocess へプロセス待機ユーティリティ関数を 2 つ追加し、cli-pr-monitor と cli-push-runner で分散していたローカル wait_with_timeout 実装を削除・置換。エラーパス処理が異なる 2 variant を導入し、ワークスペース全体で重複排除を実現。タスク記録を実装完了状況で詳細化。

Changes

lib-subprocess 実装と既存 crate の統合

Layer / File(s)	Summary
lib-subprocess への wait_with_timeout 関数実装とテスト src/lib-subprocess/src/lib.rs	ExitStatus / Duration / Instant import 追加。POLL_INTERVAL_MS 定数導入。wait_with_timeout_safe は try_wait エラー時に kill + wait 後 Err を返す。wait_with_timeout_basic は kill 前に Err を即時返す。両関数の正常完了・タイムアウト・エラーケースを検証するテスト群を追加。
cli-pr-monitor での lib-subprocess 統合 src/cli-pr-monitor/Cargo.toml, src/cli-pr-monitor/src/runner.rs, src/cli-pr-monitor/src/stages/push_jj_bookmark.rs	Cargo.toml へ lib-subprocess 依存を追加。runner.rs で ExitStatus import を削除しローカル wait_with_timeout 関数を削除。push_jj_bookmark.rs で crate::runner::wait_with_timeout から lib_subprocess::wait_with_timeout_safe への呼び出し置換。
cli-push-runner での lib-subprocess 統合と複数ステージ置換 src/cli-push-runner/src/runner.rs, src/cli-push-runner/src/stages/bookmark_check.rs, src/cli-push-runner/src/stages/lint_screen.rs, src/cli-push-runner/src/stages/pr_size_check.rs, src/cli-push-runner/src/stages/push_jj_bookmark.rs, src/cli-push-runner/src/stages/scratch_file_warning.rs	runner.rs で import 整理・lib_subprocess::wait_with_timeout_basic 取り込み。ローカル wait_with_timeout 関数と関連テストケースを削除。run_cmd で wait_with_timeout_basic 呼び出し置換。5 つのステージ（bookmark_check / lint_screen / pr_size_check / push_jj_bookmark / scratch_file_warning）で各呼び出しを lib_subprocess::wait_with_timeout_basic に統一。
タスク記録と実装完了状況の詳細化 docs/todo11.md	「173b: wait_with_timeout」セクションを、抽出予定の概要から実装完了状況へ更新。wait_with_timeout_safe / wait_with_timeout_basic の 2 variant エラーパス仕様、lib-subprocess 追加、各 crate 依存復活、ステージ置換、workspace test/clippy 完了を詳細化。173c/d/e は未着手である旨を明記。

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• aloekun/claude-code-hook-test#174: PR #174 は scratch_file_warning ステージで jj file list のタイムアウト処理を導入し、本 PR ではその同一ステージの run_jj_file_list_at で wait_with_timeout_basic への置換を実施しています。

• aloekun/claude-code-hook-test#175: 本 PR と PR #175 はいずれも src/cli-push-runner/src/stages/bookmark_check.rs の run_bookmark_check ステージロジックを変更します。PR #175 は bookmark_check ステージを導入・実装し、本 PR ではその同一ステージでプロセス待機ユーティリティを wait_with_timeout_basic に置換しています。

• aloekun/claude-code-hook-test#61: 本 PR は cli-pr-monitor の push_jj_bookmark および複数ステージで、ローカル crate::runner::wait_with_timeout から lib_subprocess::wait_with_timeout_* へ置換し、ローカルヘルパーを削除しています。PR #61 は当該 wait_with_timeout ヘルパーと push_jj_bookmark ステージを導入したPRです。

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	タイトルは、プルリクエストの主な変更内容である「wait_with_timeout を 2 variant で lib-subprocess に extract」を明確に要約しており、具体的で関連性が高い。
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/cli-push-runner/src/stages/bookmark_check.rs (1)

88-90: ⚡ Quick win

共通根因: _basic variant の Err 契約に対して呼び出し側 cleanup が未実装です。

wait_with_timeout_basic は try_wait エラー時に child を回収しない設計ですが、3箇所とも ? で即 return しており、Err 経路の後始末が漏れます。

• src/cli-push-runner/src/stages/bookmark_check.rs#L88-L90: _safe へ切替えるか、Err 分岐で kill + wait（必要に応じて reader 回収）を追加してください。
• src/cli-push-runner/src/stages/pr_size_check.rs#L160-L162: 同様に Err 分岐 cleanup を明示するか _safe を使用してください。
• src/cli-push-runner/src/stages/scratch_file_warning.rs#L234-L236: 同様に Err 分岐 cleanup を明示するか _safe を使用してください。

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli-push-runner/src/stages/bookmark_check.rs` around lines 88 - 90, The
three call sites use lib_subprocess::wait_with_timeout_basic which does not
clean up the child on Err; for each site either switch to the safe variant (e.g.
wait_with_timeout_safe) or explicitly perform the error-path cleanup by killing
the child, waiting for it, and dropping/closing any associated reader handles:
for src/cli-push-runner/src/stages/bookmark_check.rs (lines 88-90) replace the
wait_with_timeout_basic call with the _safe variant or add an Err branch that
calls child.kill()/child.wait() and closes the reader before returning the Err;
for src/cli-push-runner/src/stages/pr_size_check.rs (lines 160-162) do the same
(use _safe or add explicit kill+wait and reader reclamation on Err); and for
src/cli-push-runner/src/stages/scratch_file_warning.rs (lines 234-236) apply the
same fix (either swap to the _safe API or implement kill+wait and reader cleanup
in the Err path) so no child or reader is leaked on failure.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/cli-push-runner/src/stages/bookmark_check.rs`:
- Around line 88-90: The three call sites use
lib_subprocess::wait_with_timeout_basic which does not clean up the child on
Err; for each site either switch to the safe variant (e.g.
wait_with_timeout_safe) or explicitly perform the error-path cleanup by killing
the child, waiting for it, and dropping/closing any associated reader handles:
for src/cli-push-runner/src/stages/bookmark_check.rs (lines 88-90) replace the
wait_with_timeout_basic call with the _safe variant or add an Err branch that
calls child.kill()/child.wait() and closes the reader before returning the Err;
for src/cli-push-runner/src/stages/pr_size_check.rs (lines 160-162) do the same
(use _safe or add explicit kill+wait and reader reclamation on Err); and for
src/cli-push-runner/src/stages/scratch_file_warning.rs (lines 234-236) apply the
same fix (either swap to the _safe API or implement kill+wait and reader cleanup
in the Err path) so no child or reader is leaked on failure.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f3cec832-6a70-4afa-9fc3-6b4d928e7795

📥 Commits

Reviewing files that changed from the base of the PR and between 8d4793a and 25500f9.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (11)

• docs/todo11.md
• src/cli-pr-monitor/Cargo.toml
• src/cli-pr-monitor/src/runner.rs
• src/cli-pr-monitor/src/stages/push_jj_bookmark.rs
• src/cli-push-runner/src/runner.rs
• src/cli-push-runner/src/stages/bookmark_check.rs
• src/cli-push-runner/src/stages/lint_screen.rs
• src/cli-push-runner/src/stages/pr_size_check.rs
• src/cli-push-runner/src/stages/push_jj_bookmark.rs
• src/cli-push-runner/src/stages/scratch_file_warning.rs
• src/lib-subprocess/src/lib.rs