fix: resolve all CI failures on develop branch (build, format, infra-validation)#34
Merged
DeeDee1103 merged 9 commits intomainfrom Mar 21, 2026
Merged
Conversation
…r and service tests - Implemented ScanJobsControllerTests to validate controller actions and responses. - Created ScanJobsServiceTests to ensure service methods function correctly with in-memory database. - Added tests for path safety in PathSafetyTests to verify path validation logic. - Developed ScannerControllerTests to test execution and retrieval of scanner executions. - Introduced ApiKeyAuthenticationHandlerTests to validate API key authentication behavior. - Added project files for new test projects with necessary dependencies.
…ccompanied by documentation updates
- Introduced SECURITY-PATTERNS-QUICK-REFERENCE.md for quick copy-paste security code patterns. - Implemented EmailValidation class for RFC 5322 compliant email validation. - Created FileSecurityValidation class to validate file existence and size before processing. - Added HttpClientConfigurationExtensions for secure HTTP client configuration. - Developed KeyVaultIntegration class for managing secrets from Azure Key Vault. - Implemented RateLimitingMiddleware to prevent API abuse by limiting requests per IP address. - Added RateLimitOptions for customizable rate limiting configuration.
- Added a new document outlining the API versioning strategy for StorageLens, including versioning model, breaking changes, change categories, deprecation policy, implementation rules, and documentation requirements. feat: Implement upstream resilience options - Created `UpstreamResilienceOptions` and `UpstreamClientResilienceOptions` classes to manage resilience settings for upstream clients, including retry attempts, timeouts, and circuit breaker configurations. feat: Add upstream resilience profile resolver - Introduced `UpstreamResilienceProfileResolver` to resolve resilience profiles for clients based on global options and client-specific overrides. feat: Define workflow step state contract - Added `WorkflowStepStateContract` and `WorkflowStepStatus` enum to manage workflow step statuses and transitions, ensuring valid state transitions. test: Add unit tests for DuplicatesService - Implemented comprehensive tests for `DuplicatesService`, covering summary retrieval, recalculation of duplicate groups, and pagination handling in file inventory. test: Create tests for upstream resilience options - Developed tests for `UpstreamResilienceProfileResolver` to validate the resolution of client-specific overrides and clamping of invalid values. test: Implement integration tests for HTTP client policies - Added integration tests to verify the application of retry policies for safe GET requests and the absence of retries for POST requests. test: Create architecture contract tests - Added tests to ensure the correct behavior of workflow resilience options and validation of workflow step transitions.
- Introduced runtime-web-env-post.json to define the environment for the StorageLens web application. - Created runtime-web-env-recreated.txt and runtime-web-env.txt for environment variable management. - Added runtime-web-final.log and runtime-web-final2.log to capture database initialization warnings and connection issues. - Implemented runtime-web-state.log and runtime-web-state.txt to log ASP.NET Core Data Protection warnings. - Added video metadata and extraction scripts, including extract_frames.py for frame extraction from videos. - Generated extraction summary files and logs for processed videos in the extracted-frames directory.
… middleware - Added BackgroundDatabaseInitialization class for async database setup on application start. - Introduced CorrelationIdDelegatingHandler to propagate correlation IDs in outbound HTTP calls. - Created CorrelationIdMiddleware to manage correlation IDs for incoming requests. - Developed DistributedRateLimiter for rate limiting across service replicas using IDistributedCache. - Implemented GlobalExceptionHandler for centralized error handling with consistent problem details. - Added HealthCheckExtensions for standardized health check endpoints. - Integrated ObservabilityExtensions for Serilog logging and OpenTelemetry tracing. - Created request validators for various DTOs to enforce input validation. - Established integration tests for Locations service with in-memory database setup.
…ace, infra-validation)
Copilot created this pull request from a session on behalf of
DeeDee1103
March 21, 2026 17:37
View session
There was a problem hiding this comment.
Pull request overview
This PR aims to restore CI green on develop by addressing build/format failures and adjusting infra validation, while also introducing shared infrastructure for observability, resilience, authentication/authorization, health checks, and several operational/docs updates.
Changes:
- Fix compilation/formatting issues and update infra-validation to build Bicep templates natively.
- Add shared infrastructure for observability (Serilog + OpenTelemetry), health checks, API key auth, correlation IDs, upstream HTTP client resilience, and rate limiting primitives.
- Apply service-wide wiring updates (middleware/auth/rate limiting/health checks) plus broader documentation and repository governance additions (workflows, CODEOWNERS, issue templates, docs).
Reviewed changes
Copilot reviewed 146 out of 199 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src/StorageLens.Web/appsettings.json | Adds upstream resilience settings and removes hardcoded alert email. |
| src/StorageLens.Web/Program.cs | Wires observability, rate limiting, upstream client config, and container DB behavior. |
| src/StorageLens.Web/Pages/Shared/_MarketingLayout.cshtml | Updates favicon/logo markup. |
| src/StorageLens.Web/Pages/Shared/_Layout.cshtml | Updates favicon/logo markup. |
| src/StorageLens.Web/Pages/Alerts.cshtml.cs | Hardens email validation and adds config-write hashing/audit logging. |
| src/StorageLens.Web/Pages/About.cshtml | Replaces image logo with CSS/HTML logo mark. |
| src/StorageLens.Web/Clients/ScanJobsApiClient.cs | Expands “service unavailable” handling to circuit-breaker/timeouts. |
| src/StorageLens.Web/Clients/OrchestratorApiClient.cs | Expands “service unavailable” handling to circuit-breaker/timeouts. |
| src/StorageLens.Web/Clients/LocationsApiClient.cs | Expands “service unavailable” handling to circuit-breaker/timeouts. |
| src/StorageLens.Web/Clients/FileInventoryApiClient.cs | Expands “service unavailable” handling to circuit-breaker/timeouts. |
| src/StorageLens.Web/Clients/DuplicatesApiClient.cs | Expands “service unavailable” handling to circuit-breaker/timeouts. |
| src/StorageLens.Web/Clients/AnalyticsApiClient.cs | Expands “service unavailable” handling to circuit-breaker/timeouts. |
| src/StorageLens.Shared.Infrastructure/Validators/RequestValidators.cs | Adds FluentValidation validators for shared request contracts. |
| src/StorageLens.Shared.Infrastructure/UpstreamResilienceOptions.cs | Adds global + per-client resilience option model and resolver. |
| src/StorageLens.Shared.Infrastructure/StorageLens.Shared.Infrastructure.csproj | Adds framework reference and many infra/otel/polly/validation packages. |
| src/StorageLens.Shared.Infrastructure/StartupDatabaseInitializer.cs | Removes reflection calls; uses EF relational creator APIs when available. |
| src/StorageLens.Shared.Infrastructure/RateLimitingMiddleware.cs | Adds custom in-memory rate limiting middleware + DI extensions. |
| src/StorageLens.Shared.Infrastructure/ObservabilityExtensions.cs | Adds Serilog + OpenTelemetry + validators + global exception handling wiring. |
| src/StorageLens.Shared.Infrastructure/KeyVaultIntegration.cs | Adds optional Azure Key Vault configuration and secret retrieval helpers. |
| src/StorageLens.Shared.Infrastructure/HttpClientConfigurationExtensions.cs | Adds shared HttpClient resilience configuration using Polly. |
| src/StorageLens.Shared.Infrastructure/HealthCheckExtensions.cs | Adds standardized liveness/readiness endpoints and JSON response writer. |
| src/StorageLens.Shared.Infrastructure/GlobalExceptionHandler.cs | Adds centralized RFC7807 problem details exception handler. |
| src/StorageLens.Shared.Infrastructure/FileSecurityValidation.cs | Adds pre-stream file validation helpers to reduce file-based DoS risk. |
| src/StorageLens.Shared.Infrastructure/EmailValidation.cs | Adds stricter email validation utilities. |
| src/StorageLens.Shared.Infrastructure/DistributedRateLimiter.cs | Adds distributed rate limiting implementation based on IDistributedCache. |
| src/StorageLens.Shared.Infrastructure/CorrelationIdMiddleware.cs | Adds correlation-id middleware and extension method. |
| src/StorageLens.Shared.Infrastructure/CorrelationIdDelegatingHandler.cs | Propagates correlation IDs on outbound HttpClient calls. |
| src/StorageLens.Shared.Infrastructure/BackgroundDatabaseInitialization.cs | Moves DB init/seeding to background on app start. |
| src/StorageLens.Shared.Infrastructure/AuditLogger.cs | Adds structured audit logger abstraction for security-relevant events. |
| src/StorageLens.Shared.Infrastructure/ApiKeyAuthenticationHandler.cs | Adds API key authentication scheme/handler. |
| src/StorageLens.Services.Scanner/appsettings.json | Adds upstream resilience settings and per-client overrides. |
| src/StorageLens.Services.Scanner/Services/PathSafety.cs | Adds path normalization and reparse-point checks to reduce traversal risk. |
| src/StorageLens.Services.Scanner/Program.cs | Adds observability, rate limiting, API key authZ, health checks, background DB init. |
| src/StorageLens.Services.Scanner/Controllers/ScannerController.cs | Enforces authorization via [Authorize]. |
| src/StorageLens.Services.ScanJobs/Services/ScanJobsService.cs | Improves DTO projection and uses ExecuteUpdate for relational DBs. |
| src/StorageLens.Services.ScanJobs/Program.cs | Adds observability, rate limiting, API key authZ, health checks, background DB init. |
| src/StorageLens.Services.ScanJobs/Extensions/ScanJobsSeeder.cs | Adds CancellationToken support and passes it to SaveChangesAsync. |
| src/StorageLens.Services.ScanJobs/Controllers/ScanJobsController.cs | Enforces authorization via [Authorize]. |
| src/StorageLens.Services.Orchestrator/appsettings.json | Adds upstream resilience settings with long timeouts for specific clients. |
| src/StorageLens.Services.Orchestrator/StorageLens.Services.Orchestrator.csproj | Adds Polly integration package. |
| src/StorageLens.Services.Orchestrator/Services/WorkflowOrchestratorService.cs | Adds workflow/step status transition enforcement and uses enums consistently. |
| src/StorageLens.Services.Orchestrator/Program.cs | Adds observability, rate limiting, API key authZ, health checks, and resilient upstream clients. |
| src/StorageLens.Services.Orchestrator/Models/WorkflowStepStateContract.cs | Adds step-state contract and allowed transitions. |
| src/StorageLens.Services.Orchestrator/Controllers/OrchestratorController.cs | Enforces authorization via [Authorize]. |
| src/StorageLens.Services.Orchestrator/Clients/UpstreamResilienceProfileResolver.cs | Introduces orchestrator-local resilience profile resolver. |
| src/StorageLens.Services.Orchestrator/Clients/UpstreamResilienceOptions.cs | Introduces orchestrator-local resilience options model. |
| src/StorageLens.Services.Locations/Services/LocationService.cs | Improves DTO projection and uses ExecuteUpdate/Delete for relational DBs. |
| src/StorageLens.Services.Locations/Program.cs | Adds observability, rate limiting, API key authZ, health checks, background DB init, and testable Program. |
| src/StorageLens.Services.Locations/Extensions/LocationsSeeder.cs | Adds CancellationToken support and passes it to SaveChangesAsync. |
| src/StorageLens.Services.Locations/Controllers/LocationsController.cs | Enforces auth, removes manual validation in favor of FluentValidation. |
| src/StorageLens.Services.Hashing/appsettings.json | Adds upstream resilience settings and per-client overrides. |
| src/StorageLens.Services.Hashing/Services/HashingService.cs | Uses shared file validation helper before hashing streams. |
| src/StorageLens.Services.Hashing/Program.cs | Adds observability, rate limiting, API key authZ, health checks, background DB init. |
| src/StorageLens.Services.Hashing/Controllers/HashingController.cs | Enforces authorization via [Authorize]. |
| src/StorageLens.Services.FileInventory/Services/FileInventoryService.cs | Improves DTO projection, chunks path queries, adds relational fast-path updates. |
| src/StorageLens.Services.FileInventory/Program.cs | Adds observability, rate limiting, API key authZ, health checks, background DB init. |
| src/StorageLens.Services.FileInventory/Extensions/FileInventorySeeder.cs | Adds CancellationToken support and passes it to SaveChangesAsync. |
| src/StorageLens.Services.FileInventory/Controllers/FilesController.cs | Enforces authorization via [Authorize]. |
| src/StorageLens.Services.Duplicates/appsettings.json | Adds upstream resilience settings. |
| src/StorageLens.Services.Duplicates/Program.cs | Adds observability, rate limiting, API key authZ, health checks, background DB init. |
| src/StorageLens.Services.Duplicates/Extensions/DuplicatesSeeder.cs | Adds CancellationToken support and passes it to SaveChangesAsync. |
| src/StorageLens.Services.Duplicates/Controllers/DuplicatesController.cs | Enforces authorization via [Authorize]. |
| src/StorageLens.Services.Analytics/appsettings.json | Adds upstream resilience settings. |
| src/StorageLens.Services.Analytics/Program.cs | Adds observability, rate limiting, API key authZ, health checks. |
| src/StorageLens.Services.Analytics/Controllers/AnalyticsController.cs | Enforces authorization via [Authorize]. |
| runtime-web-env.txt | Adds captured runtime env output (diagnostic artifact). |
| runtime-web-env-recreated.txt | Adds recreated runtime env output (diagnostic artifact). |
| runtime-port-web.txt | Adds port mapping capture (diagnostic artifact). |
| runtime-net-5001.txt | Adds connectivity capture (diagnostic artifact). |
| runtime-http.json | Adds HTTP response capture (diagnostic artifact). |
| runtime-curl-success.txt | Adds curl output (diagnostic artifact). |
| runtime-curl-retry.txt | Adds curl output (diagnostic artifact). |
| runtime-curl-logo-check.txt | Adds curl output (diagnostic artifact). |
| runtime-curl-final2.txt | Adds curl output (diagnostic artifact). |
| runtime-curl-final.txt | Adds curl output (diagnostic artifact). |
| runtime-curl-cmd-err.txt | Adds curl verbose output (diagnostic artifact). |
| runtime-curl-after-fix.txt | Adds curl output (diagnostic artifact). |
| infra/main.bicep | Adds App Insights resource + connection string wiring. |
| frame-tool-check-python.txt | Adds diagnostic artifact. |
| frame-tool-check-ffmpeg.txt | Adds diagnostic artifact. |
| docs/technical/api-versioning-strategy.md | Adds API versioning policy doc. |
| docs/reviews/review-refresh-2026-03-20.md | Adds review refresh documentation. |
| docs/operations/sli-slo-targets.md | Adds reliability targets documentation. |
| docs/operations/secrets-rotation-runbook.md | Adds secrets rotation runbook. |
| docs/operations/project-health-dashboard.md | Adds project health dashboard doc. |
| docs/operations/monitoring-logging.md | Updates monitoring guide with SLI/SLO governance references. |
| docs/operations/incident-response-runbook.md | Adds incident response runbook. |
| docs/operations/deployment-guide.md | Adds observability provisioning checklist item. |
| docs/operations/ci-cd-pipeline.md | Adds CI/CD pipeline documentation. |
| docs/developer/onboarding-checklist.md | Adds onboarding checklist. |
| docs/developer/issue-management-and-release-process.md | Adds issue/release process doc. |
| docs/developer/docs-ownership-matrix.md | Adds docs ownership + cadence matrix. |
| docs/developer/developer-guide.md | Adds Copilot/quality gate guidance and expanded governance references. |
| docs/developer/contributing.md | Updates PR checklist with CI/docs governance and Copilot references. |
| docs/developer/architecture-review-process.md | Adds architecture review process. |
| docs/developer/SECURITY-PATTERNS-QUICK-REFERENCE.md | Adds security patterns reference doc. |
| docs/architecture/system-overview.md | Adds observability architecture description. |
| docs/architecture/adr/0000-template.md | Adds ADR template. |
| docs/README.md | Updates docs index with new docs and “Recent Updates” section. |
| docker-compose.yml | Adds Jaeger + Redis and wires env vars/dependencies across services. |
| copilot-instructions.md | Adds project-specific Copilot guidance. |
| StorageLens.sln | Adds new test projects to solution. |
| SKILL.md | Adds Copilot skill definition file. |
| CHANGELOG.md | Adds extensive unreleased notes for security/infrastructure updates. |
| AGENTS.md | Adds specialized agent definitions and invocation patterns. |
| .prompt.md | Adds repo-level custom prompt context. |
| .instructions.md | Adds repo-level Copilot instructions. |
| .github/workflows/tests-and-coverage.yml | Adds test + coverage gating workflow. |
| .github/workflows/infra-validation.yml | Adds infra validation workflow for Bicep compilation. |
| .github/workflows/docs-guard.yml | Enhances docs guard rules and messages. |
| .github/workflows/code-quality.yml | Adds formatting + vulnerability + build CI workflow. |
| .github/dependabot.yml | Adds Dependabot automation for NuGet and GitHub Actions. |
| .github/ISSUE_TEMPLATE/feature_request.yml | Adds feature request template. |
| .github/ISSUE_TEMPLATE/config.yml | Adds issue template config + security contact link. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Adds bug report template. |
| .github/CODEOWNERS | Adds code ownership rules (infra/workflows/shared infra). |
| .env.example | Updates env example to include service API key and guidance. |
| .claude/settings.local.json | Adds local tool configuration (permissions list). |
| .agent.md | Adds custom agent definitions (overlaps with AGENTS.md). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| builder.Services.AddScoped<IAlertEmailService, SmtpAlertEmailService>(); | ||
|
|
||
| builder.Services.AddScoped<ITierAccessService, TierAccessService>(); | ||
|
|
||
| var dbCfg = builder.Configuration.GetSection(DatabaseConfigOptions.SectionName).Get<DatabaseConfigOptions>() | ||
| ?? new DatabaseConfigOptions(); | ||
|
|
||
| if (!dbCfg.UseInMemory && string.Equals(Environment.GetEnvironmentVariable("DOTNET_RUNNING_IN_CONTAINER"), "true", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
This forces UseInMemory=true for any container runtime, which will also apply in production container environments and can silently disable the intended real database configuration. Consider narrowing the condition to only the LocalDb provider (or another explicit 'local-container' flag), e.g. only flip to in-memory when dbCfg.Provider == \"LocalDb\" (or connection string is missing/LocalDb) and running in container.
Suggested change
| if (!dbCfg.UseInMemory && string.Equals(Environment.GetEnvironmentVariable("DOTNET_RUNNING_IN_CONTAINER"), "true", StringComparison.OrdinalIgnoreCase)) | |
| if (!dbCfg.UseInMemory | |
| && string.Equals(Environment.GetEnvironmentVariable("DOTNET_RUNNING_IN_CONTAINER"), "true", StringComparison.OrdinalIgnoreCase) | |
| && string.Equals(dbCfg.Provider, "LocalDb", StringComparison.OrdinalIgnoreCase)) |
Comment on lines
+101
to
+121
| protected override async ValueTask<RateLimitLease> AcquireAsyncCore( | ||
| int permitCount, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| try | ||
| { | ||
| var current = await IncrementAsync(cancellationToken); | ||
| if (current <= _limit) | ||
| return new GrantedLease(); | ||
|
|
||
| _logger.LogWarning("Rate limit exceeded for key {CacheKey}", _cacheKey); | ||
| return new DeniedLease(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // On cache failure (Redis unavailable) fall back to allow — prefer availability | ||
| // over strict rate limiting to avoid a cascading outage. | ||
| _logger.LogError(ex, "Distributed rate limiter cache error for {CacheKey} — allowing request", _cacheKey); | ||
| return new GrantedLease(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The distributed limiter increment is not atomic (GetAsync + SetAsync), so concurrent requests can overwrite each other and undercount, making enforcement unreliable across replicas. It also ignores permitCount (always increments by 1) which can break correctness if the limiter requests multiple permits. Consider using an atomic increment in the backing store (e.g., Redis INCR + key expiry via StackExchange.Redis), and apply permitCount to the increment logic.
Comment on lines
+126
to
+139
| private async Task<long> IncrementAsync(CancellationToken ct) | ||
| { | ||
| var data = await _cache.GetAsync(_cacheKey, ct); | ||
| long count = data is null ? 0 : BitConverter.ToInt64(data); | ||
|
|
||
| count++; | ||
|
|
||
| var options = new DistributedCacheEntryOptions | ||
| { | ||
| AbsoluteExpirationRelativeToNow = _window | ||
| }; | ||
| await _cache.SetAsync(_cacheKey, BitConverter.GetBytes(count), options, ct); | ||
| return count; | ||
| } |
There was a problem hiding this comment.
The distributed limiter increment is not atomic (GetAsync + SetAsync), so concurrent requests can overwrite each other and undercount, making enforcement unreliable across replicas. It also ignores permitCount (always increments by 1) which can break correctness if the limiter requests multiple permits. Consider using an atomic increment in the backing store (e.g., Redis INCR + key expiry via StackExchange.Redis), and apply permitCount to the increment logic.
Comment on lines
+124
to
+125
| => new DeniedLease(); // Synchronous path not supported; use async | ||
|
|
There was a problem hiding this comment.
Returning DeniedLease unconditionally for the synchronous acquire path can cause unexpected 429s if any part of the pipeline uses synchronous acquisition (even occasionally). If sync acquisition can occur, either implement it safely (even if limited) or explicitly document/ensure that only async acquisition is used by the configured ASP.NET Core rate limiting policy.
Suggested change
| => new DeniedLease(); // Synchronous path not supported; use async | |
| { | |
| try | |
| { | |
| var current = IncrementAsync(CancellationToken.None).GetAwaiter().GetResult(); | |
| if (current <= _limit) | |
| return new GrantedLease(); | |
| _logger.LogWarning("Rate limit exceeded for key {CacheKey}", _cacheKey); | |
| return new DeniedLease(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // On cache failure (Redis unavailable) fall back to allow — prefer availability | |
| // over strict rate limiting to avoid a cascading outage. | |
| _logger.LogError(ex, "Distributed rate limiter cache error for {CacheKey} — allowing request", _cacheKey); | |
| return new GrantedLease(); | |
| } | |
| } |
Comment on lines
+37
to
+44
| if (string.IsNullOrWhiteSpace(configuredKey)) | ||
| { | ||
| // If no key is configured, allow through (development/in-memory mode) | ||
| var claims = new[] { new Claim(ClaimTypes.Name, "internal-service") }; | ||
| var identity = new ClaimsIdentity(claims, ApiKeyAuthentication.SchemeName); | ||
| var ticket = new AuthenticationTicket(new ClaimsPrincipal(identity), Scheme.Name); | ||
| return Task.FromResult(AuthenticateResult.Success(ticket)); | ||
| } |
There was a problem hiding this comment.
Failing open when ServiceApiKey is missing disables authentication entirely on misconfiguration. For production safety, consider failing closed by default and only allowing the bypass in explicit dev/test scenarios (e.g., IHostEnvironment.IsDevelopment() or a dedicated Auth:AllowAnonymousWhenMissingKey=true setting).
| **When validating email input:** | ||
|
|
||
| ```csharp | ||
| using StorageLens.Shared.Infrastructure.Security; |
There was a problem hiding this comment.
The quick reference uses StorageLens.Shared.Infrastructure.Security namespaces, but the PR adds EmailValidation/FileSecurityValidation under StorageLens.Shared.Infrastructure (no .Security namespace in the diffs). Update the doc examples to match the actual namespaces/types so copy/paste snippets compile.
Suggested change
| using StorageLens.Shared.Infrastructure.Security; | |
| using StorageLens.Shared.Infrastructure; |
| **Before opening any file stream:** | ||
|
|
||
| ```csharp | ||
| using StorageLens.Shared.Infrastructure.Security; |
There was a problem hiding this comment.
The quick reference uses StorageLens.Shared.Infrastructure.Security namespaces, but the PR adds EmailValidation/FileSecurityValidation under StorageLens.Shared.Infrastructure (no .Security namespace in the diffs). Update the doc examples to match the actual namespaces/types so copy/paste snippets compile.
| /// - Key: per-IP (X-Forwarded-For → RemoteIpAddress fallback) | ||
| /// - Rejected requests: 429 + Retry-After header | ||
| /// </summary> | ||
| public sealed class DistributedFixedWindowPolicy : IRateLimiterPolicy<string> |
There was a problem hiding this comment.
This introduces non-trivial rate limiting behavior (partitioning, cache-based counting, retry-after metadata). Add unit/integration tests that cover: correct partition key selection (X-Forwarded-For vs RemoteIp), limit enforcement, retry-after metadata, cache failure fallback behavior, and concurrency behavior (at minimum verifying current limitations or the atomic increment approach if updated).
| /// A <see cref="RateLimiter"/> that uses <see cref="IDistributedCache"/> to | ||
| /// share window counts across multiple service replicas. | ||
| /// </summary> | ||
| internal sealed class DistributedWindowLimiter : RateLimiter |
There was a problem hiding this comment.
This introduces non-trivial rate limiting behavior (partitioning, cache-based counting, retry-after metadata). Add unit/integration tests that cover: correct partition key selection (X-Forwarded-For vs RemoteIp), limit enforcement, retry-after metadata, cache failure fallback behavior, and concurrency behavior (at minimum verifying current limitations or the atomic increment approach if updated).
| /// </summary> | ||
| /// <param name="builder">The WebApplicationBuilder for the current service.</param> | ||
| /// <param name="serviceName">Human-readable service name used in traces and log enrichment.</param> | ||
| public static WebApplicationBuilder AddStorageLensObservability( |
There was a problem hiding this comment.
The PR title/description frames this as a CI-failure fix, but the diff introduces substantial new platform functionality (observability stack, authZ, rate limiting, distributed resilience, new docs/governance/workflows, docker-compose services, and infra resources). Consider splitting into smaller PRs (CI fixes vs. feature/platform additions), or updating the PR description to reflect the expanded scope so reviewers and release notes are accurate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three CI workflows (
code-quality,quality-gates,infra-validation) were consistently failing on every push todevelop. All failures stemmed from newly introduced files in the security/observability refactor that were never compiled or formatted locally before being pushed.Build errors (
quality-gates)HealthCheckExtensions.cs— missingusing Microsoft.AspNetCore.Routing→ CS0246 onIEndpointRouteBuilderDistributedRateLimiter.cs—MetadataName<TimeSpan>used asstringdirectly; fixed with.NamepropertyGlobalExceptionHandler.cs—ArgumentNullExceptionarm afterArgumentExceptionin switch → CS8510 unreachable; reorderedObservabilityExtensions.cs— missingusing Microsoft.Extensions.Hosting→ CS1061 onIsDevelopment()Duplicates,FileInventory,Locations,ScanJobs) —SeedAsyncsignatures didn't acceptCancellationToken, mismatching theFunc<IServiceProvider, TDbContext, CancellationToken, Task>delegate inBackgroundDatabaseInitialization.Start; addedCancellationToken cancellationToken = defaultto all fourLocations/Program.cs—WebApplicationFactory<Program>in integration tests couldn't access the top-levelProgramtype; addedpublic partial class Program {}Whitespace (
code-quality)dotnet format --verify-no-changesfailed on tab-indented code across bothClass1.csfiles and on extra alignment spaces inDistributedRateLimiter.cs/GlobalExceptionHandler.cs. Applieddotnet formatto auto-fix.Infra validation (
infra-validation)azure/cli@v2runsaz bicep buildinside a Docker container —/tmp/main.jsonis written to the container's filesystem and disappears when it exits, sotest -f /tmp/main.jsonon the runner always fails. Replaced with a nativerun:step: