anthropics / anthropics/claude-plugins-official

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

Aberta
#5,322 1 comentário 0 reações 0 responsáveis Ver no GitHub
Linguagem predominante
Python
Estrelas
36.3k
Forks
4.1k
Merge médio
2d 14h
PRs com merge (30d)
539

Descrição

## 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.

Guia de contribuição

Nenhum guia de contribuição indexado para este repositório

Direção de pesquisa

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.

Escrita pelo modelo de indexação a partir do texto da issue.

Avaliação

Stack de tecnologia
git, python
Domínio
security, testing-qa
Tipo de issue
Bug
Dificuldade
4/5
Tempo estimado
3-5 dias
Status de atividade
Pouca atividade
Clareza
Claramente especificada
Facilidade para iniciantes
68/100

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.