Skip to content

fix: discussion label updates truncated to 3 instead of max labels#25430

Merged
pelikhan merged 3 commits intomainfrom
copilot/fix-discussion-label-limit
Apr 9, 2026
Merged

fix: discussion label updates truncated to 3 instead of max labels#25430
pelikhan merged 3 commits intomainfrom
copilot/fix-discussion-label-limit

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 9, 2026

update_discussion was silently truncating labels to 3 due to validateLabels defaulting maxCount to 3 when not explicitly provided — despite tryEnforceArrayLimit above it correctly using MAX_LABELS = 10.

Changes

  • update_discussion.cjs: Pass MAX_LABELS explicitly to validateLabels, aligning both limit checks:

    // Before — defaults to maxCount=3
    validateLabels(item.labels, allowedLabels.length > 0 ? allowedLabels : undefined);
    
    // After
    validateLabels(item.labels, allowedLabels.length > 0 ? allowedLabels : undefined, MAX_LABELS);
  • update_discussion.test.cjs: Added regression test asserting all 5 requested labels are applied (not truncated to 3); extended defaultLabels fixture to include Label5–Label11. Added a test case asserting that requesting more than 10 labels (> MAX_LABELS) is rejected with an E003 error and no label mutations are called.

Copilot AI linked an issue Apr 9, 2026 that may be closed by this pull request
The validateLabels call in update_discussion.cjs was not passing maxCount,
so it defaulted to 3. Now passes MAX_LABELS (10) to be consistent with
the tryEnforceArrayLimit check above it.

Fixes: Discussion labels limited to 3 per item

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/11bde075-8540-40c3-955b-2bc46c63e5b5

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix discussion labels limited to 3 per item fix: discussion label updates truncated to 3 instead of MAX_LABELS (10) Apr 9, 2026
Copilot AI requested a review from mnkiefer April 9, 2026 05:10
@mnkiefer
Copy link
Copy Markdown
Collaborator

mnkiefer commented Apr 9, 2026

@copilot Compare to max count and behavior for issue and PR labels, so as to stay consistent.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 9, 2026

@copilot Compare to max count and behavior for issue and PR labels, so as to stay consistent.

Checked the consistency:

  • add_labels.cjs / remove_labels.cjs: use maxCount = config.max || 10 and pass it to validateLabels
  • update_issue.cjs: uses tryEnforceArrayLimit(labels, MAX_LABELS, …) (MAX_LABELS = 10), no separate validateLabels call

The fix in cc03161 aligns update_discussion.cjs with both patterns by explicitly passing MAX_LABELS (10) to validateLabels, consistent with the rest of the codebase.

@mnkiefer
Copy link
Copy Markdown
Collaborator

mnkiefer commented Apr 9, 2026

@copilot Also add a test case for more than 10 items.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 9, 2026

@copilot Also add a test case for more than 10 items.

Added in e2900ea — a new test case requests 11 labels (> MAX_LABELS = 10) and asserts the result is { success: false, error: "E003: Cannot add more than 10 labels (received 11)" } with no label mutations called. Also extended defaultLabels with Label7–Label11 to support this test.

@mnkiefer mnkiefer marked this pull request as ready for review April 9, 2026 05:35
Copilot AI review requested due to automatic review settings April 9, 2026 05:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes update_discussion label updates being unintentionally truncated to 3 labels by aligning validateLabelsmaxCount with the existing MAX_LABELS = 10 enforcement.

Changes:

  • Pass MAX_LABELS explicitly into validateLabels within update_discussion.cjs to prevent the implicit maxCount=3 default from truncating label arrays.
  • Add regression coverage ensuring 5 labels are applied (no truncation) and that requesting >10 labels fails with E003 and performs no label mutations.
  • Extend the test label fixture to include Label5Label11 so the new cases can resolve label IDs.
Show a summary per file
File Description
actions/setup/js/update_discussion.cjs Ensures label validation uses MAX_LABELS (10) instead of validateLabels’ default (3).
actions/setup/js/update_discussion.test.cjs Adds regression tests for the 10-label limit and prevents silent truncation to 3.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 0

@mnkiefer
Copy link
Copy Markdown
Collaborator

mnkiefer commented Apr 9, 2026

@copilot Investigate failing CI checks/tests.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 9, 2026

@copilot Investigate failing CI checks/tests.

The CI failure is pre-existing and unrelated to this PR. My changes only touch actions/setup/js/update_discussion.cjs and update_discussion.test.cjs (JavaScript files) — zero Go files changed across all 3 commits.

The failing job is Integration: Workflow Misc Part 2TestStepOrderingValidation_SecretRedactionBeforeUploads in pkg/workflow/step_order_validation_integration_test.go. It fails because the compiled workflow still emits a step named "Upload Safe Outputs Items", which satisfies the test's strings.Contains(contentStr, "name: Upload Safe Outputs") check — the test expects that step to have been fully removed (merged into the unified agent artifact). This reproduces identically on the base commit before any of my changes.

Copilot AI requested a review from mnkiefer April 9, 2026 06:08
@mnkiefer mnkiefer changed the title fix: discussion label updates truncated to 3 instead of MAX_LABELS (10) fix: discussion label updates truncated to 3 instead of MAX_LABELS Apr 9, 2026
@mnkiefer mnkiefer changed the title fix: discussion label updates truncated to 3 instead of MAX_LABELS fix: discussion label updates truncated to 3 instead of max labels Apr 9, 2026
@github-actions github-actions bot mentioned this pull request Apr 9, 2026
@mnkiefer mnkiefer marked this pull request as draft April 9, 2026 07:06
@mnkiefer mnkiefer marked this pull request as ready for review April 9, 2026 07:06
@pelikhan pelikhan merged commit 9fbff36 into main Apr 9, 2026
296 of 348 checks passed
@pelikhan pelikhan deleted the copilot/fix-discussion-label-limit branch April 9, 2026 13:31
@github-actions github-actions bot added the lgtm label Apr 9, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Hey @Copilot 👋 — great catch on the validateLabels / MAX_LABELS mismatch in update_discussion.cjs! This is a clean, well-scoped bug fix and the PR is in excellent shape. Here's a quick summary of how it checked out against the contribution guidelines:

  • On-topic — Directly addresses a bug in a core agentic-workflow action (update_discussion).
  • Follows process — Authored by the Copilot coding agent as expected per CONTRIBUTING.md, with a core team member (mnkiefer) assigned as reviewer.
  • Focused — Touches exactly two files, both scoped to the same bug.
  • No new dependencies — Pure logic fix; no manifest changes.
  • Tests included — Two new regression tests cover the happy path (≤10 labels accepted) and the error path (>10 labels rejected with E003).
  • Clear description — Body explains the root cause, the before/after code, and what the tests assert.

This PR looks ready for maintainer review. Nice work! 🚀

Generated by Contribution Check · ● 2.2M ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discussion labels limited to 3 per item

4 participants