anthropics / anthropics/claude-code

[BUG] Race condition in security-guidance's _agentic_review_with_race causes redundant security analysis

Đang mở
#93,310 2 bình luận 0 reaction 0 người được giao Xem trên GitHub
area:plugins area:security bug
Ngôn ngữ chính
Python
Star
145k
Fork
23.1k
Chỉ số merge pull request
Chỉ số pull request đang chờ

Mô tả

### Preflight Checklist

- [x] I have searched [existing issues](https://github.com/anthropics/claude-code/issues?q=is%3Aissue%20state%3Aopen%20label%3Abug) and this hasn't been reported yet
- [x] This is a single bug report (please file separate reports for different bugs)
- [x] I am using the latest version of Claude Code

### What's Wrong?

In `plugins/security-guidance/hooks/security_reminder_hook.py`, the function `_agentic_review_with_race` runs two threads in a race: an agentic reviewer thread (`_agentic`) and a delayed fallback reviewer thread (`_fallback`). They communicate their completion using a shared queue `q` of size 1.

If the agentic thread finishes first, it puts its result into `q`. The main thread immediately retrieves this result using `q.get()`, which empties the queue, and then returns.

When the fallback thread wakes up after its delay (`delay_s`), it checks if the agentic thread finished by calling `if not q.empty(): return`. Because the main thread already consumed the item from `q`, the queue is empty. The fallback thread incorrectly assumes the agentic thread hasn't finished, so it proceeds to run the expensive `analyze_code_security` function anyway. This results in redundant LLM/security analysis calls and wasted resources — silently, with no error or indication anything went wrong.

### What Should Happen?

The fallback thread should correctly detect that the agentic reviewer already finished (via a dedicated completion signal, not queue emptiness) and skip re-running `analyze_code_security` entirely when the agentic reviewer already succeeded within the delay window.

### Error Messages/Logs

```shell
None — this is a silent correctness/efficiency bug, not a crash. That's part of what makes it easy to miss: analyze_code_security simply runs twice with no warning.
```

### Steps to Reproduce

This was found via static code review, not live reproduction — no special setup is needed to see the bug, just reading the logic:

1. Open `plugins/security-guidance/hooks/security_reminder_hook.py`.
2. Locate `_agentic_review_with_race`. Note `q = queue.Queue(maxsize=1)`.
3. Trace what happens when `_agentic` finishes before the `delay_s` timeout: it does `q.put_nowait(("agentic", r))`, and the main thread's `winner, (g, v, m) = q.get()` immediately drains that item.
4. Trace `_fallback()`: after `time.sleep(delay_s)`, it checks `if not q.empty(): return`. Since the queue was already drained in step 3, this check is always False in this scenario — `_fallback` proceeds to call `analyze_code_security` even though `_agentic` already completed successfully.

To confirm at runtime: add a log line at the start of `_fallback()`'s `analyze_code_security` call, make a commit where the agentic reviewer typically finishes well within `SG_AGENTIC_RACE_DELAY_S` (default 180s), and observe the fallback's expensive path still fires.

### Claude Model

None

### Is this a regression?

I don't know

### Last Working Version

_No response_

### Claude Code Version

N/A — found via static code review of the plugin source (plugins/security-guidance/hooks/security_reminder_hook.py on the main branch), not a live reproduction tied to a specific runtime/OS/terminal combination. The bug is in pure Python thread-synchronization logic, independent of platform.

### Platform

Anthropic API

### Operating System

macOS

### Terminal/Shell

PyCharm terminal

### Additional Information

Suggested fix — use a dedicated threading.Event instead of queue emptiness as the completion signal:

def _agentic_review_with_race(
repo_root: str,
diff_files: List[Tuple[str, str]],
rel_touched: List[str],
previous_findings: List[Dict[str, Any]],
) -> Tuple[Optional[str], List[Dict[str, Any]], Dict[str, Any]]:
"""Race the agentic reviewer against a delayed single-shot fallback."""
import queue as _queue
import threading as _th
import time as _t

if os.environ.get("SG_AGENTIC_NO_RACE") == "1":
return agentic_review(repo_root, diff_files, rel_touched)

delay_s = int(os.environ.get("SG_AGENTIC_RACE_DELAY_S", "180"))
q: "_queue.Queue[Tuple[str, Any]]" = _queue.Queue(maxsize=1)
fallback_started = _th.Event()
agentic_finished = _th.Event()

def _agentic() -> None:
try:
r = agentic_review(repo_root, diff_files, rel_touched)
except Exception as e:
r = (None, [], {"agentic_fallback": f"race_crash:{type(e).__name__}"})
try:
q.put_nowait(("agentic", r))
except _queue.Full:
pass
finally:
agentic_finished.set()

def _fallback() -> None:
_t.sleep(delay_s)
if agentic_finished.is_set():
return # agentic finished within the delay — never start fallback
fallback_started.set()
try:
g, v = analyze_code_security(
diff_files, is_diff=True, previous_findings=previous_findings
)
except Exception as e:
g, v = None, []
try:
q.put_nowait(("fallback", (g, v, {"agentic": False})))
except _queue.Full:
pass

_th.Thread(target=_agentic, daemon=True).start()
_th.Thread(target=_fallback, daemon=True).start()

winner, (g, v, m) = q.get()
m = dict(m)
m["race_winner"] = 1 if winner == "agentic" else 2
m["race_delay_s"] = delay_s
m["race_started"] = 1 if fallback_started.is_set() else 0
return g, v, m

Confidence: high — directly visible in the thread-synchronization logic; confirmed by tracing queue state through the code, no runtime logs needed.

Found via an AI code-investigation tool (Meeba Brain) while testing its bug-discovery accuracy on real open-source repos — verified by hand against the source before filing.

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 in plugins/security-guidance/hooks/security_reminder_hook.py at _agentic_review_with_race and trace the agentic and fallback thread paths, including the queue and completion checks. Review any existing security-guidance hook tests before making changes. Done means an agentic review that finishes before the delay does not trigger a second analyze_code_security call, while the fallback still works when needed.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Lĩnh vực
security
Loại issue
Lỗi
Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
75/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.