gps-denied-onboard/_docs/03_implementation/reviews/batch_01_review.md

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.