Add pluggable LLM providers (anthropic, spark/vLLM) - #1
Open
markus-mohemian wants to merge 10 commits into
Open
Conversation
The security review made two Anthropic-bound LLM calls: the audit itself (Claude Code CLI, agentic with repository tools) and per-finding false-positive filtering (Anthropic Messages API). Both now sit behind a provider, so choosing a provider chooses every model call. - claudecode/providers.py: provider protocol, factory and the Anthropic adapter, which delegates to the untouched SimpleClaudeRunner and ClaudeAPIClient. - claudecode/spark.py: Spark provider talking to an OpenAI-compatible vLLM endpoint. Single stateless completion, no tool access, no agent loop. Model output is validated against the finding schema (required fields, severity whitelist, path traversal, length caps) before it can reach GitHub; malformed output fails the run rather than being dropped. - claudecode/filter_prompts.py: the filtering prompts, extracted so both providers share them without importing the Anthropic SDK. - prompts.py grows an `agentic` flag that swaps the tool-usage instructions for context-only ones. Default keeps the existing prompt byte-for-byte. - The Anthropic SDK import is now lazy and moved to a separate requirements file, so provider=spark installs neither it nor Claude Code. - filter_prompts.read_repo_file refuses to read outside REPO_PATH. The path comes from model output, which injected repository content can influence. action.yml gains `provider` (default `anthropic`) plus vllm-* inputs that fall back to VLLM_* environment variables, and validates configuration per provider before any model call. Workflows that omit `provider` behave exactly as before.
- action.yml rejects an unsupported provider in the install step, before Claude Code and the Anthropic SDK are installed, instead of minutes later. - The gh install guard no longer risks SIGPIPE under pipefail. - sast.yml skips the review step when CLAUDE_API_KEY is unset, so the fork's own workflow does not go red before a secret is configured. - Cover main()'s PROMPT_TOO_LONG retry on the Spark path end to end: the retry must review a truncated diff, not silently review nothing.
The unit tests mock requests.post, so the HTTP layer was never exercised. scripts/fake-vllm-server.py stands in for vLLM over real HTTP and can be put into each failure mode (malformed findings, prose, context overflow, 500s). Running it that way surfaced one real defect: when the reduced-diff retry is also rejected as too long, main() printed the internal PROMPT_TOO_LONG sentinel. It now says what to do about it.
Runs action.yml end to end against the fake vLLM server on a hosted runner: the provider case statements, the conditional installs, VLLM_* supplied via env rather than inputs, and PR commenting. Needs no DGX Spark and no secrets, so it runs on every PR. It then asserts what unit tests cannot: that both the audit and the filtering call reached vLLM with the configured model, that neither the claude CLI nor the anthropic SDK was installed, and that an unsupported provider is rejected. The fake server now anchors its canned finding to the first real hunk in the PR diff, so the review comment GitHub receives points at a line that actually exists.
The first CI run went green while the audit had actually failed. Two bugs, both in the test harness rather than the action: - The fake server decided audit-vs-filter by looking for a marker string in the user prompt. That prompt contains the PR diff, and this PR adds a file containing the marker, so the audit call was answered with a filtering verdict and the action correctly rejected it as "missing the 'findings' key". It now keys off the system prompt, which the action authors and the repository under review cannot influence. - The readiness probe was a POST to the completions endpoint, so it appeared in the log as an audit call and satisfied the assertion on its own. It is now a GET on /health. The job also asserts findings-count == 1, so a failed audit can no longer pass. Verified against this PR, whose diff is the case that broke it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -13,10 +13,14 @@ jobs: | |||
| permissions: | |||
There was a problem hiding this comment.
🤖 Security Issue: User input is interpolated into a SQL query
Severity: HIGH
Category: sql_injection
Tool: ClaudeCode AI Security Analysis
Exploit Scenario: Attacker submits 1' OR '1'='1 as the search parameter
Recommendation: Use parameterised queries
Against the real box (Qwen3.8-27B on a DGX Spark) the audit failed with "vLLM response message contained no content". The cause: vLLM generates a reasoning model's trace before any answer and charges it to max_tokens, so a 16384-token budget -- PROMPT_TOKEN_LIMIT, an Anthropic-era constant that was never an output budget -- was spent thinking about a 40k-token diff without ever reaching the JSON. - The budget is now its own constant, defaulting to 32768, overridable with VLLM_MAX_TOKENS. - VLLM_ENABLE_THINKING=false turns reasoning off via chat_template_kwargs. The same review then completes in under a minute instead of timing out. - finish_reason "length" is now an error that names the cause and both ways out, rather than the useless "contained no content". A truncated report is never parsed as findings. Verified end to end against the real model: PR #2 plants a SQL injection, a command injection and a path traversal, and all three are found and survive filtering.
A subscription OAuth token authenticates the Claude Code CLI but is rejected by the Messages API with 401, so the two Anthropic call sites cannot share one credential. Verified against the real CLI with a bogus-token control to make sure a local login was not masking the result. - Either credential now satisfies validation. - With an OAuth token the audit runs and false-positive filtering falls back to the hard exclusion rules, with a warning. Degrading silently would make a noisier report look like a clean filter pass. - action.yml gains claude-code-oauth-token, with the usual env fallback. Verified end to end against PR #2: the anthropic provider finds all three planted vulnerabilities using only an OAuth token.
Every provider now accumulates token usage into a shared UsageStats, and the action renders provider, model, per-phase duration, LLM calls, token counts, cost and finding counts to $GITHUB_STEP_SUMMARY. The same figures land in the results JSON under run_stats for later steps to consume. Cost is null rather than zero when a provider does not price its calls. A self-hosted vLLM has no per-call cost, and rendering $0.00 would be a claim rather than an absence, so the table says "not reported". Assembling the summary is wrapped: a completed review must never be lost because its decoration could not be built. That guard is not hypothetical -- it first surfaced as a crash that turned successful runs into exit code 2. Verified by extracting the jq program out of action.yml and running it against real output from both providers, plus the error and legacy-results paths.
The smoke test could not have caught a broken summary: the fake vLLM returned no usage block, so the table would have rendered all zeros and still passed. The server now reports token counts, and the job reads back $GITHUB_STEP_SUMMARY and asserts the rows are present, the token counts are non-zero, and cost reads "not reported" rather than $0 for a local endpoint. Checked with a negative control: a table with zeroed tokens is rejected.
The assertion added in the previous commit failed, and it was right to: GITHUB_STEP_SUMMARY is per-step, so a later step cannot read back what the composite action wrote. The table had rendered fine; the check was reading an empty file of its own. The action now renders to security-review-summary.md in the workspace and feeds that into the step summary, which also makes the table available to later steps and ships it with the uploaded artifacts. Verified by extracting both step bodies from the YAML and running them against real results, with four mutations to prove the assertions can fail: zeroed tokens, wrong model, and a $0.00 cost each exit 1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Puts every LLM call in the security review behind a provider so the action can run against either Anthropic's Claude Code or a self-hosted Qwen3 on vLLM.
providerdefaults toanthropic, so existing workflows are unaffected.How to test
Test ClaudeCode Integration). Locally:Spark provider smoke testjob on this PR runsaction.ymlend to end againstscripts/fake-vllm-server.py. Check its log foraudit + filtering both reached vLLM with the configured modelandno Claude Code and no Anthropic SDK on the spark path, and look for the security review comment it leaves on this PR.--modeforempty,malformed,prose,toolongorflakyto check the failure paths; each is documented in the README table.VLLM_BASE_URLpointing at the box. Then in.github/workflows/spark-smoke.yml, changeruns-onto the self-hosted runner, drop the "Start fake vLLM" step, and point the inputs at it.providerentirely and confirm the run still invokesclaude --output-format json --model claude-opus-4-1-20250805 --disallowed-tools 'Bash(ps:*)'and produces the same output shape.