Skip to content

refactor(cli-pr-monitor): モノリシック main.rs をモジュール分割 - #38

Merged
aloekun merged 1 commit into
masterfrom
refactor/cli-pr-monitor-module-split
Apr 15, 2026
Merged

refactor(cli-pr-monitor): モノリシック main.rs をモジュール分割#38
aloekun merged 1 commit into
masterfrom
refactor/cli-pr-monitor-module-split

Conversation

@aloekun

@aloekun aloekun commented Apr 15, 2026

Copy link
Copy Markdown
Owner

Summary\

\

  • 1443行のモノリシック main.rs を push-runner と同じ stages/ 構成に分割\
  • 動作変更なし、全43テストパス、clippy警告ゼロ\
  • cli-pr-monitor takt化 (Phase 1) の前準備
    \

変更内容\


src/cli-pr-monitor/src/ を以下のモジュール構成に分割:

| モジュール | 内容 |
|-----------|------|
| main.rs (62行) | 引数パース + dispatch |
| config.rs | Config, PostPrMonitorConfig, load_config |
| state.rs | PrMonitorState, CiState, CodeRabbitState, read/write |
| log.rs | log_info, truncate_safe |
| runner.rs | run_cmd_direct, drain_pipe, run_gh_quiet, checker_exe_path |
| util.rs | PrInfo, get_pr_info, parse_pr_number_from_url, get_jj_bookmarks |
| stages/create_pr.rs | run_create_pr, TempFile, convert_body_to_file, ensure_head_arg |
| stages/monitor.rs | start_monitoring, run_monitor_only, print_cron_instruction |
| stages/daemon.rs | spawn_daemon, run_daemon |
| stages/mark_notified.rs | run_mark_notified |
\

背景\


cli-pr-monitor の takt 化 (Phase 1) では新しい stages (poll, collect, takt) を追加する。モノリシックな1443行に直接変更を加えると diff が読めなくなるため、先にモジュール分割を行う。
\

Test plan\

\

  • cargo test -- 全43テストパス\
  • cargo clippy -- -D warnings -- 警告ゼロ\
  • cargo fmt --check -- フォーマット済み\
  • pnpm build:cli-pr-monitor -- release ビルド成功\
  • takt push-runner 経由で push 成功

Summary by CodeRabbit

リリースノート

  • 新機能
    • PR監視機能を追加しました。設定ファイルで有効化でき、ポーリング間隔と最大監視期間をカスタマイズできます。
    • バックグラウンドデーモンモードでPRの自動監視をサポートしました。
    • CI と CodeRabbit ステータスチェック機能を追加しました。
    • 通知状態を管理する機能を追加しました。

1443行の main.rs を push-runner と同じ stages/ 構成に分割:
- config.rs: Config, PostPrMonitorConfig, load_config
- state.rs: PrMonitorState, CiState, CodeRabbitState, read/write
- log.rs: log_info, truncate_safe
- runner.rs: run_cmd_direct, drain_pipe, run_gh_quiet, checker_exe_path
- util.rs: PrInfo, get_pr_info, parse_pr_number_from_url, get_jj_bookmarks, epoch/utc helpers
- stages/create_pr.rs: run_create_pr, TempFile, convert_body_to_file, ensure_head_arg
- stages/monitor.rs: start_monitoring, run_monitor_only, print_cron_instruction
- stages/daemon.rs: spawn_daemon, run_daemon
- stages/mark_notified.rs: run_mark_notified

動作変更なし。全43テストパス、clippy警告ゼロ。
cli-pr-monitor takt化 (Phase 1) の前準備。
@coderabbitai

coderabbitai Bot commented Apr 15, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Rust製CLIツール cli-pr-monitor を、単一の main.rs から複数の専門化されたモジュール(configlogrunnerstateutilstages)に再構成しました。PR作成、監視設定、デーモン実行、状態管理に関する責務を分離し、長期実行監視と通知機能を実装しています。

Changes

Cohort / File(s) Summary
Configuration & Logging
src/cli-pr-monitor/src/config.rs, src/cli-pr-monitor/src/log.rs
TOML設定ファイル(hooks-config.toml)の読み込みと デフォルト値を定義。ポーリング間隔とタイムアウト定数を追加。情報ログとUTF-8安全な文字列切詰めヘルパーを実装。
External Command Execution
src/cli-pr-monitor/src/runner.rs
パイプドリェン、タイムアウト管理、標準出力/標準エラー統合を備えた外部コマンド実行ユーティリティを追加。gh コマンドと check-ci-coderabbit.exe の実行を支援。
PR & Repository Utilities
src/cli-pr-monitor/src/util.rs
PR情報取得(GitHub APIおよび jj ブックマーク経由)、PR番号URL解析、タイムスタンプ形式化(ISO8601)機能を実装。
State Management
src/cli-pr-monitor/src/state.rs
PR監視状態を JSON で永続化する PrMonitorState 構造を定義。CI/CodeRabbit サブ状態、検査結果からの状態更新ロジックを含む。
Stage Handlers
src/cli-pr-monitor/src/stages/mod.rs, src/cli-pr-monitor/src/stages/create_pr.rs, src/cli-pr-monitor/src/stages/daemon.rs, src/cli-pr-monitor/src/stages/mark_notified.rs, src/cli-pr-monitor/src/stages/monitor.rs
PR作成ステージ(マルチラインボディ処理、--head 正規化)、デーモン実行ループ(ポーリング、タイムアウト、状態更新)、監視開始、通知マーク機能を実装。
Main Entry Point
src/cli-pr-monitor/src/main.rs
モジュール構造に切り替え(-1388行)。 --daemon, --mark-notified, --monitor-only へのディスパッチロジックを保持し、各ステージハンドラへ委譲。

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed PRタイトルは「モノリシック main.rs をモジュール分割」と明確にリファクタリングの主な変更内容を説明しており、1443行のmain.rsを複数のモジュールに分割するという中核的な変更を正確に反映している。
Docstring Coverage ✅ Passed Docstring coverage is 81.01% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/cli-pr-monitor/src/main.rs (1)

38-60: ⚠️ Potential issue | 🟠 Major

制御フラグの判定は -- より前の引数だけを見るべきです。

Line 38-60 は全引数を any(...) で見ているので、gh pr create にそのまま転送したい値がたまたま --monitor-only / --daemon / --mark-notified と一致すると別モードに誤分岐します。先に -- で CLI 用引数と転送用引数を分離してから判定してください。

💡 修正案
 fn main() {
     let args: Vec<String> = std::env::args().collect();
+    let split = args.iter().position(|a| a == "--").unwrap_or(args.len());
+    let cli_args = &args[1..split];
+    let gh_args: Vec<String> = if split < args.len() {
+        args[split + 1..].to_vec()
+    } else {
+        cli_args.to_vec()
+    };
 
-    if args.iter().any(|a| a == "--daemon") {
-        let state_file = args
+    if cli_args.iter().any(|a| a == "--daemon") {
+        let state_file = cli_args
             .iter()
             .position(|a| a == "--state-file")
-            .and_then(|i| args.get(i + 1))
+            .and_then(|i| cli_args.get(i + 1))
             .map(PathBuf::from)
             .unwrap_or_else(state_file_path);
         std::process::exit(run_daemon(&state_file));
     }
 
-    if args.iter().any(|a| a == "--mark-notified") {
+    if cli_args.iter().any(|a| a == "--mark-notified") {
         std::process::exit(run_mark_notified());
     }
 
-    if args.iter().any(|a| a == "--monitor-only") {
+    if cli_args.iter().any(|a| a == "--monitor-only") {
         std::process::exit(run_monitor_only());
     }
-
-    let gh_args: Vec<String> = if let Some(pos) = args.iter().position(|a| a == "--") {
-        args[pos + 1..].to_vec()
-    } else {
-        args[1..].to_vec()
-    };
 
     std::process::exit(run_create_pr(&gh_args));
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli-pr-monitor/src/main.rs` around lines 38 - 60, The flag checks
currently examine all args which can misclassify flags that appear after the
"--" passthrough; change logic to split args at the "--" first (compute pos =
args.iter().position(|a| a == "--") and set cli_args = &args[0..pos] or
&args[..] if None), then run the existing flag checks against cli_args (use
cli_args.iter().any(...) for "--daemon", "--mark-notified", "--monitor-only"),
resolve state_file from cli_args when calling run_daemon (use the same
position+get logic on cli_args and state_file_path fallback), and only after
those checks build gh_args from the tail (args[pos+1..] or args[1..]) to forward
to gh pr create; keep run_daemon, run_mark_notified, run_monitor_only and
state_file_path calls unchanged except for using cli_args.
🧹 Nitpick comments (3)
src/cli-pr-monitor/src/runner.rs (1)

46-74: try_wait で取得した終了ステータスが破棄されている

Line 49 で try_wait()Ok(Some(_)) を返した時点で終了ステータスが取得できていますが、それを破棄しています。その後 Line 73 で再度 child.wait() を呼び出しているため、冗長な呼び出しになっています。

また、Line 64 で stdout と stderr を直接結合していますが、combine_output 関数(Line 78-86)は改行区切りで結合するロジックを持っています。一貫性のために combine_output を使用するか、または不要であれば #[allow(dead_code)] を付けた combine_output を削除することを検討してください。

♻️ 終了ステータスを効率的に取得する修正案
     let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs);
-    let timed_out = loop {
+    let exit_status = loop {
         match child.try_wait() {
-            Ok(Some(_)) => break false,
+            Ok(Some(status)) => break Some(status),
             Ok(None) => {
                 if std::time::Instant::now() >= deadline {
                     let _ = child.kill();
                     let _ = child.wait();
-                    break true;
+                    break None; // timed out
                 }
                 std::thread::sleep(Duration::from_millis(100));
             }
-            Err(_) => break true,
+            Err(_) => break None,
         }
     };

     let stdout_text = stdout_handle.join().unwrap_or_default();
     let stderr_text = stderr_handle.join().unwrap_or_default();
-    let combined = format!("{}{}", stdout_text, stderr_text).trim().to_string();
+    let combined = combine_output(&stdout_text, &stderr_text);

-    if timed_out {
+    if exit_status.is_none() {
         return (
             false,
             format!("{}\n(timeout after {}s)", combined, timeout_secs),
         );
     }

-    let code = child.wait().map(|s| s.code().unwrap_or(1)).unwrap_or(1);
+    let code = exit_status.unwrap().code().unwrap_or(1);
     (code == 0, combined)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli-pr-monitor/src/runner.rs` around lines 46 - 74, The code discards the
exit status returned by child.try_wait() (match arm Ok(Some(_))) and later
redundantly calls child.wait(); change the loop to capture the ExitStatus from
try_wait (e.g., store it in a variable like exit_status) and break with that
status or with a timeout marker, then avoid the second child.wait() by using the
captured status to determine success; also stop concatenating stdout/stderr
manually and either call the existing combine_output(stdout_handle.join(),
stderr_handle.join()) to produce the combined string or remove/annotate the
unused combine_output function with #[allow(dead_code)] so behavior is
consistent (referencing child.try_wait, child.wait, stdout_handle,
stderr_handle, combine_output).
src/cli-pr-monitor/src/stages/daemon.rs (1)

38-51: 非 Windows 版 spawn_daemon はデーモン化が不完全

非 Windows 版では setsid や double-fork パターンが実装されていないため、真のデーモン化ではありません。親プロセスが終了した際に、シグナルの伝播や制御端末の問題が発生する可能性があります。

PR の目的が Windows 環境での使用であれば問題ありませんが、Linux/macOS でも使用する場合は検討が必要です。

💡 Unix 系での改善案 (daemonize クレート使用)

daemonize または nix クレートを使用した本格的なデーモン化の実装を検討できます。ただし、現在のシンプルな実装でも stdin/stdout/stderr を null に設定しているため、多くのユースケースで動作するはずです。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli-pr-monitor/src/stages/daemon.rs` around lines 38 - 51, spawn_daemon
currently just spawns the same executable with stdio nulled, which is not a true
Unix daemon (missing setsid/double-fork), so implement proper daemonization in
the non-Windows path: modify spawn_daemon to perform a real daemonize (use the
daemonize crate or nix::unistd to call setsid and perform the double-fork
pattern) before returning the child PID or change the function to run the
daemonization in the child process created by Command; reference the
spawn_daemon function and replace the simple Command::new(&exe)...spawn() flow
with a proper daemonization sequence (or integrate daemonize::Daemonize) and
ensure errors are mapped to the same Result<String> error format.
src/cli-pr-monitor/src/stages/create_pr.rs (1)

146-152: has_head_flag の二重呼び出しは軽微な非効率

ensure_head_arg 内 (Line 125) で既に has_head_flag が呼び出されています。ここでの事前チェックはログ出力のためと理解できますが、bookmark が空の場合にも不要なログが出力されない点を確認してください。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli-pr-monitor/src/stages/create_pr.rs` around lines 146 - 152, Remove
the redundant has_head_flag check by making ensure_head_arg return whether it
actually inserted a head (e.g., change ensure_head_arg(final_args, &bookmarks)
-> (String, bool) or -> (String, Option<String>)); then at the call site call
get_jj_bookmarks(), assign (final_args, added) = ensure_head_arg(final_args,
&bookmarks), and only call log_info using bookmarks.first() when added is true
and bookmarks.first() is Some; update the ensure_head_arg implementation to
perform the single has_head_flag check internally and signal back whether it
modified final_args.
🤖 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/cli-pr-monitor/src/runner.rs`:
- Around line 109-115: The checker_exe_path() function currently hardcodes a
".exe" suffix; change it to build the filename without a fixed extension and
append ".exe" only on Windows (e.g., using cfg!(windows) to detect platform) so
the produced PathBuf is correct cross-platform; update the run_daemon() error
messages that mention the executable name (references: checker_exe_path() and
run_daemon()) to use the name "check-ci-coderabbit" without a ".exe" suffix
(replace the messages at the three spots currently referencing ".exe" with the
suggested strings).

In `@src/cli-pr-monitor/src/state.rs`:
- Around line 104-109: The code currently assigns state.ci and state.coderabbit
to serde_json::from_value(...).ok() whenever the keys exist, which can clear
previously valid state on deserialization failure; change the logic to only
overwrite when deserialization succeeds: use result.get("ci") and
result.get("coderabbit") to retrieve the values, attempt
serde_json::from_value(...) and only set state.ci and state.coderabbit when the
from_value call returns Ok(parsed) (preserving the existing Option on Err),
mirroring how findings are handled so invalid payloads don't revert prior state.

---

Outside diff comments:
In `@src/cli-pr-monitor/src/main.rs`:
- Around line 38-60: The flag checks currently examine all args which can
misclassify flags that appear after the "--" passthrough; change logic to split
args at the "--" first (compute pos = args.iter().position(|a| a == "--") and
set cli_args = &args[0..pos] or &args[..] if None), then run the existing flag
checks against cli_args (use cli_args.iter().any(...) for "--daemon",
"--mark-notified", "--monitor-only"), resolve state_file from cli_args when
calling run_daemon (use the same position+get logic on cli_args and
state_file_path fallback), and only after those checks build gh_args from the
tail (args[pos+1..] or args[1..]) to forward to gh pr create; keep run_daemon,
run_mark_notified, run_monitor_only and state_file_path calls unchanged except
for using cli_args.

---

Nitpick comments:
In `@src/cli-pr-monitor/src/runner.rs`:
- Around line 46-74: The code discards the exit status returned by
child.try_wait() (match arm Ok(Some(_))) and later redundantly calls
child.wait(); change the loop to capture the ExitStatus from try_wait (e.g.,
store it in a variable like exit_status) and break with that status or with a
timeout marker, then avoid the second child.wait() by using the captured status
to determine success; also stop concatenating stdout/stderr manually and either
call the existing combine_output(stdout_handle.join(), stderr_handle.join()) to
produce the combined string or remove/annotate the unused combine_output
function with #[allow(dead_code)] so behavior is consistent (referencing
child.try_wait, child.wait, stdout_handle, stderr_handle, combine_output).

In `@src/cli-pr-monitor/src/stages/create_pr.rs`:
- Around line 146-152: Remove the redundant has_head_flag check by making
ensure_head_arg return whether it actually inserted a head (e.g., change
ensure_head_arg(final_args, &bookmarks) -> (String, bool) or -> (String,
Option<String>)); then at the call site call get_jj_bookmarks(), assign
(final_args, added) = ensure_head_arg(final_args, &bookmarks), and only call
log_info using bookmarks.first() when added is true and bookmarks.first() is
Some; update the ensure_head_arg implementation to perform the single
has_head_flag check internally and signal back whether it modified final_args.

In `@src/cli-pr-monitor/src/stages/daemon.rs`:
- Around line 38-51: spawn_daemon currently just spawns the same executable with
stdio nulled, which is not a true Unix daemon (missing setsid/double-fork), so
implement proper daemonization in the non-Windows path: modify spawn_daemon to
perform a real daemonize (use the daemonize crate or nix::unistd to call setsid
and perform the double-fork pattern) before returning the child PID or change
the function to run the daemonization in the child process created by Command;
reference the spawn_daemon function and replace the simple
Command::new(&exe)...spawn() flow with a proper daemonization sequence (or
integrate daemonize::Daemonize) and ensure errors are mapped to the same
Result<String> error format.
🪄 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: c15a06cc-a52b-4f64-91be-ad26d1f9c743

📥 Commits

Reviewing files that changed from the base of the PR and between dfdad6b and 5e96616.

📒 Files selected for processing (11)
  • src/cli-pr-monitor/src/config.rs
  • src/cli-pr-monitor/src/log.rs
  • src/cli-pr-monitor/src/main.rs
  • src/cli-pr-monitor/src/runner.rs
  • src/cli-pr-monitor/src/stages/create_pr.rs
  • src/cli-pr-monitor/src/stages/daemon.rs
  • src/cli-pr-monitor/src/stages/mark_notified.rs
  • src/cli-pr-monitor/src/stages/mod.rs
  • src/cli-pr-monitor/src/stages/monitor.rs
  • src/cli-pr-monitor/src/state.rs
  • src/cli-pr-monitor/src/util.rs

Comment on lines +109 to +115
pub(crate) fn checker_exe_path() -> PathBuf {
std::env::current_exe()
.unwrap_or_default()
.parent()
.unwrap_or(Path::new("."))
.join("check-ci-coderabbit.exe")
}

@coderabbitai coderabbitai Bot Apr 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if there are platform-specific build configurations
rg -n "target_os" src/cli-pr-monitor/
# Check Cargo.toml for platform-specific settings
cat src/cli-pr-monitor/Cargo.toml

Repository: aloekun/claude-code-hook-test

Length of output: 508


🏁 Script executed:

cat -n src/cli-pr-monitor/src/stages/daemon.rs

Repository: aloekun/claude-code-hook-test

Length of output: 7939


.exe 拡張子がハードコードされている

checker_exe_path().exe 拡張子をハードコードしており、run_daemon() 関数(プラットフォーム固有でない)から呼び出されています。非 Windows 環境で実行された場合、実行可能ファイルのパスが正しく解決されません。

また、run_daemon() の 70、75、117 行目のエラーメッセージも .exe を明記しているため、同様に修正が必要です。

修正案
 pub(crate) fn checker_exe_path() -> PathBuf {
+    let exe_name = if cfg!(target_os = "windows") {
+        "check-ci-coderabbit.exe"
+    } else {
+        "check-ci-coderabbit"
+    };
     std::env::current_exe()
         .unwrap_or_default()
         .parent()
         .unwrap_or(Path::new("."))
-        .join("check-ci-coderabbit.exe")
+        .join(exe_name)
 }

エラーメッセージも同様に修正してください:

  • 70 行目: "check-ci-coderabbit が見つかりません"
  • 75 行目: "check-ci-coderabbit が見つかりません"
  • 117 行目: "check-ci-coderabbit 失敗"
📝 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
pub(crate) fn checker_exe_path() -> PathBuf {
std::env::current_exe()
.unwrap_or_default()
.parent()
.unwrap_or(Path::new("."))
.join("check-ci-coderabbit.exe")
}
pub(crate) fn checker_exe_path() -> PathBuf {
let exe_name = if cfg!(target_os = "windows") {
"check-ci-coderabbit.exe"
} else {
"check-ci-coderabbit"
};
std::env::current_exe()
.unwrap_or_default()
.parent()
.unwrap_or(Path::new("."))
.join(exe_name)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli-pr-monitor/src/runner.rs` around lines 109 - 115, The
checker_exe_path() function currently hardcodes a ".exe" suffix; change it to
build the filename without a fixed extension and append ".exe" only on Windows
(e.g., using cfg!(windows) to detect platform) so the produced PathBuf is
correct cross-platform; update the run_daemon() error messages that mention the
executable name (references: checker_exe_path() and run_daemon()) to use the
name "check-ci-coderabbit" without a ".exe" suffix (replace the messages at the
three spots currently referencing ".exe" with the suggested strings).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ご指摘ありがとうございます。現状このプロジェクトは Windows 環境でのみ使用しており、クロスプラットフォーム対応の必要性がないため、今回は対応不要と判断しました。将来的にマルチプラットフォーム対応が必要になった際に改めて検討します。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aloekun、ご説明いただきありがとうございます。Windows 環境専用であれば、.exe のハードコードは問題ありません。将来マルチプラットフォーム対応が必要になった際にご検討ください。

(´• ω •`) ご理解いただきありがとうございます。


✏️ Learnings added
Learnt from: aloekun
Repo: aloekun/claude-code-hook-test PR: 38
File: src/cli-pr-monitor/src/runner.rs:109-115
Timestamp: 2026-04-15T05:44:09.231Z
Learning: The `cli-pr-monitor` tool in `aloekun/claude-code-hook-test` is Windows-only. Hardcoded `.exe` suffixes (e.g., in `checker_exe_path()` in `src/cli-pr-monitor/src/runner.rs` and error messages in `src/cli-pr-monitor/src/stages/daemon.rs`) are intentional. Cross-platform support is not required.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: aloekun
Repo: aloekun/claude-code-hook-test PR: 36
File: docs/adr/adr-016-long-running-command-strategy.md:32-44
Timestamp: 2026-04-14T15:56:49.970Z
Learning: In `claude-code-hook-test`, the push-runner (`src/cli-push-runner`) has two independent timeout layers:
1. Bash tool `timeout` (e.g., 600000ms): applies to the entire `pnpm push` process tree.
2. push-runner `push.timeout` / `DEFAULT_PUSH_TIMEOUT_SECS` (300s): applies only to the `jj git push` command inside `run_push()` in `src/cli-push-runner/src/stages/push.rs`. These two layers do not conflict.

Learnt from: CR
Repo: aloekun/claude-code-hook-test PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-14T18:21:03.822Z
Learning: Implement Post-PR Monitor for automatic CI and CodeRabbit monitoring after push/PR creation (ADR-009)

Comment on lines +104 to +109
if let Some(ci_val) = result.get("ci") {
state.ci = serde_json::from_value(ci_val.clone()).ok();
}
if let Some(cr_val) = result.get("coderabbit") {
state.coderabbit = serde_json::from_value(cr_val.clone()).ok();
}

@coderabbitai coderabbitai Bot Apr 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

不正な ci / coderabbit ペイロードで既存 state を消さないでください。

Line 104-109 はキーが存在するだけで serde_json::from_value(...).ok() を代入しているため、チェック結果が一時的に壊れているだけで直前まで保持していた ci / coderabbitNone に戻ります。findings と同じく、デシリアライズ成功時だけ上書きしないと監視状態が後退します。

💡 修正案
     if let Some(ci_val) = result.get("ci") {
-        state.ci = serde_json::from_value(ci_val.clone()).ok();
+        if let Ok(ci) = serde_json::from_value(ci_val.clone()) {
+            state.ci = Some(ci);
+        }
     }
     if let Some(cr_val) = result.get("coderabbit") {
-        state.coderabbit = serde_json::from_value(cr_val.clone()).ok();
+        if let Ok(coderabbit) = serde_json::from_value(cr_val.clone()) {
+            state.coderabbit = Some(coderabbit);
+        }
     }
📝 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
if let Some(ci_val) = result.get("ci") {
state.ci = serde_json::from_value(ci_val.clone()).ok();
}
if let Some(cr_val) = result.get("coderabbit") {
state.coderabbit = serde_json::from_value(cr_val.clone()).ok();
}
if let Some(ci_val) = result.get("ci") {
if let Ok(ci) = serde_json::from_value(ci_val.clone()) {
state.ci = Some(ci);
}
}
if let Some(cr_val) = result.get("coderabbit") {
if let Ok(coderabbit) = serde_json::from_value(cr_val.clone()) {
state.coderabbit = Some(coderabbit);
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli-pr-monitor/src/state.rs` around lines 104 - 109, The code currently
assigns state.ci and state.coderabbit to serde_json::from_value(...).ok()
whenever the keys exist, which can clear previously valid state on
deserialization failure; change the logic to only overwrite when deserialization
succeeds: use result.get("ci") and result.get("coderabbit") to retrieve the
values, attempt serde_json::from_value(...) and only set state.ci and
state.coderabbit when the from_value call returns Ok(parsed) (preserving the
existing Option on Err), mirroring how findings are handled so invalid payloads
don't revert prior state.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有効な指摘です。serde_json::from_value(...).ok()None を返した場合に既存の state を上書きしてしまう問題は認識しました。ただし本 PR はリファクタ (モジュール分割) のみを目的としており、動作変更は含めない方針です。次の PR (cli-pr-monitor takt 化 Phase 1) で修正を反映します。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aloekun、ご確認ありがとうございます。リファクタ PR で動作変更を含めない方針は理解しました。

次の PR での修正を忘れずに追跡できるよう、GitHub Issue を作成しましょうか?作成する場合、以下の内容で登録します:

  • タイトル: fix(cli-pr-monitor): update_state_from_check_result でデシリアライズ失敗時に既存 state が None に戻る問題を修正
  • 内容: state.rsupdate_state_from_check_result において、ci / coderabbit キーが存在するだけで serde_json::from_value(...).ok() を代入しているため、一時的に壊れたペイロードで既存 state が None にリセットされる問題。デシリアライズ成功時のみ上書きするよう修正が必要。

ご希望の場合はお知らせください。


🧠 Learnings used
Learnt from: CR
Repo: aloekun/claude-code-hook-test PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-14T18:21:03.822Z
Learning: Implement Post-PR Monitor for automatic CI and CodeRabbit monitoring after push/PR creation (ADR-009)

@aloekun
aloekun merged commit 5fb3f0d into master Apr 15, 2026
1 check passed
@aloekun
aloekun deleted the refactor/cli-pr-monitor-module-split branch April 15, 2026 05:46
aloekun added a commit that referenced this pull request Apr 15, 2026
update_state_from_check_result で serde_json::from_value(...).ok() を
if let Ok(...) パターンに変更。デシリアライズ失敗時に既存の
state.ci / state.coderabbit が None で上書きされる問題を修正。

CodeRabbit PR #38 指摘への対応。テスト2件追加。
aloekun added a commit that referenced this pull request Apr 15, 2026
update_state_from_check_result で serde_json::from_value(...).ok() を
if let Ok(...) パターンに変更。デシリアライズ失敗時に既存の
state.ci / state.coderabbit が None で上書きされる問題を修正。

CodeRabbit PR #38 指摘への対応。テスト2件追加。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant