small fixes 2026-08-19: sweep nudge for a dropped §10 section; the walk-as-tightener refresh contract - #491
small fixes 2026-08-19: sweep nudge for a dropped §10 section; the walk-as-tightener refresh contract#491espg wants to merge 14 commits into
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of a2d5d5b (phase 1, issue #488), fresh context. The shape is right — declaration-driven detection, the preserve-vs-missing split against _usable/_preserved, checking the payload write_root_coverage actually returned rather than the input envelope, and thirteen real (non-vacuous) tests including an end-to-end pass on the committed temporal/ fixture. ruff check --select=E,F,W,I --ignore=E501 and ruff format --check are clean on the four touched files; TestMissingSectionWarning + TestFinisher pass locally (16 passed); commit message is §3-shaped; no new dependency; module sizes are well inside §4 (coverage_toc.py 546, sweep_stages.py 597). Question (3) in the body checks out — N818 on src/zagg/registry.py:64 is on main unchanged.
Six findings, most severe first:
- HIGH — the warning's stated cause is false for the store this seat sees most. No code path on the staged side ever writes a §10 section (only
sweep.MocFamily.finishandrefresh_root_coveragedo), so on a temporal store that is ingested and staged-swept but never walked the section was never built, nothing dropped it, and the nudge fires forever saying a pre-§10.4 producer "may have dropped it". The PR's own new wiring test builds exactly that store. Question (1) in the body discloses only a narrower variant of this. - MEDIUM —
toc_section_missingis the only key in the finisher'soutthat is not pre-seeded; on an emptyby_shardit is absent from the run record entirely, contradicting the docstring that surfaces it unconditionally, and no test covers that branch. - MEDIUM — "a section that is missing here was dropped by someone else" does not hold on
write_root_coverage's overwrite branch, where this very call discards the standing section and then blames a predecessor (reproduced: two warnings back to back). - LOW — the check reads the possibly-stale
manifestargument one line before the finisher re-readsfreshfrom the store for the same reason. - LOW —
_usable's DEBUG "ignoring a section with an unknown spec" fires on the arm that deliberately honors the section, and the newcaplog.text == ""assertions lean on that line staying below capture level. - LOW —
_standing_sectionre-implementsconftest.toc_words, which this same test module already uses.
Nothing here blocks phase 2; (1) and (2) are worth folding before this leaves draft, since phase 2 (issue #487) makes the wording in (3) normative in docs/specification.md.
Generated by Claude Code
| logger.warning( | ||
| f"coverage[toc]: {store_root} declares temporal fields but its root " | ||
| f"{ROOT_COVERAGE_NAME} carries no {TEMPORAL_COVERAGE_SPEC} section — a producer " | ||
| f"that predates spec §10.4 may have dropped it, and `when=` pruning degrades to " |
There was a problem hiding this comment.
🤖 from Claude (review)
HIGH — the message asserts a cause that is false for the store this seat sees most: "never built" and "dropped" are indistinguishable here, and at the staged finisher "never built" is the steady state.
Nothing on the staged path ever writes a §10 section. The only two writers in the tree are the families sweep and the walk:
$ grep -rn "build_temporal_section(" src/zagg
src/zagg/coverage.py:418: section = build_temporal_section(contributions, toc_fields, source="refresh")
src/zagg/sweep.py:301: section = build_temporal_section(
runner.py (ingest, write_root_coverage at lines 1135/3278) and run_finisher itself both build the root envelope with no section at all. So for a temporal store that is ingested and then swept only by the staged sweep (issue #384) and never walked, the section was never built — nobody dropped anything — and this line fires on every finish, forever, telling the operator that
a producer that predates spec §10.4 may have dropped it
The PR's own new wiring test is exactly that store, and its docstring states the false cause:
def test_a_temporal_store_with_no_root_section_is_nudged(self, tmp_path, caplog):
"""... so a section missing HERE was dropped by a producer that
predates §10.4 ..."""
m = _stage_store(tmp_path / "s", fields=TIMED_FIELDS)_stage_store builds the store from scratch in tmp_path with real temporal companions in the leaves (_located_leaf_slabs) and writes the root MOC via write_root_coverage — a healthy, brand-new store with no predecessor at all. The nudge is right that a walk is wanted; the diagnosis it hands the operator is wrong, and it is the diagnosis they will act on (chasing a stale worker that does not exist).
Question (1) in the PR body discloses a strictly narrower version of this — "leaves written before the declaration was added". The mainline case (declaration correct, leaves genuinely carrying companions, simply never walked) is not that, and is the common one.
Cheapest fix is the message: name both causes, e.g. … carries no {TEMPORAL_COVERAGE_SPEC} section — no walk has built one yet, or a producer that predates spec §10.4 dropped it; …. If the single-cause wording is deliberate, it rests on an assumption (staged-only stores are expected to be walked) that is not stated anywhere in the tree and belongs in the issue thread as a question.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9d05ae6 — the message no longer asserts a cause.
… carries no zagg-coverage-toc/1 section — no walk has built one yet, or a
producer dropped it; either way `when=` pruning degrades to opening every
candidate until it is built, so run zagg.coverage.refresh_root_coverage('…')
Both causes, neither claimed, one remedy that fixes either. test_a_temporal_store_whose_root_carries_no_section_warns now asserts the line names both, so the single-cause wording cannot come back silently.
One correction to the finding's framing, verified before writing it down: in the default pipeline the walk does precede the staged finisher. runner.py runs the families sweep first and the staged sweep second, in the same post-run hook:
sweep_after_run(store_path, leaves, store_kwargs=store_kwargs)
if config.output.get("sweep") == "stages":
from zagg.sweep_stages import stage_sweep_after_run
stage_sweep_after_run(store_path, leaves, store_kwargs=store_kwargs)DEFAULT_FAMILIES = ("stats", "moc", "submap", "overview") includes moc, and MocFamily.finish is one of the two section writers. So a missing section at the finisher is anomalous for a default-pipeline store — but not for one swept with --stages standalone, nor for one whose fail-open sweep_after_run failed. That is exactly why naming a cause is wrong and naming both is right; the docstring now records the ordering and both seats it does not cover.
The test docstring is fixed too: that store is the never-walked case, and says so.
Generated by Claude Code
| write_root_coverage(store_root, envelope, **store_kwargs) | ||
| written = write_root_coverage(store_root, envelope, **store_kwargs) | ||
| out["root_moc"] = True | ||
| out["toc_section_missing"] = warn_if_section_missing(store_root, written, manifest) |
There was a problem hiding this comment.
🤖 from Claude (review)
MEDIUM — toc_section_missing is the only key in out that is not pre-seeded, so it vanishes from the run record whenever by_shard is empty.
Every other key is initialized in the dict literal and then overwritten:
out = {
"root_moc": False,
"manifest_updated": False,
"objects_touched": 0,
"touch_failures": 0,
"lease_released": False,
}This one is set only inside if by_shard:. Confirmed against a2d5d5b:
m = _stage_store(tmp_path / "s", fields=TIMED_FIELDS)
out = run_finisher(str(tmp_path / "s"), m, {}, {}, run_id="A")
# KEYS: ['lease_released', 'manifest_updated', 'objects_touched', 'root_moc', 'touch_failures']Consequences:
- the finisher block of the PUT run record (
sweep_stats_{ts}_stages.json,_write_stage_record) has a conditional shape — a field that is present on most runs and absent on an empty-work-set run is worse for a log consumer than a field that is always there andfalse; run_finisher's own docstring says the nudge is surfaced "attoc_section_missing" with no qualifier, so a reader indexingsummary["finisher"]["toc_section_missing"]gets aKeyError. In-repo callers happen to use.get, so nothing crashes today — it is the published shape that is wrong;- no test covers the empty-
by_shardbranch, so the divergence is invisible to CI.
Fix is one line: seed "toc_section_missing": False in the literal alongside "root_moc": False, and add a case asserting the key is present-and-False on an empty work set.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in 8be2cec. "toc_section_missing": False is now seeded in the literal alongside "root_moc", so the finisher block of the stage record has one shape regardless of the work set.
New case in tests/test_sweep_stage.py::TestFinisher:
def test_an_empty_work_set_still_reports_the_nudge_key(self, tmp_path):
m = _stage_store(tmp_path / "s", fields=TIMED_FIELDS)
out = run_finisher(str(tmp_path / "s"), m, {}, {}, run_id="A")
assert out["root_moc"] is False and out["toc_section_missing"] is FalseThe docstring now states the guarantee where a consumer will read it ("that key is ALWAYS in the returned dict … a run with an empty work set writes no root object and reports False"), rather than leaving the unqualified "surfaced at toc_section_missing" to imply it.
Generated by Claude Code
| sweep preserves, the walk tightens — see | ||
| :func:`zagg.coverage.refresh_root_coverage`); preserving is what | ||
| makes the belt below meaningful, since a section that is missing | ||
| here was dropped by someone else. The written payload is checked by |
There was a problem hiding this comment.
🤖 from Claude (review)
MEDIUM — "a section that is missing here was dropped by someone else" is not true on the overwrite branch: this very call can be the thing that dropped it, and then it blames a predecessor.
write_root_coverage preserves only when it rebuilt the envelope by union. On an unparsable or incompatible standing object it overwrites, and the section goes with it — deliberately, and documented there (src/zagg/hive.py):
# A rebuilt `merged` dropped the existing carrier's extra keys with it;
# an OVERWRITE (`merged is envelope`) deliberately discards the stale
# section too, exactly as it discards the stale ranges.
carried = isinstance(existing, dict) and merged is not envelopeSo the docstring's step (1) claim — the finisher "PRESERVES the spec §10 temporal section", therefore "a section that is missing here was dropped by someone else" — holds on the compatible branch only. Reproduced on a2d5d5b (standing envelope carrying a good section, order bumped so the finisher's envelope is incompatible):
WARNING zagg.hive: existing coverage.moc at …/s has an incompatible envelope; overwriting (regenerable cache)
WARNING zagg.coverage_toc: coverage[toc]: …/s declares temporal fields but its root coverage.moc
carries no zagg-coverage-toc/1 section — a producer that predates spec §10.4 may have dropped it …
Two warnings, back to back, the second misattributing what the first just did. Severity is bounded (the remedy is still correct, and the D9 posture makes the loss recoverable), but it is a wrong causal claim in a docstring that phase 2 (issue #487) is about to make normative, plus an untested branch. Suggest scoping the docstring sentence to the union arm, and either checking written is not envelope before warning or letting the message name the overwrite as a possible cause.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in e47e9d2. The docstring sentence is scoped to the arm it holds on, and the overwrite arm is named as a dropper this call can itself be:
Preserving holds on
zagg.hive.write_root_coverage's UNION arm; an unparsable or incompatible standing root is OVERWRITTEN there by design (D9 regenerable cache) and the stale section goes with the stale ranges, so on that arm this very call is the dropper — which is why the belt below names no cause.
The "names no cause" half is the other half of the fix: after 9d05ae6 the warning says only "no walk has built one yet, or a producer dropped it", so the back-to-back pair no longer misattributes — the second line makes no claim the first contradicts, and the remedy it names is still correct on this arm.
The branch is now covered, in TestFinisher:
def test_an_incompatible_standing_root_drops_the_section_and_nudges(self, tmp_path, caplog):
…
env["encoding"] = "bitmap" # the leaf sidecar's encoding at the root
(tmp_path / "s" / "coverage.moc").write_text(json.dumps(env, indent=1))
…
assert "incompatible envelope; overwriting" in caplog.text
assert out["toc_section_missing"] is TrueIt PUTs the incompatible envelope outright rather than routing it through the writer, which would only reproduce the overwrite one call early.
Generated by Claude Code
| written = write_root_coverage(store_root, envelope, **store_kwargs) | ||
| out["root_moc"] = True | ||
| out["toc_section_missing"] = warn_if_section_missing(store_root, written, manifest) | ||
| fresh = read_manifest(store_root, **store_kwargs) |
There was a problem hiding this comment.
🤖 from Claude (review)
LOW — the check reads the caller's (possibly stale) manifest argument while the next line re-reads the manifest from the store for the actuals RMW.
warn_if_section_missing(store_root, written, manifest) on line 332 uses the manifest run_stage_sweep read at admission — before the lease, the stages, and however many minutes of fanned-out work; line 333 then does fresh = read_manifest(store_root, **store_kwargs) precisely because the in-store manifest may have moved. The temporal declaration lives in pyramid.overview.fields, which fresh carries too, so the check could simply run after the re-read and be consistent with the RMW below it — the write on line 330 has to come first, but the check does not.
Concretely: a store whose temporal: "per-centroid" declaration was added after this run started is not nudged, and one whose declaration was removed mid-run is nudged for a channel it no longer has. Rare, but free to avoid — move lines 331-332 below the fresh read and pass fresh (which also pairs naturally with pre-seeding the key, see the other comment).
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Moved, in b9a3d66 — the check now runs after the re-read and against fresh:
written = None
if by_shard:
…
written = write_root_coverage(store_root, envelope, **store_kwargs)
out["root_moc"] = True
fresh = read_manifest(store_root, **store_kwargs)
…
if written is not None:
out["toc_section_missing"] = warn_if_section_missing(store_root, written, fresh)The sentinel is what lets the write stay where it must be (before the re-read) while the check moves below it. The reason is recorded at the call site and in the docstring: "the check runs against step 2's RE-READ manifest, not the caller's admission-time copy, so it judges the declaration the store carries now."
Covered by a new case that fails on the old ordering — the caller's manifest declares no temporal channel, the store's does:
stale["pyramid"]["overview"]["fields"] = FIELDS # no `temporal:` at admission
out = run_finisher(str(tmp_path / "s"), stale, _by_shard(), {}, run_id="A")
assert out["toc_section_missing"] is TrueGenerated by Claude Code
| if not temporal_fields(manifest): | ||
| return False | ||
| section = envelope.get(TEMPORAL_KEY) if isinstance(envelope, dict) else None | ||
| if _usable(section) is not None or _preserved(section) is not None: |
There was a problem hiding this comment.
🤖 from Claude (review)
LOW — _usable logs "ignoring a section with an unknown spec" at the one call site whose decision is to honor it.
_usable is not side-effect free:
if not isinstance(section, dict) or section.get("spec") != TEMPORAL_COVERAGE_SPEC:
if section is not None:
logger.debug("coverage[toc]: ignoring a section with an unknown spec")so the future-revision arm here — the arm the docstring describes as "§10.4 preserves it verbatim … is NOT missing" — emits the opposite claim to anyone running at DEBUG. Reproduced on a2d5d5b with env["temporal"] = {"spec": "zagg-coverage-toc/2", "shards": {}}:
DEBUG zagg.coverage_toc:coverage_toc.py:424 coverage[toc]: ignoring a section with an unknown spec
Same line fires for every non-dict debris shape in the new parametrization. It is pre-existing text (merge_temporal_sections is the site it was written for, where "ignoring" is accurate), but this is a new consumer where it is not. It also makes the new assert caplog.text == "" assertions quietly dependent on the DEBUG line staying below the capture level rather than on the function being silent.
Cheapest resolution: check the two predicates in the order the docstring reads them — _preserved(section) is not None or _usable(section) is not None still short-circuits the marked-revision case before _usable can mislabel it, and the marked case is the only one where the text is wrong.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Reordered in e953364, exactly as suggested:
# Marked-revision first, in the order the docstring reads them: :func:`_usable`
# logs "ignoring a section with an unknown spec" — true at the merge seam it
# was written for, the opposite of what this seat decides — so the arm that
# HONORS a future revision must short-circuit before it is consulted.
if _preserved(section) is not None or _usable(section) is not None:
return False_usable's text is left alone — it is accurate at merge_temporal_sections, and the debris arms that still reach it here really are being ignored.
The silence is now asserted rather than assumed: test_an_unreadable_future_revision_is_not_missing captures at DEBUG and asserts caplog.text == "", so the future-revision arm has to be genuinely silent, not merely quiet at WARNING.
Generated by Claude Code
|
|
||
| contributions = {} | ||
| for i, dec in enumerate(decimals): | ||
| t = int(np.datetime64(TOC_BASE, "ns").astype("int64")) + i * 10**12 |
There was a problem hiding this comment.
🤖 from Claude (review)
LOW — _standing_section hand-rolls the toc word instead of using the shared conftest.toc_words helper this file already imports.
Lines 371-373 re-implement "an instant off TOC_BASE, packed as a toc word":
t = int(np.datetime64(TOC_BASE, "ns").astype("int64")) + i * 10**12
words = np.asarray([int(time2toc(t))], dtype=np.uint64)conftest.toc_words is the shipped fixture helper for exactly this (tests/conftest.py:78, "Shared fixture helper for the temporal channel (spec §8.3, issue #410)"), and _located_leaf_slabs in this same module already uses it that way at line 288:
when = np.datetime64(TOC_BASE, "ns") + np.timedelta64(60 * (i * 16 + j), "s")
kw["temporal"] = toc_words(1, base=str(when))Same import block, one call, no direct mortie.time2toc dependency in the test module — CLAUDE.md §4 "match the surrounding code". Not a correctness problem (the section it builds is valid and the test does exercise the preserve arm), just a second way to say the thing the file already says once.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in ab1b345 — _standing_section now packs through the shared helper, the same way _located_leaf_slabs does seven lines up, and the direct mortie.time2toc import is gone from the module:
when = np.datetime64(TOC_BASE, "ns") + np.timedelta64(1000 * i, "s")
words = toc_words(1, base=str(when))
value = float(when.astype("int64"))
contributions[dec] = [(int(words[0]), np.asarray([[value, 3.0]], dtype=np.float32), words)]Worth noting the hand-rolled version was not merely a second spelling: it passed a raw ns-since-1970 integer straight to time2toc, while toc_words goes through mortie.from_datetime64 first, so the two were packing different instants. The section was still valid and the preserve-arm assertion still held, which is exactly why reusing the shipped helper is the right call rather than a style preference.
Generated by Claude Code
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of 38da243 (phase 2, docs) plus the six phase-1 folds, with fresh context. Six findings, two HIGH.
Both HIGHs are the same root cause: §10.5 describes the §10.4 seam as if the join were its only arm. It is not — write_root_coverage has an overwrite arm (carried = isinstance(existing, dict) and merged is not envelope) where merge_temporal_sections(None, incoming) installs the caller's section verbatim with no toc_merge at all. §10.4 documents that arm; §10.5 does not, and its new MUST-NOT is broken there by MocFamily.finish, a run-scoped producer. I reproduced the narrowing end to end against the shipped writer and showed a toc_overlaps query flipping True → False, i.e. missed data, not the "wasted open" the section promises. That also refutes question (3)'s premise that the MUST-NOT is "not a new obligation on any shipped producer".
Findings, most severe first:
- HIGH (
docs/specification.md2469–2485) — the new MUST-NOT is violated by shipped code onwrite_root_coverage's overwrite arm; §10.5's closing "the one loss this contract does not prevent" misses a strictly worse loss than the one it names. - HIGH (
docs/specification.md2480–2484) — "only a whole-store walk refreshes tier 2" is false, and mis-cites §10.4's whole-merged-map rule as whole-store. Reproduced: a run-scoped sweep replaces a refresh's digest. Also contradicts issue #487's own wording. - MEDIUM (
src/zagg/hive.py1082–1091) — the same overclaime47e9d2folded out ofrun_finisheris reintroduced here, contradicting the overwrite branch documented three paragraphs above it in the same docstring. - MEDIUM (
src/zagg/coverage.py262–263) — the tightener itself can publish a word not derived from every leaf:split_leaf_name's malformed-window-labelcontinuedrops a sibling leaf without enteringtoc_failed, so the whole-store PUT proceeds. - MEDIUM (
docs/specification.md2492–2499) — the new heading re-parented §10's closing conformance paragraph into §10.5, where "the containment and weight claims above" no longer refers to what thetemporal/fixture pins. - LOW (
src/zagg/coverage.py257–273) — three un-reflowed seams (92 and 87 columns) in a block wrapped at ~75.
What I checked and found sound, so it does not appear as a finding:
toc_mergereally does only grow. 40k random pairs overtime2toc/span2tocwords: the merged word'stoc2timeenvelope contains both operands' envelopes in every case, and 500 chained merges never shrank the envelope. Bullet (1) is true of the join; the problem is only that the join is not the seam's only arm.- The deleted fragment was genuine debris.
git log -S "supersedes it). A"gives28f2fce(introduced) →38da243(removed), and the clause it duplicated still stands intact atcoverage.py:251. No sentence lost meaning. - Cross-reference targets all resolve.
zagg.hive.write_root_coverage,zagg.coverage.refresh_root_coverage,zagg.sweep_stages.run_finisher,zagg.coverage_toc.warn_if_section_missing,zagg.sweep.sweep_after_run,zagg.sweep_stages.stage_sweep_after_runall exist and are:func:;zagg.sweep.MocFamily.finishis correctly:meth:andzagg.sweep.DEFAULT_FAMILIEScorrectly:data:. (mkdocsis not installed in.venv, so I could not render — checked by name resolution instead.) 9d05ae6's pipeline-ordering claim is accurate.runner.py:3336-3348runssweep_after_runand only thenstage_sweep_after_run, inside the sameif store_layout == "hive" and get_sweep(config)block.- The other five folds hold.
8be2cec's pre-seed does fix the record shape;b9a3d66's move of the check afterfresh = read_manifestis correct andwrittenis always a dict on that path;e953364's_preserved-before-_usablereorder is right, since_usablelogs at DEBUG for any non-matching spec;ab1b345'stoc_wordsreuse matches_located_leaf_slabs' base. - CLAUDE.md §3 — all seven messages are title-only and match
git logstyle. tests/test_coverage_toc.py+tests/test_sweep_stage.py: 135 passed.ruff checkandruff format --checkreport only the two pre-existing items already flagged in question (4) (N818inregistry.py, a code block intests/data/benchmark/README.md); neither is in this diff.- Fixtures: I agree with question (3)'s conclusion that CLAUDE.md §4 does not require regenerating
tests/data/spec/— no wire format, attrs grammar, orspecmarker moved. Finding (5) is about where the conformance sentence now sits, not about the fixtures themselves.
Findings (1)–(3) are one coherent fix; (4) and (5) are independent.
Generated by Claude Code
| - **Adding observations keeps the claims true automatically.** Every | ||
| observation-adding producer composes its words through the §10.4 seam, and | ||
| tier 1's join only ever *grows* an envelope. Growing is conservative in the | ||
| direction §10.2 requires — the word must contain every instant in the shard | ||
| — so an appended observation lands inside that shard's word by | ||
| construction, whichever producer wrote it and in whatever order. | ||
| - **Removing observations leaves a word over-wide, never wrong.** A rewrite | ||
| that drops data leaves the shard claiming a window wider than the shard now | ||
| holds. A reader opens that candidate and finds nothing in the window: a | ||
| wasted open, never missed data — the same asymmetry a window edge's quantum | ||
| already has (§10.2). | ||
| - **Only a whole-store walk tightens**, and only a whole-store walk refreshes | ||
| tier 2. Tightening is what re-derives a shard's word from what its leaves | ||
| hold *now* instead of joining it with what they held before, and tier 2's | ||
| whole-coverage rule (§10.4) already denies the digest to any partial | ||
| producer. A producer MUST NOT publish a **narrowed** word for a shard unless | ||
| it derived that word from every leaf of the shard. |
There was a problem hiding this comment.
🤖 from Claude (review)
HIGH — the new MUST-NOT is violated by shipped zagg, because the §10.4 seam has an arm that is not a join. This directly refutes question (3) in the PR body.
The section rests on this:
Every observation-adding producer composes its words through the §10.4 seam, and tier 1's join only ever grows an envelope.
A producer MUST NOT publish a narrowed word for a shard unless it derived that word from every leaf of the shard.
I verified the "join only grows" half empirically and it holds — 40k random pairs of time2toc/span2toc words, toc_merge's envelope contains both operands' toc2time envelopes every time, and repeated merges never shrink. So bullet (1) is true of the join.
But write_root_coverage reaches the join only on the union arm. On the overwrite arm it does not join at all (src/zagg/hive.py):
carried = isinstance(existing, dict) and merged is not envelope
section = merge_temporal_sections(
existing.get(TEMPORAL_KEY) if carried else None, envelope.get(TEMPORAL_KEY)
)carried is False, so the call is merge_temporal_sections(None, incoming), and that returns dict(b) — the incoming section installed verbatim, with no toc_merge anywhere. §10.4 documents that arm ("a producer that overwrites the carrier wholesale … discards the stale section with it"); §10.5 does not mention it.
That matters because MocFamily.finish is a run-scoped producer, not a whole-store one — it builds its section from self._temporal, which holds only the (decimal, window) leaves this run visited — and it passes that section straight into write_root_coverage. On a windowed store whose run touched a subset of a shard's windows, plus any overwrite trigger (unparsable root, incompatible spec/encoding/order, or a ranges body that fails root_coverage_words), the result is a published word narrower than the shard's true extent, derived from some of its leaves. Reproduction against the shipped writer:
$ python repro.py
existing coverage.moc at /tmp/tmpll5np0fl failed to parse (invalid literal for int() with base 10: 'z'); overwriting (regenerable cache — the sweep rebuilds authoritatively)
standing word : 3092601599982677469 (2019-01-01 … 2021-12-31)
published word: 3218745596920049117 (2021-01-01 … 2021-12-31)
NARROWED (not a join): True
2019 window overlaps standing word : True
2019 window overlaps published word: False
The last two lines are the failure §10.2 exists to forbid: a toc_overlaps query that used to find the shard now prunes it. That is missed data, not a wasted open — so the closing paragraph is also wrong when it says "The one loss this contract does not prevent is a producer that drops the section". Dropping degrades to a full scan (safe). Narrowing-on-overwrite is strictly worse and is not named anywhere in §10.5.
Concretely, one of these:
- give §10.5 a bullet for the overwrite arm — a producer that discards the carrier MUST either drop the section entirely (safe: absent shard = unknown = candidate) or publish only words it derived from every leaf of each listed shard; and
- either fix
write_root_coverageto dropenvelope[TEMPORAL_KEY]on the overwrite arm unless the producer vouches for whole-shard coverage, or say plainly in §10.5 that §10.4's overwrite clause is a carve-out from this MUST-NOT.
Either way question (3)'s premise ("rather than a new obligation on any shipped producer") does not hold as written, and e47e9d2 already established that this exact arm is the one the phase-1 prose kept getting wrong.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Folded in 2e8bf99. The finding is correct and I reproduced it independently before writing anything down, so §10.5 now describes both arms instead of one.
Three changes, all prose — the code is untouched:
- The lead paragraph and the first bullet are scoped to §10.4's union arm ("Across §10.4's union arm the section is always safe, eventually precise … Producers therefore need no coordination on that arm"), and the lead now points forward to the carve-out.
- The MUST-NOT is restated as a rule shipped zagg satisfies, rather than dropped: "On the union arm a producer MUST NOT publish a narrowed word for a shard unless it derived that word from every leaf of the shard … The join itself satisfies this rule by construction, so the MUST-NOT binds a producer that composes the section by some other means." That keeps it useful for an external producer (moczarr writes the section from the same bytes) without condemning the writer in this tree.
- A new bullet says plainly what the overwrite arm does and what it costs:
The overwrite arm installs the incoming section verbatim. §10.4's wholesale overwrite … discards the standing section along with the stale ranges, so there is no standing word left to join against and the producer's own section is installed as written. A run-scoped producer that reaches that arm therefore publishes words derived from the leaves it visited, which for a shard whose other leaves it did not visit is narrower than the word that was standing. This arm is a carve-out from the MUST-NOT above, not a violation of it, and it is the one arm on which the section can lose a candidate rather than merely fail to prune one.
The closing paragraph is rewritten to name both losses, worse one first — narrowing ("a missed candidate, not a wasted open, and the one §10 failure a reader has no way to notice") ahead of dropping, which is the milder one.
On your option (2): changing write_root_coverage's overwrite arm is a behavioural change to shipped code, and phase 2 is a documentation issue (#487) — §6 reserves that scope call for @espg. It is now question (6) in the PR body, with your reproduction and three concrete options (leave-and-document / preserve the standing section across the arm / refuse the overwrite while a section is standing), so it is decided rather than absorbed.
Question (3) in the PR body is rewritten too — its old premise ("rather than a new obligation on any shipped producer") did not survive this finding, as you say.
No wire format, attrs grammar, or spec marker moved, so the §7 fixtures need no regeneration; tests/test_spec_conformance.py is 173 passed on the folded head.
Generated by Claude Code
| - **Only a whole-store walk tightens**, and only a whole-store walk refreshes | ||
| tier 2. Tightening is what re-derives a shard's word from what its leaves | ||
| hold *now* instead of joining it with what they held before, and tier 2's | ||
| whole-coverage rule (§10.4) already denies the digest to any partial | ||
| producer. A producer MUST NOT publish a **narrowed** word for a shard unless |
There was a problem hiding this comment.
🤖 from Claude (review)
HIGH — "only a whole-store walk refreshes tier 2" is false about the shipped merge, and it mis-cites §10.4 to justify itself.
Only a whole-store walk tightens, and only a whole-store walk refreshes tier 2. … tier 2's whole-coverage rule (§10.4) already denies the digest to any partial producer.
§10.4's rule is not whole-store, it is whole-merged-map:
It is replaced, and only by a producer whose own map covered every shard the merged map lists
and merge_temporal_sections implements exactly that, with no reference to the store:
listed = set(merged["shards"])
for side in (b, a):
if side.get("digest") is not None and set(side.get("shards") or {}) >= listed:
merged["digest"] = side["digest"]
breakSo any producer whose incoming map is a superset of the standing map installs its digest — which is the normal case for MocFamily.finish, not an exotic one: a fresh store's first sweep (standing map empty), and any incremental run that touched every shard the standing map lists. Reproduction against the shipped function, with tagged digests:
$ python repro2.py
merged digest: {'tag': 'sweep-digest'} # standing = whole-store refresh over {1111,1112}
fresh store : {'tag': 'sweep-digest'} # standing = None
In the first line a run-scoped sweep replaced a whole-store refresh's digest. Nothing about that is a whole-store walk.
Two knock-on inconsistencies:
- issue spec 10: document the walk-as-tightener refresh contract (staged sweep preserves, composition keeps claims true) #487's own wording is "The walk (
MocFamily/refresh_root_coverage) is the only tightener and the only digest refresher" — it countsMocFamilyas a digest refresher. §10.5 andcoverage.py's "its only tier-2 refresher" both narrow that torefresh_root_coveragealone, which neither the issue nor the code supports. section_unchanged's docstring already carries the same misreading a level down — "A producer that walked only part of the store always builds a digest, and §10.4 always drops that digest at the seam" — and thefor side in (b, a)loop above shows "always" is wrong there too. Promoting the misreading into a Contract heading is what makes it worth fixing rather than leaving.
Suggested fix: say what the code does — tier 2 is replaced (never unioned) by whichever side's map covers the merged map, newest first, so a run-scoped producer can and does install it; the whole-store walk is only the one that guarantees the digest describes the whole store. Then the third bullet is about tier 1 alone.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in c0e386b, and the code-side knock-on in coverage.py with it (c30ffee, see below). You are right on both halves: §10.4's rule is a test on the two maps at the seam, and for side in (b, a): … set(side["shards"]) >= listed installs a run-scoped producer's digest whenever its map covers the merged one — which is the fresh-store case, not an exotic one.
The third bullet is now tier 1 only, and tier 2 gets its own bullet stating the shipped rule:
Tier 2 is a weaker rule, and it is not the walk's alone. §10.4 replaces the digest with whichever side's map covers the merged map, newest side first. That is a test on the two maps meeting at the seam, not on the store: a run-scoped producer whose own map happens to cover the merged one — a fresh store's first sweep, where the standing map is empty, or any later run that touched every shard the standing map lists — does install its digest, and legitimately so, because it covered everything the composed map claims. What §10.4 buys is that the digest covers every shard the map lists; what the whole-store walk alone adds is that the map lists every shard the store holds. A reader MUST NOT read the second from the first — §10.2's escape hatch means an unlisted shard is unknown, so a digest can be whole over the map and still describe less than the store.
The tightener bullet now names refresh_root_coverage as zagg's only whole-store producer, which is the claim issue #487 actually supports, rather than the only digest refresher.
I dropped one sentence I had drafted for this bullet — "a reader that needs the store-wide guarantee reads source: "refresh"" — because it is not true either: merge_temporal_sections takes source from b, so a later run-scoped sweep composing over a refresh's section stamps "sweep" while the refresh's digest may still be installed. Better to state the map/store distinction and stop there than to hand a reader a signal that does not carry the weight.
Your knock-on (1) is folded in c30ffee (the reflow commit, since it rewrites exactly those lines): refresh_root_coverage's docstring said "THIS WALK IS THE SECTION'S ONLY TIGHTENER, and its only tier-2 refresher" and now reads
… THIS WALK IS THE SECTION'S ONLY TIGHTENER (spec §10.5, issue #487) …
It is NOT the only tier-2 refresher — §10.4 installs whichever side's map
covers the MERGED map, which a run-scoped sweep's map often does — but it
is the only producer whose map is guaranteed to list the whole store.
Knock-on (2) — section_unchanged's "always drops that digest at the seam" — is left standing: it is pre-existing text in a function this PR does not touch, so under CLAUDE.md §4 it is a flag, not a fix. It is the same misreading and worth a follow-up.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Correction to the sha in my reply above: the reflow commit carrying knock-on (1) is ce51de9, not c30ffee — I wrote that reply before the commit existed and quoted a sha I did not have. Nothing else in that reply changes; the docstring text quoted there is what ce51de9 contains.
The spec-side fix for this finding is c0e386b, as stated.
Generated by Claude Code
| THIS SEAM IS WHY NO CALLER HAS TO REFRESH (spec §10.5, issue #487). The | ||
| tier-1 join only ever GROWS an envelope, and growing is conservative in | ||
| the direction §10.2 needs, so every observation added through here lands | ||
| inside its shard's word by construction — the containment claim stays | ||
| true across any number of producers in any order, with no coordination. | ||
| Only a whole-store walk (:func:`zagg.coverage.refresh_root_coverage`) | ||
| TIGHTENS, and only it may narrow a word or replace the digest; a pass | ||
| that adds no observations may preserve the section untouched, which is | ||
| exactly what the staged sweep does | ||
| (:func:`zagg.sweep_stages.run_finisher`). |
There was a problem hiding this comment.
🤖 from Claude (review)
MEDIUM — this is the same overclaim e47e9d2 folded out of run_finisher, reintroduced here: the new paragraph is false on the branch documented three paragraphs above it in this very docstring.
Paragraph 2 of this docstring already says:
An unparsable or incompatible existing object is logged and OVERWRITTEN
and the new paragraph then asserts, unscoped:
The tier-1 join only ever GROWS an envelope … the containment claim stays true across any number of producers in any order, with no coordination.
Only a whole-store walk (:func:zagg.coverage.refresh_root_coverage) TIGHTENS, and only it may narrow a word or replace the digest
Both sentences are contradicted by this function's own code, ~30 lines below:
carried = isinstance(existing, dict) and merged is not envelope
section = merge_temporal_sections(
existing.get(TEMPORAL_KEY) if carried else None, envelope.get(TEMPORAL_KEY)
)On the overwrite arm carried is False, so there is no join: the incoming section is installed verbatim (merge_temporal_sections returns dict(b) when a is None). A caller passing a partial section therefore does narrow a word here, and does replace the digest here, without being a whole-store walk. I posted the end-to-end reproduction on docs/specification.md; the short version is that a query window matching the standing word stops matching the published one.
The fold e47e9d2 scoped run_finisher's preserve claim to "write_root_coverage's UNION arm" for precisely this reason — and run_finisher is the safe caller (it passes no section, so it can only drop). MocFamily.finish, which passes a run-scoped section, is the one that actually needs the warning, and it is the caller this paragraph is silent about.
Minimum fix: scope both sentences to the union arm ("on the UNION arm the tier-1 join only ever grows …"), and state what the overwrite arm does to a caller-supplied section. Better fix: make the overwrite arm not install a caller's section it cannot vouch for.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in d5b3d45. You are right that this is e47e9d2's overclaim wearing a different hat, and right that it landed on the safe caller's docstring the first time and the unsafe one's this time.
Both sentences are scoped, and the arm the paragraph was silent about now has its own paragraph naming the caller that actually reaches it:
THE UNION ARM IS WHY NO CALLER HAS TO REFRESH (spec §10.5, issue #487).
On that arm the tier-1 join only ever GROWS an envelope, and growing is
conservative in the direction §10.2 needs, so every observation added
through it lands inside its shard's word by construction …
THE OVERWRITE ARM ABOVE IS THE CARVE-OUT, and callers passing a section
have to know it: `carried` is False there, so nothing is joined and the
caller's own section is installed VERBATIM. A run-scoped caller —
:meth:`zagg.sweep.MocFamily.finish`, which builds its section from the
leaves this run visited — therefore publishes a word NARROWER than the
standing one for any shard whose other window leaves it did not visit,
and replaces the digest with its own. §10.5 names that arm's cost: a
narrowed word prunes a shard that does hold data in the window, a MISSED
candidate rather than a wasted open. It is bounded by D9 (the carrier is
a regenerable cache — the arm only runs when the standing carrier was
already unusable, and one refresh walk restores precision), but it is not
the join's guarantee, and nothing here re-derives it.
Note the paragraph is now adjacent to the "unparsable or incompatible existing object is logged and OVERWRITTEN" sentence it used to contradict, and refers back to it ("THE OVERWRITE ARM ABOVE") rather than reasserting past it.
Your "better fix" — making the overwrite arm not install a section it cannot vouch for — is a behavioural change to shipped code on a documentation issue (#487), so it is question (6) in the PR body for @espg rather than something I fold here (§6). Your three-option framing is carried into it verbatim, with the reproduction.
Generated by Claude Code
| escape hatch can never be the thing that deletes the section. THIS WALK IS | ||
| THE SECTION'S ONLY TIGHTENER, and its only tier-2 refresher (spec §10.5, |
There was a problem hiding this comment.
🤖 from Claude (review)
MEDIUM — the tightener itself can publish a word it did not derive from every leaf of the shard, so §10.5's new MUST-NOT is broken on the one path §10.5 blesses.
THIS WALK IS THE SECTION'S ONLY TIGHTENER, and its only tier-2 refresher
The walk's temporal accounting is guarded per shard by toc_failed, and that guard is only reached for leaves that get as far as read_leaf_temporal. Three earlier continues bypass it, and one of them can drop a sibling window leaf of a shard that still gets published:
try:
decimal, _window = split_leaf_name(name)
except ValueError:
logger.warning(
f"refresh: skipping STAMPED leaf {name!r} with a malformed "
f"window label (frozen grammar, mortie#62) — it will NOT be "
f"listed in {ROOT_COVERAGE_NAME}"
)
continuesplit_leaf_name splits on the first _ and then validate_labels the suffix, so 1111_bad$label.zarr raises while 1111_2020.zarr beside it reads fine. The shard id is right there in the name, but the branch discards it, so 1111 never enters toc_failed and never gets contributions.popped. With no other failure, toc_failed stays empty, the compose-with-standing path is skipped, and:
obstore.put(store, ROOT_COVERAGE_NAME, json.dumps(envelope, indent=1).encode())PUTs a word for shard 1111 derived from one of its two leaves, outright, superseding whatever the standing section held. That is exactly "publish a narrowed word for a shard … not derived from every leaf of the shard".
(The other two continues are fine: a foreign-order decimal and a non-D1 id are different shard ids, not siblings of a published one.)
This is arguably pre-existing under §10.2's "a producer whose read of any input behind a shard's word FAILED … MUST omit that shard" — but §10.5 promotes it from a latent gap to a stated MUST-NOT that the walk violates, while simultaneously naming the walk as the safe path. Either add the shard to toc_failed here (the name is parsable enough to recover the id up to the first _), or say in §10.5 that the tightener's skip carve-outs are a known bound on the guarantee.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Real bug, fixed in 45be6b1 with a test. Judged small, local and testable exactly as you scoped it: the id up to the first _ is recoverable, so the branch banks the shard the same way the read-failure branch below it does.
Reproduced first, on the committed temporal/ fixture — a standing word widened as a sibling window's data would have, plus a stamped 11213_bad$label.zarr beside 11213.zarr:
standing widened: 4102444799790673452 (2051222399273140224, 5344627255551197184)
published word: 10689250968998768172 (5344625483877187584, 5344627255551197184)
NARROWED (lost the standing extent): True
After the fix the same script publishes the widened word and logs the compose path (refresh: the temporal section is PARTIAL — 1 shard(s) did not read; composing with the standing section rather than replacing it).
The change:
if toc_fields:
# Only the LABEL is malformed: the id up to the first
# `_` is still readable, and a sibling window of that
# same shard reads fine and would be published alone.
# §10.2's word must cover every leaf of a LISTED shard,
# so bank the loss exactly as the read failure below
# does. …
lost = name.removesuffix(".zarr").split("_", 1)[0]
toc_failed.add(lost)
contributions.pop(lost, None)Two details worth stating:
- Gated on
toc_fields. Without the gate a non-temporal store's malformed leaf would maketoc_failednon-empty and drag the walk through the compose-with-standing path to emit a "temporal section is PARTIAL" warning about a channel the store does not have.test_refresh_skips_malformed_window_label(the non-temporal case) stays silent. - The
ValueErrorarm is unambiguous here.split_leaf_nameraises for a non-.zarrname or a bad label, and the caller already gated onname.endswith(".zarr"), so the only reachable raise is the bad label — which is why splitting on the first_recovers the same idsplit_leaf_namewould have returned.
New case TestOnCommittedStores::test_a_malformed_window_label_costs_its_shard_not_its_extent pins both the word and the query it protects (it fails on ab1b345):
assert int(envelope["temporal"]["shards"]["11213"]) == wide
lo, hi = far_at, far_at + DAY_NS
assert bool(np.asarray(toc_overlaps(np.array([wide], dtype=np.uint64), lo, hi))[0])
assert not bool(np.asarray(toc_overlaps(np.array([narrow], dtype=np.uint64), lo, hi))[0])Your parenthetical about the other two continues is recorded in the docstring, since it is the reason this branch is the only one that needs the treatment: "the two carve-outs below it name DIFFERENT shard ids, so neither can do that".
Generated by Claude Code
| The one loss this contract does not prevent is a producer that **drops** the | ||
| section instead of preserving it — a writer predating §10.4 that rebuilds the | ||
| root carrier from the keys it knows. The succession rule forbids it but cannot | ||
| bind code that shipped before the rule existed, so the loss is *bounded* | ||
| rather than prevented: the section is a regenerable accelerator (D9), a reader | ||
| that finds it absent falls back to opening every candidate — slow, and still | ||
| correct — and one walk restores it. | ||
|
|
There was a problem hiding this comment.
🤖 from Claude (review)
MEDIUM — inserting the ### 10.5 heading silently re-parented §10's closing conformance paragraph into §10.5, where its "claims above" now point at the wrong claims.
The paragraph immediately below this insertion is unchanged, but its scope changed. On a2d5d5b it closed §10.4 and served all of §10:
`spec` string at all) claims no revision and is debris a producer MAY
replace.
Conformance for an external reader is §7's `temporal/` fixture: … records the
shard word and the decoded digest so the containment and weight claims above
are pinned on committed bytes.
After 38da243 the new subsection sits between them, so that sentence now reads as the conformance statement for §10.5 — and the temporal/ fixture pins nothing in §10.5. It pins §10.2 containment and §10.3 weight, both of which are now two subsections further "above". A fixture of committed bytes cannot pin a producer-behaviour contract ("only a whole-store walk tightens") at all; there is no byte in temporal.expected.json that would change if a producer violated it.
This matters more than usual here because the paragraph is the page's own answer to CLAUDE.md §4 — the moczarr-decodes-from-spec-plus-fixtures rule — and PR question (3)'s justification for leaving the fixtures untouched rests on it. (I agree the fixtures need no change: no wire format, attrs grammar, or spec marker moved, and zagg-coverage-toc/1 is unchanged. The problem is only that the anchor sentence now sits under a subsection it does not describe.)
Fix: move the conformance paragraph back above ### 10.5, or add an explicit "(§10.2, §10.3)" so it does not read as §10.5's conformance.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in 7b78432, taking your first option and a bit of the second. The paragraph is moved back above ### 10.5, where it closes §10.4 and serves §10.1–§10.4 as it did on a2d5d5b, and the claims it pins are now named rather than left to "above":
Conformance for an external reader is §7's
temporal/fixture: its rootcoverage.moccarries this section, and the fixture'stemporal.expected.jsonrecords the shard word and the decoded digest so the containment and weight claims above (§10.2, §10.3) are pinned on committed bytes. §10.5 below is a contract on producer behaviour, and no fixture of committed bytes can pin one: its conformance is the prose.
The added second sentence is your observation about temporal.expected.json — there is no byte in it that would change if a producer violated §10.5 — stated on the page rather than only in this thread, so the next person to add a subsection under §10 knows which side of the line theirs falls on before they move the anchor again.
Agreed on the fixtures needing no change, and that is now grounded on the page instead of on a paragraph that had drifted under the wrong heading.
Generated by Claude Code
| lives at the leaf's OWN node. A temporal-declaring store (spec §10, issue #480) also has | ||
| its ``zagg-coverage-toc/1`` section rebuilt from this same walk — | ||
| fail-open per SHARD, so an unreadable companion costs the section that | ||
| shard and never the refresh; and a walk that lost any shard COMPOSES its | ||
| rebuild with the standing section (§10.4) instead of replacing it, so the | ||
| escape hatch can never be the thing that deletes the section. A successful | ||
| escape hatch can never be the thing that deletes the section. THIS WALK IS | ||
| THE SECTION'S ONLY TIGHTENER, and its only tier-2 refresher (spec §10.5, | ||
| issue #487): every other producer composes through | ||
| :func:`zagg.hive.write_root_coverage`'s join, which only ever GROWS an | ||
| envelope, so their claims stay true without ever getting narrower. Nothing | ||
| NEEDS this call for correctness — an un-refreshed section is over-wide at | ||
| worst, costing a wasted candidate open and never a missed one — which is | ||
| what lets a no-observation pass preserve the section instead | ||
| (:func:`zagg.sweep_stages.run_finisher`); refresh buys PRECISION, and | ||
| repairs a section a pre-§10.4 producer dropped | ||
| (:func:`zagg.coverage_toc.warn_if_section_missing` names it for that). A successful |
There was a problem hiding this comment.
🤖 from Claude (review)
LOW — the edit left three un-reflowed seams in a docstring that is otherwise wrapped at ~72–78 columns.
Measured widths in the current file:
256 76 ``{stem}.pyramid.zarr`` column (issue #383), the one derived family that
257 92 lives at the leaf's OWN node. A temporal-declaring store (spec §10, issue #480) also has
258 69 its ``zagg-coverage-toc/1`` section rebuilt from this same walk —
…
271 50 repairs a section a pre-§10.4 producer dropped
272 87 (:func:`zagg.coverage_toc.warn_if_section_missing` names it for that). A successful
273 28 refresh also re-arms the
Lines 257 and 272 are the two splice points where the removed merge artifact and the added sentence were stitched in, and 271/273 are the short remainders. ruff format will not touch docstring prose and E501 is ignored, so nothing flags it — but CLAUDE.md §4's "match the surrounding code" applies, and this block is the most-read prose in the module. One reflow of lines 256–273 clears it.
Separately, for the record: I confirmed the deleted fragment really was debris and no sentence lost meaning. git log -S "supersedes it). A" -- src/zagg/coverage.py returns 28f2fce (introduced) and 38da243 (removed), and the intact clause it duplicated still stands at line 251–252 — "REPLACES the root object (no union: the walk supersedes it)". Good removal.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Reflowed in ce51de9. The whole block from "lives at the leaf's OWN node" through the end of the docstring is rewrapped; the two 92/87-column seams and the 50/28-column remainders they left are gone, and every line the fold touched is now 61–78 columns, matching the paragraphs above it.
Two things went in with the reflow rather than beside it, because they rewrite the same lines:
-
c0e386b's code-side knock-on. The claim "THIS WALK IS THE SECTION'S ONLY TIGHTENER, and its only tier-2 refresher" is the same misreading of §10.4 you filed on the spec, so it is corrected here:… THIS WALK IS THE SECTION'S ONLY TIGHTENER (spec §10.5, issue #487): every other producer composes through the UNION arm of :func:`zagg.hive.write_root_coverage`, whose join only ever GROWS an envelope, so their claims stay true without ever getting narrower. It is NOT the only tier-2 refresher — §10.4 installs whichever side's map covers the MERGED map, which a run-scoped sweep's map often does — but it is the only producer whose map is guaranteed to list the whole store.Note
write_root_coverage's reference moved to the start of a clause ("the UNION arm of :func:…" rather than ":func:…'s UNION arm") specifically so the long role token no longer forces a short remainder line. -
The malformed-label sentence from
45be6b1is wrapped into the same block rather than appended past it.
Line 254 stays at 80 columns (`.zarr` off the shard-order depth is checked for the D11roleattr and) — pre-existing, untouched by this PR, left alone per CLAUDE.md §4.
Thanks for the git log -S "supersedes it). A" check on the deleted fragment — good to have that recorded on the thread rather than resting on my reading of it.
One correction to my reply on the docs/specification.md tier-2 thread: I cited this commit there as c30ffee before it existed. The correct sha is ce51de9 — this one.
Generated by Claude Code
|
🤖 from Claude Status: both phases landed, both adversarial reviews folded (12 findings, 12 commits), CI green on
The phase-2 review is why this took two rounds, and it was right. §10.5 as first written asserted an invariant that shipped zagg violates: it described §10.4's seam as if the tier-1 join were its only arm. On the overwrite arm That is a missed candidate, not a wasted open — the opposite of what §10.2 promises, and strictly worse than the dropped-section case #488 exists to nudge about. §10.5 now says so plainly instead of claiming it cannot happen. Why Questions (1)–(5) are review questions rather than blockers — the PR is coherent under either answer to each. One scope note worth surfacing rather than burying: The draft→ready flip is not taken, per the standing conflict recorded on issue #50 (CLAUDE.md §2 has the author flip it once phases are complete, findings folded and CI green — all true here; the routine prompt forbids it outright). Generated by Claude Code |
Two small fixes, both on the spec §10 temporal section, both downstream of the same ruling (
preserve-don't-refreshis the staged sweep's permanent posture).Closes #488
Closes #487
Phases
a2d5d5b): warn when a temporal-declaring store's rootcoverage.moccarries no §10 section.38da243): document the walk-as-tightener refresh contract in §10 + the three docstrings the issue names.9d05ae6,8be2cec,e47e9d2,b9a3d66,e953364,ab1b345); every inline thread replied to.2e8bf99,c0e386b,d5b3d45,45be6b1,7b78432,ce51de9); every inline thread replied to.Phase 1 — the nudge (issue #488)
New
zagg.coverage_toc.warn_if_section_missing(store_root, envelope, manifest), wired intosweep_stages.run_finisheron the payloadwrite_root_coverageactually returned, surfaced astoc_section_missingin the finisher's summary dict (always present, whatever the work set).The gap it covers is the one §10.4's succession rule cannot close: the rule binds producers that know it, and a pre-#481 producer rebuilds the root envelope from the keys it knows and drops the section it never learned to copy. Severity is bounded by D9 — the section is a regenerable accelerator, so a reader that loses it falls back to opening every candidate: slow, correct, and fully repaired by one
refresh_root_coveragewalk. So this is a logged nudge naming that remedy, and explicitly not a locking scheme (the #208 no-lock ruling stands), exactly as the issue frames it.Placed at the staged-sweep finisher and nowhere else, deliberately: that is the one root-writing seam where a section can go missing without anyone noticing. The finisher adds no observations, so it preserves rather than rebuilds (issue #487), and
MocFamily.finish— the walk — writes the section itself, so telling its operator to run a walk would be circular advice.The message asserts no cause, which the phase-1 review corrected. An absent section is an observation, and two causes reach it that this seat cannot tell apart: a producer dropped it, or no walk has built one yet (only the walk writes a section —
MocFamily.finishandrefresh_root_coverageare its two writers). In the default pipeline the walk does precede the staged finisher (runner.pyrunssweep_after_runoverDEFAULT_FAMILIES,mocamong them, beforestage_sweep_after_runin the same post-run hook), so a section missing at the finisher is genuinely anomalous there — but not for a store swept with--stagesstandalone, nor one whose fail-open families sweep failed. One remedy fixes all of them, so the line names both causes and blames neither:Detection is declaration-driven, like every other reach for the temporal channel in this module (
temporal_fields's standing rule: never a member enumeration of the leaf), so the check is free — no leaf is opened. See question (1). A standing section at an unknown revision is not "missing": §10.4 preserves it verbatim, so nothing was dropped; only an absent key or an unmarked carrier (debris, by_preserved's own rule) warns.Phase 2 — the refresh contract (issue #487)
A new normative
### 10.5 Refresh contract — the walk is the only tightenerindocs/specification.md, stating the always-safe, eventually-precise invariant as bullets, plus the losses it bounds rather than prevents.Prose only — no grammar, wire format, or
specmarker changed, so the §7 conformance fixtures (tools/generate_spec_fixtures.py→tests/data/spec/) are untouched andtests/test_spec_conformance.pystays green (173 passed).Cross-referencing docstring lines on the three functions the issue names:
sweep_stages.run_finisher(landed with phase 1, since the finisher's preserve posture is what the check depends on),hive.write_root_coverage, andcoverage.refresh_root_coverage. While editing the last one I also removed a dangling merge artifact sitting in it — the fragmentlives at the leaf's OWN node. A/supersedes it)., a stray duplicate of the(no union: the walk supersedes it)clause seven lines above. An earlier pass worked around it rather than fixing it (9ed4cd0, "keep the new refresh sentence clear of the pre-existing fragment"); it is in the very docstring this issue names, so it is fixed here.Corrected by the phase-2 fold — §10.5 as first written described only one of §10.4's two arms.
write_root_coveragereaches the tier-1 join only on the union arm; on the overwrite armcarriedisFalse, so the call ismerge_temporal_sections(None, incoming), which returnsdict(incoming)and installs the producer's section verbatim with notoc_mergeanywhere. The review reproduced a standing 2019–2021 word being replaced by a 2021-only one, flipping a January-2019toc_overlapsquery fromTruetoFalse. §10.5 now scopes the join's guarantee to the union arm, gives the overwrite arm its own bullet naming what it costs, restates the MUST-NOT as a union-arm rule that shipped zagg satisfies, and names narrowing as the worse of the two losses the contract bounds — a missed candidate, not a wasted open — ahead of dropping. Tier 2's rule is restated as §10.4 actually writes it (whole-merged-map, not whole-store), withrefresh_root_coveragenamed as the only whole-store producer rather than the only digest refresher. Still prose only: no wire format, attrs grammar, orspecmarker moved, so the §7 fixtures still need no regeneration. See question (6) for the behavioural question this exposes, which is not decided here.Phase-1 review folds
Six findings, one commit each, every thread replied to. Two were substantive:
9d05ae6— the warning asserted a cause it cannot know ("a producer that predates §10.4 may have dropped it"). Now names both causes and blames neither; the test that builds exactly the never-walked store had its docstring corrected to say so.e47e9d2—run_finisher's docstring claimed "a section missing here was dropped by someone else", which is false onwrite_root_coverage's overwrite arm: an unparsable or incompatible standing root is discarded by design (D9) and the stale section goes with the stale ranges, so on that arm this very call is the dropper. Docstring scoped to the union arm, andtest_an_incompatible_standing_root_drops_the_section_and_nudgesnow pins the overwrite arm end to end.Plus
8be2cec(seedtoc_section_missingin theoutliteral so the stage record's shape does not depend on the work set, + a test),b9a3d66(read the store's re-read manifest, not the caller's admission-time copy, + a test that fails on the old ordering),e953364(consult_preservedbefore_usable, so the arm that honors a future revision does not also emit_usable's "ignoring a section with an unknown spec"),ab1b345(reuseconftest.toc_wordsinstead of a second hand-rolledtime2toccall — which, it turned out, had been packing a different instant).Phase-2 review folds
Six findings, one commit each, every thread replied to. All six landed on phase 2; five are prose, one is a real code bug.
2e8bf99— §10.5's new MUST-NOT ("a producer MUST NOT publish a narrowed word … unless it derived it from every leaf") was violated by shipped zagg, because §10.4's overwrite arm is not a join (see the phase-2 note above). Scoped the contract to the union arm, added an overwrite-arm bullet, restated the MUST-NOT as one the code satisfies, and rewrote the closing paragraph to name narrowing as the worse loss.c0e386b— "only a whole-store walk refreshes tier 2" was false.merge_temporal_sectionsinstalls whichever side's map covers the merged map (for side in (b, a): … set(side["shards"]) >= listed), which a fresh store's first sweep and any run touching every listed shard both satisfy. Tier 2 now has its own bullet stating the map-versus-store distinction.d5b3d45—write_root_coverage's new docstring paragraph reintroduced the overclaime47e9d2had just removed fromrun_finisher, three paragraphs below the same docstring's own "OVERWRITTEN" sentence. Scoped to the union arm, with a second paragraph namingMocFamily.finishas the run-scoped caller that reaches the other one.45be6b1— code bug, with a test.refresh_root_coverage's malformed-window-labelcontinuediscarded a stamped leaf without recording its shard intoc_failed, so a sibling window of the same shard was published alone — the tightener itself narrowing a word. Now banks the shard (the id up to the first_is recoverable) so the compose-with-standing path engages, mirroring the read-failure branch below it. New casetest_a_malformed_window_label_costs_its_shard_not_its_extentfails onab1b345.7b78432— inserting### 10.5had re-parented §10's closing conformance paragraph under it, where "the claims above" no longer named what the fixture pins. Moved back to close §10.4, with "(§10.2, §10.3)" made explicit and a sentence saying why no fixture can pin §10.5.ce51de9— reflowed the un-reflowed 92- and 87-column seams the phase-2 edit left inrefresh_root_coverage's ~75-column docstring, and foldedc0e386b's code-side knock-on ("its only tier-2 refresher") into the same rewrite of those lines.One sub-point was declined and stands for review:
section_unchanged's docstring carries the same tier-2 misreading a level down ("A producer that walked only part of the store always builds a digest, and §10.4 always drops that digest at the seam"). It is pre-existing text in a function this PR does not touch, so under CLAUDE.md §4 it is flagged rather than fixed here.How it was tested
Twelve unit cases in
tests/test_coverage_toc.py::TestMissingSectionWarningand six intests/test_sweep_stage.py::TestFinisher, covering: the warn arm and both causes named in the line, the no-declaration arm, the section-present arm, the future-revision arm (silent at DEBUG too), four debris shapes, an empty work set, a stale caller manifest, the incompatible-standing-root overwrite arm, the preserve arm with a standing section installed, and an end-to-end pass on the committedtemporal/fixture that strips the section by PUTting the envelope outright — the pre-§10.4 clobber shape — asserts the warning, runs the named remedy, and asserts it stops. Phase 2's fold adds one case on thetemporal/fixture pinning that a skipped malformed-label sibling costs its shard's precision and never its containment.pytest -q --deselect tests/test_lambda_build.py -p no:randomlyonce51de9→ 4511 passed, 38 skipped (4510 onab1b345; 4507 on38da243).ruff check --select=E,F,W,I --ignore=E501 src testsandruff format --checkon the touched files → clean.tests/test_spec_conformance.py→ 173 passed, unchanged: no fixture was regenerated because no wire format, attrs grammar, orspecmarker moved.Questions for review
Is the declaration-driven detection the right bound, or should the check open one leaf? A store that declares
temporal: "per-centroid"but whose leaves were all written before the declaration existed warns on every staged sweep, forever, and the walk it names will never produce a section for it. Options: (a) what is implemented — free, declaration-only, accepts that false positive; (b) additionally require the finisher'sby_shardto be non-empty; (c) open one leaf's declared sibling to confirm a companion really exists, at one GET per finish. (a) matches the module's standing "declaration is the discovery" rule; (c) matches the issue's literal wording ("when a store's leaves carry temporal companions"). Left at (a).Should the same nudge fire on the reader side? The issue names a moczarr sibling issue (same detection, reader's seat), but zagg's own readers (
load_temporal_coverage,shards_overlapping) returnNonesilently by §10's absence posture, which is correct for them. Not touched here.§10.5 is written as a
Contract, not as informative. Its one MUST-NOT is now scoped to §10.4's union arm ("on the union arm a producer MUST NOT publish a narrowed word for a shard unless it derived that word from every leaf of the shard"), which the join satisfies by construction — so it binds an external producer composing the section by other means (moczarr), not any shipped zagg writer. Its original unscoped form was violated by shipped zagg, which the phase-2 review caught; see the phase-2 note and question (6). If you would rather §10.5 be purely informative, the fix is one heading word and dropping that MUST-NOT.Pre-existing, flagged not fixed (CLAUDE.md §4):
ruff check src testsunder the repo's own config (select = [E, F, W, I, N]) reportsN818onsrc/zagg/registry.py:64(UnknownCapabilitywants anErrorsuffix), andruff format --checkwants to reformat a python block insidetests/data/benchmark/README.md. Both are onmainunchanged and neither is in CI's gate (the lint bot runs--select=E,F,W,I).One flaky test observed, not reproduced. The first full-suite run on phase 1 failed
tests/test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries; three later full runs and a targeted run all pass, and the diff touches nothing in that path. Recording it rather than calling it clean.Should
write_root_coverage's overwrite arm keep discarding the temporal section? Raised by the phase-2 review; behavioural, pre-existing, and deliberately not decided in this PR. The arm was written to treat the section like the ranges — stale carrier, stale section, both discarded under D9. But the two are not symmetric: a discarded section costs a reader a slow query (absent shard = unknown = candidate), while a section replaced by a run-scoped producer's narrower one costs a missed candidate, which no reader can detect. Reproduced against the shipped writer:The trigger set is real but narrow: an unparsable root, an incompatible
spec/encoding/order, or arangesbody that failsroot_coverage_words— combined with a run-scoped producer (MocFamily.finish) that visited a subset of a windowed shard's leaves.Options:
existing[TEMPORAL_KEY]through even when the carrier is discarded, on the grounds that the section's succession rule (§10.4) is independent of the ranges' validity: the ranges failing to parse says nothing about whether the temporal words are still true. Small change (carriedstops gating theexisting.get(TEMPORAL_KEY)argument), but it means a genuinely corrupt carrier can hand its section to the replacement.My read is (b) if the arm should change at all — it is the option whose reasoning is about the section rather than about the carrier — but it is a behaviour change on shipped code raised on a docs issue, so it needs a maintainer call rather than mine (CLAUDE.md §6). No issue opened for it, per the same rule.
Process note
The phases were worked slightly out of the §2 order: phase 2 (docs only) was written while the phase-1 adversarial review was still running, rather than strictly after folding it. All six phase-1 findings landed against phase-1 code and tests and none in a file phase 2 touched, so nothing was folded onto a moved target — but the deviation is recorded rather than left silent. The phase-2 review ran against the folded head.