Repository navigation
feat(role-groups): composite add-styled-role v1 #1557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3480c9a
2fc96d6
d0ddbae
79c871b
a90e9be
c16b887
2fbcc21
98495f5
93d42b1
6213dd7
70dd7d7
7ae4f83
abdb6c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # ADR: Role Groups Composite Actions & Concurrency Hardening | ||
|
|
||
| **Date:** 2026-06-23 | ||
| **Status:** Accepted | ||
| **Scope:** Role Groups service (feat/role-groups PR #1544) | ||
|
|
||
| ## Decision | ||
|
|
||
| ### Shipped (v2.21.0+) | ||
|
|
||
| 1. **IDOR Protection (A1, A2):** All role group queries now require guild ownership verification. `addRoleToGroup` now signature-checks `guildId`; `createRoleGroup` rejects `fromMessageId` from other guilds. Integration tests added. | ||
| 2. **Feature-Breaking Validation (B1):** Schema fixed to accept CUID message IDs (not Discord snowflakes). Existing tests pass. | ||
| 3. **Tie-Breaking Correctness (C1):** `seedStyleFromMessage` now correctly defaults to Primary on any non-Primary tie. Test updated to exercise real Secondary×1 + Danger×1 case. | ||
| 4. **Type Safety (D):** All `any` types replaced with Prisma model types (`RoleGroupModel`, `ReactionRoleMappingModel`). Lint reports 0 errors (from 12). | ||
| 5. **Test Hygiene (E1):** `process.env.DISCORD_TOKEN` now restored after each test suite (afterAll hook). | ||
| 6. **UX (E2):** Error state cleared before retry in `handleCreateRoleGroup`, preventing stale banners. | ||
| 7. **Transaction Atomicity (F1):** `createRoleGroup` link operation wrapped in `prisma.$transaction` with conditional `updateMany` to detect race and reject if message already grouped during transaction. | ||
|
|
||
| ### Deferred (v2.2 post-launch) | ||
|
|
||
| **F2: Concurrency Guard for `addRoleToMessage` Appends** | ||
|
|
||
| - Current design: guard checks `length >= 25` but concurrent appends can both pass, then both insert. | ||
| - Symptom: Message ends up with >25 buttons. | ||
| - Fix: Wrap in transaction + final constraint OR DB unique on (messageId, sequence). | ||
| - Opened GitHub issue #1558 (label: `ready-for-agent`). | ||
| - Priority: v2.2; does not block launch. | ||
|
|
||
| ## Also Filed | ||
|
|
||
| - **Issue #1559:** CI lint gate reports green despite 12 type-aware errors in backend (type safety gate gap). | ||
|
|
||
| ## Test Coverage | ||
|
|
||
| - New IDOR tests: `createRoleGroup` rejects cross-guild message, `addRoleToGroup` rejects cross-guild group. | ||
| - Tie-breaking test: real Secondary+Danger tie. | ||
| - Concurrency (transaction): conditional updateMany validates link atomicity. | ||
|
|
||
| ## References | ||
|
|
||
| - Service: `packages/backend/src/services/RoleGroupService.ts` | ||
| - Routes: `packages/backend/src/routes/roleGroups.ts` | ||
| - Schema: `packages/backend/src/schemas/management.ts` | ||
| - Tests: `packages/backend/tests/unit/services/RoleGroupService.test.ts`, `packages/backend/tests/integration/routes/roleGroups.test.ts` | ||
| - Frontend: `packages/frontend/src/pages/ReactionRoles.tsx` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| import type { Express, Response } from 'express' | ||
| import type { AuthenticatedRequest } from '../middleware/auth' | ||
| import { validateParams, validateBody } from '../middleware/validate' | ||
| import { asyncHandler } from '../middleware/asyncHandler' | ||
| import { AppError } from '../errors/AppError' | ||
| import { managementSchemas as s } from '../schemas/management' | ||
| import { writeLimiter } from '../middleware/rateLimit' | ||
| import { roleGroupService } from '../services/RoleGroupService' | ||
|
|
||
| function p(val: string | string[]): string { | ||
| return typeof val === 'string' ? val : val[0] | ||
| } | ||
|
|
||
| export function setupRoleGroupsRoutes(app: Express): void { | ||
| app.post( | ||
| '/api/guilds/:guildId/role-groups', | ||
| writeLimiter, | ||
| validateParams(s.guildIdParam), | ||
| validateBody(s.createRoleGroupBody), | ||
| asyncHandler(async (req: AuthenticatedRequest, res: Response) => { | ||
| const guildId = p(req.params.guildId) | ||
| const data = s.createRoleGroupBody.parse(req.body) | ||
|
|
||
| try { | ||
| const group = await roleGroupService.createRoleGroup({ | ||
| guildId, | ||
| name: data.name, | ||
| fromMessageId: data.fromMessageId, | ||
| style: data.style, | ||
| }) | ||
| res.status(201).json(group) | ||
| } catch (error: unknown) { | ||
| if (error instanceof AppError) { | ||
| throw error as AppError | ||
|
Check warning on line 34 in packages/backend/src/routes/roleGroups.ts
|
||
| } | ||
| const err = | ||
| error instanceof Error ? error : new Error(String(error)) | ||
| const message = err.message || 'Failed to create role group' | ||
| if ( | ||
| message.includes('not found') || | ||
| message.includes('not-found') | ||
| ) { | ||
| throw AppError.notFound(message) | ||
| } | ||
| if ( | ||
| message.includes('conflict') || | ||
| message.includes('already-grouped') | ||
| ) { | ||
| throw AppError.conflict(message) | ||
| } | ||
| throw AppError.badRequest(message) | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| app.get( | ||
| '/api/guilds/:guildId/role-groups', | ||
| validateParams(s.guildIdParam), | ||
| asyncHandler(async (req: AuthenticatedRequest, res: Response) => { | ||
| const guildId = p(req.params.guildId) | ||
|
|
||
| const groups = await roleGroupService.listRoleGroups(guildId) | ||
| res.json({ groups }) | ||
| }), | ||
| ) | ||
|
|
||
| app.get( | ||
| '/api/guilds/:guildId/role-groups/:id', | ||
| validateParams(s.roleGroupIdParam), | ||
| asyncHandler(async (req: AuthenticatedRequest, res: Response) => { | ||
| const guildId = p(req.params.guildId) | ||
| const id = p(req.params.id) | ||
|
|
||
| const group = await roleGroupService.getRoleGroup(id, guildId) | ||
| if (!group) { | ||
| throw AppError.notFound('Role group not found') | ||
| } | ||
| res.json(group) | ||
| }), | ||
| ) | ||
|
|
||
| app.patch( | ||
| '/api/guilds/:guildId/role-groups/:id', | ||
| writeLimiter, | ||
| validateParams(s.roleGroupIdParam), | ||
| validateBody(s.updateRoleGroupBody), | ||
| asyncHandler(async (req: AuthenticatedRequest, res: Response) => { | ||
| const guildId = p(req.params.guildId) | ||
| const id = p(req.params.id) | ||
| const data = s.updateRoleGroupBody.parse(req.body) | ||
|
|
||
| try { | ||
| const group = await roleGroupService.updateRoleGroup( | ||
| id, | ||
| guildId, | ||
| data, | ||
| ) | ||
| if (!group) { | ||
| throw AppError.notFound('Role group not found') | ||
| } | ||
| res.json(group) | ||
| } catch (error: unknown) { | ||
| if (error instanceof AppError) { | ||
| throw error as AppError | ||
|
Check warning on line 104 in packages/backend/src/routes/roleGroups.ts
|
||
| } | ||
| const err = | ||
| error instanceof Error ? error : new Error(String(error)) | ||
| const message = err.message || 'Failed to update role group' | ||
| throw AppError.badRequest(message) | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| app.post( | ||
| '/api/guilds/:guildId/role-groups/:id/roles', | ||
| writeLimiter, | ||
| validateParams(s.roleGroupIdParam), | ||
| validateBody(s.addRoleToGroupBody), | ||
| asyncHandler(async (req: AuthenticatedRequest, res: Response) => { | ||
| const guildId = p(req.params.guildId) | ||
| const id = p(req.params.id) | ||
| const botToken = process.env.DISCORD_TOKEN?.trim() | ||
| if (!botToken) { | ||
| throw AppError.serviceUnavailable('Bot token not configured') | ||
| } | ||
|
|
||
| const data = s.addRoleToGroupBody.parse(req.body) | ||
|
|
||
| try { | ||
| const result = await roleGroupService.addRoleToGroup( | ||
| guildId, | ||
| id, | ||
| data, | ||
| botToken, | ||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| res.json(result) | ||
| } catch (error: unknown) { | ||
| if (error instanceof AppError) { | ||
| throw error as AppError | ||
|
Check warning on line 139 in packages/backend/src/routes/roleGroups.ts
|
||
| } | ||
| const err = | ||
| error instanceof Error ? error : new Error(String(error)) | ||
| const message = err.message || 'Failed to add role to group' | ||
|
|
||
| // Check for specific error patterns (from tests and RoleGroupService) | ||
| if ( | ||
| message.includes('not found') || | ||
| message.includes('not-found') | ||
| ) { | ||
| throw AppError.notFound(message) | ||
| } | ||
| if ( | ||
| message.includes('25 buttons') || | ||
| message.includes('25-buttons') || | ||
| message.includes('250 roles') || | ||
| message.includes('250-roles') || | ||
| message.includes('already mapped') || | ||
| message.includes('duplicate-name') | ||
| ) { | ||
| throw AppError.conflict(message) | ||
| } | ||
| if (message.includes('Label too long')) { | ||
| throw AppError.badRequest(message) | ||
| } | ||
| throw AppError.badRequest(message) | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| app.delete( | ||
| '/api/guilds/:guildId/role-groups/:id/roles/:roleId', | ||
| writeLimiter, | ||
| validateParams(s.roleGroupRoleIdParam), | ||
| asyncHandler(async (req: AuthenticatedRequest, res: Response) => { | ||
| const guildId = p(req.params.guildId) | ||
| const id = p(req.params.id) | ||
| const roleId = p(req.params.roleId) | ||
|
|
||
| const success = await roleGroupService.detachRoleFromGroup( | ||
| id, | ||
| roleId, | ||
| guildId, | ||
| ) | ||
| if (!success) { | ||
| throw AppError.notFound('Role group or role not found') | ||
| } | ||
| res.json({ success: true }) | ||
| }), | ||
| ) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.