-
Notifications
You must be signed in to change notification settings - Fork 1
ci: add workflow to strip template boilerplate from PR bodies #130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||||||||||||||||||||||||||||||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||||||||||||||||||||||||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| name: Clean PR Body | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
| pull_request: | |
| pull_request_target: |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTML comment stripping regex /^[ \t]*<!--[\s\S]*?-->[ \t]*\n?/gm has an issue: it will strip comments that appear at the start of a line within code blocks. For example, in a Markdown code block showing HTML/XML examples, a line starting with <!-- would be removed even though it's legitimate content, not template boilerplate. The regex should avoid matching inside code fences (delimited by triple backticks). Consider processing the body to skip code blocks, or use a more sophisticated approach that preserves code block contents.
| // preserving inline references like `<!-- -->` in code spans. | |
| let stripped = body.replace(/^[ \t]*<!--[\s\S]*?-->[ \t]*\n?/gm, ''); | |
| // preserving inline references like `<!-- -->` in code spans, | |
| // and avoiding matches inside fenced code blocks (``` ... ```). | |
| const commentRe = /^[ \t]*<!--[\s\S]*?-->[ \t]*\n?/gm; | |
| const segments = body.split(/(```[\s\S]*?```)/); | |
| let stripped = segments | |
| .map((segment, idx) => { | |
| // Captured fenced code blocks appear at odd indices and start with ``` | |
| if (idx % 2 === 1 && segment.startsWith('```')) { | |
| return segment; | |
| } | |
| return segment.replace(commentRe, ''); | |
| }) | |
| .join(''); |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trailer regex on line 48 only matches Signed-off-by and Co-authored-by trailers, but Git and GitHub support additional trailers like Reviewed-by, Tested-by, Acked-by, and others. Consider whether these should also be preserved. If they should be excluded intentionally, the current implementation is correct, but if they should be preserved, the regex should be expanded to match all common Git trailers.
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code destructures only the first element from the match array on line 51: for (const [line] of stripped.matchAll(trailerRe)). However, matchAll returns match objects where the full match is at index 0, and capture groups follow. Since the regex /^(Signed-off-by|Co-authored-by):\s+.+$/gm has one capture group, match[0] is the full match (the entire trailer line), and match[1] is the captured trailer type. Using [line] gets the full match, which is correct. However, it would be clearer to use [fullLine] or match[0] to make the intent more obvious.
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The separator line filter on line 61 only checks for lines that are exactly --------- after trimming. However, GitHub and Markdown may use different numbers of dashes for horizontal rules (typically 3 or more dashes: ---, ----, etc.). This overly specific check may not catch all separator variations. Consider using a regex like /^-{3,}$/ to match any line with 3 or more dashes, or clarify why only 9 dashes specifically should be removed.
| .filter(l => !l.match(/^(Signed-off-by|Co-authored-by):\s+/) && l.trim() !== '---------') | |
| .filter(l => !l.match(/^(Signed-off-by|Co-authored-by):\s+/) && !l.trim().match(/^-{3,}$/)) |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison cleaned !== body.trim() may cause unnecessary API calls when the body has no trailing whitespace but the workflow still reformats it. The issue is that cleaned is built from summary.trim() which removes trailing whitespace, but this is compared against body.trim(). If the original body has leading/trailing whitespace differences or internal whitespace variations (e.g., multiple newlines collapsed to double), this comparison will always be false even when semantically equivalent. Consider comparing both after normalizing whitespace, or compare against the original body without trimming.
| if (cleaned !== body.trim()) { | |
| if (cleaned.replace(/\s+$/, '') !== body.replace(/\s+$/, '')) { |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API call to update the PR body (line 70) has no error handling. If the API call fails (e.g., due to rate limiting, network issues, or permission problems), the workflow will fail silently or with an unclear error. Consider wrapping this in a try-catch block and providing informative error messages, or at least logging when the update succeeds vs. fails.
| await github.rest.pulls.update({ | |
| owner, | |
| repo, | |
| pull_number: pr_number, | |
| body: cleaned, | |
| }); | |
| core.info(`Cleaned PR #${pr_number} body to Summary + trailers`); | |
| try { | |
| await github.rest.pulls.update({ | |
| owner, | |
| repo, | |
| pull_number: pr_number, | |
| body: cleaned, | |
| }); | |
| core.info(`Cleaned PR #${pr_number} body to Summary + trailers`); | |
| } catch (error) { | |
| const message = error && error.message ? error.message : String(error); | |
| core.error(`Failed to clean PR #${pr_number} body: ${message}`); | |
| core.setFailed(`Failed to update PR body for #${pr_number}`); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright header format differs from other workflow files in this repository. Other workflow files use the standard Apache 2.0 header with # comment style, not the SPDX-FileCopyrightText format. For consistency, this should follow the same format as files like ci-checks.yml, copyright-check.yml, and conventional-commit.yml.