Retry upload-asset pushes after concurrent branch updates - #51893
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51893 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. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Adds bounded recovery for concurrent asset-branch updates.
Changes:
- Retries non-fast-forward pushes after fetching and rebasing.
- Preserves diagnostics and avoids retrying permanent failures.
- Tests recovery, retry exhaustion, and non-retryable errors.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/upload_assets.cjs |
Implements conflict-aware push retries. |
actions/setup/js/upload_assets.test.cjs |
Covers retry and failure scenarios. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The retry logic for concurrent branch updates looks correct and well-tested. Implementation uses --porcelain with ignoreReturnCode, joins stderr+stdout for error detection, uses a regex covering both non-fast-forward and fetch first patterns, retries with fetch+rebase, and throws immediately for non-concurrent failures. Tests cover all three code paths. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.2 AIC · ⌖ 9.05 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Key Strengths:
Test Inflation Analysis:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues before merging.
📋 Key Themes & Highlights
Key Issues
- Missing
git rebase --abort— if rebase encounters a conflict the working tree is left inREBASE_HEADstate, corrupting subsequent Git operations in the same runner (high risk for CI environments). stderr/stdoutordering for--porcelain— porcelain rejection lines go to stdout; current join order puts stderr first, so the regex and error message may miss them on some git versions.- Minor: retry-exhaustion test could be more explicit about the surfaced error value.
Positive Highlights
- ✅ Retry cap (3 attempts) and fast-fail for permanent errors (permissions, branch protection) are well-designed.
- ✅ Three-scenario test suite (recovery, exhaustion, non-retryable) matches the three code paths exactly — great TDD coverage.
- ✅ Warning log on each retry provides actionable diagnostics for operators.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.6 AIC · ⌖ 6.71 AIC · ⊞ 7.1K
Comment /matt to run again
| const remoteBranch = `refs/remotes/origin/${normalizedBranchName}`; | ||
| await exec.exec("git", ["fetch", "--no-tags", "origin", `+refs/heads/${normalizedBranchName}:${remoteBranch}`]); | ||
| await exec.exec("git", ["rebase", remoteBranch]); | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] If git rebase fails (e.g. merge conflict), the process throws without aborting the in-progress rebase, leaving the working tree in a broken REBASE_HEAD state for any subsequent Git operations in the same runner.
💡 Suggested fix: abort rebase on failure
try {
await exec.exec('git', ['rebase', remoteBranch]);
} catch (rebaseErr) {
await exec.exec('git', ['rebase', '--abort'], { ignoreReturnCode: true });
throw rebaseErr;
}Without --abort, a rebase conflict corrupts the workspace for any further Git operations.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 56402d0. Added try/catch around git rebase that calls git rebase --abort with ignoreReturnCode: true before rethrowing, so the working tree is never left in a broken REBASE_HEAD state. A test for this path was also added.
| } | ||
| const pushError = [pushResult.stderr, pushResult.stdout].filter(Boolean).join("\n").trim() || `git push exited with code ${pushResult.exitCode}`; | ||
| const isNonFastForward = /non-fast-forward|fetch first/i.test(pushError); | ||
| if (!isNonFastForward || attempt === maxPushAttempts) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The error message is built by joining stderr then stdout, but --porcelain output (including rejection lines like ! [rejected] ...) goes to stdout, not stderr. On a rejection, stderr is often empty and stdout carries the diagnostic — so the order should be stdout first, or both should be shown clearly.
💡 Suggested fix
const pushError = [pushResult.stdout, pushResult.stderr].filter(Boolean).join("\n").trim()
|| `git push exited with code ${pushResult.exitCode}`;This ensures the porcelain rejection line (! [rejected]) is the leading text in the error message and in the regex test for isNonFastForward.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 5e1da83. Swapped the join order to [pushResult.stdout, pushResult.stderr] so the porcelain rejection line (! [rejected]) leads the error message and is the primary input to the isNonFastForward regex. Test mocks updated to place the rejection text in stdout to match actual --porcelain behaviour.
| expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "--no-tags", "origin", "+refs/heads/assets/test-workflow:refs/remotes/origin/assets/test-workflow"]); | ||
| expect(mockExec.exec).toHaveBeenCalledWith("git", ["rebase", "refs/remotes/origin/assets/test-workflow"]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The retry-exhaustion test verifies getExecOutput is called 3 times, but doesn't assert the final error message thrown (via mockCore.setFailed) contains the right content. If the error were swallowed or reformatted, the test would still pass.
💡 Suggested assertion
expect(mockCore.setFailed).toHaveBeenCalledWith(
expect.stringContaining('non-fast-forward')
);This is partially present but the test name says "stop after three attempts" — adding an explicit assertion about what error is surfaced makes the specification complete.
@copilot please address this.
There was a problem hiding this comment.
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("non-fast-forward")) was already present in the test as of d67cbba. The test mock was also updated in 5e1da83 to put the rejection text in stdout (matching --porcelain output), so the assertion now exercises the correct code path end-to-end.
|
@copilot quick triage: this PR has a fresh blocking Matt Pocock review requesting changes. Please address the missing
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit |
|
@copilot quick triage: your follow-up already landed after the prior sous-chef nudge, and checks now look complete. Please do one final pr-finisher pass focused on the remaining maintainer confidence items from the Matt review response, confirm the branch is clean after the rebase-abort safeguard test, and hand back a short maintainer-facing summary. Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31454952282
|
…t push --porcelain output Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Concurrent workflow runs can publish assets from the same branch tip, causing one push to fail with a non-fast-forward rejection and leaving its asset unavailable.
fetch firstandnon-fast-forwardpush rejections.Run URL: https://github.com/github/gh-aw/actions/runs/31450173394> Generated by 👨🍳 PR Sous Chef · gpt54 · 24.1 AIC · ⌖ 5.21 AIC · ⊞ 6.1K · ◷
@copilotquick triage: your follow-up already landed after the prior sous-chef nudge, and checks now look complete. Please do one final pr-finisher pass focused on the remaining maintainer confidence items from the Matt review response, confirm the branch is clean after the rebase-abort safeguard test, and hand back a short maintainer-facing summary.Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31454952282> Generated by 👨🍳 PR Sous Chef · gpt54 · 15.9 AIC · ⌖ 5.21 AIC · ⊞ 8.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31454952282> Generated by 👨🍳 PR Sous Chef · gpt54 · 15.9 AIC · ⌖ 5.21 AIC · ⊞ 8.5K · ◷