mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-13 20:28:06 -10:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
144b6ef381 | ||
|
|
9446b42c2d | ||
|
|
f9fecd5c5a | ||
|
|
9e2156727f | ||
|
|
33a970a6bb | ||
|
|
7338e16a06 | ||
|
|
7817776f6c | ||
|
|
c8cbb6a581 |
No files matched your search
@@ -68,6 +68,26 @@ ecr review --pr 4057
|
||||
ecr review --pr 4057 --post
|
||||
```
|
||||
|
||||
Options (most to least common):
|
||||
|
||||
| Flag | What it does |
|
||||
| --- | --- |
|
||||
| `--pr <n>` | Review GitHub PR #n by number (diff fetched via `gh`, no checkout); not combinable with `--base`/`--head`/`--staged`. |
|
||||
| `--post` | With `--pr`, also post the result as the PR comment (needs `gh` auth). Omit to preview only; re-run with `--post` to publish. |
|
||||
| `--staged` | Review only staged changes. |
|
||||
| `--base <ref>` | Base ref to diff against (default: merge-base with the default branch). |
|
||||
| `--head <ref>` | Head ref to diff (default: working tree, incl. uncommitted changes). |
|
||||
| `--agents <a,b>` | Run only these agents (comma-separated ids); default: all. |
|
||||
| `--route` | Let an LLM router pick the relevant agents from the diff. |
|
||||
| `--repo <owner/repo>` | Repo for `--pr` (default: inferred from the current checkout). |
|
||||
| `--json` | Emit machine-readable JSON on stdout. |
|
||||
| `--no-fail` | Always exit 0 (otherwise a `request_changes` decision exits non-zero). |
|
||||
| `-h`, `--help` | Show help. |
|
||||
|
||||
`--pr` uses the PR's diff (authoritative) but reads your checked-out files for
|
||||
surrounding context; for full fidelity, `gh pr checkout <n>` first and run a plain
|
||||
`ecr review`.
|
||||
|
||||
In CI it runs automatically from the scaffolded workflows — by label or a `/review`
|
||||
comment (see **CI usage**). From Claude Code (or another agent), add a slash command
|
||||
that runs it; eas-cli's
|
||||
@@ -113,31 +133,6 @@ and applies Anthropic prompt caching automatically.
|
||||
|
||||
</details>
|
||||
|
||||
<details>
|
||||
<summary><b><code>ecr review</code> options</b></summary>
|
||||
|
||||
```
|
||||
--base <ref> Base ref to diff against (default: merge-base with default branch)
|
||||
--head <ref> Head ref to diff (default: working tree, incl. uncommitted changes)
|
||||
--staged Review only staged changes
|
||||
--pr <n> Review GitHub PR #n by number (diff fetched via gh, no checkout);
|
||||
not combinable with --base/--head/--staged
|
||||
--repo <owner/repo> Repo for --pr (default: inferred from the current checkout)
|
||||
--post With --pr: also post the result as the PR comment (needs gh auth).
|
||||
Omit to preview only; re-run with --post to publish.
|
||||
--agents <a,b> Run only these agents (comma-separated ids); default: all
|
||||
--route Let an LLM router pick the relevant agents from the diff
|
||||
--json Emit machine-readable JSON on stdout
|
||||
--no-fail Always exit 0 (otherwise a request_changes decision exits non-zero)
|
||||
-h, --help Show help
|
||||
```
|
||||
|
||||
`--pr` uses the PR's diff (authoritative) but reads your checked-out files for
|
||||
surrounding context; for full fidelity, `gh pr checkout <n>` first and run a plain
|
||||
`ecr review`.
|
||||
|
||||
</details>
|
||||
|
||||
<details>
|
||||
<summary><b>Configuration — <code>.expo-code-review/</code></b></summary>
|
||||
|
||||
|
||||
+25
-11
@@ -496,17 +496,31 @@ is dependable.
|
||||
Root cause: the reviewer runs one pass per agent with no verification, so an LLM's
|
||||
plausible-but-wrong claim ships as-is. Fixes, cheapest/highest-leverage first:
|
||||
|
||||
1. ✅ **Quote-grounding (deterministic) — shipped.** Findings now carry an
|
||||
`evidence` field (the flagged code, copied verbatim); `verifyFindings` checks it
|
||||
against the real file and drops any finding whose evidence isn't there. Verified:
|
||||
a hallucinated critical quoting `const item = next++` is dropped because that
|
||||
text isn't in the file. Fails safe — `unknown` (too little evidence / file not on
|
||||
disk) never drops.
|
||||
2. ✅ **Adversarial verify pass for criticals — shipped.** A restricted `verifier`
|
||||
agent (read+grep) re-reads the cited file and must confirm each surviving critical
|
||||
is genuine (biased to reject; refuted criticals dropped). Runs in parallel, 3-min
|
||||
cap, fails open (keeps a critical if verification itself errors). Decision is
|
||||
re-derived after drops (no criticals left → soften `request_changes`).
|
||||
1. ✅ **Quote-grounding (deterministic) — shipped, then softened.** Findings carry an
|
||||
`evidence` field (the flagged code, verbatim); `verifyFindings` grades it against
|
||||
the real file. Originally, evidence absent from the file was a **hard drop**.
|
||||
Measured on ~13 real PRs that proved too aggressive: it dropped 3 findings vs. 1
|
||||
surfaced, and ≥1 dropped finding was a **confirmed real bug** (#4057's "--account
|
||||
silently ignored"). Root cause: exact-substring is a good POSITIVE signal but a
|
||||
poor NEGATIVE one — it misfires on structural/"missing" bugs (no single line *is*
|
||||
the bug), cross-line quotes, ellipsis, copied comment/diff markers, and slightly
|
||||
wrong locations. **Fix (shipped 2026-07-23):**
|
||||
- **Fuzzy match** (`matchEvidence`/`evidenceFragments`): exact substring OR any
|
||||
substantive line/fragment present verbatim, after stripping comment/diff markers
|
||||
and splitting on newlines + ellipsis. Rescues the mechanical misquotes.
|
||||
- **Escalate, don't drop:** absent evidence (any severity) now routes to the LLM
|
||||
verifier (which re-reads the real file + nearby files) instead of being dropped;
|
||||
a finding is dropped ONLY on refutation. `present`/`unknown` non-criticals keep
|
||||
the cheap no-LLM fast path.
|
||||
- **Verifier judges substance, not wording:** the verifier prompt now treats the
|
||||
quote as an imperfect hint and rejects only if the underlying problem isn't real;
|
||||
the reviewer prompt asks for one contiguous verbatim line (no ellipsis/paraphrase).
|
||||
2. ✅ **Adversarial verify pass — shipped (now all-severity on demand).** A restricted
|
||||
`verifier` agent (read+grep) re-reads the cited file and must confirm the finding is
|
||||
genuine (biased to reject). Runs for every critical, and for any finding whose
|
||||
evidence didn't ground (see #1). Parallel, 3-min cap, fails open (keeps the finding
|
||||
if verification itself errors). Decision is re-derived after drops (no criticals
|
||||
left → soften `request_changes`).
|
||||
3. **Oracle checks.** A finding that asserts "won't compile / type error" can be
|
||||
validated against `tsc`; "crashes"/"breaks tests" against the test suite. At
|
||||
minimum, never surface a compile-error claim when the package compiles.
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "@expo/code-review-cli",
|
||||
"version": "0.2.0",
|
||||
"version": "0.2.2",
|
||||
"description": "Generic, config-driven AI code reviewer engine. Repos supply their agents via .expo-code-review/.",
|
||||
"license": "MIT",
|
||||
"repository": {
|
||||
|
||||
@@ -1,6 +1,11 @@
|
||||
import { test, expect } from 'bun:test';
|
||||
|
||||
import { renderMarkdown, parseEmbeddedFingerprints, parseReviewState } from '../core/render.js';
|
||||
import {
|
||||
renderMarkdown,
|
||||
parseEmbeddedFingerprints,
|
||||
parseReviewState,
|
||||
buildDiffLineIndex,
|
||||
} from '../core/render.js';
|
||||
import { fingerprintFinding } from '../core/schema.js';
|
||||
import type { CoordinatorOutput, Finding } from '../core/schema.js';
|
||||
|
||||
@@ -44,22 +49,70 @@ test('review state (review + dismissals) round-trips via parseReviewState', () =
|
||||
expect(state!.review.findings.length).toBe(1);
|
||||
});
|
||||
|
||||
test('links a finding location to the PR diff line when link context is given', () => {
|
||||
test('links a finding location to the PR diff line when the line is in the diff', () => {
|
||||
const out = renderMarkdown({ ...base, findings: [finding({ file: 'src/a.ts', line: 12 })] }, 'tag', [], {
|
||||
repo: 'expo/eas-cli',
|
||||
prNumber: 42,
|
||||
diffLines: new Map([['src/a.ts', new Set([12])]]),
|
||||
});
|
||||
// Markdown link wrapping the `file:line`, pointing at the Files-changed diff anchor.
|
||||
expect(out).toContain('[`src/a.ts:12`](https://github.com/expo/eas-cli/pull/42/files#diff-');
|
||||
expect(out).toMatch(/R12\)/); // right-hand line anchor for line 12
|
||||
});
|
||||
|
||||
test('a finding NOT in the diff links to the source blob on the base (not a dead diff anchor)', () => {
|
||||
// Line 99 is not a changed line → link to the base blob, not the diff anchor.
|
||||
const out = renderMarkdown({ ...base, findings: [finding({ file: 'src/a.ts', line: 99 })] }, 'tag', [], {
|
||||
repo: 'expo/eas-cli',
|
||||
prNumber: 42,
|
||||
diffLines: new Map([['src/a.ts', new Set([12])]]),
|
||||
baseSha: 'abc123',
|
||||
});
|
||||
expect(out).toContain('[`src/a.ts:99`](https://github.com/expo/eas-cli/blob/abc123/src/a.ts#L99)');
|
||||
expect(out).not.toContain('/pull/42/files#diff-'); // not the diff anchor
|
||||
});
|
||||
|
||||
test('a finding on a file absent from the diff links to the base blob', () => {
|
||||
const out = renderMarkdown({ ...base, findings: [finding({ file: 'other.ts', line: 3 })] }, 'tag', [], {
|
||||
repo: 'expo/eas-cli',
|
||||
prNumber: 42,
|
||||
diffLines: new Map([['src/a.ts', new Set([12])]]),
|
||||
baseSha: 'abc123',
|
||||
});
|
||||
expect(out).toContain('[`other.ts:3`](https://github.com/expo/eas-cli/blob/abc123/other.ts#L3)');
|
||||
});
|
||||
|
||||
test('out-of-diff finding is plain text when no base SHA is available', () => {
|
||||
const out = renderMarkdown({ ...base, findings: [finding({ file: 'other.ts', line: 3 })] }, 'tag', [], {
|
||||
repo: 'expo/eas-cli',
|
||||
prNumber: 42,
|
||||
diffLines: new Map([['src/a.ts', new Set([12])]]),
|
||||
});
|
||||
expect(out).toContain('`other.ts:3`');
|
||||
expect(out).not.toContain('https://github.com');
|
||||
});
|
||||
|
||||
test('location is plain (unlinked) code when no link context is given', () => {
|
||||
const out = renderMarkdown({ ...base, findings: [finding({ file: 'src/a.ts', line: 12 })] }, 'tag');
|
||||
expect(out).toContain('`src/a.ts:12`');
|
||||
expect(out).not.toContain('https://github.com');
|
||||
});
|
||||
|
||||
test('buildDiffLineIndex: collects right-side added + context lines, skips deletions', () => {
|
||||
const patch = [
|
||||
'--- a/src/a.ts',
|
||||
'+++ b/src/a.ts',
|
||||
'@@ -10,3 +10,4 @@',
|
||||
' context10', // right line 10 (context)
|
||||
'-removed', // left only, no right line
|
||||
'+added11', // right line 11
|
||||
'+added12', // right line 12
|
||||
' context13', // right line 13
|
||||
].join('\n');
|
||||
const index = buildDiffLineIndex([{ path: 'src/a.ts', patch }]);
|
||||
expect([...index.get('src/a.ts')!].sort((a, b) => a - b)).toEqual([10, 11, 12, 13]);
|
||||
});
|
||||
|
||||
test('comment footer no longer says "Phase 1"', () => {
|
||||
expect(renderMarkdown(base, 'tag')).not.toContain('Phase 1');
|
||||
});
|
||||
|
||||
@@ -3,17 +3,66 @@ import { mkdtempSync, writeFileSync, rmSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import path from 'node:path';
|
||||
|
||||
import { verifyFindings } from '../core/verify.js';
|
||||
import { verifyFindings, matchEvidence, evidenceFragments } from '../core/verify.js';
|
||||
import type { Finding } from '../core/schema.js';
|
||||
import type { OpencodeHandle } from '../core/opencode.js';
|
||||
|
||||
const FILE_CONTENT = [
|
||||
'export const answer = 42;',
|
||||
'function realThing() {',
|
||||
' return items[next++]!;',
|
||||
'}',
|
||||
].join('\n');
|
||||
|
||||
// ---- pure matching (matchEvidence / evidenceFragments) ----
|
||||
|
||||
test('matchEvidence: exact (whitespace-normalized) substring is present', () => {
|
||||
expect(matchEvidence('return items[next++]!;', FILE_CONTENT)).toBe('present');
|
||||
expect(matchEvidence(' return items[next++]!; ', FILE_CONTENT)).toBe('present');
|
||||
});
|
||||
|
||||
test('matchEvidence: fuzzy — a cross-line quote still matches on a contiguous line', () => {
|
||||
// Evidence joins two non-adjacent lines; the exact join is not a substring, but
|
||||
// one substantive line is → present (rescues cross-line quotes).
|
||||
const evidence = 'function realThing() {\n return items[next++]!;';
|
||||
expect(matchEvidence(evidence, FILE_CONTENT)).toBe('present');
|
||||
});
|
||||
|
||||
test('matchEvidence: fuzzy — ellipsis elision matches on a fragment', () => {
|
||||
expect(matchEvidence('function realThing() { … return items[next++]!; }', FILE_CONTENT)).toBe(
|
||||
'present'
|
||||
);
|
||||
});
|
||||
|
||||
test('matchEvidence: fuzzy — copied comment/diff markers are stripped before matching', () => {
|
||||
expect(matchEvidence('+ return items[next++]!;', FILE_CONTENT)).toBe('present');
|
||||
expect(matchEvidence('// return items[next++]!;', FILE_CONTENT)).toBe('present');
|
||||
});
|
||||
|
||||
test('matchEvidence: a genuinely invented quote is absent', () => {
|
||||
expect(matchEvidence('const item = next++; // nowhere in the file', FILE_CONTENT)).toBe('absent');
|
||||
});
|
||||
|
||||
test('matchEvidence: too-short evidence is unknown (never conclude absent)', () => {
|
||||
expect(matchEvidence('x}', FILE_CONTENT)).toBe('unknown');
|
||||
expect(matchEvidence('', FILE_CONTENT)).toBe('unknown');
|
||||
});
|
||||
|
||||
test('evidenceFragments: splits on newlines + ellipsis, strips markers, drops short bits', () => {
|
||||
const frags = evidenceFragments('+ return items[next++]!;\n// x\nconst somethingLongEnough = 1;');
|
||||
expect(frags).toContain('return items[next++]!;');
|
||||
expect(frags).toContain('const somethinglongenough = 1;'); // normalized (lowercased)
|
||||
expect(frags.some(f => f === 'x')).toBe(false); // too short, dropped
|
||||
});
|
||||
|
||||
// ---- verifyFindings integration ----
|
||||
|
||||
let dir: string;
|
||||
let file: string;
|
||||
|
||||
beforeAll(() => {
|
||||
dir = mkdtempSync(path.join(tmpdir(), 'ecr-verify-test-'));
|
||||
file = path.join(dir, 'sample.ts');
|
||||
writeFileSync(file, 'export const answer = 42;\nfunction realThing() { return items[next++]!; }\n');
|
||||
writeFileSync(file, FILE_CONTENT);
|
||||
});
|
||||
afterAll(() => rmSync(dir, { recursive: true, force: true }));
|
||||
|
||||
@@ -27,27 +76,44 @@ const finding = (over: Partial<Finding>): Finding => ({
|
||||
...over,
|
||||
});
|
||||
|
||||
// A dummy handle is safe here: only NON-critical findings are used, so the LLM
|
||||
// verify path (which would need a real server) is never reached.
|
||||
// A dummy handle: any LLM verify call throws, which exercises the fail-OPEN path
|
||||
// (keep the finding). Grounded non-criticals never reach the LLM at all.
|
||||
const handle = {} as OpencodeHandle;
|
||||
|
||||
test('quote-grounding keeps findings whose evidence is present, drops absent ones', async () => {
|
||||
const findings = [
|
||||
finding({ title: 'present', evidence: 'return items[next++]!;' }),
|
||||
finding({ title: 'hallucinated', evidence: 'const item = next++; // not in the file' }),
|
||||
];
|
||||
const res = await verifyFindings(handle, findings, '/', undefined);
|
||||
test('grounded non-critical is kept without an LLM call', async () => {
|
||||
const res = await verifyFindings(
|
||||
handle,
|
||||
[finding({ title: 'present', evidence: 'return items[next++]!;' })],
|
||||
'/'
|
||||
);
|
||||
expect(res.kept.map(f => f.title)).toEqual(['present']);
|
||||
expect(res.dropped.map(d => d.finding.title)).toEqual(['hallucinated']);
|
||||
expect(res.dropped).toEqual([]);
|
||||
});
|
||||
|
||||
test('too-short evidence is unknown, never dropped', async () => {
|
||||
const res = await verifyFindings(handle, [finding({ title: 'short', evidence: 'x}' })], '/', undefined);
|
||||
test('absent evidence is ESCALATED, not hard-dropped (fail-open keeps it here)', async () => {
|
||||
// The key regression guard: an imperfect/absent quote must not be a silent drop.
|
||||
// With the dummy handle the escalated LLM call throws → fail-open → kept.
|
||||
const res = await verifyFindings(
|
||||
handle,
|
||||
[finding({ title: 'absent-but-maybe-real', evidence: 'const item = next++; // not in file' })],
|
||||
'/'
|
||||
);
|
||||
expect(res.kept.map(f => f.title)).toEqual(['absent-but-maybe-real']);
|
||||
expect(res.dropped).toEqual([]);
|
||||
});
|
||||
|
||||
test('too-short evidence is unknown, kept without an LLM call', async () => {
|
||||
const res = await verifyFindings(handle, [finding({ title: 'short', evidence: 'x}' })], '/');
|
||||
expect(res.kept.map(f => f.title)).toEqual(['short']);
|
||||
expect(res.dropped).toEqual([]);
|
||||
});
|
||||
|
||||
test('unreadable file is unknown, never dropped', async () => {
|
||||
const f = finding({ title: 'missing-file', file: '/no/such/file.ts', evidence: 'const somethingLongEnough = 1;' });
|
||||
const res = await verifyFindings(handle, [f], '/', undefined);
|
||||
const f = finding({
|
||||
title: 'missing-file',
|
||||
file: '/no/such/file.ts',
|
||||
evidence: 'const somethingLongEnough = 1;',
|
||||
});
|
||||
const res = await verifyFindings(handle, [f], '/');
|
||||
expect(res.kept.map(x => x.title)).toEqual(['missing-file']);
|
||||
});
|
||||
+30
-9
@@ -207,26 +207,38 @@ export function buildCrossCuttingTask(
|
||||
export function buildVerifierSystem(): string {
|
||||
return [
|
||||
'You are a skeptical verifier of a single code-review finding. Your default is',
|
||||
'DISTRUST. Using your read/grep tools, open the cited file, locate the code, and',
|
||||
'confirm the finding against what the source ACTUALLY says.',
|
||||
'DISTRUST. Using your read/grep tools, open the cited file (search nearby files',
|
||||
'if the code is not exactly there), locate the relevant code, and judge whether',
|
||||
'the described PROBLEM is actually present in the source.',
|
||||
'',
|
||||
'Judge the SUBSTANCE, not the wording. The finding\'s quoted "evidence" may be',
|
||||
'paraphrased, abbreviated, quoted across non-adjacent lines, or slightly',
|
||||
'misquoted, and its file/line may be approximate. None of that alone makes the',
|
||||
'finding false — verify against what the code actually does. Do NOT reject merely',
|
||||
'because the quoted snippet is not a verbatim match; reject only if the',
|
||||
'underlying problem is not real.',
|
||||
'',
|
||||
'Mark verified=false (reject) if any of these hold:',
|
||||
'- the code the finding describes or quotes is not actually present as claimed',
|
||||
' (it misread or invented the code),',
|
||||
'- the described failure/exploit cannot actually occur,',
|
||||
'- the described problem does not actually occur in the code (it misread or',
|
||||
' invented the behavior),',
|
||||
'- the described failure/exploit cannot actually happen,',
|
||||
"- the claim is internally contradictory (e.g. asserts a type error in code that",
|
||||
' compiles), or',
|
||||
'- you cannot substantiate it after reading the file.',
|
||||
'- you cannot substantiate the underlying issue after reading the file.',
|
||||
'',
|
||||
'Only mark verified=true when you have CONFIRMED, from the real source, that the',
|
||||
'flagged code exists as described and the problem is genuine. When unsure, reject.',
|
||||
'Mark verified=true when you have CONFIRMED, from the real source, that the',
|
||||
'described problem genuinely exists. When genuinely unsure whether it is real,',
|
||||
'reject.',
|
||||
'',
|
||||
'Return ONLY this JSON object and nothing else:',
|
||||
'{"verified": true|false, "reason": "one concise sentence grounded in the file"}',
|
||||
].join('\n');
|
||||
}
|
||||
|
||||
export function buildVerifierTask(finding: Finding): string {
|
||||
export function buildVerifierTask(
|
||||
finding: Finding,
|
||||
opts: { evidenceUngrounded?: boolean } = {}
|
||||
): string {
|
||||
const lines = [
|
||||
'Verify this finding by reading the real source (do not trust its wording):',
|
||||
'',
|
||||
@@ -245,6 +257,15 @@ export function buildVerifierTask(finding: Finding): string {
|
||||
'EVIDENCE'
|
||||
);
|
||||
}
|
||||
if (opts.evidenceUngrounded) {
|
||||
lines.push(
|
||||
'',
|
||||
'NOTE: the quoted evidence could NOT be located verbatim in the file. It may be',
|
||||
'a paraphrase, an elision, or a slightly wrong location — do not reject on that',
|
||||
'basis alone. Read the file (and nearby files) and judge whether the described',
|
||||
'problem is genuinely present.'
|
||||
);
|
||||
}
|
||||
lines.push(
|
||||
'',
|
||||
'Open the file, find the relevant code, and return the single verdict JSON object.'
|
||||
|
||||
+80
-7
@@ -6,10 +6,66 @@ import type { CoordinatorOutput, Decision, DismissalRecord, Finding, Severity }
|
||||
/**
|
||||
* Enough PR context to turn a finding's `file:line` into a link to that line in
|
||||
* the PR's "Files changed" diff. Omitted for terminal output (plain text).
|
||||
*
|
||||
* `diffLines` maps each changed file to the set of right-side (new-version) line
|
||||
* numbers present in the PR's diff. A `#diff-…R<line>` anchor only exists for lines
|
||||
* actually shown in the diff, so we link ONLY when the finding's file+line is in
|
||||
* here — otherwise the finding points at unchanged code (a caller, a helper the PR
|
||||
* merely uses) and a diff link would be dead, so we render plain text instead.
|
||||
*/
|
||||
export interface LinkContext {
|
||||
repo: string; // owner/repo
|
||||
prNumber: number;
|
||||
diffLines?: Map<string, Set<number>>;
|
||||
/**
|
||||
* The PR base commit SHA (the branch the PR targets, e.g. `main`). When a finding
|
||||
* points at code NOT in the diff — a caller or helper the PR merely references,
|
||||
* which is by definition unchanged and therefore identical on the base — we link
|
||||
* to the source blob at this commit (`/blob/<sha>/<path>#L<line>`) instead of a
|
||||
* diff anchor: still a working, line-accurate link, just to the file on the base
|
||||
* branch rather than the diff. A commit SHA (not a branch name) so it's a stable
|
||||
* permalink that won't drift as the base advances.
|
||||
*/
|
||||
baseSha?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the file → right-side-line-numbers index from changed files' patch text,
|
||||
* by walking each unified-diff hunk (`@@ -a,b +c,d @@`) and collecting the new-tree
|
||||
* line number of every added (`+`) and context (` `) line. Deleted (`-`) lines have
|
||||
* no right-side line and are skipped.
|
||||
*/
|
||||
export function buildDiffLineIndex(
|
||||
files: Array<{ path: string; patch: string }>
|
||||
): Map<string, Set<number>> {
|
||||
const index = new Map<string, Set<number>>();
|
||||
const hunkRe = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/;
|
||||
for (const file of files) {
|
||||
const lines = new Set<number>();
|
||||
let right = 0;
|
||||
let inHunk = false;
|
||||
for (const raw of file.patch.split('\n')) {
|
||||
const hunk = hunkRe.exec(raw);
|
||||
if (hunk) {
|
||||
right = parseInt(hunk[1]!, 10);
|
||||
inHunk = true;
|
||||
continue;
|
||||
}
|
||||
if (!inHunk || raw.startsWith('+++') || raw.startsWith('---') || raw.startsWith('\\')) {
|
||||
continue;
|
||||
}
|
||||
const marker = raw[0];
|
||||
if (marker === '+' || marker === ' ') {
|
||||
lines.add(right);
|
||||
right++;
|
||||
}
|
||||
// '-' is left-side only (no new-tree line); anything else is ignored.
|
||||
}
|
||||
if (lines.size > 0) {
|
||||
index.set(file.path, lines);
|
||||
}
|
||||
}
|
||||
return index;
|
||||
}
|
||||
|
||||
const DECISION_LABEL: Record<Decision, string> = {
|
||||
@@ -49,19 +105,36 @@ function locationText(finding: Finding): string {
|
||||
}
|
||||
|
||||
/**
|
||||
* Render a finding's location as inline code, linked to the exact diff line in the
|
||||
* PR's "Files changed" tab when PR context is available. GitHub anchors each file's
|
||||
* diff as `diff-<sha256(path)>` and each right-hand (added/context) line as `…R<n>`.
|
||||
* Render a finding's location as inline code, linked to the code it points at:
|
||||
* - in the diff (file+line shown in a hunk) → the PR's "Files changed" tab at that
|
||||
* line (`#diff-<sha256(path)>R<n>`), so the reader lands in the review diff;
|
||||
* - not in the diff (unchanged code the PR references, e.g. a caller/helper) → the
|
||||
* source blob on the PR base at that line (`/blob/<baseSha>/<path>#L<n>`);
|
||||
* - if neither is possible (no link context / no base SHA) → plain inline code.
|
||||
* Never emits a dead diff anchor for a line that isn't in the diff.
|
||||
*/
|
||||
function location(finding: Finding, link?: LinkContext): string {
|
||||
const text = locationText(finding);
|
||||
if (!link) {
|
||||
return `\`${text}\``;
|
||||
}
|
||||
const fileHash = createHash('sha256').update(finding.file).digest('hex');
|
||||
const anchor = finding.line != null ? `diff-${fileHash}R${finding.line}` : `diff-${fileHash}`;
|
||||
const url = `https://github.com/${link.repo}/pull/${link.prNumber}/files#${anchor}`;
|
||||
return `[\`${text}\`](${url})`;
|
||||
const fileLines = link.diffLines?.get(finding.file);
|
||||
// In the diff when the file is present and (if the finding names a line) that line
|
||||
// is one of the diff's right-side lines. A file-level finding (no line) counts as
|
||||
// in-diff as long as the file appears in the diff.
|
||||
const inDiff = fileLines != null && (finding.line == null || fileLines.has(finding.line));
|
||||
if (inDiff) {
|
||||
const fileHash = createHash('sha256').update(finding.file).digest('hex');
|
||||
const anchor = finding.line != null ? `diff-${fileHash}R${finding.line}` : `diff-${fileHash}`;
|
||||
const url = `https://github.com/${link.repo}/pull/${link.prNumber}/files#${anchor}`;
|
||||
return `[\`${text}\`](${url})`;
|
||||
}
|
||||
if (link.baseSha) {
|
||||
const lineAnchor = finding.line != null ? `#L${finding.line}` : '';
|
||||
const url = `https://github.com/${link.repo}/blob/${link.baseSha}/${finding.file}${lineAnchor}`;
|
||||
return `[\`${text}\`](${url})`;
|
||||
}
|
||||
return `\`${text}\``;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+102
-63
@@ -22,36 +22,81 @@ export interface VerificationResult {
|
||||
}
|
||||
|
||||
/**
|
||||
* Deterministic quote-grounding: does the finding's `evidence` snippet actually
|
||||
* appear in the file? Returns `unknown` (don't judge) when there's too little
|
||||
* evidence or the file can't be read (e.g. a base-ref checkout that lacks a
|
||||
* PR-added file), so we never drop a finding we couldn't actually check.
|
||||
* Break `evidence` into normalized, substantive fragments for fuzzy matching:
|
||||
* split on newlines AND ellipses (the model often elides with `…`/`...`), strip
|
||||
* leading diff markers (`+`/`-`) and comment markers (`//`, `#`, `*`) it may have
|
||||
* copied along, normalize, and keep only fragments long enough to be meaningful.
|
||||
*/
|
||||
export function evidenceFragments(evidence: string): string[] {
|
||||
return evidence
|
||||
.split(/\r?\n|…|\.\.\./)
|
||||
.map(line => line.replace(/^[+\-\s]*/, '').replace(/^(\/\/+|#+|\*+|\/\*)\s?/, ''))
|
||||
.map(normalizeCode)
|
||||
.filter(fragment => fragment.length >= MIN_EVIDENCE_LEN);
|
||||
}
|
||||
|
||||
/**
|
||||
* Does the finding's `evidence` correspond to code in the file?
|
||||
* - exact (whitespace-normalized) substring → 'present'
|
||||
* - else any substantive line/fragment present verbatim → 'present' (fuzzy: this
|
||||
* rescues cross-line quotes, ellipsis elisions, and copied comment/diff markers)
|
||||
* - a real quote that matches nothing → 'absent'
|
||||
* Returns 'unknown' (don't judge) when evidence is too short to conclude anything
|
||||
* or the file can't be read, so we never drop a finding we couldn't actually check.
|
||||
* NOTE: 'absent' is NOT terminal — the caller escalates it to the LLM verifier
|
||||
* rather than dropping, because an imperfect quote does not mean a false finding.
|
||||
*/
|
||||
export function matchEvidence(
|
||||
evidence: string,
|
||||
content: string
|
||||
): 'present' | 'absent' | 'unknown' {
|
||||
const normEvidence = normalizeCode(evidence);
|
||||
if (normEvidence.length < MIN_EVIDENCE_LEN) {
|
||||
return 'unknown';
|
||||
}
|
||||
const normContent = normalizeCode(content);
|
||||
if (normContent.includes(normEvidence)) {
|
||||
return 'present';
|
||||
}
|
||||
const fragments = evidenceFragments(evidence);
|
||||
if (fragments.length === 0) {
|
||||
return 'unknown';
|
||||
}
|
||||
return fragments.some(fragment => normContent.includes(fragment)) ? 'present' : 'absent';
|
||||
}
|
||||
|
||||
/** Read the cited file and grade the evidence against it (see matchEvidence). */
|
||||
async function evidencePresence(
|
||||
finding: Finding,
|
||||
cwd: string
|
||||
): Promise<'present' | 'absent' | 'unknown'> {
|
||||
const evidence = normalizeCode(finding.evidence ?? '');
|
||||
if (evidence.length < MIN_EVIDENCE_LEN) {
|
||||
return 'unknown';
|
||||
}
|
||||
let content: string;
|
||||
try {
|
||||
content = await readFile(path.resolve(cwd, finding.file), 'utf8');
|
||||
} catch {
|
||||
return 'unknown';
|
||||
}
|
||||
return normalizeCode(content).includes(evidence) ? 'present' : 'absent';
|
||||
return matchEvidence(finding.evidence ?? '', content);
|
||||
}
|
||||
|
||||
/**
|
||||
* Guard against hallucinated findings before they're surfaced:
|
||||
* 1. Quote-grounding (deterministic, all findings): drop any whose quoted
|
||||
* `evidence` is definitively not in the file.
|
||||
* 2. Adversarial verify (LLM, criticals only): a skeptical pass re-reads the real
|
||||
* file and must confirm the critical is genuine; refuted criticals are dropped.
|
||||
* Fails OPEN — if a verify call itself errors, the critical is kept (better a
|
||||
* possible false positive than hiding a real critical on an infra hiccup).
|
||||
* Guard against hallucinated findings before they're surfaced, WITHOUT silently
|
||||
* dropping real ones on an imperfect quote:
|
||||
* 1. Quote-grounding (deterministic, all findings): grade each finding's `evidence`
|
||||
* against the file (exact + fuzzy — see matchEvidence).
|
||||
* 2. LLM verify (adversarial, in parallel) runs for a finding when EITHER:
|
||||
* - its evidence is `absent` (any severity) — the quote isn't grounded, but
|
||||
* that alone doesn't make the finding false, so the verifier re-reads the
|
||||
* real file (and nearby files) and judges the underlying problem; or
|
||||
* - it's a `critical` (even if grounded) — a skeptical double-check.
|
||||
* A finding is dropped ONLY when the verifier refutes it. `present`/`unknown`
|
||||
* non-criticals are kept without an LLM call (the fast, cheap path).
|
||||
* Fails OPEN — if a verify call itself errors, the finding is kept (better a
|
||||
* possible false positive than hiding a real finding on an infra hiccup).
|
||||
*
|
||||
* This replaces the old "absent evidence → hard drop" rule, which was suppressing
|
||||
* real findings whose natural evidence (a structural/absence bug, a cross-line
|
||||
* quote, a slightly-wrong location) wasn't a verbatim substring.
|
||||
*/
|
||||
export async function verifyFindings(
|
||||
handle: OpencodeHandle,
|
||||
@@ -63,66 +108,60 @@ export async function verifyFindings(
|
||||
let cost = 0;
|
||||
const tokens: TokenUsage = {};
|
||||
|
||||
// Phase 1 — quote-grounding for every finding.
|
||||
// Phase 1 — deterministic quote-grounding for every finding.
|
||||
const checked = await Promise.all(
|
||||
findings.map(async finding => ({ finding, presence: await evidencePresence(finding, cwd) }))
|
||||
);
|
||||
const survivors: Finding[] = [];
|
||||
|
||||
// Decide which findings need an LLM check vs. can be kept directly.
|
||||
const verdicts = new Map<Finding, 'keep' | 'drop'>();
|
||||
const toVerify: Array<{ finding: Finding; presence: 'present' | 'absent' | 'unknown' }> = [];
|
||||
for (const { finding, presence } of checked) {
|
||||
if (presence === 'absent') {
|
||||
dropped.push({ finding, reason: 'quoted code not found in file (likely hallucinated)' });
|
||||
onProgress?.(
|
||||
` verify: dropped ${finding.severity} "${finding.title}" — quoted code not in ${finding.file}`
|
||||
);
|
||||
if (presence === 'absent' || finding.severity === 'critical') {
|
||||
toVerify.push({ finding, presence });
|
||||
} else {
|
||||
survivors.push(finding);
|
||||
verdicts.set(finding, 'keep'); // grounded (or uncheckable) non-critical
|
||||
}
|
||||
}
|
||||
|
||||
// Phase 2 — adversarial verify for surviving criticals, in parallel.
|
||||
const refuted = new Set<Finding>();
|
||||
// Phase 2 — LLM verify (parallel). Refuted → drop; verified or errored → keep.
|
||||
await Promise.all(
|
||||
survivors
|
||||
.filter(finding => finding.severity === 'critical')
|
||||
.map(async (finding, index) => {
|
||||
try {
|
||||
const { value, cost: verifyCost, tokens: verifyTokens } = await promptAndParse(
|
||||
handle,
|
||||
{
|
||||
agent: VERIFIER_AGENT,
|
||||
system: buildVerifierSystem(),
|
||||
text: buildVerifierTask(finding),
|
||||
title: `verify-${index}`,
|
||||
maxWaitMs: VERIFY_TIMEOUT_MS,
|
||||
finalizeOnTimeout: true,
|
||||
},
|
||||
parseVerdict
|
||||
);
|
||||
cost += verifyCost;
|
||||
addTokenUsage(tokens, verifyTokens);
|
||||
if (!value.verified) {
|
||||
refuted.add(finding);
|
||||
onProgress?.(
|
||||
` verify: dropped critical "${finding.title}" — ${value.reason || 'refuted by verifier'}`
|
||||
);
|
||||
}
|
||||
} catch (error) {
|
||||
// Fail open: keep the critical if verification itself failed.
|
||||
toVerify.map(async ({ finding, presence }, index) => {
|
||||
try {
|
||||
const { value, cost: verifyCost, tokens: verifyTokens } = await promptAndParse(
|
||||
handle,
|
||||
{
|
||||
agent: VERIFIER_AGENT,
|
||||
system: buildVerifierSystem(),
|
||||
text: buildVerifierTask(finding, { evidenceUngrounded: presence === 'absent' }),
|
||||
title: `verify-${index}`,
|
||||
maxWaitMs: VERIFY_TIMEOUT_MS,
|
||||
finalizeOnTimeout: true,
|
||||
},
|
||||
parseVerdict
|
||||
);
|
||||
cost += verifyCost;
|
||||
addTokenUsage(tokens, verifyTokens);
|
||||
if (value.verified) {
|
||||
verdicts.set(finding, 'keep');
|
||||
} else {
|
||||
verdicts.set(finding, 'drop');
|
||||
dropped.push({ finding, reason: value.reason || 'refuted by verifier' });
|
||||
onProgress?.(
|
||||
` verify: could not verify critical "${finding.title}" (${errorMessage(error)}); keeping it`
|
||||
` verify: dropped ${finding.severity} "${finding.title}" — ${value.reason || 'refuted by verifier'}`
|
||||
);
|
||||
}
|
||||
})
|
||||
} catch (error) {
|
||||
// Fail open: keep the finding if verification itself failed.
|
||||
verdicts.set(finding, 'keep');
|
||||
onProgress?.(
|
||||
` verify: could not verify "${finding.title}" (${errorMessage(error)}); keeping it`
|
||||
);
|
||||
}
|
||||
})
|
||||
);
|
||||
|
||||
const kept: Finding[] = [];
|
||||
for (const finding of survivors) {
|
||||
if (refuted.has(finding)) {
|
||||
dropped.push({ finding, reason: 'refuted by verifier' });
|
||||
} else {
|
||||
kept.push(finding);
|
||||
}
|
||||
}
|
||||
|
||||
// Preserve original order.
|
||||
const kept = findings.filter(finding => verdicts.get(finding) === 'keep');
|
||||
return { kept, dropped, cost, tokens };
|
||||
}
|
||||
+39
-6
@@ -3,7 +3,9 @@ import { tmpdir } from 'node:os';
|
||||
import path from 'node:path';
|
||||
|
||||
import { run } from '../core/exec.js';
|
||||
import { commentMarker, parseReviewState, renderMarkdown } from '../core/render.js';
|
||||
import { parseUnifiedDiff } from '../core/diff.js';
|
||||
import { buildDiffLineIndex, commentMarker, parseReviewState, renderMarkdown } from '../core/render.js';
|
||||
import type { LinkContext } from '../core/render.js';
|
||||
import { fingerprintFinding } from '../core/schema.js';
|
||||
import type { CoordinatorOutput, DismissalRecord } from '../core/schema.js';
|
||||
import type { Reporter } from './reporter.js';
|
||||
@@ -65,12 +67,42 @@ export class GitHubReporter implements Reporter {
|
||||
const dismissed = existing
|
||||
? (parseReviewState(existing.body, this.options.commentTag)?.dismissed ?? [])
|
||||
: [];
|
||||
await this.upsertComment(renderMarkdown(review, this.options.commentTag, dismissed, this.linkContext()));
|
||||
const link = await this.linkContextAsync();
|
||||
await this.upsertComment(renderMarkdown(review, this.options.commentTag, dismissed, link));
|
||||
}
|
||||
|
||||
/** PR context for turning finding locations into diff-line links. */
|
||||
private linkContext(): { repo: string; prNumber: number } {
|
||||
return { repo: this.options.repo, prNumber: this.options.prNumber };
|
||||
/**
|
||||
* PR context for turning finding locations into links: the set of lines actually
|
||||
* in the diff (for in-diff findings → diff-anchor links) and the base commit SHA
|
||||
* (for out-of-diff findings → source-blob links on the base). Both fetches fail
|
||||
* soft — a missing piece just degrades to a plain-text location, never a dead link.
|
||||
*/
|
||||
private async linkContextAsync(): Promise<LinkContext> {
|
||||
const link: LinkContext = { repo: this.options.repo, prNumber: this.options.prNumber };
|
||||
const prArgs = [String(this.options.prNumber), '--repo', this.options.repo];
|
||||
const cwd = this.options.cwd;
|
||||
await Promise.all([
|
||||
(async () => {
|
||||
try {
|
||||
const { stdout } = await run('gh', ['pr', 'diff', ...prArgs], { cwd });
|
||||
link.diffLines = buildDiffLineIndex(parseUnifiedDiff(stdout));
|
||||
} catch {
|
||||
// leave diffLines unset → in-diff findings degrade to plain text
|
||||
}
|
||||
})(),
|
||||
(async () => {
|
||||
try {
|
||||
const { stdout } = await run('gh', ['pr', 'view', ...prArgs, '--json', 'baseRefOid'], { cwd });
|
||||
const oid = (JSON.parse(stdout) as { baseRefOid?: string }).baseRefOid;
|
||||
if (oid) {
|
||||
link.baseSha = oid;
|
||||
}
|
||||
} catch {
|
||||
// leave baseSha unset → out-of-diff findings degrade to plain text
|
||||
}
|
||||
})(),
|
||||
]);
|
||||
return link;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -104,9 +136,10 @@ export class GitHubReporter implements Reporter {
|
||||
}
|
||||
}
|
||||
|
||||
const link = await this.linkContextAsync();
|
||||
await this.patchComment(
|
||||
existing.id,
|
||||
renderMarkdown(state.review, this.options.commentTag, dismissed, this.linkContext())
|
||||
renderMarkdown(state.review, this.options.commentTag, dismissed, link)
|
||||
);
|
||||
return { dismissedCount: dismissed.length, matched, unmatched };
|
||||
}
|
||||
|
||||
+7
-5
@@ -65,7 +65,7 @@ Return **only** a single fenced ```json code block, an object of this shape:
|
||||
"line": 142,
|
||||
"title": "short one-line summary",
|
||||
"rationale": "why this is a problem, with the concrete failure/exploit path",
|
||||
"evidence": "the exact line(s) of code you are flagging, copied VERBATIM",
|
||||
"evidence": "one contiguous line of the flagged code, copied VERBATIM",
|
||||
"suggestion": "optional concrete fix, or omit"
|
||||
}
|
||||
]
|
||||
@@ -73,7 +73,9 @@ Return **only** a single fenced ```json code block, an object of this shape:
|
||||
```
|
||||
|
||||
`line` is the start line in the new version of the file, or `null` if not
|
||||
line-specific. `evidence` MUST be the flagged code copied **verbatim** from the
|
||||
file — it is used to verify the finding, and a finding whose evidence isn't found
|
||||
in the file is discarded (don't paraphrase or invent it). If you have nothing to
|
||||
report, return `{ "findings": [] }`. Emit no prose outside the JSON block.
|
||||
line-specific. `evidence` is used to help verify the finding, so make it easy to
|
||||
locate: copy **one contiguous line** of the flagged code **verbatim** (not spanning
|
||||
multiple lines, no `…` elisions, no paraphrasing). For a structural/"missing" issue,
|
||||
quote the single most relevant real line (e.g. the early `return` that skips the
|
||||
handling). If you have nothing to report, return `{ "findings": [] }`. Emit no prose
|
||||
outside the JSON block.
|
||||
Reference in new issue
Block a user