From bb6c0604126bcfb33bc239622499b6ed252c8167 Mon Sep 17 00:00:00 2001 From: omartelo Date: Sat, 24 Oct 2026 09:42:09 -0300 Subject: [PATCH] refactor(project): read a checkout's pull request in one place The cards' badges asked gh themselves, once a minute per open pull request of the shown project, while the merge watch read the same pull requests in the backend. The pull request watch (internal/project, branchpr.go) is now the one reader: it reads every checkout a live agent runs in and every one last read with an open pull request each minute, and any other when a window asks with project.RecheckPullRequest; every read goes out as branch-pull-request, which the badges listen to, and a merge it sees after an open read is announced through relay.AnnounceMerge as before. project.PullRequest is gone: the window was its only caller. The badge still follows the shell's cwd. --- api/events.go | 5 + api/project.go | 23 +- api/testdata/contract.golden | 23 +- docs/ceilings.md | 24 +- internal/project/branchpr.go | 201 ++++++++++++ internal/project/branchpr_test.go | 308 +++++++++++++++++++ internal/project/pr.go | 15 - internal/project/pr_test.go | 56 ---- internal/project/project.go | 42 +-- internal/project/project_test.go | 33 -- internal/relay/checkouts.go | 37 +++ internal/relay/checkouts_test.go | 32 ++ internal/relay/merged.go | 6 +- internal/relay/merged_test.go | 29 ++ internal/relay/mergewatch.go | 94 ------ internal/relay/mergewatch_test.go | 145 --------- internal/relay/relay.go | 6 +- internal/routes/project.go | 2 +- main.go | 9 +- native/backend/project.go | 11 +- native/cmd/lich-native/cardstate_test.go | 2 +- native/cmd/lich-native/gitstate.go | 62 ++-- native/cmd/lich-native/gitstate_test.go | 68 ++-- native/cmd/lich-native/model.go | 4 + native/cmd/lich-native/model_test.go | 2 +- native/cmd/lich-native/pulls.go | 4 + native/cmd/lich-native/stores_events_test.go | 60 ++-- 27 files changed, 796 insertions(+), 507 deletions(-) create mode 100644 internal/project/branchpr.go create mode 100644 internal/project/branchpr_test.go create mode 100644 internal/relay/checkouts.go create mode 100644 internal/relay/checkouts_test.go delete mode 100644 internal/relay/mergewatch.go delete mode 100644 internal/relay/mergewatch_test.go diff --git a/api/events.go b/api/events.go index dbcd3422a..8d082266e 100644 --- a/api/events.go +++ b/api/events.go @@ -274,6 +274,11 @@ var ( // instead of the screen that made the change telling the others. PullRequestChangedEvent = defineEvent[PRChanged]("pull-request-changed") + // BranchPullRequestEvent is every read of a checkout's branch pull request, + // changed or not: the backend is the one reader of it, and a window that + // asked for a recheck hears its answer here. + BranchPullRequestEvent = defineEvent[BranchPullRequest]("branch-pull-request") + // ServicesEvent carries every project service and its state, emitted once a // change to either is true: a save, a delete, a start, an exit. ServicesEvent = defineEvent[[]Service]("services") diff --git a/api/project.go b/api/project.go index 8fb129b22..a863d1288 100644 --- a/api/project.go +++ b/api/project.go @@ -35,7 +35,7 @@ var ( ProjectWorktreeAdopted = define[PathOptions, bool]("project.WorktreeAdopted") ProjectGitHubAccounts = define[None, []string]("project.GitHubAccounts") ProjectIssue = define[NumberOptions, Issue]("project.Issue") - ProjectPullRequest = define[PathOptions, *PullRequest]("project.PullRequest") + ProjectRecheckPullRequest = define[PathOptions, None]("project.RecheckPullRequest") ProjectListPullRequests = define[ListPullRequestsOptions, []PRSummary]("project.ListPullRequests") ProjectPullRequestDetail = define[NumberOptions, *PRDetail]("project.PullRequestDetail") ProjectPullRequestConversation = define[NumberOptions, *PRConversation]("project.PullRequestConversation") @@ -353,13 +353,22 @@ type OpenedProject struct { Path string `json:"path"` } -// PullRequest identifies the open GitHub pull request for a work tree's current -// branch, as reported by the gh CLI. State is one of gh's OPEN, CLOSED, MERGED; -// only OPEN reaches the window (see parsePullRequest). +// PullRequest is the GitHub pull request of a work tree's current branch, as +// reported by the gh CLI, whatever its state: one of gh's OPEN, CLOSED, MERGED. type PullRequest struct { - Number int `json:"number"` - URL string `json:"url"` - State string `json:"state"` + Number int `json:"number"` + URL string `json:"url"` + Title string `json:"title"` + State string `json:"state"` + HeadRefName string `json:"headRefName"` + BaseRefName string `json:"baseRefName"` +} + +// BranchPullRequest is one read of the pull request of the branch checked out +// at Path; PR is nil when the branch has none. +type BranchPullRequest struct { + Path string `json:"path"` + PR *PullRequest `json:"pr"` } // DiffStats is what one status read says about a work tree: its uncommitted diff --git a/api/testdata/contract.golden b/api/testdata/contract.golden index 3e22d17a0..f4377f958 100644 --- a/api/testdata/contract.golden +++ b/api/testdata/contract.golden @@ -78,7 +78,6 @@ project.Missing(PathsOptions) []string project.Open(None) *OpenedProject project.PickFile(PickFileOptions) string project.PickSaveFile(PickSaveFileOptions) string -project.PullRequest(PathOptions) *PullRequest project.PullRequestConflicts(PRConflictsOptions) []string project.PullRequestConversation(NumberOptions) *PRConversation project.PullRequestDetail(NumberOptions) *PRDetail @@ -86,6 +85,7 @@ project.PullRequestDiff(NumberOptions) string project.PullRequestMergeBase(NumberOptions) string project.ReadFile(CheckoutFileOptions) string project.ReadFileAt(ReadFileAtOptions) string +project.RecheckPullRequest(PathOptions) None project.Relocate(ProjectIDOptions) *OpenedProject project.RemoveWorktree(RemoveWorktreeOptions) None project.ReplyToReviewThread(PRCommentBodyOptions) None @@ -211,6 +211,7 @@ themes.UpdateFromGit(UpdateThemeOptions) ThemeGitResult tray.SetLabels(TrayLabels) None event appupdate-progress AppUpdateProgress +event branch-pull-request BranchPullRequest event claude-profile-login ProfileLogin event codex-profile-login ProfileLogin event lich-plugin-profiles PluginProfiles @@ -543,12 +544,6 @@ PickSaveFileOptions { defaultName string } -PullRequest { - number int - url string - state string -} - PRConflictsOptions { path string number int @@ -1383,6 +1378,11 @@ AppUpdateProgress { total int64 } +BranchPullRequest { + path string + pr *PullRequest +} + ProfileLogin { profile string output,omitempty string @@ -1723,6 +1723,15 @@ ThemeSource { Syntax = map[string]SyntaxStyle +PullRequest { + number int + url string + title string + state string + headRefName string + baseRefName string +} + PluginProfileFailure { profile string name string diff --git a/docs/ceilings.md b/docs/ceilings.md index 1b9ca69b3..980367e1b 100644 --- a/docs/ceilings.md +++ b/docs/ceilings.md @@ -321,17 +321,19 @@ work when nobody knows it and that the call site never shows. The mechanism and drops the prompt is reported undelivered with its reason, except at a session that was mid-turn, whose ack lich stops waiting for after the receipt window. - **A merge is announced only to a card running at that moment, and one made outside lich up to a minute late** - (`internal/relay`, `AnnounceMerge`; called from the Pulls screen's merge and by `RunMergeWatch`): a merge on - github.com, with `gh` in a terminal or by an agent is found by reading, each minute, the pull request of every - checkout a live agent runs in, one `gh pr view` per checkout. It is announced only when one read saw the pull - request open and a later one sees it merged, so a pull request that merged while lich was closed, or while every - agent in its checkout was parked, is never announced; a card parked while the merge is seen, or never opened, - stays believing it is open. What was read open and what was announced live in memory, so a restart forgets both: - a merge already announced is not repeated, because the next read finds it merged without having seen it open. - The session that opened one of the checkout's sessions hears it too, wherever it runs, but only that one: the - session that opened the opener does not. The notice also lands as a delivery like any other, so a cleanup - clicked in the second before it starts a turn finds no session mid-turn, asks nothing, and removes the checkout - with the notice still on its way. + (`internal/relay`, `AnnounceMerge`; called from the Pulls screen's merge and by the pull request watch, + `internal/project` `RunBranchPRWatch`): a merge on github.com, with `gh` in a terminal or by an agent is found by + the watch, the one reader of a checkout's pull request, which the cards' badges hear too. Each minute it reads, + one `gh pr view` each, every checkout a live agent runs in and every one whose last read found an open pull + request; any other is read only when a window asks (its branch or head moved, a turn ended there, the window came + back into focus). A merge is announced only when one read saw the pull request open and a later one sees it + merged, so one that merged while lich was closed, or before any read saw it open, is never announced; a card + parked while the merge is seen, or never opened, stays believing it is open. What was read and what was announced + live in memory, so a restart forgets both: a merge already announced is not repeated, because the next read + finds it merged without having seen it open. The session that opened one of the checkout's sessions hears it + too, wherever it runs, but only that one: the session that opened the opener does not. The notice also lands as + a delivery like any other, so a cleanup clicked in the second before it starts a turn finds no session mid-turn, + asks nothing, and removes the checkout with the notice still on its way. - **Only a Claude Code subagent becomes a lich card** (lich-plugin's `hooks/agent-cards.js`, its `docs/agent-cards.md`): the plugin's mod takes the model's general-purpose `Agent` call and runs it as a lich session with `lich open --subagent --prompt`, in the background. The card opens in the asking session's checkout diff --git a/internal/project/branchpr.go b/internal/project/branchpr.go new file mode 100644 index 000000000..3e979f9ee --- /dev/null +++ b/internal/project/branchpr.go @@ -0,0 +1,201 @@ +package project + +import ( + "context" + "encoding/json" + "errors" + "io/fs" + "log/slog" + "os" + "slices" + "sync" + "time" + + "github.com/lichdotdev/lich/api" + "github.com/lichdotdev/lich/internal/events" +) + +// The pull request of each checkout's branch has one reader, here. The cards' +// badges hear every read as api.BranchPullRequestEvent, and a merge made +// outside lich (on github.com, with gh in a terminal, by an agent) is told to +// the agents in its checkout from the same reads, the way the Pulls screen's +// merge is. + +// branchPRTick is how often the watch reads the pull request of every checkout +// it follows: one gh call per checkout, for a badge and a merge notice that +// read as fine a minute late. +const branchPRTick = time.Minute + +// prMerged is the state gh reports a merged pull request in. +const prMerged = "MERGED" + +// branchPRs is what the watch last read at each checkout, and which reads are +// running. A path in last was read at least once; its value is nil for a +// branch with no pull request. +type branchPRs struct { + mu sync.Mutex + last map[string]*PullRequest + reading map[string]bool + again map[string]bool +} + +// SetAgentCheckouts wires the checkouts an agent runs in, which the watch reads +// every tick: a pull request opens there whether or not a window is looking. +func (s *Service) SetAgentCheckouts(checkouts func() []string) { s.agentCheckouts = checkouts } + +// SetAnnounceMerge wires who tells a checkout's agents that its pull request +// merged (internal/relay AnnounceMerge). +func (s *Service) SetAnnounceMerge(announce func(context.Context, api.AnnounceMergeOptions) (api.None, error)) { + s.announceMerge = announce +} + +// RecheckPullRequest reads the pull request of opts.Path's branch again, behind +// the call: the answer arrives as api.BranchPullRequestEvent. A window asks +// when it has seen the branch or the head move, a turn end, or anything else +// gh would answer differently after. +func (s *Service) RecheckPullRequest(_ context.Context, opts api.PathOptions) (api.None, error) { + go s.readBranchPR(opts.Path) + return api.None{}, nil +} + +// RunBranchPRWatch reads the followed checkouts every branchPRTick, for as long +// as lich runs. +func (s *Service) RunBranchPRWatch() { + ticker := time.NewTicker(branchPRTick) + defer ticker.Stop() + for range ticker.C { + for _, path := range s.followedCheckouts() { + s.readBranchPR(path) + } + } +} + +// followedCheckouts is every checkout an agent runs in, and every one whose +// last read found an open pull request, which may merge or close with nobody +// in the checkout. A checkout with neither is read only when a window asks. +func (s *Service) followedCheckouts() []string { + var paths []string + if s.agentCheckouts != nil { + paths = s.agentCheckouts() + } + s.prs.mu.Lock() + defer s.prs.mu.Unlock() + for path, pr := range s.prs.last { + if pr != nil && pr.State == prOpen && !slices.Contains(paths, path) { + paths = append(paths, path) + } + } + return paths +} + +// readBranchPR reads path's pull request, one read at a time per path. One +// asked for while another runs is read again once it ends, since the branch +// may have moved under the first. +func (s *Service) readBranchPR(path string) { + if !s.prs.claim(path) { + return + } + for { + s.recordBranchPR(path) + if !s.prs.release(path) { + return + } + } +} + +// recordBranchPR reads path's pull request, reports it and announces a merge +// the read before did not see. A failed read reports nothing and keeps what the +// last one saw, except for a checkout that is gone, which is forgotten. +func (s *Service) recordBranchPR(path string) { + pr, err := s.BranchPullRequest(path) + if err != nil { + if _, statErr := os.Stat(path); errors.Is(statErr, fs.ErrNotExist) { + s.prs.forget(path) + return + } + slog.Debug("project: read the pull request of a checkout", "path", path, "err", err) + return + } + merged := s.prs.record(path, pr) + if s.emit != nil { + events.Send(s.emit, api.BranchPullRequestEvent, api.BranchPullRequest{Path: path, PR: pr}) + } + if !merged || s.announceMerge == nil { + return + } + opts := api.AnnounceMergeOptions{Checkout: path, Number: pr.Number, Title: pr.Title, Branch: pr.HeadRefName, Base: pr.BaseRefName} + if _, err := s.announceMerge(context.Background(), opts); err != nil { + slog.Warn("project: announce a merge made outside lich", "path", path, "err", err) + } +} + +// BranchPullRequest returns the pull request of the branch checked out at path +// whatever its state, nil when the branch has none. +func (s *Service) BranchPullRequest(path string) (*PullRequest, error) { + out, err := s.gh(prLookupTimeout, path, "pr", "view", "--json", "number,url,title,state,headRefName,baseRefName") + if errors.Is(err, errNoPullRequest) { + return nil, nil + } + if err != nil { + return nil, err + } + var pr PullRequest + if err := json.Unmarshal(out, &pr); err != nil { + return nil, errUnreadableAnswer(err) + } + // gh emits `{}` in some no-PR states. + if pr.Number == 0 { + return nil, nil + } + return &pr, nil +} + +// claim marks a read of path running and answers whether the caller is the one +// to run it; otherwise the running one reads again when it ends. +func (b *branchPRs) claim(path string) bool { + b.mu.Lock() + defer b.mu.Unlock() + if b.reading == nil { + b.reading, b.again = map[string]bool{}, map[string]bool{} + } + if b.reading[path] { + b.again[path] = true + return false + } + b.reading[path] = true + return true +} + +// release ends a read of path and answers whether another was asked for +// meanwhile, which the caller then runs. +func (b *branchPRs) release(path string) bool { + b.mu.Lock() + defer b.mu.Unlock() + if b.again[path] { + delete(b.again, path) + return true + } + delete(b.reading, path) + return false +} + +// record keeps pr as path's last read and answers whether it is the pull +// request the read before found open, now merged. One first read already +// merged is not announced: it may have merged long before lich started, and +// its agents heard of it then or never will. +func (b *branchPRs) record(path string, pr *PullRequest) bool { + b.mu.Lock() + defer b.mu.Unlock() + if b.last == nil { + b.last = map[string]*PullRequest{} + } + prev := b.last[path] + b.last[path] = pr + return prev != nil && prev.State == prOpen && pr != nil && pr.State == prMerged && pr.Number == prev.Number +} + +func (b *branchPRs) forget(path string) { + b.mu.Lock() + defer b.mu.Unlock() + delete(b.last, path) +} diff --git a/internal/project/branchpr_test.go b/internal/project/branchpr_test.go new file mode 100644 index 000000000..3b392aa0b --- /dev/null +++ b/internal/project/branchpr_test.go @@ -0,0 +1,308 @@ +package project + +import ( + "context" + "errors" + "slices" + "sync" + "testing" + "time" + + "github.com/lichdotdev/lich/api" +) + +// forgePRs answers gh's branch pull request read with what each checkout's +// branch has at the moment, and counts the reads. +type forgePRs struct { + mu sync.Mutex + out map[string]string + err error + reads map[string]int + hold chan struct{} +} + +func newForgePRs() *forgePRs { return &forgePRs{out: map[string]string{}, reads: map[string]int{}} } + +func (f *forgePRs) set(path, out string) { + f.mu.Lock() + defer f.mu.Unlock() + f.out[path] = out +} + +func (f *forgePRs) run(_ time.Duration, dir, _ string, _ ...string) ([]byte, error) { + f.mu.Lock() + hold := f.hold + f.mu.Unlock() + if hold != nil { + <-hold + } + f.mu.Lock() + defer f.mu.Unlock() + f.reads[dir]++ + if f.err != nil { + return nil, f.err + } + if f.out[dir] == "" { + return nil, errNoPullRequest + } + return []byte(f.out[dir]), nil +} + +func (f *forgePRs) readsOf(path string) int { + f.mu.Lock() + defer f.mu.Unlock() + return f.reads[path] +} + +const ( + featOpen = `{"number":42,"url":"u","title":"Add merge notice","state":"OPEN","headRefName":"feat/merge","baseRefName":"main"}` + featMerged = `{"number":42,"url":"u","title":"Add merge notice","state":"MERGED","headRefName":"feat/merge","baseRefName":"main"}` + featClosed = `{"number":42,"url":"u","title":"Add merge notice","state":"CLOSED","headRefName":"feat/merge","baseRefName":"main"}` +) + +// watching is a Service on forge that records every BranchPullRequestEvent and +// every merge it announces. +type watching struct { + *Service + mu sync.Mutex + events []api.BranchPullRequest + merges []api.AnnounceMergeOptions + mergeOK error +} + +func newWatching(forge *forgePRs) *watching { + w := &watching{Service: &Service{runner: forge.run}} + w.SetEmit(func(name string, data any) { + if name == api.BranchPullRequestEvent.Name { + w.mu.Lock() + w.events = append(w.events, data.(api.BranchPullRequest)) + w.mu.Unlock() + } + }) + w.SetAnnounceMerge(func(_ context.Context, opts api.AnnounceMergeOptions) (api.None, error) { + w.mu.Lock() + defer w.mu.Unlock() + w.merges = append(w.merges, opts) + return api.None{}, w.mergeOK + }) + return w +} + +func (w *watching) merged() []api.AnnounceMergeOptions { + w.mu.Lock() + defer w.mu.Unlock() + return slices.Clone(w.merges) +} + +func TestBranchPullRequestKeepsAMergedOne(t *testing.T) { + t.Parallel() + gh := &fakeGH{out: []byte(featMerged)} + pr, err := withGH(gh).BranchPullRequest("/repo") + if err != nil { + t.Fatalf("BranchPullRequest: %v", err) + } + want := PullRequest{Number: 42, URL: "u", Title: "Add merge notice", State: "MERGED", HeadRefName: "feat/merge", BaseRefName: "main"} + if pr == nil || *pr != want { + t.Fatalf("BranchPullRequest = %+v, want %+v", pr, want) + } + if want := []string{"pr", "view", "--json", "number,url,title,state,headRefName,baseRefName"}; !slices.Equal(gh.args, want) { + t.Errorf("args = %v, want %v", gh.args, want) + } + if gh.dir != "/repo" || gh.timeout != prLookupTimeout { + t.Errorf("wrong scope: dir %q timeout %v", gh.dir, gh.timeout) + } + + for name, gh := range map[string]*fakeGH{"no PR": {err: errNoPullRequest}, "empty object": {out: []byte(`{}`)}} { + if pr, err := withGH(gh).BranchPullRequest("/repo"); err != nil || pr != nil { + t.Errorf("%s = %+v, %v, want nil, nil", name, pr, err) + } + } + if _, err := withGH(&fakeGH{err: errTest}).BranchPullRequest("/repo"); !errors.Is(err, errTest) { + t.Errorf("failed lookup err = %v, want %v", err, errTest) + } + if _, err := withGH(&fakeGH{out: []byte(`not json`)}).BranchPullRequest("/repo"); err == nil { + t.Error("malformed answer: want a decode error") + } +} + +// The badge read runs as the project's account, like every other GitHub call. +func TestBranchPullRequestRunsAsTheProjectsAccount(t *testing.T) { + t.Parallel() + account := &accountGH{token: "gho_secret", pr: featOpen} + svc := &Service{runner: account.run} + svc.SetAccounts(func(string) string { return "octocat" }) + if pr, err := svc.BranchPullRequest("/repo"); err != nil || pr == nil { + t.Fatalf("BranchPullRequest = %+v, %v, want the open PR", pr, err) + } + if account.viewToken != "gho_secret" { + t.Errorf("read ran with token %q, want the configured account's", account.viewToken) + } +} + +func TestEveryReadIsReported(t *testing.T) { + t.Parallel() + forge := newForgePRs() + w := newWatching(forge) + w.readBranchPR("/wt/feat") + forge.set("/wt/feat", featOpen) + w.readBranchPR("/wt/feat") + w.readBranchPR("/wt/feat") + + if len(w.events) != 3 || w.events[0].PR != nil || w.events[1].PR == nil || w.events[2].PR == nil { + t.Fatalf("events = %+v, want none, then the open PR twice", w.events) + } + if w.events[1].Path != "/wt/feat" || w.events[1].PR.Number != 42 { + t.Fatalf("event = %+v, want #42 at /wt/feat", w.events[1]) + } +} + +func TestAMergeAfterAnOpenReadIsAnnounced(t *testing.T) { + t.Parallel() + forge := newForgePRs() + forge.set("/wt/feat", featOpen) + w := newWatching(forge) + w.readBranchPR("/wt/feat") + forge.set("/wt/feat", featMerged) + w.readBranchPR("/wt/feat") + w.readBranchPR("/wt/feat") + + want := []api.AnnounceMergeOptions{{Checkout: "/wt/feat", Number: 42, Title: "Add merge notice", Branch: "feat/merge", Base: "main"}} + if got := w.merged(); !slices.Equal(got, want) { + t.Fatalf("merges = %+v, want %+v", got, want) + } +} + +func TestAMergeFirstReadMergedOrAfterACloseIsNotAnnounced(t *testing.T) { + t.Parallel() + forge := newForgePRs() + forge.set("/wt/a", featMerged) + forge.set("/wt/b", featOpen) + w := newWatching(forge) + w.readBranchPR("/wt/a") + w.readBranchPR("/wt/b") + forge.set("/wt/b", featClosed) + w.readBranchPR("/wt/b") + forge.set("/wt/b", featMerged) + w.readBranchPR("/wt/b") + + if got := w.merged(); len(got) != 0 { + t.Fatalf("merges = %+v, want none", got) + } +} + +func TestAFailedReadKeepsWhatTheLastOneSaw(t *testing.T) { + t.Parallel() + dir := t.TempDir() + forge := newForgePRs() + forge.set(dir, featOpen) + w := newWatching(forge) + // The announce failing is only logged: the merge already happened. + w.mergeOK = errors.New("disk gone") + w.readBranchPR(dir) + forge.mu.Lock() + forge.err = errors.New("gh is not signed in to GitHub.") + forge.mu.Unlock() + w.readBranchPR(dir) + if len(w.events) != 1 { + t.Fatalf("events = %+v, want only the read that answered", w.events) + } + forge.mu.Lock() + forge.err = nil + forge.mu.Unlock() + forge.set(dir, featMerged) + w.readBranchPR(dir) + + if got := w.merged(); len(got) != 1 { + t.Fatalf("merges = %+v, want the one merge", got) + } +} + +func TestTheWatchFollowsAgentsAndOpenPullRequests(t *testing.T) { + t.Parallel() + forge := newForgePRs() + forge.set("/wt/open", featOpen) + forge.set("/wt/merged", featMerged) + w := newWatching(forge) + w.SetAgentCheckouts(func() []string { return []string{"/wt/agent", "/wt/open"} }) + for _, path := range []string{"/wt/open", "/wt/merged", "/wt/none"} { + w.readBranchPR(path) + } + + got := w.followedCheckouts() + slices.Sort(got) + if want := []string{"/wt/agent", "/wt/open"}; !slices.Equal(got, want) { + t.Fatalf("followed = %v, want %v", got, want) + } + + w.SetAgentCheckouts(nil) + if got := w.followedCheckouts(); !slices.Equal(got, []string{"/wt/open"}) { + t.Fatalf("followed with no agent = %v, want the open PR's checkout", got) + } +} + +func TestAGoneCheckoutIsNoLongerFollowed(t *testing.T) { + t.Parallel() + here, gone := t.TempDir(), t.TempDir()+"/removed" + forge := newForgePRs() + forge.set(here, featOpen) + forge.set(gone, featOpen) + w := newWatching(forge) + w.readBranchPR(here) + w.readBranchPR(gone) + forge.mu.Lock() + forge.err = errors.New("gh failed") + forge.mu.Unlock() + w.readBranchPR(here) + w.readBranchPR(gone) + + if got := w.followedCheckouts(); !slices.Equal(got, []string{here}) { + t.Fatalf("followed = %v, want %s alone: the other checkout is gone", got, here) + } +} + +func TestARecheckDuringAReadReadsAgainAfterIt(t *testing.T) { + t.Parallel() + forge := newForgePRs() + forge.hold = make(chan struct{}) + w := newWatching(forge) + done := make(chan struct{}) + go func() { w.readBranchPR("/wt/feat"); close(done) }() + waitClaimed(t, w, "/wt/feat") + + if _, err := w.RecheckPullRequest(t.Context(), api.PathOptions{Path: "/wt/feat"}); err != nil { + t.Fatalf("RecheckPullRequest: %v", err) + } + waitAgain(t, w, "/wt/feat") + forge.mu.Lock() + close(forge.hold) + forge.hold = nil + forge.mu.Unlock() + <-done + + if got := forge.readsOf("/wt/feat"); got != 2 { + t.Fatalf("reads = %d, want 2: the one running and the one asked meanwhile", got) + } +} + +func waitClaimed(t *testing.T, w *watching, path string) { + t.Helper() + waitPRs(t, w, func(b *branchPRs) bool { return b.reading[path] }) +} + +func waitAgain(t *testing.T, w *watching, path string) { + t.Helper() + waitPRs(t, w, func(b *branchPRs) bool { return b.again[path] }) +} + +func waitPRs(t *testing.T, w *watching, ok func(*branchPRs) bool) { + t.Helper() + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(time.Millisecond) { + w.prs.mu.Lock() + done := ok(&w.prs) + w.prs.mu.Unlock() + if done { + return + } + } + t.Fatal("the watch never reached the state") +} diff --git a/internal/project/pr.go b/internal/project/pr.go index 81fac3f56..cc112a08e 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -158,21 +158,6 @@ func (s *Service) PullRequestDetail(_ context.Context, opts api.NumberOptions) ( return detail, nil } -// BranchPullRequest returns the pull request of the branch checked out at path -// whatever its state, nil when the branch has none: the merge watch -// (internal/relay) needs to see it go from open to merged, which the badge's -// OPEN-only read hides. -func (s *Service) BranchPullRequest(path string) (*PRDetail, error) { - out, err := s.gh(prLookupTimeout, path, "pr", "view", "--json", "number,title,state,headRefName,baseRefName") - if errors.Is(err, errNoPullRequest) { - return nil, nil - } - if err != nil { - return nil, err - } - return parsePRDetail(out, false) -} - // parsePRDetail decodes gh's JSON and reduces the check rollup. With openOnly it // returns nil for a non-OPEN PR — gh still reports a merged/closed branch PR, // but the badge wants only an actionable one — matching parsePullRequest's diff --git a/internal/project/pr_test.go b/internal/project/pr_test.go index 2b8bb0b3c..517f8f3ed 100644 --- a/internal/project/pr_test.go +++ b/internal/project/pr_test.go @@ -525,62 +525,6 @@ func TestPullRequestDetailFlow(t *testing.T) { }) } -func TestBranchPullRequestKeepsAMergedOne(t *testing.T) { - t.Parallel() - gh := &fakeGH{out: []byte(`{"number":7,"title":"t","state":"MERGED","headRefName":"feat","baseRefName":"main"}`)} - pr, err := withGH(gh).BranchPullRequest("/repo") - if err != nil { - t.Fatalf("BranchPullRequest: %v", err) - } - if pr == nil || pr.Number != 7 || pr.State != "MERGED" || pr.HeadRefName != "feat" || pr.BaseRefName != "main" { - t.Fatalf("BranchPullRequest = %+v, want the merged PR", pr) - } - if gh.dir != "/repo" || gh.timeout != prLookupTimeout { - t.Errorf("wrong scope: dir %q timeout %v", gh.dir, gh.timeout) - } - - if pr, err := withGH(&fakeGH{err: errNoPullRequest}).BranchPullRequest("/repo"); err != nil || pr != nil { - t.Errorf("no PR = %+v, %v, want nil, nil", pr, err) - } - if _, err := withGH(&fakeGH{err: errTest}).BranchPullRequest("/repo"); !errors.Is(err, errTest) { - t.Errorf("failed lookup err = %v, want %v", err, errTest) - } -} - -// TestPullRequestBadgeRunsThroughTheRunner pins the badge to the same runner as -// the rest: it used to shell out to gh on its own, which left it running as -// whatever account gh had active while the screen beside it used the project's. -func TestPullRequestBadgeRunsThroughTheRunner(t *testing.T) { - t.Parallel() - gh := &fakeGH{out: []byte(`{"number":7,"url":"https://github.com/o/r/pull/7","state":"OPEN"}`)} - pr := withGH(gh).pullRequest("/repo") - if pr == nil || pr.Number != 7 { - t.Fatalf("PullRequest() = %+v, want the open PR", pr) - } - if want := []string{"pr", "view", "--json", "number,url,state"}; !slices.Equal(gh.args, want) { - t.Errorf("args = %v, want %v", gh.args, want) - } - if gh.dir != "/repo" || gh.timeout != prLookupTimeout { - t.Errorf("wrong scope: dir %q timeout %v", gh.dir, gh.timeout) - } - - // A failure still hides the badge rather than surfacing anything. - if pr := withGH(&fakeGH{err: errTest}).pullRequest("/repo"); pr != nil { - t.Errorf("failed lookup = %+v, want nil", pr) - } - - // And it runs as the project's account, like every other GitHub call. - account := &accountGH{token: "gho_secret", pr: `{"number":7,"url":"u","state":"OPEN"}`} - svc := &Service{runner: account.run} - svc.SetAccounts(func(string) string { return "octocat" }) - if pr := svc.pullRequest("/repo"); pr == nil { - t.Fatal("PullRequest() = nil, want the open PR") - } - if account.viewToken != "gho_secret" { - t.Errorf("badge ran with token %q, want the configured account's", account.viewToken) - } -} - func TestMergePullRequestFlow(t *testing.T) { t.Parallel() t.Run("the built args reach gh", func(t *testing.T) { diff --git a/internal/project/project.go b/internal/project/project.go index 75e9337a9..6dd15911d 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -9,7 +9,6 @@ import ( "context" "crypto/sha256" "encoding/hex" - "encoding/json" "fmt" "os" "path/filepath" @@ -58,6 +57,13 @@ type Service struct { // fixes keeps the review threads waiting for a push (threadfix.go); nil // keeps nothing. fixes threadFixStore + // prs is the pull request watch's last reads (branchpr.go). Zero value + // ready. + prs branchPRs + // agentCheckouts and announceMerge wire the watch to the sessions + // (branchpr.go); nil follows no checkout and announces nothing. + agentCheckouts func() []string + announceMerge func(context.Context, api.AnnounceMergeOptions) (api.None, error) } // New returns a project service using the given picker. @@ -144,41 +150,9 @@ func (s *Service) Branch(path string) string { type PullRequest = api.PullRequest // prLookupTimeout caps the gh network call so a slow forge or hung auth prompt -// never stalls the footer poll. +// never stalls the pull request watch (branchpr.go). const prLookupTimeout = 5 * time.Second -// PullRequest answers pullRequest over RPC. -func (s *Service) PullRequest(_ context.Context, opts api.PathOptions) (*PullRequest, error) { - return s.pullRequest(opts.Path), nil -} - -// pullRequest returns the open pull request for the path's current branch, or nil -// when there is none, gh is missing/unauthenticated, or the path is not a GitHub -// repo. It shells out to `gh pr view`, which resolves the PR from the checked-out -// branch. Any failure yields nil, matching Branch's "hide the segment" contract. -func (s *Service) pullRequest(path string) *PullRequest { - out, err := s.gh(prLookupTimeout, path, "pr", "view", "--json", "number,url,state") - if err != nil { - return nil - } - return parsePullRequest(out) -} - -// parsePullRequest decodes gh's `pr view --json` output. It returns nil for -// malformed JSON, a zero PR number (gh emits `{}` in some no-PR states), or a -// non-OPEN state — gh reports the branch's PR even after it is merged or closed, -// so without the state gate a merged PR would keep showing the badge. -func parsePullRequest(out []byte) *PullRequest { - var pr PullRequest - if err := json.Unmarshal(out, &pr); err != nil { - return nil - } - if pr.Number == 0 || pr.URL == "" || pr.State != "OPEN" { - return nil - } - return &pr -} - // Exists answers exists over RPC. func (s *Service) Exists(_ context.Context, opts api.PathOptions) (bool, error) { return s.exists(opts.Path), nil diff --git a/internal/project/project_test.go b/internal/project/project_test.go index 500a0aa61..eb79abb19 100644 --- a/internal/project/project_test.go +++ b/internal/project/project_test.go @@ -201,39 +201,6 @@ func TestBranchesOfNothing(t *testing.T) { } } -// TestParsePullRequest proves the gh output decoder: a real PR yields its number -// and URL, while malformed JSON, an empty object, and a PR missing its number or -// URL all collapse to nil so the badge hides instead of showing garbage. -func TestParsePullRequest(t *testing.T) { - t.Parallel() - tests := []struct { - name string - out string - want *PullRequest - }{ - {"open", `{"number":7,"url":"https://github.com/o/r/pull/7","state":"OPEN"}`, &PullRequest{Number: 7, URL: "https://github.com/o/r/pull/7", State: "OPEN"}}, - {"merged", `{"number":7,"url":"https://github.com/o/r/pull/7","state":"MERGED"}`, nil}, - {"closed", `{"number":7,"url":"https://github.com/o/r/pull/7","state":"CLOSED"}`, nil}, - {"missing state", `{"number":7,"url":"https://github.com/o/r/pull/7"}`, nil}, - {"empty object", `{}`, nil}, - {"zero number", `{"number":0,"url":"https://github.com/o/r/pull/0","state":"OPEN"}`, nil}, - {"missing url", `{"number":7,"state":"OPEN"}`, nil}, - {"malformed", `not json`, nil}, - {"empty bytes", ``, nil}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := parsePullRequest([]byte(tt.out)) - switch { - case tt.want == nil && got != nil: - t.Errorf("parsePullRequest(%q) = %+v, want nil", tt.out, got) - case tt.want != nil && (got == nil || *got != *tt.want): - t.Errorf("parsePullRequest(%q) = %+v, want %+v", tt.out, got, tt.want) - } - }) - } -} - // TestDiff proves Diff counts dirty files and added/deleted lines against HEAD, // and returns the zero value for clean repositories and non-repositories. func TestDiff(t *testing.T) { diff --git a/internal/relay/checkouts.go b/internal/relay/checkouts.go new file mode 100644 index 000000000..c78c3d029 --- /dev/null +++ b/internal/relay/checkouts.go @@ -0,0 +1,37 @@ +package relay + +import ( + "context" + "log/slog" + + "github.com/lichdotdev/lich/api" + "github.com/lichdotdev/lich/internal/store" +) + +// AgentCheckouts is every checkout a live provider session runs in, each once: +// the ones the pull request watch (internal/project) reads every tick, since a +// pull request opens and merges there with no window looking. A workspace that +// fails to load follows none until the next call. +func (s *Service) AgentCheckouts() []string { + projects, err := s.sessions.LoadState(context.Background(), api.None{}) + if err != nil { + slog.Warn("relay: read sessions for the pull request watch", "err", err) + return nil + } + return s.agentCheckouts(projects) +} + +func (s *Service) agentCheckouts(projects []store.Project) []string { + seen := map[string]bool{} + var checkouts []string + for _, p := range projects { + for _, sess := range p.Sessions { + c := sessionCwd(sess.Path, p.Path) + if !seen[c] && s.runsAgent(sess.ID, sess.Kind) { + seen[c] = true + checkouts = append(checkouts, c) + } + } + } + return checkouts +} diff --git a/internal/relay/checkouts_test.go b/internal/relay/checkouts_test.go new file mode 100644 index 000000000..6c5e82b7f --- /dev/null +++ b/internal/relay/checkouts_test.go @@ -0,0 +1,32 @@ +package relay + +import ( + "errors" + "slices" + "testing" +) + +func TestAgentCheckoutsAreTheOnesALiveAgentRunsIn(t *testing.T) { + t.Parallel() + // "shell" and "parked" are in /wt/feat with "agent"; "root" runs in the + // project's own directory; "other" makes /wt/fix live. + svc := newRelay(merging(), newFakeTerminal("root", "agent", "shell", "other"), nil) + + got := svc.AgentCheckouts() + if want := []string{"/src/lich", "/wt/feat", "/wt/fix"}; !slices.Equal(got, want) { + t.Fatalf("AgentCheckouts = %v, want %v", got, want) + } + + svc = newRelay(merging(), newFakeTerminal("shell"), nil) + if got := svc.AgentCheckouts(); len(got) != 0 { + t.Fatalf("AgentCheckouts with only a shell live = %v, want none", got) + } +} + +func TestAgentCheckoutsFollowNoneWhenTheWorkspaceFailsToLoad(t *testing.T) { + t.Parallel() + svc := newRelay(fakeSessions{err: errors.New("disk gone")}, newFakeTerminal(), nil) + if got := svc.AgentCheckouts(); got != nil { + t.Fatalf("AgentCheckouts = %v, want nil", got) + } +} diff --git a/internal/relay/merged.go b/internal/relay/merged.go index d7b82a45c..c2c6982f7 100644 --- a/internal/relay/merged.go +++ b/internal/relay/merged.go @@ -12,8 +12,8 @@ import ( ) // AnnounceMerge tells every agent running in opts.Checkout that the pull request on -// its branch was merged, from lich's own screen or, as the merge watch finds, -// anywhere else (mergewatch.go). A merge already announced is not announced +// its branch was merged, from lich's own screen or, as the pull request watch +// finds, anywhere else (internal/project, branchpr.go). A merge already announced is not announced // again. An agent keeps notes on the work // it did there, and without this the last thing they say about it is that it is // waiting on a merge, which a later session reads back as true long after the @@ -53,7 +53,7 @@ func (s *Service) AnnounceMerge(ctx context.Context, opts api.AnnounceMergeOptio // firstToldOf records the merge of pull request number at checkout as // announced and answers whether it was not yet: the Pulls screen announces its -// own merge and the merge watch reads it a moment later, in either order. +// own merge and the pull request watch reads it a moment later, in either order. func (s *Service) firstToldOf(checkout string, number int) bool { key := fmt.Sprint(checkout, "\x00", number) s.mu.Lock() diff --git a/internal/relay/merged_test.go b/internal/relay/merged_test.go index fe56cc098..3540e2780 100644 --- a/internal/relay/merged_test.go +++ b/internal/relay/merged_test.go @@ -132,3 +132,32 @@ func TestMergeNoticeNamesThePullRequestAndAsksForNoWork(t *testing.T) { t.Errorf("notice %q carries an escape out of the title", got) } } + +// The Pulls screen announces its own merge and the pull request watch reads it +// merged a moment later: the agents hear it once. +func TestAnnounceMergeTellsAMergeOnce(t *testing.T) { + t.Parallel() + term := newFakeTerminal("agent") + svc := newRelay(merging(), term, nil) + opts := api.AnnounceMergeOptions{Checkout: "/wt/feat", Number: 42, Title: "Add merge notice", Branch: "feat/merge", Base: "main"} + + for range 2 { + if _, err := svc.AnnounceMerge(t.Context(), opts); err != nil { + t.Fatalf("AnnounceMerge: %v", err) + } + } + + if !awaitWritten(term, "agent", "#42") { + t.Fatalf("typed at agent = %q, want the merge notice", term.written("agent")) + } + opts.Number = 43 + if _, err := svc.AnnounceMerge(t.Context(), opts); err != nil { + t.Fatalf("AnnounceMerge: %v", err) + } + if !awaitWritten(term, "agent", "#43") { + t.Fatalf("typed at agent = %q, want the second pull request's notice", term.written("agent")) + } + if got := strings.Count(term.written("agent"), "#42"); got != 1 { + t.Fatalf("agent heard #42 %d times, want once", got) + } +} diff --git a/internal/relay/mergewatch.go b/internal/relay/mergewatch.go deleted file mode 100644 index 35506b3a4..000000000 --- a/internal/relay/mergewatch.go +++ /dev/null @@ -1,94 +0,0 @@ -package relay - -import ( - "context" - "log/slog" - "time" - - "github.com/lichdotdev/lich/api" - "github.com/lichdotdev/lich/internal/store" -) - -// mergeWatchTick is how often the merge watch reads the pull request of every -// checkout an agent runs in: one gh call per checkout, for a notice that -// reads as fine a minute late. -const mergeWatchTick = time.Minute - -// prOpen and prMerged are the states gh reports a pull request in. -const ( - prOpen = "OPEN" - prMerged = "MERGED" -) - -// BranchPullRequest reads the pull request of the branch checked out at path, -// whatever its state, nil for none (internal/project implements it). -type BranchPullRequest func(path string) (*api.PRDetail, error) - -// RunMergeWatch announces a merge made outside lich (on github.com, with gh in -// a terminal, by an agent) the way the Pulls screen announces its own, until -// the process ends. Started once at launch: nothing tells lich of such a -// merge, so it reads every checkout an agent runs in each mergeWatchTick. -func (s *Service) RunMergeWatch(read BranchPullRequest) { - ticker := time.NewTicker(mergeWatchTick) - defer ticker.Stop() - for range ticker.C { - s.watchMerges(read) - } -} - -// watchMerges reads each checkout's pull request once and announces the ones -// that merged since the last read. A failed read (gh missing, not a GitHub -// repo) is left for the next tick and keeps what the last one saw. -func (s *Service) watchMerges(read BranchPullRequest) { - projects, err := s.sessions.LoadState(context.Background(), api.None{}) - if err != nil { - slog.Warn("relay: read sessions for the merge watch", "err", err) - return - } - for _, checkout := range s.agentCheckouts(projects) { - pr, err := read(checkout) - if err != nil { - slog.Debug("relay: read the pull request of a checkout", "checkout", checkout, "err", err) - continue - } - if !s.mergedSinceLastRead(checkout, pr) { - continue - } - opts := api.AnnounceMergeOptions{Checkout: checkout, Number: pr.Number, Title: pr.Title, Branch: pr.HeadRefName, Base: pr.BaseRefName} - if _, err := s.AnnounceMerge(context.Background(), opts); err != nil { - slog.Warn("relay: announce a merge made outside lich", "checkout", checkout, "err", err) - } - } -} - -// agentCheckouts is every checkout a live provider session runs in, each once. -func (s *Service) agentCheckouts(projects []store.Project) []string { - seen := map[string]bool{} - var checkouts []string - for _, p := range projects { - for _, sess := range p.Sessions { - c := sessionCwd(sess.Path, p.Path) - if !seen[c] && s.runsAgent(sess.ID, sess.Kind) { - seen[c] = true - checkouts = append(checkouts, c) - } - } - } - return checkouts -} - -// mergedSinceLastRead records the pull request read at checkout and answers -// whether it is the one the last read found open, now merged. One first read -// already merged is not announced: it may have merged long before lich -// started, and its agents have heard of it then or never will. -func (s *Service) mergedSinceLastRead(checkout string, pr *api.PRDetail) bool { - s.mu.Lock() - defer s.mu.Unlock() - was, open := s.openPRs[checkout] - if pr != nil && pr.State == prOpen { - s.openPRs[checkout] = pr.Number - return false - } - delete(s.openPRs, checkout) - return open && pr != nil && pr.State == prMerged && pr.Number == was -} diff --git a/internal/relay/mergewatch_test.go b/internal/relay/mergewatch_test.go deleted file mode 100644 index 4680b6c30..000000000 --- a/internal/relay/mergewatch_test.go +++ /dev/null @@ -1,145 +0,0 @@ -package relay - -import ( - "errors" - "strings" - "sync" - "testing" - - "github.com/lichdotdev/lich/api" -) - -// fakePRs answers the merge watch's reads with what each checkout's branch has -// at the moment, and counts the reads. -type fakePRs struct { - mu sync.Mutex - prs map[string]*api.PRDetail - err error - reads map[string]int -} - -func (f *fakePRs) set(checkout string, pr *api.PRDetail) { - f.mu.Lock() - defer f.mu.Unlock() - f.prs[checkout] = pr -} - -func (f *fakePRs) read(path string) (*api.PRDetail, error) { - f.mu.Lock() - defer f.mu.Unlock() - f.reads[path]++ - return f.prs[path], f.err -} - -func newFakePRs() *fakePRs { - return &fakePRs{prs: map[string]*api.PRDetail{}, reads: map[string]int{}} -} - -func featPR(state string) *api.PRDetail { - return &api.PRDetail{Number: 42, Title: "Add merge notice", State: state, HeadRefName: "feat/merge", BaseRefName: "main"} -} - -func TestMergeWatchAnnouncesAMergeMadeOutsideLich(t *testing.T) { - t.Parallel() - term := newFakeTerminal("agent") - svc := newRelay(merging(), term, nil) - prs := newFakePRs() - prs.set("/wt/feat", featPR(prOpen)) - - svc.watchMerges(prs.read) - prs.set("/wt/feat", featPR(prMerged)) - svc.watchMerges(prs.read) - - if !awaitWritten(term, "agent", "#42") { - t.Fatalf("typed at agent = %q, want the merge notice", term.written("agent")) - } - svc.watchMerges(prs.read) - if got := strings.Count(term.written("agent"), "#42"); got != 1 { - t.Fatalf("agent heard the notice %d times, want once", got) - } -} - -func TestMergeWatchReadsOnlyCheckoutsAnAgentRunsIn(t *testing.T) { - t.Parallel() - // "shell" and "parked" are in /wt/feat too; only "other" makes /wt/fix live. - term := newFakeTerminal("shell", "other") - svc := newRelay(merging(), term, nil) - prs := newFakePRs() - - svc.watchMerges(prs.read) - - if prs.reads["/wt/fix"] != 1 || prs.reads["/wt/feat"] != 0 || prs.reads["/src/lich"] != 0 { - t.Fatalf("reads = %v, want /wt/fix alone", prs.reads) - } -} - -func TestMergeWatchLeavesAPullRequestFirstSeenMerged(t *testing.T) { - t.Parallel() - term := newFakeTerminal("agent") - svc := newRelay(merging(), term, nil) - prs := newFakePRs() - prs.set("/wt/feat", featPR(prMerged)) - - svc.watchMerges(prs.read) - - if awaitWritten(term, "agent", "#42") { - t.Fatalf("typed at agent = %q, want nothing: it merged before the watch saw it open", term.written("agent")) - } -} - -func TestMergeWatchLeavesAClosedPullRequest(t *testing.T) { - t.Parallel() - term := newFakeTerminal("agent") - svc := newRelay(merging(), term, nil) - prs := newFakePRs() - prs.set("/wt/feat", featPR(prOpen)) - svc.watchMerges(prs.read) - prs.set("/wt/feat", featPR("CLOSED")) - svc.watchMerges(prs.read) - prs.set("/wt/feat", featPR(prMerged)) - svc.watchMerges(prs.read) - - if awaitWritten(term, "agent", "#42") { - t.Fatalf("typed at agent = %q, want nothing for a closed PR", term.written("agent")) - } -} - -func TestMergeWatchKeepsWhatItSawThroughAFailedRead(t *testing.T) { - t.Parallel() - term := newFakeTerminal("agent") - svc := newRelay(merging(), term, nil) - prs := newFakePRs() - prs.set("/wt/feat", featPR(prOpen)) - svc.watchMerges(prs.read) - prs.err = errors.New("gh is not signed in to GitHub.") - svc.watchMerges(prs.read) - prs.err = nil - prs.set("/wt/feat", featPR(prMerged)) - svc.watchMerges(prs.read) - - if !awaitWritten(term, "agent", "#42") { - t.Fatalf("typed at agent = %q, want the merge notice", term.written("agent")) - } -} - -func TestMergeWatchAndThePullsMergeAnnounceOnce(t *testing.T) { - t.Parallel() - term := newFakeTerminal("agent") - svc := newRelay(merging(), term, nil) - prs := newFakePRs() - prs.set("/wt/feat", featPR(prOpen)) - svc.watchMerges(prs.read) - - if _, err := svc.AnnounceMerge(t.Context(), api.AnnounceMergeOptions{Checkout: "/wt/feat", Number: 42, Title: "Add merge notice", Branch: "feat/merge", Base: "main"}); err != nil { - t.Fatalf("AnnounceMerge: %v", err) - } - prs.set("/wt/feat", featPR(prMerged)) - svc.watchMerges(prs.read) - - if !awaitWritten(term, "agent", "#42") { - t.Fatalf("typed at agent = %q, want the merge notice", term.written("agent")) - } - if got := strings.Count(term.written("agent"), "#42"); got != 1 { - t.Fatalf("agent heard the notice %d times, want once", got) - } -} diff --git a/internal/relay/relay.go b/internal/relay/relay.go index 721d66ab9..9865972b5 100644 --- a/internal/relay/relay.go +++ b/internal/relay/relay.go @@ -420,11 +420,8 @@ type Service struct { workerFinished func(workerID string) error // raiseWindow brings lich's window to the front (SetRaiseWindow). raiseWindow func(sessionID string) - // openPRs is the open pull request the merge watch last read at each - // checkout (mergewatch.go). - openPRs map[string]int // mergesTold is the merges already announced, by checkout and number, so - // the merge watch and the Pulls screen's merge never announce one twice. + // the pull request watch and the Pulls screen's merge never announce one twice. mergesTold map[string]bool } @@ -457,7 +454,6 @@ func New(sessions Sessions, term Terminal, events Events) *Service { collectors: make(map[string][]chan struct{}), nudgeTimer: make(map[string]*time.Timer), nudging: make(map[string]*sync.Mutex), - openPRs: make(map[string]int), mergesTold: make(map[string]bool), sessions: sessions, term: term, diff --git a/internal/routes/project.go b/internal/routes/project.go index 41b646e0e..e50f66e5c 100644 --- a/internal/routes/project.go +++ b/internal/routes/project.go @@ -57,7 +57,7 @@ func projectBranches(d *rpc.Handler, s *project.Service) { func projectPullRequests(d *rpc.Handler, s *project.Service) { rpc.Handle(d, api.ProjectGitHubAccounts, s.GitHubAccounts) rpc.Handle(d, api.ProjectIssue, s.Issue) - rpc.Handle(d, api.ProjectPullRequest, s.PullRequest) + rpc.Handle(d, api.ProjectRecheckPullRequest, s.RecheckPullRequest) rpc.Handle(d, api.ProjectListPullRequests, s.ListPullRequests) rpc.Handle(d, api.ProjectPullRequestDetail, s.PullRequestDetail) rpc.Handle(d, api.ProjectPullRequestConversation, s.PullRequestConversation) diff --git a/main.go b/main.go index e28700952..26b303d71 100644 --- a/main.go +++ b/main.go @@ -458,9 +458,12 @@ func registerServices(db *store.Service, term *terminal.Service, svcs *services. // The scheduled prompts are the relay's other clock: nobody calls in for // them, they come due. go rl.RunSchedules() - // A merge made outside lich reaches its agents through the relay's own - // read of their checkouts' pull requests. - go rl.RunMergeWatch(proj.BranchPullRequest) + // The pull request watch reads every checkout an agent runs in, and tells + // its agents of a merge made outside lich as the Pulls screen tells them + // of its own. + proj.SetAgentCheckouts(rl.AgentCheckouts) + proj.SetAnnounceMerge(rl.AnnounceMerge) + go proj.RunBranchPRWatch() // A turn a usage limit ended is picked up again once it resets, through // the same scheduled prompt. Codex reports no end for that turn, so the // terminal watches its open turns for one. diff --git a/native/backend/project.go b/native/backend/project.go index 20cc863d9..b5369d6a8 100644 --- a/native/backend/project.go +++ b/native/backend/project.go @@ -10,16 +10,19 @@ type BaseStatus = api.BaseStatus type PullRequest = api.PullRequest +type BranchPullRequest = api.BranchPullRequest + // BaseStatus returns where path stands against its base, nil when it has no // base to stand against (the base branch itself, no remote). func (c *Client) BaseStatus(ctx context.Context, path string) (*BaseStatus, error) { return callMethod(ctx, c, api.ProjectBaseStatus, api.PathOptions{Path: path}) } -// PullRequest returns the open pull request of path's branch through gh, nil -// when there is none or gh cannot answer. Each call is a network round trip. -func (c *Client) PullRequest(ctx context.Context, path string) (*PullRequest, error) { - return callMethod(ctx, c, api.ProjectPullRequest, api.PathOptions{Path: path}) +// RecheckPullRequest asks the backend to read the pull request of path's +// branch again; the answer arrives as api.BranchPullRequestEvent. +func (c *Client) RecheckPullRequest(ctx context.Context, path string) error { + _, err := callMethod(ctx, c, api.ProjectRecheckPullRequest, api.PathOptions{Path: path}) + return err } // OpenExternal opens url in the user's browser. diff --git a/native/cmd/lich-native/cardstate_test.go b/native/cmd/lich-native/cardstate_test.go index 2f65e1389..7daebd6a9 100644 --- a/native/cmd/lich-native/cardstate_test.go +++ b/native/cmd/lich-native/cardstate_test.go @@ -74,7 +74,7 @@ func fakeUnread(t *testing.T, calls chan<- []any) *backend.Client { case "/rpc/project.Home": _ = json.NewEncoder(w).Encode(backend.Project{ID: "home", Path: "/home/me"}) case "/rpc/store.AddProject", "/rpc/store.AddSession", - "/rpc/project.Diff", "/rpc/project.BaseStatus", "/rpc/project.PullRequest": + "/rpc/project.Diff", "/rpc/project.BaseStatus", "/rpc/project.RecheckPullRequest": _ = json.NewEncoder(w).Encode(nil) case "/rpc/store.SetSessionUnread": var args []api.SessionUnreadOptions diff --git a/native/cmd/lich-native/gitstate.go b/native/cmd/lich-native/gitstate.go index fb7593482..2a00cb18e 100644 --- a/native/cmd/lich-native/gitstate.go +++ b/native/cmd/lich-native/gitstate.go @@ -4,7 +4,6 @@ import ( "context" "log" "slices" - "time" "github.com/lichdotdev/lich/native/backend" ) @@ -34,25 +33,19 @@ func standingOf(b *backend.BaseStatus) baseStanding { return baseStanding{count: b.Behind} } -// prRecheck is how long an open pull request is believed before gh is asked -// again: a merge or close on GitHub moves neither branch nor head, so nothing -// else would ever make the badge go. -const prRecheck = time.Minute - -// prLookup is a path's pull request as asked for at a branch and head. The -// pr survives a new commit on the same branch, so the badge does not blink, +// prLookup is a path's pull request as last asked for at a branch and head. +// The pr survives a new commit on the same branch, so the badge does not blink, // and is dropped with a new branch. type prLookup struct { branch, head string pr *backend.PullRequest - asked time.Time } -// refreshPath re-reads a shown path's changes and base standing, and asks gh -// for its pull request again when the branch or head moved: a merge that -// closed one shows without waiting. An open one is asked about again every -// prRecheck; one the session just opened, at the end of its turn -// (recheckMissingPR). +// refreshPath re-reads a shown path's changes and base standing, and asks the +// backend to read its pull request again when the branch or head moved: a +// merge that closed one shows without waiting. The backend reads an open one +// again on its own (internal/project branchpr.go); one the session just opened, +// the window asks for at the end of its turn (recheckMissingPR). func (m *model) refreshPath(ctx context.Context, path string) { d, err := m.client.Diff(ctx, path) if err != nil { @@ -67,11 +60,9 @@ func (m *model) refreshPath(ctx context.Context, path string) { changed := m.git[path] != d || !sameBase(m.base[path], b) m.git[path], m.base[path] = d, b prev, looked := m.prs[path] - now := time.Now() - ask := !looked || prev.branch != d.Branch || prev.head != d.Head || - (prev.pr != nil && now.Sub(prev.asked) >= prRecheck) + ask := !looked || prev.branch != d.Branch || prev.head != d.Head if ask { - next := prLookup{branch: d.Branch, head: d.Head, asked: now} + next := prLookup{branch: d.Branch, head: d.Head} if prev.branch == d.Branch { next.pr = prev.pr } @@ -80,35 +71,40 @@ func (m *model) refreshPath(ctx context.Context, path string) { } m.mu.Unlock() if ask { - go m.lookupPR(ctx, path, d.Branch, d.Head) + go m.recheckPR(ctx, path) } if changed { m.invalidate() } } -// lookupPR stores gh's answer unless the checkout moved on while it was -// asked. A failed lookup (no gh, not a GitHub repo) reads as no PR. -func (m *model) lookupPR(ctx context.Context, path, branch, head string) { - pr, err := m.client.PullRequest(ctx, path) - if err != nil { - log.Printf("pull request of %s: %v", path, err) +func (m *model) recheckPR(ctx context.Context, path string) { + if err := m.client.RecheckPullRequest(ctx, path); err != nil { + log.Printf("recheck the pull request of %s: %v", path, err) + } +} + +// setBranchPR keeps a read of a shown path's pull request; the badge shows only +// an open one. A read of a path no card shows is dropped: the window asks for +// one when it first shows it. +func (m *model) setBranchPR(b backend.BranchPullRequest) { + pr := b.PR + if pr != nil && pr.State != "OPEN" { pr = nil } m.mu.Lock() - cur := m.prs[path] - current := cur.branch == branch && cur.head == head - if current { - cur.pr = pr - m.prs[path] = cur + l, shown := m.prs[b.Path] + if shown { + l.pr = pr + m.prs[b.Path] = l } m.mu.Unlock() - if current { + if shown { m.invalidate() } } -// stalePullRequests makes the next git tick ask gh again for every path, +// stalePullRequests makes the next git tick ask for a new read of every path, // keeping the badges up meanwhile: a PR opened or merged in the browser // shows when the user comes back. Callers hold m.mu. func (m *model) stalePullRequests() { @@ -118,7 +114,7 @@ func (m *model) stalePullRequests() { } } -// recheckMissingPR forgets what gh answered for the card of a session whose +// recheckMissingPR forgets what was read for the card of a session whose // turn just finished, when it shows no pull request: `gh pr create` moves // neither branch nor head, so nothing else would bring up the PR the turn // opened. It answers the path to refresh, "" when there is none to ask about diff --git a/native/cmd/lich-native/gitstate_test.go b/native/cmd/lich-native/gitstate_test.go index 31c5a286a..d2430ed52 100644 --- a/native/cmd/lich-native/gitstate_test.go +++ b/native/cmd/lich-native/gitstate_test.go @@ -32,15 +32,17 @@ func TestStandingOf(t *testing.T) { } } -// fakeForge serves a checkout whose branch and head the test moves, with the -// PR gh would answer at each head, so an answer never depends on when the -// lookup lands. +// fakeForge serves a checkout whose branch and head the test moves, and plays +// the backend's pull request watch: a recheck is answered with a +// BranchPullRequestEvent carrying the PR at the head when the read lands, so +// an answer never depends on when the recheck was asked. type fakeForge struct { mu sync.Mutex branch, head string - prs map[string]*backend.PullRequest // head -> open PR - hold chan struct{} // while set, gh does not answer - answered int // pull request lookups served + prs map[string]*backend.PullRequest // head -> PR + hold chan struct{} // while set, reads do not land + answered int // reads landed + m *model } func (f *fakeForge) set(branch, head string) { @@ -49,14 +51,28 @@ func (f *fakeForge) set(branch, head string) { f.branch, f.head = branch, head } +// watch is the model the reads land at. +func (f *fakeForge) watch(m *model) *model { + f.mu.Lock() + defer f.mu.Unlock() + f.m = m + return m +} + +// read lands a recheck of path once hold lets it. +func (f *fakeForge) read(path string, hold chan struct{}) { + if hold != nil { + <-hold + } + f.mu.Lock() + pr, m := f.prs[f.head], f.m + f.answered++ + f.mu.Unlock() + send(m, api.BranchPullRequestEvent.Name, api.BranchPullRequest{Path: path, PR: pr}) +} + func (f *fakeForge) client(t *testing.T) *backend.Client { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - f.mu.Lock() - hold := f.hold - f.mu.Unlock() - if hold != nil && r.URL.Path == "/rpc/project.PullRequest" { - <-hold - } f.mu.Lock() defer f.mu.Unlock() switch r.URL.Path { @@ -64,9 +80,11 @@ func (f *fakeForge) client(t *testing.T) *backend.Client { _ = json.NewEncoder(w).Encode(backend.DiffStats{Branch: f.branch, Head: f.head}) case "/rpc/project.BaseStatus": _ = json.NewEncoder(w).Encode(backend.BaseStatus{Behind: 2}) - case "/rpc/project.PullRequest": - _ = json.NewEncoder(w).Encode(f.prs[f.head]) - f.answered++ + case "/rpc/project.RecheckPullRequest": + var args []api.PathOptions + _ = json.NewDecoder(r.Body).Decode(&args) + go f.read(args[0].Path, f.hold) + _ = json.NewEncoder(w).Encode(nil) default: t.Errorf("unexpected call %s", r.URL.Path) } @@ -105,11 +123,11 @@ func awaitPROnCard(t *testing.T, m *model, want int) { func TestCardFollowsThePullRequestOfItsBranch(t *testing.T) { forge := &fakeForge{prs: map[string]*backend.PullRequest{ - "c2": {Number: 7}, - "c4": {Number: 9}, + "c2": {Number: 7, State: "OPEN"}, + "c4": {Number: 9, State: "OPEN"}, }} forge.set("feat", "c1") - m := newTestModel(forge.client(t)) + m := forge.watch(newTestModel(forge.client(t))) prOnCard(t, m, 0) if b := standingOf(cardOf(t, m).base); b != (baseStanding{count: 2}) { t.Fatalf("base %+v, want 2 behind", b) @@ -121,10 +139,10 @@ func TestCardFollowsThePullRequestOfItsBranch(t *testing.T) { forge.mu.Lock() forge.hold = make(chan struct{}) forge.mu.Unlock() - forge.set("feat", "c3") // a new commit: the badge stays while gh is asked + forge.set("feat", "c3") // a new commit: the badge stays while the PR is read m.refreshGit(context.Background()) if pr := cardOf(t, m).pr; pr == nil || pr.Number != 7 { - t.Fatalf("new commit on the branch: PR %+v before gh answers, want #7 kept", pr) + t.Fatalf("new commit on the branch: PR %+v before the read lands, want #7 kept", pr) } forge.mu.Lock() close(forge.hold) @@ -132,7 +150,7 @@ func TestCardFollowsThePullRequestOfItsBranch(t *testing.T) { forge.mu.Unlock() prOnCard(t, m, 0) // c3 has no open PR: merged - forge.prs["c3"] = &backend.PullRequest{Number: 8} + forge.prs["c3"] = &backend.PullRequest{Number: 8, State: "OPEN"} m.mu.Lock() m.stalePullRequests() // the window came back: opened in the browser m.mu.Unlock() @@ -144,7 +162,7 @@ func TestCardFollowsThePullRequestOfItsBranch(t *testing.T) { forge.set("other", "c4") m.refreshGit(context.Background()) if pr := cardOf(t, m).pr; pr != nil { - t.Fatalf("new branch: PR #%d before gh answers, want the old branch's dropped", pr.Number) + t.Fatalf("new branch: PR #%d before the read lands, want the old branch's dropped", pr.Number) } forge.mu.Lock() close(forge.hold) @@ -158,7 +176,7 @@ func TestCardFollowsThePullRequestOfItsBranch(t *testing.T) { func TestCardFindsThePullRequestItsTurnOpened(t *testing.T) { forge := &fakeForge{prs: map[string]*backend.PullRequest{}} forge.set("feat", "c1") - m := newTestModel(forge.client(t)) + m := forge.watch(newTestModel(forge.client(t))) prOnCard(t, m, 0) for deadline := time.Now().Add(5 * time.Second); ; time.Sleep(10 * time.Millisecond) { forge.mu.Lock() @@ -168,12 +186,12 @@ func TestCardFindsThePullRequestItsTurnOpened(t *testing.T) { break } if time.Now().After(deadline) { - t.Fatal("gh was never asked about c1") + t.Fatal("the PR at c1 was never read") } } forge.mu.Lock() - forge.prs["c1"] = &backend.PullRequest{Number: 5} + forge.prs["c1"] = &backend.PullRequest{Number: 5, State: "OPEN"} forge.mu.Unlock() send(m, api.SessionStatusEvent.Name, statusEvent{ID: "s1", State: statusBusy}) send(m, api.SessionStatusEvent.Name, statusEvent{ID: "s1", State: statusDone}) diff --git a/native/cmd/lich-native/model.go b/native/cmd/lich-native/model.go index bc1a2f110..765823608 100644 --- a/native/cmd/lich-native/model.go +++ b/native/cmd/lich-native/model.go @@ -373,6 +373,10 @@ func (m *model) follow(ctx context.Context, events <-chan backend.Event) { if m.prChanged != nil { m.prChanged() } + case api.BranchPullRequestEvent.Name: + if b, err := backend.Decode(ev, api.BranchPullRequestEvent); err == nil { + m.setBranchPR(b) + } case api.AppUpdateProgressEvent.Name: if p, err := backend.Decode(ev, api.AppUpdateProgressEvent); err == nil && m.appUpdateProgress != nil { m.appUpdateProgress(p) diff --git a/native/cmd/lich-native/model_test.go b/native/cmd/lich-native/model_test.go index 2b5fb36b9..924421098 100644 --- a/native/cmd/lich-native/model_test.go +++ b/native/cmd/lich-native/model_test.go @@ -28,7 +28,7 @@ func fakeDiff(t *testing.T, notGit string) *backend.Client { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var args []api.PathOptions body, _ := io.ReadAll(r.Body) - if r.URL.Path == "/rpc/project.BaseStatus" || r.URL.Path == "/rpc/project.PullRequest" { + if r.URL.Path == "/rpc/project.BaseStatus" || r.URL.Path == "/rpc/project.RecheckPullRequest" { _ = json.NewEncoder(w).Encode(nil) return } diff --git a/native/cmd/lich-native/pulls.go b/native/cmd/lich-native/pulls.go index 7ff89b7a7..0b06387d7 100644 --- a/native/cmd/lich-native/pulls.go +++ b/native/cmd/lich-native/pulls.go @@ -160,6 +160,10 @@ func (p *pullsScreen) setFocused(focused bool) { p.focused = focused } +// prRecheck is how long the Pulls screen believes what it shows before reading +// it again. +const prRecheck = time.Minute + // recheck owes what the screen shows a newer answer once prRecheck has passed // since the last one: a push or a merge made outside lich announces nothing. // It answers when the next one is due. diff --git a/native/cmd/lich-native/stores_events_test.go b/native/cmd/lich-native/stores_events_test.go index b238c710c..4da9b99d6 100644 --- a/native/cmd/lich-native/stores_events_test.go +++ b/native/cmd/lich-native/stores_events_test.go @@ -67,7 +67,7 @@ func TestSessionTouchedReadsGitNowAndNudgesTheDiff(t *testing.T) { switch method { case "project.Diff": return backend.DiffStats{Branch: "main", Head: head}, http.StatusOK - case "project.BaseStatus", "project.PullRequest": + case "project.BaseStatus", "project.RecheckPullRequest": return nil, http.StatusOK } return failing("unexpected " + method) @@ -102,7 +102,7 @@ func TestSelectingACardReadsItsGitWithoutATick(t *testing.T) { switch method { case "project.Diff": return backend.DiffStats{Branch: "main", Head: head}, http.StatusOK - case "project.BaseStatus", "project.PullRequest": + case "project.BaseStatus", "project.RecheckPullRequest": return nil, http.StatusOK } return failing("unexpected " + method) @@ -220,7 +220,7 @@ func TestGitStoreKeepsTheLastReadingThroughATransientFailure(t *testing.T) { return backend.DiffStats{Branch: "main", Head: "h"}, http.StatusOK case "project.Exists": return exists, http.StatusOK - case "project.BaseStatus", "project.PullRequest": + case "project.BaseStatus", "project.RecheckPullRequest": return nil, http.StatusOK } return failing("unexpected " + method) @@ -245,53 +245,55 @@ func TestGitStoreKeepsTheLastReadingThroughATransientFailure(t *testing.T) { } } -func TestAnOpenPullRequestIsAskedAgainOnASlowCadence(t *testing.T) { - forge := &fakeForge{prs: map[string]*backend.PullRequest{"c1": {Number: 7}}} +// A merge on GitHub moves neither branch nor head: the backend's own read of +// the open pull request is what takes the badge down. +func TestAMergeTheBackendReadsTakesTheBadgeDown(t *testing.T) { + forge := &fakeForge{prs: map[string]*backend.PullRequest{"c1": {Number: 7, State: "OPEN"}}} forge.set("feat", "c1") - m := newTestModel(forge.client(t)) + m := forge.watch(newTestModel(forge.client(t))) prOnCard(t, m, 7) - forge.mu.Lock() - forge.prs["c1"] = nil // merged on GitHub: branch and head did not move - forge.mu.Unlock() - m.refreshGit(context.Background()) - time.Sleep(50 * time.Millisecond) - if pr := cardOf(t, m).pr; pr == nil || pr.Number != 7 { - t.Fatalf("PR %+v before the cadence elapsed, want #7 kept", pr) - } + send(m, api.BranchPullRequestEvent.Name, api.BranchPullRequest{Path: checkout, PR: &backend.PullRequest{Number: 7, State: "MERGED"}}) + awaitPROnCard(t, m, 0) + send(m, api.BranchPullRequestEvent.Name, api.BranchPullRequest{Path: elsewhere, PR: &backend.PullRequest{Number: 9, State: "OPEN"}}) m.mu.Lock() - l := m.prs[checkout] - l.asked = l.asked.Add(-2 * prRecheck) - m.prs[checkout] = l + _, kept := m.prs[elsewhere] m.mu.Unlock() - prOnCard(t, m, 0) + if kept { + t.Error("a read of a path no card shows was kept") + } } -func TestAPathWithNoPullRequestIsNotAskedAgainByTheCadence(t *testing.T) { +func TestAPathIsRecheckedOnlyWhenItsBranchOrHeadMoves(t *testing.T) { + var mu sync.Mutex + head := "h1" stub := &rpcStub{answer: func(method string, _ []json.RawMessage) (any, int) { + mu.Lock() + defer mu.Unlock() switch method { case "project.Diff": - return backend.DiffStats{Branch: "main", Head: "h"}, http.StatusOK - case "project.BaseStatus", "project.PullRequest": + return backend.DiffStats{Branch: "main", Head: head}, http.StatusOK + case "project.BaseStatus", "project.RecheckPullRequest": return nil, http.StatusOK } return failing("unexpected " + method) }} m := newTestModel(stub.client(t)) m.refreshGit(context.Background()) - eventually(t, "the first lookup", func() bool { return stub.calls("project.PullRequest") == 1 }) + eventually(t, "the first recheck", func() bool { return stub.calls("project.RecheckPullRequest") == 1 }) - m.mu.Lock() - l := m.prs[checkout] - l.asked = l.asked.Add(-2 * prRecheck) - m.prs[checkout] = l - m.mu.Unlock() m.refreshGit(context.Background()) time.Sleep(50 * time.Millisecond) - if n := stub.calls("project.PullRequest"); n != 1 { - t.Errorf("gh asked %d times for a branch with no PR, want 1", n) + if n := stub.calls("project.RecheckPullRequest"); n != 1 { + t.Errorf("rechecked %d times with nothing moved, want 1", n) } + + mu.Lock() + head = "h2" + mu.Unlock() + m.refreshGit(context.Background()) + eventually(t, "the recheck the new head owes", func() bool { return stub.calls("project.RecheckPullRequest") == 2 }) } func TestALoginEndingReloadsTheProfilesBeforeTheScreenHearsIt(t *testing.T) {