diff --git a/docs/coderabbit-monitoring-efficiency.md b/docs/coderabbit-monitoring-efficiency.md index 7bb16ea..6a6451f 100644 --- a/docs/coderabbit-monitoring-efficiency.md +++ b/docs/coderabbit-monitoring-efficiency.md @@ -44,9 +44,14 @@ ADR-018 が廃止したのは「同一プロセス内で 4 段間接連携 (daem - ✅ **Bb-1 (順位 53) — マージ済 (PR #113、2026-05-05)**: `cli-pr-monitor` の rate-limit retry を CronCreate park モデルに切り替え。post-merge-feedback で T2-2 のみ採用 (順位 75)、T3-1/T3-2 はユーザー判断で却下 (memory: feedback_no_unenforced_rules)。 - ✅ **Bb-2 (順位 54) + 順位 75 (T2-2 Bb-1 follow-up) — マージ済 (PR #114、2026-05-05)**: review 完了待ちを CronCreate park モデルに展開し、polling 完全排除 + 二重 polling (45s gh API + 5s observer) を撤廃。T2-2 の sibling parity 回帰テストを同 PR で bundled。CR Major 2 件 (head 不一致時の wakeup 誤判定 / fresh push での recheck count 持ち越し) は fold-in 修正で land。post-merge-feedback で 2 件採用 / 5 件様子見 / 3 件却下 (詳細レポート: `.claude/feedback-reports/114.md`)。 -- ✅ **Bb-3 (順位 55) — 実装完了 (2026-05-05、PR 起票準備中)**: `[review_recheck]` config セクションを `pr-monitor-config.toml` に追加し旧 hard-coded const (INITIAL_REVIEW_WAIT_SECS / REVIEW_RECHECK_WAIT_SECS / MAX_REVIEW_RECHECKS) を `PollContext` 経由で thread。SessionStart hook (`hooks-session-start`) に catch-up nudge を追加 (state.action が `parked_*` かつ `next_wakeup_at_unix <= now` のとき additionalContext に `[PR_MONITOR_CATCHUP]` を注入)。MonitorConfig::poll_interval_secs (Bb-2 で未使用化) を削除し既存 toml は legacy フィールド無視で後方互換。T2-2 follow-up として `finalize_park_siblings_have_symmetric_write_state_handling` を 3 sibling 化 (`finalize_initial_review_park` を追加)。 +- ✅ **Bb-3 (順位 55) — マージ済 (PR #115、2026-05-06)**: `[review_recheck]` config セクションを `pr-monitor-config.toml` に追加し旧 hard-coded const (INITIAL_REVIEW_WAIT_SECS / REVIEW_RECHECK_WAIT_SECS / MAX_REVIEW_RECHECKS) を `PollContext` 経由で thread。SessionStart hook (`hooks-session-start`) に catch-up nudge を追加 (state.action が `parked_*` かつ `next_wakeup_at_unix <= now` のとき additionalContext に `[PR_MONITOR_CATCHUP]` を注入)。MonitorConfig::poll_interval_secs (Bb-2 で未使用化) を削除し既存 toml は legacy フィールド無視で後方互換。T2-2 follow-up として `finalize_park_siblings_have_symmetric_write_state_handling` を 3 sibling 化 (`finalize_initial_review_park` を追加)。 - **設計判断: SessionStart catch-up は別プロセス spawn せず additionalContext で nudge** (advisor 推奨 option b)。Windows 環境での handle 継承 / stdout 可視性問題を回避し、PARK signal flow を session 内に保つ。 - **false-positive 抑制**: terminal action (`action_required` / `passed_clean` / `continue_monitoring`) では `next_wakeup_at_unix` が古い park 由来で残っていても nudge を出さない (action prefix `parked_` を gate に使用)。 + - **fold-in 修正 (PR review 中に発生)**: CR Major #1 (`max_review_rechecks=0` / `wait_secs=0` / overflow) → `ReviewRecheckConfig::sanitize()` を実装し `load_config` 経路で適用。CR Major #2 (sanitize 自身の境界値テストが overflow を許容する自己矛盾) → 上限を `i64::MAX` から `MAX_SAFE_WAIT_SECS = 1 年 (31,536,000秒)` に変更し `now_unix + wait_secs as i64` の overflow を構造的に防止。CR から両 Major fix に positive 評価あり (``)。 + - **dogfood 実証**: PR #115 自身で Bundle b 全機能 (Bb-1 rate-limit park + 自動 retrigger / Bb-2 wakeup 経路 / Bb-2 review_recheck count / Bb-3 sanitize) が連携動作。`@coderabbitai review` 自動投稿 → review_recheck park → wakeup → 再 review 取得のサイクルを実 PR で完走。 + - **post-merge-feedback で 3 件採用 / 4 件様子見 / 3 件却下** (詳細レポート: `.claude/feedback-reports/115.md`)。採用 3 件は順位 76 (cross-module overflow 統合テスト) / 順位 77 (Unix timestamp baseline 境界値テスト) / 順位 78 (ADR-038 + CLAUDE.md security 拡充) として todo.md に登録済。 + +**🎉 Bundle b 完成**: Bb-1 / Bb-2 / Bb-3 全 3 PR + 順位 75 (T2-2 Bb-1 follow-up) + 順位 76-78 (Bb-3 follow-up) = CronCreate park モデルへの移行完結。同プロセス常駐型 polling から「Rust 状態管理 + Claude Code 周期確認」の責務分離 (ADR-030 パターンの 3 つ目の適用例) へ転換。 --- diff --git a/docs/todo.md b/docs/todo.md index 29b6fb2..a6e1005 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -55,7 +55,6 @@ | 49 | 🔧 Tier 2 | **`parse_findings` 系の error-path test infrastructure (PR #101 T2-1) ★ Bundle a Sub-PR 2** | todo5.md | M | 順位 42 / 43 / 46 と同 PR (Sub-PR 2、`unwrap_or_else(\|_\| empty)` silent fail 抑止 + cli-pr-monitor mock infra 流用) | | 51 | 🚀 Tier 1 | **`.takt/review-diff.txt` を fix→review iteration 間で refresh (PR #103 観測)** | todo5.md | M | なし (PR #103 で stale-diff false positive による wasted iter ×2 = ~10 分浪費を実観測、6-iter outlier の構造的根因対策、Bundle Z 3 層では塞げない独立改善) | | 52 | 💎 Tier 3 | **comment-lint hook の MultiEdit 対応 (順位 50 follow-up)** | todo5.md | S | なし (順位 50 で v1 = Edit のみ実装、MultiEdit は whole-file fallback で no-regression、利用頻度低く優先度は低) | -| 55 | 💎 Tier 3 | **✅ 完了 (Bb-3、PR 起票準備中、2026-05-05): config 拡張 + SessionStart catch-up + T2-2 follow-up (Parity test coverage 拡張)** | todo5.md | S+S | 完了内容: `[review_recheck]` config セクション追加 + `MonitorConfig::poll_interval_secs` 削除 + `PollContext` scalar threading + SessionStart hook の catch-up nudge (action=`parked_*` gate で false-positive 抑制) + parity test を 3 sibling 化。設計判断詳細は `docs/coderabbit-monitoring-efficiency.md` Bb-3 エントリ | | 56 | 🔧 Tier 2 | **comment-lint hook test 拡充 (PR #104 T2-1+T2-2 bundle)** | todo5.md | S | なし (UTF-8 multi-byte 5 パターン + block comment boundary 6 パターンを `locate_string_line_ranges` / `span_overlaps_ranges` の回帰テストとして体系化、PR #104 Critical/Minor fix の固定化) | | 57 | 🔧 Tier 2 | **Aggregation cap integration test (PR #105 T2-1 採用)** | todo5.md | S | なし (`collect_all_violations` の MAX_VIOLATIONS contract を test 化、将来の lint 追加時に `truncate(MAX)` 削除 regression を防止する explicit 安全網) | | 60 | 💎 Tier 3 | **analyze-session の transcript filter 絞り込み (旧 #A-3)** | todo5.md | M | なし (旧 docs/pipeline-token-efficiency.md #A-3、ADR-036/037 化に伴い計画書削除、本 task のみ todo に移管。analyze-session の input range を PR 作成 commit〜merge に限定して input token 30-50% 削減見込み、dogfood で実測必要) | @@ -67,6 +66,9 @@ | 67 | 💎 Tier 3 | **ADR-030 に abrupt 終了時の振る舞いを spec として明記 (PR #109 T3-2 採用) ★ Bundle c** | todo5.md | XS | 順位 63 / 64 と同 PR (実装と仕様の整合性確保、L1 in-process Drop guard + L2 out-of-process reaper の責務分離 + SLA 化) | | 68 | 🔧 Tier 2 | **`no-ephemeral-todo-reference` self-exclusion invariant 単体テスト追加 (PR #110 T2-1 採用) ★ Bundle d** | todo5.md | S | なし (PR #110 直接対策、placeholder N 戦略の machine-enforceable 保護、TP/FP/Edge 3 軸テスト) | | 69 | 💎 Tier 3 | **`no-ephemeral-todo-reference` の `yaml`/`yml` extensions 追加理由をコメントで明記 (PR #110 T3-1 採用) ★ Bundle d** | todo5.md | XS | なし (rule⑥ コメント欄に 1-2 行追記、設計 doc と実装の経緯保存、git blame 不要化) | +| 76 | 🔧 Tier 2 | **config sanitize → poll arithmetic の cross-module overflow 統合テスト (PR #115 T2-1 採用) ★ Bb-3 follow-up** | todo5.md | M | なし (PR #115 で `review_recheck_sanitize_keeps_i64_max_boundary` が unit test isolation のため cross-module overflow を見逃した実証ベース、`MAX_SAFE_WAIT_SECS` / `0` / `u64::MAX inject` の全 code path で `poll.rs` の `now_unix + wait_secs as i64` が overflow しないことを統合 test で machine-enforce) | +| 77 | 🔧 Tier 2 | **Unix timestamp baseline での境界値テスト matrix (PR #115 T2-2 採用) ★ Bb-3 follow-up** | todo5.md | S | なし (`MAX_SAFE_WAIT_SECS = 1年` の根拠が現在の timestamp ~1.7e9 に依存する time-dependent。`now + 0` / `now + MAX_SAFE_WAIT_SECS` / `now + MAX_SAFE_WAIT_SECS + 1` の境界値で i64 overflow safety を自動検証、user-editable config boundary として今後の変更にも追随) | +| 78 | 💎 Tier 3 | **ADR-038 (Rust timestamp arithmetic safety) + CLAUDE.md security 拡充 (PR #115 T3-1 採用) ★ Bb-3 follow-up** | todo5.md | S | なし (config が user-editable system boundary のとき `sanitize()` 値域検証を必須化し dependent arithmetic に `// SAFETY: により上限保証` コメントを要求するパターンを ADR + CLAUDE.md に codify、Rust 固有の checked_add + MAX_SAFE capping + time-dependent test の 3 層を明文化) | **戦略**: 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/todo5.md b/docs/todo5.md index e45ba49..ab754d2 100644 --- a/docs/todo5.md +++ b/docs/todo5.md @@ -656,3 +656,128 @@ なし (Effort XS、コメント追記のみ) +--- + +### config sanitize → poll arithmetic の cross-module overflow 統合テスト (PR #115 T2-1 採用) ★ Bb-3 follow-up + +> **動機**: PR #115 の sanitize() 実装後、CR から Major #2 として「`review_recheck_sanitize_keeps_i64_max_boundary` テストが `i64::MAX as u64` を valid として通すが、poll.rs の `now_unix + wait_secs as i64` で確実に overflow する」という **unit test isolation 起因の自己矛盾**を指摘された。即修正で `MAX_SAFE_WAIT_SECS = 1 年` に上限を変更したが、原因は **「config layer の単体テストが downstream arithmetic の overflow safety を確認していなかった」**こと。同類の cross-module integrity 問題は今後も sanitize() に新フィールド追加・既存値の境界変更で再発しうる。 +> +> **本タスクの位置づけ**: Bb-3 完成 (PR #115 マージ済) 後の follow-up。post-merge-feedback Tier 2 #1 採用。 +> +> **参照**: PR #115 CR Major #2 (id 3192783887)、`.claude/feedback-reports/115.md` Tier 2 #1、`src/cli-pr-monitor/src/config.rs` (sanitize)、`src/cli-pr-monitor/src/stages/poll.rs` line 662 / 734 (cast points) +> +> **実行優先度**: 🔧 **Tier 2** — Effort M。順位 77 (境界値テスト matrix) と同一 PR で land すると DRY (両方とも `wait_secs` boundary を扱う)。 + +#### 設計決定 (案) + +- **テストの配置**: `src/cli-pr-monitor/src/stages/poll.rs` の `#[cfg(test)] mod tests` 内に integration test 風のセクションを追加 (config + poll を貫通させる) +- **テストの形**: 以下 3 経路を verify + 1. `MAX_SAFE_WAIT_SECS` を sanitize() 通過後、`finalize_initial_review_park` / `schedule_next_review_recheck_park` 経由で書き込まれる `state.next_wakeup_at_unix` が `i64::MAX` 以下 (= overflow しない) + 2. `0` を sanitize() で default に置換後、書き込まれる `state.next_wakeup_at_unix` が `now_unix + 300` (default fallback 動作) + 3. `u64::MAX` を sanitize() で default に置換後、書き込まれる `state.next_wakeup_at_unix` が `i64::MAX` 以下 (= overflow しない) +- **invariant assertion**: `state.next_wakeup_at_unix > now_unix && state.next_wakeup_at_unix < i64::MAX` を全 path で assert +- **test fault injection**: 既存の `PR_MONITOR_STATE_FILE_OVERRIDE` env var + `env_override_lock` Mutex を再利用 (T2-2 で確立済の pattern) + +#### 作業計画 + +- [ ] `poll.rs` test module に `cross_module_overflow_safety_<シナリオ>` テスト 3 件追加 +- [ ] 全 path で `state.next_wakeup_at_unix` の値域 invariant を assert +- [ ] `sanitize() を経由しない直接 inject 経路` (= テスト用 helper で異常値を直接書く) も対象に追加し、sanitize layer がない場合の overflow を **negative test** で確認 (sanitize の保護が機能していることの裏付け) +- [ ] cargo test 全 pass を確認、CI 統合 +- [ ] 派生プロジェクト deploy には影響なし (test only) +- [ ] 本 todo5.md エントリを削除 + +#### 完了基準 + +- 統合テスト 3-4 件追加 (positive + negative) +- `now_unix + sanitize 後の wait_secs` が i64 overflow しないことを machine-enforce +- 将来 `MAX_SAFE_WAIT_SECS` を変更したり sanitize() に新フィールド追加した際、テストが落ちて invariant 違反を検知 + +#### 詰まっている箇所 + +- `now_unix` の取得を test 環境で固定する仕組み (現実装は `SystemTime::now()` で test 不安定要因)。最低限「`SystemTime::now() + sanitize 後の値 < i64::MAX`」が常に成立することを assert する形で済ます (実環境の `now_unix` は ~1.78e9 << i64::MAX なので、sanitize 後の値が `MAX_SAFE_WAIT_SECS = 1 年 = 3.15e7` 以下なら確実に safe) + +--- + +### Unix timestamp baseline での境界値テスト matrix (PR #115 T2-2 採用) ★ Bb-3 follow-up + +> **動機**: `MAX_SAFE_WAIT_SECS = 365 * 24 * 60 * 60` の根拠 (i64 overflow safety) は **現在の Unix timestamp が ~1.78e9 (2026 年)** に依存する time-dependent な前提。例えば 2100 年の時点では `now_unix` が ~4.1e9 に達し、`now_unix + MAX_SAFE_WAIT_SECS` が i64::MAX に対する safety margin が縮む (= 計算上は安全だが、根拠の説明性が弱まる)。境界値を明示的にテストすることで `MAX_SAFE_WAIT_SECS` の妥当性を future-proof に保証。 +> +> **本タスクの位置づけ**: 順位 76 と同一 PR で land 推奨 (両方とも boundary を扱う)。post-merge-feedback Tier 2 #2 採用。 +> +> **参照**: PR #115 `MAX_SAFE_WAIT_SECS` 定数 (config.rs)、`.claude/feedback-reports/115.md` Tier 2 #2 +> +> **実行優先度**: 🔧 **Tier 2** — Effort S。順位 76 と統合 PR で land。 + +#### 設計決定 (案) + +- **テストの形**: `MAX_SAFE_WAIT_SECS` の境界値 matrix を `config.rs` test module に追加 + - `now_2026 = 1_800_000_000_i64` (~ 2026 年) baseline + - `now_2100 = 4_100_000_000_i64` (~ 2100 年) baseline (future-proof check) + - 各 baseline で `now + MAX_SAFE_WAIT_SECS` / `now + MAX_SAFE_WAIT_SECS + 1` の i64 overflow safety を `checked_add` で assert +- **既存 `review_recheck_sanitize_max_safe_boundary` テストを拡張**: 現実装は `now_unix_2026` 単体だが、`now_2100` も追加して未来の Unix timestamp でも safe であることを machine-enforce +- **コメント補強**: `MAX_SAFE_WAIT_SECS` 定数の doc comment に「now_2100 + 1 年 < i64::MAX で safety margin あり」を明記 + +#### 作業計画 + +- [ ] `config.rs` test module の `review_recheck_sanitize_max_safe_boundary` を拡張 (now_2026 + now_2100 の 2 baseline) +- [ ] `MAX_SAFE_WAIT_SECS` doc comment に future-proof の根拠 (year 2100 でも safe) を追記 +- [ ] cargo test 全 pass を確認 +- [ ] 順位 76 と同 PR で land +- [ ] 本 todo5.md エントリを削除 + +#### 完了基準 + +- `now_2100 + MAX_SAFE_WAIT_SECS` が `checked_add` で `Some(_)` を返すこと (= overflow しない) を assert +- `MAX_SAFE_WAIT_SECS` 定数 doc に future-proof 根拠が記述される + +#### 詰まっている箇所 + +なし (Effort S、test 拡張 + doc コメント追加のみ) + +--- + +### ADR-038 (Rust timestamp arithmetic safety) + CLAUDE.md security 拡充 (PR #115 T3-1 採用) ★ Bb-3 follow-up + +> **動機**: PR #115 で「config が user-editable system boundary のとき、sanitize() で値域検証 + 下流 arithmetic で安全範囲保証」というパターンが実証された (CR Major #1 + #2 が両方とも同型の「config 値→arithmetic 入力」cross-layer integrity 問題)。同型の bug class は今後も Rust + config 駆動の component で発生しうるため、組織的 learning として codify。 +> +> **本タスクの位置づけ**: 順位 76 / 77 (test 層) の補完層 = ドキュメント / ADR 層。3 つを別 PR で land すると依存関係が読みやすい (test 層先 → 後で ADR が test を参照)。post-merge-feedback Tier 3 #1 採用。 +> +> **参照**: PR #115 CR Major #1+#2 解消経緯、`.claude/feedback-reports/115.md` Tier 3 #1、CLAUDE.md `security.md` (input validation)、ADR-022 (責務分離原則) の延長 +> +> **実行優先度**: 💎 **Tier 3** — Effort S。順位 76 / 77 が land した後の codification PR。 + +#### 設計決定 (案) + +- **ADR-038 (新規)**: `docs/adr/adr-038-timestamp-arithmetic-safety.md` を作成 + - **タイトル**: Rust timestamp arithmetic の overflow safety pattern + - **Context**: PR #115 で sanitize() が `i64::MAX as u64` を valid として通したが downstream の `now_unix + wait as i64` で overflow した CR Major #2 を引用 + - **Decision**: 以下 3 層で overflow を構造的に防ぐ + 1. **Sanitize layer**: config に `MAX_SAFE_WAIT_SECS` 等の上限を設定し、`sanitize()` で値域違反を default fallback + 2. **Arithmetic layer**: `now_unix + wait as i64` のような cast point に `// SAFETY: が 以下を保証` コメント (人間レビュー時の手がかり) + 3. **Test layer**: `now + sanitize 後の値 < i64::MAX` invariant を `checked_add` で machine-enforce (順位 76/77 で実装) + - **Consequences**: cross-module overflow を test layer で構造的に検知。`MAX_SAFE_WAIT_SECS` の根拠が future-proof (2100 年でも safe) +- **CLAUDE.md `security.md` (`~/.claude/rules/common/security.md`) 拡充**: 「config は user-editable system boundary、必ず sanitize() で値域検証」+ 「Rust の `as` cast は overflow check しない、`checked_add` を併用」を追加。global rule なので全 Rust project に適用される +- **本 PR の効果**: ADR + CLAUDE.md で codified 後、将来同型 bug が発生したら「ADR-038 違反」として一発で指摘可能 + +#### 作業計画 + +- [ ] `docs/adr/adr-038-timestamp-arithmetic-safety.md` を新規作成 (Context / Decision / Consequences) +- [ ] CLAUDE.md (project) Architecture Decisions リストに ADR-038 を追加 +- [ ] `~/.claude/rules/common/security.md` に「config sanitize + Rust arithmetic safety」セクション追加 +- [ ] (任意) `~/.claude/rules/rust/coding-style.md` に `// SAFETY:` コメント pattern を補足 +- [ ] 順位 76/77 が land 済の前提で「Test layer で検証する」を ADR で言及 (前後関係を明示) +- [ ] 派生プロジェクト deploy には影響なし (docs / global rule のみ) +- [ ] 本 todo5.md エントリを削除 + +#### 完了基準 + +- ADR-038 が land し、CLAUDE.md からリンクされる +- `~/.claude/rules/common/security.md` に Rust arithmetic safety pattern が追加される +- 将来「config 値が arithmetic で overflow」という形の bug が出たら、ADR-038 を引用して一発で指摘できる + +#### 詰まっている箇所 + +- 順位 76/77 land 前後の順番: ADR で test layer に言及するため、test 実装が先のほうが自然。ただし ADR を先 land して「test を ADR-038 に従って実装する」流れも可能。実装時に ROI で判断 (test PR と ADR PR を分けるか、まとめるか) +- `~/.claude/` 配下の global rule 編集は本 repo 外への影響あり、慎重に (memory `feedback_no_unenforced_rules.md` 「強制力のないルール追加は却下」原則を踏まえる必要あり = 機械検知できないルールは却下されうる)。本 task は ADR + 既存 rule 拡充で「機械検知の根拠」を提供する形なので OK だが、CLAUDE.md security.md の追記内容が「ルールだけ増やす」と評価されないよう、順位 76/77 の test との連携を明示する +