mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 02:41:14 +00:00
[AZ-508] Consolidate _iso_ts_now into helpers/iso_timestamps
Batch 48 / Cycle 1 (greenfield Step 7). Closes cumulative review batches 31-33 F2 and 28-30 F3 by replacing the duplicated private _iso_ts_now() one-liners with a single Layer-1 helper: src/gps_denied_onboard/helpers/iso_timestamps.py iso_ts_now() -> str Output format matches the canonical FDR _TS fixture (YYYY-MM-DDTHH:MM:SS.ffffffZ); no FDR schema change. Migrated call-sites (3): c7_inference/onnx_trt_ep_runtime, c7_inference/thermal_publisher, plus the 3 c6_tile_cache callers that previously imported from the local c6_tile_cache/_timestamp shim (now deleted, superseded by the Layer-1 helper). Spec drift resolved (Choose A, user-approved): spec listed 5 call sites + +00:00 regex; on-disk reality at batch start is 3 sites + Z-suffix matching every existing helper and the FDR _TS fixture. Spec preamble + AC-2 regex updated in the task file; documented in batch_48_cycle1_report.md. Tests: 9 new AC tests (AC-1..AC-7 + Layer-1 invariant + public-surface defensive); 216 focused tests pass including the unmodified AZ-272 FDR schema suite and AZ-270 / AZ-507 layering lints. Verdict: PASS (no findings). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,222 @@
|
||||
# Batch 48 / Cycle 1 — Implementation Report
|
||||
|
||||
**Date**: 2026-05-13
|
||||
**Tasks**: AZ-508 — ISO-timestamp helper consolidation (2pt)
|
||||
**Total complexity**: 2 points
|
||||
**Result**: PASS (per-batch code review)
|
||||
**Jira tracker state**: AZ-508 transitioned To Do → In Progress (prior
|
||||
session) → In Testing (this batch)
|
||||
|
||||
## Scope
|
||||
|
||||
Hygiene PBI from cumulative reviews batches 28–30 (Finding F3) and
|
||||
batches 31–33 (Finding F2). Consolidates the duplicated private
|
||||
`_iso_ts_now()` one-liners across `c6_tile_cache` and `c7_inference`
|
||||
into a single Layer-1 helper at
|
||||
`src/gps_denied_onboard/helpers/iso_timestamps.py` (`iso_ts_now() -> str`,
|
||||
RFC 3339 UTC microsecond `Z`-suffix). Closes F2 before the upcoming
|
||||
C2 batches (AZ-339 / AZ-340) and the C4/C3 batches (AZ-358 / AZ-349)
|
||||
would have added the 8th and 9th copies of the helper.
|
||||
|
||||
## Spec Drift Resolution (User-Approved Choose A)
|
||||
|
||||
The task spec assumed:
|
||||
- **5** call-sites needing migration.
|
||||
- AC-2 format regex `\.\d{6}\+00:00$`.
|
||||
- "FDR records standardize on `+00:00` per `test_az272_fdr_record_schema.py`".
|
||||
|
||||
On-disk reality at Batch 48 start:
|
||||
- **3** call-sites: c6's three callers had already been locally
|
||||
consolidated under `src/gps_denied_onboard/components/c6_tile_cache/_timestamp.py`
|
||||
(export `iso_ts_now`, same `Z`-suffix format); `tensorrt_runtime.py`
|
||||
carries no local `_iso_ts_now` definition.
|
||||
- All 3 existing helpers produce `%Y-%m-%dT%H:%M:%S.%fZ` (`Z` suffix).
|
||||
- The canonical FDR `ts` fixture is `_TS = "2026-05-11T00:00:00.000000Z"`
|
||||
(also `Z` suffix). The `+00:00` strings in that test file belong to
|
||||
*other* DTO fields (manifest `generated_at_iso`, `observed_at_iso`),
|
||||
not the FDR `ts` envelope.
|
||||
|
||||
Picking the spec's `+00:00` literally would have silently changed FDR
|
||||
record `ts` bit-shape — explicitly **Excluded** by the spec ("no schema
|
||||
change") and would have broken AC-5 (FDR schema tests pass unmodified).
|
||||
|
||||
User selected **Choose A**: Z-format helper, byte-identical to existing
|
||||
3 helpers + FDR `_TS` fixture; AC-2 regex corrected in the spec; drift
|
||||
documented in this report and in the spec's preamble.
|
||||
|
||||
## Files Changed
|
||||
|
||||
### Production (new)
|
||||
|
||||
- `src/gps_denied_onboard/helpers/iso_timestamps.py` —
|
||||
`iso_ts_now() -> str`. Stateless free function; stdlib `datetime` +
|
||||
`timezone` only; format
|
||||
`datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S.%fZ")`.
|
||||
|
||||
### Production (modified)
|
||||
|
||||
- `src/gps_denied_onboard/helpers/__init__.py` — re-exports `iso_ts_now`
|
||||
through the helpers package facade (alphabetical insertion in both
|
||||
the import block and `__all__`).
|
||||
- `src/gps_denied_onboard/components/c6_tile_cache/cache_budget_enforcer.py` —
|
||||
import path `c6_tile_cache._timestamp` → `helpers.iso_timestamps`
|
||||
(one-line edit; call-sites untouched).
|
||||
- `src/gps_denied_onboard/components/c6_tile_cache/postgres_filesystem_store.py` —
|
||||
same path swap (2 call-sites untouched).
|
||||
- `src/gps_denied_onboard/components/c6_tile_cache/freshness_gate.py` —
|
||||
same path swap (2 call-sites untouched).
|
||||
- `src/gps_denied_onboard/components/c7_inference/onnx_trt_ep_runtime.py` —
|
||||
removed local `def _iso_ts_now`; added top-of-file
|
||||
`from gps_denied_onboard.helpers.iso_timestamps import iso_ts_now as _iso_ts_now`
|
||||
(preserves the `_iso_ts_now` symbol at 2 call-sites); removed the
|
||||
now-unused `from datetime import datetime, timezone`.
|
||||
- `src/gps_denied_onboard/components/c7_inference/thermal_publisher.py` —
|
||||
same pattern (1 call-site preserved); removed the now-unused
|
||||
`from datetime import datetime, timezone`.
|
||||
|
||||
### Production (deleted)
|
||||
|
||||
- `src/gps_denied_onboard/components/c6_tile_cache/_timestamp.py` —
|
||||
the c6-internal consolidation shim is no longer needed; the three
|
||||
c6 callers now import directly from `helpers.iso_timestamps`. The
|
||||
Layer-1 helper supersedes the Layer-2 component-local one.
|
||||
|
||||
### Tests (new)
|
||||
|
||||
- `tests/unit/test_az508_iso_timestamps.py` — 9 tests:
|
||||
- `test_ac1_import_and_call_returns_str` (AC-1)
|
||||
- `test_ac2_format_matches_canonical_regex` (AC-2 format)
|
||||
- `test_ac2_fromisoformat_roundtrip_yields_utc_aware_datetime` (AC-2 parseability)
|
||||
- `test_ac3_two_successive_calls_are_non_decreasing` (AC-3)
|
||||
- `test_ac4_no_other_iso_ts_now_definition_exists_in_src` (AC-4
|
||||
AST-walk over `src/`)
|
||||
- `test_ac4_c6_and_c7_callers_import_from_helpers` (AC-4 explicit
|
||||
call-site sweep — 5 files)
|
||||
- `test_ac6_helper_uses_stdlib_only` (AC-6, stdlib whitelist)
|
||||
- `test_helper_is_layer_1_no_component_imports` (Constraint
|
||||
§ Layer-1 discipline)
|
||||
- `test_helper_public_surface_is_minimal` (defensive: `__all__`
|
||||
is exactly `["iso_ts_now"]`)
|
||||
|
||||
### Docs (modified)
|
||||
|
||||
- `_docs/02_document/module-layout.md` — appended
|
||||
`### shared/helpers/iso_timestamps` row to the helpers section.
|
||||
- `_docs/02_tasks/todo/AZ-508_hygiene_iso_timestamps_consolidation.md` —
|
||||
drift note added to the description; AC-2 regex corrected from
|
||||
`\+00:00$` to `Z$`.
|
||||
- `_docs/03_implementation/reviews/batch_48_review.md` (new) —
|
||||
per-batch code review (PASS).
|
||||
- `_docs/_process_leftovers/2026-05-11_d_cross_cve_1_opencv_pin_deferred.md` —
|
||||
replay-attempt timestamp + reason updated (PyPI shows `gtsam==4.2.1`
|
||||
but `requires_dist: numpy<2.0.0`; block unchanged).
|
||||
|
||||
## Acceptance Criteria Coverage
|
||||
|
||||
| AC | Description | Status | Test |
|
||||
|----|-------------|--------|------|
|
||||
| AC-1 | Helper exists, importable, callable, returns `str` | PASS | `test_ac1_import_and_call_returns_str` |
|
||||
| AC-2 | Format regex match + `fromisoformat` round-trip | PASS | `test_ac2_format_matches_canonical_regex`, `test_ac2_fromisoformat_roundtrip_yields_utc_aware_datetime` |
|
||||
| AC-3 | Lexicographic monotonicity under normal clock | PASS | `test_ac3_two_successive_calls_are_non_decreasing` |
|
||||
| AC-4 | All call sites migrated; no stray definitions | PASS | `test_ac4_no_other_iso_ts_now_definition_exists_in_src`, `test_ac4_c6_and_c7_callers_import_from_helpers` |
|
||||
| AC-5 | FDR schema tests pass unmodified | PASS | `tests/unit/test_az272_fdr_record_schema.py` runs unchanged (35 tests pass) |
|
||||
| AC-6 | No new third-party dependencies | PASS | `test_ac6_helper_uses_stdlib_only` + `pyproject.toml` unchanged |
|
||||
| AC-7 | AZ-270 layering lint passes | PASS | `tests/unit/test_az270_compose_root.py::test_ac6_only_compose_root_imports_concrete_strategies` |
|
||||
|
||||
## Test Results
|
||||
|
||||
- **Focused suite** (tests/unit/test_az508_iso_timestamps.py +
|
||||
test_az272_fdr_record_schema.py + tests/unit/c6_tile_cache/ +
|
||||
tests/unit/c7_inference/): **216 passed, 53 environment-skipped, 0 failed**
|
||||
in 7.97s. Environment-gated skips (Docker compose for c6 integration,
|
||||
CUDA / TensorRT / Jetson hardware for c7) — unchanged from Batch 47.
|
||||
- **Focused per-helper**: 9/9 new AZ-508 tests PASS in 1.36s.
|
||||
- **Layering lint**: AZ-270 + AZ-507 lints PASS (no cross-component
|
||||
import violations introduced).
|
||||
- **Lint**: `ruff check` clean on every file authored or modified by
|
||||
this batch. 3 pre-existing ruff findings (B905, RUF022, RUF023) in
|
||||
`onnx_trt_ep_runtime.py` left untouched per
|
||||
`coderule.mdc` "fix pre-existing lints only if in the modified area".
|
||||
|
||||
## Architectural Decisions
|
||||
|
||||
1. **Layer-1 helper supersedes the c6 internal `_timestamp.py`**.
|
||||
The c6-internal consolidation shipped earlier (cumulative review
|
||||
28–30 F3) was a Layer-2 stop-gap pending the cross-component
|
||||
consolidation. With the Layer-1 helper now in place, the c6 shim
|
||||
is redundant and was deleted; the three c6 callers now import
|
||||
the cross-component helper directly. This collapses two
|
||||
abstractions into one.
|
||||
|
||||
2. **Preserve `_iso_ts_now` symbol at c7 call-sites via import alias**.
|
||||
The c7 modules each have 1–2 in-method `_iso_ts_now()` call-sites.
|
||||
Rather than edit every call-site, the new module-level
|
||||
`from gps_denied_onboard.helpers.iso_timestamps import iso_ts_now as _iso_ts_now`
|
||||
keeps the local symbol name unchanged → minimal diff per c7 file
|
||||
and minimal risk of accidentally missing a call-site. Matches
|
||||
the task spec's preferred form (Scope § Included bullet 2).
|
||||
|
||||
3. **Stdlib-only helper, no Clock dependency**. The helper is wall-clock
|
||||
metadata about when an FDR record envelope was emitted — explicitly
|
||||
NOT a payload field that might need an injectable Clock for tests.
|
||||
The task spec calls this out (Excluded § "Any change to flight_clock
|
||||
/ wall_clock"), and the new module docstring reiterates it so
|
||||
future contributors do not route `age_seconds`-style fields through
|
||||
it.
|
||||
|
||||
4. **AST-walk AC-4 test instead of a grep gate**. AC-4 originally
|
||||
contemplated a CI-level `grep -rn "def _iso_ts_now" src/` gate. The
|
||||
in-repo test in `test_az508_iso_timestamps.py` uses an AST walk
|
||||
instead — it catches both `def _iso_ts_now` AND
|
||||
`def iso_ts_now` defined anywhere outside the helper, with file
|
||||
paths in the assertion message, and runs as part of the standard
|
||||
unit suite (no separate CI hook needed). The same test also enforces
|
||||
the consumer-side import discipline so the regression surface is
|
||||
"the import or the test breaks", not "a stray definition lingers".
|
||||
|
||||
## Carried-over Findings (from prior batches)
|
||||
|
||||
- **F1 from cumulative review 43–45 / Batch 46 / Batch 47**: closed by
|
||||
this batch. `_iso_ts_now` is now in exactly one place.
|
||||
- **F2 from Batch 46 / Batch 47** (spec→impl drift on C7 API names for
|
||||
AZ-339 / AZ-340 / AZ-358 / AZ-349): **NOT addressed in this batch**;
|
||||
belongs to a spec-hygiene PBI to be filed before AZ-339 starts.
|
||||
- **D-CROSS-CVE-1 opencv pin leftover**: replay attempted at Batch 48
|
||||
bootstrap; PyPI shows `gtsam==4.2.1` (newer than 4.2 referenced in
|
||||
the leftover) but still pins `numpy<2.0.0`. Block unchanged;
|
||||
leftover timestamp updated.
|
||||
|
||||
## New Findings (per Batch 48 code review)
|
||||
|
||||
None.
|
||||
|
||||
## Jira Tracker
|
||||
|
||||
- AZ-508 was already `In Progress` at Batch 48 start (transitioned in
|
||||
a prior `/autodev` session per the state file). Read-back via
|
||||
`getJiraIssue` confirmed `status.name == "In Progress"` before any
|
||||
source edits. Will transition `In Progress → In Testing` after this
|
||||
batch's commit lands.
|
||||
- Task spec archived to `_docs/02_tasks/done/AZ-508_hygiene_iso_timestamps_consolidation.md`
|
||||
as part of the implement skill Step 13.
|
||||
|
||||
## Next
|
||||
|
||||
With AZ-508 closed, the carried-over F1 from batches 43–47 is resolved
|
||||
and the next C2 / C4 / C3 batches can ship without re-adding the
|
||||
helper. Top Batch 49 candidates (per Batch 47's "Next" list, updated):
|
||||
|
||||
- **AZ-339** — MegaLoc + MixVPR strategies (5pt). Same skeleton as
|
||||
UltraVPR (B47) but two strategies; F2 spec-hygiene PBI may want to
|
||||
ship first.
|
||||
- **AZ-340** — SelaVPR + EigenPlaces + SALAD strategies (5pt). Closes
|
||||
the C2 alternative-backbone buffet.
|
||||
- **AZ-358** — C4 OpenCV/GTSAM pose estimator (5pt). Changes pace from
|
||||
C2-heavy streak; opens C4.
|
||||
- **AZ-389** — C5 internal orthorectifier for mid-flight tiles (3pt).
|
||||
- **Spec-hygiene PBI** for F2 (C7 API drift in AZ-339 / AZ-340 / AZ-358 /
|
||||
AZ-349 specs) — strongly recommended before AZ-339.
|
||||
|
||||
Per autodev orchestrator: Batch 48 is the third batch since the last
|
||||
cumulative review (batches 43–45). The implement skill Step 14.5
|
||||
triggers a **cumulative review batches 46–48** before Batch 49 starts.
|
||||
@@ -0,0 +1,126 @@
|
||||
# Code Review Report — Batch 48
|
||||
|
||||
**Batch**: 48 (Cycle 1)
|
||||
**Tasks**: AZ-508 — ISO-timestamp helper consolidation (2 pt)
|
||||
**Date**: 2026-05-13
|
||||
**Verdict**: **PASS**
|
||||
|
||||
## Findings
|
||||
|
||||
_No findings._
|
||||
|
||||
## Phase Summary
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
|
||||
- Task spec: `_docs/02_tasks/todo/AZ-508_hygiene_iso_timestamps_consolidation.md`
|
||||
- Cumulative review F2 (batches 31–33) + F3 (batches 28–30) — origin of the
|
||||
hygiene PBI.
|
||||
- Changed source files: 1 new helper + 1 helper package re-export +
|
||||
3 c6 import rewires + 2 c7 local-def → import rewires + 1 deleted
|
||||
c6 internal shim.
|
||||
- Changed test files: 1 new (`tests/unit/test_az508_iso_timestamps.py`,
|
||||
9 tests covering AC-1..AC-7 + Layer-1 invariant).
|
||||
- Changed docs: `module-layout.md` (new `shared/helpers/iso_timestamps`
|
||||
entry) + task spec (drift note + corrected AC-2 regex).
|
||||
|
||||
### Phase 2 — Spec Compliance (AC-by-AC)
|
||||
|
||||
| AC | Verdict | Evidence |
|
||||
|----|---------|----------|
|
||||
| AC-1 | PASS | `test_ac1_import_and_call_returns_str` |
|
||||
| AC-2 | PASS | `test_ac2_format_matches_canonical_regex` + `test_ac2_fromisoformat_roundtrip_yields_utc_aware_datetime` |
|
||||
| AC-3 | PASS | `test_ac3_two_successive_calls_are_non_decreasing` |
|
||||
| AC-4 | PASS | `test_ac4_no_other_iso_ts_now_definition_exists_in_src` (AST walk over `src/`) + `test_ac4_c6_and_c7_callers_import_from_helpers` |
|
||||
| AC-5 | PASS | `tests/unit/test_az272_fdr_record_schema.py` runs unmodified, 100% pass |
|
||||
| AC-6 | PASS | `test_ac6_helper_uses_stdlib_only` (AST whitelist) — `pyproject.toml` unchanged |
|
||||
| AC-7 | PASS | `tests/unit/test_az270_compose_root.py::test_ac6_only_compose_root_imports_concrete_strategies` passes |
|
||||
|
||||
**Spec drift note**: the task spec originally listed 5 call-sites and a
|
||||
`+00:00` regex. On-disk reality at implementation time: 3 call-sites
|
||||
(c6 already locally consolidated under `_timestamp.py`; `tensorrt_runtime.py`
|
||||
had no local copy) and the canonical FDR `ts` format is `Z`-suffix per
|
||||
the `_TS = "2026-05-11T00:00:00.000000Z"` fixture. Spec + AC-2 regex
|
||||
updated in this batch; no behavioral drift from the spec's actual intent
|
||||
("single Layer-1 helper, byte-identical to existing locals, no FDR
|
||||
schema change"). User explicitly approved the path via Choose A.
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
|
||||
- **SRP**: pure stateless free function; no class wrapper (per Constraint
|
||||
§ "no class wrapping").
|
||||
- **Error handling**: no exception suppression; stdlib `datetime.now`
|
||||
cannot raise under normal operation.
|
||||
- **Naming**: `iso_ts_now` matches the canonical name used by all three
|
||||
pre-existing local helpers — no naming churn in call sites
|
||||
(`from … import iso_ts_now as _iso_ts_now` alias preserved in the c7
|
||||
files).
|
||||
- **Complexity**: 1 line. n/a.
|
||||
- **DRY**: net deletion — eliminates duplication (the explicit goal).
|
||||
- **Test quality**: assertions cover format, parseability, monotonicity,
|
||||
absence of stray definitions, import-path enforcement, layer-1
|
||||
invariant, and stdlib-only constraint.
|
||||
- **Dead code**: removed unused `from datetime import datetime, timezone`
|
||||
imports from both c7 files after the local `_iso_ts_now` defs were
|
||||
removed (adjacent-hygiene, permitted by `coderule.mdc`).
|
||||
|
||||
### Phase 4 — Security Quick-Scan
|
||||
|
||||
No SQL, no `subprocess`, no `eval` / `exec`, no secrets, no untrusted
|
||||
input handling. Helper is stdlib-only and side-effect-free. n/a.
|
||||
|
||||
### Phase 5 — Performance Scan
|
||||
|
||||
`datetime.now(timezone.utc).strftime(...)` is the same call all three
|
||||
previous local helpers made; zero runtime delta. No new allocations on
|
||||
the hot path.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
|
||||
Single task in batch — no inter-task interface concerns. Helper aligns
|
||||
with the existing 8-helper convention (single-file Layer-1 modules under
|
||||
`src/gps_denied_onboard/helpers/`, re-exported from `helpers/__init__.py`).
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
|
||||
- **Layer direction**: helper sits at Layer 1 (Foundation / shared);
|
||||
consumers in c6 (Layer 2 Infrastructure) and c7 (Layer 2
|
||||
Infrastructure) import strictly downward. ✓
|
||||
- **Public API respect**: consumers import via
|
||||
`gps_denied_onboard.helpers.iso_timestamps` (or the `helpers` package
|
||||
facade after the `__init__.py` re-export). Internal modules of c6/c7
|
||||
are not imported across components. ✓
|
||||
- **No new cyclic deps**: the helper imports only stdlib; no possible
|
||||
cycle. ✓
|
||||
- **Duplicate symbols**: eliminated by design — the AC-4 AST walk
|
||||
asserts zero other `iso_ts_now` / `_iso_ts_now` definitions remain
|
||||
anywhere under `src/`. ✓
|
||||
- **Cross-cutting concern home**: moved from per-component locals to
|
||||
the canonical `helpers/` directory. ✓
|
||||
- **AZ-507 layering lint**: `test_ac6_only_compose_root_imports_concrete_strategies`
|
||||
passes — helper imports are allowed from components. ✓
|
||||
- **AZ-270 lint**: passes. ✓
|
||||
|
||||
## Pre-existing Findings (NOT introduced by this batch)
|
||||
|
||||
The following ruff findings remain in
|
||||
`src/gps_denied_onboard/components/c7_inference/onnx_trt_ep_runtime.py`
|
||||
but are pre-existing (verified by re-running ruff against `HEAD` before
|
||||
this batch's edits):
|
||||
|
||||
- **B905** at line 547 — `zip()` without `strict=`.
|
||||
- **RUF022** at lines 88–98 — `__all__` not sorted.
|
||||
- **RUF023** at lines 127–134 — `__slots__` not sorted.
|
||||
|
||||
Per `coderule.mdc` "Pre-existing lint errors should only be fixed if
|
||||
they're in the modified area" — left untouched. Candidate for a future
|
||||
c7 hygiene PBI.
|
||||
|
||||
## Verdict Justification
|
||||
|
||||
- Critical findings: 0
|
||||
- High findings: 0
|
||||
- Medium findings: 0
|
||||
- Low findings: 0
|
||||
|
||||
**PASS** — proceed to commit per implement skill Step 11.
|
||||
Reference in New Issue
Block a user