slp-modular-cms/aidlc-docs/_shared/reverse-engineering/code-quality-assessment.md

Code Quality Assessment

All figures below were measured during this analysis (2026-07-27) rather than inferred.

Build

dotnet build SlpModularCms.sln -c Release — succeeds: 0 errors, 50 warnings, ~27s.

Warning categories:

• NU1903 — known high-severity vulnerabilities in transitive packages (the majority of the 50). Confirmed by dotnet list package --vulnerable --include-transitive:
  • Microsoft.OpenApi 2.0.0 — GHSA-v5pm-xwqc-g5wc (High)
  • System.Security.Cryptography.Xml 10.0.9 — GHSA-cvvh-rhrc-wg4q and four further advisories (High) Both arrive transitively (OpenAPI tooling; Data Protection's XML key handling). A CI gate on dotnet list package --vulnerable would fail today until these are pinned to patched versions.
• NU1510 — redundant PackageReferences that will not be pruned: Microsoft.Extensions.Logging.Abstractions (Core, Modules.Identity.Tests), Microsoft.Extensions.Hosting.Abstractions (Core.Tests). Cosmetic.

No C# compiler warnings — nullable reference types are respected throughout.

Test Coverage

Backend — all suites pass

Suite	Tests	Result	Duration
SlpModularCms.Core.Tests	54	✅ all passed	0.9s
SlpModularCms.Modules.Identity.Tests	37	✅ all passed	1.0s
SlpModularCms.Modules.Availability.Tests	78	✅ all passed	0.5s
SlpModularCms.Modules.Master.Tests	50	✅ all passed	0.6s
Total	219	0 failed, 0 skipped	~3s

Frontend — all suites pass

pnpm test (Vitest): 34 test files, 213 tests, all passed, ~39s. Every page, API hook and interactive component has a colocated test; MSW supplies request mocking per domain.

Coverage posture

• Unit tests: Good and genuinely broad — controllers, services, repositories and background services are all covered on the backend; pages, hooks and dialogs on the frontend.
• Integration tests: None. There is no WebApplicationFactory-based suite, so the composed pipeline is never exercised end to end. The things that only exist in composition are therefore untested: middleware ordering, the availability gate's real interaction with static files, the /api/v1 prefix convention, the two SPA fallbacks, CORS, rate limiting, and JWT validation against real configuration.
• Contract tests: None for the master↔slave protocol. Both sides are unit-tested in isolation with mocks, so a change to one side's contract would not be caught.
• End-to-end tests: None.
• Coverage numbers: coverlet.runsettings is configured (excluding migrations, obj/, generated OpenAPI interceptors and [ExcludeFromCodeCoverage] members), and the frontend has a test:coverage script with v8, but no threshold is enforced anywhere — nothing fails a build for dropping coverage.

Code Quality Indicators

• Backend linting: Nothing beyond compiler nullable warnings — no .editorconfig, no analyzer package, no format check. dotnet format --verify-no-changes is not wired up anywhere.
• Frontend linting: ESLint 10 with typescript-eslint, react-hooks and react-refresh plugins, plus Prettier with format:check. pnpm run lint currently FAILS — see Technical Debt below. This is a blocking fact for any CI pipeline that runs lint as a gate.
• Type checking: tsc -b runs as part of pnpm build, so type errors do fail the frontend build.
• Code style: Consistent within each side. Backend uses XML doc comments on interfaces, entities and non-obvious services; several comments explain why rather than what (the nonfile constraint, the reason build-production is a separate job in the reference project, the master-gate bypass rationale). Frontend is Prettier-formatted with 4-space indent.
• Comment language: Mixed Dutch and English in the C# codebase — ModuleOrchestrator logs and doc comments are Dutch, most newer code is English, and some user-facing strings are Dutch (AvailabilityMiddleware's 503 detail, SetupController's success message). Not a defect, but it means user-visible API messages are Dutch-only with no localisation path, while the frontend is fully i18n'd (NL/EN).
• Documentation: Strong. README.md is thorough and current (including the single-host model and production setup); CLAUDE.md/AGENTS.md/.junie/guidelines.md/.github/copilot-instructions.md document the solution layout; aidlc-docs/ holds the full AI-DLC history per feature.
• Naming: Follows .NET and React conventions consistently.
• Reproducibility: frontend/pnpm-lock.yaml exists and publish uses --frozen-lockfile. No packages.lock.json for any .NET project, so NuGet restore is not locked.

Technical Debt

Blocking for CI as it stands

1. pnpm run lint fails: 5 errors, 1 warning. Any workflow that gates on lint will go red on the current master:
   • src/components/cms/AddCmsInstanceDialog.tsx:55 — setState called synchronously inside an effect (react-hooks/set-state-in-effect)
   • src/components/users/InviteUserDialog.tsx:50 — same rule
   • src/components/users/InviteUserDialog.tsx:54 — variable accessed before declaration
   • src/pages/SettingsPage.tsx:40 — same setState-in-effect rule
   • src/components/cms/SetStatusDialog.tsx:32 — react-refresh/only-export-components: a non-component export shares the file
   • src/components/cms/SetStatusDialog.tsx:72 — warning: "Compilation Skipped: Use of incompatible library" Note the tests all pass regardless — these are lint-rule violations, not observed runtime failures.
2. Two high-severity transitive vulnerabilities (Microsoft.OpenApi 2.0.0, System.Security.Cryptography.Xml 10.0.9). A vulnerability gate cannot be switched on until these are addressed.

Deployment and operations gaps

3. No CI/CD whatsoever — no .gitea/workflows/, no build/test/deploy automation. Every deployment is manual today.
4. No health-check endpoint. There is nothing to point uptime monitoring at, and no existing endpoint can stand in: Availability and System/capabilities are CMS domain functionality (product on/off state and loaded-module reporting, both also serving the master↔slave protocol), not health signals. A dedicated health check — outside /api/v1 domain routing and outside the availability gate, reporting infrastructure liveness such as process up, database reachable and migrations applied — has to be built. It is in scope for the gitea-deployment-workflow feature because the framework supplies it almost for free: AddHealthChecks() + MapHealthChecks("/health") require no package, and a database probe costs only Microsoft.Extensions.Diagnostics.HealthChecks.EntityFrameworkCore 10.0.9 plus .AddDbContextCheck<ApplicationDbContext>(). /health must be added to AvailabilityMiddleware._bypassPrefixes, or the gate will return 503 for it on a disabled instance. See api-documentation.md § Observability endpoints.
5. No observability. No Sentry, no structured logging, no analytics, no metrics or tracing. Logging is the console provider at Warning level in the production baseline — which means production would emit almost nothing useful.
6. Data Protection has no persistent key ring. Both ApiKeyProtector and MasterApiKeyProtector use the default file-system store. A redeploy or app-pool recycle that discards the key folder makes every stored slave API key permanently unreadable, silently breaking master↔slave communication until instances are re-added. Already flagged in README.md; still unaddressed in code.
7. No forwarded-headers middleware, while UseHttpsRedirection() runs early. Behind a hosting provider's TLS-terminating proxy the app sees plain HTTP, which can cause redirect loops or wrong scheme in generated URLs. There is no UseForwardedHeaders and no ASPNETCORE_FORWARDEDHEADERS_ENABLED guidance.
8. The admin SPA bundle is environment-specific. frontend/src/lib/config.ts requires VITE_API_BASE_URL as an absolute URL, so a test build and a production build cannot be the same artifact — even though in the single-host model the API is same-origin with the SPA and a relative base would work. This forces "build twice" in CI (exactly the pattern the reference project had to adopt for VITE_APP_ENV), or a small code change to default to same-origin.
9. dotnet publish requires Node and pnpm because of the BuildAndCopyAdminFrontend target. Convenient locally; a hard constraint on any build agent, and it couples backend publish time to frontend install/build time.
10. ApplicationDbContext migrations are never applied automatically while the two module contexts are. This asymmetry means a fresh deployment silently starts with no Identity tables until someone runs dotnet ef database update, and there is no migration step in any pipeline (because there is no pipeline).
11. No Test environment configuration. Only appsettings.json and appsettings.Development.json exist; there is no appsettings.Test.json and no defined ASPNETCORE_ENVIRONMENT value for the test environment, even though three environments are in scope.
12. The production appsettings.json ships placeholder secrets (<secure-long-random-secret-key-from-env>, <production-db-host>). Safe — they are not real credentials, and env vars are expected to override them — but startup fails confusingly rather than clearly if an env var is missed, since JwtSettings:Secret is present-but-nonsense rather than absent.

Design-level debt

13. The availability gate does not cover the public website. Static files are served before orchestrator.UseModules(app) installs AvailabilityMiddleware, so disabling an instance blocks the API and admin routes but still serves wwwroot/index.html and its assets. Whether that is intended is a product decision, but it is currently implicit rather than documented, and it changes what "we disabled that customer" actually means.
14. AvailabilityController.UpdateStatus casts the injected IAvailabilityService to PersistentAvailabilityService and returns 400 if the cast fails — the controller depends on a concrete implementation. Carried over from the previous assessment; still present.
15. Module load failures are swallowed. ModuleOrchestrator logs and continues when an assembly cannot be loaded or a module cannot be instantiated. A deployment that ships a broken or missing module DLL starts up "successfully" with reduced capability rather than failing fast — hard to detect without monitoring, and directly relevant to trusting a deployment.
16. AvailabilityMiddleware.IsAdminBypass reads the JWT without validating its signature (JwtSecurityTokenHandler.ReadJwtToken), so anyone can craft an unsigned token carrying an Owner role claim and bypass the availability gate. Requests still fail authentication at protected endpoints afterwards, so this is not a privilege escalation — but it does mean the gate is bypassable by an unauthenticated caller, including on a master-disabled instance.
17. Inconsistent persistence style: Modules.Master and Modules.Availability use the repository pattern; Core's identity services use ApplicationDbContext and Identity managers directly. Recognised divide between older and newer code.
18. Microsoft.Extensions.Http.Resilience 9.6.0 on net10.0 targets — the only dependency out of step with the otherwise uniform 10.0.x line.
19. API error and status messages are Dutch-only, with no localisation mechanism on the backend, while the frontend is fully bilingual.

Patterns and Anti-patterns

Good Patterns

• Module/plugin architecture with reflection-based discovery — capability is a property of what is deployed, which makes the master/slave distinction a packaging concern rather than a configuration flag.
• Push and pull status synchronisation with fail-open — the master↔slave design assumes messages get lost and instances restart, and it self-heals in both directions without a broker. The fail-open rule is the right default for a commercial kill-switch.
• UpdateStatusResult { success, slaveContactSuccess } — honestly reports partial success instead of collapsing two different outcomes into one boolean.
• JWT with refresh-token rotation, access token in memory only, refresh cookie httpOnly and path-scoped to /api/v1/auth.
• Uniform RFC 9457 ProblemDetails via a global handler, mirrored by a typed ProblemDetailsError in the frontend client.
• Centralised route prefixing (ApiPrefixConvention) rather than repeating api/v1 in every controller.
• The nonfile route constraint on both SPA fallbacks — missing assets still 404 instead of being handed an HTML page, which is a genuinely easy mistake to make.
• Options pattern used consistently for all four configuration sections.
• Resilience pipeline on outbound master→slave calls with jittered exponential backoff.
• High, real test coverage with fast suites (219 backend tests in ~3s) and MSW-based frontend tests that avoid brittle mocking.
• Comments that explain rationale, not mechanics — several of the trickiest decisions in the codebase are documented at the point of the decision.
• Three-file appsettings pattern with appsettings.local.json gitignored and no real secrets committed.

Anti-patterns

• Concrete-type cast in AvailabilityController.UpdateStatus (item 14).
• Unvalidated JWT parsing in the availability gate's admin bypass (item 16).
• Silent module-load failure (item 15).
• Build-time environment coupling in the frontend config, forcing per-environment bundles (item 8).
• Backend project reference used purely as a deployment mechanism for module DLLs — it works and is documented, but the compile-time dependency does not reflect an actual code dependency.
• Asymmetric migration strategy across the three DbContext types (item 10).
• Mixed-language comments and Dutch-only user-facing API strings (items 19 and the note above).