small fixes 2026-08-11: granule_count skip-key term; objects_overviews → objects_sweep rename - #435
small fixes 2026-08-11: granule_count skip-key term; objects_overviews → objects_sweep rename#435espg wants to merge 9 commits into
Conversation
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial self-review of phase 1 (896a183), read against CLAUDE.md and issue #433's acceptance criteria.
The change itself is correct and the acceptance evidence is real. The fourth term is sourced from the child's own stamp rather than its relayed block, which is the same lesson PR #421's phase-1 review had to teach (1d7d80a) and is what makes the term non-empty at the finest tuple instead of decorative. The leaf-arm test injects no stamp grammar — it asserts "run_id" not in stamp and rides what write_column actually writes — and both new tests were shown to fail against the base commit. The _summed_generation sum and the stage column's own stamp are computed over the same readers.values(), so the term ratchets without double-counting. sweep_overview.py's cascade blocks are correctly left alone (#417 set that precedent).
Four findings, one of them blocking:
- (blocking, §4)
sweep_stage.pyis now 1,202 lines, over the module cap — 1,194 at the base. §4 says raise it before going past 1,200, not after. Seven of the ten added lines are docstring prose duplicating whatzagg.column.generation_keyalready documents; tighten those rather than splitting the module. docs/specification.md§4.5 claims a guarantee the code does not provide — "the fold it serves is the same content in every case a writer produces" is false for a same-granule re-run that produces different bytes, and this page is the normative one external readers decode from. State the residual as a known boundary (which is what the PR body's question (1) is asking to be ruled on) rather than as a property.generation_keyhas no direct test despite changing arity and carrying a MUST in §4.5 — term order, the()sentinel, and the both-terms-absent block are unpinned.stamped_generation_keyunionsrun_idsbut replacesgranule_count, and the invariant that makes the replacement safe lives in another module. Say so at the override.
Findings (2) and (3) are non-blocking for phase 2 under §2; (1) should be resolved before phase 2 adds to the same file (it does not — phase 2 touches .github/scripts/ only — but the cap breach stands on its own).
Generated by Claude Code
| "n_leaves": int(n), | ||
| "max_leaf_timestamp": max(stamps) if stamps else None, | ||
| "run_ids": sorted(runs), | ||
| "granule_count": int(granules), |
There was a problem hiding this comment.
🤖 from Claude (review)
Blocking: this phase pushes sweep_stage.py past the §4 module cap.
$ wc -l src/zagg/sweep_stage.py # this commit
1202
$ git show 671834c:src/zagg/sweep_stage.py | wc -l # base
1194
CLAUDE.md §4 pre-approves overages below 1,200 and says that if a file is heading past 1,200 you stop and raise it before continuing — not that you land the overage and mention it. PR #421 was explicitly written to keep this module under the cap (its body records "which also keeps sweep_stage.py under the §4 module cap, at 1,194 lines against 1,185 on main"), so the headroom was already known to be eight lines and this phase spent ten.
The behavioral part of the change is 3 lines (the accumulator, the unpack, the emitted key); the other 7 are docstring prose I added on top of prose #417 had already expanded. Bring the file back under 1,200 by tightening the prose this phase added — the module docstring paragraph and _summed_generation's docstring both now restate what zagg.column.generation_key documents at length, and the cross-reference is already there. Do not split the module to make room: that is the thing §4 says to raise first.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in ef36dcf — src/zagg/sweep_stage.py is back to 1,198 lines, under the cap, and no code moved out of the module.
The finding is correct on both counts: the file was at 1,202 and §4 says raise before crossing, not after. What came out is the prose this phase added on top of zagg.column.generation_key's own documentation — the module-docstring sentence is one clause again, _summed_generation's docstring is back to two lines pointing at generation_key, and _ColumnReader.generation's lost a line. The three behavioral lines (accumulator, unpack, emitted key) all stayed. A reflow left over from the phase-1 edit also cost a line and is gone.
$ wc -l src/zagg/sweep_stage.py
1198
tests/test_column.py hit the same wall while folding the finding below (it landed at 1,204 with the new unit class) and was trimmed to 1,199 in 63bffa2 before that commit was pushed — flagging it because the cap applies to the test module too and the fix was the same kind of trim, not a split.
Generated by Claude Code
| more rather than inheriting the blind spot. A rewrite that changes neither | ||
| the leaf count, the second, the writing run nor the granule count is | ||
| invisible to the gate by construction; the fold it serves is the same | ||
| content in every case a writer produces, and the `/1` content hash remains |
There was a problem hiding this comment.
🤖 from Claude (review)
The normative text asserts a guarantee the implementation does not provide.
A rewrite that changes neither the leaf count, the second, the writing run nor the granule count is invisible to the gate by construction; the fold it serves is the same content in every case a writer produces, and the
/1content hash remains the divergence backstop.
The first clause is right and belongs in the spec. The bolded one is false, and this page is the contract external readers (moczarr) decode against — it must not claim a property nobody enforces. Counterexamples a writer really produces: a fleet leaf re-run over the same granule set after a code or field-declaration change; a repaired torn write; any re-run whose inputs are identical but whose fold differs. Each lands inside the second, at an unchanged leaf count, with no run_id and an unchanged granule_count — different bytes, identical key.
That residual is exactly what PR body question (1) is raising for a ruling, so it should be stated as a known boundary, not laundered into a guarantee. Suggested shape: keep the boundary sentence, drop the claim, and let the /1 content hash carry the backstop role it already has.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in 74c4629 — the claim is gone and the residual is now labelled as what it is. §4.5 reads:
Known boundary. A rewrite that changes none of the four terms — same leaf count, same recorded second, no
run_id, samegranule_count— is invisible to the gate. A fleet leaf re-run over the same granule set that nonetheless folds different bytes (a changed field declaration, a repaired torn write) is the shape of it. The gate is a staleness ratchet, not a content check: it deliberately decides without folding, and the/1content hash is where content divergence is caught.
The finding is right that a normative page must not assert an unenforced property, and the counterexamples it names are the ones now written into the text. The framing also matches what the PR body's question (1) asks to be ruled on, so the spec and the question no longer say different things.
Generated by Claude Code
| int(block.get("n_leaves") or 0), | ||
| block.get("max_leaf_timestamp"), | ||
| tuple(sorted(set(block.get("run_ids") or ()))), | ||
| int(block.get("granule_count") or 0), |
There was a problem hiding this comment.
🤖 from Claude (review)
A contract-bearing public function changed arity and still has no direct test.
generation_key is public in zagg.column, its tuple is written into docs/specification.md §4.5 as normative, and its additive rule ("absent MUST read as the zero, never as a wildcard") is a MUST in that section. Every test that touches it drives a full sweep, so what is actually pinned is the sweep's behavior, not the key's:
- nothing asserts the tuple's shape or term order — a future edit that appends
granule_countin position 2 instead of 4 changes no test; - nothing asserts
generation_key(None) == ()/ the "matches no generation" property the docstring claims; - the additive rule is pinned only indirectly, through a 7-artifact sweep and an envelope edit, for
granule_countandrun_idsseparately but never for a block missing both (a genuinely pre-staged sweep skip gate: same-second rewrite blind spot — close it with the run_id already in the stamps #417 store, which is the case the rule was written for).
These are three cheap assertions against a pure function. The behavioral tests are the right acceptance evidence and should stay; this is the unit-level complement that makes the spec sentence enforceable. stamped_generation_key deserves the same treatment for its leaf arm (no block, stamp with granule_count and no run_id → the leaf identity plus the granule term), which is the arm this whole issue is about and is currently reachable only through _ColumnReader.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in 63bffa2 — TestGenerationKey in tests/test_column.py, seven cases against the two pure functions, placed before TestColumnResolutions so the test file's order mirrors zagg/column.py's.
All three gaps the finding names are now pinned, plus the two the leaf arm needs:
- term order and normalization —
generation_key(BLOCK) == (2, ts, ("stage-a", "stage-b"), 7), with the ids recorded out of order in the fixture so the sorted-set normalization is asserted rather than incidental; - the sentinel —
generation_key(v) == ()forNone,[],"generation"and0; - the additive MUST, parametrized over
run_idsandgranule_count: dropping either moves the key, and the dropped form equals the explicit-Noneform (so "absent" and "null" cannot diverge). A separate case covers a block missing both — the genuinely pre-staged sweep skip gate: same-second rewrite blind spot — close it with the run_id already in the stamps #417 store — keying on(2, ts, (), 0); - the leaf arm (this issue's) — no block, a stamp with
granule_countand norun_id→(1, written_at, (), 3), and appending a granule moves it; - the stage arm — the id unioned onto the block's, the count taken off the stamp, and a pre-small fixes: granule_count term in the staged-sweep skip key; objects_overviews → objects_sweep rename #433 block (no count, stamp has one) reading
7rather than0.
The sweep-level tests stay exactly as they were; this is the unit complement, not a replacement.
Generated by Claude Code
| return generation_key({**block, "run_ids": sorted(runs)}) | ||
| return generation_key( | ||
| {**block, "run_ids": sorted(runs), "granule_count": stamp.get("granule_count")} | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
The two stamp-sourced terms are composed differently and the docstring does not say why.
return generation_key(
{**block, "run_ids": sorted(runs), "granule_count": stamp.get("granule_count")}
)run_ids is unioned with whatever the relayed block carries; granule_count replaces it outright. For a stage column that is safe — the recorded block and the stamp are both summed over the same readers.values() at the relay-column write, so they are equal by construction — but that invariant lives in sweep_stage.py, not here, and nothing in this function's docstring or in a test says it holds. A future reader has two ways to "fix" the asymmetry (union it, or max() it), and one of them (summing block + stamp) would double-count every granule at every level.
Either state the invariant in the docstring where the override happens ("a stage column's stamp granule_count and its recorded block's are the same sum over the same children, so replacing is not losing information; unioning or adding would double-count"), or read the term the same way the run ids are read. I'd take the docstring — the override is the simpler code and it is what makes the pre-#433 stage column upgrade cleanly (empty block term, populated stamp).
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Took the docstring option, in 45532c0. stamped_generation_key now states the asymmetry and the invariant behind it at the point of the override:
The two are composed differently ON PURPOSE (review finding): the run id is UNIONED with the block's, the granule count REPLACES it. A stage column's stamp count and its recorded block's are the same sum over the same children (
zagg.sweep_stage._summed_generationand the relay column's stamp both reduce that run's readers), so replacing loses nothing and upgrades a pre-#433 block — which carries no count — off the stamp. Unioning or ADDING them would double-count every granule at every level of the ladder.
The code is unchanged, for the reason the finding itself gives: the override is what makes a pre-#433 stage column upgrade cleanly, and max() would silently absorb a shrinking granule count (a leaf re-run over a narrower set — legal, and a real staleness signal). The invariant is also now executable rather than only prose — TestGenerationKey.test_the_stage_arm_unions_the_id_and_takes_the_stamp_count (63bffa2) asserts both halves, including the pre-#433 block reading its count off the stamp.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial self-review of phase 2 (9d52ed2), read against CLAUDE.md and issue #433's acceptance criteria.
The rename is complete and correctly fenced. A repo-wide grep for objects_overviews is empty after the diff (the only surviving occurrence is the docstring clause naming the old name), and the two files touched are exactly the ones issue #433 authorizes plus their test — no .github/workflows/*.yml was touched, which is right: none mentions the key, and §1 would forbid it if one did. The genuinely overview-specific names (_overview_node, overview_nodes, the comments about overview zarrs) are correctly left alone — they name a family, not the bucket, and renaming them would have been the easy over-reach here. test_measured_keys_are_the_documented_contract is the right regression guard for a rename whose readers all go through .get.
Three findings, none blocking:
sweep_objectsbreaks the accumulator naming pattern it sits in (metadata/rollups/telemetry) and inverts the key's word order —sweepis what §4's "match the surrounding code" gives.- The docstring reflow left a 36-character orphan line mid-paragraph, and the "renamed from
objects_overviewson issue #433" clause is changelog in a contract docstring. write_path_totalreads the renamed key through.get(..., 0), so a producer that misses a rename under-subtracts silently instead of raising. Pre-existing shape, not introduced here, but the PR body should say whether that tolerance is load-bearing (historical rows) or vestigial.
Also flagging, not as a defect: renaming the local accumulator is slightly wider than the issue's letter. Issue #433 authorizes the key; the variable carried the same one-family-name-for-two-families defect, so the spirit covers it — but it should be stated in the PR body rather than discovered in the diff.
Generated by Claude Code
| metadata = 0 | ||
| rollups = 0 | ||
| overviews = 0 | ||
| sweep_objects = 0 |
There was a problem hiding this comment.
🤖 from Claude (review)
The accumulator's new name breaks the pattern of the three it sits with.
metadata = 0
rollups = 0
sweep_objects = 0
telemetry = 0Every other accumulator in this block is a bare plural noun matching the tail of its key (objects_metadata → metadata, objects_rollups → rollups, objects_telemetry → telemetry). objects_sweep → sweep_objects both breaks that and inverts the key's word order, so the one line where the rename is most visible is the one that reads least like the key it emits. sweep is the name the pattern gives, it collides with nothing in this function's scope, and it makes the emit line "objects_sweep": sweep, — symmetric with its three neighbours. CLAUDE.md §4: match the surrounding code.
Renaming the accumulator at all is a judgement call worth stating explicitly in the PR body, incidentally: issue #433 authorizes the key rename, and the local variable carried exactly the same defect (it counted two families under a one-family name), so it is in the spirit of the ruling — but it is not literally what the issue names.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in a1af00d — the accumulator is sweep, so the block reads:
metadata = 0
rollups = 0
sweep = 0
telemetry = 0and the emit line is "objects_sweep": sweep,, symmetric with its three neighbours. Nothing else in store_object_counts's scope is named sweep, and the three increment sites moved with it.
On the second half — recorded in the PR body under phase 2 rather than left in the diff. The variable is not literally what issue #433 names, but it carried the identical defect (one accumulator counting overview zarrs and stage columns under a name that said overviews), and leaving it would have made the emit line read "objects_sweep": overviews — the exact mismatch the issue exists to remove. _overview_node and overview_nodes were deliberately not touched: those name the overview-zarr family, which is real and unchanged.
Generated by Claude Code
| SWEEP bucket, of which overview zarrs are one family and ancestor-node | ||
| stage columns (issue #418) another — renamed from ``objects_overviews`` | ||
| on issue #433, since the key held two families and its name said one. | ||
| ``objects_per_shard`` keys are the |
There was a problem hiding this comment.
🤖 from Claude (review)
The reflow left a ragged orphan mid-paragraph.
stage columns (issue #418) another — renamed from ``objects_overviews``
on issue #433, since the key held two families and its name said one.
``objects_per_shard`` keys are the
dispatched shards' external labels (``grid.shard_label``); a data object
objects_sweep is four characters shorter than the name it replaced, so the paragraph no longer fills — line 425 stops at 36 characters in the middle of a filled block. Cosmetic, but this is the module's return contract and it is the first thing a reader of the rename sees. Re-fill the paragraph.
Also worth a second look while the text is being touched: "renamed from objects_overviews on issue #433" reads like changelog. The docstring's job is to say what the key is; the old name is useful for one release and then it is noise. Consider dropping the clause and letting git log carry the provenance, or at minimum shortening it to a parenthetical.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Fixed in cae7a18 — the paragraph is re-filled and the changelog clause is gone. It now reads:
Returns ``{"objects_total", "objects_metadata", "objects_per_shard",
"objects_rollups", "objects_sweep", "objects_telemetry", "objects_other",
"other_keys"}``. ``objects_sweep`` is the SECOND-PASS SWEEP bucket, of
which overview zarrs are one family and ancestor-node stage columns
(issue #418) another — named for the pass, not for one of its families
(issue #433). ``objects_per_shard`` keys are the dispatched shards'
external labels (``grid.shard_label``); a data object whose block resolves
to an undispatched shard is keyed ``"block:<n>"`` so a stray write is
visible rather than silently pooled. ``other_keys`` is a capped sample of
unclassifiable keys.
Both points taken. The old name is out of the prose entirely — "named for the pass, not for one of its families (issue #433)" says the same thing without dating the docstring, and it explains the rule (name the bucket after what fills it) rather than recording an event. The reflow reaches the end of the paragraph, not just the edited lines, since two more lines downstream had gone ragged for the same reason.
Generated by Claude Code
| measured["objects_total"] | ||
| - measured.get("objects_rollups", 0) | ||
| - measured.get("objects_overviews", 0) | ||
| - measured.get("objects_sweep", 0) |
There was a problem hiding this comment.
🤖 from Claude (review)
A renamed key read through .get(..., 0) fails silently, which is the one failure mode a rename introduces.
return (
measured["objects_total"]
- measured.get("objects_rollups", 0)
- measured.get("objects_sweep", 0)
- measured.get("objects_telemetry", 0)
)objects_total is subscripted; the three subtrahends are not. If any producer of measured misses the rename, write_path_total does not raise — it silently under-subtracts and reports an inflated write-path total, which then fails the audit as a mismatch somewhere else entirely. That is a worse diagnostic than a KeyError naming the key.
The new test_measured_keys_are_the_documented_contract guards the one producer in this repo, which is why I am not calling this blocking. But two questions the PR body should answer rather than leave implicit:
- is
write_path_totalever handed a record read back from persisted metrics history (where a pre-rename row genuinely lacks the key), or only a freshstore_object_countsdict? If only the latter, the.gettolerance buys nothing and the subscript is strictly better; - if it is ever handed a historical row, then
.getis load-bearing and silently returning 0 for a pre-small fixes: granule_count term in the staged-sweep skip key; objects_overviews → objects_sweep rename #433 row is the intended behavior — in which case say so here, because right now nothing distinguishes the two readings.
Either way this is a pre-existing shape the rename inherited, not something the diff introduced; if the answer is (2), leave the code and document it.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Answered and documented in 427e632; the code is left as it is, because the answer is (2) — the tolerance is load-bearing, though not for the reason the finding guessed.
It is not about historical rows. objects_overviews was never recorded into a metrics record — bench_metrics threads objects_total / objects_write_path / objects_expected / objects_per_shard / objects_telemetry / objects_mismatch and stops — so no persisted row has ever carried the key under either name, and there is no history to be tolerant of.
What the .get is actually for is object_count_mismatch, which calls write_path_total on a partial measured. test_hive_metadata_ceiling_covers_sweep_written_root_moc is the live example — it builds
measured = {
"objects_total": 3 + exp["per_shard_max"],
"objects_metadata": 3,
"objects_per_shard": {_KEY_A: exp["per_shard_max"]},
"objects_other": 0,
"other_keys": [],
}with no sweep buckets at all, and asserts object_count_mismatch(measured, exp) is None. Subscripting would turn every such caller into a KeyError, forcing explicit zeros for buckets a caller has no opinion about. So the finding's option (1) is wrong for this codebase and the code stays.
The docstring now says so, including the cost the finding correctly identifies:
Only
objects_totalis subscripted; the three subtrahends are read tolerantly ON PURPOSE (review finding) — :func:object_count_mismatchaccepts a PARTIALmeasuredcarrying just the terms an assertion needs, so a caller that never swept omits the sweep buckets rather than writing zeros. The cost of that tolerance is that a bucket RENAME would under-subtract silently instead of raising; the key set the measured side emits is pinned directly against this docstring so a half-done rename fails there first.
That last clause is test_measured_keys_are_the_documented_contract, which the finding already identified as the guard — it is now named in the place a future renamer will be reading.
Generated by Claude Code
|
🤖 from Claude Both phases are complete, all seven self-review findings are folded with a reply and a fix sha on every thread, and the local suite is green at the branch tip — Labelled Three things worth a reviewer's eye specifically:
Two process notes, disclosed rather than buried:
Generated by Claude Code |
|
🤖 from Claude Closing the loop on the one item left open above: CI is fully green at Generated by Claude Code |
2 participants
Closes #433.
Two ruled follow-ups from PR #421's review questions, bundled per CLAUDE.md §5. They share no code: phase 1 is the staged sweep's skip gate, phase 2 is the CI object-count model's bucket name.
Phases
896a183, foldsef36dcf/74c4629/63bffa2/45532c0):granule_countbecomes the key's fourth term, closing the finest dispatch tuple whose children carry norun_id.objects_overviews→objects_sweep(9d52ed2, foldsa1af00d/cae7a18/427e632): mechanical rename + grep-clean in.github/scripts/bench_objects.pyand its test.Phase 1 —
granule_countas the fourth key term (issue #433 (1), PR #421 Q1 option 1c)The residual blind spot. PR #421 closed the same-second rewrite hole for stage children by adding
run_idsto the skip key. It could not close the finest dispatch tuple, whose children are fleet-written leaf columns:column.write_columnstamps throughhive.stamp_commitwithout arun_id, and specification §4.6 makes that absence load-bearing ("Fleet-written leaves and columns never carry it; readers treat absence as 'not a stage artifact'"). So at that tuple the id term is empty on both sides, and a leaf rewritten inside its own recorded second at an unchangedn_leaves+ max stamp timestamp is invisible to the gate.The fix — option 1c, no writer touched. Every commit stamp, fleet or stage, already records
granule_count(hive.stamp_commitwrites it unconditionally), and the stage worker already reads the child's whole stamp. It becomes the key's fourth term:stamped_generation_keytakes it off the child's own stamp, exactly as it already takes the run id — the same reasoning as PR #421's phase-1 fold (1d7d80a): the relayed block describes the child's children, while the stamp describes the child._summed_generationsums it into the recorded block, so the term ratchets up the ladder liken_leavesdoes.The two stamp-sourced terms are composed differently on purpose: the run id is unioned with the block's, the granule count replaces it. For a stage column the two agree by construction —
_summed_generationand the relay column's own stamp both reduce the samereaders.values()— so replacing loses nothing, and it is what lets a pre-#433 stage column (block with no count, stamp with one) upgrade off the stamp. Unioning or adding would double-count every granule at every level. That is now stated at the override and asserted inTestGenerationKey.Zero extra I/O, zero writer change, no change to fleet stamp semantics.
Additive, never a wildcard. An absent
granule_countreads as0, matching #417's rule forrun_ids: a store swept before the term existed folds once more rather than inheriting the blind spot. Pinned bytest_entry_without_a_granule_count_refolds_once(7 written, 0 current) alongside the existingtest_entry_without_run_ids_stays_current(0 written, 7 current), so the two upgrade paths are pinned in opposite directions.Spec (§4 obligation). §4.5's normative block gains the fourth term, the additive rule for it, the statement of which arm each of the last two terms covers (stage children →
run_ids; fleet-written leaf children →granule_count), and a Known boundary paragraph for what the key still cannot see (question (1) below). §4.4 and §4.6 spell the block as{n_leaves, max_leaf_timestamp, run_ids, granule_count}. §4.8's concurrency split records that a fleet rewrite carries norun_idfor either mechanism to see, which is why the fourth term exists.docs/hive_layout.md's soft-barrier paragraph gets the narrative half.Conformance fixtures: unaffected, re-verified. No fixture under
tests/data/spec/carries agenerationblock at all (grep -rl generation tests/data/spec/is empty) —tests/data/spec/column/is a fleet-written leaf column, whosezagg_columnpayload has nogeneration(that block is written only bywrite_stage_column), andtests/data/spec/pyramid/is a manifest whose stage content is the per-entryactuals, untouched here.tools/generate_spec_fixtures.pymaterializes no stage artifact. So there is no fixture surface to regenerate;tests/test_spec_fixtures.pypasses untouched. Same finding as PR #421, re-checked rather than assumed.Tests. The issue asks for the #417 fixture's leaf-arm variant:
test_same_second_fleet_rewrite_of_a_leaf_is_refoldedre-runs leaf1111over one more granule and restamps it to its own recorded second without injecting a run id — it asserts"run_id" not in stamp, so the arm rides the stampwrite_columnactually produces rather than injected grammar (the weakness PR #421's self-review caught in its first draft). It then asserts the rewrite's partial (136 * 10) reaches every level above, not just the immediate parent.TestGenerationKeyintests/test_column.pyadds the unit-level complement the review asked for: term order, the()sentinel, the additive rule per term and for a block missing both, and both arms ofstamped_generation_key.Both new sweep tests were verified to fail against the base commit: with
src/zagg/column.pyandsrc/zagg/sweep_stage.pystashed back to671834c,test_same_second_fleet_rewrite_of_a_leaf_is_refoldedandtest_entry_without_a_granule_count_refolds_onceboth fail; they pass at896a183.Module cap (§4). Phase 1 as first written took
src/zagg/sweep_stage.pyto 1,202 lines, over the cap; the self-review caught it andef36dcfbrought it back to 1,198 by tightening the prose the phase had added on top ofzagg.column.generation_key's own documentation — no code moved out of the module.tests/test_column.pyhit the same wall when the unit class landed (1,204) and was trimmed to 1,199 in the same commit that added it.Phase 2 —
objects_overviews→objects_sweep(issue #433 (2), PR #421 Q3)The key holds two artifact families — sweep overview zarrs (issue #201) and ancestor-node stage columns (issue #418) — and its name said one. PR #421 documented the mismatch rather than renaming, because the rename needed CI-change authorization; issue #433 grants it.
Blast radius is two files:
.github/scripts/bench_objects.py(named by the issue) andtests/test_benchmark_objects.py. A repo-wide grep forobjects_overviewsacrosssrc,tests,docs,.github,toolsanddeploymentis now empty. No.github/workflows/*.ymlwas touched — none mentions the key, so §1 never came into play. See question (2) for why the rename is narrower than the issue anticipated.The local accumulator moved with the key (
overviews→sweep), matching its three neighboursmetadata/rollups/telemetry: it counted both families under a one-family name, which is the same defect the key had, and leaving it would have made the emit line read"objects_sweep": overviews. The genuinely overview-specific names —_overview_node,overview_nodes, and the comments about overview zarrs — are deliberately untouched: they name a family, not the bucket.Test. The rename rides the existing assertions, plus one new guard:
test_measured_keys_are_the_documented_contractpins the returned key SET against the docstring. That matters becausewrite_path_totalreads the sweep-side buckets through.get(..., 0), so a half-done rename would under-subtract silently rather than raise. That tolerance is load-bearing and now documented at the function —object_count_mismatchis called with a partialmeasured(test_hive_metadata_ceiling_covers_sweep_written_root_mocpasses a five-key dict with no sweep buckets at all), so subscripting would turn those callers intoKeyErrors.Adversarial review (folded)
A fresh-context review ran after each phase and posted inline findings; all seven are folded, with a reply and a fix sha on every thread.
Phase 1 — the module-cap breach above (
ef36dcf); the spec asserting a guarantee the code does not provide, now a stated Known boundary (74c4629);generation_keychanging arity with no direct test, nowTestGenerationKey(63bffa2); and the union-vs-replace asymmetry of the two stamp-sourced terms, now stated at the override (45532c0).Phase 2 — the accumulator's name breaking its neighbours' pattern (
a1af00d); a ragged reflow and a changelog clause in the return-contract docstring (cae7a18); and the.get(..., 0)question, answered by investigation and documented rather than changed (427e632).Process deviation, disclosed: CLAUDE.md §2 asks for the review and the fold to run as separate Opus-class subagents. No agent-spawning tool was reachable in this environment, so both passes were run by the same agent as separate, explicitly-scoped passes — review first (findings only, no edits), then fold (one commit per finding, staging only the files that finding touches, a reply on every thread). The independence the rule buys was approximated, not guaranteed. The same deviation was recorded on PR #421.
How it was tested
uv run pytest -q(full suite, afteruv sync --extra test) → 3807 passed, 38 skipped at the phase-1 tip; re-run at the branch tip with the same result.uv run ruff check src tests .github/scripts→ clean apart from the pre-existingN818below.uv run ruff format --check src tests→ clean apart from the pre-existing markdown fence below. The PR'sruffCI job is green.src/zagg/column.pyandsrc/zagg/sweep_stage.pyback to671834c(above).uv run pytest tests/test_benchmark_objects.py -q(30 passed) plus the repo-wide grep.Questions for review
/1content hash named as the divergence backstop. Flagging it because closing it would need a per-artifact content term the skip gate exists to avoid computing — no action wanted unless you disagree with recording it as a boundary..github/scripts/bench_metrics.py,run_cold_benchmark.py/run_warm_benchmark.pyand "the rendered panel template" alongsidebench_objects.py. A repo-wide grep forobjects_overviewsreturns only.github/scripts/bench_objects.pyandtests/test_benchmark_objects.py:bench_metrics.pynever copies the key into a record (it threadsobjects_total/objects_write_path/objects_expected/objects_per_shard/objects_telemetry/objects_mismatchand stops), so no panel and norun_*_benchmark.pyreads it, and there is norun_cold_benchmark.py/run_warm_benchmark.pyin the tree (the entrypoints arerun_benchmark.py,run_full_aoi_benchmark.py,run_raster_benchmark.py). No persisted metrics row has ever carried the key under either name. Say the word if the key was meant to be added to the recorded set as part of this — that is a real gap (the second-pass bucket is invisible in metrics history today), but it is a model change rather than a rename and I did not smuggle it in.overviews→sweepis the variable that emits it, carrying the same one-name-two-families defect. Kept, for the reason above; trivially reverted if you want the diff to be literally the key.Pre-existing findings (not fixed here)
uv run ruff check src testsis red on the base commit withN818 Exception name UnknownCapability should be named with an Error suffix(src/zagg/registry.py:64). The PR lint bot's ruleset (--select=E,F,W,I) does not see it.uv run ruff format --check src testsflags a python fence intests/data/benchmark/README.md. Also pre-existing, also unseen by the bot's ruleset.Both were flagged the same way on PR #379, PR #416 and PR #421.