Compare commits

..
8 Commits
Author SHA1 Message Date
Brent Vatne 144b6ef381 Release v0.2.2 2026-07-22 22:52:20 -07:00
Brent Vatne 9446b42c2d Link out-of-diff findings to the source blob on the PR base
Instead of plain text, a finding that points at unchanged code the PR merely
references (not in the diff) now links to that line in the source at the PR base
commit (`/blob/<baseSha>/<path>#L<line>`) — the code is unchanged, so it's
identical on the base, and this is still a working, line-accurate link (just to
the file view, not the diff). In-diff findings still link to the diff anchor.

The reporter fetches the base SHA (gh pr view --json baseRefOid) alongside the
diff; both fail soft to plain text.
2026-07-22 22:49:53 -07:00
Brent Vatne f9fecd5c5a Link finding locations only when they're actually in the PR diff
Previously every finding's `file:line` was turned into a `#diff-<sha>R<line>`
link, but that anchor only exists for lines shown in the PR diff. Findings on
unchanged code the PR merely references (e.g. a caller, a helper) got dead links.

- buildDiffLineIndex(): walk each changed file's hunks and collect the right-side
  (new-tree) line numbers actually in the diff.
- location(): emit a markdown link only when the finding's file+line is in that
  index; otherwise render plain inline code (no dead link).
- GitHubReporter fetches the diff once (gh pr diff, fails soft) and passes the
  index into the link context for both report() and applyDismissal().
2026-07-22 22:46:56 -07:00
Brent Vatne 9e2156727f Release v0.2.1 2026-07-22 22:39:09 -07:00
Brent Vatne 33a970a6bb Quote-grounding: fuzzy match + escalate-on-absence (stop silent-dropping real findings)
Measured on ~13 real PRs, the deterministic "evidence absent from file → hard drop"
rule was too aggressive: it dropped 3 findings vs 1 surfaced, and at least one drop
was a confirmed real bug. Exact-substring is a good positive signal but a poor
negative one — it misfires on structural/"missing" bugs, cross-line quotes, ellipsis,
copied comment/diff markers, and slightly-wrong locations.

- Fuzzy match (matchEvidence/evidenceFragments): exact substring OR any substantive
  line/fragment present verbatim, after stripping comment/diff markers and splitting
  on newlines + ellipsis.
- Escalate, don't drop: absent evidence (any severity) now routes to the LLM verifier
  instead of a hard drop; a finding is dropped only on refutation. present/unknown
  non-criticals keep the no-LLM fast path.
- Verifier judges substance not wording (quote treated as an imperfect hint);
  reviewer prompt asks for one contiguous verbatim line.

Tests: +8 (pure matchEvidence/evidenceFragments cases + escalate-not-drop guard).
2026-07-22 22:38:57 -07:00
Brent Vatne 7338e16a06 Docs: render ecr review flags as a table (cleaner than the aligned code block) 2026-07-22 19:34:10 -07:00
Brent Vatne 7817776f6c Docs: order ecr review flags by likelihood of use 2026-07-22 19:26:24 -07:00
Brent Vatne c8cbb6a581 Docs: inline ecr review flags into Usage (drop the separate options section) 2026-07-22 19:24:38 -07:00
10 changed files with 441 additions and 145 deletions

No files matched your search

+20 -25
View File
@@ -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
View File
@@ -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
View File
@@ -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": {
+55 -2
View File
@@ -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');
});
+82 -16
View File
@@ -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
View File
@@ -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
View File
@@ -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
View File
@@ -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
View File
@@ -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
View File
@@ -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.