From 70c28b9c2f9e29333443b03fbbdda35e1a0ce878 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Tue, 2 Jun 2026 08:52:38 -0700 Subject: [PATCH] =?UTF-8?q?security-guidance:=20emit=20schema-valid=20Stop?= =?UTF-8?q?-hook=20output=20(#2159)=20=E2=80=94=202.0.2=20=E2=86=92=202.0.?= =?UTF-8?q?3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2159. The Stop hook emits feedback via `hookSpecificOutput: {hookEventName: "Stop", additionalContext}`, but `Stop`/`SubagentStop` are NOT members of CC's `hookSpecificOutput` discriminated union (coreSchemas.ts — valid members are PreToolUse, PostToolUse, UserPromptSubmit, SessionStart, etc.). So the emitted shape violates CC's documented hook-output schema. Impact is CC-version-dependent — important nuance, established empirically: - Reporter (array0224-cloud) on CLI 2.1.150 / 2.1.152: CC rejects the Stop feedback; the block/reason never reaches the model, so the auto-rewake/fix loop is lost. (Detection still runs + logs.) - On CLI 2.1.160 (current) the asyncRewake completion path is lenient: its gate is `isSyncHookJSONOutput` (hooks.ts) which is just `!(json.async === true)` — NOT a strict schema parse. So the invalid hookSpecificOutput is tolerated: metrics + rewakeSummary are still consumed and the model still receives the findings. I could NOT reproduce the rejection on 2.1.160, and BQ confirms Stop-path vulns_found metrics are recorded normally (~21k with-vuln fires / 3d), i.e. NOT dropped. (An earlier draft of this message claimed metrics were dropped — that was wrong; corrected after checking telemetry + repro'ing the old plugin on 2.1.160.) So this is defensive schema-correctness: the plugin should emit output that conforms to CC's documented union regardless of how strictly a given CC version validates it. The reporter's environment validates strictly; relying on the current version's leniency is fragile. Fix (CC's documented asyncRewake "clean pattern" — hooks.ts: "error text on stderr, JSON on stdout"): - For Stop/SubagentStop, emit_metrics writes guidance to stderr (the asyncRewake body channel CC delivers via `stderr || stdout`) and sets top-level `decision: "block"` + `reason` (valid SyncHookJSONOutput fields; also the documented sync Stop-hook contract for the `-p` fallback). It does NOT emit a Stop hookSpecificOutput. - PostToolUse (commit-review, push-sweep) is unchanged — valid union member, keeps the modern hookSpecificOutput protocol. Verified locally on macOS Python 3.13: - py_compile clean. - 11 new tests (test_2159_stop_hook_schema.py) pin the contract: Stop output carries no hookSpecificOutput, uses top-level decision/reason, writes guidance to stderr; the emitted hookEventName (when present) is a valid union member. 2 existing tests that asserted the buggy Stop->hookSpecificOutput shape were corrected. Full suite 464/464 pass + 2 skipped. - END-TO-END in /tmux on CLI 2.1.160: * FIXED plugin (2.0.3): staged pickle.loads + os.system, benign edit pulls the file into review_set; Stop LLM review found 2 critical vulns; CC delivered a clean rewake ("Background security review found issues" + both findings). All hooks (UPS, PostToolUse[Edit] pattern, PostToolUse[Bash] commit-review, Stop) fired clean; zero schema rejections / errors / http_err in the debug log. * OLD plugin (2.0.2) on the SAME 2.1.160: also delivered Stop feedback (confirming the no-repro-on-latest finding above) — which proves the fix carries NO regression risk on current CC while making the output robust for the stricter versions where it actually breaks. Version bumped 2.0.2 -> 2.0.3 per the per-PR-bump policy (#2114: a bump is the only way the fix reaches the existing fleet — relevant for users still on the CC versions where this breaks). Closes #2159. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude-plugin/marketplace.json | 2 +- .../.claude-plugin/plugin.json | 2 +- .../hooks/security_reminder_hook.py | 69 +++++++++++++------ 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 21e53d80..efe635e8 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -2215,7 +2215,7 @@ { "name": "security-guidance", "description": "Security review for Claude-generated code. Pattern-based warnings on edits, LLM-powered diff review on Stop, and an agentic commit reviewer that catches injection, XSS, SSRF, hardcoded secrets, and 25+ other vulnerability classes.", - "version": "2.0.2", + "version": "2.0.3", "author": { "name": "Anthropic", "email": "support@anthropic.com" diff --git a/plugins/security-guidance/.claude-plugin/plugin.json b/plugins/security-guidance/.claude-plugin/plugin.json index 63a9c3da..1509ab03 100644 --- a/plugins/security-guidance/.claude-plugin/plugin.json +++ b/plugins/security-guidance/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "security-guidance", - "version": "2.0.2", + "version": "2.0.3", "description": "Security review for Claude-generated code. Pattern-based warnings on edits, LLM-powered diff review on Stop, and an agentic commit reviewer that catches injection, XSS, SSRF, hardcoded secrets, and 25+ other vulnerability classes.", "author": { "name": "David Dworken", diff --git a/plugins/security-guidance/hooks/security_reminder_hook.py b/plugins/security-guidance/hooks/security_reminder_hook.py index caa7759e..d3f726e8 100755 --- a/plugins/security-guidance/hooks/security_reminder_hook.py +++ b/plugins/security-guidance/hooks/security_reminder_hook.py @@ -221,15 +221,34 @@ def emit_metrics( task-notification one-liner. Must be in the same JSON line as the metrics because CC stops scanning stdout after the first {-prefixed line. - `additional_context` (asyncRewake findings): model-visible guidance text - that CC surfaces via the modern hook-output protocol - (hookSpecificOutput.additionalContext) instead of the legacy stderr + - exit(2) pair. The caller passes the finding-explanation text it would - have written to stderr; the JSON channel carries it cleanly so CC's UI - shows the reason properly instead of "Permission denied with no reason". - See anthropics/claude-plugins-official#1375 and #1783. Empty/None - means no hookSpecificOutput field is emitted (preserves backward compat - for legacy emit-sites that only want metrics). + `additional_context` (asyncRewake findings): model-visible guidance text. + Delivery channel depends on `hook_event_name` because CC's hook-output + contract is NOT symmetric across events: + + - PostToolUse (commit-review, push-sweep): surfaced via the modern + hookSpecificOutput.additionalContext protocol. `PostToolUse` is a + member of CC's hookSpecificOutput discriminated union + (coreSchemas.ts), so the JSON validates and metrics/rewakeSummary + are consumed. See #1375 / #1783 for why this replaced the legacy + stderr + exit(2) shape for PostToolUse. + + - Stop / SubagentStop: there is NO `Stop` member in that union, so + emitting hookSpecificOutput{hookEventName:"Stop"} makes the whole + line fail isSyncHookJSONOutput validation — which on the asyncRewake + path silently drops metrics AND rewakeSummary, and (because the + legacy stderr write was removed) leaks the raw JSON to the model as + the rewake body. CC's asyncRewake delivery actually reads + `stderr || stdout` for the model-visible body and only scans stdout + JSON for metrics+rewakeSummary — it never reads additionalContext + on this path. So for Stop we use the documented clean pattern: + guidance on stderr, valid JSON (metrics + rewakeSummary + + top-level decision/reason) on stdout. The top-level decision:"block" + + reason also covers the sync-fallback path (single-shot `claude -p`, + where asyncRewake degrades to a sync Stop hook that reads + decision/reason). See #2159. + + Empty/None additional_context emits neither channel (back-compat for + metrics-only callers). `system_message` (optional, asyncRewake only): user-visible TUI message, distinct from rewakeSummary which is the task-notification one-liner. @@ -237,10 +256,9 @@ def emit_metrics( surface; systemMessage adds a per-fire override when the static rewakeMessage isn't specific enough for the finding being shown. - `hook_event_name` (used only when additional_context is set): which event - the hookSpecificOutput attaches to. Defaults to "PostToolUse" since the - commit-review and push-sweep handlers are the most common callers; - handle_stop_hook explicitly passes "Stop". + `hook_event_name` (used only when additional_context is set): selects the + delivery channel above. Defaults to "PostToolUse" (commit-review and + push-sweep are the most common callers); handle_stop_hook passes "Stop". """ head = {} if _PV and "pv" not in metrics: @@ -252,14 +270,23 @@ def emit_metrics( if rewake_summary: out["rewakeSummary"] = rewake_summary if additional_context: - # Wrap in hookSpecificOutput per CC's modern hook-output contract. - # Drops the legacy `sys.stderr.write(...) + sys.exit(2)` shape that - # left CC's UI showing "denied with no reason" (#1783) and triggered - # "json output validation failed" on older CC versions (#1375). - out["hookSpecificOutput"] = { - "hookEventName": hook_event_name, - "additionalContext": additional_context, - } + if hook_event_name in ("Stop", "SubagentStop"): + # Stop is NOT in CC's hookSpecificOutput union — emitting it there + # fails schema validation and drops metrics+rewakeSummary (#2159). + # Clean pattern: guidance on stderr (the asyncRewake body channel, + # delivered via `stderr || stdout`), top-level decision/reason for + # the sync-fallback path. stdout JSON stays valid so metrics + + # rewakeSummary survive. + sys.stderr.write(additional_context) + sys.stderr.flush() + out["decision"] = "block" + out["reason"] = additional_context + else: + # PostToolUse et al. — valid union member; modern protocol. + out["hookSpecificOutput"] = { + "hookEventName": hook_event_name, + "additionalContext": additional_context, + } if system_message: out["systemMessage"] = system_message print(json.dumps(out), flush=True)