diff --git a/_docs/03_implementation/cumulative_review_batches_16-18_cycle1_report.md b/_docs/03_implementation/cumulative_review_batches_16-18_cycle1_report.md new file mode 100644 index 0000000..4b9a33e --- /dev/null +++ b/_docs/03_implementation/cumulative_review_batches_16-18_cycle1_report.md @@ -0,0 +1,195 @@ +# Cumulative Code Review — Batches 16-18 (cycle 1) + +**Window**: Batches 16 (AZ-367), 17 (AZ-364 + folds AZ-360), 18 (AZ-371) +**Trigger**: K=3 cumulative review fired after batch 18 (`/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 15) + +| Batch | Task(s) | Component(s) | Net LOC | +|-------|---------|--------------|---------| +| 16 | AZ-367 (C14 — extract TileGridStitcher) | Common.Imaging + RegionProcessing + RouteManagement | +290 / -95 | +| 17 | AZ-364 (C11 — decompose RouteProcessingService) + AZ-360 (C08 — replace IServiceProvider) | Services.RouteManagement (orchestrator + 7 collaborators) | +1177 / -435 | +| 18 | AZ-371 (C18 — magic numbers → ProcessingConfig/MapConfig + LF-2 CT) | Common.Configs + Api + Services.* (all) | +404 / -68 | + +**Files in cumulative window** (production): + +- **NEW**: + - `SatelliteProvider.Common/Imaging/TileGridStitcher.cs` (b16) + - `SatelliteProvider.Services.RouteManagement/TileInfo.cs` (b17) + - `SatelliteProvider.Services.RouteManagement/RouteRegionMatcher.cs` (b17) + - `SatelliteProvider.Services.RouteManagement/RouteCsvWriter.cs` (b17) + - `SatelliteProvider.Services.RouteManagement/RouteSummaryWriter.cs` (b17) + - `SatelliteProvider.Services.RouteManagement/RouteImageRenderer.cs` (b17, expanded in b18) + - `SatelliteProvider.Services.RouteManagement/TilesZipBuilder.cs` (b17) + - `SatelliteProvider.Services.RouteManagement/RegionFileCleaner.cs` (b17) + +- **MODIFIED** (touched by ≥1 batch in window): + - `SatelliteProvider.Common/SatelliteProvider.Common.csproj` (b16: ImageSharp NuGet) + - `SatelliteProvider.Common/Configs/{ProcessingConfig, MapConfig}.cs` (b18: +8 keys) + - `SatelliteProvider.Api/Program.cs` (b18: GetTileByLatLon HttpContext + CT) + - `SatelliteProvider.Api/appsettings.json` (b18: +8 keys) + - `SatelliteProvider.Services.RegionProcessing/RegionService.cs` (b16: stitcher delegation; b18: IOptions) + - `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs` (b16: stitcher delegation; b17: orchestrator decomposition + IServiceProvider→IRegionService; b18: IOptions) + - `SatelliteProvider.Services.RouteManagement/{RoutePointGraphBuilder, RouteValidator}.cs` (b18: IOptions) + - `SatelliteProvider.Services.RouteManagement/RouteService.cs` (b18: 3-arg → 4-arg DI ctor with IOptions) + - `SatelliteProvider.Services.RouteManagement/RouteManagementServiceCollectionExtensions.cs` (b17: +5 collaborator singletons) + - `SatelliteProvider.Services.TileDownloader/GoogleMapsDownloaderV2.cs` (b18: 4 const → MapConfig + tolerance from ProcessingConfig) + - `SatelliteProvider.Services.TileDownloader/TileService.cs` (b18: IOptions + TileSizePixels) + +- **DELETED**: + - `SatelliteProvider.Tests/RouteProcessingServiceTests.cs` (b17 — orchestrator now smoke-covered; testable bits moved to RouteImageRendererTests) + +**Test deltas in window**: + +| Batch | New tests | Notes | +|-------|-----------|-------| +| 16 | +11 | `TileGridStitcherTests` (3 byte-equality pixel-equivalence tests + 8 unit tests) | +| 17 | +10 net | New: 14 (RouteImageRendererTests×4, RouteRegionMatcherTests×4, RouteCsvWriterTests×1, RouteSummaryWriterTests×2, TilesZipBuilderTests×1, RegionFileCleanerTests×2). Moved: 4 (`ExtractTileCoordinatesFromFilename` tests). Deleted: 4 (the original location of those tests). | +| 18 | +8 | ConfigDefaultsTests×7 + TileServiceTests AZ371_AC3 ×1 | + +End-of-window unit count: **141** (was 112 before batch 16; net +29). + +## 2. Phase 1 — Context Loading + +Re-read: + +- AZ-367, AZ-364, AZ-360, AZ-371 task specs (`_docs/02_tasks/done/AZ-367*.md`, `done/AZ-364*.md`, `done/AZ-360*.md`, `done/AZ-371*.md`) +- `_docs/02_document/module-layout.md` (component boundaries) +- `_docs/02_document/architecture.md` `## Architecture Vision` and current state +- `_docs/02_document/architecture_compliance_baseline.md` (5 baseline findings, all `RESOLVED` from prior epics — no carry-over) +- `_docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md` (C14, C11, C08, C18 entries) + +All four tasks sit within epic AZ-350. Batches 16 + 17 close Phase 3 (Structural cleanup); batch 18 opens Phase 4 (typing/config/tooling). Common theme across the window: **promote inline complexity into named, single-responsibility modules without changing observable HTTP / DB / file-output behavior.** + +## 3. Phase 2 — Spec Compliance (cumulative) + +| Task | ACs | Verification | +|------|-----|--------------| +| AZ-367 (C14) | AC-1 (single stitcher used by both consumers) / AC-2 (pixel-for-pixel identical output for existing scenarios) / AC-3 (37+ unit + 5 smoke green) | Three pixel-byte-equality unit tests + integration smoke. Verified in batch 16. | +| AZ-364 (C11) | AC-1 (single-responsibility collaborators, queue-free unit tests) / AC-2 (same lifecycle + DB writes + file outputs) / AC-3 (unit + smoke green; route image identical) | 14 new collaborator tests + 4 moved tests + 5 smoke. Verified in batch 17. | +| AZ-360 (C08) | AC-1 (no `IServiceProvider` in `RouteProcessingService`) / AC-2 (37 unit + 5 smoke green) | Constructor signature inspected; no `IServiceProvider` field, no `using var scope`. Smoke covers route processing end-to-end. Folded into AZ-364 batch 17. | +| AZ-371 (C18) | AC-1 (all listed magic numbers replaced) / AC-2 (defaults preserve behavior) / AC-3 (CT flows from `GetTileByLatLon` to downloader) | `ConfigDefaultsTests`×7 pin defaults to original literals. `TileServiceTests.DownloadAndStoreSingleTileAsync_ForwardsCancellationTokenToDownloader_AZ371_AC3` captures CT identity through the mock. Verified in batch 18. | + +No spec-gap detected on any of the four tasks. + +## 4. Phase 3 — Code Quality (cumulative) + +- **SOLID**: SRP improvements continue across the window. The `RouteProcessingService` god-class went from ~750 LOC with 6+ responsibilities to a ~280-LOC orchestrator (batch 17). The grid-placement loop is now a single `TileGridStitcher.StitchAsync` call instead of two duplicated implementations (batch 16). Operational levers are no longer baked into source (batch 18). +- **Error handling**: every new collaborator uses `ArgumentNullException.ThrowIfNull` for entry-point guards. `TileGridStitcher` throws `ArgumentOutOfRangeException` for non-positive tile size and `InvalidOperationException` for empty placements. No bare `catch` introduced anywhere in the window. The `swallowTileLoadErrors` flag (batch 16) is a deliberate, documented behavior preserved from the pre-refactor route stitcher (corrupt-tile-in-batch survives) — not silent error suppression. +- **Naming**: every new type is intent-revealing — `TileGridStitcher`, `TilePlacement`, `StitchResult`, `MissingTile`, `MissingTileReason`, `TileInfo`, `RouteRegionMatcher`, `RouteCsvWriter`, `RouteSummaryWriter`, `RouteImageRenderer`, `TilesZipBuilder`, `RegionFileCleaner`. None reads "Manager" / "Helper" / "Util". The corresponding methods (`StitchAsync`, `Match`, `WriteAsync`, `RenderAsync`, `BuildAsync`, `CleanupAsync`) read directly from the call site without ambiguity. +- **Complexity**: every new method in the window is ≤ 80 LOC. The decomposed `RouteProcessingService.GenerateRouteMapsAsync` is ~40 LOC of dispatch vs. the ~350 LOC pre-refactor. `RouteImageRenderer.RenderAsync` is the largest at 100 LOC, dominated by the geofence rectangle + cross overlay loops which are inherently linear in their input. +- **DRY**: positive net DRY across the window. The grid-placement loop existed in two services → now in one (TileGridStitcher). The `TileSizePixels = 256` literal existed in two places (TileService + RouteImageRenderer briefly) → batch 18 consolidates both into `MapConfig.TileSizePixels`. The `0.0001` lat/lon tolerance existed in `RouteValidator.cs` and `GoogleMapsDownloaderV2.cs` → now `ProcessingConfig.LatLonTolerance` (batch 18). The 4 `private const`s in `GoogleMapsDownloaderV2` (batch 18) are gone. +- **Test quality**: all new tests use xUnit + FluentAssertions + the project's `// Arrange / // Act / // Assert` pattern. Three pixel-byte-equality tests in batch 16 are the strongest behavior-preservation evidence in the run so far (40,000 + 10,000 + 65,536 pixel comparisons against inlined pre-refactor code). +- **Dead code**: nothing left dangling in this window. The `private const int TileSizePixels = 256` in `RouteImageRenderer` (born in batch 17) is gone (batch 18). The `BackgroundColor(Color.Black)` call in the route stitcher is gone (batch 16, proven equivalent to default `new Image`). The 9 helper methods that lived on `RouteProcessingService` (batch 17) are deleted, not commented out. The pre-refactor `RouteProcessingServiceTests.cs` is deleted, with its testable methods moved to `RouteImageRendererTests.cs`. +- **Static-vs-instance**: batch 18 demoted three methods from `static` to instance (`TileService.BuildTileEntity`, `GoogleMapsDownloaderV2.CalculateTileSizeInMeters`, `RouteValidator.ValidatePolygon`) because each one now reads instance state (`_mapConfig` / `_processingConfig`). This is exactly the path `coderule.mdc` prescribes ("Only use static methods for pure, self-contained computations") — moving the data dependency into the instance moves the method off the static surface in lockstep. + +No code-quality finding. + +## 5. Phase 4 — Security Quick-Scan (cumulative) + +- No new SQL building, no new string-interpolated queries, no `Process.Start` / `eval` / native invocation introduced in the window. +- No new credentials or secrets. Batch 18 promotes operational levers to config but every new key is a non-secret operational tunable (timeouts, polling intervals, tolerances, retry delays). +- The `appsettings.json` additions (batch 18) carry no secrets — values match prior source literals exactly. +- The new collaborators in batch 17 take `Guid routeId` and `IEnumerable` — no user-controlled file paths reach these constructors. `RouteImageRenderer.ExtractTileCoordinatesFromFilename` (batch 17) routes through `StorageConfig.TryExtractTileCoordinates` (a known-good parser established in batch 12 / AZ-366) and never `Path.Combine`s user input into a file path. +- `TilesZipBuilder.BuildAsync` (batch 17) reuses the file paths produced by the tile downloader — the sources are server-controlled, not client-controlled. +- The CT forwarding in batch 18 (LF-2) **improves** the security posture: `/api/satellite/tiles/latlon` clients can now cancel runaway downloads, removing a potential DoS amplifier (a slow client holding a connection while the server downloads tiles). +- AZ-353 sanitized 5xx envelope still applies to all new code paths — no batch in the window introduces a `Results.Problem(detail: ex.Message, ...)` regression. + +No security finding. + +## 6. Phase 5 — Performance Scan (cumulative) + +- **Stitcher consolidation (b16)**: same O(N) grid-placement loop, same per-tile `Image.LoadAsync` cost, same min/max scan. One additional `ToList()` on the input enumerable — bounded by region/route tile count (≤ 690 in the integration smoke). No measurable regression. +- **Decomposition (b17)**: every collaborator runs the same logic that previously lived in `RouteProcessingService`. `RouteRegionMatcher.Match` keeps the same point-by-distance ranking; `RouteCsvWriter`/`RouteSummaryWriter`/`TilesZipBuilder`/`RegionFileCleaner` all do the same I/O the orchestrator did before. Constructor injections are resolved once at app start (singletons via `RouteManagementServiceCollectionExtensions`), so the per-route overhead is unchanged. +- **IServiceProvider → IRegionService (b17 / AZ-360)**: removes a `using var scope = _serviceProvider.CreateScope()` allocation per pending route iteration. Net **positive** perf change (microscopic, but in the right direction). +- **Magic-number config (b18)**: each consuming class reads its config value once into a local field at construction time, so the per-call cost is identical to the pre-refactor literal. The two sites that previously did `0.0001 < something` per-iteration in a closure (`GoogleMapsDownloaderV2.GetTilesWithMetadataAsync`, `RouteValidator.ValidatePolygon`) now hoist the tolerance into a local — eliminates repeated property access in the hot loop. +- No new I/O introduced anywhere in the window. No new DB calls. No new HTTP calls. + +No performance regression. Slight positive direction on (b17/AZ-360) and the closure hoist in (b18). + +## 7. Phase 6 — Cross-Task Consistency (cumulative — main focus of K=3 review) + +Style & pattern consistency across the three batches: + +- **Class shape**: every new collaborator in the window is a `public class` (not `public static class`, not `sealed`, not `record`) with a single explicit constructor, a single `_logger`-style readonly field, and one or two `IOptions`-bound config fields. The shape matches the precedent set by `TileCsvWriter` (cycle 1 batch 13) and `RegionFailureClassifier` (cycle 1 batch 11). Consistent. +- **DI policy split**: shared / cross-cutting utilities live in `Common` and are constructed at the call site (`new TileCsvWriter()`, `new TileGridStitcher()`). Per-component collaborators that need `IOptions` and an `ILogger` are DI-registered as singletons. Batch 17 adds 5 such registrations to `RouteManagementServiceCollectionExtensions`; batch 16 deliberately did not register `TileGridStitcher` because it has no dependencies (pure utility). The split is consistent with existing Common utilities and with the precedent from cycle 1 batch 13 (TileCsvWriter, no DI registration). +- **Test convention**: every new test file uses xUnit + FluentAssertions + the `// Arrange / // Act / // Assert` comment block. No drift. The pixel-byte-equality tests in `TileGridStitcherTests` (batch 16) document **why** they need a literal inlined copy of the pre-refactor code (behavior preservation evidence) — comment-supported, not surprising. +- **Error-handling convention**: every new public method that takes a reference parameter uses `ArgumentNullException.ThrowIfNull(...)` as the first line. Domain validation throws `ArgumentException` / `ArgumentOutOfRangeException` / `InvalidOperationException` with descriptive messages. Consistent with the prior cumulative window. +- **Configuration injection convention**: batch 18 settles a single pattern — every config-aware class takes `IOptions` in the constructor, calls `.Value` once, stores the result in a `private readonly TConfig _config` field (or copies specific scalars into typed fields when only one or two are used). Five classes (`RegionService`, `RouteProcessingService`, `RoutePointGraphBuilder`, `RouteValidator`, `RouteImageRenderer`, `TileService`) follow this exact shape. `GoogleMapsDownloaderV2` already followed it; the new code matches. +- **No conflicting refactor styles**: no batch introduces a heavyweight pattern (factory, mediator, generic abstract base, fluent builder) that the next batch then has to inherit or work around. Each new class is a self-contained collaborator. +- **Symbol drift**: searched all new public type names across the codebase. **`TileSizePixels`** appears as a `MapConfig` property (batch 18), as a `TileEntity.TileSizePixels` column (DB-layer, unchanged for years), and as the parameter name `int tileSizePixels` in `TileGridStitcher.StitchAsync`. No conflicting definitions, no constants by the same name in two places after batch 18 — the `private const` in `RouteImageRenderer` born in batch 17 was explicitly removed by batch 18 (cumulative-review hygiene closing the loop within the same window). **`MaxPointSpacingMeters`** is now a single `ProcessingConfig` property (batch 18) — the public const that briefly lived on `RoutePointGraphBuilder` (cycle 1 batch 15) is gone. **`LatLonTolerance`** is now a single `ProcessingConfig` property (batch 18) — replacing two separate `0.0001` literals. +- **Interface drift**: `ITileService`, `IRegionService`, `IRouteService`, `IRegionRepository`, `IRouteRepository`, `ITileRepository`, `ISatelliteDownloader` — all unchanged across the window. No public API additions or removals. +- **Constructor-fan-out coordination**: batch 17 added 5 ctor parameters to `RouteProcessingService`; batch 18 added 1 more (the 6th). The DI container resolves all 6 because batch 17 also added the corresponding `services.AddSingleton` registrations and batch 18 only added a bind that was already in the container (`IOptions`). Smoke verifies the full chain end-to-end. +- **Dual-ctor pattern in RouteService**: batch 18 changed the production-callable ctor from 3-arg to 4-arg. The 8-arg test/decompose ctor is preserved. ASP.NET Core's default DI container will pick the 4-arg overload because `RouteValidator`, `RoutePointGraphBuilder`, `GeofenceGridCalculator`, `RouteResponseMapper` are NOT DI-registered (instantiated inline in the 4-arg `:this(...)` chain). Verified by the smoke run. + +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: + - `TileGridStitcher` (batch 16) → `Common/Imaging/` (Layer 1 — Foundation). Adds an ImageSharp NuGet to `Common.csproj`. The Common layer's `Imports from: (none)` invariant from `module-layout.md` is preserved (NuGet packages are external dependencies, not project references). Verified — no new `` lines in `Common.csproj`. + - 7 new RouteManagement collaborators (batch 17) → `SatelliteProvider.Services.RouteManagement/` (Layer 3 — Application). Correct. + - 8 new keys in `ProcessingConfig` / `MapConfig` (batch 18) → `Common/Configs/` (Layer 1). Correct. + - 1 new `appsettings.json` block (batch 18) → `Api/` (Layer 4). Correct. + - All cross-component imports in the window go through Common Public API symbols (DTOs, configs, interfaces, utils, imaging) or DataAccess Public API symbols (entity types, repository interfaces). No internal-file imports across components. + +2. **No new cyclic dependencies**: the module DAG is unchanged in this window. + - `RouteManagement → Common`, `RouteManagement → DataAccess`. ✅ + - `RegionProcessing → Common`, `RegionProcessing → DataAccess`. ✅ + - `TileDownloader → Common`, `TileDownloader → DataAccess`. ✅ + - `Api → Common`, `Api → all three Services`. ✅ + - **No sibling references between RouteManagement / RegionProcessing / TileDownloader.** Verified by reading the four `.csproj` files. + +3. **Duplicate symbols across components**: + - `TileGridStitcher` — single definition in `Common.Imaging`. No duplicate. + - `TilePlacement`, `StitchResult`, `MissingTile`, `MissingTileReason` — companion records / enum in the same file. No duplicates elsewhere. + - `TileInfo`, `RouteRegionMatcher`, `RouteCsvWriter`, `RouteSummaryWriter`, `RouteImageRenderer`, `TilesZipBuilder`, `RegionFileCleaner` — all single-defined in `Services.RouteManagement`. No duplicates. + - The cross-batch hygiene observation from §7 (TileSizePixels const briefly lived in two places, then collapsed by batch 18) is exactly the kind of finding the cumulative review is designed to surface — it's already resolved within the same window, before the report writes. + +4. **Cross-cutting concerns not locally re-implemented**: + - Stitching → `Common.Imaging.TileGridStitcher` (batch 16). Region service and route renderer both consume it. Cross-cutting concern correctly hoisted. + - Operational config → `Common.Configs.{ProcessingConfig, MapConfig}` (batch 18). Every consuming service reads from the same options-bound source. Cross-cutting concern correctly hoisted. + - Per-route output generation (CSV, summary, ZIP, stitched, cleanup) → kept inside `Services.RouteManagement` (batch 17). These are RouteManagement-specific concerns, not cross-cutting. + +5. **Public API growth**: Common Public API gains: + - 4 new types + 1 enum under `Common.Imaging` (batch 16) — used by both Region and Route renderers. + - 8 new properties on existing config classes (batch 18) — non-breaking additions, defaulted to original literal values. + - No new interface, no breaking signature change. Backward-compatible growth. + +6. **DI registration coherence**: batch 17 added 5 singleton registrations to `RouteManagementServiceCollectionExtensions`. Batch 18 added no new DI registrations because `IOptions` and `IOptions` were already bound at startup. Smoke verifies the full chain — all 12 (`SatelliteProvider.Services.*`) resolution paths complete successfully. + +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 (F1–F5) were resolved by epics AZ-309 + AZ-315 prior to this run. The baseline file already reflects this. | +| 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**: 141 / 141 passing (was 112 before batch 16; net +29 across the window — TileGridStitcher×11, RouteManagement collaborators×10 net, ConfigDefaults×7, AZ-371 AC-3 CT×1). +- **Integration smoke**: 5 / 5 scenarios passing; container exits 0. Idempotency tests (AZ-362), Migration 012 tests (AZ-357), CORS / 5xx-sanitization / 501-stub / security tests, region/route processing happy paths — all green. +- **No skipped tests, no flaky tests** in this window. The three pixel-byte-equality tests in `TileGridStitcherTests` are deterministic (synthetic inputs + inlined pre-refactor code). + +## 12. Action + +Auto-chain to next batch per `/implement` Step 14. K=3 counter resets; next cumulative review fires after batch 21. Phase 4 continues with **AZ-370** (C17 — status / point-type enums + AC RT2 update, 3 SP, no dependencies in `todo/`). diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 68bf93f..79f7706 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -8,7 +8,7 @@ status: in_progress sub_step: phase: 4 name: execution - detail: "batch 18 (AZ-371) complete; cumulative K=3 review (16-18) next" + detail: "K=3 review (16-18) PASS; next batch 19 candidate AZ-370" retry_count: 0 cycle: 1 tracker: jira