Skip to content

Python: Improve PR template and breaking-change label automation#6473

Open
eavanvalkenburg wants to merge 7 commits into
microsoft:mainfrom
eavanvalkenburg:improve-pr-template
Open

Python: Improve PR template and breaking-change label automation#6473
eavanvalkenburg wants to merge 7 commits into
microsoft:mainfrom
eavanvalkenburg:improve-pr-template

Conversation

@eavanvalkenburg

Copy link
Copy Markdown
Member

Motivation & Context

The PR template lacked a structured way to link the issue a PR fixes, guidance for reviewers, and reminders about issue linkage. There was also no automation tying the breaking change label to the [BREAKING] title prefix.

Description & Review Guide

  • What are the major changes?
    • PR template: add a structured Related Issue section (GitHub closing keywords), a Review Guide prompt, and checklist items for issue linkage / no duplicate PRs; invert the breaking-change item so the common (non-breaking) case is checked.
    • label-title-prefix.yml: adding the breaking change label now auto-prepends [BREAKING] to the title.
    • New label-breaking-change.yml: a PR title containing [BREAKING] auto-applies the breaking change label (reverse direction, loop-safe).
  • What is the impact of these changes? Better PR hygiene and two-way sync between the label and title prefix. No code/runtime impact.
  • What do you want reviewers to focus on? The two workflow scripts and the loop-safety reasoning.

Related Issue

Fixes #

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue.
  • This is not a breaking change.

Copilot AI review requested due to automatic review settings June 11, 2026 13:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR improves contribution hygiene by enhancing the pull request template and adding/adjusting GitHub Actions automation to keep the breaking change label and a [BREAKING] title prefix in sync.

Changes:

  • Updated PR template to add a structured Related Issue section, review prompts, and checklist adjustments.
  • Extended label-title-prefix.yml to support bracketed title prefixes (e.g., [BREAKING]) when specific labels are applied.
  • Added a new label-breaking-change.yml workflow to apply the breaking change label when the PR title contains [BREAKING].

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
.github/workflows/label-title-prefix.yml Adds bracket-prefix support and refactors title update logic for label-driven prefixes.
.github/workflows/label-breaking-change.yml New workflow to apply the breaking change label based on [BREAKING] in PR titles.
.github/pull_request_template.md Adds Related Issue section, review guide prompts, and updates checklist items (incl. breaking-change guidance).

Comment thread .github/workflows/label-breaking-change.yml Outdated
Comment thread .github/workflows/label-title-prefix.yml Outdated
Comment thread .github/pull_request_template.md Outdated
Comment thread .github/pull_request_template.md Outdated
Comment thread .github/workflows/label-title-prefix.yml Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Flagged issue

.github/workflows/label-breaking-change.yml:23-26 says the label should be added when the title has the [BREAKING] prefix, but line 26 matches [BREAKING] anywhere in the title. A PR titled Docs: explain the [BREAKING] convention would be mislabeled as a breaking change.


Source: automated DevFlow PR review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 5 | Confidence: 87%

✓ Correctness

The two workflows have a case-sensitivity mismatch that can produce duplicate title prefixes. label-breaking-change.yml uses a case-insensitive regex (/\[BREAKING\]/i) to detect the prefix and add the label, but addBracketPrefix in label-title-prefix.yml uses the case-sensitive String.includes() to check if the prefix is already present. If a user types [Breaking] or [breaking] in the title, the first workflow adds the label (case-insensitive match), and the second workflow then prepends [BREAKING] again (case-sensitive check fails), resulting in [BREAKING] [Breaking] ....

✓ Security Reliability

This PR adds a new GitHub Actions workflow and modifies an existing one to keep the breaking change label and [BREAKING] title prefix in two-way sync. From a security/reliability perspective, the changes are sound: both workflows use pull_request_target safely (no code checkout from forks, only metadata reads and API calls), the action is pinned to a commit hash, permissions are minimally scoped, and loop safety between the two workflows is guaranteed by GitHub's built-in protection that GITHUB_TOKEN-triggered events do not create new workflow runs.

✓ Test Coverage

This PR adds GitHub Actions workflow automation and PR template improvements. The repository has an established pattern of extracting workflow JavaScript logic into .github/scripts/ with corresponding tests in .github/tests/ (e.g., check_team_membership.js, pr_limit_moderation.js). The new addBracketPrefix function and the regex-based label detection logic in label-breaking-change.yml are written inline without tests. While the pre-existing addTitlePrefix function was also untested, the new logic introduces additional complexity (case-insensitive regex matching, idempotency checks, two-workflow loop safety) that would benefit from unit tests following the repo's own conventions.

✓ Failure Modes

The PR is well-structured and the loop-safety between the two workflows is sound (GITHUB_TOKEN events don't trigger new workflow runs). The only operational concern is that the github.rest.issues.update() and github.rest.pulls.update() calls in label-title-prefix.yml are not awaited, meaning API failures (rate limits, permission errors, transient failures) are silently lost and the title update may not complete before the step exits. The new companion workflow (label-breaking-change.yml) correctly uses await, making this an inconsistency.

✗ Design Approach

The automation is close, but both workflow changes currently key off the presence of [BREAKING] anywhere in the title instead of treating it as a true title prefix. That creates false positives when a title merely mentions the token, and it also fails to normalize the token to the front of the title when the breaking change label is applied. Those cases contradict the new template text and the comments in the workflows themselves.

Flagged Issues

  • .github/workflows/label-breaking-change.yml:23-26 says the label should be added when the title has the [BREAKING] prefix, but line 26 matches [BREAKING] anywhere in the title. A PR titled Docs: explain the [BREAKING] convention would be mislabeled as a breaking change.
  • .github/workflows/label-title-prefix.yml:28-30,51-58 documents bracket-prefix labels as prepending [BREAKING] ..., but addBracketPrefix() only checks title.includes(prefix). If the title already contains [BREAKING] later in the string, applying the label leaves the marker in the middle instead of making it a prefix.

Automated review by eavanvalkenburg's agents

@github-actions

Copy link
Copy Markdown
Contributor

Flagged issue

.github/workflows/label-title-prefix.yml:28-30,51-58 documents bracket-prefix labels as prepending [BREAKING] ..., but addBracketPrefix() only checks title.includes(prefix). If the title already contains [BREAKING] later in the string, applying the label leaves the marker in the middle instead of making it a prefix.


Source: automated DevFlow PR review

@moonbox3 moonbox3 added documentation Improvements or additions to documentation python .NET labels Jun 11, 2026
@github-actions github-actions Bot changed the title Improve PR template and breaking-change label automation .NET: Improve PR template and breaking-change label automation Jun 11, 2026
@github-actions github-actions Bot changed the title Improve PR template and breaking-change label automation Python: Improve PR template and breaking-change label automation Jun 11, 2026
@eavanvalkenburg eavanvalkenburg marked this pull request as ready for review June 11, 2026 15:28

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 5 | Confidence: 85%

✓ Correctness

The PR adds PR template improvements and two-way breaking-change label automation. The existing unresolved review comments correctly identify the main issues: the case-sensitive duplicate-prefix check in addBracketPrefix, the undeclared labelAdded variable, the no-op API calls when the title is unchanged, and the permission concern with issues.addLabels. Beyond those already-flagged items, I found one additional functional gap: the title-to-label sync in label-pr.yml only fires on opened/synchronize/reopened (the defaults), so editing a title to add [BREAKING] after the PR is opened will not auto-apply the label until the next push.

✓ Security Reliability

The migration of breaking-change label logic from the deleted label-breaking-change.yml (which triggered on [opened, edited]) into label-pr.yml (which triggers on default pull_request_target types: opened, synchronize, reopened) drops handling of the edited event. This means if a user adds [BREAKING] to a PR title after opening, the label will no longer be auto-applied — a regression in the stated two-way sync feature. The remaining issues (case-sensitive duplicate-prefix risk, missing const for labelAdded, no-op API calls) are already tracked in existing unresolved review threads.

✓ Test Coverage

This PR adds new JavaScript logic (addBracketPrefix function, breaking-change label sync) inline in GitHub Actions workflow files but provides no unit tests. The repository has an established pattern for testing workflow scripts: logic is extracted into .github/scripts/ and tested via .github/tests/test_*.js using Node.js's built-in node:test module (see test_pr_limit_moderation.js, test_check_team_membership.js). The new addBracketPrefix function and the label↔title sync logic have testable edge cases (idempotency, case sensitivity, loop safety) that would benefit from the same treatment.

✓ Failure Modes

The restructured title-update logic in label-title-prefix.yml does not await the github.rest.issues.update() and github.rest.pulls.update() calls. If the API request fails (rate-limit, network timeout, 5xx), the promise rejects after the script function has already returned, so the step reports success and the error is silently lost — breaking the two-way sync guarantee without any indication.

✗ Design Approach

The new breaking-change automation has one design gap: it only checks the PR title on the workflow's existing pull_request_target trigger, so editing an already-open PR title to add [BREAKING] will not apply the label. That leaves the newly documented “label/title stay in sync automatically” behavior only partially implemented.

Flagged Issues

  • .github/workflows/label-pr.yml adds reverse title→label sync logic, but the workflow's pull_request_target trigger (line 9) uses default activity types (opened, synchronize, reopened) which exclude edited. Changing an existing PR title to include [BREAKING] never reaches this code path. This contradicts the documented contract in .github/pull_request_template.md and .github/skills/pull-requests/SKILL.md that the label and title prefix stay in sync automatically. The deleted label-breaking-change.yml explicitly handled edited; this is a regression.

Automated review by eavanvalkenburg's agents

@github-actions

Copy link
Copy Markdown
Contributor

Flagged issue

.github/workflows/label-pr.yml adds reverse title→label sync logic, but the workflow's pull_request_target trigger (line 9) uses default activity types (opened, synchronize, reopened) which exclude edited. Changing an existing PR title to include [BREAKING] never reaches this code path. This contradicts the documented contract in .github/pull_request_template.md and .github/skills/pull-requests/SKILL.md that the label and title prefix stay in sync automatically. The deleted label-breaking-change.yml explicitly handled edited; this is a regression.


Source: automated DevFlow PR review

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Addressed the automated review summaries in 3c0321f. The workflow JavaScript now lives in .github/scripts/title_prefix.js with node:test coverage, reverse sync runs on PR title edits, [BREAKING] detection is limited to the title prefix group while supporting both [BREAKING] Python: and Python: [BREAKING] ordering, title updates are awaited/skipped when unchanged, and the PR template/skill guidance were updated.

Comment thread dotnet/Directory.Packages.props Outdated
eavanvalkenburg and others added 5 commits June 12, 2026 14:32
- Add a structured "Related Issue" section using GitHub closing keywords
- Add a Review Guide prompt (major changes, impact, reviewer focus) with a
  note that the focus item is for human reviewers only
- Add checklist items for issue linkage / no duplicate PRs and invert the
  breaking-change item (checked = not breaking)
- Extend label-title-prefix to prepend [BREAKING] when the "breaking change"
  label is added
- Add label-breaking-change workflow to apply the "breaking change" label
  when a PR title contains [BREAKING]

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add root .github/skills/pull-requests/SKILL.md covering PR description
  authoring (following the PR template) and the review-comment workflow
  (review -> plan -> user review -> implement -> reply to all -> resolve)
- Symlink the skill from python/.github/skills and dotnet/.github/skills
- Reference the skill from python/AGENTS.md and dotnet/AGENTS.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the title -> 'breaking change' label logic into the existing label-pr
workflow (which already applies the python/.NET labels) and drop the separate
label-breaking-change workflow.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation .NET python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants