diff --git a/docs/adr/adr-034-coderabbit-auto-monitoring.md b/docs/adr/adr-034-coderabbit-auto-monitoring.md index 12222238..63efa9b2 100644 --- a/docs/adr/adr-034-coderabbit-auto-monitoring.md +++ b/docs/adr/adr-034-coderabbit-auto-monitoring.md @@ -73,6 +73,9 @@ CR は format を時間経過で変更するため、本リポジトリの実装 |---|---|---| | ~2026 年初頃 (旧 format) | `Rate limit exceeded` (本文先頭、heading なし) | `Please wait \*?\*?(\d+) minutes? and (\d+) seconds?` / 短縮形 `Please wait \*?\*?(\d+) minutes?` | | 2026-05 観測 (新 format) | `rate limited by coderabbit.ai` (HTML コメント `` 内、`## Review limit reached` heading 併設) | `More reviews will be available in (\d+) minutes? and (\d+) seconds?` / 短縮形 `More reviews will be available in (\d+) minutes?` | +| 2026-07-20 観測 (第 3 世代、PR #309) | 変わらず `rate limited by coderabbit.ai` (`## Review limit reached` heading も維持) | `Next review available in[:*\s]*(\d+) minutes?...` (`**Next review available in:** **57 minutes**` の形。ラベルと数値の間に markdown 強調が挟まるため区切りを文字クラスで吸収) | + +**未知書式の fail-closed fallback (2026-07-20 追加、WP-15 追補 R2)**: marker は一致したが wait time regex がどれも一致しない場合、旧実装は `parse_rate_limit` が `None` を返し「rate-limit ではない」= 検知の沈黙に倒れていた。書式追随は本質的に後追いになるため、**marker 一致を制限の根拠として採用し、待機時間だけを既定値 (`UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES` = 30 分) で埋める**方式に変更した。既定値が実際の reset より短い場合は wakeup 後に再検出されて再 park されるだけで、retry は `max_retries` で有界。既定値適用時は checker が stderr に警告を出し (cli-pr-monitor がログ転送)、書式再変更の検知シグナルを兼ねる。これにより「書式変更 → 即 silent success」の経路は構造的に閉じ、書式追随 (下記手順) は待機時間精度の改善に格下げされる。 **HTML マーカー優先**: walkthrough body の HTML コメント (``) は heading 文言や本文より stable な可能性が高いため、新 format 検出の優先 source とする (CR 側で UI 文言は変えても internal marker は維持する傾向、本リポジトリ未検証だが post-merge-feedback で再評価)。 @@ -82,9 +85,9 @@ format drift で `is_rate_limit_comment` が常時 false を返す symptom (= 30 1. **観測**: 該当 PR で `gh api repos/.../issues//comments --jq '.[] | select(.user.login == "coderabbitai[bot]")' | head` で walkthrough body を確認 2. **grep**: 新 marker 候補 (HTML コメント / heading / 本文文言) を特定 -3. **marker 配列 append**: `src/check-ci-coderabbit/src/main.rs` の `RATE_LIMIT_MARKERS` 配列に新 marker を追加 -4. **regex 追加**: 同 main.rs の `extract_wait_time` から呼ばれる `extract__format_wait_time` ヘルパーを新規追加 (旧 + 新の共存パターン) -5. **fixture 追加**: 同 `#[cfg(test)]` mod に新 format fixture を 2-3 variant 追加 (順位 168 と同 pattern)。既存 fixture は backward compat のため維持 +3. **marker 配列 append**: `src/check-ci-coderabbit/src/markers.rs` の `RATE_LIMIT_MARKERS` 配列に新 marker を追加 +4. **regex 追加**: `src/check-ci-coderabbit/src/rate_limit.rs` の `extract_wait_time` から呼ばれる `extract__format_wait_time` ヘルパーを新規追加 (既存世代との共存パターン) +5. **fixture 追加**: 同 `#[cfg(test)]` mod に新 format fixture を 2-3 variant 追加 (順位 168 と同 pattern)。既存 fixture は backward compat のため維持。**実 incident の comment body を出典付きで fixture 化する** (ADR-049) 6. **ADR-034 update**: 本 ADR の § 既知 CR rate-limit format 一覧 table に新 format 行を append ### auto-trigger 投稿 diff --git a/docs/harness-improvement-plan.md b/docs/harness-improvement-plan.md index e23a8ea3..ed8cae12 100644 --- a/docs/harness-improvement-plan.md +++ b/docs/harness-improvement-plan.md @@ -267,7 +267,9 @@ Anthropic 公式のハーネスエンジニアリング指針（決定論的基 > > **Linux 実測で発見した副次不具合**: `cli-pr-monitor` の lock が**同時取得**を許していた (8 スレッド中 6 つが取得)。`create_new` は atomic だが直後のファイルは空で、その窓を読んだ側が TOML parse 失敗を一律「stale」と扱って全員 takeover していた。Windows ではスケジューリング差で顕在化していなかっただけで欠陥は同じ。parse 失敗を内容で 2 分 (空 = 書き込み中 → busy / 非空の不正 = 破損 → takeover) して修正。**「Windows だけで回していると気付けない設計欠陥が実在した」= Linux 実測と WP-16 (CI matrix) の価値を裏づける実例。** > -> **`完了` 条件**: (1) 本変更が master に入り release-binaries.yml が実走して `nightly` release が生成されること、(2) 実際の claude.ai/code セッションで `cloud-setup.sh` を走らせ hooks 発火と `cargo test` 通過を確認すること。いずれも本セッションでは実施不能 (release 未生成 / クラウド環境未使用) のため `実装済` に留める。**Linux 側の未検証領域**: `#[cfg(windows)]` ガードのテスト (pump_child_io の deadlock 保護、run_cmd_capture の stdout/stderr 分離) は Linux で skip されるため、WP-16 の CI matrix で扱う。以下は当初ステップ (記録用)。 +> **`完了` 条件 (1) 達成 (2026-07-20、2026-07-21 に再実測)**: PR #307 マージ時の release-binaries.yml run は build job が失敗した (master が赤で、#308 の stop-tool-call-leak E2E 修正が必要だった)。#308 マージ後の run (commit `541adde1`) が成功し、固定タグ `nightly` の prerelease が生成された (tarball `claude-code-hooks-x86_64-unknown-linux-gnu.tar.gz` 9,721,643 bytes + `.sha256`)。**認証なしでの取得可否を実測**: WSL Ubuntu 24.04 から素の `curl -sSfL` で両 asset を取得 (gh CLI 認証なし) → `sha256sum -c` 一致 → 展開して 16 バイナリ + BUILD_INFO を確認 → **release バイナリそのもので hooks 実発火**まで確認 (`hooks-pre-tool-validate` が `rm -rf /` を exit 2 でブロックし `echo hello` を exit 0 で通す、`hooks-session-start` が additionalContext JSON を出力)。これで § WP-15 ④ の「public リポジトリの Release asset は素の HTTPS で取得できるため gh CLI 認証は不要」という設計判断が実 URL・実 asset で裏付けられた。 +> +> **`完了` 条件**: (1) 本変更が master に入り release-binaries.yml が実走して `nightly` release が生成されること (**上記のとおり達成**)、(2) 実際の claude.ai/code セッションで `cloud-setup.sh` を走らせ hooks 発火と `cargo test` 通過を確認すること。(2) は本セッションでは実施不能 (クラウド環境未使用) のため `実装済` に留める。**Linux 側の未検証領域**: `#[cfg(windows)]` ガードのテスト (pump_child_io の deadlock 保護、run_cmd_capture の stdout/stderr 分離) は Linux で skip されるため、WP-16 の CI matrix で扱う。以下は当初ステップ (記録用)。 - **目的**: 使い捨てのクラウドセッションで 19 crate をビルドせずにハーネスを即時有効化する。 - **ステップ**: @@ -277,6 +279,44 @@ Anthropic 公式のハーネスエンジニアリング指針（決定論的基 - **受け入れ基準**: claude.ai/code セッションで SessionStart / PostToolUse / Stop hooks が発火し、`cargo test` と push pipeline の dry-run が通る。 - **要確認事項**: クラウドのデフォルト network access は Trusted（許可リスト制）。GitHub Release のダウンロードは既定で通るはずだが、setup script 内で `gh` CLI の認証が必要な場合は環境変数（環境設定の env vars）でトークンを渡す構成を検証すること。 +#### WP-15 追補: 監視 fail-open 修正のゼロ再構築（旧 PR #309 全破棄、2026-07-20 決定） + +> **実装済 (2026-07-21)**: 3 コミットで R1〜R4 を実装し、R5 を上記 § WP-15 に再録した。旧 #309 のコード・テストは一切参照せず、plan の要件記述のみから再構築している。 +> +> **破棄の実施**: PR #309 を破棄理由付きコメントで close、remote branch `fix/monitor-fail-open-signals` を削除、ローカル 4 commits を abandon、bookmark を forget、`.claude/pr-monitor-state.json` の stale wakeup state を削除。いずれも実施済み。 +> +> **① R2 (書式追随 + fail-closed 化)**: CR の第 3 世代書式 `**Next review available in:** **57 minutes**` の抽出を追加 (区切りを `[:*\s]*` で吸収し強調記法の変化に耐える)。加えて**書式追随を前提にしない構造**へ変更した: marker (`rate limited by coderabbit.ai`) が一致したのに待機時間をどの既知書式でも読めない場合、旧実装は `None` = 「rate-limit ではない」に倒れていたが、marker 一致を制限の根拠として採用し待機時間だけを既定 30 分で埋める。既定値が実 reset より短ければ wakeup 後に再検出されて再 park されるだけで、retry は `max_retries` で有界。既定値適用時は checker が stderr に警告し (monitor がログ転送)、「30 分」を CR の申告値と誤読させない + 書式再変更の検知シグナルを兼ねる。ADR-034 の既知 format 一覧に第 3 世代行と本方針を追記し、更新手順の stale なファイル参照 (`main.rs` → `markers.rs` / `rate_limit.rs`) も修正。 +> +> **② R1/R4 (silent success の排除、本丸)**: takt reviewer の提案どおり `decide()` に rate_limit を渡し、**action の算出そのものを正す**一点修正にした (旧 #309 の monitor 側 2 箇所分散は不採用)。R1 = rate-limit 検出中かつ「レビュー実施の陽性証拠」が無ければ `continue_monitoring` を返し、判断を monitor 既存の rate-limit branch (park / 再 trigger) に委ねる。`has_actionable` 分岐**より前**に置くのが要点で、これが無いと過去サイクル由来の未解決スレッドだけで `action_required` に抜けて監視が終わる。R4 = rate-limit を検出できなかった場合の backstop として、陽性証拠が無い限り `stop_monitoring_success` を出さない。**陽性証拠の定義**: `review_state` (commit status) は制限中でも pass になるため証拠に使わず、`push_time` で絞られた「今サイクルの CR 出力そのもの」= `walkthrough_clean` / `actionable_comments` が読めた (`Some(0)` 含む) / `new_comments > 0` のみを採用。`unresolved_threads` は push_time で絞られず過去サイクルの残骸を含み得るので除外した。`build_summary` も rate-limit 中は「CodeRabbit指摘なし」と断定せず「レート制限中 (レビュー未実施)」を出す。 +> +> **③ R3 (判定文)**: 判定順を「未確定 → 重大 → 未解決 → 軽微 → 問題なし」に整理し、未確定要素 (park / rate-limit / review 未完了 / 未解決スレッド) を findings の有無**より先に**評価する。`compute_verdict` を未確定判定と findings 判定の 2 関数に分割し、「断定文はどの guard を通過して初めて出せるのか」を関数境界で表現した。 +> +> **検証 (実測)**: Windows + WSL Ubuntu 24.04 (実 Linux) の双方で `cargo test --workspace` 全 pass・`clippy --workspace --all-targets --all-features -- -D warnings` clean。`lint:docs` / `lint:md` 退行なし。**既存テストは無改修で全 pass** (新 gate が確立済み挙動を乱していないことの確認。decide/summary 100 件、monitor verdict 13 件)。 +> +> **incident 実データでの実測 (実エントリポイント経由)**: close 後の PR #309 に残る実 rate-limit comment (2026-07-20T12:10:47Z 投稿 / 12:38:33Z 編集、第 3 世代書式) に対し、**実 exe を `--push-time 2026-07-20T12:37:00Z` で実走**させた (この push_time は rate-limit comment の `updated_at` を含みつつ、後から投稿された CR の「Review finished」コメントを除外するため incident 当時と同形の入力になる)。結果は `action: continue_monitoring` / `summary: "CI実行中。CodeRabbitレート制限中 (レビュー未実施)"` / `rate_limit.wait_minutes: 57` / `wait_time_parsed: true`。**同一入力で修正前バイナリと比較**: `.claude/` にデプロイ済みだった旧 #309 branch 由来の exe (= R2 相当の検知修正は入っているが `decide()` 統合は無い状態) は `action: stop_monitoring_success` / `summary: "CI実行中。CodeRabbit指摘なし"` を返した。**rate-limit を検知できていても `decide()` に渡っていなければ silent success になる**という根本原因が、実データで直接裏づけられた形になっている (同時に、症状側パッチでは不十分だったことの実証でもある)。 +> +> **E2E カバレッジの正直な申告**: 上記で担保されたのは (a) 全 gate のユニット検証、(b) **checker の実エントリポイントを実データで通した単体実測**、(c) 修正前後の差分の実測、の 3 点。**未実測**は次のとおり: cli-pr-monitor 側の統合経路 (checker 起動 → `continue_monitoring` 受領 → `handle_rate_limit_branch` で park → PARK signal 出力) は、本セッション中に CR レート制限が自然発生しなかったため実走させていない。park 後の wakeup → 再 trigger 経路も同様に未実測 (この経路の実測はレート制限の自然発生時にしか行えない)。monitor 側の分岐順序 (terminal 短絡が rate-limit branch より先に発火する構造) は本変更で触っておらず、`continue_monitoring` を返せば branch に到達することは既存実装の性質に依存している。また `#[cfg(windows)]` ガードのテストは Linux で skip される (WP-16 の CI matrix で扱う既存ギャップ)。 +> +> **`完了` 条件**: 本変更を含む PR がマージされ、その後の実 push/PR サイクルで CR レート制限が発生した際に (a) 監視が success で終わらず park すること、(b) レポート判定文が保留を出すこと、を実観測したら `完了`。それまでは `実装済` に留める。 +> +> **経緯**: WP-15 の PR #307 運用中に、PR 監視系の fail-open 不具合群（CodeRabbit レート制限中の silent success 等）が実発火した。修正 PR #309（`fix/monitor-fail-open-signals`、4 commits、head `908f6a9b`）を作成したが、4 コミット目の初版が「本番 config では一度も実行されない誤修正 + 実エントリポイントを迂回して pass するテスト」であり、pre-push review（takt）の High REJECT（finding `SIM-NEW-iteration.rs-L259`）→ fix step の自動書き直しを経た合成物となった。コミットメッセージには無効と判明した検証主張が残存し、実装も症状側への多層パッチ（monitor 側 2 箇所分散）である。**続修よりゼロ再構築が速いと判断し、#309 は再利用なしで全破棄する**（中途半端な状態の引き継ぎを避け、それによってより良い実装が制約される可能性を排除するため。健全に見えるコミットも含めて引き継がない）。 + +- **破棄対象と手順（本追補の時点では未実施）**: 新 PR 作成後に PR #309 を参照コメント付きで close + remote branch 削除。ローカルの 4 commits（change-id: `ppkwnvsl` / `mnkuluov` / `ovwltoyx` / `tzlssomy`）を abandon し、bookmark `fix/monitor-fail-open-signals` を forget。`.claude/pr-monitor-state.json` に残る #309 向け stale wakeup state を破棄（SessionStart catchup の「#309 監視再開」案内は無視してよい）。 +- **やりたいこと（要件のみ。実装方式は新実装の裁量に委ね、旧 #309 のコード・テストは参照しない）**: + 1. **R1（必須）**: CodeRabbit がレート制限でレビューを開始できないまま、監視が「レビュー済み・指摘なし」（`stop_monitoring_success` / 判定文「問題は見つかりませんでした」）と報告する silent success を排除する（PR #307 / #309 で実観測）。 + 2. **R2（必須）**: CR の rate-limit comment 書式変更で検知が沈黙しないこと。既知 3 世代（`Please wait **N minutes and M seconds**` → `More reviews will be available in N minutes and M seconds` → `**Next review available in:** **N minutes**`）に加え、未知書式でも marker（`rate limited by coderabbit.ai`）一致時は制限として扱う。[ADR-034](adr/adr-034-coderabbit-auto-monitoring.md) が予告していた再発事案（PR #182/#184 に次ぐ 2 度目の書式変更起因 regression）。 + 3. **R3（必須）**: 監視レポートの人間向け判定文が、findings が空でも未解決スレッド・レート制限等の未確定要素を無視して「問題なし」と断定しないこと（実観測: 「未解決スレッド2件」表示と同一レポート内で「問題は見つかりませんでした」）。 + 4. **R4（推奨）**: success 判定に「レビューが実際に実施された陽性証拠」を要求し、CR がマーカー文言自体を変えても silent success に戻らない構造にする。見送る場合は残存リスクとして todo 化する。 + 5. **R5（必須）**: WP-15 `完了` 条件 (1) 達成の記録を本 plan に再録する（旧 #309 の docs コミット相当。事実: 2026-07-20 に PR #307/#308 マージ後、release-binaries.yml が成功し `nightly` prerelease を生成〔tarball 9.72MB + sha256、commit `541adde1`〕。認証なし curl 取得 → checksum 一致 → 展開 → release バイナリそのもので Linux 上の hooks 実発火まで実測済み）。 +- **検証済みの根本原因（再調査不要、2026-07-20 コード実読）**: ① CR はレート制限中も commit check を「pass / Review completed」にする（外部 SaaS 挙動、実観測）→ ② checker はこれを `review_state` に採用（`src/check-ci-coderabbit/src/main.rs` の `fetch_coderabbit_commit_state`）→ ③ `parse_rate_limit` の結果は出力 JSON に添付されるだけで `decide()`（`src/check-ci-coderabbit/src/decide.rs`）に渡らない → ④ 本 repo の PR に CI run は無く、`decide()` は `runs` 空の pending を pending 扱いしない → ⑤ 判定条件をすり抜け `stop_monitoring_success` → ⑥ monitor 側は action をそのまま採用し、terminal 短絡（`src/cli-pr-monitor/src/stages/poll/iteration.rs`）が rate-limit 処理（`handle_rate_limit_branch`。park / 再トリガー機構は既存・有界）より先に発火する。 +- **参考（拘束しない）**: takt reviewer は「`decide()` に rate_limit を渡して action の算出自体を正す」統合を提案していた。旧 #309 の monitor 側 2 箇所分散は、この High finding を誘発した反面教師。 +- **検証要件（全段階で必須。検証せずに進めない）**: + - 全コミットで Windows + Linux（WSL）の `cargo test --workspace` + clippy `-D warnings`。docs 変更は `lint:docs` / `lint:md`。 + - incident 実データでの実測: close 後の PR #309 に実 rate-limit comment（2026-07-20T12:10:47Z 投稿、現行書式）が残る見込み。`--push-time` をコメント時刻以前に指定すれば checker 単体で incident 入力を副作用なしに再現できる（PR #307 側のコメントはレビュー完了時に walkthrough へ編集され残っていない）。 + - E2E のカバレッジを正直に申告する: wakeup 経路の実測は CR レート制限の自然発生時のみ可能。発生しなかった場合に、何がユニット / checker 単体実測で担保され、何が未実測かを PR に明記する。 + - 検証主張の規律: 旧作業では「wakeup 経路 vs 初回 park 経路」という経路違いの比較を 2 回「実環境検証済み」と報告した。実測は経路の同一性を確認してから主張する。 +- **教訓（新実装のセルフチェック）**: (a) 修正が本番 config（`check_ci=true` / `check_coderabbit=true`、skip なし）の経路で実行されることをテストで固定する（旧初版は skip 構成でしか呼ばれない dead code だった）。(b) テストは実エントリポイントを迂回しない。(c) fixture は実データを使う（[ADR-049](adr/adr-049-incident-eval-regression-suite.md)）。 + ### WP-16: CI matrix（移植退行防止） - **ステップ**: `windows-latest` + `ubuntu-latest` で cargo test + hooks smoke test（fixture stdin → 期待する block/pass 判定を assert。WP-08 の資産を流用）。安定後に required check 化（todo 順位 6 の Branch Protection 整備と連動）。 diff --git a/src/check-ci-coderabbit/src/decide.rs b/src/check-ci-coderabbit/src/decide.rs index 86fbe9a9..4ab3759c 100644 --- a/src/check-ci-coderabbit/src/decide.rs +++ b/src/check-ci-coderabbit/src/decide.rs @@ -1,18 +1,54 @@ //! CI / CodeRabbit 状態から `(status, action)` を判定するロジックと人間向け summary。 -use crate::models::{CiStatus, CodeRabbitStatus}; +use crate::models::{CiStatus, CodeRabbitStatus, RateLimitInfo}; -/// CI と CodeRabbit の状態から `(status, action)` を決定する。 +/// CodeRabbit が**この push サイクルで実際にレビューを実施した**陽性証拠があるか。 +/// +/// `review_state` (commit status) を証拠に使ってはならない: CR はレート制限で +/// レビューを開始できなかった場合でも commit status を pass にする (2026-07-20、 +/// PR #307/#309 で実観測)。そのため commit status だけを見ると「レビュー済み・ +/// 指摘なし」と区別が付かず silent success に倒れる。 +/// +/// 証拠として採用するのは、いずれも `push_time` で絞り込まれた「今サイクルの +/// CR 出力そのもの」に限る: +/// - `walkthrough_clean`: CR が walkthrough を投稿し clean marker を出した +/// - `actionable_comments`: CR の review body から "Actionable comments posted: N" +/// を読めた (`Some(0)` も「レビューして 0 件だった」= 陽性証拠) +/// - `new_comments`: 今サイクルの CR コメントが存在する +/// +/// `unresolved_threads` は `push_time` で絞られず過去サイクルの残骸を含み得るため +/// 証拠に採用しない (未解決スレッドの存在は「今回レビューが走った」ことを示さない)。 +fn has_review_evidence(cr: &CodeRabbitStatus) -> bool { + cr.walkthrough_clean || cr.actionable_comments.is_some() || cr.new_comments > 0 +} + +/// CI / CodeRabbit / rate-limit の状態から `(status, action)` を決定する。 /// /// 判定優先順位 (上から): /// 1. CI failure → error / stop_monitoring_failure /// 2. walkthrough_clean かつ unresolved_threads 無し → complete / stop_monitoring_success -/// 3. review_state == not_found かつ has_actionable → action_required -/// 4. CI pending or CR pending → continue_monitoring -/// 5. review_state failure/error → stop_monitoring_failure -/// 6. has_actionable → action_required -/// 7. それ以外 → complete / stop_monitoring_success -pub(crate) fn decide(ci: &CiStatus, cr: &CodeRabbitStatus) -> (String, String) { +/// 3. **rate-limit 検出かつレビュー実施の陽性証拠なし → continue_monitoring** (R1) +/// 4. review_state == not_found かつ has_actionable → action_required +/// 5. CI pending or CR pending → continue_monitoring +/// 6. review_state failure/error → stop_monitoring_failure +/// 7. has_actionable → action_required +/// 8. **レビュー実施の陽性証拠なし → continue_monitoring** (R4) +/// 9. それ以外 → complete / stop_monitoring_success +/// +/// 3 と 8 はどちらも「レビュー未実施を success/action_required と誤って確定しない」 +/// ための gate だが、役割が違うので両方必要: +/// - 3 は rate-limit が判明しているケースを **7 (has_actionable) より先に**捕まえ、 +/// 監視を継続して呼び出し側の rate-limit branch (park / 再 trigger) に委ねる。 +/// これが無いと、過去サイクル由来の未解決スレッドがあるだけで action_required に +/// 抜け、レビューが走っていないのに監視が終了する。 +/// - 8 は rate-limit marker 自体を CR が変えた場合の backstop。陽性証拠が無い限り +/// success を出さないので、marker 追随に失敗しても silent success には戻らない +/// (最悪 max_duration まで監視して timed_out = 安全側)。 +pub(crate) fn decide( + ci: &CiStatus, + cr: &CodeRabbitStatus, + rate_limit: Option<&RateLimitInfo>, +) -> (String, String) { if ci.overall == "failure" { return ("error".to_string(), "stop_monitoring_failure".to_string()); } @@ -29,6 +65,9 @@ pub(crate) fn decide(ci: &CiStatus, cr: &CodeRabbitStatus) -> (String, String) { "stop_monitoring_success".to_string(), ); } + if rate_limit.is_some() && !has_review_evidence(cr) { + return ("pending".to_string(), "continue_monitoring".to_string()); + } if cr.review_state == "not_found" && has_actionable { return ("action_required".to_string(), "action_required".to_string()); } @@ -43,6 +82,9 @@ pub(crate) fn decide(ci: &CiStatus, cr: &CodeRabbitStatus) -> (String, String) { if has_actionable { return ("action_required".to_string(), "action_required".to_string()); } + if !has_review_evidence(cr) { + return ("pending".to_string(), "continue_monitoring".to_string()); + } ( "complete".to_string(), "stop_monitoring_success".to_string(), @@ -50,9 +92,22 @@ pub(crate) fn decide(ci: &CiStatus, cr: &CodeRabbitStatus) -> (String, String) { } /// CI / CodeRabbit 状態を人間向け日本語サマリー文字列で返す。 -pub(crate) fn build_summary(ci: &CiStatus, cr: &CodeRabbitStatus) -> String { +/// +/// rate-limit 検出中でレビュー実施の陽性証拠が無い場合、CR 部分を +/// 「レート制限中」に差し替える。`review_state` は制限中でも pass になるため、 +/// 差し替えないと「CodeRabbit指摘なし」= レビュー済みで問題なしと読める +/// 文言を出してしまう (PR #307/#309 実観測の silent success の一部)。 +pub(crate) fn build_summary( + ci: &CiStatus, + cr: &CodeRabbitStatus, + rate_limit: Option<&RateLimitInfo>, +) -> String { let ci_part = build_summary_ci_part(ci); - let cr_part = build_summary_cr_part(cr); + let cr_part = if rate_limit.is_some() && !has_review_evidence(cr) { + "CodeRabbitレート制限中 (レビュー未実施)".to_string() + } else { + build_summary_cr_part(cr) + }; format!("{}。{}", ci_part, cr_part) } @@ -119,6 +174,181 @@ mod tests { use super::*; use crate::models::CiRunSummary; + /// PR #309 incident の CR 状態を再現する。 + /// + /// 2026-07-20 の実観測: CR はレート制限でレビューを開始できなかったが + /// commit status は pass (`review_state = "success"`)、今サイクルの CR 出力は + /// 皆無 (`actionable_comments = None` / `new_comments = 0` / walkthrough なし)、 + /// 一方で過去サイクル由来の未解決スレッドが 2 件残っていた。 + fn pr309_incident_cr_status() -> CodeRabbitStatus { + CodeRabbitStatus { + review_state: "success".to_string(), + new_comments: 0, + actionable_comments: None, + unresolved_threads: Some(2), + walkthrough_clean: false, + } + } + + /// PR #309 incident の rate-limit 情報 (第 3 世代書式、57 分待機)。 + fn pr309_incident_rate_limit() -> RateLimitInfo { + RateLimitInfo { + until_unix_secs: 1_784_556_707, + comment_event_time: "2026-07-20T12:10:47Z".to_string(), + wait_minutes: 57, + wait_seconds: 0, + wait_time_parsed: true, + } + } + + fn ci_no_runs(overall: &str) -> CiStatus { + CiStatus { + overall: overall.to_string(), + runs: vec![], + } + } + + /// R1 (incident 再現): rate-limit 検出中でレビュー実施の陽性証拠が無ければ、 + /// 未解決スレッドがあっても `action_required` で監視を終了せず継続する。 + /// + /// この gate が無いと `has_actionable` 分岐に先に落ち、「レビューは走って + /// いないのに未解決スレッドを理由に監視終了」= rate-limit branch (park / + /// 再 trigger) に一度も到達しない。 + #[test] + fn decide_rate_limited_without_review_evidence_continues_monitoring() { + let ci = ci_no_runs("pending"); + let cr = pr309_incident_cr_status(); + let rl = pr309_incident_rate_limit(); + + let (status, action) = decide(&ci, &cr, Some(&rl)); + + assert_eq!(status, "pending"); + assert_eq!( + action, "continue_monitoring", + "rate-limit 中はレビュー未実施なので監視を継続し rate-limit branch に委ねる" + ); + } + + /// R1 の gate が効いているのは **rate_limit の有無だけ**であることを対比で固定する。 + /// 同じ CR 状態でも rate_limit が無ければ従来どおり `action_required`。 + #[test] + fn decide_same_cr_status_without_rate_limit_keeps_action_required() { + let ci = ci_no_runs("pending"); + let cr = pr309_incident_cr_status(); + + let (_, action) = decide(&ci, &cr, None); + + assert_eq!( + action, "action_required", + "rate_limit が無ければ未解決スレッドは通常どおり action_required" + ); + } + + /// R1 が効きすぎないこと: walkthrough clean marker はレビュー完走の陽性証拠なので、 + /// 同サイクルに rate-limit comment が残っていても success を出す + /// (残骸 rate-limit comment で監視が終わらなくなるのを防ぐ)。 + #[test] + fn decide_rate_limited_but_walkthrough_clean_still_completes() { + let ci = ci_no_runs("success"); + let cr = CodeRabbitStatus { + review_state: "not_found".to_string(), + new_comments: 0, + actionable_comments: None, + unresolved_threads: Some(0), + walkthrough_clean: true, + }; + let rl = pr309_incident_rate_limit(); + + let (status, action) = decide(&ci, &cr, Some(&rl)); + + assert_eq!(status, "complete"); + assert_eq!(action, "stop_monitoring_success"); + } + + /// R1 が効きすぎないこと 2: CR が実際にレビューして指摘を出していれば + /// (= 陽性証拠あり)、rate-limit comment が残っていても action_required を出す。 + #[test] + fn decide_rate_limited_with_actionable_evidence_reports_action_required() { + let ci = ci_no_runs("success"); + let cr = CodeRabbitStatus { + review_state: "success".to_string(), + new_comments: 0, + actionable_comments: Some(3), + unresolved_threads: Some(0), + walkthrough_clean: false, + }; + let rl = pr309_incident_rate_limit(); + + let (_, action) = decide(&ci, &cr, Some(&rl)); + + assert_eq!(action, "action_required"); + } + + /// R4 backstop: rate-limit を **検出できなかった** 場合でも、レビュー実施の + /// 陽性証拠が無い限り `stop_monitoring_success` を出さない。 + /// + /// CR が marker 文言自体を変えて `parse_rate_limit` が沈黙しても、 + /// commit status の pass だけで success を確定しないことを固定する。 + #[test] + fn decide_without_review_evidence_does_not_report_success() { + let ci = ci_no_runs("success"); + let cr = CodeRabbitStatus { + review_state: "success".to_string(), + new_comments: 0, + actionable_comments: None, + unresolved_threads: Some(0), + walkthrough_clean: false, + }; + + let (status, action) = decide(&ci, &cr, None); + + assert_eq!(status, "pending"); + assert_eq!( + action, "continue_monitoring", + "commit status の pass だけでは「レビュー済み・指摘なし」と確定できない" + ); + } + + /// R4 の陽性証拠として `actionable_comments = Some(0)` が有効であること + /// (「レビューして 0 件だった」= レビューは走った) を単独で固定する。 + #[test] + fn decide_actionable_zero_counts_as_review_evidence() { + let ci = ci_no_runs("success"); + let cr = CodeRabbitStatus { + review_state: "success".to_string(), + new_comments: 0, + actionable_comments: Some(0), + unresolved_threads: Some(0), + walkthrough_clean: false, + }; + + let (status, action) = decide(&ci, &cr, None); + + assert_eq!(status, "complete"); + assert_eq!(action, "stop_monitoring_success"); + } + + /// R1: summary の CR 部分が rate-limit 中に「指摘なし」と断定しないこと。 + #[test] + fn summary_rate_limited_does_not_claim_no_findings() { + let ci = ci_no_runs("success"); + let cr = pr309_incident_cr_status(); + let rl = pr309_incident_rate_limit(); + + let summary = build_summary(&ci, &cr, Some(&rl)); + + assert!( + summary.contains("レート制限中"), + "rate-limit 中であることを summary に明示すべき: {}", + summary + ); + assert!( + !summary.contains("指摘なし"), + "レビュー未実施なのに「指摘なし」と断定してはならない: {}", + summary + ); + } + #[test] fn decide_ci_pending() { let ci = CiStatus { @@ -132,7 +362,7 @@ mod tests { review_state: "success".to_string(), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "pending"); assert_eq!(action, "continue_monitoring"); } @@ -147,7 +377,7 @@ mod tests { review_state: "pending".to_string(), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "pending"); assert_eq!(action, "continue_monitoring"); } @@ -162,7 +392,7 @@ mod tests { review_state: "not_found".to_string(), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "pending"); assert_eq!(action, "continue_monitoring"); } @@ -180,7 +410,7 @@ mod tests { review_state: "success".to_string(), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "error"); assert_eq!(action, "stop_monitoring_failure"); } @@ -198,7 +428,7 @@ mod tests { unresolved_threads: Some(0), walkthrough_clean: false, }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "action_required"); assert_eq!(action, "action_required"); } @@ -216,7 +446,7 @@ mod tests { unresolved_threads: Some(3), walkthrough_clean: false, }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "action_required"); assert_eq!(action, "action_required"); } @@ -234,7 +464,7 @@ mod tests { unresolved_threads: Some(0), walkthrough_clean: false, }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "action_required"); assert_eq!(action, "action_required"); } @@ -252,7 +482,7 @@ mod tests { unresolved_threads: Some(0), walkthrough_clean: false, }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "complete"); assert_eq!(action, "stop_monitoring_success"); } @@ -267,7 +497,7 @@ mod tests { review_state: "failure".to_string(), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "error"); assert_eq!(action, "stop_monitoring_failure"); } @@ -285,7 +515,7 @@ mod tests { unresolved_threads: Some(3), walkthrough_clean: false, }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "action_required"); assert_eq!(action, "action_required"); } @@ -303,7 +533,7 @@ mod tests { unresolved_threads: Some(0), walkthrough_clean: false, }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "complete"); assert_eq!(action, "stop_monitoring_success"); } @@ -318,7 +548,7 @@ mod tests { review_state: "not_found".to_string(), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "pending"); assert_eq!(action, "continue_monitoring"); } @@ -336,7 +566,7 @@ mod tests { unresolved_threads: Some(0), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "complete"); assert_eq!(action, "stop_monitoring_success"); } @@ -354,7 +584,7 @@ mod tests { unresolved_threads: Some(2), ..Default::default() }; - let (status, action) = decide(&ci, &cr); + let (status, action) = decide(&ci, &cr, None); assert_eq!(status, "action_required"); assert_eq!(action, "action_required"); } @@ -372,7 +602,7 @@ mod tests { unresolved_threads: Some(0), walkthrough_clean: false, }; - let summary = build_summary(&ci, &cr); + let summary = build_summary(&ci, &cr, None); assert!(summary.contains("CI成功")); assert!(summary.contains("指摘なし")); } @@ -387,7 +617,7 @@ mod tests { }], }; let cr = CodeRabbitStatus::default(); - let summary = build_summary(&ci, &cr); + let summary = build_summary(&ci, &cr, None); assert!(summary.contains("CI失敗")); assert!(summary.contains("test")); } @@ -405,7 +635,7 @@ mod tests { unresolved_threads: Some(1), walkthrough_clean: false, }; - let summary = build_summary(&ci, &cr); + let summary = build_summary(&ci, &cr, None); assert!(summary.contains("新規指摘3件")); assert!(summary.contains("未解決スレッド1件")); } diff --git a/src/check-ci-coderabbit/src/main.rs b/src/check-ci-coderabbit/src/main.rs index fbacafca..d670431b 100644 --- a/src/check-ci-coderabbit/src/main.rs +++ b/src/check-ci-coderabbit/src/main.rs @@ -292,8 +292,8 @@ fn run_check(args: CliArgs) -> CheckResult { }; let findings = fetch_findings(&repo, pr, &args.push_time); - let (status, action) = decide(&ci, &cr); - let summary = build_summary(&ci, &cr); + let (status, action) = decide(&ci, &cr, rate_limit.as_ref()); + let summary = build_summary(&ci, &cr, rate_limit.as_ref()); CheckResult { status, diff --git a/src/check-ci-coderabbit/src/models.rs b/src/check-ci-coderabbit/src/models.rs index 8dbde37e..c2c1c3e8 100644 --- a/src/check-ci-coderabbit/src/models.rs +++ b/src/check-ci-coderabbit/src/models.rs @@ -28,6 +28,19 @@ pub(crate) struct RateLimitInfo { pub(crate) comment_event_time: String, pub(crate) wait_minutes: u64, pub(crate) wait_seconds: u64, + /// `wait_minutes` / `wait_seconds` を既知書式から実際に読めたか。 + /// + /// `false` = marker だけ一致した未知書式で、待機時間は既定値 + /// ([`crate::rate_limit::UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES`])。 + /// + /// **本 field は出力 JSON 上の観測用で、消費するコードは無い**。 + /// cli-pr-monitor の `RateLimitState` は本 field を持たず (typed 化すると + /// 全 struct literal の更新が必要になる一方、得られるのは park summary の + /// 文言精度という副次的な利得のため見送った)、既定値適用を運用者に伝える + /// 経路は [`crate::rate_limit::parse_rate_limit`] が出す stderr 警告 + /// (monitor がログ転送する) が担う。値自体は monitor が保持する checker の + /// 生 JSON (`check_output`) から参照できる。 + pub(crate) wait_time_parsed: bool, } #[derive(Serialize, Default)] diff --git a/src/check-ci-coderabbit/src/rate_limit.rs b/src/check-ci-coderabbit/src/rate_limit.rs index fea0def5..e7eac4ad 100644 --- a/src/check-ci-coderabbit/src/rate_limit.rs +++ b/src/check-ci-coderabbit/src/rate_limit.rs @@ -3,6 +3,15 @@ use crate::markers::{is_rate_limit_comment, rate_limit_event_time}; use crate::models::{GhComment, RateLimitInfo}; +/// marker は一致したが待機時間を既知書式で読めなかったときに使う既定待機時間 (分)。 +/// +/// CR は rate-limit comment の書式を過去 2 回変更しており (ADR-034)、書式追随は +/// 常に後追いになる。待機時間が読めないことを「rate-limit ではない」と扱うと +/// silent success に戻るため、marker 一致を制限の根拠として採用し、待機時間だけを +/// 保守的な既定値で埋める。値が実際の reset より短ければ wakeup 後に再検出されて +/// 再度 park されるだけ (retry は `max_retries` で有界) なので安全側に倒れる。 +pub(crate) const UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES: u64 = 30; + /// CodeRabbit rate-limit comment を検出し、reset 時刻 (unix epoch) を返す。 pub(crate) fn parse_rate_limit(json: &str, push_time: &str) -> Option { let comments: Vec = serde_json::from_str(json).ok()?; @@ -32,7 +41,10 @@ pub(crate) fn parse_rate_limit(json: &str, push_time: &str) -> Option Option (u64, u64, bool) { + match extract_wait_time(body) { + Some((minutes, seconds)) => (minutes, seconds, true), + None => (UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES, 0, false), + } +} + /// 旧 format (`Please wait **N minutes? and M seconds?**`) を抽出。 pub(crate) fn extract_old_format_wait_time(body: &str) -> Option<(u64, u64)> { let re_full = regex::Regex::new(r"Please wait \*?\*?(\d+) minutes? and (\d+) seconds?").ok()?; @@ -76,9 +113,31 @@ pub(crate) fn extract_new_format_wait_time(body: &str) -> Option<(u64, u64)> { Some((m, 0)) } -/// 旧 / 新どちらかの format に一致すれば `(minutes, seconds)` を返す。旧 → 新の順で試行。 +/// 第 3 世代 format (`**Next review available in:** **N minutes**`) を抽出。 +/// +/// PR #309 (2026-07-20) で実観測。ラベルと数値の間に markdown の `**` と `:` が +/// 挟まるため、区切りは `[:*\s]*` で吸収する (CR が強調記法を変えても壊れにくい)。 +pub(crate) fn extract_next_review_format_wait_time(body: &str) -> Option<(u64, u64)> { + let re_full = regex::Regex::new( + r"Next review available in[:*\s]*(\d+) minutes?[*\s]*and[:*\s]*(\d+) seconds?", + ) + .ok()?; + if let Some(caps) = re_full.captures(body) { + let m: u64 = caps.get(1)?.as_str().parse().ok()?; + let s: u64 = caps.get(2)?.as_str().parse().ok()?; + return Some((m, s)); + } + let re_min = regex::Regex::new(r"Next review available in[:*\s]*(\d+) minutes?").ok()?; + let caps = re_min.captures(body)?; + let m: u64 = caps.get(1)?.as_str().parse().ok()?; + Some((m, 0)) +} + +/// 既知 3 世代のいずれかに一致すれば `(minutes, seconds)` を返す。古い世代から順に試行。 pub(crate) fn extract_wait_time(body: &str) -> Option<(u64, u64)> { - extract_old_format_wait_time(body).or_else(|| extract_new_format_wait_time(body)) + extract_old_format_wait_time(body) + .or_else(|| extract_new_format_wait_time(body)) + .or_else(|| extract_next_review_format_wait_time(body)) } /// ISO 8601 (`YYYY-MM-DDTHH:MM:SSZ` 形式) を unix epoch 秒に変換する。 @@ -143,6 +202,45 @@ pub(crate) fn days_in_month_check(year: i64, month: i64) -> i64 { mod tests { use super::*; + /// PR #309 の実 rate-limit comment body (2026-07-20T12:10:47Z 投稿) の忠実な抜粋。 + /// + /// 出典: `gh api repos/aloekun/claude-code-hook-test/issues/309/comments`。 + /// この incident が「CR 書式変更で rate-limit 検知が沈黙する」2 度目の regression + /// (PR #182/#184 に次ぐ) を起こした実入力そのもの。ADR-049 に従い合成データでは + /// なく実データを fixture 化する。 + /// + /// 構造上の要点は 2 つ: + /// - walkthrough header marker (`summarize by coderabbit.ai`) を **同一 comment 内に** + /// 併せ持つ。`is_clean_walkthrough_comment` が rate-limit comment を除外していなければ + /// clean walkthrough と誤認され得る配置。 + /// - 待機時間が第 3 世代書式 (`**Next review available in:** **57 minutes**`)。 + const PR309_RATE_LIMIT_BODY: &str = r#" + + +[![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/aloekun/claude-code-hook-test/pull/309) + + + + +> [!WARNING] +> ## Review limit reached +> +> `@aloekun`, you've reached your PR review limit, so we couldn't start this review. +> +> **Next review available in:** **57 minutes** +> +> Enable **usage-based reviews** in Billing to review now."#; + + /// PR #309 の実 comment を GH API の comments JSON 形として組み立てる。 + fn pr309_comments_json() -> String { + serde_json::json!([{ + "user": {"login": "coderabbitai[bot]"}, + "body": PR309_RATE_LIMIT_BODY, + "created_at": "2026-07-20T12:10:47Z" + }]) + .to_string() + } + #[test] fn iso8601_epoch_zero() { assert_eq!(parse_iso8601_to_unix("1970-01-01T00:00:00Z"), Some(0)); @@ -239,14 +337,44 @@ mod tests { assert!(parse_rate_limit(json, "2026-04-29T00:00:00Z").is_none()); } + /// R2: marker は一致するが待機時間が既知 3 世代のどれにも一致しない未知書式でも、 + /// rate-limit として検出し続ける (旧実装は `None` を返し監視が silent success に倒れた)。 + /// 待機時間は既定値で埋め、`wait_time_parsed = false` で「読めなかった」ことを明示する。 #[test] - fn rate_limit_no_match_when_no_wait_time() { + fn rate_limit_falls_back_to_default_wait_when_format_unknown() { let json = r#"[{ "user": {"login": "coderabbitai[bot]"}, "body": "Rate limit exceeded but format is unusual", "created_at": "2026-04-30T00:00:00Z" }]"#; - assert!(parse_rate_limit(json, "2026-04-29T00:00:00Z").is_none()); + let result = parse_rate_limit(json, "2026-04-29T00:00:00Z") + .expect("marker 一致時は未知書式でも rate-limit として検出すべき"); + assert_eq!(result.wait_minutes, UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES); + assert_eq!(result.wait_seconds, 0); + assert!( + !result.wait_time_parsed, + "未知書式では既定値であることを wait_time_parsed=false で申告する" + ); + let base = parse_iso8601_to_unix("2026-04-30T00:00:00Z").unwrap(); + assert_eq!( + result.until_unix_secs, + base + (UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES as i64) * 60 + 60 + ); + } + + /// R2 の fallback が「marker 無しの comment まで rate-limit 扱いする」方向に + /// 効きすぎていないことを固定する (fallback の適用範囲は marker 一致後のみ)。 + #[test] + fn rate_limit_fallback_does_not_fire_without_marker() { + let json = r#"[{ + "user": {"login": "coderabbitai[bot]"}, + "body": "Next review available in: 57 minutes", + "created_at": "2026-04-30T00:00:00Z" + }]"#; + assert!( + parse_rate_limit(json, "2026-04-29T00:00:00Z").is_none(), + "marker 非一致なら待機時間書式に一致しても rate-limit ではない" + ); } #[test] @@ -351,6 +479,58 @@ mod tests { assert_eq!(result.wait_seconds, 0); } + /// R2 / incident 再現: PR #309 の実 comment から第 3 世代書式の待機時間を読む。 + /// 旧実装ではここが `None` に倒れ、`decide()` に rate_limit が届かず silent success + /// になっていた (本 incident の起点)。 + #[test] + fn rate_limit_detected_from_pr309_incident_body() { + let result = parse_rate_limit(&pr309_comments_json(), "2026-07-20T12:00:00Z") + .expect("PR #309 の実 rate-limit comment は検出されなければならない"); + assert_eq!(result.wait_minutes, 57); + assert_eq!(result.wait_seconds, 0); + assert!( + result.wait_time_parsed, + "第 3 世代書式は既知書式として読めるので fallback ではない" + ); + assert_eq!(result.comment_event_time, "2026-07-20T12:10:47Z"); + let base = parse_iso8601_to_unix("2026-07-20T12:10:47Z").unwrap(); + assert_eq!(result.until_unix_secs, base + 57 * 60 + 60); + } + + /// PR #309 の実 comment は walkthrough header marker を併せ持つが、rate-limit + /// comment である限り clean walkthrough と判定してはならない。 + /// (この排他が崩れると `decide()` が walkthrough_clean で早期 success に倒れる) + #[test] + fn pr309_incident_body_is_not_treated_as_clean_walkthrough() { + let clean = crate::parsers::parse_walkthrough_clean_marker( + &pr309_comments_json(), + "2026-07-20T12:00:00Z", + ); + assert!( + !clean, + "rate-limit comment は walkthrough header を含んでも clean 扱いしない" + ); + } + + #[test] + fn wait_time_next_review_format_minutes_only() { + let body = "**Next review available in:** **57 minutes**"; + assert_eq!(extract_wait_time(body), Some((57, 0))); + } + + #[test] + fn wait_time_next_review_format_with_seconds() { + let body = "**Next review available in:** **3 minutes and 20 seconds**"; + assert_eq!(extract_wait_time(body), Some((3, 20))); + } + + /// 強調記法が付かない素の書式でも読めること (CR が markdown を変えても壊れない)。 + #[test] + fn wait_time_next_review_format_without_markdown_emphasis() { + let body = "Next review available in 12 minutes."; + assert_eq!(extract_wait_time(body), Some((12, 0))); + } + #[test] fn rate_limit_picks_latest_when_mixed_old_and_new_formats() { let json = r#"[ diff --git a/src/cli-pr-monitor/src/stages/monitor.rs b/src/cli-pr-monitor/src/stages/monitor.rs index 7988bc66..1dcc108b 100644 --- a/src/cli-pr-monitor/src/stages/monitor.rs +++ b/src/cli-pr-monitor/src/stages/monitor.rs @@ -313,23 +313,55 @@ fn print_report(result: &crate::stages::poll::PollResult, pr_label: &str) { } } -fn compute_verdict(result: &crate::stages::poll::PollResult) -> &'static str { +/// 人間向けの判定文を組み立てる。 +/// +/// 「問題は見つかりませんでした」は **findings が空**かつ**未確定要素が残って +/// いない**ときにだけ出す。findings が空であることは「見るべきものが無かった」 +/// の十分条件ではなく、レート制限でレビューが走っていない / 未解決スレッドが +/// 残っている場合でも空になり得る (PR #307/#309 実観測: 「未解決スレッド2件」を +/// 表示しながら同一レポートで「問題は見つかりませんでした」と断定していた)。 +/// +/// 判定順は「未確定 → 重大 → 未解決 → 軽微 → 問題なし」。未確定要素は findings の +/// 有無より先に評価し、断定文へ落ちる経路を構造的に塞ぐ。 +fn compute_verdict(result: &crate::stages::poll::PollResult) -> String { + verdict_for_unsettled_review(result).unwrap_or_else(|| verdict_for_findings(result)) +} + +/// レビューがまだ確定していない場合の保留判定文を返す。確定済みなら `None`。 +/// +/// findings の中身を見る前に評価する。ここを通過して初めて +/// [`verdict_for_findings`] の断定文 (「問題は見つかりませんでした」等) を出せる。 +fn verdict_for_unsettled_review(result: &crate::stages::poll::PollResult) -> Option { match result.action.as_str() { "parked_rate_limit" => { - return "CodeRabbit rate-limit のため wakeup を予約 (上記 PARK signal 参照)"; + return Some( + "CodeRabbit rate-limit のため wakeup を予約 (上記 PARK signal 参照)".to_string(), + ); } "parked_review_recheck" => { - return "review 完了待ちのため wakeup を予約 (上記 PARK signal 参照)"; + return Some( + "review 完了待ちのため wakeup を予約 (上記 PARK signal 参照)".to_string(), + ); } _ => {} } - if let Some(cr) = &result.coderabbit { - if cr.review_state == "not_found" || cr.review_state == "pending" { - return "CodeRabbit review が未完了のため、判定を保留します"; - } + if result.rate_limit.is_some() { + return Some( + "CodeRabbit がレート制限中でレビューが実施されていないため、判定を保留します" + .to_string(), + ); } + let cr = result.coderabbit.as_ref()?; + if cr.review_state == "not_found" || cr.review_state == "pending" { + return Some("CodeRabbit review が未完了のため、判定を保留します".to_string()); + } + None +} + +/// レビュー確定後の判定文を findings と未解決スレッドから組み立てる。 +fn verdict_for_findings(result: &crate::stages::poll::PollResult) -> String { let critical_major = result .findings .iter() @@ -340,12 +372,25 @@ fn compute_verdict(result: &crate::stages::poll::PollResult) -> &'static str { .count(); if critical_major > 0 { - "修正が必要な指摘があります" - } else if !result.findings.is_empty() { - "重大な問題は見つかりませんでした。軽微な改善提案があります" - } else { - "問題は見つかりませんでした" + return "修正が必要な指摘があります".to_string(); + } + + let unresolved_threads = result + .coderabbit + .as_ref() + .and_then(|c| c.unresolved_threads) + .unwrap_or(0); + if unresolved_threads > 0 { + return format!( + "未解決スレッドが{}件残っているため、判定を保留します", + unresolved_threads + ); + } + + if !result.findings.is_empty() { + return "重大な問題は見つかりませんでした。軽微な改善提案があります".to_string(); } + "問題は見つかりませんでした".to_string() } fn print_findings_table(findings: &[lib_report_formatter::Finding]) { @@ -593,6 +638,89 @@ mod tests { assert_eq!(compute_verdict(&r), VERDICT_NO_PROBLEMS); } + /// R3 用: 未解決スレッド数と rate-limit を指定できる `PollResult` を組む。 + fn poll_result_with_unsettled( + unresolved_threads: Option, + rate_limit: Option, + findings: Vec, + ) -> PollResult { + PollResult { + action: "stop_monitoring_success".into(), + summary: "test".into(), + ci: None, + coderabbit: Some(CodeRabbitState { + review_state: "success".into(), + new_comments: 0, + actionable_comments: None, + unresolved_threads, + }), + findings, + check_output: None, + rate_limit, + } + } + + /// PR #309 incident の rate-limit 情報 (第 3 世代書式、57 分待機)。 + fn pr309_rate_limit() -> crate::state::RateLimitState { + crate::state::RateLimitState { + until_unix_secs: 1_784_556_707, + comment_event_time: "2026-07-20T12:10:47Z".into(), + wait_minutes: 57, + wait_seconds: 0, + } + } + + /// R3 (incident 再現): rate-limit 中は findings が空でも「問題なし」と断定しない。 + /// + /// findings が空なのは「レビューして何も無かった」からではなく「レビューが + /// 走っていない」からであり、両者を判定文で区別する必要がある。 + #[test] + fn verdict_holds_when_rate_limit_present_even_with_no_findings() { + let r = poll_result_with_unsettled(Some(0), Some(pr309_rate_limit()), vec![]); + let verdict = compute_verdict(&r); + assert!( + verdict.contains("レート制限"), + "rate-limit 中であることを判定文に出すべき: {}", + verdict + ); + assert_ne!(verdict, VERDICT_NO_PROBLEMS); + } + + /// R3 (incident 再現): 「未解決スレッド2件」を表示しながら同一レポートで + /// 「問題は見つかりませんでした」と断定していた矛盾を塞ぐ (PR #307/#309 実観測)。 + #[test] + fn verdict_holds_when_unresolved_threads_remain_with_no_findings() { + let r = poll_result_with_unsettled(Some(2), None, vec![]); + let verdict = compute_verdict(&r); + assert_eq!(verdict, "未解決スレッドが2件残っているため、判定を保留します"); + } + + /// findings が Minor のみでも、未解決スレッドが残っていれば + /// 「重大な問題は見つかりませんでした」と断定しない。 + #[test] + fn verdict_holds_when_unresolved_threads_remain_with_minor_findings() { + let r = poll_result_with_unsettled(Some(1), None, vec![finding("minor")]); + let verdict = compute_verdict(&r); + assert_ne!(verdict, VERDICT_MINOR); + assert!(verdict.contains("未解決スレッドが1件"), "verdict={}", verdict); + } + + /// 重大な指摘がある場合は、未解決スレッドより「修正が必要」を優先する + /// (どちらも行動を促す文言だが、より強い方を出す)。 + #[test] + fn verdict_critical_takes_precedence_over_unresolved_threads() { + let r = poll_result_with_unsettled(Some(2), None, vec![finding("critical")]); + assert_eq!(compute_verdict(&r), VERDICT_CRITICAL); + } + + /// 新 guard が効きすぎないこと: 未解決スレッド 0 件・rate-limit なし・findings 空 + /// なら従来どおり「問題は見つかりませんでした」を出す。 + #[test] + fn verdict_no_problems_when_no_unsettled_signals_remain() { + let r = poll_result_with_unsettled(Some(0), None, vec![]); + assert_eq!(compute_verdict(&r), VERDICT_NO_PROBLEMS); + } + /// 順位 141: `resume_fix_push_time_or_started_at` Case A — /// state に `fix_push_time` が設定済みの場合、fallback の `started_at` ではなく /// state の値が返されることを検証する。