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
52 changes: 51 additions & 1 deletion __tests__/unit/ai-review-prompt.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ describe('renderReviewMarkdown', () => {
},
42, 'abc1234', meta
);
expect(out).toContain('AI Code Review for PR #42');
expect(out).toContain('Automated review for PR #42');
expect(out).toContain('💬 Comment');
expect(out).toContain('Adds X.');
expect(out).toContain('`a.js:5`');
Expand All @@ -300,6 +300,56 @@ describe('renderReviewMarkdown', () => {
expect(out).not.toContain('✅ Approve');
});

it('renders the new "Automated review" header with the HTML supersede marker', () => {
const out = renderReviewMarkdown(
{
verdict: 'comment',
summary: 'x',
findings: [{ file: 'a.js', line: 1, quoted_line: '+x', claim: 'no test for x at a.js:1' }]
},
99, 'def5678', meta
);
expect(out).toContain('<!-- temper-automated-review -->');
expect(out).toContain('## Automated review for PR #99');
expect(out).not.toContain('## AI Code Review');
});

it('shows finding-source breakdown footer (mechanical vs AI)', () => {
const out = renderReviewMarkdown(
{
verdict: 'comment',
summary: 'mixed.',
findings: [
{ source: 'oracle:rivet-validate', severity: 'warning', artifact_id: 'REQ-001', claim: 'REQ-001: foo' },
{ file: 'a.js', line: 1, quoted_line: '+x', claim: 'no test for x at a.js:1' }
]
},
99, 'def5678', meta
);
expect(out).toMatch(/_Findings: 1 mechanical \(rivet\) · 1 from local AI model\._/);
});

it('renders grouped oracle finding using artifact_ids array', () => {
const out = renderReviewMarkdown(
{
verdict: 'comment',
summary: 'group.',
findings: [
{
source: 'oracle:rivet-validate',
severity: 'warning',
artifact_id: '3 artifacts',
artifact_ids: ['REQ-1', 'REQ-2', 'REQ-3'],
claim: 'no downstream — affecting: REQ-1, REQ-2, REQ-3'
}
]
},
99, 'def5678', meta
);
expect(out).toContain('🟡 `3 artifacts`');
expect(out).toContain('affecting: REQ-1, REQ-2, REQ-3');
});

it('renders oracle findings with severity badge and artifact_id (not file:line)', () => {
const out = renderReviewMarkdown(
{
Expand Down
107 changes: 106 additions & 1 deletion __tests__/unit/rivet-oracle.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import {
isRivetProject,
runRivetValidate,
runRivetImpact,
runRivetOracle
runRivetOracle,
subtractFindings,
groupOracleFindings,
findingKey
} from '../../src/rivet-oracle.js';

function makeRunnerOk(stdout) {
Expand Down Expand Up @@ -201,6 +204,108 @@ describe('runRivetImpact', () => {
});
});

describe('findingKey', () => {
it('combines source, artifact_id, and message', () => {
const f = {
source: 'oracle:rivet-validate',
artifact_id: 'REQ-001',
message: 'missing field'
};
expect(findingKey(f)).toBe('oracle:rivet-validate|REQ-001|missing field');
});

it('falls back to claim when message is absent', () => {
const f = { source: 'oracle:rivet-validate', artifact_id: 'X', claim: 'X: foo' };
expect(findingKey(f)).toContain('X: foo');
});

it('returns empty string for nullish input', () => {
expect(findingKey(null)).toBe('');
expect(findingKey(undefined)).toBe('');
});
});

describe('subtractFindings', () => {
const base = [
{ source: 'oracle:rivet-validate', artifact_id: 'REQ-001', message: 'no downstream', severity: 'warning' },
{ source: 'oracle:rivet-validate', artifact_id: 'REQ-002', message: 'no downstream', severity: 'warning' }
];

it('drops findings already present at base', () => {
const head = [...base, { source: 'oracle:rivet-validate', artifact_id: 'REQ-003', message: 'no downstream', severity: 'warning' }];
const delta = subtractFindings(head, base);
expect(delta).toHaveLength(1);
expect(delta[0].artifact_id).toBe('REQ-003');
});

it('keeps everything when base is empty', () => {
expect(subtractFindings(base, [])).toEqual(base);
expect(subtractFindings(base, undefined)).toEqual(base);
});

it('returns [] when head ⊆ base (no new findings = correct silence)', () => {
expect(subtractFindings(base, base)).toEqual([]);
});

it('treats same artifact + same message but different severity as the same finding', () => {
const head = [{ source: 'oracle:rivet-validate', artifact_id: 'REQ-001', message: 'no downstream', severity: 'error' }];
expect(subtractFindings(head, base)).toEqual([]);
});
});

describe('groupOracleFindings', () => {
it('collapses identical messages into one finding with artifact_ids list', () => {
const findings = [
{ source: 'oracle:rivet-validate', severity: 'warning', artifact_id: 'REQ-001', message: 'no downstream' },
{ source: 'oracle:rivet-validate', severity: 'warning', artifact_id: 'REQ-002', message: 'no downstream' },
{ source: 'oracle:rivet-validate', severity: 'warning', artifact_id: 'REQ-003', message: 'no downstream' }
];
const grouped = groupOracleFindings(findings);
expect(grouped).toHaveLength(1);
expect(grouped[0].artifact_ids).toEqual(['REQ-001', 'REQ-002', 'REQ-003']);
expect(grouped[0].claim).toContain('REQ-001');
expect(grouped[0].claim).toContain('REQ-002');
expect(grouped[0].artifact_id).toBe('3 artifacts');
});

it('keeps singletons in their original shape', () => {
const findings = [
{ source: 'oracle:rivet-validate', severity: 'error', artifact_id: 'REQ-001', message: 'broken ref' }
];
const grouped = groupOracleFindings(findings);
expect(grouped).toHaveLength(1);
expect(grouped[0].artifact_id).toBe('REQ-001');
expect(grouped[0].artifact_ids).toBeUndefined();
});

it('keeps different severities as separate groups even with same message', () => {
const findings = [
{ source: 'oracle:rivet-validate', severity: 'warning', artifact_id: 'A', message: 'foo' },
{ source: 'oracle:rivet-validate', severity: 'error', artifact_id: 'B', message: 'foo' }
];
expect(groupOracleFindings(findings)).toHaveLength(2);
});

it('passes through model findings without grouping', () => {
const findings = [
{ file: 'a.js', line: 1, quoted_line: '+x', claim: 'no test' },
{ file: 'b.js', line: 1, quoted_line: '+y', claim: 'no test' }
];
expect(groupOracleFindings(findings)).toHaveLength(2);
});

it('truncates the displayed id list at maxIdsShown', () => {
const ids = Array.from({ length: 25 }, (_, i) => `REQ-${String(i + 1).padStart(3, '0')}`);
const findings = ids.map((id) => ({
source: 'oracle:rivet-validate', severity: 'warning', artifact_id: id, message: 'shared'
}));
const grouped = groupOracleFindings(findings, { maxIdsShown: 5 });
expect(grouped).toHaveLength(1);
expect(grouped[0].artifact_ids).toHaveLength(25);
expect(grouped[0].claim).toMatch(/\(\+20 more\)/);
});
});

describe('runRivetOracle', () => {
let tmpDir;
beforeEach(() => {
Expand Down
23 changes: 20 additions & 3 deletions src/ai-review-prompt.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,16 @@ export function renderReviewMarkdown(parsed, prNumber, headSha, meta, opts = {})
request_changes: '🔴 Request changes'
}[verdict];

const oracleCount = findings.filter(
(f) => typeof f?.source === 'string' && f.source.startsWith('oracle:')
).length;
const modelCount = findings.length - oracleCount;

const lines = [];
lines.push(`## AI Code Review for PR #${prNumber}`);
// HTML marker first so supersedePreviousReviews can identify our comments
// even if the visible header changes.
lines.push('<!-- temper-automated-review -->');
lines.push(`## Automated review for PR #${prNumber}`);
if (meta?.headRepo && meta?.baseBranch && meta?.headBranch && meta?.baseRepo) {
lines.push(`${meta.headRepo}:\`${meta.headBranch}\` → ${meta.baseRepo}:\`${meta.baseBranch}\``);
}
Expand All @@ -260,6 +268,10 @@ export function renderReviewMarkdown(parsed, prNumber, headSha, meta, opts = {})
lines.push('');
lines.push(`**Summary:** ${parsed.summary || '(none)'}`);
lines.push('');
lines.push(
`_Findings: ${oracleCount} mechanical (rivet) · ${modelCount} from local AI model._`
);
lines.push('');

if (findings.length === 0) {
lines.push('**Findings:** None.');
Expand All @@ -268,13 +280,18 @@ export function renderReviewMarkdown(parsed, prNumber, headSha, meta, opts = {})
lines.push('');
findings.forEach((f, i) => {
if (typeof f.source === 'string' && f.source.startsWith('oracle:')) {
// Oracle finding: artifact-anchored, mechanically validated.
// Oracle finding: artifact-anchored, mechanically validated. When
// findings are grouped, `artifact_ids` is a multi-element array;
// when single, it falls back to `artifact_id`.
const sevBadge =
f.severity === 'error' ? '🔴'
: f.severity === 'warning' ? '🟡'
: 'ℹ️';
const oracleName = f.source.replace('oracle:', '');
lines.push(`${i + 1}. ${sevBadge} \`${f.artifact_id || '<unknown>'}\` _(${oracleName})_`);
const label = Array.isArray(f.artifact_ids) && f.artifact_ids.length > 1
? `${f.artifact_ids.length} artifacts`
: f.artifact_id || '<unknown>';
lines.push(`${i + 1}. ${sevBadge} \`${label}\` _(${oracleName})_`);
lines.push(` ${f.claim}`);
} else {
// Model finding: file:line-anchored, post-validated.
Expand Down
60 changes: 54 additions & 6 deletions src/ai-review.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ import {
renderReviewMarkdown,
buildResponseFormat
} from './ai-review-prompt.js';
import { runRivetOracle } from './rivet-oracle.js';
import {
runRivetOracle,
runRivetValidate,
subtractFindings,
groupOracleFindings
} from './rivet-oracle.js';
import { hasRivetYaml, withTempRepoCheckout } from './rivet-fetch.js';

const _reviewTimestamps = new Map();
Expand Down Expand Up @@ -270,6 +275,11 @@ async function callLocalAI(endpoint, model, systemPrompt, userPrompt, options =
}
}

// Stable HTML marker for supersedePreviousReviews. Markdown renders it as
// nothing visible but a substring search finds it reliably regardless of
// header text changes. The legacy 'AI Code Review' string also matches old
// comments still in the wild from before the rename.
const REVIEW_MARKER = '<!-- temper-automated-review -->';
const AI_REVIEW_SIGNATURE = 'AI Code Review';

function formatReviewComment(aiResponse, prNumber, headSha, meta) {
Expand Down Expand Up @@ -323,7 +333,7 @@ async function supersedePreviousReviews(octokit, owner, repo, prNumber) {
for (const comment of comments) {
if (
comment.body &&
comment.body.includes(AI_REVIEW_SIGNATURE) &&
(comment.body.includes(REVIEW_MARKER) || comment.body.includes(AI_REVIEW_SIGNATURE)) &&
!comment.body.startsWith('>')
) {
await octokit.request(
Expand Down Expand Up @@ -415,12 +425,50 @@ async function reviewPullRequest(octokit, owner, repo, prNumber) {
timeout: rivetCfg.timeout_ms || 60000
})
);
if (oracleSummary?.findings) {
oracleFindings = oracleSummary.findings;
let headOracleFindings = oracleSummary?.findings || [];

// Delta filter: subtract findings already present at the PR base.
// Without this, the review surfaces the entire repo backlog every
// time (see spar#175 — 91 pre-existing warnings drowning the diff).
let baseValidate = null;
if (baseSha && baseSha !== headSha) {
try {
baseValidate = await withTempRepoCheckout(
octokit,
owner,
repo,
baseSha,
(repoPath) => runRivetValidate(resolvedBinary, repoPath, {
timeout: rivetCfg.timeout_ms || 60000
})
);
} catch (err) {
getLogger().warn(
{ pr: prNumber, err: err.message },
'base-ref validate failed — falling back to head-only oracle findings'
);
}
}
const droppedCount = headOracleFindings.length;
headOracleFindings = subtractFindings(
headOracleFindings,
baseValidate?.findings || []
);
const baseDelta = droppedCount - headOracleFindings.length;

// Group identical messages so 50 "Every requirement should be
// satisfied by ..." diagnostics collapse into one finding listing
// the affected artifact ids.
oracleFindings = groupOracleFindings(headOracleFindings);

getLogger().info(
{ pr: prNumber, oracleFindings: oracleFindings.length },
'Rivet oracle complete'
{
pr: prNumber,
oracleFindings: oracleFindings.length,
droppedAsPreExisting: baseDelta,
afterGrouping: oracleFindings.length
},
'Rivet oracle complete (delta + grouped)'
);
}
} catch (err) {
Expand Down
Loading
Loading