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

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.