Update autodev state and dependencies table for Phase 2 progress
ci/woodpecker/push/02-build-push Pipeline failed

- Changed autodev state sub_step to reflect new phase and task details: updated phase from 7 to 2, renamed task to 'refactor-analysis-gate', and revised detail to indicate the creation of new tasks AZ-844, AZ-845, AZ-846, and AZ-847, awaiting Phase-2 gate.
- Updated dependencies table with the latest task counts and complexity points, reflecting the addition of new tasks and the closure of AZ-777 in Jira. Total tasks now stand at 173 with 557 complexity points.
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-23 17:11:50 +03:00
parent ade0c86f2b
commit 9dc04cc677
12 changed files with 758 additions and 6 deletions
@@ -0,0 +1,178 @@
# Cumulative Code Review — Cycle 3 — Batches 104109
**Date**: 2026-05-23
**Scope**: union of files changed across cycle-3 batches 104, 106, 107, 108, 108b, 109
**Tasks covered**: AZ-777 spec refresh + Phase 1 + Phase 2; AZ-836 (Epic AZ-835 C1); AZ-838 (Epic AZ-835 C2); AZ-839 (Epic AZ-835 C3) + 108b fixture-path fix; AZ-840 (Epic AZ-835 C4)
**Mode**: cumulative (all 7 phases)
**Verdict**: **FAIL** (0 Critical, 1 High, 2 Medium, 0 Low)
**Baseline file**: `_docs/02_document/architecture_compliance_baseline.md`**still absent** (carried over from cycle 2 retro action), no `## Baseline Delta` section emitted (see Notes)
## Scope of files reviewed
**Production source** (6 files):
1. `src/gps_denied_onboard/components/c11_tile_manager/tile_downloader.py` — modified (b104; AZ-777 Phase 1 contract adaptation: `_LIST_PATH` / `_GET_PATH` aligned with `Program.cs:187-209`)
2. `src/gps_denied_onboard/components/c11_tile_manager/route_client.py`**new** (b107; ~600 LOC; `SatelliteProviderRouteClient`, `RouteSeedResult`, helpers)
3. `src/gps_denied_onboard/components/c11_tile_manager/errors.py` — modified (b107; new `SatelliteProviderRouteError` + `RouteValidationError` + `RouteTransientError` + `RouteTerminalFailureError`)
4. `src/gps_denied_onboard/components/c11_tile_manager/__init__.py` — modified (b107; re-exports new public surface)
5. `src/gps_denied_onboard/replay_input/tlog_route.py`**new** (b106; `RouteSpec`, `RouteExtractionError`, `extract_route_from_tlog`)
6. `src/gps_denied_onboard/replay_input/__init__.py` — modified (b106; re-exports new public surface)
**Tests** (10 files): `tests/unit/c11_tile_manager/test_tile_downloader.py` (rewritten, b104; 14 ACs), `tests/unit/replay_input/test_tlog_route.py` (new, b106; 14 tests), `tests/e2e/satellite_provider/__init__.py` + `test_smoke.py` (new, b104; 2 tier-2 tests), `tests/e2e/replay/_operator_pre_flight.py` (new, b108; ~430 LOC), `tests/e2e/replay/conftest.py` (modified, b108+b108b), `tests/e2e/replay/test_operator_pre_flight_driver.py` (new, b108; 11 unit tests), `tests/e2e/replay/_e2e_orchestrator.py` (new, b109; 656 LOC), `tests/e2e/replay/test_e2e_orchestrator_unit.py` (new, b109; 17 unit tests), `tests/e2e/replay/test_az835_e2e_real_flight.py` (new, b109; tier-2 integration).
**CLI / fixtures** (2 files): `tests/fixtures/derkachi_c6/seed_route.py` (new, b107), `scripts/mint_dev_jwt.py` (new, b104).
**Compose / env** (2 files): `docker-compose.test.jetson.yml` (modified, b104), `.env.test.example` (modified, b104).
## Findings
| # | Severity | Category | File | Title |
|---|----------|----------|------|-------|
| F1 | High | Architecture | `src/gps_denied_onboard/components/c11_tile_manager/route_client.py:56` | `RouteSpec` DTO placement violates AZ-507 cross-component contract surface |
| F2 | Medium | Architecture | `_docs/02_document/module-layout.md` | Module layout stale — cycle-3 additions unregistered (cycle-2 carry-over worsened) |
| F3 | Medium | Maintainability | `tests/unit/test_az270_compose_root.py:194` | `test_ac6_only_compose_root_imports_concrete_strategies` lint scope is narrower than module-layout.md rule 9 |
### Finding Details
**F1: `RouteSpec` DTO placement violates AZ-507 cross-component contract surface** (High / Architecture)
- Location: `src/gps_denied_onboard/components/c11_tile_manager/route_client.py:56`
- Description: `route_client.py` (a `components/c11_tile_manager/*.py` file) imports `RouteSpec` from `gps_denied_onboard.replay_input.tlog_route`. Per `module-layout.md` rule 9 (AZ-507 cross-component contract surface):
> "the only places a `components/<X>/*.py` file may import are: its own subpackage (`gps_denied_onboard.components.<X>.*`), `_types/*`, `_types.inference_errors`, `helpers/*`, `config`, `logging`, `fdr_client`, `clock`, `frame_source` (interface only)."
`replay_input` is not in this allow-list. The architecture rationale: cross-component DTOs reach consumers through `_types/*`, not through cross-cutting coordinator packages. The current placement makes c11 (an Adapter, Layer 4) structurally depend on `replay_input` (a coordinator, Layer 4) — a Layer 4 → Layer 4 cross-cutting edge that the layering table does not declare as allowed.
- Impact: The dependency is **intentional and documented** — AZ-838 task spec line 19 explicitly specifies `from gps_denied_onboard.replay_input.tlog_route import RouteSpec`, and the route_client docstring acknowledges the source (`Takes a gps_denied_onboard.replay_input.tlog_route.RouteSpec (produced by AZ-836 / C1)`). But "intentional" does not equal "compliant"; the architecture rule was not amended at decompose time, and the AZ-270 lint is too narrow to catch this case (see F3). The next task that imports a similarly-placed DTO will compound the drift.
- Suggestion: relocate `RouteSpec` (plus `RouteExtractionError` if exported as part of the cross-component surface) to `src/gps_denied_onboard/_types/route.py`. After the move, both `c11_tile_manager.route_client` and `replay_input.tlog_route` import the DTO from `_types`, which is in both modules' allow-lists. AZ-836's `extract_route_from_tlog` continues to live in `replay_input/`; AZ-838's `SatelliteProviderRouteClient` continues to live in `c11_tile_manager/`. The behavioral surface is unchanged. Estimated complexity: 2 SP (move + update imports + verify AZ-838/AZ-836 tests + module-layout.md update).
- Tasks: AZ-838 (primary — owns the violating import), AZ-836 (secondary — owns the DTO definition).
**F2: Module layout stale — cycle-3 additions unregistered (cycle-2 carry-over worsened)** (Medium / Architecture)
- Location: `_docs/02_document/module-layout.md`
- Description: cycle 3 introduced new package files that are not registered in the authoritative file-ownership map. The cycle-2 cumulative review (`98-102`) already flagged 6 unregistered cycle-2 additions (F1 there); none of those carry-overs have been resolved, and cycle 3 added more:
- **c11_tile_manager Internal list** (currently lists `satellite_provider_downloader.py` + `satellite_provider_uploader.py`): missing `_types.py`, `config.py`, `errors.py`, `idempotent_retry.py`, `signing_key.py`, `tile_downloader.py`, `tile_uploader.py`, **`route_client.py`** (cycle-3 NEW).
- **shared/replay_input file list** (currently lists `__init__.py`, `interface.py`, `tlog_video_adapter.py`, `auto_sync.py`, `tests/`): missing `errors.py` (cycle-2 carry), `tlog_ground_truth.py` (cycle-2 carry), **`tlog_route.py`** (cycle-3 NEW).
- **Carried over from cycle-2 review** (still unregistered): `replay_api/` package (7 files), `cli/render_map.py`, `cli/replay_api_entrypoint.py`, `helpers/gps_compare.py`, `helpers/accuracy_report.py`.
- Impact: `/implement` Step 4 (File Ownership) resolves a task's `Component` field against this file. Any future task touching the unregistered areas will hit the BLOCKING ownership check at Step 4 — the skill explicitly STOPs when the component isn't found and forbids guessing from prose. Cycle-3 batches 104109 happened to operate inside already-listed component directories (c11_tile_manager/**, replay_input/**) so the staleness did not block them, but the next task that needs a new component or extends `replay_api/` will block.
- Suggestion: cycle-3 Step 13 (Update Docs) should reconcile module-layout.md with on-disk reality. The minimum: refresh the c11_tile_manager Internal list, the shared/replay_input file list, and add the cycle-2 carry-over entries (replay_api Per-Component Mapping entry, cli additions, helpers additions, replay_input file list completion). Severity escalates to High if a fourth consecutive cycle leaves the file stale.
- Tasks: AZ-838, AZ-836 (primary, cycle-3 contributors); AZ-700, AZ-697, AZ-699, AZ-701 (secondary, cycle-2 carry).
**F3: `test_ac6_only_compose_root_imports_concrete_strategies` lint scope is narrower than module-layout.md rule 9** (Medium / Maintainability)
- Location: `tests/unit/test_az270_compose_root.py:194-219`
- Description: `module-layout.md` rule 9 documents `test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies` as the lint that "enforces this on every `components/**/*.py`". In practice the lint only checks for `gps_denied_onboard.components.<other_component>` import edges — it walks `components/**/*.py`, parses `ImportFrom` nodes, and flags only when `node.module.startswith("gps_denied_onboard.components.")` with a different leaf component. The full rule-9 allow-list (`_types/*`, `_types.inference_errors`, `helpers/*`, `config`, `logging`, `fdr_client`, `clock`, `frame_source` interface only) is NOT enforced. Imports from `replay_input`, `replay_api`, `runtime_root`, `cli/*`, `frame_source` non-interface modules, etc. all pass the lint silently. F1 is the concrete consequence: the c11 → replay_input import slipped through both code review and the AZ-270 lint.
- Impact: `module-layout.md` rule 9 is documented as enforced; in practice it is partially enforced, partially honor-system. Reviewers (human or AI) reading the rule-9 paragraph reasonably assume the lint covers it; the test name and docstring reinforce that. The asymmetry is a maintainability risk — the rule and its enforcement diverge silently.
- Suggestion: either expand `test_ac6_only_compose_root_imports_concrete_strategies` to enforce the full allow-list (one extra branch in the AST walker), or amend rule 9 to admit the additional imports the codebase actually relies on (with a documented rationale per module). The first is preferable — the rule's intent is structural, and lint coverage matters more than rule wording.
- Tasks: cross-cutting; surface in cycle-3 retrospective.
## Verdict Logic
- 0 Critical → no FAIL trigger from Critical
- 1 High (F1) → **FAIL trigger**
- 2 Medium (F2, F3) → not a verdict driver
- 0 Low
Result: **FAIL**`/implement` Step 14.5 gate stops. Per `implement/SKILL.md` Step 14.5 + the auto-fix matrix, F1 (High Architecture) **escalates** rather than auto-fixes; F2 + F3 are eligible for Medium-Style auto-fix on the matrix but the High-Architecture finding alone gates the whole report. Re-run requires user direction (Choose A/B/C in the implement skill's Step 14.5 escalation block).
## Phase-by-Phase Notes
### Phase 1 — Context Loading
Inputs read:
- Task specs: AZ-836 (`done/`), AZ-838 (`done/`), AZ-839 (`done/`), AZ-840 (`done/`), AZ-777 (refreshed spec; closure logged in `done/`); AZ-841 (`todo/`), AZ-842 (`todo/`); Epic AZ-835 (`todo/`).
- Batch reports: `batch_104_cycle3_report.md`, `batch_106_cycle3_report.md`, `batch_107_cycle3_report.md`, `batch_108_cycle3_report.md`, `batch_108b_cycle3_report.md`, `batch_109_cycle3_report.md`.
- Architecture / layout: `_docs/02_document/module-layout.md` (rule 9 + per-component sections + Layering Table); `_docs/02_document/architecture.md` (header read-through; full re-read deferred to per-finding evidence).
- Last cumulative review: `_docs/03_implementation/cumulative_review_batches_98-102_cycle2_report.md` (carry-over baseline).
- Restrictions / solution overview: not re-read (already covered in per-batch reviews).
- ADR directory: `_docs/02_document/adr/` does NOT exist; ADR compliance check skipped (logged in Phase 7 below).
### Phase 2 — Spec Compliance
Cross-batch promise points (per-batch ACs already verified in batch reports):
- **AZ-836 (`RouteSpec` + extractor) → AZ-838 (`SatelliteProviderRouteClient`)**: AZ-838 task spec line 19 explicitly specifies `from gps_denied_onboard.replay_input.tlog_route import RouteSpec`. Implementation matches. The DTO contract is not formally documented in `_docs/02_document/contracts/c11_tilemanager/` — Spec-Gap candidate, but downgrades because both producer and consumer are owned by the same Epic (AZ-835) and the Epic spec describes the DTO shape inline. Note (not a separate finding): if `RouteSpec` survives F1 remediation by moving to `_types/route.py`, a contract `_docs/02_document/contracts/shared_types/route.md` is the right home.
- **AZ-838 (`SatelliteProviderRouteClient`) → AZ-839 (C3 fixture, `populate_c6_from_route`)**: the fixture's driver imports `SatelliteProviderRouteClient` and uses `seed_route()`; signature matches AZ-838's `seed_route(spec, *, name=None) -> RouteSeedResult`. Cross-batch wiring sound.
- **AZ-839 (C3 fixture, `PopulatedC6Cache`) → AZ-840 (orchestrator)**: AZ-840's `_e2e_orchestrator.write_effective_replay_config` overlays `c6_tile_cache.root_dir` onto the operator YAML using the cache_root the C3 fixture chose. AZ-840 batch report documents the contract; per-test fixtures consume `PopulatedC6Cache` directly. Sound.
- **AZ-777 contract adaptation (b104) → satellite-provider real endpoints**: `tile_downloader.py` `_LIST_PATH` / `_GET_PATH` now point at the real endpoints (`/api/satellite/tiles/inventory` + `/tiles/{z}/{x}/{y}`). The leftover `_docs/_process_leftovers/2026-05-21_az777_complexity_override.md` 2026-05-21 addendum recorded this as the "largest single sub-deliverable of the refreshed Phase 1". Implementation matches.
No Spec-Gap findings.
### Phase 3 — Code Quality
- All cycle-3 production modules (`tlog_route.py`, `route_client.py`, expanded `errors.py`, modified `tile_downloader.py`) carry module + class + function docstrings consistent with the project pattern (cycle-2 baseline preserved).
- `route_client.py` is ~600 LOC with one class (`SatelliteProviderRouteClient`) plus one DTO (`RouteSeedResult`) plus module-level helpers. The class has 5 public methods (validate, seed_route, _post_route, _poll_route_status, _verify_inventory). Each method is single-responsibility. No method exceeds the 50-line / cyclomatic-10 thresholds enumerated in the skill's Phase 3 list (per code reading; not measured).
- `tlog_route.py` `extract_route_from_tlog` uses Douglas-Peucker for waypoint coarsening — correct choice per AZ-836 spec.
- Tests follow Arrange / Act / Assert per coderule (verified by sampling `test_tlog_route.py` and `test_e2e_orchestrator_unit.py`; no exhaustive enumeration).
No Code Quality findings.
### Phase 4 — Security Quick-Scan
- `route_client.py` HTTP client uses `httpx.Client` with `timeout` parameter (no infinite hangs), argv-style request construction (no shell), and bearer-token auth via the existing C11 plumbing. No secrets in source.
- `route_client.py` JSON request payload built via `json.dumps` on dataclass fields → no injection.
- `route_client.py` URL construction uses `_ROUTE_STATUS_PATH_TPL.format(id=...)` where `id` is a UUID returned by the server — type-bounded, no injection surface.
- `tile_downloader.py` modifications (b104) are confined to `_LIST_PATH` / `_GET_PATH` constants (per batch report); no new auth/parsing surface.
- `scripts/mint_dev_jwt.py` (new, b104): JWT minting tooling for dev/test JWT signing keys. Per file naming (`mint_dev_jwt.py`) and per the `.env.test.example` pairing this is intended for non-prod use; not reviewed line-by-line in this pass.
No Security findings.
### Phase 5 — Performance Scan
- `route_client._poll_route_status` polls with default 5 s interval, max 60 attempts (= 5 min ceiling) using `time.sleep`. Configurable via constructor. Standard polling, not a perf concern.
- `route_client._enumerate_route_tile_coords` walks the route's `regionSizeMeters × N waypoints` tile coverage locally; per AZ-838 batch report this is ~50100 tiles for the Derkachi route. O(N) over waypoints.
- `tlog_route.extract_route_from_tlog` runs Douglas-Peucker on the active GPS segment; per the unit test, completes in milliseconds for the Derkachi clip.
- `_operator_pre_flight.py` and `_e2e_orchestrator.py` run inside the test harness; performance is bounded by the wall-clock budget (15 min on Tier-2).
No Performance findings.
### Phase 6 — Cross-Task Consistency
- **Sequential Epic chain**: AZ-836 (C1) → AZ-838 (C2) → AZ-839 (C3) → AZ-840 (C4). Each batch's "Files changed" is disjoint at the production level (C1 in `replay_input/`, C2 in `c11_tile_manager/`, C3+C4 in `tests/e2e/replay/`). No conflicting patterns; the test layer wires the production chain together via the orchestrator.
- **Symbol uniqueness**: `RouteSpec`, `RouteExtractionError`, `extract_route_from_tlog`, `SatelliteProviderRouteClient`, `RouteSeedResult`, `SatelliteProviderRouteError`, `RouteValidationError`, `RouteTransientError`, `RouteTerminalFailureError`, `OrchestratorStep`, `OrchestrationFailure`, `OrchestrationReport`, `PopulatedC6Cache` — each defined exactly once across cycle-3 production + tests. No duplicates.
- **AZ-839 b108b fix**: the hot-fix renamed `tile_store_path = cache_root / "tile_store"``cache_root / "tiles"` to match `PostgresFilesystemStore` layout. Cross-task consistency preserved (the path AZ-840 reads now matches the path AZ-839 writes).
No Cross-Task Consistency findings.
### Phase 7 — Architecture Compliance
**Layer-direction analysis** (against module-layout.md "Allowed Dependencies" + rule 9):
- `replay_input/tlog_route.py` (Layer 4 cross-cutting coordinator): imports `_types.geo` (Layer 1), `helpers.gps_compare` (Layer 1), `helpers.wgs_converter` (Layer 1), and intra-package `replay_input.errors` + `replay_input.tlog_ground_truth`. All imports are downward (Layer 4 → Layer 1) or intra-package. Compliant.
- `c11_tile_manager/route_client.py` (Layer 4 component): imports own subpackage (`c11_tile_manager.errors`) + third-party (`httpx`) + **`replay_input.tlog_route.RouteSpec`** — see F1. The cross-cutting `replay_input` is not in c11's allow-list per rule 9. Architecture finding F1 (High).
- `c11_tile_manager/tile_downloader.py` (Layer 4 component): modifications confined to constants. No new cross-component edges introduced.
**Public API respect**:
- `c11_tile_manager.__init__.py` re-exports the new public surface (`RouteSeedResult`, `SatelliteProviderRouteClient`, plus the new error classes). Consumers calling `from gps_denied_onboard.components.c11_tile_manager import SatelliteProviderRouteClient` reach the package's public surface. ✅
- `replay_input.__init__.py` re-exports `RouteSpec`, `RouteExtractionError`, `extract_route_from_tlog`. ✅
- The F1 violation is a public API respect violation in the OPPOSITE direction: `c11.route_client` reaches into `replay_input.tlog_route` (a sub-module path) rather than the package's `__init__` re-export — but the deeper issue is that no direction of this import is rule-9-compliant.
**Cyclic-dependency check**:
- New edges this cycle: `c11_tile_manager.route_client → replay_input.tlog_route` (F1) + `c11_tile_manager.route_client → c11_tile_manager.errors` (intra-package).
- `replay_input.tlog_route → c11_tile_manager.*`? No (verified via grep). Acyclic.
- `replay_input/__init__.py` re-exports `RouteSpec` from `tlog_route`. No back-edge to c11.
- No new cycles introduced.
**Duplicate-symbol check**: see Phase 6 — no duplicates.
**Cross-cutting concerns not locally re-implemented**: none observed. Logging via `logging.getLogger(_COMPONENT)`, FDR via `fdr_client`, helpers consumed from canonical locations.
**ADR compliance**: `_docs/02_document/adr/` directory does **not exist**. The check is skipped per `code-review/SKILL.md` Phase 7 #6 ("If the directory does not exist or has only the index file, ADRs are skipped — log this skip in the report so the absence is visible"). Carry-over: `module-layout.md` references ADR-001 (monolith), ADR-002 (build-time exclusion), ADR-009 (interface-first DI), ADR-011 (replay-as-configuration) inline; these are documented in `architecture.md` but not as standalone ADR files. If the ADR directory is created in cycle-N (per a future retro action), this skip should retroactively re-evaluate the cycle-3 batches against any ADR whose `Evidence` overlaps the cycle-3 changed-file set.
**Single Architecture finding**: F1 — c11.route_client imports a non-allow-listed package. Documented but unaddressed at the architecture level.
## Notes
- **No `## Baseline Delta` section**: `_docs/02_document/architecture_compliance_baseline.md` was identified in the cycle-2 LESSONS entry (2026-05-20 architecture) and again in the cycle-2 cumulative review notes as a cycle-2 Step 6 (Decompose) prerequisite. The baseline file was NOT created in cycle 2 retrospective and was NOT created in cycle 3 either. Carry-over → cycle-3 retrospective. Without the baseline, "carried over / resolved / newly introduced" structural-violation accounting is not possible; F1 is therefore counted as "newly introduced this cycle" by inspection (`route_client.py` is a cycle-3-new file), and F2 is "carried over from cycle 2 with worsening" by inspection of the cycle-2 cumulative review F1.
- **Cumulative-review cadence drift continues**: `/implement` Step 14.5 says K=3 default. Cycle 3 has 6 completed batches (104, 106, 107, 108, 108b, 109) without a cumulative review until this make-up review. Two cumulative reviews were due (after 104+106+107, after 108+108b+109). Cycle-2 cumulative review (`98-102`) noted the same drift and flagged it for the cycle-2 retrospective; the action did not land. Recurring. Cycle-3 retrospective should pick it up — possible mechanism: a `cumulative_review_pending: true` marker in `_docs/_autodev_state.md` that the implement skill flips on at K-batch boundaries and clears only on review file write, surfacing in the autodev Status Summary footer.
- **AZ-270 lint coverage gap**: F3 documents the gap explicitly. Adjacent: the existing-code flow's Phase A Step 2 (Architecture Baseline Scan) feeds Step 4 (Code Testability Revision) and would also benefit from a tighter lint, since baseline-mode code-review uses the same `module-layout.md` rule 9 as enforcement input.
- **Suite docs (parent)**: `<workspace-root>/../docs` does not exist (probed during R1 reconciliation). No suite-level cross-reference applies to this review.
## Artifacts
- Verdict consumed by: `/implement` Step 14.5 gate (FAIL → STOP, escalate via Choose A/B/C — auto-fix not eligible for High Architecture).
- F1 carried forward to cycle-3 retrospective for action assignment; remediation candidate: 2-SP refactor task to relocate `RouteSpec` to `_types/route.py`.
- F2 carried forward to cycle-3 Step 13 (Update Docs) at minimum; severity escalation watch if the staleness persists into cycle 4.
- F3 carried forward to cycle-3 retrospective; remediation candidate: 1-SP test-update task to expand `test_ac6_only_compose_root_imports_concrete_strategies`.
- Architecture compliance baseline action: blocked across cycle 2 → cycle 3; surface in cycle-3 retrospective with explicit owner.