mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-14 09:58:11 -10:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
144b6ef381 | ||
|
|
9446b42c2d | ||
|
|
f9fecd5c5a |
No files matched your search
+1
-1
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "@expo/code-review-cli",
|
||||
"version": "0.2.1",
|
||||
"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');
|
||||
});
|
||||
|
||||
+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}\``;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+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 };
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user