fix(cli-push-runner): 空 @ 時の bookmark_check 誤誘導を修正 (push T8) - #280
Merged
Conversation
`@` が空で bookmark が `@-` にある状態 (jj new 直後の正常な再 push 状態) で、 同一 run 内の advance_jj_bookmarks が「bookmark を @- に自動更新」と報告した 直後に bookmark_check が「bookmark が見つかりません」と報告し、 `jj bookmark create <name> -r @` を案内していた。従うと空の WIP コミットに bookmark が付く破壊的操作になる。PR #279 (T1) の dogfood push で実際に発火。 根本原因は「@ が空なら @- を対象にする」規則の二重定義。advance は determine_target_revision() で規則を持つのに、bookmark_check は OWN_WORKSPACE_BOOKMARKS_REVSET ("@" 厳密一致) で独自に検査していたため、 両者の判定が食い違った。 修正: - determine_target_revision() から working_copy_is_empty() を切り出し、 bookmark_check と共有する (規則の二重定義を解消)。 - 「@ に bookmark が無い」を 2 ケースに切り分ける判定 enum BookmarkCheckOutcome と pure function decide_bookmark_check() を追加。 jj 呼び出しは closure 注入 (ADR-021 原則 3、既存 dispatch_bookmark_advance と同じ流儀)。 - `@` 空 + bookmark が @-: `jj edit @-` + 空 WIP の abandon を案内 (T1 セッションで実証済みの回避策)。 - bookmark 皆無: 従来の作成案内が正しいので維持。 - main.rs に重複していた同じ誤案内を撤去し、ケース別案内を出す bookmark_check に一本化。 exit 7 による中断は維持し、案内文のみを正す方針を採った。計画の方針 2 (検査を @- 対象にして続行) は、[diff] command = "jj diff -r @" のため `@` が空のまま続行すると diff が空になり takt レビューが無言 skip された まま push される (誤誘導バグをレビューバイパスに置き換える) ため不採用。 方針 3 の「push すべき新変更がない」も、再現記録の事実 4 (jj edit @- 後に push 成功 = 変更はあった) と矛盾するため不採用。 ADR-021 原則 5 との関係: bookmark_check が `@` 厳密一致に狭めているのは PR #271 (他 workspace の bookmark 混入) の対策。本修正の @- 照会は案内文の 出し分け (診断) 専用で、push 対象の組み立ては `@` のまま維持する。 テスト: mod t8_empty_head_misdirection に 7 本追加 (186 → 193 passed)。 由来 incident と再現状態を module doc に明記 (ADR-049 の流儀)。bad = 2 ケースが潰れないこと、good = bookmark 皆無が NoBookmarks のままである ことを固定。サンドボックス jj repo で配布 exe が記録の出力を逐語再現する ことを確認した上で修正後 exe と before/after 比較した。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough「@」と「@-」の bookmark 判定を整理し、空の working copy に対する誤案内を修正した。exit 7 の制御フローを維持し、判定ロジックの共有化、案内出力の整理、回帰テスト、計画書の実装記録を追加した。 Changesbookmark_check 誤誘導修正
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PushPipeline
participant bookmark_check
participant jj
PushPipeline->>bookmark_check: bookmark_check 実行
bookmark_check->>jj: jj bookmark list -r @
bookmark_check->>jj: jj bookmark list -r `@-`
bookmark_check-->>PushPipeline: BookmarkCheckOutcome と案内ログ
PushPipeline-->>PushPipeline: exit 7 で中断
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし (レビュー指摘が現時点で 0 件のため評価対象なし) Applicable Findings (Medium 以下)(該当なし) Filtered (not applicable)(該当なし) 差分概要 (レビュー指摘が無いための軽量サマリー)対象:
incident 由来 (PR #279 dogfood push で実際に発火した誤誘導) のバグ修正で、方針転換の経緯 (誤誘導修正がレビューバイパスを誘発しうると気づき不採用にした判断) も docs に記録されており、変更の意図は自己完結して追跡可能。 次のアクション
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/push-pipeline-fix-plan.md`:
- Line 200: Update the 実施結果 entry in the push pipeline fix plan to use the
current date 2026-07-16 and record PR `#280` instead of the future date and 「PR
未採番」; also update the corresponding PR-number-missing entry referenced by the
review comment.
In `@src/cli-push-runner/src/stages/bookmark_check.rs`:
- Around line 137-143: Update the decision order in the bookmark-check flow so
head_is_empty is handled before bookmarks_at_head, returning EmptyWorkingCopy
with parent_bookmarks even when the empty @ has bookmarks. Preserve Proceed only
for non-empty heads with bookmarks, and update the tests covering this success
path to assert the corrected outcome.
- Around line 99-103: Update the bookmark-check flow around
run_jj_bookmark_list(PARENT_REVSET) to preserve parent lookup failures or
absence instead of converting them with unwrap_or_default(). Ensure root commits
branch to the guidance for creating work, rather than suggesting the unusable jj
edit `@-` command, and apply the same outcome handling to the related logic at the
other affected locations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7c35d8c-4626-40f5-ab7c-8178458e4b86
📒 Files selected for processing (4)
docs/push-pipeline-fix-plan.mdsrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/stages/bookmark_check.rssrc/cli-push-runner/src/stages/push_jj_bookmark.rs
| 「push すべき新変更がない」旨に修正。「bookmark が皆無」= 既存の案内が正しいので維持 | ||
| (上記 5.)。**現状は両者が同じ文面に潰れており、これが誤誘導の実体**。 | ||
| - **リスク**: 中。jj 変更検出は ADR-021 の設計原則に従うこと (revset 合成の流儀)。 | ||
| - **実施結果 (2026-07-17, 実装済み / PR 未採番)**: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
実施日と PR 番号を現在の記録に合わせてください。
現在は 2026-07-16 ですが、実装済み記録が未来日の 2026-07-17 になっています。また、本変更は PR #280 であるため、「PR 未採番」と判定表の PR 番号欠落も更新してください。
Also applies to: 555-555
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/push-pipeline-fix-plan.md` at line 200, Update the 実施結果 entry in the
push pipeline fix plan to use the current date 2026-07-16 and record PR `#280`
instead of the future date and 「PR 未採番」; also update the corresponding
PR-number-missing entry referenced by the review comment.
Comment on lines
+99
to
+103
| || { | ||
| run_jj_bookmark_list(PARENT_REVSET) | ||
| .map(|raw| parse_non_trunk_bookmarks(&raw)) | ||
| .unwrap_or_default() | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
存在しない @- を案内しないでください。
root commit では run_jj_bookmark_list("@-") が失敗しますが、unwrap_or_default() が「親は存在するが bookmark がない」状態へ変換し、実行不能な jj edit @-`` を案内します。親照会の失敗または親不存在を outcome に保持し、root では作業内容を作成する案内へ分岐してください。
Also applies to: 161-168, 343-354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli-push-runner/src/stages/bookmark_check.rs` around lines 99 - 103,
Update the bookmark-check flow around run_jj_bookmark_list(PARENT_REVSET) to
preserve parent lookup failures or absence instead of converting them with
unwrap_or_default(). Ensure root commits branch to the guidance for creating
work, rather than suggesting the unusable jj edit `@-` command, and apply the same
outcome handling to the related logic at the other affected locations.
Comment on lines
+137
to
+143
| if !bookmarks_at_head.is_empty() { | ||
| return BookmarkCheckOutcome::Proceed(bookmarks_at_head); | ||
| } | ||
| if head_is_empty { | ||
| return BookmarkCheckOutcome::EmptyWorkingCopy { | ||
| parent_bookmarks: parent_bookmarks(), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
空の @ に bookmark がある場合も中断してください。
現在は bookmark の存在を先に判定するため、空の @ に bookmark があると Proceed になります。しかし Line 164-167 の説明どおり、この状態では jj diff -r @ が空となり、親の未 push 変更を AI レビューせず push できます。head_is_empty を先に判定し、この成功経路を固定しているテストも修正してください。
Also applies to: 368-378
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli-push-runner/src/stages/bookmark_check.rs` around lines 137 - 143,
Update the decision order in the bookmark-check flow so head_is_empty is handled
before bookmarks_at_head, returning EmptyWorkingCopy with parent_bookmarks even
when the empty @ has bookmarks. Preserve Proceed only for non-empty heads with
bookmarks, and update the tests covering this success path to assert the
corrected outcome.
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)
Applicable Findings (Medium 以下)
Filtered (not applicable)該当なし 次のアクション
|
aloekun
added a commit
that referenced
this pull request
Jul 16, 2026
PR #280 の CodeRabbit レビュー指摘のうち Major 2 件を採用する。どちらも 「T8 が直したはずの穴が、条件違いで残っていた」類の指摘。 1. 判定順を反転し、bookmark が空の `@` にある場合も中断する (bookmark_check.rs) 従来は「`@` に bookmark があれば続行」を先に判定していたため、bookmark が 空の `@` に付いていると Proceed していた。この経路は `jj diff -r @` が空に なり、祖先の未 push 変更が AI レビューを経ずに push される — 本タスクが 方針 2 を却下した理由と同じ穴が、bookmark の位置違いで残っていた。 `advance_jj_bookmarks` は非 trunk bookmark が 2 つ以上あると fallback 更新を skip するため、この状態は実在する (サンドボックスで再現確認済み: 修正前は 「非 trunk bookmark 検出 (1 件): feat/b」で通過し PR diff 0 行へ進んでいた)。 「レビュー範囲 = `@` だから `@` は非空でなければならない」という本タスクの 不変条件に判定順を揃えた。 2. `@-` 照会の失敗を握り潰さない (bookmark_check.rs) `unwrap_or_default()` が照会失敗を「親はあるが bookmark 無し」に変換して いたため、`@-` の存在を確認できていないのに実行不能な `jj edit @-` を 案内し得た。ParentState::Unavailable として保持し、親を確認できない場合は `jj edit @-` を案内せず「`@` に変更を作成するか `jj edit <change_id>` で 移動」を案内する。 Minor 1 件 (docs の日付) は不採用: CodeRabbit は UTC 基準で「2026-07-16」と 指摘しているが、本 repo の記録は JST 基準 (既存の T0/T1 も同様) のため 2026-07-17 が正しい。同指摘のうち PR 番号 (未採番 → #280) は採用した。 テスト: t8_empty_head_misdirection を 7 → 10 本に拡充 (193 → 196 passed)。 バイパス経路を固定していた既存テスト 1 本は中断側へ反転させた。 新規は ParentState::Unavailable 時に `jj edit @-` を案内しないことの固定。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ush T8) PR #280 のレビュー指摘を反映する。いずれも「T8 が直したはずの穴が、条件違いで 残っていた」類の指摘で、本タスクの主題 (正確な案内) そのものに関わる。 ## CodeRabbit Major (2 件、採用) 1. 判定順を反転し、bookmark が空の `@` にある場合も中断する 従来は「`@` に bookmark があれば続行」を先に判定していたため、bookmark が 空の `@` に付いていると Proceed していた。この経路は `jj diff -r @` が空に なり、祖先の未 push 変更が AI レビューを経ずに push される — 本タスクが 方針 2 を却下した理由と同じ穴が、bookmark の位置違いで残っていた。 `advance_jj_bookmarks` は非 trunk bookmark が 2 つ以上あると fallback 更新を skip するため、この状態は実在する (サンドボックスで再現確認済み: 修正前は 「非 trunk bookmark 検出 (1 件): feat/b」で通過し PR diff 0 行へ進んでいた)。 「レビュー範囲 = `@` だから `@` は非空でなければならない」という本タスクの 不変条件に判定順を揃えた。 2. `@-` 照会の失敗を握り潰さない `unwrap_or_default()` が照会失敗を「親はあるが bookmark 無し」に変換して いたため、`@-` の存在を確認できていないのに実行不能な `jj edit @-` を 案内し得た。ParentState::Unavailable として保持し、親を確認できない場合は `jj edit @-` を案内しない。 ## simplicity-review 非ブロッキング警告 (2 件、採用) 3. `query_parent_state()` の jj 失敗を log する 同ファイルの他の jj 失敗処理や push_jj_bookmark.rs は log_info する慣習が あり、ここだけ欠落していた。親を確認できない理由が残らないと、root commit なのか jj 不調なのかを切り分けられない。 4. `@-` に bookmark が無い場合の案内を分ける `jj edit @-` だけを案内すると次は `NoBookmarks` で止まり根本解決にならない。 bookmark 作成まで含めて 1 度に案内する 3 つ目の variant に分けた。 ## 不採用 CodeRabbit Minor の日付指摘は不採用: CodeRabbit は UTC 基準で「2026-07-16」と 指摘しているが、本 repo の記録は JST 基準 (既存の T0/T1 も同様) のため 2026-07-17 が正しい。同指摘のうち PR 番号 (未採番 → #280) は採用した。 テスト: t8_empty_head_misdirection を 7 → 12 本に拡充 (193 → 198 passed)。 バイパス経路を固定していた既存テスト 1 本は中断側へ反転させた。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aloekun
force-pushed
the
fix/bookmark-check-empty-head
branch
from
July 16, 2026 17:47
4f9265f to
e9e2288
Compare
This was referenced Jul 16, 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
@が空で bookmark が@-にある状態 (jj new 直後の正常な再 push 状態) で、bookmark_checkが「bookmark 皆無」と誤報告しjj bookmark create <name> -r @へ誤誘導していたのを修正。従うと空の WIP コミットに bookmark が付く破壊的操作になる。
@が空なら@-を対象にする」規則の二重定義を解消。working_copy_is_empty()をadvance_jj_bookmarksとbookmark_checkで共有する。BookmarkCheckOutcome(Proceed/EmptyWorkingCopy/NoBookmarks) に切り分け、ケース別の案内を出す。
@空時は実証済みの回避策jj edit @-を案内する。main.rsに重複していた同じ誤案内を撤去し、案内の出力元をbookmark_checkに一本化。good (bookmark 皆無は従来案内のまま) を固定。
Context
Why: PR #279 (T1) の dogfood push で実際に発火した incident。同一 run 内で
advance_jj_bookmarksが「bookmark を@-に自動更新」と報告した直後にbookmark_checkが「bookmark が見つかりません」と報告するという矛盾が出力され、案内に従うと bookmark を壊す方向に誘導されていた。
Trigger:
docs/push-pipeline-fix-plan.md§4 T8。当初はコード監査による推測だったが、T1 セッションで in the wild に再現したため実施対象として確定した。
Scope decision: 計画の方針 2 (検査を
@-対象にして続行) は不採用とした。[diff] command = "jj diff -r @"のため@が空のまま続行すると diff が空になり、main.rsの「diff が空のためレビューをスキップ」経路で takt レビューが無言 skip されたまま
@-の変更が push される — 誤誘導バグをレビューバイパスに置き換えることになる。方針 3 の「push すべき新変更がない」も、再現記録の事実 4 (
jj edit @-後に push 成功= 変更はあった) と矛盾するため不採用。exit 7 による中断は維持し、案内文のみを正す
方針をユーザー承認のうえ採用した。
タスク順は T4-T7 を飛ばして T1 の次に実施した (T1 の dogfood で再現が取れた
タイミングを優先。T8 は他タスクと独立のため順序入替は無害)。
Validation
cargo test -p cli-push-runner: 193 passed (T8 回帰テスト 7 本追加、186 → 193)cargo clippy --workspace --all-targets -- -D warnings: 0 warningspnpm pushpre-push review: verdict=APPROVE (simplicity / security とも、fix iteration 0、2m 10s / パイプライン全体 184s)
@= 空 WIP /@-= bookmark) を張った jj repo で、現行の配布 exe が記録の出力を逐語で再現することを確認した上で修正後 exe と比較。案内が
jj bookmark create -r @→jj edit @-+ 空 WIP の abandon に変わり、exit 7 は維持。jj edit @-+ abandon した後の再実行でbookmark_checkを通過し後続 stage へ進むこと、および「@非空 + bookmark 皆無」では従来の作成案内がそのまま出ることを確認 (2 ケースの取り違え防止)。
.claude/cli-push-runner.exe) を再ビルド済み。fresh build と一致を確認。References
function、副作用は closure 注入) に準拠。原則 5 (
@/@-/@--の優先度付き revset) に対し
bookmark_checkが@厳密一致に狭めているのは PR feat: pipeline lock + Stop hook 品質ゲート skip — Concurrent checkout 事故の根本解決 (順位280/289/290/291消化) #271 の対策であり、本修正の@-照会は診断専用で push 対象は@のまま維持する。(由来 incident の明記 / bad + good ペア / 最小 assert)。
head_is_empty_or_assume_not()の既定は案内文の出し分けにのみ影響し、両分岐とも push を中断する。
OWN_WORKSPACE_BOOKMARKS_REVSETを@厳密一致に狭めた経緯。docs/push-pipeline-fix-plan.md§4 T8 実施結果 / §8 判定記録。Summary by CodeRabbit