From 0fe2ca539c5f83f3d15cc6fddf311dd3cc4b373e Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Fri, 31 Jul 2026 05:06:39 -0700 Subject: [PATCH 1/3] fix(dev): declare @types/express as a runtime dependency AdkApiServer exposes `readonly app: express.Application`, and the class is exported from dev/src/index.ts, so dev/dist/types/server/adk_api_server.d.ts emits `import express from 'express'`. express@4 ships no bundled types, so a package whose public declarations reference them must declare @types/express itself rather than rely on a sibling workspace's dependency being hoisted. The lockfile delta is the manifest move plus one incidental correction npm makes on any install: the hoisted adm-zip entry loses a stale "dev": true, which it earns from core listing adm-zip as a runtime dependency. --- dev/package.json | 2 +- package-lock.json | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/dev/package.json b/dev/package.json index 717b930e7..94e83f70a 100644 --- a/dev/package.json +++ b/dev/package.json @@ -41,7 +41,6 @@ }, "devDependencies": { "@types/cors": "^2.8.19", - "@types/express": "^4.17.21", "@types/node": "^20.12.7", "@typescript-eslint/eslint-plugin": "^7.7.1", "@typescript-eslint/parser": "^7.7.1", @@ -61,6 +60,7 @@ "@mikro-orm/mysql": "^6.6.6", "@mikro-orm/postgresql": "^6.6.6", "@mikro-orm/sqlite": "^6.6.6", + "@types/express": "^4.17.21", "camelcase-keys": "^6.2.2", "commander": "^14.0.0", "cors": "^2.8.5", diff --git a/package-lock.json b/package-lock.json index 8dcde50f7..9a08926e6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -123,6 +123,7 @@ "@mikro-orm/mysql": "^6.6.6", "@mikro-orm/postgresql": "^6.6.6", "@mikro-orm/sqlite": "^6.6.6", + "@types/express": "^4.17.21", "camelcase-keys": "^6.2.2", "commander": "^14.0.0", "cors": "^2.8.5", @@ -141,7 +142,6 @@ }, "devDependencies": { "@types/cors": "^2.8.19", - "@types/express": "^4.17.21", "@types/node": "^20.12.7", "@typescript-eslint/eslint-plugin": "^7.7.1", "@typescript-eslint/parser": "^7.7.1", @@ -5196,7 +5196,6 @@ "version": "0.5.17", "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.5.17.tgz", "integrity": "sha512-+Ut8d9LLqwEvHHJl1+PIHqoyDxFgVN847JTVM3Izi3xHDWPE4UtzzXysMZQs64DMcrJfBeS/uoEP4AD3HQHnQQ==", - "dev": true, "license": "MIT", "engines": { "node": ">=12.0" From 2d40fdcbd15d9108690edb897ed4e1c5ef75de91 Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Fri, 31 Jul 2026 05:06:50 -0700 Subject: [PATCH 2/3] feat(ci): gate the published type closure against a scratch consumer Adds `npm run check:published-types`, run by a new `published-types` CI job. It packs every publishable workspace, installs the tarballs into a throwaway project under the OS temp directory, and type-checks it, failing on TS2307 / TS7016 anywhere in the output. Three properties are load-bearing, and each defeats a distinct reason the existing checks cannot see a phantom type dependency: the probe lives outside the repository (npm hoists every workspace dependency into the repo-root node_modules, so anything checked from inside the tree resolves types a consumer never receives); it installs packed tarballs rather than `file:` links, so the declared dependencies closure is what gets tested; and skipLibCheck is false, without which an unresolvable module in a dependency's .d.ts degrades silently to `any` and the check passes on a broken tree. A separate job rather than a step in the run-tests matrix: the check needs one network install of the full closure and only needs proving once, and running in parallel costs no extra wall clock. --- .github/workflows/validation.yaml | 21 ++- package.json | 1 + scripts/check_published_types.mjs | 270 ++++++++++++++++++++++++++++++ 3 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 scripts/check_published_types.mjs diff --git a/.github/workflows/validation.yaml b/.github/workflows/validation.yaml index 7bb00098e..0cde24342 100644 --- a/.github/workflows/validation.yaml +++ b/.github/workflows/validation.yaml @@ -7,7 +7,7 @@ on: branches: [main] env: - NODE_OPTIONS: "--max-old-space-size=8192" + NODE_OPTIONS: '--max-old-space-size=8192' jobs: run-tests: @@ -48,3 +48,22 @@ jobs: - name: Run documentation build check run: npm run docs:check + + published-types: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v6 + + - name: Use Node.js + uses: actions/setup-node@v6 + + - name: Install dependencies + run: npm install + + - name: Build packages + run: npm run build + + - name: Typecheck a scratch consumer against the published closure + run: npm run check:published-types diff --git a/package.json b/package.json index 86cee8241..d8914e08e 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,7 @@ "clean:all": "rm package-lock.json && rm -rf ./node_modules && npm run clean:all --workspaces", "rebuild": "npm run clean:all && npm install && npm run build", "ts:check": "tsc --noEmit", + "check:published-types": "node scripts/check_published_types.mjs", "lint": "eslint \"**/*.ts\"", "lint:fix": "eslint --fix \"**/*.ts\"", "format": "prettier \"**/*.ts\" --write", diff --git a/scripts/check_published_types.mjs b/scripts/check_published_types.mjs new file mode 100644 index 000000000..14a1c44c0 --- /dev/null +++ b/scripts/check_published_types.mjs @@ -0,0 +1,270 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Fails when a published package's declaration files reference a module that + * package does not list in its `dependencies`. + * + * Every publishable workspace is packed with `npm pack`, the tarballs are + * installed into a throwaway consumer project, and that project is + * type-checked. Three properties make the check meaningful; each one defeats a + * reason the repository's other checks cannot see this class of defect: + * + * - The scratch project lives under the OS temp directory, never inside the + * repository. npm hoists every workspace dependency into the repo-root + * `node_modules`, so anything type-checked from a path inside the repo walks + * up into that directory and resolves types a consumer never receives. + * - It installs packed tarballs, not `file:` links to the workspace + * directories, so what gets installed is the declared `dependencies` + * closure rather than the development tree. + * - `skipLibCheck` is false. With it on, an unresolvable module inside a + * dependency's `.d.ts` degrades silently to `any` and this check passes on a + * broken tree. + * + * `@types/node` belongs to the probe's toolchain rather than to any package's + * `dependencies`: published declarations do reference Node globals, but every + * Node TypeScript consumer already installs `@types/node`, and pinning its + * major from a library is a known source of consumer version conflicts. + * + * The verdict is the one a consumer sees, not a per-package one. All packages + * are installed side by side, so hoisting lets one workspace's declared + * dependency satisfy a sibling workspace's undeclared import. Per-package + * declaration hygiene is not enforced here. + */ + +import {execFileSync} from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import {fileURLToPath} from 'node:url'; + +const REPO_ROOT = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..', +); + +/** Diagnostics meaning "a module in the published closure did not resolve". */ +const UNRESOLVED_MODULE_CODES = new Set(['TS2307', 'TS7016']); + +const DIAGNOSTIC_PATTERN = /error (TS\d+):/; + +const PROBE_TSCONFIG = { + compilerOptions: { + target: 'ES2022', + module: 'nodenext', + moduleResolution: 'nodenext', + strict: true, + noEmit: true, + skipLibCheck: false, + types: ['node'], + }, + include: ['src/**/*.ts'], +}; + +function readJson(file) { + return JSON.parse(fs.readFileSync(file, 'utf8')); +} + +function indent(line) { + return ` ${line}`; +} + +/** + * Runs npm without a shell. `npm run` exports npm's own CLI entry point as + * `npm_execpath`; invoking that with the current node binary sidesteps both + * shell quoting (semver ranges contain `^`, which cmd.exe eats) and Windows' + * `npm.cmd`, which `execFileSync` cannot spawn directly. + */ +function npm(args, cwd) { + const npmCli = process.env.npm_execpath; + return npmCli + ? execFileSync(process.execPath, [npmCli, ...args], {cwd, encoding: 'utf8'}) + : execFileSync('npm', args, {cwd, encoding: 'utf8'}); +} + +/** Publishable workspaces, as `{name, dir}` pairs. */ +function publishablePackages() { + const {workspaces} = readJson(path.join(REPO_ROOT, 'package.json')); + const packages = []; + for (const workspace of workspaces) { + const dir = path.join(REPO_ROOT, workspace); + const manifest = readJson(path.join(dir, 'package.json')); + if (!manifest.private) { + packages.push({name: manifest.name, dir}); + } + } + return packages; +} + +function assertBuilt(packages) { + for (const {dir} of packages) { + const declarations = path.join(dir, 'dist', 'types', 'index.d.ts'); + if (!fs.existsSync(declarations)) { + const relative = path.relative(REPO_ROOT, declarations); + throw new Error(`${relative} not found; run \`npm run build\` first.`); + } + } +} + +/** + * `typescript` and `@types/node` at the repository's own ranges, so the probe's + * toolchain cannot drift from the one the packages are built with. + */ +function toolchainSpecs() { + const {devDependencies} = readJson(path.join(REPO_ROOT, 'package.json')); + return ['typescript', '@types/node'].map( + (name) => `${name}@${devDependencies[name]}`, + ); +} + +/** + * The tarball directory is read back rather than `npm pack --json` parsed: npm + * writes notices to stderr and the JSON shape has moved between majors. + */ +function packAll(packages, tarballDir) { + fs.mkdirSync(tarballDir, {recursive: true}); + for (const {dir} of packages) { + npm(['pack', '--pack-destination', tarballDir, '--loglevel=error'], dir); + } + const tarballs = fs + .readdirSync(tarballDir) + .filter((entry) => entry.endsWith('.tgz')) + .map((entry) => path.join(tarballDir, entry)); + if (tarballs.length !== packages.length) { + throw new Error( + `Expected ${packages.length} tarballs in ${tarballDir}, ` + + `found ${tarballs.length}.`, + ); + } + return tarballs; +} + +function probeFileName(packageName) { + return packageName.replace(/[^a-z0-9]+/gi, '_').replace(/^_|_$/g, ''); +} + +function writeProbeProject(scratchDir, packages) { + fs.writeFileSync( + path.join(scratchDir, 'package.json'), + JSON.stringify({ + name: 'adk-published-types-probe', + private: true, + version: '0.0.0', + type: 'module', + }), + ); + fs.writeFileSync( + path.join(scratchDir, 'tsconfig.json'), + JSON.stringify(PROBE_TSCONFIG), + ); + const srcDir = path.join(scratchDir, 'src'); + fs.mkdirSync(srcDir); + for (const {name} of packages) { + // `export type Probe` is load-bearing: with the namespace unused the import + // is elided and the package's declaration graph is never pulled in. + fs.writeFileSync( + path.join(srcDir, `${probeFileName(name)}.ts`), + `import * as pkg from '${name}';\nexport type Probe = typeof pkg;\n`, + ); + } +} + +/** + * `--legacy-peer-deps` is deliberate: core declares five `@mikro-orm/*` drivers + * as non-optional peers, and which database driver a consumer picks is not part + * of the `dependencies` closure under test here. + */ +function installClosure(scratchDir, tarballs) { + npm( + [ + 'install', + ...tarballs, + ...toolchainSpecs(), + '--no-audit', + '--no-fund', + '--legacy-peer-deps', + '--loglevel=error', + ], + scratchDir, + ); +} + +function typecheck(scratchDir) { + const tsc = path.join(scratchDir, 'node_modules', 'typescript', 'bin', 'tsc'); + const args = [tsc, '--noEmit', '-p', 'tsconfig.json']; + try { + return execFileSync(process.execPath, args, { + cwd: scratchDir, + encoding: 'utf8', + }); + } catch (error) { + // tsc exits non-zero for any diagnostic, so diagnostics on stdout are the + // result. An empty stdout means tsc itself failed to run. + if (error.stdout) { + return error.stdout; + } + throw error; + } +} + +function classify(diagnostics) { + const unresolved = []; + const other = []; + for (const line of diagnostics.split('\n')) { + const match = DIAGNOSTIC_PATTERN.exec(line); + if (match) { + const bucket = UNRESOLVED_MODULE_CODES.has(match[1]) ? unresolved : other; + bucket.push(line.trim()); + } + } + return {unresolved, other}; +} + +function removeScratchDir(scratchDir) { + try { + fs.rmSync(scratchDir, {recursive: true, force: true, maxRetries: 3}); + } catch (error) { + // Cleanup must never mask the verdict the check just produced. + process.stderr.write(`Could not remove ${scratchDir}: ${error.message}\n`); + } +} + +function main() { + const packages = publishablePackages(); + assertBuilt(packages); + + const scratchDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'adk-published-types-'), + ); + let diagnostics; + try { + const tarballs = packAll(packages, path.join(scratchDir, 'tarballs')); + writeProbeProject(scratchDir, packages); + installClosure(scratchDir, tarballs); + diagnostics = typecheck(scratchDir); + } finally { + removeScratchDir(scratchDir); + } + + const {unresolved, other} = classify(diagnostics); + if (other.length > 0) { + const listed = other.map(indent).join('\n'); + process.stdout.write(`Unrelated diagnostics, ignored:\n${listed}\n`); + } + if (unresolved.length > 0) { + const listed = unresolved.map(indent).join('\n'); + process.stdout.write( + `Published declarations reference modules the packages do not ` + + `declare:\n${listed}\nMove each missing package into the ` + + `"dependencies" of the workspace owning the dist/types file above.\n`, + ); + process.exit(1); + } + const names = packages.map(({name}) => name).join(', '); + process.stdout.write(`Published type closure resolves for: ${names}.\n`); +} + +main(); From b2fcc63a22c1423eaeda02dd60d145102326231f Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Fri, 31 Jul 2026 11:21:48 -0700 Subject: [PATCH 3/3] refactor(ci): trim the published-types check to what decides the verdict Review follow-ups on scripts/check_published_types.mjs: - Drop the "unrelated diagnostics, ignored" bucket. Those diagnostics were collected, formatted and printed but never consumed and could not influence the exit code; they stay non-fatal, they are simply no longer echoed. - Drop the tarball-count guard. The directory is created fresh under the mkdtemp scratch root, npm pack writes exactly one .tgz per invocation and execFileSync already throws on a non-zero exit, so it could not fire. - Inline the one-line indent() helper into its single remaining caller. - Correct the npm() docstring: execFileSync without shell:true never routes through cmd.exe, so shell quoting was never the reason. The npm.cmd spawn restriction on Windows is, and that is now all it claims. Also reverts an incidental requote of NODE_OPTIONS in validation.yaml; nothing in this repo formats YAML, so that hunk was editor churn. --- .github/workflows/validation.yaml | 2 +- scripts/check_published_types.mjs | 51 +++++++++++-------------------- 2 files changed, 19 insertions(+), 34 deletions(-) diff --git a/.github/workflows/validation.yaml b/.github/workflows/validation.yaml index 0cde24342..f63674013 100644 --- a/.github/workflows/validation.yaml +++ b/.github/workflows/validation.yaml @@ -7,7 +7,7 @@ on: branches: [main] env: - NODE_OPTIONS: '--max-old-space-size=8192' + NODE_OPTIONS: "--max-old-space-size=8192" jobs: run-tests: diff --git a/scripts/check_published_types.mjs b/scripts/check_published_types.mjs index 14a1c44c0..a91489e98 100644 --- a/scripts/check_published_types.mjs +++ b/scripts/check_published_types.mjs @@ -68,15 +68,10 @@ function readJson(file) { return JSON.parse(fs.readFileSync(file, 'utf8')); } -function indent(line) { - return ` ${line}`; -} - /** - * Runs npm without a shell. `npm run` exports npm's own CLI entry point as - * `npm_execpath`; invoking that with the current node binary sidesteps both - * shell quoting (semver ranges contain `^`, which cmd.exe eats) and Windows' - * `npm.cmd`, which `execFileSync` cannot spawn directly. + * Runs npm. `npm run` exports npm's own CLI entry point as `npm_execpath`; + * invoking that with the current node binary keeps this working on Windows, + * where npm is `npm.cmd` and `execFileSync` cannot spawn it directly. */ function npm(args, cwd) { const npmCli = process.env.npm_execpath; @@ -129,17 +124,10 @@ function packAll(packages, tarballDir) { for (const {dir} of packages) { npm(['pack', '--pack-destination', tarballDir, '--loglevel=error'], dir); } - const tarballs = fs + return fs .readdirSync(tarballDir) .filter((entry) => entry.endsWith('.tgz')) .map((entry) => path.join(tarballDir, entry)); - if (tarballs.length !== packages.length) { - throw new Error( - `Expected ${packages.length} tarballs in ${tarballDir}, ` + - `found ${tarballs.length}.`, - ); - } - return tarballs; } function probeFileName(packageName) { @@ -210,17 +198,18 @@ function typecheck(scratchDir) { } } -function classify(diagnostics) { - const unresolved = []; - const other = []; - for (const line of diagnostics.split('\n')) { - const match = DIAGNOSTIC_PATTERN.exec(line); - if (match) { - const bucket = UNRESOLVED_MODULE_CODES.has(match[1]) ? unresolved : other; - bucket.push(line.trim()); - } - } - return {unresolved, other}; +/** + * Only unresolved-module diagnostics decide the verdict. Any other type error + * inside a dependency's own declarations is somebody else's bug and must not + * redden this job. + */ +function unresolvedModuleDiagnostics(diagnostics) { + return diagnostics + .split('\n') + .filter((line) => + UNRESOLVED_MODULE_CODES.has(DIAGNOSTIC_PATTERN.exec(line)?.[1]), + ) + .map((line) => line.trim()); } function removeScratchDir(scratchDir) { @@ -249,13 +238,9 @@ function main() { removeScratchDir(scratchDir); } - const {unresolved, other} = classify(diagnostics); - if (other.length > 0) { - const listed = other.map(indent).join('\n'); - process.stdout.write(`Unrelated diagnostics, ignored:\n${listed}\n`); - } + const unresolved = unresolvedModuleDiagnostics(diagnostics); if (unresolved.length > 0) { - const listed = unresolved.map(indent).join('\n'); + const listed = unresolved.map((line) => ` ${line}`).join('\n'); process.stdout.write( `Published declarations reference modules the packages do not ` + `declare:\n${listed}\nMove each missing package into the ` +