Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughThe change adds chatbot settings validation across the model, view, controller, and configuration service. It also adds UTF-8 byte validation, API error mapping, an NLU project reference, and date validation to ChangesChatbot settings validation
Calls date validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| /// </summary> | ||
| private static void ValidateColumnLengths(ChatbotDepartmentConfig config) | ||
| { | ||
| if (config.AllowedPlatforms != null && config.AllowedPlatforms.Length > 500) |
There was a problem hiding this comment.
Magic number obscures the inline column-size limit of 500, violating Rule [9] by breaking explicit schema contracts. Extract this literal to a private const (e.g., MaxAllowedPlatformsLength) for the length check and error message, and apply the same fix to CallsController.cs:1710 and lines 166, 169, and 172 of ChatbotDepartmentConfigService.cs.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.cs:
Line 163:
Magic number obscures the inline column-size limit of 500, violating Rule [9] by breaking explicit schema contracts. Extract this literal to a private const (e.g., `MaxAllowedPlatformsLength`) for the length check and error message, and apply the same fix to `CallsController.cs:1710` and lines 166, 169, and 172 of `ChatbotDepartmentConfigService.cs`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| if (!ModelState.IsValid) | ||
| { | ||
| var existing = await _chatbotConfigService.GetConfigAsync(DepartmentId); |
There was a problem hiding this comment.
Unguarded async operation occurs because the awaited call to GetConfigAsync on line 60 falls outside the try/catch block, violating Rule [1]. Wrap this await in error handling or move the ModelState branch inside the existing try block to log and handle exceptions with context.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs:
Line 60:
Unguarded async operation occurs because the awaited call to `GetConfigAsync` on line 60 falls outside the try/catch block, violating Rule [1]. Wrap this await in error handling or move the `ModelState` branch inside the existing try block to log and handle exceptions with context.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| if (!ModelState.IsValid) | ||
| { | ||
| var existing = await _chatbotConfigService.GetConfigAsync(DepartmentId); |
There was a problem hiding this comment.
Unguarded external call on line 60 leaves GetConfigAsync exposed to unhandled exceptions, violating Rule [27]. Enclose this call in a try/catch block to log contextual data (DepartmentId) and map the error to a fallback view or application-level exception.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs:
Line 60:
Unguarded external call on line 60 leaves `GetConfigAsync` exposed to unhandled exceptions, violating Rule [27]. Enclose this call in a try/catch block to log contextual data (`DepartmentId`) and map the error to a fallback view or application-level exception.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.cs`:
- Around line 156-173: Update ValidateColumnLengths and the ChatbotController
exception handling so oversized configuration fields produce a typed validation
failure that is mapped to BadRequest (HTTP 400) before the generic exception
handler. Preserve the existing HTTP 500 behavior for unrelated exceptions and
apply the validation path to AllowedPlatforms, LlmApiEndpoint, LlmModelName, and
encrypted LlmApiKey.
In `@Web/Resgrid.Web/Areas/User/Models/ChatbotSettingsModel.cs`:
- Around line 30-31: Update the user-settings save flow in
ChatbotSettingsController.Index to validate request.LlmApiEndpoint with the
existing LlmEndpointValidator.IsValid check before copying it into
ChatbotDepartmentConfig or calling SaveConfigAsync. Reuse the same validation
arguments and invalid-input handling as the other API/LLM configuration writer,
or place the check in a shared boundary covering both writers.
- Around line 36-40: Update the validation for the write-only API-key property
in ChatbotSettingsModel so the limit is based on the UTF-8 byte length required
by encryption rather than character count, ensuring encrypted values remain
within the 1000-character LlmApiKey column. Preserve the existing write-only and
HasLlmApiKey behavior while applying validation consistently to Unicode input.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2518004e-c7be-4836-9d3a-3f6b9c8158be
📒 Files selected for processing (5)
Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.csWeb/Resgrid.Web/Areas/User/Models/ChatbotSettingsModel.csWeb/Resgrid.Web/Areas/User/Views/ChatbotSettings/Index.cshtml
| /// <summary> | ||
| /// Guards against SQL truncation (error 8152) by validating string fields against the | ||
| /// ChatbotDepartmentConfigs column sizes (M0068/M0070) before hitting the database. | ||
| /// LlmApiKey is checked post-encryption since the ciphertext is what gets stored. | ||
| /// </summary> | ||
| private static void ValidateColumnLengths(ChatbotDepartmentConfig config) | ||
| { | ||
| if (config.AllowedPlatforms != null && config.AllowedPlatforms.Length > 500) | ||
| throw new ArgumentException("AllowedPlatforms cannot exceed 500 characters.", nameof(config)); | ||
|
|
||
| if (config.LlmApiEndpoint != null && config.LlmApiEndpoint.Length > 500) | ||
| throw new ArgumentException("LlmApiEndpoint cannot exceed 500 characters.", nameof(config)); | ||
|
|
||
| if (config.LlmModelName != null && config.LlmModelName.Length > 200) | ||
| throw new ArgumentException("LlmModelName cannot exceed 200 characters.", nameof(config)); | ||
|
|
||
| if (config.LlmApiKey != null && config.LlmApiKey.Length > 1000) | ||
| throw new ArgumentException("Encrypted LlmApiKey cannot exceed 1000 characters; supply a shorter API key.", nameof(config)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Map configuration validation failures to HTTP 400.
ValidateColumnLengths throws ArgumentException for request data. Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs catches every exception and returns HTTP 500. An oversized AllowedPlatforms, LlmApiEndpoint, LlmModelName, or API key is therefore reported as a server failure instead of a client validation error.
Use a typed validation exception or a shared validation result. Map it to BadRequest before the generic exception handler.
🤖 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/ChatbotDepartmentConfigService.cs` around lines
156 - 173, Update ValidateColumnLengths and the ChatbotController exception
handling so oversized configuration fields produce a typed validation failure
that is mapped to BadRequest (HTTP 400) before the generic exception handler.
Preserve the existing HTTP 500 behavior for unrelated exceptions and apply the
validation path to AllowedPlatforms, LlmApiEndpoint, LlmModelName, and encrypted
LlmApiKey.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs (1)
63-69: 🩺 Stability & Availability | 🟠 MajorGuard the configuration reload on invalid model state.
Line 65 calls
GetConfigAsyncbefore thetryblock. A configuration-service failure can escape the action and turn an invalid submission into an unhandled 500. Move this branch into handled error flow or add local error handling. Keepmodel.LlmApiKeycleared on every return path.This repeats the earlier review finding for the same unguarded reload.
🤖 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 63 - 69, Update the invalid-ModelState branch in the relevant action of ChatbotSettingsController so GetConfigAsync is executed within the existing handled error flow or protected by local error handling. Preserve clearing model.LlmApiKey on every return path, including configuration-service failure, while retaining the existing validation response behavior.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs`:
- Around line 305-309: Narrow the ArgumentException handling in
ChatbotController.UpdateConfig() so authorization and DTO validation failures
cannot be converted to HTTP 400 responses. Perform column-length config
validation explicitly before SaveConfigAsync(), then wrap only SaveConfigAsync()
if its ArgumentException contract must remain; return a stable client-safe
message instead of ex.Message, while preserving downstream failure propagation.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.cs`:
- Around line 63-69: Update the invalid-ModelState branch in the relevant action
of ChatbotSettingsController so GetConfigAsync is executed within the existing
handled error flow or protected by local error handling. Preserve clearing
model.LlmApiKey on every return path, including configuration-service failure,
while retaining the existing validation response behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dbe8af30-7560-40bb-976a-359309916068
📒 Files selected for processing (6)
Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.csWeb/Resgrid.Web.Services/Controllers/v4/ChatbotController.csWeb/Resgrid.Web/Areas/User/Controllers/ChatbotSettingsController.csWeb/Resgrid.Web/Areas/User/Models/ChatbotSettingsModel.csWeb/Resgrid.Web/Attributes/MaxUtf8BytesAttribute.csWeb/Resgrid.Web/Resgrid.Web.csproj
🚧 Files skipped from review as they are similar to previous changes (2)
- Web/Resgrid.Web/Areas/User/Models/ChatbotSettingsModel.cs
- Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.cs
| catch (ArgumentException ex) | ||
| { | ||
| // Field-length (column size) validation from the config service. | ||
| return BadRequest(new { error = ex.Message }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C 8 \
'throw\s+new\s+ArgumentException|catch\s*\(\s*ArgumentException|SaveConfigAsync' \
--glob '*.cs' . || trueRepository: Resgrid/Core
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- target file location ---\n'
git ls-files | rg 'Web/Resgrid.Web.Services/Controllers/v4/ChatbotController\.cs$|ChatbotController\.cs$' || true
printf '\n--- ChatbotController outline ---\n'
ast-grep outline 'Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs' --view compact || true
printf '\n--- UpdateConfig and SaveConfigAsync sections ---\n'
rg -n -C 12 'UpdateConfig|SaveConfigAsync|ArgumentException|try|catch'Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs || true
printf '\n--- save-config service/repository implementations ---\n'
rg -n -C 8 'SaveConfigAsync|UpdateConfig\(\)' --glob '**/*.cs' . | sed -n '1,220p' || trueRepository: Resgrid/Core
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file location ---'
git ls-files | rg 'Web/Resgrid.Web.Services/Controllers/v4/ChatbotController\.cs$|ChatbotController\.cs$' || true
printf '%s\n' ''
printf '%s\n' '--- ChatbotController outline ---'
ast-grep outline 'Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs' --view compact || true
printf '%s\n' ''
printf '%s\n' '--- UpdateConfig and SaveConfigAsync sections ---'
rg -n -C 12 'UpdateConfig|SaveConfigAsync|ArgumentException|try|catch' 'Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs' || true
printf '%s\n' ''
printf '%s\n' '--- save-config service/repository implementations ---'
rg -n -C 8 'SaveConfigAsync|UpdateConfig\(\)' --glob '**/*.cs' . | sed -n '1,220p' || trueRepository: Resgrid/Core
Length of output: 25542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ChatbotDepartmentConfigService SaveConfigAsync ---'
sed -n '100,155p' 'Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.cs'
printf '%s\n' ''
printf '%s\n' '--- repo Save implementation candidates ---'
rg -n -C 10 'class .*DepartmentConfig|SaveConfigAsync|UpdateAsync\(.*ChatbotDepartmentConfig' --glob '**/*.cs' . | sed -n '1,260p' || true
printf '%s\n' ''
printf '%s\n' '--- precise repository/implementation files ---'
git ls-files | rg 'Chatbot.*Config.*Repository|DepartmentConfig.*Repository|\bRepository.*\.cs$' | rg -i 'chatbot|config' || trueRepository: Resgrid/Core
Length of output: 27268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ValidateColumnLengths implementation ---'
rg -n -C 25 'ValidateColumnLengths|class ChatbotDepartmentConfigService' 'Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.cs' || true
printf '%s\n' ''
printf '%s\n' '--- Repository method bodies ---'
sed -n '20,220p' 'Repositories/Resgrid.Repositories.DataRepository/ChatbotDepartmentConfigRepository.cs' || trueRepository: Resgrid/Core
Length of output: 4936
Narrow the ArgumentException handling to config validation only.
ChatbotController.UpdateConfig() wraps authorization, DTO validation, and SaveConfigAsync() in a single try. SaveConfigAsync() also catches and rethrows downstream failures, so the broad catch can return authorization/repository failures as HTTP 400 with internal exception text. Move column-length validation to an explicit 400 condition before saving, wrap only SaveConfigAsync() in a narrow ArgumentException block if it keeps that contract, or introduce a dedicated config-validation exception type. Return a stable client-safe message in the 400 path.
🤖 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/v4/ChatbotController.cs` around lines
305 - 309, Narrow the ArgumentException handling in
ChatbotController.UpdateConfig() so authorization and DTO validation failures
cannot be converted to HTTP 400 responses. Perform column-length config
validation explicitly before SaveConfigAsync(), then wrap only SaveConfigAsync()
if its ArgumentException contract must remain; return a stable client-safe
message instead of ex.Message, while preserving downstream failure propagation.
Source: Coding guidelines
| catch (ArgumentException ex) | ||
| { | ||
| // Field-length (column size) validation from the config service. | ||
| return BadRequest(new { error = ex.Message }); | ||
| } |
There was a problem hiding this comment.
Overly broad exception handling in the catch (ArgumentException ex) block captures internal server errors from _encryptionService.Encrypt() where Rfc2898DeriveBytes throws ArgumentNullException for invalid encryption salts. Introduce a dedicated ColumnLengthValidationException thrown by ValidateColumnLengths and catch only that specific type to ensure genuine ArgumentExceptions fall through to catch(Exception).
// Throw a dedicated ColumnLengthValidationException from ValidateColumnLengths instead of ArgumentException,
// then catch only that here so internal ArgumentException subclasses (e.g. from Rfc2898DeriveBytes in the
// encryption path) still fall through to catch(Exception) for proper logging and 500 response.
catch (ColumnLengthValidationException ex)
{
return BadRequest(new { error = ex.Message });
}Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/ChatbotController.cs:
Line 305 to 309:
Overly broad exception handling in the `catch (ArgumentException ex)` block captures internal server errors from `_encryptionService.Encrypt()` where `Rfc2898DeriveBytes` throws `ArgumentNullException` for invalid encryption salts. Introduce a dedicated `ColumnLengthValidationException` thrown by `ValidateColumnLengths` and catch only that specific type to ensure genuine `ArgumentExceptions` fall through to `catch(Exception)`.
Suggested Code:
// Throw a dedicated ColumnLengthValidationException from ValidateColumnLengths instead of ArgumentException,
// then catch only that here so internal ArgumentException subclasses (e.g. from Rfc2898DeriveBytes in the
// encryption path) still fall through to catch(Exception) for proper logging and 500 response.
catch (ColumnLengthValidationException ex)
{
return BadRequest(new { error = ex.Message });
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <summary> | ||
| /// Validates that a string's UTF-8 encoded byte count does not exceed a maximum. Use when the | ||
| /// stored representation depends on byte length (e.g. values encrypted before persisting), where | ||
| /// a character-count check (StringLength) would pass multi-byte Unicode input that overflows a |
There was a problem hiding this comment.
Unsafe type casting violates team standards. Apply the as operator or pattern matching for all casts, ensuring proper null checks before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Web/Resgrid.Web/Attributes/MaxUtf8BytesAttribute.cs:
Line 9:
Unsafe type casting violates team standards. Apply the `as` operator or pattern matching for all casts, ensuring proper null checks before usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
Pull Request Description
RG-T117: Chatbox Fixes
This PR addresses data validation and input handling issues in the Chatbox feature that were causing database errors, along with a related date validation fix in the Calls API.
Changes
Chatbot Settings Input Validation
ChatbotDepartmentConfigServicethat checks string fields against database column sizes before saving, preventing SQL truncation errors (error 8152)StringLengthdata annotation validators to theChatbotSettingsModelforAllowedPlatforms,LlmApiEndpoint,LlmModelName, andLlmApiKeyfieldsmaxlengthattributes and inline validation message spans to the chatbot settings form inputs for immediate client-side feedbackChatbotSettingsControllerto properly handle invalid model state by re-loading the existing API key indicator before returning the view with errorsCalls API Date Validation
GetCallsendpoint to reject missing or invalid date parameters that bind toDateTime.MinValue, which falls below SQL Server's datetime floor (1753-01-01) and causes aSqlDateTimeoverflow exception at the repository layerendDateis not earlier thanstartDate400 BadRequestwith a descriptive message when validation failsImpact
These changes prevent unhandled database exceptions when users submit chatbot configuration with overly long values or when API consumers omit date parameters, replacing them with clear, user-friendly validation errors.
Summary by CodeRabbit