From 2dbb4c477082a85bd00ebdc810d4f0e39eb911f4 Mon Sep 17 00:00:00 2001 From: EmersonBraun Date: Tue, 25 Aug 2026 23:03:10 -0300 Subject: [PATCH] feat: make GitHub reviews incremental and idempotent --- CHANGELOG.md | 1 + agents/code-review/reporters.ts | 36 ++++++++++-- agents/code-review/sources.ts | 12 +++- docs/OPERATIONS.md | 10 ++++ llms-full.txt | 11 ++++ readme-standard-v1.json | 2 +- src/cli.ts | 39 ++++++++++++- src/github-review-state.ts | 94 +++++++++++++++++++++++++++++++ test/github-review-state.test.mjs | 62 ++++++++++++++++++++ 9 files changed, 258 insertions(+), 9 deletions(-) create mode 100644 src/github-review-state.ts create mode 100644 test/github-review-state.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index e52740d..ff797f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,4 +14,5 @@ All notable changes will be documented here. This project follows Semantic Versi - Added primary-lens execution coverage to review summaries so partial provider degradation is visible. - Repositioned the CLI and GitHub Action as provider-neutral. - Made provider selection explicit and removed provider-specific model defaults. +- Added bounded GitHub review reconciliation with SHA/policy fingerprints, incremental compare scope when the prior SHA is an ancestor, fork-safe skip behavior, and idempotent summary updates. - Added package metadata, CLI help, open-source governance, and contribution guidance. diff --git a/agents/code-review/reporters.ts b/agents/code-review/reporters.ts index 5833917..b89b5c6 100644 --- a/agents/code-review/reporters.ts +++ b/agents/code-review/reporters.ts @@ -1,5 +1,6 @@ import { writeFileSync } from 'node:fs' import type { Finding, Reporter, ReviewResult } from './agent.js' +import { githubGet } from '../../src/github-review-state.js' /** * Reporters turn a ReviewResult into an output surface. They are orchestration code @@ -106,17 +107,44 @@ async function githubPost(token: string, path: string, body: unknown): Promise<{ 'content-type': 'application/json', }, body: JSON.stringify(body), + signal: AbortSignal.timeout(10_000), }) if (!res.ok) throw new Error(`GitHub POST ${path} → ${res.status}: ${await res.text()}`) return res.json() as Promise<{ html_url?: string }> } +async function githubPatch(token: string, path: string, body: unknown): Promise { + const res = await fetch(`https://api.github.com${path}`, { + method: 'PATCH', + headers: { + authorization: `Bearer ${token}`, + accept: 'application/vnd.github+json', + 'user-agent': 'agentskit-code-review', + 'content-type': 'application/json', + }, + body: JSON.stringify(body), + signal: AbortSignal.timeout(10_000), + }) + if (!res.ok) throw new Error(`GitHub PATCH ${path} → ${res.status}: ${await res.text()}`) +} + /** One summary comment on the PR thread (uses the issues endpoint — always available). */ -export function githubSummaryReporter(c: { owner: string; repo: string; number: number; token: string }): Reporter { +export function githubSummaryReporter(c: { owner: string; repo: string; number: number; token: string; marker?: string }): Reporter { return { name: 'github-summary', async emit(review: ReviewResult) { - await githubPost(c.token, `/repos/${c.owner}/${c.repo}/issues/${c.number}/comments`, { body: renderMarkdown(review) }) + const body = `${c.marker ? `${c.marker}\n` : ''}${renderMarkdown(review)}` + if (!c.marker) { + await githubPost(c.token, `/repos/${c.owner}/${c.repo}/issues/${c.number}/comments`, { body }) + return + } + const comments = await githubGet>( + c.token, + `/repos/${c.owner}/${c.repo}/issues/${c.number}/comments?per_page=100`, + ) + const existing = comments.find((comment) => comment.body?.includes(c.marker!)) + if (existing) await githubPatch(c.token, `/repos/${c.owner}/${c.repo}/issues/comments/${existing.id}`, { body }) + else await githubPost(c.token, `/repos/${c.owner}/${c.repo}/issues/${c.number}/comments`, { body }) }, } } @@ -126,7 +154,7 @@ export function githubSummaryReporter(c: { owner: string; repo: string; number: * overall verdict + summary body. Findings outside the diff are folded into the body * (GitHub rejects review comments on unchanged lines). */ -export function githubInlineReporter(c: { owner: string; repo: string; number: number; token: string; commitId?: string }): Reporter { +export function githubInlineReporter(c: { owner: string; repo: string; number: number; token: string; commitId?: string; marker?: string }): Reporter { // Never emit APPROVE — a GitHub Actions token and your own PR both reject it (422). const eventFor = (v: ReviewResult['verdict']) => (v === 'REQUEST CHANGES' ? 'REQUEST_CHANGES' : 'COMMENT') return { @@ -140,7 +168,7 @@ export function githubInlineReporter(c: { owner: string; repo: string; number: n body: `**${SEV_EMOJI[f.severity]} ${f.severity} · ${f.category}** — ${f.title}\n\n${f.rationale}\n\n💡 ${f.suggestion}${f.suggestedPatch ? `\n\n\`\`\`diff\n${f.suggestedPatch}\n\`\`\`` : ''}`, })) const body = - `## Code review — ${review.verdict}\n\n${review.summary}` + + `${c.marker ? `${c.marker}\n` : ''}## Code review — ${review.verdict}\n\n${review.summary}` + (outOfDiff.length ? `\n\n### Findings outside the diff\n${groupBySeverity(outOfDiff)}` : '') const payload = { body, diff --git a/agents/code-review/sources.ts b/agents/code-review/sources.ts index d012b7e..b034b20 100644 --- a/agents/code-review/sources.ts +++ b/agents/code-review/sources.ts @@ -22,7 +22,7 @@ export interface SourceLimits { export type SourceConfig = | { kind: 'git-diff'; base: string; head?: string; cwd?: string; redact?: boolean; limits?: SourceLimits } - | { kind: 'github-pr'; owner: string; repo: string; number: number; token: string; redact?: boolean; limits?: SourceLimits } + | { kind: 'github-pr'; owner: string; repo: string; number: number; token: string; baselineSha?: string; redact?: boolean; limits?: SourceLimits } | { kind: 'paths'; paths: string[]; cwd?: string; redact?: boolean; limits?: SourceLimits } | { kind: 'stdin'; content: string; filename?: string; redact?: boolean; limits?: SourceLimits } | { kind: 'isolated-snapshot'; cwd: string; patterns: string[]; redact?: boolean; limits?: SourceLimits } @@ -162,7 +162,15 @@ async function fromGithubPr(c: Extract): Pr } const pr = await api<{ head: { sha: string } }>(`/repos/${c.owner}/${c.repo}/pulls/${c.number}`); const sha = pr.head.sha const files: Array<{ filename: string; patch?: string; status: string }> = [] - for (let page = 1; ; page++) { const batch = await api(`/repos/${c.owner}/${c.repo}/pulls/${c.number}/files?per_page=100&page=${page}`); files.push(...batch); if (batch.length < 100) break } + const filesPath = c.baselineSha + ? `/repos/${c.owner}/${c.repo}/compare/${c.baselineSha}...${sha}` + : `/repos/${c.owner}/${c.repo}/pulls/${c.number}/files?per_page=100&page=1` + if (c.baselineSha) { + const comparison = await api<{ files?: typeof files }>(filesPath) + files.push(...(comparison.files ?? [])) + } else { + for (let page = 1; ; page++) { const batch = await api(`/repos/${c.owner}/${c.repo}/pulls/${c.number}/files?per_page=100&page=${page}`); files.push(...batch); if (batch.length < 100) break } + } const targets: ReviewTarget[] = [] for (const f of files) { if (f.status === 'removed') continue diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 0828557..b80a3f7 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -124,6 +124,16 @@ permissions: Use environment protection or organization secrets for sensitive providers. Rotate a secret after suspected exposure and review provider usage plus GitHub audit logs. +When `--post` is used with `--pr`, the reviewer stores a hidden SHA and policy +fingerprint marker in the summary comment. Re-running the same head SHA with +the same policy skips provider calls and updates no comments. A new SHA uses +GitHub compare scope only when the previous marked SHA is an ancestor; a +missing marker, force-push, or changed fingerprint falls back to the full PR +file list. Fork PRs are reported as `SKIPPED` with exit `2` on this workflow +boundary; do not switch to `pull_request_target` to expose secrets. Summary +comments are reconciled by marker, while POST/PATCH failures remain visible for +manual retry. + ## Advisory and blocking behavior The Action is advisory by default: `fail-on-block: 'false'` adds `--no-fail`. Findings still post, but surviving blocker/high findings do not fail the job. `--no-fail` never suppresses provider, source, reporter, or review-execution errors. For enforcement: diff --git a/llms-full.txt b/llms-full.txt index 7ba0f40..d71fcdb 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -523,6 +523,16 @@ permissions: Use environment protection or organization secrets for sensitive providers. Rotate a secret after suspected exposure and review provider usage plus GitHub audit logs. +When `--post` is used with `--pr`, the reviewer stores a hidden SHA and policy +fingerprint marker in the summary comment. Re-running the same head SHA with +the same policy skips provider calls and updates no comments. A new SHA uses +GitHub compare scope only when the previous marked SHA is an ancestor; a +missing marker, force-push, or changed fingerprint falls back to the full PR +file list. Fork PRs are reported as `SKIPPED` with exit `2` on this workflow +boundary; do not switch to `pull_request_target` to expose secrets. Summary +comments are reconciled by marker, while POST/PATCH failures remain visible for +manual retry. + ## Advisory and blocking behavior The Action is advisory by default: `fail-on-block: 'false'` adds `--no-fail`. Findings still post, but surviving blocker/high findings do not fail the job. `--no-fail` never suppresses provider, source, reporter, or review-execution errors. For enforcement: @@ -829,4 +839,5 @@ All notable changes will be documented here. This project follows Semantic Versi - Added primary-lens execution coverage to review summaries so partial provider degradation is visible. - Repositioned the CLI and GitHub Action as provider-neutral. - Made provider selection explicit and removed provider-specific model defaults. +- Added bounded GitHub review reconciliation with SHA/policy fingerprints, incremental compare scope when the prior SHA is an ancestor, fork-safe skip behavior, and idempotent summary updates. - Added package metadata, CLI help, open-source governance, and contribution guidance. diff --git a/readme-standard-v1.json b/readme-standard-v1.json index 43607f3..c2aebd6 100644 --- a/readme-standard-v1.json +++ b/readme-standard-v1.json @@ -221,7 +221,7 @@ "docs/OPERATIONS.md", "test/cli-smoke.test.mjs" ], - "sourceHash": "sha256:aae7093f7641e5be3cec91d5a24bee31311bfe474395895e95923f91d6b33b5a" + "sourceHash": "sha256:6a63e5d118023456293b828a631bb15aa6c93fe217318728db6d517394777960" }, "exceptions": [] } diff --git a/src/cli.ts b/src/cli.ts index bf5c945..da304fb 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -24,6 +24,7 @@ import { ollamaReview } from './ollama-adapter.js' import type { SourceConfig } from '../agents/code-review/sources.js' import { diagnoseProvider, factoryFor, providerEntry, providerRegistry, resolveProviderId, type DoctorReport, type ProviderEntry } from './provider-registry.js' import { loadReviewConfig, type ResolvedReviewConfig } from './review-config.js' +import { getGithubReviewState, reviewFingerprint } from './github-review-state.js' const HELP = `AgentsKit Code Review — deep, low-noise review with your model @@ -156,13 +157,47 @@ async function main() { conventions: flag('conventions'), }, }) - const source = await resolveSource(reviewConfig) + let source = await resolveSource(reviewConfig) + const githubState = source.kind === 'github-pr' && has('post') + ? await getGithubReviewState({ + ...source, + fingerprint: reviewFingerprint({ + engine: '@agentskit/code-review@0.1.0', + provider: reviewConfig.provider, + model: reviewConfig.model, + transport: reviewConfig.transport, + lenses: reviewConfig.lenses, + votes: reviewConfig.votes, + retries: reviewConfig.retries, + thresholds: reviewConfig.thresholds, + budget: reviewConfig.budget, + context: reviewConfig.context, + redaction: reviewConfig.redaction, + conventions: reviewConfig.conventions ?? 'auto', + }), + }) + : undefined + if (githubState?.fork) { + console.error(`SKIPPED: fork PR ${githubState.owner}/${githubState.repo}#${githubState.number} cannot be posted from this workflow boundary`) + process.exitCode = 2 + return + } + if (githubState?.alreadyReviewed) { + console.log(`SKIPPED: ${githubState.owner}/${githubState.repo}#${githubState.number} already reviewed at ${githubState.sha} with the same fingerprint`) + return + } + if (githubState?.scope === 'incremental' && githubState.baselineSha && source.kind === 'github-pr') { + source = { ...source, baselineSha: githubState.baselineSha } + } const reporters: Reporter[] = [markdownReporter()] const sarif = flag('sarif') if (sarif) reporters.push(sarifReporter({ file: sarif })) if (has('post') && source.kind === 'github-pr') { const { owner, repo, number, token } = source - reporters.push(githubInlineReporter({ owner, repo, number, token }), githubSummaryReporter({ owner, repo, number, token })) + reporters.push( + githubInlineReporter({ owner, repo, number, token, commitId: githubState?.sha, marker: githubState?.marker }), + githubSummaryReporter({ owner, repo, number, token, marker: githubState?.marker }), + ) } const config: CodeReviewConfig = { diff --git a/src/github-review-state.ts b/src/github-review-state.ts new file mode 100644 index 0000000..f008063 --- /dev/null +++ b/src/github-review-state.ts @@ -0,0 +1,94 @@ +import { createHash } from 'node:crypto' + +const API = 'https://api.github.com' +const GET_TIMEOUT_MS = 5_000 +const MARKER_PREFIX = '` +} + +export async function githubGet(token: string, path: string): Promise { + let lastError: unknown + for (let attempt = 0; attempt < 2; attempt++) { + try { + const response = await fetch(`${API}${path}`, { + headers: { authorization: `Bearer ${token}`, accept: 'application/vnd.github+json', 'user-agent': 'agentskit-code-review' }, + signal: AbortSignal.timeout(GET_TIMEOUT_MS), + }) + if (!response.ok) throw new Error(`GitHub GET ${path} → ${response.status}`) + return await response.json() as T + } catch (error) { + lastError = error + if (attempt === 1) throw error + } + } + throw lastError instanceof Error ? lastError : new Error('GitHub GET failed') +} + +export function markerIn(body: string | undefined, marker: string): boolean { + return body?.includes(marker) ?? false +} + +function previousMarker(body: string | undefined, fingerprint: string): string | undefined { + const match = body?.match(new RegExp(`${MARKER_PREFIX} sha=([^ ]+) fingerprint=${fingerprint} -->`)) + return match?.[1] +} + +export async function getGithubReviewState(input: { + owner: string + repo: string + number: number + token: string + fingerprint: string +}): Promise { + const pr = await githubGet<{ + head: { sha: string; repo?: { full_name?: string } } + base: { sha: string; repo?: { full_name?: string } } + }>(input.token, `/repos/${input.owner}/${input.repo}/pulls/${input.number}`) + const marker = reviewMarker(pr.head.sha, input.fingerprint) + const comments = await githubGet>( + input.token, + `/repos/${input.owner}/${input.repo}/issues/${input.number}/comments?per_page=100`, + ) + const previousSha = comments.map((comment) => previousMarker(comment.body, input.fingerprint)).find(Boolean) + let scope: GithubReviewState['scope'] = 'full' + if (previousSha && previousSha !== pr.head.sha) { + const comparison = await githubGet<{ status?: string }>(input.token, `/repos/${input.owner}/${input.repo}/compare/${previousSha}...${pr.head.sha}`) + if (comparison.status === 'ahead') scope = 'incremental' + } + return { + owner: input.owner, + repo: input.repo, + number: input.number, + sha: pr.head.sha, + baseSha: pr.base.sha, + fork: pr.head.repo?.full_name !== undefined && pr.head.repo.full_name !== pr.base.repo?.full_name, + fingerprint: input.fingerprint, + marker, + alreadyReviewed: comments.some((comment) => markerIn(comment.body, marker)), + scope, + ...(scope === 'incremental' && previousSha ? { baselineSha: previousSha } : {}), + } +} diff --git a/test/github-review-state.test.mjs b/test/github-review-state.test.mjs new file mode 100644 index 0000000..1581a10 --- /dev/null +++ b/test/github-review-state.test.mjs @@ -0,0 +1,62 @@ +import assert from 'node:assert/strict' +import test from 'node:test' +import { getGithubReviewState, reviewFingerprint, reviewMarker } from '../dist/src/github-review-state.js' + +test('reconciles the same SHA and fingerprint without provider work', async () => { + const originalFetch = globalThis.fetch + const fingerprint = reviewFingerprint({ provider: 'codex-cli', votes: 3 }) + const marker = reviewMarker('head-1', fingerprint) + globalThis.fetch = async (url) => { + const path = new URL(url).pathname + if (path.endsWith('/pulls/7')) return Response.json({ head: { sha: 'head-1', repo: { full_name: 'org/repo' } }, base: { sha: 'base-1', repo: { full_name: 'org/repo' } } }) + if (path.endsWith('/comments')) return Response.json([{ id: 1, body: `${marker}\n## Code review — APPROVE` }]) + return new Response('not found', { status: 404 }) + } + try { + const state = await getGithubReviewState({ owner: 'org', repo: 'repo', number: 7, token: 'test-token', fingerprint }) + assert.equal(state.alreadyReviewed, true) + assert.equal(state.fork, false) + } finally { globalThis.fetch = originalFetch } +}) + +test('retries one idempotent GET and detects fork ownership', async () => { + const originalFetch = globalThis.fetch + let pulls = 0 + globalThis.fetch = async (url) => { + const path = new URL(url).pathname + if (path.endsWith('/pulls/8')) { + pulls++ + if (pulls === 1) throw new Error('temporary network failure') + return Response.json({ head: { sha: 'head-2', repo: { full_name: 'contributor/repo' } }, base: { sha: 'base-2', repo: { full_name: 'org/repo' } } }) + } + if (path.endsWith('/comments')) return Response.json([]) + return new Response('not found', { status: 404 }) + } + try { + const state = await getGithubReviewState({ owner: 'org', repo: 'repo', number: 8, token: 'test-token', fingerprint: 'f' }) + assert.equal(pulls, 2) + assert.equal(state.fork, true) + assert.equal(state.alreadyReviewed, false) + } finally { globalThis.fetch = originalFetch } +}) + +test('uses incremental scope only when the previous reviewed SHA is an ancestor', async () => { + const originalFetch = globalThis.fetch + const fingerprint = 'stable' + const oldMarker = reviewMarker('head-old', fingerprint) + const calls = [] + globalThis.fetch = async (url) => { + const path = new URL(url).pathname + calls.push(path) + if (path.endsWith('/pulls/9')) return Response.json({ head: { sha: 'head-new', repo: { full_name: 'org/repo' } }, base: { sha: 'base-1', repo: { full_name: 'org/repo' } } }) + if (path.endsWith('/comments')) return Response.json([{ body: oldMarker }]) + if (path.includes('/compare/head-old...head-new')) return Response.json({ status: 'ahead' }) + return new Response('not found', { status: 404 }) + } + try { + const state = await getGithubReviewState({ owner: 'org', repo: 'repo', number: 9, token: 'test-token', fingerprint }) + assert.equal(state.scope, 'incremental') + assert.equal(state.baselineSha, 'head-old') + assert.ok(calls.some(path => path.includes('/compare/head-old...head-new'))) + } finally { globalThis.fetch = originalFetch } +})