Commit `f150dbe6d8`

f150dbe6d87e2cd72e724d229eb642d681a23c32

parent: 7cc95d1fc1

Verified · cmc ci/build: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 16:36 UTC

webhooks: the other half of the mutations record an event

Recorded: push, issue create/comment/close/open, mr create/comment/merge,
release.created, status, build.*, repo.imported. Everything else changed
the repository silently, so a webhook subscribed to it could never fire.

Added: mr.closed, mr.reviewed, mr.edited, mr.retargeted, issue.edited,
issue.labeled, issue.assigned, issue.milestoned, mr.milestoned,
release.deleted. mr close and issue edit also notify participants, which
they did not.

EventKinds is the published list. TestEventKindsAreRecorded reads the
RecordEvent calls out of the source and fails if either side has
something the other does not — a name in only one place is a subscription
that silently never fires, which is the failure this issue is about. That
guard is why `webhook add --events` now refuses a name this forge never
emits, before validating the URL: a typo should not need a resolvable
host to report.

No repo.deleted. events.repo_id and webhooks.repo_id both cascade from
repos, so recording one would delete it, and every webhook that could
have received it, in the same statement. A repository's deletion is not
observable through its own webhooks; the audit log is where it lives.

Closes #112

Layout: unified · split

e2e/webhook_test.go +53

		@@ -254,3 +254,56 @@ func TestWebhooks(t *testing.T) {
254	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

		@@ -344,6 +344,13 @@ func runIssueEdit(c *Ctx, args []string) int {
344	344	if err := c.Store.UpdateIssueText(issue.ID, title, body, format); err != nil {
345	345	return c.fail(protocol.ExitFailure, "%v", err)
346	346	}
	347	c.Store.RecordEvent(repo.ID, c.User.ID, "issue.edited", fmt.Sprintf(`{"number":%d}`, issue.Number))
	348	if parts, err := c.Store.IssueParticipants(issue.ID); err == nil {
	349	notify(c, parts, notice{repo: repo, kind: "issue",
	350	subject: issueSubject(repo, issue.Number, issue.Title),
	351	action: fmt.Sprintf("edited #%d", issue.Number),
	352	path: fmt.Sprintf("%s/issues/%d", repo.Path(), issue.Number)})
	353	}
347	354	return c.emit(map[string]any{"number": issue.Number}, func(w io.Writer) {
348	355	fmt.Fprintf(w, "edited %s#%d\n", repo.Path(), issue.Number)
349	356	})
		@@ -394,6 +401,8 @@ func runIssueLabel(c *Ctx, args []string) int {
394	401	if err != nil {
395	402	return c.fail(protocol.ExitFailure, "%v", err)
396	403	}
	404	c.Store.RecordEvent(repo.ID, c.User.ID, "issue.labeled",
	405	fmt.Sprintf(`{"number":%d,"labels":%s}`, issue.Number, jsonStrings(updated.Labels)))
397	406	return c.emit(map[string]any{"number": issue.Number, "labels": updated.Labels}, func(w io.Writer) {
398	407	fmt.Fprintf(w, "labels on %s#%d: %s\n", repo.Path(), issue.Number, strings.Join(updated.Labels, ", "))
399	408	})
		@@ -449,6 +458,8 @@ func runIssueAssign(c *Ctx, args []string) int {
449	458	if err != nil {
450	459	return c.fail(protocol.ExitFailure, "%v", err)
451	460	}
	461	c.Store.RecordEvent(repo.ID, c.User.ID, "issue.assigned",
	462	fmt.Sprintf(`{"number":%d,"assignees":%s}`, issue.Number, jsonStrings(updated.Assignees)))
452	463	return c.emit(map[string]any{"number": issue.Number, "assignees": updated.Assignees}, func(w io.Writer) {
453	464	fmt.Fprintf(w, "assignees on %s#%d: %s\n", repo.Path(), issue.Number, strings.Join(updated.Assignees, ", "))
454	465	})

internal/control/milestone.go +10 −3

		@@ -159,7 +159,7 @@ func runIssueMilestone(c *Ctx, args []string) int {
159	159	if len(args) != 3 {
160	160	return c.fail(protocol.ExitUsage, "usage: issue milestone <owner/name> <n> <title\|none>")
161	161	}
162		return setItemMilestone(c, repo, args[2], func(id int64) error {
	162	return setItemMilestone(c, repo, "issue", issue.Number, args[2], func(id int64) error {
163	163	return c.Store.SetIssueMilestone(issue.ID, id)
164	164	})
165	165	}
		@@ -175,12 +175,13 @@ func runMRMilestone(c *Ctx, args []string) int {
175	175	if len(args) != 3 {
176	176	return c.fail(protocol.ExitUsage, "usage: mr milestone <owner/name> <n> <title\|none>")
177	177	}
178		return setItemMilestone(c, repo, args[2], func(id int64) error {
	178	return setItemMilestone(c, repo, "mr", mr.Number, args[2], func(id int64) error {
179	179	return c.Store.SetMRMilestone(mr.ID, id)
180	180	})
181	181	}
182	182
183		func setItemMilestone(c *Ctx, repo store.Repo, title string, set func(int64) error) int {
	183	// noun and number name what the milestone was set on, for the event.
	184	func setItemMilestone(c *Ctx, repo store.Repo, noun string, number int64, title string, set func(int64) error) int {
184	185	var id int64
185	186	if title != "none" {
186	187	m, err := c.Store.MilestoneByTitle(repo.ID, title)
		@@ -192,6 +193,12 @@ func setItemMilestone(c *Ctx, repo store.Repo, title string, set func(int64) err
192	193	if err := set(id); err != nil {
193	194	return c.fail(protocol.ExitFailure, "%v", err)
194	195	}
	196	cleared := title
	197	if cleared == "none" {
	198	cleared = ""
	199	}
	200	c.Store.RecordEvent(repo.ID, c.User.ID, noun+".milestoned",
	201	fmt.Sprintf(`{"number":%d,"milestone":%q}`, number, cleared))
195	202	if title == "none" {
196	203	return c.emit(map[string]string{"milestone": ""}, func(w io.Writer) {
197	204	fmt.Fprintln(w, "milestone cleared")

internal/control/mr.go +12

		@@ -601,6 +601,7 @@ func runMREdit(c *Ctx, args []string) int {
601	601	if err := c.Store.UpdateMRText(mr.ID, title, body, format); err != nil {
602	602	return c.fail(protocol.ExitFailure, "%v", err)
603	603	}
	604	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.edited", fmt.Sprintf(`{"number":%d}`, mr.Number))
604	605	return c.emit(map[string]any{"number": mr.Number}, func(w io.Writer) {
605	606	fmt.Fprintf(w, "edited %s!%d\n", repo.Path(), mr.Number)
606	607	})
		@@ -649,6 +650,8 @@ func runMRRetarget(c *Ctx, args []string) int {
649	650	return c.fail(protocol.ExitFailure, "%v", err)
650	651	}
651	652	c.Store.AddMRSystemComment(mr.ID, c.User.ID, fmt.Sprintf("retargeted from %s to %s", old, target))
	653	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.retargeted",
	654	fmt.Sprintf(`{"number":%d,"from":%q,"to":%q}`, mr.Number, old, target))
652	655	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
653	656	notify(c, parts, notice{repo: repo, kind: "mr",
654	657	subject: mrSubject(repo, mr.Number, mr.Title),
		@@ -700,6 +703,8 @@ func runMRReview(c *Ctx, args []string) int {
700	703	if err := c.Store.AddMRReview(mr.ID, c.User.ID, verdict, mr.HeadSHA); err != nil {
701	704	return c.fail(protocol.ExitFailure, "%v", err)
702	705	}
	706	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.reviewed",
	707	fmt.Sprintf(`{"number":%d,"verdict":%q}`, mr.Number, verdict))
703	708	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
704	709	notify(c, parts, notice{repo: repo, kind: "mr",
705	710	subject: mrSubject(repo, mr.Number, mr.Title),
		@@ -1171,6 +1176,13 @@ func runMRClose(c *Ctx, args []string) int {
1171	1176	if err := c.Store.MarkClosed(mr.ID, c.User.ID, ""); err != nil {
1172	1177	return c.fail(protocol.ExitFailure, "%v", err)
1173	1178	}
	1179	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.closed", fmt.Sprintf(`{"number":%d}`, mr.Number))
	1180	if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
	1181	notify(c, parts, notice{repo: repo, kind: "mr",
	1182	subject: mrSubject(repo, mr.Number, mr.Title),
	1183	action: fmt.Sprintf("closed !%d", mr.Number),
	1184	path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
	1185	}
1174	1186	return c.emit(map[string]any{"number": mr.Number, "state": "closed"}, func(w io.Writer) {
1175	1187	fmt.Fprintf(w, "closed %s!%d\n", repo.Path(), mr.Number)
1176	1188	})

internal/control/output.go +16

		@@ -1,5 +1,7 @@
1	1	package control
2	2
	3	import "encoding/json"
	4
3	5	// Named payloads for the commands other surfaces decode. A command used
4	6	// to declare its output inline, so the web type-asserted map keys and
5	7	// nothing tied what a command emits to what a client reads (#126).
		@@ -55,3 +57,17 @@ type CommitOut struct {
55	57	SHA string `json:"sha"`
56	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	259	return c.fail(protocol.ExitFailure, "%v", err)
260	260	}
261	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	263	return c.emit(map[string]string{"deleted": rel.Tag}, func(w io.Writer) {
263	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	431	// deleteRepo removes a repository the caller has already been cleared to
432	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	440	func deleteRepo(c *Ctx, repo store.Repo) int {
434	441	// Open MRs sourced from this repo keep working (targets own the
435	442	// objects) but must show that the source is gone.

internal/control/webhook.go +6

		@@ -46,6 +46,12 @@ func runWebhookAdd(c *Ctx, args []string) int {
46	46	if code >= 0 {
47	47	return code
48	48	}
	49	// A name that is not an event is a subscription that never fires, and
	50	// nothing would ever say so. Checked before the URL, which resolves
	51	// DNS: a typo here should not need a reachable host to report.
	52	if code := checkEventNames(c, events); code >= 0 {
	53	return code
	54	}
49	55	if err := webhook.ValidateURL(url, c.Cfg.Webhooks.AllowLocal); err != nil {
50	56	return c.failErr(err)
51	57	}

Commit f150dbe6d8

Commit `f150dbe6d8`