Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an MCP health endpoint and DTO that probes response cache and the Resgrid API, integrates MVC/response cache/HTTP client and Sentry into the MCP web host (Kestrel on port 5050), updates container entrypoint/expose, and instruments server lifecycle and tools with Sentry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant HealthController as "HealthController"
participant ToolRegistry as "McpToolRegistry"
participant ResponseCache as "IResponseCache"
participant HttpFactory as "IHttpClientFactory"
participant ResgridApi as "Resgrid API"
Client->>HealthController: GET /health/current
HealthController->>ToolRegistry: fetch server metadata & tool count
HealthController->>ResponseCache: write sentinel key
ResponseCache-->>HealthController: write ack
HealthController->>ResponseCache: read sentinel key
ResponseCache-->>HealthController: read value
HealthController->>ResponseCache: remove sentinel key
HealthController->>HttpFactory: create client, send HEAD to API base
HttpFactory->>ResgridApi: HEAD /
ResgridApi-->>HttpFactory: HTTP response
HttpFactory-->>HealthController: success/failure
HealthController-->>Client: 200 OK with HealthResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/Controllers/HealthController.cs`:
- Around line 45-65: The health checks are only verifying configuration, not
connectivity; replace the superficial checks in HealthController for CacheOnline
and ApiOnline by performing real probes: for cache use the injected
_responseCache to set and get a short-lived sentinel key (e.g., set a TTL and
verify retrieval) to determine CacheOnline, and for the API perform an HTTP
HEAD/GET to SystemBehaviorConfig.ResgridApiBaseUrl and treat success (2xx) as
ApiOnline; remove the unnecessary try/catch around simple null/whitespace
checks, and if you prefer the lighter option instead of probes rename the
properties to CacheConfigured/ApiConfigured and update callers accordingly
(refer to CacheOnline, ApiOnline, _responseCache, and
SystemBehaviorConfig.ResgridApiBaseUrl to locate changes).
In `@Web/Resgrid.Web.Mcp/Dockerfile`:
- Around line 49-50: The HEALTHCHECK uses curl which is not present in the
dotnet/aspnet:9.0.3-noble-amd64 base image so the probe will always fail; fix by
either installing a HTTP client in the final image (e.g., add apt-get update &&
apt-get install -y curl or wget in the final stage before the HEALTHCHECK) or
replace the HEALTHCHECK command with a .NET-based health probe callable from the
container; update the Dockerfile HEALTHCHECK line accordingly and ensure the
package install happens in the same final stage that defines HEALTHCHECK
(reference: the Dockerfile HEALTHCHECK and the base image
dotnet/aspnet:9.0.3-noble-amd64).
In `@Web/Resgrid.Web.Mcp/Program.cs`:
- Line 1: The file starts with stray BOM characters before the initial
directive; remove the leading BOM bytes so the first token is "using System;"
(i.e., ensure the top of Program.cs begins exactly with the "using System;" line
and no invisible characters precede it) to resolve the build error.
In `@Web/Resgrid.Web.Mcp/Resgrid.Web.Mcp.csproj`:
- Line 1: Remove the stray double UTF-8 BOM at the start of the csproj and
Program.cs so the files begin exactly with "<Project" and "using" respectively;
open Resgrid.Web.Mcp.csproj and Program.cs, delete any invisible BOM characters
at the very start, and save both files as UTF-8 without BOM (or re-encode using
your editor/CLI) so MSBuild sees valid XML/CS source.
🧹 Nitpick comments (5)
Web/Resgrid.Web.Mcp/Dockerfile (1)
8-9: Port 80 is still exposed but appears unused.If Kestrel is only configured to listen on port 5050, the
EXPOSE 80on line 8 is misleading. Consider removing it unless another service in the container listens on 80.Web/Resgrid.Web.Mcp/Program.cs (1)
36-39: Hardcoded port; consider making it configurable.Port 5050 is hardcoded in Kestrel config. For operational flexibility (especially in container orchestrators that assign ports), consider reading it from configuration or an environment variable.
Example
webBuilder.UseKestrel(serverOptions => { - serverOptions.ListenAnyIP(5050); + var port = context.Configuration.GetValue("HealthCheck:Port", 5050); + serverOptions.ListenAnyIP(port); });Web/Resgrid.Web.Mcp/Models/HealthResult.cs (1)
6-42: Consider using arecordfor this immutable DTO.
HealthResultis pure state with no behavior — a good fit for arecordper the project's coding guidelines. This also gives you immutability and value equality for free.Example
public sealed record HealthResult( string SiteId, string ServerVersion, string ServerName, int ToolCount, bool ApiOnline, bool CacheOnline, bool ServerRunning);As per coding guidelines, "Separate state from behavior (e.g., use records for state and place operations in separate static classes)".
Web/Resgrid.Web.Mcp/Controllers/HealthController.cs (2)
16-16: PreferControllerBaseoverControllerfor API-only controllers.
Controlleradds View/Razor support that isn't needed here.ControllerBaseis the appropriate base class for API controllers.Proposed fix
- public sealed class HealthController : Controller + public sealed class HealthController : ControllerBase
33-67: UseOk(result)instead ofJson(result)for a proper API response.
Json()bypasses content negotiation and forces JSON.Ok(result)(fromControllerBase) lets the framework handle serialization and returns a proper200 OKwith content negotiation.- return Json(result); + return Ok(result);
Web/Resgrid.Web.Mcp/Dockerfile
Outdated
| HEALTHCHECK --interval=30s --timeout=10s --retries=3 --start-period=10s \ | ||
| CMD curl -f http://localhost:5050/health/current || exit 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if curl or wget is present in the aspnet base image
echo "Checking for curl..."
docker run --rm mcr.microsoft.com/dotnet/aspnet:9.0.3-noble-amd64 which curl 2>&1 || true
echo "---"
echo "Checking for wget..."
docker run --rm mcr.microsoft.com/dotnet/aspnet:9.0.3-noble-amd64 which wget 2>&1 || trueRepository: Resgrid/Core
Length of output: 191
🏁 Script executed:
#!/bin/bash
# First, locate and examine the Dockerfile
find . -name "Dockerfile" -path "*/Resgrid.Web.Mcp/*" | head -5Repository: Resgrid/Core
Length of output: 88
🌐 Web query:
mcr.microsoft.com/dotnet/aspnet:9.0.3-noble-amd64 curl wget available
💡 Result:
mcr.microsoft.com/dotnet/aspnet:9.0.3-noble-amd64 does not include curl or wget by default (this is consistent with the standard dotnet/aspnet images, where both tools are often absent unless you add them). [1]
If you need them, install via apt in your Dockerfile:
FROM mcr.microsoft.com/dotnet/aspnet:9.0.3-noble-amd64
RUN apt-get update \
&& apt-get install -y --no-install-recommends curl wget ca-certificates \
&& rm -rf /var/lib/apt/lists/*This same approach is commonly required on dotnet/aspnet images when curl isn’t present. [2]
Notes:
- Alpine-based variants typically rely on the (usually present)
wgetinstead ofcurl. [3] - If you use a chiseled/distroless variant, you generally can’t
apt-get installtools at all (no package manager), so you’d need a different strategy. [4]
Sources: [1] [2] [3] [4]
🏁 Script executed:
#!/bin/bash
cat -n ./Web/Resgrid.Web.Mcp/DockerfileRepository: Resgrid/Core
Length of output: 3432
curl is not available in the dotnet/aspnet:9.0.3-noble-amd64 base image.
This HEALTHCHECK will fail with "exec: curl: not found", permanently marking the container as unhealthy. The base image is not installing curl anywhere in the build stages.
Either install curl in the final stage via apt-get, use wget (if available), or implement a .NET-based health check:
Proposed fix
HEALTHCHECK --interval=30s --timeout=10s --retries=3 --start-period=10s \
- CMD curl -f http://localhost:5050/health/current || exit 1
+ CMD wget --no-verbose --tries=1 --spider http://localhost:5050/health/current || exit 1Or install curl in the final stage before the HEALTHCHECK.
📝 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.
| HEALTHCHECK --interval=30s --timeout=10s --retries=3 --start-period=10s \ | |
| CMD curl -f http://localhost:5050/health/current || exit 1 | |
| HEALTHCHECK --interval=30s --timeout=10s --retries=3 --start-period=10s \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:5050/health/current || exit 1 |
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Dockerfile` around lines 49 - 50, The HEALTHCHECK uses
curl which is not present in the dotnet/aspnet:9.0.3-noble-amd64 base image so
the probe will always fail; fix by either installing a HTTP client in the final
image (e.g., add apt-get update && apt-get install -y curl or wget in the final
stage before the HEALTHCHECK) or replace the HEALTHCHECK command with a
.NET-based health probe callable from the container; update the Dockerfile
HEALTHCHECK line accordingly and ensure the package install happens in the same
final stage that defines HEALTHCHECK (reference: the Dockerfile HEALTHCHECK and
the base image dotnet/aspnet:9.0.3-noble-amd64).
Web/Resgrid.Web.Mcp/Program.cs
Outdated
| @@ -1,4 +1,4 @@ | |||
| using System; | |||
| using System; | |||
There was a problem hiding this comment.
Multiple BOM characters — same root cause as the build failure.
Line 1 contains four BOM characters. Strip them so the file starts with using.
-using System;
+using System;📝 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.
| using System; | |
| using System; |
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Program.cs` at line 1, The file starts with stray BOM
characters before the initial directive; remove the leading BOM bytes so the
first token is "using System;" (i.e., ensure the top of Program.cs begins
exactly with the "using System;" line and no invisible characters precede it) to
resolve the build error.
| @@ -1,4 +1,4 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <Project Sdk="Microsoft.NET.Sdk.Web"> | |||
There was a problem hiding this comment.
Double BOM causing build failure — this is the pipeline error.
The file begins with two UTF-8 BOM characters (), which causes MSBuild error MSB4025: "Data at the root level is invalid. Line 1, position 1." Remove the extra BOM so the file starts cleanly with <Project.
The same double-BOM issue also appears in Program.cs (line 1: using). Fix both files.
-<Project Sdk="Microsoft.NET.Sdk.Web">
+<Project Sdk="Microsoft.NET.Sdk.Web">📝 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.
| <Project Sdk="Microsoft.NET.Sdk.Web"> | |
| <Project Sdk="Microsoft.NET.Sdk.Web"> |
🧰 Tools
🪛 GitHub Actions: .NET
[error] 1-1: MSB4025: The project file could not be loaded. Data at the root level is invalid. Line 1, position 1.
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Resgrid.Web.Mcp.csproj` at line 1, Remove the stray
double UTF-8 BOM at the start of the csproj and Program.cs so the files begin
exactly with "<Project" and "using" respectively; open Resgrid.Web.Mcp.csproj
and Program.cs, delete any invisible BOM characters at the very start, and save
both files as UTF-8 without BOM (or re-encode using your editor/CLI) so MSBuild
sees valid XML/CS source.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/appsettings.json`:
- Around line 9-15: The appsettings.json "Sentry" section is unused; either
delete that section or bind it into the programmatic setup in Program.cs: inside
the webBuilder.UseSentry(options => { ... }) lambda, call
configuration.GetSection("Sentry").Bind(options) (or equivalent binding)
before/after applying ExternalErrorConfig values so settings like
IncludeActivityData, MaxBreadcrumbs, MinimumBreadcrumbLevel, MinimumEventLevel
and Debug are actually consumed; alternatively remove the "Sentry" block from
appsettings.json so it no longer confuses operators.
In `@Web/Resgrid.Web.Mcp/Controllers/HealthController.cs`:
- Around line 38-57: The TracesSampler in Program.cs is excluding the wrong
paths for the health endpoint — it checks for "/health/getcurrent" and
"/api/health/getcurrent" while the actual action is HealthController.GetCurrent
exposed at "/health/current"; update the TracesSampler checks to exclude
"/health/current" and "/api/health/current" (and consider making the match
case-insensitive and tolerant of a trailing slash) so real health probes are
sampled out and not sent to Sentry.
In `@Web/Resgrid.Web.Mcp/McpServerHost.cs`:
- Around line 147-169: The catch block currently calls transaction.Finish(ex)
and the finally block always calls transaction.Finish(), causing a double
finish; fix by introducing a boolean guard (e.g., transactionFinished) declared
before the try, set transactionFinished = true after calling
transaction.Finish(ex) in the catch inside McpServerHost.ExecuteAsync, and in
the finally only call transaction.Finish() if transactionFinished is false so
the span is finished exactly once.
In `@Web/Resgrid.Web.Mcp/Program.cs`:
- Line 57: The Sentry configuration currently sets options.SendDefaultPii = true
which sends user PII; change it to false to avoid sending IPs/user
identifiers/cookies unless explicitly required for a justified use-case, i.e.,
update the Sentry options configuration (the options.SendDefaultPii setting in
Program.cs) to set SendDefaultPii = false and, if needed, add a short comment or
feature flag around the Sentry options to document when/why to enable PII
sending.
- Around line 92-99: The middleware pipeline in the Configure lambda is missing
the Sentry tracing middleware so the configured TracesSampler will never run;
inside the webBuilder.Configure(app => { ... }) block, add a call to
app.UseSentryTracing() in the pipeline (place it after app.UseRouting() and
before app.UseEndpoints(...)) so Sentry creates transactions for incoming HTTP
requests and the TracesSampler can apply the health-check exclusion logic.
🧹 Nitpick comments (12)
Web/Resgrid.Web.Mcp/Tools/CallsToolProvider.cs (4)
66-71: Redundanttransaction.Finish()— thefinallyblock already handles it.Lines 69 and 104 both call
transaction.Finish()when the access token is invalid. While Sentry's SDK treats a secondFinish()as a no-op, the early call is unnecessary and clutters the control flow. Let thefinallyblock be the single place that finishes the transaction.♻️ Suggested fix
if (string.IsNullOrWhiteSpace(args?.AccessToken)) { transaction.Status = SpanStatus.InvalidArgument; - transaction.Finish(); return CreateErrorResponse("Access token is required"); }
136-148: Same redundanttransaction.Finish()issue — two early calls here.Both validation branches (lines 139 and 146) call
transaction.Finish()before returning, but thefinallyblock at line 183 will finish the transaction regardless. Remove the early calls for consistency with thefinally-based pattern.♻️ Suggested fix
if (string.IsNullOrWhiteSpace(args?.AccessToken)) { transaction.Status = SpanStatus.InvalidArgument; - transaction.Finish(); return CreateErrorResponse("Access token is required"); } if (args.CallId <= 0) { transaction.Status = SpanStatus.InvalidArgument; - transaction.Finish(); return CreateErrorResponse("Valid call ID is required"); }
189-262:create_callandclose_calllack Sentry instrumentation.The
get_active_callsandget_call_detailstools now have Sentry transaction tracking, breadcrumbs, and exception capture, butcreate_call(andclose_callbelow) do not. This creates an observability gap — especially for the write operations, where tracing failures is arguably more valuable.Consider applying the same Sentry pattern to these two tools for uniform coverage.
60-61: StaticSentrySdkcalls are hidden dependencies that hinder testability.All Sentry interactions (
StartTransaction,AddBreadcrumb,CaptureException) go through the staticSentrySdkfacade, making these paths difficult to unit-test in isolation. Consider injecting a thinIHubor a custom abstraction if you want to verify transaction/breadcrumb behavior in tests. As per coding guidelines, "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions".Web/Resgrid.Web.Mcp/Dockerfile (2)
8-9: Port 80 is exposed but never bound by Kestrel.Program.cs configures Kestrel to listen only on port 5050. The
EXPOSE 80declaration is misleading and may confuse orchestrators or operators. Consider removing it if the MCP service doesn't serve anything on port 80.-EXPOSE 80 EXPOSE 5050
40-48: NoHEALTHCHECKinstruction in the Dockerfile despite adding a health endpoint.The PR adds a
/health/currentendpoint, but the Dockerfile doesn't define aHEALTHCHECKinstruction for container orchestrators to use. Without it, Docker/Compose/Swarm won't automatically probe container readiness. If you intend to rely on Kubernetes liveness/readiness probes or an external orchestrator instead, this is fine — but if Docker-native health checks are desired, add aHEALTHCHECKusing a tool available in the image (e.g., installcurl/wget, or use a .NET-based probe).Web/Resgrid.Web.Mcp/McpServerHost.cs (1)
128-129: Transaction is not bound to the current scope.The transaction created via
SentrySdk.StartTransaction(...)is not set on the current scope. Any child spans created elsewhere (e.g., in tool providers) won't be correlated to this transaction. If you want distributed tracing across the MCP lifecycle, bind it:SentrySdk.ConfigureScope(scope => scope.Transaction = transaction);This is optional if no child spans are expected from this transaction.
Web/Resgrid.Web.Mcp/Program.cs (2)
38-48: Configuration is loaded and parsed twice.Lines 41-48 build a
ConfigurationBuilderand runConfigProcessorinsideConfigureWebHostDefaults. Lines 101-107 build the same configuration again inConfigureAppConfiguration. The first build exists to runConfigProcessorearly (before Sentry init), which is a reasonable motivation, but the duplication is worth noting.Consider extracting the config-loading +
ConfigProcessorinvocation into a shared helper, or runningConfigProcessoronce inConfigureAppConfigurationand moving the Sentry setup toConfigureServiceswhere the processed config values are already available.Also applies to: 101-107
68-83:__HttpPathis an internal Sentry.AspNetCore key — consider a more robust alternative.The
TracesSamplerrelies onsamplingContext.CustomSamplingContext["__HttpPath"], which is an undocumented internal implementation detail of Sentry.AspNetCore and not part of the public API. WhileTransactionContext.Nameis a documented alternative, note thatTracesSamplerexecutes before ASP.NET Core routing completes, so the transaction name may still be "Unknown route" or unavailable at sampling time.For reliable path-based filtering of health checks, use
options.SetBeforeSendTransaction(...)instead, which runs after routing is finalized:options.SetBeforeSendTransaction((transaction, _) => { var name = transaction.TransactionContext?.Name?.ToLower(); if (name == "/health/getcurrent" || name == "/health" || name == "/api/health/getcurrent") return null; // Drop health-check transactions return transaction; });Alternatively, verify your Sentry .NET SDK version is 3.22.0+ (which improved transaction naming with URL paths) and then test whether
TransactionContext.Nameis reliably populated in yourTracesSampler.Web/Resgrid.Web.Mcp/Controllers/HealthController.cs (3)
18-18: UseControllerBaseinstead ofControllerfor an API-only controller.
Controllerinherits fromControllerBaseand adds Razor view support (View(),ViewBag,TempData), none of which are needed here.ControllerBaseis the appropriate base class for API controllers.- public sealed class HealthController : Controller + public sealed class HealthController : ControllerBase
59-103: Silently swallowed exceptions in probe methods make connectivity failures hard to diagnose.Both
ProbeCacheConnectivityAsyncandProbeApiConnectivityAsynccatch all exceptions and returnfalsewith no logging. When a health check starts failing, operators will have no diagnostic information in logs. Consider logging atDebugorWarninglevel inside the catch blocks.This would require injecting
ILogger<HealthController>— which is a single additional dependency.
85-98: HEAD requests may be rejected by some API endpoints.Not all servers/load balancers respond to
HEAD /— some return 405 Method Not Allowed or close the connection. A lightweightGETto a known health or root endpoint may be more reliable. If the Resgrid API has its own health endpoint, target that specifically.
| "Sentry": { | ||
| "IncludeActivityData": true, | ||
| "MaxBreadcrumbs": 50, | ||
| "MinimumBreadcrumbLevel": "Information", | ||
| "MinimumEventLevel": "Error", | ||
| "Debug": false | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "UseSentry|SentryOptions|AddSentry|GetSection.*Sentry" --type=cs -C3Repository: Resgrid/Core
Length of output: 3075
🏁 Script executed:
cat -n Web/Resgrid.Web.Mcp/Program.cs | sed -n '50,80p'Repository: Resgrid/Core
Length of output: 1644
🏁 Script executed:
cat -n Web/Resgrid.Web.Mcp/Program.cs | sed -n '50,110p'Repository: Resgrid/Core
Length of output: 2634
Remove the Sentry section from appsettings.json or add code to consume it.
The Sentry configuration section is not consumed by the application. In Program.cs (lines 53–84), Sentry is configured entirely programmatically via webBuilder.UseSentry(options => {...}) using values from ExternalErrorConfig. The SDK does not automatically read the Sentry section from appsettings.json—this would require an explicit GetSection("Sentry") call within the options lambda. As written, the settings like IncludeActivityData, MaxBreadcrumbs, MinimumBreadcrumbLevel, MinimumEventLevel, and Debug in appsettings.json are ignored and serve only to confuse operators.
Either remove the section entirely or integrate it into the programmatic setup via configuration binding.
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/appsettings.json` around lines 9 - 15, The
appsettings.json "Sentry" section is unused; either delete that section or bind
it into the programmatic setup in Program.cs: inside the
webBuilder.UseSentry(options => { ... }) lambda, call
configuration.GetSection("Sentry").Bind(options) (or equivalent binding)
before/after applying ExternalErrorConfig values so settings like
IncludeActivityData, MaxBreadcrumbs, MinimumBreadcrumbLevel, MinimumEventLevel
and Debug are actually consumed; alternatively remove the "Sentry" block from
appsettings.json so it no longer confuses operators.
| [HttpGet("current")] | ||
| public async Task<IActionResult> GetCurrent() | ||
| { | ||
| var result = new HealthResult | ||
| { | ||
| ServerVersion = Assembly.GetEntryAssembly()?.GetName().Version?.ToString() ?? "Unknown", | ||
| ServerName = McpConfig.ServerName ?? "Resgrid MCP Server", | ||
| SiteId = "0", | ||
| ToolCount = _toolRegistry.GetToolCount(), | ||
| ServerRunning = true | ||
| }; | ||
|
|
||
| // Check cache connectivity with real probe | ||
| result.CacheOnline = await ProbeCacheConnectivityAsync(); | ||
|
|
||
| // Check API connectivity with real probe | ||
| result.ApiOnline = await ProbeApiConnectivityAsync(); | ||
|
|
||
| return Json(result); | ||
| } |
There was a problem hiding this comment.
Route mismatch: TracesSampler excludes wrong paths for this endpoint.
The actual health endpoint route is /health/current (Line 17 [Route("health")] + Line 38 [HttpGet("current")]). However, in Program.cs Lines 74-76, the TracesSampler checks for /health/getcurrent and /api/health/getcurrent — neither matches the real route. Health check probes will be sampled and sent to Sentry, adding noise.
Fix the paths in Program.cs:
- if (path == "/health/getcurrent" ||
- path == "/health" ||
- path == "/api/health/getcurrent")
+ if (path == "/health/current" ||
+ path == "/health" ||
+ path?.StartsWith("/health/") == true)🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Controllers/HealthController.cs` around lines 38 - 57,
The TracesSampler in Program.cs is excluding the wrong paths for the health
endpoint — it checks for "/health/getcurrent" and "/api/health/getcurrent" while
the actual action is HealthController.GetCurrent exposed at "/health/current";
update the TracesSampler checks to exclude "/health/current" and
"/api/health/current" (and consider making the match case-insensitive and
tolerant of a trailing slash) so real health probes are sampled out and not sent
to Sentry.
| { | ||
| options.Dsn = ExternalErrorConfig.ExternalErrorServiceUrlForMcp; | ||
| options.AttachStacktrace = true; | ||
| options.SendDefaultPii = true; |
There was a problem hiding this comment.
SendDefaultPii = true sends user PII to Sentry — compliance risk.
This enables automatic collection of IP addresses, user identifiers, cookies, and other personally identifiable information in Sentry events. This may violate GDPR/CCPA requirements depending on your deployment context and data processing agreements with Sentry.
Unless there's a specific need for PII in error reports, default to false:
- options.SendDefaultPii = true;
+ options.SendDefaultPii = false;📝 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.
| options.SendDefaultPii = true; | |
| options.SendDefaultPii = false; |
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Program.cs` at line 57, The Sentry configuration
currently sets options.SendDefaultPii = true which sends user PII; change it to
false to avoid sending IPs/user identifiers/cookies unless explicitly required
for a justified use-case, i.e., update the Sentry options configuration (the
options.SendDefaultPii setting in Program.cs) to set SendDefaultPii = false and,
if needed, add a short comment or feature flag around the Sentry options to
document when/why to enable PII sending.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/Program.cs`:
- Around line 73-74: Replace the culture-sensitive .ToLower() comparison on
samplingContext.CustomSamplingContext["__HttpPath"] with an ordinal
case-insensitive comparison: check that samplingContext.CustomSamplingContext
contains "__HttpPath", get its string value safely, and use string.Equals(value,
"/health/getcurrent", StringComparison.OrdinalIgnoreCase) (or equivalent) to
compare; update the condition where samplingContext.CustomSamplingContext and
"__HttpPath" are referenced to avoid the ToLower() allocation and potential
locale issues.
- Line 59: Assembly.GetEntryAssembly() can be null causing an NRE in the
options.Release assignment; update the assignment in Program.cs (the line using
Assembly.GetEntryAssembly() and options.Release) to use a safe fallback and
null-safe access (for example get a non-null assembly via
Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly(), then guard
GetName() and Version and default to a fallback string like "unknown" if any
part is null) so options.Release never gets set from a null reference.
🧹 Nitpick comments (2)
Web/Resgrid.Web.Mcp/Program.cs (2)
34-41: Configuration is built twice — consider consolidating.The
IConfigurationis constructed manually here (lines 34–38) to feedConfigProcessorbefore Sentry setup, then rebuilt inConfigureAppConfiguration(lines 99–104). This duplication means any source added in one place but not the other will silently diverge. Consider extracting a shared helper that builds the configuration once and reusing it, or moving theConfigProcessorcalls intoConfigureServiceswherehostContext.Configurationis already available (Sentry can be configured conditionally afterward).Also applies to: 99-105
85-89: Hardcoded port 5050 — consider making it configurable.The health-check port is hardcoded. If this conflicts with another service in a deployment, there's no way to override it without a code change. Consider reading the port from configuration.
Proposed fix sketch
- serverOptions.ListenAnyIP(5050); // Health check port + var healthPort = config.GetValue<int>("AppOptions:HealthCheckPort", 5050); + serverOptions.ListenAnyIP(healthPort);
|
|
||
| options.TracesSampleRate = ExternalErrorConfig.SentryPerfSampleRate; | ||
| options.Environment = ExternalErrorConfig.Environment; | ||
| options.Release = Assembly.GetEntryAssembly().GetName().Version.ToString(); |
There was a problem hiding this comment.
Assembly.GetEntryAssembly() can return null — potential NullReferenceException.
In certain hosting scenarios (e.g., running under test hosts or some containerized environments), GetEntryAssembly() may return null. This would cause an NRE on .GetName().Version.ToString().
Proposed fix
- options.Release = Assembly.GetEntryAssembly().GetName().Version.ToString();
+ options.Release = Assembly.GetEntryAssembly()?.GetName().Version?.ToString() ?? "unknown";🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Program.cs` at line 59, Assembly.GetEntryAssembly() can
be null causing an NRE in the options.Release assignment; update the assignment
in Program.cs (the line using Assembly.GetEntryAssembly() and options.Release)
to use a safe fallback and null-safe access (for example get a non-null assembly
via Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly(), then guard
GetName() and Version and default to a fallback string like "unknown" if any
part is null) so options.Release never gets set from a null reference.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/Program.cs`:
- Around line 40-41: The code assigns configResult and envConfigResult from
ConfigProcessor.LoadAndProcessConfig and
ConfigProcessor.LoadAndProcessEnvVariables but never checks them; add a
fail-fast check after those calls (similar to other startups) that if both
configResult and envConfigResult are false then log an explicit error and stop
startup (throw an InvalidOperationException or call Environment.Exit/return) so
the app fails early with clear context instead of later throwing when
SystemBehaviorConfig.ResgridApiBaseUrl or related settings are missing; update
the block around ConfigProcessor.LoadAndProcessConfig/LoadAndProcessEnvVariables
to perform this validation and emit a helpful error message referencing the
failed config load.
🧹 Nitpick comments (5)
Web/Resgrid.Web.Mcp/Program.cs (5)
34-38: Duplicate configuration loading.The same
appsettings.json+ environment variables sources are built here (lines 34–38) and again inConfigureAppConfiguration(lines 103–108). Consider extracting a shared helper or building the configuration once and reusing it to keep things DRY.
47-48: Remove commented-out code.Lines 47–48, 54–55, and 65–66 contain commented-out configuration. These are dead code that adds noise. If they're needed for reference, capture them in documentation or a commit message instead.
Also applies to: 54-55, 62-67
88-92: Health-check port is hardcoded.Port
5050is hardcoded. Consider making it configurable via configuration (e.g.,config["HealthCheckPort"]) to allow flexibility across environments without rebuilding.
93-101:UseSentryTracing()called unconditionally but Sentry is configured conditionally.
UseSentryTracing()on line 96 runs regardless of whether Sentry was initialized (lines 43–86 are conditional). While the Sentry SDK treats this as a no-op when not initialized, wrapping it in the same condition would make the intent explicit and avoid potential issues with future SDK versions.Proposed adjustment
webBuilder.Configure(app => { app.UseRouting(); - app.UseSentryTracing(); + if (!string.IsNullOrWhiteSpace(ExternalErrorConfig.ExternalErrorServiceUrlForMcp)) + { + app.UseSentryTracing(); + } app.UseEndpoints(endpoints => { endpoints.MapControllers(); }); });
112-112: Unused variableconfiguration.
hostContext.Configurationis captured intoconfigurationbut never referenced. Remove it to avoid confusion.-var configuration = hostContext.Configuration;
| bool configResult = ConfigProcessor.LoadAndProcessConfig(config["AppOptions:ConfigPath"]); | ||
| bool envConfigResult = ConfigProcessor.LoadAndProcessEnvVariables(config.AsEnumerable()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the Program.cs file and check its size
find . -name "Program.cs" -path "*/Web/Resgrid.Web.Mcp/*"Repository: Resgrid/Core
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for ConfigProcessor usage to understand the context
rg -t cs "ConfigProcessor" --max-count 20Repository: Resgrid/Core
Length of output: 3028
🏁 Script executed:
#!/bin/bash
# Search for uses of configResult and envConfigResult variables
rg -t cs "configResult|envConfigResult" -B2 -A2Repository: Resgrid/Core
Length of output: 6184
🏁 Script executed:
#!/bin/bash
# Read the Program.cs file to see the full context around lines 40-41
sed -n '1,150p' ./Web/Resgrid.Web.Mcp/Program.cs | cat -nRepository: Resgrid/Core
Length of output: 6931
🏁 Script executed:
#!/bin/bash
# Find and read ConfigProcessor.cs to understand what LoadAndProcessConfig and LoadAndProcessEnvVariables do
find . -name "ConfigProcessor.cs" -type fRepository: Resgrid/Core
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for the LoadAndProcessConfig method implementation
rg -t cs "public static.*LoadAndProcessConfig" -A 15Repository: Resgrid/Core
Length of output: 1145
🏁 Script executed:
#!/bin/bash
# Read more of ConfigProcessor.cs to understand what happens when config is not found
sed -n '1,100p' ./Core/Resgrid.Config/ConfigProcessor.cs | cat -nRepository: Resgrid/Core
Length of output: 3458
🏁 Script executed:
#!/bin/bash
# Also search for LoadAndProcessEnvVariables
rg -t cs "public static.*LoadAndProcessEnvVariables" -A 15Repository: Resgrid/Core
Length of output: 1158
Configuration load failures are silently ignored — startup will proceed with potentially missing configuration.
configResult and envConfigResult are assigned but never checked. If LoadAndProcessConfig or LoadAndProcessEnvVariables return false, the application continues without error, potentially with incomplete configuration. This leads to confusing failures downstream (e.g., line 136-140 checks SystemBehaviorConfig.ResgridApiBaseUrl and will throw an InvalidOperationException with unclear root cause if configuration failed to load).
Other similar files in the codebase (e.g., Web/Resgrid.Web/Startup.cs, Web/Resgrid.Web.Services/Startup.cs) validate these return values with if (!configResult && !envConfigResult). Apply the same pattern here to fail fast and provide clear error context.
Proposed fix
bool configResult = ConfigProcessor.LoadAndProcessConfig(config["AppOptions:ConfigPath"]);
bool envConfigResult = ConfigProcessor.LoadAndProcessEnvVariables(config.AsEnumerable());
+
+ if (!configResult && !envConfigResult)
+ {
+ throw new InvalidOperationException(
+ "Failed to load Resgrid configuration from both config file and environment variables. " +
+ "Ensure AppOptions:ConfigPath is set or RESGRID__* environment variables are configured.");
+ }📝 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.
| bool configResult = ConfigProcessor.LoadAndProcessConfig(config["AppOptions:ConfigPath"]); | |
| bool envConfigResult = ConfigProcessor.LoadAndProcessEnvVariables(config.AsEnumerable()); | |
| bool configResult = ConfigProcessor.LoadAndProcessConfig(config["AppOptions:ConfigPath"]); | |
| bool envConfigResult = ConfigProcessor.LoadAndProcessEnvVariables(config.AsEnumerable()); | |
| if (!configResult && !envConfigResult) | |
| { | |
| throw new InvalidOperationException( | |
| "Failed to load Resgrid configuration from both config file and environment variables. " + | |
| "Ensure AppOptions:ConfigPath is set or RESGRID__* environment variables are configured."); | |
| } |
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Program.cs` around lines 40 - 41, The code assigns
configResult and envConfigResult from ConfigProcessor.LoadAndProcessConfig and
ConfigProcessor.LoadAndProcessEnvVariables but never checks them; add a
fail-fast check after those calls (similar to other startups) that if both
configResult and envConfigResult are false then log an explicit error and stop
startup (throw an InvalidOperationException or call Environment.Exit/return) so
the app fails early with clear context instead of later throwing when
SystemBehaviorConfig.ResgridApiBaseUrl or related settings are missing; update
the block around ConfigProcessor.LoadAndProcessConfig/LoadAndProcessEnvVariables
to perform this validation and emit a helpful error message referencing the
failed config load.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Approve |
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores