fix(release): harden stale-branch fallback error handling (review follow-up)#169
Merged
Conversation
derekmisler
previously approved these changes
May 4, 2026
Three changes from code review of PR #168: 1. [NOTABLE] Capture deleteRef error; only swallow 404 (race-condition concurrent deletion). All other deleteRef failures are now wrapped with context linking back to the original updateRef error and re-thrown so createRef is never called blindly. 2. [MINOR] Add `working-directory: ${{ github.workspace }}` to the 'Create release commit with pinned refs' step, making the CWD assumption explicit rather than relying on the GitHub Actions default. 3. [MINOR] Narrow the `else if (status === 422)` stale-branch path to only fire when GitHub returns 'Reference already exists'. Any other 422 (invalid ref name, permissions, etc.) is now re-thrown unchanged instead of triggering a silent delete+recreate. Tests updated accordingly: replace the two original stale-branch tests with four targeted cases covering the narrowed trigger condition, the re-throw path for unknown 422s, the 404 race-condition pass-through, and the non-race deleteRef failure wrap-and-rethrow. All 34 unit tests pass. Assisted-By: docker-agent
docker-agent
dismissed
derekmisler’s stale review
The merge-base changed after approval.
docker-agent
force-pushed
the
fix/release-absolute-path-bug
branch
from
e3e7579 to
a127e30
Compare
Assisted-By: docker-agent
docker-agent
commented
May 4, 2026
Contributor
Author
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
All three targeted fixes (narrowed 422 guard, structured deleteRef error propagation, explicit working-directory) are correctly implemented. CI is green (13/13 checks). Two minor observations noted as inline comments — neither blocks merge.
F1: Document that force:true hard-resets the target branch to baseRef
HEAD, calling out the footgun for long-lived branches.
F2: Pass { cause: deleteErr } to the wrapped Error thrown on non-404
deleteRef failures so Node error chaining preserves the original stack.
Assisted-By: docker-agent
derekmisler
approved these changes
May 4, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Context
Follow-up to #168 (now merged). Addresses three findings from the PR review:
🟡 Fix 1 (NOTABLE):
deleteReferror now propagated unless it's a 404 raceBefore: The
catch {}block silently swallowed alldeleteReferrors, socreateRefwas called unconditionally even if deletion failed for a non-race reason (permissions, transient network), potentially leaving the branch in an inconsistent state with no useful error.After:
deleteReferrors are captured. A 404 is treated as a race-condition success (the branch is already gone) andcreateRefproceeds. Any other status throws a contextual error that includes thedeleteRefstatus/message and the originalupdateReferror, so the operator knows exactly what happened:🔵 Fix 2 (MINOR): Explicit
working-directoryon the release stepAdded
working-directory: ${{ github.workspace }}to theCreate release commit with pinned refsstep so the CWD assumption that makesfind dist/ -type fcorrect is explicit, not implicit.🔵 Fix 3 (MINOR): Narrowed
else if (status === 422)guardBefore: Any 422 from
updateRefthat wasn't"Reference does not exist"would silently trigger delete+recreate, masking unrelated errors (invalid ref name, protected ref, etc.).After: The delete+recreate path only fires on
"Reference already exists". All other 422s are re-thrown unchanged.Tests
Replaced the two original stale-branch tests with four targeted cases:
'Reference already exists'422 → triggers delete+recreate ✓deleteRefreturns 404 (race) →createRefstill runs ✓deleteRefreturns non-404 → wrapped error re-thrown,createRefnot called ✓All 34 unit tests pass. TypeScript type-check passes.