diff --git a/docs/adr/adr-055-firing-telemetry-collection.md b/docs/adr/adr-055-firing-telemetry-collection.md index 1a243504..9afcaeb8 100644 --- a/docs/adr/adr-055-firing-telemetry-collection.md +++ b/docs/adr/adr-055-firing-telemetry-collection.md @@ -75,9 +75,11 @@ warm-up 後に実データで棚卸し (step 2/3) を後続 PR で行う。本 A `decision` は「hook がツールを実際に停止したか」ではなく「発火の重み」を表す軸である。 custom rule / jj-op-verify は additionalContext の助言層で実際には block しないが、severity -に応じて block/warn を記録する。逆に stop-quality は infra エラー (stdin/parse 失敗) の -fail-closed 経路でも block を emit するため、「hook が block を emit した総数」として記録 -する。file-length gate の fail-closed 経路 (jj 失敗の判定不能 block) は ROI 信号を汚さない +に応じて block/warn を記録する。stop-quality は当初 infra エラー (stdin/parse 失敗) の +fail-closed 経路でも block を emit するため「hook が block を emit した総数」として記録して +いたが、後述の Amendment (2026-07-29) でこの定義を撤回し、実 quality 違反 (品質ステップ +失敗) の block のみ記録するよう限定した (WP-12 ROI 信号から infra ノイズを除外)。 +file-length gate の fail-closed 経路 (jj 失敗の判定不能 block) は当初から ROI 信号を汚さない よう記録しない。 ### 副作用注入によるテスト可能性 @@ -239,6 +241,44 @@ stop-feedback-dispatch / user-prompt-feedback-recovery は本 PR では計装し これらにも当てはまるが、計装は各 hook を触る PR で個別に行う (ADR-059 段階展開に連動)。 §計装スコープ の除外リストは本 amendment に合わせて更新した。 +## Amendment (2026-07-29): block 記録を実 quality 違反に限定 (順位309、WP-12 ROI 信号の精度) + +初版 § 計装スコープ は stop-quality の block 記録を「hook が block を emit した総数」と +定義し、infra エラー (stdin 読込 / JSON parse 失敗) の fail-closed 経路も計上していた。 +WP-12 step 2/3 (発火数で hook の維持・削除を判断する ROI 棚卸し) の観点では、この infra +エラー混入が発火数を歪め「実際に品質違反を捕捉した回数」と乖離する。CodeRabbit Major 指摘 +(順位309) を受け、**block 記録を実 quality 違反 (品質ステップ失敗) パス限定に絞り込む**。 + +### 3 hook の状態 (計装スコープ表のうち block を emit する hook) + +| hook | 修正前 | 修正後 | +|---|---|---| +| hooks-stop-quality | stdin/parse 失敗の fail-closed block でも記録 | `emit_block(reason, cause: BlockCause)` に変更し、`cause.records_firing()` (QualityViolation のみ真) の場合だけ telemetry に記録する。infra エラー経路 (`read_stdin_or_block` / `parse_hook_input_or_block`) と worker thread panic (`run_quality_steps` の join 失敗) は `InfraError` 扱いで block decision のみ emit し記録しない。実 quality 違反 (品質ステップ失敗) のみ `QualityViolation` | +| hooks-stop-tool-call-leak | (変更なし) | `emit_block` は `run_check` の実 leak 検出時のみ呼ばれ、transcript 読取失敗・連続数 0・上限到達は fail-open で return するため既に実 leak 限定。ADR-061 の回収層 (`prompt-recovery`) は `should_recover` 成立時のみ warn 記録 | +| hooks-pre-tool-validate | (変更なし) | `record_preset_block` は `validate_command` が hit を返す (= preset にマッチした実 violation) 経路のみ。stdin/parse 失敗は `ExitCode::FAILURE` で return し記録しない。既に preset match 限定 | + +### 設計: 経路を型で区別しテストで固定 + +stop-quality に `BlockCause { QualityViolation, InfraError }` を導入し、`emit_block(reason, cause)` +が `cause.records_firing()` (QualityViolation のみ真) のときだけ telemetry に記録する。 +telemetry 記録を closure 注入した `emit_block_with` をテストの芯とし、「QualityViolation は +recorder を発火 / InfraError は発火しない」を副作用で観測して回帰ガードにする。leak / preset +は record 呼び出しが既に violation 経路にしか存在しないため、コード構造でこの不変条件が保たれる +(追加のテストは設けない)。 + +また `run_quality_steps` の各失敗は `StepFailure { message, cause }` で由来を保持し、worker +thread panic (join の `Err`) は実 quality 違反ではないため `InfraError`、ステップの実失敗は +`QualityViolation` とする。`block_on_failures` は `aggregate_block_cause` で全体の cause を +決め (1 件でも実失敗があれば `QualityViolation`、全て panic なら `InfraError`)、`emit_block` +に渡す。これにより内部障害 (panic) を品質違反として ROI 信号に誤計上しない (CodeRabbit Major 指摘)。 + +### 帰結 + +- 「発火 0 = 削除候補」の ROI 信号が infra エラーの fail-closed block で汚染されなくなり、 + 発火数は「hook が実際に品質違反を捕捉した回数」を表す。 +- fail-open 原則は不変。telemetry 記録の有無に関わらず block decision 自体は emit するため、 + infra エラー時も Claude への block 通知は従来どおり行われ、ゲート挙動は変わらない。 + ## 関連 ADR - [ADR-039](adr-039-experimental-feature-standard-pattern.md) — 試験運用標準パターン (opt-in / kill-switch / bounded lifetime) diff --git a/docs/todo-summary2.md b/docs/todo-summary2.md index 42e5dca6..3183f2a5 100644 --- a/docs/todo-summary2.md +++ b/docs/todo-summary2.md @@ -69,7 +69,6 @@ | 306 | 💎 Tier 3 | **quality gate isolation 機構を見送り、recovery による risk acceptance とした判断の記録 (negative result) (273.md T3-5 採用)** | todo16.md | S | なし (spike 見送り convention に従い、isolation 機構を却下し recovery コストの低さ (順位304) を理由に risk acceptance した根拠を記録。recovery は isolation の代替ではなく、予防機能の欠如という残存リスクと再検討条件を明記する) | | 307 | 🔧 Tier 2 | **WP-12 step 2: 発火テレメトリ ROI 棚卸し pre-step (発火 0 の rule/preset/hook を削除候補提示)** | todo16.md | M | なし (**着手条件 = ADR-055 収集層マージから 28 日 warm-up 後**。それ以前は全項目が発火 0 = データ無しで判定無意味。集計は Rust exe、weekly-review に file-length-watchlist 同型 facet で接続、incident 由来ルールは発火 0 でも維持推奨の区別) | | 308 | 💎 Tier 3 | **WP-12 step 3: ADR-039 bounded lifetime 判定の発火数機械化** | todo16.md | S | 順位 307 (step 2 の集計基盤に依存)。試験運用 ADR 機構の卒業/廃止検討を発火数で自動 promote。step 3 完了で WP-12 完了 | -| 309 | 🚀 Tier 1 | **telemetry の block 記録を実 quality 違反に限定（infra エラー混入除外）(275.md T1-1 採用)** | todo16.md | M | なし (CodeRabbit Major。ADR-055 で「emit 総数」と意図的定義したが WP-12 ROI 棚卸しが infra エラー〔stdin/parse 失敗〕混入で歪むため実 violation パス限定に絞る。3 hook 横断で分割 PR 推奨、ADR-055 amendment 併記) | | 310 | 🚀 Tier 1 | **custom-regex preset の生 regex が telemetry id に流れる privacy footgun 是正（非ブロッキング follow-up 統合）(275.md T1-2 採用)** | todo16.md | S | なし (現行 config は named preset のみで非発火だが派生プロジェクトの latent footgun。fallback を合成 id〔"custom-block"〕に正規化 + ADR-055 に config privacy 注記) | | 311 | 🚀 Tier 1 | **逐語的関数複製（3+ コピー）を pre-push 検出する DRY lint rule (275.md T1-3 採用)** | todo16.md | M | なし (is_truthy 三重複製事案。ADR-007 regex 層に threshold 検出追加。順位 313 の fixture と抱き合わせ) | | 312 | 🔧 Tier 2 | **`.claude/telemetry/` の per-pid×日次 partition ファイル retention/cleanup (275.md T2-1 採用)** | todo16.md | M | 順位 307 (WP-12 step 2 と同時期＝step1 マージから 28 日後 2026-08-12 頃に着手) | diff --git a/docs/todo16.md b/docs/todo16.md index c2f2fa9d..6bb4d893 100644 --- a/docs/todo16.md +++ b/docs/todo16.md @@ -272,29 +272,6 @@ --- -### telemetry の block 記録を実 quality 違反に限定（infra エラー混入の除外）(275.md T1-1 採用) - -> **動機**: CodeRabbit Major 指摘。`emit_block` / `record_*_firing` が品質違反だけでなく fail-closed の infra エラー（stdin 読込失敗 / JSON parse 失敗）でも発火を記録する。ADR-055 では「hook が block を emit した総数」として意図的にこの設計にしたが、WP-12 の ROI 棚卸し（発火数で hook 維持を判断）では infra エラー混入が発火数を歪めるため、実 quality 違反パス（`block_on_failures` 等）限定に絞り込む方が信号が正確になる。 -> -> **重要**: これは ADR-055 で「意図的」と記録した判断の見直しであり、実装時は ADR-055 の該当記述（emit 総数の定義）も併せて amendment する。3 hook 横断（hooks-stop-quality / hooks-stop-tool-call-leak / hooks-pre-tool-validate）のため実装は分割 PR 推奨。stop-tool-call-leak は実 leak でのみ emit_block を呼ぶため既に実質限定されている点も確認する。 -> -> **参照**: `.claude/feedback-reports/275.md` Tier 1 #1、`src/hooks-stop-quality/src/main.rs`（`emit_block` / `record_block_firing`）、[ADR-055](adr/adr-055-firing-telemetry-collection.md) § 計装スコープ、WP-12 step 2（順位 307、集計精度の前提）。 -> -> **実行優先度**: 🚀 Tier 1 — Severity Medium / Effort M。 - -#### 作業計画 - -- [ ] 各 hook の記録呼び出しを実 quality 違反パス限定に移動（infra エラー経路では記録しない）。record 位置の見直し。 -- [ ] [ADR-055](adr/adr-055-firing-telemetry-collection.md) の「emit 総数」定義を amendment（実 violation 限定に方針変更した根拠を記録）。 -- [ ] 各 hook のユニットテストで「infra エラー経路では telemetry を記録しない」ことを検証。 -- [ ] 本エントリ削除 + todo-summary2.md 行削除。 - -#### 完了基準 - -- telemetry の block 記録が実 quality 違反に限定され、infra エラー（stdin/parse 失敗）では記録されないことがテストで保証され、ADR-055 の定義も整合していること。 - ---- - ### custom-regex preset の生 regex が telemetry id に流れる privacy footgun の是正（非ブロッキング follow-up 統合）(275.md T1-2 採用) > **動機**: PR #275 の pre-push simplicity review 非ブロッキング warning（= セッション中に検出された「非ブロッキング follow-up」）。`tag_source(name, ...)` の `name` が named preset 名でなく `blocked_patterns` の生正規表現文字列の場合、その regex テキストがそのまま telemetry の `id` フィールドに載り、ADR-055 の「コマンド本文・内容は非記録」プライバシー原則と緊張する。現行 `hooks-config.toml` は named preset のみのため**非発火**だが、派生プロジェクトが raw-regex エントリを足すと該当する latent footgun。 diff --git a/src/hooks-stop-quality/src/main.rs b/src/hooks-stop-quality/src/main.rs index bdd4f862..4af273cb 100644 --- a/src/hooks-stop-quality/src/main.rs +++ b/src/hooks-stop-quality/src/main.rs @@ -72,9 +72,37 @@ struct QualityStepConfig { /// デフォルトのステップタイムアウト（秒） const DEFAULT_STEP_TIMEOUT_SECS: u64 = 60; -/// block 判定を stdout に出力するヘルパー -fn emit_block(reason: &str) { - record_block_firing(); +/// block 判定の発火源。telemetry 記録対象 (実 quality 違反) か、infra エラー +/// (stdin 読込 / JSON parse 失敗の fail-closed block) かを区別する。 +/// +/// 順位309 / ADR-055 § 計装スコープ amendment: WP-12 ROI 棚卸し (発火数で hook 維持を +/// 判断する) の信号を歪めないため、infra エラーの fail-closed block は telemetry に +/// 記録しない。実 quality 違反 (品質ステップ失敗) のみを「hook が発火した」として計上する。 +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum BlockCause { + QualityViolation, + InfraError, +} + +impl BlockCause { + /// この block を telemetry の firing として記録すべきか。実 quality 違反のみ true。 + fn records_firing(self) -> bool { + matches!(self, BlockCause::QualityViolation) + } +} + +/// block 判定を stdout に出力するヘルパー。実 quality 違反 (`QualityViolation`) のみ +/// telemetry に firing を記録する (`InfraError` は記録しない、順位309)。 +fn emit_block(reason: &str, cause: BlockCause) { + emit_block_with(reason, cause, record_block_firing); +} + +/// [`emit_block`] のテスト可能な芯。telemetry 記録を closure で注入し、`cause` による +/// 記録有無をテストが副作用で観測できるようにする (prod は [`record_block_firing`])。 +fn emit_block_with(reason: &str, cause: BlockCause, record_firing: impl FnOnce()) { + if cause.records_firing() { + record_firing(); + } let decision = BlockDecision { decision: "block".to_string(), reason: reason.to_string(), @@ -84,8 +112,9 @@ fn emit_block(reason: &str) { } } -/// Stop 品質ゲートが block を発火したこと (品質失敗・fail-closed infra エラーを含む -/// emit 総数) を telemetry に記録する (WP-12、fail-open)。 +/// Stop 品質ゲートが実 quality 違反で block を発火したことを telemetry に記録する +/// (WP-12、fail-open)。infra エラー (stdin/parse 失敗) の fail-closed block はこの経路を +/// 通さず記録しない (順位309、ADR-055 § 計装スコープ amendment)。 fn record_block_firing() { lib_telemetry::record(&lib_telemetry::Firing { hook: "hooks-stop-quality", @@ -254,10 +283,10 @@ fn pipeline_is_running() -> bool { 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 - )); + emit_block( + &format!("品質ゲートエラー: stdin読み込みに失敗しました: {}", e), + BlockCause::InfraError, + ); return None; } Some(input) @@ -268,10 +297,10 @@ 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 - )); + emit_block( + &format!("品質ゲートエラー: 入力JSONのパースに失敗しました: {}", e), + BlockCause::InfraError, + ); None } } @@ -338,6 +367,16 @@ fn expand_step_placeholders(cmd: &str) -> String { expanded.replace("{{CLAUDE_DIR}}", &normalized) } +/// 1 step の失敗理由と、その失敗の由来 (`BlockCause`) を保持する。 +/// +/// worker thread panic は実 quality 違反ではなく infra エラーであるため、 +/// `run_quality_steps` の時点で由来を記録しておく (順位309 / ADR-055 § 計装スコープ +/// amendment: panic を `QualityViolation` として誤計上しないため)。 +struct StepFailure { + message: String, + cause: BlockCause, +} + /// 各ステップを並列に実行し、失敗を step 定義順で集約する (WP-05)。 /// /// 逐次実行では合計時間が全ステップの和になり Stop hook が肥大化していた @@ -346,8 +385,9 @@ fn expand_step_placeholders(cmd: &str) -> String { /// 並列化し、総時間を最遅ステップまで短縮する。網羅性は全ステップ実行で維持。 /// /// 失敗集約は spawn 順 (= step 定義順) を保つため決定論的。worker が panic した場合は -/// fail-closed で failure 扱いにして block する (品質ゲートを黙って通さない)。 -fn run_quality_steps(steps: &[QualityStepConfig], timeout: u64) -> Vec { +/// fail-closed で failure 扱いにして block するが、由来は `BlockCause::InfraError` +/// として記録する (実 quality 違反ではないため)。 +fn run_quality_steps(steps: &[QualityStepConfig], timeout: u64) -> Vec { let handles: Vec<(String, std::thread::JoinHandle<(bool, String)>)> = steps .iter() .map(|step| { @@ -362,34 +402,70 @@ fn run_quality_steps(steps: &[QualityStepConfig], timeout: u64) -> Vec { }) .collect(); - let mut failures: Vec = Vec::new(); - for (name, handle) in handles { - match handle.join() { - Ok((success, output)) => { - if !success { - failures.push(format!("**{}** failed:\n```\n{}\n```", name, output)); - } - } - Err(_) => { - failures.push(format!( - "**{}** failed: worker thread が panic しました (fail-closed)", - name - )); - } - } + handles + .into_iter() + .filter_map(|(name, handle)| step_failure_from_join(&name, handle.join())) + .collect() +} + +/// 1 step の `JoinHandle::join()` 結果を `StepFailure` に変換する (成功時は `None`)。 +/// +/// worker panic (`Err`) は実 quality 違反ではないため `BlockCause::InfraError` として +/// 記録する (順位309: panic を telemetry の quality 違反として誤計上しないため)。 +fn step_failure_from_join( + name: &str, + result: std::thread::Result<(bool, String)>, +) -> Option { + match result { + Ok((true, _)) => None, + Ok((false, output)) => Some(StepFailure { + message: format!("**{}** failed:\n```\n{}\n```", name, output), + cause: BlockCause::QualityViolation, + }), + Err(_) => Some(StepFailure { + message: format!( + "**{}** failed: worker thread が panic しました (fail-closed)", + name + ), + cause: BlockCause::InfraError, + }), + } +} + +/// 失敗一覧全体としての `BlockCause` を決定する。 +/// +/// 1 件でも実 quality 違反 (`BlockCause::QualityViolation`) があれば、実際に品質 +/// チェックが失敗したことを意味するため全体を `QualityViolation` とする。全失敗が +/// worker panic (`BlockCause::InfraError`) のみの場合に限り `InfraError` とする +/// (順位309: panic 単体を quality 違反として telemetry に誤計上しない)。 +fn aggregate_block_cause(failures: &[StepFailure]) -> BlockCause { + if failures + .iter() + .any(|f| f.cause == BlockCause::QualityViolation) + { + BlockCause::QualityViolation + } else { + BlockCause::InfraError } - failures } -fn block_on_failures(failures: &[String]) { +fn block_on_failures(failures: &[StepFailure]) { if failures.is_empty() { return; } let reason = format!( "品質ゲートが失敗しました。以下の問題を修正してください:\n\n{}", - failures.join("\n\n") + join_failure_messages(failures) ); - emit_block(&reason); + emit_block(&reason, aggregate_block_cause(failures)); +} + +fn join_failure_messages(failures: &[StepFailure]) -> String { + failures + .iter() + .map(|f| f.message.as_str()) + .collect::>() + .join("\n\n") } #[cfg(test)] @@ -511,6 +587,31 @@ cmd = "pnpm test" assert!(json.contains(r#""reason":"test failed""#)); } + /// 順位309: 実 quality 違反 (品質ステップ失敗) の block は telemetry recorder を発火する。 + #[test] + fn quality_violation_block_invokes_the_telemetry_recorder() { + let mut recorded = false; + emit_block_with("品質ゲート失敗", BlockCause::QualityViolation, || { + recorded = true + }); + assert!(recorded, "実 quality 違反の block は firing を記録する"); + } + + /// 順位309 / ADR-055 § 計装スコープ amendment: infra エラー (stdin/parse 失敗) の + /// fail-closed block は ROI 信号 (発火数で hook 維持を判断) を歪めるため telemetry + /// recorder を発火しない。record 位置を実 violation 経路限定に移した回帰ガード。 + #[test] + fn infra_error_block_does_not_invoke_the_telemetry_recorder() { + let mut recorded = false; + emit_block_with("stdin 読み込み失敗", BlockCause::InfraError, || { + recorded = true + }); + assert!( + !recorded, + "infra エラーの fail-closed block は firing を記録しない (順位309)" + ); + } + #[test] fn step_timeout_default_is_reasonable() { const { assert!(DEFAULT_STEP_TIMEOUT_SECS >= 30) }; @@ -546,22 +647,72 @@ cmd = "pnpm test" assert_eq!(failures.len(), 2, "失敗した 2 ステップのみ集約される"); assert!( - failures[0].contains("fail-b"), + failures[0].message.contains("fail-b"), "spawn 順 = step 定義順を保つ (fail-b が先): {:?}", - failures + failures[0].message ); assert!( - failures[1].contains("fail-d"), + failures[1].message.contains("fail-d"), "spawn 順 = step 定義順を保つ (fail-d が後): {:?}", - failures + failures[1].message ); assert!( - !failures.iter().any(|f| f.contains("pass-")), - "成功ステップは failure に含まれない: {:?}", - failures + !failures.iter().any(|f| f.message.contains("pass-")), + "成功ステップは failure に含まれない" ); } + /// 順位309: 実際にステップが失敗した場合 (panic ではない) は `QualityViolation` + /// として記録される。 + #[test] + fn step_failure_from_join_tags_real_failures_as_quality_violation() { + let failure = step_failure_from_join("step", Ok((false, "boom".to_string()))) + .expect("失敗した step は Some を返す"); + assert_eq!(failure.cause, BlockCause::QualityViolation); + } + + /// 順位309 / ADR-055 § 計装スコープ amendment: worker panic は `InfraError` として + /// 記録される (実 quality 違反として telemetry に誤計上しない回帰ガード)。 + #[test] + fn step_failure_from_join_tags_panics_as_infra_error() { + let result: std::thread::Result<(bool, String)> = Err(Box::new("panic")); + let failure = step_failure_from_join("step", result).expect("panic は Some を返す"); + assert_eq!(failure.cause, BlockCause::InfraError); + } + + #[test] + fn step_failure_from_join_returns_none_for_success() { + assert!(step_failure_from_join("step", Ok((true, String::new()))).is_none()); + } + + /// 順位309: 実 quality 違反が 1 件でもあれば全体は `QualityViolation` とする + /// (panic と混在していても、実際にチェックが失敗した事実は変わらないため)。 + #[test] + fn aggregate_block_cause_is_quality_violation_when_any_failure_is_a_violation() { + let failures = vec![ + StepFailure { + message: "panic".to_string(), + cause: BlockCause::InfraError, + }, + StepFailure { + message: "real failure".to_string(), + cause: BlockCause::QualityViolation, + }, + ]; + assert_eq!(aggregate_block_cause(&failures), BlockCause::QualityViolation); + } + + /// 順位309 / ADR-055 § 計装スコープ amendment: 失敗が全て worker panic の場合のみ + /// `InfraError` とする (panic 単体を quality 違反として誤計上しない回帰ガード)。 + #[test] + fn aggregate_block_cause_is_infra_error_when_all_failures_are_panics() { + let failures = vec![StepFailure { + message: "worker thread が panic しました".to_string(), + cause: BlockCause::InfraError, + }]; + assert_eq!(aggregate_block_cause(&failures), BlockCause::InfraError); + } + /// T7: ADR-010 の実配置 `/.claude/.exe` からルートを導出する。 #[test] fn project_root_from_exe_derives_parent_of_claude_dir() {