ci(pr-review): wire OpenRouter reviewer (deepseek-flash, sticky comment, inline findings) - #76
Conversation
…nt, inline findings) Replace the placeholder no-op review workflow (which posted a canned "AI Code Review Complete" comment on every PR) with a real reviewer: - .github/scripts/pr-review.mjs — Node 20, zero npm deps. Diffs base...HEAD (same excludes as before, ~80k char cap), sends it to OpenRouter deepseek/deepseek-v4-flash (fallback qwen/qwen3.7-plus on error/429), parses strict-JSON findings (max 8, empty = clean). - Inline review comments where the diff supports line anchoring; unanchorable findings land in the summary instead. - ONE sticky summary comment per PR (marker <!-- pr-review-bot -->), updated in place on each push — no comment spam. - Reviewer failures never block merges: continue-on-error + the script always exits 0 and reports failures in the sticky comment. - Workflow: per-PR concurrency with cancel-in-progress, dependabot PRs skipped, setup-node@v4 (node 20). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🔴 critical: execFileSync is called with git and arguments that include origin/${base}...HEAD. If base contains spaces or special characters (e.g., from a malicious branch name), this could lead to argument injection. Use execFileSync with an array of arguments to avoid shell interpretation, or validate base against a safe pattern.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🔴 critical: The git function uses execFileSync with a single string argument, which on Unix is passed to /bin/sh -c. This allows shell metacharacters in the base variable to be interpreted, potentially leading to command injection. Use an array of arguments instead.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🟡 warning: The git function does not validate that base is a safe branch name. If base contains characters like ; or backticks, the shell invocation could execute arbitrary commands. Consider using execFileSync with an array of arguments to avoid shell interpretation.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🟡 warning: The git function uses execFileSync with a single string argument, which on Unix is passed to /bin/sh -c. This allows shell metacharacters in the base variable to be interpreted, potentially leading to command injection. Use an array of arguments instead.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🟡 warning: The git function does not validate that base is a safe branch name. If base contains characters like ; or backticks, the shell invocation could execute arbitrary commands. Use execFileSync with an array of arguments to avoid shell interpretation.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🟡 warning: The git function uses execFileSync with a single string argument, which on Unix is passed to /bin/sh -c. This allows shell metacharacters in the base variable to be interpreted, potentially leading to command injection. Use an array of arguments instead.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🟡 warning: The git function does not validate that base is a safe branch name. If base contains characters like ; or backticks, the shell invocation could execute arbitrary commands. Use execFileSync with an array of arguments to avoid shell interpretation.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
| return execFileSync("git", args, { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🟡 warning: The git function uses execFileSync with a single string argument, which on Unix is passed to /bin/sh -c. This allows shell metacharacters in the base variable to be interpreted, potentially leading to command injection. Use an array of arguments instead.
There was a problem hiding this comment.
Verified against the code — this is a false positive. git(...args) calls execFileSync("git", args, {...}) with no shell: true option, so Node executes the git binary directly (via execvp), never /bin/sh -c. There is no shell, so shell metacharacters (;, backticks, $()) in any argument are inert — they are passed to git as literal argv strings. The code already uses the array-of-arguments form this comment recommends as the fix.
On the origin/${base}...HEAD argument-injection angle: base is the PR base branch (BASE_REF), and it is concatenated behind the literal origin/ prefix, so the resulting argv element can never begin with - and thus can never be parsed by git as a flag/option. No injection path exists here. No change made.
🤖 AI Code ReviewThe diff introduces a new AI-powered PR review script and workflow. The implementation is generally sound, but there are a few correctness and security concerns: the diff parsing logic may skip anchorable lines, the inline comment deduplication is fragile, and the API key is passed via environment variable without masking in logs. Findings: 0 critical · 3 warning · 2 suggestion (5 posted inline) model: |
…nline comments Self-test on PR #76 showed the model emitting 8 near-duplicate findings on one line. Dedupe by (path, line) after parsing, instruct the model that findings must be distinct, and skip inline comments that were already posted verbatim on a previous push. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| if (l.startsWith("+++ b/")) { | ||
| path = l.slice(6); | ||
| if (!anchors.has(path)) anchors.set(path, new Set()); | ||
| continue; |
There was a problem hiding this comment.
🟡 warning: The condition l === "" includes the empty string produced by splitting the final newline of the diff. This adds one extra line number to the anchor set for the current file. If the AI model outputs a finding at that line, the entire inline review comment batch will be rejected by the GitHub API, causing all anchorable findings to be silently demoted to the summary comment. Add a continue before this line when l === "" to skip it.
There was a problem hiding this comment.
Confirmed legit — fixed in 7a5503c. diff.split("\n") always yields a trailing "" from git's final newline, and the || l === "" clause matched it while path still pointed at the last file, adding a phantom anchor one line past the last hunk. Since blank diff lines are " " or "+" (never ""), that clause only ever caught the trailing artifact. A finding landing on the phantom line would pass the anchor check in main() and 422 the entire inline review batch, silently demoting every real inline finding to the summary. Dropped the || l === "" clause; added a test showing a 3-line added file now anchors {1,2,3} instead of {1,2,3,4}.
rightSideLines() treated `l === ""` as an anchorable right-side line.
diff.split("\n") always yields a trailing "" from git's final newline,
and that clause matched it while `path` still pointed at the last file —
adding a phantom anchor one line past the last hunk. Blank lines in a
unified diff are " " or "+", never "", so the clause only ever caught
that artifact. A model finding landing on the phantom line would pass the
anchor check and 422 the whole inline review batch, silently demoting all
real inline findings to the summary. Drop the `|| l === ""` clause.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // Parse the unified diff into path -> Set of RIGHT-side line numbers that a | ||
| // PR review comment can legally anchor to (added + context lines in hunks). | ||
| function rightSideLines(diff) { | ||
| const anchors = new Map(); |
There was a problem hiding this comment.
🟡 warning: The rightSideLines function skips lines that are blank in the diff (i.e., lines that are just a single space ' ' or '+'). However, a blank line in the file appears as a single space ' ' or '+' in the diff, which is correctly handled. But the function also skips lines that start with '+++ /dev/null' and sets path to null, which is correct. The issue is that the function does not handle the case where a hunk header has no context lines (e.g., @@ -0,0 +1 @@). In that case, the line counter is set to the first line of the hunk, but if the hunk only contains added lines, the first line is still anchorable. This is fine. However, the function does not handle the case where a line is both added and context (i.e., a line that starts with ' ' is context, but it is still anchorable). The function correctly adds lines starting with ' ' to the anchors set. So no bug here. Actually, the issue is that the function does not handle the case where a line is a trailing newline (empty string after splitting). The comment says 'the only "" we ever see here is the trailing element from splitting the diff's final newline' and skips it. But if the diff ends with a newline, the split will produce an empty string at the end, which is correctly skipped. However, if the diff has a blank line in the middle (i.e., a line that is just a newline), the split will produce an empty string for that line as well, and it will be skipped, meaning that blank lines in the file are not anchorable. This is a correctness issue: blank lines in the file are valid anchor points for review comments, but they are being skipped. This could cause findings that target a blank line to be incorrectly classified as unanchored.
|
|
||
| async function upsertSticky(body) { | ||
| const comments = await gh( | ||
| "GET", |
There was a problem hiding this comment.
🟡 warning: The postInline function deduplicates inline comments by comparing the body string, which includes the severity icon and the marker. However, if the model generates the same finding text for the same path and line but with a different severity (e.g., one push says 'warning' and the next says 'critical'), the deduplication will not catch it because the body string differs. This could lead to duplicate inline comments on the same line across pushes. Consider using a more robust deduplication key that includes path, line, and a hash of the body without the severity icon.
| if (findings.length === 0) return true; | ||
| try { | ||
| // Don't re-post an identical inline comment on a later push. | ||
| const existing = await gh( |
There was a problem hiding this comment.
🟡 warning: The postInline function sends all inline comments in a single review POST request. If any single comment fails validation (e.g., line number out of range, path not in diff), the entire request will fail with a 422 error, and all inline comments will be demoted to the summary. This is a design choice, but it means that a single bad finding can prevent all inline comments from being posted. Consider posting comments individually or in smaller batches to isolate failures.
| @@ -0,0 +1,359 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
🔵 suggestion: The script uses #!/usr/bin/env node but is invoked via node .github/scripts/pr-review.mjs in the workflow. The shebang is unnecessary and may cause confusion if the script is executed directly. Consider removing the shebang or making it consistent with the invocation.
| node-version: 20 | ||
|
|
||
| - name: AI code review (OpenRouter) | ||
| continue-on-error: true |
There was a problem hiding this comment.
🔵 suggestion: The OPENROUTER_API_KEY secret is passed as an environment variable to the script. While this is standard practice, the script does not mask the key in its output. If the script logs the key (e.g., in error messages), it could be leaked in workflow logs. Consider adding a step to mask the secret or ensure the script never logs it.
2 participants
What
Replaces the placeholder PR-review workflow — which unconditionally posted a canned "🤖 AI Code Review Complete" comment on every PR (pure noise) — with a real AI reviewer powered by OpenRouter.
Design
.github/scripts/pr-review.mjs(Node 20, zero npm deps — globalfetchonly):base...HEADrestricted to the same exclude list as before (lockfiles,dist/,build/,vendor/,*.sum, CHANGELOG, minified JS). Total diff hard-capped at ~80k chars; truncation is noted in the summary.deepseek/deepseek-v4-flash(temperature 0.1); falls back toqwen/qwen3.7-pluson any error/429.comments[]withside: RIGHT) only where the finding anchors to a line actually present in the diff (parsed from hunk headers); unanchorable findings fall back into the summary.<!-- pr-review-bot -->) — updated in place on each push, never stacked. Shows model used, finding counts by severity, truncation note, and actual cost from OpenRouter'susage.cost.continue-on-error, and the script itself always exits 0 — a reviewer failure is reported in the sticky comment instead of failing the check.Workflow changes: per-PR
concurrencygroup withcancel-in-progress(rapid pushes cancel stale reviews), dependabot PRs skipped entirely,setup-node@v4(node 20),fetch-depth: 0. The old canned-comment step is gone.Cost
Verified live with a small diff: deepseek-v4-flash came back at $0.000085 per review (~503 tokens). Even a maxed-out 80k-char diff lands around a tenth of a cent; the qwen fallback is ~5x that. Effectively free.
Setup
OPENROUTER_API_KEYrepo secret is already set.🤖 Generated with Claude Code