fix: restore thread id in get_review_comments response#2245
fix: restore thread id in get_review_comments response#2245omid-ant wants to merge 1 commit intogithub:mainfrom
Conversation
|
This is a blocker to our workflow. Currently we are unable to resolve threads automatically through MCP using Claude, for example, because it does not know the IDs of the threads. Excited to see this PR and hope it can be approved soon. |
Cherry-picks upstream PR github#2245. Without the thread ID, resolve_thread / unresolve_thread are unusable. Drop this patch when upstream merges.
|
Tracks #2331. |
|
Supporting this PR. This is exactly the gap that #2229 identified — |
Cherry-picks upstream PR github#2245. Without the thread ID, resolve_thread / unresolve_thread are unusable. Drop this patch when upstream merges.
scottlz0310-user
left a comment
There was a problem hiding this comment.
As the reporter of #2229, I checked out this branch, built it locally, and verified the
behavior.
Verified:
- The id field now correctly appears in the JSON response from get_review_comments
(e.g., "id": "PRRT_kwDO...") - All existing tests pass (go test ./...)
One suggestion: the existing test for get_review_comments already provides "id" in the
mock data but never asserts thread.ID in the validation. Adding an assertion like
assert.Equal(t, "RT_kwDOA0xdyM4AX1Yz", thread.ID) would prevent silent regressions in
the future.
Summary
Restore the thread
idfield toMinimalReviewThreadso thatpull_request_review_writewithmethod: resolve_thread/unresolve_threadis actually usable.Why
#2062 dropped
idfrom the minimized review-thread output, noting it was "not used for any subsequent tool call". While this was true at the time, #1919 later addedresolve_thread/unresolve_thread, both of which require a thread node ID. The tool's own docstring reads:…but that tool no longer returns them, so there's no way to resolve a thread without leaving the MCP server. This results in callers of this tool being unable to resolve threads through the
resolve_threadtool.The GraphQL query (
reviewThreadNodeinpkg/github/pullrequests.go) already fetchesID— it's just dropped during minimization.Also see: #2229
What changed
ID stringtoMinimalReviewThreadconvertToMinimalReviewThreadviafmt.Sprintf("%v", thread.ID)MCP impact
pull_request_read(get_review_comments) now includesidper threadPrompts tested (tool changes only)
Security / limits
"id":"PRRT_kwDO…",), negligible impact on Reduce context usage forget_pull_request_review_commentsusingpull_request_read#2062's 14.5% savingsTool renaming
deprecated_tool_aliases.goDocs
Fixes #2331