fix(cli): fall back to PR when default branch is protected#2201
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://24e8c353-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 |
ReviewFindingsLow
Info
Previous runReviewFindingsLow
Info
Previous run (2)ReviewFindingsLow
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Low
Info
Previous run (5)ReviewFindingsMedium
Low
Info
Previous run (6)ReviewFindingsMedium
Low
Info
Previous runReviewFindingsLow
Info
Previous run (2)ReviewFindingsLow
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Low
Info
Previous run (5)ReviewFindingsMedium
Low
Info
Previous run (6)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) |
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
removed
the
ok-to-test
|
🤖 Finished Review · ✅ Success · Started 7:36 PM UTC · Completed 7:48 PM UTC |
| if e.StatusCode == http.StatusNotFound { | ||
| return forge.ErrNotFound | ||
| } | ||
| if e.StatusCode == http.StatusUnprocessableEntity && isAlreadyExistsError(e) { |
There was a problem hiding this comment.
[low] sentinel collision
APIError.Unwrap() now returns forge.ErrAlreadyExists for every 422 whose message contains 'already exists'. Future code branching on IsAlreadyExists for unrelated API calls may get unexpected matches.
| const scaffoldBranch = "fullsend/scaffold-install" | ||
| if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { | ||
| if !forge.IsAlreadyExists(branchErr) { | ||
| printer.StepFail("Failed to create scaffold branch") |
There was a problem hiding this comment.
[low] race condition / branch staleness
When CreateBranch returns ErrAlreadyExists, the existing branch may be based on an old commit. Practical impact is limited since scaffold files are idempotent boilerplate and the PR requires human review.
fullsend-ai-review
Bot
added
ready-for-merge
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. Two minor notes inline.
|
|
||
| // isBranchProtectionError checks whether a 422 APIError indicates branch | ||
| // protection rather than another validation failure (e.g. non-fast-forward). | ||
| func isBranchProtectionError(apiErr *APIError) bool { |
There was a problem hiding this comment.
[moderate, non-blocking] isBranchProtectionError and isAlreadyExistsError don't have direct unit tests — the tests in admin_test.go and workflows_test.go exercise FakeClient with pre-wrapped sentinels, so they never validate that a real GitHub API error gets classified correctly.
Table-driven tests with realistic payloads (including negative cases like non-fast-forward 422s) would make these heuristics easier to maintain. Something like:
func TestIsBranchProtectionError(t *testing.T) {
tests := []struct{ name string; apiErr *APIError; want bool }{
{"protection", &APIError{StatusCode: 422, Message: "protected branch"}, true},
{"non-ff", &APIError{StatusCode: 422, Message: "Update is not a fast forward"}, false},
}
// ...
}| if e.StatusCode == http.StatusNotFound { | ||
| return forge.ErrNotFound | ||
| } | ||
| if e.StatusCode == http.StatusUnprocessableEntity && isAlreadyExistsError(e) { |
There was a problem hiding this comment.
[minor, non-blocking] isAlreadyExistsError is wired into Unwrap() globally — any 422 from any endpoint whose message contains "already exists" now unwraps to ErrAlreadyExists. Contrast with ErrBranchProtected, which is scoped to the commitFilesTo call site. That asymmetry is correct for ErrBranchProtected since it needs context, but worth a brief doc comment here noting the design choice so future readers don't wonder about it.
No existing callers check for ErrAlreadyExists, so no immediate behavioral change. If a future endpoint returns a 422 with "already exists" for a different reason (e.g. SHA conflict), the consequence is graceful degradation rather than a hard failure.
Test isBranchProtectionError, isAlreadyExistsError, and APIError.Unwrap against realistic GitHub API payloads including protected branch push rejections, required status checks, required reviews, reference already-exists, PR already-exists (custom code), non-fast-forward (negative case), and unrelated validation errors. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:37 PM UTC · Completed 8:48 PM UTC |
| if e.StatusCode == http.StatusNotFound { | ||
| return forge.ErrNotFound | ||
| } | ||
| if e.StatusCode == http.StatusUnprocessableEntity && isAlreadyExistsError(e) { |
There was a problem hiding this comment.
[low] sentinel collision
APIError.Unwrap() now returns forge.ErrAlreadyExists for every 422 whose message contains "already exists". This follows the existing pattern where all 404s become forge.ErrNotFound, but future code branching on IsAlreadyExists for unrelated API calls may get unexpected matches.
| } | ||
| refUpdateResp.Body.Close() | ||
|
|
||
| return true, nil |
There was a problem hiding this comment.
[low] fail-open
isBranchProtectionError uses substring matching on GitHub API error messages to detect branch protection 422 errors. A non-protection 422 error whose message contains these substrings could be misclassified, triggering the PR-creation fallback. The fallback path is safe (creates a PR requiring human review).
| const scaffoldBranch = "fullsend/scaffold-install" | ||
| if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { | ||
| if !forge.IsAlreadyExists(branchErr) { | ||
| printer.StepFail("Failed to create scaffold branch") |
There was a problem hiding this comment.
[low] race condition / branch staleness
When CreateBranch returns ErrAlreadyExists (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. Practical impact is limited since scaffold files are idempotent boilerplate and the PR requires human review.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Review · |
- Add "rule violation" to isBranchProtectionError for GitHub rulesets - Always attempt PR creation in CommitScaffoldFiles for crash recovery - Add comment explaining intentional Unwrap asymmetry for ErrBranchProtected - Correct ADR-0005 sentinel error wrapping description - Add ruleset violation test case and scaffold-branch-also-protected tests - Fix BranchUpToDate tests to inject ErrAlreadyExists for re-run scenario Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
fix-protected-branch-fallback
branch
from
65a9e24 to
b4c2edb
Compare
|
🤖 Finished Review · ✅ Success · Started 9:04 PM UTC · Completed 9:20 PM UTC |
waynesun09
added
the
ok-to-test
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Retro · ✅ Success · Started 11:37 PM UTC · Completed 11:47 PM UTC |
Retro Analysis: PR #2201 — Protected Branch FallbackTimeline
AssessmentWhat went well:
Improvement opportunities identified (all covered by existing issues):
ConclusionNo new proposals filed — all identified improvement areas are well-covered by existing open issues. The core themes (repeated findings, dispatch deduplication, author response awareness) have 20+ open issues between them. The most impactful existing issues to prioritize for reducing the noise seen on this PR are #1271 (prevent review self-triggering loop), #1552 (pass prior findings to follow-up reviews), and #1672 (honor author dismissals). |
3 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