Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughThis refactoring consolidates ASP.NET Core application startup configuration by extracting a dedicated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 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)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Web/Resgrid.Web.Mcp/Program.cs`:
- Line 53: The assignment using Assembly.GetEntryAssembly() and Version should
use null-conditional operators like the change in Program.cs; update the same
pattern wherever GetEntryAssembly()?.GetName().Version?.ToString() is needed —
specifically adjust the Release assignment in the Program classes (symbols:
options.Release in Resgrid.Web/Program.cs, Resgrid.Web.Services/Program.cs,
Resgrid.Web.Eventing/Program.cs, Resgrid.Workers.Console/Program.cs) and the
health/version exposures in HealthController classes (symbols: HealthController
in Resgrid.Web.Controllers, Resgrid.Web.Services.Controllers.v4,
Resgrid.Web.Eventing.Controllers) as well as the logging version read in
Resgrid.Framework.Logging (symbol: Logging class); replace direct
Assembly.GetEntryAssembly().GetName().Version.ToString() calls with
null-conditional variants and handle a null result safely (e.g., assign null or
empty string) to avoid NullReferenceException.
In `@Web/Resgrid.Web.Mcp/Startup.cs`:
- Around line 36-41: TokenRefreshService.GetValidTokenAsync has a TOCTOU race
where concurrent callers both see a missing/expired token and perform duplicate
refreshes; change GetValidTokenAsync to atomically coordinate refreshes (e.g.,
use a ConcurrentDictionary<string, Lazy<Task<Token>>> or
ConcurrentDictionary.AddOrUpdate with a factory that returns a single Task to
perform refresh) so only the first caller triggers the refresh and others await
that Task; ensure you replace the current token-read/refresh branches in
TokenRefreshService.GetValidTokenAsync with the AddOrUpdate/Lazy<Task> pattern,
correctly await the stored Task, and handle/remap exceptions and token storage
on completion.
🧹 Nitpick comments (4)
Web/Resgrid.Web.Mcp/Program.cs (1)
51-74:TracesSampleRateon line 51 is redundant whenTracesSampleris set.When both
TracesSampleRateandTracesSamplerare configured, the Sentry SDK usesTracesSamplerand ignoresTracesSampleRate. Since the sampler already falls back toExternalErrorConfig.SentryPerfSampleRate(line 73), line 51 is effectively dead code. Not a bug, but removing it avoids confusion about which value controls sampling.♻️ Suggested cleanup
options.SendDefaultPii = true; options.AutoSessionTracking = true; - options.TracesSampleRate = ExternalErrorConfig.SentryPerfSampleRate; options.Environment = ExternalErrorConfig.Environment;Web/Resgrid.Web.Mcp/Startup.cs (3)
23-27: Side effect inConfigureServices: logging initialization.
Framework.Logging.Initialize(...)is a one-time global side effect invoked during service registration. While pragmatic for startup, it makes this method harder to test and couples it to the staticLoggingclass. If testability or startup ordering ever becomes a concern, consider moving this toConfigureor into a hosted service'sStartAsync.
14-14:Configurationproperty is injected but unused.
IConfigurationis stored on Line 14 and set in the constructor, butConfigureServicesreads all configuration from static classes (ExternalErrorConfig,SystemBehaviorConfig). If there's no plan to useConfiguration, consider removing it to avoid misleading readers into thinking it drives behavior.
68-79: Tool providers are registered as concrete types without interfaces.All 10 tool providers and
McpToolRegistryare registered as singletons of their concrete type. This works but limits testability and makes it harder to substitute implementations. If these are consumed by other services, consider extracting interfaces (at minimum forMcpToolRegistry) to align with the coding guideline to "prefer composition with interfaces to extend behavior." As per coding guidelines, "Prefer composition with interfaces to extend behavior".
| options.TracesSampleRate = ExternalErrorConfig.SentryPerfSampleRate; | ||
| options.Environment = ExternalErrorConfig.Environment; | ||
| options.Release = Assembly.GetEntryAssembly().GetName().Version.ToString(); | ||
| options.Release = Assembly.GetEntryAssembly()?.GetName().Version?.ToString(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all GetEntryAssembly() usages without null-conditional operators
rg -n 'GetEntryAssembly\(\)\.' --type cs -A2 -B2Repository: Resgrid/Core
Length of output: 3757
🏁 Script executed:
#!/bin/bash
# Check specifically for the Logging.cs file mentioned in the review
fd -e cs -p 'Logging.cs' | head -20Repository: Resgrid/Core
Length of output: 89
Good defensive null-safety improvement, but multiple other locations need the same fix.
Using null-conditional operators on GetEntryAssembly() and Version prevents potential NullReferenceException in hosted environments where GetEntryAssembly() can return null. The change in Program.cs follows C# best practices.
However, the codebase has multiple other unsafe usages of this same pattern:
Web/Resgrid.Web/Program.csline 59Web/Resgrid.Web.Services/Program.csline 58Web/Resgrid.Web.Eventing/Program.csline 57Workers/Resgrid.Workers.Console/Program.csline 103Web/Resgrid.Web/Controllers/HealthController.csline 35Web/Resgrid.Web.Services/Controllers/v4/HealthController.csline 42Web/Resgrid.Web.Eventing/Controllers/HealthController.csline 37Core/Resgrid.Framework/Logging.csline 40
Apply the same null-conditional operator pattern to all these locations for consistency and safety.
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Program.cs` at line 53, The assignment using
Assembly.GetEntryAssembly() and Version should use null-conditional operators
like the change in Program.cs; update the same pattern wherever
GetEntryAssembly()?.GetName().Version?.ToString() is needed — specifically
adjust the Release assignment in the Program classes (symbols: options.Release
in Resgrid.Web/Program.cs, Resgrid.Web.Services/Program.cs,
Resgrid.Web.Eventing/Program.cs, Resgrid.Workers.Console/Program.cs) and the
health/version exposures in HealthController classes (symbols: HealthController
in Resgrid.Web.Controllers, Resgrid.Web.Services.Controllers.v4,
Resgrid.Web.Eventing.Controllers) as well as the logging version read in
Resgrid.Framework.Logging (symbol: Logging class); replace direct
Assembly.GetEntryAssembly().GetName().Version.ToString() calls with
null-conditional variants and handle a null result safely (e.g., assign null or
empty string) to avoid NullReferenceException.
| // Register infrastructure services | ||
| services.AddMemoryCache(); | ||
| services.AddSingleton<IResponseCache, ResponseCache>(); | ||
| services.AddSingleton<IRateLimiter, RateLimiter>(); | ||
| services.AddSingleton<ITokenRefreshService, TokenRefreshService>(); | ||
| services.AddSingleton<IAuditLogger, AuditLogger>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.cs" | xargs rg -l 'class ResponseCache|class RateLimiter|class TokenRefreshService|class AuditLogger' | head -20Repository: Resgrid/Core
Length of output: 273
🏁 Script executed:
# Find and examine these implementations
rg -B 2 -A 20 'class ResponseCache\s*[:\(]' --type csRepository: Resgrid/Core
Length of output: 1912
🏁 Script executed:
rg -B 2 -A 20 'class RateLimiter\s*[:\(]' --type csRepository: Resgrid/Core
Length of output: 1998
🏁 Script executed:
rg -B 2 -A 20 'class TokenRefreshService\s*[:\(]' --type csRepository: Resgrid/Core
Length of output: 2199
🏁 Script executed:
rg -B 2 -A 20 'class AuditLogger\s*[:\(]' --type csRepository: Resgrid/Core
Length of output: 1756
🏁 Script executed:
cat -n Web/Resgrid.Web.Mcp/Infrastructure/RateLimiter.csRepository: Resgrid/Core
Length of output: 5735
🏁 Script executed:
cat -n Web/Resgrid.Web.Mcp/Infrastructure/ResponseCache.csRepository: Resgrid/Core
Length of output: 2181
🏁 Script executed:
cat -n Web/Resgrid.Web.Mcp/Infrastructure/TokenRefreshService.csRepository: Resgrid/Core
Length of output: 5217
🏁 Script executed:
cat -n Web/Resgrid.Web.Mcp/Infrastructure/AuditLogger.csRepository: Resgrid/Core
Length of output: 6643
RateLimiter and ResponseCache are already properly thread-safe, but TokenRefreshService has a race condition.
RateLimiter uses ConcurrentDictionary with explicit locking in RequestCounter.TryAddIfUnderLimit (lines 138-156); ResponseCache delegates to IMemoryCache, which is thread-safe by design. However, TokenRefreshService.GetValidTokenAsync has a time-of-check-time-of-use race: two concurrent requests can both read an expired/missing token (line 36), both decide to refresh (lines 50-54), and both issue redundant token refresh calls. Use ConcurrentDictionary.AddOrUpdate with a factory method or add async coordination to prevent duplicate refresh attempts.
🤖 Prompt for AI Agents
In `@Web/Resgrid.Web.Mcp/Startup.cs` around lines 36 - 41,
TokenRefreshService.GetValidTokenAsync has a TOCTOU race where concurrent
callers both see a missing/expired token and perform duplicate refreshes; change
GetValidTokenAsync to atomically coordinate refreshes (e.g., use a
ConcurrentDictionary<string, Lazy<Task<Token>>> or
ConcurrentDictionary.AddOrUpdate with a factory that returns a single Task to
perform refresh) so only the first caller triggers the refresh and others await
that Task; ensure you replace the current token-read/refresh branches in
TokenRefreshService.GetValidTokenAsync with the AddOrUpdate/Lazy<Task> pattern,
correctly await the stored Task, and handle/remap exceptions and token storage
on completion.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit