From 489c762b95812922607abb38b6572d485492c54c Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 18:44:38 +0900 Subject: [PATCH 1/8] =?UTF-8?q?feat(lib-jj-helpers):=20pipeline=20lock=20?= =?UTF-8?q?=E3=82=92=E8=BF=BD=E5=8A=A0=20=E2=80=94=20=E5=AE=9F=E8=A1=8C?= =?UTF-8?q?=E4=B8=AD=20pipeline=20=E3=81=A8=20Stop=20hook=20=E3=81=AE?= =?UTF-8?q?=E7=9B=B8=E4=BA=92=E6=8E=92=E4=BB=96=E7=94=A8=20(=E9=A0=86?= =?UTF-8?q?=E4=BD=8D280)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/lib-jj-helpers/src/lib.rs | 3 + src/lib-jj-helpers/src/pipeline_lock.rs | 256 ++++++++++++++++++++++++ 2 files changed, 259 insertions(+) create mode 100644 src/lib-jj-helpers/src/pipeline_lock.rs diff --git a/src/lib-jj-helpers/src/lib.rs b/src/lib-jj-helpers/src/lib.rs index 9a058300..e7cdebf3 100644 --- a/src/lib-jj-helpers/src/lib.rs +++ b/src/lib-jj-helpers/src/lib.rs @@ -23,6 +23,9 @@ //! - [`get_jj_bookmarks`]: 上記を組み合わせた high-level エントリポイント //! - [`resolve_git_dir`] / [`inject_git_dir_for_gh`]: 非 colocated jj workspace //! での gh 用 `GIT_DIR` 導出と自動注入 (ADR-045 恒久対策候補 1) +//! - [`pipeline_lock`]: 実行中 pipeline と Stop hook 品質ゲートの相互排他 (順位 280) + +pub mod pipeline_lock; use std::process::{Command, Stdio}; diff --git a/src/lib-jj-helpers/src/pipeline_lock.rs b/src/lib-jj-helpers/src/pipeline_lock.rs new file mode 100644 index 00000000..0e6d5c2b --- /dev/null +++ b/src/lib-jj-helpers/src/pipeline_lock.rs @@ -0,0 +1,256 @@ +//! Pipeline lock — 実行中 pipeline (merge/push) と Stop hook 品質ゲートの相互排他 (順位 280)。 +//! +//! PR #267 のマージで「background の merge pipeline がローカル同期の checkout 実行中に、 +//! ターン終了で発火した Stop hook 品質ゲート (cargo/jj) が同じ working copy 上で競合」し、 +//! jj が Concurrent checkout で中断する事故が実発生した (ADR-045 § Known operational risks)。 +//! 本モジュールは pipeline 実行区間で lock ファイルを保持し、hooks-stop-quality が +//! fresh な lock を検知したら品質ゲートを skip する (fail-open) ための基盤を提供する。 +//! +//! 設計は `cli-pr-monitor/src/lock.rs` の実績パターンを踏襲: +//! - `OpenOptions::create_new` による atomic create (read-then-write TOCTOU の排除) +//! - age ベースの stale 判定 + takeover (クラッシュした pipeline の lock が永続しない) +//! - RAII guard (Drop で削除) +//! +//! 相違点: timestamp は ISO8601 ではなく unix epoch 秒を直接記録する (parser 不要)。 +//! future timestamp は stale 扱い (破損 lock が永続 fresh 化する bug class の再発防止、 +//! lock.rs の PastTime と同じ invariant)。 +//! +//! ファイル形式は `key=value` 行 (pid / start_unix / label)。外部 config ではなく +//! 内部の一時ファイルのため、依存追加 (serde/toml) を避けた最小形式とする。 + +use std::fs::OpenOptions; +use std::io::Write; +use std::path::{Path, PathBuf}; + +/// lock ファイル名 (`.claude/` 配下、gitignore 対象、checkout ごとに独立)。 +pub const PIPELINE_LOCK_FILENAME: &str = "pipeline.lock"; + +/// stale 判定 threshold。pipeline の実測最長 (push ~15 分) の 2x で安全マージン。 +pub const PIPELINE_LOCK_STALE_SECS: i64 = 1800; + +/// lock 取得成功時に保持する RAII guard。Drop で lock ファイルを削除する。 +pub struct PipelineLock { + path: PathBuf, +} + +impl Drop for PipelineLock { + fn drop(&mut self) { + if let Err(e) = std::fs::remove_file(&self.path) { + if e.kind() != std::io::ErrorKind::NotFound { + eprintln!("[pipeline-lock] cleanup 失敗: {}", e); + } + } + } +} + +/// lock 取得結果。Busy / Unavailable でも pipeline 自体は継続してよい +/// (lock は Stop hook への advisory であり、pipeline の実行可否を左右しない)。 +pub enum PipelineLockResult { + Acquired(PipelineLock), + Busy { holder_pid: u32, holder_age_secs: i64 }, + Unavailable { reason: String }, +} + +/// `claude_dir` (通常 `/.claude`) に pipeline lock を取得する。 +pub fn acquire_pipeline_lock(claude_dir: &Path, label: &str) -> PipelineLockResult { + acquire_pipeline_lock_at( + claude_dir.join(PIPELINE_LOCK_FILENAME), + label, + PIPELINE_LOCK_STALE_SECS, + current_unix_secs(), + ) +} + +/// テスト用: path / threshold / now を引数化。 +pub fn acquire_pipeline_lock_at( + path: PathBuf, + label: &str, + stale_threshold_secs: i64, + now_unix: i64, +) -> PipelineLockResult { + if let Some(parent) = path.parent() { + let _ = std::fs::create_dir_all(parent); + } + let content = build_lock_content(std::process::id(), now_unix, label); + + match OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(mut f) => { + if let Err(e) = f.write_all(content.as_bytes()) { + eprintln!("[pipeline-lock] 書き込み失敗 (継続): {}", e); + } + PipelineLockResult::Acquired(PipelineLock { path }) + } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + if let Some((pid, age_secs)) = + read_fresh_lock(&path, stale_threshold_secs, now_unix) + { + return PipelineLockResult::Busy { + holder_pid: pid, + holder_age_secs: age_secs, + }; + } + if let Err(e) = std::fs::write(&path, content) { + eprintln!("[pipeline-lock] takeover 書き込み失敗 (継続): {}", e); + } + PipelineLockResult::Acquired(PipelineLock { path }) + } + Err(e) => PipelineLockResult::Unavailable { + reason: e.to_string(), + }, + } +} + +/// fresh な pipeline lock が存在するか (Stop hook 用の読み取り専用チェック)。 +/// 戻り値は `Some((holder_pid, age_secs))`。lock 不在 / stale / parse 不能は `None`。 +pub fn pipeline_lock_holder(claude_dir: &Path) -> Option<(u32, i64)> { + read_fresh_lock( + &claude_dir.join(PIPELINE_LOCK_FILENAME), + PIPELINE_LOCK_STALE_SECS, + current_unix_secs(), + ) +} + +fn build_lock_content(pid: u32, start_unix: i64, label: &str) -> String { + format!( + "pid={}\nstart_unix={}\nlabel={}\n", + pid, + start_unix, + label.replace(['\r', '\n'], " ") + ) +} + +/// 既存 lock が fresh なら `Some((pid, age_secs))`。 +/// +/// stale 条件 (いずれかで None = takeover 可): +/// - parse 失敗 (破損) +/// - age >= threshold (クラッシュした pipeline の残骸) +/// - start_unix が未来 (破損 future-dated lock の永続 fresh 化防止) +fn read_fresh_lock(path: &Path, stale_threshold_secs: i64, now_unix: i64) -> Option<(u32, i64)> { + let content = std::fs::read_to_string(path).ok()?; + let pid: u32 = parse_field(&content, "pid")?.parse().ok()?; + let start_unix: i64 = parse_field(&content, "start_unix")?.parse().ok()?; + if start_unix > now_unix { + return None; + } + let age_secs = now_unix - start_unix; + if age_secs < stale_threshold_secs { + Some((pid, age_secs)) + } else { + None + } +} + +fn parse_field<'a>(content: &'a str, key: &str) -> Option<&'a str> { + content + .lines() + .find_map(|line| line.strip_prefix(key)?.strip_prefix('=')) + .map(str::trim) +} + +/// 実行中 exe の親ディレクトリ (= `.claude/`) を返す。 +/// +/// pipeline exe / hook exe はいずれも `.claude/` 配下に配置される (ADR-010) ため、 +/// lock ファイルの置き場所を exe-relative で解決する (cwd 非依存 = 順位 287 の規約)。 +pub fn exe_claude_dir() -> Option { + std::env::current_exe() + .ok()? + .parent() + .map(Path::to_path_buf) +} + +fn current_unix_secs() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_lock_path(prefix: &str) -> PathBuf { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.subsec_nanos()) + .unwrap_or(0); + std::env::temp_dir().join(format!( + "pipeline-lock-{}-{}-{}", + prefix, + std::process::id(), + nanos + )) + } + + #[test] + fn acquire_creates_lock_and_drop_removes_it() { + let path = temp_lock_path("acquire"); + let result = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_000); + assert!(matches!(result, PipelineLockResult::Acquired(_))); + assert!(path.exists()); + drop(result); + assert!(!path.exists(), "RAII drop で lock が削除される"); + } + + #[test] + fn second_acquire_is_busy_while_fresh() { + let path = temp_lock_path("busy"); + let _guard = acquire_pipeline_lock_at(path.clone(), "merge", 1800, 1_000_000); + let second = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_100); + match second { + PipelineLockResult::Busy { + holder_pid, + holder_age_secs, + } => { + assert_eq!(holder_pid, std::process::id()); + assert_eq!(holder_age_secs, 100); + } + _ => panic!("fresh lock 保持中は Busy になるべき"), + } + } + + #[test] + fn stale_lock_is_taken_over() { + let path = temp_lock_path("stale"); + std::fs::write(&path, "pid=99999\nstart_unix=1000000\nlabel=crashed\n").unwrap(); + let result = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_000 + 1800); + assert!( + matches!(result, PipelineLockResult::Acquired(_)), + "threshold 到達で takeover" + ); + } + + #[test] + fn future_dated_lock_is_treated_as_stale() { + let path = temp_lock_path("future"); + std::fs::write(&path, "pid=99999\nstart_unix=2000000\nlabel=corrupt\n").unwrap(); + let result = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_000); + assert!( + matches!(result, PipelineLockResult::Acquired(_)), + "future timestamp は stale 扱い (永続 fresh 化 bug class の防止)" + ); + } + + #[test] + fn corrupt_lock_is_taken_over() { + let path = temp_lock_path("corrupt"); + std::fs::write(&path, "not a lock file").unwrap(); + let result = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_000); + assert!(matches!(result, PipelineLockResult::Acquired(_))); + } + + #[test] + fn read_fresh_lock_parses_fields_and_age() { + let path = temp_lock_path("read"); + std::fs::write(&path, "pid=4321\nstart_unix=1000000\nlabel=merge\n").unwrap(); + let held = read_fresh_lock(&path, 1800, 1_000_500); + assert_eq!(held, Some((4321, 500))); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn missing_lock_reads_as_not_held() { + let path = temp_lock_path("missing"); + assert_eq!(read_fresh_lock(&path, 1800, 1_000_000), None); + } +} From 4d3e01c11af025d271e124d93adc99d029c23cd5 Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 18:47:03 +0900 Subject: [PATCH 2/8] =?UTF-8?q?feat(pipeline):=20merge-pipeline=20/=20push?= =?UTF-8?q?-runner=20=E3=81=8C=E5=AE=9F=E8=A1=8C=E4=B8=AD=20pipeline=20loc?= =?UTF-8?q?k=20=E3=82=92=E4=BF=9D=E6=8C=81=20(=E9=A0=86=E4=BD=8D280)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/cli-merge-pipeline/src/pipeline.rs | 2 ++ src/cli-push-runner/src/main.rs | 2 ++ src/lib-jj-helpers/src/pipeline_lock.rs | 29 +++++++++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/src/cli-merge-pipeline/src/pipeline.rs b/src/cli-merge-pipeline/src/pipeline.rs index 87e2b78e..f6e6275e 100644 --- a/src/cli-merge-pipeline/src/pipeline.rs +++ b/src/cli-merge-pipeline/src/pipeline.rs @@ -416,6 +416,8 @@ fn build_context() -> Result { } pub(crate) fn run_pipeline() -> i32 { + let _pipeline_lock = lib_jj_helpers::pipeline_lock::hold_pipeline_lock("merge", log_info); + let settings = match resolve_settings() { Ok(s) => s, Err(code) => return code, diff --git a/src/cli-push-runner/src/main.rs b/src/cli-push-runner/src/main.rs index 8a9f621b..9c6957b2 100644 --- a/src/cli-push-runner/src/main.rs +++ b/src/cli-push-runner/src/main.rs @@ -107,6 +107,8 @@ fn run_pipeline() -> i32 { } }; + let _pipeline_lock = lib_jj_helpers::pipeline_lock::hold_pipeline_lock("push", log_info); + let has_diff = config.diff.is_some(); let workflow = resolve_takt_workflow(&config); log_info(&format!( diff --git a/src/lib-jj-helpers/src/pipeline_lock.rs b/src/lib-jj-helpers/src/pipeline_lock.rs index 0e6d5c2b..b3b9dea7 100644 --- a/src/lib-jj-helpers/src/pipeline_lock.rs +++ b/src/lib-jj-helpers/src/pipeline_lock.rs @@ -147,6 +147,35 @@ fn parse_field<'a>(content: &'a str, key: &str) -> Option<&'a str> { .map(str::trim) } +/// pipeline 実行区間で lock を保持する便宜関数 (merge-pipeline / push-runner 用)。 +/// +/// lock は Stop hook への advisory であり pipeline の実行可否を左右しないため、 +/// Busy / Unavailable / exe dir 解決失敗はいずれも警告ログのみで `None` を返し、 +/// pipeline は lock なしで継続する。戻り値の guard を pipeline 終了まで保持すること。 +pub fn hold_pipeline_lock(label: &str, log: fn(&str)) -> Option { + let Some(dir) = exe_claude_dir() else { + log("[pipeline-lock] exe dir 解決失敗 (lock なしで継続)"); + return None; + }; + match acquire_pipeline_lock(&dir, label) { + PipelineLockResult::Acquired(lock) => Some(lock), + PipelineLockResult::Busy { + holder_pid, + holder_age_secs, + } => { + log(&format!( + "[pipeline-lock] 別 pipeline が実行中 (pid={}, age={}s) — lock なしで継続 (advisory)", + holder_pid, holder_age_secs + )); + None + } + PipelineLockResult::Unavailable { reason } => { + log(&format!("[pipeline-lock] 取得不可 (継続): {}", reason)); + None + } + } +} + /// 実行中 exe の親ディレクトリ (= `.claude/`) を返す。 /// /// pipeline exe / hook exe はいずれも `.claude/` 配下に配置される (ADR-010) ため、 From ff09a9c303c1edc73094b1f5d7f382ea4b006e46 Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 18:48:20 +0900 Subject: [PATCH 3/8] =?UTF-8?q?feat(hooks-stop-quality):=20pipeline=20lock?= =?UTF-8?q?=20=E6=A4=9C=E7=9F=A5=E3=81=A7=E5=93=81=E8=B3=AA=E3=82=B2?= =?UTF-8?q?=E3=83=BC=E3=83=88=E3=82=92=20skip=20(=E9=A0=86=E4=BD=8D280?= =?UTF-8?q?=E3=80=81fail-open)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Cargo.lock | 1 + src/hooks-stop-quality/Cargo.toml | 1 + src/hooks-stop-quality/src/main.rs | 32 ++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index c4b00685..5fc78d78 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -451,6 +451,7 @@ dependencies = [ name = "hooks-stop-quality" version = "0.1.0" dependencies = [ + "lib-jj-helpers", "lib-subprocess", "serde", "serde_json", diff --git a/src/hooks-stop-quality/Cargo.toml b/src/hooks-stop-quality/Cargo.toml index 44a1e4a3..d49df5ba 100644 --- a/src/hooks-stop-quality/Cargo.toml +++ b/src/hooks-stop-quality/Cargo.toml @@ -8,5 +8,6 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" toml = "0.8" lib-subprocess = { path = "../lib-subprocess" } +lib-jj-helpers = { path = "../lib-jj-helpers" } # [profile.release] は workspace root (Cargo.toml) に集約 (ADR-026) diff --git a/src/hooks-stop-quality/src/main.rs b/src/hooks-stop-quality/src/main.rs index 961f9e15..f2eb79a9 100644 --- a/src/hooks-stop-quality/src/main.rs +++ b/src/hooks-stop-quality/src/main.rs @@ -206,6 +206,10 @@ fn main() { return; } + if pipeline_is_running() { + return; + } + let stop_config = config.stop_quality.unwrap_or_default(); let steps = stop_config.steps.unwrap_or_default(); let timeout = stop_config @@ -221,6 +225,34 @@ fn main() { block_on_failures(&failures); } +/// 実行中 pipeline (merge/push) が fresh な lock を保持している間、品質ゲートを skip する +/// (順位 280、ADR-045 § Known operational risks の Concurrent checkout 事故対策)。 +/// +/// background pipeline のローカル同期 checkout と本 hook の cargo/jj 実行が同一 +/// working copy 上で競合し、jj が「Concurrent checkout」で中断する事故が PR #267 で +/// 実発生した。lock は `.claude/pipeline.lock` (exe-relative 解決 = 順位 287 規約) を +/// merge-pipeline / push-runner が実行区間で保持する。 +/// +/// skip は fail-open: Stop 時点のゲートは助言層で、本物のゲートは push pipeline 側の +/// quality_gate にある (ADR-043 の線引き)。stale threshold (30 分) 超過の lock は無視 +/// されるため、クラッシュした pipeline が永続 skip を招くことはない。 +/// kill-switch: lock ファイルの削除 (または pipeline 終了を待つ)。 +fn pipeline_is_running() -> bool { + let Some(dir) = lib_jj_helpers::pipeline_lock::exe_claude_dir() else { + return false; + }; + match lib_jj_helpers::pipeline_lock::pipeline_lock_holder(&dir) { + Some((pid, age_secs)) => { + eprintln!( + "[stop-quality] pipeline lock 検知 (pid={}, age={}s) — pipeline 実行中のため品質ゲートを skip (fail-open、順位280)", + pid, age_secs + ); + true + } + None => false, + } +} + /// stdin を読み取る。失敗時は block 判定を emit して None を返す (fail-closed)。 fn read_stdin_or_block() -> Option { let mut input = String::new(); From 970723a403de14195694a65aa62542f6b35ea9ff Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 18:50:27 +0900 Subject: [PATCH 4/8] =?UTF-8?q?feat(push-runner):=20bookmark=20=E6=A4=9C?= =?UTF-8?q?=E5=87=BA=E3=82=92=E8=87=AA=20workspace=20=E7=B7=9A=E4=B8=8A=20?= =?UTF-8?q?(::@=20~=20::trunk())=20=E3=81=AB=E9=99=90=E5=AE=9A=20(?= =?UTF-8?q?=E9=A0=86=E4=BD=8D290=E6=B6=88=E5=8C=96)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/cli-push-runner/src/stages/bookmark_check.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/cli-push-runner/src/stages/bookmark_check.rs b/src/cli-push-runner/src/stages/bookmark_check.rs index 8822eef3..3a4eb3dc 100644 --- a/src/cli-push-runner/src/stages/bookmark_check.rs +++ b/src/cli-push-runner/src/stages/bookmark_check.rs @@ -25,6 +25,16 @@ use crate::log::{log_info, log_stage}; const JJ_TIMEOUT_SECS: u64 = 30; +/// bookmark 検出の対象 revset: 自 workspace の @ の祖先のうち trunk の祖先を除いた範囲 +/// (= 自分のブランチ線上のみ、順位 290 / PR #269 feedback T1-1)。 +/// +/// `jj bookmark list` を無条件に使うとリポジトリ全体の bookmark (並行 workspace の +/// 作業中 bookmark を含む) を拾い、push stage の `-b` 付与対象に混入する。`::@` 単独では +/// 「共有祖先上の他 workspace の bookmark」も含まれる (CodeRabbit 指摘) ため、 +/// `~ ::trunk()` で共有履歴を除外する。この revset に含まれる bookmark は構造上 +/// 自分のブランチ線上のコミットを指すものに限られる。 +const OWN_BRANCH_BOOKMARKS_REVSET: &str = "::@ ~ ::trunk()"; + /// `jj bookmark list` で非 trunk なローカル bookmark の存在を確認し、 /// 検出した bookmark 名を返す。`None` = 非 trunk bookmark が無く push 不可 (pipeline 中断)。 /// @@ -78,7 +88,7 @@ fn run_jj_bookmark_list() -> Result { use std::process::Stdio; let mut child = Command::new("jj") - .args(["bookmark", "list"]) + .args(["bookmark", "list", "-r", OWN_BRANCH_BOOKMARKS_REVSET]) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() From a29d532f5349e001e25d754b2dcc4d712746fc03 Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 18:51:16 +0900 Subject: [PATCH 5/8] =?UTF-8?q?fix(merge-pipeline):=20run=5Ffeedback=5Fonl?= =?UTF-8?q?y=20=E3=81=AE=20docstring=20=E4=BF=AE=E6=AD=A3=20+=20Result=20?= =?UTF-8?q?=E4=BC=9D=E6=92=AD=20regression=20test=20(=E9=A0=86=E4=BD=8D289?= =?UTF-8?q?/291=E6=B6=88=E5=8C=96)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs: ADR-045 の merge foreground 暫定ルールを pipeline lock に置換 + 順位280/289/290/291 エントリ削除 --- src/cli-merge-pipeline/Cargo.toml | 3 ++ src/cli-merge-pipeline/src/pipeline.rs | 43 ++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/cli-merge-pipeline/Cargo.toml b/src/cli-merge-pipeline/Cargo.toml index 0c5e3dd0..f65fc5c1 100644 --- a/src/cli-merge-pipeline/Cargo.toml +++ b/src/cli-merge-pipeline/Cargo.toml @@ -11,4 +11,7 @@ lib-jj-helpers = { path = "../lib-jj-helpers" } lib-pending-file = { path = "../lib-pending-file" } lib-subprocess = { path = "../lib-subprocess" } +[dev-dependencies] +tempfile = "3" + # [profile.release] は workspace root (Cargo.toml) に集約 (ADR-026) diff --git a/src/cli-merge-pipeline/src/pipeline.rs b/src/cli-merge-pipeline/src/pipeline.rs index f6e6275e..a83070c7 100644 --- a/src/cli-merge-pipeline/src/pipeline.rs +++ b/src/cli-merge-pipeline/src/pipeline.rs @@ -206,7 +206,13 @@ fn run_ai_step(label: &str, ctx: Option<&PipelineContext>) { /// ADR-030 L2 recovery の対象にならない。本経路は marker の有無に依存せず feedback /// workflow を単独で再実行する。マージ済み PR の番号を明示指定する前提。 /// -/// 終了コード: 0 = report 生成成功、1 = 失敗 (marker は通常経路と同様に残る)。 +/// 終了コード: 0 = report 生成成功、1 = 失敗。 +/// +/// marker の扱い (順位 289): feedback workflow 本体の失敗では通常経路と同様に +/// `.failed` marker が残る (`run_ai_step_for` 内の `FailedMarkerGuard`)。ただし +/// **owner_repo の検出/validation 失敗パスでは marker を書かない** — 本経路は同期 CLI +/// で人間が直接ログを見る前提のため、L2 recovery (UserPromptSubmit hook) への引き継ぎは +/// 不要という意図的な設計。 pub(crate) fn run_feedback_only(pr_number: u64) -> i32 { let label = "feedback-only"; let Some(owner_repo) = detect_owner_repo() else { @@ -222,7 +228,17 @@ pub(crate) fn run_feedback_only(pr_number: u64) -> i32 { return 1; } - match run_ai_step_for(label, pr_number, &owner_repo) { + feedback_only_outcome(label, run_ai_step_for(label, pr_number, &owner_repo)) +} + +/// `--feedback-only` の終了コードを**本呼び出しの Result のみ**から導出する。 +/// +/// 順位 291 の regression guard: 初版はディスク上の report 存在 (`path.exists()`) を +/// 成功根拠にしており、stale report が存在すると失敗した再実行でも exit 0 を返す +/// hidden-coupling があった (pre-push simplicity-review REJECT: SIM-NEW-pipeline-L224)。 +/// 本関数はファイルシステム状態を一切参照しない。 +fn feedback_only_outcome(label: &str, result: Result) -> i32 { + match result { Ok(report) => { log_step( label, @@ -673,4 +689,27 @@ mod tests { } ); } + + /// 順位 291 regression guard: 終了コードは本呼び出しの Result のみから導出され、 + /// ディスク上の stale report の存在に影響されない (SIM-NEW-pipeline-L224 の再発防止)。 + /// 「stale report が存在する状態で Err → exit 1」が incident の再現シナリオ。 + #[test] + fn feedback_only_outcome_fails_on_err_even_when_stale_report_exists() { + let temp = tempfile::tempdir().expect("tempdir"); + let stale_report = temp.path().join("267.md"); + std::fs::write(&stale_report, "stale report from previous run").unwrap(); + + let code = feedback_only_outcome( + "test", + Err("concurrent run guard trip".to_string()), + ); + assert_eq!(code, 1, "stale report が存在しても Err は exit 1"); + assert!(stale_report.exists(), "前提: stale report は存在したまま"); + } + + #[test] + fn feedback_only_outcome_succeeds_only_from_ok_result() { + let code = feedback_only_outcome("test", Ok(PathBuf::from("reports/267.md"))); + assert_eq!(code, 0); + } } From 054adfcf919f65f16630056762d18cc2a1419f85 Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 19:52:41 +0900 Subject: [PATCH 6/8] =?UTF-8?q?docs:=20ADR-045=20=E3=82=92=20pipeline=20lo?= =?UTF-8?q?ck=20=E6=A9=9F=E6=A7=8B=E3=81=AB=E6=9B=B4=E6=96=B0=20+=20?= =?UTF-8?q?=E9=A0=86=E4=BD=8D280/289/290/291=20=E3=82=A8=E3=83=B3=E3=83=88?= =?UTF-8?q?=E3=83=AA=E5=89=8A=E9=99=A4?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .gitignore | 1 + Cargo.lock | 1 + .../adr-045-jj-workspace-parallel-sessions.md | 5 +- docs/todo-summary.md | 4 - docs/todo13.md | 77 ------------------- 5 files changed, 6 insertions(+), 82 deletions(-) diff --git a/.gitignore b/.gitignore index 018155c8..2be175f1 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,7 @@ src/*/Cargo.lock .claude/.session-id .claude/pr-monitor-state.json .claude/pr-monitor-state.json.tmp +.claude/pipeline.lock .claude/pr-monitor.lock .claude/scheduled_tasks.lock # ADR-029: post-merge-feedback の pending file (cli-merge-pipeline 生成、skill が consume する一時 artifact) diff --git a/Cargo.lock b/Cargo.lock index 5fc78d78..58ffddec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -143,6 +143,7 @@ dependencies = [ "lib-subprocess", "serde", "serde_json", + "tempfile", "toml", ] diff --git a/docs/adr/adr-045-jj-workspace-parallel-sessions.md b/docs/adr/adr-045-jj-workspace-parallel-sessions.md index aa6c8402..d6f380ed 100644 --- a/docs/adr/adr-045-jj-workspace-parallel-sessions.md +++ b/docs/adr/adr-045-jj-workspace-parallel-sessions.md @@ -115,6 +115,8 @@ src の大規模分割 (メイン) と lint/facet/docs (改善) は編集領域 6. マージ等の repo 境界操作の前に、background task (monitor / lint 等) の完了を確認する 7. 出力混線 (重複・欠落・身に覚えのないテキスト) を発見したら、直ちに両セッションを停止し、`jj op log` と会話ログを保存してから再開する +補足 — Stop hook との競合は機構で防止済み (2026-07-13、順位 280 実装): merge-pipeline / push-runner は実行区間で `.claude/pipeline.lock` を保持し、hooks-stop-quality は fresh な lock を検知すると品質ゲートを skip する (fail-open、`lib-jj-helpers::pipeline_lock`)。これにより「background の merge pipeline がローカル同期の checkout 実行中に、ターン終了で発火した Stop hook の cargo/jj が Concurrent checkout を誘発する」事故 (PR #267 マージで実観測) は構造的に再発しない。lock 実装前の暫定運用だった「merge をターン保持 (foreground 相当) で実行する」は不要になった。stale threshold は 30 分 (クラッシュした pipeline の lock は自動失効)。 + ### Operation Verification Checklist (2026-07-13 新設、暫定手順) 変更系 jj 操作 (`new` / `abandon` / `describe` / `rebase` / `squash` / `git fetch` / `git push`) の直後に、operation が記録されたことを確認する: @@ -126,7 +128,8 @@ jj op log --limit 1 --no-graph - 直前の操作に対応する op (description が操作内容と一致) が先頭にあること - 無い場合は「operation not recorded」= 上記 output corruption リスクの兆候。作業を止めて状態を確認する - `jj op log` は working copy を snapshot しない (副作用なし) ため、確認自体は安全 -- 本手順は PostToolUse hook による自動化 (todo 順位 275-278 と同経緯の feedback 採用分) が実装されるまでの暫定。hook 実装後は自動検証に置き換わる +- 本手順は `hooks-post-tool-jj-op-verify` (Bash PostToolUse、PR #267 で実装) により自動化済み。手動確認は hook 無効時・hook 対象外の verb を使う場合の fallback +- hook 運用の既知の注意 (2026-07-13 実観測): Bash tool がコマンドを background 実行した場合、hook はコマンド完了前の PostToolUse 時点で op log を見るため「operation not recorded」警告が出ることがある。この場合は task 完了後に `jj op log` で実状態を確認する (警告は誤報だが、確認を促す方向の誤りなので安全側) ### マージ方法 (各 workspace で独立) diff --git a/docs/todo-summary.md b/docs/todo-summary.md index 5e881768..af4505bc 100644 --- a/docs/todo-summary.md +++ b/docs/todo-summary.md @@ -128,7 +128,6 @@ | 275 | 🔧 Tier 2 | **層別テストテンプレート (StubOllama パターン・integration 独立性) の共有化 (PR #265 post-merge-feedback T2-1 採用)** | todo13.md | M | なし (WP-11/ADR-054 の多層防御実装で「空 StubOllama による LLM 未呼び出し証明」「tempdir+jj init+CwdRestore の integration 独立性」を都度設計。WP-17 の classifier/scope guard 拡張で同種判断が再発見込み。shared crate 化の境界は ADR-044 で判定、WP-17 着手前の実施が効果的) | | 276 | 💎 Tier 3 | **ADR-007 に「コメント配置の意思決定フロー」を追加 (PR #265 post-merge-feedback T3-2 採用)** | todo13.md | S | なし (PR #265 で非 doc コメントの Bundle Z block が 2 回発生 = doc コメント/識別子名/マーカー付き Why の配置判断が未文書化。linter 自動化は NLP 必要で却下済み、既存 Q1-Q3 形式で人間/AI の判断補助を doc 化。バッチ PR で消化可) | | 277 | 💎 Tier 3 | **PR body 配置タイミング規約を dev-conventions に明記 (PR #265 post-merge-feedback T3-3 採用)** | todo13.md | XS | なし (push パイプライン実行中の working copy に `__pr-body.md` を作成し snapshot 混入をかろうじて回避したヒヤリハット実発生。「push 完了後に scratchpad で準備し --body-file に絶対パス」を規約化。バッチ PR 消化可、並列安全化 PR docs への相乗りも可) | -| 280 | 🚀 Tier 1 | **pipeline lock + Stop hook 品質ゲート skip 機構 (PR #267 マージ事故の根本解決)** | todo13.md | S-M | なし (background merge pipeline の checkout と Stop hook 品質ゲートの Concurrent checkout 競合が実発生。--feedback-only (PR #268) は対症療法で競合自体は防げない。lock.rs パターン lib 化 + hooks-stop-quality の skip。**PR #268 の次の PR で対応予定 (2026-07-13 合意)**、実装後に ADR-045 の「merge は foreground」暫定ルールを撤去) | | 281 | 🚀 Tier 1 | **config-reading hook の current_dir() 解決を検出する lint rule (PR #267 post-merge-feedback T1-1 採用)** | todo13.md | S | なし (新規 hook が cwd 基準 config 解決を実装し pre-push REJECT → fix 修正の実例。cwd drift による silent fail-open は新規 hook のたびに再発しうる。Severity High。順位 287 と同一 PR bundle 推奨) | | 282 | 🚀 Tier 1 | **jj-op-verify の変更系 verb 網羅拡大 — undo/restore/split/bookmark move 等 (PR #267 post-merge-feedback T1-2 採用)** | todo13.md | M | なし (特に `jj undo` の検出漏れは lost-update 再発リスク高。拡張時は expected_op_keyword を jj 0.42 実機の op log 出力と要照合) | | 283 | 🚀 Tier 1 | **jj-op-verify の verb 検出を command-boundary に anchor (PR #267 post-merge-feedback T1-3 採用)** | todo13.md | S | なし (commit message 引用符内の "jj new" 等での false positive 防止。実装時に accepted risk で一度見送った経緯あり = 着手時に実観測 0 件のままか再確認。順位 285 と表裏) | @@ -137,9 +136,6 @@ | 286 | 🔧 Tier 2 | **config path 解決の cwd 跨ぎ integration test (PR #267 post-merge-feedback T2-3 採用)** | todo13.md | M | なし (FIXED 済 cwd-config bug の regression guard。既存テストは pure parser のみで file-lookup 経路未カバー。Severity High、Adoption Risk = OS 依存) | | 287 | 💎 Tier 3 | **「config 読み hook は exe-relative 解決必須」convention の明文化 (PR #267 post-merge-feedback T3-1 採用)** | todo13.md | XS | なし (順位 281 の文書層補完。**281 と同一 PR bundle 推奨**、別作業に切り出す価値は低い) | | 288 | 🔧 Tier 2 | **post-merge feedback の pre-push reports を対象 PR の全 run 集約に拡張 (PR #268 post-merge-feedback T2-1 採用)** | todo13.md | M | なし (「最新 1 run」参照は複数 push した PR で分析が最終 push 分に偏る。PR #267 feedback の evidence-scope 注記で実観測。context の prepush_reports_dir 配列化 + facet 複数 dir 対応。独立 PR 推奨) | -| 289 | 💎 Tier 3 | **run_feedback_only の docstring 修正 — 検出失敗パスは marker を書かない旨を明記 (PR #268 post-merge-feedback T3-1 採用)** | todo13.md | XS | なし (現行 docstring「marker は通常経路と同様に残る」と実装 (owner_repo 失敗パスは marker なし = 意図的) の drift 修正。**順位 280 PR に同乗推奨**) | -| 290 | 🚀 Tier 1 | **cli-push-runner の bookmark 検出を ::@ (自 workspace 祖先) に限定 (PR #269 post-merge-feedback T1-1 採用)** | todo13.md | S | なし (jj bookmark list はリポジトリ全体対象のため並行 workspace の bookmark も -b 付与対象に含む。実 push で複数 bookmark 付与を実観測、feedback と dogfood が独立に同一検出。--all 廃止の仕上げ。**順位 280 PR で消化予定**) | -| 291 | 🔧 Tier 2 | **run_ai_step_for の Result 伝播 regression test (PR #269 post-merge-feedback T2-1 採用)** | todo13.md | S | なし (SIM-NEW-pipeline-L224 = path.exists() 偽陽性 PASS の修正を両呼び出し元で固定。stale report 存在下の再実行失敗が exit 0 にならないことを assert。順位 280 PR 同乗可) | **戦略**: Tier 1 を 2〜3 セッションで片付け → Tier 2 で ADR-032 の前提 + rate-limit + convergence cost 削減を進める → Tier 3 で ADR-032 を land + ドキュメント整備。Tier 4-5 は cleanup / 外部展開で daily efficiency への直接効果は小さい。 diff --git a/docs/todo13.md b/docs/todo13.md index 66554a98..637162f1 100644 --- a/docs/todo13.md +++ b/docs/todo13.md @@ -890,29 +890,6 @@ --- -### pipeline lock + Stop hook 品質ゲート skip 機構 (PR #267 マージ事故の根本解決) - -> **動機**: PR #267 のマージで「background の merge pipeline がローカル同期の checkout を実行中に、ターン終了で発火した Stop hook 品質ゲート (cargo/jj) が同じ working copy 上で競合」し、jj が「Concurrent checkout」で中断・working copy が旧状態に取り残される事故が実発生 (jj 側の保護と自動解決で損失ゼロ、復旧 3 コマンド)。実施済みの `--feedback-only` (PR #268) は結果の一つ (feedback 未実行) の回復手段にすぎず、競合自体は防げていない。post-merge feedback の transcript window (first_commit〜merged_at) はマージ処理中の事故を構造的に見えないため、feedback からの提案も期待できない。 -> -> **設計案**: (1) cli-merge-pipeline / cli-push-runner が実行中に lock ファイル (PID + timestamp、`cli-pr-monitor/src/lock.rs` の stale takeover パターンを lib 化して流用) を保持、(2) hooks-stop-quality が開始時に lock を確認し、生きている pipeline 保持中は品質ゲートを skip (ログ + fail-open。Stop 時点のゲートは助言層で、本物のゲートは push 側)、(3) ADR-045 運用ルールに「lock 機構実装までは merge-pr を foreground 実行」の暫定ルールを追記済み → 実装後に撤去。 -> -> **参照**: ADR-045 § Known operational risks、ADR-030 (feedback recovery)、ADR-043 (skip は助言層ゲートの fail-open で整合)、`src/cli-pr-monitor/src/lock.rs` (流用元)、PR #268 (`--feedback-only` = 対症療法側) -> -> **実行優先度**: 🚀 Tier 1 — Effort S-M。**PR #268 の次の PR で対応予定 (ユーザー合意済み 2026-07-13)**。 - -#### 作業計画 - -- [ ] lock を lib (lib-jj-helpers or 新 lib) に一般化 (PID + timestamp + stale takeover) -- [ ] cli-merge-pipeline / cli-push-runner の実行区間で lock 保持 -- [ ] hooks-stop-quality に lock 検知 → skip (fail-open) を追加 + off/stale ケースのテスト -- [ ] ADR-045 の「merge は foreground」暫定ルールを撤去し本機構に置換 -- [ ] 本エントリ削除 + todo-summary.md 行削除 - -#### 完了基準 - -- background の merge/push pipeline 実行中にターンを終了しても Stop hook 品質ゲートが working copy に触れず、Concurrent checkout 事故が構造的に再発しないこと。 - ---- ### config-reading hook の `current_dir()` 解決を検出する lint rule (PR #267 post-merge-feedback T1-1 採用) @@ -1068,62 +1045,8 @@ --- -### run_feedback_only の docstring 修正 — 検出失敗パスは marker を書かない旨を明記 (PR #268 post-merge-feedback T3-1 採用) - -> **動機**: 現行 docstring は「失敗時 (marker は通常経路と同様に残る)」と記すが、owner_repo 検出/validation 失敗パスでは marker を書かない (同期 CLI で人間が直接ログを見るため意図的)。spec-impl drift の芽を摘む。 -> -> **参照**: `.claude/feedback-reports/268.md` Tier 3 #1、`src/cli-merge-pipeline/src/pipeline.rs` (`run_feedback_only` docstring) -> -> **実行優先度**: 💎 Tier 3 — Effort XS。**順位 280 の実装 PR に同乗推奨** (cli-merge-pipeline を触る同一 PR で消化)。 - -#### 作業計画 - -- [ ] docstring の終了コード契約の記述を実装に合わせて修正 -- [ ] 本エントリ削除 + todo-summary.md 行削除 - -#### 完了基準 - -- docstring と実装の marker 挙動が一致していること。 - ---- -### cli-push-runner の bookmark 検出を `::@` (自 workspace 祖先) に限定 (PR #269 post-merge-feedback T1-1 採用) -> **動機**: bookmark_check の検出 (`jj bookmark list`) はリポジトリ全体を対象とするため、並行 workspace の bookmark も拾い、push の `-b` 付与対象に含めてしまう。本セッションの実 push で `-b -b ` と複数 bookmark が付与された実観測あり (両方自分のもので無害だったが、並行 workspace では他者の作業中 bookmark を巻き込む余地)。`--all` 廃止 (PR #267) の仕上げとして、検出を `::@ ~ trunk()` 等の revset で自 workspace の祖先に限定する。feedback pipeline とセッション内 dogfood が独立に同一問題を検出 (相互裏付け)。 -> -> **参照**: `.claude/feedback-reports/269.md` Tier 1 #1、`src/cli-push-runner/src/stages/bookmark_check.rs`、ADR-045 § Known operational risks (bookmark conflicts) -> -> **実行優先度**: 🚀 Tier 1 — Effort S。**順位 280 の実装 PR で消化予定** (並列安全化の仕上げとして同一テーマ)。 - -#### 作業計画 - -- [ ] bookmark 検出を revset ベース (`jj log -r 'bookmarks() & ::@ ~ trunk()'` 等) に変更。`::@` revset のみでは自 workspace 所有の保証にならない (祖先 commit が並行 workspace と共有され得る) ため、workspace root commit の照合等、追加の所有権検証を組み合わせる + テスト -- [ ] 本エントリ削除 + todo-summary.md 行削除 - -#### 完了基準 - -- push の `-b` 付与対象が自 workspace の祖先にある bookmark に限定され、`::@` revset のみに依存しない追加の所有権検証 (workspace root commit の照合等) を伴うこと。 - ---- - -### run_ai_step_for の Result 伝播 regression test (PR #269 post-merge-feedback T2-1 採用) - -> **動機**: PR #268 の pre-push review で REJECT された `path.exists()` 偽陽性 PASS (SIM-NEW-pipeline-L224) の修正 (`Result` の直接伝播) を、両呼び出し元 (`run_feedback_only` / `run_ai_step`) で regression test として固定する。stale report 存在下での再実行失敗が exit 0 にならないことの検証が核心。 -> -> **参照**: `.claude/feedback-reports/269.md` Tier 2 #1、`src/cli-merge-pipeline/src/pipeline.rs` (`run_ai_step_for`) -> -> **実行優先度**: 🔧 Tier 2 — Effort S。順位 280 の実装 PR に同乗可 (cli-merge-pipeline を触る場合)。 - -#### 作業計画 - -- [ ] Result 伝播の unit/regression test を追加 (stale report + Err ケースで exit 1 を assert) -- [ ] 本エントリ削除 + todo-summary.md 行削除 - -#### 完了基準 - -- 偽陽性 PASS バグの再発がテストで検出されること。 - ---- ## 既知課題 (記録のみ、本セッションで未対応) From 913c2e058c1bca86824f790ea18e7589f4071930 Mon Sep 17 00:00:00 2001 From: aloekun Date: Mon, 13 Jul 2026 23:24:09 +0900 Subject: [PATCH 7/8] =?UTF-8?q?fix(review):=20CodeRabbit=20Major=202?= =?UTF-8?q?=E4=BB=B6=E5=AF=BE=E5=BF=9C=20=E2=80=94=20lock=20=E3=81=AE=20to?= =?UTF-8?q?ken=20=E6=89=80=E6=9C=89=E6=A8=A9=E7=A2=BA=E8=AA=8D=20+=20bookm?= =?UTF-8?q?ark=20=E3=82=92=20@=20=E9=99=90=E5=AE=9A=20(PR=20#271)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/stages/bookmark_check.rs | 50 ++++++-- src/lib-jj-helpers/src/pipeline_lock.rs | 108 ++++++++++++++++-- 2 files changed, 139 insertions(+), 19 deletions(-) diff --git a/src/cli-push-runner/src/stages/bookmark_check.rs b/src/cli-push-runner/src/stages/bookmark_check.rs index 3a4eb3dc..674b780b 100644 --- a/src/cli-push-runner/src/stages/bookmark_check.rs +++ b/src/cli-push-runner/src/stages/bookmark_check.rs @@ -21,19 +21,49 @@ use std::process::Command; use lib_jj_helpers::is_trunk_bookmark; +use super::push_jj_bookmark::advance_jj_bookmarks; use crate::log::{log_info, log_stage}; const JJ_TIMEOUT_SECS: u64 = 30; -/// bookmark 検出の対象 revset: 自 workspace の @ の祖先のうち trunk の祖先を除いた範囲 -/// (= 自分のブランチ線上のみ、順位 290 / PR #269 feedback T1-1)。 +/// bookmark 検出の対象 revset: **現在の workspace の `@` が指す bookmark のみ** +/// (順位 290 / PR #269・#271 CodeRabbit Major)。 /// -/// `jj bookmark list` を無条件に使うとリポジトリ全体の bookmark (並行 workspace の -/// 作業中 bookmark を含む) を拾い、push stage の `-b` 付与対象に混入する。`::@` 単独では -/// 「共有祖先上の他 workspace の bookmark」も含まれる (CodeRabbit 指摘) ため、 -/// `~ ::trunk()` で共有履歴を除外する。この revset に含まれる bookmark は構造上 -/// 自分のブランチ線上のコミットを指すものに限られる。 -const OWN_BRANCH_BOOKMARKS_REVSET: &str = "::@ ~ ::trunk()"; +/// 設計判断 (PR #271 で確定): bookmark の「所有権 (どの workspace のものか)」は +/// **履歴 (revset) から復元できない**。`::@ ~ ::trunk()` (自ブランチ線) を試みたが、 +/// 他 workspace が作った trunk 未マージのコミットの上で作業すると、そのコミットを指す +/// 他 workspace の bookmark が `::@` に混入する (CodeRabbit Major)。revset での所有権推定を +/// 諦め、push stage の `-b` 付与対象を「今 push したい作業 = `@` に付いた bookmark」に +/// 限定する。これにより他 workspace の bookmark 混入を構造的に排除する (安全側)。 +/// +/// トレードオフ: stacked bookmark (feature/base → feature/api → feature/ui を `@` 先頭で +/// 一括 push) の運用では `@` の bookmark だけでは不足する。ただし現状その運用実績はなく、 +/// 必要になった時点で明示オプトインの stack push モード (`[push] stack_push` 等) を追加する +/// 拡張余地を残す (todo 登録済み)。所有権を厳密に扱うには bookmark/workspace の別 metadata +/// 管理が必要だが、現用途では過剰。 +const OWN_WORKSPACE_BOOKMARKS_REVSET: &str = "@"; + +/// `OWN_WORKSPACE_BOOKMARKS_REVSET` (`@` 厳密一致) で bookmark 存在を検査する前に、 +/// `advance_jj_bookmarks()` (push stage が使う既存の前進処理と同一) で `@` より手前に +/// 残っている bookmark を前進させる (simplicity review 指摘対応: takt fix / 手動 +/// `jj describe` で `@` が bookmark より先に進んだ状態のまま `pnpm push` を再実行すると、 +/// advance 前に厳密一致で検査してしまい push stage の自動修復が走る前に pipeline が +/// 中断していた)。`None` = 非 trunk bookmark が無く push 不可 (pipeline 中断)。 +pub(crate) fn run_bookmark_check() -> Option> { + advance_lagging_bookmark(); + detect_own_workspace_bookmarks() +} + +/// `advance_jj_bookmarks()` を実行し、失敗時は fail-open で警告ログのみ出す +/// (advance はあくまで検査精度を上げるための前処理で、失敗しても検査自体は続行する)。 +fn advance_lagging_bookmark() { + if let Err(e) = advance_jj_bookmarks() { + log_info(&format!( + "bookmark_check: bookmark 自動更新失敗、検査を続行します: {}", + e + )); + } +} /// `jj bookmark list` で非 trunk なローカル bookmark の存在を確認し、 /// 検出した bookmark 名を返す。`None` = 非 trunk bookmark が無く push 不可 (pipeline 中断)。 @@ -43,7 +73,7 @@ const OWN_BRANCH_BOOKMARKS_REVSET: &str = "::@ ~ ::trunk()"; /// /// fail-open: jj 実行失敗時は warning ログのみで `Some(空)` を返し、push 自体は止めない /// (push stage は空リストなら base コマンドをそのまま実行する)。 -pub(crate) fn run_bookmark_check() -> Option> { +fn detect_own_workspace_bookmarks() -> Option> { let raw = match run_jj_bookmark_list() { Ok(output) => output, Err(e) => { @@ -88,7 +118,7 @@ fn run_jj_bookmark_list() -> Result { use std::process::Stdio; let mut child = Command::new("jj") - .args(["bookmark", "list", "-r", OWN_BRANCH_BOOKMARKS_REVSET]) + .args(["bookmark", "list", "-r", OWN_WORKSPACE_BOOKMARKS_REVSET]) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() diff --git a/src/lib-jj-helpers/src/pipeline_lock.rs b/src/lib-jj-helpers/src/pipeline_lock.rs index b3b9dea7..83601a5e 100644 --- a/src/lib-jj-helpers/src/pipeline_lock.rs +++ b/src/lib-jj-helpers/src/pipeline_lock.rs @@ -28,17 +28,39 @@ pub const PIPELINE_LOCK_FILENAME: &str = "pipeline.lock"; /// stale 判定 threshold。pipeline の実測最長 (push ~15 分) の 2x で安全マージン。 pub const PIPELINE_LOCK_STALE_SECS: i64 = 1800; -/// lock 取得成功時に保持する RAII guard。Drop で lock ファイルを削除する。 +/// lock 取得成功時に保持する RAII guard。Drop で **自分が書いた** lock ファイルのみ削除する。 pub struct PipelineLock { path: PathBuf, + /// 取得インスタンスを一意識別するランダムトークン (PR #271 CodeRabbit Major 対応)。 + token: String, } impl Drop for PipelineLock { + /// **所有権確認付き削除**: lock ファイルの token が自分のものと一致した場合のみ削除する。 + /// + /// 無条件削除だと、stale takeover 後 (別プロセス B が同じパスに B の lock を書いた後) に + /// 旧プロセス A の Drop が **B の lock を消してしまう** (CodeRabbit Major、典型的な + /// stale-lock-takeover + unconditional-unlock 問題)。token 一致確認で「他人の lock を + /// 消さない」ことを保証する。 + /// + /// 残余 TOCTOU (read → remove 間の takeover): fresh な lock は takeover されない + /// (stale threshold 到達が takeover の必要条件) ため、自分の token を read できた時点で + /// 他プロセスは未 takeover。よって「read で自分の token → その直後に他プロセスが + /// takeover」は fresh 中は起きず、実用上安全。pid/start_unix ではなく token を使うのは + /// PID 再利用による誤一致を避けるため。 fn drop(&mut self) { - if let Err(e) = std::fs::remove_file(&self.path) { - if e.kind() != std::io::ErrorKind::NotFound { - eprintln!("[pipeline-lock] cleanup 失敗: {}", e); + match std::fs::read_to_string(&self.path) { + Ok(content) => { + if parse_field(&content, "token") == Some(self.token.as_str()) { + if let Err(e) = std::fs::remove_file(&self.path) { + if e.kind() != std::io::ErrorKind::NotFound { + eprintln!("[pipeline-lock] cleanup 失敗: {}", e); + } + } + } } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => eprintln!("[pipeline-lock] cleanup 時の read 失敗: {}", e), } } } @@ -71,14 +93,15 @@ pub fn acquire_pipeline_lock_at( if let Some(parent) = path.parent() { let _ = std::fs::create_dir_all(parent); } - let content = build_lock_content(std::process::id(), now_unix, label); + let token = generate_token(); + let content = build_lock_content(&token, std::process::id(), now_unix, label); match OpenOptions::new().write(true).create_new(true).open(&path) { Ok(mut f) => { if let Err(e) = f.write_all(content.as_bytes()) { eprintln!("[pipeline-lock] 書き込み失敗 (継続): {}", e); } - PipelineLockResult::Acquired(PipelineLock { path }) + PipelineLockResult::Acquired(PipelineLock { path, token }) } Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { if let Some((pid, age_secs)) = @@ -92,7 +115,7 @@ pub fn acquire_pipeline_lock_at( if let Err(e) = std::fs::write(&path, content) { eprintln!("[pipeline-lock] takeover 書き込み失敗 (継続): {}", e); } - PipelineLockResult::Acquired(PipelineLock { path }) + PipelineLockResult::Acquired(PipelineLock { path, token }) } Err(e) => PipelineLockResult::Unavailable { reason: e.to_string(), @@ -100,6 +123,23 @@ pub fn acquire_pipeline_lock_at( } } +/// 取得インスタンスを一意識別する 128bit ランダムトークン (hex)。 +/// +/// `uuid` crate を追加せず std のみで生成する (本 crate は依存ゼロ方針)。 +/// `RandomState` は生成ごとに OS エントロピー由来のハッシュキーで初期化されるため、 +/// 空状態の `finish()` は毎回異なる値を返す。2 つ連結して 128bit の識別子とする。 +/// 暗号用途ではなく「lock インスタンスの衝突しない識別」が目的。 +fn generate_token() -> String { + use std::hash::{BuildHasher, Hasher}; + let a = std::collections::hash_map::RandomState::new() + .build_hasher() + .finish(); + let b = std::collections::hash_map::RandomState::new() + .build_hasher() + .finish(); + format!("{a:016x}{b:016x}") +} + /// fresh な pipeline lock が存在するか (Stop hook 用の読み取り専用チェック)。 /// 戻り値は `Some((holder_pid, age_secs))`。lock 不在 / stale / parse 不能は `None`。 pub fn pipeline_lock_holder(claude_dir: &Path) -> Option<(u32, i64)> { @@ -110,9 +150,10 @@ pub fn pipeline_lock_holder(claude_dir: &Path) -> Option<(u32, i64)> { ) } -fn build_lock_content(pid: u32, start_unix: i64, label: &str) -> String { +fn build_lock_content(token: &str, pid: u32, start_unix: i64, label: &str) -> String { format!( - "pid={}\nstart_unix={}\nlabel={}\n", + "token={}\npid={}\nstart_unix={}\nlabel={}\n", + token, pid, start_unix, label.replace(['\r', '\n'], " ") @@ -282,4 +323,53 @@ mod tests { let path = temp_lock_path("missing"); assert_eq!(read_fresh_lock(&path, 1800, 1_000_000), None); } + + #[test] + fn acquire_writes_a_token() { + let path = temp_lock_path("token"); + let _guard = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_000); + let content = std::fs::read_to_string(&path).unwrap(); + let token = parse_field(&content, "token").expect("token が書かれる"); + assert_eq!(token.len(), 32, "128bit hex"); + assert!(token.chars().all(|c| c.is_ascii_hexdigit())); + } + + #[test] + fn generate_token_is_unique_per_call() { + assert_ne!(generate_token(), generate_token(), "取得ごとに異なる token"); + } + + /// CodeRabbit Major #271 の regression guard: stale takeover 後に旧プロセスの Drop が + /// **新プロセスの lock を消さない**。A の guard を保持したまま同じパスを B が takeover + /// (別 token を上書き) し、A を drop しても B の lock ファイルが残ることを確認する。 + #[test] + fn drop_does_not_remove_lock_after_takeover() { + let path = temp_lock_path("takeover-guard"); + let a_guard = acquire_pipeline_lock_at(path.clone(), "A", 1800, 1_000_000); + assert!(matches!(a_guard, PipelineLockResult::Acquired(_))); + + let b_takeover_content = + build_lock_content("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", 55555, 1_000_100, "B"); + std::fs::write(&path, &b_takeover_content).unwrap(); + + drop(a_guard); + + assert!(path.exists(), "A の Drop が B の lock を消してはならない"); + let after = std::fs::read_to_string(&path).unwrap(); + assert!( + after.contains("token=bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"), + "B の lock がそのまま残る" + ); + let _ = std::fs::remove_file(&path); + } + + /// 通常ケース: 自分の token が残っていれば Drop で削除される。 + #[test] + fn drop_removes_lock_when_token_matches() { + let path = temp_lock_path("self-remove"); + let guard = acquire_pipeline_lock_at(path.clone(), "push", 1800, 1_000_000); + assert!(path.exists()); + drop(guard); + assert!(!path.exists(), "自分の token の lock は削除される"); + } } From 524097c01000cda203c42b7f76fca6fe8e919b6c Mon Sep 17 00:00:00 2001 From: aloekun Date: Tue, 14 Jul 2026 16:38:23 +0900 Subject: [PATCH 8/8] =?UTF-8?q?fix(lib-jj-helpers):=20pipeline=20lock=20?= =?UTF-8?q?=E3=81=AE=20stale=20takeover=20=E3=82=92=20atomic=E5=8C=96=20(C?= =?UTF-8?q?odeRabbit=20re-review=20Major=E5=AF=BE=E5=BF=9C)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 スレッドで実レース)。 --- src/lib-jj-helpers/src/pipeline_lock.rs | 82 ++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/src/lib-jj-helpers/src/pipeline_lock.rs b/src/lib-jj-helpers/src/pipeline_lock.rs index 83601a5e..b5f839b0 100644 --- a/src/lib-jj-helpers/src/pipeline_lock.rs +++ b/src/lib-jj-helpers/src/pipeline_lock.rs @@ -112,17 +112,67 @@ pub fn acquire_pipeline_lock_at( holder_age_secs: age_secs, }; } - if let Err(e) = std::fs::write(&path, content) { + takeover_stale_lock(path, token, content, stale_threshold_secs, now_unix) + } + Err(e) => PipelineLockResult::Unavailable { + reason: e.to_string(), + }, + } +} + +/// stale と判定した lock を takeover する。`create_new` により先着 1 プロセスのみ +/// `Acquired` になることを保証し (CodeRabbit re-review Major 対応)、レースに負けた +/// 側 (create_new が `AlreadyExists` を返す) は `busy_from_disk` で現在の holder 情報を +/// 読み直して `Busy` を返す。 +/// +/// 残余 TOCTOU: `remove_file` と `create_new` の間隙に他プロセスの `remove_file` が +/// 割り込む窓が理論上残る (完全な atomic には OS レベルの file lock が必要)。本 lock は +/// advisory (fail-open, ADR-043) であり、stale threshold 境界での同時 takeover という +/// 稀なケースに限られるため許容する。 +fn takeover_stale_lock( + path: PathBuf, + token: String, + content: String, + stale_threshold_secs: i64, + now_unix: i64, +) -> PipelineLockResult { + if let Err(e) = std::fs::remove_file(&path) { + if e.kind() != std::io::ErrorKind::NotFound { + eprintln!("[pipeline-lock] takeover 時の remove 失敗 (継続): {}", e); + } + } + match OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(mut f) => { + if let Err(e) = f.write_all(content.as_bytes()) { eprintln!("[pipeline-lock] takeover 書き込み失敗 (継続): {}", e); } PipelineLockResult::Acquired(PipelineLock { path, token }) } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + busy_from_disk(&path, stale_threshold_secs, now_unix) + } Err(e) => PipelineLockResult::Unavailable { reason: e.to_string(), }, } } +/// takeover レースに負けた際、ディスク上の現在の holder 情報から `Busy` を組み立てる。 +/// 直後に holder が drop 済みで読めない場合は `holder_pid: 0` で `Busy` を返す +/// (相手が既に確保していた事実は変わらないため `Acquired` にはしない)。 +fn busy_from_disk(path: &Path, stale_threshold_secs: i64, now_unix: i64) -> PipelineLockResult { + match read_fresh_lock(path, stale_threshold_secs, now_unix) { + Some((pid, age_secs)) => PipelineLockResult::Busy { + holder_pid: pid, + holder_age_secs: age_secs, + }, + None => PipelineLockResult::Busy { + holder_pid: 0, + holder_age_secs: 0, + }, + } +} + /// 取得インスタンスを一意識別する 128bit ランダムトークン (hex)。 /// /// `uuid` crate を追加せず std のみで生成する (本 crate は依存ゼロ方針)。 @@ -372,4 +422,34 @@ mod tests { drop(guard); assert!(!path.exists(), "自分の token の lock は削除される"); } + + /// CodeRabbit re-review Major の regression guard: 同じ stale lock に対する + /// takeover を 2 スレッドが同時に行っても、`Acquired` になるのは 1 つだけ。 + #[test] + fn concurrent_stale_takeover_only_one_wins() { + let path = temp_lock_path("concurrent-stale"); + std::fs::write(&path, "pid=99999\nstart_unix=1000000\nlabel=crashed\n").unwrap(); + + let path_a = path.clone(); + let path_b = path.clone(); + let a = std::thread::spawn(move || { + acquire_pipeline_lock_at(path_a, "A", 1800, 1_000_000 + 1800) + }); + let b = std::thread::spawn(move || { + acquire_pipeline_lock_at(path_b, "B", 1800, 1_000_000 + 1800) + }); + let result_a = a.join().unwrap(); + let result_b = b.join().unwrap(); + + let acquired_count = [&result_a, &result_b] + .into_iter() + .filter(|r| matches!(r, PipelineLockResult::Acquired(_))) + .count(); + assert_eq!( + acquired_count, 1, + "stale takeover のレースで Acquired になるのは 1 プロセスのみのはず" + ); + + let _ = std::fs::remove_file(&path); + } }