Skip to content

fix(memory): skip legacy non-string rows in AsyncSQLiteSession.get_items - #4302

Closed
hsusul wants to merge 1 commit into
openai:mainfrom
hsusul:fix/async-sqlite-get-items-typeerror
Closed

fix(memory): skip legacy non-string rows in AsyncSQLiteSession.get_items#4302
hsusul wants to merge 1 commit into
openai:mainfrom
hsusul:fix/async-sqlite-get-items-typeerror

Conversation

@hsusul

@hsusul hsusul commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

AsyncSQLiteSession.get_items decodes each stored row with json.loads inside a try/except json.JSONDecodeError, so it only skips rows that fail to parse as JSON. Every other decode site in the session layer also skips rows whose json.loads raises TypeError — i.e. rows whose message_data is a non-string (most commonly NULL):

  • AsyncSQLiteSession.pop_itemexcept (json.JSONDecodeError, TypeError)
  • sync SQLiteSession.get_items and .pop_itemexcept (json.JSONDecodeError, TypeError)
  • MongoDBSession.get_itemsexcept (json.JSONDecodeError, TypeError) ("Skip corrupted or malformed entries, including legacy non-string values.")

Because async get_items was the lone exception, a single non-string row made the entire read raise
TypeError: the JSON object must be str, bytes or bytearray, not NoneType instead of skipping that row.
db_path, sessions_table, and messages_table are public constructor arguments, so a session can be pointed at a database/table created by an older schema or external tooling whose message_data column is nullable — exactly the "legacy non-string values" case the sibling backends already guard. This also makes get_items and pop_item disagree on the same data: pop_item quietly drops the row while get_items crashes.

The fix widens the get_items decode guard to (json.JSONDecodeError, TypeError), matching pop_item and the other backends. Both the unlimited read and the limited (window-expanding) read share the same _decode_rows helper, so both paths are covered.

Test plan

New regression test test_async_sqlite_session_get_items_skips_non_string_rows seeds a legacy-shaped (nullable message_data) table with a NULL row among valid rows and asserts both get_items() and get_items(limit=1) skip it and return the valid items.

  • Fails on main (raises TypeError), passes with this change. Verified by reverting only the src change and re-running: the read raises TypeError.
  • uv run pytest tests/extensions/memory/test_async_sqlite_session.py → 40 passed
  • uv run pytest tests/memory tests/extensions/memory/test_async_sqlite_session.py tests/extensions/memory/test_advanced_sqlite_session.py → 232 passed, 1 skipped
  • uv run ruff check (changed files) → All checks passed
  • uv run ruff format --check (changed files) → already formatted
  • uv run pyright (changed files) → 0 errors, 0 warnings
  • git diff --check → clean

Not run: the full tests/extensions/memory suite requires optional extras (sqlalchemy, etc.) that are not installed in this minimal environment; those modules are unrelated to this change (SQLite-only fix).

Issue number

N/A — no issue filed; small correctness fix consistent with the sibling backends.

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@seratch

seratch commented Aug 8, 2026

Copy link
Copy Markdown
Member

I confirmed that a manually created table with nullable message_data can make AsyncSQLiteSession.get_items() raise TypeError, and that this catch would skip the row.

However, AsyncSQLiteSession has defined message_data as TEXT NOT NULL since it was introduced, and SDK writes always serialize items as JSON strings. The PR does not identify an earlier SDK schema, supported migration path, or real user scenario that can produce the tested NULL row. The configurable database path and table names do not by themselves make arbitrary external table schemas part of the supported contract.

I am going to close this PR for now.

@seratch seratch closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants