From a3e5947ba9240be3c38567214e5ecf9da72bdeb7 Mon Sep 17 00:00:00 2001 From: Jacob Atchley Date: Mon, 21 Sep 2026 11:34:25 -0500 Subject: [PATCH 1/3] feat: open a pull request's build gate runs from the pull request Seeing whether a pull request's CI gate passed meant leaving boardwalk, or scrolling the project's build list for a run that may have aged off it. B on an opened pull request reads its policy evaluations, keeps the build validation policies that have produced a run, and opens the build list over those runs: enter reads the failing gate's logs, Q re-runs it. Policy evaluations rather than a branch search is what makes these the gates rather than whatever happened to build the branch; a gate with no run yet is left out, and a pull request nothing gates says so instead of opening empty. The gate list is the first second instance of a list view in one stack, so it does not rely on the invariant the ErrMsg broadcast depends on: it fetches through gateRunsMsg, which names its pull request, ignores the project listing, and reports its own failures rather than emitting an unowned ErrMsg. TestListViewsAreConstructedOnlyByRootBuild is narrowed to that one exception. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EnkNZwrGWfCeQDsHVkvPuE --- README.md | 12 +++ internal/azdo/policies.go | 77 ++++++++++++++++++ internal/azdo/policies_test.go | 82 +++++++++++++++++++ internal/azdo/pullrequests.go | 13 ++- internal/azdo/pullrequests_test.go | 17 ++++ internal/ui/builds.go | 100 ++++++++++++++++++++++- internal/ui/builds_test.go | 110 ++++++++++++++++++++++++++ internal/ui/keys.go | 17 ++-- internal/ui/pullrequestdetail.go | 100 ++++++++++++++++++++++- internal/ui/pullrequestdetail_test.go | 68 ++++++++++++++++ internal/ui/root_test.go | 20 +++-- 11 files changed, 599 insertions(+), 17 deletions(-) create mode 100644 internal/azdo/policies.go create mode 100644 internal/azdo/policies_test.go diff --git a/README.md b/README.md index 82dc15b..37656bd 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,7 @@ listed newest first. | key | | |---|---| | `enter` | open the full pull request | +| `B` | from an opened pull request, the runs its build gates produced | | `v` | only pull requests waiting on your review | | `d` | cycle drafts hidden → drafts only → all | | `P` | publish a draft, or put a published pull request back into draft | @@ -205,6 +206,17 @@ first. `w` opens the first linked work item. `c` replies to the first unresolved thread and `R` resolves it, both without leaving the view. `D` opens the diff. +`B` opens the runs behind the pull request's build gates — the build list +again, over those runs rather than the project's recent ones, so `enter` +reads the failing gate's logs and `Q` re-runs it from there. The runs come +from the pull request's policy evaluations rather than from a branch search, +which is what makes them the gates rather than whatever happened to build +that branch: a gate that has not started a run yet is left out, and a pull +request nothing gates says so on the status line instead of opening an empty +list. The status line carries the policy's own verdict over with it, because +it is not the build's — a rejected gate over a green run means the policy was +evaluated against an earlier iteration. + `A` approves, `W` waits for the author and `X` rejects, each on a second press of the same key. They show when the pull request is yours to vote on — named directly, or through a group of yours, whether it was resolved at startup or diff --git a/internal/azdo/policies.go b/internal/azdo/policies.go new file mode 100644 index 0000000..e362caf --- /dev/null +++ b/internal/azdo/policies.go @@ -0,0 +1,77 @@ +package azdo + +import ( + "fmt" + "net/url" +) + +// buildPolicyTypeID is Azure DevOps's identifier for the build validation +// policy. The evaluations endpoint returns every policy on a pull request — +// reviewer counts, linked work items, comment resolution — and this is the one +// that runs a pipeline, so it is the only one that can name a build. +const buildPolicyTypeID = "0609b952-1397-4640-95ec-e00a01b2c241" + +// unnamedPolicy is what a build validation with no display name is called. A +// policy is named when it is configured and most are, but an unnamed one still +// gates the pull request and still has a run worth opening. +const unnamedPolicy = "build validation" + +// GateBuild is one build validation policy on a pull request, and the run it +// produced. Status is the policy's own verdict — queued, running, approved, +// rejected — which is not the same as the build's: a rejected gate whose build +// succeeded means the policy was evaluated against an older iteration. +type GateBuild struct { + BuildID int + Policy string + Status string +} + +// PullRequestGateBuilds lists the runs the build validation policies on pr +// have produced, newest evaluation first as the endpoint returns them. +// +// A policy with no run yet is left out rather than returned with a zero id: +// the caller's whole purpose is opening the build, and a row that cannot be +// opened is worse than a shorter list that says so. +func (c *Client) PullRequestGateBuilds(pr PullRequest) ([]GateBuild, error) { + if pr.ProjectID == "" { + return nil, fmt.Errorf("!%d does not say which project its repository belongs to, so its build gates cannot be looked up", pr.ID) + } + + var resp struct { + Value []struct { + Configuration struct { + Type struct { + ID string `json:"id"` + } `json:"type"` + Settings struct { + DisplayName string `json:"displayName"` + } `json:"settings"` + } `json:"configuration"` + Status string `json:"status"` + Context struct { + BuildID int `json:"buildId"` + } `json:"context"` + } `json:"value"` + } + + artifact := fmt.Sprintf("vstfs:///CodeReview/CodeReviewId/%s/%d", pr.ProjectID, pr.ID) + endpoint := fmt.Sprintf("%s/%s/%s/_apis/policy/evaluations?artifactId=%s&api-version=%s", + c.root(), url.PathEscape(c.Org), url.PathEscape(c.Project), + url.QueryEscape(artifact), APIVersion) + if err := c.get(endpoint, &resp); err != nil { + return nil, err + } + + var gates []GateBuild + for _, v := range resp.Value { + if v.Configuration.Type.ID != buildPolicyTypeID || v.Context.BuildID == 0 { + continue + } + name := v.Configuration.Settings.DisplayName + if name == "" { + name = unnamedPolicy + } + gates = append(gates, GateBuild{BuildID: v.Context.BuildID, Policy: name, Status: v.Status}) + } + return gates, nil +} diff --git a/internal/azdo/policies_test.go b/internal/azdo/policies_test.go new file mode 100644 index 0000000..8e58c12 --- /dev/null +++ b/internal/azdo/policies_test.go @@ -0,0 +1,82 @@ +package azdo + +import ( + "net/http" + "strings" + "testing" +) + +// gateBody is one pull request's policy evaluations: two build validations — +// one that has produced a run and one still queued behind the policy itself — +// and a reviewer policy that is not a build at all. +const gateBody = `{"value":[ + {"configuration":{"type":{"id":"0609b952-1397-4640-95ec-e00a01b2c241","displayName":"Build"}, + "settings":{"displayName":"CI gate","buildDefinitionId":12}}, + "status":"rejected","context":{"buildId":9001}}, + {"configuration":{"type":{"id":"0609b952-1397-4640-95ec-e00a01b2c241","displayName":"Build"}, + "settings":{"displayName":"Nightly gate","buildDefinitionId":13}}, + "status":"queued","context":{}}, + {"configuration":{"type":{"id":"fa4e907d-c16b-4a4c-9dfa-4906e5d171dd","displayName":"Minimum number of reviewers"}, + "settings":{"minimumApproverCount":2}}, + "status":"approved","context":{}} +]}` + +func TestPullRequestGateBuildsKeepsTheBuildsAPolicyHasRun(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + want := "vstfs:///CodeReview/CodeReviewId/p-guid/512" + if got := r.URL.Query().Get("artifactId"); got != want { + t.Errorf("artifactId = %q, want %q", got, want) + } + if !strings.Contains(r.URL.Path, "/_apis/policy/evaluations") { + t.Errorf("path = %q, want the policy evaluations endpoint", r.URL.Path) + } + w.Write([]byte(gateBody)) + }) + + gates, err := c.PullRequestGateBuilds(PullRequest{ID: 512, ProjectID: "p-guid"}) + if err != nil { + t.Fatalf("PullRequestGateBuilds returned %v", err) + } + if len(gates) != 1 { + t.Fatalf("got %d gates, want only the one with a build", len(gates)) + } + got := gates[0] + if got.BuildID != 9001 { + t.Errorf("build id = %d, want 9001", got.BuildID) + } + if got.Policy != "CI gate" { + t.Errorf("policy = %q, want CI gate", got.Policy) + } + if got.Status != "rejected" { + t.Errorf("status = %q, want rejected", got.Status) + } +} + +func TestPullRequestGateBuildsNamesAnUnnamedPolicy(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"value":[ + {"configuration":{"type":{"id":"0609b952-1397-4640-95ec-e00a01b2c241","displayName":"Build"},"settings":{}}, + "status":"approved","context":{"buildId":42}}]}`)) + }) + + gates, err := c.PullRequestGateBuilds(PullRequest{ID: 7, ProjectID: "p-guid"}) + if err != nil { + t.Fatalf("PullRequestGateBuilds returned %v", err) + } + if len(gates) != 1 { + t.Fatalf("got %d gates, want 1", len(gates)) + } + if gates[0].Policy != "build validation" { + t.Errorf("policy = %q, want the fallback name", gates[0].Policy) + } +} + +func TestPullRequestGateBuildsWithoutAProjectIDSaysSo(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + t.Error("the evaluations endpoint was called without a project id") + }) + + if _, err := c.PullRequestGateBuilds(PullRequest{ID: 512}); err == nil { + t.Fatal("PullRequestGateBuilds returned no error for a pull request with no project id") + } +} diff --git a/internal/azdo/pullrequests.go b/internal/azdo/pullrequests.go index 558a15e..77e67e8 100644 --- a/internal/azdo/pullrequests.go +++ b/internal/azdo/pullrequests.go @@ -63,6 +63,11 @@ type PullRequest struct { Title string Repo string RepoID string + // ProjectID is the GUID of the team project the repository belongs to. + // The project is known by name everywhere else in boardwalk, but the + // policy endpoint addresses a pull request by an artifact id built from + // the GUID, and the name will not do there. + ProjectID string Author string AuthorKey string // uniqueName, lowercased, compared against Client.Me IsDraft bool @@ -193,8 +198,11 @@ type pullRequestJSON struct { UniqueName string `json:"uniqueName"` } `json:"createdBy"` Repository struct { - ID string `json:"id"` - Name string `json:"name"` + ID string `json:"id"` + Name string `json:"name"` + Project struct { + ID string `json:"id"` + } `json:"project"` } `json:"repository"` LastMergeSourceCommit struct { CommitID string `json:"commitId"` @@ -217,6 +225,7 @@ func (v pullRequestJSON) pullRequest() PullRequest { Title: v.Title, Repo: v.Repository.Name, RepoID: v.Repository.ID, + ProjectID: v.Repository.Project.ID, Author: v.CreatedBy.DisplayName, AuthorKey: strings.ToLower(v.CreatedBy.UniqueName), IsDraft: v.IsDraft, diff --git a/internal/azdo/pullrequests_test.go b/internal/azdo/pullrequests_test.go index f94262a..7652487 100644 --- a/internal/azdo/pullrequests_test.go +++ b/internal/azdo/pullrequests_test.go @@ -602,3 +602,20 @@ func TestCreateThreadReportsTheServersRefusal(t *testing.T) { t.Errorf("error = %v, want the server's own message in it", err) } } + +func TestPullRequestParsesTheProjectItsRepositoryBelongsTo(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"value":[ + {"pullRequestId":512,"title":"Retry webhooks","sourceRefName":"refs/heads/x","targetRefName":"refs/heads/main", + "creationDate":"2026-09-10T09:00:00Z","createdBy":{"displayName":"Dev","uniqueName":"dev@acme.test"}, + "repository":{"id":"r1","name":"platform-api","project":{"id":"p-guid","name":"Platform"}}}]}`)) + }) + + prs, err := c.PullRequests() + if err != nil { + t.Fatalf("PullRequests returned %v", err) + } + if prs[0].ProjectID != "p-guid" { + t.Errorf("project id = %q, want p-guid", prs[0].ProjectID) + } +} diff --git a/internal/ui/builds.go b/internal/ui/builds.go index 86e888d..ebc241b 100644 --- a/internal/ui/builds.go +++ b/internal/ui/builds.go @@ -79,6 +79,19 @@ type timelineMsg struct { Err error } +// gateRunsMsg carries the runs behind one pull request's build gates. It is +// its own message rather than a buildsMsg because both listings can be alive +// at once — the project's recent runs underneath, one pull request's gates on +// top — and Root broadcasts data to every view in the stack. It names the +// pull request for the same reason timelineMsg names its build, and carries +// Err rather than emitting ErrMsg, which has no owner to route on and would +// be read by the project list as its own fetch failing. +type gateRunsMsg struct { + PR int + Builds []azdo.Build + Err error +} + // buildPRMsg carries the outcome of looking up the pull request a build ran // for. It names the build for the same reason timelineMsg does: pressing p // again for a different build before the first lookup answers must not let @@ -116,6 +129,16 @@ type Builds struct { queueDefinition int queueLabel string + // prID and gateBuilds are set when this list was opened from a pull + // request's build gates rather than from the menu: the pull request it + // belongs to, and the runs its gates produced. gateBuilds is what Init + // fetches instead of the project's recent runs, and it is fixed for the + // life of the view — a gate that starts a fresh run after this list + // opened is picked up by reopening it from the pull request, which is + // where the policy evaluations are read. + prID int + gateBuilds []int + // linkBuild is the id of the build a pull request lookup is in flight // for, or 0 when none is. It is how a late answer knows whether it is // still the one asked about — see buildPRMsg. @@ -156,8 +179,23 @@ func NewBuilds(c *azdo.Client) *Builds { return m } -// Init fetches the project's recent runs. +// NewBuildsForPullRequest opens the same browser over one pull request's gate +// runs. The ids come from the policy evaluations the pull request view read, +// so this fetches each run by id rather than filtering the project's recent +// ones: a gate that ran days ago is still the gate on this pull request, and +// would have fallen off the end of a fixed-size listing. +func NewBuildsForPullRequest(c *azdo.Client, prID int, ids []int) *Builds { + m := NewBuilds(c) + m.prID, m.gateBuilds = prID, ids + return m +} + +// Init fetches the project's recent runs, or the gate runs this list was +// opened for. func (m *Builds) Init() tea.Cmd { + if len(m.gateBuilds) > 0 { + return tea.Batch(m.fetchGateBuilds(), m.work.begin(1)) + } client := m.client fetch := func() tea.Msg { builds, err := client.Builds(buildPageSize) @@ -169,6 +207,30 @@ func (m *Builds) Init() tea.Cmd { return tea.Batch(fetch, m.work.begin(1)) } +// fetchGateBuilds reads each gate run by id. They are fetched in one command +// rather than one each so the list appears whole: a handful of gates is a +// short serial walk, and rows arriving one at a time would reorder the list +// under the cursor as each answer landed. +func (m *Builds) fetchGateBuilds() tea.Cmd { + client, prID, ids := m.client, m.prID, m.gateBuilds + return func() tea.Msg { return gateRuns(prID, ids, client.BuildByID) } +} + +// gateRuns reads each gate run and folds the outcome into one answer. The +// fetch is a parameter rather than the client so this — the part that decides +// what the list is told — can be exercised without a server. +func gateRuns(prID int, ids []int, fetch func(int) (azdo.Build, error)) gateRunsMsg { + builds := make([]azdo.Build, 0, len(ids)) + for _, id := range ids { + b, err := fetch(id) + if err != nil { + return gateRunsMsg{PR: prID, Err: fmt.Errorf("could not fetch build %d: %w", id, err)} + } + builds = append(builds, b) + } + return gateRunsMsg{PR: prID, Builds: builds} +} + // applyRows rebuilds the browser's rows, carrying forward whatever timelines // have already loaded. func (m *Builds) applyRows() { @@ -268,7 +330,25 @@ func (m *Builds) Update(msg tea.Msg) (View, tea.Cmd) { return m, m.work.tick(msg) case buildsMsg: + if m.prID != 0 { + // The project's recent runs, broadcast to a list that is showing + // one pull request's gates. See gateRunsMsg. + return m, nil + } + m.work.done() + m.builds, m.loaded = msg.Builds, true + m.applyRows() + return m, m.fetchTimelines() + + case gateRunsMsg: + if m.prID == 0 || msg.PR != m.prID { + return m, nil + } m.work.done() + if msg.Err != nil { + m.status, m.failed = msg.Err.Error(), true + return m, nil + } m.builds, m.loaded = msg.Builds, true m.applyRows() return m, m.fetchTimelines() @@ -507,11 +587,19 @@ func (m *Builds) Body(width, height int) string { return placeholder("builds", m.status, m.failed, m.work.View()) } if m.browser.Len() == 0 { - return emptyState("no pipeline runs in this project", width, height) + return emptyState(m.emptyLine(), width, height) } return m.browser.View() } +// emptyLine says what is missing in the terms the list was opened in. +func (m *Builds) emptyLine() string { + if m.prID != 0 { + return fmt.Sprintf("no gate runs left to show for !%d", m.prID) + } + return "no pipeline runs in this project" +} + // refresh re-reads the list from scratch, dropping every timeline with it so // the rows rebuild against what the server says now. It is its own method // because three things ask for it — the refresh key and the two actions that @@ -566,8 +654,14 @@ func (m *Builds) handleArmedBuild(msg tea.KeyMsg) tea.Cmd { return armed.fire(m.client) } -// Title reports the run count and the project the builds belong to. +// Title reports the run count and the project the builds belong to, naming +// the pull request when the list is one pull request's gates rather than the +// project's recent runs. func (m *Builds) Title() string { + if m.prID != 0 { + return fmt.Sprintf("build gates for !%d (%d) · %s/%s", + m.prID, m.browser.Len(), m.client.Org, m.client.Project) + } return fmt.Sprintf("builds (%d) · %s/%s", m.browser.Len(), m.client.Org, m.client.Project) } diff --git a/internal/ui/builds_test.go b/internal/ui/builds_test.go index 5d7f8e7..85fd147 100644 --- a/internal/ui/builds_test.go +++ b/internal/ui/builds_test.go @@ -381,3 +381,113 @@ func TestBuildsEmptyStateSaysThereAreNoRuns(t *testing.T) { t.Errorf("empty view did not draw the gulls:\n%s", out) } } + +func TestBuildsForAPullRequestNamesItInTheTitle(t *testing.T) { + c, builds := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001, 9000}) + updated, _ := m.Update(gateRunsMsg{PR: 512, Builds: builds}) + m = updated.(*Builds) + + if !strings.Contains(m.Title(), "!512") { + t.Errorf("title = %q, want it to name the pull request", m.Title()) + } + if !strings.Contains(m.Title(), "(2)") { + t.Errorf("title = %q, want the run count", m.Title()) + } +} + +func TestBuildsForAPullRequestOpensOnTheGateRunsItWasGiven(t *testing.T) { + c, _ := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + + if got := m.gateBuilds; len(got) != 1 || got[0] != 9001 { + t.Errorf("gate builds = %v, want the ids it was opened with", got) + } +} + +func TestBuildsForAPullRequestSaysSoWhenEmpty(t *testing.T) { + c, _ := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ := m.Update(gateRunsMsg{PR: 512}) + m = updated.(*Builds) + + if body := m.Body(120, 20); !strings.Contains(body, "!512") { + t.Errorf("empty state = %q, want it to name the pull request", body) + } +} + +func TestBuildsForAPullRequestReportsItsOwnFetchFailing(t *testing.T) { + c, _ := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ := m.Update(gateRunsMsg{PR: 512, Err: errTest}) + m = updated.(*Builds) + + status, isErr := m.Status() + if !isErr || !strings.Contains(status, errTest.Error()) { + t.Errorf("status = %q, isErr = %v, want the failure reported", status, isErr) + } +} + +// The gate list and the project list can both be alive in one stack — menu, +// builds, p, then B from the pull request it opened — and Root broadcasts +// data to every view in it. Each listing must land in exactly one of them. +func TestBuildsForAPullRequestIgnoresTheProjectListing(t *testing.T) { + c, builds := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ := m.Update(buildsMsg{Builds: builds}) + m = updated.(*Builds) + + if m.loaded { + t.Errorf("the gate list took the project's recent runs as its own:\n%s", m.Body(120, 20)) + } +} + +func TestBuildsIgnoresGateRunsThatAreNotItsOwn(t *testing.T) { + c, builds := buildFixture() + + project := newBuilds(t) + updated, _ := project.Update(gateRunsMsg{PR: 512, Builds: builds[:1]}) + project = updated.(*Builds) + if project.browser.Len() != 2 { + t.Errorf("the project list took a gate listing: %d rows, want its own 2", project.browser.Len()) + } + + gates := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ = gates.Update(gateRunsMsg{PR: 999, Builds: builds}) + gates = updated.(*Builds) + if gates.loaded { + t.Error("the gate list took another pull request's gate runs") + } +} + +// The gate list is the one Builds instance Root does not build, so the +// property that makes a second instance safe is asserted here rather than +// assumed: every answer it reads back names the pull request it belongs to, +// including a failed one. An ErrMsg would name nobody and be read by the +// project list as its own fetch failing. +func TestGateRunsAnswerNamesThePullRequestEvenWhenTheFetchFails(t *testing.T) { + _, builds := buildFixture() + + got := gateRuns(512, []int{9001, 9000}, func(id int) (azdo.Build, error) { + for _, b := range builds { + if b.ID == id { + return b, nil + } + } + return azdo.Build{}, errTest + }) + if got.PR != 512 || got.Err != nil || len(got.Builds) != 2 { + t.Fatalf("gateRuns = %+v, want !512's two runs and no error", got) + } + + got = gateRuns(512, []int{7}, func(int) (azdo.Build, error) { return azdo.Build{}, errTest }) + if got.PR != 512 { + t.Errorf("a failed answer names !%d, want !512", got.PR) + } + if got.Err == nil { + t.Error("a failed fetch reported no error") + } + if got.Builds != nil { + t.Errorf("a failed fetch returned %d runs, want none", len(got.Builds)) + } +} diff --git a/internal/ui/keys.go b/internal/ui/keys.go index 717a422..2d07e71 100644 --- a/internal/ui/keys.go +++ b/internal/ui/keys.go @@ -28,11 +28,18 @@ var ( // keyState is capital because s is already "copy slack link"; a is kept // alongside it rather than folded in, since Active is the common case the // picker would otherwise make one keystroke slower. - keyState = key.NewBinding(key.WithKeys("S"), key.WithHelp("S", "set state")) - keyBranch = key.NewBinding(key.WithKeys("b"), key.WithHelp("b", "branch")) - keyDrafts = key.NewBinding(key.WithKeys("d"), key.WithHelp("d", "drafts")) - keyReview = key.NewBinding(key.WithKeys("v"), key.WithHelp("v", "needs my review")) - keyLinkedPR = key.NewBinding(key.WithKeys("p"), key.WithHelp("p", "linked pull request")) + keyState = key.NewBinding(key.WithKeys("S"), key.WithHelp("S", "set state")) + keyBranch = key.NewBinding(key.WithKeys("b"), key.WithHelp("b", "branch")) + keyDrafts = key.NewBinding(key.WithKeys("d"), key.WithHelp("d", "drafts")) + keyReview = key.NewBinding(key.WithKeys("v"), key.WithHelp("v", "needs my review")) + keyLinkedPR = key.NewBinding(key.WithKeys("p"), key.WithHelp("p", "linked pull request")) + // keyGateBuilds opens the runs a pull request's build validation policies + // produced — the reverse of keyLinkedPR, which finds the pull request a + // run built. It is capital because "b" is the branch key on the work item + // list, and that list reaches the pull request pane this binding lives on + // through keyLinkedPR: a key meaning one thing on a list and another on a + // pane that list opens is the trap keyDiff's comment names. + keyGateBuilds = key.NewBinding(key.WithKeys("B"), key.WithHelp("B", "build gates")) keyLinkedItem = key.NewBinding(key.WithKeys("w"), key.WithHelp("w", "linked work item")) keyLogs = key.NewBinding(key.WithKeys("enter"), key.WithHelp("enter", "logs")) keyItem = key.NewBinding(key.WithKeys("enter"), key.WithHelp("enter", "open item")) diff --git a/internal/ui/pullrequestdetail.go b/internal/ui/pullrequestdetail.go index 2bb2d02..d96d900 100644 --- a/internal/ui/pullrequestdetail.go +++ b/internal/ui/pullrequestdetail.go @@ -48,6 +48,15 @@ type threadResolvedMsg struct { Err error } +// gateBuildsMsg carries the runs this pull request's build validation +// policies produced. It names its pull request for the same reason +// threadsLoadedMsg does: Root broadcasts data to every view in the stack. +type gateBuildsMsg struct { + PR int + Gates []azdo.GateBuild + Err error +} + // voteCastMsg carries the outcome of casting a reviewer vote. type voteCastMsg struct { PR int @@ -133,6 +142,20 @@ type PullRequestDetail struct { // considered, so there is nothing left to arm a vote while it is open. armedVote *pendingVote + // gatesPending is true while a build gate lookup is in flight, so a + // second press does not start another, and a late answer knows whether + // it is still the one being waited on. + gatesPending bool + + // hidden is true from the moment this view pushes something on top of + // itself until it is back on top. Root broadcasts gateBuildsMsg to the + // whole stack, so a lookup that resolves while the diff pane is showing + // must not push the build list over it. A KeyMsg is proof of being back + // on top — Root routes keys to the top view alone — so the flag clears + // the moment one arrives. Same arrangement as the builds view; see its + // hidden field. + hidden bool + // armedDraft is the same arrangement for the draft toggle, and for the // same reason — see draft.go. No two of the three modal states can be // open at once: each of them swallows every key that is not its own @@ -185,6 +208,42 @@ func (m *PullRequestDetail) fetchThreads() tea.Cmd { // fetchLinked resolves the work items this pull request is attached to. The // endpoint returns bare ids, so the titles come from one batch fetch. +// fetchGateBuilds reads the runs this pull request's build validation +// policies produced. It is asked for on the keypress rather than fetched on +// entry, the same reasoning the builds view gives for its pull request link: +// most visits to this pane never ask, and the evaluations are a call of their +// own. +func (m *PullRequestDetail) fetchGateBuilds() tea.Cmd { + client, pr := m.client, m.pr + return func() tea.Msg { + gates, err := client.PullRequestGateBuilds(pr) + if err != nil { + return gateBuildsMsg{PR: pr.ID, Err: fmt.Errorf("could not load the build gates for !%d: %w", pr.ID, err)} + } + return gateBuildsMsg{PR: pr.ID, Gates: gates} + } +} + +// gateBuildIDs is the runs, in the order the policies came back in. +func gateBuildIDs(gates []azdo.GateBuild) []int { + ids := make([]int, 0, len(gates)) + for _, g := range gates { + ids = append(ids, g.BuildID) + } + return ids +} + +// gateSummary is the status line the build list is opened with. The policy +// verdict is worth carrying over because it is not the build's: a rejected +// gate whose run succeeded means the policy was evaluated against an earlier +// iteration, and the build list alone would show nothing but a green run. +func gateSummary(gates []azdo.GateBuild) string { + if len(gates) == 1 { + return fmt.Sprintf("%s: %s", gates[0].Policy, gates[0].Status) + } + return fmt.Sprintf("%d build gates", len(gates)) +} + func (m *PullRequestDetail) fetchLinked() tea.Cmd { client, repo, id := m.client, m.pr.RepoID, m.pr.ID return func() tea.Msg { @@ -445,11 +504,39 @@ func (m *PullRequestDetail) Update(msg tea.Msg) (View, tea.Cmd) { m.invalidate() return m, nil + case gateBuildsMsg: + m.work.done() + if msg.PR != m.pr.ID || !m.gatesPending { + // Either an answer for the other pull request this stack can have + // open, or one this view is no longer waiting on. + return m, nil + } + m.gatesPending = false + if msg.Err != nil { + m.status, m.failed = msg.Err.Error(), true + return m, nil + } + if len(msg.Gates) == 0 { + m.status, m.failed = fmt.Sprintf("no build policy has run for !%d", m.pr.ID), false + return m, nil + } + if m.hidden { + // The lookup was still in flight when the user opened something + // else — see the hidden field's doc. + return m, nil + } + m.status, m.failed = gateSummary(msg.Gates), false + builds := NewBuildsForPullRequest(m.client, m.pr.ID, gateBuildIDs(msg.Gates)) + return m, func() tea.Msg { return PushMsg{View: builds} } + case StatusMsg: m.status = msg.Text return m, nil case tea.KeyMsg: + // A key is proof this view is back on top: Root routes keys to the + // top view alone. + m.hidden = false // The reply prompt owns every key while it is open, including esc and // the letters that are otherwise actions — typing "o" into it must add // the letter, not open a browser. Mirrors the branch prompt in @@ -519,13 +606,22 @@ func (m *PullRequestDetail) Update(msg tea.Msg) (View, tea.Cmd) { // and the diff pane shows each comment against the line it was // written on rather than fetching the same threads again. diff := NewPullRequestDiff(m.client, m.pr, m.threads) + m.hidden = true return m, func() tea.Msg { return PushMsg{View: diff} } + case key.Matches(msg, keyGateBuilds): + if m.gatesPending { + return m, nil // already looking them up + } + m.gatesPending = true + m.status, m.failed = "looking for the build gates…", false + return m, tea.Batch(m.fetchGateBuilds(), m.work.begin(1)) case key.Matches(msg, keyLinkedItem): if len(m.linked) == 0 { m.status = "no work items are linked to this pull request" return m, nil } item := NewItem(m.client, m.linked[0], nil) + m.hidden = true return m, func() tea.Msg { return PushMsg{View: item} } case key.Matches(msg, keyReply): t, ok := m.selectedThread() @@ -740,13 +836,13 @@ func (m *PullRequestDetail) Title() string { // keys_test.go, which renders every view's footer through the real help // component rather than trusting a character count. func (m *PullRequestDetail) Keys() help.KeyMap { - short := []key.Binding{keyDiff, keyLinkedItem, keyReply, keyResolve} + short := []key.Binding{keyDiff, keyGateBuilds, keyReply, keyResolve} // The draft toggle is in the panel rather than on the footer, labelled for // the pull request it is looking at — "publish" on a draft, "mark draft" // on a published one — and absent altogether on a merged or abandoned one, // which this view reaches through a work item's or a build's link. Offering // a key that would only be refused is worse than not offering it. - full := []key.Binding{keyDiff, keyLinkedItem, keyReply, keyResolve} + full := []key.Binding{keyDiff, keyGateBuilds, keyLinkedItem, keyReply, keyResolve} if draftable(m.pr) { full = append(full, draftBinding(m.pr.IsDraft)) } diff --git a/internal/ui/pullrequestdetail_test.go b/internal/ui/pullrequestdetail_test.go index 65ced49..257e7bc 100644 --- a/internal/ui/pullrequestdetail_test.go +++ b/internal/ui/pullrequestdetail_test.go @@ -695,3 +695,71 @@ func TestVoteCastIgnoresAnotherPullRequests(t *testing.T) { t.Error("a vote belonging to another pull request was applied") } } + +func TestPullRequestDetailOpensTheGateBuilds(t *testing.T) { + m := newPRDetail(t, []azdo.Thread{}) + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + m = updated.(*PullRequestDetail) + if cmd == nil { + t.Fatal("B produced no command") + } + if status, _ := m.Status(); !strings.Contains(status, "build gate") { + t.Errorf("status = %q, want it to say the gates are being looked up", status) + } + + updated, cmd = m.Update(gateBuildsMsg{PR: 512, Gates: []azdo.GateBuild{{BuildID: 9001, Policy: "CI gate", Status: "rejected"}}}) + m = updated.(*PullRequestDetail) + if cmd == nil { + t.Fatal("the gate answer produced no command") + } + push, ok := cmd().(PushMsg) + if !ok { + t.Fatalf("the gate answer produced %T, want a PushMsg", cmd()) + } + if !strings.Contains(push.View.Title(), "!512") { + t.Errorf("pushed view = %q, want the pull request's build list", push.View.Title()) + } +} + +func TestPullRequestDetailSaysWhenNothingGatesIt(t *testing.T) { + m := newPRDetail(t, []azdo.Thread{}) + m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + + updated, cmd := m.Update(gateBuildsMsg{PR: 512}) + m = updated.(*PullRequestDetail) + if cmd != nil { + t.Errorf("a pull request with no gates pushed a view anyway") + } + if status, _ := m.Status(); !strings.Contains(status, "no build") { + t.Errorf("status = %q, want it to say there is nothing gating this pull request", status) + } +} + +func TestPullRequestDetailIgnoresAGateAnswerForAnotherPullRequest(t *testing.T) { + // Root broadcasts data to every view in the stack, and two of these can be + // open at once — see replySentMsg. + m := newPRDetail(t, []azdo.Thread{}) + m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + + _, cmd := m.Update(gateBuildsMsg{PR: 999, Gates: []azdo.GateBuild{{BuildID: 1}}}) + if cmd != nil { + t.Error("an answer for another pull request pushed a view") + } +} + +func TestPullRequestDetailDoesNotJumpToGatesFromUnderAnotherView(t *testing.T) { + m := newPRDetail(t, nil) + m.pr.SourceCommit, m.pr.TargetCommit = "aaa", "bbb" + + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + m = updated.(*PullRequestDetail) + // The diff pane goes on top while the lookup is still in flight. + updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'D'}}) + m = updated.(*PullRequestDetail) + + _, cmd := m.Update(gateBuildsMsg{PR: 512, Gates: []azdo.GateBuild{{BuildID: 9001}}}) + if cmd != nil { + t.Error("the gate answer pushed a view over the one the user had opened") + } +} diff --git a/internal/ui/root_test.go b/internal/ui/root_test.go index dfc248c..9fd8271 100644 --- a/internal/ui/root_test.go +++ b/internal/ui/root_test.go @@ -344,11 +344,18 @@ func TestRootBroadcastsErrMsgToTheViewItBelongsToEvenWhenNotOnTop(t *testing.T) // emit ErrMsg with no id naming which instance it belongs to, which is only // safe to broadcast because exactly one instance of each is ever alive in a // stack — every one is built once, by Root.build, from an empty stack (see -// NewRoot and the menu's enter handler in Root.key). A ninth feature that -// pushed a second instance of one of these three views — a work item view -// reachable from inside another view's drill-down, say — would make a -// broadcast ErrMsg ambiguous between two live views and resurrect the very -// bug the broadcast was written to fix, silently. +// NewRoot and the menu's enter handler in Root.key). A feature that pushed a +// second instance of one of these three views — a work item view reachable +// from inside another view's drill-down, say — would make a broadcast ErrMsg +// ambiguous between two live views and resurrect the very bug the broadcast +// was written to fix, silently. +// +// NewBuildsForPullRequest is the one exception, and it earns it by not +// relying on the invariant at all: a build list opened on a pull request's +// gates fetches through gateRunsMsg, which names its pull request, ignores +// the project listing, and reports its own failures rather than emitting an +// unowned ErrMsg — see the gate tests in builds_test.go. Its call to +// NewBuilds is skipped below for that reason, and nothing else may be. // // There is no runtime state that exercises this today — the risk is a future // call site, not a reachable program state — so this checks the source itself @@ -378,6 +385,9 @@ func TestListViewsAreConstructedOnlyByRootBuild(t *testing.T) { if strings.Contains(line, "func "+ctor) { continue // the constructor's own definition, not a call site. } + if name == "builds.go" && ctor == "NewBuilds(" && strings.Contains(line, "m := NewBuilds(c)") { + continue // NewBuildsForPullRequest; see this test's doc. + } if name != "root.go" { t.Errorf("%s:%d calls %s outside root.go — a second live instance of this view "+ "would make the ErrMsg broadcast ambiguous between it and the original", From 4bca6a51a8f63253383020cf01c66f2b023b74ac Mon Sep 17 00:00:00 2001 From: Jacob Atchley Date: Mon, 21 Sep 2026 15:40:22 -0500 Subject: [PATCH 2/3] fix: ask the policy evaluations endpoint for a preview api-version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing B came back with "The requested version "7.1" of the resource is under preview. The -preview flag must be supplied in the api-version for such requests." Policy evaluations are still a preview resource and refuse a plain version outright, so the endpoint gets APIVersion's preview twin — the same arrangement connectionData already needed. Verified against a live pull request: the four build gates on !11924 come back with their policy names, verdicts and run ids, and each run reads. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EnkNZwrGWfCeQDsHVkvPuE --- internal/azdo/policies.go | 11 ++++++++++- internal/azdo/policies_test.go | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/internal/azdo/policies.go b/internal/azdo/policies.go index e362caf..cae41aa 100644 --- a/internal/azdo/policies.go +++ b/internal/azdo/policies.go @@ -11,6 +11,15 @@ import ( // that runs a pipeline, so it is the only one that can name a build. const buildPolicyTypeID = "0609b952-1397-4640-95ec-e00a01b2c241" +// evaluationsAPIVersion is APIVersion's preview twin for this endpoint, the +// same arrangement connectionData needs. Policy evaluations are still under +// preview, and asking for a plain version is refused outright: "The requested +// version \"7.1\" of the resource is under preview. The -preview flag must be +// supplied in the api-version for such requests." The .1 is the revision the +// shape read below belongs to; a bare -preview is accepted too, but pinning +// the revision is the point of pinning the version at all. +const evaluationsAPIVersion = APIVersion + "-preview.1" + // unnamedPolicy is what a build validation with no display name is called. A // policy is named when it is configured and most are, but an unnamed one still // gates the pull request and still has a run worth opening. @@ -57,7 +66,7 @@ func (c *Client) PullRequestGateBuilds(pr PullRequest) ([]GateBuild, error) { artifact := fmt.Sprintf("vstfs:///CodeReview/CodeReviewId/%s/%d", pr.ProjectID, pr.ID) endpoint := fmt.Sprintf("%s/%s/%s/_apis/policy/evaluations?artifactId=%s&api-version=%s", c.root(), url.PathEscape(c.Org), url.PathEscape(c.Project), - url.QueryEscape(artifact), APIVersion) + url.QueryEscape(artifact), evaluationsAPIVersion) if err := c.get(endpoint, &resp); err != nil { return nil, err } diff --git a/internal/azdo/policies_test.go b/internal/azdo/policies_test.go index 8e58c12..32fc03d 100644 --- a/internal/azdo/policies_test.go +++ b/internal/azdo/policies_test.go @@ -30,6 +30,13 @@ func TestPullRequestGateBuildsKeepsTheBuildsAPolicyHasRun(t *testing.T) { if !strings.Contains(r.URL.Path, "/_apis/policy/evaluations") { t.Errorf("path = %q, want the policy evaluations endpoint", r.URL.Path) } + // Azure DevOps refuses this endpoint at a plain version: "The + // requested version "7.1" of the resource is under preview. The + // -preview flag must be supplied in the api-version for such + // requests." + if got := r.URL.Query().Get("api-version"); !strings.HasSuffix(got, "-preview.1") { + t.Errorf("api-version = %q, want the preview form the endpoint requires", got) + } w.Write([]byte(gateBody)) }) From 23edab4bbc5f60f69a7365811164c2b4d74ca59d Mon Sep 17 00:00:00 2001 From: Jacob Atchley Date: Mon, 21 Sep 2026 15:55:43 -0500 Subject: [PATCH 3/3] style: gofmt the pull request struct's new field The ProjectID field and the project object beside it went in by hand and left the surrounding alignment stale, which the CI gofmt check caught. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EnkNZwrGWfCeQDsHVkvPuE --- internal/azdo/pullrequests.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/azdo/pullrequests.go b/internal/azdo/pullrequests.go index 77e67e8..528288b 100644 --- a/internal/azdo/pullrequests.go +++ b/internal/azdo/pullrequests.go @@ -59,10 +59,10 @@ func (r Reviewer) VoteLabel() string { // PullRequest is one active pull request, in any repository in the project. type PullRequest struct { - ID int - Title string - Repo string - RepoID string + ID int + Title string + Repo string + RepoID string // ProjectID is the GUID of the team project the repository belongs to. // The project is known by name everywhere else in boardwalk, but the // policy endpoint addresses a pull request by an artifact id built from