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

Code Review — Batch 04 cycle 3

Tasks reviewed: AZ-492 (Perf harness PT-07 + PT-08 + JWT-attach) Date: 2026-05-12 Verdict: PASS_WITH_WARNINGS (2 Low findings; 0 Critical/High/Medium)

Phase 1: Context

Spec inputs read: _docs/02_tasks/todo/AZ-492_perf_harness_pt07_pt08_jwt_attach.md; project restrictions / solution overview from prior batches still in force; cycle-2 retrospective Action 2 explicitly promoted this work; LESSONS.md [process] rule on Deferred-status NFRs is the governing guardrail. Changed files mapped to the AZ-492 ACs:

• SatelliteProvider.IntegrationTests/PerfBootstrap.cs (new) → AC-1 (mint surface) + AC-3 (UAV fixture) + AC-6 (no duplicate mint)
• SatelliteProvider.IntegrationTests/Program.cs (modified) → dispatch for AC-1 / AC-3 subcommands
• scripts/run-performance-tests.sh (rewritten) → AC-1 / AC-2 / AC-3
• _docs/02_document/tests/performance-tests.md → AC-4
• _docs/02_document/tests/traceability-matrix.md → AC-4 (status visibility)
• _docs/_process_leftovers/2026-05-11_perf-pt07-harness.md (deleted) → AC-5
• Module-layout + tests_integration doc updates → architectural documentation
• _docs/06_metrics/retro_2026-05-11_cycle2.md § Action 2 → process-resolution back-reference

Phase 2: Spec Compliance

AC	Status	Evidence
AC-1: PT-01..PT-06 no longer 401	Covered (static)	10 curl sites in scripts/run-performance-tests.sh all carry -H "$AUTH_HEADER"; AUTH_HEADER="Authorization: Bearer $PERF_JWT_TOKEN". The wait_region_completed polling helper also carries it (line 131). The PT-08 curl_args array carries it (line 389). Runtime verification deferred to Step 16.
AC-2: PT-07 runs to completion	Covered (static)	Two timing arrays (PT07_COLD_MS, PT07_WARM_MS) populated by 20 cold + 20 warm requests at the same coordinate set. percentile() awk helper computes p50 and p95 for both. Pass/fail asserts warm_p95 < cold_p95. Same-coordinate design guarantees the warm pass hits the cache (otherwise the cold pass would not have populated it).
AC-3: PT-08 runs to completion	Covered (static)	--gen-uav-fixture produces the JPEG once; 20 batches of 10 distinct-coordinate items uploaded. Per-batch accepted/rejected/failed counts surfaced. Batch p95 gated at 2000 ms. Per-item gate cost is a derived proxy (see Spec-vs-reality below).
AC-4: Spec status reflects implementation	Covered	performance-tests.md PT-07 and PT-08 both carry **Status**: **Implemented (AZ-492).**; "Deferred — harness work tracked in " language gone. traceability-matrix.md rows moved from ◐ recorded / ◐ recorded (Deferred) to ✓.
AC-5: Leftover drained	Covered	_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md deleted; ls _docs/_process_leftovers/ shows no files.
AC-6: Token-mint surface reused, not duplicated	Covered	rg 'new JwtSecurityToken\(' matches one source-code site only (SatelliteProvider.TestSupport/JwtTokenFactory.cs). PerfBootstrap.MintToken() is a 6-line delegation to JwtTokenFactory.Create. The shell script does not inline JWT logic — it shells out to dotnet <dll> --mint-only.

Contract verification: AZ-492 has no ## Contract section; not applicable.

Consumer-side contract verification: AZ-492 depends on AZ-487 (JWT_SECRET surface) and AZ-491 (canonical JwtTokenFactory). Both consumed correctly — the new PerfBootstrap.cs uses JwtTestHelpers.ResolveSecretOrThrow (the AZ-487-introduced env-var surface) and JwtTokenFactory.Create (the AZ-491 canonical factory). No drift.

Scope creep check: Implementation stayed within harness-only scope per spec § Excluded ("Any change to production code; this is harness-only work"). Documentation updates to module-layout / tests_integration / traceability-matrix are explicitly in scope for the doc-sync part of the spec. Retro back-reference is process hygiene, not scope creep.

Phase 3: Code Quality

SOLID:

• PerfBootstrap has a clear single responsibility: short-circuit perf-harness subcommands. Two methods, both pure-CLI dispatch + delegate. Good.
• Program.cs dispatch block is 13 lines — minimal coupling between perf-bootstrap and the rest of the runner. The decision to put it FIRST (before any HTTP / DB / env read) is correct — these subcommands should work even on a host where the API and Postgres are not running.

Error handling:

• PerfBootstrap.MintToken catches InvalidOperationException from JwtTestHelpers.ResolveSecretOrThrow and writes to stderr, returning exit code 1. Good — the shell can detect the failure cleanly.
• PerfBootstrap.GenerateUavFixture validates args.Length >= 2 and emits a usage hint on stderr with exit code 2. Good.
• Shell script: every curl capture checks the HTTP code and adds to FAIL on mismatch. The percentile() helper guards against NR == 0 to avoid div-by-zero. The dotnet --mint-only capture checks [[ -z "$PERF_JWT_TOKEN" ]] after the call to catch an empty-output failure.

Naming: PerfSubject, PerfTokenLifetime, PerfBootstrap — clear, consistent prefix. Shell variables follow the existing convention (PT07_*, PT08_*, PERF_*). AUTH_HEADER is uppercase per shell-script convention.

Complexity:

• PerfBootstrap.MintToken — 14 lines, no branches except the try/catch.
• PerfBootstrap.GenerateUavFixture — 12 lines.
• PerfBootstrap.CreateValidJpeg — 20 lines, single loop.
• scripts/run-performance-tests.sh — 430 lines total. Each PT-NN scenario is ~25–60 lines and is independently readable. The PT-08 batch loop is the longest single block (~70 lines including metadata construction); it could be extracted into a function but the inlining keeps the data flow obvious. Acceptable.

DRY:

• The same JPEG-creation pattern now exists in three places: UavTileImageFactory.CreateRandomJpeg (unit), UavUploadTests.CreateValidJpeg (integration), and PerfBootstrap.CreateValidJpeg (perf bootstrap). This is flagged as L2 below with a recommended consolidation path.

Test quality: AZ-492 spec's "Unit Tests" table notes that AC-1 (Bearer-attach helper) is only testable if a helper function is introduced — the shell script inlines the header directly, so no shell-side helper to test. AC-6 is tested by the repo-wide grep (one source site → not new). Both AC verifications above use the static-check approach the spec authorises. The mint logic itself is unit-tested in SatelliteProvider.Tests/TestSupport/JwtTokenFactoryTests (AZ-491); the PerfBootstrap.MintToken wrapper is a 6-line delegation and is exercised end-to-end by Step 16's actual perf run.

Dead code: None added. The deleted leftover file removes ~50 lines of stale process documentation.

Phase 4: Security Quick-Scan

• Token lifetime: 4 hours, mitigation for AZ-492 Risk 3 (token expiry mid-test). Tokens are minted on each script run and never persisted; the perf script's tmpdir is wiped on exit (trap-based cleanup). No JWT material ends up on disk.
• Secret handling: JWT_SECRET is read from env or .env (gitignored). Never echoed. The byte-length check fails fast for under-32-byte secrets — same contract as scripts/run-tests.sh. Good.
• PERF_JWT_TOKEN env var: documented as the operator-supplied alternative to in-script minting. If the operator pre-mints, the script does not echo the token value (only its byte length). Good.
• Subject value: perf-tests — distinct from the integration test runner subject (integration-tests) so audit logs can disambiguate the source. Good.
• permissions: GPS claim: required by AZ-488's UAV upload endpoint. Granted to the perf token so PT-08 can exercise the AC-3 path. No other permissions are minted — least-privilege ish.
• Input validation on --gen-uav-fixture: path is treated as a literal filesystem path. The script writes to $PERF_TMP_DIR/uav_fixture.jpg which is a freshly-created mktemp -d directory. No path-traversal risk in the current call site; if a future consumer passes an untrusted path it would write to that path — documented behaviour for an internal test helper.

No new attack surface introduced; no secret material touches version control; no new endpoints exposed.

Phase 5: Performance Scan

The harness IS the performance scan. Observations on the harness itself:

• dotnet build runs once per script invocation (or skipped if the DLL already exists). Build time ~5–10 s on dev hardware — acceptable for a perf run.
• dotnet <dll> --mint-only startup is ~1.5–2 s (CLR cold start + a tiny token mint). Acceptable for a one-time bootstrap.
• The cold + warm passes in PT-07 do 40 total region requests at ~200m zoom 18 — about ~40 × (5–15 s per request) = 3.5–10 minutes for PT-07 alone. PT-08 adds another 20 × (200–2000 ms) = 4–40 s. The full script run on dev hardware is in the 8–15 minute range; not fast, not glacial.
• The awk percentile helper sorts the input array — O(N log N) per call. With N=20 this is trivial.

No performance concerns in the harness code itself.

Phase 6: Cross-Task Consistency

• Naming alignment with AZ-491: PerfBootstrap.PerfSubject = "perf-tests" mirrors the AZ-491 pattern of JwtTestHelpers.DefaultSubject = "integration-tests". Consistent. PermissionsClaimType = "permissions" matches the value UavUploadTests and the API use.
• Architecture alignment with AZ-491 + AZ-493: pure / stateless logic stays in TestSupport (JwtTokenFactory, IntegrationTestResetGuard); side-effectful / dependency-bearing logic stays in the consumer (IntegrationTestDatabaseReset for Npgsql, PerfBootstrap for ImageSharp). Same boundary as batches 2 and 3. Good.
• Documentation pattern: AZ-492 follows the AZ-491 / AZ-493 doc pattern — tests_integration.md gets the runtime surface, module-layout.md gets the boundary rationale, performance-tests.md gets the test-spec status flip. Consistent.
• Duplicate test-helper detection (the Phase 6 rule added by AZ-491 review): the JPEG factory triple is flagged below as L2 — the rule fires.

Findings

L1 (Low / Spec accuracy) — AZ-492 AC-3 "per-item gate cost" satisfied by proxy, not direct measurement

Location: scripts/run-performance-tests.sh PT-08 (~line 410); _docs/02_document/tests/performance-tests.md PT-08 entry.

Issue: AC-3 says "the script reports per-item gate cost and end-to-end batch latency". The end-to-end batch latency is direct; the per-item gate cost is reported as the derived proxy batch_p95 / batch_size. True per-call UavTileQualityGate.Validate timing requires server-side instrumentation that the AZ-492 spec § Excluded explicitly excludes ("Any change to production code; this is harness-only work").

Severity rationale: Low / Spec accuracy. The deviation is a conscious trade-off documented in both the script comments AND the test-spec doc AND the traceability-matrix row (which now reads ✓ (batch p95) / ◐ (per-item proxy only)). The AC is satisfied in spirit (the harness produces a per-item number); future work on production-side timing would replace the proxy with the real value.

Suggested follow-up: PBI to add a quality_gate_validate_ms field to the per-item response or a metrics endpoint, then update the perf script to consume it. Estimate 2 SP. Sequence: after the first AZ-492 perf-gate result determines whether the proxy is misleading in practice.

L2 (Low / Maintainability) — Duplicate CreateValidJpeg-shaped JPEG factory now in THREE locations

Location:

• SatelliteProvider.Tests/TestUtilities/UavTileImageFactory.cs (unit tests — internal)
• SatelliteProvider.IntegrationTests/UavUploadTests.cs § CreateValidJpeg (integration — private)
• SatelliteProvider.IntegrationTests/PerfBootstrap.cs § CreateValidJpeg (perf bootstrap — private)

Issue: All three produce a 256×256 random-noise JPEG via ImageSharp Image<Rgba32> with random.Next(256) per channel and JpegEncoder { Quality = 95 }. The implementations differ trivially (constants, comments) but the logical surface is identical. This is exactly the cycle-2 problem that AZ-491 solved for the JWT factory.

Severity rationale: Low / Maintainability. None of the three is wrong; the issue is future drift — if the quality gate becomes pickier (e.g. enforces minimum entropy), all three factories must update in lockstep. The AZ-491 review explicitly added a Phase 6 rule for this pattern, and the rule fires here.

Suggested follow-up: Move the JPEG factory to SatelliteProvider.TestSupport/UavTileImageFactory.cs (extending the AZ-491 boundary). PerfBootstrap then becomes a one-line var bytes = UavTileImageFactory.CreateRandomJpeg();. The integration-tests UavUploadTests consumes the same surface, eliminating the third copy. Cost: 1–2 SP. The ImageSharp dependency would have to be added to SatelliteProvider.TestSupport.csproj, which is acceptable because both consumers (Tests + IntegrationTests) already depend on it.

Not a blocker for this PBI because (a) the proximate AZ-492 scope was harness-only, (b) extracting the factory in this batch would have pulled ImageSharp into TestSupport — a non-trivial architectural change that warrants its own review, and (c) the precedent is well-established (AZ-491 split the JWT factory; this is the natural sequel).

Architecture Compliance

• Layering: PerfBootstrap sits in SatelliteProvider.IntegrationTests (Layer-99 test infra). It calls into SatelliteProvider.TestSupport (Layer-99 test infra) which it already ProjectReferences. No production layer touched. The new module-layout note documents this explicitly.
• WebApi documentation convention (AZ-495): not relevant to this batch; no WebApi changes.
• Test-isolation guardrail (AZ-493): PT-07's distinct-coordinate cold pass means the test data spreads across 20 cells per run. The persistent Postgres volume + AZ-493 reset hook handle cleanup between integration-test runs; the perf script doesn't share a runner with the integration tests, but if a future operator runs PT-07 against the same volume the cells will accumulate. Not a problem in practice (the integration-test reset hook truncates tiles on the next integration run), but worth noting in the perf-gate playbook if PT-07 ever starts mis-firing due to pre-existing cells at the chosen coordinates.

Recurring patterns (for the cycle 3 retrospective)

• Spec-vs-reality on derived measurements: PT-08's "per-item gate cost" became a proxy because the spec didn't constrain the measurement path. AZ-493 had a similar pattern (DB-name vs Host-allowlist). The cycle-3 retro should capture: "ACs that prescribe a measurement should also prescribe the path for collecting it, or note that the harness gets to choose between direct measurement and a proxy."
• Triple-duplicate test fixtures: the JWT factory was consolidated in AZ-491; the JPEG factory is the natural next target. Capture as an Improvement Action for cycle 4.

Verdict

PASS_WITH_WARNINGS — implementation satisfies all 6 ACs (runtime verification of AC-1 / AC-2 / AC-3 at Step 16). Two Low findings, both deferred to future PBIs by explicit scope choices in AZ-492. No Critical/High/Medium findings.

Ready to merge / advance to the next batch.