fix(appview): include role strongRef when upgrading bootstrap membership (ATB-37) (#68)

* fix(appview): include role strongRef when upgrading bootstrap membership

upgradeBootstrapMembership was writing a PDS record without a role field.
When the firehose re-indexed the event, membershipConfig.toUpdateValues()
set roleUri = record.role?.role.uri ?? null, overwriting the Owner's
roleUri with null and stripping their permissions on first login.

Fix: look up the existing roleUri in the roles table (parsing the AT URI
to extract did/rkey), then include it as a strongRef in the PDS record —
same pattern used by writeMembershipRecord for new members.

Closes ATB-37

* fix(appview): address review feedback on ATB-37 bootstrap upgrade

- Pass ctx.logger to parseAtUri so malformed roleUris emit structured
warnings at parse time
- Add explicit error log when parseAtUri returns null (data integrity
signal — a stored roleUri that can't be parsed is unexpected)
- Upgrade warn → error in DB failure catch: a failed role lookup during
bootstrap upgrade has the same consequence as the role-not-found path
(firehose will null out roleUri), so both warrant error level
- Add DB failure test for the role lookup catch block
- Add malformed roleUri test (parseAtUri returns null path)
- Add result.created assertions to tests 1 and 2
- Add DB state assertions to all three new bootstrap upgrade tests

apps/appview/src/lib/__tests__/membership.test.ts

···
     expect(updated.roleUri).toBe(ownerRoleUri);
     expect(updated.role).toBe("Owner");
   });
+
+  it("includes role strongRef in PDS record when upgrading bootstrap membership with a known role", async () => {
+    // This is the ATB-37 regression test. When upgradeBootstrapMembership writes the
+    // PDS record without a role field, the firehose re-indexes the event and sets
+    // roleUri = null (record.role?.role.uri ?? null), stripping the Owner's role.
+    const testDid = `did:plc:test-bootstrap-roleref-${Date.now()}`;
+    const forumUri = `at://${ctx.config.forumDid}/space.atbb.forum.forum/self`;
+    const ownerRoleRkey = "ownerrole789";
+    const ownerRoleCid = "bafyowner789";
+
+    // Insert the Owner role so upgradeBootstrapMembership can look it up
+    await ctx.db.insert(roles).values({
+      did: ctx.config.forumDid,
+      rkey: ownerRoleRkey,
+      cid: ownerRoleCid,
+      name: "Owner",
+      description: "Forum owner",
+      priority: 10,
+      createdAt: new Date(),
+      indexedAt: new Date(),
+    });
+
+    const ownerRoleUri = `at://${ctx.config.forumDid}/space.atbb.forum.role/${ownerRoleRkey}`;
+
+    await ctx.db.insert(users).values({
+      did: testDid,
+      handle: "bootstrap.roleref",
+      indexedAt: new Date(),
+    });
+
+    await ctx.db.insert(memberships).values({
+      did: testDid,
+      rkey: "bootstrap",
+      cid: "bootstrap",
+      forumUri,
+      roleUri: ownerRoleUri,
+      role: "Owner",
+      createdAt: new Date(),
+      indexedAt: new Date(),
+    });
+
+    const mockAgent = {
+      com: {
+        atproto: {
+          repo: {
+            putRecord: vi.fn().mockResolvedValue({
+              data: {
+                uri: `at://${testDid}/space.atbb.membership/tidabc`,
+                cid: "bafyupgradedabc",
+              },
+            }),
+          },
+        },
+      },
+    } as any;
+
+    const result = await createMembershipForUser(ctx, mockAgent, testDid);
+
+    expect(result.created).toBe(true);
+
+    // The PDS record must include the role strongRef so the firehose
+    // preserves the roleUri when it re-indexes the upgrade event.
+    expect(mockAgent.com.atproto.repo.putRecord).toHaveBeenCalledWith(
+      expect.objectContaining({
+        record: expect.objectContaining({
+          role: {
+            role: {
+              uri: ownerRoleUri,
+              cid: ownerRoleCid,
+            },
+          },
+        }),
+      })
+    );
+
+    // DB row must reflect the upgrade: real rkey/cid, roleUri preserved
+    const [updated] = await ctx.db
+      .select()
+      .from(memberships)
+      .where(and(eq(memberships.did, testDid), eq(memberships.forumUri, forumUri)))
+      .limit(1);
+    expect(updated.cid).toBe("bafyupgradedabc");
+    expect(updated.rkey).not.toBe("bootstrap");
+    expect(updated.roleUri).toBe(ownerRoleUri);
+  });
+
+  it("omits role from PDS record when upgrading bootstrap membership without a roleUri", async () => {
+    const testDid = `did:plc:test-bootstrap-norole-${Date.now()}`;
+    const forumUri = `at://${ctx.config.forumDid}/space.atbb.forum.forum/self`;
+
+    await ctx.db.insert(users).values({
+      did: testDid,
+      handle: "bootstrap.norole",
+      indexedAt: new Date(),
+    });
+
+    // Bootstrap membership with no roleUri
+    await ctx.db.insert(memberships).values({
+      did: testDid,
+      rkey: "bootstrap",
+      cid: "bootstrap",
+      forumUri,
+      createdAt: new Date(),
+      indexedAt: new Date(),
+    });
+
+    const mockAgent = {
+      com: {
+        atproto: {
+          repo: {
+            putRecord: vi.fn().mockResolvedValue({
+              data: {
+                uri: `at://${testDid}/space.atbb.membership/tiddef`,
+                cid: "bafynoroledef",
+              },
+            }),
+          },
+        },
+      },
+    } as any;
+
+    const result = await createMembershipForUser(ctx, mockAgent, testDid);
+
+    expect(result.created).toBe(true);
+    const callArg = mockAgent.com.atproto.repo.putRecord.mock.calls[0][0];
+    expect(callArg.record.role).toBeUndefined();
+
+    // DB row must reflect the upgrade: real rkey/cid, roleUri stays null
+    const [updated] = await ctx.db
+      .select()
+      .from(memberships)
+      .where(and(eq(memberships.did, testDid), eq(memberships.forumUri, forumUri)))
+      .limit(1);
+    expect(updated.cid).toBe("bafynoroledef");
+    expect(updated.rkey).not.toBe("bootstrap");
+    expect(updated.roleUri).toBeNull();
+  });
+
+  it("upgrades bootstrap membership without role when roleUri references a role not in DB", async () => {
+    const testDid = `did:plc:test-bootstrap-missingrole-${Date.now()}`;
+    const forumUri = `at://${ctx.config.forumDid}/space.atbb.forum.forum/self`;
+    // A roleUri that has no matching row in the roles table
+    const danglingRoleUri = `at://${ctx.config.forumDid}/space.atbb.forum.role/nonexistent`;
+
+    await ctx.db.insert(users).values({
+      did: testDid,
+      handle: "bootstrap.missingrole",
+      indexedAt: new Date(),
+    });
+
+    await ctx.db.insert(memberships).values({
+      did: testDid,
+      rkey: "bootstrap",
+      cid: "bootstrap",
+      forumUri,
+      roleUri: danglingRoleUri,
+      createdAt: new Date(),
+      indexedAt: new Date(),
+    });
+
+    const mockAgent = {
+      com: {
+        atproto: {
+          repo: {
+            putRecord: vi.fn().mockResolvedValue({
+              data: {
+                uri: `at://${testDid}/space.atbb.membership/tidghi`,
+                cid: "bafymissingghi",
+              },
+            }),
+          },
+        },
+      },
+    } as any;
+
+    // Upgrade should still succeed even if role lookup finds nothing
+    const result = await createMembershipForUser(ctx, mockAgent, testDid);
+    expect(result.created).toBe(true);
+
+    const callArg = mockAgent.com.atproto.repo.putRecord.mock.calls[0][0];
+    expect(callArg.record.role).toBeUndefined();
+
+    // DB row must reflect the upgrade: real rkey/cid, dangling roleUri preserved
+    const [updated] = await ctx.db
+      .select()
+      .from(memberships)
+      .where(and(eq(memberships.did, testDid), eq(memberships.forumUri, forumUri)))
+      .limit(1);
+    expect(updated.cid).toBe("bafymissingghi");
+    expect(updated.rkey).not.toBe("bootstrap");
+    expect(updated.roleUri).toBe(danglingRoleUri);
+  });
+
+  it("logs error and continues upgrade when role DB lookup fails during bootstrap upgrade", async () => {
+    const testDid = `did:plc:test-bootstrap-dberr-${Date.now()}`;
+    const forumUri = `at://${ctx.config.forumDid}/space.atbb.forum.forum/self`;
+    const ownerRoleUri = `at://${ctx.config.forumDid}/space.atbb.forum.role/ownerrole`;
+
+    await ctx.db.insert(users).values({
+      did: testDid,
+      handle: "bootstrap.dberr",
+      indexedAt: new Date(),
+    });
+
+    await ctx.db.insert(memberships).values({
+      did: testDid,
+      rkey: "bootstrap",
+      cid: "bootstrap",
+      forumUri,
+      roleUri: ownerRoleUri,
+      role: "Owner",
+      createdAt: new Date(),
+      indexedAt: new Date(),
+    });
+
+    const origSelect = ctx.db.select.bind(ctx.db);
+    vi.spyOn(ctx.db, "select")
+      .mockImplementationOnce(() => origSelect() as any)  // forums lookup
+      .mockImplementationOnce(() => origSelect() as any)  // memberships check (bootstrap found)
+      .mockReturnValueOnce({                              // roles query in upgradeBootstrapMembership
+        from: vi.fn().mockReturnValue({
+          where: vi.fn().mockReturnValue({
+            limit: vi.fn().mockRejectedValue(new Error("DB connection lost")),
+          }),
+        }),
+      } as any);
+
+    const errorSpy = vi.spyOn(ctx.logger, "error");
+
+    const mockAgent = {
+      com: {
+        atproto: {
+          repo: {
+            putRecord: vi.fn().mockResolvedValue({
+              data: {
+                uri: `at://${testDid}/space.atbb.membership/tidjkl`,
+                cid: "bafydberrjkl",
+              },
+            }),
+          },
+        },
+      },
+    } as any;
+
+    const result = await createMembershipForUser(ctx, mockAgent, testDid);
+
+    expect(result.created).toBe(true);
+    const callArg = mockAgent.com.atproto.repo.putRecord.mock.calls[0][0];
+    expect(callArg.record.role).toBeUndefined();
+    expect(errorSpy).toHaveBeenCalledWith(
+      expect.stringContaining("Role lookup failed during bootstrap upgrade"),
+      expect.objectContaining({ operation: "upgradeBootstrapMembership" })
+    );
+
+    vi.restoreAllMocks();
+  });
+
+  it("logs error and omits role when bootstrap membership has a malformed roleUri", async () => {
+    const testDid = `did:plc:test-bootstrap-malformed-${Date.now()}`;
+    const forumUri = `at://${ctx.config.forumDid}/space.atbb.forum.forum/self`;
+    // Syntactically invalid AT URI — parseAtUri will return null
+    const malformedRoleUri = "not-a-valid-at-uri";
+
+    await ctx.db.insert(users).values({
+      did: testDid,
+      handle: "bootstrap.malformed",
+      indexedAt: new Date(),
+    });
+
+    await ctx.db.insert(memberships).values({
+      did: testDid,
+      rkey: "bootstrap",
+      cid: "bootstrap",
+      forumUri,
+      roleUri: malformedRoleUri,
+      createdAt: new Date(),
+      indexedAt: new Date(),
+    });
+
+    const errorSpy = vi.spyOn(ctx.logger, "error");
+
+    const mockAgent = {
+      com: {
+        atproto: {
+          repo: {
+            putRecord: vi.fn().mockResolvedValue({
+              data: {
+                uri: `at://${testDid}/space.atbb.membership/tidmno`,
+                cid: "bafymalformedmno",
+              },
+            }),
+          },
+        },
+      },
+    } as any;
+
+    const result = await createMembershipForUser(ctx, mockAgent, testDid);
+
+    expect(result.created).toBe(true);
+    const callArg = mockAgent.com.atproto.repo.putRecord.mock.calls[0][0];
+    expect(callArg.record.role).toBeUndefined();
+    expect(errorSpy).toHaveBeenCalledWith(
+      expect.stringContaining("roleUri failed to parse"),
+      expect.objectContaining({ operation: "upgradeBootstrapMembership", roleUri: malformedRoleUri })
+    );
+  });
 });

apps/appview/src/lib/membership.ts

···
 import { memberships, forums, roles } from "@atbb/db";
 import { eq, and, asc } from "drizzle-orm";
 import { TID } from "@atproto/common-web";
+import { parseAtUri } from "./at-uri.js";
 
 export async function createMembershipForUser(
   ctx: AppContext,
···
     // record. Upgrade them by writing a real record to the user's PDS and
     // updating the DB row with the actual rkey/cid.
     if (membership.cid === "bootstrap") {
-      return upgradeBootstrapMembership(ctx, agent, did, forumUri, forum.cid, membership.id);
+      return upgradeBootstrapMembership(ctx, agent, did, forumUri, forum.cid, membership.id, membership.roleUri);
     }
 
     return { created: false };
···
   did: string,
   forumUri: string,
   forumCid: string,
-  membershipId: bigint
+  membershipId: bigint,
+  roleUri: string | null
 ): Promise<{ created: boolean; uri?: string; cid?: string }> {
   const rkey = TID.nextStr();
   const now = new Date().toISOString();
 
+  // Look up the role so we can include it as a strongRef in the PDS record.
+  // Without this, the firehose will re-index the event and set roleUri = null
+  // (record.role?.role.uri ?? null), stripping the member's role.
+  let roleRef: { uri: string; cid: string } | null = null;
+  if (roleUri) {
+    const parsed = parseAtUri(roleUri, ctx.logger);
+    if (!parsed) {
+      ctx.logger.error("roleUri failed to parse — role omitted from PDS record", {
+        operation: "upgradeBootstrapMembership",
+        did,
+        roleUri,
+      });
+    } else {
+      try {
+        const [role] = await ctx.db
+          .select({ cid: roles.cid })
+          .from(roles)
+          .where(and(eq(roles.did, parsed.did), eq(roles.rkey, parsed.rkey)))
+          .limit(1);
+        if (role) {
+          roleRef = { uri: roleUri, cid: role.cid };
+        }
+      } catch (error) {
+        if (error instanceof TypeError || error instanceof ReferenceError || error instanceof SyntaxError) {
+          throw error;
+        }
+        ctx.logger.error("Role lookup failed during bootstrap upgrade — omitting role from PDS record", {
+          operation: "upgradeBootstrapMembership",
+          did,
+          roleUri,
+          error: error instanceof Error ? error.message : String(error),
+        });
+      }
+    }
+  }
+
+  const record: Record<string, unknown> = {
+    $type: "space.atbb.membership",
+    forum: {
+      forum: { uri: forumUri, cid: forumCid },
+    },
+    createdAt: now,
+    joinedAt: now,
+  };
+
+  if (roleRef) {
+    record.role = { role: { uri: roleRef.uri, cid: roleRef.cid } };
+  }
+
   const result = await agent.com.atproto.repo.putRecord({
     repo: did,
     collection: "space.atbb.membership",
     rkey,
-    record: {
-      $type: "space.atbb.membership",
-      forum: {
-        forum: { uri: forumUri, cid: forumCid },
-      },
-      createdAt: now,
-      joinedAt: now,
-    },
+    record,
   });
 
   // Update the bootstrap row with PDS-backed values, preserving roleUri