Skip to content

Refactor: extract a shared extractStateDelta helper and adopt it in InMemorySessionService (stacked on #466) - #609

Open
AmaadMartin wants to merge 6 commits into
feat/firestore-session-servicefrom
feat/shared-state-delta-helper
Open

Refactor: extract a shared extractStateDelta helper and adopt it in InMemorySessionService (stacked on #466)#609
AmaadMartin wants to merge 6 commits into
feat/firestore-session-servicefrom
feat/shared-state-delta-helper

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):
    N/A

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

Problem: Three session backends independently reimplement the same rule for
splitting a state map into app: / user: / session buckets while dropping
temp: keys. adk-python already has this exactly once, as
sessions/_session_util.extract_state_delta.

Solution: A pure, behaviour-preserving refactor that leaves the rule in one
place and has every backend delegate to it.

Stacked on #466 (feat/firestore-session-service), not branched from
main — see the collision note below. Base branch is
feat/firestore-session-service.

Collision check (required, and it found something)

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 \
  --json number,title,headRefName
gh pr diff <n> --repo AmaadMartin/adk-js --name-only   # every sessions-adjacent PR

Screened every open PR whose title or branch mentioned state/delta/session or
extract/helper/dedup/split/refactor, then diffed the eight that touch
core/src/sessions/: #607, #492, #477, #466, #386, #376, #293, #132.

What this PR adds on top of #466

  1. Moves the splitter into its own modulecore/src/sessions/state_utils.ts
    — out of base_session_service.ts, and renames it extractStateDelta to
    match the adk-python reference (_session_util.extract_state_delta), which
    is the shared implementation this work converges on. The return type is now a
    named StateDeltas interface instead of an inline object type.
  2. Adopts it at the third call site, InMemorySessionService.appendEvent
    the one backend Feat: port FirestoreSessionService from adk-python to @google/adk-integrations #466 leaves on a hand-rolled loop. This is the only
    behavioural code path this PR touches that Feat: port FirestoreSessionService from adk-python to @google/adk-integrations #466 does not.
  3. Makes the export deliberate. In Feat: port FirestoreSessionService from adk-python to @google/adk-integrations #466 the helper is published to
    @google/adk consumers only as a side effect of common.ts doing
    export * from './sessions/base_session_service.js'. Since
    @google/adk-integrations genuinely imports it across the package boundary,
    this PR keeps it public but declares it explicitly:
    export {extractStateDelta} / export type {StateDeltas}.

Everything else is import churn: database_session_service.ts (2 sites) and
firestore_session_service.ts (2 sites) switch to the new name/module.

Notes on the in-memory call site (the one non-mechanical edit)

The in-memory loop is not a verbatim instance of the pattern, so two
properties are preserved explicitly rather than by construction:

  • The session bucket is intentionally unused here. Session state is
    applied by super.appendEvent()BaseSessionService.updateSessionState,
    which writes every non-temp: key with its prefix intact. Wiring the
    helper's session bucket in would strip app:/user: keys out of
    session.state and change behaviour. Only {app, user} are destructured, and
    a comment at the call site says why.
  • Lazy allocation is preserved via Object.keys(...).length > 0 guards, so
    appending an event with no scoped keys still leaves this.appState[appName]
    and this.userState[appName][userId] unallocated. Disclosure: this is not
    observable through the public API — both readers pass the value to
    mergeStates(), which defaults undefined and {} identically — so no test
    can distinguish guard-present from guard-absent without reaching into private
    fields, which this repo forbids. The guards are kept because dropping them
    would allocate an empty object per app and per user on every appendEvent,
    which the original code avoided. Both guards are still exercised in both
    directions by the existing updates app state / updates user state tests
    (each has one delta empty and the other non-empty).

Parity vs. local convention

Local TS convention wins on module layout and naming style; parity wins on
observable behaviour. The bucket keys (app/user/session), the precedence
order (app:user:temp: → session), and temp:-dropping all match
adk-python exactly. Verified field-by-field against
src/google/adk/sessions/_session_util.py.

Deviation from the task spec, stated explicitly

The spec said do not export the helper from common.ts, on the reasoning that
no out-of-package consumer existed and that a follow-up "will add the export
when an out-of-package consumer actually exists". On this stacked base that
consumer does exist (FirestoreSessionService in integrations), so the
export is added now, explicitly, exactly as the spec anticipated. Nothing else
in the spec was reduced or skipped.

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.

New core/test/sessions/state_utils_test.ts (13 cases) ports the adk-python
TestExtractStateDelta cases one-for-one and adds the TypeScript-specific ones:
empty map, undefined, each bucket in isolation, temp: dropping, the mixed
map, purity against a frozen input, a prefix-only key ({'app:': 1}
{app: {'': 1}}), two repeated-prefix cases, and value-carried-by-reference.
The four cases from #466's base_session_service_test.ts moved here with the
code
rather than being deleted, and the move is verbatim — every fixture
literal and assertion is unchanged, modulo the splitStateDelta
extractStateDelta rename. Two are renamed only to disambiguate newly-added
neighbours (strips only the leading prefix...when a prefix repeats, which
now sits beside a new ...when another prefix follows it); their inputs and
assertions are identical. base_session_service_test.ts had no other content,
so the file is removed. The move can be checked mechanically:

git show fork/feat/firestore-session-service:core/test/sessions/base_session_service_test.ts \
  | sed -n '11,25p' > /tmp/orig_case.txt
sed -n '55,68p' core/test/sessions/state_utils_test.ts \
  | sed 's/extractStateDelta/splitStateDelta/' > /tmp/new_case.txt
diff /tmp/orig_case.txt /tmp/new_case.txt   # differs only by a trailing blank line

Three cases added to core/test/sessions/in_memory_session_service_test.ts for
the newly-refactored call site: app state accumulated across successive events,
user state accumulated across successive events, and an exact toEqual on the
state getSession returns (the existing cases only use non-exhaustive
toHaveProperty, so nothing pinned the absence of spurious keys).

npx vitest run --project unit:core core/test/sessions/state_utils_test.ts \
  core/test/sessions/in_memory_session_service_test.ts \
  core/test/sessions/database_session_service_test.ts     # 73 passed
npx vitest run --project unit:integrations                # 65 passed

The 26 DatabaseSessionService and 31 pre-existing InMemorySessionService
cases, and all 65 integrations cases, pass unchanged — the signal that this
refactor is behaviour-preserving. No existing test was edited, skipped, or
weakened.

Coverage. state_utils.ts is at 100% statements / branches / functions /
lines:

npx vitest run --project unit:core core/test/sessions/state_utils_test.ts \
  --coverage --coverage.include='core/src/sessions/state_utils.ts'
 state_utils.ts |     100 |      100 |     100 |     100 |

Proof the tests can fail (coverage is not proof). Each mutation was applied
to the source, the suite re-run, and the source restored:

# Mutation Result
1 else if (!key.startsWith(State.TEMP_PREFIX))else 2 failed — drops a temporary key from every bucket: expected { Object (app, user, ...) } to deeply equal { app: {}, user: {}, session: {} }
2 app[key.slice(State.APP_PREFIX.length)]app[key] 7 failed — routes an app-prefixed key…: expected { app: { 'app:theme': 'dark' }, …(2) } to deeply equal { app: { theme: 'dark' }, …(2) }
3 user[key.slice(State.USER_PREFIX.length)]slice(State.APP_PREFIX.length) 3 failed — routes a user-prefixed key…: expected { app: {}, …(2) } to deeply equal { app: {}, user: { lang: 'en' }, …(1) }
4 in-memory {...this.appState[appName], ...appDelta}{...appDelta} 1 failed — accumulates app state across successive events: expected { 'app:second': 'b' } to match object { 'app:first': 'a', 'app:second': 'b' }
5 in-memory user spread drops the existing state 1 failed — accumulates user state across successive events: expected { 'user:second': 'b' } to match object { 'user:first': 'a', …(1) }
6 in-memory {app: appDelta, user: userDelta} → buckets swapped 7 failed, incl. exposes exactly the prefixed and plain keys on the retrieved session: expected { 'app:appKey': 'appValue', …(4) } to deeply equal { 'app:appKey': 'appValue', …(2) }

Manual End-to-End (E2E) Tests:
No new E2E test. This is an internal refactor with no I/O, no new public
behaviour and no cross-process boundary; the DatabaseSessionService suite
already drives the real MikroORM/SQLite path (:memory:) end to end through
both converted call sites, and the FirestoreSessionService suite drives the
third. To reproduce locally:

npm ci
npm run build
npx vitest run --project unit:core core/test/sessions/state_utils_test.ts \
  core/test/sessions/in_memory_session_service_test.ts \
  core/test/sessions/database_session_service_test.ts
npx vitest run --project unit:integrations

CI status: absent. This PR's base is feat/firestore-session-service, not
main, so the pull_request: branches: [main] workflows never trigger. It was
validated locally instead, on the exact commit pushed:

  • npm run build — clean (also proves the new extractStateDelta export
    resolves for the integrations package, which imports it from @google/adk).
  • npx vitest run --project unit:core <3 files> — 73 passed.
  • npx vitest run --project unit:integrations — 65 passed.
  • npx eslint <all 7 touched files> — clean, no suppressions added anywhere in
    the diff (no any, @ts-expect-error, eslint-disable, or coverage-ignore).
  • npx prettier --check <all 7 touched files> — clean.
  • npm run docs:check (typedoc --treatWarningsAsErrors) — clean, so the new
    public export is fully documented.
  • npm run ts:check — reports no error in any file this PR touches. The
    repo has pre-existing repo-wide ts:check failures in unrelated files (Fix: make ts:check green over a stated slice and gate CI on it #370
    tracks making it green); this PR neither adds to them nor depends on them.

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 6 commits August 3, 2026 20:08
Relocate the app:/user:/session splitter out of base_session_service.ts
into its own sessions/state_utils.ts module and rename it
extractStateDelta, matching adk-python's
sessions/_session_util.extract_state_delta.

Because @google/adk-integrations consumes it across the package
boundary, export it explicitly from common.ts rather than letting it
ride on the wholesale 'export *' of base_session_service.js.

Its unit tests move with it, from base_session_service_test.ts to
state_utils_test.ts, and gain the edge cases the adk-python suite pins.
…endEvent

Replace the hand-rolled per-key app:/user: routing loop with the shared
helper. The session bucket is deliberately unused here: session state is
applied by super.appendEvent(), which keeps the prefixes on the keys.

Lazy allocation of the appState/userState entries is preserved, so
appending an event that carries no scoped keys still leaves those maps
untouched.

Adds appendEvent coverage for state accumulated across successive events
and for the session-state contract the helper must not disturb.
The assertion is an exact toEqual on the state getSession returns, not on
the stored session state, so say so.
appendEvent calls super.appendEvent() twice, so pointing at one of them
by position is misleading.
Three carry-over defects from moving the helper out of
base_session_service.ts:

- The parameter was widened from 'Record<string, unknown> | undefined'
  to an optional one. No caller omits it, so the '?' only made a
  meaningless no-arg call legal on a newly published symbol.
- The '{@link mergeStates}' cross-reference was downgraded to an inert
  code span. TypeDoc resolves it across modules and docs:check runs with
  --treatWarningsAsErrors, so the link is safe to keep.
- Three fixture literals in the migrated 'routes each key by prefix'
  case were rewritten in transit, obscuring that the move preserved it.
The argument can no longer be omitted, so 'absent' described a call the
signature rejects.
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