Repository navigation
feat: add guild automation control-plane core - #192
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for regal-bunny-0c8efe ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces new management API endpoints for automod templates and guild channel access, adds a Criativaria preset builder, refactors the guild automation execution layer by removing the GuildAutomationExecutionService and replacing distributed Redis-based locking with in-memory locking, simplifies various handlers through code inlining, and updates the database schema to adjust the relationship between manifests and automation runs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bot/src/utils/guildAutomation/applyPlan.ts (1)
1-16: 🛠️ Refactor suggestion | 🟠 MajorMissing
errorLogimport for error handling.The file lacks the
errorLogimport from@lucky/shared/utils, which is required per bot package guidelines. This is needed for proper error logging in the deletion catch block (line 185).import { autoMessageService, autoModService, manifestOnboardingToDiscordEdit, guildRoleAccessService, roleManagementService, updateModerationSettings, type GuildAutomationManifestDocument, type GuildAutomationPlan, } from '@lucky/shared/services' +import { errorLog } from '@lucky/shared/utils'As per coding guidelines: "Use
errorLoganddebugLogfrom@lucky/shared/utilsfor logging throughout the bot package."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.ts` around lines 1 - 16, The file is missing the required errorLog import from `@lucky/shared/utils` which is used for error handling in the deletion catch block inside applyPlan.ts; add errorLog to the module imports (alongside any existing imports from '@lucky/shared/utils') so the catch block uses errorLog(...) instead of an undefined identifier, ensuring consistent logging per bot package guidelines and referencing the deletion catch block and functions like applyPlan where the error is currently handled.
🧹 Nitpick comments (14)
packages/bot/src/utils/guildAutomation/applyPlan.ts (2)
168-176: Inconsistent error handling between role and channel deletion.Role deletion (lines 173-175) has no error handling and will throw on failure, aborting the entire operation. Channel deletion (lines 183-188) catches errors and continues. Consider whether this asymmetry is intentional—if a role deletion fails mid-operation, previously applied changes won't be rolled back.
If role deletion should also be resilient:
♻️ Optional: Add consistent error handling for role deletion
if (role.editable) { - await role.delete('Lucky guild automation protected-delete apply') + try { + await role.delete('Lucky guild automation protected-delete apply') + } catch (error) { + errorLog({ + message: 'Failed to delete role during guild automation apply', + data: { guildId: guild.id, roleId: role.id, error }, + }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.ts` around lines 168 - 176, Role deletion in the roles loop (the await role.delete('Lucky guild automation protected-delete apply') call in applyPlan.ts) lacks error handling and will abort the whole operation on failure; wrap the await role.delete(...) in a try/catch and handle errors the same way channel deletion does (catch errors, log them with the same logger or console, and continue) so a single failed role.delete won't stop the rest of the automation from applying.
223-296: Function exceeds 50-line limit.
applyAutomationModulesspans 73 lines, exceeding the 50-line guideline. Consider extracting module-specific handlers while keeping the orchestration simple:♻️ Suggested refactor approach
// Extract handlers that return applied/skipped module names async function applyOnboardingModule( guild: Guild, desired: GuildAutomationManifestDocument, ): Promise<string | null> { const payload = manifestOnboardingToDiscordEdit(desired.onboarding) if (payload) { await guild.editOnboarding(payload) return 'onboarding' } return null } // Similar handlers for each module... // Then orchestrate in applyAutomationModules: const handlers = [ { module: 'onboarding', fn: () => applyOnboardingModule(guild, desired) }, { module: 'roles', fn: () => applyRolesModule(guild, desired, allowProtected) }, // ... ] for (const { module, fn } of handlers) { if (shouldApplyModule(plan, module, allowProtected)) { const result = await fn() if (result) appliedModules.push(result) } }As per coding guidelines: "Functions must be less than 50 lines with cyclomatic complexity less than 10."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.ts` around lines 223 - 296, The applyAutomationModules function is too long; refactor by extracting each module's logic into small handler functions (e.g., applyOnboardingModule, applyRolesModule, applyModerationModule, applyAutoMessagesModule, applyReactionRolesModule, applyCommandAccessModule, applyParityModule) that return either an applied module name or a skipped message (string | null or {applied?:string, skipped?:string}), then simplify applyAutomationModules to iterate over a handlers array and call shouldApplyModule(plan, '<module>', allowProtected) before invoking each handler and collecting returned applied/skipped names; keep existing helpers (manifestOnboardingToDiscordEdit, applyRolesAndChannels, autoModService.updateSettings, updateModerationSettings, upsertAutoMessage, applyReactionRoleRules, guildRoleAccessService.replaceRoleGrants) inside the new handlers so behavior is unchanged.packages/bot/src/utils/guildAutomation/captureGuildState.ts (1)
83-189: Function exceeds 50-line guideline.
captureGuildAutomationStatespans ~106 lines, exceeding the 50-line function limit. The logic is largely declarative data mapping with low cyclomatic complexity, so this is a lower-priority refactor. If addressed, consider extracting thePromise.allcalls and return object construction into smaller helper functions.As per coding guidelines: "Functions must be less than 50 lines with cyclomatic complexity less than 10."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/captureGuildState.ts` around lines 83 - 189, captureGuildAutomationState is too long; split its responsibilities into smaller helpers: extract the async parallel fetch block (the Promise.all that assigns automodSettings, moderationSettings, welcomeMessage, leaveMessage, reactionRoleMessages, exclusiveRoles, roleGrants, parity) into a new helper like fetchGuildAutomationData(guildId, botUserId) that returns a typed object, and move the large return object construction into a buildGuildManifest(guild, onboarding, fetchedData, roles, channels) helper; keep captureGuildAutomationState to calling those helpers, computing roles/channels, and returning the result. Reference the existing symbols captureGuildAutomationState, captureParity, autoModService.getSettings, autoMessageService.getWelcomeMessage/getLeaveMessage, reactionRolesService.listReactionRoleMessages, roleManagementService.listExclusiveRoles, guildRoleAccessService.listRoleGrants in the new helpers so locating code is straightforward.packages/shared/src/services/guildAutomation/manifestSchema.ts (1)
120-125: Schema relaxation removes structural validation for moderation settings.Changing
automodandmoderationSettingsfrom typed schemas toz.record(z.unknown())accepts any object structure. This loses:
- Compile-time type checking for known fields
- Runtime validation of expected properties
- Clear documentation of the expected shape
If this flexibility is intentional (e.g., to support evolving automod configurations), consider adding a brief code comment explaining the design decision per coding guidelines. Alternatively, define a minimal base schema with
.passthrough()to validate known fields while allowing extras.💡 Alternative: Minimal base validation with passthrough
const automodBaseSchema = z.object({ enabled: z.boolean().optional(), }).passthrough() // Then use automodBaseSchema.optional() instead of z.record(z.unknown()).optional()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/manifestSchema.ts` around lines 120 - 125, The moderation schema currently relaxes structure by using z.record(z.unknown()) for fields automod and moderationSettings; restore meaningful validation by replacing those records with a minimal base schema (e.g., an automodBaseSchema with known properties like enabled and other expected keys, using .passthrough() to allow extras) and use automodBaseSchema.optional() and a similar base for moderationSettings, or if the loose shape is intentional add a concise code comment explaining why structural validation was removed; update the object in manifestSchema.ts (the moderation object containing automod and moderationSettings) to reference these base schemas to preserve runtime validation and compile-time typing.packages/bot/src/functions/moderation/commands/index.ts (1)
2-3: Usenode:prefix for Node.js built-in modules.Same issue as flagged in
automod/commands/index.ts. For consistency and ESM best practices, use thenode:prefix.♻️ Proposed fix
-import path from 'path' -import { fileURLToPath } from 'url' +import path from 'node:path' +import { fileURLToPath } from 'node:url'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/moderation/commands/index.ts` around lines 2 - 3, Update the built-in module imports to use the Node.js "node:" protocol for ESM consistency: replace the import of path and fileURLToPath from 'path' and 'url' with the namespaced imports using 'node:path' and 'node:url' (refer to the top-level import statements that currently import path and fileURLToPath in packages/bot/src/functions/moderation/commands/index.ts). Ensure both import specifiers are updated so other code using path and fileURLToPath remains unchanged.packages/bot/src/functions/automod/commands/index.ts (1)
2-3: Usenode:prefix for Node.js built-in modules.SonarCloud correctly flags that ESM best practice is to use
node:pathandnode:urlfor built-in modules. This improves clarity and avoids potential conflicts with npm packages.♻️ Proposed fix
-import path from 'path' -import { fileURLToPath } from 'url' +import path from 'node:path' +import { fileURLToPath } from 'node:url'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/automod/commands/index.ts` around lines 2 - 3, Change the imports of Node built-ins to use the node: prefix: replace "import path from 'path'" with "import path from 'node:path'" and "import { fileURLToPath } from 'url'" with "import { fileURLToPath } from 'node:url'"; update any references to path and fileURLToPath in this module (e.g., the top-level import statements in this file) so they continue to work unchanged after switching to node: imports.packages/bot/src/functions/management/commands/guildconfig.ts (2)
97-401: File exceeds 250-line limit.The
executefunction spans ~300 lines with inline subcommand handling. While the consolidated approach is readable, the file is approximately 400 lines total, exceeding the project's 250-line guideline.Consider extracting subcommand handlers into separate functions or a handlers module to reduce file length, though this can be deferred given the clear structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/management/commands/guildconfig.ts` around lines 97 - 401, The execute handler in guildconfig.ts has grown too large (contains inline logic for subcommands capture, plan, apply/reconcile, status, cutover) and should be split to keep the file under the 250-line guideline; refactor by extracting each subcommand block into separate functions (e.g., handleCapture, handlePlan, handleApplyOrReconcile, handleStatus, handleCutover) that accept the same context (interaction, guild) and call existing helpers like captureGuildAutomationState, guildAutomationService methods, applyAutomationModules, interactionReply and errorLog, then replace the inline blocks in execute with a small dispatcher that calls these new handler functions based on subcommand; keep existing behavior and return values/updates (including interaction.editReply and guildAutomationService updates) so tests and logic remain unchanged.
203-229: Invert negated condition for clarity.SonarCloud flagged the negated condition
if (!blockedByProtected). Inverting the branches improves readability:♻️ Proposed refactor to use positive condition first
- if (!blockedByProtected) { - const applyResult = await applyAutomationModules({ - guild, - desired: - planResult.desired as GuildAutomationManifestDocument, - plan: planResult.plan, - allowProtected, - }) - - applyDiagnostics = { - ...applyDiagnostics, - appliedModules: applyResult.appliedModules, - skippedModules: applyResult.skippedModules, - } - - await guildAutomationService.updateRunStatus({ - runId: planResult.runId, - status: 'completed', - diagnostics: applyDiagnostics, - }) - } else { + if (blockedByProtected) { await guildAutomationService.updateRunStatus({ runId: planResult.runId, status: 'blocked', diagnostics: applyDiagnostics, }) + } else { + const applyResult = await applyAutomationModules({ + guild, + desired: + planResult.desired as GuildAutomationManifestDocument, + plan: planResult.plan, + allowProtected, + }) + + applyDiagnostics = { + ...applyDiagnostics, + appliedModules: applyResult.appliedModules, + skippedModules: applyResult.skippedModules, + } + + await guildAutomationService.updateRunStatus({ + runId: planResult.runId, + status: 'completed', + diagnostics: applyDiagnostics, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/management/commands/guildconfig.ts` around lines 203 - 229, Invert the negated condition for clarity: replace the `if (!blockedByProtected)` branch with a positive `if (blockedByProtected)` first so the blocked case calls `guildAutomationService.updateRunStatus` with status 'blocked' and diagnostics, and the else branch performs the apply flow (calling `applyAutomationModules` with `guild`, `planResult.desired`, `planResult.plan`, `allowProtected`), merges `applyResult` into `applyDiagnostics` and then calls `guildAutomationService.updateRunStatus` with status 'completed'; update references to `planResult.runId`, `applyDiagnostics`, and `applyResult` accordingly to preserve behavior.packages/shared/src/services/guildAutomation/types.ts (2)
196-202: Type widening from enum literals tostringloses exhaustiveness checking.The
latestRun.type,latestRun.status,drifts[].module, anddrifts[].severityfields are now typed asstringinstead of the specific union types (AutomationRunType,AutomationRunStatus,AutomationModule,DriftSeverity).This aligns with the Prisma schema (which stores plain
Stringfields), but consumers performing exhaustive pattern matching (e.g.,switchon status) will silently accept invalid values without TypeScript warnings.Consider adding runtime validation in
getStatus()if strict enum conformance is needed, or keep this as a documentation note that database values may not conform to expected literals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/types.ts` around lines 196 - 202, The fields latestRun.type, latestRun.status, drifts[].module and drifts[].severity were widened to plain string losing exhaustiveness checks; revert those specific fields in packages/shared/src/services/guildAutomation/types.ts to the corresponding union types (AutomationRunType, AutomationRunStatus, AutomationModule, DriftSeverity) so TypeScript preserves literal unions for consumers, and if DB string values may be invalid add runtime validation inside the getStatus() helper (validate and map/throw/fallback to a safe sentinel like "UNKNOWN") to ensure only expected enum literals are used at runtime.
76-86: Type relaxation trades safety for flexibility.The change from specific typed interfaces (
AutoModSettingsUpdate,ModerationSettingsUpdate) to permissive index signatures ([key: string]: unknown) allows arbitrary fields to pass through without compile-time validation. This is a reasonable trade-off for a control-plane that needs to handle varying automod configurations, but consumers should validate at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/types.ts` around lines 76 - 86, The types for automod and moderationSettings were relaxed to include permissive index signatures which removes compile-time safety; adjust types.ts by either restoring the original specific update interfaces (AutoModSettingsUpdate, ModerationSettingsUpdate) for known fields and adding an explicit fallback type (e.g., Record<string, unknown> or a named ExtraAutomodFields/ExtraModerationFields) for extension, and/or add runtime validation helpers that consumers can call to validate unknown keys before usage; update the automod and moderationSettings declarations to reference these stricter update types (or the combined specific+fallback types) so callers get type safety for known properties while still allowing extensibility at runtime.packages/bot/src/functions/management/commands/guildconfig.spec.ts (1)
64-172: Test coverage is streamlined but may need expansion.The tests cover the happy path for
capture,plan, andapplysubcommands. Consider adding tests for:
statusandcutoversubcommands- Error handling (service failures)
- Protected operations blocking (
blockedByProtectedbranch)These can be added in a follow-up if the current scope is intentionally limited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/management/commands/guildconfig.spec.ts` around lines 64 - 172, Tests only cover happy paths for capture/plan/apply; add unit tests for the remaining branches: add tests that call guildconfigCommand.execute with the 'status' and 'cutover' subcommands to ensure getStatusMock and runCutoverMock are invoked and interaction.editReply is called, add tests that simulate service failures by having mocks (e.g., createPlanMock, captureGuildAutomationStateMock, runCutoverMock) reject and assert error handling paths (interaction.editReply with error), and add a test that exercises the protected-operations branch (set createPlanMock to return a plan with protected ops and call execute with allow_protected false to assert blockedByProtected handling and that applyAutomationModulesMock is not called); reference guildconfigCommand.execute, getStatusMock, runCutoverMock, createPlanMock, captureGuildAutomationStateMock, applyAutomationModulesMock, recordCaptureMock, and updateRunStatusMock when locating code to change.prisma/schema.prisma (1)
137-147: Drop the extraguildIdindex onGuildAutomationManifest.
guildId@unique`` already gives Postgres an index, so this@@index([guildId])only adds another structure to maintain on every manifest write and is what produced the duplicate SQL index in the migration.♻️ Proposed fix
model GuildAutomationManifest { id String `@id` `@default`(cuid()) guildId String `@unique` version Int `@default`(1) manifest Json moduleOwnership Json? lastCapturedState Json? lastCapturedAt DateTime? createdBy String? createdAt DateTime `@default`(now()) updatedAt DateTime `@updatedAt` - @@index([guildId]) @@map("guild_automation_manifests") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prisma/schema.prisma` around lines 137 - 147, The model defines guildId as unique, which already creates a PostgreSQL index, so the extra index declaration @@index([guildId]) in the GuildAutomationManifest model should be removed to avoid duplicate indexes; locate the GuildAutomationManifest model in schema.prisma and delete the @@index([guildId]) line (leave guildId String `@unique`, version, manifest, etc. unchanged), then regenerate the migration to reflect the removal.packages/shared/src/services/guildAutomation/service.ts (2)
186-196: Extract nested ternary into a helper function for readability.SonarCloud flagged this nested ternary. Consider extracting the actual state resolution logic into a named helper to improve readability.
♻️ Proposed refactor
+ function resolveActualState( + providedState: GuildAutomationManifestInput | undefined, + lastCapturedState: unknown, + ): GuildAutomationManifestDocument | null { + if (providedState) { + return guildAutomationManifestSchema.parse(providedState) + } + if (lastCapturedState) { + return toManifestDocument(lastCapturedState) + } + return null + } // In createPlan: - const actual = options?.actualState - ? guildAutomationManifestSchema.parse(options.actualState) - : manifestRow.lastCapturedState - ? toManifestDocument(manifestRow.lastCapturedState) - : null + const actual = resolveActualState( + options?.actualState, + manifestRow.lastCapturedState, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/service.ts` around lines 186 - 196, Extract the nested ternary that computes actual into a small helper (e.g., resolveActualState or getActualState) that accepts options and manifestRow and returns either guildAutomationManifestSchema.parse(options.actualState), toManifestDocument(manifestRow.lastCapturedState), or null; replace the inline expression in service.ts with a call to that helper and keep the existing null check/throw behavior unchanged so the error message remains the same.
208-215: Extract severity calculation into a named function.SonarCloud flagged the nested ternary. A helper function with clear thresholds improves readability and makes the severity logic reusable.
♻️ Proposed refactor
+ function computeDriftSeverity( + operationCount: number, + ): 'none' | 'low' | 'medium' | 'high' { + if (operationCount === 0) return 'none' + if (operationCount < 3) return 'low' + if (operationCount < 8) return 'medium' + return 'high' + } // In createPlan loop: - const severity: 'none' | 'low' | 'medium' | 'high' = - count === 0 - ? 'none' - : count < 3 - ? 'low' - : count < 8 - ? 'medium' - : 'high' + const severity = computeDriftSeverity(count)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/service.ts` around lines 208 - 215, Replace the nested ternary that assigns severity with a small named helper: implement a function (e.g., getSeverityFromCount or calculateSeverity) that accepts count and returns the union type 'none' | 'low' | 'medium' | 'high' using the same thresholds (0 -> 'none', count < 3 -> 'low', count < 8 -> 'medium', else 'high'), then replace the inline ternary expression that sets the severity variable with a call to that helper; ensure the helper is exported or kept local to the module as appropriate and used everywhere this logic may be needed for reusability and clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/routes/guilds.ts`:
- Around line 115-124: The route handler for GET /api/guilds/:guildId/channels
currently only uses requireAuth and trusts outer wiring; update it to enforce
the same guild-module access middleware used by neighboring guild-scoped
endpoints (e.g., requireGuildModuleAccess) and apply validateParams(...) with
the Zod params schema for guildId (e.g., guildIdParamsSchema) before the async
handler; locate the handler around guildService.getGuildTextChannelOptions and
replace the middleware list to be [requireAuth,
validateParams(guildIdParamsSchema), requireGuildModuleAccess] (or the actual
middleware name used nearby) so guildId is validated locally and access is
checked.
In `@packages/backend/tests/integration/routes/management.test.ts`:
- Around line 251-260: The test is using `as any` casts on the mocked responses
for `mockAutoModService.getSettings` and `mockAutoModService.updateSettings`;
remove those casts and return properly typed objects instead (e.g., create or
import a fixture type matching the service return types or use the TypeScript
`satisfies` operator to assert the shape), or if unavoidable add a single-line
eslint disable with justification above the offending mock (//
eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- explain why) so
the linter is satisfied; update the two mockResolvedValue calls for
`mockAutoModService.getSettings` and `mockAutoModService.updateSettings` to use
the typed fixtures/satisfies assertion or the documented eslint exception.
In `@packages/bot/src/utils/guildAutomation/applyPlan.ts`:
- Around line 183-188: The catch block that swallows errors when deleting
channels in applyPlan should log the failure instead of silently continuing:
import and use errorLog from `@lucky/shared/utils` inside the catch for the await
channel.delete('Lucky guild automation protected-delete apply') call (within the
applyPlan function) and pass a descriptive message plus the caught error and
channel identifier (e.g., channel.id or channel.name) so tests expecting
errorLog are satisfied and failures are recorded; after logging, keep the
continue to preserve current control flow.
- Around line 246-262: The unsafe "as never" casts bypass type safety when
passing desired.moderation.automod and desired.moderation.moderationSettings
into autoModService.updateSettings and updateModerationSettings; replace these
casts by ensuring the manifest has proper types or add runtime validation/guards
before calling the services (e.g., narrow desired.moderation with type
predicates or validate shape/nullability), and only call
autoModService.updateSettings(guild.id, validatedAutomod) and
updateModerationSettings(guild.id, validatedModerationSettings) when the values
pass those checks; update the GuildAutomationManifestDocument types or add
conversion/validation helpers to guarantee correct types rather than using "as
never".
In `@packages/bot/src/utils/guildAutomation/captureGuildState.ts`:
- Line 87: The silent .catch(() => null) on the guild.fetchOnboarding() call
hides all errors; change it to catch the error into a variable, keep the
existing isOnboardingNotConfiguredError check to return null for expected "not
configured" errors, and for all other errors call errorLog from
`@lucky/shared/utils` with structured fields (message, code, stack, cause and any
available correlation/user context) before returning null; ensure errorLog is
imported and reference guild.fetchOnboarding(), onboarding,
isOnboardingNotConfiguredError and errorLog in your fix.
- Around line 143-144: Add a brief inline comment next to the automod assignment
explaining why you cast automodSettings to a loose Record<string, unknown>
(e.g., for manifest/schema serialization, forward compatibility with unknown
future AutoModSettings fields, or to satisfy storage/transport typing),
referencing the automod and automodSettings symbols and the AutoModSettings
interface so reviewers understand the intentional loss of specific typing.
In `@packages/shared/src/services/guildAutomation/service.ts`:
- Line 32: Replace all generic Error throws in the guild automation service with
domain-specific error classes: add new error classes (e.g.,
GuildAutomationNotFoundError with code 'ERR_AUTOMATION_NOT_FOUND',
GuildAutomationInvalidPayloadError with code 'ERR_AUTOMATION_INVALID_PAYLOAD',
and GuildAutomationLockError with code 'ERR_AUTOMATION_LOCK_CONFLICT' and
retryable = true) in the shared types/errors area, export them, and then update
the service.ts locations that currently throw generic errors (the throws for
invalid manifest payload, missing manifest, and lock contention) to throw the
corresponding new classes (use the guildId or relevant context in constructors
and preserve original messages while adding the stable code property).
- Around line 19-20: Replace the in-memory Map-based lock (locks Map) used
around applyManifestDiff() with the Redis-based distributed lock helpers:
acquire the lock by calling RedisClient.setNxPx(lockKey, lockValue, ttl) instead
of setting locks.set(key, ...), and release it with
RedisClient.delIfValueMatches(lockKey, lockValue) instead of deleting from the
Map; update the locking logic in the function(s) that reference locks (e.g.,
applyManifestDiff) to generate a unique lockKey/lockValue, handle acquisition
failure/ttl expiry similarly to the current Map logic, and ensure the release
happens in finally/failure paths to avoid deadlocks.
---
Outside diff comments:
In `@packages/bot/src/utils/guildAutomation/applyPlan.ts`:
- Around line 1-16: The file is missing the required errorLog import from
`@lucky/shared/utils` which is used for error handling in the deletion catch block
inside applyPlan.ts; add errorLog to the module imports (alongside any existing
imports from '@lucky/shared/utils') so the catch block uses errorLog(...)
instead of an undefined identifier, ensuring consistent logging per bot package
guidelines and referencing the deletion catch block and functions like applyPlan
where the error is currently handled.
---
Nitpick comments:
In `@packages/bot/src/functions/automod/commands/index.ts`:
- Around line 2-3: Change the imports of Node built-ins to use the node: prefix:
replace "import path from 'path'" with "import path from 'node:path'" and
"import { fileURLToPath } from 'url'" with "import { fileURLToPath } from
'node:url'"; update any references to path and fileURLToPath in this module
(e.g., the top-level import statements in this file) so they continue to work
unchanged after switching to node: imports.
In `@packages/bot/src/functions/management/commands/guildconfig.spec.ts`:
- Around line 64-172: Tests only cover happy paths for capture/plan/apply; add
unit tests for the remaining branches: add tests that call
guildconfigCommand.execute with the 'status' and 'cutover' subcommands to ensure
getStatusMock and runCutoverMock are invoked and interaction.editReply is
called, add tests that simulate service failures by having mocks (e.g.,
createPlanMock, captureGuildAutomationStateMock, runCutoverMock) reject and
assert error handling paths (interaction.editReply with error), and add a test
that exercises the protected-operations branch (set createPlanMock to return a
plan with protected ops and call execute with allow_protected false to assert
blockedByProtected handling and that applyAutomationModulesMock is not called);
reference guildconfigCommand.execute, getStatusMock, runCutoverMock,
createPlanMock, captureGuildAutomationStateMock, applyAutomationModulesMock,
recordCaptureMock, and updateRunStatusMock when locating code to change.
In `@packages/bot/src/functions/management/commands/guildconfig.ts`:
- Around line 97-401: The execute handler in guildconfig.ts has grown too large
(contains inline logic for subcommands capture, plan, apply/reconcile, status,
cutover) and should be split to keep the file under the 250-line guideline;
refactor by extracting each subcommand block into separate functions (e.g.,
handleCapture, handlePlan, handleApplyOrReconcile, handleStatus, handleCutover)
that accept the same context (interaction, guild) and call existing helpers like
captureGuildAutomationState, guildAutomationService methods,
applyAutomationModules, interactionReply and errorLog, then replace the inline
blocks in execute with a small dispatcher that calls these new handler functions
based on subcommand; keep existing behavior and return values/updates (including
interaction.editReply and guildAutomationService updates) so tests and logic
remain unchanged.
- Around line 203-229: Invert the negated condition for clarity: replace the `if
(!blockedByProtected)` branch with a positive `if (blockedByProtected)` first so
the blocked case calls `guildAutomationService.updateRunStatus` with status
'blocked' and diagnostics, and the else branch performs the apply flow (calling
`applyAutomationModules` with `guild`, `planResult.desired`, `planResult.plan`,
`allowProtected`), merges `applyResult` into `applyDiagnostics` and then calls
`guildAutomationService.updateRunStatus` with status 'completed'; update
references to `planResult.runId`, `applyDiagnostics`, and `applyResult`
accordingly to preserve behavior.
In `@packages/bot/src/functions/moderation/commands/index.ts`:
- Around line 2-3: Update the built-in module imports to use the Node.js "node:"
protocol for ESM consistency: replace the import of path and fileURLToPath from
'path' and 'url' with the namespaced imports using 'node:path' and 'node:url'
(refer to the top-level import statements that currently import path and
fileURLToPath in packages/bot/src/functions/moderation/commands/index.ts).
Ensure both import specifiers are updated so other code using path and
fileURLToPath remains unchanged.
In `@packages/bot/src/utils/guildAutomation/applyPlan.ts`:
- Around line 168-176: Role deletion in the roles loop (the await
role.delete('Lucky guild automation protected-delete apply') call in
applyPlan.ts) lacks error handling and will abort the whole operation on
failure; wrap the await role.delete(...) in a try/catch and handle errors the
same way channel deletion does (catch errors, log them with the same logger or
console, and continue) so a single failed role.delete won't stop the rest of the
automation from applying.
- Around line 223-296: The applyAutomationModules function is too long; refactor
by extracting each module's logic into small handler functions (e.g.,
applyOnboardingModule, applyRolesModule, applyModerationModule,
applyAutoMessagesModule, applyReactionRolesModule, applyCommandAccessModule,
applyParityModule) that return either an applied module name or a skipped
message (string | null or {applied?:string, skipped?:string}), then simplify
applyAutomationModules to iterate over a handlers array and call
shouldApplyModule(plan, '<module>', allowProtected) before invoking each handler
and collecting returned applied/skipped names; keep existing helpers
(manifestOnboardingToDiscordEdit, applyRolesAndChannels,
autoModService.updateSettings, updateModerationSettings, upsertAutoMessage,
applyReactionRoleRules, guildRoleAccessService.replaceRoleGrants) inside the new
handlers so behavior is unchanged.
In `@packages/bot/src/utils/guildAutomation/captureGuildState.ts`:
- Around line 83-189: captureGuildAutomationState is too long; split its
responsibilities into smaller helpers: extract the async parallel fetch block
(the Promise.all that assigns automodSettings, moderationSettings,
welcomeMessage, leaveMessage, reactionRoleMessages, exclusiveRoles, roleGrants,
parity) into a new helper like fetchGuildAutomationData(guildId, botUserId) that
returns a typed object, and move the large return object construction into a
buildGuildManifest(guild, onboarding, fetchedData, roles, channels) helper; keep
captureGuildAutomationState to calling those helpers, computing roles/channels,
and returning the result. Reference the existing symbols
captureGuildAutomationState, captureParity, autoModService.getSettings,
autoMessageService.getWelcomeMessage/getLeaveMessage,
reactionRolesService.listReactionRoleMessages,
roleManagementService.listExclusiveRoles, guildRoleAccessService.listRoleGrants
in the new helpers so locating code is straightforward.
In `@packages/shared/src/services/guildAutomation/manifestSchema.ts`:
- Around line 120-125: The moderation schema currently relaxes structure by
using z.record(z.unknown()) for fields automod and moderationSettings; restore
meaningful validation by replacing those records with a minimal base schema
(e.g., an automodBaseSchema with known properties like enabled and other
expected keys, using .passthrough() to allow extras) and use
automodBaseSchema.optional() and a similar base for moderationSettings, or if
the loose shape is intentional add a concise code comment explaining why
structural validation was removed; update the object in manifestSchema.ts (the
moderation object containing automod and moderationSettings) to reference these
base schemas to preserve runtime validation and compile-time typing.
In `@packages/shared/src/services/guildAutomation/service.ts`:
- Around line 186-196: Extract the nested ternary that computes actual into a
small helper (e.g., resolveActualState or getActualState) that accepts options
and manifestRow and returns either
guildAutomationManifestSchema.parse(options.actualState),
toManifestDocument(manifestRow.lastCapturedState), or null; replace the inline
expression in service.ts with a call to that helper and keep the existing null
check/throw behavior unchanged so the error message remains the same.
- Around line 208-215: Replace the nested ternary that assigns severity with a
small named helper: implement a function (e.g., getSeverityFromCount or
calculateSeverity) that accepts count and returns the union type 'none' | 'low'
| 'medium' | 'high' using the same thresholds (0 -> 'none', count < 3 -> 'low',
count < 8 -> 'medium', else 'high'), then replace the inline ternary expression
that sets the severity variable with a call to that helper; ensure the helper is
exported or kept local to the module as appropriate and used everywhere this
logic may be needed for reusability and clarity.
In `@packages/shared/src/services/guildAutomation/types.ts`:
- Around line 196-202: The fields latestRun.type, latestRun.status,
drifts[].module and drifts[].severity were widened to plain string losing
exhaustiveness checks; revert those specific fields in
packages/shared/src/services/guildAutomation/types.ts to the corresponding union
types (AutomationRunType, AutomationRunStatus, AutomationModule, DriftSeverity)
so TypeScript preserves literal unions for consumers, and if DB string values
may be invalid add runtime validation inside the getStatus() helper (validate
and map/throw/fallback to a safe sentinel like "UNKNOWN") to ensure only
expected enum literals are used at runtime.
- Around line 76-86: The types for automod and moderationSettings were relaxed
to include permissive index signatures which removes compile-time safety; adjust
types.ts by either restoring the original specific update interfaces
(AutoModSettingsUpdate, ModerationSettingsUpdate) for known fields and adding an
explicit fallback type (e.g., Record<string, unknown> or a named
ExtraAutomodFields/ExtraModerationFields) for extension, and/or add runtime
validation helpers that consumers can call to validate unknown keys before
usage; update the automod and moderationSettings declarations to reference these
stricter update types (or the combined specific+fallback types) so callers get
type safety for known properties while still allowing extensibility at runtime.
In `@prisma/schema.prisma`:
- Around line 137-147: The model defines guildId as unique, which already
creates a PostgreSQL index, so the extra index declaration @@index([guildId]) in
the GuildAutomationManifest model should be removed to avoid duplicate indexes;
locate the GuildAutomationManifest model in schema.prisma and delete the
@@index([guildId]) line (leave guildId String `@unique`, version, manifest, etc.
unchanged), then regenerate the migration to reflect the removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 257b03a3-70e6-436a-9d7c-0b3cf61bd50e
📒 Files selected for processing (31)
CHANGELOG.mdREADME.mdpackages/backend/src/constants/automodTemplates.tspackages/backend/src/constants/guildAutomationPresets.tspackages/backend/src/routes/guildAutomation.tspackages/backend/src/routes/guilds.tspackages/backend/src/routes/index.tspackages/backend/src/routes/management.tspackages/backend/src/schemas/management.tspackages/backend/src/services/GuildService.tspackages/backend/tests/integration/routes/guildAutomation.test.tspackages/backend/tests/integration/routes/guilds.test.tspackages/backend/tests/integration/routes/management.test.tspackages/backend/tests/unit/routes/index.test.tspackages/bot/src/functions/automod/commands/index.tspackages/bot/src/functions/management/commands/guildconfig.spec.tspackages/bot/src/functions/management/commands/guildconfig.tspackages/bot/src/functions/management/commands/index.tspackages/bot/src/functions/moderation/commands/index.tspackages/bot/src/register.spec.tspackages/bot/src/register.tspackages/bot/src/utils/guildAutomation/applyPlan.tspackages/bot/src/utils/guildAutomation/captureGuildState.tspackages/bot/src/utils/guildAutomation/diff.spec.tspackages/bot/src/utils/guildAutomation/manifestSchema.spec.tspackages/shared/src/services/guildAutomation/diff.tspackages/shared/src/services/guildAutomation/manifestSchema.tspackages/shared/src/services/guildAutomation/service.tspackages/shared/src/services/guildAutomation/types.tsprisma/migrations/20260311193000_add_guild_automation_manifest/migration.sqlprisma/schema.prisma
💤 Files with no reviewable changes (3)
- packages/bot/src/utils/guildAutomation/diff.spec.ts
- packages/backend/tests/unit/routes/index.test.ts
- packages/bot/src/utils/guildAutomation/manifestSchema.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/bot/src/utils/guildAutomation/applyPlan.ts (1)
280-291:⚠️ Potential issue | 🟠 MajorRemove
as nevermoderation casts and validate before service calls.Lines 283 and 290 bypass type safety at a service boundary. This can pass malformed moderation payloads into shared services.
As per coding guidelines: "Always validate and sanitize external data (HTTP, DB) at the boundary using type guards or schema validators".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.ts` around lines 280 - 291, The code is currently using unsafe casts ("as never") when passing desired.moderation.automod and desired.moderation.moderationSettings into service boundaries; remove those casts and validate/sanitize the payloads before calling autoModService.updateSettings(guild.id, ...) and updateModerationSettings(guild.id, ...). Implement or call the existing type guard/schema validator for ModerationAutomod and ModerationSettings (or add simple runtime checks) to ensure the objects conform, and if validation fails, log or return early rather than calling the service with malformed data.
🧹 Nitpick comments (12)
packages/bot/src/functions/automod/commands/index.spec.ts (1)
5-14: Same source-string testing concern as other loader specs.This test has the same brittle pattern as the moderation and management loader specs. Testing raw source text doesn't validate actual loader behavior.
If source-string validation is intentional (e.g., enforcing code generation consistency), consider extracting a shared test utility rather than duplicating the same test structure across three files.
♻️ Shared test utility approach
// test-utils/loaderSourceValidator.ts export const validateLoaderSource = ( category: string, sourcePath: string ) => { const source = fs.readFileSync(sourcePath, 'utf8') expect(source).not.toContain('excludePatterns') expect(source).toContain(`category: '${category}'`) expect(source).toContain("import path from 'node:path'") expect(source).toContain("import { fileURLToPath } from 'node:url'") }As per coding guidelines: "Test behavior, not implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/automod/commands/index.spec.ts` around lines 5 - 14, The test describe('automod command loader source') in index.spec.ts is brittle because it asserts raw source text; replace the duplicated string checks with a shared utility (e.g., create test-utils/loaderSourceValidator.ts exporting validateLoaderSource(category, sourcePath)) and call validateLoaderSource('automod', path.join(__dirname, 'index.ts')) from this spec, or better, rewrite the spec to test actual loader behavior by importing the loader module and asserting its runtime properties (category === 'automod' and no excludePatterns) instead of reading the file contents; update other similar specs to reuse validateLoaderSource if you keep the source-string approach.packages/bot/src/functions/moderation/commands/index.spec.ts (2)
5-14: Testing source code strings is brittle and tests implementation rather than behavior.This test reads the raw source file and asserts on string patterns. Per coding guidelines, tests should verify behavior, not implementation details. String-based assertions break on:
- Formatting changes (quotes, spacing)
- Import reordering
- Comments or minor refactors
Consider testing actual loader behavior instead—verify the exported module has the expected
categoryproperty, correct command count, or proper structure.♻️ Behavioral test approach
-import { describe, expect, it } from '@jest/globals' -import fs from 'node:fs' -import path from 'node:path' - -describe('moderation command loader source', () => { - it('uses moderation category with centralized loader defaults', () => { - const sourcePath = path.join(__dirname, 'index.ts') - const source = fs.readFileSync(sourcePath, 'utf8') - - expect(source).not.toContain('excludePatterns') - expect(source).toContain("category: 'moderation'") - expect(source).toContain("import path from 'node:path'") - expect(source).toContain("import { fileURLToPath } from 'node:url'") - }) -}) +import { describe, expect, it } from '@jest/globals' +import loader from './index.js' + +describe('moderation command loader', () => { + it('exports commands with moderation category', async () => { + const commands = await loader() + expect(commands.length).toBeGreaterThan(0) + commands.forEach(cmd => { + expect(cmd.category).toBe('moderation') + }) + }) +})As per coding guidelines: "Test behavior, not implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/moderation/commands/index.spec.ts` around lines 5 - 14, Replace brittle string-based assertions in the test that reads the source file with a behavior-driven test that imports/loads the module under test (the exported loader from the index.ts file) and asserts on its runtime shape: require or import the module and check that the exported loader object has category === 'moderation', that its commands (or load function) returns/contains the expected number of commands or expected structure, and that it does not expose an excludePatterns property; update the test names accordingly (the existing it() description can remain) and remove assertions that inspect raw source via source/sourcePath.
7-7: Replace__dirnamewith the ESM-equivalent pattern.
__dirnameis a CommonJS global not available in native ESM. UsefileURLToPath(import.meta.url)instead to align with the project's ESM-only guidelines and the pattern already used in other command files.Example from similar test file
Other command files like
packages/bot/src/functions/automod/commands/index.tsuse:import { fileURLToPath } from 'node:url' // ... const dirName = path.dirname(fileURLToPath(import.meta.url))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/moderation/commands/index.spec.ts` at line 7, The test uses the CommonJS __dirname global when building sourcePath; replace that with the ESM pattern by importing fileURLToPath from 'node:url' and computing dirName = path.dirname(fileURLToPath(import.meta.url)), then build sourcePath using that dirName (i.e. update the symbol sourcePath and its construction to use dirName instead of __dirname); ensure the new import and dirName calculation are added at the top of the file near other imports and referenced where sourcePath is defined.packages/bot/src/functions/management/commands/index.spec.ts (1)
5-13: Consistent with other loader specs, but same brittleness concerns apply.The additions (lines 11-13) extend the source-string validation pattern. While this maintains consistency with the automod and moderation specs, the same concerns about testing implementation rather than behavior apply here.
If this pattern is intentional for enforcing generated code consistency, adding a comment explaining the rationale would help future maintainers understand why source-string testing is used instead of behavioral tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/management/commands/index.spec.ts` around lines 5 - 13, The spec index.spec.ts now asserts source-string fragments (expect(...).toContain for "category: 'management'", "import path from 'node:path'", and "import { fileURLToPath } from 'node:url'") which is intentionally brittle but used for generated-code consistency; add a short comment above the test describing that these source-string assertions are intentional to enforce generated loader output consistency (matching automod/moderation specs) and note the tradeoff vs behavioral testing so future maintainers understand why we test the source string rather than behavior.packages/bot/src/utils/guildAutomation/captureGuildState.spec.ts (3)
167-167: Consider a typed partial mock to avoidas any.Per coding guidelines,
anyshould be avoided. A typed approach usingPartial<Guild>or a custom mock interface could provide better type safety while still allowing the mock to omit unneeded properties.Example typed mock interface
// At top of file type MockGuild = Pick<Guild, 'id' | 'name' | 'members' | 'roles' | 'channels'> & { fetchOnboarding: jest.Mock } // In test const result = await captureGuildAutomationState( guild as unknown as Guild, 'bot-self' )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/captureGuildState.spec.ts` at line 167, Replace the unsafe "as any" cast when calling captureGuildAutomationState by creating a typed partial/mock for the Guild used in the test (e.g., use Partial<Guild> or a narrow MockGuild that picks required properties like id, name, members, roles, channels and stubs methods such as fetchOnboarding) and cast the test guild to that type (or to unknown then Guild) so the call becomes type-safe; update the test's guild declaration and the invocation of captureGuildAutomationState to use this typed mock instead of "as any".
37-127: Consider extracting sub-factories for readability (optional).The
createGuildhelper exceeds the 50-line guideline at ~91 lines. While comprehensive test fixtures often require more setup, extractingcreateChannels(),createRoles(), andcreateMembers()sub-helpers could improve maintainability and make individual channel/role configurations reusable across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/captureGuildState.spec.ts` around lines 37 - 127, The createGuild test helper is long and should be split into sub-factories to improve readability and reuse; refactor createGuild by extracting createChannels(), createRoles(), and createMembers() functions (or similarly named helpers) that return the respective objects (members.cache, roles.cache, channels.cache) and then compose them inside createGuild (preserve overrides parameter and existing keys like members, roles, channels, fetchOnboarding, and id); update tests to import/call these sub-factories or keep createGuild calling them so existing tests keep the same shape.
228-235: Prefer using an Error object for the rejection mock.The test correctly verifies rethrow behavior, but mocking with a string (
'onboarding-fail') doesn't reflect real-world error scenarios whereErrorobjects are thrown. Using anErrorinstance would make the test more realistic and align with the coding guideline to "never throw strings."Suggested change
it('rethrows unexpected onboarding fetch errors', async () => { const guild = createGuild() - ;(guild.fetchOnboarding as jest.Mock).mockRejectedValue('onboarding-fail') + const unexpectedError = new Error('onboarding-fail') + ;(guild.fetchOnboarding as jest.Mock).mockRejectedValue(unexpectedError) await expect( captureGuildAutomationState(guild as any, 'bot-self'), - ).rejects.toBe('onboarding-fail') + ).rejects.toBe(unexpectedError) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/captureGuildState.spec.ts` around lines 228 - 235, The test currently mocks fetchOnboarding rejection with a string literal which is unrealistic; update the mock in captureGuildAutomationState.spec (the test block that sets (guild.fetchOnboarding as jest.Mock).mockRejectedValue('onboarding-fail')) to reject with an Error object instead (e.g., mockRejectedValue(new Error('onboarding-fail'))) so the test simulates a real thrown Error and still asserts that captureGuildAutomationState rethrows it.packages/shared/src/services/guildAutomation/types.ts (2)
76-86: Index signatures bypass compile-time type checking.The
[key: string]: unknownindex signatures allow arbitrary properties without compiler validation. While runtime validation via Zod (.strict()inautoModSettingsBody) catches invalid requests, type-level enforcement is lost—callers can pass any key without compile errors.If this flexibility is intentional to support dynamic template properties, consider documenting that rationale. Otherwise, define explicit optional fields for known properties.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/types.ts` around lines 76 - 86, The types automod and moderationSettings currently include permissive index signatures ([key: string]: unknown) which bypass compile-time checks; remove those index signatures from the automod and moderationSettings interfaces in types.ts and instead declare explicit optional fields for any known dynamic properties (e.g., add named optional properties for template-related fields) or, if truly needed, replace the signature with a documented Record type alias and comment explaining why dynamic keys are required; also update the corresponding runtime validator autoModSettingsBody (Zod schema) to match the tightened type shape so compile-time and runtime validation stay in sync.
195-205: Use existing union types instead ofstringfor type safety.The properties
type,status,module, andseverityare typed asstringinGuildAutomationStatus, but the corresponding union types (AutomationRunType,AutomationRunStatus,AutomationModule,DriftSeverity) are defined in this same file (lines 11–29). Usingstringloses compile-time exhaustiveness checks and IDE autocomplete. The fact that Prisma stores these as string fields does not prevent the TypeScript interface from using union types—validation happens at the database boundary.♻️ Proposed fix to restore type safety
latestRun: { id: string - type: string - status: string + type: AutomationRunType + status: AutomationRunStatus createdAt: Date } | null drifts: Array<{ - module: string - severity: string + module: AutomationModule + severity: DriftSeverity updatedAt: Date }>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/guildAutomation/types.ts` around lines 195 - 205, In GuildAutomationStatus (fields latestRun and drifts) replace the loose string types with the existing union types defined earlier: use AutomationRunType for latestRun.type, AutomationRunStatus for latestRun.status, AutomationModule for each drifts[].module, and DriftSeverity for each drifts[].severity; keep createdAt/updatedAt as Date. Update the type annotations for latestRun and drifts accordingly so the interface uses AutomationRunType, AutomationRunStatus, AutomationModule, and DriftSeverity for compile-time safety and IDE autocomplete.packages/bot/src/utils/guildAutomation/applyPlan.spec.ts (2)
294-595: Reduceas anyusage in new tests with typed builders/fixtures.Frequent
as anycasts in these new cases weaken compile-time contract checks between test inputs andapplyAutomationModules.As per coding guidelines: "Do not use
anytypes - ESLint enforces this at error level".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.spec.ts` around lines 294 - 595, The tests overuse "as any" which bypasses type checking; update the specs to use typed builders/fixtures instead: change createGuild to return a properly typed Guild (or use a generic createGuild<TGuild>() helper), type the desired payloads to the real DesiredState or AutomationDesired types used by applyAutomationModules, and type buildPlan to return the real Plan type so plan: buildPlan(['roles']) is strongly typed; ensure mocks like roleCreateMock/channelCreateMock keep their Jest types (e.g., jest.MockedFunction) and replace each "as any" around guild, desired, and plan with the correct types so TypeScript/ESLint no-any errors are resolved while keeping test behavior identical.
79-600: Consider splitting this spec by module concern.This file now exceeds the repository size guideline; splitting into focused specs (
roles,moderation,onboarding, etc.) will improve maintainability.As per coding guidelines: "Files must not exceed 250 lines and this is enforced".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.spec.ts` around lines 79 - 600, Spec file exceeds the 250-line guideline — split tests by module concern. Create focused spec files (e.g., applyPlan.roles.spec.ts, applyPlan.moderation.spec.ts, applyPlan.onboarding.spec.ts, applyPlan.automessages.spec.ts, applyPlan.reactionroles.spec.ts, applyPlan.commandaccess.spec.ts) and move each related it(...) block into the matching file; keep shared helpers (createGuild, buildPlan) and common mocks (manifestOnboardingToDiscordEditMock, getWelcomeMessageMock, getLeaveMessageMock, createMessageMock, updateMessageMock, updateSettingsMock, updateModerationSettingsMock, listExclusiveRolesMock, removeExclusiveRoleMock, setExclusiveRoleMock, replaceRoleGrantsMock, errorLogMock) in a single test-utils module and import them into each spec; ensure each new spec imports applyAutomationModules and only sets up the beforeEach mocks relevant to that module so files stay under 250 lines and maintain test isolation.packages/bot/src/utils/guildAutomation/applyPlan.ts (1)
256-329: SplitapplyAutomationModulesinto smaller module appliers.This function now combines orchestration plus per-module execution and exceeds the project function-size guideline. Extracting per-module handlers would improve readability and testability.
As per coding guidelines: "Functions must be less than 50 lines with cyclomatic complexity less than 10".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/guildAutomation/applyPlan.ts` around lines 256 - 329, The applyAutomationModules function is too large; extract each module's logic into small handlers (e.g., applyOnboardingModule, applyRolesModule, applyModerationModule, applyAutoMessagesModule, applyReactionRolesModule, applyCommandAccessModule, applyParityModule) that accept the same context (guild, desired, allowProtected) and return an object/tuple indicating applied and skipped module names; keep shouldApplyModule checks in the orchestrator or inside each handler, preserve all existing calls (manifestOnboardingToDiscordEdit, guild.editOnboarding, applyRolesAndChannels, autoModService.updateSettings, updateModerationSettings, upsertAutoMessage, applyReactionRoleRules, guildRoleAccessService.replaceRoleGrants) and await semantics, aggregate their returned appliedModules/skippedModules into the final ApplyResult, and ensure types/signature of applyAutomationModules and the ApplyResult remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bot/src/functions/management/commands/guildconfig.ts`:
- Around line 391-394: The current reply exposes raw error.message to users;
update the error handling in the guild automation command to use
createUserFriendlyError from `@lucky/shared/general` to produce a sanitized
user-facing message and send it with interactionReply, while logging the full
error details to your existing logger/structured logs (do not include raw
error.message in the reply). Locate the reply code that builds content using
error instanceof Error ? `❌ ${error.message}` and replace that branch to call
createUserFriendlyError(error) for the message and then call
interactionReply(...) to send it; ensure the original error is forwarded to your
logger for diagnostics.
- Around line 203-226: If applyAutomationModules or the subsequent update call
throws, the run can remain non-terminal; wrap the apply + successful update
block (the call to applyAutomationModules and the
guildAutomationService.updateRunStatus that sets status: 'completed' or
'blocked') with a try/catch that, on any exception, calls
guildAutomationService.updateRunStatus({ runId: planResult.runId, status:
'failed', diagnostics: {...applyDiagnostics, error: <stringified error> }}) (and
swallow/log any errors from that update to avoid masking the original error)
before rethrowing or returning the original error; ensure you reference
applyAutomationModules, guildAutomationService.updateRunStatus and
planResult.runId when making the change.
- Around line 327-345: Replace the non-deterministic cache lookup
guild.members.cache.get(bot.id) with an awaited guild.members.fetch(bot.id) in
the loop so external bots are always resolved; wrap the fetch in a try/catch
that skips the bot when fetch throws a 404 (member not found) and rethrows or
logs other errors, only proceed to compute removableRoleIds and call
member.roles.remove(removableRoleIds, 'Lucky cutover removed legacy bot
permissions') when the fetch succeeds, and only increment cleanedBots after a
successful removal.
- Around line 110-381: The execute function is too large/complex—extract each
subcommand block (capture, plan, apply, reconcile, status, cutover) into
dedicated handler functions (e.g. handleCapture, handlePlan,
handleApplyOrReconcile, handleStatus, handleCutover) and make execute a thin
dispatcher that calls these handlers based on subcommand; move logic that calls
captureGuildAutomationState, guildAutomationService.createPlan,
applyAutomationModules, guildAutomationService.updateRunStatus,
guildAutomationService.getStatus/listRuns, and guildAutomationService.runCutover
into their corresponding handlers and return the reply logic from there. Also
replace the local interactionReply import with the package import from
`@lucky/shared/general` and wrap any user-facing errors using
createUserFriendlyError instead of exposing error.message directly so handlers
throw or return friendly errors for the caller to surface. Ensure unique symbols
referenced: execute, captureGuildAutomationState, guildAutomationService
(methods: recordCapture, createPlan, updateRunStatus, getStatus, listRuns,
runCutover, getManifest), applyAutomationModules, and summaryEmbed are
moved/used inside the new handler functions.
In `@packages/bot/src/utils/guildAutomation/applyPlan.ts`:
- Around line 203-220: The catch block that currently does "throw error" should
normalize any non-Error throws into a proper Error and rethrow with the original
value as the cause; update the catch in applyPlan (around the channel.delete
call and shouldIgnoreProtectedDeleteError usage) so that if the error is not
handled by shouldIgnoreProtectedDeleteError you construct and throw a new Error
with a clear message like "Failed to delete channel during guild automation
apply" and attach the original error as the cause (using Error's cause option or
wrapping it if the environment lacks cause), preserving guild.id and channel.id
context in the message or data passed to the new Error.
---
Duplicate comments:
In `@packages/bot/src/utils/guildAutomation/applyPlan.ts`:
- Around line 280-291: The code is currently using unsafe casts ("as never")
when passing desired.moderation.automod and
desired.moderation.moderationSettings into service boundaries; remove those
casts and validate/sanitize the payloads before calling
autoModService.updateSettings(guild.id, ...) and
updateModerationSettings(guild.id, ...). Implement or call the existing type
guard/schema validator for ModerationAutomod and ModerationSettings (or add
simple runtime checks) to ensure the objects conform, and if validation fails,
log or return early rather than calling the service with malformed data.
---
Nitpick comments:
In `@packages/bot/src/functions/automod/commands/index.spec.ts`:
- Around line 5-14: The test describe('automod command loader source') in
index.spec.ts is brittle because it asserts raw source text; replace the
duplicated string checks with a shared utility (e.g., create
test-utils/loaderSourceValidator.ts exporting validateLoaderSource(category,
sourcePath)) and call validateLoaderSource('automod', path.join(__dirname,
'index.ts')) from this spec, or better, rewrite the spec to test actual loader
behavior by importing the loader module and asserting its runtime properties
(category === 'automod' and no excludePatterns) instead of reading the file
contents; update other similar specs to reuse validateLoaderSource if you keep
the source-string approach.
In `@packages/bot/src/functions/management/commands/index.spec.ts`:
- Around line 5-13: The spec index.spec.ts now asserts source-string fragments
(expect(...).toContain for "category: 'management'", "import path from
'node:path'", and "import { fileURLToPath } from 'node:url'") which is
intentionally brittle but used for generated-code consistency; add a short
comment above the test describing that these source-string assertions are
intentional to enforce generated loader output consistency (matching
automod/moderation specs) and note the tradeoff vs behavioral testing so future
maintainers understand why we test the source string rather than behavior.
In `@packages/bot/src/functions/moderation/commands/index.spec.ts`:
- Around line 5-14: Replace brittle string-based assertions in the test that
reads the source file with a behavior-driven test that imports/loads the module
under test (the exported loader from the index.ts file) and asserts on its
runtime shape: require or import the module and check that the exported loader
object has category === 'moderation', that its commands (or load function)
returns/contains the expected number of commands or expected structure, and that
it does not expose an excludePatterns property; update the test names
accordingly (the existing it() description can remain) and remove assertions
that inspect raw source via source/sourcePath.
- Line 7: The test uses the CommonJS __dirname global when building sourcePath;
replace that with the ESM pattern by importing fileURLToPath from 'node:url' and
computing dirName = path.dirname(fileURLToPath(import.meta.url)), then build
sourcePath using that dirName (i.e. update the symbol sourcePath and its
construction to use dirName instead of __dirname); ensure the new import and
dirName calculation are added at the top of the file near other imports and
referenced where sourcePath is defined.
In `@packages/bot/src/utils/guildAutomation/applyPlan.spec.ts`:
- Around line 294-595: The tests overuse "as any" which bypasses type checking;
update the specs to use typed builders/fixtures instead: change createGuild to
return a properly typed Guild (or use a generic createGuild<TGuild>() helper),
type the desired payloads to the real DesiredState or AutomationDesired types
used by applyAutomationModules, and type buildPlan to return the real Plan type
so plan: buildPlan(['roles']) is strongly typed; ensure mocks like
roleCreateMock/channelCreateMock keep their Jest types (e.g.,
jest.MockedFunction) and replace each "as any" around guild, desired, and plan
with the correct types so TypeScript/ESLint no-any errors are resolved while
keeping test behavior identical.
- Around line 79-600: Spec file exceeds the 250-line guideline — split tests by
module concern. Create focused spec files (e.g., applyPlan.roles.spec.ts,
applyPlan.moderation.spec.ts, applyPlan.onboarding.spec.ts,
applyPlan.automessages.spec.ts, applyPlan.reactionroles.spec.ts,
applyPlan.commandaccess.spec.ts) and move each related it(...) block into the
matching file; keep shared helpers (createGuild, buildPlan) and common mocks
(manifestOnboardingToDiscordEditMock, getWelcomeMessageMock,
getLeaveMessageMock, createMessageMock, updateMessageMock, updateSettingsMock,
updateModerationSettingsMock, listExclusiveRolesMock, removeExclusiveRoleMock,
setExclusiveRoleMock, replaceRoleGrantsMock, errorLogMock) in a single
test-utils module and import them into each spec; ensure each new spec imports
applyAutomationModules and only sets up the beforeEach mocks relevant to that
module so files stay under 250 lines and maintain test isolation.
In `@packages/bot/src/utils/guildAutomation/applyPlan.ts`:
- Around line 256-329: The applyAutomationModules function is too large; extract
each module's logic into small handlers (e.g., applyOnboardingModule,
applyRolesModule, applyModerationModule, applyAutoMessagesModule,
applyReactionRolesModule, applyCommandAccessModule, applyParityModule) that
accept the same context (guild, desired, allowProtected) and return an
object/tuple indicating applied and skipped module names; keep shouldApplyModule
checks in the orchestrator or inside each handler, preserve all existing calls
(manifestOnboardingToDiscordEdit, guild.editOnboarding, applyRolesAndChannels,
autoModService.updateSettings, updateModerationSettings, upsertAutoMessage,
applyReactionRoleRules, guildRoleAccessService.replaceRoleGrants) and await
semantics, aggregate their returned appliedModules/skippedModules into the final
ApplyResult, and ensure types/signature of applyAutomationModules and the
ApplyResult remain unchanged.
In `@packages/bot/src/utils/guildAutomation/captureGuildState.spec.ts`:
- Line 167: Replace the unsafe "as any" cast when calling
captureGuildAutomationState by creating a typed partial/mock for the Guild used
in the test (e.g., use Partial<Guild> or a narrow MockGuild that picks required
properties like id, name, members, roles, channels and stubs methods such as
fetchOnboarding) and cast the test guild to that type (or to unknown then Guild)
so the call becomes type-safe; update the test's guild declaration and the
invocation of captureGuildAutomationState to use this typed mock instead of "as
any".
- Around line 37-127: The createGuild test helper is long and should be split
into sub-factories to improve readability and reuse; refactor createGuild by
extracting createChannels(), createRoles(), and createMembers() functions (or
similarly named helpers) that return the respective objects (members.cache,
roles.cache, channels.cache) and then compose them inside createGuild (preserve
overrides parameter and existing keys like members, roles, channels,
fetchOnboarding, and id); update tests to import/call these sub-factories or
keep createGuild calling them so existing tests keep the same shape.
- Around line 228-235: The test currently mocks fetchOnboarding rejection with a
string literal which is unrealistic; update the mock in
captureGuildAutomationState.spec (the test block that sets
(guild.fetchOnboarding as jest.Mock).mockRejectedValue('onboarding-fail')) to
reject with an Error object instead (e.g., mockRejectedValue(new
Error('onboarding-fail'))) so the test simulates a real thrown Error and still
asserts that captureGuildAutomationState rethrows it.
In `@packages/shared/src/services/guildAutomation/types.ts`:
- Around line 76-86: The types automod and moderationSettings currently include
permissive index signatures ([key: string]: unknown) which bypass compile-time
checks; remove those index signatures from the automod and moderationSettings
interfaces in types.ts and instead declare explicit optional fields for any
known dynamic properties (e.g., add named optional properties for
template-related fields) or, if truly needed, replace the signature with a
documented Record type alias and comment explaining why dynamic keys are
required; also update the corresponding runtime validator autoModSettingsBody
(Zod schema) to match the tightened type shape so compile-time and runtime
validation stay in sync.
- Around line 195-205: In GuildAutomationStatus (fields latestRun and drifts)
replace the loose string types with the existing union types defined earlier:
use AutomationRunType for latestRun.type, AutomationRunStatus for
latestRun.status, AutomationModule for each drifts[].module, and DriftSeverity
for each drifts[].severity; keep createdAt/updatedAt as Date. Update the type
annotations for latestRun and drifts accordingly so the interface uses
AutomationRunType, AutomationRunStatus, AutomationModule, and DriftSeverity for
compile-time safety and IDE autocomplete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea81855a-c2e1-4cb4-9310-c172871eb97c
📒 Files selected for processing (14)
packages/backend/tests/integration/routes/guildAutomation.test.tspackages/bot/src/functions/automod/commands/index.spec.tspackages/bot/src/functions/automod/commands/index.tspackages/bot/src/functions/management/commands/guildconfig.spec.tspackages/bot/src/functions/management/commands/guildconfig.tspackages/bot/src/functions/management/commands/index.spec.tspackages/bot/src/functions/management/commands/index.tspackages/bot/src/functions/moderation/commands/index.spec.tspackages/bot/src/functions/moderation/commands/index.tspackages/bot/src/utils/guildAutomation/applyPlan.spec.tspackages/bot/src/utils/guildAutomation/applyPlan.tspackages/bot/src/utils/guildAutomation/captureGuildState.spec.tspackages/bot/src/utils/guildAutomation/captureGuildState.tspackages/shared/src/services/guildAutomation/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bot/src/functions/automod/commands/index.ts
- packages/bot/src/functions/management/commands/index.ts
- packages/bot/src/functions/moderation/commands/index.ts
LucasSantana-Dev
enabled auto-merge (squash)
March 13, 2026 02:59
|
|
1 task done
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
* feat(automation): add guild automation control-plane routes/services/command * test(db): add automation coverage and migration * docs: update README and CHANGELOG for automation control-plane * fix(automation): align moderation manifest typing with execution flow * fix(automation): restore protected deletion and onboarding error semantics * test(automation): raise sonar coverage for control-plane loaders * test(automation): expand apply-plan coverage branches * test(automation): cover capture guild state branches for sonar * test(automation): cover remaining apply and cutover branches * refactor(automation): drop dashboard guild-channel slice from track A
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Track A of root-delta packaging: automation control-plane only (backend/shared/bot/prisma).
Included
/guildconfigcommand + management/moderation/automod command registration pathguild_automation_manifests,guild_automation_runs,guild_automation_drifts)Verification
npm run test --workspace=packages/backend -- tests/integration/routes/guildAutomation.test.ts tests/integration/routes/guilds.test.tsnpm run test --workspace=packages/bot -- src/functions/management/commands/guildconfig.spec.ts src/register.spec.tsnpm run type:checknpm run lintSummary by CodeRabbit
New Features
Improvements