diff --git a/.github/workflows/review-request.yml b/.github/workflows/review-request.yml index 8302c908..d1ad2c7c 100644 --- a/.github/workflows/review-request.yml +++ b/.github/workflows/review-request.yml @@ -226,13 +226,15 @@ jobs: # 2026-08-11 の再要求で実観測)。この文言は markers.rs の RATE_LIMIT_MARKERS # (`Rate limit exceeded` / `rate limited by coderabbit.ai`) の**どちらにも一致しない**。 # markers.rs が見ているのは walkthrough comment の placeholder であって command ack - # ではなく、ack は本 workflow だけが観測する comment class だからである。よって本 - # workflow の rate-limit marker は markers.rs の**上位集合**になる: - # - `Rate limit exceeded` / `rate limited by coderabbit.ai` … markers.rs と共有。 - # walkthrough が placeholder として投稿された場合を拾う。触るときは両方直す - # (片方だけ直すと本 workflow だけが silent success に戻る。 - # ADR-034 § CR rate-limit format evolution)。 - # - `Review rate limited.` … command ack 専用。本 workflow 固有。 + # ではない。当初は「ack は本 workflow だけが観測する comment class」として本 workflow + # 固有の marker にしたが、**その前提が誤り**だった: placeholder は同じコメントが後から + # 実レビュー本文へ編集されるため marker が消え、ack だけが残る窓がある (PR #412 / + # #387 の実データ)。よって ack 文言も markers.rs へ入れ、**3 marker とも共有**にした。 + # - `Rate limit exceeded` / `rate limited by coderabbit.ai` … placeholder 側。 + # - `Review rate limited.` … command ack 側。受理時は `Review finished.`。 + # 触るときは markers.rs と同時に直す (片方だけ直すと直さなかった層が silent success に + # 戻る。ADR-034 § CR rate-limit format evolution)。同期は + # `scripts/lint-workflows.mjs` の CodeRabbit marker 検査が機械的に見る。 # # **rate-limit を陽性証拠より先に見る。** ack が拒否と言っているのに walkthrough # marker を持つコメント (placeholder) が同時に存在しうる — #387 では 3 秒差で diff --git a/docs/adr/adr-034-coderabbit-auto-monitoring.md b/docs/adr/adr-034-coderabbit-auto-monitoring.md index 7817d4f1..f8f6a832 100644 --- a/docs/adr/adr-034-coderabbit-auto-monitoring.md +++ b/docs/adr/adr-034-coderabbit-auto-monitoring.md @@ -74,6 +74,17 @@ 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 強調が挟まるため区切りを文字クラスで吸収) | +| 2026-08-20 判明 (**別 comment class**、PR #387/#412/#427 の実データ) | `Review rate limited.` (command ack = `` を持つ auto-generated reply。`Action not completed` の details ブロック内) | **無し** — ack は待ち時間を書かない。`UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES` (30 分) に倒れる | + +**2 つの comment class を混同しない (2026-08-20 追加)**: 上表の第 1〜3 世代は **walkthrough comment が placeholder として投稿されたとき**の marker、第 4 世代は **`@coderabbitai review` への command ack** の marker で、body の語彙が全く別 (ack は `rate limited by coderabbit.ai` を含まない)。**「同じ SaaS だから同じ marker で拾える」と仮定してはならない。** + +発見の経緯は、順位 431 (review-request の success 判定) を実装する際に `markers.rs` の marker をそのまま流用しようとして、PR #387 の生 body を `gh api` で読んで一致しないことに気づいたもの。読まずに land していれば「レート制限を検知できない検知機構」ができていた。 + +**ack を marker 集合に入れる理由**: placeholder は**同じコメントが後から実レビュー本文へ編集される**ため marker が消える。一方 ack は要求 1 回につき 1 コメントが残る。実データ (2026-08-20 に PR #340〜#428 を機械集計) では、**#412 が ack 3 件 / placeholder marker 0 件**、#387 が 1 件 / 0 件。この窓では ack だけが唯一の証拠になる。影響は (a) park / 再 trigger 経路に入らず polling を続ける、(b) 事後の棚卸しでレート制限を過少計数する、の 2 つ (silent success には**ならない** — ADR-064 の陽性証拠 gate が別途効く)。 + +**共存時はどちらを採るか (2026-08-20 追加、PR #429 CodeRabbit Major)**: ack と placeholder は同じ拒否に対して**数秒差で両方投稿される**ことがある (PR #427 で 3 秒差)。素朴に「event time が最新の候補」を採ると、placeholder の直後に ack が `updated_at` を更新した場合に ack が選ばれ、**読める待機時間を捨てて 30 分 fallback に落ちる**。PR #387 の実データがこの形 (placeholder created 18:22:49 / ack updated 18:22:51)。 + +そこで `parse_rate_limit` は、最新候補が待機時間を持たない場合に限り、**同一 event 窓 (`SAME_EVENT_WINDOW_SECS` = 120 秒) 内で待機時間を持つ候補**を優先する。**窓を切るのが要点**で、窓なしに優先すると数十分前の解け済み placeholder を新しい拒否 ack より優先し、既に過ぎた reset 時刻を返して park が効かず即再試行 → 再拒否で `max_retries` を浪費する。 **未知書式の 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」の経路は構造的に閉じ、書式追随 (下記手順) は待機時間精度の改善に格下げされる。 diff --git a/docs/bugfix-batch-plan.md b/docs/bugfix-batch-plan.md index 56a18708..a7cc6853 100644 --- a/docs/bugfix-batch-plan.md +++ b/docs/bugfix-batch-plan.md @@ -27,6 +27,8 @@ | K | fix(subprocess): timeout の孫プロセス穴を塞ぐ | 323 | 未着手 | | L | fix(automation): 自動化経路の小穴・ノイズ修正束 | 467 + 181 | 未着手 | +**挿入 (2026-08-20)**: 順位 431 の調査で `markers.rs` の rate-limit marker が CodeRabbit の **command ack 形式**を拾わないことが判明した (PR #412 / #387 の実データ)。検出層の穴なので **PR F を保留して先に別 PR で処理する** (ユーザー判断)。台帳の前提が変わった項目に着手したら、周辺への影響まで確認する — 本計画の 9 件中 7 件でずれが出ている以上、ずれの周辺は常に疑う。 + 消化順は A → L の表の順。**PR をスタックしない** — 順位 376 (PR H で修正するまで push-runner の bookmark 自動前進がスタック境界を壊す既知バグ) を踏むため、各 PR は前の PR がマージされてから master 起点で作る。 ## 共通の運用ルール diff --git a/scripts/lint-workflows.mjs b/scripts/lint-workflows.mjs index ef2662a1..5fd9aee4 100644 --- a/scripts/lint-workflows.mjs +++ b/scripts/lint-workflows.mjs @@ -110,9 +110,11 @@ if (prMonitor) { // **1 か所だけ追随すると残りが黙って壊れる**。ここで「全員が同じ文字列を持っている」 // ことだけを固定する (どの層がどう使うかは各ファイルのコメントが持つ)。 // -// 注: `Review rate limited.` (command ack の文言) は review-request.yml 専用で、 -// markers.rs には無い。markers.rs が見ているのは walkthrough comment の placeholder で -// あって command ack ではないため — 意図的な非共有であり、ここでは検査しない。 +// `Review rate limited.` は当初 review-request.yml 専用だったが、同じ穴が +// markers.rs 側にもあることが実データで判明したため (PR #412 / #387) 共有 marker へ +// 格上げした。ack は placeholder と別 comment class だが、**どちらの層も同じ +// 「レート制限で拒否された」事実を判定している**ので、片方だけ追随すると再び +// 非対称に戻る。 const SHARED_CR_MARKERS = [ { marker: 'rate limited by coderabbit.ai', @@ -122,6 +124,10 @@ const SHARED_CR_MARKERS = [ marker: 'Rate limit exceeded', files: [join(WORKFLOW_DIR, 'review-request.yml'), MARKERS_RS], }, + { + marker: 'Review rate limited.', + files: [join(WORKFLOW_DIR, 'review-request.yml'), MARKERS_RS], + }, { marker: '', files: [join(WORKFLOW_DIR, 'review-request.yml'), join(WORKFLOW_DIR, 'pr-monitor.yml'), MARKERS_RS], diff --git a/src/check-ci-coderabbit/src/markers.rs b/src/check-ci-coderabbit/src/markers.rs index c6db281b..ab6c4e4e 100644 --- a/src/check-ci-coderabbit/src/markers.rs +++ b/src/check-ci-coderabbit/src/markers.rs @@ -7,8 +7,25 @@ use crate::models::GhComment; /// CR は format を時間経過で変更するため multi-variant 配列で対応する /// (PR #182/#184 で silent regression を実体観測)。詳細は ADR-034 § CR rate-limit /// format evolution 参照。 -pub(crate) const RATE_LIMIT_MARKERS: &[&str] = - &["Rate limit exceeded", "rate limited by coderabbit.ai"]; +/// +/// **2 つの comment class が混在する。** 前 2 つは walkthrough comment が +/// placeholder として投稿されたときの marker で、3 つ目は `@coderabbitai review` +/// への **command ack** (auto-generated reply) の拒否文言。両者は body の語彙が +/// 全く別で、ack 側は `rate limited by coderabbit.ai` を含まない。 +/// +/// **ack を入れる理由 (2026-08-20 追加)**: placeholder は**同じコメントが後から +/// 実レビュー本文へ編集される**ため marker が消える。一方 ack は要求 1 回につき +/// 1 コメントが残る。placeholder が出ない / 既に書き換わった時点では、ack だけが +/// 唯一の証拠になる。実データで PR #412 (ack 3 件 / placeholder marker 0 件) と +/// PR #387 (同 1 件 / 0 件) を確認した。ack を持たない旧実装はこの窓で +/// rate-limit を検出できず、park / 再 trigger 経路に入らないまま polling を +/// 続けていた (silent success ではない — ADR-064 の陽性証拠 gate が別途効く)。 +pub(crate) const RATE_LIMIT_MARKERS: &[&str] = &[ + "Rate limit exceeded", + "rate limited by coderabbit.ai", + // command ack の拒否文言。受理時は "Review finished." になるため衝突しない。 + "Review rate limited.", +]; /// 順位 208: CR walkthrough comment が clean 判定を示すときに body に含まれる marker。 pub(crate) const WALKTHROUGH_CLEAN_MARKER: &str = diff --git a/src/check-ci-coderabbit/src/rate_limit.rs b/src/check-ci-coderabbit/src/rate_limit.rs index e7eac4ad..3dfc7c75 100644 --- a/src/check-ci-coderabbit/src/rate_limit.rs +++ b/src/check-ci-coderabbit/src/rate_limit.rs @@ -37,10 +37,11 @@ pub(crate) fn parse_rate_limit(json: &str, push_time: &str) -> Option Option bool { + c.body + .as_deref() + .map(|b| resolve_wait_time(b).2) + .unwrap_or(false) +} + +/// **同一 rate-limit event の中では、待機時間を持つ候補を優先する。** +/// +/// 候補は event time の降順で渡る。素朴に最新を採ると、placeholder (待機時間あり) の +/// 直後に command ack (待機時間なし) が更新された場合に ack が選ばれ、読める +/// `12 minutes and 30 seconds` を捨てて 30 分 fallback に落ちる。ack は +/// `updated_at` を持つため実際に起こる (PR #387: placeholder 18:22:49 / +/// ack updated 18:22:51)。 +/// +/// **窓を切るのが要点**。窓なしで「待機時間を持つ候補」を無条件に優先すると、 +/// 数十分前の解け済み placeholder を新しい拒否 ack より優先してしまう。 +fn prefer_candidate_with_wait_time<'a>( + candidates: &[&'a GhComment], + latest: &'a GhComment, +) -> &'a GhComment { + if has_known_wait_time(latest) { + return latest; + } + let Some(latest_unix) = rate_limit_event_time(latest).and_then(parse_iso8601_to_unix) else { + return latest; + }; + candidates + .iter() + .copied() + .find(|c| { + has_known_wait_time(c) + && rate_limit_event_time(c) + .and_then(parse_iso8601_to_unix) + .map(|t| (latest_unix - t).abs() <= SAME_EVENT_WINDOW_SECS) + .unwrap_or(false) + }) + .unwrap_or(latest) +} + /// 未知書式で既定値を適用したことを stderr に警告する。 /// /// cli-pr-monitor は checker の stderr をログ転送するため、既定値で埋めた事実が @@ -451,6 +501,140 @@ mod tests { ); } + /// command ack の拒否 (第 4 世代 marker、2026-08-20 追加) を検出すること。 + /// + /// body は PR #387 のコメント id=5244249470 (2026-08-10) の実データ。placeholder 側の + /// marker (rate limited by coderabbit.ai) を**一切含まない**のが要点で、旧実装は + /// これを rate-limit と認識できなかった。 + /// + /// ack には待ち時間が書かれないため、wait time は未知書式 fallback (30 分) に倒れる。 + /// これは ADR-034 § 未知書式の fail-closed fallback が定めた既定の振る舞い。 + #[test] + fn rate_limit_detected_from_command_ack_without_placeholder_markers() { + let json = r#"[{ + "user": {"login": "coderabbitai[bot]"}, + "body": "\n\n
\nAction not completed\n\nReview rate limited.\n\n
", + "created_at": "2026-08-10T18:22:46Z", + "updated_at": "2026-08-10T18:22:51Z" + }]"#; + let result = parse_rate_limit(json, "2026-08-10T18:00:00Z") + .expect("command ack の拒否も rate-limit として検出すること"); + assert!( + !result.wait_time_parsed, + "ack には待ち時間が無いので未知書式 fallback に倒れる" + ); + assert_eq!(result.wait_minutes, UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES); + } + + /// **受理の ack を rate-limit と誤検出しないこと。** 拒否と受理は同じ auto-generated + /// reply の body で、違いは "Review rate limited." と "Review finished." の 1 行だけ。 + /// body は PR #387 のコメント id=5252180495 の実データ。 + #[test] + fn successful_command_ack_is_not_treated_as_rate_limit() { + let json = r#"[{ + "user": {"login": "coderabbitai[bot]"}, + "body": "\n\n
\nAction performed\n\nReview finished.\n\n
", + "created_at": "2026-08-11T10:51:35Z" + }]"#; + assert!( + parse_rate_limit(json, "2026-08-11T00:00:00Z").is_none(), + "受理 ack を rate-limit と読むと、レビュー済みの PR が park される" + ); + } + + /// ack と placeholder が**両方**投稿される通常ケース (PR #427 の実データ、3 秒差) では、 + /// 待ち時間を持つ placeholder 側が採用されること。 + /// + /// ack marker の追加で「待ち時間の分かる方を捨てて 30 分 fallback に落ちる」退行が + /// 起きないことを固定する。parse_rate_limit は event time の新しい方を採るため、 + /// placeholder が後着する限りこの順序で決まる。 + /// CodeRabbit #429 Major の regression guard: **ack の event time が placeholder より + /// 新しくても**、待機時間を持つ placeholder 側が採られること。 + /// + /// ack は `updated_at` を持つため、これは実際に起きる形である (PR #387 の実データ: + /// placeholder created 18:22:49 / ack updated 18:22:51)。素朴に「最新を採る」と + /// 読める 12 分 30 秒を捨てて 30 分 fallback に落ち、解除時刻が不正確になる。 + #[test] + fn placeholder_wait_time_wins_even_when_ack_is_updated_later() { + let json = r#"[ + { + "user": {"login": "coderabbitai[bot]"}, + "body": "\n
\nAction not completed\n\nReview rate limited.\n\n
", + "created_at": "2026-08-19T18:11:13Z", + "updated_at": "2026-08-19T18:11:18Z" + }, + { + "user": {"login": "coderabbitai[bot]"}, + "body": "\n\n\n> [!WARNING]\n> ## Review limit reached\n>\n> More reviews will be available in 12 minutes and 30 seconds.", + "created_at": "2026-08-19T18:11:16Z" + } + ]"#; + let result = parse_rate_limit(json, "2026-08-19T18:00:00Z") + .expect("ack が後着でも rate-limit として検出すること"); + assert!( + result.wait_time_parsed, + "ack が後着でも、同一 event 内の placeholder の待機時間を採るべき" + ); + assert_eq!(result.wait_minutes, 12); + assert_eq!(result.wait_seconds, 30); + // reset 時刻は placeholder の event time 基準で計算される (絶対時刻なので正しい)。 + let base = parse_iso8601_to_unix("2026-08-19T18:11:16Z").unwrap(); + assert_eq!(result.until_unix_secs, base + 12 * 60 + 30 + 60); + } + + /// 上の優先を**窓で切る**理由の対比: placeholder から十分に離れた ack は + /// **別の拒否 (新しい event)** なので、解け済みの古い待機時間を流用しない。 + /// + /// 流用すると既に過ぎた reset 時刻を返し、park が効かず即再試行 → 再び拒否、で + /// `max_retries` を無駄に消費する。窓を超えたら未知書式 fallback (30 分) に倒す。 + #[test] + fn old_placeholder_wait_time_is_not_reused_for_a_much_later_ack() { + let json = r#"[ + { + "user": {"login": "coderabbitai[bot]"}, + "body": "\n\n\n> [!WARNING]\n> ## Review limit reached\n>\n> More reviews will be available in 12 minutes and 30 seconds.", + "created_at": "2026-08-19T18:11:16Z" + }, + { + "user": {"login": "coderabbitai[bot]"}, + "body": "\n
\nAction not completed\n\nReview rate limited.\n\n
", + "created_at": "2026-08-19T18:40:00Z" + } + ]"#; + let result = parse_rate_limit(json, "2026-08-19T18:00:00Z") + .expect("後の拒否 ack も rate-limit として検出すること"); + assert!( + !result.wait_time_parsed, + "29 分後の ack は別 event。古い placeholder の待機時間を流用しない" + ); + assert_eq!(result.wait_minutes, UNKNOWN_FORMAT_FALLBACK_WAIT_MINUTES); + assert_eq!(result.comment_event_time, "2026-08-19T18:40:00Z"); + } + + #[test] + fn placeholder_wait_time_wins_when_ack_and_placeholder_coexist() { + let json = r#"[ + { + "user": {"login": "coderabbitai[bot]"}, + "body": "\n
\nAction not completed\n\nReview rate limited.\n\n
", + "created_at": "2026-08-19T18:11:13Z" + }, + { + "user": {"login": "coderabbitai[bot]"}, + "body": "\n\n\n> [!WARNING]\n> ## Review limit reached\n>\n> More reviews will be available in 12 minutes and 30 seconds.", + "created_at": "2026-08-19T18:11:16Z" + } + ]"#; + let result = parse_rate_limit(json, "2026-08-19T18:00:00Z") + .expect("placeholder があるケースも従来どおり検出すること"); + assert!( + result.wait_time_parsed, + "待ち時間を持つ placeholder 側を採るべき (ack の 30 分 fallback に落ちない)" + ); + assert_eq!(result.wait_minutes, 12); + assert_eq!(result.wait_seconds, 30); + } + #[test] fn rate_limit_detected_from_new_format_with_html_marker_and_full_wait_time() { let json = r#"[{