Repository navigation
Diagnose cross-batch home-cell shadowing on append builds - #12
Merged
Merged
Conversation
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The append conflict resolver compares exact hashes, which is all a cell collision needs. It cannot see the other shape a cross-batch disagreement can take: an appended place is centred in a cell an earlier batch already owns at a finer precision, while this batch answers for that point only through a coarser cell it fully covers. No hash collides, both rows are legitimate covers, and the runtime's longest-prefix walk hands the point to the earlier row - so centroid_inside does not help, because the two owners are never compared. The state is reachable: the new spec builds it from a horseshoe whose slot holds another town's centre, and the lookup does answer with the horseshoe. It does not occur in the data. Running the 7-country slice as seven separate --append invocations - us,mx,nl,de,at,ch,cz, which covers the US/Mexico, Alpine and Dutch/German borders - the counter reports 0 in every one of the six append batches, and a post-hoc sweep of the finished index finds 0 home-cell violations of this shape in either the batched or the single-batch build, cross-country or otherwise. There is a reason for that beyond luck. Reaching the state needs a place whose cover terminates at a coarse cell that fully contains it, which puts its centroid deep in the interior of that cell - structurally far from any foreign polygon, which is what would have to intrude to own a finer cell over the same point. Administrative boundaries overlap in slivers at the border, and those land in the edge cells, not the interior one. The fixture only reaches it by making two polygons overlap in a way admin data does not. So the reconciliation is not implemented; the detector is, so the claim stays falsifiable. If a world build ever reports a non-zero number, the repair is a few lines from the counter. Cost is a bounded query set - one candidate per place per precision below the cell the batch answers with, not one per row - and the slice build time is unchanged.
Codex is right, and the counter this fixes is the instrument the previous round used to decline the cross-batch reconciliation, so that zero was not trustworthy. The scan took its depth from the deepest row the current batch emits. The condition it exists to detect is precisely that the batch answers for a point through a *coarse* cell while a finer row already exists - so the bound was smallest exactly where it needed to be largest, and a batch built with a lower --max-precision than the database would never look deep enough either. Depth now comes from MAX(LENGTH(geohash)) over the stored lookup table, widened by the batch's own deepest row. The spec was worse than the bug. It asserted the counter reported 1 on a Bigtown whose polygon spilled past the precision-4 cell, so the batch also emitted precision-5 rows elsewhere and the bound stayed large: green without ever exercising the thing that was broken. The fixture is now the cell exactly, the appended batch emits ONE row, and the spec asserts that row set before asserting the count - so a bound that cannot look deeper fails it. Against the old scan it does fail. Re-measured on the same seven-country slice, one country per --append invocation: still 0 in every one of the six append batches, and the finished database is byte-identical. The independent post-hoc sweep of the index - which walks the finished rows and is not subject to this bound at all - agrees: 0 nested shadowing, 0 cross-country, 0 won by a place with an invalid centroid. The conclusion stands; it now rests on an instrument proven able to fail. Build time is unchanged (one extra aggregate per batch).
Three review findings on the diagnostic, all of which would have let it report a number that no lookup could produce. A counter used to justify not building something has to be at least as trustworthy as the thing it replaces, so all three are fixed rather than argued down. Scan paths came from the rows that survived exact-hash conflict resolution. A place that emits the coarse cell over its centre and loses it to a co-centred, more populous incumbent kept no row of its own, so it was skipped entirely - even though a finer stored row whose owner is not centred there is exactly the shadow being looked for, and the in-batch reconciliation would hand that finer cell to the challenger. Every place with a valid home cell is now scanned, independent of what it won. Each stored row on a centroid path was judged on its own. Longest-prefix lookup only ever returns the deepest one, so a shallower, worse-ranked owner could report a shadow the runtime cannot produce, and several nested rows inflated one shadowed centre into several. Only the deepest owner on the path is judged now, and the count is per place. Descendant hashes were sliced out of the cached home geohash, which stops at HOME_CELL_PRECISION (12). Past that the slice returns the same 12-character hash repeatedly instead of real descendants, so a database built deeper than the cache could never be scanned below 12. Hashes past the cached length are encoded from the centroid, as ownsHomeCell() already does. Inert at the precisions used here - the world build runs at 5 - but the cap is a builder option, so the bug is real wherever it is raised. Red-green for the two behavioural findings: `reports a shadowed centre even when the batch loses the cell it emitted` (Bigtown owns no row at all after the append, asserted before the count) and `judges only the deepest stored row on a centroid path` (a weak precision-4 owner the challenger outranks under a precision-5 owner that outranks it - the correct answer is zero). Each fails against the behaviour it replaces.
sebschlo
force-pushed
the
fix/shadowed-home-cell-detector
branch
from
August 20, 2026 20:11
848e0f5 to
0e03801
Compare
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e03801fad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every chain started at precision 1, so the deepest-owner search could land on a cell at or above the level a place's cover terminates at - cells it never claimed. reconcileNestedHomeCells() only propagates a claim *below* the cover cell holding the centroid, and the diagnostic has to respect the same bound or it reports shadows the builder would never have contested. The concrete shape, and now the spec: the same-country dominant-city rollup suppresses a minor locality's own row, the promoted parent cell then defers to a more populous existing owner at that exact hash, and the suppressed locality is centred somewhere inside that parent while outranking its owner on the home-cell rule. Scanning from the top of the chain reports a shadow for a cell that was never in contention; the correct answer is zero. Chains now start one level below the cover cell that holds the centroid, and a place whose cover already terminates at the deepest stored precision is skipped outright. This one could only ever inflate the count, never suppress a detection - unlike the earlier `deepest` bound, which was blind in the direction that mattered and put a measured zero in doubt. The measurement stands; what was at stake here is whether a non-zero number could be trusted, which is the only use this counter has. Re-measured on the seven-country slice, one country per --append: still 0 in every one of the six append batches, artifact byte-identical.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e51456db9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit stopped the scan from reaching cells coarser than a
place's own cover, which was right, but it started the chain one level
too deep and bought a false negative for a false positive.
Dominant-city promotion assigns a place to an ancestor cell, and
buildCompactLookupRows then drops that place's exact home-cover row as
redundant with the ancestor. The row is gone before the exact-hash
resolver runs, so an earlier batch owning that same hash is never
compared against it and stays the runtime's deepest match. Starting at
homeCoverPrecision + 1 looked straight past it. A fixture confirms it:
Metro's home-cover row s000d is discarded, an earlier batch owns s000d,
the lookup at Metro's own centre answers with that earlier place, and the
counter reported 0.
Chains now run from the home-cover cell downwards, inclusive. Both bounds
are load-bearing and they pull opposite ways, so each has a spec that
fails when the bound moves its way:
- `reports a home-cover row that compaction discarded` fails at
homeCoverPrecision + 1.
- `does not report a cell the place never claimed` fails at precision 1.
Judging the home-cover cell is safe when the row was retained: either the
batch won it and the scan stops at a batch-owned cell, or it lost it to
the incumbent on merit and the same comparator declines here too.
The false-positive fixture also turned out to be contaminated - Metro was
itself genuinely shadowed in it, so the spec had been passing for the
wrong reason, blind to a real detection rather than avoiding a spurious
one. Metro is now centred clear of the earlier place, and the spec
asserts that no second place is being shadowed before asserting the count.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #11 so that PR stays single-purpose: it carries the
centroid_insidefix, which changes real output, and nothing here is required by it. #11 has merged (fb00431) and this branch is now rebased ontomasterwithgit rebase --onto; base retargeted. The detector files are byte-identical to their pre-rebase state and none of #11's files appear in this diff. 223 specs, 0 failures.What this is
A build-time counter, not a behaviour change. Every database it produces is byte-identical.
The append conflict resolver compares exact hashes, which is all a cell collision needs. It cannot see the other shape a cross-batch disagreement can take: an appended place is centred in a cell an earlier batch already owns at a finer precision, while this batch answers for that point only through a coarser cell it fully covers. No hash collides, both rows are legitimate covers, and the runtime's longest-prefix walk hands the point to the earlier row.
The state is reachable — a spec builds it from a horseshoe whose slot holds another town's centre, and the lookup does answer with the horseshoe. It does not occur in the data: on the seven-country slice built as seven separate
--appendinvocations (us,mx,nl,de,at,ch,cz — the US/Mexico, Alpine and Dutch/German borders), the counter reports 0 in every one of the six append batches.So the reconciliation is not implemented, and this makes that decision falsifiable instead of asserted. If a world build ever prints a non-zero number, the repair is a few lines from the counter.
Why it is written the way it is
A counter used to justify not building something has to be at least as trustworthy as the thing it replaces. Three review findings on the first cut would each have let it report a number no lookup could produce, and all three are fixed here:
--max-precisionthan the database never looked deep enough either).HOME_CELL_PRECISIONare encoded from the centroid rather than sliced out of the cached 12-character home geohash, which would otherwise repeat the same hash. Inert at the precisions used here (the world build runs at 5), but the cap is a builder option.Validation
Red-green for each behavioural claim, every one failing against the behaviour it replaces:
reports a home cell shadowed by a finer row from an earlier batch— the appended batch emits one row and nothing deeper, asserted before the count, so a scan bounded by the batch's depth cannot pass it.reports a shadowed centre even when the batch loses the cell it emitted— Bigtown owns no row after the append, asserted before the count.judges only the deepest stored row on a centroid path— a weak precision-4 owner the challenger outranks, under a precision-5 owner that outranks it; the correct answer is zero.Full suite green: 223 specs, 0 failures. Slice build time unchanged; resulting database byte-identical.
Addresses #11 (comment), #11 (comment) and #11 (comment).