mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 09:01:16 +00:00
[AZ-808] [AZ-809] [AZ-810] [AZ-811] [AZ-812] Cycle 8 close
Closes cycle 8 (Strict input validation across every public API endpoint). After 4 batches, every JSON-body, multipart-envelope, and query-parameter endpoint rejects unknown fields, missing required axes, type mismatches, and business-rule violations BEFORE the handler runs, all surfacing RFC 7807 ValidationProblemDetails per error-shape.md v1.0.0. Artifacts: - cumulative_review_batches_01-04_cycle8_report.md PASS_WITH_WARNINGS. Cross-batch consistency check across all 5 cycle-8 tasks: 0 Critical / 0 High / 0 Medium / 4 Low (all surfaced as per-batch findings; no NEW cumulative-only categories). 5 follow-up PBI candidates surfaced (test-helper consolidation, validator filter decision matrix in docs, RoutePointDto wire-shape unification, service-layer RouteValidator retirement decision). - implementation_completeness_cycle8_report.md PASS. Every cycle-8 task promise is implemented as production behaviour. Production code verified for scaffold / placeholder / NotImplemented markers: none found in any cycle-8 validator. All five pipelines (region POST, lat/lon GET, route POST, upload POST, inventory POST) WIRED. - implementation_report_strict_validation_cycle8.md Final cycle implementation report. 41 / 41 ACs covered across 5 tasks (AZ-812, AZ-808, AZ-811, AZ-809, AZ-810). 63 new unit test methods + 52 new integration test methods + 4 new curl probe scripts + 3 new contract docs (region-request, tile-latlon, route-creation) + 1 contract version bump (uav-tile-upload v1.1.0 -> v1.2.0). Handoff to autodev Step 11 (Run Tests) documented. Autodev state transitions Step 10 (Implement) -> Step 11 (Run Tests). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,181 @@
|
|||||||
|
# Cumulative Code Review — Batches 01–04 cycle 8
|
||||||
|
|
||||||
|
**Batch range**: 01-04 (cycle 8)
|
||||||
|
**Cycle**: 8 (Strict input validation across all public API endpoints)
|
||||||
|
**Date**: 2026-05-23
|
||||||
|
**Verdict**: PASS_WITH_WARNINGS
|
||||||
|
**Trigger**: Implement skill Step 14.5 (K=3 default → first cumulative review at batch 4 because the cycle ran 1→2→3→4 contiguously; review covers the full batch range since the cycle's first batch)
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
|
||||||
|
| Batch | Tasks | Surfaces touched |
|
||||||
|
|-------|-------|------------------|
|
||||||
|
| 01 | AZ-812 | `SatelliteProvider.Common/DTO/RequestRegionRequest.cs`, `SatelliteProvider.Api/Program.cs`, `SatelliteProvider.IntegrationTests/{Models,RegionTests,IdempotentPostTests,SecurityTests,RegionFieldRenameTests,Program}.cs`, `scripts/run-performance-tests.sh`, `_docs/02_document/modules/{common_dtos,api_program}.md` |
|
||||||
|
| 02 | AZ-808 + AZ-811 | `SatelliteProvider.Api/Validators/{RegionRequestValidator,GetTileByLatLonQueryValidator,RejectUnknownQueryParamsEndpointFilter}.cs` (NEW), `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs` (NEW), `SatelliteProvider.Common/DTO/RequestRegionRequest.cs` (`[JsonRequired]`), `SatelliteProvider.Api/{Program,Swagger/ParameterDescriptionFilter}.cs`, `SatelliteProvider.Tests/Validators/{RegionRequestValidatorTests,GetTileByLatLonQueryValidatorTests,RejectUnknownQueryParamsEndpointFilterTests}.cs` (NEW), `SatelliteProvider.IntegrationTests/{RegionRequestValidationTests,GetTileByLatLonValidationTests,ProblemDetailsAssertions,TileInventoryValidationTests,RegionFieldRenameTests,TileTests,JwtIntegrationTests,SecurityTests,Program}.cs`, `scripts/{probe_region_validation,probe_latlon_validation,run-performance-tests}.sh`, `README.md`, `_docs/02_document/contracts/api/{region-request,tile-latlon}.md` (NEW v1.0.0), `_docs/02_document/modules/{api_program,common_uuidv5}.md`, `_docs/02_document/{system-flows,tests/blackbox-tests,tests/security-tests}.md` |
|
||||||
|
| 03 | AZ-809 | `SatelliteProvider.Common/DTO/{CreateRouteRequest,RoutePoint,GeofencePolygon,GeoPoint}.cs` (`[JsonRequired]`), `SatelliteProvider.Api/Validators/{CreateRouteRequestValidator,RoutePointValidator,GeofencePolygonValidator}.cs` (NEW), `SatelliteProvider.Api/Program.cs`, `SatelliteProvider.Tests/Validators/{CreateRouteRequestValidatorTests,RoutePointValidatorTests,GeofencePolygonValidatorTests}.cs` (NEW), `SatelliteProvider.IntegrationTests/{CreateRouteValidationTests,Program}.cs`, `scripts/probe_route_validation.sh` (NEW), `_docs/02_document/contracts/api/route-creation.md` (NEW v1.0.0), `_docs/02_document/modules/{api_program,common_dtos}.md`, `_docs/02_document/{system-flows,tests/blackbox-tests,tests/security-tests}.md` |
|
||||||
|
| 04 | AZ-810 | `SatelliteProvider.Common/DTO/UavTileMetadata.cs` (`[JsonRequired]`), `SatelliteProvider.Api/Validators/{UavTileBatchMetadataPayloadValidator,UavTileMetadataValidator,UavUploadValidationFilter}.cs` (NEW), `SatelliteProvider.Api/Program.cs`, `SatelliteProvider.Tests/Validators/{UavTileBatchMetadataPayloadValidatorTests,UavTileMetadataValidatorTests}.cs` (NEW), `SatelliteProvider.IntegrationTests/{UavUploadValidationTests,Program}.cs`, `scripts/probe_upload_validation.sh` (NEW), `_docs/02_document/contracts/api/uav-tile-upload.md` (v1.1.0 → v1.2.0), `_docs/02_document/modules/api_program.md` |
|
||||||
|
|
||||||
|
## Phase-by-Phase Summary (cumulative)
|
||||||
|
|
||||||
|
### Phase 1: Context Loading
|
||||||
|
|
||||||
|
The 4 batches share a coherent theme — **strict input validation across every public API endpoint**, anchored on the cycle-7 (`tile-inventory.md` v2.0.0 + `error-shape.md` v1.0.0 + `InventoryRequestValidator` + `GlobalExceptionHandler`) infrastructure. The cycle covers the full surface:
|
||||||
|
|
||||||
|
| Endpoint | Method | Batch | Pattern | Result |
|
||||||
|
|---------|--------|-------|---------|--------|
|
||||||
|
| `POST /api/satellite/request` | JSON body | 02 (AZ-808) | `.WithValidation<RequestRegionRequest>()` | All inputs validated |
|
||||||
|
| `POST /api/satellite/route` | JSON body | 03 (AZ-809) | `.WithValidation<CreateRouteRequest>()` + nested DTO chain | All inputs validated |
|
||||||
|
| `POST /api/satellite/upload` | multipart | 04 (AZ-810) | `.AddEndpointFilter<UavUploadValidationFilter>()` | All inputs validated |
|
||||||
|
| `GET /api/satellite/tiles/latlon` | query params | 02 (AZ-811) | `.WithValidation<GetTileByLatLonQuery>() + RejectUnknownQueryParamsEndpointFilter` | All inputs validated |
|
||||||
|
| `POST /api/satellite/tiles/inventory` | JSON body | 07 (AZ-794+795+796) | `.WithValidation<TileInventoryRequest>()` | Pre-existing |
|
||||||
|
| `GET /api/satellite/region/{id}` | path Guid | n/a | Framework Guid coercion | Reads only — strict-validation N/A by design |
|
||||||
|
| `GET /api/satellite/route/{id}` | path Guid | n/a | Framework Guid coercion | Reads only — strict-validation N/A by design |
|
||||||
|
|
||||||
|
AZ-812 (batch 1) was the prerequisite renaming work that aligned `Region` input wire to OSM `lat`/`lon` — the same convention every subsequent cycle-8 batch standardized on.
|
||||||
|
|
||||||
|
### Phase 2: Spec Compliance
|
||||||
|
|
||||||
|
| Batch | ACs claimed | ACs covered | Spec gaps |
|
||||||
|
|-------|-------------|-------------|-----------|
|
||||||
|
| 01 (AZ-812) | 6 | 6 | 0 |
|
||||||
|
| 02 (AZ-808) | 8 | 8 | 0 |
|
||||||
|
| 02 (AZ-811) | 9 | 9 | 0 |
|
||||||
|
| 03 (AZ-809) | 9 | 9 | 0 |
|
||||||
|
| 04 (AZ-810) | 9 | 9 | 0 |
|
||||||
|
| **Total** | **41** | **41** | **0** |
|
||||||
|
|
||||||
|
Cumulative AC pass rate: 100 % across 41 acceptance criteria. All published contracts (`region-request.md` v1.0.0, `tile-latlon.md` v1.0.0, `route-creation.md` v1.0.0, `uav-tile-upload.md` v1.2.0) are internally consistent with each other and with `error-shape.md` v1.0.0.
|
||||||
|
|
||||||
|
### Phase 3: Code Quality (cumulative)
|
||||||
|
|
||||||
|
**Validator file inventory** (cycle-8 additions):
|
||||||
|
|
||||||
|
| File | Lines | RuleFor count | Cross-field rules | Status |
|
||||||
|
|------|-------|---------------|-------------------|--------|
|
||||||
|
| `RegionRequestValidator.cs` | ~45 | 6 | 0 | Clean, SRP |
|
||||||
|
| `GetTileByLatLonQueryValidator.cs` | ~30 | 3 | 0 | Clean, SRP |
|
||||||
|
| `RejectUnknownQueryParamsEndpointFilter.cs` | ~60 | n/a (filter, not validator) | n/a | Clean, reusable |
|
||||||
|
| `CreateRouteRequestValidator.cs` | ~95 | 7 | 1 (createTilesZip ⇒ requestMaps) | Clean, RuleForEach chains |
|
||||||
|
| `RoutePointValidator.cs` | ~40 | 2 | 0 | Clean (OverridePropertyName documented inline) |
|
||||||
|
| `GeofencePolygonValidator.cs` | ~60 | 4 | 2 (NW-of-SE corners) | Clean, nested GeoCornerValidator |
|
||||||
|
| `UavTileBatchMetadataPayloadValidator.cs` | ~50 | 3 + RuleForEach | 0 | Clean, SRP |
|
||||||
|
| `UavTileMetadataValidator.cs` | ~60 | 5 | 0 | Clean (FlightId deliberate no-op documented inline) |
|
||||||
|
| `UavUploadValidationFilter.cs` | ~120 | n/a (filter) | 1 (items.Count == files.Count) | Clean, SRP (parse → validate → cross-field) |
|
||||||
|
|
||||||
|
**Consistency observations**:
|
||||||
|
|
||||||
|
- All validators follow the cycle-7 pattern: file-private class, `AbstractValidator<T>`, `RuleFor` chains, `WithMessage(...)` carrying user-friendly text. Per-item `RuleForEach` uses `SetValidator(new ChildValidator(...))` consistently.
|
||||||
|
- `[JsonRequired]` placement on the DTO is the cycle-8 standard for "the deserializer rejects missing axes". Five DTOs got the annotation across the cycle (`RequestRegionRequest`, `CreateRouteRequest`, `RoutePoint`, `GeofencePolygon`/`GeoPoint`, `UavTileMetadata`/`UavTileBatchMetadataPayload`).
|
||||||
|
- `ArgumentNullException.ThrowIfNull` used consistently in validator constructors that take `IOptions<TConfig>`. Test fixtures supply test-only `Microsoft.Extensions.Options.Options.Create(new TConfig{...})`.
|
||||||
|
- No silent error suppression in any of the cycle's new code (verified by grepping the new files for `catch`/`empty/`).
|
||||||
|
- File-level XML/// comments are absent (project convention — DTOs and validators rely on filenames + brief in-file comment blocks). Where non-obvious decisions were made (`OverridePropertyName` ordering in `RoutePointValidator`, `FlightId` deliberate no-op in `UavTileMetadataValidator`, `metadata.` prefix in `UavUploadValidationFilter`), an inline comment captures the *why*.
|
||||||
|
|
||||||
|
### Phase 4: Security Quick-Scan (cumulative)
|
||||||
|
|
||||||
|
Cycle 8 is fundamentally a security cycle: it tightens every endpoint's input validation. Threat-model deltas:
|
||||||
|
|
||||||
|
- **Attack surface reduced**: Every public endpoint now rejects unknown fields, type mismatches, and out-of-range values BEFORE the handler runs. `UnmappedMemberHandling.Disallow` (cycle 7) is now backed by per-endpoint FluentValidation rules at all four POST/upload endpoints + the one GET query-param endpoint. Pre-cycle-8, a hostile caller could send `{"latitude": 91, "extra": "fingerprint"}` to `POST /api/satellite/request` and the handler would either silently ignore the extra field or crash on the bad latitude (sensitive log info). Now the request is rejected at the filter layer with a stable ValidationProblemDetails body.
|
||||||
|
- **DoS surface bounded**: Each list-bearing payload now has an explicit cap — `points.Count <= 500` (route), `items.Count <= 100` (UAV upload), `coords.Count <= 1000` (tile inventory, cycle 7). Multipart body size still bounded by Kestrel's `MaxRequestBodySize`.
|
||||||
|
- **Fingerprinting reduced**: Unknown-field rejection (via `UnmappedMemberHandling.Disallow`) prevents attackers from probing for hidden fields. Every validator produces an identically-shaped `ValidationProblemDetails` so error responses don't leak server state.
|
||||||
|
- **Auth model unchanged**: Cycle 8 did NOT change authn/authz — every endpoint retained its `RequireAuthorization(...)` chain. The validation filter runs AFTER authorization (no validator burns CPU for unauthenticated callers).
|
||||||
|
- **No new secrets**: Verified via grep for the cycle's diff (no API keys, no connection strings, no JWT secrets in code).
|
||||||
|
- **No new PII in logs**: Validators don't log payload contents. Exception handler logs only correlation IDs and exception types for 5xx, and for 4xx writes the ProblemDetails to the response body (caller's own input).
|
||||||
|
|
||||||
|
Net effect: cycle 8 closes a meaningful class of input-handling defects without introducing new attack surface.
|
||||||
|
|
||||||
|
### Phase 5: Performance Scan (cumulative)
|
||||||
|
|
||||||
|
- Per-request overhead: each validator runs in microseconds (in-memory rule checks against record fields). Worst case is `CreateRouteRequest` with `points.Count = 500` × per-point validator = ~1 ms estimated. UAV upload at `items.Count = 100` × per-item validator = ~200 µs. Neither approaches the cost of the downstream DB ops or tile downloads.
|
||||||
|
- Multipart endpoint: `UavUploadValidationFilter` calls `ReadFormAsync` once; the buffered form is reused by the downstream handler (ASP.NET caches `IFormCollection` on the request). Net cost: zero extra IO.
|
||||||
|
- No N+1, no blocking I/O, no synchronous DB calls in any validator.
|
||||||
|
- Pre-existing performance harness (`scripts/run-performance-tests.sh` PT-01..PT-07) was updated by AZ-812 (batch 1) to use the new `lat`/`lon` URL shape; PT thresholds were re-verified against the post-cycle-8 stack and remain green.
|
||||||
|
|
||||||
|
### Phase 6: Cross-Task Consistency (cumulative)
|
||||||
|
|
||||||
|
- **ProblemDetails / ValidationProblemDetails shape**: every cycle-8 endpoint produces the same RFC 7807 body per `error-shape.md` v1.0.0 — verified by both `ProblemDetailsAssertions.AssertValidationProblem` (status + title + errors object) and `AssertErrorsContainsMention` (substring-permissive match on either keys or messages). The shared helper was promoted to `ProblemDetailsAssertions.cs` in batch 2; batches 3 + 4 consume it without re-deriving local copies.
|
||||||
|
- **Error key naming**: all four batches follow the camelCase JSON-path convention (per `error-shape.md` Inv-4). Nested collections use indexed paths (`items[0].latitude`, `points[1].lon`, `geofences.polygons[0].northWest`). Where FluentValidation's default key would diverge from the wire (e.g. `Latitude` C# vs `lat` wire), an `OverridePropertyName` is applied — and the override is documented in code AND in `api_program.md` so a future reader cannot remove it by accident.
|
||||||
|
- **Cross-task collision check**: No two validators share a class name. No two `MapPost` chains accidentally apply the same filter twice. No two contract docs reference each other circularly. No two `[JsonRequired]` placements conflict (each DTO is owned by exactly one cycle-8 task).
|
||||||
|
- **Test fixture consistency**: `ProblemDetailsAssertions` is now the single source of truth for ProblemDetails shape assertions across all four batches (batch 1's `RegionFieldRenameTests` was migrated to use it in batch 2; batches 3 + 4 used it from day one). `JwtTestHelpers` (cycle 3) was unchanged.
|
||||||
|
- **Contract version coherence**: `region-request.md` v1.0.0, `tile-latlon.md` v1.0.0, `route-creation.md` v1.0.0, `uav-tile-upload.md` v1.2.0 — all reference `error-shape.md` v1.0.0. The version-bump on UAV upload (vs the v1.0.0 baseline for the three other new contracts) reflects that UAV upload had a pre-existing v1.1.0 contract from AZ-488 + AZ-503; the cycle-8 changes were additive (no breaking changes to the v1.1.0 shape).
|
||||||
|
|
||||||
|
### Phase 7: Architecture Compliance (cumulative)
|
||||||
|
|
||||||
|
- **Layer direction**: No cross-component dependencies added or removed. New validators + filters live in `SatelliteProvider.Api/Validators/` (Layer 4 = WebApi). New `[JsonRequired]` attributes touch DTOs in `SatelliteProvider.Common/DTO/` (Layer 0 = Common). `SatelliteProvider.Common` does not depend on FluentValidation — the attribute is `System.Text.Json.Serialization.JsonRequiredAttribute`, no new package reference needed.
|
||||||
|
- **Public API respect**: No internal symbols newly exposed. DTOs were already public (cycle-2 + cycle-5 + cycle-6 work). Validators are internal-by-default (file-private class) — only `IValidator<T>` resolves via DI.
|
||||||
|
- **No cycles**: dependency graph for the cycle-8 work:
|
||||||
|
- `SatelliteProvider.Common` → (FluentValidation? NO — only `System.Text.Json.Serialization`)
|
||||||
|
- `SatelliteProvider.Api/Validators/*` → (`FluentValidation`, `Microsoft.Extensions.Options`, `Common.DTO`, `Common.Configs`) — no cycle.
|
||||||
|
- `SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs` → (`FluentValidation`, `Microsoft.AspNetCore.Http`, `Common.DTO`) — no cycle.
|
||||||
|
- **DI surface**: `AddValidatorsFromAssemblyContaining<Program>()` (cycle 7) discovers the new validators automatically. The `UavUploadValidationFilter` is registered as transient (matches the existing endpoint-filter registration in batch 2 cycle 8 for `RejectUnknownQueryParamsEndpointFilter`).
|
||||||
|
- **Documentation alignment**: `_docs/02_document/modules/api_program.md` was updated in all four batches; the cumulative diff is internally consistent (no contradictory descriptions, no overlapping section headers, no broken cross-references). `_docs/02_document/contracts/api/` gained three new files (`region-request.md`, `tile-latlon.md`, `route-creation.md`) and one bumped file (`uav-tile-upload.md`). `_docs/02_document/system-flows.md` F1/F2/F4 were updated to reflect the validator filter step.
|
||||||
|
- **No ADRs to breach**: the project has no `_docs/02_document/adr/` folder (verified via Glob). Future architectural decisions about validator placement / endpoint-filter ordering would warrant an ADR, but the cycle-8 work is convention-following, not convention-setting.
|
||||||
|
|
||||||
|
## Baseline Delta (cumulative)
|
||||||
|
|
||||||
|
| Class | Count | Notes |
|
||||||
|
|-------|-------|-------|
|
||||||
|
| Carried over | 0 | Cycle-7 retro had no Architecture-class entries to carry; cycle-1 baseline empty |
|
||||||
|
| Resolved | 0 | None — cycle 8 is strictly additive |
|
||||||
|
| Newly introduced | 1 | F1 in batch 4: `FixedTimeProvider` duplication has crossed the cycle-2-advisory "promote to shared" threshold (3+ consumers). Tracked as a Low-priority follow-up PBI. |
|
||||||
|
|
||||||
|
## Cumulative Findings (new this cycle)
|
||||||
|
|
||||||
|
Per-batch findings are listed in their respective `reviews/batch_NN_cycle8_review.md` files. The cumulative scan surfaces **no NEW finding categories** beyond what the per-batch reviews already captured. The cumulative-only observations are:
|
||||||
|
|
||||||
|
1. **DRY threshold crossed for `FixedTimeProvider` test helper** (Low / Maintainability, traced from batch 4 F1)
|
||||||
|
- Cycle 2 introduced `FixedTimeProvider` in two test files (`UavTileQualityGateTests`, `UavTileUploadHandlerTests`) with a file-comment advisory: "if a third consumer appears, promote to `SatelliteProvider.TestSupport`."
|
||||||
|
- Cycle 8 batch 4 added two more consumers (`UavTileBatchMetadataPayloadValidatorTests`, `UavTileMetadataValidatorTests`). Total = 4.
|
||||||
|
- Recommended action: open follow-up PBI "Promote `FixedTimeProvider` test helper to `SatelliteProvider.TestSupport`" (≈1 SP, mechanical).
|
||||||
|
|
||||||
|
2. **`PostBatch` multipart helper duplicated across integration test suites** (Low / Maintainability, traced from batch 4 F2)
|
||||||
|
- `UavUploadTests.cs` (cycle 2) and `UavUploadValidationTests.cs` (cycle 8 batch 4) both define an identical `PostBatch(client, metadata, files)` helper.
|
||||||
|
- Recommended action: bundle with item 1 above into a single "test helper consolidation" follow-up PBI, OR open as a separate ≈1 SP PBI.
|
||||||
|
|
||||||
|
3. **Wire-shape input/output naming asymmetry on the route endpoints** (Info / Wire-shape asymmetry, traced from batch 3 F3)
|
||||||
|
- Cycle 8 standardized `RoutePoint` input wire on OSM `lat`/`lon` (via `[JsonPropertyName]` on `RoutePoint`).
|
||||||
|
- The corresponding response DTO `RoutePointDto` still serializes its underlying C# `Latitude`/`Longitude` properties verbatim.
|
||||||
|
- This asymmetry is pre-existing; AZ-809 documented it in `route-creation.md` but did not unify (would be a breaking change to existing clients of `GET /api/satellite/route/{id}`).
|
||||||
|
- Recommended action: open a successor PBI (cycle 9 candidate) to consider unifying via a `lat`/`lon` rename on `RoutePointDto` — would be a `route-creation.md` v2.0.0 + a corresponding integration-test migration. Coordinate with any external consumer of the GET response.
|
||||||
|
|
||||||
|
4. **Service-layer `RouteValidator` retention** (Info / Defence-in-depth, traced from batch 3 F2)
|
||||||
|
- The pre-cycle-8 service-layer `RouteValidator` covers roughly the same surface as the new `CreateRouteRequestValidator`. The pre-cycle-8 path was kept as a defence-in-depth backstop in case some non-HTTP code path enqueues a route.
|
||||||
|
- Recommended action: defer to a follow-up PBI (cycle 9 candidate). Cleanup is mechanical but needs verification that no background path bypasses the API layer.
|
||||||
|
|
||||||
|
5. **Validator filter taxonomy is now stable** (Info / Architecture)
|
||||||
|
- Cycle 8 established three validator filter patterns:
|
||||||
|
- JSON body → `.WithValidation<T>()` (cycle-7 generic filter; used by AZ-808 + AZ-809)
|
||||||
|
- Multipart envelope → bespoke `UavUploadValidationFilter` (AZ-810)
|
||||||
|
- Query parameters → `.WithValidation<TQuery>()` + `RejectUnknownQueryParamsEndpointFilter` + nullable DTO + `NotNull` + `CascadeMode.Stop` (AZ-811; pattern is reusable)
|
||||||
|
- All three produce identically-shaped `ValidationProblemDetails` per `error-shape.md` v1.0.0.
|
||||||
|
- Recommended action: codify the three patterns in `_docs/02_document/modules/api_program.md::Api/Validators` as a decision matrix so the next endpoint author knows which to use. (Already partially done — the existing section names each filter but does not present the matrix explicitly.)
|
||||||
|
|
||||||
|
## Recurring patterns to surface for cycle-8 retrospective
|
||||||
|
|
||||||
|
1. **The "publish a v1.0.0 contract per new endpoint" cadence is sustainable**: cycle 8 produced 3 new contract docs + 1 version bump in 4 batches, each one self-consistent with `error-shape.md` v1.0.0 and cross-referenced from the validator file. The new-task / decompose skills already point at this template; cycle 8 confirms it scales.
|
||||||
|
2. **`[JsonRequired]` + `UnmappedMemberHandling.Disallow` + FluentValidation is the canonical pattern**: every cycle-8 endpoint uses the three layers (deserializer rejects missing/unknown axes, FluentValidation rejects business-rule violations). Worth a one-paragraph entry in `_docs/02_document/architecture.md` so the pattern is discoverable by the next contributor.
|
||||||
|
3. **Probe scripts have proven valuable** as an out-of-process verification check during validator development: batches 02, 03, 04 each shipped a `probe_<endpoint>_validation.sh` script that exercises every failure mode via `curl`. Several cycle-8 mid-batch fixes (AZ-811 binder short-circuit, AZ-809 `OverridePropertyName` discovery) were found via probe scripts before the integration tests caught them.
|
||||||
|
4. **Mid-batch root-cause investigations were captured in the per-batch reports**: batch 2 (AZ-811 binder short-circuit) and batch 3 (`OverridePropertyName` quirk) both carry detailed "Auto-Fix Attempts" sections explaining the failure mode, the diagnostic step, and the fix. This is the pattern `coderule.mdc` "Debugging Over Contemplation" calls for — worth normalizing in the implement skill's batch-report template.
|
||||||
|
|
||||||
|
## Verdict Logic
|
||||||
|
|
||||||
|
- 0 Critical, 0 High, 0 Medium.
|
||||||
|
- 4 Low findings across the 4 batches (1 in batch 1, 0 in batch 2, 1 in batch 3, 2 in batch 4) — all surfaced as per-batch findings; cumulative scan found NO new categories beyond what each batch review already captured.
|
||||||
|
- 4 Info findings — all are pre-existing or design-decision items, all documented, all with clear follow-up PBI candidates.
|
||||||
|
- → **PASS_WITH_WARNINGS**.
|
||||||
|
|
||||||
|
## Recommendation to /implement
|
||||||
|
|
||||||
|
Cumulative review passes. All four batches of cycle 8 are accepted. **Cycle 8 implementation phase is complete** — implement skill should:
|
||||||
|
|
||||||
|
1. Commit batch 4 (AZ-810).
|
||||||
|
2. Transition AZ-810 → In Testing in tracker.
|
||||||
|
3. Archive AZ-810's task spec to `_docs/02_tasks/done/`.
|
||||||
|
4. Hand back to autodev orchestrator for Step 11 (Run Tests), which will run the full integration suite to ratify cycle 8 end-to-end before the cycle's implementation report is sealed.
|
||||||
|
|
||||||
|
Follow-up PBIs surfaced by this cumulative review (not blocking cycle-8 closure):
|
||||||
|
|
||||||
|
- (Low, ~1 SP) Promote `FixedTimeProvider` test helper to `SatelliteProvider.TestSupport`.
|
||||||
|
- (Low, ~1 SP) Promote `PostBatch` multipart helper to a shared `UavUploadMultipartFixture`.
|
||||||
|
- (Info, ~2 SP) Codify validator-filter decision matrix in `_docs/02_document/modules/api_program.md::Api/Validators`.
|
||||||
|
- (Info, ~3 SP — coordination required) Unify response-side `RoutePointDto` to use `lat`/`lon` wire keys (v2.0.0 of `route-creation.md`).
|
||||||
|
- (Info, ~2 SP) Decide whether to retire service-layer `RouteValidator` now that the API layer strictly validates.
|
||||||
@@ -0,0 +1,152 @@
|
|||||||
|
# Product Implementation Completeness Gate — Cycle 8
|
||||||
|
|
||||||
|
**Cycle**: 8
|
||||||
|
**Date**: 2026-05-23
|
||||||
|
**Scope**: AZ-812, AZ-808, AZ-811, AZ-809, AZ-810 (4 batches; cycle theme: strict input validation at every public API endpoint)
|
||||||
|
|
||||||
|
## Inputs Reviewed
|
||||||
|
|
||||||
|
- `_docs/02_tasks/done/AZ-812_region_field_rename_to_osm.md`
|
||||||
|
- `_docs/02_tasks/done/AZ-808_region_endpoint_validation.md`
|
||||||
|
- `_docs/02_tasks/done/AZ-811_latlon_get_endpoint_validation.md`
|
||||||
|
- `_docs/02_tasks/done/AZ-809_route_endpoint_validation.md`
|
||||||
|
- `_docs/02_tasks/done/AZ-810_upload_metadata_validation.md`
|
||||||
|
- `_docs/02_document/architecture.md`
|
||||||
|
- `_docs/02_document/system-flows.md`
|
||||||
|
- `_docs/02_document/module-layout.md`
|
||||||
|
- `_docs/02_document/modules/api_program.md`
|
||||||
|
- `_docs/02_document/contracts/api/region-request.md` v1.0.0 (this cycle)
|
||||||
|
- `_docs/02_document/contracts/api/tile-latlon.md` v1.0.0 (this cycle)
|
||||||
|
- `_docs/02_document/contracts/api/route-creation.md` v1.0.0 (this cycle)
|
||||||
|
- `_docs/02_document/contracts/api/uav-tile-upload.md` v1.2.0 (this cycle)
|
||||||
|
- `_docs/02_document/contracts/api/error-shape.md` v1.0.0 (cycle 7)
|
||||||
|
- `_docs/03_implementation/batch_0{1,2,3,4}_cycle8_report.md`
|
||||||
|
- `_docs/03_implementation/reviews/batch_0{1,2,3,4}_cycle8_review.md`
|
||||||
|
- `_docs/03_implementation/cumulative_review_batches_01-04_cycle8_report.md`
|
||||||
|
- Source code under each task's ownership envelope (`SatelliteProvider.Api/Validators/*`, `SatelliteProvider.Api/Program.cs`, `SatelliteProvider.Common/DTO/{RequestRegionRequest, CreateRouteRequest, RoutePoint, GeofencePolygon, GeoPoint, UavTileMetadata}.cs`, `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs`)
|
||||||
|
|
||||||
|
## Per-Task Classification
|
||||||
|
|
||||||
|
### AZ-812 — Region API field rename (Latitude/Longitude → Lat/Lon, OSM convention)
|
||||||
|
|
||||||
|
**Verdict**: PASS
|
||||||
|
|
||||||
|
Evidence (source code, not tests or reports):
|
||||||
|
|
||||||
|
- **`SatelliteProvider.Common/DTO/RequestRegionRequest.cs`** — record properties renamed from `Latitude`/`Longitude` to `Lat`/`Lon`. `[JsonPropertyName("lat")]` and `[JsonPropertyName("lon")]` attributes attached so the wire format is exactly `{"lat":..,"lon":..}`. Verified at the source.
|
||||||
|
- **`SatelliteProvider.Api/Program.cs::RequestRegion` handler** — accesses `req.Lat`/`req.Lon` instead of the pre-cycle-8 `Latitude`/`Longitude`. Verified by grep.
|
||||||
|
- **`scripts/run-performance-tests.sh`** — PT-03/04/05/07 JSON bodies use `{"lat":..,"lon":..}` after the rename.
|
||||||
|
|
||||||
|
Search for unresolved markers in modified source: no `placeholder` / `TODO` / `NotImplemented` / `scaffold` / `fake` matches.
|
||||||
|
|
||||||
|
End-to-end production pipeline check: `POST /api/satellite/request` accepts `{"lat":..,"lon":..}`, deserializes to `RequestRegionRequest`, handler reads `req.Lat`/`req.Lon`, downstream `IRegionService` + `IRegionRequestQueue` enqueues + returns the region ID. The legacy `{"latitude":..,"longitude":..}` shape is rejected at the deserializer level via `UnmappedMemberHandling.Disallow` (cycle 7). No mocks, no scaffolded fallbacks.
|
||||||
|
|
||||||
|
### AZ-808 — Region POST strict validation
|
||||||
|
|
||||||
|
**Verdict**: PASS
|
||||||
|
|
||||||
|
Evidence (source code, not tests or reports):
|
||||||
|
|
||||||
|
- **`SatelliteProvider.Api/Validators/RegionRequestValidator.cs`** — FluentValidation `AbstractValidator<RequestRegionRequest>` with 6 rules: `Id` non-empty, `Lat` ∈ [-90, 90], `Lon` ∈ [-180, 180], `SizeMeters` ∈ [100, 10000], `ZoomLevel` ∈ [0, 22], `StitchTiles` is bool (handled via `[JsonRequired]`).
|
||||||
|
- **`SatelliteProvider.Common/DTO/RequestRegionRequest.cs`** — `[JsonRequired]` on `Id`, `Lat`, `Lon`, `SizeMeters`, `ZoomLevel`, `StitchTiles` (verified via earlier session reads).
|
||||||
|
- **`SatelliteProvider.Api/Program.cs:252`** — `.WithValidation<RequestRegionRequest>()` chained onto the `MapPost("/api/satellite/request", ...)` endpoint. Verified via Grep.
|
||||||
|
|
||||||
|
Search for unresolved markers: no matches in `RegionRequestValidator.cs`.
|
||||||
|
|
||||||
|
End-to-end production pipeline check: any invalid `POST /api/satellite/request` (out-of-range, missing field, unknown field, type mismatch) is rejected before the handler runs — the request never reaches `IRegionRequestQueue.EnqueueAsync` or any database operation. ValidationProblemDetails (RFC 7807) returned per `error-shape.md` v1.0.0.
|
||||||
|
|
||||||
|
### AZ-811 — lat/lon GET endpoint strict validation
|
||||||
|
|
||||||
|
**Verdict**: PASS
|
||||||
|
|
||||||
|
Evidence (source code, not tests or reports):
|
||||||
|
|
||||||
|
- **`SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs`** — nullable record (`double? Lat`, `double? Lon`, `int? Zoom`) so missing values surface as null rather than the default-zero coercion the binder would otherwise apply. Required so the validator's `NotNull` rule can fire (instead of `NotNull` being shadowed by the default value).
|
||||||
|
- **`SatelliteProvider.Api/Validators/GetTileByLatLonQueryValidator.cs`** — `CascadeMode.Stop` + `NotNull` + range checks for `Lat`/`Lon`/`Zoom`.
|
||||||
|
- **`SatelliteProvider.Api/Validators/RejectUnknownQueryParamsEndpointFilter.cs`** — reusable filter that compares the request's query keys against an allow-list (`[lat, lon, zoom]`) and rejects unknown keys with the same `ValidationProblemDetails` shape.
|
||||||
|
- **`SatelliteProvider.Api/Program.cs:212-218`** — `MapGet("/api/satellite/tiles/latlon", ...)` chain wires `.AddEndpointFilter(new RejectUnknownQueryParamsEndpointFilter(new[] { "lat", "lon", "zoom" }))` + `.WithValidation<GetTileByLatLonQuery>()` + `.Produces<DownloadTileResponse>(200)` + `.ProducesProblem(400)`. Verified via Grep.
|
||||||
|
- **`SatelliteProvider.Api/Swagger/ParameterDescriptionFilter.cs`** — describes the `lat`/`lon`/`zoom` query parameters in OpenAPI (post-rename).
|
||||||
|
|
||||||
|
Search for unresolved markers: no matches.
|
||||||
|
|
||||||
|
End-to-end production pipeline check: `GET /api/satellite/tiles/latlon?lat=...&lon=...&zoom=...` either (a) reaches the handler with non-null nullable values (validator passed) and the `.Value` deref drives `ITileService.DownloadTileAsync`, OR (b) is rejected at the filter chain with HTTP 400 + ValidationProblemDetails. No silent default-zero coercion. No mocks on the success path.
|
||||||
|
|
||||||
|
### AZ-809 — Route POST strict validation
|
||||||
|
|
||||||
|
**Verdict**: PASS
|
||||||
|
|
||||||
|
Evidence (source code, not tests or reports):
|
||||||
|
|
||||||
|
- **`SatelliteProvider.Common/DTO/{CreateRouteRequest, RoutePoint, GeofencePolygon, GeoPoint}.cs`** — `[JsonRequired]` annotations added to every non-optional axis. `RoutePoint` carries `[JsonPropertyName("lat")]`/`[JsonPropertyName("lon")]` for the OSM input wire.
|
||||||
|
- **`SatelliteProvider.Api/Validators/CreateRouteRequestValidator.cs`** — 7 root rules (id non-empty + 4 range rules on `regionSizeMeters`/`zoomLevel` + `points` count + cross-field `createTilesZip ⇒ requestMaps`) + `RuleForEach(req => req.Points).SetValidator(new RoutePointValidator())` + `RuleForEach(req => req.Geofences!.Polygons).SetValidator(new GeofencePolygonValidator()).OverridePropertyName("geofences.polygons")`. The `OverridePropertyName` on the deep expression is documented inline because FluentValidation drops the parent path otherwise.
|
||||||
|
- **`SatelliteProvider.Api/Validators/RoutePointValidator.cs`** — `OverridePropertyName("lat"/"lon")` chained after each range rule so error keys match the wire format.
|
||||||
|
- **`SatelliteProvider.Api/Validators/GeofencePolygonValidator.cs`** — nested `GeoCornerValidator` (file-private) + cross-field NW-of-SE invariants on `Lat` (NW.Lat > SE.Lat) and `Lon` (NW.Lon < SE.Lon).
|
||||||
|
- **`SatelliteProvider.Api/Program.cs:268`** — `.WithValidation<CreateRouteRequest>()` chained onto the `MapPost("/api/satellite/route", ...)` endpoint. Verified via Grep.
|
||||||
|
|
||||||
|
Search for unresolved markers: no matches.
|
||||||
|
|
||||||
|
End-to-end production pipeline check: any invalid `POST /api/satellite/route` is rejected before the handler runs. The handler delegates to `IRouteService.CreateRouteAsync` which (a) persists the route, (b) computes intermediate points via `GeoUtils.Interpolate`, (c) enqueues region requests if `requestMaps=true`. The validator runs strictly upstream of all three. The cross-field `NW.Lat > SE.Lat` rule prevents NaN-geometry payloads from reaching the interpolator. The pre-cycle-8 service-layer `RouteValidator` remains as a defence-in-depth backstop (documented in `route-creation.md` Validator Cleanup Advisory).
|
||||||
|
|
||||||
|
### AZ-810 — UAV upload metadata strict validation (multipart envelope)
|
||||||
|
|
||||||
|
**Verdict**: PASS
|
||||||
|
|
||||||
|
Evidence (source code, not tests or reports):
|
||||||
|
|
||||||
|
- **`SatelliteProvider.Common/DTO/UavTileMetadata.cs`** — `[JsonRequired]` on `Latitude`/`Longitude`/`TileZoom`/`TileSizeMeters`/`CapturedAt` (`UavTileMetadata` record) and `Items` (`UavTileBatchMetadataPayload` record). `FlightId` deliberately stays nullable per AZ-503 anonymous-flight semantics; file-comment block documents the AZ-810 rationale.
|
||||||
|
- **`SatelliteProvider.Api/Validators/UavTileBatchMetadataPayloadValidator.cs`** — root validator: `Items` NotNull + NotEmpty + `Must(<= MaxBatchSize)` + `RuleForEach(p => p.Items).SetValidator(new UavTileMetadataValidator(qualityConfig, timeProvider))`. TimeProvider is threaded through to the per-item validator.
|
||||||
|
- **`SatelliteProvider.Api/Validators/UavTileMetadataValidator.cs`** — per-item validator: lat ∈ [-90, 90], lon ∈ [-180, 180], tileZoom ∈ [0, 22], tileSizeMeters > 0, capturedAt within `[now - MaxAgeDays, now + CapturedAtFutureSkewSeconds]`. `FlightId` intentionally not validated beyond JSON shape (rationale documented inline).
|
||||||
|
- **`SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs`** — `IEndpointFilter` for the multipart endpoint. Reads the `metadata` form field, deserializes with the strict global `JsonSerializerOptions` (so `UnmappedMemberHandling.Disallow` applies), runs `IValidator<UavTileBatchMetadataPayload>`, then enforces `items.Count == files.Count`. FluentValidation errors prefixed with `metadata.` so wire key is `metadata.items[0].latitude` (full path).
|
||||||
|
- **`SatelliteProvider.Api/Program.cs:128 + 239`** — `builder.Services.AddTransient<UavUploadValidationFilter>()` (line 128, transient lifetime: fresh instance per request; no shared mutable state to amortize) + `.AddEndpointFilter<UavUploadValidationFilter>()` (line 239 in the `MapPost("/api/satellite/upload", ...)` chain) + `.Accepts<UavTileBatchUploadRequest>("multipart/form-data")` + `.Produces<UavTileBatchUploadResponse>(200)` + `.ProducesProblem(400)`. Verified via Grep.
|
||||||
|
|
||||||
|
Search for unresolved markers: no matches.
|
||||||
|
|
||||||
|
End-to-end production pipeline check: any invalid `POST /api/satellite/upload` is rejected before the handler runs — the request never reaches `IUavTileUploadHandler.HandleAsync`. The downstream handler retains its own envelope checks as defence-in-depth (covers direct handler callers in unit tests). For valid requests, the multipart body is buffered once by `ReadFormAsync` and the cached `IFormCollection` is reused by the downstream handler (ASP.NET caches it on the request). Per-item `IUavTileQualityGate` remains the byte-level quality gate (unchanged from AZ-488).
|
||||||
|
|
||||||
|
## System Pipeline Audit
|
||||||
|
|
||||||
|
The cycle-8 work does NOT introduce new pipelines — it tightens the input validation on existing pipelines. The relevant production pipelines and their classifications:
|
||||||
|
|
||||||
|
| Pipeline | Cycle-8 touchpoint | Classification | Evidence |
|
||||||
|
|----------|-------------------|----------------|----------|
|
||||||
|
| `POST /api/satellite/request → IRegionRequestQueue → IRegionService` | AZ-808 validator + AZ-812 field rename added at the entry edge | WIRED | `Program.cs:252` (validator chain) + handler reads `req.Lat`/`req.Lon` (post-rename) |
|
||||||
|
| `GET /api/satellite/tiles/latlon → ITileService.DownloadTileAsync` | AZ-811 validator + filter added at the entry edge | WIRED | `Program.cs:212-218` (validator + filter chain) + handler `.Value` deref |
|
||||||
|
| `POST /api/satellite/tiles/inventory → ITileService.GetInventoryAsync` | Cycle 7 (`InventoryRequestValidator`) — not touched by cycle 8 | WIRED (pre-existing) | `Program.cs:227` (`.WithValidation<TileInventoryRequest>()`) |
|
||||||
|
| `POST /api/satellite/route → IRouteService.CreateRouteAsync` | AZ-809 validator chain added at the entry edge | WIRED | `Program.cs:268` (validator chain) + cross-field invariants + nested DTO chain |
|
||||||
|
| `POST /api/satellite/upload → IUavTileUploadHandler.HandleAsync` | AZ-810 multipart filter + validator added at the entry edge | WIRED | `Program.cs:128 + 239` (DI registration + endpoint chain) |
|
||||||
|
|
||||||
|
No pipeline is `PARTIALLY WIRED` or `NOT WIRED`. Every pipeline has its full validator chain in production code; the handlers are unchanged behaviorally (they retain pre-cycle-8 logic plus, where applicable, defence-in-depth backstops).
|
||||||
|
|
||||||
|
## Gate Verdict: PASS
|
||||||
|
|
||||||
|
Every promise from the 5 cycle-8 task specs is implemented as production behaviour.
|
||||||
|
|
||||||
|
- No FAIL.
|
||||||
|
- No BLOCKED.
|
||||||
|
- No PARTIALLY WIRED.
|
||||||
|
- No remediation tasks required.
|
||||||
|
- Proceed to /implement Step 16 (Final Test Run). Per the existing-code flow, the next autodev step (Step 11 — Run Tests) owns the full-suite gate, so /implement Step 16 hands off to autodev Step 11 rather than re-running the suite.
|
||||||
|
|
||||||
|
## Files / Symbols Checked
|
||||||
|
|
||||||
|
Production code:
|
||||||
|
|
||||||
|
- `SatelliteProvider.Api/Program.cs` (DI registrations + endpoint chains — lines 100-128 + 208-280)
|
||||||
|
- `SatelliteProvider.Api/Validators/RegionRequestValidator.cs` (AZ-808)
|
||||||
|
- `SatelliteProvider.Api/Validators/GetTileByLatLonQueryValidator.cs` (AZ-811)
|
||||||
|
- `SatelliteProvider.Api/Validators/RejectUnknownQueryParamsEndpointFilter.cs` (AZ-811)
|
||||||
|
- `SatelliteProvider.Api/Validators/CreateRouteRequestValidator.cs` (AZ-809)
|
||||||
|
- `SatelliteProvider.Api/Validators/RoutePointValidator.cs` (AZ-809)
|
||||||
|
- `SatelliteProvider.Api/Validators/GeofencePolygonValidator.cs` (AZ-809)
|
||||||
|
- `SatelliteProvider.Api/Validators/UavTileBatchMetadataPayloadValidator.cs` (AZ-810)
|
||||||
|
- `SatelliteProvider.Api/Validators/UavTileMetadataValidator.cs` (AZ-810)
|
||||||
|
- `SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs` (AZ-810)
|
||||||
|
- `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs` (AZ-811)
|
||||||
|
- `SatelliteProvider.Common/DTO/RequestRegionRequest.cs` (AZ-812 + AZ-808)
|
||||||
|
- `SatelliteProvider.Common/DTO/{CreateRouteRequest, RoutePoint, GeofencePolygon, GeoPoint}.cs` (AZ-809)
|
||||||
|
- `SatelliteProvider.Common/DTO/UavTileMetadata.cs` (AZ-810)
|
||||||
|
- `SatelliteProvider.Api/Swagger/ParameterDescriptionFilter.cs` (AZ-811)
|
||||||
|
|
||||||
|
Cross-task scaffold-marker search (`rg -i 'placeholder|TODO|NotImplemented|scaffold|fake'` against `SatelliteProvider.Api/Validators/`): no matches in any cycle-8 production validator. The only `return null` is in `GlobalValidatorConfig.cs:24` (cycle 7), inside the `PropertyNameResolver` callback where returning null means "use the default name policy" — that is the documented sentinel value, not a stub.
|
||||||
|
|
||||||
|
Cross-cycle architectural compliance: every cycle-8 production code addition lives in the cycle's existing ownership layer (`SatelliteProvider.Api/Validators/` for validators + filters, `SatelliteProvider.Common/DTO/` for DTOs). No public-API surface expansion in lower layers. No new cross-component dependencies.
|
||||||
@@ -0,0 +1,190 @@
|
|||||||
|
# Implementation Report — Cycle 8
|
||||||
|
|
||||||
|
**Cycle**: 8
|
||||||
|
**Date**: 2026-05-23
|
||||||
|
**Tasks shipped**: AZ-812 (batch 1), AZ-808 + AZ-811 (batch 2), AZ-809 (batch 3), AZ-810 (batch 4)
|
||||||
|
**Verdict**: PASS (Product Implementation Completeness Gate — `implementation_completeness_cycle8_report.md`)
|
||||||
|
**Code Review Verdict**: PASS_WITH_WARNINGS (4 Low across 4 batches, all DRY-in-test-helpers or design-decision documented; 0 Critical, 0 High, 0 Medium)
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
Cycle 8 completes **strict input validation across every public API endpoint** of the satellite-provider service. The cycle delivers the per-endpoint children of the AZ-795 epic ("Strict Input Validation") that cycle 7 set up the foundations for (`UnmappedMemberHandling.Disallow`, `GlobalExceptionHandler`, `ValidationEndpointFilter<T>`, `error-shape.md` v1.0.0). After cycle 8, every JSON-body, multipart-envelope, and query-parameter endpoint:
|
||||||
|
|
||||||
|
- Rejects unknown fields at the deserializer level (`UnmappedMemberHandling.Disallow` from cycle 7).
|
||||||
|
- Rejects missing required fields via `[JsonRequired]` (deserializer-layer, surfaces as `JsonException` → `GlobalExceptionHandler` → RFC 7807 `ValidationProblemDetails`).
|
||||||
|
- Rejects out-of-range / business-rule-violating values via FluentValidation (12.0.0), with errors in the same `ValidationProblemDetails` shape and consistent camelCase JSON-path error keys per `error-shape.md` v1.0.0 Inv-4.
|
||||||
|
- Documents the validation in a per-endpoint contract under `_docs/02_document/contracts/api/`.
|
||||||
|
- Ships per-validator unit tests + integration tests + curl probe scripts (3 new contract docs + 1 version bump + 4 new probe scripts + ~50 new unit/integration test methods).
|
||||||
|
|
||||||
|
The endpoint coverage table:
|
||||||
|
|
||||||
|
| Endpoint | Method | Cycle 8 batch | Validator |
|
||||||
|
|----------|--------|---------------|-----------|
|
||||||
|
| `POST /api/satellite/request` | JSON body | 02 (AZ-808) | `RegionRequestValidator` |
|
||||||
|
| `POST /api/satellite/route` | JSON body (nested DTO chain) | 03 (AZ-809) | `CreateRouteRequestValidator` + `RoutePointValidator` + `GeofencePolygonValidator` |
|
||||||
|
| `POST /api/satellite/upload` | multipart/form-data | 04 (AZ-810) | `UavTileBatchMetadataPayloadValidator` + `UavTileMetadataValidator` + bespoke `UavUploadValidationFilter` |
|
||||||
|
| `GET /api/satellite/tiles/latlon` | query params | 02 (AZ-811) | `GetTileByLatLonQueryValidator` + `RejectUnknownQueryParamsEndpointFilter` |
|
||||||
|
| `POST /api/satellite/tiles/inventory` | JSON body | (cycle 7) | `InventoryRequestValidator` (pre-existing) |
|
||||||
|
|
||||||
|
Batch 1 (AZ-812) was the prerequisite rename — every cycle-8 endpoint now uses OSM `lat`/`lon` wire keys for input coordinates (the rename closed a long-standing inconsistency with the Leaflet / Slippy Map convention).
|
||||||
|
|
||||||
|
## Batches
|
||||||
|
|
||||||
|
| Batch | Tasks | Verdict | Report | Review |
|
||||||
|
|-------|-------|---------|--------|--------|
|
||||||
|
| 01 | AZ-812 — Region API field rename (Latitude/Longitude → Lat/Lon, OSM convention) | PASS | `batch_01_cycle8_report.md` | `reviews/batch_01_cycle8_review.md` |
|
||||||
|
| 02 | AZ-808 — Region POST strict validation **+** AZ-811 — lat/lon GET strict validation | PASS_WITH_NOTES | `batch_02_cycle8_report.md` | `reviews/batch_02_cycle8_review.md` |
|
||||||
|
| 03 | AZ-809 — Route POST strict validation + nested DTO chain | PASS_WITH_NOTES | `batch_03_cycle8_report.md` | `reviews/batch_03_cycle8_review.md` |
|
||||||
|
| 04 | AZ-810 — UAV upload metadata strict validation (multipart envelope) | PASS_WITH_WARNINGS | `batch_04_cycle8_report.md` | `reviews/batch_04_cycle8_review.md` |
|
||||||
|
|
||||||
|
Cumulative cross-batch review: `cumulative_review_batches_01-04_cycle8_report.md` — PASS_WITH_WARNINGS. The cumulative scan surfaced no new finding categories beyond what each per-batch review had already captured.
|
||||||
|
|
||||||
|
## Code Changes
|
||||||
|
|
||||||
|
### Batch 1 — AZ-812 (Region API OSM field rename)
|
||||||
|
|
||||||
|
- `SatelliteProvider.Common/DTO/RequestRegionRequest.cs` — properties renamed `Latitude`/`Longitude` → `Lat`/`Lon` + `[JsonPropertyName("lat")]`/`[JsonPropertyName("lon")]` so the wire is `{"lat":..,"lon":..}`.
|
||||||
|
- `SatelliteProvider.Api/Program.cs::RequestRegion` handler — property access updated.
|
||||||
|
- `scripts/run-performance-tests.sh` — PT-03/04/05/07 JSON bodies migrated to `lat`/`lon`.
|
||||||
|
|
||||||
|
### Batch 2 — AZ-808 + AZ-811 (Region POST + lat/lon GET validators)
|
||||||
|
|
||||||
|
- **AZ-808**:
|
||||||
|
- `SatelliteProvider.Common/DTO/RequestRegionRequest.cs` — `[JsonRequired]` on every property.
|
||||||
|
- `SatelliteProvider.Api/Validators/RegionRequestValidator.cs` (NEW) — 6 rules (id non-empty + range checks on `Lat`/`Lon`/`SizeMeters`/`ZoomLevel`).
|
||||||
|
- `SatelliteProvider.Api/Program.cs` — `.WithValidation<RequestRegionRequest>()` chained onto `MapPost("/api/satellite/request", ...)`; removed inline size check.
|
||||||
|
- **AZ-811**:
|
||||||
|
- `SatelliteProvider.Api/DTOs/GetTileByLatLonQuery.cs` (NEW) — nullable record so the validator's `NotNull` rules can fire.
|
||||||
|
- `SatelliteProvider.Api/Validators/GetTileByLatLonQueryValidator.cs` (NEW) — `CascadeMode.Stop` + `NotNull` + range checks.
|
||||||
|
- `SatelliteProvider.Api/Validators/RejectUnknownQueryParamsEndpointFilter.cs` (NEW) — reusable filter that allow-lists query keys.
|
||||||
|
- `SatelliteProvider.Api/Program.cs` — `MapGet("/api/satellite/tiles/latlon", ...)` chain gets `.AddEndpointFilter(new RejectUnknownQueryParamsEndpointFilter(new[] { "lat", "lon", "zoom" }))` + `.WithValidation<GetTileByLatLonQuery>()` + handler `.Value` deref.
|
||||||
|
- `SatelliteProvider.Api/Swagger/ParameterDescriptionFilter.cs` — lat/lon/zoom descriptions (post-rename).
|
||||||
|
- **Shared**:
|
||||||
|
- `SatelliteProvider.IntegrationTests/ProblemDetailsAssertions.cs` — promoted `AssertErrorsContainsMention` to a shared helper (closes batch-1 DRY warning).
|
||||||
|
|
||||||
|
### Batch 3 — AZ-809 (Route POST validator + nested DTO chain)
|
||||||
|
|
||||||
|
- `SatelliteProvider.Common/DTO/{CreateRouteRequest, RoutePoint, GeofencePolygon, GeoPoint}.cs` — `[JsonRequired]` on every non-optional axis; removed implicit defaults; `RoutePoint` carries `[JsonPropertyName("lat")]`/`[JsonPropertyName("lon")]`.
|
||||||
|
- `SatelliteProvider.Api/Validators/CreateRouteRequestValidator.cs` (NEW) — 7 root rules + `RuleForEach(req => req.Points).SetValidator(new RoutePointValidator())` + `RuleForEach(req => req.Geofences!.Polygons).SetValidator(new GeofencePolygonValidator()).OverridePropertyName("geofences.polygons")`. The `OverridePropertyName` on the deep expression is documented inline + in `api_program.md` because FluentValidation drops the parent path otherwise.
|
||||||
|
- `SatelliteProvider.Api/Validators/RoutePointValidator.cs` (NEW) — `OverridePropertyName("lat"/"lon")` chained after each range rule so error keys match the wire format.
|
||||||
|
- `SatelliteProvider.Api/Validators/GeofencePolygonValidator.cs` (NEW) — nested `GeoCornerValidator` (file-private) + cross-field NW-of-SE invariants.
|
||||||
|
- `SatelliteProvider.Api/Program.cs` — `.WithValidation<CreateRouteRequest>()` + `.Accepts<>` + `.Produces<>` + `.ProducesProblem(400)` on `MapPost("/api/satellite/route", ...)`.
|
||||||
|
- Service-layer `RouteValidator` retained as defence-in-depth backstop; documented in `route-creation.md` Validator Cleanup Advisory.
|
||||||
|
|
||||||
|
### Batch 4 — AZ-810 (UAV upload validator + multipart filter)
|
||||||
|
|
||||||
|
- `SatelliteProvider.Common/DTO/UavTileMetadata.cs` — `[JsonRequired]` on `Latitude`/`Longitude`/`TileZoom`/`TileSizeMeters`/`CapturedAt` + `Items`. `FlightId` deliberately stays nullable per AZ-503 anonymous-flight semantics.
|
||||||
|
- `SatelliteProvider.Api/Validators/UavTileBatchMetadataPayloadValidator.cs` (NEW) — root validator: items NotNull + NotEmpty + count cap + `RuleForEach` dispatching to the per-item validator.
|
||||||
|
- `SatelliteProvider.Api/Validators/UavTileMetadataValidator.cs` (NEW) — per-item validator: lat/lon/tileZoom ranges + tileSizeMeters > 0 + capturedAt freshness window via injectable `TimeProvider`.
|
||||||
|
- `SatelliteProvider.Api/Validators/UavUploadValidationFilter.cs` (NEW) — `IEndpointFilter` that intercepts the multipart body, reads the `metadata` form field, deserializes with the strict global `JsonSerializerOptions`, runs the validator, then enforces `items.Count == files.Count`. FluentValidation errors prefixed with `metadata.` so the wire key is `metadata.items[0].latitude` (full path).
|
||||||
|
- `SatelliteProvider.Api/Program.cs` — `AddTransient<UavUploadValidationFilter>()` + `.AddEndpointFilter<UavUploadValidationFilter>()` + `.Accepts<UavTileBatchUploadRequest>("multipart/form-data")` + `.Produces<UavTileBatchUploadResponse>(200)` + `.ProducesProblem(400)` on `MapPost("/api/satellite/upload", ...)`. Transient lifetime mirrors `RejectUnknownQueryParamsEndpointFilter` (no shared mutable state to amortize).
|
||||||
|
|
||||||
|
## Test Changes
|
||||||
|
|
||||||
|
### Unit tests (`SatelliteProvider.Tests/Validators/`)
|
||||||
|
|
||||||
|
| File | Methods | Batch |
|
||||||
|
|------|---------|-------|
|
||||||
|
| `RegionRequestValidatorTests.cs` (NEW) | 11 | 02 (AZ-808) |
|
||||||
|
| `GetTileByLatLonQueryValidatorTests.cs` (NEW) | 9 | 02 (AZ-811) |
|
||||||
|
| `RejectUnknownQueryParamsEndpointFilterTests.cs` (NEW) | 4 | 02 (AZ-811) |
|
||||||
|
| `CreateRouteRequestValidatorTests.cs` (NEW) | 16 | 03 (AZ-809) |
|
||||||
|
| `RoutePointValidatorTests.cs` (NEW) | 4 | 03 (AZ-809) |
|
||||||
|
| `GeofencePolygonValidatorTests.cs` (NEW) | 6 | 03 (AZ-809) |
|
||||||
|
| `UavTileBatchMetadataPayloadValidatorTests.cs` (NEW) | 4 | 04 (AZ-810) |
|
||||||
|
| `UavTileMetadataValidatorTests.cs` (NEW) | 9 | 04 (AZ-810) |
|
||||||
|
| **Total** | **63 new unit-test methods** | |
|
||||||
|
|
||||||
|
### Integration tests (`SatelliteProvider.IntegrationTests/`)
|
||||||
|
|
||||||
|
| File | Methods | Batch |
|
||||||
|
|------|---------|-------|
|
||||||
|
| `RegionFieldRenameTests.cs` (NEW) | 2 (happy + legacy-reject) | 01 (AZ-812) |
|
||||||
|
| `RegionRequestValidationTests.cs` (NEW) | 10 | 02 (AZ-808) |
|
||||||
|
| `GetTileByLatLonValidationTests.cs` (NEW) | 8 | 02 (AZ-811) |
|
||||||
|
| `CreateRouteValidationTests.cs` (NEW) | 16 | 03 (AZ-809) |
|
||||||
|
| `UavUploadValidationTests.cs` (NEW) | 16 | 04 (AZ-810) |
|
||||||
|
| **Total** | **52 new integration-test methods** | |
|
||||||
|
|
||||||
|
`SatelliteProvider.IntegrationTests/Program.cs` was updated by every batch to wire the new test entry points into BOTH `RunSmokeSuite` and `RunFullSuite`.
|
||||||
|
|
||||||
|
### Probe scripts (`scripts/`)
|
||||||
|
|
||||||
|
| Script | Batch |
|
||||||
|
|--------|-------|
|
||||||
|
| `probe_region_validation.sh` (NEW) | 02 (AZ-808) |
|
||||||
|
| `probe_latlon_validation.sh` (NEW) | 02 (AZ-811) |
|
||||||
|
| `probe_route_validation.sh` (NEW) | 03 (AZ-809) |
|
||||||
|
| `probe_upload_validation.sh` (NEW) | 04 (AZ-810) |
|
||||||
|
|
||||||
|
Each script exercises happy + every failure mode via `curl` against a running API container; reuses a consistent JWT-mint + status-code-assertion driver structure.
|
||||||
|
|
||||||
|
## Documentation Changes
|
||||||
|
|
||||||
|
### New contracts (`_docs/02_document/contracts/api/`)
|
||||||
|
|
||||||
|
| File | Version | Batch |
|
||||||
|
|------|---------|-------|
|
||||||
|
| `region-request.md` | 1.0.0 (NEW) | 02 (AZ-808) |
|
||||||
|
| `tile-latlon.md` | 1.0.0 (NEW) | 02 (AZ-811) |
|
||||||
|
| `route-creation.md` | 1.0.0 (NEW) | 03 (AZ-809) |
|
||||||
|
| `uav-tile-upload.md` | 1.1.0 → 1.2.0 (MINOR bump) | 04 (AZ-810) |
|
||||||
|
|
||||||
|
All four reference `error-shape.md` v1.0.0 (cycle 7) for the canonical RFC 7807 body shape.
|
||||||
|
|
||||||
|
### Updated docs
|
||||||
|
|
||||||
|
- `_docs/02_document/modules/api_program.md` — endpoint descriptions, `Api/Validators` section (8 new entries), `Common/DTO` notes on `[JsonRequired]` placements, DI Registration list (3 new entries — 1 for `RejectUnknownQueryParamsEndpointFilter`, 1 for `UavUploadValidationFilter`, 1 cross-cutting `AddValidatorsFromAssemblyContaining<Program>()` re-anchored).
|
||||||
|
- `_docs/02_document/modules/common_dtos.md` — DTO descriptions updated with `[JsonRequired]` markers + constraint summaries.
|
||||||
|
- `_docs/02_document/modules/common_uuidv5.md` — example URL updated to post-rename `?lat=&lon=&zoom=`.
|
||||||
|
- `_docs/02_document/system-flows.md` — F1 (lat/lon GET) / F2 (region POST) / F4 (route POST) updated with the validator filter step + preconditions + error scenarios.
|
||||||
|
- `_docs/02_document/tests/blackbox-tests.md` — BT-01/N01/N02/06/N03/N04/N05/18 trigger + pass-criteria updates.
|
||||||
|
- `_docs/02_document/tests/security-tests.md` — SEC-01/04/05 references the validators + `GlobalExceptionHandler` JsonException branch.
|
||||||
|
- `README.md` — endpoint example URL updated to post-rename.
|
||||||
|
|
||||||
|
## AC Coverage
|
||||||
|
|
||||||
|
| AC range | Status | Test source |
|
||||||
|
|----------|--------|-------------|
|
||||||
|
| AZ-812 AC-1..AC-6 (6 ACs) | Covered | `RegionFieldRenameTests` (positive + legacy-reject) + `RegionTests` + `IdempotentPostTests` + `SecurityTests` + `scripts/run-performance-tests.sh` PT-03..PT-07. |
|
||||||
|
| AZ-808 AC-1..AC-8 (8 ACs) | Covered | `RegionRequestValidationTests` (10 methods covering happy + 9 failure modes) + `RegionRequestValidatorTests` (11 unit methods). |
|
||||||
|
| AZ-811 AC-1..AC-9 (9 ACs) | Covered | `GetTileByLatLonValidationTests` (8 methods) + `GetTileByLatLonQueryValidatorTests` (9 methods) + `RejectUnknownQueryParamsEndpointFilterTests` (4 methods). |
|
||||||
|
| AZ-809 AC-1..AC-9 (9 ACs) | Covered | `CreateRouteValidationTests` (16 methods) + `CreateRouteRequestValidatorTests` (16) + `RoutePointValidatorTests` (4) + `GeofencePolygonValidatorTests` (6). |
|
||||||
|
| AZ-810 AC-1..AC-9 (9 ACs) | Covered | `UavUploadValidationTests` (16 methods) + `UavTileBatchMetadataPayloadValidatorTests` (4) + `UavTileMetadataValidatorTests` (9). Existing AZ-488 `UavUploadTests` payloads traced against the new validator rules — all happy paths still valid (AC-9 preserved). |
|
||||||
|
| **Total** | **41/41 ACs covered.** | No deferrals, no in-scope test gaps. |
|
||||||
|
|
||||||
|
## Completeness Gate
|
||||||
|
|
||||||
|
`_docs/03_implementation/implementation_completeness_cycle8_report.md` — **PASS**. Every cycle-8 task's promises (validators, filters, endpoint chains, contract docs, [JsonRequired] placements) are implemented as production behaviour; no scaffold / placeholder / NotImplemented markers introduced; named integrations (FluentValidation 12.0.0 DI, `UnmappedMemberHandling.Disallow`, `GlobalExceptionHandler` JsonException branch, `ValidationProblemDetails`, `IEndpointFilter`) are wired against real production code paths in `SatelliteProvider.Api/Program.cs`.
|
||||||
|
|
||||||
|
## Handoff to autodev Step 11 (Run Tests)
|
||||||
|
|
||||||
|
Per `/implement` Step 16: since the next existing-code flow step is **Run Tests**, the implement skill does **not** run the full suite again. The `test-run` skill owns the full-suite gate to avoid duplicate runs.
|
||||||
|
|
||||||
|
Recommendation for `test-run`:
|
||||||
|
|
||||||
|
- Full integration-test suite runs via `docker-compose -f docker-compose.yml -f docker-compose.tests.yml up --build --abort-on-container-exit` (per `AGENTS.md`). All four new validation test entry points (`RegionFieldRenameTests`, `RegionRequestValidationTests`, `GetTileByLatLonValidationTests`, `CreateRouteValidationTests`, `UavUploadValidationTests`) are wired into both `RunSmokeSuite` and `RunFullSuite` in `SatelliteProvider.IntegrationTests/Program.cs`.
|
||||||
|
- AZ-488 happy-path regression coverage (`UavUploadTests`) runs unchanged — verify it stays green to confirm AC-9 (no regression).
|
||||||
|
- Cycle-7 inventory tests (`TileInventoryValidationTests`, `TileInventoryTests`) run unchanged — verify they stay green to confirm cycle 8 did not regress the cycle 7 foundations.
|
||||||
|
- Performance harness (`scripts/run-performance-tests.sh`) PT-01 (lat/lon GET) + PT-03..PT-07 (region POST) URLs were updated to the post-rename wire format in batch 1; if `test-run` invokes PT, confirm the budgets remain green.
|
||||||
|
- If the DNS-flake from cycle 5/6 recurs against `mt1.google.com` / `tile.googleapis.com`, treat it as the same host-network flakiness — out of scope for cycle 8 (this cycle does not touch the Google Maps download path).
|
||||||
|
|
||||||
|
## Git
|
||||||
|
|
||||||
|
- Branch: `dev` (per `.cursor/rules/git-workflow.mdc`).
|
||||||
|
- Auto-push: NOT enabled this session — per project convention, commit will happen here; user will be asked about push.
|
||||||
|
- Commits (planned subject lines, per the git-workflow rule's `[TRACKER-ID] Summary` format):
|
||||||
|
- `[AZ-810] Strict validation for POST /api/satellite/upload metadata` (batch 4)
|
||||||
|
- (Batches 1-3 already committed individually in their respective autodev runs.)
|
||||||
|
|
||||||
|
## Follow-up PBIs Surfaced by the Cumulative Review
|
||||||
|
|
||||||
|
These are not blocking cycle-8 closure; they emerged from the cumulative scan as candidates for cycle 9 or later:
|
||||||
|
|
||||||
|
| ID candidate | Title | SP | Rationale |
|
||||||
|
|--------------|-------|----|-----------|
|
||||||
|
| (open) | Promote `FixedTimeProvider` test helper to `SatelliteProvider.TestSupport` | 1 | The cycle-2 advisory ("if a 3rd consumer appears, promote") was crossed by batch 4; now duplicated across 4 test files. |
|
||||||
|
| (open) | Promote `PostBatch` multipart helper to a shared `UavUploadMultipartFixture` | 1 | Duplicated between `UavUploadTests.cs` (cycle 2) and `UavUploadValidationTests.cs` (cycle 8 batch 4). |
|
||||||
|
| (open) | Codify validator-filter decision matrix in `api_program.md::Api/Validators` | 2 | Cycle 8 established three validator filter patterns (JSON body, multipart, query params); document the decision matrix so the next endpoint author knows which to use. |
|
||||||
|
| (open — coordination required) | Unify response-side `RoutePointDto` to use `lat`/`lon` wire keys (v2.0.0 of `route-creation.md`) | 3 | Cycle 8 standardized input wire on OSM `lat`/`lon`; response DTO still uses `latitude`/`longitude` — breaking change for `GET /api/satellite/route/{id}` clients. |
|
||||||
|
| (open) | Decide whether to retire service-layer `RouteValidator` now that the API layer strictly validates | 2 | Currently retained as a defence-in-depth backstop; could be removed if no non-HTTP code path enqueues routes. |
|
||||||
@@ -2,13 +2,13 @@
|
|||||||
|
|
||||||
## Current Step
|
## Current Step
|
||||||
flow: existing-code
|
flow: existing-code
|
||||||
step: 10
|
step: 11
|
||||||
name: Implement
|
name: Run Tests
|
||||||
status: in_progress
|
status: not_started
|
||||||
sub_step:
|
sub_step:
|
||||||
phase: 7
|
phase: 0
|
||||||
name: batch-loop
|
name: awaiting-invocation
|
||||||
detail: "batch 3 of 4 complete (AZ-809 In Testing); next: batch 4 = AZ-810 UAV upload metadata validator"
|
detail: ""
|
||||||
retry_count: 0
|
retry_count: 0
|
||||||
cycle: 8
|
cycle: 8
|
||||||
tracker: jira
|
tracker: jira
|
||||||
|
|||||||
Reference in New Issue
Block a user