Skip to content

Fix: declare dotenv in the workspace root manifest - #602

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/root-manifest-declare-dotenv
Open

Fix: declare dotenv in the workspace root manifest#602
AmaadMartin wants to merge 1 commit into
mainfrom
fix/root-manifest-declare-dotenv

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):
    No existing issue.

  2. Or, if no issue exists, describe the change:

Problem: tests/ imports dotenv but no manifest that owns tests/ declares it.

20 of the 26 suites under tests/e2e/** contain a top-level import * as dotenv from 'dotenv'. tests/ is not in the root workspaces array (["core", "dev", "integrations"]), so those bare specifiers resolve by Node's upward node_modules walk, terminating at the repository-root node_modules. Nothing in the root package.json asks for dotenv — the only declaration in the repo is dev/package.json:67 ("dotenv": "^17.2.3", a real runtime dependency of @google/adk-devtools, used by dev/src/cli/cli.ts and dev/src/cli/cli_create.ts). npm hoists a single copy to the root and the e2e tree picks it up incidentally.

So the e2e tree depends on an undeclared, incidental property of another workspace's dependency list. If dev ever drops or renames dotenv, npm prunes the hoisted root copy and 20 suites stop resolving. Two details make that worse than it sounds:

  1. The suites gate their bodies on credentials (describe.skipIf(!hasAKey)), but the dotenv import is a static top-level import. Resolution happens at collection time, before any skip logic runs — so the failure would hit even on machines with no API keys, including CI.
  2. CI exercises this path: .github/workflows/validation.yaml runs npm install then npm run test:coverage, which includes --project e2e.

Solution: declare dotenv in the root devDependencies at the same range the dev workspace uses (^17.2.3), and record it in the lockfile.

This is manifest hygiene, not a behaviour change. devDependencies (not dependencies) because the root package is the monorepo shell and dotenv is needed only to run tests from it. The range is byte-identical to dev/package.json deliberately — a different range is how you end up with two installed copies later.

Precedent: commit 00f37755, "fix(deps): hoist @google/genai 2.x to the workspace root" (google#564), solved this exact class of problem the same way — one line added to root devDependencies plus the lock refresh. That commit is why this uses root devDependencies rather than inventing a tests/package.json or adding a tests workspace entry.

Deliberately out of scope: four other packages that tests/ imports rely on the same hoisting (zod, @google-cloud/vertexai, openapi-types, @modelcontextprotocol/sdk). They are tracked separately so this diff stays a one-line manifest change.

Overlapping-PR disclosure (collision check): before implementing I checked all 500 open PRs on the fork and diff-checked 13 plausibly adjacent manifest/lockfile PRs (#250, #338, #345, #415, #425, #467, #507, #541, #544, #549, #575, #170) for an added dotenv line. One overlaps: #360 ("Fix: declare the root workspace's test dependencies in package.json") inserts this identical line, bundled with four other packages (@google-cloud/vertexai, @mikro-orm/core, openapi-types, zod) in the same commit. At the time of writing #360 is open, unreviewed, and CONFLICTING against main. These two PRs will textually conflict on this one line — whichever lands first, the other should drop the dotenv hunk (or be closed as redundant). This PR is the narrow, single-purpose version; #360 is the bundled one. Flagging it so a maintainer picks deliberately rather than discovering the conflict at merge time.

Diff — exactly two files, two insertions:

 package-lock.json | 1 +
 package.json      | 1 +
 2 files changed, 2 insertions(+)
--- a/package.json
+++ b/package.json
@@ -46,6 +46,7 @@
     "@types/node": "^20.12.7",
     "@vitest/coverage-v8": "^3.2.4",
     "concurrently": "^9.2.1",
+    "dotenv": "^17.2.3",
     "esbuild": "^0.25.9",

The lock delta is the matching key in packages[""].devDependencies. No package was added, removed, or re-resolved, and no "dev" flag changed anywhere in the lock.

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.

No unit test accompanies this change, deliberately. It adds zero executable lines, so line/branch coverage over it is undefined rather than 100% — there is nothing for a unit test to execute, and a test asserting the contents of package.json would be a tautology with no precedent in this repo (the @google/genai equivalent, google#564, shipped with no test). Coverage thresholds in vitest.config.ts are untouched and unaffected: the coverage include globs cover only core/src, dev/src, and integrations/src.

What replaces coverage is a falsification experiment — the "prove the test can fail" analogue. It is what demonstrates this line is load-bearing rather than cosmetic, and a reviewer cannot reconstruct it from the diff.

Falsification experiment (run in a throwaway directory outside the worktree, since deleted). Both arms simulate the refactor this change defends against: "dotenv": "^17.2.3" deleted from the copied dev/package.json, then npm install --package-lock-only --ignore-scripts. The arms differ only in whether the root manifest declares dotenv.

Arm Root declares dotenv? Result: packages["node_modules/dotenv"]
Control (pre-change root manifest) no ABSENT — only the unrelated nested node_modules/@mikro-orm/core/node_modules/dotenv@17.3.1 survives, which files under tests/ cannot resolve. This is the bug.
Treatment (this PR's root manifest) yes 17.4.2, still present. This is the fix.

The control arm is the failure this change prevents: in a real install, tests/e2e/** would fail with Cannot find package 'dotenv' at collection time.

Unit Tests:
[x] I have added or updated unit tests for my change. — N/A, justified above: the change adds no executable code. The falsification experiment above is the substitute evidence.
[x] All unit tests pass locally. — no test outcome changes; see the e2e before/after below.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

From the repository root:

  1. npm install --registry=https://registry.npmjs.org — succeeded; left the lock at the single-line diff above (git diff --stat re-verified after the full install: still 2 insertions, no churn).

  2. npm ls dotenvdotenv@17.4.2 is now a direct dependency of the root adk@1.5.0, and still exactly one copy at the root:

    adk@1.5.0
    ├─┬ @google/adk-devtools@1.5.0 -> ./dev
    │ └── dotenv@17.4.2 deduped
    ├─┬ @google/adk@1.5.0 -> ./core
    │ └─┬ @mikro-orm/core@6.6.14
    │   └── dotenv@17.3.1
    └── dotenv@17.4.2
    

    Before the change the root entry was absent; dotenv appeared only under the dev workspace and, nested, under @mikro-orm/core.

  3. npm run build, then npx vitest run --project e2eall 26 e2e files collect, with zero module-resolution errors (grep for Cannot find package / ERR_MODULE_NOT_FOUND / Failed to resolve import returns 0 hits).

    Outcome is identical before and after the change, which is the point — this is a manifest-hygiene change with no behavioural delta:

    Test Files Tests
    Before (git stash) 16 failed | 6 passed | 4 skipped (26) 22 failed | 11 passed | 12 skipped (45)
    After 16 failed | 6 passed | 4 skipped (26) 22 failed | 11 passed | 12 skipped (45)

    To be explicit about what those failures are, since it matters for reading this PR: they are credential failures on a machine with no GEMINI_API_KEY / GOOGLE_GENAI_API_KEY / GOOGLE_CLOUD_PROJECT (e.g. Error: API key must be provided via constructor or GOOGLE_GENAI_API_KEY or GEMINI_API_KEY environment variable from core/src/models/google_llm.ts:119). They are pre-existing and unrelated — identical on the base commit — and are collection-clean, which is the property this change is about. Suites whose credential gate is evaluated inside describe.skipIf report as skipped; suites that construct a model in the describe body surface the credential error instead. Neither is a resolution error.

  4. npm run ts:check — unchanged from the base commit. Its pre-existing errors (in core/test/utils/live_connection_utils_test.ts, tests/integration/agents/agent_with_sandbox_executor_test.ts, tests/integration/sessions/vertex_ai_session_service_test.ts) are untouched by a manifest-only change.

Per the repo's guidance I did not run the full npm test / npm run test:coverage; the targeted commands above are the relevant ones.

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. — N/A, JSON manifests only.
[x] I have added tests that prove my fix is effective or that my feature works. — see the falsification experiment; no executable code is added, so no unit test.
[x] New and existing unit tests pass locally with my changes.

Twenty of the 26 suites under tests/e2e statically import dotenv, but
tests/ belongs to no workspace, so the import resolves only as a side
effect of npm hoisting the dev workspace's dependency to the repository
root. If @google/adk-devtools ever drops dotenv, npm prunes the hoisted
copy and those suites stop resolving their import at collection time --
before their credential skipIf gates can run.

Declare dotenv in the root devDependencies at the same range the dev
workspace uses, so the manifest that owns tests/ states the requirement
it actually has. The installed tree is unchanged: dotenv@17.4.2 already
satisfies ^17.2.3 and stays the single root copy.
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