feat(cli-pr-monitor): takt NoChange cleanup 後に @ を PR tip へ re-parent (task 6) - #67
Merged
Merged
Conversation
… (task 6) takt fix が NoChange で空 fix commit を abandon した後、@ が stale な空 commit の上に残り、次の jj new がそこに積まれる問題を解消する。abandon 成功後に単一の非 trunk local bookmark を PR tip として解決し、jj new -r <tip> で @ を PR tip 直下に戻す。 以下は fail-safe で skip する: - bookmark 0 件 / 複数件 (stacked PR 想定) - @- が既に PR tip と一致 (redundant な空 commit を作らない) - jj new -r <tip> 自体の失敗 (ログのみで処理継続) 同じフローは takt 未完了経路 (monitor.rs:80) の abandon にも適用される。 関連: PR #64 / PR #66 で連続発生した事例 参照: ADR-022 原則 5 (PR 包含 changeset の不変性)
📝 WalkthroughWalkthroughこのPRは、cli-pr-monitorの空の修正コミットクリーンアップ（タスク6）の実装を完了しています。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as fix_commit.rs
participant Resolve as push_jj_bookmark.rs
participant JJ as jj CLI
participant Log as jj log
Client->>JJ: jj abandon (empty commit)
JJ-->>Client: success/failure
alt abandon successful
Client->>Resolve: resolve_pr_tip_commit_id()
Resolve->>JJ: jj bookmark list (non-trunk)
JJ-->>Resolve: bookmarks
alt exactly 1 bookmark found
Resolve->>Log: jj log -r <bookmark> commit_id
Log-->>Resolve: commit_id
Resolve-->>Client: Some(commit_id)
Client->>Log: check parent_commit_id_is(pr_tip)
Log-->>Client: false (needs reparent)
Client->>JJ: jj new -r <pr_tip>
JJ-->>Client: reparent complete
else multiple or zero bookmarks
Resolve-->>Client: None
Client->>Client: skip reparent
end
else abandon failed
Client->>Client: return early
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
src/cli-pr-monitor/src/fix_commit.rs (2)
437-444: nit:CwdRestore構造体が各統合テストで重複定義されています。
integration_try_abandon_reparents_at_to_pr_tip_after_cleanupとintegration_try_abandon_skips_reparent_with_multiple_bookmarks(および既存integration_try_abandon_empty_fix_commit_without_id_drops_orphan_child) で同一のCwdRestorestruct +Dropimpl が繰り返されています。#[cfg(test)] mod testsの先頭に 1 つ抽出すると DRY になります。本 PR のスコープ外でもよく、優先度は低です。Also applies to: 549-556
🤖 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 437 - 444, Multiple integration tests duplicate the CwdRestore struct and its Drop impl; extract a single shared definition into the tests module to avoid repetition. Remove the duplicated CwdRestore struct + impl from the individual tests (those in integration_try_abandon_reparents_at_to_pr_tip_after_cleanup, integration_try_abandon_skips_reparent_with_multiple_bookmarks, and integration_try_abandon_empty_fix_commit_without_id_drops_orphan_child) and add one definition of struct CwdRestore { original: std::path::PathBuf } with its Drop impl (fn drop calling std::env::set_current_dir(&self.original)) at the top of the #[cfg(test)] mod tests so all tests can reuse it.
156-211: reparent ロジックの fail-safe 設計は妥当。3 段階の skip (PR tip 未解決 /
@-既一致 /jj new失敗) がすべてログのみで非致命に倒されており、try_abandon_empty_fix_commitの呼び出し側への副作用がない点は良好です。parent_commit_id_isが失敗時falseを返して reparent を試行するフォールバック挙動も docstring で明示されています。nit:
resolve_pr_tip_commit_idが 0 件 / 複数件 / 取得失敗のいずれかでNoneになるため、ログ上その区別がつきません。将来 triage の必要が出たらresolve_pr_tip_commit_idをResult<Option<String>, &'static str>相当の返りに変えて理由付きログを残す余地がありますが、現状の運用では優先度低です。🤖 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 156 - 211, The PR nit requests more informative logging when resolve_pr_tip_commit_id() fails: change resolve_pr_tip_commit_id to return a Result<Option<String>, String> (or a small enum) so it can signal concrete failure reasons (no bookmarks / ambiguous / exec error), update reparent_at_to_pr_tip to match on Ok(Some(id)) / Ok(None) / Err(reason) and log the specific reason when skipping re-parent, and adjust any callers (e.g., try_abandon_empty_fix_commit) to handle the new signature; keep parent_commit_id_is and the existing jj new error handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli-pr-monitor/src/fix_commit.rs`:
- Around line 437-444: Multiple integration tests duplicate the CwdRestore
struct and its Drop impl; extract a single shared definition into the tests
module to avoid repetition. Remove the duplicated CwdRestore struct + impl from
the individual tests (those in
integration_try_abandon_reparents_at_to_pr_tip_after_cleanup,
integration_try_abandon_skips_reparent_with_multiple_bookmarks, and
integration_try_abandon_empty_fix_commit_without_id_drops_orphan_child) and add
one definition of struct CwdRestore { original: std::path::PathBuf } with its
Drop impl (fn drop calling std::env::set_current_dir(&self.original)) at the top
of the #[cfg(test)] mod tests so all tests can reuse it.
- Around line 156-211: The PR nit requests more informative logging when
resolve_pr_tip_commit_id() fails: change resolve_pr_tip_commit_id to return a
Result<Option<String>, String> (or a small enum) so it can signal concrete
failure reasons (no bookmarks / ambiguous / exec error), update
reparent_at_to_pr_tip to match on Ok(Some(id)) / Ok(None) / Err(reason) and log
the specific reason when skipping re-parent, and adjust any callers (e.g.,
try_abandon_empty_fix_commit) to handle the new signature; keep
parent_commit_id_is and the existing jj new error handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2199fcc-dece-415a-8132-82de017f8344
📒 Files selected for processing (4)
docs/todo.mdsrc/cli-pr-monitor/src/fix_commit.rssrc/cli-pr-monitor/src/stages/mod.rssrc/cli-pr-monitor/src/stages/push_jj_bookmark.rs
5 tasks
aloekun
added a commit
that referenced
this pull request
Apr 22, 2026
…68) * feat(cli-pr-monitor): observer mode で post-pr review を並行通知化 (task 2) ## 主要変更 - `cli-pr-monitor --observe` サブコマンドを新設 (read-only 観測パス) - `pr-monitor-state.json` を 5 秒間隔ポーリング - `action != continue_monitoring` 検出で state 全文を stdout に出して exit 0 - `notified=true` はサイレント exit (Claude Code 再起動時の重複防止) - 10 分タイムアウトで exit 1 (orphan OK) - `decide()` を pure function として切り出し、7 パターンの unit test を追加 - `poll.rs`: iteration を跨いで `notified` flag を preserve (`PrMonitorState::new` が毎回 false リセットする挙動を修正) - `start_monitoring` 冒頭で state を明示初期化 (新セッション開始時の reset) - `package.json` に `observe-pr` / `mark-notified` スクリプト復活 - ADR-018 に 2026-04-22 追記で仕組みを反映 ## 付随: todo.md cleanup - task 4 (post-pr-review workflow の push 反映確認) を削除 理由: task 3 (PR #61) + task 6 (PR #67) の bookmark auto-advance で gap 解消済み ADR-022 の「主フロー vs 観測」責務分離原則とも整合 - task 2 を実装完了状態に縮退 (E2E 検証のみ残置) ## 参照 - ADR-018 追記 — observer モード仕様 - ADR-022 — 主フロー 100% 機械的 / observer は read-only side effect - docs/todo.md task 2 ## テスト - `cargo test -p cli-pr-monitor`: 97 passed / 0 failed / 7 ignored - `cargo clippy --release`: warning なし - 手動検証: 終端状態検出 (exit 0) / notified=true サイレント exit の両分岐確認 * fix(review): apply CodeRabbit fixes for #68 Resolved findings: - [Major] src/cli-pr-monitor/src/stages/monitor.rs:38 observer 起動前に stale state を読ませない初期化位置にしてください。 * refactor(cli-pr-monitor): started_at の fallback を現在時刻にする (CodeRabbit nitpick) ## 変更 - `start_monitoring` の init_state で `pr_info.push_time.clone().unwrap_or_default()` を `unwrap_or_else(utc_now_iso8601)` に置き換え - `push_time=None` だと `started_at=""` になる観測性の問題を防御的に修正 ## 背景 PR #68 CodeRabbit 2 回目レビュー (Nitpick) の提案: 現状の `run_monitor_only` 経路では直前で `Some(utc_now_iso8601())` が設定されるため 到達しないが、将来 `start_monitoring` が他経路から直接呼ばれた場合に started_at が 空文字列になり observability を損なう恐れあり。`run_create_pr` 冒頭の早期 reset と 同じフォールバック (utc_now_iso8601) に揃える。 ## テスト - `cargo test -p cli-pr-monitor`: 97 pass
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.
概要
takt fix が NoChange で空 fix commit を abandon した後、
@が stale な空 commit の上に残り、次のjj newがそこに積まれる問題を解消する (task 6)。背景
PR #64 / PR #66 で連続発生。毎回
jj abandon <current>→jj abandon <stale>→jj new -r <PR-tip>の 3 ステップ手修正が必要だった。変更内容
fix_commit.rs::try_abandon_empty_fix_commit: abandon 成功後にreparent_at_to_pr_tipを呼び出すフローを追加push_jj_bookmark.rs::resolve_pr_tip_commit_id: 単一の非 trunk local bookmark を PR tip として解決@-が PR tip と一致する場合は redundant な空 commit を作らずスキップstages/mod.rs:push_jj_bookmarkをpub(crate)化 (fix_commit.rs から参照するため)テスト
integration_try_abandon_reparents_at_to_pr_tip_after_cleanup: メインシナリオで@-が PR tip に戻ることを実 jj で確認integration_try_abandon_skips_reparent_with_multiple_bookmarks: 複数 bookmark 時の fail-safe skip 挙動を確認残タスク
参照
Summary by CodeRabbit
リリースノート