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)