Python: Improve PR template and breaking-change label automation#6473
Python: Improve PR template and breaking-change label automation#6473eavanvalkenburg wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
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.ymlto support bracketed title prefixes (e.g.,[BREAKING]) when specific labels are applied. - Added a new
label-breaking-change.ymlworkflow to apply thebreaking changelabel 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). |
|
Flagged issue
Source: automated DevFlow PR review |
There was a problem hiding this comment.
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.ymluses a case-insensitive regex (/\[BREAKING\]/i) to detect the prefix and add the label, butaddBracketPrefixinlabel-title-prefix.ymluses the case-sensitiveString.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 changelabel and[BREAKING]title prefix in two-way sync. From a security/reliability perspective, the changes are sound: both workflows usepull_request_targetsafely (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 thatGITHUB_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 newaddBracketPrefixfunction and the regex-based label detection logic inlabel-breaking-change.ymlare written inline without tests. While the pre-existingaddTitlePrefixfunction 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()andgithub.rest.pulls.update()calls inlabel-title-prefix.ymlare 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 usesawait, 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 thebreaking changelabel 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-26says the label should be added when the title has the[BREAKING]prefix, but line 26 matches[BREAKING]anywhere in the title. A PR titledDocs: explain the [BREAKING] conventionwould be mislabeled as a breaking change. -
.github/workflows/label-title-prefix.yml:28-30,51-58documents bracket-prefix labels as prepending[BREAKING] ..., butaddBracketPrefix()only checkstitle.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
|
Flagged issue
Source: automated DevFlow PR review |
There was a problem hiding this comment.
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 undeclaredlabelAddedvariable, the no-op API calls when the title is unchanged, and the permission concern withissues.addLabels. Beyond those already-flagged items, I found one additional functional gap: the title-to-label sync inlabel-pr.ymlonly fires onopened/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]) intolabel-pr.yml(which triggers on defaultpull_request_targettypes:opened, synchronize, reopened) drops handling of theeditedevent. 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, missingconstforlabelAdded, 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_*.jsusing Node.js's built-innode:testmodule (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.ymldoes notawaitthegithub.rest.issues.update()andgithub.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_targettrigger, 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.ymladds reverse title→label sync logic, but the workflow'spull_request_targettrigger (line 9) uses default activity types (opened,synchronize,reopened) which excludeedited. Changing an existing PR title to include[BREAKING]never reaches this code path. This contradicts the documented contract in.github/pull_request_template.mdand.github/skills/pull-requests/SKILL.mdthat the label and title prefix stay in sync automatically. The deletedlabel-breaking-change.ymlexplicitly handlededited; this is a regression.
Automated review by eavanvalkenburg's agents
|
Flagged issue
Source: automated DevFlow PR review |
|
Addressed the automated review summaries in 3c0321f. The workflow JavaScript now lives in |
- 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>
c0ffae8 to
f7a1de7
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 changelabel to the[BREAKING]title prefix.Description & Review Guide
label-title-prefix.yml: adding thebreaking changelabel now auto-prepends[BREAKING]to the title.label-breaking-change.yml: a PR title containing[BREAKING]auto-applies thebreaking changelabel (reverse direction, loop-safe).Related Issue
Fixes #
Contribution Checklist