mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 11:11:23 +00:00
[AZ-399] [AZ-400] C8 TlogReplayFcAdapter + ReplaySink + JsonlReplaySink
Opens E-DEMO-REPLAY (AZ-265): the two C8 strategies that let the upcoming compose_replay (AZ-401) and gps-denied-replay CLI (AZ-402) run the production C1-C5 pipeline against a recorded (.tlog, video) pair without touching live FC I/O. AZ-400 lands the contract ReplaySink Protocol (emit + close per replay_protocol.md v1.0.0) and JsonlReplaySink: orjson-serialised JSONL, fsync-on-close, build-flag gated (BUILD_REPLAY_SINK_JSONL), double-close idempotent, FDR mirror on open/close. The drifted AZ-390 stub in interface.py is removed; the canonical Protocol now lives in replay_sink.py per module-layout.md and is re-exported via __init__.py. AZ-390 conformance test widened. AZ-399 lands TlogReplayFcAdapter: full FcAdapter Protocol surface, build-flag gated (BUILD_TLOG_REPLAY_ADAPTER), pymavlink stream-parse with bounded pre-scan + fail-fast on missing required messages (R-DEMO-3), dedicated decode thread feeding the existing AZ-391 SubscriptionBus. Outbound surface raises FcEmitError per Invariant 5; request_source_set_switch raises SourceSetSwitchNotSupportedError. Pacing honours Invariant 6 via Clock.sleep_until_ns. time_offset_ms shifts every emitted received_at per Invariant 8. Non-monotonic timestamps raise FcOpenError. Test coverage: 188 c8_fc_adapter tests pass; 1 skipped (AZ-399 AC-1 500 MB tlog RSS bound, deferred to AZ-404 e2e behind RUN_REPLAY_E2E). Code review: PASS_WITH_WARNINGS — 1 Medium (mapping logic duplicates AZ-391 live decoder; intentional today, four behavioural deltas documented), 2 Low. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -191,7 +191,7 @@ Bootstrap reference: `_docs/02_tasks/todo/AZ-263_initial_structure.md`. Architec
|
||||
- **Directory**: `src/gps_denied_onboard/components/c8_fc_adapter/`
|
||||
- **Public API**:
|
||||
- `__init__.py` (re-exports `FcAdapter`, `GcsAdapter`, `ReplaySink`, `EmittedExternalPosition`)
|
||||
- `interface.py` (`FcAdapter`, `GcsAdapter`, `ReplaySink` Protocols)
|
||||
- `interface.py` (`FcAdapter`, `GcsAdapter` Protocols; `ReplaySink` Protocol lives in `replay_sink.py` per the replay contract)
|
||||
- **Internal**:
|
||||
- `pymavlink_ardupilot_adapter.py` (ArduPilot Plane via pymavlink)
|
||||
- `msp2_inav_adapter.py` (iNav via MSP2)
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
# Batch 59 — Cycle 1 Report
|
||||
|
||||
**Date**: 2026-05-14
|
||||
**Tasks**: AZ-399 (C8 `TlogReplayFcAdapter`), AZ-400 (C8 `ReplaySink` Protocol + `JsonlReplaySink`)
|
||||
**Verdict**: COMPLETE — PASS_WITH_WARNINGS
|
||||
|
||||
## Summary
|
||||
|
||||
Opened the E-DEMO-REPLAY epic (AZ-265) by landing the two C8 strategies that let the upcoming `compose_replay` (AZ-401) and `gps-denied-replay` CLI (AZ-402) consume a recorded `(.tlog, video)` pair without touching live FC I/O.
|
||||
|
||||
`JsonlReplaySink` (AZ-400) implements the contract `ReplaySink` Protocol from `_docs/02_document/contracts/replay/replay_protocol.md` v1.0.0: `emit(EstimatorOutput)` writes exactly one orjson-serialised JSONL line, `close()` `fsync`s, idempotent close fires `replay.sink.double_close` DEBUG. The on-wire shape is computed by an explicit `_to_jsonable` helper instead of `dataclasses.asdict` — `asdict` cannot meet AC-4 (numpy 6×6 → flat 36-float list, not a nested 2-D shape) or AC-5 (enum → name string, not value or repr) within the orjson default options.
|
||||
|
||||
The AZ-390 `interface.py` previously carried a `ReplaySink` stub with a single-method `write(...)` shape that had drifted from the contract. `interface.py` now owns only `FcAdapter` + `GcsAdapter` per the corrected `module-layout.md` line; the canonical Protocol lives in `replay_sink.py` and is re-exported through `__init__.py`. The AZ-390 conformance test was widened to assert both `emit` + `close` and to reject partial implementations.
|
||||
|
||||
`TlogReplayFcAdapter` (AZ-399) is the replay-mode `FcAdapter` strategy. Construction validates `BUILD_TLOG_REPLAY_ADAPTER` and the dialect (`ARDUPILOT_PLANE` or `INAV` only — `GCS_QGC` is rejected). `open(...)` runs a bounded pre-scan (`_PRESCAN_MAX_MESSAGES = 6000`) that asserts every required message group (RAW_IMU OR SCALED_IMU2; ATTITUDE; GPS_RAW_INT OR GPS2_RAW; HEARTBEAT) is represented, then starts a dedicated decode thread that streams the rest of the tlog through pymavlink's `recv_match(blocking=False)` — never materialising the file (R-DEMO-2). Frames are wrapped in `FcTelemetryFrame(received_at=msg._timestamp_ns + time_offset_ns, signed=False)` and dispatched via the existing AZ-391 `SubscriptionBus` so live and replay consumers see identical fan-out shape (Invariant 1).
|
||||
|
||||
Outbound surface is hard-failed per Invariant 5 (`emit_external_position` and `emit_status_text` raise `FcEmitError("replay adapter does not emit to FC")`); `request_source_set_switch` raises `SourceSetSwitchNotSupportedError`. Pacing honours Invariant 6: `pace=REALTIME` calls `Clock.sleep_until_ns(received_at)` between frames, `pace=ASAP` skips the call. Non-monotonic timestamps inside the dispatched stream raise `FcOpenError` (mirrors the AZ-398 TlogDerivedClock policy).
|
||||
|
||||
The decode-side path duplicates the AP mapping logic from `_inbound_mavlink.PymavlinkInboundDecoder` deliberately — replay differs in four observable ways (timestamp source, signed flag, non-monotonic policy, no STATUSTEXT spoof promotion) and a shared mapper would have to expose seams for all four. Captured as F1 Medium/Maintainability in the batch review.
|
||||
|
||||
## Files added / modified
|
||||
|
||||
### Added (4)
|
||||
|
||||
- `src/gps_denied_onboard/components/c8_fc_adapter/replay_sink.py` — `ReplaySink` Protocol, `JsonlReplaySink`, `_to_jsonable` helper, module-level `create(...)` factory, `_BUILD_FLAG = "BUILD_REPLAY_SINK_JSONL"` gating, `replay.sink.opened/closed/emit_progress/double_close` log + FDR mirror.
|
||||
- `src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py` — `TlogReplayFcAdapter`, `ReplayPace` enum, `REQUIRED_MESSAGE_TYPES`, fail-fast pre-scan, dedicated decode thread, build-flag gating, FDR mirror on open + missing-messages.
|
||||
- `tests/unit/c8_fc_adapter/test_az400_replay_sink.py` — 21 tests covering AC-1..AC-10 plus schema fidelity, write-side OS error, JSON validity, double-close DEBUG, factory entrypoint.
|
||||
- `tests/unit/c8_fc_adapter/test_az399_tlog_replay_adapter.py` — 22 tests covering AC-2..AC-10 (AC-1 deferred to AZ-404 e2e via `@pytest.mark.skip` with prerequisite reason), constructor validation, double-open guard, INAV dialect parity, multi-subscriber fan-out, current_flight_state init/update, warm-start hint cache, non-monotonic guard, INFO log + FDR mirror, required-message catalog sanity.
|
||||
|
||||
### Modified (4)
|
||||
|
||||
- `src/gps_denied_onboard/components/c8_fc_adapter/__init__.py` — re-exports `ReplaySink` from `replay_sink.py` and updates `__all__`.
|
||||
- `src/gps_denied_onboard/components/c8_fc_adapter/interface.py` — removed the drifted `ReplaySink` stub; module docstring clarifies that the Protocol now lives in `replay_sink.py`.
|
||||
- `tests/unit/c8_fc_adapter/test_az390_adapter_protocol.py` — `test_ac1_replay_sink_protocol_conformance` widened to the contract shape; new `test_ac1_replay_sink_rejects_partial_surface` guards against future stub drift.
|
||||
- `_docs/02_document/module-layout.md` — `c8_fc_adapter` Public API line corrected: `ReplaySink` lives in `replay_sink.py`, not `interface.py`.
|
||||
|
||||
## Task Results
|
||||
|
||||
| Task | Status | Files Modified | Focused tests | AC Coverage | Issues |
|
||||
|--------|--------|------------------------------------------|---------------|--------------------------------------------|--------|
|
||||
| AZ-400 | Done | 1 added (`replay_sink.py`) / 3 modified | 21/21 pass | 10/10 covered | None |
|
||||
| AZ-399 | Done | 1 added (`tlog_replay_adapter.py`) / 0 modified | 22/22 pass + 1 skipped (AC-1 with prerequisite) | 10/10 covered (AC-1 skipped per skill rule) | None |
|
||||
|
||||
## AC Test Coverage: 20/20 covered (1 AC skipped with prerequisite reason; counts as Covered per skill rule)
|
||||
|
||||
- AZ-400 AC-1..AC-10 — all directly asserted.
|
||||
- AZ-399 AC-2..AC-10 — all directly asserted (AC-4 + AC-8 + AC-10 also have ancillary edge-case tests).
|
||||
- AZ-399 AC-1 (500 MB tlog RSS bound) — `@pytest.mark.skip` with explicit prerequisite reason; deferred to AZ-404 e2e gated behind `RUN_REPLAY_E2E=1`.
|
||||
|
||||
## Code Review Verdict: PASS_WITH_WARNINGS
|
||||
|
||||
See `_docs/03_implementation/reviews/batch_59_review.md`. Three findings recorded — Medium ×1, Low ×2 — none blocking:
|
||||
|
||||
1. **F1 Medium / Maintainability** — `_handle_imu/_attitude/_gps/_heartbeat` + `_map_fix_type` + `_map_mav_state` duplicate the AZ-391 live decoder. Intentional today (four behavioural deltas — timestamp source, signed flag, non-monotonic policy, STATUSTEXT absence). Revisit during AZ-405 or a future refactor if a third caller appears.
|
||||
2. **F2 Low / Maintainability** — `_PRESCAN_MAX_MESSAGES = 6000` is module-level. Future-proofing note; thread through the constructor when a real fixture pushes the budget.
|
||||
3. **F3 Low / Maintainability** — `# noqa: SIM115` on the unbuffered `open(...)` carries a justification comment; acceptable.
|
||||
|
||||
No Critical / High / Architecture findings. Auto-fix not required.
|
||||
|
||||
## Auto-Fix Attempts: 0
|
||||
|
||||
## Stuck Agents: None
|
||||
|
||||
## Tests Run
|
||||
|
||||
- Focused suite (`tests/unit/c8_fc_adapter/`): **188 passed, 1 skipped** (the AZ-399 AC-1 prerequisite gate).
|
||||
- Full repo suite: deferred to Step 16 (Final Test Run) per the implement skill's "exactly once at end of implementation phase" cadence.
|
||||
|
||||
## Next Batch
|
||||
|
||||
The replay track is half-wired:
|
||||
|
||||
- ✅ `Clock` Protocol (AZ-398, batch 57)
|
||||
- ✅ `FrameSource` + `VideoFileFrameSource` (AZ-398, batch 57)
|
||||
- ✅ `TlogReplayFcAdapter` (this batch)
|
||||
- ✅ `ReplaySink` + `JsonlReplaySink` (this batch)
|
||||
- ⏳ `compose_replay(config) -> ReplayRoot` (AZ-401)
|
||||
- ⏳ `gps-denied-replay` CLI (AZ-402)
|
||||
- ⏳ `gps-denied-replay-cli` Dockerfile + CI matrix + SBOM diff (AZ-403)
|
||||
- ⏳ E2E replay fixture test (AZ-404)
|
||||
- ⏳ Auto-sync IMU take-off detection (AZ-405)
|
||||
|
||||
Next eligible batch (dependencies satisfied): AZ-401 + AZ-389 (C5 orthorectifier, independent track) — to be selected by the next `/autodev` batch loop.
|
||||
@@ -0,0 +1,103 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 59 (AZ-399, AZ-400)
|
||||
**Date**: 2026-05-14
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Medium | Maintainability | src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:560-720 | MAVLink mapping logic duplicates `_inbound_mavlink.PymavlinkInboundDecoder` |
|
||||
| 2 | Low | Maintainability | src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:457 | `_PRESCAN_MAX_MESSAGES = 6000` is implicit and hard-codes a 30 s @ 200 Hz budget |
|
||||
| 3 | Low | Maintainability | src/gps_denied_onboard/components/c8_fc_adapter/replay_sink.py:184 | `noqa: SIM115` suppression with inline rationale is fine; flag for future re-evaluation if a context-managed alternative emerges |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1: MAVLink mapping logic duplicates the AZ-391 live decoder** (Medium / Maintainability)
|
||||
|
||||
- Location: `src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:560-720`
|
||||
- Description: `_handle_imu`, `_handle_attitude`, `_handle_gps`, `_handle_heartbeat`, `_map_fix_type`, and `_map_mav_state` mirror the AZ-391 `PymavlinkInboundDecoder` implementations almost line-for-line. Today the duplication is intentional — the replay path differs from live in three observable ways: (a) `received_at` is the tlog timestamp + `time_offset_ns` (not `Clock.monotonic_ns()`), (b) `signed=False` is hard-coded per the D-CROSS-CVE-1 risk, (c) non-monotonic timestamps raise `FcOpenError` instead of being dropped, and (d) STATUSTEXT spoof-promotion is intentionally absent because the replay channel carries the recorded stream verbatim. A shared mapper module would have to expose enough seams to model all four differences and would risk losing the readability win.
|
||||
- Suggestion: revisit during AZ-405 (auto-sync) or AZ-403 (cross-component refactor) if a third caller appears. For now, leave the duplication and document the four behavioural deltas in the module docstring.
|
||||
- Task: AZ-399
|
||||
|
||||
**F2: `_PRESCAN_MAX_MESSAGES = 6000` magic number lacks runtime override** (Low / Maintainability)
|
||||
|
||||
- Location: `src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:457`
|
||||
- Description: The pre-scan budget is sized to "≈ 30 s of telemetry at 200 Hz" in the comment, but it is a module constant. A 1 Hz HEARTBEAT-only fixture (e.g., a long-coast clip) might satisfy the IMU/attitude/GPS budget within 6000 records but never see the late HEARTBEAT — leading to a false-negative fail-fast. The current Derkachi fixture sits comfortably under the budget, so this is a future-proofing note, not a present bug.
|
||||
- Suggestion: thread `prescan_max_messages` through the constructor with the current value as default once a real fixture pushes the budget. No change required for batch 59.
|
||||
- Task: AZ-399
|
||||
|
||||
**F3: `# noqa: SIM115` rationale comment** (Low / Maintainability)
|
||||
|
||||
- Location: `src/gps_denied_onboard/components/c8_fc_adapter/replay_sink.py:184`
|
||||
- Description: The comment "owned for the sink lifetime" justifies the suppression — the file handle outlives the constructor by design (close happens in `JsonlReplaySink.close`). Acceptable.
|
||||
- Suggestion: no action; keep the comment for future readers.
|
||||
- Task: AZ-400
|
||||
|
||||
## Phase Summary
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
|
||||
Read inputs:
|
||||
|
||||
- `_docs/02_tasks/todo/AZ-399_replay_tlog_adapter.md`
|
||||
- `_docs/02_tasks/todo/AZ-400_replay_jsonl_sink.md`
|
||||
- `_docs/02_document/contracts/replay/replay_protocol.md`
|
||||
- `_docs/02_document/contracts/c8_fc_adapter/fc_adapter_protocol.md`
|
||||
- `_docs/02_document/module-layout.md`
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
|
||||
**AZ-400** — all 10 ACs covered by `tests/unit/c8_fc_adapter/test_az400_replay_sink.py`. Contract `ReplaySink` Protocol surface (`emit(EstimatorOutput) -> None`, `close() -> None`) matches `replay_protocol.md` v1.0.0; the prior AZ-390 stub was widened in `interface.py` to match this batch's contract. Module-layout home updated.
|
||||
|
||||
**AZ-399** — AC-2 through AC-10 covered by `tests/unit/c8_fc_adapter/test_az399_tlog_replay_adapter.py`; AC-1 (500 MB tlog RSS bound) is present as a `@pytest.mark.skip` placeholder with prerequisite reason, deferred to AZ-404 e2e behind `RUN_REPLAY_E2E=1` per the implement skill's "skipped tests count as Covered" rule. Contract surface (`TlogReplayFcAdapter(tlog_path, target_fc_dialect, clock, wgs_converter, time_offset_ms, pace, fdr_client, source_factory)`) matches the `replay_protocol.md` constructor; Invariants 5, 6, 8 enforced and tested.
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
|
||||
- SOLID: `SubscriptionBus` reused via composition (Invariant 1: replay frames hit identical fan-out shape). `_to_jsonable` is a pure function for testability.
|
||||
- Error handling: explicit `FcOpenError` / `FcAdapterConfigError` / `FcEmitError` / `SourceSetSwitchNotSupportedError` in `tlog_replay_adapter`; explicit `ReplaySinkError` / `ReplaySinkConfigError` in `replay_sink`. No bare `except`.
|
||||
- Naming: clear (`_msg_timestamp_ns`, `_FRAME_PROGRESS_INTERVAL`, `REQUIRED_MESSAGE_TYPES`).
|
||||
- Complexity: longest method ≈ 40 lines (`_dispatch`); under the 50-line threshold.
|
||||
- Tests: every test follows Arrange / Act / Assert with language-appropriate `# Arrange|Act|Assert` markers.
|
||||
- Dead code: none introduced.
|
||||
|
||||
### Phase 4 — Security
|
||||
|
||||
- No SQL / command injection vectors.
|
||||
- No hardcoded secrets.
|
||||
- Tlog file path is operator-supplied and opened via `pymavlink.mavutil.mavlink_connection`; the lazy `pymavlink` import means a missing C extension surfaces as `FcOpenError`, not an `ImportError` at composition time.
|
||||
- Output JSONL path validation already in place from AZ-400 (parent-dir check + binary-mode open).
|
||||
|
||||
### Phase 5 — Performance
|
||||
|
||||
- Stream-parse via `recv_match(blocking=False)` — never materialises the tlog.
|
||||
- Pre-scan ceiling caps file walk at `_PRESCAN_MAX_MESSAGES` (Finding F2 caveat).
|
||||
- AC-7 throughput proxy passes (1000 frames in < 1 s on Tier-1 hardware).
|
||||
- `JsonlReplaySink.emit` is `orjson.dumps` + `write` — microsecond-class.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
|
||||
- AZ-399 and AZ-400 both live under `c8_fc_adapter` and route through the existing component-local plumbing (`SubscriptionBus`, `errors.py`, `fdr_client`).
|
||||
- The AZ-400 `ReplaySink` Protocol is exactly what AZ-401's `compose_replay` will require.
|
||||
- No DTO drift: both files consume `EstimatorOutput`, `FcTelemetryFrame`, `LatLonAlt`, `Quat` from the existing `_types/*` shared layer.
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
|
||||
- Layer direction: `tlog_replay_adapter` and `replay_sink` import from `_types`, `helpers`, `fdr_client`, `clock`, `logging`, plus the own-component `_subscription` / `errors`. All importees are at or above the importer's layer per `module-layout.md` (Layer 4 component → Layer 1 cross-cutting / Layer 2 shared types).
|
||||
- Public API respect: imports are limited to documented public surfaces (`FdrRecord`, `iso_ts_now`, `get_logger`, `WgsConverter`).
|
||||
- No new cyclic dependencies.
|
||||
- No duplicate symbols across components (the AZ-391 mapping duplication noted in F1 is *within* the same component).
|
||||
- Cross-cutting concerns (FDR enqueue, structured logging) consumed via shared helpers — not re-implemented locally.
|
||||
|
||||
## Verdict Logic
|
||||
|
||||
- 0 Critical, 0 High, 1 Medium, 2 Low → **PASS_WITH_WARNINGS**.
|
||||
|
||||
## Outputs
|
||||
|
||||
- `verdict`: PASS_WITH_WARNINGS
|
||||
- `findings`: 3 (1 Medium + 2 Low)
|
||||
- `critical_count`: 0
|
||||
- `high_count`: 0
|
||||
- `report_path`: `_docs/03_implementation/reviews/batch_59_review.md`
|
||||
@@ -12,6 +12,6 @@ sub_step:
|
||||
retry_count: 0
|
||||
cycle: 1
|
||||
tracker: jira
|
||||
last_completed_batch: 58
|
||||
last_completed_batch: 59
|
||||
last_cumulative_review: batches_55-57
|
||||
current_batch: 59
|
||||
current_batch: 60
|
||||
|
||||
Reference in New Issue
Block a user