Skip to content

chore: cache npm downloads in the cross-language workflow - #306

Open
AmaadMartin wants to merge 8 commits into
mainfrom
fix/cross-language-workflow-npm-cache
Open

chore: cache npm downloads in the cross-language workflow#306
AmaadMartin wants to merge 8 commits into
mainfrom
fix/cross-language-workflow-npm-cache

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 30, 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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Problem: The run-tests job in .github/workflows/cross-language-integration.yml invokes actions/setup-node@v6 with no with: block at all, so the cache input is unset. With cache unset the action performs neither a cache restore before the job nor a cache save in its post step, and the job's npm install therefore does a full cold resolve plus a full tarball download of the entire workspace dependency tree on every push to main and every pull request — on macos-latest, the most expensive runner class in use here.

setup-node's automatic caching cannot rescue this. Its package-manager-cache input defaults to true, but auto-caching only engages when package.json declares packageManager or devEngines.packageManager. Verified against this checkout: grep -rn --include=package.json -E '"(packageManager|devEngines|engines)"' . across the root and every workspace (core/, dev/, integrations/) returns zero matches, so the auto-detect path never fires. Corroborating the state of the repo today, grep -rn "cache" .github/ also returned zero matches — no workflow here caches anything.

This is a CI-efficiency defect, not a correctness defect: the job produces the right answer, just slowly and with avoidable registry traffic.

Solution: Set the cache input explicitly. Two added lines, one file, zero deletions:

      - name: Use Node.js
        uses: actions/setup-node@v6
        with:
          cache: npm

Why this shape, and what was deliberately not added:

  • No cache-dependency-path. This is an npm-workspaces repo ("workspaces": ["core", "dev", "integrations"]), so there is exactly one lockfile — package-lock.json at the repository root (find . -maxdepth 3 -name package-lock.json confirms no nested lockfiles). setup-node scans the workspace root for it automatically, making the input redundant configuration.
  • No node-version / node-version-file. There is no .nvmrc, no .node-version, and no engines field anywhere in this repo, so node-version-file would hard-fail the step. Pinning the Node version is a separate decision and is deliberately out of scope for this PR.
  • No hand-rolled actions/cache step and no restore-keys fallbacks. setup-node's built-in handling is the whole point of the change.
  • npm install is left as-is (not "improved" to npm ci), and the Go setup, build, and test steps are untouched. Same triggers, same runner, same pass/fail semantics.

Cache key shape produced by setup-node (node-cache-${RUNNER_OS}-${os.arch()}-${packageManager}-${hashFiles(lockfile)}), which on this job resolves to:

node-cache-macOS-<arch>-npm-<sha256 of package-lock.json>

Because the job runs on macos-latest only, exactly one cache entry is ever produced by this workflow — negligible against the repository's Actions cache budget. There are no npm restore-keys, so a lockfile change rotates to a clean miss rather than restoring a stale partial cache.

Scope / collision check: Before implementing, I listed all open PRs on this fork (gh pr list --state open --limit 100) and diffed the file lists of every plausibly adjacent CI change (#296, #250, #245, #237, #235, #204, #207). #296 (ci: cache npm downloads in the validation workflow) makes the analogous change in .github/workflows/validation.yaml; the remaining CI PRs also only touch validation.yaml. No open PR touches cross-language-integration.yml, so there is no collision and nothing to stack on. This PR deliberately does not touch validation.yaml — that would conflict with #296.

Release impact: none. The commit and this PR are titled chore: on purpose — this repo runs release-please ("release-type": "node"), and a feat:/fix: title on a CI-only change would open a release PR and bump every workspace version for no product change. chore: is not a release-triggering conventional-commit type.

Revert: delete the two added lines.

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 unit test was added, deliberately. The deliverable is two lines of GitHub Actions YAML; there is no executable unit under test, and a test asserting on the contents of a workflow file would be pure churn. No application code is added or changed, so no JS test suite exercises this diff. The verification burden is carried by static checks plus the live CI run.

Static checks run locally on the pushed commit:

  1. YAML parses
    $ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/cross-language-integration.yml')); print('yaml ok')"
    yaml ok
    
  2. The key is set exactly where intended, and only there
    $ python3 -c "import yaml; s=yaml.safe_load(open('.github/workflows/cross-language-integration.yml'))['jobs']['run-tests']['steps'][1]; print(s)"
    {'name': 'Use Node.js', 'uses': 'actions/setup-node@v6', 'with': {'cache': 'npm'}}
    
    with s['uses'] == 'actions/setup-node@v6', s['with']['cache'] == 'npm' and set(s['with']) == {'cache'} all asserting cleanly.
  3. Proof the check can fail (negative control). The same assertion run against the unfixed file from main (git show main:.github/workflows/cross-language-integration.yml) fails, so the check has real signal rather than passing vacuously:
    {'name': 'Use Node.js', 'uses': 'actions/setup-node@v6'}
        assert s['with']['cache'] == 'npm'
               ~^^^^^^^^
    KeyError: 'with'
    
  4. The diff is minimal
    $ git diff --stat main
     .github/workflows/cross-language-integration.yml | 2 ++
     1 file changed, 2 insertions(+)
    
    git diff --name-only main lists only that one file.
  5. No forbidden inputs crept in
    $ grep -nE "node-version|cache-dependency-path" .github/workflows/cross-language-integration.yml
    (no output)
    
  6. validation.yaml is untouched — it appears in neither git status nor the diff.
  7. No lockfile churn. npm install --dry-run --ignore-scripts resolved cleanly against the unchanged lockfile (added 1163 packages) and git status --porcelain afterwards showed only the workflow file. (--ignore-scripts was needed only because this sandbox has no node_modules, so the repo's husky prepare script is not installed locally; it is unrelated to this change.)

Manual End-to-End (E2E) Tests:

The live CI run on this PR is the end-to-end test — the change only has observable behaviour on a GitHub-hosted runner. Reproduce it with:

  1. gh pr checks <pr-number> --watch --fail-fast, and confirm the Cross-Language Tests workflow's run-tests job passes. (The validation workflow also defines a job named run-tests, so two similarly named checks appear — identify them by workflow name.)
  2. gh run view <run-id> --log on the first run: the Use Node.js step should report a miss and the post step should report a save.
  3. gh run rerun <run-id> without touching package-lock.json, then read the re-run log: Use Node.js should report a restore from the same key, and the post step should decline to re-save. A re-run of the same PR (rather than a second PR) is required because a pull_request-triggered cache is scoped to that PR's merge ref.

Observed results on this PR (macos-latest, arm64, Node v24.18.0 / npm 11.16.0 as provisioned by the runner). Both attempts of the Cross-Language Tests run-tests job concluded success:

Attempt 1 (cold) Attempt 2 (re-run, lockfile unchanged)
Use Node.js cache line npm cache is not found Cache restored from key: node-cache-macOS-arm64-npm-56fcdc8f564da454e69438f0c13d07bf1025685b0519cecdc64092109b985331
Post Use Node.js Cache saved with the key: node-cache-macOS-arm64-npm-56fcdc8f564da454e69438f0c13d07bf1025685b0519cecdc64092109b985331 (~85 MB / 88,878,655 B) Cache hit occurred on the primary key node-cache-macOS-arm64-npm-56fcdc8f…b985331, not saving cache.
Use Node.js step duration 1s 4s (restore + extract of the 85 MB archive)
Install dependencies added 1089 packages, and audited 1093 packages in 16s added 1089 packages, and audited 1093 packages in 18s

The restore/save cycle works exactly as designed, and the key is stable across the two attempts.

Honest caveat on the timings: this sample shows no wall-clock win. npm install self-reported 16s cold vs 18s warm, and the Use Node.js step itself grew by ~3s to restore and extract the 85 MB archive — so the cached run was marginally slower end to end, not faster. I am reporting the measurement rather than the expectation. Two things to note when weighing it: a single before/after pair on a hosted runner is dominated by runner-to-runner variance at this scale (a ~16s step), and the durable benefit of the cache is that ~85 MB of tarballs no longer come from the registry on every push and PR — which is registry-traffic and registry-availability insurance, not a speed claim. If a reviewer considers avoided registry traffic insufficient justification for the added restore step on this particular job, that is a reasonable reason to decline this PR; I would rather surface that than assert a speedup the logs do not support.

Two warnings appear in the run annotations. Both are pre-existing and unrelated to this change — they are emitted by the Setup Go step, are present on unmodified main, and do not fail the job: Restore cache failed: Dependencies file is not found in … Supported file pattern: go.sum (actions/setup-go's own default caching, which finds no root go.sum), and the actions/setup-go@v5 Node 20 deprecation notice. Fixing either is out of scope here.

The validation workflow was unaffected, as intended: its run-tests legs passed on ubuntu-latest, macos-latest and windows-latest.

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.
[ ] 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.

AmaadMartin and others added 8 commits July 28, 2026 14:48
* docs: document minimum supported Node.js version in README

Add a short prerequisite note under the Installation section stating that
ADK for TypeScript requires Node.js 18 or newer, so new users know which
Node.js runtime they need before running npm install @google/adk.

The version reflects the mandated fallback: no engines.node field is
declared in any package.json in the repo.

* docs: reference current Node.js LTS instead of a fixed version

Node.js 18 is EOL and any hard-coded minimum version goes stale over time.
Reword the installation prerequisite to point readers at the current Node.js
LTS releases, which stays accurate without future edits.

Addresses PR review feedback on #526.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
…#536)

`ToolAuthHandler` accepted an `authCredential` and then never read it. When
no auth response was present it went straight to `requestCredential()`, so a
credential handed to `OpenAPIToolset`/`RestApiTool` at construction time was
ignored and the tool returned `{pending: true}` on every call. For `apiKey`,
`http` and `serviceAccount` schemes nothing ever resolves that request — no
user interaction is involved — so the tool could never complete.

Fall back to the configured credential when there is no auth response, which
mirrors `_get_auth_response() or self.auth_credential` in adk-python.

Also narrow what gets written to session state. The credential store exists
to avoid repeating work that either cannot be repeated (an auth response is
readable once) or is expensive (an exchange costs a round trip). A static
credential that needed no exchange is neither, so it is no longer persisted —
that would only copy the developer's secret into the session store.
…hon parity (#542)

* Feat: add LoadMcpResourceTool and MCPToolset resource access

Port adk-python's LoadMcpResourceTool to adk-js for cross-language parity.

- Add listResources/getResourceInfo/readResource to MCPToolset, following
  the existing create -> try -> closeSession-in-finally session idiom.
- Add LoadMcpResourceTool (mirrors the in-repo LoadArtifactsTool idiom):
  declares load_mcp_resource({resource_names}), and processLlmRequest injects
  resolved resource contents (text + base64 binary, no decode step) into the
  LlmRequest.
- Export the tool from core/src/index.ts (@google/adk public API).

* test: cover LoadMcpResourceTool and MCPToolset resource access

Add full unit coverage (100% line + branch of the new code):

- load_mcp_resource_tool_test.ts: init, declaration, runAsync (incl. default),
  list injection (incl. empty + swallowed list errors), text/binary/unknown
  content, base64 blob passthrough + default mime type, swallowed read errors,
  and all no-op guard paths (non-matching/absent function response, missing
  parts).
- mcp_toolset_test.ts: listResources/getResourceInfo/readResource happy paths
  and error paths (unknown name, missing URI), plus session-cleanup assertions
  for success and failure (closeSession in finally, no leaked sessions).

* test(e2e): exercise LoadMcpResourceTool against a real MCP server

Add a no-mock end-to-end test that spawns a real MCP server over stdio
(mcp_resource_server.mjs, exposing a text and a binary resource) and drives
the real MCPToolset + LoadMcpResourceTool: listing/resolving/reading resources
and injecting their contents (text + base64 binary) into an LlmRequest.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
* feat(tools): add ExampleTool for few-shot examples

Port adk-python's ExampleTool to adk-js. The tool accepts a static
Example[] or a BaseExampleProvider and, on each outgoing LLM request,
appends a few-shot <EXAMPLES> block (built via buildExampleSi from the
latest user query) to the system instruction. It is never declared to
the model (mirrors PreloadMemoryTool) and is a no-op when no user text
is present. Exported from the public @google/adk API.

* test(tools): cover ExampleTool unit and end-to-end paths

Add Vitest coverage for ExampleTool: static list and provider paths,
model-style passthrough, no-op branches (missing user content, empty
parts, text-less first part), runAsync throwing, and the public export.
Includes an end-to-end block that drives processLlmRequest through a
real Context/InvocationContext (no mocks). 100% line/branch coverage of
the new tool.

* refactor(tools): apply simplicity audit feedback

Use a constructor parameter property for `examples` (repo convention),
and drop the redundant provider end-to-end test whose only unique aspect
was a spy — keeping the no-mock e2e block strictly mock-free. The
provider selection path stays fully covered by the unit tests; the tool
retains 100% line/branch coverage.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
* feat(agents): support clone() for RoutedAgent

RoutedAgent derives its routing targets from config.agents rather than
subAgents, so the inherited BaseAgent.clone() rebuilt the agent from the
already-parented originals and threw "already has a parent agent".

Add a RoutedAgent.clone() override that deep-clones the routing targets
(via a private cloneRoutingTargets helper) and passes them through the
agents override, so super.clone() rebuilds the constructor with fresh,
detached copies that are re-parented onto the clone. The array-vs-record
shape and record keys are preserved so the clone routes identically, and
parent-override rejection plus the detached-root guarantee are still
enforced by the base implementation.

Remove the now-obsolete "documented limitation" test (and its unused
RoutedAgent import) from base_agent_test; positive coverage lives in
routed_agent_test.

* test(agents): cover RoutedAgent.clone()

Add a clone describe suite exercising the new override and the
cloneRoutingTargets helper: array and record forms, deep-clone and
re-parenting of targets, originals left untouched, functional routing on
the clone (record form), verbatim agents override, non-agents overrides,
and parentAgent-override rejection. Includes a no-mock end-to-end case
that clones a RoutedAgent whose targets are real LlmAgents.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
* Feat(tools): add SSRF-safe load_web_page tool for adk-python parity

Ports the adk-python load_web_page tool to adk-js. Fetches a URL and
returns its extracted, readable text, hardened against SSRF:

- only http/https schemes are fetched
- localhost-style hostnames and hosts resolving to non-global IPs
  (private, loopback, link-local, shared/CGNAT, reserved, multicast,
  IPv4-mapped IPv6) are rejected before any connection
- redirects are never followed (redirect: 'manual')
- a configurable timeout (default 30s) bounds every request
- expected failures return the parity string "Failed to fetch url: <url>"
  instead of throwing

Exposes loadWebPage(), the LOAD_WEB_PAGE FunctionTool, and the
LoadWebPageOptions type via the @google/adk public API.

* Refactor(tools): inline single-use failure prefix in load_web_page

Addresses simplicity-audit feedback: the FAILURE_PREFIX constant had a
single caller, so its literal is inlined into failedToFetchMessage, which
remains the sole formatter of the parity failure string.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
…n parity) (#525)

* feat(tools): add EnterpriseWebSearchTool for Gemini web grounding

Ports adk-python's EnterpriseWebSearchTool to adk-js, closing a
cross-language parity gap. The tool is a Gemini 2+ built-in grounding
source that appends {enterpriseWebSearch: {}} to the outgoing LlmRequest
config; it performs no client-side execution. Mirrors the
google_maps_grounding_tool idiom (extracted applyEnterpriseWebSearch
function + ADK_DISABLE_GEMINI_MODEL_ID_CHECK escape hatch). Exported
from the public API via common.ts.

* test(tools): add unit tests for EnterpriseWebSearchTool

Covers every branch of applyEnterpriseWebSearch (100% line/branch):
model-unset guard, Gemini 2+ (plain + path form), config
initialization, Gemini 1.x with/without other tools, non-Gemini
rejection, and the ADK_DISABLE_GEMINI_MODEL_ID_CHECK escape hatch, plus
the runAsync no-op and the exported singleton. Mock-free; imports via
the @google/adk public entry point.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
The Cross-Language Tests job invokes actions/setup-node with no inputs, so
npm's cache directory is never restored or saved and every run on the
macos-latest runner does a full cold resolve and download of the workspace
dependency tree.

setup-node's automatic caching cannot engage here: it only turns itself on
when package.json declares packageManager or devEngines.packageManager, and
this repo declares neither. Setting cache: npm explicitly is therefore
required.

The root package-lock.json is auto-discovered by setup-node, so
cache-dependency-path is unnecessary.
This was referenced Jul 30, 2026
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.

2 participants