Skip to content

fix(github): ensure local branch exists for fork PR checkout#1454

Merged
arnestrickmann merged 2 commits intogeneralaction:mainfrom
jschwxrz:emdash/feat-allow-pr-review-on-forks-apv
Mar 13, 2026
Merged

fix(github): ensure local branch exists for fork PR checkout#1454
arnestrickmann merged 2 commits intogeneralaction:mainfrom
jschwxrz:emdash/feat-allow-pr-review-on-forks-apv

Conversation

@jschwxrz
Copy link
Collaborator

summary:

  • fix PR review worktree setup for forked PRs when direct fallback is needed
  • remove --detach from fallback gh pr checkout
  • after checkout explicitly verify refs/heads/ exists
  • If missing create the local branch ref from HEAD

this ensures downstream worktree creation can reliably use the expected local branch for fork PR review flows.

@vercel
Copy link

vercel bot commented Mar 13, 2026

@jschwxrz is attempting to deploy a commit to the General Action Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR fixes fork PR checkout in the fallback path of ensurePullRequestBranch by removing --detach from the gh pr checkout command and adding an explicit local-branch-ref guard. After the checkout, git show-ref --verify confirms that refs/heads/<branch> exists; if it does not (a known gh/fork edge case), git branch --force <branch> HEAD creates it so that the downstream git worktree add call in WorktreeService.createWorktreeFromBranch can reliably reference the branch by name.

  • The core logic is correct: the verification + fallback creation runs inside the outer try block, so any failure in git branch --force still propagates to the outer catch and is rethrown.
  • The finally block correctly restores the previous HEAD ref regardless of outcome, unchanged from before.
  • A stale comment on line 900 still says "with detach to avoid working tree conflicts", which is the opposite of the new behaviour — it should be updated.
  • The error logged in the catch block at line 934 ("Failed to checkout pull request branch via gh:") can now be emitted for failures originating from git branch --force, which may make debugging harder; a more neutral message would improve observability.

Confidence Score: 4/5

  • Safe to merge — the fix correctly addresses the missing local branch ref edge case for fork PRs with no logic regressions.
  • The change is narrow and well-targeted: it removes --detach and adds a post-checkout guard that creates the local branch ref when gh pr checkout leaves HEAD detached. The finally block that restores the previous branch is unaffected. The only issues are a stale comment and a slightly misleading error log message — neither affects runtime correctness.
  • No files require special attention beyond the stale comment on line 900 of src/main/services/GitHubService.ts.

Important Files Changed

Filename Overview
src/main/services/GitHubService.ts Removes --detach from gh pr checkout fallback and adds a post-checkout guard that creates a local branch ref via git branch --force if git show-ref shows the ref is missing. Logic is sound; one stale comment and a slightly misleading error message remain.

Sequence Diagram

sequenceDiagram
    participant Caller as githubIpc.ts
    participant GHS as GitHubService
    participant Git as git (exec)
    participant GH as gh CLI

    Caller->>GHS: ensurePullRequestBranch(path, prNumber, branchName)

    GHS->>Git: git fetch origin refs/pull/N/head:refs/heads/<branch> --force
    alt fetch succeeds
        Git-->>GHS: ok
    else fetch fails (fork PR / no direct access)
        Git-->>GHS: error
        GHS->>Git: git rev-parse --abbrev-ref HEAD
        Git-->>GHS: previousRef

        GHS->>GH: gh pr checkout <N> --branch <branch> --force
        GH-->>GHS: ok (working tree updated)

        GHS->>Git: git show-ref --verify --quiet refs/heads/<branch>
        alt ref exists
            Git-->>GHS: exit 0
        else ref missing (gh/fork edge case)
            Git-->>GHS: exit 1
            GHS->>Git: git branch --force <branch> HEAD
            Git-->>GHS: branch created
        end

        Note over GHS,Git: finally block
        GHS->>Git: git checkout <previousRef> --force
        Git-->>GHS: working tree restored
    end

    GHS-->>Caller: safeBranch (string)

    Caller->>GHS: WorktreeService.createWorktreeFromBranch(path, ..., branchName)
    GHS->>Git: git worktree add <worktreePath> <branchName>
    Git-->>GHS: worktree created
Loading

Comments Outside Diff (1)

  1. src/main/services/GitHubService.ts, line 900 (link)

    Stale comment still references --detach

    The comment on line 900 still says "try gh pr checkout with detach to avoid working tree conflicts", but this PR removes --detach from the command below. The comment now describes the opposite of what the code does — gh pr checkout without --detach explicitly will touch the working tree. The comment should be updated to reflect the new intent.

Last reviewed commit: 60cfd4a

@arnestrickmann
Copy link
Contributor

Thanks! good catch

@arnestrickmann arnestrickmann merged commit 8f33553 into generalaction:main Mar 13, 2026
2 of 3 checks passed
@jschwxrz jschwxrz deleted the emdash/feat-allow-pr-review-on-forks-apv branch March 13, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants