role CRUD, with a guard the spec did not ask for

Phase 2 part 3 of docs/roles-and-teams.md. Create, rename and delete, each
returning a discriminated result rather than throwing, so a router can say which
rule refused and why instead of parsing an error string. Written by a Sonnet
subagent against the plan; reviewed, exercised against the live database, and
amended here.

The amendment is the reason a review was worth doing. renameRole changes
roles.name, and there is no foreign key yet — part 2 ships ON UPDATE CASCADE as
its own release — so a rename would leave every holder pointing at a name that no
longer exists. It is unreachable today only by accident: manage-users.ts validates
against the USER_ROLES union, so a runtime-created role cannot be assigned and a
non-system role has no holders. That evaporates the moment part 4 makes UserRole a
plain string, and parts 2 and 4 could land in either order.

So rename now refuses while anybody holds it, sharing one holder-count definition
with delete so the two cannot disagree about what "in use" means. It comes out when
the cascade lands, and then the cascade does the job properly by moving holders
rather than refusing.

Adding that reason made the router's switch non-exhaustive, which the typechecker
caught — the discriminated-result shape earning its keep on its first change.

System roles refuse both rename and delete: the platform's own code names them by
string, so renaming one is a code change that has not happened. Delete never
reassigns holders to a fallback, because that is a permission change nobody asked
for.

Exercised end to end: duplicate, empty name, system rename, system delete, a real
rename, a rename onto an existing name, delete, and delete of something absent.
The four seeded roles survive with the owner counted against Super Admin.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Officer-Session: 656f5c85-e49a-4f1f-b9f3-042a04b4d043

src/databases/officer_db/src/roles/index.ts

@@ -1,2 +1,13 @@
 export { roles } from './schema';
-export { listRoles, getRole, seedRole, type RoleRow } from './queries';
+export {
+  listRoles,
+  getRole,
+  seedRole,
+  createRole,
+  renameRole,
+  deleteRole,
+  type RoleRow,
+  type CreateRoleResult,
+  type RenameRoleResult,
+  type DeleteRoleResult,
+} from './queries';

src/databases/officer_db/src/roles/queries.ts

@@ -1,6 +1,7 @@
-import { asc, eq, sql } from 'drizzle-orm';
+import { and, asc, eq, sql } from 'drizzle-orm';
 import { db } from '../db';
 import { users } from '../auth/schema';
+import type { UserRole } from '../auth/schema';
 import { roles } from './schema';
 
 export type RoleRow = {
@@ -59,3 +60,137 @@ export async function seedRole(row: {
     .returning({ name: roles.name });
   return inserted.length > 0;
 }
+
+// ── Write half: role CRUD ─────────────────────────────────────────────────────────────────────────
+//
+// Every function here returns a discriminated result instead of throwing, because the router has to give
+// a person a reason they can act on ("Developer is a system role" vs "3 accounts still hold it" are two
+// different screens), and a caught exception only carries a string to parse. The one thing each guard
+// shares: it re-checks its own invariant inside the write, not just before it, so a race between the
+// check and the statement fails closed rather than silently succeeding.
+
+/** How many accounts hold this role. One definition, so rename and delete cannot disagree on "in use". */
+async function roleHolderCount(name: string): Promise<number> {
+  const [row] = await db
+    .select({ count: sql<number>`count(*)::int` })
+    .from(users)
+    .where(eq(users.role, name as UserRole));
+  return row?.count ?? 0;
+}
+
+export type CreateRoleResult = { ok: true; role: RoleRow } | { ok: false; reason: 'invalid-name' | 'duplicate' };
+
+/**
+ * Create a role at runtime. `isSystem` is always false here — a role the platform's own code names by
+ * string (Developer, Super Admin, Member; see the schema comment) is one seeded at startup, never one
+ * typed into a form, and nothing reachable through this function may claim otherwise.
+ */
+export async function createRole(input: { name: string; description?: string | null }): Promise<CreateRoleResult> {
+  const name = input.name.trim();
+  if (!name) return { ok: false, reason: 'invalid-name' };
+
+  const inserted = await db
+    .insert(roles)
+    .values({ name, description: input.description?.trim() || null, isSystem: false })
+    .onConflictDoNothing({ target: roles.name })
+    .returning();
+  const role = inserted[0];
+  if (!role) return { ok: false, reason: 'duplicate' };
+  return { ok: true, role };
+}
+
+export type RenameRoleResult =
+  | { ok: true; role: RoleRow }
+  | { ok: false; reason: 'not-found' | 'system' | 'invalid-name' | 'duplicate' | 'in-use'; holderCount?: number };
+
+/**
+ * Rename a role. Refuses outright on `isSystem` (D4 in docs/roles-and-teams.md): `rolePermitsDevTools()`
+ * tests for `'Developer'` by string, `bootstrap.ts` writes `'Super Admin'`, and `users.role` defaults to
+ * `'Member'`. Renaming one of those four is a code change across three call sites that has not happened,
+ * not a data edit — so this refuses rather than quietly breaking them.
+ *
+ * `users.role` and `role_permissions.role` are NOT yet foreign keys onto this table (phase 2 part 2 of the
+ * plan, shipped as its own release), so this UPDATE does not cascade anywhere today — it only relabels the
+ * `roles` row itself, leaving every holder and every grant still naming the OLD name. Once the FK lands
+ * with `ON UPDATE CASCADE`, this same statement starts moving them automatically and nothing here changes.
+ */
+export async function renameRole(from: string, to: string): Promise<RenameRoleResult> {
+  const name = to.trim();
+  if (!name) return { ok: false, reason: 'invalid-name' };
+
+  const existing = await getRole(from);
+  if (!existing) return { ok: false, reason: 'not-found' };
+  if (existing.isSystem) return { ok: false, reason: 'system' };
+  if (name === from) return { ok: true, role: existing };
+
+  /*
+    Refuse a rename while anybody holds it, for exactly as long as there is no foreign key.
+
+    `users.role` is a plain text column today — phase 2 part 2 of `docs/roles-and-teams.md` ships the
+    foreign key with ON UPDATE CASCADE as its own release — so renaming this row would leave every holder
+    pointing at a name that no longer exists.
+
+    It happens to be unreachable right now: `manage-users.ts` validates an incoming role against the
+    `USER_ROLES` union, so a role created at runtime cannot be assigned and a non-system role has no
+    holders. That is safety by accident, and it evaporates the moment part 4 makes `UserRole` a plain
+    string. This guard means those two parts can land in either order without opening a window.
+
+    Delete it when the cascade exists, and not before — the cascade then does this job properly, by moving
+    the holders rather than refusing.
+  */
+  const held = await roleHolderCount(from);
+  if (held > 0) return { ok: false, reason: 'in-use', holderCount: held };
+
+  const clash = await getRole(name);
+  if (clash) return { ok: false, reason: 'duplicate' };
+
+  // `isSystem: false` in the WHERE is the re-check: belt and suspenders against a role becoming system
+  // between the read above and this write.
+  const [role] = await db
+    .update(roles)
+    .set({ name })
+    .where(and(eq(roles.name, from), eq(roles.isSystem, false)))
+    .returning();
+  if (!role) return { ok: false, reason: 'system' };
+  return { ok: true, role };
+}
+
+export type DeleteRoleResult =
+  | { ok: true }
+  | { ok: false; reason: 'not-found' | 'system' | 'in-use'; holderCount?: number };
+
+/**
+ * Delete a role. Refuses if it is system-named (D4), and refuses if any account currently holds it.
+ *
+ * Never reassigns holders to a fallback — silently moving accounts onto a different role's grants is a
+ * permission change nobody asked for, and "absence denies" is the posture everywhere else in this system.
+ * The holder count and the delete run in one transaction so a role cannot be handed to someone in the gap
+ * between counting and deleting.
+ */
+export async function deleteRole(name: string): Promise<DeleteRoleResult> {
+  const existing = await getRole(name);
+  if (!existing) return { ok: false, reason: 'not-found' };
+  if (existing.isSystem) return { ok: false, reason: 'system' };
+
+  return db.transaction(async (tx) => {
+    // `name` is not necessarily one of the four literal roles the column's TS type enumerates — this
+    // function accepts any role that exists as a ROW, which by definition this one is not (yet, if it
+    // never was). The cast is type-level only: `role` is a plain `text` column, so drizzle emits the same
+    // `role = $1` whether or not the value belongs to that union.
+    const [holders] = await tx
+      .select({ count: sql<number>`count(*)::int` })
+      .from(users)
+      .where(eq(users.role, name as UserRole));
+    const count = holders?.count ?? 0;
+    if (count > 0) return { ok: false, reason: 'in-use', holderCount: count };
+
+    // Same re-check as renameRole, same reason: the row must not vanish out from under a role that
+    // became system between the read above and this statement.
+    const deleted = await tx
+      .delete(roles)
+      .where(and(eq(roles.name, name), eq(roles.isSystem, false)))
+      .returning({ name: roles.name });
+    if (!deleted.length) return { ok: false, reason: 'system' };
+    return { ok: true };
+  });
+}