AGENTS.md

@@ -235,16 +235,24 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`.
   `false`s has spanned `conflictWindow` (2m); a mergeable poll, an unknown one or
   a failed poll all break the run. The debounce is wall-clock, measured from the
   tick that fired each poll, because what it outlasts is the recompute and
-  `--interval` spans seconds to hours. Arming and the run are independent:
+  `--interval` spans seconds to hours. The window starts at the later of the
-  movement arms and nothing else, since `base.sha` is the base branch tip as of
+  run's first observation and the arm, which is what keeps arming and the run
-  the response, so it moves for every open PR whenever main does and a run reset
+  independent: movement arms without resetting the run — `base.sha` is the base
-  there could never complete on a busy repo. `Mergeability` stays tri-state for
+  branch tip as of the response, so it moves for every open PR whenever main does
-  what the bool cannot carry: an absent/null flag from some other Gitea, and a
+  and a reset there could never complete on a busy repo — while an arming poll
-  state no successful poll ever filled in. Head and base SHAs are read only to
+  still cannot confirm a run it played no part in. Re-arming an already-armed
+  watch is a no-op, so a base moving under every poll advances the window once
+  and never again. Both times are pointers because the zero `time.Time` is a
+  legal clock value and cannot also mean "no run". `Mergeability` stays tri-state
+  for what the bool cannot carry: an absent/null flag from some other Gitea, and
+  a state no successful poll ever filled in. Head and base SHAs are read only to
   arm — a push never alerts. The residue: a conflict landed by the push just
-  before the watch began, on a head and base that never move again, is never
+  before the watch began is unreported while the commits stay put, since Gitea's
-  reported; Gitea's payload has no field separating it from a check in flight
+  payload has no field separating it from a check in flight (`merge_base` is the
-  (`merge_base` is the true merge base and does not move on recheck).
+  true merge base and does not move on recheck). The commits rarely stay put —
+  a merge to the base branch moves `base.sha` under every open PR — but that
+  move only arms; the two minutes it then has to outlast are the recompute the
+  move started, not the falses before it.
 - 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

README.md

@@ -94,15 +94,29 @@ watched, and the baseline line — written to stderr, as a JSON record under
 Gitea reports `mergeable: false` both for a real conflict and while it
 recomputes the merge base after a push, so a conflict is only reported once the
 watch has seen a merge computation start (the PR was mergeable, or its head or
-base commit moved) **and** the non-mergeable polls have then run unbroken for
+base commit moved) **and** the non-mergeable polls have **then** run unbroken for
-two minutes. The debounce is a duration, not a poll count, because what it has
+two minutes. The window runs from whichever came later, so the poll that starts
-to outlast is Gitea's recompute and `--interval` ranges from seconds to hours.
+the merge computation never confirms a run of falses that predates it. The
+debounce is a duration, not a poll count, because what it has to outlast is
+Gitea's recompute and `--interval` ranges from seconds to hours.
 
-One case is therefore never reported: a conflict introduced by the push
+So a conflict introduced by the push immediately before the watch started is not
-immediately before the watch started, on a PR whose head and base never move
+reported while the commits stay put: nothing in Gitea's payload separates it from
-again. Nothing in Gitea's payload separates that from a merge check still in
+a merge check still in flight, and the baseline line is the only notice of it.
-flight, so watchpr stays silent about it for as long as it runs — the baseline
+That is a narrower gap than it looks, because `base.sha` is the base branch's
-line is how you see it.
+tip — it moves for every open PR whenever anything merges to the base branch, so
+the commits rarely stay put for long. The first such move arms the rule, and is
+itself silent: the window then runs from that arming poll rather than from the
+falses that predate it, so what is eventually reported is a conflict that
+outlasted the merge computation the move started.
+
+Arming is a one-way latch. Moves after that one — and every move seen by a watch
+that was already armed at the baseline — neither re-arm nor restart the window,
+because a restart on every `base.sha` move could never complete on a busy base
+branch. That is the cost side of the same trade: on a busy base branch a
+conflict can be confirmed while the newest merge recompute is less than two
+minutes old. The window guarantees that the run of falses outlasted *a* merge
+computation this watch saw start, not that it outlasted the most recent one.
 
 ```bash
 # Watch until something meaningful happens (default interval 60s)

cmd/watchpr/main.go

@@ -5,11 +5,15 @@
 // agent's own pushes and comments) are ignored, and so is any condition that was
 // already true at the baseline -- which watchpr prints on stderr (as a JSON
 // record under --json), so a run started against a conflicted or CI-red PR says
-// so. One case is never reported at all: a conflict introduced by the push
+// so. A conflict introduced by the push immediately before the watch started is
-// immediately before the watch started, on a PR whose head and base then never
+// not reported while the commits stay put, because nothing in Gitea's payload
-// move, stays silent for the whole run, because nothing in Gitea's payload
+// separates it from a merge check still in flight; the baseline line is the only
-// separates it from a merge check still in flight. The baseline line is the
+// notice of it. The first later move of the head or base arms the rule without
-// only notice of it.
+// alerting, and the window then runs from that arm rather than from the falses
+// that predate it. Moves after that neither re-arm nor restart the window -- a
+// restart on every base move could never complete on a busy base branch -- so a
+// conflict can be confirmed while the newest merge recompute is less than two
+// minutes old.
 //
 //	watchpr owner/repo#12 owner/repo:15
 //	watchpr --once --json owner/repo#12
@@ -73,7 +77,7 @@ func newRootCmd() *cobra.Command {
 				}
 				refs = append(refs, ref)
 			}
-			c := clientFor()
+			c := clientFor(jsonMode)
 			if once {
 				return runOnce(c, refs, jsonMode)
 			}
@@ -99,15 +103,35 @@ func newRootCmd() *cobra.Command {
 // clientFor builds the Gitea client. Watching public repos works anonymously,
 // so an unavailable token is a warning, not a failure; a poll that is actually
 // rejected re-mints then.
-func clientFor() *agent.GiteaClient {
+func clientFor(jsonMode bool) *agent.GiteaClient {
 	token, err := agent.GiteaToken()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "warning: no Gitea token (%v); polling anonymously\n", err)
+		warn(os.Stderr, jsonMode, "no Gitea token (%v); polling anonymously", err)
 		token = ""
 	}
 	return agent.NewGiteaClient(token)
 }
 
+// warnRecord is the --json form of a warning. Under --json every notice watchpr
+// itself writes to stderr -- warnings and the baseline -- is an NDJSON record,
+// so a caller parsing them line by line never has to guess which shape a line
+// is. A terminal failure is the one exception: SilenceErrors stays off, so
+// cobra prints it as a plain "Error: ..." line and the exit status is non-zero.
+type warnRecord struct {
+	Warning string `json:"warning"`
+}
+
+// warn writes a non-fatal notice to stderr in whichever form the caller asked
+// for.
+func warn(w io.Writer, jsonMode bool, format string, args ...any) {
+	msg := fmt.Sprintf(format, args...)
+	if jsonMode {
+		_ = json.NewEncoder(w).Encode(warnRecord{Warning: msg})
+		return
+	}
+	_, _ = fmt.Fprintf(w, "warning: %s\n", msg)
+}
+
 // runOnce fetches and prints the current state of each PR, then exits 0.
 func runOnce(c *agent.GiteaClient, refs []agent.PRRef, jsonMode bool) error {
 	login := agent.AgentLogin()
@@ -140,7 +164,7 @@ func runWatch(c *agent.GiteaClient, refs []agent.PRRef, interval time.Duration,
 		emitBaselines(os.Stderr, states, interval, jsonMode)
 	}
 	onError := func(ref agent.PRRef, err error) {
-		fmt.Fprintf(os.Stderr, "warning: polling %s: %v\n", ref.String(), err)
+		warn(os.Stderr, jsonMode, "polling %s: %v", ref.String(), err)
 	}
 
 	res, err := agent.Watch(c, refs, login, ticker.C, onBaseline, onError)
@@ -233,8 +257,11 @@ func baselineLine(st agent.PRState) string {
 
 func suppressedAtBaseline(st agent.PRState) string {
 	var conds []string
-	if st.Mergeable == agent.MergeNo {
+	switch st.Mergeable {
+	case agent.MergeNo:
 		conds = append(conds, "already non-mergeable (a real conflict, or Gitea still recomputing)")
+	case agent.MergeUnknown:
+		conds = append(conds, "mergeability unknown (the conflict rule is disarmed until Gitea reports mergeable or the commits move)")
 	}
 	if st.CIStatus == "failure" || st.CIStatus == "error" {
 		conds = append(conds, "CI already "+st.CIStatus)

cmd/watchpr/main_test.go

@@ -256,3 +256,58 @@ func TestBaselineIsEmittedInJSONMode(t *testing.T) {
 		t.Errorf("clean baseline = %q, want no suppression field", buf.String())
 	}
 }
+
+// Under --json stderr is the baseline and warning channel, so every notice
+// watchpr writes there has to be one shape: a caller parsing it line by line
+// must never meet a bare `warning:` line between two NDJSON records. Cobra's
+// terminal `Error: ...` line is not covered here -- SilenceErrors stays off, so
+// it is plain text on stderr alongside a non-zero exit.
+func TestJSONModeWarningsAndBaselineAreRecords(t *testing.T) {
+	st := agent.PRState{
+		Ref:       agent.PRRef{Owner: "unkin", Repo: "repo", Number: 7},
+		State:     "open",
+		Mergeable: agent.MergeNo,
+	}
+	var buf bytes.Buffer
+	warn(&buf, true, "no Gitea token (%v); polling anonymously", errors.New("vault refused"))
+	emitBaselines(&buf, []agent.PRState{st}, 30*time.Second, true)
+	warn(&buf, true, "polling %s: %v", st.Ref.String(), errors.New("HTTP 502"))
+
+	lines := strings.Split(strings.TrimSpace(buf.String()), "\n")
+	if len(lines) != 3 {
+		t.Fatalf("stderr = %q, want 3 records", buf.String())
+	}
+	for _, line := range lines {
+		var rec map[string]any
+		if err := json.Unmarshal([]byte(line), &rec); err != nil {
+			t.Errorf("stderr line %q is not a JSON record: %v", line, err)
+		}
+	}
+	var first warnRecord
+	if err := json.Unmarshal([]byte(lines[0]), &first); err != nil || !strings.Contains(first.Warning, "vault refused") {
+		t.Errorf("first record = %q, want the token warning", lines[0])
+	}
+
+	buf.Reset()
+	warn(&buf, false, "polling %s: %v", st.Ref.String(), errors.New("HTTP 502"))
+	if got := buf.String(); !strings.HasPrefix(got, "warning: ") {
+		t.Errorf("plain-mode warning = %q, want the warning: prefix", got)
+	}
+}
+
+// An unknown mergeability disarms the conflict rule exactly as a non-mergeable
+// baseline does, so the baseline has to name it too.
+func TestBaselineNamesAnUnknownMergeability(t *testing.T) {
+	st := agent.PRState{
+		Ref:       agent.PRRef{Owner: "unkin", Repo: "repo", Number: 7},
+		State:     "open",
+		Mergeable: agent.MergeUnknown,
+		CIStatus:  "success",
+	}
+	if got := suppressedAtBaseline(st); !strings.Contains(got, "mergeability unknown") {
+		t.Errorf("suppressed = %q, want it to name the unknown mergeability", got)
+	}
+	if got := baselineLine(st); !strings.Contains(got, "not alerting") {
+		t.Errorf("baselineLine = %q, want the suppression note", got)
+	}
+}

internal/agent/watch.go

@@ -161,20 +161,36 @@ const conflictWindow = 2 * time.Minute
 // 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. Arming and the run of falses are independent:
-// movement only arms, because a base branch that moves under the PR on every
+// movement only arms and never resets the run, because a base branch that moves
-// push to main would otherwise restart the run forever.
+// under the PR on every push to main would otherwise restart it forever. The
+// window is instead measured from the later of the run's start and the arm, so
+// an arming poll confirms nothing it has not itself outlasted.
 type prWatch struct {
 	prev  PRState
 	armed bool
+	// armedAt is when the rule armed mid-watch; nil while disarmed, and nil when
+	// the baseline armed it, since then there is no transition to measure from.
+	armedAt *time.Time
 	// conflictSince is when the current unbroken run of non-mergeable polls
-	// began; zero when no run is in progress.
+	// began; nil when no run is in progress.
-	conflictSince time.Time
+	conflictSince *time.Time
 }
 
 func newPRWatch(baseline PRState) *prWatch {
 	return &prWatch{prev: baseline, armed: baseline.Mergeable == MergeYes}
 }
 
+// arm records the disarmed->armed transition and when it happened. Re-arming is
+// a no-op, so a base branch moving under every poll advances nothing.
+func (w *prWatch) arm(now time.Time) {
+	if w.armed {
+		return
+	}
+	w.armed = true
+	at := now
+	w.armedAt = &at
+}
+
 // 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 {
@@ -187,28 +203,43 @@ func mergeInputsChanged(prev, cur PRState) bool {
 func (w *prWatch) track(st PRState, now time.Time) {
 	switch st.Mergeable {
 	case MergeNo:
-		if w.conflictSince.IsZero() {
+		if w.conflictSince == nil {
-			w.conflictSince = now
+			at := now
+			w.conflictSince = &at
 		}
 	case MergeYes:
-		w.armed = true
+		w.arm(now)
-		w.conflictSince = time.Time{}
+		w.conflictSince = nil
 	default:
-		w.conflictSince = time.Time{}
+		w.conflictSince = nil
 	}
 }
 
 // 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.conflictSince = time.Time{}
+	w.conflictSince = nil
+}
+
+// windowStart is the later of the run's first observation and the arm, so the
+// window always covers observations this watch can attribute a merge
+// computation to. A baseline arm records no time and leaves the run governing.
+func (w *prWatch) windowStart() *time.Time {
+	if w.conflictSince == nil {
+		return nil
+	}
+	if w.armedAt != nil && w.armedAt.After(*w.conflictSince) {
+		return w.armedAt
+	}
+	return w.conflictSince
 }
 
 // confirmed reports whether the run of non-mergeable observations has spanned
-// the recompute window. The run starts at its first observation, so a lone
+// the recompute window since it started counting, so neither a lone
-// non-mergeable poll never confirms anything whatever the interval.
+// non-mergeable poll nor the poll that armed the rule confirms anything.
 func (w *prWatch) confirmed(now time.Time) bool {
-	return !w.conflictSince.IsZero() && now.Sub(w.conflictSince) >= conflictWindow
+	start := w.windowStart()
+	return start != nil && now.Sub(*start) >= conflictWindow
 }
 
 // observe folds in the newest snapshot, taken at now, and reports whether the
@@ -216,7 +247,7 @@ func (w *prWatch) confirmed(now time.Time) bool {
 func (w *prWatch) observe(cur PRState, now time.Time) (bool, string) {
 	changed, reason := MeaningfulChange(w.prev, cur)
 	if mergeInputsChanged(w.prev, cur) {
-		w.armed = true
+		w.arm(now)
 	}
 	w.prev = cur
 	w.track(cur, now)

internal/agent/watch_test.go

@@ -1253,6 +1253,19 @@ func TestWatchConflictSequences(t *testing.T) {
 			name:     "a base moving under every poll still confirms a conflict",
 			interval: normal, baseline: yes, polls: movingBase(MergeNo, 20), want: conflict,
 		},
+		{
+			name:     "a base move does not confirm a run it took no part in",
+			interval: normal, baseline: no, polls: []PRState{no, no, no, movedNo}, want: "",
+		},
+		{
+			name:     "a push does not confirm a run it took no part in",
+			interval: normal, baseline: no, polls: []PRState{no, no, no, pushedNo}, want: "",
+		},
+		{
+			name:     "an armed run confirms a window after the arm, not before",
+			interval: normal, baseline: no,
+			polls: []PRState{no, no, no, movedNo, movedNo, movedNo}, want: conflict,
+		},
 		{
 			name:     "a conflict that predates the watch stays silent forever",
 			interval: relaxed, baseline: no, polls: repeat(no, 50), want: "",
@@ -1331,3 +1344,48 @@ func TestWatchConflictIsolatedFromANeighboursPushes(t *testing.T) {
 		t.Errorf("alert names %s, want %s", res.Ref, conflicted)
 	}
 }
+
+// The window is measured from the arm, not from the run the arming poll walked
+// in on: a base branch that moves under a long-conflicted PR arms the rule and
+// then has to outlast the recompute itself before anything is reported.
+func TestWatchConflictWindowStartsAtTheArm(t *testing.T) {
+	stuck := base()
+	stuck.Mergeable = MergeNo
+	moved := stuck
+	moved.BaseSHA = "base111"
+
+	f := &fakeFetcher{states: []PRState{stuck, stuck, stuck, stuck, moved, moved, moved}}
+	res, err := Watch(f, []PRRef{stuck.Ref}, "unkin-agent", spacedTicks(6, time.Minute), 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 two minutes after the base moved", res.Reason)
+	}
+	if f.calls != 7 {
+		t.Errorf("fetch calls = %d, want 7: the window runs from the arming poll, not from the run it inherited", f.calls)
+	}
+}
+
+// The zero time is a legal clock value, so it must not double as the "no run in
+// progress" sentinel: a caller whose ticks start at the zero time gets the same
+// debounce as any other.
+func TestWatchConflictWindowCountsFromTheZeroTime(t *testing.T) {
+	ok := base()
+	conflicted := base()
+	conflicted.Mergeable = MergeNo
+
+	ticks := make(chan time.Time, 2)
+	ticks <- time.Time{}
+	ticks <- time.Time{}.Add(10 * time.Minute)
+	close(ticks)
+
+	f := &fakeFetcher{states: []PRState{ok, conflicted, conflicted}}
+	res, err := Watch(f, []PRRef{ok.Ref}, "unkin-agent", ticks, 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 run starting at the zero time still counts", res.Reason)
+	}
+}