Copilot commented on code in PR #816:
URL:
https://github.com/apache/rocketmq-dashboard/pull/816#discussion_r3702368140
##########
server/src/main/java/org/apache/rocketmq/studio/ops/ai/OpenAiCompatibleLlmClient.java:
##########
@@ -208,19 +217,40 @@ private HttpRequest request(LlmConfigVO config, String
accept, Map<String, Objec
return builder.build();
}
- private Map<String, Object> requestBody(LlmConfigVO config, String prompt,
String modelOverride,
+ private Map<String, Object> requestBody(LlmConfigVO config,
List<LlmChatMessage> messages, String modelOverride,
boolean stream) {
Map<String, Object> body = new LinkedHashMap<>();
body.put("model", StringUtils.hasText(modelOverride) ?
modelOverride.trim() : config.getModel().trim());
- body.put("messages", List.of(Map.of(
- "role", "user",
- "content", StringUtils.hasText(prompt) ? prompt.trim() : "")));
+ body.put("messages", normalizeMessages(messages));
body.put("temperature", config.getTemperature());
body.put("max_tokens", config.getMaxTokens());
body.put("stream", stream);
return body;
}
+ private List<Map<String, String>> normalizeMessages(List<LlmChatMessage>
messages) {
+ List<LlmChatMessage> normalizedMessages = messages == null ||
messages.isEmpty()
+ ? List.of(LlmChatMessage.user(""))
+ : messages;
+ return normalizedMessages.stream()
+ .map(message -> Map.of(
+ "role", normalizeRole(message),
+ "content", normalizeMessage(message == null ? null :
message.content())))
+ .toList();
+ }
+
+ private String normalizeRole(LlmChatMessage message) {
+ if (message == null || !StringUtils.hasText(message.role())) {
+ return "user";
+ }
+ String role = message.role().trim();
+ return "assistant".equals(role) ? "assistant" : "user";
+ }
Review Comment:
`normalizeRole` currently collapses any non-"assistant" role to "user". For
an OpenAI-compatible chat-completions payload, this breaks support for standard
roles like "system" (and potentially future roles), and can silently change
semantics if callers ever pass such messages via the new `List<LlmChatMessage>`
API. It’s safer to preserve known roles (at least `system`, `user`,
`assistant`) and only default unknown/blank roles to `user`.
##########
server/src/main/java/org/apache/rocketmq/studio/ops/ai/LlmConversationMemory.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.rocketmq.studio.ops.ai;
+
+import org.springframework.stereotype.Component;
+import org.springframework.util.StringUtils;
+
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Deque;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+@Component
+public class LlmConversationMemory {
+
+ private static final int MAX_CONVERSATIONS = 200;
+ private static final int MAX_MESSAGES_PER_CONVERSATION = 12;
+ private static final int MAX_MESSAGE_CHARS = 8_000;
+
+ private final Map<String, Deque<LlmChatMessage>> conversations =
+ new LinkedHashMap<>(16, 0.75f, true);
+
+ public synchronized List<LlmChatMessage> appendUserAndSnapshot(String
conversationId, String message) {
+ LlmChatMessage userMessage =
LlmChatMessage.user(normalizeContent(message));
+ String id = normalizeConversationId(conversationId);
+ if (!StringUtils.hasText(id)) {
+ return List.of(userMessage);
+ }
+ Deque<LlmChatMessage> messages = conversations.computeIfAbsent(id,
ignored -> new ArrayDeque<>());
Review Comment:
`LlmConversationMemory` is a singleton Spring `@Component` and the storage
key is only the client-provided `conversationId`. This makes it possible for
different users/sessions to collide or intentionally spoof the same
`conversationId`, causing cross-user conversation context to be injected into
the provider prompt (privacy / data-leak risk). Consider scoping the key by
authenticated user/session (e.g., `${principalId}:${conversationId}`) or
generating server-side conversation IDs instead of trusting a caller-supplied
ID.
##########
server/src/main/java/org/apache/rocketmq/studio/ops/ai/OpenAiCompatibleLlmGateway.java:
##########
@@ -52,8 +54,11 @@ public SseEmitter chat(ChatDTO request) {
return errorEmitter(unsupportedProviderException());
}
+ String conversationId = request == null ? null :
request.getConversationId();
+ String message = request == null ? null : request.getMessage();
+ List<LlmChatMessage> messages =
conversationMemory.appendUserAndSnapshot(conversationId, message);
SseEmitter emitter = new SseEmitter(60_000L);
- executor.execute(() -> streamChat(request, config, emitter));
+ executor.execute(() -> streamChat(request, config, emitter, messages));
Review Comment:
Conversation state is mutated before the provider call
(`appendUserAndSnapshot`), and the assistant turn is appended only after
streaming completes. If two requests for the same `conversationId` are
in-flight concurrently, the second user turn can be appended before the first
assistant turn, resulting in mis-ordered history (assistant answer attached
after a later user question). Also, failed streams leave an unmatched user turn
in memory, so a retry may duplicate the user message in context. Consider
enforcing single in-flight request per conversationId or appending/committing
turns atomically (e.g., buffer per-request and only commit on success).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
