Skip to content

feat: add label update support to update-discussion safe-output#22384

Closed
Copilot wants to merge 4 commits intomainfrom
copilot/update-discussion-labels
Closed

feat: add label update support to update-discussion safe-output#22384
Copilot wants to merge 4 commits intomainfrom
copilot/update-discussion-labels

Conversation

Copy link
Contributor

Copilot AI commented Mar 23, 2026

update-discussion supported title and body updates but not labels, despite the config already parsing labels/allowed-labels fields and generating allow_labels/allowed_labels runtime config.

Changes

Validation (safe_outputs_validation_config.go)

  • Extended requiresOneOf from title,bodytitle,body,labels so a labels-only update is valid
  • Added labels field validation (array of strings, sanitized, max 128 chars each)

MCP tool schema (safe_outputs_tools.json)

  • Added labels array field to update_discussion input schema

Runtime handler (update_discussion.cjs)

  • fetchLabelIds() — resolves label names → GitHub node IDs (case-insensitive, up to 100 repo labels)
  • addLabelsToDiscussion() / removeLabelsFromDiscussion() — GraphQL addLabelsToLabelable / removeLabelsFromLabelable mutations
  • executeDiscussionUpdate() — fetches current labels (up to 100) in the initial query, then performs a replacement diff (add new, remove old) after any title/body update
  • buildDiscussionUpdateData() — extracts and processes labels whenever provided (consistent with update_issue); filters against allowed_labels when configured

Example config

safe-outputs:
  update-discussion:
    labels:                       # enable label updates
    allowed-labels: [bug, idea]   # restrict to specific labels
    target: "*"

When target: "*", the agent must include discussion_number in its output. A label update without title: or body: enabled does not call the updateDiscussion mutation — only the label mutations run.

Tests (update_discussion.test.cjs)

New test file covering: add, remove, full replacement, no-op (unchanged), allowed-labels filtering, always-on label processing (consistent with update_issue), combined title+labels, and labels-only update.

[!WARNING]


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Verify that update-discussion for label updates works as designed feat: add label update support to update-discussion safe-output Mar 23, 2026
Copilot AI requested a review from mnkiefer March 23, 2026 06:42
@mnkiefer
Copy link
Contributor

@copilot List the form of expected ops calls for update project, given one wants to either add, remove labels or update a set to a give list.

Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot List the form of expected ops calls for update project, given one wants to either add, remove labels or update a set to a give list.

For update_discussion, the label operations always work as a set replacement — the agent provides the desired final set and the handler diffs against what's currently on the discussion:

Always runs first:

graphql(getDiscussionQuery, { owner, repo, number })
  → returns discussion.id + discussion.labels.nodes[{id, name}]

For any label update, label IDs are resolved next:

graphql(labelsQuery, { owner, repo })
  → returns all repo labels [{id, name}]
  → matched against requested names (case-insensitive)

Then, based on the diff:

Intent Calls made
Add labels (none currently) addLabelsToLabelable({ labelableId, labelIds: [new...] })
Remove labels (no desired labels) removeLabelsFromLabelable({ labelableId, labelIds: [old...] })
Replace set (mix of add & remove) Both addLabelsToLabelable and removeLabelsFromLabelable
Labels already match Neither mutation called

So the pattern is always: fetch current state → diff → apply only the delta. The agent specifies the desired final set in labels: [...]; the handler handles the add/remove split automatically.

If title/body are also updated, updateDiscussion(...) runs before the label mutations.

@mnkiefer
Copy link
Contributor

@copilot Summarize the fix.

Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot Summarize the fix.

The update-discussion safe-output handler already parsed labels/allowed-labels from frontmatter config but never acted on them. This PR wires up the missing runtime support:

  • Validation (safe_outputs_validation_config.go): accepts labels as a valid standalone update target (previously only title or body were required)
  • MCP tool schema (safe_outputs_tools.json): adds labels array field to the update_discussion input schema so the agent can pass label names
  • Runtime handler (update_discussion.cjs): fetches current discussion labels, resolves requested label names to node IDs, then diffs and applies only the add/remove delta via addLabelsToLabelable / removeLabelsFromLabelable GraphQL mutations; respects allowed_labels filtering
  • Tests (update_discussion.test.cjs): covers add, remove, replace, no-op, allowed-labels filtering, disabled state, and combined title+labels scenarios

@mnkiefer
Copy link
Contributor

@copilot This should be consistent with how issue labels are handled in update-issue safe output.

Remove the allow_labels === true gate from buildDiscussionUpdateData so
labels are always processed when provided, consistent with update_issue
behavior. Optional allowed_labels config still restricts which labels
may be set.

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1ed3573c-43c2-487f-b854-fbc8e7b139f8
Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot This should be consistent with how issue labels are handled in update-issue safe output.

Fixed in 53f187d. The allow_labels === true gate has been removed from buildDiscussionUpdateData so labels are always processed when provided — consistent with how update_issue handles them. allowed_labels filtering is preserved (if configured it still restricts which labels can be set). Tests updated accordingly.

@pelikhan
Copy link
Contributor

@copilot review update consistency with update-issue

…Data

- Remove unused parseBoolTemplatable import and _includeFooter assignment
  from buildDiscussionUpdateData; executeDiscussionUpdate never reads it
- Export buildDiscussionUpdateData matching update_issue's export of
  buildIssueUpdateData for unit-test parity

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/bb00d1b8-9f7d-4585-9d0d-eb12204d2716
Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot review update consistency with update-issue

Remaining inconsistencies fixed in 66d3e52:

  • Dead _includeFooter code removed: buildDiscussionUpdateData was setting updateData._includeFooter (and importing parseBoolTemplatable) but executeDiscussionUpdate never reads it — unlike executeIssueUpdate which actually consumes it. Cleaned up.
  • buildDiscussionUpdateData now exported: matches update_issue's module.exports = { main, buildIssueUpdateData } pattern for unit-test parity.

The structural API differences (GraphQL vs REST, no append/prepend operations, no footer attribution for discussions) are intentional given the different GitHub APIs involved.

Copilot AI requested a review from pelikhan March 23, 2026 12:36
@mnkiefer mnkiefer marked this pull request as ready for review March 23, 2026 18:12
Copilot AI review requested due to automatic review settings March 23, 2026 18:12
Copy link
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

Adds labels support to the update_discussion safe-output so discussions can be updated via title/body and/or label replacement, including config/schema/validation and new Vitest coverage.

Changes:

  • Extend safe-output validation and MCP tool schema to accept labels for update_discussion.
  • Implement discussion label replacement via GraphQL label ID resolution + add/remove mutations.
  • Add a new update_discussion Vitest suite covering label add/remove/replace/no-op and mixed updates.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
pkg/workflow/safe_outputs_validation_config.go Allows labels as a valid requiresOneOf field and validates label arrays.
actions/setup/js/safe_outputs_tools.json Exposes labels in the update_discussion tool input schema and updates description.
actions/setup/js/update_discussion.cjs Implements label resolution and add/remove mutations, plus label processing in update data builder.
actions/setup/js/update_discussion.test.cjs Adds tests covering the new label update behavior and combinations with title updates.
Comments suppressed due to low confidence (1)

actions/setup/js/update_discussion.cjs:313

  • validateLabels(...) can return valid:false for reasons other than “all labels were filtered out” (e.g., label removal attempts like -bug, non-array input, or sanitization producing an empty set). Treating any !valid result as updateData.labels = [] makes a validation failure/destructive input translate into “clear all labels”, which can bypass allowed_labels intent. Instead, handle errors explicitly (return {success:false, error: ...} for true validation errors) and only treat the specific “no valid labels after allowed filtering” case as a no-op (leave labels undefined so existing labels aren’t removed).
      // Filter to allowed labels only; if none remain treat as an empty set
      const labelsResult = validateLabels(item.labels, allowedLabels);
      if (!labelsResult.valid) {
        // All labels were filtered out (e.g. none in allowed list) - treat as empty set
        updateData.labels = [];
      } else {
        updateData.labels = labelsResult.value ?? [];
      }

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

Comment on lines +305 to +313
if (allowedLabels.length > 0) {
// Filter to allowed labels only; if none remain treat as an empty set
const labelsResult = validateLabels(item.labels, allowedLabels);
if (!labelsResult.valid) {
// All labels were filtered out (e.g. none in allowed list) - treat as empty set
updateData.labels = [];
} else {
updateData.labels = labelsResult.value ?? [];
}
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

In the allowed-labels branch, validateLabels(item.labels, allowedLabels) uses validateLabels' default maxCount=3, which will silently truncate discussions to 3 labels even though MAX_LABELS is 10 and you already enforce tryEnforceArrayLimit with MAX_LABELS. Pass MAX_LABELS (and any other intended params) into validateLabels, or avoid calling validateLabels for limiting and do only allowed-list filtering + sanitization here.

This issue also appears on line 306 of the same file.

Copilot uses AI. Check for mistakes.
Comment on lines +206 to +209
// Compute add/remove sets
const labelsToAdd = requestedLabelData.filter(l => !currentLabelIds.has(l.id)).map(/** @param {any} l */ l => l.id);
const labelsToRemove = currentLabels.filter(/** @param {any} l */ l => !requestedLabelIdSet.has(l.id)).map(/** @param {any} l */ l => l.id);

Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

Label replacement currently removes any existing labels whose IDs aren’t in requestedLabelIdSet, but requestedLabelIdSet is derived only from labels successfully resolved by fetchLabelIds. If the agent provides a label that doesn’t exist (or isn’t in the first 100 repo labels), fetchLabelIds will drop it and you can end up removing all existing labels unintentionally. Consider failing the update when any requested labels can’t be resolved, or (safer) only perform removals when all requested labels were successfully resolved.

Suggested change
// Compute add/remove sets
const labelsToAdd = requestedLabelData.filter(l => !currentLabelIds.has(l.id)).map(/** @param {any} l */ l => l.id);
const labelsToRemove = currentLabels.filter(/** @param {any} l */ l => !requestedLabelIdSet.has(l.id)).map(/** @param {any} l */ l => l.id);
// Detect any requested labels that could not be resolved
const resolvedLabelNames = new Set(requestedLabelData.map(/** @param {any} l */ l => l.name));
const unresolvedLabels = Array.isArray(updateData.labels)
? updateData.labels.filter((/** @type {string} */ name) => !resolvedLabelNames.has(name))
: [];
// Compute add/remove sets
const labelsToAdd = requestedLabelData
.filter((/** @param {any} l */ l) => !currentLabelIds.has(l.id))
.map((/** @param {any} l */ l) => l.id);
/** @type {string[]} */
let labelsToRemove = [];
if (unresolvedLabels.length === 0) {
// Only remove labels when all requested labels were successfully resolved
labelsToRemove = currentLabels
.filter((/** @param {any} l */ l) => !requestedLabelIdSet.has(l.id))
.map((/** @param {any} l */ l) => l.id);
} else {
core.warning(
`Skipping label removals for discussion #${discussionNumber} because some requested labels could not be resolved: ${unresolvedLabels.join(
", "
)}`
);
}

Copilot uses AI. Check for mistakes.
@mnkiefer
Copy link
Contributor

  • Not needed at this time

@mnkiefer mnkiefer closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants