missions/_docs/03_implementation/reviews/batch_01_review.md

Code Review Report

Batch: 1 Tasks: AZ-576 (test_infrastructure) Date: 2026-05-15 Verdict: PASS_WITH_WARNINGS

Inputs

• Task spec: _docs/tasks/todo/AZ-576_test_infrastructure.md
• Changed files: 31 files under tests/ (JwksMock service + E2E.Tests project + TLS cert+key + regen-cert.sh)
• Restrictions: _docs/00_problem/restrictions.md
• Architecture: _docs/02_document/architecture.md, _docs/02_document/module-layout.md

Phase 2 — Spec Compliance

AC	Coverage	Verification
AC-1 stack boots	Skip-with-reason in InfrastructureSanity.Stack_boots_in_dependency_order_when_compose_runs	Verified at orchestration level by scripts/run-tests.sh; the TRX→CSV pipeline reports the skip with explicit reason.
AC-2 jwks-mock responds	InfrastructureSanity.Jwks_mock_serves_jwks_and_signs_tokens (SkippableFact, runs when env vars set)	Asserts JWKS body has ≥ 1 EC P-256 ES256 key.
AC-3 discovery ≥ 1 test/folder	8 Sanity.Discovery_smoke_test_runs tests + AaaPatternEnforcement	All 8 folders covered; dotnet test discovered 16 tests across 8+1 folders.
AC-4 report.csv generated	4 unit tests in TrxToCsvPostProcessorTests + manual e2e of converter	Header asserted exactly; CSV escaping covered; trait map merge covered.
AC-5 CA trust end-to-end	Bundled into AC-2 (HTTPS handshake is implicit on GET https://jwks-mock:8443/...)	A failed handshake aborts the GET.
AC-6 JWKS rotation observable	InfrastructureSanity.Jwks_rotation_returns_a_new_kid (SkippableFact)	Asserts rotation returns a kid not previously published and that the new kid joins the JWKS.
AC-7 AAA pattern enforced	AaaPatternEnforcement.Every_test_method_under_Tests_uses_AAA_markers	Regex over source files asserts ordered // Arrange? // Act // Assert markers. Test passes (16 of 16 tests are AAA-clean).

No Spec-Gap findings.

Phase 3 — Code Quality

• Clean separation of concerns: KeyStore (state) / TokenSigner (logic) / per-endpoint static handlers.
• Thread safety: KeyStore uses a single Lock gate; mutation paths are inside lock { ... }.
• Disposal: KeyStore and TestBase implement IDisposable; KeyStore.Dispose() walks both active + retired entries.
• AAA convention enforced by the AaaPatternEnforcement self-test.
• TokenSigner deliberately supports alg_override="HS256" and alg_override="none" — required for NFT-SEC-09 / NFT-SEC-10 negative tests; the surface is gated by an explicit override flag.
• No bare catches. Two narrow catch (JsonException) and catch (BadImageFormatException or FileLoadException) blocks each rethrow with context.

Phase 4 — Security Quick-Scan

• TLS keypair (tests/Azaion.Missions.JwksMock/tls/jwks-mock.key) and cert (tests/jwks-mock-ca.crt) are committed test artifacts — documented as such in regen-cert.sh. Self-signed, never used outside the test docker network.
• Mock-only alg_override paths cannot be reached without an explicit per-call override flag (the consumer never sets these; only NFT-SEC-* tests will).
• All DB access goes through Npgsql parameter substitution. The dynamic TRUNCATE in DbResetFixture uses PostgreSQL format(... %I, ...) identifier quoting against pg_tables.tablename — safe.
• No hardcoded secrets; JWT issuer / audience come from env vars.

Phase 5 — Performance

• TRX→CSV converter is single-pass over the XML.
• Reflection-based trait map iterates types/methods once (~16 methods in this assembly).
• No N+1 queries; the only DB code is fixture setup + count assertions.

Phase 6 — Cross-Task Consistency

N/A — batch contains a single task.

Phase 7 — Architecture Compliance

The test infrastructure lives entirely under tests/ — outside the documented component tree (module-layout.md only catalogs production components). No production code was modified.

• No new ProjectReference from Azaion.Missions.E2E.Tests → Azaion.Missions.csproj — blackbox boundary preserved as required by the task spec.
• JwksMock is a self-contained ASP.NET Core project; no cross-component imports.
• Reporting.Cli shares two source files with the test project via <Compile Include="..\Reporting\..." Link=...>. The test project explicitly excludes Reporting.Cli/** from compile — no double-compile, no cycle.
• No new cyclic module dependencies introduced.

Architecture findings: none.

Findings

#	Severity	Category	File:Line	Title
1	Low	Maintainability	tests/jwks-mock-ca.crt + tests/Azaion.Missions.JwksMock/tls/jwks-mock.crt	TLS cert is duplicated across two paths to satisfy the docker mount + the mock build context simultaneously. Documented in regen-cert.sh. Acceptable trade-off for deterministic test runs without cross-context build hacks.
2	Low	Maintainability	tests/Azaion.Missions.E2E.Tests/Fixtures/ComposeRestartFixture.cs	docker compose invocation from inside the e2e-consumer container will fail unless the host's docker socket is mounted. Behaviour is gated by COMPOSE_RESTART_ENABLED=1 so it cannot fire by accident; AZ-583/AZ-584 will decide whether they need this or whether to invoke compose restarts from the host runner.

Verdict

PASS_WITH_WARNINGS — 0 Critical, 0 High, 0 Medium, 2 Low. Both Low findings are infrastructure trade-offs documented in source.

Auto-Fix Attempts

0 — no eligible findings, no escalation.