fix(merge-pipeline, stop-hook): sync_local の stale master 解消 + Stop hook の takt subsession skip + ADR-013/004 amendment - #222
Merged
aloekun merged 2 commits intoJun 27, 2026
Conversation
…ook の takt subsession skip 新 PC での merge-pipeline 実行時に発覚した「post-merge-feedback subsession が 読み取り専用 (edit: false) のはずなのに lib.rs を stray 編集した」事故の 連鎖根本原因を 2 layer で修正。 ## バグ A (本筋): merge-pipeline sync_local の stale local master 依存 src/cli-merge-pipeline/src/main.rs:685 の `jj new <branch>` (= jj new master) は local master bookmark を解決するため、`.jj/repo/config.toml` に `remotes.origin.auto-track-bookmarks = "*"` が無い環境では `jj git fetch` 後も local master が古い tip に固定され、stale code (= squash 前の状態) に working copy が乗ってしまう。 修正: `jj new <branch>@origin` (= jj new master@origin) に変更。 remote tracking ref を直接参照することで、auto-track-bookmarks 設定の 有無に関わらず必ず最新 tip に着地する。ADR-011 (push 戦略) への依存を 構造的に切り離す。 ## バグ B (多層防御): hooks-stop-quality が takt subsession で無差別発火 src/hooks-stop-quality/src/main.rs:122 は stop_hook_active のみで判定し、 takt subsession (edit: false で起動された分析専用 session) でも品質ゲートを 発火させていた。stale tree + clippy 失敗で「直せ」指示が出ると、edit: false のはずの subsession が stray edit を試みる。 修正: stop_hook_active チェックの直後で `.takt/runs/*/meta.json` を scan、 status: \"running\" の active takt run が 1 件以上存在すれば品質ゲートを skip。reaper module の同 marker 利用パターン (hooks-session-start) を踏襲。 ## ADR 更新 - ADR-013 (merge-pipeline): sync_local の `master@origin` 直接参照を前提条件 として明文化 (バグ A 修正の根拠 cite) - ADR-004 (Stop hook 品質ゲート): takt subsession skip 条件 = active meta.json を判定材料として明文化 (バグ B 修正の根拠 cite、ADR-031 weekly-review 等の read-only workflow と整合) ADR-011 (push 戦略) は touch しない (= push と merge は別 concern、coincidental coupling を解消する)。
📝 WalkthroughWalkthroughtakt の Stop フックに subsession 由来の skip 条件を追加し、 ChangesStop hook quality gate skip
Merge pipeline sync_local base update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 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/adr/adr-013-merge-pipeline.md`:
- Line 64: ADR内のコマンド表記が統一されていないため、本文中に残っている旧表記を修正してください。`adr-013-merge-pipeline`
の手順説明で `jj new master@origin` を正とするなら、同じ文書内の `new master` が残っている箇所もすべて
`master@origin` ベースに揃え、`sync_local` や関連手順の記述が一貫するように更新してください。該当箇所は `sync_local`
の説明とその周辺の手順セクションを確認して修正してください。
In `@src/hooks-stop-quality/src/main.rs`:
- Around line 71-96: takt_subsession_active currently skips the quality gate for
any run whose meta.json says status is "running", which can over-disable Stop
hook behavior for stale or unrelated runs. Tighten the check in
takt_subsession_active (and the caller path in should_skip_quality_gate) so it
only returns true for the intended read-only takt subsession, using an
additional freshness or run-type/ownership condition alongside
meta_status_is_running rather than trusting every running entry under
TAKT_RUNS_DIR. Ensure orphan or stale runs do not cause a global skip.
🪄 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: 621e5484-522a-4abd-aa0d-7d7aad1f8b66
📒 Files selected for processing (4)
docs/adr/adr-004-stop-hook-quality-gate.mddocs/adr/adr-013-merge-pipeline.mdsrc/cli-merge-pipeline/src/main.rssrc/hooks-stop-quality/src/main.rs
…+ ADR-013 旧表記統一 ## 変更内容 ### CR Major (src/hooks-stop-quality/src/main.rs) `takt_subsession_active()` の skip 判定に **mtime ベースの freshness check** を追加。 status: "running" のみを根拠にすると、abrupt termination で残った orphan run が .takt/runs/ に永続的に存在し、以降の全セッションで品質ゲートが skip されてしまう 致命的 regression を構造的に解消した。 - 新 helper `meta_is_active_run()`: status == "running" AND mtime fresh の AND 条件 - 新 helper `meta_is_fresh()`: mtime が ACTIVE_RUN_FRESH_THRESHOLD_SECS (= 1500s) 以内 - ACTIVE_RUN_FRESH_THRESHOLD_SECS は hooks-session-start の reaper の ORPHAN_THRESHOLD_SECS と同値 = 両者の「abrupt termination 判定」共通契約 - fail-closed: mtime 取得失敗 / 未来時刻 (clock skew) は active 扱いしない - test 7 件追加 (boundary 値 / orphan skip / fresh vs stale 混在 / 各種 fail-closed 経路) ### CR Minor (docs/adr/adr-013-merge-pipeline.md) ADR 内に残っていた旧表記 `jj new master` を 2 箇所 `jj new master@origin` に更新: - Line 14 「マージ → fetch → new master を毎回手動で実行」 - Line 53 「jj git fetch && jj new master でローカル同期」 ### ADR-004 拡張 § takt subsession skip の判定条件に「mtime が ACTIVE_RUN_FRESH_THRESHOLD_SECS 以内」 を追記、新 sub-section「freshness check の必要性 (CR PR #222 Major 指摘対応)」で orphan の永続 skip 問題と reaper との threshold 整合を明文化。 ## 検証 - cargo test -p hooks-stop-quality: 25 passed (= 元 18 + 新 7) - cargo test --workspace: 1315 passed - cargo clippy --workspace -- -D warnings: clean - markdownlint clean (両 ADR) ## CR thread 両 thread に resolved reply 投稿済。
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks-stop-quality/src/main.rs (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
1500sの契約値は共通化しておきたいです。ここは
src/hooks-session-start/src/reaper.rsの orphan 判定閾値と同じ値に依存しているので、片方だけ更新されると Stop hook の skip 判定と reaper の回収判定が静かにずれます。共通定数に寄せるか、少なくとも両者の一致を検証するテストを足しておくと将来のドリフトを防げます。Based on learnings,ACTIVE_RUN_FRESH_THRESHOLD_SECSmust stay aligned with the orphan-run threshold used by thehooks-session-startreaper module (currently 1500 seconds).🤖 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/hooks-stop-quality/src/main.rs` at line 72, The 1500-second freshness threshold used by ACTIVE_RUN_FRESH_THRESHOLD_SECS in main.rs is duplicated with the orphan-run cutoff in the hooks-session-start reaper logic, so keep them from drifting. Refactor the threshold into a shared constant or shared source of truth that both the Stop hook path and the reaper module use, and update the relevant symbols (ACTIVE_RUN_FRESH_THRESHOLD_SECS and the reaper’s orphan-detection threshold) to reference it. If sharing the constant is not practical, add a test that asserts both values stay equal so future changes fail fast.Source: Learnings
🤖 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.
Nitpick comments:
In `@src/hooks-stop-quality/src/main.rs`:
- Line 72: The 1500-second freshness threshold used by
ACTIVE_RUN_FRESH_THRESHOLD_SECS in main.rs is duplicated with the orphan-run
cutoff in the hooks-session-start reaper logic, so keep them from drifting.
Refactor the threshold into a shared constant or shared source of truth that
both the Stop hook path and the reaper module use, and update the relevant
symbols (ACTIVE_RUN_FRESH_THRESHOLD_SECS and the reaper’s orphan-detection
threshold) to reference it. If sharing the constant is not practical, add a test
that asserts both values stay equal so future changes fail fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3bdf3ac-8cc5-4e29-b836-f5d910184169
📒 Files selected for processing (3)
docs/adr/adr-004-stop-hook-quality-gate.mddocs/adr/adr-013-merge-pipeline.mdsrc/hooks-stop-quality/src/main.rs
✅ Files skipped from review due to trivial changes (2)
- docs/adr/adr-013-merge-pipeline.md
- docs/adr/adr-004-stop-hook-quality-gate.md
4 tasks
aloekun
added a commit
that referenced
this pull request
Jun 27, 2026
… 223/224) (#223) * docs(todo): PR #222 post-merge-feedback T1-1 + T3-1 採用 entries 追加 (順位 223/224) PR #222 post-merge-feedback (.claude/feedback-reports/222.md) で採用承認された 2 項目を docs/todo10.md + docs/todo-summary.md に登録。Bundle 222-FB として 1 PR 推奨 (T1 mechanical + T3 docs の 2 層防御を 1 PR で land)。 - 順位 223 (🔧 Tier 2、Effort M): ACTIVE_RUN_FRESH_THRESHOLD_SECS と ORPHAN_THRESHOLD_SECS の compile-time 同期。hooks-stop-quality と hooks-session-start で 1500s magic number を別 crate 独立に持ち、現状はコメント契約のみで mechanical enforcement が欠落。cli-merge-pipeline/src/feedback.rs:60 の derived const + 上流定数 precedent を cross-crate に拡張、Option C (const re-export + const _: () = assert!(...) compile-time check) を MVP 推奨。analyzer Tier 1 = mechanical enforcement → feedback_tier_classification per project Tier 2 に再分類。 - 順位 224 (💎 Tier 3、Effort XS): ADR-043 (Security/Quality Gate Fail-Closed) に hooks-stop-quality の error handling を具体例として追記。meta_is_fresh / meta_is_active_run / takt_subsession_active の 4 error path (mtime 取得失敗 / clock skew / malformed JSON / file read error) すべてが false 返却 → gate effective に倒れる fail-closed 構造を concrete instantiation として codify。 新 PC 環境でも同 task list を確認できるよう docs-only PR として分離 (実装は別 PR で着手)。 * fix(review): apply CodeRabbit fixes for #223 Resolved findings: - [Major] docs/todo10.md:925 「MVP 推奨」の記述と実際のアプローチに齟齬がある。
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
新 PC で発覚した「post-merge-feedback subsession が read-only (
edit: false) のはずなのにsrc/lib-report-formatter/src/lib.rsを stray 編集した」事故の 連鎖根本原因を 2 layer で修正 する。ユーザー分析 (2026-06-26 セッション) で確定した root cause:
merge-pipelineのsync_local()がjj new master(= local bookmark 参照) で stale local master に着地 → working tree に古いコード (=cargo clippy失敗対象) が乗るhooks-stop-qualityがstop_hook_activeのみで判定し、takt subsession の Stop event でも品質ゲートが発火 → stale tree の lint 失敗を「直せ」と subsession に指示 →edit: false制約に反して stray edit を試みるthis PC では
.jj/repo/config.tomlのauto-track-bookmarks = "*"(ADR-011 推奨設定) が偶然 local master を fast-forward させてバグ A が顕在化していなかったが、構造的バグはずっと存在。今回両方を直接修正する。変更内容
バグ A 修正:
sync_localをmaster@origin(= remote tracking ref) 直接参照に変更src/cli-merge-pipeline/src/main.rsのsync_local():format!("jj new {}", branch)→ 例:jj new masterformat!("jj new {}@origin", branch)→ 例:jj new master@originmaster@origin(= remote tracking ref) は jj clone 直後から自動生成され、.jj/repo/config.tomlのauto-track-bookmarks設定の有無に依存しない。ADR-011 (push 戦略) との偶発的結合 (coincidental coupling) を構造的に解消。新 helper
sync_local_new_command(branch: &str) -> Stringを抽出して unit test 3 件で command format を pin (master / main /@originsuffix 不在検出)。バグ B 修正:
hooks-stop-qualityに takt subsession skip 判定を追加src/hooks-stop-quality/src/main.rs:takt_subsession_active(repo_root: &Path) -> bool:.takt/runs/*/meta.jsonを scan、いずれかがstatus: "running"なら true。1 件目で短絡 return、malformed JSON は defensive skipmeta_status_is_running(meta_path: &Path) -> bool: 単一 meta.json の status 判定 (test 容易化のため切り出し)should_skip_quality_gate(hook_input: &HookInput) -> bool:stop_hook_activeチェック + takt subsession check を組み合わせた skip 判定main()関数長 81 行 (元 75 + 本 PR の +6) をread_stdin_or_block/parse_hook_input_or_block/warn_no_steps_configured/run_quality_steps/block_on_failuresの 5 helper に分解して 18 行に縮小 (順位 48 関数長 50 行ガイドライン内)unit test 9 件追加 (runs dir 不在 / meta.json 0 件 / 全 status=completed / 一部 status=running / single running / malformed JSON / 各 status 値の境界等を網羅)。
ADR-013 amendment
docs/adr/adr-013-merge-pipeline.md:jj new master@originに更新、注釈追記master@originは clone 直後から存在、ADR-011 との分離)ADR-004 amendment
docs/adr/adr-004-stop-hook-quality-gate.md:edit: falsesubsession は範囲外).takt/runs/*/meta.jsonの status: "running").failedmarker +hooks-session-startreaper module) との相補的利用ADR-011 は touch しない
ADR-011 (push 戦略) は push の関心領域そのまま で維持。merge-pipeline は
master@origin直接参照に変わったため、auto-track-bookmarks設定 (= push の Effect 1) と merge-pipeline は 完全に分離。両 ADR の責務領域が綺麗に分かれる。検証
cargo build --workspace: cleancargo test --workspace: 1308 tests passed (新規 12 件 = 3 sync_local + 9 takt_subsession)cargo clippy --workspace -- -D warnings: cleanなぜ this PC で問題が顕在化しなかったか
.jj/repo/config.tomlの[remotes.origin] auto-track-bookmarks = "*"設定 (ADR-011 推奨) が 偶然 次の effect を提供していた:jj git fetch時に local master がmaster@originに追従merge-pipeline は Effect 2 に 暗黙的に依存 していたが、ADR-011 / ADR-013 のどちらにも記述されていなかった (= coincidental coupling の典型例)。
本 PR で merge-pipeline は
master@originを直接参照する設計に変わり、Effect 2 への依存が消滅。新 PC でauto-track-bookmarks設定が無くても safe に動く。PR 計画への影響
本 PR は PR-W1 と PR-W2 の間に挟む follow-up として位置付け。merge-pipeline 自体は PR-W3 で分割対象になるが、本修正は分割前に root cause 解消を優先 (= 分割と修正は独立した変更、bundle すると review boundary が曖昧化する)。
Summary by CodeRabbit
taktの実行中サブセッションでは、Stop フックの品質ゲートを条件付きで自動スキップするようになりました。