fix(simulation): synthetic-overflow placements get the ocean-mask guard (T-1206) #218
Assignees
jpmschweitzer (Jeroen Schweitzer)
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: jpmschweitzer/settled-reach#218
Reference in New Issue
Block a user
Summary
Batch-9 server track — closing the placement gap PR #215's review exposed. The synthetic-overflow path (D-211 Phase 4) derived positions by pure grid arithmetic with no terrain input at all, and the gap was far more widespread than assumed: 46 of 109 synthetic placements across the 267 real bodies sat in open water at seed 42 (e.g. GJ903c at a genuine polar ocean cell).
Fix:
synthetic_attractornow takes the terrain analysis and land-corrects via a bounded nearest-land ring walk (T-1116's proven pattern: row-major tie-break, column wrap, row clamp; MAX_LAND_SEARCH_RING=128 sized empirically — real polar ocean bands push nearest land up to 125 cells). Land positions pass through untouched. Degradation defined and pinned: no land within the bound → the synthetic attractor is skipped and the existing not-placed warning reports it — never a panic, never a fabricated water position.Compatibility (the load-bearing requirement): direct before/after scan of all 267 real bodies — all 63 land-arithmetic placements byte-identical; post-fix 0 in water, all 109 synthetic placements preserved, confirmed at a second seed. Every golden/determinism harness passes unchanged — no regeneration.
Records: road_graph.rs anchor comment + the D-210 amendment updated to mark the gap CLOSED (validated).
Test plan
Review: ocean-guard-synthetic -> main (type: code)
Reviewers: Hoshe (QA) + Tyre (architecture/governance). Both REQUEST_CHANGES — three small, surgical findings on merge-quality engineering.
Hoshe (QA): REQUEST_CHANGES
cascade_for_body(42, "GJ251c")real-body tests) is directly reusable: add a real-body test asserting every synthetic placement on GJ903c (or whichever body exercises overflow) lands off ocean_mask. Without it, a guard regression is only caught by synthetic fixtures, not the production data that motivated the fix.Hoshe's confirmations: land-passthrough byte-identical BY CONSTRUCTION (the is-land short-circuit precedes all correction math); Option<&TerrainAnalysis> provably always Some in production (Settlement > Topography in CascadeLayer's Ord, pinned by layers_are_ordered); wrap conventions consistent; tree carries zero scratch remnants.
Tyre (architecture/governance): REQUEST_CHANGES
Tyre's rulings on record: duplicate-over-share is CORRECT for the ring walk (different resolutions, predicates, return shapes, index spaces — a generic helper would couple two files across a layer boundary to save ~25 commented lines; stated so it isn't re-litigated; note the two implementations wrap at different points, both correct, not line-mirrors); determinism clean (seed-invariant guard over seed-derived input — the cross-seed claim holds structurally); scope containment exact; the D-210 addendum + road_graph comment citation-faithful and validated.
Verdict: CHANGES REQUESTED
All three to the authoring agent: real-body regression test, D-211 amendment, constant derivation + recalibration note. Fix round → gate → merge (no eyeball owed — data-layer; markers leave the ocean visibly when T-1207 lands).
Review round addressed — all three findings, plus one they uncovered
Commit
4b75be597. Fullcargo testgreen (30 binaries).Hoshe: real-body regression test — fixed
Two tests now hold the claim on production data:
t1206_verification_scan— the T-1206 verification scan itself, committed as an#[ignore]d full-corpus test (267 bodies,cargo test --lib t1206_verification_scan -- --ignored --nocapture). Your complaint was that the scan was throwaway, so the headline number had nothing holding it; this makes it re-runnable. It doubles as the executable form of the recalibration instruction now written on the search bound.real_body_synthetic_placements_never_land_in_ocean— the fast gate, pinning six bodies the scan identifies as actually reaching Phase-4 overflow, asserting no synthetic placement sits on an ocean cell.Corrected corpus figures at seed 42: 267 bodies, all reaching Layer 3, 344 placements, 109 synthetic, 0 in water. The synthetic count matches what the original scan reported, so the PR's evidence is reproducible rather than anecdotal.
The finding behind the finding
Asking for this test surfaced a real bug, and it is worth recording because it is the same defect class you were objecting to.
CascadeSnapshot::terrain_analysisis transient — the cascade sets it toNoneas soon as DistrictProfile and RoadGraph have consumed it (D-203/T-1048, ~2 MB a body). So it is alwaysNoneon a returned snapshot, regardless of whatcascade_snapshot_for_body's doc-comment says about "still carrying" it. My first draft read the ocean mask off the snapshot and skipped bodies where it was absent — which skipped every body while reporting success:A green
assert_eq!(in_water, 0)over an empty set. I was one step from reporting that the PR's evidence could not be reproduced on current data.Two things caught it, both added because of your finding: a vacuity guard (
synthetic_seen > 0) that refuses to pass over an empty set, and counters that separate "no synthetic placements" from "never got that far" — those two states were rendering identically. Terrain is now re-derived through the samerun_layer1_with_moisturecall the cascade used, so the grid matches the one the placements were computed against. The trap is documented on the helper so it isn't rediscovered.Also worth noting for the record: the first body list I picked paired GJ903c with GJ251c/GJ380c/GJ820Bc/GJ338Bd — and those four have no synthetic placements at all, so the assertion would have rested on a single body. The pinned list is now taken from the scan's own output rather than guessed.
Tyre 1:
MAX_LAND_SEARCH_RINGderivation — fixedconst MAX_LAND_SEARCH_RING: u16 = 128→fn max_land_search_ring(ta: &TerrainAnalysis) -> u16 { ta.h / 2 }.The whole justification was expressed in grid heights while the value was a literal, so the 512×256 coupling was load-bearing and implicit. On the current working grid (
GRID_W × GRID_H = 512 × 256) this evaluates to exactly the 128 it replaces — no behaviour change, and the byte-identical-placement guarantee is untouched. Recalibration owner recorded: whoever changes the body corpus or grid resolution re-runs the scan and confirms the observed maximum still sits underh / 2.One consequence, flagged rather than buried.
nearest_land_cell_clamps_rows_no_wrapuses a 16×16 fixture, so its bound drops from 128 to 8 — which now sits between the clamped distance to the far pole (15) and the wrapped one (1). The assertion moves from position to absence, and gets stronger: previously both a clamping and a wrapping implementation returnedSome((15,0)), so only the position could be pinned. Now anySomeat all proves rows wrapped.Tyre 2: D-211 amendment — fixed
Dated amendment (2026-08-06) on D-211. It records that step 4's outcome set is no longer total — synthetic overflow may now resolve to a defined SKIP where no land exists within the bound — and that step 5's name-fulfillment warning consequently fires for a new legitimate reason (a genuinely water-locked body), not only for a pipeline failure. It states explicitly that no re-decision is required, since position remains a pure function of seed + terrain: this fulfils the placement intent rather than deviating from it.
The cross-reference to D-210 is now a live anchor pointing at its 2026-07-26 amendment — the matched-attractor half of this same placement-in-water gap — instead of a dead end.
pql decisions validate→{"ok":true}.Ready for re-review.
Round 2 addressed — 6 fixed, 1 retracted
Commit
fc55bd897.Both reviewers returned REQUEST_CHANGES. Findings below in the order they matter, not the order received.
The headline: both of you found the same defect, independently
Tyre's #1 and Hoshe's #1 are the same gap, reached without seeing each other's output. That convergence is the strongest signal of this round, and it was correct.
The spacing walk in
synthetic_attractorpicks a candidate that satisfiesMIN_SPACINGagainst every already-placed city — and then the ocean correction moves that candidate by up toh/2= 128 cells with no re-validation. D-211 step 4 promises the synthetic attractor is placed "at a position that respects minimum city spacing"; the fix was buying the land half of that promise with the spacing half. I confirmed the sequencing in the code before accepting it (attractor_matching.rs: spacing.all()at the walk, correction immediately after), and confirmed Tyre's point thatroad_graph::collapse_colocatedis no safety net — it dedupes by name, not position.Fix: the spacing predicate is now threaded into the ring walk (
nearest_cell_matching), so the search returns the nearest cell satisfying land and spacing, with identical tie-break order and identical degradation-to-skip. Not a second walk — a predicate on the existing one.Honest limit: whether this was ever a live violation or only a latent one is unmeasured, and I'd rather say so than imply I caught it in the act — I replaced the behaviour before testing the old path. The corpus scan can't settle it either, because by the time it ran the predicate was already in, which guarantees the result by construction.
My first fix over-asserted, and the corpus caught it
Worth recording because it nearly shipped. I initially asserted
MIN_SPACINGacross all placement pairs. The 267-body scan returned 13 violations — none of them the guard's doing.D-211's spacing promise is step 4's alone. Matched placements (Tier A greedy, Tier B/C Hungarian) sit on their terrain attractor and were never subject to it; two real river mouths 12 cells apart is legitimate geography. Had that assertion shipped it would have failed the push gate and blamed the ocean guard for pre-existing placements.
I checked rather than assumed: of the 12 distinct bodies carrying those 13 pairs, not one has a synthetic placement at all — every violation is matched-vs-matched. Both checks are now scoped to pairs involving a synthetic placement, with the 13 documented in-code so this isn't re-litigated.
Corpus evidence, re-run with both invariants
Remaining findings
t1206_verification_scanlacks the vacuity guards its sibling hasbodies_loaded > 0/synthetic_seen > 0to the fast test and left the scan — the one test whose entire purpose is being the re-runnable evidence — able to go green over an empty set, which is exactly how its first draft failed. Both guards added;!bodies.is_empty()only ever proved the directory listing worked.cascade_snapshot_for_bodydoc-comment is falseNonefor a full-cascade call. Corrected in place, with the re-derivation recipe and a note that this sentence cost a false-clean 267-body scan.(0,0)is not returned; the fixture ista_all_landand the test asserts(0,0)is returned. Rewritten to describe what actually happens and why(0,0)is the production-relevant case.Σ 8rover1..=128= 66,048, not(h/2)² = 16,384. The old figure was one quadrant's area, not cumulative ring cost. Conclusion ("negligible") unchanged, but this paragraph is the executable recalibration rationale, so the arithmetic has to be right.synthetic_seen > 0needs only one, but resting on a single body is precisely how the first draft ended up depending on GJ903c alone. Three keeps independent terrain coverage at a third less cost; the 267-body sweep stays behind--ignored.Retracted: Tyre #4 (shared ring-search helper)
Retracting this one, per the skill's pushback process, on the grounds that the reviewer was wrong about it — specifically, wrong against their own prior ruling.
Round 1, Tyre, on this exact code:
Round 2 asks to factor it into a shared helper. Neither implementation has changed since that ruling, and no new evidence is offered — so this is the re-litigation the first round explicitly foreclosed. The cross-layer coupling objection still stands.
Worth noting the round-2 fix moves in the direction #4 wanted anyway: fixing the spacing gap by adding a predicate to the existing walk rather than writing a second one means this PR ends with fewer bespoke ring walks than a naive fix would have produced.
Full
cargo testgreen. Ready for re-review.Round 3 addressed — 6 findings, all doc-accuracy, one root cause
Commit
d5e617eff.Both reviewers returned REQUEST_CHANGES, and between them raised the same three issues twice — from opposite ends. Tyre found the comments were stale; Hoshe found the fixes for them weren't committed yet (I was editing while the review ran). Same three hunks either way.
The root cause is worth stating plainly
The round-2 fix outran its own documentation. Folding the spacing predicate into the ring walk changed what three comments describe — and two of those comments were written by this PR, one round earlier. That is the third consecutive round where this PR has caught its own documentation drifting behind its own code.
road_graph.rscitesnearest_land_cell, which round 2 made#[cfg(test)]nearest_cell_matching; the "guarantees the placement pixel is land" claim corrected to land-and-spacing-or-skipmax_land_search_ringdoc names the test-only wrapper as the thing that walks the boundHoshe's framing is the one that matters: had this merged as it stood, the governance record checked in specifically to be the source of truth would have misdescribed the shipped code in the same commit.
Independent verification — the part I could not have supplied myself
road_graph.rs::nearest_passable_cell's independent implementation exactly; the predicate is consulted only on already-land candidates, so neither of D-211's promises is double-spent.Option<&TerrainAnalysis>-always-Someclaim structurally rather than by inspection:CascadeLayerderivesOrdfrom declaration order withTopographybeforeSettlement, so theup_to >= CascadeLayer::Settlementguard provably implies the Topography block that populatesterrain_analysisalready ran.The retraction is settled
Tyre confirmed the shared-ring-search-helper retraction with new grounds rather than a restatement — round 2 actually strengthened the case for keeping them apart: this walk is now parameterised by an arbitrary predicate over native
u16terrain coordinates, whileroad_graph's is aRouteGridmethod over downsampledusizerouting cells with a fixed cost test and an unrelated bound (COASTAL_ANCHOR_MAX_RING = 3vs derivedh/2). Unifying them would mean generic-ising over index type and grid accessor for ~40 lines of different-domain logic.That closes it properly — resolved on evidence, not on my assertion against theirs.
Also confirmed, no action needed
MIN_SPACINGappears exactly once in D-211 and is scoped to step 4's own placement; Tier A/B/C matched placements sit on their terrain attractor and were never subject to it. The scoping correctly includes synthetic-vs-matched and synthetic-vs-synthetic while excluding matched-vs-matched — consistent with whatsynthetic_attractor's own walk checks against.nearest_land_cellas#[cfg(test)]— sound factoring; zero-risk delegation that cannot drift from the production path.believability.rs's corrected doc-comment is accurate — verified against the exact conditional incascade.rs.22 module tests green, clippy and fmt clean. Ready for re-review.
Pull request closed