Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a complete chatbot subsystem: new core and NLU projects, models/interfaces, NLU providers, services (ingress, routing, sessions, identity/linking, rate limiting), action handlers, platform adapters, persistence entities and migrations, web controllers and wiring, solution and project references, and tests. Replaces SMS text-command paths to use chatbot ingress. ChangesChatbot subsystem integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("Webhook")] | ||
| [Consumes("application/json")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> Webhook() |
| /// </summary> | ||
| [HttpPost("GenerateLinkingCode")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> GenerateLinkingCode() |
| /// </summary> | ||
| [HttpPost("UnlinkAccount")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> UnlinkAccount([FromBody] UnlinkRequest request) |
| /// </summary> | ||
| [HttpPost("OAuthComplete")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<IActionResult> OAuthComplete([FromBody] OAuthCompleteRequest request) |
| { | ||
| try | ||
| { | ||
| if (request == null) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
Core/Resgrid.Chatbot.NLU/Services/EntityExtractor.cs-110-124 (1)
110-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTime regex matches bare numbers, producing false-positive entities.
With both the
:mmgroup and theam|pmgroup optional,\b(\d{1,2})...\bmatches any 1–2 digit number — e.g.c5,call 12, or a status code — and emits aTimeExpression. Require either a colon-minutes or an am/pm suffix.🔧 Proposed fix
- var timePattern = new Regex(@"\b(\d{1,2})(:\d{2})?\s*(am|pm|AM|PM)?\b"); + var timePattern = new Regex(@"\b(\d{1,2}:\d{2}(\s*(am|pm))?|\d{1,2}\s*(am|pm))\b", RegexOptions.IgnoreCase);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot.NLU/Services/EntityExtractor.cs` around lines 110 - 124, The time regex in ExtractTimes currently allows bare 1–2 digit numbers and produces false positives; update the timePattern Regex in ExtractTimes so it only matches times that have either a colon+minutes or an am/pm suffix (e.g. use an alternation that matches \d{1,2}:\d{2} optionally with am/pm OR \d{1,2} with an am/pm), then continue to add a ChatbotEntity as before (keep EntityType = "TimeExpression", Value = match.Value, NormalizedValue = match.Value.ToLowerInvariant(), Confidence = 0.85) so only valid time-like tokens are captured.Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs-245-253 (1)
245-253:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Anthropic endpoint/auth mismatch in OpenAI-compatible provider
CloudNluProviderType.Anthropicpoints tohttps://api.anthropic.com/v1/messages, but this provider always sends an OpenAI chat-completions-style request body (and Bearer auth) and parses an OpenAI response (choices[0].message.content). Anthropic’s native/v1/messagesexpectsx-api-key+anthropic-versionand a different request/response, while the OpenAI SDK compatibility layer targets/v1/chat/completions. Adjust Anthropic handling to use the compatibility endpoint (and compatible config/headers) or implement the native messages flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs` around lines 245 - 253, The Anthropic branch in the endpoint switch (CloudNluProviderType.Anthropic) is pointing to the native Anthropic messages endpoint but OpenAiCompatibleNluProvider (driven by ChatbotConfig.CloudNluProvider) actually sends OpenAI-style chat-completions requests and expects OpenAI responses; update the Anthropic case to use the OpenAI-compatible endpoint (/v1/chat/completions) and ensure the request/header logic in OpenAiCompatibleNluProvider (the code that builds Authorization headers and request body) uses Bearer auth and the same request/response shape so parsing (choices[0].message.content) remains valid — alternatively, if you prefer native Anthropic support, implement a separate flow that targets /v1/messages and sets x-api-key + anthropic-version and adapts request/response parsing.Core/Resgrid.Chatbot/Handlers/MessagesActionHandler.cs-35-35 (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing null-conditional on
FullName.
profile?.FullName.AsFirstNameLastNameonly short-circuits on a nullprofile. Ifprofileis non-null butFullNameis null,AsFirstNameLastNamedereferences null and throws (caught → generic error).MyStatusActionHandlerguards this correctly withprofile?.FullName?.AsFirstNameLastName.🛡️ Proposed fix
- sb.AppendLine("Messages for " + (profile?.FullName.AsFirstNameLastName ?? "User") + ":"); + sb.AppendLine("Messages for " + (profile?.FullName?.AsFirstNameLastName ?? "User") + ":");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/MessagesActionHandler.cs` at line 35, The AppendLine call in MessagesActionHandler.cs uses profile?.FullName.AsFirstNameLastName which only guards against a null profile but will throw if profile.FullName is null; update the expression in the sb.AppendLine call to use the null-conditional on FullName (i.e., profile?.FullName?.AsFirstNameLastName) and keep the fallback "User" so it mirrors the safe pattern used in MyStatusActionHandler, ensuring AsFirstNameLastName is only invoked when FullName is non-null.Core/Resgrid.Chatbot/Handlers/CallDetailActionHandler.cs-31-34 (1)
31-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against a null
intent.Parameters.
DepartmentActionHandlernull-checksintent.Parametersbefore access, but hereintent.Parameters.TryGetValue(...)is called directly. IfParameterscan be null, this throws an NPE that the catch block turns into the generic "Error retrieving call details." response instead of the intended "Please specify a call number" prompt.🛡️ Proposed guard
- if (!intent.Parameters.TryGetValue("callId", out var callIdStr) || !int.TryParse(callIdStr, out var callId)) + if (intent.Parameters == null + || !intent.Parameters.TryGetValue("callId", out var callIdStr) + || !int.TryParse(callIdStr, out var callId)) { return new ChatbotResponse { Text = "Please specify a call number, e.g., C1445", Processed = false }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/CallDetailActionHandler.cs` around lines 31 - 34, The code in CallDetailActionHandler (inside the handler that reads intent.Parameters) calls intent.Parameters.TryGetValue directly which can throw if intent.Parameters is null; add a null check for intent.Parameters before attempting TryGetValue (mirror the pattern used in DepartmentActionHandler) and treat a null or missing parameter the same way—return new ChatbotResponse { Text = "Please specify a call number, e.g., C1445", Processed = false } when Parameters is null or when callId is not present/parsable; update the conditional around TryGetValue/int.TryParse to first ensure intent.Parameters != null and then proceed.Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs-49-65 (1)
49-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
userStatesagainst null (droplastActionLogsguard).
lastActionLogscomes fromActionLogsService.GetLastActionLogsForDepartmentAsync, which unconditionally forcesbypassCache = trueand returns a locally constructedList<ActionLog>, so it won’t be null due to cache retrieval.userStatescomes fromUserStateService.GetLatestStatesForDepartmentAsync, which uses_cacheProvider.RetrieveAsync; if that ever returns null,userStates.FirstOrDefault(...)will throw—so apply a null-to-empty fallback before the loop (e.g.,userStates ??= new List<UserState>()) inCore/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs(lines 49-65).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs` around lines 49 - 65, The code uses userStates without guarding for null; update PersonnelActionHandler (around where GetLastActionLogsForDepartmentAsync and GetLatestStatesForDepartmentAsync are called) to ensure userStates is non-null before use—e.g., after calling var userStates = await _userStateService.GetLatestStatesForDepartmentAsync(session.DepartmentId); add userStates ??= new List<UserState>(); and remove any unnecessary null guard for lastActionLogs (since GetLastActionLogsForDepartmentAsync returns a constructed list). This ensures userStates.FirstOrDefault(...) inside the loop won’t throw.Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs-105-105 (1)
105-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
identity.LastUsedAtmutation isn’t persisted inChatbotIngressService.
ChatbotIngressServicesetsidentity.LastUsedAt = DateTime.UtcNow;(afterGetIdentityAsync/GetIdentityByPhoneAsync) and then proceeds to department resolution/authorization without any subsequent call to_userIdentityServiceto save/touch the identity. InChatbotUserIdentityService,LastUsedAtis persisted only duringLinkUserAsyncvia_identityRepository.UpdateAsync/InsertAsync, while identity reads map to a model viaToModel(...)(so the in-memory assignment won’t automatically write back).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` at line 105, ChatbotIngressService sets identity.LastUsedAt = DateTime.UtcNow but never persists that change; after obtaining identity via GetIdentityAsync/GetIdentityByPhoneAsync and setting LastUsedAt, call the appropriate persistence method on _userIdentityService (or a new Update/Tou ch method) to save the updated identity before proceeding to department resolution/authorization so the change is stored in the repository—mirror how ChatbotUserIdentityService persists changes via LinkUserAsync which calls _identityRepository.UpdateAsync/InsertAsync.Core/Resgrid.Chatbot/Services/CodeLinkingService.cs-48-68 (1)
48-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCollision retry loop doesn't fail when exhausted.
If all 10 attempts to generate a unique code collide with existing codes, the loop exits and the colliding code is still inserted, which will cause a duplicate key violation or link to the wrong user's code.
🐛 Proposed fix
// Generate a unique code (retry on the rare collision with an existing code). string code; var attempts = 0; do { code = GenerateRandomCode(); attempts++; } while (await _linkingCodeRepository.GetByCodeAsync(code) != null && attempts < 10); +if (await _linkingCodeRepository.GetByCodeAsync(code) != null) + throw new InvalidOperationException("Unable to generate a unique linking code. Please try again."); + var entity = new LinkingCodeEntity🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/CodeLinkingService.cs` around lines 48 - 68, The retry loop that calls GenerateRandomCode() and checks _linkingCodeRepository.GetByCodeAsync(code) can exit after 10 attempts with a colliding code; after the loop, add a final uniqueness check (e.g., call _linkingCodeRepository.GetByCodeAsync(code)) and if it still exists throw a clear exception (InvalidOperationException or a custom exception) instead of proceeding to create the LinkingCodeEntity and calling InsertAsync; alternatively extend retry logic to continue until a unique code is found but ensure a bounded failure path that surfaces an error when uniqueness cannot be achieved.Core/Resgrid.Chatbot/Services/RedisSessionStore.cs-163-177 (1)
163-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrune cutoff omits the
> 0fallback used byTtl.
Ttl(Lines 35-37) falls back to 30 minutes whenDefaultSessionTimeoutMinutes <= 0, but here the default cutoff uses the raw config value. IfDefaultSessionTimeoutMinutesis0,effectiveCutoffbecomesDateTime.UtcNow, pruning effectively all active sessions on the next sweep.🛠️ Proposed fix
- var effectiveCutoff = cutoff ?? DateTime.UtcNow.AddMinutes(-ChatbotConfig.DefaultSessionTimeoutMinutes); + var timeoutMinutes = ChatbotConfig.DefaultSessionTimeoutMinutes > 0 + ? ChatbotConfig.DefaultSessionTimeoutMinutes + : 30; + var effectiveCutoff = cutoff ?? DateTime.UtcNow.AddMinutes(-timeoutMinutes);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/RedisSessionStore.cs` around lines 163 - 177, PruneExpiredAsync uses ChatbotConfig.DefaultSessionTimeoutMinutes directly which can be zero and cause immediate pruning; mirror the Ttl fallback logic (use the same > 0 check and default to 30 minutes) when computing effectiveCutoff so the cutoff uses the same timeout as the Ttl method; locate PruneExpiredAsync, the Ttl logic, ChatbotConfig.DefaultSessionTimeoutMinutes, _fallback and LastActivity and change the cutoff calculation to use the same conditional fallback (DefaultSessionTimeoutMinutes > 0 ? DefaultSessionTimeoutMinutes : 30) before subtracting minutes from DateTime.UtcNow.Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs-238-260 (1)
238-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
IncrementAsynccan leak a TTL-less counter on first-increment failures.
ICacheProvider.IncrementAsyncreturns0when the cache is unavailable, andChatbotRateLimiter.CheckAndConsumeRedisAsynctreatscount <= 0as fail-open (return true), so redis issues won’t block traffic.- Expiration is set only when
value == 1(KeyExpireAsyncis called only on the first increment). IfKeyExpireAsyncthrows, the increment still happens but TTL isn’t applied, and subsequent increments for the same key won’t retryKeyExpireAsync—so the counter can outlive the intended window until it’s cleared/evicted.Providers/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.cs-92-94 (1)
92-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce one
ChatbotDepartmentConfigsrow perDepartmentId
ChatbotDepartmentConfigService.SaveConfigAsyncassumes a single row per department: it callsGetByDepartmentIdAsync(which returnsFirstOrDefault()) and then inserts if null or updates the returned row otherwise—so duplicate rows would make the selected config nondeterministic. UpdateM0068_ChatbotTables(and the PostgresM0068_ChatbotTablesPgcounterpart) to makeIX_ChatbotDepartmentConfigs_DepartmentIdunique (this repo’s FluentMigrator unique index syntax uses.WithOptions().Unique()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.cs` around lines 92 - 94, The index on ChatbotDepartmentConfigs should be made unique to enforce one row per DepartmentId: update the Create.Index("IX_ChatbotDepartmentConfigs_DepartmentId") fluent call in M0068_ChatbotTables (and the analogous migration class M0068_ChatbotTablesPg) to add .WithOptions().Unique() so the index on table "ChatbotDepartmentConfigs" column "DepartmentId" is created as a unique index; keep the same index name and column reference when changing both migration files.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-409-433 (1)
409-433:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument request/response contracts for new Chatbot endpoints.
Please add
<param>and<returns>tags for methods with inputs/outputs (e.g.,UnlinkAccount,OAuthStart,OAuthComplete,UpdateConfig) so generated API docs remain actionable for client implementers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 409 - 433, Add missing XML doc <param> and <returns> tags for the Chatbot controller methods so generated API docs include request/response contracts: update the member docs for UnlinkAccount(Resgrid.Web.Services.Controllers.v4.UnlinkRequest) to include a <param name="request"> describing the UnlinkRequest payload and a <returns> describing the unlink result/status; for OAuthStart(System.String) add <param name="provider"> explaining the provider string and a <returns> that specifies the returned authorization URL; for OAuthComplete(Resgrid.Web.Services.Controllers.v4.OAuthCompleteRequest) add a <param name="request"> describing the code/state payload and a <returns> describing the link result and any returned user identifier/metadata; and for UpdateConfig(Resgrid.Web.Services.Controllers.v4.ChatbotConfigRequest) add <param name="request"> describing the config fields and a <returns> describing the update response (success/errors). Ensure tag text matches the parameter names used in the method signatures (UnlinkRequest, provider, OAuthCompleteRequest, ChatbotConfigRequest) and clearly states the type of data returned.
🧹 Nitpick comments (19)
Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs (2)
20-34: ⚖️ Poor tradeoffPrefer Service Locator over constructor injection.
This handler injects six dependencies through the constructor. The repo convention is to resolve dependencies explicitly via
Bootstrapper.GetKernel().Resolve<T>()and to keep constructor injection minimal. This applies to the sibling handlers in this PR as well.As per coding guidelines: "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection" and "Minimize constructor injection; keep the number of injected dependencies small".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs` around lines 20 - 34, The constructor for PersonnelActionHandler currently accepts six services (_departmentsService, _usersService, _actionLogsService, _userStateService, _customStateService, _authorizationService); change it to use the Service Locator pattern instead: remove the multi-parameter constructor injection and inside the constructor call Bootstrapper.GetKernel().Resolve<T>() for each required service to assign the corresponding private fields (use Resolve<IDepartmentsService>(), Resolve<IUsersService>(), Resolve<IActionLogsService>(), Resolve<IUserStateService>(), Resolve<ICustomStateService>(), Resolve<IAuthorizationService>()), keeping the private fields and other class usages unchanged so dependencies are resolved explicitly at construction via Bootstrapper.GetKernel().Resolve rather than passed in.
62-70: ⚖️ Poor tradeoffPer-user
awaitcalls create an N+1 pattern.Each user triggers two awaited service calls (
GetCustomPersonnelStatusAsync,GetCustomPersonnelStaffingAsync). It is bounded to 15 here, but if these resolve custom states per call, consider fetching the department's custom states once and mapping in memory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/PersonnelActionHandler.cs` around lines 62 - 70, The loop in PersonnelActionHandler.cs is causing an N+1 by awaiting _customStateService.GetCustomPersonnelStatusAsync and GetCustomPersonnelStaffingAsync per user; instead, fetch the department-level custom status and staffing collections once (e.g. call a single _customStateService method such as GetCustomPersonnelStatusesForDepartmentAsync(session.DepartmentId) and GetCustomPersonnelStaffingsForDepartmentAsync(session.DepartmentId) or a combined GetAllCustomStatesForDepartmentAsync) before the foreach, build in-memory dictionaries keyed by the relevant id (match against lastAction or state entries from lastActionLogs/userStates), and then inside the foreach use those dictionaries to resolve status and staffing for sb.AppendLine rather than calling the async methods per user.Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs (1)
43-45: ⚡ Quick winRemove the unused
customStatescall.
customStatesis assigned at Line 43 but never read, soGetAllActiveCustomStatesForDepartmentAsyncis an unnecessary awaited call on every status update.♻️ Proposed change
- var customStates = await _customStateService.GetAllActiveCustomStatesForDepartmentAsync(session.DepartmentId); var action = await _actionLogsService.GetLastActionLogForUserAsync(session.UserId, session.DepartmentId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/StatusActionHandler.cs` around lines 43 - 45, Remove the unused await call that assigns customStates: delete the line that awaits _customStateService.GetAllActiveCustomStatesForDepartmentAsync(...) and the unused variable customStates in StatusActionHandler (so only keep the subsequent awaits for _actionLogsService.GetLastActionLogForUserAsync and _customStateService.GetCustomPersonnelStatusAsync); ensure no other code references customStates and that removing this call does not remove necessary side-effects from GetAllActiveCustomStatesForDepartmentAsync.Core/Resgrid.Chatbot/Handlers/UnitsActionHandler.cs (1)
43-48: ⚖️ Poor tradeoffPer-unit
awaitis an N+1 pattern.
GetCustomUnitStateAsyncis awaited once per unit inside the loop. It is bounded to 15 here, but if a department-wide custom-state lookup is available, resolving in memory would avoid repeated round-trips.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Handlers/UnitsActionHandler.cs` around lines 43 - 48, The loop in UnitsActionHandler.cs does an N+1 await by calling _customStateService.GetCustomUnitStateAsync(unitState) for each unitState in unitList; change this to a single batch call: collect the unit identifiers from unitList, add or use a bulk method on _customStateService (e.g., GetCustomUnitStatesAsync or similar) that accepts the collection and returns all statuses, build a lookup (by Unit.Id or another unique key), then iterate unitList and use the lookup to append sb.AppendLine($"{unitState.Unit?.Name}: {lookup[unitId]?.ButtonText ?? "Unknown"}") so only one async call is made instead of per-unit awaits.Core/Resgrid.Chatbot/Interfaces/IConversationEngine.cs (1)
16-22: ⚡ Quick winUse an immutable record for
ConversationResultstate.Line 16-22 defines a pure state container as a mutable class. Converting it to a record with init-only properties better matches the project’s C# guidance.
As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#" and "Separate state from behavior (e.g., use records for state and place operations in separate static classes)".Proposed refactor
-public class ConversationResult -{ - public ChatbotResponse Response { get; set; } - public bool IsComplete { get; set; } - public bool NeedsFollowUp { get; set; } - public ChatbotDialogState NextState { get; set; } -} +public record ConversationResult +{ + public ChatbotResponse Response { get; init; } + public bool IsComplete { get; init; } + public bool NeedsFollowUp { get; init; } + public ChatbotDialogState NextState { get; init; } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Interfaces/IConversationEngine.cs` around lines 16 - 22, Replace the mutable class ConversationResult with an immutable record type: define ConversationResult as a record (either positional or standard) exposing ChatbotResponse Response { get; init; }, bool IsComplete { get; init; }, bool NeedsFollowUp { get; init; }, and ChatbotDialogState NextState { get; init; } so the state is immutable; keep the same property names and types so call sites using ConversationResult, ChatbotResponse and ChatbotDialogState continue to compile and update code that previously mutated properties to create new records instead.Core/Resgrid.Chatbot/Interfaces/IIntentMapper.cs (1)
4-6: ⚡ Quick winAlign namespace with the interface layer.
Line 4 places
IIntentMapperinResgrid.Chatbot.Servicesalthough the file belongs toCore/Resgrid.Chatbot/Interfaces. Keep this in an interfaces namespace to avoid contract-layer drift.As per coding guidelines, "Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations".Proposed fix
-namespace Resgrid.Chatbot.Services +namespace Resgrid.Chatbot.Interfaces🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Interfaces/IIntentMapper.cs` around lines 4 - 6, The IIntentMapper interface is declared under Resgrid.Chatbot.Services but should be in the interfaces namespace; update the namespace declaration for the IIntentMapper interface to Resgrid.Chatbot.Interfaces (keeping the interface name IIntentMapper unchanged) and adjust any referencing usings/imports in consumer files if necessary so the contract lives in the intended Interfaces layer.Core/Resgrid.Chatbot/Models/ChatbotPlatformCapabilities.cs (1)
18-96: 💤 Low value
MicrosoftTeams, andSignalfall through to default capabilities.These enum members aren't handled in the switch, so they silently get the conservative default (
MaxMessageLength = 1600, all capability flagsfalse). That's likely incorrect for WhatsApp (supports much longer messages and markdown-style formatting). No adapters exist for them yet, so this is latent, but consider either adding explicit cases or asserting via the default that they're intentionally unsupported for now.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Models/ChatbotPlatformCapabilities.cs` around lines 18 - 96, The ForPlatform method in ChatbotPlatformCapabilities omits explicit cases for ChatbotPlatform.WhatsApp, ChatbotPlatform.MicrosoftTeams and ChatbotPlatform.Signal so they silently fall back to the conservative default; update ForPlatform to handle these enum members explicitly (either by adding tailored ChatbotPlatformCapabilities entries for WhatsApp, MicrosoftTeams and Signal with appropriate MaxMessageLength and capability flags, or by changing the default branch to throw/Log/Assert for unsupported platforms) so their intent is explicit; locate the switch inside ChatbotPlatformCapabilities.ForPlatform and add separate cases for WhatsApp, MicrosoftTeams and Signal (or change the "_" arm to surface an unsupported-platform error) to fix the silent fall-through.Core/Resgrid.Chatbot/Services/ChatbotTemplateRenderer.cs (1)
19-19: 💤 Low value
capabilitiesparameter is fetched but never used.
ChatbotPlatformCapabilitiesis retrieved on line 19 and passed to every render method, but none of them actually use it. Either remove it until platform-specific formatting is implemented, or add a TODO comment documenting the intended use.Also applies to: 55-55, 75-75, 89-89, 105-105, 112-112, 128-128, 144-144, 160-160, 176-176, 183-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotTemplateRenderer.cs` at line 19, The local variable capabilities created via ChatbotPlatformCapabilities.ForPlatform(platform) is never used across the render methods (e.g., in ChatbotTemplateRenderer methods that call RenderX with capabilities), so either remove the capabilities argument and its retrieval where it's passed to each Render method, or keep it and add a clear TODO explaining intended platform-specific formatting; update all call sites that currently pass capabilities (lines noted include the calls around Render... methods) and the corresponding method signatures to match (or add TODO comments next to the ChatbotPlatformCapabilities.ForPlatform(platform) call and each unused parameter) so the code and signatures stay consistent.Core/Resgrid.Chatbot/Services/ChatbotRateLimiter.cs (1)
20-20: 💤 Low valueIn-memory counter dictionary grows unbounded.
The
_countersdictionary is never pruned. Over time, if many distinct users or departments hit the rate limiter, entries accumulate indefinitely. Consider periodic cleanup of stale entries (e.g., remove counters whoseWindowStartis older than a few minutes).💡 Suggested cleanup approach
Add a periodic or lazy cleanup when counter count exceeds a threshold:
private bool CheckAndConsumeMemory(string key, int limit) { if (limit <= 0) return true; + // Lazy cleanup: prune stale entries when dictionary grows large + if (_counters.Count > 10000) + PruneStaleCounters(); + var counter = _counters.GetOrAdd(key, _ => new Counter()); ... } +private void PruneStaleCounters() +{ + var cutoff = DateTime.UtcNow - Window - Window; // 2 windows old + foreach (var kvp in _counters) + { + if (kvp.Value.WindowStart < cutoff) + _counters.TryRemove(kvp.Key, out _); + } +}Also applies to: 86-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotRateLimiter.cs` at line 20, The _counters ConcurrentDictionary in ChatbotRateLimiter grows unbounded; implement pruning by removing stale Counter entries whose WindowStart is older than a configured retention (e.g., 5–10 minutes). Add a cleanup trigger in the rate limiter (either a background timer in the ChatbotRateLimiter class or a lazy cleanup run when _counters.Count exceeds a threshold) that enumerates keys and calls _counters.TryRemove(key, out _) for Counter instances with WindowStart < DateTime.UtcNow - retention; ensure you reference the Counter type and its WindowStart property and keep removal operations thread-safe using TryRemove to avoid races. Make retention and threshold configurable constants/fields and run cleanup periodically or on-demand to prevent unbounded growth.Core/Resgrid.Chatbot/Services/OAuthLinkingService.cs (1)
24-24: ⚖️ Poor tradeoffConsider using
IHttpClientFactoryfor better lifecycle management.A static
HttpClientworks but doesn't benefit from DNS refresh or connection pooling management thatIHttpClientFactoryprovides. For OAuth flows with infrequent calls this is acceptable, but worth considering for consistency with ASP.NET Core best practices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/OAuthLinkingService.cs` at line 24, The static HttpClient field _http in OAuthLinkingService should be replaced with an injected IHttpClientFactory to follow ASP.NET Core best practices: remove the private static readonly HttpClient _http, add an IHttpClientFactory parameter to the OAuthLinkingService constructor, store the factory in a private readonly field, and obtain an HttpClient instance inside methods that make HTTP calls via _httpFactory.CreateClient() (or a named/typed client) so connection pooling and DNS refresh are handled properly; update all uses of _http to use the created client and register the service in DI if not already.Core/Resgrid.Chatbot/Services/RedisSessionStore.cs (1)
30-33: 💤 Low valueConsider resolving
ICacheProvidervia the service locator per repo convention.The codebase convention is to resolve dependencies explicitly through
Bootstrapper.GetKernel().Resolve<T>()rather than constructor injection. This is a single dependency, so the change is small; align only if the chatbot wiring follows the same pattern as the rest of the codebase.As per coding guidelines: "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/RedisSessionStore.cs` around lines 30 - 33, RedisSessionStore's constructor currently accepts ICacheProvider via constructor injection; change it to resolve the dependency using the repo's service-locator pattern: inside RedisSessionStore (constructor or field initializer) obtain the ICacheProvider by calling Bootstrapper.GetKernel().Resolve<ICacheProvider>() and assign it to the _cacheProvider field, removing the constructor parameter (or leaving a parameterless constructor if needed) so the class follows the project's Bootstrapper.GetKernel().Resolve<T>() convention for ICacheProvider resolution.Providers/Resgrid.Providers.Chatbot/Interfaces/IChatbotPlatformAdapter.cs (1)
6-11: 🏗️ Heavy liftIChatbotPlatformAdapter contract is incomplete vs rich adapter capabilities
IChatbotPlatformAdapteronly exposesParseInboundMessageAsync+FormatOutboundResponseAsync(Telegram controller uses exactly those viaIEnumerable<IChatbotPlatformAdapter>); meanwhileDiscordBotAdapter/SlackBotAdapter/TelegramBotAdapteralso implementValidateAsync,IsUserAuthorizedAsync, andSendResponseAsync, but those extra members aren’t referenced outside the adapter classes—either align the interface by extending/splitting contracts or remove/limit these members if they’re not part of the pipeline yet.IChatbotPlatformAdapterConfigis declared insideProviders/Resgrid.Providers.Chatbot/Adapters/DiscordBotAdapter.cs; move it into theProviders/Resgrid.Providers.Chatbot/Interfacesfolder for consistency/discoverability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Chatbot/Interfaces/IChatbotPlatformAdapter.cs` around lines 6 - 11, The IChatbotPlatformAdapter contract is currently incomplete: extend the interface IChatbotPlatformAdapter to include the additional lifecycle methods that concrete adapters already implement—add signatures for ValidateAsync(...), IsUserAuthorizedAsync(...), and SendResponseAsync(...) alongside the existing ParseInboundMessageAsync and FormatOutboundResponseAsync so callers can rely on a single consistent contract; alternatively, if these methods are optional, split into a new IChatbotPlatformLifecycle (or IChatbotAuthorizer / ISender) and have DiscordBotAdapter, SlackBotAdapter, TelegramBotAdapter implement the finer-grained interfaces. Also move the nested IChatbotPlatformAdapterConfig type out of DiscordBotAdapter.cs into the Providers/Resgrid.Providers.Chatbot.Interfaces folder and update references so the config interface is discoverable and shared across adapters.Providers/Resgrid.Providers.Chatbot/Adapters/TwilioSmsAdapter.cs (1)
27-38: 💤 Low valueReflection fallback may silently yield empty body.
When
rawRequestis aTwilioRequestbut not aTwilioMessage, the reflection lookup forBody/MessageSid/SmsMessageSidonly succeeds if those properties exist on the concrete type; otherwisebodystaysnulland the message text is lost without any signal. Prefer binding to the strongly-typedTwilioMessageon the controller side so this reflection path isn't relied upon, and confirm the baseTwilioRequestactually exposes these members.Does Twilio.AspNet.Common TwilioRequest (Twilio.AspNet.Core 8.0.1) expose Body, MessageSid, or SmsMessageSid properties?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Chatbot/Adapters/TwilioSmsAdapter.cs` around lines 27 - 38, The reflection fallback in TwilioSmsAdapter (when reading rawRequest to populate body and messageSid) can silently leave body null; fix by using strong model binding to TwilioMessage on the controller so the adapter receives a typed TwilioMessage rather than relying on reflection on TwilioRequest, and update TwilioSmsAdapter to first try casting rawRequest to TwilioMessage and read its Body/MessageSid/SmsMessageSid properties directly, only using reflection as a last resort and emitting a warning/error log if neither cast nor reflection yields a non-empty body or messageSid (check variables body and messageSid and the class TwilioSmsAdapter to implement these changes).Tests/Resgrid.Tests/Chatbot/ChatbotDeptConfigAndSessionTests.cs (1)
23-30: 💤 Low valueConsider isolating static configuration mutation.
The
CacheMock()helper setsSystemBehaviorConfig.CacheEnabled = falseglobally, which may affect test isolation if tests run in parallel or if other tests depend on cache behavior.Consider using test-specific mocks or setup/teardown to restore original values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Resgrid.Tests/Chatbot/ChatbotDeptConfigAndSessionTests.cs` around lines 23 - 30, The helper CacheMock currently mutates the global static Resgrid.Config.SystemBehaviorConfig.CacheEnabled = false which can break test isolation; change CacheMock (or surrounding test setup) to avoid permanent static mutation by capturing the original value, setting CacheEnabled to false only for the duration of the test, and restoring the original value in teardown (or use per-test setup/teardown fixtures) or rework tests to inject a test-specific config instead of touching the static; locate the CacheMock method and ensure any change to SystemBehaviorConfig.CacheEnabled is reverted after the test or replaced with dependency injection to prevent cross-test pollution.Tests/Resgrid.Tests/Chatbot/ChatbotSecurityTests.cs (1)
191-206: 💤 Low valueConsider isolating static configuration mutation.
Lines 194-195 set
ChatbotConfigstatic fields, which may affect test isolation if tests run in parallel. Consider using setup/teardown to restore original values or using test-specific configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Resgrid.Tests/Chatbot/ChatbotSecurityTests.cs` around lines 191 - 206, This test mutates static ChatbotConfig fields (ChatbotConfig.DiscordClientId and ChatbotConfig.OAuthRedirectUri) which can break test isolation; update the test to preserve and restore the original static values (or set them in a test setup/teardown) around the use of OAuthLinkingService.StartLink and ExchangeAndLinkAsync so parallel tests aren't affected, i.e., capture original values before changing, assign the test values, run the assertions using StartLink/ExchangeAndLinkAsync, then restore the originals (or move these assignments into a [SetUp]/[TearDown] or use a disposable test helper to ensure cleanup).Repositories/Resgrid.Repositories.DataRepository/ChatbotLinkingCodeRepository.cs (1)
24-31: 💤 Low valueConsider using Service Locator pattern instead of constructor injection.
As per coding guidelines, the codebase uses the Service Locator pattern via
Bootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/ChatbotLinkingCodeRepository.cs` around lines 24 - 31, The constructor of ChatbotLinkingCodeRepository currently uses constructor injection to set _connectionProvider, _sqlConfiguration, _queryFactory, and _unitOfWork; change this to resolve those dependencies via the Service Locator pattern by calling Bootstrapper.GetKernel().Resolve<T>() for each dependency inside the constructor (replace the parameters and assignments with resolves for IConnectionProvider, SqlConfiguration, IQueryFactory, and IUnitOfWork respectively), ensuring the class still assigns the resolved instances to the same private fields (_connectionProvider, _sqlConfiguration, _queryFactory, _unitOfWork) and keeps the base(...) call consistent with the resolved values.Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs (1)
33-39: 💤 Low valueConsider using Service Locator pattern instead of constructor injection.
As per coding guidelines, the codebase uses the Service Locator pattern via
Bootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs` around lines 33 - 39, Replace constructor injection in ChatbotTelegramController by resolving dependencies from the Service Locator: inside the ChatbotTelegramController constructor, call Bootstrapper.GetKernel().Resolve<IChatbotIngressService>() to assign _chatbotIngressService and Bootstrapper.GetKernel().Resolve<IEnumerable<IChatbotPlatformAdapter>>() to assign _adapters, removing the ctor parameters for IChatbotIngressService and IEnumerable<IChatbotPlatformAdapter>; keep the field names _chatbotIngressService and _adapters and ensure any null checks or exceptions remain as before after resolution.Repositories/Resgrid.Repositories.DataRepository/ChatbotIdentityRepository.cs (1)
24-31: 💤 Low valueConsider using Service Locator pattern instead of constructor injection.
As per coding guidelines, the codebase uses the Service Locator pattern via
Bootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/ChatbotIdentityRepository.cs` around lines 24 - 31, The constructor of ChatbotIdentityRepository currently accepts IConnectionProvider, SqlConfiguration, IUnitOfWork and IQueryFactory via parameters; change it to resolve those dependencies using the Service Locator by calling Bootstrapper.GetKernel().Resolve<T>() for each dependency inside the constructor (replace direct parameter assignments for IConnectionProvider, SqlConfiguration, IUnitOfWork, IQueryFactory) so the class uses Bootstrapper.GetKernel().Resolve<T>() to obtain instances instead of constructor injection.Web/Resgrid.Web/Startup.cs (1)
508-512: 💤 Low valueConsider module-based registration for consistency.
While this manual registration works, it's inconsistent with the pattern used in
Web/Resgrid.Web.Services/Startup.cs(lines 647-649), where chatbot-related services are registered viaRegisterModule(new ChatbotModule()). For consistency and maintainability, consider either:
- Adding the same
ChatbotModuleregistration here (if the module is designed to conditionally register only what's needed), or- Creating a dedicated lightweight module for the Web-specific chatbot services.
This would make the registration pattern uniform across both startup files and easier to discover.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Startup.cs` around lines 508 - 512, The manual Autofac registration for Resgrid.Chatbot.Services.ChatbotDepartmentConfigService (.As<Resgrid.Chatbot.Interfaces.IChatbotDepartmentConfigService>().InstancePerLifetimeScope()) is inconsistent with the module-based pattern used elsewhere; fix by replacing the explicit builder.RegisterType(...) call with a module registration (call builder.RegisterModule(new ChatbotModule())) so the ChatbotModule handles registering ChatbotDepartmentConfigService and related chatbot services, or alternatively create a small Web-specific module (e.g., WebChatbotModule) that registers Resgrid.Chatbot.Services.ChatbotDepartmentConfigService as IChatbotDepartmentConfigService and then call builder.RegisterModule(new WebChatbotModule()) to keep startup registrations uniform and discoverable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fb1e40c-4ab6-400a-9a91-5246bea6345e
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/Department/Department.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (101)
.gitignoreCore/Resgrid.Chatbot.NLU/NLUModule.csCore/Resgrid.Chatbot.NLU/Providers/KeywordIntentClassifier.csCore/Resgrid.Chatbot.NLU/Providers/MLNetNluProvider.csCore/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.csCore/Resgrid.Chatbot.NLU/Resgrid.Chatbot.NLU.csprojCore/Resgrid.Chatbot.NLU/Services/EntityExtractor.csCore/Resgrid.Chatbot/ChatbotModule.csCore/Resgrid.Chatbot/Config/ChatbotConfig.csCore/Resgrid.Chatbot/Handlers/CalendarActionHandler.csCore/Resgrid.Chatbot/Handlers/CallDetailActionHandler.csCore/Resgrid.Chatbot/Handlers/CallsActionHandler.csCore/Resgrid.Chatbot/Handlers/DepartmentActionHandler.csCore/Resgrid.Chatbot/Handlers/HelpActionHandler.csCore/Resgrid.Chatbot/Handlers/MessagesActionHandler.csCore/Resgrid.Chatbot/Handlers/MyStatusActionHandler.csCore/Resgrid.Chatbot/Handlers/PersonnelActionHandler.csCore/Resgrid.Chatbot/Handlers/StaffingActionHandler.csCore/Resgrid.Chatbot/Handlers/StatusActionHandler.csCore/Resgrid.Chatbot/Handlers/UnitsActionHandler.csCore/Resgrid.Chatbot/Interfaces/IChatbotActionHandler.csCore/Resgrid.Chatbot/Interfaces/IChatbotDepartmentConfigService.csCore/Resgrid.Chatbot/Interfaces/IChatbotIngressService.csCore/Resgrid.Chatbot/Interfaces/IChatbotIntentRouter.csCore/Resgrid.Chatbot/Interfaces/IChatbotRateLimiter.csCore/Resgrid.Chatbot/Interfaces/IChatbotSessionManager.csCore/Resgrid.Chatbot/Interfaces/IChatbotSessionStore.csCore/Resgrid.Chatbot/Interfaces/IChatbotTemplateRenderer.csCore/Resgrid.Chatbot/Interfaces/IChatbotUserIdentityService.csCore/Resgrid.Chatbot/Interfaces/IConversationEngine.csCore/Resgrid.Chatbot/Interfaces/IEntityExtractor.csCore/Resgrid.Chatbot/Interfaces/IIntentMapper.csCore/Resgrid.Chatbot/Interfaces/INLUProvider.csCore/Resgrid.Chatbot/Models/ChatbotEntity.csCore/Resgrid.Chatbot/Models/ChatbotIntent.csCore/Resgrid.Chatbot/Models/ChatbotMessage.csCore/Resgrid.Chatbot/Models/ChatbotMessageLog.csCore/Resgrid.Chatbot/Models/ChatbotPlatform.csCore/Resgrid.Chatbot/Models/ChatbotPlatformCapabilities.csCore/Resgrid.Chatbot/Models/ChatbotResponse.csCore/Resgrid.Chatbot/Models/ChatbotSession.csCore/Resgrid.Chatbot/Models/ChatbotUserIdentity.csCore/Resgrid.Chatbot/Models/DepartmentLlmOverride.csCore/Resgrid.Chatbot/Models/NLUResult.csCore/Resgrid.Chatbot/Resgrid.Chatbot.csprojCore/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Chatbot/Services/ChatbotIntentRouter.csCore/Resgrid.Chatbot/Services/ChatbotRateLimiter.csCore/Resgrid.Chatbot/Services/ChatbotSessionManager.csCore/Resgrid.Chatbot/Services/ChatbotTemplateRenderer.csCore/Resgrid.Chatbot/Services/ChatbotUserIdentityService.csCore/Resgrid.Chatbot/Services/CodeLinkingService.csCore/Resgrid.Chatbot/Services/ConversationEngine.csCore/Resgrid.Chatbot/Services/IntentMapper.csCore/Resgrid.Chatbot/Services/OAuthLinkingService.csCore/Resgrid.Chatbot/Services/RedisSessionStore.csCore/Resgrid.Model/ChatbotDepartmentConfig.csCore/Resgrid.Model/ChatbotIdentity.csCore/Resgrid.Model/ChatbotLinkingCode.csCore/Resgrid.Model/Providers/ICacheProvider.csCore/Resgrid.Model/Repositories/IChatbotDepartmentConfigRepository.csCore/Resgrid.Model/Repositories/IChatbotIdentityRepository.csCore/Resgrid.Model/Repositories/IChatbotLinkingCodeRepository.csProviders/Resgrid.Providers.Cache/AzureRedisCacheProvider.csProviders/Resgrid.Providers.Chatbot/Adapters/DiscordBotAdapter.csProviders/Resgrid.Providers.Chatbot/Adapters/SignalWireSmsAdapter.csProviders/Resgrid.Providers.Chatbot/Adapters/SlackBotAdapter.csProviders/Resgrid.Providers.Chatbot/Adapters/TelegramBotAdapter.csProviders/Resgrid.Providers.Chatbot/Adapters/TwilioSmsAdapter.csProviders/Resgrid.Providers.Chatbot/ChatbotProviderModule.csProviders/Resgrid.Providers.Chatbot/Interfaces/IChatbotPlatformAdapter.csProviders/Resgrid.Providers.Chatbot/Resgrid.Providers.Chatbot.csprojProviders/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.csProviders/Resgrid.Providers.Migrations/Migrations/M0069_ChatbotLinkingCodes.csProviders/Resgrid.Providers.Migrations/Migrations/M0070_ChatbotDepartmentConfigColumns.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0068_ChatbotTablesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0069_ChatbotLinkingCodesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0070_ChatbotDepartmentConfigColumnsPg.csRepositories/Resgrid.Repositories.DataRepository/ChatbotDepartmentConfigRepository.csRepositories/Resgrid.Repositories.DataRepository/ChatbotIdentityRepository.csRepositories/Resgrid.Repositories.DataRepository/ChatbotLinkingCodeRepository.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csResgrid.slnTests/Resgrid.Tests/Chatbot/ChatbotDeptConfigAndSessionTests.csTests/Resgrid.Tests/Chatbot/ChatbotSecurityTests.csTests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.csWeb/Resgrid.Web.Services/Controllers/ChatbotTelegramController.csWeb/Resgrid.Web.Services/Controllers/SignalWireController.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web.Services/Controllers/v4/ChatbotController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.csprojWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.csWeb/Resgrid.Web/Areas/User/Models/ChatbotSettingsModel.csWeb/Resgrid.Web/Areas/User/Views/ChatbotSettings/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/Settings.cshtmlWeb/Resgrid.Web/Resgrid.Web.csprojWeb/Resgrid.Web/Startup.csWeb/Resgrid.Web/wwwroot/js/ng/react-elements.css
| public string ProviderName => "CloudLLM"; | ||
| public int Priority => 100; | ||
|
|
||
| private readonly HttpClient _httpClient; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Why does creating a new HttpClient per request cause socket exhaustion, and what is the recommended singleton/IHttpClientFactory pattern?
💡 Result:
Creating a new HttpClient per request causes socket exhaustion because every instance manages its own connection pool [1][2]. When you dispose of an HttpClient, the underlying connection is not immediately released by the operating system; instead, the TCP socket enters a TIME_WAIT state, which typically lasts several minutes [1][3][4]. Under heavy traffic, creating new instances repeatedly causes these sockets to pile up in the TIME_WAIT state, eventually exhausting the available ephemeral port range and leading to SocketException errors (e.g., "Only one usage of each socket address is normally permitted") [5][4][6]. To resolve this, use one of the two recommended patterns: 1. IHttpClientFactory (Recommended) Introduced to manage the lifecycle of underlying HttpMessageHandler instances, the factory pools these handlers to be reused, which avoids both socket exhaustion and stale DNS issues [1][7][8]. You register the factory in your dependency injection container and inject IHttpClientFactory into your services to create clients as needed [9][7]. - Pros: Manages pooling automatically, handles DNS refreshes by cycling handlers, and supports middleware/Polly integration [1][7][8]. 2. Static/Singleton HttpClient Alternatively, you can maintain a single static or singleton instance of HttpClient throughout the application's lifetime [1][10]. - Crucial Requirement: To address the issue where a singleton client ignores DNS changes, you must configure the underlying SocketsHttpHandler with a PooledConnectionLifetime (e.g., 2 minutes) [1][8]. - Pros: Simple to implement without extra factory infrastructure [1]. - Note: Avoid injecting "Typed Clients" into singleton services, as this can capture the client and prevent it from rotating connections, which defeats the purpose of the factory's management [8][11]. In modern.NET development, IHttpClientFactory is generally preferred for its built-in lifecycle management and ease of integration with resilience policies like retries or circuit breakers [7][8].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines
- 2: https://softwareengineering.stackexchange.com/questions/330364/should-we-create-a-new-single-instance-of-httpclient-for-all-requests
- 3: https://dev.to/outdated-dev/port-exhaustion-context-switching-and-why-httpclientfactory-exists-net-2api
- 4: https://medium.com/@ravikumar.makwana/the-httpclient-mistake-that-quietly-breaks-net-apps-in-production-de4a01cb9040
- 5: https://nelsonbn.com/blog/dotnet-httpclient-use-correctly/
- 6: https://learn.microsoft.com/en-us/azure/architecture/antipatterns/improper-instantiation/
- 7: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-10.0
- 8: https://learn.microsoft.com/en-us/dotnet/core/extensions/httpclient-factory
- 9: https://stackoverflow.com/questions/58427764/httpclient-and-socket-exhaustion-clarification
- 10: https://contrivedexample.com/2017/07/01/using-httpclient-as-it-was-intended-because-youre-not/
- 11: https://www.reddit.com/r/dotnet/comments/1ehvght/httpclientfactory_question/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- line 1-120 ----"
sed -n '1,140p' "$FILE" | cat -n
echo "---- find catch blocks near 200-260 ----"
sed -n '180,270p' "$FILE" | cat -n
echo "---- find ParseOpenAiResponse near 330-420 ----"
sed -n '320,430p' "$FILE" | cat -n
echo "---- find ResolveEndpoint and request body mapping ----"
rg -n "ResolveEndpoint|anthropic|/v1/messages|chat/completions|ChatCompletions|messages" "$FILE"
echo
rg -n "catch\s*\(" "$FILE"Repository: Resgrid/Core
Length of output: 14350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "OpenAiCompatibleNluProvider" -S Core | head -n 50
# Try to locate the module files mentioned in the original comment
rg -n "class (NLUModule|ChatbotModule)\b|NLUModule|ChatbotModule" -S Core | head -n 50
# Show request-body building around where messages list is constructed
FILE="Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs"
sed -n '120,220p' "$FILE" | cat -nRepository: Resgrid/Core
Length of output: 4493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Core/Resgrid.Chatbot.NLU/NLUModule.cs"
echo "---- NLUModule.cs ----"
sed -n '1,120p' "$FILE" | cat -n
PROVIDER="Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs"
echo "---- logging usage in provider ----"
rg -n "Resgrid\.Framework\.Logging|LogException|LogError|LogInfo|LogDebug" "$PROVIDER" || true
echo "---- check if provider implements IDisposable ----"
rg -n "class OpenAiCompatibleNluProvider|IDisposable" "$PROVIDER"Repository: Resgrid/Core
Length of output: 1392
🌐 Web query:
Anthropic Messages API v1/messages request body schema differences from OpenAI chat completions (messages/role/content, system, max_tokens)
💡 Result:
Anthropic Messages API v1/messages request body vs OpenAI Chat Completions request body: main schema differences (messages/role/content, system, max_tokens). 1) messages/role/content - Anthropic (messages[]): each item has role and content; content can be either a single string or an array of typed content blocks. Anthropic notes that a string is shorthand for an array containing one text block (type: "text"). [1] - Anthropic also states that input messages must alternate user/assistant turns, and that there is no system role in the input messages for the initial system instruction (use the top-level system parameter instead). [1] - OpenAI Chat Completions (messages[]): messages are similarly an array of objects with role and content, but OpenAI’s schema is not based on Anthropic’s typed content blocks; content is the usual chat message content (commonly a string in the request schema). [2] Migration implication: if your OpenAI messages[i].content is a string, Anthropic will accept it, but if you relied on richer OpenAI content part types, you’ll likely need to map them into Anthropic’s content blocks structure (array of {type,...}). [1] 2) system - Anthropic: system instructions are provided via a top-level request field named system. [1][3] - Anthropic also documents that you can include role:"system" messages after the first turn as additional system instructions; those mid-conversation system messages have the same authority as the top-level system field, but are appended to the history. [3] - OpenAI Chat Completions: system instructions are provided inside the messages array as a message with role:"system". [2] Migration implication: move OpenAI’s {role:"system", content:"..."} into Anthropic’s top-level system field (or keep it as role:"system" only for mid-conversation additions, per Anthropic’s guidance). [3][1] 3) max_tokens - Anthropic: max_tokens is the maximum number of tokens to generate for the response; their docs describe it as a maximum generation limit and emphasize Claude may stop before reaching it. [4][5] - OpenAI Chat Completions: max_tokens is described as an upper bound on tokens that can be generated; OpenAI documentation also notes max_tokens has been deprecated in favor of max_completion_tokens (e.g., for newer models). [6][7] Migration implication: for OpenAI, some models may require using max_completion_tokens rather than max_tokens; for Anthropic /v1/messages, the parameter is max_tokens. [7][6][4] Quick before/after shape (illustrative) - OpenAI chat.completions request: system typically appears as a message in messages (role:"system"). [2] - Anthropic messages request: system typically appears as top-level system, and messages[i].content can be a string or an array of typed blocks. [1][3] If you share your exact OpenAI request body (including tool/function calls or multimodal inputs), I can produce an exact field-by-field transformation to the Anthropic /v1/messages format.
Citations:
- 1: https://gemdocs.org/gems/anthropic/1.16.3/Anthropic/Models/MessageCountTokensParams.html
- 2: https://openai-hd4n6.mintlify.app/api-reference/chat/create-chat-completion
- 3: https://platform.claude.com/docs/en/build-with-claude/working-with-messages
- 4: https://github.com/anthropics/prompt-eng-interactive-tutorial/blob/0d277542e927652da25b0014c9b346723af55881/Anthropic%201P/01_Basic_Prompt_Structure.ipynb
- 5: https://github.com/anthropics/anthropic-sdk-python/blob/49d639a6/src/anthropic/types/beta/message_count_tokens_params.py
- 6: https://cdn.jsdelivr.net/npm/openai@4.76.0/src/resources/chat/completions.ts
- 7: https://community.openai.com/t/why-was-max-tokens-changed-to-max-completion-tokens/938077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Core/Resgrid.Chatbot.NLU/NLUModule.cs"
sed -n '1,120p' "$FILE" | cat -n
PROVIDER="Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs"
rg -n "Resgrid\.Framework\.Logging|LogException|LogError|LogInfo|LogDebug" "$PROVIDER" || true
rg -n "class OpenAiCompatibleNluProvider|IDisposable" "$PROVIDER"Repository: Resgrid/Core
Length of output: 1282
🌐 Web query:
Anthropic Messages API v1/messages request body schema fields (messages role/content arrays, system, max_tokens) compared to OpenAI chat completions
💡 Result:
The Anthropic Messages API (v1) and OpenAI Chat Completions API share a similar high-level purpose but differ in structural design and required parameters [1][2][3]. Key comparisons regarding request body schema fields are outlined below. System Prompt Anthropic: Uses a dedicated top-level system field [4][5][1]. It does not support a system role within the messages array [2][6]. OpenAI: Defines system instructions by including a message with the role system within the messages array [7][1][2]. Messages (role/content arrays) Anthropic: Requires an array of objects where each object has a role (user or assistant) and content [4][8]. Content can be a string or an array of content blocks (e.g., text, images, tool_use) [9][5]. Roles must strictly alternate between user and assistant [8][10]. OpenAI: Also uses an array of objects for messages, but the roles include system, user, assistant, and tool [7][1]. OpenAI is generally more flexible with message ordering and does not strictly require alternation in the same manner as Anthropic [7][6]. Max Tokens Anthropic: The max_tokens field is mandatory in the request body [2]. It specifies the maximum number of tokens to generate [4][9]. OpenAI: The equivalent parameter (max_tokens or max_completion_tokens) is optional; if omitted, the API defaults to a model-specific limit [2]. Structural Differences Summary Feature: Anthropic Messages API | OpenAI Chat Completions Endpoint: /v1/messages | /v1/chat/completions System Prompt: Top-level system field [4][1] | Role: system in messages [1][2] Required Fields: max_tokens is mandatory [2] | max_tokens is optional [2] Content: Rich blocks (text, images, tool_use) [3] | Simple string (or specific tool blocks) [1] Response Wrapper: Returns content array directly [1][2] | Returns a choices array [1][2] Both APIs support streaming via Server-Sent Events (SSE) and provide similar usage metrics, though field names (e.g., input_tokens vs. prompt_tokens) vary [1][2]. When migrating between them, system prompts must be moved to the top level for Anthropic, and the response parsing logic must be updated to account for the different wrappers (content vs. choices) [2].
Citations:
- 1: https://claude4u.com/en/guide/llm-api-format-comparison
- 2: https://awesomeagents.ai/migrations/openai-to-anthropic-api/
- 3: https://portkey.ai/blog/open-ai-responses-api-vs-chat-completions-vs-anthropic-anthropic-messages-api
- 4: https://platform.claude.com/docs/en/build-with-claude/working-with-messages
- 5: https://docs.aws.amazon.com/bedrock/latest/userguide/model-parameters-anthropic-claude-messages.html
- 6: https://platform.claude.com/docs/en/api/openai-sdk
- 7: https://stackoverflow.com/questions/76192496/openai-v1-completions-vs-v1-chat-completions-end-points
- 8: https://platform.claude.com/docs/en/api/beta/messages/create
- 9: https://docs.litellm.ai/docs/anthropic_unified/
- 10: https://dev.to/mukundakatta/400-from-anthropic-api-two-consecutive-user-messages-every-time-59c8
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs"
# Locate ParseOpenAiResponse method and print it
rg -n "ParseOpenAiResponse" "$FILE"
sed -n '220,420p' "$FILE" | cat -nRepository: Resgrid/Core
Length of output: 6105
Fix HttpClient lifetime, exception logging, and Anthropic compatibility in OpenAiCompatibleNluProvider
OpenAiCompatibleNluProvidercreates a newHttpClientin its constructor while being registered asInstancePerLifetimeScope; switch to a sharedHttpClient/IHttpClientFactory(per-request timeout viaCancellationToken) instead of per-scope instantiation.ClassifyAsyncandParseOpenAiResponsecatch exceptions and returnNLUResultwithout usingResgrid.Framework.Logging(useLogException(ex, ...)/LogErroras appropriate).ResolveEndpoint()routesCloudNluProviderType.Anthropicto/v1/messages, but the request/response logic uses OpenAI chat-completions semantics (model/messages/response_formatin the body andchoices[0].message.content+usage.total_tokensparsing); update request/response schema for Anthropic (or restrict Anthropic to an actual OpenAI-compatible endpoint).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs` at line
39, OpenAiCompatibleNluProvider currently creates a per-instance HttpClient and
mis-handles errors and Anthropic semantics; change the HttpClient handling to
use a shared client via IHttpClientFactory injected into the class (or a single
static/shared HttpClient) and stop creating new HttpClient in the constructor,
use CancellationToken timeouts passed into ClassifyAsync/request calls instead
of per-instance Timeout. Update exception handling in ClassifyAsync and
ParseOpenAiResponse to log via Resgrid.Framework.Logging (use LogException(ex,
"...") and LogError(...) where appropriate) and still return NLUResult on
failure. Fix ResolveEndpoint/response handling so Anthropic is either routed
only when the provider supports OpenAI chat-completion semantics or adapt the
request/response payload and parsing to Anthropic’s /v1/messages schema (or else
remove Anthropic mapping); update ParseOpenAiResponse to parse Anthropic
response fields when Anthropic is selected (instead of
choices[0].message.content and usage.total_tokens) or restrict Anthropic to
compatible endpoints.
| catch (Exception ex) | ||
| { | ||
| sw.Stop(); | ||
| return new NLUResult | ||
| { | ||
| IntentName = "unknown", | ||
| Confidence = 0, | ||
| ProviderName = ProviderName, | ||
| RawResponse = $"Cloud NLU error: {ex.Message.Truncate(200)}", | ||
| LatencyMs = sw.ElapsedMilliseconds | ||
| }; | ||
| } |
There was a problem hiding this comment.
Log swallowed exceptions.
The generic catch (and the JsonException catch in ParseOpenAiResponse) returns an unknown result but never records the underlying error, making cloud-NLU failures invisible in diagnostics. Capture via the framework logger.
🪵 Proposed fix
catch (Exception ex)
{
sw.Stop();
+ Resgrid.Framework.Logging.LogException(ex, "Cloud NLU classification failed.");
return new NLUResult
{
IntentName = "unknown",
Confidence = 0,
ProviderName = ProviderName,
RawResponse = $"Cloud NLU error: {ex.Message.Truncate(200)}",
LatencyMs = sw.ElapsedMilliseconds
};
}As per coding guidelines: "Use Resgrid.Framework.Logging.LogException(Exception ex, ...) when catching exceptions, as it automatically captures caller information via attributes".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch (Exception ex) | |
| { | |
| sw.Stop(); | |
| return new NLUResult | |
| { | |
| IntentName = "unknown", | |
| Confidence = 0, | |
| ProviderName = ProviderName, | |
| RawResponse = $"Cloud NLU error: {ex.Message.Truncate(200)}", | |
| LatencyMs = sw.ElapsedMilliseconds | |
| }; | |
| } | |
| catch (Exception ex) | |
| { | |
| sw.Stop(); | |
| Resgrid.Framework.Logging.LogException(ex, "Cloud NLU classification failed."); | |
| return new NLUResult | |
| { | |
| IntentName = "unknown", | |
| Confidence = 0, | |
| ProviderName = ProviderName, | |
| RawResponse = $"Cloud NLU error: {ex.Message.Truncate(200)}", | |
| LatencyMs = sw.ElapsedMilliseconds | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Chatbot.NLU/Providers/OpenAiCompatibleNluProvider.cs` around
lines 220 - 231, The catch blocks in OpenAiCompatibleNluProvider (the generic
catch around the NLU call and the JsonException catch in ParseOpenAiResponse)
currently swallow exceptions and return an "unknown" NLUResult; update those
catches to call Resgrid.Framework.Logging.LogException(ex,
"OpenAiCompatibleNluProvider: <brief context>") (or similar contextual message
referencing the method like ParseOpenAiResponse or the calling method) before
returning the NLUResult so the underlying exception and caller info are recorded
while preserving the existing return behavior.
| public CallsActionHandler( | ||
| ICallsService callsService, | ||
| IDepartmentsService departmentsService, | ||
| ICustomStateService customStateService, | ||
| IUserProfileService userProfileService) | ||
| { | ||
| _callsService = callsService; | ||
| _departmentsService = departmentsService; | ||
| _customStateService = customStateService; | ||
| _userProfileService = userProfileService; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Service Locator in the constructor to match project standard.
This constructor uses dependency injection directly, which conflicts with the repository’s C# convention for explicit resolution in constructors.
Proposed change
- public CallsActionHandler(
- ICallsService callsService,
- IDepartmentsService departmentsService,
- ICustomStateService customStateService,
- IUserProfileService userProfileService)
- {
- _callsService = callsService;
- _departmentsService = departmentsService;
- _customStateService = customStateService;
- _userProfileService = userProfileService;
- }
+ public CallsActionHandler()
+ {
+ _callsService = Bootstrapper.GetKernel().Resolve<ICallsService>();
+ _departmentsService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>();
+ _customStateService = Bootstrapper.GetKernel().Resolve<ICustomStateService>();
+ _userProfileService = Bootstrapper.GetKernel().Resolve<IUserProfileService>();
+ }As per coding guidelines, "**/*.cs: Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public CallsActionHandler( | |
| ICallsService callsService, | |
| IDepartmentsService departmentsService, | |
| ICustomStateService customStateService, | |
| IUserProfileService userProfileService) | |
| { | |
| _callsService = callsService; | |
| _departmentsService = departmentsService; | |
| _customStateService = customStateService; | |
| _userProfileService = userProfileService; | |
| } | |
| public CallsActionHandler() | |
| { | |
| _callsService = Bootstrapper.GetKernel().Resolve<ICallsService>(); | |
| _departmentsService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); | |
| _customStateService = Bootstrapper.GetKernel().Resolve<ICustomStateService>(); | |
| _userProfileService = Bootstrapper.GetKernel().Resolve<IUserProfileService>(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Chatbot/Handlers/CallsActionHandler.cs` around lines 20 - 30,
Replace constructor injection in CallsActionHandler with explicit service
resolution using the service locator: remove the constructor parameters and
inside the CallsActionHandler constructor assign _callsService,
_departmentsService, _customStateService, and _userProfileService by calling
Bootstrapper.GetKernel().Resolve<T>() for each corresponding service type
(ICallsService, IDepartmentsService, ICustomStateService, IUserProfileService)
so the class follows the repository standard of resolving dependencies inside
the constructor via the service locator.
| public async Task<ChatbotResponse> HandleAsync(ChatbotMessage message, ChatbotIntent intent, ChatbotSession session) | ||
| { | ||
| try | ||
| { | ||
| var department = await _departmentsService.GetDepartmentByIdAsync(session.DepartmentId); | ||
| var activeCalls = await _callsService.GetActiveCallsByDepartmentAsync(session.DepartmentId); |
There was a problem hiding this comment.
Guard session before dereferencing DepartmentId.
session.DepartmentId is accessed without a null check; a null session will throw and bypass intended handler behavior.
Proposed change
public async Task<ChatbotResponse> HandleAsync(ChatbotMessage message, ChatbotIntent intent, ChatbotSession session)
{
try
{
+ if (session == null)
+ {
+ return new ChatbotResponse
+ {
+ Text = "Unable to determine your department for this request.",
+ Processed = false
+ };
+ }
+
var department = await _departmentsService.GetDepartmentByIdAsync(session.DepartmentId);
var activeCalls = await _callsService.GetActiveCallsByDepartmentAsync(session.DepartmentId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Chatbot/Handlers/CallsActionHandler.cs` around lines 34 - 39, In
HandleAsync of CallsActionHandler (method signature: Task<ChatbotResponse>
HandleAsync(ChatbotMessage message, ChatbotIntent intent, ChatbotSession
session)), add a null guard for the incoming session before dereferencing
session.DepartmentId: if session is null (or session.DepartmentId is null/empty
depending on types) return an appropriate ChatbotResponse (or error/fallback)
and/or log the condition instead of calling
_departmentsService.GetDepartmentByIdAsync and
_callsService.GetActiveCallsByDepartmentAsync; ensure the guard occurs at the
top of the method so subsequent code assumes a non-null session and
DepartmentId.
| public ChatbotIngressService( | ||
| IChatbotUserIdentityService userIdentityService, | ||
| IChatbotSessionManager sessionManager, | ||
| IChatbotIntentRouter intentRouter, | ||
| IEnumerable<IChatbotActionHandler> actionHandlers, | ||
| IConversationEngine conversationEngine, | ||
| IChatbotTemplateRenderer templateRenderer, | ||
| IUserProfileService userProfileService, | ||
| IDepartmentsService departmentsService, | ||
| IDepartmentSettingsService departmentSettingsService, | ||
| ILimitsService limitsService, | ||
| IAuthorizationService authorizationService, | ||
| IChatbotDepartmentConfigService departmentConfigService, | ||
| IChatbotRateLimiter rateLimiter) | ||
| { | ||
| _userIdentityService = userIdentityService; | ||
| _sessionManager = sessionManager; | ||
| _intentRouter = intentRouter; | ||
| _actionHandlers = actionHandlers; | ||
| _conversationEngine = conversationEngine; | ||
| _templateRenderer = templateRenderer; | ||
| _userProfileService = userProfileService; | ||
| _departmentsService = departmentsService; | ||
| _departmentSettingsService = departmentSettingsService; | ||
| _limitsService = limitsService; | ||
| _authorizationService = authorizationService; | ||
| _departmentConfigService = departmentConfigService; | ||
| _rateLimiter = rateLimiter; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Constructor injection of 13 dependencies deviates from the repo's Service Locator convention.
This service takes 13 injected dependencies. The repository convention is to resolve dependencies via the kernel inside the constructor rather than constructor injection, and to keep dependency counts small. Consider resolving these through Bootstrapper.GetKernel().Resolve<T>() and/or splitting orchestration responsibilities to reduce coupling.
As per coding guidelines: "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection" and "Minimize constructor injection; keep the number of injected dependencies small".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 29 - 57,
The ChatbotIngressService constructor currently injects 13 dependencies
(parameters and backing fields like _userIdentityService, _sessionManager,
_intentRouter, _actionHandlers, _conversationEngine, _templateRenderer,
_userProfileService, _departmentsService, _departmentSettingsService,
_limitsService, _authorizationService, _departmentConfigService, _rateLimiter);
change this to resolve those services inside the constructor using the Service
Locator pattern (call Bootstrapper.GetKernel().Resolve<T>() for each needed
type) or refactor responsibilities to reduce coupling (extract orchestration
into smaller services and only resolve/inject the smaller set), updating the
constructor and assignments in ChatbotIngressService accordingly so the
parameter list is minimal and fields are populated via resolved instances.
| builder.RegisterType<DiscordBotAdapter>() | ||
| .As<IChatbotPlatformAdapter>() | ||
| .InstancePerLifetimeScope(); | ||
|
|
||
| builder.RegisterType<SlackBotAdapter>() | ||
| .As<IChatbotPlatformAdapter>() | ||
| .InstancePerLifetimeScope(); | ||
|
|
||
| builder.RegisterType<TelegramBotAdapter>() | ||
| .As<IChatbotPlatformAdapter>() | ||
| .InstancePerLifetimeScope(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for an implementation/registration of the config interface
rg -nP 'IChatbotPlatformAdapterConfig' --type=cs -C3
# Confirm adapters are consumed via IEnumerable selection by Platform
rg -nP 'IEnumerable<\s*IChatbotPlatformAdapter\s*>|Resolve<[^>]*IChatbotPlatformAdapter' --type=cs -C3Repository: Resgrid/Core
Length of output: 4380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find any implementations of IChatbotPlatformAdapterConfig
rg -n --type=cs "class\s+\w+\s*:\s*.*IChatbotPlatformAdapterConfig|IChatbotPlatformAdapterConfig\s*\{" .
# 2) Find registrations of IChatbotPlatformAdapterConfig
rg -n --type=cs "Register(Type|Instance)|As<|IChatbotPlatformAdapterConfig" Providers/Resgrid.Providers.Chatbot -S
rg -n --type=cs "As<\s*IChatbotPlatformAdapterConfig\s*>|IChatbotPlatformAdapterConfig\s*>" . -S
# 3) Confirm how IChatbotPlatformAdapter is resolved/consumed
rg -n --type=cs "Resolve<[^>]*IChatbotPlatformAdapter|Resolve<\s*IEnumerable<\s*IChatbotPlatformAdapter|IEnumerable<\s*IChatbotPlatformAdapter\s*>" . -SRepository: Resgrid/Core
Length of output: 38
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any implementations by looking for the required methods
rg -n --type=cs "string\s+GetToken\s*\(\s*ChatbotPlatform\s+platform\s*\)|string\s+GetBotId\s*\(\s*ChatbotPlatform\s+platform\s*\)" . -S
# Find all references to the interface name (to locate any implementation + registrations)
rg -n --type=cs "IChatbotPlatformAdapterConfig" . -S
# Specifically check for any DI registration of the config interface around the chatbot provider module
rg -n --type=cs "ChatbotProviderModule|ChatbotProvider" Providers/Resgrid.Providers.Chatbot -S
rg -n --type=cs "IChatbotPlatformAdapterConfig" Providers/Resgrid.Providers.Chatbot -SRepository: Resgrid/Core
Length of output: 2208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Any class/record implementing the config interface
rg -n --type=cs ":\s*IChatbotPlatformAdapterConfig\b" . -S || true
# Any methods that match the config interface shape (likely implementations)
rg -n --type=cs "string\s+GetToken\s*\(\s*ChatbotPlatform\s+platform\s*\)" . -S || true
rg -n --type=cs "string\s+GetBotId\s*\(\s*ChatbotPlatform\s+platform\s*\)" . -S || true
# Any DI registrations mentioning the config interface (any module)
rg -n --type=cs "IChatbotPlatformAdapterConfig" . -S || trueRepository: Resgrid/Core
Length of output: 1202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Implementation candidates for config interface (method shape)
rg -n --type=cs "GetToken\s*\(\s*ChatbotPlatform\s+[A-Za-z_]\w*\s*\)\s*=>" . -S || true
rg -n --type=cs "GetToken\s*\(\s*ChatbotPlatform\s+[A-Za-z_]\w*\s*\)\s*\{" . -S || true
rg -n --type=cs "GetBotId\s*\(\s*ChatbotPlatform\s+[A-Za-z_]\w*\s*\)\s*=>" . -S || true
rg -n --type=cs "GetBotId\s*\(\s*ChatbotPlatform\s+[A-Za-z_]\w*\s*\)\s*\{" . -S || true
# Any DI registration of the config interface
rg -n --type=cs "IChatbotPlatformAdapterConfig" . -S || true
# Any usage that resolves IChatbotPlatformAdapter directly (not IEnumerable)
rg -n --type=cs "Resolve<\s*IChatbotPlatformAdapter\s*>" . -S || true
rg -n --type=cs "GetService<\s*IChatbotPlatformAdapter\s*>" . -S || true
rg -n --type=cs "IChatbotPlatformAdapter\s+\w+\s*=" . -S || true
# Confirm IEnumerable usage
rg -n --type=cs "IEnumerable<\s*IChatbotPlatformAdapter\s*>" . -S || trueRepository: Resgrid/Core
Length of output: 1350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant DI registrations
nl -ba Providers/Resgrid.Providers.Chatbot/ChatbotProviderModule.cs | sed -n '1,120p'
# Show how adapters are selected/used in the controller
nl -ba Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs | sed -n '1,140p'Repository: Resgrid/Core
Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- ChatbotProviderModule.cs ----"
cat -n Providers/Resgrid.Providers.Chatbot/ChatbotProviderModule.cs | sed -n '1,160p'
echo
echo "---- ChatbotTelegramController.cs ----"
cat -n Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs | sed -n '1,220p'Repository: Resgrid/Core
Length of output: 7764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any DI registrations of the config interface
rg -n --type=cs "As<\s*IChatbotPlatformAdapterConfig\s*>|Register.*IChatbotPlatformAdapterConfig|RegisterInstance.*IChatbotPlatformAdapterConfig|Resolve<\s*IChatbotPlatformAdapterConfig\s*>" . -S || true
# Look for any class/record implementing the config interface
rg -n --type=cs ":\s*IChatbotPlatformAdapterConfig\b" . -S || true
# Look for any direct resolution/injection of IChatbotPlatformAdapter (non-Enumerable)
rg -n --type=cs "\bIChatbotPlatformAdapter\b" . -S | rg -v "IEnumerable<\s*IChatbotPlatformAdapter\s*>|interface\s+IChatbotPlatformAdapter\b" || trueRepository: Resgrid/Core
Length of output: 1418
Register IChatbotPlatformAdapterConfig in DI to prevent chatbot adapter resolution failures
DiscordBotAdapter, SlackBotAdapter, and TelegramBotAdapter all require IChatbotPlatformAdapterConfig in their constructors, but ChatbotProviderModule only registers IChatbotPlatformAdapter implementations and no IChatbotPlatformAdapterConfig implementation/registration is present in the codebase. Since ChatbotTelegramController requests IEnumerable<IChatbotPlatformAdapter>, Autofac will attempt to construct these adapters and fail at runtime without the config dependency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Providers/Resgrid.Providers.Chatbot/ChatbotProviderModule.cs` around lines 21
- 31, ChatbotProviderModule currently registers DiscordBotAdapter,
SlackBotAdapter, and TelegramBotAdapter (all of which depend on
IChatbotPlatformAdapterConfig) but never registers
IChatbotPlatformAdapterConfig, causing Autofac to fail when
ChatbotTelegramController resolves IEnumerable<IChatbotPlatformAdapter>; to fix,
add a registration for an implementation or instance of
IChatbotPlatformAdapterConfig in ChatbotProviderModule (e.g. register a
ChatbotPlatformAdapterConfig concrete type or a factory/instance) and choose
appropriate lifetime (SingleInstance or InstancePerLifetimeScope) so the
adapters can be constructed successfully.
| public async Task<IActionResult> Webhook() | ||
| { | ||
| // Verify the secret token Telegram echoes back in the X-Telegram-Bot-Api-Secret-Token | ||
| // header (configured via setWebhook). When a secret is configured, requests without a | ||
| // matching token are rejected so the webhook cannot be driven by arbitrary callers. | ||
| var configuredSecret = ChatbotConfig.TelegramWebhookSecretToken; | ||
| if (!string.IsNullOrEmpty(configuredSecret)) | ||
| { | ||
| var providedSecret = Request.Headers["X-Telegram-Bot-Api-Secret-Token"].ToString(); | ||
| if (!SecretMatches(providedSecret, configuredSecret)) | ||
| return Unauthorized(); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| using var reader = new StreamReader(Request.Body); | ||
| var body = await reader.ReadToEndAsync(); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(body)) | ||
| return Ok("ok"); | ||
|
|
||
| // Find the Telegram adapter | ||
| IChatbotPlatformAdapter telegramAdapter = null; | ||
| foreach (var adapter in _adapters) | ||
| { | ||
| if (adapter.Platform == ChatbotPlatform.Telegram) | ||
| { | ||
| telegramAdapter = adapter; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (telegramAdapter == null) | ||
| return Ok("ok"); // Telegram adapter not configured | ||
|
|
||
| // Parse the Telegram Update to extract message parameters | ||
| var parameters = new Dictionary<string, string>(); | ||
| ParseTelegramUpdate(body, parameters); | ||
|
|
||
| if (!parameters.ContainsKey("text") || string.IsNullOrWhiteSpace(parameters["text"])) | ||
| return Ok("ok"); // Non-text update (e.g., sticker, photo) - ignore | ||
|
|
||
| // Convert to ChatbotMessage | ||
| var chatbotMessage = await telegramAdapter.ParseInboundMessageAsync(parameters); | ||
|
|
||
| // Process through the chatbot pipeline | ||
| var response = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | ||
|
|
||
| // Format and send response via Telegram adapter | ||
| var formattedResponse = await telegramAdapter.FormatOutboundResponseAsync(response); | ||
|
|
||
| return Ok("ok"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); | ||
| return Ok("ok"); // Always return 200 to Telegram | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if FormatOutboundResponseAsync is expected to send the message as a side effect
# or if the return value should be handled
rg -n -A5 -B5 'FormatOutboundResponseAsync' --type=csRepository: Resgrid/Core
Length of output: 7342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect TelegramBotAdapter.cs around FormatOutboundResponseAsync
tele_adapter_file="$(rg -l --type=cs 'class TelegramBotAdapter' Providers/Resgrid.Providers.Chatbot/Adapters | head -n 1)"
echo "Telegram adapter file: $tele_adapter_file"
sed -n '1,200p' "$tele_adapter_file"
# 2) Check other parts of TelegramBotAdapter for any outbound send calls
rg -n --type=cs -i 'send|sendmessage|send_message|SendMessage|Telegram\.|TelegramBotClient|BotClient|ITelegram|Answer|reply|EditMessage|Send' "$tele_adapter_file" || true
# 3) Find if formattedResponse is used anywhere in controller (or send happens elsewhere in the pipeline)
rg -n --type=cs 'formattedResponse|FormatOutboundResponseAsync' Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs
sed -n '1,220p' Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs
# 4) Search for Telegram delivery in the repo (quick scan for common Telegram library calls)
rg -n --type=cs -i 'Telegram\.Bot|SendMessage|sendMessage|ITelegramBot|TelegramBotClient' Providers Web || trueRepository: Resgrid/Core
Length of output: 33097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where TelegramBotAdapter.FormatOutboundResponseAsync is used
rg -n --type=cs 'FormatOutboundResponseAsync\(' .
# Find where SendResponseAsync is called (TelegramBotAdapter has it, but controller doesn't)
rg -n --type=cs 'SendResponseAsync\(' .
# Locate IChatbotIngressService.ProcessMessageAsync implementation to see if it triggers delivery
rg -n --type=cs 'ProcessMessageAsync\(' Providers Web
# Narrow: find ProcessMessageAsync in Providers/Resgrid.Providers.Chatbot (or similar)
rg -n --type=cs 'class .*Chatbot.*Ingress' Providers Web || trueRepository: Resgrid/Core
Length of output: 2049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect IChatbotIngressService + its implementation(s)
rg -n --type=cs 'interface IChatbotIngressService|class .*Chatbot.*Ingress|ProcessMessageAsync\(' Providers Web
# Print the interface and any implementation files found for ProcessMessageAsync
rg -n --type=cs 'Task<ChatbotResponse>\s+ProcessMessageAsync' Providers Web || true
# Locate the specific implementation file(s) for ProcessMessageAsync
impl_files=$(rg -l --type=cs 'ProcessMessageAsync\(' Providers Web | tr '\n' ' ')
echo "Implementation candidate files: $impl_files"
for f in $impl_files; do
echo "---- $f (top/around ProcessMessageAsync) ----"
sed -n '1,220p' "$f" | sed -n '1,60p'
rg -n --type=cs 'ProcessMessageAsync\(' "$f" -n || true
# show 80 lines around first match
line=$(rg -n --type=cs 'ProcessMessageAsync\(' "$f" | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-40)); end=$((line+80))
sed -n "${start},${end}p" "$f"
fi
done
# 2) Inspect TwilioController / SignalWireController to see how they deliver responses
for controller in Web/Resgrid.Web.Services/Controllers/TwilioController.cs Web/Resgrid.Web.Services/Controllers/SignalWireController.cs; do
if [ -f "$controller" ]; then
echo "---- $controller (around ProcessMessageAsync call) ----"
line=$(rg -n --type=cs 'ProcessMessageAsync\(' "$controller" | head -n1 | cut -d: -f1)
start=$((line-60)); end=$((line+120))
sed -n "${start},${end}p" "$controller"
fi
done
# 3) Inspect IChatbotPlatformAdapter for SendResponseAsync usage
rg -n --type=cs 'SendResponseAsync\(' Providers/Resgrid.Providers.Chatbot/Interfaces/IChatbotPlatformAdapter.cs
sed -n '1,120p' Providers/Resgrid.Providers.Chatbot/Interfaces/IChatbotPlatformAdapter.csRepository: Resgrid/Core
Length of output: 32862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate IChatbotIngressService definition + implementations
rg -n --type=cs 'interface IChatbotIngressService|class .*Chatbot.*Ingress|ProcessMessageAsync\(' Providers Web | head -n 50
# Print the interface
iface="$(rg -l --type=cs 'interface IChatbotIngressService' Providers Web | head -n 1)"
echo "Interface: $iface"
sed -n '1,200p' "$iface"
# Print ProcessMessageAsync implementations (most likely in Providers/Resgrid.Providers.Chatbot or similar)
mapfile -t impls < <(rg -l --type=cs 'Task<ChatbotResponse>\s+ProcessMessageAsync' Providers Web || true)
echo "Implementations found: ${`#impls`[@]}"
for f in "${impls[@]}"; do
echo "---- $f ----"
sed -n '1,220p' "$f"
done
# Also search for SendResponseAsync usage inside chatbot ingress/pipeline
rg -n --type=cs 'SendResponseAsync\(' Providers Web || trueRepository: Resgrid/Core
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where IChatbotIngressService is defined
rg -n --type=cs 'interface IChatbotIngressService' .
# Find all ProcessMessageAsync declarations (implementation signatures)
rg -n --type=cs 'ProcessMessageAsync\s*\(' .
# Focus: ChatbotResponse return type
rg -n --type=cs 'Task<\s*ChatbotResponse\s*>\s+ProcessMessageAsync' .
# Find common class names
rg -n --type=cs 'Chatbot.*Ingress|Ingress.*Chatbot' .
# Where SendResponseAsync is used (to see if delivery happens in the pipeline)
rg -n --type=cs 'SendResponseAsync\(' .Repository: Resgrid/Core
Length of output: 3228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect IChatbotIngressService interface
sed -n '1,120p' Core/Resgrid.Chatbot/Interfaces/IChatbotIngressService.cs
# Inspect ChatbotIngressService.ProcessMessageAsync
sed -n '1,220p' Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs
# Inspect ChatbotResponse + Processed flag to see how delivery is handled
rg -n --type=cs 'class ChatbotResponse|Processed' Core/Resgrid.Chatbot Providers/Resgrid.Providers.Chatbot | head -n 50
# show ChatbotResponse definition
resp_file="$(rg -l --type=cs 'class ChatbotResponse' Core/Resgrid.Chatbot Providers/Resgrid.Providers.Chatbot | head -n 1)"
echo "ChatbotResponse file: $resp_file"
sed -n '1,200p' "$resp_file"
# Inspect IChatbotPlatformAdapter to see expected responsibilities
sed -n '1,120p' Providers/Resgrid.Providers.Chatbot/Interfaces/IChatbotPlatformAdapter.cs
# Inspect TelegramBotAdapter.SendResponseAsync implementation
sed -n '60,120p' Providers/Resgrid.Providers.Chatbot/Adapters/TelegramBotAdapter.cs
# Search for SendResponseAsync usage in ChatbotIngressService or elsewhere
rg -n --type=cs 'SendResponseAsync\(' Core/Resgrid.Chatbot Providers/Resgrid.Providers.Chatbot . | head -n 100Repository: Resgrid/Core
Length of output: 20532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs"
# Print tail of file to include the rest of ProcessMessageAsync
wc -l "$file"
sed -n '220,520p' "$file"
# Search within the file for SendResponseAsync / FormatOutboundResponseAsync usage
rg -n --type=cs 'SendResponseAsync|FormatOutboundResponseAsync|IChatbotPlatformAdapter' "$file"Repository: Resgrid/Core
Length of output: 5520
Fix Telegram outbound delivery: FormatOutboundResponseAsync only formats and its result is ignored
ChatbotTelegramController.WebhookawaitstelegramAdapter.FormatOutboundResponseAsync(response)intoformattedResponsebut never uses it.TelegramBotAdapter.FormatOutboundResponseAsyncjust returnsresponse?.Text(no send), and the actualSendResponseAsync(...)method is never called (and is currently a stub).- Add/standardize an adapter method responsible for sending (and implement Telegram sending), then handle failures instead of returning
Ok("ok")without delivery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/ChatbotTelegramController.cs` around
lines 48 - 106, The webhook currently formats a Telegram reply but never sends
it: in ChatbotTelegramController.Webhook you await
IChatbotPlatformAdapter.FormatOutboundResponseAsync(response) into
formattedResponse but never call a send method, and
TelegramBotAdapter.SendResponseAsync is a stub; fix by adding/standardizing a
send API on the adapter (e.g., IChatbotPlatformAdapter.SendResponseAsync or
ensure FormatOutboundResponseAsync returns a payload and implement
TelegramBotAdapter.SendResponseAsync to perform the HTTP call to Telegram),
update ChatbotTelegramController.Webhook to call that send method with
formattedResponse (handle async result, log failures via Logging.LogException or
similar), and return success only when send succeeds (or return a
500/appropriate status and log on failure) instead of always returning Ok("ok")
without delivery.
| var result = await _chatbotIngressService.ProcessMessageAsync(request); | ||
|
|
||
| messageEvent.Processed = true; | ||
| response = LaMLResponse.Message.Respond(result.Text); |
There was a problem hiding this comment.
Potential NullReferenceException if chatbot service returns null.
ProcessMessageAsync result is dereferenced without null check. If the service returns null, accessing result.Text will throw. Unlike the exception catch block which handles failures gracefully, this path would crash silently within the try block.
🛡️ Proposed fix to add null handling
var result = await _chatbotIngressService.ProcessMessageAsync(request);
-messageEvent.Processed = true;
-response = LaMLResponse.Message.Respond(result.Text);
+if (result != null)
+{
+ messageEvent.Processed = result.Processed;
+ response = LaMLResponse.Message.Respond(result.Text);
+}
+else
+{
+ response = LaMLResponse.Message.Respond("Unable to process your request at this time.");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = await _chatbotIngressService.ProcessMessageAsync(request); | |
| messageEvent.Processed = true; | |
| response = LaMLResponse.Message.Respond(result.Text); | |
| var result = await _chatbotIngressService.ProcessMessageAsync(request); | |
| if (result != null) | |
| { | |
| messageEvent.Processed = result.Processed; | |
| response = LaMLResponse.Message.Respond(result.Text); | |
| } | |
| else | |
| { | |
| response = LaMLResponse.Message.Respond("Unable to process your request at this time."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/SignalWireController.cs` around lines
255 - 258, The code in SignalWireController is dereferencing the result of
_chatbotIngressService.ProcessMessageAsync(request) without a null check, so if
ProcessMessageAsync returns null accessing result.Text will throw; modify the
block in the SignalWireController method that calls
_chatbotIngressService.ProcessMessageAsync(request) to check if result is null
before using result.Text (e.g., set a safe fallback message or log and mark
messageEvent.Processed appropriately), then call LaMLResponse.Message.Respond
with the safe text; ensure messageEvent.Processed is still set when appropriate
and handle/log the null case consistently with the existing exception handling.
| var chatbotResponse = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | ||
|
|
||
| messageEvent.Processed = true; | ||
| } | ||
|
|
||
| if (!isDispatchSource) | ||
| { | ||
| // Reuse the profile fetched above when the department was resolved via mobile number; | ||
| // only hit the DB again if the department came from the phone-number lookup path. | ||
| var profile = userProfile ?? await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | ||
| if (chatbotResponse.Processed) | ||
| messageEvent.Processed = true; | ||
|
|
||
| if (profile != null) | ||
| { | ||
| var payload = _textCommandService.DetermineType(textMessage.Text); | ||
| var customActions = customStates.FirstOrDefault(x => x.Type == (int)CustomStateTypes.Personnel); | ||
| var customStaffing = customStates.FirstOrDefault(x => x.Type == (int)CustomStateTypes.Staffing); | ||
|
|
||
| switch (payload.Type) | ||
| { | ||
| case TextCommandTypes.None: | ||
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | ||
| break; | ||
| case TextCommandTypes.Help: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var help = new StringBuilder(); | ||
| help.Append("Resgrid Text Commands" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("These are the commands you can text to alter your status and staffing. Text help for help." + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("Core Commands" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | ||
| help.Append("HELP: This help text" + Environment.NewLine); | ||
| help.Append("CALLS: List active calls" + Environment.NewLine); | ||
| help.Append("C[CallId]: Get Call Detail i.e. C1445" + Environment.NewLine); | ||
| help.Append("UNITS: List unit statuses" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("Status or Action Commands" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
|
|
||
| if (customActions != null && customActions.IsDeleted == false && customActions.GetActiveDetails() != null && customActions.GetActiveDetails().Any()) | ||
| { | ||
| var activeDetails = customActions.GetActiveDetails(); | ||
| for (int i = 0; i < activeDetails.Count; i++) | ||
| { | ||
| help.Append($"{activeDetails[i].ButtonText.Trim().Replace(" ", "").Replace("-", "").Replace(":", "")} or {i + 1}: {activeDetails[i].ButtonText}" + Environment.NewLine); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| help.Append("responding or 1: Responding" + Environment.NewLine); | ||
| help.Append("notresponding or 2: Not Responding" + Environment.NewLine); | ||
| help.Append("onscene or 3: On Scene" + Environment.NewLine); | ||
| help.Append("available or 4: Available" + Environment.NewLine); | ||
| } | ||
|
|
||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("Staffing Commands" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
|
|
||
| if (customStaffing != null && customStaffing.IsDeleted == false && customStaffing.GetActiveDetails() != null && customStaffing.GetActiveDetails().Any()) | ||
| { | ||
| var activeDetails = customStaffing.GetActiveDetails(); | ||
| for (int i = 0; i < activeDetails.Count; i++) | ||
| { | ||
| help.Append($"{activeDetails[i].ButtonText.Trim().Replace(" ", "").Replace("-", "").Replace(":", "")} or S{i + 1}: {activeDetails[i].ButtonText}" + Environment.NewLine); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| help.Append("available or s1: Available Staffing" + Environment.NewLine); | ||
| help.Append("delayed or s2: Delayed Staffing" + Environment.NewLine); | ||
| help.Append("unavailable or s3: Unavailable Staffing" + Environment.NewLine); | ||
| help.Append("committed or s4: Committed Staffing" + Environment.NewLine); | ||
| help.Append("onshift or s4: On Shift Staffing" + Environment.NewLine); | ||
| } | ||
|
|
||
| response.Message(help.ToString()); | ||
|
|
||
| //_communicationService.SendTextMessage(profile.UserId, "Resgrid TCI Help", help.ToString(), department.DepartmentId, textMessage.To, profile); | ||
| break; | ||
| case TextCommandTypes.Action: | ||
| messageEvent.Processed = true; | ||
| await _actionLogsService.SetUserActionAsync(profile.UserId, department.DepartmentId, (int)payload.GetActionType()); | ||
| response.Message(string.Format("Resgrid received your text command. Status changed to: {0}", payload.GetActionType())); | ||
| //_communicationService.SendTextMessage(profile.UserId, "Resgrid TCI Status", string.Format("Resgrid recieved your text command. Status changed to: {0}", payload.GetActionType()), department.DepartmentId, textMessage.To, profile); | ||
| break; | ||
| case TextCommandTypes.Staffing: | ||
| messageEvent.Processed = true; | ||
| await _userStateService.CreateUserState(profile.UserId, department.DepartmentId, (int)payload.GetStaffingType()); | ||
| response.Message(string.Format("Resgrid received your text command. Staffing level changed to: {0}", payload.GetStaffingType())); | ||
| //_communicationService.SendTextMessage(profile.UserId, "Resgrid TCI Staffing", string.Format("Resgrid recieved your text command. Staffing level changed to: {0}", payload.GetStaffingType()), department.DepartmentId, textMessage.To, profile); | ||
| break; | ||
| case TextCommandTypes.Stop: | ||
| messageEvent.Processed = true; | ||
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | ||
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); | ||
| break; | ||
| case TextCommandTypes.CustomAction: | ||
| messageEvent.Processed = true; | ||
| await _actionLogsService.SetUserActionAsync(profile.UserId, department.DepartmentId, payload.GetCustomActionType()); | ||
|
|
||
| if (customActions != null && customActions.IsDeleted == false && customActions.GetActiveDetails() != null && customActions.GetActiveDetails().Any() && | ||
| customActions.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomActionType()) != null) | ||
| { | ||
| var detail = customActions.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomActionType()); | ||
| response.Message(string.Format("Resgrid received your text command. Status changed to: {0}", detail.ButtonText)); | ||
| } | ||
| else | ||
| { | ||
| response.Message("Resgrid received your text command and updated your status"); | ||
| } | ||
|
|
||
| break; | ||
| case TextCommandTypes.CustomStaffing: | ||
| messageEvent.Processed = true; | ||
| await _userStateService.CreateUserState(profile.UserId, department.DepartmentId, payload.GetCustomStaffingType()); | ||
|
|
||
| if (customStaffing != null && customStaffing.IsDeleted == false && customStaffing.GetActiveDetails() != null && customStaffing.GetActiveDetails().Any() && | ||
| customStaffing.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomStaffingType()) != null) | ||
| { | ||
| var detail = customStaffing.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomStaffingType()); | ||
| response.Message(string.Format("Resgrid received your text command. Staffing changed to: {0}", detail.ButtonText)); | ||
| } | ||
| else | ||
| { | ||
| response.Message("Resgrid received your text command and updated your staffing"); | ||
| } | ||
|
|
||
| break; | ||
| case TextCommandTypes.MyStatus: | ||
| messageEvent.Processed = true; | ||
|
|
||
|
|
||
| var userStatus = await _actionLogsService.GetLastActionLogForUserAsync(profile.UserId); | ||
| var userStaffing = await _userStateService.GetLastUserStateByUserIdAsync(profile.UserId); | ||
|
|
||
| var customStatusLevel = await _customStateService.GetCustomPersonnelStatusAsync(department.DepartmentId, userStatus); | ||
| var customStaffingLevel = await _customStateService.GetCustomPersonnelStaffingAsync(department.DepartmentId, userStaffing); | ||
|
|
||
| response.Message( | ||
| $"Hello {profile.FullName.AsFirstNameLastName} at {DateTime.UtcNow.TimeConverterToString(department)} your current status is {customStatusLevel.ButtonText} and your current staffing is {customStaffingLevel.ButtonText}."); | ||
| break; | ||
| case TextCommandTypes.Calls: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var activeCalls = await _callsService.GetActiveCallsByDepartmentAsync(department.DepartmentId); | ||
|
|
||
| var activeCallText = new StringBuilder(); | ||
| activeCallText.Append($"Active Calls for {department.Name}" + Environment.NewLine); | ||
| activeCallText.Append("---------------------" + Environment.NewLine); | ||
|
|
||
| foreach (var activeCall in activeCalls) | ||
| { | ||
| activeCallText.Append($"CallId: {activeCall.CallId} Name: {activeCall.Name} Nature:{StringHelpers.StripHtmlTagsCharArray(activeCall.NatureOfCall)}" + Environment.NewLine); | ||
| } | ||
|
|
||
| response.Message(activeCallText.ToString().Truncate(1200)); | ||
| break; | ||
| case TextCommandTypes.Units: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var unitStatus = await _unitsService.GetAllLatestStatusForUnitsByDepartmentIdAsync(department.DepartmentId); | ||
|
|
||
| var unitStatusesText = new StringBuilder(); | ||
| unitStatusesText.Append($"Unit Statuses for {department.Name}" + Environment.NewLine); | ||
| unitStatusesText.Append("---------------------" + Environment.NewLine); | ||
|
|
||
| foreach (var unit in unitStatus) | ||
| { | ||
| var unitState = await _customStateService.GetCustomUnitStateAsync(unit); | ||
| unitStatusesText.Append($"{unit.Unit.Name} is {unitState.ButtonText}" + Environment.NewLine); | ||
| } | ||
|
|
||
| response.Message(unitStatusesText.ToString().Truncate(1200)); | ||
| break; | ||
| case TextCommandTypes.CallDetail: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var call = await _callsService.GetCallByIdAsync(int.Parse(payload.Data)); | ||
|
|
||
| var callText = new StringBuilder(); | ||
| callText.Append($"Call Information for {call.Name}" + Environment.NewLine); | ||
| callText.Append("---------------------" + Environment.NewLine); | ||
| callText.Append($"Id: {call.CallId}" + Environment.NewLine); | ||
| callText.Append($"Number: {call.Number}" + Environment.NewLine); | ||
| callText.Append($"Logged: {call.LoggedOn.TimeConverterToString(department)}" + Environment.NewLine); | ||
| callText.Append("-----Nature-----" + Environment.NewLine); | ||
| callText.Append(call.NatureOfCall + Environment.NewLine); | ||
| callText.Append("-----Address-----" + Environment.NewLine); | ||
|
|
||
| if (!String.IsNullOrWhiteSpace(call.Address)) | ||
| callText.Append(call.Address + Environment.NewLine); | ||
| else if (!string.IsNullOrEmpty(call.GeoLocationData) && call.GeoLocationData.Length > 1) | ||
| { | ||
| try | ||
| { | ||
| string[] points = call.GeoLocationData.Split(char.Parse(",")); | ||
|
|
||
| if (points != null && points.Length == 2) | ||
| { | ||
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| } | ||
| } | ||
|
|
||
| response.Message(callText.ToString()); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| else if (textMessage.To == Config.NumberProviderConfig.TwilioResgridNumber) // Resgrid master text number | ||
| { | ||
| var profile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | ||
| var payload = _textCommandService.DetermineType(textMessage.Text); | ||
|
|
||
| switch (payload.Type) | ||
| { | ||
| case TextCommandTypes.None: | ||
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | ||
| break; | ||
| case TextCommandTypes.Help: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var help = new StringBuilder(); | ||
| help.Append("Resgrid Text Commands" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("This is the Resgrid system for first responders (https://resgrid.com) automated text system. Your department isn't signed up for inbound text messages, but you can send the following commands." + | ||
| Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | ||
| help.Append("HELP: This help text" + Environment.NewLine); | ||
|
|
||
| response.Message(help.ToString()); | ||
|
|
||
| break; | ||
| case TextCommandTypes.Stop: | ||
| messageEvent.Processed = true; | ||
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | ||
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); | ||
| break; | ||
| } | ||
| } | ||
| response.Message(chatbotResponse.Text); |
There was a problem hiding this comment.
Potential NullReferenceException if chatbot service returns null.
Same issue as SignalWireController: chatbotResponse is dereferenced without null check. While the catch block provides a fallback message for exceptions, a null return would cause a NullReferenceException before the catch handles it gracefully.
🛡️ Proposed fix to add null handling
var chatbotResponse = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage);
-if (chatbotResponse.Processed)
- messageEvent.Processed = true;
-
-response.Message(chatbotResponse.Text);
+if (chatbotResponse != null)
+{
+ if (chatbotResponse.Processed)
+ messageEvent.Processed = true;
+
+ response.Message(chatbotResponse.Text);
+}
+else
+{
+ response.Message("Unable to process your request at this time.");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var chatbotResponse = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | |
| messageEvent.Processed = true; | |
| } | |
| if (!isDispatchSource) | |
| { | |
| // Reuse the profile fetched above when the department was resolved via mobile number; | |
| // only hit the DB again if the department came from the phone-number lookup path. | |
| var profile = userProfile ?? await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | |
| if (chatbotResponse.Processed) | |
| messageEvent.Processed = true; | |
| if (profile != null) | |
| { | |
| var payload = _textCommandService.DetermineType(textMessage.Text); | |
| var customActions = customStates.FirstOrDefault(x => x.Type == (int)CustomStateTypes.Personnel); | |
| var customStaffing = customStates.FirstOrDefault(x => x.Type == (int)CustomStateTypes.Staffing); | |
| switch (payload.Type) | |
| { | |
| case TextCommandTypes.None: | |
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | |
| break; | |
| case TextCommandTypes.Help: | |
| messageEvent.Processed = true; | |
| var help = new StringBuilder(); | |
| help.Append("Resgrid Text Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("These are the commands you can text to alter your status and staffing. Text help for help." + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("Core Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | |
| help.Append("HELP: This help text" + Environment.NewLine); | |
| help.Append("CALLS: List active calls" + Environment.NewLine); | |
| help.Append("C[CallId]: Get Call Detail i.e. C1445" + Environment.NewLine); | |
| help.Append("UNITS: List unit statuses" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("Status or Action Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| if (customActions != null && customActions.IsDeleted == false && customActions.GetActiveDetails() != null && customActions.GetActiveDetails().Any()) | |
| { | |
| var activeDetails = customActions.GetActiveDetails(); | |
| for (int i = 0; i < activeDetails.Count; i++) | |
| { | |
| help.Append($"{activeDetails[i].ButtonText.Trim().Replace(" ", "").Replace("-", "").Replace(":", "")} or {i + 1}: {activeDetails[i].ButtonText}" + Environment.NewLine); | |
| } | |
| } | |
| else | |
| { | |
| help.Append("responding or 1: Responding" + Environment.NewLine); | |
| help.Append("notresponding or 2: Not Responding" + Environment.NewLine); | |
| help.Append("onscene or 3: On Scene" + Environment.NewLine); | |
| help.Append("available or 4: Available" + Environment.NewLine); | |
| } | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("Staffing Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| if (customStaffing != null && customStaffing.IsDeleted == false && customStaffing.GetActiveDetails() != null && customStaffing.GetActiveDetails().Any()) | |
| { | |
| var activeDetails = customStaffing.GetActiveDetails(); | |
| for (int i = 0; i < activeDetails.Count; i++) | |
| { | |
| help.Append($"{activeDetails[i].ButtonText.Trim().Replace(" ", "").Replace("-", "").Replace(":", "")} or S{i + 1}: {activeDetails[i].ButtonText}" + Environment.NewLine); | |
| } | |
| } | |
| else | |
| { | |
| help.Append("available or s1: Available Staffing" + Environment.NewLine); | |
| help.Append("delayed or s2: Delayed Staffing" + Environment.NewLine); | |
| help.Append("unavailable or s3: Unavailable Staffing" + Environment.NewLine); | |
| help.Append("committed or s4: Committed Staffing" + Environment.NewLine); | |
| help.Append("onshift or s4: On Shift Staffing" + Environment.NewLine); | |
| } | |
| response.Message(help.ToString()); | |
| //_communicationService.SendTextMessage(profile.UserId, "Resgrid TCI Help", help.ToString(), department.DepartmentId, textMessage.To, profile); | |
| break; | |
| case TextCommandTypes.Action: | |
| messageEvent.Processed = true; | |
| await _actionLogsService.SetUserActionAsync(profile.UserId, department.DepartmentId, (int)payload.GetActionType()); | |
| response.Message(string.Format("Resgrid received your text command. Status changed to: {0}", payload.GetActionType())); | |
| //_communicationService.SendTextMessage(profile.UserId, "Resgrid TCI Status", string.Format("Resgrid recieved your text command. Status changed to: {0}", payload.GetActionType()), department.DepartmentId, textMessage.To, profile); | |
| break; | |
| case TextCommandTypes.Staffing: | |
| messageEvent.Processed = true; | |
| await _userStateService.CreateUserState(profile.UserId, department.DepartmentId, (int)payload.GetStaffingType()); | |
| response.Message(string.Format("Resgrid received your text command. Staffing level changed to: {0}", payload.GetStaffingType())); | |
| //_communicationService.SendTextMessage(profile.UserId, "Resgrid TCI Staffing", string.Format("Resgrid recieved your text command. Staffing level changed to: {0}", payload.GetStaffingType()), department.DepartmentId, textMessage.To, profile); | |
| break; | |
| case TextCommandTypes.Stop: | |
| messageEvent.Processed = true; | |
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | |
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); | |
| break; | |
| case TextCommandTypes.CustomAction: | |
| messageEvent.Processed = true; | |
| await _actionLogsService.SetUserActionAsync(profile.UserId, department.DepartmentId, payload.GetCustomActionType()); | |
| if (customActions != null && customActions.IsDeleted == false && customActions.GetActiveDetails() != null && customActions.GetActiveDetails().Any() && | |
| customActions.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomActionType()) != null) | |
| { | |
| var detail = customActions.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomActionType()); | |
| response.Message(string.Format("Resgrid received your text command. Status changed to: {0}", detail.ButtonText)); | |
| } | |
| else | |
| { | |
| response.Message("Resgrid received your text command and updated your status"); | |
| } | |
| break; | |
| case TextCommandTypes.CustomStaffing: | |
| messageEvent.Processed = true; | |
| await _userStateService.CreateUserState(profile.UserId, department.DepartmentId, payload.GetCustomStaffingType()); | |
| if (customStaffing != null && customStaffing.IsDeleted == false && customStaffing.GetActiveDetails() != null && customStaffing.GetActiveDetails().Any() && | |
| customStaffing.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomStaffingType()) != null) | |
| { | |
| var detail = customStaffing.GetActiveDetails().FirstOrDefault(x => x.CustomStateDetailId == payload.GetCustomStaffingType()); | |
| response.Message(string.Format("Resgrid received your text command. Staffing changed to: {0}", detail.ButtonText)); | |
| } | |
| else | |
| { | |
| response.Message("Resgrid received your text command and updated your staffing"); | |
| } | |
| break; | |
| case TextCommandTypes.MyStatus: | |
| messageEvent.Processed = true; | |
| var userStatus = await _actionLogsService.GetLastActionLogForUserAsync(profile.UserId); | |
| var userStaffing = await _userStateService.GetLastUserStateByUserIdAsync(profile.UserId); | |
| var customStatusLevel = await _customStateService.GetCustomPersonnelStatusAsync(department.DepartmentId, userStatus); | |
| var customStaffingLevel = await _customStateService.GetCustomPersonnelStaffingAsync(department.DepartmentId, userStaffing); | |
| response.Message( | |
| $"Hello {profile.FullName.AsFirstNameLastName} at {DateTime.UtcNow.TimeConverterToString(department)} your current status is {customStatusLevel.ButtonText} and your current staffing is {customStaffingLevel.ButtonText}."); | |
| break; | |
| case TextCommandTypes.Calls: | |
| messageEvent.Processed = true; | |
| var activeCalls = await _callsService.GetActiveCallsByDepartmentAsync(department.DepartmentId); | |
| var activeCallText = new StringBuilder(); | |
| activeCallText.Append($"Active Calls for {department.Name}" + Environment.NewLine); | |
| activeCallText.Append("---------------------" + Environment.NewLine); | |
| foreach (var activeCall in activeCalls) | |
| { | |
| activeCallText.Append($"CallId: {activeCall.CallId} Name: {activeCall.Name} Nature:{StringHelpers.StripHtmlTagsCharArray(activeCall.NatureOfCall)}" + Environment.NewLine); | |
| } | |
| response.Message(activeCallText.ToString().Truncate(1200)); | |
| break; | |
| case TextCommandTypes.Units: | |
| messageEvent.Processed = true; | |
| var unitStatus = await _unitsService.GetAllLatestStatusForUnitsByDepartmentIdAsync(department.DepartmentId); | |
| var unitStatusesText = new StringBuilder(); | |
| unitStatusesText.Append($"Unit Statuses for {department.Name}" + Environment.NewLine); | |
| unitStatusesText.Append("---------------------" + Environment.NewLine); | |
| foreach (var unit in unitStatus) | |
| { | |
| var unitState = await _customStateService.GetCustomUnitStateAsync(unit); | |
| unitStatusesText.Append($"{unit.Unit.Name} is {unitState.ButtonText}" + Environment.NewLine); | |
| } | |
| response.Message(unitStatusesText.ToString().Truncate(1200)); | |
| break; | |
| case TextCommandTypes.CallDetail: | |
| messageEvent.Processed = true; | |
| var call = await _callsService.GetCallByIdAsync(int.Parse(payload.Data)); | |
| var callText = new StringBuilder(); | |
| callText.Append($"Call Information for {call.Name}" + Environment.NewLine); | |
| callText.Append("---------------------" + Environment.NewLine); | |
| callText.Append($"Id: {call.CallId}" + Environment.NewLine); | |
| callText.Append($"Number: {call.Number}" + Environment.NewLine); | |
| callText.Append($"Logged: {call.LoggedOn.TimeConverterToString(department)}" + Environment.NewLine); | |
| callText.Append("-----Nature-----" + Environment.NewLine); | |
| callText.Append(call.NatureOfCall + Environment.NewLine); | |
| callText.Append("-----Address-----" + Environment.NewLine); | |
| if (!String.IsNullOrWhiteSpace(call.Address)) | |
| callText.Append(call.Address + Environment.NewLine); | |
| else if (!string.IsNullOrEmpty(call.GeoLocationData) && call.GeoLocationData.Length > 1) | |
| { | |
| try | |
| { | |
| string[] points = call.GeoLocationData.Split(char.Parse(",")); | |
| if (points != null && points.Length == 2) | |
| { | |
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | |
| } | |
| } | |
| catch | |
| { | |
| } | |
| } | |
| response.Message(callText.ToString()); | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| else if (textMessage.To == Config.NumberProviderConfig.TwilioResgridNumber) // Resgrid master text number | |
| { | |
| var profile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | |
| var payload = _textCommandService.DetermineType(textMessage.Text); | |
| switch (payload.Type) | |
| { | |
| case TextCommandTypes.None: | |
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | |
| break; | |
| case TextCommandTypes.Help: | |
| messageEvent.Processed = true; | |
| var help = new StringBuilder(); | |
| help.Append("Resgrid Text Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("This is the Resgrid system for first responders (https://resgrid.com) automated text system. Your department isn't signed up for inbound text messages, but you can send the following commands." + | |
| Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | |
| help.Append("HELP: This help text" + Environment.NewLine); | |
| response.Message(help.ToString()); | |
| break; | |
| case TextCommandTypes.Stop: | |
| messageEvent.Processed = true; | |
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | |
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); | |
| break; | |
| } | |
| } | |
| response.Message(chatbotResponse.Text); | |
| var chatbotResponse = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | |
| if (chatbotResponse != null) | |
| { | |
| if (chatbotResponse.Processed) | |
| messageEvent.Processed = true; | |
| response.Message(chatbotResponse.Text); | |
| } | |
| else | |
| { | |
| response.Message("Unable to process your request at this time."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 150 -
155, The code calls _chatbotIngressService.ProcessMessageAsync and dereferences
chatbotResponse without checking for null, which can throw a
NullReferenceException; update the block that handles the chatbot reply (around
the chatbotResponse variable, messageEvent.Processed and response.Message calls)
to check if chatbotResponse is null before accessing its members, set
messageEvent.Processed = true only when chatbotResponse?.Processed == true, and
call response.Message with a safe fallback string when chatbotResponse is null
(keeping existing exception handling behavior).
| public ChatbotSettingsController(IChatbotDepartmentConfigService chatbotConfigService, IAuthorizationService authorizationService) | ||
| { | ||
| _chatbotConfigService = chatbotConfigService; | ||
| _authorizationService = authorizationService; | ||
| } |
There was a problem hiding this comment.
Violates Service Locator pattern guideline.
This controller uses constructor injection, but the coding guidelines require using Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors instead of constructor injection. As per coding guidelines, "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection."
♻️ Refactor to use Service Locator pattern
- private readonly IChatbotDepartmentConfigService _chatbotConfigService;
- private readonly IAuthorizationService _authorizationService;
+ private IChatbotDepartmentConfigService _chatbotConfigService;
+ private IAuthorizationService _authorizationService;
- public ChatbotSettingsController(IChatbotDepartmentConfigService chatbotConfigService, IAuthorizationService authorizationService)
+ public ChatbotSettingsController()
{
- _chatbotConfigService = chatbotConfigService;
- _authorizationService = authorizationService;
+ _chatbotConfigService = Bootstrapper.GetKernel().Resolve<IChatbotDepartmentConfigService>();
+ _authorizationService = Bootstrapper.GetKernel().Resolve<IAuthorizationService>();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ChatbotSettingsController(IChatbotDepartmentConfigService chatbotConfigService, IAuthorizationService authorizationService) | |
| { | |
| _chatbotConfigService = chatbotConfigService; | |
| _authorizationService = authorizationService; | |
| } | |
| public ChatbotSettingsController() | |
| { | |
| _chatbotConfigService = Bootstrapper.GetKernel().Resolve<IChatbotDepartmentConfigService>(); | |
| _authorizationService = Bootstrapper.GetKernel().Resolve<IAuthorizationService>(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs` around
lines 22 - 26, The constructor for ChatbotSettingsController currently uses
constructor injection for IChatbotDepartmentConfigService and
IAuthorizationService; change it to use the service-locator pattern by calling
Bootstrapper.GetKernel().Resolve<T>() inside the ChatbotSettingsController
constructor and assign the resolved instances to _chatbotConfigService and
_authorizationService respectively (use
Bootstrapper.GetKernel().Resolve<IChatbotDepartmentConfigService>() and
Bootstrapper.GetKernel().Resolve<IAuthorizationService>()), removing the
constructor parameters so dependencies are resolved explicitly within the
constructor.
2 participants
Summary by CodeRabbit
New Features
Infrastructure