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

Code Review Report

Batch: 04 (cycle 8) Tasks: AZ-810 (POST /api/satellite/upload strict metadata validation, multipart envelope) Date: 2026-05-23 Verdict: PASS_WITH_WARNINGS

Findings

#	Severity	Category	File:Line	Title
1	Low	Maintainability / DRY	SatelliteProvider.Tests/Validators/UavTileBatchMetadataPayloadValidatorTests.cs, SatelliteProvider.Tests/Validators/UavTileMetadataValidatorTests.cs, SatelliteProvider.Tests/Services/UavTileQualityGateTests.cs, SatelliteProvider.Tests/Services/UavTileUploadHandlerTests.cs	FixedTimeProvider duplicated across four test files (now exceeds the "3 consumers → promote" threshold the cycle-2 file comment named)
2	Low	Maintainability / DRY	SatelliteProvider.IntegrationTests/UavUploadTests.cs:~270 + SatelliteProvider.IntegrationTests/UavUploadValidationTests.cs:600-614	PostBatch(client, metadata, files) multipart-build helper duplicated with identical signature/behavior across the two upload integration suites
3	Info	Wire-shape asymmetry	SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs:67-77	Errors raised inside the metadata-JSON deserialization (malformed JSON, type-mismatch, unknown root field, malformed flightId) all surface under errors["metadata"] — the JSON path inside the JsonException is intentionally not exploded into errors["metadata.items[0].latitude"]. Documented in the contract; tests assert the actual key.

Finding Details

F1: FixedTimeProvider duplication has now crossed the "promote to shared" threshold (Low / Maintainability)

• Location: four test files in SatelliteProvider.Tests/ carry an identical private sealed class FixedTimeProvider : TimeProvider { ... } body. Two pre-existed (AZ-488 cycle 2 — UavTileQualityGateTests, UavTileUploadHandlerTests); two are new in this batch (UavTileBatchMetadataPayloadValidatorTests, UavTileMetadataValidatorTests).
• Description: The cycle-2 file-level comment in UavTileQualityGateTests explicitly said "if a third consumer appears, promote to SatelliteProvider.TestSupport." Batch 4 added the third AND fourth consumer. The duplication is currently harmless (the implementations are byte-identical), but the next reader changing one of them risks a silent drift, especially since FluentValidation's RuleForEach.SetValidator(...) propagates the TimeProvider instance the root validator was given — a fork would not be detected by either side's unit tests.
• Suggestion: Promote FixedTimeProvider to SatelliteProvider.TestSupport/FixedTimeProvider.cs (analogous to JwtTokenFactory, IntegrationTestResetGuard). Update the four call-sites in a follow-up Low PBI. Do NOT do it as part of AZ-810 — it is out of task scope and would push four test files into the diff for no functional benefit.
• Suggestion target: open follow-up PBI "Promote FixedTimeProvider test helper to SatelliteProvider.TestSupport" (≈1 SP).
• Task: AZ-810 (cycle-2 carryover; AZ-810 just made the duplication visible enough to act on).

F2: PostBatch multipart helper duplicated across two integration test suites (Low / Maintainability)

• Location: SatelliteProvider.IntegrationTests/UavUploadTests.cs (cycle 2) and the new SatelliteProvider.IntegrationTests/UavUploadValidationTests.cs both define private static async Task<HttpResponseMessage> PostBatch(HttpClient client, object metadata, IReadOnlyList<byte[]> files) with identical signatures and bodies (serialize metadata via System.Text.Json, build MultipartFormDataContent, attach each file under the files name with image/jpeg).
• Description: Same drift risk as F1, but limited to the integration test project. The helpers diverging would break only the suite that did not get the update — both suites pass against the production endpoint, so the silent-drift surface is small. Still worth flagging because UavUploadTests' helper has subtly different JsonSerializerOptions setup that may want to be unified.
• Suggestion: Promote PostBatch to a shared UavUploadMultipartFixture (test-only helper) inside SatelliteProvider.IntegrationTests/. Both suites then reference one canonical builder. Defer to a follow-up PBI alongside F1.
• Task: AZ-810.

F3: Metadata-JSON deserialization errors collapse to errors["metadata"] (Info / Wire-shape asymmetry)

• Location: SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs::InvokeAsync lines 67-77 + 78-88 (the JsonException-catch path and the manual filter responses for missing form field / missing metadata).
• Description: When the JSON inside the metadata form field fails strict deserialization (malformed JSON, unknown root field via UnmappedMemberHandling.Disallow, unknown nested field, nested type mismatch, malformed flightId UUID, missing required field surfaced as JsonException), the filter catches the exception manually and surfaces it under a single error key — errors["metadata"] — with the full JsonException.Message (which itself includes the JSON path like $.items[0].latitude). It does NOT explode the JsonException path into a separate prefixed error key like errors["metadata.items[0].latitude"]. This is by design: the metadata is a NESTED JSON value inside a multipart form field, so the form-level wire-shape correctly reports the error at the form-field granularity. The JSON path is preserved inside the message text so debuggers can still localize. FluentValidation rule violations DO get the full prefixed path (errors["metadata.items[0].latitude"]).
• Suggestion: NONE. Documented in _docs/02_document/contracts/api/uav-tile-upload.md v1.2.0 "Metadata validation" section + _docs/02_document/contracts/api/error-shape.md. Integration tests use AssertErrorsContainsMention (substring match) so they tolerate either shape — important so the contract can later choose to explode JSON paths without breaking tests.
• Task: AZ-810.

Phase Summary

Phase	Outcome
1. Context Loading	Read AZ-810 spec, _docs/02_document/contracts/api/uav-tile-upload.md v1.1.0 (pre-bump), _docs/02_document/contracts/api/error-shape.md v1.0.0, batch-2 RegionRequestValidator + batch-3 CreateRouteRequestValidator for cycle-8 conventions, GlobalExceptionHandler.cs for the BadHttpRequestException → ValidationProblemDetails shape, and UavUploadTests.cs (AZ-488) for the legacy multipart fixture. The endpoint is uniquely multipart, so the cycle-8 generic ValidationEndpointFilter<T>() (JSON-body-only) does NOT fit — a new UavUploadValidationFilter extracts the metadata form field, runs deserialization with strict JsonSerializerOptions, then runs FluentValidation, then enforces the cross-field items.Count == files.Count invariant.
2. Spec Compliance	All 9 ACs covered. AC-1 (14 rules across deserializer + FluentValidation + cross-field) verified. AC-2 (happy path) verified by HappyPath_Returns200. AC-3 (validators in own files, ≥11 unit tests) — UavTileBatchMetadataPayloadValidator.cs + UavTileMetadataValidator.cs (2 files), 13 unit tests. AC-4 (integration ≥13) — 16 methods. AC-5 (contract v1.2.0) — bumped, validation rules section added. AC-6 (api_program.md) — updated. AC-7 ([JsonRequired] + .ProducesProblem(400)) — done; range annotations omitted per existing project pattern (FluentValidation messages convey the range). AC-8 (probe script) — scripts/probe_upload_validation.sh covers happy + 14 failure modes. AC-9 (no AZ-488 regression) — AZ-488 happy paths all use valid metadata (lat/lon in range, zoom=18, tileSizeMeters=200, capturedAt fresh) so the new validator accepts them unchanged; verified by tracing each UavUploadTests payload against the new validator rules.
3. Code Quality	Three new files in SatelliteProvider.Api/Validators/ follow SRP cleanly. UavTileBatchMetadataPayloadValidator is 6 rules (1 NotNull + 1 NotEmpty + 1 count cap + RuleForEach). UavTileMetadataValidator is 5 range/freshness rules + explanatory comment on the deliberate FlightId no-op. UavUploadValidationFilter is ~120 lines doing exactly one job — buffer the form, deserialize the metadata, run the validator, check items/files parity. ArgumentNullException.ThrowIfNull used consistently; no silent catches; manual ValidationProblem shapes match the RFC 7807 contract. [JsonRequired] placement on UavTileMetadata/UavTileBatchMetadataPayload follows the cycle-7 (TileInventoryRequest) and batch-3 cycle-8 (CreateRouteRequest) precedent. Two DRY findings (F1 + F2) — both test-only, both deferred to follow-up PBIs.
4. Security	All validation runs BEFORE any DB work, file write, or queue enqueue. The filter intercepts on the endpoint pipeline — even an authenticated caller cannot reach the handler without passing the validator. Cross-field items.Count == files.Count prevents an attacker from posting 100 metadata + 1 file (which would otherwise zip-bomb-style let the loop iterate over a short files array). UnmappedMemberHandling.Disallow prevents fingerprinting via unknown fields. The JsonException.Message surfaced under errors["metadata"] may include the offending JSON snippet — this is acceptable because the body is supplied by the caller themselves; it does not leak server-side state. JWT auth + RequireAuthorization(SatellitePermissions.UavUploadPolicy) retained on the endpoint. No new secrets, no PII in logs.
5. Performance	Validators run synchronously against in-memory record fields — microseconds even at the MaxBatchSize = 100 upper bound. ReadFormAsync buffers the multipart body once; the buffered IFormCollection is reused by the downstream handler (ASP.NET caches it on the HttpRequest). For invalid requests we DO buffer the full body before rejecting, but Kestrel's MaxRequestBodySize bounds this; the alternative (streaming validation) would require a custom multipart parser and is overkill. No N+1, no blocking I/O. Filter overhead per request: one ReadFormAsync (already needed by the handler), one JsonSerializer.Deserialize of the metadata string, one synchronous FluentValidation pass.
6. Cross-Task Consistency	Uses the same ProblemDetailsAssertions helper as batches 02/03 of cycle 8 and cycle 7. Error keys follow the same camelCase JSON-path policy per error-shape.md v1.0.0 Inv-4. ValidationProblemDetails produced has the same shape as the JSON-body endpoints (status=400, title="One or more validation errors occurred.", errors as a dict of arrays). Per-item indexed paths (items[0].latitude) follow the same convention as RegionRequestValidator's nested-DTO chain. New UavUploadValidationFilter is intentionally distinct from the generic ValidationEndpointFilter<T> — different envelope shape (multipart vs JSON body) — and the two filters' shape choices are mutually compatible.
7. Architecture Compliance	New validators + filter in SatelliteProvider.Api/Validators/ — owned by WebApi (Layer 4). [JsonRequired] additions in SatelliteProvider.Common/DTO/UavTileMetadata.cs (Layer 0). No new cross-layer dependencies. No cycles. IValidator<UavTileBatchMetadataPayload> resolves via the existing AddValidatorsFromAssemblyContaining<Program>() registration (cycle 7). UavUploadValidationFilter is added to DI as transient (matches existing endpoint filter registration pattern). No public-API surface changes in Common (DTOs already public, attributes are metadata-only). No ADRs to breach (project has no _docs/02_document/adr/ folder).

Files Reviewed

AZ-810 (UAV upload validator + multipart filter)

• SatelliteProvider.Api/Validators/UavTileBatchMetadataPayloadValidator.cs — NEW — root validator. RuleFor(p => p.Items) NotNull + NotEmpty + Must(<= MaxBatchSize); RuleForEach(p => p.Items).SetValidator(new UavTileMetadataValidator(qualityConfig, timeProvider)). The TimeProvider is threaded through so unit tests can inject a fixed clock and the produced per-item validator sees the same clock.
• SatelliteProvider.Api/Validators/UavTileMetadataValidator.cs — NEW — per-item validator. 5 rules (lat ∈ [-90, 90], lon ∈ [-180, 180], tileZoom ∈ [0, 22], tileSizeMeters > 0, capturedAt within now ± CapturedAtFutureSkewSeconds/now - MaxAgeDays). FlightId intentionally NOT validated beyond JSON shape — the AZ-503 anonymous-flight semantics keep it nullable, and shape-level rejection (malformed UUID) is handled at the deserializer layer.
• SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs — NEW — IEndpointFilter that intercepts multipart bodies. Reads metadata field, deserializes with the global strict JsonSerializerOptions (so UnmappedMemberHandling.Disallow applies), runs IValidator<UavTileBatchMetadataPayload>, then checks items.Count == files.Count. FluentValidation errors are prefixed with metadata. so the wire-key is metadata.items[0].latitude (full path); cross-field violation surfaces under BOTH errors["metadata.items"] AND errors["files"] so client UI code keyed on either field can act.
• SatelliteProvider.Common/DTO/UavTileMetadata.cs — [JsonRequired] on Latitude, Longitude, TileZoom, TileSizeMeters, CapturedAt (UavTileMetadata record) and on Items (UavTileBatchMetadataPayload record). FlightId stays optional. File-level comment block updated with the AZ-810 rationale so the next reader cannot accidentally remove the attributes thinking they are redundant.
• SatelliteProvider.Api/Program.cs — registered UavUploadValidationFilter as transient (builder.Services.AddTransient<UavUploadValidationFilter>()) and wired .AddEndpointFilter<UavUploadValidationFilter>() onto the MapPost("/api/satellite/upload", ...) chain along with .Accepts<> + .Produces<> + .ProducesProblem(400). Order: RequireAuthorization runs first (no validator burns CPU for unauthenticated callers), then AddEndpointFilter, then handler. Transient lifetime is intentional (fresh instance per request, no shared mutable state) — matches the cycle-8 batch-2 RejectUnknownQueryParamsEndpointFilter precedent.
• SatelliteProvider.Tests/Validators/UavTileBatchMetadataPayloadValidatorTests.cs — NEW — unit tests for the root validator: NotEmpty (empty list), MaxBatchSize boundary (100 vs 101), per-item failure propagation with indexed paths (items[1].latitude).
• SatelliteProvider.Tests/Validators/UavTileMetadataValidatorTests.cs — NEW — unit tests for the per-item validator: each range rule (positive + negative), freshness window (positive + past/future negative), FlightId null/Guid handled. Uses local FixedTimeProvider (see F1 — duplicated).
• SatelliteProvider.IntegrationTests/UavUploadValidationTests.cs — NEW — 16 integration tests covering happy path + 14 failure modes (rules 2-14 + AC-4 type-mismatch). Uses ProblemDetailsAssertions for the RFC 7807 shape check and AssertErrorsContainsMention for path/message substring matching.
• SatelliteProvider.IntegrationTests/Program.cs — wired UavUploadValidationTests.RunAll into BOTH the smoke and the full suites (matches the batch-2/3 cycle-8 pattern).
• scripts/probe_upload_validation.sh — NEW — bash + curl probe of the happy path + every failure mode. Reuses the existing probe_route_validation.sh structure (JWT mint, status-code assertion, --exit-on-fail).
• _docs/02_document/contracts/api/uav-tile-upload.md — bumped v1.1.0 → v1.2.0. Added "Metadata validation" section enumerating all 14 rules + the three enforcement layers (deserializer / FluentValidation / cross-field) + the error-shape mapping. Expanded "HTTP 400 — envelope error" section with the new failure shapes. Added v1.2.0 changelog entry.
• _docs/02_document/modules/api_program.md — updated endpoint description for POST /api/satellite/upload; added Api/Validators entries for the three new files; added Common/DTO (AZ-488) note about the new [JsonRequired] attributes; added DI registration entry for UavUploadValidationFilter.

Test Evidence

Pending — the implement skill defers the full integration-test suite run to autodev Step 11 (Run Tests). Per-file lint check on all 9 modified/new .cs files returned NO errors (ReadLints clean). Build sanity is implicit: ReadLints would surface compilation errors as Critical, and none surfaced.

Verdict Logic

• 0 Critical, 0 High, 0 Medium.
• 2 Low findings (F1 + F2) — both DRY in test-only code, both have a clear follow-up PBI plan, both safe to defer.
• 1 Info finding (F3) — documented design decision, contract-aligned, tests tolerant.
• PASS_WITH_WARNINGS — implement skill may proceed to Step 11 (commit + ask about push). Both Low findings tracked in this report for the cumulative review (Step 14.5) and the cycle-8 implementation report.