fix(#2489): retry scaffold push with force on non-fast-forward

[rh-hemartin]

Fixes #2489

Summary

Scaffold pushes now retry with force: true when hitting "Update is not a fast forward" errors. Resolves race where GitHub's async auto_init materializes a commit between ref fetch and ref update.

Root cause

CreateRepo sets auto_init: true (line 377). GitHub creates initial commit async — can complete after commitFilesTo fetches current ref SHA (line 658) but before updating ref (line 795). Scaffold commit built from stale parent → non-fast-forward rejection.

Fix

• Detect 422 non-fast-forward via isNonFastForwardError helper
• Retry ref update once with force: true
• Safe: repo just created, we own the only commit
• Separate from branch protection errors (still rejected as before)

Testing

• New test TestCommitFiles_NonFastForwardRetry verifies retry logic
• All existing CommitFiles tests pass
• Full internal/forge/github + internal/layers test suites pass

Impact

Should eliminate most common e2e flake — appeared in nearly every failed run. TestVendorFromSubdirectory and TestAdminInstallUninstall hit this repeatedly.

[qodo-code-review]

PR Summary by Qodo

Retry scaffold ref update with force on GitHub non-fast-forward (422)
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Description

• Detect GitHub 422 non-fast-forward ref update failures during scaffold pushes.
• Retry the ref update once with force: true to handle auto_init race conditions.
• Add a focused test asserting the force retry behavior and attempt count.

Diagram

graph TD
A["Scaffold: CommitFiles"] --> B["Fetch ref SHA"] --> C["Create tree/commit"] --> D["PATCH ref"] --> E{"422 error?"}
E -->|"No"| F["Success"]
E -->|"Non-fast-forward"| G["Retry PATCH (force)"] --> F
E -->|"Branch protection"| H["Return ErrBranchProtected"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Refetch head + rebuild commit (no force)

• ➕ Avoids force-pushing even in newly created repos
• ➕ Ensures the new commit is a true descendant of the latest head
• ➖ More API calls and complexity (recompute tree/commit parent) for a rare race
• ➖ Harder to keep behavior deterministic under concurrent initialization

2. Wait/poll for auto_init completion before scaffolding

• ➕ Eliminates the race by ensuring the initial commit exists before ref fetch/update
• ➕ Keeps ref update semantics strictly fast-forward
• ➖ Adds latency and polling logic to repo creation/scaffold flows
• ➖ Still needs a timeout/fallback strategy if initialization is delayed

3. Disable auto_init for scaffold-created repos

• ➕ Removes the underlying source of the asynchronous initial commit race
• ➕ Simplifies initial push semantics
• ➖ May change expected repo UX/behavior (no default README/.gitignore/license)
• ➖ Potentially impacts other flows relying on auto-init defaults

Recommendation: The chosen approach (detect non-fast-forward 422s and retry once with force) is the best tradeoff for reliability vs complexity in the scaffold context. It narrowly targets the known auto_init race, preserves branch-protection failures, and keeps runtime overhead minimal compared to refetch/rebuild or polling solutions.

Files changed (2) +70 / -3

Bug fix (1) +24 / -3

github.goRetry Git ref update with force on non-fast-forward 422 +24/-3

Retry Git ref update with force on non-fast-forward 422

• Extends the branch ref update step to distinguish 422 causes: branch protection remains an immediate failure, while non-fast-forward errors trigger a single retry with 'force: true'. Adds an 'isNonFastForwardError' helper to detect the condition from GitHub error messages.

internal/forge/github/github.go

Tests (1) +46 / -0

github_test.goAdd test covering non-fast-forward force retry on PATCH ref +46/-0

Add test covering non-fast-forward force retry on PATCH ref

• Introduces 'TestCommitFiles_NonFastForwardRetry', using an httptest server to simulate an initial 422 non-fast-forward on ref update followed by a successful forced retry. Asserts the first PATCH omits 'force', the second includes it, and exactly one retry occurs.

internal/forge/github/github_test.go

[github-actions]

Site preview

Preview: https://35a7b833-site.fullsend-ai.workers.dev

Commit: e17e485df45f99f7bcf7363c6cd549cd2953d38e

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 e2e: TestVendorFromSubdirectory flaky — 422 non-fast-forward on scaffold push

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Unscoped force ref retry 📎 Requirement gap ☼ Reliability

Description

commitFilesTo retries a 422 non-fast-forward PATCH to update a git ref by setting force: true
without verifying it is operating on a brand-new scaffold repo/branch, which can overwrite
legitimate concurrent commits in existing repos. Because commitFilesTo is shared by
CommitFiles/CommitFilesToBranch and used by non-scaffold workflows (e.g., vendoring and config
repo updates), this turns a real concurrency signal into a destructive force-update across multiple
call sites.

Code

internal/forge/github/github.go[R802-811]

+			// Non-fast-forward from auto_init race — retry with force.
+			// Safe for freshly-created repos where we own the only commit.
+			if isNonFastForwardError(apiErr) {
+				refPayload["force"] = true
+				retryResp, retryErr := c.patch(ctx, fmt.Sprintf("/repos/%s/%s/git/refs/heads/%s", owner, repo, branch), refPayload)
+				if retryErr != nil {
+					return false, fmt.Errorf("update ref (force): %w", retryErr)
+				}
+				retryResp.Body.Close()
+				return true, nil

Relevance

⭐⭐⭐ High

Team often tightens/limits 422 retry/classification logic to avoid unsafe broad behavior (PRs #816,
#2201).

PR-#816
PR-#2201

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited code changes implement a retry path in commitFilesTo that detects a 422 non-fast-forward
response from PATCH /git/refs/... and then sets refPayload["force"] = true before retrying the
ref update. Since commitFilesTo is the common implementation behind both CommitFiles and
CommitFilesToBranch and is invoked from non-scaffold flows (e.g., vendoring and other repo update
paths), the force retry is not constrained to the brand-new scaffold push scenario required for
safety; therefore, in normal concurrent-update situations the retry can forcibly move the branch ref
and clobber unrelated commits.

Ensure ref update payload/strategy is explicitly safe for brand-new repo scaffold pushes
internal/forge/github/github.go[650-812]
internal/cli/vendor.go[200-213]
internal/forge/github/github.go[641-653]
internal/forge/github/github.go[790-812]
internal/cli/vendor.go[200-214]
internal/layers/harnesswrappers.go[129-140]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`commitFilesTo` currently retries any 422 non-fast-forward ref update by setting `force: true`, but this behavior is only potentially safe in the narrow “brand-new repo scaffold push” context (if at all) and is unsafe when `CommitFiles`/`CommitFilesToBranch` are used on existing repositories. As implemented, a legitimate concurrency/non-fast-forward signal in non-scaffold workflows (e.g., vendoring and config repo updates) can be “resolved” by forcibly moving the branch ref, overwriting other commits.

## Issue Context
- Compliance requires that any non-fast-forward mitigation be explicitly safe and constrained to the brand-new scaffold pathway, not applied broadly.
- `commitFilesTo` is the shared implementation behind `CommitFiles` and `CommitFilesToBranch`, and it is called from multiple non-scaffold code paths (e.g., vendoring, config repo updates).
- The current retry behavior turns a non-fast-forward (often caused by normal concurrent updates) into a destructive force update.
- Fix should either (a) avoid force by rebuilding the commit on top of the latest ref, or (b) strictly gate force behind an explicit option/parameter enabled only for the known fresh-repo scaffold path.

## Fix Focus Areas
- internal/forge/github/github.go[641-653]
- internal/forge/github/github.go[650-812]
- internal/forge/github/github.go[790-812]
- internal/cli/vendor.go[200-214]
- internal/layers/harnesswrappers.go[129-140]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Test missing default handler 🐞 Bug ⚙ Maintainability

Description

TestCommitFiles_NonFastForwardRetry’s httptest handler has no default case, so unexpected requests
can silently return an implicit 200/empty body and fail later with less actionable errors. Other
tests in this file fail fast on unexpected requests, improving regression detection.

Code

internal/forge/github/github_test.go[R1808-1840]

+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == "GET" && r.URL.Path == "/repos/org/repo":
+			json.NewEncoder(w).Encode(map[string]string{"default_branch": "main"})
+		case r.Method == "GET" && r.URL.Path == "/repos/org/repo/git/ref/heads/main":
+			json.NewEncoder(w).Encode(map[string]any{"object": map[string]string{"sha": "commit"}})
+		case r.Method == "GET" && r.URL.Path == "/repos/org/repo/git/commits/commit":
+			json.NewEncoder(w).Encode(map[string]any{"tree": map[string]string{"sha": "tree"}})
+		case r.Method == "GET" && strings.HasPrefix(r.URL.Path, "/repos/org/repo/git/trees/tree"):
+			json.NewEncoder(w).Encode(map[string]any{"tree": []any{}, "truncated": false})
+		case r.Method == "POST" && r.URL.Path == "/repos/org/repo/git/trees":
+			json.NewEncoder(w).Encode(map[string]string{"sha": "newtree"})
+		case r.Method == "POST" && r.URL.Path == "/repos/org/repo/git/commits":
+			json.NewEncoder(w).Encode(map[string]string{"sha": "newcommit"})
+		case r.Method == "PATCH" && r.URL.Path == "/repos/org/repo/git/refs/heads/main":
+			patchCount++
+			var body map[string]any
+			require.NoError(t, json.NewDecoder(r.Body).Decode(&body))
+
+			if patchCount == 1 {
+				// First attempt: non-fast-forward
+				assert.Nil(t, body["force"], "first attempt should not force")
+				w.WriteHeader(http.StatusUnprocessableEntity)
+				json.NewEncoder(w).Encode(map[string]any{
+					"message": "Update is not a fast forward",
+				})
+			} else {
+				// Second attempt: with force
+				assert.Equal(t, true, body["force"], "retry should force")
+				w.WriteHeader(http.StatusOK)
+			}
+		}
+	}))

Relevance

⭐⭐ Medium

No direct precedent; similar “make tests stricter” suggestions were rejected as unnecessary in
github_test.go (PR #2342).

PR-#2342

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test’s server handler only handles known endpoints and has no default error path, while
nearby tests demonstrate the preferred fail-fast pattern with a default case.

internal/forge/github/github_test.go[1806-1840]
internal/forge/github/github_test.go[1738-1778]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The httptest server in `TestCommitFiles_NonFastForwardRetry` does not fail on unexpected requests, reducing test clarity/robustness.

### Issue Context
Other tests in `github_test.go` include a `default:` branch that `t.Errorf/t.Fatalf` and returns 404 for unhandled endpoints.

### Fix Focus Areas
- internal/forge/github/github_test.go[1806-1840]
- internal/forge/github/github_test.go[1738-1778]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:45 AM UTC · Completed 9:59 AM UTC
Commit: de388e8 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/forge/github/github.go	75.00%	3 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [fail-open] internal/forge/github/github.go:983 — isNonFastForwardError matches on substrings "not a fast forward" and "not a fast-forward". A false positive on an unrelated 422 error message containing these words would trigger an unintended force push. The risk is mitigated by the status-code gate (only 422s reach this check), the prior isBranchProtectionError filter, and the allowForce flag (only CommitFilesToBranch enables it). Consistent with existing string-matching patterns (isBranchProtectionError, isAlreadyExistsError), but the stakes are higher here because the consequence is a force push.

• [intent-scope-mismatch] internal/forge/github/github.go:638 — CommitFiles passes allowForce=false, so it does not benefit from the force-retry logic. The root cause (auto_init race) could theoretically affect CommitFiles when called shortly after CreateRepo. In practice the race primarily manifests on the CommitFilesToBranch scaffold path (the e2e failure site), but consider documenting why the default-branch path is not affected or extending the fix if it is.

• [solution-selection-justification] internal/forge/github/github.go:805 — Issue e2e: TestVendorFromSubdirectory flaky — 422 non-fast-forward on scaffold push #2489 identified option 2 (auto_init:false) as "likely cleanest," but this PR implements option 1 (force:true retry). Both approaches are defensible — option 1 handles the race reactively while option 2 avoids it entirely. A brief code comment or PR note explaining why option 1 was chosen over option 2 would help future maintainers.

Previous run

Review

Findings

Medium

• [logic-error] internal/forge/github/github.go:804 — The force-push retry in commitFilesTo applies unconditionally to all callers: CommitFiles (used by vendor.go for vendoring binaries and harnesswrappers.go for wrapper commits) and CommitFilesToBranch (used for scaffold branch fallback). The comment says "Safe for freshly-created repos where we own the only commit," but no guard verifies this condition. While all current callers are fullsend-owned automation paths (narrowing the blast radius compared to general collaborative editing), a concurrent fullsend run on the same repo hitting a legitimate non-fast-forward could still result in a silent force-push discarding work.
  Remediation: Gate the force-push retry on a caller-provided flag (e.g., allowForceOnNonFF bool) from the caller that knows the repo was just created, or verify the branch has at most one commit before force-pushing.

• [error-handling] internal/forge/github/github.go:808 — If the force-push retry itself returns a 422 with a branch-protection message, the error is wrapped as update ref (force): <err> without checking isBranchProtectionError. The caller in layers/commit.go:20 checks forge.IsBranchProtected(err) to decide whether to fall back to a scaffold PR branch. Because the retry error does not wrap forge.ErrBranchProtected, the branch-protection fallback path is unreachable on the forced attempt.
  Remediation: After the force-push retry fails, check isBranchProtectionError on the retry error and wrap with forge.ErrBranchProtected if applicable.

Low

• [test-inadequate] internal/forge/github/github_test.go:1805 — The test only covers the successful retry path. There is no test for the case where the force-push retry itself fails (e.g., returns a 422 branch-protection error on the forced attempt).
• [fail-open] internal/forge/github/github.go:972 — isNonFastForwardError matches on substring "fast forward" or "fast-forward". A false positive on an unrelated error could trigger unintended force push. Consistent with existing patterns (isBranchProtectionError) but stakes are higher due to force-push consequence.
• [intent-mismatch] internal/forge/github/github.go:805 — The PR implements solution option 1 (force:true on ref update) but issue e2e: TestVendorFromSubdirectory flaky — 422 non-fast-forward on scaffold push #2489 identified option 2 (auto_init:false) as "likely cleanest." The implementation diverges from the issue's suggested direction without explicit justification.
• [architectural-conflict] internal/forge/github/github.go:805 — The retry logic constructs a new PATCH request rather than wrapping in retryOnRepoRace. This is actually correct since retryOnRepoRace only retries on 404/409 (not 422), but a brief code comment explaining why would address the apparent inconsistency.

---

Labels: PR fixes a bug in the forge/github install scaffold flow, directly addressing e2e test flakiness

Previous run

Review

Findings

Medium

• [logic-error] internal/forge/github/github.go:804 — The force-push retry in commitFilesTo applies unconditionally to all callers: CommitFiles (used by vendor.go for vendoring binaries and harnesswrappers.go for wrapper commits) and CommitFilesToBranch (used for scaffold branch fallback). The comment says "Safe for freshly-created repos where we own the only commit," but no guard verifies this condition. While all current callers are fullsend-owned automation paths (narrowing the blast radius compared to general collaborative editing), a concurrent fullsend run on the same repo hitting a legitimate non-fast-forward could still result in a silent force-push discarding work.
  Remediation: Gate the force-push retry on a caller-provided flag (e.g., allowForceOnNonFF bool) from the caller that knows the repo was just created, or verify the branch has at most one commit before force-pushing.

• [error-handling] internal/forge/github/github.go:808 — If the force-push retry itself returns a 422 with a branch-protection message, the error is wrapped as update ref (force): <err> without checking isBranchProtectionError. The caller in layers/commit.go:20 checks forge.IsBranchProtected(err) to decide whether to fall back to a scaffold PR branch. Because the retry error does not wrap forge.ErrBranchProtected, the branch-protection fallback path is unreachable on the forced attempt.
  Remediation: After the force-push retry fails, check isBranchProtectionError on the retry error and wrap with forge.ErrBranchProtected if applicable.

Low

• [test-inadequate] internal/forge/github/github_test.go:1805 — The test only covers the successful retry path. There is no test for the case where the force-push retry itself fails (e.g., returns a 422 branch-protection error on the forced attempt).
• [fail-open] internal/forge/github/github.go:972 — isNonFastForwardError matches on substring "fast forward" or "fast-forward". A false positive on an unrelated error could trigger unintended force push. Consistent with existing patterns (isBranchProtectionError) but stakes are higher due to force-push consequence.
• [intent-mismatch] internal/forge/github/github.go:805 — The PR implements solution option 1 (force:true on ref update) but issue e2e: TestVendorFromSubdirectory flaky — 422 non-fast-forward on scaffold push #2489 identified option 2 (auto_init:false) as "likely cleanest." The implementation diverges from the issue's suggested direction without explicit justification.
• [architectural-conflict] internal/forge/github/github.go:805 — The retry logic constructs a new PATCH request rather than wrapping in retryOnRepoRace. This is actually correct since retryOnRepoRace only retries on 404/409 (not 422), but a brief code comment explaining why would address the apparent inconsistency.

Previous run (2)

Review

Findings

High

• [logic-error] internal/forge/github/github.go:804 — The force-push retry in commitFilesTo applies unconditionally to all callers: CommitFiles (used by vendor.go for vendoring binaries and harnesswrappers.go for wrapper commits) and CommitFilesToBranch (used for scaffold branch fallback). The comment says "Safe for freshly-created repos where we own the only commit," but no guard verifies this condition. On an established repository with concurrent pushers, a legitimate non-fast-forward rejection would be silently resolved by force-pushing, discarding another developer's commit.
  Remediation: Either (a) scope the force-push retry to only the scaffold code path by passing an option/flag (e.g., allowForceOnNonFF bool) from the caller that knows the repo was just created, or (b) instead of force-pushing, re-fetch the ref and rebuild the commit on top of the new parent.

Medium

• [error-handling] internal/forge/github/github.go:808 — If the force-push retry itself returns a 422 with a branch protection message, the error is wrapped as update ref (force): <err> without checking isBranchProtectionError. The caller (e.g., layers/commit.go:38 which checks forge.IsBranchProtected) loses the semantic signal, causing misdiagnosis of the failure.
  Remediation: After the force-push retry fails, check isBranchProtectionError on the retry error and wrap with forge.ErrBranchProtected if applicable.

Low

• [test-inadequate] internal/forge/github/github_test.go:1805 — The test only covers the successful retry path. There is no test for the case where the force-push retry itself fails (e.g., returns a 422 branch-protection error on the forced attempt).
• [fail-open] internal/forge/github/github.go:972 — isNonFastForwardError matches on substring "fast forward" or "fast-forward". A false positive on an unrelated error could trigger unintended force push. Consistent with existing patterns (isBranchProtectionError) but stakes are higher due to force-push consequence.
• [architectural-conflict] internal/forge/github/github.go:814 — The retry logic constructs a new PATCH request rather than wrapping in retryOnRepoRace. This is actually correct since retryOnRepoRace only retries on 404/409 (not 422), but a brief code comment explaining why would address the apparent inconsistency.
• [pattern-inconsistency] internal/forge/github/github.go:792 — Changed refPayload from map[string]string to map[string]any. This is correct and necessary since the payload now contains a bool (force), but breaks the established pattern for string-only payloads.
• [code-organization] internal/forge/github/github.go:798 — Error handling now has three levels of nesting. An early return after the branch protection check could flatten this.

---

Labels: PR fixes a bug in the forge/github install scaffold flow, directly addressing e2e test flakiness

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:14 AM UTC · Completed 11:27 AM UTC
Commit: a2348de · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:34 AM UTC · Completed 11:47 AM UTC
Commit: e17e485 · View workflow run →

[fullsend-ai-review]

[low] fail-open

isNonFastForwardError matches on substrings 'not a fast forward' and 'not a fast-forward'. A false positive on an unrelated 422 error message containing these words would trigger an unintended force push. Mitigated by the status-code gate, the isBranchProtectionError pre-filter, and the allowForce flag. Consistent with existing patterns but higher stakes due to force-push consequence.

[fullsend-ai-review]

[low] intent-scope-mismatch

CommitFiles passes allowForce=false, so it does not benefit from the force-retry logic. The auto_init race could theoretically affect CommitFiles when called shortly after CreateRepo. Consider documenting why the default-branch path is not affected or extending the fix.

[fullsend-ai-review]

[low] solution-selection-justification

Issue #2489 identified option 2 (auto_init:false) as 'likely cleanest' but this PR implements option 1 (force:true retry). Both approaches are defensible. A brief code comment explaining why option 1 was chosen would help future maintainers.

[ralphbean]

I think this needs a change before we can merge. See inline.

[ralphbean]

[high] I traced the e2e error from #2489 through the call chain — I think the retry lands on a path that doesn't get hit.

CommitScaffoldFiles calls CommitFiles first (commit.go:19), which passes allowForce=false here. A non-fast-forward 422 isn't a branch-protection error, so commit.go:20 doesn't fall through to CommitFilesToBranch — it returns the error directly at commit.go:68.

CommitFilesToBranch(allowForce=true) only runs when the default branch is protected, but a freshly created repo's default branch never is.

Could be worth considering option 2 from the issue (auto_init: false) — eliminates the race instead of retrying through it.

[waynesun09]

Multi-Agent Review — 6 agents (Claude, Gemini, Codex)

5 of 6 agents independently confirmed @ralphbean's inline finding: the force-retry path is unreachable in the actual failing scaffold flow. Three additional unique findings below (HIGH, 2x MEDIUM) that are not covered by existing review comments.

Assisted-by: Claude (review), Gemini (review), Codex (review)

[waynesun09]

3 new findings (1 HIGH, 2 MEDIUM) from multi-agent review — unique issues not covered by existing comments. See inline.

Assisted-by: Claude (review), Gemini (review), Codex (review)

[waynesun09]

[high] stale-parent-on-force

The force retry pushes newCommit.SHA — the same commit object built in step 6 with the stale commitSHA (fetched in step 1) as its parent. If auto_init created a commit between step 1 and step 7, the force-pushed commit's parent chain doesn't include the auto_init commit, leaving it orphaned in the object store. The scaffold commit history won't properly descend from the initial commit.

Secondary to the CRITICAL (this path is currently unreachable), but if force-retry is kept: re-fetch the current ref SHA and recreate the commit object with the correct parent before pushing. Moot if auto_init: false is adopted.

[waynesun09]

[medium] wrong-path-tested

These tests exercise CommitFilesToBranch (allowForce=true), but the actual e2e failure goes through CommitFiles (allowForce=false). TestCommitFiles_NonFastForwardNoRetry below confirms CommitFiles does NOT retry — effectively documenting the bug rather than validating the fix.

No test exercises the full CommitScaffoldFiles flow to show the non-fast-forward is handled. Consider adding an integration test in internal/layers/ that calls CommitScaffoldFiles with a mock client returning a non-fast-forward 422 from CommitFiles.

[waynesun09]

[medium] stale-comment

The comment above (line 792) says "A 422 here typically means branch protection rules prevent the push" but the code below now dispatches on three distinct 422 variants: non-422 errors (line 801), branch protection (line 804), and non-fast-forward (line 811). The comment should reflect the expanded handling.