From 09050179cd1edfa82de857125facb94123dfcd94 Mon Sep 17 00:00:00 2001 From: omartelo Date: Mon, 3 Aug 2026 10:47:34 -0300 Subject: [PATCH] fix: open a session on a fork PR that allows maintainer edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every pull request from a fork was refused a session, on the grounds that an agent's commits would have nowhere to push. GitHub asks the author that exact question when the pull request is opened, and "allow edits by maintainers" is on by default — so the blanket refusal turned a permission that was already granted into a dead button. The gate now reads maintainerCanModify alongside isCrossRepository, in both places that held the rule: CreateWorktreeFromPR, which refuses before the worktree exists, and the header button, which greys out with the reason on its tooltip. Only a fork that withholds the permission is still refused, and both messages now name which of the two it is. Verified by hand against a fork PR: gh pr checkout inside a detached worktree sets branch..remote to the fork, so a push goes back to the branch the pull request is on. --- CHANGELOG.md | 7 +++ frontend/src/components/pulls/Pulls.tsx | 7 ++- frontend/src/lib/api-types.ts | 6 +- frontend/src/lib/pulls/merge-gate.test.ts | 1 + internal/project/pr.go | 76 ++++++++++++----------- internal/project/pr_test.go | 13 ++++ internal/project/worktree.go | 22 ++++--- internal/project/worktree_test.go | 21 ++++++- 8 files changed, 103 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d516ab6ad..bfccc4e79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Open a session on a fork's pull request when its author allows it.** Every + pull request from a fork was refused a session, on the grounds that an agent's + commits would have nowhere to push — but GitHub asks the author that very + question when the pull request is opened, and "allow edits by maintainers" is + on by default. lich now reads the answer: with the permission on, the fork's + branch checks out and opens like any other, and only a fork that withholds it + still greys the button out, saying which of the two it is. - **A thread in Conversation shows the lines it is about, not the file.** GitHub sends a review comment with the whole hunk it sits in, which on a file the branch adds is the entire file — so a remark about ten lines arrived under a diff --git a/frontend/src/components/pulls/Pulls.tsx b/frontend/src/components/pulls/Pulls.tsx index 9efda5dc1..7a2eff264 100644 --- a/frontend/src/components/pulls/Pulls.tsx +++ b/frontend/src/components/pulls/Pulls.tsx @@ -198,9 +198,10 @@ export function Pulls({ list = false }: PullsProps) { checkedOut && sessionsOf(sessions, projectId ?? "").some((s) => s.path === checkedOut.path) ? "Go to session" : "Open in Session", - blocked: detail?.isCrossRepository - ? "The head branch lives on a fork — its commits could not be pushed back" - : null, + blocked: + detail?.isCrossRepository && !detail.maintainerCanModify + ? "The head branch lives on a fork that does not allow edits by maintainers — its commits could not be pushed back" + : null, busy: opening, run: () => void openInSession(), } diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 84adbe384..ff0d2d0e2 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -131,8 +131,12 @@ export interface PullRequestDetail { baseRefName: string headRefName: string changedFiles: number - /** The head branch lives on a fork: no session can be opened on it. */ + /** The head branch lives on a fork. */ isCrossRepository: boolean + /** GitHub's "allow edits by maintainers" on a fork PR: what decides whether a + * session can be opened on it. False on a same-repository PR, which needs no + * such permission. */ + maintainerCanModify: boolean checks: ChecksRollup /** The rollup itself, worst state first; null when the PR reports no checks. */ checkRuns: CheckItem[] | null diff --git a/frontend/src/lib/pulls/merge-gate.test.ts b/frontend/src/lib/pulls/merge-gate.test.ts index 1b77940a4..fb9619724 100644 --- a/frontend/src/lib/pulls/merge-gate.test.ts +++ b/frontend/src/lib/pulls/merge-gate.test.ts @@ -30,6 +30,7 @@ const detail = (over: Partial = {}): PullRequestDetail => ({ headRefName: "quiet-willow", changedFiles: 1, isCrossRepository: false, + maintainerCanModify: false, checks: { total: 0, passed: 0, failed: 0, pending: 0 }, checkRuns: null, commits: null, diff --git a/internal/project/pr.go b/internal/project/pr.go index 318cac8e0..32580e52e 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -65,7 +65,7 @@ func runGH(timeout time.Duration, dir, token string, args ...string) ([]byte, er } // prViewFields is the gh `pr view --json` selection backing the Pulls panel. -const prViewFields = "number,url,state,title,body,isDraft,mergeable,mergeStateStatus,reviewDecision,baseRefName,headRefName,isCrossRepository,statusCheckRollup,changedFiles,commits" +const prViewFields = "number,url,state,title,body,isDraft,mergeable,mergeStateStatus,reviewDecision,baseRefName,headRefName,isCrossRepository,maintainerCanModify,statusCheckRollup,changedFiles,commits" // prSelector renders the pull request argument gh's subcommands take ahead of // their flags. Zero means "the branch checked out at path" — gh's own default, @@ -117,10 +117,14 @@ type PRDetail struct { Checks ChecksRollup `json:"checks"` CheckRuns []CheckItem `json:"checkRuns"` Commits []PRCommit `json:"commits"` - // IsCrossRepository marks a head branch that lives on a fork — the one PR - // a session cannot be opened on (CreateWorktreeFromPR), so the screen can - // say so instead of letting the button fail. + // IsCrossRepository marks a head branch that lives on a fork. IsCrossRepository bool `json:"isCrossRepository"` + // MaintainerCanModify is GitHub's "allow edits by maintainers" on a fork PR, + // and the whole of what decides whether a session can be opened on one: with + // it a push back to the fork's branch is authorised, without it there is + // nowhere for an agent's commits to go (CreateWorktreeFromPR). It is false on + // a same-repository PR, where nothing needs the permission. + MaintainerCanModify bool `json:"maintainerCanModify"` } // PRCommit is one commit the pull request would land, split the way git itself @@ -136,21 +140,22 @@ type PRCommit struct { // ghPRView mirrors the requested gh payload; statusCheckRollup is reduced to // ChecksRollup inside parsePRDetail and never leaves this package raw. type ghPRView struct { - Number int `json:"number"` - URL string `json:"url"` - State string `json:"state"` - Title string `json:"title"` - Body string `json:"body"` - IsDraft bool `json:"isDraft"` - Mergeable string `json:"mergeable"` - MergeStateStatus string `json:"mergeStateStatus"` - ReviewDecision string `json:"reviewDecision"` - BaseRefName string `json:"baseRefName"` - HeadRefName string `json:"headRefName"` - ChangedFiles int `json:"changedFiles"` - IsCrossRepository bool `json:"isCrossRepository"` - StatusCheckRollup []checkItem `json:"statusCheckRollup"` - Commits []ghCommit `json:"commits"` + Number int `json:"number"` + URL string `json:"url"` + State string `json:"state"` + Title string `json:"title"` + Body string `json:"body"` + IsDraft bool `json:"isDraft"` + Mergeable string `json:"mergeable"` + MergeStateStatus string `json:"mergeStateStatus"` + ReviewDecision string `json:"reviewDecision"` + BaseRefName string `json:"baseRefName"` + HeadRefName string `json:"headRefName"` + ChangedFiles int `json:"changedFiles"` + IsCrossRepository bool `json:"isCrossRepository"` + MaintainerCanModify bool `json:"maintainerCanModify"` + StatusCheckRollup []checkItem `json:"statusCheckRollup"` + Commits []ghCommit `json:"commits"` } // ghCommit is one entry of gh's commits array. Authors is a list because of @@ -201,22 +206,23 @@ func parsePRDetail(out []byte, openOnly bool) (*PRDetail, error) { return nil, nil } return &PRDetail{ - Number: v.Number, - URL: v.URL, - Title: v.Title, - Body: v.Body, - State: v.State, - IsDraft: v.IsDraft, - Mergeable: v.Mergeable, - MergeStateStatus: v.MergeStateStatus, - ReviewDecision: v.ReviewDecision, - BaseRefName: v.BaseRefName, - HeadRefName: v.HeadRefName, - ChangedFiles: v.ChangedFiles, - IsCrossRepository: v.IsCrossRepository, - Checks: reduceChecks(v.StatusCheckRollup), - CheckRuns: toCheckItems(v.StatusCheckRollup), - Commits: toCommits(v.Commits), + Number: v.Number, + URL: v.URL, + Title: v.Title, + Body: v.Body, + State: v.State, + IsDraft: v.IsDraft, + Mergeable: v.Mergeable, + MergeStateStatus: v.MergeStateStatus, + ReviewDecision: v.ReviewDecision, + BaseRefName: v.BaseRefName, + HeadRefName: v.HeadRefName, + ChangedFiles: v.ChangedFiles, + IsCrossRepository: v.IsCrossRepository, + MaintainerCanModify: v.MaintainerCanModify, + Checks: reduceChecks(v.StatusCheckRollup), + CheckRuns: toCheckItems(v.StatusCheckRollup), + Commits: toCommits(v.Commits), }, nil } diff --git a/internal/project/pr_test.go b/internal/project/pr_test.go index 02f72a174..73011e622 100644 --- a/internal/project/pr_test.go +++ b/internal/project/pr_test.go @@ -67,6 +67,19 @@ func TestParsePRDetail(t *testing.T) { } }) + // The pair the session button reads: a fork alone says nothing, the maintainer + // permission is what decides whether it can be worked on. + t.Run("a fork PR carries its maintainer-edit permission", func(t *testing.T) { + raw := `{"number":131,"state":"OPEN","isCrossRepository":true,"maintainerCanModify":true}` + pr, err := parsePRDetail([]byte(raw), true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if pr == nil || !pr.IsCrossRepository || !pr.MaintainerCanModify { + t.Errorf("expected an editable fork detail, got %+v", pr) + } + }) + t.Run("non-open PR is hidden", func(t *testing.T) { pr, err := parsePRDetail([]byte(`{"number":88,"state":"MERGED","url":"x"}`), true) if err != nil { diff --git a/internal/project/worktree.go b/internal/project/worktree.go index e7049caa0..958a18447 100644 --- a/internal/project/worktree.go +++ b/internal/project/worktree.go @@ -238,15 +238,17 @@ func (s *Service) CreateWorktree(projectPath, projectID, name, base string, base } // prHead is the little of a pull request CreateWorktreeFromPR needs: which -// branch to check out, and whether that branch lives on a fork. +// branch to check out, whether that branch lives on a fork, and whether the fork +// takes pushes from a maintainer. type prHead struct { RefName string `json:"headRefName"` CrossRepo bool `json:"isCrossRepository"` + CanModify bool `json:"maintainerCanModify"` } // pullRequestHead resolves a pull request's head branch through gh. func (s *Service) pullRequestHead(path string, number int) (prHead, error) { - out, err := s.gh(prReadTimeout, path, prArgs("view", number, "--json", "headRefName,isCrossRepository")...) + out, err := s.gh(prReadTimeout, path, prArgs("view", number, "--json", "headRefName,isCrossRepository,maintainerCanModify")...) if err != nil { return prHead{}, err } @@ -265,11 +267,13 @@ func (s *Service) pullRequestHead(path string, number int) (prHead, error) { // is created detached and handed to `gh pr checkout`, which owns resolving the // head ref and its tracking — the part raw git cannot do for a pull request. // -// It refuses a PR from a fork: the branch would check out, but the commits an -// agent makes there have nowhere to push, so the failure belongs before the -// worktree exists rather than at the first push. A failed checkout takes the -// worktree with it — a detached husk would sit in the picker offering a resume -// of nothing, and would hold the path against a second attempt. +// It refuses a PR from a fork that has "allow edits by maintainers" off: the +// branch would check out, but the commits an agent makes there have nowhere to +// push, so the failure belongs before the worktree exists rather than at the +// first push. With the permission on, the fork's branch takes a push like any +// other and the pull request opens. A failed checkout takes the worktree with it +// — a detached husk would sit in the picker offering a resume of nothing, and +// would hold the path against a second attempt. func (s *Service) CreateWorktreeFromPR(projectPath, projectID string, number int) (*Worktree, error) { if number <= 0 { return nil, fmt.Errorf("%d is not a pull request number.", number) @@ -278,8 +282,8 @@ func (s *Service) CreateWorktreeFromPR(projectPath, projectID string, number int if err != nil { return nil, err } - if head.CrossRepo { - return nil, fmt.Errorf("Pull request #%d comes from a fork: its branch could not be pushed back.", number) + if head.CrossRepo && !head.CanModify { + return nil, fmt.Errorf("Pull request #%d comes from a fork that does not allow edits by maintainers: its branch could not be pushed back.", number) } wtPath, err := reserveWorktreePath(projectPath, projectID, head.RefName) if err != nil { diff --git a/internal/project/worktree_test.go b/internal/project/worktree_test.go index 6f8e05fac..1479cc968 100644 --- a/internal/project/worktree_test.go +++ b/internal/project/worktree_test.go @@ -430,10 +430,10 @@ func TestCreateWorktreeFromPR(t *testing.T) { } }) - t.Run("a fork pull request is refused before anything is created", func(t *testing.T) { + t.Run("a fork pull request with no maintainer edits is refused before anything is created", func(t *testing.T) { t.Setenv("XDG_DATA_HOME", t.TempDir()) repo, _ := initRepo(t) - gh := &scriptedGH{headJSON: `{"headRefName":"patch-1","isCrossRepository":true}`} + gh := &scriptedGH{headJSON: `{"headRefName":"patch-1","isCrossRepository":true,"maintainerCanModify":false}`} _, err := (&Service{runner: gh.run}).CreateWorktreeFromPR(repo, "proj", 103) if err == nil || !strings.Contains(err.Error(), "fork") { @@ -444,6 +444,23 @@ func TestCreateWorktreeFromPR(t *testing.T) { } }) + // The permission is the whole gate: with it on, the fork's branch takes a + // push back and the pull request is workable like any other. + t.Run("a fork pull request that allows maintainer edits is checked out", func(t *testing.T) { + data := t.TempDir() + t.Setenv("XDG_DATA_HOME", data) + repo, _ := initRepo(t) + gh := &scriptedGH{headJSON: `{"headRefName":"patch-1","isCrossRepository":true,"maintainerCanModify":true}`} + + wt, err := (&Service{runner: gh.run}).CreateWorktreeFromPR(repo, "proj", 103) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if canonicalPath(gh.checkoutDir) != wt.Path { + t.Errorf("gh pr checkout ran in %q, want %q", gh.checkoutDir, wt.Path) + } + }) + t.Run("a failed checkout leaves no worktree behind", func(t *testing.T) { data := t.TempDir() t.Setenv("XDG_DATA_HOME", data)