Skip to content

Fix: load .env from the CLI entrypoint instead of at module import - #433

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-dotenv-module-scope-side-effect
Open

Fix: load .env from the CLI entrypoint instead of at module import#433
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-dotenv-module-scope-side-effect

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A — no public issue is tracking this.
  2. Or, if no issue exists, describe the change:

Problem: dev/src/cli/cli.ts called dotenv.config({quiet: true}) at module scope, between the import block and the first declaration. Anything that imports the module therefore reads ${process.cwd()}/.env off disk and merges it into process.env as a side effect of module evaluation. dev/test/cli/cli_test.ts imports createProgram from that module, so every run of the unit:dev vitest project performed that read, with process.cwd() being the repo root.

That makes the suite non-hermetic in a way the usual remedies cannot fix:

  1. The pollution comes from a file on disk, not from the parent shell. A repo-root .env is .gitignored (so invisible in git status) and absent on CI runners, so a contributor who never exported DATABASE_URL still gets it, and CI never sees what they see.
  2. It happens during module evaluation. ESM imports are hoisted and evaluated before any test-framework hook runs, so scrubbing process.env in setupFiles, in beforeAll, or with vi.stubEnv in beforeEach is strictly too late — by the time the first hook executes the values are already merged. I verified this by adding beforeEach(() => { vi.stubEnv('DATABASE_URL', ''); }) to cli_test.ts with a repo-root .env: the value was still present at import time.

The leak is not theoretical for this file. dev/src/cli/cli.ts reads the injected value directly in getSessionServiceFromOptions:

options['session_service_uri'] || process.env.DATABASE_URL || 'memory://';

so with DATABASE_URL in a repo-root .env, every web / api_server / run / deploy case in cli_test.ts that does not pass --session_service_uri resolves a session service from the developer's database URI instead of the intended memory:// default.

Solution: relocate the call — delete import dotenv from 'dotenv' and dotenv.config({quiet: true}) from dev/src/cli/cli.ts, and make dotenv.config({quiet: true}) the first statement inside the existing try block of dev/src/cli_entrypoint.ts (the bin target, "adk": "./dist/esm/cli_entrypoint.js"). The side effect is moved, not removed:

before after
import '…/cli/cli.js' reads ${cwd}/.env, mutates process.env no side effect
createProgram() no side effect no side effect
adk <cmd> (bin) reads ${cwd}/.env before any action reads ${cwd}/.env before any action

Why this is the right fix rather than a test-only workaround:

  • adk-python already does it this way. The Python SDK never calls load_dotenv at module scope; the only invocation lives inside load_dotenv_for_agent(...) in src/google/adk/cli/utils/envs.py, called from command bodies and loaders (cli_tools_click.py, cli.py, agent_loader.py, service_registry.py, fast_api.py). Importing google.adk.cli.cli_tools_click reads no .env. The TypeScript SDK was the outlier.
  • Rejected: vi.mock('dotenv', …) in the tests. It works mechanically (vi.mock is hoisted above imports) but leaves the filesystem side effect in shipped code and has to be repeated in every test file that ever transitively imports cli.js.
  • Rejected: moving the call to the first line of createProgram(). That closes the import-time hole but not the leak — cli_test.ts calls createProgram() in beforeEach, so the .env would be read once per test case instead of once per file.
  • Rejected: a setupFiles module that scrubs process.env. Cannot work for this bug (hooks run after module evaluation). It is worthwhile for the separate ambient-shell-variable problem, which is not folded in here.

Scope — this is the only occurrence. A sweep for module-scope dotenv.config() across the repo found exactly one: dev/src/cli/cli.ts. The two apparent hits in dev/src/cli/cli_create.ts are inside the AGENT_TEMPLATE template literal — they are the text of the sample agent.ts that adk create scaffolds, where a module-scope dotenv.config() is correct and intended, so they are deliberately untouched. The ~15 dotenv.config({path: envPath}) calls under tests/e2e/** pass an explicit path, belong to the e2e project, and are out of scope.

Ordering safety. Under ESM, import {createProgram} from './cli/cli.js' is hoisted, so the whole cli.ts module graph now evaluates before dotenv.config() runs. This is safe because the only code whose evaluation moves relative to the dotenv call is cli.ts's own module body (its imports were already evaluated before the call in the old layout), and that body reads no process.env: it is LOG_LEVEL_MAP, plain function declarations, and the Argument/Option constants, whose defaults are string literals and process.cwd(). The one env-sensitive default, getTempDir(...) (which calls os.tmpdir() and so reads TMPDIR), is evaluated inside createProgram(), which the new entrypoint calls after dotenv.config(). grep -n "process\.env" dev/src/cli/cli.ts returns a single hit, inside getSessionServiceFromOptions.

Behaviour claims, grounded. dotenv resolved in this workspace is 17.4.2 (node -p "require('dotenv/package.json').version"). node_modules/dotenv/lib/main.js:241 computes path.resolve(process.cwd(), '.env') at call time, and :372 defaults override to false. A missing .env is not an exception — config() returns {parsed, error} — so relocating the call introduces no new failure mode on machines with no .env. {quiet: true} is carried over verbatim, so no v17 tip banner appears.

No breaking change. CLI users invoke dist/esm/cli_entrypoint.js via the adk bin, which still loads .env before any command action with the same options and precedence. Library users import @google/adk-devtools, whose entry (dev/src/index.ts) exports only AdkApiClient and AdkApiServer and never re-exported cli.ts, so it never triggered the load. The only consumers of dev/src/cli/cli.js in the repo are dev/src/cli_entrypoint.ts and dev/test/cli/cli_test.ts; both are handled. Generated agent scaffolds are untouched.

Collision check (required). gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus a filtered pass over all open PRs for dotenv|env|entrypoint|cli, then gh pr diff --name-only on the plausible neighbours. No open PR lands this change. The adjacent ones are test-only and attack a different source of pollution (the ambient shell, not a .env file), which the plan for this fix explicitly keeps separate: #348 (dev/test/cli/cli_test.ts only — scrubs an ambient DATABASE_URL via vi.stubEnv), #281 and #302 (hermeticity helpers under core/test/tests/), #369 (consolidates the tests/e2e dotenv loader, explicit-path calls only). None removes the module-scope call, and as explained above a stubEnv/setupFiles scrub cannot fix the import-time read. This branch is cut from main rather than stacked because its hunks are disjoint from #348's (imports block and a new trailing describe vs. the mock factory and beforeEach), and stacking on a test-only PR that may not merge would pointlessly couple a two-line source fix to it.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

One new describe('module import side effects') block was added to dev/test/cli/cli_test.ts; no existing test in that file was modified or deleted. It writes a .env holding ADK_DOTENV_IMPORT_SENTINEL=leaked into a fs.mkdtemp directory, points process.cwd() at it with vi.spyOn (not process.chdir, which the repo uses nowhere and which is unavailable in worker threads), calls vi.resetModules() so the dynamic import('../../src/cli/cli.js') actually re-evaluates rather than hitting the registry entry created by the file's static import, and asserts the sentinel never reaches process.env. The sentinel name is one no production code reads, so the test pins the mechanism rather than piggy-backing on DATABASE_URL (which would be flaky for anyone who genuinely exports it). The afterEach deletes the sentinel and removes the temp dir on every path; the process.cwd spy is restored by the file's existing afterEach(() => vi.restoreAllMocks()).

A second case in the same block calls dotenv.config({quiet: true}) over the same fixture and asserts the sentinel does appear. That is the positive control: without it the first assertion could pass for the wrong reason (a mistyped fixture filename), and it also pins the behaviour the relocated entrypoint call relies on.

Proof each test can fail (mutation runs).

  1. Reverted the dev/src/cli/cli.ts hunk (restored import dotenv from 'dotenv' and the module-scope dotenv.config({quiet: true})) and re-ran npx vitest run --project unit:dev dev/test/cli/cli_test.ts:
FAIL  |unit:dev| dev/test/cli/cli_test.ts > CLI Entrypoint > module import side effects > should not read a .env file from the working directory on import
AssertionError: expected 'leaked' to be undefined
- Expected: undefined
+ Received: "leaked"
 ❯ dev/test/cli/cli_test.ts:451:44
Tests  1 failed | 25 passed (26)
  1. Mutated the fixture path in the test setup from .env to .env.wrong-name to check the positive control is not vacuous:
FAIL  |unit:dev| dev/test/cli/cli_test.ts > CLI Entrypoint > module import side effects > should still load that .env once dotenv.config runs, as the entrypoint does
AssertionError: expected undefined to be 'leaked' // Object.is equality
- Expected: "leaked"
+ Received: undefined

Both mutations were reverted and the suite re-run green.

Commands run (targeted only; the full repo suite was not run):

npx vitest run --project unit:dev dev/test/cli/cli_test.ts   # 26 passed
npx vitest run --project unit:dev                            # 14 files, 225 passed
npx vitest run --project unit:core                           # 168 files, 2351 passed
npm run build                                                # all workspaces, exit 0
npm run lint                                                 # clean
npm run format:check                                         # "All matched files use Prettier code style!"
npm run ts:check                                             # see note

Note on ts:check: the repo currently has a large number of pre-existing tsc --noEmit errors in test files unrelated to this change (this is what PRs #370 / #408 / #421 are about). None of them are in the three files this PR touches — git-grepping the full ts:check output for cli/cli.ts, cli_entrypoint, or cli_test returns nothing, before or after.

Also verified the stated postcondition directly: with printf 'DATABASE_URL=bogus://nope\n' > .env at the repo root, npx vitest run --project unit:dev still reports 14 passed / 225 passed, and node -e "import('./dev/dist/esm/cli/cli.js').then(() => console.log(process.env.DATABASE_URL))" prints undefined — importing the built module no longer injects the file's value.

Coverage. The change in dev/src/cli/cli.ts is a pure deletion, so there are no new lines to cover there; measured coverage of that file under the targeted run is unchanged at 95.3% statements / 73.17% branches. The two added lines in dev/src/cli_entrypoint.ts are not unit-tested, deliberately: that file runs createProgram().parse(process.argv) at module scope, so importing it from a test would execute the CLI. Restructuring it into an exported main() behind an import.meta.url guard just to reach a coverage number would be scope creep on a two-line fix. Those two lines are covered by the manual E2E below instead.

Manual End-to-End (E2E) Tests:

Run against the real built binary (npm run build first, so dev/dist/esm/cli_entrypoint.js reflects the change), from a scratch directory with DATABASE_URL not exported in the shell (echo "${DATABASE_URL-unset}" prints unset):

  1. .env still honoured by the shipped CLI — with printf 'DATABASE_URL=bogus://nope\n' > .env:

    $ node <repo>/dev/dist/esm/cli_entrypoint.js api_server . --port 8124
    [ADK CLI] Error starting API server: Unsupported session service URI: bogus://nope
    

    The URI from the file reached getSessionServiceFromOptions, which is the proof the entrypoint's dotenv.config() ran before the command action. (This doubles as an error-path check: the unsupported-scheme throw from getSessionServiceFromUri is reported exactly as before.)

  2. Explicit flag still wins over the .env value (precedence unchanged) — same .env:

    $ node <repo>/dev/dist/esm/cli_entrypoint.js api_server . --port 8125 --session_service_uri memory://
    | ADK API Server started                                                      |
    
  3. An exported shell variable still beats the .env entry (override: false preserved) — same .env:

    $ DATABASE_URL=memory:// node <repo>/dev/dist/esm/cli_entrypoint.js api_server . --port 8126
    | ADK API Server started                                                      |
    
  4. No new stdout/stderr noise in any run — no dotenv v17 "injected env (N) from .env // tip: …" banner appeared, confirming {quiet: true} survived the move.

  5. Scratch directories and the temporary repo-root .env were deleted afterwards.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits July 31, 2026 20:17
Importing dev/src/cli/cli.ts ran dotenv.config({quiet: true}) during module
evaluation, so merely importing the module read ${cwd}/.env off disk and
merged it into process.env. Move the call to the first statement of the
cli_entrypoint try block, where the shipped adk binary still loads .env
before any command action runs, with the same options and precedence.
Adds a regression case that writes a .env holding a sentinel into a temp
dir, points process.cwd() at it, resets the module registry and re-imports
cli.js, then asserts the sentinel never lands in process.env. A companion
case runs dotenv.config() over the same fixture to prove the fixture is
real and that the entrypoint's call still loads it.
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.

1 participant