Skip to content

small fixes 2026-08-19: sweep nudge for a dropped §10 section; the walk-as-tightener refresh contract - #491

Draft
espg wants to merge 14 commits into
mainfrom
claude/small-fixes-2026-08-19
Draft

small fixes 2026-08-19: sweep nudge for a dropped §10 section; the walk-as-tightener refresh contract#491
espg wants to merge 14 commits into
mainfrom
claude/small-fixes-2026-08-19

Conversation

@espg

@espg espg commented Aug 19, 2026

Copy link
Copy Markdown
Member

Two small fixes, both on the spec §10 temporal section, both downstream of the same ruling (preserve-don't-refresh is the staged sweep's permanent posture).

Closes #488
Closes #487

Phases

Phase 1 — the nudge (issue #488)

New zagg.coverage_toc.warn_if_section_missing(store_root, envelope, manifest), wired into sweep_stages.run_finisher on the payload write_root_coverage actually returned, surfaced as toc_section_missing in 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_coverage walk. 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.finish and refresh_root_coverage are its two writers). In the default pipeline the walk does precede the staged finisher (runner.py runs sweep_after_run over DEFAULT_FAMILIES, moc among them, before stage_sweep_after_run in the same post-run hook), so a section missing at the finisher is genuinely anomalous there — but not for a store swept with --stages standalone, nor one whose fail-open families sweep failed. One remedy fixes all of them, so the line names both causes and blames neither:

… 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(…)

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 tightener in docs/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 spec marker changed, so the §7 conformance fixtures (tools/generate_spec_fixtures.pytests/data/spec/) are untouched and tests/test_spec_conformance.py stays 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, and coverage.refresh_root_coverage. While editing the last one I also removed a dangling merge artifact sitting in it — the fragment lives 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_coverage reaches the tier-1 join only on the union arm; on the overwrite arm carried is False, so the call is merge_temporal_sections(None, incoming), which returns dict(incoming) and installs the producer's section verbatim with no toc_merge anywhere. The review reproduced a standing 2019–2021 word being replaced by a 2021-only one, flipping a January-2019 toc_overlaps query from True to False. §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), with refresh_root_coverage named as the only whole-store producer rather than the only digest refresher. Still prose only: no wire format, attrs grammar, or spec marker 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.
  • e47e9d2run_finisher's docstring claimed "a section missing here was dropped by someone else", which is false on write_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, and test_an_incompatible_standing_root_drops_the_section_and_nudges now pins the overwrite arm end to end.

Plus 8be2cec (seed toc_section_missing in the out literal 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 _preserved before _usable, so the arm that honors a future revision does not also emit _usable's "ignoring a section with an unknown spec"), ab1b345 (reuse conftest.toc_words instead of a second hand-rolled time2toc call — 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_sections installs 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.
  • d5b3d45write_root_coverage's new docstring paragraph reintroduced the overclaim e47e9d2 had just removed from run_finisher, three paragraphs below the same docstring's own "OVERWRITTEN" sentence. Scoped to the union arm, with a second paragraph naming MocFamily.finish as the run-scoped caller that reaches the other one.
  • 45be6b1code bug, with a test. refresh_root_coverage's malformed-window-label continue discarded a stamped leaf without recording its shard in toc_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 case test_a_malformed_window_label_costs_its_shard_not_its_extent fails on ab1b345.
  • 7b78432 — inserting ### 10.5 had 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 in refresh_root_coverage's ~75-column docstring, and folded c0e386b'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::TestMissingSectionWarning and six in tests/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 committed temporal/ 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 the temporal/ 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:randomly on ce51de94511 passed, 38 skipped (4510 on ab1b345; 4507 on 38da243).
  • ruff check --select=E,F,W,I --ignore=E501 src tests and ruff format --check on the touched files → clean. tests/test_spec_conformance.py → 173 passed, unchanged: no fixture was regenerated because no wire format, attrs grammar, or spec marker moved.

Questions for review

  1. 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's by_shard to 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).

  2. 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) return None silently by §10's absence posture, which is correct for them. Not touched here.

  3. §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.

  4. Pre-existing, flagged not fixed (CLAUDE.md §4): ruff check src tests under the repo's own config (select = [E, F, W, I, N]) reports N818 on src/zagg/registry.py:64 (UnknownCapability wants an Error suffix), and ruff format --check wants to reformat a python block inside tests/data/benchmark/README.md. Both are on main unchanged and neither is in CI's gate (the lint bot runs --select=E,F,W,I).

  5. 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.

  6. 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:

    existing coverage.moc at /tmp/… 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 trigger set is real but narrow: an unparsable root, an incompatible spec/encoding/order, or a ranges body that fails root_coverage_words — combined with a run-scoped producer (MocFamily.finish) that visited a subset of a windowed shard's leaves.

    Options:

    • (a) Leave as-is and document it — what this PR does. §10.5 names the arm, states that it installs the incoming section verbatim, and names the missed-candidate cost. Cheapest, changes no behaviour, and stays inside issue spec 10: document the walk-as-tightener refresh contract (staged sweep preserves, composition keeps claims true) #487's documentation scope. The loss stays bounded by D9 and one refresh walk repairs it.
    • (b) Preserve the standing section across the overwrite arm — carry 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 (carried stops gating the existing.get(TEMPORAL_KEY) argument), but it means a genuinely corrupt carrier can hand its section to the replacement.
    • (c) Refuse the overwrite outright while a section is standing — fail loud instead of silently narrowing. Safest for the reader, but it breaks the D9 "the sweep is the authoritative rebuilder, never die on garbage" posture the arm exists to hold, and would strand exactly the stores the escape hatch is for.

    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.

@espg espg added the implement label Aug 19, 2026

@espg espg left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Adversarial review of 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:

  1. 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.finish and refresh_root_coverage do), 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.
  2. MEDIUMtoc_section_missing is the only key in the finisher's out that is not pre-seeded; on an empty by_shard it is absent from the run record entirely, contradicting the docstring that surfaces it unconditionally, and no test covers that branch.
  3. 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).
  4. LOW — the check reads the possibly-stale manifest argument one line before the finisher re-reads fresh from the store for the same reason.
  5. LOW_usable's DEBUG "ignoring a section with an unknown spec" fires on the arm that deliberately honors the section, and the new caplog.text == "" assertions lean on that line staying below capture level.
  6. LOW_standing_section re-implements conftest.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

Comment thread src/zagg/coverage_toc.py Outdated
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 "

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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

Comment thread src/zagg/sweep_stages.py Outdated
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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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:

  1. 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 and false;
  2. run_finisher's own docstring says the nudge is surfaced "at toc_section_missing" with no qualifier, so a reader indexing summary["finisher"]["toc_section_missing"] gets a KeyError. In-repo callers happen to use .get, so nothing crashes today — it is the published shape that is wrong;
  3. no test covers the empty-by_shard branch, 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 False

The 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

Comment thread src/zagg/sweep_stages.py Outdated
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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 envelope

So 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 True

It 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

Comment thread src/zagg/sweep_stages.py
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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 True

Generated by Claude Code

Comment thread src/zagg/coverage_toc.py Outdated
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:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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

Comment thread tests/test_sweep_stage.py Outdated

contributions = {}
for i, dec in enumerate(decimals):
t = int(np.datetime64(TOC_BASE, "ns").astype("int64")) + i * 10**12

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 espg left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

Adversarial review of 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 TrueFalse, 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:

  1. HIGH (docs/specification.md 2469–2485) — the new MUST-NOT is violated by shipped code on write_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.
  2. HIGH (docs/specification.md 2480–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.
  3. MEDIUM (src/zagg/hive.py 1082–1091) — the same overclaim e47e9d2 folded out of run_finisher is reintroduced here, contradicting the overwrite branch documented three paragraphs above it in the same docstring.
  4. MEDIUM (src/zagg/coverage.py 262–263) — the tightener itself can publish a word not derived from every leaf: split_leaf_name's malformed-window-label continue drops a sibling leaf without entering toc_failed, so the whole-store PUT proceeds.
  5. MEDIUM (docs/specification.md 2492–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 the temporal/ fixture pins.
  6. LOW (src/zagg/coverage.py 257–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_merge really does only grow. 40k random pairs over time2toc/span2toc words: the merged word's toc2time envelope 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" gives 28f2fce (introduced) → 38da243 (removed), and the clause it duplicated still stands intact at coverage.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_run all exist and are :func:; zagg.sweep.MocFamily.finish is correctly :meth: and zagg.sweep.DEFAULT_FAMILIES correctly :data:. (mkdocs is not installed in .venv, so I could not render — checked by name resolution instead.)
  • 9d05ae6's pipeline-ordering claim is accurate. runner.py:3336-3348 runs sweep_after_run and only then stage_sweep_after_run, inside the same if 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 after fresh = read_manifest is correct and written is always a dict on that path; e953364's _preserved-before-_usable reorder is right, since _usable logs at DEBUG for any non-matching spec; ab1b345's toc_words reuse matches _located_leaf_slabs' base.
  • CLAUDE.md §3 — all seven messages are title-only and match git log style.
  • tests/test_coverage_toc.py + tests/test_sweep_stage.py: 135 passed. ruff check and ruff format --check report only the two pre-existing items already flagged in question (4) (N818 in registry.py, a code block in tests/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, or spec marker 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

Comment thread docs/specification.md Outdated
Comment on lines +2469 to +2485
- **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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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:

  1. 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
  2. either fix write_root_coverage to drop envelope[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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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

Comment thread docs/specification.md Outdated
Comment on lines +2480 to +2484
- **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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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"]
        break

So 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:

  1. 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 counts MocFamily as a digest refresher. §10.5 and coverage.py's "its only tier-2 refresher" both narrow that to refresh_root_coverage alone, which neither the issue nor the code supports.
  2. 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 the for 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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

Comment thread src/zagg/hive.py Outdated
Comment on lines +1082 to +1091
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`).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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

Comment thread src/zagg/coverage.py Outdated
Comment on lines +262 to +263
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,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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}"
    )
    continue

split_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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 make toc_failed non-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 ValueError arm is unambiguous here. split_leaf_name raises for a non-.zarr name or a bad label, and the caller already gated on name.endswith(".zarr"), so the only reachable raise is the bad label — which is why splitting on the first _ recovers the same id split_leaf_name would 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

Comment thread docs/specification.md Outdated
Comment on lines +2492 to +2499
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 root coverage.moc carries this section, and the fixture's temporal.expected.json records 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

Comment thread src/zagg/coverage.py Outdated
Comment on lines +257 to +272
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude (review)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 from Claude

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 45be6b1 is 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

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

espg commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Status: both phases landed, both adversarial reviews folded (12 findings, 12 commits), CI green on ce51de9. Marked waiting — question (6) is a behavioural fork I should not pick.

  • CI: ruff, build, build / build-x86_64, build / build-arm64 all pass on the pushed head (run 32253024944); test (3.12) / test (3.13) still running at the time of writing.
  • Local on the same head: pytest -q --deselect tests/test_lambda_build.py -p no:randomly4511 passed, 38 skipped, run independently of the fold agent's own pass and agreeing with it. ruff check --select=E,F,W,I --ignore=E501 src tests clean.
  • Reviews: two fresh-context adversarial passes, six findings each, every one replied to on its thread. Eleven folded, one declined with rationale (section_unchanged's docstring carries the same tier-2 misreading a level down — pre-existing text in a function this PR does not touch, so flagged rather than fixed under §4).

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 carried is False, so merge_temporal_sections(None, incoming) returns the incoming section verbatim — no join — and a run-scoped producer can therefore replace a wider standing word with a narrower one. Reproduced against the shipped writer:

standing word : 3092601599982677469 (2019-01-01 … 2021-12-31)
published word: 3218745596920049117 (2021-01-01 … 2021-12-31)
2019 window overlaps standing word : True
2019 window overlaps published word: False

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 waiting rather than nothing-to-continue. Question (6) asks whether that arm should keep discarding the section at all. It is pre-existing behaviour, the trigger set is narrow (an unparsable or incompatible standing carrier, plus a run-scoped producer that visited a subset of a windowed shard's leaves), and three options are laid out in the description with the reproduction. (a) is what is implemented — document it, change nothing. My read is (b) if the arm should change at all, since the section's succession rule is about the section rather than about the ranges' validity, but that is a behaviour change on shipped code raised on a documentation issue, so it is a call for the maintainer and not for this PR. No issue was opened for it (§6).

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: 45be6b1 is a code change on an otherwise-documentation phase. The phase-2 review found that refresh_root_coverage — the tightener itself — could narrow a shard's word, because its malformed-window-label branch skipped a stamped leaf without banking the shard in toc_failed, so a sibling window of the same shard was published alone. It is in the function issue #487 names, the fix mirrors the read-failure branch directly below it, and it comes with a test that fails on the prior head. Folding it rather than filing it seemed right, but it is the one place this PR reaches past documentation.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants