!239 · krz/gitbay

e2e/webhook_test.go +53

	@@ -254,3 +254,56 @@ func TestWebhooks(t *testing.T) {
254	t.Fatal("non-http scheme accepted")	254	t.Fatal("non-http scheme accepted")
255	}	255	}
256	}	256	}
		257
		258	// TestWebhookEventCoverage drives the mutations #112 found unrecorded and
		259	// asserts each reaches a subscriber. Half the forge's mutations recorded
		260	// nothing, so a webhook could be subscribed to them and never fire.
		261	func TestWebhookEventCoverage(t *testing.T) {
		262	inst := startInstance(t)
		263	aliceKey := inst.newKey(t, "alice")
		264	bobKey := inst.newKey(t, "bob")
		265	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
		266	inst.admin(t, "admin", "user", "create", "bob", "--key", bobKey+".pub")
		267	if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/app"); code != 0 {
		268	t.Fatalf("repo create: %s", errOut)
		269	}
		270	if _, _, code := inst.ssh(t, aliceKey, "", "repo", "access", "grant", "alice/app", "bob", "write"); code != 0 {
		271	t.Fatal("grant failed")
		272	}
		273
		274	// A name that is not an event is refused rather than silently never
		275	// firing.
		276	if _, errOut, code := inst.ssh(t, aliceKey, "", "webhook", "add", "alice/app",
		277	"https://example.test/h", "--events", "issue.tagged"); code != 2 \|\|
		278	!strings.Contains(errOut, "not an event this forge records") {
		279	t.Fatalf("bad event name: exit %d, %s", code, errOut)
		280	}
		281
		282	// Drive each newly recorded mutation.
		283	steps := [][]string{
		284	{"issue", "create", "alice/app", "--title", "'first'", "--body", "'b'"},
		285	{"issue", "edit", "alice/app", "1", "--title", "'renamed'"},
		286	{"issue", "label", "alice/app", "1", "--add", "bug"},
		287	{"issue", "assign", "alice/app", "1", "--add", "bob"},
		288	{"milestone", "create", "alice/app", "v1"},
		289	{"issue", "milestone", "alice/app", "1", "v1"},
		290	}
		291	for _, s := range steps {
		292	if _, errOut, code := inst.ssh(t, aliceKey, "", s...); code != 0 {
		293	t.Fatalf("%v: %s", s, errOut)
		294	}
		295	}
		296
		297	out, errOut, code := inst.ssh(t, aliceKey, "", "feed", "--json")
		298	if code != 0 {
		299	t.Fatalf("feed: %s", errOut)
		300	}
		301	for _, kind := range []string{
		302	"issue.created", "issue.edited", "issue.labeled",
		303	"issue.assigned", "issue.milestoned",
		304	} {
		305	if !strings.Contains(out, `"`+kind+`"`) {
		306	t.Errorf("%s was not recorded:\n%s", kind, out)
		307	}
		308	}
		309	}

internal/control/events.go added +66

@@ -0,0 +1,66 @@
	1	package control
	2
	3	import (
	4	"slices"
	5	"strings"
	6
	7	"gitbay.org/gitbay/internal/protocol"
	8	)
	9
	10	// EventKinds is every event this forge records, and so every value a
	11	// webhook's `--events` filter may name. It is the published list: the API
	12	// wiki page renders it, and TestEventKindsAreRecorded asserts the code
	13	// still emits exactly these, so the documentation cannot drift from the
	14	// server (#112).
	15	//
	16	// Not here, deliberately: repo.deleted. events.repo_id and
	17	// webhooks.repo_id both cascade from repos, so recording one would delete
	18	// it and every webhook that could have received it in the same statement.
	19	var EventKinds = []string{
	20	"build.cancelled",
	21	"build.failure",
	22	"build.success",
	23	"issue.assigned",
	24	"issue.closed",
	25	"issue.commented",
	26	"issue.created",
	27	"issue.edited",
	28	"issue.labeled",
	29	"issue.milestoned",
	30	"issue.open",
	31	"mr.closed",
	32	"mr.commented",
	33	"mr.created",
	34	"mr.edited",
	35	"mr.merged",
	36	"mr.milestoned",
	37	"mr.retargeted",
	38	"mr.reviewed",
	39	"push",
	40	"release.created",
	41	"release.deleted",
	42	"repo.archived",
	43	"repo.imported",
	44	"repo.unarchived",
	45	"status",
	46	}
	47
	48	// checkEventNames refuses a --events list naming something this forge
	49	// never emits. "*" is every kind. -1 means proceed.
	50	func checkEventNames(c *Ctx, events string) int {
	51	if events == "*" {
	52	return -1
	53	}
	54	for _, k := range strings.Split(events, ",") {
	55	k = strings.TrimSpace(k)
	56	if k == "" {
	57	return c.fail(protocol.ExitUsage, "empty event name in --events")
	58	}
	59	if !slices.Contains(EventKinds, k) {
	60	return c.fail(protocol.ExitUsage,
	61	"%q is not an event this forge records; known events are: %s",
	62	k, strings.Join(EventKinds, ", "))
	63	}
	64	}
	65	return -1
	66	}

internal/control/events_test.go added +70

@@ -0,0 +1,70 @@
	1	package control
	2
	3	import (
	4	"os"
	5	"path/filepath"
	6	"regexp"
	7	"slices"
	8	"strings"
	9	"testing"
	10	)
	11
	12	// recordEventKind pulls the kind out of a RecordEvent call. The kind is
	13	// either a literal or a literal prefix concatenated with a variable, and
	14	// the second shape is why this reads source rather than trusting a list.
	15	var recordEventKind = regexp.MustCompile(`RecordEvent\([^,]+,\s[^,]+,\s"([a-z.]+)"`)
	16
	17	// TestEventKindsAreRecorded keeps the published list and the code
	18	// together: an event added without documenting it, or documented without
	19	// being emitted, fails here. Webhook subscribers filter on these strings,
	20	// so a name that exists in only one of the two places is a subscription
	21	// that silently never fires (#112).
	22	func TestEventKindsAreRecorded(t *testing.T) {
	23	emitted := map[string]bool{}
	24	roots := []string{".", filepath.Join("..", "hookd")}
	25	for _, root := range roots {
	26	entries, err := os.ReadDir(root)
	27	if err != nil {
	28	t.Fatal(err)
	29	}
	30	for _, e := range entries {
	31	if !strings.HasSuffix(e.Name(), ".go") \|\| strings.HasSuffix(e.Name(), "_test.go") {
	32	continue
	33	}
	34	src, err := os.ReadFile(filepath.Join(root, e.Name()))
	35	if err != nil {
	36	t.Fatal(err)
	37	}
	38	for _, m := range recordEventKind.FindAllStringSubmatch(string(src), -1) {
	39	emitted[m[1]] = true
	40	}
	41	}
	42	}
	43	// The kinds built from a variable suffix, which the pattern above
	44	// sees only as their prefix. Each is spelled out so the list stays
	45	// exhaustive rather than approximate.
	46	for _, k := range []string{
	47	"build.success", "build.failure", // "build."+args[1]
	48	"issue.closed", "issue.open", // "issue."+state
	49	"repo.archived", "repo.unarchived", // "repo."+verb+"d"
	50	"issue.commented", "mr.commented", // thread.event
	51	"issue.milestoned", "mr.milestoned", // noun+".milestoned"
	52	} {
	53	emitted[k] = true
	54	}
	55	// Prefixes the pattern caught from those concatenations.
	56	for _, partial := range []string{"build.", "issue.", "repo."} {
	57	delete(emitted, partial)
	58	}
	59
	60	for _, k := range EventKinds {
	61	if !emitted[k] {
	62	t.Errorf("EventKinds lists %q but nothing records it", k)
	63	}
	64	}
	65	for k := range emitted {
	66	if !slices.Contains(EventKinds, k) {
	67	t.Errorf("%q is recorded but missing from EventKinds; add it, and to the API wiki page", k)
	68	}
	69	}
	70	}

internal/control/issue.go +11

 	if err := c.Store.UpdateIssueText(issue.ID, title, body, format); err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, "issue.edited", fmt.Sprintf(`{"number":%d}`, issue.Number))
+	if parts, err := c.Store.IssueParticipants(issue.ID); err == nil {
+		notify(c, parts, notice{repo: repo, kind: "issue",
+			subject: issueSubject(repo, issue.Number, issue.Title),
+			action:  fmt.Sprintf("edited #%d", issue.Number),
+			path:    fmt.Sprintf("%s/issues/%d", repo.Path(), issue.Number)})
+	}
 	return c.emit(map[string]any{"number": issue.Number}, func(w io.Writer) {
 		fmt.Fprintf(w, "edited %s#%d\n", repo.Path(), issue.Number)
 	})
 	if err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, "issue.labeled",
+		fmt.Sprintf(`{"number":%d,"labels":%s}`, issue.Number, jsonStrings(updated.Labels)))
 	return c.emit(map[string]any{"number": issue.Number, "labels": updated.Labels}, func(w io.Writer) {
 		fmt.Fprintf(w, "labels on %s#%d: %s\n", repo.Path(), issue.Number, strings.Join(updated.Labels, ", "))
 	})
 	if err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, "issue.assigned",
+		fmt.Sprintf(`{"number":%d,"assignees":%s}`, issue.Number, jsonStrings(updated.Assignees)))
 	return c.emit(map[string]any{"number": issue.Number, "assignees": updated.Assignees}, func(w io.Writer) {
 		fmt.Fprintf(w, "assignees on %s#%d: %s\n", repo.Path(), issue.Number, strings.Join(updated.Assignees, ", "))
 	})

internal/control/milestone.go +10 −3

 	if len(args) != 3 {
 		return c.fail(protocol.ExitUsage, "usage: issue milestone <owner/name> <n> <title|none>")
 	}
-	return setItemMilestone(c, repo, args[2], func(id int64) error {
+	return setItemMilestone(c, repo, "issue", issue.Number, args[2], func(id int64) error {
 		return c.Store.SetIssueMilestone(issue.ID, id)
 	})
 }
 	if len(args) != 3 {
 		return c.fail(protocol.ExitUsage, "usage: mr milestone <owner/name> <n> <title|none>")
 	}
-	return setItemMilestone(c, repo, args[2], func(id int64) error {
+	return setItemMilestone(c, repo, "mr", mr.Number, args[2], func(id int64) error {
 		return c.Store.SetMRMilestone(mr.ID, id)
 	})
 }
-func setItemMilestone(c *Ctx, repo store.Repo, title string, set func(int64) error) int {
+// noun and number name what the milestone was set on, for the event.
+func setItemMilestone(c *Ctx, repo store.Repo, noun string, number int64, title string, set func(int64) error) int {
 	var id int64
 	if title != "none" {
 		m, err := c.Store.MilestoneByTitle(repo.ID, title)
 	if err := set(id); err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	cleared := title
+	if cleared == "none" {
+		cleared = ""
+	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, noun+".milestoned",
+		fmt.Sprintf(`{"number":%d,"milestone":%q}`, number, cleared))
 	if title == "none" {
 		return c.emit(map[string]string{"milestone": ""}, func(w io.Writer) {
 			fmt.Fprintln(w, "milestone cleared")

internal/control/mr.go +12

 	if err := c.Store.UpdateMRText(mr.ID, title, body, format); err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.edited", fmt.Sprintf(`{"number":%d}`, mr.Number))
 	return c.emit(map[string]any{"number": mr.Number}, func(w io.Writer) {
 		fmt.Fprintf(w, "edited %s!%d\n", repo.Path(), mr.Number)
 	})
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
 	c.Store.AddMRSystemComment(mr.ID, c.User.ID, fmt.Sprintf("retargeted from %s to %s", old, target))
+	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.retargeted",
+		fmt.Sprintf(`{"number":%d,"from":%q,"to":%q}`, mr.Number, old, target))
 	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
 		notify(c, parts, notice{repo: repo, kind: "mr",
 			subject: mrSubject(repo, mr.Number, mr.Title),
 	if err := c.Store.AddMRReview(mr.ID, c.User.ID, verdict, mr.HeadSHA); err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.reviewed",
+		fmt.Sprintf(`{"number":%d,"verdict":%q}`, mr.Number, verdict))
 	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
 		notify(c, parts, notice{repo: repo, kind: "mr",
 			subject: mrSubject(repo, mr.Number, mr.Title),
 	if err := c.Store.MarkClosed(mr.ID, c.User.ID, ""); err != nil {
 		return c.fail(protocol.ExitFailure, "%v", err)
 	}
+	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.closed", fmt.Sprintf(`{"number":%d}`, mr.Number))
+	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
+		notify(c, parts, notice{repo: repo, kind: "mr",
+			subject: mrSubject(repo, mr.Number, mr.Title),
+			action:  fmt.Sprintf("closed !%d", mr.Number),
+			path:    fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
+	}
 	return c.emit(map[string]any{"number": mr.Number, "state": "closed"}, func(w io.Writer) {
 		fmt.Fprintf(w, "closed %s!%d\n", repo.Path(), mr.Number)
 	})

internal/control/output.go +16

	@@ -1,5 +1,7 @@
1	package control	1	package control
2		2
		3	import "encoding/json"
		4
3	// Named payloads for the commands other surfaces decode. A command used	5	// Named payloads for the commands other surfaces decode. A command used
4	// to declare its output inline, so the web type-asserted map keys and	6	// to declare its output inline, so the web type-asserted map keys and
5	// nothing tied what a command emits to what a client reads (#126).	7	// nothing tied what a command emits to what a client reads (#126).
	@@ -55,3 +57,17 @@ type CommitOut struct {
55	SHA string `json:"sha"`	57	SHA string `json:"sha"`
56	Subject string `json:"subject"`	58	Subject string `json:"subject"`
57	}	59	}
		60
		61	// jsonStrings renders a string slice as a JSON array, for the event data
		62	// the webhook payload carries. A nil slice is [], never null: a consumer
		63	// iterating the field should not have to special-case one of them.
		64	func jsonStrings(ss []string) string {
		65	if ss == nil {
		66	ss = []string{}
		67	}
		68	raw, err := json.Marshal(ss)
		69	if err != nil {
		70	return "[]"
		71	}
		72	return string(raw)
		73	}

internal/control/release.go +1

	@@ -259,6 +259,7 @@ func runReleaseDelete(c *Ctx, args []string) int {
259	return c.fail(protocol.ExitFailure, "%v", err)	259	return c.fail(protocol.ExitFailure, "%v", err)
260	}	260	}
261	os.RemoveAll(assetDir(c.Cfg.Server.Root, repo, rel.ID))	261	os.RemoveAll(assetDir(c.Cfg.Server.Root, repo, rel.ID))
		262	c.Store.RecordEvent(repo.ID, c.User.ID, "release.deleted", fmt.Sprintf(`{"tag":%q}`, rel.Tag))
262	return c.emit(map[string]string{"deleted": rel.Tag}, func(w io.Writer) {	263	return c.emit(map[string]string{"deleted": rel.Tag}, func(w io.Writer) {
263	fmt.Fprintf(w, "deleted release %s\n", rel.Tag)	264	fmt.Fprintf(w, "deleted release %s\n", rel.Tag)
264	})	265	})

internal/control/repo.go +7

	@@ -430,6 +430,13 @@ func runRepoDelete(c *Ctx, args []string) int {
430		430
431	// deleteRepo removes a repository the caller has already been cleared to	431	// deleteRepo removes a repository the caller has already been cleared to
432	// delete: the database row, then the directory and its wiki companion.	432	// delete: the database row, then the directory and its wiki companion.
		433	//
		434	// There is deliberately no repo.deleted event. events.repo_id and
		435	// webhooks.repo_id both cascade from repos, so recording one would delete
		436	// it, and every webhook that could have subscribed, in the same
		437	// statement. A repository's deletion is not observable through its own
		438	// webhooks; an instance that needs to hear about it wants the audit log
		439	// (#112).
433	func deleteRepo(c *Ctx, repo store.Repo) int {	440	func deleteRepo(c *Ctx, repo store.Repo) int {
434	// Open MRs sourced from this repo keep working (targets own the	441	// Open MRs sourced from this repo keep working (targets own the
435	// objects) but must show that the source is gone.	442	// objects) but must show that the source is gone.

internal/control/webhook.go +6

 	if code >= 0 {
 		return code
 	}
+	// A name that is not an event is a subscription that never fires, and
+	// nothing would ever say so. Checked before the URL, which resolves
+	// DNS: a typo here should not need a reachable host to report.
+	if code := checkEventNames(c, events); code >= 0 {
+		return code
+	}
 	if err := webhook.ValidateURL(url, c.Cfg.Webhooks.AllowLocal); err != nil {
 		return c.failErr(err)
 	}

webhooks: the other half of the mutations record an event !239