Improve signed-commits push refusal rendering in fallback issue bodies #47056
+186
−2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -236,6 +236,80 @@ function buildManifestProtectionCreatePrUrl(githubServer, repoParts, baseBranch, | |
| return createPrUrl; | ||
| } | ||
|
|
||
| /** | ||
| * Build a formatted markdown error section for a push-failure note block. | ||
| * | ||
| * When the raw error message matches the `pushSignedCommits: refusing unsigned push` | ||
| * pattern the section is expanded into a structured block that names the cause and | ||
| * lists remediation steps. For all other errors it degrades gracefully to a single | ||
| * `**Original error:**` line using the sanitised, whitespace-collapsed form. | ||
| * | ||
| * The returned string contains one or more blockquote lines (lines starting with `>`) | ||
| * with no leading or trailing blank lines. It is designed to be embedded directly | ||
| * inside a `> [!NOTE]` block in a GitHub issue body, for example: | ||
| * | ||
| * ``` | ||
| * > [!NOTE] | ||
| * > Intro sentence. | ||
| * > | ||
| * ${buildPushErrorSection(rawMsg, sanitizedMsg)} | ||
| * > | ||
| * > **Workflow Run:** [details](url) | ||
| * ``` | ||
| * | ||
| * @param {string} rawErrorMessage - Unprocessed error message; used for pattern detection and cause extraction. | ||
| * @param {string} sanitizedErrorMessage - Sanitised, whitespace-collapsed message; used as the fallback line. | ||
| * @returns {string} One or more blockquote lines for the error section (no leading/trailing blank lines). | ||
| */ | ||
| function buildPushErrorSection(rawErrorMessage, sanitizedErrorMessage) { | ||
| // Only render the structured block for PushSignedCommitsUnsupportedShape errors, | ||
| // identified by the unique "cannot represent" boilerplate text. PushSignedCommitsPolicyViolation | ||
| // errors also start with "refusing unsigned push" but lack this boilerplate, so they fall | ||
| // through to the sanitized original-error fallback. | ||
| if (!/pushSignedCommits: refusing unsigned push/.test(rawErrorMessage) || !/createCommitOnBranch GraphQL mutation cannot represent/.test(rawErrorMessage)) { | ||
| return `> **Original error:** ${sanitizedErrorMessage}`; | ||
| } | ||
|
|
||
| // Extract the specific cause (e.g. "merge commit detected") anchored to the boilerplate. | ||
| // Using '.*?' (lazy) instead of '[^']*' (restricted) handles apostrophes in branch names. | ||
| const causeMatch = rawErrorMessage.match(/refusing unsigned push for branch '.*?': ([^.]+?)(?=\. GitHub's createCommitOnBranch)/); | ||
| const cause = causeMatch ? causeMatch[1].trim() : "unsupported commit shape"; | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Unsanitized 💡 Suggested fixExtract the cause from const causeMatch = sanitizedErrorMessage.match(/refusing unsigned push for branch [^:]*: ([^.]+)/);
const cause = causeMatch ? causeMatch[1].trim() : "unsupported commit shape";
|
||
| const remediationLines = _remediationForCause(cause); | ||
|
|
||
| return [ | ||
| `> **Error:** Signed commit push refused — ${cause}`, | ||
| `>`, | ||
| `> GitHub's \`createCommitOnBranch\` GraphQL API cannot represent:`, | ||
| `> - Merge commits`, | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The bullet list always enumerates all four unsupported shapes regardless of which cause was detected, which can mislead users. If the cause is 💡 Suggested improvementEither scope the bullet list to shapes relevant to the detected cause, or make the intro more neutral: `> GitHub's \`createCommitOnBranch\` GraphQL API does not support any of the following commit shapes:`A coverage-based test would look like: it('does not list merge commits for submodule cause', () => {
const raw = "pushSignedCommits: refusing unsigned push for branch 'b': submodule change detected.";
expect(buildPushErrorSection(raw, raw)).not.toContain('Merge commits');
});@copilot please address this. |
||
| `> - Symlinks (mode \`120000\`)`, | ||
| `> - Submodule entries (mode \`160000\`)`, | ||
| `> - Executable files (mode \`100755\`)`, | ||
| `>`, | ||
| ...remediationLines, | ||
| ].join("\n"); | ||
| } | ||
|
|
||
| /** | ||
| * Returns cause-specific remediation lines for a signed-commits push refusal. | ||
| * @param {string} cause - The extracted cause string from the error message. | ||
| * @returns {string[]} Two blockquote lines: the fix instruction and the unsigned-push alternative. | ||
| */ | ||
| function _remediationForCause(cause) { | ||
| const c = cause.toLowerCase(); | ||
| let rewriteInstruction; | ||
| if (c.includes("merge commit")) { | ||
| rewriteInstruction = "Use `git rebase` instead of `git merge` to rewrite the commit history without merge commits"; | ||
| } else if (c.includes("submodule")) { | ||
| rewriteInstruction = "Remove the submodule entry from the commit history"; | ||
| } else if (c.includes("symlink")) { | ||
| rewriteInstruction = "Remove the symlink from the commit history"; | ||
| } else { | ||
| rewriteInstruction = "Rewrite the commits to use only regular files (mode `100644`) with no merge commits or special entries"; | ||
| } | ||
| return [`> **To fix:** ${rewriteInstruction},`, `> or set \`signed-commits: false\` in your workflow step if signed commits are not required.`]; | ||
| } | ||
|
|
||
| /** | ||
| * Renders protected-files fallback issue body with a prefilled compare URL. | ||
| * @param {string} mainBodyContent | ||
|
|
@@ -271,4 +345,5 @@ module.exports = { | |
| generatePatchPreview, | ||
| buildManifestProtectionCreatePrUrl, | ||
| renderManifestProtectionFallbackBody, | ||
| buildPushErrorSection, | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[^.]+cause regex truncates at the first period: any cause text containing a.(e.g.executable file at path ./scripts/run.sh detected) is silently truncated, leaving a misleading partial message in the issue body.💡 Suggested fix
Capture up to the sentence-ending period more carefully, or use a greedier stop before the next
\nor end-of-line:Alternatively, if the upstream error format always puts the cause between the colon and the first
., use a non-greedy match against.specifically:/refusing unsigned push for branch '[^']*': (.+?)(?=\. )/Either way, add a test case with a cause that contains a period character.