Skip to content

Feat: declare the Node.js floor on the workspace root and derive the checked manifest list (stacked on #544) - #549

Open
AmaadMartin wants to merge 2 commits into
feat/declare-node-engines-published-packagesfrom
feat/declare-node-engines
Open

Feat: declare the Node.js floor on the workspace root and derive the checked manifest list (stacked on #544)#549
AmaadMartin wants to merge 2 commits into
feat/declare-node-engines-published-packagesfrom
feat/declare-node-engines

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 2, 2026

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 existing issue.
  2. Or, if no issue exists, describe the change:

Stacked on #544 (feat/declare-node-engines-published-packages), which declares
engines.node on the three published workspaces. This PR is the two-commit delta that
closes the gaps a correctness review found in that base. It is deliberately not a
competing implementation — see "Collision check" below.

Problem: Two gaps remain after #544.

  1. The workspace root declares no floor, and a test forbids adding one. Feat: declare a supported Node.js range (>=20.6.0) on the published packages and enforce it against the lockfile #544 declares
    engines.node on core, dev and integrations only, on the rationale that "a
    declaration there reaches nobody". That is true for registry consumers and false for
    contributors: npm evaluates a project's own engines during npm install. Verified
    directly:

    $ cat package.json   # {"name":"root-engines-probe","engines":{"node":">=99"}}
    $ npm install
    npm WARN EBADENGINE Unsupported engine {
    npm WARN EBADENGINE   package: 'root-engines-probe@1.0.0',
    npm WARN EBADENGINE   required: { node: '>=99' }, current: { node: 'v22.22.2' } }
    $ echo 'engine-strict=true' > .npmrc && npm install
    npm ERR! code EBADENGINE   # hard failure
    

    So a contributor on an unsupported Node currently learns about it from a CI failure
    rather than from npm install. Worse, Feat: declare a supported Node.js range (>=20.6.0) on the published packages and enforce it against the lockfile #544 pins the omission with
    it('leaves engines off the workspace root'), so anyone who wants the root declaration
    has to delete a test to get it.

  2. A workspace added later inherits no coverage. The regression test hardcoded
    const PUBLISHED_WORKSPACES = ['core', 'dev', 'integrations'], so the "cannot be
    omitted from a future workspace" half of the invariant did not exist. Proven by
    mutation: adding a fourth workspace with no engines block left the suite green at
    8/8.

Solution:

  1. Declare "engines": {"node": ">=20.6.0"} on the root package.json, in the same
    position among the metadata keys as the three workspace manifests use (after
    repository). The lockfile hunk is npm re-syncing the root manifest mirror — no
    dependency was added or removed.
  2. Derive the checked manifest list from the root manifest's own workspaces array
    (const MANIFEST_DIRS = ['.', ...]) instead of hardcoding it, so a new workspace
    generates its own assertions and fails until it declares the shared range.

Two consequential test edits, called out explicitly rather than slipped in:

  • Removed it('leaves engines off the workspace root'). It asserted exactly the
    behaviour this PR reverses, so it cannot coexist with the fix. The root is not left
    uncovered: it is now the '.' entry in MANIFEST_DIRS and is covered by
    '%s declares the shared engines.node range' (asserts the exact literal) and
    '%s declares a floor every runtime dependency accepts' (asserts the floor against the
    lockfile) — strictly stronger than the assertion it replaces, which only checked that
    the field was absent.
  • Added it('leaves engines off unpublished test fixtures'). The "which manifests must
    stay without a floor" job that the removed test was doing genuinely does need an owner;
    unpublished fixtures are the correct target for it, since they inherit the floor through
    the workspace they link against.

readManifest became synchronous because it.each needs the derived workspace list at
collection time. readLockfile is untouched.

Range: unchanged from the base branch at >=20.6.0, which is the exact intersection of
the engines.node ranges in the runtime dependency closure (Node 20.5.0 is rejected by 27
@opentelemetry/* packages declaring ^18.19.0 || >=20.6.0; 20.6.0 satisfies every range).
It is grounded in the committed package-lock.json and pinned from below by the lockfile
test. Maintainers should ratify the number consciously rather than inherit it from the
stack
— if you prefer the rounder >=20, it is a one-line edit to EXPECTED_NODE_RANGE
plus the four manifests, and the lockfile test will then (correctly) fail until the OTel
constraint is addressed.

Known imprecision, inherited and not introduced here: the check validates the floor
of the declared range, not every version the range admits. >=20.6.0 also admits Node
21.x, which 3 entries in the closure reject (@ts-morph/common/node_modules/{balanced-match, brace-expansion,minimatch}, range 18 || 20 || >=22). Node 21 is EOL and >=20 has the
same hole, so this is not a defect this PR introduces; flagging it so the test's name is not
read as a stronger guarantee than it makes.

Collision check (required before implementation; recorded here either way). Ran:

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 --json number,title,headRefName

448 open PRs; six are adjacent to this work. #544 (>=20.6.0, 3 manifests, same test
path), #508 (>=20, 3 manifests, same test path, plus .nvmrc/CI), #445 (>=22.12.0, 3
manifests, test under tests/integration/package_manifests/), #133 (root + 3 manifests at
>=22.0.0, no test), #509 (README + root + CI) and #510 (stacked on #508). No single PR
delivers root coverage together with a workspace-derived regression test
, which is the gap
this PR fills. Because the change merely overlaps rather than duplicating, it is stacked on
#544's branch rather than branched from main, so it adds no seventh competing
implementation. The four PRs propose four mutually incompatible floors (>=20, >=20.6.0,
>=22.0.0, >=22.12.0) on the same lines of the same files; a maintainer will need to
pick one and close the rest
, and this delta applies cleanly on top of whichever survives.

eslint.config.js is intentionally untouched: it contains no eslint-plugin-n import and
no version option, so the "drop redundant version options" half of the original task was
vacuous.

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.

$ npx vitest run --project integration tests/integration/packaging/engines_test.ts
 ✓ tests/integration/packaging/engines_test.ts (11 tests) 42ms
   Tests  11 passed (11)        # 8 on the base branch -> 11 here

Mutation testing — every assertion proven able to fail. Each mutation was applied on its
own, run, then reverted (the tree was re-verified clean after each):

# Mutation Result Failure message
1 engines deleted from the root manifest 2 failed | 9 passed expected undefined to be '>=20.6.0' / ./package.json declares engines.node undefined, which is not a satisfiable semver range
2 4th workspace newpkg with no engines, appended to workspaces 2 failed | 11 passed (13) newpkg/package.json declares engines.node undefined, which is not a satisfiable semver range
3 engines deleted from integrations 2 failed | 9 passed integrations/package.json declares engines.node undefined, ...
4 engines added to the unpublished fixture 1 failed | 10 passed expected { node: '>=20.6.0' } to be undefined
5 core floor lowered to >=20.5.0 2 failed | 9 passed engines.node >=20.5.0 admits Node 20.5.0, which the listed runtime dependencies reject ...: expected [ …(27) ] to deeply equal []

Mutations 1 and 2 are the two that passed against the base branch (8/8 green) and now
fail. Mutation 2 also demonstrates the list is genuinely derived: the case count rises 11 →
13 because the new workspace generates its own assertions.

Regression suite (the only place an engines mismatch could surface at install time, since
these fixtures link the workspaces via file:):

$ npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
 Test Files  1 passed (1)
      Tests  20 passed | 4 skipped (24)
   Duration  561.43s

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

Run from the repository root on Node >= 20.6.0:

$ npm install                       # no EBADENGINE warning (local Node v22.22.2)
$ npm run build                     # exit 0
$ npx tsc --noEmit --pretty false   # 281 errors, 0 in the changed file (281 pre-existing on main)
$ npm run lint                      # exit 0, clean
$ npm run format:check
All matched files use Prettier code style!

$ npm pkg get engines -w core -w dev -w integrations
{
  "@google/adk":              {"node": ">=20.6.0"},
  "@google/adk-devtools":     {"node": ">=20.6.0"},
  "@google/adk-integrations": {"node": ">=20.6.0"}
}
$ npm pkg get engines         # root, new in this PR
{"node": ">=20.6.0"}

$ npm pack --dry-run -w core && npm pack --dry-run -w dev && npm pack --dry-run -w integrations
# all three succeed; each tarball lists package.json

CI note: this PR is stacked on #544, so its base is not main. The validation workflow is
scoped to pull_request: branches: [main] and therefore never triggers here — only
auto-assign ran. Everything above was run locally against the exact pushed commit
6c62a1d2.

To see the consumer-facing effect, install on Node < 20.6.0 and observe
npm warn EBADENGINE; with engine-strict=true in .npmrc the install fails outright.

npm install after the change produces no lockfile churn beyond the 3-line root
manifest mirror shown in the diff.

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 2, 2026 16:32
The published workspaces declare engines.node, but the workspace root did
not, so a contributor running npm install in the repo root got no signal
about an unsupported Node version until CI failed. npm evaluates a
project's own engines during install, emitting EBADENGINE (a hard failure
under engine-strict), so the root declaration is what makes the floor
visible at the point of onboarding.

The lockfile change is npm re-syncing the root manifest mirror; no
dependency was added or removed.
The manifest list was hardcoded, so a workspace added later inherited no
coverage: adding a fourth workspace with no engines block left the suite
green. Deriving the list from the root manifest's workspaces array makes
a new workspace generate its own assertions, which fail until it declares
the shared range.

Replaces the assertion that the root leaves engines undefined, now that
the root declares it; the root is covered by the derived list like any
other manifest. The unpublished-fixture guard takes over the job of
pinning which manifests must stay without a floor.
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