Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions data/reviews.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
[
{
"repo": "Alamofire/Alamofire",
"pr": 4038,
"title": "Fix for DataPreprocessor doesn't have it's own failure reason #3073",
"url": "https://github.com/Alamofire/Alamofire/pull/4038",
"reviewed_at": "2026-06-08T02:38:00.000Z",
"mode": "agent",
"findings": {
"high": 0,
"medium": 1,
"low": 0
},
"merged": false,
"pr_state": "open"
}
]
11 changes: 7 additions & 4 deletions examples/sample.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
'use strict';

// Example: user lookup with a few subtle bugs for the AI reviewer to catch.

async function getUser(db, userId) {
const row = await db.query('SELECT * FROM users WHERE id = ?', [userId]);
// Bug 1: no null check — row could be undefined if user doesn't exist
if (!row) return null;
return { name: row.name, email: row.email };
}

Expand All @@ -18,7 +16,12 @@ async function processUsers(db, userIds) {
}

async function deleteUser(db, userId, requestingUserId) {
// Bug 2: no authorization check — any caller can delete any user
if (!requestingUserId) throw new Error('requestingUserId is required');
const requester = await getUser(db, requestingUserId);
if (!requester) throw new Error('Requesting user not found');
if (requestingUserId !== userId && !requester.isAdmin) {
throw new Error('Unauthorized: only admins can delete other users');
}
await db.query('DELETE FROM users WHERE id = ?', [userId]);
return { deleted: userId };
}
Expand Down
50 changes: 50 additions & 0 deletions scripts/log-review.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';
// Usage: node log-review.js <owner/repo> <pr-number> <mode>
// Fetches PR metadata + latest Preflight review from GitHub, appends to data/reviews.json.

const { execSync } = require('child_process');
const fs = require('fs');
const path = require('path');

const [repo, pr, mode] = process.argv.slice(2);
if (!repo || !pr || !mode) {
console.error('Usage: log-review.js owner/repo pr-number mode');
process.exit(1);
}

function gh(endpoint) {
return JSON.parse(execSync(`gh api ${endpoint}`, { encoding: 'utf-8' }));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Shell injection via unsanitized repo and pr arguments in gh helper

The gh function builds a shell command by string-interpolating endpoint directly: execSync(gh api ${endpoint}). The values repo and pr come from process.argv without any sanitisation. A malicious caller could pass repo = 'x/y --method DELETE' or a PR number containing shell metacharacters, causing arbitrary gh API calls or command execution. The arguments should be passed as an array with execSync('gh', ['api', endpoint], …) or at minimum validated to match expected patterns before use.

Category: security issues

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Command injection via unsanitized repo and pr arguments

The gh function builds a shell command by directly interpolating repo and pr into a template string passed to execSync. Since repo and pr come from process.argv without any validation or escaping, a malicious value such as owner/repo; rm -rf / would be executed as a shell command. execSync uses /bin/sh by default when given a string, so shell metacharacters are interpreted. The values should be validated (e.g. /^[\w.-]+\/[\w.-]+$/ for repo and /^\d+$/ for pr) before use, or the command should be passed as an array with execFileSync to avoid shell interpretation.

Category: security issues

}

const prData = gh(`repos/${repo}/pulls/${pr}`);
const reviews = gh(`repos/${repo}/pulls/${pr}/reviews`);

const review = [...reviews].reverse().find(r => r.body && r.body.includes('Preflight'))
|| [...reviews].reverse().find(r => r.body);

let high = 0, medium = 0, low = 0;
if (review) {
const body = review.body;
const hm = body.match(/(\d+) high/i); if (hm) high = parseInt(hm[1], 10);
const mm = body.match(/(\d+) medium/i); if (mm) medium = parseInt(mm[1], 10);
const lm = body.match(/(\d+) low/i); if (lm) low = parseInt(lm[1], 10);
}

const entry = {
repo,
pr: parseInt(pr, 10),
title: prData.title,
url: prData.html_url,
reviewed_at: new Date().toISOString(),
mode,
findings: { high, medium, low },
merged: prData.merged,
pr_state: prData.state,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] data/reviews.json read failure crashes the process with no error handling

On line 44, fs.readFileSync(dataFile, 'utf-8') and JSON.parse(...) are called without any try/catch. If the file does not exist, is empty, or contains malformed JSON, the process throws an unhandled exception and the log entry is never written. There is also no guarantee the data/ directory exists. These calls should be wrapped in a try/catch, defaulting to an empty array [] when the file is missing or unparseable.

Category: edge cases

const dataFile = path.join(__dirname, '..', 'data', 'reviews.json');
const log = JSON.parse(fs.readFileSync(dataFile, 'utf-8'));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] readFileSync throws if data/reviews.json does not exist; no error handling

Line 46 calls fs.readFileSync(dataFile, 'utf-8') with no try/catch. If data/reviews.json is missing (e.g. first run, CI clean checkout), the process crashes with an unhandled exception and the review is never logged. The file should be initialised to [] when absent, or the read should be wrapped in a try/catch that falls back to an empty array.

Category: edge cases

log.unshift(entry);
fs.writeFileSync(dataFile, JSON.stringify(log, null, 2) + '\n');

console.log(`[preflight] Logged — ${high} high, ${medium} medium, ${low} low → ${prData.html_url}`);
88 changes: 88 additions & 0 deletions scripts/preflight-agent.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
#!/usr/bin/env bash
# Preflight — agent mode
# Usage: ./scripts/preflight-agent.sh <owner/repo> <pr-number>
#
# Runs Claude Code as the reviewer directly — no Anthropic API key needed.
# Requires: claude CLI (Claude Code), gh CLI authenticated with repo access.
# Works with public repos, private repos, and GitHub Enterprise (gh handles auth).
set -euo pipefail

REPO="${1:?Usage: preflight-agent.sh owner/repo pr-number}"
PR="${2:?Usage: preflight-agent.sh owner/repo pr-number}"

if ! command -v claude &>/dev/null; then
echo "Error: claude CLI not found. Install Claude Code: https://claude.ai/code"
exit 1
fi

if ! command -v gh &>/dev/null; then
echo "Error: gh CLI not found. Install: https://cli.github.com"
exit 1
fi

if ! gh api "repos/$REPO" --silent 2>/dev/null; then
echo "Error: gh cannot access $REPO. Run 'gh auth login' and ensure you have repo access."

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Unquoted $REPO interpolated into gh api URL allows argument injection

Line 24 uses gh api "repos/$REPO" where $REPO is taken directly from the first positional argument without validation. A value such as x/y --method DELETE would inject extra flags into the gh invocation. The variable should be validated to match the pattern [A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+ before use, and/or the gh api call should use -- to separate options from the path.

Category: security issues

exit 1
fi

echo "[preflight] Starting agent review of $REPO#$PR ..."

claude -p "You are Preflight, an automated code reviewer. Review GitHub PR $REPO#$PR and post your findings as inline comments.

## Steps

1. Fetch the list of changed files:
gh api repos/$REPO/pulls/$PR/files --paginate

2. Filter out files that should be skipped (do not review these):
- Paths containing: vendor/, node_modules/
- Filenames matching: *.generated.*, package-lock.json, go.sum, *.lock, *.pb.go, *_generated.go
- Files where (additions + deletions) > 500
- After filtering, take only the top 10 files ranked by (additions + deletions) descending

3. For each file to review, fetch its full content using the blob sha from step 1:
gh api repos/$REPO/git/blobs/{sha} --jq '.content' | base64 -d

4. Study the diff patch for each file. Note exactly which line numbers in the new file were added or changed (lines starting with '+' in the patch, tracking the @@ hunk headers).

5. Review only the changed lines for real bugs — not style, not formatting. Categories:
- null/nil dereference
- unhandled errors / ignored return values
- race conditions
- resource leaks (unclosed files, connections, goroutines)
- security issues (injection, path traversal, unvalidated input, auth bypass)
- logic errors / off-by-one
- type mismatches

6. Post a single PR review via:
gh api repos/$REPO/pulls/$PR/reviews --method POST --input -

Build the JSON payload with:
- commit_id: the PR head SHA (fetch from gh api repos/$REPO/pulls/$PR --jq '.head.sha')
- event: COMMENT
- body: MUST start with exactly '## Preflight Review', followed by a blank line,
then 2-3 sentences summarizing what the PR does, then a blank line,
then exactly one of these findings lines:
'**Findings:** N high, N medium, N low' (if bugs found)
'**No bugs found** in the reviewed files.' (if none)
- comments: array of inline findings (high + medium severity only)
Each comment needs: path, line (new-file line number), side: RIGHT, body

Format each inline comment body as:
**[SEVERITY] Title**

Explanation citing exact symbol name and line.

_Category: category_

7. If no bugs are found, post the review with an empty comments array.

## Rules
- Only comment on lines present in the diff. Never flag unchanged lines.
- Cite exact line numbers, function names, and variable names.
- Consolidate everything into a single gh api call — do not post multiple reviews.
- If files were skipped due to filters, list them in the summary body." \
--allowedTools "Bash" \
--max-turns 30

node "$(dirname "$0")/log-review.js" "$REPO" "$PR" "agent"
28 changes: 28 additions & 0 deletions scripts/preflight.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#!/usr/bin/env bash
# Usage: ./scripts/preflight.sh <owner/repo> <pr-number>
# Runs a local dry-run review against any GitHub PR (public or private).
set -euo pipefail

REPO="${1:?Usage: preflight.sh owner/repo pr-number}"
PR="${2:?Usage: preflight.sh owner/repo pr-number}"

if [[ -z "${ANTHROPIC_API_KEY:-}" ]]; then
echo "Error: ANTHROPIC_API_KEY is not set."
echo "Export it first: export ANTHROPIC_API_KEY=sk-ant-..."
exit 1
fi

# Pull a token from gh CLI — works for both public and private repos
# as long as the user is authenticated with sufficient scope (repo).
GITHUB_TOKEN=$(gh auth token)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] gh auth token failure is not handled — token silently empty under set -e

With set -euo pipefail, if gh auth token fails (user not logged in, token expired) the script exits immediately at line 17 without printing a meaningful error message. GITHUB_TOKEN is never set. While set -e prevents the bad token from propagating, the user sees no actionable error. The call should be wrapped: GITHUB_TOKEN=$(gh auth token) || { echo 'Error: gh auth token failed. Run gh auth login.'; exit 1; }.

Category: unhandled errors


SHA=$(gh api repos/"$REPO"/pulls/"$PR" --jq '.head.sha')

GITHUB_TOKEN="$GITHUB_TOKEN" \
GITHUB_REPOSITORY="$REPO" \
PR_NUMBER="$PR" \
PR_HEAD_SHA="$SHA" \
DRY_RUN=true \
node "$(dirname "$0")/review.js"

node "$(dirname "$0")/log-review.js" "$REPO" "$PR" "api"
27 changes: 21 additions & 6 deletions scripts/review.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const GITHUB_API = 'https://api.github.com';
const MODEL = 'claude-sonnet-4-6';
const MAX_FILES = 10;
const MAX_LINES = 500; // skip files with more changed lines than this
const DRY_RUN = process.env.DRY_RUN === 'true';

// ── File filters ─────────────────────────────────────────────────────────────

Expand Down Expand Up @@ -241,12 +242,12 @@ async function main() {
}

if (skipped.length > 0) {
await postComment(
`## Preflight: Files Skipped\n\n` +
const msg = `Files skipped:\n${skipped.map(s => ` - ${s}`).join('\n')}`;
if (DRY_RUN) console.log(`[preflight] ${msg}`);
else await postComment(`## Preflight: Files Skipped\n\n` +
`The following files exceeded review limits and were not checked:\n\n` +
skipped.map(s => `- ${s}`).join('\n') +
`\n\n> Adjust \`.reviewbot.yaml\` to change thresholds.`
);
`\n\n> Adjust \`.reviewbot.yaml\` to change thresholds.`);
}

if (toReview.length === 0) {
Expand Down Expand Up @@ -301,8 +302,22 @@ async function main() {
}

// ── Step 7: post review ──────────────────────────────────────────────────
await postReview(findings, summaryBody);
console.log(`[preflight] Done — ${findings.length} finding(s)`);
if (DRY_RUN) {
console.log('\n' + summaryBody + '\n');
if (findings.length === 0) {
console.log('[preflight] No findings.');
} else {
findings.forEach(f => {
console.log(`\n[${f.severity.toUpperCase()}] ${f.file}:${f.line} — ${f.title}`);
console.log(f.body);
console.log(`Category: ${f.category}`);
});
}
console.log(`\n[preflight] Done — ${findings.length} finding(s) (dry run, nothing posted)`);
} else {
await postReview(findings, summaryBody);
console.log(`[preflight] Done — ${findings.length} finding(s)`);
}

} catch (err) {
// Fail silently: never block a PR due to a review bot error.
Expand Down
Loading