satellite-provider/_docs/03_implementation/reviews/batch_02_cycle2_review.md

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) — RESOLVED in cycle 3 (AZ-495)

• 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)
• Resolution (AZ-495, cycle 3): Option B formalized as canonical convention. _docs/02_document/module-layout.md § Documentation Layout now explicitly states WebApi has no components/* folder; documentation anchor is modules/api_program.md. The .cursor/skills/new-task/SKILL.md Step 4 (Codebase Analysis) directs future agents at the correct path. Finding will not recur.

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 ").

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.