refactor(cli-merge-pipeline): feedback.rs + main.rs を 800 行以下に module 分割 (PR-W3) - #230
Merged
Conversation
…te lint + 高並列 regression test を登録 (順位 236-237)
…n link 修正 (adr-030 / plan / todo2)
📝 WalkthroughWalkthroughmain.rs から処理ロジックを config・feedback・github・pipeline の各モジュールへ分割した。post-merge-feedback の実装を feedback.rs から feedback/ ディレクトリ配下（context・markers・mod・pr_metadata・takt・transcript）へ再構成し、設定ローダー、PR検出、マージ処理、失敗マーカー/並行起動ガードを追加した。関連ドキュメントの参照パスとタスクリストも更新した。 Changescli-merge-pipeline 分割とフィードバック機能再編
Sequence Diagram(s)sequenceDiagram
participant Pipeline as pipeline::run_pipeline
participant GitHub as github.rs
participant Feedback as feedback::run
participant Takt as feedback/takt.rs
participant Markers as feedback/markers.rs
Pipeline->>GitHub: detect_pr_number / detect_owner_repo
Pipeline->>GitHub: gh pr view --json state
Pipeline->>GitHub: gh api .../merge (squash)
Pipeline->>GitHub: delete_remote_branch
Pipeline->>Feedback: run(FeedbackInput)
Feedback->>Markers: check_concurrent_run_guard
Feedback->>Markers: write_pending_marker_logged
Feedback->>Takt: run_takt_workflow
Takt-->>Feedback: 成否(bool)
Feedback->>Takt: copy_feedback_report
alt コピー成功
Feedback->>Markers: cleanup_failed_marker (disarm)
else コピー失敗
Feedback-->>Pipeline: Err
Pipeline->>Markers: write_failed_marker
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/cli-merge-pipeline/src/feedback/transcript.rs (1)
41-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win複数 session jsonl ファイルがある場合、出力順序が時系列通りにならない可能性。
fs::read_dirのファイル走査順序は非決定的で、各ファイルはマッチした行をその場で書き込むため、複数の*.jsonl(複数 Claude セッション) が同一 range に重なる場合、合成 transcript 内でファイル間の順序が時刻順にならない可能性があります。ファイル内の順序自体は保たれるため致命的ではありませんが、PR の作業が複数セッションに渡るのは一般的なシナリオであるため、downstream の takt workflow が読む文脈の質に影響する可能性があります。
(timestamp, line)を収集してから書き込み前に timestamp でソートする方が安全です。♻️ 提案: マッチした行を timestamp でソートしてから書き込む
pub fn filter_transcripts( source_dir: &Path, range: &PrTimeRange, out_path: &Path, ) -> Result<usize, String> { if let Some(parent) = out_path.parent() { fs::create_dir_all(parent) .map_err(|e| format!("出力ディレクトリ作成失敗 {}: {}", parent.display(), e))?; } - let mut writer = fs::File::create(out_path) - .map(std::io::BufWriter::new) - .map_err(|e| format!("出力ファイル作成失敗 {}: {}", out_path.display(), e))?; - - let mut written = 0usize; + let mut matched: Vec<(String, String)> = Vec::new(); let entries = fs::read_dir(source_dir) .map_err(|e| format!("transcript dir 読込失敗 {}: {}", source_dir.display(), e))?; for entry in entries.flatten() { let path = entry.path(); if path.extension().and_then(|s| s.to_str()) != Some("jsonl") { continue; } let file = match fs::File::open(&path) { Ok(f) => f, Err(_) => continue, }; let reader = BufReader::new(file); for line in reader.lines().map_while(Result::ok) { if line.trim().is_empty() { continue; } - if entry_matches_filter(&line, range) { - writeln!(writer, "{}", line).map_err(|e| format!("出力書込失敗: {}", e))?; - written += 1; + if let Some(ts) = entry_timestamp_if_matches(&line, range) { + matched.push((ts, line)); } } } - writer.flush().map_err(|e| format!("flush 失敗: {}", e))?; - Ok(written) + matched.sort_by(|a, b| a.0.cmp(&b.0)); + let mut writer = fs::File::create(out_path) + .map(std::io::BufWriter::new) + .map_err(|e| format!("出力ファイル作成失敗 {}: {}", out_path.display(), e))?; + for (_, line) in &matched { + writeln!(writer, "{}", line).map_err(|e| format!("出力書込失敗: {}", e))?; + } + writer.flush().map_err(|e| format!("flush 失敗: {}", e))?; + Ok(matched.len()) }🤖 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-merge-pipeline/src/feedback/transcript.rs` around lines 41 - 82, The transcript merge in filter_transcripts writes matching lines as files are read, so multiple session .jsonl inputs can be interleaved in non-deterministic order. Update filter_transcripts to collect matched entries from each file with their parsed timestamp (and line text), then sort the collected items by timestamp before writing to the BufWriter. Keep the current per-file line order behavior when timestamps are equal, and make the change in the filter_transcripts / entry_matches_filter flow so the output is consistently chronological across sessions.src/cli-merge-pipeline/src/feedback/mod.rs (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pub公開がPRの設計方針 (pub(crate)) と一致していない
write_failed_marker、fetch_pr_diff_summary、project_transcript_dir、FEEDBACK_DIR/CONTEXT_PATH/TRANSCRIPT_PATH、FeedbackInput、runはいずれも crate 外から参照されることのないバイナリクレート内シンボルだが、pubで宣言されている。PR概要では「pub(crate) を使用してクロスモジュールアクセス」を方針としているため、pub(crate)に揃えることで意図と実装の一致、および公開面の最小化が図れる。Also applies to: 44-57, 70-70
🤖 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-merge-pipeline/src/feedback/mod.rs` around lines 27 - 29, The feedback module’s public exports and related symbols are broader than intended for an internal binary crate. Update the visibility of write_failed_marker, fetch_pr_diff_summary, project_transcript_dir, FEEDBACK_DIR, CONTEXT_PATH, TRANSCRIPT_PATH, FeedbackInput, and run to pub(crate) so they remain accessible across modules but are not publicly exported; check the feedback module and its markers, pr_metadata, transcript, and entry-point definitions to align all of them with the crate-only design.src/cli-merge-pipeline/src/feedback/takt.rs (1)
42-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winspawn/try_wait の失敗理由が握り潰されている
Err(_) => return false(spawn失敗) とErr(_) => break None(try_wait失敗) はいずれもエラー詳細を捨ててboolのみ返している。呼び出し元のmod.rs::run()はこのfalseを受け取った後、reconcile_takt_outputで「report 不在」という汎用メッセージにしかならず、実際の原因（例:pnpmが見つからない、権限エラーなど）が.failedmarker やログに残らない。write_pending_marker_loggedなど他箇所では eprintln で詳細を残すパターンがあるため、ここでも同様にすべき。♻️ 提案修正
.spawn() { Ok(c) => c, - Err(_) => return false, + Err(e) => { + eprintln!("[merge-pipeline] [feedback] takt spawn失敗: {}", e); + return false; + } }; let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs); let exited_success = loop { match child.try_wait() { Ok(Some(status)) => break Some(status.success()), Ok(None) if std::time::Instant::now() >= deadline => break None, - Err(_) => break None, + Err(e) => { + eprintln!("[merge-pipeline] [feedback] takt try_wait失敗: {}", e); + break None; + } Ok(None) => std::thread::sleep(Duration::from_millis(POLL_INTERVAL_MS)), } };🤖 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-merge-pipeline/src/feedback/takt.rs` around lines 42 - 60, The `takt.rs` process handling in `spawn_takt`/the `child.try_wait()` loop is swallowing the underlying `std::io::Error`, so failures only surface as a generic false result. Update the `Command::new("pnpm").spawn()` and `child.try_wait()` error branches to log or print the actual error details before returning, matching the existing detailed logging style used elsewhere such as `write_pending_marker_logged`, so `mod.rs::run()` can still fail but the real cause is preserved in logs/markers.src/cli-merge-pipeline/src/feedback/markers.rs (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CONTEXT_PATH定数の代わりにパスをハードコードしている
.takt/post-merge-feedback-context.jsonはfeedback::CONTEXT_PATH(mod.rs L45) と同一の値だが、ここではリテラル文字列として重複している。今後CONTEXT_PATHを変更した際にこのメッセージだけ追従し忘れるドリフトリスクがある。♻️ 提案修正
- 注意: この再実行は `.takt/post-merge-feedback-context.json` を読み直すだけなので、\n \ + 注意: この再実行は `{}` を読み直すだけなので、\n \ 失敗から再実行までの間に **別 PR が `pnpm merge-pr` を実行している** と context が\n \ 上書きされ、誤った PR の transcript range が使われます。再実行前に\n \ - `.takt/post-merge-feedback-context.json` の `pr_number` が #{} と一致することを必ず確認してください。\n", + `{}` の `pr_number` が #{} と一致することを必ず確認してください。\n", + // ... 既存引数に加えて crate::feedback::CONTEXT_PATH を2箇所追加🤖 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-merge-pipeline/src/feedback/markers.rs` around lines 36 - 40, The retry guidance is hardcoding `.takt/post-merge-feedback-context.json` instead of using the shared `feedback::CONTEXT_PATH`, which can drift from the actual source of truth. Update the message in `markers.rs` to reference `feedback::CONTEXT_PATH` (or the same shared constant used in `mod.rs`) so the path stays consistent if it changes elsewhere, while keeping the existing `pr_number` warning text intact.
🤖 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-merge-pipeline/src/feedback/markers.rs`:
- Around line 349-357: Update concurrent_run_guard_passes_when_context_stale in
markers::tests so it actually exercises check_concurrent_run_guard and asserts
the Ok path when the context is stale, instead of only checking context_age_secs
immediately after fs::write; if you intend to keep testing context_age_secs
directly, rename the test to match that behavior. Use the existing helpers
check_concurrent_run_guard, context_age_secs, and CONCURRENT_RUN_GUARD_SECS to
keep the test aligned with its name.
In `@src/cli-merge-pipeline/src/feedback/pr_metadata.rs`:
- Around line 43-71: `fetch_pr_time_range` currently runs `gh pr view` with
`Command::output()` and can hang indefinitely; update this path to use a
timeout-enabled execution flow that kills the `gh` process if it exceeds the
limit and returns a clear timeout error. Apply the same timeout handling pattern
used for `fetch_pr_diff_summary` so both PR metadata fetchers share consistent
behavior, and keep the existing error mapping around process start, non-success
exit, and JSON parsing in `parse_pr_time_range`.
In `@src/cli-merge-pipeline/src/github.rs`:
- Around line 49-54: `run_gh_logged`, `delete_remote_branch`,
`fetch_pr_time_range`, and `fetch_pr_diff_summary` currently call
`Command::new("gh").output()` directly, which can block the merge pipeline
indefinitely; route these `gh` invocations through the existing timeout-capped
command path instead. Update the `github.rs` helpers and the
`feedback/pr_metadata.rs` fetch functions to use the same timeout wrapper as
`run_cmd_shell_capped_reporting`, preserving the current logging/return behavior
while ensuring auth or network stalls cannot hang the pipeline.
---
Nitpick comments:
In `@src/cli-merge-pipeline/src/feedback/markers.rs`:
- Around line 36-40: The retry guidance is hardcoding
`.takt/post-merge-feedback-context.json` instead of using the shared
`feedback::CONTEXT_PATH`, which can drift from the actual source of truth.
Update the message in `markers.rs` to reference `feedback::CONTEXT_PATH` (or the
same shared constant used in `mod.rs`) so the path stays consistent if it
changes elsewhere, while keeping the existing `pr_number` warning text intact.
In `@src/cli-merge-pipeline/src/feedback/mod.rs`:
- Around line 27-29: The feedback module’s public exports and related symbols
are broader than intended for an internal binary crate. Update the visibility of
write_failed_marker, fetch_pr_diff_summary, project_transcript_dir,
FEEDBACK_DIR, CONTEXT_PATH, TRANSCRIPT_PATH, FeedbackInput, and run to
pub(crate) so they remain accessible across modules but are not publicly
exported; check the feedback module and its markers, pr_metadata, transcript,
and entry-point definitions to align all of them with the crate-only design.
In `@src/cli-merge-pipeline/src/feedback/takt.rs`:
- Around line 42-60: The `takt.rs` process handling in `spawn_takt`/the
`child.try_wait()` loop is swallowing the underlying `std::io::Error`, so
failures only surface as a generic false result. Update the
`Command::new("pnpm").spawn()` and `child.try_wait()` error branches to log or
print the actual error details before returning, matching the existing detailed
logging style used elsewhere such as `write_pending_marker_logged`, so
`mod.rs::run()` can still fail but the real cause is preserved in logs/markers.
In `@src/cli-merge-pipeline/src/feedback/transcript.rs`:
- Around line 41-82: The transcript merge in filter_transcripts writes matching
lines as files are read, so multiple session .jsonl inputs can be interleaved in
non-deterministic order. Update filter_transcripts to collect matched entries
from each file with their parsed timestamp (and line text), then sort the
collected items by timestamp before writing to the BufWriter. Keep the current
per-file line order behavior when timestamps are equal, and make the change in
the filter_transcripts / entry_matches_filter flow so the output is consistently
chronological across sessions.
🪄 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: 684a854c-9c25-467d-b442-17bac7444a62
📒 Files selected for processing (16)
docs/adr/adr-030-deterministic-post-merge-feedback.mddocs/file-length-enforcement-plan.mddocs/todo-summary.mddocs/todo13.mddocs/todo2.mdsrc/cli-merge-pipeline/src/config.rssrc/cli-merge-pipeline/src/feedback.rssrc/cli-merge-pipeline/src/feedback/context.rssrc/cli-merge-pipeline/src/feedback/markers.rssrc/cli-merge-pipeline/src/feedback/mod.rssrc/cli-merge-pipeline/src/feedback/pr_metadata.rssrc/cli-merge-pipeline/src/feedback/takt.rssrc/cli-merge-pipeline/src/feedback/transcript.rssrc/cli-merge-pipeline/src/github.rssrc/cli-merge-pipeline/src/main.rssrc/cli-merge-pipeline/src/pipeline.rs
💤 Files with no reviewable changes (1)
- src/cli-merge-pipeline/src/feedback.rs
aloekun
added a commit
that referenced
this pull request
Jul 1, 2026
…下に module 分割 (PR-W4) (#231) * docs(plan): file-length-enforcement-plan PR-W3 を land 済 (#230) に更新 * docs(todo): PR #230 post-merge-feedback 採用 4 件を登録 (順位 238-241) * refactor(cli-push-runner): config.rs + stages/lint_screen.rs を 800 行以下に module 分割 (PR-W4) * docs(plan): PR-W4 分割に伴い lint_screen.rs / config.rs のリンクを module ディレクトリに更新
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>
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.
PR-W3: cli-merge-pipeline 分割 (file_length enforcement Phase 1)\
mechanical refactor、behavior 不変、test count 不変 (59 tests = feedback 41 + main 18 を維持)。
\
分割内容\
\
feedback.rs(1432 行) →feedback/6 module: mod(172) / markers(374) / pr_metadata(285) / context(276) / transcript(263) / takt(123)\main.rs(921 行) → main(25) / config(139) / github(259) / pipeline(558)\\
検証\
\
cargo test -p cli-merge-pipeline: 59 passed / 0 failed\cargo clippy -p cli-merge-pipeline -- -D warnings: clean\cargo fmt --check: clean\cargo test --workspace/clippy --workspace: regression なし\\
コミット構成 (3 commits、code と docs を分離)\
\
docs(todo): PR fix(permissions): cli-pr-monitor --monitor-only を allow に追加し監視 wakeup の都度確認を解消 #229 post-merge-feedback T1-1/T2-1 採用 (順位 236-237) — 前回 feedback の Todo、本 PR に同梱\refactor(cli-merge-pipeline): module 分割本体 (mechanical)\docs: W3 split 反映 — plan status 更新 + feedback.rs → feedback/ broken link 修正 (adr-030 / plan / todo2)\
push 注記\
PR_SIZE_CHECK_OVERRIDE=1で pr_size_check (block_threshold 1500) を意図的バイパス (mechanical refactor、~4857 行 diff)。順位 151 の override 想定 use case に該当。Summary by CodeRabbit
新機能
Bug Fixes
ドキュメント