Skip to content

Fix: keep temp: state readable for the whole invocation in DatabaseSessionService (stacked on #132) - #607

Open
AmaadMartin wants to merge 4 commits into
fix/apply-temp-state-statedeltafrom
fix/temp-state-visible-within-invocation
Open

Fix: keep temp: state readable for the whole invocation in DatabaseSessionService (stacked on #132)#607
AmaadMartin wants to merge 4 commits into
fix/apply-temp-state-statedeltafrom
fix/temp-state-visible-within-invocation

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 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):
    Related: Output key doesn't work with temp state scope google/adk-python#4564
    Related: fix: temp-scoped state now visible to subsequent agents in same invocation google/adk-python#4618

  2. Or, if no issue exists, describe the change:

Stacked PR. Base branch is fix/apply-temp-state-statedelta (#132), not main.
Collision check (gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, 506 open PRs
scanned, then gh pr diff --name-only on every session/state-adjacent PR: #132, #376, #466,
#477, #492, #503) found that #132 already lands the base-class half of this fix — the
exported applyTempState helper and its call in BaseSessionService.appendEvent. Rather than
ship a competing implementation, this PR stacks on #132 and contributes only the delta it does
not cover. #466 (Firestore) and #477 (listSessions contract) touch the same files but neither
touches temp: state application.

Problem (what #132 leaves unfixed):

  1. DatabaseSessionService.appendEvent rebuilds session.state from persisted rows and
    reassigns it (session.state = newMergedState). Persisted rows never hold temp: keys, so
    that assignment wipes every temp: key already on the in-memory session. Fix: Apply temp: state from runAsync stateDelta during the invocation (#260) #132 re-applies the
    current event's temp delta after the merge, which fixes a single append but not the
    invocation:
    • a temp: key written by event 1 is gone after an unrelated event 2 is appended;
    • a temp: key written by toolContext.state.set('temp:x', …) (which mutates
      session.state directly) is gone after any append;
    • the stale-session reload branch replaces session.state too, with the same effect.
  2. To apply temp state after the merge, Fix: Apply temp: state from runAsync stateDelta during the invocation (#260) #132 has to keep an untrimmed copy of the event, so it
    trims a cloneDeep(event) and returns the clone. That diverges from BaseSessionService and
    from adk-python: the caller's event.actions.stateDelta keeps its temp: keys, and the object
    in session.events is no longer the object the caller passed in.
  3. BaseSessionService.updateSessionState still skips State.TEMP_PREFIX. Once the delta is
    trimmed first that branch is unreachable, and adk-python removed the equivalent skip in
    _update_session_state.
  4. Nothing pins the actual user-visible symptom from Output key doesn't work with temp state scope google/adk-python#4564 (a SequentialAgent
    whose first agent has outputKey: 'temp:out' and whose second interpolates {temp:out}), and
    nothing exercises the event.partial short circuit that guards the new ordering.

Solution:

  • core/src/sessions/database_session_service.ts — call applyTempState(session, event) before
    the trim (the ordering BaseSessionService and adk-python's append_event both use), snapshot
    the temp:-prefixed entries of session.state before the transaction, and restore them with
    session.state = {...newMergedState, ...tempState}. Snapshotting before the transaction is what
    makes the State.set path and the stale-session reload path work: both replace session.state
    inside the transaction. Because temp is applied before the trim, the cloneDeep is no longer
    needed — the event is trimmed in place and returned, as on every other backend.
  • core/src/sessions/base_session_service.ts — delete the now-dead TEMP_PREFIX skip in
    updateSessionState. Behaviour-neutral: appendEvent trims the delta before calling it, so
    the branch could never fire. Verified by re-adding the skip and re-running the suites — 185/185
    still passed, which is exactly why there is no test for this hunk. Both updateSessionState and
    applyTempState now iterate Object.entries(event.actions?.stateDelta ?? {}) instead of guarding
    with a two-line if (!event.actions || !event.actions.stateDelta) return;. That is not cosmetic:
    Event.actions and EventActions.stateDelta are both required types, so the old guard was only
    reachable by casting, and the cast is what the applyTempState no-op test used to do. The
    optional chain keeps the runtime tolerance an existing test proves is needed (see below) without
    a branch the type system says cannot happen.

Nothing new is persisted. trimTempDeltaState still runs before any write to storage, and the
temp:-free getSession result is asserted in every new backend test.

Parity note (which side wins): adk-python wins on observable semantics — ordering
(_apply_temp_state_trim_temp_delta_state_update_session_state), the in-place trim, and
returning the caller's event. The session.state reassignment in the JS database backend is a
local structural difference that this PR deliberately preserves (see
src/google/adk/sessions/database_session_service.py:741-750, which mutates session.state via
super().append_event and therefore needs no snapshot); only its temp-dropping side effect is
fixed.

API surface: unchanged by this PR. applyTempState is exported by #132 (and is public through
core/src/common.ts's export * from './sessions/base_session_service.js'); this PR adds no new
export and does not touch core/src/index.ts. A future Firestore session service (#466) inherits
the fixed behaviour provided it either calls super.appendEvent or calls applyTempState itself
— and, if it reassigns session.state the way the database backend does, it needs the same
snapshot.

InMemorySessionService and VertexAiSessionService need no source change: both delegate to
super.appendEvent, which trims the delta in place before the storage-side append sees it. Pinned
by tests rather than asserted in a comment.

Suppressions: net −7. This PR removes 8 as unknown as casts and adds 1. Removed: the two in
the applyTempState no-op test (which existed only to fabricate an Event without actions, an
Event the type system forbids — the test now feeds a reachable empty stateDelta and asserts the
same thing); the redundant as unknown as Session on the new Vertex fixture, whose literal already
supplies all six Session fields; and five copies of
(service as unknown as {orm: MikroORM}).orm, replaced by a single module-level
ormOf(service) helper used at all five call sites. Also reverted a no-op event
trimmedEvent rename in the database transaction tail: trimTempDeltaState mutates and returns
the same object, so the rename changed nothing at runtime while implying an untrimmed event
still existed.

Two review suggestions were not taken, with reasons:

  • Collapse updateSessionState to Object.assign(session.state, event.actions?.stateDelta ?? {}).
    Not equivalent: Object.assign(undefined, {}) throws where the copy loop was a no-op, and five
    pre-existing VertexAiSessionService appendEvent tests pass Session fixtures with no state
    field. Applying it fails all five with Cannot convert undefined or null to object. Taking it
    would mean editing five existing tests in the same change that touches the code they cover, to
    buy two lines and a narrower runtime contract. The nullish-coalescing rewrite above captures the
    line saving with zero behavioural delta.
  • Also drop the five pre-existing as unknown as Session casts in
    vertex_ai_session_service_test.ts (L936/988/1025/1057/1082) as a "one-line-each freebie".

    Those five are load-bearing, not redundant: every one of those literals omits state (and two
    also omit lastUpdateTime), so removing the cast is a compile error. Only the new L1108 literal
    is complete, and that is the one this PR fixes.

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 (no existing test was weakened, skipped or deleted; the two edits are the cast removals
described above, which keep every assertion):

  • core/test/sessions/database_session_service_test.ts (5 new cases)
    • trims the caller event in place and returns that same event;
    • keeps temp: state from an earlier event readable when a later event is appended;
    • keeps temp: state written straight onto session.state (the State.set path) across an append;
    • leaves a partial event untouched (no state applied, delta not trimmed, no event appended);
    • keeps temp: state when the append reloads a stale session.
  • core/test/sessions/in_memory_session_service_test.ts (1 new case) — partial event short circuit.
  • core/test/runner/temp_state_visibility_test.ts (new file, 1 case) — Runner +
    SequentialAgent + two LlmAgents over InMemorySessionService with a local BaseLlm double:
    agent 1 writes outputKey: 'temp:out', agent 2's instruction interpolates {temp:out}. Asserts
    agent 2's captured system instruction contains agent 1's output, and that getSession afterwards
    returns state with no temp:-prefixed key. It is a unit test driving fakes, so it lives in
    core/test/, not tests/e2e/.

CI is absent for this PR, by construction. .github/workflows/validation.yaml is gated on
pull_request: branches: [main], so its run-tests job never fires for a stacked PR whose base is
fix/apply-temp-state-statedelta; only auto-assign runs, and that is not validation. Everything
below was therefore run locally on the exact pushed commit
(e2a00d25b319731af60f7de46f755d344ccf3154).

Commands run (on the pushed commit):

npx vitest run --project unit:core core/test/sessions/ core/test/runner/ core/test/agents/ \
  core/test/tools/agent_tool_test.ts core/test/context/   # 41 files, 480 tests passed
npm run build                                             # exit 0
npm run lint                                              # exit 0
npx prettier --check <the six touched files>              # clean
npx tsc --noEmit                                          # 293 pre-existing errors, identical
                                                          # count before and after this diff, none
                                                          # in the files it touches

Proving the tests can fail. Each new test was run against the unfixed code:

mutation failing test(s) message
revert database_session_service.ts to the #132 version all 4 new non-partial DB cases expected undefined to be 'from-agent-1', expected undefined to be 'written-by-a-tool', and (for the in-place case) expected {…} to be {…} // Object.is equality
delete applyTempState(session, event) from BaseSessionService.appendEvent runner regression Error: Context variable not found: `temp:out`. — thrown from core/src/agents/instructions.ts:54, i.e. the exact symptom of google/adk-python#4564
delete the if (event.partial) return event; guard from both services both partial cases expected { 'temp:k1': 'v1', sk: 'v2' } to not have property "temp:k1"
re-add the TEMP_PREFIX skip to updateSessionState none (185/185 pass) confirms that hunk is behaviour-neutral dead-code removal
drop the ?? {} fallback in applyTempState (i.e. event.actions.stateDelta) pre-existing handles event without actions in appendEvent Cannot read properties of undefined (reading 'stateDelta') — proves the optional chain is load-bearing, not decoration

Coverage. Every line and branch added by this PR is covered. Measured with
--coverage.include limited to the two changed source files over
core/test/sessions/ core/test/runner/: the uncovered statements/branches that remain
(base_session_service.ts 130-142, 182; database_session_service.ts 75-96, 240-241, 305-343,
410-430) are all pre-existing code this PR does not touch — getOrCreateSession, URI parsing, and
the event-dedup replace branch. The event.partial guards (previously uncovered in both services)
are now covered by the two new partial cases, and both sides of the new event.actions?.stateDelta
chain are covered (the absent-actions side by the pre-existing Vertex test above).

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

Reproduces google/adk-python#4564 against a real model:

const runner = new InMemoryRunner({
  agent: new SequentialAgent({
    name: 'seq',
    subAgents: [
      new LlmAgent({name: 'a1', model: '<model>', outputKey: 'temp:out'}),
      new LlmAgent({
        name: 'a2',
        model: '<model>',
        instruction: 'Rewrite: {temp:out}',
      }),
    ],
  }),
});

Run one turn and confirm (a) a2 does not throw Context variable not found: `temp:out`,
(b) a2's prompt contains a1's output, and (c) getSession(...) afterwards returns a session
whose state has no temp: key. Repeat with
new DatabaseSessionService({dbName: 'sqlite://./tmp.db'}) and confirm the temp: value is still
readable after a second agent appends its own event.

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.

Amaad Martin added 4 commits August 3, 2026 19:41
…ice appends

DatabaseSessionService.appendEvent rebuilds session.state from persisted
rows and reassigns it, which dropped every temp: key already on the
in-memory session - including ones written earlier in the same
invocation via State.set. Capture the temp entries before the
transaction (so the stale-session reload path is covered too) and merge
them back over the rebuilt state.

Applying temp state before the trim also removes the need to clone the
event: the caller's event is trimmed in place and returned, matching
BaseSessionService and adk-python.
The event delta is trimmed of temp: keys before updateSessionState runs,
so the skip can never fire. Removing it mirrors adk-python's
_update_session_state. Behaviour is unchanged; the added test pins the
partial-event short circuit that guards the new ordering.
Reproduces the user-visible symptom in TypeScript: a first LlmAgent with
outputKey 'temp:out' followed by a second whose instruction interpolates
{temp:out}. Without the fix the second agent throws
"Context variable not found: `temp:out`".
…RM cast

Complexity review follow-ups:
- revert the no-op event -> trimmedEvent rename in the transaction tail;
  trimTempDeltaState mutates and returns the same object, so the rename
  only added churn.
- replace the two-line `!actions || !stateDelta` guards in
  applyTempState and updateSessionState with
  `event.actions?.stateDelta ?? {}`. This drops the branch the tests had
  to cast their way into while keeping the runtime tolerance the
  VertexAiSessionService 'event without actions' path needs.
- feed the applyTempState no-op test a reachable empty stateDelta
  instead of casting away the required actions field.
- drop the redundant Session cast on the new Vertex fixture and hoist
  the private-ORM cast in the database tests into one `ormOf` helper
  used by all five call sites.
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