anthropics / anthropics/claude-plugins-official

security-guidance: failed LLM reviews still mark commits as reviewed

Đang mở
#5,322 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
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

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.