fix(push-runner): pre-push レビューを PR 全体に修正 + 範囲カバレッジ検査 (順位 288/264) - #313
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPR全体のbase branch範囲をConfigで一元解決し、diff・docs-only routing・PRサイズ検査・post-takt再ゲートへ渡すよう変更した。diff生成では範囲カバレッジを検証し、不足時にfail-closedで停止する。 ChangesPR範囲レビュー統一
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PushRunner
participant Config
participant DiffStage
participant jj
PushRunner->>Config: PR範囲を解決
Config-->>PushRunner: <base>..@
PushRunner->>DiffStage: PR範囲を渡してdiff実行
DiffStage->>jj: diff --git -r <base>..@
DiffStage->>jj: diff --summary <base>..@
jj-->>DiffStage: 変更ファイル集合
DiffStage-->>PushRunner: 検証済みdiffまたはエラー
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
## 問題 `[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 している。
## 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 の祖先未レビュー穴の検証) は明示して 残す。エントリ全体を消すと未着手部分が失われるため削除しない。
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/todo15.md`:
- Around line 447-449: docs/todo15.md の該当エントリで、[diff] stage の PR
範囲化に関する作業計画と完了基準を実装済みとして更新してください。既存の実装済み記述と整合させ、完了したチェック項目を未完了一覧から削除し、残タスクとして
post-merge feedback の全 run 集約と bookmark_check.rs の祖先未レビュー穴の検証だけを残してください。
In `@src/cli-push-runner/src/config/mod.rs`:
- Around line 64-83: Update the base-branch resolution and validation flow
around Config::resolve_base_branch, load_config, and
validate_base_branch_ranges_agree so a legacy section-level default_branch is
resolved once as the shared base branch for the entire configuration. Ensure all
stages, including absent or docs-only stages, use that same effective branch,
preventing valid legacy configurations from being rejected due to inconsistent
defaults.
In `@src/cli-push-runner/src/stages/diff.rs`:
- Around line 140-153: 設定ロード時に[diff]
commandへ{{PR_RANGE}}プレースホルダーが必ず含まれるよう検証を追加し、未設定の場合は明確なエラーで失敗させてください。diff検証処理のmissing判定や成功条件は変更せず、既存の設定読み込み処理と[diff]
commandのシンボルを利用して実装してください。
- Around line 121-130: Update parse_summary_paths to return a Result and reject
any non-empty summary containing an unknown status or malformed R/C entry
instead of silently dropping it. Propagate that parse error through the caller
before evaluating expected.is_empty(), while preserving the successful
empty-summary behavior and existing valid-entry parsing.
- Around line 266-269: Move the verify_diff_covers_pr_range check in the diff
execution flow before the empty-output branch, ensuring fetch_summary is
obtained and coverage is validated even when output is empty. Preserve
report_coverage_failure and DiffResult::Error on failure, then return
DiffResult::Empty only after coverage validation succeeds.
🪄 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: 3739835e-e363-4b16-9967-8d96a8549d51
📒 Files selected for processing (14)
docs/adr/adr-027-push-review-simplicity-focus.mddocs/todo-summary2.mddocs/todo15.mdpush-runner-config.tomlsrc/cli-push-runner/src/config/docs_only_routing.rssrc/cli-push-runner/src/config/lint_screen.rssrc/cli-push-runner/src/config/mod.rssrc/cli-push-runner/src/config/pr_size_check.rssrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/stages/diff.rssrc/cli-push-runner/src/stages/docs_only_routing.rssrc/cli-push-runner/src/stages/post_takt_regate.rssrc/cli-push-runner/src/stages/pr_size_check.rstemplates/push-runner-config.toml
💤 Files with no reviewable changes (1)
- src/cli-push-runner/src/config/pr_size_check.rs
| > **(a) 実装済 (2026-07-21)**: PR #311 で 4 回目の再発 (695 行の PR に対しレビュー対象 37 行、security-review が実 diff と矛盾して "docs-only / No dependency changes" と記載) を観測し、`[diff]` stage を修正した。top-level `default_branch` を新設して `diff` / `docs_only_routing` / `pr_size_check` の 3 stage が同一解決を共有し、`[diff] command` は `{{PR_RANGE}}` プレースホルダ経由で範囲を受け取る (config に revset を直書きできない)。加えて生成 diff が PR 範囲の全変更ファイルを含むかを `jj diff --summary` と突き合わせる**範囲カバレッジ検査**を fail-closed で追加し、config の書き方に依存せず「レビュー範囲 < PR 範囲」を検知できるようにした (未更新の派生プロジェクト config も捕まる)。順位 264 (`--git` 形式切替) も同 PR で同時実施 (範囲検査が `diff --git` ヘッダを読む要件と重なるため)。**ADR-027 の射程についてのユーザー判断**: 範囲拡張のレビュー時間コストを実測したところ 37 行 4m32s → 1011 行 4m43s (**+11 秒**) で、ADR-027 の速度改善は arch-review facet 除去によるものであり diff 範囲縮小は寄与していないことが判明したため、範囲拡張を採用した。 | ||
| > | ||
| > **残タスク**: 本エントリ本体の「post-merge feedback の全 run 集約」と、上記 (b) `bookmark_check.rs` の祖先未レビュー穴の検証は未着手。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
実装済みの作業計画を同期してください。
ここでは [diff] stage の PR 範囲化を「実装済み」としていますが、同じエントリ内の作業計画・完了基準には、なお未完了の作業として残っています。該当チェック項目を完了扱いに更新し、残タスクだけを記載してください。
🤖 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 `@docs/todo15.md` around lines 447 - 449, docs/todo15.md の該当エントリで、[diff] stage
の PR
範囲化に関する作業計画と完了基準を実装済みとして更新してください。既存の実装済み記述と整合させ、完了したチェック項目を未完了一覧から削除し、残タスクとして
post-merge feedback の全 run 集約と bookmark_check.rs の祖先未レビュー穴の検証だけを残してください。
| impl Config { | ||
| /// PR base branch を解決する。優先順: section override → top-level → 既定値。 | ||
| /// | ||
| /// section override は後方互換のために残してある (既存の派生プロジェクト config が | ||
| /// `[pr_size_check] default_branch` 等を持つため)。新規に section 側で持たせないこと。 | ||
| /// override 値が stage 間で食い違わないことは [`validate_base_branch_ranges_agree`] | ||
| /// が `load_config` 時に fail-closed で保証する。 | ||
| pub(crate) fn resolve_base_branch(&self, section_override: Option<&str>) -> String { | ||
| section_override | ||
| .map(str::trim) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(str::to_string) | ||
| .or_else(|| { | ||
| self.default_branch | ||
| .as_deref() | ||
| .map(str::trim) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(str::to_string) | ||
| }) | ||
| .unwrap_or_else(|| DEFAULT_BASE_BRANCH.to_string()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
section override が後方互換になっていません。
例えば既存 config が [pr_size_check] default_branch = "main" だけを持つ場合、PR size は main..@、存在しない diff/docs-only stage は master..@ となり、Line 262 で設定全体が拒否されます。実際に有効な stage だけを比較するか、legacy override を共通 base branch として一度だけ解決してください。
Also applies to: 255-267
🤖 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-push-runner/src/config/mod.rs` around lines 64 - 83, Update the
base-branch resolution and validation flow around Config::resolve_base_branch,
load_config, and validate_base_branch_ranges_agree so a legacy section-level
default_branch is resolved once as the shared base branch for the entire
configuration. Ensure all stages, including absent or docs-only stages, use that
same effective branch, preventing valid legacy configurations from being
rejected due to inconsistent defaults.
| let expected = parse_summary_paths(&summary); | ||
| if expected.is_empty() { | ||
| if summary.trim().is_empty() { | ||
| return Ok(()); | ||
| } | ||
| return Err(format!( | ||
| "summary に行はあるが 1 件もパースできませんでした ({} 行)。jj の出力書式が\ | ||
| 変わった可能性があります", | ||
| summary.lines().filter(|l| !l.trim().is_empty()).count() | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
未知 status が既知 status と混在すると coverage から消えます。
M known.rs と X omitted.rs が混在すると、Line 194 で後者だけが破棄され、expected が空ではないため parse エラーにもなりません。parser を Result にして、非空行が一つでも未知 status、または不正な R/C 形式なら fail-closed にしてください。
Also applies to: 168-197
🤖 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-push-runner/src/stages/diff.rs` around lines 121 - 130, Update
parse_summary_paths to return a Result and reject any non-empty summary
containing an unknown status or malformed R/C entry instead of silently dropping
it. Propagate that parse error through the caller before evaluating
expected.is_empty(), while preserving the successful empty-summary behavior and
existing valid-entry parsing.
| let missing: Vec<&String> = expected.iter().filter(|p| !covered.contains(*p)).collect(); | ||
| if missing.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| Err(format!( | ||
| "{} ファイルが未収録 (例: {})", | ||
| missing.len(), | ||
| missing | ||
| .iter() | ||
| .take(3) | ||
| .map(|s| s.as_str()) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| )) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
ファイル集合の一致だけでは tip-only diff を検出できません。
祖先と tip が同じファイルを変更した場合、jj diff -r @ でも expected ⊆ covered が成立し、祖先の hunks を欠いたまま Line 142 で成功します。少なくとも [diff] command に {{PR_RANGE}} が含まれることを設定ロード時に必須化してください。
🤖 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-push-runner/src/stages/diff.rs` around lines 140 - 153, 設定ロード時に[diff]
commandへ{{PR_RANGE}}プレースホルダーが必ず含まれるよう検証を追加し、未設定の場合は明確なエラーで失敗させてください。diff検証処理のmissing判定や成功条件は変更せず、既存の設定読み込み処理と[diff]
commandのシンボルを利用して実装してください。
| if let Err(reason) = verify_diff_covers_pr_range(&output, fetch_summary) { | ||
| report_coverage_failure(pr_range, &reason); | ||
| return DiffResult::Error; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
空出力も coverage 検査後に判定してください。
現在は tip が空なら summary を取得せず DiffResult::Empty になります。祖先に変更がある古い -r @ config では AI review を完全に skip できるため、Line 266 の検査を空出力分岐より前へ移動してください。
修正案
+ if let Err(reason) = verify_diff_covers_pr_range(&output, fetch_summary) {
+ report_coverage_failure(pr_range, &reason);
+ return DiffResult::Error;
+ }
+
if output.is_empty() {
...
return DiffResult::Empty;
}
-
- if let Err(reason) = verify_diff_covers_pr_range(&output, fetch_summary) {
- report_coverage_failure(pr_range, &reason);
- return DiffResult::Error;
- }Also applies to: 563-574
🤖 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-push-runner/src/stages/diff.rs` around lines 266 - 269, Move the
verify_diff_covers_pr_range check in the diff execution flow before the
empty-output branch, ensuring fetch_summary is obtained and coverage is
validated even when output is empty. Preserve report_coverage_failure and
DiffResult::Error on failure, then return DiffResult::Empty only after coverage
validation succeeds.
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)
Applicable Findings (Medium 以下)
Filtered (not applicable)該当なし 次のアクション
|
1ec2eb6 to
a410e29
Compare
a410e29 to
cff96fe
Compare
概要
pnpm pushの pre-push AI レビューが tip コミット 1 本しか見ていなかった欠陥を修正します。push-runner-config.tomlの[diff] commandにjj diff -r @が直書きされており、複数コミットを 1 回の push で送ると 祖先コミットがセルフレビューを一度も経ずに merge される状態でした。docs/todo-summary2.md 順位 288(🚀 Tier 1)として既知で、Severity High で PR #268/#300/#301/#311 と 4 回再発していた事案です。順位 264(--git形式切替)も同時に解消します。根本原因
同じパイプライン内の
pr_size_checkとdocs_only_routingは<base>..@(PR 範囲)を Rust 側で組み立てていましたが、[diff]だけがjj diff -r @(tip のみ)で 非対称でした。しかもdocs_only_routing.rsの doc コメントにこの非対称が明記されたまま放置されていました。レビュアーは渡された diff が PR 全体かを検証できないため、範囲が狭いことによる見落としは誰にも検知されません(実際 security-review が実 diff と矛盾して "docs-only / No dependency changes" と報告していました)。変更
default_branchを新設し、diff/docs_only_routing/pr_size_checkの 3 stage がConfig::resolve_base_branch経由で同一の値を使う。section 側のdefault_branchは後方互換の override として残す。[diff] commandは{{PR_RANGE}}プレースホルダを使い、push-runner が<base>..@に展開する(狭い範囲を直書きできない)。jj diff --summaryと突き合わせ、不足なら exit 5。config の書き方に依存せず「レビュー範囲 < PR 範囲」を検知できるため、未更新の派生プロジェクト config も捕まえる。--git形式へ切替(順位 264): 範囲検査がヘッダを読む要件に加え、jj 既定形式は色を落とすと+/-が消え LLM が削除を追加と誤読する(PR docs(adr): ADR-051 クロスシステム設定 coupling パターンを起票 (順位263) #256 で実害)。templates/push-runner-config.tomlも同時修正(pnpm deploy:hooksで派生配布のため必須)。ADR-051(cross-system config coupling)として exe/config 同時更新義務を両 config にコメント記載。ADR-027 の射程を実測で確定
ADR-027 が狭めたのは reviewer の観点(cross-file 探索の除去)であってコミット範囲ではなかった、という点を amendment として明文化しました。範囲拡張のコストを実測:
27 倍に広げて +11 秒。ADR-027 の速度改善は arch-review facet 除去によるもので、範囲縮小は寄与していないことが判明したため、範囲拡張を採用しました。
この PR 自身が範囲修正の価値を実証
範囲検査コード(私が書いた部分)に対し、PR 全体をレビュー対象にしたことで pre-push セルフレビューが 3 ラウンドに渡り 5 件の指摘を検出しました(すべて親コミットにあり tip-only レビューでは 1 件も見えなかった)。5 件は 5 コミット目(この PR の tip、"欠陥 5 件を修正")に集約:
R <old> <new>を 1 個の壊れたパスとして扱い、rename を含む push が必ず exit 5 で停止。実 jj 出力で修正を検証済み。post_takt_regate.rsをConfig::diff_pr_range()に統一。実測で見つけた副次バグ(実シェルで検証)
範囲 summary の取得を当初シェル経由で実装したところ、cmd.exe はクォートを除去せず jj に渡すため
Revision '"master..@"' doesn't existで失敗しました。sh は除去するので Linux だけ通る = Windows の全 push が止まる形。ユニットテストは summary 取得を注入していたため検出できず、実シェルで叩いて初めて判明しました。direct args 呼び出しに修正済み。既知の限定事項(意図的に未修正)
summary_line_new_pathのrsplit_once(' ')は、rename 先パス自体に空白が含まれると誤分割します。ただし self-mitigating(silent pass ではなく fail-closed の missing-files に落ちる)で、正しく直すには jj の空白パス escaping 形式の調査が必要、かつ本リポジトリに空白入りパスは存在しないため、修正コスト > 得られる安全性と判断し未修正としました(security-review も non-blocking と評価)。検証
cargo test --workspace全 pass・clippy clean・lint:docs/lint:md退行なし。依存関係(マージ順序)
この PR は master ベースのため、#312(pipeline lock race 修正)を先にマージしてから本 PR を master に rebase すると、本 PR の CI に含まれる flaky なロックテスト(#312 が修正した race 由来)が解消されます。
🤖 Generated with Claude Code
Summary by CodeRabbit
改善
ドキュメント