-
Notifications
You must be signed in to change notification settings - Fork 8
FIX: Auto-detect chat intent server-side; remove user-facing selector #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c5b6ccc
360f4f0
4454819
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import json | ||
| import logging | ||
| from typing import Any | ||
| from typing import Any, Optional | ||
|
|
||
| import eventlet | ||
| import redis | ||
|
|
@@ -52,12 +52,47 @@ def create_redis_xgroup(self, channel_id, group_id): | |
| else: | ||
| raise | ||
|
|
||
| def _resolve_chat_intent(self, chat_message_id: str, data: dict, content: Any) -> Optional[str]: | ||
| """ | ||
| Pull the AI-detected chat_intent out of the inbound payload and persist it | ||
| to ChatMessage.chat_intent the first time we see it for a given message. | ||
| Subsequent events re-use the cached value. | ||
| """ | ||
| if not hasattr(self, "_chat_intent_by_msg"): | ||
| self._chat_intent_by_msg = {} | ||
|
|
||
| cached = self._chat_intent_by_msg.get(chat_message_id) | ||
| if cached: | ||
| return cached | ||
|
|
||
| intent_name = data.get("chat_intent") | ||
| if not intent_name and isinstance(content, dict): | ||
| intent_name = content.get("chat_intent") | ||
| if not intent_name: | ||
| return None | ||
|
|
||
| try: | ||
| from backend.core.models.chat_intent import ChatIntent | ||
| from backend.core.models.chat_message import ChatMessage | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These imports run on every stream event (the helper is invoked from |
||
| chat_intent = ChatIntent.objects.get(name=intent_name) | ||
| chat_message = ChatMessage.objects.get(chat_message_id=chat_message_id) | ||
| chat_message.chat_intent = chat_intent | ||
| chat_message.save(update_fields=["chat_intent"]) | ||
| # Mirror the pre-PR behavior where Chat.chat_intent tracked the | ||
| # latest message's intent (used by PastConversations badge). | ||
| chat_message.chat.chat_intent = chat_intent | ||
| chat_message.chat.save(update_fields=["chat_intent"]) | ||
| except Exception as e: | ||
| logging.error(f"Failed to persist chat_intent={intent_name}: {e}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bare This catches See companion comment on the line below for the cache-write-outside-try issue, which compounds this. |
||
|
|
||
| self._chat_intent_by_msg[chat_message_id] = intent_name | ||
|
greptile-apps[bot] marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cache write outside the If the Move the cache write into the Suggested shape: try:
chat_intent_obj = ChatIntent.objects.get(name=intent_name)
except ChatIntent.DoesNotExist:
logging.error(f"Unknown chat_intent name from AI: {intent_name!r}")
return None # don't cache; let a later event try again
try:
with transaction.atomic():
ChatMessage.objects.filter(chat_message_id=chat_message_id) \
.update(chat_intent=chat_intent_obj)
Chat.objects.filter(chat_messages__chat_message_id=chat_message_id) \
.update(chat_intent=chat_intent_obj)
except DatabaseError:
logging.exception("Failed to persist chat_intent")
return None
self._chat_intent_by_msg[chat_message_id] = intent_name
return intent_name |
||
| return intent_name | ||
|
|
||
| def process_message( | ||
| self, | ||
| sid: str, | ||
| channel_id: str, | ||
| chat_id: str, | ||
| chat_intent: str, | ||
| payload: dict[str, Any], | ||
| discussion_status: str | ||
| ): | ||
|
|
@@ -75,6 +110,7 @@ def process_message( | |
| 2: "summary", | ||
| 3: "chat_name", | ||
| 4: "completed", | ||
| 5: "chat_intent", | ||
| 99: "stop", | ||
| } | ||
|
|
||
|
|
@@ -83,6 +119,8 @@ def process_message( | |
| chat_message_id = data["chat_message_id"] | ||
| content = data["content"] | ||
|
|
||
| chat_intent = self._resolve_chat_intent(chat_message_id, data, content) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retry no longer signals "this is a retry" to the AI gatekeeper. Pre-PR, the Second, related concern: when the gatekeeper does classify the retry, the new Mitigations (pick one):
|
||
|
|
||
| if event_type == "chat_name": | ||
| self.chat_name = data["content"] | ||
| self.persist_response( | ||
|
|
@@ -120,7 +158,7 @@ def _validate_message(self, group_id, channel_id): | |
| ) | ||
| return messages | ||
|
|
||
| def _handle_redis_message(self, sid, channel_id, chat_id, chat_intent, group_id, messages, discussion_status: str): | ||
| def _handle_redis_message(self, sid, channel_id, chat_id, group_id, messages, discussion_status: str): | ||
| for _, msg_list in messages: | ||
| for message_id, payload in msg_list: | ||
| logging.info(f" === Message ID: {message_id} ===") | ||
|
|
@@ -129,7 +167,6 @@ def _handle_redis_message(self, sid, channel_id, chat_id, chat_intent, group_id, | |
| sid=sid, | ||
| channel_id=channel_id, | ||
| chat_id=chat_id, | ||
| chat_intent=chat_intent, | ||
| payload=payload, | ||
| discussion_status=discussion_status | ||
| ) | ||
|
|
@@ -138,7 +175,7 @@ def _handle_redis_message(self, sid, channel_id, chat_id, chat_intent, group_id, | |
| self.redis_client.xack(channel_id, group_id, message_id) | ||
|
|
||
| def __stream_listener( | ||
| self, sid: str, channel_id: str, chat_id: str, chat_message_id: str, chat_intent: str, group_id: str, discussion_status: str | ||
| self, sid: str, channel_id: str, chat_id: str, chat_message_id: str, group_id: str, discussion_status: str | ||
| ): | ||
|
|
||
| while True: | ||
|
|
@@ -148,7 +185,7 @@ def __stream_listener( | |
| if not messages: | ||
| continue | ||
|
|
||
| self._handle_redis_message(sid, channel_id, chat_id, chat_intent, group_id, messages, discussion_status) | ||
| self._handle_redis_message(sid, channel_id, chat_id, group_id, messages, discussion_status) | ||
|
|
||
| except redis.exceptions.RedisError as e: | ||
| logging.error(f"[REDIS ERROR] {e}") | ||
|
|
@@ -196,15 +233,15 @@ def __stream_listener( | |
| ) | ||
| break | ||
|
|
||
| def listen_to_redis_stream(self, sid: str, channel_id: str, chat_id: str, chat_message_id: str, chat_intent: str, discussion_status: str): | ||
| def listen_to_redis_stream(self, sid: str, channel_id: str, chat_id: str, chat_message_id: str, discussion_status: str): | ||
| """Listens to the Redis stream from llm server and processes the messages.""" | ||
| group_id = f"group_{chat_id}_{chat_message_id}" | ||
| self.create_redis_xgroup(channel_id, group_id) | ||
| self.__stream_listener(sid, channel_id, chat_id, chat_message_id, chat_intent, group_id, discussion_status) | ||
| self.__stream_listener(sid, channel_id, chat_id, chat_message_id, group_id, discussion_status) | ||
|
|
||
| def stream_prompt_response(self, sid: str, channel_id: str, chat_id: str, chat_message_id: str, chat_intent: str, discussion_status: str): | ||
| def stream_prompt_response(self, sid: str, channel_id: str, chat_id: str, chat_message_id: str, discussion_status: str): | ||
| """Starts a background thread to listen redis pubsub channel from AI server""" | ||
| args = (sid, channel_id, chat_id, chat_message_id, chat_intent, discussion_status) | ||
| args = (sid, channel_id, chat_id, chat_message_id, discussion_status) | ||
| try: | ||
| sio.start_background_task(self.listen_to_redis_stream, *args) | ||
| except Exception as e: | ||
|
|
@@ -237,12 +274,10 @@ def process_prompt(self, sid: str, channel_id: str, chat_id: str, chat_message_i | |
| "GENERATE": ChatMessageStatus.GENERATE, | ||
| } | ||
| if is_retry: | ||
| chat_intent = ChatMessageStatus.TRANSFORM_RETRY | ||
| prompt = ( | ||
| f"Faulty yaml:{chat_message.technical_content} \n Error:{chat_message.transformation_error_message}" | ||
| ) | ||
| else: | ||
| chat_intent = chat_message.chat_intent.name | ||
| prompt = chat_message.prompt | ||
|
|
||
| if discussion_status in DISCUSSION_STATUS_MAP: | ||
|
|
@@ -311,7 +346,6 @@ def process_prompt(self, sid: str, channel_id: str, chat_id: str, chat_message_i | |
| "db_map": db_metadata, | ||
| "visitran_model": visitran_models, | ||
| "chat_name": chat_name, | ||
| "chat_intent": chat_intent, | ||
| "db_type": self.project_instance.database_type, | ||
| "llm_model_architect": chat_message.llm_model_architect, | ||
| "llm_model_developer": chat_message.llm_model_developer, | ||
|
|
@@ -335,7 +369,6 @@ def process_prompt(self, sid: str, channel_id: str, chat_id: str, chat_message_i | |
| channel_id=channel_id, | ||
| chat_id=chat_id, | ||
| chat_message_id=chat_message_id, | ||
| chat_intent=chat_intent, | ||
| discussion_status=chat_message.discussion_type, | ||
| ) | ||
|
|
||
|
|
@@ -347,10 +380,9 @@ def process_prompt(self, sid: str, channel_id: str, chat_id: str, chat_message_i | |
| channel_id=channel_id, | ||
| chat_id=chat_id, | ||
| chat_message_id=chat_message_id, | ||
| chat_intent=chat_intent, | ||
| discussion_status=chat_message.discussion_type, | ||
| ) | ||
| logging.info(f"process_prompt: chat_intent={chat_intent}, sid={sid}, channel_id={channel_id}") | ||
| logging.info(f"process_prompt: sid={sid}, channel_id={channel_id}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likely billing bug — TRANSFORM may be double-charged in non-WS mode. In the cloud override of chat_intent = chat_message.chat_intent.name if chat_message.chat_intent else "INFO"
if chat_intent != "TRANSFORM":
self.consume_tokens(...)In this (non-WS) branch the Redis stream listener is started after WS mode probably happens to work because eventlet yields on Redis I/O and the listener drains in time, but relying on yield ordering for billing correctness is brittle. Fix options, in order of preference:
The test plan claims billing was verified — please add an explicit non-WS test before merge. |
||
| chat_message = self._get_chat_message(chat_id=chat_id, chat_message_id=chat_message_id) | ||
|
|
||
| return chat_message | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,9 @@ | |
|
|
||
| class ChatSerializer(serializers.ModelSerializer): | ||
| user = UserMinimalSerializer(read_only=True) | ||
| chat_intent_name = serializers.CharField( | ||
| source='chat_intent.display_name', read_only=True, default=None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same field name (
The rationale (chat list shows the human label, message-level needs the canonical token for branching) is fine, but the asymmetry under one shared field name is undocumented and a future bug magnet — anyone wiring up a new component will assume both return the same thing. Either rename the chat-list one to |
||
| ) | ||
|
|
||
| class Meta: | ||
| model = Chat | ||
|
|
@@ -13,6 +16,7 @@ class Meta: | |
| 'project_id', | ||
| 'chat_name', | ||
| 'chat_intent', | ||
| 'chat_intent_name', | ||
| 'created_at', | ||
| 'modified_at', | ||
| 'is_deleted', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ class Meta: | |
|
|
||
| class ChatMessageSerializer(serializers.ModelSerializer): | ||
| user = UserMinimalSerializer(read_only=True) | ||
| chat_intent_name = serializers.CharField(source='chat_intent.name', read_only=True, default=None) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See companion comment on |
||
|
|
||
| class Meta: | ||
| model = ChatMessage | ||
|
|
@@ -29,6 +30,7 @@ class Meta: | |
| 'transformation_status', | ||
| 'transformation_error_message', | ||
| 'chat_intent', | ||
| 'chat_intent_name', | ||
| 'llm_model_architect', | ||
| 'llm_model_developer', | ||
| 'created_at', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner to initialize
self._chat_intent_by_msg = {}in__init__rather than lazy-init viahasattr. IfLLMServerContextis ever pooled or reused across requests (the rest of the class is stateful enough that this seems plausible), this dict carries entries across requests because there's no reset point. An explicit init in__init__makes the lifecycle obvious.