Skip to content

Fix: align dev's express and @types/express ranges with core - #637

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/align-express-range-across-workspaces
Open

Fix: align dev's express and @types/express ranges with core#637
AmaadMartin wants to merge 2 commits into
mainfrom
fix/align-express-range-across-workspaces

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: core and dev both ship express as a runtime dependency, but declared different semver ranges for it, and the same for its type package:
Package core/package.json dev/package.json
express (dependencies) ^4.22.1 ^4.21.2
@types/express (devDependencies) ^4.17.25 ^4.17.21

express was the only divergent runtime dependency in the repo — every other package declared by more than one workspace already uses an identical range string (js-yaml ^4.1.1, winston ^3.19.0, zod ^4.2.1, @google/adk ^1.5.0).

This is not a live runtime defect: the lockfile hoists exactly one express@4.22.2 and one @types/express@4.17.25, and 4.22.2 satisfies both ranges. The defect is in the declared contract, and it has two consequences:

  1. The manifests give a false signal. A reader of dev/package.json reasonably concludes devtools was deliberately kept compatible with express 4.21. It was not; the range is simply stale, and because whichever workspace resolves first wins the hoist, the divergence is invisible in practice and stays wrong indefinitely.
  2. @google/adk-devtools is published to npm with package.json in its files array, so the stale floor is part of a released package's public contract.

Solution: Align dev up to core (express: ^4.22.1, @types/express: ^4.17.25) and regenerate the lockfile. Aligning up rather than down is deliberate: core's floor is the newer one, the installed tree already runs 4.22.2, and lowering core would weaken the published floor of @google/adk for no benefit.

Not a breaking change; patch-level. express is a regular dependencies entry of @google/adk-devtools, not a peer dependency, so narrowing it cannot cause an ERESOLVE in a consumer install. A consumer already inherits express@^4.22.1 transitively, because @google/adk-devtools depends on @google/adk@^1.5.0 which demands it — so the effective resolution downstream is unchanged; only the manifest now states it honestly. @types/express is a devDependency and is never installed by consumers.

The change also adds the missing regression guard. Nothing in the repo prevented this drift from reappearing, which is the actual root cause. tests/integration/workspace_manifests/dependency_alignment_test.ts derives the workspace list from the root manifest's workspaces array, indexes every dependencies/devDependencies entry, and fails when one package name is declared with more than one range string — naming every workspace (field) -> range site so the fix is visible without opening four files. peerDependencies are excluded (a peer range is deliberately allowed to be wider than the dependency range satisfying it) and the root manifest is excluded (it is the tooling root, and it carries separately-tracked eslint/prettier drift that is a different question).

Out of scope, stated explicitly: the root manifest also diverges from dev on eslint (^9.37.0 vs ^8.57.0) and prettier (^3.6.2 vs ^3.2.5). That is dev-tooling-only and materially different (the dev-local lint scripts may be dead config); it is tracked separately and is not touched here. No file under core/src, dev/src, or integrations/src is modified, and no coverage threshold is touched.

Collision check (required before implementation): all 536 open PRs on the fork were listed and every plausibly adjacent one was diffed. No PR changes an express or @types/express range string, and none adds a cross-workspace range-alignment guard, so this is not a duplicate. Three PRs overlap by touching the same manifest lines without changing any range: #562 and #382 relocate dev's @types/express from devDependencies to dependencies (keeping ^4.17.21), and #292 does the same for core (keeping ^4.17.25). This PR is not stacked on any of them, deliberately: those three are mutually competing siblings (two of them make the same dev edit in different ways), so stacking would import a contested diff and pick a winner among them. This change composes with whichever lands, because the guard indexes by package name across both dependency fields — it does not care which field an entry lives in — and the manifest edit is a value change that survives the field move as a trivial conflict resolution.

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.

New guard test (run targeted, not via the full suite):

npx vitest run --project integration tests/integration/workspace_manifests/dependency_alignment_test.ts
# ✓ tests/integration/workspace_manifests/dependency_alignment_test.ts (1 test) 7ms

Existing express-dependent suites, re-run unmodified — no existing test was edited, skipped, or deleted:

npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts
# ✓ 51 passed (51)
npx vitest run --project unit:core core/test/a2a/agent_to_a2a_test.ts \
  core/test/a2a/agent_to_a2a_body_parsing_test.ts core/test/a2a/auth_test.ts
# ✓ 19 passed (19)

Proof the new test can fail (mutation). Both assertions were run against mutated inputs and confirmed to fail:

  1. Reverted the fix (git checkout HEAD~1 -- dev/package.json, restoring ^4.21.2 / ^4.17.21) — fails with exactly the two expected violations and no others:
AssertionError: expected [ …(2) ] to deeply equal []
- []
+ [
+   "express: core (dependencies) -> ^4.22.1, dev (dependencies) -> ^4.21.2",
+   "@types/express: core (devDependencies) -> ^4.17.25, dev (devDependencies) -> ^4.17.21",
+ ]
  1. Emptied the root workspaces array — the anti-vacuity assertion fires, so a manifest-reading change that silently scans nothing cannot pass:
AssertionError: expected 0 to be greater than 1
 ❯ tests/integration/workspace_manifests/dependency_alignment_test.ts:62:31

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

Coverage: the production change is manifest-only, so there is no new production line to cover; the coverage include globs are core/src, dev/src, integrations/src and the new test imports no production module, so the configured thresholds are unaffected and were not touched.

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

npm install
npm ls express
npm ls @types/express
git diff --stat package-lock.json
npm run build
node dev/dist/esm/cli_entrypoint.js api_server --port 8791 tests/integration/adk_web/agent
curl http://localhost:8791/list-apps
curl -X POST -H 'Content-Type: application/json' -d '{"state":{"k":"v"}}' \
  http://localhost:8791/apps/agent/users/u1/sessions/s1

npm ls express — a single deduped express@4.22.2 for both packages, and the MCP SDK keeps its own nested express@5.2.1 as before:

adk@1.5.0
├─┬ @google/adk-devtools@1.5.0 -> ./dev
│ └── express@4.22.2
└─┬ @google/adk@1.5.0 -> ./core
  ├─┬ @a2a-js/sdk@0.3.13
  │ └── express@4.22.2 deduped
  ├─┬ @modelcontextprotocol/sdk@1.29.0
  │ ├─┬ express-rate-limit@8.5.2
  │ │ └── express@4.22.2 deduped
  │ └── express@5.2.1
  └── express@4.22.2 deduped

npm ls @types/express — a single @types/express@4.17.25:

adk@1.5.0
├─┬ @google/adk-devtools@1.5.0 -> ./dev
│ └── @types/express@4.17.25
└─┬ @google/adk@1.5.0 -> ./core
  └── @types/express@4.17.25 deduped

The install produced no ERESOLVE and the entire package-lock.json delta is the two mirrored range strings under packages/dev. Nothing under node_modules/** in the lockfile changed, and no new nested node_modules/*/node_modules/express appeared:

@@ -107,7 +107,7 @@
         "esbuild-shim-plugin": "^1.0.3",
-        "express": "^4.21.2",
+        "express": "^4.22.1",
@@ -119,7 +119,7 @@
         "@types/cors": "^2.8.19",
-        "@types/express": "^4.17.21",
+        "@types/express": "^4.17.25",

Real server boot on the reconciled tree (no mocks — a genuine express app over HTTP): GET /list-apps returned 200 with ["agent"], and POST /apps/agent/users/u1/sessions/s1 exercised express.json() body parsing, returning 200 with {"id":"s1","appName":"agent","userId":"u1","state":{"k":"v"},"events":[],"lastUpdateTime":...}.

Repo checks that CI runs, on the exact pushed commit: npm run build (all three workspaces), npm run lint (exit 0), npm run format:check (exit 0), npx secretlint (exit 0), scripts/check_license.sh (exit 0).

One honest caveat. npm run ts:check exits non-zero, but it does so identically on the base commit: 281 tsc --noEmit errors before and after, with a byte-identical set of file:line:col locations, none in any file this PR touches. It is pre-existing and out of scope here (CI does not run ts:check; separate PRs address it). The new test file itself produces zero type errors.

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 August 4, 2026 07:05
core declares express ^4.22.1 / @types/express ^4.17.25 while dev declared
^4.21.2 / ^4.17.21. Both workspaces already resolve the single hoisted
express@4.22.2 and @types/express@4.17.25, so dev's ranges were a stale,
untrue floor that ships in the published @google/adk-devtools manifest.

Align up to core: core's floor is the newer one, and lowering it would weaken
the published floor of @google/adk for no benefit. The lockfile delta is the
two mirrored range strings under packages/dev; no resolved version changes.
Nothing in the repo prevented the express range drift from reappearing, which
is the actual root cause. Index every dependencies/devDependencies entry of the
workspaces named by the root manifest and fail when one package name is
declared with more than one range string, naming every site.

peerDependencies are excluded (a peer range is deliberately wider), as is the
root manifest (tooling root, and it carries separately-tracked eslint/prettier
drift).
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