Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions docs/adr/adr-030-deterministic-post-merge-feedback.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 を `<timestamp>-<sanitized-task-label>` 形式で生成する (workflow 名ではなく **task label** を suffix に使う)。task label の sanitization は概ね「lowercase + 空白/特殊文字 → `-`」だが内部仕様で、Rust 側で再現するのは脆い。

#### 採用する規約

**task label は workflow 名を必ず prefix として含む `"<workflow-name> [<context>]"` 形式とする。**

| workflow | task label の例 | 結果の dir suffix |
|---|---|---|
| `pre-push-review` | `"pre-push-review"` | `<ts>-pre-push-review` |
| `post-pr-review` | `"post-pr-review"` | `<ts>-post-pr-review` |
| `post-merge-feedback` | `"post-merge-feedback for #77"` | `<ts>-post-merge-feedback-for-77` |

すべての run dir 名に `-<workflow>` という連続部分文字列が必ず現れる。Rust 側のマッチングは `name.contains(&format!("-{}", workflow))` の 1 行で完結し、context suffix の有無に関わらず一律にマッチする。

#### 制約

workflow 名同士が部分文字列関係になってはいけない。「部分文字列関係」とは `-<workflow-A>` が `-<workflow-B>...` の中に含まれること、すなわち `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` は `<ts>-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)
Expand Down
4 changes: 3 additions & 1 deletion pr-monitor-config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 命名 `<ts>-<sanitized-task>` から workflow 種別を一意に逆引きするため。
task = "post-pr-review"
extra_args = ["--pipeline", "--skip-git"]

[fix]
Expand Down
134 changes: 127 additions & 7 deletions src/cli-merge-pipeline/src/feedback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// として含む `"<workflow-name> [<context>]"` 形式とする。これにより takt の sanitization 後の
/// dir 名 (`<timestamp>-<sanitized-task-label>`) が必ず workflow 名を含み、`find_latest_run_dir`
/// が `name.contains("-<workflow>")` でマッチできる。
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;
Expand Down Expand Up @@ -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<PathBuf> {
/// `.takt/runs/` 配下で `workflow` 名を含む最新ディレクトリ (lex-sort 末尾) を返す。
///
/// takt の run dir は `<timestamp>-<sanitized-task-label>` 形式。task label が
/// ADR-030 §task labeling convention に従い workflow 名を prefix として含む場合、
/// dir 名にも `-<workflow>` という連続部分文字列が必ず現れる:
/// - task = `"<workflow>"` → dir = `<ts>-<workflow>`
/// - task = `"<workflow> for #<pr>"` → dir = `<ts>-<workflow>-for-<pr>`
///
/// どちらの形にも `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<PathBuf> {
let needle = format!("-{}", workflow);
let mut candidates: Vec<PathBuf> = 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
Expand All @@ -258,10 +276,10 @@ fn find_latest_run_dir(runs_dir: &Path, suffix: &str) -> Option<PathBuf> {
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<PathBuf> {
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)
Expand Down Expand Up @@ -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/<pr>.md` にコピーする。
pub fn copy_feedback_report(repo_root: &Path, pr_number: u64) -> Result<PathBuf, String> {
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");
Expand Down Expand Up @@ -653,6 +671,108 @@ mod tests {

let _ = fs::remove_dir_all(&root);
}

#[test]
fn find_latest_run_dir_matches_workflow_name_only() {
// task = "<workflow>" のケース: dir = "<ts>-<workflow>"
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 = "<workflow> for #<pr>" のケース: dir = "<ts>-<workflow>-for-<pr>"
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());
}
}

/// 全工程を実行する高水準エントリポイント。
Expand Down
4 changes: 2 additions & 2 deletions src/cli-pr-monitor/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
}

Expand Down
3 changes: 2 additions & 1 deletion templates/pr-monitor-config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]