diff --git a/docs/coderabbit-monitoring-efficiency.md b/docs/coderabbit-monitoring-efficiency.md index 039f90c..7bb16ea 100644 --- a/docs/coderabbit-monitoring-efficiency.md +++ b/docs/coderabbit-monitoring-efficiency.md @@ -42,28 +42,11 @@ ADR-018 が廃止したのは「同一プロセス内で 4 段間接連携 (daem ### 進捗 -- ✅ **Bb-1 (順位 53) — マージ済 (PR #113、2026-05-05)**: `cli-pr-monitor` の rate-limit retry を CronCreate park モデルに切り替え (詳細 PR description 参照)。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 / 未マージ、2026-05-05)**: review 完了待ちを CronCreate park モデルに展開し、polling 完全排除 + 二重 polling (45s gh API + 5s observer) を撤廃。T2-2 の sibling parity 回帰テストを同 PR で bundled。 - - `PrMonitorState` に `review_recheck_count` を追加、wakeup 跨ぎで `build_state_for_iteration` が値を保持 - - `wakeup_reason` 値に `"review_recheck"` を追加 (Bb-1 の `"rate_limit_retry"` と sibling) - - `run_poll_loop(is_wakeup: bool)` を single-iteration model に書き換え: - - `is_wakeup=false` (fresh push): checker 未呼び出し → `finalize_initial_review_park` で `INITIAL_REVIEW_WAIT_SECS=300s` 後の wakeup 予約 + park signal emit + early return (todo5.md spec 準拠、CR review 開始前の wasteful API call 回避) - - `is_wakeup=true` (CronCreate wakeup): `run_one_iteration` で 1 回 check → terminal / rate-limit park (Bb-1) / `finalize_review_recheck_park` (Bb-2) のいずれか - - `finalize_review_recheck_park` / `schedule_next_review_recheck_park`: `review_recheck_count` をインクリメント、`MAX_REVIEW_RECHECKS=3` 到達なら `action_required` で抜ける (review が想定時間内に未完了通知) - - `format_review_park_signal`: `[PR_MONITOR_PARK]` envelope に `reason: review_recheck` discriminator 付き、Bb-1 と同一 format で Claude Code 側パーサが両 signal を統一処理可能 - - 既存 `format_park_signal` (Bb-1) にも `reason: rate_limit_retry` 追加で uniformity - - `run_monitor_only` に `detect_wakeup_resume`: 既存 state の `pr` / `repo` 一致 + `next_wakeup_at_unix <= now` で wakeup 判定、該当時は state.started_at を push_time として継続 + `start_monitoring_wakeup` で reset skip - - `state_file_path()` に env 変数 `PR_MONITOR_STATE_FILE_OVERRIDE` 経路追加 (T2-2 fault injection 用、本番コードは env 設定なしで挙動変化なし) - - `observe.rs` 削除 + `pnpm observe-pr` script 削除 + `--observe` 引数削除 (single-iteration 化で polling 自体が消えたため observer も不要、ADR-018 addendum を実質 supersede) - - `MonitorConfig::poll_interval_secs` を `#[allow(dead_code)]` で marker (Bb-3 で削除予定、後方互換のため保持) - - `print_report` を `parked_review_recheck` verdict 対応に拡張 - - 副次的 refactor (touch-trigger ratchet 適用): `start_monitoring_inner` を `try_acquire_monitor_lock` + `init_or_resume_state` + `run_takt_stage` (+ `invoke_takt_into_outcome`) + `finalize_repush` に分割、`format_review_park_signal` を `collect_review_park_fields` + format に分割、`finalize_review_recheck_park` を `finalize_review_recheck_max_reached` + `schedule_next_review_recheck_park` に分割 - - **T2-2 (順位 75 follow-up)** ★: 3 件の write_state 失敗時 fail-safe 回帰テスト追加 - - `finalize_parked_returns_action_required_when_write_state_fails`: rate-limit park の fail-safe 確認 - - `schedule_next_review_recheck_park_returns_action_required_when_write_state_fails`: review park の fail-safe 確認 (sibling parity) - - `finalize_park_siblings_have_symmetric_write_state_handling`: 両 sibling とも write 失敗で `action_required` に収束する invariant を 1 テストで machine-enforce (Bb-3 で新 `finalize_*` 追加時に invariant 違反を test 失敗で検出) - - 全 123 テスト pass、clippy clean、release ビルド済 (`.claude/cli-pr-monitor.exe` 配置済) -- ⏳ **Bb-3 (順位 55)** — 未着手 (Bb-2 land 後、`monitor.toml` 設定外出し + SessionStart catch-up) +- ✅ **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` を追加)。 + - **設計判断: 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 に使用)。 --- diff --git a/docs/todo.md b/docs/todo.md index 75c83fc..29b6fb2 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -55,9 +55,7 @@ | 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、利用頻度低く優先度は低) | -| 53 | 🚀 Tier 1 | **rate-limit retry の CronCreate 化 (Bundle b PR-1) ★ Bundle b** | todo5.md | M | なし (PR #104 で 47 min rate-limit 検出 → 現状 `max_duration_secs=600s` cap でバウンス → `action_required` 通知 = auto-retry 不能。CronCreate で reset_at+60s に wakeup 仕掛けることで長時間待機を可能にする致命点解消) | -| 54 | 🔧 Tier 2 | **review 完了待ちの CronCreate 化 + observer 廃止 (Bundle b PR-2) ★ Bundle b** | todo5.md | M | 順位 53 land 後 (Bb-1 で導入する Cron 機構を review 完了待ちにも展開、45s polling + 5s observer polling を完全排除、固定値 wakeup 化) | -| 55 | 💎 Tier 3 | **config 拡張 + SessionStart catch-up (Bundle b PR-3) ★ Bundle b** | todo5.md | S | 順位 53 / 54 land 後 (固定値の `monitor.toml` 化 + Claude Code 不在時に発火した wakeup を SessionStart で catch-up、AI 不在時の silent loss 防止) | +| 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 で実測必要) | @@ -69,7 +67,6 @@ | 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 不要化) | -| 75 | 🔧 Tier 2 | **`finalize_parked` write_state 失敗時 fail-safe の回帰テスト追加 (PR #113 T2-2 採用) ★ Bb-1 follow-up** | todo5.md | S | なし (PR #113 で `finalize_posted_retrigger` との error path 対称性を後付け修正、本 task は test 固定化。durable な re-park ループ regression を防止、`finalize_*` sibling parity の machine-enforceable 保護) | **戦略**: Tier 1 を 2〜3 セッションで片付け → Tier 2 で ADR-032 の前提 + rate-limit + convergence cost 削減を進める → Tier 3 で ADR-032 を land + ドキュメント整備。Tier 4-5 は cleanup / 外部展開で daily efficiency への直接効果は小さい。 @@ -101,6 +98,8 @@ **PR #113 (Bb-1 = Bundle b PR-1) post-merge-feedback (2026-05-05)**: 9 findings に対して **1 件のみ採用** (順位 75 = T2-2 の `finalize_parked` write_state 失敗時 fail-safe 回帰テスト)。T1 #1/#2 (lint rule 案) は NLP 必要 / FP リスクで却下、T2-1 (Windows path test) / T2-3 (state cycle integration) / T2-4 (CronCreate format lint) は ROI 不見合いで不採用、**T3-1 / T3-2 (`~/.claude/rules/common/coding-style.md` への ルール追記)** は **ユーザー判断で却下** — 「強制力のないルール追加は却下: 機械検知できなければ何もしない方がマシ。ルール乱立は重要ルール埋没の害悪」(memory: feedback_no_unenforced_rules.md として codify 済)、T3-3 (PARK signal 設計 ADR) は premature で 🤔 様子見保留。**本 PR 含意**: Bb-1 の sibling parity invariant (`finalize_*` 群の error path 対称性) は Bb-2 / Bb-3 で同種関数を追加する際に再発確度が高いため、**test レベルで machine-enforceable に保護** することを Bb-2 着手前の前提条件とする。 +**PR #114 (Bb-2 + 順位 75 = Bundle b PR-2 + T2-2) post-merge-feedback (2026-05-05)** ✅ 完了: 9 findings に対して **2 件採用 / 5 件様子見 / 3 件却下**。**Bb-3 (順位 55) で fold-in する採用提案**: T2-2 (Parity test coverage 拡張 = `finalize_park_siblings_have_symmetric_write_state_handling` テストに `finalize_initial_review_park` を追加、self-violation 解消、Effort S)。T2-1 (Legacy JSON deserialize test) は **PR #114 で既に実装済** (`state_legacy_json_without_new_fields_deserializes_with_defaults`、Bb-3 以降の新フィールド追加時に同 pattern を継続するための reference として保存)。**様子見 (5 件)**: T1-1 (finalize_* parity lint、Effort M + NLP 必要、簡易プロキシで再評価) / T1-2 (polling block lint、FP リスクで dogfood 後判断) / T2-3 (env override コメント強化、XS Low) / T2-4 (CI parallel race 確認、preventive only) / T3-1 (Wakeup Resume Invariant ADR) / T3-2 (DI 戦略 ADR、Bb-3 着手時に再検討)。**却下 (3 件)**: T1-3 (Serde schema lint、ROI 低、T2-1 で代替) / T3-3 (parity invariant の global rules 追加) / T3-4 (test-only env var prefix rule) — 後 2 件は **memory: feedback_no_unenforced_rules.md** を直接引用してアナライザが正しく即却下判定。Bb-2 land 時点で Bundle b の核 (CronCreate park モデル) 完成、残る Bb-3 は config 整理 + SessionStart catch-up + T2-2 follow-up を bundled。 + --- ## 現在進行中 diff --git a/docs/todo5.md b/docs/todo5.md index ef40651..e45ba49 100644 --- a/docs/todo5.md +++ b/docs/todo5.md @@ -173,152 +173,6 @@ --- -### rate-limit retry の CronCreate 化 (Bundle b PR-1) ★ Bundle b - -> **動機**: PR #104 で CodeRabbit の 47 分 rate-limit を実観測したが、現状の `cli-pr-monitor` は同一プロセス内で `std::thread::sleep` する設計のため `max_duration_secs=600s` (10 分) を超える待機ができず、長時間 rate-limit ではバウンスして `action_required` 通知 → ユーザー手動介入が必要になる。これは「Code Rabbit が rate-limit にかかった場合、解除後に自動的に `@coderabbitai review` を投稿する」という user vision の致命的乖離点。 -> -> **本タスクの位置づけ**: Bundle b (CR operation 安定化) の最優先 PR。CronCreate 機構を初導入し、47 分 rate-limit を含む長時間待機を auto-retry 経路に乗せる。Bb-2 (review 完了待ちの CronCreate 化) / Bb-3 (config 拡張) は本 PR で導入する Cron 機構の上に積む。 -> -> **参照**: 本セッションでの設計議論 (advisor 経由)、`src/cli-pr-monitor/src/stages/poll.rs:288` `handle_rate_limit_retry`、`docs/adr/adr-018-pr-monitor-takt-migration.md`、`docs/adr/adr-019-coderabbit-review-hybrid-policy.md`、PR #104 (`feat/comment-lint-changed-lines-scope` で 47 min rate-limit 観測) -> -> **実行優先度**: 🚀 **Tier 1** — Effort M。Phase 3 dogfood 完了後着手推奨 (パイプライン改善が落ち着いたタイミングで Cron 機構を導入)。 - -#### 設計決定 (案) - -- **CronCreate 機構**: Claude Code の Cron 機能で reset_at + 60s に one-shot wakeup を仕掛ける。session 非起動時は発火せず (= SessionStart catch-up は Bb-3 で対応) -- **state 拡張**: `PrMonitorState` に `next_wakeup_at: Option>` / `wakeup_reason: Option` を追加 -- **handle_rate_limit_retry の改修**: - - 現状: `std::thread::sleep(Duration::from_secs(sleep_secs))` で同プロセス内待機 - - 新: `state.next_wakeup_at = reset_at + 60s` を保存 → CronCreate 仕掛け → 即 exit -- **wakeup 発火時の処理**: 1 回だけ gh API 確認 → rate-limit 残存なら `@coderabbitai review` post → state 更新 → CronCreate 再仕掛け (recheck) -- **`max_duration_secs > sleep_secs` 制約の撤廃**: 同プロセス常駐ではなくなるため、長時間待機の budget cap が不要に -- **既存 `max_rate_limit_retries` cap は維持**: 無限ループ防止 - -#### 作業計画 - -- [ ] CronCreate API の Rust からの呼び出し方法を調査 (Claude Code の Cron 機構が外部 CLI から呼べるか / hook 経由か) -- [ ] `PrMonitorState` に `next_wakeup_at` / `wakeup_reason` フィールド追加 + serde test -- [ ] `handle_rate_limit_retry` を `state.next_wakeup_at` 保存 + CronCreate 仕掛けに置換 (sleep 削除) -- [ ] wakeup 発火時の entry point 実装 (新 stage `wakeup` or 既存 `monitor` の再 entry) -- [ ] `max_duration_secs > sleep_secs` 制約と関連 `Err` 経路を削除 -- [ ] 単体テスト: rate-limit 検出 → state.next_wakeup_at 設定 → exit が正しく行われる -- [ ] 単体テスト: wakeup 経由再起動 → @coderabbitai review post → 次回 wakeup 仕掛け -- [ ] integration test: 47 min 待機シナリオ (test 中はモック時刻で短縮) -- [ ] dogfood 1-2 PR で実 rate-limit シナリオの auto-retry を確認 -- [ ] 派生プロジェクト (techbook-ledger / auto-review-fix-vc) deploy 確認 -- [ ] 本 todo5.md エントリを削除 - -#### 完了基準 - -- 47 min rate-limit でも auto-retry が動作する (バウンスしない) -- session 起動中は CronCreate で wakeup → @coderabbitai review post → review 再開 -- session 非起動時は wakeup スキップ (Bb-3 の SessionStart catch-up に依存) -- 既存 `max_rate_limit_retries` cap が引き続き機能 - -#### 詰まっている箇所 - -- CronCreate を Rust 外部プロセスから呼び出せるか未確認 (hook 経由 / claude CLI 経由 / 専用 IPC)。調査結果次第で設計を組み直す可能性あり (例: Cron 仕掛けを Rust ではなく hook script で行う) -- session 非起動中の wakeup 振る舞いは Bb-3 (SessionStart catch-up) に委ねるが、移行期 (Bb-1 land 後 Bb-3 land 前) は AI 不在時の rate-limit retry が止まる過渡期になる。Bb-3 を近接 land する想定 - ---- - -### review 完了待ちの CronCreate 化 + observer 廃止 (Bundle b PR-2) ★ Bundle b - -> **動機**: 現状 `cli-pr-monitor` は 45s 間隔で gh API を polling し CR review 完了を待つ + observer (BG) は state file を 5s 間隔で polling する二重 polling 設計。Claude Code が「常時稼働で polling」する負担を強いている。user vision は「経験則時刻 / GitHub UI 待機時刻に通知」 = polling 完全排除。 -> -> **本タスクの位置づけ**: Bundle b PR-2。Bb-1 で導入した CronCreate 機構を review 完了待ちにも展開し、45s polling + 5s observer polling を排除。固定値 wakeup (push+5min, recheck+5min, cap=3) で代替。 -> -> **参照**: `src/cli-pr-monitor/src/stages/poll.rs:275` (45s polling)、`src/cli-pr-monitor/src/stages/observe.rs:26` (5s polling)、本セッション設計議論 -> -> **実行優先度**: 🔧 **Tier 2** — Effort M。順位 53 (Bb-1) land 後着手。 - -#### 設計決定 (案) - -- **review 完了待ちフロー** (新): - - push 直後: cli-pr-monitor が state init (`next_wakeup_at = now + initial_review_wait_secs`, `wakeup_reason = "review_check"`) → 即 exit - - wakeup 発火: 1 回だけ gh API 確認 → 分岐 - - findings あり / APPROVE → `state.action = action_required` / `stop_monitoring_*` → AI 介入トリガ → cron 解除 → exit - - review なし (CR 検討中) → recheck カウンタ +1、`next_wakeup_at = now + review_recheck_wait_secs` → CronCreate 再仕掛け → exit - - max_review_rechecks 到達 → `action_required` で「review が想定時間内に完了していない」と通知 -- **observer 廃止**: 5s polling を完全削除。terminal state 通知は wakeup 経路の AI 介入トリガで代替 (Claude Code が wakeup で起動して state を読む) -- **45s polling の poll loop 削除**: `poll.rs` を「1 回 check + state 更新 + exit」に短縮 -- **既存 `max_duration_secs` の意味変更**: 「single invocation の処理 timeout」に縮小 (= gh api timeout 等の安全装置) - -#### 作業計画 - -- [ ] `poll.rs` の poll loop を「single check + state update + exit」に短縮 -- [ ] `observe.rs` を削除 (or stub 化、SessionStart catch-up は Bb-3 で代替) -- [ ] CronCreate 経由の wakeup entry point を Bb-1 と統一 -- [ ] config に `initial_review_wait_secs` / `review_recheck_wait_secs` / `max_review_rechecks` を追加 -- [ ] 単体テスト: push → state init → exit、wakeup → review check → 分岐 -- [ ] integration test: review 完了 / 未完了 / max recheck 到達 の各経路 -- [ ] dogfood 数 PR で polling 排除の挙動確認 (ログから poll loop が呼ばれていないこと) -- [ ] 派生プロジェクト deploy 確認 -- [ ] 本 todo5.md エントリを削除 - -#### 完了基準 - -- `cli-pr-monitor` の poll loop が完全削除 (single check + exit のみ) -- observer の 5s polling が削除 -- review 完了 / 未完了 / max recheck 到達 の各経路で意図通りの state 遷移 -- 派生プロジェクトでも polling 不在を確認 - -#### 詰まっている箇所 - -- 既存の `cli-pr-monitor` を呼ぶ caller (push-runner / pnpm create-pr) との互換性維持。caller は「monitor が完了するまで待つ」前提で呼んでいる可能性 → Bb-2 では monitor が即 exit するため caller 側の挙動再確認が必要 -- session 非起動時に wakeup が発火しないケース (= AI が長時間離席した場合の review check が止まる) は Bb-3 の SessionStart catch-up で対応 - ---- - -### config 拡張 + SessionStart catch-up (Bundle b PR-3) ★ Bundle b - -> **動機**: Bb-1 / Bb-2 で導入した固定値 (initial_review_wait_secs / review_recheck_wait_secs / rate_limit_buffer_secs / max_review_rechecks 等) は user 要望で「変更しやすい設計」が求められた。また、Claude Code session 非起動中に発火しない wakeup を SessionStart で catch-up しないと、AI 離席中の review monitoring が静かに停止する silent loss が発生する。 -> -> **本タスクの位置づけ**: Bundle b PR-3。Bb-1 / Bb-2 で導入した内部固定値を `monitor.toml` に切り出し + SessionStart hook 拡張で AI 起動時に pending wakeup を catch-up する。 -> -> **参照**: 本セッション user 要望「固定値は後から調整するため、変更しやすい設計にしてください」「次回ユーザー起動時にまとめて処理」、`src/hooks-session-start/`、ADR-030 L2 recovery パターン -> -> **実行優先度**: 💎 **Tier 3** — Effort S。順位 53 / 54 land 後着手。Bb-1 land と Bb-3 land の間は AI 離席中の rate-limit retry が止まる過渡期になるため、近接 land を推奨。 - -#### 設計決定 (案) - -- **monitor.toml の拡張**: - ```toml - [monitor] - initial_review_wait_secs = 300 # push 直後 → 初回 review check - review_recheck_wait_secs = 300 # review なしの再 check 間隔 - rate_limit_buffer_secs = 60 # reset_at に追加する余裕 - max_review_rechecks = 3 # b) の cap - max_rate_limit_retries = 3 # 既存 - ``` -- **SessionStart catch-up**: - - `hooks-session-start` が起動時に `state.next_wakeup_at <= now` を確認 - - pending wakeup あり → `cli-pr-monitor wakeup` を即時 invoke (= 通常の wakeup 経路に乗せる) - - これにより AI 離席で逃した wakeup を起動時に消化 -- **既存設定との互換性**: 既存 `poll_interval_secs` / `max_duration_secs` は Bb-2 で意味変化済 (single invocation timeout)、deprecation 警告を出す or 自然消滅させる - -#### 作業計画 - -- [ ] `monitor.toml` に新 config キーを追加 + parser 実装 + default 値テスト -- [ ] Bb-1 / Bb-2 で内部 const として記述した固定値を config 参照に置換 -- [ ] `hooks-session-start` に `next_wakeup_at <= now` 検出 → `cli-pr-monitor wakeup` invoke ロジック追加 -- [ ] 単体テスト: SessionStart で pending wakeup があれば invoke、なければ no-op -- [ ] integration test: AI 離席シナリオ → 起動時 catch-up -- [ ] dogfood: 実際に Claude Code を一度閉じて再起動した時に pending が処理されること確認 -- [ ] 派生プロジェクト deploy 確認 -- [ ] 本 todo5.md エントリを削除 - -#### 完了基準 - -- 固定値が `monitor.toml` で調整可能 -- SessionStart で pending wakeup を catch-up -- AI 離席時の silent loss が解消 - -#### 詰まっている箇所 - -- 既存 `poll_interval_secs` / `max_duration_secs` の deprecation 戦略 (即削除 vs 段階廃止)。Bb-2 で意味変化しているため即削除でも問題なさそうだが、派生プロジェクト config との後方互換を考慮する必要あり - ---- ### comment-lint hook test 拡充 (PR #104 T2-1+T2-2 bundle) @@ -802,44 +656,3 @@ なし (Effort XS、コメント追記のみ) ---- - -### `finalize_parked` write_state 失敗時 fail-safe の回帰テスト追加 (PR #113 T2-2 採用) ★ Bb-1 follow-up - -> **動機**: PR #113 (Bb-1) で `finalize_parked` の write_state 失敗時に PARK signal を emit し続ける非対称性 (sibling の `finalize_posted_retrigger` は失敗時に `action_required` で抜ける fail-safe を持つ) を pre-push-review が OBS-1 として検出 → CodeRabbit が Major Finding #1 として再検出 → takt fix loop が auto-fix で対称ガードを追加して land。本 task は **その fix を回帰テストで固定化** し、Bb-2 / Bb-3 で同種の `finalize_*` 拡張が予定されている際に対称性崩れの再発を検知できるようにする。 -> -> **本タスクの位置づけ**: PR #113 post-merge-feedback Tier 2 #2 採用 (Severity Medium / Frequency Medium / Effort S / Adoption Risk None / ✅ 採用)。3 ソース (PR diff / Session / Pre-push) で独立指摘された原本バグ。`finalize_posted_retrigger` との error handling 対称性が崩れていた箇所への regression test が現状存在しない。Bb-2 / Bb-3 で類似 `finalize_*` 拡張予定 → Frequency Medium 評価。 -> -> **参照**: `.claude/feedback-reports/113.md` Tier 2 #2、`src/cli-pr-monitor/src/stages/poll.rs` の `finalize_parked` / `finalize_posted_retrigger`、CodeRabbit PR #113 Finding #1、pre-push-review OBS-1 -> -> **実行優先度**: 🔧 **Tier 2** — Effort S。Bb-1 land 済 (PR #113)、Bb-2 着手前に入れて parity invariant を保護する想定。 - -#### 設計決定 (案) - -- **テスト配置**: `src/cli-pr-monitor/src/stages/poll.rs` の `#[cfg(test)] mod tests` (既存 `format_park_signal_*` テスト群の隣) -- **テストシナリオ** (`finalize_parked_returns_action_required_when_write_state_fails`): - - `state_file_path()` が書き込めない条件を構築 (e.g. パス先を read-only にする、または `write_state_to(&Path)` が呼ばれる前提を破壊する経路を mock) - - `finalize_parked(&mut state, &rl, &pr_info, wakeup_at, max_retries, &result)` を呼ぶ - - 戻り値の `PollResult.action == "action_required"` を assert (PARK signal を emit し続けず fail-safe で抜けること) - - state.action / next_wakeup_at_unix / wakeup_reason が parked_rate_limit ではなく action_required 経路の値になっていること -- **副次テスト** (`finalize_posted_retrigger_and_finalize_parked_have_symmetric_write_state_handling`): - - 両関数とも write_state 失敗で `action_required` を返すことを assert (parity invariant の machine-enforceable 保護) - - これにより Bb-2 / Bb-3 で新 `finalize_*` 関数を追加する際、同 invariant に従わないと test が落ちて気づける - -#### 作業計画 - -- [ ] `state_file_path()` 書き込み失敗を強制する mock または fault-injection 機構を調査 (現状 `pub(crate) fn state_file_path() -> PathBuf` を直接差し替える経路がない可能性 → trait or env var で injectable にする小 refactor が必要かも) -- [ ] `finalize_parked_returns_action_required_when_write_state_fails` を追加 -- [ ] `finalize_posted_retrigger_and_finalize_parked_have_symmetric_write_state_handling` で parity invariant を assert -- [ ] cargo clippy / cargo test で 127+ tests pass -- [ ] 本 todo5.md エントリを削除 - -#### 完了基準 - -- `finalize_parked` の write_state 失敗時 fail-safe が回帰テストで保護される -- `finalize_*` sibling parity が test レベルで invariant 化される (新 finalize_* 追加時に test が落ちて気づける) -- 全テスト pass、clippy clean - -#### 詰まっている箇所 - -- `state_file_path()` の write 失敗を test から強制する経路が現状不明。candidates: (a) `write_state_to(&Path, &state)` を直接呼んで read-only path を渡す (関数は pub(crate)、tmp dir に read-only を作るのが Windows でやや厄介) / (b) `state_file_path()` を環境変数で override 可能にする小 refactor / (c) `finalize_*` 関数群を `WriteState` trait DI 化する大 refactor (over-engineered) → (a) or (b) を優先。fault-injection は Bb-2 で同種テストを追加する際にも再利用可能 diff --git a/pr-monitor-config.toml b/pr-monitor-config.toml index dd8c0cb..12acd69 100644 --- a/pr-monitor-config.toml +++ b/pr-monitor-config.toml @@ -6,11 +6,18 @@ [monitor] enabled = true -poll_interval_secs = 120 max_duration_secs = 600 check_ci = true check_coderabbit = true +# review 完了待ち park 制御 (Bb-3 順位 55)。 +# CodeRabbit walkthrough 確認後、review 完了をポーリングする CronCreate 経路の +# 待機秒数と最大再チェック回数。コメントアウトで全てデフォルト (5 分 / 5 分 / 3 回) を使用。 +# [review_recheck] +# initial_review_wait_secs = 300 +# review_recheck_wait_secs = 300 +# max_review_rechecks = 3 + [takt] workflow = "post-pr-review" # task は workflow 名を prefix として含める (ADR-030 §task labeling convention)。 diff --git a/src/cli-pr-monitor/src/config.rs b/src/cli-pr-monitor/src/config.rs index 5b9c0bf..bbab8d2 100644 --- a/src/cli-pr-monitor/src/config.rs +++ b/src/cli-pr-monitor/src/config.rs @@ -3,11 +3,6 @@ use std::path::{Path, PathBuf}; use crate::log::log_info; -/// poll_interval_secs のデフォルト。 -/// 120s から 180s に延長 (PR #88 T2-4): rate-limit 浪費を抑えるため、 -/// セッション単独でも polling 回数を 5 → ~3 サイクル/監視に削減。 -/// max_duration_secs (600s) の維持と組み合わせて、polling 総数を 40% 削減する。 -pub(crate) const DEFAULT_POLL_INTERVAL: u64 = 180; pub(crate) const DEFAULT_MAX_DURATION: u64 = 600; pub(crate) const DEFAULT_STEP_TIMEOUT_SECS: u64 = 300; pub(crate) const DEFAULT_CHECK_TIMEOUT_SECS: u64 = 60; @@ -21,18 +16,14 @@ pub(crate) struct Config { pub(crate) fix: FixConfig, #[serde(default)] pub(crate) rate_limit: RateLimitConfig, + #[serde(default)] + pub(crate) review_recheck: ReviewRecheckConfig, } #[derive(Deserialize, Clone)] pub(crate) struct MonitorConfig { #[serde(default = "default_enabled")] pub(crate) enabled: bool, - /// Bb-2 で polling loop を廃止 (single-iteration + CronCreate park モデル) したため - /// 本フィールドは現状未使用。既存 `pr-monitor-config.toml` との後方互換のため保持。 - /// Bb-3 (config 整理) で削除予定。 - #[serde(default = "default_poll_interval")] - #[allow(dead_code)] - pub(crate) poll_interval_secs: u64, #[serde(default = "default_max_duration")] pub(crate) max_duration_secs: u64, #[serde(default = "default_check_ci")] @@ -44,9 +35,6 @@ pub(crate) struct MonitorConfig { fn default_enabled() -> bool { true } -fn default_poll_interval() -> u64 { - DEFAULT_POLL_INTERVAL -} fn default_max_duration() -> u64 { DEFAULT_MAX_DURATION } @@ -61,7 +49,6 @@ impl Default for MonitorConfig { fn default() -> Self { Self { enabled: default_enabled(), - poll_interval_secs: default_poll_interval(), max_duration_secs: default_max_duration(), check_ci: default_check_ci(), check_coderabbit: default_check_coderabbit(), @@ -132,6 +119,80 @@ impl Default for RateLimitConfig { } } +/// review 完了待ち park 制御 (Bb-3 順位 55) +/// +/// CodeRabbit walkthrough 確認後、review 完了をポーリングする CronCreate 経路の +/// 待機秒数と最大再チェック回数を制御する。 +/// 旧 hard-coded const (poll.rs INITIAL_REVIEW_WAIT_SECS / REVIEW_RECHECK_WAIT_SECS / +/// MAX_REVIEW_RECHECKS) を config 化したもの。 +#[derive(Deserialize, Clone)] +pub(crate) struct ReviewRecheckConfig { + /// fresh push 経路 (initial park) の wait 秒数 + #[serde(default = "default_initial_review_wait_secs")] + pub(crate) initial_review_wait_secs: u64, + /// wakeup 経路 (continue_monitoring) で次回 wakeup までの wait 秒数 + #[serde(default = "default_review_recheck_wait_secs")] + pub(crate) review_recheck_wait_secs: u64, + /// recheck 上限。到達後は action_required で抜ける + #[serde(default = "default_max_review_rechecks")] + pub(crate) max_review_rechecks: u32, +} + +fn default_initial_review_wait_secs() -> u64 { + 300 +} +fn default_review_recheck_wait_secs() -> u64 { + 300 +} +fn default_max_review_rechecks() -> u32 { + 3 +} + +impl Default for ReviewRecheckConfig { + fn default() -> Self { + Self { + initial_review_wait_secs: default_initial_review_wait_secs(), + review_recheck_wait_secs: default_review_recheck_wait_secs(), + max_review_rechecks: default_max_review_rechecks(), + } + } +} + +/// `wait_secs` の実用的な上限 (1 年 = 31,536,000 秒)。 +/// +/// PR #115 CR Major #2 採用: poll.rs が `now_unix + wait_secs as i64` を計算するため、 +/// `wait_secs` を `i64::MAX` ぎりぎりまで許容すると `now_unix (~1.78e9 in 2026)` との +/// 加算で確実に算術 overflow し、release build では負の wakeup_at にラップする。 +/// 1 年 = 3.15e7 << i64::MAX = 9.22e18 で `now_unix + 1年` は overflow しない。 +/// CronCreate の auto-expire は 7 日のため、1 年は user 編集の上限として十分な余裕を持つ。 +const MAX_SAFE_WAIT_SECS: u64 = 365 * 24 * 60 * 60; + +impl ReviewRecheckConfig { + /// 異常値 (0 / 実用域超過) をデフォルト値にフォールバックする。 + /// + /// PR #115 CR Major #1 / #2 採用: 防御的 input validation。 + /// poll.rs が `wait_secs as i64` を `now_unix + wait` に加算するため、`wait_secs == 0` + /// は wakeup を即時化、`max_review_rechecks == 0` は recheck を瞬時に max 到達させる、 + /// `wait_secs > MAX_SAFE_WAIT_SECS` (1 年) は `now_unix + wait` の i64 加算で overflow して + /// wakeup_at が破損する。これらを `load_config` 経路で defensively 修正する + /// (config が user 編集可能な system boundary のため、CLAUDE.md + /// "ALWAYS validate at system boundaries" 原則に従う)。 + fn sanitize(mut self) -> Self { + if self.initial_review_wait_secs == 0 || self.initial_review_wait_secs > MAX_SAFE_WAIT_SECS + { + self.initial_review_wait_secs = default_initial_review_wait_secs(); + } + if self.review_recheck_wait_secs == 0 || self.review_recheck_wait_secs > MAX_SAFE_WAIT_SECS + { + self.review_recheck_wait_secs = default_review_recheck_wait_secs(); + } + if self.max_review_rechecks == 0 { + self.max_review_rechecks = default_max_review_rechecks(); + } + self + } +} + fn config_path() -> PathBuf { let filename = "pr-monitor-config.toml"; @@ -174,8 +235,11 @@ pub(crate) fn load_config() -> Config { return Config::default(); } }; - match toml::from_str(&content) { - Ok(config) => config, + match toml::from_str::(&content) { + Ok(mut config) => { + config.review_recheck = config.review_recheck.sanitize(); + config + } Err(e) => { log_info(&format!( "pr-monitor-config.toml パースエラー (デフォルト使用): {}", @@ -195,7 +259,6 @@ mod tests { let toml_str = r#" [monitor] enabled = true -poll_interval_secs = 45 max_duration_secs = 900 check_ci = true check_coderabbit = false @@ -207,7 +270,6 @@ extra_args = ["--pipeline", "--skip-git"] "#; let config: Config = toml::from_str(toml_str).unwrap(); assert!(config.monitor.enabled); - assert_eq!(config.monitor.poll_interval_secs, 45); assert_eq!(config.monitor.max_duration_secs, 900); assert!(config.monitor.check_ci); assert!(!config.monitor.check_coderabbit); @@ -218,6 +280,22 @@ extra_args = ["--pipeline", "--skip-git"] assert_eq!(takt.extra_args.as_ref().unwrap().len(), 2); } + /// Bb-3: 旧 `poll_interval_secs` フィールド (Bb-2 で未使用化、Bb-3 で削除) + /// が残った既存 config を読み込む際に、unknown field でパースエラーにならず + /// 無視されることを確認する後方互換テスト。 + #[test] + fn config_ignores_legacy_poll_interval_secs() { + let toml_str = r#" +[monitor] +enabled = true +poll_interval_secs = 45 +max_duration_secs = 900 +"#; + let config: Config = toml::from_str(toml_str).unwrap(); + assert!(config.monitor.enabled); + assert_eq!(config.monitor.max_duration_secs, 900); + } + #[test] fn config_monitor_only_no_takt() { let toml_str = r#" @@ -233,9 +311,8 @@ enabled = true fn config_defaults_when_empty_monitor() { let toml_str = "[monitor]\n"; let config: Config = toml::from_str(toml_str).unwrap(); - // serde(default) により空の [monitor] でも MonitorConfig::default() と同じ値 assert!(config.monitor.enabled); - assert_eq!(config.monitor.poll_interval_secs, DEFAULT_POLL_INTERVAL); + assert_eq!(config.monitor.max_duration_secs, DEFAULT_MAX_DURATION); } #[test] @@ -305,4 +382,125 @@ max_retries = 5 assert!(!config.rate_limit.auto_retry_enabled); assert_eq!(config.rate_limit.max_retries, 5); } + + #[test] + fn config_review_recheck_defaults() { + let toml_str = "[monitor]\n"; + let config: Config = toml::from_str(toml_str).unwrap(); + assert_eq!(config.review_recheck.initial_review_wait_secs, 300); + assert_eq!(config.review_recheck.review_recheck_wait_secs, 300); + assert_eq!(config.review_recheck.max_review_rechecks, 3); + } + + #[test] + fn config_review_recheck_custom() { + let toml_str = r#" +[monitor] + +[review_recheck] +initial_review_wait_secs = 600 +review_recheck_wait_secs = 900 +max_review_rechecks = 5 +"#; + let config: Config = toml::from_str(toml_str).unwrap(); + assert_eq!(config.review_recheck.initial_review_wait_secs, 600); + assert_eq!(config.review_recheck.review_recheck_wait_secs, 900); + assert_eq!(config.review_recheck.max_review_rechecks, 5); + } + + /// PR #115 CR Major #1: `max_review_rechecks=0` は recheck を瞬時に max 到達させ + /// 機能を無効化するため、デフォルト値にフォールバックする。 + #[test] + fn review_recheck_sanitize_replaces_zero_max_review_rechecks() { + let cfg = ReviewRecheckConfig { + initial_review_wait_secs: 100, + review_recheck_wait_secs: 200, + max_review_rechecks: 0, + } + .sanitize(); + assert_eq!( + cfg.max_review_rechecks, 3, + "0 はデフォルト 3 にフォールバック" + ); + assert_eq!(cfg.initial_review_wait_secs, 100, "他フィールドは不変"); + assert_eq!(cfg.review_recheck_wait_secs, 200, "他フィールドは不変"); + } + + /// PR #115 CR Major #1: `wait_secs=0` は wakeup を即時化しスケジューリング意図を失うため、 + /// デフォルト値にフォールバックする。 + #[test] + fn review_recheck_sanitize_replaces_zero_wait_secs() { + let cfg = ReviewRecheckConfig { + initial_review_wait_secs: 0, + review_recheck_wait_secs: 0, + max_review_rechecks: 5, + } + .sanitize(); + assert_eq!(cfg.initial_review_wait_secs, 300); + assert_eq!(cfg.review_recheck_wait_secs, 300); + assert_eq!(cfg.max_review_rechecks, 5, "他フィールドは不変"); + } + + /// PR #115 CR Major #2: `wait_secs > MAX_SAFE_WAIT_SECS` (1 年) は poll.rs の + /// `now_unix + wait as i64` 加算で算術 overflow するため、デフォルト値に + /// フォールバックする。`u64::MAX` / `i64::MAX as u64` 等の極端値も対象。 + #[test] + fn review_recheck_sanitize_replaces_unrealistic_wait_secs() { + let cfg = ReviewRecheckConfig { + initial_review_wait_secs: u64::MAX, + review_recheck_wait_secs: i64::MAX as u64, + max_review_rechecks: 3, + } + .sanitize(); + assert_eq!(cfg.initial_review_wait_secs, 300); + assert_eq!(cfg.review_recheck_wait_secs, 300); + } + + #[test] + fn review_recheck_sanitize_keeps_valid_values_unchanged() { + let cfg = ReviewRecheckConfig { + initial_review_wait_secs: 600, + review_recheck_wait_secs: 900, + max_review_rechecks: 5, + } + .sanitize(); + assert_eq!(cfg.initial_review_wait_secs, 600); + assert_eq!(cfg.review_recheck_wait_secs, 900); + assert_eq!(cfg.max_review_rechecks, 5); + } + + /// PR #115 CR Major #2: 1 年 (MAX_SAFE_WAIT_SECS) ぎりぎりは valid、 + /// 1 年 + 1 秒は default に置換される境界値を machine-enforce する。 + /// 加えて、`now_unix + sanitize 後の値 < i64::MAX` invariant が成立することを assert。 + #[test] + fn review_recheck_sanitize_max_safe_boundary() { + let cfg_at_limit = ReviewRecheckConfig { + initial_review_wait_secs: MAX_SAFE_WAIT_SECS, + review_recheck_wait_secs: MAX_SAFE_WAIT_SECS, + max_review_rechecks: 1, + } + .sanitize(); + assert_eq!( + cfg_at_limit.initial_review_wait_secs, MAX_SAFE_WAIT_SECS, + "1 年ジャストは valid" + ); + + let cfg_over_limit = ReviewRecheckConfig { + initial_review_wait_secs: MAX_SAFE_WAIT_SECS + 1, + review_recheck_wait_secs: MAX_SAFE_WAIT_SECS + 1, + max_review_rechecks: 1, + } + .sanitize(); + assert_eq!( + cfg_over_limit.initial_review_wait_secs, 300, + "1 年 + 1 秒は default にフォールバック" + ); + + let now_unix_2026: i64 = 1_800_000_000; + let safe_sum = now_unix_2026.checked_add(cfg_at_limit.initial_review_wait_secs as i64); + assert!( + safe_sum.is_some(), + "sanitize 後の値は now_unix + wait で overflow しない (CR Major #2 invariant)" + ); + } } diff --git a/src/cli-pr-monitor/src/stages/poll.rs b/src/cli-pr-monitor/src/stages/poll.rs index 3c288e6..9514ffc 100644 --- a/src/cli-pr-monitor/src/stages/poll.rs +++ b/src/cli-pr-monitor/src/stages/poll.rs @@ -33,19 +33,17 @@ struct PollContext<'a> { max_duration: u64, skip_ci: bool, skip_coderabbit: bool, + /// fresh push 経路 (initial park) の wait 秒数 (Bb-3 順位 55: config 由来) + initial_review_wait_secs: u64, + /// wakeup 経路で次回 wakeup までの wait 秒数 (Bb-3 順位 55: config 由来) + review_recheck_wait_secs: u64, + /// recheck 上限 (Bb-3 順位 55: config 由来) + max_review_rechecks: u32, } -/// 初回 push 後の review check までの park 期間 (Bb-2)。 -/// CR が review を完了するまでの待機を CronCreate に委譲し polling を完全排除する。 -pub(crate) const INITIAL_REVIEW_WAIT_SECS: u64 = 300; -/// review が未完了だった場合の recheck 間隔 (Bb-2)。 -pub(crate) const REVIEW_RECHECK_WAIT_SECS: u64 = 300; -/// review_recheck の累積上限 (Bb-2)。到達時は action_required で抜ける。 -pub(crate) const MAX_REVIEW_RECHECKS: u32 = 3; - /// single-iteration check + park-or-terminate モデル (Bb-2)。 /// -/// `is_wakeup=false` (fresh push): checker は呼ばず、即 INITIAL_REVIEW_WAIT_SECS 後の +/// `is_wakeup=false` (fresh push): checker は呼ばず、即 `initial_review_wait_secs` 後の /// wakeup を予約して exit する (CR review 開始前の wasteful API call を回避、todo5.md spec)。 /// /// `is_wakeup=true` (CronCreate からの再 invoke): 1 回 checker を呼び、結果に応じて @@ -75,6 +73,9 @@ pub(crate) fn run_poll_loop(full_config: &Config, pr_info: &PrInfo, is_wakeup: b max_duration: config.max_duration_secs, skip_ci: !config.check_ci, skip_coderabbit: !config.check_coderabbit, + initial_review_wait_secs: full_config.review_recheck.initial_review_wait_secs, + review_recheck_wait_secs: full_config.review_recheck.review_recheck_wait_secs, + max_review_rechecks: full_config.review_recheck.max_review_rechecks, }; if !is_wakeup { @@ -549,6 +550,7 @@ struct ReviewParkSignalFields { exe: String, cwd: String, recheck: u32, + max_rechecks: u32, } fn collect_review_park_fields( @@ -588,6 +590,7 @@ fn collect_review_park_fields( exe, cwd, recheck: state.review_recheck_count, + max_rechecks: ctx.max_review_rechecks, } } @@ -626,7 +629,7 @@ CronCreate({{ wakeup_iso = f.wakeup_iso, wait_secs = f.wait_secs, recheck = f.recheck, - max = MAX_REVIEW_RECHECKS, + max = f.max_rechecks, exe = f.exe, cwd = f.cwd, ) @@ -635,7 +638,7 @@ CronCreate({{ /// Bb-2: fresh push 経路で review_recheck park を行う (checker 呼び出しなし)。 /// /// 動機: push 直後は CR がまだ review を開始していない可能性が高く、即 check は wasteful。 -/// INITIAL_REVIEW_WAIT_SECS 後に wakeup を予約 → 1 回 check という 2-step フローに分離する。 +/// `initial_review_wait_secs` 後に wakeup を予約 → 1 回 check という 2-step フローに分離する。 /// /// CR Major #2 fix (Bb-2 PR #114 review): 既存 state に残った `review_recheck_count` を /// fresh push では 0 に明示リセット (前サイクルが MAX 到達等で残った count が新 push に @@ -656,12 +659,12 @@ fn finalize_initial_review_park(ctx: &PollContext<'_>) -> PollResult { .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs() as i64) .unwrap_or(0); - state.next_wakeup_at_unix = Some(now_unix + INITIAL_REVIEW_WAIT_SECS as i64); + state.next_wakeup_at_unix = Some(now_unix + ctx.initial_review_wait_secs as i64); state.wakeup_reason = Some("review_recheck".into()); state.action = "parked_review_recheck".into(); state.summary = format!( "review check を {}s 後に予約 (initial wait, recheck=0/{})", - INITIAL_REVIEW_WAIT_SECS, MAX_REVIEW_RECHECKS + ctx.initial_review_wait_secs, ctx.max_review_rechecks ); if let Err(e) = write_state(&state) { @@ -682,9 +685,9 @@ fn finalize_initial_review_park(ctx: &PollContext<'_>) -> PollResult { /// Bb-2: wakeup 経路の review_recheck park (checker check 後に continue_monitoring の場合)。 /// -/// `review_recheck_count` をインクリメントし、`MAX_REVIEW_RECHECKS` 到達なら +/// `review_recheck_count` をインクリメントし、`max_review_rechecks` 到達なら /// `action_required` で抜ける (review が想定時間内に未完了を通知)。 -/// 未到達なら REVIEW_RECHECK_WAIT_SECS 後の wakeup を予約して return。 +/// 未到達なら `review_recheck_wait_secs` 後の wakeup を予約して return。 fn finalize_review_recheck_park(ctx: &PollContext<'_>) -> PollResult { let mut state = read_state().unwrap_or_else(|| { PrMonitorState::new( @@ -695,17 +698,20 @@ fn finalize_review_recheck_park(ctx: &PollContext<'_>) -> PollResult { }); state.review_recheck_count += 1; - if state.review_recheck_count >= MAX_REVIEW_RECHECKS { - return finalize_review_recheck_max_reached(&mut state); + if state.review_recheck_count >= ctx.max_review_rechecks { + return finalize_review_recheck_max_reached(&mut state, ctx.max_review_rechecks); } schedule_next_review_recheck_park(&mut state, ctx) } -fn finalize_review_recheck_max_reached(state: &mut PrMonitorState) -> PollResult { +fn finalize_review_recheck_max_reached( + state: &mut PrMonitorState, + max_review_rechecks: u32, +) -> PollResult { log_info(&format!( "[review_recheck] max {} 回到達、action_required で抜ける", - MAX_REVIEW_RECHECKS + max_review_rechecks )); let summary = format!( "review が想定時間内に完了せず ({} recheck 後)。手動で PR を確認してください", @@ -725,13 +731,13 @@ fn schedule_next_review_recheck_park( .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs() as i64) .unwrap_or(0); - state.next_wakeup_at_unix = Some(now_unix + REVIEW_RECHECK_WAIT_SECS as i64); + state.next_wakeup_at_unix = Some(now_unix + ctx.review_recheck_wait_secs as i64); state.wakeup_reason = Some("review_recheck".into()); state.action = "parked_review_recheck".into(); state.head_commit = ctx.pr_info.head_commit.clone(); state.summary = format!( "review check を {}s 後に予約 (recheck={}/{})", - REVIEW_RECHECK_WAIT_SECS, state.review_recheck_count, MAX_REVIEW_RECHECKS + ctx.review_recheck_wait_secs, state.review_recheck_count, ctx.max_review_rechecks ); if let Err(e) = write_state(state) { @@ -1142,6 +1148,9 @@ mod tests { max_duration: 600, skip_ci: false, skip_coderabbit: false, + initial_review_wait_secs: 300, + review_recheck_wait_secs: 300, + max_review_rechecks: 3, }; let outcome = schedule_next_review_recheck_park(&mut state, &ctx); @@ -1181,10 +1190,34 @@ mod tests { max_duration: 600, skip_ci: false, skip_coderabbit: false, + initial_review_wait_secs: 300, + review_recheck_wait_secs: 300, + max_review_rechecks: 3, }; schedule_next_review_recheck_park(&mut state, &ctx) } + fn invoke_finalize_initial_review_park_with_bad_path( + pr_info: &crate::util::PrInfo, + ) -> PollResult { + let checker_path = std::path::PathBuf::from("dummy"); + let rate_limit_config = RateLimitConfig::default(); + let ctx = PollContext { + checker: &checker_path, + push_time: "2026-05-01T00:00:00Z", + pr_info, + rate_limit_config: &rate_limit_config, + start: std::time::Instant::now(), + max_duration: 600, + skip_ci: false, + skip_coderabbit: false, + initial_review_wait_secs: 300, + review_recheck_wait_secs: 300, + max_review_rechecks: 3, + }; + finalize_initial_review_park(&ctx) + } + fn seed_stale_recheck_state(tmp_path: &std::path::Path) { let mut stale_state = PrMonitorState::new(Some(42), Some("o/r".into()), "2026-05-01T00:00:00Z".into()); @@ -1193,6 +1226,48 @@ mod tests { crate::state::write_state_to(tmp_path, &stale_state).unwrap(); } + /// Bb-3 (順位 55): `max_review_rechecks` の config 化が実際に PARK signal に + /// 反映されることを machine-enforce する (default 3 ではなく custom 値が出力されること)。 + #[test] + fn format_review_park_signal_uses_configured_max_rechecks() { + let state = + PrMonitorState::new(Some(42), Some("o/r".into()), "2026-05-01T00:00:00Z".into()); + let pr_info = crate::util::PrInfo { + pr_number: Some(42), + repo: Some("o/r".into()), + push_time: Some("2026-05-01T00:00:00Z".into()), + head_commit: None, + }; + let checker = std::path::PathBuf::from("dummy"); + let rate_limit_config = RateLimitConfig::default(); + let ctx = PollContext { + checker: &checker, + push_time: "2026-05-01T00:00:00Z", + pr_info: &pr_info, + rate_limit_config: &rate_limit_config, + start: std::time::Instant::now(), + max_duration: 600, + skip_ci: false, + skip_coderabbit: false, + initial_review_wait_secs: 120, + review_recheck_wait_secs: 240, + max_review_rechecks: 7, + }; + + let signal = format_review_park_signal(&state, &ctx); + + assert!( + signal.contains("max_rechecks: 7"), + "PARK signal に config 値 (max_rechecks: 7) が反映されること: {}", + signal + ); + assert!( + !signal.contains("max_rechecks: 3"), + "default 値 3 が hard-coded で残っていないこと: {}", + signal + ); + } + /// CR Major #2 fix (Bb-2 PR #114 review): fresh push 経路では `finalize_initial_review_park` /// が `review_recheck_count` を 0 に明示リセットすること。前サイクルが MAX 到達 (count=3) /// で残った state を持ち越さないことを machine-enforce する。 @@ -1223,6 +1298,9 @@ mod tests { max_duration: 600, skip_ci: false, skip_coderabbit: false, + initial_review_wait_secs: 300, + review_recheck_wait_secs: 300, + max_review_rechecks: 3, }; let outcome = finalize_initial_review_park(&ctx); @@ -1243,9 +1321,11 @@ mod tests { ); } - /// Bb-2 (T2-2): finalize_parked と schedule_next_review_recheck_park の sibling parity - /// (共に write_state 失敗で action_required を返す) を 1 テストで machine-enforce する。 - /// 新 finalize_* 関数を追加する際、本テストが落ちて invariant 維持を強制する。 + /// Bb-2 (T2-2) + Bb-3 follow-up: 3 つの finalize_* park sibling + /// (`finalize_parked` / `schedule_next_review_recheck_park` / `finalize_initial_review_park`) + /// は全て write_state 失敗で `action_required` を返す invariant を 1 テストで + /// machine-enforce する。新 finalize_* 関数を追加する際、本テストが落ちて + /// invariant 維持を強制する。 #[test] fn finalize_park_siblings_have_symmetric_write_state_handling() { let _guard = env_override_lock(); @@ -1261,16 +1341,29 @@ mod tests { let outcome_rate_limit = invoke_finalize_parked_with_bad_path(&pr_info); let outcome_review = invoke_review_park_with_bad_path(&pr_info); + let outcome_initial = invoke_finalize_initial_review_park_with_bad_path(&pr_info); std::env::remove_var("PR_MONITOR_STATE_FILE_OVERRIDE"); + assert_eq!( + outcome_rate_limit.action, "action_required", + "finalize_parked: write_state 失敗 → action_required" + ); + assert_eq!( + outcome_review.action, "action_required", + "schedule_next_review_recheck_park: write_state 失敗 → action_required" + ); + assert_eq!( + outcome_initial.action, "action_required", + "finalize_initial_review_park: write_state 失敗 → action_required" + ); assert_eq!( outcome_rate_limit.action, outcome_review.action, - "sibling parity: finalize_parked と schedule_next_review_recheck_park は同じ action を返すこと" + "sibling parity (rate_limit ↔ review_recheck)" ); assert_eq!( - outcome_rate_limit.action, "action_required", - "両 sibling とも write_state 失敗 → action_required へ収束する fail-safe" + outcome_review.action, outcome_initial.action, + "sibling parity (review_recheck ↔ initial_review)" ); } } diff --git a/src/cli-pr-monitor/src/state.rs b/src/cli-pr-monitor/src/state.rs index 76cbf63..f52083c 100644 --- a/src/cli-pr-monitor/src/state.rs +++ b/src/cli-pr-monitor/src/state.rs @@ -49,7 +49,7 @@ pub(crate) struct PrMonitorState { /// review 完了待ちの recheck 回数 (Bb-2 で導入)。 /// /// `parked_review_recheck` 経路で wakeup が発火するたびにインクリメントされる。 - /// `MAX_REVIEW_RECHECKS` 到達で `action_required` 経路に抜ける (review が想定時間内に + /// `max_review_rechecks` (config) 到達で `action_required` 経路に抜ける (review が想定時間内に /// 完了していない通知)。新規 push で `PrMonitorState::new` により 0 にリセット、wakeup 経路で /// は build_state_for_iteration が既存値を保持する。 #[serde(default)] diff --git a/src/hooks-session-start/src/main.rs b/src/hooks-session-start/src/main.rs index 77bcad2..605660d 100644 --- a/src/hooks-session-start/src/main.rs +++ b/src/hooks-session-start/src/main.rs @@ -1,21 +1,22 @@ -//! SessionStart hook — セッション ID を環境変数とファイルに伝播する +//! SessionStart hook — セッション ID を環境変数とファイルに伝播する + PR monitor catch-up //! -//! Claude Code の SessionStart イベントで発火し、以下の2つの経路で -//! session_id を伝播する: +//! Claude Code の SessionStart イベントで発火し、以下の3つの経路で session 起動準備を行う: //! //! 1. $CLAUDE_ENV_FILE に export 文を追記 → Bash ツールから参照可能 //! 2. .claude/.session-id ファイルに書き出し → 子プロセス (exe) から参照可能 +//! 3. PR monitor catch-up (Bb-3 順位 55): cli-pr-monitor の state file を読み、 +//! `next_wakeup_at_unix` が現在時刻以前 (= 待機時刻を過ぎている) なら +//! `additionalContext` で Claude に手動再起動を促すメッセージを差し込む。 +//! 別プロセス spawn ではなく Claude に nudge する設計 (handle 継承や stdout +//! 可視性の問題を回避し、PARK signal flow を session 内に保つ)。 //! //! .session-id ファイルは「同一 ID スキップ」方式: //! - 既に同じ session_id が書かれていれば何もしない (冪等) //! - 異なる ID (新セッション or サブセッション) の場合は上書きする -//! -//! 現在 cli-pr-monitor は daemon + state file 方式に移行済みのため -//! .session-id を直接参照しないが、将来の拡張用にこの仕組みは維持する。 use serde::Deserialize; use std::io::Read; -use std::path::Path; +use std::path::{Path, PathBuf}; /// SessionStart hook の stdin JSON (必要なフィールドのみ) #[derive(Deserialize)] @@ -23,8 +24,29 @@ struct HookInput { session_id: Option, } +/// catch-up nudge で案内する手動再開コマンド。 +/// pre-push-review (PR #115) 指摘 [B]: nudge 文字列のうちスクリプト名は const に切り出して +/// rename 時の drift を防ぐ。実際のコマンド実行ロジックは package.json (`scripts.push`) + +/// cli-push-runner にあり、本 const は表示用 hint のみ。 +const RESUME_MONITORING_COMMAND: &str = "pnpm push --monitor-only"; + +/// cli-pr-monitor の state file から catch-up に必要な field のみ部分デシリアライズ。 +/// 完全な PrMonitorState を別 crate から共有しないことで coupling を最小化する。 +#[derive(Deserialize)] +struct ParkedStatePartial { + pr: Option, + repo: Option, + next_wakeup_at_unix: Option, + wakeup_reason: Option, + /// 監視ステータス。`"parked_*"` (parked_rate_limit / parked_review_recheck) のみ + /// catch-up nudge の対象。`"action_required"` 等の terminal 値では + /// next_wakeup_at_unix が古い park 由来で残っていても nudge を抑制する。 + #[serde(default)] + action: String, +} + /// session-id ファイルのパス (.claude/.session-id) -fn session_id_file_path() -> std::path::PathBuf { +fn session_id_file_path() -> PathBuf { std::env::current_exe() .unwrap_or_default() .parent() @@ -32,6 +54,62 @@ fn session_id_file_path() -> std::path::PathBuf { .join(".session-id") } +/// cli-pr-monitor の state file パス (`/pr-monitor-state.json`)。 +/// hooks-session-start.exe は cli-pr-monitor.exe と同じ `.claude/` 配下に配置される +/// 前提 (deploy:hooks スクリプトで保証)。 +fn pr_monitor_state_path() -> PathBuf { + std::env::current_exe() + .unwrap_or_default() + .parent() + .unwrap_or(Path::new(".")) + .join("pr-monitor-state.json") +} + +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) +} + +/// `next_wakeup_at_unix` が現在時刻以前なら catch-up nudge メッセージを返す。 +/// +/// session が park 中に終了 (CronCreate 発火前) し、後で再開された場合、 +/// CronCreate スケジュールが消えているため自動 wakeup は起こらない。 +/// このとき手動で監視継続するための指示を Claude に渡す。 +/// +/// 返り値: Some(message) なら additionalContext に注入する文字列、None なら何もしない。 +/// +/// 抑制条件: action が `"parked_*"` でない (= terminal 状態) 場合、`next_wakeup_at_unix` +/// が残っていても false-positive nudge を出さない。terminal 経路では cli-pr-monitor が +/// `next_wakeup_at_unix` を明示クリアしないため、action ベースの guard が必要。 +fn compute_catchup_nudge(state: &ParkedStatePartial, now_unix: i64) -> Option { + if !state.action.starts_with("parked_") { + return None; + } + let wakeup_at = state.next_wakeup_at_unix?; + if wakeup_at > now_unix { + return None; + } + let pr = state + .pr + .map(|n| format!("#{}", n)) + .unwrap_or_else(|| "?".into()); + let repo = state.repo.as_deref().unwrap_or("?"); + let reason = state.wakeup_reason.as_deref().unwrap_or("unknown"); + Some(format!( + "[PR_MONITOR_CATCHUP]\n\ + pending wakeup detected for PR {pr} ({repo}), reason={reason}, scheduled_at_unix={wakeup_at}, now={now_unix}.\n\ + CronCreate may have expired during session downtime. If the PR is still relevant, run `{cmd}` to resume monitoring.", + cmd = RESUME_MONITORING_COMMAND + )) +} + +fn read_parked_state(path: &Path) -> Option { + let content = std::fs::read_to_string(path).ok()?; + serde_json::from_str(&content).ok() +} + fn main() { // stdin から JSON を読み取り let mut input = String::new(); @@ -73,12 +151,24 @@ fn main() { let _ = std::fs::write(&sid_path, &session_id); } - // 3. additionalContext で Claude のコンテキストにも注入 - // serde_json で組み立てることで session_id 内の特殊文字を安全にエスケープ + emit_session_start_output(&session_id); +} + +/// `additionalContext` (session_id + 任意の PR monitor catch-up nudge) を組み立て +/// Claude Code に返す JSON を stdout に書き出す。 +/// serde_json で組み立てることで session_id 内の特殊文字を安全にエスケープする。 +fn emit_session_start_output(session_id: &str) { + let mut context = format!("CLAUDE_CODE_SESSION_ID={}", session_id); + if let Some(state) = read_parked_state(&pr_monitor_state_path()) { + if let Some(nudge) = compute_catchup_nudge(&state, current_unix_secs()) { + context.push_str("\n\n"); + context.push_str(&nudge); + } + } let output = serde_json::json!({ "hookSpecificOutput": { "hookEventName": "SessionStart", - "additionalContext": format!("CLAUDE_CODE_SESSION_ID={}", session_id), + "additionalContext": context, } }); println!("{}", output); @@ -234,6 +324,164 @@ mod tests { let _ = std::fs::remove_file(&tmp); } + fn parked_state( + pr: Option, + repo: Option<&str>, + wakeup_at: Option, + reason: Option<&str>, + action: &str, + ) -> ParkedStatePartial { + ParkedStatePartial { + pr, + repo: repo.map(String::from), + next_wakeup_at_unix: wakeup_at, + wakeup_reason: reason.map(String::from), + action: action.into(), + } + } + + #[test] + fn catchup_nudge_none_when_no_wakeup_scheduled() { + let state = parked_state( + Some(42), + Some("o/r"), + None, + Some("review_recheck"), + "parked_review_recheck", + ); + assert!(compute_catchup_nudge(&state, 1_775_088_000).is_none()); + } + + #[test] + fn catchup_nudge_none_when_wakeup_in_future() { + let state = parked_state( + Some(42), + Some("o/r"), + Some(1_775_088_000), + Some("review_recheck"), + "parked_review_recheck", + ); + let now = 1_775_087_999; + assert!(compute_catchup_nudge(&state, now).is_none()); + } + + #[test] + fn catchup_nudge_emitted_when_wakeup_passed() { + let state = parked_state( + Some(42), + Some("owner/repo"), + Some(1_775_088_000), + Some("review_recheck"), + "parked_review_recheck", + ); + let now = 1_775_088_001; + let msg = compute_catchup_nudge(&state, now).expect("nudge should be emitted"); + assert!(msg.contains("[PR_MONITOR_CATCHUP]")); + assert!(msg.contains("PR #42")); + assert!(msg.contains("owner/repo")); + assert!(msg.contains("review_recheck")); + assert!( + msg.contains(RESUME_MONITORING_COMMAND), + "nudge は const RESUME_MONITORING_COMMAND を hint として埋め込むこと (pre-push-review #115 [B] 対策、コマンド名 rename 時に test が落ちて drift を catch)" + ); + } + + #[test] + fn catchup_nudge_handles_missing_optional_fields() { + let state = parked_state(None, None, Some(0), None, "parked_review_recheck"); + let msg = compute_catchup_nudge(&state, 1).expect("nudge should still be emitted"); + assert!(msg.contains("PR ?")); + assert!(msg.contains("(?)")); + assert!(msg.contains("reason=unknown")); + } + + /// terminal 経路では `next_wakeup_at_unix` が古い park 由来で残っていても + /// false-positive nudge を出さない (advisor 指摘: 順位 55 review)。 + #[test] + fn catchup_nudge_suppressed_for_terminal_action_required() { + let state = parked_state( + Some(42), + Some("o/r"), + Some(1_775_088_000), + Some("review_recheck"), + "action_required", + ); + let now = 1_775_088_001; + assert!( + compute_catchup_nudge(&state, now).is_none(), + "terminal 経路 (action_required) では nudge を抑制すること" + ); + } + + #[test] + fn catchup_nudge_suppressed_for_continue_monitoring() { + let state = parked_state( + Some(42), + Some("o/r"), + Some(1_775_088_000), + None, + "continue_monitoring", + ); + let now = 1_775_088_001; + assert!(compute_catchup_nudge(&state, now).is_none()); + } + + #[test] + fn catchup_nudge_emitted_for_parked_rate_limit() { + let state = parked_state( + Some(42), + Some("o/r"), + Some(1_775_088_000), + Some("rate_limit_retry"), + "parked_rate_limit", + ); + let msg = compute_catchup_nudge(&state, 1_775_088_001) + .expect("parked_rate_limit should emit nudge"); + assert!(msg.contains("rate_limit_retry")); + } + + #[test] + fn read_parked_state_returns_none_when_file_missing() { + let tmp = + std::env::temp_dir().join(format!("test-parked-state-missing-{}", std::process::id())); + let _ = std::fs::remove_file(&tmp); + assert!(read_parked_state(&tmp).is_none()); + } + + #[test] + fn read_parked_state_parses_partial_fields() { + let tmp = + std::env::temp_dir().join(format!("test-parked-state-partial-{}", std::process::id())); + let json = r#"{ + "pr": 42, + "repo": "owner/repo", + "started_at": "2026-05-01T00:00:00Z", + "action": "parked_review_recheck", + "summary": "...", + "notified": false, + "daemon_pid": null, + "daemon_status": "active", + "next_wakeup_at_unix": 1775088000, + "wakeup_reason": "review_recheck" + }"#; + std::fs::write(&tmp, json).unwrap(); + + let state = read_parked_state(&tmp).expect("should parse"); + assert_eq!(state.pr, Some(42)); + assert_eq!(state.repo.as_deref(), Some("owner/repo")); + assert_eq!(state.next_wakeup_at_unix, Some(1_775_088_000)); + assert_eq!(state.wakeup_reason.as_deref(), Some("review_recheck")); + assert_eq!(state.action, "parked_review_recheck"); + + let _ = std::fs::remove_file(&tmp); + } + + #[test] + fn pr_monitor_state_path_ends_with_filename() { + let path = pr_monitor_state_path(); + assert!(path.to_string_lossy().ends_with("pr-monitor-state.json")); + } + #[test] fn session_id_file_empty_is_written() { let tmp = std::env::temp_dir().join(format!("test-sid-empty-{}", std::process::id()));