Fix: declare @types/express in dev, and gate the published type closure in CI - #382
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: declare @types/express in dev, and gate the published type closure in CI#382AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 2 commits
July 31, 2026 05:06
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.
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.
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.
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: #issue_number
Related: #issue_number
Problem: a package's published
.d.tsfiles can reference a module the package does not declare in itsdependencies. Those declarations ship to consumers, but adevDependencyis never installed for them, so the reference dangles. Two independent halves:@google/adk-devtoolsdoes not declare@types/express.AdkApiServerexposes the public memberreadonly app: express.Application(dev/src/server/adk_api_server.ts:78) and is exported fromdev/src/index.ts, sodev/dist/types/server/adk_api_server.d.tsemitsimport express from 'express'.express@4ships no bundled declarations, and@types/expresssat indev/package.json'sdevDependencies.Nothing in CI could catch this class of defect, which is why it survived. Three independent reasons, each of which the new gate has to defeat:
skipLibCheck: true. The roottsconfig.jsonsets it. With it on, an unresolvable module inside a dependency's.d.tsdoes not error — it degrades silently toany.core/node_modulesdoes not exist; every workspacedevDependencyis hoisted to the repo-rootnode_modules. Anything type-checked from a path inside the repo (npm run ts:check,npm run docs:check, thetests/integration/build_setupfixtures) walks up into that directory and resolves types a consumer never receives.file:directory installs. Thebuild_setupfixtures depend on"@google/adk": "file:../../../../core", which links the workspace directory instead of exercising the packed tarball's declared closure.It is also invisible to an
import-graph linter such aseslint-plugin-importorknip: TypeScript resolves@types/*automatically fromnode_modules/@types, so there is noimport ... from '@types/express'statement anywhere to flag. (Feat: fail the build on phantom dependencies in the published src trees (import/no-extraneous-dependencies) #323 and Feat: add a dependency-hygiene gate (npm run deps:check) to CI #250 add exactly those linters for the runtime import graph; they are complementary to this, not overlapping — see Collision check.)Solution: two commits.
fix(dev)— move@types/expressfromdevDependenciestodependenciesindev/package.json, keeping the existing^4.17.21range untouched. A package must type its own public API from its own declared closure, not from whatever a sibling workspace happens to hoist.feat(ci)— addnpm run check:published-types(scripts/check_published_types.mjs) and a newpublished-typesjob in.github/workflows/validation.yaml. The script packs every publishable workspace withnpm pack, installs the tarballs into a throwaway project, type-checks it, and fails onTS2307/TS7016anywhere in the output. Every other diagnostic is ignored, so an unrelated upstream type error inside some dependency's.d.tscannot redden the job.Three properties of the script are load-bearing, and map one-to-one onto the three reasons above. They are documented in the file header because a future refactor will otherwise "simplify" them away:
fs.mkdtempunderos.tmpdir(), never inside the reponode_modulesnpm packtarballs, notfile:linksfile:directory installs"skipLibCheck": falsein the probe tsconfiganyEach is verified by a mutation below, including a deliberate check that flipping
skipLibChecktotruemakes the gate wrongly pass.Smaller decisions worth stating so a reviewer does not have to ask:
run-testsmatrix. That matrix runs on three operating systems; this check needs one network install of the full closure and only needs proving once. A parallel job costs no extra wall clock (measured ~1m31s end to end locally). This was challenged in review as 16 lines of duplicatedCheckout/Use Node.js/npm install/npm run build; I kept the separate job and the reasoning is under Review follow-ups below. Happy to fold it in if a maintainer disagrees — it is a four-line edit either way.--legacy-peer-depson the probe install is deliberate:coredeclares five@mikro-orm/*drivers as non-optionalpeerDependencies. Which database driver a consumer picks is not part of thedependenciesclosure this gate is about.@types/nodeis treated as the probe's toolchain, not a package dependency. Published declarations do reference Node globals (Bufferincore/dist/types/skills/loader.d.ts,node:child_processindev), but every Node TypeScript consumer already installs@types/node, and pinning its major from a library is a known source of consumer conflicts. The probe installstypescriptand@types/nodeat the ranges read from the rootpackage.jsondevDependencies, so the probe's toolchain cannot drift from the repo's. (Whethercoreshould declare@types/nodeis Fix: declare @types/node so the published declarations can resolve Buffer #327's question, and is unaffected either way by this gate.)@types/adm-zip,@types/lodash-es,@types/corsand root-level@types/js-yamldeliberately stay indevDependencies. None is reachable from a public signature:adm-zipis used only inside function bodies incore/src/skills/loader.tsand only bydev/build.jsindev, lodash'scloneDeep/isEmptyare only ever called and never named in a type position,yaml.load()returnsunknown, andcorsis used as middleware only. The gate is green with all four left where they are, which is the check confirming the audit rather than my asserting it.@google/adk-devtoolsalso references@google/genaiand@opentelemetry/apifrom its public.d.tswithout declaring them at all. Those resolve transitively through@google/adk, so the gate stays green. Fixing them means adding new direct dependencies (a version-range decision, and@opentelemetry/apiis singleton-sensitive), which is a different operation from moving an existing entry — deliberately left out of this PR. Feat: add a dependency-hygiene gate (npm run deps:check) to CI #250 and Feat: fail the build on phantom dependencies in the published src trees (import/no-extraneous-dependencies) #323 both propose exactly those additions.Honest limitation, stated up front. The gate reports the verdict a consumer sees, not per-package declaration hygiene. All packages are installed side by side, so hoisting lets one workspace's declared dependency satisfy a sibling's undeclared import. Concretely: once
@google/adkdeclares@types/express(#292), reverting thedev/package.jsonhalf of this PR still passes — mutation 3 below measures exactly that. Thedevchange is therefore justified as declaration hygiene, not as something this gate enforces; the limitation is written into the script's header comment so nobody later mistakes a green run for per-package proof. I looked for a way to demonstrate thedevdefect observably under a non-hoisting install strategy and could not produce one in a reproducible environment, so I am not claiming one.Not a breaking change. Moving an entry from
devDependenciestodependenciesonly ever adds packages to a consumer's tree. Two trade-offs:@types/express@5would now also see@types/express@4nested under@google/adk-devtools. This introduces no new class of conflict:express@^4.21.2is already a hard runtime dependency of@google/adk-devtools, so that consumer already has express 4 in their tree. The alternative — apeerDependency— would force every consumer to install it by hand for a transitive implementation detail.Lockfile. The
package-lock.jsondelta is 3 lines: thedevmanifest move, plus one incidental correctionnpm installmakes unconditionally — the hoistedadm-zipentry loses a stale"dev": true, which it earns fromcorelistingadm-zipas a runtime dependency. I confirmed that line appears from a barenpm installon the untouched base branch before making any edit, so it is not avoidable when regenerating the lockfile. It is the same stale flag #183 fixes on its own. No versions change and nothing is added to or removed from the tree.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:
[ ] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
No new vitest unit test, and that is a decision rather than an omission. The new code is a CI orchestration script under
scripts/, which is outsidevitest.config.ts's coverageinclude(core/src/**,dev/src/**,integrations/src/**), so it moves no coverage threshold. More importantly the repo does not setallowJs, so a*_test.tsimportingscripts/check_published_types.mjsbreaksnpm run ts:checkwith an unresolvable-declaration error — and the only ways out of that are a suppression or anany, neither of which is acceptable. No file undercore/src,dev/srcorintegrations/srcis modified, so no existing test needed to change, and none was changed or deleted.The script's correctness is established by the mutation matrix below, which is a stronger claim than a unit test of a regex: the gate is observed failing on a broken tree and passing on a fixed one.
Mutation proof — every property of the gate, run against a mutated tree. A check that has never been observed failing is not a check.
Published type closure resolves for: @google/adk, @google/adk-devtools, @google/adk-integrations.core/package.jsonhunk (@types/express+openapi-typesback todevDependencies),npm install, re-runTS2307foropenapi-typescoreand thedevhunks,npm install, re-runTS7016forexpresscore, revert only thedevhunk,npm install, re-run"skipLibCheck": truemv core/dist/types/index.d.tsaside, re-run on the fixed treeError: core/dist/types/index.d.ts not found; run \npm run build` first.` — a forgotten build cannot masquerade as a pass, and is distinguishable from a real failureFull output of mutation 2, the worst case:
The
@google/adk-devtoolsline is thedevdefect this PR fixes, caught by the gate only becausecore's declaration is missing at the same time — mutation 3 is the same line disappearing behind hoisting.Existing suites re-run, both unchanged by this PR:
npx vitest run --project integration -t "Build setup"fails in my sandbox and I could not use it as a signal. It fails inbeforeAllwithHook timed out in 10000msatexecAsync('npm install'); I timed that same install directly at 68.8s against vitest's default 10s hook timeout, so it is the environment's slow registry, not this change — the same pre-existing flake #117, #247 and #260 target. Nothing in this diff touches those fixtures or the packages they install.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
The real proof is a genuine consumer install with no mocks, exactly what a user does:
Then the user-visible half — proving the type is real rather than
any, underskipLibCheck: true, the setting that hid the bug:Before the fix this file compiled clean, because
toA2areturnedPromise<any>for every consumer. That diagnostic is the clearest single piece of evidence the defect was real and is gone.CI status:
absent, validated locally instead. GitHub Actions triggers onpull_request: branches: [main], and this PR's base is #292's branch, so no workflow will run on it — including, ironically, thepublished-typesjob it adds. The full gate was therefore run locally on the exact pushed commit:Per repository guidance only the targeted test files above were run, not the whole suite.
Collision check. Ran
gh pr list --state open --limit 300(286 open PRs) and diffed every plausibly adjacent one: #273, #292, #327, #323, #360, #347, #250, #249, #244, #290, #277.corehalf of this work (openapi-typesand@types/expressintocore'sdependencies). Rather than ship a byte-identical duplicate of a 4-line manifest move, this PR is stacked on Fix: declare @types/express as a runtime dependency of @google/adk #292, so those two files do not appear in the diff below at all. This also matters for correctness: the new gate is red without them, so it could not land onmainalone.knip,npm run deps:check) and Feat: fail the build on phantom dependencies in the published src trees (import/no-extraneous-dependencies) #323 (import/no-extraneous-dependencies) add dependency-hygiene linters over the runtime import graph. They are complementary, not competing: neither can see a@types/*phantom, because there is no import statement for TypeScript's automatic@typesresolution to flag, and neither type-checks the published closure. Both also propose adding@google/genai/@opentelemetry/api/lodash-estodev— additions this PR deliberately stays out of. Feat: add a dependency-hygiene gate (npm run deps:check) to CI #250 does touch.github/workflows/validation.yaml(a step insiderun-tests) and rootpackage.jsonscripts; this PR adds a separate job and a separate script name, so the two merge without semantic conflict.@types/nodetocore'sdependencies. Orthogonal: the probe installs@types/nodeas toolchain either way, so the gate is green with or without it.@types/expressindev/package.json, and no open PR type-checks a packed-tarball consumer closure. That is this PR's delta.Review follow-ups. A complexity review raised six findings; five are applied in
refactor(ci): trim the published-types check to what decides the verdict, and one I did not take.Applied:
classify()collapsed tounresolvedModuleDiagnostics().mkdtempscratch root,npm packwrites exactly one.tgzper invocation, andexecFileSyncalready throws on a non-zero exit.indent()helper into its single remaining caller.npm()docstring. It claimed shell quoting as half the rationale ("cmd.exe eats^"), which is wrong —execFileSyncwithoutshell: truenever routes through cmd.exe. The real and only reason is thatnpmisnpm.cmdon Windows andexecFileSynccannot spawn it directly. The two-line ternary stays, becausenode scripts/check_published_types.mjsrun directly (withoutnpm runsettingnpm_execpath) is a real caller of the fallback.NODE_OPTIONSinvalidation.yaml. Nothing in this repo formats YAML —format:checkisprettier "**/*.ts"and lint-staged covers onlyjs,ts,json,md— so that hunk was editor churn, correctly flagged.Net −15 lines in the script.
Not applied — folding the
published-typesjob into therun-testsmatrix as a step. The finding is right that it deletes 16 lines of YAML, but I think the trade goes the other way on three grounds:windows-latestandmacos-latest, each a full network install of the published closure (measured 1m31s locally, and the slowest part is the install). It also moves that time onto the critical path, where a parallel job adds ~0 wall clock.license-check.ymlandcross-language-integration.ymlboth stand up their own job re-declaringCheckout(and, for the latter,Use Node.js+npm install). Re-declaring setup per job is inherent to GitHub Actions, not duplication this change introduces; removing it properly needs a composite action, which is more machinery, not less.The finding's stated upside — that the matrix would exercise the Windows branch of
npm()— is real but thin: that branch exists for developers running the check locally on Windows, and buying its coverage costs a full closure install on two extra runners on every PR.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.