# Hygiene — Consolidate c1_vio strategy facade orchestration spine

**Task**: AZ-528_hygiene_c1_vio_facade_spine_consolidation
**Name**: c1_vio strategy facade orchestration-spine helper consolidation
**Description**: Replace the 3-way byte-equivalent orchestration-spine duplication across the c1_vio `VioStrategy` modules (`okvis2.py`, `vins_mono.py`, `klt_ransac.py`) with a single c1-internal helper module at `src/gps_denied_onboard/components/c1_vio/_facade_spine.py`. Closes cumulative review batches 52-54 Finding F1 (Medium / Maintainability).
**Complexity**: 3 points
**Dependencies**: AZ-334 (the trigger that escalated the duplication from 2-way to 3-way; all 3 callers now exist). AZ-332 + AZ-333 are already in `done/`.
**Component**: c1_vio (epic AZ-254 / E-C1)
**Tracker**: AZ-528
**Epic**: AZ-254 (E-C1)

### Document Dependencies

- `_docs/03_implementation/cumulative_review_batches_52-54_cycle1_report.md` § F1 — the finding being closed.
- `_docs/02_document/components/01_c1_vio/description.md` — the c1_vio component description (helper goes inside the c1_vio boundary).
- `_docs/02_tasks/done/AZ-332_c1_okvis2_strategy.md`, `AZ-333_c1_vins_mono_strategy.md`, `AZ-334_c1_klt_ransac_strategy.md` — the three task-specs whose existing AC tests must continue to pass unmodified after the consolidation.

## Problem

Three c1_vio `VioStrategy` modules each duplicate the same orchestration-spine helpers:

**Module-level free functions (byte-identical across all 3)**:

| Function | File | Notes |
|----------|------|-------|
| `_now_iso() -> str` | `okvis2.py`, `vins_mono.py`, `klt_ransac.py` | ISO-8601 UTC timestamp for FDR `ts` |
| `_bias_norm(bias: ImuBias) -> float` | `okvis2.py`, `vins_mono.py`, `klt_ransac.py` | L2 norm of `(accel ‖ gyro)` 6-vector |
| `_se3_from_4x4(matrix) -> gtsam.Pose3` | `okvis2.py`, `vins_mono.py`, `klt_ransac.py` | Lazy gtsam import + np-array coerce |
| `_frame_ts_ns(frame: NavCameraFrame) -> int` | `okvis2.py`, `vins_mono.py` | UTC-epoch ns from `frame.timestamp` (KLT/RANSAC uses inline access) |
| `_frame_image(frame: NavCameraFrame) -> np.ndarray` | `okvis2.py`, `vins_mono.py` | Contiguous uint8 ndarray + 2D/3D shape check (KLT/RANSAC owns inline grayscale conversion) |

**Instance methods (byte-identical modulo strategy-local config-knob references + module-level constants)**:

| Method | Files | Differences |
|--------|-------|-------------|
| `_classify_state(self, fq) -> VioState` | all 3 | Threshold attribute reference: `self._okvis2_cfg.degraded_feature_threshold` vs `self._cfg.min_features_for_pose` |
| `_tick_lost(self, frame_id)` | all 3 | Byte-identical — increments `_consecutive_lost`, escalates to LOST at `_lost_frame_threshold` |
| `_emit_transition(self, new_state, frame_id)` | all 3 | Differs only in module-level `_PRODUCER_ID` + `_STRATEGY_LABEL` constants captured at emit time |

The geometry-specific pipeline (OKVIS2 cascade init + native binding driver, VINS-Mono loosely-coupled estimator driver, KLT/RANSAC seed/track/recoverPose ladder) is unique to each strategy and stays inside each strategy module — this PBI does not touch the geometry pipelines.

The per-batch reviews for B53 + B54 explicitly deferred consolidation to the next cumulative review (batches 52-54). That cumulative review formally raised the finding from Low to Medium and opened this ticket.

## Outcome

- A new c1-internal module `src/gps_denied_onboard/components/c1_vio/_facade_spine.py` exposes:
  - Free functions: `now_iso()`, `bias_norm(bias)`, `se3_from_4x4(matrix)`, `frame_ts_ns(frame)`, `frame_image(frame, *, producer_id)`.
  - A `FacadeSpine` mixin class providing `_classify_state(self, fq) -> VioState`, `_tick_lost(self, frame_id) -> None`, `_emit_transition(self, new_state, frame_id) -> None`. The mixin reads strategy-specific values via per-instance attributes that each concrete strategy must set: `_feature_threshold: int`, `_warm_start_max_frames: int`, `_lost_frame_threshold: int`, `_producer_id: str`, `_strategy_label: str`, plus the existing `_reported_state`, `_frames_since_warmup`, `_consecutive_lost`, `_last_emitted_state`, `_latest_bias`, `_fdr` attributes.
- The three c1_vio strategy modules import the helpers + inherit from the mixin. The duplicated bodies are deleted. The deferred-consolidation comments referencing this PBI are also deleted.
- A new unit test `tests/unit/c1_vio/test_az528_facade_spine.py` covers AC-1..AC-7 below.
- The existing AZ-332 / AZ-333 / AZ-334 AC tests pass unmodified.

## Scope

### Included

- Add `src/gps_denied_onboard/components/c1_vio/_facade_spine.py` with the listed free functions + `FacadeSpine` mixin. Import surface: only L1/L2 substrate (`_types.nav`, `components.c1_vio.errors`, `_types.fdr_record`, stdlib datetime + math + numpy).
- Migrate the three c1_vio strategy modules to import the free functions and inherit from `FacadeSpine`. Delete the duplicated bodies + any in-line `post-AZ-334 hygiene` / `Batch 53 review F1` / `Batch 54 review F1` deferred-consolidation comments.
- Add `tests/unit/c1_vio/test_az528_facade_spine.py` with AC-1..AC-7 (3 free-function tests + 3 mixin-behaviour tests + 1 regression-guard test).
- Re-run the existing `tests/unit/c1_vio/test_okvis2_strategy.py`, `test_vins_mono_strategy.py`, `test_klt_ransac_strategy.py` AC sub-tests unmodified to verify the consolidation preserves behaviour at every call site.

### Excluded

- Sharing the spine across other components (c2, c3, c4, c5). Each component owns its own FDR record kind + state machine; sharing across components would entangle the component boundaries that AZ-507 carved cleanly.
- Refactoring the strategy-specific geometry pipelines (OKVIS2 cascade init, VINS-Mono estimator driver, KLT/RANSAC pose-recovery ladder). Those are correctly per-strategy.
- Changing the `vio.health` FDR record schema, the `VioState` enum values, or the `CURRENT_SCHEMA_VERSION` constant.
- Refactoring the test fakes in `tests/unit/c1_vio/conftest.py` (`FakeOkvis2Backend` / `FakeVinsMonoBackend`) or the `_patch_pose_recovery` helper in `test_klt_ransac_strategy.py`. These are F2 from cumulative review batches 52-54; they sit at a different abstraction layer and will ride a later hygiene pass.
- Hoisting the spine to `src/gps_denied_onboard/helpers/`. Strategy state machines + FDR record producers are c1-internal concerns by component-architecture decision.

## Acceptance Criteria

**AC-1: Helper module exists at the canonical path with the expected surface**
Given a fresh checkout
When `from gps_denied_onboard.components.c1_vio._facade_spine import now_iso, bias_norm, se3_from_4x4, frame_ts_ns, frame_image, FacadeSpine` is run
Then all imports succeed; `FacadeSpine` is a class; `now_iso`, `bias_norm`, `se3_from_4x4`, `frame_ts_ns`, `frame_image` are callables; `FacadeSpine` has methods `_classify_state`, `_tick_lost`, `_emit_transition`

**AC-2: `now_iso()` returns ISO-8601 UTC**
Given the helper imported
When `now_iso()` is called
Then the return value is a string that parses as an aware UTC `datetime` via `datetime.fromisoformat(...)` and the offset is `+00:00` (matching the existing OKVIS2 / VINS-Mono / KLT-RANSAC behaviour — NOT the `Z`-suffix variant; that is `iso_ts_from_clock` in AZ-526)

**AC-3: `bias_norm(bias)` matches the L2 formula**
Given an `ImuBias(accel_bias=(1.0, 2.0, 2.0), gyro_bias=(0.0, 0.0, 0.0))`
When `bias_norm(bias)` is called
Then the result equals `3.0` (sqrt(1+4+4))

**AC-4: `se3_from_4x4(matrix)` builds a `gtsam.Pose3`**
Given a 4×4 identity matrix
When `se3_from_4x4(np.eye(4))` is called
Then the returned object is a `gtsam.Pose3` whose translation is `(0, 0, 0)` and whose rotation is the identity

**AC-5: `FacadeSpine._classify_state` mirrors the existing logic across all three strategies**
Given a `FacadeSpine` subclass instance with `_reported_state=INIT`, `_frames_since_warmup=0`, `_warm_start_max_frames=5`, `_feature_threshold=50`
When `_classify_state(FeatureQuality(tracked=80))` is called
Then the return is `VioState.INIT`; after `_frames_since_warmup=5`, the same call returns `VioState.TRACKING`; with `tracked=10`, it returns `VioState.DEGRADED`

**AC-6: `FacadeSpine._tick_lost` transitions correctly**
Given `_reported_state=TRACKING`, `_consecutive_lost=0`, `_lost_frame_threshold=3`
When `_tick_lost("frame_42")` is called once
Then `_reported_state == VioState.DEGRADED`; calling 2 more times escalates `_reported_state == VioState.LOST`

**AC-7: `FacadeSpine._emit_transition` emits exactly one FDR record per state change**
Given `_last_emitted_state=TRACKING`, a fake `FdrClient`
When `_emit_transition(VioState.TRACKING, "frame_42")` is called (no state change)
Then no record is enqueued
When `_emit_transition(VioState.DEGRADED, "frame_42")` is called (state change)
Then exactly one FDR record is enqueued with `kind="vio.health"`, the payload contains `{state, consecutive_lost, bias_norm, strategy_label, frame_id}`, and `_last_emitted_state == VioState.DEGRADED`

**AC-8: No stray duplicated definitions remain in the three strategy modules**
Given `grep -rn "^def _now_iso\|^def _bias_norm\|^def _se3_from_4x4\|^def _frame_ts_ns\|^def _frame_image" src/gps_denied_onboard/components/c1_vio/` after the task lands
When the search runs
Then matches appear only inside `_facade_spine.py`; zero matches in `okvis2.py`, `vins_mono.py`, `klt_ransac.py`

**AC-9: All AZ-332 / AZ-333 / AZ-334 existing AC tests pass unmodified**
Given the existing `tests/unit/c1_vio/test_okvis2_strategy.py`, `test_vins_mono_strategy.py`, `test_klt_ransac_strategy.py`
When the suites run after this task
Then every previously-passing AC sub-test still passes; no test file outside the new `test_az528_facade_spine.py` is modified

**AC-10: AZ-270 layer lint still passes**
Given the helper lives inside the c1_vio component (Layer 3) and only imports from L1 substrate
When `tests/unit/test_az270_compose_root.py::test_ac6_only_compose_root_imports_concrete_strategies` runs
Then the test passes (helper is c1-internal, underscore-prefixed, not in `c1_vio/__init__.py`'s Public API surface)

## Constraints

- The helper module is c1-internal (`_facade_spine.py` underscore prefix). No other component may import it.
- Strategy-specific values (`_feature_threshold`, `_producer_id`, `_strategy_label`, etc.) enter the mixin through `self` attributes set by each concrete strategy's `__init__` — NOT through constructor parameters on the mixin. Keeps the mixin shape stable.
- The free functions are stateless and pure. They MUST NOT capture module-level state from any strategy module.
- Error envelope preserved: `_frame_image` continues to raise `VioFatalError` on bad shape; no new error types introduced.
- `_now_iso()` keeps the existing `+00:00` offset format (not `Z`) to maintain byte-equivalence with the current OKVIS2 / VINS-Mono / KLT-RANSAC behaviour. The canonical `Z`-suffix helper is `iso_ts_from_clock` in `helpers/iso_timestamps.py` (AZ-526) which serves a different purpose (FDR record timestamps from an injected `Clock`).
- Test additions go into a new `test_az528_facade_spine.py`; AZ-332 / AZ-333 / AZ-334 test files are NOT touched.
- The c1_vio module-level `_PRODUCER_ID` and `_STRATEGY_LABEL` Final constants remain in each strategy module (they parametrise the mixin via the `self._producer_id` / `self._strategy_label` attributes the constructor sets). Hoisting them to `_facade_spine.py` would lose per-strategy identity.

## Risks & Mitigation

**Risk 1: The mixin's attribute dependencies (`_feature_threshold`, `_warm_start_max_frames`, etc.) are not declared statically — a strategy forgetting to set one breaks the mixin at runtime, not at type-check time.**
- *Mitigation*: Document the required attributes in the `FacadeSpine` class docstring + introduce a `_facade_spine_attrs` property class-level check that runs at `__init_subclass__` time (or a `_validate_spine_attrs()` instance helper called from each concrete `__init__`). Add a unit test that explicitly verifies the three concrete strategies have all required attributes set after construction.
- *Alternative*: a stricter Protocol-typed parameter object passed to `_classify_state` / `_emit_transition`. Rejected — it would force every call site to construct a parameter object, which is more boilerplate than the bug it prevents.

**Risk 2: A 3-way refactor accidentally changes the state-machine semantics for one of the strategies**
- *Mitigation*: AC-9 (all 3 existing AC test suites pass unmodified) is the safety net — if any strategy regresses, its AC tests will fail. Plus the focused AC-5..AC-7 tests in the new file directly exercise the mixin logic in isolation.

**Risk 3: A future c1_vio strategy author forgets the mixin exists and adds a 4th local copy**
- *Mitigation*: Add an AST-walk regression guard inside `test_az528_facade_spine.py` (modeled on the AZ-508 / AZ-526 / AZ-527 pattern) that asserts zero module-level `_now_iso` / `_bias_norm` / `_se3_from_4x4` / `_frame_ts_ns` / `_frame_image` definitions in any of the 3 strategy modules.

**Risk 4: Inheritance from a mixin couples the three strategies in a way that constrains future divergence**
- *Risk*: A future strategy might need a different state-machine — e.g., a smoother-loop strategy with an extra "INITIALIZING" state.
- *Mitigation*: The mixin is opt-in (free function `_emit_transition` is also exposed for strategies that want the FDR-record format but not the state machine). A future divergent strategy can either subclass `FacadeSpine` and override the relevant methods or skip the mixin entirely and use only the free functions.

## Runtime Completeness

- **Named capability**: a c1-internal helper module `_facade_spine.py` exposing 5 free functions (`now_iso`, `bias_norm`, `se3_from_4x4`, `frame_ts_ns`, `frame_image`) + 1 mixin class (`FacadeSpine`) providing `_classify_state` / `_tick_lost` / `_emit_transition`.
- **Production code that must exist**: real implementations in `_facade_spine.py`; real imports + inheritance in each of the 3 strategy modules (`okvis2.py`, `vins_mono.py`, `klt_ransac.py`); existing module-level `_PRODUCER_ID` / `_STRATEGY_LABEL` / `_lost_frame_threshold` / `_warm_start_max_frames` / `_feature_threshold` constants preserved with appropriate `self.*` attribute assignments in each strategy's `__init__`.
- **Allowed external stubs**: none for production code. The unit test uses a minimal `FacadeSpine` subclass (test-only) + a fake `FdrClient` matching the existing AZ-273 ringbuf shape.
- **Unacceptable substitutes**: keeping one or more local definitions "for parity"; raising a different exception type; hoisting the helper to `helpers/`; changing the existing FDR record payload shape; bundling the test-fake consolidation (F2) into this PBI.
