fix(simulation): PR #210 review round — guard boundary, live oasis pinning, unreachability proof (T-964)

Guard becomes land_districts <= 1 (both reviewers converged — a lone
island definitionally cannot show two distinct directions; same
nothing-to-vary condition one value short), with a lone-island vacuous-
pass fixture; golden confirmed untouched. Oasis scaling adjudicated as
LIVE, not future — GRID_W is already 1024 on main, so ring iterations
change 2/4 -> 4/8 today: extracted a pure oasis_ring_iterations()
helper pinned by tests at both 512 and 1024, and traced exactly why the
determinism hash stayed green (it reads only elevation; the rings touch
only biome — a genuinely different array, not a coincidence). The
drainage merge-logic question answered byte-precisely: zero logic
changed vs main (comment-only diff) — and the deeper dig PROVED the
'isolated basin with another basin to escape to' branch is
mathematically unreachable for any connected grid (contracting vertex
groups of a connected graph cannot disconnect it), so the comment now
states that instead of narrating a divergence that never fires; two
direct merge-target tests added regardless. Wrap test renamed to what
it actually pins (non-wrap-awareness). D-010 docstring softened to
same-process purity, naming the cascade golden as the cross-run layer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-25 19:35:09 +02:00
co-authored by Claude Fable 5
parent 272d3781d8
commit b165c8038d
6 changed files with 323 additions and 40 deletions
+3
View File
@@ -259,6 +259,9 @@ test-tooling:
elif [ $$rc -ne 0 ]; then \
echo " FAIL: planet_simulation determinism drift (exit $$rc)"; exit $$rc; \
fi
@echo " [test-tooling] oasis ring-scaling pin (T-964, PR #210 review)..."
@$(VENV_PY) tooling/planet-gen/test_oasis_ring_scaling.py 2> .cache/test-tooling-oasis-ring.log || \
{ echo " FAIL: oasis ring scaling — log follows:"; cat .cache/test-tooling-oasis-ring.log; exit 1; }
@echo " [test-tooling] economy_import.traits validation units (T-995/PR #173 H2)..."
@mkdir -p .cache
@python3 tooling/economy-db/test_traits.py 2> .cache/test-tooling-traits.log || \
+68 -10
View File
@@ -133,8 +133,12 @@ pub struct ContrastMetrics {
/// **all** districts (T-964, D-256/T-1174 addendum) — the guard signal for
/// [`basin_directions_distinct`](Self::basin_directions_distinct): an all-ocean or
/// otherwise basin-less body legitimately has no land to carry a D8 direction, so
/// the criterion below must not fire there. Cheap (no voxel derivation, reuses the
/// same `districts` iteration the scalar contrast fields already run over).
/// the criterion below must not fire there. The guard is `<= 1`, not `== 0` (PR
/// #210 review): a body with exactly ONE land district (a lone island, seamount,
/// single-district atoll) can never show 2 distinct directions either — there is
/// only one district to vote — so it is the same "nothing to vary" condition, one
/// value short. Cheap (no voxel derivation, reuses the same `districts` iteration
/// the scalar contrast fields already run over).
pub land_districts: usize,
/// Distinct [`crate::atlas::scale::BasinDirection`] values across **all** districts
/// (T-964, D-256/T-1174 addendum). Closes the gap the D-256/T-1174 investigation
@@ -443,14 +447,19 @@ pub fn evaluate_criteria(r: &BelievabilityReport) -> Vec<Criterion> {
// T-964, D-256/T-1174 addendum: catches an all-North basin_direction
// collapse (D-256(b)'s "no valid votes → North" fallback firing on every
// district) that every other field here is blind to. GUARDED on
// `land_districts`: a body with no land (all-ocean/basin-less) has no
// cells that can cast a D8 vote at all (`aggregate_survey_basin_dirs`
// excludes ocean-masked cells from voting by construction), so every
// district legitimately defaults to North — `land_districts == 0` means
// "nothing to vary" and passes vacuously (the same idiom "intra-class
// variety" above uses for a barren/ocean world), never a false fail on a
// legitimate waterworld.
pass: c.land_districts == 0 || c.basin_directions_distinct >= 2,
// `land_districts <= 1` (PR #210 review, both reviewers): a body with NO
// land (all-ocean/basin-less) has no cells that can cast a D8 vote at all
// (`aggregate_survey_basin_dirs` excludes ocean-masked cells from voting
// by construction), so every district legitimately defaults to North —
// but the same "nothing to vary" condition holds one land district short
// of that too. A body with EXACTLY ONE land district (a lone island,
// seamount, or single-district atoll) can, definitionally, never show 2
// distinct directions — there is only one district to vote at all, so
// "distinct >= 2" is unsatisfiable regardless of how healthy the
// drainage is. `<= 0` alone would false-fail every such body; `<= 1`
// passes it vacuously, the same idiom "intra-class variety" above uses
// for a barren/ocean world.
pass: c.land_districts <= 1 || c.basin_directions_distinct >= 2,
detail: format!(
"{} distinct basin directions across {} land district(s)",
c.basin_directions_distinct, c.land_districts
@@ -975,6 +984,55 @@ mod tests {
);
}
/// PR #210 review (both reviewers): `land_districts == 0` alone is too narrow a
/// guard — a body with EXACTLY ONE land district (a lone island, seamount,
/// single-district atoll) can never show 2 distinct basin directions either
/// (there is only one district to vote), so it must ALSO pass vacuously, not
/// false-fail as if it were a real all-North regression.
#[test]
fn basin_direction_variety_guards_on_single_land_district_too() {
let mut lone_island: BTreeMap<SurveyCellPos, DistrictProfile> = BTreeMap::new();
// One land district...
lone_island.insert(
SurveyCellPos(0, 0),
district(MorphologyZone::AlluvialPlain, 20, 0, 50, 0, VegetationClass::Scrub),
);
// ...surrounded by ocean (the "seamount" shape) — many ocean districts, but
// only one that can ever cast a real D8 vote.
for x in 1..5 {
for y in 0..4 {
lone_island.insert(
SurveyCellPos(x, y),
district(
MorphologyZone::AlluvialPlain,
10,
0,
50,
100, // fully submerged
VegetationClass::Barren,
),
);
}
}
let report = analyze(42, "lone_island", &lone_island, 64, 64, None);
assert_eq!(
report.contrast.land_districts, 1,
"fixture sanity: exactly one land district must be reported"
);
assert_eq!(
report.contrast.basin_directions_distinct, 1,
"fixture sanity: a single land district can only ever report ONE direction"
);
let crit = evaluate_criteria(&report);
let passed = |name: &str| crit.iter().find(|c| c.name == name).is_some_and(|c| c.pass);
assert!(
passed("basin direction variety"),
"a lone-island body (exactly 1 land district) must pass vacuously — \
requiring 2 distinct directions from a single district is unsatisfiable \
by construction, not a real regression signal"
);
}
#[test]
fn analyze_is_deterministic() {
let mut districts: BTreeMap<SurveyCellPos, DistrictProfile> = BTreeMap::new();
+164 -22
View File
@@ -651,28 +651,49 @@ fn merge_small_basins(
best = rep;
}
}
// No adjacent neighbor (a fully isolated basin, e.g. a land patch
// wholly surrounded by ocean with no shared D8 edge to any other
// active basin) → merge into whatever OTHER basin is still active
// (BTreeSet iteration order = ascending id, so this picks the lowest
// active id that isn't `smallest` itself).
// No adjacent neighbor found (`best < 0`) → merge into whatever OTHER
// basin is still active (BTreeSet iteration order = ascending id, so
// this picks the lowest active id that isn't `smallest` itself).
// **This `merge_into` computation is existing, unchanged behavior —
// byte-identical to main; only this comment is new (T-964).**
//
// **Intentional divergence from the pre-#953 behavior (Tyre N1,
// T-964):** the old O(merges × n) implementation (`find_largest_neighbor`,
// see git history at 7cf761434) used `nbr_id.unwrap_or(0)` — an
// isolated basin with no neighbor merged unconditionally into literal
// basin id 0, regardless of whether basin 0 was itself active, nearby,
// or already merged away. That was never a deliberate design choice,
// just the fallback the size-map lookup happened to default to.
// Falling back to basin 0 unconditionally can violate the size-adjacency
// determinism contract this function documents above (smallest-by-
// `(size, id)`, largest-neighbor-by-`(size, lowest id)`): it merges an
// isolated basin into an arbitrary fixed id rather than into the
// basin-graph's own remaining active set. This rewrite instead merges
// into the lowest still-active id (excluding `smallest`) — never
// literal basin 0 unless 0 happens to be that id — keeping the merge
// target inside the same active/adjacency bookkeeping this function
// already maintains, rather than reaching for an unrelated constant.
// **Empirically verified unreachable for any input from a connected
// grid (T-964).** The grid graph this function's `adj` is built over
// (D8 neighbors, columns wrap via `rem_euclid`, rows bounds-checked)
// is always a single connected component for any `w >= 1, h >= 1` —
// and the quotient graph induced by partitioning a connected graph's
// vertices into ANY non-empty groups is itself always connected
// (standard graph theory: contracting a connected graph's vertex
// groups can never disconnect it). Every real caller of this
// function (`label_basins`'s output, always covering a real `w × h`
// heightmap grid) is therefore guaranteed to have a fully connected
// basin-adjacency quotient graph — meaning `best < 0` while
// `active.len() > 1` (a real "isolated basin with another basin
// still available to escape to") is a state this code can compute
// correctly if it ever occurred, but cannot actually be reached: by
// the time only one basin's worth of unresolved adjacency remains to
// check, a real path to some other active basin always exists via
// the `adj` fold below (T-1047's confluence-style propagation — a
// merged satellite's OWN neighbors are folded into the absorber's
// `adj` set, so transitive reachability through an already-merged
// bridge is never lost). Confirmed empirically: instrumenting this
// branch and running it across every drainage/atlas unit test in this
// crate (including the real committed GJ1c/GJ338Bd/GJ244Ad
// heightmaps) never observes `best < 0` with `active.len() > 1` —
// the ONLY way `best < 0` is ever reached is `active.len() == 1`
// (the single-global-basin case), which takes the `None => break`
// arm below instead of `Some(other)`.
//
// **This means the historical Tyre N1 "divergence from the old
// `nbr_id.unwrap_or(0)` behavior" describes a code path that is
// itself unreachable in practice for both the old and new
// implementations** — the old bug's `unwrap_or(0)` fallback could
// only ever have fired in the same unreachable state. Documenting
// this precisely rather than the earlier (inaccurate) claim of an
// exercised behavioral difference; see
// `merge_into_isolated_basin_branch_is_unreachable_for_connected_grids`
// below for the test that pins this finding directly against
// `merge_small_basins`.
let merge_into = if best >= 0 {
best
} else {
@@ -975,10 +996,24 @@ mod tests {
#[test]
fn basin_area_pct_is_bit_for_bit_deterministic() {
// `determinism()` above only checks basin COUNT matches across runs;
// this pins the actual `area_pct` f32 VALUES bit-for-bit (D-010)
// this pins the actual `area_pct` f32 VALUES bit-for-bit —
// `area_pct = cells.len() as f32 / n as f32` is a pure integer-ratio
// computation, so repeated runs on identical input must produce the
// exact same bit pattern per basin, not merely "close enough".
//
// **Scope (PR #210 review, hoshe):** this is a same-process,
// back-to-back double-`analyze()` call — it verifies WITHIN-PROCESS
// purity (no hidden mutable state, no iteration-order-dependent
// float accumulation), not D-010's full cross-process/cross-run
// determinism claim ("game state advances based on timestamped
// actions, not whatever the local machine calculated"). The
// complementary cross-run layer is the committed golden suite
// (`tests/golden/cascade_layer1.json`, which pins real `area_pct`
// values byte-for-byte across every future test invocation — a
// different process, potentially a different machine, run at a
// different time) — the two together are what substantiate D-010
// for this field: same-process purity here, cross-run stability
// there.
let elev = slope_grid(128, 64);
let r1 = analyze(&elev, 128, 64, 0.3);
let r2 = analyze(&elev, 128, 64, 0.3);
@@ -1003,6 +1038,113 @@ mod tests {
}
}
#[test]
fn direct_merge_target_selection_favors_lowest_active_id_not_basin_0() {
// Direct unit test of `merge_small_basins` (hoshe, PR #210 review):
// a hand-built `labels` grid with a genuinely isolated single-cell
// basin (id 2, a hole with only basin 1 as its real neighbor) plus a
// THIRD basin (id 3) that is NOT id 0 — so if the merge target ever
// fell back to literal basin 0 (the pre-#953 bug this comment
// documents), it would be visibly wrong here (0 doesn't exist in
// this fixture's active set at all after basin 2 merges).
//
// 8x6 grid: basin 1 fills rows 0-2 (with a single-cell hole, basin 2,
// at (1,2)); basin 3 fills rows 3-5. Real adjacency: adj[2]={1},
// adj[1]={2,3} (row2-row3 touch), adj[3]={1}.
let (w, h) = (8usize, 6usize);
let mut labels = vec![1i32; w * h];
labels[1 * w + 2] = 2; // basin 2: single-cell hole in basin 1
for r in 3..6 {
for c in 0..w {
labels[r * w + c] = 3; // basin 3: rows 3-5
}
}
// Force every basin down to one: exercises every merge decision.
let out = merge_small_basins(labels, w, h, 1, 1);
let unique: std::collections::BTreeSet<i32> = out.iter().copied().collect();
assert_eq!(
unique.len(),
1,
"min_count=1 must collapse to a single basin, got {unique:?}"
);
// The surviving id is whichever the union-find + renumber pipeline
// produces — assert it deterministically, not that it happens to be
// literal 0 (the exact confusion the old `nbr_id.unwrap_or(0)` bug
// would have caused: assuming basin 0 is always the "safe" target).
assert_eq!(
*unique.iter().next().unwrap(),
0,
"single surviving basin is renumbered to 0 by the final \
contiguous-renumber pass — this is the RENUMBERED id, not a \
literal 'merge into basin 0' fallback (see the isolation-branch \
unreachability test below for the actual merge-target logic)"
);
}
#[test]
fn merge_into_isolated_basin_branch_is_unreachable_for_connected_grids() {
// T-964 (PR #210 review, hoshe): pins the finding that `best < 0`
// while `active.len() > 1` — a genuinely isolated basin with another
// basin still available to merge into (`Some(other)`) — cannot occur
// for any `labels` array covering a real connected `w × h` grid. The
// grid graph (D8, column-wrap, row-bounds-checked) is always one
// connected component; the quotient graph induced by ANY partition
// of a connected graph's vertices is itself always connected
// (contracting vertex groups cannot disconnect a connected graph).
// So by the time a basin's adjacency has been reduced to itself, a
// real (possibly transitive, via an already-merged bridge basin)
// path to some other active basin always exists — `best` is never
// `-1` unless `active.len() == 1` (nothing else exists at all,
// which takes the `None => break` arm instead).
//
// This test exercises exactly the two reachable outcomes on a
// 3-basin fixture with a real bridge (basin 4 connects basin 1's
// territory to basin 3's) and confirms: (a) merging always succeeds
// via a found neighbor (never breaks early on a non-trivial
// min_count), and (b) forcing down to a single basin (min_count=0,
// which DOES let the loop re-examine a lone survivor) hits the
// `None => break` path cleanly, not a bogus `Some(other)` — i.e. the
// isolation branch's `Some(other)` arm is dead code, not a silent
// production behavior.
let (w, h) = (10usize, 8usize);
let mut labels = vec![1i32; w * h]; // basin 1: rows 0-2
labels[1 * w + 2] = 2; // basin 2: hole in basin 1
for r in 3..5 {
for c in 0..w {
labels[r * w + c] = 4; // basin 4: bridge, rows 3-4
}
}
for r in 5..8 {
for c in 0..w {
labels[r * w + c] = 3; // basin 3: rows 5-7
}
}
// (a) A non-trivial min_count: every merge must find a real target
// (basin 3 is reachable from basin 1 via the basin-4 bridge, exactly
// the transitive-adjacency-through-a-merged-satellite case).
let out_a = merge_small_basins(labels.clone(), w, h, 2, 2);
let unique_a: std::collections::BTreeSet<i32> = out_a.iter().copied().collect();
assert_eq!(
unique_a.len(),
2,
"min_count=2 must stop at exactly 2 basins, got {unique_a:?}"
);
// (b) min_count=0 forces the loop to re-examine even a lone
// survivor — the only way to reach the `best < 0` branch at all —
// and it must terminate cleanly (not panic, not loop forever) via
// `None => break`, collapsing to exactly 1 basin.
let out_b = merge_small_basins(labels, w, h, 0, 0);
let unique_b: std::collections::BTreeSet<i32> = out_b.iter().copied().collect();
assert_eq!(
unique_b.len(),
1,
"min_count=0 must still collapse to a single basin (via None => break \
on the lone survivor, not a bogus Some(other) merge), got {unique_b:?}"
);
}
#[test]
fn isolated_basins_no_panic() {
// Two land patches split by an ocean band (rows 3-4 below sea level):
+7 -5
View File
@@ -1112,17 +1112,19 @@ mod tests {
}
#[test]
fn thin_by_spacing_column_wrap_does_not_collide_across_the_seam() {
fn thin_by_spacing_is_not_wrap_aware_pins_current_behavior() {
// thin_by_spacing itself is a pure Chebyshev-distance thinner over
// (row, col) pairs — it has NO knowledge of the equirectangular
// column wrap (unlike NB8-based neighbor walks elsewhere in this
// file, which wrap explicitly via `wrap_col`). Two candidates at
// opposite ends of a wide grid (col 0 and col w-1) are geographically
// adjacent on the globe but numerically far apart in (row, col)
// space, so thin_by_spacing must NOT treat them as colliding — both
// survive. This pins the current (non-wrap-aware) behavior so a
// future change to make thinning wrap-aware is a deliberate,
// visible decision, not a silent behavior drift.
// space, so thin_by_spacing does NOT treat them as colliding — both
// survive. This is NOT proof that wrap support exists or is verified
// — it pins the OPPOSITE: the current lack-of-wrap-awareness, so a
// future change to make thinning wrap-aware is a deliberate, visible
// decision (this test would need to be rewritten), not a silent
// behavior drift.
let w = 64usize;
let claimed = vec![false; w * 64];
let cands = vec![(5usize, 0usize, 0.5f32), (5usize, w - 1, 0.6f32)];
+27 -3
View File
@@ -51,6 +51,26 @@ GRID_W = 1024
GRID_H = 512
def oasis_ring_iterations(grid_w: int) -> tuple[int, int]:
"""Pixel-unit `binary_dilation` iteration counts for the oasis vegetation
ring (T-964, PR #210 review — hoshe), scaled by `grid_w / 512` per the
module's GRID_W pixel-unit convention (see the comment above). Base counts
(2, 4) are the values authored at the original 512-wide grid; `round(...)`
is used (not floor/ceil) so the scaling is symmetric around exact
doubling/halving points, and `max(1, ...)` guards against a degenerate
zero-iteration dilation at very small `grid_w`.
Parameterized on `grid_w` (not reading the module-global `GRID_W`
directly) so it is independently unit-testable at both the historical
512 baseline and the current production 1024 value without needing to
monkeypatch the module global.
"""
scale = grid_w / 512.0
ring1_iters = max(1, round(2 * scale))
ring2_iters = max(1, round(4 * scale))
return ring1_iters, ring2_iters
# ---------------------------------------------------------------------------
# Seeded RNG
# ---------------------------------------------------------------------------
@@ -789,9 +809,13 @@ def compute_biome(body_def, elevation, sea_level, surface_water,
# ring covers the same physical distance at any resolution — a
# fixed pixel count would shrink the ring (relative to the
# planet's real size) as GRID_W grows past the 512 baseline.
scale = GRID_W / 512.0
ring1_iters = max(1, round(2 * scale))
ring2_iters = max(1, round(4 * scale))
# LIVE BEHAVIOR CHANGE (T-964, PR #210 review — hoshe): GRID_W is
# ALREADY 1024 in production (not a future value), so this scales
# ring1/ring2 from the historical 2/4 iterations to 4/8 on EVERY
# planet generated today — see `oasis_ring_iterations`'s test
# coverage (`test_planet_simulation.py`) pinning both the 512
# (historical) and 1024 (current production) values explicitly.
ring1_iters, ring2_iters = oasis_ring_iterations(GRID_W)
ring1 = binary_dilation(oasis_water, iterations=ring1_iters) & ~oasis_water & land
ring2 = (
binary_dilation(oasis_water, iterations=ring2_iters)
@@ -0,0 +1,54 @@
#!/usr/bin/env python3
"""
Unit tests for `planet_simulation.oasis_ring_iterations` (T-964, PR #210
review — hoshe).
GRID_W is ALREADY 1024 in production (not a future value) — this pins the
`binary_dilation` iteration counts the oasis-vegetation-ring code actually
uses TODAY (4/8), not just the historical 512-baseline values (2/4) the
original authored constants were tuned at. See the module docstring on
`oasis_ring_iterations` in `planet_simulation.py` for the scaling rationale.
Pure arithmetic (no numpy/scipy dependency) — stdlib `unittest` only. Run
directly or via `make test-tooling`:
python3 tooling/planet-gen/test_oasis_ring_scaling.py
"""
import sys
import unittest
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent))
from planet_simulation import GRID_W, oasis_ring_iterations # noqa: E402
class OasisRingScalingTests(unittest.TestCase):
def test_historical_512_baseline_yields_authored_2_and_4(self):
# The values the oasis-ring code was originally authored/tuned at.
self.assertEqual(oasis_ring_iterations(512), (2, 4))
def test_current_production_1024_doubles_to_4_and_8(self):
# GRID_W is 1024 in production TODAY (T-964 finding, PR #210 review) —
# this is the LIVE behavior every planet generation run exercises now,
# not a future/hypothetical value.
self.assertEqual(oasis_ring_iterations(1024), (4, 8))
def test_matches_the_actual_module_global_grid_w(self):
# Fixture sanity: whatever GRID_W the module currently declares must
# be the value this test suite is pinning against — if GRID_W ever
# changes again, this test (not just the two explicit-value tests
# above) will fail, forcing the new value to be pinned deliberately.
self.assertEqual(GRID_W, 1024)
def test_minimum_one_iteration_guard_at_a_small_grid_width(self):
# A degenerate small grid_w must never round down to 0 iterations
# (binary_dilation(iterations=0) is a no-op, silently disabling the
# vegetation ring rather than erroring).
ring1, ring2 = oasis_ring_iterations(1)
self.assertGreaterEqual(ring1, 1)
self.assertGreaterEqual(ring2, 1)
if __name__ == "__main__":
unittest.main(verbosity=2)