refactor(hooks): post-pr-monitor を daemon + state file アーキテクチャに変更 - #24
Merged
Conversation
📝 WalkthroughWalkthroughPR監視フローを、CronCreateベースのセッション再開アプローチから、バックグラウンドデーモンとJSON状態ファイル（ Changes
Sequence DiagramssequenceDiagram
participant User as User (pnpm push)
participant PostPR as Post-PR Monitor\n(hooks-post-pr-monitor.exe)
participant Daemon as Daemon Process\n(--daemon)
participant CheckCI as check-ci-coderabbit.exe
participant StateFile as State File\n.claude/pr-monitor-state.json
participant Claude as Claude\n(CronCreate)
User->>PostPR: pnpm push をトリガー
PostPR->>StateFile: 初期状態を書き込み (action: "continue_monitoring")
PostPR->>Daemon: デーモンを起動 (--daemon --state-file ...)
PostPR->>Claude: CronCreate を stdout に出力 (cat state file)
loop ポーリングループ
Daemon->>Daemon: wait poll_interval_secs
Daemon->>CheckCI: 実行 (--push-time, --repo, --pr)
CheckCI-->>Daemon: JSON チェック結果を返す
Daemon->>StateFile: 結果を原子書き込み (.tmp -> rename)
Daemon->>Daemon: action/経過時間を判定（継続 or 終了）
alt 終了条件
Daemon->>StateFile: daemon_status="done"/"error"
Daemon->>Daemon: 退出
end
end
Claude->>StateFile: CronCreate が定期的に state を読む (cat)
Claude->>Claude: state.action を参照して処理
sequenceDiagram
participant Dev as Developer
participant Hook as hooks-session-start
participant File as .session-id
Dev->>Hook: フック実行
Hook->>Hook: session_id を生成
alt ファイルが存在しない
Hook->>File: 新規書き込み
else ファイル内容 == session_id
Hook->>Hook: 書き込みをスキップ
else ファイル内容 != session_id
Hook->>File: 上書き
end
Hook-->>Dev: 完了
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
src/hooks-post-pr-monitor/src/main.rs (1)
460-474: 非 Windows 環境での daemon デタッチについて確認。非 Windows 版の
spawn_daemonはsetsidや二重 fork パターンを使用していません。現在の実装では、親プロセス終了時に daemon が SIGHUP を受け取る可能性があります。Windows 専用プロジェクトであれば問題ありませんが、将来的にクロスプラットフォーム対応する場合は
nix::unistd::setsid()等の使用を検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks-post-pr-monitor/src/main.rs` around lines 460 - 474, The current spawn_daemon function spawns the child but does not fully detach it (no setsid or double-fork), so the daemon may receive SIGHUP when the parent exits; update spawn_daemon to properly daemonize on non-Windows by invoking a session/daemonization step (e.g., call nix::unistd::setsid() in the child or perform the conventional double-fork) before returning the child PID, ensuring stdio is redirected and the working dir/umask are handled as needed; modify the Command handling around spawn_daemon to execute the pre-exec detachment (or perform fork/setsid in a small helper) and reference spawn_daemon and the child creation logic when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks-post-pr-monitor/src/main.rs`:
- Around line 531-538: The slice using output[..output.len().min(200)] can panic
on UTF-8 multi-byte boundaries; change uses of that slice in the parsing/error
branch (where state.summary is set and in log_info) to safely truncate by
character boundary instead of bytes—compute a safe byte index <=200 (e.g. using
output.char_indices or checking is_char_boundary) and slice up to that index,
then use that safe substring for state.summary and the log_info call; ensure
write_state_to(state_file, &state) still receives the updated state.
---
Nitpick comments:
In `@src/hooks-post-pr-monitor/src/main.rs`:
- Around line 460-474: The current spawn_daemon function spawns the child but
does not fully detach it (no setsid or double-fork), so the daemon may receive
SIGHUP when the parent exits; update spawn_daemon to properly daemonize on
non-Windows by invoking a session/daemonization step (e.g., call
nix::unistd::setsid() in the child or perform the conventional double-fork)
before returning the child PID, ensuring stdio is redirected and the working
dir/umask are handled as needed; modify the Command handling around spawn_daemon
to execute the pre-exec detachment (or perform fork/setsid in a small helper)
and reference spawn_daemon and the child creation logic when making the change.
🪄 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: 5e29fd87-e122-4f02-ba38-2f1d023d343f
⛔ Files ignored due to path filters (1)
src/hooks-post-pr-monitor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.claude/hooks-config.toml.gitignoredocs/adr/adr-009-post-pr-monitor.mdpackage.jsonsrc/hooks-post-pr-monitor/Cargo.tomlsrc/hooks-post-pr-monitor/src/main.rssrc/hooks-session-start/src/main.rs
## 背景 claude -p --resume が VSCode 拡張セッションで動作しない問題を解決。 外部プロセスから Claude セッションに状態を注入する設計がアンチパターンであると認識し、 外部 daemon が監視を完結させ結果を state file に書き出す方式に移行。 ## 主な変更 - hooks-post-pr-monitor.exe: claude -p コード削除、daemon + state file に全面書き換え - PR 作成後に daemon をバックグラウンドスポーン (Windows detached process) - check-ci-coderabbit.exe を定期ポーリングし pr-monitor-state.json を更新 - stdout に CronCreate セットアップ指示を出力 - --mark-notified モード追加 (二重通知防止) - checker 失敗時のエラーハンドリング追加 - check_ci/check_coderabbit 設定の daemon 側フィルタ - state file 書き込み → daemon スポーンの順序で race condition 解消 - hooks-session-start.exe: .session-id の「先勝ち」→「同一IDスキップ」方式に変更 - ADR-009: フロー図を daemon + state file アーキテクチャに更新 - hooks-config.toml: SessionStart / post_pr_monitor コメントを新設計に更新 - package.json: mark-notified, check-monitor スクリプト追加 - .gitignore: pr-monitor-state.json 等のランタイムファイルを除外 - Skill (post-pr-create-review-check): state file ベースのワークフローに書き換え
aloekun
force-pushed
the
refactor/daemon-state-file
branch
from
April 5, 2026 03:25
45b83ff to
32d3710
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/hooks-post-pr-monitor/src/main.rs (3)
468-482: 非 Windows 環境での daemon 分離が不完全な可能性。非 Windows 版の
spawn_daemonはsetsid()やダブルフォークを使用していないため、親プロセスが終了したり TTY が閉じられた場合に SIGHUP を受信して終了する可能性があります。最大監視時間が 10 分と短いため実用上は問題ないかもしれませんが、macOS/Linux でより堅牢な daemon 化が必要な場合は検討してください。
♻️ Unix で setsid を使用する例
#[cfg(not(target_os = "windows"))] fn spawn_daemon(state_file: &Path) -> Result<u32, String> { + use std::os::unix::process::CommandExt; + let exe = std::env::current_exe() .map_err(|e| format!("exe パス取得失敗: {}", e))?; - let child = Command::new(&exe) + let mut cmd = Command::new(&exe); + cmd .args(["--daemon", "--state-file", &state_file.to_string_lossy()]) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .spawn() - .map_err(|e| format!("daemon スポーン失敗: {}", e))?; + .stderr(std::process::Stdio::null()); + + // SAFETY: setsid is async-signal-safe + unsafe { + cmd.pre_exec(|| { + libc::setsid(); + Ok(()) + }); + } + let child = cmd.spawn() + .map_err(|e| format!("daemon スポーン失敗: {}", e))?; + Ok(child.id()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks-post-pr-monitor/src/main.rs` around lines 468 - 482, The spawn_daemon function currently just spawns the exe and can still receive SIGHUP/tty signals; update spawn_daemon to fully daemonize on Unix by performing a double-fork (or at minimum fork + setsid) in the parent process, call libc::setsid() in the first child to detach from the controlling terminal, optionally do a second fork to prevent reacquisition of a tty, close/redirect stdio (keeping your existing stdin/stdout/stderr nulls), and return the daemon PID; ensure you handle and map fork/setsid errors into the same Result<String> error mapping used by spawn_daemon so callers (and logging) get clear failure messages.
690-708: daemon スポーン失敗時の戻り値について。daemon スポーンが失敗した場合でも
0(成功) を返し、CronCreate 指示を出力しています。daemon_pidはNone("?"と表示) でdaemon_statusは"error"に設定されるため、state file を読めば問題は検出できますが、ユーザーにとっては分かりにくい可能性があります。スポーン失敗時に明示的なエラーメッセージと非ゼロ終了コードを返すことを検討してください。
🛠️ スポーン失敗時にエラー終了する例
// Daemon スポーン (state file が存在する状態で起動) match spawn_daemon(&state_path) { Ok(pid) => { state.daemon_pid = Some(pid); log_info(&format!("daemon スポーン完了 (PID: {})", pid)); } Err(e) => { state.daemon_status = "error".to_string(); state.summary = format!("daemon スポーン失敗: {}", e); + let _ = write_state(&state); log_info(&format!("daemon スポーン失敗: {}", e)); + return 1; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks-post-pr-monitor/src/main.rs` around lines 690 - 708, When spawn_daemon(&state_path) returns Err(e) don't proceed as if successful: in the Err arm keep setting state.daemon_status and state.summary and call write_state(&state) and log the detailed error, but do not call print_cron_instruction(&state, &monitor_config); instead return a non‑zero exit code (or call std::process::exit(1)) so the process fails visibly. Move print_cron_instruction(&state, &monitor_config) into the Ok(pid) path (after setting state.daemon_pid and logging success), ensure write_state(&state) is still called before returning, and replace the final unconditional "0" with a success return (0) only for the Ok branch and a non‑zero for the Err branch.
598-602: state 書き込みエラーの無視について。
write_state_toの結果がlet _ = ...で無視されています。書き込み失敗は致命的ではありませんが、ログ出力があると運用時のデバッグが容易になります。📝 エラー時にログを出力する例
// 7. Write updated state and sleep - let _ = write_state_to(state_file, &state); + if let Err(e) = write_state_to(state_file, &state) { + log_info(&format!("state 更新失敗 (継続): {}", e)); + } std::thread::sleep(Duration::from_secs(poll_interval));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks-post-pr-monitor/src/main.rs` around lines 598 - 602, The call to write_state_to(state_file, &state) is being ignored with let _ = ..., so write failures are silent; change this to check the Result and log any error (e.g., use if let Err(e) = write_state_to(state_file, &state) { log::error!("Failed to write state to {:?}: {}", state_file, e); } or similar) so write errors are reported before sleeping; reference write_state_to, state_file, state, and poll_interval when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks-post-pr-monitor/src/main.rs`:
- Around line 573-577: The current unconditional overwrite of state.action from
"action_required" to "stop_monitoring_success" when skip_coderabbit is enabled
is unsafe; update the logic to only convert CodeRabbit-originated
"action_required" to "stop_monitoring_success" by introducing and checking an
explicit provenance flag (e.g., state.action_source == "coderabbit" or
state.action_from_coderabbit boolean) before assigning, and set that provenance
when the state is constructed/parsing results (where check-ci-coderabbit /
decide() results are produced); ensure decide() and any code paths that create
State populate the new provenance field so CI-originated "action_required" still
maps to stop_monitoring_failure.
---
Nitpick comments:
In `@src/hooks-post-pr-monitor/src/main.rs`:
- Around line 468-482: The spawn_daemon function currently just spawns the exe
and can still receive SIGHUP/tty signals; update spawn_daemon to fully daemonize
on Unix by performing a double-fork (or at minimum fork + setsid) in the parent
process, call libc::setsid() in the first child to detach from the controlling
terminal, optionally do a second fork to prevent reacquisition of a tty,
close/redirect stdio (keeping your existing stdin/stdout/stderr nulls), and
return the daemon PID; ensure you handle and map fork/setsid errors into the
same Result<String> error mapping used by spawn_daemon so callers (and logging)
get clear failure messages.
- Around line 690-708: When spawn_daemon(&state_path) returns Err(e) don't
proceed as if successful: in the Err arm keep setting state.daemon_status and
state.summary and call write_state(&state) and log the detailed error, but do
not call print_cron_instruction(&state, &monitor_config); instead return a
non‑zero exit code (or call std::process::exit(1)) so the process fails visibly.
Move print_cron_instruction(&state, &monitor_config) into the Ok(pid) path
(after setting state.daemon_pid and logging success), ensure write_state(&state)
is still called before returning, and replace the final unconditional "0" with a
success return (0) only for the Ok branch and a non‑zero for the Err branch.
- Around line 598-602: The call to write_state_to(state_file, &state) is being
ignored with let _ = ..., so write failures are silent; change this to check the
Result and log any error (e.g., use if let Err(e) = write_state_to(state_file,
&state) { log::error!("Failed to write state to {:?}: {}", state_file, e); } or
similar) so write errors are reported before sleeping; reference write_state_to,
state_file, state, and poll_interval when making the change.
🪄 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: 002dd6a9-3423-4438-89c1-9a25da3f70bf
⛔ Files ignored due to path filters (1)
src/hooks-post-pr-monitor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.claude/hooks-config.toml.gitignoredocs/adr/adr-009-post-pr-monitor.mdpackage.jsonsrc/hooks-post-pr-monitor/Cargo.tomlsrc/hooks-post-pr-monitor/src/main.rssrc/hooks-session-start/src/main.rs
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- src/hooks-post-pr-monitor/Cargo.toml
- package.json
- docs/adr/adr-009-post-pr-monitor.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/hooks-config.toml
Comment on lines
+573
to
577
| // coderabbit スキップ時は action_required を無視して success に | ||
| if state.action == "action_required" { | ||
| state.action = "stop_monitoring_success".to_string(); | ||
| } | ||
| Err(e) => return Err(format!("claude プロセスの待機に失敗: {}", e)), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: check-ci-coderabbit が CI 関連で action_required を返すケースがあるか確認
rg -n 'action_required' src/check-ci-coderabbit/src/main.rsRepository: aloekun/claude-code-hook-test
Length of output: 875
🏁 Script executed:
#!/bin/bash
# Get context around lines 376-377 and 398-399 to see when action_required is returned
sed -n '370,405p' src/check-ci-coderabbit/src/main.rs | cat -nRepository: aloekun/claude-code-hook-test
Length of output: 1507
🏁 Script executed:
#!/bin/bash
# Get context around line 216 to see how action_required is handled
sed -n '210,225p' src/check-ci-coderabbit/src/main.rs | cat -nRepository: aloekun/claude-code-hook-test
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Look for the main function/logic that determines what status to return
rg -B 5 -A 5 'stop_monitoring_failure|stop_monitoring_success' src/check-ci-coderabbit/src/main.rs | head -100Repository: aloekun/claude-code-hook-test
Length of output: 2947
skip_coderabbit 時の action_required 上書きの脆弱性について
skip_coderabbit が有効な場合、action_required を stop_monitoring_success に無条件で書き換えています。
現在の check-ci-coderabbit 実装では CI の action_required 結論は失敗扱い（failure マッチ対象）となるため、decide() 関数で stop_monitoring_failure として返されます。しかし、このロジックは実装の詳細に依存しており、将来的に以下のような変更があった場合に問題が生じる可能性があります：
- CI が
action_requiredを返すケースが追加された場合 action_requiredの処理ロジックが変更された場合- CodeRabbit からの
action_requiredと CI からのaction_requiredを区別する必要が生じた場合
上書きロジックが両者を区別しないため、本来失敗扱いすべき CI の action_required が成功扱いになる可能性があります。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks-post-pr-monitor/src/main.rs` around lines 573 - 577, The current
unconditional overwrite of state.action from "action_required" to
"stop_monitoring_success" when skip_coderabbit is enabled is unsafe; update the
logic to only convert CodeRabbit-originated "action_required" to
"stop_monitoring_success" by introducing and checking an explicit provenance
flag (e.g., state.action_source == "coderabbit" or state.action_from_coderabbit
boolean) before assigning, and set that provenance when the state is
constructed/parsing results (where check-ci-coderabbit / decide() results are
produced); ensure decide() and any code paths that create State populate the new
provenance field so CI-originated "action_required" still maps to
stop_monitoring_failure.
This was referenced Apr 5, 2026
This was referenced Apr 15, 2026
5 tasks
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
claude -p --resumeが VSCode 拡張セッションで動作しない問題を解決pr-monitor-state.jsonに書き出す方式に移行Changes
hooks-post-pr-monitor.exe: claude -p 削除、daemon + state file に全面書き換えhooks-session-start.exe: .session-id の「先勝ち」→「同一IDスキップ」方式package.json: mark-notified, check-monitor スクリプト追加.gitignore: ランタイムファイル除外Test plan
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores