test: E2E daemon + state file 検証 (close without merge) - #23
Closed
aloekun wants to merge 1 commit into
Closed
Conversation
claude -p --resume が VSCode 拡張セッションで動作しない問題を解決。 外部 daemon が監視を完結させ、結果を pr-monitor-state.json に書き出す方式に変更。 CronCreate は UX 最適化レイヤーとして維持。
📝 WalkthroughWalkthroughPR のメインの変更は、PR 監視メカニズムを Claude 駆動の CronCreate ジョブベースから、バックグラウンドデーモンが状態ファイルに監視結果を書き込む永続化ベースへ移行します。設定、ドキュメント、ツール、フック実装を更新し、セッション ID の書き込みポリシーも変更します。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as ユーザー
participant PR as Post-PR Monitor<br/>(hooks-post-pr-monitor)
participant Daemon as Daemon<br/>プロセス
participant Checker as check-ci-<br/>coderabbit.exe
participant State as State File<br/>(.claude/pr-monitor-<br/>state.json)
participant Claude as Claude<br/>CronCreate
User->>PR: pnpm push/pnpm pr-create
PR->>PR: daemon を起動
PR->>State: 初期状態を書き込み
PR->>Claude: CronCreate 指示を出力<br/>(cat state file)
PR-->>User: リターン
loop Daemon ポーリング
Daemon->>State: 現在の状態を読み込み
alt action が continue_monitoring
Daemon->>Checker: check-ci-coderabbit.exe を実行
Checker-->>Daemon: CI/Coderabbit 結果を返す
Daemon->>State: 結果で状態を更新
Daemon->>Daemon: スリープ (定期間隔)
else action が終了/max_duration 到達
Daemon->>Daemon: daemon を終了
end
end
Claude->>State: CronCreate が state をポーリング
State-->>Claude: 監視結果を返す
Claude->>Claude: 完了/タイムアウト/エラーの場合<br/>Cron ジョブを削除
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: 8
🤖 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-config.toml:
- Around line 116-123: Update the SessionStart explanatory block to reflect the
new design by removing references to the old resume flow (specifically
".session-id" and the "claude -p --resume" invocation) and replacing them with
the new session startup semantics used elsewhere in this config (keep CronCreate
and pr-monitor-state.json details consistent). Locate the SessionStart heading
and its first lines that mention ".session-id" and "claude -p --resume", delete
or reword those lines to describe the current startup mechanism and how the
daemon reads/writes .claude/pr-monitor-state.json so the file’s explanations no
longer conflict.
In `@docs/adr/adr-009-post-pr-monitor.md`:
- Around line 147-153: The ADR currently documents the daemon + state-file
design in the prose but still displays the old "claude -p" flow in the
architecture/flow diagram section; update that flow diagram so it depicts the
new daemon → .claude/pr-monitor-state.json → Claude (state-file read) flow, mark
CronCreate as an optional UX layer that only cats the state file, and remove or
replace any arrows/labels that show external processes injecting state into
Claude or starting from `claude -p` so the diagram matches the new design.
In `@src/hooks-post-pr-monitor/src/main.rs`:
- Around line 641-653: The code currently spawns the daemon (spawn_daemon)
before persisting the initial state, risking a race where the daemon exits if
the state file is missing; change the flow to persist an initial state first
(set state.daemon_status = "starting" or similar, ensure state.daemon_pid = None
and state.summary appropriate) by calling write_state(&state) and checking its
result, then call spawn_daemon(&state_path), update state.daemon_pid /
state.daemon_status / state.summary based on spawn outcome, and call
write_state(&state) again to persist the updated state; alternatively, if you
prefer daemon-side changes, add short retry logic in the daemon to wait for the
state file, but do not leave the current spawn-before-write ordering.
- Around line 479-527: The config booleans check_ci and check_coderabbit from
monitor_config are never passed to the checker; update the checker_args
construction (the vector named checker_args used before calling run_cmd_direct)
to append the appropriate flags when monitor_config.check_ci and
monitor_config.check_coderabbit are enabled (use
monitor_config.check_ci.unwrap_or(false) /
monitor_config.check_coderabbit.unwrap_or(false) or equivalent), so that
check_ci/check_coderabbit settings read by load_config actually affect the
executed command invoked by run_cmd_direct.
- Around line 529-558: When the checker fails (success == false) or JSON parsing
of output fails, do not silently continue; set state.daemon_status =
"checker_failed", set state.summary to a short message containing the checker
error or parse error and/or the raw output, reset state.notified = false, call
write_state_to(state_file, &state) to persist this failure state, and then exit
the loop (return 0) so the user sees the failure instead of a stale pending
state; update the block around the success/serde_json::from_str handling and use
update_state_from_check_result only on successful parse.
In `@src/hooks-session-start/src/main.rs`:
- Around line 65-70: The comment block describing session-ID handling conflicts
with the file-level header: update either the file header or this local comment
to make the policy consistent; specifically choose whether the system is
"first-win (do not overwrite)" or "overwrite on different ID", then edit the
relevant text in main.rs so the header spec and the session handling comment
around the session_id logic (references: CLAUDE_CODE_SESSION_ID, .session-id,
and the hooks-post-pr-monitor.exe note) state the same policy; if you also
change behavior rather than just text, update the session write logic to
implement the chosen policy and adjust the comments accordingly.
- Around line 251-263: Test session_id_file_empty_is_written only checks
should_write but doesn't verify the post-write file content; modify the test to
perform the actual write when should_write is true (use the same tmp path used
earlier), then read the file back and assert its contents equal the expected
session id (e.g., "session-A"), and keep the existing cleanup (_ =
std::fs::remove_file(&tmp)) — update references in the test function
session_id_file_empty_is_written (tmp, existing, should_write) accordingly.
- Around line 65-74: The module doc says "first-write-wins" but the current
logic (should_write computed from std::fs::read_to_string(&sid_path) and
existing.trim() != session_id) allows overwrites; change the implementation to
match the doc by making should_write true only when the file does not exist
(i.e., Ok(_) => false, Err(_) => true) in the code around
session_id_file_path()/should_write, remove or correct the incorrect comment
about environment-variable precedence, and update the test
session_id_file_empty_is_written to actually write and assert the file contents
at session_id_file_path() rather than only asserting the boolean condition.
🪄 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: 22598ddc-0a7d-4acd-bea3-fd70edb482ad
⛔ Files ignored due to path filters (1)
src/hooks-post-pr-monitor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.claude/hooks-config.tomldocs/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
Comment on lines
+116
to
+123
| # ─── Post-PR Monitor: daemon + state file ─── | ||
| # | ||
| # gh pr create / git push / jj git push / pnpm push 検出後に | ||
| # CI + CodeRabbit の自動モニタリングを CronCreate で開始する。 | ||
| # check-ci-coderabbit.exe によるポーリングで監視し、 | ||
| # 結果の action フィールドに従って Claude が行動する。 | ||
| # pnpm pr-create / pnpm push 実行時に daemon がバックグラウンドで起動し、 | ||
| # check-ci-coderabbit.exe を定期ポーリングして結果を | ||
| # .claude/pr-monitor-state.json に書き出す。 | ||
| # Claude は CronCreate で state file を cat し、action に従って行動する。 | ||
| # CronCreate は UX 最適化レイヤーであり、コア機能ではない。 | ||
| # フォールバック: cat .claude/pr-monitor-state.json で手動確認可能。 |
There was a problem hiding this comment.
同ファイル冒頭の SessionStart 説明がまだ旧 --resume フローです。
Line 11-12 には .session-id と claude -p --resume が残っているので、このセクションだけ新設計に更新すると設定ファイル内の説明が矛盾します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks-config.toml around lines 116 - 123, Update the SessionStart
explanatory block to reflect the new design by removing references to the old
resume flow (specifically ".session-id" and the "claude -p --resume" invocation)
and replacing them with the new session startup semantics used elsewhere in this
config (keep CronCreate and pr-monitor-state.json details consistent). Locate
the SessionStart heading and its first lines that mention ".session-id" and
"claude -p --resume", delete or reword those lines to describe the current
startup mechanism and how the daemon reads/writes .claude/pr-monitor-state.json
so the file’s explanations no longer conflict.
Comment on lines
+147
to
+153
| - **2026-04-05**: `claude -p --resume` → daemon + state file アーキテクチャに変更。 | ||
| VSCode 拡張では `~/.claude/sessions/` に CLI セッションのみ登録されるため、 | ||
| `claude -p --resume <session_id>` で VSCode セッションにアクセスできないことが判明。 | ||
| 外部プロセスから Claude セッションに状態を注入する設計自体がアンチパターンであると認識。 | ||
| 新設計: daemon が外部で監視を完結させ、結果を `.claude/pr-monitor-state.json` に書き出す。 | ||
| Claude は state file を読むだけ。CronCreate は UX 最適化レイヤーとして維持（state file の | ||
| cat のみ実行）し、コア機能ではない。フォールバック: 手動 `cat` で確認可能。 |
There was a problem hiding this comment.
ADR 本文のフロー図も新設計に揃えた方がよいです。
変更履歴は daemon + state file 化を説明していますが、Line 55-83 のアーキテクチャ図はまだ claude -p 起点の旧フローです。今のままだと同じ ADR の中で新旧設計が混在します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/adr-009-post-pr-monitor.md` around lines 147 - 153, The ADR
currently documents the daemon + state-file design in the prose but still
displays the old "claude -p" flow in the architecture/flow diagram section;
update that flow diagram so it depicts the new daemon →
.claude/pr-monitor-state.json → Claude (state-file read) flow, mark CronCreate
as an optional UX layer that only cats the state file, and remove or replace any
arrows/labels that show external processes injecting state into Claude or
starting from `claude -p` so the diagram matches the new design.
Comment on lines
+479
to
+527
| let config = load_config(); | ||
| let monitor_config = config.post_pr_monitor.unwrap_or_default(); | ||
| let poll_interval = monitor_config.poll_interval_secs.unwrap_or(DEFAULT_POLL_INTERVAL); | ||
| let max_duration = monitor_config.max_duration_secs.unwrap_or(DEFAULT_MAX_DURATION); | ||
|
|
||
| let checker = checker_exe_path(); | ||
| if !checker.exists() { | ||
| log_info(&format!("check-ci-coderabbit.exe が見つかりません: {}", checker.display())); | ||
| if let Some(mut state) = read_state_from(state_file) { | ||
| state.daemon_status = "error".to_string(); | ||
| state.summary = "check-ci-coderabbit.exe が見つかりません".to_string(); | ||
| let _ = write_state_to(state_file, &state); | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| let mut child = cmd | ||
| .stdin(std::process::Stdio::piped()) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()) | ||
| .spawn() | ||
| .map_err(|e| format!("claude の起動に失敗: {}", e))?; | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| // stdin にプロンプトを書き込んで閉じる (ドロップで EOF 送信) | ||
| if let Some(mut stdin) = child.stdin.take() { | ||
| stdin | ||
| .write_all(prompt.as_bytes()) | ||
| .map_err(|e| format!("claude の stdin 書き込みに失敗: {}", e))?; | ||
| } | ||
| loop { | ||
| // 1. Read current state (state file 削除検出で graceful exit) | ||
| let mut state = match read_state_from(state_file) { | ||
| Some(s) => s, | ||
| None => { | ||
| log_info("state file が見つかりません、daemon を終了します"); | ||
| return 0; | ||
| } | ||
| }; | ||
|
|
||
| let stdout_handle = drain_pipe(child.stdout.take().unwrap()); | ||
| let stderr_handle = drain_pipe(child.stderr.take().unwrap()); | ||
| // 2. Build checker arguments | ||
| let mut checker_args: Vec<String> = vec![ | ||
| "--push-time".to_string(), | ||
| state.started_at.clone(), | ||
| ]; | ||
| if let Some(ref repo) = state.repo { | ||
| checker_args.push("--repo".to_string()); | ||
| checker_args.push(repo.clone()); | ||
| } | ||
| if let Some(pr) = state.pr { | ||
| checker_args.push("--pr".to_string()); | ||
| checker_args.push(pr.to_string()); | ||
| } | ||
|
|
||
| // タイムアウト付きで完了を待つ | ||
| let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs); | ||
| let timed_out = loop { | ||
| match child.try_wait() { | ||
| Ok(Some(_)) => break false, | ||
| Ok(None) => { | ||
| if std::time::Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| break true; | ||
| } | ||
| std::thread::sleep(Duration::from_millis(100)); | ||
| // 3. Run check-ci-coderabbit.exe | ||
| let (success, output) = run_cmd_direct( | ||
| &checker.to_string_lossy(), | ||
| &[], | ||
| &checker_args, | ||
| DEFAULT_CHECK_TIMEOUT_SECS, | ||
| ); |
There was a problem hiding this comment.
check_ci / check_coderabbit が実行系に反映されていません。
このブロックで checker に渡しているのは --push-time / --repo / --pr だけで、check_ci / check_coderabbit は Line 402-410 の表示文言にしか使われていません。.claude/hooks-config.toml の Line 129-130 は実際の監視スイッチとして説明しているので、現状だと設定値と挙動がずれます。
🤖 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 479 - 527, The config
booleans check_ci and check_coderabbit from monitor_config are never passed to
the checker; update the checker_args construction (the vector named checker_args
used before calling run_cmd_direct) to append the appropriate flags when
monitor_config.check_ci and monitor_config.check_coderabbit are enabled (use
monitor_config.check_ci.unwrap_or(false) /
monitor_config.check_coderabbit.unwrap_or(false) or equivalent), so that
check_ci/check_coderabbit settings read by load_config actually affect the
executed command invoked by run_cmd_direct.
Comment on lines
+529
to
+558
| // 4. Parse output and update state | ||
| if success { | ||
| if let Ok(result) = serde_json::from_str::<serde_json::Value>(&output) { | ||
| update_state_from_check_result(&mut state, &result); | ||
| } | ||
| Err(e) => return Err(format!("claude プロセスの待機に失敗: {}", e)), | ||
| } | ||
| }; | ||
|
|
||
| if timed_out { | ||
| let stdout = stdout_handle.join().unwrap_or_default(); | ||
| let stderr = stderr_handle.join().unwrap_or_default(); | ||
| let combined = combine_output(&stdout, &stderr); | ||
| let mut msg = format!("timed out after {}s", timeout_secs); | ||
| if !combined.is_empty() { | ||
| msg = format!("{}\n{}", msg, combined); | ||
| state.last_checked = Some(utc_now_iso8601()); | ||
| state.notified = false; // 新しいデータを書いたので notified をリセット | ||
|
|
||
| // 5. Check terminal action → exit | ||
| if state.action != "continue_monitoring" { | ||
| state.daemon_status = "completed".to_string(); | ||
| let _ = write_state_to(state_file, &state); | ||
| log_info(&format!("監視完了: action={}, summary={}", state.action, state.summary)); | ||
| return 0; | ||
| } | ||
| return Ok((false, msg)); | ||
| } | ||
|
|
||
| let success = child.wait().map(|s| s.success()).unwrap_or(false); | ||
| let stdout = stdout_handle.join().unwrap_or_default(); | ||
| let stderr = stderr_handle.join().unwrap_or_default(); | ||
| let combined = combine_output(&stdout, &stderr); | ||
| // 6. Check timeout | ||
| if start.elapsed() >= Duration::from_secs(max_duration) { | ||
| state.daemon_status = "timed_out".to_string(); | ||
| state.summary = format!("監視タイムアウト ({}秒)", max_duration); | ||
| let _ = write_state_to(state_file, &state); | ||
| log_info(&format!("監視タイムアウト ({}秒)", max_duration)); | ||
| return 0; | ||
| } | ||
|
|
||
| Ok((success, combined)) | ||
| // 7. Write updated state and sleep | ||
| let _ = write_state_to(state_file, &state); | ||
| std::thread::sleep(Duration::from_secs(poll_interval)); |
There was a problem hiding this comment.
checker 失敗時に stale state のまま監視継続してしまいます。
success=false や JSON パース失敗を無視して last_checked だけ更新しているので、check-ci-coderabbit.exe が壊れていてもユーザーには pending に見えたまま timeout まで原因が出ません。
💡 修正イメージ
- if success {
- if let Ok(result) = serde_json::from_str::<serde_json::Value>(&output) {
- update_state_from_check_result(&mut state, &result);
- }
- }
+ if !success {
+ state.daemon_status = "error".to_string();
+ state.summary = format!("check-ci-coderabbit.exe 失敗: {}", output);
+ let _ = write_state_to(state_file, &state);
+ return 1;
+ }
+
+ let result = match serde_json::from_str::<serde_json::Value>(&output) {
+ Ok(result) => result,
+ Err(e) => {
+ state.daemon_status = "error".to_string();
+ state.summary = format!("checker 出力の JSON パース失敗: {}", e);
+ let _ = write_state_to(state_file, &state);
+ return 1;
+ }
+ };
+ update_state_from_check_result(&mut state, &result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 4. Parse output and update state | |
| if success { | |
| if let Ok(result) = serde_json::from_str::<serde_json::Value>(&output) { | |
| update_state_from_check_result(&mut state, &result); | |
| } | |
| Err(e) => return Err(format!("claude プロセスの待機に失敗: {}", e)), | |
| } | |
| }; | |
| if timed_out { | |
| let stdout = stdout_handle.join().unwrap_or_default(); | |
| let stderr = stderr_handle.join().unwrap_or_default(); | |
| let combined = combine_output(&stdout, &stderr); | |
| let mut msg = format!("timed out after {}s", timeout_secs); | |
| if !combined.is_empty() { | |
| msg = format!("{}\n{}", msg, combined); | |
| state.last_checked = Some(utc_now_iso8601()); | |
| state.notified = false; // 新しいデータを書いたので notified をリセット | |
| // 5. Check terminal action → exit | |
| if state.action != "continue_monitoring" { | |
| state.daemon_status = "completed".to_string(); | |
| let _ = write_state_to(state_file, &state); | |
| log_info(&format!("監視完了: action={}, summary={}", state.action, state.summary)); | |
| return 0; | |
| } | |
| return Ok((false, msg)); | |
| } | |
| let success = child.wait().map(|s| s.success()).unwrap_or(false); | |
| let stdout = stdout_handle.join().unwrap_or_default(); | |
| let stderr = stderr_handle.join().unwrap_or_default(); | |
| let combined = combine_output(&stdout, &stderr); | |
| // 6. Check timeout | |
| if start.elapsed() >= Duration::from_secs(max_duration) { | |
| state.daemon_status = "timed_out".to_string(); | |
| state.summary = format!("監視タイムアウト ({}秒)", max_duration); | |
| let _ = write_state_to(state_file, &state); | |
| log_info(&format!("監視タイムアウト ({}秒)", max_duration)); | |
| return 0; | |
| } | |
| Ok((success, combined)) | |
| // 7. Write updated state and sleep | |
| let _ = write_state_to(state_file, &state); | |
| std::thread::sleep(Duration::from_secs(poll_interval)); | |
| // 4. Parse output and update state | |
| if !success { | |
| state.daemon_status = "error".to_string(); | |
| state.summary = format!("check-ci-coderabbit.exe 失敗: {}", output); | |
| let _ = write_state_to(state_file, &state); | |
| return 1; | |
| } | |
| let result = match serde_json::from_str::<serde_json::Value>(&output) { | |
| Ok(result) => result, | |
| Err(e) => { | |
| state.daemon_status = "error".to_string(); | |
| state.summary = format!("checker 出力の JSON パース失敗: {}", e); | |
| let _ = write_state_to(state_file, &state); | |
| return 1; | |
| } | |
| }; | |
| update_state_from_check_result(&mut state, &result); | |
| state.last_checked = Some(utc_now_iso8601()); | |
| state.notified = false; // 新しいデータを書いたので notified をリセット | |
| // 5. Check terminal action → exit | |
| if state.action != "continue_monitoring" { | |
| state.daemon_status = "completed".to_string(); | |
| let _ = write_state_to(state_file, &state); | |
| log_info(&format!("監視完了: action={}, summary={}", state.action, state.summary)); | |
| return 0; | |
| } | |
| // 6. Check timeout | |
| if start.elapsed() >= Duration::from_secs(max_duration) { | |
| state.daemon_status = "timed_out".to_string(); | |
| state.summary = format!("監視タイムアウト ({}秒)", max_duration); | |
| let _ = write_state_to(state_file, &state); | |
| log_info(&format!("監視タイムアウト ({}秒)", max_duration)); | |
| return 0; | |
| } | |
| // 7. Write updated state and sleep | |
| let _ = write_state_to(state_file, &state); | |
| 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 529 - 558, When the
checker fails (success == false) or JSON parsing of output fails, do not
silently continue; set state.daemon_status = "checker_failed", set state.summary
to a short message containing the checker error or parse error and/or the raw
output, reset state.notified = false, call write_state_to(state_file, &state) to
persist this failure state, and then exit the loop (return 0) so the user sees
the failure instead of a stale pending state; update the block around the
success/serde_json::from_str handling and use update_state_from_check_result
only on successful parse.
Comment on lines
+641
to
+653
| 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); | ||
| log_info(&format!("daemon スポーン失敗: {}", e)); | ||
| } | ||
| } | ||
|
|
||
| let _ = write_state(&state); |
There was a problem hiding this comment.
初期 state を書く前に daemon を起動しているので、起動直後に監視が落ち得ます。
Line 499-504 では state file が無いと daemon が即終了しますが、ここは Line 641 で spawn してから Line 653 で初回 write しています。初期 state を先に永続化するか、daemon 側で state 出現まで短くリトライしないと、監視が一度も始まらない race になります。
🤖 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 641 - 653, The code
currently spawns the daemon (spawn_daemon) before persisting the initial state,
risking a race where the daemon exits if the state file is missing; change the
flow to persist an initial state first (set state.daemon_status = "starting" or
similar, ensure state.daemon_pid = None and state.summary appropriate) by
calling write_state(&state) and checking its result, then call
spawn_daemon(&state_path), update state.daemon_pid / state.daemon_status /
state.summary based on spawn outcome, and call write_state(&state) again to
persist the updated state; alternatively, if you prefer daemon-side changes, add
short retry logic in the daemon to wait for the state file, but do not leave the
current spawn-before-write ordering.
Comment on lines
+65
to
+70
| // 同一 ID スキップ方式: 既に同じ session_id が書き込み済みなら何もしない。 | ||
| // 異なる ID（新セッション or サブセッション）は上書きする。 | ||
| // | ||
| // サブセッション (claude -p) が上書きしても、hooks-post-pr-monitor.exe は | ||
| // 環境変数 CLAUDE_CODE_SESSION_ID を優先して読むため、メインセッションの | ||
| // Bash ツール内では正しい ID が使われる。.session-id はフォールバック用。 |
There was a problem hiding this comment.
コメント方針がファイル先頭の仕様説明と矛盾しています。
この変更ブロックは「異なる ID は上書き」ですが、冒頭ドキュメントは「先勝ち（上書きしない）」の説明のままです。運用ミス防止のため、どちらかに統一してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks-session-start/src/main.rs` around lines 65 - 70, The comment block
describing session-ID handling conflicts with the file-level header: update
either the file header or this local comment to make the policy consistent;
specifically choose whether the system is "first-win (do not overwrite)" or
"overwrite on different ID", then edit the relevant text in main.rs so the
header spec and the session handling comment around the session_id logic
(references: CLAUDE_CODE_SESSION_ID, .session-id, and the
hooks-post-pr-monitor.exe note) state the same policy; if you also change
behavior rather than just text, update the session write logic to implement the
chosen policy and adjust the comments accordingly.
Comment on lines
+65
to
74
| // 同一 ID スキップ方式: 既に同じ session_id が書き込み済みなら何もしない。 | ||
| // 異なる ID（新セッション or サブセッション）は上書きする。 | ||
| // | ||
| // サブセッション (claude -p) が上書きしても、hooks-post-pr-monitor.exe は | ||
| // 環境変数 CLAUDE_CODE_SESSION_ID を優先して読むため、メインセッションの | ||
| // Bash ツール内では正しい ID が使われる。.session-id はフォールバック用。 | ||
| let sid_path = session_id_file_path(); | ||
| let should_write = match std::fs::read_to_string(&sid_path) { | ||
| Ok(existing) => existing.trim().is_empty(), | ||
| Ok(existing) => existing.trim() != session_id, | ||
| Err(_) => true, // ファイルが存在しない |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# session_id の参照優先順位と .session-id 利用箇所を確認
rg -n -C4 'CLAUDE_CODE_SESSION_ID|\.session-id|--resume|session_id'
# post-pr-monitor 関連ファイルを特定
fd -i 'post-pr-monitor|hooks-post-pr-monitor|pr-monitor'Repository: aloekun/claude-code-hook-test
Length of output: 14614
🏁 Script executed:
# Check full test implementation for session_id_file_empty_is_written
sed -n '251,270p' src/hooks-session-start/src/main.rs
# Also check if there are any other write patterns in post-pr-monitor
rg -n 'session.?id|\.session-id' --iglob '*post*' --iglob '*monitor*'Repository: aloekun/claude-code-hook-test
Length of output: 646
🏁 Script executed:
# Find and examine post-pr-monitor to verify environment variable usage
fd -i 'post.*pr.*monitor|hooks.*post.*pr' -type f | head -20Repository: aloekun/claude-code-hook-test
Length of output: 243
🏁 Script executed:
# Find and examine post-pr-monitor to verify environment variable usage
fd -i 'post.*pr.*monitor|hooks.*post.*pr' -t f | head -20Repository: aloekun/claude-code-hook-test
Length of output: 108
🏁 Script executed:
# Search for any references to hooks-post-pr-monitor reading the .session-id file
rg -n 'hooks-post-pr-monitor|\.session-id' docs/adr/adr-009-post-pr-monitor.md
# Also check the hooks-config.toml mentioned in the original review context
fd 'hooks-config.toml' -t fRepository: aloekun/claude-code-hook-test
Length of output: 247
🏁 Script executed:
# Check if hooks-post-pr-monitor exists as a Rust binary in the repo
find . -name '*post*pr*monitor*' -o -name '*pr*monitor*' 2>/dev/null | head -20
# Also check if there's any use of .session-id file in bash scripts
rg '\.session-id' -t bash -t sh
# Look at the ADR document more carefully
cat docs/adr/adr-009-post-pr-monitor.md | head -160Repository: aloekun/claude-code-hook-test
Length of output: 4691
🏁 Script executed:
# Check if hooks-post-pr-monitor is implemented and how it uses .session-id
ls -la src/hooks-post-pr-monitor/
# Look for any Rust code in hooks-post-pr-monitor that references .session-id
rg '\.session-id|session.?id' src/hooks-post-pr-monitor/ -t rustRepository: aloekun/claude-code-hook-test
Length of output: 362
🏁 Script executed:
# Check the main.rs of hooks-post-pr-monitor
cat src/hooks-post-pr-monitor/src/main.rs
# Also verify the module documentation claim about environment variable priority
rg -n 'CLAUDE_CODE_SESSION_ID' -B2 -A2Repository: aloekun/claude-code-hook-test
Length of output: 36397
モジュール文書と実装のポリシーが矛盾しています。
モジュール先頭のドキュメント（12-14行目）は「先勝ち」（ファイルが存在すれば上書きしない）と明記していますが、実装（65-74行目）では existing.trim() != session_id で異なる ID なら常に上書きしています。また、68-70行目の「environment variable を優先して読む」という説明は根拠が不明確です。hooks-post-pr-monitor は最新アーキテクチャ（ADR-009, 2026-04-05）に従い、.session-id ではなく daemon + state file（.claude/pr-monitor-state.json）を使用するため、この説明は正確ではありません。
コメント（65-66行目）か文書（12-14行目）のいずれかを修正し、ポリシーを統一してください。
また、テスト session_id_file_empty_is_written（251-263行目）は条件判定ロジックのみを検証しており、実際のファイル書き込み動作を検証していません。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks-session-start/src/main.rs` around lines 65 - 74, The module doc
says "first-write-wins" but the current logic (should_write computed from
std::fs::read_to_string(&sid_path) and existing.trim() != session_id) allows
overwrites; change the implementation to match the doc by making should_write
true only when the file does not exist (i.e., Ok(_) => false, Err(_) => true) in
the code around session_id_file_path()/should_write, remove or correct the
incorrect comment about environment-variable precedence, and update the test
session_id_file_empty_is_written to actually write and assert the file contents
at session_id_file_path() rather than only asserting the boolean condition.
Comment on lines
+251
to
+263
| fn session_id_file_empty_is_written() { | ||
| let tmp = std::env::temp_dir().join(format!( | ||
| "test-sid-empty-{}", | ||
| std::process::id() | ||
| )); | ||
| let _ = std::fs::write(&tmp, ""); | ||
|
|
||
| // 空ファイル → 書き込むべき ("" != "session-A") | ||
| let existing = std::fs::read_to_string(&tmp).unwrap(); | ||
| let should_write = existing.trim() != "session-A"; | ||
| assert!(should_write); | ||
|
|
||
| let _ = std::fs::remove_file(&tmp); |
There was a problem hiding this comment.
session_id_file_empty_is_written が“書き込み後状態”を検証していません。
should_write の真偽だけだとテスト名どおりの保証にならないため、実際に書いて内容まで assert してください。
修正例
#[test]
fn session_id_file_empty_is_written() {
@@
let existing = std::fs::read_to_string(&tmp).unwrap();
let should_write = existing.trim() != "session-A";
assert!(should_write);
+ let _ = std::fs::write(&tmp, "session-A");
+ let content = std::fs::read_to_string(&tmp).unwrap();
+ assert_eq!(content, "session-A");
let _ = std::fs::remove_file(&tmp);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks-session-start/src/main.rs` around lines 251 - 263, Test
session_id_file_empty_is_written only checks should_write but doesn't verify the
post-write file content; modify the test to perform the actual write when
should_write is true (use the same tmp path used earlier), then read the file
back and assert its contents equal the expected session id (e.g., "session-A"),
and keep the existing cleanup (_ = std::fs::remove_file(&tmp)) — update
references in the test function session_id_file_empty_is_written (tmp, existing,
should_write) accordingly.
Owner
Author
|
E2E テスト完了。daemon + state file アーキテクチャの動作を確認。CodeRabbit 指摘は別途対応。 |
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.
Post-PR Monitor の daemon + state file アーキテクチャ E2E テスト。検証後にクローズします。
Summary by CodeRabbit
リリースノート
New Features
mark-notifiedとcheck-monitorを追加しました。Documentation
Chores