Prevent issue-intent labels from removing existing labels - #53672
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53672 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold is 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
PR TriageCategory: chore (WIP) | Risk: low | Priority: low (score 20)
Recommended action: defer — recheck once agent finishes and diff is populated
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: non-blocking
I don't see a changed-line correctness or maintainability issue here severe enough to block merge.
Reviewed themes
- The GraphQL intent path now avoids re-proposing labels that are already present.
- The fallback restoration via REST covers the destructive replacement behavior this PR is fixing.
- The new
labelsSuggestedplumbing is propagated through manifest logging and covered by tests. - Regression coverage hits the important cases in the diff: already-applied labels, confidence-gated suggestions, and restoration after partial replacement.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.9 AIC · ⌖ 8.68 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The implementation is correct and well-covered by new tests. The mergedSpecs dedup removal is safe because the caller pre-filters newLabelSpecs to exclude already-existing labels before calling applyIssueIntentLabels. The REST recovery fallback and labelsSuggested propagation through the manifest are complete and properly tested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.2 AIC · ⌖ 8.76 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review (over-engineering only): one minor nit found.
net: -2 lines possible.
Generated by ✂️ Ponytail Reviewer for #53672 · auto · 21.2 AIC · ⌖ 4.29 AIC · ⊞ 7.3K
Comment /ponytail to run again
| const labelsAdded = newLabelSpecs.filter(spec => afterNamesLower.has(spec.name.toLowerCase())).map(spec => spec.name); | ||
| const labelsSuggested = newLabelSpecs.filter(spec => hasLabelIntentMetadata(spec) && !afterNamesLower.has(spec.name.toLowerCase())).map(spec => spec.name); | ||
|
|
||
| if (newLabelSpecs.length === 0) { |
There was a problem hiding this comment.
L412-413: delete: redundant "No new labels to add" info log. Line 415's "Successfully added 0 labels" already conveys this.
There was a problem hiding this comment.
Pull request overview
Prevents issue-intent label operations from deleting existing labels and improves outcome reporting.
Changes:
- Skips already-applied labels and restores unexpectedly removed labels.
- Separates applied labels from confidence-gated suggestions.
- Adds regression and manifest persistence coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/add_labels.cjs |
Adds filtering, recovery, and observed outcome reporting. |
actions/setup/js/add_labels.test.cjs |
Covers no-ops, suggestions, and restoration. |
actions/setup/js/safe_output_manifest.cjs |
Persists suggested labels. |
actions/setup/js/safe_output_manifest.test.cjs |
Tests label outcome persistence. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| const existingLabelNames = normalizeLabelNames(issueData.labels || []); | ||
| const mergedSpecs = [...labelSpecs, ...existingLabelNames.filter(name => !requestedNamesLower.has(name.toLowerCase())).map(name => ({ name }))]; | ||
| const mergedSpecs = [...labelSpecs, ...existingLabelNames.map(name => ({ name }))]; |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and one test-fidelity concern.
📋 Key Themes & Highlights
Issues Found
-
Silent label loss (
add_labels.cjs:410) — Plain-name specs that the mutation drops are tracked in neitherlabelsAddednorlabelsSuggested. ThelabelsSuggestedbucket only captures specs with intent metadata, so a{ name: "foo" }spec that fails to apply disappears silently. -
Duplicate entries in
mergedSpecs(add_labels.cjs:82) — The old de-duplication guard was removed. If any caller-suppliedlabelSpecoverlaps withexistingLabelNames, the mutation receives the same label twice. -
Test assertion may be testing the wrong snapshot (
add_labels.test.cjs:350) — The "restore pre-existing labels" test checksresult.after_state.labels, butafter_stateis populated by a post-mutationfetchIssueStatecall that uses the default mock. The test should re-mock the GET to return the fully-restored set, or assertlabelsAdded/afterLabelsdirectly.
Positive Highlights
- ✅ Excellent defensive recovery — detect missing existing labels post-mutation and restore them via REST before returning.
- ✅ Clean separation of
labelsAddedvslabelsSuggestedin both the result and the manifest. - ✅ Three targeted regression tests for the three main failure modes — good test-first thinking.
- ✅ Mutation is skipped entirely when all requested labels are already applied — avoids unnecessary round-trips.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 54.3 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
| : existingLabels; | ||
| const afterNamesLower = new Set(afterLabels.map(name => name.toLowerCase())); | ||
| const labelsAdded = newLabelSpecs.filter(spec => afterNamesLower.has(spec.name.toLowerCase())).map(spec => spec.name); | ||
| const labelsSuggested = newLabelSpecs.filter(spec => hasLabelIntentMetadata(spec) && !afterNamesLower.has(spec.name.toLowerCase())).map(spec => spec.name); |
There was a problem hiding this comment.
[/diagnosing-bugs] Silent loss: plain-name newLabelSpecs entries dropped by the mutation land in neither labelsAdded nor labelsSuggested.
labelsSuggested is gated on hasLabelIntentMetadata(spec), so a { name: "foo" } spec that the mutation silently drops vanishes from the result with no trace — callers can't distinguish "applied", "suggested", and "silently dropped".
💡 Suggested fix
Capture all unapplied new labels and split them afterwards:
const labelsAdded = newLabelSpecs.filter(spec => afterNamesLower.has(spec.name.toLowerCase())).map(s => s.name);
const labelsNotApplied = newLabelSpecs.filter(spec => !afterNamesLower.has(spec.name.toLowerCase()));
const labelsSuggested = labelsNotApplied.filter(spec => hasLabelIntentMetadata(spec)).map(s => s.name);
// optionally warn on labelsNotApplied that are not suggestedA regression test with a metadata-free spec that the mutation silently drops would lock this in.
@copilot please address this.
| const requestedNamesLower = new Set(labelSpecs.map(spec => spec.name.toLowerCase())); | ||
| const existingLabelNames = normalizeLabelNames(issueData.labels || []); | ||
| const mergedSpecs = [...labelSpecs, ...existingLabelNames.filter(name => !requestedNamesLower.has(name.toLowerCase())).map(name => ({ name }))]; | ||
| const mergedSpecs = [...labelSpecs, ...existingLabelNames.map(name => ({ name }))]; |
There was a problem hiding this comment.
[/diagnosing-bugs] mergedSpecs still duplicates existing labels when the caller passes specs that overlap with existingLabelNames.
After the change, mergedSpecs = [...labelSpecs, ...existingLabelNames.map(name => ({ name }))] no longer de-duplicates. If any labelSpec name matches an existing label, the mutation receives that label twice. Downstream buildIssueIntentLabelUpdates may or may not handle duplicates; if it doesn't, the mutation payload could be malformed or trigger unexpected behavior.
💡 Suggested fix
Keep the original de-duplication but move it to operate only on the labels passed to the mutation, not on newLabelSpecs (the caller already filters out already-applied labels):
const existingNamesLower = new Set(labelSpecs.map(s => s.name.toLowerCase()));
const mergedSpecs = [
...labelSpecs,
...existingLabelNames
.filter(name => !existingNamesLower.has(name.toLowerCase()))
.map(name => ({ name })),
];This mirrors the original dedup logic and prevents double-entries in the mutation payload.
@copilot please address this.
| nodes: [ | ||
| { id: "LABEL_feature-openapi", name: "feature-openapi" }, | ||
| { id: "LABEL_area-minimal", name: "area-minimal" }, | ||
| { id: "LABEL_bug", name: "bug" }, |
There was a problem hiding this comment.
[/tdd] The "restore pre-existing labels" test asserts result.after_state.labels but the mock addLabels REST call returns { data: [...] } — the actual after_state is captured by attachExecutionState from a separate fetchIssueState call that is NOT re-mocked in this test.
This means result.after_state.labels likely reflects the pre-mutation state fetched from the default mock (only feature-openapi), not the restored set. If so, the assertion expect(result.after_state.labels).toEqual(["feature-openapi", "area-minimal", "bug"]) is either coincidentally passing or testing the wrong thing.
💡 Suggested fix
Mock the afterState fetch (mockGithub.rest.issues.get returning the post-restore label set) or explicitly verify result.afterLabels (the value returned from applyIssueIntentLabels) rather than relying on the after_state snapshot populated by attachExecutionState.
@copilot please address this.
|
@copilot Please address the remaining blocking review feedback on this PR, then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in Validation:
The duplicate |
|
🎉 This pull request is included in a new release. Release: |
The
add_labelsGraphQL intent path could remove an existing label when its requested confidence fell below the repository automation threshold. It also reported requested labels as added even when GitHub retained them only as suggestions.Prevent destructive no-ops
Defend existing state
Report observed outcomes
labelsAddedfrom the returned label state.labelsSuggested.Regression coverage