Skip to content

small fixes 2026-08-05: refuse a sub-second windowing epoch (issue #390) - #398

Merged
espg merged 11 commits into
mainfrom
claude/small-fixes-2026-08-05
Aug 24, 2026
Merged

small fixes 2026-08-05: refuse a sub-second windowing epoch (issue #390)#398
espg merged 11 commits into
mainfrom
claude/small-fixes-2026-08-05

Conversation

@espg

@espg espg commented Aug 5, 2026

Copy link
Copy Markdown
Member

Closes #390. Refs PR #367 (the validate-vs-render pattern the #390 entries mirror), PR #411 (which landed this bundle's former #406 entry separately — see the checklist), espg/mortie#159 (the module split that entry de-risked).

Checklist (one entry per bundled small-fix)

The first two entries are the same insight applied twice, which is why the second rides this branch rather than a follow-up: #390's fix exists because restating a renderer's rule lets the restatement drift from the renderer, and the raster branch three lines away was doing exactly that. The third entry is unrelated to them and rides the bundle purely as a same-day small-fix (CLAUDE.md §5). The branch keeps the dated claude/small-fixes- name.

Merge with main (2026-08-24)

The PR was parked as entangled with the #410 temporal design; that series has since merged in full (#456, #463, #473, #479, #481, #507), so the parking is resolved and main was merged into this branch (2d30bef1, regular merge commit — no rebase, no force-push).

Conflict inventory: one conflicted file, src/zagg/config.py, one hunk — the module imports. This branch adds timezone (for _UNIX_EPOCH); main adds asdict. Resolved as the union of both. Everything else auto-merged, and the former #406 commit ae408ea dropped out exactly as predicted in the hand-off comment: git diff origin/main after the merge touches only src/zagg/config.py, tests/test_hive_windows.py, and docs/hive_layout.md — the two #390 guards plus the reader caveat.

Guard re-verified at the current render sites (config.py moved substantially under the #410 series — e.g. #473 added _validate_time_source next door):

  • get_windowing is still the only windowing-epoch render site: the point branch renders _windows.iso_utc(_windows.parse_utc(block["epoch"])) (config.py:3613) and the raster branch renders _windows.iso_utc(_UNIX_EPOCH) (config.py:3605). grep -rn iso_utc src/zagg shows no other epoch render.
  • The round-trip refusal still guards it from _validate_windowing (config.py:1454), reached from validate_config (:537) and the raster-resolve path (:1215); the schedule: none early return and the raster knob rejection still precede it, so both carve-outs hold unchanged.
  • The refusal message and all guard tests are unchanged and pass: test_subsecond_epoch_rejected, test_whole_second_epoch_accepted, test_schedule_none_keeps_subsecond_epoch, test_raster_epoch_renders_through_iso_utc, test_raster_and_point_epochs_spell_one_instant_alike (14 parametrized cases).
  • The Per-centroid temporal companion for t-digests: a 64-bit hierarchical time cell, mirroring the spatial location companion #410 series added a second epoch-bearing block, output.time_source (time_axis.toc_source), and it does not need this guard on its consumption side: its epoch is consumed at full parsed precision (time_axis.observation_words calls parse_utc and computes in nanoseconds — nothing truncates it), and when the block is absent the fallback consumes get_windowing's already-guarded rendered epoch. The guard itself was not relocated — but the adversarial review found a third direction this note originally missed: _validate_time_source's cross-check compared the declared time_source epoch against get_windowing's already-rendered epoch and runs before _validate_windowing, so a config declaring both blocks with the byte-identical sub-second epoch was refused with a false "disagrees" diagnostic instead of the windowing.epoch: validate the rendered value (sub-second epoch silently shifts every window conversion) #390 message. Fixed in the fold (12039a3b): the cross-check now compares declared-vs-declared parsed instants, which also tightens it (a real sub-second disagreement no longer truncates away).

What this does

output.windowing.epoch was validated at full parsed precision (_validate_windowing) but rendered through whole-second truncation — get_windowing canonicalizes it with windows.iso_utc, which is timespec="seconds". A sub-second epoch such as 2018-01-01T00:00:00.5Z therefore passed validation and then shifted every window boundary conversion by the dropped fraction, invisibly.

Precisely: there is one render site — get_windowing at config.py:2746, iso_utc(parse_utc(block["epoch"])) — but several consumers of the value it renders: the manifest temporal block (hive.build_manifest, hive.py:330-350) and every stamp conversion through windows.iso_time_range. The truncation happens once and propagates to all of them.

The fix mirrors the merged #367 pattern (f06a1e7 refinement — make the predicate the renderer): validate the rendered form by round-tripping it, and refuse when it is not the declared instant.

rendered_epoch = _windows.iso_utc(parsed_epoch)
if _windows.parse_utc(rendered_epoch) != parsed_epoch:
    raise ValueError(...)

Calling the renderer rather than restating its truncation rule is the point: the guard cannot drift from get_windowing if the rendering precision ever changes. The refusal names the declared instant, the rendered value and the exact drop:

output.windowing.epoch '2018-01-01T00:00:00.000001+00:00' carries sub-second precision
that the canonical whole-second rendering drops: it is recorded as
'2018-01-01T00:00:00+00:00', which would shift every window conversion by 1 µs —
declare the epoch at second precision

Refuse, not widen — consistent with the merged range ruling on #367: 1 s granularity means finer intent is a config error, not something to round for the author. The guard is "the rendering is lossless", not "the declaration is spelled canonically", so every whole-second spelling stays legal (…00:00Z, …00:00+00:00, naive …00:00, and offset-bearing 2018-01-01T05:30:00+05:30) and canonicalizes to the same instant. The comparison is between instants, not spellings, so a whole-second sub-minute offset (…00:00:30+00:00:30, which Python accepts) round-trips clean too.

Scope note: _validate_windowing rejects the conversion knobs on raster configs, so this guard is on the point-pipeline branch only, where the epoch is author-declared. The raster branch's own hardcoded rendering is the second entry, below.

What the second entry does (the raster branch)

get_windowing's raster branch returned the epoch as a string literal:

"epoch": "1970-01-01T00:00:00+00:00",

That literal is not a value — it is a restatement of windows.iso_utc's output format (windows.py:122-124: seconds precision, +00:00 offset). Three lines below, the point branch renders through _windows.iso_utc(_windows.parse_utc(block["epoch"])) and follows iso_utc automatically. So if iso_utc's spelling or precision ever changed — Z instead of +00:00, say — the two branches would write different spellings of the same instant into their manifests' temporal.epoch. That is the #390 drift class, one level up: the same reason the guard calls the renderer rather than restating its truncation rule.

The fix hoists the epoch to a module-level constant held as an instant, not a spelling, and renders it at use:

#: The raster branch's fixed windowing epoch (issue #247, ratified): raster
#: window membership is the acquisition's STAC ``datetime`` ...
#: Held as an *instant*, not a spelling — ``get_windowing`` renders it through
#: ``windows.iso_utc`` ... (issue #390).
_UNIX_EPOCH = datetime(1970, 1, 1, tzinfo=timezone.utc)
"epoch": _windows.iso_utc(_UNIX_EPOCH),

A datetime rather than an ISO string was chosen because a string constant — even one spelled ...Z — still is a rendering, so it keeps a second spelling in the file for the renderer to drift from; the datetime leaves the module with exactly one place that decides how an instant is written. The call shape iso_utc(<datetime>) also matches how the explicit-window bounds are already rendered just above in the same function (config.py:2737-2738, _windows.iso_utc(start)), so no new shape is introduced. parse_utc is not wrapped around it: it is the declared-input normalizer, and an already-aware constant has nothing to normalize (iso_utc calls .astimezone(timezone.utc) itself). Placement follows _POINT_WINDOW_WIDTH (config.py:1087) — a #: constant immediately above its only consumer.

The #390 sub-second guard itself does not extend to raster, deliberately. There is no author-declared value on that branch to guard: _validate_windowing refuses epoch/scale/units outright on raster configs (config.py:993-999, ratified #247 — window membership is the acquisition's STAC datetime, already an ISO-8601 UTC instant, so there is nothing to convert). A round-trip check there would be validating a constant the code had just written one line earlier — it can never fail, so it would test nothing and only suggest a knob exists. The validation asymmetry is left exactly as it is; only the hardcoded rendering was at issue.

Also updated: the get_windowing docstring (the epoch canonicalization is lossless on any config that passed validate_config, unlike explicit-window bounds, which truncate either way) and the docs/hive_layout.md validation paragraph, which already documented the bound-truncation edge and now records the epoch as the one time value held to the stricter rule.

Adversarial review fold

  • 1bca2f1 — the refusal now renders parsed_epoch.isoformat() instead of repr(epoch). yaml.safe_load types an ISO timestamp scalar as a datetime (YAML 1.1 timestamp resolution) and windows.parse_utc passes a datetime through, so on the path a real config takes the author got a stdlib constructor call back rather than their own value. test_subsecond_epoch_rejected gains a datetime-typed param so the loader-shaped input is pinned.
  • fd2fdd0 — the dropped amount is reported as parsed_epoch.microsecond (an integer count of µs) rather than total_seconds(), which rendered the smallest and most plausible violation — a microsecond-precision timestamp pasted from a data file — as 1e-06 s. The comment above the guard now states that the drop is exactly the microseconds field, hence integral and never negative, instead of leaving that to be re-derived. The message's closing sentence ("Window granularity is whole seconds, so a finer epoch is a config error, never a silent shift") is dropped as a verbatim restatement of the code comment and the docs/hive_layout.md paragraph — CLAUDE.md §4 terseness, and the neighbouring refusals in this file state the rule and the fix and stop. A .000001Z param pins the new rendering.
  • c33b0f1 — the get_windowing docstring no longer asserts losslessness unconditionally. It holds only for the validated path: validate_config never appears in runner.py, which calls get_windowing at :809, :2793 and :3227, and load_config_from_dict is documented as skipping validation. The docstring now scopes the claim and names the unvalidated paths that still truncate. No second guard was added at the render site — in the shipped flows the dispatcher validates before it builds the worker event, so this is not a hole the PR opens.
  • dd5ed4dtest_schedule_none_keeps_subsecond_epoch pins the deliberate carve-out: the guard sits after the schedule: none early return (config.py:962), so an inert block with a sub-second epoch validates and get_windowing returns None. That is the case the refusal would false-positive on if the guard were ever hoisted, and it is the schedule: none ≡ absent-block equivalence the surrounding tests already treat as load-bearing (test_absent_is_none). The raster carve-out is left to the existing raster-knob rejection test, which refuses epoch being present at all and so subsumes the sub-second case.

Assessment of the three flagged sites (issue #390, "assess while in there")

The issue asked whether the render-only downward truncation of recorded range ends is the same class, and to fix only if so. Assessment: none of the three is the #390 class, so none is fixed here — but the three do not share one rationale, so they are taken per site.

The #390 defect is a validate-vs-render mismatch: a declared value is accepted at one precision and then computed with at another, so the store's behavior silently disagrees with the config the author wrote.

(1) windows.union_time_range (windows.py:302). No declared input, and in practice no truncation at all: the inputs are leaf D15 time_range stamps, already seconds-precision ISO strings, so iso_utc(parse_utc(x)) over them is idempotent. Nothing is dropped here; the site only re-renders values another site already truncated.

(2) windows.iso_time_range (windows.py:313). No declared input. It renders a measured extent — the worker's [float(col.min()), float(col.max())] over data actually written (processing/worker.py:684) — converted through the (now-guarded) epoch and truncated to the seconds precision convention documents for every ISO instant in the store (docs/hive_layout.md, "Stamps carry the truth"). There is no author-declared instant for the rendering to contradict, and the truncation is uniform across both ends and every consumer, including the idempotence compare at sweep.py:234 that skips a root-summary rewrite when the union renders unchanged. Correct recording at a documented precision, not a silent shift of a declared conversion. It is one of the two paths into question (1) below.

(3) raster._us_iso (raster.py:494, truncating at :502). The provenance here is not the point worker's, and an earlier revision of this section wrongly gave it that one. This site is fed at raster.py:1334:

instants = [_iso_us(e["datetime"]) for e in granules if e.get("assets")]
if instants:
    time_range = [_us_iso(min(instants)), _us_iso(max(instants))]

e["datetime"] is the acquisition's STAC datetime — a declared ISO instant carried in the catalog, not a float measured off the data — and sub-second STAC datetimes are routine (Sentinel-2 and Landsat items commonly carry milliseconds). So "no declared input" is simply false here, and the ruling has to rest on something else.

Redone on its own facts, the conclusion holds for a different reason: nothing is computed from the truncated value. Raster window membership is decided at dispatch on the parsed instant — runner.py:2285-2288 takes min(parse_utc(e["datetime"])) per acquisition group and hands it to windows_intersecting — and the leaf's own time axis is the microsecond int64 times_us built by raster_time_index (raster.py:534, consumed at :1250), also full precision. _us_iso is reached only at raster.py:1336, writing the D15 stamp. The truncation therefore changes what is recorded, never what is computed: no window assignment, no time coordinate and no filter moves by it. That is materially different from the epoch defect, where the truncated value is the conversion every window boundary is computed through.

One consequence worth recording rather than closing silently: because the raster stamp's input is a declared STAC datetime that routinely carries milliseconds, the ≤ 1 s narrowing in question (1) is reachable through the raster path too, not only through a float delta_time column.

A separate raster/point asymmetry surfaced while making the change above and is filed as #404, not fixed here: the raster branch records units: "seconds" in the manifest temporal block (config.py:2757) while the raster store's time coordinate array carries CF attrs units: "microseconds since 1970-01-01T00:00:00" (processing/raster.py:835). Tracing confirms these are different contracts, not an inconsistency in code — the manifest triple describes a source time_field conversion, and no consumer applies it to the stored array. Both readers of windowing["units"] are point-only: windows.iso_time_range (windows.py:315) is reached solely from hive.process_and_write_hive (hive.py:1299), and runner._windowed_units (runner.py:2219) solely from SpatialStrategy's _run_local/_run_lambda. The raster fan-out uses _raster_windowed_units (runner.py:2245), which reads only schedule/windows and decides membership from parse_utc(e["datetime"]) (runner.py:2286-2288), and the raster worker renders its own D15 stamp at raster.py:1332-1336. So on raster the triple is write-only. #404 owns the question of whether to drop it or document it.

What the third entry does (issue #406 — mortie's flat exports) — historical; landed via PR #411

Four import sites reached mort2polygon / mort2geo through mortie's submodule path when both names are already exported flat from mortie/__init__.py:

from mortie.tools import mort2polygon   # before
from mortie import mort2polygon         # after

mortie.tools is not an advertised submodule — mortie/__init__.py re-exports these names flat (both are in its __all__), and only arrow and morton_index are exposed as submodules via from . import …. So the submodule spelling was an implementation detail being reached through, and a private path can break on any mortie release without a deprecation cycle.

The immediate motivation is espg/mortie#159, which splits mortie's tools.py into domain modules and moves mort2polygon into convert.py. These sites were that split's entire external blast radius in zagg; after this commit it is zero. The change is mechanical — same function object, same call, different import path — and works on every mortie version that has the flat export, so the mortie>=0.7.2 floor in pyproject.toml is untouched.

Sites changed (ae408ea, +4/−4 across three files):

file symbol
src/zagg/grids/healpix.py:252 (shards_bbox) mort2polygon
src/zagg/grids/healpix.py:436 (shard_footprint) mort2polygon
demo/05_california_read.ipynb (cell 12) mort2polygon
notebooks/aoi_mask.ipynb (cell 3) mort2geo

The two src/ sites are the ones that matter for the package; both are function-local imports and stay in place (shard_footprint's neighbouring from shapely.geometry import Polygon still sorts after mortie, so ruff's I rules are unaffected).

Two corrections to the site list as filed on #406, both found by re-running the issue's own grep -rn "mortie.tools" . against the tree:

  1. notebooks/aoi_mask.ipynb imports mort2geo, not mort2polygon. Same treatment — mort2geo is flat-exported and in __all__ too, and mortie.mort2geo is mortie.tools.mort2geo — but worth recording, since 88S offsets extraction: per-chunk byte offsets for the pinned benchmark shard (issue #158) #159's split moves the two names independently. That notebook already imported moc_to_order flat two cells earlier, so the file is now internally consistent.
  2. The three data/ scripts named on the issue are not in the repo. data/build_aoi_shardmap.py, data/conus/build_conus_shardmap.py and data/conus/plot_conus_shardmap.py are untracked working files (git ls-files data is empty on main and on this branch), so there is nothing for a commit here to change. They still carry the old import locally and will need the same one-word swap out-of-tree if 88S offsets extraction: per-chunk byte offsets for the pinned benchmark shard (issue #158) #159 lands; that is not this PR's to make, and Use mortie's flat exports instead of the mortie.tools submodule path (6 sites) #406 can close on the in-tree sites.

Notebook edits touch the import line only — neither notebook was re-executed, and both carry zero stored outputs already, so the two .ipynb diffs are one line each (git diff --stat: demo/05_california_read.ipynb | 2 +-, notebooks/aoi_mask.ipynb | 2 +-). Binder runnability (CLAUDE.md §4) is unaffected: the import path change requires no new dependency and no version floor move.

What the fourth entry does (the time_range tail narrowing — ruled (b))

Question (1) below asked which of three arms to take on the ≤ 1 s downward narrowing of the recorded time_range upper bound. The ruling is (b): document the narrowing as a reader caveat and keep the stamp bytes stable — not (a) render-outward and not (c) sub-second precision, both of which change the bytes of a stamp field external decoders read and so pull the change inside CLAUDE.md §4's normative-store-contract rule (docs/specification.md plus the tools/generate_spec_fixtures.pytests/data/spec/ fixtures in the same PR).

ce7d033 is docs-only: no rendering code, no stamp bytes, and no test's expected values change (git show --stat ce7d033 is docs/hive_layout.md | 24 +++-). The caveat lands in The commit stamp section, immediately after the sentence that defines time_range as "the actual [t_min, t_max] written":

Reader caveat — t_max floors, so the recorded range can end up to 1 s early. Both ends render through windows.iso_utc's whole-second granularity (isoformat(timespec="seconds")), so each truncates to the second containing it. On t_min that is harmless: the recorded start is at or before the true first observation. On t_max it is not — a last observation at 12:00:00.7 is recorded as 12:00:00, so the closed [t_min, t_max] is a slight under-estimate of the extent written, not an envelope around it. A consumer that prunes leaves on the stamp must treat t_max as inclusive with 1 s of slack (test the query against t_max + 1 s); pruning on the recorded value alone can skip a leaf that genuinely holds data in the queried interval. […]

The Time windows section's "Stamps carry the truth" bullet (D15), which is where a reader first meets the field, gains a one-clause pointer to it rather than a second copy of the text — normative-ish prose duplicated across sections drifts:

-  `time_range` as ISO-8601 UTC strings; the root `coverage.moc` summary
+  `time_range` as ISO-8601 UTC strings (both ends at whole-second
+  granularity, which narrows the tail by up to 1 s — see
+  [The commit stamp](#the-commit-stamp)); the root `coverage.moc` summary

No fixture regeneration is required, and this was verified rather than assumed. CLAUDE.md §4 triggers the fixture rule on a change to a wire format, an attrs grammar, or a versioned spec marker; a sentence describing behavior that already ships changes none of the three. Concretely, at ce7d033:

  • grep -c time_range docs/specification.md0, and grep -c time_range tools/generate_spec_fixtures.py0;
  • grep -rl time_range tests/data/spec/no files (the three fixture pairs minimal, pyramid, kitchen_sink never carry the field);
  • the diff touches no spec marker: morton-hive/1//2 are written unchanged by hive.stamp_commit, and no zagg-*/N string appears in the diff.

So the spec page and the fixtures are untouched and owe no update — a reviewer does not have to re-derive that.

But (see question (2)) the spec page had no home for the normative half of this caveat, so that half did not land. The hazard is specifically aimed at "a consumer that prunes on the stamp — an external reader decoding from the spec", and such a reader never opens docs/hive_layout.md; the intent was to land a short normative sentence in docs/specification.md as well. time_range turns out not to be described there at all, so there was nothing to qualify and no section it belongs to — inventing one was judged worse than reporting the gap.

Questions for review

  1. The recorded time_range upper bound floors downward, narrowing the envelope. — RULED (b), landed in ce7d033. time_range is documented as the closed [t_min, t_max] actually written, and both ends floor to the second containing them. Flooring t_min is harmless (the recorded start is at or before the true first observation), but flooring t_max makes the recorded end up to 1 s earlier than the true last observation: data at 12:00:00.7 is recorded as ending at 12:00:00. A consumer that prunes on the stamp — an external reader decoding from the spec, per CLAUDE.md §4 — can therefore prune a leaf that genuinely holds data in the queried interval. No in-tree consumer prunes on time_range today (every use is a union into the regenerable root coverage.moc summary or the overview rollup), so nothing is broken now. Reachable on both pipelines: via a float delta_time column (point) and via a millisecond-bearing STAC datetime (raster — see (3) above).

    The three arms as posed: (a) render the envelope outward — floor t_min, ceil t_max — which makes the stamp a guaranteed superset and never prunes real data, but redefines a field documented as the actual value into an envelope and changes the stamp bytes every store writes; (b) leave it and document the ≤ 1 s narrowing as a reader caveat, keeping "actual" literally true and the bytes stable; (c) record t_max at sub-second precision, which breaks the store-wide "every ISO instant is seconds-precision" convention for one field.

    Cost asymmetry that decided it: (a) changes the bytes of a stamp field external decoders read, which puts it inside CLAUDE.md §4's normative-store-contract rule — it would require docs/specification.md and the tools/generate_spec_fixtures.pytests/data/spec/ fixtures updated in the same PR. (c) changes those bytes too. (b) does not. Ruling: (b) — the exposure is bounded, no in-tree reader is affected, and byte-stability of a stamp field external decoders already read is worth more than the ≤ 1 s tightening. Implemented in the section above; the ≤ 1 s narrowing is now recorded behavior, not an unflagged trap.

  2. time_range is not described in docs/specification.md at all — so the caveat could not reach the reader it is aimed at. Reporting rather than acting, because the fix is a scope call, not a docs edit.

    The caveat's whole subject is an external reader that decodes from the spec and the fixtures alone (CLAUDE.md §4) — precisely the reader that raster temporal block declares units: seconds while the time coordinate array declares microseconds — which governs for an external reader? #404 records a docstring cannot protect. That reader never opens docs/hive_layout.md. Landing a normative sentence in docs/specification.md was therefore the intent, and it did not land because there is nothing there to qualify:

    • grep -c time_range docs/specification.md0; likewise t_min/t_max/morton-hive/2, and the only "commit stamp" mentions (:589, :1078) say the stamp is written last and never describe its contents.
    • The page's sections are §1–3 array byte layouts, §4 pyramid/overview declarations (the role/zagg_overview attrs grammar), §5 O11 hashes, §6 zagg-ragged/2, §7 fixtures. None is a home for a leaf-stamp field, and the page states its own scope explicitly: it "owns the array-level contracts inside a leaf; it cites mortie's page for path and word semantics and never restates them", because "duplicated normative text drifts".
    • The stamp grammar is ceded to mortie's spec page — which describes morton-hive/2 leaf naming and the schedule grammar (§6.3) and the stamp's coverage envelope (§7.1), but not the D15 time_range. Its only time_range mention is §7.3, as an informative carrier field on the root coverage MOC — a different object from the leaf stamp.

    So the leaf stamp's window/time_range pair appears to have no normative description on either page: an external decoder learns the field exists only from docs/hive_layout.md, a narrative page explicitly not the contract. That is a larger gap than this small-fix, and where it should be closed is a fork: (a) add a leaf-stamp section to zagg's docs/specification.md (contradicts its stated scope split, and would need the fixture question answered — today's fixtures carry no stamp time_range); (b) extend mortie's §6.3 to describe the /2 stamp fields, since that page already owns morton-hive/N (cross-repo, so out of this PR either way); or (c) rule the field deliberately informative and say so once, in whichever page owns it. No issue filed and no spec edit made — filing is a side-effecting directive per CLAUDE.md §6, and inventing a section in the normative page to hang one sentence on seemed clearly worse than surfacing the gap.

Testing

  • uv run pytest -q at head ce7d033 (the caveat commit): 3,508 passed / 16 skipped / 1 failed in 240 s — the same counts as the ae408ea run below, as a docs-only diff must produce. The one failure is the pre-existing, flagged-not-fixed test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds. uv run ruff check src tests → the one pre-existing N818 at src/zagg/registry.py:64; uv run ruff format --check src tests → the one pre-existing tests/data/benchmark/README.md diff. Nothing new from this commit, and no test's expected values were touched — the ruling (b) is explicitly the arm that leaves the stamp bytes alone. No test was added: there is no behavioral surface to pin, since the commit adds prose describing rendering that already ships and is already covered (windows.iso_utc's seconds precision is asserted by the existing windowing tests).
  • uv run pytest -q at head ae408ea (the Use mortie's flat exports instead of the mortie.tools submodule path (6 sites) #406 commit): 3,508 passed / 16 skipped / 1 failed. The one failure is the pre-existing, flagged-not-fixed test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds; the flaky test_client_transport.py::TestStatusPoller test that also failed in the b95ac50 run below passed this time, which is the whole of the +1 in the pass count over the same 3,525 collected. uv run ruff check src tests / uv run ruff format --check src tests are byte-for-byte unchanged from b95ac50 — the same two pre-existing items (N818 at src/zagg/registry.py:64, the tests/data/benchmark/README.md format diff) and nothing new from this diff.
  • For Use mortie's flat exports instead of the mortie.tools submodule path (6 sites) #406 specifically: grep -rn "mortie.tools\|from mortie import tools" . --exclude-dir=.git returned the four sites in the table before the change and nothing after (the acceptance check the issue names). No test was added — the change is import-path-only with no behavioral surface to pin, so the existing coverage of shards_bbox / shard_footprint is the test, and both were additionally exercised end-to-end against a real California shard key (3800171670338011145: a 129-vertex footprint and bbox [-118.3887, 33.8687, -118.2129, 34.0486]) to confirm the rebound name resolves and returns the same shapes. Verified in the resolved env that mortie.mort2polygon is mortie.tools.mort2polygon and likewise for mort2geo (mortie 0.9.4), so the swap is object-identical and not merely name-compatible.
  • uv run pytest -q at head b95ac50: 3,507 passed / 16 skipped / 2 failed, both pre-existing and flagged-not-fixed — test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds (environment-dependent, flagged on pyramid v2: leaf-worker column writes (issue #383) #391/leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397) and the flaky test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries poller test. (Counts differ from the earlier dd5ed4d run of 3,485/38 because this environment has the catalog/analysis extras installed, which un-skips those suites.) Targeted: tests/test_hive_windows.py tests/test_raster_pipeline.py tests/test_config.py tests/test_windows.py tests/test_hive.py629 passed, with every pre-existing raster windowing test (test_raster_pipeline.py::TestRasterConfig::test_windowing_validates_and_normalizes and the knob-rejection matrix) passing unchanged.
  • New coverage in tests/test_hive_windows.py: test_subsecond_epoch_rejected (parametrized over a plain .5Z epoch, an offset-bearing .001+05:30 one, a .000001Z microsecond one and a datetime-typed one as yaml.safe_load produces, asserting the message quotes the declared instant, the rendered value and the µs drop, so the refusal stays tied to what get_windowing emits), test_whole_second_epoch_accepted (parametrized over four legal spellings, asserting each canonicalizes to 2018-01-01T00:00:00+00:00) and test_schedule_none_keeps_subsecond_epoch (the inert-block carve-out).
  • uv run ruff check src tests / uv run ruff format --check src tests: clean on the diff. Two pre-existing items flagged, not fixed: N818 at src/zagg/registry.py:64 and the format diff on tests/data/benchmark/README.md (both predate this branch and are already flagged on pyramid v2: leaf-worker column writes (issue #383) #391/leaf skip-if-current: input-identity no-op with lifecycle touch (issue #388) #397).
  • Checked that no shipped config, doc example, or test fixture declares a sub-second epoch, so nothing existing is newly refused.
  • For the raster entry, two new tests in tests/test_hive_windows.py pin the drift-proofing rather than today's string:
    • TestWindowingConfig::test_raster_epoch_renders_through_iso_utc — asserts the rendered raster epoch equals windows.iso_utc(windows.parse_utc(datetime(1970, 1, 1, tzinfo=utc))), i.e. against the renderer's own output, not a literal. It therefore survives a change to iso_utc's precision or offset form and can only be broken by a literal re-entering config.py.
    • TestManifestTemporal::test_raster_and_point_epochs_spell_one_instant_alike — builds a point config declaring the Unix epoch and a raster config, runs both through hive.build_manifest, and asserts the two temporal.epoch strings are identical. This is the test that actually fails if someone reintroduces a literal on either branch.
    • Both were mutation-checked: with the literal restored and iso_utc switched to emit Z, both fail; on the shipped code both pass. A shared _raster_windowed() helper was added next to the existing _windowed(); no existing test was modified.

Not addressed here, and not this PR's to fix: src/zagg/config.py is ~2,981 lines, far over CLAUDE.md §4's ~1,200 ceiling. That is pre-existing and already covered by the planned config.pyconfig/ split (issue #330 / PR #349), so this PR does not split it.


Generated by Claude Code

@espg espg added the implement label Aug 5, 2026

@espg espg left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Adversarial review of the phase commit against issue #390 and CLAUDE.md. Reviewed at branch head c733d7c (the PR body's "Testing" section cites head 78244fd — worth reconciling; the branch tip here is c733d7c).

The guard itself is sound. I tried to break it and could not:

  • No false positive on legal epochs. The predicate is parse_utc(iso_utc(parsed)) != parsed, i.e. "does the instant survive the render", not "is the spelling canonical". Whole-second offsets survive, including offsets that are not whole minutes — 2018-01-01T00:00:30+00:00:30 round-trips clean (Python accepts sub-minute offsets, so this was a real candidate). Naive input, Z, +00:00 and +05:30 all pass, as the accept-test asserts.
  • The shift can never be negative. isoformat(timespec="seconds") drops microseconds, which are always >= 0, so rendered <= parsed always. (Its formatting is a separate problem — see the inline note at config.py:1062.)
  • Scoping is as claimed. Raster returns early at config.py:1001 and schedule: none at :962, so the guard reaches only the point branch where the epoch is author-declared.
  • Calling the renderer rather than restating its rule is the right call and matches _explicit_window_bounds (config.py:1140) exactly.

Four inline findings, none of them a defect in the predicate: a message that quotes a datetime repr on the real YAML path (and is untested there), 1e-06 s formatting for the microsecond case, a docstring that asserts losslessness as a property of get_windowing when it is a property of the validated path only, and two unpinned carve-outs.

On the assessment of the three flagged sites — one of the three has the wrong rationale.

The claim I checked hardest, and which holds: "No in-tree consumer prunes on time_range." True. Every use in src/ is a union feed or a union compare — coverage.py:349/367, sweep_overview.py:1317/1339 and :1461/1489, runner.py:1086/1488/3029/3749, client.py:1180, plus the idempotence compare at sweep.py:234-236. hive.py:854-876 only validates the pair. Nothing selects, filters or prunes on it. The "nothing is broken now" part of the assessment is grounded.

What does not hold is the shared justification. The PR body says all three sites "have no declared input. They render a measured extent — the worker's [float(col.min()), float(col.max())] over data actually written (processing/worker.py:684)." That provenance is the point worker's, and it is correct for union_time_range and iso_time_range. It is not the input to raster._us_iso. That site is fed from raster.py:1334:

instants = [_iso_us(e["datetime"]) for e in granules if e.get("assets")]
if instants:
    time_range = [_us_iso(min(instants)), _us_iso(max(instants))]

e["datetime"] is the acquisition's STAC datetime — a declared ISO instant carried in the catalog, not a float measured off the data. Sub-second STAC datetimes are routine (Sentinel-2 and Landsat items commonly carry milliseconds), so there is a declared value here to compare a rendered value against, which is precisely the property the assessment says is absent and which it uses to rule the site out of the #390 class.

The conclusion may well survive the corrected reasoning — window membership is decided at dispatch on the parsed instant (raster_time_index), never on the truncated stamp, so the truncation changes what is recorded, not what is computed, which is genuinely a different thing from the epoch defect. But issue #390 asked to "determine whether these are the same class ... and say which in the PR body", per site. One site got another site's evidence. Please redo that one on its own facts, and note that a sub-second STAC datetime makes the ≤ 1 s narrowing in question (1) reachable through the raster path too, not only through float delta_time.

Minor grounding slip in the same section: iso_time_range is cited as windows.py:318 ("was :313"). windows.py is untouched by this diff, so nothing moved — the iso_utc call is still at :313; :318 is the for t in time_range clause.

On question (1) — the t_max narrowing. I am not ruling, but one consequence is worth having on the record before anyone picks: arm (a) (floor t_min, ceil t_max) changes the bytes of a stamp field external decoders read, which puts it squarely inside CLAUDE.md §4's normative-store-contract rule — it would require docs/specification.md plus the tools/generate_spec_fixtures.py fixtures updated in the same PR. Arm (b) does not. That asymmetry is a real part of the cost comparison and is not currently in the writeup. Independently, the options as posed are complete and answerable in one pass, which is what §6 asks for.

Convention check (verified, not assumed):

  • §3 — commit is title-only, refuse a sub-second windowing epoch at validation (issue #390), matching recent git log style. Clean.
  • §4 spec rule — not applicable, checked rather than presumed. docs/specification.md contains no windowing-epoch content (its only epoch hit, at :1066, is an unrelated "schema epoch" phrase) and tools/generate_spec_fixtures.py never references epoch. Refusing a config alters no wire format, attrs grammar or versioned spec marker, so no spec/fixture update is owed here.
  • §4 dependencies — none added; the test file's new import re is stdlib.
  • §4 tests-in-same-PR — satisfied; uv run pytest tests/test_hive_windows.py -q gives 119 passed locally.
  • §4 module ceiling — src/zagg/config.py is 2,981 lines, already ~2.5x the 1,200 ceiling before this PR, which adds 22. Pre-existing and not this PR's to fix, but it is standing and un-raised as far as I can see.
  • Lint — uv run ruff check src tests reports exactly one error, the pre-existing N818 at src/zagg/registry.py:64, matching the PR body's claim.
  • §6 — no @-mentions, #-references are all real issues/PRs, enumerated options use (a)/(b)/(c). Clean.

Generated by Claude Code

Comment thread src/zagg/config.py Outdated
rendered_epoch = _windows.iso_utc(parsed_epoch)
if _windows.parse_utc(rendered_epoch) != parsed_epoch:
raise ValueError(
f"output.windowing.epoch {epoch!r} carries sub-second precision that the "

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

{epoch!r} quotes a datetime repr, not the text the author wrote — on the path a real config actually takes.

load_config reads the YAML with yaml.safe_load (config.py:172), and YAML 1.1 resolves an ISO timestamp scalar to a datetime object, not a string. windows.parse_utc passes a datetime straight through (windows.py:110-111), so the guard still fires correctly — but the message does not read like the config:

$ python -c "import yaml; print(repr(yaml.safe_load('epoch: 2018-01-01T00:00:00.5Z')['epoch']))"
datetime.datetime(2018, 1, 1, 0, 0, 0, 500000, tzinfo=datetime.timezone.utc)

Feeding that object through validate_config (verified locally at c733d7c):

output.windowing.epoch datetime.datetime(2018, 1, 1, 0, 0, 0, 500000,
tzinfo=datetime.timezone.utc) carries sub-second precision that the canonical
whole-second rendering drops: ...

The author wrote 2018-01-01T00:00:00.5Z and gets a stdlib constructor call back. Both new parametrized cases inject str epochs directly into the block, so the message assertion at tests/test_hive_windows.py:258 is only ever exercised on a spelling the YAML loader never produces — the message quality on the production path is untested.

Two things worth doing: render the declared value at full precision rather than repr-ing whatever type the loader handed over (e.g. parsed_epoch.isoformat(), which reads 2018-01-01T00:00:00.500000+00:00 for both input types), and add a datetime-typed param to test_subsecond_epoch_rejected so the loader-shaped input is pinned.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Both done in 1bca2f1.

The message now renders the parsed instant rather than repr-ing whatever the loader handed over:

f"output.windowing.epoch {parsed_epoch.isoformat()!r} carries sub-second "

which reads identically for a str config and for the datetime yaml.safe_load produces — '2018-01-01T00:00:00.500000+00:00' either way, at full precision so the author still sees the fraction that is being refused. A comment above the guard records why it is isoformat() and not repr, so a later edit does not quietly reintroduce the constructor call.

test_subsecond_epoch_rejected gained a declared param (the full-precision instant the message quotes) and a third case constructed the way the loader produces it — datetime(2018, 1, 1, 0, 0, 0, 500000, tzinfo=timezone.utc) — so the production-shaped input is now pinned on the message, not just on the refusal.

(fd2fdd0 then changed the drop rendering in the same message; the final text is quoted in the PR body.)


Generated by Claude Code

Comment thread src/zagg/config.py Outdated
f"output.windowing.epoch {epoch!r} carries sub-second precision that the "
f"canonical whole-second rendering drops: it is recorded as "
f"{rendered_epoch!r}, which would shift every window conversion by "
f"{(parsed_epoch - _windows.parse_utc(rendered_epoch)).total_seconds()} s. "

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

total_seconds() renders small shifts in scientific notation. Verified at c733d7c:

... which would shift every window conversion by 1e-06 s

for epoch: 2018-01-01T00:00:00.000001Z. That is the smallest representable violation and the one most likely to be a genuine accident (a microsecond-precision timestamp pasted in from a data file), and it is the case whose message reads worst. 1e-06 s also silently defeats the test's shift assertion style — the two params happen to be 0.5 and 0.001, which float.__str__ renders decimally, so the regex at tests/test_hive_windows.py:260 passes; a .000001 param would need a different literal. Formatting the drop explicitly (e.g. as microseconds, parsed_epoch.microsecond, which is exactly what is lost and is always an integer) avoids both.

Two smaller notes on the same block:

  1. The sign is fine — isoformat(timespec="seconds") drops microseconds, which are always >= 0, so parsed_epoch - parse_utc(rendered_epoch) can never be negative. Worth stating in the comment above rather than leaving a reader to re-derive it, since a negative "shift by" would be a nonsense message.
  2. Terseness (CLAUDE.md §4): the closing sentence — "Window granularity is whole seconds, so a finer epoch is a config error, never a silent shift" — restates the code comment at :1050-1055 and the new docs/hive_layout.md paragraph verbatim in intent. The neighbouring refusals in this file (e.g. :1109-1113, :1119-1123) state the rule and the fix and stop. Dropping that sentence leaves the message with what the author needs: the value, the rendered form, the drop, and "declare the epoch at second precision".

Generated by Claude Code

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

All three folded in fd2fdd0.

The drop is now parsed_epoch.microsecond, so the smallest violation reads as a count rather than in scientific notation:

output.windowing.epoch '2018-01-01T00:00:00.000001+00:00' carries sub-second precision
that the canonical whole-second rendering drops: it is recorded as
'2018-01-01T00:00:00+00:00', which would shift every window conversion by 1 µs —
declare the epoch at second precision

(1) The never-negative property is stated where the guard is, not left to be re-derived: "The drop is reported in microseconds because that is exactly what iso_utc discards and nothing else: an integer, so no scientific notation for the 1 µs case, and never negative."

(2) The closing sentence is gone. The message now stops at the actionable "declare the epoch at second precision", matching :1109-1113 / :1119-1123; the rule it restated is still carried by the comment above the guard and by the docs/hive_layout.md paragraph.

The tests moved with it — the parametrized shift is now the µs count (500000, 1000) and the case you named, .000001Z1, is pinned as a third param, so the formatting that read worst is the one the suite now asserts. Full suite green at dd5ed4d: 3,485 passed / 38 skipped.


Generated by Claude Code

Comment thread src/zagg/config.py Outdated

``epoch`` and explicit-window boundaries are canonicalized to ISO-8601
UTC strings; ``windows`` is ``None`` except for ``schedule: explicit``,
UTC strings. The ``epoch`` canonicalization is lossless — ``timespec=

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

"The epoch canonicalization is lossless" is stated as a property of get_windowing, but it is only a property of the validated path — and get_windowing has in-tree callers that never validate.

validate_config does not appear anywhere in runner.py:

$ grep -c "validate_config" src/zagg/runner.py
0

yet runner.py calls get_windowing at :809, :2793 and :3227, and the epoch it returns is written into the manifest temporal block (runner.py:2217, hive.py:333) and drives every stamp conversion through windows.iso_time_range (windows.py:315). zagg.notebook.run (notebook.py:276) takes a PipelineConfig and does not validate either. load_config_from_dict is explicitly documented as skipping validation — your own comment at sweep_overview.py:255:

the Lambda worker builds its config with load_config_from_dict, which never calls validate_config

So agg(load_config_from_dict(payload)) with a sub-second epoch still truncates silently, exactly as before this PR. In practice the dispatcher validates before it builds the event, so the shipped flows are covered — this is not a hole the PR opens, and I am not asking for a second guard at the render site.

What I am asking for is that the docstring not assert an invariant the function does not hold. "lossless" here is load-bearing: a future reader of get_windowing may take it as licence to skip a check. Scope it — "lossless on any config that passed validate_config; configs built through load_config_from_dict without validation still truncate" — which is the same caveat _fold_plan and declare_pyramid already carry for the templating path.

Same over-claim in the PR body: "the one place it is consumed". The epoch is consumed at config.py:2764 (iso_utc(parse_utc(block["epoch"]))), and that value then fans out to the manifest and to iso_time_range; one render site, several consumers of the rendered value.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Scoped in c33b0f1 — agreed the unqualified "lossless" was an invariant the function does not hold, and it is exactly the kind of claim a later reader would lean on to skip a check. The docstring now reads:

``epoch`` and explicit-window boundaries are canonicalized to ISO-8601
UTC strings. The ``epoch`` canonicalization is lossless on any config that
passed ``validate_config`` — ``timespec="seconds"`` would otherwise truncate
a sub-second epoch and shift every window conversion, so
:func:`_validate_windowing` refuses one outright (issue #390). A config
built without that check (``load_config_from_dict``, a hand-rolled worker
payload) still truncates here; explicit bounds truncate either way,
documented on :func:`_explicit_window_bounds`.

Phrasing follows the caveat processing/write.py:728-736 and processing/raster.py:568-575 already carry for the unvalidated-payload path.

No second guard at the render site, as you asked — the dispatcher validates before it builds the event, so this is a documentation accuracy fix, not a hole to plug.

The PR body over-claim is corrected too, in the same terms: one render site (config.py:2746, iso_utc(parse_utc(block["epoch"]))), several consumers of what it renders — the manifest temporal block (hive.py:330-350) and every stamp conversion through windows.iso_time_range.


Generated by Claude Code

# canonicalizes to the same instant.
_windowed(cfg, epoch=epoch)
validate_config(cfg)
assert get_windowing(cfg)["epoch"] == "2018-01-01T00:00:00+00:00"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

The two deliberate carve-outs are unpinned. The guard sits after two early returns in _validate_windowing, and both exemptions are correct — but nothing in the suite says so, and a refactor that hoists the guard would flip them silently.

(1) schedule: none. The early return at config.py:962 means an inert block keeps a sub-second epoch. Verified at c733d7c:

cfg.output["windowing"] = {"schedule": "none", "time_field": "delta_time",
                           "epoch": "2018-01-01T00:00:00.5Z", "scale": "gps"}
validate_config(cfg)          # passes
get_windowing(cfg)            # None

That is right — get_windowing returns None, the epoch is never rendered, and per the comment at :955-958 an inert block must stay equivalent to an absent one. But it is exactly the case where the new refusal would be a false positive if the guard ever moved above that return, and TestWindowingConfig has no test for it.

(2) The raster branch. The PR body leans on this in its scope note — "the raster branch hardcodes epoch ... so this guard is on the point-pipeline branch only" — and the early return at config.py:1001 is what makes it true. The existing raster-knob rejection test covers epoch being present at all, which subsumes the sub-second case today, but the scope claim itself is inferred rather than asserted.

(1) is the one worth adding: two lines in this class, and it pins the interaction between the new guard and the schedule: none equivalence rule that the surrounding tests already treat as load-bearing (test_absent_is_none at :50).

Separately, the comment at :243-244"the offset is whole minutes, so only the sub-second part is ever what iso_utc drops" — generalises further than it should. Python's fromisoformat accepts sub-minute offsets (2018-01-01T00:00:30+00:00:30 parses, and round-trips clean through the guard — I checked). The guard is still correct there because it compares instants, not spellings, which is the stronger statement and the one the comment should make.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Both folded in dd5ed4d.

(1) test_schedule_none_keeps_subsecond_epoch added to TestWindowingConfig, right after the accept test:

    def test_schedule_none_keeps_subsecond_epoch(self, cfg):
        # The guard sits AFTER the ``schedule: none`` early return, and must:
        # an inert block is equivalent to an absent one (test_absent_is_none),
        # get_windowing returns None, and the epoch is never rendered — so
        # there is no conversion to shift and nothing to refuse.
        cfg.output["store_layout"] = "hive"
        cfg.output["windowing"] = {
            "schedule": "none",
            "time_field": "delta_time",
            "epoch": "2018-01-01T00:00:00.5Z",
            "scale": "gps",
        }
        validate_config(cfg)
        assert get_windowing(cfg) is None

That is the pair you described — it validates and returns None — so a hoist of the guard above config.py:962 now fails a test rather than silently turning the equivalence rule into a refusal.

Raster carve-out left as you judged: the existing raster-knob rejection test refuses epoch being present at all, which subsumes the sub-second case, so a second test there would only re-assert the earlier return.

(2) Comment fixed to the stronger statement:

            # The fraction survives a non-UTC declaration: the guard compares
            # INSTANTS, not spellings, so it is the dropped fraction that
            # refuses this — never the offset (Python accepts sub-minute
            # offsets, and a whole-second one of those round-trips clean).

Thanks for checking +00:00:30 against the guard rather than against the old comment — the whole-minute framing was doing work it could not support.


Generated by Claude Code

@espg

espg commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Status: the single bundled fix is complete, the adversarial review round is folded, and the PR is awaiting review — applying waiting.

  • Phase commit: c733d7c — refuse a sub-second output.windowing.epoch at validation by round-tripping the rendered value (the merged PR small fixes 2026-08-03: explicit-window point entries #367 make-the-predicate-the-renderer pattern).
  • Adversarial self-review: 1 round, 4 inline findings, all four folded — one commit each, every thread answered with its fix sha:
    • 1bca2f1 — the refusal quoted a datetime repr on the real config path (yaml.safe_load types an ISO scalar as datetime, and parse_utc passes it through), so the author saw a stdlib constructor call instead of what they wrote. Now renders the declared instant; a datetime-typed param pins the loader-shaped input.
    • fd2fdd0 — a 1 µs shift printed as 1e-06 s, the smallest and most plausible real violation reading worst. Now reported in whole microseconds, with the never-negative property stated rather than left to be re-derived, and the redundant closing sentence dropped (§4 terseness).
    • c33b0f1 — the get_windowing docstring asserted losslessness unqualified; it holds only for configs that passed validate_config (runner.py never calls it, and load_config_from_dict is documented as skipping it). Scoped, with the unvalidated paths named. No second render-site guard added — deliberately out of scope.
    • dd5ed4d — pinned the schedule: none carve-out (an inert block keeps a sub-second epoch and never renders it) and corrected an over-general test comment: the guard compares instants, not spellings, which also covers sub-minute tz offsets.
  • The review also caught a real error in my own assessment, now corrected in the PR body: the three flagged sites were justified with one shared rationale ("measured extent, no declared input"), which is the point worker's provenance and simply wrong for raster._us_iso — that one is fed from the acquisition's STAC datetime (raster.py:1334), a declared instant that routinely carries milliseconds. Redone per site. The conclusion survives on its own facts — raster window membership is decided at dispatch on the parsed instant and the leaf time axis is microsecond int64, so the truncation changes what is recorded, never what is computed — but the reasoning that got there was borrowed, and issue windowing.epoch: validate the rendered value (sub-second epoch silently shifts every window conversion) #390 asked for a per-site determination.

Verification. Full suite green locally at head dd5ed4d: 3,485 passed / 38 skipped, 0 failures. ruff check / format --check clean on the diff (the pre-existing N818 at src/zagg/registry.py:64 and the tests/data/benchmark/README.md format diff are flagged, not touched). On CI, ruff and both Lambda builds are green on dd5ed4d; the test (3.12) / test (3.13) legs were still running when this run ended — they were both green on the phase commit c733d7c, and the same suite passes locally at the fold head, but the checks are worth a glance before merge. If either leg came back red, the next routine run should clear waiting and work it.

Standing for review — "Questions for review" (1): the recorded time_range upper bound floors downward, so the stamp can under-claim the true last observation by up to 1 s. No in-tree consumer prunes on it (verified — every use is a union feed or the sweep.py:234 union compare), so nothing is broken today, but an external decoder that prunes on the stamp could miss data. Three arms are laid out with the cost asymmetry the review surfaced: rendering the envelope outward changes stamp bytes and therefore pulls docs/specification.md plus the conformance fixtures into the same PR under §4, while documenting the caveat does not. I have not picked one — that is a ruling, not a fix.

One process note, flagged rather than guessed. CLAUDE.md §2/§5 say a PR whose phases are complete and whose findings are folded should be flipped draft→ready-for-review alongside waiting, and names that flip as the one exception to the @espg-only list. This routine's own prompt says the opposite — "never mark a PR ready-for-review, drafts only" — in three separate places. That is a genuine conflict, so per the standing instruction I stopped rather than guessed and took the reversible option: the PR stays draft with waiting applied. Flip it with gh pr ready if the CLAUDE.md rule is the one that should win, or amend the routine prompt if the draft-only rule is.


Generated by Claude Code

@espg espg added the waiting label Aug 5, 2026
@espg

espg commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

CI finished after the status note above: fully green on dd5ed4dtest (3.12) and test (3.13) both pass, alongside ruff and both Lambda builds. The caveat in that note about the test legs still running is resolved; nothing needs a second look on the checks.


Generated by Claude Code

@espg espg removed the waiting label Aug 8, 2026
@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Added issue #406 to this bundle as ae408ea — swapped the four in-tree mortie.tools imports (src/zagg/grids/healpix.py ×2, demo/05_california_read.ipynb, notebooks/aoi_mask.ipynb) to mortie's flat exports, +4/−4 with no behaviour change; waiting was cleared to land it and the body now carries Closes #406 plus two corrections to the site list as filed (see the new section).

@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Replying to the adversarial review's "On question (1) — the t_max narrowing" note (it rode the review body rather than an inline thread, so this is the thread for it).

Ruled (b): document the ≤ 1 s narrowing as a reader caveat and keep the stamp bytes stable. Landed in ce7d033 — docs-only, no rendering code, no stamp bytes, no test expectations touched (docs/hive_layout.md | 24 +++- is the whole diff).

The review's cost asymmetry is what decided it, and it is now in the PR body rather than only on the thread: (a) render-outward and (c) sub-second precision both change the bytes of a stamp field external decoders read, which pulls them inside CLAUDE.md §4's normative-store-contract rule (docs/specification.md and the tools/generate_spec_fixtures.pytests/data/spec/ fixtures in the same PR); (b) does not.

What landed, in The commit stamp, immediately after the sentence defining time_range as "the actual [t_min, t_max] written":

Reader caveat — t_max floors, so the recorded range can end up to 1 s early. Both ends render through windows.iso_utc's whole-second granularity (isoformat(timespec="seconds")), so each truncates to the second containing it. On t_min that is harmless: the recorded start is at or before the true first observation. On t_max it is not — a last observation at 12:00:00.7 is recorded as 12:00:00, so the closed [t_min, t_max] is a slight under-estimate of the extent written, not an envelope around it. A consumer that prunes leaves on the stamp must treat t_max as inclusive with 1 s of slack (test the query against t_max + 1 s) […]

It also records both reach paths the review established — the float delta_time column on the point path and the millisecond-bearing STAC datetime on the raster path — and that no in-tree consumer prunes on the field today, which was the review's own verified finding. The Time windows D15 bullet gains a one-clause pointer to it rather than a second copy.

No fixture regeneration was owed, and I verified that rather than presuming it (same discipline as the review's own §4 spec check): grep -c time_range docs/specification.md → 0, grep -c time_range tools/generate_spec_fixtures.py → 0, grep -rl time_range tests/data/spec/ → no files, and no spec marker appears in the diff. A clarifying sentence about behavior that already ships is not a wire format, an attrs grammar, or a versioned marker.

One thing did not land, and it is worth a look. The review scoped (b) to docs/hive_layout.md on the grounds that it "is not the normative contract" — right about the file, but the hazard is aimed squarely at "a consumer that prunes on the stamp — an external reader decoding from the spec, per CLAUDE.md §4", and that reader never opens hive_layout.md. That is the same failure mode #404 records. So the intent was a short normative sentence in docs/specification.md too — and there is nowhere to put it: time_range is not described on that page at all (grep -c → 0; likewise t_min/t_max/morton-hive/2, and the only "commit stamp" mentions say the stamp is written last, never what it contains). mortie's page, which owns morton-hive/N, describes /2 leaf naming (§6.3) and the stamp's coverage envelope (§7.1) but not the D15 time_range either — its only mention of the name is §7.3, an informative carrier on the root coverage MOC, a different object.

So the leaf stamp's window/time_range pair appears to have no normative description on either page: an external decoder learns the field exists only from a narrative page that is explicitly not the contract. I stopped rather than inventing a section in the normative page to hang one sentence on, and filed nothing (§6). Written up as question (2) in the body with the three ways it could be closed.

@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Issue #406 has moved to PR #411 — no action needed here, and nothing on this branch was touched.

Why: this PR closes #390, whose design is now entangled with #410 (temporal/timespan representation), so it will not merge soon. #406 is an unrelated mechanical import swap that is blocking espg/mortie#169 (it moves mort2polygon/mort2geo into mortie/convert.py and cannot merge while any mortie.tools consumer is live), so it needed a path that does not wait on a temporal-design decision.

PR #411 cherry-picks ae408ea from this branch (authorship preserved, clean apply) and adds the two data/ sites this branch structurally cannot reach — it forked at 196867b, before PR #395 merged and added data/ to the repo.

Consequence for this PR: once #411 merges, ae408ea becomes a no-op here and its diff for those four files should simply drop out on rebase. This PR keeps #390 as its scope. Re-scoping it is not mine to do — flagging only.

Comment thread src/zagg/config.py
# instead of the value they wrote. The drop is reported in microseconds
# because that is exactly what iso_utc discards and nothing else: an integer,
# so no scientific notation for the 1 µs case, and never negative.
rendered_epoch = _windows.iso_utc(parsed_epoch)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

non-blocking — the merge brought in a second consumer of the rendered windowing epoch that runs before this guard, and it turns a sub-second epoch into a false "two different clocks" diagnostic.

_validate_time_source (added by #473, now config.py:1010) is called from validate_config at config.py:408, while _validate_windowing — and therefore this guard — is called at config.py:537. The cross-check inside it compares the declared output.time_source.epoch against get_windowing(config)["epoch"], i.e. the value already truncated by iso_utc:

if parse_utc(block["epoch"]) != parse_utc(windowing["epoch"]):
    disagree.append(("epoch", block["epoch"], windowing["epoch"]))

So a config that declares both blocks with the same sub-second epoch is refused by the wrong check, with a message that is factually untrue. Repro on 2d30bef1 (hive + healpix, schedule: yearly, scale: gps, both epochs literally "2018-01-01T00:00:00.5Z"):

output.time_source disagrees with output.windowing
  (epoch: 2018-01-01T00:00:00.5Z vs 2018-01-01T00:00:00+00:00)
  — window routing and the spec §8.3 toc words would run on two different clocks …

The two declarations are byte-identical; the only thing that "disagrees" is one side having been rendered. Dropping output.time_source from the same config yields the correct #390 message, so the guard is reachable — it is just preempted here, which is precisely the failure mode #390 exists to name.

This narrows the PR body's merge note. It reasons about time_source only in the two directions it is consumed ("full parsed precision" / "absent-block fallback consumes get_windowing's already-guarded output") and concludes "No relocation was needed". There is a third direction — _validate_time_source compares against the rendered epoch, ahead of the guard — and on that one the conclusion does not hold.

Fail-closed either way, so not blocking. Two cheap fixes if you want the right message: move _validate_windowing(config) ahead of _validate_time_source(config) in validate_config (the ordering comments at :397-408 give no reason it must precede), or have the cross-check compare parse_utc(block["epoch"]) against parse_utc(windowing_block["epoch"]) off the raw block rather than the normalized dict. Leaving it as-is is also defensible — but then it is worth a line in the merge note saying so, since the note currently reads as an exhaustive sweep of the #410 epoch surface.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Accepted — fixed in 12039a3b, via your second option (compare declared against declared) rather than the reorder.

The reorder was the wrong lever here on inspection: _validate_time_source is called at config.py:408, above the pipeline-kind branch, so it runs on every kind, while _validate_windowing runs inside the kind branches (:537 spatial, :1215 raster). Hoisting the windowing validator to sit ahead of it would start applying windowing checks to temporal/event pipelines that today return early at :411 — a scope change well past this finding.

So the cross-check now reads the epoch off the raw block:

declared_epoch = config.output["windowing"]["epoch"]
if parse_utc(block["epoch"]) != parse_utc(declared_epoch):
    disagree.append(("epoch", block["epoch"], declared_epoch))

Reaching that line means get_windowing took its point branch and parsed a declared epoch off the block, so the key is present and parses — the raster branch fixes scale to utc, which returns at the TOC_SOURCE_SCALES check above, and a malformed block returns from the try. The comparison is still between two parsed instants, so the purpose you flagged (both blocks on one clock for routing and word-encoding) is untouched; the only change is which spelling of the windowing side it compares and quotes. It is strictly tighter, too: a whole-second time_source epoch against a sub-second windowing epoch previously compared equal (the rendering truncated the difference away) and now reports the real disagreement.

Pinned in tests/test_config.py::TestTimeSource::test_an_agreeing_sub_second_pair_gets_the_sub_second_refusal — your byte-identical "2018-01-01T00:00:00.5Z"-in-both-blocks config, asserting the #390 carries sub-second precision message. It fails on the parent commit with exactly the message you quoted. The second half of the test pins that a genuine epoch disagreement still raises and now quotes the declared 2018-01-01T00:00:00.5Z rather than the rendered form.

On the merge note: you are right that "No relocation was needed" no longer holds as written — no relocation, but the cross-check did need adjusting. Flagged to the parent for a body patch.

uv run ruff check src tests / ruff format --check show only main's two standing items (N818 registry.py:64, the tests/data/benchmark/README.md format diff); tests/test_hive_windows.py tests/test_config.py green at 499 passed.

Comment thread docs/hive_layout.md
millisecond-bearing STAC `datetime` on the raster path
(`processing/raster._us_iso`). The narrowing is recorded rather than fixed
(ruled on [PR #398](https://github.com/englacial/zagg/pull/398)): no in-tree
consumer prunes on `time_range` — every use is a union into the regenerable

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

nit — the enumeration is one use short on the post-#410 tree.

"every use is a union into the regenerable root coverage.moc summary or the overview rollup" misses MocFamily.finish in src/zagg/sweep.py:340-342, which uses time_range as a skip-if-current predicate, not a union into a summary:

covered = covered and (
    union_time_range(existing.get("time_range"), time_range)
    == existing.get("time_range")
)

That gates whether the root coverage object is rewritten at all. It is not pruning and it is not a defect — both sides of the comparison are floored by the same renderer, so a rerun whose true t_max advanced by < 1 s produces an identical recorded value and there is genuinely nothing to rewrite. But the sentence is written as an exhaustive survey ("every use is …"), and it is the load-bearing half of the "recorded rather than fixed" ruling, so a reader auditing the claim will find a third call site and have to re-derive that it is harmless.

Suggest widening it — e.g. "no in-tree consumer prunes on time_range: every use is a union into the regenerable root coverage.moc summary or the overview rollup, or a skip-if-current comparison between two identically-rendered values (sweep.MocFamily.finish)". The rest of the caveat checks out against the merged tree: windows.iso_time_range -> iso_utc -> isoformat(timespec="seconds") and raster._us_iso's int(us) // 1_000_000 both floor to the containing second, the #the-commit-stamp anchor resolves (docs/hive_layout.md:591), and the caveat reads coherently where main's +643 lines left it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Accepted — fixed in bcc95d23, essentially your suggested wording. The sentence now reads:

no in-tree consumer prunes on time_range — every use is a union into the regenerable root coverage.moc summary or the overview rollup, or a skip-if-current comparison between two identically-rendered values (sweep.MocFamily.finish, which rewrites the root object only when the union widens what is already recorded) — and rendering the range outward instead would change the bytes of a stamp field external readers decode.

I added the parenthetical on what the predicate gates because that is the half that makes it self-evidently harmless to a reader auditing the claim: both sides of union_time_range(existing.get("time_range"), time_range) == existing.get("time_range") (src/zagg/sweep.py:340-342) come through the same renderer, so a rerun whose true t_max advanced by < 1 s compares equal and there is genuinely nothing to rewrite — no pruning, no data skipped.

Staged alone; docs-only, no test impact.

@espg

espg commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Merged main into the branch and the PR is conflict-free again — espg ruled it merge-ready pending this update (in-session, 2026-08-24). Merge commit: 2d30bef1 (regular merge, no rebase, no force-push).

Conflict inventory. One conflicted file, one hunk: src/zagg/config.py's module imports — this branch adds timezone (for _UNIX_EPOCH), main adds asdict (dataclasses). Resolved as the union. Everything else auto-merged, and main's side won wholesale for what PR #411 already landed: the former #406 entry's commit ae408ea dropped out exactly as predicted in the hand-off comment — after the merge, git diff origin/main touches only src/zagg/config.py, tests/test_hive_windows.py, and docs/hive_layout.md (the two #390 guards + the reader caveat), and grep -rn "mortie.tools" src demo notebooks is empty.

Guard re-verified on post-#410 main (the series #456/#463/#473/#479/#481/#507 merged while this PR was parked; config.py's windowing neighborhood moved):

  • get_windowing is still the only windowing-epoch render site — point branch iso_utc(parse_utc(block["epoch"])) at config.py:3613, raster branch iso_utc(_UNIX_EPOCH) at :3605; no other iso_utc epoch render in src/zagg.
  • The round-trip refusal guards it from _validate_windowing (:1454), reached via validate_config (:537) and the raster-resolve path (:1215); the schedule: none early return and the raster knob rejection still precede it, so both carve-outs hold. Refusal message unchanged.
  • The Per-centroid temporal companion for t-digests: a 64-bit hierarchical time cell, mirroring the spatial location companion #410 series added a second epoch-bearing block (output.time_source, validate_config: refuse a temporal companion without a resolvable clock at submission (issue #472) #473's _validate_time_source next door) — verified it does not need this guard: its epoch is consumed at full parsed precision (time_axis.observation_words computes in ns; nothing truncates), and the absent-block fallback consumes get_windowing's already-guarded rendered epoch. No relocation needed.
  • The docs caveat's "no in-tree consumer prunes on time_range" claim was re-checked against the new modules (sweep_overview.py, coverage.py, sweep.py, client.py): every consumer is still a union_time_range into a regenerable summary.

Local state at 2d30bef1: full pytest 4,692 passed / 38 skipped / 2 failed in 298 s — both failures pre-existing and already flagged in the Testing section (test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds, env-dependent; the flaky test_client_transport.py::TestStatusPoller poller test); ruff check / ruff format --check show only the two pre-existing main-side items (N818 at src/zagg/registry.py:64, tests/data/benchmark/README.md); the 14 parametrized #390 guard tests pass.

PR body updated: Closes #406 removed (issue closed via PR #411, merged 2026-08-08), the checklist entry struck-and-annotated rather than erased, and a "Merge with main (2026-08-24)" section records the resolution and re-verification.

Adversarial review + fold (this round): the fresh-context review over the merge-resolution diff verified the import union complete and the surviving payload exactly the intended three files, and posted two findings — both folded:

  • 12039a3b_validate_time_source's epoch cross-check compared the declared time_source epoch against get_windowing's already-rendered one and runs ahead of _validate_windowing, so a both-blocks byte-identical sub-second epoch drew a false "disagrees" refusal that masked the windowing.epoch: validate the rendered value (sub-second epoch silently shifts every window conversion) #390 message. Now compares declared-vs-declared parsed instants (also strictly tighter — a real sub-second disagreement no longer truncates away); pinned by test_an_agreeing_sub_second_pair_gets_the_sub_second_refusal.
  • bcc95d23 — the docs caveat's consumer survey now names sweep.MocFamily.finish's skip-if-current predicate instead of overclaiming "every use is a union".

CI: green end-to-end on both the merge commit 2d30bef1 and the head bcc95d23 (ruff, test 3.12/3.13, build x86_64/arm64). Head sha: bcc95d23b2dabbe45fb3b96520668dbc52f57afd. Marking ready for review; waiting stays on — the ball is with review/merge.

@espg
espg marked this pull request as ready for review August 24, 2026 17:43
@espg
espg merged commit 8233c4c into main Aug 24, 2026
8 checks passed
@espg
espg deleted the claude/small-fixes-2026-08-05 branch August 24, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

windowing.epoch: validate the rendered value (sub-second epoch silently shifts every window conversion)

2 participants