diff --git a/docs/todo.md b/docs/todo.md index 8b23a2ca..19821e81 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -121,7 +121,43 @@ - SessionStart hook は master に実装済み (`src/hooks-session-start/`)。セッション引継ぎ設計は session ID → jsonl transcript 紐付けの ADR が必要 - takt-test-vc での試験運用を先に行い、本プロジェクトに反映 -### 3. Cargo workspace 化 + rust-test template 反映 (PR-β、実装済み) +### 3. cli-pr-monitor の --body 複数行引数切り詰め修正 + +- **やろうとしたこと**: `pnpm create-pr -- --body "複数行テキスト"` で PR body が最初の行だけに切り詰められる問題を修正する +- **現在地**: 未着手。原因特定済み + - [ ] cli-pr-monitor.exe の引数転送ロジックで複数行 body のクォーティングが失われる原因を調査 + - [ ] `--body-file` 対応の追加、または引数転送ロジックの修正 + - [ ] 修正後に複数行 body での PR 作成テスト +- **詰まっている箇所**: なし +- **根拠**: PR #49 作成時に発生。`--body "$(cat <<'EOF' ... EOF)"` 形式で pnpm 経由で渡すと、シェル引数分割で body が `## Summary` だけに切り詰められた + - **Why**: cli-pr-monitor.exe が `--` 以降の引数を gh pr create に転送する際、pnpm のシェル経由で改行・スペースを含む body が複数の引数に分割される + - **How to apply / 再開手順**: `src/cli-pr-monitor/src/stages/create_pr.rs` の引数処理を確認し、`--body-file` を使う方式か、引数を結合する方式で修正 + +### 4. push-runner の takt fix 後 bookmark 乖離問題 + +- **やろうとしたこと**: takt fix ステップ後に @ が bookmark より先に進み、`jj git push` で "No bookmarks found" となる問題を修正する +- **現在地**: 未着手。原因特定済み + - [ ] `src/cli-push-runner/src/stages/push.rs` の push ステップで、push 前に bookmark を @ に追従させるロジックを追加 + - [ ] または takt ステップ後に `jj squash` 相当の処理を自動実行 + - [ ] 修正後に takt fix が発火するケースでの回帰テスト +- **詰まっている箇所**: なし +- **根拠**: PR #49 の push pipeline で発生。takt の fix ステップがコード修正 → @ が bookmark から乖離 → push で bookmark が見つからない + - **Why**: takt は @ 上で直接コード修正するため、fix が入ると @ が新しい commit に進むが、bookmark は旧 commit のまま残る + - **How to apply / 再開手順**: push ステップ内で `push_jj_bookmark::advance_jj_bookmarks()` が既にあるが、これは trunk 以降の bookmark を target に前進させるもの。takt fix 後の bookmark 乖離は別の問題 (bookmark 自体が @ より古い位置にある)。push 前に `jj bookmark set -r @` で bookmark を @ に合わせる処理を追加する + +### 5. push-runner の空 diff 時 pipeline 中断を正常終了に + +- **やろうとしたこと**: push 対象の変更がない場合 (レビュー済みコードの再 push 等)、exit code 5 で中断するのではなく skip として正常終了するオプションを追加する +- **現在地**: 未着手。原因特定済み + - [ ] `src/cli-push-runner/src/stages/diff.rs` の空 diff 判定を "skip review + proceed to push" モードに変更 + - [ ] push-runner-config.toml に `allow_empty_diff = true` 等のオプション追加を検討 + - [ ] 修正後に空 diff ケースでの回帰テスト +- **詰まっている箇所**: なし +- **根拠**: PR #49 で squash 後の再 push 時に発生。jj squash で @ が空コミットになり、`jj diff -r @` が空 → push-runner が "diff 出力が空です" で exit 5 + - **Why**: push-runner は diff が空 = レビュー対象なし = パイプライン中断と判断するが、takt fix 後の再 push や bookmark 移動後の push では「diff は空だが push は必要」なケースがある + - **How to apply / 再開手順**: diff が空の場合に takt レビューをスキップして push ステップに直接進むパスを追加。push-runner-config.toml で挙動を制御できるようにする + +### 6. Cargo workspace 化 + rust-test template 反映 (PR-β、実装済み) - **やろうとしたこと**: PR #44 のセッション知見を元に: 1. Rust test を push pipeline で一発実行できるよう Cargo workspace 化 @@ -167,11 +203,17 @@ ADR-019 および ADR-020 の「次ステップ」セクションで明記され ## 完了履歴 -### conflicted bookmarks の棚卸し +### conflicted bookmarks の棚卸し + push 前 bookmark 自動前進 (PR #49) - [x] **feat/merge-pipeline** (conflicted): `jj bookmark forget` で削除。ADR-013 は master にマージ済み、固有差分は takt 移行前の旧 push_pipeline 設定のみで価値なし - [x] **feat/session-start-hook** (conflicted): `jj bookmark forget` で削除。bookmark 版は旧レイアウト (.claude/) + 「先勝ち」方式。master 版 (`src/hooks-session-start/`) は新レイアウト + 「同一IDスキップ」方式 + テスト 13 本で上位互換 - [x] **feat/push-runner-auto-bookmark** (未 push): `push_jj_bookmark.rs` (bookmark 自動前進機能) を master の `src/cli-push-runner/src/stages/` に cherry-pick 後、`jj bookmark forget` で削除。テスト 10 本 pass +- [x] **CodeRabbit Critical**: `jj bookmark set` の引数順序修正 (`--` 後に `-r` → `-r` を `--` の前に移動) — takt 自動修正 +- [x] **CodeRabbit Major**: `main()` / `master()` を bare bookmark 名 `main` / `master` に修正 — takt 自動修正 +- [x] **CodeRabbit Minor**: `@` が root commit の場合の `@-` 不在ガード追加 (`determine_target_revision` を `Option` に変更) +- [x] **CodeRabbit Minor**: `trunk()` 成功かつ 0 件で `main`/`master` にフォールバックしない問題修正 (非空チェック追加) +- **知見**: cherry-pick 元の bookmark 放置コードに Critical バグが潜んでいた。cherry-pick = 「レビュー済みコードの移植」ではない。push pipeline + CodeRabbit の二重レビューが有効に機能 +- **副産物**: 3 件の仕組み改善タスク (#3, #4, #5) を発見・記録 ### cli-pr-monitor Known Issues (PR #13) diff --git a/src/cli-push-runner/src/main.rs b/src/cli-push-runner/src/main.rs index 17eae38b..a9352fe5 100644 --- a/src/cli-push-runner/src/main.rs +++ b/src/cli-push-runner/src/main.rs @@ -1,4 +1,4 @@ -//! Push Runner — takt ベースの pre-push パイプライン +//! Push Runner — takt ベースの pre-push パイプライン //! //! pnpm push から呼び出され、以下のステージを実行する: //! Stage 1: quality_gate — TOML で定義されたコマンド群をグループ間で並列実行 @@ -25,7 +25,14 @@ use std::time::Instant; use config::load_config; use log::log_info; -use stages::{run_diff, run_push, run_quality_gate, run_takt}; +use stages::{run_diff, run_push, run_quality_gate, run_takt, DiffResult}; + +const EXIT_SUCCESS: i32 = 0; +const EXIT_QUALITY_GATE_FAILURE: i32 = 1; +const EXIT_TAKT_FAILURE: i32 = 2; +const EXIT_PUSH_FAILURE: i32 = 3; +const EXIT_CONFIG_ERROR: i32 = 4; +const EXIT_DIFF_FAILURE: i32 = 5; fn run_pipeline() -> i32 { let start = Instant::now(); @@ -34,7 +41,7 @@ fn run_pipeline() -> i32 { Ok(c) => c, Err(e) => { log_info(&format!("設定エラー: {}", e)); - return 4; + return EXIT_CONFIG_ERROR; } }; @@ -48,32 +55,40 @@ fn run_pipeline() -> i32 { // Stage 1: quality_gate if !run_quality_gate(&config.quality_gate) { log_info("パイプライン中断: quality_gate 失敗。問題を修正して再実行してください。"); - return 1; + return EXIT_QUALITY_GATE_FAILURE; } // Stage 1.5: diff + let mut skip_takt = false; if let Some(diff_config) = &config.diff { - if !run_diff(diff_config) { - log_info("パイプライン中断: diff 取得失敗。"); - return 5; + match run_diff(diff_config) { + DiffResult::HasContent => {} + DiffResult::Empty => { + log_info("diff が空のためレビューをスキップして push に進みます。"); + skip_takt = true; + } + DiffResult::Error => { + log_info("パイプライン中断: diff 取得失敗。"); + return EXIT_DIFF_FAILURE; + } } } // Stage 2: takt - if !run_takt(&config.takt) { + if !skip_takt && !run_takt(&config.takt) { log_info("パイプライン中断: takt ワークフロー失敗。"); - return 2; + return EXIT_TAKT_FAILURE; } // Stage 3: push if !run_push(&config.push) { log_info("パイプライン中断: push 失敗。"); - return 3; + return EXIT_PUSH_FAILURE; } let elapsed = start.elapsed(); log_info(&format!("パイプライン完了 ({:.0}s)", elapsed.as_secs_f64())); - 0 + EXIT_SUCCESS } fn main() { diff --git a/src/cli-push-runner/src/stages/diff.rs b/src/cli-push-runner/src/stages/diff.rs index c1efc9f3..9112889c 100644 --- a/src/cli-push-runner/src/stages/diff.rs +++ b/src/cli-push-runner/src/stages/diff.rs @@ -4,6 +4,16 @@ use std::process::Command; use crate::config::DiffConfig; use crate::log::log_stage; +#[derive(Debug, PartialEq)] +pub(crate) enum DiffResult { + /// diff に内容があり、ファイルへの書き出しが完了した + HasContent, + /// diff 出力が空 (レビュー対象なし、push は続行可能) + Empty, + /// diff コマンドの実行またはファイル書き出しに失敗した + Error, +} + /// diff 取得専用: 出力を切り詰めずに全行を取得する。 /// runner::run_cmd は MAX_LINES=40 で打ち切るため diff には使えない。 fn run_diff_cmd(cmd: &str) -> Result { @@ -20,7 +30,7 @@ fn run_diff_cmd(cmd: &str) -> Result { } } -pub(crate) fn run_diff(config: &DiffConfig) -> bool { +pub(crate) fn run_diff(config: &DiffConfig) -> DiffResult { log_stage("diff", &format!("実行: {}", config.command)); let output = match run_diff_cmd(&config.command) { @@ -30,23 +40,23 @@ pub(crate) fn run_diff(config: &DiffConfig) -> bool { if !err.is_empty() { eprintln!("{}", err); } - return false; + return DiffResult::Error; } }; if output.is_empty() { log_stage( "diff", - "diff 出力が空です。レビュー対象の変更がありません。diff コマンドの revision 指定を確認してください。", + "diff 出力が空です。レビューをスキップして push に進みます。", ); - return false; + return DiffResult::Empty; } let path = Path::new(&config.output_path); if let Some(parent) = path.parent() { if let Err(e) = std::fs::create_dir_all(parent) { log_stage("diff", &format!("ディレクトリ作成失敗: {}", e)); - return false; + return DiffResult::Error; } } @@ -57,11 +67,11 @@ pub(crate) fn run_diff(config: &DiffConfig) -> bool { "diff", &format!("書き出し完了: {} ({} 行)", config.output_path, line_count), ); - true + DiffResult::HasContent } Err(e) => { log_stage("diff", &format!("ファイル書き出し失敗: {}", e)); - false + DiffResult::Error } } } @@ -84,9 +94,8 @@ mod tests { } #[test] - fn run_diff_returns_false_when_output_is_empty() { + fn run_diff_returns_empty_when_output_is_empty() { let out_path = std::env::temp_dir().join("test-run-diff-empty.txt"); - // Ensure a clean slate in case a previous run left the file. let _ = std::fs::remove_file(&out_path); let config = DiffConfig { @@ -97,9 +106,10 @@ mod tests { let result = run_diff(&config); - assert!( - !result, - "run_diff must return false when the diff command produces empty output" + assert_eq!( + result, + DiffResult::Empty, + "run_diff must return Empty when the diff command produces empty output" ); assert!( !out_path.exists(), diff --git a/src/cli-push-runner/src/stages/mod.rs b/src/cli-push-runner/src/stages/mod.rs index 5058950c..9300c6a1 100644 --- a/src/cli-push-runner/src/stages/mod.rs +++ b/src/cli-push-runner/src/stages/mod.rs @@ -4,7 +4,7 @@ mod push_jj_bookmark; mod quality_gate; mod takt; -pub(crate) use diff::run_diff; +pub(crate) use diff::{run_diff, DiffResult}; pub(crate) use push::run_push; pub(crate) use quality_gate::run_quality_gate; pub(crate) use takt::run_takt; diff --git a/src/cli-push-runner/src/stages/push.rs b/src/cli-push-runner/src/stages/push.rs index 997bd7a3..b5abc261 100644 --- a/src/cli-push-runner/src/stages/push.rs +++ b/src/cli-push-runner/src/stages/push.rs @@ -1,7 +1,7 @@ +use super::push_jj_bookmark::advance_jj_bookmarks; use crate::config::{PushConfig, DEFAULT_PUSH_TIMEOUT_SECS}; use crate::log::log_stage; use crate::runner::run_stage_cmd; -use super::push_jj_bookmark::advance_jj_bookmarks; pub(crate) fn run_push(config: &PushConfig) -> bool { // (takt fix や手動 jj describe で @ が進んでも bookmark が旧コミットのまま残る問題の対策) diff --git a/src/cli-push-runner/src/stages/push_jj_bookmark.rs b/src/cli-push-runner/src/stages/push_jj_bookmark.rs index 91eed907..0d0abd59 100644 --- a/src/cli-push-runner/src/stages/push_jj_bookmark.rs +++ b/src/cli-push-runner/src/stages/push_jj_bookmark.rs @@ -10,23 +10,74 @@ pub(super) fn advance_jj_bookmarks() -> Result<(), String> { let bookmarks = get_bookmarks_in_range(&target)?; if bookmarks.is_empty() { - return Ok(()); // 前進対象なし (新規ブランチ等) + // Fallback: takt fix が @ を amend すると旧 commit が obsolete になり、 + // revset ベースの検索では発見できない。`jj bookmark list` は obsolete + // commit 上の bookmark も返すため、こちらで再探索する。 + return advance_bookmarks_via_list(&target); } - for bookmark in &bookmarks { - match set_bookmark(bookmark, &target) { + apply_bookmarks(&bookmarks, &target, ""); + Ok(()) +} + +fn apply_bookmarks(bookmarks: &[String], target: &str, label: &str) { + for bookmark in bookmarks { + match set_bookmark(bookmark, target) { Ok(()) => log_stage( "push", - &format!("bookmark '{}' を {} に自動更新", bookmark, target), + &format!("bookmark '{}' を {} に自動更新{}", bookmark, target, label), ), Err(e) => { - log_info(&format!("bookmark '{}' の更新失敗 (続行): {}", bookmark, e)); + log_info(&format!( + "bookmark '{}' の更新失敗{} (続行): {}", + bookmark, label, e + )); } } } +} + +/// `jj bookmark list` の出力から非 trunk ローカル bookmark を取得し、target に前進させる。 +/// revset ベースの `get_bookmarks_in_range` が空を返した場合のフォールバック。 +/// +/// 安全策: 非 trunk bookmark が 1 つだけの場合のみ前進させる。 +/// 複数ある場合は無関係な bookmark を誤って移動するリスクがあるためスキップする。 +fn advance_bookmarks_via_list(target: &str) -> Result<(), String> { + let bookmarks = get_local_bookmarks_from_list()?; + dispatch_bookmark_advance(&bookmarks, target, |b, t| { + apply_bookmarks(b, t, " (fallback)") + }); Ok(()) } +fn dispatch_bookmark_advance( + bookmarks: &[String], + target: &str, + apply: impl FnOnce(&[String], &str), +) { + match bookmarks.len() { + 0 => { + log_info("ローカル bookmark が見つかりません (新規ブランチ等)"); + } + 1 => { + log_info(&format!( + "fallback: bookmark '{}' を {} に前進させます", + bookmarks[0], target + )); + apply(bookmarks, target); + } + _ => { + // 複数の非 trunk bookmark がある場合、無関係な bookmark を + // 誤って移動するリスクがあるためスキップする + log_info(&format!( + "複数の bookmark ({}) が存在するため fallback 更新をスキップします: {}", + bookmarks.len(), + bookmarks.join(", ") + )); + } + } +} + fn determine_target_revision() -> Result, String> { let output = run_jj_log("@", "if(empty, \"empty\", \"content\")")?; if output.trim() == "empty" { @@ -102,6 +153,30 @@ fn run_jj_log(revset: &str, template: &str) -> Result { ) } +/// `jj bookmark list` の出力をパースし、非 trunk のローカル bookmark 名を返す。 +/// 出力形式: "name: commit_id description\n @origin: commit_id\n" +/// インデントで始まる行はリモート追跡情報なのでスキップする。 +fn get_local_bookmarks_from_list() -> Result, String> { + let output = run_jj(&["bookmark", "list"], "jj bookmark list 実行失敗")?; + Ok(dedup(parse_bookmark_list_output(&output))) +} + +fn parse_bookmark_list_output(output: &str) -> Vec { + output + .lines() + .filter(|line| !line.starts_with(' ') && !line.starts_with('\t')) + .filter_map(|line| line.split(':').next()) + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty() && !is_trunk_bookmark(s)) + .collect() +} + +const TRUNK_BOOKMARKS: &[&str] = &["main", "master", "trunk", "develop"]; + +fn is_trunk_bookmark(name: &str) -> bool { + TRUNK_BOOKMARKS.contains(&name) +} + fn dedup(items: Vec) -> Vec { let mut seen = std::collections::HashSet::new(); items @@ -193,4 +268,97 @@ mod tests { vec!["a", "a", "b"] ); } + + // --- is_trunk_bookmark --- + + #[test] + fn is_trunk_bookmark_matches_known_names() { + assert!(is_trunk_bookmark("main")); + assert!(is_trunk_bookmark("master")); + assert!(is_trunk_bookmark("trunk")); + assert!(is_trunk_bookmark("develop")); + } + + #[test] + fn is_trunk_bookmark_rejects_feature_bookmarks() { + assert!(!is_trunk_bookmark("feat/xyz")); + assert!(!is_trunk_bookmark("fix/bug-123")); + assert!(!is_trunk_bookmark("main-feature")); + } + + // --- parse_bookmark_list_output --- + + #[test] + fn parse_bookmark_list_typical_output() { + let output = "\ +feat/xyz: abc1234 add feature + @origin: abc1234 add feature +main: def5678 initial + @origin: def5678 initial +"; + assert_eq!(parse_bookmark_list_output(output), vec!["feat/xyz"]); + } + + #[test] + fn parse_bookmark_list_multiple_feature_bookmarks() { + let output = "\ +feat/a: 111 desc +feat/b: 222 desc +main: 333 desc +"; + assert_eq!(parse_bookmark_list_output(output), vec!["feat/a", "feat/b"]); + } + + #[test] + fn parse_bookmark_list_empty_output() { + assert_eq!(parse_bookmark_list_output(""), Vec::::new()); + } + + #[test] + fn parse_bookmark_list_only_trunk() { + let output = "main: abc123 desc\nmaster: def456 desc\n"; + assert_eq!(parse_bookmark_list_output(output), Vec::::new()); + } + + // --- dispatch_bookmark_advance --- + + #[test] + fn dispatch_zero_bookmarks_does_not_call_apply() { + let called = std::cell::Cell::new(false); + dispatch_bookmark_advance(&[], "abc123", |_, _| called.set(true)); + assert!(!called.get()); + } + + #[test] + fn dispatch_one_bookmark_calls_apply_with_correct_args() { + let captured = std::cell::RefCell::new(None::<(Vec, String)>); + dispatch_bookmark_advance(&["feat/xyz".to_string()], "abc123", |b, t| { + *captured.borrow_mut() = Some((b.to_vec(), t.to_string())) + }); + assert_eq!( + *captured.borrow(), + Some((vec!["feat/xyz".to_string()], "abc123".to_string())) + ); + } + + #[test] + fn dispatch_multiple_bookmarks_does_not_call_apply() { + let called = std::cell::Cell::new(false); + dispatch_bookmark_advance( + &["feat/a".to_string(), "feat/b".to_string()], + "abc123", + |_, _| called.set(true), + ); + assert!(!called.get()); + } + + #[test] + fn parse_bookmark_list_skips_indented_remote_lines() { + let output = "\ +feat/xyz: abc1234 desc + @origin: abc1234 desc + @upstream: abc1234 desc +"; + assert_eq!(parse_bookmark_list_output(output), vec!["feat/xyz"]); + } }