fix(hooks): post-pr-monitor の4件の既知問題を修正 - #16
Merged
Conversation
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/hooks-post-pr-monitor/src/main.rs:
- Around line 191-199: The current logic only tries the first jj bookmark (uses
split(',').next()) so you miss PR heads on later bookmarks; update the code
paths around get_jj_bookmark() and the similar block at lines 223-241 to iterate
over all comma-separated local_bookmarks, trying each entry with
run_gh_quiet("pr", "list", "--head", <bookmark>, ...) until you successfully
parse a pr_number; return a PrInfo with the first successful pr_number and repo,
or None if none match. Ensure you reference and reuse the same
parsing/validation logic used now (the s.parse::<u64>().ok() flow) for each
bookmark.
- Around line 420-459: convert_body_to_file currently writes to a fixed temp
filename "gh-pr-body.md" and returns an Option<PathBuf>, which causes cross-run
collisions and leaked files on early returns; change it to create a unique
temporary file per call (use tempfile::NamedTempFile or create a
random/UUID-based filename in std::env::temp_dir()) and keep the NamedTempFile
handle alive until the caller is done so the OS file is removed on drop—i.e.,
update the function signature to return (Vec<String>, Option<NamedTempFile>) or
otherwise return the owned temp-file handle (not just PathBuf), write the
resolved body into that unique temp file, push its displayed path into result as
before, and ensure the file is always cleaned up by relying on the NamedTempFile
drop rather than manual cleanup that can be skipped on early returns.
- Around line 373-377: The code currently ignores errors from writing the prompt
to the child's stdin (the write_all call inside the child.stdin.take() block),
which hides real failures; update that block to check the Result from
stdin.write_all(prompt.as_bytes()) and propagate a proper Err (e.g., return
Err(...)) on failure instead of discarding it so callers can see the actual
write error; ensure any error type matches the surrounding function's Result (or
convert it) and keep dropping stdin after handling the result to still send EOF.
🪄 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: 48221473-740f-4c15-b0fb-73ca2883b9dc
📒 Files selected for processing (3)
.claude/hooks-config.toml.claude/hooks-post-pr-monitor/src/main.rsdocs/todo.md
aloekun
force-pushed
the
fix/post-pr-monitor-issues
branch
from
April 3, 2026 12:00
93208db to
be23ce7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.claude/hooks-post-pr-monitor/src/main.rs (2)
441-476: 複数の--body引数がある場合、最初の変換のみが保持されます。現在の実装では、引数リストに複数の
--bodyがある場合（稀なケースですが）、temp_guardが上書きされ、最初に作成された一時ファイルが即座に削除されます。gh CLI の仕様上、複数の
--bodyが渡されることは通常ないため、実用上は問題ありませんが、ログに一貫性がなくなる可能性があります。♻️ 複数 body 対応（必要に応じて）
-fn convert_body_to_file(args: &[String]) -> (Vec<String>, Option<TempFile>) { +fn convert_body_to_file(args: &[String]) -> (Vec<String>, Vec<TempFile>) { let mut result = Vec::with_capacity(args.len()); let mut i = 0; - let mut temp_guard: Option<TempFile> = None; + let mut temp_guards: Vec<TempFile> = Vec::new(); while i < args.len() { if args[i] == "--body" && i + 1 < args.len() { let body = &args[i + 1]; if body.contains('\n') || body.contains("\\n") { // ... (ファイル作成ロジック) match std::fs::write(&path, &resolved) { Ok(()) => { // ... - temp_guard = Some(TempFile(path)); + temp_guards.push(TempFile(path)); } // ... } } } // ... } - (result, temp_guard) + (result, temp_guards) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks-post-pr-monitor/src/main.rs around lines 441 - 476, The loop currently overwrites temp_guard when multiple "--body" args are converted, causing earlier temp files (TempFile) to be dropped immediately; change temp_guard from an Option<TempFile> to a collection (e.g., Vec<TempFile>) and push each created TempFile instead of assigning to temp_guard so all temporary files remain alive until function end, update any drop/cleanup logic to iterate that collection, and adjust where result pushes the corresponding "--body-file" paths to still match each converted body.
384-385:unwrap()をexpect()に置き換えることを推奨します。
Stdio::piped()を設定しているためtake()がNoneを返すことは通常ありませんが、デバッグ時のエラーメッセージの明確化のためexpect()の使用を検討してください。♻️ 提案
- let stdout_handle = drain_pipe(child.stdout.take().unwrap()); - let stderr_handle = drain_pipe(child.stderr.take().unwrap()); + let stdout_handle = drain_pipe(child.stdout.take().expect("stdout should be piped")); + let stderr_handle = drain_pipe(child.stderr.take().expect("stderr should be piped"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks-post-pr-monitor/src/main.rs around lines 384 - 385, child.stdout.take().unwrap() と child.stderr.take().unwrap() がパニック時のデバッグ情報不足を招くため、unwrap() を具体的な期待理由を付けた expect() に置き換えてください（例: child.stdout.take().expect("child.stdout should be piped as Stdio::piped()") と同様に child.stderr も）。対象箇所は drain_pipe(child.stdout.take().unwrap()) と drain_pipe(child.stderr.take().unwrap())、および関連する drain_pipe 呼び出しで、None の場合に分かりやすいエラーメッセージを返すように修正してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/hooks-post-pr-monitor/src/main.rs:
- Around line 441-476: The loop currently overwrites temp_guard when multiple
"--body" args are converted, causing earlier temp files (TempFile) to be dropped
immediately; change temp_guard from an Option<TempFile> to a collection (e.g.,
Vec<TempFile>) and push each created TempFile instead of assigning to temp_guard
so all temporary files remain alive until function end, update any drop/cleanup
logic to iterate that collection, and adjust where result pushes the
corresponding "--body-file" paths to still match each converted body.
- Around line 384-385: child.stdout.take().unwrap() と
child.stderr.take().unwrap() がパニック時のデバッグ情報不足を招くため、unwrap() を具体的な期待理由を付けた
expect() に置き換えてください（例: child.stdout.take().expect("child.stdout should be piped
as Stdio::piped()") と同様に child.stderr も）。対象箇所は
drain_pipe(child.stdout.take().unwrap()) と
drain_pipe(child.stderr.take().unwrap())、および関連する drain_pipe 呼び出しで、None
の場合に分かりやすいエラーメッセージを返すように修正してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6418468f-f5df-4c70-98fd-2cfc5b7c7f92
📒 Files selected for processing (2)
.claude/hooks-post-pr-monitor/src/main.rsdocs/todo.md
✅ Files skipped from review due to trivial changes (1)
- docs/todo.md
This was referenced Apr 3, 2026
This was referenced Apr 14, 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\
PR #13 で報告された hooks-post-pr-monitor の既知問題4件をすべて修正。
\
修正内容\
\
\
Test plan\
\
Summary by CodeRabbit
リリースノート
機能改善
ドキュメント