# Code Review Report — Batch 02 cycle 2

**Batch**: AZ-488 (UAV tile upload endpoint + 5-rule quality gate)
**Date**: 2026-05-11
**Verdict**: PASS_WITH_WARNINGS

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Style | _docs/02_document/components/01_web_api/description.md | Task spec referenced a doc path that does not exist (carried over from batch 01) |
| 2 | Low | Maintainability | SatelliteProvider.Services.TileDownloader/UavTileQualityGate.cs:23 | `JpegMagicBytes` declared as mutable `byte[]` instead of `ReadOnlySpan<byte>` static |
| 3 | Low | Maintainability | SatelliteProvider.Common/Configs/StorageConfig.cs | UAV path layout diverges from `StorageConfig.GetTileFilePath` — two contracts in one component (grandfathered per AZ-488 § Constraints) |
| 4 | Low | Performance | SatelliteProvider.Services.TileDownloader/UavTileUploadHandler.cs:152 | `File.WriteAllBytesAsync` requires `byte[]`; current code does `imageBytes.ToArray()` per accept (extra allocation) |

### Finding Details

**F1: Task spec referenced a doc path that does not exist in the codebase** (Low / Style)
- Location: `_docs/02_document/components/01_web_api/description.md` (referenced; does not exist)
- Description: The AZ-488 task spec § Scope > Documentation lists `_docs/02_document/components/01_web_api/description.md` as a doc to update. The component-doc folders are `01_common`, `02_data_access`, `03_tile_downloader`, `04_region_processing`, `05_route_management` — there is no `01_web_api` folder. This finding was first reported in batch 01 cycle 2 (AZ-487 F1) and is unchanged. WebApi's documentation lives in `_docs/02_document/modules/api_program.md` and has been updated there.
- Suggestion: Carry-over from batch 01 — needs an explicit operator decision: (a) create the missing folder with a stub that defers to `api_program.md`, or (b) update the documentation conventions to acknowledge WebApi lives in `modules/`. No change in this batch beyond updating `modules/api_program.md` and `components/03_tile_downloader/description.md`.
- Task: AZ-488 (carried over from AZ-487)

**F2: `JpegMagicBytes` declared as mutable `byte[]` instead of `ReadOnlySpan<byte>` static** (Low / Maintainability)
- Location: `SatelliteProvider.Services.TileDownloader/UavTileQualityGate.cs:23`
- Description: `private static readonly byte[] JpegMagicBytes = { 0xFF, 0xD8, 0xFF };` — `byte[]` allows in-place mutation of `JpegMagicBytes[0] = …` from inside the class. Not a security issue since the type is `private static`, but `static ReadOnlySpan<byte> JpegMagicBytes => [0xFF, 0xD8, 0xFF];` is a more intent-revealing C# 12 pattern, also slightly faster (no heap allocation; backed by RVA literal).
- Suggestion: Refactor when a follow-up touches this file. Not blocking — the constant is private and isolated.
- Task: AZ-488

**F3: UAV path layout diverges from `StorageConfig.GetTileFilePath`** (Low / Maintainability)
- Location: `SatelliteProvider.Common/Configs/StorageConfig.cs` (GetTileFilePath) vs `SatelliteProvider.Services.TileDownloader/UavTileUploadHandler.cs:182` (BuildUavTileFilePath)
- Description: Google Maps tiles use `StorageConfig.GetTileFilePath` → `{TilesDirectory}/{zoom}/{x_bucket}/{y_bucket}/tile_{z}_{x}_{y}_{ts}.jpg`. UAV tiles use `UavTileUploadHandler.BuildUavTileFilePath` → `{TilesDirectory}/uav/{z}/{x}/{y}.jpg`. Two file-naming contracts coexist in one component. This is explicitly grandfathered by the AZ-488 task spec § Scope/Constraints ("Per-source file-path strategy is fixed; do NOT migrate Google Maps files"), so it's intentional, not a defect.
- Suggestion: Documented in `architecture.md` § ADR-004 and `data_model.md`. If a future task unifies storage layouts, both consumers should move to a single helper on `StorageConfig`. Carrying this as a known divergence is acceptable.
- Task: AZ-488

**F4: `File.WriteAllBytesAsync` requires `byte[]` — `imageBytes.ToArray()` per accept** (Low / Performance)
- Location: `SatelliteProvider.Services.TileDownloader/UavTileUploadHandler.cs:152`
- Description: `await File.WriteAllBytesAsync(filePath, imageBytes.ToArray(), cancellationToken);` allocates a new array of up to 5 MiB per accepted tile. With `MaxBatchSize=100` × `MaxBytes=5 MiB` that is up to 500 MiB of extra allocations per batch worst-case. The `(String, ReadOnlyMemory<Byte>, CancellationToken)` overload of `File.WriteAllBytesAsync` is .NET 9+, so it is NOT available on this project's `net8.0` target — `ToArray()` is the only API-direct option here.
- Suggestion: Use `await using var fs = new FileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.None, bufferSize: 4096, useAsync: true); await fs.WriteAsync(imageBytes, cancellationToken);` — `FileStream.WriteAsync(ReadOnlyMemory<byte>, CancellationToken)` is available on net8.0 and skips the `ToArray()` copy. Not blocking — quality-gate cost target (< 50 ms / item) is dominated by Rule 5 decode + downsample, not the allocation. Address when PT-08 measurement starts (see `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md`).
- Task: AZ-488

## Phase Notes

### Phase 1 — Context Loading
- Task spec: `_docs/02_tasks/todo/AZ-488_uav_tile_upload.md` (now archived under done/ at end of batch).
- Plan artifacts: `_docs/02_task_plans/uav-batch-upload/00_research/00_ac_assessment.md`, `_docs/02_task_plans/uav-batch-upload/01_solution/solution_draft01.md`, `_docs/02_task_plans/uav-batch-upload/problem.md`.
- Contracts consumed: `_docs/02_document/contracts/data-access/tile-storage.md` v1.0.0 (per-source UPSERT).
- Contract produced: `_docs/02_document/contracts/api/uav-tile-upload.md` v1.0.0 (frozen).
- Prior batch: batch 01 cycle 2 (AZ-487) — JWT bearer middleware + Swagger Authorize button + `RequireAuthorization()` on all endpoints. AZ-488 layers permission policy on top.

### Phase 2 — Spec Compliance
All 10 ACs are demonstrably covered by automated tests:

| AC | Description | Tests |
|----|-------------|-------|
| AC-1 | Happy path single item persists with source='uav' | `UavUploadTests.HappyPath_BatchOfTwoTiles_Returns200_PersistsRows`, `UavTileUploadHandlerTests.HappyPath_SingleItem_InsertsRow_WithUavSource` |
| AC-2 | Mixed batch partial reject | `UavUploadTests.MixedBatch_PartialReject_Returns200_WithPerItemResults`, `UavTileUploadHandlerTests.MixedBatch_OnlyAcceptedItemsInserted` |
| AC-3 | Multi-source coexistence with Google Maps | `UavUploadTests.MultiSourceCoexistence_AZ484_Cycle2` |
| AC-4 | Same-source UPSERT | `UavUploadTests.SameSourceUpsert_AZ484_Cycle2` |
| AC-5 | Unauth → 401 | `UavUploadTests.NoToken_Returns401` |
| AC-6 | Missing GPS perm → 403 | `UavUploadTests.ValidTokenWithoutGpsPermission_Returns403`, `PermissionsRequirementTests.HandleRequirement_*` (12 unit tests) |
| AC-7a | Wrong content-type / magic → INVALID_FORMAT | `UavTileQualityGateTests.Validate_RejectsNonJpegContentType`, `Validate_RejectsJpegContentTypeWithWrongMagic` |
| AC-7b | Size out of band | `UavTileQualityGateTests.Validate_RejectsTooSmall`, `Validate_RejectsTooLarge` |
| AC-7c | Wrong dimensions | `UavTileQualityGateTests.Validate_RejectsWrongDimensions` |
| AC-7d | Captured-at future / too old | `UavTileQualityGateTests.Validate_RejectsCapturedInFuture`, `Validate_RejectsCapturedTooOld` |
| AC-7e | Blank/uniform → IMAGE_TOO_UNIFORM | `UavTileQualityGateTests.Validate_RejectsUniformImage`, `Validate_AcceptsHighVarianceImage` |
| Rule ordering | First-failing rule wins | `UavTileQualityGateTests.Validate_FormatBeforeDimensions` |
| AC-8 | Oversized batch → 400 | `UavUploadTests.OversizedBatch_Returns400`, `UavTileUploadHandlerTests.Oversized_EnvelopeRejected` |
| AC-9 | Contract docs match impl | Manual: `_docs/02_document/contracts/api/uav-tile-upload.md` v1.0.0 frozen, reject reasons match `Common.DTO.UavTileRejectReasons`, request shape matches `UavTileBatchUploadRequest` + `UavTileBatchMetadataPayload` |
| AC-10 | Existing tests pass | Deferred to test execution (Step 11) |

**Spec gaps** — none. The earlier PT-08 gap (NFR mandated in same commit) was closed by adding `PT-08` to `_docs/02_document/tests/performance-tests.md` with Status `Deferred — harness work tracked in _docs/_process_leftovers/2026-05-11_perf-pt07-harness.md` and a cross-reference appended to that leftover so PT-08 lands when the PT-07 harness lands. Per the AZ-488 task spec § Risk 4 / cycle 1 retro Action 2, the Deferred branch is explicitly sanctioned ("NOT as an active scenario" → "Deferred — harness work tracked in <follow-up ticket>").

**Contract verification** — `_docs/02_document/contracts/api/uav-tile-upload.md` v1.0.0:
- Request shape matches `UavTileBatchUploadRequest` + `UavTileBatchMetadataPayload` + `UavTileMetadata` (multipart `metadata` JSON string + `files` collection; per-item ordinal alignment).
- Response shape matches `UavTileBatchUploadResponse` + `UavTileUploadResultItem` (per-item `index`, `status`, `tileId?`, `rejectReason?`, `rejectDetails?`).
- Reject-reason closed enumeration matches `UavTileRejectReasons` constants exactly (7 reasons: `INVALID_FORMAT`, `SIZE_OUT_OF_BAND`, `WRONG_DIMENSIONS`, `CAPTURED_AT_FUTURE`, `CAPTURED_AT_TOO_OLD`, `IMAGE_TOO_UNIFORM`, `STORAGE_FAILURE`).
- Status codes (200, 400, 401, 403) match `Program.cs` endpoint annotations.
- Cross-reference to `tile-storage.md` v1.0.0 is present (per-source UPSERT semantics).

### Phase 3 — Code Quality
- **SRP**: `UavTileQualityGate` validates only; `UavTileUploadHandler` orchestrates only; `PermissionsAuthorizationHandler` authorizes only. Clean separation.
- **Error handling**: per-item `try/catch` in `UavTileUploadHandler.HandleAsync` narrowed to `IOException` / `UnauthorizedAccessException` → `STORAGE_FAILURE`. No bare catches. Envelope-level errors return `EnvelopeRejected=true` with the original message preserved (no swallowing).
- **Naming**: `UavTileQualityResult.Pass()/Fail()`, `UavTileUploadHandlerResult.EnvelopeRejected`, `BuildUavTileFilePath` all read at the call site.
- **Complexity**: `Validate` is ~70 lines but linear with one short-circuit per rule — easy to follow. No methods exceed 50 logical lines.
- **DRY**: `ReadOnlyMemoryStream` is a small, internal utility (no `MemoryStream` over a `byte[]` copy path).
- **Test quality**: each rule has both happy and reject coverage; rule ordering is independently tested. Mocked-repo handler tests assert call count + arguments, not just "no exception".
- **Dead code**: legacy `UploadImageRequest` was deleted; old stub test `StubUpload_Returns501` was deleted to match the new shape.

### Phase 4 — Security Quick-Scan
- No SQL string interpolation in this batch (DataAccess goes through Dapper parameterized queries already).
- No `Process.Start`, no `eval`, no dynamic SQL.
- No hardcoded secrets in implementation code.
- Input validation: image bytes are size-bounded (5 KiB - 5 MiB), dimensions are enforced exact-equal to `MapConfig.TileSizePixels`, JSON metadata is bounded by `MaxBatchSize`, framework body-size limit set to `MaxBatchSize × MaxBytes`.
- Path-traversal: `BuildUavTileFilePath` takes `int` tileZoom/X/Y and uses `Path.Combine` with `InvariantCulture` integer formatting — no caller-supplied strings, no `..` escape vector.
- Sensitive data in logs: `UavTileUploadHandler` logs storage failures with `_logger.LogError(ex, "UAV tile persistence failed at index {Index}", index)` — no file paths or user data in the message. Reject-reason `RejectDetails` is set only by the quality gate (currently always null for the 7 closed reasons; safe).
- Deserialization: `JsonSerializer.Deserialize<UavTileBatchMetadataPayload>` with case-insensitive matching but no `JsonStringEnumConverter` injection — safe (no enum fields in the payload).

### Phase 5 — Performance Scan
- Rule 5 (luminance variance) does `Image.Load<L8>` then `Mutate(Resize(32,32))` then `ProcessPixelRows`. Welford's online variance avoids the 2-pass sum + sum-of-squares. Correct shape for the < 50 ms target.
- Rule 3 uses `Image.Identify` (header-only) — does NOT decode the full image. Correct.
- N+1 query risk: `InsertAsync` is per-accepted-item. Acceptable at `MaxBatchSize=100`; if batches grow, a `BulkInsertAsync` would help. Out of scope for this PBI.
- Blocking I/O in async context: file write uses `File.WriteAllBytesAsync` (true async). Good.
- F4 (above) is the only observed allocation hot-spot — Low severity.

### Phase 6 — Cross-Task Consistency
- AZ-487 (batch 01 cycle 2) exposed `AddSatelliteJwt(builder.Configuration)`; AZ-488 layers `AddAuthorization` on top with the `RequiresGpsPermission` policy. Compatible.
- AZ-484 produced `TileSourceConverter.ToWireValue(TileSource.Uav)` — AZ-488 calls it via the sanctioned path. Compatible (per L-001 in `_docs/LESSONS.md`).
- DTO layering: `UavTileMetadata`, `UavTileBatchUploadResponse`, `UavTileRejectReasons` live in `SatelliteProvider.Common.DTO` so both the API and the service can reference them without a Service → API dependency. The endpoint-specific `UavTileBatchUploadRequest` envelope stays in `SatelliteProvider.Api.DTOs`. Layering preserved.
- JSON conventions: handler uses `JsonSerializerOptions { PropertyNameCaseInsensitive = true }`, matching the API's camelCase / case-insensitive `ConfigureHttpJsonOptions` block.

### Phase 7 — Architecture Compliance
Files in scope (touched in this batch):
- `SatelliteProvider.Api/Program.cs` — Layer 4 (Api). Imports: `Common.*`, `DataAccess.*`, `Services.*`, `Api.Authentication.*`, `Api.DTOs.*`. All directionally correct.
- `SatelliteProvider.Api/Authentication/PermissionsRequirement.cs` — Layer 4 (Api). Imports: `Microsoft.AspNetCore.Authorization`, `System.Security.Claims`, `System.Text.Json`. No cross-component import. ✓
- `SatelliteProvider.Api/DTOs/UavTileBatchUploadRequest.cs` — Layer 4 (Api). Imports: `Microsoft.AspNetCore.Http`, `Microsoft.AspNetCore.Mvc`. ✓
- `SatelliteProvider.Common/Configs/UavQualityConfig.cs` — Layer 1 (Common). No upward imports. ✓
- `SatelliteProvider.Common/DTO/UavTileMetadata.cs`, `UavTileBatchUploadResponse.cs` — Layer 1 (Common). No upward imports. ✓
- `SatelliteProvider.Services.TileDownloader/UavTileQualityGate.cs` — Layer 3 (Services). Imports: `Common.Configs`, `Common.DTO`, `SixLabors.ImageSharp.*`. ✓
- `SatelliteProvider.Services.TileDownloader/UavTileUploadHandler.cs` — Layer 3 (Services). Imports: `Common.*`, `DataAccess.Models`, `DataAccess.Repositories`. Service → DataAccess is allowed per `module-layout.md`. ✓
- `SatelliteProvider.Services.TileDownloader/TileDownloaderServiceCollectionExtensions.cs` — Layer 3 (Services). Adds `IUavTileQualityGate`, `IUavTileUploadHandler` singletons. ✓

**Layer direction**: clean. No Service → API import. No DataAccess → Service. No DataAccess → API.

**Public API respect**: cross-component imports go through:
- `Common.Configs.{UavQualityConfig, StorageConfig, MapConfig}` (public)
- `Common.DTO.{UavTileMetadata, UavTileBatchUploadResponse, UavTileRejectReasons, UavTileUploadStatus, UavTileBatchMetadataPayload, UavTileUploadResultItem}` (public)
- `Common.Enums.{TileSource, TileSourceConverter}` (public)
- `Common.Utils.GeoUtils` (public)
- `DataAccess.Models.TileEntity`, `DataAccess.Repositories.ITileRepository` (public per AZ-484)
- `Services.TileDownloader.{IUavTileQualityGate, IUavTileUploadHandler, UavUploadFile}` (public)

No internal-file imports across components.

**No new cyclic dependencies**: import graph is acyclic — Api → Services → DataAccess → Common; Services → Common; Api → Common. No new edges added.

**Duplicate symbols across components**: none.

**Cross-cutting concerns**: `PermissionsAuthorizationHandler` is an Api-layer concern (authorization handlers map to ASP.NET Core's authorization pipeline, which lives at the Api layer). Correctly placed in `SatelliteProvider.Api/Authentication/`. Not duplicated elsewhere.

## Baseline Delta

No `_docs/02_document/architecture_compliance_baseline.md` exists in this repository. Skip baseline-delta partitioning.

## Verdict Logic

- 0 Critical findings
- 0 High findings
- 0 Medium findings
- 4 Low findings

→ **PASS_WITH_WARNINGS** — proceed to commit.