-
-
Notifications
You must be signed in to change notification settings - Fork 86
RG-T117 Chatbox fixes #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -266,6 +266,7 @@ public async Task<IActionResult> GetConfig() | |
| /// </summary> | ||
| [HttpPut("Config")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| public async Task<IActionResult> UpdateConfig([FromBody] ChatbotConfigRequest request) | ||
| { | ||
| try | ||
|
|
@@ -301,6 +302,11 @@ public async Task<IActionResult> UpdateConfig([FromBody] ChatbotConfigRequest re | |
|
|
||
| return Ok(new { success = true }); | ||
| } | ||
| catch (ArgumentException ex) | ||
| { | ||
| // Field-length (column size) validation from the config service. | ||
| return BadRequest(new { error = ex.Message }); | ||
| } | ||
|
Comment on lines
+305
to
+309
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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
🤖 Prompt for AI AgentsSource: Coding guidelines
Comment on lines
+305
to
+309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overly broad exception handling in the // 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 LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,19 @@ public async Task<IActionResult> Index(ChatbotSettingsModel model, CancellationT | |
| if (!await _authorizationService.CanUserModifyDepartmentAsync(UserId, DepartmentId)) | ||
| return Unauthorized(); | ||
|
|
||
| // Same SSRF guard as the v4 API config writer (ChatbotController.UpdateConfig). | ||
| if (!string.IsNullOrWhiteSpace(model.LlmApiEndpoint) && | ||
| !Resgrid.Chatbot.NLU.LlmEndpointValidator.IsValid(model.LlmApiEndpoint, out var llmEndpointError)) | ||
| ModelState.AddModelError(nameof(model.LlmApiEndpoint), llmEndpointError); | ||
|
|
||
| if (!ModelState.IsValid) | ||
| { | ||
| var existing = await _chatbotConfigService.GetConfigAsync(DepartmentId); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unguarded async operation occurs because the awaited call to Kody rule violation: Handle async operations with proper error handling Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unguarded external call on line 60 leaves Kody rule violation: Add try-catch blocks for external calls Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| model.HasLlmApiKey = !string.IsNullOrWhiteSpace(existing?.LlmApiKey); | ||
| model.LlmApiKey = null; | ||
| return View(model); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| var config = new ChatbotDepartmentConfig | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| using System.ComponentModel.DataAnnotations; | ||
| using System.Text; | ||
|
|
||
| namespace Resgrid.Web.Attributes | ||
| { | ||
| /// <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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe type casting violates team standards. Apply the Kody rule violation: Use safe type casting with as operator Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| /// fixed-size column. | ||
| /// </summary> | ||
| public sealed class MaxUtf8BytesAttribute : ValidationAttribute | ||
| { | ||
| private readonly int _maxBytes; | ||
|
|
||
| public MaxUtf8BytesAttribute(int maxBytes) | ||
| : base($"Cannot exceed {maxBytes} bytes when UTF-8 encoded.") | ||
| { | ||
| _maxBytes = maxBytes; | ||
| } | ||
|
|
||
| public override bool IsValid(object value) | ||
| { | ||
| if (value is null) | ||
| return true; | ||
|
|
||
| return value is string s && Encoding.UTF8.GetByteCount(s) <= _maxBytes; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toCallsController.cs:1710and lines 166, 169, and 172 ofChatbotDepartmentConfigService.cs.Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.