Skip to content

fix: recognize resolved GitHub review threads in summary#125

Merged
titouanmathis merged 2 commits into
mainfrom
fix/github-resolved-threads
Jul 16, 2026
Merged

fix: recognize resolved GitHub review threads in summary#125
titouanmathis merged 2 commits into
mainfrom
fix/github-resolved-threads

Conversation

@titouanmathis

Copy link
Copy Markdown
Contributor

Problem

On GitHub PRs, the review summary keeps re-listing findings in its "Still open from earlier reviews" block even after the human has resolved those threads (observed on studiometa/ui#511, where all 15 bot threads were resolved yet still appeared as open). The reviewer also re-raises already-settled concerns.

Root cause

Both summary paths decide resolution with notes.some((n) => n.resolved === true):

  • src/summary-carryover.ts — skips resolved threads when building the carry-over block.
  • src/prior-threads.ts — flags a prior thread resolved for the reviewer prompt.

But on GitHub note.resolved was always undefined:

  1. normalizeGitHubDiscussions never set it.
  2. The data was never fetched — getDiscussions reads the REST endpoints GET /pulls/{n}/comments and GET /issues/{n}/comments, and neither REST endpoint returns thread-resolution state. On GitHub, review-thread resolution lives only in the GraphQL API (pullRequest.reviewThreads.nodes.isResolved).

So every prior thread looked open: resolved findings were carried over indefinitely, and resolved concerns were re-raised. This works on GitLab because its discussions API returns resolved per note inline.

Fix

  • Add GitHubClient.listResolvedReviewCommentIds() — paginated GraphQL reviewThreads query returning the database ids of comments in resolved threads. Derives the GraphQL endpoint from the REST base (github.com → /graphql; GHE /api/v3/api/graphql) and surfaces GraphQL errors as typed GitHubApiErrors with an actionable hint.
  • getDiscussions fetches resolution alongside the two REST calls and passes it to normalizeGitHubDiscussions, which stamps resolved onto each note — mirroring GitLab's per-note field so the shared helpers work unchanged on both platforms.

Tests

  • github.test.ts: resolved-id extraction, GHE endpoint, thread pagination, typed error on GraphQL errors.
  • platforms/github.test.ts: notes flagged resolved by id; default-unresolved; end-to-end getDiscussions → carry-over exclusion.
  • cli.integration.test.ts: in-memory backend now models resolution via GraphQL; new test asserts a resolved prior finding is dropped from the summary (reproduces #511), while the existing unresolved-carry-over test still passes.

npm run check passes (lint, format, typecheck, typecheck:tests, 696 tests, build).

🤖 Generated with Claude Code

titouanmathis and others added 2 commits July 16, 2026 11:31
GitHub's REST comment endpoints omit thread-resolution state, so every
prior thread looked open. The summary carry-over and prior-thread context
therefore kept re-listing findings that had already been resolved.

Read resolution from the GraphQL `reviewThreads` API and stamp it onto the
normalized notes (`resolved`), mirroring GitLab's per-note field so the
shared `notes.some((n) => n.resolved)` checks work on both platforms.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Code Review

Risk: Low — The change appears to address the GitHub resolved-thread carry-over bug without introducing blocking defects, so it is safe to merge.

This MR adds a GitHub GraphQL read path for resolved review-thread state, threads that data through discussion normalization, and reuses the existing shared carry-over / prior-thread helpers via the normalized resolved flag. The test coverage is strong across client, platform, and CLI integration layers, and the implementation matches the stated intent of fixing stale "Still open from earlier reviews" entries for resolved GitHub threads.


Review usage: 260,877 in (210,432 cached) / 5,466 out tokens — $0.2607 (cloudflare-ai-gateway/gpt-5.4, thinking: medium)

Reviewed by @weareikko/code-review v0.8.2 for commit 02a810c.

@titouanmathis titouanmathis merged commit fde0442 into main Jul 16, 2026
6 checks passed
@titouanmathis titouanmathis deleted the fix/github-resolved-threads branch July 16, 2026 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant