fix(web): run automatic actions after a repository rescan

The rescan button of one repository now triggers automatic actions too. The webhook calls run in the background so the response does not wait for them.

README.md

@@ -96,7 +96,7 @@ actions:
 
 ### Automatic actions
 
-An action with `auto: true` fires without a click. After every scan, and once at startup, the server sends the webhook for each current alert whose kind is in `kinds`. It sends once per action, repo, alert URL and head commit. A new commit on a pull request is a new send.
+An action with `auto: true` fires without a click. After every scan, after every single-repository rescan, and once at startup, the server sends the webhook for each current alert whose kind is in `kinds`. It sends once per action, repo, alert URL and head commit. A new commit on a pull request is a new send.
 
 ```yaml
 actions:
@@ -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.go

@@ -119,10 +119,13 @@ func (c *Collector) OnRefresh(fn func(context.Context, View)) { c.onRefresh = ap
 
 func (c *Collector) refreshAndNotify(ctx context.Context) {
 	c.Refresh(ctx)
-	if len(c.onRefresh) == 0 || ctx.Err() != nil {
+	if ctx.Err() != nil {
 		return
 	}
-	v := c.View()
+	c.notify(ctx, c.View())
+}
+
+func (c *Collector) notify(ctx context.Context, v View) {
 	for _, fn := range c.onRefresh {
 		fn(ctx, v)
 	}
@@ -239,7 +242,12 @@ func (c *Collector) RefreshRepo(ctx context.Context, name string) (View, error)
 	c.mu.Lock()
 	c.repos[canon] = r
 	c.mu.Unlock()
-	return c.View(), nil
+	v := c.View()
+	if len(c.onRefresh) > 0 {
+		// The request context ends with the response, which must not wait for webhooks.
+		go c.notify(context.WithoutCancel(ctx), v)
+	}
+	return v, nil
 }
 
 func (c *Collector) configured(name string) (string, bool) {

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},
 	}
@@ -507,3 +519,37 @@ func TestPROpenDoesNotChangeCounts(t *testing.T) {
 		t.Fatalf("view = %+v", v)
 	}
 }
+
+func TestRefreshRepoRunsRefreshHooksWithTheNewView(t *testing.T) {
+	f := &fakeFetcher{fn: func(name string) (model.Repo, error) {
+		return model.Repo{FullName: name, URL: "u", Issues: make([]model.Issue, 3)}, nil
+	}}
+	c := newCollector(f, "o/a")
+	got := make(chan View, 1)
+	c.OnRefresh(func(_ context.Context, v View) { got <- v })
+	if _, err := c.RefreshRepo(context.Background(), "o/a"); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case v := <-got:
+		if len(v.Repos) != 1 || len(v.Repos[0].Issues) != 3 {
+			t.Fatalf("hook saw a stale view: %+v", v.Repos)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("refresh hook did not run after a repository refresh")
+	}
+}
+
+func TestRefreshRepoFailureSkipsRefreshHooks(t *testing.T) {
+	c := newCollector(&fakeFetcher{fn: func(string) (model.Repo, error) { return model.Repo{}, errors.New("nope") }}, "o/a")
+	called := make(chan struct{}, 1)
+	c.OnRefresh(func(context.Context, View) { called <- struct{}{} })
+	if _, err := c.RefreshRepo(context.Background(), "unknown/x"); err == nil {
+		t.Fatal("unknown repository must fail")
+	}
+	select {
+	case <-called:
+		t.Fatal("hook ran for a refused refresh")
+	case <-time.After(200 * time.Millisecond):
+	}
+}

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,
-				Detail: "open for " + Age(age), URL: pr.URL, Ref: pr.HeadSHA, Author: pr.Author, Since: pr.CreatedAt})
-		}
+		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})
 		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},