From 9417fe3b46c2371629083fc532b76e77c4dc12e1 Mon Sep 17 00:00:00 2001 From: aloekun Date: Sat, 27 Jun 2026 12:41:54 +0900 Subject: [PATCH 1/2] =?UTF-8?q?fix(merge-pipeline,=20stop-hook):=20sync=5F?= =?UTF-8?q?local=20=E3=81=AE=20stale=20master=20=E8=A7=A3=E6=B6=88=20+=20S?= =?UTF-8?q?top=20hook=20=E3=81=AE=20takt=20subsession=20skip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 新 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 ` (= 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 @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 を解消する)。 --- docs/adr/adr-004-stop-hook-quality-gate.md | 22 ++ docs/adr/adr-013-merge-pipeline.md | 19 +- src/cli-merge-pipeline/src/main.rs | 37 ++- src/hooks-stop-quality/src/main.rs | 267 ++++++++++++++++++--- 4 files changed, 301 insertions(+), 44 deletions(-) diff --git a/docs/adr/adr-004-stop-hook-quality-gate.md b/docs/adr/adr-004-stop-hook-quality-gate.md index 876b16c5..d2632b4a 100644 --- a/docs/adr/adr-004-stop-hook-quality-gate.md +++ b/docs/adr/adr-004-stop-hook-quality-gate.md @@ -42,6 +42,28 @@ Claude Code の Stop フック入力には `stop_hook_active` フラグが含ま これにより最大1回のリトライで収束する。エージェントは1回目の停止で品質チェックの結果を受け取り、 修正を試みた後に再度停止を試みる。2回目は `stop_hook_active: true` なのでそのまま停止が許可される。 +### takt subsession skip (2026-06-26 追加、PR-W1 follow-up) + +takt workflow が起動する subsession (例: weekly-review の whole-tree reviewer / post-merge-feedback の analyze-pr / analyze-session / analyze-prepush-reports) は **`edit: false` で起動される read-only な分析セッション** が多い。これらの subsession で Stop フックが品質ゲート失敗を返すと、subsession は `edit: false` 制約と矛盾する「直せ」指示を受け取り、稀に **stray edit を試みる事故** が発生する (2026-06-26、PR #221 で観測。post-merge-feedback subsession が `src/lib-report-formatter/src/lib.rs` を意図せず編集)。 + +そもそも品質ゲートの趣旨は **本対話セッションの品質担保** であり、takt subsession に適用すべきではない (= 本 ADR の責務範囲外)。よって以下の条件で品質ゲートを skip する: + +- `.takt/runs/*/meta.json` を scan し、いずれかが **`status: "running"` であれば skip** +- 1 件目が見つかった時点で短絡 return (= I/O 最小化) +- malformed JSON / read error は defensive に skip (`status == "running"` と誤判定しない fail-closed) + +#### 同 marker の他用途 + +`.takt/runs//meta.json` の `status` field は ADR-030 (= 決定論的 post-merge-feedback) の `.failed` marker 経路と、`hooks-session-start` の reaper module (ADR-030 §L2 out-of-process orphan run 検出) でも使われており、本 ADR の追加判定は既存 marker の **読み取り側責務拡張** のみで実装される (新規 marker 不要、既存設計を再利用)。 + +#### 実装の所在 + +[src/hooks-stop-quality/src/main.rs](../../src/hooks-stop-quality/src/main.rs) の `should_skip_quality_gate()` で `stop_hook_active` チェック直後に `takt_subsession_active()` を呼ぶ 2 段判定。test 9 件で各種ケース (no runs dir / no meta / status=completed のみ / status=running 混在 / malformed JSON 等) を網羅。 + +#### 由来事例 + +PR-3a 系統で複数 PR を local で iterative に merge していた最中、新 PC で `.jj/repo/config.toml` の `auto-track-bookmarks` 設定欠落により merge-pipeline の `sync_local()` が stale local master を base にしたことが root cause。働きとして stale tree 上で `cargo clippy` が `unnecessary_sort_by` warning を flag し、後続の post-merge-feedback subsession に Stop hook 経由で「修正せよ」指示が伝達された (= 連鎖の半分)。merge-pipeline 側の根本修正は [ADR-013](adr-013-merge-pipeline.md) § sync_local の前提条件 を参照。本 ADR の subsession skip は **同型事故の多層防御** として導入。 + ### 出力形式 品質ゲート失敗時: diff --git a/docs/adr/adr-013-merge-pipeline.md b/docs/adr/adr-013-merge-pipeline.md index ccc178ce..c3250c45 100644 --- a/docs/adr/adr-013-merge-pipeline.md +++ b/docs/adr/adr-013-merge-pipeline.md @@ -61,7 +61,7 @@ cli-merge-pipeline.exe (スタンドアロン) | マージ戦略 | squash 固定 | master の履歴を 1 PR = 1 コミットに保つ | | PR 検出 | jj bookmark から自動検出 | `pnpm push` / `pnpm create-pr` と同じ方式で一貫性がある | | ブランチ削除 | `--delete-branch` で自動削除 | マージ済みブランチの残留を防ぐ | -| ローカル同期 | `jj git fetch` + `jj new master` | マージ後すぐに master 最新から作業を開始できる | +| ローカル同期 | `jj git fetch` + `jj new master@origin` | マージ後すぐに master 最新から作業を開始できる。`master@origin` (= remote tracking ref) を直接参照することで local master bookmark の状態に依存しない (詳細: 後述「§ sync_local の前提条件」) | | ステップ分離 | `pre_steps`（マージ前）/ `post_steps`（マージ後） | 学び提案等の post-merge 処理を正しいタイミングで実行 | | 学び提案機能 | 将来実装（`post_steps` に `type = "ai"` ステップ） | config に追加するだけで拡張可能 | @@ -84,6 +84,23 @@ step_timeout = 120 # prompt = "analyze_pr_learnings" ``` +### sync_local の前提条件 (2026-06-26 追加、PR-W1 follow-up) + +`sync_local()` は **squash マージで origin に新コミット (= マージ済 tip) が出来た直後** に、その新 tip を base にした空の作業コピーを置くことが責務。実装は以下の 2 ステップ: + +1. `jj git fetch` で `master@origin` を最新化 +2. `jj new master@origin` で remote tracking ref を base に新 commit を切る + +`master@origin` (= remote tracking ref) を直接参照する設計上の理由: + +- **local bookmark `master` の状態に依存しない**: jj は `jj git fetch` 時に local bookmark を自動 fast-forward させるかどうかが `.jj/repo/config.toml` の `[remotes.origin] auto-track-bookmarks` 設定に依存する。設定が無いと local master は古い tip に固定され、`jj new master` (= local bookmark 参照) は stale な base に着地してしまう +- **`master@origin` は jj clone 直後から自動生成される**: 設定なしで必ず存在する ref のため、新 PC / fresh clone でも前提条件を満たす +- **ADR-011 (push 戦略) との分離**: ADR-011 が確立した `auto-track-bookmarks = "*"` 設定は push の関心領域 (新規 bookmark の auto-track) のためのもの。merge-pipeline は同設定の副作用 (= local bookmark の fast-forward) に偶発的に依存していたが、本設計でその依存を解消した + +#### 過去の不具合 (2026-06-26 観測) + +新 PC で `.jj/repo/config.toml` に `auto-track-bookmarks` 設定が無い状態で merge-pipeline を実行したところ、stale local master に作業コピーが乗り、`post_steps` の post-merge-feedback subsession が古い lint warning (`unnecessary_sort_by`) を「fix」しようとして `src/lib-report-formatter/src/lib.rs` を stray 編集する事故が発生した。原因連鎖の半分が本 sync_local 設計のバグであり、本 ADR 改訂と [src/cli-merge-pipeline/src/main.rs](../../src/cli-merge-pipeline/src/main.rs) の修正で根本解消した。残り半分の連鎖 (Stop hook の subsession 無差別発火) は [ADR-004](adr-004-stop-hook-quality-gate.md) § takt subsession skip で多層防御を入れている。 + ## 影響 ### Positive diff --git a/src/cli-merge-pipeline/src/main.rs b/src/cli-merge-pipeline/src/main.rs index c5de9203..75e666e9 100644 --- a/src/cli-merge-pipeline/src/main.rs +++ b/src/cli-merge-pipeline/src/main.rs @@ -665,7 +665,14 @@ fn run_pipeline() -> i32 { 0 } -/// jj git fetch → jj new でローカルを最新に同期する +/// jj git fetch → jj new @origin でローカルを最新に同期する。 +/// +/// `@origin` は remote tracking ref への直接参照で、local bookmark の +/// 状態に依存しない。`` のみ (= local bookmark) を渡すと +/// `.jj/repo/config.toml` の `[remotes.origin] auto-track-bookmarks = "*"` 設定が +/// 無い環境で `jj git fetch` 後も local bookmark が古い tip に固定され、 +/// stale code に working copy が乗る (= post-merge-feedback subsession が +/// 古い lint warning を「fix」しようとして stray edit する事故、ADR-013 参照)。 fn sync_local(branch: &str) -> i32 { log_info("ローカル同期中: jj git fetch"); let (success, output) = run_cmd_shell_capped_reporting( @@ -682,7 +689,7 @@ fn sync_local(branch: &str) -> i32 { return 1; } - let new_cmd = format!("jj new {}", branch); + let new_cmd = sync_local_new_command(branch); log_info(&format!("ローカル同期中: {}", new_cmd)); let (success, output) = run_cmd_shell_capped_reporting("new-branch", &new_cmd, DEFAULT_STEP_TIMEOUT_SECS, MAX_LINES); @@ -695,12 +702,17 @@ fn sync_local(branch: &str) -> i32 { } log_info(&format!( - "ローカル同期完了。{} の最新状態で作業を開始できます。", + "ローカル同期完了。{}@origin の最新状態で作業を開始できます。", branch )); 0 } +/// `jj new @origin` の command 文字列を組み立てる (test 用に切り出し)。 +fn sync_local_new_command(branch: &str) -> String { + format!("jj new {}@origin", branch) +} + fn main() { std::process::exit(run_pipeline()); } @@ -764,6 +776,25 @@ prompt = "analyze_pr_learnings" ); } + #[test] + fn sync_local_new_command_references_remote_tracking_ref_for_master() { + assert_eq!(sync_local_new_command("master"), "jj new master@origin"); + } + + #[test] + fn sync_local_new_command_references_remote_tracking_ref_for_main() { + assert_eq!(sync_local_new_command("main"), "jj new main@origin"); + } + + #[test] + fn sync_local_new_command_never_references_bare_local_bookmark() { + let cmd = sync_local_new_command("master"); + assert!( + cmd.contains("@origin"), + "sync_local must use remote tracking ref (master@origin), never bare local bookmark — ADR-013 § sync_local 設計" + ); + } + #[test] fn should_skip_branch_delete_true_for_fork_pr() { let info = PrHeadInfo { diff --git a/src/hooks-stop-quality/src/main.rs b/src/hooks-stop-quality/src/main.rs index 4bf22744..fae99026 100644 --- a/src/hooks-stop-quality/src/main.rs +++ b/src/hooks-stop-quality/src/main.rs @@ -9,6 +9,13 @@ //! 無限ループ防止: //! stop_hook_active が true の場合、品質ゲートをスキップして停止を許可します。 //! これにより最大1回のリトライで収束します。 +//! +//! takt subsession skip (ADR-004 § takt subsession skip): +//! `.takt/runs/*/meta.json` で status: "running" の active takt run が存在する場合、 +//! 品質ゲートを skip します。takt subsession は edit: false で起動される read-only +//! 分析セッションが多く (例: weekly-review whole-tree reviewer / post-merge-feedback +//! analyzer)、Stop hook が「直せ」指示を出すと subsession が edit: false 制約に +//! 反して stray edit を試みる事故が発生する (PR #221 で実観測)。 use lib_subprocess::run_cmd_shell_capped; use serde::{Deserialize, Serialize}; @@ -52,6 +59,54 @@ struct QualityStepConfig { /// デフォルトのステップタイムアウト（秒） const DEFAULT_STEP_TIMEOUT_SECS: u64 = 60; +/// `.takt/runs/` の相対パス (repo root から)。hooks-session-start の reaper module と同値。 +const TAKT_RUNS_DIR: &str = ".takt/runs"; + +/// takt meta.json の必要 field のみ部分デシリアライズ (status 判定のみ)。 +#[derive(Deserialize)] +struct TaktMetaPartial { + status: Option, +} + +/// `.takt/runs//meta.json` を scan して active takt run があるか判定する。 +/// +/// 条件: いずれかの meta.json が `status: "running"` であれば true (= subsession active)。 +/// 1 件以上見つかった時点で短絡 return する。malformed JSON / non-dir / read error は skip。 +/// +/// ADR-004 § takt subsession skip: takt subsession は `edit: false` で起動される +/// read-only 分析 session が多く、Stop hook が品質ゲート失敗の「直せ」指示を返すと +/// 制約に反して stray edit を試みる事故が発生する。本関数で active subsession を +/// 検知して品質ゲートを skip することで、ADR-004 の趣旨 (= 本対話セッションの品質担保) +/// と takt の `edit: false` 制約の整合を取る。 +fn takt_subsession_active(repo_root: &Path) -> bool { + let runs_dir = repo_root.join(TAKT_RUNS_DIR); + let entries = match std::fs::read_dir(&runs_dir) { + Ok(e) => e, + Err(_) => return false, + }; + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_dir() { + continue; + } + if meta_status_is_running(&path.join("meta.json")) { + return true; + } + } + false +} + +/// 単一の `meta.json` が `status: "running"` か判定する (test 用に切り出し)。 +fn meta_status_is_running(meta_path: &Path) -> bool { + let Ok(content) = std::fs::read_to_string(meta_path) else { + return false; + }; + let Ok(meta) = serde_json::from_str::(&content) else { + return false; + }; + meta.status.as_deref() == Some("running") +} + /// block 判定を stdout に出力するヘルパー fn emit_block(reason: &str) { let decision = BlockDecision { @@ -97,73 +152,107 @@ const MAX_LINES: usize = 20; fn main() { let (config, config_found) = load_config(); - // stdin を消費（fail-closed: エラー時は block） + let Some(input) = read_stdin_or_block() else { + return; + }; + let Some(hook_input) = parse_hook_input_or_block(&input) else { + return; + }; + + if should_skip_quality_gate(&hook_input) { + return; + } + + let stop_config = config.stop_quality.unwrap_or_default(); + let steps = stop_config.steps.unwrap_or_default(); + let timeout = stop_config + .step_timeout + .unwrap_or(DEFAULT_STEP_TIMEOUT_SECS); + + if steps.is_empty() { + warn_no_steps_configured(config_found); + return; + } + + let failures = run_quality_steps(&steps, timeout); + block_on_failures(&failures); +} + +/// stdin を読み取る。失敗時は block 判定を emit して None を返す (fail-closed)。 +fn read_stdin_or_block() -> Option { let mut input = String::new(); if let Err(e) = io::stdin().read_to_string(&mut input) { emit_block(&format!( "品質ゲートエラー: stdin読み込みに失敗しました: {}", e )); - return; + return None; } + Some(input) +} - let hook_input: HookInput = match serde_json::from_str(&input) { - Ok(v) => v, +/// HookInput を JSON 解析する。失敗時は block 判定を emit して None を返す (fail-closed)。 +fn parse_hook_input_or_block(input: &str) -> Option { + match serde_json::from_str(input) { + Ok(v) => Some(v), Err(e) => { emit_block(&format!( "品質ゲートエラー: 入力JSONのパースに失敗しました: {}", e )); - return; + None } - }; + } +} - // 無限ループ防止: stop_hook_active が true なら品質ゲートをスキップ +/// 品質ゲートを skip すべきか判定する。 +/// +/// 2 条件のいずれかで skip: +/// - `stop_hook_active = true`: 無限ループ防止 (最大 1 retry で収束、ADR-004) +/// - `takt_subsession_active = true`: ADR-004 § takt subsession skip (edit: false の +/// subsession に「直せ」指示を返さない) +fn should_skip_quality_gate(hook_input: &HookInput) -> bool { if hook_input.stop_hook_active.unwrap_or(false) { - return; + return true; } + std::env::current_dir() + .map(|cwd| takt_subsession_active(&cwd)) + .unwrap_or(false) +} - // 設定からステップとタイムアウトを取得 - let stop_config = config.stop_quality.unwrap_or_default(); - let steps = stop_config.steps.unwrap_or_default(); - let timeout = stop_config - .step_timeout - .unwrap_or(DEFAULT_STEP_TIMEOUT_SECS); - - // ステップが無い場合は警告を出して停止許可 - if steps.is_empty() { - if !config_found { - eprintln!( - "[stop-quality] Warning: hooks-config.toml not found. Quality gate is disabled." - ); - eprintln!("[stop-quality] Place hooks-config.toml in the same directory as this exe."); - } else { - eprintln!( - "[stop-quality] Warning: No quality steps configured. Quality gate is disabled." - ); - } - return; +fn warn_no_steps_configured(config_found: bool) { + if !config_found { + eprintln!( + "[stop-quality] Warning: hooks-config.toml not found. Quality gate is disabled." + ); + eprintln!("[stop-quality] Place hooks-config.toml in the same directory as this exe."); + } else { + eprintln!( + "[stop-quality] Warning: No quality steps configured. Quality gate is disabled." + ); } +} - // 品質チェックを順番に実行 +fn run_quality_steps(steps: &[QualityStepConfig], timeout: u64) -> Vec { let mut failures: Vec = Vec::new(); - - for step in &steps { - let (success, output) = - run_cmd_shell_capped(&step.name, &step.cmd, timeout, MAX_LINES); + for step in steps { + let (success, output) = run_cmd_shell_capped(&step.name, &step.cmd, timeout, MAX_LINES); if !success { failures.push(format!("**{}** failed:\n```\n{}\n```", step.name, output)); } } + failures +} - // 失敗があれば block を出力 - if !failures.is_empty() { - let reason = format!( - "品質ゲートが失敗しました。以下の問題を修正してください:\n\n{}", - failures.join("\n\n") - ); - emit_block(&reason); +fn block_on_failures(failures: &[String]) { + if failures.is_empty() { + return; } + let reason = format!( + "品質ゲートが失敗しました。以下の問題を修正してください:\n\n{}", + failures.join("\n\n") + ); + emit_block(&reason); } #[cfg(test)] @@ -272,4 +361,102 @@ cmd = "pnpm py-typecheck" assert_eq!(steps.len(), 3); assert_eq!(steps[0].cmd, "pnpm py-lint"); } + + use std::sync::atomic::{AtomicU32, Ordering}; + + static UNIQUE_COUNTER: AtomicU32 = AtomicU32::new(0); + + fn unique_temp_root(prefix: &str) -> PathBuf { + let n = UNIQUE_COUNTER.fetch_add(1, Ordering::Relaxed); + let pid = std::process::id(); + let dir = std::env::temp_dir().join(format!("stop_quality_{}_{}_{}", prefix, pid, n)); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir + } + + fn write_run_meta(root: &Path, slug: &str, status: &str) { + let run_dir = root.join(".takt/runs").join(slug); + std::fs::create_dir_all(&run_dir).unwrap(); + let json = serde_json::json!({ "status": status }); + std::fs::write( + run_dir.join("meta.json"), + serde_json::to_string_pretty(&json).unwrap(), + ) + .unwrap(); + } + + #[test] + fn takt_subsession_active_returns_false_when_runs_dir_missing() { + let root = unique_temp_root("no-runs-dir"); + assert!(!takt_subsession_active(&root)); + } + + #[test] + fn takt_subsession_active_returns_false_when_no_meta_json_files() { + let root = unique_temp_root("empty-runs-dir"); + std::fs::create_dir_all(root.join(".takt/runs/orphan-slug")).unwrap(); + assert!(!takt_subsession_active(&root)); + } + + #[test] + fn takt_subsession_active_returns_false_when_all_status_completed() { + let root = unique_temp_root("all-completed"); + write_run_meta(&root, "run-a", "completed"); + write_run_meta(&root, "run-b", "failed"); + assert!(!takt_subsession_active(&root)); + } + + #[test] + fn takt_subsession_active_returns_true_when_any_status_running() { + let root = unique_temp_root("one-running"); + write_run_meta(&root, "completed-run", "completed"); + write_run_meta(&root, "active-run", "running"); + write_run_meta(&root, "failed-run", "failed"); + assert!(takt_subsession_active(&root)); + } + + #[test] + fn takt_subsession_active_returns_true_for_single_running_run() { + let root = unique_temp_root("single-running"); + write_run_meta(&root, "active", "running"); + assert!(takt_subsession_active(&root)); + } + + #[test] + fn takt_subsession_active_skips_malformed_meta_json() { + let root = unique_temp_root("malformed"); + let run_dir = root.join(".takt/runs/malformed-run"); + std::fs::create_dir_all(&run_dir).unwrap(); + std::fs::write(run_dir.join("meta.json"), "not-valid-json{").unwrap(); + assert!(!takt_subsession_active(&root)); + } + + #[test] + fn meta_status_is_running_returns_true_for_running_status() { + let root = unique_temp_root("status-running"); + write_run_meta(&root, "test", "running"); + let meta_path = root.join(".takt/runs/test/meta.json"); + assert!(meta_status_is_running(&meta_path)); + } + + #[test] + fn meta_status_is_running_returns_false_for_other_statuses() { + let root = unique_temp_root("status-other"); + for status in &["completed", "failed", "cancelled", "pending"] { + write_run_meta(&root, status, status); + let meta_path = root.join(format!(".takt/runs/{}/meta.json", status)); + assert!( + !meta_status_is_running(&meta_path), + "status {:?} must not be detected as running", + status + ); + } + } + + #[test] + fn meta_status_is_running_returns_false_when_file_missing() { + let root = unique_temp_root("missing"); + let meta_path = root.join(".takt/runs/never-existed/meta.json"); + assert!(!meta_status_is_running(&meta_path)); + } } From 4ab5edb668a8726c3534625207883bbe4b47e160 Mon Sep 17 00:00:00 2001 From: aloekun Date: Sat, 27 Jun 2026 13:18:24 +0900 Subject: [PATCH 2/2] =?UTF-8?q?fix(stop-hook,=20adr-013):=20CR=20PR=20#222?= =?UTF-8?q?=20findings=20=E5=AF=BE=E5=BF=9C=20=E2=80=94=20freshness=20chec?= =?UTF-8?q?k=20=E8=BF=BD=E5=8A=A0=20+=20ADR-013=20=E6=97=A7=E8=A1=A8?= =?UTF-8?q?=E8=A8=98=E7=B5=B1=E4=B8=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 変更内容 ### 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 投稿済。 --- docs/adr/adr-004-stop-hook-quality-gate.md | 12 +- docs/adr/adr-013-merge-pipeline.md | 4 +- src/hooks-stop-quality/src/main.rs | 136 ++++++++++++++++++++- 3 files changed, 145 insertions(+), 7 deletions(-) diff --git a/docs/adr/adr-004-stop-hook-quality-gate.md b/docs/adr/adr-004-stop-hook-quality-gate.md index d2632b4a..58800fc4 100644 --- a/docs/adr/adr-004-stop-hook-quality-gate.md +++ b/docs/adr/adr-004-stop-hook-quality-gate.md @@ -48,9 +48,17 @@ takt workflow が起動する subsession (例: weekly-review の whole-tree revi そもそも品質ゲートの趣旨は **本対話セッションの品質担保** であり、takt subsession に適用すべきではない (= 本 ADR の責務範囲外)。よって以下の条件で品質ゲートを skip する: -- `.takt/runs/*/meta.json` を scan し、いずれかが **`status: "running"` であれば skip** +- `.takt/runs/*/meta.json` を scan し、いずれかが **`status: "running"` かつ mtime が `ACTIVE_RUN_FRESH_THRESHOLD_SECS` (= 1500s) 以内** であれば skip - 1 件目が見つかった時点で短絡 return (= I/O 最小化) -- malformed JSON / read error は defensive に skip (`status == "running"` と誤判定しない fail-closed) +- malformed JSON / read error / mtime 取得失敗 / 未来時刻 (clock skew) は defensive に skip (= active 扱いしない、fail-closed) + +#### freshness check の必要性 (CR PR #222 Major 指摘対応) + +`status: "running"` は **abrupt termination (kill -9 / SIGKILL / power loss / OOM)** で残った orphan run でも残り続ける。hooks-session-start の reaper module (ADR-030 §L2) は SessionStart 時のみ scan するため、reaper 発火前の Stop event では古い orphan run が `.takt/runs/` に残存している可能性がある。 + +orphan を fresh subsession と同一視すると、**1 つの orphan が残っているだけで以降の全ての通常セッションの品質ゲートが永続的に skip される** 致命的な regression が発生する (= ADR-004 の趣旨「本対話セッションの品質担保」が完全に崩れる)。 + +mtime ベースの freshness check (= takt の TAKT_TIMEOUT_SECS 1200s + 5 分余裕 = 1500s 以内) を AND 条件として追加することで、orphan の永続 skip 問題を構造的に防ぐ。1500s 閾値は reaper の `ORPHAN_THRESHOLD_SECS` と同値で、**両者が「これ以上の age は abrupt termination」と判定する共通契約** を形成する。 #### 同 marker の他用途 diff --git a/docs/adr/adr-013-merge-pipeline.md b/docs/adr/adr-013-merge-pipeline.md index c3250c45..dc970aaa 100644 --- a/docs/adr/adr-013-merge-pipeline.md +++ b/docs/adr/adr-013-merge-pipeline.md @@ -11,7 +11,7 @@ Push Pipeline (ADR-008) と同様の「ガード + 専用 CLI」パターンで ### 現状の問題 1. **`gh pr merge` の直接実行**: マージ後にローカルの jj 環境を同期し忘れるリスクがある -2. **手動ステップの多さ**: マージ → fetch → new master を毎回手動で実行するのは煩雑 +2. **手動ステップの多さ**: マージ → fetch → new master@origin を毎回手動で実行するのは煩雑 3. **将来の拡張**: マージ後に「直前の PR から学びを抽出し、次の開発に活かす」機能を追加する余地を確保したい ### 検討した選択肢 @@ -50,7 +50,7 @@ cli-merge-pipeline.exe (スタンドアロン) ├─ jj bookmark → gh pr list --head で PR を自動検出 ├─ pre_steps を順次実行（マージ前チェック） ├─ gh pr merge --squash --delete-branch を実行 - ├─ jj git fetch && jj new master でローカル同期 + ├─ jj git fetch && jj new master@origin でローカル同期 └─ post_steps を順次実行（学び提案等の拡張ポイント） ``` diff --git a/src/hooks-stop-quality/src/main.rs b/src/hooks-stop-quality/src/main.rs index fae99026..c082d7ec 100644 --- a/src/hooks-stop-quality/src/main.rs +++ b/src/hooks-stop-quality/src/main.rs @@ -62,6 +62,15 @@ const DEFAULT_STEP_TIMEOUT_SECS: u64 = 60; /// `.takt/runs/` の相対パス (repo root から)。hooks-session-start の reaper module と同値。 const TAKT_RUNS_DIR: &str = ".takt/runs"; +/// freshness threshold (秒)。meta.json の mtime がこの値以内なら active 扱い。 +/// +/// hooks-session-start の reaper module の `ORPHAN_THRESHOLD_SECS` (= 1500s +/// = takt timeout 1200s + 余裕 5 分) と同値。本 threshold を超えた `status: "running"` は +/// abrupt termination で残った orphan run とみなし、active subsession 判定から除外する。 +/// この上限により、orphan run が永久に残って品質ゲートを skip し続ける問題を防ぐ +/// (ADR-004 § takt subsession skip 参照)。 +const ACTIVE_RUN_FRESH_THRESHOLD_SECS: u64 = 1500; + /// takt meta.json の必要 field のみ部分デシリアライズ (status 判定のみ)。 #[derive(Deserialize)] struct TaktMetaPartial { @@ -70,8 +79,11 @@ struct TaktMetaPartial { /// `.takt/runs//meta.json` を scan して active takt run があるか判定する。 /// -/// 条件: いずれかの meta.json が `status: "running"` であれば true (= subsession active)。 +/// 条件: いずれかの meta.json が `status: "running"` **かつ** mtime が +/// `ACTIVE_RUN_FRESH_THRESHOLD_SECS` 以内であれば true (= subsession active)。 /// 1 件以上見つかった時点で短絡 return する。malformed JSON / non-dir / read error は skip。 +/// freshness check で「abrupt termination で残った orphan run が永続的に品質ゲートを +/// skip させる」問題を防ぐ (CR PR #222 Major 指摘の根本対策)。 /// /// ADR-004 § takt subsession skip: takt subsession は `edit: false` で起動される /// read-only 分析 session が多く、Stop hook が品質ゲート失敗の「直せ」指示を返すと @@ -89,14 +101,26 @@ fn takt_subsession_active(repo_root: &Path) -> bool { if !path.is_dir() { continue; } - if meta_status_is_running(&path.join("meta.json")) { + if meta_is_active_run(&path.join("meta.json")) { return true; } } false } -/// 単一の `meta.json` が `status: "running"` か判定する (test 用に切り出し)。 +/// 単一の `meta.json` が active run (= status: "running" AND fresh) か判定する。 +/// +/// freshness は meta.json の filesystem mtime で判定。 +/// `ACTIVE_RUN_FRESH_THRESHOLD_SECS` 以内なら fresh、超えていれば orphan とみなして +/// active 扱いしない (CR PR #222 Major 指摘対応)。 +fn meta_is_active_run(meta_path: &Path) -> bool { + if !meta_status_is_running(meta_path) { + return false; + } + meta_is_fresh(meta_path) +} + +/// 単一の `meta.json` の status が `"running"` か判定する (test 用に切り出し)。 fn meta_status_is_running(meta_path: &Path) -> bool { let Ok(content) = std::fs::read_to_string(meta_path) else { return false; @@ -107,6 +131,25 @@ fn meta_status_is_running(meta_path: &Path) -> bool { meta.status.as_deref() == Some("running") } +/// 単一の `meta.json` の mtime が `ACTIVE_RUN_FRESH_THRESHOLD_SECS` 以内か判定する。 +/// +/// fail-closed: mtime 取得失敗 / 未来時刻 (= clock skew) は false (= active 扱いしない)。 +/// これにより orphan run / 異常な timestamp で品質ゲートが skip され続ける事故を防ぐ。 +fn meta_is_fresh(meta_path: &Path) -> bool { + let metadata = match std::fs::metadata(meta_path) { + Ok(m) => m, + Err(_) => return false, + }; + let mtime = match metadata.modified() { + Ok(t) => t, + Err(_) => return false, + }; + match mtime.elapsed() { + Ok(elapsed) => elapsed.as_secs() < ACTIVE_RUN_FRESH_THRESHOLD_SECS, + Err(_) => false, + } +} + /// block 判定を stdout に出力するヘルパー fn emit_block(reason: &str) { let decision = BlockDecision { @@ -459,4 +502,91 @@ cmd = "pnpm py-typecheck" let meta_path = root.join(".takt/runs/never-existed/meta.json"); assert!(!meta_status_is_running(&meta_path)); } + + fn set_meta_mtime_to_past(meta_path: &Path, secs_ago: u64) { + use std::time::{Duration, SystemTime}; + let f = std::fs::OpenOptions::new() + .write(true) + .open(meta_path) + .expect("open meta.json for mtime set"); + let past = SystemTime::now() - Duration::from_secs(secs_ago); + f.set_modified(past).expect("set_modified"); + } + + #[test] + fn meta_is_fresh_returns_true_for_just_written_file() { + let root = unique_temp_root("fresh-just-written"); + write_run_meta(&root, "now", "running"); + let meta_path = root.join(".takt/runs/now/meta.json"); + assert!(meta_is_fresh(&meta_path)); + } + + #[test] + fn meta_is_fresh_returns_false_for_stale_mtime_above_threshold() { + let root = unique_temp_root("fresh-stale"); + write_run_meta(&root, "old", "running"); + let meta_path = root.join(".takt/runs/old/meta.json"); + set_meta_mtime_to_past(&meta_path, ACTIVE_RUN_FRESH_THRESHOLD_SECS + 60); + assert!(!meta_is_fresh(&meta_path)); + } + + #[test] + fn meta_is_fresh_returns_true_just_below_threshold_boundary() { + let root = unique_temp_root("fresh-just-below"); + write_run_meta(&root, "boundary", "running"); + let meta_path = root.join(".takt/runs/boundary/meta.json"); + set_meta_mtime_to_past(&meta_path, ACTIVE_RUN_FRESH_THRESHOLD_SECS - 10); + assert!(meta_is_fresh(&meta_path)); + } + + #[test] + fn meta_is_fresh_returns_false_when_file_missing() { + let root = unique_temp_root("fresh-missing"); + let meta_path = root.join(".takt/runs/never-existed/meta.json"); + assert!(!meta_is_fresh(&meta_path)); + } + + #[test] + fn takt_subsession_active_returns_false_for_stale_orphan_running_run() { + let root = unique_temp_root("orphan-stale"); + write_run_meta(&root, "orphan", "running"); + let meta_path = root.join(".takt/runs/orphan/meta.json"); + set_meta_mtime_to_past(&meta_path, ACTIVE_RUN_FRESH_THRESHOLD_SECS + 3600); + assert!( + !takt_subsession_active(&root), + "stale orphan run (status: running but mtime > threshold) must not block quality gate (CR PR #222 Major 指摘対応)" + ); + } + + #[test] + fn takt_subsession_active_distinguishes_fresh_running_from_stale_running() { + let root = unique_temp_root("orphan-mixed"); + write_run_meta(&root, "stale-orphan", "running"); + set_meta_mtime_to_past( + &root.join(".takt/runs/stale-orphan/meta.json"), + ACTIVE_RUN_FRESH_THRESHOLD_SECS + 60, + ); + write_run_meta(&root, "fresh-active", "running"); + assert!( + takt_subsession_active(&root), + "fresh running run must override stale orphan (= 過剰 skip ではなく適切な active 判定)" + ); + } + + #[test] + fn meta_is_active_run_requires_both_running_and_fresh() { + let root = unique_temp_root("active-conditions"); + write_run_meta(&root, "completed-fresh", "completed"); + let completed = root.join(".takt/runs/completed-fresh/meta.json"); + assert!(!meta_is_active_run(&completed), "fresh but not running"); + + write_run_meta(&root, "running-stale", "running"); + let stale = root.join(".takt/runs/running-stale/meta.json"); + set_meta_mtime_to_past(&stale, ACTIVE_RUN_FRESH_THRESHOLD_SECS + 30); + assert!(!meta_is_active_run(&stale), "running but stale"); + + write_run_meta(&root, "running-fresh", "running"); + let active = root.join(".takt/runs/running-fresh/meta.json"); + assert!(meta_is_active_run(&active), "running AND fresh = active"); + } }