Skip to content

Resolve session-store method checks against the instance - #1215

Open
shashvat-singham wants to merge 3 commits into
anthropics:mainfrom
shashvat-singham:fix/store-implements-instance-attribute
Open

Resolve session-store method checks against the instance#1215
shashvat-singham wants to merge 3 commits into
anthropics:mainfrom
shashvat-singham:fix/store-implements-instance-attribute

Conversation

@shashvat-singham

@shashvat-singham shashvat-singham commented Aug 16, 2026

Copy link
Copy Markdown

Stacked on #1214. GitHub diffs this against main, so message_parser.py / test_message_parser.py show up here too — those belong to #1214. This PR is only the session_store_validation.py change and its test. Merging #1214 first makes this diff single-file.

Problem

_store_implements resolves the method on type(store), so it only recognises class-level definitions. But SessionStore is a structural Protocol — an implementation assigned on the instance satisfies it just as well, and calling it works fine at runtime. Those stores are nonetheless rejected during pre-flight validation:

class DelegatingStore(SessionStore):
    def __init__(self, inner: SessionStore) -> None:
        self.list_sessions = inner.list_sessions
    ...

validate_session_store_options(
    ClaudeAgentOptions(session_store=DelegatingStore(inner), continue_conversation=True)
)
# ValueError: continue_conversation with session_store requires the store to
# implement list_sessions()

Verified against main, three shapes all wrongly reported as not implementing it:

store _store_implements should be
class-level async def list_sessions True True
assigned in __init__ (delegation) False True
patched with AsyncMock False True
genuinely missing False False

The AsyncMock row is probably the most likely way to meet this in practice — someone stubbing a store in their own tests gets a ValueError that only appears when continue_conversation=True, pointing at a method their double clearly has.

Change

Look the attribute up on the instance and compare the underlying function against the Protocol default:

impl = getattr(store, method, None)
if impl is None:
    return False
default = getattr(SessionStore, method, None)
return getattr(impl, "__func__", impl) is not default

A bound method is still matched against the Protocol default via __func__, so an unimplemented store is still detected; anything that isn't a bound method (a plain callable assigned on the instance) is compared directly and is never the default. The getattr(impl, ...) also makes the existing impl is None guard meaningful — previously impl was computed and then not used for the decision.

Confirmed the negative case still works: a store without list_sessions is still rejected, and test_continue_conversation_requires_list_sessions still passes.

Tests

Added test_continue_conversation_ok_when_list_sessions_set_on_instance, which fails on main and passes with the change.

$ pytest tests/test_session_store_conformance.py -q
8 failed, 22 passed

The 8 failures are all the [trio] parametrisations and reproduce identically on an unmodified tree here (no trio backend in my env) — unrelated to this change; the [asyncio] side is green.

parse_message wraps malformed input in MessageParseError -- non-dict
data, a missing type, missing required fields all get the parser's own
error type. But a "message" field that is not a dict escaped as a bare
TypeError from indexing into it:

    parse_message({"type": "user", "message": "hi"})
    # TypeError: string indices must be integers, not 'str'

Same for the assistant branch. The existing handlers only catch
KeyError, so TypeError/AttributeError from indexing a non-dict fell
through, and a single malformed line from the CLI stream would surface
as an unrelated-looking TypeError instead of the documented parse error.

Catch TypeError/AttributeError alongside KeyError in both branches and
raise MessageParseError with the offending data attached, like every
other malformation.
_store_implements looked the method up on type(store), so it only saw
class-level definitions. SessionStore is a structural Protocol, though,
so an implementation assigned on the instance satisfies it just as well
-- and those stores were rejected before the subprocess even spawned:

    class DelegatingStore(SessionStore):
        def __init__(self, inner):
            self.list_sessions = inner.list_sessions

    validate_session_store_options(
        ClaudeAgentOptions(session_store=DelegatingStore(inner),
                           continue_conversation=True)
    )
    # ValueError: continue_conversation with session_store requires the
    # store to implement list_sessions()

even though calling list_sessions() on that store works fine. The same
applies to a store whose method is a functools.partial, and to a test
double patched with AsyncMock -- arguably the most common way to hit
this, since it fails only under continue_conversation.

Look the attribute up on the instance and compare the underlying
function against the Protocol default, so a bound method is still
matched against the default while a plain callable assigned on the
instance counts as an implementation.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instance-level check now accepts any non-None attribute, not necessarily a callable. A store with self.list_sessions = "disabled" passes validation and then fails later when the SDK tries to call it. Please require callable(impl) before comparing it with the Protocol default and add a non-callable instance-attribute regression case.

@shashvat-singham

Copy link
Copy Markdown
Author

You're right — resolving against the instance widened the check past callables, and the failure it lets through is exactly the one this pre-flight validation exists to catch. Fixed in 1d76136.

impl = getattr(store, method, None)
# A non-callable attribute is not an implementation: accepting it would
# defer the failure to the call site mid-session, which is exactly what
# this pre-flight check exists to prevent.
if not callable(impl):
    return False

callable() subsumes the old impl is None guard (None isn't callable), so the negative case is unchanged, and every shape the PR set out to accept is still accepted — a bound method, an implementation assigned in __init__, an AsyncMock — since all of them are callable.

Regression test added as test_non_callable_instance_attribute_is_not_an_implementation, using your exact example:

class DisabledStore(SessionStore):
    def __init__(self) -> None:
        self.list_sessions = "disabled"
    ...

with pytest.raises(ValueError, match="list_sessions"):
    validate_session_store_options(
        ClaudeAgentOptions(session_store=DisabledStore(), continue_conversation=True)
    )

It fails on the previous commit with Failed: DID NOT RAISE <class 'ValueError'>.

$ pytest tests/test_session_store_conformance.py::TestSessionStoreOptionsValidation -q
8 passed

ruff check / ruff format --check on src/ tests/ scripts/ clean, mypy clean on the changed file. (The [trio] parametrisations elsewhere in that file fail identically before and after here — no trio backend in my env.)

Resolving against the instance widened the check past callables: a
non-callable attribute (self.list_sessions = "disabled") passed
validation and then failed at the call site mid-session, which is
what this pre-flight check exists to prevent.

Guard with callable(impl) before comparing against the Protocol
default, and cover the non-callable instance attribute.
@shashvat-singham
shashvat-singham force-pushed the fix/store-implements-instance-attribute branch from 1d76136 to cbd459f Compare September 4, 2026 16:07
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.

2 participants