mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 16:41:13 +00:00
[AZ-528] Consolidate c1_vio strategy facade orchestration spine
Replace 3-way byte-equivalent orchestration-spine duplication across okvis2.py / vins_mono.py / klt_ransac.py with a single c1-internal helper at components/c1_vio/_facade_spine.py. Closes cumulative review batches 52-54 Finding F1. No behaviour change — all existing AZ-332 / AZ-333 / AZ-334 AC tests pass unmodified (114 c1_vio tests green, 237 with adjacent regression suite). The helper exposes 5 stateless free functions (now_iso, bias_norm, se3_from_4x4, frame_ts_ns, frame_image) and a FacadeSpine mixin class providing _classify_state / _tick_lost / _emit_transition. Concrete strategies inherit the mixin and set spine-required instance attributes in __init__. Mirrors the AZ-527 precedent for c2_vpr-side _assert_engine_output_dim consolidation. New test file test_az528_facade_spine.py covers AC-1..AC-8 with 19 tests, including an AST regression guard that prevents future re-introduction of the consolidated free functions in any strategy module, plus a Risk-1 static check that every strategy's __init__ assigns every spine-required attribute. Archive AZ-528 task spec to done/, bump autodev state to batch 56. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,145 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 55 (AZ-528 — c1_vio strategy facade orchestration-spine consolidation)
|
||||
**Date**: 2026-05-14
|
||||
**Verdict**: PASS
|
||||
|
||||
## Scope
|
||||
|
||||
Single-task hygiene batch closing Finding F1 from
|
||||
`_docs/03_implementation/cumulative_review_batches_52-54_cycle1_report.md`.
|
||||
Replaces the 3-way byte-equivalent orchestration-spine duplication
|
||||
across `okvis2.py`, `vins_mono.py`, `klt_ransac.py` with a single
|
||||
c1-internal helper module `_facade_spine.py`. No behaviour change;
|
||||
existing AZ-332 / AZ-333 / AZ-334 AC tests pass unmodified.
|
||||
|
||||
### Changed files
|
||||
|
||||
- `src/gps_denied_onboard/components/c1_vio/_facade_spine.py` — NEW
|
||||
c1-internal helper. Exports 5 stateless free functions (`now_iso`,
|
||||
`bias_norm`, `se3_from_4x4`, `frame_ts_ns`, `frame_image`) and one
|
||||
mixin class (`FacadeSpine`) providing `_classify_state`,
|
||||
`_tick_lost`, `_emit_transition`. 225 lines including docstring +
|
||||
attribute declarations.
|
||||
- `src/gps_denied_onboard/components/c1_vio/okvis2.py` — inherit from
|
||||
`FacadeSpine`; import the 5 free functions; delete the 5 local
|
||||
module-level definitions + 3 instance methods + unused
|
||||
`_BIAS_NORM_FLOOR` constant; set 3 new spine-required attributes in
|
||||
`__init__` (`_feature_threshold`, `_producer_id`, `_strategy_label`).
|
||||
- `src/gps_denied_onboard/components/c1_vio/vins_mono.py` — same
|
||||
pattern as `okvis2.py`.
|
||||
- `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` — same
|
||||
pattern; reuses `_grayscale` (KLT/RANSAC-specific, not consolidated).
|
||||
Spine threshold attribute reads `_cfg.min_features_for_pose` (the
|
||||
KLT-side equivalent of OKVIS2's `degraded_feature_threshold`).
|
||||
- `tests/unit/c1_vio/test_az528_facade_spine.py` — NEW. 19 tests
|
||||
covering AC-1..AC-8 + a Risk-1 mitigation test that statically
|
||||
verifies every concrete strategy's `__init__` sets every
|
||||
spine-required attribute.
|
||||
|
||||
## Phase 2 — Spec Compliance
|
||||
|
||||
| AC | Test | Verified |
|
||||
|-------|-----------------------------------------------------------------------------------------------|----------|
|
||||
| AC-1 | `test_ac1_helper_module_exposes_documented_surface` | ✓ |
|
||||
| AC-2 | `test_ac2_now_iso_returns_aware_utc_with_plus_offset` | ✓ |
|
||||
| AC-3 | `test_ac3_bias_norm_matches_l2_formula` + `test_ac3_bias_norm_includes_gyro_component` | ✓ |
|
||||
| AC-4 | `test_ac4_se3_from_4x4_builds_identity_pose` | ✓ |
|
||||
| AC-5 | `test_ac5_classify_state_returns_init_during_warmup` + tracking + degraded | ✓ |
|
||||
| AC-6 | `test_ac6_tick_lost_demotes_tracking_to_degraded_first_call` + escalates-to-lost | ✓ |
|
||||
| AC-7 | `test_ac7_emit_transition_no_record_on_steady_state` + one-record-per-change + idempotent | ✓ |
|
||||
| AC-8 | `test_ac8_no_duplicated_free_functions_remain_in_strategy_module` (parametrised over 3 files) | ✓ |
|
||||
| AC-9 | Existing AZ-332 / AZ-333 / AZ-334 AC tests — all unmodified, all pass | ✓ |
|
||||
| AC-10 | `tests/unit/test_az270_compose_root.py::test_ac6_only_compose_root_imports_concrete_strategies` | ✓ |
|
||||
|
||||
Test counts:
|
||||
|
||||
- `tests/unit/c1_vio/test_az528_facade_spine.py` — 19 tests, all pass.
|
||||
- `tests/unit/c1_vio/` total — 120 collected, 114 pass + 6 tier-2
|
||||
skipped (unchanged from pre-AZ-528 state).
|
||||
- Adjacent regression suite (`test_az270_compose_root`, `test_az272_fdr_record_schema`,
|
||||
`test_az273_fdr_client_ringbuf`, `test_ac1_scaffold_layout`,
|
||||
`tests/unit/c1_vio/`) — 237 pass + 6 skipped.
|
||||
|
||||
Scope discipline: only the 5 files in the task spec's "Included"
|
||||
list were touched; no test files outside the new
|
||||
`test_az528_facade_spine.py` were modified (AC-9 hard requirement).
|
||||
|
||||
## Phase 3 — Code Quality
|
||||
|
||||
- **SRP**: `FacadeSpine` owns one responsibility — the c1_vio
|
||||
strategy state machine + FDR `vio.health` record emission. Free
|
||||
functions are stateless pure utilities. Each concrete strategy
|
||||
retains its geometry pipeline + native-binding driver. Clean split.
|
||||
- **Naming**: instance attributes the mixin reads are documented in
|
||||
the class docstring; type annotations on the class declare them for
|
||||
IDE / type-checker consumption (Risk 1 mitigation).
|
||||
- **Comments**: deferred-consolidation comments referencing this PBI
|
||||
(`# post-AZ-334 hygiene PBI ...`) were removed from
|
||||
`klt_ransac.py` as required by the task spec.
|
||||
- **Dead code**: removed `_BIAS_NORM_FLOOR` from `okvis2.py` (verified
|
||||
via grep; not used anywhere in the c1_vio component or
|
||||
cross-component).
|
||||
- **DRY**: the consolidation eliminates ~120 lines of duplicated code
|
||||
across the three strategy modules.
|
||||
- **Test quality**: every test follows Arrange / Act / Assert; AC
|
||||
tests use parametrisation where appropriate (AC-8 over the 3
|
||||
strategy modules).
|
||||
|
||||
## Phase 4 — Security Quick-Scan
|
||||
|
||||
N/A — pure refactor, no new inputs, no I/O, no string interpolation
|
||||
into queries, no subprocess invocation. The free functions take only
|
||||
typed DTOs (`ImuBias`, `NavCameraFrame`, `np.ndarray`); the mixin
|
||||
methods only mutate instance state.
|
||||
|
||||
## Phase 5 — Performance
|
||||
|
||||
No hot-path regression. The mixin call sites are byte-equivalent to
|
||||
the previous in-line definitions (same conditionals, same
|
||||
`FdrClient.enqueue` invocation, same string interpolation in
|
||||
`bias_norm`). The single new indirection is method dispatch through
|
||||
the mixin's MRO — Python class-method resolution is O(1) on a fixed
|
||||
MRO and is amortised inside the hot-path's existing call overhead.
|
||||
|
||||
## Phase 6 — Cross-Task Consistency
|
||||
|
||||
Single-task batch — N/A.
|
||||
|
||||
## Phase 7 — Architecture Compliance
|
||||
|
||||
- **Layer direction**: `_facade_spine.py` imports only from L1
|
||||
substrate (`_types.nav`, `components.c1_vio.errors`,
|
||||
`fdr_client.records`) + stdlib (`datetime`, `math`) + numpy. No
|
||||
imports from a higher layer; no imports from a sibling component.
|
||||
- **Public API respect**: `_facade_spine` is underscore-prefixed and
|
||||
NOT in `c1_vio/__init__.py`'s `__all__`. AZ-270's
|
||||
`test_ac6_only_compose_root_imports_concrete_strategies`
|
||||
regression-gate passes. Mirrors the AZ-527 precedent for
|
||||
`_assert_engine_output_dim` in `c2_vpr`.
|
||||
- **Cycles**: no new cyclic dependencies. `_facade_spine` is a leaf
|
||||
inside `c1_vio`; the 3 strategy modules already imported from
|
||||
`c1_vio.errors`, so the new sibling import slots in cleanly.
|
||||
- **Duplicate symbols**: AC-8 AST regression guard asserts the 5
|
||||
consolidated free-function names appear only in `_facade_spine.py`.
|
||||
- **Cross-cutting concerns**: the spine is a c1-internal concern by
|
||||
task-spec decision (component state machines + per-strategy FDR
|
||||
record producers do not generalise across c2..c5; AZ-507 carved
|
||||
that boundary intentionally). NOT hoisted to `shared/helpers/`.
|
||||
|
||||
## Findings
|
||||
|
||||
None.
|
||||
|
||||
## Conclusion
|
||||
|
||||
PASS. The consolidation removes ~120 lines of duplication, preserves
|
||||
behaviour across all three strategies (verified by 237 passing tests
|
||||
in the focused + adjacent suite), and respects the c1_vio component
|
||||
boundary. The cumulative review batches 52-54 Finding F1 is closed.
|
||||
|
||||
Risk 4 (mixin couples future divergence) is mitigated by the task
|
||||
spec's documented escape hatch: future divergent strategies can
|
||||
either override mixin methods or use the free functions in isolation.
|
||||
The 3 current strategies' state machines are byte-equivalent
|
||||
post-refactor, so the mixin is the right shape today.
|
||||
Reference in New Issue
Block a user