fix(cli): fall back to PR when default branch is protected#2201
fix(cli): fall back to PR when default branch is protected#2201waynesun09 wants to merge 1 commit into
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://d0e5886d-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · Started 1:12 AM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
waynesun09
force-pushed
the
fix-protected-branch-fallback
branch
from
9be2798 to
cc39241
Compare
waynesun09
added
the
ok-to-test
|
🤖 Finished Review · ✅ Success · Started 1:17 AM UTC · Completed 1:29 AM UTC |
ReviewFindingsMedium
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| if !strings.Contains(branchErr.Error(), "already exists") { | ||
| printer.StepFail("Failed to create scaffold branch") | ||
| return fmt.Errorf("creating scaffold branch: %w", branchErr) | ||
| } |
There was a problem hiding this comment.
[medium] error-handling-idiom
Error checking uses strings.Contains for already exists instead of typed error comparison. The codebase uses errors.Is() and this same PR introduces the ErrBranchProtected sentinel pattern, making the inconsistency notable.
Suggested fix: Define forge.ErrAlreadyExists sentinel and use errors.Is() checks, following the ErrBranchProtected pattern introduced in this PR.
There was a problem hiding this comment.
Acknowledged — an ErrAlreadyExists sentinel would be cleaner. However, this is a broader refactor: CreateBranch returns 422 with "Reference already exists" and CreateChangeProposal returns 422 with "A pull request already exists" — both would need the sentinel, and the forge interface is forge-agnostic (can't depend on github.APIError). The string check is pragmatic for this PR. Filed as a follow-up improvement.
|
|
||
| const scaffoldBranch = "fullsend/scaffold-install" | ||
| if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { | ||
| if !strings.Contains(branchErr.Error(), "already exists") { |
There was a problem hiding this comment.
[medium] race condition / branch staleness
When CreateBranch returns already exists from a previous failed run, the existing branch may be based on an old commit. CommitFilesToBranch commits on top of potentially stale content, causing merge conflicts or unintended diff.
Suggested fix: When the branch already exists, force-update its ref to the current default branch HEAD before committing, or document that the scaffold PR may require manual rebase.
There was a problem hiding this comment.
Fair point. In practice, the scaffold branch is created fresh from the default branch HEAD and only contains fullsend scaffold files (no user content), so staleness is low-risk — the PR diff will still be correct even if the base has advanced, and GitHub handles the merge. A force-update would require a new UpdateBranchRef forge method which is out of scope here. The idempotency guard (branchCommitted == false → skip PR creation) handles the common re-run case where files haven't changed.
| } | ||
|
|
||
| if branchCommitted { | ||
| prBody := fmt.Sprintf("This PR adds the fullsend scaffold files for per-repo installation.\n\n"+ |
There was a problem hiding this comment.
[medium] error handling / edge case
FakeClient shares CommitFilesChanged between CommitFiles and CommitFilesToBranch. The BranchUpToDate test relies on this shared control but the coupling is fragile and should be verified or separated.
Suggested fix: Consider adding a separate CommitFilesToBranchChanged field or documenting that CommitFilesChanged controls both methods.
There was a problem hiding this comment.
Accepted tradeoff. The shared CommitFilesChanged knob is intentional — both methods have the same idempotency semantics (compare content, return whether anything changed). Adding a separate field would add test API surface for no practical benefit since no test needs them to differ. The BranchUpToDate test name documents this coupling.
waynesun09
force-pushed
the
fix-protected-branch-fallback
branch
from
cc39241 to
b7c4ac9
Compare
github-actions
Bot
removed
the
ok-to-test
|
🤖 Finished Review · ✅ Success · Started 1:46 AM UTC · Completed 1:58 AM UTC |
fullsend-ai-review
Bot
added
the
requires-manual-review
|
Closes #1689 |
waynesun09
added
the
ok-to-test
waynesun09
force-pushed
the
fix-protected-branch-fallback
branch
from
b7c4ac9 to
2aa22c4
Compare
github-actions
Bot
removed
the
ok-to-test
|
🤖 Agent run interrupted (process terminated) |
When `fullsend github setup owner/repo` or `fullsend admin install owner/repo` encounters a protected default branch (422 on ref update), the CLI now falls back to creating a feature branch and opening a PR with the scaffold files instead of failing. Variables and secrets are set regardless of which path is taken. - Add ErrBranchProtected sentinel and CommitFilesToBranch to forge.Client - Refactor CommitFiles to detect 422 as branch protection failure - Add PR-based fallback in applyPerRepoScaffold Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
fix-protected-branch-fallback
branch
from
2aa22c4 to
c03bd36
Compare
waynesun09
added
the
ok-to-test
|
🤖 Finished Review · ✅ Success · Started 12:51 PM UTC · Completed 1:03 PM UTC |
| } | ||
|
|
||
| // 8. Update branch ref to point to new commit. | ||
| // 7. Update branch ref to point to new commit. |
There was a problem hiding this comment.
[medium] fail-open
All 422 responses on the ref-update PATCH are mapped to ErrBranchProtected, but GitHub returns 422 for multiple distinct conditions beyond branch protection (invalid SHA, fast-forward conflict, nonexistent ref, other validation failures). Non-protection 422 errors would silently trigger the PR-creation fallback, masking the real error.
Suggested fix: Inspect the 422 response body (APIError.Message or APIError.Errors) for branch-protection-specific signals before wrapping as ErrBranchProtected. Only return the sentinel when the response clearly indicates branch protection.
|
|
||
| const scaffoldBranch = "fullsend/scaffold-install" | ||
| if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { | ||
| if !strings.Contains(branchErr.Error(), "already exists") { |
There was a problem hiding this comment.
[low] race condition / branch staleness
When CreateBranch returns already exists (from a previous failed run), the existing branch may be based on an old commit of the default branch. CommitFilesToBranch commits on top of potentially stale content.
Suggested fix: When the scaffold branch already exists, delete and recreate it from the current default branch HEAD.
| if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { | ||
| if !strings.Contains(branchErr.Error(), "already exists") { | ||
| printer.StepFail("Failed to create scaffold branch") | ||
| return fmt.Errorf("creating scaffold branch: %w", branchErr) |
There was a problem hiding this comment.
[low] error-handling-idiom
Error checking uses strings.Contains(err.Error(), already exists) for both CreateBranch and CreateChangeProposal errors instead of typed error comparison with sentinel errors.
Suggested fix: Define forge.ErrAlreadyExists sentinel and use errors.Is() checks.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
added
ok-to-test
2 participants
Fixes #1689
Summary
When
fullsend admin installorfullsend github setupencounters a protected default branch (422 on ref update), the CLI now falls back to creating a feature branch (fullsend/scaffold-install) and opening a PR with the scaffold files instead of failing with an opaque error.Covers both install paths:
fullsend github setup owner/repo,fullsend admin install owner/repo) — fallback inapplyPerRepoScaffoldfullsend admin install org) — fallback inWorkflowsLayer.Install()for the.fullsendconfig repoChanges:
ErrBranchProtectedsentinel error andCommitFilesToBranchtoforge.ClientinterfaceCommitFilesin the GitHub client to detect 422 at the ref-update step as a branch protection failureTest plan
go test ./internal/forge/... ./internal/cli/... ./internal/layers/... -count=1— all pass