Fix review pipeline bugs, improve performance, and harden against prompt injection - #40
Fix review pipeline bugs, improve performance, and harden against prompt injection#40matej wants to merge 2 commits into
Conversation
Correctness fixes: - Claude API findings validation was silently disabled for everyone: validate_api_access() pinged the retired claude-3-5-haiku-20241022 (404s since Feb 2026), causing FindingsFilter to drop the Claude filtering stage on every run. Now validates against claude-haiku-4-5. - The claudecode-timeout action input was never applied: action.yml exports CLAUDE_TIMEOUT but the Python never read it, so reviews always ran with the 20-minute default. initialize_clients() now wires it up. - comment-pr-findings.js fetched only the first 100 PR files, silently dropping inline comments for findings in later files. Now paginates. - GitHub API requests had no HTTP timeout and could hang the action indefinitely; all requests now use a 30s timeout. - Malformed (non-dict) findings from the model could crash the final severity count with an unhandled exception; they are now skipped with a warning, and the duplicate exit-code severity count was removed. Performance: - Claude API finding validation now runs in parallel (4 workers) instead of sequentially per finding - the filtering stage on a 10-finding PR drops from ~10x to ~3x single-call latency. - Bot-comment reactions are no longer fetched one API call per comment: the embedded reactions summary short-circuits the N+1 pattern when only the bot's seed reactions exist. - Diff packing no longer stops at the first oversized file; smaller files after it still fit into the embedded diff (one giant generated file no longer evicts the rest of the PR from full-diff review). - File content embedded in filter prompts is now windowed (±150 lines around the finding, 40k char cap) with line numbers, instead of entire files of unbounded size. - action.yml skips apt-get for gh/jq when already present (both are pre-installed on GitHub-hosted runners; saves two apt round-trips per run) and upgrades Node 18 (EOL) to Node 22. Security hardening: - The Claude review subprocess no longer inherits GITHUB_TOKEN/GH_TOKEN (the review only needs the local checkout), and network-capable tools (WebFetch, WebSearch, curl, wget, nc) are disallowed - a prompt-injected review can no longer exfiltrate data or act on GitHub. - The review prompt now instructs Claude to treat instructions embedded in PR content as a malicious signal to report, not follow. - Expanded security categories: SSRF, CSRF, CORS misconfiguration, disabled TLS verification, IDOR, TOCTOU, supply-chain (typosquatted deps, install hooks) and CI/CD risks (workflow injection, dangerous pull_request_target patterns, unpinned mutable action refs). - Memory-safety findings are now kept for .hpp/.cxx/.hh/.hxx/.m/.mm files (previously only .c/.cc/.cpp/.h). Review quality: - Inline comments now dedupe against existing bot comment threads (path + title), so re-reviews stop posting duplicate threads for still-unresolved findings. - The final prompt reminder uses a concrete reporting bar instead of "better to miss than flood" phrasing, which measurably depresses recall on Opus 4.7+ models; borderline-real findings are now reported with calibrated confidence and ranked by the downstream filter. Tests: 16 new Python tests (claudecode/test_review_hardening.py plus runner env/tooling assertions) and 2 new bun tests (files pagination, finding dedup). 237 Python + 27 JS tests passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 PR Summary:
This PR fixes several real bugs in the review pipeline (retired API-validation model that silently disabled Claude false-positive filtering, unwired claudecode-timeout, un-paginated PR files fetch in the JS commenter, missing HTTP timeouts, crash on non-dict findings), adds performance work (parallel per-finding validation, reactions N+1 short-circuit, greedy diff packing, windowed filter context), and adds prompt-injection hardening (expanded tool denylist, GitHub credentials stripped from the review subprocess, untrusted-content instruction in the review prompt). It also broadens the security prompt taxonomy, loosens the final noise filter, and adds tests for the new behaviors. The changes touch the highest-value paths of the action, so the correctness of the new filtering/packing heuristics and the strength of the injection mitigations matter a lot.
12 files reviewed
| File | Changes |
|---|---|
claudecode/github_action_audit.py |
HTTP timeouts, greedy diff packing, reactions short-circuit, tool denylist, env stripping, timeout wiring |
claudecode/claude_api_client.py |
Current validation model; windowed, line-numbered file context for filter prompts |
claudecode/findings_filter.py |
Parallel per-finding Claude validation; more C/C++/ObjC extensions |
claudecode/constants.py |
New constants for validation model, timeouts, workers, windowing |
claudecode/prompts.py |
Untrusted-content instruction, expanded security categories, loosened final reminder |
scripts/comment-pr-findings.js |
Paginated PR files fetch and inline-comment deduplication by file+title |
action.yml |
Skip redundant gh/jq installs; bump Node to 22 |
tests |
Tests for hardening, timeouts, windowing, packing, pagination, dedup |
README.md |
Rewritten security-considerations section describing new mitigations |
Found 8 security, correctness, reliability, and maintainability issues. Consider addressing the suggestions in the comments.
| # prompt-injection attempt in a malicious PR cannot exfiltrate data | ||
| # or fetch attacker-controlled instructions; ps is disallowed so | ||
| # credentials can't be snooped from process listings. | ||
| disallowed_tools = ','.join([ |
There was a problem hiding this comment.
🤖 Code Review Finding: Network tool denylist is trivially bypassable, yet README claims network tools are disallowed
Severity: MEDIUM
Category: security
Impact: A prompt injection embedded in a malicious PR diff can still exfiltrate repository contents or secrets from the runner (e.g. python3 -c HTTP POST, or git push to an attacker remote), while maintainers reading the new README section may believe egress is blocked and relax the "trusted PRs only" discipline.
Recommendation: Switch from a denylist to an allowlist for the review subprocess (e.g. --allowed-tools 'Read,Grep,Glob,Bash(git diff:*),Bash(git log:*),Bash(git show:*)'), or run the review with no Bash at all / behind a network-egress sandbox. Then reword the README to describe the actual guarantee rather than naming a few blocked binaries.
| # credentials (the repo is already checked out and diffs are | ||
| # local). Stripping them limits the blast radius of any | ||
| # prompt-injected tool use. | ||
| subprocess_env = { |
There was a problem hiding this comment.
🤖 Code Review Finding: Stripping GITHUB_TOKEN/GH_TOKEN does not remove the git-persisted credential the subprocess can read
Severity: MEDIUM
Category: security
Impact: A prompt-injected review can recover the workflow token via Read .git/config or git config --get-regexp http.*extraheader and use it (combined with any remaining egress path) to act on the repository, defeating the stated mitigation.
Recommendation: Either document/require persist-credentials: false on actions/checkout in the README examples, or have the action unset the persisted header for the duration of the review (git config --unset-all http.https://github.com/.extraheader, restoring it afterwards if later steps need it). Also soften the README wording to "GitHub credentials are removed from the subprocess environment" rather than an absolute claim.
| # If truncated, stop fetching further pages (saves API calls) | ||
| if is_truncated: | ||
| logger.info(f"Diff truncated at {files_with_patches} files ({current_chars} chars)") | ||
| break |
There was a problem hiding this comment.
🤖 Code Review Finding: Greedy diff packing still stops pagination on the first skipped file, so later pages are never considered
Severity: MEDIUM
Category: correctness
Impact: On a PR with, say, 150 changed files where file #3 is a large generated file, files 101-150 are never fetched or packed even though ample character budget remains — those files silently never reach the embedded diff (and are absent from included_file_list/files_reviewed), reproducing exactly the eviction problem the change set out to fix.
Recommendation: Keep paginating and only stop when the budget is actually exhausted; use is_truncated purely as a reporting flag for partial-diff mode.
| # If truncated, stop fetching further pages (saves API calls) | |
| if is_truncated: | |
| logger.info(f"Diff truncated at {files_with_patches} files ({current_chars} chars)") | |
| break | |
| # Keep fetching further pages: an oversized file only skips | |
| # itself, so smaller files on later pages can still be packed. | |
| if current_chars >= max_diff_chars: | |
| logger.info(f"Diff budget exhausted at {files_with_patches} files ({current_chars} chars)") | |
| break |
| numbered.append(f"... ({total_lines - end} later lines omitted)") | ||
|
|
||
| result = '\n'.join(numbered) | ||
| if len(result) > max_chars: |
There was a problem hiding this comment.
🤖 Code Review Finding: Hard character cap can truncate away the very line the filter is asked to validate
Severity: MEDIUM
Category: correctness
Impact: The per-finding validator receives context that stops before the flagged location while being told the content is "windowed around the finding", so it must judge blind and is likely to mark real findings as unverifiable/false positives — silently dropping true findings for exactly the large-line files the windowing was added to handle.
Recommendation: Enforce the character budget relative to the focus line instead of the tail: accumulate lines outward from focus_line until max_chars is reached (or trim the leading portion of the window and mark it as truncated) so the focus line is always present. Add a test that a window of very long lines still contains the focus line.
| # Simple test call to verify API access | ||
| self.client.messages.create( | ||
| model="claude-3-5-haiku-20241022", | ||
| model=API_VALIDATION_MODEL, |
There was a problem hiding this comment.
🤖 Code Review Finding: Validation pings a different model than filtering uses, so a bad claude-model still silently disables filtering
Severity: MEDIUM
Category: reliability
Impact: The same class of failure this PR fixes recurs in a new form: filtering appears enabled, logs a success line, and then silently keeps 100% of findings while paying full retry latency (3 retries plus backoff per finding). The hardcoded alias will also rot again when Haiku 4.5 is retired.
Recommendation: Validate with self.model (a max_tokens=1 ping is cheap and proves the real dependency), keeping API_VALIDATION_MODEL only as an explicit fallback, and escalate repeated per-finding API failures to a visible warning that filtering is effectively off.
| model=API_VALIDATION_MODEL, | |
| model=self.model, |
| plus_one = summary.get('+1', 0) | ||
| minus_one = summary.get('-1', 0) | ||
| # Only skip when the summary is consistent with just the bot's seed | ||
| # thumbs (at most one of each, and no other reaction types). | ||
| return not (total <= 2 and plus_one <= 1 and minus_one <= 1 | ||
| and plus_one + minus_one == total) |
There was a problem hiding this comment.
🤖 Code Review Finding: Reactions short-circuit also skips single-thumb summaries, which can hide a human reaction
Severity: LOW
Category: correctness
Impact: Reviewer 👍/👎 feedback on such comments is silently treated as absent, so the next run loses the signal it was meant to feed back into the prompt (e.g. a 👎 marked false positive gets re-reported).
Recommendation: Only skip when the summary is empty or exactly one 👍 and one 👎 (both seeds present); fetch in every other case. This keeps essentially all of the N+1 saving.
| plus_one = summary.get('+1', 0) | |
| minus_one = summary.get('-1', 0) | |
| # Only skip when the summary is consistent with just the bot's seed | |
| # thumbs (at most one of each, and no other reaction types). | |
| return not (total <= 2 and plus_one <= 1 and minus_one <= 1 | |
| and plus_one + minus_one == total) | |
| plus_one = summary.get('+1', 0) | |
| minus_one = summary.get('-1', 0) | |
| # Skip only when there are no reactions at all, or exactly the bot's | |
| # two seed thumbs. A single thumb may be a human reaction on a comment | |
| # whose seeding partially failed, so it must still be fetched. | |
| if total == 0: | |
| return False | |
| return not (total == 2 and plus_one == 1 and minus_one == 1) |
| // Skip findings that already have an inline comment thread from a | ||
| // previous review run (dismissing a review keeps its comments, so | ||
| // re-posting would create duplicate threads) | ||
| if (existingFindingKeys.has(`${file}::${title}`)) { |
There was a problem hiding this comment.
🤖 Code Review Finding: Deduplication keyed only on file+title pins persistent findings to stale line positions
Severity: LOW
Category: maintainability
Impact: Long-lived PRs accumulate findings whose only visible comment is attached to an outdated hunk, so a HIGH finding that still applies can be effectively invisible in the Files-changed view while the summary review still says "Please address the high-severity issues before merging" with zero inline comments.
Recommendation: When a key matches but the finding's current line differs from the existing comment's line, update the existing comment in place (PATCH /repos/{o}/{r}/pulls/comments/{id}) or post a reply in that thread pointing at the new location, instead of skipping silently; at minimum log the suppressed findings into the review body so they remain discoverable.
| @@ -305,7 +329,13 @@ def get_unified_review_prompt( | |||
| - Below 0.7: Don't report (too speculative) | |||
|
|
|||
| FINAL REMINDER: | |||
There was a problem hiding this comment.
🤖 Code Review Finding: Loosened final reminder assumes a downstream filter that is disabled by default
Severity: LOW
Category: maintainability
Impact: Default installs get a strictly more permissive reporting bar with no compensating validation stage, increasing borderline/LOW findings — and since the exit code and REQUEST_CHANGES verdict are driven by HIGH counts, a mis-calibrated HIGH now fails builds more often.
Recommendation: Make the sentence conditional on filtering actually being enabled (the prompt is already parameterized elsewhere), or flip enable-claude-filtering to default true now that validation works again, so the prompt's stated assumption matches runtime behavior.
Both reviews independently confirmed several weaknesses in the previous commit; all valid findings are addressed here. Security (GitHub #1/#2, Codex F8): - Replace the bypassable Bash denylist with an allowlist: the review subprocess may only run read-only git commands (diff/log/show/status/ blame); everything else (python, node, openssl, arbitrary binaries) is denied in headless mode. Network tools stay denylisted as defense in depth. - Remove credentials persisted by actions/checkout from .git/config before the scan step, so the review subprocess cannot read the workflow token (env stripping alone did not cover this). - Reword README to describe defense-in-depth honestly instead of claiming egress is blocked. Correctness (GitHub #3/#4/#5/#6, Codex F1/F2/F3/F5/F9): - Diff packing now keeps fetching later pages until the character budget is genuinely exhausted; an oversized file on page 1 no longer hides every file on pages 2+. - The filter-prompt window now grows outward from the finding line, so the char cap can never truncate away the very line being validated; focus lines beyond EOF clamp to the end of the file. - API validation now pings the configured model instead of a hardcoded one - a misconfigured/retired CLAUDE_MODEL is caught up front instead of silently failing open on every finding (plus a loud warning when all validation calls fail). - Reactions short-circuit is stricter: only an exact two-seed summary skips the fetch; single thumbs (possible human reaction after seed failure) and null counters are fetched safely. Review quality (GitHub #7/#8, Codex F6/F7/F10): - Dedup no longer suppresses findings whose previous thread is outdated (position: null) and only matches bot-authored comments; suppressed duplicates are listed in the review summary so they stay discoverable. - Comment pagination degrades gracefully on mid-pagination API errors (e.g. GitHub's 3,000-file cap) instead of aborting the run; a page-1 failure still surfaces as an error. - The prompt's borderline-finding guidance now reflects whether the downstream Claude filter is actually enabled at runtime, instead of assuming it. - The injection guardrail no longer demands a HIGH finding for inert prompt-injection strings in test fixtures/docs; it asks for judgment and intent-matched severity. Tests: 243 Python + 28 JS passing (6 new Python tests, 1 new JS test, dedup mocks updated for live-position/bot-author semantics). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review findings in f97c10a, cross-checked against an independent adversarial review (Codex) of the same diff — the two reviews agreed on the core issues. Fixed:
Adopted from the adversarial review beyond the above: graceful degradation on mid-pagination API errors (3,000-file cap, Codex F6) while still failing loudly on page-1 errors, and a softened injection guardrail that doesn't flag inert prompt-injection strings in test fixtures/docs as attacks (Codex F10). Not adopted: in-place PATCH-updating of drifted comment threads (finding 7's stronger variant) — the summary-note + outdated-thread re-posting covers the discoverability gap at much lower complexity; can revisit if drift proves noisy in practice. Tests: 243 Python + 28 JS passing. 🤖 Generated with Claude Code |
1 participant
Stacked on #39. An extensive review of the current implementation surfaced real bugs, performance waste, and hardening gaps. Research into what other LLM review tools (upstream anthropics/claude-code-security-review, CodeRabbit, Greptile, Qodo, Copilot code review, Cursor BugBot) do informed the quality changes.
Correctness fixes
validate_api_access()pinged the retiredclaude-3-5-haiku-20241022claude-haiku-4-5.claudecode-timeoutinput never appliedCLAUDE_TIMEOUTbut Python never read it; reviews always used the 20-min default. Now wired throughinitialize_clients().comment-pr-findings.jsPerformance
enable-claude-filteringis on, and it now actually turns on again).gh/jq(two apt round-trips saved per run); Node 18 (EOL since April 2025) → Node 22.Security hardening (we're cyber-verified now — the review does more, and is itself harder to abuse)
GITHUB_TOKEN/GH_TOKEN; the review only needs the local checkout.WebFetch,WebSearch,Bash(curl:*),Bash(wget:*),Bash(nc:*)added to disallowed tools — a prompt-injected review can no longer exfiltrate data or fetch attacker instructions.pull_request_targetpatterns, unpinned mutable action refs) — sourced from the GitHub Security Lab checklist..hpp/.cxx/.hh/.hxx/.m/.mmfiles (previously only.c/.cc/.cpp/.h— same gap as upstream PR fix(filter): don't drop memory-safety findings in .hpp/.cxx/.hh C++ files anthropics/claude-code-security-review#110).Review quality
Testing
pytest claudecode: 237 passed (1 pre-existing environment-dependent failure onmain, unrelated).bun test: 27 passed.🤖 Generated with Claude Code