anthropics / anthropics/claude-plugins-official
security-guidance: with setting_sources=[] the reviewer can't see its own cwd, so 56% of reviews waste their first Read
- Lingua principale
- Python
- Stelle
- 36.3k
- Fork
- 4.1k
- Merge medio
- 2g 14h
- PR unite (30g)
- 539
Descrizione
Measured on `security-guidance@2.0.7` (commit `4ee7b5d4146ebe60840fdf7c2256ca6104ee790d`), Claude Code CLI 2.1.x on macOS.
## Summary
The agentic security reviewer is spawned with `setting_sources=[]` and handed an
investigate prompt containing **only repo-relative paths**. Under that option the
inner CLI also loses the working-directory context, so the agent has **no source
at all** for the absolute root — it infers one from the **repository name** and
burns its first `Read` on a directory that does not exist.
It recovers on the next turn, so nothing errors and no metric moves. The cost is
a wasted turn out of `max_turns=18`, in a review whose own system prompt says:
> "The #1 cause of missed vulnerabilities is not reading the file that contains
> them."
## Minimal repro (no repo, no diff, one turn)
`setting_sources=[]` is the isolating variable. Same `cwd` in all three, only
the options differ:
```python
import asyncio
from claude_agent_sdk import ClaudeAgentOptions, AssistantMessage, query
REPO = "/Users/me/projects/acme/backend" # a repo nested one level down
Q = ("Reply with ONLY your current working directory as an absolute path, "
"or the exact word UNKNOWN if you were never told it.")
async def probe(label, **kw):
opts = ClaudeAgentOptions(cwd=REPO, allowed_tools=[], max_turns=1, **kw)
out = ""
async for m in query(prompt=Q, options=opts):
if isinstance(m, AssistantMessage):
for b in m.content:
out += getattr(b, "text", "") or ""
print(label, "->", out.strip(), "OK" if out.strip().startswith(REPO) else "WRONG")
async def main():
await probe("system_prompt=str + setting_sources=[]",
system_prompt="You answer in one line.", setting_sources=[])
await probe("system_prompt=str, setting_sources unset",
system_prompt="You answer in one line.")
await probe("no system_prompt + setting_sources=[]", setting_sources=[])
asyncio.run(main())
```
Observed:
| options | answer | |
|---|---|---|
| `system_prompt=str` + `setting_sources=[]` | `.../projects/acme-backend` | **WRONG** |
| `system_prompt=str`, `setting_sources` unset | `.../projects/acme/backend` | OK |
| no `system_prompt` + `setting_sources=[]` | `.../projects/acme-backend` | **WRONG** |
The agent does not report "UNKNOWN": it confidently returns the repo directory
name with the separator collapsed. That is exactly the path the reviewer then
tries to `Read`.
(For what it's worth, `claude -p --system-prompt "..."` on the same cwd answers
correctly, so the plain-string `system_prompt` is not the variable here —
`setting_sources=[]` is.)
## Field measurement
Population: every session under `~/.claude/projects/*/*.jsonl` in a 7-day window
whose first record is a `queue-operation` whose `content` **starts with**
`Review this change for security vulnerabilities.` — that is the investigate
prompt verbatim, so the match is exact. (A containment check instead of
`startswith` yields the same 308 here, but false-positives on any session that
merely quotes the string; use `startswith`.) Metric: the **first** `tool_use` of
that session, checked with `os.path.exists`.
| repo | sessions | first call to a non-existent path |
|---|---|---|
| `projects/psi-engine/backend` | 117 | **109** |
| `projects/psi-engine/frontend` | 81 | **61** |
| `$HOME` (repo root == home) | 102 | **0** |
| **all repos** | **308** | **173 (56%)** |
Typical failure: the real tree is `~/projects/psi-engine/backend`, and the
reviewer's first call is
`Read ~/projects/psi-engine-backend/infra/deploy.sh` — one separator short.
The 0/102 row is the tell: when the repo root has no nested segment, guessing
from the name happens to be right. The bug only fires for repos nested a level
down.
## Root cause
`hooks/llm.py::agentic_review()` builds the investigate prompt as
`"Changed files ..."` + relative paths + `context_note`, and `context_note` is
non-empty **only** under the eval harness (`SG_AGENTIC_CONTEXT_DIR` set). In
production it is `""`, so nothing ever names `opts.cwd` — and per the repro
above, `setting_sources=[]` means the agent cannot recover it from anywhere else.
Worth flagging for whoever picks this up: `review_api.build_investigate_prompt()`
looks like the fix site — it is documented as "the importable surface" and has
the same defect — but it currently has **zero callers**; the shipped hook path
goes through `llm.agentic_review()`'s own inline copy of the prompt. Patching
only `review_api.py` compiles, reviews clean, and changes nothing at runtime.
The two copies also differ: `build_investigate_prompt` appends
`extensibility.guidance_block()`, which the live path does not emit — so they
can't simply be collapsed into one call without changing the production prompt.
## Proposed fix
Put the text in one place and have both paths consume it (`llm.py` already
imports `review_api`, so no new dependency and no import cycle):
```python
# review_api.py
def build_repo_root_note(repo_root: str) -> str:
if not repo_root:
return ""
return (
f"\n\nREPO ROOT (absolute): {repo_root}\n"
"The paths listed above are RELATIVE to that root -- prefix it "
"before any Read/Grep/Glob. Do NOT guess an absolute path from "
"the repository name.\n"
)
```
`build_investigate_prompt()` gains a keyword-only `repo_root: str = ""`
(backwards compatible — omitting it emits nothing) and inserts the note before
`context_note`. In `llm.agentic_review()`, `context_note` is seeded with
`review_api.build_repo_root_note(context_dir)` and the existing harness NOTE is
appended with `+=`. `context_dir` is exactly what is passed as `opts.cwd`, so it
is correct in both the production and eval-harness branches.
Full diff (applies with `patch -p1` from the repo root):
```diff
--- a/plugins/security-guidance/hooks/review_api.py
+++ b/plugins/security-guidance/hooks/review_api.py
@@ -151,12 +151,35 @@
},
"required": ["findings"],
}
+
+
+def build_repo_root_note(repo_root: str) -> str:
+ """Absolute-root line for the investigate prompt.
+
+ The reviewer spawns with ``setting_sources=[]`` (no CLAUDE.md, no
+ project settings) and a hard turn cap, so nothing else in its context
+ says where the repo lives on disk. Handed only repo-relative paths it
+ guesses an absolute root from the repository name and burns its first
+ Read on a directory that does not exist -- in a review whose own system
+ prompt says the #1 cause of a missed vulnerability is not reading the
+ file. Canonical text lives here because ``llm.agentic_review`` builds
+ its own copy of the investigate prompt and must emit the same line.
+ """
+ if not repo_root:
+ return ""
+ return (
+ f"\n\nREPO ROOT (absolute): {repo_root}\n"
+ "The paths listed above are RELATIVE to that root -- prefix it "
+ "before any Read/Grep/Glob. Do NOT guess an absolute path from "
+ "the repository name.\n"
+ )
def build_investigate_prompt(
touched_paths: list[str],
diff_files: list[tuple[str, str]],
*,
+ repo_root: str = "",
context_note: str = "",
) -> str:
capped, _ = cap_diff_for_prompt(diff_files)
@@ -167,6 +190,7 @@
"Review this change for security vulnerabilities.\n\n"
"Changed files (you may Read these and any other file in the repo):\n"
+ "\n".join(f" - {p}" for p in touched_paths[:50])
+ + build_repo_root_note(repo_root)
+ context_note
+ "\n\nUnified diff (only + lines are new):\n\n"
+ diff_text
--- a/plugins/security-guidance/hooks/llm.py
+++ b/plugins/security-guidance/hooks/llm.py
@@ -1195,10 +1195,10 @@
# trace cross-file data flow. The harness sets SG_AGENTIC_CONTEXT_DIR to a
# full repo (worktree at the commit, or the live clone at HEAD).
context_dir = os.environ.get("SG_AGENTIC_CONTEXT_DIR") or repo_dir
- context_note = ""
+ context_note = review_api.build_repo_root_note(context_dir)
if context_dir != repo_dir:
- context_note = (
- "\n\nNOTE: your working directory is the full repository for "
+ context_note += (
+ "\nNOTE: your working directory is the full repository for "
"context (Grep for callers, read related files). The DIFF below "
"is authoritative for what changed — the repo checkout may be at "
"a different commit, so if a touched file looks different on "
```
## Verification
- Intercepted `claude_agent_sdk.query` to capture the real production prompt:
the line is present and matches `opts.cwd`.
- Two live post-patch reviews made their **first** call to
`~/projects/psi-engine/backend/infra/deploy.sh`, which exists, and completed
the two-stage pipeline (`investigate_turns: 5`, 3 findings surviving
self-refute, no `agentic_fallback`).
- A/B on a second machine, same prompt except the line: without it the first
`Read` misses and the agent has to `find` the repo; with it, zero failed reads.
## Related, pre-existing (not caused by this patch)
Findings come back with an **absolute** `filePath` in 278 of 288 pre-patch
sessions measured, while the dedup keys downstream — `_dedup_against_state` and
the `seen` sets in `security_reminder_hook.py` — compare `(filePath, category)`
against `previous_findings` written by other producers in repo-relative form. So
the two shapes already fail to match today; the worst case is a duplicated
finding, never a dropped one. Stating the expected `filePath` shape in the
investigate prompt would close it. Stage 2 (`build_refute_prompt`) likewise never
receives the root; it fails open (`survived = candidates`), so it degrades
precision, not recall.
Guida per i contributori
Nessuna guida per i contributori indicizzata per questo repository
Direzione di ricerca
Start in plugins/security-guidance/hooks/review_api.py and plugins/security-guidance/hooks/llm.py, especially build_investigate_prompt() and llm.agentic_review(). The issue includes the intended prompt text and a patch; verify both prompt-building paths include the absolute repo root passed as opts.cwd. Done means the production investigate prompt contains the REPO ROOT note and still preserves the existing eval harness note.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- python
- Ambito
- ai-infra-agents, security, tooling
- Tipo di issue
- Bug
- Difficoltà
- 2/5
- Tempo stimato
- 1-3 ore
- Stato di attività
- Attiva
- Chiarezza
- Specificata chiaramente
- Idoneità per principianti
- 82/100