From b0189c47e007cd035f48e675cf1127c0767c0538 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Fri, 28 Aug 2026 22:20:15 +0800 Subject: [PATCH 1/9] fix(sessions): support MySQL schema creation Signed-off-by: linhongyu510 --- .../extensions/memory/sqlalchemy_session.py | 15 ++++-- .../memory/test_sqlalchemy_session.py | 50 ++++++++++++++++++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 81f7dcdae8..35d435ff1b 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -62,6 +62,14 @@ _T = TypeVar("_T") +# MySQL-family dialects require a bounded VARCHAR for indexed string columns. +_MYSQL_SESSION_ID_MAX_LENGTH = 190 +_SESSION_ID_TYPE = String().with_variant( + String(_MYSQL_SESSION_ID_MAX_LENGTH), + "mysql", + "mariadb", +) + class SQLAlchemySession(SessionABC): """SQLAlchemy implementation of [`Session`][agents.memory.session.Session].""" @@ -163,7 +171,8 @@ def __init__( 'mysql+aiomysql://', or 'sqlite+aiosqlite://'). create_tables (bool, optional): Whether to automatically create the required tables and indexes. Defaults to False for production use. Set to True for - development and testing when migrations aren't used. + development and testing when migrations aren't used. Automatically created + MySQL and MariaDB schemas store session IDs in VARCHAR(190) columns. sessions_table (str, optional): Override the default table name for sessions if needed. messages_table (str, optional): Override the default table name for messages if needed. session_settings (SessionSettings | None, optional): Session configuration settings @@ -189,7 +198,7 @@ def __init__( self._sessions = Table( sessions_table, self._metadata, - Column("session_id", String, primary_key=True), + Column("session_id", _SESSION_ID_TYPE, primary_key=True), Column( "created_at", TIMESTAMP(timezone=False), @@ -211,7 +220,7 @@ def __init__( Column("id", Integer, primary_key=True, autoincrement=True), Column( "session_id", - String, + _SESSION_ID_TYPE, ForeignKey(f"{sessions_table}.session_id", ondelete="CASCADE"), nullable=False, ), diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index b985d0a7e9..2c34857d1c 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -17,7 +17,8 @@ ResponseReasoningItemParam, Summary, ) -from sqlalchemy import event, insert, select, text, update +from sqlalchemy import create_mock_engine, event, insert, select, text, update +from sqlalchemy.dialects import postgresql, sqlite from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine from sqlalchemy.sql import Select @@ -35,6 +36,53 @@ DB_URL = "sqlite+aiosqlite:///:memory:" +@pytest.mark.parametrize("dialect_url", ["mysql://", "mariadb://"]) +async def test_schema_create_all_compiles_for_mysql_family(dialect_url: str): + """MySQL-family schema creation includes both tables and the session-time index.""" + session = SQLAlchemySession.from_url("schema_compile", url=DB_URL) + tables = (session._sessions, session._messages) + statements: list[str] = [] + + def record(statement: Any, *args: Any, **kwargs: Any) -> None: + statements.append(str(statement.compile(dialect=engine.dialect))) + + engine = create_mock_engine(dialect_url, record) + + try: + session._metadata.create_all(engine) + for table in tables: + assert table.c.session_id.type.compile(dialect=engine.dialect) == "VARCHAR(190)" + finally: + await session.engine.dispose() + + assert any("CREATE TABLE agent_sessions" in statement for statement in statements) + messages_ddl = next( + statement for statement in statements if "CREATE TABLE agent_messages" in statement + ) + assert ( + "FOREIGN KEY(session_id) REFERENCES agent_sessions (session_id) ON DELETE CASCADE" + in messages_ddl + ) + assert any( + "CREATE INDEX idx_agent_messages_session_time " + "ON agent_messages (session_id, created_at)" in statement + for statement in statements + ) + + +async def test_schema_keeps_unbounded_session_ids_for_sqlite_and_postgresql(): + """SQLite and PostgreSQL retain the pre-existing unbounded string type.""" + session = SQLAlchemySession.from_url("schema_compile", url=DB_URL) + + try: + for table in (session._sessions, session._messages): + session_id_type = table.c.session_id.type + assert session_id_type.compile(dialect=postgresql.dialect()) == "VARCHAR" + assert session_id_type.compile(dialect=sqlite.dialect()) == "VARCHAR" + finally: + await session.engine.dispose() + + def _make_message_item(item_id: str, text_value: str) -> TResponseInputItem: content: ResponseOutputTextParam = { "type": "output_text", From af3327596c0c2a60b8a32508f9999d1cb5368f4c Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Fri, 4 Sep 2026 18:32:46 +0800 Subject: [PATCH 2/9] fix(sessions): preserve case-sensitive MySQL IDs --- .../extensions/memory/sqlalchemy_session.py | 2 +- .../memory/test_sqlalchemy_session.py | 21 ++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 35d435ff1b..02c54d042b 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -65,7 +65,7 @@ # MySQL-family dialects require a bounded VARCHAR for indexed string columns. _MYSQL_SESSION_ID_MAX_LENGTH = 190 _SESSION_ID_TYPE = String().with_variant( - String(_MYSQL_SESSION_ID_MAX_LENGTH), + String(_MYSQL_SESSION_ID_MAX_LENGTH, collation="utf8mb4_bin"), "mysql", "mariadb", ) diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index 2c34857d1c..574ebfcec0 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -51,7 +51,10 @@ def record(statement: Any, *args: Any, **kwargs: Any) -> None: try: session._metadata.create_all(engine) for table in tables: - assert table.c.session_id.type.compile(dialect=engine.dialect) == "VARCHAR(190)" + assert ( + table.c.session_id.type.compile(dialect=engine.dialect) + == "VARCHAR(190) COLLATE utf8mb4_bin" + ) finally: await session.engine.dispose() @@ -251,6 +254,22 @@ async def test_session_isolation(agent: Agent): assert "dogs" not in result.final_output.lower() +async def test_session_ids_are_case_sensitive(): + """Session IDs that differ only by case retain separate histories.""" + engine = create_async_engine(DB_URL) + upper = SQLAlchemySession("Foo", engine=engine, create_tables=True) + lower = SQLAlchemySession("foo", engine=engine, create_tables=True) + + try: + await upper.add_items([{"role": "user", "content": "upper"}]) + await lower.add_items([{"role": "user", "content": "lower"}]) + + assert await upper.get_items() == [{"role": "user", "content": "upper"}] + assert await lower.get_items() == [{"role": "user", "content": "lower"}] + finally: + await engine.dispose() + + async def test_get_items_with_limit(agent: Agent): """Test the limit parameter in get_items.""" session_id = "limit_test" From 62651f1660f99ed5972828d0c90814511b473e60 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Sat, 5 Sep 2026 01:48:59 +0800 Subject: [PATCH 3/9] fix(sessions): reject MySQL session IDs the column cannot keep distinct utf8mb4_bin is a PAD SPACE collation, so MySQL and MariaDB ignore trailing spaces when comparing VARCHAR values. "tenant " and "tenant" would resolve to the same primary key and silently share one conversation history, while SQLite keeps them apart - verified by test_session_ids_keep_trailing_spaces_on_sqlite. Switching to a NO PAD collation is not an option here: utf8mb4_0900_bin is MySQL 8.0.4+ only and MariaDB and MySQL 5.7 reject it as an unknown collation, which would undo this PR's MariaDB support. Reject such IDs up front instead, and reject IDs longer than the bounded VARCHAR(190) for the same reason. Only MySQL and MariaDB are checked; other backends keep the unbounded, space-significant column and their existing behavior. Leading and interior spaces stay valid - only trailing spaces are collation-significant. --- .../extensions/memory/sqlalchemy_session.py | 33 ++++++++++ .../memory/test_sqlalchemy_session.py | 60 ++++++++++++++++++- 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 02c54d042b..67f477ef61 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -69,6 +69,38 @@ "mysql", "mariadb", ) +# Dialect names that receive the bounded, utf8mb4_bin variant above. +_MYSQL_FAMILY_DIALECTS = frozenset({"mysql", "mariadb"}) + + +def _validate_mysql_session_id(session_id: str, dialect_name: str) -> None: + """Reject session IDs that the MySQL-family column cannot keep distinct. + + ``utf8mb4_bin`` is a PAD SPACE collation, so MySQL and MariaDB ignore + trailing spaces when comparing ``VARCHAR`` values. ``"a"`` and ``"a "`` + would therefore resolve to the same primary key and silently share one + conversation history, while SQLite and PostgreSQL keep them apart. IDs + longer than the column would also be truncated or rejected by the server + only at write time. + + Only MySQL and MariaDB are checked: other backends store the unbounded, + space-significant type and keep their existing behavior. + """ + if dialect_name not in _MYSQL_FAMILY_DIALECTS: + return + if session_id != session_id.rstrip(" "): + raise ValueError( + "session_id must not end with a space on MySQL or MariaDB: " + f"{session_id!r}. The session_id column uses the utf8mb4_bin " + "collation, which ignores trailing spaces when comparing values, " + "so this ID would share stored history with " + f"{session_id.rstrip(' ')!r}." + ) + if len(session_id) > _MYSQL_SESSION_ID_MAX_LENGTH: + raise ValueError( + f"session_id must be at most {_MYSQL_SESSION_ID_MAX_LENGTH} characters on " + f"MySQL or MariaDB, got {len(session_id)}: {session_id!r}." + ) class SQLAlchemySession(SessionABC): @@ -186,6 +218,7 @@ def __init__( else SessionSettings() ) self._engine = engine + _validate_mysql_session_id(session_id, engine.dialect.name) self._ensure_ascii = ensure_ascii self._configure_sqlite_engine(engine) self._init_lock = ( diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index 574ebfcec0..93b53e97d4 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -25,7 +25,10 @@ pytest.importorskip("sqlalchemy") # Skip tests if SQLAlchemy is not installed from agents import Agent, Runner, TResponseInputItem -from agents.extensions.memory.sqlalchemy_session import SQLAlchemySession +from agents.extensions.memory.sqlalchemy_session import ( + SQLAlchemySession, + _validate_mysql_session_id, +) from agents.testing import ScriptedModel from tests.test_responses import get_text_message @@ -270,6 +273,61 @@ async def test_session_ids_are_case_sensitive(): await engine.dispose() +@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) +async def test_mysql_session_id_rejects_trailing_space(dialect_name: str): + """utf8mb4_bin is PAD SPACE, so a trailing space would collide on MySQL. + + MySQL and MariaDB ignore trailing spaces when comparing VARCHAR values + under this collation, so "tenant " and "tenant" would resolve to the same + primary key and share one history. SQLite keeps them apart (asserted in + test_session_ids_keep_trailing_spaces_on_sqlite), so accepting the ID would + make the same code diverge per backend. + """ + with pytest.raises(ValueError, match="must not end with a space"): + _validate_mysql_session_id("tenant ", dialect_name) + + +@pytest.mark.parametrize("dialect_name", ["sqlite", "postgresql"]) +async def test_non_mysql_dialects_still_accept_trailing_space(dialect_name: str): + """Other backends keep the unbounded, space-significant column.""" + _validate_mysql_session_id("tenant ", dialect_name) + + +@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) +async def test_mysql_session_id_rejects_ids_longer_than_the_column(dialect_name: str): + """The bounded VARCHAR(190) cannot store a longer ID without truncation.""" + with pytest.raises(ValueError, match="at most 190 characters"): + _validate_mysql_session_id("a" * 191, dialect_name) + + _validate_mysql_session_id("a" * 190, dialect_name) + + +@pytest.mark.parametrize("session_id", [" leading", "mid dle", "tenant"]) +async def test_mysql_session_id_allows_spaces_that_do_not_pad(session_id: str): + """Only trailing spaces are collation-significant; the rest stay valid.""" + _validate_mysql_session_id(session_id, "mysql") + + +async def test_session_ids_keep_trailing_spaces_on_sqlite(): + """Oracle for the MySQL guard: SQLite treats a trailing space as distinct. + + This is the behavior the MySQL-family column cannot reproduce, which is why + such IDs are rejected up front there instead of silently merging. + """ + engine = create_async_engine(DB_URL) + bare = SQLAlchemySession("tenant", engine=engine, create_tables=True) + padded = SQLAlchemySession("tenant ", engine=engine, create_tables=True) + + try: + await bare.add_items([{"role": "user", "content": "bare"}]) + await padded.add_items([{"role": "user", "content": "padded"}]) + + assert await bare.get_items() == [{"role": "user", "content": "bare"}] + assert await padded.get_items() == [{"role": "user", "content": "padded"}] + finally: + await engine.dispose() + + async def test_get_items_with_limit(agent: Agent): """Test the limit parameter in get_items.""" session_id = "limit_test" From 359f366098f765f5a4ec15d7783bfcfc4a951b52 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Sat, 5 Sep 2026 13:32:41 +0800 Subject: [PATCH 4/9] fix(sessions): scope MySQL session ID length check to generated schema The length bound describes only the VARCHAR(190) column this module creates. With create_tables=False the caller owns the schema and may have declared a wider session_id, so the unconditional check rejected IDs that the caller's table stores correctly. Gate the length check on created_schema. Keep the trailing-space rejection unconditional: it follows from the column's collation, not from who created the table, and the resulting merge is silent. --- .../extensions/memory/sqlalchemy_session.py | 24 +++++--- .../memory/test_sqlalchemy_session.py | 55 ++++++++++++++++--- 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 67f477ef61..0eb8a51ad4 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -73,15 +73,24 @@ _MYSQL_FAMILY_DIALECTS = frozenset({"mysql", "mariadb"}) -def _validate_mysql_session_id(session_id: str, dialect_name: str) -> None: +def _validate_mysql_session_id(session_id: str, dialect_name: str, *, created_schema: bool) -> None: """Reject session IDs that the MySQL-family column cannot keep distinct. ``utf8mb4_bin`` is a PAD SPACE collation, so MySQL and MariaDB ignore trailing spaces when comparing ``VARCHAR`` values. ``"a"`` and ``"a "`` would therefore resolve to the same primary key and silently share one - conversation history, while SQLite and PostgreSQL keep them apart. IDs - longer than the column would also be truncated or rejected by the server - only at write time. + conversation history, while SQLite and PostgreSQL keep them apart. That + merge is silent and corrupts stored history, so it is rejected regardless + of who owns the schema: any MySQL-family collation that ignores trailing + spaces produces it, and a caller-managed column cannot opt out of the + comparison semantics its own collation defines. + + The length bound is different: it describes only the column this module + creates. A caller-managed schema may declare a wider ``session_id`` (for + example ``VARCHAR(255)``) that stores longer IDs correctly, and the dialect + name alone does not reveal the real column width, so the bound is checked + only when this module created the schema. Otherwise the caller-owned schema + enforces its own constraint. Only MySQL and MariaDB are checked: other backends store the unbounded, space-significant type and keep their existing behavior. @@ -96,7 +105,7 @@ def _validate_mysql_session_id(session_id: str, dialect_name: str) -> None: "so this ID would share stored history with " f"{session_id.rstrip(' ')!r}." ) - if len(session_id) > _MYSQL_SESSION_ID_MAX_LENGTH: + if created_schema and len(session_id) > _MYSQL_SESSION_ID_MAX_LENGTH: raise ValueError( f"session_id must be at most {_MYSQL_SESSION_ID_MAX_LENGTH} characters on " f"MySQL or MariaDB, got {len(session_id)}: {session_id!r}." @@ -204,7 +213,8 @@ def __init__( create_tables (bool, optional): Whether to automatically create the required tables and indexes. Defaults to False for production use. Set to True for development and testing when migrations aren't used. Automatically created - MySQL and MariaDB schemas store session IDs in VARCHAR(190) columns. + MySQL and MariaDB schemas store session IDs in VARCHAR(190) columns, and + session IDs longer than that are rejected only for those schemas. sessions_table (str, optional): Override the default table name for sessions if needed. messages_table (str, optional): Override the default table name for messages if needed. session_settings (SessionSettings | None, optional): Session configuration settings @@ -218,7 +228,7 @@ def __init__( else SessionSettings() ) self._engine = engine - _validate_mysql_session_id(session_id, engine.dialect.name) + _validate_mysql_session_id(session_id, engine.dialect.name, created_schema=create_tables) self._ensure_ascii = ensure_ascii self._configure_sqlite_engine(engine) self._init_lock = ( diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index 93b53e97d4..208d905f2a 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -8,7 +8,9 @@ from contextlib import asynccontextmanager from datetime import datetime, timedelta from pathlib import Path +from types import SimpleNamespace from typing import Any, cast +from unittest.mock import MagicMock import pytest from openai.types.responses.response_output_message_param import ResponseOutputMessageParam @@ -274,7 +276,8 @@ async def test_session_ids_are_case_sensitive(): @pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -async def test_mysql_session_id_rejects_trailing_space(dialect_name: str): +@pytest.mark.parametrize("created_schema", [True, False]) +async def test_mysql_session_id_rejects_trailing_space(dialect_name: str, created_schema: bool): """utf8mb4_bin is PAD SPACE, so a trailing space would collide on MySQL. MySQL and MariaDB ignore trailing spaces when comparing VARCHAR values @@ -282,30 +285,66 @@ async def test_mysql_session_id_rejects_trailing_space(dialect_name: str): primary key and share one history. SQLite keeps them apart (asserted in test_session_ids_keep_trailing_spaces_on_sqlite), so accepting the ID would make the same code diverge per backend. + + The merge is silent and corrupts stored history, so it is rejected for + caller-managed schemas too: the collation defines the comparison, and a + caller cannot opt out of it by owning the table. """ with pytest.raises(ValueError, match="must not end with a space"): - _validate_mysql_session_id("tenant ", dialect_name) + _validate_mysql_session_id("tenant ", dialect_name, created_schema=created_schema) @pytest.mark.parametrize("dialect_name", ["sqlite", "postgresql"]) async def test_non_mysql_dialects_still_accept_trailing_space(dialect_name: str): """Other backends keep the unbounded, space-significant column.""" - _validate_mysql_session_id("tenant ", dialect_name) + _validate_mysql_session_id("tenant ", dialect_name, created_schema=True) @pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -async def test_mysql_session_id_rejects_ids_longer_than_the_column(dialect_name: str): - """The bounded VARCHAR(190) cannot store a longer ID without truncation.""" +async def test_mysql_session_id_rejects_ids_longer_than_the_created_column(dialect_name: str): + """The VARCHAR(190) this module creates cannot store a longer ID.""" with pytest.raises(ValueError, match="at most 190 characters"): - _validate_mysql_session_id("a" * 191, dialect_name) + _validate_mysql_session_id("a" * 191, dialect_name, created_schema=True) + + _validate_mysql_session_id("a" * 190, dialect_name, created_schema=True) + + +@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) +async def test_caller_managed_schema_keeps_its_own_session_id_length(dialect_name: str): + """A caller-owned schema may declare a wider column, so the bound must not apply. + + With ``create_tables=False`` the caller owns the table and may have declared + ``session_id VARCHAR(255)``, which stores a 191-character ID correctly. The + dialect name alone does not reveal the real column width, so rejecting the + ID here would break a schema that worked before this change. + """ + _validate_mysql_session_id("a" * 191, dialect_name, created_schema=False) - _validate_mysql_session_id("a" * 190, dialect_name) + +@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) +async def test_constructor_defers_session_id_length_to_caller_managed_schema(dialect_name: str): + """Construction with create_tables=False must not impose the generated column width. + + Exercised through the public constructor rather than the helper, because the + constructor is the boundary a caller with an existing ``VARCHAR(255)`` schema + actually crosses. A trailing-space ID is still rejected on the same path, + since that collides under the column's own collation regardless of owner. + """ + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name=dialect_name) + long_id = "a" * 191 + + session = SQLAlchemySession(long_id, engine=engine, create_tables=False) + assert session.session_id == long_id + + with pytest.raises(ValueError, match="must not end with a space"): + SQLAlchemySession("tenant ", engine=engine, create_tables=False) @pytest.mark.parametrize("session_id", [" leading", "mid dle", "tenant"]) async def test_mysql_session_id_allows_spaces_that_do_not_pad(session_id: str): """Only trailing spaces are collation-significant; the rest stay valid.""" - _validate_mysql_session_id(session_id, "mysql") + _validate_mysql_session_id(session_id, "mysql", created_schema=True) async def test_session_ids_keep_trailing_spaces_on_sqlite(): From 01e48e6f4de67ce350e07d00157fe3efb655c056 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Sat, 5 Sep 2026 15:47:58 +0800 Subject: [PATCH 5/9] fix(sessions): leave MySQL session ID length to the database The length bound was gated on `create_tables`, but that flag only requests idempotent creation: `create_all` uses `checkfirst`, so it leaves an existing table untouched. `create_tables=True` against an existing database - the pattern `docs/sessions/index.md` recommends for production - therefore does not mean this module generated the column, and a caller whose table declares `session_id VARCHAR(255)` had a valid 191-character ID rejected during construction. Drop the bound entirely rather than trying to infer schema ownership. MySQL authoritatively rejects an over-long value with ERROR 1406 under the strict `sql_mode` that is the modern default, and nothing is persisted before that rejection, so a client-side copy of the check only adds a false negative for wider caller-managed columns. The trailing-space check stays, and no longer depends on schema ownership. Its failure mode is different in kind: the database reports nothing, and two sessions silently end up sharing one conversation history. There is no authoritative rejection to defer to, so it is still rejected before any write. `_MYSQL_SESSION_ID_MAX_LENGTH` remains the width of the generated column. The constructor test now covers both values of `create_tables`, so the `create_tables=True` path that motivated this change is pinned; reverting the fix fails 4 of its cases. Co-authored-by: Claude --- .../extensions/memory/sqlalchemy_session.py | 49 +++++++------- .../memory/test_sqlalchemy_session.py | 66 ++++++++----------- 2 files changed, 52 insertions(+), 63 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 0eb8a51ad4..40532870c3 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -73,24 +73,28 @@ _MYSQL_FAMILY_DIALECTS = frozenset({"mysql", "mariadb"}) -def _validate_mysql_session_id(session_id: str, dialect_name: str, *, created_schema: bool) -> None: - """Reject session IDs that the MySQL-family column cannot keep distinct. +def _validate_mysql_session_id(session_id: str, dialect_name: str) -> None: + """Reject session IDs that a MySQL-family column cannot keep distinct. ``utf8mb4_bin`` is a PAD SPACE collation, so MySQL and MariaDB ignore trailing spaces when comparing ``VARCHAR`` values. ``"a"`` and ``"a "`` - would therefore resolve to the same primary key and silently share one - conversation history, while SQLite and PostgreSQL keep them apart. That - merge is silent and corrupts stored history, so it is rejected regardless - of who owns the schema: any MySQL-family collation that ignores trailing - spaces produces it, and a caller-managed column cannot opt out of the - comparison semantics its own collation defines. - - The length bound is different: it describes only the column this module - creates. A caller-managed schema may declare a wider ``session_id`` (for - example ``VARCHAR(255)``) that stores longer IDs correctly, and the dialect - name alone does not reveal the real column width, so the bound is checked - only when this module created the schema. Otherwise the caller-owned schema - enforces its own constraint. + therefore resolve to the same primary key and two ``SQLAlchemySession`` + instances silently read and write one shared conversation history, while + SQLite and PostgreSQL keep them apart. + + The database never reports this: there is no error to surface, only two + sessions whose stored history has been merged. That is persistent + corruption of one session's history by another, so it is rejected up front + rather than left to a rejection that never comes. + + Length is deliberately not checked here. MySQL authoritatively rejects an + over-long value with ``ERROR 1406`` under the strict ``sql_mode`` that is + the modern default, and a caller-managed schema may declare a wider + ``session_id`` (for example ``VARCHAR(255)``) that stores it correctly. The + dialect name does not reveal the real column width, and ``create_tables`` + only requests idempotent creation -- ``create_all`` uses ``checkfirst`` and + leaves an existing table untouched -- so neither is evidence of the width + this module generates. Only MySQL and MariaDB are checked: other backends store the unbounded, space-significant type and keep their existing behavior. @@ -100,15 +104,10 @@ def _validate_mysql_session_id(session_id: str, dialect_name: str, *, created_sc if session_id != session_id.rstrip(" "): raise ValueError( "session_id must not end with a space on MySQL or MariaDB: " - f"{session_id!r}. The session_id column uses the utf8mb4_bin " - "collation, which ignores trailing spaces when comparing values, " - "so this ID would share stored history with " - f"{session_id.rstrip(' ')!r}." - ) - if created_schema and len(session_id) > _MYSQL_SESSION_ID_MAX_LENGTH: - raise ValueError( - f"session_id must be at most {_MYSQL_SESSION_ID_MAX_LENGTH} characters on " - f"MySQL or MariaDB, got {len(session_id)}: {session_id!r}." + f"{session_id!r}. MySQL-family collations such as the utf8mb4_bin " + "used by the generated schema ignore trailing spaces when " + "comparing values, so this ID would silently share stored history " + f"with {session_id.rstrip(' ')!r}." ) @@ -228,7 +227,7 @@ def __init__( else SessionSettings() ) self._engine = engine - _validate_mysql_session_id(session_id, engine.dialect.name, created_schema=create_tables) + _validate_mysql_session_id(session_id, engine.dialect.name) self._ensure_ascii = ensure_ascii self._configure_sqlite_engine(engine) self._init_lock = ( diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index 208d905f2a..0d2bee8277 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -276,8 +276,7 @@ async def test_session_ids_are_case_sensitive(): @pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -@pytest.mark.parametrize("created_schema", [True, False]) -async def test_mysql_session_id_rejects_trailing_space(dialect_name: str, created_schema: bool): +async def test_mysql_session_id_rejects_trailing_space(dialect_name: str): """utf8mb4_bin is PAD SPACE, so a trailing space would collide on MySQL. MySQL and MariaDB ignore trailing spaces when comparing VARCHAR values @@ -286,65 +285,56 @@ async def test_mysql_session_id_rejects_trailing_space(dialect_name: str, create test_session_ids_keep_trailing_spaces_on_sqlite), so accepting the ID would make the same code diverge per backend. - The merge is silent and corrupts stored history, so it is rejected for - caller-managed schemas too: the collation defines the comparison, and a - caller cannot opt out of it by owning the table. + The database reports nothing here -- there is no error to surface, only two + sessions whose stored history has silently merged -- so it is rejected up + front rather than left to a rejection that never arrives. """ with pytest.raises(ValueError, match="must not end with a space"): - _validate_mysql_session_id("tenant ", dialect_name, created_schema=created_schema) + _validate_mysql_session_id("tenant ", dialect_name) @pytest.mark.parametrize("dialect_name", ["sqlite", "postgresql"]) async def test_non_mysql_dialects_still_accept_trailing_space(dialect_name: str): """Other backends keep the unbounded, space-significant column.""" - _validate_mysql_session_id("tenant ", dialect_name, created_schema=True) + _validate_mysql_session_id("tenant ", dialect_name) @pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -async def test_mysql_session_id_rejects_ids_longer_than_the_created_column(dialect_name: str): - """The VARCHAR(190) this module creates cannot store a longer ID.""" - with pytest.raises(ValueError, match="at most 190 characters"): - _validate_mysql_session_id("a" * 191, dialect_name, created_schema=True) - - _validate_mysql_session_id("a" * 190, dialect_name, created_schema=True) - - -@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -async def test_caller_managed_schema_keeps_its_own_session_id_length(dialect_name: str): - """A caller-owned schema may declare a wider column, so the bound must not apply. - - With ``create_tables=False`` the caller owns the table and may have declared - ``session_id VARCHAR(255)``, which stores a 191-character ID correctly. The - dialect name alone does not reveal the real column width, so rejecting the - ID here would break a schema that worked before this change. - """ - _validate_mysql_session_id("a" * 191, dialect_name, created_schema=False) - - -@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -async def test_constructor_defers_session_id_length_to_caller_managed_schema(dialect_name: str): - """Construction with create_tables=False must not impose the generated column width. - - Exercised through the public constructor rather than the helper, because the - constructor is the boundary a caller with an existing ``VARCHAR(255)`` schema - actually crosses. A trailing-space ID is still rejected on the same path, - since that collides under the column's own collation regardless of owner. +@pytest.mark.parametrize("create_tables", [True, False]) +async def test_constructor_does_not_impose_a_session_id_length_bound( + dialect_name: str, create_tables: bool +): + """Length is left to the database, for either value of ``create_tables``. + + A caller-managed table may declare ``session_id VARCHAR(255)`` and store a + 191-character ID correctly, and MySQL authoritatively rejects an over-long + value with ``ERROR 1406`` under the strict ``sql_mode`` that is the modern + default. Neither the dialect name nor ``create_tables`` reveals the real + column width: ``create_all`` uses ``checkfirst`` and leaves an existing + table untouched, so ``create_tables=True`` against an existing database -- + the pattern documented in ``docs/sessions/index.md`` -- does not mean this + module generated the column. + + Exercised through the public constructor because that is the boundary such + a caller crosses. A trailing-space ID is still rejected on the same path, + since that merges history under the column's own collation instead of + producing an error. """ engine = MagicMock(spec=AsyncEngine) engine.dialect = SimpleNamespace(name=dialect_name) long_id = "a" * 191 - session = SQLAlchemySession(long_id, engine=engine, create_tables=False) + session = SQLAlchemySession(long_id, engine=engine, create_tables=create_tables) assert session.session_id == long_id with pytest.raises(ValueError, match="must not end with a space"): - SQLAlchemySession("tenant ", engine=engine, create_tables=False) + SQLAlchemySession("tenant ", engine=engine, create_tables=create_tables) @pytest.mark.parametrize("session_id", [" leading", "mid dle", "tenant"]) async def test_mysql_session_id_allows_spaces_that_do_not_pad(session_id: str): """Only trailing spaces are collation-significant; the rest stay valid.""" - _validate_mysql_session_id(session_id, "mysql", created_schema=True) + _validate_mysql_session_id(session_id, "mysql") async def test_session_ids_keep_trailing_spaces_on_sqlite(): From ad7b35ca8b79deb86b78bd8c76d2e43ea933cd82 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Sat, 5 Sep 2026 18:20:24 +0800 Subject: [PATCH 6/9] Declare CHARACTER SET utf8mb4 with the utf8mb4_bin collation The MySQL variant emitted `VARCHAR(190) COLLATE utf8mb4_bin` with no character set. A column given only a collation inherits the database character set, and the server rejects a collation that does not belong to that set with ERROR 1253 COLLATION 'utf8mb4_bin' is not valid for CHARACTER SET '' so on an install whose default is not utf8mb4 -- a stock MySQL 5.7 defaults to latin1 -- create_all() failed before either table existed. Switching to mysql.VARCHAR lets the charset be declared alongside the collation. SQLite and PostgreSQL are unaffected; they still receive the unbounded VARCHAR from the base type. Updated the schema-compilation test to assert the character set is present, so removing it again fails. Co-authored-by: Claude --- src/agents/extensions/memory/sqlalchemy_session.py | 13 ++++++++++++- tests/extensions/memory/test_sqlalchemy_session.py | 6 +++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 40532870c3..137e6e17f2 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -48,6 +48,7 @@ text as sql_text, update, ) +from sqlalchemy.dialects import mysql as mysql_dialect from sqlalchemy.exc import IntegrityError, OperationalError from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine @@ -64,8 +65,18 @@ # MySQL-family dialects require a bounded VARCHAR for indexed string columns. _MYSQL_SESSION_ID_MAX_LENGTH = 190 +# ``CHARACTER SET`` is declared alongside the collation: a column given only a +# collation inherits the database character set, and the server rejects +# ``utf8mb4_bin`` against a non-utf8mb4 inherited set with +# "ERROR 1253 COLLATION 'utf8mb4_bin' is not valid for CHARACTER SET ''". +# A MySQL 5.7 install defaulting to latin1 would otherwise fail in +# ``create_all()`` before either table exists. _SESSION_ID_TYPE = String().with_variant( - String(_MYSQL_SESSION_ID_MAX_LENGTH, collation="utf8mb4_bin"), + mysql_dialect.VARCHAR( + _MYSQL_SESSION_ID_MAX_LENGTH, + charset="utf8mb4", + collation="utf8mb4_bin", + ), "mysql", "mariadb", ) diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index 0d2bee8277..b546c85e6b 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -56,9 +56,13 @@ def record(statement: Any, *args: Any, **kwargs: Any) -> None: try: session._metadata.create_all(engine) for table in tables: + # CHARACTER SET must be emitted with the collation: a column given + # only a collation inherits the database character set, and the + # server rejects utf8mb4_bin against a non-utf8mb4 set with + # ERROR 1253, failing create_all() on e.g. a latin1 MySQL 5.7. assert ( table.c.session_id.type.compile(dialect=engine.dialect) - == "VARCHAR(190) COLLATE utf8mb4_bin" + == "VARCHAR(190) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin" ) finally: await session.engine.dispose() From 8bc0d41b17ee02b89ff581c3b4e1f280cf9e592f Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Sat, 5 Sep 2026 20:21:00 +0800 Subject: [PATCH 7/9] fix(sessions): respect caller-managed MySQL collation Remove the dialect-only trailing-space guard. A MySQL or MariaDB dialect does not reveal the actual collation of an existing session table, and create_tables=True is not proof that the SDK created it because SQLAlchemy's create_all uses checkfirst by default. Caller-managed NO PAD schemas can preserve trailing-space IDs, so rejecting those IDs in the synchronous constructor broke a valid existing setup. Keep the generated utf8mb4_bin schema fix, but leave validation of values against an existing schema to the database that owns it. Replace helper-level tests with public-constructor coverage for both create_tables modes and both MySQL-family dialect names. --- .../extensions/memory/sqlalchemy_session.py | 41 ---------- .../memory/test_sqlalchemy_session.py | 78 ++++--------------- 2 files changed, 17 insertions(+), 102 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 137e6e17f2..9b487398fc 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -80,46 +80,6 @@ "mysql", "mariadb", ) -# Dialect names that receive the bounded, utf8mb4_bin variant above. -_MYSQL_FAMILY_DIALECTS = frozenset({"mysql", "mariadb"}) - - -def _validate_mysql_session_id(session_id: str, dialect_name: str) -> None: - """Reject session IDs that a MySQL-family column cannot keep distinct. - - ``utf8mb4_bin`` is a PAD SPACE collation, so MySQL and MariaDB ignore - trailing spaces when comparing ``VARCHAR`` values. ``"a"`` and ``"a "`` - therefore resolve to the same primary key and two ``SQLAlchemySession`` - instances silently read and write one shared conversation history, while - SQLite and PostgreSQL keep them apart. - - The database never reports this: there is no error to surface, only two - sessions whose stored history has been merged. That is persistent - corruption of one session's history by another, so it is rejected up front - rather than left to a rejection that never comes. - - Length is deliberately not checked here. MySQL authoritatively rejects an - over-long value with ``ERROR 1406`` under the strict ``sql_mode`` that is - the modern default, and a caller-managed schema may declare a wider - ``session_id`` (for example ``VARCHAR(255)``) that stores it correctly. The - dialect name does not reveal the real column width, and ``create_tables`` - only requests idempotent creation -- ``create_all`` uses ``checkfirst`` and - leaves an existing table untouched -- so neither is evidence of the width - this module generates. - - Only MySQL and MariaDB are checked: other backends store the unbounded, - space-significant type and keep their existing behavior. - """ - if dialect_name not in _MYSQL_FAMILY_DIALECTS: - return - if session_id != session_id.rstrip(" "): - raise ValueError( - "session_id must not end with a space on MySQL or MariaDB: " - f"{session_id!r}. MySQL-family collations such as the utf8mb4_bin " - "used by the generated schema ignore trailing spaces when " - "comparing values, so this ID would silently share stored history " - f"with {session_id.rstrip(' ')!r}." - ) class SQLAlchemySession(SessionABC): @@ -238,7 +198,6 @@ def __init__( else SessionSettings() ) self._engine = engine - _validate_mysql_session_id(session_id, engine.dialect.name) self._ensure_ascii = ensure_ascii self._configure_sqlite_engine(engine) self._init_lock = ( diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index b546c85e6b..b02a06d25c 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -27,10 +27,7 @@ pytest.importorskip("sqlalchemy") # Skip tests if SQLAlchemy is not installed from agents import Agent, Runner, TResponseInputItem -from agents.extensions.memory.sqlalchemy_session import ( - SQLAlchemySession, - _validate_mysql_session_id, -) +from agents.extensions.memory.sqlalchemy_session import SQLAlchemySession from agents.testing import ScriptedModel from tests.test_responses import get_text_message @@ -279,74 +276,33 @@ async def test_session_ids_are_case_sensitive(): await engine.dispose() -@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) -async def test_mysql_session_id_rejects_trailing_space(dialect_name: str): - """utf8mb4_bin is PAD SPACE, so a trailing space would collide on MySQL. - - MySQL and MariaDB ignore trailing spaces when comparing VARCHAR values - under this collation, so "tenant " and "tenant" would resolve to the same - primary key and share one history. SQLite keeps them apart (asserted in - test_session_ids_keep_trailing_spaces_on_sqlite), so accepting the ID would - make the same code diverge per backend. - - The database reports nothing here -- there is no error to surface, only two - sessions whose stored history has silently merged -- so it is rejected up - front rather than left to a rejection that never arrives. - """ - with pytest.raises(ValueError, match="must not end with a space"): - _validate_mysql_session_id("tenant ", dialect_name) - - -@pytest.mark.parametrize("dialect_name", ["sqlite", "postgresql"]) -async def test_non_mysql_dialects_still_accept_trailing_space(dialect_name: str): - """Other backends keep the unbounded, space-significant column.""" - _validate_mysql_session_id("tenant ", dialect_name) - - @pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) @pytest.mark.parametrize("create_tables", [True, False]) -async def test_constructor_does_not_impose_a_session_id_length_bound( +async def test_constructor_does_not_impose_generated_mysql_schema_constraints( dialect_name: str, create_tables: bool ): - """Length is left to the database, for either value of ``create_tables``. - - A caller-managed table may declare ``session_id VARCHAR(255)`` and store a - 191-character ID correctly, and MySQL authoritatively rejects an over-long - value with ``ERROR 1406`` under the strict ``sql_mode`` that is the modern - default. Neither the dialect name nor ``create_tables`` reveals the real - column width: ``create_all`` uses ``checkfirst`` and leaves an existing - table untouched, so ``create_tables=True`` against an existing database -- - the pattern documented in ``docs/sessions/index.md`` -- does not mean this - module generated the column. - - Exercised through the public constructor because that is the boundary such - a caller crosses. A trailing-space ID is still rejected on the same path, - since that merges history under the column's own collation instead of - producing an error. + """Caller-managed MySQL schemas remain authoritative. + + ``create_tables=True`` only asks SQLAlchemy to create missing tables. Its + default ``checkfirst`` behavior leaves an existing table untouched, so the + flag does not prove that this module owns either the column width or its + collation. A caller-managed table may use ``VARCHAR(255)`` and a NO PAD + collation, making both a 191-character ID and a trailing-space ID valid. + + The constructor is therefore deliberately neutral for both values of + ``create_tables``. The generated schema itself is covered by the DDL tests + above; a live database remains the authority for an existing schema. """ engine = MagicMock(spec=AsyncEngine) engine.dialect = SimpleNamespace(name=dialect_name) - long_id = "a" * 191 - - session = SQLAlchemySession(long_id, engine=engine, create_tables=create_tables) - assert session.session_id == long_id - - with pytest.raises(ValueError, match="must not end with a space"): - SQLAlchemySession("tenant ", engine=engine, create_tables=create_tables) - -@pytest.mark.parametrize("session_id", [" leading", "mid dle", "tenant"]) -async def test_mysql_session_id_allows_spaces_that_do_not_pad(session_id: str): - """Only trailing spaces are collation-significant; the rest stay valid.""" - _validate_mysql_session_id(session_id, "mysql") + for session_id in ("a" * 191, "tenant "): + session = SQLAlchemySession(session_id, engine=engine, create_tables=create_tables) + assert session.session_id == session_id async def test_session_ids_keep_trailing_spaces_on_sqlite(): - """Oracle for the MySQL guard: SQLite treats a trailing space as distinct. - - This is the behavior the MySQL-family column cannot reproduce, which is why - such IDs are rejected up front there instead of silently merging. - """ + """SQLite stores trailing-space IDs as distinct values.""" engine = create_async_engine(DB_URL) bare = SQLAlchemySession("tenant", engine=engine, create_tables=True) padded = SQLAlchemySession("tenant ", engine=engine, create_tables=True) From 9b61d2cad1c136efead56d647cf4b5fc43f9b449 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Tue, 8 Sep 2026 23:22:06 +0800 Subject: [PATCH 8/9] fix(sessions): reject trailing-space IDs under PAD SPACE MySQL collations The generated MySQL/MariaDB schema uses utf8mb4_bin, which is PAD SPACE: trailing spaces are insignificant in VARCHAR comparisons, so "tenant" and "tenant " silently resolve to the same primary key and share one conversation history with no error anywhere. 8bc0d41b removed the constructor-side guard because a dialect name cannot distinguish a SDK-created PAD SPACE column from a caller-managed NO PAD column. Move validation into _ensure_tables(), where create_all() has already run and we can query the actual column collation: - MySQL reads PAD_ATTRIBUTE from information_schema.COLLATIONS. - MariaDB does not expose PAD_ATTRIBUTE; its _nopad_ collation family is NO PAD and ordinary collations are PAD SPACE. - Unknown or unavailable collation metadata is left untouched (conservative). - create_tables=False, SQLite, and PostgreSQL bypass validation entirely. - No session-ID length validation is reintroduced. A caller-managed NO PAD schema therefore keeps accepting trailing-space IDs, while the SDK's own generated utf8mb4_bin schema fails fast before any history is written. --- .../extensions/memory/sqlalchemy_session.py | 52 +++++++- .../memory/test_sqlalchemy_session.py | 126 +++++++++++++++--- 2 files changed, 159 insertions(+), 19 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 9b487398fc..482769e148 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -49,8 +49,13 @@ update, ) from sqlalchemy.dialects import mysql as mysql_dialect -from sqlalchemy.exc import IntegrityError, OperationalError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.exc import IntegrityError, OperationalError, SQLAlchemyError +from sqlalchemy.ext.asyncio import ( + AsyncConnection, + AsyncEngine, + async_sessionmaker, + create_async_engine, +) from ...items import TResponseInputItem from ...memory.session import SessionABC @@ -299,6 +304,48 @@ async def _deserialize_item(self, item: str) -> TResponseInputItem: # ------------------------------------------------------------------ # Session protocol implementation # ------------------------------------------------------------------ + async def _validate_session_id_collation(self, conn: AsyncConnection) -> None: + """Reject trailing-space IDs only when the actual MySQL collation pads spaces.""" + if self._engine.dialect.name not in {"mysql", "mariadb"}: + return + if not self.session_id.endswith(" "): + return + + try: + collation_result = await conn.execute( + sql_text( + "SELECT COLLATION_NAME FROM information_schema.COLUMNS " + "WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = :table_name " + "AND COLUMN_NAME = 'session_id'" + ), + {"table_name": self._sessions.name}, + ) + collation = collation_result.scalar_one_or_none() + if not collation: + return + + pad_attribute: str | None + if getattr(self._engine.dialect, "is_mariadb", False): + pad_attribute = "NO PAD" if "_nopad_" in collation.casefold() else "PAD SPACE" + else: + pad_result = await conn.execute( + sql_text( + "SELECT PAD_ATTRIBUTE FROM information_schema.COLLATIONS " + "WHERE COLLATION_NAME = :collation" + ), + {"collation": collation}, + ) + pad_attribute = pad_result.scalar_one_or_none() + except SQLAlchemyError: + return + + if pad_attribute == "PAD SPACE": + raise ValueError( + f"session_id {self.session_id!r} ends with a space, which is not distinct " + f"under the column's PAD SPACE collation {collation!r}; two sessions would " + "silently share one history" + ) + async def _ensure_tables(self) -> None: """Ensure tables are created before any database operations.""" if not self._create_tables: @@ -315,6 +362,7 @@ async def _ensure_tables(self) -> None: async with self._engine.begin() as conn: await conn.run_sync(self._metadata.create_all) + await self._validate_session_id_collation(conn) self._create_tables = False # Only create once finally: self._init_lock.release() diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index b02a06d25c..04a802e412 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -10,7 +10,7 @@ from pathlib import Path from types import SimpleNamespace from typing import Any, cast -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock import pytest from openai.types.responses.response_output_message_param import ResponseOutputMessageParam @@ -278,27 +278,119 @@ async def test_session_ids_are_case_sensitive(): @pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) @pytest.mark.parametrize("create_tables", [True, False]) -async def test_constructor_does_not_impose_generated_mysql_schema_constraints( +async def test_constructor_does_not_impose_a_session_id_length_bound( dialect_name: str, create_tables: bool ): - """Caller-managed MySQL schemas remain authoritative. - - ``create_tables=True`` only asks SQLAlchemy to create missing tables. Its - default ``checkfirst`` behavior leaves an existing table untouched, so the - flag does not prove that this module owns either the column width or its - collation. A caller-managed table may use ``VARCHAR(255)`` and a NO PAD - collation, making both a 191-character ID and a trailing-space ID valid. - - The constructor is therefore deliberately neutral for both values of - ``create_tables``. The generated schema itself is covered by the DDL tests - above; a live database remains the authority for an existing schema. - """ + """The constructor leaves the actual schema authoritative for session ID length.""" engine = MagicMock(spec=AsyncEngine) engine.dialect = SimpleNamespace(name=dialect_name) + long_id = "a" * 191 - for session_id in ("a" * 191, "tenant "): - session = SQLAlchemySession(session_id, engine=engine, create_tables=create_tables) - assert session.session_id == session_id + session = SQLAlchemySession(long_id, engine=engine, create_tables=create_tables) + + assert session.session_id == long_id + + +class _ScalarResult: + def __init__(self, value: str | None) -> None: + self._value = value + + def scalar_one_or_none(self) -> str | None: + return self._value + + +@pytest.mark.parametrize("dialect_name", ["mysql", "mariadb"]) +async def test_validate_session_id_collation_rejects_pad_space( + dialect_name: str, +) -> None: + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name=dialect_name, is_mariadb=dialect_name == "mariadb") + session = SQLAlchemySession( + "tenant ", + engine=engine, + create_tables=True, + sessions_table="custom_sessions", + ) + conn = MagicMock() + conn.execute = AsyncMock(side_effect=[_ScalarResult("utf8mb4_bin"), _ScalarResult("PAD SPACE")]) + + with pytest.raises( + ValueError, + match=( + r"session_id 'tenant ' ends with a space, which is not distinct under the " + r"column's PAD SPACE collation 'utf8mb4_bin'; two sessions would silently " + r"share one history" + ), + ): + await session._validate_session_id_collation(conn) + + expected_calls = 1 if dialect_name == "mariadb" else 2 + assert conn.execute.await_count == expected_calls + assert conn.execute.await_args_list[0].args[1] == {"table_name": "custom_sessions"} + if dialect_name == "mysql": + assert conn.execute.await_args_list[1].args[1] == {"collation": "utf8mb4_bin"} + + +async def test_validate_session_id_collation_allows_mariadb_nopad() -> None: + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name="mysql", is_mariadb=True) + session = SQLAlchemySession("tenant ", engine=engine, create_tables=True) + conn = MagicMock() + conn.execute = AsyncMock(return_value=_ScalarResult("utf8mb4_nopad_bin")) + + await session._validate_session_id_collation(conn) + + assert conn.execute.await_count == 1 + + +@pytest.mark.parametrize("pad_attribute", ["NO PAD", None]) +async def test_validate_session_id_collation_allows_non_pad_or_unknown( + pad_attribute: str | None, +) -> None: + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name="mysql") + session = SQLAlchemySession("tenant ", engine=engine, create_tables=True) + conn = MagicMock() + if pad_attribute is None: + conn.execute = AsyncMock(return_value=_ScalarResult(None)) + else: + conn.execute = AsyncMock( + side_effect=[_ScalarResult("utf8mb4_0900_bin"), _ScalarResult(pad_attribute)] + ) + + await session._validate_session_id_collation(conn) + + +@pytest.mark.parametrize("dialect_name", ["sqlite", "postgresql"]) +async def test_validate_session_id_collation_skips_non_mysql_dialects( + dialect_name: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name=dialect_name) + monkeypatch.setattr(SQLAlchemySession, "_configure_sqlite_engine", MagicMock()) + session = SQLAlchemySession("tenant ", engine=engine, create_tables=True) + conn = MagicMock() + conn.execute = AsyncMock() + + await session._validate_session_id_collation(conn) + + conn.execute.assert_not_awaited() + + +async def test_create_tables_false_skips_session_id_collation_validation( + monkeypatch: pytest.MonkeyPatch, +) -> None: + session = SQLAlchemySession.from_url("tenant ", url=DB_URL, create_tables=False) + validate = AsyncMock() + monkeypatch.setattr(session, "_validate_session_id_collation", validate) + + try: + await session._ensure_tables() + finally: + await session.engine.dispose() + + validate.assert_not_awaited() async def test_session_ids_keep_trailing_spaces_on_sqlite(): From 4fb468bb9c2a3f1540627750567033c8cb1416a3 Mon Sep 17 00:00:00 2001 From: linhongyu510 Date: Wed, 9 Sep 2026 00:11:40 +0800 Subject: [PATCH 9/9] fix(sessions): validate reopened MySQL schemas --- .../extensions/memory/sqlalchemy_session.py | 37 ++++++++++++++---- .../memory/test_sqlalchemy_session.py | 39 +++++++++++++++++++ 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/agents/extensions/memory/sqlalchemy_session.py b/src/agents/extensions/memory/sqlalchemy_session.py index 482769e148..6be385e854 100644 --- a/src/agents/extensions/memory/sqlalchemy_session.py +++ b/src/agents/extensions/memory/sqlalchemy_session.py @@ -260,6 +260,7 @@ def __init__( self._session_factory = async_sessionmaker(self._engine, expire_on_commit=False) self._create_tables = create_tables + self._session_id_collation_validated = False # --------------------------------------------------------------------- # Convenience constructors @@ -328,14 +329,25 @@ async def _validate_session_id_collation(self, conn: AsyncConnection) -> None: if getattr(self._engine.dialect, "is_mariadb", False): pad_attribute = "NO PAD" if "_nopad_" in collation.casefold() else "PAD SPACE" else: - pad_result = await conn.execute( - sql_text( - "SELECT PAD_ATTRIBUTE FROM information_schema.COLLATIONS " - "WHERE COLLATION_NAME = :collation" - ), - {"collation": collation}, - ) - pad_attribute = pad_result.scalar_one_or_none() + try: + pad_result = await conn.execute( + sql_text( + "SELECT PAD_ATTRIBUTE FROM information_schema.COLLATIONS " + "WHERE COLLATION_NAME = :collation" + ), + {"collation": collation}, + ) + pad_attribute = pad_result.scalar_one_or_none() + except SQLAlchemyError: + version_result = await conn.execute(sql_text("SELECT VERSION()")) + version = version_result.scalar_one_or_none() + pad_attribute = ( + "PAD SPACE" + if version + and version.partition(".")[0].isdigit() + and int(version.partition(".")[0]) < 8 + else None + ) except SQLAlchemyError: return @@ -349,6 +361,14 @@ async def _validate_session_id_collation(self, conn: AsyncConnection) -> None: async def _ensure_tables(self) -> None: """Ensure tables are created before any database operations.""" if not self._create_tables: + if ( + not self._session_id_collation_validated + and self._engine.dialect.name in {"mysql", "mariadb"} + and self.session_id.endswith(" ") + ): + async with self._engine.connect() as conn: + await self._validate_session_id_collation(conn) + self._session_id_collation_validated = True return assert self._init_lock is not None @@ -363,6 +383,7 @@ async def _ensure_tables(self) -> None: async with self._engine.begin() as conn: await conn.run_sync(self._metadata.create_all) await self._validate_session_id_collation(conn) + self._session_id_collation_validated = True self._create_tables = False # Only create once finally: self._init_lock.release() diff --git a/tests/extensions/memory/test_sqlalchemy_session.py b/tests/extensions/memory/test_sqlalchemy_session.py index 04a802e412..eb7faf3932 100644 --- a/tests/extensions/memory/test_sqlalchemy_session.py +++ b/tests/extensions/memory/test_sqlalchemy_session.py @@ -21,6 +21,7 @@ ) from sqlalchemy import create_mock_engine, event, insert, select, text, update from sqlalchemy.dialects import postgresql, sqlite +from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine from sqlalchemy.sql import Select @@ -331,6 +332,25 @@ async def test_validate_session_id_collation_rejects_pad_space( assert conn.execute.await_args_list[1].args[1] == {"collation": "utf8mb4_bin"} +async def test_validate_session_id_collation_rejects_pad_space_on_mysql_57() -> None: + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name="mysql", is_mariadb=False) + session = SQLAlchemySession("tenant ", engine=engine, create_tables=True) + conn = MagicMock() + conn.execute = AsyncMock( + side_effect=[ + _ScalarResult("utf8mb4_bin"), + SQLAlchemyError("Unknown column PAD_ATTRIBUTE"), + _ScalarResult("5.7.44"), + ] + ) + + with pytest.raises(ValueError, match="PAD SPACE collation"): + await session._validate_session_id_collation(conn) + + assert conn.execute.await_count == 3 + + async def test_validate_session_id_collation_allows_mariadb_nopad() -> None: engine = MagicMock(spec=AsyncEngine) engine.dialect = SimpleNamespace(name="mysql", is_mariadb=True) @@ -393,6 +413,25 @@ async def test_create_tables_false_skips_session_id_collation_validation( validate.assert_not_awaited() +async def test_existing_mysql_schema_validates_trailing_space_session_id( + monkeypatch: pytest.MonkeyPatch, +) -> None: + engine = MagicMock(spec=AsyncEngine) + engine.dialect = SimpleNamespace(name="mysql", is_mariadb=False) + conn = MagicMock() + connection_context = MagicMock() + connection_context.__aenter__ = AsyncMock(return_value=conn) + connection_context.__aexit__ = AsyncMock(return_value=None) + engine.connect.return_value = connection_context + session = SQLAlchemySession("tenant ", engine=engine, create_tables=False) + validate = AsyncMock() + monkeypatch.setattr(session, "_validate_session_id_collation", validate) + + await session._ensure_tables() + + validate.assert_awaited_once_with(conn) + + async def test_session_ids_keep_trailing_spaces_on_sqlite(): """SQLite stores trailing-space IDs as distinct values.""" engine = create_async_engine(DB_URL)