diff --git a/.changeset/hook-readonly-instead-of-strip.md b/.changeset/hook-readonly-instead-of-strip.md new file mode 100644 index 0000000..1878fa6 --- /dev/null +++ b/.changeset/hook-readonly-instead-of-strip.md @@ -0,0 +1,5 @@ +--- +'@systemfsoftware/claude-code-comment-checker': patch +--- + +The hook no longer crashes on startup in some environments and silently skips every write, and it no longer needs write access to your files — it flags unnecessary comments read-only and reports them to the model. \ No newline at end of file diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index daf1b56..a067003 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,7 @@ "plugins": [ { "name": "comment-checker", - "description": "PostToolUse hook that flags unnecessary comments. Runs comment-checker --strip, then direnv exec if it is missing.", + "description": "PostToolUse hook that flags unnecessary comments. Runs comment-checker, then direnv exec if it is missing.", "source": "./" } ] diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 2141a52..d9e5285 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "comment-checker", "version": "0.3.3", - "description": "PostToolUse hook that flags unnecessary comments. Runs comment-checker --strip, then direnv exec if it is missing. Names flake.nix when that is why it is missing.", + "description": "PostToolUse hook that flags unnecessary comments. Runs comment-checker, then direnv exec if it is missing. Names flake.nix when that is why it is missing.", "author": { "name": "systemfsoftware", "url": "https://github.com/systemfsoftware/comment-checker" diff --git a/.claude/settings.json b/.claude/settings.json index e46cd4a..46d09d9 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "command -v comment-checker >/dev/null 2>&1 && exec comment-checker || { command -v direnv >/dev/null 2>&1 && exec direnv exec \"${PWD:-.}\" comment-checker; } || exit 0" + "command": "command -v comment-checker >/dev/null 2>&1 && exec comment-checker\ncommand -v direnv >/dev/null 2>&1 && exec direnv exec \"${PWD:-.}\" comment-checker\necho \"comment-checker did not run — nothing checked this write.\" >&2\nexit 1" } ] } diff --git a/CONCEPTS.md b/CONCEPTS.md index c2e7574..56b8525 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -97,8 +97,17 @@ A manifest not on the list is intentionally outside the gate: keeping a file off the surface means its version can never drift out of step, and adding a manifest to it is a deliberate decision, not a default. +## Hook gate + +### Silent pass +A verification mechanism (a hook, a gate, a check) exits success while checking nothing — the failure mode of going green. Distinct from a false negative, which is a wrong verdict on a real check; a silent pass means the check never ran, the input never reached it, or its failure was swallowed. A hook must never claim a check that did not happen: absent checker exits with guidance, a flagged write exits with the report, and no surface ends in a blanket success. +*Avoid:* fail-open (reserved for the classifier's deliberate spare) + ## Flagged ambiguities - "context" had been used for both the language (scope/position) and the evidence (adjacent code) — these are distinct; adjacent syntax is the - only context that carries the mention. \ No newline at end of file + only context that carries the mention. +- "fail open" in the classifier spares a comment it cannot judge (a deliberate + conservative verdict) while "silent pass" in the hook gate exits green while + checking nothing (a defect) — distinct meanings, distinct domains. \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index 1ea1d97..0c4d855 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -201,7 +201,7 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "claude-code-comment-checker" -version = "0.3.2" +version = "0.3.3" dependencies = [ "clap", "proptest", diff --git a/docs/plans/2026-08-30-2114-fix-posttooluse-readonly-plan.md b/docs/plans/2026-08-30-2114-fix-posttooluse-readonly-plan.md new file mode 100644 index 0000000..b73ce9d --- /dev/null +++ b/docs/plans/2026-08-30-2114-fix-posttooluse-readonly-plan.md @@ -0,0 +1,136 @@ +--- +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +product_contract_source: ce-plan-bootstrap +execution: code +created: 2026-08-30 +updated: 2026-08-30 +type: fix +title: Make PostToolUse reporting actually work - Plan +--- + +# Make PostToolUse reporting actually work + +## Goal Capsule + +- **Objective:** The hook stays a `PostToolUse` hook and runs **read-only**: it never writes files, never passes `--strip`. When a write contains flagged comments it blocks via exit 2 with the report on stderr, and the launcher never crashes or silently skips (the crash made every write look "not fired"; the swallow made real blocks invisible). +- **Means:** Restore plain `comment-checker` (check mode, exit-2 block) on `PostToolUse` in both shipped surfaces, fix the Deno launcher so it cannot crash before the check runs (sensitive-env scrubbing + catch-all), remove the silent `|| exit 0` swallow in `.claude/settings.json`, and pin the wiring in a test (KTD1–KTD3). +- **Product authority:** user-directed — `PostToolUse`, no `--strip`, read-only enforcement. +- **Open blockers:** None. +- **Execution profile:** code. Launcher + wiring + tests + docs + changeset; no classifier or Rust core changes. +- **Stop conditions:** Both registrations are `PostToolUse` and run check mode (no `--strip`); a flagged payload exits 2 with the report on stderr; a blocked write surfaces its report; no `NotCapable` crash on a path with Deno-sensitive env vars present; no `|| exit 0` swallow in the settings hook; one-shot repo gate green. +- **Tail ownership:** `ce-work` after this plan is written (pipeline). + +--- + +## Product Contract + +### Summary + +The hook is a `PostToolUse` guard for `Write|Edit|MultiEdit` that reports flagged comments and blocks the model's next step with exit 2. It must be read-only: no `--strip`, no file mutation. Two defects made it look broken: the Deno launcher could crash with `NotCapable` before the check ran, and the project-level settings hook swallowed every failure with `|| exit 0`. This plan removes both so every write is actually checked and every block actually surfaces. + +### Problem Frame + +`PostToolUse` fires after the tool ran; enforcement is reporting + exit 2 (the host feeds the stderr report back and blocks). The shipped plugin hook ran `comment-checker --strip`, which mutates files and fails loudly on read-only paths; a read-only variant must drop the flag entirely. The launcher crashed when an inherited Deno-sensitive env var made the spawn fail at the permission layer. The settings hook swallowed all failures, so even when the checker did run and block, the gate was invisible. + +### Requirements + +- R1. Both shipped hook surfaces register on `PostToolUse` for `Write|Edit|MultiEdit`. +- R2. The registered commands run the checker in check mode with no `--strip`; the hook never writes files. +- R3. Exit-code contract preserved: `0` pass (skip note on stdout), `2` block (report on stderr), per `tests/exit_codes.rs`. +- R4. When the checker cannot start (missing binary, denied spawn, any other failure), the launcher continues its fallback chain, never throws an uncaught error; if nothing ran, it emits guidance and exits non-zero (never 0). +- R5. The checker's exit code passes through unchanged. +- R6. Docs describe the read-only PostToolUse behavior. +- R7. Release intent recorded in `.changeset/`. + +### Scope Boundaries + +**In scope:** launcher hardening, wiring (check mode on both surfaces), settings-hook swallow removal, wiring-contract test, docs, changeset. + +**Deferred/Out:** `--strip` (removed from the hook path; the CLI flag remains supported for manual use); classifier rules (no change); pre-commit gates. + +--- + +## Planning Contract + +### Key Technical Decisions + +- KTD1. **Stay `PostToolUse`, check mode, read-only.** The user-directed design: report flagged comments and block with exit 2 after the write; never `--strip`, never mutate. +- KTD2. **The launcher never crashes and never silently fails.** Every spawn (direct checker and `direnv exec`) scrubs Deno-sensitive env vars (LD_*/DYLD_*) and passes only PATH+HOME so an inherited variable cannot trigger a `NotCapable` denial; any start error is "binary unavailable" and continues the chain; the final fallback emits guidance and exits non-zero; the checker's 0/2 passes through. The settings-hook command carries the same guarantee without `|| exit 0`. +- KTD3. **The wiring is pinned.** A committed test asserts `PostToolUse` + check mode on both surfaces, no `--strip` in the hook path, and no swallow. + +### Assumptions + +- A1. `PostToolUse` payloads carry the same `tool_input` fields the binary decodes; check mode needs no file writes. +- A2. The `--strip` CLI flag stays supported for manual use outside the hook. +- A3. No classifier changes. + +--- + +## Implementation Units + +### U1. Restore read-only PostToolUse enforcement and harden the launcher + +- **Goal:** Both surfaces run plain `comment-checker` on `PostToolUse`; the launcher cannot crash or swallow; blocks surface via exit 2. +- **Requirements:** R1–R5 +- **Dependencies:** none +- **Files:** `hooks/run.ts`, `hooks/hooks.json`, `.claude/settings.json` +- **Approach:** + 1. `hooks/run.ts`: run the checker with no `--strip`; scrubbed env (LD_*/DYLD_* removed, PATH+HOME passed) for both the direct and `direnv exec` spawns; catch-all start-error fallback chain; final fallback emits guidance (flake hint) + exits 1; checker exit code passes through. + 2. `hooks/hooks.json`: `PostToolUse`, matcher `Write|Edit|MultiEdit`, command wraps `deno run` with `env -u LD_*…` scrubbing, timeout kept. + 3. `.claude/settings.json`: `PostToolUse`; command runs `comment-checker` (then `direnv`) with no `--strip` and no `|| exit 0` swallow. +- **Patterns:** existing fallback chain shape; `tests/exit_codes.rs` check contract. +- **Test scenarios:** + - Launcher smoke (crash-class): checker on PATH with a Deno-sensitive env var set → checker runs, no `NotCapable`, exit code passes through. + - Launcher smoke: checker + direnv absent → guidance + exit 1, no crash. + - Block smoke: stub checker exiting 2 → launcher exits 2; stub exiting 0 → launcher exits 0. +- **Verification:** `deno check` on `hooks/run.ts`; the smokes behave as stated; both JSONs valid and `PostToolUse`. +- **Execution note:** run the crash-class smoke first — it reproduced the observed `NotCapable` before the fix. + +### U2. Pin the wiring contract in a test + +- **Goal:** A committed test fails if the hook surfaces drift from `PostToolUse` + check mode or re-introduce `--strip`/a swallow. +- **Requirements:** R1–R3 +- **Dependencies:** U1 +- **Files:** `crates/comment-checker/tests/wire.rs` (new) +- **Approach:** Read `../../hooks/hooks.json`, `../../.claude/settings.json`, `../../hooks/run.ts` relative to the crate (CARGO_MANIFEST_DIR/../..) and assert: `PostToolUse` with matcher `Write|Edit|MultiEdit` in both; the launcher invokes the checker without `--strip`; no hook command ends in `|| exit 0`. +- **Patterns:** existing black-box tests under `crates/comment-checker/tests/`. +- **Test scenarios:** + - `hooks/hooks.json` registers `PostToolUse` with the matcher. + - `.claude/settings.json` registers `PostToolUse` and contains no `--strip` / `|| exit 0`. + - `hooks/run.ts` invokes the checker without `--strip`. +- **Verification:** `cargo test --all-targets` green. + +### U3. Document the read-only behavior + +- **Goal:** READMEs and plugin metadata describe the PostToolUse read-only check; no stale `--strip`-in-hook claims. +- **Requirements:** R6 +- **Dependencies:** U1 +- **Files:** `npm/packages/comment-checker/README.md`, `.claude-plugin/plugin.json`, `.claude-plugin/marketplace.json` (only if they claim the hook strips), root `README.md` mentions if any. +- **Approach:** Confirm the wiring examples use plain `comment-checker`; adjust any description that says the hook runs `--strip`; keep `--strip` documented as a manual CLI option. +- **Test expectation:** none — docs/metadata; diff-checked. + +### U4. Record release intent + +- **Goal:** Reach consumers via the release pipeline. +- **Requirements:** R7 +- **Dependencies:** U1–U3 (content settled) +- **Files:** `.changeset/.md` (new) +- **Approach:** `patch`; consumer voice; single paragraph — the hook no longer crashes on some environments and no longer needs write access; it reports flagged comments read-only. +- **Test expectation:** none — release metadata; changeset format gate in CI. + +--- + +## Verification Contract + +| # | Command | Applies | Done signal | +|---|---|---|---| +| 1 | `cargo fmt --check && cargo clippy --all-targets -- -D warnings && cargo test --all-targets` | all | one-shot gate green, incl. `tests/wire.rs` | +| 2 | `deno check hooks/run.ts` (via deno.jsonc) | U1 | launcher type-checks | +| 3 | Launcher smokes: crash-class; both-absent → guidance + exit 1; stub exit 2/0 pass-through | U1 | three behaviors run in this session | +| 4 | Flagged `Write` payload → real `comment-checker` (check mode) | U1 | exit 2, report on stderr, file unchanged | + +## Definition of Done + +- **Global:** R1–R7 hold; one-shot gate green; smokes pass; no dead-end code in the diff; the previous `--strip` wiring experiment is removed, not commented. +- **Per-unit:** U1 launcher hardened + wiring restored and smoked; U2 wire test green; U3 docs consistent; U4 changeset present. \ No newline at end of file diff --git a/docs/solutions/runtime-errors/deno-env-sensitive-spawn-crash-silent-pass-hook.md b/docs/solutions/runtime-errors/deno-env-sensitive-spawn-crash-silent-pass-hook.md new file mode 100644 index 0000000..ca15236 --- /dev/null +++ b/docs/solutions/runtime-errors/deno-env-sensitive-spawn-crash-silent-pass-hook.md @@ -0,0 +1,61 @@ +--- +title: "Deno denies spawning when LD_*/DYLD_* env vars are present, and the hook either crashed or silently passed — scrub the env class and absorb every spawn failure" +date: 2026-08-30 +category: runtime-errors +module: hooks (Deno launcher + hook surfaces) +problem_type: runtime_error +component: dev-tooling +symptoms: + - "`Uncaught (in promise) NotCapable: Requires --allow-run permissions to spawn subprocess with LD_FOR_BUILD environment variable` on every Write tool result; writes landed but nothing was checked" + - "Launcher exit 2 blocks the write while exit 1 does not on some hosts — the silent-pass hole was the settings surface ending `|| exit 0`, and the 8-name env scrub missed unlisted LD_* vars (LD_PRELOAD_64, LD_ASSUME_KERNEL)" + - "A source-text grep test passed green after the fix while asserting a token ('NotCapable') that existed only in a comment — reverting the catch-all still left the test green (CHK1)" +root_cause: missing_validation +resolution_type: code_fix +severity: high +tags: [deno, hook, notcapable, env-scrub, allow-run, silent-pass, chk1] +--- + +# Deno env-sensitive spawn crash and the silent-pass hook + +## Problem + +The PostToolUse hook could not enforce. The Deno launcher crashed with an uncaught `NotCapable` error whenever a Deno-sensitive env var (`LD_*`, `DYLD_*`) was present, and the settings-surface hook ended in a silent `|| exit 0` — every write passed without being checked. + +## Symptoms + +- Live crash on this machine (env carries `LD_FOR_BUILD=ld`): the installed plugin launcher threw `NotCapable: Requires --allow-run permissions to spawn subprocess with LD_FOR_BUILD environment variable` at `run.ts:31` (`Command.output()`) after every Write tool result. +- The repo settings hook ended `|| exit 0`, converting any failure into a green exit. +- A finite 8-name scrub list (LD_PRELOAD, LD_LIBRARY_PATH, LD_DEBUG, LD_AUDIT, LD_FOR_BUILD, DYLD_*...) missed other class members; the first smoke with `LD_PRELOAD_64` set re-triggered the crash class. +- The wire test `assert!(run_ts.contains("NotCapable"))` was satisfied by a comment in run.ts, not the catch block — the exact regression it claimed to pin passed CI. + +## What Didn't Work + +- Scrubbing by finite name list. Deno's sensitive-env check is prefix-based: any `LD_*`/`DYLD_*` variable denies the spawn, so an uncovered name re-triggers the crash class. +- Scrubbing inside run.ts via `Deno.env.delete`. The denial fires inside the spawn regardless of how the env was set, and deletion is itself gated by `--allow-env`. +- A text-grep test for the regression token. If the token lives in a comment (or the fixer's own prose), the test certifies nothing about the mechanism (CHK1). + +## Solution + +- **Class-level env sweep in the hook command** (POSIX sh, keeps the `--allow-run` allowlist): + +```sh +env $(env | awk -F= '$1 ~ /^(LD_|DYLD_)/ { printf "-u %s ", $1 }') deno run --config "${CLAUDE_PLUGIN_ROOT}/hooks/deno.jsonc" --allow-read --allow-run=comment-checker,direnv --allow-env=CLAUDE_PROJECT_DIR,PATH,HOME "${CLAUDE_PLUGIN_ROOT}/hooks/run.ts" +``` + +- **Absorb every spawn failure in the launcher.** The previous code rethrew non-`NotFound` errors; now `} catch { return undefined }` maps any spawn failure to the binary-unavailable fallback chain, so no environment can turn the hook into a crash. +- **No source-text wiring tests.** The text-grep test was removed as tautology — it restated the config that contained the strings. Verification is behavioral only (smokes; see the filed launcher-smoke residual). + +## Why This Works + +Deno denies the spawn itself when the process environment carries a sensitive var — inside the spawn, regardless of `env:` merge semantics or allow flags. So the only reliable defenses are (a) scrub the whole prefix class before Deno starts, and (b) never let a denial escape the launcher. The guard's failure mode is silent pass, so the surface must exit `1` (guidance) or `2` (flagged) — never `0` — when nothing ran. + +## Prevention + +- Scrub env classes by prefix, never by name list; when the runtime's denial is prefix-based, the scrub must be too. +- For a verification mechanism, pin the mechanism (catch-without-rethrow structure, exit-code contract), never a string that can live in a comment. A behavioral smoke (stub binaries on a temp PATH, sensitive var set, real payload piped) is stronger and needs only deno in CI. +- Hooks must never claim a check that did not happen: absent checker -> exit 1 with a bare status line (stderr), flagged comment -> exit 2 with report, and no `|| exit 0` anywhere in the surface. A hook reports state; it never instructs the agent — no `pnpm add`, no "run direnv allow", nothing the model would act on from the hook's own text. + +## Related Issues + +- Code review run 20260830-212446 (correctness P1/100 env-class, 3 reviewers on the CHK1 wire test). +- Residuals filed: #86 (fail-open stdin decision), #90 (committed launcher smoke), #91 (catch-all conflates spawn-denied with not-found). \ No newline at end of file diff --git a/hooks/hooks.json b/hooks/hooks.json index 5dc3949..efd96bc 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "deno run --config \"${CLAUDE_PLUGIN_ROOT}/hooks/deno.jsonc\" --allow-read --allow-run=comment-checker,direnv --allow-env=CLAUDE_PROJECT_DIR,PATH,HOME \"${CLAUDE_PLUGIN_ROOT}/hooks/run.ts\"", + "command": "env $(env | awk -F= '$1 ~ /^(LD_|DYLD_)/ { printf \"-u %s \", $1 }') deno run --config \"${CLAUDE_PLUGIN_ROOT}/hooks/deno.jsonc\" --allow-read --allow-run=comment-checker,direnv --allow-env=CLAUDE_PROJECT_DIR,PATH,HOME \"${CLAUDE_PLUGIN_ROOT}/hooks/run.ts\"", "timeout": 30 } ] diff --git a/hooks/run.ts b/hooks/run.ts old mode 100644 new mode 100755 index 58f40c3..43abfa6 --- a/hooks/run.ts +++ b/hooks/run.ts @@ -1,8 +1,6 @@ #!/usr/bin/env -S deno run --allow-read --allow-run=comment-checker,direnv --allow-env=CLAUDE_PROJECT_DIR,PATH,HOME -import { exists } from '@std/fs/exists' import { writeAll } from '@std/io/write-all' -import { join } from '@std/path' import { type } from 'arktype' const Env = type({ @@ -30,32 +28,29 @@ async function run(cmd: string, args: string[]): Promise { stderr: 'inherit', }).output() return code - } catch (error) { - if (error instanceof Deno.errors.NotFound) return undefined - throw error + } catch { + return undefined } } -const strip = ['--strip'] const projectDir = env.CLAUDE_PROJECT_DIR -const fromPath = await run('comment-checker', strip) +const fromPath = await run('comment-checker', []) if (fromPath !== undefined) Deno.exit(fromPath) -const fromDirenv = await run('direnv', ['exec', projectDir, 'comment-checker', ...strip]) -if (fromDirenv !== undefined) Deno.exit(fromDirenv) +const fromDirenv = await run('direnv', ['exec', projectDir, 'comment-checker']) +if (fromDirenv !== undefined) { + if (fromDirenv !== 0 && fromDirenv !== 2) { + await writeAll( + Deno.stderr, + new TextEncoder().encode('comment-checker did not run — nothing checked this write.\n'), + ) + } + Deno.exit(fromDirenv) +} -const flake = await exists(join(projectDir, 'flake.nix')) await writeAll( Deno.stderr, - new TextEncoder().encode( - [ - 'comment-checker did not run, so nothing checked this write.', - flake - ? 'This project has flake.nix. Run direnv allow or nix develop so comment-checker is on PATH (the flake wraps it in bwrap).' - : 'Install it: pnpm add -g @systemfsoftware/claude-code-comment-checker', - '', - ].join('\n'), - ), + new TextEncoder().encode('comment-checker did not run — nothing checked this write.\n'), ) -Deno.exit(1) +Deno.exit(1) \ No newline at end of file diff --git a/npm/packages/comment-checker/README.md b/npm/packages/comment-checker/README.md index 92be7ec..806c61b 100644 --- a/npm/packages/comment-checker/README.md +++ b/npm/packages/comment-checker/README.md @@ -106,24 +106,14 @@ comment-checker --prompt "Formatting Guidelines Violation:\n\n{{comments}}\n\nPl Pass `--strip` to delete flagged whole-line comments directly from the target file on disk when invoked: -```json -{ - "hooks": { - "PostToolUse": [ - { - "matcher": "Write|Edit|MultiEdit", - "hooks": [ - { - "type": "command", - "command": "comment-checker --strip" - } - ] - } - ] - } -} +```bash +comment-checker --strip <<'JSON' +{"tool_name":"Write","tool_input":{"file_path":"src/client.py","content":"def load(path):\n # parse the config file\n return open(path).read()\n"}} +JSON ``` +The hook itself runs check mode and never modifies your files. + ## Troubleshooting ### `command not found: comment-checker`