# Cumulative Code Review — Batches 01–03 (Cycle 1)

**Trigger**: `implement/SKILL.md` Step 14.5 — `K=3` batches completed
**Date**: 2026-05-19
**Cycle**: 1
**Scope**: union of files changed in `batch_01_cycle1`, `batch_02_cycle1`, `batch_03_cycle1`
**Mode**: inline (matching the per-batch precedent set in batches 1–2; `/code-review` sub-skill deliberately skipped to conserve context)
**Baseline**: `_docs/02_document/architecture_compliance_baseline.md` does NOT exist yet — no Baseline Delta section is produced; this run becomes the de-facto baseline that future cumulative reviews will compare against.

## Tasks in scope

| Batch | Tasks                                                 | Component(s)                                                           |
|-------|-------------------------------------------------------|------------------------------------------------------------------------|
| 01    | AZ-640 `initial_structure`                            | workspace, CI/Docker, observability scaffold                           |
| 02    | AZ-641, AZ-642, AZ-644                                | `mavlink_layer` (transport + heartbeat + codec), `mission_client` (pull) |
| 03    | AZ-645, AZ-646, AZ-647                                | `mission_client` (waypoint POST + mapobjects pull/push)                |

## Phase 1 — Spec coverage

Every Included scope item across these 7 tasks is implemented in production code:

- AZ-640: workspace + crates + per-crate Public API stubs + CI + Docker bootstrap.
- AZ-641: UDP + serial transport, heartbeat 1 Hz, link-state broadcast.
- AZ-642: MAVLink v2 codec (encode + decode + CRC + 17 supported messages + truncation).
- AZ-644: GET /missions/{id} + schema validation + bounded retry + typed errors.
- AZ-645: POST /missions/{id}/middle-waypoint + bounded retry + ack parsing.
- AZ-646: GET /missions/{id}/mapobjects + schema validation + typed errors + health.
- AZ-647: write-ahead disk queue + per-endpoint POST + crash-recovery sweep.

Deferred Excluded items remain unimplemented and explicitly out-of-scope (signing AZ-643, mapobjects cache in mapobjects_store AZ-665+, BIT orchestration in mission_executor AZ-650, terminal-state decisions in mission_executor AZ-648/652).

## Phase 2 — Architecture compliance

Layering table (`module-layout.md §Allowed Dependencies`) is enforced by `cargo tree`-grade reality:

| Component       | Layer | `Imports from` (per module-layout.md) | Actual deps                      | Status |
|-----------------|-------|---------------------------------------|----------------------------------|--------|
| `shared`        | 1     | —                                     | external only                    | ✓      |
| `mavlink_layer` | 2     | `shared`                              | `shared`, plus external (tokio, reqwest, …) | ✓      |
| `mission_client`| 2     | `shared`                              | `shared`, plus external (reqwest, jsonschema, uuid, …) | ✓      |
| autopilot bin   | 5     | 1, 2, 3, 4                            | currently 1 + the Layer-2 actors (Layer 3/4 still scaffold) | ✓ (will expand as later batches add coordinators) |

No Layer 2 → Layer 2 violation introduced. No same-layer crate imports another same-layer crate.

Public API surface for the two Layer 2 actors implemented so far matches the module-layout entries:

- `mavlink_layer::{MavlinkLayer, MavlinkHandle, MavlinkConnection, …}` — present.
- `mission_client::{MissionClient, MissionClientHandle, Mission, MapObjectsDiff, MissionUpdateAck, FetchError, PostError, PullError, PushReport, PerEndpointStatus, …}` — present. (`module-layout.md` listed `push_mapobjects()`; we implemented the task-spec form `push_mapobjects_diff(mission_id, diff)` plus the new `recover_pending_pushes()` — recorded as W4 in batch 03 report; doc fix in Step 13.)

## Phase 3 — Code quality (cross-batch)

- **SRP**: each module/crate has one reason to change. mavlink_layer codec is split into `crc / messages / encoder / decoder / parse_errors / transport / heartbeat`; mission_client is split into `missions_api / mapobjects_sync / schema / retry`. No god modules.
- **Error handling**: typed errors at every crate boundary; no `.unwrap()` on runtime paths except the once-init schema-compile `OnceLock` (compile-time correctness). `RawHttpError` is exhaustively classified into `FetchError` / `PostError` / `PullError` / `PerEndpointStatus`.
- **No silent suppression**: CRC mismatches, schema failures, transient HTTP errors, write-ahead disk failures, partial-success push outcomes — all surface to typed counters, logs, or per-endpoint statuses.
- **Tests follow Arrange / Act / Assert** per `coderule.mdc`.

## Phase 4 — Test quality (cross-batch)

| Layer                            | Test count | Test technology                                       |
|----------------------------------|------------|-------------------------------------------------------|
| mavlink_layer unit               | 21         | in-process                                            |
| mavlink_layer codec_round_trip   | 3          | real codec, 17 messages                               |
| mavlink_layer udp_link           | 4          | real `tokio::net::UdpSocket` loopback                 |
| mavlink_layer serial_link        | 1 ignored  | `#[ignore]` with documented `socat`-pty prereq        |
| mission_client unit              | 15         | in-process (incl. 4 disk-queue + 5 mapobjects-schema) |
| mission_client pull_mission      | 5          | `wiremock` HTTP                                       |
| mission_client waypoint_post     | 4          | `wiremock` HTTP                                       |
| mission_client mapobjects_pull   | 4          | `wiremock` HTTP + real-port-refused                   |
| mission_client mapobjects_push   | 5          | `wiremock` HTTP + `tempfile` real FS                  |
| shared                           | 6          | in-process                                            |
| stub crates                      | 1 each     | smoke                                                 |

No fakes for HTTP, sockets, or disk inside the test boundary. External services (ArduPilot SITL, central missions API) are stubbed at the wire boundary only.

## Phase 5 — Docs alignment

- `architecture.md`, `module-layout.md`, `data_model.md`, per-component `description.md` files were used as authoritative inputs throughout. No code path drifted from a documented capability.
- Crate-level Rust doc comments call out which AZ-NNN owns each surface.
- Shared schemas (`mission-schema.json`, `mapobjects-{bundle,observations,ignored}.json`) carry their architectural pointer in `description`.
- **Pending doc updates** (will be addressed in greenfield Step 13 — Update Docs):
  - `module-layout.md` Public API for `mission_client` should list `push_mapobjects_diff` + `recover_pending_pushes` (the task spec finalised these names after the module layout was authored).
  - `data_model.md §MapObjectsBundle` should explicitly point at `crates/shared/contracts/mapobjects-bundle.json` as the wire contract (currently points only at the typed Rust model).

## Phase 6 — Cross-task consistency

Concerns that span batches:

- **`ExponentialBackoff` duplication** (W2 from batch 02, W3 from batch 03). Now used at 5 call sites in 2 crates: `mavlink_layer::internal::retry` (1 call site for the reconnect ladder) and `mission_client::internal::retry` (4 call sites: pull_mission GET, middle-waypoint POST, observations POST, ignored POST). Each crate's retry policy diverges (`mavlink_layer` uses fixed multipliers; `mission_client` uses options-driven base/cap), so a naive merge into `shared::retry` would lose nuance. **Promote when the third crate joins** (likely `detection_client` in AZ-660 / AZ-661). Recorded as a refactor candidate; not a blocking finding now.
- **Options struct growth**: `MissionClientOptions` gained `post_max_attempts`, `push_max_attempts`, `state_dir` in batch 03. The struct is not `#[non_exhaustive]`, so an external consumer (none yet) constructing it directly would break. Since the crate currently has zero external callers (no `mission_executor` integration yet), this is acceptable; add `#[non_exhaustive]` when AZ-648 starts consuming the API.
- **Naming consistency between docs and code**: see W4 in batch 03 (`push_mapobjects` vs `push_mapobjects_diff`). Resolved at code level by following the task spec; doc level pending Step 13.
- **`Mission::start(Vec<MissionItem>)` vs eventual `Mission::start(Mission)`**: the rename will happen in AZ-648. No current call sites broken.
- **Schema wire contracts are local-only**: all four schemas (`mission-schema.json` + 3 mapobjects) live in `crates/shared/contracts/`. The architecture.md `§8 Q5` open question on missions-repo extraction is still open — recorded as W5 in batch 03 report. Add a schema-snapshot regression test when the missions repo extraction lands.

## Phase 7 — Architecture compliance (re-confirmation, post-batch-03)

| Check                                          | Result |
|------------------------------------------------|--------|
| No cyclic crate dependencies                   | ✓      |
| No Layer 2 → Layer 2 import                    | ✓      |
| No Layer 3 → Layer 3 import                    | ✓ (no Layer 3 implemented yet) |
| Public API matches `module-layout.md`          | ✓ (with W4 follow-up at the doc level) |
| Forbidden technologies absent                  | ✓ (no `mavlink`-rs, no pymavlink-bindgen, no OpenSSL on the airframe) |
| Frozen choices (`architecture.md`) respected   | ✓ (in-flight central writes forbidden — AZ-647 enforces via terminal-state-only push; durable queue lives on disk) |

## Duplicate symbol detection

Run by `cargo build` itself (each crate is its own compilation unit) and verified by `cargo doc` namespace inspection:

- No two crates expose a public type with the same fully-qualified path.
- No two `tests/*.rs` integration test files define a function with the same name (rustc would error).
- `ExponentialBackoff` is intentionally duplicated across `mavlink_layer` and `mission_client` (different defaults, different retry policies); see Phase 6 for the promotion-trigger note.

## Verdict

**PASS_WITH_WARNINGS**.

The PASS_WITH_WARNINGS verdict carries forward the per-batch W1–W5 findings (state_dir default, push-budget defaults vs 24h NFR, retry duplication, push_mapobjects naming, schema co-ownership). All are non-blocking and have explicit follow-up triggers (composition-root wiring, third-crate retry promotion, Step 13 doc sync, missions-repo extraction).

Auto-Fix Gate matrix (`implement/SKILL.md` Step 10): no Critical / Security / High-Architecture findings; all open warnings are Low-Maintainability or doc-drift. No escalation required.

## Continuation

Proceed to batch 04. Recommended task selection: see `batch_03_cycle1_report.md` → "Next Batch".
