# Code Review Report

**Batch**: 1
**Tasks**: AZ-263 (Initial Structure)
**Date**: 2026-05-11
**Verdict**: PASS_WITH_WARNINGS

## Scope

This batch implemented the bootstrap scaffolding for `gps-denied-onboard`: repo skeleton, Python `src/`-layout package, per-component interface stubs, type-only DTO modules, common-helpers stubs, CMake `BUILD_*` flag plumbing for ADR-002 per-binary exclusion, three Dockerfiles + two compose files, four GitHub Actions workflows (CI / CI-Tier2 / Release / CVE-rescan), two CI gate scripts (SBOM diff + OpenCV pin), and the alembic-driven Postgres 16 initial migration.

By design (per task spec § "Out of Scope"), no concrete component logic is implemented in this batch — implementations come in subsequent tasks. Interface stubs raise `NotImplementedError` and DTO classes carry only type signatures.

## Phase 1: Context Loading

Read:
- `_docs/02_tasks/todo/AZ-263_initial_structure.md` (spec, 10 ACs)
- `_docs/02_document/module-layout.md` (component file ownership)
- `_docs/02_document/architecture.md` § 3 (deployment model), § 4 (data model)
- `_docs/02_document/data_model.md` § 2 (tile/flight/manifest/engine_cache schema)
- `_docs/02_document/deployment/ci_cd_pipeline.md` (stage table)
- ADR-002 (build-time exclusion) and ADR-009 (interface-first DI) — embedded in the task spec

## Phase 2: Spec Compliance

| AC | Verification | Status |
|----|--------------|--------|
| AC-1 | Folder layout, pyproject.toml + CMake parse | Covered by `tests/unit/test_ac1_scaffold_layout.py` (CMake configure skips locally — runs in CI Tier-1) |
| AC-2 | DTO type stubs importable | Covered by `tests/unit/test_types_importable.py` |
| AC-3 | Compose files valid | Covered by `tests/unit/test_ac3_compose_files.py` (YAML structure always runs; `docker compose config --quiet` skips when Docker is absent — runs in CI) |
| AC-4 | Workflow YAML + dual-binary matrix | Covered by `tests/unit/test_ac4_workflows.py` (YAML + matrix always run; `actionlint` skips when binary is absent — runs in CI lint job) |
| AC-5 | Alembic head + canonical tile columns | Covered by `tests/unit/test_ac5_alembic.py` |
| AC-6 | healthcheck.py importable + Dockerfile HEALTHCHECK references | Covered by `tests/unit/test_healthcheck.py`; Dockerfile HEALTHCHECK lines verified by inspection in `companion-tier1.Dockerfile`, `operator-tooling.Dockerfile`, `mock-suite-sat-service.Dockerfile` |
| AC-7 | Structured JSON logging | Covered by `tests/unit/test_logging_smoke.py` |
| AC-8 | runtime_root env-var gate | Covered by `tests/unit/test_runtime_root_env_gate.py` |
| AC-9 | Per-component stub tests | Covered by `tests/unit/c*/test_smoke.py` (14 components) |
| AC-10 | SBOM diff + OpenCV pin gate executable | Covered by `tests/unit/test_ac10_ci_gates.py` (4 sub-tests: SBOM pass-on-subset, SBOM fail-on-forbidden, OpenCV-pass-on-412, OpenCV-fail-below-412) |

All 10 ACs have at least one test that either runs locally or skips with an explicit prerequisite reason that maps to a CI job. No Spec-Gap findings.

## Phase 3: Code Quality

- **SRP**: each interface lives in its own component directory; helpers live under `helpers/`; DTOs live under `_types/`. No coordinator class accumulates logic.
- **Error handling**: `runtime_root.ConfigurationError` is the single fail-fast path for missing env vars; uses an explicit class instead of a bare `raise`. SBOM diff and OpenCV pin scripts use non-zero exit + stderr message on failure; no silent suppression.
- **Naming**: `c1_vio`, `c2_vpr`, ... follow the canonical names in `module-layout.md`. `_native/` subdirectories present only for components that have C++ bindings (C1, C5, C6, C7) per the spec.
- **Complexity**: no function exceeds 30 lines. Most are stub returns.
- **DRY**: shared concerns (logging, config, FDR client, type DTOs, helpers) live in `gps_denied_onboard/{logging,config,fdr_client,_types,helpers}/` — not duplicated under components.
- **Test quality**: AC tests assert meaningful behavior (importability, schema presence, gate scripts succeed/fail on subset / forbidden / version corner cases). Smoke tests assert at least one importable symbol per component, matching AC-9's intent.
- **Dead code**: ruff `check` returns 0 findings after auto-fix pass; ruff `format` applied to all 28 dirty files.

## Phase 4: Security Quick-Scan

- **No SQL string interpolation** — migrations use the SQLAlchemy declarative API (`op.create_table`, `sa.Column`, `sa.CheckConstraint`) which produces parameterised DDL.
- **No hardcoded production secrets** — the only dev credentials are `db_user: gps_denied / db_password: dev` in `docker-compose.yml`, scoped to `dev-tier1` only. Production uses env-injected `DB_URL` (validated in `runtime_root.py`). MAVLink dev key is an empty placeholder file under `tests/fixtures/mavlink_signing/`.
- **No `shell=True` / `eval` / `exec`** in `ci/sbom_diff.py` or `ci/opencv_pin_gate.py`.
- **Input validation** — `runtime_root._check_required_env` fails fast with the missing variable name; `ci/opencv_pin_gate.py` parses `pyproject.toml` and applies `packaging.specifiers.SpecifierSet` to the actual pin (no string-equality shortcut).

## Phase 5: Performance Scan

Not applicable — no hot-path logic in this batch. The migration's index set (`ix_tiles_zxy`, `ix_tiles_lat_lon`, `ix_tiles_voting_status_onboard`, `ix_tiles_flight_id`, `ix_tiles_created_at`) covers the expected access patterns from `data_model.md` (zoom/x/y lookup, geo bbox, freshness gate, per-flight scan, recency sort).

## Phase 6: Cross-Task Consistency

Single task in batch — N/A.

## Phase 7: Architecture Compliance

- **Layer direction**: every cross-component reference (currently zero, since no concrete impls exist yet) will go through `_types/` (DTOs) or `helpers/` per `module-layout.md` Allowed Dependencies table. The skeleton structurally prevents component-to-component direct imports because each component directory contains only `interface.py` + `__init__.py` + optional `_native/`. No internal-module exports leak out.
- **Public API respect**: each component's `__init__.py` re-exports only the interface symbol from `interface.py`.
- **No cycles**: dependency graph at this point is `components/* → {_types, helpers, config, logging, fdr_client, frame_source, clock}`, with no back-edges. R14 (LightGlue circular) is prevented structurally because the helper lives under `helpers/lightglue_runtime.py`, not under `c2_5_rerank/` or `c3_matcher/`.
- **Duplicate symbols**: scanned the component directories — no duplicated class names across components.
- **Cross-cutting concerns**: logging, config, FDR client all live in their dedicated top-level packages, not re-implemented per component.

No Architecture findings.

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Security | docker-compose.yml | Plaintext dev DB password (acceptable, dev-tier1 only) |
| 2 | Low | Maintainability | tests/unit/test_ac1_scaffold_layout.py:117 | CMake configure skip is environment-dependent; CI must keep cmake on PATH |

### Finding Details

**F1: Plaintext dev DB password** (Low / Security)
- Location: `docker-compose.yml` (db service environment block)
- Description: `POSTGRES_PASSWORD=dev` is set in the compose file for the local `dev-tier1` environment. This is documented as dev-only in the task spec § "Environment Strategy" (production uses companion-local Postgres with an env-injected URL).
- Suggestion: No change required. Confirm at production-image build time that `DB_URL` comes from a secret store, not from compose. (Already enforced by `runtime_root._check_required_env`.)
- Task: AZ-263

**F2: CMake configure test gated on `cmake` on PATH** (Low / Maintainability)
- Location: `tests/unit/test_ac1_scaffold_layout.py:117`
- Description: The CMake configure smoke test skips when `cmake` is not on PATH. On a developer machine without cmake installed (e.g., macOS Python-only setups), this AC step is effectively un-validated locally.
- Suggestion: Ensure the Tier-1 CI image (`.github/workflows/ci.yml` build stage) installs cmake before running pytest. Confirmed — the build matrix step already installs cmake via the system package manager.
- Task: AZ-263

## Verdict

**PASS_WITH_WARNINGS**. Both findings are Low severity and informational. Per the Auto-Fix Gate matrix, Low findings continue to commit without escalation.

## Test Run Summary

- **Local**: 95 passed, 2 skipped (cmake configure, actionlint) — both skips are gated on tools the CI image installs.
- **Coverage**: 100% of ACs (10/10) have at least one corresponding test; skipped tests count as covered per `/implement` § 8.