Skip to content

Fix: round-trip EventActions losslessly through VertexAiSessionService - #503

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/vertex-session-event-actions-round-trip
Open

Fix: round-trip EventActions losslessly through VertexAiSessionService#503
AmaadMartin wants to merge 4 commits into
mainfrom
fix/vertex-session-event-actions-round-trip

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 2, 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):
    No public issue is associated with this report; it was found while reading the
    Vertex AI session persistence code against its adk-python counterpart.

  2. Or, if no issue exists, describe the change:
    Problem: VertexAiSessionService translated EventActions between ADK and
    the Agent Engine Sessions API with a different hand-written field list on each
    side, which made the translation asymmetric and lossy.

  3. Asymmetric field name (a real bug). appendEvent copied event.actions
    onto the request verbatim, so the transfer target went out on the wire as
    transferToAgent. The read path looks for the API's spelling instead
    (actions['transferAgent'], vertex_ai_session_service.ts:557 before this
    change). adk-python writes 'transfer_agent' and renames it back on read
    (src/google/adk/sessions/vertex_ai_session_service.py,
    append_event / _from_api_event); adk-js implemented only the read half,
    so a transferToAgent value could not survive a save + load.

  4. Hand-maintained read allowlist (a latent trap). _fromApiEvent rebuilt
    EventActions field by field. The list happened to cover every field of
    today's EventActions, but nothing forces anyone to extend it, so the next
    field added to EventActions would be silently dropped on reload.

Scope of impact (stated precisely). appendEvent also stores the whole
event under config.rawEvent, and _fromApiEvent returns early from rawEvent
when it is present, so neither defect is observable while that channel works.
They bite on the fallback the service itself implements and expects to hit:
appendEvent's catch retries the append without rawEvent, and sessions
written before rawEvent support existed still take the legacy path. So: a real
bug on a real code path, not a bug on every write.

Solution: stop enumerating fields. Both directions now copy the whole
actions object through one helper that renames a single key
(transferToAgent <-> transferAgent),
skipping null/undefined values and passing every other key through
untouched. The read path builds its result with the existing
createEventActions() so the four container defaults keep living in one place.
Adding a field to EventActions now requires no change to
vertex_ai_session_service.ts for it to persist.

Notes on the design:

  • Cross-language parity rule applied: parity wins for anything observable
    across the boundary, so the wire name is transferAgent (matching
    adk-python and the API's documented field), while the local ADK model keeps
    its transferToAgent name. Compaction precedence also mirrors adk-python:
    customMetadata._compaction overrides any compaction arriving inside
    actions.
  • Backward compatible on read: because unknown keys now pass through
    untouched, an event written by today's adk-js (with transferToAgent on the
    wire) still deserializes to actions.transferToAgent. Nothing that reads
    correctly today reads incorrectly after this change. A test pins this.
  • No case converter. The Vertex TS SDK already speaks camelCase, and a
    recursive converter would rewrite user-controlled keys inside stateDelta,
    artifactDelta and customMetadata. The only translation needed is the one
    semantic field rename.
  • No new dependency, no new public API, no visibility widened: the two
    converters are module-private and the tests drive them through the public
    appendEvent / getSession with an injected Sessions client.
  • Out of scope on purpose: adk-python's write path has the mirror-image
    6-key allowlist. It is queued separately and is not touched here.
  • The rawEvent retry catch in appendEvent (lines 467-480) is the only
    uncovered region left in the file. It is pre-existing, untouched by this
    change, and is the subject of a separate change; the legacy read path it
    produces is covered here by serving events with rawEvent stripped.
  • tests/integration/sessions/vertex_ai_session_service_test.ts builds a third
    Sessions client the same way the two existing suites in that file do. That
    construction trips a known duplicate-@google/genai type mismatch (private
    customBaseUrl), which already occurs twice in that file on main. CI does
    not typecheck the test tree (validation.yaml runs build / tests / lint /
    format / docs), and the root cause is package deduplication, not this change,
    so no suppression or cast was added to hide it.

Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 (402 open PRs) plus gh pr diff --name-only on every plausibly adjacent
PR. Seven open PRs touch
core/src/sessions/vertex_ai_session_service.ts (#270, #474, #287, #201, #331,
#303, #227), but none touches the actions conversion: grepping their diffs for
transferAgent|transferToAgent|_fromApiEvent|partialCopy|renameAction returns
one unrelated hit (#303 retypes the eventMetadata partialCopy). No PR lands
this change, so this branches from main rather than stacking.

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.

Eight new cases in core/test/sessions/vertex_ai_session_service_test.ts
(EventActions round trip), all additive — no existing case was edited,
weakened, skipped or deleted, and all 57 pre-existing cases still pass
unmodified (including the exact-match assertion on the append payload).

npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts
  -> Test Files 1 passed, Tests 65 passed (57 pre-existing + 8 new)
npx vitest run --project integration tests/integration/sessions/vertex_ai_session_service_test.ts
  -> Test Files 1 passed, Tests 5 passed (3 pre-existing + 2 new)
npx tsc --noEmit    # no new errors in the changed files (see note above)
npx eslint core/src/sessions/vertex_ai_session_service.ts core/test/sessions/vertex_ai_session_service_test.ts tests/integration/sessions/vertex_ai_session_service_test.ts
  -> clean
npx prettier --check <the three changed files>   -> clean
npm run build   -> success

Coverage of core/src/sessions/vertex_ai_session_service.ts from the targeted
unit run: 96.84% lines / 90.29% branches for the whole file; every line and
branch added by this change is covered. The lcov report shows the only
uncovered lines are 207 and 467-480, both pre-existing and untouched.

Proof the new tests can fail. Each was run against mutated source and
observed to FAIL (source restored with git checkout after each):

Mutation Failing test(s) Message
toApiActions renames transferToAgent -> transferToAgent (no write rename) unit writes transferToAgent under the API transferAgent field; integration sends the actions under the API field names expected { stateDelta: {}, …(4) } to match object { transferAgent: 'agent-b' }
fromApiActions renames transferAgent -> transferAgent (no read rename) unit reads the API transferAgent field back as transferToAgent, round-trips every action field expected undefined to be 'agent-b'
fromApiActions renames in the write direction unit reads the API transferAgent field back..., round-trips..., still reads actions written with the legacy transferToAgent name expected undefined to be 'agent-b'
delete if (value == null) continue; unit drops null action values instead of clobbering the defaults expected null to be undefined
delete the if (compactionData) override unit prefers the customMetadata compaction over one inside actions expected { startTime: 1, endTime: 2, …(1) } to deeply equal { startTime: 1600000000000, …(2) }
force rawEvent to undefined (drop the early return) unit restores actions from rawEvent and ignores the typed actions expected 'stale-agent' to be 'agent-b'
config.actions = toApiActions(event.actions ?? createEventActions()) unit sends no actions when the event has none and the pre-existing handles event without actions in appendEvent expected { stateDelta: {}, …(3) } to be undefined
filter the read result through a hand-written 7-key allowlist unit round-trips every action field through the legacy channel; integration recovers the actions from the legacy channel on read expected { stateDelta: { counter: 1 }, …(6) } to deeply equal { stateDelta: { counter: 1 }, …(7) }

Note the mutation table also shows why both a write-name test and a round-trip
test are needed: with the write rename removed, the round-trip test still passes
(the read path's pass-through carries the wrong key back), so only the
assertions on the request payload catch a wrong wire name.

Manual End-to-End (E2E) Tests:
No manual E2E against a live Agent Engine: the defect only manifests on the
rawEvent-less fallback channel, which cannot be triggered on demand from
outside the service. The integration suite is the substitute — it stands up a
loopback http.Server behind the real @google-cloud/vertexai Sessions
client and the real @google/genai ApiClient, records the actual append
request body, serves that body back from the session get + events list
endpoints with rawEvent stripped, and asserts the actions survive
appendEvent -> getSession. That is the only test that can catch a wrong wire
name, because a mock-object test uses the same key on both sides.

To run it yourself:

npm install && npm run build
npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts
npx vitest run --project integration tests/integration/sessions/vertex_ai_session_service_test.ts

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.


CI note. The first run-tests (windows-latest) run failed in
core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout (Test timed out in 5000ms) — a Windows-only
shell-exec flake in a file this PR does not touch (unrelated PR #502 fails on
windows-latest too, with different tests). It passed on rerun; all six checks
are green.

Amaad Martin added 4 commits August 1, 2026 23:06
The Agent Engine Sessions API spells ADK's transferToAgent as
transferAgent. appendEvent copied event.actions onto the request verbatim,
so the field went out under a name the read path never looks for and the
transfer target was lost on the legacy (no rawEvent) channel.

Both directions now copy the whole actions object through one rename map
instead of a hand-maintained field list, so a field added to EventActions
persists without another change here.
Review feedback: two single-entry inverse rename tables and a keyMap
lookup generalised over one field. Pass the two names as arguments
instead, drop the no-op ExtendedEventActions cast and the defensive
copy of an argument the helper never mutates, and let the helper take
the optional API actions directly.
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.

1 participant