fix(example): fix/data-agent-multi-session-inbox-routing - #3095
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Per-conversationId gate keys via a thread peer is the right fix for multiple chats collapsing into one session, and funneling both the currentSession probe and the real dispatch through buildConversationInbound removes the duplicated key construction that caused the drift. COMMENT because the DM-to-thread key change needs an explicit migration answer and one new message is provider-specific.
Findings
- [Critical]
ChatController.java:582— no-agent path changes the canonical key shape; pre-existing sessions will not resolve (silent context reset). - [Warning]
ChatController.java:565—DASHSCOPE_API_KEYtext hardcoded in a generic controller. - [Info]
ChatController.java:391— probe/dispatch equality is asserted nowhere. - [Info] test — blank-
conversationIdcase missing.
Suggestions
If pre-change sessions must keep working, fall back to the DM shape when conversationId is blank or resolve both keys on lookup; otherwise record the intentional break in the PR description.
Automated review by github-manager-bot
| if (agentId == null || agentId.isBlank()) { | ||
| // No agent override and no conversation scoping — pure binding-driven routing. | ||
| inbound = InboundMessage.dm(ChatUiChannel.CHANNEL_ID, userId, List.copyOf(msgs)); | ||
| inbound = buildConversationInbound(userId, null, conversationId, msgs); |
There was a problem hiding this comment.
This changes session identity for the no-agentId path, not just routing. Previously that branch built InboundMessage.dm(...), whose canonical key has no |t: segment; now the same user gets a thread-scoped key. Sessions registered under the old DM-shaped key stop matching, so an existing chat silently starts a fresh context after upgrade — and resolveGateKey now returns null when conversationId is blank, so currentSession reports exists=false. Is there a migration/back-compat path (keep the DM key when conversationId is blank, or match both keys on lookup)? If the dataagent example has no persisted users to migrate, worth stating in the PR description so reviewers can accept the break.
| List<Msg> payload = messages != null ? List.copyOf(messages) : List.of(); | ||
| InboundMessage.Builder builder = | ||
| InboundMessage.builder( | ||
| ChatUiChannel.CHANNEL_ID, Peer.thread(conversationId), payload) |
There was a problem hiding this comment.
Peer.thread(conversationId) + senderId(userId) + parentPeer(Peer.direct(userId)) is the kind of triple that breaks quietly if a router rule later keys off parentPeer. Routing both branches through buildConversationInbound is the right shape, but the test only asserts the key format, not that the dispatched inbound in executeChat yields the same canonicalKey as the resolveGateKey probe. One assertion tying probe and dispatch together would prevent exactly the drift that caused this bug.
| String userId, String agentId, String message, String conversationId) { | ||
| long startMs = System.currentTimeMillis(); | ||
|
|
||
| if (agentId != null && !agentId.isBlank()) { |
There was a problem hiding this comment.
The new ha.getModel() == null pre-check hardcodes provider-specific guidance (DASHSCOPE_API_KEY, dataagent.dashscope.api-key) into a generic controller, while HarnessAgent is model-agnostic across the OpenAI/Anthropic/Gemini/Ollama extensions — this message will mislead users on another provider. Consider a provider-neutral wording, and/or surfacing it where the model is actually resolved so all entry points benefit, not only executeChat.
| class ChatControllerConversationRoutingTest { | ||
|
|
||
| private static final ChatUiChannel CHANNEL = | ||
| ChatUiChannel.create( |
There was a problem hiding this comment.
Solid focused test. Two additions would lock the fix down: (1) blank/null conversationId on the no-agent path — asserting the resulting key documents the migration decision above; (2) a regression that two turns with the same conversationId map to the same key while a different one does not, which is the user-visible invariant.
00eddcd to
e3f8c90
Compare
Thanks for the thorough review. I have addressed all feedback in the latest push. [Critical] DM‑to‑thread key change & migration For the defensive fallback path: when [Warning] Hard‑coded DASHSCOPE message [Info] probe / dispatch canonical‑key equality [Info] same‑conversation‑id invariant & blank‑id handling For this example module, I do not think strict backward‑compatibility for pre‑fix persisted sessions (e.g. implementing dual‑key lookup within |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds per-conversation (|t: thread) session scoping to the data-agent chat controller via a shared buildConversationInbound(...) helper, plus a routing test that pins distinct gate keys per conversation and stable keys for the same conversation. The refactor is a clear improvement: probe and dispatch now go through one builder, so the canonical key they compare cannot drift apart the way the previous hand-rolled accountId(conversationId) probe allowed, and the new tests assert exactly that invariant.
One consistency gap is worth resolving before merge: the blank-conversation fallback in executeChat and the new early return in resolveGateKey no longer agree.
Findings
- [Warning]
ChatController.java:368—resolveGateKeynow returnsnullfor a blank conversation id, whileexecuteChat(line 581) still dispatches for a blank id using the pre-thread DM shape. On that path a session is created but nothing can resolve its key:/resetreports "No active session to reset.",currentSessionreturnsexists=false, andrecordRunSessiondrops itsRUN_SESSIONactivity event. Before this change the two sides agreed, because the oldPeer.direct+accountId(...)probe produced the same DM-shaped key. See the inline comment for the two ways to close it. - [Info]
ChatController.java:392— thesenderIdrequirement for thread peers is well documented, but only indirectly protected; an assertion on the builtInboundMessage'ssenderId/parentPeerwould pin the constraint itself rather than just the resulting key.
Good bits
- Sharing one builder between the probe and the dispatch path removes the class of bug this file has had repeatedly, and
probeAndDispatchInboundShareCanonicalKeylocks that in. sameUserDifferentConversationsShareRoomButNotThreadis the right assertion to have for a multi-session inbox: it documents that|r:<user>stays shared while the thread segment separates conversations.- The fail-fast when a running agent has no configured
Modelturns an opaque NPE deep in a ReAct loop into an actionable message namingapplication.yml.
Tests
ChatControllerConversationRoutingTest covers the non-blank matrix well (distinct keys, stable keys, room/thread separation, back-compat DM shape). The uncovered case is the one flagged above: probe/dispatch agreement for a blank conversation id.
Automated review by github-manager-bot
| */ | ||
| private String resolveGateKey(String userId, String agentId, String conversationId) { | ||
| if (agentId == null || agentId.isBlank()) return null; | ||
| if (conversationId == null || conversationId.isBlank()) return null; |
There was a problem hiding this comment.
[Warning] This new guard makes resolveGateKey return null for a blank conversation id, but executeChat (line 581) still dispatches for a blank id by falling back to the pre-thread DM shape. The two sides now disagree about which session a request belongs to.
Consequence on that path: the message is dispatched and a session is created with a |r:<user>-style gate key, but nothing can resolve that key any more — /reset answers "No active session to reset.", the currentSession probe reports exists=false, and recordRunSession's dedupeKey is null so the RUN_SESSION activity event is silently dropped.
Before this change the two sides agreed, because resolveGateKey built a Peer.direct + accountId(conversationId) probe and produced exactly the same DM-shaped key that InboundMessage.dm(...) does.
The HTTP entry points mint a UUID (lines 164-166, 277-279), so I could not confirm a reachable caller that arrives here with a blank id — but the comment on the fallback branch says it exists for back-compat, which implies it is expected to be live. Please pick one:
- mirror the fallback here — build the DM probe when the conversation id is blank instead of returning
null; or - if a blank id really cannot reach
executeChat, delete the branch at line 581 and make the invariant explicit, so the two paths cannot drift again.
ChatControllerConversationRoutingTest.probeAndDispatchInboundShareCanonicalKey asserts probe/dispatch agreement only for a non-blank conv-a; a blank-id case would pin whichever option you choose.
| InboundMessage.Builder builder = | ||
| InboundMessage.builder( | ||
| ChatUiChannel.CHANNEL_ID, Peer.thread(conversationId), payload) | ||
| .senderId(userId) |
There was a problem hiding this comment.
[Info] Setting senderId here because thread peers are not DM peers is a genuinely subtle routing constraint, and the javadoc explains it well.
It is only protected by the canonical-key assertions, though: if a future edit dropped .senderId(userId), gateKey would likely stay stable and these tests would keep passing while the gateway silently lost the authenticated user id on the thread path. An assertion on the built InboundMessage itself (getSenderId(), getParentPeer()) would pin the constraint the comment describes.
As the HTTP entry point already emits a UUID automatically, removed the compatibility logic that permits message sending without an ID. Treat messages missing an ID as invalid / unexpected events for a cleaner codebase. |
e3f8c90 to
d074b3a
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after the update (new head d074b3a5). Root-cause fix is correct: conversations are now routed as thread peers (|t:<conversationId>) instead of a DM-shaped key with accountId, so multiple chats per user/agent no longer overwrite each other in the inbox. Probe (resolveGateKey) and dispatch now share one builder, eliminating the key-divergence class of bugs, and blank ids are rejected up front — I verified both stream() and send() mint a UUID before executeChat, and every resolveGateKey caller already handles a null gateKey, so there is no regression for existing callers. New routing tests cover key distinctness, stability, and sender/parent-peer shape. CLA signed.
Verdict: COMMENT — leaving this non-approving pending maintainer confirmation of the one gateKey-shape migration consideration inline; everything else looks good to go.
Automated review by github-manager-bot
| // No agent override and no conversation scoping — pure binding-driven routing. | ||
| inbound = InboundMessage.dm(ChatUiChannel.CHANNEL_ID, userId, List.copyOf(msgs)); | ||
| inbound = buildConversationInbound(userId, null, pinnedConversationId, msgs); | ||
| } else { |
There was a problem hiding this comment.
[Warning] The dispatch shape change from Peer.direct(userId) + accountId(conversationId) to Peer.thread(conversationId) + parentPeer(direct) changes the canonical gateKey (|t: segment replaces the DM-shaped key). For the example's own new-session flow this is exactly the fix (distinct conversations no longer collapse), but for a deployed DataAgent with pre-existing MAIN sessions keyed under the old DM-shaped gateKey, the first message after upgrade will resolve to a new gateKey — history/listen-by-gate lookups (findSessionKeyByGate, /reset, tool-event bus subscription at the first turn) will miss the old session until it ages out. If that migration blind-spot matters for users running the example against persisted state, consider either (a) a fallback lookup on the legacy key shape during resolveGateKey, or (b) an explicit note in the PR description / release note that existing chat sessions start fresh after upgrade.
| long startMs = System.currentTimeMillis(); | ||
|
|
||
| if (agentId != null && !agentId.isBlank()) { | ||
| HarnessAgent ha = catalogService.getRunningAgent(userId, agentId); |
There was a problem hiding this comment.
[Info] Good UX improvement turning the missing-model condition into an actionable error instead of a downstream failure. Minor: this is a check-then-act on catalogService.getRunningAgent — the agent can be stopped or reconfigured between here and chatUiChannel.dispatch, so the guard is best-effort only (dispatch still needs its own error path, which it has). Also consider emitting the same IllegalStateException text from the stream() SSE error frame so the frontend shows the hint rather than a generic stream error — quick check whether the existing error mapping already covers it.
2 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
Background
Fixes #1626
After the data-agent is launched locally, creating a second chat session causes the new session to replace the previous one in the left history sidebar, so only one session ever appears. Expected behavior: multiple independent sessions for the same user and agent should coexist in the inbox.
New conversations keep overwriting old ones, leaving only a single session visible on the left sidebar：
Root causes
Every new conversation created on the frontend generates a distinct
conversationId. However, the backend route does not incorporate this value into the session identifier. As a result, all conversations are mapped to the same backend session, which explains why the inbox only returns a single entry.conversationId, so the routing layer cannot distinguish individual conversationsRepair ideas
conversationIdinto the routing key (|t:xxx), so that each new conversation maps to a dedicated backend session.DASHSCOPE_API_KEYconfiguration.Changes
ChatControllerbuildConversationInbound()to construct inbound messages, and perform routing based on conversation‑IDChatControllerChatControllerConversationRoutingTestconversationIdresolves to differentgateKeyafter fix：
How to test
Run unit tests:
mvn -pl agentscope-examples/agents/agentscope-dataagent -am \ test -Dtest=ChatControllerConversationRoutingTest \ -Dfrontend-maven-plugin.skip=trueExpected:
BUILD SUCCESS.Manual verification
‑ Configure
DASHSCOPE_API_KEYand start the Data Agent‑ Send messages in Session A → one entry appears in the sidebar
‑ Click "New Conversation", send messages in Session B → two entries show up in the sidebar
‑ Switch back to Session A and refresh the page → both entries remain, conversation history works as expected
Checklist
mvn spotless:apply（如需要）ChatControllerConversationRoutingTest，BUILD SUCCESS）