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

Code Review Report — Cumulative (Batches 10+11+12)

Batch: 12 (cumulative; covers batches 10, 11, 12) Tasks reviewed: AZ-357 (drop tile Version + AC-2 follow-up), AZ-362 (idempotent POST), AZ-366 (Haversine consolidation) Date: 2026-05-10 Mode: Cumulative (Phases 1–7 on union of changed files since last review at batch 9) Verdict: PASS Critical: 0 | High: 0 | Medium: 0 | Low: 1 (informational, non-blocking)

Files in scope

File	Tasks	Change
SatelliteProvider.DataAccess/Migrations/012_DropTileVersionConstraint.sql	AZ-357	New (drop 5-col index, dedupe, recreate 4-col index, version nullable)
SatelliteProvider.DataAccess/Models/TileEntity.cs	AZ-357	Modified (Version → int?)
SatelliteProvider.DataAccess/Repositories/TileRepository.cs	AZ-357	Modified (4-col upsert; ORDER BY updated_at DESC)
SatelliteProvider.Services.TileDownloader/TileService.cs	AZ-357	Modified (removed year-version filter; BuildTileEntity writes Version = null)
SatelliteProvider.Common/DTO/TileMetadata.cs	AZ-357	Modified (Version → int?)
SatelliteProvider.Api/Program.cs	AZ-357, AZ-362	Modified (DownloadTileResponse.Version → int?; OpenAPI Description for idempotent POSTs)
SatelliteProvider.Tests/TileServiceTests.cs	AZ-357	Modified (replaced stale-version test with TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1; added BuildTileEntity_DoesNotPopulateVersion_AZ357)
SatelliteProvider.IntegrationTests/MigrationTests.cs	AZ-357 AC-2 follow-up	New (populated-duplicates dedupe + index-shape verification)
SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj	AZ-357 AC-2 follow-up	Modified (Npgsql 9.0.2)
docker-compose.tests.yml	AZ-357 AC-2 follow-up	Modified (DB_CONNECTION_STRING env var, postgres healthcheck dependency)
SatelliteProvider.Services.RegionProcessing/RegionService.cs	AZ-362	Modified (early-return idempotency check in RequestRegionAsync)
SatelliteProvider.Services.RouteManagement/RouteService.cs	AZ-362	Modified (early-return idempotency check in CreateRouteAsync)
SatelliteProvider.Tests/RegionServiceTests.cs	AZ-362	Modified (+1 unit test asserting no AddAsync/EnqueueAsync on duplicate id)
SatelliteProvider.Tests/RouteServiceTests.cs	AZ-362	Modified (+1 unit test asserting no InsertRouteAsync/RegionRequest on duplicate id)
SatelliteProvider.IntegrationTests/IdempotentPostTests.cs	AZ-362	New (2 end-to-end HTTP tests)
SatelliteProvider.IntegrationTests/Program.cs	AZ-357 AC-2, AZ-362	Modified (registers MigrationTests + IdempotentPostTests in smoke + full suites)
SatelliteProvider.Common/Configs/StorageConfig.cs	AZ-366	Modified (added static TryExtractTileCoordinates co-located with GetTileFilePath)
SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs	AZ-366	Modified (deleted local CalculateDistance; ExtractTileCoordinatesFromFilename is now thin wrapper over StorageConfig helper)
SatelliteProvider.Tests/StorageConfigTests.cs	AZ-366	New (6 tests including writer-parser round-trip for AC-2)

Findings

LOW-1 — AZ-362 idempotency check sits above validation (informational)

Severity: Low (informational, not blocking) Location: RouteService.cs:32-39 (idempotency check), RouteService.cs:41-54 (validation) Observation: For both region POST and route POST, the idempotency early-return runs before input validation. A client retrying a duplicate id with a now-invalid payload will receive 200 + the original resource instead of 400 + validation errors. Reason this is not a defect: This matches the documented idempotency contract (the persisted resource wins; clients should not mutate the body across retries). The only payload that creates the resource is the original one, which by definition was valid. Reordering would mean validating a payload that's about to be ignored. Recommendation: No change. Documented in batch 11 report. Consider mentioning it explicitly in OpenAPI Description if a future client integration surfaces the surprise.

Phase Summaries

Phase 1 — Context loading

Read all 3 task descriptions from Jira (AZ-357, AZ-362, AZ-366), the corresponding batch reports (10, 11, 12), the AZ-357 task spec at _docs/02_tasks/done/, and the changed source files end-to-end. Verified prior cumulative review (batch 09) baseline.

Phase 2 — Spec compliance

Task	AC	Evidence
AZ-357 (C06) AC-1 Cache survives year boundary	Unit test TreatsCachedTileFromPriorYearAsFresh_AZ357_AC1 passes; year-version filter has been removed from TileService.DownloadAndStoreTilesAsync (grep for currentVersion returns 0 hits).
AZ-357 AC-2 Migration runs cleanly on populated tile data	Two integration tests in MigrationTests.cs: dedupe SQL test seeds 6 rows including a 3-way duplicate + an updated_at tie + a unique row, runs the dedupe SQL from migration 012, asserts only 3 rows survive (correct winners by updated_at DESC, id DESC); index-shape test queries pg_indexes to confirm idx_tiles_unique_location is unique on (latitude, longitude, tile_zoom, tile_size_meters). Both green.
AZ-357 AC-3 Upsert behaves on the new key	TileRepository.InsertAsync uses ON CONFLICT (latitude, longitude, tile_zoom, tile_size_meters). End-to-end full integration suite re-inserts 690+ tiles for the same region across multiple test runs without unique-violation errors.
AZ-357 AC-4 All tests stay green	77 unit + smoke + full integration green.
AZ-362 (C09) AC-1 Region duplicate-id POST → 200 + existing resource	Unit RequestRegionAsync_DuplicateId_ReturnsExistingResource_NoReQueue_AZ362_AC1 (verifies repo + queue are not invoked); integration RegionPost_SameIdTwice_BothReturn200_NoDuplicateProcessing_AZ362_AC1 (HTTP shape + same CreatedAt + log line).
AZ-362 AC-2 Route duplicate-id POST → 200 + existing resource	Unit CreateRouteAsync_DuplicateId_ReturnsExistingRoute_NoReinsertion_AZ362_AC2; integration RoutePost_SameIdTwice_BothReturn200_NoReinsertion_AZ362_AC2 (TotalPoints unchanged ≡ no point regeneration).
AZ-362 AC-3 OpenAPI documents idempotency	Program.cs:151-155, 169-173 — Description strings on both MapPost registrations explicitly call out AZ-362 contract.
AZ-362 AC-4 4xx validation paths preserved	CreateRoute_InvalidPayload_Returns400_AZ353_AC3 integration test still green — non-duplicate-id POSTs still hit request.Points.Count < 2 etc.
AZ-366 (C13) AC-1 One Haversine in the codebase	Grep for Math.Sin(dLat/2) returns matches only at GeoUtils.cs:28. Grep for EARTH_RADIUS returns matches only in GeoUtils.cs. Grep for the deleted 4-arg signature CalculateDistance(double, double, double, double) returns 0 hits.
AZ-366 AC-2 Writer + parser co-located	Both GetTileFilePath and TryExtractTileCoordinates live as methods on StorageConfig in SatelliteProvider.Common/Configs/StorageConfig.cs. New unit test GetTileFilePath_RoundTrip_ParserRecoversOriginalCoordinates_AZ366_AC2 mechanically proves the inverse-pair invariant.
AZ-366 AC-3 Tests stay green	77 unit (4 pre-existing AZ-352 ExtractTileCoordinatesFromFilename tests still pass against the now-thin wrapper).

All ACs traced and demonstrably satisfied.

Phase 3 — Code quality

• SOLID — Single Responsibility:
  • RegionFailureClassifier (batch 9) and StorageConfig.TryExtractTileCoordinates (batch 12) are textbook SRP wins — single-purpose, easily named, easily tested.
  • RegionService.RequestRegionAsync and RouteService.CreateRouteAsync (batch 11) gain one new responsibility each (idempotency check). The check is small, named in the comment, and lives at the top so the existing creation logic is visually separated. Acceptable.
  • RouteProcessingService.ExtractTileCoordinatesFromFilename (batch 12) is now a 9-line wrapper. Thin enough that future deletion (once all callers move to the static helper directly) would be trivial — flagged as future cleanup, not a current defect.
• Error handling:
  • AZ-357 migration uses DROP INDEX IF EXISTS for idempotency under DbUp's tracking, deterministic dedupe ordering (updated_at DESC, id DESC), and sequenced ALTER COLUMN clauses that don't conflict.
  • AZ-362 leaves the underlying repository path unchanged — if a TOCTOU race fires between the GetByIdAsync check and InsertAsync, the unique-key PostgresException propagates up to the AZ-353 global handler. No silent suppression introduced.
  • AZ-366 parser returns (-1, -1) sentinel only for the documented format-error path; null path still throws ArgumentNullException. The wrapper still logs the warning. Strictly equivalent to the prior AZ-352 behavior from a caller's perspective.
• Naming:
  • TryExtractTileCoordinates follows the .NET Try* convention (returns bool, uses out params for the parsed values, (-1, -1) sentinel preserved on the wrapper for backward-compat).
  • RoutePost_SameIdTwice_BothReturn200_NoReinsertion_AZ362_AC2 — long, but specific, and the pattern is consistent across the new test suite.
• Complexity:
  • RouteProcessingService.cs shrinks ~30 LOC across the two deleted/wrapped methods.
  • RegionService.cs and RouteService.cs each grow ~10 LOC for the idempotency early-return; small, localized.
  • No new method exceeds 50 lines; no cyclomatic-complexity hotspot introduced.
• DRY:
  • AZ-366 removed a duplicate Haversine. Single source of truth: GeoUtils.
  • AZ-366 removed the by-string-convention coupling between StorageConfig.GetTileFilePath (writer) and RouteProcessingService.ExtractTileCoordinatesFromFilename (parser). Coupling is now structural.
• Test quality:
  • 8 new tests across the window (6 in StorageConfigTests + 2 in unit suite for AZ-362). All assert on specific values, mock interactions, and observable state — no "no exception thrown" tests.
  • MigrationTests proves the SQL DELETE shape against a populated table — directly closes the gap that batch 10 had flagged.
  • IdempotentPostTests use a 1ms tolerance on CreatedAt to account for PostgreSQL's microsecond truncation vs .NET's 100ns ticks — explicitly documented in the test comment.
  • All tests follow the project's // Arrange / // Act / // Assert pattern.
• Dead code:
  • AZ-366 deleted RouteProcessingService.CalculateDistance(double, double, double, double) and ate one inline call site.
  • The version column in the tiles table is now nullable but preserved — explicitly per coderule.mdc ("Do not rename any databases or tables or table columns without confirmation"). Future cleanup for that column is tracked as AZ-373 (C20 — clarify or drop MapsVersion / Version).
  • The unused FindExistingTileAsync in TileRepository is still present — slated for AZ-376 (C23) deletion. Out of scope for this window.

Phase 4 — Security

• AZ-357: No new attack surface. Migration runs at startup with the application's own DB credentials; no user input flows in. The dedupe SQL uses parameterless ROW_NUMBER OVER (PARTITION BY ...) so SQL injection is structurally impossible.
• AZ-362: No new attack surface. The idempotency check uses Guid (not string) — no injection possible. The check is read-only (does not mutate state for the duplicate path) so it cannot be used to enumerate existing resources beyond what the caller already authenticated to know (the id they own).
• AZ-366: Pure string parsing. Path.GetFileNameWithoutExtension is allocation-only; no I/O. No PII or secrets cross the boundary.
• No CORS, auth, or 5xx-leakage regressions introduced. The 4xx vs 5xx contract from AZ-353 is preserved.

Phase 5 — Performance

• AZ-357: Migration runs once at startup. Production rollout might need a maintenance window for the dedupe DELETE on a large tiles table, but the SQL pattern itself uses ROW_NUMBER OVER PARTITION BY (single seq scan + sort) — acceptable for the scale documented in baseline metrics.
• AZ-362: Adds one extra GetByIdAsync call per POST (the duplicate-detection read). For an INSERT-heavy workload this is one extra read per write — acceptable; primary-key lookup is O(log n) on the index. Could be optimized with INSERT ... ON CONFLICT DO NOTHING + RETURNING, but that's an optimization for later (TOCTOU window note in the batch report explicitly acknowledges this).
• AZ-366: Net zero performance change. Same Haversine math, same parsing logic, same single call sites.

Phase 6 — Cross-task consistency

• All three batches touch SatelliteProvider.IntegrationTests/Program.cs. The registrations are non-conflicting and follow the established pattern (one await NewTestClass.RunAll(...) per suite).
• Logging style is consistent: structured fields with {PascalCase} placeholders, _logger.LogInformation for happy-path observability (idempotency hits), _logger.LogWarning for parse failures.
• Test file naming follows the established <Component>Tests.cs convention; new test methods follow MethodName_Condition_Expectation_AZxxx_ACy pattern.
• DTO and OpenAPI changes from AZ-357 and AZ-362 both touch Program.cs but in non-overlapping regions.

Phase 7 — Architecture

Check	Result
Layer direction	All cross-component imports go bottom-up. AZ-366's RouteProcessingService (Services.RouteManagement) imports StorageConfig (Common). No reversed direction.
Public API respect	New public symbols: StorageConfig.TryExtractTileCoordinates (Common). New int? shape on existing public DTOs (TileMetadata.Version, DownloadTileResponse.Version) — backward compatible for JSON consumers; explicit shape change documented in batch 10 report.
New cyclic dependencies	None. The split (per module-layout.md) is preserved.
Duplicate symbols across components	None. TryExtractTileCoordinates is unique to Common; idempotency checks are private behavior of their respective services.
Cross-cutting concerns mis-placed	None. Migration belongs in DataAccess; idempotency checks belong in the services that own the resource lifecycle; tile-filename parser belongs next to the tile-filename writer in Common.Configs.

Baseline delta

architecture_compliance_baseline.md lists 5 findings, all Resolved as of epic AZ-309 (per the prior cumulative review). This cumulative review introduces 0 new architecture findings.

Status	Count
Carried over	0
Resolved	5 (already by AZ-309)
Newly introduced	0
Newly identified non-blocking observations	1 (LOW-1 above)

Test results referenced

• Unit: 77 / 77 passing (was 71 at end of batch 11 → +6 new StorageConfigTests).
• Integration smoke: 5 / 5 passing.
• Stub-contract integration: 3 / 3 passing.
• Idempotent POST integration: 2 / 2 passing.
• Migration integration: 2 / 2 passing.
• Full integration suite: green; container exits 0.

Conclusion

Verdict: PASS. Phase 2 (Correctness) closed cleanly across AZ-357 + AZ-362 + AZ-366. Phase 3 (Structural cleanup) opened with AZ-366 also clean. Architecture baseline still at zero findings.

The single LOW-1 observation (idempotency check above validation) is informational and matches the documented contract — no remediation required.

The implement skill may proceed to batch 13 (AZ-368 / C15 TileCsvWriter) without an auto-fix loop or user gate.