Merge pull request #2077 from anthropics/fix-1358-1375-1783-hook-output-protocol

security-guidance: emit findings via hookSpecificOutput.additionalContext (#1358 #1375 #1783)

plugins/security-guidance/hooks/security_reminder_hook.py

@@ -190,7 +190,13 @@ CONTINUATION_SUFFIX = (
     "response."
 )
 
-def emit_metrics(metrics, rewake_summary=None):
+def emit_metrics(
+    metrics,
+    rewake_summary=None,
+    additional_context=None,
+    system_message=None,
+    hook_event_name="PostToolUse",
+):
     """
     Write a SyncHookJSONOutput line to stdout for Claude Code to pick up.
     For asyncRewake (Stop) hooks, CC scans stdout for the first {-prefixed line
@@ -213,6 +219,27 @@ def emit_metrics(metrics, rewake_summary=None):
     rewakeSummary in hooks.json, shown to the user in the terminal as the
     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).
+
+    `system_message` (optional, asyncRewake only): user-visible TUI message,
+    distinct from rewakeSummary which is the task-notification one-liner.
+    Use sparingly — the rewakeMessage in hooks.json is the primary user
+    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".
     """
     head = {}
     if _PV and "pv" not in metrics:
@@ -223,6 +250,17 @@ def emit_metrics(metrics, rewake_summary=None):
     out = {"metrics": 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 system_message:
+        out["systemMessage"] = system_message
     print(json.dumps(out), flush=True)
 
 # =====================================================================
@@ -1379,18 +1417,26 @@ def handle_commit_review_posttooluse(input_data):
         if s in sev:
             sev[s] += 1
 
+    # Rebuild guidance from new_vulns only — concrete_guidance from the LLM
+    # still lists deduped entries. Pass via additional_context so CC surfaces
+    # the reason via hookSpecificOutput.additionalContext instead of empty
+    # stdout (#1783) / stderr-only "json output validation failed" (#1375).
+    _commit_guidance = (PROVENANCE_BANNER + "\n\n"
+                        + _format_vulns_guidance(new_vulns)
+                        + CONTINUATION_SUFFIX + "\n")
     emit_metrics({
         "vulns_found": len(new_vulns), **_base, **_agentic_m,
         "critical_count": sev["critical"], "high_count": sev["high"],
         "files_reviewed": len(diff_files), "review_ms": review_ms,
         **({"deduped": n_deduped} if n_deduped else {}),
-    }, rewake_summary=_format_vulns_summary(new_vulns, prefix="Commit security review found"))
-
-    # Rebuild guidance from new_vulns only — concrete_guidance from the LLM
-    # still lists deduped entries.
-    sys.stderr.write(PROVENANCE_BANNER + "\n\n"
-                     + _format_vulns_guidance(new_vulns)
-                     + CONTINUATION_SUFFIX + "\n")
+    }, rewake_summary=_format_vulns_summary(new_vulns, prefix="Commit security review found"),
+       additional_context=_commit_guidance,
+       hook_event_name="PostToolUse")
+
+    # exit(2) is preserved per the asyncRewake protocol — it's what CC
+    # uses as the "force fix" signal that triggers the rewakeMessage flow.
+    # The stderr.write was removed; additional_context above now carries
+    # the same text via the modern JSON channel. See #1358/#1375/#1783.
     sys.exit(2)
 
 def handle_push_sweep_posttooluse(input_data):
@@ -1647,17 +1693,23 @@ def handle_push_sweep_posttooluse(input_data):
     # Metrics — keep within the 10-key cap; agentic sub-metrics are dropped
     # here in favour of the push-sweep funnel keys (telemetry can join on session_id
     # to the per-commit fires for agentic detail). rewake_summary must ride
-    # this line (CC reads only the first {-prefixed stdout line); it's a
-    # no-op when new_vulns is empty since we exit 0 below.
-    emit_metrics({
+    # this line (CC reads only the first {-prefixed stdout line); the emit
+    # is deferred to the two exit points below so the with-vulns path can
+    # also pass additional_context in the same JSON line (#1375/#1783) —
+    # the by-design "CC keeps only the first JSON line" constraint means
+    # we can't emit twice. Builds the shared metrics dict here; vulns path
+    # adds additional_context, no-vulns path emits as-is.
+    _push_metrics = {
         **_base, "pushed": len(push_range), "unreviewed": len(tail),
         "prefix_advanced": prefix_advanced, "vulns_found": len(new_vulns),
         "files_reviewed": len(diff_files), "review_ms": review_ms,
         **({"deduped": n_deduped} if n_deduped else {}),
-    }, rewake_summary=_format_vulns_summary(new_vulns, prefix="Push security review found"))
+    }
+    _push_rewake_summary = _format_vulns_summary(new_vulns, prefix="Push security review found")
 
     if not new_vulns:
         debug_log("Push sweep: no new findings")
+        emit_metrics(_push_metrics, rewake_summary=_push_rewake_summary)
         sys.exit(0)
 
     # First-push of a big branch can surface many findings at once across
@@ -1710,9 +1762,14 @@ def handle_push_sweep_posttooluse(input_data):
         guidance = _format_vulns_guidance(reported) or ""
     else:
         guidance = concrete_guidance or _format_vulns_guidance(reported) or ""
-    sys.stderr.write(
-        PROVENANCE_BANNER + "\n\n" + guidance + CONTINUATION_SUFFIX + "\n"
-    )
+    # Emit metrics + additional_context together — single JSON line is the
+    # contract CC's hook parser expects. exit(2) preserved as the asyncRewake
+    # "force fix" trigger (see comment near handle_commit_review_posttooluse).
+    # See #1358 / #1375 / #1783.
+    emit_metrics(_push_metrics, rewake_summary=_push_rewake_summary,
+                 additional_context=(PROVENANCE_BANNER + "\n\n"
+                                     + guidance + CONTINUATION_SUFFIX + "\n"),
+                 hook_event_name="PostToolUse")
     sys.exit(2)
 
 def handle_stop_hook(input_data):
@@ -1945,6 +2002,11 @@ def handle_stop_hook(input_data):
         # untracked_baseline_n is the signal for whether the UPS-time
         # untracked-snapshot capture actually ran.
         sweep_trimmed = {k: v for k, v in sweep.items() if k != "warn_unresolved_mask"}
+        # Pass guidance via additional_context so CC surfaces the findings via
+        # hookSpecificOutput.additionalContext instead of stderr-only (which
+        # was the cause of "json output validation failed" / empty-reason UI in
+        # #1375 / #1783). exit(2) preserved as the asyncRewake "force fix"
+        # signal — that's the documented mechanism. See #1358 / #1375 / #1783.
         emit_metrics({
             "vulns_found": len(vulns),
             "untracked_baseline_n": len(untracked_at_baseline),
@@ -1958,10 +2020,10 @@ def handle_stop_hook(input_data):
             **({"diff_truncated": llm._last_review_truncated_bytes}
                if llm._last_review_truncated_bytes else {}),
             **sweep_trimmed,
-        }, rewake_summary=_format_vulns_summary(vulns))
-
-        # Exit code 2 with stderr forces Claude to continue and fix
-        sys.stderr.write(PROVENANCE_BANNER + "\n\n" + concrete_guidance + CONTINUATION_SUFFIX + "\n")
+        }, rewake_summary=_format_vulns_summary(vulns),
+           additional_context=(PROVENANCE_BANNER + "\n\n"
+                               + concrete_guidance + CONTINUATION_SUFFIX + "\n"),
+           hook_event_name="Stop")
         sys.exit(2)
 
     if llm._last_call_claude_http_error is not None: