fix(collector): include fork pull requests in pr_open

External contributors open pull requests from forks. The pr_open alert now covers them, so the review action sees them. The consumer must treat fork pull requests as untrusted.

README.md

@@ -110,7 +110,7 @@ actions:
       token_env: ACTION_TOKEN
 ```
 
-- `pr_open` is an informational alert kind. It exists for each open, non-draft pull request from the same repository (not a fork) in a non-archived repo. `subject` is `#N title`, `url` is the pull request, `ref` is the head commit SHA and `detail` is `open for <age>`. It has severity `ok`: it never changes repo health, the counts, the metrics or the dashboard, and the UI hides it. It is in the `alerts` array of `GET /api/state`, which gains an optional `ref` field on every alert.
+- `pr_open` is an informational alert kind. It exists for each open, non-draft pull request including pull requests from forks, in a non-archived repo. Fork pull requests come from external contributors, so a consumer of these alerts, including an auto action webhook, must treat them as untrusted input. `subject` is `#N title`, `url` is the pull request, `ref` is the head commit SHA and `detail` is `open for <age>`. It has severity `ok`: it never changes repo health, the counts, the metrics or the dashboard, and the UI hides it. It is in the `alerts` array of `GET /api/state`, which gains an optional `ref` field on every alert.
 - `authors` limits auto sends to alerts that carry an author, which today means `pr_open`. Without `authors`, any alert of a listed kind fires.
 - The webhook request is the same as for a button press, plus `alert.ref` when the alert has one.
 - The server keeps the sent set in memory and drops an entry when its alert disappears, so a restart sends again for every open pull request. A failed send is not recorded and retries on the next scan. That covers a network error, a non-2xx answer and a `409` (the consumer is busy). Any other 2xx answer counts as sent.

config.example.yaml

@@ -26,7 +26,7 @@ thresholds: # negative disables a check
 #     status_url: https://example.internal/hooks/rerun/status # optional
 #     confirm: "Run this action for the selected alert?" # optional; omit for no dialog
 #   - id: review
-#     kinds: [pr_open] # required with auto; one alert per open, non-draft, non-fork PR
+#     kinds: [pr_open] # required with auto; one alert per open, non-draft PR, forks included; consumers must treat fork PRs as untrusted
 #     auto: true # fire without a click, once per repo, PR and head commit
 #     authors: [lukaszraczylo] # optional; case-insensitive; omit for any author
 #     webhook:

internal/collector/collector_test.go

@@ -93,7 +93,7 @@ func TestEvaluate(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			r := fresh()
 			tt.mutate(&r)
-			alerts := Evaluate(&r, t0, th())
+			alerts := withoutPROpen(Evaluate(&r, t0, th()))
 			got := kinds(alerts)
 			if len(got) != len(tt.want) {
 				t.Fatalf("alerts = %+v, want kinds %v", alerts, tt.want)
@@ -110,11 +110,22 @@ func TestEvaluate(t *testing.T) {
 	}
 }
 
+// withoutPROpen drops the informational pr_open alerts so tests of other kinds stay focused.
+func withoutPROpen(in []model.Alert) []model.Alert {
+	var out []model.Alert
+	for _, a := range in {
+		if a.Kind != KindPROpen {
+			out = append(out, a)
+		}
+	}
+	return out
+}
+
 func TestEvaluateDisabledThresholds(t *testing.T) {
 	r := model.Repo{FullName: "o/r", RefreshedAt: t0, PRs: []model.PullRequest{{Number: 1, Fork: true, CreatedAt: t0.Add(-100 * 24 * time.Hour)}},
 		Issues: []model.Issue{{UpdatedAt: t0.Add(-100 * 24 * time.Hour)}}}
 	off := config.Thresholds{PRWarnAfter: -1, PRCritAfter: -1, IssueStaleAfter: -1, WorkflowStuck: -1, DataStaleAfter: -1}
-	if a := Evaluate(&r, t0, off); len(a) != 0 {
+	if a := withoutPROpen(Evaluate(&r, t0, off)); len(a) != 0 {
 		t.Fatalf("disabled checks produced %+v", a)
 	}
 }
@@ -123,10 +134,10 @@ func TestAgeAdvancesWithoutRefresh(t *testing.T) {
 	r := model.Repo{FullName: "o/r", RefreshedAt: t0, PRs: []model.PullRequest{{Number: 1, Fork: true, CreatedAt: t0.Add(-40 * time.Hour)}}}
 	cfg := th()
 	cfg.DataStaleAfter = -1
-	if a := Evaluate(&r, t0, cfg); len(a) != 0 {
+	if a := withoutPROpen(Evaluate(&r, t0, cfg)); len(a) != 0 {
 		t.Fatalf("unexpected %+v", a)
 	}
-	if a := Evaluate(&r, t0.Add(10*time.Hour), cfg); len(a) != 1 || a[0].Severity != model.Warn {
+	if a := withoutPROpen(Evaluate(&r, t0.Add(10*time.Hour), cfg)); len(a) != 1 || a[0].Severity != model.Warn {
 		t.Fatalf("PR must cross the warn threshold as time passes: %+v", a)
 	}
 }
@@ -240,7 +251,8 @@ func TestViewSortsWorstFirstAndAlertsBySeverity(t *testing.T) {
 	if v.Repos[0].FullName != "o/crit" || v.Repos[1].FullName != "o/warn" || v.Repos[2].FullName != "o/ok" {
 		t.Fatalf("order: %s %s %s", v.Repos[0].FullName, v.Repos[1].FullName, v.Repos[2].FullName)
 	}
-	if len(v.Alerts) != 2 || v.Alerts[0].Severity != model.Crit || v.Crit != 1 || v.Warn != 1 {
+	alerts := withoutPROpen(v.Alerts)
+	if len(alerts) != 2 || alerts[0].Severity != model.Crit || v.Crit != 1 || v.Warn != 1 {
 		t.Fatalf("alerts: %+v", v.Alerts)
 	}
 }
@@ -470,7 +482,7 @@ func TestEvaluatePROpen(t *testing.T) {
 		want     bool
 	}{
 		"open":     {pr(func(*model.PullRequest) {}), false, true},
-		"fork":     {pr(func(p *model.PullRequest) { p.Fork = true }), false, false},
+		"fork":     {pr(func(p *model.PullRequest) { p.Fork = true }), false, true},
 		"draft":    {pr(func(p *model.PullRequest) { p.Draft = true }), false, false},
 		"archived": {pr(func(*model.PullRequest) {}), true, false},
 	}

internal/collector/health.go

@@ -64,10 +64,8 @@ func Evaluate(r *model.Repo, now time.Time, th config.Thresholds) []model.Alert
 		}
 		age := now.Sub(pr.CreatedAt)
 		subject := fmt.Sprintf("#%d %s", pr.Number, pr.Title)
-		if !pr.Fork {
+		alerts = append(alerts, model.Alert{Severity: model.OK, Repo: r.FullName, Kind: KindPROpen, Subject: subject,
-			alerts = append(alerts, model.Alert{Severity: model.OK, Repo: r.FullName, Kind: KindPROpen, Subject: subject,
+			Detail: "open for " + Age(age), URL: pr.URL, Ref: pr.HeadSHA, Author: pr.Author, Since: pr.CreatedAt})
-				Detail: "open for " + Age(age), URL: pr.URL, Ref: pr.HeadSHA, Author: pr.Author, Since: pr.CreatedAt})
-		}
 		switch {
 		case th.PRCritAfter > 0 && age >= th.PRCritAfter:
 			add(model.Crit, KindPRWaiting, subject, "open "+Age(age), pr.URL, pr.CreatedAt)

internal/web/auto_test.go

@@ -43,7 +43,10 @@ func TestAutoRunner(t *testing.T) {
 		authors []string
 		steps   []autoStep
 	}{
-		"fork skipped":  {steps: []autoStep{{[]model.PullRequest{autoPR(1, "a", "x", fork)}, 200, 0}}},
+		"fork triggers once per sha": {steps: []autoStep{
+			{[]model.PullRequest{autoPR(1, "a", "x", fork)}, 200, 1},
+			{[]model.PullRequest{autoPR(1, "a", "x", fork)}, 200, 0},
+		}},
 		"draft skipped": {steps: []autoStep{{[]model.PullRequest{autoPR(1, "a", "x", draft)}, 200, 0}}},
 		"author filter": {authors: []string{"Alice"}, steps: []autoStep{
 			{[]model.PullRequest{autoPR(1, "a", "bob", nil), autoPR(2, "b", "alice", nil)}, 200, 1},