fix(client): PR #192 review round — per-rung legend, Region coalescing + clamp boundary tests, halving-loop honesty
All Hoshe/Araminta findings addressed (Tyre approved outright), none retracted — plus a Dudley stop-and-flag discovery that improved on the asked-for fix: - Legend 100x lie (Araminta, blocking): subtitle computed via spacing_for_rung() and refreshed at all three _held_granularity_v2 write sites. Region test asserts 204.800 km/cell; the District direction needed a stale-header-aware helper — a District-only test spuriously passes against the old literal by coincidence. - Region coalescing coverage (Hoshe 1): both directions tested (Region-vs-District separate slots; Region-vs-Region coalesces). - Clamp boundary tests + dangling citations (Hoshe 2): writing the requested halving-loop-fires test surfaced that the loop is PROVABLY UNREACHABLE at current constants (per-axis clamp forecloses it — brute-forced independently on both server and client sides). Ruling: the loop stays as defensive code; the test became a property sweep pinning both the wire-cap invariant and the loop's no-op status (a future constant change breaks it loudly); doc comments on both sides drop the load-bearing framing and state the truth; the old client mirror test that claimed the loop fires (passing on the per-axis clamp alone) is replaced the same way. Client citations now name the real server tests verbatim. - Governance (Tyre): D-226 amendment note — progressive cross-rung refinement EXTENDS T-1124 §4 (not supersedes); legacy u32 field scheduled for retirement (T-1159). Server: 1818 lib tests green, clippy/fmt clean. Client: zoom_ladder 48/48, window_request 26/26, viewer 74/74; gdlint clean. Every fix revert-verified.
This commit is contained in:
@@ -1364,6 +1364,76 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// **PR #192 review — Hoshe 1: zero coalescing coverage for
|
||||
/// `WindowGranularity::Region` before this test**, despite Region being
|
||||
/// the highest-fan-out path (progressive capped-density tiling fires
|
||||
/// multiple concurrent Region `DeriveWindow` items per pan/zoom). Mirrors
|
||||
/// `submit_window_does_not_coalesce_different_granularity`'s pattern
|
||||
/// exactly, substituting Region for Quarter: a Region request and a
|
||||
/// District request for the SAME `(connection, body)` are separate
|
||||
/// in-flight slots (the coalescing key is `(conn_id, body_id,
|
||||
/// granularity)`) and must NOT coalesce — both survive as independent
|
||||
/// pending items.
|
||||
#[test]
|
||||
fn submit_window_does_not_coalesce_region_and_district() {
|
||||
let q = GenerationQueue::with_threads(1);
|
||||
// See `submit_window_coalesces_same_connection_and_body`'s comment on
|
||||
// why the occupier must be `analyze()`, not `FillChunk`.
|
||||
q.submit(analyze("Occupier5"), GenPriority::Low);
|
||||
|
||||
let conn = ConnectionId(13);
|
||||
q.submit_window(
|
||||
derive_window_at("OrbitalGranBody", conn, (0, 0), WindowGranularity::Region),
|
||||
GenPriority::Immediate,
|
||||
);
|
||||
q.submit_window(
|
||||
derive_window_at("OrbitalGranBody", conn, (0, 0), WindowGranularity::District),
|
||||
GenPriority::Immediate,
|
||||
);
|
||||
assert_eq!(
|
||||
q.pending_count(),
|
||||
2,
|
||||
"same (connection, body) but Region vs. District must NOT coalesce — \
|
||||
separate in-flight slots, same as the existing District/Quarter pair"
|
||||
);
|
||||
}
|
||||
|
||||
/// The coalescing-DOES-happen counterpart to the test above, for Region
|
||||
/// specifically: two submissions for the SAME `(connection, body,
|
||||
/// Region)` still collapse to one pending item — confirms Region's
|
||||
/// coalescing key behaves identically to District/Quarter's, not just
|
||||
/// that it avoids cross-granularity aliasing.
|
||||
#[test]
|
||||
fn submit_window_coalesces_same_connection_body_and_region_granularity() {
|
||||
let q = GenerationQueue::with_threads(1);
|
||||
q.submit(analyze("Occupier6"), GenPriority::Low);
|
||||
|
||||
let conn = ConnectionId(15);
|
||||
q.submit_window(
|
||||
derive_window_at(
|
||||
"SameOrbitalGranBody",
|
||||
conn,
|
||||
(0, 0),
|
||||
WindowGranularity::Region,
|
||||
),
|
||||
GenPriority::Immediate,
|
||||
);
|
||||
q.submit_window(
|
||||
derive_window_at(
|
||||
"SameOrbitalGranBody",
|
||||
conn,
|
||||
(5, 5),
|
||||
WindowGranularity::Region,
|
||||
),
|
||||
GenPriority::Immediate,
|
||||
);
|
||||
assert_eq!(
|
||||
q.pending_count(),
|
||||
1,
|
||||
"same (connection, body, Region) must still coalesce to one pending item"
|
||||
);
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
// TerrainAnalysisCache (T-1137, PR #187 review — Tyre C1)
|
||||
// -------------------------------------------------------------------
|
||||
|
||||
+117
-10
@@ -359,18 +359,33 @@ fn clamp_window_n(raw_n: u32, granularity: u32) -> u32 {
|
||||
/// - **`District`/`Quarter`:** per-axis cap is [`DISTRICT_WINDOW_MAX_N`]
|
||||
/// (64, unchanged) — byte-identical clamped `n` to [`clamp_window_n`] for
|
||||
/// every input these two variants can produce (verified by
|
||||
/// `clamp_window_n_v2_matches_legacy_for_finer_than_district_rungs`,
|
||||
/// `clamp_window_n_v2_delegates_to_legacy_for_district_and_quarter`,
|
||||
/// below).
|
||||
/// - **`Region`:** per-axis cap is [`DISTRICT_WINDOW_MAX_N_REGION`] (6,400 —
|
||||
/// see that constant's doc for the derivation), then the SAME
|
||||
/// wire-size-ceiling shrink applies on top via [`WindowGranularity::cell_grid_side`]
|
||||
/// — a request whose `cell_grid_side(n)` would exceed
|
||||
/// `sqrt(WIRE_CAP_CELLS)` region cells across is walked back by *halving*
|
||||
/// `n` until it fits (region's `cell_grid_side` is a ROUNDING division, not
|
||||
/// the finer rungs' exact multiplication, so there's no closed-form inverse
|
||||
/// the way `cap_n = sqrt(WIRE_CAP_CELLS) / g` is for the finer case — a
|
||||
/// short bounded loop is the correct tool here, not a formula that would
|
||||
/// have to fight its own rounding).
|
||||
/// see that constant's doc for the derivation), then a halving loop walks
|
||||
/// `n` back if `cell_grid_side(n)` would still exceed `sqrt(WIRE_CAP_CELLS)`
|
||||
/// region cells across.
|
||||
///
|
||||
/// **This loop is defensive, not currently reachable — stated plainly, not
|
||||
/// left implicit.** `DISTRICT_WINDOW_MAX_N_REGION` is DERIVED as
|
||||
/// `sqrt(WIRE_CAP_CELLS) * DISTRICTS_PER_REGION` specifically so the
|
||||
/// per-axis clamp alone already forecloses the loop's trigger condition: a
|
||||
/// brute-force sweep of every `raw_n` in `[1, DISTRICT_WINDOW_MAX_N_REGION]`
|
||||
/// shows `cell_grid_side(n)` never exceeds `sqrt(WIRE_CAP_CELLS)` (64), so
|
||||
/// `n /= 2` never executes for any input the per-axis clamp lets through —
|
||||
/// verified by `clamp_window_n_v2_region_per_axis_cap_alone_satisfies_wire_cap_for_all_inputs`,
|
||||
/// which pins BOTH the invariant (`cell_grid_side(result)² ≤ WIRE_CAP_CELLS`)
|
||||
/// AND the loop's current no-op status (`result == raw_n.clamp(1,
|
||||
/// DISTRICT_WINDOW_MAX_N_REGION)` for every swept input). The loop is kept
|
||||
/// anyway as the general, correct algorithm (region's `cell_grid_side` is a
|
||||
/// ROUNDING division, not the finer rungs' exact multiplication, so there
|
||||
/// is no closed-form inverse the way `cap_n = sqrt(WIRE_CAP_CELLS) / g` is
|
||||
/// for the finer case) — it is the safety net for a FUTURE cap derivation
|
||||
/// that doesn't land exactly on the boundary (a new rung from a later
|
||||
/// measurement pass, or a `WIRE_CAP_CELLS` retune that isn't a perfect
|
||||
/// square times `DISTRICTS_PER_REGION`). If a future constant change makes
|
||||
/// the loop actually fire, the pinned no-op assertion above breaks loudly,
|
||||
/// forcing a deliberate look rather than a silent behavior change.
|
||||
fn clamp_window_n_v2(raw_n: u32, granularity: WindowGranularity) -> u32 {
|
||||
match granularity {
|
||||
WindowGranularity::District | WindowGranularity::Quarter => clamp_window_n(
|
||||
@@ -2537,6 +2552,98 @@ mod tests {
|
||||
assert_eq!(clamp_window_n(8, WINDOW_GRANULARITY_QUARTER), 8);
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
// clamp_window_n_v2 (T-1152; PR #192 review — Hoshe, coordinator ruling
|
||||
// 2026-07-22: test 2 reframed per the brute-force finding that the
|
||||
// Region halving loop is unreachable at the CURRENT constants — see
|
||||
// clamp_window_n_v2's doc comment for the full rationale)
|
||||
// -------------------------------------------------------------------
|
||||
|
||||
/// The exact boundary: `n = DISTRICT_WINDOW_MAX_N_REGION` (6,400) is the
|
||||
/// largest per-axis-legal `n`, and it lands EXACTLY on the wire-size
|
||||
/// ceiling (`cell_grid_side(6400) = 64 = sqrt(WIRE_CAP_CELLS)`,
|
||||
/// `64² = 4,096 = WIRE_CAP_CELLS`) — uncontested, meaning the request is
|
||||
/// NOT further reduced by the halving loop; the per-axis clamp alone is
|
||||
/// already exact at this boundary.
|
||||
#[test]
|
||||
fn clamp_window_n_v2_region_exact_boundary_n6400_uncontested() {
|
||||
let result = clamp_window_n_v2(DISTRICT_WINDOW_MAX_N_REGION, WindowGranularity::Region);
|
||||
assert_eq!(
|
||||
result, DISTRICT_WINDOW_MAX_N_REGION,
|
||||
"n=6400 must pass through unmodified — it already lands exactly on the ceiling"
|
||||
);
|
||||
let side = WindowGranularity::Region.cell_grid_side(result) as u32;
|
||||
assert_eq!(
|
||||
side * side,
|
||||
WIRE_CAP_CELLS,
|
||||
"n=6400's cell_grid_side must land EXACTLY on WIRE_CAP_CELLS, not under or over it"
|
||||
);
|
||||
}
|
||||
|
||||
/// **Reframed per the coordinator's 2026-07-22 ruling (PR #192 review —
|
||||
/// Hoshe).** The originally-briefed name/shape
|
||||
/// (`clamp_window_n_v2_region_halving_loop_fires_above_boundary`, e.g.
|
||||
/// n=6450) does not hold: `raw_n.clamp(1, DISTRICT_WINDOW_MAX_N_REGION)`
|
||||
/// runs BEFORE the halving loop's condition is ever checked, so any
|
||||
/// `raw_n > DISTRICT_WINDOW_MAX_N_REGION` is clamped to exactly 6,400 —
|
||||
/// the SAME uncontested boundary the test above proves — before
|
||||
/// `cell_grid_side` ever sees the raw value. A brute-force sweep (done
|
||||
/// by hand before writing this test, see `clamp_window_n_v2`'s doc
|
||||
/// comment) confirms `cell_grid_side(n)` never exceeds `sqrt(WIRE_CAP_CELLS)`
|
||||
/// for ANY `n` in `[1, DISTRICT_WINDOW_MAX_N_REGION]` — so the halving
|
||||
/// loop is unreachable at the CURRENT constant derivation, not a bug to
|
||||
/// manufacture a test around (coordinator's option 1, not option 2).
|
||||
///
|
||||
/// This test proves the ACTUAL property: the per-axis cap ALONE already
|
||||
/// satisfies the wire-size ceiling for every reachable input, and pins
|
||||
/// the loop's current no-op status explicitly — swept across
|
||||
/// `[1, 2 × DISTRICT_WINDOW_MAX_N_REGION]` (double the legal range, so
|
||||
/// wildly-oversized wire values are covered too, never trusting the
|
||||
/// wire). If a FUTURE constant change (a new rung, a `WIRE_CAP_CELLS`
|
||||
/// retune) ever makes the loop fire, the second assertion below breaks
|
||||
/// LOUDLY — forcing a deliberate look rather than a silent behavior
|
||||
/// change (exactly the safety-net role the loop exists for).
|
||||
#[test]
|
||||
fn clamp_window_n_v2_region_per_axis_cap_alone_satisfies_wire_cap_for_all_inputs() {
|
||||
for raw_n in 1..=(2 * DISTRICT_WINDOW_MAX_N_REGION) {
|
||||
let result = clamp_window_n_v2(raw_n, WindowGranularity::Region);
|
||||
let side = WindowGranularity::Region.cell_grid_side(result) as u32;
|
||||
assert!(
|
||||
side * side <= WIRE_CAP_CELLS,
|
||||
"raw_n={raw_n}: clamped result {result} (side {side}) exceeds WIRE_CAP_CELLS"
|
||||
);
|
||||
assert_eq!(
|
||||
result,
|
||||
raw_n.clamp(1, DISTRICT_WINDOW_MAX_N_REGION),
|
||||
"raw_n={raw_n}: the halving loop must be a no-op at current constants — \
|
||||
the per-axis clamp alone must already be the final answer"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// `District`/`Quarter` through `clamp_window_n_v2` must be BYTE-IDENTICAL
|
||||
/// to the legacy `clamp_window_n` for every input either variant can
|
||||
/// legally carry — `clamp_window_n_v2` is documented as delegating to the
|
||||
/// legacy function unchanged for these two rungs, this pins that claim
|
||||
/// with a sweep rather than a handful of spot values.
|
||||
#[test]
|
||||
fn clamp_window_n_v2_delegates_to_legacy_for_district_and_quarter() {
|
||||
// Sweep well past DISTRICT_WINDOW_MAX_N so the "never trust the wire"
|
||||
// oversized-input case is covered too, not just in-range values.
|
||||
for raw_n in 0..=(DISTRICT_WINDOW_MAX_N * 3) {
|
||||
assert_eq!(
|
||||
clamp_window_n_v2(raw_n, WindowGranularity::District),
|
||||
clamp_window_n(raw_n, WINDOW_GRANULARITY_DISTRICT),
|
||||
"District: clamp_window_n_v2 must match clamp_window_n exactly at raw_n={raw_n}"
|
||||
);
|
||||
assert_eq!(
|
||||
clamp_window_n_v2(raw_n, WindowGranularity::Quarter),
|
||||
clamp_window_n(raw_n, WINDOW_GRANULARITY_QUARTER),
|
||||
"Quarter: clamp_window_n_v2 must match clamp_window_n exactly at raw_n={raw_n}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
// quantize_min_wl_m (T-1150, PR #191 review — Hoshe 1 / Tyre C3, design doc §5)
|
||||
// -------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user