diff --git a/action/src/comment-handler.ts b/action/src/comment-handler.ts index 9acb2d6..0b5e7ad 100644 --- a/action/src/comment-handler.ts +++ b/action/src/comment-handler.ts @@ -151,10 +151,7 @@ export async function handleComment(context: Context): Promise { const { findings, summary } = await runFastReview(prContext); - if (findings.length > 0) { - await poster.postReview(prNumber, findings); - } - await poster.postSummaryComment(prNumber, summary); + await poster.postReviewResults(prNumber, prContext, findings, summary); } catch (err) { const msg = err instanceof Error ? err.message : String(err); await poster.postAcknowledgement(prNumber, `[ERROR] **OpenReview** — Review failed: ${msg}`); diff --git a/action/src/pr-handler.ts b/action/src/pr-handler.ts index 550d536..3bb674e 100644 --- a/action/src/pr-handler.ts +++ b/action/src/pr-handler.ts @@ -82,15 +82,11 @@ export async function handlePullRequest(context: Context): Promise { const { findings, summary } = await runFastReview(prContext); - // Post batch review with inline comments - if (findings.length > 0) { - await poster.postReview(prNumber, findings); - } - - // Post or update summary comment - await poster.postSummaryComment(prNumber, summary); + const finalSummary = await poster.postReviewResults(prNumber, prContext, findings, summary); - core.info(`Review complete: ${summary.totalFindings} findings in ${summary.duration}`); + core.info( + `Review complete: ${finalSummary.totalFindings} open, ${finalSummary.resolvedCount ?? 0} resolved (${finalSummary.duration})`, + ); } catch (err) { const msg = err instanceof Error ? err.message : String(err); core.error(`Review failed: ${msg}`); diff --git a/cli/src/commands/review.ts b/cli/src/commands/review.ts index 95e4d34..298d037 100644 --- a/cli/src/commands/review.ts +++ b/cli/src/commands/review.ts @@ -134,15 +134,15 @@ export function registerReviewCommand(program: Command): void { } const poster = new CommentPoster(client); - - if (findings.length > 0) { - await poster.postReview(prNumber, findings); - } - - await poster.postSummaryComment(prNumber, summary); + const finalSummary = await poster.postReviewResults( + prNumber, + prContext, + findings, + summary, + ); process.stderr.write( - `✅ Review posted on PR #${prNumber} (${findings.length} finding${findings.length === 1 ? '' : 's'})\n`, + `✅ Review posted on PR #${prNumber} (${finalSummary.totalFindings} open, ${finalSummary.resolvedCount ?? 0} resolved)\n`, ); } } catch (err) { diff --git a/core/src/github/comments.ts b/core/src/github/comments.ts index 963fed6..24d00ba 100644 --- a/core/src/github/comments.ts +++ b/core/src/github/comments.ts @@ -1,5 +1,11 @@ import { formatInlineComment, formatSummaryComment } from '../review/formatter.js'; -import type { ReviewFinding, ReviewSummary } from '../review/types.js'; +import { + buildReviewState, + enrichReviewSummary, + parseReviewState, + type ReviewState, +} from '../review/review-state.js'; +import type { PRContext, ReviewFinding, ReviewSummary } from '../review/types.js'; import type { GitHubClient } from './client.js'; @@ -7,8 +13,11 @@ import type { GitHubClient } from './client.js'; export type { ReviewFinding, ReviewSummary }; export type { FindingCategory, FindingSeverity, FindingSource, Citation } from '../review/types.js'; export { formatInlineComment, formatSummaryComment }; +export { parseReviewState, enrichReviewSummary, buildReviewState }; +export type { ReviewState }; -const SUMMARY_MARKER = ''; +export const REVIEW_SUMMARY_MARKER = ''; +export const COVERAGE_SUMMARY_MARKER = ''; /* ------------------------------------------------------------------ */ /* Comment Poster */ @@ -40,25 +49,57 @@ export class CommentPoster { ); } + /** + * Incremental review post: only new inline comments, summary updated with + * resolved/open tracking across commits (gitar-bot style). + */ + async postReviewResults( + prNumber: number, + pr: PRContext, + findings: ReviewFinding[], + baseSummary: ReviewSummary, + ): Promise { + const previousState = await this.fetchReviewState(prNumber); + const summary = enrichReviewSummary(pr, findings, baseSummary, previousState); + const state = buildReviewState(pr, findings); + + const newFindings = summary.newFindings ?? findings; + if (newFindings.length > 0) { + await this.postReview(prNumber, newFindings); + } + + await this.postSummaryComment(prNumber, summary, state); + return summary; + } + /** * Post or update the summary comment. * Uses replace-not-duplicate strategy via the HTML marker tag. */ - async postSummaryComment(prNumber: number, summary: ReviewSummary): Promise { - const body = formatSummaryComment(summary); - const existingId = await this.findSummaryComment(prNumber); + async postSummaryComment( + prNumber: number, + summary: ReviewSummary, + state?: ReviewState, + ): Promise { + const body = formatSummaryComment(summary, state); + await this.postOrUpdateMarkedComment(prNumber, REVIEW_SUMMARY_MARKER, body); + } - if (existingId) { - await this.client['api'].patch( - `/repos/${this.client.owner}/${this.client.repo}/issues/comments/${existingId}`, - { body }, - ); - } else { - await this.client['api'].post( - `/repos/${this.client.owner}/${this.client.repo}/issues/${prNumber}/comments`, - { body }, - ); - } + /** + * Post or update the coverage summary on the original PR. + * Replaces the previous coverage comment instead of posting a new one each push. + */ + async postCoverageComment(prNumber: number, body: string): Promise { + const content = body.includes(COVERAGE_SUMMARY_MARKER) + ? body + : `${COVERAGE_SUMMARY_MARKER}\n${body}`; + await this.postOrUpdateMarkedComment(prNumber, COVERAGE_SUMMARY_MARKER, content); + } + + /** Load persisted review state from the existing summary comment, if any. */ + async fetchReviewState(prNumber: number): Promise { + const body = await this.fetchMarkedCommentBody(prNumber, REVIEW_SUMMARY_MARKER); + return body ? parseReviewState(body) : null; } /** Reply in a PR thread by posting a new issue comment. */ @@ -79,19 +120,77 @@ export class CommentPoster { /* ---- Internal helpers ---- */ - private async findSummaryComment(prNumber: number): Promise { + private async postOrUpdateMarkedComment( + prNumber: number, + marker: string, + body: string, + ): Promise { + const latestIssue = await this.fetchLatestIssueComment(prNumber); + const marked = await this.findMarkedCommentRecord(prNumber, marker); + + // Update in place only when our summary is already the newest timeline comment. + // Otherwise append a fresh comment so results are visible at the bottom. + if (marked && latestIssue && marked.id === latestIssue.id) { + await this.client['api'].patch( + `/repos/${this.client.owner}/${this.client.repo}/issues/comments/${marked.id}`, + { body }, + ); + return; + } + + await this.client['api'].post( + `/repos/${this.client.owner}/${this.client.repo}/issues/${prNumber}/comments`, + { body }, + ); + } + + private async fetchLatestIssueComment( + prNumber: number, + ): Promise<{ id: number; body: string } | null> { + const { data } = await this.client['api'].get( + `/repos/${this.client.owner}/${this.client.repo}/issues/${prNumber}/comments`, + { params: { per_page: 1, page: 1, sort: 'created', direction: 'desc' } }, + ); + + const comment = data[0]; + if (!comment || typeof comment.body !== 'string') return null; + return { id: comment.id, body: comment.body }; + } + + private async fetchMarkedCommentBody( + prNumber: number, + marker: string, + ): Promise { + const comment = await this.findMarkedCommentRecord(prNumber, marker); + return typeof comment?.body === 'string' ? comment.body : null; + } + + private async findMarkedComment(prNumber: number, marker: string): Promise { + const comment = await this.findMarkedCommentRecord(prNumber, marker); + return comment?.id ?? null; + } + + /** + * Return the newest issue comment containing `marker`. + * Older duplicates are left untouched — we always refresh the latest one so + * updates appear near the bottom of busy PR threads. + */ + private async findMarkedCommentRecord( + prNumber: number, + marker: string, + ): Promise<{ id: number; body: string } | null> { let page = 1; const perPage = 100; while (true) { const { data } = await this.client['api'].get( `/repos/${this.client.owner}/${this.client.repo}/issues/${prNumber}/comments`, - { params: { per_page: perPage, page } }, + { params: { per_page: perPage, page, sort: 'created', direction: 'desc' } }, ); for (const comment of data) { - if (typeof comment.body === 'string' && comment.body.includes(SUMMARY_MARKER)) { - return comment.id; + if (typeof comment.body === 'string' && comment.body.includes(marker)) { + return { id: comment.id, body: comment.body }; } } diff --git a/core/src/index.ts b/core/src/index.ts index a87aa42..d598ecb 100644 --- a/core/src/index.ts +++ b/core/src/index.ts @@ -14,13 +14,23 @@ export type { ParsedDiff, Hunk, Line, MoveEvent } from './github/diff.js'; export { CommentPoster } from './github/comments.js'; // Review -export { runFastReview } from './review/fast-review.js'; +export { runFastReview, buildFastReviewSummary } from './review/fast-review.js'; +export { fingerprintPullRequestDiff, normalizeDiffForFingerprint, extractDiffForFiles } from './review/diff-fingerprint.js'; +export { isReviewableFile, filterReviewableFiles } from './review/reviewable-files.js'; export { runRLM } from './review/rlm-runner.js'; export type { RLMEvent, RLMEventHandler, RLMEventType } from './review/rlm-runner.js'; export { SnapshotBuilder } from './review/snapshot.js'; export type { SnapshotOptions } from './review/snapshot.js'; export { runLinters, deduplicateFindings } from './review/linters.js'; export { formatInlineComment, formatSummaryComment } from './review/formatter.js'; +export { + parseReviewState, + enrichReviewSummary, + buildReviewState, + fingerprintFinding, + diffReviewFindings, +} from './review/review-state.js'; +export type { ReviewState, StoredFinding, ReviewDiff } from './review/review-state.js'; export { sortFindings } from './review/types.js'; export type { ReviewFinding, diff --git a/core/src/review/diff-fingerprint.ts b/core/src/review/diff-fingerprint.ts new file mode 100644 index 0000000..f7ac700 --- /dev/null +++ b/core/src/review/diff-fingerprint.ts @@ -0,0 +1,44 @@ +import { createHash } from 'node:crypto'; + +import { filterReviewableFiles } from './reviewable-files.js'; + +/** + * Stable SHA-256 fingerprint for a PR's reviewable patch. + * Same diff on different PR numbers/commits yields the same hash. + */ +export function fingerprintPullRequestDiff(diff: string, files: string[]): string { + const reviewableFiles = filterReviewableFiles(files); + const filteredDiff = extractDiffForFiles(diff, reviewableFiles); + const normalized = normalizeDiffForFingerprint(filteredDiff); + const payload = `${reviewableFiles.join('\n')}\n---\n${normalized}`; + return createHash('sha256').update(payload, 'utf8').digest('hex'); +} + +export function extractDiffForFiles(rawDiff: string, files: string[]): string { + if (files.length === 0) return ''; + + const want = new Set(files); + const parts = rawDiff.split(/^(?=diff --git )/m); + const kept: string[] = []; + + for (const part of parts) { + if (!part.trim()) continue; + const match = /^diff --git a\/(.+?) b\/(.+?)$/m.exec(part); + if (match && want.has(match[2])) { + kept.push(part); + } + } + + return kept.join(''); +} + +/** Normalize diff text so equivalent patches fingerprint the same. */ +export function normalizeDiffForFingerprint(diff: string): string { + return diff + .replace(/\r\n/g, '\n') + .replace(/^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@/gm, '@@') + .split('\n') + .map((line) => line.trimEnd()) + .join('\n') + .trim(); +} diff --git a/core/src/review/fast-review.ts b/core/src/review/fast-review.ts index 31c770f..4038838 100644 --- a/core/src/review/fast-review.ts +++ b/core/src/review/fast-review.ts @@ -6,6 +6,7 @@ import { config } from '../config/env.js'; import { createMainLLM, createStructuredLLM } from '../llm/router.js'; import { deduplicateFindings, runLinters } from './linters.js'; +import { isReviewableFile } from './reviewable-files.js'; import type { FindingSeverity, PRContext, @@ -102,37 +103,6 @@ function normalizeCategory(raw: string): 'bug' | 'flag' { return CATEGORY_BUG_KEYWORDS.some((kw) => s.includes(kw)) ? 'bug' : 'flag'; } -/* ------------------------------------------------------------------ */ -/* Non-reviewable file patterns */ -/* ------------------------------------------------------------------ */ - -const SKIP_PATTERNS = [ - /^yarn\.lock$/, - /^pnpm-lock\.yaml$/, - /^package-lock\.json$/, - /\.lock$/, - /\.min\.(js|css)$/, - /\.map$/, - /\.snap$/, - /\.svg$/, - /\.png$/, - /\.jpg$/, - /\.jpeg$/, - /\.gif$/, - /\.ico$/, - /\.woff2?$/, - /\.ttf$/, - /\.eot$/, - /\.pdf$/, - /dist\//, - /\.generated\./, - /vendor\//, -]; - -function isReviewableFile(filename: string): boolean { - return !SKIP_PATTERNS.some((pattern) => pattern.test(filename)); -} - /* ------------------------------------------------------------------ */ /* Diff chunking */ /* ------------------------------------------------------------------ */ @@ -286,7 +256,7 @@ export async function runFastReview( } } - const summary = buildSummary(sorted, pr.files.length, duration); + const summary = buildFastReviewSummary(sorted, pr.files.length, duration); return { findings: sorted, summary, trace }; } @@ -1009,7 +979,7 @@ function validateWithSnap( /* Summary builder */ /* ------------------------------------------------------------------ */ -function buildSummary( +export function buildFastReviewSummary( findings: ReviewFinding[], fileCount: number, durationMs: number, diff --git a/core/src/review/formatter.ts b/core/src/review/formatter.ts index e41ca21..0e344f8 100644 --- a/core/src/review/formatter.ts +++ b/core/src/review/formatter.ts @@ -1,3 +1,4 @@ +import { embedReviewState, formatFindingListItem, type ReviewState } from './review-state.js'; import type { FindingSeverity, ReviewFinding, ReviewSummary } from './types.js'; /* ------------------------------------------------------------------ */ @@ -11,41 +12,71 @@ const SEVERITY_BADGES: Record = { informational: '[INFO] **Informational**', }; -const SUMMARY_MARKER = ''; +export const SUMMARY_MARKER = ''; /* ------------------------------------------------------------------ */ -/* Summary Comment (§5.4) */ +/* Summary Comment */ /* ------------------------------------------------------------------ */ -export function formatSummaryComment(summary: ReviewSummary): string { - let md = `${SUMMARY_MARKER}\n## OpenReview Summary\n\n`; - md += `**Files reviewed:** ${summary.filesReviewed} | **Duration:** ${summary.duration} | **Mode:** ${summary.mode}\n\n`; +export function formatSummaryComment(summary: ReviewSummary, state?: ReviewState): string { + const lines: string[] = [SUMMARY_MARKER]; - if (summary.totalFindings === 0) { - md += '[SUCCESS] No issues found.\n'; - } else { - md += '| Severity | Count |\n|---|---|\n'; + if (state) { + lines.push(embedReviewState(state)); + } + + lines.push('## Code Review', ''); + + const resolvedCount = summary.resolvedCount ?? summary.resolvedFindings?.length ?? 0; + const openCount = summary.totalFindings; + const statusBadge = summary.approved ? '✅ **Approved**' : '⚠️ **Findings**'; + const tally = + resolvedCount > 0 + ? `**${resolvedCount} resolved** / **${openCount} open**` + : `**${openCount} finding${openCount === 1 ? '' : 's'}**`; + + lines.push(`${statusBadge} · ${tally}`, ''); - const order: FindingSeverity[] = ['severe', 'non-severe', 'investigate', 'informational']; - for (const sev of order) { - const count = summary.findingsBySeverity[sev]; - if (count > 0) { - md += `| ${SEVERITY_BADGES[sev]} | ${count} |\n`; - } - } + if (summary.narrative) { + lines.push(summary.narrative, ''); + } + + if (resolvedCount > 0 && summary.resolvedFindings?.length) { + lines.push( + '
', + `✅ ${resolvedCount} resolved`, + '', + ...summary.resolvedFindings.map(formatFindingListItem), + '', + '
', + '', + ); + } - md += `\n**Total:** ${summary.totalFindings} finding${summary.totalFindings === 1 ? '' : 's'}\n`; + const openFindings = summary.openFindings ?? []; + if (openCount > 0 && openFindings.length > 0) { + lines.push( + '
', + `${summary.approved ? 'ℹ️' : '🔎'} ${openCount} open`, + '', + ...openFindings.map((f) => formatFindingListItem(f)), + '', + '
', + '', + ); + } else if (openCount === 0) { + lines.push('✅ No open findings.', ''); } - md += '\n---\n'; - md += - '*Trigger deep review: `@openreview rlm` | Ask a question: `@openreview `*\n'; + lines.push( + `Files reviewed: ${summary.filesReviewed} · Duration: ${summary.duration}`, + ); - return md; + return lines.join('\n'); } /* ------------------------------------------------------------------ */ -/* Inline Comment (§5.5) */ +/* Inline Comment */ /* ------------------------------------------------------------------ */ export function formatInlineComment(finding: ReviewFinding): string { diff --git a/core/src/review/review-state.ts b/core/src/review/review-state.ts new file mode 100644 index 0000000..50c34f5 --- /dev/null +++ b/core/src/review/review-state.ts @@ -0,0 +1,228 @@ +import type { FindingCategory, FindingSeverity, PRContext, ReviewFinding, ReviewSummary } from './types.js'; + +/* ------------------------------------------------------------------ */ +/* Types */ +/* ------------------------------------------------------------------ */ + +export const REVIEW_STATE_MARKER = '`; +} + +export function parseReviewState(body: string): ReviewState | null { + const start = body.indexOf(REVIEW_STATE_MARKER); + if (start === -1) return null; + + const payloadStart = start + REVIEW_STATE_MARKER.length; + const end = body.indexOf(' -->', payloadStart); + if (end === -1) return null; + + try { + const raw = Buffer.from(body.slice(payloadStart, end), 'base64url').toString('utf-8'); + const parsed = JSON.parse(raw) as ReviewState; + if (parsed?.version !== 1 || !Array.isArray(parsed.findings)) return null; + return parsed; + } catch { + return null; + } +} + +export function buildReviewState(pr: PRContext, findings: ReviewFinding[]): ReviewState { + return { + version: 1, + headSha: pr.metadata.headSha, + reviewedAt: new Date().toISOString(), + findings: findings.map(toStoredFinding), + }; +} + +/* ------------------------------------------------------------------ */ +/* Summary enrichment */ +/* ------------------------------------------------------------------ */ + +function categoryLabel(finding: Pick): string { + if (finding.category === 'bug') return 'Bug'; + if (finding.severity === 'informational') return 'Quality'; + return 'Flag'; +} + +export function buildReviewNarrative( + pr: PRContext, + diff: ReviewDiff, +): string { + const parts: string[] = []; + + const title = pr.metadata.title?.trim(); + if (title) parts.push(title); + + const bodyFirstLine = pr.metadata.body + ?.split('\n') + .map((line) => line.trim()) + .find((line) => line.length > 0); + if (bodyFirstLine && bodyFirstLine !== title) { + parts.push(bodyFirstLine); + } + + if (diff.resolved.length > 0 && diff.open.length === 0) { + parts.push( + `Resolved ${diff.resolved.length} previous finding${diff.resolved.length === 1 ? '' : 's'}; no remaining open findings.`, + ); + } else if (diff.resolved.length > 0) { + parts.push( + `Resolved ${diff.resolved.length} previous finding${diff.resolved.length === 1 ? '' : 's'}; ${diff.open.length} still open.`, + ); + } else if (diff.open.length === 0) { + parts.push('No issues found in the latest changes.'); + } else if (diff.new.length > 0 && diff.unchanged.length === 0) { + parts.push(`${diff.open.length} finding${diff.open.length === 1 ? '' : 's'} to review.`); + } else if (diff.new.length > 0) { + parts.push( + `${diff.new.length} new finding${diff.new.length === 1 ? '' : 's'}; ${diff.unchanged.length} still open from earlier review.`, + ); + } else { + parts.push(`${diff.open.length} finding${diff.open.length === 1 ? '' : 's'} still need attention.`); + } + + return parts.join(' '); +} + +export function formatFindingListItem(finding: Pick): string { + return `- **${categoryLabel(finding)}**: ${finding.title}`; +} + +export function enrichReviewSummary( + pr: PRContext, + findings: ReviewFinding[], + base: ReviewSummary, + previousState: ReviewState | null, +): ReviewSummary { + const previousFindings = previousState?.findings ?? []; + const diff = diffReviewFindings(previousFindings, findings); + const actionableOpen = diff.open.filter((f) => f.severity !== 'informational'); + + const findingsBySeverity: ReviewSummary['findingsBySeverity'] = { + severe: 0, + 'non-severe': 0, + investigate: 0, + informational: 0, + }; + for (const finding of diff.open) { + findingsBySeverity[finding.severity]++; + } + + return { + ...base, + findingsBySeverity, + totalFindings: diff.open.length, + openFindings: diff.open, + resolvedFindings: diff.resolved, + newFindings: diff.new, + narrative: buildReviewNarrative(pr, diff), + approved: actionableOpen.length === 0, + headSha: pr.metadata.headSha, + resolvedCount: diff.resolved.length, + newCount: diff.new.length, + }; +} diff --git a/core/src/review/reviewable-files.ts b/core/src/review/reviewable-files.ts new file mode 100644 index 0000000..0ec1ea2 --- /dev/null +++ b/core/src/review/reviewable-files.ts @@ -0,0 +1,30 @@ +const SKIP_PATTERNS: RegExp[] = [ + /^yarn\.lock$/, + /^pnpm-lock\.yaml$/, + /^package-lock\.json$/, + /\.lock$/, + /\.min\.(js|css)$/, + /\.map$/, + /\.snap$/, + /\.svg$/, + /\.png$/, + /\.jpg$/, + /\.jpeg$/, + /\.gif$/, + /\.ico$/, + /\.woff2?$/, + /\.ttf$/, + /\.eot$/, + /\.pdf$/, + /dist\//, + /\.generated\./, + /vendor\//, +]; + +export function isReviewableFile(filename: string): boolean { + return !SKIP_PATTERNS.some((pattern) => pattern.test(filename)); +} + +export function filterReviewableFiles(files: string[]): string[] { + return files.filter(isReviewableFile).sort(); +} diff --git a/core/src/review/types.ts b/core/src/review/types.ts index 49e56c4..10b8eb9 100644 --- a/core/src/review/types.ts +++ b/core/src/review/types.ts @@ -41,6 +41,27 @@ export interface ReviewSummary { mode: 'fast' | 'rlm'; findingsBySeverity: Record; totalFindings: number; + /** Open findings from the latest review pass. */ + openFindings?: ReviewFinding[]; + /** Findings fixed since the previous review pass. */ + resolvedFindings?: Array<{ + fingerprint: string; + category: FindingCategory; + severity: FindingSeverity; + file: string; + startLine: number; + title: string; + }>; + /** Findings that appeared only in this pass. */ + newFindings?: ReviewFinding[]; + /** Short narrative shown under the review header. */ + narrative?: string; + /** True when no severe/non-severe/investigate findings remain open. */ + approved?: boolean; + /** Head commit SHA for this review pass. */ + headSha?: string; + resolvedCount?: number; + newCount?: number; } /** Diagnostic trace for observability into the review pipeline. */ diff --git a/service/.env.example b/service/.env.example index b4f2430..0aad8b2 100644 --- a/service/.env.example +++ b/service/.env.example @@ -62,6 +62,15 @@ JOB_BACKOFF_MS=5000 JOB_KEEP_COMPLETED=200 JOB_KEEP_FAILED=500 +# ----------------------------------------------------------------------------- +# Optional — Review result cache (Redis) +# ----------------------------------------------------------------------------- +# Reuse identical review findings when another PR has the same reviewable diff. +# Requires REDIS_URL (same instance as BullMQ). Set to "false" to disable. +REVIEW_CACHE_ENABLED=true +# TTL in seconds (default 7 days). +REVIEW_CACHE_TTL_SECONDS=604800 + # ----------------------------------------------------------------------------- # Optional — Coverage Service integration (opt-in) # ----------------------------------------------------------------------------- diff --git a/service/src/config.ts b/service/src/config.ts index d61eadc..834a88d 100644 --- a/service/src/config.ts +++ b/service/src/config.ts @@ -80,6 +80,12 @@ const ConfigSchema = z.object({ coverageServiceCoverageCommand: stringOrEmpty, coverageServiceTestCommand: stringOrEmpty, coverageServiceInstallCommand: stringOrEmpty, + + reviewCacheEnabled: z + .string() + .optional() + .transform((raw) => raw?.toLowerCase() !== 'false'), + reviewCacheTtlSeconds: numberFromEnv(604_800), // 7 days }); export type ServiceConfig = z.infer & { @@ -126,6 +132,9 @@ export function loadServiceConfig(): ServiceConfig { coverageServiceCoverageCommand: process.env.COVERAGE_SERVICE_COVERAGE_COMMAND, coverageServiceTestCommand: process.env.COVERAGE_SERVICE_TEST_COMMAND, coverageServiceInstallCommand: process.env.COVERAGE_SERVICE_INSTALL_COMMAND, + + reviewCacheEnabled: process.env.REVIEW_CACHE_ENABLED, + reviewCacheTtlSeconds: process.env.REVIEW_CACHE_TTL_SECONDS, }); return { diff --git a/service/src/dispatch/coverage/summary.test.ts b/service/src/dispatch/coverage/summary.test.ts index aac82b1..7323ff0 100644 --- a/service/src/dispatch/coverage/summary.test.ts +++ b/service/src/dispatch/coverage/summary.test.ts @@ -34,13 +34,26 @@ describe('buildOriginalPRComment', () => { it('includes the test PR link when files were generated', () => { const out = buildOriginalPRComment({ run: baseRun, + prTitle: 'feat: add array helpers', testPrUrl: 'https://github.com/kenil27/band/pull/99', testPrFileCount: 3, }); + expect(out).toContain('**feat: add array helpers**'); expect(out).toContain('Generated **3 test files**'); expect(out).toContain('https://github.com/kenil27/band/pull/99'); }); + it('shows threshold headline when workflow reports threshold met', () => { + const out = buildOriginalPRComment({ + run: { + ...baseRun, + workflowSummary: { thresholdReached: true, targetCoverage: 80 }, + }, + }); + expect(out).toContain('threshold met'); + expect(out).toContain('73.5% diff coverage'); + }); + it('says "no tests" when run completed without files', () => { const out = buildOriginalPRComment({ run: baseRun, diff --git a/service/src/dispatch/coverage/summary.ts b/service/src/dispatch/coverage/summary.ts index beda4e5..a0786cf 100644 --- a/service/src/dispatch/coverage/summary.ts +++ b/service/src/dispatch/coverage/summary.ts @@ -16,6 +16,8 @@ import type { PrRun } from './types.js'; export interface SummaryInput { run: PrRun; + /** PR title — used for context when the description is empty. */ + prTitle?: string; /** URL of the stacked test PR — omitted when no tests were generated. */ testPrUrl?: string | null; /** Number of files included in the stacked PR (may be 0). */ @@ -45,6 +47,15 @@ export function buildOriginalPRComment(input: SummaryInput): string { '', ]; + const headline = buildCoverageHeadline(run); + if (headline) { + lines.push(headline, ''); + } + + if (input.prTitle?.trim()) { + lines.push(`**${input.prTitle.trim()}**`, ''); + } + const diffRow = buildDiffCoverageTable(run); if (diffRow) { lines.push(diffRow, ''); @@ -124,6 +135,28 @@ function describeStatus(status: PrRun['status']): string { } } +function buildCoverageHeadline(run: PrRun): string | null { + const diffAfter = run.diffCoverageAfter; + if (diffAfter == null) return null; + + const workflow = run.workflowSummary as + | { thresholdReached?: boolean; targetCoverage?: number } + | null + | undefined; + const thresholdMet = workflow?.thresholdReached === true; + const target = workflow?.targetCoverage; + + if (thresholdMet) { + return `✅ **${diffAfter.toFixed(1)}% diff coverage** · threshold met${target != null ? ` (${target}%)` : ''}`; + } + + if (target != null && diffAfter < target) { + return `⚠️ **${diffAfter.toFixed(1)}% diff coverage** · below ${target}% target`; + } + + return `**${diffAfter.toFixed(1)}% diff coverage**`; +} + function buildDiffCoverageTable(run: PrRun): string | null { if (run.diffCoverageBefore == null && run.diffCoverageAfter == null) return null; diff --git a/service/src/jobs/processors/coverage-analysis.test.ts b/service/src/jobs/processors/coverage-analysis.test.ts index 656c894..4d42e83 100644 --- a/service/src/jobs/processors/coverage-analysis.test.ts +++ b/service/src/jobs/processors/coverage-analysis.test.ts @@ -85,6 +85,7 @@ function makeLogger(): Logger { function makeRuntime() { const poster = { postAcknowledgement: vi.fn(async () => {}), + postCoverageComment: vi.fn(async () => {}), } as unknown as CommentPoster; const client = {} as unknown as GitHubClient; const pr = {} as PRContext; @@ -183,9 +184,10 @@ describe('processCoverageAnalysis', () => { }), ); - // 1 ack + 1 summary - expect(poster.postAcknowledgement).toHaveBeenCalledTimes(2); - const summary = (poster.postAcknowledgement as unknown as ReturnType).mock.calls[1][1] as string; + // 1 ack + 1 coverage summary (updated in place on later pushes) + expect(poster.postAcknowledgement).toHaveBeenCalledTimes(1); + expect(poster.postCoverageComment).toHaveBeenCalledTimes(1); + const summary = (poster.postCoverageComment as unknown as ReturnType).mock.calls[0][1] as string; expect(summary).toContain('Diff coverage'); expect(summary).toContain('https://github.com/kenil27/band/pull/99'); }); diff --git a/service/src/jobs/processors/coverage-analysis.ts b/service/src/jobs/processors/coverage-analysis.ts index 833fe23..839fa33 100644 --- a/service/src/jobs/processors/coverage-analysis.ts +++ b/service/src/jobs/processors/coverage-analysis.ts @@ -71,21 +71,12 @@ export async function processCoverageAnalysis( prNumber: job.prNumber, }); - log.info({ prRunId: job.prRunId }, 'starting coverage analysis'); - const { client, poster } = await buildPRRuntime(job, deps.auth); const coverage = (deps.coverageClientFactory ?? defaultCoverageClientFactory)( deps, ); - // Only post the "started" ack on the first attempt — repeated comments are - // annoying when BullMQ retries the job. - if (!job.prRunId) { - await poster.postAcknowledgement( - job.prNumber, - '[INFO] **OpenReview** — Coverage analysis started. A stacked PR with the generated tests will be opened on completion.', - ); - } + // ------------------------------------------------------------------ // // 1) Ensure repository is registered // @@ -164,15 +155,26 @@ export async function processCoverageAnalysis( } // ------------------------------------------------------------------ // - // 6) Summary comment on the original PR // + // 6) Summary comment on the original PR (posted last so it stays at // + // the bottom of the timeline after the stacked-test commit). // // ------------------------------------------------------------------ // const summary = buildOriginalPRComment({ run, + prTitle: job.title, testPrUrl, testPrFileCount, }); - await poster.postAcknowledgement(job.prNumber, summary); + await poster.postCoverageComment(job.prNumber, summary); + log.info( + { + prRunId, + diffCoverageAfter: run.diffCoverageAfter, + testPrUrl, + testPrFileCount, + }, + 'coverage summary comment updated', + ); if (run.status === 'FAILED') { // Throw so BullMQ records the attempt as failed — useful in the dashboard. diff --git a/service/src/jobs/processors/review.ts b/service/src/jobs/processors/review.ts index 50bcd93..d64839c 100644 --- a/service/src/jobs/processors/review.ts +++ b/service/src/jobs/processors/review.ts @@ -1,18 +1,31 @@ -import { runFastReview } from '@openreview/core'; +import { + buildFastReviewSummary, + fingerprintPullRequestDiff, + runFastReview, +} from '@openreview/core'; +import type { ServiceConfig } from '../../config.js'; import type { GitHubAuth } from '../../github/auth.js'; import type { Logger } from '../../logger.js'; +import type { ReviewCache } from '../../review/review-cache.js'; import type { FastReviewJob } from '../types.js'; import { buildPRRuntime } from './context.js'; +export interface FastReviewDeps { + auth: GitHubAuth; + logger: Logger; + reviewCache: ReviewCache; + cfg: ServiceConfig; +} + /** * Process a fast review job: fetch PR data, run runFastReview, post results. - * Mirrors what the GitHub Action does in action/src/pr-handler.ts. + * Reuses cached findings when an identical reviewable diff was seen before. */ export async function processFastReview( job: FastReviewJob, - deps: { auth: GitHubAuth; logger: Logger }, + deps: FastReviewDeps, ): Promise { const log = deps.logger.child({ job: 'review-fast', @@ -23,23 +36,40 @@ export async function processFastReview( log.info('starting fast review'); const { poster, pr } = await buildPRRuntime(job, deps.auth); - - await poster.postAcknowledgement( - job.prNumber, - '[INFO] **OpenReview** — Review started... results will appear shortly.', - ); + const fingerprint = fingerprintPullRequestDiff(pr.diff, pr.files); try { - const { findings, summary } = await runFastReview(pr); + const cached = await deps.reviewCache.get(job.owner, job.repo, fingerprint); + let findings; + let summary; + let fromCache = false; - if (findings.length > 0) { - await poster.postReview(job.prNumber, findings); + if (cached) { + findings = cached.findings; + summary = buildFastReviewSummary(findings, pr.files.length, 0); + fromCache = true; + } else { + const result = await runFastReview(pr); + findings = result.findings; + summary = result.summary; + await deps.reviewCache.set(job.owner, job.repo, fingerprint, findings); } - await poster.postSummaryComment(job.prNumber, summary); + + const finalSummary = await poster.postReviewResults(job.prNumber, pr, findings, summary); log.info( - { findings: findings.length, duration: summary.duration }, - 'fast review complete', + { + findings: findings.length, + newFindings: finalSummary.newCount ?? findings.length, + resolved: finalSummary.resolvedCount ?? 0, + approved: finalSummary.approved ?? false, + duration: finalSummary.duration, + fromCache, + diffFingerprint: fingerprint.slice(0, 12), + }, + fromCache + ? 'fast review complete (cache hit) — summary comment updated' + : 'fast review complete — summary comment updated', ); } catch (err) { const msg = err instanceof Error ? err.message : String(err); diff --git a/service/src/review/review-cache.ts b/service/src/review/review-cache.ts new file mode 100644 index 0000000..b3896bf --- /dev/null +++ b/service/src/review/review-cache.ts @@ -0,0 +1,88 @@ +import { config as coreConfig } from '@openreview/core'; +import type { ReviewFinding } from '@openreview/core'; +import type { Redis } from 'ioredis'; + +import type { ServiceConfig } from '../config.js'; +import type { Logger } from '../logger.js'; + +const CACHE_VERSION = 'v1'; + +export interface CachedReviewResult { + findings: ReviewFinding[]; + model: string; + cachedAt: string; +} + +export class ReviewCache { + constructor( + private readonly redis: Redis, + private readonly cfg: ServiceConfig, + private readonly log: Logger, + ) {} + + isEnabled(): boolean { + return this.cfg.reviewCacheEnabled; + } + + async get( + owner: string, + repo: string, + fingerprint: string, + ): Promise { + if (!this.isEnabled()) return null; + + const key = this.cacheKey(owner, repo, fingerprint); + try { + const raw = await this.redis.get(key); + if (!raw) return null; + + const parsed = JSON.parse(raw) as CachedReviewResult; + if (!Array.isArray(parsed.findings)) return null; + + this.log.info( + { fingerprint: fingerprint.slice(0, 12), model: parsed.model }, + 'review cache hit', + ); + return parsed; + } catch (err) { + this.log.warn( + { err: (err as Error).message, fingerprint: fingerprint.slice(0, 12) }, + 'review cache read failed', + ); + return null; + } + } + + async set( + owner: string, + repo: string, + fingerprint: string, + findings: ReviewFinding[], + ): Promise { + if (!this.isEnabled()) return; + + const payload: CachedReviewResult = { + findings, + model: coreConfig.mainModel, + cachedAt: new Date().toISOString(), + }; + + const key = this.cacheKey(owner, repo, fingerprint); + try { + await this.redis.set(key, JSON.stringify(payload), 'EX', this.cfg.reviewCacheTtlSeconds); + this.log.info( + { fingerprint: fingerprint.slice(0, 12), findings: findings.length }, + 'review cache stored', + ); + } catch (err) { + this.log.warn( + { err: (err as Error).message, fingerprint: fingerprint.slice(0, 12) }, + 'review cache write failed', + ); + } + } + + private cacheKey(owner: string, repo: string, fingerprint: string): string { + return `openreview:review-cache:${CACHE_VERSION}:${owner}/${repo}:${coreConfig.mainModel}:${fingerprint}`; + } +} diff --git a/service/src/worker.ts b/service/src/worker.ts index 63a5fa7..9878376 100644 --- a/service/src/worker.ts +++ b/service/src/worker.ts @@ -17,6 +17,7 @@ import { processRlmReview } from './jobs/processors/rlm.js'; import { QUEUE_NAME } from './jobs/types.js'; import type { CoverageAnalysisJob, OpenReviewJob } from './jobs/types.js'; import { createLogger } from './logger.js'; +import { ReviewCache } from './review/review-cache.js'; /** * Worker entrypoint. Pulls jobs from BullMQ and runs the corresponding @@ -38,6 +39,7 @@ async function main(): Promise { const auth = createPatAuth(cfg); const connection = createRedisConnection(cfg); + const reviewCache = new ReviewCache(connection, cfg, logger); const worker = new Worker( QUEUE_NAME, @@ -45,7 +47,7 @@ async function main(): Promise { const data = job.data; switch (data.kind) { case 'review-fast': - return processFastReview(data, { auth, logger }); + return processFastReview(data, { auth, logger, reviewCache, cfg }); case 'review-rlm': return processRlmReview(data, { auth, logger }); case 'chat': diff --git a/tests/core/github/comments.test.ts b/tests/core/github/comments.test.ts index 48c0dbb..1338af2 100644 --- a/tests/core/github/comments.test.ts +++ b/tests/core/github/comments.test.ts @@ -2,18 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { GitHubClient } from '../../../core/src/github/client.js'; import { CommentPoster } from '../../../core/src/github/comments.js'; -import type { ReviewFinding, ReviewSummary } from '../../../core/src/review/types.js'; - -vi.mock('../../../core/src/review/formatter.js', () => ({ - formatInlineComment: vi.fn((f: ReviewFinding) => `inline:${f.id}`), - formatSummaryComment: vi.fn( - (s: ReviewSummary) => `\nsummary:${s.totalFindings}`, - ), -})); - -/* ------------------------------------------------------------------ */ -/* Helpers */ -/* ------------------------------------------------------------------ */ +import type { PRContext, ReviewFinding, ReviewSummary } from '../../../core/src/review/types.js'; function makeMockClient() { const api = { @@ -31,7 +20,7 @@ function makeMockClient() { return { client, api }; } -function makeFinding(id: string, file = 'src/index.ts', line = 10): ReviewFinding { +function makeFinding(id: string, file = 'src/index.ts', line = 10, title = `Finding ${id}`): ReviewFinding { return { id, category: 'bug', @@ -39,178 +28,220 @@ function makeFinding(id: string, file = 'src/index.ts', line = 10): ReviewFindin file, startLine: line, endLine: line, - title: `Finding ${id}`, + title, explanation: 'Test explanation', source: 'ai', citations: [], }; } -function makeSummary(totalFindings = 3): ReviewSummary { +function makeSummary(totalFindings = 1): ReviewSummary { return { filesReviewed: 5, duration: '10s', mode: 'fast', - findingsBySeverity: { severe: 1, 'non-severe': 1, investigate: 1, informational: 0 }, + findingsBySeverity: { severe: totalFindings, 'non-severe': 0, investigate: 0, informational: 0 }, totalFindings, }; } -/* ------------------------------------------------------------------ */ -/* postReview */ -/* ------------------------------------------------------------------ */ +function makePr(): PRContext { + return { + owner: 'test-owner', + repo: 'test-repo', + prNumber: 7, + diff: '', + files: ['src/index.ts'], + metadata: { + title: 'Test PR', + body: 'Test body', + headSha: 'head-sha-2', + baseSha: 'base-sha', + author: 'dev', + }, + instructions: '', + learnings: [], + }; +} + +function encodedState(findings: ReviewFinding[]) { + const state = { + version: 1, + headSha: 'head-sha-1', + reviewedAt: '2026-01-01T00:00:00.000Z', + findings: findings.map((f) => ({ + fingerprint: `${f.file}:${f.startLine}:${f.title.toLowerCase()}`, + category: f.category, + severity: f.severity, + file: f.file, + startLine: f.startLine, + title: f.title, + })), + }; + const payload = Buffer.from(JSON.stringify(state), 'utf-8').toString('base64url'); + return `\n`; +} -describe('postReview', () => { +describe('postReviewResults', () => { let poster: CommentPoster; let api: ReturnType['api']; beforeEach(() => { vi.clearAllMocks(); - ({ client: poster, api } = (() => { - const m = makeMockClient(); - return { client: new CommentPoster(m.client), api: m.api }; - })()); + const m = makeMockClient(); + poster = new CommentPoster(m.client); + api = m.api; }); - it('posts batch review with correct endpoint and body', async () => { - const findings = [makeFinding('f1', 'a.ts', 5), makeFinding('f2', 'b.ts', 12)]; + it('posts only new inline comments on a follow-up commit', async () => { + const previousFinding = makeFinding('old', 'src/a.ts', 5, 'Old issue'); + const newFinding = makeFinding('new', 'src/b.ts', 12, 'New issue'); + + api.get.mockResolvedValue({ + data: [{ id: 200, body: encodedState([previousFinding]) }], + }); api.post.mockResolvedValue({ data: {} }); + api.patch.mockResolvedValue({ data: {} }); - await poster.postReview(42, findings); + const summary = await poster.postReviewResults( + 7, + makePr(), + [newFinding], + makeSummary(1), + ); expect(api.post).toHaveBeenCalledOnce(); - const [url, body] = api.post.mock.calls[0]; - expect(url).toBe('/repos/test-owner/test-repo/pulls/42/reviews'); - expect(body.event).toBe('COMMENT'); - expect(body.comments).toHaveLength(2); - expect(body.comments[0]).toEqual({ - path: 'a.ts', - line: 5, - side: 'RIGHT', - body: 'inline:f1', - }); - expect(body.comments[1]).toEqual({ - path: 'b.ts', - line: 12, - side: 'RIGHT', - body: 'inline:f2', - }); - }); - - it('skips when findings is empty', async () => { - await poster.postReview(42, []); + const [reviewUrl, reviewBody] = api.post.mock.calls[0]; + expect(reviewUrl).toContain('/pulls/7/reviews'); + expect(reviewBody.comments).toHaveLength(1); + expect(reviewBody.comments[0].path).toBe('src/b.ts'); - expect(api.post).not.toHaveBeenCalled(); + expect(summary.resolvedCount).toBe(1); + expect(summary.newCount).toBe(1); + expect(api.patch).toHaveBeenCalledOnce(); }); -}); -/* ------------------------------------------------------------------ */ -/* postSummaryComment */ -/* ------------------------------------------------------------------ */ + it('skips inline review when all findings are unchanged', async () => { + const finding = makeFinding('same', 'src/a.ts', 5, 'Same issue'); -describe('postSummaryComment', () => { - let poster: CommentPoster; - let api: ReturnType['api']; + api.get.mockResolvedValue({ + data: [{ id: 200, body: encodedState([finding]) }], + }); + api.patch.mockResolvedValue({ data: {} }); - beforeEach(() => { - vi.clearAllMocks(); - const m = makeMockClient(); - poster = new CommentPoster(m.client); - api = m.api; + const summary = await poster.postReviewResults(7, makePr(), [finding], makeSummary(1)); + + expect(api.post).not.toHaveBeenCalled(); + expect(summary.newCount).toBe(0); + expect(summary.resolvedCount).toBe(0); + expect(api.patch).toHaveBeenCalledOnce(); }); +}); - it('creates new comment when no existing summary', async () => { - // findSummaryComment returns null (no marker found, single page with < 100 results) +describe('postCoverageComment', () => { + it('creates new comment when no existing coverage marker', async () => { + const { client, api } = makeMockClient(); + const poster = new CommentPoster(client); api.get.mockResolvedValue({ data: [] }); api.post.mockResolvedValue({ data: {} }); - await poster.postSummaryComment(7, makeSummary()); + await poster.postCoverageComment(7, 'Coverage done'); expect(api.post).toHaveBeenCalledOnce(); - const [url, body] = api.post.mock.calls[0]; - expect(url).toBe('/repos/test-owner/test-repo/issues/7/comments'); - expect(body.body).toContain(''); + expect(api.post.mock.calls[0][1].body).toContain(''); }); - it('updates existing comment when marker found', async () => { - api.get.mockResolvedValue({ - data: [ - { id: 100, body: 'unrelated comment' }, - { id: 200, body: '\nold summary' }, - ], - }); + it('updates existing coverage comment when it is already the latest timeline comment', async () => { + const { client, api } = makeMockClient(); + const poster = new CommentPoster(client); + api.get + .mockResolvedValueOnce({ + data: [{ id: 300, body: '\nold' }], + }) + .mockResolvedValueOnce({ + data: [{ id: 300, body: '\nold' }], + }); api.patch.mockResolvedValue({ data: {} }); - await poster.postSummaryComment(7, makeSummary(5)); + await poster.postCoverageComment(7, '\nnew'); - expect(api.patch).toHaveBeenCalledOnce(); - const [url, body] = api.patch.mock.calls[0]; - expect(url).toBe('/repos/test-owner/test-repo/issues/comments/200'); - expect(body.body).toContain(''); + expect(api.patch).toHaveBeenCalledWith( + '/repos/test-owner/test-repo/issues/comments/300', + expect.objectContaining({ body: expect.stringContaining('new') }), + ); expect(api.post).not.toHaveBeenCalled(); }); - it('paginates to find existing marker comment', async () => { - // First page: 100 comments, none with marker - const page1 = Array.from({ length: 100 }, (_, i) => ({ - id: i + 1, - body: `comment ${i}`, - })); - // Second page: fewer than 100, one has the marker - const page2 = [ - { id: 201, body: 'another comment' }, - { id: 202, body: '\nold' }, - ]; - - api.get.mockResolvedValueOnce({ data: page1 }).mockResolvedValueOnce({ data: page2 }); - api.patch.mockResolvedValue({ data: {} }); + it('posts a new coverage comment when a newer timeline comment exists', async () => { + const { client, api } = makeMockClient(); + const poster = new CommentPoster(client); + api.get + .mockResolvedValueOnce({ + data: [{ id: 400, body: 'some other bot comment' }], + }) + .mockResolvedValueOnce({ + data: [{ id: 300, body: '\nold' }], + }); + api.post.mockResolvedValue({ data: {} }); - await poster.postSummaryComment(7, makeSummary()); + await poster.postCoverageComment(7, '\nnew'); - expect(api.get).toHaveBeenCalledTimes(2); - expect(api.get.mock.calls[0][1]).toEqual({ params: { per_page: 100, page: 1 } }); - expect(api.get.mock.calls[1][1]).toEqual({ params: { per_page: 100, page: 2 } }); - expect(api.patch).toHaveBeenCalledWith( - '/repos/test-owner/test-repo/issues/comments/202', - expect.objectContaining({ body: expect.stringContaining('') }), - ); + expect(api.post).toHaveBeenCalledOnce(); + expect(api.patch).not.toHaveBeenCalled(); }); }); -/* ------------------------------------------------------------------ */ -/* postChatReply */ -/* ------------------------------------------------------------------ */ - -describe('postChatReply', () => { - it('posts to correct /issues/{prNumber}/comments endpoint', async () => { +describe('postSummaryComment', () => { + it('creates new comment when no existing summary', async () => { const { client, api } = makeMockClient(); const poster = new CommentPoster(client); + api.get.mockResolvedValue({ data: [] }); api.post.mockResolvedValue({ data: {} }); - await poster.postChatReply(99, 'Here is my reply'); + await poster.postSummaryComment(7, makeSummary()); expect(api.post).toHaveBeenCalledOnce(); - const [url, body] = api.post.mock.calls[0]; - expect(url).toBe('/repos/test-owner/test-repo/issues/99/comments'); - expect(body).toEqual({ body: 'Here is my reply' }); + expect(api.post.mock.calls[0][1].body).toContain(''); }); -}); -/* ------------------------------------------------------------------ */ -/* postAcknowledgement */ -/* ------------------------------------------------------------------ */ + it('updates the newest summary when it is already the latest timeline comment', async () => { + const { client, api } = makeMockClient(); + const poster = new CommentPoster(client); + api.get + .mockResolvedValueOnce({ + data: [{ id: 300, body: '\nnewest' }], + }) + .mockResolvedValueOnce({ + data: [{ id: 300, body: '\nnewest' }], + }); + api.patch.mockResolvedValue({ data: {} }); + + await poster.postSummaryComment(7, makeSummary(2)); + + expect(api.patch).toHaveBeenCalledWith( + '/repos/test-owner/test-repo/issues/comments/300', + expect.objectContaining({ body: expect.stringContaining('') }), + ); + expect(api.post).not.toHaveBeenCalled(); + }); -describe('postAcknowledgement', () => { - it('posts standalone comment', async () => { + it('posts a new summary when newer timeline comments exist', async () => { const { client, api } = makeMockClient(); const poster = new CommentPoster(client); + api.get + .mockResolvedValueOnce({ + data: [{ id: 400, body: 'Review started...' }], + }) + .mockResolvedValueOnce({ + data: [ + { id: 300, body: '\nolder summary' }, + ], + }); api.post.mockResolvedValue({ data: {} }); - await poster.postAcknowledgement(15, 'Got it, reviewing now...'); + await poster.postSummaryComment(7, makeSummary(2)); expect(api.post).toHaveBeenCalledOnce(); - const [url, body] = api.post.mock.calls[0]; - expect(url).toBe('/repos/test-owner/test-repo/issues/15/comments'); - expect(body).toEqual({ body: 'Got it, reviewing now...' }); + expect(api.patch).not.toHaveBeenCalled(); }); }); diff --git a/tests/core/review/diff-fingerprint.test.ts b/tests/core/review/diff-fingerprint.test.ts new file mode 100644 index 0000000..5ef8a5a --- /dev/null +++ b/tests/core/review/diff-fingerprint.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from 'vitest'; + +import { + extractDiffForFiles, + fingerprintPullRequestDiff, + normalizeDiffForFingerprint, +} from '../../../core/src/review/diff-fingerprint.js'; + +const SAMPLE_DIFF = `diff --git a/src/utils/mathUtils.js b/src/utils/mathUtils.js +index 1111111..2222222 100644 +--- a/src/utils/mathUtils.js ++++ b/src/utils/mathUtils.js +@@ -1,3 +1,7 @@ + export const add = (a, b) => { + return a + b; + }; ++ ++export const clamp = (val, min, max) => { ++ return Math.min(Math.max(val, min), max); ++}; +`; + +describe('fingerprintPullRequestDiff', () => { + it('returns the same hash for identical diffs and files', () => { + const files = ['src/utils/mathUtils.js']; + const a = fingerprintPullRequestDiff(SAMPLE_DIFF, files); + const b = fingerprintPullRequestDiff(SAMPLE_DIFF, files); + expect(a).toBe(b); + }); + + it('returns the same hash when file list order differs', () => { + const a = fingerprintPullRequestDiff(SAMPLE_DIFF, ['src/utils/mathUtils.js', 'src/other.js']); + const b = fingerprintPullRequestDiff(SAMPLE_DIFF, ['src/other.js', 'src/utils/mathUtils.js']); + expect(a).toBe(b); + }); + + it('returns different hashes for different patch content', () => { + const other = SAMPLE_DIFF.replace('clamp', 'round'); + const a = fingerprintPullRequestDiff(SAMPLE_DIFF, ['src/utils/mathUtils.js']); + const b = fingerprintPullRequestDiff(other, ['src/utils/mathUtils.js']); + expect(a).not.toBe(b); + }); + + it('ignores non-reviewable files in the fingerprint', () => { + const withLock = fingerprintPullRequestDiff(SAMPLE_DIFF, [ + 'src/utils/mathUtils.js', + 'package-lock.json', + ]); + const withoutLock = fingerprintPullRequestDiff(SAMPLE_DIFF, ['src/utils/mathUtils.js']); + expect(withLock).toBe(withoutLock); + }); +}); + +describe('normalizeDiffForFingerprint', () => { + it('strips hunk line numbers', () => { + const normalized = normalizeDiffForFingerprint('@@ -1,3 +1,7 @@\n+foo'); + expect(normalized).toContain('@@\n+foo'); + expect(normalized).not.toContain('+1,7'); + }); +}); + +describe('extractDiffForFiles', () => { + it('keeps only requested file sections', () => { + const multi = `${SAMPLE_DIFF}\ndiff --git a/src/other.js b/src/other.js\n+++ b/src/other.js\n`; + const extracted = extractDiffForFiles(multi, ['src/utils/mathUtils.js']); + expect(extracted).toContain('mathUtils.js'); + expect(extracted).not.toContain('other.js'); + }); +}); diff --git a/tests/core/review/formatter.test.ts b/tests/core/review/formatter.test.ts index 09aff78..e084853 100644 --- a/tests/core/review/formatter.test.ts +++ b/tests/core/review/formatter.test.ts @@ -1,12 +1,9 @@ import { describe, expect, it } from 'vitest'; import { formatInlineComment, formatSummaryComment } from '../../../core/src/review/formatter.js'; +import { embedReviewState } from '../../../core/src/review/review-state.js'; import type { ReviewFinding, ReviewSummary } from '../../../core/src/review/types.js'; -/* ------------------------------------------------------------------ */ -/* Helper */ -/* ------------------------------------------------------------------ */ - function makeFinding(overrides: Partial = {}): ReviewFinding { return { id: 'test-1', @@ -34,31 +31,12 @@ function makeSummary(overrides: Partial = {}): ReviewSummary { }; } -/* ------------------------------------------------------------------ */ -/* formatInlineComment — exhaustive */ -/* ------------------------------------------------------------------ */ - describe('formatInlineComment', () => { it('renders severe bug badge', () => { const result = formatInlineComment(makeFinding({ severity: 'severe' })); expect(result).toContain('[CRITICAL] **Bug — Severe**'); }); - it('renders non-severe bug badge', () => { - const result = formatInlineComment(makeFinding({ severity: 'non-severe' })); - expect(result).toContain('[MODERATE] **Bug — Non-severe**'); - }); - - it('renders investigate flag badge', () => { - const result = formatInlineComment(makeFinding({ severity: 'investigate' })); - expect(result).toContain('[FLAG] **Investigate**'); - }); - - it('renders informational flag badge', () => { - const result = formatInlineComment(makeFinding({ severity: 'informational' })); - expect(result).toContain('[INFO] **Informational**'); - }); - it('includes title and explanation', () => { const result = formatInlineComment( makeFinding({ title: 'My Title', explanation: 'My Explanation' }), @@ -66,175 +44,71 @@ describe('formatInlineComment', () => { expect(result).toContain('My Title'); expect(result).toContain('My Explanation'); }); - - it('includes suggested fix with GitHub suggestion syntax', () => { - const result = formatInlineComment(makeFinding({ suggestedFix: 'const x = 2;' })); - expect(result).toContain('```suggestion'); - expect(result).toContain('const x = 2;'); - expect(result).toContain('**Suggested fix:**'); - }); - - it('omits suggested fix section when not provided', () => { - const result = formatInlineComment(makeFinding({ suggestedFix: undefined })); - expect(result).not.toContain('Suggested fix'); - expect(result).not.toContain('```suggestion'); - }); - - it('shows linter attribution for linter source', () => { - const result = formatInlineComment(makeFinding({ source: 'linter', linterName: 'ESLint' })); - expect(result).toContain('> Source: ESLint'); - }); - - it('shows combined attribution for both source', () => { - const result = formatInlineComment(makeFinding({ source: 'both', linterName: 'Ruff' })); - expect(result).toContain('> Source: AI + Ruff'); - }); - - it('falls back to "linter" when linterName is undefined for linter source', () => { - const result = formatInlineComment(makeFinding({ source: 'linter', linterName: undefined })); - expect(result).toContain('> Source: linter'); - }); - - it('does not show source attribution for AI-only findings', () => { - const result = formatInlineComment(makeFinding({ source: 'ai' })); - expect(result).not.toContain('Source:'); - }); - - it('handles multiline suggested fix', () => { - const fix = 'if (x) {\n return true;\n}'; - const result = formatInlineComment(makeFinding({ suggestedFix: fix })); - expect(result).toContain(fix); - }); - - it('handles special characters in title and explanation', () => { - const result = formatInlineComment( - makeFinding({ - title: 'Use `??` instead of `||`', - explanation: 'The `||` operator coerces to boolean — use `??` for nullish coalescing.', - }), - ); - expect(result).toContain('`??`'); - expect(result).toContain('`||`'); - }); - - it('handles empty title and explanation', () => { - const result = formatInlineComment(makeFinding({ title: '', explanation: '' })); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); // Badge should still be there - }); }); -/* ------------------------------------------------------------------ */ -/* formatSummaryComment — exhaustive */ -/* ------------------------------------------------------------------ */ - describe('formatSummaryComment', () => { it('always includes the HTML marker', () => { const result = formatSummaryComment(makeSummary()); expect(result).toContain(''); }); - it('always includes the header', () => { - const result = formatSummaryComment(makeSummary()); - expect(result).toContain('## OpenReview Summary'); - }); - - it('shows files reviewed, duration, and mode', () => { - const result = formatSummaryComment( - makeSummary({ filesReviewed: 42, duration: '5m 30s', mode: 'rlm' }), - ); - expect(result).toContain('**Files reviewed:** 42'); - expect(result).toContain('**Duration:** 5m 30s'); - expect(result).toContain('**Mode:** rlm'); - }); - - it('shows no-issues message for zero findings', () => { - const result = formatSummaryComment(makeSummary({ totalFindings: 0 })); - expect(result).toContain('[SUCCESS] No issues found.'); - expect(result).not.toContain('| Severity'); - }); - - it('shows severity table for findings', () => { - const result = formatSummaryComment( - makeSummary({ - totalFindings: 6, - findingsBySeverity: { severe: 1, 'non-severe': 2, investigate: 1, informational: 2 }, - }), - ); - expect(result).toContain('[CRITICAL] **Bug — Severe**'); - expect(result).toContain('[MODERATE] **Bug — Non-severe**'); - expect(result).toContain('[FLAG] **Investigate**'); - expect(result).toContain('[INFO] **Informational**'); - expect(result).toContain('**Total:** 6 findings'); - }); - - it('only shows non-zero severity rows', () => { + it('uses Code Review header with approved badge when approved', () => { const result = formatSummaryComment( makeSummary({ - totalFindings: 1, - findingsBySeverity: { severe: 1, 'non-severe': 0, investigate: 0, informational: 0 }, + approved: true, + totalFindings: 0, + narrative: 'All good.', }), ); - expect(result).toContain('[CRITICAL] **Bug — Severe**'); - expect(result).not.toContain('[MODERATE] **Bug — Non-severe**'); - expect(result).not.toContain('[FLAG] **Investigate**'); - expect(result).not.toContain('[INFO] **Informational**'); + expect(result).toContain('## Code Review'); + expect(result).toContain('✅ **Approved**'); + expect(result).toContain('All good.'); }); - it('uses singular "finding" for count of 1', () => { + it('shows resolved and open collapsible sections', () => { const result = formatSummaryComment( makeSummary({ + approved: false, totalFindings: 1, - findingsBySeverity: { severe: 1, 'non-severe': 0, investigate: 0, informational: 0 }, + resolvedCount: 2, + resolvedFindings: [ + { + fingerprint: 'a', + category: 'bug', + severity: 'non-severe', + file: 'src/a.ts', + startLine: 1, + title: 'Resolved import error', + }, + { + fingerprint: 'b', + category: 'flag', + severity: 'informational', + file: 'src/b.ts', + startLine: 2, + title: 'Duplicated helper', + }, + ], + openFindings: [makeFinding({ title: 'Still broken' })], + narrative: 'Partial fix.', }), ); - expect(result).toContain('**Total:** 1 finding'); - expect(result).not.toContain('1 findings'); - }); - - it('uses plural "findings" for count > 1', () => { - const result = formatSummaryComment( - makeSummary({ - totalFindings: 5, - findingsBySeverity: { severe: 5, 'non-severe': 0, investigate: 0, informational: 0 }, - }), - ); - expect(result).toContain('5 findings'); - }); - - it('includes trigger hints', () => { - const result = formatSummaryComment(makeSummary()); - expect(result).toContain('`@openreview rlm`'); - expect(result).toContain('`@openreview `'); - }); - - it('handles very large counts', () => { - const result = formatSummaryComment( - makeSummary({ - totalFindings: 9999, - filesReviewed: 500, - findingsBySeverity: { severe: 9999, 'non-severe': 0, investigate: 0, informational: 0 }, - }), - ); - expect(result).toContain('9999'); - expect(result).toContain('500'); - }); - - it('severity rows appear in correct order (severe first)', () => { - const result = formatSummaryComment( - makeSummary({ - totalFindings: 4, - findingsBySeverity: { severe: 1, 'non-severe': 1, investigate: 1, informational: 1 }, - }), - ); - - const severeIdx = result.indexOf('[CRITICAL] **Bug — Severe**'); - const nonSevereIdx = result.indexOf('[MODERATE] **Bug — Non-severe**'); - const investigateIdx = result.indexOf('[FLAG] **Investigate**'); - const infoIdx = result.indexOf('[INFO] **Informational**'); - expect(severeIdx).toBeLessThan(nonSevereIdx); - expect(nonSevereIdx).toBeLessThan(investigateIdx); - expect(investigateIdx).toBeLessThan(infoIdx); + expect(result).toContain('✅ 2 resolved'); + expect(result).toContain('**Bug**: Resolved import error'); + expect(result).toContain('**Quality**: Duplicated helper'); + expect(result).toContain('🔎 1 open'); + expect(result).toContain('**Bug**: Still broken'); + }); + + it('embeds review state when provided', () => { + const state = { + version: 1 as const, + headSha: 'abc', + reviewedAt: '2026-01-01T00:00:00.000Z', + findings: [], + }; + const result = formatSummaryComment(makeSummary(), state); + expect(result).toContain(embedReviewState(state)); }); }); diff --git a/tests/core/review/review-state.test.ts b/tests/core/review/review-state.test.ts new file mode 100644 index 0000000..a1e6a9f --- /dev/null +++ b/tests/core/review/review-state.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it } from 'vitest'; + +import { + diffReviewFindings, + embedReviewState, + enrichReviewSummary, + fingerprintFinding, + findingsMatch, + normalizeFindingTitle, + parseReviewState, + toStoredFinding, +} from '../../../core/src/review/review-state.js'; +import type { PRContext, ReviewFinding, ReviewSummary } from '../../../core/src/review/types.js'; + +function makeFinding(overrides: Partial = {}): ReviewFinding { + return { + id: 'f-1', + category: 'bug', + severity: 'non-severe', + file: 'src/foo.ts', + startLine: 10, + endLine: 10, + title: 'Missing null check', + explanation: 'x can be null', + source: 'ai', + citations: [], + ...overrides, + }; +} + +function makePr(overrides: Partial = {}): PRContext { + return { + owner: 'acme', + repo: 'app', + prNumber: 1, + diff: '', + files: ['src/foo.ts'], + metadata: { + title: 'Add fixture deletion', + body: 'Implements round deletion for tournaments.', + headSha: 'abc123', + baseSha: 'def456', + author: 'dev', + }, + instructions: '', + learnings: [], + ...overrides, + }; +} + +function makeSummary(): ReviewSummary { + return { + filesReviewed: 3, + duration: '8s', + mode: 'fast', + findingsBySeverity: { severe: 0, 'non-severe': 1, investigate: 0, informational: 0 }, + totalFindings: 1, + }; +} + +describe('fingerprintFinding', () => { + it('normalizes title whitespace and case', () => { + const a = fingerprintFinding(makeFinding({ title: ' Missing Null Check ' })); + const b = fingerprintFinding(makeFinding({ title: 'missing null check' })); + expect(a).toBe(b); + }); +}); + +describe('findingsMatch', () => { + it('matches when line number drifts slightly', () => { + const prev = toStoredFinding(makeFinding({ startLine: 10 })); + const curr = makeFinding({ startLine: 14 }); + expect(findingsMatch(prev, curr)).toBe(true); + }); + + it('does not match different titles', () => { + const prev = toStoredFinding(makeFinding({ title: 'Issue A' })); + const curr = makeFinding({ title: 'Issue B' }); + expect(findingsMatch(prev, curr)).toBe(false); + }); +}); + +describe('diffReviewFindings', () => { + it('marks missing previous findings as resolved', () => { + const previous = [toStoredFinding(makeFinding({ title: 'Fixed bug' }))]; + const current = [makeFinding({ title: 'New bug', startLine: 40 })]; + + const diff = diffReviewFindings(previous, current); + expect(diff.resolved).toHaveLength(1); + expect(diff.resolved[0].title).toBe('Fixed bug'); + expect(diff.new).toHaveLength(1); + expect(diff.new[0].title).toBe('New bug'); + }); + + it('keeps unchanged findings out of the new list', () => { + const previous = [toStoredFinding(makeFinding())]; + const current = [makeFinding()]; + + const diff = diffReviewFindings(previous, current); + expect(diff.resolved).toHaveLength(0); + expect(diff.new).toHaveLength(0); + expect(diff.unchanged).toHaveLength(1); + }); +}); + +describe('review state embed/parse', () => { + it('round-trips state in a comment body', () => { + const state = { + version: 1 as const, + headSha: 'sha1', + reviewedAt: '2026-01-01T00:00:00.000Z', + findings: [toStoredFinding(makeFinding())], + }; + const body = `\n${embedReviewState(state)}\n## Code Review`; + expect(parseReviewState(body)?.headSha).toBe('sha1'); + expect(parseReviewState(body)?.findings).toHaveLength(1); + }); +}); + +describe('enrichReviewSummary', () => { + it('builds approved summary when previous issues are gone', () => { + const previous = { + version: 1 as const, + headSha: 'old', + reviewedAt: '2026-01-01T00:00:00.000Z', + findings: [toStoredFinding(makeFinding())], + }; + + const summary = enrichReviewSummary(makePr(), [], makeSummary(), previous); + expect(summary.approved).toBe(true); + expect(summary.resolvedCount).toBe(1); + expect(summary.totalFindings).toBe(0); + expect(summary.narrative).toContain('Resolved 1 previous finding'); + }); + + it('includes PR title and body in narrative', () => { + const summary = enrichReviewSummary(makePr(), [makeFinding()], makeSummary(), null); + expect(summary.narrative).toContain('Add fixture deletion'); + expect(summary.narrative).toContain('Implements round deletion'); + }); +}); + +describe('normalizeFindingTitle', () => { + it('trims and collapses spaces', () => { + expect(normalizeFindingTitle(' hello world ')).toBe('hello world'); + }); +});