Fix add_labels issue-intent path failing on pull requests - #51168
Conversation
…requests Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes pull-request label application by avoiding the unsupported GraphQL mutation.
Changes:
- Falls back to the REST labels endpoint for pull requests.
- Preserves the intent-aware GraphQL path for issues.
- Updates tests for both pull-request detection paths.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/add_labels.cjs |
Routes pull-request labeling through REST. |
actions/setup/js/add_labels.test.cjs |
Verifies REST fallback and unchanged issue behavior. |
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
|
|
|
|
|
|
|
No ADR enforcement needed: PR #51168 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
…tions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
The fix is correct: UpdatePullRequestInput does not accept a labels field, so the PR path now falls through to the REST addLabels endpoint. Logic, test coverage, and comments are all clean. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19 AIC · ⊞ 5.5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
AnalysisAll three modified tests verify the critical behavioral change: pull requests with
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor test coverage note.
📋 Key Themes & Highlights
Key Themes
- Correct root-cause fix: The broken
updatePullRequest(input: { labels: ... })mutation is removed; the PR path now unconditionally falls back to RESTissues.addLabels, which matches the API's actual capabilities. - Issue path unchanged: The label-merging and
updateIssueintent-mutation logic for non-PR items is preserved; only the PR branch is simplified. - Test coverage updated: Both PR detection paths (via
pull_requestfield and viaPR_node ID prefix) now assert the REST fallback behaviour.
Positive Highlights
- ✅ PR detection logic is explicit and readable (
isPullRequestItemhelper / inline comment) - ✅ The
core.infolog message clearly explains the trade-off (no intent metadata for PRs) - ✅ Test names were updated to describe the new expected behaviour — good specification hygiene
- ✅ One inline comment left on a minor missing assertion in the second PR test
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.1 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
Review: no blocking issues
The PR correctly fixes the root bug — PR node IDs no longer reach the invalid updatePullRequest/updateIssue GraphQL mutations; they now cleanly fall through to the REST addLabels endpoint. The follow-up refactor extracting isPullRequestItem and applyIssueIntentLabels into named helpers (per the earlier reviewer comment) is a clean improvement and preserves existing merge/dedup semantics.
💡 Themes checked
- Correctness: verified no code path can still send a
PR_node ID intoupdateIssue— theif (isPullRequestItem(...))branch short-circuits beforeapplyIssueIntentLabelsis called, and falls through to the RESTaddLabelscall at the bottom of the try block. - Redundant API call:
fetchIssueState(forbeforeState) and the subsequentgithubClient.rest.issues.get(forissueData/node ID) both hit the same REST endpoint back-to-back. This is a minor extra API call, not new behavior introduced by this PR (it existed in the priorupdatePullRequestcode path too), so not flagged as blocking, but worth a follow-up cleanup. - Test coverage gap: flagged as an inline comment — the two new PR-fallback tests assert
labelsAddedand the REST call args, but don't assert on the resulting after-statelabelssnapshot, unlike the issue-intent-path test. Non-blocking but worth adding. - Sub-agent (grumpy-coder) findings: reviewed all three; the redundant-GET finding predates this PR (not actionable here), the error-handling gap is low severity and already caught generically, and the test-coverage gap was escalated as the one inline comment above.
🔎 Code quality review by PR Code Quality Reviewer · auto · 54.1 AIC · ⊞ 7.8K
Comment /review to run again
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please refresh the branch if needed and run the Recent reviewer activity looks mostly green, but please do one final pass on the remaining non-blocking review notes and confirm validation is still clean after branch refresh. Run context: https://github.com/github/gh-aw/actions/runs/31213603061
|
Ran
No merge performed. |
|
$ Please refresh the branch, address any remaining review feedback, run the
Run: https://github.com/github/gh-aw/actions/runs/31215510357
|
…s-safe-output-failure Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran
No merge performed. |
|
🎉 This pull request is included in a new release. Release: |
The
add_labelssafe output routed pull requests throughupdatePullRequest(input: { pullRequestId: ..., labels: ... }), butUpdatePullRequestInputhas nolabelsfield, so every PR label application failed and thesafe_outputsjob ended inpartial_success:There is no Labelable mutation that accepts
LabelUpdateInputintent metadata (rationale/confidence/suggest) —updateIssueis the only mutation that supports it. So the PR path degrades to the REST endpoint rather than attempting an intent-aware mutation.Changes
actions/setup/js/add_labels.cjs: when the target is detected as a PR (RESTpull_requestfield orPR_node ID prefix), skip the GraphQL intent mutation and fall through torest.issues.addLabels, logging that intent metadata is not supported for pull requests. The invalidupdatePullRequestmutation and the query-selection branch are removed; the issue path now inlines theupdateIssuemutation directly.updateIssue's replace-the-set behavior.actions/setup/js/add_labels.test.cjs: the two PR tests now assert the REST fallback (no GraphQL mutation issued;issues.addLabelscalled with the expected number and labels); the issue test was renamed to state that it exercises theupdateIssueintent mutation.Trade-off
PR labels are applied without intent metadata. This removes the need for the
issue-intent: falseworkaround and unblocks the failing checks, but restoring metadata on PRs depends on GitHub exposing a Labelable mutation that acceptsLabelUpdateInput.Run: https://github.com/github/gh-aw/actions/runs/31215510357> Generated by 👨🍳 PR Sous Chef · gpt54 · 5.84 AIC · ⊞ 6.1K · ◷