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

Code Review Report — Batch 03 cycle 3

Batch: 03 (cycle 3) — AZ-493 (integration test DB-reset hook) Date: 2026-05-12 Verdict: PASS_WITH_WARNINGS

Findings

#	Severity	Category	File:Line	Title
1	Low	Spec-Gap	_docs/02_tasks/done/AZ-493_integration_test_db_reset_hook.md § Safety + § Risk 1	Task spec prescribed "DB name contains _test" as Guard 2; production reality uses Database=satelliteprovider and rename requires user confirmation

Finding Details

F1: Task spec prescribed "DB name contains _test" as Guard 2; production reality uses Database=satelliteprovider (Low / Spec-Gap)

• Location: _docs/02_tasks/done/AZ-493_integration_test_db_reset_hook.md § Safety (line ~98) and § Risk 1 (line ~125)
• Description: The AZ-493 spec asserted "the integration test compose file already uses a separate DB" and prescribed the second guard as "the DB name contains _test". Reading docker-compose.tests.yml showed the integration tests use the same Database=satelliteprovider as the API service. Renaming the database is gated on user confirmation per coderule.mdc ("Do not rename any databases or tables or table columns without confirmation."). The spec's "or equivalent" qualifier was used to substitute a Host allowlist (postgres, localhost, 127.0.0.1) as Guard 2.
• Suggestion: Capture this in the batch report (already done — Spec-vs-reality notes section) and consider a future PBI to align on the spec's preferred guard pattern if the team wants _test in the DB name. The current implementation provides identical safety (remote / production hostnames are rejected) and is unit-tested. No code action required. Pattern matches cycle-3 batch-1 F1 (AZ-496 Tests.csproj non-existence) — a recurring symptom of task specs encoding assumptions that aren't verified against the actual codebase before authoring.
• Task: AZ-493

Phase-by-Phase Summary

Phase	Result	Notes
1. Context Loading	OK	AZ-493 spec read; 6 ACs identified; defense-in-depth choice on _coordinateCounter documented before implementation
2. Spec Compliance	OK	6/6 ACs verified at code level. AC-6 (existing tests pass unchanged) waits on Step 16 final test gate; the structural prerequisite (Program.cs startup path is the only diff) is satisfied
3. Code Quality	OK	Clean separation of pure guard logic (IntegrationTestResetGuard — static, I/O-free, easy to test) from side-effectful reset (IntegrationTestDatabaseReset — instance, owns the Npgsql connection). Single responsibility per class. Errors surface with structured operator-friendly messages. No silent suppression. Test class has 8 well-isolated [Fact] / [Theory] cases following Arrange/Act/Assert.
4. Security Quick-Scan	OK	The guard's express purpose is preventing accidental truncate against non-test databases. Two independent checks (env sentinel + Host allowlist) — either alone would have prevented the cycle-2 scenario; together they reduce the false-positive risk to near zero. Unit tests confirm the rejection paths with representative production-shape hostnames
5. Performance Scan	OK	Reset is O(table-count) Npgsql round-trips wrapped in a single transaction; AZ-493 NFR budget (< 1 s on O(10K) rows) is comfortably within Postgres TRUNCATE behavior
6. Cross-Task Consistency	OK	The new IntegrationTestResetGuard lives in TestSupport alongside the AZ-491 JwtTokenFactory — both are pure utility surfaces consumed by both test projects. The boundary is enforced: IntegrationTestDatabaseReset (Npgsql-bearing) stays in IntegrationTests so the Npgsql dependency does not leak into unit tests. This explicit separation is the architectural improvement the AZ-491 batch teed up
7. Architecture Compliance	OK	No production-component change. TestSupport's role as a shared/cross-cutting test library is reinforced (now serves two cycle-3 PBIs). No new ProjectReferences in production code. No cycles. Layering invariant preserved: TestSupport is below both test projects; production projects do NOT reference TestSupport

Cross-cutting observations (info, no finding)

• The guard's AllowedHosts is a constant list, not a configurable env var. This is intentional — the safety guarantee weakens if operators can override it. A future PBI that needs to support a non-listed Host (e.g., a Kubernetes pod hostname for ephemeral test environments) would extend the list in source, not via runtime config.
• The decision to retain the wallclock-seeded _coordinateCounter in UavUploadTests is recorded as defense-in-depth. AC-2 explicitly allowed either approach; the batch chose retention because the additional code surface is minimal and the safety benefit (against --keep-state collisions) is non-trivial. The retention comment back-references AZ-493 so future agents understand why the cycle-2 workaround is still there.

Baseline Delta

Class	Count	Notes
Carried over	0	No Architecture findings; nothing recurring
Resolved	1 (informal)	The cycle-2 Pattern 5 (Integration test state leakage from persistent Postgres volume) is structurally closed by this batch. Tracked in retro_2026-05-11_cycle2.md § 6 Pattern 5; not an Architecture-class baseline entry, so it does not appear in the cycle-1 baseline
Newly introduced	0	—

Verdict Logic

• 0 Critical, 0 High, 0 Medium, 1 Low → PASS_WITH_WARNINGS
• The Low finding is the spec-vs-reality observation about Guard 2 — documented, mitigated, and unit-tested. Does not block commit per the implement-skill auto-fix gate.

Recommendation to /implement

Proceed to commit + push + tracker transition (Steps 11-13).