Resolve PR review threads from review comment node IDs - #50349
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
TriageCategory: bug | Risk: low | Score: 50/100 (impact 20, urgency 12, quality 18) Recommended action: batch_review Targeted fix (191+/27-, 2 files) enabling PRRC_ node IDs for review thread resolution, includes tests. Draft PR — batch with similar safe-output/review-thread fixes.
|
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
There was a problem hiding this comment.
Pull request overview
Adds fallback resolution from review comment node IDs to their containing review threads.
Changes:
- Detects
PullRequestReviewCommentIDs and searches the PR’s review threads. - Uses the recovered thread ID while preserving scope validation.
- Adds focused success and rejection tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/resolve_pr_review_thread.cjs |
Implements comment-to-thread lookup and resolution. |
actions/setup/js/resolve_pr_review_thread.test.cjs |
Tests comment ID fallback and unrelated node rejection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| nodes { | ||
| id | ||
| isResolved | ||
| comments(first: 100) { |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.7 AIC · ⌖ 13.1 AIC · ⊞ 5.4K
| pullRequest(number: $number) { | ||
| reviewThreads(first: 100, after: $cursor) { | ||
| pageInfo { | ||
| hasNextPage |
There was a problem hiding this comment.
The inner comments(first: 100) query fetches at most 100 comments per thread. If a thread has >100 comments, the matching comment will be missed and the fallback will silently return comment_without_thread instead of the correct thread.
Consider using the pullRequestThread field directly on PullRequestReviewComment (available in GitHub's GraphQL schema) in the initial lookup query instead of scanning all threads. That removes the O(threads × comments) fan-out and the 100-comment cap entirely:
... on PullRequestReviewComment {
pullRequestThread {
id
isResolved
}
pullRequest { ... }
}@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. The PullRequestReviewComment fragment now fetches reviewThread { id isResolved } directly in the initial node lookup query, eliminating the O(threads × comments) fan-out and the per-page cap entirely. findThreadInfoForReviewComment has been removed.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
Quality SummaryStrengths:
Opportunities for enhancement:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Issues
-
Hidden data loss (
comments(first: 100)): The inner loop over thread comments is capped at 100 with no pagination cursor. A thread with >100 comments will never match the targetcommentIdand the function will silently returncomment_without_thread— the root cause of the original bug may resurface for high-activity threads. The fix is to usePullRequestReviewComment.reviewThreaddirectly in the initial lookup fragment, which is a simpler, O(1) alternative that avoids the pagination problem entirely. -
Silent fallback (
threadNode.id || threadId): Now thatidis explicitly selected in the fragment, falling back to the inputthreadIdwhenidis absent is misleading and masks schema regressions. -
Missing test for
comment_without_thread: The error path where a comment exists but belongs to no discoverable thread is untested. This is the most important negative-path assertion for the new feature.
Positive Highlights
- ✅ Clean separation via the new
findThreadInfoForReviewCommenthelper - ✅ Existing scoping/validation logic is correctly reused after thread ID recovery
- ✅ Good use of
core.infoto log the ID remapping for observability - ✅ Updated test correctly changes the rejection case to use
Issueinstead ofPullRequestReviewComment
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.1 AIC · ⌖ 8.22 AIC · ⊞ 7.1K
Comment /matt to run again
| nodes { | ||
| id | ||
| isResolved | ||
| comments(first: 100) { |
There was a problem hiding this comment.
[/diagnosing-bugs] comments(first: 100) has no pagination — a review thread with more than 100 comments will never match and the function silently returns comment_without_thread.
💡 Simpler fix: query the parent thread directly
The PullRequestReviewComment GraphQL type exposes a reviewThread field, so you can get the parent thread ID in one query — no iteration needed:
... on PullRequestReviewComment {
reviewThread {
id
isResolved
}
pullRequest { number repository { nameWithOwner name owner { login } } }
}This eliminates findThreadInfoForReviewComment entirely and removes the O(threads × comments) pagination loop.
@copilot please address this.
There was a problem hiding this comment.
Fixed. The initial query now uses reviewThread { id isResolved } on the PullRequestReviewComment fragment, resolving the parent thread in a single round-trip. The findThreadInfoForReviewComment pagination loop has been removed entirely.
| @@ -165,12 +227,12 @@ describe("resolve_pr_review_thread", () => { | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] No test for the comment_without_thread path — when a PRRC_ comment exists but no review thread contains it, the handler should return a descriptive error. Without a test, this code path is invisible to CI.
💡 Suggested test sketch
it('should return error when comment is not found in any review thread', async () => {
mockGraphql.mockImplementation(query => {
if (query.includes('reviewThreads')) {
return Promise.resolve({
repository: {
pullRequest: {
reviewThreads: {
pageInfo: { hasNextPage: false, endCursor: null },
nodes: [{ id: 'PRRT_other', isResolved: false, comments: { nodes: [] } }],
},
},
},
});
}
return Promise.resolve({
node: {
__typename: 'PullRequestReviewComment',
pullRequest: { number: 42, repository: { name: 'r', nameWithOwner: 'o/r', owner: { login: 'o' } } },
},
});
});
const { main } = require('./resolve_pr_review_thread.cjs');
const handler = await main({ max: 10 });
const result = await handler({ type: 'resolve_pull_request_review_thread', thread_id: 'PRRC_kwDOOrphan' });
expect(result.success).toBe(false);
expect(result.error).toContain('PRRC_kwDOOrphan');
});@copilot please address this.
There was a problem hiding this comment.
A test for the comment_without_thread path is present: "should return comment_without_thread when the PRRC_ node has no reviewThread" mocks reviewThread: null and asserts success: false with an error containing the comment ID.
| @@ -62,12 +79,87 @@ async function getThreadPullRequestInfo(github, threadId) { | |||
|
|
|||
| return { | |||
There was a problem hiding this comment.
[/diagnosing-bugs] threadNode.id || threadId — the id field is now explicitly selected in the GraphQL fragment, so the || threadId fallback silently masks any future schema regression where id is unexpectedly absent. Prefer asserting its presence explicitly.
💡 Suggestion
if (!threadNode.id) {
core.warning(`PullRequestReviewThread node returned without an id for input ${threadId}`);
return { status: 'missing' };
}
return {
status: 'thread',
threadId: threadNode.id,
...
};This makes the failure observable rather than silently using the input ID as the resolved ID.
@copilot please address this.
There was a problem hiding this comment.
Fixed. The fallback threadNode.id || threadId was removed. The code now checks if (!threadNode.id) and returns { status: "missing" } with a core.warning, making the absence observable rather than silently substituting the input ID.
|
@copilot Please address the remaining blocking review feedback on this PR, then run the Open review feedback to close out:
Failed checks: Please refresh the branch as needed after fixes and resolve addressed review threads once the reviewer feedback has been handled.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please address the remaining review feedback on this PR, refresh the branch if GitHub allows it, and then run the Open review feedback to close out:
Failed checks from compact context: none currently listed. Please resolve addressed review threads once the reviewer feedback has been handled.
|
|
🎉 This pull request is included in a new release. Release: |
resolve_pull_request_review_threadrejectedPullRequestReviewCommentnode IDs (PRRC_...) even though each comment maps to exactly one review thread. This caused safe-output batches to fail when agents passed comment IDs instead of thread IDs.Comment-to-thread fallback
PullRequestReviewCommentnodes during thread lookup.PullRequestReviewThreadvia the PRreviewThreadsconnection.Validation behavior
Coverage
PRRC_...ID through its parentPRRT_....{ "type": "resolve_pull_request_review_thread", "thread_id": "PRRC_kwDOComment123" }Now maps to the containing review thread before calling
resolveReviewThread.Run: https://github.com/github/gh-aw/actions/runs/30949820215