feat: pipeline lock + Stop hook 品質ゲート skip — Concurrent checkout 事故の根本解決 (順位280/289/290/291消化) - #271
Merged
Merged
Conversation
…ression test (順位289/291消化) docs: ADR-045 の merge foreground 暫定ルールを pipeline lock に置換 + 順位280/289/290/291 エントリ削除
|
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:
📝 WalkthroughWalkthroughmerge/push パイプラインに Changesパイプライン競合回避
関連するパイプライン判定修正
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MergePushPipeline
participant PipelineLock
participant StopQualityHook
MergePushPipeline->>PipelineLock: merge/push ロックを取得
PipelineLock-->>MergePushPipeline: Acquired または Busy/Unavailable
StopQualityHook->>PipelineLock: fresh lock を確認
PipelineLock-->>StopQualityHook: holder 情報を返却
StopQualityHook-->>StopQualityHook: 品質ゲートを skip
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)(該当なし — レビュー未着) Applicable Findings (Medium 以下)(該当なし — レビュー未着) Filtered (not applicable)(該当なし) 差分概要 (軽量サマリー)12 ファイル変更、todo 順位 280/289/290/291 の実装消化 PR。
実装は CLAUDE.md 記載の ADR-045 (jj workspace 並列運用) の既知リスク対策および該当 todo エントリの設計内容と一致しており、意図的設計からの逸脱は確認されない。 次のアクション
|
![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
🤖 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 `@src/cli-push-runner/src/stages/bookmark_check.rs`:
- Around line 28-37: Update OWN_BRANCH_BOOKMARKS_REVSET to use
first_ancestors(@) ~ ::trunk() so bookmark detection follows only the
workspace’s first-parent branch line and excludes merge side branches. Add
coverage for histories containing merges and side branches, verifying unintended
bookmarks are not selected.
In `@src/lib-jj-helpers/src/pipeline_lock.rs`:
- Around line 36-44: Update PipelineLock to retain its ownership identity,
including pid and start_unix, and initialize those fields wherever PipelineLock
is constructed, including the takeover path. In Drop::drop, read and parse the
current lock contents and remove the file only when they match the guard’s
stored ownership values; otherwise leave the newer owner’s lock intact.
🪄 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: 113c4725-19aa-40ce-be0f-8f7795f175a9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.gitignoredocs/adr/adr-045-jj-workspace-parallel-sessions.mddocs/todo-summary.mddocs/todo13.mdsrc/cli-merge-pipeline/Cargo.tomlsrc/cli-merge-pipeline/src/pipeline.rssrc/cli-push-runner/src/main.rssrc/cli-push-runner/src/stages/bookmark_check.rssrc/hooks-stop-quality/Cargo.tomlsrc/hooks-stop-quality/src/main.rssrc/lib-jj-helpers/src/lib.rssrc/lib-jj-helpers/src/pipeline_lock.rs
💤 Files with no reviewable changes (2)
- docs/todo13.md
- docs/todo-summary.md
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)
Applicable Findings (Medium 以下)(該当なし) Filtered (not applicable)(該当なし — 今回の CodeRabbit actionable 指摘 2 件はいずれも applicable と判定) 次のアクション
|
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
前回指摘のフォローアップ状況
Applicable Findings (Critical / High / Major)(該当なし — 過去の指摘2件はいずれも修正・CodeRabbit確認済み。新規 actionable 指摘なし) Filtered (not applicable)(該当なし) 次のアクション
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)(該当なし — 過去の指摘2件は修正・CodeRabbit確認済みで、今回の再レビュー依頼からも新規指摘は発生していない) Filtered (not applicable)(該当なし) 次のアクション
|
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib-jj-helpers/src/pipeline_lock.rs (1)
87-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winstale takeover も atomic にしてください
read_fresh_lockの後にstd::fs::writeで上書きすると、同じ stale lock を見た 2 プロセスがどちらもAcquiredになり得ます。takeover もcreate_new(true)で行い、AlreadyExistsを敗者として扱ってください。concurrent stale takeoverの回帰テストも追加すると安心です。🤖 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/lib-jj-helpers/src/pipeline_lock.rs` around lines 87 - 124, Make the stale-lock takeover in acquire_pipeline_lock_at atomic: after read_fresh_lock confirms the existing lock is stale, replace the direct std::fs::write overwrite with a create_new(true)-based takeover mechanism so only one concurrent process acquires it. Treat AlreadyExists as a losing acquisition and return the appropriate non-acquired result, while preserving the existing stale-lock and normal-acquisition behavior; add a regression test covering concurrent stale takeovers.
🤖 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.
Outside diff comments:
In `@src/lib-jj-helpers/src/pipeline_lock.rs`:
- Around line 87-124: Make the stale-lock takeover in acquire_pipeline_lock_at
atomic: after read_fresh_lock confirms the existing lock is stale, replace the
direct std::fs::write overwrite with a create_new(true)-based takeover mechanism
so only one concurrent process acquires it. Treat AlreadyExists as a losing
acquisition and return the appropriate non-acquired result, while preserving the
existing stale-lock and normal-acquisition behavior; add a regression test
covering concurrent stale takeovers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cb6093e-7f1a-4061-bfd3-87317ad9e14c
📒 Files selected for processing (2)
src/cli-push-runner/src/stages/bookmark_check.rssrc/lib-jj-helpers/src/pipeline_lock.rs
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)
Applicable Findings (Medium 以下)(該当なし) Filtered (not applicable)(該当なし — 今回の新規 outside-diff 指摘 1 件は applicable と判定。ADR-045 等の意図的設計との矛盾は無く、 次のアクション
|
…bbit re-review Major対応) std::fs::write による無条件上書きだと、同じ stale lock を同時に見た 2 プロセス が両方とも Acquired になり得るレースがあった。remove_file + create_new に変更し、 takeover レースに負けた側 (create_new が AlreadyExists) は Busy を返すようにした。 関数長超過回避のため takeover_stale_lock / busy_from_disk へ分割。 regression test: concurrent_stale_takeover_only_one_wins (2 スレッドで実レース)。
This was referenced Jul 14, 2026
aloekun
added a commit
that referenced
this pull request
Jul 14, 2026
…注記 (順位292-300) (#272) * docs(todo): 順位292-300 登録 — jj-op-verify再分類/ADR-045注記/PR#271 post-merge-feedback採用5件 - 292: cli-pr-monitor lock.rsをtoken方式の所有権検証へ統一 - 293: push-runner stack pushモード (opt-in, YAGNI見送り継続) - 294: jj-op-verify hookの位置づけ再整理 (並列workspace安全化→混線緩和層) - 295: ADR-045にコミット消失事故の「並列原因」診断が未検証である旨の注記追加 - 296-300: PR #271 post-merge-feedback (271.md) T2-2/T2-3/T3-1/T3-2/T3-3 採用分 * docs(todo): 順位293の完了基準をclose判断パスも含むよう修正 (CodeRabbit Minor対応、PR #272)
aloekun
added a commit
that referenced
this pull request
Jul 16, 2026
* fix(cli-push-runner): 空 `@` 時の bookmark_check 誤誘導を修正 (push パイプライン改善 T8) `@` が空で 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> * fix(review): CodeRabbit Major 2 件 + simplicity 警告 2 件を反映 (PR #280 / push 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
概要
順位 280 (pipeline lock + Stop hook 品質ゲート skip) を実装し、順位 289/290/291 を同乗消化する。PR #267 マージで実発生した「background merge pipeline のローカル同期 checkout と、ターン終了で発火する Stop hook 品質ゲート (cargo/jj) の Concurrent checkout 競合」の根本解決。
変更内容 (コミット単位)
pipeline_lockモジュール新設。cli-pr-monitor/src/lock.rsの実績パターン (atomic create_new / age ベース stale takeover / RAII Drop) を踏襲し、timestamp は epoch 秒直記録に簡素化 (future-dated lock は stale 扱い = 永続 fresh 化 bug class の防止)。テスト 7 件.claude/pipeline.lockを保持 (advisory、Busy/Unavailable は警告のみで pipeline 続行)jj bookmark list -r "::@ ~ ::trunk()"で自 workspace のブランチ線上に限定。::@単独では共有祖先上の他 workspace bookmark を含みうる (CodeRabbit 指摘) ため~ ::trunk()で共有履歴を除外run_feedback_onlyの docstring を実装に合わせ修正 (owner_repo 検出失敗パスは marker を書かない = 意図的)。exit code 導出を純関数feedback_only_outcomeに抽出し、「stale report 存在下でも Err は exit 1」の regression test を追加 (SIM-NEW-pipeline-L224 の再発防止)実地検証 (本 PR の push 自体が dogfood)
.claude/pipeline.lockが RAII cleanup で消滅していることを確認jj git push -b pipeline-lock-stop-skipの単一 bookmark 指定 — 順位 290 の revset 限定が機能cargo clippy --workspace --all-targets -- -D warningsclean /cargo test --workspace全 33 suite ok /pnpm lint:docsOK設計判断・注意点
::@のみに依存しない追加の所有権検証 (workspace root commit の照合等) を伴うこと」に強化されていた。本実装の~ ::trunk()(共有履歴の除外) はこの「追加の検証」を trunk 除外方式で満たすという解釈。より厳密な workspace root 照合が必要と判断される場合はレビューで指摘いただきたい🤖 Generated with Claude Code
Summary by CodeRabbit