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

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.