Skip to content
Draft
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
11 changes: 11 additions & 0 deletions .claude/skills/pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ If reviewers or the author linked to sources (docs, issues, code snippets, bench

Evaluate these first — they matter more than line-level nits:

- **Bloat check**: Does the value this PR adds justify the maintenance burden it introduces? Every new file, abstraction, and code path has an ongoing cost — someone has to understand it, update it, and debug it. Flag PRs that add significant complexity (new modules, abstractions, config options, CLI flags) without proportional value. Also flag: vendored files that should be dependencies, IDE/editor files (`.idea/`, `.vscode/`), or code that reimplements something already available in a dependency.
- **AI slop check**: Does the code read like unedited LLM output? Signs: excessive docstrings on every function (especially multi-paragraph ones restating the function signature), over-commented code explaining the obvious, cargo-culted error handling that catches exceptions only to re-raise them, unnecessary abstractions wrapping single calls, verbose variable names that read like natural language sentences, or boilerplate patterns copy-pasted without adaptation to the context. If you spot these patterns, flag it directly — ask the author to trim the noise.
- **Splittability**: Does the PR bundle independent changes that could be reviewed separately? Look for: unrelated bug fixes alongside a feature, refactors mixed with new functionality, changes to multiple subsystems with no dependency between them, or "while I was here" cleanup. If the PR contains independently reviewable units, ask the author to split it — smaller PRs get better reviews and merge faster. Only flag this when the pieces are truly independent; don't ask to split tightly coupled changes.
- **Motivation**: Is the PR description clear about *why* this change is needed? If the motivation is missing or vague, ask the author to clarify. A PR that doesn't explain the problem it solves is hard to review properly.
- **Purpose & scope**: Does the PR do what the description claims? Is the scope appropriate or should it be split?
- **Architectural fit**: Does this fit the existing patterns in `speculators`? Does it introduce unnecessary abstractions or bypass existing ones? Read surrounding code in the same module to understand local conventions (naming, error handling, structure) and flag deviations.
Expand Down Expand Up @@ -90,6 +93,14 @@ If the PR description, commit messages, or code comments reference a paper (arXi

Only flag mismatches you can concretely demonstrate by quoting both the paper and the code. Do not flag stylistic differences in how math is expressed if the computation is equivalent.

### Phase 3.7: CONTRIBUTING.md compliance

Check the PR against the project's contribution guidelines (`CONTRIBUTING.md`):

1. **Issue linkage for significant changes**: If the PR adds new training algorithms/model support, modifies the data pipeline or CLI/API, is a large refactor, or touches 3+ files — verify it references an assigned issue. Small fixes (typos, docs, bug fixes < 20 lines, type annotations, minor deps) are exempt.
2. **DCO sign-off**: All commits must have a `Signed-off-by` line. Check with `gh pr view -R vllm-project/speculators <number> --json commits --jq '.commits[].messageBody'` — if any commit lacks the sign-off, flag it (CI will also catch this, but noting it saves a round-trip).
3. **Documentation updates**: If the PR changes user-facing behavior (CLI flags, config options, APIs), check whether docs were updated. Missing doc updates for behavior changes should be flagged.

### Phase 4: Line-level review

Apply path-specific focus based on which files changed:
Expand Down
82 changes: 82 additions & 0 deletions .claude/skills/review-open-prs/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
# Review Open PRs

Scan all open pull requests on the speculators repo and run `/pr-review` on each one that needs attention. Designed for periodic cron execution.

## Step 1: Fetch reviewable PRs

Run:

```bash
gh pr list --repo vllm-project/speculators --state open --json number,title,isDraft,reviewDecision,updatedAt,url --limit 100
```

Filter for PRs that meet **ALL** criteria:

1. **Not a draft** — `isDraft` is `false`
2. **Not already approved** — `reviewDecision` is not `"APPROVED"`
3. **Recent activity** — `updatedAt` is within the last 30 days

If no PRs match, report "No reviewable PRs found." and stop.

## Step 2: Skip already-reviewed PRs

For each candidate PR, check whether you have already reviewed it since its last push:

```bash
gh api repos/vllm-project/speculators/pulls/<number>/reviews --jq '[.[] | select(.user.login == "orestis-z")] | last | .submitted_at'
```

Also get the last push timestamp:

```bash
gh pr view <number> --repo vllm-project/speculators --json commits --jq '.commits[-1].committedDate'
```

**Skip** the PR if your last review is more recent than the last push — there is nothing new to review. If you have never reviewed the PR, it qualifies.

## Step 3: Review each PR

For each qualifying PR, spawn an Agent (subagent) to run the review. Launch **all agents in a single message** so they run in parallel:

```
Agent({
description: "Review PR #<number>",
prompt: "Run /pr-review <number> on the vllm-project/speculators repo. Follow the skill instructions fully — gather context, review, and post."
})
```

Send all Agent tool calls in one response to maximize parallelism. Each agent independently fetches context, reviews, and posts — they don't share state, so parallel execution is safe.

If an agent fails or errors out, note the error in the summary and continue.

## Step 4: Summary

After all PRs are processed, present a summary:

```
## PR Review Sweep Complete

- PRs scanned: <total open>
- PRs filtered out: <count> (draft: N, approved: N, stale: N, already reviewed: N)
- PRs reviewed: <count>

### Reviewed
| PR | Title | Result |
|----|-------|--------|
| #<number> | <title> | <findings count> findings |

### Skipped (already reviewed, no new commits)
| PR | Title | Last reviewed |
|----|-------|---------------|
| #<number> | <title> | <date> |
```

## Cron Integration

This skill is designed to be called by a durable cron job. Recommended schedule:

```
7 * * * * (hourly at :07)
```

The skip-already-reviewed logic in Step 2 ensures idempotent runs — re-running within the same hour is harmless.
70 changes: 70 additions & 0 deletions scripts/review-open-prs.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#!/bin/bash
# Review open PRs: fetch non-draft, unapproved PRs and post reviews.
#
# Usage:
# ./scripts/review-open-prs.sh # headless (default)
# ./scripts/review-open-prs.sh --interactive # live TUI (debugging)
#
# Run on a schedule (system cron):
# 0 * * * * /workspace/speculators/scripts/review-open-prs.sh >> /workspace/speculators/pr-review.log 2>&1

set -euo pipefail

export HOME=/root
export PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:$PATH"

# Claude authenticates via Vertex AI -- cron doesn't inherit these
export CLAUDE_CODE_USE_VERTEX="${CLAUDE_CODE_USE_VERTEX:-1}"
export ANTHROPIC_VERTEX_PROJECT_ID="${ANTHROPIC_VERTEX_PROJECT_ID:-itpc-gcp-ai-eng-claude}"

export REPO_DIR="${REPO_DIR:-$(cd "$(dirname "$0")/.." && pwd)}"
cd "$REPO_DIR"

# Re-exec from /tmp so git-reset doesn't delete the running script.
if [ "$(realpath "$0")" != "/tmp/review-open-prs-running.sh" ]; then
cp "$(realpath "$0")" /tmp/review-open-prs-running.sh
exec /tmp/review-open-prs-running.sh "$@"
fi

# Git sync and user switch -- only as root (claude-runner re-enters below).
if [ "$(id -u)" -eq 0 ]; then
git fetch origin
git reset --hard origin/main
# Pre-merge fallback: restore skill from feature branch if not yet on main
if [ ! -f .claude/skills/review-open-prs/SKILL.md ] || [ ! -f scripts/review-open-prs.sh ]; then
git checkout origin/feat/pr-review-cron-v2 -- .claude/skills/review-open-prs/ scripts/review-open-prs.sh 2>/dev/null || true
fi

# Claude blocks --dangerously-skip-permissions for root -- re-exec as claude-runner.
WS_GID=$(stat -c '%g' /workspace)
chmod -R g+rwX .claude 2>/dev/null || true
chmod g+r /root/.claude/.credentials.json 2>/dev/null || true
chown root:"$WS_GID" /root/.claude/.credentials.json 2>/dev/null || true
chmod g+r /root/.config/gcloud/credentials.db /root/.config/gcloud/application_default_credentials.json 2>/dev/null || true
chown root:"$WS_GID" /root/.config/gcloud/credentials.db /root/.config/gcloud/application_default_credentials.json 2>/dev/null || true
chmod -R g+rwX /root/.config/gcloud/logs 2>/dev/null || true
exec runuser -u claude-runner -- "$0" "$@"
fi

INTERACTIVE=false
while [ $# -gt 0 ]; do
case "$1" in
--interactive) INTERACTIVE=true; shift ;;
*) echo "Unknown option: $1"; exit 1 ;;
esac
done

EXTRA_ARGS=()
[ -n "${MAX_TURNS:-}" ] && EXTRA_ARGS+=(--max-turns "$MAX_TURNS")

echo "=== PR Review Sweep: $(date -Iseconds) ==="
echo "Running as: $(whoami)"
echo "Mode: $([ "$INTERACTIVE" = true ] && echo "interactive (live TUI)" || echo "headless")"
[ -n "${MAX_TURNS:-}" ] && echo "Max turns: $MAX_TURNS" || echo "Max turns: unlimited"
echo "======================================="

if [ "$INTERACTIVE" = true ]; then
echo "/review-open-prs" | claude --model opus --dangerously-skip-permissions "${EXTRA_ARGS[@]}"
else
claude -p "/review-open-prs" --model opus --dangerously-skip-permissions "${EXTRA_ARGS[@]}"
fi