fix(agentgit): register the member's SSH key on every BBS login, not just at signup (#131)

provisionGit returned early when the Forgejo account already existed, so
EnsureKey only ever ran during first provisioning. A member who deleted their
key on git.profullstack.com never got it back: every later BBS login hit the
`if !created { return }` and skipped key registration entirely. The comment on
the web verify path ("key is added on next BBS login") was describing behaviour
that could not happen.

Key registration now runs on every provisionGit call that carries a session
key. EnsureKey was already idempotent — it GETs the account's keys and compares
key material ignoring the comment — so re-running it is free when nothing
changed, and it re-adds a removed key, picks up a rotated one, and backfills
members who joined before AgentGit captured keys. The welcome email stays gated
on `created`, since the one-time password is only meaningful for a new account.

Two things that would have made this unreliable in the new every-login path:

- The key title was the constant "agentbbs". Forgejo rejects a duplicate title
  with 422, so a member who rotated their BBS key would have had the new one
  silently dropped. The title now carries a short fingerprint, so distinct keys
  coexist and the same key stays stable across logins.

- EnsureKey mapped *every* 422 to "already exists" and returned nil. That hid
  genuine rejections forever, which matters far more now the call is on the hot
  path. Only "has been used" bodies are swallowed; a rejected key surfaces with
  Forgejo's own reason so it lands in the logs.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

cmd/agentbbs/gitkey_test.go

@@ -0,0 +1,81 @@
+package main
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/profullstack/agentbbs/internal/forgejo"
+	"github.com/profullstack/agentbbs/internal/store"
+)
+
+// Throwaway keys generated for these tests only.
+const testPubKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOeWE4BpdSRsfc8l6w9clDKPTDH9GX/oYSgtxM3ohyhV chovy@bbs"
+
+func TestGitKeyTitleIsPerKey(t *testing.T) {
+	const other = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHHfPGCIu0pk6TrpwdX9VBrGbQXUU44L5ovCRqFFsWSo chovy@bbs"
+
+	a := gitKeyTitle(testPubKey)
+	if !strings.HasPrefix(a, "agentbbs ") {
+		t.Errorf("title = %q, want an \"agentbbs \" prefix", a)
+	}
+	// Two different keys must not collide on title: Forgejo 422s a duplicate
+	// title, which would silently drop the member's rotated key.
+	if b := gitKeyTitle(other); a == b {
+		t.Errorf("distinct keys share the title %q", a)
+	}
+	// The same key is stable across logins, so we don't pile up entries.
+	if a != gitKeyTitle(testPubKey) {
+		t.Error("gitKeyTitle is not stable for the same key")
+	}
+	// Unparseable input still yields a usable title rather than panicking.
+	if got := gitKeyTitle("not-a-key"); got != "agentbbs" {
+		t.Errorf("gitKeyTitle(garbage) = %q, want \"agentbbs\"", got)
+	}
+}
+
+// TestProvisionGitRegistersKeyForExistingAccount is the regression guard for the
+// bug that lost chovy's key: provisionGit used to return early when the Forgejo
+// account already existed, so EnsureKey ran only at first provisioning and a key
+// deleted on AgentGit never came back on any later BBS login.
+func TestProvisionGitRegistersKeyForExistingAccount(t *testing.T) {
+	var posted map[string]any
+	createAttempted := false
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		// Account already exists — this is the case that used to bail out.
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v1/users/chovy":
+			_, _ = w.Write([]byte(`{"id":1,"login":"chovy"}`))
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v1/users/chovy/keys":
+			_, _ = w.Write([]byte(`[]`))
+		case r.Method == http.MethodPost && r.URL.Path == "/api/v1/admin/users/chovy/keys":
+			_ = json.NewDecoder(r.Body).Decode(&posted)
+			w.WriteHeader(http.StatusCreated)
+			_, _ = w.Write([]byte(`{"id":7}`))
+		case r.Method == http.MethodPost && r.URL.Path == "/api/v1/admin/users":
+			createAttempted = true
+			w.WriteHeader(http.StatusUnprocessableEntity)
+		default:
+			t.Errorf("unexpected %s %s", r.Method, r.URL.Path)
+		}
+	}))
+	defer srv.Close()
+
+	a := &app{forgejo: forgejo.Config{BaseURL: srv.URL, Token: "secret"}}
+	u := &store.User{ID: 1, Name: "chovy", Email: "chovy@example.com", EmailVerified: true}
+
+	a.provisionGit(u, testPubKey)
+
+	if createAttempted {
+		t.Error("must not re-create an account that already exists")
+	}
+	if posted == nil {
+		t.Fatal("key was never POSTed for an existing account — the early return is back")
+	}
+	if posted["key"] != testPubKey {
+		t.Errorf("posted key = %v, want %q", posted["key"], testPubKey)
+	}
+}

cmd/agentbbs/main.go

@@ -1236,23 +1236,28 @@ func (a *app) provisionGit(u *store.User, pubKey string) {
 		log.Error("forgejo provision", "user", u.Name, "err", err)
 		return
 	}
-	if !created {
-		return
+	if created {
+		log.Info("provisioned git account", "user", u.Name, "host", a.forgejo.BaseURL)
 	}
-	log.Info("provisioned git account", "user", u.Name, "host", a.forgejo.BaseURL)
 	// Register the BBS SSH key so the member can push with the same key they sign
-	// in with. No-op when called without a session key (e.g. the web verify flow).
+	// in with. This runs on EVERY call, not only when the account was just made:
+	// EnsureKey is idempotent, so re-running it re-adds a key the member removed
+	// on AgentGit, picks up a rotated BBS key, and backfills members who joined
+	// before the key was captured. Gating it on `created` meant a member's key
+	// could only ever be registered once, and never came back once deleted.
+	// No-op when called without a session key (e.g. the web verify flow).
 	if pubKey != "" {
-		if added, err := a.forgejo.EnsureKey(u.Name, "agentbbs", pubKey); err != nil {
+		if added, err := a.forgejo.EnsureKey(u.Name, gitKeyTitle(pubKey), pubKey); err != nil {
 			log.Error("forgejo ssh key", "user", u.Name, "err", err)
 		} else if added {
 			log.Info("registered git ssh key", "user", u.Name)
 		}
 	}
 	// Email the verified address their web sign-in link + one-time password so
-	// they can log in to the Forgejo UI and create repositories. Best-effort:
-	// the account already exists, so a mail failure must not block anything.
-	if a.mail.Configured() {
+	// they can log in to the Forgejo UI and create repositories. First creation
+	// only — password is empty for an account that already existed. Best-effort:
+	// a mail failure must not block anything.
+	if created && a.mail.Configured() {
 		if err := a.mail.Send(u.Email, "Your git.profullstack.com account is ready",
 			gitWelcomeEmailBody(u.Name, password, a.forgejo.LoginURL())); err != nil {
 			log.Error("git welcome email", "user", u.Name, "err", err)
@@ -1276,6 +1281,22 @@ func gitWelcomeEmailBody(name, password, loginURL string) string {
 		"If you didn't request this, you can ignore this email.\n"
 }
 
+// gitKeyTitle labels the key in Forgejo. The fingerprint is baked into the title
+// so a member who rotates their BBS key gets a second entry rather than colliding
+// with the old one — Forgejo rejects a duplicate title with 422, which would
+// otherwise drop the new key on the floor.
+func gitKeyTitle(pubKey string) string {
+	pk, _, _, _, err := gossh.ParseAuthorizedKey([]byte(pubKey))
+	if err != nil {
+		return "agentbbs"
+	}
+	fp := strings.TrimPrefix(gossh.FingerprintSHA256(pk), "SHA256:")
+	if len(fp) > 12 {
+		fp = fp[:12]
+	}
+	return "agentbbs " + fp
+}
+
 // authorizedKey renders the session's public key as a single authorized_keys
 // line, or "" when the session has no key (guests / keyboard-interactive).
 func authorizedKey(s ssh.Session) string {

internal/forgejo/forgejo.go

@@ -207,8 +207,15 @@ func (c Config) EnsureKey(username, title, pubKey string) (added bool, err error
 	if err != nil {
 		return false, err
 	}
+	// Forgejo answers 422 both for "this key/title is already here" (benign, we
+	// raced or the comment differs) and for "this key content is unusable".
+	// Treating every 422 as benign hid real rejections forever, so only swallow
+	// the ones that say the key or title is already taken.
 	if status == http.StatusUnprocessableEntity {
-		return false, nil // key already exists (raced or comment differs)
+		if alreadyUsed(resp) {
+			return false, nil
+		}
+		return false, fmt.Errorf("forgejo rejected key %q: %s", username, truncate(resp, 200))
 	}
 	if status < 200 || status >= 300 {
 		return false, fmt.Errorf("forgejo add key %q: %d: %s", username, status, truncate(resp, 200))
@@ -216,6 +223,14 @@ func (c Config) EnsureKey(username, title, pubKey string) (added bool, err error
 	return true, nil
 }
 
+// alreadyUsed reports whether a 422 body is Forgejo saying the key or its title
+// is already on the account, as opposed to rejecting the key content itself.
+// Forgejo's wording: "Key content has been used as non-deploy key" /
+// "Key title has been used".
+func alreadyUsed(resp string) bool {
+	return strings.Contains(strings.ToLower(resp), "has been used")
+}
+
 // keyMaterial returns the type+base64 of an authorized-key line, dropping the
 // optional comment so the same key compares equal regardless of how it's labeled.
 func keyMaterial(authorizedKey string) string {

internal/forgejo/key422_test.go

@@ -0,0 +1,56 @@
+package forgejo
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// key422Server answers the dedupe GET with an empty list, then returns 422 with
+// the given body for the POST.
+func key422Server(t *testing.T, body string) *httptest.Server {
+	t.Helper()
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodGet {
+			_, _ = w.Write([]byte(`[]`))
+			return
+		}
+		w.WriteHeader(http.StatusUnprocessableEntity)
+		_, _ = w.Write([]byte(body))
+	}))
+}
+
+func TestEnsureKeyTreatsAlreadyUsed422AsBenign(t *testing.T) {
+	srv := key422Server(t, `{"message":"Key content has been used as non-deploy key"}`)
+	defer srv.Close()
+
+	c := Config{BaseURL: srv.URL, Token: "secret"}
+	added, err := c.EnsureKey("alice", "agentbbs", aliceKey)
+	if err != nil {
+		t.Fatalf("a duplicate key must not be an error, got %v", err)
+	}
+	if added {
+		t.Error("expected added=false for a key already on the account")
+	}
+}
+
+// A 422 that is Forgejo rejecting the key content must surface, not be silently
+// swallowed as "already exists" — that is how an unregisterable key stayed
+// invisible in the logs forever.
+func TestEnsureKeySurfacesRejecting422(t *testing.T) {
+	srv := key422Server(t, `{"message":"Key content is not a valid SSH key"}`)
+	defer srv.Close()
+
+	c := Config{BaseURL: srv.URL, Token: "secret"}
+	added, err := c.EnsureKey("alice", "agentbbs", aliceKey)
+	if err == nil {
+		t.Fatal("expected an error when Forgejo rejects the key content")
+	}
+	if added {
+		t.Error("expected added=false on rejection")
+	}
+	if !strings.Contains(err.Error(), "not a valid SSH key") {
+		t.Errorf("error should carry Forgejo's reason, got %v", err)
+	}
+}