fix(#2592): default to fork-based scaffold PR for non-owner users - #2594
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
|
🤖 Finished Review · ✅ Success · Started 7:51 AM UTC · Completed 8:06 AM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Labels: PR modifies scaffold install flow in internal/layers/commit.go and forge abstraction in internal/forge/ Previous run (2)ReviewFindingsMedium
Labels: PR modifies scaffold install flow in internal/layers/commit.go and forge abstraction in internal/forge/ Previous run (3)Looks good to me Previous run (4)ReviewFindingsMedium
Labels: PR modifies scaffold install flow in internal/layers/commit.go and forge abstraction in internal/forge/ |
| if e.StatusCode == http.StatusNotFound { | ||
| return forge.ErrNotFound | ||
| } | ||
| if e.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
[medium] logic-error
Mapping all HTTP 403 responses to forge.ErrForbidden in APIError.Unwrap() is overly broad. GitHub returns 403 for secondary rate limits, SAML SSO enforcement, and other policy-based denials — not only permission failures. Any of these would incorrectly trigger the fork-and-push fallback path in commitScaffoldViaPR. The existing Unwrap() comment (lines 75-82) explicitly explains why ErrBranchProtected is NOT mapped globally because 422s are context-dependent — the same reasoning applies to 403.
Suggested fix: Only map 403 to ErrForbidden at the call site (e.g., CreateBranch) rather than globally in Unwrap(), following the established ErrBranchProtected pattern. Alternatively, inspect the 403 response message to distinguish permission denials from rate limits or SAML enforcement.
|
/fs-fix need to find out if the user already has a fork and reuse that where possible |
|
🤖 Finished Fix · ✅ Success · Started 8:38 AM UTC · Completed 8:50 AM UTC |
Add FindExistingFork to forge.Client interface so commitScaffoldViaPR can check whether the user already has a fork before trying (and failing) to push to upstream. When an existing fork is found, the scaffold branch is created directly on the fork, skipping the unnecessary 403 round-trip. The CreateFork fallback path remains for users who don't yet have a fork. Addresses review feedback on #2594
🔧 Fix agent — iteration 1 (human-triggered)Added FindExistingFork to forge.Client so commitScaffoldViaPR detects existing forks and reuses them, skipping the unnecessary upstream push attempt that would fail with 403. Fixed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:53 AM UTC · Completed 9:06 AM UTC |
|
/fs-fix fix the logic error spotted by fullsend-ai-review as get the coverage for new code to or above 80% |
|
🤖 Finished Fix · ✅ Success · Started 9:26 AM UTC · Completed 9:37 AM UTC |
The global APIError.Unwrap() mapping of HTTP 403 to forge.ErrForbidden was overly broad — GitHub returns 403 for secondary rate limits, SAML SSO, and other policy denials, not only permission failures. This follows the established ErrBranchProtected pattern: the sentinel is wrapped at the call site (CreateBranch) where the operation context disambiguates the cause. Also adds tests for FindExistingFork, CreateFork, CreateBranch 403 wrapping, FakeClient fork methods, IsForbidden helper, and commit.go edge cases to bring patch coverage above the 80% threshold. Addresses review feedback on #2594
🔧 Fix agent — iteration 2 (human-triggered)Fixed the logic error by scoping the 403-to-ErrForbidden mapping from the global APIError.Unwrap() to the CreateBranch call site, following the established ErrBranchProtected pattern. Added comprehensive tests for all new code paths (FindExistingFork, CreateFork, CreateBranch 403, fake client methods, IsForbidden, commit.go edge cases) to bring patch coverage above 80%. Fixed (2):
Tests: passed Decision points
Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 9:40 AM UTC · Completed 9:54 AM UTC |
| func (c *LiveClient) FindExistingFork(ctx context.Context, owner, repo string) (string, error) { | ||
| user, err := c.GetAuthenticatedUser(ctx) | ||
| if err != nil { | ||
| return "", fmt.Errorf("find existing fork: %w", err) |
There was a problem hiding this comment.
[medium] resource-leak
In FindExistingFork, when c.do() returns a response with StatusNotFound, the code returns ("", nil) without closing resp.Body. Every other call site in this file that receives a response from do() closes the body. This leaks an HTTP response body on every call where the user does not already have a fork.
Suggested fix: Add defer resp.Body.Close() immediately after the nil-error check on c.do(), before the status code checks.
| } | ||
|
|
||
| // CreateFork creates a fork of owner/repo under the authenticated user's | ||
| // account. If a fork already exists, the GitHub API returns 202 with the |
There was a problem hiding this comment.
[medium] logic-error
CreateFork only extracts the fork owner login from the GitHub API response, but GitHub may create a fork with a different repo name if the user already has an unrelated repo with the same name (e.g., 'repo' becomes 'repo-1'). The caller in commit.go uses the original repo name for all subsequent API calls (CreateBranch, CommitFilesToBranch), which would 404 if the fork name differs.
Suggested fix: Have CreateFork return both the owner login and the actual repo name from the response. Update the forge.Client interface and commitScaffoldViaPR to use the returned repo name for branchRepo.
| CreateRepo(ctx context.Context, org, name, description string, private bool) (*Repository, error) | ||
| DeleteRepo(ctx context.Context, owner, repo string) error | ||
|
|
||
| // FindExistingFork checks whether the authenticated user already |
There was a problem hiding this comment.
[medium] breaking-api
Adding FindExistingFork and CreateFork to the forge.Client interface is a breaking change for any out-of-tree implementations. Currently only FakeClient and github.LiveClient exist (both updated), but AGENTS.md documents planned GitLab/Forgejo support. Document the cross-forge contract so future implementors have clear expectations.
Suggested fix: Verify no in-progress forge implementations exist. Add doc comments specifying the behavioral contract for all forge implementations.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 1:28 PM UTC · Completed 1:39 PM UTC |
…body - FindExistingFork/CreateFork now return (forkOwner, forkRepo, err) so callers use the actual fork repo name instead of assuming it matches the upstream name (GitHub may rename forks to avoid collisions) - Close resp.Body on the 404 early-return path in FindExistingFork - Add cross-forge behavioral contract docs on new interface methods - Add tests for renamed fork scenarios - Update commitScaffoldViaPR to use returned forkRepo for branchRepo Addresses review feedback on #2594
🔧 Fix agent — iteration 3 (human-triggered)Addressed all 3 review findings: fixed resp.Body leak on 404 path in FindExistingFork, changed fork method signatures to return actual repo name (handling GitHub rename collisions), and added cross-forge behavioral contract documentation to interface methods. Added test cases for renamed fork scenarios. Fixed (3):
Tests: passed Decision points
Updated by fullsend fix agent |
Site previewPreview: https://71e0b925-site.fullsend-ai.workers.dev Commit: |
|
/fs-review |
|
I agree with Barak here, I think the sentiment that the users have is that they want to use their fork first. So I agree that the CLI needs to check if the user has a fork, then use it, even with push access to the original. If a fork is not detected we can ask users about if proceed by using the original repository, and if not tell them to create a fork. I don't think we need to create the fork from the CLI. But up to you. |
558949d to
7c60772
Compare
|
Thanks @rh-hemartin — this PR now implements fork-first as the default. Here's the decision tree:
On your point about not creating the fork from the CLI — I went with offering it as the default prompt option because The fork creation also handles GitHub's async 202 response by polling |
Add FindExistingFork and CreateFork to the forge.Client interface with GitHub LiveClient and FakeClient implementations. Scope HTTP 403 to ErrForbidden mapping to CreateBranch call site only, following the established ErrBranchProtected pattern. Add comprehensive tests for all new methods. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Refactor commitScaffoldViaPR to use a fork-first decision tree: - Owner → push branch directly, same-repo PR - Existing fork → reuse it (no prompt) - No fork, interactive → prompt: [f]ork (default) / [u]pstream - No fork, non-interactive → auto-fork - Upstream 403 → clear error suggesting fork option Fork creation polls GetRepo every 3s for up to 2min to handle GitHub's async 202 Accepted response. Cross-fork PRs use the "forkOwner:branchName" head format. Add io.Reader parameter to CommitScaffoldFiles for interactive prompts. Callers pass os.Stdin (interactive) or nil (auto-fork). Fixes: #2592 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
Update existing tests to set FakeClient.AuthenticatedUser matching the test's repo owner so the new fork-detection logic correctly identifies the user as the owner and takes the direct-push path. Without this, tests hit the fork path and timeout waiting for a non-existent fork to become ready. Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
7c60772 to
60744d6
Compare
|
🤖 Finished Retro · ❌ Failure · Started 2:09 PM UTC · Completed 2:31 PM UTC |
Summary
Fixes #2592 —
fullsend github setupnow defaults to fork-based PRs for non-owner contributors instead of pushing branches directly to the upstream repo.Decision tree for scaffold PR creation
Key changes
internal/layers/commit.go— RefactoredcommitScaffoldViaPRwith fork-first logic. Addedio.Readerparameter for interactive prompts. New helpers:commitBranchAndPR,commitViaFork,forkAndCommit,waitForFork,promptForkChoice.internal/forge/forge.go— AddedErrForbidden,IsForbidden(),FindExistingFork(),CreateFork()to theClientinterface.internal/forge/github/github.go— Implemented fork/forbidden methods onLiveClient. Scoped 403→ErrForbiddenmapping toCreateBranchonly.internal/forge/fake.go— ExtendedFakeClientwith fork simulation fields.internal/cli/admin.go— Passos.StdintoCommitScaffoldFiles(interactive).internal/layers/workflows.go— PassniltoCommitScaffoldFiles(non-interactive, auto-fork).Fork readiness
GitHub fork creation returns 202 Accepted and can take 30s+ for large repos. After
CreateFork, the code pollsGetRepoevery 3s for up to 2 minutes until the fork is materialized.Cross-fork PRs
When creating a PR from a fork, the PR head is set to
"forkOwner:branchName"so GitHub correctly identifies the source branch across repositories.Test plan
commit_test.gocovering the full decision treeAuthenticatedUserfor fork-detection compatibilitygo vet ./...cleango build ./...clean