fix: rollback on rename throw, description validation, delete/DRY cleanup

- Hoist isRename/trimmedName above try block so catch can roll back user
  migration when updateRoleByName throws (not just returns null)
- Add description type + max-length (2000) validation in create and update,
  consistent with groups handler
- Remove redundant getRoleByName existence check in deleteRoleHandler —
  use deleteRoleByName return value directly
- Skip no-op name write when body.name equals current name (use isRename)
- Extract getUserModel() accessor to DRY repeated Model<IUser> casts
- Use name.trim() consistently in createRoleByName error messages
- Add tests: rename-throw rollback, description validation (create+update),
  update delete test mocks to match simplified handler

packages/api/src/admin/roles.spec.ts

@@ -191,6 +191,22 @@ describe('createAdminRolesHandlers', () => {
       expect(deps.createRoleByName).not.toHaveBeenCalled();
     });
 
+    it('returns 400 when description exceeds max length', async () => {
+      const deps = createDeps();
+      const handlers = createAdminRolesHandlers(deps);
+      const { req, res, status, json } = createReqRes({
+        body: { name: 'editor', description: 'a'.repeat(2001) },
+      });
+
+      await handlers.createRole(req, res);
+
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({
+        error: 'description must not exceed 2000 characters',
+      });
+      expect(deps.createRoleByName).not.toHaveBeenCalled();
+    });
+
     it('returns 409 when role already exists', async () => {
       const deps = createDeps({
         createRoleByName: jest.fn().mockRejectedValue(new Error('Role "editor" already exists')),
@@ -487,6 +503,44 @@ describe('createAdminRolesHandlers', () => {
       expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor');
     });
 
+    it('rolls back user migration when rename throws', async () => {
+      const deps = createDeps({
+        getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null),
+        updateRoleByName: jest.fn().mockRejectedValue(new Error('db crash')),
+      });
+      const handlers = createAdminRolesHandlers(deps);
+      const { req, res, status } = createReqRes({
+        params: { name: 'editor' },
+        body: { name: 'new-name' },
+      });
+
+      await handlers.updateRole(req, res);
+
+      expect(status).toHaveBeenCalledWith(500);
+      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(2);
+      expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(1, 'editor', 'new-name');
+      expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor');
+    });
+
+    it('returns 400 when description exceeds max length', async () => {
+      const deps = createDeps({
+        getRoleByName: jest.fn().mockResolvedValue(mockRole()),
+      });
+      const handlers = createAdminRolesHandlers(deps);
+      const { req, res, status, json } = createReqRes({
+        params: { name: 'editor' },
+        body: { description: 'a'.repeat(2001) },
+      });
+
+      await handlers.updateRole(req, res);
+
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({
+        error: 'description must not exceed 2000 characters',
+      });
+      expect(deps.updateRoleByName).not.toHaveBeenCalled();
+    });
+
     it('returns 500 on unexpected error', async () => {
       const deps = createDeps({
         getRoleByName: jest.fn().mockResolvedValue(mockRole()),
@@ -586,7 +640,7 @@ describe('createAdminRolesHandlers', () => {
 
   describe('deleteRole', () => {
     it('deletes role and returns 200', async () => {
-      const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()) });
+      const deps = createDeps();
       const handlers = createAdminRolesHandlers(deps);
       const { req, res, status, json } = createReqRes({ params: { name: 'editor' } });
 
@@ -610,7 +664,7 @@ describe('createAdminRolesHandlers', () => {
     });
 
     it('returns 404 when role not found', async () => {
-      const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(null) });
+      const deps = createDeps({ deleteRoleByName: jest.fn().mockResolvedValue(null) });
       const handlers = createAdminRolesHandlers(deps);
       const { req, res, status, json } = createReqRes({ params: { name: 'nonexistent' } });
 
@@ -622,7 +676,6 @@ describe('createAdminRolesHandlers', () => {
 
     it('returns 500 on error', async () => {
       const deps = createDeps({
-        getRoleByName: jest.fn().mockResolvedValue(mockRole()),
         deleteRoleByName: jest.fn().mockRejectedValue(new Error('db down')),
       });
       const handlers = createAdminRolesHandlers(deps);

packages/api/src/admin/roles.ts

@@ -6,6 +6,7 @@ import type { Response } from 'express';
 import type { ServerRequest } from '~/types/http';
 
 const MAX_NAME_LENGTH = 500;
+const MAX_DESCRIPTION_LENGTH = 2000;
 
 interface RoleNameParams {
   name: string;
@@ -93,6 +94,14 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
           .status(400)
           .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` });
       }
+      if (description !== undefined && typeof description !== 'string') {
+        return res.status(400).json({ error: 'description must be a string' });
+      }
+      if (description && description.length > MAX_DESCRIPTION_LENGTH) {
+        return res
+          .status(400)
+          .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` });
+      }
       const role = await createRoleByName({
         name: name.trim(),
         description: description ?? '',
@@ -111,10 +120,11 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
   }
 
   async function updateRoleHandler(req: ServerRequest, res: Response) {
+    const { name } = req.params as RoleNameParams;
+    const body = req.body as Partial<IRole>;
+    let isRename = false;
+    let trimmedName = '';
     try {
-      const { name } = req.params as RoleNameParams;
-      const body = req.body as Partial<IRole>;
-
       if (
         body.name !== undefined &&
         (!body.name || typeof body.name !== 'string' || !body.name.trim())
@@ -127,8 +137,8 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
           .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` });
       }
 
-      const trimmedName = body.name?.trim();
-      const isRename = trimmedName !== undefined && trimmedName !== name;
+      trimmedName = body.name?.trim() ?? '';
+      isRename = trimmedName !== '' && trimmedName !== name;
 
       if (isRename && SystemRoles[name as keyof typeof SystemRoles]) {
         return res.status(403).json({ error: 'Cannot rename system role' });
@@ -149,8 +159,17 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
         }
       }
 
+      if (body.description !== undefined && typeof body.description !== 'string') {
+        return res.status(400).json({ error: 'description must be a string' });
+      }
+      if (body.description && body.description.length > MAX_DESCRIPTION_LENGTH) {
+        return res
+          .status(400)
+          .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` });
+      }
+
       const updates: Partial<IRole> = {};
-      if (trimmedName !== undefined) {
+      if (isRename) {
         updates.name = trimmedName;
       }
       if (body.description !== undefined) {
@@ -171,6 +190,13 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
 
       return res.status(200).json({ role });
     } catch (error) {
+      if (isRename) {
+        try {
+          await updateUsersByRole(trimmedName, name);
+        } catch (rollbackError) {
+          logger.error('[adminRoles] rollback failed after updateRole error:', rollbackError);
+        }
+      }
       logger.error('[adminRoles] updateRole error:', error);
       return res.status(500).json({ error: 'Failed to update role' });
     }
@@ -208,12 +234,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
         return res.status(403).json({ error: 'Cannot delete system role' });
       }
 
-      const existing = await getRoleByName(name);
-      if (!existing) {
+      const deleted = await deleteRoleByName(name);
+      if (!deleted) {
         return res.status(404).json({ error: 'Role not found' });
       }
-
-      await deleteRoleByName(name);
       return res.status(200).json({ success: true });
     } catch (error) {
       logger.error('[adminRoles] deleteRole error:', error);