Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
67 changes: 66 additions & 1 deletion src/cli.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<number>();
let nextId = 1000;
const jsonResponse = (value: unknown): Response =>
new Response(JSON.stringify(value), { headers: { 'content-type': 'application/json' } });
Expand All @@ -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 },
Expand Down Expand Up @@ -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> = {}): Config {
Expand Down Expand Up @@ -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');
});
});
95 changes: 95 additions & 0 deletions src/github.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
107 changes: 107 additions & 0 deletions src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -332,4 +362,81 @@ export class GitHubClient {
getCurrentUser(): Promise<GitHubUser> {
return this.request('/user');
}

/**
* Derive the GraphQL endpoint from the REST base. github.com exposes GraphQL
* at `<origin>/graphql`, while GitHub Enterprise Server exposes it at
* `<origin>/api/graphql` (its REST base is `<origin>/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<T>(query: string, variables: Record<string, unknown>): Promise<T> {
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<Set<number>> {
const resolved = new Set<number>();
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;
}
}
Loading