feat(agent): route load-session through a no-op session store port#4768
feat(agent): route load-session through a no-op session store port#4768mmabrouk wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
size:M
Reviewer guide: interesting code
|
| messages: Sequence[Message], | ||
| result: Optional[AgentResult] = None, | ||
| ) -> None: | ||
| """Persist one completed cold turn.""" |
There was a problem hiding this comment.
This is the contract being frozen ahead of any real store. Confirm save_turn taking messages plus an optional AgentResult fits a file-backed or platform-backed adapter without a later signature break.
| make_not_acceptable_response: Callable[[str, Any], Response], | ||
| make_stream_response: Callable[[WorkflowStreamingResponse, str], Response], | ||
| handle_failure: Callable[[Exception], Any], | ||
| session_store: Optional[SessionStore] = None, |
There was a problem hiding this comment.
The msg-{idx} ids restart at 1 per response, so they are not stable across load-session calls. Confirm no client treats these ids as durable.
| session_store: Optional[SessionStore] = None, | ||
| ): | ||
| """Build the v1 ``POST /load-session`` endpoint over the session-store port.""" | ||
| store = session_store or NoopSessionStore() |
There was a problem hiding this comment.
The or NoopSessionStore() default is what keeps the response identical to the old hardcoded stub when no adapter is wired. Worth confirming this is the only place the default is chosen.
| response = LoadSessionResponse( | ||
| session_id=request.session_id, | ||
| messages=[ | ||
| message_to_vercel_ui_message(message, message_id=f"msg-{idx}") |
There was a problem hiding this comment.
Loaded neutral messages convert back to Vercel UIMessage here, the inverse of the inbound vercel_ui_messages_to_messages on /messages. Confirm the two directions stay symmetric so a saved turn round-trips.
|
Superseded. Replacing the path-based stack with PRs sliced by functional area showing final code only, so reviewers don't comment on intermediate scaffolding that a later PR rewrites. See the new set. |
1 participant
This PR is part of a stack. Review bottom-up.
Each PR's diff is only its own delta. Merge from the bottom. This PR's base is #4767 (merge that first).
Context
This PR routes the agent
POST /load-sessionendpoint through a newSessionStoreport instead of returning a hardcoded empty stub. It sits onfix/agent-session-id-tracingand is slice #5 (cold session persistence) indocs/design/agent-workflows/pr-stack.md: the port-only seam.The cold runtime still receives the full message history on every turn. This PR does not change that. It adds the seam where a server-owned history store will later attach, and it locks the load-session response shape before any storage lands.
What this changes
make_load_session_endpointused to build a fixed response. It now takes an optionalsession_storeand callsstore.load(session_id), then converts each neutralMessageback into a VercelUIMessageat the adapter boundary.The default store is
NoopSessionStore. Itsloadreturns()and itssave_turndiscards. So with no real adapter wired, the response is identical to before:Before (hardcoded stub):
{"session_id": "sess_abc", "messages": []}After (NoopSessionStore, same output):
{"session_id": "sess_abc", "messages": []}With a real store that returns one user
Message, the same path now produces:{"session_id": "sess_abc", "messages": [ {"id": "msg-1", "role": "user", "parts": [{"type": "text", "text": "hello"}]} ]}Key architectural decision to review
Land the persistence seam now with no real adapter.
SessionStoreandNoopSessionStorelive insdks/python/agenta/sdk/agents/interfaces.py. The tradeoff: the API shape of/load-sessionand theload/save_turncontract get pinned and exported now, while actual storage ships in a later slice. The runtime keeps sending full history every turn, so nothing depends on the store for correctness yet. The risk is over-fitting the port to a store that does not exist. Check thatloadreturning aSequence[Message]andsave_turntakingmessagesplus an optionalAgentResultwill fit a real platform-backed or file-backed adapter without a signature break.The second thing to scrutinize is the conversion direction at the boundary in
routing.py. Loaded neutralMessageobjects flow back out throughmessage_to_vercel_ui_message, which is the inverse of the inboundvercel_ui_messages_to_messageson/messages. The store speaks neutral messages; the wire speaks VercelUIMessage. Confirm those two directions stay symmetric so a saved turn round-trips.How to review this PR
sdks/python/agenta/sdk/agents/interfaces.py. Read theSessionStoreABC andNoopSessionStore. Check the method signatures are the contract you want frozen.sdks/python/agenta/sdk/agents/adapters/vercel/routing.py,make_load_session_endpoint. Confirm thesession_store or NoopSessionStore()default preserves old behavior, and thatregister_agent_message_routesthreads the optionalsession_storethrough.__init__.pyexports forSessionStoreandNoopSessionStore.msg-{idx}ids start at 1 per response, so they are not stable across calls. Check that no consumer treats them as durable.Tests / notes
test_load_session_uses_session_store_portinjects a fake store, assertsloadis called with the session id, and asserts the response renders one user message as a VercelUIMessage. The existingtest_load_session_returns_stub_historystill passes against the defaultNoopSessionStore, which proves the no-op path is unchanged.