perf(cli-finding-classifier): Ollama eval を env opt-in 化 (push T1) - #279
Merged
Conversation
T0 (PR #278) の dogfood push で得た stage 別実測を §5 T0 に記録した。 あわせて、その実測が T1 の前提と食い違う点を T1 セクションに申し送りとして残す。 - quality_gate 実測 93.9s に対し、T1 が根拠に引く 269s は約 3 倍。 T1 の期待効果 (-2〜4.5 分/push) と受け入れ基準 (269s → 90s 未満) は そのままでは使えない可能性が高い。 - 想定原因はローカル LLM 環境の更新 (ADR-040 記録時 RTX 3070 8GB → 現 RTX PRO 5000 48GB)。ADR-040 の resource 数値は stale。 - T1 着手前に `--ignored` スイート全体と eval テスト単体を実測し、 前提が生きているかを判定してから方針を決める手順を記載。 T1 は別セッションで実施するため、そのセッションが本ファイルだけで 判断できるよう計測コマンドと判断分岐まで書き下している。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e から除外 (push パイプライン改善 T1) assert を持たない計測専用テスト run_lint_screen_against_all_fixtures が、 quality_gate と takt fix step の `cargo test -- --ignored` に巻き込まれて 毎 push 実行されていた。LINT_SCREEN_EVALS が truthy のときだけ走るよう テスト側にガードを入れる (呼出箇所が gate / fix / 手動と複数あるため コマンド側では漏れる)。 実測 (2026-07-16, Ollama 起動状態): - --ignored スイート全体: 63s → 21s (-42s) - eval 単体: 41.3s → 0s (skip) - opt-in 経路は 15 fixture が正常実行され agreement 86.7% (GO) なお計画が根拠に引いていた 269s は再現せず 63s だった (GPU 更新により mistral:7b の推論が高速化)。期待効果を -2〜4.5 分/push → -42s/push に 下方修正し、ADR-040 の resource 数値が stale である旨を記録した。 step_timeout: 600 → 300。600 に上げた主因 (eval) が消えたため実測ベースで right-size。cold build 実測の最遅コマンドは cargo test の 28s で、約 10 倍の マージンを確保。step_timeout は group 単位でなくコマンド単位の適用。 tests/lint_screen_evals.rs は変更前から 799 行 (上限 800) でガード追加分が 入らないため、main.rs (schema/metrics) と e2e.rs (実 Ollama 呼出) に分割した。 Cargo が tests/<name>/main.rs を test target として認識するため target 名と 起動コマンドは不変。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughlint-screen の Ollama eval を Changeslint-screen eval 実行制御
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant lint_screen_evals
participant Ollama
participant screen_diff
TestRunner->>lint_screen_evals: LINT_SCREEN_EVALS を確認
lint_screen_evals->>Ollama: eval 用モデルを呼び出す
Ollama-->>screen_diff: LLM の分類結果
screen_diff-->>lint_screen_evals: AgreementMetrics と latency
lint_screen_evals-->>TestRunner: 集計レポートを出力
Possibly related PRs
🚥 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)該当なし Applicable Findings (Medium 以下)該当なし Filtered (not applicable)該当なし（レビュー指摘自体が 0 件のため、fitness filter 適用対象なし） 次のアクション
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/push-pipeline-fix-plan.md`:
- Around line 281-286: 同文書内の旧ファイル参照を更新し、削除済みの tests/lint_screen_evals.rs
と旧行番号ではなく、分割後の tests/lint_screen_evals/e2e.rs
にある該当関数名を参照してください。分割後の構成および記録内容と一致する表記に揃えてください。
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11a0cd07-9efc-46eb-9610-76caafa65d6a
📒 Files selected for processing (5)
docs/adr/adr-038-local-llm-finding-classification.mddocs/push-pipeline-fix-plan.mdpush-runner-config.tomlsrc/cli-finding-classifier/tests/lint_screen_evals/e2e.rssrc/cli-finding-classifier/tests/lint_screen_evals/main.rs
Comment on lines
+281
to
+286
| - **ファイル分割 (T1 に付随して発生)**: `tests/lint_screen_evals.rs` が変更前から 799 行 | ||
| (上限 800) で、ガード追加分が入らなかった。file-length linter は touch-trigger ratchet | ||
| のため、`tests/lint_screen_evals/{main.rs,e2e.rs}` に分割した (main = schema/metrics の | ||
| 常時実行テスト 608 行 / e2e = env ガード + 実 Ollama 呼出 + レポート)。 | ||
| Cargo が `tests/<name>/main.rs` を test target として自動認識するため、 | ||
| target 名 `lint_screen_evals` と既存の起動コマンドは不変。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
同文書内の旧ファイル参照も更新してください。
Lines 209-210 は削除済みの tests/lint_screen_evals.rs と旧行番号を参照したままです。tests/lint_screen_evals/e2e.rs の関数名参照へ更新し、分割後の記録と整合させてください。
🤖 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 `@docs/push-pipeline-fix-plan.md` around lines 281 - 286,
同文書内の旧ファイル参照を更新し、削除済みの tests/lint_screen_evals.rs と旧行番号ではなく、分割後の
tests/lint_screen_evals/e2e.rs にある該当関数名を参照してください。分割後の構成および記録内容と一致する表記に揃えてください。
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)該当なし Applicable Findings (Medium 以下)該当なし Filtered (not applicable)
次のアクション
|
aloekun
added a commit
that referenced
this pull request
Jul 16, 2026
* fix(cli-push-runner): 空 `@` 時の bookmark_check 誤誘導を修正 (push パイプライン改善 T8) `@` が空で bookmark が `@-` にある状態 (jj new 直後の正常な再 push 状態) で、 同一 run 内の advance_jj_bookmarks が「bookmark を @- に自動更新」と報告した 直後に bookmark_check が「bookmark が見つかりません」と報告し、 `jj bookmark create <name> -r @` を案内していた。従うと空の WIP コミットに bookmark が付く破壊的操作になる。PR #279 (T1) の dogfood push で実際に発火。 根本原因は「@ が空なら @- を対象にする」規則の二重定義。advance は determine_target_revision() で規則を持つのに、bookmark_check は OWN_WORKSPACE_BOOKMARKS_REVSET ("@" 厳密一致) で独自に検査していたため、 両者の判定が食い違った。 修正: - determine_target_revision() から working_copy_is_empty() を切り出し、 bookmark_check と共有する (規則の二重定義を解消)。 - 「@ に bookmark が無い」を 2 ケースに切り分ける判定 enum BookmarkCheckOutcome と pure function decide_bookmark_check() を追加。 jj 呼び出しは closure 注入 (ADR-021 原則 3、既存 dispatch_bookmark_advance と同じ流儀)。 - `@` 空 + bookmark が @-: `jj edit @-` + 空 WIP の abandon を案内 (T1 セッションで実証済みの回避策)。 - bookmark 皆無: 従来の作成案内が正しいので維持。 - main.rs に重複していた同じ誤案内を撤去し、ケース別案内を出す bookmark_check に一本化。 exit 7 による中断は維持し、案内文のみを正す方針を採った。計画の方針 2 (検査を @- 対象にして続行) は、[diff] command = "jj diff -r @" のため `@` が空のまま続行すると diff が空になり takt レビューが無言 skip された まま push される (誤誘導バグをレビューバイパスに置き換える) ため不採用。 方針 3 の「push すべき新変更がない」も、再現記録の事実 4 (jj edit @- 後に push 成功 = 変更はあった) と矛盾するため不採用。 ADR-021 原則 5 との関係: bookmark_check が `@` 厳密一致に狭めているのは PR #271 (他 workspace の bookmark 混入) の対策。本修正の @- 照会は案内文の 出し分け (診断) 専用で、push 対象の組み立ては `@` のまま維持する。 テスト: mod t8_empty_head_misdirection に 7 本追加 (186 → 193 passed)。 由来 incident と再現状態を module doc に明記 (ADR-049 の流儀)。bad = 2 ケースが潰れないこと、good = bookmark 皆無が NoBookmarks のままである ことを固定。サンドボックス jj repo で配布 exe が記録の出力を逐語再現する ことを確認した上で修正後 exe と before/after 比較した。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(review): CodeRabbit Major 2 件 + simplicity 警告 2 件を反映 (PR #280 / push T8) PR #280 のレビュー指摘を反映する。いずれも「T8 が直したはずの穴が、条件違いで 残っていた」類の指摘で、本タスクの主題 (正確な案内) そのものに関わる。 ## CodeRabbit Major (2 件、採用) 1. 判定順を反転し、bookmark が空の `@` にある場合も中断する 従来は「`@` に bookmark があれば続行」を先に判定していたため、bookmark が 空の `@` に付いていると Proceed していた。この経路は `jj diff -r @` が空に なり、祖先の未 push 変更が AI レビューを経ずに push される — 本タスクが 方針 2 を却下した理由と同じ穴が、bookmark の位置違いで残っていた。 `advance_jj_bookmarks` は非 trunk bookmark が 2 つ以上あると fallback 更新を skip するため、この状態は実在する (サンドボックスで再現確認済み: 修正前は 「非 trunk bookmark 検出 (1 件): feat/b」で通過し PR diff 0 行へ進んでいた)。 「レビュー範囲 = `@` だから `@` は非空でなければならない」という本タスクの 不変条件に判定順を揃えた。 2. `@-` 照会の失敗を握り潰さない `unwrap_or_default()` が照会失敗を「親はあるが bookmark 無し」に変換して いたため、`@-` の存在を確認できていないのに実行不能な `jj edit @-` を 案内し得た。ParentState::Unavailable として保持し、親を確認できない場合は `jj edit @-` を案内しない。 ## simplicity-review 非ブロッキング警告 (2 件、採用) 3. `query_parent_state()` の jj 失敗を log する 同ファイルの他の jj 失敗処理や push_jj_bookmark.rs は log_info する慣習が あり、ここだけ欠落していた。親を確認できない理由が残らないと、root commit なのか jj 不調なのかを切り分けられない。 4. `@-` に bookmark が無い場合の案内を分ける `jj edit @-` だけを案内すると次は `NoBookmarks` で止まり根本解決にならない。 bookmark 作成まで含めて 1 度に案内する 3 つ目の variant に分けた。 ## 不採用 CodeRabbit Minor の日付指摘は不採用: CodeRabbit は UTC 基準で「2026-07-16」と 指摘しているが、本 repo の記録は JST 基準 (既存の T0/T1 も同様) のため 2026-07-17 が正しい。同指摘のうち PR 番号 (未採番 → #280) は採用した。 テスト: t8_empty_head_misdirection を 7 → 12 本に拡充 (193 → 198 passed)。 バイパス経路を固定していた既存テスト 1 本は中断側へ反転させた。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 16, 2026
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
run_lint_screen_against_all_fixturesをLINT_SCREEN_EVALSenv opt-in 化し、quality_gate と takt fix step のcargo test -- --ignoredから除外--ignoredスイート全体が 63s → 21s (-42s)。dogfood push の quality_gate は93.9s (PR feat(cli-push-runner): stage 別の所要時間ログを追加 (push パイプライン改善 T0) #278) → 46.5s
step_timeoutを 600 → 300 に right-size。600 の根拠だった eval が消えたため、cold build 実測 (最遅 =
cargo test28s) に約 10 倍のマージンで再設定tests/lint_screen_evals.rsを{main.rs, e2e.rs}に分割 (file-length ratchet 対応)Context
push パイプラインの遅延調査 (
docs/push-pipeline-fix-plan.md) の T1。#[ignore]付きだが、gate と takt fix step が--ignoredを無条件で付けるため毎 push (fix があれば 2 回) 実 LLM を呼んでいた。このテストは assert を持たず
(
report_summaryは println のみ)、gate では時間だけ払って何も検証しない。削除ではなく opt-in 化したのは、モデル/プロンプト変更時の人手評価に価値があるため。
着手前の実測で計画の前提が崩れたため、期待効果を修正した上で実施している。
計画が根拠に引いていた「eval 269s」は再現せず 63s だった (GPU が RTX PRO 5000 48GB に
更新済みで mistral:7b の推論が高速化)。期待効果を -2〜4.5 分/push → -42s/push に
下方修正し、§1 結論の「eval が 12 分超の主犯」という認定も訂正した (真の主犯は
takt の execute/fix = T10/T12)。判断根拠は §8 判定記録に記載。
Scope: ファイル分割は T1 に付随して発生した (対象ファイルが変更前から 799 行 =
上限 800 で、ガード追加分が入らなかった)。rename 検出により diff は 518 行に収まり、
PR size の warning 閾値 800 を下回るため分割 PR にはしていない。
Validation
cargo test: 全 workspace pass (失敗 0)。--ignoredも passcargo clippy --workspace --all-targets --all-features -- -D warnings: cleanpnpm lint/pnpm lint:docs: OKLINT_SCREEN_EVALS=1で 15 fixture が実行されagreement 86.7% (13/15、GO) を確認。未設定時は skip メッセージを出して即 return
--ignored63s → 21s、eval 単体 41.3s → 0spnpm pushpre-push review:verdict=APPROVE(simplicity / security とも、fix iteration 0)。simplicity の non-blocking 指摘 (e2e.rs のモジュールコメントが
自身の gate 境界を不正確に記述) は修正し、再 push で再 APPROVE
References
docs/push-pipeline-fix-plan.md: §5 T1 実施結果 / §8 判定記録Summary by CodeRabbit