From 3117cb078a2e17f9f2eed2c60d1687e4bd19927a Mon Sep 17 00:00:00 2001 From: aloekun Date: Sat, 18 Apr 2026 18:36:58 +0900 Subject: [PATCH] =?UTF-8?q?feat(cli-merge-pipeline):=20PR=20detection=20?= =?UTF-8?q?=E3=82=92=20@-/@--=20=E3=81=BE=E3=81=A7=E6=8B=A1=E5=BC=B5=20+?= =?UTF-8?q?=20trunk=20filter=20=E8=BF=BD=E5=8A=A0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit jj new 直後の「@ 空 / bookmark は @- 上」構成 (PR #53 実測) で PR detection が空振りする問題を解消。 - BOOKMARK_SEARCH_REVSETS = ["@", "@-", "@--"] で近い順に探索 - trunk bookmark (master/main/trunk/develop) を除外し、fresh checkout で @- が master を指す状態での false hit を防止 - get_jj_bookmarks を parse / query / select の 3 層に分割し、 select_from_revsets をクロージャ注入で単体テスト可能に docs/todo.md: task 7 完了削除 + cli-pr-monitor 側の水平展開タスク新設 refs: ADR-013 --- docs/todo.md | 54 ++++++++ src/cli-merge-pipeline/src/main.rs | 199 +++++++++++++++++++++++++++-- 2 files changed, 242 insertions(+), 11 deletions(-) diff --git a/docs/todo.md b/docs/todo.md index d2d4d7fa..bef8a310 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -125,6 +125,60 @@ - ADR-022 (責務分離): takt は commit 操作禁止、Rust 側が担当 - PR #44 の事故事例: 元 description の破壊で得た教訓 +### 5. cli-pr-monitor の auto re-push に bookmark 自動前進を移植 + +- **やろうとしたこと**: takt 自動修正後の auto re-push で「修正コミットができても bookmark が動かず remote に届かない」問題を解消。cli-push-runner には PR #50 で `push_jj_bookmark.rs` の advance ロジックが入っているが、cli-pr-monitor の `run_push` は `jj new` → `jj git push` だけで bookmark を進めない +- **現在地**: 設計確定、未実装。PR #53 で症状を実測 (CodeRabbit 修正が local commit `466aff1d` に入ったが bookmark が動かず remote 未反映、手動で `jj bookmark set` + `jj git push --bookmark` が必要だった) +- **実装内容**: + - [ ] `src/cli-pr-monitor/src/stages/push.rs:run_push` の `jj git push` 直前に bookmark advance を挿入 + - [ ] cli-push-runner の `push_jj_bookmark::advance_jj_bookmarks` を共有化するか、まず port (copy) するかを決定 + - [ ] unit テスト: 既存の `decide_repush` テスト群と同じスタイルで bookmark advance の挙動を検証 + - [ ] E2E 検証: CodeRabbit Major ありの PR で takt 修正が remote まで自動到達することを確認 +- **詰まっている箇所**: + - **共通化方針**: ADR-024 (共通 jj helper crate、試験運用) の延長線で `jj-helpers` クレート化するのが理想だが、まず port から着手して機能等価を確認した方が安全 + - **task 4 (コミット分離) との順序**: task 4 を先に実装すると takt fix が「明示的な子コミット」になるため bookmark advance のターゲットが安定する。task 5 → 4 の順か、4 と同時に対応するかを設計確定時に判断 +- **参照 ADR / PR**: + - PR #50 (cli-push-runner の bookmark fallback) + - ADR-024 (共通 jj helper、試験運用) + - 関連: task 4 (コミット分離) + +### 6. post-pr-review workflow の verdict に push 反映確認を追加 + +- **やろうとしたこと**: post-pr-review workflow が local file の状態だけを見て `approved` を判定する設計のため、auto re-push が失敗していても `approved` になる gap を埋める。PR #53 で「local 修正済み + bookmark 未前進 → workflow approved」の食い違いを実測 +- **現在地**: 設計段階、未着手。task 5 (問題 A) の defense-in-depth 的位置付け +- **実装内容案**: + - [ ] workflow の analyze (or 終了前) ステップで `gh api` を叩いて remote の最新 commit SHA を取得し、local の fix commit SHA と比較 + - [ ] 一致しなければ `verdict = action_required` にダウングレードし「remote 未反映」のメッセージを出す +- **詰まっている箇所**: + - **task 5 実装後の優先度再評価**: task 5 が入れば「auto re-push が失敗しない限り approved は妥当」となるため、task 6 の優先度は大きく下がる。実装可否は task 5 完了後に判断 + - **takt workflow の YAML から外部コマンド呼び出しが可能か**: facets instruction 内でシェルが呼べるかは要調査 (ADR-019/020 の facets 設計内) +- **参照 ADR**: + - ADR-018 (post-pr-monitor takt 化) + - ADR-019 (CodeRabbit レビュー運用ハイブリッド) + - 関連: task 5 (bookmark auto-advance) + +### 7. cli-pr-monitor の bookmark 検出を `@-..@--` まで拡張 (cli-merge-pipeline の水平展開) + +- **やろうとしたこと**: [src/cli-pr-monitor/src/util.rs:86](src/cli-pr-monitor/src/util.rs#L86) の `get_jj_bookmarks` は revset `@` のみで bookmark を探しており、cli-merge-pipeline で先に解消した task 7 (旧) と同じ「`@` 空 / bookmark が `@-` 上」問題を抱えている。`pnpm create-pr` や PR 検索フェーズで同じ空振りが起きる可能性があるため、横展開して整合させる +- **現在地**: 設計確定、未着手 (cli-merge-pipeline 側の PR で実装パターンが確定済み) +- **実装内容**: + - [ ] [src/cli-pr-monitor/src/util.rs](src/cli-pr-monitor/src/util.rs) の `get_jj_bookmarks` を cli-merge-pipeline と同じ 3 層 (parse / query / select_from_revsets) に分割 + - [ ] `BOOKMARK_SEARCH_REVSETS = &["@", "@-", "@--"]` + `TRUNK_BOOKMARKS` filter を移植 + - [ ] unit テスト: parse と優先順位を cli-merge-pipeline のテストと同等に追加 + - [ ] E2E: 空 `@` 状態で `pnpm create-pr` の Strategy B が成功することを確認 +- **詰まっている箇所**: + - **共通化するかコピーするか**: ADR-024 (共通 jj helpers crate、試験運用) の延長で `jj-helpers` クレート化するのが理想だが、まず cli-pr-monitor への port (コピー) で機能等価を確認するのが安全。共通化は 2 個目の port 完了後に判断 + - **create_pr.rs での利用箇所**: [src/cli-pr-monitor/src/stages/create_pr.rs:183](src/cli-pr-monitor/src/stages/create_pr.rs#L183) でも `get_jj_bookmarks` を使っており、`--head` 引数に先頭の bookmark を渡している。trunk filter を入れた結果「先頭が feature bookmark」保証が強まるので挙動は改善方向だが、回帰テストで確認する +- **参照**: + - 先行実装: cli-merge-pipeline の同等対応 (task 7 旧、PR で完了済み) + - ADR-013 (cli-merge-pipeline), ADR-024 (共通 jj helpers、試験運用) + +### 8. 雑務: 過去の delete-pending bookmark cleanup + +- **やろうとしたこと**: `jj git push --tracked` で `Refusing to push deleted bookmark fix/push-allow-new` の警告が出るため、`jj bookmark forget fix/push-allow-new` で消す +- **現在地**: 未対応。push を block しないので緊急性なし +- **詰まっている箇所**: なし + --- ## スコープ外だが将来検討 diff --git a/src/cli-merge-pipeline/src/main.rs b/src/cli-merge-pipeline/src/main.rs index 298edf5c..628b3cff 100644 --- a/src/cli-merge-pipeline/src/main.rs +++ b/src/cli-merge-pipeline/src/main.rs @@ -217,10 +217,64 @@ fn run_gh_logged(args: &[&str]) -> Option { } } -/// 現在の jj change に紐づく全ブックマーク名を取得する +/// Bookmark 検索に使用する revset のリスト (近い順 = 優先順)。 +/// +/// `select_from_revsets` は先頭から順に試し、最初に (trunk 除外後の) bookmark が +/// 見つかった時点で後続の revset を検索しない ("@" で見つかれば "@--" は触らない)。 +/// +/// - `@`: 標準 `git` ブランチ運用、または bookmark が現在のコミット上にある場合 +/// - `@-`: `jj new` で空 `@` を作った直後 (PR #53 で実測) +/// - `@--`: 連続 `jj new` や中間空コミット運用向けのフォールバック +const BOOKMARK_SEARCH_REVSETS: &[&str] = &["@", "@-", "@--"]; + +/// PR 検出から除外する trunk 系 bookmark。 +/// cli-push-runner/push_jj_bookmark.rs と同じリストを採用。 +const TRUNK_BOOKMARKS: &[&str] = &["main", "master", "trunk", "develop"]; + +fn is_trunk_bookmark(name: &str) -> bool { + TRUNK_BOOKMARKS.contains(&name) +} + +/// 現在の jj change 周辺に紐づく全ブックマーク名を取得する。 +/// +/// `BOOKMARK_SEARCH_REVSETS` の順で検索し、最初に非空の結果が得られた revset の +/// bookmark を返す。すべての revset で空なら空 Vec。 fn get_jj_bookmarks() -> Vec { + select_from_revsets(BOOKMARK_SEARCH_REVSETS, query_bookmarks_at) +} + +/// 指定 revset を優先順に試し、最初に非空の bookmark リストを得た revset の結果を返す。 +/// テスト用に `query` をクロージャで注入できる。 +fn select_from_revsets(revsets: &[&str], query: F) -> Vec +where + F: Fn(&str) -> Vec, +{ + for (i, revset) in revsets.iter().enumerate() { + let bookmarks = query(revset); + if !bookmarks.is_empty() { + if i > 0 { + log_info(&format!( + "revset '{}' で bookmark を検出: {:?}", + revset, bookmarks + )); + } + return bookmarks; + } + } + Vec::new() +} + +/// 指定 revset の bookmark 名を `jj log` で取得する (I/O)。 +fn query_bookmarks_at(revset: &str) -> Vec { let output = match Command::new("jj") - .args(["log", "-r", "@", "--no-graph", "-T", "local_bookmarks.map(|b| b.name()).join(\",\")"]) + .args([ + "log", + "-r", + revset, + "--no-graph", + "-T", + "local_bookmarks.map(|b| b.name()).join(\",\") ++ \"\\n\"", + ]) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) .output() @@ -229,7 +283,10 @@ fn get_jj_bookmarks() -> Vec { Ok(o) => { let stderr = String::from_utf8_lossy(&o.stderr).trim().to_string(); if !stderr.is_empty() { - log_info(&format!("jj bookmark 取得失敗: {}", stderr)); + log_info(&format!( + "jj bookmark 取得失敗 (revset={}): {}", + revset, stderr + )); } return Vec::new(); } @@ -239,15 +296,25 @@ fn get_jj_bookmarks() -> Vec { } }; - let s = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if s.is_empty() { - return Vec::new(); - } + parse_bookmark_list_output(&String::from_utf8_lossy(&output.stdout)) +} - s.split(',') - .map(|b| b.trim().to_string()) - .filter(|b| !b.is_empty()) - .collect() +/// `jj log` テンプレート出力 (カンマ区切り × 行) からユニークな bookmark 名を抽出する。 +/// trunk 系 bookmark (master/main/trunk/develop) は PR 検索対象から除外する。 +fn parse_bookmark_list_output(raw: &str) -> Vec { + let mut seen = Vec::new(); + for line in raw.lines() { + for name in line.split(',').map(str::trim).filter(|s| !s.is_empty()) { + if is_trunk_bookmark(name) { + continue; + } + let name = name.to_string(); + if !seen.contains(&name) { + seen.push(name); + } + } + } + seen } /// 現在のブックマークから PR 番号を検出する @@ -596,4 +663,114 @@ step_timeout = 60 fn combine_output_both_empty() { assert_eq!(combine_output("", ""), ""); } + + // ─── bookmark 検出ロジック ─── + + #[test] + fn parse_bookmark_list_output_empty() { + assert!(parse_bookmark_list_output("").is_empty()); + assert!(parse_bookmark_list_output("\n\n").is_empty()); + } + + #[test] + fn parse_bookmark_list_output_single() { + assert_eq!(parse_bookmark_list_output("feat/x\n"), vec!["feat/x"]); + } + + #[test] + fn parse_bookmark_list_output_csv_on_one_line() { + assert_eq!( + parse_bookmark_list_output("feat/a,feat/b\n"), + vec!["feat/a", "feat/b"] + ); + } + + #[test] + fn parse_bookmark_list_output_multiple_lines() { + // @ と @- の両方にヒットした場合 (将来 revset を両方カバーする場合向け) + let raw = "feat/current\nfeat/parent\n"; + assert_eq!( + parse_bookmark_list_output(raw), + vec!["feat/current", "feat/parent"] + ); + } + + #[test] + fn parse_bookmark_list_output_deduplicates() { + let raw = "feat/x,feat/x\nfeat/x\n"; + assert_eq!(parse_bookmark_list_output(raw), vec!["feat/x"]); + } + + #[test] + fn parse_bookmark_list_output_trims_whitespace() { + assert_eq!( + parse_bookmark_list_output(" feat/a , feat/b \n"), + vec!["feat/a", "feat/b"] + ); + } + + #[test] + fn parse_bookmark_list_output_excludes_trunk_bookmarks() { + // fresh checkout で @- が master を指すケース (option B の注意点) + assert!(parse_bookmark_list_output("master\n").is_empty()); + assert_eq!( + parse_bookmark_list_output("master,feat/x\n"), + vec!["feat/x"] + ); + } + + #[test] + fn is_trunk_bookmark_known_names_rejected() { + assert!(is_trunk_bookmark("main")); + assert!(is_trunk_bookmark("master")); + assert!(is_trunk_bookmark("trunk")); + assert!(is_trunk_bookmark("develop")); + assert!(!is_trunk_bookmark("feat/x")); + assert!(!is_trunk_bookmark("main-feature")); + } + + #[test] + fn select_from_revsets_returns_empty_when_all_revsets_empty() { + let result = select_from_revsets(&["@", "@-"], |_| Vec::new()); + assert!(result.is_empty()); + } + + #[test] + fn select_from_revsets_prefers_current_over_parent() { + // @ と @- の両方に bookmark があるとき、近い @ を優先する + let result = select_from_revsets(&["@", "@-"], |r| match r { + "@" => vec!["feat/current".to_string()], + "@-" => vec!["feat/parent".to_string()], + _ => Vec::new(), + }); + assert_eq!(result, vec!["feat/current"]); + } + + #[test] + fn select_from_revsets_falls_back_to_parent_when_current_empty() { + // PR #53 で実測した「@ 空, @- に bookmark」ケース + let result = select_from_revsets(&["@", "@-"], |r| match r { + "@" => Vec::new(), + "@-" => vec!["feat/parent".to_string()], + _ => Vec::new(), + }); + assert_eq!(result, vec!["feat/parent"]); + } + + #[test] + fn select_from_revsets_stops_at_first_hit() { + // 優先度の低い revset は検索されない (副作用が発生しないことを確認) + use std::cell::RefCell; + let calls = RefCell::new(Vec::::new()); + let result = select_from_revsets(&["@", "@-", "@--"], |r| { + calls.borrow_mut().push(r.to_string()); + if r == "@-" { + vec!["feat/hit".to_string()] + } else { + Vec::new() + } + }); + assert_eq!(result, vec!["feat/hit"]); + assert_eq!(*calls.borrow(), vec!["@".to_string(), "@-".to_string()]); + } }