Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughAdds a new Model Context Protocol (MCP) server project with many tool providers and supporting infrastructure (API client, audit, rate limiting, caching, token refresh), host/Docker support, expanded CI/CD with multi-image Docker builds and GitHub Releases, a voice-service guard fix, and ImageSharp version bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Mcp as MCP Server
participant Registry as Tool Registry
participant Provider as Tool Provider
participant ApiClient as ApiClient
participant ResgridAPI as Resgrid API
Client->>Mcp: JSON-RPC tool_call (stdio)
Mcp->>Registry: dispatch to registered provider
Registry->>Provider: invoke handler
Provider->>ApiClient: HTTP Get/Post/Put/Delete (Bearer token)
ApiClient->>ResgridAPI: HTTP request
ResgridAPI-->>ApiClient: HTTP response
ApiClient-->>Provider: deserialized result
Provider-->>Mcp: tool result
Mcp-->>Client: JSON-RPC response
sequenceDiagram
participant Client as Client
participant AuthProv as AuthenticationToolProvider
participant ApiClient as ApiClient
participant TokenSvc as TokenRefreshService
participant Cache as Token Cache
participant TokenServer as Token Server
Client->>AuthProv: authenticate(username,password)
AuthProv->>ApiClient: AuthenticateAsync
ApiClient->>TokenServer: POST /connect/token
TokenServer-->>ApiClient: access_token, refresh_token, expires_in
ApiClient->>TokenSvc: CacheToken(userId, tokens)
TokenSvc->>Cache: store token
ApiClient-->>AuthProv: AuthenticationResult
AuthProv-->>Client: auth response with accessToken
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical 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 (1)
Core/Resgrid.Services/VoiceService.cs (1)
266-268:⚠️ Potential issue | 🔴 Critical
addonPlansis not null-checked here — same bug you just fixed inCanDepartmentUseVoiceAsync.Line 268 calls
addonPlans.Select(...)without a null/empty guard. IfGetAllAddonPlansByTypeAsyncreturnsnull, this throws aNullReferenceException. Apply the same guard pattern used inCanDepartmentUseVoiceAsync.Proposed fix
var result = new DepartmentVoiceUtilization(); var addonPlans = await _subscriptionsService.GetAllAddonPlansByTypeAsync(PlanAddonTypes.PTT); -var addonPayment = await _subscriptionsService.GetCurrentPaymentAddonsForDepartmentAsync(departmentId, addonPlans.Select(x => x.PlanAddonId).ToList()); -if (addonPayment != null && addonPayment.Count > 0) +if (addonPlans != null && addonPlans.Any()) { + var addonPayment = await _subscriptionsService.GetCurrentPaymentAddonsForDepartmentAsync(departmentId, addonPlans.Select(x => x.PlanAddonId).ToList()); + + if (addonPayment != null && addonPayment.Count > 0) + { foreach (var payment in addonPayment) { result.SeatLimit += (int)(payment.Quantity * 10); } var livekitProvider = new LiveKitProvider(); var voiceChannels = await _departmentVoiceChannelRepository.GetDepartmentVoiceChannelByDepartmentIdAsync(departmentId); if (voiceChannels != null) { foreach (var channel in voiceChannels) { var participants = await livekitProvider.ListRoomParticipants(channel.DepartmentVoiceChannelId); if (participants != null) result.CurrentlyActive += participants.Count(); } } + } }
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.cs`:
- Around line 53-57: Reset currently calls _counters.TryRemove(clientId, ...)
but keys are stored as "{clientId}:{operation}", so nothing is removed; update
the Reset method to enumerate the _counters keys (e.g., _counters.Keys or
_counters.ToList()) and for each key that StartsWith(clientId + ":") call
_counters.TryRemove(key, out _); keep the debug log (optionally include how many
keys removed); reference the Reset method and the _counters concurrent
dictionary used to store "{clientId}:{operation}" keys.
In `@Web/Resgrid.Web.Mcp/McpToolRegistry.cs`:
- Around line 92-102: Remove the duplicated inventory registration: delete the
repeated calls to _inventoryTools.RegisterTools(server) and
_registeredTools.AddRange(_inventoryTools.GetToolNames()) that appear a second
time, and restore consistent indentation so the reports block uses the same
tabbing style as the preceding code; ensure only one inventory registration
remains and that the subsequent reports registration uses
_reportsTools.RegisterTools(server) and
_registeredTools.AddRange(_reportsTools.GetToolNames()) with matching
indentation.
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs`:
- Around line 188-215: The JSON-RPC DTOs use PascalCase properties but
incoming/outgoing JSON uses lowercase/camelCase keys; update JsonRpcRequest,
JsonRpcResponse, JsonRpcError, and ToolCallParams by either annotating each
property with System.Text.Json.Serialization.JsonPropertyName to match
"jsonrpc", "id", "method", "params", "name", "arguments", "result", "error",
"code", "message", "data", etc., or consistently pass a JsonSerializerOptions
instance (e.g., PropertyNamingPolicy = JsonNamingPolicy.CamelCase and
PropertyNameCaseInsensitive = true) to all JsonSerializer.Serialize/Deserialize
calls so deserialization/serialization matches the JSON-RPC field names.
- Around line 48-50: In the loop inside the McpServer class that awaits
Console.In.ReadLineAsync(), change the handling of the returned value so that if
ReadLineAsync() returns null (EOF) you break out of the loop instead of
continuing; specifically, replace the string.IsNullOrEmpty(line) continue check
with a null check (line == null) that breaks the loop, while preserving any
existing handling for empty strings if needed.
In `@Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs`:
- Around line 183-187: The PUT currently sends the whole args (which contains
AccessToken) as the request body in CalendarToolProvider where you call
_apiClient.PutAsync at "/api/v4/Calendar/UpdateItem"; change the body to an
anonymous object that copies only the calendar fields required for update
(exclude AccessToken) — mirror the pattern used in create_calendar_item — and
keep args.AccessToken as the separate auth parameter to PutAsync so the token is
not serialized into the JSON payload.
🟠 Major comments (16)
Web/Resgrid.Web.Mcp/Tools/SchemaBuilder.cs-32-47 (1)
32-47: 🛠️ Refactor suggestion | 🟠 MajorRedundant dictionary creation when
Itemsis set.When
kvp.Value.Items != null, lines 33-37 create a dictionary that is immediately discarded and replaced by lines 41-46. Build the dictionary once:Suggested refactor
private static Dictionary<string, object> BuildProperties(Dictionary<string, PropertySchema> properties) { var props = new Dictionary<string, object>(); foreach (var kvp in properties) { - props[kvp.Key] = new Dictionary<string, object> + var prop = new Dictionary<string, object> { ["type"] = kvp.Value.Type, ["description"] = kvp.Value.Description }; if (kvp.Value.Items != null) { - props[kvp.Key] = new Dictionary<string, object> - { - ["type"] = kvp.Value.Type, - ["items"] = new Dictionary<string, object> { ["type"] = kvp.Value.Items }, - ["description"] = kvp.Value.Description - }; + prop["items"] = new Dictionary<string, object> { ["type"] = kvp.Value.Items }; } + + props[kvp.Key] = prop; } return props; }.github/workflows/dotnet.yml-44-47 (1)
44-47:⚠️ Potential issue | 🟠 Major
docker-build-and-pushjob is missing apermissionsblock.As flagged by CodeQL, this job doesn't restrict
GITHUB_TOKENpermissions. While it primarily needs Docker Hub credentials (via secrets), it should still declare minimal permissions to follow the principle of least privilege.Suggested fix
docker-build-and-push: needs: build-and-test if: github.event_name == 'push' && github.ref == 'refs/heads/master' runs-on: ubuntu-latest + + permissions: + contents: readWeb/Resgrid.Web.Mcp/Dockerfile-39-46 (1)
39-46:⚠️ Potential issue | 🟠 MajorContainer runs as root and downloads an unverified third-party binary.
Two concerns with the final stage:
No non-root user (flagged by Trivy DS-0002): The container runs as root by default, which is a security risk in production. Add a non-root user.
Unverified binary download (Line 41): The
waitbinary is fetched from a third-party GitHub release without checksum verification. A compromised release or MITM could inject a malicious binary into the image.Suggested fix
FROM base AS final + +# Create non-root user +RUN adduser --disabled-password --gecos "" --uid 1001 appuser + ## Add the wait script to the image -ADD https://github.com/ufoscout/docker-compose-wait/releases/download/2.9.0/wait wait -RUN chmod +x wait +ADD --chmod=755 https://github.com/ufoscout/docker-compose-wait/releases/download/2.9.0/wait wait +# TODO: Add checksum verification for the wait binary WORKDIR /app COPY --from=publish /app/publish . + +USER appuser ENTRYPOINT ["sh", "-c", "./wait && dotnet Resgrid.Web.Mcp.dll"]Web/Resgrid.Web.Mcp/ApiClient.cs-81-101 (1)
81-101:⚠️ Potential issue | 🟠 MajorMixed JSON libraries:
System.Text.Jsonfor serialization,Newtonsoft.Jsonfor deserialization.
PostAsJsonAsync/PutAsJsonAsync(Lines 113, 136) useSystem.Text.Jsoninternally, whileJsonConvert.DeserializeObjectuses Newtonsoft.Json. Different default behaviors (camelCase vs. PascalCase, null handling, etc.) can cause silent data loss or mismatches. Pick one library consistently.🐛 Option A: Use Newtonsoft.Json consistently
-using System.Net.Http.Json; // ... // In PostAsync: -var response = await client.PostAsJsonAsync(endpoint, request, cancellationToken); +var json = JsonConvert.SerializeObject(request); +var content = new StringContent(json, System.Text.Encoding.UTF8, "application/json"); +var response = await client.PostAsync(endpoint, content, cancellationToken); // In PutAsync: -var response = await client.PutAsJsonAsync(endpoint, request, cancellationToken); +var json = JsonConvert.SerializeObject(request); +var content = new StringContent(json, System.Text.Encoding.UTF8, "application/json"); +var response = await client.PutAsync(endpoint, content, cancellationToken);Also applies to: 103-124, 126-147
Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs-68-72 (1)
68-72:⚠️ Potential issue | 🟠 MajorUser-supplied date strings are concatenated into the URL without encoding.
args.StartDateandargs.EndDateare injected directly into the query string. Malformed or malicious input could break the URL or inject additional query parameters. UseUri.EscapeDataString.🛡️ Proposed fix
if (!string.IsNullOrWhiteSpace(args.StartDate) && !string.IsNullOrWhiteSpace(args.EndDate)) { - endpoint += $"?startDate={args.StartDate}&endDate={args.EndDate}"; + endpoint += $"?startDate={Uri.EscapeDataString(args.StartDate)}&endDate={Uri.EscapeDataString(args.EndDate)}"; }Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.cs-59-81 (1)
59-81:⚠️ Potential issue | 🟠 Major
CleanupExpiredmay remove counters that still have recent requests.
LastRequestreturnsPeek()(the oldest entry in the queue). If a counter contains both old and recent requests, the oldest will be past the cutoff, causing the entire counter to be removed — losing track of recent requests and effectively resetting the rate limit for that client/operation pair.Either call
CleanupOldRequestson each counter first and then check if it's empty, or check the newest timestamp instead.🐛 Proposed fix
private void CleanupExpired(object state) { - var cutoff = DateTime.UtcNow.Subtract(Window); var keysToRemove = new System.Collections.Generic.List<string>(); + var now = DateTime.UtcNow; foreach (var kvp in _counters) { - if (kvp.Value.LastRequest < cutoff) + kvp.Value.CleanupOldRequests(now, Window); + if (kvp.Value.Count == 0) { keysToRemove.Add(kvp.Key); } }Web/Resgrid.Web.Mcp/Tools/MessagesToolProvider.cs-74-78 (1)
74-78:⚠️ Potential issue | 🟠 MajorAvoid leaking internal exception details to clients.
Every
catchblock passesex.Messagedirectly toCreateErrorResponse, which is returned to the MCP caller. Exception messages can contain sensitive internal details (database connection strings, stack info, internal paths). Return a generic error message instead and keep the details in the log only.This applies to all tool providers in this PR (InventoryToolProvider, ShiftsToolProvider, PersonnelToolProvider, etc.).
🛡️ Proposed fix (apply to all catch blocks across all providers)
catch (Exception ex) { _logger.LogError(ex, "Error retrieving inbox"); - return CreateErrorResponse(ex.Message); + return CreateErrorResponse("An internal error occurred. Please try again later."); }Also applies to: 120-124, 177-181, 224-228, 271-275
Web/Resgrid.Web.Mcp/Infrastructure/TokenRefreshService.cs-87-107 (1)
87-107:⚠️ Potential issue | 🟠 Major
RefreshTokenAsyncis a stub that always returns failure —GetValidTokenAsyncwill silently returnnullfor any expired or uncached token.This means the
ITokenRefreshServiceis effectively non-functional for its primary purpose. If any downstream code relies onGetValidTokenAsyncreturning a valid token after refresh, it will getnullwith only a warning-level log. At minimum, this should be clearly documented or throwNotImplementedExceptionso callers fail fast rather than silently degrading.Do you want me to open an issue to track implementing the refresh token grant flow?
Web/Resgrid.Web.Mcp/Program.cs-52-53 (1)
52-53:⚠️ Potential issue | 🟠 MajorConfig load results are captured but never checked.
configResultandenvConfigResultare assigned but never inspected. If configuration loading fails, the server will start with potentially missing or invalid settings, leading to hard-to-diagnose runtime failures.🛡️ Proposed fix
bool configResult = ConfigProcessor.LoadAndProcessConfig(configuration["AppOptions:ConfigPath"]); bool envConfigResult = ConfigProcessor.LoadAndProcessEnvVariables(configuration.AsEnumerable()); + +if (!configResult && !envConfigResult) +{ + throw new InvalidOperationException("Failed to load Resgrid configuration from file or environment variables."); +}Web/Resgrid.Web.Mcp/Infrastructure/TokenRefreshService.cs-19-32 (1)
19-32:⚠️ Potential issue | 🟠 Major
TokenRefreshServicecreates aTimerbut never disposes it — resource leak.The
_cleanupTimeris created in the constructor but the class does not implementIDisposable. Since this is registered as a singleton, the timer will run for the process lifetime, but if the service is ever replaced or the container is disposed, the timer will keep firing. ImplementIDisposableto clean up the timer.🛡️ Proposed fix
-public sealed class TokenRefreshService : ITokenRefreshService +public sealed class TokenRefreshService : ITokenRefreshService, IDisposable { // ... existing fields ... + public void Dispose() + { + _cleanupTimer?.Dispose(); + } + // ... rest of class ... }Web/Resgrid.Web.Mcp/Program.cs-71-86 (1)
71-86:⚠️ Potential issue | 🟠 MajorNo guard when
ApiBaseUrlis missing — all API calls will fail at runtime.If
McpServer:ApiBaseUrlis not configured,client.BaseAddressis never set. Every tool provider uses relative URLs (e.g.,/api/v4/...), andHttpClientwill throwInvalidOperationExceptionat runtime when attempting those requests. Fail fast at startup instead.🛡️ Proposed fix
services.AddHttpClient("ResgridApi", client => { var apiBaseUrl = configuration["McpServer:ApiBaseUrl"]; - if (!string.IsNullOrWhiteSpace(apiBaseUrl)) - { - client.BaseAddress = new Uri(apiBaseUrl); - } + if (string.IsNullOrWhiteSpace(apiBaseUrl)) + { + throw new InvalidOperationException("McpServer:ApiBaseUrl configuration is required."); + } + client.BaseAddress = new Uri(apiBaseUrl); client.DefaultRequestHeaders.Add("Accept", "application/json"); client.Timeout = TimeSpan.FromSeconds(30); })Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs-66-83 (1)
66-83:⚠️ Potential issue | 🟠 Major
argumentsobject is stored inAuditEntry.Detailsand may contain sensitive data.In
LogToolCallAsync, the rawargumentsobject (which may include access tokens or other credentials) is embedded in the audit entry details and subsequently logged. Consider sanitizing or redacting sensitive fields before logging.Web/Resgrid.Web.Mcp/Tools/UnitsToolProvider.cs-77-81 (1)
77-81:⚠️ Potential issue | 🟠 MajorException messages are returned to the MCP client in all tool handlers.
Same concern as in other providers —
ex.Messagemay expose internal details. Use theCreateErrorResponsehelper with a generic message and log the exception separately (which you already do).Proposed fix (apply to all four handlers)
catch (Exception ex) { _logger.LogError(ex, "Error retrieving units"); - return CreateErrorResponse(ex.Message); + return CreateErrorResponse("An internal error occurred"); }Also applies to: 127-131, 194-198, 244-248
Web/Resgrid.Web.Mcp/Tools/AuthenticationToolProvider.cs-90-98 (1)
90-98:⚠️ Potential issue | 🟠 MajorException message leaks internal details to the MCP client.
ex.Messageis included in the error response returned to the caller. This could expose sensitive internal information (e.g., connection strings, stack details). Return a generic message instead.Proposed fix
catch (Exception ex) { _logger.LogError(ex, "Error in authentication tool"); return new { success = false, - error = $"Authentication error: {ex.Message}" + error = "An internal error occurred during authentication" }; }Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs-61-78 (1)
61-78:⚠️ Potential issue | 🟠 MajorError response leaks internal exception details to the client.
ex.Messageis sent asDatain the JSON-RPC error response (line 73 and line 173). This can expose internal implementation details, stack info, or connection strings to untrusted callers.Proposed fix
Error = new JsonRpcError { Code = -32603, Message = "Internal error", - Data = ex.Message + Data = null }Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs-86-101 (1)
86-101:⚠️ Potential issue | 🟠 Major
AuditEntry.ToString()serializesAccessToken, risking token leakage in logs.
ToString()serializes the entire object, includingAccessToken. IfAuditEntryinstances are ever logged (directly or via structured logging that callsToString()), the access token will appear in log output. MarkAccessTokenwith[JsonIgnore]in the serialization output or exclude it fromToString().Proposed fix
+ [JsonIgnore] public string AccessToken { get; set; }Or override
ToString()to exclude it explicitly.
🟡 Minor comments (13)
.github/workflows/dotnet.yml-143-150 (1)
143-150:⚠️ Potential issue | 🟡 MinorRelease creation will fail on workflow re-runs (duplicate tag).
softprops/action-gh-releasecreates git tagv4.<run_number>.0. If the workflow is re-run (samerun_number), the tag already exists and the step will fail. Consider addingmake_latest: trueand handling the case where the tag already exists, or use a more unique version component.Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs-627-632 (1)
627-632:⚠️ Potential issue | 🟡 MinorOverwriting
EnableStaffingSupressmay discard user input on validation failure.Fetching
SuppressStaffingInfohere is correct — the view needs it to render. However, Line 630 overwritesmodel.EnableStaffingSupresswith the persisted DB value, which discards whatever the user submitted in the form. If the user toggled the checkbox and then hits a validation error elsewhere, their selection is silently reverted.Consider only assigning
SuppressStaffingInfoto the model without overwritingEnableStaffingSupress:Suggested fix
model.SuppressStaffingInfo = await _departmentSettingsService.GetDepartmentStaffingSuppressInfoAsync(DepartmentId, false); - if (model.SuppressStaffingInfo != null) - { - model.EnableStaffingSupress = model.SuppressStaffingInfo.EnableSupressStaffing; - }Web/Resgrid.Web.Mcp/Tools/CallsToolProvider.cs-222-285 (1)
222-285:⚠️ Potential issue | 🟡 MinorInconsistent indentation:
RegisterCloseCallTooluses spaces while the rest of the file uses tabs.Lines 222–285 switch to space-based indentation, breaking consistency with the rest of the file. This looks like a copy-paste artifact.
Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs-170-187 (1)
170-187:⚠️ Potential issue | 🟡 MinorMissing
ItemId > 0validation inupdate_calendar_item.Both
dispatch_call(Line 66 in DispatchToolProvider) anddelete_calendar_item(implicitly via int default 0) validate IDs, butupdate_calendar_itemdoesn't check thatItemIdis positive before making the API call.🛡️ Proposed fix
if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + +if (args.ItemId <= 0) +{ + return CreateErrorResponse("Valid item ID is required"); +}Web/Resgrid.Web.Mcp/Tools/ReportsToolProvider.cs-57-91 (1)
57-91:⚠️ Potential issue | 🟡 Minor
startDateandendDateare declaredrequiredin the schema but not validated in the handler.If the MCP server doesn't enforce schema-level validation before invoking the handler, null or empty date values will be forwarded to the API. This applies to all four report-generation tools.
🛡️ Proposed fix (example for generate_calls_report; repeat for other three)
if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + +if (string.IsNullOrWhiteSpace(args.StartDate) || string.IsNullOrWhiteSpace(args.EndDate)) +{ + return CreateErrorResponse("Start date and end date are required"); +}Web/Resgrid.Web.Mcp/ApiClient.cs-45-57 (1)
45-57:⚠️ Potential issue | 🟡 MinorPotential
NullReferenceExceptionif deserialization returns null.If the response body is not valid JSON (e.g., empty or unexpected format),
JsonConvert.DeserializeObject<TokenResponse>(content)returnsnull, and accessingtokenResponse.AccessTokenon Line 53 will throw an NRE. Add a null check.🛡️ Proposed fix
var tokenResponse = JsonConvert.DeserializeObject<TokenResponse>(content); +if (tokenResponse is null) +{ + return new AuthenticationResult + { + IsSuccess = false, + ErrorMessage = "Failed to parse authentication response" + }; +}Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.cs-18-33 (1)
18-33:⚠️ Potential issue | 🟡 Minor
RateLimiterowns aTimerbut does not implementIDisposable.The
_cleanupTimerwill never be disposed, leaking OS resources if the DI container tears down this singleton. Consider implementingIDisposableto dispose the timer.🛡️ Proposed fix
-public sealed class RateLimiter : IRateLimiter +public sealed class RateLimiter : IRateLimiter, IDisposable { // ... existing fields ... + + public void Dispose() + { + _cleanupTimer?.Dispose(); + } }Web/Resgrid.Web.Mcp/appsettings.json-12-17 (1)
12-17:⚠️ Potential issue | 🟡 MinorProduction API URL as default in
appsettings.json.
ApiBaseUrldefaults tohttps://api.resgrid.com, which is the production endpoint. Developers running this locally without overriding the config will make requests against production. Consider using an empty or localhost placeholder and requiring explicit configuration for production.Web/Resgrid.Web.Mcp/Tools/PersonnelToolProvider.cs-172-172 (1)
172-172:⚠️ Potential issue | 🟡 MinorLogging
UserIdat Information level may constitute PII in production logs.
UserIdcould be considered personally identifiable information depending on its format. Consider logging at Debug level or using a hashed/truncated form at Information level to reduce PII exposure in production logs. This also applies to similar patterns in other tool providers that log user-supplied identifiers.Web/Resgrid.Web.Mcp/Tools/InventoryToolProvider.cs-149-183 (1)
149-183:⚠️ Potential issue | 🟡 MinorNo validation on
Quantityforupdate_inventory.The
update_inventorytool only validatesaccessToken. A negativeQuantityvalue would be sent directly to the API. Add a server-side guard to catch obviously invalid values before making the API call.🛡️ Proposed validation
if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + +if (args.Quantity < 0) +{ + return CreateErrorResponse("Quantity cannot be negative"); +}Web/Resgrid.Web.Mcp/McpServerHost.cs-14-34 (1)
14-34:⚠️ Potential issue | 🟡 Minor
_stoppingCtsis never disposed.
CancellationTokenSourceimplementsIDisposable. The linked CTS created on Line 40 allocates internal handles that should be cleaned up. ImplementIDisposableonMcpServerHost(or dispose inStopAsync).🛡️ Proposed fix — dispose in StopAsync
public async Task StopAsync(CancellationToken cancellationToken) { _logger.LogInformation("Stopping Resgrid MCP Server..."); if (_executingTask == null) { return; } try { _stoppingCts?.Cancel(); } finally { await Task.WhenAny(_executingTask, Task.Delay(Timeout.Infinite, cancellationToken)); + _stoppingCts?.Dispose(); } _logger.LogInformation("Resgrid MCP Server stopped"); }Web/Resgrid.Web.Mcp/Tools/MessagesToolProvider.cs-149-183 (1)
149-183:⚠️ Potential issue | 🟡 MinorMissing validation for
subject,body, andrecipientsinsend_message.The schema declares
subject,body, andrecipientsas required, but onlyaccessTokenis validated in code. IfRecipientsis null or empty, an API call will still be attempted with invalid data. Validate before calling the API.🛡️ Proposed validation
if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + +if (string.IsNullOrWhiteSpace(args.Subject)) +{ + return CreateErrorResponse("Subject is required"); +} + +if (string.IsNullOrWhiteSpace(args.Body)) +{ + return CreateErrorResponse("Body is required"); +} + +if (args.Recipients == null || args.Recipients.Length == 0) +{ + return CreateErrorResponse("At least one recipient is required"); +}Web/Resgrid.Web.Mcp/Tools/UnitsToolProvider.cs-136-201 (1)
136-201:⚠️ Potential issue | 🟡 Minor
set_unit_statusdoesn't validateStatusTyperange.The schema description documents valid values 1–13, but the handler only validates
UnitId > 0. An out-of-rangeStatusType(e.g., 0 or 99) will be sent to the API, which may return an opaque error. A client-side range check would provide a better error message.
🧹 Nitpick comments (15)
Web/Resgrid.Web.Mcp/Dockerfile (1)
35-37: Missingdotnet buildstep before publish.The
publishstage jumps straight from thebuildstage (which only didrestore+COPY) topublish. Whiledotnet publishimplicitly compiles, an explicit build step helps catch compilation errors earlier and improves Docker layer caching — the build layer would be reused when only publish args change. This matches the pattern used in the other project Dockerfiles in this repo. Minor, but worth aligning.Web/Resgrid.Web.Mcp/Tools/SchemaBuilder.cs (1)
52-57: Consider makingPropertySchemaimmutable.
PropertySchemais a data carrier that shouldn't need mutation after construction. Usinginitsetters (or arecord) aligns with the coding guidelines preference for immutable data.Suggested refactor
- public sealed class PropertySchema - { - public string Type { get; set; } - public string Description { get; set; } - public string Items { get; set; } // For array types - } + public sealed record PropertySchema + { + public required string Type { get; init; } + public required string Description { get; init; } + public string Items { get; init; } // For array types + }As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#".
Web/Resgrid.Web.Mcp/Resgrid.Web.Mcp.csproj (1)
43-43:System.IO.Pipelinesversion is behind otherSystem.*packages.Other
System.*packages use 9.0.3, butSystem.IO.Pipelinesis pinned to 9.0.0. Consider aligning to 9.0.3 for consistency and to pick up any patches..github/workflows/dotnet.yml (1)
125-141: PR body is written to a file without sanitization.
pr.bodyis user-controlled content written directly torelease_notes.mdand used as the GitHub Release body. While not a direct code execution risk here (it's rendered as Markdown), a malicious PR body could inject misleading release notes. This is acceptable for most internal workflows, but worth noting for supply-chain-aware repositories.Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.cs (1)
35-51: TOCTOU betweenCountcheck andAddRequestallows slight over-admission.
CleanupOldRequests,Count, andAddRequesteach acquire the lock independently. Two concurrent callers can both seeCount < 100and bothAddRequest, admitting 101 requests. For a rate limiter this is a minor inaccuracy but worth noting. A single atomic check-and-add under one lock would be more correct.♻️ Suggested: combine check + add in a single method on RequestCounter
+// In RequestCounter: +public bool TryAddRequest(DateTime now, TimeSpan window, int maxRequests) +{ + lock (_lock) + { + var cutoff = now.Subtract(window); + while (_requests.Count > 0 && _requests.Peek() < cutoff) + _requests.Dequeue(); + + if (_requests.Count >= maxRequests) + return false; + + _requests.Enqueue(now); + return true; + } +}Web/Resgrid.Web.Mcp/Tools/DispatchToolProvider.cs (1)
153-160:TokenArgsandCreateErrorResponseare duplicated across every tool provider.
TokenArgs,CreateErrorResponse, and the access-token validation pattern are copy-pasted identically inDispatchToolProvider,CallsToolProvider,CalendarToolProvider,ReportsToolProvider, and others. Consider extracting these into a shared base or utility class to reduce duplication.Web/Resgrid.Web.Mcp/Infrastructure/ResponseCache.cs (1)
32-50: Cache stampede: concurrent callers can all miss and invoke the factory simultaneously.Between
TryGetValueandSet, multiple threads with the same key will all executefactory(). For expensive or side-effectful factories this wastes resources or causes unexpected behavior. Consider usingSemaphoreSlimper key or aConcurrentDictionary<string, Lazy<Task<T>>>to ensure single-flight execution.Web/Resgrid.Web.Mcp/Tools/ReportsToolProvider.cs (1)
37-263: Four report-generation methods are nearly identical — consider a parameterized helper.
RegisterGenerateCallsReportTool,RegisterGeneratePersonnelReportTool,RegisterGenerateUnitsReportTool, andRegisterGenerateActivityReportTooldiffer only in tool name, description, log message, and endpoint. A single helper method accepting these as parameters would eliminate ~150 lines of duplication.♻️ Sketch
+private void RegisterReportTool(McpServer server, string toolName, string description, string logPrefix, string apiEndpoint) +{ + _toolNames.Add(toolName); + var schema = SchemaBuilder.BuildObjectSchema( + new Dictionary<string, SchemaBuilder.PropertySchema> + { + ["accessToken"] = new SchemaBuilder.PropertySchema { Type = "string", Description = "OAuth2 access token obtained from authentication" }, + ["startDate"] = new SchemaBuilder.PropertySchema { Type = "string", Description = "Start date for report (ISO 8601 format)" }, + ["endDate"] = new SchemaBuilder.PropertySchema { Type = "string", Description = "End date for report (ISO 8601 format)" }, + ["format"] = new SchemaBuilder.PropertySchema { Type = "string", Description = "Report format: 'pdf', 'excel', or 'json' (default: json)" } + }, + new[] { "accessToken", "startDate", "endDate" } + ); + + server.AddTool(toolName, description, schema, async (arguments) => + { + try + { + var args = JsonConvert.DeserializeObject<GenerateReportArgs>(arguments.ToString()); + if (string.IsNullOrWhiteSpace(args?.AccessToken)) + return CreateErrorResponse("Access token is required"); + + _logger.LogInformation("{LogPrefix} from {StartDate} to {EndDate}", logPrefix, args.StartDate, args.EndDate); + var reportData = new { startDate = args.StartDate, endDate = args.EndDate, format = args.Format ?? "json" }; + var result = await _apiClient.PostAsync<object, object>(apiEndpoint, reportData, args.AccessToken); + return new { success = true, data = result, message = $"{logPrefix} generated successfully" }; + } + catch (Exception ex) + { + _logger.LogError(ex, "Error {LogPrefix}", logPrefix); + return CreateErrorResponse(ex.Message); + } + }); +}Web/Resgrid.Web.Mcp/Tools/MessagesToolProvider.cs (1)
283-311: Duplicated argument DTOs across all tool providers.
TokenArgs(and similar patterns like ID-based args) are identically redefined as private nested classes in every tool provider. Consider extracting shared argument types into a common file (e.g.,ToolArguments.cs) to reduce duplication. Per coding guidelines, userecordtypes for these immutable data carriers.♻️ Example shared argument types
// Tools/ToolArguments.cs namespace Resgrid.Web.Mcp.Tools; public sealed record TokenArgs( [property: JsonProperty("accessToken")] string AccessToken ); public sealed record EntityIdArgs( [property: JsonProperty("accessToken")] string AccessToken, [property: JsonProperty("id")] int Id );Web/Resgrid.Web.Mcp/Infrastructure/TokenRefreshService.cs (1)
133-146: Userecordtypes for these immutable data carriers.
TokenCacheandRefreshTokenResultare simple state containers that would benefit from being records, aligning with the coding guideline to prefer immutable data. As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#" and "Use records for state and place operations in separate static classes".♻️ Proposed change
-private sealed class TokenCache -{ - public string AccessToken { get; set; } - public string RefreshToken { get; set; } - public DateTime ExpiresAt { get; set; } -} +private sealed record TokenCache(string AccessToken, string RefreshToken, DateTime ExpiresAt); -private sealed class RefreshTokenResult -{ - public bool IsSuccess { get; set; } - public string AccessToken { get; set; } - public string RefreshToken { get; set; } - public int ExpiresIn { get; set; } -} +private sealed record RefreshTokenResult(bool IsSuccess, string AccessToken = null, string RefreshToken = null, int ExpiresIn = 0);Web/Resgrid.Web.Mcp/McpToolRegistry.cs (1)
26-51: Consider introducing an interface and using a collection to reduce constructor bloat.The constructor takes 11 dependencies (10 providers + logger). All providers share the same
RegisterTools/GetToolNamescontract. Extracting anIToolProviderinterface and acceptingIEnumerable<IToolProvider>would eliminate the need to touch this class every time a new provider is added — the DI container handles the wiring.♻️ Sketch
public interface IToolProvider { void RegisterTools(McpServer server); IEnumerable<string> GetToolNames(); } public sealed class McpToolRegistry { private readonly ILogger<McpToolRegistry> _logger; private readonly IEnumerable<IToolProvider> _providers; private readonly List<string> _registeredTools = new(); public McpToolRegistry(ILogger<McpToolRegistry> logger, IEnumerable<IToolProvider> providers) { _logger = logger; _providers = providers; } public void RegisterTools(McpServer server) { _logger.LogInformation("Registering MCP tools..."); foreach (var provider in _providers) { provider.RegisterTools(server); _registeredTools.AddRange(provider.GetToolNames()); } _logger.LogInformation("Registered {Count} MCP tools", _registeredTools.Count); } }As per coding guidelines, "Prefer composition with interfaces to extend behavior" and "Minimize constructor injection; keep the number of injected dependencies small".
Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs (1)
129-131: Double serialize–deserialize to extractToolCallParams.
request.Paramsis already aJsonElementafter the initial deserialization. Re-serializing it to a string and then deserializing again is unnecessary. Consider usingJsonSerializer.Deserialize<ToolCallParams>(((JsonElement)request.Params).GetRawText())or changingParamstoJsonElement?.Web/Resgrid.Web.Mcp/Tools/AuthenticationToolProvider.cs (1)
26-101: Mixed JSON serializer usage — Newtonsoft for deserialization, System.Text.Json in the transport layer.
McpServerdeserializes the incoming JSON-RPC request usingSystem.Text.Json, soargumentsarriving in the handler will be aSystem.Text.Json.JsonElement. Thearguments.ToString()call on aJsonElementdoes return the raw JSON, so the subsequentJsonConvert.DeserializeObjectworks — but this two-serializer path is fragile and easy to break if someone changes the transport serializer or passes a different runtime type. Consider standardizing on one serializer across the MCP project.Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs (1)
29-49:LogOperationAsynccontains noawait— compiler warning CS1998 and misleading async signature.The only async work (the API call) is commented out. This method currently runs synchronously. Either make it non-async returning
Task.CompletedTask, or add the async call.Quick fix
- public async Task LogOperationAsync(AuditEntry entry) + public Task LogOperationAsync(AuditEntry entry) { try { _logger.LogInformation( "AUDIT: User={UserId}, Operation={Operation}, Success={Success}, Duration={Duration}ms", entry.UserId, entry.Operation, entry.Success, entry.DurationMs ); } catch (Exception ex) { _logger.LogError(ex, "Error writing audit log"); } + return Task.CompletedTask; }Web/Resgrid.Web.Mcp/Tools/UnitsToolProvider.cs (1)
256-260:TokenArgsis duplicated across at least 5 tool providers.
TokenArgswith[JsonProperty("accessToken")]is defined identically inCalendarToolProvider,CallsToolProvider,InventoryToolProvider,MessagesToolProvider, and here. Extract it into a shared class (e.g., inTools/or a common namespace) to reduce duplication.As per coding guidelines, code should follow DRY principles.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.github/workflows/dotnet.yml:
- Around line 146-153: The Create GitHub Release step (uses:
softprops/action-gh-release@v2) is not idempotent because tag_name (v${{
steps.version.outputs.version }}) can already exist on re-runs; change the
workflow to check for an existing tag before running this step and only invoke
the softprops action when the tag does not exist. Implement a prior step (e.g.,
a small actions/github-script or curl/gh check) that queries the repo tags for
the computed tag (use the same expression as tag_name) and sets an
output/boolean like tag_exists, then conditionally run the Create GitHub Release
step using that output so existing tags are detected and the release creation is
skipped or updated intentionally.
In `@Web/Resgrid.Web.Mcp/McpToolRegistry.cs`:
- Around line 100-101: The log currently hardcodes provider count as 10 in
McpToolRegistry when calling _logger.LogInformation; replace the magic constant
with a derived count (e.g. use the actual provider collection like
_registeredProviders.Count if such a field exists, or compute distinct providers
from _registeredTools via _registeredTools.Select(t =>
t.Provider).Distinct().Count()) so the logged provider count always reflects the
real number of providers.
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs`:
- Around line 47-54: The loop in McpServer.cs currently blocks on
Console.In.ReadLineAsync (which isn't cancellation-aware), so replace the direct
await with a cancellation-aware pattern: start the ReadLineAsync task and a
cancellation task (e.g., Task.Delay(Timeout.Infinite, cancellationToken)) and
use Task.WhenAny to await the first to complete; if the cancellation task
finished, break/exit the loop (or throw/cancel) instead of waiting for stdin,
otherwise process the ReadLineAsync result as before. Reference
Console.In.ReadLineAsync and the surrounding while
(!cancellationToken.IsCancellationRequested) loop in McpServer; ensure you
dispose/observe the tasks and avoid leaving a pending ReadLineAsync when
cancellation occurs.
- Around line 137-140: Replace the thrown generic Exception when a tool isn't
found with setting the JSON-RPC error on the response to Invalid params (-32602)
so the client gets a proper client-side error; specifically, in McpServer where
_tools.TryGetValue(toolCallParams.Name, out var toolDef) is checked, stop
throwing and instead assign response.Error = new JsonRpcError { Code = -32602,
Message = $"Tool not found: {toolCallParams.Name}" } (or the project's
equivalent JsonRpc error type) and return the response immediately.
In `@Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs`:
- Around line 68-72: The query-string values StartDate and EndDate are
concatenated raw into the endpoint in CalendarToolProvider (the code building
endpoint = "/api/v4/Calendar/GetItems"), which can produce malformed URLs; fix
by URL-encoding args.StartDate and args.EndDate (e.g., use Uri.EscapeDataString
or HttpUtility.UrlEncode) when constructing the query string so the line that
currently does endpoint +=
$"?startDate={args.StartDate}&endDate={args.EndDate}"; becomes endpoint +=
$"?startDate={Uri.EscapeDataString(args.StartDate)}&endDate={Uri.EscapeDataString(args.EndDate)}";
ensuring you handle null/whitespace checks already present.
🧹 Nitpick comments (6)
Web/Resgrid.Web.Mcp/McpToolRegistry.cs (2)
26-51: Large constructor with 11 dependencies — consider a collection-based approach.The coding guidelines advise minimizing constructor injection. All 10 tool providers share the same shape (
RegisterTools+GetToolNames), so they could implement a common interface (e.g.,IToolProvider) and be injected asIEnumerable<IToolProvider>. This would also eliminate the hardcoded provider count on Line 100 and make adding/removing providers a one-line DI registration change.♻️ Sketch of the collection-based approach
-public McpToolRegistry( - ILogger<McpToolRegistry> logger, - AuthenticationToolProvider authTools, - CallsToolProvider callsTools, - DispatchToolProvider dispatchTools, - PersonnelToolProvider personnelTools, - UnitsToolProvider unitsTools, - MessagesToolProvider messagesTools, - CalendarToolProvider calendarTools, - ShiftsToolProvider shiftsTools, - InventoryToolProvider inventoryTools, - ReportsToolProvider reportsTools) +public McpToolRegistry( + ILogger<McpToolRegistry> logger, + IEnumerable<IToolProvider> toolProviders) { _logger = logger; - _authTools = authTools; - // ... all individual assignments ... + _toolProviders = toolProviders; _registeredTools = new List<string>(); }public void RegisterTools(McpServer server) { _logger.LogInformation("Registering MCP tools..."); - // Register Authentication tools - _authTools.RegisterTools(server); - _registeredTools.AddRange(_authTools.GetToolNames()); - // ... repeated for each provider ... + foreach (var provider in _toolProviders) + { + provider.RegisterTools(server); + _registeredTools.AddRange(provider.GetToolNames()); + } _logger.LogInformation("Registered {Count} MCP tools across {ProviderCount} providers", - _registeredTools.Count, 10); + _registeredTools.Count, _toolProviders.Count()); }As per coding guidelines, "Minimize constructor injection; keep the number of injected dependencies small" and "Prefer composition with interfaces to extend behavior".
56-102:RegisterToolscan be called multiple times, duplicating entries in_registeredTools.There's no guard preventing repeated invocation. Each call appends to
_registeredToolswithout clearing it first. IfRegisterToolsis only ever called once, consider adding a guard or clearing the list at the start.🛡️ Quick guard
public void RegisterTools(McpServer server) { + if (_registeredTools.Count > 0) + { + _logger.LogWarning("Tools already registered — skipping duplicate registration"); + return; + } + _logger.LogInformation("Registering MCP tools...");Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs (1)
259-320: Consider usingrecordtypes orinitsetters for the argument DTOs.These classes are effectively immutable after deserialization. Using C# records or
init-only properties would better communicate intent and align with the guideline to prefer immutable data.As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#".
Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs (2)
133-140: Serialize→Deserialize round-trip to extractToolCallParamsfromrequest.Params.
request.Paramsis typed asobjectbut at runtime will be aJsonElementfromSystem.Text.Json. You can cast and deserialize directly, avoiding the redundant serialization pass:♻️ Proposed fix
- var toolCallParams = JsonSerializer.Deserialize<ToolCallParams>( - JsonSerializer.Serialize(request.Params)); + var paramsElement = (JsonElement)request.Params; + var toolCallParams = JsonSerializer.Deserialize<ToolCallParams>(paramsElement.GetRawText());
65-82:ex.Messageis exposed in error responseDatafield — potential information leak.Internal exception messages (e.g., connection strings, stack details) could reach the client. Consider omitting
Dataor using a sanitized message in production, and logging the full exception server-side only (which you already do on Line 67)..github/workflows/dotnet.yml (1)
122-126: Duplicated version extraction logic across jobs.The
VERSION="4.${{ github.run_number }}.0"formula appears in bothdocker-build-and-push(Line 82) andgithub-release(Line 125). If the versioning scheme changes, both need updating in lockstep.You could extract this into a dedicated lightweight job whose output is consumed by both downstream jobs, or use a reusable workflow. Low risk as-is, though.
| - name: Create GitHub Release | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| tag_name: v${{ steps.version.outputs.version }} | ||
| name: Release v${{ steps.version.outputs.version }} | ||
| body_path: release_notes.md | ||
| draft: false | ||
| prerelease: false |
There was a problem hiding this comment.
❓ Verification inconclusive
softprops/action-gh-release@v2 existing release tag behavior documentation
softprops/action-gh-release@v2 documents this “existing tag/release” behavior in its README / Marketplace listing:
- If the tag already has a GitHub Release, the action updates that existing release (rather than failing/creating a second release), including uploading/updating the configured assets. [1][2]
- If you do not explicitly set release fields (
name,body,draft,prerelease, etc.) and a release already exists for the tag, the existing release keeps its original values for those fields (i.e., they’re retained, not overwritten). [1][2] - Related: asset replacement is controlled by
overwrite_files(documented as defaulting totrue). [1][2]
Sources:
[1] softprops/action-gh-release README (GitHub)
[2] “GH Release” action listing (GitHub Marketplace)
Minor: workflow re-runs may regenerate existing release tags.
If a workflow run is manually re-triggered, github.run_number remains the same, so the tag v4.${{ github.run_number }}.0 would already exist. softprops/action-gh-release@v2 will update the existing release in this case, which is safe but means re-runs are not idempotent — the release notes and other fields will be regenerated from the latest state.
🤖 Prompt for AI Agents
In @.github/workflows/dotnet.yml around lines 146 - 153, The Create GitHub
Release step (uses: softprops/action-gh-release@v2) is not idempotent because
tag_name (v${{ steps.version.outputs.version }}) can already exist on re-runs;
change the workflow to check for an existing tag before running this step and
only invoke the softprops action when the tag does not exist. Implement a prior
step (e.g., a small actions/github-script or curl/gh check) that queries the
repo tags for the computed tag (use the same expression as tag_name) and sets an
output/boolean like tag_exists, then conditionally run the Create GitHub Release
step using that output so existing tags are detected and the release creation is
skipped or updated intentionally.
| _logger.LogInformation("Registered {Count} MCP tools across {ProviderCount} providers", | ||
| _registeredTools.Count, 10); |
There was a problem hiding this comment.
Hardcoded provider count 10 will silently drift.
If a provider is added or removed, this log line will report an incorrect count. Derive it from the actual number of providers or use the collection-based approach above.
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/McpToolRegistry.cs` around lines 100 - 101, The log
currently hardcodes provider count as 10 in McpToolRegistry when calling
_logger.LogInformation; replace the magic constant with a derived count (e.g.
use the actual provider collection like _registeredProviders.Count if such a
field exists, or compute distinct providers from _registeredTools via
_registeredTools.Select(t => t.Provider).Distinct().Count()) so the logged
provider count always reflects the real number of providers.
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/ApiClient.cs`:
- Around line 106-110: The code currently logs full response bodies when
deserialization returns null (see _logger.LogError calls in GetAsync, PostAsync,
and PutAsync), which can leak sensitive data; update each of these handlers to
stop logging the response content and instead log only the endpoint and the HTTP
status code (use the response.StatusCode or equivalent) via
_logger.LogError("Failed to deserialize response from {Endpoint}. StatusCode:
{StatusCode}", endpoint, statusCode) and adjust the thrown
InvalidOperationException to include the endpoint and status code but not the
body; ensure all occurrences of the _logger.LogError that currently include the
content (in methods GetAsync, PostAsync, PutAsync) are changed consistently.
- Around line 80-88: The catch block in ApiClient.cs currently returns the raw
exception message (ex.Message) to the MCP consumer; change the returned
AuthenticationResult in the catch of the authentication routine to use a generic
error string (e.g. "Authentication failed" or "An error occurred during
authentication") instead of ex.Message, while keeping the existing
_logger.LogError(ex, ...) so the full exception stays in server logs; update the
AuthenticationResult returned in that catch to set ErrorMessage to the generic
message and leave IsSuccess = false.
- Around line 50-58: The log statement in ApiClient.cs exposes sensitive token
JSON when DeserializeObject returns null; replace the detailed content log in
the token deserialization failure branch (where tokenResponse is checked after
ReadAsStringAsync/DeserializeObject) with a non-sensitive message or a redacted
placeholder in the _logger.LogError call, e.g. log "Failed to deserialize token
response" and optionally include a safe correlation id or truncated/redacted
snippet but never the raw content; keep returning the AuthenticationResult with
IsSuccess=false and the same ErrorMessage.
In `@Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs`:
- Around line 112-128: The current bare catch in SanitizeSensitiveData silently
swallows all errors; change SanitizeSensitiveData from static to an instance
method so it can use the class _logger, and replace the broad catch with
targeted handling: catch JsonException (or JsonSerializationException) and
return the safe placeholder, but for any other exceptions log the full exception
via _logger (including message/stack) and rethrow so outer LogOperationAsync can
observe/fail; ensure SanitizeJToken usage remains unchanged and update callers
to use the instance method.
- Around line 185-200: The ToString() on AuditLogger currently serializes
Details raw and can leak secrets; update ToString() to pass Details through the
existing SanitizeSensitiveData helper (or a new/shared internal static
sanitizer) before constructing safeObject so Details is scrubbed of fields like
password/token, and make SanitizeSensitiveData accessible (change its visibility
from private to internal static or move it to a shared static helper) so
ToString() can call it; keep AccessToken excluded as before and ensure any
usages like LogToolCallAsync still call the same sanitizer to avoid duplication.
In `@Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.cs`:
- Around line 36-52: The IsAllowedAsync method performs a non-atomic
check-then-act using RequestCounter.Count and then calling
RequestCounter.AddRequest, which allows a TOCTOU race for concurrent requests;
move the compound logic into RequestCounter by adding an atomic method (e.g.,
TryAddIfUnderLimit or TryIncrementWithinWindow) that accepts the current time
and Window and performs CleanupOldRequests, checks the current count against
MaxRequestsPerMinute, and conditionally adds the request under a single internal
lock, then update IsAllowedAsync to call that new RequestCounter method
(referencing IsAllowedAsync, RequestCounter, Count, AddRequest,
CleanupOldRequests, MaxRequestsPerMinute, Window and _counters).
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs`:
- Around line 91-182: The JsonRpcResponse currently serializes both Result and
Error (one as null) which violates JSON-RPC 2.0; update the JsonRpcResponse type
so null members are omitted when serializing (e.g., add the
System.Text.Json.Serialization attribute to the Result and Error properties:
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]), or alternatively
ensure HandleRequestAsync uses JsonSerializer with options that set
IgnoreNullValues/DefaultIgnoreCondition = WhenWritingNull when writing
responses; modify the JsonRpcResponse class (and import
System.Text.Json.Serialization) or change the serialization call to use these
options so responses include only result OR error.
In `@Web/Resgrid.Web.Mcp/Tools/AuthenticationToolProvider.cs`:
- Line 64: The logs in AuthenticationToolProvider currently record plaintext
usernames via _logger.LogInformation and _logger.LogWarning using args.Username,
which leaks PII; change those logging calls to avoid storing the raw username by
either logging a non-PII event (e.g., "Authentication attempt" / "Failed
authentication") or log a deterministic, non-reversible identifier instead
(compute a SHA-256 or HMAC of args.Username and truncate to a safe length) and
log that hashed identifier; update every call site that references args.Username
in _logger.LogInformation/_logger.LogWarning within AuthenticationToolProvider
to use the new pattern and ensure the hashing uses a fixed salt/key if you need
determinism.
In `@Web/Resgrid.Web.Mcp/Tools/ShiftsToolProvider.cs`:
- Around line 194-227: The signup_for_shift handler in ShiftsToolProvider.cs
deserializes SignupShiftArgs but doesn't validate ShiftId and ShiftDayId (they
default to 0) and the catch block is mis-indented; update the async lambda that
processes SignupShiftArgs to validate args.ShiftId and args.ShiftDayId (ensure
they are > 0 and return CreateErrorResponse with clear messages when invalid or
args is null), keep existing AccessToken check, and only call
_apiClient.PostAsync when validation passes; also fix the indentation/brace
placement so the catch block properly pairs with the try in the async
(arguments) => { ... } block to restore correct structure and readability (refer
to SignupShiftArgs, _apiClient.PostAsync, and the surrounding async lambda in
ShiftsToolProvider).
🟡 Minor comments (13)
Web/Resgrid.Web.Mcp/ApiClient.cs-189-206 (1)
189-206:⚠️ Potential issue | 🟡 Minor
DeleteAsynchas an inconsistent error contract compared toGetAsync/PostAsync/PutAsync.
GetAsync,PostAsync, andPutAsyncall callEnsureSuccessStatusCode(), throwingHttpRequestExceptionon non-2xx responses.DeleteAsyncsilently returnsfalseinstead. This means tool providers that callDeleteAsyncdon't get an exception in thecatchblock for HTTP errors — they just getsuccess = falsein the response — while all other verbs throw.Consider making this consistent: either have
DeleteAsyncalso callEnsureSuccessStatusCode()(and change the return type), or document/formalize the difference.Web/Resgrid.Web.Mcp/Tools/MessagesToolProvider.cs-266-292 (1)
266-292:⚠️ Potential issue | 🟡 MinorMissing
MessageIdvalidation indelete_message— destructive operation.
MessageIdArgs.MessageIddefaults to0if omitted or malformed. For a destructive operation like delete, this should be validated before hitting the API, similar to howCalendarToolProvider.delete_calendar_itemvalidatesItemId > 0.🛡️ Proposed fix
var args = JsonConvert.DeserializeObject<MessageIdArgs>(arguments.ToString()); if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + if (args.MessageId <= 0) + { + return CreateErrorResponse("Valid message ID is required"); + } + _logger.LogInformation("Deleting message {MessageId}", args.MessageId);Web/Resgrid.Web.Mcp/Tools/UnitsToolProvider.cs-248-255 (1)
248-255:⚠️ Potential issue | 🟡 MinorInconsistent
catchblock indentation.Same issue as the other providers —
catchat Line 249 doesn't align with thetryat Line 227.🔧 Fix indentation
} - catch (Exception ex) - { - _logger.LogError(ex, "Error retrieving unit locations"); - return CreateErrorResponse("Failed to retrieve unit locations. Please try again later."); - } + catch (Exception ex) + { + _logger.LogError(ex, "Error retrieving unit locations"); + return CreateErrorResponse("Failed to retrieve unit locations. Please try again later."); + } }Web/Resgrid.Web.Mcp/Tools/MessagesToolProvider.cs-219-245 (1)
219-245:⚠️ Potential issue | 🟡 MinorMissing
MessageIdvalidation inget_messageas well.Same issue:
get_messagepasses an unvalidatedMessageIdto the API. While read-only, a0would yield a confusing error from the backend.🛡️ Proposed fix
var args = JsonConvert.DeserializeObject<MessageIdArgs>(arguments.ToString()); if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + if (args.MessageId <= 0) + { + return CreateErrorResponse("Valid message ID is required"); + } + _logger.LogInformation("Retrieving message {MessageId}", args.MessageId);Web/Resgrid.Web.Mcp/Tools/ShiftsToolProvider.cs-100-127 (1)
100-127:⚠️ Potential issue | 🟡 MinorMissing validation for
ShiftId— defaults to0on bad input.
ShiftIdArgs.ShiftIdis anintthat defaults to0if the caller omits or sends a non-numeric value. Unlike the calendar and inventory providers (which validateitemId > 0), this handler sendsshiftId=0straight to the API. Apply the same guard here.🛡️ Proposed fix
var args = JsonConvert.DeserializeObject<ShiftIdArgs>(arguments.ToString()); if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + if (args.ShiftId <= 0) + { + return CreateErrorResponse("Valid shift ID is required"); + } + _logger.LogInformation("Retrieving shift details for {ShiftId}", args.ShiftId);Web/Resgrid.Web.Mcp/Tools/InventoryToolProvider.cs-222-228 (1)
222-228:⚠️ Potential issue | 🟡 MinorInconsistent
catchblock indentation.Same mis-indentation pattern as the other providers — the
catchat Line 223 doesn't align with itstryat Line 205.🔧 Fix indentation
} - catch (Exception ex) - { - _logger.LogError(ex, "Error retrieving low stock items"); - return CreateErrorResponse("Failed to retrieve low stock items. Please try again later."); - } + catch (Exception ex) + { + _logger.LogError(ex, "Error retrieving low stock items"); + return CreateErrorResponse("Failed to retrieve low stock items. Please try again later."); + } }Web/Resgrid.Web.Mcp/Tools/InventoryToolProvider.cs-149-183 (1)
149-183:⚠️ Potential issue | 🟡 MinorMissing
ItemIdvalidation inupdate_inventory— write operation.
update_inventorysends an update to the API without validatingItemId > 0. This is a state-changing operation.CalendarToolProvider.update_calendar_itemvalidatesItemId > 0; this tool should do the same for consistency and safety.🛡️ Proposed fix
var args = JsonConvert.DeserializeObject<UpdateInventoryArgs>(arguments.ToString()); if (string.IsNullOrWhiteSpace(args?.AccessToken)) { return CreateErrorResponse("Access token is required"); } + if (args.ItemId <= 0) + { + return CreateErrorResponse("Valid inventory item ID is required"); + } + _logger.LogInformation("Updating inventory item {ItemId}", args.ItemId);Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs-256-262 (1)
256-262:⚠️ Potential issue | 🟡 MinorInconsistent indentation on
catchblock — same issue as other providers.The
catchat Line 257 is indented one level less than the correspondingtryat Line 234, breaking the visual pairing. This pattern repeats inShiftsToolProviderandInventoryToolProvideras well.🔧 Fix indentation
} - catch (Exception ex) - { - _logger.LogError(ex, "Error deleting calendar item"); - return CreateErrorResponse("Failed to delete calendar item. Please try again later."); - } + catch (Exception ex) + { + _logger.LogError(ex, "Error deleting calendar item"); + return CreateErrorResponse("Failed to delete calendar item. Please try again later."); + } }Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.Test.md-9-18 (1)
9-18:⚠️ Potential issue | 🟡 MinorDocumentation example may be inaccurate:
credentialsvscredential.The sensitive fields list (Lines 9-18) includes
credential(singular), but the nested example on Line 66 shows"credentials"(plural) being redacted. If the implementation uses exact case-insensitive matching on field names,credentialswould not be matched and thus not redacted — making this example misleading. Please verify the actual sanitization behavior and either addcredentialsto the sensitive fields set or correct the example.Also applies to: 57-71
Web/Resgrid.Web.Mcp/Tools/PersonnelToolProvider.cs-249-254 (1)
249-254:⚠️ Potential issue | 🟡 MinorInconsistent indentation —
catchblock is misaligned from itstry.Same issue as in
CallsToolProvider.RegisterCloseCallTool: thecatchon Line 249 is indented one level less than thetryon Line 227, breaking visual structure.Web/Resgrid.Web.Mcp/Tools/DispatchToolProvider.cs-103-151 (1)
103-151:⚠️ Potential issue | 🟡 Minor
get_dispatch_statusmay overlap withget_personnel_statusfromPersonnelToolProvider.This tool calls
/api/v4/Personnel/GetPersonnelStatuses(Line 134), whilePersonnelToolProvider.get_personnel_statuscalls/api/v4/PersonnelStatuses/GetAllStatuses. Both tools describe retrieving personnel status information, which could confuse an LLM picking the right tool. Consider either differentiating the descriptions more clearly or consolidating into one tool if they return equivalent data.Web/Resgrid.Web.Mcp/Tools/CallsToolProvider.cs-222-285 (1)
222-285:⚠️ Potential issue | 🟡 MinorInconsistent indentation —
RegisterCloseCallToolis indented one level less than sibling methods.The method body starting at Line 222 uses class-level indentation (one tab) instead of the two-tab indentation used by the other
Register*methods (e.g.,RegisterGetActiveCallsToolat Line 36). Additionally, thecatchblock on Lines 278-282 is misaligned from its correspondingtryon Line 243. This looks like a copy-paste artifact.🔧 Indentation fix (partial diff)
- private void RegisterCloseCallTool(McpServer server) - { - const string toolName = "close_call"; - _toolNames.Add(toolName); + private void RegisterCloseCallTool(McpServer server) + { + const string toolName = "close_call"; + _toolNames.Add(toolName);Apply the same one-level indent increase to the entire method body (Lines 222-285) and realign the
catchblock with itstry.Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs-46-66 (1)
46-66:⚠️ Potential issue | 🟡 Minor
asyncmethod with noawait— compiler warning CS1998.
LogOperationAsyncis declaredasyncbut contains noawaitexpression (the only one is commented out on Line 60). This produces a compiler warning and runs synchronously despite returning aTask, which misleads callers about actual async behavior.Until external forwarding is wired up, drop
asyncand returnTask.CompletedTask:Proposed fix
- public async Task LogOperationAsync(AuditEntry entry) + public Task LogOperationAsync(AuditEntry entry) { try { // Log locally _logger.LogInformation( "AUDIT: User={UserId}, Operation={Operation}, Success={Success}, Duration={Duration}ms", entry.UserId, entry.Operation, entry.Success, entry.DurationMs ); // Could also send to external audit service or database // await _apiClient.PostAsync<AuditEntry, object>("/api/v4/Audit/LogEntry", entry, entry.AccessToken); } catch (Exception ex) { _logger.LogError(ex, "Error writing audit log"); } + + return Task.CompletedTask; }
🧹 Nitpick comments (16)
.github/workflows/dotnet.yml (2)
27-27: Consider pinning third-party actions to full commit SHAs.All actions (
actions/checkout@v4,docker/login-action@v3,softprops/action-gh-release@v2, etc.) are pinned to mutable major-version tags. A compromised upstream tag could inject malicious code into your pipeline. Pinning to immutable commit SHAs mitigates this supply-chain risk. You can use tools like StepSecurity or Dependabot to keep SHA pins updated.
104-105: Add scope to GHA Docker cache to prevent overwrites across matrix jobs.All five matrix jobs write to the same default GHA cache scope, causing later builds to overwrite earlier cache entries. This reduces hit rates on subsequent runs. Docker's documentation recommends setting a unique
scopeper matrix entry to preserve independent caches.♻️ Proposed fix
- cache-from: type=gha - cache-to: type=gha,mode=max + cache-from: type=gha,scope=${{ matrix.image }} + cache-to: type=gha,scope=${{ matrix.image }},mode=maxWeb/Resgrid.Web.Mcp/Tools/ShiftsToolProvider.cs (1)
233-258:TokenArgs,CreateErrorResponse, and arg base properties are duplicated across every tool provider.
TokenArgs, theAccessTokenproperty, andCreateErrorResponseare copy-pasted identically inCalendarToolProvider,CallsToolProvider,DispatchToolProvider,InventoryToolProvider,MessagesToolProvider,UnitsToolProvider, etc. Consider extracting a sharedToolProviderBaseor at minimum shared DTOs and a static helper to reduce the duplication surface.This is not blocking, but as the provider count grows, so does the maintenance cost.
Web/Resgrid.Web.Mcp/Tools/SchemaBuilder.cs (1)
49-54: Consider makingPropertySchemaimmutable.
PropertySchemais a data carrier that is always set at construction time. Arecordorinit-only properties would align with the coding guidelines' preference for immutable data and prevent accidental mutation.♻️ Optional refactor
- public sealed class PropertySchema - { - public string Type { get; set; } - public string Description { get; set; } - public string Items { get; set; } // For array types - } + public sealed record PropertySchema(string Type, string Description, string Items = null);Web/Resgrid.Web.Mcp/Tools/CalendarToolProvider.cs (1)
68-72: Silently ignores partial date filters.If only
startDateor onlyendDateis provided (but not both), the filter is silently dropped and all items are returned. Consider validating that both are present or neither, and returning an error when only one is supplied.Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.cs (1)
103-112:Disposedoesn't prevent post-disposal use.After
Dispose()is called,IsAllowedAsyncandResetcan still be invoked — they'll operate on the stale_countersdictionary and the timer callback may have already fired one last time. Consider adding anObjectDisposedExceptionguard, or at minimum documenting the expected lifecycle.Web/Resgrid.Web.Mcp/Tools/CallsToolProvider.cs (1)
13-24: Consider extracting a commonIToolProviderinterface.All tool providers (
CallsToolProvider,PersonnelToolProvider,DispatchToolProvider, etc.) share the same public surface:RegisterTools(McpServer)andGetToolNames(). Extracting anIToolProviderinterface would allowMcpToolRegistryto depend onIEnumerable<IToolProvider>instead of each concrete type, making registration open for extension and simplifying DI wiring. As per coding guidelines, "Prefer composition with interfaces to extend behavior".Web/Resgrid.Web.Mcp/McpServerHost.cs (2)
37-73: Faulted_executingTaskbeforeStopAsynccould go unobserved.
ExecuteAsyncis started as a fire-and-forget task on Line 57. If it faults (e.g., an exception escapes despite the catch blocks), theTaskScheduler.UnobservedTaskExceptionevent fires. WhileExecuteAsynccurrently catches all exceptions, a future refactor could miss this. Consider adding a continuation to observe faults:♻️ Optional safety net
_executingTask = ExecuteAsync(_stoppingCts.Token); +_executingTask.ContinueWith(t => + _logger.LogError(t.Exception, "MCP Server background task faulted"), + TaskContinuationOptions.OnlyOnFaulted);
119-128:Disposeshould set_mcpServerto null for GC hygiene.While
McpServeris notIDisposable, setting_mcpServer = nullinDisposewould release the reference to the tool dictionary and logger, aiding garbage collection if the host is long-lived after shutdown.Web/Resgrid.Web.Mcp/Tools/PersonnelToolProvider.cs (1)
172-175: Hardcoded status type range (0–6) is fragile.If the Resgrid API adds new status types, this validation will reject them. Consider removing or relaxing the client-side validation and letting the server API enforce valid ranges, or define the valid values in a shared constant/enum.
Web/Resgrid.Web.Mcp/Tools/DispatchToolProvider.cs (1)
51-100:dispatch_callis a safety-critical mutating operation — consider adding a confirmation or guard.Dispatching personnel and units to an active call is a high-stakes operation in a CAD system. Since MCP tools can be invoked by an LLM, there's a risk of accidental dispatch due to misinterpretation. Consider adding a
confirmboolean parameter to the schema (defaulting tofalse) or including a prominent note in the tool description advising the LLM to always confirm with the user before invoking.Web/Resgrid.Web.Mcp/Tools/AuthenticationToolProvider.cs (1)
31-48: Inconsistent schema construction — other providers useSchemaBuilder.All other tool providers use
SchemaBuilder.BuildObjectSchema(...)to construct input schemas, butAuthenticationToolProviderbuilds the dictionary inline. Consider usingSchemaBuilderfor consistency and to reduce duplication.Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs (1)
133-135: Unnecessary serialize→deserialize round-trip to extractToolCallParams.
request.Paramsis anobjectthat, afterSystem.Text.Jsondeserialization, is aJsonElement. Re-serializing it to a string only to deserialize it again intoToolCallParamsis wasteful. You can cast and deserialize directly from theJsonElement.♻️ Proposed fix
-var toolCallParams = JsonSerializer.Deserialize<ToolCallParams>( - JsonSerializer.Serialize(request.Params)); +var paramsElement = (JsonElement)request.Params; +var toolCallParams = paramsElement.Deserialize<ToolCallParams>();Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs (3)
23-44:IApiClientis injected but never used.The only reference is in a commented-out line (Line 60). This adds an unnecessary dependency that complicates DI registration and unit testing (requires a mock for a service that's never called). Remove it until external audit forwarding is actually implemented.
As per coding guidelines, "Minimize constructor injection; keep the number of injected dependencies small".
♻️ Proposed fix
public sealed class AuditLogger : IAuditLogger { private readonly ILogger<AuditLogger> _logger; - private readonly IApiClient _apiClient; - public AuditLogger(ILogger<AuditLogger> logger, IApiClient apiClient) + public AuditLogger(ILogger<AuditLogger> logger) { _logger = logger; - _apiClient = apiClient; }
68-81: LGTM — authentication audit entry construction is straightforward.Minor note:
userIdandsuccessare duplicated in both the top-levelAuditEntryproperties and theDetailsanonymous object, which slightly bloats records. Consider trimmingDetailsto only additional context not already captured by the entry's fields.
169-183: Consider makingAuditEntrya record with init-only properties.
AuditEntryis a pure data carrier with no behavior beyondToString(). Using arecord(or at minimuminitsetters) would better express immutability after construction and aligns with the project's coding guidelines.As per coding guidelines, "Separate state from behavior (e.g., use records for state and place operations in separate static classes)" and "Prefer functional patterns and immutable data where appropriate in C#".
♻️ Sketch
- public sealed class AuditEntry - { - public string UserId { get; set; } - public string Operation { get; set; } - public bool Success { get; set; } - public DateTime Timestamp { get; set; } - public string IpAddress { get; set; } - public object Details { get; set; } - public long DurationMs { get; set; } - - [JsonIgnore] - public string AccessToken { get; set; } + public sealed record AuditEntry + { + public string UserId { get; init; } + public string Operation { get; init; } + public bool Success { get; init; } + public DateTime Timestamp { get; init; } + public string IpAddress { get; init; } + public object Details { get; init; } + public long DurationMs { get; init; } + + [JsonIgnore] + public string AccessToken { get; init; }
| try | ||
| { | ||
| // Convert to JToken for easier manipulation | ||
| var json = JsonConvert.SerializeObject(data); | ||
| var jToken = JToken.Parse(json); | ||
|
|
||
| SanitizeJToken(jToken); | ||
|
|
||
| // Convert back to object | ||
| return JsonConvert.DeserializeObject(jToken.ToString()); | ||
| } | ||
| catch | ||
| { | ||
| // If sanitization fails, return a safe placeholder | ||
| return new { sanitized = true, error = "Unable to sanitize data" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bare catch silently swallows sanitization failures — risky for a security component.
If SanitizeSensitiveData fails (e.g., due to a circular reference or unexpected type), the error is silently eaten and replaced with a generic placeholder. Since this is the mechanism protecting against sensitive data leakage, failures here should be observable. Because the method is static, it can't access _logger — consider making it an instance method, or narrowing the catch to JsonException and re-throwing unexpected errors so they surface in LogOperationAsync's outer catch.
Proposed fix (instance method with logging)
- private static object SanitizeSensitiveData(object data)
+ private object SanitizeSensitiveData(object data)
{
if (data == null)
return null;
try
{
var json = JsonConvert.SerializeObject(data);
var jToken = JToken.Parse(json);
SanitizeJToken(jToken);
return JsonConvert.DeserializeObject(jToken.ToString());
}
- catch
+ catch (Exception ex)
{
+ _logger.LogWarning(ex, "Failed to sanitize audit data — returning safe placeholder");
return new { sanitized = true, error = "Unable to sanitize data" };
}
}🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs` around lines 112 - 128,
The current bare catch in SanitizeSensitiveData silently swallows all errors;
change SanitizeSensitiveData from static to an instance method so it can use the
class _logger, and replace the broad catch with targeted handling: catch
JsonException (or JsonSerializationException) and return the safe placeholder,
but for any other exceptions log the full exception via _logger (including
message/stack) and rethrow so outer LogOperationAsync can observe/fail; ensure
SanitizeJToken usage remains unchanged and update callers to use the instance
method.
| private async Task<JsonRpcResponse> HandleRequestAsync(JsonRpcRequest request, CancellationToken cancellationToken) | ||
| { | ||
| var response = new JsonRpcResponse | ||
| { | ||
| Jsonrpc = "2.0", | ||
| Id = request.Id | ||
| }; | ||
|
|
||
| try | ||
| { | ||
| switch (request.Method) | ||
| { | ||
| case "initialize": | ||
| response.Result = new | ||
| { | ||
| protocolVersion = "2024-11-05", | ||
| capabilities = new | ||
| { | ||
| tools = new { } | ||
| }, | ||
| serverInfo = new | ||
| { | ||
| name = _serverName, | ||
| version = _serverVersion | ||
| } | ||
| }; | ||
| break; | ||
|
|
||
| case "tools/list": | ||
| var toolsList = new List<object>(); | ||
| foreach (var tool in _tools.Values) | ||
| { | ||
| toolsList.Add(new | ||
| { | ||
| name = tool.Name, | ||
| description = tool.Description, | ||
| inputSchema = tool.InputSchema | ||
| }); | ||
| } | ||
| response.Result = new { tools = toolsList }; | ||
| break; | ||
|
|
||
| case "tools/call": | ||
| var toolCallParams = JsonSerializer.Deserialize<ToolCallParams>( | ||
| JsonSerializer.Serialize(request.Params)); | ||
|
|
||
| if (!_tools.TryGetValue(toolCallParams.Name, out var toolDef)) | ||
| { | ||
| throw new Exception($"Tool not found: {toolCallParams.Name}"); | ||
| } | ||
|
|
||
| var result = await toolDef.Handler(toolCallParams.Arguments); | ||
| response.Result = new | ||
| { | ||
| content = new[] | ||
| { | ||
| new | ||
| { | ||
| type = "text", | ||
| text = JsonSerializer.Serialize(result) | ||
| } | ||
| } | ||
| }; | ||
| break; | ||
|
|
||
| case "ping": | ||
| response.Result = new { }; | ||
| break; | ||
|
|
||
| default: | ||
| response.Error = new JsonRpcError | ||
| { | ||
| Code = -32601, | ||
| Message = "Method not found", | ||
| Data = request.Method | ||
| }; | ||
| break; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger?.LogError(ex, "Error handling method {Method}", request.Method); | ||
| response.Error = new JsonRpcError | ||
| { | ||
| Code = -32603, | ||
| Message = "Internal error", | ||
| Data = null | ||
| }; | ||
| } | ||
|
|
||
| return response; | ||
| } |
There was a problem hiding this comment.
JSON-RPC 2.0 spec violation: responses include both result and error fields.
Per the JSON-RPC 2.0 specification, a response object must contain either result or error, but not both. The current JsonRpcResponse class always serializes both properties (one as null). System.Text.Json emits null values by default, so every response will contain "result": null or "error": null, which may break spec-compliant clients.
🐛 Proposed fix — conditionally ignore null members
private sealed class JsonRpcResponse
{
[JsonPropertyName("jsonrpc")]
public string Jsonrpc { get; set; }
[JsonPropertyName("id")]
public object Id { get; set; }
[JsonPropertyName("result")]
+ [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public object Result { get; set; }
[JsonPropertyName("error")]
+ [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public JsonRpcError Error { get; set; }
}🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs` around lines 91 - 182,
The JsonRpcResponse currently serializes both Result and Error (one as null)
which violates JSON-RPC 2.0; update the JsonRpcResponse type so null members are
omitted when serializing (e.g., add the System.Text.Json.Serialization attribute
to the Result and Error properties: [JsonIgnore(Condition =
JsonIgnoreCondition.WhenWritingNull)]), or alternatively ensure
HandleRequestAsync uses JsonSerializer with options that set
IgnoreNullValues/DefaultIgnoreCondition = WhenWritingNull when writing
responses; modify the JsonRpcResponse class (and import
System.Text.Json.Serialization) or change the serialization call to use these
options so responses include only result OR error.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/ApiClient.cs`:
- Around line 70-71: Remove logging of the auth error response body and only log
the HTTP status; specifically, stop calling response.Content.ReadAsStringAsync
into errorContent and change the _logger.LogWarning call in the
authentication-failure branch (the code that currently calls
response.Content.ReadAsStringAsync and then _logger.LogWarning("Authentication
failed: {StatusCode} - {Error}", response.StatusCode, errorContent)) to log only
the status code (e.g., _logger.LogWarning("Authentication failed: {StatusCode}",
response.StatusCode)), following the same pattern used for the other
deserialization-failure fixes.
In `@Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs`:
- Around line 40-44: The AuditLogger class currently injects an unused
IApiClient via the AuditLogger constructor (parameter IApiClient apiClient) and
stores it in the _apiClient field; remove the IApiClient constructor parameter
and the corresponding private field (e.g., _apiClient) and any commented
references to it so the constructor only takes ILogger<AuditLogger> and no
unused DI is required; update the AuditLogger(ILogger<AuditLogger> logger,
IApiClient apiClient) signature to AuditLogger(ILogger<AuditLogger> logger) and
remove the unused _apiClient member and its assignments.
In `@Web/Resgrid.Web.Mcp/Tools/ShiftsToolProvider.cs`:
- Around line 100-126: The get_shift_details handler currently forwards a
default ShiftId (0) to the API when deserialization fails or no id is provided;
update the anonymous async handler in ShiftsToolProvider.cs to validate
args.ShiftId > 0 (similar to signup_for_shift) after checking AccessToken and
before calling _apiClient.GetAsync, and return CreateErrorResponse("ShiftId is
required") or a similar validation error when the id is missing/<=0; reference
the ShiftIdArgs type and the args.ShiftId usage so the guard is added right
before the _logger.LogInformation/_apiClient.GetAsync call.
🧹 Nitpick comments (5)
Web/Resgrid.Web.Mcp/ApiClient.cs (1)
91-119: Null deserialization check afterEnsureSuccessStatusCodethrows inside the sametry— exception is caught and re-thrown with duplicate logging.If the response returns a non-success status code,
EnsureSuccessStatusCode()throwsHttpRequestException, which is caught at Line 114 and logged. If deserialization returnsnull, theInvalidOperationExceptionthrown at Line 109 is also caught at Line 114 and logged. Both paths produce the same generic log. This is fine functionally but worth noting the duplicate log noise. Same pattern applies toPostAsyncandPutAsync.Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs (2)
150-152: Double serialization to extractToolCallParamsfromParams.
request.Paramsis already a deserializedobject(aJsonElement). Serializing it back to a string just to deserialize intoToolCallParamsis wasteful. UseJsonElementdirectly:♻️ Proposed fix
- var toolCallParams = JsonSerializer.Deserialize<ToolCallParams>( - JsonSerializer.Serialize(request.Params)); + var paramsElement = (JsonElement)request.Params; + var toolCallParams = JsonSerializer.Deserialize<ToolCallParams>(paramsElement.GetRawText());
108-204:HandleRequestAsynccan set bothResultandErroron the same response if an exception occurs afterResultis assigned.If a tool handler (Line 165) succeeds and sets
response.Result(Lines 166-176), but then an unrelated error occurs beforereturn, thecatchat Line 193 will also setresponse.Erroron the same object. While unlikely given the current flow, this is fragile. Consider initializingresponse.Result = nullin the catch block, or returning a fresh error response.Web/Resgrid.Web.Mcp/Tools/AuthenticationToolProvider.cs (2)
33-50: Schema built inline instead of usingSchemaBuilderlike other tool providers.Other providers (e.g.,
ShiftsToolProvider,CalendarToolProvider) useSchemaBuilder.BuildObjectSchemafor consistency. This provider constructs the schema dictionary manually. Consider aligning with the shared pattern for uniformity.♻️ Proposed fix
- new Dictionary<string, object> - { - ["type"] = "object", - ["properties"] = new Dictionary<string, object> - { - ["username"] = new Dictionary<string, object> - { - ["type"] = "string", - ["description"] = "The username or email address of the user" - }, - ["password"] = new Dictionary<string, object> - { - ["type"] = "string", - ["description"] = "The user's password" - } - }, - ["required"] = new[] { "username", "password" } - }, + SchemaBuilder.BuildObjectSchema( + new Dictionary<string, SchemaBuilder.PropertySchema> + { + ["username"] = new SchemaBuilder.PropertySchema { Type = "string", Description = "The username or email address of the user" }, + ["password"] = new SchemaBuilder.PropertySchema { Type = "string", Description = "The user's password" } + }, + new[] { "username", "password" } + ),
124-126: HMAC key is hardcoded in source.While this key is used for log-correlation hashing (not cryptographic security), hardcoding it means it can't be rotated without redeployment, and it's visible in source control. Consider loading it from configuration.
| var errorContent = await response.Content.ReadAsStringAsync(cancellationToken); | ||
| _logger.LogWarning("Authentication failed: {StatusCode} - {Error}", response.StatusCode, errorContent); |
There was a problem hiding this comment.
Error response body logged on authentication failure may contain sensitive data.
The auth server's error response body is logged at Warning level. Similar to the deserialization-failure logging that was already fixed (Lines 108, 142, 176), this response may contain internal details or tokens. Log only the status code.
🔒 Proposed fix
- var errorContent = await response.Content.ReadAsStringAsync(cancellationToken);
- _logger.LogWarning("Authentication failed: {StatusCode} - {Error}", response.StatusCode, errorContent);
+ _logger.LogWarning("Authentication failed with status code: {StatusCode}", response.StatusCode);📝 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.
| var errorContent = await response.Content.ReadAsStringAsync(cancellationToken); | |
| _logger.LogWarning("Authentication failed: {StatusCode} - {Error}", response.StatusCode, errorContent); | |
| _logger.LogWarning("Authentication failed with status code: {StatusCode}", response.StatusCode); |
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/ApiClient.cs` around lines 70 - 71, Remove logging of the
auth error response body and only log the HTTP status; specifically, stop
calling response.Content.ReadAsStringAsync into errorContent and change the
_logger.LogWarning call in the authentication-failure branch (the code that
currently calls response.Content.ReadAsStringAsync and then
_logger.LogWarning("Authentication failed: {StatusCode} - {Error}",
response.StatusCode, errorContent)) to log only the status code (e.g.,
_logger.LogWarning("Authentication failed: {StatusCode}", response.StatusCode)),
following the same pattern used for the other deserialization-failure fixes.
| public AuditLogger(ILogger<AuditLogger> logger, IApiClient apiClient) | ||
| { | ||
| _logger = logger; | ||
| _apiClient = apiClient; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
IApiClient is injected but never used — remove the unused dependency.
_apiClient is only referenced in a commented-out line (Line 60). Keeping it in the constructor forces unnecessary coupling and DI wiring. Remove it until it's actually needed.
♻️ Proposed fix
- public AuditLogger(ILogger<AuditLogger> logger, IApiClient apiClient)
+ public AuditLogger(ILogger<AuditLogger> logger)
{
_logger = logger;
- _apiClient = apiClient;
}Also remove the field:
private readonly ILogger<AuditLogger> _logger;
- private readonly IApiClient _apiClient;🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.cs` around lines 40 - 44, The
AuditLogger class currently injects an unused IApiClient via the AuditLogger
constructor (parameter IApiClient apiClient) and stores it in the _apiClient
field; remove the IApiClient constructor parameter and the corresponding private
field (e.g., _apiClient) and any commented references to it so the constructor
only takes ILogger<AuditLogger> and no unused DI is required; update the
AuditLogger(ILogger<AuditLogger> logger, IApiClient apiClient) signature to
AuditLogger(ILogger<AuditLogger> logger) and remove the unused _apiClient member
and its assignments.
| async (arguments) => | ||
| { | ||
| try | ||
| { | ||
| var args = JsonConvert.DeserializeObject<ShiftIdArgs>(arguments.ToString()); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(args?.AccessToken)) | ||
| { | ||
| return CreateErrorResponse("Access token is required"); | ||
| } | ||
|
|
||
| _logger.LogInformation("Retrieving shift details for {ShiftId}", args.ShiftId); | ||
|
|
||
| var result = await _apiClient.GetAsync<object>( | ||
| $"/api/v4/Shifts/GetShift?shiftId={args.ShiftId}", | ||
| args.AccessToken | ||
| ); | ||
|
|
||
| return new { success = true, data = result }; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error retrieving shift details"); | ||
| return CreateErrorResponse("Failed to retrieve shift details. Please try again later."); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
get_shift_details lacks ShiftId validation — can send shiftId=0 to the API.
Unlike signup_for_shift which validates ShiftId > 0, this handler passes the default int value (0) straight into the query string when deserialization doesn't populate it. Add a similar guard.
🛡️ Proposed fix
if (string.IsNullOrWhiteSpace(args?.AccessToken))
{
return CreateErrorResponse("Access token is required");
}
+ if (args.ShiftId <= 0)
+ {
+ return CreateErrorResponse("Valid shift ID is required");
+ }
+
_logger.LogInformation("Retrieving shift details for {ShiftId}", args.ShiftId);🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Tools/ShiftsToolProvider.cs` around lines 100 - 126, The
get_shift_details handler currently forwards a default ShiftId (0) to the API
when deserialization fails or no id is provided; update the anonymous async
handler in ShiftsToolProvider.cs to validate args.ShiftId > 0 (similar to
signup_for_shift) after checking AccessToken and before calling
_apiClient.GetAsync, and return CreateErrorResponse("ShiftId is required") or a
similar validation error when the id is missing/<=0; reference the ShiftIdArgs
type and the args.ShiftId usage so the guard is added right before the
_logger.LogInformation/_apiClient.GetAsync call.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation