# 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 `PackageReference`s** 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).