watchpr: arm the conflict rule on a new merge computation

A mergeable=false baseline meant a conflict could never be reported, since Gitea sends false while it recomputes after a push. Arm on a head or base SHA change as well as on a mergeable poll, break the conflict run on unknown and failed polls, and print the baseline with the conditions it suppresses.
2026-09-26 20:48:31 +10:00
parent 6988be9ff3
commit 54c1d868d3
5 changed files with 346 additions and 43 deletions
@@ -200,6 +200,11 @@ type PullRequest struct {
 		Ref   string `json:"ref"`
 		Label string `json:"label"`
 	} `json:"head"`
+	// Base.Sha is the base branch tip as of this response, not the merge base
+	// recorded when the PR was opened.
+	Base struct {
+		Sha string `json:"sha"`
+	} `json:"base"`
 }

 // prPageSize is the per-page limit for the pulls listing; maxPRPages caps how
@@ -17,9 +17,11 @@ 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.
+// Mergeability is Gitea's mergeable flag. Gitea 1.26 always sends a plain bool,
+// and sends false both for a real conflict and while it recomputes the merge
+// base after a push, so false on its own decides nothing (prWatch resolves it).
+// Unknown covers what the bool cannot carry: an absent or null flag from another
+// Gitea build, and a snapshot no successful poll ever filled in.
 type Mergeability int

 const (
@@ -68,6 +70,7 @@ type PRState struct {
 	State            string       `json:"state"` // open / closed
 	Merged           bool         `json:"merged"`
 	HeadSHA          string       `json:"head_sha"`
+	BaseSHA          string       `json:"base_sha"`
 	Mergeable        Mergeability `json:"mergeable"`
 	CIStatus         string       `json:"ci_status"` // success / pending / failure / error / ""
 	NonAgentComments int          `json:"non_agent_comments"`
@@ -101,6 +104,7 @@ func FetchState(c *GiteaClient, ref PRRef, agentLogin string) (PRState, error) {
 		State:            pr.State,
 		Merged:           pr.Merged,
 		HeadSHA:          pr.Head.Sha,
+		BaseSHA:          pr.Base.Sha,
 		Mergeable:        pr.Mergeable,
 		CIStatus:         ci,
 		NonAgentComments: countNonAgentComments(comments, agentLogin),
@@ -141,48 +145,67 @@ 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.
+// conflictPolls is how many consecutive non-mergeable polls of an unchanged
+// merge computation confirm a real conflict.
 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.
+// prWatch tracks one PR across polls, because mergeability needs more memory
+// than the previous snapshot. Gitea reports mergeable=false while it recomputes
+// the merge base after a push, so a false is only trusted once this watch has
+// seen a merge computation start: the PR was mergeable at some point, or its
+// head or base SHA moved. A bare false inherited from the baseline says nothing
+// -- it is equally a conflict the operator is already waiting on and a recompute
+// in flight -- so it arms nothing.
 type prWatch struct {
-	prev          PRState
-	everMergeable bool
-	conflicts     int
+	prev      PRState
+	armed     bool
+	conflicts int
 }

 func newPRWatch(baseline PRState) *prWatch {
-	w := &prWatch{prev: baseline}
-	w.track(baseline)
-	return w
+	return &prWatch{prev: baseline, armed: baseline.Mergeable == MergeYes}
 }

-// track folds one snapshot's mergeability into the run of observations. Unknown
-// neither confirms a conflict nor clears one.
+// mergeInputsChanged reports whether the commits Gitea merges have moved, which
+// starts a fresh merge computation whose result is attributable to this watch.
+func mergeInputsChanged(prev, cur PRState) bool {
+	return cur.HeadSHA != prev.HeadSHA || cur.BaseSHA != prev.BaseSHA
+}
+
+// track folds one snapshot's mergeability into the run of observations. The
+// polls that confirm a conflict must be adjacent, so anything but another
+// non-mergeable observation breaks the run.
 func (w *prWatch) track(st PRState) {
 	switch st.Mergeable {
-	case MergeYes:
-		w.everMergeable = true
-		w.conflicts = 0
 	case MergeNo:
 		w.conflicts++
+	case MergeYes:
+		w.armed = true
+		w.conflicts = 0
+	default:
+		w.conflicts = 0
 	}
 }

+// missed records a poll that never produced a snapshot; the run of adjacent
+// non-mergeable observations does not survive the gap.
+func (w *prWatch) missed() {
+	w.conflicts = 0
+}
+
 // 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)
+	if mergeInputsChanged(w.prev, cur) {
+		w.armed = true
+		w.conflicts = 0
+	}
 	w.prev = cur
 	w.track(cur)
 	if changed {
 		return true, reason
 	}
-	if w.everMergeable && w.conflicts >= conflictPolls && cur.State == "open" {
+	if w.armed && w.conflicts >= conflictPolls && cur.State == "open" {
 		return true, "PR lost mergeability (conflict)"
 	}
 	return false, ""
@@ -202,10 +225,11 @@ const MaxPollFailures = 20
 // is gone, renamed, or no longer visible), and MaxPollFailures consecutive
 // failures of one PR all abort, because a watcher that sees nothing must not
 // look healthy.
-// 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) {
+// onBaseline, if set, receives every captured baseline once, before the first
+// tick, so a caller can show what state the watch started from.
+func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Time, onBaseline func([]PRState), onError func(PRRef, error)) (WatchResult, error) {
 	watches := make(map[string]*prWatch, len(refs))
+	baselines := make([]PRState, 0, len(refs))
 	for _, ref := range refs {
 		st, err := f.FetchState(ref, agentLogin)
 		if err != nil {
@@ -215,9 +239,10 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti
 			return WatchResult{Ref: ref, Reason: reason, State: st}, nil
 		}
 		watches[ref.String()] = newPRWatch(st)
+		baselines = append(baselines, st)
 	}
 	if onBaseline != nil {
-		onBaseline()
+		onBaseline(baselines)
 	}
 	fails := make(map[string]int, len(refs))
 	for range ticks {
@@ -229,6 +254,7 @@ func Watch(f StateFetcher, refs []PRRef, agentLogin string, ticks <-chan time.Ti
 					return WatchResult{}, fmt.Errorf("polling %s: %w", key, err)
 				}
 				fails[key]++
+				watches[key].missed()
 				if onError != nil {
 					onError(ref, err)
 				}
@@ -265,8 +291,8 @@ 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, a
-// new head commit, an unchanged snapshot) return false. Mergeability is not
-// decided here: it takes a whole run of observations, which prWatch keeps.
+// new head or base 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
@@ -18,6 +18,7 @@ func base() PRState {
 		State:            "open",
 		Merged:           false,
 		HeadSHA:          "abc123",
+		BaseSHA:          "base000",
 		Mergeable:        MergeYes,
 		CIStatus:         "pending",
 		NonAgentComments: 0,
@@ -209,16 +210,16 @@ func TestWatchDetectsMergeAfterBaseline(t *testing.T) {
 	merged.Merged = true
 	f := &fakeFetcher{states: []PRState{open, merged}} // baseline open, then merged

-	baselineFired := false
+	var baselines []PRState
 	ticks := make(chan time.Time, 1)
 	ticks <- time.Now()
 	res, err := Watch(f, []PRRef{open.Ref}, "unkin-agent",
-		ticks, func() { baselineFired = true }, nil)
+		ticks, func(sts []PRState) { baselines = sts }, nil)
 	if err != nil {
 		t.Fatalf("Watch: %v", err)
 	}
-	if !baselineFired {
-		t.Errorf("onBaseline should fire for an open baseline")
+	if len(baselines) != 1 || baselines[0].Ref != open.Ref {
+		t.Errorf("onBaseline received %v, want the one open baseline", baselines)
 	}
 	if res.Reason != "PR merged" {
 		t.Errorf("reason = %q, want %q", res.Reason, "PR merged")
@@ -903,7 +904,8 @@ func TestPRWatchMergeability(t *testing.T) {
 		{"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 between conflicted polls breaks the run", MergeYes, []Mergeability{MergeNo, MergeUnknown, MergeNo}, 0},
+		{"the run restarts after an unknown", MergeYes, []Mergeability{MergeNo, MergeUnknown, MergeNo, MergeNo}, 4},
 		{"unknown alone is never a conflict", MergeYes, []Mergeability{MergeUnknown, MergeUnknown, MergeUnknown}, 0},
 	}
 	for _, tt := range tests {
@@ -928,8 +930,9 @@ func TestPRWatchMergeability(t *testing.T) {
 	}
 }

-// An absent or null mergeable flag means Gitea has not computed it yet, and must
-// decode as unknown rather than as a conflict.
+// Gitea 1.26 always sends mergeable as a plain bool, so unknown is reserved for
+// what that bool cannot carry: an absent or null flag must decode as unknown
+// rather than as a conflict, and unknown must encode back as null.
 func TestMergeabilityDecoding(t *testing.T) {
 	tests := map[string]Mergeability{
 		`{"number":7}`:                   MergeUnknown,
@@ -954,3 +957,206 @@ func TestMergeabilityDecoding(t *testing.T) {
 		t.Errorf("unknown mergeability encoded as %s, want null", out)
 	}
 }
+
+// The inverse of the bug this PR fixed: Gitea reports mergeable=false while it
+// recomputes the merge base after a push, and watchpr is normally started right
+// after the agent pushes, so the baseline can land inside that window. Demanding
+// a mergeable observation to arm the rule left a genuinely conflicted PR silent
+// forever. The new head is a new merge computation, so the conflict it confirms
+// belongs to this watch.
+func TestWatchAlertsWhenBaselineLandedInTheRecomputeWindow(t *testing.T) {
+	baseline := base()
+	baseline.Mergeable = MergeNo // still recomputing; the push is not visible yet
+
+	pushed := base()
+	pushed.Mergeable = MergeNo
+	pushed.HeadSHA = "def456"
+
+	f := &fakeFetcher{states: []PRState{baseline, pushed, pushed, pushed}}
+	res, err := Watch(f, []PRRef{baseline.Ref}, "unkin-agent", drainableTicks(3), nil, nil)
+	if err != nil {
+		t.Fatalf("Watch: %v", err)
+	}
+	if res.Reason != "PR lost mergeability (conflict)" {
+		t.Fatalf("reason = %q, want the mergeability loss; a conflict must not be unreportable because the baseline caught the recompute", res.Reason)
+	}
+	if f.calls != 3 {
+		t.Errorf("fetch calls = %d, want 3 (baseline + the two conflicted polls on the new head)", f.calls)
+	}
+}
+
+// A base-branch move restarts the same merge computation, so a conflict that
+// only becomes visible once someone merges into main alerts too.
+func TestWatchAlertsWhenBaseMovedIntoAConflict(t *testing.T) {
+	baseline := base()
+	baseline.Mergeable = MergeNo
+
+	moved := base()
+	moved.Mergeable = MergeNo
+	moved.BaseSHA = "base111"
+
+	f := &fakeFetcher{states: []PRState{baseline, moved, moved}}
+	res, err := Watch(f, []PRRef{baseline.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)
+	}
+}
+
+// The fix this PR exists for: a conflict predating the watch, on a head and base
+// that never move, is the condition the operator is already waiting on and must
+// stay silent however long the watch runs.
+func TestWatchNeverAlertsOnAStableBaselineConflict(t *testing.T) {
+	stuck := base()
+	stuck.Mergeable = MergeNo
+	f := &fakeFetcher{states: []PRState{stuck}}
+
+	res, err := Watch(f, []PRRef{stuck.Ref}, "unkin-agent", drainableTicks(100), nil, nil)
+	if err != nil {
+		t.Fatalf("Watch: %v", err)
+	}
+	if res.Reason != "" {
+		t.Fatalf("Watch ended with %q after 100 unchanged polls; a conflict that predates the watch is not a change", res.Reason)
+	}
+}
+
+// Head and base movement arm the conflict rule but are not themselves alerts: a
+// push, or a base that moves under the PR, must not end a watch.
+func TestWatchArmingMovementDoesNotAlert(t *testing.T) {
+	start := base()
+	pushed := base()
+	pushed.HeadSHA = "def456"
+	rebased := base()
+	rebased.HeadSHA = "789abc"
+	rebased.BaseSHA = "base111"
+	f := &fakeFetcher{states: []PRState{start, pushed, rebased}}
+
+	res, err := Watch(f, []PRRef{start.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 new head or base is not a change worth alerting on", res.Reason)
+	}
+}
+
+// The polls that confirm a conflict must be adjacent. A failed poll produces no
+// snapshot, so the run cannot span it and two non-adjacent falses do not fire.
+func TestWatchConflictRunDoesNotSpanAFailedPoll(t *testing.T) {
+	ok := base()
+	conflicted := base()
+	conflicted.Mergeable = MergeNo
+	f := &fakeFetcher{
+		states: []PRState{ok, conflicted, conflicted, conflicted, conflicted},
+		errs:   []error{nil, nil, errors.New("HTTP 502"), nil, nil},
+	}
+
+	res, err := Watch(f, []PRRef{ok.Ref}, "unkin-agent", drainableTicks(4), nil, func(PRRef, error) {})
+	if err != nil {
+		t.Fatalf("Watch: %v", err)
+	}
+	if res.Reason != "PR lost mergeability (conflict)" {
+		t.Fatalf("reason = %q, want the mergeability loss on the two adjacent polls", res.Reason)
+	}
+	if f.calls != 5 {
+		t.Errorf("fetch calls = %d, want 5: the falses either side of the failed poll are not a run", f.calls)
+	}
+}
+
+// perRefFetcher scripts a separate sequence per ref, so a multi-ref watch can be
+// driven with each PR doing something different.
+type perRefFetcher struct {
+	states map[string][]PRState
+	calls  map[string]int
+}
+
+func (f *perRefFetcher) FetchState(ref PRRef, _ string) (PRState, error) {
+	key := ref.String()
+	seq := f.states[key]
+	i := f.calls[key]
+	if i >= len(seq) {
+		i = len(seq) - 1
+	}
+	f.calls[key]++
+	return seq[i], nil
+}
+
+// Each ref keeps its own run of observations: one PR's mergeability, pushes and
+// resolutions must neither arm nor disarm another's conflict rule.
+func TestWatchTracksRefsIndependently(t *testing.T) {
+	refA := PRRef{Owner: "unkin", Repo: "repo", Number: 1}
+	refB := PRRef{Owner: "unkin", Repo: "repo", Number: 2}
+	snap := func(ref PRRef, m Mergeability, head string) PRState {
+		s := base()
+		s.Ref = ref
+		s.Mergeable = m
+		s.HeadSHA = head
+		return s
+	}
+
+	tests := []struct {
+		name string
+		a, b []PRState
+		want string // reason, "" for no alert
+	}{
+		{
+			name: "a mergeable neighbour does not arm a baseline conflict",
+			a:    []PRState{snap(refA, MergeYes, "aaa")},
+			b:    []PRState{snap(refB, MergeNo, "bbb")},
+			want: "",
+		},
+		{
+			name: "a neighbour's push does not arm a baseline conflict",
+			a:    []PRState{snap(refA, MergeYes, "aaa"), snap(refA, MergeYes, "aa2"), snap(refA, MergeYes, "aa3")},
+			b:    []PRState{snap(refB, MergeNo, "bbb")},
+			want: "",
+		},
+		{
+			name: "a neighbour's mergeable polls do not clear another's run",
+			a:    []PRState{snap(refA, MergeYes, "aaa")},
+			b:    []PRState{snap(refB, MergeYes, "bbb"), snap(refB, MergeNo, "bbb"), snap(refB, MergeNo, "bbb")},
+			want: "PR lost mergeability (conflict)",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := &perRefFetcher{
+				states: map[string][]PRState{refA.String(): tt.a, refB.String(): tt.b},
+				calls:  map[string]int{},
+			}
+			res, err := Watch(f, []PRRef{refA, refB}, "unkin-agent", drainableTicks(6), nil, nil)
+			if err != nil {
+				t.Fatalf("Watch: %v", err)
+			}
+			if res.Reason != tt.want {
+				t.Fatalf("reason = %q, want %q", res.Reason, tt.want)
+			}
+			if tt.want != "" && res.Ref != refB {
+				t.Errorf("alert names %s, want %s", res.Ref, refB)
+			}
+		})
+	}
+}
+
+// A watch that will deliberately stay silent about a pre-existing conflict has
+// to hand its caller the baseline it is staying silent about.
+func TestWatchReportsBaselineStates(t *testing.T) {
+	stuck := base()
+	stuck.Mergeable = MergeNo
+	stuck.CIStatus = "failure"
+	f := &fakeFetcher{states: []PRState{stuck}}
+
+	var got []PRState
+	if _, err := Watch(f, []PRRef{stuck.Ref}, "unkin-agent", drainableTicks(1),
+		func(sts []PRState) { got = sts }, nil); err != nil {
+		t.Fatalf("Watch: %v", err)
+	}
+	if len(got) != 1 {
+		t.Fatalf("onBaseline received %d state(s), want 1", len(got))
+	}
+	if got[0].Mergeable != MergeNo || got[0].CIStatus != "failure" {
+		t.Errorf("baseline = %+v, want the conflicted, CI-red snapshot the watch started from", got[0])
+	}
+}