Fall back to unsigned push instead of failing when a rebase hits a genuine merge conflict in pushSigned提交#56568
pushSigned提交#56568Conversation
…mmits Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
pushSigned提交
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
Lean already. Ship. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See 网络 Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
This fallback fixes one failure mode, but it introduces a worse one: on a genuine rebase conflict you now silently abandon the signed-commit path and push unsigned commits instead. That can violate branch rules in repositories that require signed commits, and there is no coverage here proving the caller handles that policy failure cleanly instead of regressing back into the old issue-fallback path.
Key blocking theme
- Genuine content conflicts now take a completely different transport path with different policy semantics.
- The new tests only cover the happy path where unsigned pushes are allowed; they do not cover a signed-commits-required repo or a failing fallback push.
- This makes the behavior change under-protected for the exact class of repos that opted into signed replay in the first place.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 9.85 AIC · ⌖ 7.34 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one comment, one minor issue. Overall this is a well-structured, well-tested fix.
📋 Key Themes & Highlights
Key Themes
- Warning message accuracy: The
conflictMessagestring includes a user-facing "retry" instruction that is misleading when re-used inside the success-pathcore.warningcall. Also, prefixing a non-error outcome withERR_SYSTEM:may cause false positives in log monitoring. See the inline comment on line 604.
Positive Highlights
- ✅ The fix is minimal and surgical — only the genuine conflict branch gains new behaviour; the shallow/partial-clone path is unchanged.
- ✅
allowGitPushFallback: falseguard is correctly respected, preserving the strict-failure mode for callers that rely on it. - ✅ 新建 tests are thorough: fallback success path, explicit-disable path, and updated backfill test all cover the changed logic, with clear arrange/act/assert structure.
- ✅ Good changeset entry explaining the before/after behaviour.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 26.9 AIC · ⌖ 10.5 AIC · ⊞ 7.7K
Comment /matt to run again
| // of a pull request), push the ORIGINAL un-rebased commits directly via unsigned | ||
| // `git push`. GitHub will still create the pull request; it will simply report the | ||
| // branch as having conflicts that need to be resolved, the same as any normal PR. | ||
| core.warning(`${conflictMessage} Falling back to an unsigned git push of the un-rebased commit(s) so the pull request can still be created (it will show as having merge conflicts with the base branch).`); |
There was a problem hiding this comment.
[/diagnosing-bugs] The warning string embeds conflictMessage verbatim, which includes "Resolve conflicts by rebasing/cherry-picking locally and retry." — but in this path the operation succeeds, so that instruction is misleading to anyone reading logs.
💡 Suggested fix
Split the message so the "retry" instruction only appears in the thrown error, not in the fallback-success warning:
const diagnosticMessage =
`${ERR_SYSTEM}: pushSigned提交: failed to rebase commit range onto current GraphQL parent (${firstGraphqlParentOid}). ` +
`Root cause: ${combinedOutput.trim()}`;
if (allowGitPushFallback === false) {
throw new Error(`${diagnosticMessage} Resolve conflicts by rebasing/cherry-picking locally and retry.`);
}
core.warning(
`${diagnosticMessage} Falling back to an unsigned git push of the un-rebased commit(s) so the pull request can still be created (it will show as having merge conflicts with the base branch).`
);This also keeps the ERR_SYSTEM: prefix out of a warning that doesn't represent a system failure, which may otherwise trigger false monitoring alerts.
@copilot please address this.
There was a problem hiding this comment.
Fixed as suggested: split the message so the "retry" instruction only appears on the throw path, and the fallback-success warning no longer implies a retry is needed.
There was a problem hiding this comment.
Clean, well-tested fix. The allowGitPushFallback === false strict-equality guard correctly defaults to allowing the fallback for existing callers (undefined is not strictly false). The "no-backfill" test correctly gets allowGitPushFallback: false to preserve its throw semantics — without it that test would silently start passing for the wrong reason. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 21.3 AIC · ⌖ 10.5 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Pull request overview
Adds unsigned push fallback so conflicting agent branches can still open pull requests.
Changes:
- Falls back after genuine rebase conflicts.
- Preserves strict mode when fallback is disabled.
- Adds integration coverage and a changeset.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/push_signed_commits.cjs |
Implements conflict fallback. |
actions/setup/js/push_signed_commits.test.cjs |
Tests fallback and strict failure. |
.changeset/fix-signed-push-genuine-rebase-conflict-fallback.md |
Documents the behavior change. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
| // of a pull request), push the ORIGINAL un-rebased commits directly via unsigned | ||
| // `git push`. GitHub will still create the pull request; it will simply report the | ||
| // branch as having conflicts that need to be resolved, the same as any normal PR. | ||
| core.warning(`${conflictMessage} Falling back to an unsigned git push of the un-rebased commit(s) so the pull request can still be created (it will show as having merge conflicts with the base branch).`); | ||
| const fallbackSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken }); |
| // Genuine merge conflict (not a shallow/partial-clone object-fetch issue, and no custom | ||
| // resolver handled it): rebasing the commit range onto the current base cannot be done | ||
| // automatically, and replaying the stale-base commits through GraphQL would silently |
| const fallbackSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken }); | ||
| core.info(`pushSigned提交: unsigned git push fallback (unresolved rebase conflict) completed, using pushed SHA ${fallbackSha}`); | ||
| return fallbackSha; |
|
Branch protection will block unsigned commits so this will fail for branches who require signed commits. You can use signed/* in gh-aw to trigger this protection. |
|
Category: bug | Risk: medium | Score: 58/100 (impact 27/50, urgency 16/30, quality 15/20) | Action: fast_track | Batch: workflow-ux
|
|
Category: bug | Risk: medium | Score: 72 (impact 40 + urgency 18 + quality 14) | Action: fast_track | Batch: workflow-ux. This is a high-value workflow/PR-creation fix with clear regression coverage and a targeted fallback path.
|
|
@copilot There is still blocking review feedback on this PR. Please address the signed-commit branch-protection concern, verify the fallback-path coverage is complete for policy-rejected repos, refresh the branch if needed, and then run the Current blocking feedback, newest first:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See 网络 Configuration for more information.
|
…allback Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Found one remaining gap from the linked review: when the object backfill succeeds but the retried rebase still fails, that was a genuine content conflict revealed post-backfill, yet it unconditionally threw instead of taking the unsigned-push fallback. Fixed in a5f0498's follow-up (latest commit) — that path now goes through the same fallback, preflight checks, and fatal-abort handling as the non-backfill genuine-conflict case, with a new test ( |
|
Category: bug | Risk: medium | Score: 56 (impact 27/urgency 15/quality 14) | Action: fast_track | CI: unknown
|
|
Category: bug | Risk: medium | Score: 56 (impact 27/urgency 15/quality 14) | Action: fast_track
|
|
Category: bug | Risk: medium | Score: 70/100 (impact 32/50, urgency 24/30, quality 14/20) | Action: fast_track | Batch: workflow-safety | Note: High-impact fallback behavior for signed-push/PR creation; good fit for expedited review.
|
|
@copilot There is still blocking review feedback on this PR. Please verify the latest signed-commit branch-protection handling on the current head, address any remaining review concern, refresh the branch if needed, and then run the Current blocking feedback, newest first:
Your follow-up says the combined-error path and signed-commit rejection coverage were added, but this PR still needs a fresh maintainer-ready handoff on the current head. Run: https://github.com/github/gh-aw/actions/runs/33241945198
|
Verified on the current head (8ab960c, after merging main): the branch-protection combined-error path ( |
|
@copilot Please refresh this PR for forward progress. Re-run the Run: https://github.com/github/gh-aw/actions/runs/33246602890
|
Branch is already up to date with |
When an agent branch conflicted with content added to the base branch (e.g. both modified
CHANGELOG.md),pushSigned提交aborted the pre-replay rebase and threw. This causedcreate_pull_requestto abandon the pull request entirely and fall back to opening a GitHub issue, even though GitHub could have created the PR and simply flagged it as having conflicts to resolve.Fix
pushSigned提交(actions/setup/js/push_signed_commits.cjs), when the rebase-onto-current-base fails due to a genuine merge conflict (i.e. not a recoverable shallow/partial-clone object-fetch issue, and not resolved by a custom conflict resolver):git push, instead of throwing.allowGitPushFallback: false.Tests
push_signed_commits.test.cjsto cover the new fallback-on-conflict success path (branch pushed as-is, expected warnings emitted) and to keep an explicit test asserting the original throw behavior whenallowGitPushFallback: false.Also added a changeset describing the behavioral change.
Run: https://github.com/github/gh-aw/actions/runs/33195749972
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See 网络 Configuration for more information.
Automated branch refresh attempt from PR Sous Chef run https://github.com/github/gh-aw/actions/runs/33246602890.