diff --git a/AGENTS.md b/AGENTS.md index 41a2a55..53e0d53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,13 +57,19 @@ files. Prefer SDK-provided types over redefining shapes. cohesive service surfaces (`githubClient.submitReview(…)`). - `type` over `interface`. TypeScript strict mode. `node:` prefix for built-ins. - Explicit return types on exports. No `any`. No `as` or `!` — use runtime - guards or schema validation to narrow. + guards or schema validation to narrow. Truthy/falsy checks over explicit + `!== undefined` comparisons — use `if (value)` not `if (value !== undefined)` + unless distinguishing `undefined` from other falsy values (`null`, `0`, `""`, + `false`) actually matters for correctness. - Immutable by default; avoid `let`. A `reduce` must return a new accumulator each step — never mutate-and-return. When mutation is genuinely needed, add a comment justifying it. - Explicit names over abbreviations, everywhere — params, callbacks, locals. -- Early returns over nested `if/else`. Extract multi-clause conditionals into - named booleans. Name booleans for the affirmative state. +- Early returns over nested `if/else`. When a function has a primary path and + a secondary path (e.g. first-run vs re-run), return early from the simpler + branch so the remaining code flows linearly without nesting. Extract + multi-clause conditionals into named booleans. Name booleans for the + affirmative state. - Block bodies `{}` for any multiline function response — expression bodies only for one-liners. A multi-clause boolean spanning lines gets `{ return (...) }`; guard chains get explicit early returns, never a diff --git a/README.md b/README.md index 784d01a..159d5e4 100644 --- a/README.md +++ b/README.md @@ -87,12 +87,12 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm ## Outputs -| Output | Description | -| ---------------- | ------------------------------------------------------------ | -| `findings_count` | Number of findings posted (after threshold and cap) | -| `review_url` | URL of the submitted review; empty when no review was posted | -| `model_used` | Model that produced the accepted response | -| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | +| Output | Description | +| ---------------- | ------------------------------------------------------------------------- | +| `findings_count` | Number of new findings posted (after threshold, cap, and cross-run dedup) | +| `review_url` | URL of the submitted review; empty when no review was posted | +| `model_used` | Model that produced the accepted response | +| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | ## How it works @@ -102,8 +102,9 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm 4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense) and sends it to OpenRouter 5. Validates the response against a strict Zod schema, retrying with a fallback model if the primary fails 6. Filters findings by severity threshold, deduplicates overlapping findings, and caps if configured -7. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback -8. Posts one consolidated review — findings that can't be inlined render in the review body +7. On re-runs, compares findings against previously posted inline comments (by hidden HTML anchor) and filters out duplicates +8. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback +9. Posts one consolidated review — findings that can't be inlined render in the review body; re-runs upsert a summary comment with totals ## Status @@ -119,10 +120,10 @@ umm-actually is in early development — the core review pipeline works but ther - Skip-path handling with posted reasons (oversized diff, empty diff, API limits) - Cost transparency (per-run model/token/USD report in workflow summary) - `@umm review` comment trigger for on-demand re-reviews +- Cross-run finding dedup — re-runs detect previously posted inline findings via hidden HTML anchors and post only new ones, with an updatable summary comment tracking totals **In progress** -- **Review dedup on re-runs** — currently each push posts a new review; working on deduplicating findings across runs and updating a single summary comment instead of creating new ones - **Doc-staleness detection** — extending the workspace scan to doc files (`.md`, `.json`) so unchanged docs that describe changed code reach the prompt and staleness becomes a finding - **Branded check run** — using the Checks API so the CI check shows the umm-actually avatar instead of the generic GitHub Actions logo diff --git a/action.yml b/action.yml index 7fc1c02..ff5dd3b 100644 --- a/action.yml +++ b/action.yml @@ -55,7 +55,7 @@ inputs: outputs: findings_count: - description: Number of findings posted (after threshold and cap) + description: Number of new findings posted (after threshold, cap, and cross-run dedup) review_url: description: URL of the submitted review; empty when no review was posted model_used: diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 4a93268..57a63c0 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -15,9 +15,12 @@ import { annotateDiff } from "../diff/annotate-diff.js" import { computeCommentableLines } from "../diff/commentable-lines.js" import type { ReviewResponse } from "../review/finding.js" import { + buildRerunSummary, buildReviewBody, buildZeroFindingsBody, + computeAnchorKey, mapFindingsToReview, + RERUN_ANCHOR, type ReviewComment, } from "../review/comment-mapping.js" import { selectFindings } from "../review/select-findings.js" @@ -176,11 +179,19 @@ const first = (array: T[]): T => { return item } +type UpsertSummaryCommentParams = { + prNumber: number + body: string + anchor: string +} + type RecordingStubs = { deps: OrchestrateDeps fetchPullRequestCalls: { prNumber: number }[] fetchDiffCalls: { prNumber: number }[] submitReviewCalls: SubmitReviewParams[] + fetchReviewCommentsCalls: { prNumber: number }[] + upsertSummaryCommentCalls: UpsertSummaryCommentParams[] readConventionsCalls: { conventionsFile: string }[] readChangedFilesCalls: ReadChangedFilesParams[] findRelatedFilesCalls: FindRelatedFilesParams[] @@ -201,6 +212,8 @@ const makeOrchestrateDeps = ( const fetchPullRequestCalls: { prNumber: number }[] = [] const fetchDiffCalls: { prNumber: number }[] = [] const submitReviewCalls: SubmitReviewParams[] = [] + const fetchReviewCommentsCalls: { prNumber: number }[] = [] + const upsertSummaryCommentCalls: UpsertSummaryCommentParams[] = [] const readConventionsCalls: { conventionsFile: string }[] = [] const readChangedFilesCalls: ReadChangedFilesParams[] = [] const findRelatedFilesCalls: FindRelatedFilesParams[] = [] @@ -229,6 +242,17 @@ const makeOrchestrateDeps = ( usedFallbackBody: false, } }, + fetchReviewComments: async (params) => { + fetchReviewCommentsCalls.push(params) + return [] + }, + upsertSummaryComment: async (params) => { + upsertSummaryCommentCalls.push(params) + return { + url: "https://github.com/test/comment/1", + created: true, + } + }, ...overrides.githubClient, } @@ -267,6 +291,8 @@ const makeOrchestrateDeps = ( fetchPullRequestCalls, fetchDiffCalls, submitReviewCalls, + fetchReviewCommentsCalls, + upsertSummaryCommentCalls, readConventionsCalls, readChangedFilesCalls, findRelatedFilesCalls, @@ -653,6 +679,177 @@ describe("orchestrate", () => { expect(stubs.fetchPullRequestCalls).toHaveLength(0) }) }) + + describe("cross-run dedup", () => { + it("first run — posts normal review, no summary comment", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(expectedSelection.selected.length) + expect(stubs.fetchReviewCommentsCalls).toHaveLength(1) + expect(stubs.submitReviewCalls).toHaveLength(1) + expect(stubs.upsertSummaryCommentCalls).toHaveLength(0) + }) + + it("re-run — filters duplicate findings and posts only new ones", async () => { + const findings = fixtureReviewResponse.findings + const duplicateFinding = findings[0] + if (duplicateFinding === undefined) { + throw new Error("expected at least one fixture finding") + } + const duplicateAnchor = computeAnchorKey(duplicateFinding) + + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchReviewComments: async () => [ + { + path: duplicateFinding.file, + body: `some comment\n\n`, + }, + ], + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + const newFindingCount = findings.length - 1 + expect(result.findingsCount).toBe(newFindingCount) + expect(stubs.submitReviewCalls).toHaveLength(1) + expect(stubs.upsertSummaryCommentCalls).toHaveLength(1) + + const summaryCall = first(stubs.upsertSummaryCommentCalls) + expect(summaryCall.anchor).toBe(RERUN_ANCHOR) + expect(summaryCall.body).toBe( + buildRerunSummary({ + sha: fixturePrContext.headSha, + newCount: newFindingCount, + totalCount: 1 + newFindingCount, + model: "test/model", + }), + ) + }) + + it("re-run — skips review when all findings are duplicates", async () => { + const findings = fixtureReviewResponse.findings + const existingComments = findings.map((finding) => ({ + path: finding.file, + body: `comment\n\n`, + })) + + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchReviewComments: async () => existingComments, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(result.reviewUrl).toBe("") + expect(stubs.submitReviewCalls).toHaveLength(0) + expect(stubs.upsertSummaryCommentCalls).toHaveLength(1) + + const summaryCall = first(stubs.upsertSummaryCommentCalls) + expect(summaryCall.body).toBe( + buildRerunSummary({ + sha: fixturePrContext.headSha, + newCount: 0, + totalCount: findings.length, + model: "test/model", + }), + ) + }) + + it("re-run — zero findings from LLM posts summary only", async () => { + const stubs = makeOrchestrateDeps({ + fixtureResult: { + review: { analysis: "clean", findings: [] }, + }, + githubClient: { + fetchReviewComments: async () => [ + { + path: "src/a.ts", + body: "body\n\n", + }, + ], + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(stubs.submitReviewCalls).toHaveLength(0) + expect(stubs.upsertSummaryCommentCalls).toHaveLength(1) + + const summaryCall = first(stubs.upsertSummaryCommentCalls) + expect(summaryCall.body).toBe( + buildRerunSummary({ + sha: fixturePrContext.headSha, + newCount: 0, + totalCount: 1, + model: "test/model", + }), + ) + }) + + it("does not fetch review comments on skip paths", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "too_large" as const }), + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.fetchReviewCommentsCalls).toHaveLength(0) + }) + + it("continues without throwing when upsertSummaryComment fails", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchReviewComments: async () => [ + { + path: "src/a.ts", + body: "body\n\n", + }, + ], + upsertSummaryComment: async () => { + throw new Error("API rate limit") + }, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(expectedSelection.selected.length) + expect(result.reviewUrl).toBe("https://github.com/test/review/1") + expect(stubs.submitReviewCalls).toHaveLength(1) + }) + + it("degrades to first run when fetching review comments fails", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchReviewComments: async () => { + throw new Error("network error") + }, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(expectedSelection.selected.length) + expect(stubs.submitReviewCalls).toHaveLength(1) + expect(stubs.upsertSummaryCommentCalls).toHaveLength(0) + }) + }) }) describe("createPromptedGenerateFindings", () => { diff --git a/src/github/__tests__/client.test.ts b/src/github/__tests__/client.test.ts index 52681c3..39bfe1d 100644 --- a/src/github/__tests__/client.test.ts +++ b/src/github/__tests__/client.test.ts @@ -18,12 +18,24 @@ type StubResponse = { data: unknown } | { error: unknown } const makeOctokitStub = ({ getResponses = [], createReviewResponses = [], + listReviewCommentsResponses = [], + listCommentsResponses = [], + createCommentResponses = [], + updateCommentResponses = [], }: { getResponses?: StubResponse[] createReviewResponses?: StubResponse[] + listReviewCommentsResponses?: StubResponse[] + listCommentsResponses?: StubResponse[] + createCommentResponses?: StubResponse[] + updateCommentResponses?: StubResponse[] } = {}) => { const getCalls: Record[] = [] const createReviewCalls: Record[] = [] + const listReviewCommentsCalls: Record[] = [] + const listCommentsCalls: Record[] = [] + const createCommentCalls: Record[] = [] + const updateCommentCalls: Record[] = [] const takeNext = ( queue: StubResponse[], @@ -53,11 +65,53 @@ const makeOctokitStub = ({ "pulls.createReview", ) }, + listReviewComments: async (params) => { + listReviewCommentsCalls.push(params) + return takeNext( + listReviewCommentsResponses, + listReviewCommentsCalls.length, + "pulls.listReviewComments", + ) + }, + }, + issues: { + listComments: async (params) => { + listCommentsCalls.push(params) + return takeNext( + listCommentsResponses, + listCommentsCalls.length, + "issues.listComments", + ) + }, + createComment: async (params) => { + createCommentCalls.push(params) + return takeNext( + createCommentResponses, + createCommentCalls.length, + "issues.createComment", + ) + }, + updateComment: async (params) => { + updateCommentCalls.push(params) + return takeNext( + updateCommentResponses, + updateCommentCalls.length, + "issues.updateComment", + ) + }, }, }, } - return { octokit, getCalls, createReviewCalls } + return { + octokit, + getCalls, + createReviewCalls, + listReviewCommentsCalls, + listCommentsCalls, + createCommentCalls, + updateCommentCalls, + } } const makeClient = (stub: { octokit: OctokitLike }) => { @@ -377,3 +431,269 @@ describe("submitReview", () => { ).rejects.toThrow("unexpected review response shape") }) }) + +describe("fetchReviewComments", () => { + it("returns mapped path and body from review comments", async () => { + const stub = makeOctokitStub({ + listReviewCommentsResponses: [ + { + data: [ + { path: "src/a.ts", body: "comment 1" }, + { path: "src/b.ts", body: "comment 2" }, + ], + }, + ], + }) + const { client } = makeClient(stub) + + const comments = await client.fetchReviewComments({ prNumber: 7 }) + + expect(comments).toEqual([ + { path: "src/a.ts", body: "comment 1" }, + { path: "src/b.ts", body: "comment 2" }, + ]) + expect(stub.listReviewCommentsCalls).toEqual([ + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + per_page: 100, + page: 1, + }, + ]) + }) + + it("paginates when a page is full", async () => { + const fullPage = Array.from({ length: 100 }, (_, index) => ({ + path: `src/file-${index}.ts`, + body: `body-${index}`, + })) + const lastPage = [{ path: "src/last.ts", body: "last" }] + const stub = makeOctokitStub({ + listReviewCommentsResponses: [{ data: fullPage }, { data: lastPage }], + }) + const { client } = makeClient(stub) + + const comments = await client.fetchReviewComments({ prNumber: 7 }) + + expect(comments).toHaveLength(101) + expect(stub.listReviewCommentsCalls).toEqual([ + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + per_page: 100, + page: 1, + }, + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + per_page: 100, + page: 2, + }, + ]) + }) + + it("stops at the page cap and logs a warning", async () => { + const fullPage = Array.from({ length: 100 }, (_, index) => ({ + path: `src/file-${index}.ts`, + body: `body-${index}`, + })) + const responses = Array.from({ length: 10 }, () => ({ data: fullPage })) + const stub = makeOctokitStub({ listReviewCommentsResponses: responses }) + const { client, logger } = makeClient(stub) + + const comments = await client.fetchReviewComments({ prNumber: 7 }) + + expect(comments).toHaveLength(1000) + expect(stub.listReviewCommentsCalls).toHaveLength(10) + expect(stub.listReviewCommentsCalls[9]).toMatchObject({ page: 10 }) + expect(logger.messages).toContainEqual({ + level: "warn", + message: "review comments page cap reached", + data: { prNumber: 7, totalFetched: 1000 }, + }) + }) + + it("throws on a malformed response", async () => { + const stub = makeOctokitStub({ + listReviewCommentsResponses: [{ data: "not an array" }], + }) + const { client } = makeClient(stub) + + await expect(client.fetchReviewComments({ prNumber: 7 })).rejects.toThrow( + "unexpected review comments response shape", + ) + }) +}) + +describe("upsertSummaryComment", () => { + const commentUrl = + "https://github.com/aliasunder/fixture/pull/7#issuecomment-1" + const anchor = "" + const body = `${anchor}\n\n**umm-actually** re-reviewed` + + it("creates a new comment when no matching anchor exists", async () => { + const stub = makeOctokitStub({ + listCommentsResponses: [ + { + data: [ + { id: 99, body: "human comment", html_url: "https://example.com" }, + ], + }, + ], + createCommentResponses: [{ data: { html_url: commentUrl } }], + }) + const { client } = makeClient(stub) + + const result = await client.upsertSummaryComment({ + prNumber: 7, + body, + anchor, + }) + + expect(result).toEqual({ url: commentUrl, created: true }) + expect(stub.createCommentCalls).toEqual([ + { + owner: "aliasunder", + repo: "fixture", + issue_number: 7, + body, + }, + ]) + expect(stub.updateCommentCalls).toHaveLength(0) + }) + + it("updates an existing comment when the anchor is found", async () => { + const stub = makeOctokitStub({ + listCommentsResponses: [ + { + data: [ + { + id: 42, + body: `${anchor}\n\nold summary`, + html_url: commentUrl, + }, + ], + }, + ], + updateCommentResponses: [{ data: { html_url: commentUrl } }], + }) + const { client } = makeClient(stub) + + const result = await client.upsertSummaryComment({ + prNumber: 7, + body, + anchor, + }) + + expect(result).toEqual({ url: commentUrl, created: false }) + expect(stub.updateCommentCalls).toEqual([ + { + owner: "aliasunder", + repo: "fixture", + comment_id: 42, + body, + }, + ]) + expect(stub.createCommentCalls).toHaveLength(0) + }) + + it("finds the anchor comment on a later page", async () => { + const fullPage = Array.from({ length: 100 }, (_, index) => ({ + id: index + 1, + body: `comment ${index}`, + html_url: `https://example.com/${index}`, + })) + const stub = makeOctokitStub({ + listCommentsResponses: [ + { data: fullPage }, + { + data: [ + { + id: 200, + body: `${anchor}\n\nold summary`, + html_url: commentUrl, + }, + ], + }, + ], + updateCommentResponses: [{ data: { html_url: commentUrl } }], + }) + const { client } = makeClient(stub) + + const result = await client.upsertSummaryComment({ + prNumber: 7, + body, + anchor, + }) + + expect(result).toEqual({ url: commentUrl, created: false }) + expect(stub.listCommentsCalls).toHaveLength(2) + expect(stub.updateCommentCalls).toEqual([ + { owner: "aliasunder", repo: "fixture", comment_id: 200, body }, + ]) + }) + + it("throws on a malformed issue comments response", async () => { + const stub = makeOctokitStub({ + listCommentsResponses: [{ data: "not an array" }], + }) + const { client } = makeClient(stub) + + await expect( + client.upsertSummaryComment({ prNumber: 7, body, anchor }), + ).rejects.toThrow("unexpected issue comments response shape") + }) + + it("throws when the create response has no html_url", async () => { + const stub = makeOctokitStub({ + listCommentsResponses: [{ data: [] }], + createCommentResponses: [{ data: { id: 1 } }], + }) + const { client } = makeClient(stub) + + await expect( + client.upsertSummaryComment({ prNumber: 7, body, anchor }), + ).rejects.toThrow("unexpected issue comment response shape") + }) + + it("throws when the update response has no html_url", async () => { + const stub = makeOctokitStub({ + listCommentsResponses: [ + { + data: [ + { + id: 42, + body: `${anchor}\n\nold summary`, + html_url: commentUrl, + }, + ], + }, + ], + updateCommentResponses: [{ data: { id: 42 } }], + }) + const { client } = makeClient(stub) + + await expect( + client.upsertSummaryComment({ prNumber: 7, body, anchor }), + ).rejects.toThrow("unexpected issue comment response shape") + }) + + it("creates when the issue comments list is empty", async () => { + const stub = makeOctokitStub({ + listCommentsResponses: [{ data: [] }], + createCommentResponses: [{ data: { html_url: commentUrl } }], + }) + const { client } = makeClient(stub) + + const result = await client.upsertSummaryComment({ + prNumber: 7, + body, + anchor, + }) + + expect(result).toEqual({ url: commentUrl, created: true }) + }) +}) diff --git a/src/github/client.ts b/src/github/client.ts index b3d2e09..5169726 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -29,6 +29,34 @@ export type OctokitLike = { body: string comments?: ReviewComment[] }): Promise<{ data: unknown }> + listReviewComments(params: { + owner: string + repo: string + pull_number: number + per_page?: number + page?: number + }): Promise<{ data: unknown }> + } + issues: { + listComments(params: { + owner: string + repo: string + issue_number: number + per_page?: number + page?: number + }): Promise<{ data: unknown }> + createComment(params: { + owner: string + repo: string + issue_number: number + body: string + }): Promise<{ data: unknown }> + updateComment(params: { + owner: string + repo: string + comment_id: number + body: string + }): Promise<{ data: unknown }> } } } @@ -37,6 +65,8 @@ export type DiffFetchResult = export type SubmitReviewResult = { url: string; usedFallbackBody: boolean } +export type UpsertCommentResult = { url: string; created: boolean } + export type GithubClient = { fetchPullRequest: (params: { prNumber: number }) => Promise fetchDiff: (params: { prNumber: number }) => Promise @@ -49,6 +79,14 @@ export type GithubClient = { * rejects the inline anchors. */ fallbackBody: string }) => Promise + fetchReviewComments: (params: { + prNumber: number + }) => Promise<{ path: string; body: string }[]> + upsertSummaryComment: (params: { + prNumber: number + body: string + anchor: string + }) => Promise } /** The pullRequestEventSchema fields minus the event wrapper — what the REST @@ -61,7 +99,19 @@ const prResponseSchema = z.object({ base: z.object({ ref: z.string() }), }) -const reviewUrlSchema = z.object({ html_url: z.string() }) +const urlResponseSchema = z.object({ html_url: z.string() }) + +const reviewCommentListSchema = z.array( + z.object({ path: z.string(), body: z.string() }), +) + +const issueCommentListSchema = z.array( + z.object({ + id: z.int().positive(), + body: z.string(), + html_url: z.string(), + }), +) /** Octokit request errors carry a numeric `status` — duck-typed so stubs and * future octokit versions need no instanceof on octokit internals. */ @@ -79,8 +129,11 @@ export const createGithubClient = ( }: { octokit: OctokitLike; owner: string; repo: string }, logger: Logger, ): GithubClient => { + const MAX_PAGES = 10 + const PER_PAGE = 100 + const parseReviewUrl = (data: unknown): string => { - const parsed = reviewUrlSchema.safeParse(data) + const parsed = urlResponseSchema.safeParse(data) if (!parsed.success) throw new Error("unexpected review response shape") return parsed.data.html_url } @@ -189,5 +242,113 @@ export const createGithubClient = ( } } - return { fetchPullRequest, fetchDiff, submitReview } + const fetchReviewComments = async ({ + prNumber, + }: { + prNumber: number + }): Promise<{ path: string; body: string }[]> => { + const allComments: { path: string; body: string }[] = [] + + for (let page = 1; page <= MAX_PAGES; page++) { + const response = await octokit.rest.pulls.listReviewComments({ + owner, + repo, + pull_number: prNumber, + per_page: PER_PAGE, + page, + }) + const parsed = reviewCommentListSchema.safeParse(response.data) + if (!parsed.success) { + throw new Error("unexpected review comments response shape") + } + allComments.push(...parsed.data) + if (parsed.data.length < PER_PAGE) break + if (page === MAX_PAGES) { + logger.warn("review comments page cap reached", { + prNumber, + totalFetched: allComments.length, + }) + } + } + + logger.info("fetched review comments", { + prNumber, + count: allComments.length, + }) + return allComments + } + + const upsertSummaryComment = async ({ + prNumber, + body, + anchor, + }: { + prNumber: number + body: string + anchor: string + }): Promise => { + let totalFetched = 0 + + for (let page = 1; page <= MAX_PAGES; page++) { + const response = await octokit.rest.issues.listComments({ + owner, + repo, + issue_number: prNumber, + per_page: PER_PAGE, + page, + }) + const parsed = issueCommentListSchema.safeParse(response.data) + if (!parsed.success) { + throw new Error("unexpected issue comments response shape") + } + totalFetched += parsed.data.length + + const existingComment = parsed.data.find((comment) => + comment.body.includes(anchor), + ) + if (existingComment) { + const updateResponse = await octokit.rest.issues.updateComment({ + owner, + repo, + comment_id: existingComment.id, + body, + }) + const updateParsed = urlResponseSchema.safeParse(updateResponse.data) + if (!updateParsed.success) { + throw new Error("unexpected issue comment response shape") + } + logger.info("updated summary comment", { prNumber }) + return { url: updateParsed.data.html_url, created: false } + } + + if (parsed.data.length < PER_PAGE) break + if (page === MAX_PAGES) { + logger.warn("issue comments page cap reached", { + prNumber, + totalFetched, + }) + } + } + + const response = await octokit.rest.issues.createComment({ + owner, + repo, + issue_number: prNumber, + body, + }) + const parsed = urlResponseSchema.safeParse(response.data) + if (!parsed.success) { + throw new Error("unexpected issue comment response shape") + } + logger.info("created summary comment", { prNumber }) + return { url: parsed.data.html_url, created: true } + } + + return { + fetchPullRequest, + fetchDiff, + submitReview, + fetchReviewComments, + upsertSummaryComment, + } } diff --git a/src/orchestrate.ts b/src/orchestrate.ts index f2b25e7..f60406d 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -16,9 +16,13 @@ import type { import { renderCostSummary } from "./openrouter/cost-summary.js" import type { ContextReader } from "./context/workspace.js" import { + buildRerunSummary, buildReviewBody, buildZeroFindingsBody, + computeAnchorKey, + extractAnchorKeys, mapFindingsToReview, + RERUN_ANCHOR, type ReviewComment, } from "./review/comment-mapping.js" import { resolveSeverityThreshold, type Finding } from "./review/finding.js" @@ -252,39 +256,129 @@ export const orchestrate = async ( maxFindings: config.maxFindings, }) - // Step 13: post review - const hasFindings = selected.length > 0 - - const reviewPayload: ReviewPayload = hasFindings - ? buildReviewPayload({ - findings: selected, - commentableByPath, - droppedByCap, - modelUsed, - }) - : { - body: buildZeroFindingsBody({ model: modelUsed }), - comments: [], - fallbackBody: buildZeroFindingsBody({ model: modelUsed }), - } + // Step 12.5: cross-run dedup + let existingAnchors: Set + try { + const existingComments = await githubClient.fetchReviewComments({ + prNumber: prContext.prNumber, + }) + existingAnchors = extractAnchorKeys( + existingComments.map((comment) => comment.body), + ) + } catch (fetchError) { + const errorDetail = + fetchError instanceof Error + ? `[${fetchError.name}]: ${fetchError.message}` + : String(fetchError) + logger.warn("failed to fetch existing comments — treating as first run", { + error: errorDetail, + }) + existingAnchors = new Set() + } - const { url: reviewUrl } = await githubClient.submitReview({ - prNumber: prContext.prNumber, - commitId: prContext.headSha, - ...reviewPayload, - }) + const isRerun = existingAnchors.size > 0 + const isNewFinding = (finding: Finding): boolean => + !existingAnchors.has(computeAnchorKey(finding)) + const newFindings = isRerun ? selected.filter(isNewFinding) : selected - logger.info("review posted", { - reviewUrl, - findingsCount: selected.length, - modelUsed, + logger.info("cross-run dedup", { + existingAnchorCount: existingAnchors.size, + selectedCount: selected.length, + newFindingsCount: newFindings.length, + isRerun, }) - // Step 14: cost summary + // Step 13: post review const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) + if (!isRerun) { + const reviewPayload: ReviewPayload = + selected.length > 0 + ? buildReviewPayload({ + findings: selected, + commentableByPath, + droppedByCap, + modelUsed, + }) + : { + body: buildZeroFindingsBody({ model: modelUsed }), + comments: [], + fallbackBody: buildZeroFindingsBody({ model: modelUsed }), + } + + const { url: reviewUrl } = await githubClient.submitReview({ + prNumber: prContext.prNumber, + commitId: prContext.headSha, + ...reviewPayload, + }) + + logger.info("review posted", { + reviewUrl, + findingsCount: selected.length, + modelUsed, + }) + + return { + findingsCount: newFindings.length, + reviewUrl, + modelUsed, + skippedReason: "", + costSummaryMarkdown, + } + } + + // Re-run — post only new findings, upsert summary comment + const totalCount = existingAnchors.size + newFindings.length + let reviewUrl = "" + + if (newFindings.length > 0) { + const reviewPayload = buildReviewPayload({ + findings: newFindings, + commentableByPath, + droppedByCap: [], + modelUsed, + }) + + const result = await githubClient.submitReview({ + prNumber: prContext.prNumber, + commitId: prContext.headSha, + ...reviewPayload, + }) + reviewUrl = result.url + + logger.info("re-run review posted", { + reviewUrl, + newFindingsCount: newFindings.length, + totalCount, + }) + } else { + logger.info("re-run — no new findings", { totalCount }) + } + + const summaryBody = buildRerunSummary({ + sha: prContext.headSha, + newCount: newFindings.length, + totalCount, + model: modelUsed, + }) + try { + await githubClient.upsertSummaryComment({ + prNumber: prContext.prNumber, + body: summaryBody, + anchor: RERUN_ANCHOR, + }) + } catch (summaryError) { + const errorDetail = + summaryError instanceof Error + ? `[${summaryError.name}]: ${summaryError.message}` + : String(summaryError) + logger.warn("failed to upsert summary comment", { + error: errorDetail, + }) + } + return { - findingsCount: selected.length, + findingsCount: newFindings.length, reviewUrl, modelUsed, skippedReason: "", diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index b3ce9ad..2dcd1b8 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -1,9 +1,13 @@ import { describe, expect, it } from "vitest" import type { CommentableFile } from "../../diff/commentable-lines.js" import { + buildRerunSummary, buildReviewBody, buildZeroFindingsBody, + computeAnchorKey, + extractAnchorKeys, mapFindingsToReview, + RERUN_ANCHOR, } from "../comment-mapping.js" import { makeFinding } from "./make-finding.js" @@ -44,7 +48,9 @@ describe("mapFindingsToReview", () => { The guard rejects only the exact empty string. -**Failure scenario:** register(" ", "value") succeeds and the entry is orphaned.`, +**Failure scenario:** register(" ", "value") succeeds and the entry is orphaned. + +`, }, ]) }) @@ -126,6 +132,9 @@ The guard rejects only the exact empty string. expect(mapped.comments[0]?.body).toContain( "_Anchored near line 149 (the reported line is not part of the diff)._", ) + expect(mapped.comments[0]?.body).toContain( + ``, + ) }) it("snaps a finding exactly SNAP_DISTANCE (3) beyond the hunk end", () => { @@ -305,3 +314,116 @@ describe("buildZeroFindingsBody", () => { ) }) }) + +describe("computeAnchorKey", () => { + it("returns a deterministic key from file, category, and title hash", () => { + const finding = makeFinding() + const key = computeAnchorKey(finding) + + expect(key).toBe("src/greeter.ts:correctness:ffdf51bc") + expect(computeAnchorKey(finding)).toBe(key) + }) + + it("produces a different key when the title differs", () => { + const keyA = computeAnchorKey(makeFinding({ title: "Bug A" })) + const keyB = computeAnchorKey(makeFinding({ title: "Bug B" })) + + expect(keyA).not.toBe(keyB) + }) + + it("produces a different key when the file differs", () => { + const keyA = computeAnchorKey(makeFinding({ file: "src/a.ts" })) + const keyB = computeAnchorKey(makeFinding({ file: "src/b.ts" })) + + expect(keyA).not.toBe(keyB) + }) + + it("produces a different key when the category differs", () => { + const keyA = computeAnchorKey(makeFinding({ category: "correctness" })) + const keyB = computeAnchorKey(makeFinding({ category: "security" })) + + expect(keyA).not.toBe(keyB) + }) +}) + +describe("extractAnchorKeys", () => { + it("extracts keys from comment bodies with anchors", () => { + const bodies = [ + "Some comment body\n\n", + "Another body\n\n", + ] + + const keys = extractAnchorKeys(bodies) + + expect(keys).toEqual( + new Set(["src/a.ts:correctness:abcd1234", "src/b.ts:security:efgh5678"]), + ) + }) + + it("ignores bodies without anchors", () => { + const bodies = [ + "A comment from a human reviewer", + "CodeRabbit: some finding here", + "Body with\n\n", + ] + + const keys = extractAnchorKeys(bodies) + + expect(keys).toEqual(new Set(["src/a.ts:correctness:abcd1234"])) + }) + + it("returns an empty set for an empty array", () => { + expect(extractAnchorKeys([])).toEqual(new Set()) + }) +}) + +describe("buildRerunSummary", () => { + it("renders the summary with new findings", () => { + const summary = buildRerunSummary({ + sha: "abc123def456abc1", + newCount: 2, + totalCount: 5, + model: "anthropic/claude-sonnet-4-6", + }) + + expect(summary).toBe( + `${RERUN_ANCHOR}\n\n**umm-actually** re-reviewed at \`abc123d\`\n\n2 new finding(s) posted as inline comments (5 total across all reviews).\n\n---\n*umm-actually · anthropic/claude-sonnet-4-6*`, + ) + }) + + it("renders the summary with zero new findings", () => { + const summary = buildRerunSummary({ + sha: "abc123def456abc1", + newCount: 0, + totalCount: 3, + model: "anthropic/claude-sonnet-4-6", + }) + + expect(summary).toBe( + `${RERUN_ANCHOR}\n\n**umm-actually** re-reviewed at \`abc123d\`\n\nNo new findings (3 finding(s) from prior reviews).\n\n---\n*umm-actually · anthropic/claude-sonnet-4-6*`, + ) + }) + + it("truncates the sha to 7 characters", () => { + const summary = buildRerunSummary({ + sha: "0000000999999", + newCount: 1, + totalCount: 1, + model: "test/model", + }) + + expect(summary).toContain("`0000000`") + expect(summary).not.toContain("999999") + }) + + it("starts with the rerun anchor for upsert detection", () => { + const summary = buildRerunSummary({ + sha: "abc123d", + newCount: 0, + totalCount: 0, + model: "test/model", + }) + + expect(summary.startsWith(RERUN_ANCHOR)).toBe(true) + }) +}) diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 1e43acf..3e3c6d7 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -1,3 +1,4 @@ +import { createHash } from "node:crypto" import type { CommentableFile } from "../diff/commentable-lines.js" import type { Finding } from "./finding.js" @@ -24,6 +25,32 @@ export type MappedReview = { */ const SNAP_DISTANCE = 3 +/** Matches `` -- the hidden HTML anchor in each inline finding. Group 1 is the dedup key. */ +const ANCHOR_PATTERN = // + +/** Deterministic dedup key for a finding — file + category + 8-char title hash. */ +export const computeAnchorKey = ( + finding: Pick, +): string => { + const titleHash = createHash("sha256") + .update(finding.title) + .digest("hex") + .slice(0, 8) + return `${finding.file}:${finding.category}:${titleHash}` +} + +/** Extracts dedup keys from existing inline comment bodies. */ +export const extractAnchorKeys = (commentBodies: string[]): Set => { + const keys = new Set() + for (const body of commentBodies) { + const match = ANCHOR_PATTERN.exec(body) + if (match?.[1]) { + keys.add(match[1]) + } + } + return keys +} + const findingTag = (finding: Finding): string => `**[${finding.severity}/${finding.category}]** ${finding.title}` @@ -49,11 +76,12 @@ const renderCommentBody = ( snappedFromLine === undefined ? "" : `\n\n_Anchored near line ${snappedFromLine} (the reported line is not part of the diff)._` + const anchor = `\n\n` return `${findingTag(finding)} _(confidence: ${finding.confidence})_ ${finding.description} -**Failure scenario:** ${finding.failure_scenario}${suggestionBlock(finding)}${snapNote}` +**Failure scenario:** ${finding.failure_scenario}${suggestionBlock(finding)}${snapNote}${anchor}` } const nearestCommentableLine = ( @@ -226,3 +254,31 @@ export const buildReviewBody = ({ /** Body for the confirmation review posted when nothing crossed the threshold. */ export const buildZeroFindingsBody = ({ model }: { model: string }): string => `Reviewed — no findings above threshold.\n\n---\n*umm-actually · ${model}*` + +export const RERUN_ANCHOR = "" + +/** Body for the updatable issue comment posted on re-runs. */ +export const buildRerunSummary = ({ + sha, + newCount, + totalCount, + model, +}: { + sha: string + newCount: number + totalCount: number + model: string +}): string => { + const shaShort = sha.slice(0, 7) + const findingsLine = + newCount > 0 + ? `${newCount} new finding(s) posted as inline comments (${totalCount} total across all reviews).` + : `No new findings (${totalCount} finding(s) from prior reviews).` + const attribution = `---\n*umm-actually · ${model}*` + return [ + RERUN_ANCHOR, + `**umm-actually** re-reviewed at \`${shaShort}\``, + findingsLine, + attribution, + ].join("\n\n") +}