fix(runner): persist events returned by onEventCallback

[fallintoplace]

Summary

Run onEventCallback before persisting non-partial agent events and store the resolved callback output.

Previously, callers received the replacement event while session history retained the original event. This could make reloaded history differ from the streamed response and feed the original content into later invocations.

The persistence decision still uses the original event partial flag, so a plugin cannot change whether an agent-produced event is stored.

Tests

• Added regression coverage that reloads the session and verifies the callback replacement was persisted
• npx vitest run --project unit:core
• Prettier and ESLint checks for changed files
• Workspace build

Fixes #573

[Varun-S10]

Hi @fallintoplace, We verified your PR and confirmed that it fixes the issue. However, when we ran the unit tests (core/test/runner/runner_test.ts), we noticed that the test should respect abort signal after onEventCallback is failing.

Could you please check this failing test and update your PR so all tests pass?

[AmaadMartin]

The reorder is right and matches adk-python: Runner._exec_with_plugin in runners.py runs run_on_event_callback before append_event, persists the resolved event, and gates on the original event.partial — exactly what this diff does. I checked every onEventCallback implementation in the repo (LoggingPlugin returns undefined, AgentEventCapturePlugin returns the same object) and worked through the untouched should respect abort signal after onEventCallback test, which still passes only because its override returns undefined — so nothing regresses, but nothing covers the replacement path either. The one real gap: adk-js swaps in the plugin's event wholesale where Python merges it onto the original, so id/author/branch/actions now leak into session history. Details inline.

[AmaadMartin]

Not a nit. modifiedEvent ?? event now decides what goes into session history, so a plugin replacement replaces the original event's identity and actions, not just its content.

const outputEvent = modifiedEvent ?? event;

adk-python does this same reorder but does not swap the object. Runner._get_output_event merges the plugin's fields onto the original and explicitly preserves identity:

for field_name in modified_event.model_fields_set:
  if field_name in {'id', 'invocation_id', 'timestamp'}:
    continue
  update[field_name] = modified_event.__dict__[field_name]
output_event = original_event.model_copy(update=update)
if not output_event.author:
  output_event.author = original_event.author

That matters here because the persisted event feeds machinery keyed on fields a plugin has no reason to reproduce:

• author — determineAgentForResumption (core/src/runner/runner.ts:603-608) scans session.events backwards, skips events with no author, then does rootAgent.findAgent(event.author).
• branch — core/src/agents/processors/content_processor_utils.ts:76-77 drops history events whose branch isn't a prefix of the current branch when assembling the next LLM request.
• actions.stateDelta — BaseSessionService.appendEvent applies it through updateSessionState, so a replacement without it silently drops the original event's state writes.
• id — appendEvent dedupes via session.events.findIndex((e) => e.id === event.id).

createEvent() mints a fresh id, defaults invocationId to '' and actions to an empty object, so the archetypal "build a redacted event and return it" plugin loses all of the above. Before this PR that was caller-visible only; now it is the session history.

A minimal identity-preserving form:

const outputEvent = modifiedEvent
  ? {
      ...modifiedEvent,
      id: event.id,
      invocationId: event.invocationId,
      timestamp: event.timestamp,
      author: modifiedEvent.author || event.author,
      branch: modifiedEvent.branch ?? event.branch,
    }
  : event;

Caveat on my own suggestion: this does not recover actions. Python relies on model_fields_set to know which fields the plugin actually set; TS object literals have no equivalent, and a createEvent()-built replacement always carries a defined-but-empty actions, so the spread would still clobber the original's stateDelta. Full parity probably needs a documented contract on BasePlugin.onEventCallback ("return a copy of the event you were handed"), which is bigger than this fix. Fixing just the identity fields here, plus a line in the callback docs, seems like the right scope.

[AmaadMartin]

Nit, optional. The gate reads the original event but the call persists the replacement, and the session services re-check partial on whatever they are handed.

if (!event.partial) {
  await this.sessionService.appendEvent({
    session,
    event: outputEvent,
  });
}

BaseSessionService.appendEvent opens with if (event.partial) { return event; } (core/src/sessions/base_session_service.ts:168-170), and DatabaseSessionService.appendEvent has the same guard. So the effective condition is !event.partial && !outputEvent.partial: a plugin returning a partial: true replacement for a non-partial event has that event silently dropped from history — which contradicts "a plugin cannot change whether an agent-produced event is stored" in the PR description.

VertexAiSessionService.appendEvent has no partial guard of its own (it awaits super.appendEvent and then unconditionally calls this.sessions.events.append), so with that service the same replacement is written remotely. The outcome depends on which session service is configured.

Python gates on the original event.partial too, so this is upstream-consistent and I'm not asking you to change the behavior — just worth a sentence in the description or a comment here, so the next reader doesn't read the two flags as interchangeable.

[AmaadMartin]

Nit, optional. The ordering is now load-bearing — the ordering is the fix — but this comment still describes only what the callback does, so a future cleanup could hoist the appendEvent back above it and silently undo the PR. Python spells the reason out at the same spot:

# Step 3: Run the on_event callbacks before persisting so callback
# changes are stored in the session and match the streamed event.

Worth copying that second clause.

[AmaadMartin]

Nit. The new assertion checks only the text, which is the one field the replacement is guaranteed to carry.

const persistedEvent = session!.events[1];
expect(persistedEvent.content!.parts![0].text).toEqual(
  MockPlugin.ON_EVENT_CALLBACK_MSG,
);

MockPlugin.onEventCallback builds its replacement with createEvent({invocationId: '', author: '', ...}) (runner_test.ts:95-106), so this test now writes an event with an empty author and empty invocationId into session history — and passes. That is precisely the input determineAgentForResumption skips over, and nothing here notices. One more line:

expect(persistedEvent.author).toEqual('test_agent');

fails today and would pin whichever answer you land on for the identity question in my other comment. (I'd leave invocationId out of the assertion for now — asserting it against events[0].invocationId would be circular, since the yielded event is the same replacement.)

Separately, I traced the untouched should respect abort signal after onEventCallback test (runner_test.ts:677) to check this PR doesn't quietly break it: it still passes, but only because that override returns undefined, so outputEvent === event and session!.events[1].author stays 'test_agent'. It is not exercising the replacement path either.

[AmaadMartin]

Re-checked at ed10236b. All four are addressed, and the identity fix is the one that mattered — outputEvent now pins id, invocationId, timestamp, author and branch from the original, so a createEvent()-built replacement can no longer land in session history with an empty author and a fresh id. The persisted and yielded event are the same object, which is the point of the reorder.

The new expect(persistedEvent.author).toEqual('test_agent') is a real regression test: MockPlugin builds its replacement with author: '', so that line fails without the spread above it. And the actions caveat I couldn't solve in code is now handled where it belongs — BasePlugin.onEventCallback's doc tells plugin authors to copy params.event so unmodified fields survive.

The partial asymmetry is unchanged, which is correct: it matches Python, and I'm not asking for a behaviour change. CI green on all three platforms after a re-run (the earlier Windows red was the repo's timeout flake in integration/e2e suites, not this diff). LGTM.