Skip to content

Fix: restore event round trip in VertexAiSessionService - #220

Closed
AmaadMartin wants to merge 12 commits into
mainfrom
fix/vertex-ai-session-event-round-trip
Closed

Fix: restore event round trip in VertexAiSessionService#220
AmaadMartin wants to merge 12 commits into
mainfrom
fix/vertex-ai-session-event-round-trip

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):
    Closes: #issue_number
    Related: #issue_number

  2. Or, if no issue exists, describe the change:
    Problem: Four independent defects in VertexAiSessionService break the appendEvent() -> getSession() round trip on the legacy (non-rawEvent) read path and make the write path unsafe. adk-python already handles all four correctly.

  3. groundingMetadata is written but never restored. appendEvent copies it into config.eventMetadata, but _fromApiEvent's legacy branch never reads it back, so a replayed event always comes back with groundingMetadata === undefined.

  4. Write/read naming asymmetry on the agent-transfer action. appendEvent copied event.actions verbatim, so the wire payload carried ADK's transferToAgent, while the read path looks for transferAgent (the name the API's EventActions declares, and the camelCase form of the transfer_agent field adk-python writes). The two names never met, so an agent transfer written by adk-js was silently dropped when the session was replayed through the legacy path.

  5. The rawEvent fallback caught everything. The append was wrapped in an undiscriminated try/catch that retried on any throw. A transient failure (5xx, 429, timeout, socket reset) that may already have persisted the event triggered a second appendEvent, duplicating it in the session, and callers saw the failure of the retry rather than of the first attempt. It also rebuilt the request, so the retry could carry a different inv-${Date.now()} invocation id than the first attempt.

  6. partMetadata was not stripped before append. partMetadata is a Gemini Developer API-only Part field that adk-js populates for streamed/partial parts; the Agent Engine Sessions API rejects it with 400 INVALID_ARGUMENT (Unknown name "part_metadata" at 'event.content.parts[0]').

Solution: All four fixes live in core/src/sessions/vertex_ai_session_service.ts; the service is @experimental, so the wire-format corrections are in scope. No new dependency, no new export, no signature change.

  • _fromApiEvent now restores groundingMetadata from eventMetadata — a direct property read, since the two @google/genai copies in the tree declare a structurally compatible GroundingMetadata, so no cast is needed.
  • appendEvent emits the transfer action as actions.transferAgent via a small toApiEventActions mapper; every other action field, including requestedToolConfirmations, keeps its name. The read path maps transferAgent back to Event.actions.transferToAgent and still accepts a legacy transferToAgent key: the released write path copied event.actions onto the request verbatim and the SDK converter forwards that object unchanged, so already-stored sessions can carry the ADK key and dropping the fallback would silently lose their transfers.
  • The rawEvent retry is now gated on the service rejecting the payload itself (HTTP 400), which is what an API that does not know rawEvent returns. This is a deliberate behaviour change: transient failures now propagate unchanged instead of being quietly re-appended. The error is matched structurally on status rather than with instanceof ApiError, because core and @google-cloud/vertexai resolve separate @google/genai copies and instanceof is false at runtime. The retry also reuses the already-built request, so both attempts share the same name, author, invocationId and timestamp and differ only in rawEvent.
  • appendEvent strips partMetadata from every Part in both config.content and config.rawEvent.content, building a copy so the caller's event is never mutated (partialCopy shares content by reference, so stripping in place would corrupt the caller's event).

Compatibility: new writes emit actions.transferAgent and omit partMetadata, aligning adk-js with adk-python and with the API's declared schema. Old data keeps working because the read path accepts both action key names.

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.

Added to core/test/sessions/vertex_ai_session_service_test.ts, reusing the existing mock Sessions fixture and the createEvent / createEventActions / createSession factories (the append mock is typed with the SDK's request-parameter type, so the captured payload is checked by the compiler rather than cast):

  • sends transferToAgent as transferAgent and keeps every other action — asserts the entire captured config.actions, which pins both the rename and the absence of a transferToAgent key, and guards requestedToolConfirmations, which the SDK type omits so the compiler cannot catch its loss.
  • strips partMetadata from content and rawEvent without mutating the event, handles content without parts, handles an event without content.
  • retries without rawEvent when the API rejects it with 400 — records what each attempt actually carried and asserts the retry reuses the same request object (which is what keeps the invocation id and timestamp identical; the previous code minted a fresh one).
  • rethrows a server error / a network error without re-appending — the regression guard for the duplicate append, asserting exactly one call.
  • restores groundingMetadata from eventMetadata, restores transferToAgent from actions.transferAgent, restores transferToAgent from a legacy transferToAgent key.

Commands run locally on the pushed commit:

  • npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts — 61 passed.
  • Coverage of the changed file (--coverage.include='core/src/sessions/vertex_ai_session_service.ts'): every new line and branch is executed; the only uncovered branches in the file are pre-existing ones this change does not touch.
  • npm run build, npm run lint, npm run format:check — all pass.

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

The Agent Engine Sessions backend cannot be exercised in CI, so this needs a real Vertex AI project with an Agent Engine. With projectId / location / agentEngineId pointed at a live reasoning engine:

  1. createSession, then appendEvent with an event that carries actions.transferToAgent, a groundingMetadata payload, and a part with partMetadata set.
  2. Confirm the append succeeds — before this change the partMetadata part fails with 400 INVALID_ARGUMENT (Unknown name "part_metadata").
  3. getSession and confirm the event comes back with actions.transferToAgent and groundingMetadata intact, and with the part's text preserved (partMetadata is intentionally not persisted, matching adk-python).

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.

AmaadMartin and others added 12 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>
The Agent Engine Sessions legacy (rawEvent-less) read path lost data and
the write path was unsafe:

- groundingMetadata was written to eventMetadata but never restored, so
  replayed events always came back without it.
- appendEvent sent ADK's `transferToAgent` while the API (and the read
  path) use `transferAgent`, so agent transfers were silently dropped.
- the rawEvent fallback retried on any error, so a transient 5xx that may
  already have persisted the event appended it a second time and masked
  the original failure. It now retries only on 400/INVALID_ARGUMENT, and
  reuses the same request params so both attempts share an invocation id.
- partMetadata (a Gemini Developer API-only Part field) was sent as-is and
  is rejected by the Sessions API with 400 INVALID_ARGUMENT; it is now
  stripped from a copy of the content, leaving the caller's event intact.

The read path accepts a legacy `transferToAgent` key so sessions written
by earlier adk-js versions keep their transfers.
Adds unit tests for the four round-trip fixes: the transferAgent wire
name (and that the other action fields are untouched), partMetadata
stripping without mutating the caller's event, the narrowed rawEvent
fallback (retries on 400/INVALID_ARGUMENT, rethrows 5xx and network
errors exactly once), and restoring groundingMetadata plus both the new
and legacy transfer keys on read.

Also adds append -> legacy-read round-trip tests, which rebuild the
API's SessionEvent from the captured request without rawEvent to prove
the fixes end to end, and types the append mock so the captured request
is checked against the SDK's parameter type.
- Drop the local ApiEventActions extension: the SDK's EventActions is a
  fine return type, since TypeScript does not excess-property-check the
  spread that carries requestedToolConfirmations. The JSDoc now records
  that the field rides along.
- Match only the error shape the SDK can actually produce. The Sessions
  client is HTTP-only and reports failures as ApiError { status }, so the
  gRPC INVALID_ARGUMENT and numeric `code` checks were unreachable.
- Drop tests that could not fail: assertions comparing the reused request
  object with itself, a read of an absent field with no branch behind it,
  a transfer-key precedence case no writer can produce, and two round
  trips already pinned by exact wire-level assertions.
Second review pass: keep a single append -> legacy-read guard for the
transfer action (the defect that a one-sided test cannot catch) and drop
the separate round-trip suite, assert content without parts by value
rather than by reference, and drop the now-meaningless `Base` suffix on
the SDK EventActions alias.
Third review pass: fold the transferAgent rename assertion into the test
that pins the whole actions payload, fold the non-mutation assertion into
the partMetadata stripping test, inline the single-use legacy-event
builder, and trim the fallback docblock to the rationale that is not
already in the code.

Keeps the legacy `transferToAgent` read fallback: the released write path
copies `event.actions` verbatim and the SDK converter forwards that object
unchanged, so stored sessions can carry the ADK key, and dropping the
fallback would silently lose their transfers.
Fourth review pass: both halves of the transfer round trip are already
pinned to the literal wire key -- the append test asserts the whole
actions payload and the read test feeds `transferAgent` through
`_fromApiEvent` -- so the composition of the two could not fail on its
own.
@AmaadMartin
AmaadMartin force-pushed the fix/vertex-ai-session-event-round-trip branch from 63836f5 to 92e5111 Compare July 29, 2026 18:05
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Automated: ported to upstream as google#565.

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