Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion docs/coderabbit-monitoring-efficiency.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 評価あり (`<review_comment_addressed>`)。
- **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 つ目の適用例) へ転換。

---

Expand Down
4 changes: 3 additions & 1 deletion docs/todo.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 で実測必要) |
Expand All @@ -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: <sanitize-fn> により上限保証` コメントを要求するパターンを 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 への直接効果は小さい。

Expand Down
125 changes: 125 additions & 0 deletions docs/todo5.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: <sanitize-fn> が <const> 以下を保証` コメント (人間レビュー時の手がかり)
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 との連携を明示する