From 6988be9ff3fbc846b72d1bd071aa6c0d6e8cc511 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 18:45:48 +1000 Subject: [PATCH] watchpr: only alert on mergeability lost after the baseline Track mergeability across the whole run and decode it as a tri-state, so a PR already conflicted when watching starts is polled on instead of reported. --- AGENTS.md | 6 ++ README.md | 5 +- cmd/watchpr/main.go | 2 +- internal/agent/gitea.go | 12 +-- internal/agent/watch.go | 131 +++++++++++++++++++----- internal/agent/watch_test.go | 193 +++++++++++++++++++++++++++++++---- 6 files changed, 298 insertions(+), 51 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2ef6715..68433d2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -220,6 +220,12 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. half-finished (`rebase-merge`, `MERGE_HEAD`, `CHERRY_PICK_HEAD`, …). A directory whose backing repo is gone is deleted outright, but only ever inside the worktree root. +- `watchpr` alerts on changes, not on conditions: the baseline snapshot is a real + read, so a PR that is already conflicted or already CI-failing when watching + starts is polled on rather than reported. Mergeability is the one rule needing + a run of observations (`prWatch`), because a conflict must both persist for two + polls and follow a mergeable one; Gitea's `mergeable` is tri-state (absent/null + = not yet computed) and unknown counts as neither answer. - CI "combined status" comes from `/commits/{sha}/status`; an empty head SHA yields an empty state without an API call. - Gitea backs every PR with an issue of the same number and serves comments from diff --git a/README.md b/README.md index a09ac35..e09979c 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,10 @@ Non-zero exit on any API error. Poll PRs and exit (reporting what changed) when a tracked PR **merges/closes**, gets a **new comment from someone other than the agent**, its **CI fails** (failure/error), or it **loses mergeability** (a conflict appears). Benign -transitions — CI `pending`→`success`, the agent's own comments — are ignored. +transitions — CI `pending`→`success`, the agent's own comments and pushes — are +ignored, and so is any condition the PR was already in at the baseline: a PR +that is already conflicted or already failing when watching starts keeps being +watched. ```bash # Watch until something meaningful happens (default interval 60s) diff --git a/cmd/watchpr/main.go b/cmd/watchpr/main.go index 3efcee5..5a9d505 100644 --- a/cmd/watchpr/main.go +++ b/cmd/watchpr/main.go @@ -176,7 +176,7 @@ func report(key, reason string, st agent.PRState, jsonMode bool) { } func printState(st agent.PRState) { - fmt.Printf("%s state=%s merged=%t mergeable=%t ci=%s head=%s comments(non-agent)=%d\n", + fmt.Printf("%s state=%s merged=%t mergeable=%s ci=%s head=%s comments(non-agent)=%d\n", st.Ref.String(), st.State, st.Merged, st.Mergeable, ciOrNone(st.CIStatus), shortSHA(st.HeadSHA), st.NonAgentComments) } diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index 6cc8d73..6f84f75 100644 --- a/internal/agent/gitea.go +++ b/internal/agent/gitea.go @@ -189,12 +189,12 @@ func (c *GiteaClient) Whoami() (User, error) { // PullRequest is the subset of Gitea's PR object we track. type PullRequest struct { - Number int `json:"number"` - State string `json:"state"` - Title string `json:"title"` - Merged bool `json:"merged"` - Mergeable bool `json:"mergeable"` - HTMLURL string `json:"html_url"` + Number int `json:"number"` + State string `json:"state"` + Title string `json:"title"` + Merged bool `json:"merged"` + Mergeable Mergeability `json:"mergeable"` + HTMLURL string `json:"html_url"` Head struct { Sha string `json:"sha"` Ref string `json:"ref"` diff --git a/internal/agent/watch.go b/internal/agent/watch.go index 385fa8c..4719e3a 100644 --- a/internal/agent/watch.go +++ b/internal/agent/watch.go @@ -3,6 +3,7 @@ package agent import ( "errors" "fmt" + "strings" "time" ) @@ -16,17 +17,62 @@ func IsPRGone(err error) bool { return errors.Is(err, errPRGone) } +// Mergeability is Gitea's mergeable flag as a tri-state. The flag is null or +// absent while Gitea recomputes the merge base, and an unknown value must not be +// read as either answer. +type Mergeability int + +const ( + MergeUnknown Mergeability = iota + MergeYes + MergeNo +) + +func (m Mergeability) String() string { + switch m { + case MergeYes: + return "true" + case MergeNo: + return "false" + } + return "unknown" +} + +func (m Mergeability) MarshalJSON() ([]byte, error) { + switch m { + case MergeYes: + return []byte("true"), nil + case MergeNo: + return []byte("false"), nil + } + return []byte("null"), nil +} + +func (m *Mergeability) UnmarshalJSON(b []byte) error { + switch strings.TrimSpace(string(b)) { + case "true": + *m = MergeYes + case "false": + *m = MergeNo + case "null": + *m = MergeUnknown + default: + return fmt.Errorf("mergeable: unexpected value %s", b) + } + return nil +} + // PRState is a point-in-time snapshot of the PR attributes watchpr tracks. type PRState struct { - Ref PRRef `json:"ref"` - State string `json:"state"` // open / closed - Merged bool `json:"merged"` - HeadSHA string `json:"head_sha"` - Mergeable bool `json:"mergeable"` - CIStatus string `json:"ci_status"` // success / pending / failure / error / "" - NonAgentComments int `json:"non_agent_comments"` - Title string `json:"title"` - URL string `json:"url"` + Ref PRRef `json:"ref"` + State string `json:"state"` // open / closed + Merged bool `json:"merged"` + HeadSHA string `json:"head_sha"` + Mergeable Mergeability `json:"mergeable"` + CIStatus string `json:"ci_status"` // success / pending / failure / error / "" + NonAgentComments int `json:"non_agent_comments"` + Title string `json:"title"` + URL string `json:"url"` } // FetchState builds a PRState for the given ref. agentLogin's comments are @@ -95,6 +141,53 @@ func terminalState(st PRState) (bool, string) { return false, "" } +// conflictPolls is how many consecutive non-mergeable polls confirm a real +// conflict. Gitea reports mergeable=false while it recomputes the merge base +// after a push, so a single poll is debounced. +const conflictPolls = 2 + +// prWatch tracks one PR across polls. Mergeability needs more memory than the +// previous snapshot: losing it only counts as a change if the PR was mergeable +// at some point after watching began, since a conflict that predates the +// baseline is the state the operator is already waiting on. +type prWatch struct { + prev PRState + everMergeable bool + conflicts int +} + +func newPRWatch(baseline PRState) *prWatch { + w := &prWatch{prev: baseline} + w.track(baseline) + return w +} + +// track folds one snapshot's mergeability into the run of observations. Unknown +// neither confirms a conflict nor clears one. +func (w *prWatch) track(st PRState) { + switch st.Mergeable { + case MergeYes: + w.everMergeable = true + w.conflicts = 0 + case MergeNo: + w.conflicts++ + } +} + +// observe folds in the newest snapshot and reports whether the watch should end. +func (w *prWatch) observe(cur PRState) (bool, string) { + changed, reason := MeaningfulChange(w.prev, cur) + w.prev = cur + w.track(cur) + if changed { + return true, reason + } + if w.everMergeable && w.conflicts >= conflictPolls && cur.State == "open" { + return true, "PR lost mergeability (conflict)" + } + return false, "" +} + // MaxPollFailures is how many consecutive failed polls of the same PR are // tolerated before Watch gives up. The abort fires on the 20th failed tick, so // at watchpr's default 60s interval a watch rides out ~19 minutes of failure. @@ -112,7 +205,7 @@ const MaxPollFailures = 20 // onBaseline, if set, fires once after all baselines are captured and before the // first tick. func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Time, onBaseline func(), onError func(PRRef, error)) (WatchResult, error) { - prev := make(map[string]PRState, len(refs)) + watches := make(map[string]*prWatch, len(refs)) for _, ref := range refs { st, err := f.FetchState(ref, agentLogin) if err != nil { @@ -121,7 +214,7 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti if terminal, reason := terminalState(st); terminal { return WatchResult{Ref: ref, Reason: reason, State: st}, nil } - prev[ref.String()] = st + watches[ref.String()] = newPRWatch(st) } if onBaseline != nil { onBaseline() @@ -145,10 +238,9 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti continue } fails[key] = 0 - if changed, reason := MeaningfulChange(prev[key], cur); changed { + if changed, reason := watches[key].observe(cur); changed { return WatchResult{Ref: ref, Reason: reason, State: cur}, nil } - prev[key] = cur } } return WatchResult{}, nil @@ -172,15 +264,15 @@ func isFailedCI(state string) bool { // MeaningfulChange compares a previous state to the current one and reports // whether a change warrants alerting the operator, with a human-readable -// reason. Benign transitions (CI pending→success, the agent's own comments, an -// unchanged snapshot) return false. +// reason. Benign transitions (CI pending→success, the agent's own comments, a +// new head commit, an unchanged snapshot) return false. Mergeability is not +// decided here: it takes a whole run of observations, which prWatch keeps. // // Alerting conditions: // - the PR merged // - the PR closed without merging // - a new comment from someone other than the agent // - CI transitioned into failure/error -// - the PR lost mergeability (a conflict appeared) for two consecutive polls func MeaningfulChange(prev, cur PRState) (bool, string) { if !prev.Merged && cur.Merged { return true, "PR merged" @@ -195,12 +287,5 @@ func MeaningfulChange(prev, cur PRState) (bool, string) { if isFailedCI(cur.CIStatus) && !isFailedCI(prev.CIStatus) { return true, "CI failed (" + cur.CIStatus + ")" } - // Gitea computes mergeability asynchronously, so a PR can briefly report - // mergeable=false right after a push. Require the loss to persist across two - // consecutive polls (both prev and cur false, still open) before treating it - // as a real conflict; a single false poll is debounced. - if !prev.Mergeable && !cur.Mergeable && cur.State == "open" { - return true, "PR lost mergeability (conflict)" - } return false, "" } diff --git a/internal/agent/watch_test.go b/internal/agent/watch_test.go index 101ecff..66a702b 100644 --- a/internal/agent/watch_test.go +++ b/internal/agent/watch_test.go @@ -1,6 +1,7 @@ package agent import ( + "encoding/json" "errors" "fmt" "net/http" @@ -17,7 +18,7 @@ func base() PRState { State: "open", Merged: false, HeadSHA: "abc123", - Mergeable: true, + Mergeable: MergeYes, CIStatus: "pending", NonAgentComments: 0, } @@ -66,25 +67,11 @@ func TestMeaningfulChange(t *testing.T) { wantChange: true, }, { - // A single mergeable=false poll is debounced: Gitea often reports - // this transiently right after a push. - name: "mergeable true to false for one poll is benign", - mutate: func(s *PRState) { s.Mergeable = false }, - wantChange: false, - }, - { - // mergeable=false persisting into a second consecutive poll is a - // real conflict and alerts. - name: "mergeable false persisting a second poll alerts", - mutatePrev: func(s *PRState) { s.Mergeable = false }, - mutate: func(s *PRState) { s.Mergeable = false }, - wantChange: true, - }, - { - // mergeable recovered (false then true) must not alert. - name: "mergeable recovered false to true is benign", - mutatePrev: func(s *PRState) { s.Mergeable = false }, - mutate: func(s *PRState) {}, + // Mergeability takes a run of observations, so no pair of snapshots + // decides it here; prWatch owns that rule. + name: "mergeable false pair alone is not a pairwise change", + mutatePrev: func(s *PRState) { s.Mergeable = MergeNo }, + mutate: func(s *PRState) { s.Mergeable = MergeNo }, wantChange: false, }, { @@ -801,3 +788,169 @@ func TestWatchAnonymousKeepsPolling(t *testing.T) { t.Errorf("fetch calls = %d, want 3 (baseline + two polls)", f.calls) } } + +// drainableTicks returns a channel holding n ticks and already closed, so Watch +// polls exactly n times and then returns instead of blocking. +func drainableTicks(n int) <-chan time.Time { + ticks := make(chan time.Time, n) + for i := 0; i < n; i++ { + ticks <- time.Now() + } + close(ticks) + return ticks +} + +// The production bug: a PR that was already conflicted (and already CI-failing) +// when watching began must not be reported as having just changed. That state is +// what the watcher is waiting to see resolved, so the loop keeps polling. +func TestWatchIgnoresBaselineConflictAndFailure(t *testing.T) { + stuck := base() + stuck.Mergeable = MergeNo + stuck.CIStatus = "failure" + f := &fakeFetcher{states: []PRState{stuck}} + + res, err := Watch(f, []PRRef{stuck.Ref}, "unkin-agent", drainableTicks(5), nil, nil) + if err != nil { + t.Fatalf("Watch: %v", err) + } + if res.Reason != "" { + t.Fatalf("Watch ended with %q; a conflict/failure predating the watch is not a change", res.Reason) + } + if f.calls != 6 { + t.Errorf("fetch calls = %d, want 6 (baseline + 5 polls)", f.calls) + } +} + +// Mergeability lost after the baseline still alerts, on the second consecutive +// conflicted poll. +func TestWatchDetectsConflictAfterBaseline(t *testing.T) { + ok := base() + conflicted := base() + conflicted.Mergeable = MergeNo + f := &fakeFetcher{states: []PRState{ok, conflicted, conflicted}} + + res, err := Watch(f, []PRRef{ok.Ref}, "unkin-agent", drainableTicks(3), nil, nil) + if err != nil { + t.Fatalf("Watch: %v", err) + } + if res.Reason != "PR lost mergeability (conflict)" { + t.Errorf("reason = %q, want the mergeability loss", res.Reason) + } + if f.calls != 3 { + t.Errorf("fetch calls = %d, want 3 (baseline + the two conflicted polls)", f.calls) + } +} + +// CI that goes green→red during the watch alerts. +func TestWatchDetectsCIFailureAfterBaseline(t *testing.T) { + ok := base() + ok.CIStatus = "pending" + green := base() + green.CIStatus = "success" + red := base() + red.CIStatus = "failure" + f := &fakeFetcher{states: []PRState{ok, green, red}} + + res, err := Watch(f, []PRRef{ok.Ref}, "unkin-agent", drainableTicks(3), nil, nil) + if err != nil { + t.Fatalf("Watch: %v", err) + } + if res.Reason != "CI failed (failure)" { + t.Errorf("reason = %q, want the CI failure (pending→success must pass silently)", res.Reason) + } + if f.calls != 3 { + t.Errorf("fetch calls = %d, want 3 (the success poll must not end the watch)", f.calls) + } +} + +// The agent's own pushes and comments must not end a watch; a comment from +// anyone else must. +func TestWatchIgnoresAgentActivity(t *testing.T) { + start := base() + pushed := base() + pushed.HeadSHA = "def456" // the agent pushed a fix; non-agent comments unchanged + commented := pushed + commented.NonAgentComments = 1 + f := &fakeFetcher{states: []PRState{start, pushed, pushed, commented}} + + res, err := Watch(f, []PRRef{start.Ref}, "unkin-agent", drainableTicks(3), nil, nil) + if err != nil { + t.Fatalf("Watch: %v", err) + } + if res.Reason != "new comment from a non-agent user" { + t.Errorf("reason = %q, want the non-agent comment", res.Reason) + } + if f.calls != 4 { + t.Errorf("fetch calls = %d, want 4 (the agent's push and comment must not end the watch)", f.calls) + } +} + +func TestPRWatchMergeability(t *testing.T) { + snap := func(m Mergeability) PRState { + s := base() + s.Mergeable = m + return s + } + tests := []struct { + name string + baseline Mergeability + polls []Mergeability + wantPoll int // 1-based poll that ends the watch; 0 for none + }{ + {"conflicted before the watch never alerts", MergeNo, []Mergeability{MergeNo, MergeNo, MergeNo}, 0}, + {"loss after a mergeable baseline alerts on the second poll", MergeYes, []Mergeability{MergeNo, MergeNo}, 2}, + {"a lone conflicted poll is debounced", MergeYes, []Mergeability{MergeNo, MergeYes, MergeNo}, 0}, + {"unknown at baseline does not arm the rule", MergeUnknown, []Mergeability{MergeNo, MergeNo, MergeNo}, 0}, + {"unknown at baseline then a real loss alerts", MergeUnknown, []Mergeability{MergeYes, MergeNo, MergeNo}, 3}, + {"a baseline conflict resolved then lost again alerts", MergeNo, []Mergeability{MergeYes, MergeNo, MergeNo}, 3}, + {"unknown between conflicted polls does not clear them", MergeYes, []Mergeability{MergeNo, MergeUnknown, MergeNo}, 3}, + {"unknown alone is never a conflict", MergeYes, []Mergeability{MergeUnknown, MergeUnknown, MergeUnknown}, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := newPRWatch(snap(tt.baseline)) + got := 0 + for i, m := range tt.polls { + changed, reason := w.observe(snap(m)) + if !changed { + continue + } + if reason != "PR lost mergeability (conflict)" { + t.Fatalf("poll %d ended the watch with %q, want a mergeability loss", i+1, reason) + } + got = i + 1 + break + } + if got != tt.wantPoll { + t.Errorf("alerted on poll %d, want %d", got, tt.wantPoll) + } + }) + } +} + +// An absent or null mergeable flag means Gitea has not computed it yet, and must +// decode as unknown rather than as a conflict. +func TestMergeabilityDecoding(t *testing.T) { + tests := map[string]Mergeability{ + `{"number":7}`: MergeUnknown, + `{"number":7,"mergeable":null}`: MergeUnknown, + `{"number":7,"mergeable":true}`: MergeYes, + `{"number":7,"mergeable":false}`: MergeNo, + } + for body, want := range tests { + var pr PullRequest + if err := json.Unmarshal([]byte(body), &pr); err != nil { + t.Fatalf("Unmarshal(%s): %v", body, err) + } + if pr.Mergeable != want { + t.Errorf("Unmarshal(%s) mergeable = %v, want %v", body, pr.Mergeable, want) + } + } + out, err := json.Marshal(PRState{Mergeable: MergeUnknown}) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if !strings.Contains(string(out), `"mergeable":null`) { + t.Errorf("unknown mergeability encoded as %s, want null", out) + } +}