Skip to content

Require DB connection string, drop hardcoded localhost fallback - #370

Merged
birme merged 2 commits into
mainfrom
bug-fixer/225-require-db-connection-string
Sep 24, 2026
Merged

birme merged 2 commits into
mainfrom
bug-fixer/225-require-db-connection-string

Conversation

@birme

@birme birme commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Removed the hardcoded mongodb://localhost:27017/intercom-manager fallback in src/server.ts that silently masked a missing DB configuration in production.
  • The manager now fails fast at startup via validateRequiredEnv(): it logs Missing required environment variable: DB_CONNECTION_STRING and exits with code 1 when neither DB_CONNECTION_STRING nor MONGODB_CONNECTION_STRING is set.
  • DB manager construction (URL parse + Mongo/Couch selection) moved into startServer() so it runs only after env validation, mirroring how SMB_ADDRESS is already handled.
  • Kept the localhost default for local development in .env.example (and documented in readme.md), not in production code.
  • Added regression tests covering the missing/empty DB string and the legacy MONGODB_CONNECTION_STRING name.

Closes #225

Test plan

  • Tests pass (npm test)
  • TypeScript compiles (npm run typecheck)
  • Lint clean (npm run lint)
  • Manager exits with a clear FATAL message when no DB connection string is set

🤖 Generated with Claude Code

… fallback

Previously src/server.ts silently fell back to a hardcoded
'mongodb://localhost:27017/intercom-manager' when neither
DB_CONNECTION_STRING nor MONGODB_CONNECTION_STRING was set, risking a
silent misconfiguration in production. The manager now fails fast at
startup (via validateRequiredEnv) with a clear error and exit code 1
when no DB connection string is provided. The localhost default is kept
for local dev in .env.example/docker-compose, not in production code.

Closes #225

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@birme

birme commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Code Review

Verdict: Needs Changes

Summary: The change is focused, well-motivated, and correctly removes the silent localhost fallback with good regression tests for the primary failure path. There is no Blocking defect, but a cluster of three Warnings — an inconsistency between how validation and construction treat an empty connection string, unhandled startup rejections, and a missing ./log mock in the test file — which per the review rubric warrants Needs Changes.

Blocking

None.

Warnings

  • src/server.ts (validateRequiredEnv vs. dbConnectionString construction) — Inconsistent truthiness. validateRequiredEnv() uses a falsy check (!process.env.DB_CONNECTION_STRING && !process.env.MONGODB_CONNECTION_STRING), so an empty DB_CONNECTION_STRING alongside a valid MONGODB_CONNECTION_STRING passes validation. But construction uses nullish coalescing: process.env.DB_CONNECTION_STRING ?? process.env.MONGODB_CONNECTION_STRING ?? ''. An empty string is not nullish, so it wins and new URL('') throws an opaque TypeError: Invalid URL, defeating the PR's fail-fast goal. Align the two (e.g. use || in construction, or trim/normalize before both checks).
  • src/server.ts (startServer() invocation) — startServer() is called without a .catch(). Its early synchronous throws (new URL(...) on a malformed string, throw new Error('Unsupported database protocol')) and dbManager.connect() rejections occur before process.on('unhandledRejection', ...) is registered, producing an unhandled-rejection crash rather than a clean logged fatal + process.exit(1). Wrap the call in .catch(...), or validate/parse the DB URL inside validateRequiredEnv() where the clean exit path already exists.
  • src/server.test.ts — Test file does not jest.mock('./log', ...) as the project testing rules require for every backend test file. Module-level Log().warn(...) calls that run on import will pollute test output. Add the standard ./log mock.

Suggestions

  • Add a regression test for DB_CONNECTION_STRING='' while MONGODB_CONNECTION_STRING is set — the existing "legacy name" test only deletes the var (undefined), never exercising the ?? empty-string mismatch.
  • Consider validating the DB URL protocol inside validateRequiredEnv() (clean Log().error(...) + process.exit(1)) rather than throwing Unsupported database protocol deep inside startServer(), keeping all fail-fast config checks in one place.

Domain Note

Not applicable — server startup / DB manager construction only.

…or handling

Use logical-OR instead of nullish coalescing when building the DB
connection string so an empty DB_CONNECTION_STRING falls through to
MONGODB_CONNECTION_STRING, matching the falsy check in
validateRequiredEnv(). Wrap the startServer() entrypoint in a catch so
synchronous throws and rejections during startup log a fatal error and
exit cleanly. Add the standard ./log jest mock and a regression test for
the empty-string fallthrough case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@birme

birme commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: LGTM (re-review at fe0c5105a8ce147de7a91fe7c8718724e8d640cd after fixes; self-authored PR, recorded as a marker since GitHub blocks state-bearing self-review).

All three prior Needs-Changes warnings verified resolved:

  1. Empty-string handling now consistent — construction uses || matching the falsy check in validateRequiredEnv(), so DB_CONNECTION_STRING='' with MONGODB_CONNECTION_STRING set no longer throws on new URL('').
  2. startServer() entrypoint wrapped in .catch → clean logged fatal + process.exit(1) instead of an unhandled rejection.
  3. ./log mocked per repo convention + regression tests for the empty-string and neither-set cases.

Fresh pass: no straggler localhost defaults in production code, DB-manager construction correctly gated behind validation, TypeScript clean. Non-blocking nits only. CI green (lint/pretty/ts/unittests). Merging via --admin.

@birme
birme merged commit 6d0a03e into main Sep 24, 2026
4 checks passed
@birme
birme deleted the bug-fixer/225-require-db-connection-string branch September 24, 2026 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Require DB_CONNECTION_STRING env var — remove hardcoded localhost MongoDB fallback

2 participants