# 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.