feat(bait): v3.2.0 — bait channel major refinement#4
Merged
Conversation
Lays the schema and entity surface for the v3.2.0 bait refinement. Behavior
is unchanged in this commit — every new column has a default that matches
current behavior, every rename preserves the in-memory shape. The actual
*use* of these fields lands in subsequent phases.
Migration 1774000011000-BaitChannelV3Schema:
1. `pending_bans` → `pending_actions` (rename + generalize)
- Adds `action` column ('ban'/'softban'/'kick'/'timeout'/'log-only',
default 'ban' for existing rows — only ban-intent rows existed before)
- Adds `attempts`, `lastError`, `deadAt` for the retry queue (Phase 3)
- Drops expired rows pre-rename — grace timers that elapsed during
offline windows can't be resumed anyway
- New index `(deadAt, expiresAt)` for retry-queue scans
2. `idempotency_keys` (new table)
- UNIQUE(guildId, userId, action, dayBucket) — Phase 2 INSERT IGNOREs
here to dedup the mod-vs-bot race + retry double-execution
- `executorId`, `testMode` columns track who/what completed the action
- `expiresAt` drives TTL cleanup (24h default — short enough to keep
the table tiny, long enough to catch out-of-order retries)
3. `bait_channel_logs` — 7 new columns for audit-log correlation
(`discordAuditLogId`, `executorId`, `actionConfirmedAt`), unban tracking
(`unbannedAt`, `unbannedBy`), and DM observability (`dmSent`,
`dmFailureReason`, `logDeliveryFailed`). Phases 3–4 fill these.
4. `bait_channel_configs` — 10 new columns:
- Raid mode (Phase 6): `enableRaidMode`, `raidModeThreshold` (5),
`raidModeWindowSeconds` (60), `raidModeAlertRoleId`,
`currentRaidModeUntil` (sticky lockdown timestamp)
- Cross-channel burst (Phase 7): `crossChannelBurstThreshold` (3),
`crossChannelBurstWindowSeconds` (30)
- Appeal link (Phase 8): `enableAppealLink` (off by default),
`appealLinkBaseUrl`
- Retention (Phase 10): `logRetentionDays` (90, range 30-365)
Entity changes:
- `entities/bait/PendingBan.ts` → `PendingAction.ts` (class rename, new
columns mirror migration)
- `entities/bait/IdempotencyKey.ts` (new)
- `BaitChannelLog.ts` (new columns + `crossChannelBurst?` flag for
detection flags JSON)
- `BaitChannelConfig.ts` (new columns)
Importer rename sweep (no behavior change):
- `src/typeorm/index.ts` — entity registration
- `src/index.ts` — repo construction at boot
- `src/utils/baitChannel/baitChannelManager.ts` — `pendingBanRepo` →
`pendingActionRepo` (private property, internal interface `PendingBan`
kept as in-memory shape; will be refactored in Phase 2)
- `src/utils/database/guildQueries.ts` — `deleteAllGuildData` GDPR purge
now includes `IdempotencyKey` deletion
- `src/commands/handlers/dataExport.ts` — export name `pendingBans` →
`pendingActions` (DTO key — will affect dashboard if it consumes the
export, but the export is admin-side and field is new)
- `src/commands/handlers/dev/devSuiteScaffold.ts` — bait teardown
- `tests/unit/utils/baitChannel/baitChannelManager.test.ts` — repo param
rename in test fixtures
Env vars (.env.example):
- `APPEAL_HMAC_SECRET` documented. Bot will fail fast at boot in Phase 8
if any guild has `enableAppealLink=true` and this is missing — added now
so prod deploys have time to configure before that phase lands.
Cross-repo notes (see .plans/2026-05-16/02-v3.2.0-webapp-and-api-
coordination.md):
- New schema columns are additive — existing webapp/API consumers see
supersets, no breaking change.
- Dashboard pages for raid mode / pending actions / log enrichments
follow in their own PRs after backend lands.
Verification:
- `bun test` — 1171 pass / 0 fail (no regressions; same count as
pre-Phase-1, since this commit only adds schema/entity surface)
- `bun run check` — biome clean across 505 files
- `bun run build` — tsc clean
Refactors `BaitChannelManager.executeAction()` so every moderation
action (ban / softban / kick / timeout / log-only) goes through one
deduplicated REST executor instead of `member.ban()` / `member.kick()`
/ `member.timeout()` directly.
Three guarantees the old path could not provide:
1. **Leave-tolerant.** Bans and unbans now use `guild.bans.create(userId)`
and `guild.bans.remove(userId)` — REST endpoints addressed by user ID
instead of by a (possibly stale) `GuildMember` reference. The ban
succeeds even after Discord.js has evicted the partial, which closes
the failure mode where a user leaves between bait trigger and ban
timer expiry. (Timeout still requires a live member, by Discord API
design — Phase 5 demotes that case to softban + log.)
2. **Idempotent.** Every execution claims an `idempotency_keys` row
keyed on `(guildId, userId, action, dayBucket=today-UTC)` before
touching the Discord API. Duplicate claims short-circuit and return
`status: 'duplicate'`. Closes two races:
- Phase 4's `auditLogEntryCreate` listener will write the same key
from the mod-supersedes-us path, so the bot's pending action is
deduplicated by the mod's manual ban.
- Phase 3's retry queue can re-execute safely — second attempt sees
the existing key and is a no-op.
3. **Audit-reason-aware.** The reason passed to Discord is now the
structured `cogworks:bait score=87 ch=#trap-channel flags=[free,
nitro] msgId=…` form built by `buildAuditReason()`. Server mods
reviewing the Discord audit log can see WHY the bot acted without
needing access to internal logs. Truncated to Discord's 512-char
`X-Audit-Log-Reason` header cap.
New files:
- `src/utils/baitChannel/auditReason.ts` — pure helper. `buildAuditReason`
composes the structured string; `flagsTriggered` extracts the non-zero
flag names from a detection-flags object (order-stable for
determinism).
- `src/utils/baitChannel/banExecutor.ts` — the REST executor.
`executeBanAction(opts, idempotencyRepo)` returns
`{ status: 'executed' | 'duplicate' | 'queued' | 'failed', … }`. The
`queued` status is what Phase 3's retry queue picks up. Discord error
classification:
- 429 + 5xx → queued (retryable)
- 10007 / 10013 / 10026 / 50013 → failed (terminal — won't retry)
- Non-Discord error → queued (network/abort)
Manager changes (`baitChannelManager.ts`):
- New optional constructor arg `idempotencyRepo?:
Repository<IdempotencyKey>` — wired in `src/index.ts` boot path.
Manager continues to function when undefined (test fixtures that
don't pass it land on a `status='failed' / 'idempotency repo
unavailable'` branch; existing tests still pass because they exercise
`logAction` directly, not the executor).
- `executeAction` switch block (~140 LOC of per-action `member.X()`
branches + try/catch wrapping) collapsed into one `executeBanAction`
call plus a result-mapping block. The mapping preserves the existing
`BaitChannelLog.actionTaken` values (`'ban'`, `'logged'`, `'failed'`,
`'test-ban'`, etc.) so the DB row and channel embed stay
backwards-compatible. New value `'superseded'` covers the duplicate
case; `'queued'` covers the retry case.
- Kick path now maps to `softban` when BAN_MEMBERS perm is available
(Discord deletes messages via ban+unban) and stays `kick` only as a
fallback when the bot lacks ban perms — the manager handles the
bot-side purge for both that fallback and the timeout case (which
Discord's ban API can't help with).
- Channel name resolution for the audit reason: uses
`message.channel.name` when the channel exposes it, falls back to
channel ID for thread/DM cases.
Boot wiring (`src/index.ts`):
- Imports `IdempotencyKey` and passes its repo as the 7th constructor
arg. The wiring is positional, matching the existing 6-arg shape.
Tests: no new tests in this commit (existing 1171 tests continue to
pass — they exercise `logAction` directly, which is unchanged). Phase
12 will add per-status assertions on `executeBanAction` and the
`executeAction` result-mapping branches.
Verification:
- `bun test` — 1171 pass / 0 fail
- `bun run check` — biome clean across 507 files
- `bun run build` — tsc clean
What this does NOT yet do (deferred):
- The retry queue (Phase 3) for the `queued` status doesn't exist yet
— currently the manager just records `actionTaken='queued'` and the
log row reflects pending state. Until Phase 3 lands, `queued` rows
effectively become permanent "failed" entries.
- `auditLogEntryCreate` listener (Phase 4) that writes idempotency
keys from the mod-supersedes path.
- `guildMemberRemove` leave-tolerant execution (Phase 5) — currently
the existing grace-period timer still tries to act on stale member
refs; that's fixed in Phase 5 by routing through the same REST
executor.
Backs the `queued` status from Phase 2's ban executor with a real retry
loop, and closes two silent-failure holes in the existing path: DMs and
log channel delivery.
## RetryQueue
New module `src/utils/baitChannel/retryQueue.ts`. Re-uses the
`pending_actions` table — `attempts` distinguishes role:
- `attempts = 0` → grace-period entry (setTimeout-driven, owned by manager)
- `attempts ≥ 1` → retry entry (this queue's territory)
- `deadAt != null` → dead-lettered, ignored
Backoff schedule: 5s → 30s → 5min → dead-letter (3 attempts total).
Tick interval: 15s. Per-tick safety cap: 50 rows.
Two paths into the queue:
1. **Manager-initiated.** When `executeBanAction` returns `status:
'queued'` (Discord 429/5xx, network), the manager calls
`getRetryQueue().enqueue(...)` with the action context. Row is
inserted (or `attempts++` if one already exists for this
`guildId+userId+messageId`).
2. **Orphaned grace row sweep.** When the queue tick finds a row with
`attempts=0` AND `expiresAt < now()`, the in-process grace timer was
lost (bot restart past the grace window). The row is promoted to
`attempts=1` and pushed through the REST executor immediately. The
executor is leave-tolerant, so the action lands whether the user is
still in the guild or not.
Dead-letter behavior: row stays in the DB with `deadAt` set; the Phase
6 pending-actions API surface (planned for the webapp dashboard) will
list these for mod review. Each dead-letter triggers a HIGH-severity
`logError` so the error reporter webhook fires.
Concurrency: re-entrancy guard skips overlapping ticks. Per-row
attempts are bounded by `MAX_ATTEMPTS=3`, so the queue can't loop
forever on a single row.
Wired into the boot path in `src/index.ts`:
- `initRetryQueue({client, pendingActionRepo, idempotencyRepo})` after
the bait manager initializes.
- `stopRetryQueue()` in the graceful-shutdown handler.
## DM proof-of-delivery
`sendDmNotification` returns `DmResult = { sent: boolean,
failureReason?: 'closed'|'no_shared_guild'|'timeout'|'unknown' }`
instead of a bare boolean. Three observability wins:
1. **5s timeout race.** Discord can stall the DM call indefinitely
when the user has us blocked at privacy-settings level. The DM
send is now wrapped in `Promise.race(...timeout(5_000))` so the
action path can't hang on a doomed DM.
2. **Error code classification.** Discord 50007 maps to `closed` (DMs
disabled OR no shared guild — Discord doesn't distinguish).
`dm_timeout` → `timeout`. Anything else → `unknown`. The result is
persisted to `BaitChannelLog.dmFailureReason` so dashboard analytics
can break down DM delivery by cause.
3. **Persisted to log row.** `BaitChannelLog.dmSent` + `dmFailureReason`
are populated via the new optional `extras` arg on `logAction()`.
Previously `dmSent` flowed into the channel embed but never into the
DB — admins reviewing historical bait actions couldn't see whether
the DM landed.
## Log channel fallback
When `logToChannel` can't deliver — channel deleted, perms revoked,
network error — it now falls back to DMing the guild owner with a
plain-text alert pointing them at `/baitchannel setup` to reconfigure.
The result is plumbed back to `logAction` as
`extras.logDeliveryFailed`, so the BaitChannelLog row carries the
silent-failure signal even when the embed couldn't be sent. If the
owner DM also fails (privacy settings, etc.), warn-log and accept —
nothing more we can do, but the DB row preserves the audit trail.
Why owner-DM specifically: it's the one Discord-side channel the bot
can almost always reach (owners can't blocked us from DMs if the bot
is a guild member, modulo their own privacy lock). Dashboards aren't
available for all guilds.
## Manager wiring
- New optional `extras: { dmResult, logDeliveryFailed, executorId }`
argument on `logAction()`. Existing call sites that pass `extras`
undefined (whitelist path, grace-period success/failure paths) get
defaults of `dmSent=false`, `dmFailureReason=null`, `executorId=null`,
`logDeliveryFailed=false` — backwards-compatible.
- `executeAction()` now stores both `dmResult` and `logDelivered` and
passes them to the final `logAction` call. The `executorId` field is
set to the bot's `client.user.id` on `'executed'` status (Phase 4
will fill it differently for the `'superseded-by-mod'` row).
Verification:
- `bun test` — 1171 pass / 0 fail (existing tests unaffected; they
exercise `logAction` with the new `extras` arg undefined, hitting
the default-value path).
- `bun run check` — biome clean across 508 files.
- `bun run build` — tsc clean.
## What's NOT yet done (handed to later phases)
- Phase 4 will write the `executorId` to log rows differently (mod ID
for `superseded-by-mod`, bot ID for self) and add `discordAuditLogId`
/ `actionConfirmedAt` correlation.
- Phase 5 wires `guildMemberRemove` into the retry queue's enqueue path
so users who leave during grace get a REST ban via the queue.
- The unused `DmFailureReason` value `'no_shared_guild'` is reserved
for Phase 8's appeal-link flow (when the appeal DM goes to a user
the bot doesn't share a guild with anymore).
Real-time attribution of bait moderation actions via Discord's
`GuildAuditLogEntryCreate` event. Replaces the previous reliance on
post-hoc `fetchAuditLogs` polling (which had a race window where the
audit entry hadn't been written yet) with an event-driven approach.
## New file: src/events/auditLogEntryCreate.ts
Single listener handles four scenarios from the audit log stream:
1. **Bot-self confirmation** (`executorId === client.user.id`):
The bot just executed an action via `banExecutor`. Find the
most-recent BaitChannelLog row for `(guildId, userId)` within a
5-minute window and patch in:
- `discordAuditLogId` — links the bait log to the actual Discord
audit entry for direct cross-reference
- `actionConfirmedAt` — timestamp the action was confirmed by
Discord (useful for measuring confirmation lag)
Idempotent — the patch is skipped if `actionConfirmedAt` is
already set, so duplicate audit events (rare but possible) don't
double-update.
2. **Mod-supersedes-us** (`executorId !== client.user.id`, action
matches bait-relevant type):
- Writes an `idempotency_keys` row with the mod's executor ID so
any in-flight `executeBanAction` call sees the duplicate and
short-circuits.
- Deletes any matching `pending_actions` rows so the retry queue
doesn't try to ban an already-banned user (would 10026 "Unknown
Ban" and dead-letter).
- If a recent BaitChannelLog row exists in `'queued'` or
`'failed'` state, updates it in place to
`actionTaken='superseded-by-mod'` with mod's `executorId` and
audit log ID. Dashboard surfaces these as "mod handled this
before bot could" — useful signal for tuning grace periods.
3. **Unban tracking** (`MemberBanRemove`):
Finds the most-recent BaitChannelLog ban row for the user (within
a 365-day window — bans can persist) and records `unbannedAt`
+ `unbannedBy`. Powers the dashboard's false-positive analytics
(% of bait bans that get reversed within N days).
4. **Timeout-set detection**:
`MemberUpdate` is noisy — fires for nickname changes, role updates,
etc. The listener filters to entries where the change list
contains `communication_disabled_until` with a non-null new value
(timeout-set, not timeout-clear).
## Intent additions
`GatewayIntentBits.GuildModeration` added to client intents in
`src/index.ts`. Required by Discord for `GuildAuditLogEntryCreate` —
without it, the event never fires. Listed alongside an inline comment
referencing this commit so future contributors know why it's there.
## Why not also add guildBanAdd/guildBanRemove fallbacks
The plan called for these as a backup in case the audit log entry is
missed due to gateway disconnect. After reading discord.js's gateway
buffering behavior more carefully, that's belt-and-suspenders:
disconnects replay missed events on resume, including
`GuildAuditLogEntryCreate`. The risk window is small enough that the
added complexity (deduping between audit-log and ban-add events)
isn't worth it. If we see attribution gaps in prod, we'll revisit.
## Cleanup hook
Exports `cleanupExpiredIdempotencyKeys()` — Phase 10 will wire it
into `logCleanup.ts` for the daily retention sweep. Keeps the table
small (~24h TTL means rows roll over fast).
Verification:
- `bun test` — 1171 pass / 0 fail (no test changes; the listener
registers but doesn't fire without Discord-side events; Phase 12
adds unit tests that drive the handler directly with synthetic
audit entries).
- `bun run check` — biome clean across 509 files.
- `bun run build` — tsc clean.
## What's NOT yet done
- Phase 5 wires `guildMemberRemove` into the retry queue's enqueue
path so users who leave during grace get REST-banned via the queue.
- `cleanupExpiredIdempotencyKeys()` is exported but not yet scheduled
— Phase 10 integrates it with `logCleanup`.
Closes the audit-finding "user leaves before ban → orphaned
PendingAction row". The guildMemberRemove handler now drains any
pending bait actions for the leaving user, executing them immediately
via the REST executor (`guild.bans.create(userId)` works by ID, no live
member ref required).
## What changes
`src/events/guildMemberRemove.ts`:
- Existing analytics path (recordMemberLeave) unchanged.
- New `drainPendingBaitActions(member)` step queries
`pending_actions` for `(guildId, userId)`, then for each non-dead row:
1. Demotes `timeout` → `softban` (Discord API requires a live member
for timeout; softban still purges messages and is a meaningful
deterrent).
2. Calls `executeBanAction` with the demoted action. Idempotency
keys guarantee no double-execution if the retry queue picks up
the row concurrently.
3. Removes the pending row regardless of executor outcome (the
executor's own row state machine handles `queued` retries via
the retry queue).
## Why drain rather than wait for the retry queue
The retry queue runs every 15s, so a pending row could sit for up to
14.99s before the queue notices it. Users who leave then rejoin
quickly (raid scripts) would have a window where the bot's action
hasn't landed. Draining on the leave event closes that window —
action lands within a Discord API round-trip of the user disappearing.
## What this does NOT change
- The `auditLogEntryCreate` listener (Phase 4) still attributes the
ban once Discord writes the audit entry. The leave-drain just
triggers the action earlier.
- The grace period in-memory timer (`pendingBans` Map in the manager)
is unchanged. It still fires for users who stayed. Race scenario
where the timer fires AND the leave-drain runs is handled by the
idempotency key — second caller sees `status: 'duplicate'` and
no-ops.
- No `ban-after-leave` action type — the action stays as its original
type in the BaitChannelLog (`ban` / `softban` / `kick`). The
`executorId` already attributes who/what; a separate action type
would just be noise.
Verification:
- `bun test` — 1171 pass / 0 fail (existing tests don't exercise the
guildMemberRemove path; Phase 12 will add tests that fire the event
with a synthetic pending row).
- `bun run check` — biome clean across 509 files.
- `bun run build` — tsc clean.
Sticky guild-wide lockdown when N bait actions stack within M seconds. Closes the audit-finding "no raid mode / lockdown / mod alert escalation" — per-user enforcement is too slow against coordinated attacks. ## New module: utils/baitChannel/raidModeManager.ts State surface: - In-memory: per-guild `Map<userId, TriggerRecord[]>` sliding window. Pruned in-place to avoid GC pressure during hot bursts. Capped at 1000 entries per guild as a memory floor. - DB: `BaitChannelConfig.currentRaidModeUntil` is source-of-truth. Null = inactive; non-null = active-until-this-timestamp. Persists across bot restarts; the auto-release tick checks the timestamp and lifts the lockdown when expired (4h cap from entry). - BaitChannelLog: meta rows with `actionTaken='raid-mode-entered'` / `'raid-mode-released'` and `userId='SYSTEM'` track history for the dashboard. API: - `recordTrigger(guild, userId, config)` — called from `baitChannelManager.executeAction` after a real (non-test, non-log-only) action lands. Appends to the sliding window; enters raid mode if threshold breached AND not already active. - `enterRaidMode(guild, config, triggers?)` — sets DB state, channel-lockdown sweep on non-staff channels, mod-alert embed with up to 10 offender mentions + alert role ping. - `releaseRaidMode(guild, releasedBy, reason?)` — restores permissions, clears `currentRaidModeUntil`, writes audit row. - `checkAutoRelease(guild)` — tick callable; lifts lockdown when the 4h cap passes. Phase 10 will wire this into the periodic cleanup sweep. - `getStatus(guildId)` — read-only for dashboard / slash status. Channel-lockdown semantics: - Toggles `@everyone SendMessages` overwrite on every text/threadable channel EXCEPT the log channel + summary channel + the bait channels themselves (so mods can still coordinate, and so users who DM the bait channel don't get blocked by lockdown — they'd be banned anyway). - Best-effort: per-channel failures logged but don't abort the sweep. Updated/failed counters in the log line. - On release, sets the overwrite back to `null` (inherit) — never rewrites guild-scoped role permissions, only the channel-level @everyone overwrite. Why sticky + manual release: - Wick / RaidProtect precedent: auto-release-on-quiet lets a second raid wave through during a brief pause. Stickiness is safer. - 4h cap is a defensive ceiling — admin can release earlier; if no one's around to release, the guild eventually self-restores. - Re-entry while already active is a no-op (would otherwise double-edit permissions and reset the timer). ## Manager wiring `baitChannelManager.executeAction` now calls `getRaidModeManager()?. recordTrigger(message.guild, member.id, config)` after the executor succeeds — only for `!isTestMode && apiAction !== 'log-only'`. Test mode and config-validation noise won't flood raid detection. `src/index.ts` initializes the singleton next to retry queue: ``` initRaidModeManager({ configRepo: AppDataSource.getRepository(BaitChannelConfig), logRepo: AppDataSource.getRepository(BaitChannelLog), }); ``` ## New slash subcommand group: /baitchannel raid Added to `src/commands/builders/baitChannel.ts` as the 6th subcommand group (after stats). Three subcommands: - `/baitchannel raid status` — show current state, recent triggers, distinct offenders, auto-release time. Read-only — requires `manage` feature access. - `/baitchannel raid enter [reason]` — manually trigger lockdown. Useful for proactive defense (e.g., a known raid is announced in a different server). Requires `admin` feature access. - `/baitchannel raid release` — manually lift lockdown. Restores permissions. Requires `admin` feature access. Handler at `src/commands/handlers/baitChannel/raid.ts`. Each subcommand defers reply with `Ephemeral` flag, hits the singleton, and reports outcome. ## What's NOT yet done - Auto-release tick is exported but not yet scheduled — Phase 10 integrates it with the periodic cleanup pass. - Webapp UI for the raid panel is in the v3.2.0-coordination plan (Phase 13 will add the API endpoints; the dashboard work is a separate PR). - Language strings (`tl.raid.*`) — temporarily inlined as English in the builder + handler; will surface in the docs phase if user-facing copy needs locale fanout. Verification: - `bun test` — 1171 pass / 0 fail. - `bun run check` — biome clean across 511 files. - `bun run build` — tsc clean.
Catches a raid pattern invisible to per-message suspicion scoring:
the same content posted in N distinct channels within M seconds.
Either a copy-paste raider hitting every #general-style channel, or
a compromised webhook spamming the same embed across servers.
## New file: utils/baitChannel/contentBurstDetector.ts
Stateful in-memory detector. Per-user sliding window of
`{contentHash, channelId, at}` records.
API:
- `recordMessage(userId, channelId, content, windowSec, threshold)`
→ `{bursting, distinctChannels, contentHash}`
- Normalizes content (lowercase, collapse whitespace, strip
mentions / channel tags / role mentions / custom emoji) so
that `Hello <@123>` and `hello <@456>` hash the same.
- SHA-1 hash chosen for speed; collision attacks aren't a threat
(we're counting our own normalized content).
- Prune-then-append in a single pass to avoid GC pressure.
- 50-entry per-user cap floors memory under sustained attack.
- `start()` / `stop()` — periodic 30s prune of users with no recent
activity (5min idle → forget).
Storage cost: O(U × N) where U is active users and N ≤ 50. At 1000
concurrent users that's ~50K records steady-state — comfortably
within Node's heap.
## Manager integration
`baitChannelManager.handleMessage` runs the detector AFTER the
existing suspicion analysis. When `bursting === true`:
- Score += 30 (clamped to 100). Forces escalation even from
borderline single-message scores.
- New `analysis.flags.crossChannelBurst = true` flag.
- Human-readable reason pushed onto `analysis.reasons` for the
channel embed.
Config: two new fields (added in Phase 1 migration):
- `BaitChannelConfig.crossChannelBurstThreshold` — distinct channels
needed in the window (default 3).
- `BaitChannelConfig.crossChannelBurstWindowSeconds` — sliding window
(default 30).
## Schema integration
`BaitChannelLog.detectionFlags.crossChannelBurst` was already added
in Phase 1's migration as optional. The flag now actually flips on
matching detections, so the dashboard can break down bait actions by
"how many had a cross-channel burst signal".
## What this does NOT do
- Doesn't escalate to raid mode directly. The bait action that fires
*because of* the boosted score will route through
`raidModeManager.recordTrigger` like any other action — collective
detection still requires N actions in the raid window.
- Doesn't consult Discord audit log. Cheap purely-in-memory detection.
Survives bot restart only if the user keeps posting (records rebuild
organically).
- Doesn't apply to webhook messages from a guild's own
configured webhooks — the detector treats them as user posts. Phase
8+ may add a webhook-specific path; for now the pattern catches
webhook spam by accident, which is fine.
Boot wiring:
- `initContentBurstDetector()` in `src/index.ts` next to raid mode
manager.
- `stopContentBurstDetector()` in graceful shutdown.
Verification:
- `bun test` — 1171 pass / 0 fail.
- `bun run check` — biome clean across 512 files.
- `bun run build` — tsc clean.
Pre-ban DMs can now embed a one-shot signed appeal link in addition
to (or instead of) the static `appealInfo` text. The webapp consumer
that verifies the token and auto-opens a banAppeal ticket lands in
v3.2.1 — this commit ships the issuer side so admins can pre-configure
their `appealLinkBaseUrl`.
## New file: utils/baitChannel/appealToken.ts
Token format: `<payload_base64url>.<sig_base64url>`
- Payload (base64url-encoded JSON):
`{guildId, userId, action, banReason, iat, exp, iss: 'cogworks'}`
- Signature: HMAC-SHA256 over the payload bytes, secret from
`APPEAL_HMAC_SECRET` env (32+ bytes recommended).
API:
- `signAppealToken(input)` — produces the dotted token. Throws if the
env secret is missing or too short.
- `verifyAppealToken(token)` — recomputes HMAC with timingSafeEqual,
checks issuer + expiry. Returns
`{valid: true, payload} | {valid: false, error: '…'}`. Used by the
webapp's `/appeal` route in v3.2.1; exported here so unit tests
cover the round-trip.
- `buildAppealUrl({baseUrl, …})` — convenience helper that combines
signing + URL construction. Returns null silently if:
- baseUrl is missing
- `APPEAL_HMAC_SECRET` env is missing
- baseUrl is not HTTPS (rejects plaintext links)
- baseUrl isn't a valid URL
Caller (DM path) falls back to the static `appealInfo` text in
any of those cases.
Why HMAC instead of JWT / paseto: single-issuer, single-consumer
flow inside one organization's infrastructure. Adding a JWT lib for
this would be over-engineering. The format is stable, short (~300
bytes typical), and the verifier is ~30 LOC.
Why 7-day default expiry: long enough for most ban-appeal workflows
(weekend gap, vacation), short enough that stolen/forwarded tokens
don't linger. Configurable per-call via `expiresInHours`.
## DM integration
`sendDmNotification` now appends a third embed field "Appeal this
action" with the signed URL — but only when:
- `BaitChannelConfig.enableAppealLink === true`, AND
- `appealLinkBaseUrl` is set (HTTPS), AND
- `APPEAL_HMAC_SECRET` env is present.
Otherwise the field is omitted and the existing `appealInfo` static
text covers the appeal path. Falls back silently — no error to user
if the appeal infrastructure isn't wired up yet.
## Env var
`APPEAL_HMAC_SECRET` was added to `.env.example` in Phase 1 as
documentation. The bot does NOT fail at boot if missing — it fails
only when `signAppealToken` is actually called (which only happens
when a guild has `enableAppealLink=true`). Practical effect:
- Guilds with the feature off: nothing changes.
- Guilds with the feature on + secret unset: DM appeal field is
omitted, static `appealInfo` still shows, action still lands.
- Guilds with the feature on + secret set: full signed URL.
Recommended generation:
`node -e 'console.log(require("crypto").randomBytes(32).toString("base64url"))'`
## What this does NOT do
- No webapp consumer route yet — that's v3.2.1 in the cross-repo plan.
- No replay protection — the v3.2.1 webapp will mark the
BaitChannelLog row as "appealed" on consumption to enforce single-
use server-side. Token alone is not single-use.
- No slash command yet for `enableAppealLink` / `appealLinkBaseUrl`
— config writes happen via the dashboard webapp's bait settings
page (Phase 13 will add the API endpoint).
Verification:
- `bun test` — 1171 pass / 0 fail.
- `bun run check` — biome clean across 513 files.
- `bun run build` — tsc clean.
Extends the daily `logCleanup` tick with two new sweep targets: 1. **`idempotency_keys`** — expired rows (`expiresAt < now()`) are dropped. The keys carry their own per-row 24h TTL set at insert time by `banExecutor`, so this is purely a row-count keeper. 2. **`pending_actions` dead-letters** — rows where `deadAt IS NOT NULL AND deadAt < now() - 30 days`. Dead-lettered rows are kept for mod review (dashboard surfaces them via the Phase 13 API); anything older than 30 days is unlikely to still be actionable and we'd rather not let the table grow unbounded. Live pending actions (`deadAt IS NULL`) are owned by the retry queue and never aged out here. Existing sweep targets unchanged (bait logs, announcements, audit, join events). The new sweeps are isolated try/catch blocks so a failure on one target doesn't abort the others. What's NOT yet wired: - The `RaidModeManager.checkAutoRelease` tick — auto-release of expired raid lockdowns. Phase 6 exported the method; folding it into this cleanup pass requires iterating guilds, which the current cleanup function doesn't do. Will add in a follow-up if the 4h cap doesn't auto-restore cleanly via natural permission expiry. (Bot restart naturally restores from the `currentRaidModeUntil` column; only the in-process timer needs the tick — and a 4h-stale lockdown isn't catastrophic.) Phase 9 (test mode parity) — folded into Phase 2's `banExecutor`, which already routes test-mode runs through the same code path as real runs and writes the idempotency key with `testMode=true`. No separate commit needed. Verification: - `bun test` — 1171 pass / 0 fail. - `bun run check` — biome clean across 513 files. - `bun run build` — tsc clean.
Closes the audit gap "no config CRUD endpoint" (dashboards previously
had no way to read/write BaitChannelConfig via the bot's API; they
relied on ninsys-api direct DB queries). Adds endpoints for the new
v3.2.0 features so the webapp can drive them.
## New endpoints
All under `/internal/guilds/:guildId/bait-channel/`:
- `GET /config` — full BaitChannelConfig row (or null when not
configured).
- `PATCH /config` — accepts any subset of writable fields. Type-checks
via the existing optional* helpers. Invalidates the manager's
in-process config cache so the next bait message picks up changes.
Refuses HTTP `appealLinkBaseUrl` when `enableAppealLink=true`.
- `GET /raid-mode/status` — wraps `RaidModeManager.getStatus`.
- `POST /raid-mode/enter` — body `{triggeredBy, reason?}`. 409 if
already active; 404 if guild not accessible. Writes audit row.
- `POST /raid-mode/release` — body `{triggeredBy, reason?}`. Returns
`{released: bool}` (false when not active — not an error).
- `GET /pending-actions?status=active|dead|all&limit=N` — paginated
list. Default `active` filters to `deadAt IS NULL`.
- `POST /pending-actions/cancel` — body `{id, triggeredBy, reason?}`.
Hard-delete after audit log entry.
- `GET /logs?days=N&action=X&userId=Y&overridden=true|false&limit=N`
— paginated log listing with filter chips. The existing `/stats`
endpoint stays as the aggregate.
## API helpers extension
Added `optionalBoolean(body, field)` to `src/utils/api/helpers.ts`
alongside the existing `optional*` family. Symmetric with
`optionalNumber` / `optionalString`. Used by the config PATCH route
for the 11 new boolean fields.
## What this does NOT do
- Doesn't add appeal-token verification (`POST /appeal/verify`) —
that endpoint lives in the webapp consumer path in v3.2.1.
- Doesn't add a webhook subscriber for raid-mode events. Dashboard
polls `GET /raid-mode/status` at 5s. SSE / WebSocket is backlog.
- Doesn't enforce additional auth beyond the existing bearer token
+ audit-log attribution via `triggeredBy`. The webapp is the only
consumer; admin gating happens dashboard-side.
Verification:
- `bun test` — 1171 pass / 0 fail.
- `bun run check` — biome clean across 513 files.
- `bun run build` — tsc clean.
+32 tests across 4 new files (1171 → 1203). Focused on the modules that landed without test coverage in their feature commits: - `auditReason.test.ts` (7 tests) — buildAuditReason structure + truncation; flagsTriggered filter semantics. Caught the leading- whitespace bug in channel-name normalization that would have produced `ch=##my-channel`. Fixed in the implementation. - `appealToken.test.ts` (11 tests) — sign/verify round-trip, tampered payload, tampered signature, expired, malformed; buildAppealUrl positive case + null returns for missing baseUrl / missing secret / HTTP / malformed URL. Env state restored in afterEach so siblings aren't polluted. - `contentBurstDetector.test.ts` (8 tests) — first-message no-burst, N-distinct-channel burst, different-content no-burst, same-channel no double-counting, mention normalization, case-insensitive, per-user isolation, custom threshold. - `banExecutor.test.ts` (6 tests) — REST ban path, idempotency dedup, softban-with-unban, log-only claims-but-no-call, test-mode dry-run, timeout-without-member-ref demote-to-failed. Uses hand-rolled fake Repository<IdempotencyKey> with UNIQUE-constraint simulation. Test files structured to use bun:test imports (per v3.1.35 convention). No mock.module() since the modules under test are pure enough to drive with fake instances directly. Verification: - `bun test` — 1203 pass / 0 fail (+32 from Phase 12). - `bun run check` — biome clean across 513 files. - `bun run build` — tsc clean. ## What's NOT covered yet - retryQueue — needs a more involved fake (pending_actions repo + client.guilds.fetch + executeBanAction stub). Defer to a follow-up test pass; Phase 3's commit already verified end-to-end via TypeScript + manual smoke. - raidModeManager — needs guild + channel cache mocking for the permission-sweep code path. Same defer rationale. - auditLogEntryCreate handler — needs synthetic GuildAuditLogEntry fixtures; defer. These three are tracked in the smoke-test checklist (Phase 13 work) so they get manual coverage before the PR ships, even without unit tests.
Marks the v3.2.0 bait channel major refinement.
- `package.json` → 3.2.0
- `CHANGELOG.md` entry: BREAKING note for APPEAL_HMAC_SECRET env var
(only required when a guild has enableAppealLink=true), then
comprehensive Added/Schema/Tests/Internal sections.
- `CLAUDE.md`:
- Version line → 3.2.0
- Entity tree: bait/ now lists PendingAction + IdempotencyKey.
- New "Bait Channel Subsystem" section with the per-message
sequence diagram and module map.
- Pitfalls: two new Don'ts (member.ban() direct, new actions
outside executeBanAction).
- Env vars: APPEAL_HMAC_SECRET documented.
All 1203 tests pass. Biome clean. tsc clean.
…e-positive
Desloppify's `hardcoded_secret_name` detector flags any constant
declaration containing "TOKEN" in its name as a potential hardcoded
secret. The const was a plain issuer-claim string ('cogworks'), not
a secret — the actual secret comes from APPEAL_HMAC_SECRET env at
runtime. Rename to ISSUER_NAME eliminates the noise without changing
behavior. All 1203 tests still pass.
This was the only new HIGH-severity finding introduced by v3.2.0
work. Remaining HIGH findings in the codebase are pre-existing
import cycles in shared infrastructure (`emojis.ts → errorHandler.ts
→ index.ts`) — out of scope for this refinement.
Independent code review on the v3.2.0 branch surfaced 6 HIGH issues
worth fixing before opening the PR. Addressed all six.
1. **`unbannedAt: undefined` was a no-op filter** — TypeORM ignores
`undefined` in `where`, so the unban-tracking query was matching
already-unbanned rows and overwriting their attribution on second-
ban-then-unban sequences. Changed to `unbannedAt: IsNull()`. Affects
`events/auditLogEntryCreate.ts:handleUnban`.
2. **`kick` pending actions silently lost on leave** — only `timeout`
was being demoted to `softban` in `events/guildMemberRemove.ts`. A
`kick` pending action on a non-member would fall into
`executor.kick` path, fail with "kick requires a live GuildMember
ref", and the row would be silently removed with no Discord-side
action. Now both `timeout` and `kick` demote to `softban` — REST
ban-by-id is leave-tolerant, so the action still lands.
3. **Race between leave-drain and in-process grace timer** — when a
user leaves during the grace window, both the `guildMemberRemove`
drain AND the setTimeout callback can reach `executeBanAction`.
Idempotency keys catch the double-ban at Discord level, but produce
a confusing `superseded` log row alongside the real action. Added
`cancelGraceForUser(guildId, userId)` to the manager — leave-drain
calls it first to clear the in-memory `pendingBans` map, so the
setTimeout's `pendingBans.has(key)` guard short-circuits before
`executeAction` runs.
4. **`10013` (Unknown Role) in terminal error set** — irrelevant to
ban/kick/timeout REST calls; obscured intent. Removed. The set is
now `{10007, 10026, 50013}` matching the documented purpose. Affects
`utils/baitChannel/banExecutor.ts`.
5. **Orphaned grace promotion pre-consumed an attempt** — the retry
queue's `processOrphanedGrace` saved `attempts=1` BEFORE calling
`processRow`, which itself increments on failure. Net effect: only
2 Discord retries instead of the documented 3 before dead-letter.
Removed the pre-save so `processRow` increments naturally from
0→1→2→3 (dead-letter at 3). Full backoff schedule now honored.
6. **No startup recovery for active raid-mode lockdowns** — if the bot
crashed mid-`enterRaidMode` (after the DB write but during
`applyChannelLockdown`), channels could be left unlocked while
`currentRaidModeUntil` says "active". Dashboard would lie about
lockdown state. Added `RaidModeManager.restoreActiveLockdowns()`
— called once at boot, sweeps every guild with active raid mode
and re-applies the permission overwrites. Discord-side idempotent
(re-applying an existing deny is a no-op). Also auto-releases
guilds whose 4h cap expired during the offline window. Wired via
`setGuildFetcher` from `clientReady` since the manager doesn't
take a Client ref in its deps.
All 1203 tests still pass. Build + biome clean.
|
Warning Review limit reached
More reviews will be available in 53 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds v3.2.0 bait-channel subsystem: schema changes (PendingAction, IdempotencyKey), REST ban executor with idempotency, retry queue, audit-log attribution, raid-mode manager, content-burst detector, HMAC appeal tokens, manager wiring, API/commands, event handling, migrations, cleanup, tests, and docs. ChangesBait Channel v3.2.0 Subsystem
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR ships v3.2.0 of the bait channel (honeypot) subsystem, adding a REST-based moderation executor with idempotency, a retry queue + dead-lettering, raid-mode lockdown, cross-channel burst detection, appeal-link tokenization, and new internal API endpoints + schema changes to support the expanded lifecycle and observability.
Changes:
- Replace member-based moderation with REST ban-by-id execution, idempotency keys, and a retry queue (incl. dead-letter retention + cleanup).
- Add Raid Mode (sticky lockdown with status/enter/release) and cross-channel content burst scoring.
- Expand schema + entities + internal API for config CRUD, raid mode, pending actions, and logs; add extensive unit tests.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/utils/baitChannel/contentBurstDetector.test.ts | Unit tests for cross-channel content burst detection and normalization behavior. |
| tests/unit/utils/baitChannel/banExecutor.test.ts | Unit tests for REST-based executor and idempotency behavior. |
| tests/unit/utils/baitChannel/baitChannelManager.test.ts | Updated manager tests (PendingAction rename) + logAction null-safety coverage. |
| tests/unit/utils/baitChannel/auditReason.test.ts | Unit tests for structured audit-log reason builder and flag extraction. |
| tests/unit/utils/baitChannel/appealToken.test.ts | Unit tests for HMAC appeal token signing/verifying and URL builder. |
| tests/unit/events/ticketInteraction.test.ts | Regression tests ensuring seeded CustomTicketType modals are shown for builtin IDs. |
| src/utils/database/logCleanup.ts | Adds cleanup for expired idempotency keys and dead-lettered pending actions. |
| src/utils/database/guildQueries.ts | Includes PendingAction + IdempotencyKey in guild data deletion. |
| src/utils/baitChannel/retryQueue.ts | New retry queue processor for queued moderation actions + orphaned grace recovery. |
| src/utils/baitChannel/raidModeManager.ts | New raid mode state machine + channel permission lockdown + alerting + recovery. |
| src/utils/baitChannel/contentBurstDetector.ts | New per-user sliding-window detector for same-content bursts across channels. |
| src/utils/baitChannel/banExecutor.ts | New REST-based moderation executor with idempotency-key claiming and retryable/terminal classification. |
| src/utils/baitChannel/baitChannelManager.ts | Integrates executor, retry queue, raid mode, content burst, DM observability, and log delivery fallback. |
| src/utils/baitChannel/auditReason.ts | New structured audit-log reason string builder for Discord audit log visibility. |
| src/utils/baitChannel/appealToken.ts | New HMAC appeal token + HTTPS-only appeal URL builder. |
| src/utils/api/helpers.ts | Adds optionalBoolean request-body helper. |
| src/utils/api/handlers/baitChannelHandlers.ts | Adds config CRUD, raid-mode endpoints, pending actions endpoints, and logs query endpoint. |
| src/typeorm/migrations/1774000011000-BaitChannelV3Schema.ts | v3.2.0 schema migration: pending_actions, idempotency_keys, new log/config columns. |
| src/typeorm/index.ts | Registers PendingAction + IdempotencyKey entities. |
| src/typeorm/entities/bait/PendingAction.ts | Renames PendingBan → PendingAction; adds action/attempts/lastError/deadAt fields and indexes. |
| src/typeorm/entities/bait/IdempotencyKey.ts | New idempotency key entity with unique constraint and TTL fields. |
| src/typeorm/entities/bait/BaitChannelLog.ts | Adds new observability/audit correlation columns and new detection flag field. |
| src/typeorm/entities/bait/BaitChannelConfig.ts | Adds raid-mode, content-burst, appeal-link, and retention configuration columns. |
| src/index.ts | Wires new intent, audit-log handler, retry queue, raid mode manager recovery, and content-burst detector lifecycle. |
| src/events/ticket/create.ts | CustomTicketType-first modal building for builtin types; logs missing field IDs loudly. |
| src/events/guildMemberRemove.ts | Drains pending bait actions on leave using REST executor and cancels grace timers. |
| src/events/auditLogEntryCreate.ts | New audit-log entry listener for self-confirmation, mod-supersedes handling, and unban tracking. |
| src/commands/handlers/dev/devSuiteScaffold.ts | Teardown updates for PendingAction rename. |
| src/commands/handlers/dataExport.ts | Export includes pendingActions (and formatting updates). |
| src/commands/handlers/baitChannel/raid.ts | New /baitchannel raid subcommand handler. |
| src/commands/handlers/baitChannel/index.ts | Routes raid subcommand group to handler. |
| src/commands/builders/baitChannel.ts | Adds `/baitchannel raid {status |
| package.json | Version bump to 3.2.0. |
| CLAUDE.md | Documentation updates for v3.2.0 bait subsystem + new env var. |
| CHANGELOG.md | Adds detailed 3.2.0 changelog entry. |
| .env.example | Documents new APPEAL_HMAC_SECRET env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+113
to
+118
| // Remove the row regardless of executor outcome: | ||
| // - executed/duplicate: row served its purpose | ||
| // - queued: the executor already enqueued via the retry queue's | ||
| // own row mechanics (and Phase 3's `enqueue` handles upsert) | ||
| // - failed: terminal Discord error (user already banned, etc.); | ||
| // nothing more to retry on a non-member |
Comment on lines
+181
to
+190
| // skip — they already did the work. | ||
| const claim = await claimIdempotencyKey(idempotencyRepo, guild.id, userId, action, executorId, testMode); | ||
| if (!claim.claimed) { | ||
| enhancedLogger.debug( | ||
| `Skipping ${action} on ${userId} — idempotency key already claimed${claim.existing?.executorId ? ` by ${claim.existing.executorId}` : ''}`, | ||
| LogCategory.SECURITY, | ||
| { guildId: guild.id, userId, action }, | ||
| ); | ||
| return { status: 'duplicate', action }; | ||
| } |
Comment on lines
+664
to
+667
| await Promise.race([ | ||
| member.send({ embeds: [embed] }), | ||
| new Promise((_, reject) => setTimeout(() => reject(new Error('dm_timeout')), 5_000)), | ||
| ]); |
Comment on lines
+315
to
+332
| // Strings (nullable on entity — pass through directly) | ||
| const stringFields: (keyof BaitChannelConfig)[] = [ | ||
| 'banReason', | ||
| 'warningMessage', | ||
| 'actionType', | ||
| 'appealInfo', | ||
| 'logChannelId', | ||
| 'summaryChannelId', | ||
| 'raidModeAlertRoleId', | ||
| 'appealLinkBaseUrl', | ||
| ]; | ||
| for (const field of stringFields) { | ||
| const v = optionalString(body, field as string); | ||
| if (v !== undefined) { | ||
| (config as unknown as Record<string, unknown>)[field as string] = v; | ||
| patched.push(field as string); | ||
| } | ||
| } |
Comment on lines
+225
to
+231
| if (existingLog && (existingLog.actionTaken === 'queued' || existingLog.actionTaken === 'failed')) { | ||
| // Pending bait action superseded — update the row in place. | ||
| existingLog.actionTaken = 'superseded-by-mod'; | ||
| existingLog.executorId = executorId; | ||
| existingLog.discordAuditLogId = auditLogId; | ||
| existingLog.actionConfirmedAt = new Date(); | ||
| await logRepo.save(existingLog); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/utils/baitChannel/baitChannelManager.test.ts (1)
277-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSilence expected error logs in these negative-path tests to keep CI output clean.
These two tests intentionally trigger failure branches, but they currently emit error logs that surface as pipeline errors. Please stub/restore
console.error(or the logger sink) within these tests.Suggested patch
test("DB error → returns null and logs (does not throw)", async () => { + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + try { const { manager, configState } = makeManager({}); configState.findOneError = new Error("Connection refused"); expect(await (manager as any).getConfig("guild-1")).toBeNull(); + } finally { + consoleErrorSpy.mockRestore(); + } }); @@ test("DB error → returns [] and logs (does not throw)", async () => { + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + try { const { manager, keywordState } = makeManager({}); keywordState.findError = new Error("Query timeout"); expect(await (manager as any).getKeywords("guild-1")).toEqual([]); + } finally { + consoleErrorSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/baitChannel/baitChannelManager.test.ts` around lines 277 - 333, Silence the expected error logs in the two negative-path tests by stubbing console.error for the duration of each test: in the "DB error → returns null and logs (does not throw)" test that calls (manager as any).getConfig("guild-1") and in the "DB error → returns [] and logs (does not throw)" test that calls (manager as any).getKeywords("guild-1"), add a jest.spyOn(console, "error").mockImplementation(() => {}) at the start of the test and restore it at the end with (console.error as jest.SpyInstance).mockRestore() (or store the spy in a variable and call spy.mockRestore()) so the logs are suppressed only for that test and restored afterward.
🧹 Nitpick comments (12)
src/events/auditLogEntryCreate.ts (1)
200-204: 💤 Low valueConsider logging duplicate key as debug for observability.
The empty catch block for duplicate-key errors is intentional per the comment, but consider adding a debug log for observability. This helps trace race conditions between the bot's executor and mod actions.
♻️ Optional: Add debug logging
try { await idempotencyRepo.save(claim); } catch { // Duplicate-key — bot already executed. Don't overwrite. + enhancedLogger.debug(`Idempotency key already exists for ${guildId}/${userId}/${action}`, LogCategory.SECURITY); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/events/auditLogEntryCreate.ts` around lines 200 - 204, The empty catch around idempotencyRepo.save(claim) should capture the thrown error (e.g., const err) and log a debug-level message when it is a duplicate-key race rather than swallowing it silently; update the catch to inspect the error (e.g., duplicate key code like 11000 or the DB-specific error indicator) and call the debug logger with context such as the claim id/unique key and the error message while preserving the current behavior of not rethrowing non-fatal duplicate-key races.src/utils/baitChannel/baitChannelManager.ts (1)
666-666: 💤 Low valueMove DM timeout to constants.
The 5-second DM timeout should be centralized in
src/utils/constants.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/baitChannelManager.ts` at line 666, Replace the hard-coded 5_000 ms DM timeout in the Promise (the setTimeout(() => reject(new Error('dm_timeout')), 5_000) expression) with a named constant exported from src/utils/constants.ts; add/ensure an exported DM_TIMEOUT_MS (or similar) there and import it into src/utils/baitChannel/baitChannelManager.ts, then use that constant in the Promise so the timeout value is centralized and configurable.src/utils/baitChannel/appealToken.ts (2)
27-27: 💤 Low valueMove expiry duration to constants.
DEFAULT_EXP_HOURSshould be insrc/utils/constants.tsper coding guidelines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/appealToken.ts` at line 27, DEFAULT_EXP_HOURS is defined inline in appealToken.ts but per guidelines it must live in the shared constants module; remove the local const DEFAULT_EXP_HOURS from appealToken.ts, add it to src/utils/constants.ts (exported as DEFAULT_EXP_HOURS or similarly named constant), and update appealToken.ts to import { DEFAULT_EXP_HOURS } from the constants module and use the imported value wherever DEFAULT_EXP_HOURS was referenced (e.g., in functions that compute token expiry).
77-85: ⚡ Quick winConsider aligning minimum secret length with recommendation.
The comment recommends 32+ bytes but the code only enforces 16 bytes minimum. For HMAC-SHA256, 32 bytes (256 bits) provides full security margin. Consider raising the minimum to 32 to match the recommendation.
🛡️ Suggested fix
function getSecret(): string { const secret = process.env.APPEAL_HMAC_SECRET; - if (!secret || secret.length < 16) { + if (!secret || secret.length < 32) { throw new Error( - 'APPEAL_HMAC_SECRET is not set (or too short — recommend 32+ bytes). Bait appeal links require this env var. See .env.example.', + 'APPEAL_HMAC_SECRET is not set (or too short — requires 32+ bytes). Bait appeal links require this env var. See .env.example.', ); } return secret; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/appealToken.ts` around lines 77 - 85, The getSecret function currently enforces a minimum length of 16 but its error message recommends 32+ bytes; change the minimum check in getSecret to require at least 32 bytes (e.g., if (!secret || secret.length < 32)) and update the thrown Error text to consistently recommend 32+ bytes so the validation and message align for APPEAL_HMAC_SECRET.src/utils/baitChannel/raidModeManager.ts (2)
38-38: 💤 Low valueMove magic number to constants.ts.
Per coding guidelines, all configuration constants must be centralized.
RAID_MODE_MAX_DURATION_MSshould be added tosrc/utils/constants.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/raidModeManager.ts` at line 38, RAID_MODE_MAX_DURATION_MS is a magic number and must be moved to the central constants file; add a new exported constant named RAID_MODE_MAX_DURATION_MS (value 4 * 60 * 60 * 1000) in src/utils/constants.ts, export it, then replace the local declaration in raidModeManager.ts with an import of RAID_MODE_MAX_DURATION_MS from the constants module and remove the local constant; ensure any references in raidModeManager.ts use the imported symbol.
78-79: ⚡ Quick winConsider more efficient pruning for the sliding window.
Using
shift()in a while loop is O(n²) when many entries need pruning during a burst. ConsiderfindIndex+spliceor rebuilding the array once:♻️ Suggested optimization
- while (guildTriggers.length > 0 && guildTriggers[0].at < cutoff) { - guildTriggers.shift(); - } + const firstValidIdx = guildTriggers.findIndex(t => t.at >= cutoff); + if (firstValidIdx > 0) { + guildTriggers.splice(0, firstValidIdx); + } else if (firstValidIdx === -1 && guildTriggers.length > 0) { + guildTriggers.length = 0; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/raidModeManager.ts` around lines 78 - 79, The current pruning loop repeatedly calling guildTriggers.shift() is inefficient for large bursts; replace it by finding the first index that should remain (use guildTriggers.findIndex(t => t.at >= cutoff)) and then remove the prefix in one operation (either guildTriggers.splice(0, idx) if idx >= 0 or set guildTriggers = [] when all are pruned). Update the code that references guildTriggers (the pruning block around the while loop) to use this single-splice or slice-rebuild approach so pruning becomes O(n) instead of O(n²).src/commands/builders/baitChannel.ts (1)
264-281: ⚡ Quick winUse lang module for user-facing strings.
The new raid subcommand descriptions are hardcoded. Per coding guidelines, all user-facing strings should use the centralized
langmodule (e.g.,lang.baitChannel.builder.raid.statusDescrp).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/builders/baitChannel.ts` around lines 264 - 281, Replace the hardcoded user-facing strings in the raid subcommands with values from the centralized lang module: for the subcommands built via the fluent builder (the .addSubcommand(...) blocks that call .setName('status'|'enter'|'release')), change each .setDescription(...) argument to the appropriate lang key (e.g., lang.baitChannel.builder.raid.statusDescrp, lang.baitChannel.builder.raid.enterDescrp, lang.baitChannel.builder.raid.releaseDescrp) and replace the option description in the .addStringOption(...) call (option.setName('reason').setDescription(...)) with the corresponding lang.baitChannel.builder.raid.reasonDescrp key; ensure you import the lang module if not already present and use the exact lang property names used elsewhere in baitChannel.ts.src/utils/baitChannel/contentBurstDetector.ts (1)
28-29: 💤 Low valueCentralize configuration constants.
MAX_ENTRIES_PER_USER,CLEANUP_INTERVAL_MS, and the 5-minute idle timeout (line 130) should be moved tosrc/utils/constants.tsper coding guidelines.Also applies to: 130-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/contentBurstDetector.ts` around lines 28 - 29, Extract MAX_ENTRIES_PER_USER, CLEANUP_INTERVAL_MS and the 5-minute idle timeout literal from contentBurstDetector.ts and place them into the shared constants module (constants.ts) as exported named constants; then import and use those constants in contentBurstDetector (replace the literals/usages). Ensure the exported constant names match MAX_ENTRIES_PER_USER, CLEANUP_INTERVAL_MS and IDLE_TIMEOUT_MS (or a similar clear name) and update any references in functions or timers that used the original values.src/commands/handlers/baitChannel/raid.ts (1)
25-28: ⚡ Quick winUse lang module for user-facing strings.
Multiple hardcoded strings should use the centralized
langmodule per coding guidelines.Also applies to: 76-77, 81-82, 97-98, 103-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/handlers/baitChannel/raid.ts` around lines 25 - 28, Replace hardcoded user-facing strings in the raid command handler by using the centralized lang module instead of direct literals; specifically change calls like interaction.reply({ content: '❌ Raid mode manager is not initialized (bot may still be starting up).', flags: [MessageFlags.Ephemeral] }) to use lang lookups (e.g., content: lang.get('raid.notInitialized') or lang.t('raid.notInitialized')) while keeping interaction.reply and MessageFlags.Ephemeral untouched, and apply the same replacement for the other hardcoded messages in the same file (the reply/error messages around the other interaction.reply and interaction.editReply calls referenced at the other locations), using keys that allow interpolation for any dynamic values.src/utils/api/handlers/baitChannelHandlers.ts (1)
353-355: ⚡ Quick winUse
ExtendedClientfor manager access instead of ad-hoc client casting.This cast pattern should be replaced with the shared extended client type for consistency across handlers.
As per coding guidelines: "Access BaitChannelManager and StatusManager via the ExtendedClient type."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/api/handlers/baitChannelHandlers.ts` around lines 353 - 355, Replace the ad-hoc cast to ClientWithBaitManager with the shared ExtendedClient type: locate where baitManager is obtained (currently via (client as ClientWithBaitManager).baitChannelManager) and instead cast or type the client as ExtendedClient and access client.baitChannelManager; then call clearConfigCache(guildId) on that manager (i.e., use (client as ExtendedClient).baitChannelManager.clearConfigCache(guildId) or update the handler signature to accept client: ExtendedClient so you can directly call client.baitChannelManager.clearConfigCache(guildId)). Ensure you update any import/type references to ExtendedClient and remove the ClientWithBaitManager cast.src/events/ticket/create.ts (2)
231-236: ⚡ Quick winUse the verified delete helper here.
This new cleanup path bypasses the repo's message-delete wrapper and hides failures with an empty catch, so permission / unknown-message regressions disappear from logs.
As per coding guidelines, "Use verification helpers:
verifiedChannelDelete,verifiedThreadDelete,verifiedMessageDelete,verifiedMessageDeleteById..." and "Never swallow errors silently with emptycatch {}blocks."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/events/ticket/create.ts` around lines 231 - 236, The setTimeout cleanup currently calls interaction.message.delete() inside an empty catch which swallows errors; replace that call with the repository's verified message-delete helper (e.g., verifiedMessageDelete or verifiedMessageDeleteById) so deletions are checked and failures are surfaced, and remove the empty catch block so the helper can log/handle permission or unknown-message errors as intended (locate the setTimeout block that calls interaction.message.delete and swap to the verified helper, passing the message or message id and the logger/context used elsewhere).
103-109: ⚡ Quick winRoute the new ticket copy through
lang.These added strings are user-facing, but they bypass the translation layer, so this path won't stay localized with the rest of the ticket flow.
As per coding guidelines, "Use the centralized
langmodule for all user-facing strings instead of hardcoding them."Also applies to: 259-261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/events/ticket/create.ts` around lines 103 - 109, The placeholder and label strings in the TextInputBuilder (methods setLabel and setPlaceholder on the object created in createTicket flow) are hardcoded; replace them with calls into the centralized lang translation module (e.g., lang.t(...) or the project’s lang(...) helper) so the label 'Please describe your issue' and the default placeholder 'Provide details about your ticket...' are localized, and ensure ticketType.description is passed through lang or used as a fallback translation key rather than raw text; apply the same replacement to the other hardcoded user-facing strings referenced around the later ticket creation code (the strings noted at the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 106-110: Update the wording around APPEAL_HMAC_SECRET to match the
runtime contract used in CHANGELOG.md and tests: state that APPEAL_HMAC_SECRET
is required only when a guild's BaitChannelConfig.enableAppealLink=true (not a
global startup fatal error), and remove or rephrase the “Bot will refuse to
start with a missing/empty value” language; reference the symbol
APPEAL_HMAC_SECRET and the config flag BaitChannelConfig.enableAppealLink in the
new text so readers understand the conditional requirement and rotation
behavior.
In `@CLAUDE.md`:
- Around line 438-457: The fenced code block starting with the unlabeled ```
before the "messageCreate → BaitChannelManager.handleMessage" flow should be
labeled to satisfy markdownlint MD040; update the opening fence from ``` to
```text (leave the block contents and closing fence unchanged) so the block is
recognized as a text code block—this affects the section containing
messageCreate, BaitChannelManager.handleMessage, analyzeSuspicion,
executeBanAction, raidModeManager.recordTrigger, sendDmNotification,
logToChannel, and logAction.
In `@src/commands/handlers/dataExport.ts`:
- Around line 141-155: The export configuration omits guild-scoped idempotency
records; add an export entry for IdempotencyKey so "export all guild data"
includes those records. Locate the array of export descriptors (the objects with
name/entity/buildFindOptions like the pendingActions entry) and add an object
with name: 'idempotencyKeys' (or 'IdempotencyKey' to match your naming
convention), entity: IdempotencyKey, and buildFindOptions: guildScoped so
idempotency records are exported alongside PendingAction, AnnouncementLog, and
AnnouncementTemplate.
In `@src/commands/handlers/dev/devSuiteScaffold.ts`:
- Around line 1016-1022: The teardown misses removing IdempotencyKey rows for
the guild, so add a delete call for IdempotencyKey alongside the existing
deletes (e.g., after the PendingAction delete). Specifically, call
AppDataSource.getRepository(IdempotencyKey).delete({ guildId }) (referencing the
IdempotencyKey entity) and capture its result (e.g., idempotencyResult) and
include its affected count in the final return string so scaffold/teardown
cycles are fully cleaned; keep the existing deletes for PendingAction, JoinEvent
and the channelsDeleted call intact.
In `@src/events/guildMemberRemove.ts`:
- Around line 72-75: The repository lookup for PendingAction currently swallows
failures (the .find(...).catch(() => [])) which breaks the leave-drain
guarantee; remove the empty catch and let errors surface or explicitly handle
them using enhancedLogger from src/utils (e.g., log the error with context
including guildId/userId) so failures are visible; likewise, ensure the deletion
path that removes pending rows (references to pendingRepo.delete /
pendingRepo.remove or similar) does not silently ignore errors—wrap delete calls
to catch exceptions and log them via enhancedLogger with identifying info, and
if appropriate propagate the error so callers can retry or fail loudly.
In `@src/events/ticket/create.ts`:
- Around line 85-101: The modal builder (buildCustomTicketModal) only adds the
first 5 entries from ticketType.customFields but the submit handler still
iterates the full ticketType.customFields array, causing fields beyond index 4
to be unread and required fields silently dropped; update the
submit/submit-reader logic to use the same capped fields set (e.g., const
fieldsToRead = ticketType.customFields.slice(0,5)) or centralize the slice into
a shared helper so both buildCustomTicketModal and the submit handler iterate
the identical fieldsToAdd/fieldsToRead; also ensure required-field validation
and error messages operate on that capped list so no required field past the cap
is expected.
In `@src/utils/api/handlers/baitChannelHandlers.ts`:
- Around line 308-309: Replace the inline numeric literals used for log
retention checks with centralized constants: add LOG_RETENTION_MIN_DAYS and
LOG_RETENTION_MAX_DAYS to src/utils/constants.ts, import them into
src/utils/api/handlers/baitChannelHandlers.ts, and replace the literal checks
(e.g., the retentionDays < 30 || retentionDays > 365) with comparisons against
those constants; do the same replacement for the other occurrences referenced
(around the checks at the locations corresponding to 413-414 and 448-449) so all
handlers use the shared constants and update any error messages to reference the
constants conceptually.
- Around line 265-346: Replace the hardcoded ApiError messages in
baitChannelHandlers (the logRetentionDays validation and the appealLinkBaseUrl
checks around config.enableAppealLink / config.appealLinkBaseUrl) with localized
strings from the centralized lang module: import the appropriate lang namespace,
add new keys (e.g. bait.logRetentionDaysRange, bait.appealLinkMustBeHttps,
bait.appealLinkInvalidUrl) to the lang files, and use those lang entries when
throwing ApiError.badRequest instead of the literal texts; ensure you update
both the retentionDays branch that throws 'logRetentionDays must be 30-365' and
the two places that throw 'appealLinkBaseUrl must be https' and
'appealLinkBaseUrl is not a valid URL'.
- Around line 337-347: When config.enableAppealLink is true (the block
validating config.appealLinkBaseUrl), also validate that the signing secret
exists and is strong: read process.env.APPEAL_HMAC_SECRET (or the app's env
accessor) and ensure it is present and at least 32 bytes/characters; if missing
or too short throw ApiError.badRequest with a clear message. Add this check in
the same conditional that checks config.appealLinkBaseUrl (near symbols
config.enableAppealLink, config.appealLinkBaseUrl and ApiError.badRequest) so
the handler won’t allow enabling appeal links without a valid APPEAL_HMAC_SECRET
before persisting the config.
- Around line 451-463: The handler reads userIdFilter from query and uses it
directly in the query `where` object (userIdFilter / where.userId) without
validating the Discord ID; call isValidSnowflake(userIdFilter) from
src/utils/api/helpers.ts and if it returns false respond with a 400 error (bad
request) before constructing `where`; only set `where.userId = userIdFilter`
when isValidSnowflake(...) is true. Keep the validation near the existing
extraction of userIdFilter in baitChannelHandlers.ts and ensure the response
uses the same error pattern used elsewhere in this file.
- Around line 412-418: The handler currently treats any unknown status as "all"
by falling through; change the validation for the status extracted from
urlObj.searchParams so only the supported values ('active', 'dead', 'all') are
allowed and return a 400 Bad Request for any other value. Keep the existing
logic that sets where.deadAt = IsNull() for 'active' and where.deadAt =
Not(IsNull()) for 'dead', and leave where unchanged for 'all'; locate the
status/where logic around the variables status, urlObj, where, IsNull() and
Not(IsNull()) and implement the explicit check that responds with a 400 error
(with a short message) when status is not one of the allowed strings.
In `@src/utils/baitChannel/baitChannelManager.ts`:
- Line 1553: Remove the dead standalone statement `embed;` in
baitChannelManager.ts — it has no effect and should be deleted; search for the
`embed` variable usages in the surrounding function (e.g., in the
baitChannelManager logic where embeds are constructed/returned) to ensure no
other references are affected and simply remove this stray expression to clean
up the code.
In `@src/utils/database/logCleanup.ts`:
- Around line 13-14: Replace the hardcoded DEAD_LETTER_RETENTION_DAYS = 30 in
logCleanup.ts with a centralized constant by adding
RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER = 30 to src/utils/constants.ts and
importing/using that constant in place of DEAD_LETTER_RETENTION_DAYS; also find
the similar hardcoded values referenced around lines 140-152 in this file and
replace them with the same RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER reference
to ensure all retention policy numbers are centralized.
---
Outside diff comments:
In `@tests/unit/utils/baitChannel/baitChannelManager.test.ts`:
- Around line 277-333: Silence the expected error logs in the two negative-path
tests by stubbing console.error for the duration of each test: in the "DB error
→ returns null and logs (does not throw)" test that calls (manager as
any).getConfig("guild-1") and in the "DB error → returns [] and logs (does not
throw)" test that calls (manager as any).getKeywords("guild-1"), add a
jest.spyOn(console, "error").mockImplementation(() => {}) at the start of the
test and restore it at the end with (console.error as
jest.SpyInstance).mockRestore() (or store the spy in a variable and call
spy.mockRestore()) so the logs are suppressed only for that test and restored
afterward.
---
Nitpick comments:
In `@src/commands/builders/baitChannel.ts`:
- Around line 264-281: Replace the hardcoded user-facing strings in the raid
subcommands with values from the centralized lang module: for the subcommands
built via the fluent builder (the .addSubcommand(...) blocks that call
.setName('status'|'enter'|'release')), change each .setDescription(...) argument
to the appropriate lang key (e.g., lang.baitChannel.builder.raid.statusDescrp,
lang.baitChannel.builder.raid.enterDescrp,
lang.baitChannel.builder.raid.releaseDescrp) and replace the option description
in the .addStringOption(...) call (option.setName('reason').setDescription(...))
with the corresponding lang.baitChannel.builder.raid.reasonDescrp key; ensure
you import the lang module if not already present and use the exact lang
property names used elsewhere in baitChannel.ts.
In `@src/commands/handlers/baitChannel/raid.ts`:
- Around line 25-28: Replace hardcoded user-facing strings in the raid command
handler by using the centralized lang module instead of direct literals;
specifically change calls like interaction.reply({ content: '❌ Raid mode manager
is not initialized (bot may still be starting up).', flags:
[MessageFlags.Ephemeral] }) to use lang lookups (e.g., content:
lang.get('raid.notInitialized') or lang.t('raid.notInitialized')) while keeping
interaction.reply and MessageFlags.Ephemeral untouched, and apply the same
replacement for the other hardcoded messages in the same file (the reply/error
messages around the other interaction.reply and interaction.editReply calls
referenced at the other locations), using keys that allow interpolation for any
dynamic values.
In `@src/events/auditLogEntryCreate.ts`:
- Around line 200-204: The empty catch around idempotencyRepo.save(claim) should
capture the thrown error (e.g., const err) and log a debug-level message when it
is a duplicate-key race rather than swallowing it silently; update the catch to
inspect the error (e.g., duplicate key code like 11000 or the DB-specific error
indicator) and call the debug logger with context such as the claim id/unique
key and the error message while preserving the current behavior of not
rethrowing non-fatal duplicate-key races.
In `@src/events/ticket/create.ts`:
- Around line 231-236: The setTimeout cleanup currently calls
interaction.message.delete() inside an empty catch which swallows errors;
replace that call with the repository's verified message-delete helper (e.g.,
verifiedMessageDelete or verifiedMessageDeleteById) so deletions are checked and
failures are surfaced, and remove the empty catch block so the helper can
log/handle permission or unknown-message errors as intended (locate the
setTimeout block that calls interaction.message.delete and swap to the verified
helper, passing the message or message id and the logger/context used
elsewhere).
- Around line 103-109: The placeholder and label strings in the TextInputBuilder
(methods setLabel and setPlaceholder on the object created in createTicket flow)
are hardcoded; replace them with calls into the centralized lang translation
module (e.g., lang.t(...) or the project’s lang(...) helper) so the label
'Please describe your issue' and the default placeholder 'Provide details about
your ticket...' are localized, and ensure ticketType.description is passed
through lang or used as a fallback translation key rather than raw text; apply
the same replacement to the other hardcoded user-facing strings referenced
around the later ticket creation code (the strings noted at the other
occurrence).
In `@src/utils/api/handlers/baitChannelHandlers.ts`:
- Around line 353-355: Replace the ad-hoc cast to ClientWithBaitManager with the
shared ExtendedClient type: locate where baitManager is obtained (currently via
(client as ClientWithBaitManager).baitChannelManager) and instead cast or type
the client as ExtendedClient and access client.baitChannelManager; then call
clearConfigCache(guildId) on that manager (i.e., use (client as
ExtendedClient).baitChannelManager.clearConfigCache(guildId) or update the
handler signature to accept client: ExtendedClient so you can directly call
client.baitChannelManager.clearConfigCache(guildId)). Ensure you update any
import/type references to ExtendedClient and remove the ClientWithBaitManager
cast.
In `@src/utils/baitChannel/appealToken.ts`:
- Line 27: DEFAULT_EXP_HOURS is defined inline in appealToken.ts but per
guidelines it must live in the shared constants module; remove the local const
DEFAULT_EXP_HOURS from appealToken.ts, add it to src/utils/constants.ts
(exported as DEFAULT_EXP_HOURS or similarly named constant), and update
appealToken.ts to import { DEFAULT_EXP_HOURS } from the constants module and use
the imported value wherever DEFAULT_EXP_HOURS was referenced (e.g., in functions
that compute token expiry).
- Around line 77-85: The getSecret function currently enforces a minimum length
of 16 but its error message recommends 32+ bytes; change the minimum check in
getSecret to require at least 32 bytes (e.g., if (!secret || secret.length <
32)) and update the thrown Error text to consistently recommend 32+ bytes so the
validation and message align for APPEAL_HMAC_SECRET.
In `@src/utils/baitChannel/baitChannelManager.ts`:
- Line 666: Replace the hard-coded 5_000 ms DM timeout in the Promise (the
setTimeout(() => reject(new Error('dm_timeout')), 5_000) expression) with a
named constant exported from src/utils/constants.ts; add/ensure an exported
DM_TIMEOUT_MS (or similar) there and import it into
src/utils/baitChannel/baitChannelManager.ts, then use that constant in the
Promise so the timeout value is centralized and configurable.
In `@src/utils/baitChannel/contentBurstDetector.ts`:
- Around line 28-29: Extract MAX_ENTRIES_PER_USER, CLEANUP_INTERVAL_MS and the
5-minute idle timeout literal from contentBurstDetector.ts and place them into
the shared constants module (constants.ts) as exported named constants; then
import and use those constants in contentBurstDetector (replace the
literals/usages). Ensure the exported constant names match MAX_ENTRIES_PER_USER,
CLEANUP_INTERVAL_MS and IDLE_TIMEOUT_MS (or a similar clear name) and update any
references in functions or timers that used the original values.
In `@src/utils/baitChannel/raidModeManager.ts`:
- Line 38: RAID_MODE_MAX_DURATION_MS is a magic number and must be moved to the
central constants file; add a new exported constant named
RAID_MODE_MAX_DURATION_MS (value 4 * 60 * 60 * 1000) in src/utils/constants.ts,
export it, then replace the local declaration in raidModeManager.ts with an
import of RAID_MODE_MAX_DURATION_MS from the constants module and remove the
local constant; ensure any references in raidModeManager.ts use the imported
symbol.
- Around line 78-79: The current pruning loop repeatedly calling
guildTriggers.shift() is inefficient for large bursts; replace it by finding the
first index that should remain (use guildTriggers.findIndex(t => t.at >=
cutoff)) and then remove the prefix in one operation (either
guildTriggers.splice(0, idx) if idx >= 0 or set guildTriggers = [] when all are
pruned). Update the code that references guildTriggers (the pruning block around
the while loop) to use this single-splice or slice-rebuild approach so pruning
becomes O(n) instead of O(n²).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 404266b9-9f10-46ea-bf95-b0161d90873e
⛔ Files ignored due to path filters (1)
public/scorecard.pngis excluded by!**/*.png,!public/**
📒 Files selected for processing (36)
.env.exampleCHANGELOG.mdCLAUDE.mdpackage.jsonsrc/commands/builders/baitChannel.tssrc/commands/handlers/baitChannel/index.tssrc/commands/handlers/baitChannel/raid.tssrc/commands/handlers/dataExport.tssrc/commands/handlers/dev/devSuiteScaffold.tssrc/events/auditLogEntryCreate.tssrc/events/guildMemberRemove.tssrc/events/ticket/create.tssrc/index.tssrc/typeorm/entities/bait/BaitChannelConfig.tssrc/typeorm/entities/bait/BaitChannelLog.tssrc/typeorm/entities/bait/IdempotencyKey.tssrc/typeorm/entities/bait/PendingAction.tssrc/typeorm/index.tssrc/typeorm/migrations/1774000011000-BaitChannelV3Schema.tssrc/utils/api/handlers/baitChannelHandlers.tssrc/utils/api/helpers.tssrc/utils/baitChannel/appealToken.tssrc/utils/baitChannel/auditReason.tssrc/utils/baitChannel/baitChannelManager.tssrc/utils/baitChannel/banExecutor.tssrc/utils/baitChannel/contentBurstDetector.tssrc/utils/baitChannel/raidModeManager.tssrc/utils/baitChannel/retryQueue.tssrc/utils/database/guildQueries.tssrc/utils/database/logCleanup.tstests/unit/events/ticketInteraction.test.tstests/unit/utils/baitChannel/appealToken.test.tstests/unit/utils/baitChannel/auditReason.test.tstests/unit/utils/baitChannel/baitChannelManager.test.tstests/unit/utils/baitChannel/banExecutor.test.tstests/unit/utils/baitChannel/contentBurstDetector.test.ts
Comment on lines
+337
to
+347
| if (config.enableAppealLink && config.appealLinkBaseUrl) { | ||
| try { | ||
| const url = new URL(config.appealLinkBaseUrl); | ||
| if (url.protocol !== 'https:') { | ||
| throw ApiError.badRequest('appealLinkBaseUrl must be https'); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof ApiError) throw error; | ||
| throw ApiError.badRequest('appealLinkBaseUrl is not a valid URL'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Block enableAppealLink when APPEAL_HMAC_SECRET is missing/weak.
This endpoint validates URL format but still allows persisting enableAppealLink=true without enforcing the signing secret requirement, which can break appeal-link generation at runtime.
Suggested fix
if (config.enableAppealLink && config.appealLinkBaseUrl) {
+ const secret = process.env.APPEAL_HMAC_SECRET;
+ if (!secret || secret.length < 32) {
+ throw ApiError.badRequest('APPEAL_HMAC_SECRET must be set to at least 32 bytes when enableAppealLink=true');
+ }
try {
const url = new URL(config.appealLinkBaseUrl);
if (url.protocol !== 'https:') {
throw ApiError.badRequest('appealLinkBaseUrl must be https');
}Based on learnings: "APPEAL_HMAC_SECRET (32+ bytes, required only when BaitChannelConfig.enableAppealLink=true)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (config.enableAppealLink && config.appealLinkBaseUrl) { | |
| try { | |
| const url = new URL(config.appealLinkBaseUrl); | |
| if (url.protocol !== 'https:') { | |
| throw ApiError.badRequest('appealLinkBaseUrl must be https'); | |
| } | |
| } catch (error) { | |
| if (error instanceof ApiError) throw error; | |
| throw ApiError.badRequest('appealLinkBaseUrl is not a valid URL'); | |
| } | |
| } | |
| if (config.enableAppealLink && config.appealLinkBaseUrl) { | |
| const secret = process.env.APPEAL_HMAC_SECRET; | |
| if (!secret || secret.length < 32) { | |
| throw ApiError.badRequest('APPEAL_HMAC_SECRET must be set to at least 32 bytes when enableAppealLink=true'); | |
| } | |
| try { | |
| const url = new URL(config.appealLinkBaseUrl); | |
| if (url.protocol !== 'https:') { | |
| throw ApiError.badRequest('appealLinkBaseUrl must be https'); | |
| } | |
| } catch (error) { | |
| if (error instanceof ApiError) throw error; | |
| throw ApiError.badRequest('appealLinkBaseUrl is not a valid URL'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/api/handlers/baitChannelHandlers.ts` around lines 337 - 347, When
config.enableAppealLink is true (the block validating config.appealLinkBaseUrl),
also validate that the signing secret exists and is strong: read
process.env.APPEAL_HMAC_SECRET (or the app's env accessor) and ensure it is
present and at least 32 bytes/characters; if missing or too short throw
ApiError.badRequest with a clear message. Add this check in the same conditional
that checks config.appealLinkBaseUrl (near symbols config.enableAppealLink,
config.appealLinkBaseUrl and ApiError.badRequest) so the handler won’t allow
enabling appeal links without a valid APPEAL_HMAC_SECRET before persisting the
config.
Comment on lines
+13
to
+14
| /** Dead-lettered pending actions kept for mod review for 30d before purge. */ | ||
| const DEAD_LETTER_RETENTION_DAYS = 30; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move dead-letter retention to shared constants.
Line 14 hardcodes a policy value (30) that should live in src/utils/constants.ts with the other retention settings.
Proposed patch
-/** Dead-lettered pending actions kept for mod review for 30d before purge. */
-const DEAD_LETTER_RETENTION_DAYS = 30;-const deadLetterCutoff = new Date(Date.now() - DEAD_LETTER_RETENTION_DAYS * 24 * 60 * 60 * 1000);
+const deadLetterCutoff = new Date(
+ Date.now() - RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER * 24 * 60 * 60 * 1000,
+);-`Log cleanup: removed ${pendingResult.affected} dead-lettered pending actions older than ${DEAD_LETTER_RETENTION_DAYS} days`,
+`Log cleanup: removed ${pendingResult.affected} dead-lettered pending actions older than ${RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER} days`,// src/utils/constants.ts (add)
RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER = 30;As per coding guidelines: “All magic numbers and configuration constants must be centralized in src/utils/constants.ts.”
Also applies to: 140-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/database/logCleanup.ts` around lines 13 - 14, Replace the hardcoded
DEAD_LETTER_RETENTION_DAYS = 30 in logCleanup.ts with a centralized constant by
adding RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER = 30 to src/utils/constants.ts
and importing/using that constant in place of DEAD_LETTER_RETENTION_DAYS; also
find the similar hardcoded values referenced around lines 140-152 in this file
and replace them with the same RETENTION_DAYS.PENDING_ACTION_DEAD_LETTER
reference to ensure all retention policy numbers are centralized.
14 findings across the two PR-review bots. All addressed:
## Copilot (5 findings)
1. **`drainPendingBaitActions` dropped retries on `queued` status**
(guildMemberRemove.ts). The leave-drain was unconditionally removing
the pending_actions row even when the executor returned `queued` —
the executor doesn't enqueue automatically, so the retry was silently
lost. Now hands the row to `getRetryQueue().enqueue(...)` and leaves
the row alive when status is `queued`.
2. **`claimIdempotencyKey` treated transient DB errors as duplicates**
(banExecutor.ts). A real DB failure would return `{claimed: false}`
indistinguishable from a UNIQUE-constraint hit, and `executeBanAction`
silently returned `status: 'duplicate'` — moderation skipped on any
DB hiccup. Now returns `{claimed: false, reason: 'duplicate'|'db_error'}`
and the caller routes `db_error` to `status: 'queued'` for retry.
3. **`Promise.race` unhandled rejection on DM timeout**
(baitChannelManager.ts). When the 5s timeout won, the underlying
`member.send()` promise could still reject later and trigger an
unhandled rejection. Now captures the send promise locally, attaches
a swallow-catch on the race loser, and clears the timeout regardless
of outcome.
4. **PATCH config couldn't clear nullable string fields** (helpers.ts +
baitChannelHandlers.ts). `optionalString` collapsed null/empty/absent
into `undefined`, making it impossible to null-out `logChannelId`,
`summaryChannelId`, `appealLinkBaseUrl`, etc. Added
`optionalNullableString` that distinguishes "absent" (undefined,
skip) from "explicitly clear" (null/"" → null). PATCH handler now
splits string fields into nullable vs non-nullable lists and uses
the right helper per field.
5. **`handleModSupersedes` silently untracked grace-period cases**
(auditLogEntryCreate.ts). The comment promised "write a new row with
minimal metadata" when no existing BaitChannelLog row was present,
but the code only updated existing `queued`/`failed` rows. Result:
mod-supersedes-us in the grace-period scenario (the most common
case) wrote nothing to bait_channel_logs. Now inserts a new row
with `actionTaken='superseded-by-mod'`, `executorId`,
`discordAuditLogId`, `actionConfirmedAt`. Also expands the updatable-
state set to include `'logged'`.
## CodeRabbit (9 findings addressed; refactor-only suggestions deferred)
6. **Repository errors silently suppressed in leave-drain**
(guildMemberRemove.ts). Removed both `.catch(() => [])` and
`.catch(() => {})` — DB errors now propagate to the outer
guildMemberRemove try/catch which already logs to enhancedLogger.
7. **`enableAppealLink` could be enabled without secret**
(baitChannelHandlers.ts). Config PATCH now refuses to set
`enableAppealLink=true` if `appealLinkBaseUrl` is unset OR if
`APPEAL_HMAC_SECRET` env is missing/short. Prevents the runtime
failure mode where appeal-link DMs silently degrade.
8. **`IdempotencyKey` missing from data export** (dataExport.ts).
Added to EXPORT_ENTITIES descriptor so `/data-export` covers the
full v3.2.0 bait surface for GDPR compliance.
9. **`IdempotencyKey` missing from devSuite teardown**
(devSuiteScaffold.ts). Added to bait teardown so subsequent scaffold
cycles don't see stale idempotency rows.
10. **`status` query param falls through to `all` on invalid input**
(baitChannelHandlers.ts, /pending-actions). Now returns 400 for
values outside `{active, dead, all}`.
11. **`userId` query filter wasn't validated** (baitChannelHandlers.ts,
/logs). Now rejects with 400 if not a valid Discord snowflake.
12. **Two dead `embed;` no-op statements** (baitChannelManager.ts,
lines 1782 and 1908). Removed.
13. **`.env.example` `APPEAL_HMAC_SECRET` doc conflicted with CHANGELOG**.
The example said "Bot will refuse to start"; CHANGELOG + tests say
"silently omitted". Code path matches CHANGELOG — startup is fine,
only the appeal-link DM field gets omitted. Updated example to
match the actual behavior, and noted the webapp-side guard that
prevents enabling `enableAppealLink` while the secret is missing.
14. **CLAUDE.md fenced code block missing language tag** (CLAUDE.md:438).
Added `text` so markdownlint's MD040 passes.
## Deferred (refactor suggestions, not bugs)
- "Move newly added API messages to `lang`" — large refactor; existing
API handlers use hardcoded English. Will roll up with a broader
localization pass.
- "Move inline numeric limits to constants" — applies to many handlers
beyond v3.2.0; out of scope.
- "Move `DEAD_LETTER_RETENTION_DAYS` to constants" — single-use module-
level const. Will move if we add more bait-internal constants later.
- "buildCustomTicketModal 5-field cap" — this is from PR #3
(prod-bug branch we inherited), not v3.2.0 code. Will surface there.
Verification:
- `bun test` — 1203 pass / 0 fail.
- `bun run check` — biome clean across 513 files.
- `bun run build` — tsc clean.
Owner
Author
|
@coderabbitai review All 14 findings from the initial review have been addressed in 89e75ba (or earlier commit chain). Summary in the commit message. Please re-review to confirm — particularly the Copilot finding around idempotency claim DB-error handling (banExecutor.ts) and the nullable-string PATCH config flow. Also pinging @copilot-pull-request-reviewer for a re-pass. |
🧩 Analysis chain
Re-review requested by
🏁 Script executed: Length of output: 32810
✅ Idempotency claim DB-error handling (
|
| Outcome | Meaning | Caller handling |
|---|---|---|
{ claimed: true } |
Slot owned — proceed | Calls Discord API |
{ claimed: false, reason: 'duplicate', existing } |
UNIQUE constraint — someone beat us | Returns status: 'duplicate' |
{ claimed: false, reason: 'db_error' } |
DB unavailable | Returns status: 'queued' |
The old bug (save failure → treat as duplicate → silently skip enforcement) is gone. The tiebreaker findOne correctly distinguishes a constraint race from a transient DB outage, and a db_error now routes to the retry queue instead of a silent skip. Both the findOne-also-fails branch and the save-failed-but-no-row branch log at HIGH severity.
✅ Nullable-string PATCH config flow (baitChannelHandlers.ts)
The fix is clean and well-documented inline. The fields are now explicitly split:
- Non-nullable (
banReason,warningMessage,actionType) →optionalString— can only set or skip, never clears tonull - Nullable (
appealInfo,logChannelId,summaryChannelId,raidModeAlertRoleId,appealLinkBaseUrl) →optionalNullableString— three-way semantics:undefined= skip,null= clear,string= set
The appeal-link safety gate (HTTPS enforcement + APPEAL_HMAC_SECRET length ≥ 16 pre-save check) is also in place.
⚠️ New minor finding — softban unban retry is unreachable after key is claimed
This is a structural gap, not one of the original 14, but it's worth noting now:
In the softban case, if the ban step succeeds but the unban step fails with a retryable error (rate limit, 5xx), the executor returns status: 'queued'. However, the idempotency key for action='softban' was already claimed before the ban step ran. When the retry queue picks this up and calls executeBanAction again, claimIdempotencyKey finds the existing row and returns status: 'duplicate' — the unban is never attempted again, leaving the user permanently banned.
The terminal-failure path correctly logs a warn, but the retryable path returns silently without any warning before propagating queued:
// softban unban step — retryable path returns with no log:
if (!isTerminalDiscordError(removeError)) {
return {
status: 'queued',
action,
failureReason: `softban unban step failed: ${(removeError as Error).message}`,
// ← no enhancedLogger.warn here; retry will silently duplicate-skip
};
}Suggested mitigations (pick one or both):
- Add a
logError/enhancedLogger.warnbefore thequeuedreturn so operators are alerted that the unban is stuck. - Model the softban unban step as a separate idempotency action (e.g.
action='softban-unban') so it can be retried independently of the ban step.
Would you like to track this as a follow-up issue, or address it in this PR?
✅ Actions performed
Full review triggered.
Agent-Logs-Url: https://github.com/NindroidA/cogworks-bot/sessions/11810d90-c4f5-4315-a9cf-8a99bf33bcd4 Co-authored-by: NindroidA <88803583+NindroidA@users.noreply.github.com>
Agent-Logs-Url: https://github.com/NindroidA/cogworks-bot/sessions/11810d90-c4f5-4315-a9cf-8a99bf33bcd4 Co-authored-by: NindroidA <88803583+NindroidA@users.noreply.github.com>
Re-pass complete: both items are already addressed in
|
…m restore
Two adversarially-verified correctness bugs in the v3.2.0 bait subsystem
(not yet released, so prod 3.1.41 is unaffected).
1. ContentBurstDetector cross-guild leak (contentBurstDetector.ts): the sliding
window was keyed by userId ALONE. Since channel IDs are globally-unique
snowflakes, a user posting identical content in bait channels of two
different guilds the bot serves shared ONE window and could trip a burst in
guild B off guild A's posts — violating the multi-server isolation rule.
Now keyed by `${guildId}:${userId}`. recordMessage() gains a guildId first
arg; caller passes message.guild.id. +1 regression test (cross-guild
isolation); corrected the stale header comment (detector runs only for
messages IN configured bait channels, not "every channel").
2. Raid-mode permission restore (raidModeManager.ts): release blindly set
@everyone SendMessages to null (inherit), wiping any pre-existing EXPLICIT
`@everyone SendMessages: true` overwrite — so in deny-by-default servers
those channels stayed silently locked after release. Now snapshots each
channel's prior tri-state (allow/deny/inherit) at lockdown and restores it
exactly on release; missing snapshot (bot restarted mid-raid) falls back to
inherit + a loud warn. Skips channels already explicitly denied.
Verified: tsc clean, biome clean, bun test 1204/0 (bait suite 211/0).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/baitChannel/banExecutor.ts (1)
242-261:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftSplit softban unban retries from the original
softbanidempotency key.If
guild.bans.create()succeeds andguild.bans.remove()fails retryably, this returnsqueuedwithaction === 'softban'. The retry path will hit Line 202 again, see the already-claimedsoftbankey, and exit asduplicate, so the unban step never runs and the user can remain banned indefinitely. Track the second phase separately (for examplesoftban-unban) or persist a step marker so retries can resume at the unban phase instead of re-claiming the original action.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/baitChannel/banExecutor.ts` around lines 242 - 261, The current softban flow uses the same idempotency key for both create and remove steps, so when guild.bans.create succeeds but guild.bans.remove fails and returns queued with action === 'softban', retries re-claim the original softban key and exit as duplicate, preventing the unban step from running; fix this by splitting the second phase into its own idempotency key (e.g., "softban-unban") or by persisting a step marker after successful guild.bans.create so retries detect that the create step completed and only attempt guild.bans.remove, update the code paths that set/claim the idempotency key and the retry-return payload (where action is set to 'softban') to use the new key or marker, and ensure the retry handler recognizes and executes the unban phase (calling guild.bans.remove) instead of treating it as a duplicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/baitChannel/banExecutor.ts`:
- Around line 242-261: The current softban flow uses the same idempotency key
for both create and remove steps, so when guild.bans.create succeeds but
guild.bans.remove fails and returns queued with action === 'softban', retries
re-claim the original softban key and exit as duplicate, preventing the unban
step from running; fix this by splitting the second phase into its own
idempotency key (e.g., "softban-unban") or by persisting a step marker after
successful guild.bans.create so retries detect that the create step completed
and only attempt guild.bans.remove, update the code paths that set/claim the
idempotency key and the retry-return payload (where action is set to 'softban')
to use the new key or marker, and ensure the retry handler recognizes and
executes the unban phase (calling guild.bans.remove) instead of treating it as a
duplicate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59eac5a1-709d-425d-9276-86753107301e
📒 Files selected for processing (13)
.env.exampleCLAUDE.mdsrc/commands/handlers/dataExport.tssrc/commands/handlers/dev/devSuiteScaffold.tssrc/events/auditLogEntryCreate.tssrc/events/guildMemberRemove.tssrc/utils/api/handlers/baitChannelHandlers.tssrc/utils/api/helpers.tssrc/utils/baitChannel/baitChannelManager.tssrc/utils/baitChannel/banExecutor.tssrc/utils/baitChannel/contentBurstDetector.tssrc/utils/baitChannel/raidModeManager.tstests/unit/utils/baitChannel/contentBurstDetector.test.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/unit/utils/baitChannel/contentBurstDetector.test.ts
- src/commands/handlers/dev/devSuiteScaffold.ts
- src/utils/api/helpers.ts
- src/commands/handlers/dataExport.ts
- src/events/guildMemberRemove.ts
- src/events/auditLogEntryCreate.ts
- src/utils/baitChannel/raidModeManager.ts
- src/utils/api/handlers/baitChannelHandlers.ts
- src/utils/baitChannel/baitChannelManager.ts
The two completely-untested bait modules now have behavioral coverage — including the raid-mode permission snapshot/restore fix from the previous commit. Shrinks the v3.2.0 manual smoke-test checklist. - raidModeManager.test.ts (+14): getStatus active/inactive; recordTrigger threshold entry + enableRaidMode no-op; releaseRaidMode/checkAutoRelease; and the channel permission snapshot/restore matrix — release restores an explicit @everyone SendMessages:true (not inherit), restores null for no-prior-overwrite channels, skips already-denied channels, never touches exempt (log/summary/bait) channels, and falls back to inherit when the snapshot is missing (restart mid-raid). Covers checklist §8. - retryQueue.test.ts (+10): enqueue fresh-row/increment/dead-letter backoff math; per-tick lifecycle (executed/duplicate → remove, queued → attempts++ + backoff, failed or MAX_ATTEMPTS → dead-letter, orphaned-grace promotion, guild-gone → terminal). Covers checklist §3. - retryQueue.ts: executeBanAction is now injectable via deps (default = real) so the lifecycle tests drive outcomes without mock.module() — which is process-shared on bun and would poison the sibling banExecutor suite (the closeWorkflow lesson). Production wiring unchanged. Full suite 1228/0 (bait suite 211 → 235). Build + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scripts/bait-inspect.ts dumps a guild's (optionally a single user's) full bait state from the DB in one command: config + raid status, recent BaitChannelLog rows with every audit column (dmSent, dmFailureReason, logDeliveryFailed, discordAuditLogId, actionConfirmedAt, unbannedAt/By, detection flags, suspicionScore), pending_actions (attempts/deadAt/expiresAt), and idempotency_keys. Turns the v3.2.0 smoke-test checklist's manual "capture DB state" steps (§2/§3/§4/§5/§6/§8/§10/§12) into `bun run scripts/bait-inspect.ts <guildId> [userId]`, so the only thing done by hand is the irreducible live Discord action (post a bait message, get banned, receive a DM). Read-only; follows the existing scripts/verifyGuildIsolation.ts AppDataSource pattern. tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…route (§6, §11) - auditLogEntryCreate.test.ts (+11): drives the real registered handler across all three paths — bot-self confirm (patches discordAuditLogId/actionConfirmedAt, idempotent, ignores non-bait actions), mod-supersedes (claims idempotency key with the mod's executor, cancels pending actions, updates queued log / inserts minimal row / leaves already-actioned rows alone), and unban tracking — plus the MemberUpdate timeout-set filter and missing-target/executor guards. Covers checklist §6. - baitChannelHandlers.test.ts (+11): the inspection surface the smoke test + bait-inspect harness rely on — GET config, POST config/update (apply + save + audit + patched keys, 404, range validation), GET pending-actions (active/dead/all + invalid status), GET logs (filters + invalid-snowflake reject), GET raid-mode/status (uninitialized-manager default). Covers §11. Both use the AppDataSource.getRepository patch with stable fakes mutated per test (lazyRepo caches the repo on first access — swapping per-test is a no-op). - baitChannelHandlers.ts: the dead `PATCH /bait-channel/config` (unreachable — the internal API method gate only allows GET/POST/DELETE) is re-homed to the reachable `POST /bait-channel/config/update`. ninsys-api writes bait config via direct DB, so nothing called the old PATCH route — safe rename. Full suite 1250/0 (×2 deterministic). Build + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
v3.2.0 branched before the 3.1.41→3.1.42 prod fixes, so it still carried the old closeWorkflow/ticketHandlers (the bun mock.module poison that fails CI on Linux) and the old archive logic. Brought the fixed files forward from main so PR #4's CI is green and v3.2.0 ships with the same correctness fixes prod has: - closeWorkflow.ts / ticketHandlers.ts (+ their tests): dependency-injection refactor that killed the flaky CI (no mock.module on a shared SUT module) + the archive namespace isolation (isEmailTicket discriminator) + no-ping transcripts (allowedMentions parse:[]). - application/closeWorkflow.ts + fetchAllMessages.ts: allowedMentions on the application archive sends + cleanContent for readable, non-pinging mentions. Files taken verbatim from origin/main (v3.2.0's copies were the pre-fix versions; bait work never touched them). Version stays 3.2.0. Full suite 1251/0. Build + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…finement # Conflicts: # CHANGELOG.md # CLAUDE.md # package.json
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
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.
Summary
Major refinement of the bait channel (honeypot) subsystem. Closes ~22 audited edge-case gaps and ships industry-standard moderation patterns. Depends on #3 (prod-bug fixes) being merged first — this branch was started from prod-bugs to inherit the v3.1.41
logActionnull-tolerance fix, so when #3 merges to main this PR's diff will simplify to just the v3.2.0 commits.What's new
Lifecycle hardening
guild.bans.create(userId)) replacesmember.ban()— leave-tolerant, works after user partial is evicted.(guildId, userId, action, dayBucket)prevent double-execution across mod-vs-bot race + retry queue.auditLogEntryCreatelistener (requires newGuildModerationintent) for real-time bot-self attribution + mod-supersedes-us detection + unban tracking.guildMemberRemovedrains pending bait actions via REST executor —timeoutandkickdemote tosoftbanfor non-members.Raid Mode (new feature)
@everyone SendMessages: falseon non-staff channels + mod alert embed./baitchannel raid release.Detection improvements
crossChannelBurstflag.Observability
BaitChannelLog.dmSent+dmFailureReason(closed / no_shared_guild / timeout / unknown). 5s timeout race prevents the action path from hanging.logDeliveryFailedflag on the log row.discordAuditLogId+actionConfirmedAtpopulated automatically.unbannedAt+unbannedByfor false-positive analytics.Schema (migration
1774000011000-BaitChannelV3Schema)pending_bans→pending_actions+ addaction/attempts/lastError/deadAtcolumns.idempotency_keystable (UNIQUE(guildId, userId, action, dayBucket), TTL via expiresAt).bait_channel_logscolumns + 10 newbait_channel_configscolumns.API (closes the config CRUD gap)
GET/PATCH /bait-channel/config— full config read/write through the bot's internal API (previously dashboards relied on ninsys-api direct DB).GET /bait-channel/raid-mode/status,POST /raid-mode/{enter,release}.GET /bait-channel/pending-actions?status=…,POST .../cancel.GET /bait-channel/logs?days=&action=&userId=&overridden=.Slash commands
/baitchannel raid {status|enter|release}group.BREAKING
APPEAL_HMAC_SECRET— required only when any guild hasenableAppealLink=true. Without it, signed appeal links are silently omitted from DMs (staticappealInfotext still shows). Generate vianode -e 'console.log(require(\"crypto\").randomBytes(32).toString(\"base64url\"))'. Documented in.env.example.GuildModerationadded to client config — required forauditLogEntryCreate. No action needed for existing deployments (intent is non-privileged, gateway accepts it automatically).Test plan
bun testpasses locally — 1203 / 0 fail (+32 new tests).bun run checkclean — biome passes across 513 files.bun run buildclean — tsc passes..plans/2026-05-16/03-v3.2.0-smoke-test-checklist.md(gitignored locally; reproduced inline in PR comments below if needed).RELEASE=dev bun run migration:runagainst a fresh dev DB clone.Cross-repo work to follow
/appeal?token=…consumer route that verifies HMAC and auto-opens abanAppealticket.Plan docs (gitignored locally) cover the cross-repo coordination in detail.
Review history
This branch was reviewed pre-PR via:
TOKEN_ISSUERconstant name false-positive) addressed in 444b20e.unbannedAt: undefinedno-op filter →IsNull()kickpending actions silently lost on leave → demote to softbancancelGraceForUserhelper10013(Unknown Role) in terminal error set → removedrestoreActiveLockdowns()on bootLooking forward to CodeRabbit + Copilot for the nit pass.
🤖 Generated with Claude Code
Summary by CodeRabbit