diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fd1cbb..8e364ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Docs reframed for dual GitLab + GitHub support: platform auto-detection, GitHub environment/flags, both output transports, and the composite-action vs reusable-workflow `secrets: inherit` caveat ([#124]). +### Fixed + +- GitHub: resolved review threads are now recognized, so the summary no longer re-lists findings that were already addressed. Thread resolution is read from the GraphQL `reviewThreads` API (GitHub's REST comment endpoints omit it), fixing both summary carry-over and prior-thread context ([#125]). + ## [0.8.2] - 2026-07-15 ### Added @@ -41,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING**: renamed the product-scoped environment-variable prefix `GITLAB_REVIEW_* → CODE_REVIEW_*` (e.g. `GITLAB_REVIEW_MODEL → CODE_REVIEW_MODEL`, and the namespacing shim that de-prefixes provider/infra vars in shared CI) with no backward compatibility — the old names are no longer read, so existing CI configs must rename their variables. Unprefixed GitLab tokens (`GITLAB_TOKEN`, `CI_JOB_TOKEN`, …) are unchanged ([#121]). [Unreleased]: https://github.com/weareikko/code-review/compare/0.8.2...HEAD +[#125]: https://github.com/weareikko/code-review/pull/125 [#124]: https://github.com/weareikko/code-review/pull/124 [0.8.2]: https://github.com/weareikko/code-review/compare/0.8.1...0.8.2 [#123]: https://github.com/weareikko/code-review/pull/123 diff --git a/src/cli.integration.test.ts b/src/cli.integration.test.ts index 3644bd0..d04cd07 100644 --- a/src/cli.integration.test.ts +++ b/src/cli.integration.test.ts @@ -99,6 +99,9 @@ function makeGitHubBackend() { const reviewComments: GitHubReviewComment[] = []; const issueComments: GitHubComment[] = []; const reviewsPosted: Array<{ commit_id: string; comments?: GitHubReviewComment[] }> = []; + // Ids of review comments whose thread is resolved. REST omits resolution, so + // tests seed this to model GitHub's GraphQL `reviewThreads.isResolved`. + const resolvedCommentIds = new Set(); let nextId = 1000; const jsonResponse = (value: unknown): Response => new Response(JSON.stringify(value), { headers: { 'content-type': 'application/json' } }); @@ -108,6 +111,26 @@ function makeGitHubBackend() { const path = new URL(url).pathname; const bodyJson = typeof init.body === 'string' ? JSON.parse(init.body) : undefined; + // Thread resolution lives only in GraphQL; each inline comment is its own + // single-comment thread, resolved when its id is in `resolvedCommentIds`. + if (method === 'POST' && path.endsWith('/graphql')) { + return jsonResponse({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: reviewComments.map((c) => ({ + isResolved: resolvedCommentIds.has(c.id), + comments: { nodes: [{ databaseId: c.id }] }, + })), + }, + }, + }, + }, + }); + } + if (method === 'GET' && path.endsWith('/pulls/1')) { return jsonResponse({ head: { ref: 'feature', sha: HEAD_SHA }, @@ -145,7 +168,7 @@ function makeGitHubBackend() { return new Response('not found', { status: 404 }); }); - return { fetchImpl, reviewComments, issueComments, reviewsPosted }; + return { fetchImpl, reviewComments, issueComments, reviewsPosted, resolvedCommentIds }; } function makeConfig(cwd: string, overrides: Partial = {}): Config { @@ -341,4 +364,46 @@ describe('run() over GitHub with an unresolved prior finding', () => { expect(backend.issueComments[0].body).toContain('Still open from earlier reviews'); expect(backend.issueComments[0].body).toContain('src/foo.ts:3'); }); + + it('does not carry a resolved prior finding into the summary note', async () => { + const backend = makeGitHubBackend(); + + // Same seed as above, but the thread is resolved on GitHub. Resolution lives + // only in GraphQL, so REST alone can't see it — the fix reads `isResolved` + // and must therefore drop this finding from the "Still open" block. + const [prior] = buildGitHubComments( + [ + { + file: 'src/foo.ts', + line: 3, + side: 'RIGHT', + severity: 'warn', + confidence: 'high', + body: 'issue: an earlier concern that was addressed', + }, + ], + fixtures.defaultDiff, + REFS, + new Set(), + ); + const priorBody = (prior.payload as GitHubReviewCommentPayload).body; + backend.reviewComments.push({ + id: 500, + path: 'src/foo.ts', + line: 3, + side: 'RIGHT', + body: priorBody, + }); + backend.resolvedCommentIds.add(500); + + vi.stubGlobal('fetch', backend.fetchImpl); + + const result = await run(makeConfig(cwd, { forceReview: true })); + + expect(result.summary?.action).toBe('created'); + expect(backend.issueComments).toHaveLength(1); + // The resolved finding is gone — no stale "Still open" reference to it. + expect(backend.issueComments[0].body).not.toContain('Still open from earlier reviews'); + expect(backend.issueComments[0].body).not.toContain('addressed'); + }); }); diff --git a/src/github.test.ts b/src/github.test.ts index bdc733b..a0bfbaf 100644 --- a/src/github.test.ts +++ b/src/github.test.ts @@ -185,6 +185,101 @@ describe('GitHub write endpoints', () => { }); }); +function graphqlThreadsResponse( + nodes: { isResolved: boolean; ids: number[] }[], + pageInfo: { hasNextPage: boolean; endCursor: string | null } = { + hasNextPage: false, + endCursor: null, + }, +): Response { + return new Response( + JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo, + nodes: nodes.map((n) => ({ + isResolved: n.isResolved, + comments: { nodes: n.ids.map((id) => ({ databaseId: id })) }, + })), + }, + }, + }, + }, + }), + ); +} + +describe('GitHub resolved review-thread ids (GraphQL)', () => { + it('returns only the comment ids of resolved threads', async () => { + const fetchImpl = vi.fn().mockResolvedValue( + graphqlThreadsResponse([ + { isResolved: true, ids: [1, 2] }, + { isResolved: false, ids: [3] }, + ]), + ); + const client = new GitHubClient({ token: 't', fetchImpl }); + + const resolved = await client.listResolvedReviewCommentIds('o', 'r', 7); + + expect([...resolved].toSorted((a, b) => a - b)).toEqual([1, 2]); + expect(fetchImpl.mock.calls[0][0]).toBe('https://api.github.com/graphql'); + const init = fetchImpl.mock.calls[0][1]; + expect(init.method).toBe('POST'); + const body = JSON.parse(init.body as string); + expect(body.variables).toEqual({ owner: 'o', repo: 'r', pull: 7, cursor: null }); + expect(body.query).toContain('reviewThreads'); + }); + + it('targets the /api/graphql endpoint on GitHub Enterprise Server', async () => { + const fetchImpl = vi.fn().mockResolvedValue(graphqlThreadsResponse([])); + const client = new GitHubClient({ + apiUrl: 'https://ghe.example.com/api/v3', + token: 't', + fetchImpl, + }); + + await client.listResolvedReviewCommentIds('o', 'r', 7); + + expect(fetchImpl.mock.calls[0][0]).toBe('https://ghe.example.com/api/graphql'); + }); + + it('paginates across review-thread pages', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + graphqlThreadsResponse([{ isResolved: true, ids: [1] }], { + hasNextPage: true, + endCursor: 'CUR', + }), + ) + .mockResolvedValueOnce(graphqlThreadsResponse([{ isResolved: true, ids: [2] }])); + const client = new GitHubClient({ token: 't', fetchImpl }); + + const resolved = await client.listResolvedReviewCommentIds('o', 'r', 7); + + expect([...resolved].toSorted((a, b) => a - b)).toEqual([1, 2]); + expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(JSON.parse(fetchImpl.mock.calls[1][1].body as string).variables.cursor).toBe('CUR'); + }); + + it('throws a typed GitHubApiError when GraphQL returns errors', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValue( + new Response(JSON.stringify({ errors: [{ message: 'Bad credentials' }] })), + ); + const client = new GitHubClient({ token: 't', fetchImpl }); + + await expect(client.listResolvedReviewCommentIds('o', 'r', 7)).rejects.toMatchObject({ + name: 'GitHubApiError', + method: 'POST', + path: '/graphql', + }); + }); +}); + describe('GitHub client error handling', () => { it('throws a typed GitHubApiError with status and body on failure', async () => { const fetchImpl = vi diff --git a/src/github.ts b/src/github.ts index de2e7bc..146ee92 100644 --- a/src/github.ts +++ b/src/github.ts @@ -121,6 +121,36 @@ export interface Review { id: number; } +/** Minimal shape of the `reviewThreads` GraphQL query response we consume. */ +interface ReviewThreadsResponse { + repository?: { + pullRequest?: { + reviewThreads?: { + pageInfo?: { hasNextPage?: boolean; endCursor?: string | null }; + nodes?: { + isResolved?: boolean; + comments?: { nodes?: { databaseId?: number | null }[] }; + }[]; + }; + }; + } | null; +} + +const REVIEW_THREADS_QUERY = ` + query ($owner: String!, $repo: String!, $pull: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pull) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved + comments(first: 100) { nodes { databaseId } } + } + } + } + } + }`; + export class GitHubClient { private readonly base: string; private readonly token: string; @@ -332,4 +362,81 @@ export class GitHubClient { getCurrentUser(): Promise { return this.request('/user'); } + + /** + * Derive the GraphQL endpoint from the REST base. github.com exposes GraphQL + * at `/graphql`, while GitHub Enterprise Server exposes it at + * `/api/graphql` (its REST base is `/api/v3`). + */ + private graphqlEndpoint(): string { + const suffix = '/api/v3'; + if (this.base.endsWith(suffix)) return `${this.base.slice(0, -suffix.length)}/api/graphql`; + return `${this.base}/graphql`; + } + + private async graphql(query: string, variables: Record): Promise { + const url = this.graphqlEndpoint(); + const response = await this.fetchWithTimeout( + url, + { + method: 'POST', + headers: this.headers({ 'Content-Type': 'application/json' }), + body: JSON.stringify({ query, variables }), + }, + 'POST', + '/graphql', + ); + if (!response.ok) this.failure('POST', '/graphql', response, await response.text()); + const text = await response.text(); + const parsed = JSON.parse(text) as { data?: T; errors?: { message?: string }[] }; + if (parsed.errors && parsed.errors.length > 0) { + throw new GitHubApiError( + `GitHub API POST /graphql failed: ${parsed.errors.map((e) => e.message ?? '').join('; ')}`, + { + method: 'POST', + path: '/graphql', + responseBody: text, + hint: 'Ensure the token can read pull-request review threads (pull-requests: read / repo scope).', + }, + ); + } + return parsed.data as T; + } + + /** + * Return the database IDs of review comments that belong to a **resolved** + * review thread. GitHub's REST comment endpoints omit thread-resolution state; + * it is only exposed via GraphQL `reviewThreads.isResolved`. Callers use this + * set to mark normalized notes resolved so resolved threads are excluded from + * summary carry-over and prior-thread context. Paginates over threads. + */ + async listResolvedReviewCommentIds( + owner: string, + repo: string, + pull: number, + ): Promise> { + const resolved = new Set(); + let cursor: string | null = null; + let hasNext = true; + while (hasNext) { + const data: ReviewThreadsResponse = await this.graphql(REVIEW_THREADS_QUERY, { + owner, + repo, + pull, + cursor, + }); + const threads = data.repository?.pullRequest?.reviewThreads; + if (!threads) break; + for (const thread of threads.nodes ?? []) { + if (!thread.isResolved) continue; + for (const comment of thread.comments?.nodes ?? []) { + if (typeof comment.databaseId === 'number') resolved.add(comment.databaseId); + } + } + hasNext = threads.pageInfo?.hasNextPage ?? false; + cursor = threads.pageInfo?.endCursor ?? null; + if (!cursor) hasNext = false; + } + return resolved; + } } diff --git a/src/platforms/github.test.ts b/src/platforms/github.test.ts index de64f4c..0653f40 100644 --- a/src/platforms/github.test.ts +++ b/src/platforms/github.test.ts @@ -3,6 +3,7 @@ import { ConfigError } from '../errors.js'; import { extractExistingFingerprints } from '../fingerprints.js'; import { findExistingReviewedCommitSha, findExistingSummaryNote } from '../posting.js'; import { extractPriorThreads } from '../prior-threads.js'; +import { extractOpenBotFindings } from '../summary-carryover.js'; import type { DiffRefs, ReviewComment } from '../types.js'; import { buildGitHubComments, @@ -178,6 +179,29 @@ describe('normalizeGitHubDiscussions', () => { expect(discussions[0].notes).toEqual([{ id: 10, body: 'summary here' }]); }); + it('marks notes resolved when their id is in the resolved set', () => { + const discussions = normalizeGitHubDiscussions( + [ + { id: 1, body: 'resolved finding', path: 'src/a.ts', line: 3, side: 'RIGHT' }, + { id: 2, body: 'reply', path: 'src/a.ts', in_reply_to_id: 1 }, + { id: 3, body: 'open finding', path: 'src/b.ts', line: 4, side: 'RIGHT' }, + ], + [], + new Set([1, 2]), + ); + // The resolved thread's notes are both flagged; the open thread stays false. + expect(discussions[0].notes.map((n) => n.resolved)).toEqual([true, true]); + expect(discussions[1].notes.map((n) => n.resolved)).toEqual([false]); + }); + + it('defaults notes to unresolved when no resolved set is passed', () => { + const [discussion] = normalizeGitHubDiscussions( + [{ id: 1, body: 'x', path: 'src/a.ts', line: 3, side: 'RIGHT' }], + [], + ); + expect(discussion.notes[0].resolved).toBe(false); + }); + it('feeds the shared fingerprint / summary / prior-thread helpers unchanged', () => { // Build a real bot comment body (footer + fingerprint markers) via the payload builder. const [generated] = buildGitHubComments([comment()], DIFF, REFS, new Set()); @@ -213,8 +237,23 @@ describe('normalizeGitHubDiscussions', () => { }); }); +/** + * A resolved review thread's comment ids, shaped as the GraphQL `reviewThreads` + * query returns them. `ids` land in the resolved set only when `isResolved`. + */ +interface ResolvedThreadStub { + isResolved: boolean; + ids: number[]; +} + /** Routes GitHub API requests by method + path so one mock backs a whole run. */ -function routedFetch(overrides: { reviewComments?: unknown[]; issueComments?: unknown[] } = {}): { +function routedFetch( + overrides: { + reviewComments?: unknown[]; + issueComments?: unknown[]; + reviewThreads?: ResolvedThreadStub[]; + } = {}, +): { fetchImpl: ReturnType; calls: { method: string; url: string; body?: string }[]; } { @@ -222,6 +261,25 @@ function routedFetch(overrides: { reviewComments?: unknown[]; issueComments?: un const fetchImpl = vi.fn(async (url: string, init: RequestInit) => { const method = init.method ?? 'GET'; calls.push({ method, url, body: init.body as string | undefined }); + if (method === 'POST' && url.endsWith('/graphql')) { + return new Response( + JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: (overrides.reviewThreads ?? []).map((t) => ({ + isResolved: t.isResolved, + comments: { nodes: t.ids.map((id) => ({ databaseId: id })) }, + })), + }, + }, + }, + }, + }), + ); + } if (method === 'GET' && url.endsWith('/pulls/7')) { return new Response( JSON.stringify({ @@ -306,6 +364,26 @@ describe('GitHubPlatform', () => { expect(discussions[1].notes[0].id).toBe(10); }); + it('getDiscussions flags resolved threads so carry-over drops them', async () => { + // Two bot findings with fingerprint markers; only the first thread is resolved. + const resolvedBody = '**issue: resolved bug**\n\n'; + const openBody = '**issue: open bug**\n\n'; + const { fetchImpl } = routedFetch({ + reviewComments: [ + { id: 1, body: resolvedBody, path: 'src/a.ts', line: 3, side: 'RIGHT' }, + { id: 2, body: openBody, path: 'src/b.ts', line: 4, side: 'RIGHT' }, + ], + reviewThreads: [{ isResolved: true, ids: [1] }], + }); + + const discussions = await makePlatform(fetchImpl).getDiscussions(); + + // extractOpenBotFindings keeps only the still-open finding, mirroring GitLab. + const open = extractOpenBotFindings(discussions); + expect(open).toHaveLength(1); + expect(open[0].file).toBe('src/b.ts'); + }); + it('postComments posts one batched review with commit_id, skipping duplicates and off-diff', async () => { const { fetchImpl, calls } = routedFetch(); const platform = makePlatform(fetchImpl); diff --git a/src/platforms/github.ts b/src/platforms/github.ts index df51a69..8724f85 100644 --- a/src/platforms/github.ts +++ b/src/platforms/github.ts @@ -188,10 +188,15 @@ function reviewCommentPosition(comment: PullRequestReviewComment): DiscussionNot * The fingerprint markers, summary marker, and reviewed-commit footer are HTML * comments that render identically on GitHub, so `extractExistingFingerprints`, * `findExistingSummaryNote`, and the reviewed-commit scan all work as-is. + * + * `resolvedCommentIds` carries the database ids of comments in resolved review + * threads (from the GraphQL `reviewThreads` query, since REST omits resolution), + * so each note gets a `resolved` flag mirroring GitLab's per-note field. */ export function normalizeGitHubDiscussions( reviewComments: PullRequestReviewComment[], issueComments: IssueComment[], + resolvedCommentIds: Set = new Set(), ): Discussion[] { const threads = new Map(); const order: number[] = []; @@ -207,6 +212,11 @@ export function normalizeGitHubDiscussions( notes.push({ id: comment.id, body: comment.body ?? '', + // GitHub's REST comments carry no resolution state; it comes from the + // GraphQL `reviewThreads` query, keyed by comment database id. A resolved + // thread marks all its comments, so any note being resolved flags the + // whole thread for the shared `notes.some((n) => n.resolved)` checks. + resolved: resolvedCommentIds.has(comment.id), position: reviewCommentPosition(comment), }); } @@ -294,11 +304,12 @@ export class GitHubPlatform implements ReviewPlatform { } async getDiscussions(): Promise { - const [reviewComments, issueComments] = await Promise.all([ + const [reviewComments, issueComments, resolvedCommentIds] = await Promise.all([ this.client.listReviewComments(this.owner, this.repo, this.pull), this.client.listIssueComments(this.owner, this.repo, this.pull), + this.client.listResolvedReviewCommentIds(this.owner, this.repo, this.pull), ]); - return normalizeGitHubDiscussions(reviewComments, issueComments); + return normalizeGitHubDiscussions(reviewComments, issueComments, resolvedCommentIds); } buildComments(