Skip to content

fix(cli-pr-monitor): 並列テスト flaky を tempfile 一意化と state path DI で構造的に解消 - #227

Merged
aloekun merged 3 commits into
masterfrom
fix-pr-monitor-flaky-tests
Jun 30, 2026
Merged

fix(cli-pr-monitor): 並列テスト flaky を tempfile 一意化と state path DI で構造的に解消#227
aloekun merged 3 commits into
masterfrom
fix-pr-monitor-flaky-tests

Conversation

@aloekun

@aloekun aloekun commented Jun 30, 2026

Copy link
Copy Markdown
Owner

概要

cli-pr-monitor の並列テストで断続的に発生していた flaky test を構造的に解消する。原因は独立した 2 系統あり、それぞれ別コミットで修正した。加えて完了済みの todo エントリを整理する。

変更内容

1. create_pr temp-file flaky の解消 (tempfile crate + dir injection)

convert_body_to_file--body-file 用一時ファイルを pid + ms 命名で生成していたため、並列 cargo test(同一プロセス・同一 ms)で名前が衝突し flaky 化していた。

  • tempfile crate を導入し O_EXCL + ランダム名 + 衝突時リトライで一意名を生成
  • 生成先ディレクトリを引数注入(本番は std::env::temp_dir()、テストは専用 tempdir() を注入して本番 namespace から分離)
  • Drop guard を自前 TempFile struct から tempfile::TempPath に置換

2. parallel-test flaky の解消 (state path DI + env-var global 除去)

state ファイルパスを env-var 経由の ambient global で解決していたため、並列テスト間で競合していた。共有グローバルやロックではなく依存注入で根本解決する方針に沿う。

  • state path を全レイヤー(monitor / poll / rate_limit / review_recheck)に引数注入
  • env-var global を撤去

3. docs(todo) の整理

完了済みエントリ(順位 229・230)を todo-summary.md / todo13.md から削除。

テスト

push pipeline の品質ゲート(clippy / cargo test / cargo test --ignored)で検証済み。

Summary by CodeRabbit

  • Bug Fixes

    • 一時ファイルや状態ファイルの競合を避けるようになり、並列実行や不安定なテストに起因する問題が減りました。
    • 状態保存・読み取りがより明示的になり、監視や再開処理の信頼性が向上しました。
  • Chores

    • 内部の状態管理まわりの実装整理により、環境差による挙動のぶれを抑えました。

aloekun and others added 3 commits June 30, 2026 14:40
…pfile + dir injection

順位 230。convert_body_to_file は temp file 名を PID+ミリ秒で生成しており、並列 cargo test で body_with_literal_newline_converted 等が稀に衝突して fail していた。tempfile::Builder::tempfile_in による O_EXCL+乱数の一意名生成へ移行し、生成先 dir を注入可能化。テストは各自 tempfile::tempdir() を渡して本番 namespace から分離。tempfile を dev-dependency から通常依存へ昇格し、自前 TempFile struct を TempPath に置換。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…obal (fixes parallel-test flaky)

順位 229。PR_MONITOR_STATE_FILE_OVERRIDE (process-global env var) を複数 test module の独立 static LOCK が奪い合い、finalize_park_siblings_have_symmetric_write_state_handling が並列実行で約 50% fail してゲートを高確率で落としていた。env var seam と state_file_path の env 分岐、write_state/read_state ラッパ、4 つの env_override_lock を全廃。state path を PollContext + 関数引数で全面注入し、各テストは自前 path を渡す。共有グローバル可変状態を根絶し並列競合を構造的に解消。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
順位 229 (env_override_lock / state path DI) と順位 230 (create_pr tempfile) を解決済みのため、todo13.md の詳細エントリと todo-summary.md の該当行を削除。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

PR_MONITOR_STATE_FILE_OVERRIDE 環境変数と env_override_lock によるグローバルな状態ファイル切り替え機構を廃止し、write_state/read_state ラッパーを削除。PollContextstate_path: &Path フィールドを追加して、monitor・poll・rate_limit・review_recheck・create_pr の各ステージへ明示的なファイルパスを伝播する形に統一した。

Changes

明示的な state_path 注入リファクタリング

Layer / File(s) Summary
state.rs: ラッパー削除と state_file_path 単純化
src/cli-pr-monitor/src/state.rs, src/cli-pr-monitor/Cargo.toml
state_file_path() から PR_MONITOR_STATE_FILE_OVERRIDE 参照を除去し常に実行ファイル配下を返すよう変更。write_state/read_state ラッパーを削除。tempfiledev-dependencies から dependencies に昇格。
PollContext への state_path 追加と run_poll_loop 配線
src/cli-pr-monitor/src/stages/poll/mod.rs
PollContextstate_path: &'a Path を追加し、run_poll_loopstate_file_path() から取得した値を注入するよう変更。
monitor.rs: 状態読み書きをパス明示に変更
src/cli-pr-monitor/src/stages/monitor.rs
write_state/read_statewrite_state_to(&state_file_path(), ...)/read_state_from(...) に置き換え、resume_fix_push_time_or_started_atstate_path: &Path 引数を追加。テストから env override を削除。
iteration.rs: state_path の伝播
src/cli-pr-monitor/src/stages/poll/iteration.rs
build_state_for_iterationenrich_with_classifierhandle_rate_limit_branchstate_path を追加し、read_state_from/write_state_to に切り替え。テストは tempdirstate.json パスを生成して注入。
rate_limit.rs: finalize_* への state_path 伝播
src/cli-pr-monitor/src/stages/poll/rate_limit.rs
handle_rate_limit_branch/dispatch_rate_limit_outcome/finalize_posted_retrigger/finalize_parkedstate_path 引数を追加し write_state_to に置換。テストは env override を廃止し unwritable_state_path() を直接渡す形に変更。
review_recheck.rs: ctx.state_path 経由の読み書きとテスト更新
src/cli-pr-monitor/src/stages/poll/review_recheck.rs
finalize_initial_review_park/finalize_review_recheck_park/finalize_review_recheck_max_reached/schedule_next_review_recheck_parkctx.state_path 経由の read_state_from/write_state_to に変更。テストは env 変数操作を全廃し tempdir + state_path 注入に統一。
poll/mod.rs テスト・review_recheck_signal.rs テスト
src/cli-pr-monitor/src/stages/poll/mod.rs, src/cli-pr-monitor/src/stages/poll/review_recheck_signal.rs
invoke_finalize_*_with_bad_path ヘルパーに state_path 引数を追加して PollContext に注入。env_override_lock/set_var/remove_var を全廃。review_recheck_signal.rs のテストでは PollContext 初期化に state_path: Path::new("unused") を追加。
create_pr.rs: TempPath 導入と配線変更
src/cli-pr-monitor/src/stages/create_pr.rs
カスタム TempFile を廃止し write_body_tempfiletempfile::TempPath を返す形に変更。convert_body_to_filetemp_dir: &Path を追加し Option<TempPath> を返す形に変更。write_early_reset_statewrite_state_to に切り替え。
docs: 完了タスクの削除
docs/todo13.md, docs/todo-summary.md
env_override_lock 共有化と create_pr::tests flaky 修正の2タスクエントリを削除。

推定コードレビュー工数

🎯 3 (Moderate) | ⏱️ ~25 minutes

関連する可能性のある PR

  • aloekun/claude-code-hook-test#224: stages/poll/* の poll モジュール分割と iteration/signal 構造を導入した PR で、本 PR が同モジュールの state_path 伝播リファクタリングを直接実施している。
  • aloekun/claude-code-hook-test#214: rate_limit.rs/review_recheck.rsfinalize_* API を導入した PR で、本 PR がそれらのシグネチャに state_path 引数を追加している。
  • aloekun/claude-code-hook-test#114: review_recheck park モデルと PrMonitorState フィールドを実装した PR で、本 PR がその状態永続化を ctx.state_path 経由に変更している。
🚥 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 cli-pr-monitor の並列テスト flaky を、tempfile の一意化と state path DI で解消する変更を的確に要約しています。
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.
✨ 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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@src/cli-pr-monitor/src/stages/create_pr.rs`:
- Around line 207-208: `write_early_reset_state()` is still resolving
`state_file_path()` internally, so it can’t be isolated from the other state
operations in `run_create_pr`. Update `run_create_pr` to accept and pass a
`state_path: &Path` into `write_early_reset_state()`, and change that helper to
use the provided path when calling `write_state_to()`. This will let tests
inject a tempdir-backed state file and avoid sharing state across parallel or
repeated runs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7e1a4830-62bf-4b65-a0b1-f1222f228f38

📥 Commits

Reviewing files that changed from the base of the PR and between e193714 and cfe2f2d.

📒 Files selected for processing (11)
  • docs/todo-summary.md
  • docs/todo13.md
  • src/cli-pr-monitor/Cargo.toml
  • src/cli-pr-monitor/src/stages/create_pr.rs
  • src/cli-pr-monitor/src/stages/monitor.rs
  • src/cli-pr-monitor/src/stages/poll/iteration.rs
  • src/cli-pr-monitor/src/stages/poll/mod.rs
  • src/cli-pr-monitor/src/stages/poll/rate_limit.rs
  • src/cli-pr-monitor/src/stages/poll/review_recheck.rs
  • src/cli-pr-monitor/src/stages/poll/review_recheck_signal.rs
  • src/cli-pr-monitor/src/state.rs
💤 Files with no reviewable changes (2)
  • docs/todo13.md
  • docs/todo-summary.md

Comment thread src/cli-pr-monitor/src/stages/create_pr.rs
@aloekun
aloekun merged commit 3d8e2aa into master Jun 30, 2026
1 check passed
@aloekun
aloekun deleted the fix-pr-monitor-flaky-tests branch June 30, 2026 10:16
aloekun added a commit that referenced this pull request Jun 30, 2026
…228)

* docs(todo): 順位 233-235 を登録 — post-pr-review docs-only 誤判定 + post-merge-feedback 採用候補 (PR #227)

* docs(adr): ADR-045 に PR 運用知見 (secondary workspace の GIT_DIR 必須・bookmark 誤検出) を追記 (初 PR 運用ケースの dogfood)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant