# 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.