fix(session): restore compatibility with google-adk 1.22.0 through 2.8.0 - #156
Merged
Conversation
google-adk 2.4.0 renamed the table-creation hook from _prepare_tables() to the public prepare_tables(), added is_postgresql to StorageSession.to_session()/get_update_timestamp(), and switched append_event() to apply state deltas by mutating the loaded dict in place. Our encrypted models rejected the new keyword on every CRUD call, our table override was never invoked, and in-place deltas were silently dropped because the encrypted state columns lacked MutableDict change tracking. google-adk 1.22.0 through 1.25.x separately read a private _dialect_name property our model never defined. - Accept is_postgresql on to_session() and get_update_timestamp(); decide naive-vs-aware on tzinfo like upstream - Override prepare_tables() and keep _prepare_tables() as a delegating alias so one class serves both hook names - Wrap the three state columns in MutableDict.as_mutable so in-place delta application is persisted - Add _dialect_name property for google-adk 1.22.0 through 1.25.x - Add sentinels: table hook present and overridden, upstream method parameters are a subset of ours, in-place mutation persists - Ignore the upstream BaseAgentConfig DeprecationWarning emitted at import by google-adk 2.x Test: uv run pytest; verified against google-adk 1.22.0, 1.26.0, 2.0.0, 2.8.0
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
4 tasks
…utable listeners Review follow-up. Three review agents found two silent correctness gaps and one resource leak in the first cut of the ADK 2.x compatibility fix. MutableDict.as_mutable() installs a process-global mapper listener per call that is never garbage collected. Our type instance is created per service, so every EncryptedSessionService leaked three listeners that retained its EncryptedJSON and therefore the backend's key material, and the shared type instance also made event_data mutation-tracked, which upstream never does. Attribute-level MutableDict.associate_with_attribute on the three state attributes tracks in-place deltas with no global state. Event timestamps were stored as naive local time on every ADK version, but google-adk 2.7.0 switched its storage and its after_timestamp filter to naive UTC. On non-UTC hosts get_session() silently dropped events west of UTC and leaked them east of it. from_event/to_event now follow the installed release's convention and prefer the exact epoch preserved in event_data, as upstream does. get_update_timestamp treated every naive value as UTC, but google-adk < 2.4.0 wrote naive local time for non-SQLite dialects. It now decides per upstream convention: UTC for SQLite (by flag or detected from the bound engine, as 1.22.0 through 1.25.x did), for is_postgresql, or on >= 2.4.0; local otherwise. - Replace MutableDict.as_mutable with associate_with_attribute; event_data stays a plain dict - Store event timestamps as naive UTC on google-adk >= 2.7.0, naive local before; read the epoch back from event_data first - Derive the SQLite flag from the bound engine when no dialect flag is given - Tests: bound _dialect_name, prepare_tables alias idempotency with exact table set, ciphertext-at-rest on the in-place path, zero global listener growth, event_data untracked, event round-trip and after_timestamp filtering under America/New_York and Asia/Tokyo, variadic-safe signature sentinel extended to StorageEvent Test: uv run pytest; 316 passed on google-adk 1.22.0, 1.26.0, 2.0.0, 2.4.0, 2.7.1, 2.8.0 with TZ set to America/New_York or Asia/Tokyo
There was a problem hiding this comment.
🟡 Changes recommended
The new timezone fixtures use monkeypatch.undo() (can undo unrelated patches and break teardown ordering) and the added DeprecationWarning filter is overly broad unless scoped to google.adk.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Restores compatibility across google-adk 1.22.0–2.8.0 by aligning encrypted schema hooks, method signatures, timestamp conventions, and SQLAlchemy mutation tracking with upstream behavioral changes, plus adding regression/sentinel coverage to catch future ADK drift.
Changes:
- Add version-aware handling for event timestamps and naive
update_timeinterpretation, plus accept new upstream dialect keywords (is_postgresql) and legacy_dialect_name. - Rename/bridge the ADK table-creation hook by implementing
prepare_tables()and keeping_prepare_tables()as a delegating alias. - Add targeted unit + integration tests to detect signature drift, validate hook overrides, and ensure in-place state mutations persist.
File summaries
| File | Description |
|---|---|
| src/adk_secure_sessions/services/models.py | Adds dialect/UTC conventions, _dialect_name, is_postgresql signature parity, and event timestamp round-trip logic. |
| src/adk_secure_sessions/services/encrypted_session.py | Implements prepare_tables() and keeps _prepare_tables() as a compatibility alias for ADK < 2.4.0. |
| tests/unit/test_models.py | Adds unit coverage for is_postgresql, naive/aware timestamp conventions, _dialect_name, and mutation-tracking invariants. |
| tests/unit/test_encrypted_session_service.py | Adds sentinel tests ensuring the upstream hook is overridden and table creation is idempotent/uses encrypted schema. |
| tests/integration/test_state_mutation_tracking.py | New integration coverage ensuring in-place state dict updates persist and remain encrypted at rest. |
| tests/integration/test_event_timestamp_timezone.py | New integration coverage for after_timestamp filtering under forced non-UTC process timezones. |
| tests/integration/test_adk_conformance.py | Adds signature-parity sentinel tests against upstream schema classes to prevent keyword TypeError regressions. |
| pyproject.toml | Adds a warning filter to ignore upstream ADK DeprecationWarning noise during tests. |
| docs/adr/ADR-007-architecture-migration.md | Updates ADR to reflect the hook rename (prepare_tables) and compatibility approach. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+41
to
+53
| @pytest.fixture(params=["America/New_York", "Asia/Tokyo"]) | ||
| def process_timezone(request, monkeypatch) -> Iterator[str]: | ||
| """Run the test under a fixed non-UTC process timezone, west and east. | ||
|
|
||
| A wrong convention loses events west of UTC and leaks them east of it, | ||
| so both directions are exercised. | ||
| """ | ||
| monkeypatch.setenv("TZ", request.param) | ||
| time.tzset() | ||
| yield request.param | ||
| monkeypatch.undo() | ||
| time.tzset() | ||
|
|
Owner
Author
There was a problem hiding this comment.
Addressed in latest push — the fixture now saves and restores TZ itself in try/finally, re-runs tzset() in both directions, and skips when time.tzset is unavailable. No more monkeypatch.undo().
Comment on lines
+33
to
+45
| @pytest.fixture | ||
| def new_york_tz(monkeypatch): | ||
| """Run the test with the process timezone set to America/New_York. | ||
|
|
||
| Makes naive-local and naive-UTC datetimes differ by hours, so any code | ||
| that confuses the two produces a visibly wrong value instead of passing | ||
| by coincidence on UTC hosts such as CI runners. | ||
| """ | ||
| monkeypatch.setenv("TZ", "America/New_York") | ||
| time.tzset() | ||
| yield | ||
| monkeypatch.undo() | ||
| time.tzset() |
Owner
Author
There was a problem hiding this comment.
Addressed in latest push — same treatment as the integration fixture: direct os.environ save/restore in try/finally, tzset() on both sides, skip without time.tzset.
Comment on lines
+162
to
+165
| # google-adk >= 2.x emits a DeprecationWarning at import time when its own | ||
| # LlmAgentConfig subclasses the deprecated BaseAgentConfig. Upstream | ||
| # module-level code, not our defect. | ||
| "ignore:BaseAgentConfig is deprecated:DeprecationWarning", |
Owner
Author
There was a problem hiding this comment.
Keeping as-is — the warning is emitted by typing_extensions.deprecated from __init_subclass__ and is attributed to <frozen abc>, not to any google.adk module, so a module qualifier never matches and collection errors again (verified at runtime). The message prefix is unique to that one upstream decorator call, so the filter cannot hide an unrelated deprecation.
Comment on lines
+1
to
+13
| """Integration tests for in-place state mutation tracking. | ||
|
|
||
| google-adk 2.4.0+ applies state deltas by mutating the loaded ORM row's | ||
| ``state`` dict in place (``storage_session.state.update(delta)``) rather than | ||
| reassigning the attribute. SQLAlchemy only detects that if the column type is | ||
| wrapped in ``MutableDict.as_mutable``. These tests exercise that contract | ||
| directly against our encrypted models, independent of which ADK version is | ||
| installed, so a regression shows up on every matrix cell. | ||
|
|
||
| See Also: | ||
| [`adk_secure_sessions.services.models.create_encrypted_models`][adk_secure_sessions.services.models.create_encrypted_models]: | ||
| Where the ``MutableDict`` wrapping lives. | ||
| """ |
Owner
Author
There was a problem hiding this comment.
Addressed in latest push — docstring now describes MutableDict.associate_with_attribute, which is what the implementation uses.
…ion-tracking docstring Copilot review follow-up. The timezone fixtures used monkeypatch.undo(), which reverts every patch the test body made and relies on undo ordering to re-run tzset after TZ is restored. They now save and restore TZ themselves in try/finally, call tzset in both directions, and skip where tzset is unavailable. The mutation-tracking test docstring still named MutableDict.as_mutable; the implementation uses associate_with_attribute.
4 tasks
Alberto-Codes
added a commit
that referenced
this pull request
Sep 3, 2026
…with uv.lock The lockfile had not moved since March: google-adk sat at 1.26.0 while PyPI shipped 2.8.0, and `uv-secure` was failing the CI lint job on stale starlette, urllib3, and sqlparse advisories (this was the red `lint` check on #156). Upgrading surfaced a packaging defect: google-adk 2.x moved SQLAlchemy and aiosqlite behind its optional `db` extra, so this package imported `sqlalchemy` without declaring it and would fail at import for anyone installing against ADK 2.x. `aiosqlite` stayed a core ADK dependency but is declared too, since it is the driver behind our default URL. The pre-commit line also pinned ruff, uv, and uv-secure through mirrors whose versions drifted from the lockfile CI installs, the same pattern quantfit and saucier already moved away from. - Declare `sqlalchemy[asyncio]>=2.0` and `aiosqlite>=0.21` as direct dependencies; `uv lock --upgrade` to google-adk 2.8.0, cryptography 50.0.1, ruff 0.16.5, ty 0.0.78, pytest 9.1.1, docvet 1.15.1 - Run every Python hook through `uv run` so pre-commit and CI share one pinned version; add `pre-commit-hooks` whitespace/EOF/merge-marker/large-file checks; pin the docvet and actionlint actions by SHA in CI and add yamllint + actionlint steps to the lint job - Adopt ruff 0.16 defaults: keep `E402` on, ignore `RUF022` (our `__all__` ordering is plain-alphabetical, enforced by `test_public_api.py`), allow `DTZ` in tests and spike scripts, let ruff format Markdown code fences; exclude the framework-generated `_bmad`, `_bmad-output`, `.specify`, and `.claude/commands` trees from ruff and from the whitespace hooks - Move to PEP 639 license metadata (silences the uv_build classifier warning), widen `uv_build` to `<0.13`, drop the now-unused pygments `uv-secure` ignore, refresh the hook tables in CONTRIBUTING and the development guide
4 tasks
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.
google-adk 2.4.0 renamed the table-creation hook from
_prepare_tables()to the publicprepare_tables(), addedis_postgresqltoStorageSession.to_session()andget_update_timestamp(), and switchedappend_event()to apply state deltas by mutating the loaded dict in place. Against ADK 2.8.0 (current PyPI) the suite had 56 failures: every CRUD call raisedTypeErroron the unknown keyword, our encrypted table override was never invoked (upstream created plaintext-typed tables from its own metadata), and state deltas were silently dropped because our encryptedstatecolumns lackedMutableDictchange tracking. Separately, google-adk 1.22.0 through 1.25.x read a private_dialect_nameproperty our model never defined, so the minimum supported version was also broken onappend_event().is_postgresqlonto_session()andget_update_timestamp(); interpret naiveupdate_timethe way the installed ADK wrote it (UTC for SQLite by flag or bound-engine detection, foris_postgresql, or on >= 2.4.0; local otherwise)prepare_tables()and keep_prepare_tables()as a delegating alias so one class serves both hook namesMutableDict.associate_with_attributeon the threestateattributes (notas_mutable, which leaks process-global listeners that retain key material and would also trackevent_data)after_timestampfilter matches on non-UTC hosts; read the epoch back fromevent_datafirst_dialect_nameproperty for google-adk 1.22.0 through 1.25.x, and ignore the upstreamBaseAgentConfigDeprecationWarninggoogle-adk 2.x emits at importTest:
uv run pytest(locked 1.26.0); alsouv run --isolated --with "google-adk[db]==X" pytestfor X in 1.22.0, 2.0.0, 2.4.0, 2.7.1, 2.8.0, withTZ=Asia/TokyoandTZ=America/New_York— 316 passed on eachPR Review
Checklist
uv run pytest)uv run ruff check .)!in title andBREAKING CHANGE:in bodyReview Focus
as_mutableleaking never-collected global listeners that retain the backend key; event timestamps stored in the wrong convention for ADK >= 2.7 soafter_timestampsilently dropped/leaked events on non-UTC hosts;get_update_timestampregressing non-SQLite dialects on ADK < 2.4; andevent_databeing mutation-tracked contrary to upstream. All fixed, each with a test that fails without the fix.TZviatime.tzset()in both directions (New York and Tokyo). The Tokyo cell is what caught the 1.22.0 stale-session false positive; New York passed by coincidence because the error runs the other way there.models.py: theMutableDict.as_mutable(encrypted_json)wrapping is the load-bearing change.tests/integration/test_state_mutation_tracking.pyfails without it on every ADK version, so the regression is caught regardless of which upstream release CI installs.get_update_timestampnow ignores both dialect flags and branches ontzinfo, matching upstream 2.x. Naive datetimes (SQLite, PostgreSQL) are treated as UTC; this is behaviourally identical to the oldis_sqlite=Truepath for SQLite.test_adk_conformance.pyandtest_encrypted_session_service.pycheck that upstream method parameters are a subset of ours and that whichever table hook upstream exposes resolves to our override. Either would have caught the 2.4.0 break at CI time.latestCI matrix cell has not run since March 29. This PR is the first time it will exercise a 2.x release.Related
uv-secureflags stale transitive deps (starlette, urllib3, sqlparse). Lock bump and toolchain refresh follow in a separate PR.