RG-T117 Chatbot fixes #453
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ public class ChatController : V4AuthenticatedApiControllerbase | |
| private readonly ICacheProvider _cacheProvider; | ||
| private readonly IEventAggregator _eventAggregator; | ||
| private readonly IQueueService _queueService; | ||
| private readonly IUserProfileService _userProfileService; | ||
|
|
||
| public ChatController( | ||
| IChatChannelService chatChannelService, | ||
|
|
@@ -66,7 +67,8 @@ public ChatController( | |
| IAuthorizationService authorizationService, | ||
| ICacheProvider cacheProvider, | ||
| IEventAggregator eventAggregator, | ||
| IQueueService queueService, | ||
| IUserProfileService userProfileService) | ||
| { | ||
| _chatChannelService = chatChannelService; | ||
| _chatPermissionService = chatPermissionService; | ||
|
|
@@ -81,6 +83,7 @@ public ChatController( | |
| _cacheProvider = cacheProvider; | ||
| _eventAggregator = eventAggregator; | ||
| _queueService = queueService; | ||
| _userProfileService = userProfileService; | ||
| } | ||
|
|
||
| #endregion Members and Constructors | ||
|
|
@@ -770,6 +773,17 @@ public async Task<ActionResult<ChatMessageSentResult>> SendMessage(string channe | |
| if (await IsRateLimitedAsync("send", ChatConfig.SendRateLimitPerWindow)) | ||
| return RateLimitedResult<ChatMessageSentResult>(); | ||
|
|
||
| // Assistant conversations are plain text only: no threads, no attachments/GIFs, no urgent | ||
| // priority. Enforced here so every client (web and mobile) gets the same behavior. | ||
| var channel = await _chatChannelService.GetChannelByIdAsync(channelId); | ||
| var isChatbotChannel = channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot; | ||
| if (isChatbotChannel && ((ChatMessageType)input.MessageType != ChatMessageType.Text || !String.IsNullOrWhiteSpace(input.ThreadRootMessageId))) | ||
|
There was a problem hiding this comment.
Unsafe type casting: Kody rule violation: Use safe type casting with as operator Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| return BadRequest("Assistant conversations only support plain text messages."); | ||
|
|
||
| // Urgent (ack-required) priority is a channel-level broadcast concept; thread replies | ||
| // always send as normal priority no matter what the client asked for. | ||
| var isThreadReply = !String.IsNullOrWhiteSpace(input.ThreadRootMessageId); | ||
|
|
||
| var request = new ChatMessageSendRequest | ||
| { | ||
| ChatChannelId = channelId, | ||
|
|
@@ -778,7 +792,7 @@ public async Task<ActionResult<ChatMessageSentResult>> SendMessage(string channe | |
| AsIncidentCommander = input.AsIncidentCommander, | ||
| Body = input.Body, | ||
| MessageType = (ChatMessageType)input.MessageType, | ||
| Priority = isChatbotChannel || isThreadReply ? ChatMessagePriority.Normal : (ChatMessagePriority)input.Priority, | ||
| ThreadRootMessageId = input.ThreadRootMessageId, | ||
| AlsoSendToChannel = input.AlsoSendToChannel, | ||
| ClientMessageId = input.ClientMessageId, | ||
|
|
@@ -823,8 +837,7 @@ public async Task<ActionResult<ChatMessageSentResult>> SendMessage(string channe | |
| // The assistant channel is also reachable from the regular chat page, so sends that arrive | ||
| // through this generic endpoint must still feed the chatbot pipeline (the dedicated | ||
| // ChatbotController.SendChatMessage does the same for the assistant panel). | ||
| if (isChatbotChannel && !String.IsNullOrWhiteSpace(message.Body)) | ||
| { | ||
| var queued = await _queueService.EnqueueChatbotMessageAsync(new Resgrid.Model.Queue.ChatbotMessageQueueItem | ||
| { | ||
|
|
@@ -916,6 +929,9 @@ public async Task<ActionResult<ChatActionResult>> DeleteMessage(string messageId | |
| if (!await ChatEnabledAsync()) | ||
| return NotFound(); | ||
|
|
||
| if (await IsChatbotMessageChannelAsync(messageId)) | ||
| return BadRequest("Messages can't be deleted in assistant conversations."); | ||
|
Comment on lines
+932
to
+933
There was a problem hiding this comment.
Information disclosure vulnerability in DeleteMessage: IsChatbotMessageChannelAsync is called before any department or ownership validation, returning a distinguishable BadRequest ("Messages can't be deleted in assistant conversations.") for chatbot channels versus a generic Failure for non-chatbot — allowing any authenticated user to probe arbitrary message IDs and learn which belong to assistant conversations. Add CheckMessageChannelAccessAsync before the chatbot-type guard so cross-department messages return a uniform NotFound. var accessCheck = await CheckMessageChannelAccessAsync(messageId);
if (accessCheck != null)
return accessCheck;
if (await IsChatbotMessageChannelAsync(messageId))
return BadRequest("Messages can't be deleted in assistant conversations.");Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| var result = new ChatActionResult(); | ||
| result.Success = await _chatMessageService.DeleteMessageAsync(messageId, UserId, false, null, cancellationToken); | ||
| result.Status = result.Success ? ResponseHelper.Deleted : ResponseHelper.Failure; | ||
|
|
@@ -957,6 +973,9 @@ public async Task<ActionResult<ChatActionResult>> AddReaction(string messageId, | |
| if (accessCheck != null) | ||
| return accessCheck; | ||
|
|
||
| if (await IsChatbotMessageChannelAsync(messageId)) | ||
| return BadRequest("Reactions aren't available in assistant conversations."); | ||
|
|
||
| var result = new ChatActionResult(); | ||
| result.Success = await _chatMessageService.AddReactionAsync(messageId, UserId, null, input.Emoji, cancellationToken); | ||
| result.Status = result.Success ? ResponseHelper.Created : ResponseHelper.Failure; | ||
|
|
@@ -988,6 +1007,9 @@ public async Task<ActionResult<ChatActionResult>> RemoveReaction(string messageI | |
| if (accessCheck != null) | ||
| return accessCheck; | ||
|
|
||
| if (await IsChatbotMessageChannelAsync(messageId)) | ||
| return BadRequest("Reactions aren't available in assistant conversations."); | ||
|
|
||
| var result = new ChatActionResult(); | ||
| result.Success = await _chatMessageService.RemoveReactionAsync(messageId, UserId, null, emoji, cancellationToken); | ||
| result.Status = result.Success ? ResponseHelper.Deleted : ResponseHelper.Failure; | ||
|
|
@@ -1055,9 +1077,18 @@ public async Task<ActionResult<GetChatAcksResult>> GetAcks(string messageId) | |
|
|
||
| if (acks != null && acks.Any()) | ||
| { | ||
| var profiles = await _userProfileService.GetSelectedUserProfilesAsync(acks.Select(a => a.UserId).Distinct().ToList()); | ||
| var namesByUserId = (profiles ?? new List<UserProfile>()) | ||
| .Where(p => p?.UserId != null) | ||
| .ToDictionary(p => p.UserId, p => p.FullName?.AsFirstNameLastName, StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| foreach (var ack in acks) | ||
| { | ||
| var data = ConvertAckResultData(ack); | ||
| if (ack.UserId != null && namesByUserId.TryGetValue(ack.UserId, out var name)) | ||
| data.DisplayName = name; | ||
|
|
||
| result.Data.Add(data); | ||
| } | ||
|
|
||
| result.PageSize = result.Data.Count; | ||
|
|
@@ -1254,6 +1285,9 @@ public async Task<ActionResult<ChatAttachmentUploadedResult>> UploadAttachment(s | |
| if (channel == null || channel.DepartmentId != DepartmentId) | ||
| return NotFound(); | ||
|
|
||
| if (channel.ChannelType == (int)ChatChannelType.Chatbot) | ||
| return BadRequest("Attachments aren't available in assistant conversations."); | ||
|
|
||
| if (!await _chatPermissionService.CanPostAsync(channel, UserId, null)) | ||
| return Unauthorized(); | ||
|
|
||
|
|
@@ -1553,6 +1587,20 @@ private async Task<bool> IsRateLimitedAsync(string action, int limitPerWindow) | |
| return StatusCode(StatusCodes.Status429TooManyRequests, result); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// True when the message lives in an assistant (chatbot) conversation, where reactions, | ||
| /// threads and deletes are not available. | ||
| /// </summary> | ||
| private async Task<bool> IsChatbotMessageChannelAsync(string messageId) | ||
| { | ||
| var message = await _chatMessageService.GetMessageByIdAsync(messageId); | ||
| if (message == null) | ||
| return false; | ||
|
|
||
| var channel = await _chatChannelService.GetChannelByIdAsync(message.ChatChannelId); | ||
| return channel != null && channel.ChannelType == (int)ChatChannelType.Chatbot; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies the message exists in this department and the user can access its channel. | ||
| /// Returns null when access is allowed, otherwise the error result to return. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -447,6 +447,14 @@ public async Task<ActionResult<ChatbotSessionResetResult>> NewChatSession() | |
| if (session != null) | ||
| await _chatbotSessionManager.EndSessionAsync(session.SessionId); | ||
|
|
||
| // Visible confirmation in the conversation (fans out over SignalR to every client); | ||
| // without it the reset is silent and looks like the button did nothing. | ||
| var channel = await _chatChannelService.EnsureChatbotChannelAsync(DepartmentId, UserId); | ||
| if (channel != null) | ||
| await _chatMessageService.SendBotMessageAsync(channel.ChatChannelId, | ||
| DepartmentId.ToString(System.Globalization.CultureInfo.InvariantCulture), | ||
| "Starting a new conversation — your previous context has been cleared.", "Resgrid Assistant"); | ||
|
Comment on lines
+452
to
+456
There was a problem hiding this comment.
State inconsistency in the reset flow: SendBotMessageAsync runs inside the same try/catch as EndSessionAsync, so a transient failure in the confirmation send returns BadRequest even though the session was already cleared — contradicting the persisted state and causing a duplicate 'Starting a new conversation' bot message on retry. Isolate the confirmation send in its own best-effort try/catch that logs but does not fail the reset. if (session != null)
await _chatbotSessionManager.EndSessionAsync(session.SessionId);
// Confirmation is best-effort: a failure here must not turn a successful reset into a failure.
try
{
var channel = await _chatChannelService.EnsureChatbotChannelAsync(DepartmentId, UserId);
if (channel != null)
await _chatMessageService.SendBotMessageAsync(channel.ChatChannelId,
DepartmentId.ToString(System.Globalization.CultureInfo.InvariantCulture),
"Starting a new conversation — your previous context has been cleared.", "Resgrid Assistant");
}
catch (Exception ex)
{
Logging.LogException(ex);
}
var result = new ChatbotSessionResetResult { Success = true, ... };Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. There was a problem hiding this comment.
Hardcoded bot identity: the display name "Resgrid Assistant" and the confirmation message are inlined, risking drift across chatbot message paths when the brand or name changes. Extract the bot name to a shared constant (e.g., ChatbotConstants.AssistantName) and centralize reusable message templates. Kody rule violation: Centralize string constants Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| var result = new ChatbotSessionResetResult | ||
| { | ||
| Success = true, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -741,6 +741,11 @@ public class ChatAckResultData | |
| /// When the user acknowledged (null = still pending) | ||
| /// </summary> | ||
| public DateTime? AcknowledgedOn { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Display name of the user the acknowledgment is required from (populated by GetAcks) | ||
| /// </summary> | ||
| public string DisplayName { get; set; } | ||
|
Comment on lines
+745
to
+748
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Populate
🤖 Prompt for AI AgentsThere was a problem hiding this comment.
Uninitialized auto-property: Kody rule violation: Initialize properties with default values Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Unguarded external call: GetChannelByIdAsync is not wrapped in try/catch, violating Rule [27]. Wrap the call in try/catch, log with channelId context, and return an appropriate error ActionResult.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.