small fixes 2026-08-26: refine's silent granule drop; reproject provenance passthrough; mypy over .github/scripts

[espg]

Bundled small-fix PR for three independent issues.

Closes #512
Closes #517
Closes #514

Phases

• Phase 1 — refine drops a granule from legacy identity-colliding shard maps loaded via from_json (silent loss upstream of the #482 guard) #512: reproject's refine arm no longer silently drops a granule from a legacy identity-colliding shard map.
• Phase 2 — ShardMap.reproject drops per-entry pairing provenance (closest-obs epoch/offset keys) #517: reproject carries per-entry pairing provenance (paired_epochs / epoch_offsets_ns) through all three arms.
• Phase 3 — mypy: cover .github/scripts/ — bench_metrics.py now holds the re-pin driver's engine untyped-checked #514: mypy's pre-commit scope widened to .github/scripts/, bench_metrics.py annotated.

---

Phase 1 — #512: the refine seam's silent granule drop

What was wrong. ShardMap.reproject's refine arm rebuilt each entry from the catalog record alone:

entry = _granule_entry(sub_records[i])
bucket[_recorded_identity(entry)[1]] = entry

sub_records[i] is records_by_id[gid] (plus the assets overlay), so a legacy pair of two distinct entries sharing one granule id under two href prefixes — legal in a map built before the #468/#482 construction-time guard, and loadable today because from_json/from_parquet only warn (shardmap.py:329) — arrived as one identical entry. Both then hashed to the same _recorded_identity(...)[1] bucket slot and collapsed into one. The loss happened before _refuse_basename_collisions(new_keys, new_granules) a few lines down, so the guard never saw the collision and the granule count dropped by one, quietly.

The fix (candidate (b) on the issue — carry the map's own hrefs through the refine so the collision survives into the refined map, where the guard can see it). One line:

entry = _granule_entry({**sub_records[i], **gran_list[i]})

The MAP entry's own keys win over the catalog record's. _granule_entry still filters to its own fixed key set, so nothing else about the entry shape changes.

On the bucket key. The bucket stays keyed on _recorded_identity(entry)[1] — the distinguishing tuple (id, s3, https, datetime) — rather than moving to the granule id. Keying on id would not fix this case and would make it worse: the collided pair here shares one id (Gdup under p1/ and p2/), and the raster form of the same collision (issue #468 review finding (1)) is two acquisitions sharing an item id, separated only by datetime. Both collapse under an id-keyed bucket. Keeping the distinguishing-tuple key also keeps refine consistent with the coarsen arm, which already keys the same way and for the same stated reason (shardmap.py:1847-1857). The de-dup that key exists for — the same granule reaching one target shard twice — is unaffected: identical entries still produce an identical key.

Normal maps are byte-identical. A non-colliding map's entries were themselves produced by _granule_entry over these same catalog records, so the overlay is a no-op. Pinned directly by test_refine_of_a_clean_map_is_unchanged_by_the_href_carry, which asserts a refined map equals the direct build shard-key-for-shard-key and granule-for-granule.

Stale text corrected. _warn_loaded_collisions's docstring and its operator-facing warning both told the reader that "refine silently drops a collided member"; that is no longer true, so both now say reproject refuses rather than derives.

Pre-fix loss confirmed to reproduce. Before the fix, a temporary pin asserting today's behaviour passed:

$ uv run pytest -q tests/test_shardmap.py -k "refine_loses_a_legacy"
1 passed, 161 deselected in 0.88s

That pin asserted that a legacy colliding map written with to_json, loaded back through from_json (warning, not refusing), and refined came out holding one entry per shard where two went in, with both prefixes rewritten to the catalog's single s3://b/Gdup.h5. It is now flipped to pytest.raises(ValueError, match="identity collision") and lives as test_refine_of_a_loaded_legacy_collision_never_changes_the_count_silently.

Tests changed. tests/test_shardmap.py::TestBasenameCollisions:

test	change
test_refine_rebuilds_hrefs_from_the_catalog_so_it_cannot_collide	flipped and renamed test_refine_carries_the_map_hrefs_so_a_collision_survives_to_the_guard — it pinned exactly the bound this issue removes, and now asserts the refusal names both prefixes
test_refine_of_a_loaded_legacy_collision_never_changes_the_count_silently	new — the issue's acceptance path, via to_json → from_json → reproject
test_refine_of_a_clean_map_is_unchanged_by_the_href_carry	new — the no-regression half

The three share a new _legacy_collided_source helper.

---

How it was tested

$ uv run pytest -q tests/test_shardmap.py tests/test_sweep.py tests/test_closest_obs.py tests/test_catalog.py
324 passed, 1 skipped, 7 warnings in 31.95s

$ uv run ruff check src/zagg/catalog/shardmap.py tests/test_shardmap.py
All checks passed!
$ uv run ruff format --check src/zagg/catalog/shardmap.py tests/test_shardmap.py
2 files already formatted

Full-suite and pre-commit --all-files results are recorded per phase as the phases land.

---

Questions for review

1. Both phase 1 and phase 2 grow an already over-cap module, and ShardMap.reproject drops per-entry pairing provenance (closest-obs epoch/offset keys) #517 itself says it should wait for the split. src/zagg/catalog/shardmap.py is 2,044 lines against CLAUDE.md §4's ~1,200-line ceiling, and issue Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430 tracks the split. Issue ShardMap.reproject drops per-entry pairing provenance (closest-obs epoch/offset keys) #517 states its fix is "best implemented with or after the Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430 module split … rather than growing the over-cap module further". Both phases were therefore kept as small as the fix allows — phase 1 is a one-line change plus comment, phase 2 adds one small helper — and neither refactors anything. Which do you want:
   (1) land both here as-is (the module grows by roughly 30 lines of code and comment, the two silent-loss classes stop being live);
   (2) hold phase 2 only for Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430 and ship phase 1 + phase 3 now (phase 1 is a genuine data-loss fix and reads worse deferred);
   (3) hold both shardmap phases for Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430 and ship only phase 3 here.
   Absent a call, this PR proceeds on (1) and the phases checklist above tracks it.

2. pre-commit run mypy --all-files is already red on main — 174 errors across 34 files under src|tests (45 union-attr, 43 arg-type, 31 index, …), so issue mypy: cover .github/scripts/ — bench_metrics.py now holds the re-pin driver's engine untyped-checked #514's literal acceptance ("clean with the widened scope") cannot be met by this PR without fixing a large pre-existing backlog that is out of its scope (CLAUDE.md §4: flag, don't fix). Phase 3 will therefore be held to the achievable standard: widening the scope introduces no new errors, i.e. the newly covered files are clean. Note mypy is not wired into CI — lint.yml runs only ruff (--select=E,F,W,I --ignore=E501) as a fail_level: none PR-review bot — so this backlog has never gated a merge. Do you want the backlog raised as its own issue?

3. ruff check src tests and ruff format --check src tests are also red on main, both in files this PR does not touch: N818 on src/zagg/registry.py:64 (UnknownCapability wants an Error suffix), and tests/data/benchmark/README.md reformatting a python fence. Neither is reachable from CI's ruff selection (N is not selected) or from the pre-commit ruff-format hook (which does not take markdown), so both are artifacts of the bare CLI invocation in CLAUDE.md §4 rather than live failures. Left alone as pre-existing.

4. Issue mortie 1.0 floor bump: migrate retired call sites (arrow from_wkbs rename; shardmap fallback off morton_coverage_moc) #513 (the mortie 1.0 floor bump) is deliberately NOT in this bundle — mortie 1.0.0 is not released (PyPI latest is 0.9.11), so it is gated on an upstream release. Nothing in pyproject.toml, src/zagg/catalog/sources.py, or shardmap.py's morton_coverage_moc fallback was touched.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Adversarial self-review of phases 1 and 2 (commits 63cced7 and 548da15). Both phases landed without a review round, so this covers them together.

Both fixes are real and the direction on each is right. Phase 1's one-line href carry is the correct shape for issue #512 — keying the bucket on the distinguishing tuple rather than the id is well argued in the PR body and I have no quarrel with it. Phase 2's generic passthrough is the better of the two options issue #517 offered. The findings below are about how far each goes, not whether it should exist.

Findings

1. reproject's coarsen arm still drops provenance — last-wins on the union, and closest-obs builds paired_epochs per shard, so divergent copies across sibling children are the construction's normal output, not an edge case. Reproduced: 31 source entries → 13, three of four children's provenance gone per merged parent. This is issue #517's literal acceptance going unmet, and the new coarsen test does not reach it. Blocking for the issue's acceptance.
2. The reworded loader warning is wrong for the noop arm — it promises reproject "refuses"; the same-order branch returns before the guard and derives a still-colliding map. Reproduced. The behaviour is correct, the message is not.
3. The refine overlay is wider than issue #512 asked — the map wins on every _granule_entry key, not just the hrefs, so a refine against a corrected catalog now keeps stale time metadata. The comment's "the overlay is a no-op" holds only when the catalog is the one the map was built from, which reproject never checks.
4. The _carry_auxiliary docstring names the passthrough's upside only — the downside is that order-dependent per-entry keys ride through stale, which is exactly the hazard the metadata pops forty lines below exist to prevent. Finding (1) is that hazard already biting.
5. Minor: derived entries alias the source's mutable values (widens _granule_entry's existing assets precedent to list-valued provenance).
6. Nit: two new tests take a catalog fixture they never use.
7. Nit: the plain-map test hand-copies _granule_entry's key set instead of deriving it.

Module size. src/zagg/catalog/shardmap.py is 2,085 lines, up 41 from the 2,044 the PR body records and well past CLAUDE.md §4's ceiling. That is the tradeoff PR-body question (1) already puts up for a ruling, and issue #430 tracks the split; noting it so the growth is on the record, not re-litigating it. Please keep any fix for finding (1) to the smallest thing that closes the loss.

No finding here asks for a new dependency, a scope change, or anything outside this PR's branch.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

(5) Minor: the derived map's entries now alias the source map's mutable values.

entry[key] = value binds the reference, so derived.granules[i][j]["paired_epochs"] is source.granules[i][j]["paired_epochs"]. Appending to one map's provenance mutates the other's. Before this change the passthrough only ever copied scalars plus assets, so assets is the sole precedent — this widens the aliasing to every auxiliary key, and provenance is list-valued by design.

Flagging rather than pressing: matching _granule_entry's existing behaviour is defensible and deep-copying here would be a wider change to an already over-cap module. If finding (1) is fixed by concatenating lists on union, do make sure the fix builds a new list rather than extend-ing in place, or the coarsen will corrupt the source map it was handed.

---

Generated by Claude Code

[espg]

🤖 from Claude

Left by design, with the caveat taken.

The aliasing stands: _carry_auxiliary still binds the reference, so a single-child entry's paired_epochs is the source map's list object. Not changed because it is _granule_entry's existing contract for assets and copying here would either make the passthrough inconsistent with the function it extends, or pull _granule_entry into a copy discipline it has never had — a wider change than either issue in this bundle asks for, in a module already 900 lines over the cap. Whether entries should be defensively copied at all is a real question, but it is a _granule_entry question, not a passthrough one, and it belongs with the issue #430 split rather than here. Leaving it standing.

The caveat in your last paragraph was acted on. _union_auxiliary rebuilds each column:

for j, key in enumerate(cols):
    banked[key] = [row[j] for row in rows]

Not extend, so the coarsen never writes through the alias into the map it was handed. Pinned by test_the_coarsen_union_leaves_the_source_map_untouched in 691c839, which snapshots the source's granules before the reproject and asserts they are unchanged after.

---

Generated by Claude Code