[negz]

Description of your changes

Each of us points our own agent at a PR, with our own harness and skills. None of us has special skills for finding correctness bugs, so a shared agent could find them on every PR for all of us. On style and taste, each agent gives feedback from its owner's conventions, so moving those skills into the repo would let us agree on them. This doesn't replace maintainer review. The aim is that we stop spending review on what the agent already caught, and spend it on what only we can judge.

This PR adds an agent that reviews PRs automatically. It runs when an org member opens a PR or marks one ready for review. Anyone with write access can comment /review to review any PR, or to re-review after new pushes. A re-review only covers what git range-diff shows changed since the last review.

The review agent runs four reviewers: design, prose, craft and correctness. It drops any finding that fails the bar set in the reviewing-pull-requests skill, then posts the rest as a single comment review from a GitHub App. The bot never approves or requests changes.

The skills live in .agents/skills: some I've adapted from my own, and some are distilled from the reviews we've posted on this repo. I'd prefer to treat them as a starting point we can iterate on, rather than spending too much time honing them in this PR. i.e. I'd prefer us to align on the mechanism in this PR, and hone the skills in follow up PRs.

The agent runs the PR's code, and every PR comes from a fork, so it runs in a Firecracker microVM that has no credentials that could do harm. design/pr-review-agent.md explains why I think that's secure enough. The review I'd most like is on that section.

I've run it end to end on my fork:

• Install each device stack only where its driver is claimed, adding CPU pools and AMD serving negz/modelplane#13 replays Install each device stack only where its driver is claimed, adding CPU pools and AMD serving #556.
• Propose KV transfer formats for prefill/decode serving negz/modelplane#14 replays Propose user-authored routing for prefill/decode serving #514 at the commit I first reviewed.
• Install each device stack only where its driver is claimed, adding CPU pools and AMD serving negz/modelplane#9, an earlier run on Install each device stack only where its driver is claimed, adding CPU pools and AMD serving #556, shows a re-review after a force-push.

The replays' base branches stop the agent reading the upstream PR it replays, so it can't see how the real review went. The skills and agents are the ones in this PR.

To make it work after this merges:

1. Create a GitHub App owned by modelplaneai, with no webhook. It needs read and write on pull requests, read on contents, and read on organization members. Install it on this repo.
2. On this repo, set the variable REVIEW_APP_ID and the secret REVIEW_APP_KEY from the App.
3. Create a model API key of its own, with a spend limit. Set it as the secret REVIEW_MODEL_API_KEY. The review uses Anthropic's API and Claude Opus 5.5 by default. To use another endpoint, set the variables REVIEW_MODEL_API (anthropic or openai), REVIEW_MODEL_URL and REVIEW_MODEL.

Until REVIEW_APP_ID is set, none of the review jobs run.

I have:

• Read and followed Modelplane's contribution process.
• Run nix flake check (or ./nix.sh flake check) and made sure it passes.
• Added or updated tests covering any composition function changes.
• Signed off every commit with git commit -s.

[Copilot]

🟡 Changes recommended

Archive scanning can exhaust the post job’s memory or CPU before limits apply, and failed agent runs can be marked as reviewed, causing unfinished work to be skipped.

5 open issues

• Bound archive scanning before buffering members · New
• Reject failed agent runs before marking them reviewed · New
• Update the skill to match the current runtime contract · New
• Align test guidance with the required pytest style · New
• Use wire-form dictionaries for composed and observed resources · New

What changed in this PR

This PR adds shared, automated pull request reviews to Modelplane’s CI, supplementing maintainer review with repository-owned agents and review guidance.

Changes:

• Adds automatic reviews for organization members, a permission-gated /review command, and retries for unavailable KVM.
• Runs reviewers in a Firecracker VM and validates their output before posting a comment-only review, with incremental re-review support.
• Adds review skills, specialized agent prompts, and Vale checks for their prose.

File	Description
nix/​docs.nix	Exposes shared Vale styles.
nix/​checks.nix	Adds linting for skills and agent prompts.
hack/​review-vm/​flake.nix	Defines the isolated review VM and agent runtime.
hack/​review-vm/​flake.lock	Pins VM dependencies.
hack/​post_review.py	Validates, scopes, and posts review output.
design/​pr-review-agent.md	Describes the review flow and security model.
.opencode/​vale/​.vale.ini	Configures review-prose linting.
.opencode/​vale/​.gitignore	Ignores downloaded Vale styles.
.opencode/​agents/​review.md	Coordinates reviewers and reconciles findings.
.opencode/​agents/​review-prose.md	Defines prose review responsibilities.
.opencode/​agents/​review-design.md	Defines design review responsibilities.
.opencode/​agents/​review-craft.md	Defines code-craft review responsibilities.
.opencode/​agents/​review-correctness.md	Defines correctness verification responsibilities.
.github/​workflows/​review.yml	Orchestrates authorization, VM execution, and posting.
.github/​workflows/​review-retry.yml	Retries runs that fail during KVM setup.
.github/​workflows/​commands.yml	Adds permission-gated /review handling.
.agents/​skills/​writing-style/​SKILL.md	Adds project writing guidance.
.agents/​skills/​writing-style/​references/​voice.md	Provides voice and structure examples.
.agents/​skills/​writing-style/​references/​de-slop-review.md	Defines a focused prose-editing pass.
.agents/​skills/​reviewing-pull-requests/​SKILL.md	Sets finding standards and feedback conventions.
.agents/​skills/​reviewing-pull-requests/​references/​calibration.md	Provides examples for judging findings.
.agents/​skills/​reviewing-designs/​SKILL.md	Defines design assessment criteria.
.agents/​skills/​reviewing-designs/​references/​calibration.md	Provides design-review examples.
.agents/​skills/​python-code-factoring/​SKILL.md	Adds Python structure and testing guidance.
.agents/​skills/​python-code-factoring/​references/​example.md	Illustrates Python factoring patterns.
.agents/​skills/​nix-tools/​SKILL.md	Explains running CLI tools through Nix.
.agents/​skills/​nix-flake/​SKILL.md	Explains flake-based development commands.
.agents/​skills/​krm-api-design/​SKILL.md	Adds Kubernetes API design guidance.
.agents/​skills/​design-docs/​SKILL.md	Sets design-document standards.
.agents/​skills/​crossplane-python-functions/​SKILL.md	Adds composition-function guidance.
.agents/​skills/​crossplane-python-functions/​references/​example.py	Supplies a composition-function reference example.
.agents/​skills/​before-writing-code/​SKILL.md	Adds shared engineering and testing principles.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The posting path can publish partial reviews after agent failure and process unbounded compressed archives, risking skipped unfinished work and resource exhaustion in the privileged runner.

6 open issues

• Require a successful agent exit before posting a review · New
• Bound archive scanning before buffering members
• Reject failed agent runs before marking them reviewed
• Use wire-form dictionaries for composed and observed resources
• Align test guidance with the required pytest style
• Update the skill to match the current runtime contract

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The review poster does not validate successful agent completion, allowing partial reviews from failed runs to be published with commit markers that cause later re-reviews to skip unfinished work.

7 open issues

• Reject failed agent runs before posting review markers · New
• Require a successful agent exit before posting a review
• Bound archive scanning before buffering members
• Reject failed agent runs before marking them reviewed
• Use wire-form dictionaries for composed and observed resources
• Align test guidance with the required pytest style
• Update the skill to match the current runtime contract

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The parser ignores missing or nonzero agent exit status, allowing unfinished reviews to be marked complete, and reviewer skills conflict with the documented function and testing conventions.

9 open issues

• Reject reviews when the agent exit status is missing or unsuccessful · New
• Reject failed agent runs before posting review markers
• Require a successful agent exit before posting a review
• Bound archive scanning before buffering members
• Align test guidance with the project's pytest convention · New
• Reject failed agent runs before marking them reviewed
• Use wire-form dictionaries for composed and observed resources
• Align test guidance with the required pytest style
• Update the skill to match the current runtime contract

_{Review effort: Balanced}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[haarchri]

lets try it out and iterate over it - please check my comments around how some files gets created - do we need this information inside?!

[dennis-upbound]

The agent runs in a microVM, the key stays in a runner proxy, and a model-free job posts the review. That's how I'd want pull_request_target to handle untrusted code. I checked that the review job's token is read-only (contents/pull-requests/issues: read) with a 60-minute timeout.

1. A budget per review (a token or turn cap in the opencode config) as well as the workspace limit, so one runaway or malicious PR can't use up the month and stop reviews for everyone.
2. A small eval before we tune the skills, since this PR leaves that to follow-ups. Without a baseline, a follow-up that "improves" the skills can't show it did. You've already replayed #556 and #514 by hand, so this is mostly writing that down. Fine as a follow-up, but I'd like it landed before the first skill-tuning PR. Here's a version in Inspect AI:

Dataset: evals/review/cases.jsonl, 10 to 20 merged PRs. Most should be PRs where our review found something real, plus 3 or 4 clean ones that shouldn't draw findings. Each case pins the commit the first review saw, and lists what maintainers raised, taken from our review comments:

{"id": "pr-556", "pr": 556, "head": "<sha the first review saw>",
 "findings": [
   {"kind": "correctness", "file": "functions/compose-model-replica/function/fn.py",
    "what": "a single-copy engine deadlocks its rollout when no GPU is idle"},
   {"kind": "prose", "file": "docs/...", "what": "..."}]}

The eval in evals/review/review.py. The solver runs the same agent the workflow runs, and the scorers compare its findings with ours:

import asyncio, json
from inspect_ai import Task, task
from inspect_ai.dataset import Sample
from inspect_ai.model import get_model
from inspect_ai.scorer import Score, mean, scorer
from inspect_ai.solver import solver

JUDGE = "anthropic/claude-sonnet-5-5"  # cheaper than the reviewer; check it against hand labels once

def cases():
    for line in open("evals/review/cases.jsonl"):
        c = json.loads(line)
        yield Sample(id=c["id"], input=f"Review PR #{c['pr']} at {c['head']}.",
                     target=json.dumps(c["findings"]), metadata=c)

@solver
def run_review():
    async def solve(state, generate):
        # the workflow's agent and skills, from main, against the pinned head; findings as JSON
        p = await asyncio.create_subprocess_exec(
            "opencode", "run", "--agent", "review", "--format", "json", state.input_text,
            stdout=asyncio.subprocess.PIPE)
        out, _ = await p.communicate()
        state.output.completion = out.decode()
        return state
    return solve

async def matches(found: list, expected: list) -> list[bool]:
    """For each expected finding, does any agent finding raise the same problem? One judge call per case."""
    prompt = ("For each EXPECTED finding, answer true if any FOUND finding raises the same problem "
              "(same file or change, same defect), else false. Reply with a JSON list of booleans only.\n"
              f"EXPECTED: {json.dumps(expected)}\nFOUND: {json.dumps(found)}")
    return json.loads((await get_model(JUDGE).generate(prompt)).completion)

@scorer(metrics=[mean()])
def recall():
    async def score(state, target):
        expected, found = json.loads(target.text), json.loads(state.output.completion).get("findings", [])
        if not expected:  # a clean PR: score 1 when the agent raised nothing
            return Score(value=float(not found), explanation=f"{len(found)} findings on a clean PR")
        hit = await matches(found, expected)
        return Score(value=sum(hit) / len(expected), explanation=f"{sum(hit)}/{len(expected)} found")
    return score

@scorer(metrics=[mean()])
def grounded():
    """Deterministic: every finding names a file the PR touched. Catches invented findings without a judge."""
    async def score(state, target):
        found = json.loads(state.output.completion).get("findings", [])
        touched = set(state.metadata.get("files", []))  # fill from `gh pr diff --name-only` when building cases
        ok = [f for f in found if not touched or f.get("file") in touched]
        return Score(value=len(ok) / len(found) if found else 1.0)
    return score

@task
def review():
    return Task(dataset=list(cases()), solver=run_review(), scorer=[recall(), grounded()])

Run it with inspect eval evals/review/review.py, and compare runs with inspect view. Each skill change should report recall of what we raised, grounded, and the finding count on the clean PRs. A follow-up can add a precision scorer from our 👍/👎 on bot comments. Those reactions are the cheapest label source we'll get, so I'd turn them on from day one.

Before trusting the numbers:

• Check the judge once: label 2 or 3 cases by hand and confirm the judge agrees, before trusting its numbers.
• Keep a few cases out of tuning (a held-out set), so we don't fit the skills to the cases we look at.