Fix: restore event round trip in VertexAiSessionService - #565
Open
AmaadMartin wants to merge 7 commits into
Open
Fix: restore event round trip in VertexAiSessionService#565AmaadMartin wants to merge 7 commits into
AmaadMartin wants to merge 7 commits into
Conversation
added 7 commits
July 31, 2026 15:06
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.
Rebasing onto main picked up google#567, which added a structural ApiError status check to getSession in this same file. Two things went stale: - The isInvalidArgumentError docblock restated google#567's explanation of why the match is structural rather than `instanceof`. Point at that comment instead of repeating it. - The append tests hand-rolled `{name: 'ApiError', status}` literals for a class the file now imports and that the sibling getSession tests construct directly. Throw the real ApiError, so the tests exercise the shape the SDK actually produces. No behaviour change.
AmaadMartin
force-pushed
the
fix/vertex-ai-session-event-round-trip
branch
from
July 31, 2026 22:21
92e5111 to
58b2a6e
Compare
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
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
Four independent defects in
VertexAiSessionServicebreak theappendEvent()->getSession()round trip on the legacy (non-rawEvent) read path and make the write path unsafe.adk-pythonalready handles all four correctly.groundingMetadatais written but never restored.appendEventcopies it intoconfig.eventMetadata, but the legacy branch of_fromApiEventnever reads it back, so a replayed event always comes back withgroundingMetadata === undefined.appendEventcopiedevent.actionsverbatim, so the wire payload carried ADK'stransferToAgent, while the read path looks fortransferAgent(the name the API'sEventActionsdeclares, and the camelCase form of thetransfer_agentfieldadk-pythonwrites). 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.rawEventfallback caught everything. The append was wrapped in an undiscriminatedtry/catchthat retried on any throw. A transient failure (5xx, 429, timeout, socket reset) that may already have persisted the event triggered a secondappendEvent, 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 differentinv-${Date.now()}invocation id than the first attempt.partMetadatawas not stripped before append.partMetadatais a Gemini Developer API-onlyPartfield that adk-js populates for streamed/partial parts; the Agent Engine Sessions API rejects it with400 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._fromApiEventnow restoresgroundingMetadatafromeventMetadata— a direct property read, since the two@google/genaicopies in the tree declare a structurally compatibleGroundingMetadata, so no cast is needed.appendEventemits the transfer action asactions.transferAgentvia a smalltoApiEventActionsmapper; every other action field, includingrequestedToolConfirmations, keeps its name. The read path mapstransferAgentback toEvent.actions.transferToAgentand still accepts a legacytransferToAgentkey: the released write path copiedevent.actionsonto 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.rawEventretry is now gated on the service rejecting the payload itself (HTTP400), which is what an API that does not knowrawEventreturns. This is a deliberate behaviour change: transient failures now propagate unchanged instead of being quietly re-appended. The error is matched structurally onstatusrather than withinstanceof ApiError, becausecoreand@google-cloud/vertexairesolve separate@google/genaicopies andinstanceofis false at runtime. The retry also reuses the already-built request, so both attempts share the samename,author,invocationIdandtimestampand differ only inrawEvent.appendEventstripspartMetadatafrom everyPartin bothconfig.contentandconfig.rawEvent.content, building a copy so the caller'seventis never mutated (partialCopysharescontentby reference, so stripping in place would corrupt the caller's event).Compatibility: new writes emit
actions.transferAgentand omitpartMetadata, aligning adk-js withadk-pythonand with the API's declared schema. Old data keeps working because the read path accepts both action key names.Testing Plan
Unit Tests:
Added to
core/test/sessions/vertex_ai_session_service_test.ts, reusing the existing mockSessionsfixture and thecreateEvent/createEventActions/createSessionfactories (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 capturedconfig.actions, which pins both the rename and the absence of atransferToAgentkey, and guardsrequestedToolConfirmations, 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.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:
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/agentEngineIdpointed at a live reasoning engine:createSession, thenappendEventwith an event that carriesactions.transferToAgent, agroundingMetadatapayload, and a part withpartMetadataset.partMetadatapart fails with400 INVALID_ARGUMENT(Unknown name "part_metadata").getSessionand confirm the event comes back withactions.transferToAgentandgroundingMetadataintact, and with the part'stextpreserved (partMetadatais intentionally not persisted, matchingadk-python).Checklist
Additional context
The service is marked
@experimental, so the wire-format corrections (emittingactions.transferAgent, omittingpartMetadata) are in scope. The read path stays backward compatible with sessions written by earlier adk-js versions.