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

docs/todo-summary.md

@@ -99,8 +99,6 @@
 | 226 | 🚀 Tier 1 | **fmt baseline cleanup + `cargo fmt --check` gate 導入 + rustfmt 固定 (PR #224 セッション合意)** | todo13.md | M | なし (fmt enforcement がリポジトリに皆無で workspace 29 ファイルが rustfmt-clean でないドリフト蓄積。(A) `cargo fmt --all` 一括正規化 → (B) fmt --check を Stop/push gate に追加 → (C) rust-toolchain.toml で rustfmt 固定、の順。file_length plan と同型「clean baseline → gate」) |
 | 227 | 🚀 Tier 1 | **rule⑬: 非テストコードでの理由なし `#[allow(...)]` 禁止 custom lint (PR #224 セッション合意)** | todo13.md | S | なし (`#[allow]` = lint の握り潰し、既存 swallowed-error 系 rule③/④/⑩ と同 philosophy。justification マーカー無しの `#[allow(...)]` を warning 検出、test code 除外方式は着手時判断、rule_test_coverage_check で positive/negative test 機械強制) |
 | 228 | 🔧 Tier 2 | **`rate_limit_signal::cr_clean` の regression test (PR #224 post-merge-feedback T2-1 採用)** | todo13.md | S | なし (Fix 3 で拡張した `unresolved_threads` / `new_comments` / `actionable_comments` 3 field の clean 判定の回帰防止、None/境界ケース網羅、silent-clean 誤認の保護) |
-| 229 | 🔧 Tier 2 | **共有 `env_override_lock` helper 抽出 — 並列 test の state file override 競合解消 (PR #224 post-merge-feedback T2-2 採用)** | todo13.md | S | なし (複数 test module の独立 static LOCK を単一化。memory `feedback_test_dry_antipattern` の per-module 複製原則との境界 = serialization primitive は単一必須、要ユーザー確認) |
-| 230 | 🔧 Tier 2 | **`create_pr::tests` temp file collision 修正 (flaky test) — `tempfile` 移行 (PR #224 post-merge-feedback T2-4 採用)** | todo13.md | S | なし (並列 `cargo test` で `body_with_literal_newline_converted` が PID+ms 衝突で稀 fail、pre-existing。target は `cli-pr-monitor/src/stages/create_pr.rs` (report の cli-github は誤り)、CI 信頼性) |
 | 231 | 💎 Tier 3 | **ADR-022 拡張 — pre-create cleanup flow 例 + agent fmt スコープ指針 (PR #224 post-merge-feedback T3-1 採用)** | todo13.md | S | なし (CodeRabbit が `create_fix_commit` の空 findings 設計を bug 誤判定=却下 CR#2、agent 無差別 fmt の 2 事象を ADR-022 責務分離で codify、doc-only) |
 | 232 | 🔧 Tier 2 | **post-merge-feedback / workflow agent の repo 作業ツリー書込禁止 + 検知安全網 (PR #224 セッション合意)** | todo13.md | S-M | なし (merge 時に analyze-session agent が repo root に throwaway script (parse_transcript.py) を残した、日常工程ゆえ累積リスク = コンテキスト汚染。(1) feedback facets に repo 書込禁止 + jq/scratch 使用を明記 (2) post_steps/Stop hook で root 新規 untracked を warning 検知 (3) gitignore は補助) |
 

docs/todo13.md

@@ -137,54 +137,6 @@
 
 ---
 
-### 共有 `env_override_lock` helper 抽出 — 並列 test の state file override 競合解消 (PR #224 post-merge-feedback T2-2 採用)
-
-> **動機**: `PR_MONITOR_STATE_FILE_OVERRIDE` (process-global env var) 用の `static LOCK` を複数の test module (poll/mod.rs, rate_limit.rs, review_recheck.rs 等) が独立定義しており、並列 `cargo test` で state file override が競合するリスク。CodeRabbit outside-diff comment でも指摘。
->
-> **本タスクの位置づけ**: PR #224 post-merge-feedback Tier 2 #2 採用 (Effort S / Frequency Medium)。
->
-> **参照**: `.claude/feedback-reports/224.md` Tier 2 #2、各 test module の `env_override_lock()` / `static LOCK`、memory `feedback_test_dry_antipattern` (※下記の境界判断あり)。
->
-> **実行優先度**: 🔧 **Tier 2** — Effort S。
->
-> **重要 (規約整合)**: memory `feedback_test_dry_antipattern` は「test helper は共有 module 抽出せず per-module 複製」を原則とする。一方 env override LOCK は **process-global な serialization 用 static** で、複製すると各々が別 Mutex になり並列競合を防げない (= 複製が機能を壊す特殊ケース)。本タスクは「複製が正しい通常 helper」と「単一 static が必須な lock」を区別し、後者のみ共有化する。着手前にこの境界をユーザー確認。
-
-#### 作業計画
-
-- [ ] env override 用の単一 `static LOCK` を 1 箇所に集約し各 test module から参照
-- [ ] `feedback_test_dry_antipattern` との整合を明文化 (serialization primitive = 単一必須 / 通常 helper = 複製、の区別)
-- [ ] 並列 `cargo test` で state file override 競合が起きないことを確認
-- [ ] 本 entry 削除 + todo-summary.md 行削除
-
-#### 完了基準
-
-- env override の serialization が単一 LOCK で保証され並列 test で競合しない。per-module 複製原則との境界がドキュメント化される。
-
----
-
-### `create_pr::tests` temp file collision 修正 (flaky test) — `tempfile` 移行 (PR #224 post-merge-feedback T2-4 採用)
-
-> **動機**: 並列 `cargo test` で `create_pr::tests::body_with_literal_newline_converted` が稀に fail する (temp file 名が PID+ms timestamp で衝突)。PR-W2 分割以前から存在する pre-existing flaky で、PR #224 セッション中に実観測。flaky の常態化は test 全体の信頼性 (グリーンの価値) を損なう。
->
-> **本タスクの位置づけ**: PR #224 post-merge-feedback Tier 2 #4 採用 + セッション実観測。
->
-> **参照**: `.claude/feedback-reports/224.md` Tier 2 #4、`src/cli-pr-monitor/src/stages/create_pr.rs` `convert_body_to_file` (temp file 名生成。※ report の target `src/cli-github/...` は誤りで、実際は cli-pr-monitor)、`tempfile` crate (既に依存)。
->
-> **実行優先度**: 🔧 **Tier 2** — Effort S。CI 信頼性。
-
-#### 作業計画
-
-- [ ] `convert_body_to_file` の temp file 名生成を PID+ms timestamp から `tempfile::NamedTempFile` (一意保証) に移行
-- [ ] 繰り返し並列実行で衝突しないことを確認
-- [ ] `cargo test -p cli-pr-monitor` pass
-- [ ] 本 entry 削除 + todo-summary.md 行削除
-
-#### 完了基準
-
-- `body_with_literal_newline_converted` 等が並列実行でも安定 pass、temp file 名衝突が構造的に解消。
-
----
-
 ### ADR-022 拡張 — pre-create cleanup flow の具体例 + agent fmt スコープ指針 (PR #224 post-merge-feedback T3-1 採用)
 
 > **動機**: PR #224 で CodeRabbit が `create_fix_commit` の「空 findings でも commit 作成」を bug と誤判定した (ADR-022 の意図的な pre-create 設計を知らなかったため、却下した CR#2)。また分割 agent が無差別 `cargo fmt` を実行した事象も ADR-022 の責務分離原則で説明可能。両事象とも将来再発が見込まれる。

src/cli-pr-monitor/Cargo.toml

@@ -7,13 +7,15 @@ edition = "2021"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 toml = "0.8"
+# tempfile は本番の convert_body_to_file が --body-file 用一時ファイルを
+# 一意名 (O_EXCL + ランダム名) で生成するために使用 (順位 230 flaky 修正)。
+tempfile = "3"
 lib-jj-helpers = { path = "../lib-jj-helpers" }
 lib-pending-file = { path = "../lib-pending-file" }
 lib-report-formatter = { path = "../lib-report-formatter" }
 lib-subprocess = { path = "../lib-subprocess" }
 
 [dev-dependencies]
-tempfile = "3"
 proptest = "1"
 
 # [profile.release] は workspace root (Cargo.toml) に集約 (ADR-026)

src/cli-pr-monitor/src/stages/create_pr.rs

@@ -1,10 +1,12 @@
-use std::path::PathBuf;
+use std::path::Path;
+
+use tempfile::TempPath;
 
 use crate::config::DEFAULT_STEP_TIMEOUT_SECS;
 use crate::log::log_info;
 use crate::runner::{run_cmd_direct, run_gh_quiet};
 use crate::stages::monitor::start_monitoring;
-use crate::state::{write_state, PrMonitorState};
+use crate::state::{state_file_path, write_state_to, PrMonitorState};
 use crate::util::{
     get_jj_bookmarks, get_pr_head_commit, get_pr_info, parse_pr_number_from_url, utc_now_iso8601,
     PrInfo,
@@ -43,46 +45,45 @@ fn reassemble_split_body(args: &[String]) -> Vec<String> {
     result
 }
 
-// ─── --body -> --body-file 変換 (issue #1) ───
-
-/// Drop 時に自動削除される一時ファイル
-struct TempFile(PathBuf);
-
-impl Drop for TempFile {
-    fn drop(&mut self) {
-        let _ = std::fs::remove_file(&self.0);
-    }
+/// `dir` 配下に一意な一時ファイルを作成し `content` を書き出す。
+///
+/// 返り値の `TempPath` は Drop 時に自動削除される (旧 `TempFile` struct の役割)。
+/// `tempfile` は `O_EXCL` + ランダム名 + 衝突時リトライでファイルを作るため、
+/// 並列 `cargo test` (同一プロセス・同一 ms) でも本番の並列実行 (別セッション /
+/// サブエージェント) でも名前が構造的に衝突しない (順位 230 flaky 修正)。
+fn write_body_tempfile(dir: &Path, content: &str) -> std::io::Result<TempPath> {
+    let path = tempfile::Builder::new()
+        .prefix("gh-pr-body-")
+        .suffix(".md")
+        .tempfile_in(dir)?
+        .into_temp_path();
+    std::fs::write(&path, content)?;
+    Ok(path)
 }
 
 /// --body 引数に改行が含まれる場合、一時ファイルに書き出して --body-file に差し替える。
-fn convert_body_to_file(args: &[String]) -> (Vec<String>, Option<TempFile>) {
+///
+/// `temp_dir` は生成先ディレクトリ。本番は `std::env::temp_dir()` を渡し、テストは
+/// 各 test 専用の `tempfile::tempdir()` を注入して本番 namespace から分離する。
+fn convert_body_to_file(args: &[String], temp_dir: &Path) -> (Vec<String>, Option<TempPath>) {
     let mut result = Vec::with_capacity(args.len());
     let mut i = 0;
-    let mut temp_guard: Option<TempFile> = None;
+    let mut temp_guard: Option<TempPath> = None;
 
     while i < args.len() {
         if args[i] == "--body" && i + 1 < args.len() {
             let body = &args[i + 1];
             if body.contains('\n') || body.contains("\\n") {
-                let filename = format!(
-                    "gh-pr-body-{}-{}.md",
-                    std::process::id(),
-                    std::time::SystemTime::now()
-                        .duration_since(std::time::UNIX_EPOCH)
-                        .unwrap_or_default()
-                        .as_millis()
-                );
-                let path = std::env::temp_dir().join(filename);
                 let resolved = body.replace("\\n", "\n");
-                match std::fs::write(&path, &resolved) {
-                    Ok(()) => {
+                match write_body_tempfile(temp_dir, &resolved) {
+                    Ok(path) => {
                         log_info(&format!(
                             "--body に改行を検出 → --body-file に変換 ({})",
                             path.display()
                         ));
                         result.push("--body-file".to_string());
                         result.push(path.to_string_lossy().to_string());
-                        temp_guard = Some(TempFile(path));
+                        temp_guard = Some(path);
                     }
                     Err(e) => {
                         log_info(&format!(
@@ -203,14 +204,14 @@ pub(crate) fn run_create_pr(gh_args: &[String]) -> i32 {
 
 fn write_early_reset_state() {
     let early_state = PrMonitorState::new(None, None, utc_now_iso8601());
-    if let Err(e) = write_state(&early_state) {
+    if let Err(e) = write_state_to(&state_file_path(), &early_state) {
         log_info(&format!("[state] 早期 reset 書き込み失敗 (継続): {}", e));
     }
 }
 
-fn prepare_gh_pr_create_args(gh_args: &[String]) -> (Vec<String>, Option<TempFile>) {
+fn prepare_gh_pr_create_args(gh_args: &[String]) -> (Vec<String>, Option<TempPath>) {
     let reassembled = reassemble_split_body(gh_args);
-    let (mut final_args, body_tempfile) = convert_body_to_file(&reassembled);
+    let (mut final_args, body_tempfile) = convert_body_to_file(&reassembled, &std::env::temp_dir());
 
     if !has_head_flag(&final_args) {
         let bookmarks = get_jj_bookmarks();
@@ -380,46 +381,50 @@ mod tests {
 
     #[test]
     fn body_without_newline_unchanged() {
+        let dir = tempfile::tempdir().unwrap();
         let args = vec![
             "--title".into(),
             "test".into(),
             "--body".into(),
             "simple body".into(),
         ];
-        let (result, temp) = convert_body_to_file(&args);
+        let (result, temp) = convert_body_to_file(&args, dir.path());
         assert_eq!(result, args);
         assert!(temp.is_none());
     }
 
     #[test]
     fn body_with_literal_newline_converted() {
+        let dir = tempfile::tempdir().unwrap();
         let args = vec![
             "--title".into(),
             "test".into(),
             "--body".into(),
             "line1\\nline2".into(),
         ];
-        let (result, temp) = convert_body_to_file(&args);
+        let (result, temp) = convert_body_to_file(&args, dir.path());
         assert_eq!(result[0], "--title");
         assert_eq!(result[1], "test");
         assert_eq!(result[2], "--body-file");
         assert!(temp.is_some());
-        let content = std::fs::read_to_string(&temp.as_ref().unwrap().0).unwrap();
+        let content = std::fs::read_to_string(temp.as_ref().unwrap()).unwrap();
         assert!(content.contains("line1\nline2"));
     }
 
     #[test]
     fn body_with_real_newline_converted() {
+        let dir = tempfile::tempdir().unwrap();
         let args = vec!["--body".into(), "line1\nline2".into()];
-        let (result, temp) = convert_body_to_file(&args);
+        let (result, temp) = convert_body_to_file(&args, dir.path());
         assert_eq!(result[0], "--body-file");
         assert!(temp.is_some());
     }
 
     #[test]
     fn no_body_arg_unchanged() {
+        let dir = tempfile::tempdir().unwrap();
         let args = vec!["--title".into(), "test".into()];
-        let (result, temp) = convert_body_to_file(&args);
+        let (result, temp) = convert_body_to_file(&args, dir.path());
         assert_eq!(result, args);
         assert!(temp.is_none());
     }

src/cli-pr-monitor/src/stages/monitor.rs

@@ -1,3 +1,5 @@
+use std::path::Path;
+
 use crate::config::load_config;
 use crate::fix_commit::{create_fix_commit, FixCommitState};
 use crate::lock::{acquire as acquire_lock, LockResult};
@@ -6,7 +8,7 @@ use crate::stages::collect::collect_findings;
 use crate::stages::poll::run_poll_loop;
 use crate::stages::repush::execute_repush_flow;
 use crate::stages::takt::run_takt;
-use crate::state::{read_state, write_state, PrMonitorState};
+use crate::state::{read_state_from, state_file_path, write_state_to, PrMonitorState};
 use crate::util::{get_pr_info, utc_now_iso8601, PrInfo};
 
 // ─── 監視開始 (sequential chain) ───
@@ -95,7 +97,7 @@ fn init_or_resume_state(pr_info: &PrInfo, is_wakeup: bool, pr_label: &str) {
         pr_info.push_time.clone().unwrap_or_else(utc_now_iso8601),
     );
     init_state.fix_push_time = pr_info.fix_push_time.clone();
-    if let Err(e) = write_state(&init_state) {
+    if let Err(e) = write_state_to(&state_file_path(), &init_state) {
         log_info(&format!("[state] 初期化書き込み失敗 (継続): {}", e));
     }
 }
@@ -206,7 +208,8 @@ pub(crate) fn run_monitor_only() -> i32 {
             resume_push_time
         ));
         pr_info.push_time = Some(resume_push_time.clone());
-        pr_info.fix_push_time = resume_fix_push_time_or_started_at(&resume_push_time);
+        pr_info.fix_push_time =
+            resume_fix_push_time_or_started_at(&resume_push_time, &state_file_path());
         start_monitoring_wakeup(&pr_info)
     } else {
         let now = utc_now_iso8601();
@@ -218,8 +221,11 @@ pub(crate) fn run_monitor_only() -> i32 {
 
 /// 順位 141: wakeup resume 経路で state から `fix_push_time` を取り出す。
 /// legacy state (本フィールド未設定) では `started_at` に fallback して挙動を維持する。
-fn resume_fix_push_time_or_started_at(started_at_fallback: &str) -> Option<String> {
-    read_state()
+fn resume_fix_push_time_or_started_at(
+    started_at_fallback: &str,
+    state_path: &Path,
+) -> Option<String> {
+    read_state_from(state_path)
         .and_then(|s| s.fix_push_time)
         .or_else(|| Some(started_at_fallback.to_string()))
 }
@@ -230,7 +236,7 @@ fn resume_fix_push_time_or_started_at(started_at_fallback: &str) -> Option<Strin
 /// CR Major #1 fix (Bb-2 PR #114 review): 同一 PR でも新 commit が push されれば head_commit
 /// が変わるため、stored vs current head 一致も check する。head 不一致なら fresh push 扱い。
 fn detect_wakeup_resume(pr_info: &PrInfo) -> Option<String> {
-    let state = read_state()?;
+    let state = read_state_from(&state_file_path())?;
     let now_unix = std::time::SystemTime::now()
         .duration_since(std::time::UNIX_EPOCH)
         .map(|d| d.as_secs() as i64)
@@ -581,27 +587,16 @@ mod tests {
         assert_eq!(compute_verdict(&r), VERDICT_NO_PROBLEMS);
     }
 
-    /// PR_MONITOR_STATE_FILE_OVERRIDE は process-global env var のため、
-    /// override 設定 / 解除を test 並行実行で race させない serial guard。
-    fn env_override_lock() -> std::sync::MutexGuard<'static, ()> {
-        use std::sync::{Mutex, OnceLock};
-        static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
-        LOCK.get_or_init(|| Mutex::new(())).lock().unwrap()
-    }
-
     /// 順位 141: `resume_fix_push_time_or_started_at` Case A —
     /// state に `fix_push_time` が設定済みの場合、fallback の `started_at` ではなく
     /// state の値が返されることを検証する。
     #[test]
     fn resume_returns_fix_push_time_from_state_when_set() {
-        let _guard = env_override_lock();
         let tmp = tempfile::NamedTempFile::new().unwrap();
         let mut s = PrMonitorState::new(Some(1), None, "t".into());
         s.fix_push_time = Some("2026-05-22T06:06:00Z".into());
         std::fs::write(tmp.path(), serde_json::to_string(&s).unwrap()).unwrap();
-        std::env::set_var("PR_MONITOR_STATE_FILE_OVERRIDE", tmp.path());
-        let result = resume_fix_push_time_or_started_at("2026-05-22T06:00:00Z");
-        std::env::remove_var("PR_MONITOR_STATE_FILE_OVERRIDE");
+        let result = resume_fix_push_time_or_started_at("2026-05-22T06:00:00Z", tmp.path());
         assert_eq!(
             result.as_deref(),
             Some("2026-05-22T06:06:00Z"),