mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 20:01:12 +00:00
[AZ-263] Bootstrap: repo skeleton + Docker + CI + Alembic + Tier-1 tests
Implements the AZ-263 / E-BOOT initial structure task:
- Python src/-layout package `gps_denied_onboard/` with per-component
interface stubs (14 components), type-only DTOs under `_types/`,
shared helpers under `helpers/` (R14 LightGlue ownership), structured
JSON logging, runtime composition root with env-var fail-fast gate,
healthcheck module shared by Docker and CI smoke.
- CMake top-level + `cmake/{build_options,dependencies,strategies}.cmake`
with the BUILD_* per-binary flags (ADR-002) and pinned external git
refs for OKVIS2 / VINS-Mono / GTSAM / FAISS / OpenCV >=4.12.0.
- Three Dockerfiles (companion-tier1, operator-tooling,
mock-suite-sat-service) + two compose files (dev + Tier-1 test).
- Four GitHub Actions workflows: ci.yml (lint/unit/integration/dual
binary build/SBOM diff/security), ci-tier2.yml (self-hosted Jetson
AC-bound NFTs), release.yml, cve-rescan.yml.
- Two CI gate scripts: `ci/sbom_diff.py` (deployment SBOM subset +
R02 exclusion), `ci/opencv_pin_gate.py` (>=4.12.0 enforcement,
D-CROSS-CVE-1).
- Alembic-driven Postgres 16 initial migration `0001_initial.py`
mirroring satellite-provider tiles + flights + sector_classifications
+ manifests + engine_cache_entries (data_model.md s 2).
- Tier-1 test scaffolding: 95 passing unit tests covering every AC,
per-component smoke tests, structured logging JSON output check,
env-var gate check, healthcheck import check. Two CI-gated tests
(cmake configure, actionlint) skip locally with explicit reasons.
- Batch report + code review report under `_docs/03_implementation/`.
Verdict: PASS_WITH_WARNINGS (two Low findings, both informational).
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,104 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user