diff --git a/.changeset/report-to-stderr.md b/.changeset/report-to-stderr.md new file mode 100644 index 0000000..2f98503 --- /dev/null +++ b/.changeset/report-to-stderr.md @@ -0,0 +1,7 @@ +--- +'@systemfsoftware/claude-code-comment-checker': minor +--- + +Flagged-comment reports now go to stderr instead of stdout, so the agent that made the edit actually receives them. A blocked verdict previously exited 2 with its report on stdout, which the hook contract discards, so the block arrived carrying no explanation of what was flagged. + +If you capture reports yourself, read stderr. Exit codes are unchanged: 0 when nothing is flagged, 2 when something is. diff --git a/Cargo.lock b/Cargo.lock index 5e4f3b0..13d8233 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -201,7 +201,7 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "claude-code-comment-checker" -version = "0.1.7" +version = "0.1.8" dependencies = [ "clap", "proptest", diff --git a/README.md b/README.md index 977bebd..a01024f 100644 --- a/README.md +++ b/README.md @@ -11,10 +11,10 @@ It never edits your files, never sends code anywhere, and exits deterministicall pnpm add -g @systemfsoftware/claude-code-comment-checker ``` -Pipe a `Write` payload to the binary and it reports what it would block: +Pipe a `Write` payload to the binary and it reports what it would block. The report goes to stderr, so `2>&1` keeps it when you redirect: ```bash -$ echo '{"tool_name":"Write","tool_input":{"file_path":"src/load_config.py","content":"import json\n\ndef load_config(path):\n # Parse the config file\n data = json.load(open(path))\n # TODO: fix this later\n # print(data)\n return data\n"}}' | comment-checker +$ echo '{"tool_name":"Write","tool_input":{"file_path":"src/load_config.py","content":"import json\n\ndef load_config(path):\n # Parse the config file\n data = json.load(open(path))\n # TODO: fix this later\n # print(data)\n return data\n"}}' | comment-checker 2>&1 An automated reviewer flagged 3 comment(s) in src/load_config.py as unnecessary. Each is stated with the specific reason it should be removed. Do not @@ -30,7 +30,7 @@ one, make the code self-explanatory instead — better names, extraction, a clearer type — and do not re-add the comment. ``` -Exit status is the contract: `0` on pass, `2` when comments are flagged. +Exit status is the contract: `0` on pass, `2` when comments are flagged. The report is written to stderr, because that is the stream the host forwards to the model on exit 2 while stdout is discarded — see the [Claude Code hooks reference](https://code.claude.com/docs/en/hooks#exit-code-2). ## Install @@ -149,8 +149,8 @@ Deterministic, and the reason sessions and scripts can gate on the hook: | Code | Meaning | |---|---| -| 0 | Pass — no unnecessary comments found; empty input or unparseable payload also passes | -| 2 | Block — one or more unnecessary comments; report on stdout | +| 0 | Pass — no unnecessary comments found; empty input or unparseable payload also passes. Skip note on stdout | +| 2 | Block — one or more unnecessary comments; report on stderr, the stream the host forwards to the model | ## FAQ diff --git a/crates/comment-checker/src/main.rs b/crates/comment-checker/src/main.rs index 8b8b527..f06da07 100644 --- a/crates/comment-checker/src/main.rs +++ b/crates/comment-checker/src/main.rs @@ -22,12 +22,20 @@ fn main() -> ExitCode { match check(&input, cli.prompt.as_deref().unwrap_or_default()) { Outcome::Pass { note } => { - print!("{note}"); + emit_to_debug_log(¬e); ExitCode::from(0) } Outcome::Block { report } => { - print!("{report}"); + emit_to_model(&report); ExitCode::from(2) } } } + +fn emit_to_model(report: &str) { + eprint!("{report}"); +} + +fn emit_to_debug_log(note: &str) { + print!("{note}"); +} diff --git a/crates/comment-checker/tests/exit_codes.rs b/crates/comment-checker/tests/exit_codes.rs index 97db122..d2f6fd3 100644 --- a/crates/comment-checker/tests/exit_codes.rs +++ b/crates/comment-checker/tests/exit_codes.rs @@ -1,4 +1,4 @@ -//! Black-box exit-code contract assert (issue #6). +//! Black-box exit-code and output-stream contract assert (issue #6). //! //! The release workflow's smoke step hard-codes `rc -eq 2` for flagged //! payloads; that contract previously lived only in YAML, duplicated and @@ -16,37 +16,121 @@ const CLEAN_PAYLOAD: &str = r##"{"tool_name":"Write","tool_input":{"file_path":" const FLAGGED_PAYLOAD: &str = r#"{"tool_name":"Write","tool_input":{"file_path":"src/load_config.py","content":"def load_config(path):\n # TODO: fix this later\n return json.load(open(path))\n"}}"#; -fn run_binary(payload: &str) -> std::process::ExitStatus { +const EDIT_FLAGGED_PAYLOAD: &str = r#"{"tool_name":"Edit","tool_input":{"file_path":"foo.py","old_string":"x = 1\n","new_string":"x = 1 # TODO: handle this\n"}}"#; + +const MULTI_EDIT_FLAGGED_PAYLOAD: &str = r#"{"tool_name":"MultiEdit","tool_input":{"file_path":"foo.py","edits":[{"old_string":"x = 1\n","new_string":"x = 1\n# TODO: handle\n"}]}}"#; + +const REPORT_HEADER: &str = "An automated reviewer flagged"; +const REPORT_ACTION: &str = "Action: delete the flagged comments."; +const REPORT_REASON: &str = "a TODO with no tracked reference"; +const PASS_NOTE: &str = "[check-comments] Skipping"; + +struct Run { + status: std::process::ExitStatus, + stdout: String, + stderr: String, +} + +fn run_binary(payload: &str) -> Run { + run_binary_with_args(payload, &[]) +} + +fn run_binary_with_args(payload: &str, args: &[&str]) -> Run { let mut child = Command::new(env!("CARGO_BIN_EXE_comment-checker")) + .args(args) .stdin(Stdio::piped()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) .spawn() .expect("spawn comment-checker binary"); - // Test harness vs gate split: the binary reads stdin until EOF; the OS - // default write buffer is larger than any payload here. child .stdin .take() .expect("stdin") .write_all(payload.as_bytes()) .expect("write payload"); - child.wait().expect("wait for binary") + let out = child.wait_with_output().expect("wait for binary"); + Run { + status: out.status, + stdout: String::from_utf8(out.stdout).expect("stdout is utf-8"), + stderr: String::from_utf8(out.stderr).expect("stderr is utf-8"), + } +} + +fn assert_report_on_stderr_only(run: &Run, label: &str) { + for anchor in [REPORT_HEADER, REPORT_ACTION, REPORT_REASON] { + assert!( + run.stderr.contains(anchor), + "{label}: exit 2 forwards stderr to the model and drops stdout, so the \ + whole report must be on stderr; missing {anchor:?}, stderr was {:?}", + run.stderr + ); + } + assert!( + run.stdout.is_empty(), + "{label}: a blocked verdict must leave stdout empty; stdout was {:?}", + run.stdout + ); } #[test] fn clean_payload_exits_zero() { - assert!(run_binary(CLEAN_PAYLOAD).success()); + assert!(run_binary(CLEAN_PAYLOAD).status.success()); +} + +#[test] +fn clean_payload_notes_on_stdout_only() { + let run = run_binary(CLEAN_PAYLOAD); + assert!( + run.stdout.contains(PASS_NOTE), + "a pass note belongs on stdout, which a host keeps to its debug log; stdout was {:?}", + run.stdout + ); + assert!( + run.stderr.is_empty(), + "a passing verdict must leave stderr empty so nothing reaches the model; stderr was {:?}", + run.stderr + ); } #[test] fn flagged_payload_exits_with_the_blocked_contract() { - let status = run_binary(FLAGGED_PAYLOAD); + let run = run_binary(FLAGGED_PAYLOAD); assert_eq!( - status.code(), + run.status.code(), Some(i32::from(BLOCKED_EXIT_CODE)), "flagged payload must exit {BLOCKED_EXIT_CODE} — this constant is \ duplicated in .github/workflows/release.yml smoke step; changing it \ requires updating both" ); } + +#[test] +fn flagged_write_reports_on_stderr_only() { + assert_report_on_stderr_only(&run_binary(FLAGGED_PAYLOAD), "Write"); +} + +#[test] +fn flagged_edit_reports_on_stderr_only() { + assert_report_on_stderr_only(&run_binary(EDIT_FLAGGED_PAYLOAD), "Edit"); +} + +#[test] +fn flagged_multi_edit_reports_on_stderr_only() { + assert_report_on_stderr_only(&run_binary(MULTI_EDIT_FLAGGED_PAYLOAD), "MultiEdit"); +} + +#[test] +fn custom_prompt_report_lands_on_stderr() { + let run = run_binary_with_args(FLAGGED_PAYLOAD, &["--prompt", "Review:\n\n{{comments}}"]); + assert!( + run.stderr.contains("Review:") && run.stderr.contains(REPORT_HEADER), + "a --prompt report must reach the model on stderr too; stderr was {:?}", + run.stderr + ); + assert!( + run.stdout.is_empty(), + "a blocked verdict must leave stdout empty; stdout was {:?}", + run.stdout + ); +} diff --git a/npm/packages/comment-checker/README.md b/npm/packages/comment-checker/README.md index dca77a7..760726d 100644 --- a/npm/packages/comment-checker/README.md +++ b/npm/packages/comment-checker/README.md @@ -30,8 +30,8 @@ Add the hook to `~/.claude/settings.json` (user) or `.claude/settings.json` (pro ``` On `Edit` and `MultiEdit` only newly added comments are checked; restatement detection is disabled on edit fragments. -On a clean write the hook prints `[check-comments] Skipping: No unnecessary comments found` and exits 0. -When comments are flagged it prints the report with per-comment reasons and exits 2 — the status code is the contract for automation. +On a clean write the hook writes `[check-comments] Skipping: No unnecessary comments found` to stdout and exits 0. +When comments are flagged it writes the report, with per-comment reasons, to stderr and exits 2 — stderr because that is the stream a host forwards to the model on exit 2, and the status code is the contract for automation. ## Docs