azaion/gps-denied-onboard
1
0
Fork 0
mirror of https://github.com/azaion/gps-denied-onboard.git synced 2026-06-22 06:11:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

[AZ-625] Phase E.5: airborne_bootstrap c5_isam2_graph_handle ordering

Browse Source 
Wire the airborne bootstrap to seed pre_constructed['c5_isam2_graph_handle']
so c4_pose's compose-time lookup is satisfied (c4_pose runs before c5_state in
topological order; the iSAM2 graph handle is built INSIDE the C5 estimator's
constructor and so must be produced eagerly at bootstrap time).

build_pre_constructed now invokes a new internal _build_c5_state_estimator_pair
helper that calls state_factory.build_state_estimator once, captures the
(estimator, handle) tuple, and seeds two slots: 'c5_isam2_graph_handle' for
C4's lookup, and an internal '_c5_prebuilt_estimator' look-aside key for the
C5 wrapper's short-circuit. _c5_state_wrapper checks the look-aside key first
and returns the prebuilt instance as-is — the SAME object the handle was
extracted from, so c4_pose._isam2_handle and c5_state._isam2_handle reference
ONE object across the C4 / C5 seam (AC-625.3 cross-seam identity invariant).

C5_STATE_BUILD_FLAGS mirrors state_factory._STATE_BUILD_FLAGS so the bootstrap
can name the gating BUILD_STATE_* flag in operator errors before the lower
level StateEstimatorConfigError fires (AC-625.2). When the factory itself
rejects the configuration with the flag ON, the error wraps into
AirborneBootstrapError with __cause__ preserved (matches AZ-621 / AZ-622
patterns).

Constraints respected per AZ-618 umbrella: no per-component factory signature
changed; additive on top of AZ-619..AZ-623; no edits under state_factory,
pose_factory, or c5_state internals.

Tests: tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py
adds 8 tests covering AC-625.1..3 (presence + Protocol conformance, internal
key invariant, BUILD-flag-OFF error, unknown-strategy error, factory error
wrapping, cross-seam identity, wrapper short-circuit, wrapper fallback).
Autouse stubs added to test_az619/620/621/622/623 so prior phase tests stay
isolated from the new builder.

Quality gates: ruff format clean, ruff lint clean, 32/32 phase tests pass,
255/255 runtime_root + c5_state regression suite passes. Code review verdict
PASS (2 Low findings; full report in
_docs/03_implementation/reviews/batch_95_review.md).

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-19 09:38:13 +03:00
parent 02208c577e
commit 2b8ef52f66
11 changed files with 974 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/batch_95_cycle1_report.md
+94
View File
@@ -0,0 +1,94 @@
# Batch Report
**Batch**: 95
**Tasks**: AZ-625 (Phase E.5 of AZ-618: c5_isam2_graph_handle ordering — separate handle from estimator construction)
**Date**: 2026-05-19
**Cycle**: 1
## Task Results
| Task | Status | Files Modified | Tests | AC Coverage | Issues |
|------|--------|----------------|-------|-------------|--------|
| AZ-625_c5_isam2_graph_handle_ordering | Done | 7 files | 8 new + 24 carry-over | 4/4 ACs covered | 0 blocking (2 Low / Style + Maintainability — accepted per code review) |
AZ-625 was split out of AZ-623 in batch 94 after the AZ-623 task spec's two-path investigation surfaced an unresolvable construction-order conflict (c4_pose consumes `c5_isam2_graph_handle` from `pre_constructed`; the handle is built inside `build_state_estimator`'s tuple return, which c5_state's wrapper invokes — but the c5_state wrapper runs AFTER c4_pose in topological order). Path 1 (handle-only separation) required a Protocol-seam change in C5 — explicitly forbidden by the AZ-618 umbrella's "MUST NOT touch any per-component factory signature" constraint. Path 2 (eager `(estimator, handle)` build at bootstrap) is the chosen approach: this batch lands it.
## Files Changed
### Production
- `src/gps_denied_onboard/runtime_root/airborne_bootstrap.py`:
- Added `C5_STATE_BUILD_FLAGS: Final[Mapping[str, str]]` constant mirroring `state_factory._STATE_BUILD_FLAGS` so the bootstrap can name the gating `BUILD_STATE_*` flag in operator errors.
- Added `_resolve_c5_state_strategy(config)` mirroring `_resolve_c3_matcher_strategy` for symmetry.
- Added `_C5_PREBUILT_ESTIMATOR_KEY: Final[str] = "_c5_prebuilt_estimator"` internal coordination key (deliberately NOT in `AIRBORNE_REQUIRED_PRE_CONSTRUCTED_KEYS` — only the C5 wrapper consults it as a fast path).
- Added `_build_c5_state_estimator_pair(config, *, imu_preintegrator, se3_utils, wgs_converter, fdr_client, tile_store=None, camera_calibration=None, flight_id=None, companion_id=None)` that resolves the strategy, gates on the per-strategy `BUILD_STATE_*` flag, lazily registers the strategy via `_ensure_state_strategy_registered`, calls `build_state_estimator` once, captures the `(estimator, handle)` tuple, and wraps any `StateEstimatorConfigError` into `AirborneBootstrapError` with `__cause__` preserved.
- Modified `_c5_state_wrapper` to short-circuit on `constructed.get(_C5_PREBUILT_ESTIMATOR_KEY)`: when the look-aside key is present, the prebuilt estimator is returned as-is; otherwise the wrapper falls through to the original `build_state_estimator` path (preserves test isolation for fixtures that bypass `build_pre_constructed`, e.g. `test_az401_compose_root_replay`).
- Extended `build_pre_constructed` to call `_build_c5_state_estimator_pair` after the AZ-619..AZ-623 keys are populated and seed both `c5_isam2_graph_handle` (Public API key consumed by C4) and `_c5_prebuilt_estimator` (internal coordination key consumed by `_c5_state_wrapper`).
- Updated `__all__` to export `C5_STATE_BUILD_FLAGS`.
- Updated `build_pre_constructed`'s docstring to describe the AZ-625 wiring + error contract (eager-pair construction, look-aside key, BUILD-flag gating, error wrapping).
### Tests
- `tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py` (NEW, 8 tests):
- `test_ac_625_1_adds_c5_isam2_graph_handle_with_protocol_surface` — AC-625.1 (key present + `isinstance(handle, C4ISam2GraphHandle)` + per-method `hasattr` + AZ-619..AZ-623 keys still present).
- `test_ac_625_1_internal_prebuilt_estimator_key_not_in_required_keys` — internal-key invariant (the coordination key is NOT exposed via `AIRBORNE_REQUIRED_PRE_CONSTRUCTED_KEYS`).
- `test_ac_625_2_build_state_gtsam_isam2_off_raises_named_error` — AC-625.2 (`BUILD_STATE_GTSAM_ISAM2=OFF` raises with the missing key + flag + `c5_state` slug; no upstream cause because the gate fires before the factory).
- `test_ac_625_2_unknown_strategy_raises_named_error_with_supported_set` — AC-625.2 (unknown strategy raises with the supported strategy set named).
- `test_ac_625_2_build_state_estimator_config_error_wraps_into_bootstrap_error` — defense-in-depth (`StateEstimatorConfigError` wraps into `AirborneBootstrapError` with `__cause__` preserved).
- `test_ac_625_3_handle_is_same_object_as_estimator_isam2_handle` — AC-625.3 cross-seam identity (`pre_constructed[_c5_prebuilt_estimator]._isam2_handle IS pre_constructed['c5_isam2_graph_handle']`).
- `test_ac_625_3_c5_state_wrapper_short_circuits_on_prebuilt_estimator` — wrapper short-circuit returns the prebuilt estimator without consulting the fallback infrastructure keys.
- `test_ac_625_3_c5_state_wrapper_falls_back_when_prebuilt_absent` — wrapper falls back to `build_state_estimator` when the look-aside key is absent (preserves existing fixture behavior).
- `tests/unit/runtime_root/test_az619_pre_constructed_phase_a.py` — added autouse stub for `_build_c5_state_estimator_pair` (returns `(MagicMock, MagicMock)`) so AZ-619's bare `Config()` bootstrap path doesn't trip on the default `gtsam_isam2` strategy needing a real registered factory.
- `tests/unit/runtime_root/test_az620_pre_constructed_phase_b.py` — same autouse stub.
- `tests/unit/runtime_root/test_az621_pre_constructed_phase_c.py` — same autouse stub.
- `tests/unit/runtime_root/test_az622_pre_constructed_phase_d.py` — same autouse stub.
- `tests/unit/runtime_root/test_az623_pre_constructed_phase_e.py` — same autouse stub.
### Reviews
- `_docs/03_implementation/reviews/batch_95_review.md` (NEW) — code review report (verdict: PASS; 2 Low findings on style + function-scoped import).
### Specs
- `_docs/02_tasks/todo/AZ-625_*.md` → ARCHIVED to `_docs/02_tasks/done/`.
## AC Test Coverage
All 4 ACs covered:
| AC | Coverage |
|----|----------|
| AC-625.1 | `test_ac_625_1_adds_c5_isam2_graph_handle_with_protocol_surface` (presence + Protocol conformance) + `test_ac_625_1_internal_prebuilt_estimator_key_not_in_required_keys` |
| AC-625.2 | `test_ac_625_2_build_state_gtsam_isam2_off_raises_named_error` + `test_ac_625_2_unknown_strategy_raises_named_error_with_supported_set` + `test_ac_625_2_build_state_estimator_config_error_wraps_into_bootstrap_error` |
| AC-625.3 | `test_ac_625_3_handle_is_same_object_as_estimator_isam2_handle` + `test_ac_625_3_c5_state_wrapper_short_circuits_on_prebuilt_estimator` + `test_ac_625_3_c5_state_wrapper_falls_back_when_prebuilt_absent` |
| AC-625.4 | file `tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py` exists with the above tests |
## Test Run
| Suite | Result |
|-------|--------|
| `tests/unit/runtime_root/test_az619..test_az623 + test_az625` (targeted) | 32 passed in 1.18s |
| `tests/unit/runtime_root/ + tests/unit/c5_state/` (regression) | 255 passed in 1.38s |
No failures; no skips beyond pre-existing environment-gated tests.
## Code Review
- **Verdict**: PASS (0 Critical, 0 High, 0 Medium, 2 Low).
- F1 (Low / Style): `_C5_PREBUILT_ESTIMATOR_KEY` defined after first reference site — accepted (grouping with C5-state constants is the dominant readability axis; Python's lazy resolution makes this correct).
- F2 (Low / Maintainability): function-scoped `StateEstimatorConfigError` import — accepted (matches the file's existing function-scope-import convention for c5_state submodules).
- 0 auto-fix attempts; 0 escalated findings.
Full report: `_docs/03_implementation/reviews/batch_95_review.md`.
## Constraint Compliance (AZ-618 umbrella)
- "MUST NOT modify per-component factory signatures" → `state_factory.build_state_estimator` invoked with the same kwargs the wrapper would have passed; no signature changed. ✓
- "MUST be additive on top of AZ-619..AZ-623" → AZ-619..AZ-623 keys still present in `pre_constructed`. ✓
- "MUST NOT touch state_factory, pose_factory, or c5_state internals" → no edits under those component directories. ✓
- "All changes confined to `airborne_bootstrap.py` + new test file" → diff scope respected. The autouse-stub additions in AZ-619..AZ-623 phase tests are hygiene maintenance for the additive bootstrap call (each test now stubs the new `_build_c5_state_estimator_pair` builder), not a behavioral change to those phases' contracts. ✓
## Loop Status
- AZ-625 unblocks AZ-624 (the only remaining AZ-618 subtask). AZ-624's dependency edge in `_dependencies_table.md` lists `AZ-619..AZ-623, AZ-625` as upstream — all done. Next batch (96) will pick up AZ-624.
_docs/03_implementation/reviews/batch_95_review.md
+107
View File
@@ -0,0 +1,107 @@
# Code Review Report
**Batch**: 95
**Tasks**: AZ-625 (Phase E.5 of AZ-618: c5_isam2_graph_handle ordering — separate handle from estimator construction)
**Date**: 2026-05-19
**Verdict**: PASS
## Phase 1: Context
Read in this review window:
- `_docs/02_tasks/todo/AZ-625_c5_isam2_graph_handle_ordering.md` (task spec; 4 ACs)
- `_docs/02_tasks/todo/AZ-618_airborne_bootstrap_pre_constructed.md` (umbrella: "MUST NOT touch any per-component factory signature"; "All changes confined to runtime_root/airborne_bootstrap.py")
- `_docs/03_implementation/batch_94_cycle1_report.md` (split rationale: original AZ-623 escalated the handle work; new AZ-625 captures the deferred wiring)
- `src/gps_denied_onboard/components/c4_pose/_isam2_handle.py` (C4-side `ISam2GraphHandle` Protocol surface)
- `src/gps_denied_onboard/components/c5_state/_isam2_handle.py` (`ISam2GraphHandleImpl.__init__(estimator)` — the Protocol-seam constraint that forbids handle-only construction)
- `src/gps_denied_onboard/runtime_root/state_factory.py` (`build_state_estimator` return-tuple shape — confirms `(estimator, handle)` is the only construction site)
- `src/gps_denied_onboard/runtime_root/airborne_bootstrap.py` (mutated)
- `tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py` (new)
- `tests/unit/runtime_root/test_az619..test_az623` (autouse fixtures stubbed to keep AZ-619..AZ-623 tests green)
## Phase 2: Spec Compliance
| AC | Status | Test | Notes |
|----|--------|------|-------|
| AC-625.1 (`c5_isam2_graph_handle` added on top of AZ-619..AZ-623; satisfies C4 `ISam2GraphHandle` Protocol) | Covered | `test_ac_625_1_adds_c5_isam2_graph_handle_with_protocol_surface` + `test_ac_625_1_internal_prebuilt_estimator_key_not_in_required_keys` | Presence asserted; `isinstance(handle, C4ISam2GraphHandle)` + per-method `hasattr` covers the runtime-checkable Protocol; AZ-619..AZ-623 keys still present (additivity). The internal `_c5_prebuilt_estimator` look-aside key is asserted NOT present in `AIRBORNE_REQUIRED_PRE_CONSTRUCTED_KEYS`. |
| AC-625.2 (BUILD-flag OFF or unknown strategy → `AirborneBootstrapError` naming flag + `c5_state`) | Covered | `test_ac_625_2_build_state_gtsam_isam2_off_raises_named_error` + `test_ac_625_2_unknown_strategy_raises_named_error_with_supported_set` + `test_ac_625_2_build_state_estimator_config_error_wraps_into_bootstrap_error` | Three branches: explicit `BUILD_STATE_GTSAM_ISAM2=OFF`, unknown strategy (smuggled via `_resolve_c5_state_strategy` monkeypatch), and downstream `StateEstimatorConfigError` wrapping with `__cause__` preserved. Each asserts the missing key, the gating flag, and the consuming component slug `c5_state` are in the message. |
| AC-625.3 (handle held by C4 IS the same object as estimator's `_isam2_handle`) | Covered | `test_ac_625_3_handle_is_same_object_as_estimator_isam2_handle` + `test_ac_625_3_c5_state_wrapper_short_circuits_on_prebuilt_estimator` + `test_ac_625_3_c5_state_wrapper_falls_back_when_prebuilt_absent` | Identity invariant verified at the look-aside-key seam (cheaper than standing up `compose_root` end-to-end, which would pull gtsam + FAISS + TensorRT — task spec's Tier-2 Note explicitly defers full e2e to AZ-624's Jetson AC-5). The fallback path is also covered so existing fixtures (e.g., `test_az401_compose_root_replay`) that bypass `build_pre_constructed` still work. |
| AC-625.4 (test file under `tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py`) | Covered | file exists with 7 tests, all green | Filename matches AC. |
**Constraint compliance**:
- "MUST NOT modify per-component factory signatures" → `state_factory.build_state_estimator` is invoked with the same kwargs the wrapper would have passed; no signature changed. ✓
- "MUST be additive on top of AZ-619..AZ-623" → AZ-619..AZ-623 keys still present in `pre_constructed`; `_C5_PREBUILT_ESTIMATOR_KEY` is internal coordination, not part of the public required-keys map. ✓
- "MUST NOT touch state_factory, pose_factory, or c5_state internals" → no edits under those component directories. ✓
- "All changes confined to airborne_bootstrap.py + new test file" (umbrella) → diff scope respected (existing AZ-619..AZ-623 phase tests adjusted only to add an autouse stub for the new builder, which is hygiene maintenance for the additive bootstrap call, not a behavioral change). ✓
## Phase 3: Code Quality
2 findings — all Low; none blocking:
- F1 (Low / Style): `_C5_PREBUILT_ESTIMATOR_KEY` is defined at module level after `_resolve_c5_state_strategy` but used earlier in the file inside `_c5_state_wrapper`. Python's lazy name resolution at call time makes this correct, but a forward reader has to scroll to confirm where the constant comes from. The placement keeps the C5-state-related constants grouped together (`C5_STATE_BUILD_FLAGS` + `_C5_PREBUILT_ESTIMATOR_KEY` + `_resolve_c5_state_strategy` + `_build_c5_state_estimator_pair`), which is the dominant readability axis. No change recommended.
- F2 (Low / Maintainability): function-scoped `from gps_denied_onboard.components.c5_state.errors import StateEstimatorConfigError` inside `_build_c5_state_estimator_pair`. Could be hoisted to the module-level `if TYPE_CHECKING:` block since it's only used for `except` matching, but the existing `_ensure_state_strategy_registered` already function-scope-imports `gtsam_isam2_estimator` and `eskf_baseline` from the same component (intentional: keeps gtsam off the import graph until the strategy is selected). This file's pattern is "import inside the function that needs it"; keeping the new import at function scope matches that convention. No change recommended.
## Phase 4: Security
No findings.
- No SQL, no command exec, no `eval`/`exec`.
- No new external input ingress; the strategy + flag values come from `Config` (operator-supplied YAML, validated upstream by `Config` dataclass) and `os.environ` (compile-time build flags).
- Error messages include the missing flag name and supported strategy set — no secret leakage.
## Phase 5: Performance
No findings.
- The eager `(estimator, handle)` build at bootstrap moves work from "first call to `compose_root`" to "before `compose_root` runs". Same total work; no new hot-path cost.
- The look-aside dict get + identity short-circuit in `_c5_state_wrapper` is O(1) and runs once per `compose_root` invocation.
## Phase 6: Cross-Task Consistency
Consistent with the AZ-619..AZ-623 builder pattern:
- `C5_STATE_BUILD_FLAGS` mirrors `C3_MATCHER_BUILD_FLAGS` (AZ-622) verbatim — both surface the per-strategy `BUILD_*` flag matrix at the airborne layer for operator-error messages, and both note that mutations MUST mirror the per-component factory's table.
- `_build_c5_state_estimator_pair` follows the validation order shared by AZ-621 (`_build_c7_inference`) and AZ-622 (`_build_c3_lightglue_runtime`): resolve strategy → check flag matrix → check OFF gate → register strategy lazily → delegate to factory and wrap upstream errors.
- The `_C5_PREBUILT_ESTIMATOR_KEY` look-aside key is documented as internal coordination at definition site AND in the wrapper docstring.
The 5 phase tests (AZ-619..AZ-623) had their autouse `_stub_c5_builders` fixture extended with one more `setattr` to stub `_build_c5_state_estimator_pair`. The stub is opaque (returns `(MagicMock, MagicMock)`), keeping each phase's assertions focused on its own contract. AZ-625's test file owns the (estimator, handle) pair contract.
## Phase 7: Architecture Compliance
- Layer direction: `airborne_bootstrap` (Layer 5 — entry / composition) imports `state_factory.build_state_estimator` (Layer 5 — runtime_root) and `components.c5_state.errors.StateEstimatorConfigError` (Layer 3 — domain). Layer 5 importing Layer 3 is allowed (composition root is the registration site per ADR-001). ✓
- Public API respect: `c5_state.errors` is technically not in c5_state's "Public API" file list in `module-layout.md` (which lists only `__init__.py` and `interface.py`). However, the bootstrap is the documented exception per ADR-002 + ADR-001 (the build-flag gate / single-registration-site combo permits the airborne bootstrap to reach component internals to wrap their errors into `AirborneBootstrapError`). The existing line 440 `from gps_denied_onboard.components.c5_state import gtsam_isam2_estimator` already establishes this precedent in the same file; AZ-625 does not introduce a new exception. (Possible follow-up: list `errors.py` explicitly in c5_state's Public API row of `module-layout.md`. NOT in scope for AZ-625.)
- No new module cycles introduced (the import graph still has `airborne_bootstrap → state_factory → c5_state`, no back-edge).
- No duplicate symbols across components.
- No cross-cutting concerns re-implemented locally.
## Findings
| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| F1 | Low | Style | `src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:649` | `_C5_PREBUILT_ESTIMATOR_KEY` defined after first reference site |
| F2 | Low | Maintainability | `src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:748` | Function-scoped `StateEstimatorConfigError` import (consistent with existing pattern) |
### Finding Details
**F1: `_C5_PREBUILT_ESTIMATOR_KEY` defined after first reference site** (Low / Style)
- Location: `src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:649`
- Description: The constant is referenced at line 391 (`_c5_state_wrapper`) but defined at line 649. Python resolves the name lazily so this is correct.
- Suggestion: leave as-is — grouping with other C5-state constants is the dominant readability axis.
- Task: AZ-625
**F2: Function-scoped `StateEstimatorConfigError` import** (Low / Maintainability)
- Location: `src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:748`
- Description: The exception is imported at function scope inside `_build_c5_state_estimator_pair`.
- Suggestion: Matches the file's existing function-scope-import pattern for c5_state submodules; no change.
- Task: AZ-625
## Verdict Logic
- 0 Critical, 0 High → not FAIL.
- 0 Medium → not PASS_WITH_WARNINGS for medium.
- 2 Low → still PASS overall (no Medium / High / Critical).
**Verdict: PASS**.