gap-preserving per-shard temporal word-set cover (coverage.toc) - #507
Conversation
| f"coverage[toc]: keeping the standing {preserved.get('spec')!r} cover at " | ||
| f"{store_root} — {COVER_SPEC} does not read it and MUST NOT downgrade it" | ||
| ) | ||
| merged = preserved if preserved is not None else dict(section) |
There was a problem hiding this comment.
🤖 from Claude (review)
[bug] write_cover(root, None, replace=True) raises TypeError: 'NoneType' object is not iterable — dict(section) on None.
build_cover_section returns None for an empty walk (its own docstring: "the standing absence rule, so a store with no temporal channel gets no sibling object at all"), and replace=True is documented as "the refresh escape hatch" — i.e. exactly refresh_root_coverage, which today calls build_temporal_section(...) unconditionally and will call build_cover_section(...) the same way in phase 2. A refresh of a store with no temporal channel therefore crashes the escape hatch. Reproduced on this commit:
>>> write_cover(tmpdir, None, replace=True)
TypeError: 'NoneType' object is not iterable
The non-replace arm already handles it gracefully (merge_cover_sections(existing, None) → dict(a) / _cover_preserved / None), so the two arms disagree on the same input. §10.5 also pins the intended behavior: "a producer with no temporal contribution leaves the standing object untouched" — so replace=True with no section must be a no-op PUT, not an overwrite and not a crash.
Suggestion: an early guard before the replace branch —
if section is None:
return existing if isinstance(existing, dict) else None # nothing to say; leave it standingand a test asserting write_cover(root, None, replace=True) leaves a standing object byte-identical.
There was a problem hiding this comment.
🤖 from Claude
Fixed in c6f5756. write_cover now short-circuits a None section before either arm: it returns the standing object (or None on an empty root) and skips the PUT entirely, so the refresh escape hatch over a temporal-free store is a no-op instead of dict(None). Signature is section: dict | None, and the docstring states the §10.5 rule ("a producer with no temporal contribution leaves the standing object untouched") on both paths.
Two tests in TestCoverObject: test_no_contribution_leaves_the_standing_object_untouched asserts the file bytes are byte-identical after write_cover(root, None, replace=True) and after the default arm; test_no_contribution_on_an_empty_root_writes_nothing asserts None back and no object created.
|
|
||
| store = open_object_store(store_root, **store_kwargs) | ||
| try: | ||
| existing = _read_json(store, COVER_NAME) |
There was a problem hiding this comment.
🤖 from Claude (review)
[bug] The regenerable-cache posture only covers unparsable JSON, not an unparsable cover. A standing coverage.toc that is valid JSON but semantically broken kills the writer instead of being replaced.
try/except ValueError here wraps only _read_json. Everything downstream — merge_cover_sections → _decode_cover_block — runs unguarded on attacker-free-but-corrupt standing bytes, and every failure mode escapes write_cover. All four reproduced on this commit against a standing object built by build_cover_section and then damaged:
| damage | escapes as |
|---|---|
count bumped by one |
ValueError: cover shard 11210 declares 9 words and decodes 8 … |
words key deleted |
KeyError: 'words' |
words set to "junk" |
ValueError: buffer size must be a multiple of element size |
| block replaced by a string | ValueError: cover shard … block is not an object |
That contradicts both this function's own docstring ("An unparsable standing object is logged and overwritten, the regenerable-cache posture") and spec §10.5's composition rule ("an unmarked or unparsable one is debris a producer replaces"). It also matters operationally: in phase 2 this runs inside MocFamily.finish, so one corrupt byte in a regenerable accelerator takes down the spatial rollup the sweep exists for — the exact failure mode write_root_coverage guards against with its except (KeyError, TypeError, ValueError) around the union.
cover_unchanged has the same hole (it calls merge_cover_sections), so the skip-if-current test raises too, before the writer ever gets a chance.
Suggestion: make the standing side fail-open. Either validate in _cover_usable (decode every block, return None + a warning on the first failure, which then routes through the existing "debris is replaced" arm), or wrap the merge:
try:
merged = merge_cover_sections(existing, section)
except (KeyError, TypeError, ValueError, binascii.Error) as e:
logger.warning(f"standing {COVER_NAME} at {store_root} failed to parse ({e}); overwriting")
merged = dict(section)plus tests for each of the four rows above (the count-mismatch MUST-check should stay loud on the read accessor — cover_words — and be fail-open on the write seam).
There was a problem hiding this comment.
🤖 from Claude
Fixed in cddfac8. Both seams now fail open on a standing object that is JSON but not a decodable cover:
write_coverwraps the merge inexcept (KeyError, TypeError, ValueError), logs withwrite_root_coverage's wording (failed to parse (…); overwriting (regenerable cache — the sweep rebuilds authoritatively)), and writesdict(section).binascii.Erroris aValueErrorsubclass, so the bad-base64 row is covered.cover_unchangedcatches the same three and returnsFalse, so the skip-if-current test forces the rewrite instead of raising ahead of the writer.
Docstrings on both now say "unparsable" includes the semantic case, and §10.5's composition paragraph says the same normatively (48eb82e carries that spec sentence).
Tests: test_a_corrupt_standing_cover_is_replaced parametrized over your four damage modes (count bumped, words deleted, words = "junk", block replaced by a string) — each asserts cover_unchanged is False, the warning fired, and the standing shard was replaced by the incoming one. Per your note the read accessor stays loud: test_the_read_accessor_stays_loud_on_a_corrupt_block plus the existing test_a_block_whose_count_disagrees_is_refused.
|
|
||
| **Widening only.** The output's coverage contains the input's — a cover | ||
| may over-claim, never false-negative — and the never-bridge law holds at | ||
| bucket resolution: a real gap of at least one bucket survives exactly, |
There was a problem hiding this comment.
🤖 from Claude (review)
[spec] The gap-survival law is stated wrongly here and incompletely in §10.5, and the real guaranteed floor is two buckets (≈78 h ≈ 3.26 days), not one bucket / "≥ 1 day".
This docstring says "a real gap of at least one bucket survives exactly". That is false. A gap of length ≥ 1 bucket that does not contain a whole aligned bucket lands its two endpoints in adjacent buckets, whose quantized ranges abut, and toc_normalize coalesces abutting ranges by design ("Range words coalesce iff their decoded half-open envelopes overlap or abut exactly"). Measured on this commit at the pinned order 16 (B = 2^47 ns), two instants separated by gap, with the first at fraction frac into its bucket:
frac=0.9 gap=1.00B -> 1 word, gap CLOSED
frac=0.9 gap=1.05B -> 1 word, gap CLOSED
frac=0.0 gap=1.00B -> 1 word, gap CLOSED
frac=0.9 gap=1.90B -> 2 words, gap survives
frac=0.9 gap=2.00B -> 2 words, gap survives
Spec §10.5's bullet (line 2588) is technically right — "a real gap spanning at least one whole aligned bucket survives" — but it pairs that with "a gap shorter than a bucket may close", which invites the contrapositive reading "≥ 1 bucket survives", the same false claim. And the [1B, 2B) band is alignment-dependent, i.e. data-dependent, which is not something a consumer can reason about.
Why it matters: this is the only resolution promise the cover makes, and the first consumer (closest-observation S2 ingest, with a max_time_offset) prunes on it. The §10.5 derivation sells order 16 as "the finest order whose span is at least one day" — a reader will take away a ~1-day gap floor, when the guaranteed floor is ~3.26 days and anything between 1.63 and 3.26 days is a coin flip on alignment.
Suggestion, three edits:
- Fix this docstring to the exact law: a gap survives iff it contains a whole aligned bucket; gaps ≥ 2 buckets always survive, gaps < 1 bucket never do, and the band between is alignment-dependent.
- Say the same in §10.5's bullet, and state the guaranteed floor in wall-clock terms (2·2^47 ns ≈ 78.2 h) right next to the "≥ 1 day" derivation at line 2573, so the pin's justification and its promise are not two different numbers.
- Pin it:
test_gaps_of_a_bucket_or_more_surviveonly exercises ≥ 3-day gaps (≈1.84 B), so nothing in the suite touches the interesting band. Add the table above as a parametrized test — including at least onegap == 1.0Bcase asserting the gap does close, so the floor is committed as bytes rather than as prose.
There was a problem hiding this comment.
🤖 from Claude
Fixed as text + tests in c104854; the pinned day order 16 stays as ruled, only the claims about it change.
quantize_wordsdocstring now states the law exactly: a gap survives iff it contains a whole ALIGNED bucket, because widening puts the two endpoints in their own buckets andtoc_normalizecoalesces abutting ranges — so a one-span gap never survives, and the guaranteed floor is two spans (2 × 2^47 ns ≈ 78 h).- §10.5: the bullet now gives the iff plus the three regimes (≤ 1 span always closes, ≥ 2 spans always survives, the
[1, 2)band is alignment- i.e. data-dependent, "a consumer MUST NOT reason on it"), and the order-16 derivation paragraph now carries the floor beside the span so the justification and the promise are the same number:2 × 2^47ns ≈ 78.2 h ≈ 3.26 days. test_a_gap_survives_iff_it_holds_a_whole_aligned_bucketparametrizes your table at order 16, pinning both directions:(frac 0.9, 1.0B)and(frac 0.0, 1.0B)close,(frac 0.0, 1.99B)closes,(frac 0.5, 1.5B)survives,(0.0, 2.0B)and(0.9, 2.0B)survive — with a per-endpointtoc_overlapsassertion so no case can pass by losing coverage.
Also renamed test_gaps_of_a_bucket_or_more_survive → test_days_far_from_any_pass_stay_uncovered: its old name asserted the claim being corrected here.
| "spec": COVER_SPEC, | ||
| "source": b.get("source", a.get("source")), | ||
| "generated_at": b.get("generated_at", a.get("generated_at")), | ||
| "order": b.get("order", a.get("order")), |
There was a problem hiding this comment.
🤖 from Claude (review)
[bug] The cover's seam has no shard-order compatibility gate, so it composes across a re-shard where the carrier deliberately does not.
merge_cover_sections takes order from whichever side happens to be incoming and unions the two shards key sets regardless. write_root_coverage guards exactly this case:
compatible = (
existing.get("spec") == envelope.get("spec")
and existing.get("encoding") == envelope.get("encoding")
and existing.get("order") == envelope.get("order")
)and on an incompatible existing object it overwrites, which also drops the stale §10 section (carried = isinstance(existing, dict) and merged is not envelope → merge_temporal_sections(None, …)). So after a re-shard at a different parent_order, coverage.moc correctly lists only the new-order shards, while the sibling coverage.toc silently ends up holding D1 decimals at two orders under a single declared order — ids that are not even comparable, and a shards_overlapping-style consumer (phase 3) will key against the carrier's order and get a mix of hits and phantom shards.
Suggestion: mirror the carrier's gate. When a.get("order") != b.get("order"), log and treat the standing object as incompatible debris — return dict(b) — rather than unioning. That keeps the two objects' order-change behavior identical, which is the property §10.5 leans on when it says composition is "the same seam as the carrier (§10.4)".
Related, worth one sentence in §10.5 while you are here: the spec says nothing about what a producer does with the sibling when the carrier is deleted (refresh_root_coverage deletes coverage.moc when no stamped leaf remains) or overwritten wholesale. Today that leaves an orphaned coverage.toc with no cover marker pointing at it. The direction is safe (over-claim → extra candidates), but phase 2 needs a stated answer rather than an accident.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 48eb82e. merge_cover_sections now gates on shard order before the union, mirroring write_root_coverage's incompatible-envelope arm: when both sides are usable but declare different order, it logs (… ids at two orders are not comparable, regenerable-cache posture) and returns dict(b) — the incoming side wins wholesale, so the sibling moves with the carrier across a re-shard instead of holding two orders' ids under one declared order.
Spec §10.5 composition gained both sentences you asked for:
- the order gate, spelled as the carrier's (§10.4) — differing
ordermeans the standing object is incompatible debris the incoming side REPLACES; - the orphan contract — a producer that overwrites or DELETES
coverage.mocwholesale SHOULD discardcoverage.tocwith it (thecovermarker lives in the carrier, so a survivor is undiscoverable), and a reader that reaches an orphan anyway is safe because a cover shard absent from the carrier map composes under the standing staleness posture: unknown, a candidate, never authoritative. Phase 2 wires the writer against that stated contract.
Test: test_a_standing_cover_at_another_shard_order_is_replaced (order 4 standing vs order 5 incoming → incoming wins, warning fired) with the same-order case asserted alongside so the gate is the only behavior change.
| f"cover shard {decimal} declares {block.get('count')!r} words and decodes " | ||
| f"{len(words)} — the block must agree with its own buffer (spec §10.5)" | ||
| ) | ||
| return words, int(block.get("temporal_order", TEMPORAL_DAY_ORDER)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[spec] The object's declared temporal_order (and cap) are decorative: the decoder defaults to the module constant, never to what the object says.
_decode_cover_block resolves a missing per-shard temporal_order to TEMPORAL_DAY_ORDER, but §10.5 defines the fallback relative to the object: "temporal_order (optional) — present iff this shard coarsened below the object's pinned order; absence means the pinned order. Always ≤ the object's temporal_order." Nothing reads section["temporal_order"] anywhere in the module. Three consequences:
- A standing object whose pinned order is not 16 has every unmarked block decoded as order 16.
merge_cover_sectionsthen computesorder = min(o for _, o in sides)from those wrong values, requantizes at the wrong order, and writes the result out stamped"temporal_order": TEMPORAL_DAY_ORDER— i.e. the merge both mis-reads and re-labels the pin. Today §10.5 pins /1 producers at 16 so no conforming producer trips this, but that makes the required field pure decoration, and it is the one field that says how to interpret the buffers. _cover_contentomits object-leveltemporal_orderandcapentirely, socover_unchangedreports "unchanged" for a standing object at a different pin whose per-shard blocks happen to re-encode identically — the skip-if-current test would then never rewrite it.- Nothing validates the spec's "Always ≤ the object's
temporal_order"; a block declaring 20 is accepted and quantized at 20.
Suggestion: thread the object's pin through — _decode_cover_block(decimal, block, pinned) with pinned = int(section.get("temporal_order", TEMPORAL_DAY_ORDER)), refuse (or clamp, loudly) a block order above the pin, and add section.get("temporal_order") / section.get("cap") to _cover_content. Then a test with a hand-built order-14 object asserting cover_words decodes it at 14 and that merging it with a 16 object requantizes at 14.
There was a problem hiding this comment.
🤖 from Claude
Fixed in b7f6447 — the declared fields are now load-bearing on every path.
- New
_object_pin(section)resolves the object'stemporal_order(module constant only when the object omits it)._decode_cover_block(decimal, block, pinned)takes it as the per-shard default and as the ceiling: a block declaring an order above the object's pin raisesValueError(§10.5's "always ≤ the object'stemporal_order"). _encode_cover_block(cover, order, pinned)emits the per-shard key ifforder != pinned, so the key and the object's declaration are written against the same number.merge_cover_sectionsdecodes each side against its own declared pin, takes the per-shardminfrom those, and pins the composed object atmax(pin_a, pin_b)— the finer of the two is the only value every surviving block's order is still ≤, so the invariant holds after the merge; blocks that landed coarser record it explicitly.cover_wordspasses the object's pin through;_cover_contentnow carries object-leveltemporal_orderandcap, so a pin or cap change is a content change the skip-if-current test sees.- §10.5's
temporal_orderbullet now says absence means the object's order and that a reader MUST refuse a block above it.
Tests: test_a_block_decodes_at_the_objects_pin_not_the_modules (hand-built order-14 object with unmarked blocks decodes at 14), test_merging_an_object_at_a_coarser_pin_requantizes_there (14 × 16 → requantized at 14, composed pin 16, block records 14, plus the parity assertion), test_a_block_above_the_objects_pin_is_refused, test_a_pin_change_is_a_content_change (repin and recap each flip cover_unchanged).
| input behind a shard FAILED omits that shard (§10.2's whole-word rule); a | ||
| producer with no temporal contribution leaves the standing object untouched. | ||
|
|
||
| Conformance is §7's `temporal/` fixture again: it is the only fixture |
There was a problem hiding this comment.
🤖 from Claude (review)
[spec] This conformance paragraph asserts committed bytes that do not exist. As of 495a00f it is a false statement about the repository, and §340's rule ("any change to a wire format … must update that page and the conformance fixtures in the same PR — external readers decode from the spec + fixtures alone") is not yet satisfied.
Checked on this commit:
$ ls tests/data/spec/temporal
1 coverage.moc morton_hive.json # no coverage.toc
$ grep -c cover tools/generate_spec_fixtures.py
0 # generator untouched
$ jq 'has("cover")' tests/data/spec/temporal.expected.json
false
So all three claims in this paragraph are currently unbacked: "it is the only fixture carrying a coverage.toc", "temporal.expected.json's cover block records the decoded word set", and "The other six fixtures carry no coverage.toc at all, which pins the absence rule as bytes". §7's own temporal/ bullet (line 1593) is also un-updated — it still describes the fixture as the companion surface plus the root coverage.moc, with no mention of the sibling, and the generator's header comment at tools/generate_spec_fixtures.py:86 still says temporal/ is "the only fixture carrying a root coverage.moc" with nothing about coverage.toc.
This is the plan's phase 1 deliverable ("Spec §10 grammar + conformance fixtures + read-side accessors"), so the fixtures were meant to land in this commit rather than later. Either way it has to be closed before the PR goes ready.
Suggestion: land the generator change plus regenerated tests/data/spec/temporal/coverage.toc and the cover block in temporal.expected.json in the next phase commit — written through the production writer (write_cover), the way the coverage.moc fixture already is, and with the expected word set derived through the quantization law rather than transcribed, exactly as this paragraph promises. Until those bytes exist, this paragraph and §7's bullet should not be phrased in the present tense.
There was a problem hiding this comment.
🤖 from Claude
Left standing by design — the fixture work rides phase 2 deliberately, not as an oversight.
The paragraph promises bytes written through the production sweep writer ("written by the production sweep writer beside its coverage.moc"), the way the coverage.moc fixture already is. That writer wiring — MocFamily.finish calling build_cover_section / write_cover — is phase 2 of this PR, so generating the fixture in phase 1 would mean transcribing it from a test harness rather than from the producer, which is exactly what the paragraph forbids. The re-phasing is recorded in the PR body's checklist: the generator change, tests/data/spec/temporal/coverage.toc, the cover block in temporal.expected.json, §7's temporal/ bullet, and the generator header comment at tools/generate_spec_fixtures.py:86 all land in the phase-2 commit, same PR, so the #340 rule is satisfied at the PR level as written ("must update that page and the conformance fixtures in the same PR").
Your point that the present tense is unbacked until then is taken and is the reason it is a phase-2 gate rather than a follow-up issue: the PR does not go ready with the paragraph and the tree disagreeing.
| assert block["count"] <= COVER_CAP | ||
| assert "coarsened" in caplog.text | ||
| # Widening only: every original instant is still covered. | ||
| assert np.all(toc_overlaps(cover_words(section)["11213"], int(ts[0]), int(ts[-1]) + 1)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] This is the only test that exercises the cap, and its containment assertion is vacuous — it asserts nothing the cap could break.
assert np.all(toc_overlaps(cover_words(section)["11213"], int(ts[0]), int(ts[-1]) + 1))ts[0] … ts[-1] is the entire span of the input, so every word in any cover of that input overlaps it by construction. The assertion passes for a cover that dropped 90% of its words, for one built by truncation (the thing §10.5 explicitly forbids: "a producer MUST NOT truncate the word list instead, which would silently drop coverage"), and for the correct answer alike. The comment above it says "Widening only: every original instant is still covered" — which is the right claim, but not the one being made.
Two gaps behind it:
- Containment after coarsening is untested. Assert it the way
test_the_cover_contains_every_instantdoes — per instant,np.any(toc_overlaps(words, int(t), int(t) + 1))over the 600 inputs (or a stride of them) — so a truncating implementation fails. - The parity invariant is never checked on a capped or merged shard. §10.5 states it normatively at "the shard's effective order
o" and claims "zagg's suite asserts it on every shard it writes", buttest_parity_with_the_tier_one_maponly runs on_contributions([1, 5, 11]), where no shard ever coarsens, so the effective-order arm of the invariant is dead code in the suite. The two paths that produce an effective order ≠ 16 —_cap_coverhere, andmerge_cover_sections'sorder = min(...)requantize — are exactly the ones a parity test would protect. Add the parity assertion to this test atblock["temporal_order"], and totest_mixed_orders_requantize_at_the_coarser.
(I fuzzed the invariant itself over 3,000 random mixed timestamp/range word sets at order 16 and it held with zero failures, including the TOC_MAX_NS clamp arm — the commutation argument looks sound. The gap is that the suite does not pin it where it is hardest.)
There was a problem hiding this comment.
🤖 from Claude
Fixed in 2e196e5. The vacuous whole-span assertion is gone from test_the_cap_coarsens_by_order_and_records_it, replaced by three claims a truncating implementation fails:
- exact equality —
np.array_equal(cover_words(section)["11213"], quantize_words(cover, effective))at the block's recorded effective order (15), so the capped block IS the input re-quantized one order coarser, not a subset of it; - per-instant containment over a stride of the 600 inputs, the way
test_the_cover_contains_every_instantdoes it; - the parity invariant at the effective order:
toc_reduce(words) == toc_reduce(quantize_words([tier1], 15)), withtier1 = toc_reduce(cover).
test_mixed_orders_requantize_at_the_coarser gained the same parity assertion at the merged shard's effective order 14 (tier1 = toc_reduce([_leaf(1)[0], _leaf(2)[0]])), and the new order-14-pin merge test from the b7f6447 fold carries it too — so both paths that produce an effective order ≠ 16 (_cap_cover and the merge's min requantize) are now pinned, not just the never-coarsening _contributions([1, 5, 11]) case.
| word = int(toc_reduce(raw)) | ||
| digest, folded = merge_tdigests_kway(digests, delta=ROOT_TOC_DELTA, temporal=times) | ||
| return word, digest, folded | ||
| return word, digest, folded, quantize_words(raw) |
There was a problem hiding this comment.
🤖 from Claude (review)
[perf] Every moc sweep now pays a full-multiset quantization per leaf, unconditionally — including on stores that will never publish a cover — and the docstring's Cost paragraph (which is otherwise careful, down to the ~1.4 MB accumulator figure) was not updated for it.
Measured on this commit at the CA shard shape the plan cites (2.7 M words, 49 pass-days):
toc_reduce 0.002 s
quantize 0.105 s -> 45 words
~50× the step it sits beside. 0.1 s/leaf is not the wall on its own, but the transient footprint is the part worth a second look: quantize_words materializes start, end, lo, last, hi and the span2toc output as full-length uint64 arrays — ~6 × 21.6 MB ≈ 130 MB of peak on top of the raw concatenation that already exists — inside read_leaf_temporal, which the Lambda fleet runs at 2 GB with the digest fold live at the same time. Peak, not steady, so it will not show up in a small-store test.
Two things to consider:
- Compute it once from the array you already have, in place where possible:
lo/last/hican all be written into thestart/endbufferstoc2timereturned (np.right_shift(start, k, out=start)etc.) rather than allocating a new array per step. That is a cheap ~3× cut in peak. - The bigger question for phase 2: should the fold be gated at all? Today a store that declares temporal fields but whose operator only wants tier 1 still pays this on every leaf of every sweep. If the cover is always-on by design that is fine — but say so in the Cost paragraph, since it is now the dominant term of
read_leaf_temporal's per-leaf CPU after the array reads.
Either way the Cost paragraph above should name the new term; right now it describes a read that no longer exists.
[nit] while here: src/zagg/sweep.py:194 still documents the accumulator as {shard decimal: [(word, digest, times), ...]}. The tuple is a 4-tuple now, and that comment is the one place a reader looks to find out what MocFamily._temporal holds.
There was a problem hiding this comment.
🤖 from Claude
Docs fixed in e8934fe; the cost itself is accepted as designed.
Accept rationale. 0.105 s/leaf sits beside seconds of leaf array I/O — payload[:] plus its sibling per declared field is the wall, and the fold is a small constant on top of it. The alternative the two suggestions point at (skip or defer the per-leaf quantization, hand the accumulator the raw word multiset and quantize once at the root) is precisely the n_leaves × n_cells × k memory blowup this fold exists to avoid: a CA-scale shard's millions of words per leaf, held across every leaf of the walk, instead of the few dozen cover words the fold returns. The transient peak you measured is real but bounded by the raw concatenation that already exists in the same frame, and the fold is unconditional by design — the cover is an always-on accelerator, not an opt-in.
What changed. read_leaf_temporal's Cost paragraph now names the term with your measured shape: unconditional on every moc sweep including stores that never publish a cover, toc_reduce ~0.002 s vs quantize_words ~0.105 s at the CA shard shape (2.7 M words, 49 pass-days, ~45 words out), ~130 MB of transient uint64 temporaries, and the accept-with-rationale above. The returned-size paragraph now mentions the per-leaf cover words alongside the centroid figures.
[nit] src/zagg/sweep.py:194 corrected to {shard decimal: [(word, digest, times, cover), ...]} with the fourth element named as the §10.5 cover (issues #480, #489).
…s to an orphan (issue #489)
| # The §10.1 presence marker: this producer is about to PUT (or | ||
| # leave standing) a sibling at its own revision. A hint, per the | ||
| # spec — a reader falls back when the object is gone or foreign. | ||
| section[COVER_KEY] = COVER_SPEC |
There was a problem hiding this comment.
🤖 from Claude (review)
[bug] The marker is stamped unconditionally at COVER_SPEC, but write_cover (default arm, two lines below at sweep.py:358) may leave a foreign-revision sibling standing under §10.5's succession rule. The store then advertises a revision the object does not carry — §10.1 defines the value as "that object's spec string" — and, worse, the sweep and the refresh disagree about it forever: refresh_root_coverage stamps written_cover["spec"] (coverage.py:470), i.e. the truth.
Reproduced on the §7 temporal/ fixture (standing coverage.toc at zagg-coverage-toc-cover/9, section marker …/9):
sweep -> root_moc_written True | marker now: …-cover/1 | object spec: …-cover/9
sweep -> root_moc_written False (converged on the LIE)
refresh-> marker: …-cover/9 | object: …-cover/9
sweep -> root_moc_written True | marker: …-cover/1 (ping-pong)
So on any store a future producer has touched, alternating sweep/refresh re-PUTs coverage.moc on every pass — the exact churn the skip test exists to prevent — and a …/9-capable reader that trusts the marker skips a GET it could have served.
Stamping written_cover["spec"] after the PUT does not fix it on its own: the skip test compares the marker before the write, so the next pass would rewrite forever. The convergent fix is at the seam — make merge_temporal_sections treat an unknown-revision marker the way §10.5 already treats the unknown-revision object: don't let an incoming zagg-coverage-toc-cover/1 downgrade a standing …/9 marker. Then the standing …/9 survives the merge, section_unchanged is True, and the sweep skips instead of lying. That also needs §10.4's bullet ("the incoming section's value wins when both sides carry one") amended to carve out the unknown revision, so §10.1's definition of the value stays true.
Secondary, same path: when write_cover preserves a foreign object, out["cover_shards"] (sweep.py:374) still reports this pass's shard count even though none of this pass's cover landed.
There was a problem hiding this comment.
🤖 from Claude
Fixed at the merge seam, as suggested — a896a44f.
merge_temporal_sections now applies §10.4's succession rule to the marker itself: an incoming zagg-coverage-toc-cover/1 marker no longer overwrites a standing marker naming a revision this producer cannot read. That standing marker is the truth, because write_cover preserves the foreign OBJECT on both of its paths, so the store stops advertising a revision the object does not carry and the sweep/refresh ping-pong disappears. Incoming still wins for every other combination.
src/zagg/coverage_toc.py—marker = b.get(...), then the one carve-out.docs/specification.md§10.4 — the marker bullet now states the exception and why (§10.1 defines the value as the object's ownspec).tests/test_coverage_toc.py::TestOnCommittedStores::test_a_sweep_over_a_future_sibling_converges_instead_of_lying—/9object +/9marker on thetemporal/fixture, swept twice: both passesroot_moc_written False, marker and object still/9. It fails on the pre-fold code (verified by stashing the src change).
The secondary point — out["cover_shards"] reporting this pass's shard count when write_cover preserved a foreign object — is left standing: it is run-summary reporting rather than store state, and it was outside the fix this fold was directed to make. It is yours to call.
| and isinstance(written_cover, dict) | ||
| and isinstance(written_cover.get("spec"), str) | ||
| ): | ||
| section[COVER_KEY] = written_cover["spec"] |
There was a problem hiding this comment.
🤖 from Claude (review)
[bug] On the partial arm, section is merge_temporal_sections's output, which under §10.4's succession rule can be a standing section at an unknown revision, preserved verbatim ("a producer MUST copy an unreadable standing section through unchanged"). This line then writes a key into it.
Reproduced on the temporal/ fixture with the standing section re-marked zagg-coverage-toc/9, its cover key removed, and every leaf read failing:
preserved section spec: zagg-coverage-toc/9 | cover key injected: True -> zagg-coverage-toc-cover/1
The /9 producer may well define cover differently (or use it for something else entirely); "verbatim" is the whole point of the rule, and this is the one seam a mixed-version fleet meets. Guard the stamp on the section actually being this revision's, e.g.
if section is not None and section.get("spec") == TEMPORAL_COVERAGE_SPEC and isinstance(...):(or reuse coverage_toc._usable). Worth a test alongside test_an_unknown_revision_on_the_standing_side_is_preserved asserting the refresh leaves a /9 section byte-identical, marker included.
There was a problem hiding this comment.
🤖 from Claude
Fixed — 4f9b43f1.
refresh_root_coverage now stamps COVER_KEY only when the (possibly merged) section is at this revision:
if (
isinstance(section, dict)
and section.get("spec") == TEMPORAL_COVERAGE_SPEC
and isinstance(written_cover, dict)
and isinstance(written_cover.get("spec"), str)
):So a standing section preserved under §10.4 comes through untouched — verbatim means verbatim.
Test: tests/test_coverage_toc.py::TestOnCommittedStores::test_refresh_preserves_a_future_section_without_stamping_it — the temporal/ fixture re-marked zagg-coverage-toc/9 with its cover key removed and every leaf read failing; the rebuilt section compares equal to the standing one and carries no cover. Fails on the pre-fold code.
| every_word = np.concatenate( | ||
| [cell["h_tdigest"][2] for cells in by_chunk.values() for cell in cells.values()] | ||
| ).astype(np.uint64) | ||
| expect_cover = quantize_words(every_word) |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] The committed golden is degenerate, and it makes the §7/§10.5 conformance claims unfalsifiable for the external reader they exist for (moczarr#45).
The fixture's instants span ~30 min total (_obs_times_ns: TEMPORAL_STEP_S = 3, ≤ 4 cells × 97 steps + 300 steps), so at the pinned order 16 (≈ 39.1 h bucket) everything lands in ONE bucket. The committed coverage.toc is count: 1, and expect_cover == quantize_words([shard_word]) — the generator asserts exactly that at line 1226. Consequences in tests/test_spec_conformance.py:
test_the_cover_satisfies_the_parity_invariant:toc_reduceof a single word is that word, on both sides. Passes for any implementation.test_the_cover_conservatively_contains_every_member_instant: one bucket containing everything. Passes for any over-claiming cover.- A reader that implements §10.5 as "GET tier 1, quantize the one word" — i.e. that implements none of the gap preservation the whole section exists for — passes the entire suite byte-for-byte.
So the §7 paragraph this PR adds ("temporal.expected.json's cover block records the derived word set") is true but pins ~nothing of §10.5's normative content: no multi-word cover, no surviving gap, no cap coarsening, no block-level temporal_order, no multi-shard object. Also unasserted anywhere on committed bytes: the required cap, fields, element, encoding, and the per-block count MUST-check.
Concrete fix, cheap: give one cell a clock offset of ≥ 2 bucket spans (2 × 2^47 ns ≈ 78.2 h — the guaranteed floor §10.5 states) in _obs_times_ns, so the committed cover has ≥ 2 words with a gap that provably survives, and record the gap in temporal.expected.json (e.g. the uncovered [t0, t1) the suite then asserts toc_overlaps answers False for). That costs a fixture regeneration you are already doing in this commit, and turns both conformance tests into real discriminators. If instead the intent is that the gap law stays pinned only by the unit tests in TestQuantization, then §7 and §10.5's closing "Conformance is §7's temporal/ fixture" paragraph should say plainly what the fixture does and does not pin, so an external implementer does not read passing conformance as §10.5 conformance.
There was a problem hiding this comment.
🤖 from Claude
Fixed by making the fixture pin the real claims — dedfd4ca.
build_temporal now pushes the LAST cell of the plan (ordinal 3, the 300-observation cell) five whole days past the rest — TEMPORAL_GAP_DAYS, asserted in the generator to clear §10.5's two-bucket-span floor. _obs_times_ns itself is untouched, since other expectations ride it. The committed cover is now two words with a real hole, and the generator derives that hole from the inputs (_temporal_gap: widest gap between the input envelopes, pulled IN to the bucket grid) and asserts the committed cover does not overlap it, plus count >= 2.
temporal.expected.json's cover block gained cap, fields, element, encoding, count, and gap_ns. New/extended conformance tests in tests/test_spec_conformance.py:
test_the_cover_declares_the_10_5_grammar—cap == 512,fields,element,encoding, the block'scount, and the base64 buffer decoded by spec text alone (8 * countbytes, words equal to the expectation).test_the_cover_preserves_the_gap_between_the_clusters—toc_overlapsFalse acrossgap_ns, with a whole-scale window asserted True so the predicate is not answering False for everything.test_the_cover_sibling_decodes_to_the_expected_word_set— now also pinslen(words) == count >= 2.- parity and containment are unchanged in text and no longer vacuous.
Docs: §7's temporal/ paragraph and §10.5's conformance paragraph now say what the fixture pins, including the deliberate offset and gap_ns.
Re-pinned FROZEN literals (the array bytes moved), with the kitchen_sink authalic comment as the precedent:
| literal | old | new |
|---|---|---|
FROZEN_COMBINED["temporal"] |
fcfc8c33…3f81 |
6c7a0c5a…2887 |
FROZEN_ARRAYS[("temporal","6/observed")] |
bf3b7e75…3d04 |
b8c48a74…4402 |
FROZEN_ARRAYS[("temporal","6/h_tdigest_times")] |
84b51ac5…1488 |
03a1e44b…e468 |
6/h_tdigest and 6/h_tdigest_locations did NOT change — git reports those chunk files unmodified and their hashes in expected.json are byte-identical, as required (values and rng untouched).
One thing worth flagging: my first attempt offset by 3 × 2^47 ns exactly, which broke test_the_production_kernel_reproduces_the_committed_words by 2 ns on one centroid — that test drives the same instants through a float64-seconds axis (observation_words((times_ns - epoch_ns) / 1e9, …)), whose ulp at this magnitude is ~15 ns. A whole-DAY offset keeps the clock on the same fractional second and reproduces the committed words exactly; the constant's docstring records why. The test also picks the offset up from the generator (gen.TEMPORAL_GAP_CELL / gen._temporal_gap_offset_ns()) rather than restating it.
pytest tests/test_spec_conformance.py tests/test_coverage_toc.py tests/test_content_hash.py green; full suite green.
| # not left to describe leaves this walk proved absent. | ||
| from zagg.coverage_toc import delete_cover | ||
|
|
||
| delete_cover(store_root, **store_kwargs) |
There was a problem hiding this comment.
🤖 from Claude (review)
[spec] This arm deletes the sibling on any whole-store walk that produced no cover input, which is wider than the licence §10.5 actually grants and directly contradicts a normative sentence two paragraphs above it:
- §10.5 Composition: "a producer with no temporal contribution leaves the standing object untouched." Unqualified — and it is what
write_cover(None)implements on both arms. - §10.5's delete licence: "A producer that overwrites or DELETES
{store_root}/coverage.mocwholesale — the refresh path, when no stamped leaf remains — SHOULD discardcoverage.tocwith it." That sentence covers thenot decimalsarm at line 426, not this one.
The comment here cites "§10.5's wholesale-rebuild rule"; I cannot find that rule in the page. What actually justifies this arm is the carrier posture — refresh PUTs its envelope outright, so a None section drops the standing temporal key too, and the sibling dying with the section is symmetric. That is a defensible posture, but it is not written down, and it fires in cases that are not evidence of absence:
# leaves intact, read_leaf_temporal returns None for all of them (companions
# not yet written — schema evolution)
probe: sibling exists: False | section key: False | ranges: True
The sharpest case is test_a_manifest_without_a_cell_order_publishes_no_section (tests/test_coverage_toc.py:509): a manifest that merely lost cell_order forces toc_fields = {} — deliberately "publishing no section rather than guessing" — and a refresh on that store now destroys a perfectly good coverage.toc on the strength of a manifest defect, having proved nothing about the leaves. The sweep on the same store correctly leaves it standing, so the two producers disagree on the same evidence.
Suggest one of: (a) amend §10.5's composition sentence to carve out the refresh's wholesale PUT explicitly ("except where the producer replaces the carrier outright, discarding the standing section with it"), and say so in the code comment instead of citing a rule that isn't there; or (b) narrow the arm to fire only when the walk had temporal inputs to look at (toc_fields non-empty and no toc_failed), leaving the guess-nothing/manifest-defect path untouched. Either way it wants a test — see the separate test-gap note.
There was a problem hiding this comment.
🤖 from Claude
Took (b), narrowing the code rather than widening the spec — 2719ba11.
The else arm is gone: a whole-store walk that produced no cover input now leaves the standing sibling standing, which is the posture §10.5 already states and the one the sweep takes on identical evidence through write_cover(None). delete_cover survives only on the not decimals arm — the case §10.5's licence names by hand. The comment no longer cites a "wholesale-rebuild rule" that is not on the page; it names the reason (no evidence about the leaves) and points at the arm above.
Spec: the licence sentence is now scoped to deletion (A producer that DELETES {store_root}/coverage.moc wholesale — the refresh path, when no stamped leaf remains — SHOULD discard …), and a following paragraph says plainly that a producer which merely rewrites the carrier with no cover of its own does not discard the sibling, with the manifest-defect case as the reason.
Test: test_a_manifest_without_a_cell_order_publishes_no_section now snapshots coverage.toc bytes and asserts they are unchanged after BOTH the sweep and the refresh — the two producers agreeing on the same evidence, which is the thing that was broken. It fails on the pre-fold code. The not decimals arm got its own test too (test_refresh_over_an_unstamped_store_discards_both_objects), see the test-gap thread.
| rebuilt = read_cover(root) | ||
| assert rebuilt["source"] == "refresh" | ||
| assert rebuilt["shards"] == committed_cover["shards"] | ||
| assert envelope["temporal"][COVER_KEY] == rebuilt["spec"] == COVER_SPEC |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] The happy path gets cover assertions here, but every destructive or composing arm phase 2 added to refresh_root_coverage is untested — and those are the arms that can lose an operator's sibling:
- The partial/all-fail merge (
coverage.py:446-455).test_refresh_never_deletes_the_section_when_every_leaf_fails(line 534) andtest_refresh_composes_a_partial_rebuild_with_the_standing_section(line 556) assert onlyenvelope["temporal"]. Neither looks atcoverage.toc, so the wholeread_cover→merge_cover_sections→write_cover(replace=True)path is uncovered. It does work today (I checked: an all-fail refresh keeps the standing sibling'sshardsand itssource: "sweep"), which is exactly why a regression here would land silently. Two asserts in the existing tests cover it. - The no-cover-input delete (
coverage.py:477) — nothing exercises it.test_a_manifest_without_a_cell_order_publishes_no_section(line 509) already callsrefresh_root_coverageon a store that still has its committedcoverage.tocand asserts nothing about it; whatever posture that arm settles on (see the §10.5 note on line 477) belongs there as an explicit assert, plus the sweep's opposite behaviour on the same store (write_cover(None)leaves it standing). - The
not decimalsdelete (coverage.py:426) — the one arm §10.5's licence names by hand, untested. - The sweep's self-heal, i.e. the reason
cover_unchangedis in the skip test at all: carrier current, sibling deleted or corrupted out from under it. It does work (deletingcoverage.tocand re-sweeping givesroot_moc_written: True, cover_shards: 1and restores the object; anot json {sibling likewise), but nothing pins it — and a future refactor that moves the cover test above the carrier test, or drops it, still passes the suite. Worth a case intest_a_sweep_writes_the_section_the_fixture_committed, right after the idempotence assert.
Also, test_a_second_pass_over_a_multi_shard_store_writes_nothing (line 577) now implicitly depends on cover_unchanged converging across two incremental producers — the best place in the suite to assert the sibling accumulated both shards' blocks, which it currently does not check.
There was a problem hiding this comment.
🤖 from Claude
All of it covered — 7141c3ae (plus the arm-4 assertion in 2719ba11).
In tests/test_coverage_toc.py:
- Partial / all-fail merge.
test_refresh_composes_a_partial_rebuild_with_the_standing_sectionnow snapshots the standing cover and asserts the composed object still carries BOTH shards, with the failed shard's decoded words equal to the standing ones.test_refresh_never_deletes_the_section_when_every_leaf_failsassertsread_cover(root) == standing_cover— the all-failed walk re-publishes it verbatim. - The no-cover-input arm. Now an explicit assert in
test_a_manifest_without_a_cell_order_publishes_no_section: the committedcoverage.tocbytes survive both the sweep and the refresh (the arm was narrowed — see the §10.5 thread). - The
not decimalsdelete. Newtest_refresh_over_an_unstamped_store_discards_both_objects: leaves removed, refresh returnsNone, and bothcoverage.mocandcoverage.tocare gone. - Sweep self-heal. Added right after the idempotence assert in
test_a_sweep_writes_the_section_the_fixture_committed: with the carrier current, the sibling deleted and then written asnot json {, each sweep reportsroot_moc_written Trueand restores the committed shard blocks.
Also added, from your closing note: test_a_second_pass_over_a_multi_shard_store_writes_nothing now asserts the sibling accumulated both shards' blocks before the convergence passes. And a fourth case you flagged elsewhere — test_refresh_preserves_a_future_sibling_and_points_at_it — pins the refresh-level behaviour of a foreign standing cover: object untouched, and the carrier's marker names that object's own spec.
| ) | ||
| return False | ||
| try: | ||
| obstore.delete(store, COVER_NAME) |
There was a problem hiding this comment.
🤖 from Claude (review)
[nit] The return value ("whether an object was removed") is backend-dependent: S3's DeleteObject is idempotent, so obstore's S3 backend does not raise for a missing key and this returns True for an absent sibling, while the local backend raises NotFoundError and returns False. The new test pins the local answer (assert delete_cover(root) is False # absent: nothing to do, test_coverage_toc.py:1051) and would be wrong on the deployed backend — the same local-vs-S3 split _read_json deliberately papers over by catching both exception types.
Since the read above already knows the answer, derive it from existing and skip the pointless request:
if existing is None and not _garbage: # nothing there
return False
obstore.delete(store, COVER_NAME)
return True(the except (FileNotFoundError, NotFoundError) can stay for the concurrent-delete race). That also saves a DELETE against the bucket on every refresh of a non-temporal store, which today does a GET and a DELETE for an object that has never existed.
There was a problem hiding this comment.
🤖 from Claude
Fixed — a3c5b3dc.
delete_cover now distinguishes "absent" from "garbage" at the read and answers the absent case without a request:
try:
existing = _read_json(store, COVER_NAME)
garbage = False
except ValueError:
existing, garbage = None, True # debris — delete below
if existing is None and not garbage:
return False # nothing there: the read already answeredSo the local and S3 answers agree for an absent sibling, and a refresh of a non-temporal store no longer pays a DELETE against a key that has never existed. Garbage still deletes; the except (FileNotFoundError, NotFoundError) stays for the concurrent-delete race. The docstring now states the return is best-effort and backend-dependent under a race (two requests; S3's DeleteObject is idempotent).
The local test keeps its is False answer and now also pins the request: it wraps obstore.delete with a counter and asserts none was issued for the absent case.
| if sets is not None and key in sets: | ||
| hit = bool(np.any(np.atleast_1d(toc_overlaps(sets[key], q_start_ns, q_end_ns)))) | ||
| else: | ||
| hit = bool(toc_overlaps(words[key], q_start_ns, q_end_ns)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[nit] The default cover=None path lost its vectorization.
Before phase 3 the tier-1 answer was a single batched toc_overlaps over the whole word array; now it is one call per shard inside this loop, even when no cover was passed. At the published CA store's shape (~2.7k shards at o9 — see the shardmap sizing note) that is ~2.7k FFI round-trips per query on a path phase 3 had no need to change.
Suggestion: keep the batch for the keys that resolve to tier 1 and loop only over the cover keys, whose sets have to be tested individually anyway — e.g. collect tier = [k for k in keys if not (sets and k in sets)], run one toc_overlaps(np.asarray([words[k] for k in tier], np.uint64), ...), and merge with the per-shard cover hits before the final sorted.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 652a2c2, essentially your suggestion. The loop now only decides the cover keys per shard; everything that resolves to tier 1 — including a cover shard whose block failed to decode or came back empty — is collected and answered in ONE batched toc_overlaps:
if tier:
batch = np.atleast_1d(np.asarray(
toc_overlaps(np.asarray([words[k] for k in tier], np.uint64), q_start_ns, q_end_ns)
))
hits.update(zip(tier, (bool(h) for h in batch), strict=True))
return sorted(k for k, hit in hits.items() if hit)So cover=None is one FFI call again, as pre-phase-3, and the ~2.7k-shard CA store pays one call plus one per cover-listed shard. Output ordering is unchanged (the final sorted over the union of keys). Checked equivalence against the per-shard formulation over a 4-shard section × 60 windows × three cover shapes (full, partial, none): zero mismatches.
| The pruning answer, on the grammar's own predicate | ||
| (``mortie.toc_overlaps``): conservative — it never under-reports, and may | ||
| over-report by up to one quantum at a window edge. ``None`` when the | ||
| store carries no temporal section, which a caller MUST read as "no |
There was a problem hiding this comment.
🤖 from Claude (review)
[nit] The docstring's None rule is now inaccurate.
Paragraph one still says "None when the store carries no temporal section". After this change None requires no section and no readable cover, and a section-less carrier with a valid cover answers from the cover:
noext = {k: v for k, v in env.items() if k != "temporal"}
shards_overlapping(noext, LO, HI, cover=cov) -> ['11213']
shards_overlapping(noext, LO, HI) -> None
That is a defensible answer (there is temporal information), but the sentence the caller is told to MUST on should say so. test_no_information_stays_none only covers the junk-cover case, so this path is untested — and it is exactly the state the refresh's "no else arm" leaves behind (see the seam comment above), which makes it the case worth pinning.
There was a problem hiding this comment.
🤖 from Claude
Fixed in ffdd553. The sentence now reads: None only when the store offers NEITHER a readable temporal section NOR a readable cover — and it says outright that a section-less carrier with a standing cover still answers, because there is temporal information to answer from.
test_a_section_less_carrier_answers_from_the_cover pins your exact case ({k: v for k, v in envelope.items() if k != "temporal"}): cover=None is None, with the cover it is ["11213"]. Note the answer under the seam ruling on the other thread is "candidate", not "the cover's window test" — every shard is cover-only there, so the gap window returns ["11213"] too. The test asserts both windows, since that state is precisely the refresh's no-else-arm output and the cover may be arbitrarily old.
| expect = bool(toc_overlaps(np.asarray([other[0]], np.uint64), gap0, gap1)[0]) | ||
| assert ("11219" in got) == expect | ||
|
|
||
| def test_a_shard_only_the_cover_lists_is_still_a_candidate(self): |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] This test cannot distinguish the semantics its name claims.
The window [BASE_NS - DAY_NS, BASE_NS + 2*DAY_NS) sits on the shard's first campaign, so the assertion is green both when a cover-only shard is an unconditional candidate (what "is still a candidate" means, and what §10.5's "unknown, a candidate, never authoritative" says) and when it is authoritative and merely happens to hit — which is what the code does. The discriminating case is the gap window: today
shards_overlapping(envelope, gap0, gap1, cover=cover) -> []
for a shard tier 1 has never listed. Suggestion: assert the gap window here too, with whichever answer the seam is ruled to have (see the coverage_toc.py comment on that seam) — as written the test pins nothing about the rule it is named for.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 1fc0ce0, together with the seam ruling it discriminates (see the coverage_toc.py thread on that seam). The test now asserts the gap window as well as the campaign one:
gap0, gap1 = BASE_NS + 400 * DAY_NS, BASE_NS + 401 * DAY_NS
# Cover-listed AND tier-1-listed, this window prunes (the test above);
# cover-only, the same window keeps it — that is the whole rule.
assert shards_overlapping(envelope, gap0, gap1, cover=cover) == ["11213"]The ruling went your way: a cover-only shard is an unconditional candidate, so the gap window keeps it, and the assertion now fails if the code ever makes the cover authoritative there. The docstring says which window is the discriminator and why (the sibling may be arbitrarily older than the carrier at this seam).
| decoded = cover_words(cover) | ||
| assert set(decoded) == set(tier1) == {"11213", "11214"} | ||
| for shard, words in decoded.items(): | ||
| order = cover["shards"][shard].get("temporal_order", cover["temporal_order"]) |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] The standing parity test does not reach the seam its docstring claims.
_clone_shard returns two disjoint leaf lists, so each of the two sweeps writes a different shard: every shard in this assertion took merge_cover_sections' carry-over arm ("a shard on one side carries over unchanged"), and the per-shard union-then-requantize arm — the one where parity could actually break — never runs on the production writer's output, despite the docstring's "so the §10.4/§10.5 seams (not just a single-producer PUT) are inside the claim". (That arm is covered by test_mixed_orders_requantize_at_the_coarser and test_merging_an_object_at_a_coarser_pin_requantizes_there, so this is the standing test's scope, not a hole in the suite.)
Relatedly, nothing in the fixture coarsens, so .get("temporal_order", cover["temporal_order"]) always takes the default and both sides are always compared at order 16 — the coarsened branch of the invariant is never exercised here.
Suggestion: overlap the runs on one shard (e.g. a third pass for leaves in (a, b, a), or a + b on the second) so at least one shard's parity is asserted after a real union on the production path; and resolve the block's order through _object_pin rather than re-deriving the default inline, so the test reads the pin the way the module does.
There was a problem hiding this comment.
🤖 from Claude
Fixed in a86bf97. The sweep passes now overlap — for leaves in (a, b, a + b) — so the disjoint carry-over arm still runs (passes 1 and 2) and a third pass re-covers both shards, putting every block in the assertion through merge_cover_sections' per-shard union-then-requantize arm on the production writer's own output. Verified by spying on merge_cover_sections: the shard sets present on BOTH sides per call are [[], [], ["11213", "11214"]]. The docstring now says why the passes overlap instead of claiming the seam by assertion.
Also took the second suggestion: the block's order resolves as block.get("temporal_order", _object_pin(cover)), so the test reads the pin the way the module does rather than re-deriving the default inline.
Leaving the coarsened branch out by design: making a fixture leaf exceed the 512-bucket cap on the end-to-end path would need a synthetic multi-year leaf written into the §7 store, disproportionate for an arm test_the_cap_coarsens_by_order_and_records_it, test_mixed_orders_requantize_at_the_coarser and test_merging_an_object_at_a_coarser_pin_requantizes_there already pin at unit level — and _object_pin now makes this test correct if a future fixture ever does coarsen. Flagging it as standing for review rather than folded.
| continue | ||
| cover_set = _query_word_set(section, key) if key in blocks else None | ||
| if cover_set is not None and len(cover_set): | ||
| hits[key] = bool(np.any(np.atleast_1d(toc_overlaps(cover_set, q_start_ns, q_end_ns)))) |
There was a problem hiding this comment.
🤖 from Claude (review)
[perf] The cover arm is one toc_overlaps FFI call per shard, and on a store that HAS a cover that is every shard — ~450× slower than one batched call at CA scale.
This is not the thread settled in 652a2c2e: that fold restored batching for the tier-1 arm (if tier: … one batched call), which is precisely the arm a cover-carrying store almost never takes. Every shard the cover lists comes through this line individually instead.
Measured on this commit, at the shape phase 4 scales from — the real CA shardmap, 2,721 shards at o9 × ~47 cover words each:
decode 2,721 blocks (_query_word_set) 0.007 s
2,721 per-shard toc_overlaps calls 0.135 s
whole shards_overlapping(..., cover=…) 0.157 s
one concatenated call + np.add.reduceat 0.0003 s (identical answer)
tier-1 only, cover=None (batched) 0.0013 s
So the sibling makes the pruning query ~120× slower than the tier-1 query it exists to refine: 157 ms per window on the very reader path #489 is for. Decode is 7 ms of that; the FFI round-trips are the whole cost.
Suggestion — collect the cover keys the way the loop already collects tier, then answer them in one call:
if cov_keys: # cov_keys/arrs filled in the loop above
flat = np.concatenate(arrs)
offs = np.cumsum([0] + [len(a) for a in arrs])[:-1]
mask = np.atleast_1d(np.asarray(toc_overlaps(flat, q_start_ns, q_end_ns)))
hits.update(zip(cov_keys, np.add.reduceat(mask.astype(np.int64), offs) > 0, strict=True))reduceat needs every segment non-empty, which the existing empty-set-degrades-to-tier-1 rule already guarantees. I verified the batched answer is identical to the current loop's on the 2,721-shard store above.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 7fd89d4 — the cover arm now collects its word sets in the same loop that collects tier, concatenates them, makes ONE toc_overlaps call and segment-reduces with np.add.reduceat, exactly as suggested. The three-way seam is untouched: only a set that is non-empty AND decodable enters the concatenation, so reduceat never sees an empty segment and the empty/undecodable/tier-1-only shards still fall through to the batched tier-1 call as before. Every existing TestCoverPruning / TestCaliforniaShape / degrade test passes unmodified.
Re-measured on 7fd89d4 at the shape you used (2,721 synthetic o9 shards, mean 47.3 cover words each, one pass-day window):
whole shards_overlapping(..., cover=…) 0.1544 s -> 0.0089 s (17x)
of which block decode (_query_word_set) ~0.008 s
FFI round-trips 0.135 s -> 0.0003 s
tier-1 only, cover=None (batched) 0.0013 s
identical answer (95 shards selected)
So the sibling now costs ~7x the tier-1 query rather than ~120x, and what is left is the decode, not the FFI.
Pinned as a property too: TestCoverPruning::test_the_batched_query_answers_exactly_as_the_per_shard_loop builds a store carrying every arm at once — two real word sets, one structurally-legal empty block, one count-mismatched block, one tier-1-only shard, one cover-only shard — and asserts the batched answer equals a per-shard reference formulation over 60 windows, plus that the sweep actually discriminates (>3 distinct answers) rather than returning one constant.
| word-set cover — per shard, a small canonical toc word SET instead of the | ||
| section's single envelope word, so a window that falls in a gap *between* a | ||
| shard's observation campaigns prunes that shard too. It is GET-on-demand by | ||
| temporal consumers only (at CA scale it is ~300× the bootstrap object, which |
There was a problem hiding this comment.
🤖 from Claude (review)
[spec] The "~300× the bootstrap object" figure is measured against a bootstrap object that a cover-carrying store cannot have.
A store with a coverage.toc has, by construction, a temporal section in its root object — §10.1's cover marker lives there, and this page's own preceding paragraph says so — and that section's tier-1 map is one word per shard. At the CA shape both numbers refer to, measured on this commit (2,721 shards at o9, ~47 cover words each, the phase-4 pass-day shape):
coverage.toc 1,477,582 B (~1.5 MB — consistent with §10.5's "~1.2 MB")
tier-1 shard map alone, same 2,721 shards 89,836 B (~0.1 MB; more once the ranges MOC and §10.3 digest are counted)
→ ~15×, not ~300×. The 300× holds only against the published CA store's spatial-only ~7 KB root (the PR body's stated baseline) — a pre-#480 object, i.e. exactly the store that would never carry this sibling.
The conclusion is untouched: 1.5 MB inline on a 100 KB root is still an obvious no, and "not inline" stands. This is the justification's arithmetic, not the design point. The same figure is in specification.md §10.5 line 2518 ("~50–60 words per shard (~1.2 MB store-wide, ~300× the bootstrap object)"), which this paragraph paraphrases, so both would need the same edit — either drop the multiplier for "an order of magnitude larger than the whole root object", or keep a concrete anchor as "~1.5 MB against a ~0.1 MB root".
There was a problem hiding this comment.
🤖 from Claude
Fixed in c56f4cd — you are right that the denominator was a store this sibling cannot sit on. Re-measured the same shape here (2,721 o9 shards, ~47 words each): coverage.toc 1,477,890 B vs the tier-1-carrying section 97,997 B, i.e. ~15x, matching your numbers.
Three sites now read the same arithmetic:
docs/specification.md§10.5 — "~50–60 words per shard (~1.5 MB store-wide, ~15x the tier-1-carrying bootstrap object it sits beside, and ~300x a spatial-only pre-coverage sidecar: versioned temporal section (per-shard toc envelopes + root time-digest) — land before the source-coop build #480 root)".docs/hive_layout.mdsibling paragraph — "at CA scale ~1.5 MB against the ~0.1 MB tier-1-carrying bootstrap object it sits beside — ~15x, and ~300x a spatial-only pre-coverage sidecar: versioned temporal section (per-shard toc envelopes + root time-digest) — land before the source-coop build #480 root — which is why it is not inline".src/zagg/coverage_toc.pymodule docstring — same correction (it carried the bare "~300x the bootstrap object's size").
The ~1.2 MB estimate is now ~1.5 MB in both docs, and the ~300x figure survives only where it is true, explicitly against a spatial-only pre-#480 root. The conclusion ("not inline") is unchanged. PR body untouched.
| with no temporal channel carries no such key and its root object is | ||
| byte-identical to a pre-#480 one; absence is never a refusal. | ||
|
|
||
| The same store may carry one more **root sibling**, `{store_root}/coverage.toc` |
There was a problem hiding this comment.
🤖 from Claude (review)
[nit] Two other spots on this page now contradict this paragraph, and one of them is the page's own index of coverage objects.
-
The Coverage section's own table. It opens (line 619) with "Three tiers per shard plus one store-root object", and the table below has a single
rootrow —{store_root}/coverage.moc| one GET — the discovery bootstrap. As of this PR there are two store-root objects. coverage sidecar: versioned temporal section (per-shard toc envelopes + root time-digest) — land before the source-coop build #480 did not have to touch that table (itstemporalis a key insidecoverage.moc); time-occupancy refinement: gap-preserving sub-shard temporal coverage (word-set cover / time-keyed digest) #489 adds a genuinely new object, so a reader scanning the table for "where does coverage live, and what does it cost to read" will not findcoverage.toc— which is exactly the row a temporal consumer needs, since its cost line is different from every other row ("one GET, temporal consumers only, on demand"). -
The D5 node-invariant bullet (line 57): "The root alone also carries the manifest (and, in a follow-on,
coverage.moc)." Now alsocoverage.toc, and this is the bullet that states what may legally sit at the root.
Also, placement: the paragraph is inserted between the temporal-key paragraph and "A range is an inclusive run of same-order cells within one base cell, consecutive in digit-tail rank; endpoints are decimal strings…", which is still finishing the description of the root envelope's ranges block above (its next sentence defines source as "dispatcher"/"refresh"). After the insertion, "A range" reads as though it described the cover's range words — which are also ranges, on the time axis, and are base64 uint64 rather than decimal strings. Moving this paragraph below the source sentence keeps the root-envelope description contiguous.
There was a problem hiding this comment.
🤖 from Claude
All three fixed in 92ff568.
- The coverage table. "Three tiers per shard plus one store-root object" is now "…plus two store-root objects", and the table has a new row after
root:
| root sibling | [§10.5](specification.md) word-set cover: a per-shard toc word SET (temporal stores only) | `{store_root}/coverage.toc` | one opt-in GET, temporal consumers only, on demand |
-
The D5 bullet (line ~57). Updated — this is the bullet that enumerates what may legally sit at the root, and this PR adds an object to that list, so leaving it would make the invariant statement wrong. It now reads "The root alone also carries the manifest and the coverage sidecars —
coverage.moc, pluscoverage.tocon a temporal store." (The stale "in a follow-on" hedge goes with it;coverage.mocat the root has shipped.) -
Placement. The sibling paragraph moved down past the
sourcesentence, so the root-envelope description (ranges→ "A range is an inclusive run…" →sourceis"dispatcher"/"refresh") is contiguous again and "A range" can no longer be misread as the cover's range words. The paragraph now sits between that block and Reader flow.
|
🤖 from Claude espg ruling (in-session 2026-08-24) applied in f9bca34: the cover quantization pin moves from order 16 to order 18 — bucket span 2^45 ns ≈ 9.77 h, guaranteed gap floor 2 spans ≈ 19.5 h. Ruled now because the PR is unmerged and no store has been swept, so the pin moves for free; post-publication it would be a revision bump. Rationale recorded in spec §10.5 and the PR body's resolved question (3): correctness is order-independent (widening only), storage flat (~1 word/pass), cap-K absorbs future time-dense sources, and order 18 holds the cover-midpoint epoch error to ±4.9 h against the closest-observation Sentinel-2 consumer's ~4.3-day cadence while resolving consecutive-day revisits. Constant renamed |
Closes #489.
Implements the gap-preserving per-shard temporal word-set cover — option 1 of the issue's sketch, per the approved plan (#489 (comment)) and espg's rulings recorded there: a sibling root object
{store_root}/coverage.toc(never inline — at CA scale the cover is ~300× the 7 KB bootstrap object), day-order quantization derived from the toc grammar, a cap-K coarsening law, the writer riding the rollup sweep, and the spec §10 + fixture updates in the same PR (#340 rule).What it does
Tier 1 of
zagg-coverage-toc/1(#480/#481) publishes one envelope word per shard, so gaps are invisible below shard granularity. This PR adds §10.5: per shard, themortie.toc_normalizecanonical cover of its §8.3 companion words, quantized to aligned power-of-two buckets, stored in its own GET-on-demand object. "Is there data in[t0, t1)" then resolves per shard from metadata, down to a ~39 h floor, without opening a leaf. The kernels are all mortie's (toc_normalize/toc_and/toc_reduce, shipped in espg/mortie#199; floor already ≥0.9.10) — this is storage-shape + writer work only.Quantization (spec-pinned): temporal order
opartitions the toc scale (2^63 ns from the 1850 epoch) into2^oaligned buckets of2^(63−o)ns; bucket bounds are exactly representable on the grammar's own 2^31/2^32 encoding grids for everyo ≤ 31. The pinned cover order is 18 (span 2^45 ns ≈ 9.77 h; espg ruling 2026-08-24 — see resolved question (3) below). Widening only: the cover may over-claim by < 1 bucket at cluster edges, never false-negative; a real gap survives iff it contains a whole aligned bucket — guaranteed floor two bucket spans ≈ 19.5 h (never-bridge; law corrected per review finding r3839559923). Cap K = 512 words/shard, coarsen-by-order until it fits, landing order recorded per shard.Parity invariant (standing test):
toc_reduce(cover_words) == toc_reduce(quantize({tier-1 word}, o))for every shard at its effective ordero. Note this is the quantized tier-1 envelope, not the raw word — the tier-1 word lives on the native 2^31/2^32 grids, the cover on the bucket grid; equality with the raw word does not hold and the spec text says so explicitly. Quantization commutes with union and with the envelope join (verified empirically against mortie 0.9.10 before writing the spec text), which is what makes both the per-leaf fold exact and this invariant well-defined.Empirical grounding: the plan comment's decode of the published CA store (shard
3231242244, 2,699,113 exact-timestamp words → 49 pass-days). The test suite carries a scaled synthetic of that shape (49 pass-days × 200 instants) and asserts containment, gap preservation, compression to ≤ 49 words, and parity.Phases
coverpresence marker, §10.4 marker seam rule, new §10.5 sibling-object contract) +coverage_toc.pycore:quantize_words,build_cover_section/merge_cover_sections/cover_unchanged,write_cover/read_cover(GET-union-PUT + replace path, unknown-revision preservation), reader accessorsload_cover/cover_words;read_leaf_temporalextended to return the per-leaf cover (quantized leaf-side, so the accumulator never holds a CA-scale raw word multiset); unit tests.MocFamily.finish(sweep) andrefresh_root_coveragebuild/PUT the sibling and stamp the presence marker; skip-if-current extended;temporal/fixture regenerated with itscoverage.toc+ conformance assertions (Promote zagg-ragged/1 + digest + composition byte layouts to a normative spec (reader-migration gate) #340).shards_overlapping(..., cover=)per-shard window intersection; parity invariant as a standing test over the committed fixture and synthetic stores.hive_layout.mdroot-coverage docs gained the sibling. (The real-store sweep round-trip is the regenerated §7 fixture itself: production writer, pinned multi-word cover, pinned surviving gap, standing parity test over incremental multi-shard sweeps.)(Deviation from the plan's phase split, for byte-stability: fixture regeneration rides phase 2 with the writer — the
temporal/fixture'scoverage.tocis written by the production sweep writer, which only exists once the sweep is wired. Same PR either way, per the #340 rule.)How it was tested
ruff check/ruff format --checkclean on the changed files;pytest -v tests/test_coverage_toc.py(68 tests) plustest_sweep.py,test_spec_conformance.py,test_coverage_root.py,test_coverage.py(386), plus one full-suite run per phase (final: 4,681 passed, one unrelated flake intest_client_transport.py::TestStatusPollerthat passes consistently in isolation), 1 environment-specific failure (test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds— the build script's pip resolves cp310 wheels on this machine, wherezarr>=3.1.5has no distribution; unrelated to this change,deployment/untouched).Pre-existing, not fixed here (flagging per CLAUDE.md §4):
ruff check src testson origin/main already fails N818 onsrc/zagg/registry.py:64(UnknownCapability) — the PR lint bot runs--select=E,F,W,Iso CI does not see it;pre-commit's mypy hook reports 124 whole-project errors in untouched files (e.g.runner.py,coverage.py:384).Questions for review
zagg-coverage-toc-cover/1(the plan left the name to the PR). Alternative candidate waszagg-toc-cover/1; the longer form keeps the §10 family prefix.covermarker is an in-revision addition (optional key, pre-marker readers ignore unknown keys — §10.1's own rule), not azagg-coverage-toc/2bump. If you'd rather bump, say so before phase 2 stamps it.Day-order semanticsRESOLVED (espg ruling, in-session 2026-08-24): the pin is order 18 (TEMPORAL_COVER_ORDER = 18, span 2^45 ns ≈ 9.77 h, guaranteed gap floor 2 spans ≈ 19.5 h), replacing the phase-1 order-16 derivation. Rationale as ruled: correctness is order-independent (quantization only widens), storage is flat (~1 word per pass either way), the cap-K path absorbs any future time-dense source, and order 18 cuts the cover-midpoint epoch error from ±19.5 h to ±4.9 h — which matters for the closest-observation Sentinel-2 consumer against its ~4.3-day cadence — while resolving consecutive-day revisits that 39.1 h buckets fuse. Landed pre-publication inf9bca343(spec §10.5, code, fixtures regenerated); post-publication this would have been a revision bump.Refs #480, #481, espg/mortie#199.