# Code Review Report

**Batch**: 2 — AZ-286 + AZ-287 + AZ-288 (TileService / RegionService / RouteService unit tests)
**Date**: 2026-05-10
**Verdict**: PASS_WITH_WARNINGS

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Spec-Gap | (deferred) | BT-N01 (invalid coords) not unit-tested — TileService does not validate lat/lon |
| 2 | Low | Spec-Gap | (docs) | Point-type label drift between blackbox-tests.md ("original"/"intermediate") and code ("start"/"action"/"end"/"intermediate") |
| 3 | Low | Spec-Gap | (docs) | Region status drift: blackbox-tests.md "pending → processing → completed" vs code "queued → processing → completed" |

### Finding Details

**F1: BT-N01 not service-unit-tested** (Low / Spec-Gap)
- Location: out-of-scope for `TileService`
- Description: AZ-286 lists BT-N01 (invalid lat/lon) but neither `TileService.DownloadAndStoreTilesAsync` nor `GoogleMapsDownloaderV2.DownloadSingleTileAsync` validates coordinate ranges. Validation lives at the API endpoint layer (`Program.cs`).
- Suggestion: Cover BT-N01 in AZ-289 (integration tests) which exercises the HTTP API. Optionally add API-layer validation in a future testability/refactor step.
- Tasks: AZ-286, AZ-289

**F2: Point-type label drift** (Low / Spec-Gap)
- Location: `_docs/02_document/tests/blackbox-tests.md` BT-06 / BT-10 / BT-12 vs `SatelliteProvider.Services/RouteService.cs:66`
- Description: Test spec says first/last point type is `original`. Code emits `start` / `action` / `end` for user-supplied points and `intermediate` for interpolated. The spec's "original" appears to be a generic term for "user-supplied", which does not match any actual code value. RouteService unit tests assert the actual code values to keep tests honest.
- Suggestion: Update `blackbox-tests.md` BT-06/BT-10/BT-12/BT-N* phrasing OR introduce an "original" alias in code. Either option is harmless but should be tracked. The traceability matrix should be updated to reflect the chosen wording.
- Task: AZ-288 (and downstream test-spec sync in Step 12)

**F3: Region status string drift** (Low / Spec-Gap)
- Location: `_docs/02_document/tests/blackbox-tests.md` BT-03/BT-04 vs `SatelliteProvider.Services/RegionService.cs:48` (initial state = `"queued"`)
- Description: Spec uses "pending → processing → completed". Code uses "queued → processing → completed". Tests assert the code's actual transitions.
- Suggestion: Update test-spec to match code, or add a `"pending"` alias in code. Same trade-off as F2. Track in test-spec sync (Step 12).
- Task: AZ-287

## Phase Summary

- **Phase 1 (Context)**: AZ-286/287/288 specs loaded; ACs identified.
- **Phase 2 (Spec compliance)**:
  - AZ-286 AC-1: 3/4 (BT-01 ✓, BT-02 ✓ + BT-02b stale-version, BT-N02 ✓; BT-N01 deferred → F1).
  - AZ-286 AC-2: ✓ (`ISatelliteDownloader` is mocked everywhere; no real Google Maps).
  - AZ-286 AC-3: ✓ (happy path + cache reuse + zoom validation negative).
  - AZ-287 AC-1: ✓ (`RequestRegionAsync_InsertsEntityAndQueues_BT03_AC1`).
  - AZ-287 AC-2: ✓ (`ProcessRegionAsync_HappyPath_TransitionsToCompletedAndWritesArtifacts_BT03_AC2_AC3` — verifies "processing → completed"; initial "queued" is set in `RequestRegionAsync`).
  - AZ-287 AC-3: ✓ (CSV + summary files asserted on disk).
  - AZ-287 AC-4: ✓ (`ProcessRegionAsync_StitchEnabled_SetsStitchedImagePath_BT05_AC4`).
  - AZ-288 AC-1: ✓ (≤200m spacing iterated and verified).
  - AZ-288 AC-2: ✓ (start/end/action/intermediate counts verified — see F2).
  - AZ-288 AC-3: ✓ (`ComputesTotalDistanceViaHaversine_AC3` cross-checks `TotalDistanceMeters` against summed Haversine distances).
  - AZ-288 AC-4: ✓ (BT-N03 < 2 points, region-size validation, BT-N04, BT-N05).
- **Phase 3 (Quality)**: tests follow Arrange/Act/Assert; helpers (`BuildService`, `BuildRequest`, `MakeDownloaded`) keep arrange sections small; no duplication.
- **Phase 4 (Security)**: no secrets, no injection vectors. The mock API key `"test-key"` in `GoogleMapsDownloaderZoomValidationTests` never escapes the test (no HTTP performed).
- **Phase 5 (Performance)**: tests run in <1s total — no anti-patterns.
- **Phase 6 (Cross-task)**: shared fixtures (`TestCoordinates`) used uniformly. Mock setups consistent across test files.
- **Phase 7 (Architecture)**: tests sit in `SatelliteProvider.Tests/` and consume only Public API of Common, DataAccess, Services per `module-layout.md`. No layering violation. No new cycles. No duplicate symbols.

## Test Run

Container: `mcr.microsoft.com/dotnet/sdk:8.0`
- Tests discovered: 31 (4 from Batch 1 + 27 new in Batch 2)
- Passed: 31 / 31
- Total time: 0.83s

## Verdict Logic

3 Low findings, 0 Medium, 0 High, 0 Critical → PASS_WITH_WARNINGS.