Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds a new TTS microservice (Resgrid.Web.Tts) with audio generation, caching, S3 storage, rate limiting, health checks, and admin endpoints; integrates TTS-driven Twilio prompts and per-department TTS language; updates DI, solution, CI workflow to build the TTS Docker image; and adds comprehensive tests and worker scheduling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as TtsController
participant Service as TtsService
participant Cache as CacheService
participant Audio as AudioProcessingService
participant Storage as S3StorageService
Client->>API: POST /tts (text, voice, speed)
API->>Service: GenerateAsync(request)
Service->>Cache: CreateCacheKey(text,voice,speed)
Service->>Cache: TryGetCachedUrlAsync(cacheKey)
alt cache hit
Cache->>Storage: GetObjectUrlAsync(objectKey)
Storage-->>Service: Uri
Service-->>API: TtsResponse {url, cached=true}
else cache miss
Service->>Audio: GenerateNormalizedWavAsync(text,voice,speed)
Audio-->>Service: byte[] wav
Service->>Cache: StoreAsync(cacheKey, wav)
Cache->>Storage: UploadAsync(objectKey, stream)
Storage-->>Service: Uri
Service-->>API: TtsResponse {url, cached=false}
end
API-->>Client: 200 OK {url}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| [ProducesResponseType(typeof(StaticPromptRegenerationResponse), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status401Unauthorized)] | ||
| public async Task<ActionResult<StaticPromptRegenerationResponse>> RegenerateStaticPromptsAsync( |
| [HttpPost] | ||
| [ProducesResponseType(typeof(TtsResponse), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<TtsResponse>> GenerateAsync([FromBody] TtsRequest request, CancellationToken cancellationToken) |
| [HttpPost("batch")] | ||
| [ProducesResponseType(typeof(IReadOnlyCollection<TtsResponse>), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<IReadOnlyCollection<TtsResponse>>> GenerateBatchAsync([FromBody] List<TtsRequest> requests, CancellationToken cancellationToken) |
| _logger.LogWarning( | ||
| exception, | ||
| "Transient S3 failure during {OperationName} on attempt {Attempt}. Retrying in {DelayMs} ms.", | ||
| operationName, |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Core/Resgrid.Services/DepartmentSettingsService.cs (1)
46-54:⚠️ Potential issue | 🟠 MajorInvalidate the TTS language cache on create and delete too.
Line 69 only clears
DSetTtsLanguage_{departmentId}in the update path. If a department previously cached the fallback language, creating the firstTtsLanguagesetting keeps serving the stale fallback for up to 14 days; deletes have the inverse stale-override problem.🐛 Proposed cache-invalidation fix
public async Task<DepartmentSetting> SaveOrUpdateSettingAsync(int departmentId, string setting, DepartmentSettingTypes type, CancellationToken cancellationToken = default(CancellationToken)) { var savedSetting = await GetSettingByDepartmentIdType(departmentId, type); + await ClearSettingCacheAsync(departmentId, type); if (savedSetting == null) { DepartmentSetting newSetting = new DepartmentSetting(); newSetting.DepartmentId = departmentId; @@ else { - // Clear out Cache - switch (type) - { - case DepartmentSettingTypes.BigBoardMapCenterGpsCoordinates: - await _cacheProvider.RemoveAsync(string.Format(BigBoardCenterGps, departmentId)); - break; - case DepartmentSettingTypes.DisabledAutoAvailable: - await _cacheProvider.RemoveAsync(string.Format(DisableAutoAvailableCacheKey, departmentId)); - break; - case DepartmentSettingTypes.StaffingSuppressStaffingLevels: - await _cacheProvider.RemoveAsync(string.Format(StaffingSupressInfo, departmentId)); - break; - case DepartmentSettingTypes.TtsLanguage: - await _cacheProvider.RemoveAsync(string.Format(TtsLanguageCacheKey, departmentId)); - break; - case DepartmentSettingTypes.PersonnelOnUnitSetUnitStatus: - await _cacheProvider.RemoveAsync(string.Format(PersonnelOnUnitSetUnitStatusCacheKey, departmentId)); - break; - } - savedSetting.Setting = setting; return await _departmentSettingsRepository.SaveOrUpdateAsync(savedSetting, cancellationToken); } @@ public async Task<bool> DeleteSettingAsync(int departmentId, DepartmentSettingTypes type, CancellationToken cancellationToken = default(CancellationToken)) { var savedSetting = await GetSettingByDepartmentIdType(departmentId, type); if (savedSetting != null) + { + await ClearSettingCacheAsync(departmentId, type); return await _departmentSettingsRepository.DeleteAsync(savedSetting, cancellationToken); + } return false; } + + private async Task ClearSettingCacheAsync(int departmentId, DepartmentSettingTypes type) + { + switch (type) + { + case DepartmentSettingTypes.BigBoardMapCenterGpsCoordinates: + await _cacheProvider.RemoveAsync(string.Format(BigBoardCenterGps, departmentId)); + break; + case DepartmentSettingTypes.DisabledAutoAvailable: + await _cacheProvider.RemoveAsync(string.Format(DisableAutoAvailableCacheKey, departmentId)); + break; + case DepartmentSettingTypes.StaffingSuppressStaffingLevels: + await _cacheProvider.RemoveAsync(string.Format(StaffingSupressInfo, departmentId)); + break; + case DepartmentSettingTypes.TtsLanguage: + await _cacheProvider.RemoveAsync(string.Format(TtsLanguageCacheKey, departmentId)); + break; + case DepartmentSettingTypes.PersonnelOnUnitSetUnitStatus: + await _cacheProvider.RemoveAsync(string.Format(PersonnelOnUnitSetUnitStatusCacheKey, departmentId)); + break; + } + }Also applies to: 69-71, 84-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/DepartmentSettingsService.cs` around lines 46 - 54, When creating or deleting a TtsLanguage department setting you must also invalidate the TTS language cache key so stale fallback values aren't served; in DepartmentSettingsService update the create branch that builds newSetting (where savedSetting == null) and the delete path (around the block that handles removal at lines 84-91) to remove the cache key "DSetTtsLanguage_{departmentId}" in the same way the update path does; locate the code that currently clears the cache in the update path (references to savedSetting and the cache removal call) and add equivalent cache invalidation calls in the newSetting creation flow and in the delete flow so all create/update/delete paths clear the DSetTtsLanguage_{departmentId} cache.Web/Resgrid.Web.Services/Controllers/TwilioProviderController.cs (3)
504-509:⚠️ Potential issue | 🟡 MinorInput validation missing on
twilioRequest.Digits.
int.Parse(twilioRequest.Digits)at line 523 will throwFormatExceptionif Twilio posts an empty/non-numeric digits value (e.g., timeout with no input, or unexpected "#"/"*"). Considerint.TryParseand a graceful fallback prompt + hangup. Low likelihood givennumDigits: 1but Twilio can still send empty on gather timeout when no digits are pressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioProviderController.cs` around lines 504 - 509, In VoiceCallAction, validate twilioRequest.Digits before parsing: replace the direct int.Parse(twilioRequest.Digits) usage with int.TryParse on twilioRequest.Digits and handle the false case by returning a graceful Twilio response (e.g., play a retry prompt or Redirect back to the gather and then Hangup) so empty or non-numeric input doesn't throw; update logic around the int result where callId handling occurs to use the parsed value only when TryParse succeeds.
456-481:⚠️ Potential issue | 🟠 MajorBug: prompt uses
call.Addressinstead of the resolvedaddressvariable.Lines 457–473 compute a local
addressthat may be populated from a geo-lookup whencall.Addressis empty, but the ternary on line 477–479 checksaddressyet interpolatescall.Addressinto the prompt. When the address came from the geo provider,call.Addressis null/empty, so the generated prompt reads"... Priority X Address Nature Y"— an empty address slot. Use the localaddressvariable.🐛 Proposed fix
var prompts = new List<string> { !String.IsNullOrWhiteSpace(address) - ? $"{call.Name}, Priority {call.GetPriorityText()} Address {call.Address} Nature {call.NatureOfCall}" + ? $"{call.Name}, Priority {call.GetPriorityText()} Address {address} Nature {call.NatureOfCall}" : $"{call.Name}, Priority {call.GetPriorityText()} Nature {call.NatureOfCall}", TwilioVoicePromptCatalog.RepeatAndRespondToScene };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioProviderController.cs` around lines 456 - 481, The prompt construction in TwilioProviderController uses call.Address instead of the resolved local variable address, so when address was filled from GeoLocationData the prompt shows an empty address; update the prompt list creation to interpolate the local address variable (address) wherever the current code inserts call.Address, ensuring the ternary still checks String.IsNullOrWhiteSpace(address); check the block around GetAproxAddressFromLatLong, GeoLocationData and the prompts List<string> to replace call.Address with address so the generated prompt uses the resolved value.
519-539:⚠️ Potential issue | 🟠 MajorMissing
stations.Countbound check —IndexOutOfRangeExceptionpossible.
index < 8only caps the menu width but doesn't guard againststationshaving fewer entries. If a caller sends e.g. digit9while the department has 3 stations,stations[index]throws. Also, the null checkif (station != null)is effectively dead code sincestations[index]for a value-bearing list will return a reference (and indexing out of range throws, not returns null).🐛 Proposed fix
- if (index >= 0 && index < 8) + if (index >= 0 && index < Math.Min(8, stations.Count)) { var station = stations[index]; - - if (station != null) - { - await _actionLogsService.SetUserActionAsync(userId, call.DepartmentId, (int)ActionTypes.RespondingToStation, null, - station.DepartmentGroupId); - - await AppendVoicePromptAsync(response, TwilioVoicePromptCatalog.RespondingToStation(station.Name), call.DepartmentId); - response.Hangup(); - } - + await _actionLogsService.SetUserActionAsync(userId, call.DepartmentId, (int)ActionTypes.RespondingToStation, null, + station.DepartmentGroupId); + + await AppendVoicePromptAsync(response, TwilioVoicePromptCatalog.RespondingToStation(station.Name), call.DepartmentId); + response.Hangup(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioProviderController.cs` around lines 519 - 539, The code in TwilioProviderController computes index from twilioRequest.Digits then accesses stations[index] without checking stations.Count, risking IndexOutOfRangeException; update the logic after calling _departmentGroupsService.GetAllStationGroupsForDepartmentAsync(call.DepartmentId) to validate that index >= 0 && index < stations.Count (instead of index < 8), only then fetch station and proceed to call _actionLogsService.SetUserActionAsync and AppendVoicePromptAsync; remove the ineffective null-indexing assumption and add an else branch to handle invalid selections (e.g., play a retry/invalid selection prompt or hang up) so out-of-range digits are handled safely.Web/Resgrid.Web.Services/Controllers/TwilioController.cs (1)
682-707:⚠️ Potential issue | 🟠 MajorMove the “consumed” save after TTS prompt generation succeeds.
The code marks the verification voice code consumed before the fallible
AppendVoicePromptAsynccalls. If TTS URL generation fails, the caller never hears the code but cannot retry with the same code.🐛 Proposed fix
- switch ((ContactVerificationType)contactType) - { - case ContactVerificationType.MobileNumber: - profile.MobileVerificationVoiceCodeConsumed = true; - break; - case ContactVerificationType.HomeNumber: - profile.HomeVerificationVoiceCodeConsumed = true; - break; - } - - await _userProfileService.SaveProfileAsync(department.DepartmentId, profile); - var response = new VoiceResponse(); var spokenCode = string.Join(", ", code.ToCharArray()); await AppendVoicePromptAsync(response, TwilioVoicePromptCatalog.VerificationGreeting, department.DepartmentId); for (int i = 0; i < 3; i++) { response.Pause(length: 1); await AppendVoicePromptAsync(response, TwilioVoicePromptCatalog.VerificationCode(spokenCode), department.DepartmentId); } response.Pause(length: 1); await AppendVoicePromptAsync(response, TwilioVoicePromptCatalog.VerificationClosing, department.DepartmentId); response.Hangup(); + + switch ((ContactVerificationType)contactType) + { + case ContactVerificationType.MobileNumber: + profile.MobileVerificationVoiceCodeConsumed = true; + break; + case ContactVerificationType.HomeNumber: + profile.HomeVerificationVoiceCodeConsumed = true; + break; + } + + await _userProfileService.SaveProfileAsync(department.DepartmentId, profile); return CreateVoiceContentResult(response);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 682 - 707, The code sets profile.MobileVerificationVoiceCodeConsumed / profile.HomeVerificationVoiceCodeConsumed and calls _userProfileService.SaveProfileAsync before the fallible AppendVoicePromptAsync work; move the flag mutation and the SaveProfileAsync call to after the TTS prompt generation completes successfully (i.e., after the loop of AppendVoicePromptAsync calls and before returning CreateVoiceContentResult(response)) so the code is only marked consumed if AppendVoicePromptAsync (and any TTS URL generation) succeeded; locate these changes around the ContactVerificationType switch, _userProfileService.SaveProfileAsync, the AppendVoicePromptAsync calls, VoiceResponse construction, and CreateVoiceContentResult to implement the reorder.
🟡 Minor comments (7)
Web/Resgrid.Web.Tts/appsettings.Development.json-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorWindows-only default path for
AppOptions.ConfigPath.
C:\\Resgrid\\Config\\ResgridConfig.jsonwill not resolve on Linux/macOS developers or in a Linux container running withASPNETCORE_ENVIRONMENT=Development. Other services in this repo typically accept an env-var override (e.g.APPOPTIONS__CONFIGPATH/AppOptions__ConfigPath) — please confirm the TTSProgram.cshonors that so non-Windows contributors don’t have to edit this file. Consider using a relative/Linux-friendly default or documenting the override in the project README.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/appsettings.Development.json` at line 9, appsettings.Development.json contains a Windows-only default for AppOptions.ConfigPath; update Program.cs so the configuration system honors environment-variable overrides and non-Windows defaults by ensuring the configuration builder includes environment variables and the AppOptions section is bound via IConfiguration (e.g., use builder.Configuration.GetSection("AppOptions") / services.Configure<AppOptions>(...) or equivalent) so APPOPTIONS__CONFIGPATH or AppOptions__ConfigPath will override the file; also change the development default in appsettings.Development.json to a relative or Linux-friendly value (or remove the absolute path) so non-Windows developers and Linux containers don't need to edit the file.Web/Resgrid.Web.Tts/Services/TtsAudioContent.cs-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorHeads-up: record value-equality on
byte[]uses reference equality.
TtsAudioContentis arecordbut one of its members isbyte[] AudioBytes, whose generatedEquals/GetHashCodecompares by reference, not by content. If this record is ever compared, deduplicated, or used as a dictionary key, two payloads with identical bytes will not be considered equal. Based on the summary this type is only used as a cache value (not a key), so this is likely fine — but worth being explicit (e.g. add an XML comment) or overriding equality so future callers do not get tripped up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/TtsAudioContent.cs` at line 3, TtsAudioContent's generated record equality uses reference-equality for the byte[] AudioBytes, so implement content-based equality or document the reference semantics: change the positional record TtsAudioContent(byte[] AudioBytes, ...) into a record with a body and override Equals(TtsAudioContent? other) to compare AudioBytes by content (e.g., SequenceEqual) and override GetHashCode() to compute a hash from the byte contents (or use a stable hashing method like HashCode.Combine with a span-based/rolling hash); alternatively, replace AudioBytes with a value-type representation (ReadOnlyMemory<byte> or ImmutableArray<byte>) or add an XML comment on TtsAudioContent clarifying that AudioBytes equality is by reference if you choose not to override.Resgrid.sln-101-101 (1)
101-101:⚠️ Potential issue | 🟡 MinorWrong project type GUID for
Resgrid.Web.Tts.The project is registered with type GUID
{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}, which is the legacy C++ project type. Every other SDK‑style C# project in this solution uses{9A19103F-16F7-4668-BE54-9A1E7A4F7556}(e.g.Resgrid.Web.Mcpon line 83,Resgrid.Web.Eventingon line 81). Visual Studio/dotnetmay still open/build it, but tooling that respects the type GUID (code-style, some analyzers, project templates, solution filters) will misclassify it. The pre-existingResgrid.Providers.Weatherentry on line 99 has the same typo — worth fixing both while you’re here.🔧 Proposed fix
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Resgrid.Web.Tts", "Web\Resgrid.Web.Tts\Resgrid.Web.Tts.csproj", "{684FA75D-6712-41AE-A396-B6E0918899C0}" +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Resgrid.Web.Tts", "Web\Resgrid.Web.Tts\Resgrid.Web.Tts.csproj", "{684FA75D-6712-41AE-A396-B6E0918899C0}" EndProject🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resgrid.sln` at line 101, The solution file registers Resgrid.Web.Tts and Resgrid.Providers.Weather with the legacy C++ project type GUID ({FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}); update their Project(...) entries to use the SDK-style C# project type GUID ({9A19103F-16F7-4668-BE54-9A1E7A4F7556}). Locate the lines starting with Project("...") = "Resgrid.Web.Tts", "Resgrid.Web.Tts.csproj", "{684FA75D-6712-41AE-A396-B6E0918899C0}" and Project("...") = "Resgrid.Providers.Weather", ... and replace the first GUID in each Project(...) header with {9A19103F-16F7-4668-BE54-9A1E7A4F7556} so the projects are correctly classified as C# SDK projects.Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs-688-715 (1)
688-715:⚠️ Potential issue | 🟡 MinorDon’t treat an unsupported configured default as a supported language.
If
TtsConfig.DefaultVoiceis non-empty but not recognized byEspeakVoiceCatalog,IsSupportedTtsLanguageaccepts that raw value. That can persist a language the TTS backend cannot synthesize, despite the validation message saying it must be a supported eSpeak-NG language.Proposed fix
private static bool IsSupportedTtsLanguage(string ttsLanguage) { - if (EspeakVoiceCatalog.TryNormalizeIdentifier(ttsLanguage, out _)) - return true; - - return string.Equals(NormalizeTtsLanguage(ttsLanguage), GetConfiguredDefaultTtsLanguage(), StringComparison.OrdinalIgnoreCase); + return EspeakVoiceCatalog.TryNormalizeIdentifier(ttsLanguage, out _); } private static string GetConfiguredDefaultTtsLanguage() { if (EspeakVoiceCatalog.TryNormalizeIdentifier(TtsConfig.DefaultVoice, out var normalizedLanguage)) return normalizedLanguage; - if (!string.IsNullOrWhiteSpace(TtsConfig.DefaultVoice)) - return TtsConfig.DefaultVoice.Trim(); - return EspeakVoiceCatalog.DefaultIdentifier; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 688 - 715, IsSupportedTtsLanguage currently treats a raw, unrecognized TtsConfig.DefaultVoice as supported because GetConfiguredDefaultTtsLanguage returns the raw value; change GetConfiguredDefaultTtsLanguage so it only returns a normalized identifier or falls back to EspeakVoiceCatalog.DefaultIdentifier (i.e., call EspeakVoiceCatalog.TryNormalizeIdentifier and if that fails do NOT return the raw TtsConfig.DefaultVoice.Trim(), instead return EspeakVoiceCatalog.DefaultIdentifier); this ensures NormalizeTtsLanguage/IsSupportedTtsLanguage (and callers) never accept an unrecognized configured default.Core/Resgrid.Services/DepartmentSettingsService.cs-870-879 (1)
870-879:⚠️ Potential issue | 🟡 MinorDon’t return an invalid configured default voice.
Line 876 returns any non-empty
TtsConfig.DefaultVoiceeven whenEspeakVoiceCatalog.TryNormalizeIdentifierrejects it, which can push an unusable voice into TTS generation instead of falling back safely.🐛 Proposed fallback fix
private static string GetDefaultTtsLanguage() { if (EspeakVoiceCatalog.TryNormalizeIdentifier(TtsConfig.DefaultVoice, out var normalizedVoice)) return normalizedVoice; - if (!string.IsNullOrWhiteSpace(TtsConfig.DefaultVoice)) - return TtsConfig.DefaultVoice.Trim(); - return EspeakVoiceCatalog.DefaultIdentifier; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/DepartmentSettingsService.cs` around lines 870 - 879, GetDefaultTtsLanguage currently returns TtsConfig.DefaultVoice when TryNormalizeIdentifier fails but the configured value is non-empty, which may be invalid; change the logic in GetDefaultTtsLanguage so that if EspeakVoiceCatalog.TryNormalizeIdentifier(TtsConfig.DefaultVoice, out var normalizedVoice) returns true you return normalizedVoice, otherwise ignore any non-normalized TtsConfig.DefaultVoice and return EspeakVoiceCatalog.DefaultIdentifier (or trim/validate first if you intend to accept only normalized values). Ensure you only accept TtsConfig.DefaultVoice when it can be normalized by TryNormalizeIdentifier and otherwise fall back to EspeakVoiceCatalog.DefaultIdentifier.Web/Resgrid.Web.Tts/Configuration/ServiceCollectionExtensions.cs-68-83 (1)
68-83:⚠️ Potential issue | 🟡 MinorPreserve default prompts when config is unset.
Line 68 replaces the default
TtsOptions.PreGeneratedPromptswith an empty list wheneverTtsConfig.PreGeneratedPromptsis blank. That can silently disable static prompt generation for deployments that do not yet provide the new setting.🐛 Proposed default-preserving mapping
- options.PreGeneratedPrompts = ParsePrompts(TtsConfig.PreGeneratedPrompts); + var configuredPrompts = ParsePrompts(TtsConfig.PreGeneratedPrompts); + if (configuredPrompts.Count > 0) + options.PreGeneratedPrompts = configuredPrompts; } private static List<string> ParsePrompts(string rawPrompts) { if (string.IsNullOrWhiteSpace(rawPrompts))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Configuration/ServiceCollectionExtensions.cs` around lines 68 - 83, The mapping currently overwrites TtsOptions.PreGeneratedPrompts with an empty list when TtsConfig.PreGeneratedPrompts is blank; change ParsePrompts to return null when rawPrompts is null/whitespace and update the caller in ServiceCollectionExtensions to only assign options.PreGeneratedPrompts when ParsePrompts(...) returns a non-null value (i.e., leave the default prompts intact if the config is unset). Reference: ParsePrompts and the code that sets options.PreGeneratedPrompts in ServiceCollectionExtensions.cs.Web/Resgrid.Web.Tts/Services/CacheService.cs-129-138 (1)
129-138:⚠️ Potential issue | 🟡 MinorBroaden corruption handling when deserializing the cached payload.
Only
InvalidDataExceptionis caught on Line 133, butDeserializeAudioContentcan throw other exceptions for a corrupted/truncated entry:EndOfStreamExceptionfromBinaryReader.ReadByte/ReadString/ReadInt32/ReadInt64,ArgumentOutOfRangeExceptionfromnew DateTimeOffset(ticks, TimeSpan.Zero)on out-of-range ticks, andIOException/ArgumentExceptionfrom malformed string length prefixes inReadString. Any of these will bubble up into request-serving code paths and — because the poisoned key is not evicted — keep failing on subsequent reads.Either normalize all deserialization failures to
InvalidDataExceptioninsideDeserializeAudioContent, or widen the catch:🛡️ Proposed fix
- try - { - return DeserializeAudioContent(payload); - } - catch (InvalidDataException) - { - await _distributedCache.RemoveAsync(GetAudioCacheEntryKey(hash), cancellationToken); - return null; - } + try + { + return DeserializeAudioContent(payload); + } + catch (Exception ex) when (ex is InvalidDataException + or EndOfStreamException + or ArgumentException + or IOException) + { + await _distributedCache.RemoveAsync(GetAudioCacheEntryKey(hash), cancellationToken); + return null; + }Also applies to: 178-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/CacheService.cs` around lines 129 - 138, The try/catch around DeserializeAudioContent only handles InvalidDataException so other deserialization errors (EndOfStreamException, ArgumentOutOfRangeException, IOException, etc.) leave a poisoned cache key; update the catch to handle all deserialization failures by either normalizing exceptions inside DeserializeAudioContent to InvalidDataException or widening the catch here to catch Exception (or the specific deserialization exceptions) and then call _distributedCache.RemoveAsync(GetAudioCacheEntryKey(hash), cancellationToken) before returning null; make the same change for the other deserialization block that mirrors this logic (the block covering lines 178-206) so any corruption always evicts the cached entry.
🧹 Nitpick comments (14)
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js (1)
421-423: Keep the exported helper consistent with the new row-only behavior.
checkAllUnitsstill toggles every checkbox under the grid, while the new handlers intentionally target onlytbodyrow checkboxes. If this helper is still used from markup, scope it the same way to avoid affecting header or future non-row controls.♻️ Proposed alignment
function checkAllUnits(gridName, item) { - $('#' + gridName).find(':checkbox').prop('checked', item.checked); + $('#' + gridName).find('tbody :checkbox').prop('checked', item.checked); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js` around lines 421 - 423, The helper checkAllUnits currently toggles every checkbox under the grid; change its selector to only target row checkboxes (match the new handlers) by scoping to the table body—e.g., use $('#' + gridName).find('tbody :checkbox') (or $('#' + gridName + ' tbody').find(':checkbox')) instead of $('#' + gridName).find(':checkbox'); update any markup usage that expects the old behavior if needed and keep parameters (gridName, item) unchanged.Core/Resgrid.Model/Services/ITtsAudioService.cs (1)
10-10: Consider nullable annotation onvoiceparameter.
string voice = nullwill emit a CS8625 warning if/when nullable reference types are enabled forResgrid.Model. Preferstring? voice = nullfor clarity of intent, matching the optional semantic already expressed byint? speed.Proposed change
- Task<Uri> GenerateSpeechUrlAsync(string text, string voice = null, int? speed = null, CancellationToken cancellationToken = default); + Task<Uri> GenerateSpeechUrlAsync(string text, string? voice = null, int? speed = null, CancellationToken cancellationToken = default);As per coding guidelines: "Use modern C# features appropriately."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Services/ITtsAudioService.cs` at line 10, Update the GenerateSpeechUrlAsync signature to mark the voice parameter as nullable: change the parameter type in the ITtsAudioService.GenerateSpeechUrlAsync declaration from string voice = null to string? voice = null so it matches the optional int? speed and avoids CS8625 when nullable reference types are enabled; ensure any implementations of GenerateSpeechUrlAsync (methods named the same in implementing classes) are updated to accept string? as well.Web/Resgrid.Web.Tts/Models/TtsRequest.cs (1)
11-15: Consider tighteningVoicevalidation.
Voiceaccepts up to 64 chars with no pattern validation. Since voice names are ultimately forwarded to an external TTS engine / used in object keys (per the S3 caching flow described in the PR), consider constraining to an allowlist or regex (e.g.,[A-Za-z0-9_-]+) to prevent unexpected characters from propagating into cache keys or storage paths.Also,
[Range(80, 450)]onSpeed— confirm these bounds match the underlying TTS engine's supported WPM/percentage range; out-of-band values silently clamped vs rejected is a common footgun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Models/TtsRequest.cs` around lines 11 - 15, Tighten validation on the TtsRequest model: add a RegularExpression attribute to the Voice property (e.g., allow only alphanumerics, underscore and hyphen) and reduce the max length if appropriate so voice names cannot contain path-breaking characters; update the Voice property definition in the TtsRequest class to include this regex-based validation in addition to StringLength. For Speed (the Speed property on TtsRequest) verify the [Range(80, 450)] bounds against the target TTS engine and either adjust the range to match the engine’s supported min/max or change validation to explicitly reject out-of-range values (rather than implicitly clamping) so callers get immediate errors. Ensure you reference and update TtsRequest.Voice and TtsRequest.Speed in tests and any deserialization paths that rely on these constraints.Core/Resgrid.Model/EspeakVoiceCatalog.cs (1)
9-145: Use an array with Array.AsReadOnly() to prevent unsafe casting of the voice catalog.
VoicesexposesVoicesInternalasIReadOnlyList<TtsVoiceOption>, but at runtime it references a mutableList<TtsVoiceOption>. Callers could theoretically cast it back toList<TtsVoiceOption>and mutate the global catalog. Use aTtsVoiceOption[]wrapped withArray.AsReadOnly()instead to prevent unsafe downcasting. This aligns with the coding guideline to prefer functional patterns and immutable data in C#.♻️ Proposed fix
- private static readonly IReadOnlyList<TtsVoiceOption> VoicesInternal = new List<TtsVoiceOption> + private static readonly TtsVoiceOption[] VoicesInternal = { new TtsVoiceOption("af", "Afrikaans"), new TtsVoiceOption("sq", "Albanian"),public const string DefaultIdentifier = "en-us"; - public static IReadOnlyList<TtsVoiceOption> Voices => VoicesInternal; + public static IReadOnlyList<TtsVoiceOption> Voices { get; } = Array.AsReadOnly(VoicesInternal);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/EspeakVoiceCatalog.cs` around lines 9 - 145, VoicesInternal is currently a mutable List<TtsVoiceOption> typed as IReadOnlyList, which allows unsafe downcast and mutation; change the backing collection to a private static readonly TtsVoiceOption[] VoicesInternal = new TtsVoiceOption[] { ... } (populate with the same entries), keep VoiceLookup built from VoicesInternal (VoicesInternal.ToDictionary(...)), and change the public accessor to return an immutable wrapper like public static IReadOnlyList<TtsVoiceOption> Voices => Array.AsReadOnly(VoicesInternal); ensure DefaultIdentifier and VoiceLookup remain unchanged.Web/Resgrid.Web.Tts/Dockerfile (1)
11-25: Consider adding a HEALTHCHECK and pinning package versions.Optional hardening suggestions:
- Add a
HEALTHCHECKinstruction (e.g., hitting a/healthendpoint) so orchestrators can detect unhealthy TTS pods beyond liveness based on process exit.- Consider pinning
espeak-ng/ffmpegapt package versions (or at least documenting that the image is tied to Debian stable minor upgrades) for reproducible TTS output across rebuilds — audio codecs in particular can change subtle behaviors between ffmpeg versions.Also confirm whether the published app needs write access anywhere under
/appat runtime; if temp files are written during TTS generation, ensure they go to/tmpor an explicitly-writable volume, since/appis root-owned while the process runs asappuser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Dockerfile` around lines 11 - 25, Add a HEALTHCHECK to the Dockerfile that probes the application's health endpoint (e.g., GET /health on ASPNETCORE_URLS) so orchestrators can detect non-crash failures, and pin or document apt package versions for espeak-ng and ffmpeg (or note Debian stable minor-upgrade coupling) to ensure reproducible TTS output; also verify Resgrid.Web.Tts runtime file writes: if the TTS code (e.g., temp file creation in the app) writes under /app, change it to use /tmp or an explicit writable volume and/or update the Dockerfile to create and chown a writable directory before switching to USER appuser so the ENTRYPOINT dotnet process can write where needed.Core/Resgrid.Model/Services/IDepartmentSettingsService.cs (1)
172-173: Add XML doc comment for consistency.Every other member in this interface is documented with a
<summary>block. Please add equivalent XML docs forGetTtsLanguageForDepartmentAsyncdescribing the return format (normalized language tag / eSpeak voice identifier) and the default/fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Services/IDepartmentSettingsService.cs` around lines 172 - 173, Add an XML <summary> doc comment to the interface method GetTtsLanguageForDepartmentAsync(int departmentId) describing what the method returns (a normalized language tag or an eSpeak voice identifier), explain the format expected (e.g., "en-US" or "en+f3" style), and state the default/fallback behavior when no department-specific setting exists (which language/voice is returned). Place the comment directly above the Task<string> GetTtsLanguageForDepartmentAsync(int departmentId); declaration and include a brief <param name="departmentId"> description and a <returns> line that documents the exact return value semantics.Web/Resgrid.Web.Tts/Resgrid.Web.Tts.csproj (1)
12-16: Consider updating NuGet versions for currency.The pinned versions are all valid and have no known security advisories, but are notably outdated for a greenfield service:
AWSSDK.S3 3.7.414.5→ latest4.0.22Microsoft.Extensions.Caching.StackExchangeRedis 8.0.0→ latest10.0.7Swashbuckle.AspNetCore 6.6.2→ latest10.1.7Updating to the latest 8.x/10.x-compatible versions will reduce version debt and align with current ecosystem standards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Resgrid.Web.Tts.csproj` around lines 12 - 16, Update the three pinned NuGet PackageReference entries in Resgrid.Web.Tts.csproj to current compatible releases: change AWSSDK.S3 from 3.7.414.5 to 4.0.22, Microsoft.Extensions.Caching.StackExchangeRedis from 8.0.0 to 10.0.7, and Swashbuckle.AspNetCore from 6.6.2 to 10.1.7; verify the project builds and run tests after updating the PackageReference elements to ensure no breaking API changes affect code that uses AWSSDK.S3, Microsoft.Extensions.Caching.StackExchangeRedis, or Swashbuckle.AspNetCore.Web/Resgrid.Web.Tts/Controllers/TtsAdminController.cs (1)
106-118: Key-length comparison leaks timing information.The
configuredBytes.Length == suppliedBytes.Lengthshort-circuit is not constant-time and leaks the configured key length through timing of early return. For a shared admin secret this is typically acceptable, but if you want strict constant-time behavior (including identical runtime across any supplied-key length), hash both sides to a fixed size first and thenFixedTimeEquals, e.g. usingSHA256.HashData.🛡️ Optional hardening
- var configuredBytes = Encoding.UTF8.GetBytes(_options.StaticPromptAdminKey); - var suppliedBytes = Encoding.UTF8.GetBytes(suppliedKey); - - return configuredBytes.Length == suppliedBytes.Length - && CryptographicOperations.FixedTimeEquals(configuredBytes, suppliedBytes); + Span<byte> configuredHash = stackalloc byte[32]; + Span<byte> suppliedHash = stackalloc byte[32]; + SHA256.HashData(Encoding.UTF8.GetBytes(_options.StaticPromptAdminKey), configuredHash); + SHA256.HashData(Encoding.UTF8.GetBytes(suppliedKey), suppliedHash); + return CryptographicOperations.FixedTimeEquals(configuredHash, suppliedHash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Controllers/TtsAdminController.cs` around lines 106 - 118, IsAuthorized currently leaks the configured key length via the Length equality short-circuit; instead normalize both _options.StaticPromptAdminKey and suppliedKey to a fixed-size digest (e.g. SHA256.HashData) and then call CryptographicOperations.FixedTimeEquals on those digests so comparison time is independent of input lengths while keeping the existing null/whitespace checks; update IsAuthorized to compute hashA = SHA256.HashData(Encoding.UTF8.GetBytes(_options.StaticPromptAdminKey)) and hashB = SHA256.HashData(Encoding.UTF8.GetBytes(suppliedKey)) and return FixedTimeEquals(hashA, hashB).Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs (1)
91-125: Harden process orchestration against cancellation races.A few small issues in
RunProcessAsync:
process.StandardInput.FlushAsync()doesn't propagatecancellationToken(overload accepting one exists in modern .NET).- If
cancellationTokentriggers theTryKillProcesscallback while stdin is still being written/flushed, theWriteAsync/FlushAsync/Closecan throw (e.g.,IOException/ObjectDisposedException) and be surfaced ahead of the more informative process-exit exception. Consider wrapping the stdin write block in a try/catch that swallows the expected disposal exceptions when the token is cancelled, then letWaitForExitAsyncthrowOperationCanceledException.- On failure, the captured
standardOutputis only used as a fallback; if both are non-empty, stdout (which for ffmpeg-loglevel erroris normally empty) is discarded. That's fine, just noting for log completeness — you could concatenate both when diagnosing.♻️ Suggested tweak
- if (standardInput is not null) - { - await process.StandardInput.WriteAsync(standardInput.AsMemory(), cancellationToken); - await process.StandardInput.FlushAsync(); - process.StandardInput.Close(); - } + if (standardInput is not null) + { + try + { + await process.StandardInput.WriteAsync(standardInput.AsMemory(), cancellationToken); + await process.StandardInput.FlushAsync(cancellationToken); + } + catch (IOException) when (cancellationToken.IsCancellationRequested) { } + catch (ObjectDisposedException) when (cancellationToken.IsCancellationRequested) { } + finally + { + process.StandardInput.Close(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs` around lines 91 - 125, RunProcessAsync currently can surface write/flush/close exceptions if cancellation fires and disposes the process; fix by wrapping the stdin block (the WriteAsync/FlushAsync/Close calls in RunProcessAsync) in a try/catch that catches IOException and ObjectDisposedException (and OperationCanceledException) and only suppresses them when cancellationToken.IsCancellationRequested so the cancellation path lets WaitForExitAsync surface OperationCanceledException via the cancellationRegistration/TryKillProcess; also call FlushAsync with the cancellationToken overload. Finally, when throwing the non-zero exit exception from RunProcessAsync include both standardError and standardOutput (e.g., concat or prefer non-empty then append the other) so logs include both streams for diagnosis.Web/Resgrid.Web.Services/Twilio/ITwilioVoiceResponseService.cs (1)
9-18: Parameter ordering and fully-qualifiedTaskare non-idiomatic.
CancellationTokenconventionally sits at the end of the parameter list per C#/.NET guidelines, but here it precedes an optionalvoiceparameter with a default. Additionally,using System.Threading.Tasks;is already imported, soSystem.Threading.Tasks.Taskcan be shortened toTask.♻️ Proposed signatures
- System.Threading.Tasks.Task AppendPromptAsync(VoiceResponse response, string text, CancellationToken cancellationToken = default, string voice = null); - - System.Threading.Tasks.Task AppendPromptAsync(Gather gather, string text, CancellationToken cancellationToken = default, string voice = null); - - System.Threading.Tasks.Task AppendPromptsAsync(VoiceResponse response, IEnumerable<string> prompts, CancellationToken cancellationToken = default, string voice = null); - - System.Threading.Tasks.Task AppendPromptsAsync(Gather gather, IEnumerable<string> prompts, CancellationToken cancellationToken = default, string voice = null); + Task AppendPromptAsync(VoiceResponse response, string text, string voice = null, CancellationToken cancellationToken = default); + + Task AppendPromptAsync(Gather gather, string text, string voice = null, CancellationToken cancellationToken = default); + + Task AppendPromptsAsync(VoiceResponse response, IEnumerable<string> prompts, string voice = null, CancellationToken cancellationToken = default); + + Task AppendPromptsAsync(Gather gather, IEnumerable<string> prompts, string voice = null, CancellationToken cancellationToken = default);Note: this is a breaking change to the interface; call sites (including
TwilioProviderController) will need to be updated. As per coding guidelines, "Use modern C# features appropriately" and "avoid unclear abbreviations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Twilio/ITwilioVoiceResponseService.cs` around lines 9 - 18, The interface ITwilioVoiceResponseService uses fully-qualified System.Threading.Tasks.Task and places CancellationToken before an optional voice parameter, which is non-idiomatic; update all four method signatures (AppendPromptAsync(VoiceResponse ...), AppendPromptAsync(Gather ...), AppendPromptsAsync(VoiceResponse ...), AppendPromptsAsync(Gather ...)) to return Task (use the imported Task type) and move the CancellationToken parameter to the end of each parameter list (e.g., ... string voice = null, CancellationToken cancellationToken = default), then update all callers (including TwilioProviderController) to match the new ordering.Web/Resgrid.Web.Tts/Controllers/TtsController.cs (1)
50-69: Add a configurable maximum batch size limit.
GenerateBatchAsyncaccepts an unbounded batch of requests and processes them all concurrently viaTask.WhenAllwithout size constraints. Combined with expensive eSpeak+ffmpeg processing per item, a large batch can exhaust worker threads, disk, and CPU even with rate limiting in front (the rate limiter counts requests, not items). Add a configurableMaxBatchSizesetting toTtsOptionsand validate the batch size inGenerateBatchAsync, returning400 Bad Requestwhen exceeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Controllers/TtsController.cs` around lines 50 - 69, Add a MaxBatchSize int property to TtsOptions and wire TtsOptions into TtsController (e.g., via IOptions<TtsOptions> in the constructor), then validate the incoming requests list length at the start of GenerateBatchAsync: if requests is null or requests.Count == 0 handle as before, but if requests.Count > options.Value.MaxBatchSize return BadRequest(CreateProblemDetails($"Batch size exceeds MaxBatchSize of {options.Value.MaxBatchSize}")); otherwise call _ttsService.GenerateBatchAsync as before. Ensure you reference the TtsOptions.MaxBatchSize property and the GenerateBatchAsync method and keep the existing error handling behavior for ArgumentException.Workers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs (1)
40-43: InjectITtsAudioServiceinstead of resolving it from the root container.
Bootstrapper.GetKernel().Resolve<ITtsAudioService>()hides a required dependency and couples task execution to the Autofac root container. Constructor injection keeps the handler testable and avoids service-locator behavior. As per coding guidelines, “Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions”.♻️ Proposed refactor
private readonly ILogger _logger; + private readonly ITtsAudioService _ttsAudioService; - public TtsStaticPromptRefreshTask(ILogger logger) + public TtsStaticPromptRefreshTask(ILogger logger, ITtsAudioService ttsAudioService) { _logger = logger; + _ttsAudioService = ttsAudioService; } - var ttsAudioService = Bootstrapper.GetKernel().Resolve<ITtsAudioService>(); - _logger.LogInformation("TtsStaticPromptRefresh::Refreshing static prompts"); - await ttsAudioService.RegenerateStaticPromptsAsync(TwilioVoicePromptCatalog.GetStaticPrompts(), cancellationToken); + await _ttsAudioService.RegenerateStaticPromptsAsync(TwilioVoicePromptCatalog.GetStaticPrompts(), cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs` around lines 40 - 43, Replace the service-locator call to Bootstrapper.GetKernel().Resolve<ITtsAudioService>() by injecting ITtsAudioService into the TtsStaticPromptRefreshTask via its constructor: add an ITtsAudioService parameter, assign it to a private readonly field (e.g., _ttsAudioService) and use that field in ExecuteAsync when calling RegenerateStaticPromptsAsync(TwilioVoicePromptCatalog.GetStaticPrompts(), cancellationToken); remove the Resolve call and update any instantiation/DI registration of TtsStaticPromptRefreshTask so the container supplies ITtsAudioService, keeping _logger usage unchanged.Core/Resgrid.Config/TtsConfig.cs (1)
1-44: LGTM — consistent with existingResgrid.Configpattern.Mutable public static fields are required by the reflection-based
ConfigProcessor, so the deviation from the "immutable data where appropriate" guideline is acceptable here. One minor optional thought: thePreGeneratedPromptsliteral on Line 38 is ~1.5 KB of English-only copy embedded in code — since this service is explicitly about multi-lingual support, consider extracting the default set to a resource/JSON file so non-English defaults can be shipped without a code change. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Config/TtsConfig.cs` around lines 1 - 44, PreGeneratedPrompts is a large English-only string embedded in TtsConfig (PreGeneratedPrompts) — extract this default prompt set into an external resource (e.g., JSON or .resx) and load it at startup so multilingual/default variants can be shipped without re-compiling; update the TtsConfig class to read the external file into the existing public static string PreGeneratedPrompts (or add a loader method) and ensure ConfigProcessor still sets other fields as before, preserving the class name TtsConfig and the PreGeneratedPrompts symbol for backward compatibility.Web/Resgrid.Web.Tts/Services/CacheService.cs (1)
36-49: Avoid loading the full audio payload from Redis just to resolve a URL.
TryGetCachedAudioAsyncon Line 38 performs a fullIDistributedCache.GetAsyncandDeserializeAudioContent— which pulls the entire WAV byte array over the wire from Redis and allocates it — only to discard the content and return a URL. For larger prompts under any non-trivial call volume this is wasted bandwidth/allocations on the hot path. Prefer a lightweight presence check (e.g., a separate small "exists" marker key, or relying solely on_storageService.ExistsAsyncwith a short-lived existence cache) so that URL resolution doesn't deserialize audio bytes.♻️ Sketch
public async Task<Uri?> TryGetCachedUrlAsync(TtsCacheKey cacheKey, CancellationToken cancellationToken) { - if (await TryGetCachedAudioAsync(cacheKey.Hash, cancellationToken) is not null) - { - return await _storageService.GetObjectUrlAsync(cacheKey.ObjectKey, cancellationToken); - } - - if (!await _storageService.ExistsAsync(cacheKey.ObjectKey, cancellationToken)) + // Cheap existence probe first (small marker key or storage head) + if (!await AudioExistsAsync(cacheKey, cancellationToken)) { return null; } return await _storageService.GetObjectUrlAsync(cacheKey.ObjectKey, cancellationToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/CacheService.cs` around lines 36 - 49, Currently TryGetCachedUrlAsync calls TryGetCachedAudioAsync (which pulls and deserializes full WAV bytes) just to decide whether to return _storageService.GetObjectUrlAsync; change TryGetCachedUrlAsync to avoid that heavy call by performing a lightweight existence check instead — either call _storageService.ExistsAsync(cacheKey.ObjectKey, cancellationToken) first or check a small marker/exists key in the distributed cache (written when audio is cached) instead of calling TryGetCachedAudioAsync(cacheKey.Hash, ...); keep the final call to _storageService.GetObjectUrlAsync(cacheKey.ObjectKey, cancellationToken) when the existence check succeeds and do not deserialize audio bytes in this path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c07a0580-fec0-49cf-a251-11cba124ad09
⛔ Files ignored due to path filters (2)
Core/Resgrid.Localization/Areas/User/Department/Department.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.resxis excluded by!**/*.resx
📒 Files selected for processing (71)
.github/workflows/dotnet.ymlCore/Resgrid.Config/Resgrid.Config.csprojCore/Resgrid.Config/TtsConfig.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/EspeakVoiceCatalog.csCore/Resgrid.Model/Services/ICommunicationTestService.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Model/Services/ITtsAudioService.csCore/Resgrid.Model/TwilioVoicePromptCatalog.csCore/Resgrid.Services/CommunicationTestService.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/TtsAudioService.csResgrid.slnTests/Resgrid.Tests/Resgrid.Tests.csprojTests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.csTests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.csTests/Resgrid.Tests/Web/Services/TwilioVoiceResponseServiceTests.csTests/Resgrid.Tests/Web/Tts/CacheServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsAdminControllerTests.csTests/Resgrid.Tests/Web/Tts/TtsConfigurationRegistrationTests.csTests/Resgrid.Tests/Web/Tts/TtsControllerTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web.Services/Controllers/TwilioProviderController.csWeb/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.csWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web.Services/Twilio/ITwilioVoiceResponseService.csWeb/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.csWeb/Resgrid.Web.Tts/Configuration/RateLimitOptions.csWeb/Resgrid.Web.Tts/Configuration/S3StorageOptions.csWeb/Resgrid.Web.Tts/Configuration/ServiceCollectionExtensions.csWeb/Resgrid.Web.Tts/Configuration/TtsOptions.csWeb/Resgrid.Web.Tts/Controllers/TtsAdminController.csWeb/Resgrid.Web.Tts/Controllers/TtsController.csWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.csWeb/Resgrid.Web.Tts/Models/StaticPromptRegenerationRequest.csWeb/Resgrid.Web.Tts/Models/StaticPromptRegenerationResponse.csWeb/Resgrid.Web.Tts/Models/TtsRequest.csWeb/Resgrid.Web.Tts/Models/TtsResponse.csWeb/Resgrid.Web.Tts/Program.csWeb/Resgrid.Web.Tts/Properties/launchSettings.jsonWeb/Resgrid.Web.Tts/Resgrid.Web.Tts.csprojWeb/Resgrid.Web.Tts/Services/AudioProcessingService.csWeb/Resgrid.Web.Tts/Services/CacheService.csWeb/Resgrid.Web.Tts/Services/IAudioProcessingService.csWeb/Resgrid.Web.Tts/Services/ICacheService.csWeb/Resgrid.Web.Tts/Services/IStorageService.csWeb/Resgrid.Web.Tts/Services/ITtsPlaybackUrlService.csWeb/Resgrid.Web.Tts/Services/ITtsService.csWeb/Resgrid.Web.Tts/Services/PromptWarmupHostedService.csWeb/Resgrid.Web.Tts/Services/S3StorageService.csWeb/Resgrid.Web.Tts/Services/TtsAudioContent.csWeb/Resgrid.Web.Tts/Services/TtsCacheKey.csWeb/Resgrid.Web.Tts/Services/TtsPlaybackUrlService.csWeb/Resgrid.Web.Tts/Services/TtsService.csWeb/Resgrid.Web.Tts/appsettings.Development.jsonWeb/Resgrid.Web.Tts/appsettings.jsonWeb/Resgrid.Web.Tts/k8s/deployment.yamlWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Models/DepartmentSettingsModel.csWeb/Resgrid.Web/Areas/User/Views/Department/Settings.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.editcall.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.jsWeb/Resgrid.Web/wwwroot/js/ng/react-elements.cssWeb/Resgrid.Web/wwwroot/js/ng/react-elements.jsWorkers/Resgrid.Workers.Console/Commands/TtsStaticPromptRefreshCommand.csWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs (1)
79-79: Minor:Lengthshouldn't be readable on a non-seekable stream.Conventionally,
Stream.LengththrowsNotSupportedExceptionwhenCanSeekis false (seeFileStream,NetworkStream, etc.). Exposing_inner.Lengthhere makes the test double a weaker simulation of a non-seekable stream — if production code ever gated onLength, this test would silently pass despite the real-world contract differing. Not exercised by the currentUploadAsyncpath (which only callsCopyToAsync), so this is purely a fidelity nit.♻️ Optional tightening
- public override long Length => _inner.Length; + public override long Length => throw new NotSupportedException();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs` at line 79, The test stream currently exposes its Length via "public override long Length => _inner.Length;", which violates the non-seekable stream contract; change the overridden Length getter on the test double to throw NotSupportedException when CanSeek is false (keep CanSeek returning false) instead of returning _inner.Length so the fake better matches real non-seekable streams and prevents tests from masking production behavior (refer to the overridden Length property and the CanSeek implementation in the test stream).Tests/Resgrid.Tests/Web/Tts/TtsPlaybackUrlServiceTests.cs (1)
11-57: Consider adding coverage for the missing-base-URL branch.The tests exercise normalization, delimiter rejection, and request-derived base URL resolution, but don't cover the
InvalidOperationExceptionpath when bothPlaybackBaseUrlis empty andrequestis null (orHosthas no value). A small additional test would close the remaining branch inCreatePlaybackUrl.Proposed additional test
[Test] public void create_playback_url_should_throw_when_base_url_cannot_be_resolved() { var service = new TtsPlaybackUrlService(Options.Create(new TtsOptions())); FluentActions .Invoking(() => service.CreatePlaybackUrl(null, "abc123")) .Should() .Throw<InvalidOperationException>(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Tts/TtsPlaybackUrlServiceTests.cs` around lines 11 - 57, Tests are missing coverage for the branch in TtsPlaybackUrlService.CreatePlaybackUrl that throws InvalidOperationException when no PlaybackBaseUrl is configured and no request/Host is available; add a unit test that constructs TtsPlaybackUrlService(Options.Create(new TtsOptions())) and asserts that invoking CreatePlaybackUrl(null, "abc123") throws InvalidOperationException (use FluentActions.Invoking...Should().Throw<InvalidOperationException>()), which will close the untested branch.Tests/Resgrid.Tests/Web/Tts/PromptWarmupHostedServiceTests.cs (1)
46-50: Consider invoking viaStartAsyncinstead of reflection.
PromptWarmupHostedServicederives fromBackgroundService, which exposes a publicStartAsync(CancellationToken)that internally callsExecuteAsync. Using it avoids brittle reflection (which would silently break if the base type or method signature changes) and exercises the same public entry point the host uses at runtime.♻️ Proposed refactor
- await InvokeExecuteAsync(service, CancellationToken.None); + await service.StartAsync(CancellationToken.None); + await service.StopAsync(CancellationToken.None); @@ - private static Task InvokeExecuteAsync(PromptWarmupHostedService service, CancellationToken cancellationToken) - { - var method = typeof(PromptWarmupHostedService).GetMethod("ExecuteAsync", BindingFlags.Instance | BindingFlags.NonPublic); - return (Task)method!.Invoke(service, new object[] { cancellationToken })!; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Tts/PromptWarmupHostedServiceTests.cs` around lines 46 - 50, The test helper InvokeExecuteAsync uses reflection to call the non-public ExecuteAsync on PromptWarmupHostedService; replace this brittle reflection call by invoking the public StartAsync(CancellationToken) on the PromptWarmupHostedService instance so the test exercises the same host entry point. Update the test to call service.StartAsync(cancellationToken) (and await StopAsync when needed) instead of using typeof(PromptWarmupHostedService).GetMethod("ExecuteAsync") and method.Invoke, keeping the same CancellationToken handling and ensuring any cleanup uses StopAsync if the original test relied on stopping the background task.Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs (1)
71-86: Cache theRegexinstances.
Regex.ReplaceandRegex.Splitat Lines 76 and 85 are called on every prompt on the voice hot path. Hoisting them tostatic readonly Regex(or[GeneratedRegex]with a source-generated matcher) avoids pattern recompilation and cuts allocations.🛠️ Proposed fix
+ private static readonly Regex WhitespaceRegex = new(@"\s+", RegexOptions.Compiled); + private static readonly Regex SentenceSplitRegex = new(@"(?<=[\.\!\?])\s+", RegexOptions.Compiled); @@ - var normalized = Regex.Replace(text, @"\s+", " ").Trim(); + var normalized = WhitespaceRegex.Replace(text, " ").Trim(); @@ - var sentences = Regex.Split(normalized, @"(?<=[\.\!\?])\s+") + var sentences = SentenceSplitRegex.Split(normalized) .Where(sentence => !string.IsNullOrWhiteSpace(sentence));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs` around lines 71 - 86, ChunkText currently calls Regex.Replace and Regex.Split on every invocation which recompiles patterns and allocates; hoist those patterns into reusable regexes and use them instead. Add static readonly Regex fields (e.g., NormalizedWhitespaceRegex = new Regex(@"\s+", RegexOptions.Compiled | RegexOptions.CultureInvariant) and SentenceSplitRegex = new Regex(@"(?<=[\.\!\?])\s+", RegexOptions.Compiled | RegexOptions.CultureInvariant)) or replace with [GeneratedRegex] source-generated matchers, then change ChunkText to call NormalizedWhitespaceRegex.Replace(...) and SentenceSplitRegex.Split(...) (keep existing trimming and filtering logic).Tests/Resgrid.Tests/Web/Services/CommunicationTestResponseControllerTests.cs (1)
69-80: Avoid reflecting on a private method; assert behavior via the public webhook.Invoking
GetDepartmentTtsLanguageAsyncvia reflection couples the test to a private symbol name and signature. The same blank-token behavior can be verified by callingVoiceWebhook("", "1")(or a whitespace token) and assertingGetDepartmentIdByResponseTokenAsync/GetTtsLanguageForDepartmentAsyncare never invoked, which already appears to be the intended contract of the first test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Services/CommunicationTestResponseControllerTests.cs` around lines 69 - 80, The test currently reflects on the private method GetDepartmentTtsLanguageAsync which couples the spec to an internal symbol; instead call the public endpoint VoiceWebhook with a blank or whitespace token (e.g. VoiceWebhook(" ", "1")) to exercise the same behavior, await the public call's result, and assert that the returned value matches the expected null/empty response while verifying _communicationTestServiceMock.GetDepartmentIdByResponseTokenAsync and _departmentSettingsServiceMock.GetTtsLanguageForDepartmentAsync were never invoked; remove the reflection usage and reference the public VoiceWebhook call and the two mock verifications (GetDepartmentIdByResponseTokenAsync, GetTtsLanguageForDepartmentAsync) in the updated test.Web/Resgrid.Web.Services/Controllers/TwilioController.cs (1)
917-934: Minor: templateGatheris awkward.The
gatherResponsebuilt at Lines 890/911/919 is only used to carryAction/Methodinto the two repeat iterations, which then construct freshGathers. Extracting the action URI and method into local variables would avoid the "dummy" Gather and make intent clearer.🛠️ Proposed sketch
- Gather gatherResponse = null; + Uri gatherAction = null; + const string gatherMethod = "GET"; @@ - else if (twilioRequest.Digits == "6") // Set current status + else if (twilioRequest.Digits == "6") // Set current status { ... - gatherResponse = new Gather(numDigits: 1, action: new Uri($"{...}/InboundVoiceActionStatus?userId={userId}"), method: "GET") - { - BargeIn = true - }; + gatherAction = new Uri($"{...}/InboundVoiceActionStatus?userId={userId}"); } ... - if (gatherResponse == null) - { - gatherResponse = new Gather(numDigits: 1, action: new Uri($"{...}/InboundVoiceAction?userId={userId}"), method: "GET") - { - BargeIn = true - }; - } + gatherAction ??= new Uri($"{...}/InboundVoiceAction?userId={userId}"); for (int repeat = 0; repeat < 2; repeat++) { - var gather = new Gather(numDigits: 1, action: gatherResponse.Action, method: gatherResponse.Method) + var gather = new Gather(numDigits: 1, action: gatherAction, method: gatherMethod) { BargeIn = true };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 917 - 934, The current code creates a dummy Gather (gatherResponse) only to pass its Action/Method into the two loop-created Gathers; replace that by extracting the action URI and method into locals (e.g., actionUri and httpMethod) computed the same way as gatherResponse.Action/Method using Config.SystemBehaviorConfig.ResgridApiBaseUrl and userId, then construct each new Gather inside the loop with those locals (preserving BargeIn = true) and keep calls to AppendVoicePromptsAsync, AppendVoicePromptAsync, and response.Append unchanged (look for TwilioController, gatherResponse, AppendVoicePromptsAsync, AppendVoicePromptAsync).Core/Resgrid.Services/TtsAudioService.cs (1)
92-107: Consider records for the private DTOs.
GenerateSpeechRequest,GenerateSpeechResponse, andRegenerateStaticPromptsRequestare pure data carriers. Converting them to property-init records (e.g.,record GenerateSpeechRequest { string Text { get; init; } }) would align with the guideline to use records for state while maintaining immutability. Property-init records serialize correctly with Newtonsoft.Json since serialization is property-based.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/TtsAudioService.cs` around lines 92 - 107, Replace the three private DTO classes GenerateSpeechRequest, GenerateSpeechResponse, and RegenerateStaticPromptsRequest with property-init records to make them immutable data carriers (e.g., use record types with init-only properties or positional records) so serialization with Newtonsoft.Json still works; update GenerateSpeechRequest to expose Text, Voice, Speed as init properties and RegenerateStaticPromptsRequest to expose Prompts as an init List<GenerateSpeechRequest>, and remove the sealed class declarations accordingly.Web/Resgrid.Web.Tts/Services/TtsService.cs (1)
190-202: UseArgumentNullExceptionfor the null‑request case.
NormalizeRequestthrowsArgumentExceptionwhenrequestis null; the idiomatic choice isArgumentNullException.ThrowIfNull(request)(mirroringGenerateBatchAsyncon line 40). It also lets callers distinguish null from other invalid payloads (e.g., blank text).♻️ Proposed tweak
private NormalizedTtsRequest NormalizeRequest(TtsRequest? request) { - if (request is null) - { - throw new ArgumentException("A TTS request payload is required.", nameof(request)); - } + ArgumentNullException.ThrowIfNull(request); var text = request.Text?.Trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/TtsService.cs` around lines 190 - 202, Replace the manual null check in NormalizeRequest with the standard null-argument pattern: call ArgumentNullException.ThrowIfNull(request) at the top of NormalizeRequest instead of throwing new ArgumentException("A TTS request payload is required.", nameof(request)); keep the subsequent text trimming and whitespace validation as-is so blank text still throws the ArgumentException; this mirrors the usage in GenerateBatchAsync and makes the null case distinct via ArgumentNullException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/TtsAudioService.cs`:
- Around line 38-41: The code currently constructs a Uri with new
Uri(response.Data.Url, UriKind.Absolute) which can throw UriFormatException on
malformed or non-absolute URLs; update the guard in the method that returns the
Uri (around the response handling in TtsAudioService) to use
Uri.TryCreate(response.Data.Url, UriKind.Absolute, out var uri) and, if it
returns false, throw the same InvalidOperationException via
CreateRequestFailure("generate speech audio", response) so all failure modes
surface the consistent exception type; keep the existing checks for
response.IsSuccessful and null/whitespace before attempting TryCreate.
In `@Tests/Resgrid.Tests/Resgrid.Tests.csproj`:
- Line 59: The test project targets net9.0 while the referenced Resgrid.Web.Tts
project targets net8.0, causing framework mismatch; fix by either changing the
Tests/Resgrid.Tests.csproj TargetFramework to net8.0 so tests run against the
same runtime as Resgrid.Web.Tts, or update Resgrid.Web.Tts.csproj to
multi-target (TargetFrameworks) to include net9.0 (e.g., net8.0;net9.0) so both
frameworks are available for the ProjectReference; after making the change,
restore and rebuild to ensure the TTS tests in Tests/Resgrid.Tests/Web/Tts/ run
against the intended framework.
In `@Tests/Resgrid.Tests/Web/Tts/TtsRequestIdentityTests.cs`:
- Around line 56-58: The test is asserting the wrong IPv6 prefix length after
TtsRequestIdentity.AddKnownNetwork was changed to use IPNetwork(MapToIPv6(),
prefixLength + 96); update the expectation in TtsRequestIdentityTests so the
assertion that matches the IPv6-mapped network
(IPAddress.Parse("::ffff:10.42.0.0")) checks for a PrefixLength of 112 (i.e. 16
+ 96) rather than 16; locate the assertion lines in TtsRequestIdentityTests.cs
that reference IPAddress.Parse("::ffff:10.42.0.0") and change the expected
PrefixLength accordingly.
In
`@Tests/Resgrid.Tests/Workers/Console/Tasks/TtsStaticPromptRefreshTaskTests.cs`:
- Around line 19-48: The test fixture TtsStaticPromptRefreshTaskTests mutates
shared static state (Resgrid.Workers.Framework.Bootstrapper._container and
TtsConfig.ServiceBaseUrl/StaticPromptAdminKey) which can cause flaky failures if
NUnit runs tests in parallel; mark the fixture as non-parallelizable by adding
the NUnit [NonParallelizable] attribute to the TtsStaticPromptRefreshTaskTests
class (or alternatively apply [assembly: NonParallelizable] at the test
assembly) so tests that touch Bootstrapper._container and TtsConfig.* run
sequentially and avoid cross-test interference.
---
Nitpick comments:
In `@Core/Resgrid.Services/TtsAudioService.cs`:
- Around line 92-107: Replace the three private DTO classes
GenerateSpeechRequest, GenerateSpeechResponse, and
RegenerateStaticPromptsRequest with property-init records to make them immutable
data carriers (e.g., use record types with init-only properties or positional
records) so serialization with Newtonsoft.Json still works; update
GenerateSpeechRequest to expose Text, Voice, Speed as init properties and
RegenerateStaticPromptsRequest to expose Prompts as an init
List<GenerateSpeechRequest>, and remove the sealed class declarations
accordingly.
In
`@Tests/Resgrid.Tests/Web/Services/CommunicationTestResponseControllerTests.cs`:
- Around line 69-80: The test currently reflects on the private method
GetDepartmentTtsLanguageAsync which couples the spec to an internal symbol;
instead call the public endpoint VoiceWebhook with a blank or whitespace token
(e.g. VoiceWebhook(" ", "1")) to exercise the same behavior, await the public
call's result, and assert that the returned value matches the expected
null/empty response while verifying
_communicationTestServiceMock.GetDepartmentIdByResponseTokenAsync and
_departmentSettingsServiceMock.GetTtsLanguageForDepartmentAsync were never
invoked; remove the reflection usage and reference the public VoiceWebhook call
and the two mock verifications (GetDepartmentIdByResponseTokenAsync,
GetTtsLanguageForDepartmentAsync) in the updated test.
In `@Tests/Resgrid.Tests/Web/Tts/PromptWarmupHostedServiceTests.cs`:
- Around line 46-50: The test helper InvokeExecuteAsync uses reflection to call
the non-public ExecuteAsync on PromptWarmupHostedService; replace this brittle
reflection call by invoking the public StartAsync(CancellationToken) on the
PromptWarmupHostedService instance so the test exercises the same host entry
point. Update the test to call service.StartAsync(cancellationToken) (and await
StopAsync when needed) instead of using
typeof(PromptWarmupHostedService).GetMethod("ExecuteAsync") and method.Invoke,
keeping the same CancellationToken handling and ensuring any cleanup uses
StopAsync if the original test relied on stopping the background task.
In `@Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs`:
- Line 79: The test stream currently exposes its Length via "public override
long Length => _inner.Length;", which violates the non-seekable stream contract;
change the overridden Length getter on the test double to throw
NotSupportedException when CanSeek is false (keep CanSeek returning false)
instead of returning _inner.Length so the fake better matches real non-seekable
streams and prevents tests from masking production behavior (refer to the
overridden Length property and the CanSeek implementation in the test stream).
In `@Tests/Resgrid.Tests/Web/Tts/TtsPlaybackUrlServiceTests.cs`:
- Around line 11-57: Tests are missing coverage for the branch in
TtsPlaybackUrlService.CreatePlaybackUrl that throws InvalidOperationException
when no PlaybackBaseUrl is configured and no request/Host is available; add a
unit test that constructs TtsPlaybackUrlService(Options.Create(new
TtsOptions())) and asserts that invoking CreatePlaybackUrl(null, "abc123")
throws InvalidOperationException (use
FluentActions.Invoking...Should().Throw<InvalidOperationException>()), which
will close the untested branch.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 917-934: The current code creates a dummy Gather (gatherResponse)
only to pass its Action/Method into the two loop-created Gathers; replace that
by extracting the action URI and method into locals (e.g., actionUri and
httpMethod) computed the same way as gatherResponse.Action/Method using
Config.SystemBehaviorConfig.ResgridApiBaseUrl and userId, then construct each
new Gather inside the loop with those locals (preserving BargeIn = true) and
keep calls to AppendVoicePromptsAsync, AppendVoicePromptAsync, and
response.Append unchanged (look for TwilioController, gatherResponse,
AppendVoicePromptsAsync, AppendVoicePromptAsync).
In `@Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs`:
- Around line 71-86: ChunkText currently calls Regex.Replace and Regex.Split on
every invocation which recompiles patterns and allocates; hoist those patterns
into reusable regexes and use them instead. Add static readonly Regex fields
(e.g., NormalizedWhitespaceRegex = new Regex(@"\s+", RegexOptions.Compiled |
RegexOptions.CultureInvariant) and SentenceSplitRegex = new
Regex(@"(?<=[\.\!\?])\s+", RegexOptions.Compiled |
RegexOptions.CultureInvariant)) or replace with [GeneratedRegex]
source-generated matchers, then change ChunkText to call
NormalizedWhitespaceRegex.Replace(...) and SentenceSplitRegex.Split(...) (keep
existing trimming and filtering logic).
In `@Web/Resgrid.Web.Tts/Services/TtsService.cs`:
- Around line 190-202: Replace the manual null check in NormalizeRequest with
the standard null-argument pattern: call
ArgumentNullException.ThrowIfNull(request) at the top of NormalizeRequest
instead of throwing new ArgumentException("A TTS request payload is required.",
nameof(request)); keep the subsequent text trimming and whitespace validation
as-is so blank text still throws the ArgumentException; this mirrors the usage
in GenerateBatchAsync and makes the null case distinct via
ArgumentNullException.
🪄 Autofix (Beta)
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
Run ID: f97f92ae-73dc-46c5-93af-f4523887da40
📒 Files selected for processing (25)
Core/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/TtsAudioService.csTests/Resgrid.Tests/Resgrid.Tests.csprojTests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.csTests/Resgrid.Tests/Web/Services/CommunicationTestResponseControllerTests.csTests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.csTests/Resgrid.Tests/Web/Services/TwilioVoiceResponseServiceTests.csTests/Resgrid.Tests/Web/Tts/PromptWarmupHostedServiceTests.csTests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsConfigurationRegistrationTests.csTests/Resgrid.Tests/Web/Tts/TtsPlaybackUrlServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsRequestIdentityTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csTests/Resgrid.Tests/Workers/Console/Tasks/TtsStaticPromptRefreshTaskTests.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.csWeb/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.csWeb/Resgrid.Web.Tts/Configuration/ServiceCollectionExtensions.csWeb/Resgrid.Web.Tts/Configuration/TtsRequestIdentity.csWeb/Resgrid.Web.Tts/Program.csWeb/Resgrid.Web.Tts/Services/PromptWarmupHostedService.csWeb/Resgrid.Web.Tts/Services/S3StorageService.csWeb/Resgrid.Web.Tts/Services/TtsPlaybackUrlService.csWeb/Resgrid.Web.Tts/Services/TtsService.csWorkers/Resgrid.Workers.Console/Tasks/TtsStaticPromptRefreshTask.cs
✅ Files skipped from review due to trivial changes (1)
- Web/Resgrid.Web.Tts/Services/S3StorageService.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- Web/Resgrid.Web.Tts/Services/PromptWarmupHostedService.cs
- Core/Resgrid.Services/ServicesModule.cs
- Tests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.cs
- Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.cs
- Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs
- Tests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.cs
Comment on lines
+38
to
+41
| if (!response.IsSuccessful || response.Data == null || string.IsNullOrWhiteSpace(response.Data.Url)) | ||
| throw CreateRequestFailure("generate speech audio", response); | ||
|
|
||
| return new Uri(response.Data.Url, UriKind.Absolute); |
Contributor
There was a problem hiding this comment.
Inconsistent exception type on malformed URL.
The guard at Line 38 only checks for null/whitespace. If the TTS service returns a non-absolute or otherwise malformed URL, new Uri(response.Data.Url, UriKind.Absolute) throws UriFormatException instead of the InvalidOperationException callers see for other failure modes. Use Uri.TryCreate and surface the standard failure exception.
🛠️ Proposed fix
- if (!response.IsSuccessful || response.Data == null || string.IsNullOrWhiteSpace(response.Data.Url))
- throw CreateRequestFailure("generate speech audio", response);
-
- return new Uri(response.Data.Url, UriKind.Absolute);
+ if (!response.IsSuccessful || response.Data == null || string.IsNullOrWhiteSpace(response.Data.Url))
+ throw CreateRequestFailure("generate speech audio", response);
+
+ if (!Uri.TryCreate(response.Data.Url, UriKind.Absolute, out var audioUrl))
+ throw CreateRequestFailure("generate speech audio", response);
+
+ return audioUrl;📝 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.
Suggested change
| if (!response.IsSuccessful || response.Data == null || string.IsNullOrWhiteSpace(response.Data.Url)) | |
| throw CreateRequestFailure("generate speech audio", response); | |
| return new Uri(response.Data.Url, UriKind.Absolute); | |
| if (!response.IsSuccessful || response.Data == null || string.IsNullOrWhiteSpace(response.Data.Url)) | |
| throw CreateRequestFailure("generate speech audio", response); | |
| if (!Uri.TryCreate(response.Data.Url, UriKind.Absolute, out var audioUrl)) | |
| throw CreateRequestFailure("generate speech audio", response); | |
| return audioUrl; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/TtsAudioService.cs` around lines 38 - 41, The code
currently constructs a Uri with new Uri(response.Data.Url, UriKind.Absolute)
which can throw UriFormatException on malformed or non-absolute URLs; update the
guard in the method that returns the Uri (around the response handling in
TtsAudioService) to use Uri.TryCreate(response.Data.Url, UriKind.Absolute, out
var uri) and, if it returns false, throw the same InvalidOperationException via
CreateRequestFailure("generate speech audio", response) so all failure modes
surface the consistent exception type; keep the existing checks for
response.IsSuccessful and null/whitespace before attempting TryCreate.
| <ProjectReference Include="..\..\Repositories\Resgrid.Repositories.NoSqlRepository\Resgrid.Repositories.NoSqlRepository.csproj" /> | ||
| <ProjectReference Include="..\..\Web\Resgrid.Web.Services\Resgrid.Web.Services.csproj" /> | ||
| <ProjectReference Include="..\..\Web\Resgrid.Web.Mcp\Resgrid.Web.Mcp.csproj" /> | ||
| <ProjectReference Include="..\..\Web\Resgrid.Web.Tts\Resgrid.Web.Tts.csproj" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm TFMs of both projects and scan for any other references to Resgrid.Web.Tts
fd -t f 'Resgrid.Web.Tts.csproj' --exec cat {}
echo "---"
fd -t f 'Resgrid.Workers.Console.csproj' --exec cat {}
echo "---"
rg -n 'Resgrid\.Web\.Tts' -g '*.csproj' -g '*.sln'Repository: Resgrid/Core
Length of output: 5959
🏁 Script executed:
# Check the test project's target framework
head -20 Tests/Resgrid.Tests/Resgrid.Tests.csprojRepository: Resgrid/Core
Length of output: 918
🏁 Script executed:
# Search for usages of Resgrid.Web.Tts in the test project
rg -l 'Resgrid\.Web\.Tts' Tests/Resgrid.Tests/ --type cs | head -20Repository: Resgrid/Core
Length of output: 549
Target framework mismatch with Resgrid.Web.Tts — tests should run against the same framework as production.
This test project targets net9.0, while Resgrid.Web.Tts targets net8.0. The nine TTS-specific tests in Tests/Resgrid.Tests/Web/Tts/ exercise the TTS service directly, meaning they run against net9.0 assemblies even though the service is designed for net8.0. If the TTS service is deployed as net8.0 in production, the tests may not catch net8.0-specific issues or behavior differences. Consider multi-targeting Resgrid.Web.Tts to include net9.0, or aligning the test project to net8.0, to ensure tests validate the actual production framework.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Resgrid.Tests.csproj` at line 59, The test project
targets net9.0 while the referenced Resgrid.Web.Tts project targets net8.0,
causing framework mismatch; fix by either changing the
Tests/Resgrid.Tests.csproj TargetFramework to net8.0 so tests run against the
same runtime as Resgrid.Web.Tts, or update Resgrid.Web.Tts.csproj to
multi-target (TargetFrameworks) to include net9.0 (e.g., net8.0;net9.0) so both
frameworks are available for the ProjectReference; after making the change,
restore and rebuild to ensure the TTS tests in Tests/Resgrid.Tests/Web/Tts/ run
against the intended framework.
Comment on lines
+56
to
+58
| knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("10.42.0.0")) && x.PrefixLength == 16); | ||
| knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("::ffff:10.42.0.0")) && x.PrefixLength == 16); | ||
| } |
Contributor
There was a problem hiding this comment.
Test locks in the incorrect IPv6 prefix length.
After the IPNetwork(MapToIPv6(), prefixLength + 96) fix in TtsRequestIdentity.AddKnownNetwork, update this assertion to the IPv6‑correct value:
💚 Proposed fix
- knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("::ffff:10.42.0.0")) && x.PrefixLength == 16);
+ knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("::ffff:10.42.0.0")) && x.PrefixLength == 112);📝 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.
Suggested change
| knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("10.42.0.0")) && x.PrefixLength == 16); | |
| knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("::ffff:10.42.0.0")) && x.PrefixLength == 16); | |
| } | |
| knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("10.42.0.0")) && x.PrefixLength == 16); | |
| knownNetworks.Should().Contain(x => x.Prefix.Equals(IPAddress.Parse("::ffff:10.42.0.0")) && x.PrefixLength == 112); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Web/Tts/TtsRequestIdentityTests.cs` around lines 56 - 58,
The test is asserting the wrong IPv6 prefix length after
TtsRequestIdentity.AddKnownNetwork was changed to use IPNetwork(MapToIPv6(),
prefixLength + 96); update the expectation in TtsRequestIdentityTests so the
assertion that matches the IPv6-mapped network
(IPAddress.Parse("::ffff:10.42.0.0")) checks for a PrefixLength of 112 (i.e. 16
+ 96) rather than 16; locate the assertion lines in TtsRequestIdentityTests.cs
that reference IPAddress.Parse("::ffff:10.42.0.0") and change the expected
PrefixLength accordingly.
Comment on lines
+19
to
+48
| [TestFixture] | ||
| public class TtsStaticPromptRefreshTaskTests | ||
| { | ||
| private static readonly FieldInfo WorkerBootstrapperContainerField = typeof(Resgrid.Workers.Framework.Bootstrapper) | ||
| .GetField("_container", BindingFlags.Static | BindingFlags.NonPublic)!; | ||
|
|
||
| private IContainer _originalWorkerContainer; | ||
| private IContainer _testWorkerContainer; | ||
| private string _originalServiceBaseUrl; | ||
| private string _originalStaticPromptAdminKey; | ||
|
|
||
| [SetUp] | ||
| public void SetUp() | ||
| { | ||
| _originalWorkerContainer = WorkerBootstrapperContainerField.GetValue(null) as IContainer; | ||
| _originalServiceBaseUrl = TtsConfig.ServiceBaseUrl; | ||
| _originalStaticPromptAdminKey = TtsConfig.StaticPromptAdminKey; | ||
|
|
||
| TtsConfig.ServiceBaseUrl = "https://tts.example.com"; | ||
| TtsConfig.StaticPromptAdminKey = "prompt-admin-key"; | ||
| } | ||
|
|
||
| [TearDown] | ||
| public void TearDown() | ||
| { | ||
| WorkerBootstrapperContainerField.SetValue(null, _originalWorkerContainer); | ||
| _testWorkerContainer?.Dispose(); | ||
| TtsConfig.ServiceBaseUrl = _originalServiceBaseUrl; | ||
| TtsConfig.StaticPromptAdminKey = _originalStaticPromptAdminKey; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against cross-test interference on static state.
This fixture mutates static state on Resgrid.Workers.Framework.Bootstrapper._container and TtsConfig.ServiceBaseUrl/StaticPromptAdminKey. If NUnit parallelization is enabled anywhere (assembly/namespace/class level), another fixture touching those statics concurrently will cause flaky failures. Mark the fixture (or the assembly) as non-parallelizable to make that constraint explicit.
🛠️ Proposed fix
[TestFixture]
+ [NonParallelizable]
public class TtsStaticPromptRefreshTaskTests📝 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.
Suggested change
| [TestFixture] | |
| public class TtsStaticPromptRefreshTaskTests | |
| { | |
| private static readonly FieldInfo WorkerBootstrapperContainerField = typeof(Resgrid.Workers.Framework.Bootstrapper) | |
| .GetField("_container", BindingFlags.Static | BindingFlags.NonPublic)!; | |
| private IContainer _originalWorkerContainer; | |
| private IContainer _testWorkerContainer; | |
| private string _originalServiceBaseUrl; | |
| private string _originalStaticPromptAdminKey; | |
| [SetUp] | |
| public void SetUp() | |
| { | |
| _originalWorkerContainer = WorkerBootstrapperContainerField.GetValue(null) as IContainer; | |
| _originalServiceBaseUrl = TtsConfig.ServiceBaseUrl; | |
| _originalStaticPromptAdminKey = TtsConfig.StaticPromptAdminKey; | |
| TtsConfig.ServiceBaseUrl = "https://tts.example.com"; | |
| TtsConfig.StaticPromptAdminKey = "prompt-admin-key"; | |
| } | |
| [TearDown] | |
| public void TearDown() | |
| { | |
| WorkerBootstrapperContainerField.SetValue(null, _originalWorkerContainer); | |
| _testWorkerContainer?.Dispose(); | |
| TtsConfig.ServiceBaseUrl = _originalServiceBaseUrl; | |
| TtsConfig.StaticPromptAdminKey = _originalStaticPromptAdminKey; | |
| } | |
| [TestFixture] | |
| [NonParallelizable] | |
| public class TtsStaticPromptRefreshTaskTests | |
| { | |
| private static readonly FieldInfo WorkerBootstrapperContainerField = typeof(Resgrid.Workers.Framework.Bootstrapper) | |
| .GetField("_container", BindingFlags.Static | BindingFlags.NonPublic)!; | |
| private IContainer _originalWorkerContainer; | |
| private IContainer _testWorkerContainer; | |
| private string _originalServiceBaseUrl; | |
| private string _originalStaticPromptAdminKey; | |
| [SetUp] | |
| public void SetUp() | |
| { | |
| _originalWorkerContainer = WorkerBootstrapperContainerField.GetValue(null) as IContainer; | |
| _originalServiceBaseUrl = TtsConfig.ServiceBaseUrl; | |
| _originalStaticPromptAdminKey = TtsConfig.StaticPromptAdminKey; | |
| TtsConfig.ServiceBaseUrl = "https://tts.example.com"; | |
| TtsConfig.StaticPromptAdminKey = "prompt-admin-key"; | |
| } | |
| [TearDown] | |
| public void TearDown() | |
| { | |
| WorkerBootstrapperContainerField.SetValue(null, _originalWorkerContainer); | |
| _testWorkerContainer?.Dispose(); | |
| TtsConfig.ServiceBaseUrl = _originalServiceBaseUrl; | |
| TtsConfig.StaticPromptAdminKey = _originalStaticPromptAdminKey; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Workers/Console/Tasks/TtsStaticPromptRefreshTaskTests.cs`
around lines 19 - 48, The test fixture TtsStaticPromptRefreshTaskTests mutates
shared static state (Resgrid.Workers.Framework.Bootstrapper._container and
TtsConfig.ServiceBaseUrl/StaticPromptAdminKey) which can cause flaky failures if
NUnit runs tests in parallel; mark the fixture as non-parallelizable by adding
the NUnit [NonParallelizable] attribute to the TtsStaticPromptRefreshTaskTests
class (or alternatively apply [assembly: NonParallelizable] at the test
assembly) so tests that touch Bootstrapper._container and TtsConfig.* run
sequentially and avoid cross-test interference.
Member
Author
|
Approve |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Infrastructure
Tests