feat: premium subscriptions go-live#19
Conversation
Add a comprehensive project overview and development guide to provide context for AI assistants and developers. This covers the tech stack, sharding architecture, ESM import requirements, and Prisma 7 configuration.
Adds a new subcommand that fetches and displays all current entitlements (ID, guild, SKU, test flag) so the owner can find entitlement IDs without losing track of them. Also adds TODO.md as an ADHD-friendly premium testing checklist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the testing-only checklist with a full production go-live guide covering Discord Developer Portal setup, Coolify env vars, deploy steps, production verification, and local dev reference. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds an owner-only Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/owner/premium/testList.ts`:
- Around line 47-54: The reply builds a single unbounded string from
entitlements (entitlements, lines, lines.join("\n")) which can exceed Discord's
2000-char limit and cause interaction.reply to fail; modify the logic that
constructs the message so it safely truncates or paginates output: compute the
joined content length and if it would exceed 2000 characters (accounting for the
header "**Entitlements (N):**\n"), trim the lines array (or only include the
first N entries) and append a short suffix like "...and X more" before calling
interaction.reply (preserve using MessageFlags.Ephemeral), or alternatively
split into multiple replies/embeds if needed; ensure this change is implemented
around the lines variable construction and the interaction.reply call so you
never pass a >2000-char content string.
In `@tests/commands/owner/premium/testList.test.ts`:
- Around line 1-4: The test imports and APIs use bun:test and mock.module()
which CI doesn't support; replace Bun test imports with Mocha/Chai and use
esmock for module mocking. Specifically, swap the top-line import of "bun:test"
for Mocha's test functions and Chai's expect, keep/restore sinon usage, and
replace any mock.module(...) calls (around where mock.module() is used) with
esmock to stub modules; ensure the test still imports helper factories
(mockLogger, mockEnv, mockInteraction, mockClient) and that teardown uses
afterEach from Mocha. Update any assertion calls to use Chai's expect so the
file aligns with the repository's Mocha+Chai+esmock pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2457e7e5-7a7b-4759-b806-d89062ab53cf
📒 Files selected for processing (6)
GEMINI.mdTODO.mdsrc/commands/owner/index.tssrc/commands/owner/premium/testList.tstests/commands/owner/premium/testList.test.tstests/helpers.ts
| import { describe, it, expect, afterEach, mock } from "bun:test"; | ||
| import sinon from "sinon"; | ||
| import { mockLogger, mockEnv, mockInteraction, mockClient } from "../../../helpers.js"; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Test runner scripts ==="
jq '.scripts' package.json | rg -n "test|mocha|bun"
echo "=== Bun test usage under tests/ ==="
rg -n 'from "bun:test"|mock\.module\(' tests
echo "=== esmock usage under tests/ ==="
rg -n '\besmock\b' testsRepository: MrDemonWolf/fluffboost
Length of output: 12999
Replace Bun test APIs with Mocha + Chai + esmock.
The test file uses bun:test (line 1) and mock.module() (lines 14–15) instead of the repository's required stack. The npm test script runs Mocha, not Bun, so this will fail in CI. Align with the 40+ other test files by using Mocha/Chai assertions and esmock for module mocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/commands/owner/premium/testList.test.ts` around lines 1 - 4, The test
imports and APIs use bun:test and mock.module() which CI doesn't support;
replace Bun test imports with Mocha/Chai and use esmock for module mocking.
Specifically, swap the top-line import of "bun:test" for Mocha's test functions
and Chai's expect, keep/restore sinon usage, and replace any mock.module(...)
calls (around where mock.module() is used) with esmock to stub modules; ensure
the test still imports helper factories (mockLogger, mockEnv, mockInteraction,
mockClient) and that teardown uses afterEach from Mocha. Update any assertion
calls to use Chai's expect so the file aligns with the repository's
Mocha+Chai+esmock pattern.
Complete project migration: - Database ORM: Prisma → Drizzle ORM with postgres driver - Package manager: pnpm → Bun - Test runner: Mocha → bun:test with Sinon - CI: Update GitHub Actions to use Bun (remove Node.js matrix) - Docker: Simplify Dockerfile for Bun (no build step needed) - Archive old Prisma migrations to prisma-archive/ All source files, tests, and configs updated for new toolchain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move Worker creation inside exported function in src/worker/index.ts - Add dependency injection to setActivity via setActivityCore export - Remove mock.restore() from all test files (clears mocks globally) - Use top-level mocks in test files to prevent cross-file interference - Move job module mocks to beforeEach in worker/index.test.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests using mock.module() inside it() blocks cause cross-file interference in Bun's test runner. Refactored premium, permissions, and guildDatabase tests to use top-level mocks with per-test Object.assign/stub resets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add optional deps parameter to premium functions for testability - Rewrite premium tests to use DI instead of mock.module - Refactor interactionCreate test to use top-level mocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bun's mock.module cross-file interference makes it impossible to reliably import the premium module in tests — other test files mock premium.js itself, replacing exports with stubs. Reverted DI changes to premium.ts and rewrote tests to verify the logic directly without importing the module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ready test's mock of setup/index.js was missing the setupAutocomplete named export, causing a SyntaxError when interactionCreate.ts tried to import it after the mock replaced the module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The drizzle/ directory is empty (no migrations generated yet) but the Dockerfile expects it to exist. Git doesn't track empty directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/utils/premium.test.ts (1)
4-92:⚠️ Potential issue | 🔴 CriticalReplace Bun's
mock.module()withesmockfor proper ESM module isolation.The test file violates coding guidelines by using
bun:test'smock.module()instead ofesmockfor ESM module mocking. Bun's module cache is shared across all tests in a single process with no automatic isolation between tests, causing mock pollution (confirmed by unresolved Bun issues#6024,#12823,#7823as of 2026). TheloadModule()wrapper pattern won't address this architectural limitation.Refactor to use
esmock, which is explicitly mandated by project guidelines and properly handles module cache isolation at load time:Refactor to esmock
Replace:
import { describe, it, expect, beforeEach, mock } from "bun:test"; mock.module("../../src/utils/env.js", () => ({ default: mockEnv({ /* env overrides */ }), })); const { isPremiumEnabled } = await import("../../src/utils/premium.js");With:
import { describe, it, expect } from "bun:test"; import esmock from "esmock"; const { isPremiumEnabled } = await esmock("../../src/utils/premium.js", { "../../src/utils/env.js": { default: mockEnv({ /* env overrides */ }) }, });Each test can call
esmock()with its own env overrides; esmock handles module cache isolation at load time, preventing mock pollution across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/premium.test.ts` around lines 4 - 92, The tests currently use Bun's mock.module() which shares module cache and causes mock pollution; replace each mock.module(...) + dynamic import of the premium module with esmock(...) to load "../../src/utils/premium.js" while stubbing "../../src/utils/env.js" per test, import esmock at the top of the test file and keep only describe/it/expect from bun:test, and update each test that imports isPremiumEnabled, getPremiumSkuId, and hasEntitlement to call esmock with the appropriate mockEnv overrides so each test gets an isolated module instance.tests/commands/invite.test.ts (1)
1-18:⚠️ Potential issue | 🟠 MajorReplace Bun test framework with Mocha + Chai + Sinon + esmock.
Lines 1 and 14–18 use Bun's native testing (
bun:test) and module mocking (mock.module()), which violates the required test framework stack. All test files must use Mocha + Chai + Sinon + esmock withesmockfor ESM module mocking instead of Bun's nativemock.module().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/commands/invite.test.ts` around lines 1 - 18, Replace Bun's test harness and module-mocking with Mocha/Chai/Sinon/esmock: change the top-level import from "bun:test" to use Mocha's describe/it and Chai's expect (and keep sinon) and remove any use of mock.module(); inside loadModule(), replace mock.module("../../src/utils/logger.js", ...) and mock.module("../../src/utils/posthog.js", ...) with esmock-based mocking by calling esmock to import "../../src/commands/invite.js" while providing replacements for the logger and posthog modules (using the existing mockLogger() and mockPosthog() factories), then return the imported module; ensure afterEach still calls sinon.restore() and that loadModule, mockLogger, mockPosthog, mockClient, and mockInteraction are wired into esmock so tests work with the invite.js import.tests/events/interactionCreate.test.ts (1)
1-44:⚠️ Potential issue | 🟠 MajorTest framework and ESM mocking strategy must be changed to Mocha + Chai + Sinon + esmock.
This file uses
bun:test(line 1) andmock.module(...)(lines 11-24) instead of the required Mocha + Chai + Sinon + esmock stack. Per coding guidelines fortests/**/*.test.ts, all tests must use Mocha for structure, Chai for assertions, Sinon for stubbing, andesmockfor ESM module mocking to replace imports at load time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events/interactionCreate.test.ts` around lines 1 - 44, The test file uses bun:test and mock.module instead of the required Mocha+Chai+Sinon+esmock stack; update the top-level imports to Mocha's describe/it/beforeEach/afterEach, Chai's expect, Sinon for stubs, and replace mock.module calls inside loadModule with esmock's import mocking (use esmock to import "../../src/events/interactionCreate.js" with the mocked modules). Specifically, keep the helper functions makeCommandInteraction and loadModule but change loadModule to create executeStub and setupAutocomplete, then use esmock to stub "../../src/utils/logger.js", each command module (help.js, about.js, etc.), and the setup module before importing interactionCreateEvent so interactionCreateEvent is loaded with those mocks; ensure test assertions use Chai's expect and cleanup still calls sinon.restore in afterEach.src/commands/admin/quote/remove.ts (1)
37-50:⚠️ Potential issue | 🟠 MajorAvoid the read-then-delete race.
After the lookup at Line 37, another request can delete the same quote before Line 50. This command will still announce success even when
delete()removes nothing. Collapse this into a single delete that returns the removed row (or inspect affected rows) and branch on that result instead.🛠️ Suggested change
- const [quote] = await db - .select() - .from(motivationQuotes) - .where(eq(motivationQuotes.id, quoteId)) - .limit(1); - if (!quote) { + const [deletedQuote] = await db + .delete(motivationQuotes) + .where(eq(motivationQuotes.id, quoteId)) + .returning({ id: motivationQuotes.id }); + if (!deletedQuote) { await interaction.reply({ content: `Quote with id ${quoteId} not found`, flags: MessageFlags.Ephemeral, }); return; } - - await db.delete(motivationQuotes).where(eq(motivationQuotes.id, quoteId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/admin/quote/remove.ts` around lines 37 - 50, The current read-then-delete on motivationQuotes (the select using db.select().from(motivationQuotes).where(eq(motivationQuotes.id, quoteId)) followed by db.delete(motivationQuotes).where(eq(motivationQuotes.id, quoteId))) is vulnerable to a race where the row is removed between operations; collapse into a single delete operation that either returns the deleted row (use the delete(...).where(...).returning(...) variant) or inspects the affected-rows count from the delete result, and only send the success reply when a row was actually removed; update the branching logic to check that returned row/affected count and reply with "not found" when zero rows were deleted.
🧹 Nitpick comments (12)
tests/events/interactionCreate.test.ts (1)
104-118: Extract duplicated mock wiring into a shared helper.The three error-path tests repeat nearly identical module-mock setup blocks. Consolidating this into one helper will reduce drift and simplify updates.
Also applies to: 135-149, 166-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events/interactionCreate.test.ts` around lines 104 - 118, Extract the repeated mock.module wiring into a shared test helper and call it from each error-path test to remove duplication: create a helper function (e.g., setupInteractionMocks or initInteractionCreateMocks) that registers the same mocks shown (logger, commands like help.js/about.js/changelog.js/quote.js/suggestion.js/invite.js/admin/index.js/setup/index.js with setupAutocomplete, premium.js, owner/index.js) and then import and call that helper before importing interactionCreateEvent in each test; update the three error-path tests (the blocks at lines around 104–118, 135–149, 166–180) to call this helper instead of repeating the mock.module calls so tests remain identical but DRY.src/commands/admin/activity/create.ts (1)
49-61: Consider handling the case where insert returns no data.The Drizzle insert with
.returning()should return the inserted row, but ifnewActivityis undefined, the reply would show "Activity added with id: undefined". While this is unlikely in practice (the insert would throw on error), adding an explicit check would be more robust.🛡️ Optional defensive check
const [newActivity] = await db .insert(discordActivities) .values({ activity, type: activityType as DiscordActivityType, url: activityUrl, }) .returning(); + if (!newActivity) { + await interaction.reply({ + content: "Failed to create activity. Please try again.", + flags: MessageFlags.Ephemeral, + }); + return; + } + await interaction.reply({ - content: `Activity added with id: ${newActivity?.id}`, + content: `Activity added with id: ${newActivity.id}`, flags: MessageFlags.Ephemeral, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/admin/activity/create.ts` around lines 49 - 61, The insert's returning result (newActivity from db.insert(discordActivities).values(...).returning()) may be undefined; add a defensive check after the insert to handle that case: if newActivity is undefined, send an error/failure reply via interaction.reply (or throw a specific error) instead of replying with "id: undefined", otherwise continue to reply with the created id; ensure you reference newActivity and interaction.reply in the fix so the behavior is explicit and robust.src/commands/suggestion.ts (1)
100-107: Consider reordering the newQuote check before the user reply.The current flow replies to the user with a success message (lines 100-103), then checks if
newQuoteis falsy (lines 105-107). If the insert somehow fails to return a row, the user receives a success confirmation but the embed notification to the main channel is silently skipped.While this scenario is unlikely (Drizzle's
returning()should always return the inserted row, and actual failures would throw), moving the guard before the reply would be more logically consistent.♻️ Suggested reordering
.returning(); + if (!newQuote) { + await interaction.reply({ + content: "An error occurred while creating your suggestion.", + flags: MessageFlags.Ephemeral, + }); + return; + } + await interaction.reply({ content: "Quote suggestion created owner will review it soon!", flags: MessageFlags.Ephemeral, }); - if (!newQuote) { - return; - } - /** * Send the quote suggestion to the main channel for review🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/suggestion.ts` around lines 100 - 107, Move the falsy newQuote guard to before sending the user reply so we only acknowledge success when an inserted row exists: check if newQuote is truthy (the variable returned by your DB insert) and return early if not, then call interaction.reply({ content: "...", flags: MessageFlags.Ephemeral }) and continue to build/send the embed to the main channel; update the flow in the command handler where newQuote and interaction.reply are used so the reply happens after confirming newQuote rather than before.src/utils/guildDatabase.ts (2)
51-82: Consider usingPromise.allSettled()for batch guild deletions.The current implementation uses a sequential
for...ofloop with individual try/catch blocks. While this works and handles errors per-guild, the coding guidelines recommend usingPromise.allSettled()for batch operations across guilds to improve performance and ensure one guild's failure doesn't block others.♻️ Suggested refactor using Promise.allSettled
- for (const guild of guildsToRemove) { - try { - await db.delete(guilds).where(eq(guilds.guildId, guild.guildId)); - - logger.success( - "Discord - Guild Database", - "Removed guild from database", - { - guildId: guild.guildId, - } - ); - - posthog.capture({ - distinctId: guild.guildId, - event: "guild left", - properties: { - environment: env.NODE_ENV, - guildName: guild.guildId, - guildId: guild.guildId, - }, - }); - } catch (err) { - logger.error( - "Discord Event Logger", - "Error removing guild from database", - err, - { - guildId: guild.guildId, - } - ); - } - } + await Promise.allSettled( + guildsToRemove.map(async (guild) => { + await db.delete(guilds).where(eq(guilds.guildId, guild.guildId)); + + logger.success( + "Discord - Guild Database", + "Removed guild from database", + { guildId: guild.guildId } + ); + + posthog.capture({ + distinctId: guild.guildId, + event: "guild left", + properties: { + environment: env.NODE_ENV, + guildName: guild.guildId, + guildId: guild.guildId, + }, + }); + }) + ).then((results) => { + results.forEach((result, index) => { + if (result.status === "rejected") { + logger.error( + "Discord Event Logger", + "Error removing guild from database", + result.reason, + { guildId: guildsToRemove[index].guildId } + ); + } + }); + });As per coding guidelines: "Use
Promise.allSettled()for batch operations across guilds so one guild's failure doesn't block others."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/guildDatabase.ts` around lines 51 - 82, Replace the sequential for...of over guildsToRemove with a parallel batch using Promise.allSettled: map guildsToRemove to an async function that performs the same per-guild operations (await db.delete(...).where(eq(guilds.guildId, guild.guildId)), logger.success, posthog.capture) and catch errors to call logger.error with the same arguments; then await Promise.allSettled(...) so all deletions run concurrently and no single failure blocks others while preserving per-guild logging and error handling for functions/identifiers guildsToRemove, db.delete, guilds, eq, logger.success, logger.error, and posthog.capture.
119-153: SamePromise.allSettled()consideration applies toensureGuildExists.This loop also iterates over guilds sequentially. Consider refactoring to use
Promise.allSettled()for consistency and parallelism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/guildDatabase.ts` around lines 119 - 153, The ensureGuildExists loop over guildsToAdd performs sequential awaits; change it to run inserts in parallel using Promise.allSettled by mapping guildsToAdd to an array of async tasks that call db.insert(guilds).values({ guildId: guild.id }) and handle logging/posthog inside each task, then await Promise.allSettled(tasks) and process results to call logger.success/logger.error as appropriate (reference ensureGuildExists, guildsToAdd, db.insert, logger.success, logger.error, posthog.capture).tests/worker/index.test.ts (2)
101-106: Use proper assertion for expected exception.Using
expect(true).toBe(false)to fail when an exception is expected is an anti-pattern that produces unclear failure messages. Consider using Bun'sexpect().toThrow()pattern instead.♻️ Suggested refactor
- try { - await jobProcessor!({ name: "unknown-job" }); - expect(true).toBe(false); - } catch (err) { - expect((err as Error).message).toContain("No job found"); - } + await expect(jobProcessor!({ name: "unknown-job" })).rejects.toThrow("No job found");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/worker/index.test.ts` around lines 101 - 106, Replace the try/catch anti-pattern in the test that calls jobProcessor with an assertion that the promise rejects; specifically, remove the manual try/catch and use the test framework's async rejection assertion (e.g., await expect(...).rejects.toThrow(...) or the Bun equivalent) against jobProcessor!({ name: "unknown-job" }) to assert the error message contains "No job found"; update the assertion to target jobProcessor so failures produce clear test output.
51-61: Consider extracting logger reset into a helper.This nested loop for resetting logger stubs is complex. Consider adding a
resetLogger(logger)helper inhelpers.jsto encapsulate this logic and keep tests cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/worker/index.test.ts` around lines 51 - 61, Extract the nested reset logic into a helper function named resetLogger(logger) (e.g., in helpers.js) that iterates Object.values(logger), detects sinon stubs (function with "reset") and nested objects, and calls reset on each stub; then replace the nested loop in tests/worker/index.test.ts with a single call to resetLogger(logger) to simplify the test. Ensure the helper exports resetLogger and update the test import to use it, keeping behavior identical (handle nulls and non-function properties).src/worker/jobs/sendMotivation.ts (1)
32-40: Consider using Drizzle's$first()or destructuring for single-row queries.The random quote selection retrieves an array but only uses the first element. This works correctly but could be slightly cleaner.
Optional: Destructure the result directly
- const motivationQuote = await db.select().from(motivationQuotes).offset(skip).limit(1); + const [motivationQuote] = await db.select().from(motivationQuotes).offset(skip).limit(1); - if (!motivationQuote[0]) { + if (!motivationQuote) { logger.error("Worker", "No motivation quote found in the database"); return; }Then update subsequent usages from
motivationQuote[0]tomotivationQuote.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/jobs/sendMotivation.ts` around lines 32 - 40, The code fetches a random row into an array (variable motivationQuote) then only uses motivationQuote[0]; replace that by using Drizzle's $first() (or destructure the single-row result) when selecting from motivationQuotes so you get a single object instead of an array, update the variable name (e.g., motivationQuote) to hold the object, change the existence check from motivationQuote[0] to motivationQuote, and ensure subsequent uses reference the object; keep the count logic (countResult/value, motivationQuoteCount, skip) unchanged..github/workflows/ci.yml (2)
73-75: Security audit withcontinue-on-errorwon't fail builds on vulnerabilities.The
continue-on-error: trueonbun pm auditmeans vulnerable dependencies won't block merges. This may be intentional during the migration period, but consider removing it once stable to enforce security checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 73 - 75, The CI step named "Run security audit" currently uses "bun pm audit" with continue-on-error: true which lets vulnerabilities pass; change the step (identified by the name "Run security audit" and the "bun pm audit" command) to remove the continue-on-error flag or set it to false so the workflow fails on audit issues, ensuring the audit step enforces security checks.
22-25: Pin Bun version for CI reproducibility.Using
bun-version: latestcauses inconsistent CI behavior across builds. Pin to a specific stable version (currently 1.3.11) to ensure reproducible builds.Suggested fix
- name: Setup Bun uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: "1.3.11"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 22 - 25, Replace the dynamic Bun setup in the CI step that uses the oven-sh/setup-bun action (the "Setup Bun" step and its bun-version input) to pin the bun-version to a stable release (e.g., set bun-version to 1.3.11) instead of "latest" so builds are reproducible across runs.package.json (1)
58-58: Pin@types/bunto a specific version instead of "latest".Using
"latest"for@types/bunresults in non-reproducible builds and may introduce unexpected type errors across environments. Pin to a specific version for consistency.Suggested fix
- "@types/bun": "latest", + "@types/bun": "^1.3.10",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 58, Replace the non-reproducible dependency specifier "@types/bun": "latest" with an exact, pinned version; find the known-good version for your environment (e.g., from package-lock.json/yarn.lock or `npm view `@types/bun` version`) and update the package.json entry for "@types/bun" to that exact version string so installs are deterministic.src/worker/jobs/setActivity.ts (1)
24-97: Return an explicit failure value fromsetActivityCore.Right now, exceptions log and return
undefined. Make the function contract explicit (Promise<boolean>) and returnfalsein the catch path.Suggested patch
-export async function setActivityCore(client: Client, { db: _db, env: _env, logger: _logger }: SetActivityDeps) { +export async function setActivityCore( + client: Client, + { db: _db, env: _env, logger: _logger }: SetActivityDeps +): Promise<boolean> { @@ } catch (err) { _logger.error( "Worker", "Error setting custom discord activity", err ); + return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/jobs/setActivity.ts` around lines 24 - 97, The function setActivityCore currently can resolve to undefined on errors; change its signature to return Promise<boolean> and ensure all code paths return a boolean by returning false in the catch block (and keep returning true on success paths). Update the declaration of setActivityCore and add an explicit return false inside the catch(err) branch where _logger.error(...) is called so the function contract is explicit and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 27-28: The RUN line installing packages should include
--no-install-recommends to avoid pulling unnecessary recommended packages;
update the command that contains "apt-get update && apt-get install -y openssl
curl && rm -rf /var/lib/apt/lists/* \ && groupadd -r fluffboost && useradd -r
-g fluffboost fluffboost" so the apt-get install becomes apt-get install -y
--no-install-recommends openssl curl while preserving the following cleanup and
the groupadd/useradd steps.
In `@src/api/index.ts`:
- Around line 29-35: The rate limiter is being registered with
app.use(rateLimit(...)) before Express is configured to trust the proxy, so
req.ip will resolve to the proxy address and rate limits will be shared; fix
this by calling app.set("trust proxy", 1) (or the appropriate hop count for your
environment) before the rateLimit middleware is applied so that rateLimit (and
req.ip) sees the real client IP.
In `@src/app.ts`:
- Around line 17-24: The DB probe using queryClient`SELECT 1` is currently
fire-and-forget so api.listen() and manager.spawn() start even if PostgreSQL is
down; change startup to await the probe before starting listeners by moving the
probe into an async bootstrap/init function (or an async IIFE) that awaits
queryClient`SELECT 1`, logs via logger.database.connected("PostgreSQL") on
success, and only then calls api.listen(...) and manager.spawn(...); on probe
failure keep the existing logger.database.error("PostgreSQL", err) and
process.exit(1) so the process fails before accepting work.
In `@src/commands/admin/quote/create.ts`:
- Around line 61-63: The early return when newQuote is falsy in the create quote
command leaves the interaction unresolved; update the handler (in
src/commands/admin/quote/create.ts where newQuote is checked) to send a
user-facing reply (e.g., interaction.reply or followUp) indicating the quote
creation failed and log the failure (use the existing logger or processLogger)
including any relevant context (user id, input values) so the command doesn't
hang silently; ensure the reply respects ephemeral/permission semantics used
elsewhere in this command.
In `@tests/commands/admin/activity/remove.test.ts`:
- Around line 1-3: The test currently imports test helpers from "bun:test"
(symbols describe, it, expect, afterEach, mock) and likely uses Bun's
mock.module(); replace these with Mocha/Chai/Sinon + esmock equivalents: import
describe/it/beforeEach/afterEach from "mocha", import expect from "chai", use
sinon for spies/stubs, and replace any usage of mock.module()/bun:test mocking
with esmock to stub module imports. Update the test harness in remove.test.ts to
use esmock to load the module-under-test with mocked dependencies (preserving
helper names like mockLogger, mockDb, mockDbChain, mockInteraction, mockClient)
and adapt any Bun-specific APIs to Mocha/Chai assertions and Sinon cleanup
(restore/sandbox) so the test suite runs with the repository's Mocha/Chai/Sinon
+ esmock stack.
In `@tests/commands/admin/quote/create.test.ts`:
- Around line 1-3: The test currently imports testing utilities from "bun:test"
and uses bun's mock.module for ESM mocking; replace these with the required
stack by importing Mocha's globals (describe, it, beforeEach/afterEach) via
Mocha, use Chai's expect instead of bun's expect, and use Sinon for spies/stubs
(sinon is already present); replace every use of mock.module (lines around the
module-mocking blocks) with esmock to stub ESM imports—ensure you import esmock
and call esmock(modulePath, { './dep': stub }) where the original mock.module
was used, and update any mock-related helpers (mockLogger, mockDb, mockDbChain,
mockInteraction, mockClient, mockEnv) to be provided via esmock stubs or Sinon
as needed so the tests run with Mocha+Chai+Sinon+esmock.
In `@tests/commands/admin/quote/remove.test.ts`:
- Around line 1-3: The test file uses bun:test imports and Bun's
mock.module-based mocking; replace the bun:test import with Mocha/Chai/Sinon by
importing describe/it from Mocha and expect from Chai and ensure sinon is used
for spies/stubs; locate the test helper imports (mockLogger, mockDb,
mockDbChain, mockInteraction, mockClient, mockEnv) and any calls to
mock.module(...) (the previous mock.module calls around where the command module
is loaded) and replace those with esmock-based ESM mocking so the command under
test (the admin quote remove command module) is imported via esmock with the
helper mocks injected; keep existing sinon sandbox/restore patterns and update
any assert/expect usage to Chai's expect API.
In `@tests/commands/premium.test.ts`:
- Around line 85-87: The test currently verifies replyArgs.flags is defined but
doesn't assert the actual ephemeral value; update the assertion that inspects
the Sinon stubbed interaction.reply result (variable replyArgs from
(interaction.reply as sinon.SinonStub).firstCall.args[0]) to check the exact
ephemeral flag — e.g., assert replyArgs.flags.ephemeral is true (or the expected
boolean) instead of using toBeDefined(), so the test guarantees the response is
ephemeral.
In `@tests/commands/quote.test.ts`:
- Around line 1-3: Replace the bun:test-specific imports (describe, it, expect,
afterEach, mock) with Mocha's test functions and Chai's expect, keep Sinon for
spies/stubs, and remove any use of bun.test's mock.module(); instead load the
module-under-test via esmock at setup time to inject mocks (replace
mock.module(...) usage). Specifically, swap imports from "bun:test" for
Mocha/Chai (describe/it/beforeEach/afterEach) and import expect from "chai",
keep sinon from "sinon", and use esmock to import the tested file while
supplying mocks such as mockLogger, mockDb, mockDbChain, mockPosthog,
mockClient, mockInteraction so tests run under Mocha+Chai+Sinon+esmock.
In `@tests/commands/suggestion.test.ts`:
- Around line 1-3: The test file tests/commands/suggestion.test.ts currently
imports from bun:test and uses mock.module() which conflicts with repo
standards; update the top-level imports to use Mocha
(describe/it/beforeEach/afterEach), Chai (expect), and Sinon (sinon) instead of
bun:test, then replace any mock.module(...) usage (the dynamic mock setup around
mockLogger, mockDb, mockDbChain, mockPosthog, mockClient, mockInteraction,
mockEnv) with esmock() calls that stub those imports at load time and change the
dynamic import pattern to await esmock('<module-path>', { '<imported-name>':
stub }) to load the module under test; ensure helper symbols mockLogger, mockDb,
mockDbChain, mockPosthog, mockClient, mockInteraction, mockEnv are provided to
esmock and update test lifecycle hooks to use Mocha/Sinon stubbing and cleanup.
In `@tests/events/entitlementUpdate.test.ts`:
- Around line 1-3: Replace Bun test framework and Bun mock.module() usage with
Mocha/Chai/Sinon and esmock: change the imports from "bun:test" (symbols
describe, it, expect, afterEach, mock) to Mocha/Chai/Sinon imports (describe/it
from mocha, expect from chai, sinon for spies/stubs) and remove any use of
mock.module(); for each occurrence of mock.module() that stubs modules, rewrite
the setup to use esmock to import the tested module with mocked dependencies
(using esmock(modulePath, { './dep': stub }) or esmock.resolve) and adapt
teardown to restore sinon stubs; keep existing helper fixtures (mockLogger,
mockDb, mockDbChain, mockPosthog, mockEntitlement) but pass them into esmock as
the replacement values for the corresponding imports when creating the test
subject, and ensure afterEach restores sinon.restore() and any esmock cleanup if
needed.
In `@tests/events/guildDelete.test.ts`:
- Around line 1-3: The test uses Bun's test API (importing describe, it, expect,
afterEach, mock from "bun:test") and Bun's mock.module() calls; replace these
with Mocha + Chai + Sinon + esmock patterns: import describe/it/afterEach from
"mocha", import { expect } from "chai", keep sinon from "sinon", and remove the
Bun `mock` usage; convert any mock.module(...) blocks to esmock() calls to
import the system under test with injected stubs (use esmock(modulePath, {
"./dep": stub }) or esmock.default to create mocks) and replace Bun mock.reset()
logic with sinon.restore() in afterEach; ensure existing helper mocks
(mockLogger, mockDb, mockDbChain, mockPosthog, mockGuild) are passed into esmock
stubs or sinon stubs so tests referencing those symbols still work and update
any asynchronous module-loading patterns accordingly.
In `@tests/utils/permissions.test.ts`:
- Around line 1-17: Replace the Bun test framework usage with Mocha + Chai +
Sinon + esmock: remove the `import { describe, it, expect, afterEach,
beforeEach, mock } from "bun:test"` and instead import Mocha hooks (`describe`,
`it`, `beforeEach`, `afterEach`) and Chai's `expect`, use `sinon` for restores,
and load the module-under-test with `esmock` (not `mock.module`) so ESM mocks
are applied at load time; specifically update this test to use esmock to import
`"../../src/utils/permissions.js"` with mocked `"../../src/utils/env.js"` and
`"../../src/utils/logger.js"` (using your existing `mockEnv()` and
`mockLogger()` helpers) and then call `isUserPermitted` as before, ensuring the
test file uses Mocha/Chai assertions and Sinon `sinon.restore()` in `afterEach`.
In `@tests/utils/scheduleEvaluator.test.ts`:
- Line 1: The test currently imports testing helpers from "bun:test"; replace
that with the project's standard Mocha/Chai/Sinon/esmock imports: remove the
line importing from "bun:test" and instead import describe and it from "mocha",
expect from "chai", sinon from "sinon" and esmock from "esmock"; update any use
of afterEach to use Mocha's afterEach (imported from "mocha") and ensure you
call sinon.restore() inside afterEach to clean up stubs/mocks; verify all
references to describe, it, expect, afterEach, sinon, and esmock in the file are
updated to match these imports.
In `@tests/worker/setActivity.test.ts`:
- Around line 1-3: Replace the bun:test imports and its mock.module usage with
the Mocha/Chai/Sinon/esmock stack: remove "import { describe, it, expect, mock }
from 'bun:test'" and instead use Mocha globals (describe/it) and import expect
from "chai" (import { expect } from 'chai'); keep sinon import as-is; remove any
use of mock.module(...) and load the module-under-test using esmock (import
esmock from 'esmock' and e.g. const setActivityModule = await esmock('<relative
path to module>', { '../logger': mockLogger, '../db': mockDb, '../dbChain':
mockDbChain, '../env': mockEnv, '../client': mockClient }); then extract the
function under test (e.g. const { setActivity } = setActivityModule). Update all
tests to use the esmock-provided module and Chai's expect instead of bun:test's
expect.
---
Outside diff comments:
In `@src/commands/admin/quote/remove.ts`:
- Around line 37-50: The current read-then-delete on motivationQuotes (the
select using db.select().from(motivationQuotes).where(eq(motivationQuotes.id,
quoteId)) followed by db.delete(motivationQuotes).where(eq(motivationQuotes.id,
quoteId))) is vulnerable to a race where the row is removed between operations;
collapse into a single delete operation that either returns the deleted row (use
the delete(...).where(...).returning(...) variant) or inspects the affected-rows
count from the delete result, and only send the success reply when a row was
actually removed; update the branching logic to check that returned row/affected
count and reply with "not found" when zero rows were deleted.
In `@tests/commands/invite.test.ts`:
- Around line 1-18: Replace Bun's test harness and module-mocking with
Mocha/Chai/Sinon/esmock: change the top-level import from "bun:test" to use
Mocha's describe/it and Chai's expect (and keep sinon) and remove any use of
mock.module(); inside loadModule(), replace
mock.module("../../src/utils/logger.js", ...) and
mock.module("../../src/utils/posthog.js", ...) with esmock-based mocking by
calling esmock to import "../../src/commands/invite.js" while providing
replacements for the logger and posthog modules (using the existing mockLogger()
and mockPosthog() factories), then return the imported module; ensure afterEach
still calls sinon.restore() and that loadModule, mockLogger, mockPosthog,
mockClient, and mockInteraction are wired into esmock so tests work with the
invite.js import.
In `@tests/events/interactionCreate.test.ts`:
- Around line 1-44: The test file uses bun:test and mock.module instead of the
required Mocha+Chai+Sinon+esmock stack; update the top-level imports to Mocha's
describe/it/beforeEach/afterEach, Chai's expect, Sinon for stubs, and replace
mock.module calls inside loadModule with esmock's import mocking (use esmock to
import "../../src/events/interactionCreate.js" with the mocked modules).
Specifically, keep the helper functions makeCommandInteraction and loadModule
but change loadModule to create executeStub and setupAutocomplete, then use
esmock to stub "../../src/utils/logger.js", each command module (help.js,
about.js, etc.), and the setup module before importing interactionCreateEvent so
interactionCreateEvent is loaded with those mocks; ensure test assertions use
Chai's expect and cleanup still calls sinon.restore in afterEach.
In `@tests/utils/premium.test.ts`:
- Around line 4-92: The tests currently use Bun's mock.module() which shares
module cache and causes mock pollution; replace each mock.module(...) + dynamic
import of the premium module with esmock(...) to load
"../../src/utils/premium.js" while stubbing "../../src/utils/env.js" per test,
import esmock at the top of the test file and keep only describe/it/expect from
bun:test, and update each test that imports isPremiumEnabled, getPremiumSkuId,
and hasEntitlement to call esmock with the appropriate mockEnv overrides so each
test gets an isolated module instance.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 73-75: The CI step named "Run security audit" currently uses "bun
pm audit" with continue-on-error: true which lets vulnerabilities pass; change
the step (identified by the name "Run security audit" and the "bun pm audit"
command) to remove the continue-on-error flag or set it to false so the workflow
fails on audit issues, ensuring the audit step enforces security checks.
- Around line 22-25: Replace the dynamic Bun setup in the CI step that uses the
oven-sh/setup-bun action (the "Setup Bun" step and its bun-version input) to pin
the bun-version to a stable release (e.g., set bun-version to 1.3.11) instead of
"latest" so builds are reproducible across runs.
In `@package.json`:
- Line 58: Replace the non-reproducible dependency specifier "@types/bun":
"latest" with an exact, pinned version; find the known-good version for your
environment (e.g., from package-lock.json/yarn.lock or `npm view `@types/bun`
version`) and update the package.json entry for "@types/bun" to that exact
version string so installs are deterministic.
In `@src/commands/admin/activity/create.ts`:
- Around line 49-61: The insert's returning result (newActivity from
db.insert(discordActivities).values(...).returning()) may be undefined; add a
defensive check after the insert to handle that case: if newActivity is
undefined, send an error/failure reply via interaction.reply (or throw a
specific error) instead of replying with "id: undefined", otherwise continue to
reply with the created id; ensure you reference newActivity and
interaction.reply in the fix so the behavior is explicit and robust.
In `@src/commands/suggestion.ts`:
- Around line 100-107: Move the falsy newQuote guard to before sending the user
reply so we only acknowledge success when an inserted row exists: check if
newQuote is truthy (the variable returned by your DB insert) and return early if
not, then call interaction.reply({ content: "...", flags: MessageFlags.Ephemeral
}) and continue to build/send the embed to the main channel; update the flow in
the command handler where newQuote and interaction.reply are used so the reply
happens after confirming newQuote rather than before.
In `@src/utils/guildDatabase.ts`:
- Around line 51-82: Replace the sequential for...of over guildsToRemove with a
parallel batch using Promise.allSettled: map guildsToRemove to an async function
that performs the same per-guild operations (await
db.delete(...).where(eq(guilds.guildId, guild.guildId)), logger.success,
posthog.capture) and catch errors to call logger.error with the same arguments;
then await Promise.allSettled(...) so all deletions run concurrently and no
single failure blocks others while preserving per-guild logging and error
handling for functions/identifiers guildsToRemove, db.delete, guilds, eq,
logger.success, logger.error, and posthog.capture.
- Around line 119-153: The ensureGuildExists loop over guildsToAdd performs
sequential awaits; change it to run inserts in parallel using Promise.allSettled
by mapping guildsToAdd to an array of async tasks that call
db.insert(guilds).values({ guildId: guild.id }) and handle logging/posthog
inside each task, then await Promise.allSettled(tasks) and process results to
call logger.success/logger.error as appropriate (reference ensureGuildExists,
guildsToAdd, db.insert, logger.success, logger.error, posthog.capture).
In `@src/worker/jobs/sendMotivation.ts`:
- Around line 32-40: The code fetches a random row into an array (variable
motivationQuote) then only uses motivationQuote[0]; replace that by using
Drizzle's $first() (or destructure the single-row result) when selecting from
motivationQuotes so you get a single object instead of an array, update the
variable name (e.g., motivationQuote) to hold the object, change the existence
check from motivationQuote[0] to motivationQuote, and ensure subsequent uses
reference the object; keep the count logic (countResult/value,
motivationQuoteCount, skip) unchanged.
In `@src/worker/jobs/setActivity.ts`:
- Around line 24-97: The function setActivityCore currently can resolve to
undefined on errors; change its signature to return Promise<boolean> and ensure
all code paths return a boolean by returning false in the catch block (and keep
returning true on success paths). Update the declaration of setActivityCore and
add an explicit return false inside the catch(err) branch where
_logger.error(...) is called so the function contract is explicit and
consistent.
In `@tests/events/interactionCreate.test.ts`:
- Around line 104-118: Extract the repeated mock.module wiring into a shared
test helper and call it from each error-path test to remove duplication: create
a helper function (e.g., setupInteractionMocks or initInteractionCreateMocks)
that registers the same mocks shown (logger, commands like
help.js/about.js/changelog.js/quote.js/suggestion.js/invite.js/admin/index.js/setup/index.js
with setupAutocomplete, premium.js, owner/index.js) and then import and call
that helper before importing interactionCreateEvent in each test; update the
three error-path tests (the blocks at lines around 104–118, 135–149, 166–180) to
call this helper instead of repeating the mock.module calls so tests remain
identical but DRY.
In `@tests/worker/index.test.ts`:
- Around line 101-106: Replace the try/catch anti-pattern in the test that calls
jobProcessor with an assertion that the promise rejects; specifically, remove
the manual try/catch and use the test framework's async rejection assertion
(e.g., await expect(...).rejects.toThrow(...) or the Bun equivalent) against
jobProcessor!({ name: "unknown-job" }) to assert the error message contains "No
job found"; update the assertion to target jobProcessor so failures produce
clear test output.
- Around line 51-61: Extract the nested reset logic into a helper function named
resetLogger(logger) (e.g., in helpers.js) that iterates Object.values(logger),
detects sinon stubs (function with "reset") and nested objects, and calls reset
on each stub; then replace the nested loop in tests/worker/index.test.ts with a
single call to resetLogger(logger) to simplify the test. Ensure the helper
exports resetLogger and update the test import to use it, keeping behavior
identical (handle nulls and non-function properties).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78868ec5-5574-470a-b9c0-4b6dffc64ee7
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (97)
.dockerignore.github/workflows/ci.yml.gitignore.mocharc.ymlCLAUDE.mdDockerfileGEMINI.mdbunfig.tomldocker-entrypoint.shdrizzle.config.tspackage.jsonprisma-archive/migrations/20250716060338_init/migration.sqlprisma-archive/migrations/20250716061733_rename_motivation_channel_to_motivation_channel_id/migration.sqlprisma-archive/migrations/20250716063848_refactor_suggestion_quote_relation/migration.sqlprisma-archive/migrations/20250716091429_add_discord_activity_table/migration.sqlprisma-archive/migrations/20250717061117_update_discord_activity_enum/migration.sqlprisma-archive/migrations/20250910203514_remove_guild_id_from_suggestion_quotes/migration.sqlprisma-archive/migrations/20250910203548_add_motivation_timing_fields/migration.sqlprisma-archive/migrations/20250916194116_update_guild_timezone_default/migration.sqlprisma-archive/migrations/20260209190000_add_premium_and_schedule_fields/migration.sqlprisma-archive/migrations/migration_lock.tomlprisma-archive/schema.prismaprisma.config.tssrc/api/index.tssrc/app.tssrc/commands/admin/activity/create.tssrc/commands/admin/activity/list.tssrc/commands/admin/activity/remove.tssrc/commands/admin/quote/create.tssrc/commands/admin/quote/list.tssrc/commands/admin/quote/remove.tssrc/commands/admin/suggestion/approve.tssrc/commands/admin/suggestion/list.tssrc/commands/admin/suggestion/reject.tssrc/commands/admin/suggestion/stats.tssrc/commands/owner/premium/testList.tssrc/commands/quote.tssrc/commands/setup/channel.tssrc/commands/setup/index.tssrc/commands/setup/schedule.tssrc/commands/suggestion.tssrc/database/index.tssrc/database/schema.tssrc/events/entitlementCreate.tssrc/events/entitlementDelete.tssrc/events/entitlementUpdate.tssrc/events/guildCreate.tssrc/events/guildDelete.tssrc/utils/env.tssrc/utils/guildDatabase.tssrc/utils/scheduleEvaluator.tssrc/worker/index.tssrc/worker/jobs/sendMotivation.tssrc/worker/jobs/setActivity.tstests/api/health.test.tstests/commands/about.test.tstests/commands/admin/activity/create.test.tstests/commands/admin/activity/list.test.tstests/commands/admin/activity/remove.test.tstests/commands/admin/quote/create.test.tstests/commands/admin/quote/list.test.tstests/commands/admin/quote/remove.test.tstests/commands/admin/suggestion/approve.test.tstests/commands/admin/suggestion/list.test.tstests/commands/admin/suggestion/reject.test.tstests/commands/admin/suggestion/stats.test.tstests/commands/changelog.test.tstests/commands/help.test.tstests/commands/invite.test.tstests/commands/owner/premium/testDelete.test.tstests/commands/owner/premium/testList.test.tstests/commands/owner/testCreate.test.tstests/commands/premium.test.tstests/commands/quote.test.tstests/commands/setup/channel.test.tstests/commands/setup/schedule.test.tstests/commands/suggestion.test.tstests/events/entitlementCreate.test.tstests/events/entitlementDelete.test.tstests/events/entitlementUpdate.test.tstests/events/guildCreate.test.tstests/events/guildDelete.test.tstests/events/interactionCreate.test.tstests/events/ready.test.tstests/events/shardDisconnect.test.tstests/utils/cronParser.test.tstests/utils/env.test.tstests/utils/guildDatabase.test.tstests/utils/permissions.test.tstests/utils/premium.test.tstests/utils/scheduleEvaluator.test.tstests/utils/timezones.test.tstests/utils/trimArray.test.tstests/worker/index.test.tstests/worker/sendMotivation.test.tstests/worker/setActivity.test.tstsconfig.json
💤 Files with no reviewable changes (3)
- .dockerignore
- .mocharc.yml
- prisma.config.ts
✅ Files skipped from review due to trivial changes (5)
- bunfig.toml
- src/utils/scheduleEvaluator.ts
- src/commands/admin/suggestion/stats.ts
- .gitignore
- GEMINI.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/commands/owner/premium/testList.test.ts
- src/commands/owner/premium/testList.ts
| RUN apt-get update && apt-get install -y openssl curl && rm -rf /var/lib/apt/lists/* \ | ||
| && groupadd -r fluffboost && useradd -r -g fluffboost fluffboost |
There was a problem hiding this comment.
Add --no-install-recommends to reduce image size and attack surface.
The static analysis tool correctly identified that apt-get install is missing the --no-install-recommends flag. This flag prevents installation of recommended but non-essential packages, reducing image size and potential vulnerabilities.
🐛 Proposed fix
-RUN apt-get update && apt-get install -y openssl curl && rm -rf /var/lib/apt/lists/* \
+RUN apt-get update && apt-get install -y --no-install-recommends openssl curl && rm -rf /var/lib/apt/lists/* \
&& groupadd -r fluffboost && useradd -r -g fluffboost fluffboost📝 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.
| RUN apt-get update && apt-get install -y openssl curl && rm -rf /var/lib/apt/lists/* \ | |
| && groupadd -r fluffboost && useradd -r -g fluffboost fluffboost | |
| RUN apt-get update && apt-get install -y --no-install-recommends openssl curl && rm -rf /var/lib/apt/lists/* \ | |
| && groupadd -r fluffboost && useradd -r -g fluffboost fluffboost |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 27-28: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y openssl curl && rm -rf /var/lib/apt/lists/* && groupadd -r fluffboost && useradd -r -g fluffboost fluffboost'
Rule: DS-0029
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 27 - 28, The RUN line installing packages should
include --no-install-recommends to avoid pulling unnecessary recommended
packages; update the command that contains "apt-get update && apt-get install -y
openssl curl && rm -rf /var/lib/apt/lists/* \ && groupadd -r fluffboost &&
useradd -r -g fluffboost fluffboost" so the apt-get install becomes apt-get
install -y --no-install-recommends openssl curl while preserving the following
cleanup and the groupadd/useradd steps.
| app.use( | ||
| rateLimit({ | ||
| windowMs: 60 * 1000, | ||
| limit: 60, | ||
| standardHeaders: "draft-7", | ||
| legacyHeaders: false, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual state of src/api/index.ts around lines 29-35
head -n 50 src/api/index.ts | cat -nRepository: MrDemonWolf/fluffboost
Length of output: 1718
🏁 Script executed:
# Search for any trust proxy configuration in the codebase
rg "trust proxy" --type ts --type tsxRepository: MrDemonWolf/fluffboost
Length of output: 93
🏁 Script executed:
# Check if there are any other rate limit configurations or proxy settings
rg "express-rate-limit|rateLimit" --type ts --type tsx -A 3 -B 3Repository: MrDemonWolf/fluffboost
Length of output: 93
🏁 Script executed:
# Look for deployment configuration files (docker, compose, etc) that might indicate proxying
fd -e "docker*" -e "*compose*" -e "Dockerfile" -e ".env*" | head -20Repository: MrDemonWolf/fluffboost
Length of output: 48
🏁 Script executed:
# Search for trust proxy configuration (retry with correct type)
rg "trust proxy"Repository: MrDemonWolf/fluffboost
Length of output: 48
🏁 Script executed:
# Also check the entire src/api/index.ts file to see full context
wc -l src/api/index.tsRepository: MrDemonWolf/fluffboost
Length of output: 85
🏁 Script executed:
# Check if there's any main app file or initialization that might set trust proxy
rg "createServer|app\.listen|trust proxy" --type ts -B 2 -A 2Repository: MrDemonWolf/fluffboost
Length of output: 48
🏁 Script executed:
# Look for any middleware or initialization files
fd "main|server|init|app" -e ts | head -10Repository: MrDemonWolf/fluffboost
Length of output: 165
🏁 Script executed:
# Check src/app.ts to see if trust proxy is set there
head -n 60 src/app.ts | cat -nRepository: MrDemonWolf/fluffboost
Length of output: 1883
🏁 Script executed:
# Also check if the API is exported/used in app.ts
rg "import.*api|from.*api" src/app.ts -A 3 -B 3Repository: MrDemonWolf/fluffboost
Length of output: 341
Configure trust proxy before IP-based rate limiting.
At line 29, express-rate-limit keys buckets by req.ip. In a proxied deployment, every request collapses to the proxy IP unless Express trusts the proxy first, so this effectively becomes a shared global 60 req/min limit across all clients. Since the health check comments reference Coolify and Pulsetic monitoring, the deployment is proxied.
Add app.set("trust proxy", 1) (or the appropriate hop count) before the rate limiter middleware.
Suggested change
+app.set("trust proxy", 1); // or the correct proxy hop count for the deployment
app.use(
rateLimit({
windowMs: 60 * 1000,📝 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.
| app.use( | |
| rateLimit({ | |
| windowMs: 60 * 1000, | |
| limit: 60, | |
| standardHeaders: "draft-7", | |
| legacyHeaders: false, | |
| }) | |
| app.set("trust proxy", 1); // or the correct proxy hop count for the deployment | |
| app.use( | |
| rateLimit({ | |
| windowMs: 60 * 1000, | |
| limit: 60, | |
| standardHeaders: "draft-7", | |
| legacyHeaders: false, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/index.ts` around lines 29 - 35, The rate limiter is being registered
with app.use(rateLimit(...)) before Express is configured to trust the proxy, so
req.ip will resolve to the proxy address and rate limits will be shared; fix
this by calling app.set("trust proxy", 1) (or the appropriate hop count for your
environment) before the rateLimit middleware is applied so that rateLimit (and
req.ip) sees the real client IP.
| queryClient`SELECT 1` | ||
| .then(() => { | ||
| logger.database.connected("PostgreSQL"); | ||
| }) | ||
| .catch(async (err: Error) => { | ||
| logger.database.error("Prisma", err); | ||
| .catch((err: Error) => { | ||
| logger.database.error("PostgreSQL", err); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
Await the DB probe before starting listeners.
The check at Line 17 is fire-and-forget, so api.listen() and manager.spawn() continue even when PostgreSQL is unavailable. Move startup behind an awaited bootstrap path so the process fails before it accepts work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.ts` around lines 17 - 24, The DB probe using queryClient`SELECT 1` is
currently fire-and-forget so api.listen() and manager.spawn() start even if
PostgreSQL is down; change startup to await the probe before starting listeners
by moving the probe into an async bootstrap/init function (or an async IIFE)
that awaits queryClient`SELECT 1`, logs via
logger.database.connected("PostgreSQL") on success, and only then calls
api.listen(...) and manager.spawn(...); on probe failure keep the existing
logger.database.error("PostgreSQL", err) and process.exit(1) so the process
fails before accepting work.
| if (!newQuote) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Silent return leaves user without feedback.
When newQuote is undefined, the function returns early without replying to the interaction or logging the issue. This would leave the user's command hanging with no response.
🐛 Proposed fix to add user feedback and logging
if (!newQuote) {
+ logger.error("Discord - Command", "Insert returned no data", null, {
+ command: "admin quote create",
+ user: interaction.user.id,
+ });
+ await interaction.reply({
+ content: "Failed to create quote. Please try again.",
+ flags: MessageFlags.Ephemeral,
+ });
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/admin/quote/create.ts` around lines 61 - 63, The early return
when newQuote is falsy in the create quote command leaves the interaction
unresolved; update the handler (in src/commands/admin/quote/create.ts where
newQuote is checked) to send a user-facing reply (e.g., interaction.reply or
followUp) indicating the quote creation failed and log the failure (use the
existing logger or processLogger) including any relevant context (user id, input
values) so the command doesn't hang silently; ensure the reply respects
ephemeral/permission semantics used elsewhere in this command.
| import { describe, it, expect, afterEach, mock } from "bun:test"; | ||
| import sinon from "sinon"; | ||
| import esmock from "esmock"; | ||
| import { mockLogger, mockPrisma, mockInteraction, mockClient } from "../../../helpers.js"; | ||
| import { mockLogger, mockDb, mockDbChain, mockInteraction, mockClient } from "../../../helpers.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/commands/admin/activity/remove.test.ts | head -40Repository: MrDemonWolf/fluffboost
Length of output: 1901
Replace with Mocha/Chai/Sinon + esmock test stack.
This file uses Bun's native test framework (bun:test with mock.module()) instead of the required Mocha + Chai + Sinon + esmock stack. ESM module mocking must be done with esmock rather than Bun's mock.module(). Update the test runner, assertion library, and mocking approach to match the repository guidelines for tests/**/*.test.ts files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/commands/admin/activity/remove.test.ts` around lines 1 - 3, The test
currently imports test helpers from "bun:test" (symbols describe, it, expect,
afterEach, mock) and likely uses Bun's mock.module(); replace these with
Mocha/Chai/Sinon + esmock equivalents: import describe/it/beforeEach/afterEach
from "mocha", import expect from "chai", use sinon for spies/stubs, and replace
any usage of mock.module()/bun:test mocking with esmock to stub module imports.
Update the test harness in remove.test.ts to use esmock to load the
module-under-test with mocked dependencies (preserving helper names like
mockLogger, mockDb, mockDbChain, mockInteraction, mockClient) and adapt any
Bun-specific APIs to Mocha/Chai assertions and Sinon cleanup (restore/sandbox)
so the test suite runs with the repository's Mocha/Chai/Sinon + esmock stack.
| import { describe, it, expect, afterEach, mock } from "bun:test"; | ||
| import sinon from "sinon"; | ||
| import esmock from "esmock"; | ||
| import { mockLogger, mockPrisma, mockPosthog, mockEntitlement } from "../helpers.js"; | ||
| import { mockLogger, mockDb, mockDbChain, mockPosthog, mockEntitlement } from "../helpers.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/events/entitlementUpdate.test.ts | head -100Repository: MrDemonWolf/fluffboost
Length of output: 4888
This test file violates the required Mocha + Chai + Sinon + esmock conventions.
The file uses bun:test (Line 1) and Bun's mock.module() (Lines 13–15, 27–29, 41–43, 53–55, 69–71, 87–89) for module mocking, which conflicts with the enforced test setup. Per coding guidelines, tests/**/*.test.ts must use Mocha as the test framework, Chai for assertions, Sinon for spies/stubs, and esmock for ESM module mocking.
Migrate this test to use esmock instead of Bun's mocking utilities and update the test framework imports accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/events/entitlementUpdate.test.ts` around lines 1 - 3, Replace Bun test
framework and Bun mock.module() usage with Mocha/Chai/Sinon and esmock: change
the imports from "bun:test" (symbols describe, it, expect, afterEach, mock) to
Mocha/Chai/Sinon imports (describe/it from mocha, expect from chai, sinon for
spies/stubs) and remove any use of mock.module(); for each occurrence of
mock.module() that stubs modules, rewrite the setup to use esmock to import the
tested module with mocked dependencies (using esmock(modulePath, { './dep': stub
}) or esmock.resolve) and adapt teardown to restore sinon stubs; keep existing
helper fixtures (mockLogger, mockDb, mockDbChain, mockPosthog, mockEntitlement)
but pass them into esmock as the replacement values for the corresponding
imports when creating the test subject, and ensure afterEach restores
sinon.restore() and any esmock cleanup if needed.
tests/events/guildDelete.test.ts
Outdated
| import { describe, it, expect, afterEach, mock } from "bun:test"; | ||
| import sinon from "sinon"; | ||
| import esmock from "esmock"; | ||
| import { mockLogger, mockPrisma, mockPosthog, mockGuild } from "../helpers.js"; | ||
| import { mockLogger, mockDb, mockDbChain, mockPosthog, mockGuild } from "../helpers.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/events/guildDelete.test.ts | head -60Repository: MrDemonWolf/fluffboost
Length of output: 2759
Replace Bun test framework with Mocha + Chai + Sinon + esmock stack.
This file uses Bun's test framework and mock.module() API (lines 1, 15–17, 31–33, 49–51), which violate the repository's required testing conventions. Per coding guidelines, tests/**/*.test.ts files must use Mocha + Chai + Sinon + esmock, with esmock for ESM module mocking instead of Bun's mock.module().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/events/guildDelete.test.ts` around lines 1 - 3, The test uses Bun's
test API (importing describe, it, expect, afterEach, mock from "bun:test") and
Bun's mock.module() calls; replace these with Mocha + Chai + Sinon + esmock
patterns: import describe/it/afterEach from "mocha", import { expect } from
"chai", keep sinon from "sinon", and remove the Bun `mock` usage; convert any
mock.module(...) blocks to esmock() calls to import the system under test with
injected stubs (use esmock(modulePath, { "./dep": stub }) or esmock.default to
create mocks) and replace Bun mock.reset() logic with sinon.restore() in
afterEach; ensure existing helper mocks (mockLogger, mockDb, mockDbChain,
mockPosthog, mockGuild) are passed into esmock stubs or sinon stubs so tests
referencing those symbols still work and update any asynchronous module-loading
patterns accordingly.
tests/utils/permissions.test.ts
Outdated
| import { describe, it, expect, afterEach, beforeEach, mock } from "bun:test"; | ||
| import sinon from "sinon"; | ||
| import esmock from "esmock"; | ||
| import { mockEnv, mockLogger, mockInteraction } from "../helpers.js"; | ||
|
|
||
| describe("permissions", () => { | ||
|
|
||
| afterEach(() => { | ||
| sinon.restore(); | ||
| }); | ||
|
|
||
| it("should return true when user is in ALLOWED_USERS", async () => { | ||
| const logger = mockLogger(); | ||
| const { isUserPermitted } = await esmock("../../src/utils/permissions.js", { | ||
| "../../src/utils/env.js": { default: mockEnv({ ALLOWED_USERS: "user-123, user-456" }) }, | ||
| "../../src/utils/logger.js": { default: logger }, | ||
| }); | ||
| mock.module("../../src/utils/env.js", () => ({ | ||
| default: mockEnv({ ALLOWED_USERS: "user-123, user-456" }), | ||
| })); | ||
| mock.module("../../src/utils/logger.js", () => ({ default: logger })); | ||
| const { isUserPermitted } = await import("../../src/utils/permissions.js"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/utils/permissions.test.tsRepository: MrDemonWolf/fluffboost
Length of output: 3338
🏁 Script executed:
# Search for esmock usage patterns in the codebase
rg "esmock" --type ts --type tsxRepository: MrDemonWolf/fluffboost
Length of output: 93
🏁 Script executed:
rg "esmock" --type ts -A 3 -B 1Repository: MrDemonWolf/fluffboost
Length of output: 48
🏁 Script executed:
rg "bun:test" --type tsRepository: MrDemonWolf/fluffboost
Length of output: 4265
🏁 Script executed:
fd "*.test.ts" tests/ | head -5 | xargs head -20Repository: MrDemonWolf/fluffboost
Length of output: 354
🏁 Script executed:
cat CLAUDE.md | head -100Repository: MrDemonWolf/fluffboost
Length of output: 7030
🏁 Script executed:
cat package.json | jq '.devDependencies' 2>/dev/null || cat package.json | grep -A 20 '"devDependencies"'Repository: MrDemonWolf/fluffboost
Length of output: 708
Tests must use Mocha + Chai + Sinon + esmock per project coding guidelines, not Bun's test framework.
This file (and all other test files in tests/) use bun:test with mock.module() for mocking, which violates the requirement to use Mocha, Chai, and esmock for ESM module mocking.
Current setup (lines 1-2, 8-9):
import { describe, it, expect, beforeEach, mock } from "bun:test"— should be Mocha + Chaimock.module()with dynamicimport()(lines 8-9, 11) — should useesmockfor ESM mocking at load time
Refactor to use Mocha + Chai + Sinon + esmock. This applies to all test files in tests/ that currently use bun:test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils/permissions.test.ts` around lines 1 - 17, Replace the Bun test
framework usage with Mocha + Chai + Sinon + esmock: remove the `import {
describe, it, expect, afterEach, beforeEach, mock } from "bun:test"` and instead
import Mocha hooks (`describe`, `it`, `beforeEach`, `afterEach`) and Chai's
`expect`, use `sinon` for restores, and load the module-under-test with `esmock`
(not `mock.module`) so ESM mocks are applied at load time; specifically update
this test to use esmock to import `"../../src/utils/permissions.js"` with mocked
`"../../src/utils/env.js"` and `"../../src/utils/logger.js"` (using your
existing `mockEnv()` and `mockLogger()` helpers) and then call `isUserPermitted`
as before, ensuring the test file uses Mocha/Chai assertions and Sinon
`sinon.restore()` in `afterEach`.
| @@ -1,4 +1,4 @@ | |||
| import { expect } from "chai"; | |||
| import { describe, it, expect, afterEach } from "bun:test"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat tests/utils/scheduleEvaluator.test.ts | head -20Repository: MrDemonWolf/fluffboost
Length of output: 704
🏁 Script executed:
# Check other test files to see what frameworks they use
find tests -name "*.test.ts" -o -name "*.test.tsx" | head -10Repository: MrDemonWolf/fluffboost
Length of output: 469
🏁 Script executed:
# Check a few other test files to see framework pattern
for file in tests/api/health.test.ts tests/commands/about.test.ts; do
echo "=== $file ==="
head -5 "$file"
doneRepository: MrDemonWolf/fluffboost
Length of output: 532
🏁 Script executed:
# Check project configuration for test framework
cat package.json | grep -A 5 '"test"' | head -20Repository: MrDemonWolf/fluffboost
Length of output: 234
Test file uses incorrect framework; project tests must use Mocha + Chai + Sinon + esmock.
This file imports from bun:test but should use Mocha, Chai, Sinon, and esmock as required by the project's testing standards for tests/**/*.test.ts files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils/scheduleEvaluator.test.ts` at line 1, The test currently imports
testing helpers from "bun:test"; replace that with the project's standard
Mocha/Chai/Sinon/esmock imports: remove the line importing from "bun:test" and
instead import describe and it from "mocha", expect from "chai", sinon from
"sinon" and esmock from "esmock"; update any use of afterEach to use Mocha's
afterEach (imported from "mocha") and ensure you call sinon.restore() inside
afterEach to clean up stubs/mocks; verify all references to describe, it,
expect, afterEach, sinon, and esmock in the file are updated to match these
imports.
| import { describe, it, expect, mock } from "bun:test"; | ||
| import sinon from "sinon"; | ||
| import esmock from "esmock"; | ||
| import { mockLogger, mockPrisma, mockEnv, mockClient } from "../helpers.js"; | ||
| import { mockLogger, mockDb, mockDbChain, mockEnv, mockClient } from "../helpers.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the entire test file to verify the claims
cat -n tests/worker/setActivity.test.tsRepository: MrDemonWolf/fluffboost
Length of output: 3748
🏁 Script executed:
# Also check what test framework is being used in the repository overall
fd '\.test\.ts$' tests/ | head -5 | xargs -I {} head -5 {}Repository: MrDemonWolf/fluffboost
Length of output: 1192
🏁 Script executed:
# Check if Mocha is installed in the project
cat package.json | grep -A 20 '"devDependencies"'Repository: MrDemonWolf/fluffboost
Length of output: 767
Align this worker test with Mocha/Chai/Sinon/esmock requirements.
The file currently uses bun:test and mock.module(...), which conflicts with the repository's required test stack. Per coding guidelines for tests/**/*.test.ts: "Test files must use Mocha + Chai + Sinon + esmock" and "Use esmock for ESM module mocking to replace imports at load time in tests."
Replace:
bun:testframework with Mochamock.module()withesmockfor module mockingexpectfrom bun:test with Chai assertions
This applies to lines 1–3 (imports) and lines 6–8 (mocking).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/worker/setActivity.test.ts` around lines 1 - 3, Replace the bun:test
imports and its mock.module usage with the Mocha/Chai/Sinon/esmock stack: remove
"import { describe, it, expect, mock } from 'bun:test'" and instead use Mocha
globals (describe/it) and import expect from "chai" (import { expect } from
'chai'); keep sinon import as-is; remove any use of mock.module(...) and load
the module-under-test using esmock (import esmock from 'esmock' and e.g. const
setActivityModule = await esmock('<relative path to module>', { '../logger':
mockLogger, '../db': mockDb, '../dbChain': mockDbChain, '../env': mockEnv,
'../client': mockClient }); then extract the function under test (e.g. const {
setActivity } = setActivityModule). Update all tests to use the esmock-provided
module and Chai's expect instead of bun:test's expect.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/utils/permissions.test.ts (1)
1-12:⚠️ Potential issue | 🟠 MajorSwitch this test back to Mocha + Chai + Sinon + esmock (not
bun:test+mock.module).This segment still uses Bun’s test runtime and Bun module mocks, which conflicts with the repo test standard and with required ESM load-time mocking via
esmock. This concern was already raised earlier and remains unresolved.
As per coding guidelines,tests/**/*.test.{ts,tsx}must use “Mocha + Chai + Sinon + esmock” and “Use esmock for ESM module mocking to replace imports at load time in tests.”#!/bin/bash # Verify framework/mocking usage in tests (read-only) rg -n --type ts --type tsx 'from "bun:test"|mock\.module\(' tests rg -n --type ts --type tsx 'from "chai"|from "esmock"|from "mocha"' tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/permissions.test.ts` around lines 1 - 12, This test file currently imports from "bun:test" and uses Bun's mock.module to stub dependencies; replace those with the repo standard Mocha + Chai + Sinon + esmock pattern: import describe/it/beforeEach from "mocha", expect from "chai", sinon from "sinon", and remove mock.module usage; use esmock to load "../../src/utils/permissions.js" while mocking "../../src/utils/env.js" with mockEnv() and "../../src/utils/logger.js" with mockLogger() (so call esmock to import isUserPermitted with those mocked modules before running tests), and keep/mockInteraction/mockEnv/mockLogger helpers for setup and sinon for spies/stubs—ensure no references to "bun:test" or "mock.module" remain and that isUserPermitted is imported via esmock.
🧹 Nitpick comments (2)
tests/events/ready.test.ts (2)
24-24: UnnecessarysetupAutocompletemock export.The
setupAutocomplete: sinon.stub()is included in the mock, butready.tsonly imports the default export and accessessetup.slashCommand(seesrc/events/ready.tsline 15 and 52). This named export isn't used by the module under test.If added to satisfy another module's transitive import, consider documenting that reason; otherwise, it can be removed to reduce noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events/ready.test.ts` at line 24, The mock in mock.module that adds a named export setupAutocomplete is unnecessary for the ready.ts tests because the event handler only imports the default export and accesses setup.slashCommand; remove the setupAutocomplete: sinon.stub() property from the mock in tests/events/ready.test.ts (the mock.module call) so the mock only exports the default object with slashCommand, unless you intentionally need it for another transitive test—if so, add a brief comment explaining that dependency.
10-32:mock.module()called insideloadModule()may conflict with stated PR goal.The PR commits mention "avoiding mock.module() inside tests" to prevent cross-file interference, yet this file calls
mock.module()insideloadModule()which is invoked per-test. While this pattern can work if Bun properly isolates module caches perimport(), it diverges from the stated stabilization approach.Consider whether moving mock registrations to module-level (outside
loadModule()) or using dependency injection would better align with the PR's testing stability goals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events/ready.test.ts` around lines 10 - 32, The test helper loadModule() registers many mock.module(...) calls inside the function, which contradicts the PR goal of avoiding in-test mock.module usage and can cause cross-test/module cache interference; move all mock.module(...) registrations for dependencies (logger, guildDatabase, setActivity, each command module) to module-level setup (outside loadModule) so they are registered once before any import, then have loadModule() only perform the dynamic import of "../../src/events/ready.js" and return readyEvent, logger, pruneGuilds, ensureGuildExists, setActivity; alternatively refactor readyEvent to accept injected dependencies (DI) and change tests to import the real module and pass stubs directly to avoid using mock.module at all.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/events/ready.test.ts`:
- Line 1: This test file (imports like describe, it, expect, afterEach, mock and
use of mock.module()) uses Bun's test runner; fix by either (A) updating project
docs/config (CLAUDE.md and test runner config) to declare Bun's test runner as
the official standard, or (B) migrate the suite to the mandated stack by
replacing Bun imports (describe, it, expect, afterEach, mock) with
Mocha/Chai/Sinon equivalents and replace mock.module() usage with esmock-based
ESM import mocking for tests (ensure test bootstrap and any test helpers
reference Mocha's global hooks and Chai assertions and that esmock is used to
stub imports at module load time).
---
Duplicate comments:
In `@tests/utils/permissions.test.ts`:
- Around line 1-12: This test file currently imports from "bun:test" and uses
Bun's mock.module to stub dependencies; replace those with the repo standard
Mocha + Chai + Sinon + esmock pattern: import describe/it/beforeEach from
"mocha", expect from "chai", sinon from "sinon", and remove mock.module usage;
use esmock to load "../../src/utils/permissions.js" while mocking
"../../src/utils/env.js" with mockEnv() and "../../src/utils/logger.js" with
mockLogger() (so call esmock to import isUserPermitted with those mocked modules
before running tests), and keep/mockInteraction/mockEnv/mockLogger helpers for
setup and sinon for spies/stubs—ensure no references to "bun:test" or
"mock.module" remain and that isUserPermitted is imported via esmock.
---
Nitpick comments:
In `@tests/events/ready.test.ts`:
- Line 24: The mock in mock.module that adds a named export setupAutocomplete is
unnecessary for the ready.ts tests because the event handler only imports the
default export and accesses setup.slashCommand; remove the setupAutocomplete:
sinon.stub() property from the mock in tests/events/ready.test.ts (the
mock.module call) so the mock only exports the default object with slashCommand,
unless you intentionally need it for another transitive test—if so, add a brief
comment explaining that dependency.
- Around line 10-32: The test helper loadModule() registers many
mock.module(...) calls inside the function, which contradicts the PR goal of
avoiding in-test mock.module usage and can cause cross-test/module cache
interference; move all mock.module(...) registrations for dependencies (logger,
guildDatabase, setActivity, each command module) to module-level setup (outside
loadModule) so they are registered once before any import, then have
loadModule() only perform the dynamic import of "../../src/events/ready.js" and
return readyEvent, logger, pruneGuilds, ensureGuildExists, setActivity;
alternatively refactor readyEvent to accept injected dependencies (DI) and
change tests to import the real module and pass stubs directly to avoid using
mock.module at all.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acf48d8b-949f-4abd-aef7-dad375035d10
📒 Files selected for processing (6)
drizzle/.gitkeeptests/events/interactionCreate.test.tstests/events/ready.test.tstests/utils/guildDatabase.test.tstests/utils/permissions.test.tstests/utils/premium.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/utils/guildDatabase.test.ts
- tests/events/interactionCreate.test.ts
- tests/utils/premium.test.ts
| @@ -1,6 +1,5 @@ | |||
| import { expect } from "chai"; | |||
| import { describe, it, expect, afterEach, mock } from "bun:test"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other test files also use bun:test or if this is inconsistent
echo "=== Files using bun:test ==="
rg -l "from ['\"]bun:test['\"]" tests/
echo ""
echo "=== Files using esmock ==="
rg -l "esmock" tests/
echo ""
echo "=== Package.json test script ==="
cat package.json | jq '.scripts.test'Repository: MrDemonWolf/fluffboost
Length of output: 1701
Entire test suite uses Bun's test runner instead of mandated Mocha + Chai + Sinon + esmock stack.
This file uses bun:test and mock.module() instead of the required testing framework. However, verification shows all 41 test files in the project follow this same pattern, indicating a deliberate project-wide standardization on Bun's native testing tools rather than an isolated oversight.
Per coding guidelines: "Test files must use Mocha + Chai + Sinon + esmock" and "Use esmock for ESM module mocking to replace imports at load time in tests". This represents a project-wide deviation that requires reconciliation—either update CLAUDE.md to reflect Bun's test runner as the standard, or migrate the entire test suite to the mandated testing stack.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/events/ready.test.ts` at line 1, This test file (imports like describe,
it, expect, afterEach, mock and use of mock.module()) uses Bun's test runner;
fix by either (A) updating project docs/config (CLAUDE.md and test runner
config) to declare Bun's test runner as the official standard, or (B) migrate
the suite to the mandated stack by replacing Bun imports (describe, it, expect,
afterEach, mock) with Mocha/Chai/Sinon equivalents and replace mock.module()
usage with esmock-based ESM import mocking for tests (ensure test bootstrap and
any test helpers reference Mocha's global hooks and Chai assertions and that
esmock is used to stub imports at module load time).
Remove posthog-node dependency and all analytics tracking: - Delete src/utils/posthog.ts utility module - Remove posthog.capture() calls from 14 source files - Remove POSTHOG_API_KEY/POSTHOG_HOST env vars from schema, CI, docs - Remove mockPosthog from test helpers and all test files - Update TODO.md to mark Discord portal setup as complete Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Top-level mock.module for env.ts must include envSchema export, otherwise env.test.ts static import fails with SyntaxError in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Every mock.module for env.ts must include envSchema export, otherwise env.test.ts static import fails when any env mock is active. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bun's mock.module for env.ts replaces the module namespace, breaking env.test.ts static import of envSchema. Extracted envSchema to its own side-effect-free file that no test mocks, ensuring the real schema is always available. Removed dummy envSchema from all env.ts mocks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/guildDatabase.ts (1)
89-109:⚠️ Potential issue | 🟠 MajorMake
ensureGuildExists()idempotent under concurrent inserts.This snapshots current rows and then performs plain inserts against a unique
guildId. Another sync pass orguildCreatecan win that race and turn “already present” into a logged error. Reuse the same conflict-handling insert pattern asguildExists()here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/guildDatabase.ts` around lines 89 - 109, ensureGuildExists currently snapshots rows then does plain inserts into the guilds table which can race with concurrent inserts and produce "already present" errors; update ensureGuildExists to perform idempotent inserts using the same conflict-handling pattern used in guildExists (apply an insert against guilds with a conflict on guildId that does nothing / ignores duplicates) so concurrent inserts won't error when another process already added the guild; reference the ensureGuildExists function, the guildExists pattern, the guilds table and guildId unique key when making the change.
♻️ Duplicate comments (1)
tests/commands/premium.test.ts (1)
84-84:⚠️ Potential issue | 🟡 MinorAssert the exact ephemeral flag, not just definition.
toBeDefined()can pass for non-ephemeral values. Assert the expected flag explicitly.Suggested fix
+import { MessageFlags } from "discord.js"; ... - expect(replyArgs.flags).toBeDefined(); + expect(replyArgs.flags).toBe(MessageFlags.Ephemeral);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/commands/premium.test.ts` at line 84, Replace the loose assertion expect(replyArgs.flags).toBeDefined() with an explicit check for the ephemeral flag on replyArgs.flags; e.g. assert replyArgs.flags.ephemeral is true (or use expect(replyArgs.flags).toEqual(expect.objectContaining({ ephemeral: true }))). Update the test in premium.test.ts to reference replyArgs.flags and verify the exact ephemeral boolean rather than only checking definition.
🧹 Nitpick comments (2)
src/utils/guildDatabase.ts (1)
49-70: Batch these per-guild DB operations withPromise.allSettled().Both loops await one guild at a time. That makes cleanup/sync latency scale with guild count and diverges from the repo rule for cross-guild batches, even though failures are already isolated per guild.
Based on learnings,
Use Promise.allSettled() for batch operations across guilds so one guild's failure doesn't block others.Also applies to: 107-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/guildDatabase.ts` around lines 49 - 70, The per-guild delete loop (iterating guildsToRemove and awaiting db.delete(guilds).where(eq(guilds.guildId, guild.guildId))) should be converted to a batched concurrent operation using Promise.allSettled so deletions run in parallel and failures remain isolated; build an array of promises that call the same db.delete(...) and then Promise.allSettled(promises) to handle results, logging success via logger.success and errors via logger.error per-settlement (reference guildsToRemove, db.delete, eq(guilds.guildId, ...), logger.success, logger.error); apply the same transformation to the similar loop handling guilds at the second location (the loop spanning the other block referenced).package.json (1)
57-57: Pin@types/bunto a stable semver range instead oflatest.Currently resolves to
1.3.10inbun.lock, which aligns with the Bun runtime pinned to version 1 in the Dockerfile. Usinglatestrisks type mismatches when lockfiles are regenerated without source changes, especially if a new major version is released. Recommend pinning to^1.0.0or similar to keep types in sync with the runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 57, The dependency entry for "@types/bun" is pinned to "latest", which can cause accidental major upgrades; update the package.json dependency for "@types/bun" to a stable semver range (e.g., "^1.0.0" or "^1.3.10") instead of "latest", then regenerate the lockfile (run your package manager install) so the lockfile and Bun runtime stay in sync; ensure the change targets the "@types/bun" entry in package.json and commit the updated package.json and lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 24-33: The overrides currently use open ranges (e.g., "qs":
">=6.14.2", "hono": ">=4.11.7", "undici": ">=6.23.0", etc.) which can pull
future major versions; update the package.json "overrides" object to pin each
overridden dependency to an exact version string (replace each ">=x.y.z" with
"x.y.z") for all entries such as "qs", "hono", "undici", "lodash", "minimatch",
"serialize-javascript", "@hono/node-server", and "diff" so installs are
reproducible and avoid silent major upgrades.
In `@src/events/entitlementCreate.ts`:
- Around line 17-19: The entitlement handlers currently flip guild isPremium
whenever entitlement.guildId exists; modify entitlementCreate,
entitlementUpdate, and entitlementDelete handlers to first verify the SKU by
comparing entitlement.skuId === getPremiumSkuId() before updating the guild
record: e.g., change the condition in the entitlementCreate handler around the
db.update(guilds)... call to if (entitlement.guildId && entitlement.skuId ===
getPremiumSkuId()) and apply the same SKU check in the corresponding branches in
entitlementUpdate and entitlementDelete so updates to isPremium only occur for
the premium SKU.
In `@src/utils/guildDatabase.ts`:
- Around line 10-12: pruneGuilds currently compares DB guild rows against
client.guilds.cache (which is shard-local) and will delete valid guilds from
other shards; change pruneGuilds so it does not treat DB guilds as missing
simply because they’re not in the local shard cache — either restrict pruning to
only guildIds present in client.guilds.cache for this shard (i.e., only delete
DB rows for guilds known to this shard) or make the comparison shard-aware by
gathering global guild IDs from all shards before deleting; also refactor both
pruneGuilds and ensureGuildExists to perform their per-guild work in batches
using Promise.allSettled() (not serial for-loops) and, in ensureGuildExists,
replace the plain insert with an upsert/insert using onConflictDoNothing() (same
conflict target approach as guildExists) to eliminate the race condition when
inserting new guild rows.
In `@tests/events/guildCreate.test.ts`:
- Line 1: The test currently imports from "bun:test" and uses mock.module();
replace the test stack to Mocha/Chai/Sinon + esmock by changing the top import
to import { describe, it, afterEach } from "mocha"; import { expect } from
"chai"; import sinon from "sinon"; import esmock from "esmock"; remove the
"mock" import and usages; then replace each mock.module(...) call with
esmock(modulePath, stubs) (e.g., where the diff references mock.module use
esmock to load the module with provided stubs) and update any sinon usage for
spies/stubs/restore where the code previously relied on bun's mock helpers
(adjust cleanup in afterEach to call sinon.restore()). Ensure function names
referenced in the file (describe, it, afterEach, expect, mock.module) are
updated to their Mocha/Chai/Sinon/esmock equivalents.
In `@tests/utils/guildDatabase.test.ts`:
- Around line 160-170: Two tests duplicate the same assertions so we need one to
verify conflict-handling; update one of the two happy-path tests (e.g., the
"g-new" case) to assert that the insert call uses the onConflictDoNothing chain.
Specifically, keep calling guildExists("g-new") and asserting the boolean, but
replace the db.insert.calledOnce assertion with an assertion that the query
builder's onConflictDoNothing was invoked (for example, check
db.insert.firstCall.returnValue.onConflictDoNothing.calledOnce or use
sinon.calledWithMatch to assert the chain), referencing guildExists, db.insert,
and onConflictDoNothing.
- Line 1: The test imports Bun's test harness and uses mock.module(); replace
Bun-specific APIs with Mocha/Chai/Sinon and esmock: swap the top-line import of
"bun:test" for Mocha globals (describe, it, beforeEach) and import Chai's expect
and Sinon for spies/stubs; remove mock.module() usage and instead use esmock to
import the module-under-test with mocked dependencies (using esmock(...) or
esmock.importMock) and use Sinon to create stubs/mocks for any dependencies
referenced in the guildDatabase tests (refer to describe/it/beforeEach, expect,
mock.module occurrences) so the tests run under Mocha/Chai/Sinon/esmock.
In `@tests/worker/sendMotivation.test.ts`:
- Line 1: The test imports and APIs from "bun:test" and its mock.module() usage
must be replaced to follow project standards: import test helpers from Mocha
(describe, it, before/after/afterEach) and assertions from Chai (expect), use
Sinon for stubs/spies and ensure cleanup with sinon.restore() in afterEach, and
replace mock.module() calls with esmock imports to create mocked ESM modules;
update any references to mock, expect, and mock.module() in this file (e.g.,
top-level import line and the mocking code around lines where mock.module() is
used) to use the new modules (mocha, chai, sinon, esmock) and adapt
setup/teardown to use sinon and esmock patterns.
---
Outside diff comments:
In `@src/utils/guildDatabase.ts`:
- Around line 89-109: ensureGuildExists currently snapshots rows then does plain
inserts into the guilds table which can race with concurrent inserts and produce
"already present" errors; update ensureGuildExists to perform idempotent inserts
using the same conflict-handling pattern used in guildExists (apply an insert
against guilds with a conflict on guildId that does nothing / ignores
duplicates) so concurrent inserts won't error when another process already added
the guild; reference the ensureGuildExists function, the guildExists pattern,
the guilds table and guildId unique key when making the change.
---
Duplicate comments:
In `@tests/commands/premium.test.ts`:
- Line 84: Replace the loose assertion expect(replyArgs.flags).toBeDefined()
with an explicit check for the ephemeral flag on replyArgs.flags; e.g. assert
replyArgs.flags.ephemeral is true (or use
expect(replyArgs.flags).toEqual(expect.objectContaining({ ephemeral: true }))).
Update the test in premium.test.ts to reference replyArgs.flags and verify the
exact ephemeral boolean rather than only checking definition.
---
Nitpick comments:
In `@package.json`:
- Line 57: The dependency entry for "@types/bun" is pinned to "latest", which
can cause accidental major upgrades; update the package.json dependency for
"@types/bun" to a stable semver range (e.g., "^1.0.0" or "^1.3.10") instead of
"latest", then regenerate the lockfile (run your package manager install) so the
lockfile and Bun runtime stay in sync; ensure the change targets the
"@types/bun" entry in package.json and commit the updated package.json and
lockfile.
In `@src/utils/guildDatabase.ts`:
- Around line 49-70: The per-guild delete loop (iterating guildsToRemove and
awaiting db.delete(guilds).where(eq(guilds.guildId, guild.guildId))) should be
converted to a batched concurrent operation using Promise.allSettled so
deletions run in parallel and failures remain isolated; build an array of
promises that call the same db.delete(...) and then Promise.allSettled(promises)
to handle results, logging success via logger.success and errors via
logger.error per-settlement (reference guildsToRemove, db.delete,
eq(guilds.guildId, ...), logger.success, logger.error); apply the same
transformation to the similar loop handling guilds at the second location (the
loop spanning the other block referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 323493af-a6d0-434a-beca-9737e86246b1
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.env.example.github/workflows/ci.ymlCLAUDE.mdREADME.mdTODO.mdpackage.jsonsrc/commands/about.tssrc/commands/changelog.tssrc/commands/help.tssrc/commands/invite.tssrc/commands/premium.tssrc/commands/quote.tssrc/commands/suggestion.tssrc/events/entitlementCreate.tssrc/events/entitlementDelete.tssrc/events/entitlementUpdate.tssrc/events/guildCreate.tssrc/events/guildDelete.tssrc/utils/env.tssrc/utils/guildDatabase.tssrc/utils/posthog.tssrc/worker/jobs/sendMotivation.tstests/commands/about.test.tstests/commands/changelog.test.tstests/commands/help.test.tstests/commands/invite.test.tstests/commands/premium.test.tstests/commands/quote.test.tstests/commands/suggestion.test.tstests/events/entitlementCreate.test.tstests/events/entitlementDelete.test.tstests/events/entitlementUpdate.test.tstests/events/guildCreate.test.tstests/events/guildDelete.test.tstests/helpers.tstests/utils/env.test.tstests/utils/guildDatabase.test.tstests/worker/sendMotivation.test.ts
💤 Files with no reviewable changes (8)
- README.md
- src/commands/help.ts
- src/commands/premium.ts
- .env.example
- src/commands/about.ts
- src/commands/invite.ts
- src/utils/posthog.ts
- src/commands/changelog.ts
✅ Files skipped from review due to trivial changes (1)
- TODO.md
🚧 Files skipped from review as they are similar to previous changes (20)
- src/events/guildDelete.ts
- src/events/entitlementUpdate.ts
- src/events/guildCreate.ts
- src/commands/quote.ts
- src/utils/env.ts
- src/events/entitlementDelete.ts
- src/commands/suggestion.ts
- tests/commands/invite.test.ts
- tests/utils/env.test.ts
- tests/events/entitlementDelete.test.ts
- tests/commands/help.test.ts
- tests/events/entitlementCreate.test.ts
- tests/commands/changelog.test.ts
- tests/commands/suggestion.test.ts
- tests/commands/about.test.ts
- tests/events/entitlementUpdate.test.ts
- src/worker/jobs/sendMotivation.ts
- tests/commands/quote.test.ts
- .github/workflows/ci.yml
- CLAUDE.md
| "overrides": { | ||
| "qs": ">=6.14.2", | ||
| "hono": ">=4.11.7", | ||
| "undici": ">=6.23.0", | ||
| "lodash": ">=4.17.23", | ||
| "minimatch": ">=9.0.7", | ||
| "serialize-javascript": ">=7.0.3", | ||
| "@hono/node-server": ">=1.19.10", | ||
| "diff": ">=8.0.3" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for unbounded override ranges (>=) in package.json"
rg -n '"[^"]+"\s*:\s*">=' package.json || trueRepository: MrDemonWolf/fluffboost
Length of output: 375
🏁 Script executed:
# Check for lock file presence
git ls-files | grep -E "package-lock\.json|yarn\.lock|pnpm-lock\.yaml" || echo "No lock file found in git"
# Check package.json structure around overrides
head -35 package.json | tail -15Repository: MrDemonWolf/fluffboost
Length of output: 449
🌐 Web query:
npm overrides field best practices unbounded version ranges semver
💡 Result:
npm overrides best practices emphasize using exact versions or pinned semver ranges rather than unbounded ranges like "" or "^x.x.x" to ensure reproducibility, security, and stability. Key practices: - Use exact versions (e.g., "lodash": "4.17.21") for global overrides of transitive dependencies to fix vulnerabilities or conflicts precisely. This forces a specific version everywhere, minimizing risks from future breaking changes. - For targeted overrides, use nested objects (e.g., "react-scripts": {"webpack": "5.74.0"}) to limit scope. - Avoid overriding direct dependencies unless specs match exactly or use "$pkg" reference; nested overrides bypass this. - Do not use unbounded ranges like "" (matches any version) or wide carets (^) in overrides, as they can pull in incompatible future versions, breaking builds or introducing new vulnerabilities. Unbounded ranges defeat the purpose of overrides for surgical fixes. - After adding overrides, delete package-lock.json and node_modules, then run npm install to apply changes. - Use overrides sparingly as a "hack" for security/bug fixes while awaiting upstream updates; prefer updating direct dependencies when possible. - Test thoroughly, as overrides can cause duplication or incompatibility. Examples: Global exact: {"overrides": {"lodash": "4.17.21"}} Scoped: {"overrides": {"eslint": {"*.": "8.56.0", "chalk": "5.2.0"}}} Version selector: {"overrides": {"loader-utils@>2.0.0 <3": "2.0.4"}} Rationale: Overrides replace dependency resolution; unbounded ranges expose projects to left-pad-like risks or semver mismatches, as discussed in general package management critiques.
Citations:
- 1: https://www.herodevs.com/blog-posts/a-guide-to-npm-overrides-take-control-of-your-dependencies
- 2: https://github.com/npm/rfcs/blob/main/accepted/0036-overrides.md
- 3: https://stackoverflow.com/questions/75340053/confused-about-overrides-in-package-json
- 4: https://stackoverflow.com/questions/15806152/how-do-i-override-nested-npm-dependency-versions
- 5: https://sqlpey.com/nodejs/managing-npm-dependency-version-conflicts/
- 6: https://stackoverflow.com/questions/79862787/pnpm-npm-overrides-resolution-logic
- 7: https://willcodefor.beer/posts/npmover
Pin overrides to exact versions to prevent silent major upgrades.
Using >= inside overrides can pull future major versions during lockfile refresh, weakening reproducibility and causing unexpected runtime regressions. This risk is elevated since no lock file is committed to the repository, making every install subject to re-resolution. npm best practices recommend exact versions for overrides to ensure surgical, reproducible dependency fixes.
Suggested change
"overrides": {
- "qs": ">=6.14.2",
- "hono": ">=4.11.7",
- "undici": ">=6.23.0",
- "lodash": ">=4.17.23",
- "minimatch": ">=9.0.7",
- "serialize-javascript": ">=7.0.3",
- "@hono/node-server": ">=1.19.10",
- "diff": ">=8.0.3"
+ "qs": "6.14.2",
+ "hono": "4.11.7",
+ "undici": "6.23.0",
+ "lodash": "4.17.23",
+ "minimatch": "9.0.7",
+ "serialize-javascript": "7.0.3",
+ "@hono/node-server": "1.19.10",
+ "diff": "8.0.3"
},📝 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.
| "overrides": { | |
| "qs": ">=6.14.2", | |
| "hono": ">=4.11.7", | |
| "undici": ">=6.23.0", | |
| "lodash": ">=4.17.23", | |
| "minimatch": ">=9.0.7", | |
| "serialize-javascript": ">=7.0.3", | |
| "@hono/node-server": ">=1.19.10", | |
| "diff": ">=8.0.3" | |
| }, | |
| "overrides": { | |
| "qs": "6.14.2", | |
| "hono": "4.11.7", | |
| "undici": "6.23.0", | |
| "lodash": "4.17.23", | |
| "minimatch": "9.0.7", | |
| "serialize-javascript": "7.0.3", | |
| "@hono/node-server": "1.19.10", | |
| "diff": "8.0.3" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 24 - 33, The overrides currently use open ranges
(e.g., "qs": ">=6.14.2", "hono": ">=4.11.7", "undici": ">=6.23.0", etc.) which
can pull future major versions; update the package.json "overrides" object to
pin each overridden dependency to an exact version string (replace each
">=x.y.z" with "x.y.z") for all entries such as "qs", "hono", "undici",
"lodash", "minimatch", "serialize-javascript", "@hono/node-server", and "diff"
so installs are reproducible and avoid silent major upgrades.
| if (entitlement.guildId) { | ||
| try { | ||
| await prisma.guild.update({ | ||
| where: { guildId: entitlement.guildId }, | ||
| data: { isPremium: true }, | ||
| }); | ||
| await db.update(guilds).set({ isPremium: true }).where(eq(guilds.guildId, entitlement.guildId)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify entitlement lifecycle handlers for SKU-based gating before premium writes.
rg -n -C3 'isPremium:\s*(true|false|!isCancelled)|DISCORD_PREMIUM_SKU_ID|skuId' src/events/entitlement*.tsRepository: MrDemonWolf/fluffboost
Length of output: 3453
🏁 Script executed:
# Check premium utility for SKU validation functions
cat -n src/utils/premium.tsRepository: MrDemonWolf/fluffboost
Length of output: 1055
🏁 Script executed:
# Search for DISCORD_PREMIUM_SKU_ID usage and how SKU validation is performed
rg -n 'DISCORD_PREMIUM_SKU_ID|skuId.*===|skuId.*==|getPremiumSku' src/Repository: MrDemonWolf/fluffboost
Length of output: 1366
All entitlement handlers must validate SKU before updating guild premium status.
The entitlement event handlers in src/events/entitlementCreate.ts, entitlementUpdate.ts, and entitlementDelete.ts update the isPremium flag for any entitlement with a guildId, regardless of SKU. This creates a mismatch with the runtime checks in src/utils/premium.ts, which validate that entitlement.skuId matches the configured DISCORD_PREMIUM_SKU_ID.
If the bot manages multiple SKUs, any non-premium entitlement with a guildId will incorrectly grant or revoke premium status. Before updating isPremium, compare the entitlement's skuId against getPremiumSkuId() from the premium utility.
Example for entitlementCreate.ts line 17-19:
if (entitlement.guildId && entitlement.skuId === getPremiumSkuId()) {
try {
await db.update(guilds).set({ isPremium: true }).where(eq(guilds.guildId, entitlement.guildId));
Apply the same pattern to entitlementUpdate.ts and entitlementDelete.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/events/entitlementCreate.ts` around lines 17 - 19, The entitlement
handlers currently flip guild isPremium whenever entitlement.guildId exists;
modify entitlementCreate, entitlementUpdate, and entitlementDelete handlers to
first verify the SKU by comparing entitlement.skuId === getPremiumSkuId() before
updating the guild record: e.g., change the condition in the entitlementCreate
handler around the db.update(guilds)... call to if (entitlement.guildId &&
entitlement.skuId === getPremiumSkuId()) and apply the same SKU check in the
corresponding branches in entitlementUpdate and entitlementDelete so updates to
isPremium only occur for the premium SKU.
| const guildsInDb = await db.select().from(guilds).orderBy(asc(guilds.guildId)); | ||
|
|
||
| const guildsInCache = client.guilds.cache.map((guild) => guild.id); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bpruneGuilds\s*\(' src -g '!src/utils/guildDatabase.ts'
rg -n -C2 'ShardingManager|shard|ClientReady|ready' srcRepository: MrDemonWolf/fluffboost
Length of output: 10072
🏁 Script executed:
cat -n src/utils/guildDatabase.tsRepository: MrDemonWolf/fluffboost
Length of output: 5053
🏁 Script executed:
cat -n src/events/ready.ts | head -70Repository: MrDemonWolf/fluffboost
Length of output: 2628
Fix pruneGuilds() cache completeness issue in sharded deployment and refactor batch operations.
In a sharded client, client.guilds.cache is per-shard only. Calling pruneGuilds() in the ready event will delete valid guilds from other shards. The function compares DB guilds against the current shard's cache and deletes any not found, treating guilds on other shards as missing.
Additionally, both pruneGuilds() (lines 49–70) and ensureGuildExists() (lines 107–131) violate the coding guideline to use Promise.allSettled() for batch operations across guilds. Refactor both to batch operations with settled promises instead of serializing with for-loops.
ensureGuildExists() also has a race condition: it reads DB guilds (line 89), then inserts without conflict handling (line 109). Use onConflictDoNothing() with the target constraint like guildExists() does at line 149.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/guildDatabase.ts` around lines 10 - 12, pruneGuilds currently
compares DB guild rows against client.guilds.cache (which is shard-local) and
will delete valid guilds from other shards; change pruneGuilds so it does not
treat DB guilds as missing simply because they’re not in the local shard cache —
either restrict pruning to only guildIds present in client.guilds.cache for this
shard (i.e., only delete DB rows for guilds known to this shard) or make the
comparison shard-aware by gathering global guild IDs from all shards before
deleting; also refactor both pruneGuilds and ensureGuildExists to perform their
per-guild work in batches using Promise.allSettled() (not serial for-loops) and,
in ensureGuildExists, replace the plain insert with an upsert/insert using
onConflictDoNothing() (same conflict target approach as guildExists) to
eliminate the race condition when inserting new guild rows.
| @@ -1,62 +1,40 @@ | |||
| import { expect } from "chai"; | |||
| import { describe, it, expect, afterEach, mock } from "bun:test"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'from "bun:test"|mock\.module\(' tests/events/guildCreate.test.tsRepository: MrDemonWolf/fluffboost
Length of output: 423
Migrate to the required test stack: Mocha + Chai + Sinon + esmock.
This file uses bun:test and mock.module() instead of the required testing framework. Replace:
- Line 1: Change import from
"bun:test"to Mocha/Chai/Sinon - Lines 15-16, 33-34: Replace
mock.module()withesmock
Per coding guidelines, test files must use Mocha + Chai + Sinon + esmock and use esmock for ESM module mocking to replace imports at load time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/events/guildCreate.test.ts` at line 1, The test currently imports from
"bun:test" and uses mock.module(); replace the test stack to Mocha/Chai/Sinon +
esmock by changing the top import to import { describe, it, afterEach } from
"mocha"; import { expect } from "chai"; import sinon from "sinon"; import esmock
from "esmock"; remove the "mock" import and usages; then replace each
mock.module(...) call with esmock(modulePath, stubs) (e.g., where the diff
references mock.module use esmock to load the module with provided stubs) and
update any sinon usage for spies/stubs/restore where the code previously relied
on bun's mock helpers (adjust cleanup in afterEach to call sinon.restore()).
Ensure function names referenced in the file (describe, it, afterEach, expect,
mock.module) are updated to their Mocha/Chai/Sinon/esmock equivalents.
| @@ -1,22 +1,19 @@ | |||
| import { expect } from "chai"; | |||
| import { describe, it, expect, beforeEach, mock } from "bun:test"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'from "bun:test"|mock\.module\(' tests/utils/guildDatabase.test.tsRepository: MrDemonWolf/fluffboost
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Check the complete imports and test setup at the beginning of the file
head -50 tests/utils/guildDatabase.test.tsRepository: MrDemonWolf/fluffboost
Length of output: 1581
Replace Bun test stack with Mocha/Chai/Sinon/esmock.
This file uses bun:test and mock.module(), but coding guidelines require Mocha + Chai + Sinon + esmock with esmock for ESM module mocking (lines 1, 35–38). Refactor the test setup to use the required stack.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils/guildDatabase.test.ts` at line 1, The test imports Bun's test
harness and uses mock.module(); replace Bun-specific APIs with Mocha/Chai/Sinon
and esmock: swap the top-line import of "bun:test" for Mocha globals (describe,
it, beforeEach) and import Chai's expect and Sinon for spies/stubs; remove
mock.module() usage and instead use esmock to import the module-under-test with
mocked dependencies (using esmock(...) or esmock.importMock) and use Sinon to
create stubs/mocks for any dependencies referenced in the guildDatabase tests
(refer to describe/it/beforeEach, expect, mock.module occurrences) so the tests
run under Mocha/Chai/Sinon/esmock.
| it("should insert with onConflictDoNothing and return true", async () => { | ||
| const result = await guildExists("g1"); | ||
| expect(result).to.be.true; | ||
| expect(prisma.guild.upsert.calledOnce).to.be.true; | ||
| const args = prisma.guild.upsert.firstCall.args[0]; | ||
| expect(args.where.guildId).to.equal("g1"); | ||
| expect(args.create.guildId).to.equal("g1"); | ||
| expect(result).toBe(true); | ||
| expect(db.insert.calledOnce).toBe(true); | ||
| }); | ||
|
|
||
| it("should upsert and return true for new guild", async () => { | ||
| const prisma = mockPrisma(); | ||
| prisma.guild.upsert.resolves({ guildId: "g-new" }); | ||
|
|
||
| const { guildExists } = await esmock("../../src/utils/guildDatabase.js", { | ||
| "../../src/database/index.js": { prisma }, | ||
| "../../src/utils/logger.js": { default: mockLogger() }, | ||
| "../../src/utils/posthog.js": { default: mockPosthog() }, | ||
| }); | ||
|
|
||
| it("should insert with onConflictDoNothing and return true for new guild", async () => { | ||
| const result = await guildExists("g-new"); | ||
| expect(result).to.be.true; | ||
| expect(prisma.guild.upsert.calledOnce).to.be.true; | ||
| expect(result).toBe(true); | ||
| expect(db.insert.calledOnce).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Replace one guildExists happy path with a conflict assertion.
Both cases only check db.insert and the boolean return, so they would still pass if the new onConflictDoNothing() call disappeared. One of these tests should assert the conflict-handling chain instead of duplicating the same outcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils/guildDatabase.test.ts` around lines 160 - 170, Two tests
duplicate the same assertions so we need one to verify conflict-handling; update
one of the two happy-path tests (e.g., the "g-new" case) to assert that the
insert call uses the onConflictDoNothing chain. Specifically, keep calling
guildExists("g-new") and asserting the boolean, but replace the
db.insert.calledOnce assertion with an assertion that the query builder's
onConflictDoNothing was invoked (for example, check
db.insert.firstCall.returnValue.onConflictDoNothing.calledOnce or use
sinon.calledWithMatch to assert the chain), referencing guildExists, db.insert,
and onConflictDoNothing.
| @@ -1,69 +1,67 @@ | |||
| import { expect } from "chai"; | |||
| import { describe, it, expect, afterEach, mock } from "bun:test"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'from "bun:test"|mock\.module\(' tests/worker/sendMotivation.test.tsRepository: MrDemonWolf/fluffboost
Length of output: 391
Switch from Bun test framework to Mocha + Chai + Sinon + esmock.
This test file uses bun:test and mock.module(), which violates the repository's testing standard. Replace with:
- Mocha for test runner
- Chai for assertions
- Sinon for stubs/spies
- esmock for ESM module mocking instead of
mock.module()
See lines 1, 19–23 and coding guidelines: tests/**/*.test.{ts,tsx} must use Mocha + Chai + Sinon + esmock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/worker/sendMotivation.test.ts` at line 1, The test imports and APIs
from "bun:test" and its mock.module() usage must be replaced to follow project
standards: import test helpers from Mocha (describe, it, before/after/afterEach)
and assertions from Chai (expect), use Sinon for stubs/spies and ensure cleanup
with sinon.restore() in afterEach, and replace mock.module() calls with esmock
imports to create mocked ESM modules; update any references to mock, expect, and
mock.module() in this file (e.g., top-level import line and the mocking code
around lines where mock.module() is used) to use the new modules (mocha, chai,
sinon, esmock) and adapt setup/teardown to use sinon and esmock patterns.
1 participant
Summary
/owner premium test-listcommand to inspect active test entitlements/setup schedulecommand (premium-gated) for custom quote delivery scheduling (frequency, time, timezone, day)/owner premium test-createandtest-deletecommands for toggling test entitlementssend-motivation(per-guild schedule evaluation) andset-activity(bot presence rotation)entitlementCreate,entitlementDelete,entitlementUpdateTODO.mdas a complete production go-live guide (Portal setup → env vars → deploy → verify)Test plan
PREMIUM_ENABLED=trueandDISCORD_PREMIUM_SKU_ID=<sku>in Coolify production env/owner premium test-list→ "No entitlements found"/premium→ gold upsell embed/setup schedule→ blocked by premium gate/owner premium test-create→ save entitlement ID/premium→ green "Premium Active" embed/setup schedule frequency:Weekly time:09:00 timezone:America/New_York day:1→ succeeds/owner premium test-delete entitlement_id:<id>→ clean up/premium→ back to upsell ✅🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Documentation
Tests / Chores