Skip to content

fix: load .env files before TOML config interpolation#358

Open
kevinke wants to merge 1 commit into
bytebase:mainfrom
kevinke:fix/toml-env-loading
Open

fix: load .env files before TOML config interpolation#358
kevinke wants to merge 1 commit into
bytebase:mainfrom
kevinke:fix/toml-env-loading

Conversation

@kevinke

@kevinke kevinke commented Jul 12, 2026

Copy link
Copy Markdown

Problem

When using --config with dbhub.toml, .env files are never loaded. This is because loadEnvFiles() (which calls dotenv.config()) is only called inside resolveDSN() — the fallback path that runs ONLY when no TOML config is found.

As a result, ${VAR_NAME} interpolation in dbhub.toml (handled by interpolateEnvVars() in toml-loader.ts) can only resolve from system environment variables, not from .env files.

This contradicts the official documentation which states that .env files are supported.

Root Cause

In resolveSourceConfigs() (src/config/env.ts):

  1. If a TOML config exists → loadTomlConfig() runs interpolateEnvVars() reading from process.env → returns immediately
  2. loadEnvFiles() / dotenv.config() lives inside resolveDSN() — which is never reached when TOML config exists

Fix

Call loadEnvFiles() at the start of resolveSourceConfigs(), before loadTomlConfig(). This ensures .env variables are loaded into process.env and available for TOML interpolation.

The redundant call inside resolveDSN() is harmless (dotenv just re-reads the same file).

Testing

  • Verified that with the fix, .env files are loaded when using --config dbhub.toml
  • ${VAR_NAME} interpolation in TOML now correctly resolves from .env files
  • No breaking changes to existing resolveDSN() fallback path

When using --config with dbhub.toml, .env files were never loaded
because loadEnvFiles() was only called inside resolveDSN() — the
fallback path that runs when no TOML config is found.

This meant ${VAR_NAME} interpolation in dbhub.toml could only
resolve from system environment variables, not from .env files,
contradicting the documentation.

The fix calls loadEnvFiles() at the start of resolveSourceConfigs()
so that .env variables are available for TOML interpolation regardless
of which config path is taken.
Copilot AI review requested due to automatic review settings July 12, 2026 09:34
@kevinke
kevinke requested a review from tianzhou as a code owner July 12, 2026 09:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes TOML config environment-variable interpolation by ensuring .env files are loaded before dbhub.toml is parsed/interpolated, aligning runtime behavior with the TOML documentation.

Changes:

  • Load .env files earlier in the config-resolution flow so ${VAR_NAME} placeholders in TOML can resolve from .env.
  • Add explanatory comments clarifying the TOML-vs-DSN fallback interaction.

Comment thread src/config/env.ts
Comment on lines +663 to +667
// 0. Load .env files first so that environment variables are available
// for TOML interpolation (${VAR_NAME}) when using --config.
// Without this, .env is only loaded inside resolveDSN() (the fallback
// path), meaning TOML configs never get .env-backed variable resolution.
loadEnvFiles();
Comment thread src/config/env.ts
// for TOML interpolation (${VAR_NAME}) when using --config.
// Without this, .env is only loaded inside resolveDSN() (the fallback
// path), meaning TOML configs never get .env-backed variable resolution.
loadEnvFiles();
Comment thread src/config/env.ts
// for TOML interpolation (${VAR_NAME}) when using --config.
// Without this, .env is only loaded inside resolveDSN() (the fallback
// path), meaning TOML configs never get .env-backed variable resolution.
loadEnvFiles();
tianzhou added a commit that referenced this pull request Jul 19, 2026
interpolateEnvVars() reads process.env, but .env was only loaded inside
resolveDSN() — the fallback path that never runs when --config is given.
So the recommended pattern of keeping credentials in .env and referencing
them from the config file did not work:

  # .env
  DSN=postgres://user:pass@localhost:5432/mydb

  # dbhub.toml
  dsn = "${DSN}"

resolved to the literal "${DSN}" and failed with "No connector found".

Loading is scoped to TOML mode rather than unconditional. On the DSN path
resolveDSN() loads .env itself, and it has to do so after checking
process.env so it can report whether a value came from the environment or
from the file; preloading would relabel every .env-sourced DSN as an
environment variable.

Same fix as #358 by @kevinke, narrowed in scope and with a regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tianzhou added a commit that referenced this pull request Jul 19, 2026
* fix: reject DSN configuration combined with TOML config

TOML config and a DSN express conflicting intents: TOML defines sources
for one or more databases, a DSN configures exactly one. Previously
resolveSourceConfigs() returned the TOML config before resolveDSN() was
ever consulted, so a DSN — including an explicit --dsn flag — was
silently ignored with no indication of why.

Now the combination is rejected at startup with an error naming the
conflicting DSN source, mirroring the existing --id guard.

Detection is side-effect free. It deliberately does not call resolveDSN(),
which loads .env files into process.env: in TOML mode .env is otherwise
never loaded, and loading it would silently start applying unrelated
settings such as PORT and TRANSPORT. Instead findEnvFile() is split out of
loadEnvFiles() so a .env file can be inspected with dotenv.parse without
being applied.

Also corrects the documented priority order, which claimed command-line
arguments outrank TOML. That was never true for source configuration.
CLAUDE.md and the docs now separate source selection (TOML or DSN,
mutually exclusive) from every other setting (CLI > env > .env >
defaults), which TOML cannot express.

BREAKING CHANGE: a setup with both a dbhub.toml and any DSN configuration
now fails to start instead of silently using the TOML config.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix: load TOML config only via --config, drop cwd auto-discovery

resolveTomlConfigPath() fell back to ./dbhub.toml in the current working
directory when --config was absent. Because TOML config selects the
database outright and cannot be combined with a DSN, that implicit
discovery meant running DBHub from the wrong directory could silently
repoint it at a different database, or make an explicitly supplied DSN
appear to be ignored for no visible reason.

TOML is now loaded only when --config names it. This also shrinks the
blast radius of the DSN/TOML conflict error added in the previous commit:
the conflict can now only arise from an explicit --config flag, never
from a stray file in the working directory.

The MCPB bundle already passes --config ${__dirname}/dbhub.toml
explicitly, and every documented invocation does the same, so no shipped
configuration depends on auto-discovery.

BREAKING CHANGE: a dbhub.toml in the current directory is no longer
loaded automatically. Pass --config ./dbhub.toml to load it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs: describe config resolution directly

Rewrite the configuration docs to state how DBHub picks a database rather
than contrasting against earlier behavior, and trim the same framing from
the surrounding code comments.

Also restores resolveDSN's doc comment, which had been separated from its
function by the detectDSNConfig insertion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix: limit the TOML conflict guard to the --dsn flag

TOML `${VAR}` interpolation reads process.env, so `dsn = "${DSN}"` is a
supported way to keep credentials out of the config file. Treating a DSN
environment variable — exported or read from .env — as a conflict would
break that pattern, which is the whole point of interpolation.

Only the --dsn flag is unambiguous intent, so only that throws. This also
drops the .env inspection the broader check needed, along with the
findEnvFile export added for it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix: load .env before TOML config so ${VAR} interpolation resolves

interpolateEnvVars() reads process.env, but .env was only loaded inside
resolveDSN() — the fallback path that never runs when --config is given.
So the recommended pattern of keeping credentials in .env and referencing
them from the config file did not work:

  # .env
  DSN=postgres://user:pass@localhost:5432/mydb

  # dbhub.toml
  dsn = "${DSN}"

resolved to the literal "${DSN}" and failed with "No connector found".

Loading is scoped to TOML mode rather than unconditional. On the DSN path
resolveDSN() loads .env itself, and it has to do so after checking
process.env so it can report whether a value came from the environment or
from the file; preloading would relabel every .env-sourced DSN as an
environment variable.

Same fix as #358 by @kevinke, narrowed in scope and with a regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs: drop a stale comment about --readonly handling

It pointed at an isReadOnlyMode() function that does not exist, and
described a deprecation warning where parseCommandLineArgs() actually
exits on --readonly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix: validate --config has a value; drop duplicate env path check

--config now goes through requireFlagValue(), so a bare `--config` or
`--config=` reports "--config requires a value" instead of resolving to
the sentinel "true" and failing as `not found: true`. That helper already
existed for --host and --allowed-hosts and documents itself as reusable.
This matters more now that --config is the only way to load TOML.

findEnvFile() also checked the working directory twice: a bare relative
filename resolves against process.cwd(), so it was identical to the
explicit path.join(process.cwd(), ...) entry. Removed the trailing
duplicate rather than the leading one, which would have flipped
precedence so the package root won over the working directory.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: make the TOML/DSN conflict tests match a reachable state

The tests mocked loadTomlConfig() into returning a config while argv had
no --config. Since --config is the only way TOML loads, that state cannot
occur at runtime — and it specifically skipped the .env-before-TOML load,
which is gated on the flag, so those tests exercised a path production
never takes.

They now pass --config, and run from an empty temp directory so an
ambient .env cannot reach the load the flag now triggers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants