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

Code Review Report

Batch: 2 Tasks: AZ-266 (Shared Logging Module), AZ-269 (Config Loader), AZ-277 (SE3Utils Helper), AZ-280 (Sha256Sidecar Helper) Date: 2026-05-11 Verdict: PASS_WITH_WARNINGS

Scope

Batch 2 lays the cross-cutting foundation: structured JSON logging that satisfies the log_record_schema v1.0.0 contract, the precedence-aware config loader that materialises the frozen Config container, and two Layer-1 helpers (SE3 math, atomic-write + SHA-256 sidecar) that dozens of downstream component tasks depend on.

Phase 1: Context Loading

Read:

• _docs/02_tasks/todo/AZ-266_log_module.md (5 ACs + 2 NFRs)
• _docs/02_tasks/todo/AZ-269_config_loader.md (6 ACs + 2 NFRs)
• _docs/02_tasks/todo/AZ-277_se3_utils.md (9 ACs + 2 NFRs)
• _docs/02_tasks/todo/AZ-280_sha256_sidecar.md (9 ACs + 2 NFRs)
• Four contracts under _docs/02_document/contracts/ (log_record_schema, composition_root_protocol, se3_utils, sha256_sidecar)
• _docs/02_document/module-layout.md (ownership envelopes)

Ownership envelopes resolved per module-layout.md:

• AZ-266 owns src/gps_denied_onboard/logging/** + tests/unit/test_logging_*.py + new test_az266_logging_schema.py
• AZ-269 owns src/gps_denied_onboard/config/** + tests/unit/test_az269_config_loader.py
• AZ-277 owns src/gps_denied_onboard/helpers/se3_utils.py + tests/unit/test_az277_se3_utils.py
• AZ-280 owns src/gps_denied_onboard/helpers/sha256_sidecar.py + tests/unit/test_az280_sha256_sidecar.py

pyproject.toml was amended additively in this batch to pin the named-backend dependencies (gtsam>=4.2,<5.0 and atomicwrites>=1.4,<2.0) that the AZ-277 / AZ-280 contracts explicitly call out. AZ-263 left these unpinned — this batch closes the gap.

Phase 2: Spec Compliance

AZ-266 — Shared Logging Module

AC	Verification
AC-1 Single entrypoint	test_ac1_get_logger_returns_logger_with_schema_formatter
AC-2 Field order	test_ac2_field_order_stable_regardless_of_construction_order parses raw JSON and asserts tuple-equal order against CONTRACT_ORDER
AC-3 Level normalisation	test_ac3_level_warning_normalises_to_warn + parametrised pass-through for DEBUG/INFO/ERROR
AC-4 Handler topology	test_ac4_handler_topology_no_duplicates_on_reinit (Tier-1; Tier-2 journald handler factory exists but is a runtime-prerequisite skip on macOS dev)
AC-5 Non-frame records	test_ac5_non_frame_records_emit_explicit_null_frame_id
NFR-reliability	test_nfr_formatter_never_raises_on_unserialisable_kv + test_nfr_multi_line_msg_is_collapsed

NFR-performance (p99 ≤ 0.2 ms on Tier-2) is a Tier-2 microbenchmark; it runs under tests/perf/ (separate file, gated by pytest.mark.tier2). A formatter cold-bench microbenchmark in unit scope would need a hardware-specific budget; the unit suite explicitly leaves this to Tier-2. Recorded as a known follow-up below.

AZ-269 — Config Loader

AC	Verification
AC-1 env > YAML	test_ac1_env_beats_yaml
AC-2 YAML > defaults	test_ac2_yaml_beats_default_when_env_silent
AC-3 defaults fill gaps	test_ac3_defaults_fill_gaps
AC-4 multi-YAML order	test_ac4_later_yaml_path_wins
AC-5 frozen end-to-end	test_ac5_config_is_frozen_end_to_end
AC-6 missing-var fail-fast	test_ac6_missing_required_env_var_fails_with_pointer
NFR-reliability	test_nfr_reliability_loader_is_pure

NFR-performance (≤ 250 ms cold load on Tier-2) is a hardware-specific budget recorded as a Tier-2 follow-up.

AZ-277 — SE3Utils Helper

AC	Verification
AC-1 matrix/SE3 round-trip	test_ac1_matrix_se3_roundtrip_within_tolerance (20 random Ts)
AC-2 Lie round-trip	test_ac2_lie_algebra_roundtrip (20 random ξ)
AC-3 near-identity stability	test_ac3_near_identity_exp_map_is_stable
AC-4 strict orthogonality	test_ac4_strict_orthogonality_rejection
AC-5 mirror rejected	test_ac5_mirror_rotation_rejected
AC-6 bottom-row guard	test_ac6_bottom_row_guard
AC-7 dtype contract	test_ac7_float32_dtype_rejected
AC-8 determinism	test_ac8_determinism_byte_equal_outputs
AC-9 no upward imports	test_ac9_no_upward_imports_to_components (AST scan)

The implementation backs every primitive against GTSAM Pose3 directly, re-exports SE3 = gtsam.Pose3 per contract, and uses the small-angle Taylor cutoff (|xi| < 1e-10) for AC-3.

AZ-280 — Sha256Sidecar Helper

AC	Verification
AC-1 round-trip	test_ac1_roundtrip_write_and_verify (1 MiB seeded payload)
AC-2 atomicity	test_ac2_atomicity_no_partial_file_on_fault (monkeypatches atomicwrites.replace_atomic)
AC-3 independent verify	test_ac3_independent_verification_rejects_swapped_payload
AC-4 missing sidecar raises	test_ac4_missing_sidecar_raises
AC-5 malformed sidecar rejected	test_ac5_malformed_sidecar_rejected
AC-6 aggregate determinism	test_ac6_aggregate_hash_order_deterministic
AC-7 aggregate missing-path	test_ac7_aggregate_hash_missing_path_raises
AC-8 sidecar format strict	test_ac8_sidecar_format_strictness (64 hex chars, no JSON, no newline, no whitespace)
AC-9 no upward imports	test_ac9_no_upward_imports_to_components (AST scan)

The implementation backs write_atomic with atomicwrites.atomic_write; the sidecar is written via the same atomic primitive; verify streams the file from disk (no in-memory shortcut) and rejects a missing / malformed sidecar with Sha256SidecarError.

No Spec-Gap findings.

Phase 3: Code Quality

• SRP — each module owns exactly one concern (the formatter does not also own handler topology; the loader does not own composition; the helpers are pure functions / static methods with no module state).
• Error handling — all four modules use a typed exception (RequiredFieldMissingError, Se3InvalidMatrixError, Sha256SidecarError) wrapping the underlying OSError / ValueError, so callers handle one hierarchy. The logging formatter never raises into the caller; un-serialisable kv is replaced with an {"_format_error": "..."} payload.
• Naming — public symbols match the contract files verbatim.
• Complexity — no function exceeds 40 lines; cyclomatic complexity is at most ~5 (the formatter's record-shaping loop).
• DRY — shared sidecar-path computation, contract-field tuple, and reserved-stdlib-keys frozenset all live in module-level constants.
• Test quality — every AC has a directly-mapped test that asserts the contractually-named behaviour; no test bottoms out on "no exception raised". AST-based no upward imports scans for AC-9 rather than relying on a string match.
• Dead code — helpers/se3_utils.py previously stubbed compose / inverse that aren't in the contract; they were dead and were replaced. gps_denied_onboard/config/loader.py previously had a load_runtime_config stub that raised NotImplementedError; it has been replaced with the real load_config per contract — symbol name changed (load_runtime_config -> load_config) per the contract. No other call sites yet (verified via grep).

Phase 4: Security Quick-Scan

• No SQL string interpolation in any batch-2 module (loader does no DB access; the config DB_URL is opaque to the loader).
• No shell=True / eval / exec anywhere in the batch.
• No hardcoded secrets — MAVLink dev key remains an empty placeholder under tests/fixtures/ (introduced in batch 1).
• No insecure deserialization — yaml.safe_load is used in loader.py, not yaml.load. JSON serialisation in the logging formatter uses default=str only as a fallback after _coerce_jsonable has already replaced un-serialisable values.
• Atomic-write threat model: documented in the AZ-280 contract — the helper protects against accidental corruption + file-replacement, NOT against an attacker with write access. The sidecar is a corruption detector, not a signature. The mid-flight tile-gen signing key path remains the source of authenticated integrity (out of scope here).

No security findings.

Phase 5: Performance Scan

• The logging formatter avoids a stdlib logging.Formatter super-call and renders the record directly into a dict, then json.dumps once. No O(n^2) loops; no transient dict copies of kv on the hot path except where the caller doesn't pass an explicit kv (in which case the formatter materialises one from record.__dict__).
• The SHA-256 helper streams large files via 1 MiB chunks in _digest_file, avoiding the contract API's "payload: bytes" RAM limit when verifying.
• The config loader does one YAML read per file and one shallow merge. No N+1 patterns.

NFR microbenchmarks for cold-load latency (AZ-269) and per-record formatter latency (AZ-266) need Tier-2 hardware to verify against the documented budgets; they are out of unit-test scope.

Phase 6: Cross-Task Consistency

• gps_denied_onboard.logging imports nothing from config or helpers, so the cross-cutting modules form a strict layering: logging ⊥ config ⊥ helpers/*. The composition root (AZ-270, next batch) will wire them together; this batch does not.
• The register_component_block(slug, dataclass) registry in config/schema.py is the single mechanism by which a component epic (AZ-25x..AZ-26x) contributes its config block. The collision check raises ConfigError at registration time, satisfying AZ-269 § Risks & Mitigation Risk 1.
• All four modules share the convention "typed exception wraps underlying errors" (Se3InvalidMatrixError, Sha256SidecarError, RequiredFieldMissingError, no leaking OSError/ValueError).

No cross-task consistency findings.

Phase 7: Architecture Compliance

Per module-layout.md § Allowed Dependencies + § Per-Component Mapping:

• helpers/* (Layer 1): allowed imports are _types, stdlib, and named external deps (GTSAM, numpy, atomicwrites, hashlib, pathlib). AST scans (AC-9 in both helper test files) verify no gps_denied_onboard.components.* imports exist. PASS.
• logging (Layer 0 / cross-cutting): allowed imports are stdlib only. Verified — structured.py imports datetime, json, logging, os, sys, threading, plus optional systemd.journal inside the Tier-2 handler factory. PASS.
• config (Layer 0 / cross-cutting): allowed imports are stdlib + pyyaml. Verified. PASS.

No new cycles introduced (verified by python -c "import gps_denied_onboard.logging, gps_denied_onboard.config, gps_denied_onboard.helpers" succeeding without a circular-import error during the test run).

No duplicate symbols across components.

No locally re-implemented cross-cutting concern.

No Architecture findings.

Findings

#	Severity	Category	File:Line	Title
1	Low	Maintainability	tests/unit/test_az269_config_loader.py:test_ac2	AC-2 helper test pivots on fdr.queue_size rather than log.level
2	Low	Performance	tests/unit/test_az266_logging_schema.py	NFR-perf microbenchmark deferred to Tier-2 perf suite
3	Low	Maintainability	src/gps_denied_onboard/logging/structured.py	Tier-2 journald handler unverified on macOS dev (requires systemd-python)
4	Low	Scope	pyproject.toml	Batch 2 added gtsam + atomicwrites deps that AZ-263 had not pinned

Finding Details

F1: AC-2 helper test pivots on fdr.queue_size (Low / Maintainability)

• Location: tests/unit/test_az269_config_loader.py::test_ac2_yaml_beats_default_when_env_silent
• Description: The spec wording for AC-2 is "YAML wins over defaults when env is silent" using log.level. Because LOG_LEVEL is also in the required-env set, the most natural test pivots on a non-required field (fdr.queue_size) to demonstrate YAML > defaults cleanly without also having to remove a required env var. The test does demonstrate the AC, just on a different key.
• Suggestion: Document this choice in a comment, or split AC-2 into two ACs (env-silent for required field vs optional field). No code change required this batch.
• Task: AZ-269

F2: NFR-perf microbenchmark deferred (Low / Performance)

• Location: AZ-266 NFR-perf + AZ-269 NFR-perf
• Description: Both tasks declare Tier-2 latency budgets (formatter p99 ≤ 0.2 ms; cold load ≤ 250 ms). Unit-suite microbenchmarks cannot verify against the named hardware target. The batch does not block on this — perf NFRs are owned by the Tier-2 perf suite.
• Suggestion: When the Tier-2 perf suite lands (AZ-428..AZ-431), add a tests/perf/test_logging_formatter_p99.py and tests/perf/test_config_cold_load.py. Capture as a follow-up but not a blocker.
• Task: AZ-266, AZ-269

F3: Tier-2 journald handler unverified on macOS dev (Low / Maintainability)

• Location: src/gps_denied_onboard/logging/structured.py::_make_tier2_handler
• Description: The Tier-2 handler factory imports systemd.journal.JournalHandler lazily. On macOS dev (current host), systemd-python is not installable, so the factory raises a clear RuntimeError instead of falling back to stderr (intentional — falling back would violate AC-4 "exactly one journald handler"). The factory is unit-tested only via the failure-path RuntimeError.
• Suggestion: Verify Tier-2 in the Jetson CI runner once ci-tier2.yml is wired up against real hardware. Capture as a follow-up.
• Task: AZ-266

F4: pyproject.toml dep amendment (Low / Scope)

• Location: pyproject.toml::dependencies
• Description: Batch 2 added gtsam>=4.2,<5.0 and atomicwrites>=1.4,<2.0 because the AZ-277 and AZ-280 contracts explicitly name these as the backend. AZ-263 left them unpinned. Strictly, the deps belong to the consumer task (this batch), but they are runtime deps and live in the same pyproject.toml AZ-263 created.
• Suggestion: No change required; the amendment is the cleanest path. Recorded so the AZ-263 implementation report and the Product-Implementation-Completeness audit reflect the batch-2 dep addition.
• Task: AZ-277, AZ-280

Verdict

PASS_WITH_WARNINGS. Four Low-severity findings, all informational follow-ups (perf microbenchmarks for Tier-2, journald verification on Jetson, and the documented dep amendment). Per the Auto-Fix Gate matrix, Low findings continue to commit without escalation.

Test Run Summary

• Local: 139 passed, 2 skipped (cmake configure, actionlint — both CI-gated). Includes 44 new batch-2 tests.
• Coverage: every AC across the four tasks has at least one corresponding test. NFR-perf budgets are deferred to Tier-2 per the Findings section.