# Code Review Report

**Batch**: 3 — AZ-289 (Integration tests: route map processing + ZIP), AZ-290 (Non-functional tests: perf, resilience, security, limits)
**Date**: 2026-05-10
**Verdict**: PASS_WITH_WARNINGS

## Scope

Changed files in this batch:

- `SatelliteProvider.IntegrationTests/ExtendedRouteTests.cs` — added 50 MB ZIP cap assertion (RL-01).
- `SatelliteProvider.IntegrationTests/SecurityTests.cs` — new file; SEC-01..SEC-04 integration tests.
- `SatelliteProvider.IntegrationTests/Program.cs` — wires `SecurityTests.RunAll` into the integration runner.
- `SatelliteProvider.Tests/RegionRequestQueueTests.cs` — new file; covers queue capacity behavior (RS-04 / RL-02 at unit level).
- `scripts/run-performance-tests.sh` — extended with PT-01, PT-03, PT-04, PT-05.

Existing integration tests (`BasicRouteTests.RunRouteWithRegionProcessingAndStitching`, `ExtendedRouteTests.RunRouteWithTilesZipTest`, `ComplexRouteTests.RunComplexRouteWithStitching*`) already covered BT-08 / BT-09 / RS-06 / RL-01 (capped now); no duplicates added.

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Spec-Gap | `_docs/02_tasks/todo/AZ-290_nonfunctional_tests.md:24` | RS-04 spec says "rejects overflow"; implementation `BoundedChannelFullMode.Wait` blocks instead |
| 2 | Low | Maintainability | `SatelliteProvider.Tests/RegionRequestQueueTests.cs:35` | Time-based blocking assertion uses a 150ms `Task.Delay` heuristic |

### Finding Details

**F1: RS-04 spec wording vs. implementation behavior** (Low / Spec-Gap)
- Location: `_docs/02_tasks/todo/AZ-290_nonfunctional_tests.md:24`
- Description: RS-04 reads "Queue rejects overflow (capacity 1000)". The actual queue uses `Channel.CreateBounded<>(BoundedChannelFullMode.Wait)` (`SatelliteProvider.Services/RegionRequestQueue.cs:17-20`), which blocks the producer rather than rejecting the request. The unit tests assert the actually-implemented behavior (block + cancellable wait). Spec needs to be updated to "Queue back-pressures producer when full" in Step 12 (Test-Spec Sync).
- Suggestion: Fix in Step 12; do not change runtime behavior — back-pressure is the correct choice for an in-process producer/consumer.
- Task: AZ-290

**F2: Time-based assertion in capacity-block test** (Low / Maintainability)
- Location: `SatelliteProvider.Tests/RegionRequestQueueTests.cs:35-46` (`EnqueueAsync_BlocksWhenAtCapacity_UntilDequeue_RL02`)
- Description: The test relies on a 150 ms `Task.Delay` race to confirm that the third `EnqueueAsync` does not complete while the queue is full. This is a small heuristic risk on heavily loaded CI agents. Risk is low because the operation only completes after a real `DequeueAsync`, and the subsequent `WaitAsync(1s)` covers slow paths.
- Suggestion: Acceptable as-is; revisit if the test ever flakes on CI.
- Task: AZ-290

## Acceptance Criteria Coverage

### AZ-289

| AC | Coverage | Evidence |
|----|----------|----------|
| AC-1 — Route map processing completes within 180s | Existing | `WaitForRouteReady(routeId, 180, 3000)` in `RunRouteWithTilesZipTest` |
| AC-2 — ZIP structure validated (entry count = CSV, path prefix `tiles/`) | Existing + reinforced | Entry count check, `firstEntry.FullName.StartsWith("tiles/")`, plus new 50 MB cap |

### AZ-290

| AC | Coverage | Evidence |
|----|----------|----------|
| AC-1 — Performance script passes thresholds | New | `scripts/run-performance-tests.sh` covers PT-01..PT-06 with `check_threshold` per scenario |
| AC-2 — Resilience tests verify state transitions and resource limits | Mixed (unit + integration + structural) | RS-03 in `RegionServiceTests.ProcessRegionAsync_DownloaderFailure_TransitionsToFailedAndWritesErrorSummary` (Batch 2); RS-04 / RL-02 in new `RegionRequestQueueTests`; RS-01 / RS-02 covered by Docker startup + `DbUp` migrations; RS-05 (semaphore) and RL-03 / RL-04 (concurrency caps) verified structurally via `appsettings.json` (`MaxConcurrentDownloads: 4`, `MaxConcurrentRegions: 20`, `QueueCapacity: 1000`); RS-06 covered by existing `ComplexRouteTests` |
| AC-3 — Security tests confirm no injection or traversal vulnerabilities | New | `SecurityTests.cs` runs SEC-01 (non-numeric coord rejected), SEC-02 (multiple traversal attempts rejected), SEC-03 (oversized region rejected), SEC-04 (malformed JSON rejected) |

## Test Run

- Unit tests: 35 passed / 0 failed (Docker `mcr.microsoft.com/dotnet/sdk:8.0`).
- Integration test project: builds clean (0 warnings, 0 errors); runtime execution deferred to Step 7 (Run Tests) since it requires the full Docker Compose stack and a Google Maps API key.

## Baseline Delta

No new architecture findings. The two High findings from `architecture_compliance_baseline.md` (F1: TileService concrete dependency, F2: ISatelliteDownloader dead code) were both resolved by the earlier testability refactor that landed in the autodev baseline commit and remain resolved at the end of this batch.

## Verdict Rationale

No Critical or High findings. Two Low findings (spec wording drift, time-based test heuristic) are non-blocking and tracked for follow-up. Verdict: **PASS_WITH_WARNINGS** — proceed to commit.