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
6 changes: 5 additions & 1 deletion docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,11 @@ its tools are the host read bundle and repository reads confined to that
folder. The consolidate step reads each finding's lines at the head and reports
the findings it could show the model no line of; a comment whose group had no
member read carries `unverified`, which the portal and the review sent to
GitHub show as "Not checked against the file".
GitHub show as "Not checked against the file". The check step reads every
consolidated group in one more model call each: the candidate, its quoted code
and the file lines around it, answered real or not real with a reason. Only
groups it finds real are written up and posted; the others stay in the
reported result's `dropped`, each with the verdict and reason.

The portal's Code review page edits a repository's policy file without its
text: it reads the file through the policy and policy-file reads, keeps a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Nothing runs until you turn it on. Connecting GitHub or Bitbucket Data Center on

### Code review

Code review reads each new commit of a pull request for bugs: mistakes in the changed lines and breakage the change causes elsewhere in the repository. It is separate from the verdict and optional.
Code review reads each new commit of a pull request for bugs: mistakes in the changed lines and breakage the change causes elsewhere in the repository. A finding is posted only after a second read of the lines it points at confirms it. It is separate from the verdict and optional.

- **Turning it on.** A repository's [policy file](#writing-the-file) turns it on: it then runs after triage, for the groups in `for` and on the verdicts and paths in `when`. A pull request they leave out, or one whose failed rules hold the review back, is not reviewed, and its **Code review** section says **Not reviewed:** and why, such as **Not reviewed: the author is not in the maintainers group**. It needs the workspace's Anthropic API key, saved on the [Code review page](#code-review-provider-and-places).
- **What it costs.** Each commit it reviews is billed to your Anthropic account. It reviews a pull request when it is opened and after every push while it is not a draft, and **Review again**, offered once a pull request has a review, is one more review. A push that leaves the change exactly as it was reviewed, such as a rebase onto a newer base that changed none of the files it touches, is not reviewed again: the new commit keeps the last review, and the section says **Carried over from** the commit it was made for. **Review again** reviews it anyway.
Expand Down
13 changes: 8 additions & 5 deletions packages/pr-code-review-anthropic/evals/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,13 @@ A clean case becomes a defect case only with a confirmed added bug.
A bug is caught when a finding on its path overlaps any of its sites within
three lines. A finding that hits no bug is a false alarm, so a complete key
makes false alarms meaningful. A clean case is flagged when anything is
reported on it. `--e2e` scores the finders' union, the consolidated findings,
the confirmed ones (all of them unless `--check`), the final review, and the
review's comments found by both finders and by one.
reported on it. `--e2e` checks every finding, as every review does, and
scores the finders' union, the consolidated findings (what the review posted
before the check ran on every request), the ones the check confirmed, the
final review, and the review's comments found by both finders and by one. A
stage's precision is its findings that hit a bug over all its findings, so the
consolidated and final rows give precision and recall without the check and
with it from one run.

## Running

Expand All @@ -64,8 +68,7 @@ bun run eval:review -- --e2e --out e2e.json
Each step runs on the model it is deployed with: Opus 5.5 for the diff
finder, Haiku 5.5 for the rest. `--<role>-model` overrides one.

`--cases a,b` runs a subset. `--check` turns the check step on, with
`--check-model` and `--check-effort` for it. Other modes, listed at the top of
`--cases a,b` runs a subset. Other modes, listed at the top of
`review.eval.ts`: a finder alone with any prompt (`--prompt`) and rescoring
saved finder runs (`--rescore`).

Expand Down
21 changes: 11 additions & 10 deletions packages/pr-code-review-anthropic/evals/review.eval.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@
// bun run eval:review -- --rescore <results-dir> re-scores saved results without model calls
// bun run eval:review -- --rescore <e2e.json> [--out <file.json>] sweeps posting cutoffs over a saved --e2e run without
// model calls (sweep.ts) and reports the loosest one at 85% precision and the precision/recall frontier.
// bun run eval:review -- --e2e [--cases a,b] [--<role>-model m] [--<role>-effort e] [--budget n] [--check] [--out <file.json>] [--baseline]
// bun run eval:review -- --e2e [--cases a,b] [--<role>-model m] [--<role>-effort e] [--budget n] [--out <file.json>] [--baseline]
// with <role> one of plan, diff-finder, repo-finder, consolidate, check, write; each role defaults to its deployed model;
// runs the whole pr-code-review workflow on each case's head and scores the finders' union, the consolidated findings,
// the confirmed ones (all of them unless --check), the final review, and its comments found by both finders and by
// one, with each step's tokens, dollars and seconds. --baseline, over every case, also writes
// runs the whole pr-code-review workflow on each case's head, checking every finding as every review does, and
// scores the finders' union, the consolidated findings (what the review posted before the check), the ones the check
// confirmed, the final review, and its comments found by both finders and by one, with each stage's precision and
// each step's tokens, dollars and seconds. --baseline, over every case, also writes
// results/pr-code-review.baseline.json, refusing on a dirty working tree or when any case's repository is not public.
//
// Inference is native Anthropic when ANTHROPIC_API_KEY is set, else OpenCode Zen with OPENCODE_API_KEY.
Expand Down Expand Up @@ -323,7 +324,6 @@ interface E2EConfig {
models: Partial<Record<Role, string>>;
efforts: Record<Role, Effort>;
budget?: number;
check: boolean;
}

function effortFlag(value: unknown, fallback: Effort): Effort {
Expand All @@ -347,7 +347,6 @@ function e2eConfig(values: Record<string, unknown>, provider: ProviderId): E2ECo
models: Object.fromEntries(ROLES.flatMap(model)),
efforts: Object.fromEntries(ROLES.map(effort)) as Record<Role, Effort>,
...(typeof values.budget === "string" && { budget: Number(values.budget) }),
check: values.check === true,
};
}

Expand Down Expand Up @@ -382,7 +381,7 @@ async function runE2ECase(testCase: Case, config: E2EConfig, source: Provider, g
step.ms += ms;
}
const head = headOf(testCase.id);
const request = { kind: "code-review", title: testCase.title, description: testCase.description, diff: testCase.diff, checkout: { source: "github", ...head }, efforts: config.efforts, check: config.check, ...(config.budget && { budgets: { repoFinder: config.budget } }) };
const request = { kind: "code-review", title: testCase.title, description: testCase.description, diff: testCase.diff, checkout: { source: "github", ...head }, efforts: config.efforts, ...(config.budget && { budgets: { repoFinder: config.budget } }) };
const started = performance.now();
const run = await runLocalWorkflow({
workflow: prCodeReview({ sources: GITHUB }),
Expand Down Expand Up @@ -486,12 +485,15 @@ function e2eStageRow(stage: Stage, results: readonly E2EResult[], keys: readonly
const ofReach = bugs.filter((bug) => bug.reach === reach);
return [reach, ratio(ofReach.filter(isCaught).length, ofReach.length)];
}
const findings = sum(results, (r) => r.stages[stage]?.findings.length ?? 0);
const falseAlarms = sum(results, (r) => r.stages[stage]?.falseAlarms ?? 0);
return {
stage,
bugsCaught: ratio(bugs.filter(isCaught).length, bugs.length),
...Object.fromEntries(REACHES.map(caughtOfReach)),
findings: sum(results, (r) => r.stages[stage]?.findings.length ?? 0),
falseAlarms: sum(results, (r) => r.stages[stage]?.falseAlarms ?? 0),
findings,
falseAlarms,
precision: percent(findings === 0 ? null : (findings - falseAlarms) / findings),
cleanFlagged: ratio(results.filter((r) => r.stages[stage]?.flaggedClean).length, scored.filter((key) => key.kind === "clean").length),
};
}
Expand Down Expand Up @@ -616,7 +618,6 @@ async function main() {
budget: { type: "string" },
e2e: { type: "boolean", default: false },
baseline: { type: "boolean", default: false },
check: { type: "boolean", default: false },
...Object.fromEntries(Object.values(ROLE_FLAGS).flatMap((flag) => [[`${flag}-model`, { type: "string" }], [`${flag}-effort`, { type: "string" }]])),
},
});
Expand Down
1 change: 0 additions & 1 deletion packages/pr-code-review-anthropic/evals/sweep.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ function saved(diff: object[], repo: object[], kept: Candidate[], groups: Group[
const consolidated = { groups, sites: kept.map(({ path, start, end }) => ({ path, start, end })), excerpts: kept.map(() => ""), unreadFindings: [] };
const findings = orderedGroups(kept, consolidated).map((o) => o.finding);
return {
reviewInput: { request: {} },
findDiff,
findRepo,
candidates: { found: [...foundOf(findDiff, "diff"), ...foundOf(findRepo, "repo")], kept },
Expand Down
4 changes: 2 additions & 2 deletions packages/pr-code-review-anthropic/evals/sweep.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export function posts(comment: Comment, cutoff: Cutoff): boolean {
}

export interface SavedOutputs {
reviewInput: { request: { check?: boolean } };
findDiff: { reply: string };
findRepo: { reply: string };
candidates: { found: Candidate[]; kept: Candidate[] };
Expand Down Expand Up @@ -95,7 +94,8 @@ function isReal(output: { reply: string }): boolean {
export function commentsOf(outputs: SavedOutputs): Comment[] {
const { kept } = outputs.candidates;
const ordered = orderedGroups(kept, JSON.parse(outputs.consolidate.reply) as Consolidated);
const reported = outputs.reviewInput.request.check ? ordered.filter((_, i) => outputs.check[i] !== undefined && isReal(outputs.check[i])) : ordered;
// A run saved before every finding was checked has no check outputs and reported every group.
const reported = outputs.check.length === 0 ? ordered : ordered.filter((_, i) => outputs.check[i] !== undefined && isReal(outputs.check[i]));
const confidences = keptConfidences(outputs);
function comment(posted: SavedOutputs["review"]["comments"][number], index: number): Comment {
const entry = reported[index];
Expand Down
37 changes: 36 additions & 1 deletion packages/pr-code-review-anthropic/src/pr-code-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ const SHA = "0123456789abcdef0123456789abcdef01234567";
const FILE = "export function add(a: number, b: number) {\n return a - b;\n}\n";
const DIFF = "diff --git a/src/a.ts b/src/a.ts\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1,3 +1,3 @@\n export function add(a: number, b: number) {\n- return a + b;\n+ return a - b;\n }\n";
const FINDING = { path: "src/a.ts", start: 2, end: 2, priority: 1, confidence: 0.9, title: "add subtracts", why: "it uses -", trigger: "add(1, 1)", code: " return a - b;" };
/** A candidate the check step answers not real. */
const PLANTED = { path: "src/a.ts", start: 1, end: 1, priority: 2, confidence: 0.6, title: "add is not exported", why: "callers cannot import it", trigger: "import { add }", code: "export function add(a: number, b: number) {" };

let dir: string;
let tarball: Uint8Array;
Expand All @@ -50,6 +52,10 @@ let repoFinderProse = false;
let planRefused = false;
/** How many of the write step's next answers are prose with no JSON. */
let writeProse = 0;
/** Whether both finders also report `PLANTED`. */
let planted = false;
/** How many of the check step's next answers name no verdict it knows. */
let checkUnsure = 0;

const allow: WorkflowAuthorizeFn = async function allow() {
return { effect: "allow", matchingGrants: [], resolvedBy: null };
Expand Down Expand Up @@ -89,8 +95,13 @@ function answer(body: MessagesRequest) {
return [{ type: "text", text: "One defect: add subtracts." }];
}
if (system.includes("<tools>") && repoFinderProse) return [{ type: "text", text: "I read src/a.ts and ran out of budget before writing findings." }];
let text: unknown = { findings: [FINDING] };
let text: unknown = { findings: planted ? [FINDING, PLANTED] : [FINDING] };
if (system.includes("<cited_code>")) text = { groups: [{ defect: "add subtracts", members: [0], representative: 0 }] };
if (sent.includes("Is this defect real?") && checkUnsure > 0) {
checkUnsure--;
return [{ type: "text", text: JSON.stringify({ verdict: "unsure" }) }];
}
if (sent.includes("Is this defect real?")) text = sent.includes(PLANTED.title) ? { verdict: "not_real", reason: "Line 1 exports add." } : { verdict: "real", reason: "Line 2 subtracts." };
if (sent.includes("Plan the review")) text = { areas: [], outside_diff: [] };
if (sent.includes("Write the review.")) text = { summary: "One defect.", comments: [{ finding: 0, title: "add subtracts", body: "Use +." }] };
return [{ type: "text", text: JSON.stringify(text) }];
Expand Down Expand Up @@ -227,3 +238,27 @@ test("a write answer that isn't JSON fails its step, which asks again", async ()
expect(run.terminalStatus).toBe("completed");
expect(run.outputs["review"]).toMatchObject({ summary: "One defect." });
});

test("a candidate the check answers not real is left out of the review and kept with the check's verdict and reason", async () => {
planted = true;
try {
const { run, attempts } = await review(join(dir, "data"));
expect(run.terminalStatus).toBe("completed");
expect(attempts["code-review-check"]).toBe(2);
expect(run.outputs["review"]).toMatchObject({
comments: [{ path: "src/a.ts", start: 2, title: "add subtracts" }],
dropped: [{ path: "src/a.ts", start: 1, title: "add is not exported", verdict: "not_real", reason: "Line 1 exports add." }],
});
} finally {
planted = false;
}
});

test("a check answer that is not a verdict and reason fails its step, which asks again", async () => {
checkUnsure = 1;
const { run, attempts } = await review(join(dir, "data"));
expect(checkUnsure).toBe(0);
expect(attempts["code-review-check"]).toBe(2);
expect(run.terminalStatus).toBe("completed");
expect(run.outputs["review"]).toMatchObject({ comments: [{ path: "src/a.ts", start: 2 }], dropped: [] });
});
10 changes: 6 additions & 4 deletions packages/pr-code-review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ under `prefix`, as `codeReviewReviewer`, the review a pull request's manager dep

The request is `{ kind: "code-review", title, description, diff, checkout }`. The title and description are as the host
shows them, without hidden text. Optional fields are `number` (the pull request's), `efforts` per step,
`budgets.repoFinder`, `check`, and `cut` (`{ lines, of, unread? }`) when the sender cut the diff for size, `unread`
`budgets.repoFinder`, and `cut` (`{ lines, of, unread? }`) when the sender cut the diff for size, `unread`
naming the files it cut part-way or left out; the review carries it and never puts those names in a prompt.

### Process
Expand All @@ -32,9 +32,11 @@ naming the files it cut part-way or left out; the review carries it and never pu
finders reported each finding.
4. **consolidate** reads the lines each finding cites at the head, moves it onto its quoted line and groups the
findings by defect. A group none of whose findings had a line of its file shown to the model is marked `unverified`.
5. **check** confirms each group, only when the request sets `check`.
6. **write** comments on every group, most severe and most agreed-on first; each comment carries the code it quotes
and how many finders reported it.
5. **check** reads each group's candidate, the code it quotes and the file lines around it in one model call per
group, on every review, and answers real or not real with a reason. A group it finds not real is
left out of the review and kept in its `dropped`, in its finder's words with the verdict and reason.
6. **write** comments on every group left, most severe and most agreed-on first; each comment carries the code it
quotes and how many finders reported it.

A failed model call fails its step, as does an answer that can't be read; every model step then tries again twice, 5
and 10 seconds later. A step that fails for good fails the review, which shows as not finished.
Expand Down
6 changes: 3 additions & 3 deletions packages/pr-code-review/src/actions/review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function reply(value: unknown) {

/** Every message the review builds for one pull request, its findings and the consolidate step's excerpts. */
async function messages(request: { title: string; description: string; diff: string; cut?: { lines: number; of: number; unread: string[] } }, plan: string, finding: Record<string, unknown>, excerpt: string) {
const planned = await reviewInputFor({ efforts })(stockTriggerMail({ kind: "code-review", ...request, checkout, check: true }));
const planned = await reviewInputFor({ efforts })(stockTriggerMail({ kind: "code-review", ...request, checkout }));
const reviewInput = { output: planned };
const finders = await findersInput({ reviewInput, plan: reply(plan) });
const folded = await candidates({ reviewInput, findDiff: reply({ findings: [finding] }), findRepo: reply({ findings: [] }) });
Expand Down Expand Up @@ -130,13 +130,13 @@ function readsFrom(files: Record<string, string[]>) {
};
}

/** The review of one diff finder reply over `files`, the consolidate model replying `groups`. */
/** The review of one diff finder reply over `files`, the consolidate model replying `groups` and the check finding each real. */
async function reviewed(findings: unknown[], files: Record<string, string[]>, groups: unknown[] = []) {
const reviewInput = { output: await reviewInputFor({ efforts })(stockTriggerMail({ kind: "code-review", title: "t", description: "d", checkout, diff: diffOf("src/a.ts", "x") })) };
const folded = await candidates({ reviewInput, findDiff: reply({ findings }), findRepo: reply({ findings: [] }) });
const consolidate = reply((await consolidated(folded.consolidate, readsFrom(files), groups)).replied);
const checked = await checks({ reviewInput, candidates: { output: folded }, consolidate });
const written = await writeInput({ reviewInput, candidates: { output: folded }, checks: { output: checked }, check: { output: [] } });
const written = await writeInput({ reviewInput, candidates: { output: folded }, checks: { output: checked }, check: { output: checked.checks.map(() => reply({ verdict: "real", reason: "r" }).output) } });
return review({ reviewInput, writeInput: { output: written }, write: reply({ summary: "One defect.", comments: [{ finding: 0, title: "wrong total", body: "b" }] }) });
}

Expand Down
Loading
Loading