[security-guidance] Reviews that fail with API errors (e.g. HTTP 401) are reported as clean and marked reviewed

Thanks @evantayloryates for the report. I reproduced the same failure in the public source of this repository at 71cdddec623889d38af14b7a489670a03186f659 and prepared the patch and regression tests below.

The raw HTTP reviewer turns both a failed model call and a successful clean review into (None, []). Commit review and push sweep then append the affected SHA(s) to sg-reviewed-shas. This also loses a failed review when the async agentic race's fallback raises.

The patch uses findings=None for a failed review and findings=[] for a completed clean review. Commit/push handlers stop before recording SHAs on failure. Stop restores its consumed touched-path state before reporting failure. Each failure emits an exit-2 diagnostic and a failure-specific rewakeSummary instead of claiming no vulnerabilities; it does not log credential or API response contents. Successful model fallback and a dual-or review with at least one successful leg still count as reviewed.

Validation: Python 3.9.6 on macOS, actual _call_claude/urllib requests to a loopback HTTP fixture, actual temporary Git commits and a local bare push target, and the real commit/push/Stop handlers in isolated subprocesses. All credentials and model responses are synthetic. On the unmodified source, 5 of 12 tests fail; all 12 pass after the fix. A clean copy accepts the complete patch with git apply --check, then also passes all 12 tests. Coverage includes failed commit/push not entering the ledger, recovery and re-review of the same commit, clean and finding-bearing success, primary-failure/sonnet-fallback success, dual-or partial success, and Stop failure preserving the touched paths and baseline.

Scope: this repository's manifest is 2.0.0, while the report uses marketplace 2.0.8. I have not run a live Claude session, the marketplace package or a real Agent SDK reviewer; the actual terminal/mobile rendering of the async failure notification still needs product acceptance. The tests verify the emitted hook protocol, not that UI. No external model was called and no SDK was installed.

Run from the repository root:

python3 -m unittest discover -s plugins/security-guidance/tests -v

External PR creation remains restricted to collaborators. Could a maintainer take this patch or identify the preferred contribution route? Credit for the original diagnosis belongs to @evantayloryates. I have not opened a PR or pushed a contribution branch.

<details>
<summary>Complete patch and regression tests</summary>

--- a/plugins/security-guidance/hooks/llm.py
+++ b/plugins/security-guidance/hooks/llm.py
@@ -707,14 +707,15 @@
     return kept, len(vulns) - len(kept)
 
 
-def analyze_code_security(files: List[Tuple[str, str]], is_diff: bool = False, previous_findings: Optional[List[str]] = None) -> Tuple[Optional[str], List[Dict[str, Any]]]:
+def analyze_code_security(files: List[Tuple[str, str]], is_diff: bool = False, previous_findings: Optional[List[str]] = None) -> Tuple[Optional[str], Optional[List[Dict[str, Any]]]]:
     """
     Use Haiku to perform a security review of code.
     files: list of (file_path, content_or_diff) tuples
     is_diff: if True, the content is a unified diff rather than full file contents
     previous_findings: list of category strings from earlier stop hook firings this turn,
         used to prompt the reviewer to verify those issues were actually fixed.
-    Returns (formatted guidance string or None, list of vuln dicts with severity/category).
+    Returns (guidance, findings); findings is None when all model calls fail,
+    and [] when a completed review has no reportable findings.
     """
     if not HAS_API_CREDENTIALS or not files:
         return None, []
@@ -963,6 +964,9 @@
     analysis = _call_claude_dual_or(prompt, output_schema,
                                     bool_key="hasVulnerabilities",
                                     list_key="vulnerabilities")
+    if analysis is None:
+        debug_log("LLM code review: could not complete")
+        return None, None
     if not analysis or not analysis.get("hasVulnerabilities") or not analysis.get("vulnerabilities"):
         debug_log("LLM code review: no vulnerabilities found")
         return None, []
--- a/plugins/security-guidance/hooks/security_reminder_hook.py
+++ b/plugins/security-guidance/hooks/security_reminder_hook.py
@@ -882,7 +882,7 @@
                 diff_files, is_diff=True, previous_findings=previous_findings
             )
         except Exception as e:  # pragma: no cover
-            g, v = None, []
+            g, v = None, None
         try:
             q.put_nowait(("fallback", (g, v, {"agentic": False})))
         except _queue.Full:
@@ -897,6 +897,20 @@
     m["race_delay_s"] = delay_s
     m["race_started"] = 1 if fallback_started.is_set() else 0
     return g, v, m
+
+def _report_failed_review(scope, metrics):
+    """Surface a failed review without recording it as clean or reviewed."""
+    message = (f"{scope} security review could not complete; no successful review "
+               "was recorded. Check model access before retrying.")
+    debug_log(message)
+    emit_metrics({
+        **metrics, "review_failed": True,
+        **({"api_error": llm._last_call_claude_http_error}
+           if llm._last_call_claude_http_error is not None else {}),
+    }, rewake_summary=f"{scope} security review could not complete")
+    sys.stderr.write(message + "\n")
+    sys.exit(2)
+
 
 def handle_commit_review_posttooluse(input_data):
     """PostToolUse handler for Bash — reviews git commits for security issues.
@@ -1244,6 +1258,9 @@
         concrete_guidance, vulns = analyze_code_security(
             diff_files, is_diff=True, previous_findings=previous_findings
         )
+
+    if vulns is None:
+        _report_failed_review("Commit", _base)
 
     # push-sweep state: record this commit as reviewed (full 40-hex sha) so a
     # later `git push` can advance its diff base past it. Recorded here — after
@@ -1619,6 +1636,10 @@
         agentic_metrics = {}
     review_ms = int((_time.time() - review_start) * 1000)
 
+    if vulns is None:
+        _report_failed_review("Push", {**_base, "pushed": len(push_range),
+                                       "unreviewed": len(tail)})
+
     # The tail is now covered by this net-diff review.
     _append_reviewed_shas(repo_root, tail, vulns_found=len(vulns or []))
 
@@ -1867,6 +1888,10 @@
     review_ms = int(stop_review_elapsed * 1000)
     fire_index = fire_count + 1
 
+    if vulns is None:
+        restore_unreviewed_stop_state(session_id, touched_paths, snap_baseline)
+        _report_failed_review("Stop", {"diff_strategy_v2": True})
+
     # Late dedup: drop only what a concurrent commit-review wrote while our
     # LLM ran. Anything already in `previous_findings` (the consume_stop_state
     # snapshot) that the LLM re-flagged is an intentional "fix incomplete"
@@ -1946,11 +1971,7 @@
         sys.stderr.write(PROVENANCE_BANNER + "\n\n" + concrete_guidance + CONTINUATION_SUFFIX + "\n")
         sys.exit(2)
 
-    if llm._last_call_claude_http_error is not None:
-        debug_log(f"Stop hook: API call failed with status {llm._last_call_claude_http_error}")
-        restore_unreviewed_stop_state(session_id, touched_paths, snap_baseline)
-    else:
-        debug_log("Stop hook: no security issues found")
+    debug_log("Stop hook: no security issues found")
     # CC truncates metrics to 10 keys by
     # insertion order. The previous **sweep,**v2_metrics tail meant the 3
     # v2_metrics keys were always sliced off this most-common path, so the
--- /dev/null
+++ b/plugins/security-guidance/tests/test_failed_review.py
@@ -0,0 +1,137 @@
+"""Failed model calls must not become successful reviewed-SHA records."""
+import json
+import os
+from pathlib import Path
+import subprocess
+import sys
+import tempfile
+import threading
+import unittest
+from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
+
+HOOKS = Path(__file__).resolve().parents[1] / 'hooks'
+CLIENT = '''
+import json,sys,security_reminder_hook as h
+mode=sys.argv[1]
+if mode=='stop':
+ data=json.load(sys.stdin)
+ try: h.handle_user_prompt_submit(data)
+ except SystemExit: pass
+ from pathlib import Path
+ Path('app.py').write_text('print(3)\\n')
+ h.record_touched_path(data['session_id'], str(Path('app.py').resolve()))
+ h.handle_stop_hook(data)
+elif mode=='analysis':
+ print(json.dumps(h.analyze_code_security([('app.py','+print(1)')],is_diff=True)))
+else:
+ data=json.load(sys.stdin)
+ h.handle_commit_review_posttooluse(data) if mode=='commit' else h.handle_push_sweep_posttooluse(data)
+'''
+
+class FailedReviewTest(unittest.TestCase):
+    def setUp(self):
+        self.tmp=tempfile.TemporaryDirectory();self.addCleanup(self.tmp.cleanup)
+        self.root=Path(self.tmp.name).resolve();self.repo=self.root/'repo';self.repo.mkdir()
+        self.calls=[];self.statuses=[401];self.findings=[];test=self
+        class Server(BaseHTTPRequestHandler):
+            def do_POST(self):
+                body=json.loads(self.rfile.read(int(self.headers['Content-Length'])))
+                test.calls.append(body['model'])
+                status=test.statuses.pop(0) if len(test.statuses)>1 else test.statuses[0]
+                self.send_response(status);self.end_headers()
+                reply={'error':'fixture failure'} if status!=200 else {'content':[{'type':'text','text':json.dumps({'hasVulnerabilities':bool(test.findings),'vulnerabilities':test.findings})}]}
+                self.wfile.write(json.dumps(reply).encode())
+            def log_message(self,*args):pass
+        self.server=ThreadingHTTPServer(('127.0.0.1',0),Server)
+        thread=threading.Thread(target=self.server.serve_forever,daemon=True);thread.start()
+        self.addCleanup(self.server.server_close);self.addCleanup(thread.join);self.addCleanup(self.server.shutdown)
+        self.env={'PATH':'/usr/bin:/bin','HOME':str(self.root),'PYTHONPATH':str(HOOKS),
+                  'PYTHONDONTWRITEBYTECODE':'1','ANTHROPIC_API_KEY':'fixture-key',
+                  'ANTHROPIC_BASE_URL':f'http://127.0.0.1:{self.server.server_port}',
+                  'SG_AGENTIC_COMMIT_REVIEW':'0'}
+        self.git('init','-b','main');self.git('config','user.name','Fixture');self.git('config','user.email','fixture@example.invalid')
+        (self.repo/'app.py').write_text('print(1)\n');self.git('add','app.py');self.git('commit','-m','base')
+        self.base=self.git('rev-parse','HEAD').stdout.strip()
+        (self.repo/'app.py').write_text('print(2)\n');self.git('add','app.py')
+        self.commit=self.git('commit','-m','changed')
+        self.sha=self.git('rev-parse','HEAD').stdout.strip()
+        self.ledger=self.repo/'.git/sg-reviewed-shas'
+
+    def git(self,*args):
+        return subprocess.run(['/usr/bin/git',*args],cwd=self.repo,env=self.env,capture_output=True,text=True,check=True)
+
+    def client(self,mode='commit',env=None):
+        if mode=='push':
+            # A locally pushed main branch with one unreviewed source change.
+            remote=self.root/'remote.git'
+            self.git('init','--bare',str(remote));self.git('remote','add','origin',str(remote))
+            self.git('push','origin',self.base+':refs/heads/main');self.git('branch','--set-upstream-to=origin/main','main')
+            result=self.git('push','origin','main')
+            command='git push origin main'
+        else:result=self.commit;command='git commit -m changed'
+        data={'session_id':'fixture-session','cwd':str(self.repo),'tool_name':'Bash',
+              'tool_input':{'command':command},'tool_response':{'stdout':result.stdout,'stderr':result.stderr,'interrupted':False}}
+        return subprocess.run([sys.executable,'-c',CLIENT,mode],cwd=self.repo,
+                              env={**self.env,**(env or {})},input=json.dumps(data),capture_output=True,text=True,timeout=20)
+
+    def assert_failed(self,result):
+        self.assertEqual(result.returncode,2,result.stdout+result.stderr)
+        self.assertIn('could not',result.stderr.lower())
+        output=[json.loads(line) for line in result.stdout.splitlines() if line.startswith('{')]
+        self.assertTrue(output[-1]['metrics']['review_failed'])
+        self.assertTrue(output[-1]['rewakeSummary'])
+        self.assertNotIn('found issues',output[-1]['rewakeSummary'])
+        self.assertNotIn(self.sha,self.ledger.read_text() if self.ledger.exists() else '')
+        self.assertGreater(len(self.calls),0)
+
+    def test_all_failed_analysis_has_no_clean_findings_result(self):
+        r=self.client('analysis');self.assertEqual(r.returncode,0,r.stderr)
+        self.assertEqual(json.loads(r.stdout),[None,None])
+
+    def test_successful_clean_analysis_has_empty_findings(self):
+        self.statuses=[200];r=self.client('analysis')
+        self.assertEqual(r.returncode,0,r.stderr);self.assertEqual(json.loads(r.stdout),[None,[]])
+
+    def test_failed_commit_is_not_marked_reviewed(self):self.assert_failed(self.client())
+
+    def test_successful_clean_commit_is_marked_reviewed(self):
+        self.statuses=[200];r=self.client();self.assertEqual(r.returncode,0,r.stderr)
+        self.assertIn(self.sha,self.ledger.read_text())
+
+    def test_failed_commit_can_be_reviewed_after_recovery(self):
+        self.assert_failed(self.client());before=len(self.calls);self.statuses=[200]
+        r=self.client();self.assertEqual(r.returncode,0,r.stderr)
+        self.assertGreater(len(self.calls),before);self.assertIn(self.sha,self.ledger.read_text())
+
+    def test_primary_failure_with_successful_fallback_counts_as_reviewed(self):
+        self.statuses=[401,200];r=self.client();self.assertEqual(r.returncode,0,r.stderr)
+        self.assertIn(self.sha,self.ledger.read_text());self.assertEqual(len(self.calls),2)
+
+    def test_failed_push_is_not_marked_reviewed(self):self.assert_failed(self.client('push'))
+
+    def test_successful_clean_push_is_marked_reviewed(self):
+        self.statuses=[200];r=self.client('push');self.assertEqual(r.returncode,0,r.stderr)
+        self.assertIn(self.sha,self.ledger.read_text())
+
+    def test_failed_stop_reports_failure(self):
+        self.assert_failed(self.client('stop'))
+        state=json.loads((self.root/'.claude/security/security_warnings_state_fixture-session.json').read_text())
+        self.assertIn(str(self.repo/'app.py'),state['touched_paths'])
+        self.assertEqual(state['baseline_sha'],self.sha)
+
+    def test_successful_clean_stop_completes(self):
+        self.statuses=[200];r=self.client('stop')
+        self.assertEqual(r.returncode,0,r.stdout+r.stderr);self.assertGreater(len(self.calls),0)
+
+    def test_completed_review_with_findings_is_marked_reviewed(self):
+        self.statuses=[200]
+        self.findings=[{'filePath':'app.py','category':'Fixture','vulnerableCode':'print(2)',
+                        'explanation':'fixture explanation','fix':'fixture fix','severity':'high'}]
+        r=self.client();self.assertEqual(r.returncode,2,r.stdout+r.stderr)
+        self.assertNotIn('could not complete',r.stderr);self.assertIn(self.sha,self.ledger.read_text())
+
+    def test_dual_or_partial_success_is_marked_reviewed(self):
+        self.statuses=[200,401];r=self.client(env={'SG_DUAL_OR':'1','SECURITY_REVIEW_MODEL':'fixture-model'})
+        self.assertEqual(r.returncode,0,r.stderr);self.assertIn(self.sha,self.ledger.read_text())
+
+if __name__=='__main__':unittest.main()

</details>

Hi, I'm picking this up. Plan: analyze_code_security returns findings=None when every model call failed (vs [] for a completed clean review); the commit and push handlers stop before recording SHAs in sg-reviewed-shas on failure and report "security review could not run (HTTP <code>)" instead of "no security issues found"; the Stop hook keeps restoring its touched-path state and surfaces the failure once per session. — Sahil Jagtap

Fix is up: https://github.com/sahiljagtap08/claude-code/pull/3 (branch sahiljagtap/security-guidance-failed-review-not-clean, one commit on current main). Fork PRs are rejected on this repo right now, so a maintainer can cherry-pick:

git fetch https://github.com/sahiljagtap08/claude-code.git sahiljagtap/security-guidance-failed-review-not-clean
git cherry-pick FETCH_HEAD

What it does: analyze_code_security returns findings=None when every model call failed ([] still means a completed clean review). Commit review and push sweep check for that before _append_reviewed_shas and report "<scope> security review could not run (HTTP 401)" via rewakeSummary + exit 2, with review_failed/api_error in metrics. Stop keeps restoring its touched-path state and surfaces the failure once per session. Verified end-to-end against a loopback API in a temp git repo: 401 → exit 2, no ledger entry; clean 200 → unchanged. — Sahil Jagtap