feat: ISSUE-401 — migrate manufacturing config to per-guild guild_configs#408
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new per-guild configuration persistence layer (guild_configs) and starts migrating existing runtime behavior (verification roles + nomination digest scheduling) from environment-based configuration to database-backed guild config.
Changes:
- Add
guild_configstable + repository/service layer, plus a startup seeder that initializes missing rows from current env vars. - Refactor nomination digest into a per-guild scheduler (
scheduleNominationDigests/rescheduleGuildDigest) driven byGuildConfig. - Refactor verification role assignment + default role creation to use per-guild role names from
GuildConfig, and update tests accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/role.services.ts | Role assignment + role bootstrapping now takes role names from GuildConfig instead of env-derived constants. |
| src/services/tests/role.services.test.ts | Updates tests for new role-service signatures and guild-config driven defaults. |
| src/jobs/discord/nomination-digest.job.ts | Replaces single global digest job with per-guild scheduling backed by guild_configs. |
| src/jobs/discord/tests/nomination-digest.job.test.ts | Adds coverage for per-guild scheduling and rescheduling behavior. |
| src/interactions/verifyButton.ts | Loads GuildConfig per interaction and passes configured verified role name into role service. |
| src/interactions/tests/verifyButton.test.ts | Mocks getGuildConfigOrNull and adjusts interaction stubs for guild id. |
| src/index.ts | Seeds guild configs on startup; schedules per-guild digest tasks; role setup now fetches per-guild config. |
| src/domain/guild-config/guild-config.service.ts | Thin service wrapper exposing config fetch helpers + per-guild feature flag helper. |
| src/domain/guild-config/guild-config.seeder.ts | Seeds missing guild_configs rows from env values at startup. |
| src/domain/guild-config/guild-config.repository.ts | Implements DB access for guild configs (get, getAll, upsert w/ partial patch support). |
| src/domain/guild-config/tests/guild-config.service.test.ts | Unit tests for service wrapper and feature-flag helper. |
| src/domain/guild-config/tests/guild-config.seeder.test.ts | Unit tests for env → patch mapping and seeding behavior. |
| src/domain/guild-config/tests/guild-config.repository.test.ts | Unit tests for repository SQL construction/mapping. |
| src/domain/guild-config/tests/guild-config.repository.integration.test.ts | Integration tests verifying DB defaults + partial patch semantics. |
| src/config/roles.config.ts | Removes exports for verified role name / required roles; leaves temp/potential env role name resolution. |
| src/config/nomination-digest.config.ts | Removes env-based digest config shape/validation; retains only the bot-level enable flag. |
| src/config/tests/nomination-digest.config.test.ts | Updates tests to only cover the enable flag. |
| src/tests/index.startup-readonly.test.ts | Updates startup wiring tests for per-guild digest scheduling and guild-config seeding mocks. |
| package.json | Adjusts Jest scripts to exclude integration tests from default runs and adds test:integration. |
| migrations/1776961242514_add-guild-configs.cjs | Adds the new guild_configs table with defaults for multiple feature areas. |
| CLAUDE.md | Adds expanded tool-specific contribution/workflow instructions. |
Comments suppressed due to low confidence (2)
CLAUDE.md:16
- This file adds tool/vendor-specific instructions and branding ("Claude Code") in a repo that already has AI_RULES.md. AI_RULES.md prohibits adding AI/vendor branding in project metadata and advises not to modify primary human-authored instruction files for AI-specific preferences unless explicitly requested. Consider removing this addition or moving any needed guidance into AI_RULES.md in a tool-neutral form.
# Claude Code instructions for station-bot
## Implementation Protocol
Before writing any code for a ticket:
1. **Read first.** Read every file listed in the issue's Technical Elaboration, plus any adjacent files that establish the patterns the new code must follow (e.g. a sibling repository, a similar command handler, the config module for the same feature area). Do not write a single line until this step is done.
2. **Identify existing patterns.** Note the naming conventions, function signature shapes, error-handling style, and module structure already in use. New code must match these — do not introduce a new style unless the existing one is demonstrably wrong, and flag it explicitly if so.
3. **State design decisions before implementing.** For any non-trivial choice (function signature, error boundary, abstraction shape), briefly describe the decision and the pattern being followed. This surfaces misalignments before the code is written, not during review.
4. **Flag uncertainty rather than guess.** If the spec is ambiguous or a pattern isn't clear from the existing code, surface the question before writing. Do not silently pick an approach and hope it matches.
5. **Self-review before marking done.** After implementation, re-read every changed file from the perspective of a senior principal engineer. Check for: naming clarity, SRP, unnecessary abstraction, missing or overly broad error handling, test coverage gaps, and consistency with the patterns identified in step 2. Surface any concerns explicitly before opening the PR.
## Commits
- Never include `Co-Authored-By` trailers or any AI attribution in commit messages.
- Before committing any code, always run `npm run quality` locally and confirm it passes.
- Before committing, perform an objective self-review of all changed code from the perspective of a senior principal engineer: check for SOLID principles, single responsibility, clean abstractions, naming clarity, test coverage, and any code smells or over-engineering. Surface any concerns before the code is committed.
src/index.ts:21
- PR description says manufacturing env config +
getManufacturingConfig()were removed and keepalive renamed/rewritten, butsrc/index.tsstill imports/usesvalidateManufacturingConfigandgetManufacturingConfig, and still importsscheduleCreateOrderKeepAlive. Either update the PR description to match the actual changes, or include the remaining manufacturing migration changes in this PR.
import { scheduleCreateOrderKeepAlive } from './jobs/discord/manufacturing-keepalive.job.js';
import { scheduleNominationDigests } from './jobs/discord/nomination-digest.job.js';
import { addMissingDefaultRoles } from './services/role.services.js';
import { getLogger } from './utils/logger.js';
import { isReadOnlyMode, isVerificationEnabled, isPurgeJobsEnabled } from './config/runtime-flags.js';
import { isNominationDigestEnabled } from './config/nomination-digest.config.js';
import {
validateManufacturingConfig,
isManufacturingEnabled,
getManufacturingConfig,
} from './config/manufacturing.config.js';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7523d83 to
ace0831
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/commands/order-submit.command.ts:255
- The rate-limit slot reservation is no longer actually happening "before any awaits": this function awaits the DB-backed guild config lookup earlier, which reintroduces the race the reservation was meant to prevent (two concurrent invocations can both pass rate-limit checks before either reserves). Consider reserving the slot synchronously before any awaited work (then re-check limits after loading config and roll back via releaseSlot on rejection), or otherwise rework the rate-limit bookkeeping so it remains concurrency-safe.
// Reserve the slot before any awaits. Node.js is single-threaded so this
// push is atomic with respect to other synchronous code; no other invocation
// can interleave until the next await. Reserving here prevents two concurrent
// invocations from both passing the rate-limit check before either records.
// The entry carries interaction.id (unique per Discord interaction) so that
// releaseSlot() can remove exactly this reservation even if two invocations
// for the same user arrive within the same millisecond.
const interactionId = interaction.id;
submitEntries.push({ ts: now, id: interactionId });
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/commands/order-submit.command.ts:246
- The per-guild limits pulled from
guildConfig(manufacturingOrderRateLimitPer5Min,manufacturingOrderRateLimitPerHour,manufacturingOrderLimit) are used directly in comparisons. If any of these are 0 or negative in the DB (e.g., from a bad manual update), the checks will either permanently block ordering (rate limits) or behave unexpectedly. Since the seeder explicitly ignores non-positive ints, it looks like these values are intended to be >= 1—consider clamping/validating here (and falling back to defaults + logging) to make the command resilient to bad stored config.
const {
manufacturingOrderRateLimitPer5Min: orderRateLimitPer5Min,
manufacturingOrderRateLimitPerHour: orderRateLimitPerHour,
manufacturingOrderLimit: orderLimit,
} = guildConfig;
const submitEntries = (orderSubmitTimestamps.get(userId) ?? []).filter(e => e.ts > oneHourAgo);
if (submitEntries.length > 0) {
orderSubmitTimestamps.set(userId, submitEntries);
} else {
orderSubmitTimestamps.delete(userId);
}
const recentSubmits = submitEntries.filter(e => e.ts > fiveMinutesAgo);
if (recentSubmits.length >= orderRateLimitPer5Min) {
src/services/role.services.ts:106
addMissingDefaultRolesnow uses role names fromguildConfigwithout validating them. If any of the role-name fields are accidentally saved as an empty string (possible since the DB columns aretext),guild.roles.create({ name: '' })will throw and the whole role-setup flow fails for the guild. Consider filtering out empty/whitespace-only names (and optionally de-duping) and logging a clear error, or falling back toDEFAULT_ROLE_NAMESwhen any required name is invalid.
const roleNames = guildConfig
? [guildConfig.verifiedRoleName, guildConfig.tempMemberRoleName, guildConfig.potentialApplicantRoleName]
: [...DEFAULT_ROLE_NAMES];
logger.info(`[${guild.name}] Checking required roles: ${roleNames.join(', ')}`);
try {
await guild.roles.fetch();
for (const roleName of roleNames) {
const exists = guild.roles.cache.some((role) => role.name === roleName);
if (!exists) {
await guild.roles.create({
name: roleName,
reason: `Initial setup by ${client.user?.username}`,
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/commands/manufacturing-setup.command.ts:106
interaction.client.channels.fetch(forumChannelId)can fetch a forum channel from another guild if the configured ID is wrong, allowing/manufacturing setupto create the thread in the wrong server. Since this is now per-guild config, fetch the channel via the current guild (e.g.,interaction.guild.channels.fetch) and/or verify the fetched channel’sguildIdmatchesinteraction.guildIdbefore creating threads.
let channel;
try {
channel = await interaction.client.channels.fetch(forumChannelId);
} catch (error) {
logger.error('[manufacturing] Failed to fetch forum channel during setup', { error });
await interaction.editReply({
content: 'Failed to fetch the manufacturing channel. Please check the configuration.',
});
return;
}
if (!channel || channel.type !== ChannelType.GuildForum) {
await interaction.editReply({
content: 'The configured manufacturing channel is not a valid forum channel.',
});
src/jobs/discord/nomination-digest.job.ts:49
- Digest tick fetches the target channel via
client.channels.fetch(channelId), which can resolve a channel from a different guild if the configured ID is wrong. Since digest scheduling is per-guild, consider fetching viaclient.guilds.cache.get(guildId)?.channels.fetch(channelId)(and/or validatechannel.guildId === guildId) to prevent cross-guild posting.
if (!channelId || !roleId) {
logger.warn('[nomination-digest] Guild config missing channel or role; skipping tick', { guildId });
return;
}
const channel = await client.channels.fetch(channelId).catch((error: unknown) => {
logger.warn('[nomination-digest] Failed to fetch digest channel', { guildId, channelId, error });
return null;
});
…d resolution Use guild-scoped channel.fetch in order-submit and manufacturing-keepalive so a misconfigured channel/thread ID cannot resolve a channel from a different guild.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/commands/manufacturing-setup.command.ts:96
- This fetch uses interaction.client.channels.fetch(forumChannelId), which can resolve a channel from a different guild if the stored ID is misconfigured (channel IDs are global). Since this is a guild admin command, it should fetch via interaction.guild.channels.fetch (and fail if the channel isn't in the current guild) to avoid cross-guild side effects.
let channel;
try {
channel = await interaction.client.channels.fetch(forumChannelId);
} catch (error) {
logger.error('[manufacturing] Failed to fetch forum channel during setup', { error });
…al DB re-fetch, scope staff channel to guild - addMissingDefaultRoles: replace [...REQUIRED_ROLES] fallback with [VERIFIED_ROLE_NAME, TEMP_MEMBER_ROLE_NAME, POTENTIAL_APPLICANT_ROLE_NAME] so all three roles are always ensured even when DEFAULT_ROLES env var is partially set - handleOrderItemModal: use session.maxItemsPerOrder directly instead of re-fetching guild config on every item submission (N+1 DB round-trip) - Staff thread creation: scope channel fetch to interaction.guild to prevent cross-guild channel resolution
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/commands/manufacturing-setup.command.ts:95
interaction.client.channels.fetch(forumChannelId)is a global channel fetch and can resolve a channel from a different guild if the stored ID is misconfigured. That would let an admin in one guild create the Create Order thread in another guild where the bot has access. Prefer a guild-scoped fetch (e.g.,interaction.guild.channels.fetch) and also verify the fetched channel’sguildIdmatchesinteraction.guildIdbefore posting.
let channel;
try {
channel = await interaction.client.channels.fetch(forumChannelId);
} catch (error) {
…t-of-bounds crash Zero or negative integers stored in guild_configs (the DB schema has no >0 constraint) produce an invalid index in the rate-limit array access: submitEntries[submitEntries.length - limit - 1] Clamping to Math.max(1, ...) before index math prevents the out-of-bounds access.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/commands/manufacturing-setup.command.ts:100
- This uses interaction.client.channels.fetch(forumChannelId), which can resolve a channel from a different guild if the ID is misconfigured (and the bot is in that other guild). Since this command is guild-scoped admin setup, prefer interaction.guild.channels.fetch(forumChannelId) (or interaction.guildId-based fetch) to guarantee the forum channel belongs to the invoking guild and avoid cross-guild side effects.
let channel;
try {
channel = await interaction.client.channels.fetch(forumChannelId);
} catch (error) {
logger.error('[manufacturing] Failed to fetch forum channel during setup', { error });
await interaction.editReply({
content: 'Failed to fetch the manufacturing channel. Please check the configuration.',
});
return;
Summary
getManufacturingConfig()— the function and its entire config shape are gonemanufacturing.config.tsis now a single export:isManufacturingEnabled()(bot-level kill-switch only)guild_configsviagetGuildConfigOrNullmanufacturing-keepalive.job.tsrewritten as a multi-guild scheduler matching the nomination-digest pattern:scheduleManufacturingKeepalives(client, configs)returns aMap<guildId, ScheduledTask>; re-fetches config on each cron tick so live updates are picked up without a restartorder-actions.command.ts,order-submit.command.ts,manufacturing-setup.command.tsall loadguildConfigper-interactionmanufacturing-setup.command.tssaves the created thread ID back toguild_configsviaupsertGuildConfigafter setupsubmitOrder()now acceptsorderLimitas an explicit parameter instead of reading from envindex.ts: removed config validation block, switchedkeepAliveCronTasktoMap, shares thegetAllGuildConfigs()call between digest and keepalive schedulingTest plan
npm run qualitypasses (lint, typecheck, unit tests)scheduleManufacturingKeepalivescreates one task per guild withmanufacturingEnabled=true, skips othershandleManufacturingSetupCommandsaves thread ID viaupsertGuildConfigafter creationhandleOrderButtonInteractionreads all limits fromguildConfig(rate limit, order limit, max items)manufacturingRoleIdfromguildConfigCloses #401