Improve signed-commits push refusal rendering in fallback issue bodies#47056
Conversation
…odies Add `buildPushErrorSection` helper that detects signed-commits refusal errors and expands them into structured markdown with cause, a list of unsupported commit shapes, and remediation steps (git rebase / signed- commits: false), instead of rendering as a wall of text. Use it in both the bundle-path and patch-path fallback issue bodies in create_pull_request.cjs, and add 8 unit tests in the helpers test file. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves fallback issue readability for signed-commit push refusals.
Changes:
- Adds structured push-error rendering.
- Integrates it into bundle and patch fallback paths.
- Adds helper unit tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_pull_request.cjs |
Uses structured error sections in fallback issues. |
actions/setup/js/create_pull_request_helpers.cjs |
Implements signed-push error formatting. |
actions/setup/js/create_pull_request_helpers.test.cjs |
Tests the new formatter. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
| * @returns {string} One or more blockquote lines for the error section (no leading/trailing blank lines). | ||
| */ | ||
| function buildPushErrorSection(rawErrorMessage, sanitizedErrorMessage) { | ||
| if (!/pushSignedCommits: refusing unsigned push/.test(rawErrorMessage)) { |
| } | ||
|
|
||
| // Extract the specific cause (e.g. "merge commit detected") embedded in the raw message. | ||
| const causeMatch = rawErrorMessage.match(/refusing unsigned push for branch '[^']*': ([^.]+)/); |
| `> **To fix:** Use \`git rebase\` instead of \`git merge\` to incorporate upstream changes,`, | ||
| `> or set \`signed-commits: false\` in your workflow step if signed commits are not required.`, |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47056 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The structured error rendering for signed-commits push refusals is a clear readability improvement. Three issues from inline comments warrant attention:
- Overly broad pattern match — The regex also matches PushSignedCommitsPolicyViolation errors (max-patch, file-protection), incorrectly rendering those as signed-commits shape errors.
- Branch-name regex fragility — The cause extraction regex fails silently for branch names containing an apostrophe.
- Inaccurate remediation for non-merge-commit causes — git rebase only helps for merge commits; symlink/submodule/executable-bit causes need cause-specific guidance.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 13.9 AIC · ⌖ 4.37 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report
📊 Metrics (8 tests)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on three correctness issues in the new helper.
📋 Key Themes & Highlights
Key Findings
- Overly broad pattern match (line 265):
pushSignedCommits:also prefixesPushSignedCommitsPolicyViolationerrors (max-patch, file-protection), which will be incorrectly rendered as topology errors with wrong remediation steps. - Regex fragility (line 270): Branch names with apostrophes break the cause-extraction regex; a malformed result silently falls back to the generic phrase.
- Inaccurate remediation (line 283):
git rebasefixes merge-commit topology but does not help for submodule or symlink causes — those need content removal, not rebase. - Hardcoded bullet list (line 277): All four shapes are listed for every cause, even when only one is relevant, which can confuse users.
Positive Highlights
- ✅ Excellent test suite — 8 focused tests covering detection, extraction, invariants, and fallback
- ✅ Clean JSDoc with embedded usage example
- ✅ Graceful fallback for unrecognised errors
- ✅ Both push-path call sites updated consistently
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 25.4 AIC · ⌖ 4.58 AIC · ⊞ 6.6K
Comment /matt to run again
| `> **Error:** Signed commit push refused — ${cause}`, | ||
| `>`, | ||
| `> GitHub's \`createCommitOnBranch\` GraphQL API cannot represent:`, | ||
| `> - Merge commits`, |
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 submodule change detected, listing Merge commits as equally applicable implies the commit has both problems. The test suite does not verify this — a test that checks only relevant shapes appear for each cause would catch this.
💡 Suggested improvement
Either 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.
There was a problem hiding this comment.
REQUEST_CHANGES — two issues must be fixed before merge.
Blocking findings
1. Unsanitized cause embedded in markdown output (high)
The cause captured from rawErrorMessage is interpolated directly into the returned string. rawErrorMessage is the unprocessed git error at the call site — it bypasses sanitizeContent and neutralizeClosingKeywordsForIssueBody. If an attacker controls the remote error text or branch name, they can inject arbitrary markdown into the fallback issue body (e.g. mention aliases, issue-closing keywords, links). The safe string — sanitizedErrorMessage — is passed to the function but never used for cause extraction. See inline comment at line 272.
2. [^.]+ regex silently truncates causes containing a period (medium)
Any cause string with a . (e.g. executable file at path ./run.sh detected) is cut off at the dot, leaving a misleading partial message. See inline comment at line 268.
Already flagged by earlier comments (not duplicated): over-broad detection pattern (#265), apostrophe in branch name breaking the regex (#270), and
git rebaseadvice not applicable to symlink/submodule causes (#283). Those issues remain open and should be addressed together with these.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 43.5 AIC · ⌖ 4.69 AIC · ⊞ 5.6K
Comment /review to run again
| // Extract the specific cause (e.g. "merge commit detected") embedded in the raw message. | ||
| const causeMatch = rawErrorMessage.match(/refusing unsigned push for branch '[^']*': ([^.]+)/); | ||
| const cause = causeMatch ? causeMatch[1].trim() : "unsupported commit shape"; | ||
|
|
There was a problem hiding this comment.
Unsanitized cause injected into markdown: cause is captured from rawErrorMessage — the unprocessed git error — and interpolated directly into the output string without any escaping. A crafted remote branch name or git error text could embed arbitrary markdown (extra blockquote lines, @mentions, closing keywords, links) into the fallback issue body.
💡 Suggested fix
Extract the cause from sanitizedErrorMessage instead, or sanitize the captured group before embedding it:
const causeMatch = sanitizedErrorMessage.match(/refusing unsigned push for branch [^:]*: ([^.]+)/);
const cause = causeMatch ? causeMatch[1].trim() : "unsupported commit shape";sanitizedErrorMessage is already processed through sanitizeContent + neutralizeClosingKeywordsForIssueBody at the call site. The raw message is fine for the guard regex, but any captured text that lands in the output must come from the sanitized path.
| if (!/pushSignedCommits: refusing unsigned push/.test(rawErrorMessage)) { | ||
| return `> **Original error:** ${sanitizedErrorMessage}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
[^.]+ 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 \n or end-of-line:
// Stop at '. ' (period+space) or end-of-line instead of any period
const causeMatch = rawErrorMessage.match(/refusing unsigned push for branch '[^']*': ([^\n]+?)(?:\. |$)/);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.
|
@copilot run pr-finisher and lint js |
…oilerplate, fix apostrophe regex, add cause-specific remediation - Issue 1: tighten detection to require 'createCommitOnBranch GraphQL mutation cannot represent' boilerplate so PushSignedCommitsPolicyViolation errors (max-patch, file-protection) still use the sanitized original-error fallback - Issue 2: change '[^']*' to lazy '.*?' and anchor cause extraction to the boilerplate with (?=\. GitHub's createCommitOnBranch) so branch names containing apostrophes are matched correctly - Issue 3: extract _remediationForCause() with cause-specific guidance: merge commit → git rebase, submodule → remove submodule entry, symlink → remove symlink, default → shape-neutral rewrite - Tests: add policy-violation regression test, apostrophe-branch test, symlink remediation test; update submodule test to assert cause-specific text and absence of git rebase; update malformed test to use boilerplate-present-but-no-branch-pattern input Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — addressed all three reviewer comments and linted:
All 80 unit tests pass, |
When
create_pull_requestfalls back to an issue after a signed-commits push refusal (e.g. merge commit topology), the error message was emitted as a single sanitized line — a wall of unreadable text buried in a blockquote.Changes
create_pull_request_helpers.cjs— addsbuildPushErrorSection(rawErrorMessage, sanitizedErrorMessage):pushSignedCommits: refusing unsigned pushpatternmerge commit detected,submodule change detected)> **Original error:** …single line for all other errorscreate_pull_request.cjs— usesbuildPushErrorSectionin both the bundle-path and patch-path push-failure fallback bodies, replacing the inline> **Original error:** ${pushFailureMessage}linecreate_pull_request_helpers.test.cjs— 8 new unit tests covering detection, cause extraction, shape listing, remediation text, blockquote-only output invariant, and edge casesBefore
After