mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-21 08:31:13 +00:00
[AZ-528] Add AZ-528 task spec + register in dependencies table
Follow-up to cumulative review batches 52-54 Finding F1. Creates the local task-spec file under _docs/02_tasks/todo/ and adds the row to _dependencies_table.md so Batch 55's implement-loop can pick AZ-528 up. Mirrors the AZ-527 precedent from the c2_vpr-side cumulative review (49-51): cumulative review opens the Jira ticket + raises the finding, the prep commit adds the spec, the next batch implements. Sized at 3 points (1 helper module + 3 strategy edits + 1 test file with AST-walk + import-grep regression guards). Marginally larger than AZ-527's 2-point c2 consolidation because the c1 spine has both module-level free functions AND mixin-shaped instance methods. Jira: https://denyspopov.atlassian.net/browse/AZ-528 Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -1,8 +1,8 @@
|
||||
# Dependencies Table
|
||||
|
||||
**Date**: 2026-05-14 (refreshed after Batch 53: AZ-333 VINS-Mono landed — first c1_vio strategy after the AZ-332 OKVIS2 production-default; consolidation hygiene for the strategy-facade duplication deferred to a post-AZ-334 PBI; earlier same-day after Batch 51: AZ-527 hygiene PBI added from cumulative review batches 49-51 F1; 2026-05-13: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path)
|
||||
**Total Tasks**: 148 (107 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks; AZ-526 = 2pt clock-helper hygiene; AZ-527 = 2pt c2 engine-dim helper hygiene
|
||||
**Total Complexity Points**: 491 (358 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt, AZ-526 = 2pt, AZ-527 = 2pt
|
||||
**Date**: 2026-05-14 (refreshed after cumulative review batches 52-54: AZ-528 hygiene PBI added for c1_vio strategy facade orchestration-spine 3-way duplication (Medium); earlier same-day after Batch 53: AZ-333 VINS-Mono landed — first c1_vio strategy after the AZ-332 OKVIS2 production-default; consolidation hygiene for the strategy-facade duplication deferred to a post-AZ-334 PBI; earlier same-day after Batch 51: AZ-527 hygiene PBI added from cumulative review batches 49-51 F1; 2026-05-13: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path)
|
||||
**Total Tasks**: 149 (108 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks; AZ-526 = 2pt clock-helper hygiene; AZ-527 = 2pt c2 engine-dim helper hygiene; AZ-528 = 3pt c1_vio facade-spine hygiene
|
||||
**Total Complexity Points**: 494 (361 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt, AZ-526 = 2pt, AZ-527 = 2pt, AZ-528 = 3pt
|
||||
|
||||
Dependencies columns list only the tracker-ID portion (descriptive tail
|
||||
text in each task spec is omitted here for table-readability). The
|
||||
@@ -160,6 +160,7 @@ are all declared and documented below under **Cycle Check**.
|
||||
| AZ-508 | Hygiene — consolidate `_iso_ts_now` helpers into `helpers/iso_timestamps.py` | 2 | AZ-263 | AZ-264 |
|
||||
| AZ-526 | Hygiene — add `iso_ts_from_clock(clock)` to `helpers/iso_timestamps.py` | 2 | AZ-508, AZ-398 | AZ-264 |
|
||||
| AZ-527 | Hygiene — consolidate `_assert_engine_output_dim` into c2-internal helper | 2 | AZ-340 | AZ-255 |
|
||||
| AZ-528 | Hygiene — consolidate c1_vio strategy facade orchestration spine | 3 | AZ-334 | AZ-254 |
|
||||
| AZ-523 | Batch 44 — C11 internal flight-state gate removal (SRP refactor; audit-trail; closed) | 3 | AZ-317, AZ-319, AZ-329 | AZ-251 |
|
||||
| AZ-524 | Batch 44 — C12 package rename: c12_operator_tooling → c12_operator_orchestrator (audit; closed)| 2 | AZ-263, AZ-326, AZ-327, AZ-328, AZ-329, AZ-330, AZ-489 | AZ-253 |
|
||||
|
||||
@@ -247,6 +248,18 @@ are all declared and documented below under **Cycle Check**.
|
||||
- **E-C12 epic (AZ-253) summary renamed**:
|
||||
`C12 Operator Pre-flight Tooling` →
|
||||
`C12 Operator Pre-flight Orchestrator`.
|
||||
- **Hygiene PBI from cumulative review batches 52-54** (added
|
||||
2026-05-14):
|
||||
- **AZ-528** (E-C1 / AZ-254) — c1_vio strategy facade
|
||||
orchestration-spine 3-way duplication. Depends on AZ-334 (the
|
||||
trigger that escalated the duplication from 2-way to 3-way; all
|
||||
3 callers now exist in `done/`). NOT a gate on AZ-335 in a strict
|
||||
technical sense, but **recommended to sequence before AZ-335** so
|
||||
the warm-start path lands against the consolidated spine instead
|
||||
of a fourth divergent copy. 3 points: 1 new module
|
||||
(`c1_vio/_facade_spine.py`) + 3 small edits + 1 test file with
|
||||
AST-walk + import-grep regression guards. Pattern mirrors
|
||||
AZ-527's c2-side consolidation.
|
||||
- **Hygiene PBIs from cumulative review batches 31-33** (added
|
||||
2026-05-13):
|
||||
- **AZ-507** (E-CC-CONF / AZ-246) — module-layout.md ↔ AZ-270 lint
|
||||
|
||||
@@ -0,0 +1,155 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user