fix(server): make GitHub webhook registration idempotent (#68)

doc/WEBHOOKS.md

@@ -82,10 +82,17 @@ When `webhook_mode=auto` and the PAT scope check passes:
 
 1. Operator submits the add-repo form. The server clones the repo
    (`clone_repo` job) and starts indexing.
-2. In parallel, the server calls `POST /repos/{owner}/{repo}/hooks`
-   on GitHub via `server/internal/githubapi/`. The hook payload sets
-   `events: ["push"]`, `content_type: json`, and embeds the
-   server-generated `webhook_secret`.
+2. In parallel, the server registers the hook **idempotently** via
+   `server/internal/githubapi/` (`EnsureWebhook`): it first
+   `GET /repos/{owner}/{repo}/hooks` and looks for a hook whose
+   `config.url` already equals this server's delivery URL. If one
+   exists it is reused (PATCHed to refresh the secret/events) and any
+   extra duplicates pointing at the same URL are deleted; only when
+   none match does it `POST /repos/{owner}/{repo}/hooks`. The hook
+   payload sets `events: ["push"]`, `content_type: json`, and embeds
+   the server-generated `webhook_secret`. This is what prevents
+   duplicate hooks accumulating across re-adds, reindexes, and server
+   restarts (issue #68).
 3. GitHub responds with the hook id. The id is stored on the
    `git_repos` row so a later DELETE can call
    `DELETE /repos/{owner}/{repo}/hooks/{id}` cleanly.
@@ -154,7 +161,17 @@ On boot the server runs a one-shot audit
 
 This is also why rotating `CIX_PUBLIC_URL` should be paired with a
 "reregister all" sweep in the dashboard — there's no automatic
-follow-up.
+follow-up. The reconcile/reregister path is idempotent: a repo whose
+hook id is still known is PATCHed in place; a repo whose stored id was
+lost is matched by `config.url` and reused rather than re-created, so a
+sweep never leaves a repo with old **and** new hooks side by side.
+
+One caveat for repos that *already* accumulated same-URL duplicates
+before this fix: when the stored hook id is still valid, reconcile
+PATCHes that one hook and returns — it does **not** list and prune the
+sibling duplicates. Re-adding the repo (which routes through the
+`EnsureWebhook` list→match→prune path) collapses them back to a single
+hook; a plain reconcile sweep does not.
 
 ## 7. What gets re-indexed on a push
 

server/internal/githubapi/githubapi.go

@@ -73,10 +73,10 @@ func New() *Client {
 // repo-picker UI actually renders — so we don't bloat the JSON payload
 // (a single user can have several hundred repos visible via a PAT).
 type Repo struct {
-	FullName      string    `json:"full_name"`       // "owner/name"
-	DefaultBranch string    `json:"default_branch"`  // used to auto-fill the branch input
-	Private       bool      `json:"private"`         // shown as a lock icon in the dropdown
-	HTMLURL       string    `json:"html_url"`        // canonical https://github.com/... form
+	FullName      string    `json:"full_name"`      // "owner/name"
+	DefaultBranch string    `json:"default_branch"` // used to auto-fill the branch input
+	Private       bool      `json:"private"`        // shown as a lock icon in the dropdown
+	HTMLURL       string    `json:"html_url"`       // canonical https://github.com/... form
 	Description   string    `json:"description,omitempty"`
 	Owner         RepoOwner `json:"owner"`
 }
@@ -359,11 +359,22 @@ type CreateWebhookOptions struct {
 	Insecure bool // mostly for tests against http:// origins
 }
 
-// HookResponse is the slice of the GitHub response we care about.
+// HookConfig mirrors the `config` slice of a GitHub hook. We only read
+// `url` — the delivery target — which is what makes registration
+// idempotent: a hook whose config.url already equals our delivery URL is
+// the cix hook, so we reuse it instead of POSTing a duplicate.
+type HookConfig struct {
+	URL string `json:"url"`
+}
+
+// HookResponse is the slice of the GitHub response we care about. The
+// top-level URL is the hook's own API URL (…/hooks/{id}); Config.URL is the
+// delivery target the hook POSTs to.
 type HookResponse struct {
-	ID     int64  `json:"id"`
-	URL    string `json:"url"`
-	Active bool   `json:"active"`
+	ID     int64      `json:"id"`
+	URL    string     `json:"url"`
+	Active bool       `json:"active"`
+	Config HookConfig `json:"config"`
 }
 
 // CreateWebhook calls POST /repos/{owner}/{repo}/hooks. Returns the
@@ -422,6 +433,130 @@ func (c *Client) CreateWebhook(ctx context.Context, opts CreateWebhookOptions) (
 	}
 }
 
+// ListWebhooks calls GET /repos/{owner}/{repo}/hooks and returns the repo's
+// configured hooks (with their delivery config.url). Used to make
+// registration idempotent — callers match on config.url to find an existing
+// cix hook before creating a new one. Walks Link rel=next up to maxPages
+// (0 = no cap); a repo with >100 hooks is pathological, so the default
+// caller passes a small ceiling.
+func (c *Client) ListWebhooks(ctx context.Context, owner, repo, pat string, maxPages int) ([]HookResponse, error) {
+	if owner == "" || repo == "" {
+		return nil, fmt.Errorf("owner/repo required")
+	}
+	if pat == "" {
+		return nil, fmt.Errorf("PAT required")
+	}
+	out := []HookResponse{}
+	page := 0
+	pageURL := c.BaseURL + "/repos/" + url.PathEscape(owner) + "/" + url.PathEscape(repo) + "/hooks?per_page=100"
+	for pageURL != "" {
+		page++
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, pageURL, nil)
+		if err != nil {
+			return nil, err
+		}
+		c.signRequest(req, pat)
+		resp, err := c.HTTPClient.Do(req)
+		if err != nil {
+			return nil, fmt.Errorf("github API: %w", err)
+		}
+		body, _ := io.ReadAll(resp.Body)
+		resp.Body.Close()
+		switch resp.StatusCode {
+		case http.StatusOK:
+			var batch []HookResponse
+			if err := json.Unmarshal(body, &batch); err != nil {
+				return nil, fmt.Errorf("parse hooks page: %w", err)
+			}
+			out = append(out, batch...)
+		case http.StatusUnauthorized, http.StatusForbidden:
+			return nil, fmt.Errorf("%w: %s", ErrUnauthorized, githubMessage(body))
+		case http.StatusNotFound:
+			return nil, fmt.Errorf("%w: %s", ErrNotFound, githubMessage(body))
+		default:
+			return nil, fmt.Errorf("github API %d: %s", resp.StatusCode, githubMessage(body))
+		}
+		if maxPages > 0 && page >= maxPages {
+			break
+		}
+		pageURL = parseNextLink(resp.Header.Get("Link"))
+	}
+	return out, nil
+}
+
+// EnsureWebhook is the idempotent registration entry point. It lists the
+// repo's existing hooks and, if one already targets opts.URL, PATCHes it in
+// place (refreshing the secret/events/active flag) and returns its id;
+// otherwise it POSTs a fresh hook. Any *additional* hooks pointing at the
+// same delivery URL are deleted, so a repo that already accumulated
+// duplicates (issue #68) converges to exactly one cix hook.
+//
+// The returned bool reports whether a new hook was created (false = an
+// existing one was reused). Callers persist the returned id either way.
+func (c *Client) EnsureWebhook(ctx context.Context, opts CreateWebhookOptions) (HookResponse, bool, error) {
+	hooks, err := c.ListWebhooks(ctx, opts.Owner, opts.Repo, opts.PAT, 10)
+	if err != nil {
+		return HookResponse{}, false, err
+	}
+	var matches []HookResponse
+	for _, h := range hooks {
+		if sameDeliveryURL(h.Config.URL, opts.URL) {
+			matches = append(matches, h)
+		}
+	}
+	if len(matches) == 0 {
+		hr, cerr := c.CreateWebhook(ctx, opts)
+		if cerr != nil {
+			return HookResponse{}, false, cerr
+		}
+		return hr, true, nil
+	}
+
+	// Reuse the first match; PATCH to refresh delivery config. If GitHub
+	// reports it gone (raced delete between list and patch), create instead.
+	keep := matches[0]
+	hr, uerr := c.UpdateWebhook(ctx, UpdateWebhookOptions{
+		Owner:    opts.Owner,
+		Repo:     opts.Repo,
+		PAT:      opts.PAT,
+		HookID:   keep.ID,
+		URL:      opts.URL,
+		Secret:   opts.Secret,
+		Events:   opts.Events,
+		Insecure: opts.Insecure,
+	})
+	if uerr != nil {
+		if errors.Is(uerr, ErrNotFound) {
+			// matches[0] was deleted out from under us between list and
+			// patch. Create a replacement, but still prune any other
+			// same-URL duplicates we listed so we don't leave them behind.
+			hr2, cerr := c.CreateWebhook(ctx, opts)
+			if cerr != nil {
+				return HookResponse{}, false, cerr
+			}
+			for _, dup := range matches[1:] {
+				_ = c.DeleteWebhook(ctx, opts.Owner, opts.Repo, opts.PAT, dup.ID)
+			}
+			return hr2, true, nil
+		}
+		return HookResponse{}, false, uerr
+	}
+
+	// Prune any leftover duplicates so the repo ends up with one cix hook.
+	for _, dup := range matches[1:] {
+		_ = c.DeleteWebhook(ctx, opts.Owner, opts.Repo, opts.PAT, dup.ID)
+	}
+	return hr, false, nil
+}
+
+// sameDeliveryURL compares two hook delivery URLs for idempotency. Exact
+// match after trimming a trailing slash — cix builds delivery URLs
+// deterministically, so anything subtler would risk treating a genuinely
+// different endpoint as the same hook.
+func sameDeliveryURL(a, b string) bool {
+	return strings.TrimRight(a, "/") == strings.TrimRight(b, "/") && a != ""
+}
+
 // UpdateWebhookOptions parameterises a hook update. HookID identifies the
 // existing hook; URL/Secret are the new delivery config. Events defaults
 // to ["push"] when nil.