refactor(lib): drain_pipe を 3 variant で lib-subprocess に extract (順位 173c) - #207
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
Changesdrain_pipe 3バリアント抽出とcallsite移行
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
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 `@src/cli-push-runner/src/stages/pr_size_check.rs`:
- Around line 157-164: The problem is that drain_pipe_capped silently truncates
output that is critical for control flow decisions, causing incorrect behavior
when important information (like the summary line of jj diff --stat or bookmark
counts) exceeds the cap. In src/cli-push-runner/src/stages/pr_size_check.rs at
lines 157-164, replace drain_pipe_capped for stdout with drain_pipe_unlimited
since the jj diff --stat summary appears at the end and must not be truncated
for accurate size determination. In
src/cli-push-runner/src/stages/bookmark_check.rs at lines 83-90, replace
drain_pipe_capped with drain_pipe_unlimited for the stdout used in bookmark
existence determination to ensure accurate detection. In
src/cli-push-runner/src/stages/push_jj_bookmark.rs at lines 142-149, replace
drain_pipe_capped with drain_pipe_unlimited for stdout in the fallback bookmark
count judgment logic to prevent data loss that could cause incorrect moves or
skips. All three locations must switch from capped to unlimited draining since
they depend on complete, untruncated output for correctness.
🪄 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: fc73f969-8982-4148-85d7-b9b67e41bdff
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
docs/todo11.mdsrc/cli-merge-pipeline/src/main.rssrc/cli-pr-monitor/src/runner.rssrc/cli-pr-monitor/src/stages/push_jj_bookmark.rssrc/cli-push-pipeline/src/main.rssrc/cli-push-runner/src/runner.rssrc/cli-push-runner/src/stages/bookmark_check.rssrc/cli-push-runner/src/stages/lint_screen.rssrc/cli-push-runner/src/stages/pr_size_check.rssrc/cli-push-runner/src/stages/push_jj_bookmark.rssrc/cli-push-runner/src/stages/scratch_file_warning.rssrc/hooks-stop-quality/Cargo.tomlsrc/hooks-stop-quality/src/main.rssrc/lib-subprocess/src/lib.rs
Resolved findings: - [Major] src/cli-push-runner/src/stages/pr_size_check.rs:164 制御判定に使う stdout を silent truncate しており、分岐が誤判定されます。
|
@coderabbitai review |
✅ Action performedReview finished.
|
…d) (#208) * docs(todo): 順位 208 (drain_pipe_capped 境界値テスト) を追加 (PR #207 post-merge-feedback T2-1 採用) * refactor(lib): run_cmd を lib-subprocess に extract (順位 173d) * fix(review): apply CodeRabbit fixes for #208 Resolved findings: - [Major] src/lib-subprocess/src/lib.rs:247 `try_wait` エラー時に子プロセスと reader スレッドを未回収のまま返却しています
PR #207 (順位 173c) で CR が 🟠 Major として発見した silent truncate 由来 silent disable (fail-open、ADR-043 violation) の root cause = drain_pipe_capped が MAX_LINES boundary で off-by-one regression を起こしうる構造を test レベルで pin する。 追加内容 (lib-subprocess/src/lib.rs): - drain_pipe_capped: N-1 / N / N+1 行の 3 boundary test - drain_pipe_capped_reporting: 同 3 boundary test (truncated 報告 on/off 含む) - max_lines=5 で固定、各 case は独立 setup (feedback_test_dry_antipattern 準拠) 検証: - cargo test -p lib-subprocess 全 27 test pass (既存 21 + 新規 6) - cargo clippy -p lib-subprocess --tests -- -D warnings clean - 手動 mutation check: `< max_lines` → `<= max_lines` で両 variant の n_plus_1 test が落ちることを確認 (off-by-one regression 検出構造)
Summary
drain_pipeを 3 variant (_unlimited/_capped/_capped_reporting) としてlib-subprocessに extractMAX_LINESの crate 別意味 (40 = ログ表示用 / 200 = メモリ保護用 / 20 = stop hook 用) を保持3 variant の意味
drain_pipe_unlimitedread_to_string全読みrunner.rs内部 2 +stages/push_jj_bookmark.rs2 = 計 4 callsites) —check-ci-coderabbitの JSON 全量パース用drain_pipe_capped(pipe, max_lines)read_until行毎読込、max_lines超過は silent truncate (パイプは継続排出)runner.rs2 +stages/{scratch_file_warning, push_jj_bookmark, pr_size_check, lint_screen, bookmark_check}.rs10 = 計 12) + cli-push-pipeline 2 + hooks-stop-quality 2 = 計 16 callsitesdrain_pipe_capped_reporting(pipe, max_lines)"... (N lines truncated)"付与MAX_LINES 定数の扱い
各 crate に
const MAX_LINES: usize = N;を保持し、callsite で parameter として渡す形に変更:cli-push-runner の
MAX_LINESはpub(crate) const化して 5 stage file からcrate::runner::MAX_LINES参照可能にした (runner-internal const がワークスペース内 stage から参照される非標準的な visibility shape だが、定数の意味は「cli-push-runner の出力 truncate ポリシー」で 1 箇所維持が適切。将来config.rs等への移動余地あり)。内訳 (15 files changed, +219 / -179)
lib-subprocess/src/lib.rscli-pr-monitor/src/runner.rsdrain_pipeimpl + NOTE コメント削除、内部 2 callsite をdrain_pipe_unlimitedに置換cli-pr-monitor/src/stages/push_jj_bookmark.rslib_subprocess::drain_pipe_unlimitedにcli-push-runner/src/runner.rsdrain_pipeimpl 削除、MAX_LINESをpub(crate) const化、内部 2 callsite をdrain_pipe_capped(pipe, MAX_LINES)にcli-push-runner/src/stages/*.rs5 ファイルlib_subprocess::drain_pipe_capped(..., crate::runner::MAX_LINES)にcli-push-pipeline/src/main.rsdrain_pipeimpl 削除、2 callsite をdrain_pipe_capped(pipe, MAX_LINES)に (lib-subprocess は 173a で dep 追加済)hooks-stop-quality/Cargo.tomllib-subprocessdep 追加 (本 PR で初追加)hooks-stop-quality/src/main.rsdrain_pipeimpl 削除、2 callsite をdrain_pipe_capped(pipe, MAX_LINES)にcli-merge-pipeline/src/main.rsdrain_pipeimpl 削除、2 callsite をdrain_pipe_capped_reporting(pipe, MAX_LINES)にdocs/todo11.mdTest plan
cargo build --workspacepasscargo test --workspacepass (lib-subprocess 15 test + 6 影響 crate を含む workspace 全体 pass)cargo clippy --workspace -- -D warningsclean (push-runner-config.toml の gate と同等)173a/b と異なり挙動差が test 観測可能
173b の
_safevs_basicは Err 経路の差で「unreachable」だったため test で variant 分岐を pin できなかったが、173c の 3 variant は inputs で直接観測可能な挙動差 (drain_pipe_unlimitedは 500 行返却 /drain_pipe_cappedは 3 行切り捨て /_reportingは"... (2 lines truncated)"付与) を持つため、test が variant 分岐を mechanically pin する。誤って variant を merge した場合 test が失敗する。関連
combine_outputextract), PR refactor(lib): wait_with_timeout を 2 variant で lib-subprocess に extract (順位 173b) #206 (順位 173b /wait_with_timeout2 variant)docs/todo11.md順位 173 entry — 173d (run_cmd) / 173e (variant merge 判断) の残計画lib-jj-helpers) — 異なる semantics を別 helper として export する先例Summary by CodeRabbit
Refactor