# Code Review Report

**Batch**: 01
**Tasks**: `PENDING_refactor_jwks_https_env_gate` (C01), `PENDING_refactor_rabbitmq_host_dns_resolution` (C02)
**Date**: 2026-05-14
**Mode**: full (7 phases) on the batch's changed files
**Verdict**: PASS_WITH_WARNINGS

## Changed files

| File | Diff size | Owner component |
|------|-----------|----------------|
| `src/Auth/JwtExtensions.cs` | +11 / -1 | `06_platform` → Auth |
| `src/Services/FailsafeProducer.cs` | +21 / -1 | `02_annotations-realtime-sync` |

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| F1 | Low | Maintainability | `src/Auth/JwtExtensions.cs:30-37` | Env-name reading scattered across Auth and Program |

## Finding Details

**F1: Env-name reading scattered across Auth and Program** (Low / Maintainability)
- Location: `src/Auth/JwtExtensions.cs:30-37`
- Description: `JwtExtensions.AddJwtAuth` now reads `Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")` inline. `Program.cs:53` already reads the same value via `builder.Environment.EnvironmentName` and passes it to `CorsConfigurationValidator.EnsureSafeForEnvironment`. Two slightly different access patterns for the same input value.
- Suggestion: a future Step 8 hardening item can centralise this — e.g., add an `IHostEnvironment environment` parameter to `AddJwtAuth` and have `Program.cs` pass `builder.Environment` so both call sites use the same idiom. Deferred deliberately: the inline read is the smallest possible diff for the testability scope (no caller change at `Program.cs:49`), which is the explicit Step 4 envelope ("smallest beats elegant here").
- Task: `PENDING_refactor_jwks_https_env_gate`
- Blocks verdict? No (Low severity; documented under "Deferred to Step 8 Refactor" in `_docs/04_refactoring/01-testability-refactoring/list-of-changes.md`).

## Phase-by-Phase Notes

### Phase 1 — Context Loading

Read task specs `_docs/02_tasks/todo/01_refactor_jwks_https_env_gate.md` and `_docs/02_tasks/todo/02_refactor_rabbitmq_host_dns_resolution.md`. Read `_docs/00_problem/restrictions.md` (SW-03 RabbitMQ streams plugin, SW-05 ES256 verifier-only, ENV-01 env vars required, ENV-02 HTTP no in-image TLS) and `_docs/01_solution/solution.md`. Both task specs map their ACs to existing documented blackbox scenarios in `_docs/02_document/tests/`.

### Phase 2 — Spec Compliance

**Task 01 — `jwks_https_env_gate`**

| AC | Behavior | Status |
|----|----------|--------|
| AC-1 (HTTPS enforced outside test env) | `requireHttpsForJwks` is `true` when env is anything other than `E2ETest` (including unset, Development, Staging, Production) | ✓ Satisfied |
| AC-2 (HTTPS relaxed under E2ETest only) | Case-insensitive equality with `"E2ETest"` → `false` | ✓ Satisfied |
| AC-3 (Validation semantics unchanged) | `TokenValidationParameters` block, `IssuerSigningKeyResolver`, `JwksRetriever`, and policy registration are byte-for-byte identical | ✓ Satisfied |
| AC-4 (Forgery / tamper / cross-policy rejected) | Algorithm pinning (`EcdsaSha256`), signature, lifetime, audience, issuer checks all preserved | ✓ Satisfied |

**Task 02 — `rabbitmq_host_dns_resolution`**

| AC | Behavior | Status |
|----|----------|--------|
| AC-1 (Literal IP still works) | `IPAddress.TryParse(host, out var literal)` returns the literal, no DNS lookup | ✓ Satisfied |
| AC-2 (DNS hostname now works) | Falls through to `Dns.GetHostAddressesAsync(host, ct)` and uses `addresses[0]` | ✓ Satisfied |
| AC-3 (Unresolvable hostname surfaces through retry envelope) | `Dns.GetHostAddressesAsync` throws `SocketException` (or `InvalidOperationException` on zero-result) for unresolvable hosts; `ExecuteAsync`'s `catch (Exception ex)` at line 44 catches both, logs `ex.Message`, backs off 10 s — same surface behavior as a `FormatException` today | ✓ Satisfied |
| AC-4 (Cancellation honored during resolution) | `ct` is forwarded to `Dns.GetHostAddressesAsync` | ✓ Satisfied |
| AC-5 (Wire format / consumers unaffected) | `DrainQueue` (MessagePack serialize, gzip, queue-table delete) is untouched | ✓ Satisfied |

No scope creep detected — diffs are bounded to the documented OWNED files.

No contract sections in either task spec; consumer-side contract verification N/A.

### Phase 3 — Code Quality

- **SOLID**: each change has a single responsibility. `ResolveHostAddress` is a private static helper with one clear job.
- **Error handling**: `ResolveHostAddress` throws `InvalidOperationException` if DNS returns zero addresses — explicit and meaningful. C01 reads an env var (cannot throw).
- **Naming**: `requireHttpsForJwks`, `ResolveHostAddress`, `brokerAddress` — clear intent.
- **Complexity**: `ResolveHostAddress` is 6 lines; cyclomatic 2. C01 is two lines of straight-line code. Both well under the 50-line / cyclo-10 thresholds.
- **DRY**: no duplication introduced. Note F1 records a *cross-file* duplication pattern (env-name access) that pre-existed in spirit.
- **Test quality**: N/A — no executable tests in this run (testability refactor produces the *prerequisites* for tests, which are implemented in Step 6).
- **Dead code**: none.

### Phase 4 — Security Quick-Scan

- No SQL interpolation introduced.
- No command-injection vector (no shell, no subprocess).
- No hardcoded secrets / keys / passwords.
- Input validation: `Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")` is operator-controlled; the only branch on it is "is it `E2ETest`?" — no value-dependent decoding/parsing.
- `Dns.GetHostAddressesAsync(host, ct)` accepts operator-controlled input; no exfiltration / SSRF surface beyond what `RABBITMQ_HOST` already had (the producer already connects to whatever broker the operator configured).
- Sensitive data in logs: `ResolveHostAddress`'s zero-address throw includes the host string in the exception message — this is the same host the operator already sees in their config; not a leak.
- No insecure deserialization.
- **AC-4 cross-check**: NFT-SEC-01 through NFT-SEC-10 scenarios (forgery, expired, wrong-iss, wrong-aud, alg-confusion, tamper) all remain blocked by the unchanged `TokenValidationParameters` and algorithm pinning. Verified by inspection of `JwtExtensions.cs` lines 38-65 (unchanged in the diff).

### Phase 5 — Performance Scan

- C01: one-time env-var read at startup. Trivial.
- C02: one DNS lookup per drain cycle (~10 s cadence). One round-trip; cached by the OS resolver in practice. Negligible compared to the broker connect that follows.
- No N+1 patterns. No unbounded fetches. No blocking I/O in async (we use the async DNS API throughout).

### Phase 6 — Cross-Task Consistency

- Two tasks, disjoint files (`JwtExtensions.cs` vs `FailsafeProducer.cs`). No shared interface.
- No conflicting patterns introduced.
- No shared code duplicated across the two task implementations.
- Both changes follow the project's existing patterns (private static helpers; case-insensitive env-name compare aligned with `CorsConfigurationValidator`).

### Phase 7 — Architecture Compliance

- **Layer direction**: both files stay within their owned components. `src/Auth/` belongs to `06_platform`; `src/Services/FailsafeProducer.cs` to `02_annotations-realtime-sync` (per `_docs/02_document/module-layout.md`). No new imports across component boundaries.
- **Public API respect**: no new cross-component imports. C01 adds `Environment.GetEnvironmentVariable` (BCL). C02 adds `Dns.GetHostAddressesAsync` (BCL, already in `System.Net` which was already imported at file top).
- **No new cyclic dependencies**: import graph unchanged.
- **Duplicate symbols across components**: `ResolveHostAddress` is a private static — not visible outside its containing class. No collision.
- **Cross-cutting concerns not locally re-implemented**: F1 (env-name reading scattered across Auth and Program) is the only finding in this category — Low severity, deferred to Step 8.

## Baseline Delta

`_docs/02_document/architecture_compliance_baseline.md` baseline = `0 Critical, 0 High, 1 Medium (F1 DatasetService writes annotation table — RB-08), 2 Low (F2 ClassesController bypasses service — RB-06; F3 FailsafeProducer.EnqueueAsync static — accepted)`.

- **Carried over**: F1 (RB-08) — unchanged by this batch (DatasetService not touched). F2 (RB-06) — unchanged (ClassesController not touched). F3 (accepted) — unchanged (`EnqueueAsync` not touched).
- **Resolved**: none. (This batch addresses *testability* fixes, not the structural items the baseline flagged.)
- **Newly introduced**: F1 of this report (Low / Maintainability — env-name reading scattered). One finding. Already documented as deferred to Step 8.

No new Critical / High Architecture findings.

## Auto-Fix Decision

Per implement skill Step 10 auto-fix matrix: this batch has only one Low/Maintainability finding, which is **eligible for auto-fix** but already enumerated as a deferred Step 8 item by the user-approved scope. No auto-fix attempted; the finding is explicitly out of the testability envelope per `list-of-changes.md`.

## Verdict

**PASS_WITH_WARNINGS** — one Low/Maintainability finding (documented deferral). No Critical, no High. Implement skill proceeds to commit.