fix(add_labels): use updatePullRequest mutation for PRs in intent path - #50910
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes intent-aware label updates for pull requests by selecting the correct GraphQL mutation.
Changes:
- Detects pull requests using REST metadata or node ID prefix.
- Routes PRs through
updatePullRequest; issues remain onupdateIssue. - Adds regression coverage for both detection paths and regular issues.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/add_labels.cjs |
Selects the appropriate intent-label mutation. |
actions/setup/js/add_labels.test.cjs |
Tests PR detection and issue routing. |
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: 0
- Review effort level: Balanced
|
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f7ac55d0-beef-4618-b208-a9df0481736f
|
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed: The fix correctly identifies both detection signals (
pull_requestfield andPR_node ID prefix) — this is the right defensive approach since node IDs alone aren't always populated before the REST call. - Minor duplication: The two
withRetrybranches are structurally identical; a small refactor would reduce maintenance surface. - Test fixture fragility: The "regular issues" test implicitly relies on ambient fixture state; making it explicit would keep it robust to future fixture changes.
Positive Highlights
- ✅ Dual-signal PR detection (
pull_requestfield +PR_prefix) is appropriately defensive - ✅
intentHeadersextraction eliminates the one piece of existing duplication - ✅ Three focused tests cover both detection paths and the regression case
- ✅ Inline comment explaining why
updateIssuecan't accept PR node IDs — good for future readers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.5 AIC · ⊞ 7.1K
Comment /matt to run again
|
|
||
| core.info(`Successfully added ${uniqueLabels.length} labels to ${contextType} #${itemNumber} in ${itemRepo}`); | ||
| const afterLabels = result?.updateIssue?.issue?.labels?.nodes || []; | ||
| const afterLabels = itemIsPR ? result?.updatePullRequest?.pullRequest?.labels?.nodes || [] : result?.updateIssue?.issue?.labels?.nodes || []; |
There was a problem hiding this comment.
[/codebase-design] The two withRetry branches (lines 303–345) are structurally identical — differing only in mutation name, variable key, and result path. Future changes to retry config or logging must be applied twice.
💡 Suggested refactor to eliminate duplication
const [mutationQuery, mutationVars, getLabels] = itemIsPR
? [
`mutation($prId: ID!, $labels: [LabelUpdateInput!]!) {
updatePullRequest(input: { pullRequestId: $prId, labels: $labels }) {
pullRequest { id labels(first: 100) { nodes { name } } }
}
}`,
{ prId: issueNodeId, labels: labelIntentUpdates, headers: intentHeaders },
r => r?.updatePullRequest?.pullRequest?.labels?.nodes,
]
: [
`mutation($issueId: ID!, $labels: [LabelUpdateInput!]!) {
updateIssue(input: { id: $issueId, labels: $labels }) {
issue { id labels(first: 100) { nodes { name } } }
}
}`,
{ issueId: issueNodeId, labels: labelIntentUpdates, headers: intentHeaders },
r => r?.updateIssue?.issue?.labels?.nodes,
];
const result = await withRetry(
() => githubClient.graphql(mutationQuery, mutationVars),
RATE_LIMIT_RETRY_CONFIG,
`add_labels to ${contextType} #${itemNumber} in ${itemRepo}`
);
const afterLabels = getLabels(result) || [];@copilot please address this.
There was a problem hiding this comment.
Refactored in the latest commit. The two withRetry branches are now collapsed into a single call — [mutationQuery, mutationVars, getResultLabels] is selected upfront via a ternary, and withRetry is called once with the shared retry config and log label.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
|
|
@copilot Please refresh this PR branch if needed and run the
|
…plicit issue fixture in test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot review, add tests, lint js |
Done — lint passes (0 errors), all 62 JS tests pass. The PR has three new tests covering both PR-detection paths and the regular-issue path, and the |
|
@copilot The previous sous-chef nudge is now stale. Please resolve the remaining unresolved review thread (#50910 (comment)), refresh the branch if needed, and run the
|
|
/matt |
|
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs — requesting changes on a few items before merge.
📋 Key Themes & Highlights
Issues Found
- Transport concern leaking into mutation vars —
headersis embedded inside themutationVarstuple passed tographql(), which conflates HTTP transport metadata with GraphQL input variables (lines ~316, ~330). - Test mock state not restored — three new tests mutate
mockGithub.graphqlin-place without cleanup; order-dependent failures are possible. - Missing negative assertion in second PR test — the
PR_prefix test does not assertupdateIssuewas NOT called. - Silent fallback routing — the
PR_prefix path has no debug log, making future routing regressions harder to trace. - Long single-line
withRetrycall — breaks the surrounding formatting convention.
Positive Highlights
- ✅ Clean dual-detection strategy (
pull_requestfield +PR_prefix) with a clear comment explaining the rationale. - ✅
getResultLabelsextractor nicely unifies the two result shapes without duplication. - ✅ Good test coverage: three focused scenarios that directly exercise the routing logic.
- ✅
intentHeadersextracted as a named constant — avoids repeating the magic string.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27 AIC · ⊞ 7.1K
Comment /matt to run again
Thread Current state:
Hand-off: a maintainer should resolve the outdated thread and re-trigger CI before merging. |
|
@copilot run pr-finisher skill |
Local validation complete: |
|
🎉 This pull request is included in a new release. Release: |
When
add_labelshas intent metadata (rationale/confidence), it routes through GraphQL instead of REST. The mutation was unconditionallyupdateIssue, which rejects PR node IDs (PR_kwDO…) — only Issue node IDs are valid for that mutation.Changes
add_labels.cjs: After fetching the item via the REST issues endpoint, detect whether it's a PR viaissueData.pull_requestor aPR_-prefixed node ID. PRs are routed toupdatePullRequest(input: { pullRequestId, labels })instead ofupdateIssue. Both mutations shareLabelUpdateInputand the same"GraphQL-Features": "update_issue_suggestions"header.add_labels.test.cjs: Three new tests — PR detected viapull_requestfield, PR detected viaPR_node_id prefix, and regular issues still useupdateIssue.