Skip to content

refactor(cli-push-runner): config.rs + stages/lint_screen.rs を 800 行以下に module 分割 (PR-W4) - #231

Merged
aloekun merged 4 commits into
masterfrom
pr-w4-push-runner-split
Jul 1, 2026
Merged

refactor(cli-push-runner): config.rs + stages/lint_screen.rs を 800 行以下に module 分割 (PR-W4)#231
aloekun merged 4 commits into
masterfrom
pr-w4-push-runner-split

Conversation

@aloekun

@aloekun aloekun commented Jul 1, 2026

Copy link
Copy Markdown
Owner

概要

file-length-enforcement-plan の PR-W4: cli-push-runner の 800 行超 2 ファイルを module 分割。

mechanical refactor / behavior 不変 / test count 不変 (168 tests、baseline から変化なし)。

分割内容

元ファイル 分割後 最大行数
config.rs (946 行) config/{mod,lint_screen,pr_size_check,scratch_file_warning}.rs 429
stages/lint_screen.rs (982 行) stages/lint_screen/{mod,classifier,diff_filter,report}.rs 415

全ファイル ≤ 800 行を達成。

config/ の分割方針

  • mod.rs: Config 集約構造体 + config_path / load_config / validate_config + 共通定数 + 再エクスポート
  • lint_screen.rs: LintScreenConfig + LINT_SCREEN_ENABLED env override 解釈
  • pr_size_check.rs: PrSizeCheckConfig + 順位 151 定数
  • scratch_file_warning.rs: ScratchFileWarningConfig

stages/lint_screen/ の分割方針

  • mod.rs: orchestration (run_lint_screen + params 解決 + diff 読み込み)
  • classifier.rs: cli-finding-classifier.exe の subprocess 起動
  • report.rs: classifier 出力 (JSON) の markdown 整形 + 書き出し
  • diff_filter.rs: LLM 入力前の diff 前処理 (対象外拡張子除外 + metadata strip)

制約遵守 (Phase 1 共通)

  • pre-existing 非 doc コメントは移動時に削除 (config_pathexe_adjacent_config_path 分割で意図を表現)
  • 関数長 50 行: invoke_classifier から spawn_classifier を抽出
  • cross-module visibility は pub(crate) / pub(super)
  • test helper は per-module duplicate (共有 util module を作らない)

検証

  • cargo test -p cli-push-runner: 168 passed (baseline 不変)
  • cargo clippy --workspace -- -D warnings: clean
  • cargo fmt: clean
  • ✅ 全ファイル ≤ 800 行
  • cargo test --workspace: regression なし

同梱の docs コミット (mechanical refactor とは別コミット)

イテレーションコスト低減のため、前セッションのフィードバック由来 docs 変更を本 PR に同梱 (コミットは分離):

PR size

~8900 行の diff (削除 + 追加) だが mechanical な file 移動が主体。PR_SIZE_CHECK_OVERRIDE=1 で pre-push の pr_size_check を意図的バイパス (順位 151 の override 想定 use case)。

Summary by CodeRabbit

  • New Features

    • push 時のチェック機能に、差分サイズやスクラッチファイル警告などの設定が追加されました。
    • lint-screen の有効/無効を環境変数で上書きできるようになりました。
  • Bug Fixes

    • 一部の差分処理で、Markdown など対象外ファイルを自動的に除外するようになりました。
    • 失敗時の出力がより分かりやすくなり、結果の記録も安定しました。
  • Documentation

    • 実装進捗と作業予定のドキュメントを最新状態に更新しました。

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

cli-push-runner の設定読み込み(config.rs)とlint-screenステージ(stages/lint_screen.rs)がそれぞれ複数のサブモジュール(config/mod.rs, config/lint_screen.rs, config/pr_size_check.rs, config/scratch_file_warning.rs / stages/lint_screen/mod.rs, classifier.rs, diff_filter.rs, report.rs)へ分割された。合わせて計画ドキュメントとTODOドキュメントが更新された。

Changes

cli-push-runner モジュール分割とドキュメント更新

Layer / File(s) Summary
Config コア構造とロード処理
src/cli-push-runner/src/config.rs(削除), src/cli-push-runner/src/config/mod.rs
旧config.rsを削除し、Config構造体・config_path・load_config/validate_configをconfig/mod.rsへ新設。TOMLパース・quality_gate検証のテストも追加。
サブ設定モジュール
src/cli-push-runner/src/config/lint_screen.rs, .../pr_size_check.rs, .../scratch_file_warning.rs
LintScreenConfigとLINT_SCREEN_ENABLED環境変数オーバーライド、PrSizeCheckConfig、ScratchFileWarningConfigをそれぞれ独立ファイルに定義し、パース挙動テストを追加。
lint-screenステージのdiff読込・フィルタ
src/cli-push-runner/src/stages/lint_screen.rs(削除), .../stages/lint_screen/mod.rs, .../diff_filter.rs
旧stages/lint_screen.rsを削除し、run_lint_screenのフロー(diff読込、行数上限/空判定、除外ハンク処理、メタデータ除去)をmod.rsとdiff_filter.rsへ分割実装。
classifier起動とレポート出力
src/cli-push-runner/src/stages/lint_screen/classifier.rs, .../report.rs
classifier.exeのサブプロセス起動・タイムアウト判定を実装し、レポート/skip-reportのMarkdown生成・書き出しロジックを追加。
計画/TODOドキュメント更新
docs/file-length-enforcement-plan.md, docs/todo-summary.md, docs/todo13.md
PR-W3のland済ステータス反映、PR-W4の参照パス更新、post-merge-feedback由来の新規タスク(gh直叩き禁止lint、filter_transcripts決定論化、takt.rsエラー握り潰し解消、pub(crate)方針明文化)を追記。

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

  • aloekun/claude-code-hook-test#132: 同じlint-screen機能に対する先行実装で、本PRがconfig.rsとstages/lint_screen.rsをモジュール分割へリファクタリングしている対象。
  • aloekun/claude-code-hook-test#147: LINT_SCREEN_ENABLED環境変数オーバーライドロジックの元実装で、本PRでconfig/lint_screen.rsへ再配置された。
  • aloekun/claude-code-hook-test#155: markdownハンク除外機能の元実装で、本PRのdiff_filter.rsのfilter_excluded_hunksへ継承されている。

Poem

  • コードの兎、モジュールを跳ね
  • config.rsは去り、mod.rsが咲く
  • lint-screenは四つに分かれ
  • diff読み、フィルタし、レポート書く
  • TODOに新芽、進捗の跡も残す 🐇
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed config.rs と stages/lint_screen.rs をモジュール分割する変更を正確に要約しており、内容と一致しています。
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/cli-push-runner/src/stages/lint_screen/classifier.rs`:
- Around line 46-67: In classifier.rs, the current spawn/order in the stage
logic can deadlock because stdin.write_all() runs before stdout/stderr draining
starts. Reorder the flow around spawn_classifier, drain_pipe_capped, and
wait_with_timeout_basic so the stdout/stderr drain threads are started before
writing diff to child.stdin, or move the stdin write onto a separate thread with
timeout protection. Keep the existing exit handling and error mapping intact
while ensuring large diffs cannot block the push pipeline.
🪄 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: d04afcde-094a-4042-ba0b-17807496412a

📥 Commits

Reviewing files that changed from the base of the PR and between 3e7fdf9 and c744bb1.

📒 Files selected for processing (13)
  • docs/file-length-enforcement-plan.md
  • docs/todo-summary.md
  • docs/todo13.md
  • src/cli-push-runner/src/config.rs
  • src/cli-push-runner/src/config/lint_screen.rs
  • src/cli-push-runner/src/config/mod.rs
  • src/cli-push-runner/src/config/pr_size_check.rs
  • src/cli-push-runner/src/config/scratch_file_warning.rs
  • src/cli-push-runner/src/stages/lint_screen.rs
  • src/cli-push-runner/src/stages/lint_screen/classifier.rs
  • src/cli-push-runner/src/stages/lint_screen/diff_filter.rs
  • src/cli-push-runner/src/stages/lint_screen/mod.rs
  • src/cli-push-runner/src/stages/lint_screen/report.rs
💤 Files with no reviewable changes (2)
  • src/cli-push-runner/src/stages/lint_screen.rs
  • src/cli-push-runner/src/config.rs

Comment thread src/cli-push-runner/src/stages/lint_screen/classifier.rs
@aloekun
aloekun merged commit bf6977c into master Jul 1, 2026
1 check passed
@aloekun
aloekun deleted the pr-w4-push-runner-split branch July 1, 2026 08:42
aloekun added a commit that referenced this pull request Jul 1, 2026
…k todo 登録 (順位 242-245) (#232)

* docs(plan): file-length-enforcement-plan PR-W2/W4 を land 済 に更新

* docs(todo): PR #231 post-merge-feedback 採用 4 件を登録 (順位 242-245)
aloekun added a commit that referenced this pull request Jul 2, 2026
…ied-files batch mode + [file_length_gate] opt-in (#234)

* docs(todo): PR #232 post-merge-feedback 採用 1 件を登録 (順位 246)

* feat(hooks): PR-W5 file-length Stop gate — comment-lint --check-modified-files batch mode + [file_length_gate] opt-in

Phase 1 (PR-W1〜W4、#220/#224/#230/#231) で 800 行以下に整えた clean state を恒久維持する
強制層。hooks-post-tool-comment-lint-rust に --check-modified-files batch mode を追加し、
Stop hook quality_gate の 1 step として PR 範囲 (base..@) の .rs file 行数を検査。
800 行超が 1 件でもあれば exit 1 で Stop を block する (Option C-2)。

実装:
- src/hooks-post-tool-comment-lint-rust/src/modified_files_check.rs (新規、17 tests)
- main.rs に --check-modified-files dispatch 追加、Cargo.toml に toml 依存追加
- .claude/hooks-config.toml に file-length step + [file_length_gate] section

mechanical refactor、behavior 不変。既存 lint (comment/function/file_length/metrics) は不変。

## ADR-039 3 点セット (experimental feature 標準パターン)

- Config opt-in (default OFF): gate_enabled() が unwrap_or(false)。本 repo のみ dogfood で enabled=true
- Kill-switch: 下表
- Bounded lifetime: file-length-enforcement-plan.md 削除条件 3 (override 未使用で 1-2 セッション通過)

## Kill-switch table

| 起動経路 | 停止コマンド | 影響範囲 |
|---|---|---|
| .claude/hooks-config.toml の [file_length_gate] enabled=true + file-length step | enabled=false (恒久) | Stop hook の file-length 判定のみ (他 step 不変) |
| Stop hook 発火時に file-length step 実行 | env FILE_LENGTH_CHECK_OVERRIDE=1 (緊急、truthy 値) | 当該 Stop の判定を skip |

## 設計判断

- jj 変更検出は base branch を config 引数化 (default master、ADR-021 § Revset Composability)
- cmd path は cmd.exe の forward-slash 非対応のため backslash TOML literal string
- jj 失敗時は fail-closed で block (ADR-043、stop_hook_active retry-skip が永続 lock を防止)
- templates (TS/Python) は Rust 非対象のため未追加

## 検証

- cargo test -p hooks-post-tool-comment-lint-rust: 116 pass / clippy clean / fmt clean
- cargo test --workspace: regression なし / cargo clippy --workspace clean
- dogfood (deploy 済 exe): clean=exit0 / 850行file=block / OVERRIDE(=1,=true)=bypass / enabled=false=no-op / self-host=exit0

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(review): CodeRabbit #234-1 fail-closed 対応 + 削除 file skip 補正

CodeRabbit Major #234-1 (読み取り不能な既存 .rs は fail-closed に) を適用:
collect_oversize_files を Result 化し、存在するのに読めない .rs は Err → exit 1 (block)。

さらに削除 file 誤検知を補正: jj diff --name-only は削除 file も列挙する (実測確認) ため、
Path::exists() で存在確認し削除 file (非存在) は skip する。これがないと file split
refactor (元 file 削除を伴う、本 plan が促進する作業そのもの) を gate が誤 block する。
CodeRabbit の指摘文言も「*既存* .rs」であり削除 file は対象外。

- collect_oversize_files: filter(Path::exists) + Result<Vec, String>
- fail-closed 診断を run_check_modified_files で block 表示 (ADR-043 § 原則1)
- tests: skips_deleted_file (skip) + errors_on_present_but_unreadable (Err) で両分岐を assert

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aloekun added a commit that referenced this pull request Jul 3, 2026
…k を解消 (順位242) (#241)

* fix(cli-push-runner): invoke_classifier を drain-first 化し pipe deadlock を解消 (順位242)

PR #231 で CodeRabbit が Major 指摘した pre-existing 欠陥の修正。
stdin.write_all(diff) を完了してから stdout/stderr の drain thread を
spawn していたため、大きな diff で子プロセス (cli-finding-classifier)
が stdin 読込前に大量出力するとパイプバッファ (~64KB) 満杯で親子相互
ブロックの deadlock になり push pipeline がハングした。

- pipe orchestration を pump_child_io として抽出し、drain thread を
  stdin write より前に spawn する drain-first に修正 (run_cmd_capture /
  Safe Subprocess Stdout Pattern と同型)
- エラー経路 (stdin 書込失敗 / wait 失敗) で子プロセス kill + wait +
  drain join を行う abort_child を追加 (孤児プロセス残留防止)
- regression test: stdin 読込前に ~256KB の stdout を吐く子プロセスに
  ~1MB の stdin を流して完走を検証 (#[ignore]、push gate の --ignored
  step で毎 push 実行)。修正前の順序では本テストが write_all でハング
  することを実測確認済み (78 分継続、自然回復なし)
- 正常系 / 非 0 終了 / stdout 空の pump_child_io 単体テスト 3 件追加
- todo13.md 順位242 エントリ削除 + todo-summary.md 行削除
- 順位254 登録: pr_size_check がローカル master 遅延で誤計測する問題
  (本 push で 1604 行と誤 block された実観測、base を remote tracking
  ref にする改修 task)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* WIP: PR #241 レビュー待ち

* fix(review): apply CodeRabbit fixes for #241

Resolved findings:
- [Major] src/cli-push-runner/src/stages/lint_screen/classifier.rs:186 タイムアウト分岐のテストが存在しない

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant