Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds configurable Twilio ChangesTwilio voice and TTS flow
Health endpoints and dependency probes
TTS text preprocessing
Department defaults
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TwilioController
participant TwilioVoiceResponseService
participant TtsService
Caller->>TwilioController: Submit voice webhook
TwilioController->>TwilioVoiceResponseService: Render prompt with time budget
TwilioVoiceResponseService->>TtsService: Generate audio
TtsService-->>TwilioVoiceResponseService: Audio or timeout/failure
TwilioVoiceResponseService-->>TwilioController: Play or Say TwiML
TwilioController-->>Caller: Twilio voice response
sequenceDiagram
participant Monitor
participant TtsHealthEndpoint
participant TtsFullHealthCheck
participant Redis
participant Storage
participant AudioProcessingService
Monitor->>TtsHealthEndpoint: Request /health/full
TtsHealthEndpoint->>TtsFullHealthCheck: Run functional probe
TtsFullHealthCheck->>Redis: Write and read probe marker
TtsFullHealthCheck->>Storage: Check probe object
TtsFullHealthCheck->>AudioProcessingService: Generate normalized WAV
TtsFullHealthCheck-->>TtsHealthEndpoint: HealthReport with component data
TtsHealthEndpoint-->>Monitor: JSON health response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Service Locator dependency resolution.
Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs#L20-L23: resolveICacheProviderthroughBootstrapper.GetKernel().Resolve<T>().Web/Resgrid.Web.Services/Health/TtsServiceHealthCheck.cs#L22-L25: resolveIHttpClientFactorythroughBootstrapper.GetKernel().Resolve<T>().As per coding guidelines, “Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs` around lines 20 - 23, Replace constructor injection in RedisHealthCheck by resolving ICacheProvider through Bootstrapper.GetKernel().Resolve<T>(); apply the same constructor-resolution change in Web/Resgrid.Web.Services/Health/TtsServiceHealthCheck.cs at lines 22-25 for IHttpClientFactory. Remove the corresponding constructor parameters while preserving each class’s existing dependency fields and behavior.Source: Coding guidelines
Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs (1)
95-131: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding concurrency in the chunk fan-out.
Task.WhenAllfires one TTS request per chunk with no upper bound. This was previously self-limiting because callers appended one prompt at a time; TwilioController.cs now joins large lists (all active calls, all users, all units, all calendar events) into a single prompt before calling this method, so a big department can generate many chunks that all hit the TTS microservice simultaneously in one call. Worst case this self-heals via the caller's 6s budget (falls back to<Say>), but it needlessly hammers the TTS service for large lists.Consider capping concurrent chunk generation (e.g.,
SemaphoreSlim) or capping the number of joined items per prompt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs` around lines 95 - 131, The chunk fan-out in CreatePlayVerbsAsync launches an unbounded TTS request for every chunk. Add bounded concurrency around GetOrCreatePromptUrlAsync, using a SemaphoreSlim or equivalent limit while preserving cancellation and the existing fallback behavior, so large prompts do not hammer the TTS service simultaneously.Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs (2)
138-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo server-side logging of probe failures.
Failures/timeouts are only surfaced transiently in the HTTP response; nothing is logged when a probe fails, making it hard to correlate historical outages if monitoring doesn't scrape frequently enough or a poll is missed. Consider injecting
ILogger<TtsFullHealthCheck>and logging at warning/error level inside the catch blocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs` around lines 138 - 152, Update TtsFullHealthCheck to inject and retain an ILogger<TtsFullHealthCheck>, then log probe timeouts at warning level and unexpected probe failures at error level within the existing catch blocks, including the probe name and exception details where available. Preserve the current response data and errors.Add behavior.
73-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential probes bound worst-case latency to 30s.
Each probe gets its own fresh 10s
ComponentTimeout, butRunProbesAsyncawaits redis, s3, and synthesis one after another, so a full timeout cascade on/health/fullcan take up to ~30 seconds. Since this endpoint is intended for active monitoring/diagnostics, consider running the probes concurrently to bound total latency to the single slowest probe's timeout instead of their sum.♻️ Sketch of parallel execution (requires thread-safe collections)
- var data = new Dictionary<string, object>(); - var errors = new List<string>(); - - await ProbeAsync("redis", data, errors, async token => { ... }, cancellationToken); - await ProbeAsync("s3", data, errors, async token => { ... }, cancellationToken); - await ProbeAsync("synthesis", data, errors, async token => { ... }, cancellationToken); + var data = new ConcurrentDictionary<string, object>(); + var errors = new ConcurrentBag<string>(); + + await Task.WhenAll( + ProbeAsync("redis", data, errors, async token => { ... }, cancellationToken), + ProbeAsync("s3", data, errors, async token => { ... }, cancellationToken), + ProbeAsync("synthesis", data, errors, async token => { ... }, cancellationToken));Also applies to: 126-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs` around lines 73 - 115, Update RunProbesAsync so the redis, s3, and synthesis ProbeAsync calls start concurrently and are awaited together, bounding total probe time to the slowest ComponentTimeout rather than their sum. Preserve each probe’s existing behavior and ensure shared data/error collection is safe for concurrent updates.Web/Resgrid.Web.Tts/Program.cs (1)
133-146: 🩺 Stability & Availability | 🔵 TrivialConsider restricting
/health/fullgiven it performs real work.Unlike
/health, this endpoint triggers a live Piper/ffmpeg synthesis call, an S3 reachability check, and a Redis round-trip. It's mapped here with noRequireRateLimiting(...)/auth, and the existing"tts"rate-limit policy isn't applied to health endpoints. Since the comments describe this as diagnostics-only ("do not wire probes to this endpoint"), consider enforcing that at the network/ingress layer or via an explicit auth/rate-limit requirement, so it can't be hammered externally to repeatedly trigger the underlying synthesis pipeline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Tts/Program.cs` around lines 133 - 146, Restrict the deep diagnostic endpoint mapped by the `/health/full` `MapHealthChecks` call so it cannot be accessed or repeatedly invoked by unauthenticated external clients. Apply the project’s established authentication or rate-limiting mechanism, or enforce an equivalent network/ingress restriction, while leaving the shallow `/health` probe publicly usable for orchestration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs`:
- Around line 43-46: Log caught dependency-probe exceptions with
Resgrid.Framework.Logging.LogException(ex, ...) before returning the unhealthy
result in RedisHealthCheck.cs (lines 43-46), SqlDatabaseHealthCheck.cs (lines
38-41), and TtsServiceHealthCheck.cs (lines 51-54).
- Around line 25-36: Update RedisHealthCheck.CheckHealthAsync to create and use
a short local timeout cancellation token for both SetStringAsync and
GetStringAsync instead of the default token. Catch cancellation caused by that
timeout and return an unhealthy HealthCheckResult, while preserving the existing
failure handling and successful round-trip behavior.
In `@Web/Resgrid.Web.Services/Startup.cs`:
- Around line 751-756: Restrict the `/health/full` endpoint registration in
`Startup.Configure` to trusted monitoring by applying a dedicated monitoring
authorization policy or equivalent ingress-only requirement. Ensure
authentication and authorization are enforced before the deep SQL, Redis, and
TTS probes execute, while leaving only the shallow `/health` endpoint publicly
accessible.
In `@Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs`:
- Around line 148-152: Update the exception handler in the full health-check
probe flow to log the complete exception server-side, while adding only a
generic probe-failure message to the response errors collection. Preserve the
existing timing/status entry and OperationCanceledException filtering, and
ensure ex.Message is not included in HealthCheckResult.Description.
In `@Web/Resgrid.Web.Tts/Services/TextPreprocessor.cs`:
- Around line 270-276: Update DispatchShorthandMap to support the intended
casing of the “etc” shorthand used by the case-sensitive matcher, ensuring
uppercase ETC and mixed-case Etc expand without changing matching behavior for
other entries.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs`:
- Around line 20-23: Replace constructor injection in RedisHealthCheck by
resolving ICacheProvider through Bootstrapper.GetKernel().Resolve<T>(); apply
the same constructor-resolution change in
Web/Resgrid.Web.Services/Health/TtsServiceHealthCheck.cs at lines 22-25 for
IHttpClientFactory. Remove the corresponding constructor parameters while
preserving each class’s existing dependency fields and behavior.
In `@Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs`:
- Around line 95-131: The chunk fan-out in CreatePlayVerbsAsync launches an
unbounded TTS request for every chunk. Add bounded concurrency around
GetOrCreatePromptUrlAsync, using a SemaphoreSlim or equivalent limit while
preserving cancellation and the existing fallback behavior, so large prompts do
not hammer the TTS service simultaneously.
In `@Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs`:
- Around line 138-152: Update TtsFullHealthCheck to inject and retain an
ILogger<TtsFullHealthCheck>, then log probe timeouts at warning level and
unexpected probe failures at error level within the existing catch blocks,
including the probe name and exception details where available. Preserve the
current response data and errors.Add behavior.
- Around line 73-115: Update RunProbesAsync so the redis, s3, and synthesis
ProbeAsync calls start concurrently and are awaited together, bounding total
probe time to the slowest ComponentTimeout rather than their sum. Preserve each
probe’s existing behavior and ensure shared data/error collection is safe for
concurrent updates.
In `@Web/Resgrid.Web.Tts/Program.cs`:
- Around line 133-146: Restrict the deep diagnostic endpoint mapped by the
`/health/full` `MapHealthChecks` call so it cannot be accessed or repeatedly
invoked by unauthenticated external clients. Apply the project’s established
authentication or rate-limiting mechanism, or enforce an equivalent
network/ingress restriction, while leaving the shallow `/health` probe publicly
usable for orchestration.
🪄 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 Plus
Run ID: 06fed034-f892-4f80-98cf-5fbc34fd7050
⛔ Files ignored due to path filters (4)
Tests/Resgrid.Tests/Web/Services/HealthChecksTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/TwilioVoiceResponseServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Tts/TextPreprocessorTests.csis excluded by!**/Tests/**
📒 Files selected for processing (15)
Core/Resgrid.Config/TtsConfig.csCore/Resgrid.Services/DepartmentsService.csProviders/Resgrid.Providers.Number/OutboundVoiceProvider.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web.Services/Health/HealthResponseWriter.csWeb/Resgrid.Web.Services/Health/RedisHealthCheck.csWeb/Resgrid.Web.Services/Health/SqlDatabaseHealthCheck.csWeb/Resgrid.Web.Services/Health/TtsServiceHealthCheck.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web.Services/Twilio/ITwilioVoiceResponseService.csWeb/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.csWeb/Resgrid.Web.Tts/Health/TtsFullHealthCheck.csWeb/Resgrid.Web.Tts/Program.csWeb/Resgrid.Web.Tts/Services/TextPreprocessor.cs
| public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default) | ||
| { | ||
| try | ||
| { | ||
| // The provider swallows connection failures and returns false/null, so a | ||
| // failed round-trip surfaces here as a value mismatch rather than a throw. | ||
| var stored = await _cacheProvider.SetStringAsync(ProbeKey, "ok", TimeSpan.FromMinutes(1)); | ||
|
|
||
| if (!stored) | ||
| return HealthCheckResult.Unhealthy("Redis write failed; the cache provider reports it is unavailable."); | ||
|
|
||
| var value = await _cacheProvider.GetStringAsync(ProbeKey); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'ICacheProvider.cs' . -x sed -n '1,260p' {}
fd -a -i 'AzureRedisCacheProvider.cs' . -x sed -n '1,320p' {}
rg -n -C3 'SetStringAsync\(|GetStringAsync\(|AddCheck<.*RedisHealthCheck' .Repository: Resgrid/Core
Length of output: 13259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs
ast-grep outline Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs --view expanded
rg -n -C4 'class (.+SqlDatabaseHealthCheck|.*TtsServiceHealthCheck)|CancellationToken cancellationToken|WithTimeout|Timeout|SetCommand|Default|RedisHealthCheck' Web/Resgrid.Web.Services/Health Core/Resgrid.Model/Providers Web/Resgrid.Web.Services/Startup.cs
fd -a 'AzureRedisCacheProvider.cs' .
sed -n '1,360p' $(fd -a 'AzureRedisCacheProvider.cs' . | head -n1)Repository: Resgrid/Core
Length of output: 22872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'AzureRedisCacheProvider.cs' .
for f in $(fd -a 'AzureRedisCacheProvider.cs' .); do
echo '===== ' "$f" ' ====='
sed -n '1,360p' "$f"
doneRepository: Resgrid/Core
Length of output: 150
🌐 Web query:
site:github.com/Resgrid/Core AzureRedisCacheProvider.cs ICacheProvider SetStringAsync GetStringAsync
💡 Result:
In the Resgrid Core architecture, AzureRedisCacheProvider.cs is the implementation of the ICacheProvider interface [1][2]. While specific signature details for SetStringAsync or GetStringAsync are internal to that repository's implementation, the project primarily utilizes a Retrieve pattern rather than direct low-level string set/get operations [1][2]. The ICacheProvider interface typically exposes a generic RetrieveAsync method to handle caching logic, including fallback mechanisms [1][2]. Example usage of the cache provider in the Resgrid codebase: async Task GetFooAsync(int departmentId, bool bypassCache = false) { async Task getFoo { //... actual logic... return foo; } if (!bypassCache && Config.SystemBehaviorConfig.CacheEnabled) return await _cacheProvider.RetrieveAsync(cacheKey, getFoo, cacheDuration); else return await getFoo; } For projects requiring direct interaction with Redis, the Resgrid codebase also uses the standard.NET AddStackExchangeRedisCache integration where appropriate [3]. If you are working directly with the Resgrid Core source, you can find the interface definition at Core/Resgrid.Model/Providers/ICacheProvider.cs and the implementation at Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs [1][2].
Citations:
- 1: https://github.com/Resgrid/Core/blob/master/AGENTS.md
- 2: https://github.com/Resgrid/Core/blob/master/CLAUDE.md
- 3: https://github.com/Resgrid/Core/blob/master/Web/Resgrid.Web.Tts/Program.cs
Bound the Redis health probe.
The SQL and TTS health checks enforce a short probe deadline, but SetStringAsync()/GetStringAsync() receive the default cancellation token. Add a local timeout to CheckHealthAsync() and treat cancellation as an unhealthy probe failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs` around lines 25 - 36,
Update RedisHealthCheck.CheckHealthAsync to create and use a short local timeout
cancellation token for both SetStringAsync and GetStringAsync instead of the
default token. Catch cancellation caused by that timeout and return an unhealthy
HealthCheckResult, while preserving the existing failure handling and successful
round-trip behavior.
| catch (Exception ex) | ||
| { | ||
| return HealthCheckResult.Unhealthy("Redis probe failed.", ex); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log dependency-probe failures before returning unhealthy.
Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs#L43-L46: callResgrid.Framework.Logging.LogException(ex, ...).Web/Resgrid.Web.Services/Health/SqlDatabaseHealthCheck.cs#L38-L41: callResgrid.Framework.Logging.LogException(ex, ...).Web/Resgrid.Web.Services/Health/TtsServiceHealthCheck.cs#L51-L54: callResgrid.Framework.Logging.LogException(ex, ...).
The response writer does not serialize exceptions, so these failures otherwise lack diagnostic detail. As per coding guidelines, “Use Resgrid.Framework.Logging.LogException(...) when catching exceptions.”
📍 Affects 3 files
Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs#L43-L46(this comment)Web/Resgrid.Web.Services/Health/SqlDatabaseHealthCheck.cs#L38-L41Web/Resgrid.Web.Services/Health/TtsServiceHealthCheck.cs#L51-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Health/RedisHealthCheck.cs` around lines 43 - 46,
Log caught dependency-probe exceptions with
Resgrid.Framework.Logging.LogException(ex, ...) before returning the unhealthy
result in RedisHealthCheck.cs (lines 43-46), SqlDatabaseHealthCheck.cs (lines
38-41), and TtsServiceHealthCheck.cs (lines 51-54).
Source: Coding guidelines
| catch (Exception ex) when (ex is not OperationCanceledException) | ||
| { | ||
| data[name] = $"failed ({stopwatch.ElapsedMilliseconds}ms)"; | ||
| errors.Add($"The {name} probe failed: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find relevant files"
fd -a 'TtsFullHealthCheck.cs|Program.cs' . | sed 's#^\./##'
echo
echo "Locate health response serializer/call sites"
rg -n "WriteHealthResponseAsync|/health/full|Description|HealthCheckResult|errors|TtsFullHealthCheck" -S .
echo
echo "Inspect TtsFullHealthCheck around reported lines"
if [ -f Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs ]; then
sed -n '1,230p' Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs | nl -ba | sed -n '1,230p'
fi
echo
echo "Inspect Program.cs relevant health JSON section"
if [ -f Web/Resgrid.Web.Tts/Program.cs ]; then
rg -n -A 80 -B 20 "WriteHealthResponseAsync|HealthCheckResult|health" Web/Resgrid.Web.Tts/Program.cs || true
fiRepository: Resgrid/Core
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked file candidates"
git ls-files 'Web/Resgrid.Web.Tts/**' | sed -n '1,120p'
echo
echo "TtsFullHealthCheck.cs lines 120-170"
sed -n '120,170p' Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs | nl -ba -v120
echo
echo "Program.cs health-related snippets"
rg -n -C 5 'WriteHealthResponseAsync|HealthCheckResult|HealthCheckService|endpoint|mapHealth|/health|full' Web/Resgrid.Web.Tts/Program.cs | sed -n '1,260p'
echo
echo "Restricted endpoints / internal-only references around TTS Web"
rg -n -i 'denyAnonymous|Authorize|AllowAnonymous|/health|WriteHealthResponseAsync|endpointFilter|IPNetwork|XForwardedFor|internal|monitoring' Web/Resgrid.Web.Tts | sed -n '1,260p'Repository: Resgrid/Core
Length of output: 1969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TtsFullHealthCheck.cs lines 120-170"
awk 'NR>=120 && NR<=170 { printf "%d:%s\n", NR, $0 }' Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs
echo
echo "Program.cs health-related snippets"
python3 - <<'PY'
import re
from pathlib import Path
text = Path("Web/Resgrid.Web.Tts/Program.cs").read_text()
lines = text.splitlines()
for i,l in enumerate(lines,1):
if "WriteHealthResponseAsync" in l or "HealthCheckResult" in l or "Health" in l or "/health" in l or "full" in l or "mapHealth" in l or "endpointFilter" in l:
start=max(1,i-8); end=min(len(lines),i+8)
print(f"-- around line {i} --")
for j in range(start,end+1):
print(f"{j}:{lines[j-1]}")
print()
PY
echo
echo "Authorization / network / health restriction references"
rg -n -i 'denyanonymous|authorize|allowanonymous|health|/health|full|internal|monitoring|ipnetwork|x-forwarded|xforwarded|endpointfilter|maphealth|writ healthresponseasync' Web/Resgrid.Web.Tts | sed -n '1,260p'Repository: Resgrid/Core
Length of output: 20602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "WriteHealthResponseAsync and auth middleware ordering"
python3 - <<'PY'
from pathlib import Path
text = Path("Web/Resgrid.Web.Tts/Program.cs").read_text().splitlines()
for i,l in enumerate(text,1):
if "app.UseAuthentication();" in l or "app.UseAuthorization();" in l or l.strip() == "static async Task WriteHealthResponseAsync(HttpContext context, HealthReport report)":
start=max(1,i-12); end=min(len(text),i+45)
print(f"-- around line {i} --")
for j in range(start,end+1):
print(f"{j}:{text[j-1]}")
PY
echo
echo "TtsFullHealthCheck.cs lines 1-115"
awk 'NR>=1 && NR<=115 { printf "%d:%s\n", NR، $0 }' Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs
echo
echo "Authorization-related attribute/config in TTS app"
rg -n 'app\.UseAuthorization|app\.UseAuthentication|MapHealthChecks\([^)]*HealthCheckOptions|DenyAnonymousHostFilter|RequireAuthorization|AllowAnonymous|/health/full' -S .Repository: Resgrid/Core
Length of output: 1771
Do not return probe exception messages from /health/full.
errors.Add($"The {name} probe failed: {ex.Message}") flows into HealthCheckResult.Description and serializes as the JSON description, exposing SDK/client error details to callers. Log the full exception server-side and return a generic probe failure message in the response, even if /health/full is restricted to monitoring access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs` around lines 148 - 152,
Update the exception handler in the full health-check probe flow to log the
complete exception server-side, while adding only a generic probe-failure
message to the response errors collection. Preserve the existing timing/status
entry and OperationCanceledException filtering, and ensure ex.Message is not
included in HealthCheckResult.Description.
| options.MachineDetection = "Enable"; | ||
| // Async AMD: without it Twilio holds the webhook (and the callee hears | ||
| // silence for several seconds) until machine detection completes. Nothing | ||
| // branches on AnsweredBy, so run detection asynchronously and start the | ||
| // dispatch TwiML the moment the call is answered. | ||
| options.AsyncAmd = "true"; | ||
| //options.IfMachine = "Continue"; |
There was a problem hiding this comment.
Missing AsyncAmdStatusCallback causes CallResource.CreateAsync to throw an ApiException (HTTP 400), resulting in silent failures for all outbound voice dispatch calls. Set options.AsyncAmdStatusCallback to a valid webhook URL, or remove both AsyncAmd and MachineDetection to start playback immediately.
// Nothing branches on AnsweredBy, so skip machine detection entirely —
// the webhook fires the moment the call is answered, starting dispatch
// playback immediately without the multi-second AMD hold.
// (If async AMD is still desired, AsyncAmdStatusCallback is required by
// Twilio: options.AsyncAmdStatusCallback = new Uri(...);)Prompt for LLM
File Providers/Resgrid.Providers.Number/OutboundVoiceProvider.cs:
Line 40 to 46:
Missing `AsyncAmdStatusCallback` causes `CallResource.CreateAsync` to throw an ApiException (HTTP 400), resulting in silent failures for all outbound voice dispatch calls. Set `options.AsyncAmdStatusCallback` to a valid webhook URL, or remove both `AsyncAmd` and `MachineDetection` to start playback immediately.
Suggested Code:
// Nothing branches on AnsweredBy, so skip machine detection entirely —
// the webhook fires the moment the call is answered, starting dispatch
// playback immediately without the multi-second AMD hold.
// (If async AMD is still desired, AsyncAmdStatusCallback is required by
// Twilio: options.AsyncAmdStatusCallback = new Uri(...);)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // silence for several seconds) until machine detection completes. Nothing | ||
| // branches on AnsweredBy, so run detection asynchronously and start the | ||
| // dispatch TwiML the moment the call is answered. | ||
| options.AsyncAmd = "true"; |
There was a problem hiding this comment.
Magic string vulnerability identified for the Twilio AsyncAmd option configuration. String literals representing finite sets hurt maintainability and are easily mistyped. Extract to a named constant like private const string TwilioAsyncAmdEnabled = "true"; or use the Twilio SDK's typed enum.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Providers/Resgrid.Providers.Number/OutboundVoiceProvider.cs:
Line 45:
Magic string vulnerability identified for the Twilio `AsyncAmd` option configuration. String literals representing finite sets hurt maintainability and are easily mistyped. Extract to a named constant like `private const string TwilioAsyncAmdEnabled = "true";` or use the Twilio SDK's typed enum.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| [Test] | ||
| public async Task check_health_should_report_healthy_when_all_probes_pass() |
There was a problem hiding this comment.
Naming convention violation identified in test methods. Rule 7 requires PascalCase for .NET methods instead of snake_case. Rename the method to CheckHealthShouldReportHealthyWhenAllProbesPass or use [TestCase] display names.
Kody rule violation: Use proper naming conventions
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/HealthChecksTests.cs:
Line 57:
Naming convention violation identified in test methods. Rule 7 requires PascalCase for .NET methods instead of snake_case. Rename the method to `CheckHealthShouldReportHealthyWhenAllProbesPass` or use `[TestCase]` display names.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .ContinueWith(t => | ||
| { | ||
| if (t.IsFaulted && t.Exception != null) | ||
| Logging.LogException(t.Exception); |
There was a problem hiding this comment.
Missing structured logging context identified in the faulted background TTS pre-warm task. Logging only the exception object prevents operators from correlating failures to specific calls or departments. Pass structured context alongside the exception, such as Logging.LogException(t.Exception, new { op = "PreWarmPrompt", departmentId = call.DepartmentId, callId = call.CallId });.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/TwilioController.cs:
Line 764:
Missing structured logging context identified in the faulted background TTS pre-warm task. Logging only the exception object prevents operators from correlating failures to specific calls or departments. Pass structured context alongside the exception, such as `Logging.LogException(t.Exception, new { op = "PreWarmPrompt", departmentId = call.DepartmentId, callId = call.CallId });`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (points != null && points.Length == 2) | ||
| { | ||
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | ||
| callText.Append(await _geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) |
There was a problem hiding this comment.
Unvalidated external input parsing identified. Using double.Parse on GeoLocationData values throws a FormatException on malformed coordinates. Replace with double.TryParse(points[0], out var lat) && double.TryParse(points[1], out var lng) to avoid exception-driven control flow.
Kody rule violation: Use TryParse for string conversions
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/TwilioController.cs:
Line 598:
Unvalidated external input parsing identified. Using `double.Parse` on `GeoLocationData` values throws a `FormatException` on malformed coordinates. Replace with `double.TryParse(points[0], out var lat) && double.TryParse(points[1], out var lng)` to avoid exception-driven control flow.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| await _twilioVoiceResponseService.AppendPromptAsync(response, text, GetTtsPromptBudgetToken(), ttsLanguage); | ||
| } | ||
| catch (OperationCanceledException) when (TtsBudgetExpired) |
There was a problem hiding this comment.
Unsafe type casting identified. Use the as operator or pattern matching for safe casts and guard null results before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/TwilioController.cs:
Line 1353:
Unsafe type casting identified. Use the `as` operator or pattern matching for safe casts and guard null results before usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | ||
| callText.Append(await _geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) | ||
| .WaitAsync(TimeSpan.FromSeconds(2), HttpContext?.RequestAborted ?? CancellationToken.None) + Environment.NewLine); |
There was a problem hiding this comment.
Magic number identified for the 2-second geocoder timeout. Inline literals scattered across call sites easily drift from extracted timeout budgets like TtsPromptBudget. Define a private static readonly TimeSpan GeocodeTimeout = TimeSpan.FromSeconds(2); and reference it in ProcessTextCommandsAsync and ResolveCallAddressAsync.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/TwilioController.cs:
Line 599:
Magic number identified for the 2-second geocoder timeout. Inline literals scattered across call sites easily drift from extracted timeout budgets like `TtsPromptBudget`. Define a `private static readonly TimeSpan GeocodeTimeout = TimeSpan.FromSeconds(2);` and reference it in `ProcessTextCommandsAsync` and `ResolveCallAddressAsync`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| try | ||
| { | ||
| var client = _httpClientFactory.CreateClient("ByPassSSLHttpClient"); |
There was a problem hiding this comment.
Transport Layer Security (TLS) bypass vulnerability identified by instantiating ByPassSSLHttpClient. Skipping certificate validation allows attackers to execute Man-in-the-Middle (MITM) attacks and intercept health traffic. Enforce TLS 1.2+ with valid cert validation, or install the internal CA in the trust store instead of bypassing validation.
Kody rule violation: Verify SSL/TLS Server Certificates
Prompt for LLM
File Web/Resgrid.Web.Services/Health/TtsServiceHealthCheck.cs:
Line 37:
Transport Layer Security (TLS) bypass vulnerability identified by instantiating `ByPassSSLHttpClient`. Skipping certificate validation allows attackers to execute Man-in-the-Middle (MITM) attacks and intercept health traffic. Enforce TLS 1.2+ with valid cert validation, or install the internal CA in the trust store instead of bypassing validation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Point k8s liveness probes here. | ||
| endpoints.MapHealthChecks("/health", new Microsoft.AspNetCore.Diagnostics.HealthChecks.HealthCheckOptions | ||
| { | ||
| Predicate = registration => !registration.Tags.Contains("full"), |
There was a problem hiding this comment.
Fragile compiler-invisible coupling identified via the duplicated string literal "full" in the liveness predicate and health-check registration. Centralize the tag in a constant such as internal static class HealthCheckTags { public const string Full = "full"; } and reference it in both locations.
Kody rule violation: Centralize string constants
Prompt for LLM
File Web/Resgrid.Web.Services/Startup.cs:
Line 747:
Fragile compiler-invisible coupling identified via the duplicated string literal "full" in the liveness predicate and health-check registration. Centralize the tag in a constant such as `internal static class HealthCheckTags { public const string Full = "full"; }` and reference it in both locations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
|
|
||
| public void AppendSayFallback(Gather gather, string text) |
There was a problem hiding this comment.
Code duplication identified in the AppendSayFallback method body, which verbatim copies logic from line 59. Consolidate into a single overload accepting the common base type: public void AppendSayFallback(TwiML parent, string text).
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Web/Resgrid.Web.Services/Twilio/TwilioVoiceResponseService.cs:
Line 73:
Code duplication identified in the `AppendSayFallback` method body, which verbatim copies logic from line 59. Consolidate into a single overload accepting the common base type: `public void AppendSayFallback(TwiML parent, string text)`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| private readonly IStorageService _storageService; | ||
| private readonly IAudioProcessingService _audioProcessingService; | ||
| private readonly TtsOptions _options; | ||
| private readonly SemaphoreSlim _probeLock = new(1, 1); |
There was a problem hiding this comment.
Resource leak identified due to undisposed SemaphoreSlim instance _probeLock. TtsFullHealthCheck does not implement IDisposable, leaking the internal wait handle. Implement IDisposable on TtsFullHealthCheck to dispose _probeLock, or register the class as a singleton managed by the DI container.
Kody rule violation: Use using statements for disposable resources
Prompt for LLM
File Web/Resgrid.Web.Tts/Health/TtsFullHealthCheck.cs:
Line 27:
Resource leak identified due to undisposed `SemaphoreSlim` instance `_probeLock`. `TtsFullHealthCheck` does not implement `IDisposable`, leaking the internal wait handle. Implement `IDisposable` on `TtsFullHealthCheck` to dispose `_probeLock`, or register the class as a singleton managed by the DI container.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var pattern = $@"\b{Regex.Escape(kvp.Key)}\b"; | ||
| text = Regex.Replace(text, pattern, kvp.Value, RegexOptions.IgnoreCase | RegexOptions.CultureInvariant); | ||
| text = Regex.Replace(text, pattern, kvp.Value, RegexOptions.CultureInvariant); |
There was a problem hiding this comment.
Regular expression Denial-of-Service (DoS) vulnerability identified. Without a timeout, regex processing on untrusted input can be exploited, so always define a timeout when using regex.
Kody rule violation: Specify Timeout for Regular Expressions
Prompt for LLM
File Web/Resgrid.Web.Tts/Services/TextPreprocessor.cs:
Line 256:
Regular expression Denial-of-Service (DoS) vulnerability identified. Without a timeout, regex processing on untrusted input can be exploited, so always define a timeout when using regex.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| private static HttpRequest CreateRequest(string headerValue = null) | ||
| { | ||
| var context = new DefaultHttpContext(); | ||
|
|
||
| if (headerValue != null) | ||
| context.Request.Headers[FullHealthCheckAccess.HeaderName] = headerValue; | ||
|
|
||
| return context.Request; | ||
| } |
There was a problem hiding this comment.
Duplicated logic in the CreateRequest helper method copies verbatim into TtsHealthCheckAccessTests, with only HeaderName differing. Move this logic to a shared base class or static helper that accepts the header name to eliminate the copy.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Tests/Resgrid.Tests/Web/Services/HealthChecksTests.cs:
Line 164 to 172:
Duplicated logic in the `CreateRequest` helper method copies verbatim into `TtsHealthCheckAccessTests`, with only `HeaderName` differing. Move this logic to a shared base class or static helper that accepts the header name to eliminate the copy.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // (SystemBehaviorConfig.FullHealthCheckKey); an unconfigured key fails closed. | ||
| // The shallow /health endpoint stays public for the k8s probes. | ||
| app.UseWhen( | ||
| context => context.Request.Path.StartsWithSegments("/health/full", StringComparison.OrdinalIgnoreCase), |
There was a problem hiding this comment.
Duplicate hardcoded string literal for the route path "/health/full" in the new UseWhen gate diverges from the literal used in app.MapHealthChecks("/health/full", ...) on line 162, risking a silently broken authorization gate if the route changes. Extract a constant such as private const string FullHealthRoute = "/health/full"; and reference it in both StartsWithSegments and MapHealthChecks.
Kody rule violation: Centralize string constants
Prompt for LLM
File Web/Resgrid.Web.Tts/Program.cs:
Line 139:
Duplicate hardcoded string literal for the route path "/health/full" in the new `UseWhen` gate diverges from the literal used in `app.MapHealthChecks("/health/full", ...)` on line 162, risking a silently broken authorization gate if the route changes. Extract a constant such as `private const string FullHealthRoute = "/health/full";` and reference it in both `StartsWithSegments` and `MapHealthChecks`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
2 participants
This PR improves the reliability of voice call webhooks and TTS (text-to-speech) infrastructure, adds health check endpoints, and fixes several bugs in dispatch audio and text preprocessing.
TTS Reliability & Fallback:
TwilioSayFallbackEnabledflag (default off) that degrades voice prompts to Twilio's native<Say>verb when TTS generation fails or times out, preventing 500 errors that Twilio reads to callers as "application error"<Say>once expiredVoice Call Flow Improvements:
Health Check Endpoints:
/health(shallow liveness, no external calls) and/health/full(deep dependency probes) endpoints to both the API and TTS microserviceText Preprocessor Fixes:
Other Changes:
Summary by CodeRabbit
New Features
Bug Fixes