anthropics / anthropics/claude-plugins-official
security-guidance: failed LLM reviews still mark commits as reviewed
- Ngôn ngữ chính
- Python
- Star
- 36.3k
- Fork
- 4.1k
- Merge trung bình
- 2 ngày 14 giờ
- Pull request đã merge (30 ngày)
- 539
Mô tả
## Description
Verified on current `main` at [`7d48aaa`](https://github.com/anthropics/claude-plugins-official/commit/7d48aaa1247a13fc422d53ca63c4eaf76aa2bd5d).
The commit-review hook writes a commit to `.git/sg-reviewed-shas` even when the LLM review did not produce a usable result.
[`analyze_code_security()`](https://github.com/anthropics/claude-plugins-official/blob/7d48aaa1247a13fc422d53ca63c4eaf76aa2bd5d/plugins/security-guidance/hooks/llm.py#L1042-L1066) currently returns `(None, [])` for both of these cases:
- a valid review that found no vulnerabilities;
- an API, network, or structured-output failure.
[`handle_commit_review_posttooluse()`](https://github.com/anthropics/claude-plugins-official/blob/7d48aaa1247a13fc422d53ca63c4eaf76aa2bd5d/plugins/security-guidance/hooks/security_reminder_hook.py#L1345-L1425) then calls `_append_reviewed_shas()` without distinguishing between them. The push sweep later treats that SHA as covered and can skip it. The existing [`_last_call_claude_http_error`](https://github.com/anthropics/claude-plugins-official/blob/7d48aaa1247a13fc422d53ca63c4eaf76aa2bd5d/plugins/security-guidance/hooks/llm.py#L448-L456) signal is surfaced only as a metric after the SHA has been persisted.
## Deterministic reproduction
1. Create a commit containing a reviewable source file.
2. Run `handle_commit_review_posttooluse()` with the real commit output.
3. Make the review call return `(None, [])` while setting `_last_call_claude_http_error = 529`.
4. Record calls to `_append_reviewed_shas()`.
On the current implementation, the handler exits successfully and appends the commit SHA despite reporting the API error:
```text
api_error=529
files_reviewed=3
reviewed_marks=['']
```
The same ambiguity exists for an HTTP 200 response with missing or malformed structured output, where an HTTP-error-only check would not be sufficient.
## Expected behavior
A commit should be persisted as reviewed only after the reviewer returns a valid structured verdict. Both a valid empty verdict and a valid finding list count as completed reviews. Transport errors and invalid responses should leave the SHA unreviewed.
## Impact
`.git/sg-reviewed-shas` is persistent state used by the later push sweep. A transient review failure is therefore recorded as successful coverage. If the commit is still part of a later sweep range, that state lets the sweep advance its base past a commit that never received a usable verdict.
This is a security-review reliability issue, not an authorization bypass: the failure does not create a vulnerability, but it can suppress the retry path intended to catch one.
## Suggested fix
Return an explicit completion status from the code-review function and gate `_append_reviewed_shas()` on that status. The status should be tied to the selected review result rather than stored in a module global, because the agentic and single-shot reviewers can race.
Regression coverage should include:
- API failure: do not persist the SHA;
- malformed or missing structured output: do not persist the SHA;
- valid no-findings verdict: persist the SHA;
- valid findings: persist the SHA;
- agentic/fallback race: use only the winning result's completion status.
## Related but distinct
- #4478 reports sessions ending without a verdict; it does not cover persistence to `.git/sg-reviewed-shas` after a failed review.
- #4852 reports a `git mv` diff-parsing false negative that is then marked reviewed; this issue is about review transport or output failure after files were selected.
- #2098 fixed a specific deprecated API parameter that caused HTTP 400 responses; the generic failed-review state ambiguity remains.
Hướng dẫn đóng góp
Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này
Hướng nghiên cứu
Start with analyze_code_security() in plugins/security-guidance/hooks/llm.py and handle_commit_review_posttooluse() plus _append_reviewed_shas() in plugins/security-guidance/hooks/security_reminder_hook.py. Reproduce the API-error case, then add regression coverage for malformed output, valid empty findings, valid findings, and the agentic/fallback race; done means only the winning valid verdict persists the SHA.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Đánh giá
- Công nghệ
- git, python
- Lĩnh vực
- security, testing-qa
- Loại issue
- Lỗi
- Độ khó
- 4/5
- Thời gian dự kiến
- 3-5 ngày
- Mức độ hoạt động
- Ít trao đổi
- Độ rõ ràng
- Đặc tả rõ ràng
- Mức phù hợp với người mới
- 68/100