diff --git a/docs/adr/adr-030-deterministic-post-merge-feedback.md b/docs/adr/adr-030-deterministic-post-merge-feedback.md index 3d608f75..ad4d3cdd 100644 --- a/docs/adr/adr-030-deterministic-post-merge-feedback.md +++ b/docs/adr/adr-030-deterministic-post-merge-feedback.md @@ -156,6 +156,44 @@ skill ベースで運用していた analyze-pr / post-merge-feedback Phase 4 `pnpm merge-pr` の所要時間が takt workflow 実行分 (数分) 増加する。ユーザー判断 (作業計画策定時に合意) として **数分の追加レイテンシは許容**。`pnpm merge-pr` は同期実行で待つ前提とする ([ADR-016](adr-016-long-running-command-strategy.md) の長時間コマンド戦略に該当)。 +### task labeling convention (Phase B dogfood で確立) + +PR #77 の dogfood で、Rust 側の `find_latest_run_dir` が takt の run dir を見つけられず `.failed` marker が誤って書かれる事象が発生した。原因は task label と run dir の命名規則の不整合。 + +#### takt の run dir 命名 + +takt は run dir を `-` 形式で生成する (workflow 名ではなく **task label** を suffix に使う)。task label の sanitization は概ね「lowercase + 空白/特殊文字 → `-`」だが内部仕様で、Rust 側で再現するのは脆い。 + +#### 採用する規約 + +**task label は workflow 名を必ず prefix として含む `" []"` 形式とする。** + +| workflow | task label の例 | 結果の dir suffix | +|---|---|---| +| `pre-push-review` | `"pre-push-review"` | `-pre-push-review` | +| `post-pr-review` | `"post-pr-review"` | `-post-pr-review` | +| `post-merge-feedback` | `"post-merge-feedback for #77"` | `-post-merge-feedback-for-77` | + +すべての run dir 名に `-` という連続部分文字列が必ず現れる。Rust 側のマッチングは `name.contains(&format!("-{}", workflow))` の 1 行で完結し、context suffix の有無に関わらず一律にマッチする。 + +#### 制約 + +workflow 名同士が部分文字列関係になってはいけない。「部分文字列関係」とは `-` が `-...` の中に含まれること、すなわち `name.contains(&format!("-{}", workflow))` で取り違えが起きる関係を指す (実装は [`feedback.rs`](../../src/cli-merge-pipeline/src/feedback.rs) の `find_latest_run_dir`)。 + +例: + +- **NG**: `merge` ⇄ `post-merge-feedback` — workflow=`merge` の needle `-merge` は `-post-merge-feedback-...` の中央に出現するため誤マッチ +- **NG**: `post-merge` ⇄ `post-merge-feedback` — 同様に `-post-merge` が dir 末端に出現 +- **OK**: `build` ⇄ `post-merge-feedback` — `-build` が他の dir 名のどこにも現れない + +現存 3 workflow (`pre-push-review` / `post-pr-review` / `post-merge-feedback`) は問題なし。新 workflow 追加時はこの制約を確認する。 + +#### 採用根拠 + +- **invariant に応じた選択**: 「最新 run dir = 自分のもの」という同期実行 invariant に依存する代替案 (Option C) よりも、命名規約による直接対応のほうが並行 takt 実行・将来の非同期化に対して頑健 +- **既存 (`pre-push-review`) との後方互換**: pre-push-review の現行 task はすでに workflow 名と一致するため、規約を後付けで導入しても何も変える必要がない +- **post-pr-review の latent bug を予防**: 旧 task `"analyze PR review comments"` は workflow 名と無関係で、「post-pr-review の最新 run を Rust から探す」コードを書けば即破綻する。本 ADR で揃える + ### Supersede 範囲 #### ADR-014 (full supersede) diff --git a/pr-monitor-config.toml b/pr-monitor-config.toml index 4c886753..dd8c0cbb 100644 --- a/pr-monitor-config.toml +++ b/pr-monitor-config.toml @@ -13,7 +13,9 @@ check_coderabbit = true [takt] workflow = "post-pr-review" -task = "analyze PR review comments" +# task は workflow 名を prefix として含める (ADR-030 §task labeling convention)。 +# takt の run dir 命名 `-` から workflow 種別を一意に逆引きするため。 +task = "post-pr-review" extra_args = ["--pipeline", "--skip-git"] [fix] diff --git a/src/cli-merge-pipeline/src/feedback.rs b/src/cli-merge-pipeline/src/feedback.rs index 23f3b4cb..4173c1f9 100644 --- a/src/cli-merge-pipeline/src/feedback.rs +++ b/src/cli-merge-pipeline/src/feedback.rs @@ -26,8 +26,13 @@ use std::process::{Command, Stdio}; use std::time::Duration; /// takt workflow 名 / task ラベル +/// +/// 命名規約 (ADR-030 §task labeling convention): task label は workflow 名を必ず prefix +/// として含む `" []"` 形式とする。これにより takt の sanitization 後の +/// dir 名 (`-`) が必ず workflow 名を含み、`find_latest_run_dir` +/// が `name.contains("-")` でマッチできる。 const TAKT_WORKFLOW: &str = "post-merge-feedback"; -const TAKT_TASK_PREFIX: &str = "post-merge feedback for #"; +const TAKT_TASK_PREFIX: &str = "post-merge-feedback for #"; /// takt 実行のデフォルトタイムアウト (10 分) pub const TAKT_TIMEOUT_SECS: u64 = 600; @@ -239,15 +244,28 @@ fn entry_matches_filter(line: &str, range: &PrTimeRange) -> bool { ts >= lower && ts <= upper } -/// `.takt/runs/` 配下で `suffix` に一致する最新ディレクトリ (lex-sort 末尾) を返す。 -fn find_latest_run_dir(runs_dir: &Path, suffix: &str) -> Option { +/// `.takt/runs/` 配下で `workflow` 名を含む最新ディレクトリ (lex-sort 末尾) を返す。 +/// +/// takt の run dir は `-` 形式。task label が +/// ADR-030 §task labeling convention に従い workflow 名を prefix として含む場合、 +/// dir 名にも `-` という連続部分文字列が必ず現れる: +/// - task = `""` → dir = `-` +/// - task = `" for #"` → dir = `--for-` +/// +/// どちらの形にも `name.contains(&format!("-{}", workflow))` で一律にマッチする。 +/// `ends_with` を避けることで、context suffix 付きの形にも対応する。 +/// +/// 制約: workflow 名同士が部分文字列関係になってはいけない (例: `merge` と +/// `post-merge-feedback` は OK、`post-merge` と `post-merge-feedback` は NG)。 +fn find_latest_run_dir(runs_dir: &Path, workflow: &str) -> Option { + let needle = format!("-{}", workflow); let mut candidates: Vec = fs::read_dir(runs_dir) .ok()? .flatten() .filter_map(|e| { let path = e.path(); let name = path.file_name()?.to_string_lossy().into_owned(); - if name.ends_with(suffix) { + if name.contains(&needle) { Some(path) } else { None @@ -258,10 +276,10 @@ fn find_latest_run_dir(runs_dir: &Path, suffix: &str) -> Option { candidates.into_iter().next_back() } -/// `.takt/runs/*-pre-push-review/reports/` のうち最新 (lex-sort 末尾) を返す。 +/// pre-push-review workflow の最新 reports ディレクトリを返す。 pub fn find_latest_prepush_reports_dir(repo_root: &Path) -> Option { let runs_dir = repo_root.join(".takt").join("runs"); - let latest = find_latest_run_dir(&runs_dir, "-pre-push-review")?; + let latest = find_latest_run_dir(&runs_dir, "pre-push-review")?; let reports = latest.join("reports"); if reports.is_dir() { Some(reports) @@ -337,7 +355,7 @@ pub fn run_takt_workflow(repo_root: &Path, pr_number: u64, timeout_secs: u64) -> /// takt 完了後、最新 run dir の `feedback-report.md` を `.claude/feedback-reports/.md` にコピーする。 pub fn copy_feedback_report(repo_root: &Path, pr_number: u64) -> Result { let runs_dir = repo_root.join(".takt").join("runs"); - let latest = find_latest_run_dir(&runs_dir, &format!("-{}", TAKT_WORKFLOW)) + let latest = find_latest_run_dir(&runs_dir, TAKT_WORKFLOW) .ok_or("post-merge-feedback の run dir が見つかりません")?; let source = latest.join("reports").join("feedback-report.md"); @@ -653,6 +671,108 @@ mod tests { let _ = fs::remove_dir_all(&root); } + + #[test] + fn find_latest_run_dir_matches_workflow_name_only() { + // task = "" のケース: dir = "-" + let root = std::env::temp_dir().join(format!( + "feedback-find-name-only-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.subsec_nanos()) + .unwrap_or(0), + )); + let runs = root.join(".takt").join("runs"); + fs::create_dir_all(runs.join("20260425-100000-post-merge-feedback")).unwrap(); + fs::create_dir_all(runs.join("20260425-110000-other-workflow")).unwrap(); + + let latest = find_latest_run_dir(&runs, "post-merge-feedback").unwrap(); + assert!(latest + .to_string_lossy() + .contains("20260425-100000-post-merge-feedback")); + + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn find_latest_run_dir_matches_workflow_with_context_suffix() { + // task = " for #" のケース: dir = "--for-" + let root = std::env::temp_dir().join(format!( + "feedback-find-with-ctx-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.subsec_nanos()) + .unwrap_or(0), + )); + let runs = root.join(".takt").join("runs"); + fs::create_dir_all(runs.join("20260425-100000-post-merge-feedback-for-77")).unwrap(); + fs::create_dir_all(runs.join("20260425-090000-pre-push-review")).unwrap(); + + let latest = find_latest_run_dir(&runs, "post-merge-feedback").unwrap(); + assert!(latest + .to_string_lossy() + .contains("20260425-100000-post-merge-feedback-for-77")); + + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn find_latest_run_dir_picks_lex_max_across_mixed_forms() { + // task = workflow 名のみ と task = workflow + suffix の混在で、最新を正しく拾う + let root = std::env::temp_dir().join(format!( + "feedback-find-mixed-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.subsec_nanos()) + .unwrap_or(0), + )); + let runs = root.join(".takt").join("runs"); + fs::create_dir_all(runs.join("20260425-090000-post-merge-feedback")).unwrap(); + fs::create_dir_all(runs.join("20260425-100000-post-merge-feedback-for-77")).unwrap(); + fs::create_dir_all(runs.join("20260425-080000-post-merge-feedback-for-50")).unwrap(); + + let latest = find_latest_run_dir(&runs, "post-merge-feedback").unwrap(); + assert!(latest + .to_string_lossy() + .contains("20260425-100000-post-merge-feedback-for-77")); + + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn find_latest_run_dir_returns_none_when_no_match() { + let root = std::env::temp_dir().join(format!( + "feedback-find-none-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.subsec_nanos()) + .unwrap_or(0), + )); + let runs = root.join(".takt").join("runs"); + fs::create_dir_all(runs.join("20260425-090000-pre-push-review")).unwrap(); + fs::create_dir_all(runs.join("20260425-100000-analyze-pr-review-comments")).unwrap(); + + assert!(find_latest_run_dir(&runs, "post-merge-feedback").is_none()); + + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn find_latest_run_dir_returns_none_when_dir_missing() { + let nonexistent = std::env::temp_dir().join(format!( + "feedback-nonexistent-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.subsec_nanos()) + .unwrap_or(0), + )); + assert!(find_latest_run_dir(&nonexistent, "post-merge-feedback").is_none()); + } } /// 全工程を実行する高水準エントリポイント。 diff --git a/src/cli-pr-monitor/src/config.rs b/src/cli-pr-monitor/src/config.rs index 76d735ea..510e294d 100644 --- a/src/cli-pr-monitor/src/config.rs +++ b/src/cli-pr-monitor/src/config.rs @@ -162,7 +162,7 @@ check_coderabbit = false [takt] workflow = "post-pr-review" -task = "analyze PR review comments" +task = "post-pr-review" extra_args = ["--pipeline", "--skip-git"] "#; let config: Config = toml::from_str(toml_str).unwrap(); @@ -174,7 +174,7 @@ extra_args = ["--pipeline", "--skip-git"] let takt = config.takt.unwrap(); assert_eq!(takt.workflow, "post-pr-review"); - assert_eq!(takt.task, "analyze PR review comments"); + assert_eq!(takt.task, "post-pr-review"); assert_eq!(takt.extra_args.as_ref().unwrap().len(), 2); } diff --git a/templates/pr-monitor-config.toml b/templates/pr-monitor-config.toml index 7cbcb705..4a57beff 100644 --- a/templates/pr-monitor-config.toml +++ b/templates/pr-monitor-config.toml @@ -15,5 +15,6 @@ check_coderabbit = true # takt がインストールされていない場合はコメントアウトすること。 # [takt] # workflow = "post-pr-review" -# task = "analyze PR review comments" +# # task は workflow 名を prefix として含める (ADR-030 §task labeling convention)。 +# task = "post-pr-review" # extra_args = ["--pipeline", "--skip-git"]