Resolve session-store method checks against the instance - #1215
Resolve session-store method checks against the instance#1215shashvat-singham wants to merge 3 commits into
Conversation
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.
bee52ef to
ec51e7c
Compare
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
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.
|
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
Regression test added as 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
|
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.
1d76136 to
cbd459f
Compare
Problem
_store_implementsresolves the method ontype(store), so it only recognises class-level definitions. ButSessionStoreis a structuralProtocol— 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:Verified against
main, three shapes all wrongly reported as not implementing it:_store_implementsasync def list_sessionsTrueTrue__init__(delegation)FalseTrueAsyncMockFalseTrueFalseFalseThe
AsyncMockrow is probably the most likely way to meet this in practice — someone stubbing a store in their own tests gets aValueErrorthat only appears whencontinue_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:
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. Thegetattr(impl, ...)also makes the existingimpl is Noneguard meaningful — previouslyimplwas computed and then not used for the decision.Confirmed the negative case still works: a store without
list_sessionsis still rejected, andtest_continue_conversation_requires_list_sessionsstill passes.Tests
Added
test_continue_conversation_ok_when_list_sessions_set_on_instance, which fails onmainand passes with the change.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.