mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-21 22:51:14 +00:00
[AZ-526] Consolidate _iso_ts_from_clock into helpers/iso_timestamps
Closes cumulative review 46-48 F1 (Medium) + F3 (Low). Adds iso_ts_from_clock(clock) alongside iso_ts_now() in the Layer-1 helper; migrates four duplicate definitions in c2_vpr (net_vlad, ultra_vpr, _faiss_bridge) and c12_operator_orchestrator (operator_reloc_service). Output format flipped +00:00 -> Z to align with iso_ts_now() and the canonical FDR _TS fixture (FDR schema test passes unmodified). 18 helper AC tests + 186 sibling tests pass; ruff clean. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -363,9 +363,9 @@ Bootstrap reference: `_docs/02_tasks/todo/AZ-263_initial_structure.md`. Architec
|
||||
### shared/helpers/iso_timestamps
|
||||
|
||||
- **File**: `src/gps_denied_onboard/helpers/iso_timestamps.py`
|
||||
- **Purpose**: Single Layer-1 UTC ISO-8601 timestamp source for FDR record envelopes (`iso_ts_now() -> str`, format `YYYY-MM-DDTHH:MM:SS.ffffffZ` matching the canonical FDR `_TS` fixture). Replaces the duplicated private `_iso_ts_now` one-liners that previously lived inside `c6_tile_cache` (3 modules — already locally consolidated under `_timestamp.py`) and `c7_inference` (2 modules). Stateless free function; stdlib only.
|
||||
- **Owned by**: AZ-264 (AZ-508 consolidation task).
|
||||
- **Consumed by**: c6_tile_cache (`cache_budget_enforcer`, `postgres_filesystem_store`, `freshness_gate`), c7_inference (`onnx_trt_ep_runtime`, `thermal_publisher`); future C10/C11 FDR producers should import this helper rather than redefining the one-liner locally.
|
||||
- **Purpose**: Single Layer-1 UTC ISO-8601 timestamp source for FDR record envelopes. Two surfaces — `iso_ts_now() -> str` (wall-clock, microsecond precision, format `YYYY-MM-DDTHH:MM:SS.ffffffZ`) and `iso_ts_from_clock(clock: Clock) -> str` (Clock-injected, nanosecond precision, format `YYYY-MM-DDTHH:MM:SS.fffffffffZ`). Both terminate with the canonical `Z` suffix matching the FDR `_TS` fixture. Replaces the duplicated private `_iso_ts_now` (AZ-508) and `_iso_ts_from_clock` (AZ-526) one-liners that previously lived inside `c6_tile_cache`, `c7_inference`, `c2_vpr`, and `c12_operator_orchestrator`. Stateless functions; stdlib + Layer-1 `gps_denied_onboard.clock.Clock` only.
|
||||
- **Owned by**: AZ-264 (AZ-508 + AZ-526 consolidation tasks).
|
||||
- **Consumed by**: c6_tile_cache (`cache_budget_enforcer`, `postgres_filesystem_store`, `freshness_gate`) + c7_inference (`onnx_trt_ep_runtime`, `thermal_publisher`) via `iso_ts_now`; c2_vpr (`net_vlad`, `ultra_vpr`, `_faiss_bridge`) + c12_operator_orchestrator (`operator_reloc_service`) via `iso_ts_from_clock`. Future C3 / C4 / C5 FDR producers should import the appropriate helper rather than redefining the one-liner locally.
|
||||
|
||||
### shared/frame_source
|
||||
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
# Dependencies Table
|
||||
|
||||
**Date**: 2026-05-13 (refreshed after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path)
|
||||
**Total Tasks**: 146 (105 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks
|
||||
**Total Complexity Points**: 487 (354 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt
|
||||
**Date**: 2026-05-13 (refreshed after Batch 48: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; earlier same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path)
|
||||
**Total Tasks**: 147 (106 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks; AZ-526 = 2pt clock-helper hygiene
|
||||
**Total Complexity Points**: 489 (356 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt, AZ-526 = 2pt
|
||||
|
||||
Dependencies columns list only the tracker-ID portion (descriptive tail
|
||||
text in each task spec is omitted here for table-readability). The
|
||||
@@ -158,6 +158,7 @@ are all declared and documented below under **Cycle Check**.
|
||||
| AZ-490 | C5 set_takeoff_origin entrypoint — accept operator origin from C10 Manifest | 3 | AZ-263, AZ-269, AZ-266, AZ-272, AZ-273, AZ-279, AZ-381, AZ-383, AZ-384, AZ-385, AZ-386 | AZ-260 |
|
||||
| AZ-507 | Hygiene — align module-layout.md cross-component import rules with AZ-270 lint | 2 | AZ-263, AZ-270, AZ-321 | AZ-246 |
|
||||
| AZ-508 | Hygiene — consolidate `_iso_ts_now` helpers into `helpers/iso_timestamps.py` | 2 | AZ-263 | AZ-264 |
|
||||
| AZ-526 | Hygiene — add `iso_ts_from_clock(clock)` to `helpers/iso_timestamps.py` | 2 | AZ-508, AZ-398 | AZ-264 |
|
||||
| AZ-523 | Batch 44 — C11 internal flight-state gate removal (SRP refactor; audit-trail; closed) | 3 | AZ-317, AZ-319, AZ-329 | AZ-251 |
|
||||
| AZ-524 | Batch 44 — C12 package rename: c12_operator_tooling → c12_operator_orchestrator (audit; closed)| 2 | AZ-263, AZ-326, AZ-327, AZ-328, AZ-329, AZ-330, AZ-489 | AZ-253 |
|
||||
|
||||
|
||||
@@ -0,0 +1,137 @@
|
||||
# Hygiene — Consolidate `_iso_ts_from_clock` into `helpers/iso_timestamps.py`
|
||||
|
||||
**Task**: AZ-526_hygiene_iso_ts_from_clock_consolidation
|
||||
**Name**: Clock-injected ISO-timestamp helper consolidation
|
||||
**Description**: Replace the four duplicated `_iso_ts_from_clock(clock)` definitions (two module-level free functions in `c2_vpr/{net_vlad,ultra_vpr}.py`, two instance methods in `c2_vpr/_faiss_bridge.py` and `c12_operator_orchestrator/operator_reloc_service.py`) with a single Layer-1 helper alongside `iso_ts_now()` in `src/gps_denied_onboard/helpers/iso_timestamps.py`. Closes cumulative review batches 46–48 Findings F1 (Medium / Maintainability) and F3 (Low / Maintainability).
|
||||
**Complexity**: 2 points
|
||||
**Dependencies**: AZ-508 (the `helpers/iso_timestamps.py` module must exist first), AZ-398 (Clock Protocol must exist — already shipped)
|
||||
**Component**: helpers (epic AZ-264 / E-CC-HELPERS)
|
||||
**Tracker**: AZ-526
|
||||
**Epic**: AZ-264 (E-CC-HELPERS)
|
||||
|
||||
### Document Dependencies
|
||||
|
||||
- `_docs/03_implementation/cumulative_review_batches_46-48_cycle1_report.md` § F1 + F3 — the findings being closed.
|
||||
- `_docs/02_document/module-layout.md` § `shared/helpers/iso_timestamps` — the helpers row that the new function will extend.
|
||||
- `_docs/02_tasks/done/AZ-508_hygiene_iso_timestamps_consolidation.md` — the precedent task (Batch 48) that shipped `iso_ts_now()` and whose scope explicitly Excluded the Clock-injected variant.
|
||||
|
||||
## Problem
|
||||
|
||||
Four modules across two components define the same Clock-injected ISO-timestamp helper:
|
||||
|
||||
| File | Surface | Source format |
|
||||
|------|---------|---------------|
|
||||
| `src/gps_denied_onboard/components/c2_vpr/net_vlad.py` | module-level free function | `+00:00` suffix, 9-digit nanos |
|
||||
| `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py` | module-level free function | `+00:00` suffix, 9-digit nanos |
|
||||
| `src/gps_denied_onboard/components/c2_vpr/_faiss_bridge.py` | instance method on `FaissBridge` | `+00:00` suffix, 9-digit nanos |
|
||||
| `src/gps_denied_onboard/components/c12_operator_orchestrator/operator_reloc_service.py` | instance method on `OperatorReLocService` | `+00:00` suffix, 9-digit nanos |
|
||||
|
||||
The bodies are byte-identical modulo `clock` parameter vs `self._clock`. Both `net_vlad.py` and `ultra_vpr.py` carry an inline comment saying "AZ-508 will consolidate the duplicate helpers across c2/c11/c12/c6" — but AZ-508 explicitly Excluded this variant. `_faiss_bridge.py` carries a meta-rule citation rejecting consolidation; that rejection is invalidated now that AZ-508 has established the helpers/iso_timestamps.py home.
|
||||
|
||||
AZ-339 (MegaLoc + MixVPR), AZ-340 (SelaVPR + EigenPlaces + SALAD), AZ-358 (C4 OpenCVGtsam pose), and AZ-389 (C5 orthorectifier) will each add 1–3 more copies unless consolidated first.
|
||||
|
||||
## Outcome
|
||||
|
||||
- The existing helper module `src/gps_denied_onboard/helpers/iso_timestamps.py` gains a second public function: `iso_ts_from_clock(clock: Clock) -> str`. Format `YYYY-MM-DDTHH:MM:SS.fffffffffZ` (9-digit nanosecond precision, `Z` suffix matching `iso_ts_now()` and the canonical FDR `_TS` fixture).
|
||||
- The four duplicate definitions are deleted; consumers import from the helper. Module-level callers use `from … import iso_ts_from_clock as _iso_ts_from_clock`; the two instance methods are rewritten to one-line delegations (`return iso_ts_from_clock(self._clock)`).
|
||||
- `_docs/02_document/module-layout.md` `shared/helpers/iso_timestamps` row is updated to mention the second function.
|
||||
- A new AC test set is added to `tests/unit/test_az508_iso_timestamps.py` covering AC-1..AC-5 of this task.
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
|
||||
- Add to `src/gps_denied_onboard/helpers/iso_timestamps.py`:
|
||||
|
||||
```python
|
||||
from gps_denied_onboard.clock import Clock
|
||||
|
||||
def iso_ts_from_clock(clock: Clock) -> str:
|
||||
"""Return an RFC 3339 UTC timestamp built from `clock.time_ns()`.
|
||||
|
||||
Format: ``YYYY-MM-DDTHH:MM:SS.fffffffffZ`` (9-digit nanosecond
|
||||
precision, ``Z`` suffix). Use when the timestamp must follow an
|
||||
injectable Clock (replay tests, deterministic fixtures);
|
||||
use ``iso_ts_now()`` otherwise.
|
||||
"""
|
||||
ns = int(clock.time_ns())
|
||||
seconds, fraction_ns = divmod(ns, 1_000_000_000)
|
||||
dt = datetime.fromtimestamp(seconds, tz=timezone.utc)
|
||||
return f"{dt.strftime('%Y-%m-%dT%H:%M:%S')}.{fraction_ns:09d}Z"
|
||||
```
|
||||
|
||||
- Re-export `iso_ts_from_clock` through `src/gps_denied_onboard/helpers/__init__.py` and add it to `__all__`.
|
||||
- Migrate the four call sites:
|
||||
- `c2_vpr/net_vlad.py`: delete the free function; add `from gps_denied_onboard.helpers.iso_timestamps import iso_ts_from_clock as _iso_ts_from_clock` at the top.
|
||||
- `c2_vpr/ultra_vpr.py`: same.
|
||||
- `c2_vpr/_faiss_bridge.py`: rewrite `_iso_ts_from_clock(self)` to a one-line delegation `return iso_ts_from_clock(self._clock)`; remove the stale meta-rule comment block. Add the helper import at the top.
|
||||
- `c12_operator_orchestrator/operator_reloc_service.py`: same pattern as `_faiss_bridge.py`.
|
||||
- Extend `tests/unit/test_az508_iso_timestamps.py` with new AC tests (AC-1..AC-5 below). Reuse the existing test file rather than creating a new one; the helpers/iso_timestamps.py module surface is jointly tested.
|
||||
- Update `_docs/02_document/module-layout.md` `shared/helpers/iso_timestamps` row to describe both functions.
|
||||
|
||||
### Excluded
|
||||
|
||||
- Changing `Clock` Protocol or `WallClock` / `TlogDerivedClock` implementations.
|
||||
- Migrating any other timestamp emitters that use `time.time_ns()` directly without going through `Clock`.
|
||||
- Bumping FDR schema versions (AC-5 requires `test_az272_fdr_record_schema.py` to pass unmodified).
|
||||
- Migrating timestamp callers in components that don't currently have a `_iso_ts_from_clock` helper.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: Helper exists at the canonical path**
|
||||
Given a fresh checkout
|
||||
When `from gps_denied_onboard.helpers.iso_timestamps import iso_ts_from_clock` is run
|
||||
Then the import succeeds; the function is callable with a `Clock`-compatible argument
|
||||
|
||||
**AC-2: Output format matches the canonical Z-suffix nanosecond regex**
|
||||
Given any `Clock` instance returning `time_ns()` value `N`
|
||||
When `iso_ts_from_clock(clock)` is called
|
||||
Then the output matches `^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{9}Z$` and is parseable into a UTC-aware datetime via `datetime.fromisoformat(value[:26] + "+00:00")` (note: stdlib `fromisoformat` doesn't accept 9-digit fractional seconds; tests truncate to 6 digits + offset for round-trip verification)
|
||||
|
||||
**AC-3: Nanosecond accuracy**
|
||||
Given a stub `Clock` returning `time_ns() == 1_234_567_890_123_456_789`
|
||||
When `iso_ts_from_clock(clock)` is called
|
||||
Then the output ends in `.123456789Z` (the nanosecond fraction is preserved verbatim)
|
||||
|
||||
**AC-4: All four call sites migrated**
|
||||
Given a `grep -rn "def _iso_ts_from_clock\|def iso_ts_from_clock" src/` after the task lands
|
||||
When the search runs
|
||||
Then matches appear only inside `src/gps_denied_onboard/helpers/iso_timestamps.py`; zero matches in `c2_vpr/`, `c12_operator_orchestrator/`, or any other component
|
||||
|
||||
**AC-5: FDR schema tests pass unmodified**
|
||||
Given the existing `tests/unit/test_az272_fdr_record_schema.py` suite
|
||||
When the suite runs after this task
|
||||
Then every previously-passing test still passes; no test file is modified
|
||||
|
||||
**AC-6: AZ-270 + AZ-507 lints pass**
|
||||
Given the helper lives at Layer 1 and only imports from `_types` / `clock` / stdlib
|
||||
When `tests/unit/test_az270_compose_root.py::test_ac6` runs
|
||||
Then the test passes (the lint walks `src/gps_denied_onboard/components/`; helper imports are allowed)
|
||||
|
||||
## Constraints
|
||||
|
||||
- The helper imports ONLY from stdlib (`datetime`) and `gps_denied_onboard.clock` (Layer 1). NO component imports.
|
||||
- Output format `Z`-suffix nanosecond precision (`%Y-%m-%dT%H:%M:%S.{fraction_ns:09d}Z`). This intentionally differs from the existing `+00:00`-suffix output to align with `iso_ts_now()` and the canonical FDR `_TS` fixture (resolves cumulative-46-48 F3).
|
||||
- The two instance-method call sites become one-line delegations; no behavior change at the call sites.
|
||||
- Test additions go into the existing `tests/unit/test_az508_iso_timestamps.py`; do NOT create a separate test file (the helpers/iso_timestamps.py module surface is one cohesive Layer-1 concern).
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: Switching `+00:00` → `Z` changes FDR `ts` bit-shape for c2_vpr / c12 records**
|
||||
- *Risk*: Downstream consumers parsing the FDR may have hardcoded `+00:00` expectations.
|
||||
- *Mitigation*: The FDR `ts` field is typed `str` with no schema-level format validation (see `src/gps_denied_onboard/fdr_client/records.py` line 378). The canonical `_TS` test fixture already uses `Z`. AC-5 + the unchanged `test_az272_fdr_record_schema.py` is the safety net; if it fails, the consolidation must align to the existing FDR `_TS` fixture (= keep `Z`), confirming the choice.
|
||||
|
||||
**Risk 2: A future emitter forgets the helper exists and adds a 5th local copy**
|
||||
- *Risk*: The pattern recurs once we add C3/C4 strategies in AZ-345..AZ-358.
|
||||
- *Mitigation*: AZ-508's AST-walk test already asserts zero stray `iso_ts_now` definitions outside the helper module; extend it to also assert zero stray `iso_ts_from_clock` definitions. The combined assertion is the regression guard.
|
||||
|
||||
**Risk 3: Layer-1 helper depending on `Clock` introduces a cross-layer cycle**
|
||||
- *Risk*: `helpers/*` imports from `clock/*`.
|
||||
- *Mitigation*: Both `helpers/*` and `clock/*` are Layer 1 per `module-layout.md` Allowed Dependencies table. No cycle: `clock/interface.py` only imports `typing.Protocol`; the helper imports `clock.Clock` (interface only); no reverse path.
|
||||
|
||||
## Runtime Completeness
|
||||
|
||||
- **Named capability**: a Layer-1 helper `iso_ts_from_clock(clock: Clock) -> str` producing nanosecond-precision UTC ISO-8601 with `Z` suffix.
|
||||
- **Production code that must exist**: real `iso_ts_from_clock` function alongside `iso_ts_now` in the helper module; real import + delegation in each of the four flagged modules.
|
||||
- **Allowed external stubs**: none. Pure consolidation.
|
||||
- **Unacceptable substitutes**: keeping one or more local definitions "for parity"; using `+00:00` suffix in the new helper (defeats the F3 alignment); changing the nanosecond precision (FDR records assume 9-digit subseconds).
|
||||
@@ -0,0 +1,61 @@
|
||||
# Batch 49 — Implementation Report (Cycle 1)
|
||||
|
||||
**Tasks**: AZ-526 (Hygiene — consolidate `_iso_ts_from_clock` into `helpers/iso_timestamps.py`)
|
||||
**Date**: 2026-05-13
|
||||
**Cycle**: 1
|
||||
**Status**: COMPLETE (review verdict: PASS_WITH_WARNINGS, one Low / Scope drift finding)
|
||||
|
||||
## What was done
|
||||
|
||||
Added a second public function `iso_ts_from_clock(clock: Clock) -> str` to the Layer-1 helper module that AZ-508 (Batch 48) introduced. Migrated the four duplicate `_iso_ts_from_clock` definitions to import from the helper, closing cumulative review 46-48 Findings F1 (Medium / Maintainability) and F3 (Low / Maintainability).
|
||||
|
||||
### Files added
|
||||
- _None._ Reused `src/gps_denied_onboard/helpers/iso_timestamps.py` and `tests/unit/test_az508_iso_timestamps.py`.
|
||||
|
||||
### Files changed (8)
|
||||
|
||||
| File | Change |
|
||||
|------|--------|
|
||||
| `src/gps_denied_onboard/helpers/iso_timestamps.py` | Added `iso_ts_from_clock(clock)`; extended module docstring; added `TYPE_CHECKING`-guarded `Clock` import. |
|
||||
| `src/gps_denied_onboard/helpers/__init__.py` | Re-exported `iso_ts_from_clock`; added to `__all__`. |
|
||||
| `src/gps_denied_onboard/components/c2_vpr/net_vlad.py` | Deleted local module-level `def _iso_ts_from_clock`; added `from gps_denied_onboard.helpers.iso_timestamps import iso_ts_from_clock as _iso_ts_from_clock`. |
|
||||
| `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py` | Same as `net_vlad.py`. |
|
||||
| `src/gps_denied_onboard/components/c2_vpr/_faiss_bridge.py` | Rewrote `FaissBridge._iso_ts_from_clock` body to `return iso_ts_from_clock(self._clock)`; deleted stale meta-rule comment; added helper import. |
|
||||
| `src/gps_denied_onboard/components/c12_operator_orchestrator/operator_reloc_service.py` | Same as `_faiss_bridge.py`; also removed now-unused `from datetime import datetime, timezone`. |
|
||||
| `tests/unit/test_az508_iso_timestamps.py` | Added 7 AC tests (AZ-526 AC-1..AC-4); extended `_helper_uses_stdlib_only` to allow `gps_denied_onboard.clock` (Layer-1) and `typing`; updated `_public_surface_is_minimal` to expect `["iso_ts_from_clock", "iso_ts_now"]`; renamed AC-6 test for clarity; added AST scan for stray module-level `iso_ts_from_clock` defs (distinct from the AZ-508 `iso_ts_now` scan). |
|
||||
| `_docs/02_document/module-layout.md` | Updated `shared/helpers/iso_timestamps` entry to describe both functions and list the c2_vpr / c12_operator_orchestrator consumers. |
|
||||
| `_docs/02_tasks/_dependencies_table.md` | Added AZ-526 row (2pt, deps AZ-508 + AZ-398); updated date header and totals. |
|
||||
|
||||
## AC coverage
|
||||
|
||||
| AC | Verified by | Status |
|
||||
|----|-------------|--------|
|
||||
| AC-1 (import + callable) | `test_az526_ac1_import_and_call_returns_str` | PASS |
|
||||
| AC-2 (Z-suffix nanosecond regex) | `test_az526_ac2_format_matches_canonical_ns_regex`, `test_az526_ac2_fromisoformat_truncated_roundtrip_yields_utc` | PASS |
|
||||
| AC-3 (nanosecond accuracy) | `test_az526_ac3_nanosecond_fraction_preserved_verbatim`, `test_az526_ac3_sub_second_distinguishability` | PASS |
|
||||
| AC-4 (call-site migration) | `test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper`, `test_az526_ac4_all_four_call_sites_import_from_helper`, `test_az526_ac4_no_module_level_local_iso_ts_from_clock_body_in_callers` | PASS (with Spec-Drift caveat — see below) |
|
||||
| AC-5 (FDR schema tests unmodified) | `tests/unit/test_az272_fdr_record_schema.py` re-run unmodified (33 tests pass) | PASS |
|
||||
| AC-6 (AZ-270 lint passes) | `tests/unit/test_az270_compose_root.py` (8 tests pass) | PASS |
|
||||
|
||||
## Test results
|
||||
|
||||
- `tests/unit/test_az508_iso_timestamps.py` — **18 / 18 PASS** (8 pre-existing + 10 added/updated for AZ-526).
|
||||
- `tests/unit/test_az272_fdr_record_schema.py` — **33 / 33 PASS** unmodified.
|
||||
- `tests/unit/test_az270_compose_root.py` — **8 / 8 PASS**.
|
||||
- Component sibling tests for the 4 migrated files (`c2_vpr/`, `c12_operator_orchestrator/`) plus adjacent suites — **186 / 186 PASS** (`pytest -k "az338 or az337 or az489 or az490 or faiss or operator_reloc or net_vlad or ultra_vpr or _faiss"`).
|
||||
- `ruff check` on all 7 changed source/test files — clean.
|
||||
|
||||
## Architectural decisions
|
||||
|
||||
1. **`Clock` import via `TYPE_CHECKING`** — `iso_ts_from_clock` only needs `Clock` for its parameter annotation. Importing it under `TYPE_CHECKING` keeps the runtime dependency surface stdlib-only, simplifies the AC-6 lint, and avoids any risk of importing component code at module load.
|
||||
2. **Output format flip `+00:00` → `Z`** — closes cumulative-46-48 F3. The FDR `ts` field is typed `str` with no schema-level format validation, and the canonical `_TS` fixture in `test_az272_fdr_record_schema.py` already uses `Z`. AC-5 is the safety net; verified by re-running the unmodified schema test suite.
|
||||
3. **Module-level callers preserved their local symbol via `as _iso_ts_from_clock`** — call-site shape unchanged, so the existing `_emit_fdr` / `_publish` builders read identically. Instance-method callers (FaissBridge, OperatorReLocService) became one-line delegations rather than imports because the call-site is `self._iso_ts_from_clock()` — keeping the method preserves the class's encapsulated public-ish surface in tests that mock it.
|
||||
4. **Single test file (`test_az508_iso_timestamps.py`)** — the helper module is one cohesive Layer-1 concern; a separate `test_az526_*.py` file would have fragmented the AC walk across two tracker IDs without semantic benefit. The AZ-526 tests are sectioned under a `# AZ-526` divider.
|
||||
|
||||
## Spec drift noted (carried into review F1)
|
||||
|
||||
AC-4 of the AZ-526 task spec literally reads "zero matches in `c2_vpr/`, `c12_operator_orchestrator/`", but the same spec's § Scope and § Constraints both mandate that the two instance methods "become one-line delegations" — meaning `def _iso_ts_from_clock(self) -> str` remains inside `FaissBridge` and `OperatorReLocService`. Implementation followed Scope/Constraints (delegations preserved). The AST test `test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper` enforces the **spirit** of AC-4 by scanning only top-level `FunctionDef` nodes; nested class methods are not flagged. Logged as Low / Scope finding F1 in `batch_49_review.md`; no code change.
|
||||
|
||||
## Cumulative review obligation
|
||||
|
||||
This batch closes cumulative-46-48 F1 and F3. F2 (`_assert_engine_output_dim` near-duplication between `net_vlad.py` and `ultra_vpr.py`) is unchanged — leaving for a future hygiene PBI when a third strategy joins (AZ-339 batch). No new cumulative review window required after this batch (next one is at K=3 → batches 49-51).
|
||||
@@ -0,0 +1,62 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 49 — AZ-526 (Hygiene — consolidate `_iso_ts_from_clock`)
|
||||
**Date**: 2026-05-13
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Low | Scope | `_docs/02_tasks/todo/AZ-526_hygiene_iso_ts_from_clock_consolidation.md` § AC-4 | AC-4 wording contradicts § Scope (instance-method delegations) |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1: AC-4 wording contradicts § Scope (instance-method delegations)** (Low / Scope)
|
||||
|
||||
- Location: `_docs/02_tasks/todo/AZ-526_hygiene_iso_ts_from_clock_consolidation.md`, AC-4 vs § Scope / § Constraints.
|
||||
- Description: AC-4 says `grep "def _iso_ts_from_clock|def iso_ts_from_clock" src/` must yield "matches only inside `helpers/iso_timestamps.py`; zero matches in `c2_vpr/`, `c12_operator_orchestrator/`". But § Scope and § Constraints both explicitly mandate that `FaissBridge._iso_ts_from_clock(self)` and `OperatorReLocService._iso_ts_from_clock(self)` "become one-line delegations" — meaning two `def _iso_ts_from_clock` lines remain inside class bodies. Strict reading of AC-4 fails; reading aligned with Scope/Constraints passes.
|
||||
- Implementation followed Scope/Constraints (delegations preserved). The AST-based test `test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper` enforces the spirit of AC-4 by scanning only top-level `FunctionDef` nodes (so nested ClassDef methods are not flagged).
|
||||
- Suggestion: amend AC-4 to read "Then matches at module level appear only inside `helpers/iso_timestamps.py`; zero free-function matches in `c2_vpr/`, `c12_operator_orchestrator/`. Instance-method one-line delegations are permitted per § Scope." Recorded as drift; no code change needed.
|
||||
- Task: AZ-526.
|
||||
|
||||
## Phase Summary
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
Read task spec AZ-526, cumulative review 46-48 (F1, F3), `_docs/02_document/module-layout.md` § `shared/helpers/iso_timestamps`, and AZ-508 precedent. Mapped 8 changed files (2 helper, 4 component, 1 test, 2 doc) to AZ-526.
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
- AC-1: `iso_ts_from_clock` importable from helper. **PASS** (`test_az526_ac1_import_and_call_returns_str`).
|
||||
- AC-2: Format matches `^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{9}Z$`. **PASS** (`test_az526_ac2_format_matches_canonical_ns_regex`, `test_az526_ac2_fromisoformat_truncated_roundtrip_yields_utc`).
|
||||
- AC-3: Nanosecond fraction preserved verbatim for `ns=1_234_567_890_123_456_789`. **PASS** (`test_az526_ac3_nanosecond_fraction_preserved_verbatim`, `test_az526_ac3_sub_second_distinguishability`).
|
||||
- AC-4: All four call sites import from helper; no module-level stray defs. **PASS** with F1 drift (see above). Tests `test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper`, `test_az526_ac4_all_four_call_sites_import_from_helper`, `test_az526_ac4_no_module_level_local_iso_ts_from_clock_body_in_callers`.
|
||||
- AC-5: `tests/unit/test_az272_fdr_record_schema.py` runs unmodified. **PASS** (33 tests, 0 modifications).
|
||||
- AC-6: `tests/unit/test_az270_compose_root.py::test_ac6` passes. **PASS** (8 tests, all green).
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
- `iso_ts_from_clock` body is 4 lines (excluding docstring). SRP satisfied.
|
||||
- Each migrated instance method is now a single `return iso_ts_from_clock(self._clock)` — readable and intent-preserving.
|
||||
- Stale meta-rule comment in `_faiss_bridge.py` deleted (was: "Inlined here rather than importing a shared helper because every other component … defines the same one-liner locally" — that justification is no longer true).
|
||||
- The two module-level callers (`net_vlad.py`, `ultra_vpr.py`) import `iso_ts_from_clock as _iso_ts_from_clock`, preserving call-site shape with no behavioural change.
|
||||
- No dead code introduced; the `datetime`, `timezone` imports unused after the body deletion were removed from `operator_reloc_service.py`.
|
||||
- Tests follow AAA convention with explicit Arrange / Act / Assert markers.
|
||||
|
||||
### Phase 4 — Security Quick-Scan
|
||||
No SQL, no shell, no eval / exec, no dynamic deserialisation, no user-controlled paths. The helper is a pure datetime formatter over an injected protocol method. Out of scope.
|
||||
|
||||
### Phase 5 — Performance Scan
|
||||
The helper is O(1) — two arithmetic operations (`divmod`, `int()`) plus a `strftime` and an f-string. Functionally equivalent to the inlined version it replaces; one extra Python function-call frame per call site, dominated by the I/O cost of `FdrClient.enqueue` (the surrounding caller). Not in any hot path.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
Single-task batch. N/A.
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
- **Layer direction**: `helpers/iso_timestamps.py` imports `gps_denied_onboard.clock.Clock` under `TYPE_CHECKING`. Both modules are Layer-1 per `module-layout.md` Allowed Dependencies table. The runtime `from datetime import datetime, timezone` is stdlib. No upward import. **PASS.**
|
||||
- **Public API respect**: the four callers import from `gps_denied_onboard.helpers.iso_timestamps`, which is the Public API surface declared in `module-layout.md` § `shared/helpers/iso_timestamps`. **PASS.**
|
||||
- **No new cyclic dependencies**: `helpers → clock`; `clock/interface.py` only imports `typing`. No reverse path possible. **PASS.**
|
||||
- **Duplicate symbols**: net reduction (4 module-level / instance-method copies of `_iso_ts_from_clock` → 1 canonical + 2 thin delegations). **PASS.**
|
||||
- **Cross-cutting concerns not locally re-implemented**: this IS the consolidation; the cumulative-46-48 F1 + F3 findings are closed by this batch. **PASS.**
|
||||
|
||||
## Verdict Rationale
|
||||
|
||||
One Low / Scope finding (AC wording drift relative to the spec's own Scope/Constraints) — does not block. No Critical, High, or Medium findings. Verdict: **PASS_WITH_WARNINGS**.
|
||||
@@ -12,5 +12,5 @@ sub_step:
|
||||
retry_count: 0
|
||||
cycle: 1
|
||||
tracker: jira
|
||||
last_completed_batch: 48
|
||||
last_completed_batch: 49
|
||||
last_cumulative_review: batches_46-48
|
||||
|
||||
Reference in New Issue
Block a user