# Code Review Report

**Batch**: 01 (cycle 6) — AZ-505 (3 SP)
**Date**: 2026-05-12
**Verdict**: PASS

## Phase Summary

| Phase | Result |
|-------|--------|
| 1. Context Loading | OK — read `_docs/02_tasks/todo/AZ-505_tile_inventory_http2_leaflet_index.md`, `module-layout.md`, `architecture.md`, the v1.0.0 tile-storage contract, AZ-503-foundation done spec, `coderule.mdc`, `meta-rule.mdc`. Intent: ship `POST /api/satellite/tiles/inventory` + `tiles_leaflet_path` covering index + Kestrel HTTP/2, all on top of the AZ-503-foundation identity columns landed in cycle 5. |
| 2. Spec Compliance | OK — AC-1 / AC-2 / AC-3 / AC-4 / AC-5 / AC-6 / AC-7 all satisfied (see AC matrix below). Contract verification: `_docs/02_document/contracts/api/tile-inventory.md` v1.0.0 created + matches implementation Shape; `_docs/02_document/contracts/data-access/tile-storage.md` bumped to v2.0.0 with major changelog entry naming both AZ-503-foundation columns AND AZ-505 covering-index / read-rewrite as the breaking-but-additive changes (per AZ-505 AC-7). No Spec-Gap. |
| 3. Code Quality | OK after one auto-fix round (see "Auto-fixed findings" below — `ComputeLocationHash` duplicated across `TileRepository` + `TileService` consolidated into `Uuidv5.LocationHashForTile`). Single-responsibility: each new public type / method has one job. Error handling: explicit `ArgumentException` / `ArgumentNullException` on contract-invalid inputs in `TileService.GetInventoryAsync`; explicit `400 Problem` responses at the API layer; no swallowed exceptions. Comments are non-obvious-intent-only (cross-repo invariant, Dapper-bypass rationale, lock-window warning). No method longer than 50 lines outside the test seeders. |
| 4. Security Quick-Scan | OK — all SQL is parameterised (Dapper for the `LIMIT 1` path, `NpgsqlParameter` typed `Array \| Uuid` for the `ANY($1::uuid[])` path); no string interpolation in SQL; new endpoint is `.RequireAuthorization()`; the 5000-entry hard cap stops obvious request-amplification DoS; no secrets, no logging of body content. |
| 5. Performance Scan | OK — AC-4 perf gate in tests (p95 ≤ 1000 ms / 2500 tiles); inventory query is single round-trip `DISTINCT ON (location_hash)` against the new covering index leading column; no N+1; `GetTilesByLocationHashesAsync` deduplicates the request hash list before binding. `GetByTileCoordinatesAsync` rewrite is index-only-scannable on the slim `SELECT file_path` Leaflet hot path (verified by AC-3 integration test). Kestrel `Http1AndHttp2` is a pure configuration knob — no extra allocations on the hot path. |
| 6. Cross-Task Consistency | N/A — single-task batch. Cross-cycle: `tile-storage.md` v2.0.0 is the joint freeze of AZ-503-foundation (cycle 5 schema) + AZ-505 (cycle 6 read-side + covering index); consumer AZ-485 / `uav-tile-upload.md` v1.1.0 was already aligned in cycle 5; consumer AZ-316 (`gps-denied-onboard`) is gated behind `c11.use_bulk_list_endpoint=false` feature flag per the task Constraints — no in-repo consumer to verify. |
| 7. Architecture Compliance | OK — layering respected: `Api → Common/DataAccess/TileDownloader` (Layer 4 → 1+3 ✓); `TileDownloader → Common/DataAccess` (Layer 3 → 1 ✓); new `Uuidv5.LocationHashForTile` lives in `Common.Utils` (Layer 1) where it's already consumed by every downstream component — no new cross-layer or sibling-component imports. No new cycles. Public API respected: every cross-component import uses the listed Public API surface (`Uuidv5.*`, `TileSourceConverter.*`, `ITileRepository`, `ITileService`, `TileInventoryRequest`, `TileInventoryResponse`, etc.). No duplicate symbols across components after the consolidation. |

## Acceptance Criteria Coverage

| AC | Description | Test(s) | Status |
|----|-------------|---------|--------|
| AC-1 | Inventory returns one entry per request entry in input order; present/absent fields shaped per contract | `TileInventoryTests.OrderingAndPresentAbsentShaping_AC1` (25-entry interleaved, 12 mixed-source seeded + 13 absent) | Covered |
| AC-2 | `GET /tiles/{z}/{x}/{y}` returns most-recent variant via `location_hash` selection rule | `TileInventoryTests.LeafletReadReturnsMostRecentViaLocationHash_AC2` (DB-level verification of the exact SELECT used by `GetByTileCoordinatesAsync`; ServeTile is a one-line wrapper around the row and was not touched) | Covered |
| AC-3 | Leaflet hot path is `Index Only Scan using tiles_leaflet_path`, `Heap Fetches ≤ 1` | `LeafletPathIndexOnlyTests.RunAll` (10k–100k row fixture + `VACUUM ANALYZE` + EXPLAIN regex assertion, falls back to `SET enable_seqscan=off` on tiny smoke fixtures to confirm capability rather than optimiser heuristic) | Covered |
| AC-4 | p95 ≤ 1000 ms over 20 calls of 2500-entry inventory | `TileInventoryTests.PerformanceBudget_AC4` (full-suite only; smoke prints a documented skip) | Covered |
| AC-5 | 20 concurrent GETs over a single H2 connection all return `HttpVersion = 2.0` with preserved ETag + Cache-Control | `Http2MultiplexingTests.RunAll` (uses `SocketsHttpHandler { EnableMultipleHttp2Connections = false }` + `AppContext.SetSwitch("Http2UnencryptedSupport", true)` to force single-connection multiplex over h2c) | Covered |
| AC-6 | Validation: 400 on both-populated, both-empty, > 5000 entries; 401 on anonymous | `TileInventoryTests.ValidationRejects{BothPopulated,NeitherPopulated,OversizedBatch}_AC6` + `TileInventoryTests.UnauthenticatedRequestReturns401_AC6` | Covered |
| AC-7 | Contract artifacts produced in the same commit | `tile-inventory.md` v1.0.0 (new), `tile-storage.md` v2.0.0 (major bump w/ Change Log entry), `module-layout.md` + `glossary.md` + `data_model.md` + `modules/api_program.md` + `modules/dataaccess_tile_repository.md` + `components/02_data_access/description.md` + `architecture.md` updated | Covered (doc-only AC; no test required) |

All 6 functional ACs have at least one test that directly validates them. AC-7 is a documentation gate; verified by file presence + version-string + Change Log entry.

## Findings

None — all auto-fixable maintainability issues already resolved in this batch (see below).

## Auto-fixed Findings (during this review pass)

1. **F0 (Auto-fixed; Medium / Maintainability)**: `ComputeLocationHash` duplicated across `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` and `SatelliteProvider.Services.TileDownloader/TileService.cs`. Both wrapped `Uuidv5.Create(Uuidv5.TileNamespace, "{z}/{x}/{y}")` with identical bodies; this is the same anti-pattern flagged by AZ-491 retro Lesson L-002 for cross-repo identity logic. **Fix applied**: added `Uuidv5.LocationHashForTile(int z, int x, int y)` to `SatelliteProvider.Common.Utils.Uuidv5` (the single existing cross-repo-aware module for tile identity), removed both `ComputeLocationHash` helpers, updated 2 call sites in repo + 2 call sites in service + 6 call sites in tests. The `BuildTileEntity` `locationHashName` site (pre-AZ-505, AZ-503 era) was consolidated to the same helper as adjacent hygiene since it was the same formula three feet away from the call I was already touching. `idName` in `BuildTileEntity` is intentionally NOT consolidated — it uses a different name format (`"{z}/{x}/{y}/{source}/{flight_id}"`) that is a separate cross-repo invariant. **Verification**: post-fix lints clean; behaviour preserved by construction (helper inlines the exact previous code).

## Baseline Delta

No `_docs/02_document/architecture_compliance_baseline.md` exists for this project; baseline-delta section is omitted per the code-review skill's conditional emission rule.

## Notes

- **Dapper `IEnumerable` parameter expansion vs Npgsql `ANY($1::uuid[])`** — the bulk inventory query (`GetTilesByLocationHashesAsync`) deliberately bypasses Dapper. Dapper's parameter expander rewrites `IEnumerable<Guid>` into a comma-separated list of scalar placeholders, producing `ANY((@p0, @p1, ...))`, which is invalid SQL for `uuid[]` binding. The Npgsql-direct path uses an explicit `NpgsqlParameter` typed `NpgsqlDbType.Array \| Uuid` and reads via `NpgsqlDataReader` with manual `TileEntity` mapping. Inline comment in `TileRepository.GetTilesByLocationHashesAsync` documents this. Slow-query threshold (500 ms) matches the existing `GetTilesByRegionAsync` baseline.
- **Migration 015 lock window (AZ-505 Risk 2)** — DbUp wraps each script in a transaction, which is incompatible with `CREATE INDEX CONCURRENTLY`. The migration header documents this and includes both forward and back-migration SQL plus the recommended deploy window. Production deploy is a deployment-layer concern, not blocking on this PR.
- **h2c browser limitation (AZ-505 Risk 3)** — surfaced explicitly in `tile-inventory.md` v1.0.0 Non-Goals and Risks. Browser Leaflet wins come from the covering-index hot path, not multiplexing. The programmatic-client benefit (httpx `http2=True`, .NET `HttpClient`) is what AC-5 verifies.
- **AC-2 endpoint coverage shortcut** — the integration-test container does not share the API's `./tiles/` volume, so the byte-content of `GET /tiles/{z}/{x}/{y}` can't be matched against seeded JPEG files at a specific `file_path`. The AC-2 test verifies the exact SELECT statement that `TileRepository.GetByTileCoordinatesAsync` runs after the AZ-505 rewrite, against a two-row mixed-source seed. The ServeTile handler is a one-line wrapper around this row read; no other change applies. AC-3 separately verifies that the same query plan uses `Index Only Scan using tiles_leaflet_path`.

## Verdict

**PASS** — no remaining findings of any severity. One Medium / Maintainability auto-fix landed in this review pass (cross-call-site consolidation into `Uuidv5.LocationHashForTile`). Proceed to commit batch with `[AZ-505]` message.