feat(cli-pr-monitor): cli-finding-classifier 統合 + Finding C strict (ADR-038 Phase 5)#120
Merged
Merged
Conversation
…inding C strict 化
Phase 5 (A): cli-pr-monitor poll stage への classifier 統合
- 新 module classifier_runner.rs: cli-finding-classifier.exe を subprocess invoke (stdin = findings JSON、stdout = classified findings JSON)。失敗時 (exe 不在 / spawn / stdin / timeout / parse) は空 Vec を返し block しない (ADR-038 §「失敗時の振る舞い」)
- ClassifierConfig 追加: enabled / model / endpoint / timeout_secs (default OFF、試験運用 flag)
- state.classified_findings field 追加 (serde default + skip_serializing_if = "Vec::is_empty" で後方互換)
- poll.rs に enrich_with_classifier step を追加 (build_state_for_iteration 直後、findings を enrich して state を再書き出し)
Finding C strict 化 (B): normalized_issue の契約検証強化
- from_llm_output で trim 後に「改行を含む」「80 文字超」を検出し fallback("contract violation: ...") に倒す
- max chars 上限 NORMALIZED_ISSUE_MAX_CHARS=80 を const 化 (prompts/classify.txt の "max 80 characters" 指示と一致)
- 回帰テスト 3 件追加: multi-line / 81 chars / 80 chars boundary
検証:
- cli-finding-classifier: 15 unit tests (lib) + 6 (bin) pass
- cli-pr-monitor: 148 unit tests pass (classifier_runner の 4 件 + ClassifierConfig 2 件含む)
- workspace 全体 clippy clean
scope 外 (本 PR では未対応):
- prompt injection サニタイズ (auto_fix execution 経路がまだ無いため、単に classifier を invoke して state に保存するだけの段階で実害なし。後続 PR で auto_fix を実行する経路を作るタイミングで導入予定)
- ADR-038 line 61 textual fix (action_confidence 名称統一、別 PR の cleanup で対応)
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLLM分類器をpr-monitorに統合。ClassifierConfig追加、CLI分類器ランナー新規実装、ポーリングループへのエンリッチ処理、PrMonitorStateの永続化フィールド追加、分類器側のnormalized_issue検証強化、および関連ドキュメント更新を行う。 Changes分類器統合と検証強化
Sequence DiagramsequenceDiagram
participant Poll as ポーリングループ
participant Runner as ランナー
participant Exe as CLI分類器
Poll->>Runner: classify_findings(config, findings)
Runner->>Runner: 入力検証・早期リターン
alt 入力有効かつ実行ファイル解決成功
Runner->>Exe: JSONをstdinで送信
Exe->>Exe: 分類処理（外部CLI）
Exe-->>Runner: JSON出力
Runner->>Runner: 出力をパース・Vec<ClassifiedFinding>を生成
Runner-->>Poll: 分類結果を返却
else 失敗（例: missing exe, timeout, parse error）
Runner-->>Poll: 空のVecを返却
end
Poll->>Poll: classified_findings を更新・永続化
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/cli-pr-monitor/src/stages/poll.rs (1)
109-109: 💤 Low valueiteration ごとの classifier 呼び出しコスト。
run_one_iteration毎にenrich_with_classifierが走ると、wakeup 経路で複数 iteration がある場合に Ollama を都度叩くことになります。現状はclassified_findings非空ガードで実質 1 回しか走らない実装ですが、上のコメントでガードを外すと毎 iteration でfindings.len()件の Ollama 呼び出しが発生します (timeout 30s × N findings)。Phase 5 試験運用としては許容範囲ですが、本採用前に「findings の内容 hash 変化時のみ再分類」程度のキャッシュ戦略を検討する価値があります。本 PR で対応必須ではないので参考までに。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli-pr-monitor/src/stages/poll.rs` at line 109, The current code calls enrich_with_classifier(&mut state, ctx.classifier_config) every run_one_iteration which can re-query Ollama per finding; implement a simple cache keyed by a content hash to avoid reclassification unless a finding's content changed: add a classification cache map on the state (e.g. state.classification_cache: HashMap<ContentHash, Classification>), compute a stable hash for each finding payload before calling enrich_with_classifier in run_one_iteration, filter/find only changed findings (or skip calling enrich_with_classifier entirely if none changed), update the cache with new classifications after enrich_with_classifier returns, and use state.classification_cache lookups to populate classified_findings so behavior is unchanged while avoiding redundant Ollama calls.src/cli-pr-monitor/src/classifier_runner.rs (1)
38-47: 💤 Low value
current_exe()失敗時に CWD 配下を探索する挙動が意図通りか確認したいです。
unwrap_or_default()でPathBuf::new()(空パス) を返した後、parent()はNone、unwrap_or(Path::new("."))で.に倒れて./cli-finding-classifier.exeを返します。exists()チェックがあるので致命的ではないですが、本来想定していない CWD 配下の同名 exe を誤って起動する余地が残ります (cli-pr-monitorは通常.claude/から起動される前提)。Windows-only かつ
current_exe()が失敗するのは極めて稀なため `` です。runner.rs::checker_exe_pathと挙動を揃えるだけで済むので、必要なら近傍 PR でまとめて対応するのが妥当です。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli-pr-monitor/src/classifier_runner.rs` around lines 38 - 47, The current implementation uses unwrap_or_default() which makes current_exe() failures silently fall back to CWD (./cli-finding-classifier.exe); change classifier_exe_path() to only use the executable's parent if current_exe() succeeds and otherwise use the same fallback logic as runner.rs::checker_exe_path to avoid accidentally picking up a CWD binary—specifically replace the unwrap_or_default()/parent()/unwrap_or(...) chain with current_exe().ok().and_then(|p| p.parent().map(|d| d.join("cli-finding-classifier.exe"))).unwrap_or_else(|| /* use the same fallback PathBuf used in checker_exe_path */) so the behavior matches checker_exe_path().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli-pr-monitor/src/classifier_runner.rs`:
- Around line 141-207: The child-process handling currently polls in
wait_with_timeout without draining stdout/stderr, risking pipe-buffer deadlock;
update the implementation so stdout/stderr are consumed while the child runs
(either spawn threads to read_to_end for child.stdout and child.stderr and then
construct an Output when the process exits inside wait_with_timeout, or change
the child's stderr to inherit/null to eliminate the pipe risk); locate usages in
wait_with_timeout and feed_stdin/parse_classifier_output to ensure you read the
pipes you created with Stdio::piped() (or change Stdio::stderr to inherit/null
where the child is spawned) so the classifier never blocks writing large stderr
output.
In `@src/cli-pr-monitor/src/stages/poll.rs`:
- Around line 222-236: The early-return guard in enrich_with_classifier prevents
new findings from being classified because it returns when
state.classified_findings is non-empty; remove the
"!state.classified_findings.is_empty()" check so enrich_with_classifier only
skips when config.enabled is false or state.findings is empty, call
classify_findings(config, &state.findings) each iteration, assign the result to
state.classified_findings and persist via write_state as currently done; ensure
this change aligns with build_state_for_iteration and
update_state_from_check_result semantics (or alternatively add explicit logic to
clear state.classified_findings when findings change if you prefer preserving
prior classifications).
---
Nitpick comments:
In `@src/cli-pr-monitor/src/classifier_runner.rs`:
- Around line 38-47: The current implementation uses unwrap_or_default() which
makes current_exe() failures silently fall back to CWD
(./cli-finding-classifier.exe); change classifier_exe_path() to only use the
executable's parent if current_exe() succeeds and otherwise use the same
fallback logic as runner.rs::checker_exe_path to avoid accidentally picking up a
CWD binary—specifically replace the unwrap_or_default()/parent()/unwrap_or(...)
chain with current_exe().ok().and_then(|p| p.parent().map(|d|
d.join("cli-finding-classifier.exe"))).unwrap_or_else(|| /* use the same
fallback PathBuf used in checker_exe_path */) so the behavior matches
checker_exe_path().
In `@src/cli-pr-monitor/src/stages/poll.rs`:
- Line 109: The current code calls enrich_with_classifier(&mut state,
ctx.classifier_config) every run_one_iteration which can re-query Ollama per
finding; implement a simple cache keyed by a content hash to avoid
reclassification unless a finding's content changed: add a classification cache
map on the state (e.g. state.classification_cache: HashMap<ContentHash,
Classification>), compute a stable hash for each finding payload before calling
enrich_with_classifier in run_one_iteration, filter/find only changed findings
(or skip calling enrich_with_classifier entirely if none changed), update the
cache with new classifications after enrich_with_classifier returns, and use
state.classification_cache lookups to populate classified_findings so behavior
is unchanged while avoiding redundant Ollama calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 503ee52a-d74f-425b-8819-7d933f9a87d1
📒 Files selected for processing (7)
docs/local-llm-offload-analysis.mdsrc/cli-finding-classifier/src/lib.rssrc/cli-pr-monitor/src/classifier_runner.rssrc/cli-pr-monitor/src/config.rssrc/cli-pr-monitor/src/main.rssrc/cli-pr-monitor/src/stages/poll.rssrc/cli-pr-monitor/src/state.rs
PR #120 の CodeRabbit 1 回目 review (17:55Z) の Major 2 件: 1. `classifier_runner.rs:wait_with_timeout` の deadlock リスク - 旧実装は `try_wait()` の spin-loop。child の stdout/stderr が pipe buffer (~64KB) を超えるとパイプが詰まり、try_wait は永遠に Ok(None) を返してハング - 修正: thread + mpsc::channel パターン。`wait_with_output()` がスレッド内で pipe を並行 drain しつつ child を待つ。timeout は recv_timeout で実現 - timeout 時に child を kill できないが、classifier exe 側に --timeout-secs を 渡しているため自己終了する設計 2. (false positive 却下、`resolved:` reply 投稿済) `poll.rs:enrich_with_classifier` の `!classified_findings.is_empty()` guard で stale 固着リスク - 該当 guard はコードに存在せず CR の hallucination - takt が補強テストとして追加した vacuous な regression test (assert!(true) のみ) を削除 検証: - cargo test -p cli-pr-monitor: 149 pass / 0 fail - cargo clippy: clean
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
aloekun
added a commit
that referenced
this pull request
May 7, 2026
…follow-up) PR #120 (ADR-038 Phase 5 land) の事後整理を 1 commit に集約 (docs-only): 1. ADR-038 line 61 textual fix (`docs/local-llm-offload-analysis.md` §8.C) - `confidence=0.0` → `action_confidence=0.0` に統一 - 実装の `ClassifiedFinding.action_confidence` schema 名称との整合 - 機能影響なし、永続 ADR の内部一貫性確保 2. PR #120 post-merge-feedback の採用 5 件を登録 (Bundle f) - 順位 80 (Tier 1, Bundle f): rate-limit auto-retry wakeup 予約ロジック整理 - 順位 81 (Tier 1, Bundle f): CR 投稿エラー auto-retry 拡張 - 順位 82 (Tier 3, Bundle f): ADR-018 update (transient failure auto-retry) - 順位 83 (Tier 2, 独立): 複合 AND guard 独立テスト - 順位 84 (Tier 3, 独立): code-review.md checklist 追記 - 詳細エントリは docs/todo5.md 末尾、priority table は docs/todo.md - Bundle f は新規フィードバックのため、頻度が確認できるまで優先度は再観測を経て判断 3. docs/local-llm-offload-analysis.md の更新 - §7 (実装進捗ログ) に Phase 5 land 記録 + Bundle f cross-reference 追加 - §8 (次の作業候補) で A/B を ✅ LANDED にマーク - 「先行 land を推奨」等の prioritization 表現を削除し、頻度未確定の新規フィードバックを優先しない方針を明記
aloekun
added a commit
that referenced
this pull request
May 7, 2026
…follow-up) (#121) PR #120 (ADR-038 Phase 5 land) の事後整理を 1 commit に集約 (docs-only): 1. ADR-038 line 61 textual fix (`docs/local-llm-offload-analysis.md` §8.C) - `confidence=0.0` → `action_confidence=0.0` に統一 - 実装の `ClassifiedFinding.action_confidence` schema 名称との整合 - 機能影響なし、永続 ADR の内部一貫性確保 2. PR #120 post-merge-feedback の採用 5 件を登録 (Bundle f) - 順位 80 (Tier 1, Bundle f): rate-limit auto-retry wakeup 予約ロジック整理 - 順位 81 (Tier 1, Bundle f): CR 投稿エラー auto-retry 拡張 - 順位 82 (Tier 3, Bundle f): ADR-018 update (transient failure auto-retry) - 順位 83 (Tier 2, 独立): 複合 AND guard 独立テスト - 順位 84 (Tier 3, 独立): code-review.md checklist 追記 - 詳細エントリは docs/todo5.md 末尾、priority table は docs/todo.md - Bundle f は新規フィードバックのため、頻度が確認できるまで優先度は再観測を経て判断 3. docs/local-llm-offload-analysis.md の更新 - §7 (実装進捗ログ) に Phase 5 land 記録 + Bundle f cross-reference 追加 - §8 (次の作業候補) で A/B を ✅ LANDED にマーク - 「先行 land を推奨」等の prioritization 表現を削除し、頻度未確定の新規フィードバックを優先しない方針を明記
This was referenced May 7, 2026
aloekun
added a commit
that referenced
this pull request
May 7, 2026
…ow-up) (#122) PR #121 post-merge-feedback の採用 4 件 (Bundle g) 登録 + §8.A-2 (Phase 5 dogfood 計測計画) 策定を 1 commit に集約 (docs-only): 1. Bundle g 採用 4 件登録 (順位 85-88、PR #121 post-merge-feedback) - 順位 85 (Tier 1): cli-pr-monitor monitor state machine guard 強化 (`review_state: not_found && findings: []` を pending 据置) → 3 PR 連続観測 (PR #119/#120/#121) で頻度確認済 Tier 1 妥当性確定 - 順位 86 (Tier 2): state transition test 網羅 (順位 85 の回帰テスト) - 順位 87 (Tier 3): Multi-PR chaining ベストプラクティス codify → PR #119→#120→#121 の連鎖を再利用可能化 - 順位 88 (Tier 3): edge case 観測頻度 3 = Tier 1 昇格基準 codify → ユーザー方針 (新規フィードバックは頻度確認後優先) を明文化 - 詳細エントリは docs/todo5.md 末尾、bundle commentary は docs/todo.md - Bundle f (順位 80-82) との関係: f = retry logic、g = verdict logic で別軸 2. §8.A-2 Phase 5 dogfood 計画 策定 (docs/local-llm-offload-analysis.md) - ADR-038 §試験運用→本採用の未達条件 1 (5 PR dogfood) + 3 (token 削減確認) を達成する計画 - 構成: P-0 (config opt-in) + P-1〜P-5 (Tier 1 タスク優先で 5 PR) - Setup 手順 / 計測手順 / 判定基準 / 既知の注意事項 / session 跨ぎ運用ガイドを self-contained 形式で記載 - 別セッションで再開可能な構造 (本 doc 1 ファイルで全手順把握可) 3. 整合更新 - §7 § 効果実測の現状 を §8.A-2 参照に更新 (「未測定」を明示) - §8.E dependency を §8.A-2 完了 + 判定基準達成にリンク
7 tasks
5 tasks
aloekun
added a commit
that referenced
this pull request
May 7, 2026
…g + transition matrix tests (順位 85+86 / Bundle g-1) (#125) §A-2 Phase 5 dogfood P-1 (Bundle g-1)。3 PR 連続観測 (PR #119/#120/#121) で 発覚した monitor の誤 approved 判定を fix。 順位 85 (Tier 1, T1-1): - compute_verdict() に review_state guard 追加。CodeRabbit が未投稿 (review_state: not_found) もしくは進行中 (pending) のときは findings の有無に関わらず判定保留。空 findings を 'no problems' と誤同一視する false negative を防止。 順位 86 (Tier 2, T2-4): - mod tests に (action, review_state, findings) → verdict transition matrix を 12 unit test で網羅: - parked_rate_limit / parked_review_recheck (action 優先) - not_found + 空 / pending + 空 / not_found + findings (順位 85 fix) - success + 空 / minor / critical / high / major - skipped (skip_coderabbit 経路) - coderabbit field None (初期 state) - 既存 should_resume_wakeup_* テスト 7 件と並存、no regression build + deploy 済 (.claude/cli-pr-monitor.exe を release build で再生成)。
aloekun
added a commit
that referenced
this pull request
May 7, 2026
…ailure 明文化 (順位 80+82 / §A-2 P-5) (#129) * feat(cli-pr-monitor): rate-limit Posted 経路で park 予約 (順位 80) + ADR-018 transient failure 明文化 (順位 82) + §A-2 P-4 ledger (§A-2 P-5) §A-2 Phase 5 dogfood P-5 (Bundle f-1 partial)。PR #120 で観測された rate-limit auto-retry の silent exit (RateLimitOutcome::Posted → polling 継続 → max_duration timeout) を構造的に解消。 順位 80 (Tier 1, T1-1) — finalize_posted_retrigger に park 予約を追加: - Before: 投稿後 write_state → None 返し → polling 継続 → timeout で silent exit - After: 投稿後 state.action = parked_review_recheck + next_wakeup_at_unix = now + review_recheck_wait_secs を設定 + format_park_signal で PARK 出力 → Some(park_poll_result) で必ず terminal を返す - handle_rate_limit_branch / dispatch_rate_limit_outcome / finalize_posted_retrigger 各関数に review_recheck_wait_secs 引数を thread - 2 unit tests 追加 (success 経路 + write_state 失敗 action_required 経路) 順位 82 (Tier 3, T3-2) — ADR-018 に追記: - 対象 transient failure pattern を表形式で 4 種列挙 (rate-limit 待機型/即時型 + CR 投稿エラー + wakeup 未予約 fallback) - 順位 80 fix の実装ポイント (Before/After) を明文化 - 順位 81 (CR 投稿エラー) を defer した理由を 3 観測閾値ルールで説明 順位 81 (Bundle f-1 残部、Tier 1 T1-2): - 1 観測のみ (PR #120) で systemic 性未確認のため defer - 同型 error が 2 件以上観測されたら再活性化 (re-trigger 条件を ADR-018 で規定) - 本セッションのユーザー方針 feedback_no_unenforced_rules と整合 §A-2 計測ログ P-4 結果記録: - P-4 (PR #128): 1 finding (CR Nitpick: cross_module_* 命名 misleading) - classifier: action=human_review / confidence=0.9 (P-2 の 0.0 から大幅改善) - normalized_issue: Japanese 50 chars (length contract pass) - latency: 6.6s/件、fallback: 0/1 (P-2 の 1/1 から改善) - agreement: 1/1 (100%、私評価=human_review と一致) 170 active tests pass (新 2 件追加)、regression なし。 build + deploy 済 (.claude/cli-pr-monitor.exe)。 * docs(todo): 順位 80 + 82 (rate-limit park 予約 + ADR-018 update) 完了に伴い削除 + 順位 81 defer 注記
This was referenced May 8, 2026
Merged
6 tasks
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
ADR-038 (試験運用) Phase 5 として、
cli-pr-monitorpoll stage にcli-finding-classifier.exeを subprocess 経由で統合。同時に PR #119 の CR review round 3 で再指摘された Finding C (normalized_issue 契約検証) を strict 化。classifier_runner.rs+ClassifierConfig(default OFF / 試験運用 flag) +state.classified_findingsfield +enrich_with_classifierstepfrom_llm_outputで trim 後に「改行」「>80 chars」を検出し fallback に倒す +NORMALIZED_ISSUE_MAX_CHARS=80const 化設計のポイント
ureq/ Ollama を持ち込まず、cli-finding-classifier.exeを spawn する形で統合。schema (ClassifiedFinding) のみ複製し、ADR-038 で schema 変更が起きた際は両方を同期する責務をコメントで明示state.classified_findingsは#[serde(default, skip_serializing_if = "Vec::is_empty")]で既存 state.json と consumer (takt facets / Claude) を破壊しないClassifierConfig.enabled = false。dogfood 段階で個別に opt-in する設計build_state_for_iterationでexisting.classified_findingsを引き継ぎ (takt auto-fix iter で追加)検証結果
clippy --all-targets -- -D warningscleanconvergence_verdict: fully_resolved)scope 外 (本 PR では未対応)
auto_fixexecution の自動実行経路を作っていない。Claude / takt が後段でhuman_reviewに倒れた finding を判断する設計のため、現時点で実害なし。auto_fixを機械的に実行する経路を追加するタイミングで sanitization を導入予定 (PR feat(cli-finding-classifier): Ollama でローカル LLM 分類 CLI 追加 (ADR-038) #119 の WARN ベース)confidence=0.0→action_confidence=0.0の名称統一は別 PR の cleanup で対応 (機能影響なし)Test plan
cargo test --workspace --lib --bins全 passcargo clippy -p cli-pr-monitor -p cli-finding-classifier -p lib-ollama-client --all-targets -- -D warningscleanpnpm build:all成功 (.claude/cli-finding-classifier.exe配置済、cli-pr-monitor.exe も再ビルド)[classifier] enabled = trueで 1 PR の review サイクルを実走、token 削減効果を実測 — 別タスクで実施関連
Summary by CodeRabbit
新機能
改善
設定
ドキュメント