From 2e88f5532d69456b656ef1c235e3819607b7d10d Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Tue, 25 Aug 2026 14:33:34 +0000 Subject: [PATCH 1/3] fix: send the block report to stderr so the agent actually receives it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flagged verdict exited 2 with its report on stdout. Exit 2 is the blocking code, and the PostToolUse contract forwards the hook's stderr to the model and discards stdout, so the report was written to the one stream the host throws away. The block landed with no explanation attached: the model saw only that something blocked it, never which comment or why. Measured against a real host before the change: the payload for a write adding `// set x to 1` above `const x = 1;` produced a correct 611-byte report on stdout with exit 2 and an empty stderr, and the model received nothing from it. Where the hook was reached through a wrapper that emitted anything at all on stderr, that unrelated text became the block reason instead, which is how this presented as "the hook is broken". `print!` becomes `eprint!` on the Block arm only. Exit codes are untouched, and the pass note stays on stdout, where a host sends exit-0 stdout to its debug log. The two existing exit-code tests pass both before and after this change, which is precisely why it shipped: they pinned the code and never the stream. `flagged_payload_reports_on_stderr_only` now pins both halves — report present on stderr, stdout empty — and it fails on the pre-change binary (2 passed, 1 failed) and passes after. Also syncs Cargo.lock, which still recorded 0.1.7 after the crate moved to 0.1.8, so any build regenerated it as an unrelated diff. Verified: cargo fmt --check, cargo clippy --all-targets -D warnings (0 findings), cargo test --all-targets (97 passed, 0 failed). --- .changeset/report-to-stderr.md | 7 ++++ Cargo.lock | 2 +- README.md | 6 +-- crates/comment-checker/src/main.rs | 3 +- crates/comment-checker/tests/exit_codes.rs | 45 ++++++++++++++++++---- 5 files changed, 50 insertions(+), 13 deletions(-) create mode 100644 .changeset/report-to-stderr.md 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..ba218ab 100644 --- a/README.md +++ b/README.md @@ -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 a host forwards to the model on exit 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..30e3ac1f 100644 --- a/crates/comment-checker/src/main.rs +++ b/crates/comment-checker/src/main.rs @@ -26,7 +26,8 @@ fn main() -> ExitCode { ExitCode::from(0) } Outcome::Block { report } => { - print!("{report}"); + // Why: exit 2 hands the model stderr and drops the other stream. + eprint!("{report}"); ExitCode::from(2) } } diff --git a/crates/comment-checker/tests/exit_codes.rs b/crates/comment-checker/tests/exit_codes.rs index 97db122..c214eb0 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,11 +16,17 @@ 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 { +struct Run { + status: std::process::ExitStatus, + stdout: String, + stderr: String, +} + +fn run_binary(payload: &str) -> Run { let mut child = Command::new(env!("CARGO_BIN_EXE_comment-checker")) .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 @@ -31,22 +37,45 @@ fn run_binary(payload: &str) -> std::process::ExitStatus { .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"), + } } #[test] fn clean_payload_exits_zero() { - assert!(run_binary(CLEAN_PAYLOAD).success()); + assert!(run_binary(CLEAN_PAYLOAD).status.success()); } #[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_payload_reports_on_stderr_only() { + let run = run_binary(FLAGGED_PAYLOAD); + assert!( + run.stderr.contains("flagged"), + "the report must go to stderr: on exit 2 the host forwards stderr to \ + the model and drops stdout, so a report on stdout is invisible to the \ + agent it addresses. stderr was {:?}", + run.stderr + ); + assert!( + run.stdout.is_empty(), + "a blocked verdict must leave stdout empty, so nothing competes with \ + the stderr report or is mistaken for hook JSON. stdout was {:?}", + run.stdout + ); +} From 1bbf12a414614724d70994874ff732686a6c0d23 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Tue, 25 Aug 2026 15:00:38 +0000 Subject: [PATCH 2/3] refactor: name the streams instead of commenting them The stream choice was carried by a comment above `eprint!`. Naming the two emit paths carries it in code: `emit_to_model` is the stream a host forwards on exit 2, `emit_to_debug_log` is the one it keeps to itself. The comment is deleted rather than shortened, since a shortened restatement is still a restatement. Test failure messages lose the same prose for the same reason; the test names already say which contract broke. --- crates/comment-checker/src/main.rs | 13 ++++++++++--- crates/comment-checker/tests/exit_codes.rs | 9 +++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/comment-checker/src/main.rs b/crates/comment-checker/src/main.rs index 30e3ac1f..f06da07 100644 --- a/crates/comment-checker/src/main.rs +++ b/crates/comment-checker/src/main.rs @@ -22,13 +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 } => { - // Why: exit 2 hands the model stderr and drops the other stream. - eprint!("{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 c214eb0..52896ba 100644 --- a/crates/comment-checker/tests/exit_codes.rs +++ b/crates/comment-checker/tests/exit_codes.rs @@ -66,16 +66,13 @@ fn flagged_payload_exits_with_the_blocked_contract() { fn flagged_payload_reports_on_stderr_only() { let run = run_binary(FLAGGED_PAYLOAD); assert!( - run.stderr.contains("flagged"), - "the report must go to stderr: on exit 2 the host forwards stderr to \ - the model and drops stdout, so a report on stdout is invisible to the \ - agent it addresses. stderr was {:?}", + run.stderr.contains("# TODO: fix this later"), + "exit 2 forwards stderr to the model and drops stdout; stderr was {:?}", run.stderr ); assert!( run.stdout.is_empty(), - "a blocked verdict must leave stdout empty, so nothing competes with \ - the stderr report or is mistaken for hook JSON. stdout was {:?}", + "a blocked verdict must leave stdout empty; stdout was {:?}", run.stdout ); } From 86da9f61c6d8336a75d38ab7ae3bcfe22b4a05b5 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Tue, 25 Aug 2026 15:10:13 +0000 Subject: [PATCH 3/3] test: make the stream gate fail on the ways it could false-pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the gate certified less than it appeared to. Two mutations passed it: - `eprint!("{input}")` — dumping the raw hook JSON. The anchor was `# TODO: fix this later`, a string the *payload* supplies, so echoing the input satisfied it. A check keyed on a value its own input provided certifies nothing. - `eprint!("# TODO: fix this later\n")` — the anchor and nothing else. The model would get a bare comment with no file, no line, no reason. The anchors are now three strings only the report path can produce: the header, the action footer, and the classifier's reason text. Both mutations now fail, as does the original stdout regression. Coverage the gate was missing, each verified by a mutation that used to pass and now fails: - the pass note's stream — moving it to stderr was invisible before - `Edit` and `MultiEdit`, which share the Block arm with `Write` and had no binary-level test, so a per-tool branch could regress them unnoticed - `--prompt`, whose substituted report travels the same arm Docs name the stream they were relying on: the npm page said "prints", which reads as stdout, and the project README's demo redirected in a way that would now drop the report. The exit-2 claim both rest on is cited rather than asserted. Verified: cargo fmt --check, clippy --all-targets -D warnings (0), cargo test --all-targets (101 passed, 0 failed); four mutations killed (input-echo 4 failures, anchor-only 4, stdout regression 4, pass-note 1); changeset validator OK. --- README.md | 6 +- crates/comment-checker/tests/exit_codes.rs | 70 ++++++++++++++++++++-- npm/packages/comment-checker/README.md | 4 +- 3 files changed, 69 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ba218ab..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. The report is written to stderr, because that is the stream a host forwards to the model on exit 2. +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 diff --git a/crates/comment-checker/tests/exit_codes.rs b/crates/comment-checker/tests/exit_codes.rs index 52896ba..d2f6fd3 100644 --- a/crates/comment-checker/tests/exit_codes.rs +++ b/crates/comment-checker/tests/exit_codes.rs @@ -16,6 +16,15 @@ 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"}}"#; +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, @@ -23,14 +32,17 @@ struct Run { } 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::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() @@ -45,11 +57,42 @@ fn run_binary(payload: &str) -> Run { } } +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).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 run = run_binary(FLAGGED_PAYLOAD); @@ -63,11 +106,26 @@ fn flagged_payload_exits_with_the_blocked_contract() { } #[test] -fn flagged_payload_reports_on_stderr_only() { - let run = run_binary(FLAGGED_PAYLOAD); +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("# TODO: fix this later"), - "exit 2 forwards stderr to the model and drops stdout; stderr was {:?}", + run.stderr.contains("Review:") && run.stderr.contains(REPORT_HEADER), + "a --prompt report must reach the model on stderr too; stderr was {:?}", run.stderr ); assert!( 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