Improve error handling: fix swallowed errors and add proper propagation#1
Open
devin-ai-integration[bot] wants to merge 3 commits into
Open
Improve error handling: fix swallowed errors and add proper propagation#1devin-ai-integration[bot] wants to merge 3 commits into
devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR hardens the Node.js backend by validating DB env vars at startup, removing a JWT_SECRET debug log, adding process-level handlers that log and exit on fatal errors, adding Express 404 and global error middleware (including malformed JSON handling), and improving auth error responses with matching test updates. ChangesError Handling & Security Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/controllers/auth.controller.js (1)
24-29: ⚡ Quick winInconsistent status code for duplicate email (400 vs 409).
The pre-check at Lines 24-29 returns
400for an existing email, while the unique-constraint fallback returns409. The same logical condition should map to one status. Recommend409in both for a consistent client contract (the catch block already serves as the race-condition fallback).♻️ Align pre-check with 409
if (existingUser) { - return res.status(400).json({ + return res.status(409).json({ success: false, message: 'Email already registered', }); }Also applies to: 61-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/controllers/auth.controller.js` around lines 24 - 29, The pre-check that returns a 400 when an existingUser is found should return 409 to match the unique-constraint fallback; update the response in the existingUser branch (the duplicate-email pre-check) to use res.status(409).json(...) and likewise change the other duplicate-email pre-check branch (the similar block later in the same controller) so both use 409 for consistency with the catch fallback that already returns 409.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/app.js`:
- Around line 41-45: The current error handler logs the full error and returns
err.message to clients even on 500s; change the logic in the error-handling
block that uses console.error, res.status(err.status || 500).json(...) so that
only explicit client errors (status >=400 && <500) surface err.message to the
JSON response, while unexpected server errors (no err.status or status >=500)
return a generic message like "Internal server error"; continue to console.error
the full err (and stack) for server-side diagnostics but do not include those
details in the response JSON.
---
Nitpick comments:
In `@backend/src/controllers/auth.controller.js`:
- Around line 24-29: The pre-check that returns a 400 when an existingUser is
found should return 409 to match the unique-constraint fallback; update the
response in the existingUser branch (the duplicate-email pre-check) to use
res.status(409).json(...) and likewise change the other duplicate-email
pre-check branch (the similar block later in the same controller) so both use
409 for consistency with the catch fallback that already returns 409.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 939923ac-7260-4172-b8d5-aee0336f03e4
📒 Files selected for processing (5)
backend/server.jsbackend/src/app.jsbackend/src/config/database.jsbackend/src/controllers/auth.controller.jsbackend/src/models/index.js
Comment on lines
+41
to
+45
| console.error('Unhandled error:', err); | ||
| res.status(err.status || 500).json({ | ||
| success: false, | ||
| message: err.message || 'Internal server error', | ||
| }); |
There was a problem hiding this comment.
Avoid leaking internal error details to clients on 500s.
For unexpected errors (no intentional err.status), err.message can expose internals such as driver/SQL/stack text to clients. Surface err.message only for explicit client errors (4xx) and return a generic message otherwise.
🛡️ Proposed fix
console.error('Unhandled error:', err);
- res.status(err.status || 500).json({
- success: false,
- message: err.message || 'Internal server error',
- });
+ const status = err.status || 500;
+ res.status(status).json({
+ success: false,
+ message: status < 500 ? (err.message || 'Request error') : 'Internal server error',
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.error('Unhandled error:', err); | |
| res.status(err.status || 500).json({ | |
| success: false, | |
| message: err.message || 'Internal server error', | |
| }); | |
| console.error('Unhandled error:', err); | |
| const status = err.status || 500; | |
| res.status(status).json({ | |
| success: false, | |
| message: status < 500 ? (err.message || 'Request error') : 'Internal server error', | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/app.js` around lines 41 - 45, The current error handler logs the
full error and returns err.message to clients even on 500s; change the logic in
the error-handling block that uses console.error, res.status(err.status ||
500).json(...) so that only explicit client errors (status >=400 && <500)
surface err.message to the JSON response, while unexpected server errors (no
err.status or status >=500) return a generic message like "Internal server
error"; continue to console.error the full err (and stack) for server-side
diagnostics but do not include those details in the response JSON.
- server.js: Remove debug log leaking JWT_SECRET, exit process on startup failure, add unhandledRejection and uncaughtException handlers - config/database.js: Validate required DB env vars on startup with clear error message listing which vars are missing - app.js: Add 404 handler for unmatched routes, add global error-handling middleware with malformed JSON detection - auth.controller.js: Distinguish SequelizeValidationError (400) and SequelizeUniqueConstraintError (409) from generic 500s in register; validate JWT_SECRET is set before signing tokens in login - models/index.js: Fix require path case (User -> user) for Linux compat Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
force-pushed
the
devin/1780488322-improve-error-handling
branch
from
June 4, 2026 16:45
6673260 to
0804633
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/src/__tests__/auth.controller.test.js (2)
101-192: ⚡ Quick winAdd coverage for the missing
JWT_SECRETbranch.Line 101-192 does not currently assert the new login guard that returns
500withServer configuration errorwhenprocess.env.JWT_SECRETis missing. This is a key contract added in the controller and should be locked with a dedicated test.Proposed test addition
describe('login', () => { beforeEach(() => { jest.clearAllMocks(); process.env.JWT_SECRET = 'test-secret'; process.env.JWT_EXPIRES_IN = '1h'; }); + it('returns 500 when JWT_SECRET is not configured', async () => { + delete process.env.JWT_SECRET; + const fakeUser = { id: '1', name: 'Test', email: 'a@b.com', password: 'hashed' }; + User.findOne.mockResolvedValue(fakeUser); + bcrypt.compare.mockResolvedValue(true); + + const { req, res } = mockReqRes({ email: 'a@b.com', password: '123456' }); + await login(req, res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ success: false, message: 'Server configuration error' }) + ); + expect(jwt.sign).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/__tests__/auth.controller.test.js` around lines 101 - 192, Add a new test in backend/src/__tests__/auth.controller.test.js that verifies the login guard when process.env.JWT_SECRET is missing: in the test clear mocks, delete process.env.JWT_SECRET, create a mock req/res (e.g., mockReqRes with valid email/password), call the login function, and assert res.status was called with 500 and res.json was called with an object containing the message 'Server configuration error'; ensure the test does not rely on User.findOne or jwt.sign being called since the guard should return early.
102-106: ⚡ Quick winRestore mutated environment variables after each test.
Line 102-106 mutates
process.envbut does not restore previous values, which can leak state into other suites.Proposed isolation fix
describe('login', () => { + let originalJwtSecret; + let originalJwtExpiresIn; + + beforeAll(() => { + originalJwtSecret = process.env.JWT_SECRET; + originalJwtExpiresIn = process.env.JWT_EXPIRES_IN; + }); + beforeEach(() => { jest.clearAllMocks(); process.env.JWT_SECRET = 'test-secret'; process.env.JWT_EXPIRES_IN = '1h'; }); + + afterAll(() => { + if (originalJwtSecret === undefined) delete process.env.JWT_SECRET; + else process.env.JWT_SECRET = originalJwtSecret; + + if (originalJwtExpiresIn === undefined) delete process.env.JWT_EXPIRES_IN; + else process.env.JWT_EXPIRES_IN = originalJwtExpiresIn; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/__tests__/auth.controller.test.js` around lines 102 - 106, The tests mutate process.env in the beforeEach block (setting process.env.JWT_SECRET and process.env.JWT_EXPIRES_IN) but never restore originals; update the test file to save the original values (e.g., capture originalJwtSecret and originalJwtExpiresIn) before overwriting them in beforeEach, and add an afterEach that restores those originals (or deletes the keys if they were undefined) so process.env is returned to its prior state after each test; reference the existing beforeEach and add a corresponding afterEach near it to perform the restore and keep jest.clearAllMocks() as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/__tests__/auth.controller.test.js`:
- Around line 101-192: Add a new test in
backend/src/__tests__/auth.controller.test.js that verifies the login guard when
process.env.JWT_SECRET is missing: in the test clear mocks, delete
process.env.JWT_SECRET, create a mock req/res (e.g., mockReqRes with valid
email/password), call the login function, and assert res.status was called with
500 and res.json was called with an object containing the message 'Server
configuration error'; ensure the test does not rely on User.findOne or jwt.sign
being called since the guard should return early.
- Around line 102-106: The tests mutate process.env in the beforeEach block
(setting process.env.JWT_SECRET and process.env.JWT_EXPIRES_IN) but never
restore originals; update the test file to save the original values (e.g.,
capture originalJwtSecret and originalJwtExpiresIn) before overwriting them in
beforeEach, and add an afterEach that restores those originals (or deletes the
keys if they were undefined) so process.env is returned to its prior state after
each test; reference the existing beforeEach and add a corresponding afterEach
near it to perform the restore and keep jest.clearAllMocks() as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dce7900-b853-4f81-aa37-0dc5c3336c5f
📒 Files selected for processing (5)
backend/server.jsbackend/src/__tests__/auth.controller.test.jsbackend/src/app.jsbackend/src/config/database.jsbackend/src/controllers/auth.controller.js
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/app.js
- backend/src/config/database.js
- backend/server.js
- backend/src/controllers/auth.controller.js
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit of backend error handling found several places where errors were silently swallowed or not properly propagated. This PR fixes all of them:
server.js— Removedconsole.logthat leakedJWT_SECRETto stdout.startServer()catch block now callsprocess.exit(1)instead of silently continuing with a broken DB connection. AddedunhandledRejectionanduncaughtExceptionhandlers so no async error goes unnoticed.config/database.js— ValidatesDB_NAME,DB_USER,DB_PASSWORD,DB_HOST,DB_PORTare set at startup and throws a clear error listing any that are missing, instead of passingundefinedintonew Sequelize()and getting a cryptic failure later.app.js— Added 404 handler for unmatched routes (was returning Express's default HTML). Added global error-handling middleware that catches malformed JSON fromexpress.json()(returns 400) and any other unhandled errors (returns 500 with the error message).auth.controller.js— Register catch block now distinguishesSequelizeValidationError→ 400 andSequelizeUniqueConstraintError→ 409, instead of treating all errors as 500. Login now validatesJWT_SECRETis defined before callingjwt.sign()— previously it would silently sign withundefined.models/index.js— Fixedrequire('./User')→require('./user')to match the actual filename (user.js), preventing a silent crash on case-sensitive file systems (Linux).Test update — Updated
auth.controller.test.jsexpectation for the register 500 error message to match the new, more descriptive'An unexpected error occurred during registration'.Link to Devin session: https://app.devin.ai/sessions/266a6d93aa384d3f838d74c3d955d5a8
Requested by: @0xpritam
Summary by CodeRabbit
Bug Fixes
Tests