# Code Review Report

**Batch**: 5 (AZ-312, AZ-313, AZ-314 — coupling refactor: project split + DI extension methods)
**Date**: 2026-05-10
**Verdict**: PASS

## Scope

Structural refactoring (architecture baseline finding F4): split monolithic `SatelliteProvider.Services` into three per-component csprojs, rewire consumers, and extract DI registrations into per-component extension methods.

Tasks reviewed:
- **AZ-312** — split Services into TileDownloader + RegionProcessing + RouteManagement csprojs
- **AZ-313** — update consumer csprojs (Api, Tests, IntegrationTests) + `using` directives
- **AZ-314** — DI extension methods per csproj + Program.cs cleanup

## Changed files

- New csprojs:
  - `SatelliteProvider.Services.TileDownloader/SatelliteProvider.Services.TileDownloader.csproj`
  - `SatelliteProvider.Services.RegionProcessing/SatelliteProvider.Services.RegionProcessing.csproj`
  - `SatelliteProvider.Services.RouteManagement/SatelliteProvider.Services.RouteManagement.csproj`
- New DI extension files:
  - `SatelliteProvider.Services.TileDownloader/TileDownloaderServiceCollectionExtensions.cs`
  - `SatelliteProvider.Services.RegionProcessing/RegionProcessingServiceCollectionExtensions.cs`
  - `SatelliteProvider.Services.RouteManagement/RouteManagementServiceCollectionExtensions.cs`
- Moved (with namespace updates): `TileService.cs`, `GoogleMapsDownloaderV2.cs`, `RegionService.cs`, `RegionProcessingService.cs`, `RegionRequestQueue.cs`, `RouteService.cs`, `RouteProcessingService.cs`
- New common exception: `SatelliteProvider.Common/Exceptions/RateLimitException.cs`
- Modified: `SatelliteProvider.Api/Program.cs`, `SatelliteProvider.Api/SatelliteProvider.Api.csproj`, `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj`, all 5 `*Tests.cs` files (`using` updates), `SatelliteProvider.sln`, both `Dockerfile`s
- Deleted: `SatelliteProvider.Services/` directory and `SatelliteProvider.Services.csproj`

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|

No findings.

## Phase results

### Phase 2 — Spec compliance

**AZ-312 (project split)**

- AC-1 ✓ — Three new csprojs exist with the seven moved source files distributed correctly (verified by `ls`).
- AC-2 ✓ — Old `SatelliteProvider.Services/` directory and csproj deleted; `SatelliteProvider.sln` updated.
- AC-3 ✓ — `dotnet build SatelliteProvider.sln` succeeds with 0 warnings, 0 errors.
- AC-4 ✓ — None of the three new csprojs reference each other. All three reference only `SatelliteProvider.Common` + `SatelliteProvider.DataAccess`.

**AZ-313 (consumer rewire)**

- AC-1 ✓ — Solution builds clean (47.98s, 0 warnings).
- AC-2 ✓ — All 40 unit tests pass (1.93s).
- AC-3 ✓ — Grep for `using SatelliteProvider.Services;` (no `.<Component>` suffix) returns zero source-file matches.

**AZ-314 (DI extension methods)**

- AC-1 ✓ — Each `*ServiceCollectionExtensions.cs` registers exactly the services owned by its csproj:
  - `AddTileDownloader`: `IMemoryCache`, `ISatelliteDownloader`, `ITileService`
  - `AddRegionProcessing`: `IRegionRequestQueue` (factory), `IRegionService`, `RegionProcessingService` (hosted)
  - `AddRouteManagement`: `IRouteService`, `RouteProcessingService` (hosted)
- AC-2 ✓ — `Program.cs:33-35` calls the three extension methods; previously inlined registrations are gone.
- AC-3 ✓ — Smoke integration suite passes end-to-end (region processing, route processing, tile ZIP, security tests). Every required service resolves at runtime.
- AC-4 ✓ — No old service-registration code remains in `Program.cs`.

### Phase 3 — Code quality

- SOLID (SRP/DIP) **improved** — components now have a compiler-enforced boundary; cross-component coupling can no longer be introduced silently.
- Lifetimes preserved — `Singleton` stayed `Singleton`, hosted services stayed hosted services.
- Hosted-service registration order preserved (`RegionProcessingService` before `RouteProcessingService`, matching pre-refactor order in Program.cs).
- No new dead code; no scope creep.

### Phase 4 — Security quick-scan

Refactor is structural; no new input handling, query construction, or deserialization paths. Smoke security tests (SEC-01 SQL injection, SEC-02 path traversal, SEC-03 oversized region, SEC-04 malformed JSON) all pass post-refactor.

### Phase 5 — Performance scan

No hot-path changes. No new allocations in the moved code (only namespace and project boundary changes).

### Phase 6 — Cross-task consistency

- All three new extension methods follow the same `Add<Component>(this IServiceCollection)` naming convention.
- All three new csprojs share identical TFM (`net8.0`), `ImplicitUsings`, and `Nullable` settings.
- Package versions consistent (`9.0.10` for `Microsoft.Extensions.*`, `13.0.4` for `Newtonsoft.Json`, `3.1.11` for `SixLabors.ImageSharp`).

### Phase 7 — Architecture compliance

- **Layer direction** ✓ — TileDownloader (Layer 2) imports only Common + DataAccess (Layer 1). RegionProcessing and RouteManagement (Layer 3) import only Common + DataAccess (no cross-Layer-3 references after the refactor — even better than the pre-refactor baseline allowed).
- **Public API respect** ✓ — All cross-component imports use interfaces from `SatelliteProvider.Common.Interfaces`, never concrete types from sibling components.
- **No new cyclic dependencies** ✓ — Graph is a clean DAG: Common ← {DataAccess} ← {TileDownloader, RegionProcessing, RouteManagement} ← WebApi.
- **Architecture baseline F4** — **RESOLVED**. The single `SatelliteProvider.Services.csproj` packing three components is replaced by three csprojs with compiler-enforced boundaries.
- **Architecture baseline F3** — already resolved in Batch 4 (AZ-310 + AZ-311); this batch did not regress it (`Program.cs` tile endpoints still route through `ITileService`).
- **`RateLimitException` placement** — moved to `SatelliteProvider.Common/Exceptions/` rather than allowing a `RegionProcessing → TileDownloader` ProjectReference. This is the cleanest of the three options considered (move to Common / add reference / catch general `Exception`) and preserves layer 3 components as siblings rather than coupling them.

## Baseline Delta

| Status | Finding | Severity | Notes |
|--------|---------|----------|-------|
| Resolved | F4 — `SatelliteProvider.Services.csproj` packs three logical components | Medium | Split into three csprojs |
| Carried over | (none for this batch) | — | — |
| Newly introduced | (none) | — | — |

`module-layout.md` and `architecture.md` still describe the pre-split layout (single `SatelliteProvider.Services/` project). This is **expected** — documentation sync is task **AZ-315** (Batch 3 of this refactor run). Not a finding.

## Verdict

**PASS** — all 12 ACs across 3 tasks satisfied. Build clean. Unit tests 40/40. Smoke integration green. Architecture baseline F4 resolved with no new findings.