Add comprehensive test coverage for poll, review, and moderation modules#333
Add comprehensive test coverage for poll, review, and moderation modules#333BillChirico merged 13 commits intomainfrom
Conversation
Analyzes 164 test files / 3,895 test cases across the codebase. Identifies 4 untested source files (pollHandler, reviewHandler, reputationDefaults, requireGlobalAdmin), 2 failing tests, and quality gaps in DB error handling, auth security edge cases, and concurrent operation testing. https://claude.ai/code/session_01UkSj9P9QCuJJCXAUGbGjti
… coverage tests - Fix logger.test.js: use constructor-compatible mocks for PostgresTransport and DailyRotateFile, remove nested vi.mock() calls - Fix ssrfProtection.test.js: skip DNS resolution in sandboxed env - Fix nested vi.mock() deprecation warnings in config-listeners.test.js, config.test.js, and config-events.test.js - Add pollHandler.test.js (29 tests): vote recording, multi-vote, DB transaction rollback, batch expiry - Add reviewHandler.test.js (30+ tests): embed building, claim logic, race conditions, stale review expiry - Add requireGlobalAdmin.test.js (8 tests): all auth method branches - Add reputationDefaults.test.js (7 tests): structure/value validation - Add moderation.coverage.test.js: DB failure paths for createCase, checkEscalation, tempban scheduler - Add warningEngine.coverage.test.js: DB failure paths for all CRUD functions, calculateExpiry edge cases, scheduler failure - Enhance auth.coverage.test.js: CSRF state reuse prevention, DASHBOARD_URL validation edge cases, rate limit bypass for test stability - Enhance index.test.js: startup failure recovery tests for initDb, registerEventHandlers, loadConfig, invalid state file, mem0 timeout All 196 test files pass (3998 tests, 0 failures). https://claude.ai/code/session_01UkSj9P9QCuJJCXAUGbGjti
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR adds comprehensive test coverage across poll/review handlers, warning/moderation engines, and middleware while refactoring test infrastructure by converting nested Vitest mocks to hoisted top-level declarations, making transport mocks constructor-safe, and documenting coverage baselines (242 test files, 85% threshold) alongside identified testing gaps. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-333
|
🚅 Deployed to the volvox-bot-pr-333 environment in volvox-bot
|
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-333
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the bot’s Vitest coverage, primarily by adding new test suites for previously untested modules (polls, reviews, warnings/moderation failure paths) and by tightening coverage around security/auth and startup edge cases.
Changes:
- Added new module test suites for
pollHandler,reviewHandler,warningEngine/moderationDB failure paths,reputationDefaults, andrequireGlobalAdmin. - Improved/expanded existing tests (auth route edge cases, startup failure recovery, logger mocking reliability, config listener mocking, SSRF tests avoiding real DNS).
- Added a coverage analysis doc summarizing current coverage/failure/deprecation status.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/modules/warningEngine.coverage.test.js | Adds DB failure-path tests + scheduler coverage for warningEngine |
| tests/modules/reviewHandler.test.js | Adds comprehensive tests for review embed/claim/expiry/message updates |
| tests/modules/reputationDefaults.test.js | Adds basic validation tests for static defaults |
| tests/modules/pollHandler.test.js | Adds comprehensive tests for poll embed/buttons/voting/expiry |
| tests/modules/moderation.coverage.test.js | Adds moderation DB failure-path tests + tempban scheduler coverage |
| tests/modules/config.test.js | Removes nested re-mocking after resetModules() (addresses Vitest warnings) |
| tests/modules/config-events.test.js | Removes nested re-mocking after resetModules() (addresses Vitest warnings) |
| tests/logger.test.js | Fixes transport mocks to be constructible; removes nested vi.mock() usage |
| tests/index.test.js | Adds startup failure recovery tests (initDb/register handlers/loadConfig/state parsing) |
| tests/config-listeners.test.js | Fixes mocking layout (cache mock moved to top-level) |
| tests/api/utils/ssrfProtection.test.js | Avoids real DNS lookups by disabling DNS checks in async validator tests |
| tests/api/routes/auth.coverage.test.js | Adds CSRF state reuse + DASHBOARD_URL edge-case tests; bypasses rate limiting |
| tests/api/middleware/requireGlobalAdmin.test.js | Adds middleware tests for api-secret/oauth/unknown branches + 4-arg form |
| docs/test-coverage-analysis.md | Adds a coverage analysis document and recommendations |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acf8b5acdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
| Filename | Overview |
|---|---|
| tests/modules/pollHandler.test.js | Comprehensive new test file covering buildPollEmbed, buildPollButtons, handlePollVote, closePoll, and closeExpiredPolls. Mock setup is correct (constructor-safe ButtonBuilder/EmbedBuilder mocks), transaction sequences match the source, and edge cases like null votes, out-of-range indices, and multi-vote toggling are all well exercised. |
| tests/modules/reviewHandler.test.js | Comprehensive new test file covering buildReviewEmbed, buildClaimButton, updateReviewMessage, handleReviewClaim, and expireStaleReviews. Field-truncation bounds match the source (197+1=198 and 497+1=498 chars). Self-claim prevention, race-condition protection, and thread creation failure paths are all correctly tested. |
| tests/modules/moderation.coverage.test.js | New DB-failure path test file for moderation.js. Transaction rollback sequences and 42P01 escalation fallback are well exercised. Minor issue: the checkEscalation tests use warns as the threshold key in inline configs while the beforeEach mock uses count, creating a misleading discrepancy (does not cause test failures because no test exercises a threshold-trigger scenario). |
| tests/modules/warningEngine.coverage.test.js | New DB-failure coverage test file for warningEngine.js. All CRUD error paths, calculateExpiry edge cases, and scheduler failure handling are correctly verified. The editWarning null-return test correctly sets up two query mocks (SELECT + UPDATE) matching the actual source which always runs both queries regardless of SELECT result. |
| tests/api/routes/auth.coverage.test.js | Adds CSRF state-reuse prevention tests and DASHBOARD_URL edge-case validation (HTTP evil host, localhost, empty URL, fragment stripping). Rate-limit middleware is correctly bypassed to prevent hitting the 10-request cap during callback tests. |
| tests/index.test.js | Adds five new startup-failure recovery tests (initDb rejection, invalid JSON state file, mem0 health-check timeout, registerEventHandlers exception, loadConfig failure). The checkMem0HealthImpl parameter is pre-existing infrastructure and the mem0 timeout test correctly uses fake timers advanced by 10 s. |
| tests/logger.test.js | Fixes two constructor-safety bugs: winston-daily-rotate-file and PostgresTransport mocks switched from arrow functions to function keyword so they are new-able. Nested vi.mock() calls inside beforeEach/it blocks removed, resolving Vitest deprecation warnings. |
Sequence Diagram
sequenceDiagram
participant T as Test
participant H as handlePollVote
participant P as pool.connect()
participant C as client (connection)
participant DB as DB (mocked)
participant M as message.edit
T->>H: handlePollVote(interaction)
H->>P: pool.connect()
P-->>H: client
H->>C: query('BEGIN')
C->>DB: BEGIN
DB-->>C: ok
H->>C: query('SELECT ... FOR UPDATE')
C->>DB: SELECT poll
alt Poll not found / closed / expired
DB-->>C: rows=[] or closed poll
H->>C: query('ROLLBACK')
H->>T: safeReply(error)
else Valid vote
DB-->>C: rows=[poll]
H->>C: query('UPDATE polls SET votes=...')
C->>DB: UPDATE
DB-->>C: ok
H->>C: query('COMMIT')
C->>DB: COMMIT
DB-->>C: ok
H->>M: message.edit(embed)
H->>T: safeReply(confirmation)
end
Note over H,C: finally: client.release()
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/modules/moderation.coverage.test.js
Line: 981-998
Comment:
**Inconsistent threshold key name vs. `beforeEach` mock**
The `checkEscalation` tests in this file use two different key names for escalation thresholds. The `beforeEach` mock at the top of the describe block uses `count`:
```js
thresholds: [{ count: 3, action: 'timeout', duration: '1h' }]
```
But the config object constructed inline here uses `warns`:
```js
thresholds: [{ warns: 3, withinDays: 7, action: 'timeout', duration: '1h' }]
```
These tests happen to pass because no scenario in this file exercises a code path where the threshold value would be read (the count is always 0 or the error is thrown before any threshold check). However, the inconsistency can mislead future contributors about the actual shape of the escalation config. It's worth aligning both configs to the correct key name used by `src/modules/moderation.js`.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Merge branch 'main' ..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-333
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@claude review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-333
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/test-coverage-analysis.md`:
- Around line 64-73: Update the "Database Error Handling" backlog section in
docs/test-coverage-analysis.md to reflect work already completed in this PR:
create a new "Done in this PR" subsection and move the completed items
(DB-failure coverage, tests for pollHandler, reviewHandler, and
requireGlobalAdmin suites) into it, then restart numbering under each priority
heading so lists are sequential (fixing the MD029 duplicate numbering warning);
ensure the affected modules list (ai.js, triage.js, moderation.js,
warningEngine.js, pollHandler.js, reviewHandler.js) is adjusted to only show
remaining work and update any references to the original backlog items.
- Line 9: Update the incorrect coverage statement that claims an 85% threshold
for branches: replace the mixed wording so the doc lists the correct baseline as
statements 85%, branches 82%, functions 85%, lines 85%, and clarify that the
“85% across all metrics” statement applies only to the web/tests suite (not the
bot-suite); edit the Vitest coverage sentence in docs/test-coverage-analysis.md
to explicitly state those four thresholds and note that thresholds must not be
lowered.
In `@tests/modules/pollHandler.test.js`:
- Around line 292-325: The tests assert that handlePollVote sends a success
reply/edit after the transaction, but handlePollVote currently returns
immediately after 'COMMIT' and client.release() without calling safeReply() or
interaction.message.edit(); update the tests (not the implementation) to reflect
actual behavior by asserting the transaction sequence and release instead of
reply/edit — e.g., for functions in this diff reference handlePollVote,
mockClient.query (ensure 'COMMIT' was called), and verify client.release() was
invoked and that safeReply and interaction.message.edit were NOT called (or
remove those expectations), so the assertions match the handler's actual flow.
In `@tests/modules/reviewHandler.test.js`:
- Line 156: Replace the identity flatMap usage and string concatenation in the
test assertions: change calls to embed.addFields.mock.calls.flatMap((c) => c) to
embed.addFields.mock.calls.flat() in each occurrence (look for usages around the
variable allFields in reviewHandler.test.js and similar variables at the listed
lines), and convert any string concatenation (e.g., "prefix " + var + " suffix")
to template literals (e.g., `prefix ${var} suffix`) so lint warnings are
resolved while preserving the same values and test assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a66f870-99f6-4c37-8924-609ebd2e7607
📒 Files selected for processing (14)
docs/test-coverage-analysis.mdtests/api/middleware/requireGlobalAdmin.test.jstests/api/routes/auth.coverage.test.jstests/api/utils/ssrfProtection.test.jstests/config-listeners.test.jstests/index.test.jstests/logger.test.jstests/modules/config-events.test.jstests/modules/config.test.jstests/modules/moderation.coverage.test.jstests/modules/pollHandler.test.jstests/modules/reputationDefaults.test.jstests/modules/reviewHandler.test.jstests/modules/warningEngine.coverage.test.js
💤 Files with no reviewable changes (2)
- tests/modules/config-events.test.js
- tests/modules/config.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESM-only syntax:
import/export, neverrequire()/module.exports
Files:
tests/config-listeners.test.jstests/api/utils/ssrfProtection.test.jstests/logger.test.jstests/api/middleware/requireGlobalAdmin.test.jstests/modules/reputationDefaults.test.jstests/index.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.jstests/modules/reviewHandler.test.jstests/modules/pollHandler.test.js
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,tsx}: Use single quotes for strings (except in JSON files); no double quotes
Always include semicolons at the end of statements
Use 2-space indentation (spaces, not tabs)
Always include trailing commas in multi-line arrays, objects, and function parameters
Maintain a maximum line width of 100 characters
Files:
tests/config-listeners.test.jstests/api/utils/ssrfProtection.test.jstests/logger.test.jstests/api/middleware/requireGlobalAdmin.test.jstests/modules/reputationDefaults.test.jstests/index.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.jstests/modules/reviewHandler.test.jstests/modules/pollHandler.test.js
tests/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.test.js: Write bot tests using Vitest 4 with thenodeenvironment, matching thesrc/structure in thetests/directory
Maintain test coverage thresholds: statements 85%, branches 82%, functions 85%, lines 85%; never lower thresholds—add tests to cover new code instead
Files:
tests/config-listeners.test.jstests/api/utils/ssrfProtection.test.jstests/logger.test.jstests/api/middleware/requireGlobalAdmin.test.jstests/modules/reputationDefaults.test.jstests/index.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.jstests/modules/reviewHandler.test.jstests/modules/pollHandler.test.js
**/*.{js,ts,tsx,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,mjs}: Use ESM syntax (import/export) — CommonJS is not allowed
Use single quotes for strings — double quotes only allowed in JSON files
Always include semicolons at end of statements
Use 2-space indentation for all code
Use Winston logger fromsrc/logger.js— never useconsole.*methods
Files:
tests/config-listeners.test.jstests/api/utils/ssrfProtection.test.jstests/logger.test.jstests/api/middleware/requireGlobalAdmin.test.jstests/modules/reputationDefaults.test.jstests/index.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.jstests/modules/reviewHandler.test.jstests/modules/pollHandler.test.js
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to tests/**/*.test.js : Maintain test coverage thresholds: statements 85%, branches 82%, functions 85%, lines 85%; never lower thresholds—add tests to cover new code instead
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to web/tests/**/*.test.{ts,tsx} : Maintain test coverage thresholds of 85% across all metrics (statements, branches, functions, lines) for web dashboard tests
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T02:03:52.709Z
Learning: Maintain 80% test coverage threshold — never lower the coverage requirement
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to tests/**/*.test.js : Write bot tests using Vitest 4 with the `node` environment, matching the `src/` structure in the `tests/` directory
Applied to files:
tests/config-listeners.test.jsdocs/test-coverage-analysis.mdtests/logger.test.jstests/api/middleware/requireGlobalAdmin.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.jstests/modules/reviewHandler.test.jstests/modules/pollHandler.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to tests/**/*.test.js : Maintain test coverage thresholds: statements 85%, branches 82%, functions 85%, lines 85%; never lower thresholds—add tests to cover new code instead
Applied to files:
docs/test-coverage-analysis.mdtests/modules/reputationDefaults.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to web/tests/**/*.test.{ts,tsx} : Maintain test coverage thresholds of 85% across all metrics (statements, branches, functions, lines) for web dashboard tests
Applied to files:
docs/test-coverage-analysis.mdtests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to web/tests/**/*.test.{ts,tsx} : Write web dashboard tests using Vitest 4 with the `jsdom` environment and React Testing Library, matching the `web/src/` structure
Applied to files:
docs/test-coverage-analysis.mdtests/logger.test.jstests/api/middleware/requireGlobalAdmin.test.jstests/modules/reputationDefaults.test.jstests/api/routes/auth.coverage.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.jstests/modules/reviewHandler.test.jstests/modules/pollHandler.test.js
📚 Learning: 2026-03-12T02:03:52.709Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T02:03:52.709Z
Learning: Maintain 80% test coverage threshold — never lower the coverage requirement
Applied to files:
docs/test-coverage-analysis.md
📚 Learning: 2026-03-12T02:03:52.709Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T02:03:52.709Z
Learning: Applies to **/*.{js,ts,tsx,mjs} : Use Winston logger from `src/logger.js` — never use `console.*` methods
Applied to files:
tests/logger.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to src/**/*.js : Never use `console.*` methods; use the Winston logger instead via `import logger from '../logger.js'` (adjust path as needed), then call `logger.info()`, `logger.warn()`, `logger.error()`, or `logger.debug()`
Applied to files:
tests/logger.test.jstests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to web/src/app/dashboard/**/*.tsx : For dashboard routes, add a matcher entry to `dashboardTitleMatchers` in `web/src/lib/page-titles.ts`: use exact equality for leaf routes (`pathname === '/dashboard/my-route'`) and subtree checks (`pathname.startsWith('/dashboard/my-route/')`); export `metadata` using `createPageMetadata(title)` for SSR entry points
Applied to files:
tests/api/routes/auth.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to src/api/routes/**/*.js : Create API route handlers in `src/api/routes/` and mount them in `src/api/server.js`; add auth middleware if the endpoint requires authentication
Applied to files:
tests/api/routes/auth.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to web/src/**/*.{ts,tsx} : Never use `console.*` methods in web dashboard code; use appropriate logging mechanisms for React applications
Applied to files:
tests/modules/warningEngine.coverage.test.jstests/modules/moderation.coverage.test.js
📚 Learning: 2026-03-10T23:21:49.730Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to src/api/routes/*.js : Add adaptDeleteGuildIdParam in API route handlers to enforce guild moderation checks on DELETE requests without losing record id
Applied to files:
tests/modules/warningEngine.coverage.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to src/modules/**/*.js : Create feature modules in `src/modules/` and add corresponding config sections to `config.json`
Applied to files:
tests/modules/warningEngine.coverage.test.js
🪛 GitHub Actions: CI
tests/modules/reviewHandler.test.js
[warning] 156-156: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
[warning] 165-165: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
[warning] 173-173: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
[warning] 181-181: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
[warning] 188-188: Biome lint/style/useTemplate (FIXABLE): Template literals are preferred over string concatenation. Offending expression: 'https://github.com/' + 'a'.repeat(300).
[warning] 190-190: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
[warning] 198-198: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
🪛 GitHub Check: SonarCloud Code Analysis
tests/api/routes/auth.coverage.test.js
[warning] 395-395: Move function 'setupOAuthEnv' to the outer scope.
[warning] 402-402: Move function 'mockSuccessfulOAuth' to the outer scope.
tests/modules/warningEngine.coverage.test.js
[failure] 102-102: Unexpected await of a non-Promise (non-"Thenable") value.
[failure] 135-135: Unexpected await of a non-Promise (non-"Thenable") value.
tests/modules/moderation.coverage.test.js
[failure] 171-171: Unexpected await of a non-Promise (non-"Thenable") value.
[failure] 205-205: Unexpected await of a non-Promise (non-"Thenable") value.
tests/modules/reviewHandler.test.js
[warning] 165-165: Prefer Array#flat() over Array#flatMap() to flatten an array.
[warning] 156-156: Prefer Array#flat() over Array#flatMap() to flatten an array.
[warning] 190-190: Prefer Array#flat() over Array#flatMap() to flatten an array.
[warning] 198-198: Prefer Array#flat() over Array#flatMap() to flatten an array.
[warning] 206-206: Prefer Array#flat() over Array#flatMap() to flatten an array.
[warning] 181-181: Prefer Array#flat() over Array#flatMap() to flatten an array.
[warning] 173-173: Prefer Array#flat() over Array#flatMap() to flatten an array.
🪛 LanguageTool
docs/test-coverage-analysis.md
[style] ~123-~123: Consider using a different verb for a more formal wording.
Context: ...mary of Recommendations ### Immediate (fix existing issues) 1. **Fix 2 failing te...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.21.0)
docs/test-coverage-analysis.md
[warning] 130-130: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/2/3
(MD029, ol-prefix)
[warning] 131-131: Ordered list item prefix
Expected: 2; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
[warning] 132-132: Ordered list item prefix
Expected: 3; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 133-133: Ordered list item prefix
Expected: 4; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 137-137: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 138-138: Ordered list item prefix
Expected: 2; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 139-139: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 140-140: Ordered list item prefix
Expected: 4; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
[warning] 144-144: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
[warning] 145-145: Ordered list item prefix
Expected: 2; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
🔇 Additional comments (14)
tests/logger.test.js (5)
23-29: Good constructor-safewinston-daily-rotate-filemock.This change correctly makes the mock
new-able and includes the requiredlogsurface expected by Winston transports.
33-39: Postgres transport mock now matches constructor usage well.Using a function-based mock with
on,log, andcloseis a solid fix for transport lifecycle tests.
120-125: Nice module-reset pattern for config-driven logger imports.Mutating the already-mocked
node:fsexports aftervi.resetModules()keeps the test deterministic and avoids repeated mock wiring.
134-137: Good parse-error path coverage setup.Using invalid JSON via mocked
readFileSynccleanly exercises the config parse failure path.
118-137: 🧹 Nitpick | 🔵 TrivialUpdate the shared mock guidance comment to match this new test pattern.
These tests now reconfigure existing hoisted mocks instead of re-declaring per-test
vi.mock()calls. Aligning the header instructions will prevent future confusion.⛔ Skipped due to learnings
Learnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2026-03-12T02:03:36.493Z Learning: Applies to tests/**/*.test.js : Write bot tests using Vitest 4 with the `node` environment, matching the `src/` structure in the `tests/` directorytests/config-listeners.test.js (1)
18-20: Top-level cache mock hoisting is correct.This change aligns with the resetModules + dynamic import pattern and keeps
cacheDelPatternconsistently mocked.tests/modules/reputationDefaults.test.js (1)
1-40: Good baseline invariant coverage for reputation defaults.The suite validates critical default-shape assumptions and guards against accidental config regressions.
tests/api/routes/auth.coverage.test.js (2)
15-24: Rate-limit bypass mock is well-scoped for callback coverage.It isolates route behavior without test flakiness from request caps.
359-467: CSRF reuse and DASHBOARD_URL edge-case coverage is strong.These additions materially improve security and redirect-path confidence.
tests/api/utils/ssrfProtection.test.js (1)
245-246: Deterministic async SSRF validation test setup looks good.Disabling DNS resolution here is appropriate for stable unit coverage.
tests/api/middleware/requireGlobalAdmin.test.js (1)
1-153: Comprehensive middleware branch coverage.The suite exercises expected auth-method paths and overload behavior cleanly.
tests/index.test.js (1)
243-244: Startup failure-path coverage additions are well-implemented.The injectable failure hooks and recovery assertions materially improve confidence in boot-time error handling.
Also applies to: 279-317, 798-886
tests/modules/reviewHandler.test.js (1)
1-155: Excellent behavioral coverage across review workflows.The suite thoroughly covers success paths and failure modes (race conditions, self-claim guard, stale expiry, and graceful degradation).
Also applies to: 211-465
tests/modules/moderation.coverage.test.js (1)
1-228: DB failure-path coverage is well targeted and high value.The rollback, fallback, and scheduler error tests align with real moderation resilience paths.
|
|
||
| ## Current State | ||
|
|
||
| The repository currently has **242 test files** covering bot runtime code, API routes, middleware, utilities, and the Next.js dashboard. The Vitest config enforces an **85% coverage threshold** across statements, branches, functions, and lines. |
There was a problem hiding this comment.
Correct the coverage threshold in Line 9.
Line 9 says Vitest enforces 85% for branches, but the bot-suite policy is 82% branches; 85% across all metrics only applies to web/tests. The current wording mixes the two suites and misstates the enforced baseline.
Based on learnings: "Maintain test coverage thresholds: statements 85%, branches 82%, functions 85%, lines 85%; never lower thresholds—add tests to cover new code instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/test-coverage-analysis.md` at line 9, Update the incorrect coverage
statement that claims an 85% threshold for branches: replace the mixed wording
so the doc lists the correct baseline as statements 85%, branches 82%, functions
85%, lines 85%, and clarify that the “85% across all metrics” statement applies
only to the web/tests suite (not the bot-suite); edit the Vitest coverage
sentence in docs/test-coverage-analysis.md to explicitly state those four
thresholds and note that thresholds must not be lowered.
| ### 1. Database Error Handling (All Modules) — **HIGH PRIORITY** | ||
|
|
||
| Almost every module uses fire-and-forget `pool.query()` calls where failures are caught and logged. None of the tests verify this behavior. If the DB connection drops mid-operation, we have no test confidence that: | ||
| - Transactions are properly rolled back | ||
| - Partial state isn't persisted | ||
| - The bot doesn't crash | ||
|
|
||
| **Affected modules:** `ai.js`, `triage.js`, `moderation.js`, `warningEngine.js`, `pollHandler.js`, `reviewHandler.js` | ||
|
|
||
| **Recommended action:** Add tests that mock `pool.query()` to reject, verifying rollback behavior and graceful error logging. |
There was a problem hiding this comment.
Refresh this backlog section to match the work already added here.
These sections still list DB-failure coverage and the pollHandler / reviewHandler / requireGlobalAdmin suites as follow-up work, even though this PR adds those tests. That makes the analysis stale, and the carried numbering is also what markdownlint MD029 is flagging in the recommendation lists. Move completed items into a “done in this PR” section and restart numbering under each priority heading.
Also applies to: 123-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/test-coverage-analysis.md` around lines 64 - 73, Update the "Database
Error Handling" backlog section in docs/test-coverage-analysis.md to reflect
work already completed in this PR: create a new "Done in this PR" subsection and
move the completed items (DB-failure coverage, tests for pollHandler,
reviewHandler, and requireGlobalAdmin suites) into it, then restart numbering
under each priority heading so lists are sequential (fixing the MD029 duplicate
numbering warning); ensure the affected modules list (ai.js, triage.js,
moderation.js, warningEngine.js, pollHandler.js, reviewHandler.js) is adjusted
to only show remaining work and update any references to the original backlog
items.
| it('should record a new vote (single-vote mode)', async () => { | ||
| const poll = makePoll(); | ||
| mockClient.query | ||
| .mockResolvedValueOnce(undefined) // BEGIN | ||
| .mockResolvedValueOnce({ rows: [poll] }) // SELECT | ||
| .mockResolvedValueOnce(undefined) // UPDATE | ||
| .mockResolvedValueOnce(undefined); // COMMIT | ||
|
|
||
| const interaction = makeInteraction(); | ||
| await handlePollVote(interaction); | ||
|
|
||
| expect(mockClient.query).toHaveBeenCalledWith('COMMIT'); | ||
| expect(safeReply).toHaveBeenCalledWith( | ||
| interaction, | ||
| expect.objectContaining({ content: expect.stringContaining('Voted for') }), | ||
| ); | ||
| }); | ||
|
|
||
| it('should toggle off an existing vote (single-vote mode)', async () => { | ||
| const poll = makePoll({ votes: { u1: [0] } }); | ||
| mockClient.query | ||
| .mockResolvedValueOnce(undefined) // BEGIN | ||
| .mockResolvedValueOnce({ rows: [poll] }) | ||
| .mockResolvedValueOnce(undefined) // UPDATE | ||
| .mockResolvedValueOnce(undefined); // COMMIT | ||
|
|
||
| const interaction = makeInteraction(); | ||
| await handlePollVote(interaction); | ||
|
|
||
| expect(safeReply).toHaveBeenCalledWith( | ||
| interaction, | ||
| expect.objectContaining({ content: expect.stringContaining('Vote removed') }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
These success-path assertions don’t match handlePollVote().
src/modules/pollHandler.js:114-209 currently returns after COMMIT and client.release(). There is no success-path safeReply() or interaction.message.edit() after the transaction, so the expectations in Lines 303-307, 321-324, and 385-390 are asserting behavior the handler does not implement.
Also applies to: 372-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/pollHandler.test.js` around lines 292 - 325, The tests assert
that handlePollVote sends a success reply/edit after the transaction, but
handlePollVote currently returns immediately after 'COMMIT' and client.release()
without calling safeReply() or interaction.message.edit(); update the tests (not
the implementation) to reflect actual behavior by asserting the transaction
sequence and release instead of reply/edit — e.g., for functions in this diff
reference handlePollVote, mockClient.query (ensure 'COMMIT' was called), and
verify client.release() was invoked and that safeReply and
interaction.message.edit were NOT called (or remove those expectations), so the
assertions match the handler's actual flow.
| it('should include language field when present', () => { | ||
| const review = makeReview({ language: 'Python' }); | ||
| const embed = buildReviewEmbed(review); | ||
| const allFields = embed.addFields.mock.calls.flatMap((c) => c); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lint-triggering patterns are removed from this file.
rg -n "flatMap\(\(c\)\s*=>\s*c\)|'https://github\.com/'\s*\+\s*'a'\.repeat\(300\)" tests/modules/reviewHandler.test.jsRepository: VolvoxLLC/volvox-bot
Length of output: 668
Replace flatMap((c) => c) with flat() and use template literals for string concatenation.
Multiple lines trigger Biome lint warnings. Replace identity flatMap calls and string concatenation with template literals for lint compliance.
Suggested diff
- const allFields = embed.addFields.mock.calls.flatMap((c) => c);
+ const allFields = embed.addFields.mock.calls.flat();
- const review = makeReview({ url: 'https://github.com/' + 'a'.repeat(300) });
+ const review = makeReview({ url: `https://github.com/${'a'.repeat(300)}` });Applies to lines: 156, 165, 173, 181, 190, 198, 206 (flatMap), 188 (string concatenation).
📝 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.
| const allFields = embed.addFields.mock.calls.flatMap((c) => c); | |
| const allFields = embed.addFields.mock.calls.flat(); |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 156-156: Biome lint/complexity/noFlatMapIdentity (FIXABLE): Avoid unnecessary callback in flatMap call. Replace flatMap((c) => c) with flat().
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 156-156: Prefer Array#flat() over Array#flatMap() to flatten an array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/reviewHandler.test.js` at line 156, Replace the identity
flatMap usage and string concatenation in the test assertions: change calls to
embed.addFields.mock.calls.flatMap((c) => c) to
embed.addFields.mock.calls.flat() in each occurrence (look for usages around the
variable allFields in reviewHandler.test.js and similar variables at the listed
lines), and convert any string concatenation (e.g., "prefix " + var + " suffix")
to template literals (e.g., `prefix ${var} suffix`) so lint warnings are
resolved while preserving the same values and test assertions.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-333
| ### Recently Resolved Test Failures | ||
|
|
||
| | Test File | Previous Failure | Root Cause | Status in This PR | | ||
| |-----------|---------|------------| |
There was a problem hiding this comment.
The markdown table under “Recently Resolved Test Failures” has 4 columns in the header row but only 3 columns in the separator row, which can render incorrectly in some markdown viewers. Update the separator row to include the same number of columns as the header (4).
| |-----------|---------|------------| | |
| |-----------|-------------------|-----------|-------------------| |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BillChirico
added
javascript
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-333
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🧹 Preview Environment Cleaned UpThe Railway preview environment for this PR has been removed. Environment: |
Summary
This PR adds extensive test coverage for three previously untested modules (
pollHandler.js,reviewHandler.js, andreputationDefaults.js) and improves existing test suites with additional edge case and failure path coverage. It also fixes test infrastructure issues and documents coverage gaps.Key Changes
New Test Files
tests/modules/pollHandler.test.js(~471 lines): Comprehensive tests for poll creation, voting, expiry, and embed/button buildingtests/modules/reviewHandler.test.js(~465 lines): Full coverage of code review request handlingtests/modules/moderation.coverage.test.js(~229 lines): Database failure path coveragetests/modules/warningEngine.coverage.test.js(~200 lines): Database error handling for warning operationstests/api/middleware/requireGlobalAdmin.test.js(~153 lines): Security middleware teststests/modules/reputationDefaults.test.js(~40 lines): Static config validationEnhanced Existing Tests
tests/api/routes/auth.coverage.test.js: Added CSRF state reuse prevention and DASHBOARD_URL validation edge casestests/index.test.js: Added startup failure recovery tests forinitDbrejection andregisterEventHandlersexceptionstests/logger.test.js: Fixed mock constructors to usefunctionkeyword instead of arrow functions (required fornewoperator); fixed nestedvi.mock()deprecation warningstests/config-listeners.test.js: Added cache utility mock; removed nestedvi.mock()callstests/modules/config.test.js: Removed nestedvi.mock()callstests/modules/config-events.test.js: Removed nestedvi.mock()callstests/api/utils/ssrfProtection.test.js: Minor test refinementDocumentation
docs/test-coverage-analysis.md: New analysis document detailing:Notable Implementation Details
query()calls to simulate transaction sequences (BEGIN → SELECT → UPDATE → COMMIT/ROLLBACK)EmbedBuilder,ButtonBuilder, andActionRowBuilderwith chainable methods and component trackingfunctionkeyword instead of arrow functions, enabling propernewoperator semanticsrateLimitmiddleware in auth coverage tests to prevent hitting request caps during callback testingCoverage Impact
These additions bring the project closer to the 85% coverage threshold enforced by Vitest config,
https://claude.ai/code/session_01UkSj9P9QCuJJCXAUGbGjti