diff --git a/AGENTS.md b/AGENTS.md index ab4c09e..b5772f1 100644 --- a/AGENTS.md +++ b/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: - movement arms and nothing else, since `base.sha` is the base branch tip as of - the response, so it moves for every open PR whenever main does and a run reset - there could never complete on a busy repo. `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 + `--interval` spans seconds to hours. The window starts at the later of the + run's first observation and the arm, which is what keeps arming and the run + independent: movement arms without resetting the run — `base.sha` is the base + branch tip as of the response, so it moves for every open PR whenever main does + and a reset there could never complete on a busy repo — while an arming poll + 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 - reported; Gitea's payload has no field separating it from a check in flight - (`merge_base` is the true merge base and does not move on recheck). + before the watch began is unreported while the commits stay put, since Gitea's + payload has no field separating it from a check in flight (`merge_base` is the + 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 diff --git a/README.md b/README.md index 3ef3cac..a0a0590 100644 --- a/README.md +++ b/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 -two minutes. 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. +base commit moved) **and** the non-mergeable polls have **then** run unbroken for +two minutes. The window runs from whichever came later, so the poll that starts +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 -immediately before the watch started, on a PR whose head and base never move -again. Nothing in Gitea's payload separates that from a merge check still in -flight, so watchpr stays silent about it for as long as it runs — the baseline -line is how you see it. +So a conflict introduced by the push immediately before the watch started is not +reported while the commits stay put: nothing in Gitea's payload separates it from +a merge check still in flight, and the baseline line is the only notice of it. +That is a narrower gap than it looks, because `base.sha` is the base branch's +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) diff --git a/cmd/watchpr/main.go b/cmd/watchpr/main.go index 2f49166..a5a155a 100644 --- a/cmd/watchpr/main.go +++ b/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 -// immediately before the watch started, on a PR whose head and base then never -// 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 notice of it. +// so. A conflict introduced by the push immediately before the watch started is +// not reported while the commits stay put, because nothing in Gitea's payload +// separates it from a merge check still in flight; the baseline line is the only +// notice of it. The first later move of the head or base arms the rule without +// 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) diff --git a/cmd/watchpr/main_test.go b/cmd/watchpr/main_test.go index f9d2d25..62340f1 100644 --- a/cmd/watchpr/main_test.go +++ b/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) + } +} diff --git a/internal/agent/watch.go b/internal/agent/watch.go index 5cf8488..ac6be24 100644 --- a/internal/agent/watch.go +++ b/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 -// push to main would otherwise restart the run forever. +// movement only arms and never resets the run, because a base branch that moves +// 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. - conflictSince time.Time + // began; nil when no run is in progress. + 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() { - w.conflictSince = now + if w.conflictSince == nil { + at := now + w.conflictSince = &at } case MergeYes: - w.armed = true - w.conflictSince = time.Time{} + w.arm(now) + 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 -// non-mergeable poll never confirms anything whatever the interval. +// the recompute window since it started counting, so neither a lone +// 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) diff --git a/internal/agent/watch_test.go b/internal/agent/watch_test.go index 630aa21..b153fbc 100644 --- a/internal/agent/watch_test.go +++ b/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) + } +}