From 52b70e1faa77a2da26ee9a5e48714551ae17a6d7 Mon Sep 17 00:00:00 2001 From: aloekun Date: Sun, 26 Apr 2026 00:57:35 +0900 Subject: [PATCH 1/2] =?UTF-8?q?fix(post-merge-feedback):=20task=20labeling?= =?UTF-8?q?=20=E8=A6=8F=E7=B4=84=E5=B0=8E=E5=85=A5=E3=81=A7=20run=20dir=20?= =?UTF-8?q?=E6=A4=9C=E7=B4=A2=E3=82=92=E6=B1=BA=E5=AE=9A=E8=AB=96=E5=8C=96?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #77 の merge dogfood で発覚した `copy_feedback_report` の bug を修正する。 takt の run dir 命名は `-` 形式で workflow 名そのものは 含まないため、`ends_with("-")` での suffix match が context suffix (例: `-for-77`) 付きの dir に対して fail していた。 対策: 「task label は workflow 名を必ず prefix として含む」という命名規約を ADR-030 に追記し、Rust 側のマッチングを `name.contains("-")` の 1 ルールに統一。context suffix の有無に関わらず一律にマッチする。 主な変更: - src/cli-merge-pipeline/src/feedback.rs: find_latest_run_dir を contains match に変更、TAKT_TASK_PREFIX を workflow 名と sanitization 後一致する形に 修正、新規テスト 5 件追加 - src/cli-pr-monitor/src/config.rs + pr-monitor-config.toml + templates/pr-monitor-config.toml: post-pr-review の task を "analyze PR review comments" → "post-pr-review" に揃え、latent bug を予防 - docs/adr/adr-030-deterministic-post-merge-feedback.md: §task labeling convention セクション追加。規約・制約 (workflow 名同士の部分文字列禁止)・ 採用根拠を明記 Phase B 内の bugfix として完結し、Phase C には bug を持ち越さない。 --- ...r-030-deterministic-post-merge-feedback.md | 30 ++++ pr-monitor-config.toml | 4 +- src/cli-merge-pipeline/src/feedback.rs | 134 +++++++++++++++++- src/cli-pr-monitor/src/config.rs | 4 +- templates/pr-monitor-config.toml | 3 +- 5 files changed, 164 insertions(+), 11 deletions(-) diff --git a/docs/adr/adr-030-deterministic-post-merge-feedback.md b/docs/adr/adr-030-deterministic-post-merge-feedback.md index 3d608f75..37d45bbf 100644 --- a/docs/adr/adr-030-deterministic-post-merge-feedback.md +++ b/docs/adr/adr-030-deterministic-post-merge-feedback.md @@ -156,6 +156,36 @@ 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 名同士が部分文字列関係になってはいけない (例: `merge` と `post-merge-feedback` は OK、`post-merge` と `post-merge-feedback` は NG)。現存 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"] From 423b36eadec83c77384a55dc77c2674cff814726 Mon Sep 17 00:00:00 2001 From: aloekun Date: Sun, 26 Apr 2026 02:49:40 +0900 Subject: [PATCH 2/2] =?UTF-8?q?docs(adr-030):=20=E5=88=B6=E7=B4=84?= =?UTF-8?q?=E4=BE=8B=E3=82=92=E5=AE=9F=E8=A3=85=E3=81=A8=E4=B8=80=E8=87=B4?= =?UTF-8?q?=E3=81=95=E3=81=9B=E3=80=81partial=20substring=20=E3=81=AE?= =?UTF-8?q?=E5=8F=96=E3=82=8A=E9=81=95=E3=81=88=E3=82=92=E6=98=8E=E7=A4=BA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (PR #78) が指摘した不整合を修正。 旧記述: "merge と post-merge-feedback は OK" は誤り。 - find_latest_run_dir は name.contains("-{workflow}") で照合 - workflow=merge の needle "-merge" は "-post-merge-feedback-..." に 内部出現するため誤マッチが起きる - したがって merge も post-merge と同様 NG 修正内容: - 「部分文字列関係」の定義を実装メソッド (name.contains) と紐付けて明記 - NG 例を 2 件 (merge, post-merge) と OK 例 (build) を併記 - 実装ファイル feedback.rs へのリンクを追加 --- docs/adr/adr-030-deterministic-post-merge-feedback.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/adr/adr-030-deterministic-post-merge-feedback.md b/docs/adr/adr-030-deterministic-post-merge-feedback.md index 37d45bbf..ad4d3cdd 100644 --- a/docs/adr/adr-030-deterministic-post-merge-feedback.md +++ b/docs/adr/adr-030-deterministic-post-merge-feedback.md @@ -178,7 +178,15 @@ takt は run dir を `-` 形式で生成する #### 制約 -workflow 名同士が部分文字列関係になってはいけない (例: `merge` と `post-merge-feedback` は OK、`post-merge` と `post-merge-feedback` は NG)。現存 3 workflow (`pre-push-review` / `post-pr-review` / `post-merge-feedback`) は問題なし。新 workflow 追加時はこの制約を確認する。 +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 追加時はこの制約を確認する。 #### 採用根拠