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.
2026-09-26 18:45:48 +10:00
parent 0cf41409f1
commit 6988be9ff3
6 changed files with 298 additions and 51 deletions
@@ -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)
+	}
+}