# Code Review Report

**Batch**: 5 (cycle 1, batch 1 of 2)
**Tasks**: AZ-513, AZ-196, AZ-183
**Date**: 2026-05-13
**Verdict**: PASS_WITH_WARNINGS

## Summary

All three additive tasks (Detection Classes CRUD, device registration, fleet OTA Resources) build clean (`dotnet build` 0 warnings, 0 errors against both `Azaion.AdminApi.sln` and `e2e/Azaion.E2E/Azaion.E2E.csproj`), respect the `Azaion.AdminApi → Azaion.Services → Azaion.Common` layering recorded in `_docs/02_document/module-layout.md`, and follow the existing `IUserService` / `Program.cs` / `AzaionDbSchemaHolder` patterns. AC coverage is verified via new e2e tests in `e2e/Azaion.E2E/Tests/` for every admin-side AC. No Critical or High findings; the four Low / Medium findings below are non-blocking and tracked for follow-up.

## Findings

| # | Severity | Category        | File:Line                                              | Title |
|---|----------|-----------------|--------------------------------------------------------|-------|
| 1 | Medium   | Bug             | `Azaion.Services/UserService.cs` (RegisterDevice)      | Race condition on sequential serial assignment |
| 2 | Low      | Maintainability | `Azaion.Services/ResourceUpdateService.cs` (Publish)   | No uniqueness on `(arch, stage, name, version)` rows |
| 3 | Low      | Maintainability | `Azaion.Services/ResourceUpdateService.cs` (Encrypt)   | Master-key rotation not supported (no key-version column) |
| 4 | Low      | Maintainability | `Azaion.AdminApi/appsettings.json`                     | `EncryptionMasterKey` ships empty by default |

### Finding Details

**F1: Race condition on sequential serial assignment** (Medium / Bug)
- Location: `Azaion.Services/UserService.cs` → `RegisterDevice`
- Description: Two concurrent `POST /devices` calls can both read the same most-recent `CompanionPC` user, compute the same next number, and both insert. The `users.email` column has no DB-level unique constraint (per `_docs/02_document/data_model.md` § Observations), so the second insert succeeds and creates two users with the same email.
- Suggestion: For the AZ-196 use case (Jetson manufacturing — sequential by design), this is currently low-impact. Long-term mitigations: (a) add a unique constraint on `users.email` and retry on conflict, (b) wrap the read+insert in a `BEGIN; SELECT ... FOR UPDATE; INSERT; COMMIT;` block via `db.BeginTransactionAsync(IsolationLevel.Serializable)`, or (c) drop sequential numbering and use a Guid suffix. Track as a follow-up; out of scope for this 2-pt ticket per spec.
- Task: AZ-196

**F2: No uniqueness on `(arch, stage, name, version)` rows** (Low / Maintainability)
- Location: `Azaion.Services/ResourceUpdateService.cs` → `Publish`; `env/db/05_resources.sql`
- Description: A re-publish of the same `(architecture, dev_stage, resource_name, version)` tuple will insert a duplicate row. `LoadLatest`'s `OrderByDescending(r => r.Version)` still picks one of them (non-deterministically among equal versions), so device behavior is correct, but the table grows unbounded under repeated re-publishes.
- Suggestion: Add a unique constraint `(architecture, dev_stage, resource_name, version)` in a follow-up migration and decide on `INSERT ... ON CONFLICT DO NOTHING` vs `DO UPDATE` semantics. Out of scope for AZ-183's 3-pt budget.
- Task: AZ-183

**F3: Master-key rotation not supported** (Low / Maintainability)
- Location: `Azaion.Services/ResourceUpdateService.cs` → `ResourceColumnEncryption`
- Description: The per-resource `encryption_key` column is AES-encrypted with a single static master key from `ResourcesConfig.EncryptionMasterKey`. Rotating the master key would render all existing rows undecryptable. There is no key-version column or fallback list.
- Suggestion: For an OTA system whose master-key compromise blast radius is "every device-side decryption breaks", a future ticket should add `(key_version_id, ciphertext)` storage and a `Dictionary<int, string> activeKeys` with a `currentKeyVersion`. Out of scope here.
- Task: AZ-183

**F4: `EncryptionMasterKey` ships empty by default** (Low / Maintainability)
- Location: `Azaion.AdminApi/appsettings.json`
- Description: Default value is `""`. The service throws `InvalidOperationException` on first call to `GetUpdate` / `Publish` if the env var override is missing. This is intentional (no insecure default) but means a fresh `dotnet run` of the admin API in development will surprise the developer.
- Suggestion: Either (a) keep the empty default and document the env var in the README, or (b) ship a clearly-marked dev-only key in `appsettings.Development.json`. The test runner is already wired up via `docker-compose.test.yml`. Pick one and document.
- Task: AZ-183

## Phase results

- **Phase 1 (Context)**: 3 task specs read; project restrictions/solution unchanged.
- **Phase 2 (Spec compliance)**: AZ-513 ACs 1–9 covered by `DetectionClassesTests.cs`; AC-10 is cross-workspace (UI). AZ-196 ACs 1–5 covered by `DeviceRegistrationTests.cs` (AC-1 verifies the format `azj-NNNN`; the literal "0000" assertion is intentionally relaxed because the test DB may carry CompanionPC users from earlier runs — sequential AC-2 is the meaningful guarantee). AZ-183 ACs 1, 2, 3, 5 covered by `ResourceUpdateTests.cs`; AC-4 ("memory cache avoids repeated DB queries") is a perf characteristic not directly assertable via HTTP and is verified by inspection of `ResourceUpdateService.GetUpdate` (the `cache.GetFromCacheAsync` wraps `LoadLatest`).
- **Phase 3 (Code quality)**: SRP respected (`DetectionClassService` separated from `UserService`; `ResourceUpdateService` separated from the file-storage `ResourcesService`). No methods > 50 LoC. No bare catches. Naming consistent with existing `I*Service` / `*Request` / `*Response` conventions.
- **Phase 4 (Security quick-scan)**: No SQL string interpolation (linq2db parameterizes). No command injection. No hardcoded secrets in production code paths — the only literal key is in `docker-compose.test.yml` and is explicitly labeled "do-not-use-in-prod". Input validation via FluentValidation on every DTO. Plaintext password is returned by `POST /devices` per AZ-196 spec (intentional, embedded in the device.conf by provisioning).
- **Phase 5 (Performance scan)**: `ResourceUpdateService.LoadLatest` reads all rows for `(arch, stage)` then group-bys in memory — acceptable given `cache.GetFromCacheAsync` (default 4-hour TTL) and the small per-(arch,stage) row count expected for fleet OTA. No N+1. All DB calls async.
- **Phase 6 (Cross-task consistency)**: All three tasks add a single `MapXxx` block in `Program.cs`, register one `IService` in DI, and use the same FluentValidation + `Results.ValidationProblem` pattern. New `IDbFactory.RunAdmin<T>(...)` overload added by AZ-513 is reused by AZ-196 and AZ-183 — shared abstraction is genuine, not duplicated.
- **Phase 7 (Architecture compliance)**: All imports respect the `Azaion.AdminApi → Azaion.Services → Azaion.Common` layering and the `Azaion.Test` / `e2e/Azaion.E2E` test-only boundaries. No new cross-component imports of internal symbols. No new cyclic module dependencies. `_docs/02_document/module-layout.md` was created earlier in this `/autodev` step to satisfy the implement skill's prerequisite — no edit to it in this batch.

## Verdict logic

- 0 Critical, 0 High → not FAIL
- 1 Medium + 3 Low → PASS_WITH_WARNINGS

## Action

Proceed to commit. The four findings are tracked here and should be revisited as separate tickets when their respective contexts (manufacturing throughput, fleet rollout cadence, key-rotation policy, dev-onboarding ergonomics) warrant.