chore: conflicted bookmarks 棚卸し + push 前 bookmark 自動前進 - #49
Merged
Conversation
📝 WalkthroughWalkthroughプッシュ前にローカルjjブックマークをターゲットリビジョンまで自動で進める処理を新規モジュールとして追加し、run_push 実行前に条件付きで呼び出す統合を行った。ドキュメントの進行中タスクと完了履歴も更新。 Changes
Sequence Diagram(s)sequenceDiagram
participant PushRunner as Push Runner
participant JJ as JJ CLI
participant PushCmd as Push Command
PushRunner->>PushRunner: run_push 開始（config.command が "jj " か判定）
alt is "jj "
PushRunner->>JJ: jj log -r @ (テンプレ出力) -> target 決定（@ or `@-`）
JJ-->>PushRunner: template 出力
PushRunner->>JJ: jj log -r 'revset' for trunk/main/master ...（bookmark 名取得）
JJ-->>PushRunner: bookmark 名一覧（改行/カンマ）
alt bookmarks found
loop 各ブックマーク
PushRunner->>JJ: jj bookmark set -r <target> -- <name>
JJ-->>PushRunner: 成功/失敗
PushRunner->>PushRunner: 成功は通常ログ、失敗は情報ログで続行
end
else no bookmarks
PushRunner-->>PushRunner: 情報ログ（更新対象なし）
end
end
PushRunner->>PushCmd: 実際の push コマンド実行
PushCmd-->>PushRunner: push 結果
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (3)
src/cli-push-runner/src/stages/push_jj_bookmark.rs (3)
36-57: スタックした bookmark を巻き込む懸念。
(trunk()..target) & bookmarks()は target の祖先にあるすべての local bookmark を返すため、作業中にスタックしている複数 bookmark (例:feat/aの上にfeat/bを積んでいるケース) があると、feat/aまで target へ前進させてしまい履歴が崩れます。対象を「現在@から辿れる直近の bookmark のみ」「push 対象の bookmark のみ」等に絞り込むことを検討してください (例:heads((trunk()..target) & bookmarks())やjj git push --dry-runの対象 bookmark との突合)。また、最初に成功した revset でたとえ空でも
return Ok(dedup(...))している (Line 48) ため、trunk()が解決したが該当 bookmark が 0 件の場合、main/masterフォールバックには落ちません。これが意図どおりか確認してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs` around lines 36 - 57, get_bookmarks_in_range currently queries "(trunk()..target) & bookmarks()" which returns all ancestor-local bookmarks (risking stacked bookmarks) and also returns early on the first successful revset even if it yields zero bookmarks; update get_bookmarks_in_range to (1) restrict the revset to only head bookmarks by using heads((... ) & bookmarks()) or otherwise filter the parsed bookmarks from run_jj_log to only include the single nearest bookmark reachable from @ or to cross-check against the push-target list, and (2) change the logic around run_jj_log/parse_bookmarks_from_template/dedup so that if a revset succeeds but yields an empty list you do not return immediately but continue to the next revset (trunk → main → master) before finally falling back to logging and Ok(Vec::new()).
103-185: テスト観点:advance_jj_bookmarks/set_bookmarkの引数組み立ても検証したい。純関数 (
dedup/parse_bookmarks_from_template) には良い網羅がありますが、今回の主要バグ候補であるjj bookmark setの引数列や、main()/master()revset 構築文字列といった「外部コマンドに渡す文字列の組み立て」がテストされていません。Commandを直接使わず、argsを組み立てる純関数を切り出しておくとテスト容易性が上がります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs` around lines 103 - 185, テストが純関数に偏っていて、外部コマンドへ渡す引数列（`advance_jj_bookmarks` / `set_bookmark` が組み立てる `jj bookmark set` の args や `main()/master()` 用 revset 文字列）が検証されていないので、これらの文字列組み立てロジックを副作用なしの helper 関数に切り出してテスト可能にしてください：たとえば新しく `build_set_bookmark_args(...)` と `build_revset_for_branch(branch_name: &str)` のような純関数を `advance_jj_bookmarks` と `set_bookmark` から呼び出すようにリファクタし、元の関数は `Command` を実行するだけにすることで、ユニットテストから直接これらの helper を呼んで期待する args ベクタや revset 文字列をアサートできるようにします。
95-101:HashSetの代わりに短い Vec なら線形探索で十分。dedup 対象は通常数件〜十数件の bookmark 名なので、
HashSet+clone()よりVec::containsの方が簡潔でアロケーションも減らせます。ホットパスではないので必須ではありません。♻️ 提案
fn dedup(items: Vec<String>) -> Vec<String> { - let mut seen = std::collections::HashSet::new(); - items - .into_iter() - .filter(|s| seen.insert(s.clone())) - .collect() + let mut out = Vec::with_capacity(items.len()); + for s in items { + if !out.contains(&s) { + out.push(s); + } + } + out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs` around lines 95 - 101, The dedup function currently creates a HashSet and clones each string; replace that with a simple Vec-based linear check since input is small: in dedup, create seen: Vec<String>, iterate items.into_iter(), for each s check if seen.contains(&s) and if not push s into seen, then return seen—this removes the HashSet and the need to clone each item while preserving ordering and reducing allocations.
🤖 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-push-runner/src/stages/push_jj_bookmark.rs`:
- Around line 37-41: revset 配列 revsets 内で main() と master() は jj の組み込み revset
関数ではないためエラーになるので、main/master をブックマーク参照に置き換えてください：具体的には revsets を生成している箇所で
format!("(main()..{}) & bookmarks()", target) と format!("(master()..{}) &
bookmarks()", target) をそれぞれ format!("(bookmark(main)..{}) & bookmarks()",
target) と format!("(bookmark(master)..{}) & bookmarks()", target) のように修正して、既存の
trunk() entry はそのままにしてください（参照する識別子：revsets, target, trunk(), bookmarks()）。
- Around line 67-79: The call in set_bookmark builds the jj command arguments in
the wrong order causing -r <rev> to be treated as a positional bookmark name;
change the args passed to Command::new("jj") in the set_bookmark function so the
-r option comes before the positional bookmark name (use the sequence equivalent
to ["bookmark","set","-r", target, "--", name] rather than the current
["bookmark","set","--", name, "-r", target]) so jj interprets the revision
correctly.
---
Nitpick comments:
In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs`:
- Around line 36-57: get_bookmarks_in_range currently queries "(trunk()..target)
& bookmarks()" which returns all ancestor-local bookmarks (risking stacked
bookmarks) and also returns early on the first successful revset even if it
yields zero bookmarks; update get_bookmarks_in_range to (1) restrict the revset
to only head bookmarks by using heads((... ) & bookmarks()) or otherwise filter
the parsed bookmarks from run_jj_log to only include the single nearest bookmark
reachable from @ or to cross-check against the push-target list, and (2) change
the logic around run_jj_log/parse_bookmarks_from_template/dedup so that if a
revset succeeds but yields an empty list you do not return immediately but
continue to the next revset (trunk → main → master) before finally falling back
to logging and Ok(Vec::new()).
- Around line 103-185: テストが純関数に偏っていて、外部コマンドへ渡す引数列（`advance_jj_bookmarks` /
`set_bookmark` が組み立てる `jj bookmark set` の args や `main()/master()` 用 revset
文字列）が検証されていないので、これらの文字列組み立てロジックを副作用なしの helper 関数に切り出してテスト可能にしてください：たとえば新しく
`build_set_bookmark_args(...)` と `build_revset_for_branch(branch_name: &str)`
のような純関数を `advance_jj_bookmarks` と `set_bookmark` から呼び出すようにリファクタし、元の関数は `Command`
を実行するだけにすることで、ユニットテストから直接これらの helper を呼んで期待する args ベクタや revset
文字列をアサートできるようにします。
- Around line 95-101: The dedup function currently creates a HashSet and clones
each string; replace that with a simple Vec-based linear check since input is
small: in dedup, create seen: Vec<String>, iterate items.into_iter(), for each s
check if seen.contains(&s) and if not push s into seen, then return seen—this
removes the HashSet and the need to clone each item while preserving ordering
and reducing allocations.
🪄 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: 3cfe6f11-a637-4d0f-97a2-2c389b791093
📒 Files selected for processing (4)
docs/todo.mdsrc/cli-push-runner/src/stages/mod.rssrc/cli-push-runner/src/stages/push.rssrc/cli-push-runner/src/stages/push_jj_bookmark.rs
Comment on lines
+37
to
+41
| let revsets = [ | ||
| format!("(trunk()..{}) & bookmarks()", target), | ||
| format!("(main()..{}) & bookmarks()", target), | ||
| format!("(master()..{}) & bookmarks()", target), | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jj revset functions: is main()ormaster()a built-in revset function, or onlytrunk()?
💡 Result:
Only trunk is a built-in revset function (specifically, a built-in alias) in Jujutsu (jj). Neither main nor master is a built-in revset function or alias. The trunk alias is defined in the default revsets.toml configuration as latest(remote_bookmarks(exact:"main", exact:"origin") | remote_bookmarks(exact:"master", exact:"origin") | ... | root), which tries remote bookmarks named "main", "master", or "trunk" on "origin" or "upstream" remotes, falling back to root. It is explicitly listed under "Built-in Aliases" in the official documentation, alongside others like mutable and visible. The comprehensive lists of functions and built-in aliases in the revset documentation do not include main or master.
Citations:
- 1: https://jj-vcs.github.io/jj/latest/revsets/
- 2: https://github.com/martinvonz/jj/blob/main/docs/revsets.md
- 3: https://docs.jj-vcs.dev/latest/revsets/
- 4: https://git.joshthomas.dev/mirrors/jj/commit/f2f5ded5f08a5db6cad7bb0f9a69dc6a3e843ee3
- 5: https://github.com/jj-vcs/jj/blob/9ff73e7f/cli/src/config/revsets.toml
- 6: https://github.com/jj-vcs/jj/blob/main/cli/src/config/revsets.toml
🏁 Script executed:
fd -t f "push_jj_bookmark.rs" --exec cat -n {}Repository: aloekun/claude-code-hook-test
Length of output: 6402
main()/master() は jj の revset 関数ではありません。修正が必要です。
Web 検索で確認したところ、jj に組み込みの revset 関数は trunk() のみで、main() や master() という関数は存在しません。したがって (main()..{target}) & bookmarks() と (master()..{target}) & bookmarks() は常に revset エラーで失敗し、trunk() が未設定な環境では自動更新が常にスキップされます。
bookmark 名を直接参照する構文に修正してください：
修正パッチ
let revsets = [
format!("(trunk()..{}) & bookmarks()", target),
- format!("(main()..{}) & bookmarks()", target),
- format!("(master()..{}) & bookmarks()", target),
+ format!("(main..{}) & bookmarks()", target),
+ format!("(master..{}) & bookmarks()", target),
];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs` around lines 37 - 41,
revset 配列 revsets 内で main() と master() は jj の組み込み revset
関数ではないためエラーになるので、main/master をブックマーク参照に置き換えてください：具体的には revsets を生成している箇所で
format!("(main()..{}) & bookmarks()", target) と format!("(master()..{}) &
bookmarks()", target) をそれぞれ format!("(bookmark(main)..{}) & bookmarks()",
target) と format!("(bookmark(master)..{}) & bookmarks()", target) のように修正して、既存の
trunk() entry はそのままにしてください（参照する識別子：revsets, target, trunk(), bookmarks()）。
aloekun
force-pushed
the
chore/bookmark-cleanup
branch
from
April 17, 2026 11:36
4f76df3 to
33b9222
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-push-runner/src/stages/push_jj_bookmark.rs`:
- Around line 43-57: The loop currently returns as soon as run_jj_log(...)
succeeds even if parse_bookmarks_from_template(&output) yields an empty Vec, so
the fallback to other revsets (and final trunk/main/master warning) never runs;
change the logic in the loop that iterates revsets so that on Ok(output) you
parse bookmarks (via parse_bookmarks_from_template), dedup them, and only return
Ok(dedup(bookmarks)) when the resulting Vec is non-empty—if the parsed bookmarks
is empty, continue to the next revset (same as on Err), and after the loop keep
the existing log_info(...) and Ok(Vec::new()) fallback behavior. Ensure you
reference run_jj_log, parse_bookmarks_from_template, dedup and the revsets loop
when making the change.
- Around line 27-34: determine_target_revision currently always returns "@-"
when the working root is empty, but that fails when "@" is the root commit
because "@-" doesn't exist; change determine_target_revision to return
Result<Option<String>, String> and, when you detect the empty case, probe
whether "@-" exists (e.g., call run_jj_log for "@-" and check for an
empty/missing result or error); if "@-" exists return
Ok(Some("@-".to_string())), otherwise return Ok(None) to indicate “skip bookmark
update”; update the caller (set_bookmark or wherever determine_target_revision
is used) to skip running jj bookmark set when it receives None instead of a
revision.
🪄 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: 0e7bec06-3b38-4fb5-b3a1-a2faaaf1ddde
📒 Files selected for processing (4)
docs/todo.mdsrc/cli-push-runner/src/stages/mod.rssrc/cli-push-runner/src/stages/push.rssrc/cli-push-runner/src/stages/push_jj_bookmark.rs
✅ Files skipped from review due to trivial changes (1)
- src/cli-push-runner/src/stages/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli-push-runner/src/stages/push.rs
- docs/todo.md
aloekun
force-pushed
the
chore/bookmark-cleanup
branch
from
April 17, 2026 13:28
33b9222 to
12c0674
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli-push-runner/src/stages/push_jj_bookmark.rs (1)
52-76: テンプレートの区切りを改行のみに統一すると解析が単純になります
local_bookmarks.map(|b| b.name()).join(",")+ 行内カンマ split という 2 段階パースは、1 行に複数 bookmark 名を収める都合で採用されていますが、jj テンプレートで 1 bookmark = 1 行に揃えてしまえばparse_bookmarks_from_templateはlines()のみで済み、将来的に bookmark 名仕様が変わった際の事故率も下がります。既存テストの期待値も単純化できます。♻️ 提案パッチ
for revset in &revsets { - let template = "local_bookmarks.map(|b| b.name()).join(\",\") ++ \"\\n\""; + let template = "local_bookmarks.map(|b| b.name() ++ \"\\n\").join(\"\")"; match run_jj_log(revset, template) {fn parse_bookmarks_from_template(raw: &str) -> Vec<String> { raw.lines() - .flat_map(|line| line.split(',')) .map(|s| s.trim().to_string()) .filter(|s| !s.is_empty()) .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs` around lines 52 - 76, The current code emits a comma-joined template and then splits by commas in parse_bookmarks_from_template, which is brittle; change the jj template in the loop (used in run_jj_log call) to produce one bookmark per line (e.g. "local_bookmarks.map(|b| b.name()).join(\"\\n\") ++ \"\\n\"") and update parse_bookmarks_from_template to parse only by raw.lines(), trimming each line and filtering empties (remove the .split(',') step) so each bookmark is one line; keep dedup and the surrounding logic 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-push-runner/src/stages/push_jj_bookmark.rs`:
- Around line 52-76: The current code emits a comma-joined template and then
splits by commas in parse_bookmarks_from_template, which is brittle; change the
jj template in the loop (used in run_jj_log call) to produce one bookmark per
line (e.g. "local_bookmarks.map(|b| b.name()).join(\"\\n\") ++ \"\\n\"") and
update parse_bookmarks_from_template to parse only by raw.lines(), trimming each
line and filtering empties (remove the .split(',') step) so each bookmark is one
line; keep dedup and the surrounding logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f691949f-3a78-4b5b-b50b-e5a8dfc4ed16
📒 Files selected for processing (4)
docs/todo.mdsrc/cli-push-runner/src/stages/mod.rssrc/cli-push-runner/src/stages/push.rssrc/cli-push-runner/src/stages/push_jj_bookmark.rs
✅ Files skipped from review due to trivial changes (1)
- src/cli-push-runner/src/stages/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli-push-runner/src/stages/push.rs
4 tasks
This was referenced Apr 18, 2026
6 tasks
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
Summary by CodeRabbit
New Features
Documentation
Tests