fix(test): stop-tool-call-leak E2E の stdin 書き込み競合を解消 (master 赤の修正) - #308
Merged
Conversation
PR #307 マージ後の release-binaries.yml 初回実行 (ubuntu-22.04) が kill_switch_env_skips_check の Broken pipe で失敗し、master が赤になっていた。 release が生成されず cloud-setup.sh の取得対象が不在の状態だったため先行修正する。 原因は test helper の stdin 書き込みが子の即時終了と競合すること。 hooks-stop-tool-call-leak の main は kill-switch (STOP_TOOL_CALL_LEAK_OVERRIDE) と enabled = false の 2 経路で **stdin を読む前に return** する。読み手が消えたパイプへ write_all すると Unix では EPIPE になるが、旧実装は `.expect()` で panic していた。 Windows では小さな payload がバッファに収まり成功しがちなため顕在化していなかった。 BrokenPipe のみ正常として飲み込む helper `write_stdin_tolerating_early_exit` を導入。他の I/O エラーは従来どおり panic させる。 これらの test の主題は「skip されること」であって「stdin が消費されること」ではない。 **検証**: WSL では修正前も 60/60 pass してしまい競合を再現できなかったため、 write の前に 300ms の遅延を注入して EPIPE を確定的に起こす検証を行った。 同一条件で修正前は CI と同じ `Os { code: 32, kind: BrokenPipe }` で FAIL、 修正後は PASS することを実測した (この遅延は検証専用でコミットには含めない)。 あわせて Windows 34+7 pass、Linux 39 スイート全 pass、clippy 両 OS clean。 「WSL で通っても CI で落ちる」実例であり、WP-16 (CI matrix) の必要性を裏づける。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changesstdin早期終了対応
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし (レビュー指摘 0 件) Applicable Findings (Medium 以下)該当なし Filtered (not applicable)該当なし 次のアクション
|
aloekun
added a commit
that referenced
this pull request
Jul 20, 2026
## R5: WP-15 `完了` 条件 (1) の達成記録 PR #307 マージ時の release-binaries.yml run は build job が失敗しており (master が赤で #308 の E2E 修正が必要だった)、成功したのは #308 マージ後の run (commit 541adde)。この経緯も含めて記録した。 生成物は本セッションで再実測している: WSL Ubuntu 24.04 から素の curl で tarball (9,721,643 bytes) + .sha256 を**認証なしで**取得 → sha256sum -c 一致 → 展開して 16 バイナリ + BUILD_INFO 確認 → release バイナリそのもので hooks 実発火 (pre-tool-validate が破壊的削除コマンドを exit 2 でブロックし無害な echo を exit 0 で通す / session-start が additionalContext JSON を出力)。 これで WP-15 ④ の「public リポジトリの Release asset は素の HTTPS で取得 できるため gh CLI 認証は不要」という設計判断が実 URL・実 asset で裏づけられた。 旧 PR #309 の docs コミットの主張を引き写すのではなく、自分で再実測した結果を 記録している。 ## 追補の実装状況 R1〜R4 の実装内容、破棄の実施結果、検証の実測値を反映。E2E カバレッジは 正直に申告した: 担保できたのは全 gate のユニット検証 / checker 実エントリ ポイントの実データ実走 / 修正前後の差分実測の 3 点で、cli-pr-monitor 側の 統合経路 (park → PARK signal) と wakeup → 再 trigger 経路は、CR レート制限が 本セッション中に自然発生しなかったため未実測である旨を明記した。 なお本ファイルは master 時点で既に 59,798 bytes と file_size_check の 50KB 閾値を超過しており (non-blocking 警告)、本変更で 73,781 bytes になった。 分割は § 9 の退役手順で本ファイルごと削除する前提のため見送る。
aloekun
added a commit
that referenced
this pull request
Jul 21, 2026
…#311) * docs(plan): WP-15 追補 — 監視 fail-open 修正のゼロ再構築方針 (旧 PR #309 全破棄) 旧 PR #309 (fix/monitor-fail-open-signals) は、4 コミット目の初版が本番 config で 一度も実行されない誤修正 + 実エントリポイントを迂回して pass するテストであり、 takt の High REJECT → fix step の自動書き直しを経た合成物となった。続修より ゼロ再構築が速いとのユーザー判断 (2026-07-20) に基づき、再利用なしの全破棄を 決定。健全に見えるコミットも含めて引き継がない (中途半端な状態の引き継ぎと、 それによる実装の制約を排除するため)。 追補には、新実装が単独セッションで着手できるよう以下を自己完結で記録した: - 破棄対象と手順 (PR close / branch 削除 / local abandon — 本コミット時点で未実施) - 要件 R1-R5 (What のみ。実装方式は新実装の裁量、旧コードは参照しない) - コード実読で検証済みの根本原因チェーン 6 段 (再調査不要) - 検証要件 (両 OS 全スイート / incident 実データでの checker 単体実測 / E2E カバレッジの正直な申告 / 経路同一性を確認してから実測を主張する規律) - 旧作業の教訓 (本番経路での発火をテストで固定・実エントリポイント・実データ fixture) 本コミットはドキュメント更新のみ。#309 の close・破棄・新実装には着手していない。 lint:docs / lint:md pass。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(check-ci): CR rate-limit 書式の第3世代対応と未知書式 fallback (WP-15 追補 R2) CodeRabbit が rate-limit comment の待機時間書式を 3 度目に変更 (PR #309、 2026-07-20 実観測) し、`parse_rate_limit` が None を返して rate-limit 検知が 沈黙した。ADR-034 が予告していた再発事案 (PR #182/#184 に次ぐ 2 度目の 書式変更起因 regression)。 - 第 3 世代書式 `**Next review available in:** **57 minutes**` の抽出を追加。 ラベルと数値の間に markdown 強調と `:` が挟まるため区切りを `[:*\s]*` で 吸収し、CR が強調記法を変えても壊れないようにした。 - **書式追随を前提にしない fail-closed 化**: marker (`rate limited by coderabbit.ai`) が一致したのに待機時間をどの既知書式でも読めない場合、 従来は None = 「rate-limit ではない」に倒れていた。marker 一致を制限の 根拠として採用し、待機時間だけを既定 30 分で埋める方式に変更。既定値が 実 reset より短ければ wakeup 後に再検出されて再 park されるだけで、 retry は max_retries で有界。 - 既定値適用時は checker が stderr に警告 (cli-pr-monitor がログ転送) し、 「30 分」を CR の申告値と誤読させない + 書式再変更の検知シグナルを兼ねる。 fixture は PR #309 の実 comment body を出典付きで埋め込み (ADR-049)。この comment は walkthrough header marker を同一 body に併せ持つため、clean walkthrough と誤認しない排他も併せて固定した。 ADR-034 の既知 format 一覧に第 3 世代行と fallback 方針を追記し、更新手順の stale なファイル参照 (main.rs → markers.rs / rate_limit.rs) を修正。 * fix(check-ci): decide() に rate_limit を渡し silent success を排除 (WP-15 追補 R1/R4) CodeRabbit がレート制限でレビューを開始できないまま、監視が「レビュー済み・ 指摘なし」と報告する silent success を、判定ロジック側で塞ぐ。 ## 根本原因 (2026-07-20 コード実読 + PR #307/#309 実観測) CR はレート制限中も commit check を pass にする (外部 SaaS 挙動)。checker は これを review_state に採用する一方、parse_rate_limit の結果は出力 JSON に 添付されるだけで decide() には渡っていなかった。結果、decide() は 「review_state = success かつ指摘ゼロ」= 完了と読み、stop_monitoring_success を返していた。monitor 側の terminal 短絡は rate-limit branch より先に発火する ため、park / 再 trigger 機構は一度も呼ばれない。 症状は monitor 側に出るが、原因は action の算出そのものにある。旧 PR #309 は 症状側 (monitor 2 箇所) への多層パッチで High REJECT を受けたため、本実装では 算出点である decide() に一本化した。 ## 変更 - decide() / build_summary() に rate_limit を渡す。 - R1: rate-limit 検出中かつ「レビュー実施の陽性証拠」が無ければ continue_monitoring を返し、判断を monitor の rate-limit branch (park / 再 trigger、既存・有界) に委ねる。has_actionable 分岐より前に置くことで、 過去サイクル由来の未解決スレッドで action_required に抜ける穴も塞ぐ。 - R4: rate-limit を検出できなかった場合の backstop として、陽性証拠が無い限り stop_monitoring_success を出さない。CR が marker 文言自体を変えても silent success には戻らず、最悪 max_duration までの監視継続 (timed_out) に倒れる。 - build_summary は rate-limit 中に「CodeRabbit指摘なし」と断定せず 「レート制限中 (レビュー未実施)」を出す。 ## 陽性証拠の定義 review_state (commit status) は制限中でも pass になるため証拠に使わない。 push_time で絞られた「今サイクルの CR 出力そのもの」= walkthrough_clean / actionable_comments が読めた (Some(0) 含む) / new_comments > 0 のみを採用する。 unresolved_threads は push_time で絞られず過去サイクルの残骸を含み得るので 除外した。 ## 残存リスク 陽性証拠を一切残さない clean レビュー経路が CR 側に存在した場合、監視が max_duration まで走って timed_out 報告になる。silent success より安全側だが 遅くなる。既存 fixture の範囲では walkthrough_clean か actionable_comments の いずれかが必ず立つことを確認済み。 既存 decide/summary テスト 100 件は無改修で pass (新 gate が確立済み挙動を 乱していないことの確認)。incident 再現テストは PR #309 の実観測値から構成。 * fix(pr-monitor): 判定文が未確定要素を無視して断定しないよう修正 (WP-15 追補 R3) 監視レポートの人間向け判定文が、findings が空というだけで「問題は見つかり ませんでした」と断定していた。PR #307/#309 では「未解決スレッド2件」を表示 しながら同一レポート内で「問題は見つかりませんでした」と結論する矛盾が実観測 されている。 findings が空であることは「見るべきものが無かった」の十分条件ではない。 レート制限でレビューが走っていない場合も、未解決スレッドが残っている場合も 空になり得る。 - rate-limit 検出中は保留判定文を出す (R1 で checker から rate_limit が 届くようになったため、monitor 側で判別可能になった)。 - 未解決スレッドが残っている間は「問題なし」「重大な問題なし」のいずれも 出さず、件数を添えて保留する。重大な指摘がある場合は「修正が必要」を優先。 - 判定順を「未確定 → 重大 → 未解決 → 軽微 → 問題なし」に整理し、未確定要素を findings の有無より先に評価する。断定文へ落ちる経路を構造的に塞ぐ形。 compute_verdict は未確定判定 (verdict_for_unsettled_review) と findings 判定 (verdict_for_findings) に分割し、「断定文はどの guard を通過して初めて出せる のか」を関数境界で表現した (50 行ガイドラインにも整合)。 既存の verdict テスト 13 件は無改修で pass。 * docs(plan): WP-15 完了条件 (1) 達成の記録と追補の実装状況を反映 (R5) ## R5: WP-15 `完了` 条件 (1) の達成記録 PR #307 マージ時の release-binaries.yml run は build job が失敗しており (master が赤で #308 の E2E 修正が必要だった)、成功したのは #308 マージ後の run (commit 541adde)。この経緯も含めて記録した。 生成物は本セッションで再実測している: WSL Ubuntu 24.04 から素の curl で tarball (9,721,643 bytes) + .sha256 を**認証なしで**取得 → sha256sum -c 一致 → 展開して 16 バイナリ + BUILD_INFO 確認 → release バイナリそのもので hooks 実発火 (pre-tool-validate が破壊的削除コマンドを exit 2 でブロックし無害な echo を exit 0 で通す / session-start が additionalContext JSON を出力)。 これで WP-15 ④ の「public リポジトリの Release asset は素の HTTPS で取得 できるため gh CLI 認証は不要」という設計判断が実 URL・実 asset で裏づけられた。 旧 PR #309 の docs コミットの主張を引き写すのではなく、自分で再実測した結果を 記録している。 ## 追補の実装状況 R1〜R4 の実装内容、破棄の実施結果、検証の実測値を反映。E2E カバレッジは 正直に申告した: 担保できたのは全 gate のユニット検証 / checker 実エントリ ポイントの実データ実走 / 修正前後の差分実測の 3 点で、cli-pr-monitor 側の 統合経路 (park → PARK signal) と wakeup → 再 trigger 経路は、CR レート制限が 本セッション中に自然発生しなかったため未実測である旨を明記した。 なお本ファイルは master 時点で既に 59,798 bytes と file_size_check の 50KB 閾値を超過しており (non-blocking 警告)、本変更で 73,781 bytes になった。 分割は § 9 の退役手順で本ファイルごと削除する前提のため見送る。 * docs(check-ci): wait_time_parsed の doc が実装と食い違う記述を修正 セルフレビュー (pre-push-review simplicity facet) の指摘。 `wait_time_parsed` の doc は「監視側はこれを見て『実測』と『既定値』を区別 して報告する」と書いていたが、実装では monitor 側への配線を見送っており (cli-pr-monitor の RateLimitState は本 field を持たない)、記述と実装が 食い違っていた。既定値適用を運用者に伝える経路は実際には checker の stderr 警告 (monitor がログ転送) が担っている。 doc を実態に合わせ、あわせて「なぜ typed 化を見送ったか」(全 struct literal の 更新コストに対し得られるのが park summary の文言精度という副次的利得)と 「値がどこから参照できるか」(monitor が保持する checker の生 JSON) を明記した。 なお本指摘は、PR 全体 (master..@) を対象にセルフレビューを再実行して初めて 検出された。push 時のパイプラインは [diff] stage が tip コミットのみを レビュアーに渡すため (push-runner-config.toml の `command = "jj diff -r @"`)、 models.rs を含む祖先コミットがレビュー対象外だった。この構造的欠陥は docs/todo-summary2.md 順位 288 (Tier 1、Severity High で 3 連続再発) として 既知で、本 PR で 4 回目の再発となった。修正は独立 PR で行う。 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hooks-stop-tool-call-leakの E2E helper が、stdin を読まずに終了する子プロセスへ書き込んでBrokenPipeで panic していた問題を修正write_stdin_tolerating_early_exitを導入し、BrokenPipeのみ正常として飲み込む (他の I/O エラーは従来どおり panic)release-binaries.ymlが緑に戻り、nightlyrelease が生成されるようになるContext
master が赤の状態を解消するための先行修正。 PR #307 のマージ直後に
release-binaries.ymlが初回実行され、Run testsステップがkill_switch_env_skips_checkの Broken pipe で失敗した。release が生成されないためscripts/cloud-setup.shの取得対象が不在になり、WP-15 の完了条件も満たせない状態だった。原因:
hooks-stop-tool-call-leakのmainは kill-switch (STOP_TOOL_CALL_LEAK_OVERRIDE) とenabled = falseの 2 経路で stdin を読む前に return する。読み手が消えたパイプへwrite_allすると Unix では EPIPE になるが、旧実装は.expect()で panic していた。Windows では小さな payload がパイプバッファに収まり成功しがちなため、これまで顕在化していなかった。これらのテストの主題は「skip されること」であって「stdin が消費されること」ではないため、
BrokenPipeは正常系として扱うのが正しい。スコープ: master を最短で緑に戻すことを優先し、本 PR は当該 1 件に限定した。並行して発見済みの監視系 fail-open 2 件 (rate-limit 検知の silent regression / 判定行が
unresolved_threadsを無視) は別 PR で対応する。Validation
cargo test -p hooks-stop-tool-call-leak34 + 7 pass /cargo test --workspace全 pass / clippy cleanwriteの前に 300ms の遅延を注入して EPIPE を強制する検証を実施した。同一条件で 修正前は CI と同一のエラーOs { code: 32, kind: BrokenPipe }で FAIL、修正後は PASS することを実測 (遅延は検証専用でコミットには含めない)release-binaries.ymlが緑になりnightlyrelease が生成されること (マージ後に確認)References
Summary by CodeRabbit