feat(cli-pr-monitor): bookmark 検出を @-/@-- まで拡張 (PR #54 の水平展開) - #55
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughこのPRは、 Changes
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~45 minutes 関連する可能性のあるPR
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli-pr-monitor/src/util.rs (2)
85-172: parse / query / select の3層分離は綺麗にまとまっており LGTM。
parse_bookmark_list_outputを純粋関数に分離したことで、trim / dedup / trunk 除外の仕様が単体テストで網羅できるようになっている点が良いです(特にparse_bookmark_list_output_excludes_trunk_bookmarksは create_pr.rs の--head誤補完の再発防止ガードとして効いています)。select_from_revsetsのクロージャ注入により I/O なしで fallback / short-circuit / 呼び出し順序まで検証できており(select_from_revsets_stops_at_first_hitで["@","@-"]までしか叩かないことを RefCell で観測)、get_jj_bookmarksの振る舞いがテストで縛れています。is_trunk_bookmarkを exact match にしてあるのでmain-featureのような feature ブランチが巻き込まれないのも妥当(該当テスト済)。1点だけ、
TRUNK_BOOKMARKS/is_trunk_bookmark/BOOKMARK_SEARCH_REVSETSは cli-push-runner・cli-merge-pipeline にも同等物が存在しており(Lines 98-99 のコメントもcli-push-runner/push_jj_bookmark.rs と同じリストを採用と明言)、3 クレートで定義が平行してぶら下がる状態になっています。docs/todo.md でも言及のある ADR-024 (共通 jj helper crate) のスコープに乗せる形で、後続 PR でjj-helpers的な共有クレートに集約するのが良さそうです(この PR で広げる必要はなし)。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/util.rs` around lines 85 - 172, Duplicate definitions of BOOKMARK_SEARCH_REVSETS, TRUNK_BOOKMARKS, is_trunk_bookmark, parse_bookmark_list_output, query_bookmarks_at and select_from_revsets exist across multiple crates; extract them into a shared crate (e.g., jj_helpers) and re-export them so other crates can import rather than redefine. Create a new crate/module, move the constants and pure functions (BOOKMARK_SEARCH_REVSETS, TRUNK_BOOKMARKS, is_trunk_bookmark, parse_bookmark_list_output, select_from_revsets, query_bookmarks_at, get_jj_bookmarks) with identical signatures and behavior, add pub visibility and documentation, update callers to use the shared crate imports, and run/update unit tests to import these symbols from the new crate to ensure no behavior/regression changes.
135-154:query_bookmarks_atの stderr 抑止方針を文書化推奨。
.stderr(Stdio::null())でjj logの stderr を捨てているため、jj 側の動作エラーや予期しない失敗がサイレントに握り潰されます。現在、local_bookmarks.map(|b| b.name()).join(",")は jj 0.40.0 まで安定したドキュメント化済みの構文ですが、将来テンプレート DSL が変更される場合に問題の原因特定が難しくなる懸念があります。
select_from_revsetsが空結果を「ヒットなし」として扱うため、テンプレート破損や jj バージョン非互換を観測できなくなる点は改善の余地があります。デバッグ性向上のため、少なくとも関数のドキュメントコメントに「stderr は意図的に抑止し、revset 不正や一時的な失敗は握り潰す」旨を明記しておくことを推奨します。必要に応じて、o.status.success() == falseの分岐で警告ログを落とすことも検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-pr-monitor/src/util.rs` around lines 135 - 154, The function query_bookmarks_at currently discards jj stderr via .stderr(Stdio::null()), which can hide template/DLS or execution errors; add a doc comment on query_bookmarks_at explaining that stderr is intentionally suppressed and that revset/template failures will be treated as “no hits”, and optionally modify the error branch (the match that returns Vec::new() when o.status.success() is false) to emit a warning via the project logger (or a debug log) including the command stderr/output to aid future debugging when jj templates or versions change.
🤖 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/util.rs`:
- Around line 85-172: Duplicate definitions of BOOKMARK_SEARCH_REVSETS,
TRUNK_BOOKMARKS, is_trunk_bookmark, parse_bookmark_list_output,
query_bookmarks_at and select_from_revsets exist across multiple crates; extract
them into a shared crate (e.g., jj_helpers) and re-export them so other crates
can import rather than redefine. Create a new crate/module, move the constants
and pure functions (BOOKMARK_SEARCH_REVSETS, TRUNK_BOOKMARKS, is_trunk_bookmark,
parse_bookmark_list_output, select_from_revsets, query_bookmarks_at,
get_jj_bookmarks) with identical signatures and behavior, add pub visibility and
documentation, update callers to use the shared crate imports, and run/update
unit tests to import these symbols from the new crate to ensure no
behavior/regression changes.
- Around line 135-154: The function query_bookmarks_at currently discards jj
stderr via .stderr(Stdio::null()), which can hide template/DLS or execution
errors; add a doc comment on query_bookmarks_at explaining that stderr is
intentionally suppressed and that revset/template failures will be treated as
“no hits”, and optionally modify the error branch (the match that returns
Vec::new() when o.status.success() is false) to emit a warning via the project
logger (or a debug log) including the command stderr/output to aid future
debugging when jj templates or versions change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ed8553d-bee3-4ee5-9ac0-08515f99e350
📒 Files selected for processing (2)
docs/todo.mdsrc/cli-pr-monitor/src/util.rs
PR #54 で cli-merge-pipeline に適用した revset 拡張 + trunk filter を cli-pr-monitor/util.rs にも移植。get_jj_bookmarks が Strategy B の PR 検索と create_pr.rs の --head 自動補完の両方に使われているため、両経路に効く。 - util.rs を 3 層構造 (parse / query / select) に分割 - BOOKMARK_SEARCH_REVSETS = ["@", "@-", "@--"] で近い順に探索 - TRUNK_BOOKMARKS = ["main", "master", "trunk", "develop"] を除外 - create_pr.rs の --head 自動補完も改善 (master を誤って --head に渡す問題が消える) - unit テスト 11 件追加 (parse / priority / trunk filter) docs/todo.md: task 7 完了削除 → 雑務 cleanup を task 7 に繰り下げ refs: PR #54, ADR-013
08adc9b to
b818709
Compare
… 更新) docs/todo.md の「セッション 247510ea 由来: 整備タスク群 (PR-A)」を反映。 - ADR-028 (新規): 外部可視成果物の生成コマンド (pnpm create-pr / pnpm merge-pr 等) の実行ゲート - ADR-021 (原則 5 追加): bookmark 検出標準 (BOOKMARK_SEARCH_REVSETS / TRUNK_BOOKMARKS) + option A/B/C 比較 - ADR-024 (本採用に格上げ): 3 箇所 port 完了 (cli-push-runner / cli-merge-pipeline / cli-pr-monitor) により早期達成 - ADR-019 (制約・可換性追記): CodeRabbit 無料枠 1h 3 回制約、ハイブリッド再定義、M5 不採用論拠 - CLAUDE.md: ADR-028 追加、ADR-024 試験運用マーカー除去 - docs/todo.md: PR-A タスク削除 + 後続リナンバー (PR-B/C/D → #7/8/9) refs: PR #54, PR #55, セッション 247510ea-3f24-4b87-8f68-3c860e1b1b4e
… 更新) docs/todo.md の「セッション 247510ea 由来: 整備タスク群 (PR-A)」を反映。 - ADR-028 (新規): 外部可視成果物の生成コマンド (pnpm create-pr / pnpm merge-pr 等) の実行ゲート - ADR-021 (原則 5 追加): bookmark 検出標準 (BOOKMARK_SEARCH_REVSETS / TRUNK_BOOKMARKS) + option A/B/C 比較 - ADR-024 (本採用に格上げ): 3 箇所 port 完了 (cli-push-runner / cli-merge-pipeline / cli-pr-monitor) により早期達成 - ADR-019 (制約・可換性追記): CodeRabbit 無料枠 1h 3 回制約、ハイブリッド再定義、M5 不採用論拠 - CLAUDE.md: ADR-028 追加、ADR-024 試験運用マーカー除去 - docs/todo.md: PR-A タスク削除 + 後続リナンバー (PR-B/C/D → #7/8/9) refs: PR #54, PR #55, セッション 247510ea-3f24-4b87-8f68-3c860e1b1b4e
… 更新) (#56) docs/todo.md の「セッション 247510ea 由来: 整備タスク群 (PR-A)」を反映。 - ADR-028 (新規): 外部可視成果物の生成コマンド (pnpm create-pr / pnpm merge-pr 等) の実行ゲート - ADR-021 (原則 5 追加): bookmark 検出標準 (BOOKMARK_SEARCH_REVSETS / TRUNK_BOOKMARKS) + option A/B/C 比較 - ADR-024 (本採用に格上げ): 3 箇所 port 完了 (cli-push-runner / cli-merge-pipeline / cli-pr-monitor) により早期達成 - ADR-019 (制約・可換性追記): CodeRabbit 無料枠 1h 3 回制約、ハイブリッド再定義、M5 不採用論拠 - CLAUDE.md: ADR-028 追加、ADR-024 試験運用マーカー除去 - docs/todo.md: PR-A タスク削除 + 後続リナンバー (PR-B/C/D → #7/8/9) refs: PR #54, PR #55, セッション 247510ea-3f24-4b87-8f68-3c860e1b1b4e
cli-pr-monitor / cli-merge-pipeline / cli-push-runner の 3 クレートで重複していた bookmark 検出ロジックを ADR-024 (本採用) に従い `src/lib-jj-helpers/` に集約。
## 新クレート
- `src/lib-jj-helpers/` 新設 (ADR-012 命名規約、ADR-026 workspace 準拠)
- 公開 API:
- 定数: `TRUNK_BOOKMARKS`, `BOOKMARK_SEARCH_REVSETS`
- 関数: `is_trunk_bookmark`, `parse_bookmark_list_output`, `query_bookmarks_at`, `select_from_revsets`, `get_jj_bookmarks`
- 型: `StderrMode { Silent, Piped(fn(&str)) }`
- 14 unit tests (fallback_log コールバック検証 2 件を新規追加)
## 設計方針
- **stderr ハンドリングを `StderrMode` で選択**: cli-pr-monitor は `Silent` (CI ログ汚染回避)、cli-merge-pipeline は `Piped(log_info)` (診断情報を出す)
- **log 関数は `fn(&str)` ポインタで注入**: 各クレート固有 prefix (`[post-pr-monitor]` / `[merge-pipeline]`) を崩さない
- **fallback_log は `Option<fn(&str)>`**: `@-` や `@--` で hit した場合のみ通知 (noise 抑制)
## 呼び出し側差し替え
- cli-pr-monitor (`util.rs`): `get_jj_bookmarks()` を lib 呼び出しに置換、重複テスト削除
- cli-merge-pipeline (`main.rs`): 同上、stderr は `Piped` で継続
- cli-push-runner (`push_jj_bookmark.rs`): `is_trunk_bookmark` のみ lib 借用、他ロジックは crate-local 保持 (`parse_bookmark_list_output` は `jj bookmark list` 出力用で semantics が異なる)
## 検証
- cargo test --workspace: 363 tests PASS (1 ignored)
- pnpm build:all: 全 9 exe ビルド成功
- cargo clippy: modified 4 クレートに warning なし
- 行数インパクト: +425 / -492 = net -67 行 (重複テスト集約効果)
## ADR 更新
- ADR-024: 実装フェーズを「実施済」に反映、`capture_commit_id` / `diff_is_empty` は将来 PR で段階的移設と明記
- ADR-021 原則 5: 「3 クレートで重複 → lib-jj-helpers に集約済」と完了反映
- ADR-028: PR-D 参照番号を #7 に更新 (PR-B/PR-C 完了で docs/todo.md がリナンバー)
refs: ADR-024, ADR-021, PR #56 (PR-A), PR #57 (PR-B), PR #54, PR #55
cli-pr-monitor / cli-merge-pipeline / cli-push-runner の 3 クレートで重複していた bookmark 検出ロジックを ADR-024 (本採用) に従い `src/lib-jj-helpers/` に集約。
## 新クレート
- `src/lib-jj-helpers/` 新設 (ADR-012 命名規約、ADR-026 workspace 準拠)
- 公開 API:
- 定数: `TRUNK_BOOKMARKS`, `BOOKMARK_SEARCH_REVSETS`
- 関数: `is_trunk_bookmark`, `parse_bookmark_list_output`, `query_bookmarks_at`, `select_from_revsets`, `get_jj_bookmarks`
- 型: `StderrMode { Silent, Piped(fn(&str)) }`
- 14 unit tests (fallback_log コールバック検証 2 件を新規追加)
## 設計方針
- **stderr ハンドリングを `StderrMode` で選択**: cli-pr-monitor は `Silent` (CI ログ汚染回避)、cli-merge-pipeline は `Piped(log_info)` (診断情報を出す)
- **log 関数は `fn(&str)` ポインタで注入**: 各クレート固有 prefix (`[post-pr-monitor]` / `[merge-pipeline]`) を崩さない
- **fallback_log は `Option<fn(&str)>`**: `@-` や `@--` で hit した場合のみ通知 (noise 抑制)
## 呼び出し側差し替え
- cli-pr-monitor (`util.rs`): `get_jj_bookmarks()` を lib 呼び出しに置換、重複テスト削除
- cli-merge-pipeline (`main.rs`): 同上、stderr は `Piped` で継続
- cli-push-runner (`push_jj_bookmark.rs`): `is_trunk_bookmark` のみ lib 借用、他ロジックは crate-local 保持 (`parse_bookmark_list_output` は `jj bookmark list` 出力用で semantics が異なる)
## 検証
- cargo test --workspace: 363 tests PASS (1 ignored)
- pnpm build:all: 全 9 exe ビルド成功
- cargo clippy: modified 4 クレートに warning なし
- 行数インパクト: +425 / -492 = net -67 行 (重複テスト集約効果)
## ADR 更新
- ADR-024: 実装フェーズを「実施済」に反映、`capture_commit_id` / `diff_is_empty` は将来 PR で段階的移設と明記
- ADR-021 原則 5: 「3 クレートで重複 → lib-jj-helpers に集約済」と完了反映
- ADR-028: PR-D 参照番号を #7 に更新 (PR-B/PR-C 完了で docs/todo.md がリナンバー)
refs: ADR-024, ADR-021, PR #56 (PR-A), PR #57 (PR-B), PR #54, PR #55
cli-pr-monitor / cli-merge-pipeline / cli-push-runner の 3 クレートで重複していた bookmark 検出ロジックを ADR-024 (本採用) に従い `src/lib-jj-helpers/` に集約。
## 新クレート
- `src/lib-jj-helpers/` 新設 (ADR-012 命名規約、ADR-026 workspace 準拠)
- 公開 API:
- 定数: `TRUNK_BOOKMARKS`, `BOOKMARK_SEARCH_REVSETS`
- 関数: `is_trunk_bookmark`, `parse_bookmark_list_output`, `query_bookmarks_at`, `select_from_revsets`, `get_jj_bookmarks`
- 型: `StderrMode { Silent, Piped(fn(&str)) }`
- 14 unit tests (fallback_log コールバック検証 2 件を新規追加)
## 設計方針
- **stderr ハンドリングを `StderrMode` で選択**: cli-pr-monitor は `Silent` (CI ログ汚染回避)、cli-merge-pipeline は `Piped(log_info)` (診断情報を出す)
- **log 関数は `fn(&str)` ポインタで注入**: 各クレート固有 prefix (`[post-pr-monitor]` / `[merge-pipeline]`) を崩さない
- **fallback_log は `Option<fn(&str)>`**: `@-` や `@--` で hit した場合のみ通知 (noise 抑制)
## 呼び出し側差し替え
- cli-pr-monitor (`util.rs`): `get_jj_bookmarks()` を lib 呼び出しに置換、重複テスト削除
- cli-merge-pipeline (`main.rs`): 同上、stderr は `Piped` で継続
- cli-push-runner (`push_jj_bookmark.rs`): `is_trunk_bookmark` のみ lib 借用、他ロジックは crate-local 保持 (`parse_bookmark_list_output` は `jj bookmark list` 出力用で semantics が異なる)
## 検証
- cargo test --workspace: 363 tests PASS (1 ignored)
- pnpm build:all: 全 9 exe ビルド成功
- cargo clippy: modified 4 クレートに warning なし
- 行数インパクト: +425 / -492 = net -67 行 (重複テスト集約効果)
## ADR 更新
- ADR-024: 実装フェーズを「実施済」に反映、`capture_commit_id` / `diff_is_empty` は将来 PR で段階的移設と明記
- ADR-021 原則 5: 「3 クレートで重複 → lib-jj-helpers に集約済」と完了反映
- ADR-028: PR-D 参照番号を #7 に更新 (PR-B/PR-C 完了で docs/todo.md がリナンバー)
refs: ADR-024, ADR-021, PR #56 (PR-A), PR #57 (PR-B), PR #54, PR #55
Summary
@-..@--) と trunk filter を cli-pr-monitor にも水平展開util.rsのget_jj_bookmarksは Strategy B の PR 検索とcreate_pr.rsの--head自動補完の両方に使われているため、両経路に効果が及ぶChanges
src/cli-pr-monitor/src/util.rsparse_bookmark_list_output(純粋): jj log 出力から bookmark 名を抽出、trunk 除外query_bookmarks_at(I/O): 指定 revset に対し jj log を実行select_from_revsets(優先度): 複数 revset を近い順に試行、最初のヒットで停止BOOKMARK_SEARCH_REVSETS = ["@", "@-", "@--"]で近い順に探索TRUNK_BOOKMARKS = ["main", "master", "trunk", "develop"]を PR 検索対象から除外select_from_revsetsはクロージャ注入で単体テスト可能docs/todo.md副次効果
create_pr.rsの--head自動補完で、@や@-が master を指す状態でも--head masterのような誤った引数が補完されなくなる (trunk filter 効果)Test Plan
cargo test -p cli-pr-monitorで 78 テスト pass (+11)parse_bookmark_list_output_*: 7 件select_from_revsets_*: 4 件is_trunk_bookmark_known_names_rejected: 1 件cargo test --workspaceで 375 テスト passcargo clippy -p cli-pr-monitor -- -D warnings非テスト部は警告ゼロ.claude/cli-pr-monitor.exe再ビルド済@空 /@-= master 状態で--head自動補完が発生しない (trunk filter 効果確認)References
Summary by CodeRabbit
リリースノート
ドキュメント
改善
テスト