From c2ce5391c07cc78a47c92072ebd3165f3bd57320 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Mon, 5 Oct 2026 01:00:18 +0100 Subject: [PATCH] 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 | 2 +- config.example.yaml | 2 +- internal/collector/collector_test.go | 24 ++++++++++++++++++------ internal/collector/health.go | 6 ++---- internal/web/auto_test.go | 5 ++++- 5 files changed, 26 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 660c329..9d2ae73 100644 --- a/README.md +++ b/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 `. 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 `. 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. diff --git a/config.example.yaml b/config.example.yaml index df672ae..6fd9433 100644 --- a/config.example.yaml +++ b/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: diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go index 3275b4b..b6e1d9e 100644 --- a/internal/collector/collector_test.go +++ b/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}, } diff --git a/internal/collector/health.go b/internal/collector/health.go index 2f5b75c..9e80541 100644 --- a/internal/collector/health.go +++ b/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) diff --git a/internal/web/auto_test.go b/internal/web/auto_test.go index f550253..4cca614 100644 --- a/internal/web/auto_test.go +++ b/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},