Skip stale review-thread node IDs in resolve_pull_request_review_thread instead of failing safe_outputs - #51630
Conversation
…st_review_thread Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…errors Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Handles stale review-thread node IDs as successful no-op outcomes during safe-output replay.
Changes:
- Detects missing GraphQL nodes during lookup and mutation.
- Adds positive and negative error-handling tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/resolve_pr_review_thread.cjs |
Adds stale-node error handling. |
actions/setup/js/resolve_pr_review_thread.test.cjs |
Tests stale and unrelated errors. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
|
@copilot Quick triage nudge for this PR. Please refresh the branch state after the update request, re-check the latest reviewer feedback, and run the Open review context (newest first):
Branch refresh was requested.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed. I refreshed branch/review state, applied the inline reviewer fix, and pushed 33167c7. The stale-node NOT_FOUND handling is now restricted to node-scoped GraphQL errors, with a structured repository-NOT_FOUND regression test added. |
|
@copilot Quick triage nudge for this PR. Open review context (newest first):
Branch refresh was requested.
|
Refreshed branch state and re-verified on current head |
|
@copilot Quick triage nudge for this PR. Please refresh the branch state after the update request, resolve or close the remaining unresolved reviewer thread if the current head confirms it is addressed, and run the Open review context (newest first):
Branch refresh was requested.
|
Refreshed branch state and re-verified on head |
|
Excellent work on fixing this critical workflow failure! This PR cleanly handles a real production issue where stale GraphQL node IDs from PR review threads were causing the entire safe_outputs job to fail even though the intended action had already succeeded. The solution is well-reasoned:
The PR aligns with contributing standards, is focused and well-described. This looks ready for review.
|
|
@copilot Quick triage nudge for this PR. Please verify the remaining reviewer thread is resolved on the current head, close it if addressed, and run the Open review context (newest first):
Branch refresh was requested.
|
Verified on current head |
|
@copilot Quick triage nudge for this PR. Please verify the remaining reviewer thread is resolved on the current head, close it if addressed, and run the Open review context (newest first):
Branch refresh was requested.
|
Verified on current head |
|
@copilot Quick triage nudge for this PR. Please verify the remaining reviewer state on the current head, close out any addressed thread/review artifacts, and run the Open review context (newest first):
Branch refresh was requested.
|
Re-checked current head |
|
@copilot Quick triage nudge for this PR. Please re-check the current reviewer state on the latest head, clean up any remaining stale review artifacts if they still exist, and run the Open review context (newest first):
Branch refresh was requested.
|
Re-checked on current head |
|
@copilot Quick triage nudge for this PR. Please re-check the current maintainer-facing state on the latest head, confirm whether anything remains beyond approval/CI re-trigger gates, and run the
Reference: workflow run https://github.com/github/gh-aw/actions/runs/31342923495
|
Re-checked on latest head |
Review-thread node IDs captured during the agent turn can be stale by the time the
safe_outputsjob replays them (thread already resolved, deleted, or superseded). GitHub throws for these —Could not resolve to a node with the global id of 'PRRT_...'or a bareNot Found— which surfaced ascore.errorand marked the wholesafe_outputsjobfailureeven though the intended action was already accomplished.The handler previously only skipped when the lookup returned
node: null; the thrown-error path was never covered, so it fell through to the generic catch.Changes (
actions/setup/js/resolve_pr_review_thread.cjs)isMissingNodeError(error)— new helper mirroring the existingisIntegrationAccessErrorshape. Matches GraphQL errors withtype: "NOT_FOUND", messages containingcould not resolve to a node, and Octokit's bareNot Found. Intentionally not a loosenot foundsubstring match, soRepository not foundand similar still fail loudly.getThreadPullRequestInfo) and theresolveReviewThreadmutation — returning a skip result with acore.inforeason rather than propagating to the error path.Tests
NOT_FOUND), bareNot Foundon lookup, and stale-node error from the mutation — all assertsuccess: true,skipped: true, and nocore.error.Internal server errorandRepository not foundstill fail.Requested branch update from PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31336577085> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.2 AIC · ⌖ 5.19 AIC · ⊞ 8.5K · ◷
Branch refresh requested by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31338952148> Generated by 👨🍳 PR Sous Chef · gpt54 · 27.7 AIC · ⌖ 5.43 AIC · ⊞ 6.1K · ◷
Requested branch update from PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31339757619> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.8 AIC · ⌖ 5.2 AIC · ⊞ 8.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31341574451> Generated by 👨🍳 PR Sous Chef · gpt54 · 20.6 AIC · ⌖ 5.23 AIC · ⊞ 6.1K · ◷