[AZ-350] Cumulative code review batches 13-15: PASS, 0 findings

Co-authored-by: Cursor <cursoragent@cursor.com>

_docs/03_implementation/cumulative_review_batches_13-15_cycle1_report.md

@@ -0,0 +1,154 @@
+# Cumulative Code Review — Batches 13-15 (cycle 1)
+
+**Window**: Batches 13 (AZ-368), 14 (AZ-369), 15 (AZ-365)
+**Trigger**: K=3 cumulative review fired after batch 15 (`/implement` Step 14.5)
+**Date**: 2026-05-11
+**Mode**: cumulative (all 7 phases)
+**Verdict**: PASS — 0 findings
+
+## 1. Scope (cumulative diff vs. previous cumulative review at batch 12)
+
+| Batch | Task | Component(s) | Net LOC |
+|-------|------|--------------|---------|
+| 13 | AZ-368 (C15 — TileCsvWriter extraction) | Common + RegionProcessing + RouteManagement | +180 / -20 |
+| 14 | AZ-369 (C16 — DTOs out of Program.cs) | Api (DTOs / Swagger sub-namespaces) + Common.DTO | +124 / -113 |
+| 15 | AZ-365 (C12 — Decompose RouteService.CreateRouteAsync) | Services.RouteManagement | +480 / -205 |
+
+**Files in cumulative window** (production + tests):
+- `SatelliteProvider.Common/Utils/TileCsvWriter.cs` (new)
+- `SatelliteProvider.Common/DTO/{DownloadTileResponse, GetSatelliteTilesResponse, RequestRegionRequest, SatelliteTile, SaveResult}.cs` (new — relocated from `Program.cs`)
+- `SatelliteProvider.Api/DTOs/UploadImageRequest.cs` (new — kept in Api due to `IFormFile` dependency)
+- `SatelliteProvider.Api/Swagger/ParameterDescriptionFilter.cs` (new — relocated from `Program.cs`)
+- `SatelliteProvider.Api/Program.cs` (modified — 113 LOC removed, host-file SRP)
+- `SatelliteProvider.Services.RegionProcessing/RegionService.cs` (modified — `GenerateCsvFileAsync` delegates to `TileCsvWriter`)
+- `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` (modified — `GenerateRouteCsvAsync` delegates to `TileCsvWriter`)
+- `SatelliteProvider.Services.RouteManagement/RouteService.cs` (modified — orchestrator, 295 → 138 LOC)
+- `SatelliteProvider.Services.RouteManagement/{RouteValidator, RoutePointGraphBuilder, GeofenceGridCalculator, RouteResponseMapper}.cs` (new)
+- `SatelliteProvider.Tests/{TileCsvWriterTests, RouteValidatorTests, RoutePointGraphBuilderTests, GeofenceGridCalculatorTests, RouteResponseMapperTests}.cs` (new — 7 + 11 + 8 + 6 + 4 = 36 new unit tests)
+
+## 2. Phase 1 — Context Loading
+
+Re-read:
+- AZ-368, AZ-369, AZ-365 task specs (`_docs/02_tasks/done/AZ-368*.md`, `done/AZ-369*.md`, `todo/AZ-365_*.md`)
+- `_docs/02_document/module-layout.md` (component boundaries)
+- `_docs/02_document/architecture.md` `## Architecture Vision`
+- `_docs/02_document/architecture_compliance_baseline.md` (5 baseline findings, all already resolved by AZ-309 and AZ-315 prior to this run)
+- `_docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md` (C15, C16, C12 entries)
+
+All three batches sit within the same epic AZ-350 / Phase 3 ("Structural cleanup"), with shared theme: extract shared/single-responsibility helpers without changing observable HTTP / DB behavior.
+
+## 3. Phase 2 — Spec Compliance (cumulative)
+
+| Task | ACs | Verification |
+|------|-----|--------------|
+| AZ-368 | AC-1 (single-source CSV writer) / AC-2 (byte-equivalent output) | Verified in batch 13 + 7 new tests + smoke run |
+| AZ-369 | AC-1 (no DTO declarations in `Program.cs`) / AC-2 (OpenAPI shape unchanged) / AC-3 (37 unit + smoke green) | Verified in batch 14 + integration run; OpenAPI shape preserved (Swashbuckle uses simple type names, namespace-agnostic) |
+| AZ-365 | AC-1 (`CreateRouteAsync` is orchestration) / AC-2 (aggregated validation) / AC-3 (same persistence + same response) / AC-4 (37 unit + 5 smoke green) | Verified in batch 15 + 28 new tests + smoke run |
+
+No spec-gap detected on any of the three tasks.
+
+## 4. Phase 3 — Code Quality (cumulative)
+
+- **SOLID**: SRP improvements across all three batches (CSV writer extracted; DTO declarations moved to data-namespace; RouteService decomposed). No SOLID regression introduced.
+- **Error handling**: helpers consistently use `ArgumentNullException.ThrowIfNull` for entry-point guards and throw typed exceptions (`ArgumentException`, `ArgumentOutOfRangeException`) with descriptive messages. No bare catches added.
+- **Naming**: every new type has an explicit, intent-revealing name (`TileCsvWriter`, `TileCsvRow`, `RouteValidator`, `RoutePointGraphBuilder`, `GeofenceGridCalculator`, `RouteResponseMapper`, `RoutePointGraph`).
+- **Complexity**: every new method is < 30 LOC. The decomposed `CreateRouteAsync` is now 52 LOC of orchestration vs. the previous 184 LOC. `RouteProcessingService.GenerateRouteCsvAsync` and `RegionService.GenerateCsvFileAsync` shrank to 2-line delegations.
+- **DRY**: positive net DRY impact. CSV writer logic existed in two places (RegionService + RouteProcessingService) → now in one (`TileCsvWriter`). Entity-to-DTO mapping in RouteService was duplicated between `CreateRouteAsync` and `GetRouteAsync` → now in one (`RouteResponseMapper.Map`).
+- **Test quality**: every helper has dedicated unit tests using FluentAssertions and the project's standard `// Arrange / // Act / // Assert` comment pattern. Tests assert behavior (not just "no throw").
+- **Dead code**: nothing left dangling. The pre-refactor private method `RouteService.CreateGeofenceRegionGrid` is removed (logic moved to `GeofenceGridCalculator`); the pre-refactor const `MAX_POINT_SPACING_METERS` is removed (moved to `RoutePointGraphBuilder.MaxPointSpacingMeters`); inline `Program.cs` types are removed (relocated). No orphan files.
+
+No code-quality finding.
+
+## 5. Phase 4 — Security Quick-Scan (cumulative)
+
+- No new SQL building, no new string-interpolated queries, no new `Process.Start` / `eval` / native invocation.
+- No new credentials or secrets — all three batches are pure refactors.
+- Validation strengthened: `RouteValidator` now aggregates errors instead of leaking the first match. The 400 path through `Program.cs CreateRoute` catch block remains the typed-`ArgumentException` exit (AZ-353 sanitized 5xx still applies for unexpected errors).
+- No sensitive data added to log lines.
+- DTO relocation in batch 14 preserves `[Required]` annotations on `RequestRegionRequest`; the AZ-353 global handler continues to translate validation failures to HTTP 400 (verified by the existing security integration test that POSTs malformed JSON — passed in the smoke run).
+
+No security finding.
+
+## 6. Phase 5 — Performance Scan (cumulative)
+
+- `TileCsvWriter` performs the same `OrderByDescending(...).ThenBy(...)` + `F6` formatting that was inline before — same complexity.
+- `RoutePointGraphBuilder` runs the same loop and Haversine math — same complexity.
+- `GeofenceGridCalculator` runs the same nested loop — same complexity.
+- `RouteResponseMapper.Map(RouteEntity, IEnumerable<RoutePointDto>)` does one `ToList()` materialization if the input is not already a `List<>` — bounded by route size, O(N), one-time at response construction.
+- No new I/O introduced, no new DB calls.
+
+No performance regression.
+
+## 7. Phase 6 — Cross-Task Consistency (cumulative — main focus of K=3 review)
+
+Style & pattern consistency across the three batches:
+
+- **Public class with explicit constructor**: TileCsvWriter / RouteValidator / RoutePointGraphBuilder / GeofenceGridCalculator / RouteResponseMapper — all `public class` with parameterless or simple constructors. The same template the project applies to RegionFailureClassifier (cycle 1 earlier) and CorsConfigurationValidator. Consistent.
+- **Test convention**: every new test file uses xUnit + FluentAssertions + the `// Arrange / // Act / // Assert` comment block. No drift.
+- **Error-handling convention**: every new public method that takes a reference parameter uses `ArgumentNullException.ThrowIfNull(...)` as the first line. Domain validation (range, ordering, etc.) throws `ArgumentException` with a single descriptive message. Consistent.
+- **DI convention**: helpers introduced in batches 13 and 15 are stateless and constructed at use sites; no new DI registrations were necessary (`RouteService` instantiates its four helpers in the 3-arg compatibility constructor; `TileCsvWriter` is constructed at each call site by the two services that use it). Consistent with the project's "DI-only-when-it-buys-something" pattern.
+- **No conflicting refactor styles**: no batch in the window introduces a heavyweight pattern (factory, mediator, generic abstract base) that the next batch then has to inherit or work around.
+- **Symbol drift**: the only cross-batch interaction is `Common/DTO/*` (batch 14) being consumed by `RouteService.cs` (batch 15) — `RouteService` already used `Common/DTO`, so no new import surface was added. All five DTOs that moved into `Common/DTO/` are still referenced by their original consumers via the same simple type name (no fully-qualified rewrites needed).
+- **No interface drift**: `IRouteService` (consumed by `Program.cs`) and `IRegionService` (consumed by `RouteService` and `RouteProcessingService`) are unchanged across the window.
+
+No cross-task consistency finding.
+
+## 8. Phase 7 — Architecture Compliance
+
+Checks applied to the cumulative window:
+
+1. **Layer direction**: every new file lives in the layer it should:
+   - `TileCsvWriter` → Layer 1 (Common — Foundation). Correct.
+   - 5 DTOs moved Program.cs → `Common/DTO/` → Layer 1. Correct.
+   - `UploadImageRequest` deliberately kept at Layer 4 (Api/DTOs) because `IFormFile` requires `Microsoft.AspNetCore.App` framework reference — moving it to Common would force that framework reference into the foundation layer, contradicting `module-layout.md`'s `Common: Imports from: (none)` invariant. Correct architectural compromise (documented in batch 14 "Scope clarification" section).
+   - `ParameterDescriptionFilter` → `Api/Swagger/` (Layer 4 — Api). Correct.
+   - 4 RouteManagement helpers → `SatelliteProvider.Services.RouteManagement/` (Layer 3 — Application). Correct.
+   - All imports respect the layering table in `module-layout.md`.
+
+2. **Public API respect**: every cross-component import in this window resolves to a Common Public API symbol (DTOs from `Common.DTO`, utils from `Common.Utils`, interfaces from `Common.Interfaces`) or a DataAccess Public API symbol (entity types from `DataAccess.Models`, repository interfaces from `DataAccess.Repositories`). No internal-file imports across components.
+
+3. **No new cyclic dependencies**: Module-graph still a DAG. The split established in AZ-309 holds:
+   - `RouteManagement → Common`, `RouteManagement → DataAccess` (via repository interfaces). No `RouteManagement → RegionProcessing` or `RouteManagement → TileDownloader` ProjectReferences exist or were added.
+   - `RegionProcessing → Common`, `RegionProcessing → DataAccess`. Same — no sibling references.
+   - The new helpers (`RouteValidator`, etc.) live inside `RouteManagement` and are not consumed by other components — they are internal collaborators of `RouteService`.
+
+4. **Duplicate symbols across components**:
+   - `RouteValidator`, `RoutePointGraphBuilder`, `GeofenceGridCalculator`, `RouteResponseMapper` — names searched across all components; unique.
+   - `TileCsvWriter` — already established in Common in batch 13; still single-source.
+   - `DownloadTileResponse`, `RequestRegionRequest`, etc. — moved into Common.DTO; the `SatelliteProvider.IntegrationTests/Models.cs` keeps its OWN local copies on purpose (no `ProjectReference` to Common; the integration tests treat these as wire-shape contracts). Documented in batch 14. This is intentional duplication for a test-only purpose and does not cross production component boundaries.
+
+5. **Cross-cutting concerns not locally re-implemented**:
+   - CSV writing (was duplicated in two services) → now lives in `Common/Utils/TileCsvWriter` (batch 13). Cross-cutting concern correctly hoisted.
+   - DTO shapes (were inline in `Program.cs`) → now in `Common/DTO/*` (batch 14). Correctly hoisted.
+   - RouteService validation/grid/mapping → kept inside `RouteManagement` (batch 15). Correct — these are RouteManagement-specific concerns, not cross-cutting.
+
+No architecture finding introduced by the window.
+
+## 9. Baseline Delta (vs. `_docs/02_document/architecture_compliance_baseline.md`)
+
+| Bucket | Count | Notes |
+|--------|-------|-------|
+| Carried over | 0 | All 5 baseline findings were resolved by epics AZ-309 + AZ-315 prior to this run. |
+| Resolved | 0 | None to resolve in this window — baseline already clean. |
+| Newly introduced | 0 | This window introduces no new architecture findings. |
+
+## 10. Verdict
+
+**Verdict**: PASS (0 findings)
+
+| Severity | Count |
+|----------|-------|
+| Critical | 0 |
+| High | 0 |
+| Medium | 0 |
+| Low | 0 |
+
+## 11. Test posture at end of window
+
+- Unit tests: 112 / 112 passing (was 84 before batch 13). +36 new tests across the three batches (TileCsvWriter ×7, RouteValidator ×11, RoutePointGraphBuilder ×8, GeofenceGridCalculator ×6, RouteResponseMapper ×4).
+- Smoke + integration tests: green, container exits 0. All pre-existing flows preserved end-to-end.
+- No skipped tests, no flaky tests in this window.
+
+## 12. Action
+
+Auto-chain to commit (batch 15 commit) per `/implement` Step 11. Resume the next batch (candidate AZ-367 / C14 — TileGridStitcher, 3 SP) afterward. K=3 counter resets; next cumulative review fires after batch 18 (window 16+17+18).