diff --git a/appview/models/webhook.go b/appview/models/webhook.go index 9803dd83..b3068d2f 100644 --- a/appview/models/webhook.go +++ b/appview/models/webhook.go @@ -10,8 +10,13 @@ import ( type WebhookEvent string const ( - WebhookEventPush WebhookEvent = "push" - WebhookEventRepoRenamed WebhookEvent = "repository:renamed" + WebhookEventPush WebhookEvent = "push" + WebhookEventRepoRenamed WebhookEvent = "repository:renamed" + WebhookEventPullRequestCreated WebhookEvent = "pull_request:created" + WebhookEventPullRequestResubmitted WebhookEvent = "pull_request:resubmitted" + WebhookEventPullRequestMerged WebhookEvent = "pull_request:merged" + WebhookEventPullRequestClosed WebhookEvent = "pull_request:closed" + WebhookEventPullRequestReopened WebhookEvent = "pull_request:reopened" ) type Webhook struct { @@ -81,3 +86,34 @@ type WebhookRenamePayload struct { Repository WebhookRepository `json:"repository"` Sender WebhookUser `json:"sender"` } + +// WebhookPullRequestPayload represents the payload for pull_request:* events +type WebhookPullRequestPayload struct { + Action string `json:"action"` + PullRequest WebhookPullRequest `json:"pull_request"` + Repository WebhookRepository `json:"repository"` + Sender WebhookUser `json:"sender"` +} + +// WebhookPullRequest represents pull request information in webhook payload +type WebhookPullRequest struct { + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + TargetBranch string `json:"target_branch"` + Source *WebhookPullRequestSource `json:"source,omitempty"` + RoundNumber int `json:"round_number"` + Owner WebhookUser `json:"owner"` + HtmlUrl string `json:"html_url"` + PatchUrl string `json:"patch_url"` + CreatedAt string `json:"created_at"` +} + +// WebhookPullRequestSource represents the source of a branch- or fork-based +// pull request; absent for patch-based pull requests +type WebhookPullRequestSource struct { + Branch string `json:"branch"` + Repo string `json:"repo,omitempty"` + Sha string `json:"sha,omitempty"` +} diff --git a/appview/notify/db/db.go b/appview/notify/db/db.go index 0daab351..ca7c56d0 100644 --- a/appview/notify/db/db.go +++ b/appview/notify/db/db.go @@ -474,6 +474,10 @@ func (n *databaseNotifier) NewIssueState(ctx context.Context, actor syntax.DID, ) } +func (n *databaseNotifier) ResubmitPull(ctx context.Context, pull *models.Pull) { + // no-op for now +} + func (n *databaseNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) { l := log.FromContext(ctx) diff --git a/appview/notify/logging/notifier.go b/appview/notify/logging/notifier.go index e414b8d8..b031c992 100644 --- a/appview/notify/logging/notifier.go +++ b/appview/notify/logging/notifier.go @@ -95,6 +95,11 @@ func (l *loggingNotifier) NewPull(ctx context.Context, pull *models.Pull) { l.inner.NewPull(ctx, pull) } +func (l *loggingNotifier) ResubmitPull(ctx context.Context, pull *models.Pull) { + ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "ResubmitPull")) + l.inner.ResubmitPull(ctx, pull) +} + func (l *loggingNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) { ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewPullState")) l.inner.NewPullState(ctx, actor, pull) diff --git a/appview/notify/merged_notifier.go b/appview/notify/merged_notifier.go index 252db108..00ae83de 100644 --- a/appview/notify/merged_notifier.go +++ b/appview/notify/merged_notifier.go @@ -90,6 +90,10 @@ func (m *mergedNotifier) NewPull(ctx context.Context, pull *models.Pull) { m.fanout(func(n Notifier) { n.NewPull(ctx, pull) }) } +func (m *mergedNotifier) ResubmitPull(ctx context.Context, pull *models.Pull) { + m.fanout(func(n Notifier) { n.ResubmitPull(ctx, pull) }) +} + func (m *mergedNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) { m.fanout(func(n Notifier) { n.NewPullState(ctx, actor, pull) }) } diff --git a/appview/notify/notifier.go b/appview/notify/notifier.go index 52bed5ad..dca94357 100644 --- a/appview/notify/notifier.go +++ b/appview/notify/notifier.go @@ -26,6 +26,7 @@ type Notifier interface { DeleteFollow(ctx context.Context, follow *models.Follow) NewPull(ctx context.Context, pull *models.Pull) + ResubmitPull(ctx context.Context, pull *models.Pull) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) NewIssueLabelOp(ctx context.Context, actor syntax.DID, issue *models.Issue, ops []models.LabelOp) @@ -72,6 +73,7 @@ func (m *BaseNotifier) NewFollow(ctx context.Context, follow *models.Follow) func (m *BaseNotifier) DeleteFollow(ctx context.Context, follow *models.Follow) {} func (m *BaseNotifier) NewPull(ctx context.Context, pull *models.Pull) {} +func (m *BaseNotifier) ResubmitPull(ctx context.Context, pull *models.Pull) {} func (m *BaseNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) {} func (m *BaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) {} diff --git a/appview/notify/webhook/notifier.go b/appview/notify/webhook/notifier.go index 0b0f9d92..bf0ebe9c 100644 --- a/appview/notify/webhook/notifier.go +++ b/appview/notify/webhook/notifier.go @@ -20,19 +20,22 @@ import ( "tangled.org/core/appview/models" "tangled.org/core/appview/notify" "tangled.org/core/log" + "tangled.org/core/orm" ) type Notifier struct { notify.BaseNotifier - db *db.DB - logger *slog.Logger - client *http.Client + db *db.DB + baseUrl string + logger *slog.Logger + client *http.Client } -func NewNotifier(database *db.DB) *Notifier { +func NewNotifier(database *db.DB, baseUrl string) *Notifier { return &Notifier{ - db: database, - logger: log.New("webhook-notifier"), + db: database, + baseUrl: baseUrl, + logger: log.New("webhook-notifier"), client: &http.Client{ Timeout: 30 * time.Second, }, @@ -92,6 +95,109 @@ func (w *Notifier) RenameRepo(ctx context.Context, actor syntax.DID, oldRepo, ne } } +func (w *Notifier) NewPull(ctx context.Context, pull *models.Pull) { + w.pullRequestEvent(ctx, models.WebhookEventPullRequestCreated, "created", pull.OwnerDid, pull) +} + +func (w *Notifier) ResubmitPull(ctx context.Context, pull *models.Pull) { + w.pullRequestEvent(ctx, models.WebhookEventPullRequestResubmitted, "resubmitted", pull.OwnerDid, pull) +} + +func (w *Notifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) { + event, action, ok := pullStateEvent(pull.State) + if !ok { + return + } + w.pullRequestEvent(ctx, event, action, actor.String(), pull) +} + +// pullStateEvent maps a pull's state to the webhook event announcing the +// transition into that state +func pullStateEvent(state models.PullState) (models.WebhookEvent, string, bool) { + switch state { + case models.PullMerged: + return models.WebhookEventPullRequestMerged, "merged", true + case models.PullClosed: + return models.WebhookEventPullRequestClosed, "closed", true + case models.PullOpen: + return models.WebhookEventPullRequestReopened, "reopened", true + default: + return "", "", false + } +} + +func (w *Notifier) pullRequestEvent(ctx context.Context, event models.WebhookEvent, action, sender string, pull *models.Pull) { + // pull request events originate from http handlers, whose context is + // canceled as soon as the handler returns; detach so in-flight + // deliveries are not cut short + ctx = context.WithoutCancel(ctx) + + webhooks, err := w.activeWebhooksForEvent(string(pull.RepoDid), event) + if err != nil { + w.logger.Error("failed to get webhooks for repo", "repo_did", pull.RepoDid, "err", err) + return + } + if len(webhooks) == 0 { + return + } + + repo, err := db.GetRepo(w.db, orm.FilterEq("repo_did", string(pull.RepoDid))) + if err != nil { + w.logger.Error("failed to get repo", "repo_did", pull.RepoDid, "err", err) + return + } + + payload := buildPullRequestPayload(action, repo, pull, sender, w.baseUrl) + payloadBytes, err := json.Marshal(payload) + if err != nil { + w.logger.Error("failed to marshal pull request payload", "repo_did", pull.RepoDid, "err", err) + return + } + + userAgent := "Tangled-Hook/pull_request" + for _, webhook := range webhooks { + go w.sendWebhook(ctx, webhook, string(event), payload.Repository.FullName, userAgent, payloadBytes) + } +} + +func buildPullRequestPayload(action string, repo *models.Repo, pull *models.Pull, sender, baseUrl string) *models.WebhookPullRequestPayload { + htmlUrl := fmt.Sprintf("%s/%s/%s/pulls/%d", baseUrl, repo.Did, repo.Slug(), pull.PullId) + + pullRequest := models.WebhookPullRequest{ + Number: pull.PullId, + Title: pull.Title, + Body: pull.Body, + State: pull.State.String(), + TargetBranch: pull.TargetBranch, + Owner: models.WebhookUser{Did: pull.OwnerDid}, + HtmlUrl: htmlUrl, + CreatedAt: pull.Created.Format(time.RFC3339), + } + if len(pull.Submissions) > 0 { + pullRequest.RoundNumber = pull.LastRoundNumber() + pullRequest.PatchUrl = fmt.Sprintf("%s/round/%d.patch", htmlUrl, pull.LastRoundNumber()) + } + if pull.PullSource != nil { + source := &models.WebhookPullRequestSource{ + Branch: pull.PullSource.Branch, + } + if len(pull.Submissions) > 0 { + source.Sha = pull.LatestSha() + } + if pull.IsForkBased() { + source.Repo = pull.PullSource.RepoDid.String() + } + pullRequest.Source = source + } + + return &models.WebhookPullRequestPayload{ + Action: action, + PullRequest: pullRequest, + Repository: buildWebhookRepository(repo), + Sender: models.WebhookUser{Did: sender}, + } +} + func (w *Notifier) activeWebhooksForEvent(repoDid string, event models.WebhookEvent) ([]models.Webhook, error) { webhooks, err := db.GetActiveWebhooksForRepo(w.db, repoDid) if err != nil { diff --git a/appview/notify/webhook/notifier_test.go b/appview/notify/webhook/notifier_test.go new file mode 100644 index 00000000..129fbfa5 --- /dev/null +++ b/appview/notify/webhook/notifier_test.go @@ -0,0 +1,333 @@ +package webhook + +import ( + "context" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + "time" + + "github.com/bluesky-social/indigo/atproto/syntax" + "tangled.org/core/appview/db" + "tangled.org/core/appview/models" +) + +func TestPullStateEvent(t *testing.T) { + tests := []struct { + name string + state models.PullState + wantEvent models.WebhookEvent + wantAction string + wantOk bool + }{ + {"merged", models.PullMerged, models.WebhookEventPullRequestMerged, "merged", true}, + {"closed", models.PullClosed, models.WebhookEventPullRequestClosed, "closed", true}, + {"reopened", models.PullOpen, models.WebhookEventPullRequestReopened, "reopened", true}, + {"abandoned", models.PullAbandoned, "", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + event, action, ok := pullStateEvent(tt.state) + if event != tt.wantEvent || action != tt.wantAction || ok != tt.wantOk { + t.Errorf("pullStateEvent(%v) = (%q, %q, %v), want (%q, %q, %v)", + tt.state, event, action, ok, tt.wantEvent, tt.wantAction, tt.wantOk) + } + }) + } +} + +func TestBuildPullRequestPayload(t *testing.T) { + const baseUrl = "https://tangled.org" + + targetDid := syntax.DID("did:plc:target") + forkDid := syntax.DID("did:plc:fork") + + repo := &models.Repo{ + Did: "did:plc:target", + Name: "some-repo", + Knot: "knot.example.com", + Rkey: "some-repo", + Created: time.Date(2025, 9, 15, 8, 57, 23, 0, time.UTC), + } + + basePull := func() models.Pull { + return models.Pull{ + PullId: 4, + RepoDid: targetDid, + OwnerDid: "did:plc:author", + Title: "add dark mode", + Body: "implements dark mode", + TargetBranch: "main", + State: models.PullOpen, + Created: time.Date(2025, 9, 16, 10, 0, 0, 0, time.UTC), + Submissions: []*models.PullSubmission{ + {RoundNumber: 0, SourceRev: "aaaa000"}, + {RoundNumber: 1, SourceRev: "bbbb111"}, + }, + } + } + + t.Run("patch based", func(t *testing.T) { + pull := basePull() + pull.PullSource = nil + + payload := buildPullRequestPayload("created", repo, &pull, "did:plc:author", baseUrl) + + if payload.Action != "created" { + t.Errorf("action = %q, want %q", payload.Action, "created") + } + pr := payload.PullRequest + if pr.Number != 4 { + t.Errorf("number = %d, want 4", pr.Number) + } + if pr.State != "open" { + t.Errorf("state = %q, want %q", pr.State, "open") + } + if pr.Source != nil { + t.Errorf("source = %+v, want nil for patch-based pull", pr.Source) + } + if pr.RoundNumber != 1 { + t.Errorf("round_number = %d, want 1", pr.RoundNumber) + } + wantHtmlUrl := "https://tangled.org/did:plc:target/some-repo/pulls/4" + if pr.HtmlUrl != wantHtmlUrl { + t.Errorf("html_url = %q, want %q", pr.HtmlUrl, wantHtmlUrl) + } + wantPatchUrl := wantHtmlUrl + "/round/1.patch" + if pr.PatchUrl != wantPatchUrl { + t.Errorf("patch_url = %q, want %q", pr.PatchUrl, wantPatchUrl) + } + if pr.Owner.Did != "did:plc:author" { + t.Errorf("owner.did = %q, want %q", pr.Owner.Did, "did:plc:author") + } + if payload.Sender.Did != "did:plc:author" { + t.Errorf("sender.did = %q, want %q", payload.Sender.Did, "did:plc:author") + } + if payload.Repository.FullName != "did:plc:target/some-repo" { + t.Errorf("repository.full_name = %q, want %q", payload.Repository.FullName, "did:plc:target/some-repo") + } + }) + + t.Run("branch based", func(t *testing.T) { + pull := basePull() + pull.PullSource = &models.PullSource{ + Branch: "dark-mode", + RepoDid: &targetDid, + } + + payload := buildPullRequestPayload("merged", repo, &pull, "did:plc:merger", baseUrl) + + pr := payload.PullRequest + if pr.Source == nil { + t.Fatal("source = nil, want non-nil for branch-based pull") + } + if pr.Source.Branch != "dark-mode" { + t.Errorf("source.branch = %q, want %q", pr.Source.Branch, "dark-mode") + } + if pr.Source.Repo != "" { + t.Errorf("source.repo = %q, want empty for branch-based pull", pr.Source.Repo) + } + if pr.Source.Sha != "bbbb111" { + t.Errorf("source.sha = %q, want %q", pr.Source.Sha, "bbbb111") + } + if payload.Sender.Did != "did:plc:merger" { + t.Errorf("sender.did = %q, want %q", payload.Sender.Did, "did:plc:merger") + } + }) + + t.Run("fork based", func(t *testing.T) { + pull := basePull() + pull.PullSource = &models.PullSource{ + Branch: "dark-mode", + RepoDid: &forkDid, + } + + payload := buildPullRequestPayload("created", repo, &pull, "did:plc:author", baseUrl) + + pr := payload.PullRequest + if pr.Source == nil { + t.Fatal("source = nil, want non-nil for fork-based pull") + } + if pr.Source.Repo != "did:plc:fork" { + t.Errorf("source.repo = %q, want %q", pr.Source.Repo, "did:plc:fork") + } + }) + + t.Run("no submissions", func(t *testing.T) { + pull := basePull() + pull.Submissions = nil + + payload := buildPullRequestPayload("created", repo, &pull, "did:plc:author", baseUrl) + + pr := payload.PullRequest + if pr.RoundNumber != 0 { + t.Errorf("round_number = %d, want 0", pr.RoundNumber) + } + if pr.PatchUrl != "" { + t.Errorf("patch_url = %q, want empty when there are no submissions", pr.PatchUrl) + } + }) +} + +type notifierTestEnv struct { + notifier *Notifier + webhook *models.Webhook + db *db.DB + received chan string +} + +// newNotifierTestEnv sets up a real sqlite db with a repo and a webhook +// subscribed to the given events, delivering to a local test server +func newNotifierTestEnv(t *testing.T, events []string) *notifierTestEnv { + t.Helper() + + d, err := db.Make(context.Background(), filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("Make: %v", err) + } + t.Cleanup(func() { d.Close() }) + + tx, err := d.Begin() + if err != nil { + t.Fatalf("Begin: %v", err) + } + if err := db.AddRepo(tx, &models.Repo{ + Did: "did:plc:owner", + Name: "some-repo", + Knot: "knot.example.com", + Rkey: "some-repo", + RepoDid: "did:plc:repo1", + }); err != nil { + t.Fatalf("AddRepo: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + + received := make(chan string, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + received <- r.Header.Get("X-Tangled-Event") + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + webhook := &models.Webhook{ + RepoDid: syntax.DID("did:plc:repo1"), + Url: srv.URL, + Active: true, + Events: events, + } + if err := db.AddWebhook(d, webhook); err != nil { + t.Fatalf("AddWebhook: %v", err) + } + + return ¬ifierTestEnv{ + notifier: NewNotifier(d, "https://tangled.org"), + webhook: webhook, + db: d, + received: received, + } +} + +func (env *notifierTestEnv) awaitDelivery(t *testing.T, wantEvent string) { + t.Helper() + + select { + case event := <-env.received: + if event != wantEvent { + t.Errorf("X-Tangled-Event = %q, want %q", event, wantEvent) + } + case <-time.After(10 * time.Second): + t.Fatalf("webhook %s not delivered", wantEvent) + } + + // wait for the delivery record so the sender goroutine finishes + // before the db is closed + deadline := time.Now().Add(10 * time.Second) + for { + deliveries, err := db.GetWebhookDeliveries(env.db, env.webhook.Id, 10) + if err == nil && len(deliveries) > 0 { + if !deliveries[0].Success { + t.Errorf("delivery recorded as failed, want success") + } + return + } + if time.Now().After(deadline) { + t.Fatal("delivery record not written") + } + time.Sleep(10 * time.Millisecond) + } +} + +func testPull(state models.PullState) *models.Pull { + return &models.Pull{ + PullId: 1, + RepoDid: syntax.DID("did:plc:repo1"), + OwnerDid: "did:plc:author", + Title: "hello", + TargetBranch: "main", + State: state, + Created: time.Now(), + } +} + +// Pull request events fire from http handlers, whose request context is +// canceled as soon as the handler returns. Deliveries run in background +// goroutines and must not be cut short by that cancellation. +func TestPullRequestEventDeliversAfterContextCancel(t *testing.T) { + env := newNotifierTestEnv(t, []string{string(models.WebhookEventPullRequestCreated)}) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + env.notifier.NewPull(ctx, testPull(models.PullOpen)) + env.awaitDelivery(t, "pull_request:created") +} + +func TestNotifierDeliversPullRequestEvents(t *testing.T) { + allEvents := []string{ + string(models.WebhookEventPullRequestCreated), + string(models.WebhookEventPullRequestResubmitted), + string(models.WebhookEventPullRequestMerged), + string(models.WebhookEventPullRequestClosed), + string(models.WebhookEventPullRequestReopened), + } + actor := syntax.DID("did:plc:actor") + + tests := []struct { + name string + notify func(*Notifier, context.Context) + wantEvent string + }{ + { + "resubmitted", + func(n *Notifier, ctx context.Context) { n.ResubmitPull(ctx, testPull(models.PullOpen)) }, + "pull_request:resubmitted", + }, + { + "merged", + func(n *Notifier, ctx context.Context) { n.NewPullState(ctx, actor, testPull(models.PullMerged)) }, + "pull_request:merged", + }, + { + "closed", + func(n *Notifier, ctx context.Context) { n.NewPullState(ctx, actor, testPull(models.PullClosed)) }, + "pull_request:closed", + }, + { + "reopened", + func(n *Notifier, ctx context.Context) { n.NewPullState(ctx, actor, testPull(models.PullOpen)) }, + "pull_request:reopened", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + env := newNotifierTestEnv(t, allEvents) + tt.notify(env.notifier, context.Background()) + env.awaitDelivery(t, tt.wantEvent) + }) + } +} diff --git a/appview/pages/templates/repo/settings/hooks.html b/appview/pages/templates/repo/settings/hooks.html index fb43df6c..d1437b70 100644 --- a/appview/pages/templates/repo/settings/hooks.html +++ b/appview/pages/templates/repo/settings/hooks.html @@ -229,8 +229,28 @@ Repository renamed +
- Additional event types (pull requests, issues) will be available in future updates. + Additional event types (issues) will be available in future updates.
@@ -299,9 +319,19 @@- Additional event types (pull requests, issues) will be available in future updates. + Additional event types (issues) will be available in future updates.