feat(merge-pipeline): --feedback-only フラグを追加 — feedback step 未到達失敗の手動 recovery (ADR-030 補完) - #268
Conversation
… recovery (ADR-030 補完、PR #267 で実観測)
📝 WalkthroughWalkthrough
Changesフィードバック専用実行
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI as cli-merge-pipeline
participant Pipeline as pipeline::run_feedback_only
participant Step as run_ai_step_for
participant Feedback as run_feedback_and_report
CLI->>Pipeline: PR番号を渡して実行
Pipeline->>Step: AIステップを実行
Step->>Feedback: フィードバックとレポート生成
Feedback-->>Step: レポートパスまたはエラー
Step-->>Pipeline: Result<PathBuf, String>
Pipeline-->>CLI: 終了コード0または1
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし(レビュー指摘0件) Applicable Findings (Medium 以下)該当なし Filtered (not applicable)該当なし 次のアクション
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/cli-merge-pipeline/src/pipeline.rs (1)
317-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
skip_with_failed_markerとwarn_feedback_failureの重複ロジックを統合両関数とも「
.failedmarker を書き込み、成否をWARNログに出す」という同じ責務を別実装しており(Line 157-182 参照)、メッセージ文言もずれています。共通ヘルパーに切り出すと、今回発見したような「一方だけ更新されもう一方が追従しない」類の不整合を防げます。♻️ 統合案(イメージ)
+fn write_failed_marker_and_log(label: &str, repo_root: &Path, pr_number: u64, reason: &str) { + match feedback::write_failed_marker(repo_root, pr_number, reason) { + Ok(marker) => log_step( + label, + "WARN", + &format!("marker: {} (L2 recovery が拾います)", marker.display()), + ), + Err(e) => log_step(label, "WARN", &format!("marker 書込も失敗: {}", e)), + } +} + fn skip_with_failed_marker(label: &str, pr_number: u64, reason: &str) { log_step( label, "WARN", &format!("{} — feedback workflow をスキップ", reason), ); let repo_root = match std::env::current_dir() { Ok(p) => p, Err(e) => { log_step( label, "WARN", &format!("current_dir 取得失敗: {} — marker 書込を断念", e), ); return; } }; - match feedback::write_failed_marker(&repo_root, pr_number, reason) { - Ok(marker) => log_step( - label, - "WARN", - &format!("marker: {} (L2 recovery が拾います)", marker.display()), - ), - Err(e) => log_step(label, "WARN", &format!("marker 書込も失敗: {}", e)), - } + write_failed_marker_and_log(label, &repo_root, pr_number, reason); } fn warn_feedback_failure(label: &str, repo_root: &Path, pr_number: u64, reason: &str) { - match feedback::write_failed_marker(repo_root, pr_number, reason) { - Ok(marker) => log_step( - label, - "WARN", - &format!( - "feedback workflow 失敗: {} — marker: {} (L2 recovery が拾います)", - reason, - marker.display() - ), - ), - Err(marker_err) => log_step( - label, - "WARN", - &format!( - "feedback workflow 失敗: {} — marker 書込も失敗: {}", - reason, marker_err - ), - ), - } + log_step(label, "WARN", &format!("feedback workflow 失敗: {}", reason)); + write_failed_marker_and_log(label, repo_root, pr_number, reason); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli-merge-pipeline/src/pipeline.rs` around lines 317 - 338, 統合して、.failed marker の書き込みと成否のWARNログ生成を単一の共通ヘルパーに集約してください。既存の skip_with_failed_marker と warn_feedback_failure はそのヘルパーを呼び出し、各呼び出し元固有の引数やラベルだけを渡す構成に変更し、marker書き込み成功時と失敗時のメッセージ形式が両経路で一致するようにしてください。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli-merge-pipeline/src/pipeline.rs`:
- Around line 234-241: Update the Err(reason) handling in the pipeline result
reporting around log_step so the failure message no longer always references a
.failed marker. Keep the reason and existing FAIL status, but only mention the
marker when the failure originated from the feedback::run/warn_feedback_failure
path; otherwise report a generic failure without directing recovery to a
nonexistent file.
- Around line 209-223: Update the owner_repo detection and validation failure
branches in run_feedback_only to call the existing skip_with_failed_marker flow,
preserving the current failure logging and return code while ensuring the
.failed marker is written consistently with the automatic workflow.
---
Nitpick comments:
In `@src/cli-merge-pipeline/src/pipeline.rs`:
- Around line 317-338: 統合して、.failed marker
の書き込みと成否のWARNログ生成を単一の共通ヘルパーに集約してください。既存の skip_with_failed_marker と
warn_feedback_failure
はそのヘルパーを呼び出し、各呼び出し元固有の引数やラベルだけを渡す構成に変更し、marker書き込み成功時と失敗時のメッセージ形式が両経路で一致するようにしてください。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59b0b42f-3436-4647-812c-ab449f076247
📒 Files selected for processing (2)
src/cli-merge-pipeline/src/main.rssrc/cli-merge-pipeline/src/pipeline.rs
| /// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。 | ||
| pub(crate) fn run_feedback_only(pr_number: u64) -> i32 { | ||
| let label = "feedback-only"; | ||
| let Some(owner_repo) = detect_owner_repo() else { | ||
| log_step( | ||
| label, | ||
| "FAIL", | ||
| "owner_repo を取得できませんでした (gh repo view 失敗?)", | ||
| ); | ||
| return 1; | ||
| }; | ||
| if !lib_pending_file::is_valid_owner_repo(&owner_repo) { | ||
| log_step(label, "FAIL", &format!("owner_repo が不正: {}", owner_repo)); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
owner_repo 検出/検証失敗時に .failed marker が書かれない — docstring の約束と不一致
Line 209 のdocstringは「1 = 失敗 (marker は通常経路と同様に残る)」と明記していますが、実際の owner_repo 失敗パス (Line 212-223) は log_step(..., "FAIL", ...) のみで、通常経路の skip_with_failed_marker (Line 157-182 相当) が行う .failed marker 書込を行っていません。PR目的の「failure-marker handling as the automatic workflow」の再利用にも反しており、L2 recovery hookがこのケースを検知できなくなります。
既存の skip_with_failed_marker を呼び出すだけで解消できます。
🐛 修正案
pub(crate) fn run_feedback_only(pr_number: u64) -> i32 {
let label = "feedback-only";
let Some(owner_repo) = detect_owner_repo() else {
- log_step(
- label,
- "FAIL",
- "owner_repo を取得できませんでした (gh repo view 失敗?)",
- );
+ skip_with_failed_marker(
+ label,
+ pr_number,
+ "owner_repo を取得できませんでした (gh repo view 失敗?)",
+ );
return 1;
};
if !lib_pending_file::is_valid_owner_repo(&owner_repo) {
- log_step(label, "FAIL", &format!("owner_repo が不正: {}", owner_repo));
+ skip_with_failed_marker(
+ label,
+ pr_number,
+ &format!("owner_repo が不正: {}", owner_repo),
+ );
return 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。 | |
| pub(crate) fn run_feedback_only(pr_number: u64) -> i32 { | |
| let label = "feedback-only"; | |
| let Some(owner_repo) = detect_owner_repo() else { | |
| log_step( | |
| label, | |
| "FAIL", | |
| "owner_repo を取得できませんでした (gh repo view 失敗?)", | |
| ); | |
| return 1; | |
| }; | |
| if !lib_pending_file::is_valid_owner_repo(&owner_repo) { | |
| log_step(label, "FAIL", &format!("owner_repo が不正: {}", owner_repo)); | |
| return 1; | |
| } | |
| /// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。 | |
| pub(crate) fn run_feedback_only(pr_number: u64) -> i32 { | |
| let label = "feedback-only"; | |
| let Some(owner_repo) = detect_owner_repo() else { | |
| skip_with_failed_marker( | |
| label, | |
| pr_number, | |
| "owner_repo を取得できませんでした (gh repo view 失敗?)", | |
| ); | |
| return 1; | |
| }; | |
| if !lib_pending_file::is_valid_owner_repo(&owner_repo) { | |
| skip_with_failed_marker( | |
| label, | |
| pr_number, | |
| &format!("owner_repo が不正: {}", owner_repo), | |
| ); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli-merge-pipeline/src/pipeline.rs` around lines 209 - 223, Update the
owner_repo detection and validation failure branches in run_feedback_only to
call the existing skip_with_failed_marker flow, preserving the current failure
logging and return code while ensuring the .failed marker is written
consistently with the automatic workflow.
| Err(reason) => { | ||
| log_step( | ||
| label, | ||
| "FAIL", | ||
| &format!("{} (詳細は上記ログ / .failed marker)", reason), | ||
| ); | ||
| 1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
FAILメッセージが .failed marker の存在を暗示するが、実際には書かれないケースがある
Err(reason) は current_dir 取得失敗や trivial PR skip (ai_step_should_skip_trivial) からも返りますが、これらのケースでは marker は書かれません (marker書込があるのは feedback::run 失敗時の warn_feedback_failure 経路のみ)。汎用メッセージが常に「.failed marker」を参照すると、復旧作業時に存在しないファイルを探させてしまいます。
💬 修正案
Err(reason) => {
log_step(
label,
"FAIL",
- &format!("{} (詳細は上記ログ / .failed marker)", reason),
+ &format!("{} (詳細は上記ログを参照)", reason),
);
1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Err(reason) => { | |
| log_step( | |
| label, | |
| "FAIL", | |
| &format!("{} (詳細は上記ログ / .failed marker)", reason), | |
| ); | |
| 1 | |
| } | |
| Err(reason) => { | |
| log_step( | |
| label, | |
| "FAIL", | |
| &format!("{} (詳細は上記ログを参照)", reason), | |
| ); | |
| 1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli-merge-pipeline/src/pipeline.rs` around lines 234 - 241, Update the
Err(reason) handling in the pipeline result reporting around log_step so the
failure message no longer always references a .failed marker. Keep the reason
and existing FAIL status, but only mention the marker when the failure
originated from the feedback::run/warn_feedback_failure path; otherwise report a
generic failure without directing recovery to a nonexistent file.
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)
Applicable Findings (Medium 以下)
Filtered (not applicable)該当なし(全指摘がプロジェクトのソースコード内かつ ADR-030 の marker 保証という設計意図と整合しており、フィルタ対象なし) 次のアクション
|
## 問題 `[diff] command` に `jj diff -r @` が直書きされており、AI レビュアーには **tip コミットの diff しか渡っていなかった**。祖先コミットは pre-push のセルフレビューを 一度も経ずに merge される。 同じパイプライン内で `pr_size_check` と `docs_only_routing` は `<base>..@` (PR 範囲) を見ており、`[diff]` だけが非対称だった。実際 PR #311 では 695 行の PR に対して 37 行だけがレビュー対象になり、レビュアーは渡された 37 行を見て正しく「docs-only」と 判定していた。**レビュアー側からは「渡された diff が PR 全体か」を検証できない**ため、 この誤りは誰にも検知されない。 docs/todo-summary2.md 順位 288 として既知で、Severity High で PR #268/#300/#301 に 続き #311 が 4 回目の再発。 ## 変更 - **範囲の真実源を 1 箇所に**: top-level `default_branch` を新設し、`diff` / `docs_only_routing` / `pr_size_check` の 3 stage が `Config::resolve_base_branch` 経由で同じ値を使う。従来は section ごとに独立した `default_branch` を持ち、 「値を同期する義務」を config コメントで課していた (docs_only_routing.rs の doc に 明記されていた) が、義務はコード上の不変条件ではないため非対称を許していた。 section 側は後方互換の override として残す (派生プロジェクトの既存 config 対策)。 - **config から revset を排除**: `[diff] command` は `{{PR_RANGE}}` プレースホルダを 使う。push-runner が `<base>..@` に展開するため、狭い範囲を書く余地が無くなる。 - **範囲カバレッジ検査 (fail-closed)**: 生成した diff が PR 範囲の全変更ファイルを 含むか `jj diff --summary` と突き合わせ、不足があれば exit 5 で中断する。config の 書き方に依存せず「レビュー範囲 < PR 範囲」を検知できるため、未更新の派生プロジェクト config も捕まえる。summary 取得失敗・`diff --git` ヘッダ不在 (= 収録ファイルを特定 できない) も「網羅している」に倒さずエラーにする (ADR-043)。 - **`--git` 形式へ切替 (順位 264)**: 範囲検査がヘッダを読む要件に加え、jj 既定形式は 色を落とすと `+`/`-` が消えて LLM レビュアーが削除を追加と誤読する (PR #256 で simplicity-review が todo 25 行の削除を追加と誤読し false positive REJECT、約 19 分 浪費)。 - `templates/push-runner-config.toml` も同時修正 (deploy:hooks で派生配布されるため)。 ## 実測で見つけた副次バグ 範囲 summary の取得を当初 shell 経由 (`[diff] command` と同じ経路) で実装したが、 実シェルで叩いたところ **cmd.exe はクォートを除去せず jj に渡す**ため `Revision '"<base>..@"' doesn't exist` で必ず失敗した。sh は除去するので Linux だけ 通る = Windows の全 push が fail-closed で止まる形。ユニットテストは summary 取得を 注入していたため検出できなかった。direct args 呼び出し (docs_only_routing の既存 `run_jj_diff_summary`) を共有する形に修正し、doc に理由を残した。両 stage は 「PR 範囲の変更ファイル一覧」という同一の問いを扱うため、別実装にすると本 PR が 排除した非対称を再導入することになる。 ## 検証 cargo test --workspace 全 pass / clippy clean。範囲検査は incident 形状 (PR 2 ファイル 変更に対し diff は tip の 1 ファイルのみ) の再現テストで固定し、fail-closed 経路 (summary 取得失敗 / ヘッダ不在) と過剰検知しない経路 (空 PR 範囲 / Windows パス 区切りの正規化) も併せて固定した。base branch 解決は 3 stage が同一範囲に解決される ことと override の優先順位を machine-enforce している。
) * fix(push-runner): AI レビュー対象 diff を PR 全体に修正し範囲を機械検査する (順位 288/264) ## 問題 `[diff] command` に `jj diff -r @` が直書きされており、AI レビュアーには **tip コミットの diff しか渡っていなかった**。祖先コミットは pre-push のセルフレビューを 一度も経ずに merge される。 同じパイプライン内で `pr_size_check` と `docs_only_routing` は `<base>..@` (PR 範囲) を見ており、`[diff]` だけが非対称だった。実際 PR #311 では 695 行の PR に対して 37 行だけがレビュー対象になり、レビュアーは渡された 37 行を見て正しく「docs-only」と 判定していた。**レビュアー側からは「渡された diff が PR 全体か」を検証できない**ため、 この誤りは誰にも検知されない。 docs/todo-summary2.md 順位 288 として既知で、Severity High で PR #268/#300/#301 に 続き #311 が 4 回目の再発。 ## 変更 - **範囲の真実源を 1 箇所に**: top-level `default_branch` を新設し、`diff` / `docs_only_routing` / `pr_size_check` の 3 stage が `Config::resolve_base_branch` 経由で同じ値を使う。従来は section ごとに独立した `default_branch` を持ち、 「値を同期する義務」を config コメントで課していた (docs_only_routing.rs の doc に 明記されていた) が、義務はコード上の不変条件ではないため非対称を許していた。 section 側は後方互換の override として残す (派生プロジェクトの既存 config 対策)。 - **config から revset を排除**: `[diff] command` は `{{PR_RANGE}}` プレースホルダを 使う。push-runner が `<base>..@` に展開するため、狭い範囲を書く余地が無くなる。 - **範囲カバレッジ検査 (fail-closed)**: 生成した diff が PR 範囲の全変更ファイルを 含むか `jj diff --summary` と突き合わせ、不足があれば exit 5 で中断する。config の 書き方に依存せず「レビュー範囲 < PR 範囲」を検知できるため、未更新の派生プロジェクト config も捕まえる。summary 取得失敗・`diff --git` ヘッダ不在 (= 収録ファイルを特定 できない) も「網羅している」に倒さずエラーにする (ADR-043)。 - **`--git` 形式へ切替 (順位 264)**: 範囲検査がヘッダを読む要件に加え、jj 既定形式は 色を落とすと `+`/`-` が消えて LLM レビュアーが削除を追加と誤読する (PR #256 で simplicity-review が todo 25 行の削除を追加と誤読し false positive REJECT、約 19 分 浪費)。 - `templates/push-runner-config.toml` も同時修正 (deploy:hooks で派生配布されるため)。 ## 実測で見つけた副次バグ 範囲 summary の取得を当初 shell 経由 (`[diff] command` と同じ経路) で実装したが、 実シェルで叩いたところ **cmd.exe はクォートを除去せず jj に渡す**ため `Revision '"<base>..@"' doesn't exist` で必ず失敗した。sh は除去するので Linux だけ 通る = Windows の全 push が fail-closed で止まる形。ユニットテストは summary 取得を 注入していたため検出できなかった。direct args 呼び出し (docs_only_routing の既存 `run_jj_diff_summary`) を共有する形に修正し、doc に理由を残した。両 stage は 「PR 範囲の変更ファイル一覧」という同一の問いを扱うため、別実装にすると本 PR が 排除した非対称を再導入することになる。 ## 検証 cargo test --workspace 全 pass / clippy clean。範囲検査は incident 形状 (PR 2 ファイル 変更に対し diff は tip の 1 ファイルのみ) の再現テストで固定し、fail-closed 経路 (summary 取得失敗 / ヘッダ不在) と過剰検知しない経路 (空 PR 範囲 / Windows パス 区切りの正規化) も併せて固定した。base branch 解決は 3 stage が同一範囲に解決される ことと override の優先順位を machine-enforce している。 * docs: ADR-027 に「diff 局所 = 観点の限定であって範囲の限定ではない」を明記 (順位 288/264) ## ADR-027 amendment ADR-027 が狭めたのは reviewer が使う criteria (cross-file 探索を要求しない) であり、レビュー対象に含めるコミットの範囲ではなかった。この 2 つが混同され、 `[diff] command` が tip コミット限定のまま運用されて 4 回の再発を招いたため、 射程を明文化した。 「レビュー対象は PR 範囲全体」と決定した根拠も併記: - 速度は理由にならない。同一 PR でレビュー対象を 37 行 → 1011 行 (27 倍) に 広げても 4m32s → 4m43s の +11 秒。ADR-027 の速度改善は arch-review facet の 除去 (219-270s/iter) によるもので、範囲縮小は寄与していなかった。 - 範囲が狭いことによる見落としはレビュアー側から検知できない (渡された diff が PR 全体かを検証する手段が無い)。 - CodeRabbit backstop はセルフレビューを省く理由にならない。独立した層として併用する。 ## todo 更新 - 順位 264 (`--git` 切替): 完了につきエントリと table 行を削除。 - 順位 288: `[diff]` 範囲修正の部分のみ完了として記録。**残タスク** (post-merge feedback の全 run 集約、bookmark_check.rs の祖先未レビュー穴の検証) は明示して 残す。エントリ全体を消すと未着手部分が失われるため削除しない。 * fix(push-runner): pre-push 範囲検査の欠陥修正 + CodeRabbit 指摘5件対応 (#313)
概要
cli-merge-pipeline --feedback-only <PR番号>フラグを追加する (ADR-030 の recovery 機構の補完)。背景 (PR #267 で実観測)
merge pipeline が post_merge_feedback step の到達前に失敗すると (PR #267 ではローカル同期の concurrent checkout 中断)、
.failedmarker が書かれないため ADR-030 L2 recovery (UserPromptSubmit hook) の対象にならず、feedback を再実行する経路が存在しなかった。PR #267 の feedback は本フラグの初回実行で生成済み (.claude/feedback-reports/267.md)。変更内容
main.rs:--feedback-only <PR>の引数解析 (不正入力は usage エラー + exit 2)。引数なしは従来どおり通常 pipelinepipeline.rs:run_feedback_only()— marker の有無に依存せず feedback workflow を単独再実行。owner_repo 検出・validation・ai_step_should_skip_trivial・.failedmarker 書込など、自動経路と同一のガードを共有 (run_ai_step_forを共用)run_ai_step_for/run_feedback_and_reportが実際のResult<PathBuf, String>を返すよう整理し、exit code を本呼び出しの実結果から導出 (pre-push simplicity-review の REJECT 指摘 SIM-NEW-pipeline-L224 を fix step が修正: 初版のpath.exists()判定は stale report で偽 PASS を返す hidden-coupling だった)検証
[feedback-only] PASS)補足
「pipeline が feedback step 到達前に死ぬ」根本原因 (Stop hook 品質ゲートと background merge pipeline の checkout 競合) は本 PR のスコープ外で、次 PR (pipeline lock + Stop hook skip 機構、todo 登録予定) で対応する。本 PR はその事故クラスの recovery 経路を先に確保するもの。
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
--feedback-only <PR番号>オプションを追加し、指定したPRのフィードバック処理を個別に実行できるようになりました。改善