shardmap: refuse per-shard granule basename collisions at construction (issue #468)

[espg]

Closes #468. Refs #415, PR #420 (question (6) ruling: option (a)).

What this does

Enforces the per-shard granule-identity invariant where it is owned — at shardmap construction — instead of leaving its violation to surface at the leaf gate.

Post-#420 a granule's recorded identity is its driver-stripped basename (zagg.telemetry.canonical_granule_id), so two granules assigned to one shard that differ only in href prefix collapse onto one recorded id. Question (6) priced the leaf-gate consequence (a genuine contraction reading as id-multiset-drift and rewriting) and ruled it acceptable because that state is impossible in every catalog zagg reads. This turns that "because" from an assumption into a construction-time refusal, which is what makes the leaf-side predicate variants — question (6) options (b)/(c) — permanently unnecessary. Nothing worker-side changes: the ruled leaf-gate behavior (loud warning + rewrite) stays exactly as shipped.

Approach

Three helpers in src/zagg/catalog/shardmap.py:

• _recorded_identity(entry) → (canonicals, distinguishing). The canonical ids are canonical_granule_id over each href spelling — s3 and https, since runner._resolve_urls picks one by driver and the invariant must hold under either choice (question (1) ruling) — falling back to the catalog id, then the acquisition datetime for raster entries which carry no href. distinguishing is (id, s3, https, datetime) — datetime included because two acquisitions sharing an item id record as one id under raster_granule_ids, so without it that collapse read as one granule listed twice. The sibling assets (issue GEDI waveforms 2/3: generic vlen reader primitives, paired-asset shardmap, flux transform + gedi01b template #425) are excluded: a record's granule identity is the primary alone.
• _collision_label(entry) names one colliding entry by what tells it apart from its partner — the href when there is one, else id @ datetime. A raster pair shares its id, so naming both by id says nothing.
• _refuse_basename_collisions(shard_keys, granules) raises ValueError naming the shard, the collapsed canonical id, and both entries. Entries agreeing on every distinguishing field are one granule listed twice and pass; an entry with nothing to canonicalize (including an id that reduces to "") is excluded from the collision scan but warns — it has no recorded identity at all (question (6) ruling).

ShardMap: 1 per-shard granule identity collision(s) — granules assigned to one shard
record as ONE granule id, so the shard's catalog identity names fewer granules than it
reads (issue #468). Usually one basename under two key prefixes, in which case re-scope
the catalog query so each granule appears once, or de-collide the basenames; the entries
below are named by whatever separates them: shard 12094627905536 'Gdup.h5' <-
['s3://b/p1/Gdup.h5', 's3://b/p2/Gdup.h5']

Wired into the three paths that mint shard membership:

1. ShardMap.build, right after the per-shard granule lists are assembled.
2. ShardMap.reproject, after both the coarsen and refine arms. Coarsen can mint a collision the source map did not have: a pair that was cross-shard (and therefore legal) at the fine order lands in one shard once coarsened.
3. zagg.sweep.SubmapFamily.merge, the rollup fold — see question (7).

All three previously deduped on entry["id"] alone, which silently dropped one member of such a pair before anything could see it; they now dedup on the distinguishing tuple, so a granule spanning several children still counts once while two distinct granules survive to be refused.

What the guard does not reach: the refine arm looks each entry up by id and rebuilds it from the catalog record, so a source map's per-entry hrefs never reach the derived map and a planted collided pair emerges as one entry carrying the catalog's own href. The dedup-key change is therefore a no-op on that arm (kept for symmetry, and it is the correct key by construction). The noop arm is untouched (no new membership → no new collision). from_json / from_parquet still never refuse, but now warn when a loaded manifest carries an identity collision — see questions (2) and (5).

The guard costs ~0.46 s over 550,000 entries on the clean path, and its failure path is bounded: only the first four collision groups are retained while the rest are counted, so a wholly mis-scoped catalog costs 0.2 MB rather than 67 MB to report.

Phases

• Phase 1 — the check, the wiring, and tests (ed80207).
• Phase 2 — fold the first review round's four findings (ee61c13, ae23b44, 8db57ba).
• Phase 3 — fold the independent review's six findings (382caba, 129086f).
• Phase 4 — apply espg's rulings on the seven questions below (in-session, 2026-08-24): merge of main (aeb1b1a), then one fold commit per ruling — question (1) a99d408, question (2) 5f75bf6, question (6) 20fc83d.
• Phase 5 — fold the phase-4 adversarial review round: three findings (56f09c2 spelling-scoped collision namespace, ba35f6b warning attribution + manifest path, 9921f0a self-describing no-identity label).

How it was tested

tests/test_shardmap.py::TestBasenameCollisions (17 tests) and tests/test_sweep.py::TestSubmapIdentityCollisions (3 tests):

• build refuses one basename under two prefixes (the realistic CMR shape: identical catalog ids, different key prefixes), and refuses the other spelling where the ids themselves carry the prefix;
• the refusal names the shard, the collapsed id, and both hrefs — and, for a collision with no href, both acquisition datetimes, with the cause clause hedged rather than asserting a prefix cause the raster path does not have;
• control: an ordinary multi-granule catalog builds unchanged (granules_assigned == 3);
• the same granule listed twice is not a collision, and neither is one acquisition listed twice;
• an entry with nothing to canonicalize is skipped, as is an id that canonicalizes to "" — both cases verified to fail against the pre-fix predicates, by replaying the fixtures through copies of the guard carrying the old is None test and the deleted skip branch;
• two acquisitions sharing an item id collide — the test first asserts the collapse is real (raster_granule_ids([a, b]) == ["SCENE", "SCENE"]) so it cannot pass vacuously;
• four colliding groups exercise the truncation branch: the count says 4, three groups are shown, the fourth is only counted;
• coarsen refuses a collision the source order did not have — the fixture asserts the pair is legal at the fine order first, so the test proves the merge is what creates it;
• refine rebuilds hrefs from the catalog, so it cannot surface a collision (pins the guard's boundary);
• the sweep union keeps both members of a collided pair (the direct assertion that the id-keyed dedup would have dropped one), a shard-node fold refuses one, and a refused fold reaches _merged as a skip whose warning names the collision rather than a bare "merge failed".

Phase 4 adds five tests and upgrades two (TestBasenameCollisions now also covers: the https-collision-under-distinct-s3 hole at guard and build level; both loaders warning on a collided manifest and keeping it intact; a clean manifest loading warning-free; the two silent-skip tests now pin the question-(6) warning). Full local run on the phase-4 head 20fc83d (after merging main): pytest -q --deselect tests/test_lambda_build.py → 4665 passed, 37 skipped, 1 failed — the failure is tests/test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries, byte-identical to main and failing against main's checkout on this machine too (pre-existing, unrelated; flagged per §4, not fixed). Earlier full run on the pushed head 129086f: pytest -q --deselect tests/test_lambda_build.py → 4417 passed, 38 skipped. ruff check src tests clean apart from the pre-existing N818 in src/zagg/registry.py; ruff format --check clean on every touched file.

Questions for review

All seven are RESOLVED — espg's rulings, in-session 2026-08-24, recorded inline below.

1. Keyed on the href, not on entry["id"]. The issue frames the collision on rec["id"]; this keys on the href the runner resolves, because that is what the recorded identity is derived from. For every catalog zagg reads the two agree — that agreement is why rec["id"] equals the basename — so on real catalogs the choice is unobservable. Related, from the review: the guard always prefers s3 while runner._resolve_urls picks by driver, so an https-driver run whose https basenames collide (while its s3 basenames do not) is caught by neither the guard nor anything downstream. Canonicalizing both spellings would remove that assumption for one extra call per entry — but it also widens what the guard refuses, which is why I have not done it unasked. RESOLVED — ruling: close the driver hole. Canonicalize BOTH url spellings (s3 and https) when deriving the guard's identity, so an https-driver run whose https basenames collide (while s3 basenames differ) is also refused. One extra canonicalization call per entry at build time is acceptable; widening what the guard refuses is the point. Folded in a99d408 with tests pinning the https-collision-under-distinct-s3 hole shape at both the guard and build level.
2. Scope: constructors that mint membership, not readers. from_json / from_parquet load manifests built before this check existed; making them refuse would turn a historical map into an unreadable one. Should loading warn (not refuse) on a collision, or is silence right there? RESOLVED — ruling: warn on load. from_json / from_parquet warn — never refuse — when a loaded map carries an identity collision: historical maps stay readable, the hazard becomes visible. Folded in 5f75bf6: both loaders route through _warn_loaded_collisions (a RuntimeWarning carrying the guard's collision report plus the issue refine drops a granule from legacy identity-colliding shard maps loaded via from_json (silent loss upstream of the #482 guard) #512 consequence), with tests for both loaders warning, the map surviving intact, and a clean manifest loading warning-free.
3. shardmap.py is 1,955 lines — 755 over the ~1,200 ceiling and 655 over the pre-approved overage band (issues Readers: sub-leaf region read (read_subtree) — one morton subtree as a contiguous cell span #351/declare-pyramid retrofit: install/update the manifest pyramid block on an existing store #358 cover overages below 1,200), against 1,846 on main. The split is issue Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430's scope, so I did not fold a module split into a small-fix. The review's point stands: "already over" argues for adding less, not for treating the ceiling as moot — the docstrings were trimmed accordingly, but this still needs a word. RESOLVED — ruling: overage acknowledged. The 1,955 lines are acknowledged by espg; the module split rides issue Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430 after the 0.50 release train. These folds added as little to the module as possible per the ruling — helpers stay in the module the guard already uses; no split started here.
4. Unrelated pre-existing lint, not touched: ruff format --check reformats a python block inside tests/data/benchmark/README.md, and ruff check reports N818 on src/zagg/registry.py. Both are on main already. RESOLVED — stays as-is (pre-existing on main, untouched by this PR).
5. A pre-existing silent drop, surfaced but not fixed: a legacy colliding manifest loaded via from_json and then refined loses one granule, upstream of anything this check can see. Fixing it means refusing at load or carrying the map's hrefs through refine — both scope changes beyond this small-fix. RESOLVED by split — filed as issue refine drops a granule from legacy identity-colliding shard maps loaded via from_json (silent loss upstream of the #482 guard) #512 (small-fix) at espg's direction. The refine-side fix is refine drops a granule from legacy identity-colliding shard maps loaded via from_json (silent loss upstream of the #482 guard) #512's scope and is not implemented here; the question (2) load-warning is its interim mitigation — the collision is now named at load, before refine can drop a member.
6. An entry with nothing to canonicalize is skipped silently. It has no recorded identity at all, which is arguably worse than a collision. The review showed this is not exotic: _resolve_urls drops href-less records, so an entry with s3 set and https None under an https driver contributes nothing to granules_sha256 while the guard canonicalizes it from s3 and sees an identity the run never records. Should that warn? RESOLVED — ruling: warn on the silent skip. An entry with no recorded identity (nothing canonicalizes to a granule id; it contributes nothing to granules_sha256) now warns instead of being skipped silently. NOT refused — _resolve_urls drops href-less records by design; the warning is the fix. Folded in 20fc83d: one RuntimeWarning per scan carrying the count and the first offending entry, with the two silent-skip tests upgraded to pin the warning.
7. The sweep fold now refuses, which changes an unattended path — this is the one I most want a ruling on. SubmapFamily.merge is the third constructor path, and it had the same id-keyed silent drop (now fixed). But a refusal there reaches _merged's fail-open handler and the rollup node is skipped, not written, where before it succeeded by silently collapsing. Issue shardmap build: refuse per-shard basename collisions (enforce the identity-collapse invariant at construction) #468 justified a hard raise as "build-time, human-present, laptop-context"; the sweep is none of those, and "re-scope the catalog query" is not a lever an operator has when the inputs are stored leaf sub-maps. Options: (a) keep the refusal and accept a skipped node, as it stands now; (b) warn-and-collapse in the sweep while build/reproject still refuse; (c) refuse but make _merged treat an identity collision as fatal rather than fail-open. I have kept (a) and made the skip legible — the warning names the collision — rather than choose. RESOLVED — ruling: option (a), zero code change. The refusal stays; a skipped rollup node with a collision-naming warning is correct. Rollups are regenerable caches (D9): warn-and-collapse would write a WRONG derived artifact, and fatal would break fail-open on an unattended path. The sweep-fold code stands exactly as shipped in 382caba.

Process note

The per-phase self-review was run twice. The first pass was done in-session by the author after I killed two review subagents believing they had stalled — a timing error of mine, since the waits I was measuring with ran concurrently rather than in sequence. The independent fresh-context review then ran properly on the folded head and found six further findings, two of them HIGH — including the sweep constructor path in question (7), a live silent drop the in-session pass had missed entirely. That is the argument for the convention, made at this PR's expense.

One deviation from §3 that I did not rewrite: the second fold round is two commits rather than one per finding (382caba covers findings (1)–(4), 129086f covers (6) plus the failure-path bound), since the changes overlapped in the same helpers.

[espg]

🤖 from Claude (review)

Protocol deviation, stated up front: CLAUDE.md §2 has this pass run in a separate fresh-context review subagent. Two were spawned on Opus and both stalled in their analysis phase without ever reaching a post (one ~90 min, one ~40 min, killed after each was messaged to wrap up); the second had not got past loading tool schemas. So this pass was run in-session by the author rather than by an independent reviewer, and inline anchoring was dropped for a single comment. That is a weaker check than the convention asks for — findings (1) and (2) below are pinned by a reproduction and a coverage gap rather than by an independent read, and a genuinely adversarial pass may still find more. Flagging rather than presenting this as the usual loop.

Findings, most severe first.

---

(1) src/zagg/catalog/shardmap.py:192 — a no-href entry pair can collapse without being refused. HIGH.

distinguishing is (id, s3, https). Raster entries carry no href, and their recorded identity is id or datetime (zagg.telemetry.raster_granule_ids). So two raster acquisitions that share an item id but differ in acquisition datetime record as ONE id while the check sees one granule listed twice:

>>> a = {'id': 'SCENE', 's3': None, 'https': None, 'datetime': '2025-06-01T00:00:00Z'}
>>> b = {'id': 'SCENE', 's3': None, 'https': None, 'datetime': '2025-06-02T00:00:00Z'}
>>> raster_granule_ids([a, b])
['SCENE', 'SCENE']                      # both collapse in the recording
>>> _recorded_identity(a), _recorded_identity(b)
(('SCENE', ('SCENE', None, None)), ('SCENE', ('SCENE', None, None)))
>>> _refuse_basename_collisions([7], [[a, b]])
                                        # not refused -- slips through

That is precisely the collapse issue #468 exists to catch, missed on the one entry shape where the recorded id is not derived from an href. _recorded_identity already reaches for datetime when computing canonical; the distinguishing tuple should reach for it too. Change the return to include entry.get("datetime"), and cover it with a test.

(2) tests/test_shardmap.py — the refine arm's dedup-key change is behavioral and untested. MEDIUM.

bucket[entry["id"]] → bucket[_recorded_identity(entry)[1]] at shardmap.py:1804 changes which entries survive a refine, and TestBasenameCollisions exercises build and the coarsen arm only. CLAUDE.md §4 wants a test per behavioral change. The coarsen test pins that a collided pair survives the merge to be refused; the refine arm has no equivalent, and it is the arm with the more complicated entry construction (sub_records merges catalog records with map entries, so the two sides could disagree on a key). Add a refine-arm case.

(3) src/zagg/catalog/shardmap.py:190 — an id of "/" canonicalizes to the empty string. LOW.

canonical_granule_id("/") returns '' (it strips the trailing slash, then rpartition yields '', and the or text fallback yields '' too). '' is falsy but the guard at line 218 tests canonical is None, so '' becomes a live bucket key and would be reported as the collapsed id — shard 7 '' <- [...]. canonical_granule_id refuses to mint an identity from None/"" precisely so nothing gets a silently-wrong one; an id that arrives at '' through it should get the same treatment. Either widen the guard to if not canonical or let it refuse by name.

(4) src/zagg/catalog/shardmap.py:231 — the [:3] truncation branch is untested. LOW.

collisions[:3] and the trailing ' ...' only fire past three colliding groups; no test constructs that, so the branch that decides what an operator sees on a badly-mis-scoped catalog is unexercised. One _refuse_basename_collisions unit case with four groups covers it.

---

Two things I am not calling defects, left standing rather than folded:

• An entry with nothing to canonicalize is skipped silently (line 218). Such an entry has no recorded identity at all — it contributes nothing to granules_sha256 — which is arguably a worse state than a collision, and the skip hides it. Whether that should warn is a judgement about the recorded-identity contract, not an implementation choice.
• shardmap.py is 1,940 lines, far past the ~1,200 ceiling, and the standing pre-approval (issues Readers: sub-leaf region read (read_subtree) — one morton subtree as a contiguous cell span #351/declare-pyramid retrofit: install/update the manifest pyramid block on an existing store #358) only covers overages below 1,200. The PR flags this as its question (3) rather than blocking on it. Flagging looks right — the split is issue Split shardmap.py backend seam into catalog/footprints.py (post-#400 refactor) #430's scope and folding one into a small-fix would be the scope change §6 reserves — but it does need a word.

The PR's other three "Questions for review" read as honest: (1) names the href-vs-id divergence and its blast radius, (2) states the reader/constructor boundary and asks rather than assumes, (4) is pre-existing lint correctly left alone.

---

Generated by Claude Code

[espg]

🤖 from Claude

Folded all four review findings, one commit each. Reply per finding:

(1) no-href entries could collapse unrefused — fixed in ee61c13. _recorded_identity's distinguishing tuple is now (id, s3, https, datetime), so two acquisitions sharing an item id are two granules to the check, matching what raster_granule_ids records for them. _refuse_basename_collisions also falls back to datetime when naming the colliding entries, so the message says which acquisitions rather than None. Two tests: one asserting the collapse is real (raster_granule_ids([a, b]) == ["SCENE", "SCENE"]) before asserting the refusal, one keeping a single acquisition listed twice passing.

(2) refine arm untested — addressed in ae23b44, and the finding turned out to understate the situation. The refine arm cannot be given a collision at all: it looks each entry up by id and rebuilds from the catalog record, so a source map's per-entry hrefs never reach the new map. A planted collided pair comes out as one entry with the catalog's own href:

>>> source.granules[0]
[{'id': 'Gdup', 's3': 's3://b/p1/Gdup.h5', ...}, {'id': 'Gdup', 's3': 's3://b/p2/Gdup.h5', ...}]
>>> source.reproject(fine_grid, catalog=cat).granules[0]
[{'id': 'Gdup', 's3': 's3://b/Gdup.h5', 'https': 'https://h/Gdup.h5'}]

So the refine dedup-key change is a no-op in the href-collision case (kept for symmetry with coarsen and because it is the correct key by construction), and the guard's real reach is build + coarsen. test_refine_rebuilds_hrefs_from_the_catalog_so_it_cannot_collide pins that, because it bounds what this PR can promise.

That surfaces a pre-existing silent-drop path I am not fixing here and is now question (5) in the description: a legacy colliding manifest loaded via from_json and then refined loses one granule, upstream of anything this check can see. Fixing it means either refusing at load or carrying the map's hrefs through refine — both scope changes beyond this small-fix, so left standing.

(3) empty canonical id — fixed in 8db57ba. The guard is if not canonical rather than is None, so an id of "/" (which canonical_granule_id reduces to "") is skipped instead of becoming a live bucket key that would report '' as the collapsed granule id.

(4) truncation branch untested — covered in 8db57ba: four colliding groups on one shard, asserting the count says 4, three groups are shown, the fourth is only counted, and the message ends in the ellipsis.

Not folded, standing for review: the silent skip of an entry with nothing to canonicalize (a judgement about the recorded-identity contract, question (6) in the description) and the 1,940-line module (question (3)).

Verification on the pushed head 8db57ba: pytest -q --deselect tests/test_lambda_build.py → 4412 passed, 38 skipped locally; ruff check and ruff format --check clean on both touched files.

CI note: the Tests run for the first phase commit (32131877303) showed the stall this repo has hit before — updated_at never moved off run_started_at while the sibling ruff and build runs from the same push finished. The fold push has started a fresh run; if that one stalls too, the remedy is the close/reopen, which is a side-effecting action left for a human rather than taken here.

---

Generated by Claude Code

[espg]

🤖 from Claude

Correction — the process note above is wrong, and the review is being re-run properly.

I reported that two review subagents "stalled" for ~90 and ~40 minutes. That timing was my error, not theirs: the waits I was measuring with were launched as background commands, so they elapsed concurrently rather than in sequence, and I read a few minutes of real time as an hour and a half. Both PRs' timestamps say so plainly — this PR was opened at 11:28Z and the first fold landed at 11:37Z, nine minutes for the whole sequence I described as two hours of stalling.

So there was no stall. I killed two working reviewers after roughly two minutes each and then did their job in-session on a false premise. An independent fresh-context review of the folded head is running now; I will report what it finds, and update the description's process note, when it lands.

The four findings already folded (ee61c13, ae23b44, 8db57ba) stand on their own — each is pinned by a reproduction or a test, and none of them depends on how the review was conducted.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Independent fresh-context pass on head 8db57ba (folds included), run as the separate review subagent CLAUDE.md §2 asks for — the pass the process note says did not happen. I deliberately did not re-litigate the four already-folded findings; everything below is new ground, and two of the items are defects in the folds themselves.

Findings, most severe first. All six are reproduced against this head; scratch reproductions are quoted inline.

(1) HIGH — zagg.sweep.SubmapFamily.merge is the third shardmap constructor path, and the guard both misses it and now fires inside it unattended. Issue #468 says "and any other shardmap constructor path"; the description's scope paragraph names only from_json/from_parquet and never mentions the sweep. sweep.py:455 still dedups on entry["id"] alone — the exact pattern this PR replaced in coarsen — so a window union silently drops one member of a collided pair; and sweep.py:480's folded.reproject(...) now raises, which _merge_node catches at sweep.py:882 as merge failed at node ...; skipping, dropping the rollup node. That is a new failure mode on a fail-open unattended path, with an operator remedy ("re-scope the catalog query") that has no meaning there. Existing sweep coverage is green because no fixture plants a collision.

(2) HIGH — the refusal cannot name the colliding entries in the raster case fold (1) added. The or entry.get("datetime") tail is unreachable on that path (a raster collision requires a shared non-empty id), so the message prints shard 7 'SCENE' <- ['SCENE', 'SCENE'] — the discriminator never appears — while asserting a "basename across different prefixes" cause that does not exist there. The fold reply's claim that the message "says which acquisitions" is not what the code does. test_two_acquisitions_sharing_an_item_id_collide asserts only match="identity collision", which is why it shipped.

(3) MEDIUM — two of the fold's tests pass against the un-fixed code. test_an_id_that_canonicalizes_to_empty_is_skipped passes with if not canonical reverted to if canonical is None, and test_an_entry_with_nothing_to_canonicalize_is_skipped passes with the skip branch deleted outright. Both verified by re-running the shipped fixtures through copies of the guard carrying the pre-fold predicate. One-line fixes given inline. (The rest of TestBasenameCollisions survives the same treatment; test_coarsen_does_not_silently_drop_one_of_a_collided_pair is sound but mis-named — it asserts message content, not a granule list.)

(4) MEDIUM-LOW — cost. sorted(by_canonical.items()) runs before the len(named) > 1 filter, so every shard pays an O(m log m) sort that only a collision needs, and canonical_granule_id is imported inside _recorded_identity once per entry. Measured at 550,000 entries: 0.574 s → 0.345 s (40%) with those two changes and identical output. Separately the failure path accumulates every collision with its full href list to print three — 5.55 s and 67 MB traced on a wholly mis-scoped catalog.

(5) LOW / question — the guard canonicalizes s3 while runner._resolve_urls picks by data_source.driver. Safe under the docstring's stated assumption that a granule's two spellings share a basename, but that assumption is asserted rather than checked, and checking both spellings costs one extra call. Probably belongs folded into question (1) rather than decided here.

(6) LOW — §4 terseness. ~95 lines added to a 1,940-line module, ~60 of them docstring prose that restates PR #420's ruling history already carried near-verbatim in the PR description. Given question (3), "already over the ceiling" argues for adding less. Also _recorded_identity lacks the return annotation its two neighbours have.

Standing rather than folded, three notes:

• The assets exclusion is over-applied on raster entries. For a paired granule (issue #425) the primary alone is the identity, so excluding assets is right. A raster entry has no primary href — assets carries the actual per-band data hrefs — so two raster entries differing only in assets are two granules that record as one id and pass the guard as "one granule listed twice". Same shape as fold (1), and the fix is the same widening. Reachable only on a catalog with duplicate item ids or id-less items sharing a datetime, which is exactly the class #468 exists to catch, so it is worth a sentence in the description even if not fixed.
• The paired-asset interaction is correct as built and I could not construct the failure the sibling join might imply. _pair_sibling_records keys siblings by a join key derived from the primary id, so two entries sharing (id, s3, https, datetime) necessarily pair to the same sibling — the "same primary, different assets" case is unreachable from build, and coarsen's dedup collapses them last-wins exactly as the pre-PR id-keyed dedup did. No behaviour change, nothing to test.
• The refine-arm no-op claim checks out. sub_records rebuilds every field of the distinguishing tuple from records_by_id[gid], so the tuple is a pure function of the id there and the key change is genuinely inert. test_refine_rebuilds_hrefs_from_the_catalog_so_it_cannot_collide pins the right thing.

Findings (1) and (2) are the ones I would not merge without; (1) in particular touches an unattended path and reads to me like it needs an @espg call on whether the sweep fold should refuse or warn, rather than an implementation choice made here.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

HIGH — there is a third shardmap constructor path, zagg.sweep.SubmapFamily.merge; the guard both misses it and (via this line) fires inside it unattended.

Issue #468 says ShardMap.build "and any other shardmap constructor path". The description's "What the guard does not reach" paragraph names only from_json / from_parquet. But src/zagg/sweep.py:466 mints per-shard membership and constructs a ShardMap directly:

for entry in entries:
    ...
    bucket[entry["id"]] = dict(entry)      # sweep.py:455
keys = sorted(buckets)
granules = [list(buckets[k].values()) for k in keys]
folded = ShardMap(signature, keys, granules, meta)   # sweep.py:466

Two distinct problems, both reproduced against this head:

(a) The silent drop this PR removed from coarsen is still live in the sweep. bucket[entry["id"]] is exactly the id-keyed dedup the coarsen arm replaced, for exactly the reason given in the new comment above it. Folding several windows of one shard:

>>> SubmapFamily().merge([payload([k], [[a]]), payload([k], [[b]])], node=..., order=SHARD_ORDER)
# a = {'id': 'Gdup.h5', 's3': 's3://b/p1/Gdup.h5', ...}
# b = {'id': 'Gdup.h5', 's3': 's3://b/p2/Gdup.h5', ...}
[[{'id': 'Gdup.h5', 's3': 's3://b/p2/Gdup.h5', 'https': 'https://h/p2/Gdup.h5'}]]

One member gone, no error — and this is the realistic collision shape, the same one test_build_refuses_one_basename_under_two_prefixes uses (identical catalog ids, different key prefixes). The shard-node fold branch (parent_order == order) returns that ShardMap unchecked and it is written to the store as a rollup artifact.

(b) This line makes the interior fold a new unattended failure mode. When parent_order != order the fold calls folded.reproject(_ReprojectTarget(...)) (sweep.py:480), so the guard runs. Reproduced:

>>> SubmapFamily().merge([payload([k1], [[c]]), payload([k2], [[d]])], node="-31", order=SHARD_ORDER-1)
ValueError: ShardMap: 1 per-shard granule identity collision(s) — ... Re-scope the catalog query ...

_merge_node catches that at sweep.py:882, logs sweep[submap]: merge failed at node ...; skipping, increments failed, and the rollup node is not written. Before this PR that fold succeeded. Issue #468 justified a hard raise as "build-time, human-present, laptop-context" — the sweep is none of those, and the remedy the message offers ("re-scope the catalog query") is not a lever the operator has when the inputs are stored leaf sub-maps. Existing coverage does not catch it: pytest tests/test_sweep.py -k "Submap or submap or rollup" is 25 green because no fixture plants a collision.

What I would change, smallest first: (1) name the sweep fold in the "What the guard does not reach" paragraph and add it as a Question for review — it is the reason the issue's "any other constructor path" is not yet satisfied; (2) switch sweep.py:455 to the same _recorded_identity(entry)[1] key so the union stops dropping silently; (3) decide explicitly whether the sweep fold should refuse or warn — refusing there costs a rollup node on a fail-open path, which reads like an @espg call rather than an implementation choice.

---

Generated by Claude Code

[espg]

🤖 from Claude

Both halves folded in 382caba — this was the finding worth the whole second review pass, and (a) in particular was a live silent drop the PR had left standing.

(a) sweep.py now keys the union on _recorded_identity(entry)[1], the same key coarsen uses, so a collided pair survives the fold instead of one member overwriting the other. test_the_union_keeps_both_members_of_a_collided_pair pins it directly.

(b) The shard-node arm no longer differs from the interior arm: _refuse_basename_collisions(keys, granules) runs on the folded map before the branch, so a fold refuses whether or not it reprojects. Leaving the shard-node arm silent while its parent refused would have been the incoherent half of your point.

On the third item — whether the sweep should refuse at all — I have not decided it, and you're right that it isn't mine to. The behavior change is real and now stated: a fold that used to succeed by silently collapsing becomes a skipped rollup node via _merged's fail-open handler. That is the PR's new question (7), with the "build-time, human-present, laptop-context" mismatch you named. What I did do is make the skip legible rather than mysterious — test_a_refused_fold_is_a_named_skip_not_a_lost_node pins that the warning carries identity collision rather than a bare merge failed, so an operator reading the sweep log can tell this cause from a genuine fold error.

---

Generated by Claude Code

[espg]

🤖 from Claude

espg ruled question (7) (in-session, 2026-08-24): option (a), zero code change — the refusal stays, and a skipped rollup node whose warning names the collision is the correct outcome. Rationale as ruled: rollup nodes are regenerable caches (D9), so a skip loses nothing durable; warn-and-collapse (option (b)) would write a wrong derived artifact — the one outcome worse than no artifact; and fatal (option (c)) would break _merged's fail-open posture on an unattended path. The sweep-fold guard stands exactly as shipped in 382caba1; test_a_refused_fold_is_a_named_skip_not_a_lost_node remains the pin. Recorded as RESOLVED in the PR body.

[espg]

🤖 from Claude (review)

HIGH — the datetime fallback added by the previous round's fold (1) is dead in exactly the case it was added for, so the refusal names both colliding entries identically.

The fold reply claims: "_refuse_basename_collisions also falls back to datetime when naming the colliding entries, so the message says which acquisitions rather than None." That cannot happen. The chain here is

entry.get("s3") or entry.get("https") or entry.get("id") or entry.get("datetime")

so datetime is reached only when id is falsy — but a raster collision requires the two entries to share a non-empty id (that is what makes them one canonical). The or entry.get("datetime") tail is therefore unreachable on the raster-collision path, and the message degenerates. Reproduced on 8db57ba with the exact pair from test_two_acquisitions_sharing_an_item_id_collide:

ShardMap: 1 per-shard granule identity collision(s) — granules assigned to one shard
share a basename across different prefixes, so they collapse onto a single recorded
granule id ... Re-scope the catalog query so each granule appears once, or de-collide
the basenames: shard 7 'SCENE' <- ['SCENE', 'SCENE']

Two things wrong for an operator: the two entries are printed identically (['SCENE', 'SCENE']), naming nothing that distinguishes them — the datetimes, which are the whole reason this case is a collision, never appear; and the prose asserts a "basename across different prefixes" cause when there is neither a basename nor a prefix in sight, and offers a remedy ("de-collide the basenames") that has nothing to act on.

The href case is fine because the href is itself the discriminator; the raster case needs the discriminator that actually differs. Concretely: name each entry by the href when there is one, else by what _recorded_identity says distinguishes it — e.g. entry.get("s3") or entry.get("https") or (entry.get("id"), entry.get("datetime")) — and make the cause clause conditional (or generic: "differ only in fields the recorded identity discards") rather than asserting prefixes.

Nothing caught this because test_two_acquisitions_sharing_an_item_id_collide asserts only match="identity collision"; the href case has test_refusal_names_the_shard_and_both_hrefs and the raster case has no equivalent. Worth adding the mirror assertion — the message is the entire deliverable of a refusal.

---

Generated by Claude Code

[espg]

🤖 from Claude

Correct on both counts, and the fold reply you're quoting was wrong — the or entry.get("datetime") tail cannot be reached when the two entries share a non-empty id, which is precisely what makes them one canonical. Fixed in 382caba.

Naming moved into _collision_label: the href when there is one, else id @ datetime when both are present, else whatever single field remains. The raster pair now reads

shard 7 'SCENE' <- ['SCENE @ 2025-06-01T00:00:00Z', 'SCENE @ 2025-06-02T00:00:00Z']

and the prose no longer asserts a cause it cannot know: the prefix case is offered as the usual one ("Usually one basename under two key prefixes, in which case ...") followed by "the entries below are named by whatever separates them", rather than stating a basename/prefix cause outright.

Two tests, since you're right that the message is the entire deliverable of a refusal: test_a_collision_with_no_href_is_named_by_what_separates_it asserts both datetimes appear and that the cause clause is hedged, and test_an_href_collision_is_still_named_by_its_hrefs pins that the href case did not regress into the new branch.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

MEDIUM — this test passes against the code it was written to pin. It does not cover the previous round's fold (3).

The fold changed the guard from if canonical is None to if not canonical. This case does not distinguish the two. Verified by re-running the shipped fixture through a copy of the guard carrying the old is None predicate:

A: the shipped test case ALSO passes the pre-fix guard -> test is vacuous

Why: entry one (id="/") canonicalizes to "", entry two (s3="s3://b/x/") canonicalizes to "x". Under the old predicate "" becomes a live bucket key holding one entry, "x" another holding one — len(named) > 1 is false for both, so the old code returns quietly too. The bad state the fold fixed needs two entries that both reduce to "" and differ in the distinguishing tuple. That case does raise under the old predicate:

canonical('/')='' canonical('//')=''
pre-fix guard RAISES -> [(7, '', ['/', '//'])]     # reports '' as the collapsed granule id
shipped guard passes the two-empty case            # the fix works — it is just untested

So the one-line change to make this test load-bearing is a second empty-canonical entry that differs, e.g.

shardmap._refuse_basename_collisions(
    [7], [[{"id": "/", "s3": None, "https": None}, {"id": "//", "s3": None, "https": None}]]
)

Same defect one test up, at test_an_entry_with_nothing_to_canonicalize_is_skipped (line 2679): I re-ran its fixture through a copy of the guard with the whole continue branch deleted and it still passes — the two entries land in different buckets (None and "2025-06-01"), each of size one. To exercise the skip it needs two entries that both fail to canonicalize and differ, e.g. {"id": "", ...} alongside {"id": None, ...}, which without the branch collide in the None bucket and raise shard 7 None <- ['None'].

Everything else in TestBasenameCollisions holds up under the same treatment — test_two_acquisitions_sharing_an_item_id_collide in particular is well built, asserting the collapse is real before asserting the refusal.

One naming point, not a defect: test_coarsen_does_not_silently_drop_one_of_a_collided_pair (line 2712) never observes a granule list. It asserts the refusal message contains both hrefs — which is what test_refusal_names_the_shard_and_both_hrefs already asserts for build, and which follows from the collision being reported at all. The dedup-key change it claims to pin is only pinned transitively (if the key still dropped one, test_coarsen_refuses_... would fail first). Worth either renaming it to what it checks, or asserting on new_granules through a path the guard does not raise on.

---

Generated by Claude Code

[espg]

🤖 from Claude

Both vacuous tests fixed in 382caba, using your cases, and I re-ran your replay method rather than taking it on faith — a copy of the guard with the is None predicate, and a second copy with the continue branch deleted:

is-None predicate     : RAISES -> test bites the unfixed code
skip branch deleted   : RAISES -> test bites the unfixed code
current guard: silent on both

So test_an_id_that_canonicalizes_to_empty_is_skipped now uses "/" alongside "//" (both reduce to "", tuples differ), and test_an_entry_with_nothing_to_canonicalize_is_skipped uses {"id": None} alongside {"id": ""}. Each asserts the precondition inline so it cannot quietly go vacuous again.

On the naming point — you're right that test_coarsen_does_not_silently_drop_one_of_a_collided_pair never observes a granule list, and the dedup-key change was only pinned transitively. Renamed to test_coarsen_names_both_members_of_the_collided_pair, which is what it checks, and its comment now points at where the keeping is pinned directly: test_the_union_keeps_both_members_of_a_collided_pair in tests/test_sweep.py, which asserts on the surviving entries because the sweep union is the one merge the guard does not raise on.

---

Generated by Claude Code

[espg]

🤖 from Claude

Follow-up: espg ruled question (6) (in-session, 2026-08-24) — the skip branch this thread de-vacuoused now warns instead of passing silently (20fc83df). Both fixtures you supplied survive as the warning tests: test_an_entry_with_nothing_to_canonicalize_warns_and_is_skipped additionally pins one warning per scan carrying the count (not one per entry), and test_an_id_that_canonicalizes_to_empty_warns_and_is_skipped keeps the "/"/"//" pair that bites the old is None predicate.

[espg]

🤖 from Claude (review)

MEDIUM-LOW — the clean path pays an O(m log m) sort per shard that only the collision path needs, and the failure path materializes every collision to print three.

sorted(by_canonical.items()) runs over every canonical group in every shard before the if len(named) > 1 filter has a chance to discard 100% of them, which is the case on every catalog zagg actually reads. Filtering first and sorting only the survivors is the same output for O(m) work. Measured on this head, 1,100 shards x 500 entries = 550,000 entries, no collisions:

shipped guard:                            0.574 s
filter-before-sort + hoisted import:      0.345 s   (40% faster)
  sorted() alone over the same groups:    0.176 s

Two contributions, both avoidable:

• the sort above (0.176 s of the 0.574 s);
• from zagg.telemetry import canonical_granule_id sits inside _recorded_identity (line 189), so it executes once per entry — 550,000 sys.modules lookups. zagg.telemetry imports zagg.dispatch and nothing under zagg.catalog, so there is no cycle to dodge; a module-level import matches how _granule_entry's neighbours are written.

This file spends 50 lines of comment on _CELLS_BATCH_RECORDS justifying tenths of a second at exactly this scale, so 0.23 s of avoidable work on the clone build seems in scope for it.

Separately, the failure path is where the allocation goes. collisions accumulates every group with its full href list before [:3] throws all but three away. On a wholly mis-scoped catalog (every granule present under two prefixes, 550,000 entries):

wall = 5.55 s, peak traced allocation inside the guard = 67 MB   # to print 3 groups

Keeping a running count and only retaining hrefs for the first four groups preserves both the count and the ' ...' branch (and test_more_than_three_collisions_are_counted_and_truncated would still pass unchanged) at bounded cost.

---

Generated by Claude Code

[espg]

🤖 from Claude

All three folded in 382caba / 129086f, and the failure-path point was the bigger one.

• Filter before sort. sorted(...) now runs over the survivors of if len(n) > 1, not over every canonical group — on a real catalog that is a sort of nothing.
• Hoisted import. _refuse_basename_collisions imports canonical_granule_id once and passes it down; _recorded_identity grew an optional canonicalize parameter that defaults to the local import, so the other call sites (the coarsen/refine/sweep dedup keys) are unchanged.
• Bounded failure path. Only the first four collision groups are retained (four, not three, so the ' ...' branch still keys off a real count), while n_collisions accumulates. The count and the truncation are unchanged and test_more_than_three_collisions_are_counted_and_truncated passes untouched, as you predicted.

Re-measured on this head with your fixture shape:

clean path   (1,100 x 500 = 550k entries, no collisions):  0.574 s -> 0.463 s
failure path (every granule under two prefixes):           5.55 s  -> 2.36 s
peak traced allocation inside the guard:                   67 MB   -> 0.2 MB

My clean-path number does not reach your 0.345 s — same direction, less of it, probably machine noise between the two runs; I did not chase the difference since both fixes are in.

And the _CELLS_BATCH_RECORDS comparison landed: a file that spends fifty lines justifying tenths of a second at this exact scale is not the place to leave 0.1 s and 67 MB on the floor.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

LOW / question — the guard canonicalizes one href spelling; the recorded identity uses the one the run's driver picks.

href = entry.get("s3") or entry.get("https") always prefers s3. What actually reaches granules_sha256 is driver-selected:

# zagg/runner.py:2695
key = "https" if driver == "https" else "s3"
return [r[key] for r in records if r.get(key)]

The docstring's justification — "the two spellings of one granule agree on the basename by construction" — does make the choice safe: if each granule's two spellings agree, then a pair collides in s3-space iff it collides in https-space. But that is an assumption about every catalog, asserted rather than checked, and it is the only thing standing between an https-driver run and a missed collapse (two granules whose https basenames collide while their s3 basenames do not are refused by neither the guard nor anything downstream — the leaf just records one id for two granules).

Canonicalizing both spellings when both are present and treating either match as a collision costs one extra canonical_granule_id call per entry and removes the assumption entirely. Given the s3/https split is already the thing question (1) is unsure about, it may be worth folding into that question rather than deciding here — it is a scope judgement, not an implementation one.

Related, and worth one line in question (6) rather than a change: _resolve_urls drops href-less records, so an entry that has no href for the run's driver contributes nothing to granules_sha256 at all. That is the same "no recorded identity" state question (6) already flags, but it is reachable on an ordinary aggregation entry (s3 set, https None, https driver), not only on the raster shape — the guard would canonicalize such an entry from s3 and see an identity the run never records.

---

Generated by Claude Code

[espg]

🤖 from Claude

Left standing, not folded — you called it correctly as a scope judgement, and it belongs with question (1) rather than being decided here. Both halves are now written into the description: the s3-preference is question (1), and the href-less-for-this-driver case is question (6).

The reason I did not just canonicalize both spellings, even though you're right that it costs one extra call and removes the assumption: doing so widens what the guard refuses. A catalog whose https basenames collide while its s3 basenames do not would newly fail to build under an s3 driver, where today it builds and records fine. That is a strictly larger refusal surface than issue #468 asked for, on an assumption that has not actually been observed to break — the kind of scope change §6 reserves rather than an implementation choice I should make inside a small-fix.

Your related point sharpened question (6) usefully, and I have written it in with your framing: _resolve_urls drops href-less records, so an entry with s3 set, https None, under an https driver contributes nothing to granules_sha256 — while the guard canonicalizes it from s3 and sees an identity the run never records. That reaches an ordinary aggregation entry, not just the raster shape, which makes the "no recorded identity" state less exotic than the question originally implied.

---

Generated by Claude Code

[espg]

🤖 from Claude

espg ruled both halves of this thread (in-session, 2026-08-24):

Question (1) — close the driver hole. Canonicalize BOTH url spellings when deriving the guard identity; widening what the guard refuses is the point, and one extra canonicalization per entry at build time is acceptable. Folded in a99d4081: _recorded_identity now returns the canonical of each href spelling and the guard registers the entry under every one, so the https-basenames-collide-while-s3-differ run this thread described is refused under either driver. test_https_collision_under_distinct_s3_basenames_is_refused pins the exact hole shape (asserting first that the s3 canonicals differ, so the old single-spelling guard sees nothing), and test_build_refuses_an_https_only_basename_collision wires it through build.

Question (6) — warn on the silent skip. An entry with no recorded identity now warns instead of being skipped silently (not refused — _resolve_urls drops href-less records by design). Folded in 20fc83df: one RuntimeWarning per scan with the count and the first offending entry.

[espg]

🤖 from Claude (review)

LOW — §4 terseness: the two helpers are ~95 lines of which ~60 are prose, and most of the prose is PR #420's ruling history restated.

_recorded_identity is 8 lines of code under a 24-line docstring; _refuse_basename_collisions is ~30 lines of code under an 18-line docstring plus a 6-line inline comment. CLAUDE.md §4 asks for terse, reviewable code because "the reviewer's time is the constraint", and §3/§6 put the why in the PR description — which already carries this material near-verbatim, so the reader gets it twice. The load-bearing sentences here are short: the canonical is the driver-resolved href's basename; datetime distinguishes because raster_granule_ids collapses shared item ids; assets does not, per issue #425. The PR #420 question (6) pricing narrative and the options (b)/(c) argument are thread material.

That matters more than usual because of question (3): this lands ~95 lines on a module already at 1,940, i.e. 740 over the ~1,200 ceiling and 640 over the pre-approved overage band. Trimming the docstrings to their operative sentences would cut the addition by roughly half without losing anything a reader of the PR cannot recover. Not a required change — flagging it because "the module is already over" is the argument for adding less, not for treating the ceiling as moot.

Minor and mechanical, same neighbourhood: _recorded_identity(entry: dict) has no return annotation while _granule_entry(rec: dict) -> dict and _refuse_basename_collisions(...) -> None on either side of it do. tuple[str | None, tuple] matches the surrounding style (§4).

---

Generated by Claude Code

[espg]

🤖 from Claude

Folded in 129086f. The argument that landed is the one at the end: "the module is already over" is a reason to add less, not to treat the ceiling as moot. I had been reading it the other way round.

_recorded_identity's docstring goes from 24 lines to 11 and keeps only the operative sentences — what canonical is, why datetime distinguishes, why assets do not, and what the canonicalize parameter is for. _refuse_basename_collisions drops the PR #420 question (6) pricing narrative and the options (b)/(c) argument (thread material, and the description carries it), keeping the two sentences that say what the function refuses and what deliberately passes. The _collision_label docstring and the empty-canonical inline comment got the same treatment.

Return annotation added as tuple[str | None, tuple], matching _granule_entry and _refuse_basename_collisions either side of it.

Net for the module: src/zagg/catalog/shardmap.py is 1,955 lines — the guard's prose came down but the sweep-fold finding's wiring went in, so the addition is roughly flat rather than halved. Question (3) stands unchanged and still needs a word: 755 over the ceiling, 655 over the pre-approved band, with issue #430 holding the split.

---

Generated by Claude Code

[espg]

🤖 from Claude

espg ruled question (3) (in-session, 2026-08-24): the overage is acknowledged — shardmap.py stays whole for now and the split rides issue #430 after the 0.50 release train. Per the ruling, the question-(1)/(2)/(6) folds (a99d4081, 5f75bf6e, 20fc83df) added as little to the module as possible: short extensions to the helpers the guard already owns, no new abstractions, no split started here.

[espg]

🤖 from Claude

Status: all three phases done, all ten review findings answered, CI green on 129086f. Marked waiting — question (7) needs a ruling.

• CI: ruff, build, build / build-x86_64, build / build-arm64, test (3.12) and test (3.13) all pass on the pushed head (run 32136144270). No stall this time; the earlier phase-1 run's stall cleared on its own.
• Local on the same head: pytest -q --deselect tests/test_lambda_build.py → 4417 passed, 38 skipped.
• Findings: four from the first pass, six from the independent pass, every one replied to on its thread — seven folded, three left standing as questions (1), (6) and (7).

waiting rather than nothing-to-continue because question (7) is a genuine fork I should not pick: the sweep fold now refuses where it previously succeeded by silently collapsing, so an unattended rollup node gets skipped instead of written. The three options are laid out in the description; (a) is what is currently implemented, and switching to (b) or (c) is a small change once ruled.

Questions (1) and (6) are review questions rather than blockers — the PR is coherent as it stands under either answer.

The draft→ready flip is not taken, per the standing conflict 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

[espg]

🤖 from Claude (review)

MEDIUM — the two spellings share ONE namespace, so the guard refuses pairs that collide under neither driver (and double-counts the ones that do).

The comment right above says the invariant is per-driver:

# Registered under EVERY spelling's canonical: a collision in the
# spelling the run's driver picks is a collision, whichever it is.

and _recorded_identity's docstring says the same — "the invariant must hold under either choice". But by_canonical is a single dict keyed on the canonical alone, so an s3 basename and an https basename land in the same bucket. That implements "a collision in the union of the two spellings", which is strictly wider than "a collision under s3 OR a collision under https".

Verified on the pushed head (20fc83df):

a = {"id": "A", "s3": "s3://b/p/X.h5", "https": "https://h/p/Y.h5"}
b = {"id": "B", "s3": "s3://b/p/Y.h5", "https": "https://h/p/Z.h5"}
# s3 canonicals:    X.h5, Y.h5   -> distinct
# https canonicals: Y.h5, Z.h5   -> distinct
shardmap._refuse_basename_collisions([7], [[a, b]])

ValueError: ShardMap: 1 per-shard granule identity collision(s) ... shard 7 'Y.h5' <- ['s3://b/p/X.h5', 's3://b/p/Y.h5']

No driver choice makes these two record as one id, yet the build is refused — and the report names 'Y.h5' as "the collapsed id", which is a's https basename and b's s3 basename, an id no single run ever records. Replaying the same pair through the pre-fold predicate (href = s3 or https) accepts it, so this is new in a99d4081, not pre-existing.

Second symptom of the same root cause — a genuine collision whose two spellings differ per entry is counted and listed twice, burning two of the three display slots on one pair:

c = {"id": "C", "s3": "s3://b/p1/G.h5", "https": "https://h/p1/G.nc"}
d = {"id": "D", "s3": "s3://b/p2/G.h5", "https": "https://h/p2/G.nc"}

ShardMap: 2 per-shard granule identity collision(s) ... shard 7 'G.h5' <- ['s3://b/p1/G.h5', 's3://b/p2/G.h5']; shard 7 'G.nc' <- ['s3://b/p1/G.h5', 's3://b/p2/G.h5']

One collision, reported as two, with both groups naming the same pair of entries.

Both fall out if the spelling is part of the key — e.g. by_canonical.setdefault((i, canonical), {}) over enumerate(canonicals), or two dicts merged for the report. Neither shape is exercised by a test: test_https_collision_under_distinct_s3_basenames_is_refused only pins the case where the s3 canonicals differ and do not cross-match, so nothing here fails if the namespaces are separated. Both fixtures above are cheap additions to TestBasenameCollisions.

I checked the realism caveat before filing: on real CMR catalogs the s3 and https basenames of one granule are identical, so both shapes need a catalog where they diverge. But the guard is a hard ValueError at build time, and the widening question (1) authorized was "also refuse an https collision", not "refuse a cross-spelling coincidence".

[espg]

🤖 from Claude

Fixed in 56f09c23 — the canonical is now tagged with the spelling that yields it, so ids are only ever compared within a spelling.

_recorded_identity returns (("s3", <basename>), ("https", <basename>)) (falling back to ("id", ...) for the href-less raster entries), and the bucket key is that pair, not the bare canonical:

for slot_canonical in canonicals:
    by_canonical.setdefault(slot_canonical, {})[distinguishing] = label

The two spellings therefore never meet. _resolve_urls confirms the semantics — key = "https" if driver == "https" else "s3", one key per run, and records lacking it are dropped — so an s3 id and an https id are ids no single run records together.

Your X/Y — Y/Z fixture now builds; pinned as test_a_basename_shared_across_spellings_is_not_a_collision, which asserts up front that neither spelling collides on its own (else the fixture would be pinning nothing).

The double-count/double-list symptom is fixed at the report: a pair that collides under both spellings registers in both slots, so groups are deduped on the set of distinguishing tuples they name — one entry-pair, one group.

if len(named) > 1 and frozenset(named) not in pairs:
    pairs.add(frozenset(named))
    found.append((canonical, sorted(named.values())))

Your G.h5/G.nc fixture is test_a_pair_colliding_under_both_spellings_is_reported_once, asserting 1 per-shard granule identity collision(s) and exactly one occurrence of s3://b/p1/G.h5 in the message. The ordinary same-basename-both-spellings collision goes through the same dedupe, so test_an_href_collision_is_still_named_by_its_hrefs and the coarsen tests are unchanged; test_https_collision_under_distinct_s3_basenames_is_refused still refuses, and test_more_than_three_collisions_are_counted_and_truncated still shows three of four.

Full suite green: pytest tests/test_shardmap.py tests/test_sweep.py -q → 233 passed, 1 skipped.

[espg]

🤖 from Claude (review)

MEDIUM — stacklevel=3 is one frame short on every production path, so this warning blames shardmap.py instead of the caller; the tests can't see it because they alone call at depth 3.

_basename_collision_message is never reached directly in production — it is always one frame further down than the tests reach it:

call path	frames from warnings.warn	what stacklevel 3 points at
shardmap._refuse_basename_collisions(...) (tests only)	msg → refuse → test	the test — correct
ShardMap.build	msg → refuse → build → caller	shardmap.py _refuse_basename_collisions(shard_keys, granules)
reproject	msg → refuse → reproject → caller	shardmap.py reproject
SubmapFamily.merge	msg → refuse → merge → caller	sweep.py merge
from_json / from_parquet	msg → _warn_loaded_collisions → from_json → caller	shardmap.py from_json

Measured on 20fc83df — a manifest with one identity-less entry, loaded from a script whose ShardMap.from_json(p) call is on line 21:

-> shardmap.py line 1958 | ShardMap: 1 shard entry(s) carry no recorded identity — noth...

Line 1958 is return _warn_loaded_collisions(sm, path). The operator gets a warning pointing into zagg's own source, and -W error::RuntimeWarning:__main__-style module filters won't match it. Note the sibling warning in _warn_loaded_collisions is correct at stacklevel=3 (verified: it reports the caller's line 10), which is what makes the mismatch easy to miss — the two warnings sit at different depths.

Both tests that pin this warning (test_an_entry_with_nothing_to_canonicalize_warns_and_is_skipped, test_an_id_that_canonicalizes_to_empty_warns_and_is_skipped) call _refuse_basename_collisions directly — the one path where 3 happens to be right — and neither asserts record[0].filename, so the suite is blind to it.

Since the correct depth differs per caller (4 via _refuse_basename_collisions, 4 via _warn_loaded_collisions), the honest fix is to pass the depth in, or to return the unidentified report alongside the message and let each caller warn at its own level.

Related, same warning: it is the only one of the two that does not carry the manifest path. Loading a legacy map emits ShardMap: 1 shard entry(s) carry no recorded identity ... First: shard 1050 'None'. with no filename anywhere in the text and (per the above) no usable source location either — while the collision warning two lines away is prefixed f"{path}: ...".

[espg]

🤖 from Claude

Fixed in ba35f6bd, both halves.

_basename_collision_message now takes the depth from its caller — def _basename_collision_message(shard_keys, granules, *, stacklevel: int = 3, source: str | None = None) — and both wrappers pass 4: _refuse_basename_collisions(...) for build/reproject/merge, _warn_loaded_collisions(...) for from_json/from_parquet. The sibling collision warning inside _warn_loaded_collisions stays at 3, unchanged and still correct.

Measured rather than reasoned, per your method. Load path, a manifest with one identity-less entry loaded from a script whose ShardMap.from_json(p) is on line 14:

LOAD  -> fold482b_frames.py 14 /…/fold482b_sm.json: ShardMap: 1 shard entry(s) carry no recorded identity…

Build-shaped path, a stand-in wrapper calling _refuse_basename_collisions with the call site on line 9:

BUILD-LIKE -> fold482b_frames2.py line 9

Both land on the caller. The direct _refuse_basename_collisions(...) calls the tests make now attribute past the test frame, which is why the new regression test goes through a real entry point instead: test_the_unidentified_warning_blames_the_caller_not_shardmap asserts record[0].filename == __file__ after ShardMap.from_json. Verified it fails against the old depth (stacklevel=3 from _warn_loaded_collisions → AssertionError, the filename is shardmap.py).

The manifest path was cheap, so it is in: _warn_loaded_collisions passes source=path and the warning is prefixed f"{source + ': ' if source else ''}", matching the collision warning two lines away. The same test pins str(path) in str(record[0].message). Nothing else passes source — build/reproject/merge have no manifest to name, and the fixed stacklevel gives them the caller's own line instead.

[espg]

🤖 from Claude (review)

LOW — the "First:" pointer names the entry 'None' in exactly the case the warning exists for.

_collision_label was written to name a colliding entry, and its last line is a fallback for entries that always have at least an id:

return str(entry.get("id") or entry.get("datetime"))

An entry with no recorded identity is by construction one where nothing on it canonicalizes, so the common shape is {"id": None, "s3": None, "https": None} — and str(None) is the string "None". Measured on 20fc83df:

ShardMap: 1 shard entry(s) carry no recorded identity — ... First: shard 7 'None'.

The clause promises to point the operator at one offending entry and points at nothing. The other unidentified shape this PR pins — {"id": "/"} — labels as '/', which is at least real, but the all-None shape is the one test_an_entry_with_nothing_to_canonicalize_warns_and_is_skipped uses, and its assertions stop at "2 shard entry(s)", so the 'None' never shows up in a failure.

Cheapest fix: label the unidentified entry by its raw payload rather than through _collision_label, e.g. {k: entry.get(k) for k in ("id", "s3", "https", "datetime")}, which is what an operator would need to find the record anyway. Then extend the existing test's assertion past the count to the First: clause.

[espg]

🤖 from Claude

Fixed in 9921f0aa. Put it in _collision_label's final fallback rather than at the pointer, so every caller of the label gets it:

named = entry.get("id") or entry.get("datetime")
return str(named) if named else "<entry with no id, href, or datetime>"

Went with the self-describing string over the raw-payload dict because the warning already says exactly which fields it looked at ("nothing on them canonicalizes to a granule id"), and a dict of four Nones reads as noise beside it; the {"id": "/"} shape still labels as '/', unchanged.

test_an_entry_with_nothing_to_canonicalize_warns_and_is_skipped now asserts past the count: "'None'" not in message and "<entry with no id, href, or datetime>" in message.

[espg]

🤖 from Claude

espg ruled on all seven "Questions for review" (in-session, 2026-08-24); this run applied the rulings. All seven are now marked RESOLVED in the PR body with the rulings recorded inline. Summary:

Q	Ruling	Where it landed
(1)	Close the driver hole — canonicalize BOTH url spellings so an https-basename collision under distinct s3 basenames is also refused; widening the guard is the point	a99d4081 (+ 56f09c23, see below)
(2)	Warn on load — from_json / from_parquet warn, never refuse; historical maps stay readable, the hazard visible	5f75bf6e
(3)	Overage acknowledged — the shardmap.py split rides issue #430 after the 0.50 release train; these folds added minimally	body note
(4)	Stays as-is — pre-existing lint on main, untouched	—
(5)	RESOLVED by split — filed as issue #512 (small-fix) at espg's direction; the question-(2) load-warning is its interim mitigation	#512
(6)	Warn on the silent skip — an entry with no recorded identity warns instead of vanishing; not refused, since _resolve_urls drops href-less records by design	20fc83df
(7)	Option (a), zero code change — the sweep-fold refusal stays; a skipped rollup node with a collision-naming warning is correct (rollups are regenerable caches; warn-and-collapse would write a wrong artifact; fatal would break fail-open)	no change

Mechanics of this round: the branch was first brought up to date with main (regular merge, aeb1b1a7), then one fold commit per code ruling. The per-phase fresh-context adversarial review then ran over the fold diff and posted three inline findings — the substantive one being that the question-(1) implementation pooled both spellings into one namespace (a cross-spelling false refusal the ruling's per-driver semantics forbid). All three were folded: 56f09c23 (compare within a spelling, never across; one entry-pair reports once), ba35f6bd (unidentified-entry warning attributed to the caller, manifest path included on the load path), 9921f0aa (self-describing label for the no-identity pointer). Every review thread has a reply with its fix sha.

Local gate on the pushed head: ruff check / ruff format --check clean apart from the two pre-existing question-(4) items; pytest tests/test_shardmap.py tests/test_sweep.py -q -> 233 passed, 1 skipped; full suite pytest -q --deselect tests/test_lambda_build.py on the phase-4 head -> 4665 passed, 37 skipped, 1 pre-existing unrelated failure (test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries, byte-identical to main and failing on main's checkout locally too — flagged per section 4, not fixed).

The PR stays labeled implement + waiting: it rides the 0.50 merge train, and with all phases done and findings folded it is being marked ready for review per section 2 once CI concludes green on 9921f0aa.