feat: migrate session store to Redis (#79)#94
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds optional Redis-backed session storage (enabled via REDIS_URL), introduces a lazy Redis client, converts session-related middleware and routes to async to await session operations, updates shutdown to close Redis, and adds tests for the hybrid session store and async middleware adjustments. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Request timed out after 900000ms (requestId=bbef9db7-7a77-4da7-8f18-bed48f0d77d4) |
|
| Filename | Overview |
|---|---|
| src/api/utils/redisClient.js | New Redis client with lazy initialization, proper error handling, and graceful shutdown |
| src/api/utils/sessionStore.js | Hybrid Redis/in-memory session store with automatic TTL handling and backward compatibility |
| src/api/middleware/verifyJwt.js | Made async, added proper error handling for Redis session lookup with logging |
| src/api/routes/auth.js | Converted session operations to async with error handling in /me and /logout routes |
| src/api/routes/guilds.js | Added async session token retrieval with try-catch error handling and logging |
| src/index.js | Added Redis cleanup to graceful shutdown sequence with error handling |
| tests/api/utils/sessionStore.test.js | Comprehensive test coverage for both Redis and in-memory session store modes |
| .env.example | Added REDIS_URL configuration with documentation |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[API Request] --> B{Auth Method?}
B -->|API Secret| C[requireAuth passes]
B -->|OAuth JWT| D[verifyJwtToken]
D --> E{JWT Valid?}
E -->|No| F[Return 401]
E -->|Yes| G[getSessionToken userId]
G --> H{Redis Configured?}
H -->|Yes| I[Redis GET session:userId]
H -->|No| J[In-Memory Map GET]
I --> K{Session Found?}
J --> K
K -->|No| L[Return 401 Session Expired]
K -->|Yes| M[Attach user to req]
M --> N[Route Handler]
N --> O[OAuth Callback Success]
O --> P{Redis Configured?}
P -->|Yes| Q[Redis SETEX session:userId 3600s]
P -->|No| R[In-Memory Map SET with TTL]
Q --> S[Return JWT to Client]
R --> S
style I fill:#ff6b6b
style Q fill:#ff6b6b
style J fill:#4ecdc4
style R fill:#4ecdc4
Last reviewed commit: 89ee18b
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/api/middleware/oauth.js`:
- Around line 15-17: The middleware returned by the function should await
handleOAuthJwt for consistency with requireAuth; update the anonymous async
middleware in oauth.js to call await handleOAuthJwt(req, res, next, {
missingTokenError: 'No token provided' }) instead of returning its promise so
the async flow matches requireAuth() and is clearer when inspecting functions
like handleOAuthJwt and requireAuth.
In `@src/api/middleware/verifyJwt.js`:
- Around line 41-59: verifyJwtToken currently awaits
getSessionToken(decoded.userId) without catching rejections; wrap the session
check in a try/catch inside verifyJwtToken so Redis/client errors are
handled—call await getSessionToken(decoded.userId) inside the try, return the
existing { error: 'Session expired or revoked', status: 401 } when the call
succeeds but returns falsy, and on catch return a 500-style error (e.g. { error:
'Session check failed', status: 500 }) so Redis failures don’t propagate as
unhandled exceptions.
In `@tests/api/utils/sessionStore.test.js`:
- Around line 80-88: The test is touching internals via
Map.prototype.set.call(sessionStore, ...) and Map.prototype.get.call(...);
replace that with the public sessionStore API and/or time mocking: create a
session using the public method (e.g., sessionStore.set / sessionStore.create /
sessionStore.save — whichever the module exposes), then use jest.useFakeTimers()
or mock Date.now() to make the session appear expired, call
sessionStore.cleanup(), and assert via the public getter (e.g.,
sessionStore.get) that the session is removed; do not call
Map.prototype.set.call or Map.prototype.get.call directly.
- Around line 96-103: The tests call getSessionToken('user4') and
getSessionToken('missing') synchronously but the contract requires callers to
await getSessionToken() (see sessionStore.getSessionToken); update both tests to
await the calls so they use the async contract consistently (e.g., change
expect(getSessionToken(...)) to expect(await getSessionToken(...))). Keep the
existing sessionStore.set('user4', 'tok4') usage and assertions otherwise
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.env.examplepackage.jsonsrc/api/middleware/auth.jssrc/api/middleware/oauth.jssrc/api/middleware/oauthJwt.jssrc/api/middleware/verifyJwt.jssrc/api/routes/auth.jssrc/api/routes/guilds.jssrc/api/utils/redisClient.jssrc/api/utils/sessionStore.jssrc/index.jstests/api/middleware/auth.test.jstests/api/middleware/oauth.test.jstests/api/utils/sessionStore.test.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — useimport/export, neverrequire()
Usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons
Use single quotes for strings
Use 2-space indentation
No TypeScript — use plain JavaScript with JSDoc comments for documentation
Files:
src/api/routes/guilds.jssrc/api/middleware/verifyJwt.jssrc/api/utils/redisClient.jstests/api/middleware/oauth.test.jssrc/api/routes/auth.jssrc/api/middleware/oauthJwt.jssrc/api/middleware/oauth.jstests/api/utils/sessionStore.test.jstests/api/middleware/auth.test.jssrc/api/utils/sessionStore.jssrc/index.jssrc/api/middleware/auth.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston for logging — import{ info, warn, error }from../logger.js
NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read config
UsesetConfigValue(path, value, guildId?)fromsrc/modules/config.jsto update config at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
UsesafeSend()wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Files:
src/api/routes/guilds.jssrc/api/middleware/verifyJwt.jssrc/api/utils/redisClient.jssrc/api/routes/auth.jssrc/api/middleware/oauthJwt.jssrc/api/middleware/oauth.jssrc/api/utils/sessionStore.jssrc/index.jssrc/api/middleware/auth.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must achieve at least 80% code coverage on statements, branches, functions, and lines
Files:
tests/api/middleware/oauth.test.jstests/api/utils/sessionStore.test.jstests/api/middleware/auth.test.js
🧬 Code graph analysis (10)
src/api/routes/guilds.js (2)
src/api/routes/auth.js (3)
accessToken(202-202)accessToken(272-272)user(222-222)src/api/utils/sessionStore.js (1)
getSessionToken(126-128)
src/api/middleware/verifyJwt.js (3)
src/api/routes/auth.js (1)
token(235-243)web/src/lib/auth.ts (1)
jwt(132-166)src/api/utils/sessionStore.js (1)
getSessionToken(126-128)
src/api/utils/redisClient.js (1)
src/logger.js (1)
warn(237-239)
tests/api/middleware/oauth.test.js (1)
src/api/middleware/oauth.js (1)
requireOAuth(14-18)
src/api/routes/auth.js (1)
src/api/utils/sessionStore.js (2)
sessionStore(115-115)sessionStore(115-115)
src/api/middleware/oauthJwt.js (1)
src/api/middleware/verifyJwt.js (1)
verifyJwtToken(41-60)
tests/api/middleware/auth.test.js (1)
src/api/middleware/auth.js (1)
requireAuth(36-70)
src/api/utils/sessionStore.js (1)
src/api/utils/redisClient.js (1)
getRedisClient(21-44)
src/index.js (1)
src/api/utils/redisClient.js (1)
closeRedis(52-62)
src/api/middleware/auth.js (1)
src/api/middleware/oauthJwt.js (1)
handleOAuthJwt(31-57)
🔇 Additional comments (16)
.env.example (1)
85-88: LGTM!The new Session Storage section is well-documented, follows the existing file conventions, and clearly explains the optional Redis configuration with in-memory fallback.
src/api/utils/redisClient.js (1)
1-71: LGTM!The Redis client implementation is well-structured with:
- Proper lazy initialization pattern
- Graceful error handling that doesn't crash the process
- Clean shutdown via
quit()with state reset- Test helper for resetting internal state
src/index.js (2)
23-23: LGTM!Import is correctly placed with other utility imports.
290-295: LGTM!Redis shutdown is properly integrated into the graceful shutdown sequence. The placement after database closure is appropriate, and the try/catch provides additional safety even though
closeRedis()handles errors internally.src/api/utils/sessionStore.js (1)
37-113: LGTM with a note on return type consistency.The dual-backend implementation is well-designed. The JSDoc correctly documents that callers should
awaitregardless of backend.Note: Return types differ between backends (e.g.,
deletereturnsPromise<number>for Redis vsbooleanfor in-memory). This is acceptable since callers don't appear to depend on the return values, but worth keeping in mind for future maintenance.src/api/routes/guilds.js (2)
174-176: LGTM!The function was already async; now it correctly awaits
getSessionTokento handle the Redis-backed session store.
310-310: LGTM!Correctly awaits
getSessionTokento support the async Redis-backed session store.src/api/routes/auth.js (3)
232-232: LGTM!Correctly awaits
sessionStore.setafter successful OAuth token exchange. Any Redis errors will be caught by the existing try/catch block.
272-272: LGTM!Correctly awaits
sessionStore.getto retrieve the access token for the authenticated user.
294-296: LGTM!Route handler correctly made async and awaits
sessionStore.deleteto invalidate the session.package.json (1)
29-29: No action needed. The ioredis version 5.9.3 is valid and has no known security vulnerabilities.src/api/middleware/oauthJwt.js (1)
31-57: LGTM!The async conversion is correctly implemented. The function now properly awaits
verifyJwtToken(token)and the JSDoc return type is appropriately updated toPromise<boolean>. Logging uses Winston with structured metadata as required.src/api/middleware/auth.js (1)
37-69: LGTM!The async conversion is well-implemented. The inner middleware correctly awaits
handleOAuthJwtand the control flow remains correct—returning early when OAuth JWT handling is complete, or falling through to the unauthorized response.tests/api/middleware/auth.test.js (1)
62-192: LGTM!All test cases have been correctly converted to async and properly await the middleware invocation. The test coverage is comprehensive, covering API secret validation, JWT authentication, error cases, and the interaction between both auth methods.
tests/api/middleware/oauth.test.js (1)
35-135: LGTM!The test suite comprehensively covers OAuth middleware behavior including missing tokens, invalid tokens, expired tokens, missing server-side sessions, and successful authentication. All tests correctly use async/await for middleware invocation.
tests/api/utils/sessionStore.test.js (1)
1-181: Good test coverage for both session store backends.The test suite provides comprehensive coverage for the in-memory and Redis-backed session store paths, including set/get/has/delete operations, cleanup behavior, and the
getSessionTokenhelper. The mock setup withbuildRedisMock()is well-structured.
|
Claude finished @BillChirico's task in 3m 59s —— View job Code Review: PR #94 — Migrate session store to Redis
Verdict: Approved with nitpicksThis PR is well-implemented. All significant issues from prior reviews (CodeRabbit, Greptile, and the earlier Claude review) have been addressed — Redis error handling in routes, async middleware, verifyJwt session lookup try/catch, test robustness, and misleading error labels are all fixed. Three 🔵 Nitpick-level findings remain, none blocking merge: 1. AGENTS.md Key Files table not updated 2. 3. Missing test for Redis error propagation (inline comment) AI fix prompt (copy-paste to resolve all nitpicks) |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two route handlers in auth.js (/me and /logout) call async sessionStore methods without try/catch, meaning Redis failures will propagate as unhandled rejections. The verifyJwt.js session lookup is correctly wrapped — these two routes need the same treatment. Additionally, AGENTS.md Key Files table should be updated with the new/reworked files per project documentation rules.
- getRedisClient() returns ioredis instance when REDIS_URL is set, null otherwise - Connection errors logged via Winston, never crash the process - closeRedis() for graceful shutdown - _resetRedisClient() for test isolation
…ry store - Same interface: set/get/has/delete/cleanup - Redis path: SETEX with 1h TTL, key format session:<userId> - In-memory fallback when REDIS_URL is not set - get/has return Promises in Redis mode; callers must await - cleanup() is a no-op for Redis (TTL handles expiry) - Closes #79
- verifyJwtToken() → async, awaits getSessionToken() - handleOAuthJwt() → async Promise<boolean> - requireAuth() and requireOAuth() return async middleware - No functional change on in-memory path (await on sync value is transparent)
- auth.js: await sessionStore.set/get/delete for callback, /me, logout - guilds.js: await getSessionToken() in hasOAuthGuildPermission and GET /
…S_URL - src/index.js: closeRedis() called after closeDb() during SIGTERM/SIGINT - .env.example: documented REDIS_URL (commented out, optional)
…sync - tests/api/utils/sessionStore.test.js: 17 tests covering both in-memory and Redis paths - tests/api/middleware/auth.test.js: all tests now async (await middleware()) - tests/api/middleware/oauth.test.js: all tests now async (await middleware())
- Add await to oauth.js middleware for consistency with requireAuth - Wrap getSessionToken in try/catch for Redis error resilience (503) - Replace Map.prototype.set.call with vi.useFakeTimers for cleanup test - Add await to getSessionToken test calls per documented contract
BillChirico
force-pushed
the
feat/redis-sessions
branch
from
5983344 to
250dd29
Compare
There was a problem hiding this comment.
Two remaining issues to address:
🟡 guilds.js:184-191 — hasOAuthGuildPermission catch block labels all errors as "Redis error" but the try block also wraps fetchUserGuilds() (Discord API). This creates misleading log entries when Discord fails. Additionally, the error is double-logged since the caller requireGuildPermission (line 250) also catches and logs. Fix: either rename the error message, or remove the catch block entirely and let the caller handle it.
🔵 AGENTS.md — Per project documentation rules ("Added a new module → update Key Files table"), src/api/utils/redisClient.js (new) and src/api/utils/sessionStore.js (reworked with Redis backend) should be added to the Key Files table.
Everything else from prior reviews looks properly addressed — Redis error handling in routes, async middleware, test improvements, and verifyJwt.js session lookup try/catch are all solid.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — all significant issues from prior reviews have been addressed. Three nitpicks remain (documented in inline comment and PR comment): AGENTS.md Key Files table update, REDIS_URL in SENSITIVE_FIELDS, and a missing Redis error propagation test. None block merge.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/routes/auth.js (1)
231-263: 🧹 Nitpick | 🔵 TrivialNice: session-store operations are awaited + 503 handling is explicit; minor log hardening needed.
The new try/catch around session retrieval/deletion is a solid resilience improvement. Small follow-up: avoid assuming
err.messageis present (non-Errorthrows/rejections can make the logger throw).Suggested patch
} catch (err) { - error('OAuth2 callback error', { error: err.message }); + const errMsg = err instanceof Error ? err.message : String(err); + error('OAuth2 callback error', { error: errMsg }); res.status(500).json({ error: 'Authentication failed' }); } @@ try { accessToken = await sessionStore.get(userId); } catch (err) { - error('Redis error fetching session in /me', { error: err.message, userId }); + const errMsg = err instanceof Error ? err.message : String(err); + error('Redis error fetching session in /me', { error: errMsg, userId }); return res.status(503).json({ error: 'Session store unavailable' }); } @@ try { await sessionStore.delete(req.user.userId); } catch (err) { + const errMsg = err instanceof Error ? err.message : String(err); error('Redis error deleting session on logout', { - error: err.message, + error: errMsg, userId: req.user.userId, }); // User's intent is to log out — succeed anyway }Also applies to: 270-279, 301-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/auth.js` around lines 231 - 263, The catch blocks in src/api/routes/auth.js log err.message directly which can throw if a non-Error is thrown; update the error logging in the OAuth2 callback catch (and the similar catch blocks around lines 270-279 and 301-310) to safely extract and log the error by using a resilient expression like using err && err.message ? err.message : String(err) (or equivalent) when calling error('...', { error: ... }); ensure all instances reference the same safe extraction to avoid logger failures for non-Error rejections.tests/api/middleware/auth.test.js (1)
62-192:⚠️ Potential issue | 🟡 MinorMake these middleware tests hermetic against Redis (avoid relying on
sessionStore.clear()/ env state).Right now the suite imports
sessionStoredirectly and usessessionStore.clear()in teardown, which strongly assumes the in-memory backend. IfREDIS_URLis set in a dev/CI environment (or ifsessionStorechooses backend at runtime), these tests can become flaky or try to touch a real Redis instance.A simple fix is to mock
redisClientin this file (like you do intests/api/utils/sessionStore.test.js) sogetRedisClient()always returnsnull.Suggested patch
vi.mock('../../../src/logger.js', () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn() })); + +vi.mock('../../../src/api/utils/redisClient.js', () => ({ + getRedisClient: vi.fn().mockReturnValue(null), + closeRedis: vi.fn().mockResolvedValue(undefined), + _resetRedisClient: vi.fn(), +}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/middleware/auth.test.js` around lines 62 - 192, Mock the Redis client in the test so the sessionStore uses the in-memory backend regardless of env: before importing or requiring sessionStore/requireAuth, stub getRedisClient (or the module that exports redisClient) to always return null so sessionStore falls back to in-memory; ensure this mock is set up at the top of the test file (before any sessionStore/requireAuth import), and remove/avoid reliance on touching a real Redis instance (e.g., stop using sessionStore.clear() in teardown or ensure it operates on the mocked in-memory store).
🤖 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/api/middleware/verifyJwt.js`:
- Around line 42-69: The verifyJwtToken function assumes jwt.verify(token,
secret, ...) returns an object with userId; first validate that decoded is an
object and has a userId (e.g., typeof decoded === 'object' && decoded !== null
&& 'userId' in decoded) before using decoded.userId, and if that check fails
return { error: 'Invalid or expired token', status: 401 }; update the block
around jwt.verify and the subsequent getSessionToken usage to use this guard so
you never call getSessionToken or access decoded.userId on a string/Buffer or
null value, and keep the existing error paths (500 for missing secret, 401 for
invalid token, 503 for session lookup failure).
In `@src/api/routes/guilds.js`:
- Around line 174-191: The catch block in hasOAuthGuildPermission (and the
similar catch at lines 319-328) assumes err is an Error and accesses
err.message, which can throw if a non-Error is thrown; update both catch
handlers to safely stringify the error (e.g. use String(err) or a safe
serializer) and log that instead of err.message, and adjust the log text to
refer to "session store" when describing session lookup failures so the message
is accurate (targets: function hasOAuthGuildPermission and the corresponding
function around lines 319-328).
In `@src/api/utils/redisClient.js`:
- Around line 21-44: The function getRedisClient flips _initialized before
reading process.env.REDIS_URL which can permanently cache a null _client if
REDIS_URL is missing on first call; change the logic so you only set
_initialized (or set it after) when you actually decide to create and assign a
real _client — i.e., check redisUrl first, return null immediately if missing
without setting _initialized, and only mark _initialized = true (and assign
_client) when new Redis(...) succeeds in getRedisClient; also ensure the catch
block clears _initialized or leaves it false if creation fails so future calls
can retry.
In `@src/api/utils/sessionStore.js`:
- Around line 58-59: Normalize the Redis backend return values so they match the
in-memory semantics: in the Redis-backed get/getSession implementation convert
Redis null/empty-miss results to undefined (not null or other backend-specific
shape) and ensure the setter returns the same string type as the in-memory path;
in the Redis-backed delete/del/deleteSession (or destroySession) implementation
convert Redis numeric reply (0/1) to a boolean (false/true) and return that
boolean (not the raw number or other type); ensure all three locations
referenced (the get/getSession path around lines 58–59 and 92–93 and the
delete/del path around 124–127) perform these conversions and preserve Promise
return types so callers always see undefined for misses and boolean for deletes
across backends.
- Around line 45-49: Wrap each Redis operation in sessionStore.js (e.g., the set
method that uses getRedisClient() and constants SESSION_KEY_PREFIX and
SESSION_TTL_SECONDS, and the other methods at the indicated ranges) with a
try/catch that logs the error with contextual fields operation (e.g., "set",
"get", "delete", "refresh") and userId before re-throwing; locate the methods by
name (set, the other session-related methods around the provided ranges) and add
processLogger.error (or the module's logger) calls inside the catch that include
a descriptive message plus operation and userId, then rethrow the original
error.
---
Outside diff comments:
In `@src/api/routes/auth.js`:
- Around line 231-263: The catch blocks in src/api/routes/auth.js log
err.message directly which can throw if a non-Error is thrown; update the error
logging in the OAuth2 callback catch (and the similar catch blocks around lines
270-279 and 301-310) to safely extract and log the error by using a resilient
expression like using err && err.message ? err.message : String(err) (or
equivalent) when calling error('...', { error: ... }); ensure all instances
reference the same safe extraction to avoid logger failures for non-Error
rejections.
In `@tests/api/middleware/auth.test.js`:
- Around line 62-192: Mock the Redis client in the test so the sessionStore uses
the in-memory backend regardless of env: before importing or requiring
sessionStore/requireAuth, stub getRedisClient (or the module that exports
redisClient) to always return null so sessionStore falls back to in-memory;
ensure this mock is set up at the top of the test file (before any
sessionStore/requireAuth import), and remove/avoid reliance on touching a real
Redis instance (e.g., stop using sessionStore.clear() in teardown or ensure it
operates on the mocked in-memory store).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.env.examplepackage.jsonsrc/api/middleware/auth.jssrc/api/middleware/oauth.jssrc/api/middleware/oauthJwt.jssrc/api/middleware/verifyJwt.jssrc/api/routes/auth.jssrc/api/routes/guilds.jssrc/api/routes/health.jssrc/api/utils/redisClient.jssrc/api/utils/sessionStore.jssrc/api/ws/logStream.jssrc/db.jssrc/index.jssrc/logger.jssrc/modules/triage.jssrc/transports/sentry.jssrc/transports/websocket.jstests/api/middleware/auth.test.jstests/api/middleware/oauth.test.jstests/api/routes/config.test.jstests/api/routes/guilds.test.jstests/api/utils/configAllowlist.test.jstests/api/utils/sessionStore.test.jstests/api/utils/validateConfigPatch.test.jstests/api/ws/logStream.test.jstests/modules/triage-prompt.test.jstests/modules/triage-respond.test.jstests/sentry.test.jstests/transports/websocket.test.js
💤 Files with no reviewable changes (1)
- tests/api/routes/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). (2)
- GitHub Check: Greptile Review
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — useimport/export, neverrequire()
Usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons
Use single quotes for strings
Use 2-space indentation
No TypeScript — use plain JavaScript with JSDoc comments for documentation
Files:
src/api/ws/logStream.jstests/api/middleware/oauth.test.jstests/sentry.test.jssrc/transports/websocket.jssrc/modules/triage.jssrc/api/utils/redisClient.jstests/modules/triage-prompt.test.jssrc/api/routes/auth.jstests/api/routes/guilds.test.jssrc/api/routes/guilds.jssrc/api/middleware/auth.jssrc/api/utils/sessionStore.jstests/api/utils/sessionStore.test.jstests/api/middleware/auth.test.jssrc/api/middleware/verifyJwt.jssrc/api/middleware/oauth.jssrc/api/routes/health.jssrc/transports/sentry.jssrc/logger.jssrc/api/middleware/oauthJwt.jstests/api/utils/validateConfigPatch.test.jstests/transports/websocket.test.jstests/modules/triage-respond.test.jstests/api/ws/logStream.test.jssrc/db.jssrc/index.jstests/api/utils/configAllowlist.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston for logging — import{ info, warn, error }from../logger.js
NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read config
UsesetConfigValue(path, value, guildId?)fromsrc/modules/config.jsto update config at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
UsesafeSend()wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Files:
src/api/ws/logStream.jssrc/transports/websocket.jssrc/modules/triage.jssrc/api/utils/redisClient.jssrc/api/routes/auth.jssrc/api/routes/guilds.jssrc/api/middleware/auth.jssrc/api/utils/sessionStore.jssrc/api/middleware/verifyJwt.jssrc/api/middleware/oauth.jssrc/api/routes/health.jssrc/transports/sentry.jssrc/logger.jssrc/api/middleware/oauthJwt.jssrc/db.jssrc/index.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must achieve at least 80% code coverage on statements, branches, functions, and lines
Files:
tests/api/middleware/oauth.test.jstests/sentry.test.jstests/modules/triage-prompt.test.jstests/api/routes/guilds.test.jstests/api/utils/sessionStore.test.jstests/api/middleware/auth.test.jstests/api/utils/validateConfigPatch.test.jstests/transports/websocket.test.jstests/modules/triage-respond.test.jstests/api/ws/logStream.test.jstests/api/utils/configAllowlist.test.js
src/modules/triage.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/triage.js: Triage tracks at most 100 channels; channels inactive for 30 minutes are evicted from the buffer
Monitortotal_cost_usdin logs for triage budget limit violations; catch and log errors whenclassifyBudgetorrespondBudgetlimits are exceeded
Files:
src/modules/triage.js
🧠 Learnings (5)
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/**/*.js : Always use Winston for logging — import `{ info, warn, error }` from `../logger.js`
Applied to files:
src/transports/websocket.jssrc/logger.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/modules/triage.js : Monitor `total_cost_usd` in logs for triage budget limit violations; catch and log errors when `classifyBudget` or `respondBudget` limits are exceeded
Applied to files:
src/modules/triage.jstests/modules/triage-respond.test.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Update `AGENTS.md` whenever finding new pitfalls or adding features that affect AI agent context
Applied to files:
src/api/utils/sessionStore.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Update `config.json` documentation in README.md when adding new config sections or keys
Applied to files:
src/api/utils/sessionStore.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/commands/*.js : Slash commands must export both `data` (SlashCommandBuilder) and `execute(interaction)` function
Applied to files:
src/index.js
🧬 Code graph analysis (16)
src/api/ws/logStream.js (1)
tests/api/ws/logStream.test.js (2)
nonce(19-19)expiry(20-20)
tests/api/middleware/oauth.test.js (1)
src/api/middleware/oauth.js (1)
requireOAuth(14-18)
src/modules/triage.js (1)
src/logger.js (1)
warn(237-239)
src/api/routes/auth.js (4)
src/api/utils/sessionStore.js (2)
sessionStore(115-115)sessionStore(115-115)src/api/routes/guilds.js (8)
accessToken(176-176)accessToken(319-319)req(284-284)req(304-304)req(498-498)req(542-542)req(860-860)req(911-911)src/logger.js (1)
error(244-246)src/api/middleware/oauth.js (1)
requireOAuth(14-18)
tests/api/routes/guilds.test.js (3)
tests/api/routes/config.test.js (2)
app(46-46)SECRET(47-47)src/api/server.js (2)
app(28-28)app(99-99)tests/api/routes/auth.test.js (1)
app(19-19)
src/api/middleware/auth.js (1)
src/api/middleware/oauthJwt.js (1)
handleOAuthJwt(31-57)
src/api/utils/sessionStore.js (1)
src/api/utils/redisClient.js (1)
getRedisClient(21-44)
tests/api/middleware/auth.test.js (1)
src/api/middleware/auth.js (1)
requireAuth(36-70)
src/api/middleware/verifyJwt.js (1)
src/api/utils/sessionStore.js (1)
getSessionToken(126-128)
src/api/middleware/oauth.js (1)
src/api/middleware/oauthJwt.js (1)
handleOAuthJwt(31-57)
src/api/routes/health.js (2)
src/utils/restartTracker.js (1)
rows(106-106)src/utils/debugFooter.js (1)
r(305-305)
src/api/middleware/oauthJwt.js (1)
src/api/middleware/verifyJwt.js (1)
verifyJwtToken(42-69)
tests/transports/websocket.test.js (1)
src/logger.js (2)
transport(260-270)transport(299-306)
tests/modules/triage-respond.test.js (1)
src/modules/triage-respond.js (1)
buildStatsAndLog(249-277)
tests/api/ws/logStream.test.js (1)
src/logger.js (2)
transport(260-270)transport(299-306)
src/index.js (3)
src/logger.js (2)
error(244-246)info(230-232)src/api/utils/redisClient.js (1)
closeRedis(52-62)src/sentry.js (2)
sentryEnabled(22-22)sentryEnabled(22-22)
🔇 Additional comments (29)
src/modules/triage.js (1)
192-195: Looks good — structured warning log preserved.This formatting change keeps Winston logging and structured metadata intact, with no behavior change.
src/transports/websocket.js (1)
8-9: LGTM!Import reordering is a sensible organizational change — placing the base class import (
Transport) before the dependency import (WebSocket).tests/transports/websocket.test.js (1)
64-67: LGTM!Formatting changes improve readability by breaking long argument lists across multiple lines. The test suite provides comprehensive coverage of the
WebSocketTransportclass including client management, broadcasting behavior, error handling, and per-client filtering logic.Also applies to: 102-104, 116-125, 140-148, 175-180, 186-188, 196-206, 220-223, 229-232
tests/sentry.test.js (1)
5-5: LGTM! Import statement reordering.The import statement has been reorganized to include
afterEachandbeforeEachin alphabetical order. This improves readability with no functional impact.tests/modules/triage-prompt.test.js (1)
374-374: LGTM! Added trailing newline.Adding the trailing newline at the end of the file follows standard formatting conventions.
src/logger.js (1)
17-17: LGTM! Import statement reordering.The
PostgresTransportimport has been repositioned with no functional impact.src/transports/sentry.js (1)
47-50: LGTM! Improved readability with multi-line formatting.The conditional check for tag key values has been reformatted across multiple lines, improving readability without changing the logic.
tests/api/utils/validateConfigPatch.test.js (1)
280-280: LGTM! Added trailing newline.Adding the trailing newline at the end of the file follows standard formatting conventions.
src/db.js (1)
7-9: LGTM! Import statement reordering.The imports for
fileURLToPathandpghave been reordered with no functional impact.src/api/ws/logStream.js (1)
185-185: LGTM! Concise HMAC construction.The HMAC digest construction has been formatted as a single-line method chain, which is a standard and more concise pattern with no functional change.
package.json (1)
29-29: [email protected] dependency is valid and secure.The version exists, is the latest stable release, and has no known security vulnerabilities. Node.js engine requirement (>=12.22.0) is fully compatible with the project's Node.js >=20.11.0 requirement.
tests/api/utils/configAllowlist.test.js (1)
214-232: LGTM!Formatting-only changes — the test logic and assertions remain unchanged.
src/api/routes/health.js (1)
108-114: LGTM!Minor formatting adjustment — arrow function parameter now wrapped in parentheses and ternary reformatted. Logic remains unchanged.
tests/modules/triage-respond.test.js (2)
1-1: LGTM!Import specifiers reordered alphabetically — formatting-only change.
560-574: LGTM!Function call reformatted to multi-line for readability. Test logic and assertions remain unchanged.
tests/api/ws/logStream.test.js (2)
5-9: LGTM!Imports updated to explicitly list the three exported functions from
logStream.js. This aligns with the module's public API.
206-266: LGTM!
transport.log()calls reformatted to multi-line object literals for improved readability. Test behavior and assertions remain unchanged.tests/api/routes/guilds.test.js (1)
317-317: LGTM!Request chain collapsed to single line — formatting-only change with no impact on test behavior.
.env.example (1)
85-89: LGTM!Well-documented addition of the
REDIS_URLenvironment variable. The comment clearly indicates it's optional with automatic fallback to in-memory storage, aligning with the PR's hybrid session store design.tests/api/middleware/oauth.test.js (1)
35-136: LGTM!Tests correctly converted to
async/awaitto match the middleware's async signature. This ensures proper handling of async session store operations (Redis-backed or in-memory). All assertions remain unchanged.src/index.js (5)
23-23: LGTM!Import added for
closeRedisto enable graceful Redis connection cleanup during shutdown.
30-38: LGTM!Logger imports reformatted to multi-line for improved readability. No functional change.
233-238: LGTM!Error metadata object reformatted to multi-line. No functional change.
303-308: LGTM!Redis connection cleanup properly integrated into the graceful shutdown sequence:
- Correctly placed after API server stop and database pool close
- Async operation properly awaited
- Error handling with Winston logging follows coding guidelines
- Comment accurately notes it's a no-op when Redis isn't configured
459-469: LGTM!Sentry initialization reformatted to multi-line. The
.then()chain correctly handles the async import and conditionally sets tags only when Sentry is enabled.src/api/middleware/auth.js (1)
36-69: AsyncrequireAuth()flow looks correct (awaits OAuth JWT handling).Awaiting
handleOAuthJwt(req, res, next)makes the middleware’s control flow explicit and consistent with the now-async JWT verification path.src/api/middleware/oauth.js (1)
14-17: AwaitinghandleOAuthJwt()here is the right fix.Keeps the async middleware chain consistent and prevents “floating” promises during auth.
src/api/middleware/oauthJwt.js (1)
28-31: Async middleware transition is clean and behavior-preserving.Awaiting token verification while keeping the existing error/missing-token branches intact looks correct.
Also applies to: 41-57
src/api/utils/sessionStore.js (1)
16-20: TTL strategy and Redis key namespacing look solid.The 1-hour TTL alignment plus prefixed keys is a good foundation for multi-instance session behavior.
Also applies to: 45-51, 102-112
| export async function verifyJwtToken(token) { | ||
| let secret; | ||
| try { | ||
| secret = getSecret(); | ||
| } catch { | ||
| return { error: 'Session not configured', status: 500 }; | ||
| } | ||
|
|
||
| let decoded; | ||
| try { | ||
| const decoded = jwt.verify(token, secret, { algorithms: ['HS256'] }); | ||
| if (!getSessionToken(decoded.userId)) { | ||
| return { error: 'Session expired or revoked', status: 401 }; | ||
| } | ||
| return { user: decoded }; | ||
| decoded = jwt.verify(token, secret, { algorithms: ['HS256'] }); | ||
| } catch { | ||
| return { error: 'Invalid or expired token', status: 401 }; | ||
| } | ||
|
|
||
| let sessionToken; | ||
| try { | ||
| sessionToken = await getSessionToken(decoded.userId); | ||
| } catch (err) { | ||
| logError('Session lookup failed', { error: err.message, userId: decoded.userId }); | ||
| return { error: 'Session lookup failed', status: 503 }; | ||
| } | ||
|
|
||
| if (!sessionToken) { | ||
| return { error: 'Session expired or revoked', status: 401 }; | ||
| } | ||
| return { user: decoded }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for jwt.sign calls to see what payloads are being signed
rg -n "jwt\.sign\(" --type js -A 3 -B 1Repository: VolvoxLLC/volvox-bot
Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Search for jwt.verify calls to understand usage patterns
rg -n "jwt\.verify\(" --type js -B 2 -A 5Repository: VolvoxLLC/volvox-bot
Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Check the file structure and identify jwt import statements
rg -n "import.*jwt|require.*jwt" --type jsRepository: VolvoxLLC/volvox-bot
Length of output: 1739
🌐 Web query:
jsonwebtoken library jwt.verify() return type - when does it return a string vs object
💡 Result:
In jsonwebtoken (auth0/node-jsonwebtoken), jwt.verify() returns different shapes based on (a) what was signed as the payload and (b) whether you pass complete: true:
- Returns a
stringwhen the token was signed with a string (or Buffer) payload.jwt.sign()explicitly allowspayloadto be an “object literal, buffer or string”, and non-object payloads aren’t treated as claim objects. (github.com) - Returns an
object(i.e., the decoded JSON payload) when the token was signed with an object literal payload. (github.com) - Returns an object
{ payload, header, signature }if you calljwt.verify(..., { complete: true })—and in that case,payloadinside that object can still be string or object depending on what was signed. (github.com)
Citations:
- 1: https://github.com/auth0/node-jsonwebtoken?utm_source=openai
- 2: https://github.com/auth0/node-jsonwebtoken?utm_source=openai
- 3: https://github.com/auth0/node-jsonwebtoken?utm_source=openai
Guard decoded JWT payload shape before accessing decoded.userId to prevent TypeError on non-object returns.
According to jsonwebtoken's specification, jwt.verify() returns a string (or Buffer) if the token was signed with a string/Buffer payload, and an object only if signed with an object literal. The current code assumes decoded is always an object and accesses decoded.userId without validation. If a string is returned, this causes a TypeError that gets caught nowhere and produces a 500 error instead of the intended 401.
Suggested patch
export async function verifyJwtToken(token) {
@@
let decoded;
try {
decoded = jwt.verify(token, secret, { algorithms: ['HS256'] });
} catch {
return { error: 'Invalid or expired token', status: 401 };
}
+ if (!decoded || typeof decoded !== 'object' || typeof decoded.userId !== 'string') {
+ return { error: 'Invalid or expired token', status: 401 };
+ }
+ const userId = decoded.userId;
+
let sessionToken;
try {
- sessionToken = await getSessionToken(decoded.userId);
+ sessionToken = await getSessionToken(userId);
} catch (err) {
- logError('Session lookup failed', { error: err.message, userId: decoded.userId });
+ const errMsg = err instanceof Error ? err.message : String(err);
+ logError('Session lookup failed', { error: errMsg, userId });
return { error: 'Session lookup failed', status: 503 };
}
@@
if (!sessionToken) {
return { error: 'Session expired or revoked', status: 401 };
}
return { user: decoded };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/middleware/verifyJwt.js` around lines 42 - 69, The verifyJwtToken
function assumes jwt.verify(token, secret, ...) returns an object with userId;
first validate that decoded is an object and has a userId (e.g., typeof decoded
=== 'object' && decoded !== null && 'userId' in decoded) before using
decoded.userId, and if that check fails return { error: 'Invalid or expired
token', status: 401 }; update the block around jwt.verify and the subsequent
getSessionToken usage to use this guard so you never call getSessionToken or
access decoded.userId on a string/Buffer or null value, and keep the existing
error paths (500 for missing secret, 401 for invalid token, 503 for session
lookup failure).
| async function hasOAuthGuildPermission(user, guildId, anyOfFlags) { | ||
| const accessToken = getSessionToken(user?.userId); | ||
| if (!accessToken) return false; | ||
| const guilds = await fetchUserGuilds(user.userId, accessToken); | ||
| const guild = guilds.find((g) => g.id === guildId); | ||
| if (!guild) return false; | ||
| const permissions = Number(guild.permissions); | ||
| if (Number.isNaN(permissions)) return false; | ||
| return (permissions & anyOfFlags) !== 0; | ||
| try { | ||
| const accessToken = await getSessionToken(user?.userId); | ||
| if (!accessToken) return false; | ||
| const guilds = await fetchUserGuilds(user.userId, accessToken); | ||
| const guild = guilds.find((g) => g.id === guildId); | ||
| if (!guild) return false; | ||
| const permissions = Number(guild.permissions); | ||
| if (Number.isNaN(permissions)) return false; | ||
| return (permissions & anyOfFlags) !== 0; | ||
| } catch (err) { | ||
| error('Error in hasOAuthGuildPermission (session lookup or guild fetch)', { | ||
| error: err.message, | ||
| userId: user?.userId, | ||
| guildId, | ||
| }); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Harden error logging: don’t assume err is an Error (and consider “session store” wording).
Both catch blocks log err.message. If a non-Error is ever thrown/rejected, the logger call can throw and you lose the original context.
Suggested patch
async function hasOAuthGuildPermission(user, guildId, anyOfFlags) {
try {
const accessToken = await getSessionToken(user?.userId);
if (!accessToken) return false;
@@
return (permissions & anyOfFlags) !== 0;
} catch (err) {
+ const errMsg = err instanceof Error ? err.message : String(err);
error('Error in hasOAuthGuildPermission (session lookup or guild fetch)', {
- error: err.message,
+ error: errMsg,
userId: user?.userId,
guildId,
});
throw err;
}
}
@@
try {
accessToken = await getSessionToken(req.user?.userId);
} catch (err) {
+ const errMsg = err instanceof Error ? err.message : String(err);
- error('Redis error fetching session token in GET /guilds', {
- error: err.message,
+ error('Session store error fetching session token in GET /guilds', {
+ error: errMsg,
userId: req.user?.userId,
});
return res.status(503).json({ error: 'Session store unavailable' });
}Also applies to: 319-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/guilds.js` around lines 174 - 191, The catch block in
hasOAuthGuildPermission (and the similar catch at lines 319-328) assumes err is
an Error and accesses err.message, which can throw if a non-Error is thrown;
update both catch handlers to safely stringify the error (e.g. use String(err)
or a safe serializer) and log that instead of err.message, and adjust the log
text to refer to "session store" when describing session lookup failures so the
message is accurate (targets: function hasOAuthGuildPermission and the
corresponding function around lines 319-328).
| export function getRedisClient() { | ||
| if (_initialized) return _client; | ||
| _initialized = true; | ||
|
|
||
| const redisUrl = process.env.REDIS_URL; | ||
| if (!redisUrl) return null; | ||
|
|
||
| try { | ||
| _client = new Redis(redisUrl, { | ||
| maxRetriesPerRequest: 3, | ||
| enableReadyCheck: true, | ||
| lazyConnect: false, | ||
| }); | ||
|
|
||
| _client.on('error', (err) => { | ||
| logError('Redis connection error', { error: err.message }); | ||
| }); | ||
| } catch (err) { | ||
| logError('Failed to initialise Redis client', { error: err.message }); | ||
| _client = null; | ||
| } | ||
|
|
||
| return _client; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Don’t permanently cache “no Redis” when REDIS_URL is missing on first call.
_initialized is flipped before reading process.env.REDIS_URL, so an early call (e.g., before dotenv/setup, or before env stubs in tests) can lock the process into _client = null forever.
Suggested patch
export function getRedisClient() {
if (_initialized) return _client;
- _initialized = true;
-
const redisUrl = process.env.REDIS_URL;
if (!redisUrl) return null;
+ _initialized = true;
try {
_client = new Redis(redisUrl, {
maxRetriesPerRequest: 3,
enableReadyCheck: true,
lazyConnect: false,
});
@@
} catch (err) {
- logError('Failed to initialise Redis client', { error: err.message });
+ const errMsg = err instanceof Error ? err.message : String(err);
+ logError('Failed to initialise Redis client', { error: errMsg });
_client = null;
+ _initialized = false;
}
return _client;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/utils/redisClient.js` around lines 21 - 44, The function
getRedisClient flips _initialized before reading process.env.REDIS_URL which can
permanently cache a null _client if REDIS_URL is missing on first call; change
the logic so you only set _initialized (or set it after) when you actually
decide to create and assign a real _client — i.e., check redisUrl first, return
null immediately if missing without setting _initialized, and only mark
_initialized = true (and assign _client) when new Redis(...) succeeds in
getRedisClient; also ensure the catch block clears _initialized or leaves it
false if creation fails so future calls can retry.
| set(userId, accessToken) { | ||
| const client = getRedisClient(); | ||
| if (client) { | ||
| return client.setex(`${SESSION_KEY_PREFIX}${userId}`, SESSION_TTL_SECONDS, accessToken); | ||
| } |
There was a problem hiding this comment.
Log Redis operation failures with context before propagating.
These Redis calls are external I/O and currently bubble errors without local context. Add per-operation contextual logging (operation, userId) before re-throwing so failures are diagnosable in production.
As per coding guidelines: "Always log errors with context before re-throwing".
Proposed fix
+import { error } from '../../logger.js';
import { getRedisClient } from './redisClient.js';
@@
class SessionStore extends Map {
+ _withRedisErrorContext(promise, operation, userId) {
+ return promise.catch((err) => {
+ error('Redis session store operation failed', {
+ operation,
+ userId,
+ error: err.message,
+ });
+ throw err;
+ });
+ }
+
set(userId, accessToken) {
const client = getRedisClient();
if (client) {
- return client.setex(`${SESSION_KEY_PREFIX}${userId}`, SESSION_TTL_SECONDS, accessToken);
+ return this._withRedisErrorContext(
+ client.setex(`${SESSION_KEY_PREFIX}${userId}`, SESSION_TTL_SECONDS, accessToken),
+ 'set',
+ userId
+ );
}
@@
get(userId) {
const client = getRedisClient();
if (client) {
- return client.get(`${SESSION_KEY_PREFIX}${userId}`);
+ return this._withRedisErrorContext(
+ client.get(`${SESSION_KEY_PREFIX}${userId}`),
+ 'get',
+ userId
+ );
}
@@
has(userId) {
@@
- return client.exists(`${SESSION_KEY_PREFIX}${userId}`).then((n) => n > 0);
+ return this._withRedisErrorContext(
+ client.exists(`${SESSION_KEY_PREFIX}${userId}`).then((n) => n > 0),
+ 'has',
+ userId
+ );
@@
delete(userId) {
@@
- return client.del(`${SESSION_KEY_PREFIX}${userId}`);
+ return this._withRedisErrorContext(
+ client.del(`${SESSION_KEY_PREFIX}${userId}`),
+ 'delete',
+ userId
+ );
}Also applies to: 61-64, 81-84, 95-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/utils/sessionStore.js` around lines 45 - 49, Wrap each Redis
operation in sessionStore.js (e.g., the set method that uses getRedisClient()
and constants SESSION_KEY_PREFIX and SESSION_TTL_SECONDS, and the other methods
at the indicated ranges) with a try/catch that logs the error with contextual
fields operation (e.g., "set", "get", "delete", "refresh") and userId before
re-throwing; locate the methods by name (set, the other session-related methods
around the provided ranges) and add processLogger.error (or the module's logger)
calls inside the catch that include a descriptive message plus operation and
userId, then rethrow the original error.
| * @returns {string | undefined | Promise<string | null>} | ||
| */ |
There was a problem hiding this comment.
Normalize Redis-path return values to match in-memory semantics.
Line 58/59 and Line 92/93 currently allow backend-specific value shapes, which weakens the “same interface” expectation and can cause subtle caller bugs. Prefer returning undefined for misses and boolean for delete across both backends.
Proposed fix
- * `@returns` {string | undefined | Promise<string | null>}
+ * `@returns` {string | undefined | Promise<string | undefined>}
@@
get(userId) {
@@
if (client) {
- return client.get(`${SESSION_KEY_PREFIX}${userId}`);
+ return client.get(`${SESSION_KEY_PREFIX}${userId}`).then((token) => token ?? undefined);
}
@@
- * `@returns` {boolean | Promise<number>}
+ * `@returns` {boolean | Promise<boolean>}
@@
delete(userId) {
@@
if (client) {
- return client.del(`${SESSION_KEY_PREFIX}${userId}`);
+ return client.del(`${SESSION_KEY_PREFIX}${userId}`).then((count) => count > 0);
}
@@
- * `@returns` {Promise<string | null | undefined> | string | undefined}
+ * `@returns` {Promise<string | undefined> | string | undefined}Also applies to: 92-93, 124-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/utils/sessionStore.js` around lines 58 - 59, Normalize the Redis
backend return values so they match the in-memory semantics: in the Redis-backed
get/getSession implementation convert Redis null/empty-miss results to undefined
(not null or other backend-specific shape) and ensure the setter returns the
same string type as the in-memory path; in the Redis-backed
delete/del/deleteSession (or destroySession) implementation convert Redis
numeric reply (0/1) to a boolean (false/true) and return that boolean (not the
raw number or other type); ensure all three locations referenced (the
get/getSession path around lines 58–59 and 92–93 and the delete/del path around
124–127) perform these conversions and preserve Promise return types so callers
always see undefined for misses and boolean for deletes across backends.
1 participant
Summary
Replaces the in-memory session store with a Redis-backed implementation. Falls back to in-memory when Redis is not configured.
Closes #79
Changes
src/api/utils/redisClient.js(new) — lazy ioredis client viaREDIS_URLenv varsrc/api/utils/sessionStore.js— hybrid Redis/in-memory store with same interfaceverifyJwtToken,handleOAuthJwt,requireAuth,requireOAuthall asyncauth.jsandguilds.jsnowawaitall session operationscloseRedis()in shutdown sequence.env.example—REDIS_URLdocumentedHow It Works
REDIS_URLset → Redis withSETEX(1h TTL, key:session:<userId>)REDIS_URLnot set → in-memory fallback (current behavior)cleanup()is a no-op for Redis (TTL handles expiry)Testing
72 test files, 1490 tests passing (17 new session store tests)