feat(cli-pr-monitor): takt fix を独立 child commit に分離 (task 4) - #63
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough分離型 fix commit の事前作成と説明自動生成、空コミットの安全破棄を実装し、repush 判定フローに FixCommitState を導入して自動 push / 破棄の振る舞いを制御。関連ドキュメント（ADR-022、todo）を境界条件に合わせ更新。 Changes
Sequence DiagramsequenceDiagram
participant Monitor as Monitor Stage
participant Findings as Findings Collector
participant FixCommit as Fix Commit Creator
participant Takt as Takt Executor
participant Repush as Repush Logic
participant Push as Push Executor
Monitor->>Findings: Collect findings
Findings-->>Monitor: findings list
Monitor->>FixCommit: create_fix_commit(pr_number, findings)
FixCommit->>FixCommit: build description
FixCommit->>FixCommit: run `jj new -m <desc>`
FixCommit-->>Monitor: FixCommitState (Created/None)
Monitor->>Takt: run takt with pre-created commit
Takt-->>Monitor: takt result (success/failure)
alt takt_succeeded
Monitor->>Repush: execute_repush_flow(decision, fix_state)
Repush->>Repush: decide_repush_action(...)
alt RepushAction::AutoPush
Repush->>Push: finalize_commit_structure() → push_to_remote()
else RepushAction::NoChangeWithCleanup
Repush->>FixCommit: try_abandon_empty_fix_commit()
else RepushAction::Manual
Repush-->>Monitor: await manual push
end
else takt_failed
Monitor->>FixCommit: try_abandon_empty_fix_commit()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cli-pr-monitor/src/stages/push.rs (1)
22-29:⚠️ Potential issue | 🟡 Minorjj判定も実行時と同じargvパースに揃えてください。
push_to_remoteはsplit_whitespaceで実行できる一方、ここだけstarts_with("jj ")なので、先頭空白やjj.exe表記でbookmark前進だけスキップされます。PR#53対策の再発条件になるため、同じ分割結果で判定してください。修正案
- if push_command.starts_with("jj ") { + let is_jj_push = matches!( + push_command.split_whitespace().next(), + Some("jj" | "jj.exe") + ); + if is_jj_push { if let Err(e) = advance_jj_bookmarks() { log_info(&format!( "[action] bookmark 自動更新失敗 (push は続行): {}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/stages/push.rs` around lines 22 - 29, The jj check should mirror the argv parsing used elsewhere (e.g., push_to_remote) by splitting on whitespace and examining the first token; replace the starts_with("jj ") check with extracting first = push_command.split_whitespace().next(), normalize by stripping a trailing ".exe" (e.g., first.trim_end_matches(".exe")), then compare equality to "jj" and call advance_jj_bookmarks() when it matches; keep the existing error handling around advance_jj_bookmarks() unchanged.docs/todo.md (1)
93-123:⚠️ Potential issue | 🟡 Minortask 4の状態を実装済みに合わせて更新してください。
この節は実装済みの挙動を説明していますが、Line 93は「未実装」、Line 123は「ADR-022 追記予定」のままです。少なくとも「実装済み / E2E目視確認待ち」と「ADR-022追記済み」に直すと、TODOとしての現在地が一致します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/todo.md` around lines 93 - 123, Update the TODO text to reflect that task 4 is implemented: change the status from "未実装" to "実装済み / E2E目視確認待ち" and mark ADR-022 as applied by replacing "ADR-022 追記予定" with "ADR-022 追記済み"; ensure the "タスク分解" checklist reflects task 4 as completed (check the item about コミット分離ロジック実装 (案 A ベース) and E2E 検証) and keep references to the implemented behavior (pre_takt_commit_id handling, decide_repush == HasChange flow, jj new/jj abandon handling) consistent with the prose so the documentation matches the current implementation.src/cli-pr-monitor/src/stages/monitor.rs (1)
56-63:⚠️ Potential issue | 🟠 Majorfix commit作成失敗時はtaktをfail-closedで止めてください。
create_fix_commitがNoneを返してもこのままrun_taktへ進むため、jj new失敗時はtaktのeditが元の@にamendされ得ます。分離型fix commitの目的は既存commit不変なので、child作成に失敗したら自動fixをスキップする方が安全です。修正案
// ADR task 4 fix_state = create_fix_commit(pr_info.pr_number, &poll_result.findings); - // Stage 3: takt analysis + fix loop - pre_takt_cid = crate::runner::capture_commit_id(); - log_info(&format!("[state] pre_takt_commit_id: {:?}", pre_takt_cid)); - takt_succeeded = run_takt(takt_config); - log_info(&format!("[state] takt_succeeded: {}", takt_succeeded)); - if !takt_succeeded { - log_info("takt ワークフロー失敗 (非致命的: ポーリング結果はそのまま報告)"); + if fix_state.is_created() { + // Stage 3: takt analysis + fix loop + pre_takt_cid = crate::runner::capture_commit_id(); + log_info(&format!("[state] pre_takt_commit_id: {:?}", pre_takt_cid)); + takt_succeeded = run_takt(takt_config); + log_info(&format!("[state] takt_succeeded: {}", takt_succeeded)); + if !takt_succeeded { + log_info("takt ワークフロー失敗 (非致命的: ポーリング結果はそのまま報告)"); + } + } else { + log_info("[state] fix commit 作成失敗: 元 commit 保護のため takt をスキップ"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/stages/monitor.rs` around lines 56 - 63, The code must stop the takt stage when create_fix_commit returns None: after calling create_fix_commit (assigning fix_state), check whether fix_state is None and if so log an error indicating fix commit creation failed, set takt_succeeded to false (or return/exit the stage) and do not call run_takt; only call crate::runner::capture_commit_id() and run_takt when fix_state is Some to avoid running takt edits that could amend the original commit. Ensure you reference the existing fix_state variable and the functions create_fix_commit and run_takt when implementing this guard.
🧹 Nitpick comments (2)
src/cli-pr-monitor/src/stages/repush.rs (2)
629-639: 本番 cleanup 経路（try_abandon_empty_fix_commit）を直接叩く形に変えた方がリグレッション検知が厚くなります。現状、統合テストは
diff_at_is_empty()を自前で呼んだ後に直接jj abandonを実行しています（lines 630, 633-639）。これだと production のcrate::fix_commit::try_abandon_empty_fix_commit（repush.rsline 159 から呼ばれる）内部の fail-safe 分岐 — diff が空でない時に abandon をスキップしてログのみ出す挙動 — が統合テストのカバレッジから外れます。RepushAction::CleanupEmptyFixCommitのエンドツーエンド動作としてはtry_abandon_empty_fix_commitをそのまま呼ぶ方が、本 PR で追加した経路をちゃんと通すという意味で退行防止になります。♻️ 提案: 本番関数を直接呼ぶ形に寄せる
- // diff_at_is_empty で true (空 child) を確認 - assert!(diff_at_is_empty(), "no-op 時の @ は diff 空"); - - // jj abandon で片付け - let abandon_ok = StdCommand::new("jj") - .args(["abandon"]) - .current_dir(repo_dir) - .status() - .expect("jj abandon 失敗") - .success(); - assert!(abandon_ok, "jj abandon が成功すること"); + // diff_at_is_empty で true (空 child) を確認 + assert!(diff_at_is_empty(), "no-op 時の @ は diff 空"); + + // 本番 cleanup 経路を通す (fail-safe 分岐含め production と同じ関数を使う) + crate::fix_commit::try_abandon_empty_fix_commit( + "integration_test:", + Some(&pre_created_cid), + );この置換ができれば、後段の
let _ = pre_created_cid;（line 698）も不要になります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/stages/repush.rs` around lines 629 - 639, Replace the test's manual diff_at_is_empty() + direct StdCommand "jj abandon" sequence with invoking the production cleanup path by calling crate::fix_commit::try_abandon_empty_fix_commit (the same code exercised by RepushAction::CleanupEmptyFixCommit) using the test's repo_dir and relevant context; ensure you pass the same parameters the action would and assert on its return/side-effects instead of running the external command, and then remove the now-unnecessary let _ = pre_created_cid; cleanup.
534-538: fix commit メッセージ文字列のハードコードはリテラル重複になりがちです。
"fix(review): apply CodeRabbit fixes for#99"（line 535）と"fix(review)"（line 687）は production のcreate_fix_commitが生成するメッセージ形式に依存しています。メッセージ書式を変えた瞬間に 2 箇所の統合テストが同時に壊れるため、fix_commit.rs側にメッセージ生成 helper（例:pub(crate) fn fix_commit_message(pr: u64) -> String）か定数プレフィックスを用意して、production とテストの双方から参照する形にすると安全です。現状のままでも機能しますが、将来のメンテ時に気付ければ十分レベルの nit です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/stages/repush.rs` around lines 534 - 538, Tests hard-code the full fix commit string which duplicates the production format from create_fix_commit; add a single shared helper in fix_commit.rs (e.g., pub(crate) fn fix_commit_message(pr: u64) -> String or a pub(crate) const FIX_COMMIT_PREFIX: &str plus a formatting helper) and update create_fix_commit to use it, then change this test in repush.rs to call that helper (e.g., assert!(log_str.contains(fix_commit_message(99)) or contains(&format!("{} #{}", FIX_COMMIT_PREFIX, 99))); this ensures production and tests reference the same message format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/adr-022-automation-responsibility-separation.md`:
- Around line 87-91: 本文の「bookmark / tag への介入」を全面禁止とすると
push_jj_bookmark::advance_jj_bookmarks
による既存bookmarkの前進が禁止に見えるため、ADRに境界を明記してください。具体的には ADR
ファイル（adr-022）内の該当箇所で「禁止される操作＝bookmark/tag
の新規作成・名前変更（rename）・任意の移動や改変」であることを明記し、対照的に
push_jj_bookmark::advance_jj_bookmarks
のような「既存bookmarkをpush対象のコミットへ同期（自動前進）する操作」は別途許可される例外として明示してください。これにより「生成・rename・任意移動」と「既存bookmarkの同期（advance_jj_bookmarks）」を区別して記述してください。
In `@src/cli-pr-monitor/src/fix_commit.rs`:
- Around line 94-99: The function try_abandon_empty_fix_commit currently logs
commit_id but always abandons the current '@' via run_cmd_direct("jj",
&["abandon"], ...); change it to target the recorded commit when commit_id is
Some by passing that id as an argument to jj (e.g., run_cmd_direct("jj",
&["abandon", commit_id], ...)), keeping the original behavior only when
commit_id is None; locate this logic in try_abandon_empty_fix_commit and update
the run_cmd_direct call and any label handling so the actual recorded fix commit
(not whatever '@' currently points to) is abandoned when appropriate, leaving
diff_at_is_empty() check intact.
In `@src/cli-pr-monitor/src/stages/monitor.rs`:
- Around line 78-80: When cleaning up a Created fix commit, don't pass None —
retrieve the commit ID stored in FixCommitState::Created from fix_state and pass
that ID into crate::fix_commit::try_abandon_empty_fix_commit instead of None so
the function can verify "current @ is the created fix child" before abandoning;
update the else-if branch that checks fix_state.is_created() to extract the
stored commit id from fix_state and supply it as the second argument to
try_abandon_empty_fix_commit.
In `@src/cli-pr-monitor/src/stages/repush.rs`:
- Around line 697-698: The binding and comment for pre_created_cid are
inaccurate: pre_created_cid is actually used earlier (read in the assert_eq! at
line 618) so the comment and the placeholder let _ = pre_created_cid; should be
removed; either delete both the let and its comment, or instead repurpose the
value by passing Some(&pre_created_cid) into try_abandon_empty_fix_commit(...)
as suggested in the earlier review so the variable is meaningfully used—update
the code to remove the misleading unused-note and adjust call sites if you
choose to reuse the value.
---
Outside diff comments:
In `@docs/todo.md`:
- Around line 93-123: Update the TODO text to reflect that task 4 is
implemented: change the status from "未実装" to "実装済み / E2E目視確認待ち" and mark ADR-022
as applied by replacing "ADR-022 追記予定" with "ADR-022 追記済み"; ensure the "タスク分解"
checklist reflects task 4 as completed (check the item about コミット分離ロジック実装 (案 A
ベース) and E2E 検証) and keep references to the implemented behavior
(pre_takt_commit_id handling, decide_repush == HasChange flow, jj new/jj abandon
handling) consistent with the prose so the documentation matches the current
implementation.
In `@src/cli-pr-monitor/src/stages/monitor.rs`:
- Around line 56-63: The code must stop the takt stage when create_fix_commit
returns None: after calling create_fix_commit (assigning fix_state), check
whether fix_state is None and if so log an error indicating fix commit creation
failed, set takt_succeeded to false (or return/exit the stage) and do not call
run_takt; only call crate::runner::capture_commit_id() and run_takt when
fix_state is Some to avoid running takt edits that could amend the original
commit. Ensure you reference the existing fix_state variable and the functions
create_fix_commit and run_takt when implementing this guard.
In `@src/cli-pr-monitor/src/stages/push.rs`:
- Around line 22-29: The jj check should mirror the argv parsing used elsewhere
(e.g., push_to_remote) by splitting on whitespace and examining the first token;
replace the starts_with("jj ") check with extracting first =
push_command.split_whitespace().next(), normalize by stripping a trailing ".exe"
(e.g., first.trim_end_matches(".exe")), then compare equality to "jj" and call
advance_jj_bookmarks() when it matches; keep the existing error handling around
advance_jj_bookmarks() unchanged.
---
Nitpick comments:
In `@src/cli-pr-monitor/src/stages/repush.rs`:
- Around line 629-639: Replace the test's manual diff_at_is_empty() + direct
StdCommand "jj abandon" sequence with invoking the production cleanup path by
calling crate::fix_commit::try_abandon_empty_fix_commit (the same code exercised
by RepushAction::CleanupEmptyFixCommit) using the test's repo_dir and relevant
context; ensure you pass the same parameters the action would and assert on its
return/side-effects instead of running the external command, and then remove the
now-unnecessary let _ = pre_created_cid; cleanup.
- Around line 534-538: Tests hard-code the full fix commit string which
duplicates the production format from create_fix_commit; add a single shared
helper in fix_commit.rs (e.g., pub(crate) fn fix_commit_message(pr: u64) ->
String or a pub(crate) const FIX_COMMIT_PREFIX: &str plus a formatting helper)
and update create_fix_commit to use it, then change this test in repush.rs to
call that helper (e.g., assert!(log_str.contains(fix_commit_message(99)) or
contains(&format!("{} #{}", FIX_COMMIT_PREFIX, 99))); this ensures production
and tests reference the same message format.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d3010bd-ca03-4307-9532-e562d2250a44
📒 Files selected for processing (9)
docs/adr/adr-022-automation-responsibility-separation.mddocs/todo.mdsrc/cli-pr-monitor/src/fix_commit.rssrc/cli-pr-monitor/src/main.rssrc/cli-pr-monitor/src/runner.rssrc/cli-pr-monitor/src/stages/monitor.rssrc/cli-pr-monitor/src/stages/push.rssrc/cli-pr-monitor/src/stages/push_jj_bookmark.rssrc/cli-pr-monitor/src/stages/repush.rs
Comment on lines
+87
to
+91
| **依然として禁止される内容**: | ||
|
|
||
| - 既存 commit (= 人間が意図を込めた元 PR commit) の description 書き換え | ||
| - PR title / PR body の書き換え | ||
| - bookmark / tag への介入 |
There was a problem hiding this comment.
bookmark auto-advanceとの境界を明記してください。
ここで「bookmark / tag への介入」を全面禁止にすると、push_jj_bookmark::advance_jj_bookmarksによる既存bookmark前進もADR違反に見えます。禁止対象が「生成・rename・任意移動」なのか、既存bookmarkをpush対象commitへ同期する操作は別ADRで許可済みなのかを明文化してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/adr-022-automation-responsibility-separation.md` around lines 87 -
91, 本文の「bookmark / tag への介入」を全面禁止とすると push_jj_bookmark::advance_jj_bookmarks
による既存bookmarkの前進が禁止に見えるため、ADRに境界を明記してください。具体的には ADR
ファイル（adr-022）内の該当箇所で「禁止される操作＝bookmark/tag
の新規作成・名前変更（rename）・任意の移動や改変」であることを明記し、対照的に
push_jj_bookmark::advance_jj_bookmarks
のような「既存bookmarkをpush対象のコミットへ同期（自動前進）する操作」は別途許可される例外として明示してください。これにより「生成・rename・任意移動」と「既存bookmarkの同期（advance_jj_bookmarks）」を区別して記述してください。
Comment on lines
+94
to
+99
| pub(crate) fn try_abandon_empty_fix_commit(context: &str, commit_id: Option<&str>) { | ||
| if diff_at_is_empty() { | ||
| let label = commit_id.map_or_else(String::new, |id| format!(" ({})", id)); | ||
| log_info(&format!("[action] {} 空 fix commit を abandon{}", context, label)); | ||
| let (ok, out) = run_cmd_direct("jj", &["abandon"], &[], JJ_CMD_TIMEOUT_SECS); | ||
| if !ok { |
There was a problem hiding this comment.
abandon対象を記録済みfix commitに固定してください。
commit_idを受け取っていますが、現状はログ表示だけで、実際には現在の@をjj abandonします。taktや別処理で@が移動していると、空の別commitを削除し、作成済みfix commitを残す可能性があります。
修正案
pub(crate) fn try_abandon_empty_fix_commit(context: &str, commit_id: Option<&str>) {
+ if let Some(expected) = commit_id {
+ match capture_commit_id() {
+ Some(current) if current == expected => {}
+ Some(current) => {
+ log_info(&format!(
+ "[warn] {} abandon 対象不一致のためスキップ: expected={}, current={}",
+ context, expected, current
+ ));
+ return;
+ }
+ None => {
+ log_info(&format!(
+ "[warn] {} current commit id を確認できないため abandon をスキップ",
+ context
+ ));
+ return;
+ }
+ }
+ }
+
if diff_at_is_empty() {
let label = commit_id.map_or_else(String::new, |id| format!(" ({})", id));
log_info(&format!("[action] {} 空 fix commit を abandon{}", context, label));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli-pr-monitor/src/fix_commit.rs` around lines 94 - 99, The function
try_abandon_empty_fix_commit currently logs commit_id but always abandons the
current '@' via run_cmd_direct("jj", &["abandon"], ...); change it to target the
recorded commit when commit_id is Some by passing that id as an argument to jj
(e.g., run_cmd_direct("jj", &["abandon", commit_id], ...)), keeping the original
behavior only when commit_id is None; locate this logic in
try_abandon_empty_fix_commit and update the run_cmd_direct call and any label
handling so the actual recorded fix commit (not whatever '@' currently points
to) is abandoned when appropriate, leaving diff_at_is_empty() check intact.
- pre-takt で fix commit を `jj new -m` で事前作成し、takt が @ を amend する ことで fix 内容が自動的に child commit へ分離される仕組みを導入 - FixCommitState 型で (None / Created) を明示管理し、post-takt で decide_repush_action により AutoPush / UserConfirm / Cleanup を分岐 - diff_at_is_empty は jj `empty` template 利用で --stat 出力差分に非依存 - push.rs を finalize_commit_structure / push_to_remote に責務分離 - ADR-022 に「新規 child commit への自己記述」の例外条項を追記 - docs/todo.md task 4 の severity=none 記述を「分離は実施、push のみ skip」 に更新 統合テスト 2 件追加 (HasChange 時の 2 commit 構造、NoChange 時の cleanup)。
aloekun
force-pushed
the
feat/separate-fix-commit
branch
from
April 20, 2026 13:35
177c9e1 to
41e36a6
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/cli-pr-monitor/src/stages/repush.rs (1)
158-160:context文字列の末尾コロンだけ他と揃えておくと、ログ grep が楽です。他の context は
"takt 未完了:"/"fix_state=Created:"のように末尾コロンで揃えられていますが、前者は日本語ラベル、後者はfix_state=Created:と key=value 風で粒度が揃っていません。try_abandon_empty_fix_commit側は頭に[action]/[warn]を付けるだけなので、呼び出し側で簡潔なカテゴリ名（例:"no_change_cleanup","takt_incomplete"など）に揃えると grep / log 集計がしやすいかと。任意です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/stages/repush.rs` around lines 158 - 160, 呼び出し側のログ context を他と揃えて簡潔なカテゴリ名＋末尾コロンに統一してください: RepushAction::CleanupEmptyFixCommit の try_abandon_empty_fix_commit 呼び出し（現在の "fix_state=Created:"）をより短いカテゴリ名（例 "no_change_cleanup:"）に変更して末尾にコロンを付け、他のコンテキスト（"takt 未完了:" 等）と同じ形式に揃えてください。これにより try_abandon_empty_fix_commit のログカテゴリが一貫します。src/cli-pr-monitor/src/fix_commit.rs (1)
120-132:jj abandonに対象 commit id を明示すると更に安全です（任意）。直前に
capture_commit_id() == expectedをチェックしているので実害は出にくいですが、jj abandonを引数なしで呼ぶと「その瞬間の@」が対象になるため、capture と abandon の間で何らかの理由で@が移動するとズレます（現行フローでは同期的なので通常起きませんが、将来 takt や別 hook が非同期割り込みする場合に備えた防御）。commit_idがSomeのケースに限り、jj abandon <id>を渡しておくと意図が型からも読めて fail-closed になります。♻️ 提案
- let (ok, out) = run_cmd_direct("jj", &["abandon"], &[], JJ_CMD_TIMEOUT_SECS); + let (ok, out) = match commit_id { + Some(id) => run_cmd_direct("jj", &["abandon", id], &[], JJ_CMD_TIMEOUT_SECS), + None => run_cmd_direct("jj", &["abandon"], &[], JJ_CMD_TIMEOUT_SECS), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/fix_commit.rs` around lines 120 - 132, abandon を引数なしで呼んでいる run_cmd_direct("jj", &["abandon"], ...) は、commit_id が Some の場合に対象を明示できないためリスクがあるので、diff_at_is_empty() ブロック内で commit_id をチェックし、Some(id) のときは run_cmd_direct("jj", &["abandon", &id], ..., JJ_CMD_TIMEOUT_SECS) のように commit id を引数として渡すように変更してください（commit_id.map_or_else や label の処理と整合する形で実装し、None の場合は従来どおり引数なしで呼ぶ）。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli-pr-monitor/src/fix_commit.rs`:
- Around line 53-58: The bullet formatting breaks if a finding's multi-line text
is inserted as-is; update the loop that builds the commit body (the for f in
findings { ... body.push_str(...) } block) to sanitize f.issue (and future
f.suggestion) by collapsing newlines into a single separator (e.g., a space or "
| ") and trimming extra whitespace before formatting; then use the sanitized
strings in the format call so each finding becomes a single-line bullet and
preserves readability for jj describe and git viewers.
---
Nitpick comments:
In `@src/cli-pr-monitor/src/fix_commit.rs`:
- Around line 120-132: abandon を引数なしで呼んでいる run_cmd_direct("jj", &["abandon"],
...) は、commit_id が Some の場合に対象を明示できないためリスクがあるので、diff_at_is_empty() ブロック内で
commit_id をチェックし、Some(id) のときは run_cmd_direct("jj", &["abandon", &id], ...,
JJ_CMD_TIMEOUT_SECS) のように commit id を引数として渡すように変更してください（commit_id.map_or_else や
label の処理と整合する形で実装し、None の場合は従来どおり引数なしで呼ぶ）。
In `@src/cli-pr-monitor/src/stages/repush.rs`:
- Around line 158-160: 呼び出し側のログ context を他と揃えて簡潔なカテゴリ名＋末尾コロンに統一してください:
RepushAction::CleanupEmptyFixCommit の try_abandon_empty_fix_commit 呼び出し（現在の
"fix_state=Created:"）をより短いカテゴリ名（例
"no_change_cleanup:"）に変更して末尾にコロンを付け、他のコンテキスト（"takt 未完了:" 等）と同じ形式に揃えてください。これにより
try_abandon_empty_fix_commit のログカテゴリが一貫します。
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71b583a5-dc18-4687-a82d-d49ee100c08e
📒 Files selected for processing (9)
docs/adr/adr-022-automation-responsibility-separation.mddocs/todo.mdsrc/cli-pr-monitor/src/fix_commit.rssrc/cli-pr-monitor/src/main.rssrc/cli-pr-monitor/src/runner.rssrc/cli-pr-monitor/src/stages/monitor.rssrc/cli-pr-monitor/src/stages/push.rssrc/cli-pr-monitor/src/stages/push_jj_bookmark.rssrc/cli-pr-monitor/src/stages/repush.rs
✅ Files skipped from review due to trivial changes (3)
- src/cli-pr-monitor/src/stages/push_jj_bookmark.rs
- docs/todo.md
- docs/adr/adr-022-automation-responsibility-separation.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli-pr-monitor/src/main.rs
- src/cli-pr-monitor/src/stages/push.rs
aloekun
added a commit
that referenced
this pull request
Apr 20, 2026
Resolved findings: - [Minor] src/cli-pr-monitor/src/fix_commit.rs:58 finding の issue 改行サニタイズ (sanitize_to_oneline 新設 + 単体テスト 2件) - [Minor] docs/todo.md bookmark auto-advance 境界明記を follow-up に追加 ※ Major 2件 (fix_commit.rs:99 の abandon ガード、monitor.rs:80 の commit_id 伝搬) および Minor 1件 (repush.rs:698 デッドコード) は pre-push review の takt 修正で 親 commit (feat/separate-fix-commit) に既に反映済み。
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli-pr-monitor/src/fix_commit.rs`:
- Around line 87-95: When capture_commit_id() returns None you must delete the
just-created empty child (created by the preceding `jj new`) before returning
FixCommitState::None; modify the None branch (where log_info is called) to
invoke the repository/command cleanup used for the "Created" or "NoChange" flows
(e.g., call the function that drops the empty child, e.g.,
remove_empty_child()/repo.drop_empty_child() or reuse the existing cleanup
routine) and only then return FixCommitState::None; keep the log_info call but
ensure the cleanup call is executed first so the orphaned child is removed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32eff74f-8d59-4759-a920-4e3513f725df
📒 Files selected for processing (2)
docs/todo.mdsrc/cli-pr-monitor/src/fix_commit.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/todo.md
Resolved findings: - [Major] src/cli-pr-monitor/src/fix_commit.rs:95 `capture_commit_id` 失敗時に orphan child を cleanup する fail-safe を追加 (統合テスト 1 件追加) - [Minor] src/cli-pr-monitor/src/fix_commit.rs:58 finding の issue 改行サニタイズ (sanitize_to_oneline 新設 + 単体テスト 2 件) - [Minor] docs/todo.md bookmark auto-advance 境界明記を follow-up に追加 ※ Major 2件 (fix_commit.rs abandon ガード、monitor.rs commit_id 伝搬) および Minor 1件 (repush.rs デッドコード) は pre-push review の takt 修正で 親 commit (feat/separate-fix-commit) に既に反映済み。
aloekun
force-pushed
the
feat/separate-fix-commit
branch
from
April 20, 2026 17:36
b5131a8 to
f300fd5
Compare
This was referenced Apr 21, 2026
6 tasks
This was referenced Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
jj new -mにより fix commit を事前作成し、takt の amend 先を child に向けることで PR 上で original + fix の 2 commit 構造を実現FixCommitStateenum (None/Created) +decide_repush_action純粋関数で (decision × fix_state × allow_auto) のマトリクスを型で表現diff_at_is_emptyを jjemptytemplate keyword 利用に切替 (--statの "0 files changed" 出力への依存を解消)push.rsをfinalize_commit_structure/push_to_remote/ 合成run_pushに責務分離Context
CodeRabbit 指摘に対する takt 自動修正は元コミットに amend されるため、PR 上は単一 commit に見え「未対応と誤認する」「修正前後の比較が取れない」「どの指摘にどの修正が対応したか辿れない」といった問題があった (docs/todo.md task 4)。
jj のセマンティクス上、post-takt で「既に amend された差分を child に移す」ルートは破綻する (rebase は diff を保存するため empty child に後から fix を入れられない)。そのため pre-takt で child を先に作り、takt の amend 先を制御する 設計に倒した。
ADR-022 との境界は「既存 commit の意味改変禁止 / 新規 commit への自己記述は許可 (メタ情報付与)」と整理し、自動化の痕跡が後続タスクへのフィードバックリソースとして残る形にした。
severity=noneの場合も分離は行い (push だけ skip)、ユーザーが child を見てからjj describeor manual push を選べる余地を残す。Validation
cargo test -p cli-pr-monitor: 88 pass, 4 ignored (新規 unit 11件:action_*6件 +fix_commit5件)cargo test -p cli-pr-monitor -- --ignored --test-threads=1: 4 pass (新規統合 2 件含む:integration_fix_commit_separation_creates_two_commits_on_has_change,integration_fix_commit_cleanup_on_no_change_restores_original_state)cargo clippy -p cli-pr-monitor --tests -- -D warnings: clean (既存のconfig.rsのbool_assert_comparison警告は本 PR 範囲外)pnpm pushpre-push review: verdict=APPROVE (6 iterations, 29m18s; simplicity + security 両方 approved)References
Summary by CodeRabbit
新機能
改善
ドキュメント