From 58086dc94678d4d6276dc1eea82eaf2fe9c8bff9 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:36:45 -0400 Subject: [PATCH 01/14] feat: cross-run finding dedup + updatable summary on re-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each push to a PR no longer duplicates review findings. Inline comment bodies now carry a hidden anchor (file:category:title-hash) for cross-run dedup. On re-runs, existing anchors are fetched and matched — only genuinely new findings get posted. An updatable issue comment summarizes re-run results (PATCHed in place on subsequent runs). Three-branch posting logic in orchestrate.ts: - First run: unchanged behavior (body + inline comments) - Re-run with new findings: inline-only review + upsert summary - Re-run with zero new: upsert summary only, no review posted GithubClient gains fetchReviewComments (paginated, 10-page cap) and upsertSummaryComment (search-by-anchor, create-or-update). The anchor fetch is wrapped in try/catch — degrades to first-run on failure. 235 tests (24 new), 0 lint warnings, clean build + Docker. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 156 ++++++++++++ src/github/__tests__/client.test.ts | 242 ++++++++++++++++++- src/github/client.ts | 164 ++++++++++++- src/orchestrate.ts | 142 +++++++++-- src/review/__tests__/comment-mapping.test.ts | 121 +++++++++- src/review/comment-mapping.ts | 57 ++++- 6 files changed, 853 insertions(+), 29 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 4a93268..3a2fea0 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,136 @@ 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() + + await orchestrate(stubs.deps, logger) + + 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).toContain("No new findings") + }) + + 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) + }) + + 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("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..71b8852 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,189 @@ 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).toHaveLength(2) + expect(stub.listReviewCommentsCalls[1]).toMatchObject({ page: 2 }) + }) + + 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("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..7bd7c12 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 @@ -63,6 +101,23 @@ const prResponseSchema = z.object({ const reviewUrlSchema = 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(), + }), +) + +const issueCommentResponseSchema = z.object({ html_url: z.string() }) + +const MAX_PAGES = 10 +const PER_PAGE = 100 + /** Octokit request errors carry a numeric `status` — duck-typed so stubs and * future octokit versions need no instanceof on octokit internals. */ const errorStatus = (error: unknown): number | undefined => { @@ -189,5 +244,112 @@ 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 existingComment: { id: number; html_url: string } | undefined + + 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") + } + existingComment = parsed.data.find((comment) => + comment.body.includes(anchor), + ) + if (existingComment !== undefined) break + if (parsed.data.length < PER_PAGE) break + if (page === MAX_PAGES) { + logger.warn("issue comments page cap reached", { + prNumber, + totalFetched: page * PER_PAGE, + }) + } + } + + if (existingComment !== undefined) { + const response = await octokit.rest.issues.updateComment({ + owner, + repo, + comment_id: existingComment.id, + body, + }) + const parsed = issueCommentResponseSchema.safeParse(response.data) + if (!parsed.success) { + throw new Error("unexpected issue comment response shape") + } + logger.info("updated summary comment", { prNumber }) + return { url: parsed.data.html_url, created: false } + } + + const response = await octokit.rest.issues.createComment({ + owner, + repo, + issue_number: prNumber, + body, + }) + const parsed = issueCommentResponseSchema.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..9d2393f 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,127 @@ 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) { + logger.warn("failed to fetch existing comments — treating as first run", { + error: + fetchError instanceof Error ? fetchError.message : String(fetchError), + }) + existingAnchors = new Set() + } - const { url: reviewUrl } = await githubClient.submitReview({ - prNumber: prContext.prNumber, - commitId: prContext.headSha, - ...reviewPayload, + const isRerun = existingAnchors.size > 0 + const newFindings = isRerun + ? selected.filter( + (finding) => !existingAnchors.has(computeAnchorKey(finding)), + ) + : selected + + logger.info("cross-run dedup", { + existingAnchorCount: existingAnchors.size, + selectedCount: selected.length, + newFindingsCount: newFindings.length, + isRerun, }) - logger.info("review posted", { - reviewUrl, - findingsCount: selected.length, - modelUsed, - }) + // Step 13: post review + let reviewUrl: string + + if (!isRerun) { + // First run — standard review with body + inline comments + const reviewPayload: ReviewPayload = + selected.length > 0 + ? buildReviewPayload({ + findings: selected, + commentableByPath, + droppedByCap, + modelUsed, + }) + : { + body: buildZeroFindingsBody({ model: modelUsed }), + comments: [], + fallbackBody: buildZeroFindingsBody({ model: modelUsed }), + } + + const result = await githubClient.submitReview({ + prNumber: prContext.prNumber, + commitId: prContext.headSha, + ...reviewPayload, + }) + reviewUrl = result.url + + logger.info("review posted", { + reviewUrl, + findingsCount: selected.length, + modelUsed, + }) + } else if (newFindings.length > 0) { + // Re-run with new findings — post only new ones + upsert summary + const reviewPayload = buildReviewPayload({ + findings: newFindings, + commentableByPath, + droppedByCap: [], + modelUsed, + }) + + const result = await githubClient.submitReview({ + prNumber: prContext.prNumber, + commitId: prContext.headSha, + ...reviewPayload, + }) + reviewUrl = result.url + + const summaryBody = buildRerunSummary({ + sha: prContext.headSha, + newCount: newFindings.length, + totalCount: existingAnchors.size + newFindings.length, + model: modelUsed, + }) + await githubClient.upsertSummaryComment({ + prNumber: prContext.prNumber, + body: summaryBody, + anchor: RERUN_ANCHOR, + }) + + logger.info("re-run review posted", { + reviewUrl, + newFindingsCount: newFindings.length, + totalCount: existingAnchors.size + newFindings.length, + }) + } else { + // Re-run with zero new findings — upsert summary only + reviewUrl = "" + + const summaryBody = buildRerunSummary({ + sha: prContext.headSha, + newCount: 0, + totalCount: existingAnchors.size, + model: modelUsed, + }) + await githubClient.upsertSummaryComment({ + prNumber: prContext.prNumber, + body: summaryBody, + anchor: RERUN_ANCHOR, + }) + + logger.info("re-run — no new findings", { + totalCount: existingAnchors.size, + }) + } // Step 14: cost summary const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) 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..a5b5beb 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. + +`, }, ]) }) @@ -305,3 +311,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..c93ef23 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,31 @@ export type MappedReview = { */ const SNAP_DISTANCE = 3 +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] !== undefined) { + keys.add(match[1]) + } + } + return keys +} + const findingTag = (finding: Finding): string => `**[${finding.severity}/${finding.category}]** ${finding.title}` @@ -49,11 +75,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 +253,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") +} From 4b01137c592dbca3a834b39286491a65eb239d64 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:46:17 -0400 Subject: [PATCH 02/14] fix(review): resilient summary comment + doc updates for cross-run dedup Wrap upsertSummaryComment in try/catch so a failed summary comment does not crash the action after the review is already posted. Update README and action.yml to reflect the shipped dedup feature: move from "In progress" to "Shipped", add dedup step to "How it works", and clarify findings_count output includes cross-run dedup. Co-Authored-By: Claude Opus 4.6 (1M context) --- README.md | 19 ++++++++++--------- action.yml | 2 +- src/orchestrate.ts | 38 ++++++++++++++++++++++++++++---------- 3 files changed, 39 insertions(+), 20 deletions(-) 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/orchestrate.ts b/src/orchestrate.ts index 9d2393f..0cb3a66 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -340,11 +340,20 @@ export const orchestrate = async ( totalCount: existingAnchors.size + newFindings.length, model: modelUsed, }) - await githubClient.upsertSummaryComment({ - prNumber: prContext.prNumber, - body: summaryBody, - anchor: RERUN_ANCHOR, - }) + try { + await githubClient.upsertSummaryComment({ + prNumber: prContext.prNumber, + body: summaryBody, + anchor: RERUN_ANCHOR, + }) + } catch (summaryError) { + logger.warn("failed to upsert summary comment — review already posted", { + error: + summaryError instanceof Error + ? summaryError.message + : String(summaryError), + }) + } logger.info("re-run review posted", { reviewUrl, @@ -361,11 +370,20 @@ export const orchestrate = async ( totalCount: existingAnchors.size, model: modelUsed, }) - await githubClient.upsertSummaryComment({ - prNumber: prContext.prNumber, - body: summaryBody, - anchor: RERUN_ANCHOR, - }) + try { + await githubClient.upsertSummaryComment({ + prNumber: prContext.prNumber, + body: summaryBody, + anchor: RERUN_ANCHOR, + }) + } catch (summaryError) { + logger.warn("failed to upsert summary comment", { + error: + summaryError instanceof Error + ? summaryError.message + : String(summaryError), + }) + } logger.info("re-run — no new findings", { totalCount: existingAnchors.size, From 376e995e996fa23f086c787746ad26607b26dd78 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:52:34 -0400 Subject: [PATCH 03/14] style: doc comment on regex constant, deduplicate summary upsert block Co-Authored-By: Claude Opus 4.6 (1M context) --- src/orchestrate.ts | 40 ++++++++++------------------------- src/review/comment-mapping.ts | 1 + 2 files changed, 12 insertions(+), 29 deletions(-) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 0cb3a66..b12f4a4 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -319,7 +319,7 @@ export const orchestrate = async ( modelUsed, }) } else if (newFindings.length > 0) { - // Re-run with new findings — post only new ones + upsert summary + // Re-run with new findings — post only new ones const reviewPayload = buildReviewPayload({ findings: newFindings, commentableByPath, @@ -334,40 +334,26 @@ export const orchestrate = async ( }) reviewUrl = result.url - const summaryBody = buildRerunSummary({ - sha: prContext.headSha, - newCount: newFindings.length, - totalCount: existingAnchors.size + newFindings.length, - model: modelUsed, - }) - try { - await githubClient.upsertSummaryComment({ - prNumber: prContext.prNumber, - body: summaryBody, - anchor: RERUN_ANCHOR, - }) - } catch (summaryError) { - logger.warn("failed to upsert summary comment — review already posted", { - error: - summaryError instanceof Error - ? summaryError.message - : String(summaryError), - }) - } - logger.info("re-run review posted", { reviewUrl, newFindingsCount: newFindings.length, totalCount: existingAnchors.size + newFindings.length, }) } else { - // Re-run with zero new findings — upsert summary only + // Re-run with zero new findings reviewUrl = "" + logger.info("re-run — no new findings", { + totalCount: existingAnchors.size, + }) + } + + // Upsert summary comment on re-runs + if (isRerun) { const summaryBody = buildRerunSummary({ sha: prContext.headSha, - newCount: 0, - totalCount: existingAnchors.size, + newCount: newFindings.length, + totalCount: existingAnchors.size + newFindings.length, model: modelUsed, }) try { @@ -384,10 +370,6 @@ export const orchestrate = async ( : String(summaryError), }) } - - logger.info("re-run — no new findings", { - totalCount: existingAnchors.size, - }) } // Step 14: cost summary diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index c93ef23..2531dfb 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -25,6 +25,7 @@ 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. */ From e145683fa8346c1c7275fdfae4a711497518a756 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:59:03 -0400 Subject: [PATCH 04/14] test: strengthen assertions and cover error paths in dedup tests Replaced substring assertions (toContain, toMatchObject) with exact full-value matches on deterministic outputs. Added missing error-path tests for upsertSummaryComment (malformed list, create, update responses) and a resilience test proving orchestrate continues when upsert fails. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 42 ++++++++++++++++++- src/github/__tests__/client.test.ts | 63 ++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 3a2fea0..80705cb 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -753,7 +753,14 @@ describe("orchestrate", () => { expect(stubs.upsertSummaryCommentCalls).toHaveLength(1) const summaryCall = first(stubs.upsertSummaryCommentCalls) - expect(summaryCall.body).toContain("No new findings") + 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 () => { @@ -777,6 +784,16 @@ describe("orchestrate", () => { 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 () => { @@ -792,6 +809,29 @@ describe("orchestrate", () => { 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: { diff --git a/src/github/__tests__/client.test.ts b/src/github/__tests__/client.test.ts index 71b8852..1c792e2 100644 --- a/src/github/__tests__/client.test.ts +++ b/src/github/__tests__/client.test.ts @@ -477,8 +477,22 @@ describe("fetchReviewComments", () => { const comments = await client.fetchReviewComments({ prNumber: 7 }) expect(comments).toHaveLength(101) - expect(stub.listReviewCommentsCalls).toHaveLength(2) - expect(stub.listReviewCommentsCalls[1]).toMatchObject({ page: 2 }) + 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("throws on a malformed response", async () => { @@ -601,6 +615,51 @@ describe("upsertSummaryComment", () => { ]) }) + 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: [] }], From 3879ca71d76ab3329d0dba45482d220a7c675168 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 18:08:00 -0400 Subject: [PATCH 05/14] test: add MAX_PAGES cap test and findingsCount first-run assertion Addresses Sourcery review findings: add a test that exercises the 10-page pagination cap with warning log, and assert findingsCount on the first-run dedup test to guard the new return value semantics. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/__tests__/orchestrate.test.ts | 3 ++- src/github/__tests__/client.test.ts | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 80705cb..57a63c0 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -685,8 +685,9 @@ describe("orchestrate", () => { const stubs = makeOrchestrateDeps() const logger = createTestLogger() - await orchestrate(stubs.deps, logger) + 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) diff --git a/src/github/__tests__/client.test.ts b/src/github/__tests__/client.test.ts index 1c792e2..39bfe1d 100644 --- a/src/github/__tests__/client.test.ts +++ b/src/github/__tests__/client.test.ts @@ -495,6 +495,27 @@ describe("fetchReviewComments", () => { ]) }) + 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" }], From 90997a732ea2ac28ed283c79d2b38c43449a6f68 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:43:08 -0400 Subject: [PATCH 06/14] refactor: use truthy check over explicit undefined comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `if (existingComment)` over `if (existingComment !== undefined)` — the type is `T | undefined`, so the only falsy case is undefined. Explicit undefined checks are only warranted when the value could be another falsy type (null, 0, empty string, false). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/github/client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/github/client.ts b/src/github/client.ts index 7bd7c12..51830f7 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -306,7 +306,7 @@ export const createGithubClient = ( existingComment = parsed.data.find((comment) => comment.body.includes(anchor), ) - if (existingComment !== undefined) break + if (existingComment) break if (parsed.data.length < PER_PAGE) break if (page === MAX_PAGES) { logger.warn("issue comments page cap reached", { @@ -316,7 +316,7 @@ export const createGithubClient = ( } } - if (existingComment !== undefined) { + if (existingComment) { const response = await octokit.rest.issues.updateComment({ owner, repo, From 940534280d4f7d6d8f48670519fb9561afc0277d Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:44:48 -0400 Subject: [PATCH 07/14] refactor: simplify truthy checks and extract totalCount - extractAnchorKeys: `match?.[1]` over `match?.[1] !== undefined` - orchestrate: extract `totalCount` const to avoid duplicate computation Co-Authored-By: Claude Opus 4.6 (1M context) --- src/orchestrate.ts | 5 +++-- src/review/comment-mapping.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index b12f4a4..4eec5ba 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -288,6 +288,7 @@ export const orchestrate = async ( }) // Step 13: post review + const totalCount = existingAnchors.size + newFindings.length let reviewUrl: string if (!isRerun) { @@ -337,7 +338,7 @@ export const orchestrate = async ( logger.info("re-run review posted", { reviewUrl, newFindingsCount: newFindings.length, - totalCount: existingAnchors.size + newFindings.length, + totalCount: totalCount, }) } else { // Re-run with zero new findings @@ -353,7 +354,7 @@ export const orchestrate = async ( const summaryBody = buildRerunSummary({ sha: prContext.headSha, newCount: newFindings.length, - totalCount: existingAnchors.size + newFindings.length, + totalCount: totalCount, model: modelUsed, }) try { diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 2531dfb..3e3c6d7 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -44,7 +44,7 @@ export const extractAnchorKeys = (commentBodies: string[]): Set => { const keys = new Set() for (const body of commentBodies) { const match = ANCHOR_PATTERN.exec(body) - if (match?.[1] !== undefined) { + if (match?.[1]) { keys.add(match[1]) } } From fa283e56ce6ca7f49db7d97eca4413cbb3435f96 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:46:30 -0400 Subject: [PATCH 08/14] docs: codify truthy check preference in AGENTS.md Prefer `if (value)` over `if (value !== undefined)` unless distinguishing undefined from other falsy values matters. Co-Authored-By: Claude Opus 4.6 (1M context) --- AGENTS.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 41a2a55..8e852de 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,7 +57,10 @@ 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. From 01694a58e55bb96d81923d92c18b14af6e431c3a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:49:55 -0400 Subject: [PATCH 09/14] refactor: use [ErrorName]: message format in catch blocks Extract error formatting into a named const and include error.name for immediate context in log output. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/orchestrate.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 4eec5ba..f72672d 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -266,9 +266,12 @@ export const orchestrate = async ( 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: - fetchError instanceof Error ? fetchError.message : String(fetchError), + error: errorDetail, }) existingAnchors = new Set() } @@ -364,11 +367,12 @@ export const orchestrate = async ( anchor: RERUN_ANCHOR, }) } catch (summaryError) { + const errorDetail = + summaryError instanceof Error + ? `[${summaryError.name}]: ${summaryError.message}` + : String(summaryError) logger.warn("failed to upsert summary comment", { - error: - summaryError instanceof Error - ? summaryError.message - : String(summaryError), + error: errorDetail, }) } } From 3f41b5a4e57b623cb793cb3b83988fe543e93608 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:52:36 -0400 Subject: [PATCH 10/14] refactor: early return from first-run path to flatten re-run logic Return early from the !isRerun branch so re-run logic flows linearly without nesting. Eliminates the 3-branch if/else if/else and the separate if (isRerun) guard around summary upsert. Also strengthens the early-return convention in AGENTS.md. Co-Authored-By: Claude Opus 4.6 (1M context) --- AGENTS.md | 7 +++-- src/orchestrate.ts | 77 +++++++++++++++++++++++----------------------- 2 files changed, 43 insertions(+), 41 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8e852de..53e0d53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,8 +65,11 @@ files. Prefer SDK-provided types over redefining shapes. 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/src/orchestrate.ts b/src/orchestrate.ts index f72672d..ceb3847 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -291,11 +291,9 @@ export const orchestrate = async ( }) // Step 13: post review - const totalCount = existingAnchors.size + newFindings.length - let reviewUrl: string + const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) if (!isRerun) { - // First run — standard review with body + inline comments const reviewPayload: ReviewPayload = selected.length > 0 ? buildReviewPayload({ @@ -310,20 +308,32 @@ export const orchestrate = async ( fallbackBody: buildZeroFindingsBody({ model: modelUsed }), } - const result = await githubClient.submitReview({ + const { url: reviewUrl } = await githubClient.submitReview({ prNumber: prContext.prNumber, commitId: prContext.headSha, ...reviewPayload, }) - reviewUrl = result.url logger.info("review posted", { reviewUrl, findingsCount: selected.length, modelUsed, }) - } else if (newFindings.length > 0) { - // Re-run with new findings — post only new ones + + 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, @@ -341,45 +351,34 @@ export const orchestrate = async ( logger.info("re-run review posted", { reviewUrl, newFindingsCount: newFindings.length, - totalCount: totalCount, + totalCount, }) } else { - // Re-run with zero new findings - reviewUrl = "" - - logger.info("re-run — no new findings", { - totalCount: existingAnchors.size, - }) + logger.info("re-run — no new findings", { totalCount }) } - // Upsert summary comment on re-runs - if (isRerun) { - const summaryBody = buildRerunSummary({ - sha: prContext.headSha, - newCount: newFindings.length, - totalCount: totalCount, - model: modelUsed, + 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, }) - 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, - }) - } } - // Step 14: cost summary - const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) - return { findingsCount: newFindings.length, reviewUrl, From 533890516f7e55f6fa645efbbcbc3e20181bdf5e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 21:09:22 -0400 Subject: [PATCH 11/14] fix: snapped-finding anchor test gap + totalFetched consistency - Add anchor assertion to the snapped-finding test so the dedup anchor is verified on comments that include a snap note - Track actual totalFetched count in upsertSummaryComment instead of page * PER_PAGE, matching fetchReviewComments' pattern Co-Authored-By: Claude Opus 4.6 (1M context) --- src/github/client.ts | 4 +++- src/review/__tests__/comment-mapping.test.ts | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/github/client.ts b/src/github/client.ts index 51830f7..3518bad 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -290,6 +290,7 @@ export const createGithubClient = ( anchor: string }): Promise => { let existingComment: { id: number; html_url: string } | undefined + let totalFetched = 0 for (let page = 1; page <= MAX_PAGES; page++) { const response = await octokit.rest.issues.listComments({ @@ -303,6 +304,7 @@ export const createGithubClient = ( if (!parsed.success) { throw new Error("unexpected issue comments response shape") } + totalFetched += parsed.data.length existingComment = parsed.data.find((comment) => comment.body.includes(anchor), ) @@ -311,7 +313,7 @@ export const createGithubClient = ( if (page === MAX_PAGES) { logger.warn("issue comments page cap reached", { prNumber, - totalFetched: page * PER_PAGE, + totalFetched, }) } } diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index a5b5beb..2dcd1b8 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -132,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", () => { From 31becc10af5896df3ab2619927d6e63b80ae5e39 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 21:36:31 -0400 Subject: [PATCH 12/14] refactor: scope MAX_PAGES and PER_PAGE to createGithubClient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These constants are only used by fetchReviewComments and upsertSummaryComment — both inside the factory closure. Module-level overstates their visibility. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/github/client.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/github/client.ts b/src/github/client.ts index 3518bad..bd59657 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -115,9 +115,6 @@ const issueCommentListSchema = z.array( const issueCommentResponseSchema = z.object({ html_url: z.string() }) -const MAX_PAGES = 10 -const PER_PAGE = 100 - /** Octokit request errors carry a numeric `status` — duck-typed so stubs and * future octokit versions need no instanceof on octokit internals. */ const errorStatus = (error: unknown): number | undefined => { @@ -134,6 +131,9 @@ 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) if (!parsed.success) throw new Error("unexpected review response shape") From 843eda635fabf5b4f3af02e1bb2d43840a75991e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 21:41:11 -0400 Subject: [PATCH 13/14] refactor: simplify upsertSummaryComment and consolidate URL schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Return early from inside the pagination loop when the anchor comment is found, eliminating the let + break + post-loop if pattern - Consolidate reviewUrlSchema and issueCommentResponseSchema into one urlResponseSchema — both validate the same { html_url } shape Co-Authored-By: Claude Opus 4.6 (1M context) --- src/github/client.ts | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/src/github/client.ts b/src/github/client.ts index bd59657..5169726 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -99,7 +99,7 @@ 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() }), @@ -113,8 +113,6 @@ const issueCommentListSchema = z.array( }), ) -const issueCommentResponseSchema = z.object({ 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. */ const errorStatus = (error: unknown): number | undefined => { @@ -135,7 +133,7 @@ export const createGithubClient = ( 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 } @@ -289,7 +287,6 @@ export const createGithubClient = ( body: string anchor: string }): Promise => { - let existingComment: { id: number; html_url: string } | undefined let totalFetched = 0 for (let page = 1; page <= MAX_PAGES; page++) { @@ -305,10 +302,25 @@ export const createGithubClient = ( throw new Error("unexpected issue comments response shape") } totalFetched += parsed.data.length - existingComment = parsed.data.find((comment) => + + const existingComment = parsed.data.find((comment) => comment.body.includes(anchor), ) - if (existingComment) break + 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", { @@ -318,28 +330,13 @@ export const createGithubClient = ( } } - if (existingComment) { - const response = await octokit.rest.issues.updateComment({ - owner, - repo, - comment_id: existingComment.id, - body, - }) - const parsed = issueCommentResponseSchema.safeParse(response.data) - if (!parsed.success) { - throw new Error("unexpected issue comment response shape") - } - logger.info("updated summary comment", { prNumber }) - return { url: parsed.data.html_url, created: false } - } - const response = await octokit.rest.issues.createComment({ owner, repo, issue_number: prNumber, body, }) - const parsed = issueCommentResponseSchema.safeParse(response.data) + const parsed = urlResponseSchema.safeParse(response.data) if (!parsed.success) { throw new Error("unexpected issue comment response shape") } From 77b9c308ad56e167d4c9448d9e29afc25209e468 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 21:46:29 -0400 Subject: [PATCH 14/14] refactor: extract isNewFinding predicate for readability Co-Authored-By: Claude Opus 4.6 (1M context) --- src/orchestrate.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index ceb3847..f60406d 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -277,11 +277,9 @@ export const orchestrate = async ( } const isRerun = existingAnchors.size > 0 - const newFindings = isRerun - ? selected.filter( - (finding) => !existingAnchors.has(computeAnchorKey(finding)), - ) - : selected + const isNewFinding = (finding: Finding): boolean => + !existingAnchors.has(computeAnchorKey(finding)) + const newFindings = isRerun ? selected.filter(isNewFinding) : selected logger.info("cross-run dedup", { existingAnchorCount: existingAnchors.size,