fix(cli-push-runner): bookmark 乖離 fallback + 空 diff 時 takt スキップ - #50
Merged
Conversation
📝 WalkthroughWalkthroughpush-runnerの差分検出を三状態の戻り値に変更し、空差分時はtaktをスキップしてpushへ継続する制御を追加。push時のbookmark前進でローカルbookmark未検出時に Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline as Main Pipeline
participant Diff as Diff Stage
participant Takt as Takt Stage
participant Push as Push Stage
Pipeline->>Diff: run_diff(config)
Diff->>Diff: diff コマンド実行／出力解析
alt HasContent
Diff-->>Pipeline: DiffResult::HasContent
Pipeline->>Takt: run_takt (skip_takt=false)
Takt-->>Pipeline: takt 結果
Pipeline->>Push: run_push
else Empty
Diff-->>Pipeline: DiffResult::Empty
Pipeline->>Pipeline: set skip_takt = true
Pipeline->>Takt: takt をスキップ
Pipeline->>Push: run_push (直接進行)
else Error
Diff-->>Pipeline: DiffResult::Error
Pipeline-->>Pipeline: exit (diff failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
🧹 Nitpick comments (3)
docs/todo.md (2)
136-146: Note: タスク#4も本 PR で実質的にクローズ
advance_bookmarks_via_listの追加で「takt fix 後に @ が bookmark より先に進む」ケースはカバーされています。マージ時に完了履歴へ移すことを推奨します (本コメントは追跡用)。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/todo.md` around lines 136 - 146, Issue: takt fix can advance @ past bookmarks causing "No bookmarks found" on push; push_jj_bookmark::advance_jj_bookmarks() already moves bookmarks forward relative to trunk but doesn't handle bookmarks that are behind @. Fix: in src/cli-push-runner/src/stages/push.rs, before executing the push step add logic to explicitly align bookmarks to @ (for each relevant bookmark run the equivalent of `jj bookmark set <name> -r @`), or call/implement a helper (e.g., advance_bookmarks_via_list or a new function) that sets each bookmark's target to @ when the bookmark is older than @, then proceed with existing advance_jj_bookmarks() and push flow; ensure this runs only for the bookmarks affecting the push target.
148-158: Note: 本 PR の実装と記述の整合確認本タスク
#5は「allow_empty_diff = true等のオプション追加を検討」と記述されていますが、本 PR の実装 (DiffResult::Empty→ 常に takt スキップ + push 続行) はオプション無しで挙動を固定化しています。この PR のマージ後はこのセクションを「完了履歴」に移すか、残課題として「オプション化の是非」だけに絞って更新すると、ドキュメントとコードの乖離を防げます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/todo.md` around lines 148 - 158, Update the docs/todo.md section for task 5 to match the PR's implemented behavior: state that DiffResult::Empty (handled in src/cli-push-runner/src/stages/diff.rs) now unconditionally skips takt review and proceeds to push, mark the checklist items related to changing that behavior as completed, and reduce the remaining TODO to a single item about whether to add a configuration flag (`allow_empty_diff` in push-runner-config.toml) (or alternatively note that optionization is deferred), so the doc no longer contradicts the code.src/cli-push-runner/src/stages/diff.rs (1)
47-53: 確認:Empty時に既存のoutput_pathが更新されない件空 diff のときは
output_pathへの書き込みをスキップしていますが、前回実行で書き出された古い diff ファイルが残る可能性があります。現状はEmpty→skip_takt = trueなので takt が参照せず問題になりませんが、将来 takt 以外の下流ステージがoutput_pathを読むようになった場合にステール参照の温床になり得ます。安全側に倒すならEmpty時に空ファイルを書く (または既存ファイルを削除する) ことを検討してください。現状の仕様なら、このケースでは書かないことをコメントに明記しておくだけでも可です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/stages/diff.rs` around lines 47 - 53, When output.is_empty() and the function returns DiffResult::Empty, ensure output_path is not left with a stale diff: either truncate/create an empty file at output_path or remove any existing file before returning; modify the branch that currently calls log_stage(...) and returns DiffResult::Empty to perform File::create(output_path) (or std::fs::remove_file(output_path)) so the downstream stages won't read stale data, and keep the log_stage call; alternatively, if choosing to preserve current behavior, add a clear comment next to the output.is_empty() check documenting that skipping write is intentional and why.
🤖 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 39-48: advance_bookmarks_via_list currently moves all non-trunk
local bookmarks returned by get_local_bookmarks_from_list() to the target, which
can incorrectly advance unrelated feature bookmarks; change it to filter the
bookmarks before calling apply_bookmarks: only include bookmarks that are
directly related to the current push target (e.g., bookmarks pointing at
obsolete ancestors of the target or on the current mutable heads range
determined via a jj query such as log -r 'heads(mutable())'), or restrict to
bookmarks that are on the immediate ancestor chain of @; after applying this
filter, call apply_bookmarks(&filtered_bookmarks, target, " (fallback)");
additionally, when the fallback path runs, emit log_info with the number and
names of the bookmarks considered so operators can observe what will be moved
(use get_local_bookmarks_from_list(), then log_info before/after filtering).
- Around line 136-150: parse_bookmark_list_output is currently extracting
bookmark names with line.split(':').next() which leaves conflict markers like
"name (conflicted)"; update the parser to first trim the line, then take the
first whitespace-separated token (e.g., line.split_whitespace().next()) and also
explicitly strip the " (conflicted)" suffix (trim_end_matches(" (conflicted)"))
before calling is_trunk_bookmark or collecting; keep TRUNK_BOOKMARKS and
is_trunk_bookmark as-is and ensure empty strings are still filtered out.
---
Nitpick comments:
In `@docs/todo.md`:
- Around line 136-146: Issue: takt fix can advance @ past bookmarks causing "No
bookmarks found" on push; push_jj_bookmark::advance_jj_bookmarks() already moves
bookmarks forward relative to trunk but doesn't handle bookmarks that are behind
@. Fix: in src/cli-push-runner/src/stages/push.rs, before executing the push
step add logic to explicitly align bookmarks to @ (for each relevant bookmark
run the equivalent of `jj bookmark set <name> -r @`), or call/implement a helper
(e.g., advance_bookmarks_via_list or a new function) that sets each bookmark's
target to @ when the bookmark is older than @, then proceed with existing
advance_jj_bookmarks() and push flow; ensure this runs only for the bookmarks
affecting the push target.
- Around line 148-158: Update the docs/todo.md section for task 5 to match the
PR's implemented behavior: state that DiffResult::Empty (handled in
src/cli-push-runner/src/stages/diff.rs) now unconditionally skips takt review
and proceeds to push, mark the checklist items related to changing that behavior
as completed, and reduce the remaining TODO to a single item about whether to
add a configuration flag (`allow_empty_diff` in push-runner-config.toml) (or
alternatively note that optionization is deferred), so the doc no longer
contradicts the code.
In `@src/cli-push-runner/src/stages/diff.rs`:
- Around line 47-53: When output.is_empty() and the function returns
DiffResult::Empty, ensure output_path is not left with a stale diff: either
truncate/create an empty file at output_path or remove any existing file before
returning; modify the branch that currently calls log_stage(...) and returns
DiffResult::Empty to perform File::create(output_path) (or
std::fs::remove_file(output_path)) so the downstream stages won't read stale
data, and keep the log_stage call; alternatively, if choosing to preserve
current behavior, add a clear comment next to the output.is_empty() check
documenting that skipping write is intentional and why.
🪄 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: 9c9be5cc-e7ad-4772-8057-b26510388a77
📒 Files selected for processing (5)
docs/todo.mdsrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/stages/diff.rssrc/cli-push-runner/src/stages/mod.rssrc/cli-push-runner/src/stages/push_jj_bookmark.rs
Comment on lines
+136
to
+150
| fn parse_bookmark_list_output(output: &str) -> Vec<String> { | ||
| output | ||
| .lines() | ||
| .filter(|line| !line.starts_with(' ') && !line.starts_with('\t')) | ||
| .filter_map(|line| line.split(':').next()) | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty() && !is_trunk_bookmark(s)) | ||
| .collect() | ||
| } | ||
|
|
||
| const TRUNK_BOOKMARKS: &[&str] = &["main", "master", "trunk", "develop"]; | ||
|
|
||
| fn is_trunk_bookmark(name: &str) -> bool { | ||
| TRUNK_BOOKMARKS.contains(&name) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jj bookmark list default output format conflicted bookmarks
💡 Result:
In Jujutsu (jj), the default output format of jj bookmark list for conflicted bookmarks lists the bookmark name followed by "(conflicted):", with old target revisions preceded by a "-" and new target revisions preceded by a "+". For example, it would look like: bookmark-name (conflicted): - old-rev-id ... + new-rev-id ... This format is used when both local and remote bookmarks have diverged. Use jj bookmark list -c to show only conflicted bookmarks. The output can be customized with --template using the CommitRef type.
Citations:
- 1: https://man.archlinux.org/man/extra/jujutsu/jj-bookmark-list.1.en
- 2: https://man.archlinux.org/man/jj-bookmark-list.1.en.txt
- 3: https://man.archlinux.org/man/jj-bookmark-list.1.en.raw
- 4: https://man.archlinux.org/man/jj-bookmark-list.1.en
🏁 Script executed:
# First, let's view the file and understand the context
cat -n src/cli-push-runner/src/stages/push_jj_bookmark.rs | head -160Repository: aloekun/claude-code-hook-test
Length of output: 6093
🏁 Script executed:
# Let's search for where parse_bookmark_list_output is used
rg "parse_bookmark_list_output" --type rust -B 3 -A 3Repository: aloekun/claude-code-hook-test
Length of output: 4014
🏁 Script executed:
# Let's also check how jj bookmark list is called
rg "bookmark list" --type rust -B 2 -A 2Repository: aloekun/claude-code-hook-test
Length of output: 1873
🏁 Script executed:
cat -n src/cli-push-runner/src/stages/push_jj_bookmark.rs | sed -n '160,250p'Repository: aloekun/claude-code-hook-test
Length of output: 2998
🏁 Script executed:
# Search for any tests related to conflicted
rg "conflicted" --type rust src/cli-push-runner/Repository: aloekun/claude-code-hook-test
Length of output: 55
🏁 Script executed:
cat -n src/cli-push-runner/src/stages/push_jj_bookmark.rs | sed -n '250,330p'Repository: aloekun/claude-code-hook-test
Length of output: 2207
🏁 Script executed:
# Let's check if there are any actual conflicted bookmark outputs in the tests or examples
rg "conflicted" --type rust -B 5 -A 5Repository: aloekun/claude-code-hook-test
Length of output: 55
🏁 Script executed:
# Let's verify the actual jj bookmark list output format more thoroughly
# by checking jj documentation or examples in the repo
find . -name "*.md" -o -name "*.txt" | xargs grep -l "bookmark list" 2>/dev/null | head -5Repository: aloekun/claude-code-hook-test
Length of output: 55
🏁 Script executed:
# Check if there are any integration tests or examples that show jj output
rg "jj bookmark" --type rust --type markdown -B 2 -A 2 | head -50Repository: aloekun/claude-code-hook-test
Length of output: 3563
parse_bookmark_list_output のコンフリクトマーカー処理を修正
jj bookmark list はコンフリクト状態の bookmark を name (conflicted): ... 形式で出力します。現在のパーサはコンフリクトマーカーを除去していないため、line.split(':').next() で "name (conflicted)" を抽出し、そのまま jj bookmark set -- "name (conflicted)" に渡ってコマンド失敗 / 誤動作します。
最低限、スペース手前までを取得する split_whitespace().next() か trim_end_matches(" (conflicted)") 等で対応してください。
備考：TRUNK_BOOKMARKS はハードコードされていますが、get_bookmarks_in_range は revset trunk() を使用するため、設定が反映される主パスと fallback パスで trunk 判定の挙動が異なります。これは将来の改善課題とできます。
🤖 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 136 - 150,
parse_bookmark_list_output is currently extracting bookmark names with
line.split(':').next() which leaves conflict markers like "name (conflicted)";
update the parser to first trim the line, then take the first
whitespace-separated token (e.g., line.split_whitespace().next()) and also
explicitly strip the " (conflicted)" suffix (trim_end_matches(" (conflicted)"))
before calling is_trunk_bookmark or collecting; keep TRUNK_BOOKMARKS and
is_trunk_bookmark as-is and ensure empty strings are still filtered out.
aloekun
force-pushed
the
fix/push-runner-bookmark-empty-diff
branch
from
April 17, 2026 17:13
5bb48ab to
9bef35c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cli-push-runner/src/stages/push_jj_bookmark.rs (1)
164-172:⚠️ Potential issue | 🟡 Minor
(conflicted)サフィックスが bookmark 名に残り、jj bookmark set失敗の可能性。
jj bookmark listはコンフリクト状態の bookmark をname (conflicted): - old + new形式で出力します。現在の実装はline.split(':').next()→trim()のみで、"feat/xyz (conflicted)"がそのまま収集され、フォールバック経路でjj bookmark set -r <target> -- "feat/xyz (conflicted)"に渡って失敗します。単一 feature bookmark がコンフリクトしているケース（fallback が作動する主シナリオの一つ）で確実に踏み得るパスです。また、
parse_bookmark_list_outputのテスト群にコンフリクト出力のフィクスチャが無いため、この回帰が補足されません。併せてテストも追加してください。🛠️ 提案修正（サフィックス除去 + 回帰テスト）
fn parse_bookmark_list_output(output: &str) -> Vec<String> { output .lines() .filter(|line| !line.starts_with(' ') && !line.starts_with('\t')) .filter_map(|line| line.split(':').next()) - .map(|s| s.trim().to_string()) + .map(|s| s.trim().trim_end_matches(" (conflicted)").to_string()) .filter(|s| !s.is_empty() && !is_trunk_bookmark(s)) .collect() }テスト追加例:
#[test] fn parse_bookmark_list_strips_conflicted_suffix() { let output = "\ feat/xyz (conflicted): - abc1234 desc + def5678 desc main: 111 desc "; assert_eq!(parse_bookmark_list_output(output), vec!["feat/xyz"]); }併記:
TRUNK_BOOKMARKSハードコードとget_bookmarks_in_rangeのtrunk()revset 依存との乖離は、前回コメント通り将来課題として残置で問題ありません。🤖 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 164 - 172, The parse_bookmark_list_output function is currently returning bookmark names with trailing " (conflicted)" which breaks subsequent jj bookmark set calls; update parse_bookmark_list_output to strip the " (conflicted)" suffix (and any other parenthetical suffixes) after extracting the name (i.e., after line.split(':').next() and trim()) so it yields the base bookmark name, and add a unit test (e.g., parse_bookmark_list_strips_conflicted_suffix) that feeds a conflicted bookmark fixture like "feat/xyz (conflicted): - abc... \n + def...\nmain: ..." and asserts the result contains "feat/xyz" only.
🧹 Nitpick comments (1)
src/cli-push-runner/src/main.rs (1)
41-46: 開始ログの整形が崩れる可能性があります（任意）。
has_diffが false の場合、"パイプライン開始: quality_gate → takt (...)"のように矢印と空白が残って不自然になります。三項全体（"diff → "を含む）を差し替える形式にすると読みやすくなります。💡 Suggestion
- log_info(&format!( - "パイプライン開始: quality_gate → {} takt ({}) → push", - if has_diff { "diff →" } else { "" }, - config.takt.workflow, - )); + log_info(&format!( + "パイプライン開始: quality_gate → {}takt ({}) → push", + if has_diff { "diff → " } else { "" }, + config.takt.workflow, + ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-push-runner/src/main.rs` around lines 41 - 46, The formatted start log can leave an extra arrow/space when has_diff is false; change the interpolation so the entire middle segment is chosen, e.g. compute a step string (using the existing has_diff/config.diff) like let diff_segment = if has_diff { "diff → " } else { "" } and then call log_info(&format!("パイプライン開始: quality_gate → {}takt ({}) → push", diff_segment, config.takt.workflow)); this ensures no stray arrow/extra space when diff is absent and uses the existing has_diff, config.diff, log_info, and config.takt.workflow symbols.
🤖 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/main.rs`:
- Around line 60-64: ログ出力に置換文字 (�) が混入しているため、該当メッセージを正しい日本語に置き換えてください:
DiffResult::Empty ブランチで呼ばれる log_info のメッセージを "diff が空のためレビューをスキップして push に進みます。"
に、DiffResult::Error ブランチで呼ばれる log_info のメッセージを "パイプライン中断: diff 取得失敗。"
のような正しい日本語表現に修正し、skip_takt 等の既存ロジックはそのまま維持してください。
---
Duplicate comments:
In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs`:
- Around line 164-172: The parse_bookmark_list_output function is currently
returning bookmark names with trailing " (conflicted)" which breaks subsequent
jj bookmark set calls; update parse_bookmark_list_output to strip the "
(conflicted)" suffix (and any other parenthetical suffixes) after extracting the
name (i.e., after line.split(':').next() and trim()) so it yields the base
bookmark name, and add a unit test (e.g.,
parse_bookmark_list_strips_conflicted_suffix) that feeds a conflicted bookmark
fixture like "feat/xyz (conflicted): - abc... \n + def...\nmain: ..." and
asserts the result contains "feat/xyz" only.
---
Nitpick comments:
In `@src/cli-push-runner/src/main.rs`:
- Around line 41-46: The formatted start log can leave an extra arrow/space when
has_diff is false; change the interpolation so the entire middle segment is
chosen, e.g. compute a step string (using the existing has_diff/config.diff)
like let diff_segment = if has_diff { "diff → " } else { "" } and then call
log_info(&format!("パイプライン開始: quality_gate → {}takt ({}) → push", diff_segment,
config.takt.workflow)); this ensures no stray arrow/extra space when diff is
absent and uses the existing has_diff, config.diff, log_info, and
config.takt.workflow symbols.
🪄 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: 37695e8d-3ae3-4eba-b627-93247324b6d1
📒 Files selected for processing (6)
docs/todo.mdsrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/stages/diff.rssrc/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 (2)
- src/cli-push-runner/src/stages/push.rs
- 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/diff.rs
- docs/todo.md
aloekun
force-pushed
the
fix/push-runner-bookmark-empty-diff
branch
from
April 17, 2026 17:34
9bef35c to
7cab2ae
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cli-push-runner/src/stages/push_jj_bookmark.rs (1)
164-172:⚠️ Potential issue | 🟠 Major
parse_bookmark_list_outputが conflicted マーカーを剥がしていません
jj bookmark listはコンフリクト状態の bookmark をname (conflicted): ...形式で出力します。現状の実装はline.split(':').next()→trim()のみなので、"feat/xyz (conflicted)"がそのまま bookmark 名として通過し、dispatch_bookmark_advanceが単一 bookmark と判定した場合にjj bookmark set -- "feat/xyz (conflicted)"が実行され、コマンド失敗または意図せぬ挙動になります。また
TRUNK_BOOKMARKSのハードコードと、主パスget_bookmarks_in_rangeの revsettrunk()の間で trunk 判定が乖離する件も残っています（設定で trunk を変えている環境では主パスと fallback で挙動が変わる）。最低限 conflicted マーカー除去は入れておきたいところです。🛠 Proposed fix
fn parse_bookmark_list_output(output: &str) -> Vec<String> { output .lines() .filter(|line| !line.starts_with(' ') && !line.starts_with('\t')) .filter_map(|line| line.split(':').next()) - .map(|s| s.trim().to_string()) + .map(|s| s.trim().trim_end_matches(" (conflicted)").to_string()) .filter(|s| !s.is_empty() && !is_trunk_bookmark(s)) .collect() }併せて
parse_bookmark_list_outputにname (conflicted): ...を食わせるテストを 1 本追加しておくと回帰防止になります。🤖 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 164 - 172, parse_bookmark_list_output currently leaves the " (conflicted)" marker in bookmark names causing downstream commands (e.g. dispatch_bookmark_advance) to fail; update parse_bookmark_list_output to strip the conflicted marker (remove trailing " (conflicted)" or any trailing parenthetical like " (…)" after extracting the name) and then trim before calling is_trunk_bookmark so trunk checks use the canonical name; also ensure this normalization matches how get_bookmarks_in_range identifies trunk (i.e., normalize before comparing to TRUNK_BOOKMARKS or revset-derived names) and add a unit test feeding a line like "feat/xyz (conflicted): ..." to prevent regressions.
🤖 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/todo.md`:
- Around line 136-158: The docs checklist incorrectly marks tasks `#4` and `#5` as
"未着手" even though the PR implements them; update docs/todo.md to reflect
completion by marking the checklist items for task `#4` (bookmark divergence) and
task `#5` (empty-diff skip) as completed (e.g., change to [x]) and add a brief
note referencing the implemented files/entries
src/cli-push-runner/src/stages/push_jj_bookmark.rs (advance/bookmark-set logic)
and src/cli-push-runner/src/stages/diff.rs / main.rs (empty-diff skip/config) so
the document matches the current PR state.
---
Duplicate comments:
In `@src/cli-push-runner/src/stages/push_jj_bookmark.rs`:
- Around line 164-172: parse_bookmark_list_output currently leaves the "
(conflicted)" marker in bookmark names causing downstream commands (e.g.
dispatch_bookmark_advance) to fail; update parse_bookmark_list_output to strip
the conflicted marker (remove trailing " (conflicted)" or any trailing
parenthetical like " (…)" after extracting the name) and then trim before
calling is_trunk_bookmark so trunk checks use the canonical name; also ensure
this normalization matches how get_bookmarks_in_range identifies trunk (i.e.,
normalize before comparing to TRUNK_BOOKMARKS or revset-derived names) and add a
unit test feeding a line like "feat/xyz (conflicted): ..." to prevent
regressions.
🪄 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: fa84d289-828c-455d-8173-e4e4d0fbd0ae
📒 Files selected for processing (6)
docs/todo.mdsrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/stages/diff.rssrc/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 (2)
- src/cli-push-runner/src/stages/push.rs
- 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/diff.rs
Comment on lines
+136
to
+158
| ### 4. push-runner の takt fix 後 bookmark 乖離問題 | ||
|
|
||
| - **やろうとしたこと**: takt fix ステップ後に @ が bookmark より先に進み、`jj git push` で "No bookmarks found" となる問題を修正する | ||
| - **現在地**: 未着手。原因特定済み | ||
| - [ ] `src/cli-push-runner/src/stages/push.rs` の push ステップで、push 前に bookmark を @ に追従させるロジックを追加 | ||
| - [ ] または takt ステップ後に `jj squash` 相当の処理を自動実行 | ||
| - [ ] 修正後に takt fix が発火するケースでの回帰テスト | ||
| - **詰まっている箇所**: なし | ||
| - **根拠**: PR #49 の push pipeline で発生。takt の fix ステップがコード修正 → @ が bookmark から乖離 → push で bookmark が見つからない | ||
| - **Why**: takt は @ 上で直接コード修正するため、fix が入ると @ が新しい commit に進むが、bookmark は旧 commit のまま残る | ||
| - **How to apply / 再開手順**: push ステップ内で `push_jj_bookmark::advance_jj_bookmarks()` が既にあるが、これは trunk 以降の bookmark を target に前進させるもの。takt fix 後の bookmark 乖離は別の問題 (bookmark 自体が @ より古い位置にある)。push 前に `jj bookmark set <name> -r @` で bookmark を @ に合わせる処理を追加する | ||
|
|
||
| ### 5. push-runner の空 diff 時 pipeline 中断を正常終了に | ||
|
|
||
| - **やろうとしたこと**: push 対象の変更がない場合 (レビュー済みコードの再 push 等)、exit code 5 で中断するのではなく skip として正常終了するオプションを追加する | ||
| - **現在地**: 未着手。原因特定済み | ||
| - [ ] `src/cli-push-runner/src/stages/diff.rs` の空 diff 判定を "skip review + proceed to push" モードに変更 | ||
| - [ ] push-runner-config.toml に `allow_empty_diff = true` 等のオプション追加を検討 | ||
| - [ ] 修正後に空 diff ケースでの回帰テスト | ||
| - **詰まっている箇所**: なし | ||
| - **根拠**: PR #49 で squash 後の再 push 時に発生。jj squash で @ が空コミットになり、`jj diff -r @` が空 → push-runner が "diff 出力が空です" で exit 5 | ||
| - **Why**: push-runner は diff が空 = レビュー対象なし = パイプライン中断と判断するが、takt fix 後の再 push や bookmark 移動後の push では「diff は空だが push は必要」なケースがある | ||
| - **How to apply / 再開手順**: diff が空の場合に takt レビューをスキップして push ステップに直接進むパスを追加。push-runner-config.toml で挙動を制御できるようにする |
There was a problem hiding this comment.
タスク #4 / #5 のステータスが本 PR の実装内容と乖離しています
タスク #4（bookmark 乖離）と #5（空 diff スキップ）は本 PR の主目的そのものであり、src/cli-push-runner/src/stages/push_jj_bookmark.rs と src/cli-push-runner/src/stages/diff.rs / main.rs で既に実装済みです。にもかかわらず両項目が「現在地: 未着手。原因特定済み」のまま記載されており、マージ時点の事実と食い違います。
本 PR でマージする時点では「完了履歴」側へ移動するか、少なくともチェックリストを [x] に更新するのが妥当です（#3 の cli-pr-monitor --body 問題は本 PR では触っていないので #3 だけが in-flight として残る想定）。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/todo.md` around lines 136 - 158, The docs checklist incorrectly marks
tasks `#4` and `#5` as "未着手" even though the PR implements them; update docs/todo.md
to reflect completion by marking the checklist items for task `#4` (bookmark
divergence) and task `#5` (empty-diff skip) as completed (e.g., change to [x]) and
add a brief note referencing the implemented files/entries
src/cli-push-runner/src/stages/push_jj_bookmark.rs (advance/bookmark-set logic)
and src/cli-push-runner/src/stages/diff.rs / main.rs (empty-diff skip/config) so
the document matches the current PR state.
3 tasks
aloekun
added a commit
that referenced
this pull request
Apr 19, 2026
…k 5) takt 自動修正後の auto re-push で bookmark が旧 commit に取り残され remote 未反映になる 問題 (PR #53 で実測) を解消するため、cli-push-runner の push_jj_bookmark::advance_jj_bookmarks を cli-pr-monitor に port。 - src/cli-pr-monitor/src/stages/push_jj_bookmark.rs 新設 (cli-push-runner からの port) - run_push の jj new 後・push 前に advance_jj_bookmarks を挿入 (jj push のみ対象、失敗時は続行) - unit テスト 6 項目 + 実 jj を使う integration テスト 1 件 (PR #53 症状の退行防止) - log prefix は cli-pr-monitor の [action]/[state] に揃え、lib_jj_helpers::is_trunk_bookmark を再利用 共通化 (lib-jj-helpers への集約) は機能等価確認後の検討項目として TODO コメントを残す (ADR-024)。 Refs: docs/todo.md task 5, PR #50, PR #53, ADR-024
5 tasks
aloekun
added a commit
that referenced
this pull request
Apr 19, 2026
…k 5) takt 自動修正後の auto re-push で bookmark が旧 commit に取り残され remote 未反映になる 問題 (PR #53 で実測) を解消するため、cli-push-runner の push_jj_bookmark::advance_jj_bookmarks を cli-pr-monitor に port。 - src/cli-pr-monitor/src/stages/push_jj_bookmark.rs 新設 (cli-push-runner からの port) - run_push の jj new 後・push 前に advance_jj_bookmarks を挿入 (jj push のみ対象、失敗時は続行) - unit テスト 6 項目 + 実 jj を使う integration テスト 1 件 (PR #53 症状の退行防止) - log prefix は cli-pr-monitor の [action]/[state] に揃え、lib_jj_helpers::is_trunk_bookmark を再利用 共通化 (lib-jj-helpers への集約) は機能等価確認後の検討項目として TODO コメントを残す (ADR-024)。 Refs: docs/todo.md task 5, PR #50, PR #53, ADR-024
aloekun
added a commit
that referenced
this pull request
Apr 19, 2026
…k 5) takt 自動修正後の auto re-push で bookmark が旧 commit に取り残され remote 未反映になる 問題 (PR #53 で実測) を解消するため、cli-push-runner の push_jj_bookmark::advance_jj_bookmarks を cli-pr-monitor に port。 - src/cli-pr-monitor/src/stages/push_jj_bookmark.rs 新設 (cli-push-runner からの port) - run_push の jj new 後・push 前に advance_jj_bookmarks を挿入 (jj push のみ対象、失敗時は続行) - unit テスト 6 項目 + 実 jj を使う integration テスト 1 件 (PR #53 症状の退行防止) - log prefix は cli-pr-monitor の [action]/[state] に揃え、lib_jj_helpers::is_trunk_bookmark を再利用 共通化 (lib-jj-helpers への集約) は機能等価確認後の検討項目として TODO コメントを残す (ADR-024)。 Refs: docs/todo.md task 5, PR #50, PR #53, ADR-024
aloekun
added a commit
that referenced
this pull request
Apr 20, 2026
…k 5) (#61) takt 自動修正後の auto re-push で bookmark が旧 commit に取り残され remote 未反映になる 問題 (PR #53 で実測) を解消するため、cli-push-runner の push_jj_bookmark::advance_jj_bookmarks を cli-pr-monitor に port。 - src/cli-pr-monitor/src/stages/push_jj_bookmark.rs 新設 (cli-push-runner からの port) - run_push の jj new 後・push 前に advance_jj_bookmarks を挿入 (jj push のみ対象、失敗時は続行) - unit テスト 6 項目 + 実 jj を使う integration テスト 1 件 (PR #53 症状の退行防止) - log prefix は cli-pr-monitor の [action]/[state] に揃え、lib_jj_helpers::is_trunk_bookmark を再利用 共通化 (lib-jj-helpers への集約) は機能等価確認後の検討項目として TODO コメントを残す (ADR-024)。 Refs: docs/todo.md task 5, PR #50, PR #53, ADR-024
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
Test plan
Summary by CodeRabbit
バグ修正
ドキュメント
テスト