Fix add_labels silently applying no labels - #50637
Conversation
|
👋 Thanks for working on this bug fix! I can see you're investigating the Current Status: What's Needed:
As mentioned in the Contributing Guidelines, this project uses agentic development. Since you're an agent working on this, continue with your investigation and implementation. Once you have code changes, tests, and a clear description, mark the PR as ready for review. You're on the right track — keep this PR's status updated as you make progress! 🚀
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot apply fix for Intent metadata (rationale/confidence/suggest) is no longer forwarded on the add_labels REST call. If per-label intent must be persisted, it should route through the GraphQL mutation rather than REST — out of scope for this regression fix. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in a1c0288. When intent metadata (rationale/confidence/suggest) is present and |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50637 does not have the 'implementation' label and has 0 new lines of code in business logic directories (only 2 files changed, none in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, or api/). |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Fixes label application failures caused by sending objects to the REST API.
Changes:
- Sends plain names through REST.
- Routes intent-bearing labels through GraphQL.
- Updates tests for both paths.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/add_labels.cjs |
Adds REST normalization and GraphQL intent handling. |
actions/setup/js/add_labels.test.cjs |
Updates label payload and GraphQL expectations. |
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: 2
- Review effort level: Balanced
| // Intent metadata is only supported via the GraphQL updateIssue mutation. That | ||
| // mutation replaces the issue's label set, so merge the newly requested labels with | ||
| // the issue's existing labels to preserve add-only semantics. Existing labels are | ||
| // sent without intent metadata; newly requested labels carry their metadata. |
| // The REST issues.addLabels endpoint only accepts label name strings; it does not | ||
| // support issue-intent metadata (rationale/confidence/suggest). Passing objects with | ||
| // those extra keys causes GitHub to return success while silently applying no labels. | ||
| // When intent metadata is present, route through the GraphQL updateIssue/LabelUpdateInput | ||
| // mutation instead (see update_issue.cjs), which does support intent metadata. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the root-cause fix is correct and well-tested, but two correctness issues in the new GraphQL path need addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Redundant REST call:
fetchIssueStateand the new intent block both callissues.get, doubling the API cost for every intent-label operation. - Silent truncation risk: The GraphQL fragment
labels(first: 100)can silently cap the after-state label list, causingattachExecutionStateto report stale/incomplete data.
Positive Highlights
- ✅ Root cause correctly identified and fixed: objects with extra keys now route through GraphQL instead of silently failing on REST.
- ✅ Add-only semantics preserved by merging existing labels before the mutation.
- ✅ Good regression test coverage: new test for existing-label preservation, and updated assertions on all affected paths.
- ✅ Clear, accurate comments explaining the REST vs. GraphQL constraint.
| // mutation replaces the issue's label set, so merge the newly requested labels with | ||
| // the issue's existing labels to preserve add-only semantics. Existing labels are | ||
| // sent without intent metadata; newly requested labels carry their metadata. | ||
| const { data: issueData } = await withRetry( |
There was a problem hiding this comment.
[/diagnosing-bugs] Redundant issues.get call: fetchIssueState (line 258) already calls github.rest.issues.get unconditionally, then the intent path calls it again here to get node_id and labels. This doubles the REST cost and creates two separate failure points for every intent-label operation.
💡 Suggested fix
Call issues.get once above the if (useIssueIntentPath) block and use the result for both beforeState extraction and the intent path:
const { data: issueData } = await withRetry(
() => githubClient.rest.issues.get({ owner: repoParts.owner, repo: repoParts.repo, issue_number: itemNumber }),
RATE_LIMIT_RETRY_CONFIG,
`get ${contextType} #${itemNumber}`
);
const beforeState = attachExecutionState(extractIssueStateFromData(issueData), ...);
// reuse issueData.node_id and issueData.labels in the intent blockThis removes the extra round-trip and makes the failure surface smaller.
@copilot please address this.
| { issueId: issueNodeId, labels: labelIntentUpdates, headers: { "GraphQL-Features": "update_issue_suggestions" } } | ||
| ), | ||
| RATE_LIMIT_RETRY_CONFIG, | ||
| `add_labels to ${contextType} #${itemNumber} in ${itemRepo}` |
There was a problem hiding this comment.
[/diagnosing-bugs] Hard-coded labels(first: 100) cap in the GraphQL response could silently truncate the afterLabels list for issues with more than 100 labels, causing attachExecutionState to report an incorrect post-operation label set.
💡 Suggested fix
Use pagination or, at minimum, add a warning when the response indicates truncation:
// After the mutation response:
const labelPage = result?.updateIssue?.issue?.labels;
if (labelPage?.pageInfo?.hasNextPage) {
core.warning(`Label list truncated; issue #${itemNumber} has more than 100 labels`);
}
const afterLabels = labelPage?.nodes || [];Alternatively, bump the fragment to first: 250 (the GitHub API maximum) and add the guard.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics (22 tests)
|
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 44 AIC · ⌖ 7.53 AIC · ⊞ 5.4K
| // mutation replaces the issue's label set, so merge the newly requested labels with | ||
| // the issue's existing labels to preserve add-only semantics. Existing labels are | ||
| // sent without intent metadata; newly requested labels carry their metadata. | ||
| const { data: issueData } = await withRetry( |
There was a problem hiding this comment.
Redundant issues.get call — double-fetch on the intent path.
fetchIssueState (line 258) already calls github.rest.issues.get internally to capture the before-state. The intent path then makes a second issues.get call (here) solely to retrieve node_id and existing labels.
This doubles the GitHub API calls on every intent-metadata label add. Either expose the raw issue data from fetchIssueState, or call issues.get once before the fetchIssueState call and derive beforeState from that data:
const { data: issueData } = await withRetry(() => githubClient.rest.issues.get(...), ...);
const beforeState = extractIssueStateFromData(issueData); // reuse same data@copilot please address this.
|
@copilot please follow up on the latest review feedback on this PR:
Run: https://github.com/github/gh-aw/actions/runs/31034308996
|
PR Triage: #50637Category: bug | Risk: high | Priority Score: 78/100 (impact 40, urgency 23, quality 15)
|
|
@copilot please follow up on the latest review feedback on this PR:
|
|
🎉 This pull request is included in a new release. Release: |
add_labelsstopped applying labels: logs report "Successfully added N labels" but nothing appears on the issue/PR.Root cause
When issue-intent is enabled (the default), the handler passed label objects carrying intent metadata to the REST endpoint:
issues.addLabelsonly accepts label name strings. The extra keys make GitHub return200while applying nothing. Intent metadata is only supported via the GraphQLupdateIssue/LabelUpdateInputpath (seeupdate_issue.cjs), not RESTaddLabels.Changes
add_labels.cjs: send plain label name strings toissues.addLabels; drop the intent-object payload construction and the now-unusedissueIntentEnabledlocal. Validation, dedup, staged preview, andlabelsAddedreporting are unchanged.add_labels.test.cjs: update expectations that previously asserted the object payload to expect name strings.Notes for review
Intent metadata (
rationale/confidence/suggest) is no longer forwarded on theadd_labelsREST call. If per-label intent must be persisted, it should route through the GraphQL mutation rather than REST — out of scope for this regression fix.