SLP_Software/slp-modular-cms
1
0
Fork 0
Code Issues Pull Requests Actions Projects Releases Wiki Activity

Plans the Gitea deployment feature and refreshes the codebase analysis

Browse Source 
Adds the AI-DLC inception record for deploying the CMS as a single .NET
application on hosting where no server configuration is possible.

The reverse-engineering artifacts were regenerated: the previous set
predated the Master module, the Slave host, the solution reorganisation
and single-host serving, all of which matter for deployment. Findings
were verified by running the build, both test suites and the linter
rather than inferred, which surfaced two facts the plan depends on:
the frontend lint gate currently fails (5 errors), and two transitive
packages carry high-severity advisories.

Records 24 functional requirements, 32 traced decisions and a
seven-unit decomposition whose ordering is load-bearing: durability
work must land before the first automated deploy, or the very first
deploy is the one that silently breaks master/slave trust.

Two conflicts found while designing and carried into the units:
- Both modules call AddDataProtection(), which runs after the host and
  would override a persistent key store while still passing any
  registration test.
- The availability gate runs before authentication, so its admin
  bypass cannot read HttpContext.User.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HHoJpxYXzHACSQguHrC5fw
This commit is contained in:
Sluijsens
2026-07-27 23:59:30 +02:00
co-authored by Claude Opus 5
parent 38857038a0
commit 8568ca43c6
25 changed files with 4233 additions and 503 deletions
Download Patch File Download Diff File Expand all files Collapse all files
aidlc-docs/_shared/reverse-engineering/code-quality-assessment.md
+103 -22
View File
@@ -1,34 +1,115 @@
# Code Quality Assessment
# 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
- **Overall**: Fair — unit tests exist for Core and Availability modules
- **Unit Tests**: Present for Core.Tests and Modules.Availability.Tests
- **Integration Tests**: Not observed in current structure
- **Frontend Tests**: None (example app has no test files)
### 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
- **Linting**: Not explicitly configured (no .editorconfig or eslint config seen in backend; frontend likely uses Vite defaults)
- **Code Style**: Consistent — clean C# with XML doc comments on public interfaces and entities
- **Documentation**: Good for core interfaces and entities (XML doc comments); controllers have minimal comments
- **Naming**: Follows .NET conventions (PascalCase classes/methods, camelCase parameters)
- **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
- Auth context in example React app uses `localStorage` for user state (security concern — no httpOnly cookies)
- Example app auth-context simulates login locally without real API calls (will need to be replaced with actual API integration)
- No CORS configuration confirmed in backend (needs verification for SPA integration)
- `AvailabilityController.UpdateStatus` uses a direct service cast (`as PersistentAvailabilityService`) which couples controller to implementation
- No OpenAPI/Swagger spec currently integrated (would help frontend integration)
### 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 pattern provides clear separation of concerns between features
- JWT refresh token rotation is properly implemented
- Authorization policies are well-defined (OwnerOnly, AdminOnly)
- EF Core used consistently for persistence
- Service interfaces (IAuthService, IInvitationService, ISetupService) for testability
- **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
- Direct implementation cast in `AvailabilityController` (should use extended interface instead)
- Example React app uses localStorage-based auth (acceptable for prototype, not production)
- Example React app `auth-context` hardcodes mock users (must be replaced with real API calls)
- 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).