jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(simulation): PR #176 review round — all 12 findings + 2 recommendations addressed

Browse Source 
H1 demux: ShapeProbe defensive multi-shape rejection (union frames now Err, not first-match; +2 tests) and doc claim made honest. H2/T1 SystemIndex.reset_test_state() folded into SimBridge.reset_test_state() (load() inline per autoload rule) + has_pending_request() accessor. H3 no-op tests now assert the replay flag both directions. H4 retry test actually ingests a failure and asserts the retry semantic. H5 error fixture uses the normalized status string. H6 bridge_tcp e2e sends all five frame shapes over real TCP (star-map + city-names buffers asserted). H7 positive replay-on-CONNECTED test via the test_local_bridge test-mode-flip precedent (stub bridge captures + decodes the request bytes). H8/T2 stale PLACEHOLDER doc replaced with the confirmed contract. H9 is_capital doc matches the COALESCE reality. T-r1 demux ceiling written down (next shape = tagged envelope). T-r2 AtlasLayerResponse governance ceiling comment. Lead item: the four cargo-fmt-formatted files from the gate round are now committed (layer_proxy/plugin/bridge-mod/main). H10 note for the record: the 13 snapshot_*.msgpack fixtures in commit 845737617 were regenerated because they were stale against their own generator (pre-existing version-key removal) — verified harmless, no client reads that key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-07-14 17:31:15 +02:00
co-authored by Claude Fable 5
parent 9c990a0733
commit 3304ee30da
12 changed files with 249 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/scripts/autoloads/sim_bridge.gd
+6
View File
@@ -103,6 +103,12 @@ func reset_test_state() -> void:
harness.reset()
# T-949: don't leak a star-map request across tests (autoload state).
_star_map_wanted = false
# SystemIndex's static cache is the same cross-suite leak class (PR #176
# review H2/T1) — reset it here so the dozens of suites already calling
# SimBridge.reset_test_state() cover both. load() inline per the autoload
# parse-order rule (CLAUDE.md).
var SI := load("res://ui/implant/widgets/system_index.gd")
SI.reset_test_state()
func _test_snapshot() -> Dictionary:
client/scripts/protocol/protocol.gd
+5 -3
View File
@@ -774,9 +774,11 @@ static func decode_atlas_layer_response(bytes: PackedByteArray) -> Variant:
## road_graph/settlements (T-960): passthrough fields for the L2 road/rail
## graph and L3 settlement placements, mirroring the district_grid precedent
## (T-1046) — raw decoded maps/arrays, no further client-side reshaping.
## PLACEHOLDER key names ("road_graph"/"settlements") pending confirmation
## against dudley-atlas-server's RoadGraphLayer/SettlementLayer field names —
## this is the one spot to rename if the server picks different keys.
## Key names "road_graph"/"settlements" are the CONFIRMED wire contract —
## identical to server/src/atlas/layer_proxy.rs AtlasLayerResponse's field
## names (pinned 2026-07-14; round-tripped by test_atlas_overlays.gd and the
## server's msgpack round-trip tests). This remains the one client-side spot
## to touch if the contract ever changes.
static func atlas_response_from_raw(raw: Variant) -> Variant:
if not raw is Dictionary or not raw.has("status"):
return null
client/tests/test_atlas_city_names.gd
+4 -1
View File
@@ -149,8 +149,11 @@ func test_city_names_received_error_status_leaves_markers_empty() -> void:
var v := _make_viewer()
v.show_body({"body_id": NON_SOL_BODY_ID}, {"system_id": NON_SOL_SYSTEM_ID})
# Normalized string form — production handlers only ever see the output of
# Protocol.city_names_response_from_raw, which reduces {"Error": msg} to
# "Error" (review H5: the raw wire shape only passed by str() coincidence).
v._on_city_names_received(
{"body_id": NON_SOL_BODY_ID, "status": {"Error": "db unavailable"}, "cities": []}
{"body_id": NON_SOL_BODY_ID, "status": "Error", "cities": []}
)
assert_that(v.get_markers()).override_failure_message(
client/tests/test_atlas_data_delivery.gd
+52 -1
View File
@@ -228,9 +228,60 @@ func test_sim_bridge_still_routes_snapshots_after_starmap_additions() -> void:
func test_request_star_map_is_noop_in_test_mode() -> void:
# SimBridge defaults to test_mode = true (SR_LIVE unset) — request_star_map
# must not error even with no live bridge/server.
# must not error even with no live bridge/server, but it MUST still record
# the wish so replay-on-connect can fire later (review H3: this flag's
# unreset leak was the cross-suite crash fixed in this same PR).
SimBridge.reset_test_state()
SimBridge.request_star_map()
assert_bool(SimBridge._star_map_wanted).override_failure_message(
"test-mode request_star_map must still set the replay flag"
).is_true()
SimBridge.reset_test_state()
func test_request_city_names_is_noop_in_test_mode() -> void:
# Must not error — and must NOT touch the star-map replay flag (guards a
# future copy-paste wiring city-names into the wrong flag).
SimBridge.reset_test_state()
SimBridge.request_city_names("GJ903b")
assert_bool(SimBridge._star_map_wanted).is_false()
class _CaptureBridge:
var sent: Array = []
func send_message(bytes: PackedByteArray) -> int:
sent.append(bytes)
return OK
func test_replay_on_connect_sends_star_map_request_live() -> void:
# Review H7: the replay-on-CONNECTED path (_set_state →
# _send_star_map_request) is the exact mechanism behind the cross-suite
# crash fixed in this PR, and every other test runs test_mode=true where
# the guard short-circuits before touching _bridge — proving only
# "doesn't crash", never "actually replays". Exercise the positive case
# per the test_local_bridge.gd test-mode-flip precedent.
var original_test_mode: bool = SimBridge.test_mode
var original_state: SimBridge.ConnectionState = SimBridge.state
var original_bridge = SimBridge._bridge
SimBridge.reset_test_state()
var stub := _CaptureBridge.new()
SimBridge.test_mode = false
SimBridge._bridge = stub
SimBridge.state = SimBridge.ConnectionState.CONNECTING
SimBridge._star_map_wanted = true
SimBridge._set_state(SimBridge.ConnectionState.CONNECTED)
assert_int(stub.sent.size()).override_failure_message(
"reaching CONNECTED with _star_map_wanted set must replay the request"
).is_equal(1)
var decoded = Messagepack.decode(stub.sent[0])
assert_that(decoded.value).is_equal({"star_map": true})
# Restore autoload state for later suites.
SimBridge.test_mode = original_test_mode
SimBridge._bridge = original_bridge
SimBridge.state = original_state
SimBridge.reset_test_state()
client/tests/test_system_index.gd
+17 -8
View File
@@ -99,14 +99,23 @@ func test_ingest_ignores_non_ready_status() -> void:
func test_request_refresh_retries_after_a_failed_ingest() -> void:
# request_refresh() is a no-op in test mode either way (SimBridge has no
# live connection), so this only proves the _requested/_loaded bookkeeping
# doesn't get stuck: a failed ingest must clear _requested so a later
# request_refresh() is willing to ask again (not just silently no-op
# forever because a prior request was already "in flight").
SystemIndex.ingest({"status": "Ready", "nodes": [{"system_id": "GJ 1"}]})
assert_bool(SystemIndex.is_loaded()).is_true()
SystemIndex.request_refresh() # no-op — already loaded
# The retry semantic itself (review H4): a failed ingest must clear the
# in-flight flag so a later request_refresh() actually asks again — not
# silently no-op forever because a prior request was "in flight".
SystemIndex.reset_test_state()
SystemIndex.request_refresh()
assert_bool(SystemIndex.has_pending_request()).is_true()
assert_bool(SystemIndex.is_loaded()).is_false()
# A failed fetch arrives: stays unloaded, and the in-flight flag clears...
SystemIndex.ingest({"status": "Error", "error": "db unavailable", "nodes": []})
assert_bool(SystemIndex.has_pending_request()).override_failure_message(
"a failed ingest must clear _requested so a later refresh can retry"
).is_false()
assert_bool(SystemIndex.is_loaded()).is_false()
# ...so a retry genuinely re-requests instead of no-opping.
SystemIndex.request_refresh()
assert_bool(SystemIndex.has_pending_request()).is_true()
SystemIndex.reset_test_state()
func test_request_refresh_does_not_crash_when_already_loaded() -> void:
client/ui/implant/widgets/system_index.gd
+17
View File
@@ -32,6 +32,23 @@ static func is_loaded() -> bool:
return _loaded
## True while a StarMapRequest is considered in flight — the test-observable
## counterpart of the retry bookkeeping (see ingest()'s Error path).
static func has_pending_request() -> bool:
return _requested
## Test-only: clear the process-global static cache. Statics leak across
## gdUnit suites in one process — the same class as the
## SimBridge._star_map_wanted leak fixed in this PR (review H2/T1).
## SimBridge.reset_test_state() calls this, so every suite already using it
## gets both resets.
static func reset_test_state() -> void:
_cache = []
_loaded = false
_requested = false
## Ask the bridge for the star map if it hasn't been fetched yet. Safe to call
## from every screen's _ready()/on_install() — idempotent once loaded or a
## request is already in flight.
server/src/atlas/attractor_matching.rs
+3 -2
View File
@@ -40,8 +40,9 @@ pub struct CityRecord {
/// `atlas_city_names.kind == 'capital'` (authored, not derived from
/// population). Threaded onto [`CityPlacement`] for the Atlas
/// [`SettlementLayer`](crate::atlas::layer_proxy::SettlementLayer) (T-960 §2).
/// Defaults to `false` when the source doesn't track `kind` (e.g. the
/// believability harness's own settlement read).
/// Every current reader (`city_context_reader` and the believability
/// harness's own settlement read) selects `COALESCE(kind,'city')`, so an
/// un-authored `kind` yields `false` — there is no kind-less source left.
pub is_capital: bool,
}
server/src/atlas/layer_proxy.rs
+12 -2
View File
@@ -72,6 +72,11 @@ pub struct DistrictGridLayer {
/// A layer response: the computed `Layer1Output` + the coarse district grid
/// (D-225, T-1046) + the road-graph and settlement overlays (T-960 §1/§2), or
/// a non-ready status.
///
/// Growth ceiling (governance-bounded): the one-`Option`-field-per-layer
/// pattern tops out around six fields — D-226's 2026-07-13 amendment (d)
/// rules out any L5/tile Atlas layer ever, leaving T-1112 (quarter
/// footprints) and T-1113 (region climate) as the only remaining candidates.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct AtlasLayerResponse {
pub body_id: String,
@@ -726,8 +731,13 @@ mod tests {
assert_eq!(decoded.body_id, "GJ1c");
let rg = decoded.road_graph.expect("road_graph survives round trip");
assert_eq!(rg.nodes[0].position, (12, 58));
assert_eq!(rg.edges[0].maintenance, MaintenanceAuthority::Administrative);
let settlements = decoded.settlements.expect("settlements survives round trip");
assert_eq!(
rg.edges[0].maintenance,
MaintenanceAuthority::Administrative
);
let settlements = decoded
.settlements
.expect("settlements survives round trip");
assert_eq!(settlements.settlements[0].name, "Port Aldren");
assert_eq!(
settlements.settlements[0].size_class,
server/src/atlas/plugin.rs
+2 -4
View File
@@ -863,10 +863,8 @@ mod tests {
use std::sync::atomic::{AtomicU32, Ordering};
static SEQ: AtomicU32 = AtomicU32::new(0);
let n = SEQ.fetch_add(1, Ordering::Relaxed);
let path = std::env::temp_dir().join(format!(
"sr_plugin_starmap_{}_{n}.json",
std::process::id()
));
let path =
std::env::temp_dir().join(format!("sr_plugin_starmap_{}_{n}.json", std::process::id()));
std::fs::write(&path, r#"{"_meta": {}, "nodes": [], "edges": []}"#).unwrap();
let mut world = World::new();
server/src/bridge/mod.rs
+100 -7
View File
@@ -57,8 +57,14 @@ pub enum BridgeError {
/// fields), the two *new* map shapes each carry a mandatory boolean
/// discriminator field the others don't have at all (`star_map` /
/// `city_names`): a missing required field is a hard deserialize failure, not
/// a silent ignore, so every shape's required-field set is mutually
/// exclusive. `AtlasLayerRequest` itself is untouched byte-for-byte.
/// a silent ignore, so no *minimal well-formed* instance of one shape
/// satisfies another — and [`decode_inbound`] additionally REJECTS union
/// frames that carry more than one shape's discriminators outright (PR #176
/// review H1). `AtlasLayerRequest` itself is untouched byte-for-byte.
///
/// **Ceiling (D-225 trajectory):** four shapes is the practical limit of this
/// hand-rolled sniffing. The next new inbound shape must migrate the channel
/// to the tagged-envelope framing D-225 deferred — do not add a fifth probe.
#[derive(Debug)]
pub enum Inbound {
/// A batch of player inputs (the gameplay path).
@@ -71,17 +77,61 @@ pub enum Inbound {
CityNamesRequest(CityNamesRequest),
}
/// Key-presence probe for the defensive multi-shape check in
/// [`decode_inbound`]: `Option<IgnoredAny>` records whether a key exists
/// without caring about its value or type, so a union frame is detected even
/// when the individual values wouldn't parse as their target types.
#[derive(serde::Deserialize)]
struct ShapeProbe {
body_id: Option<serde::de::IgnoredAny>,
up_to: Option<serde::de::IgnoredAny>,
star_map: Option<serde::de::IgnoredAny>,
city_names: Option<serde::de::IgnoredAny>,
}
/// Demux a received frame payload into an [`Inbound`] (D-225, T-949). Tries,
/// in order: `Vec<PlayerInput>` (array) → `AtlasLayerRequest` (map,
/// `body_id`+`up_to`) → `StarMapRequest` (map, `star_map` discriminator) →
/// `CityNamesRequest` (map, `city_names` discriminator + `body_id`). Every map
/// shape's required fields are mutually exclusive (see the [`Inbound`] doc),
/// so this order is for stability, not correctness — a frame that satisfies
/// none of the four shapes is a genuinely malformed input frame.
/// `CityNamesRequest` (map, `city_names` discriminator + `body_id`).
///
/// Mutual exclusivity is enforced, not assumed: no minimal well-formed
/// instance of one shape satisfies another (see the [`Inbound`] doc), and a
/// defensive pre-check rejects any map frame carrying the discriminators of
/// more than one shape — e.g. a buggy encoder emitting
/// `{"star_map": true, "city_names": true, ...}` — instead of silently
/// routing it to whichever shape is tried first (PR #176 review H1). A frame
/// satisfying none of the four shapes is a genuinely malformed input frame.
pub fn decode_inbound(payload: &[u8]) -> Result<Inbound, BridgeError> {
if let Ok(inputs) = rmp_serde::from_slice::<Vec<PlayerInput>>(payload) {
return Ok(Inbound::Inputs(inputs));
}
// Defensive multi-shape rejection: serde ignores unknown fields, so a
// union frame would otherwise route silently by try-order. Unreachable
// from the shipped client encoders (each sends one minimal shape) — this
// guards buggy or adversarial frames.
if let Ok(probe) = rmp_serde::from_slice::<ShapeProbe>(payload) {
let atlas = probe.body_id.is_some() && probe.up_to.is_some();
let star_map = probe.star_map.is_some();
let city_names = probe.city_names.is_some();
let shapes = usize::from(atlas) + usize::from(star_map) + usize::from(city_names);
if shapes > 1 {
let dump_len = payload.len().min(256);
tracing::error!(
"inbound frame matches {} request shapes at once (atlas={}, star_map={}, city_names={}) — rejecting ambiguous frame. Raw ({} of {} bytes): {:02x?}",
shapes,
atlas,
star_map,
city_names,
dump_len,
payload.len(),
&payload[..dump_len]
);
return Err(BridgeError::DeserializationWithDump(format!(
"ambiguous inbound frame matches {shapes} request shapes (payload {} bytes)",
payload.len()
)));
}
}
if let Ok(req) = rmp_serde::from_slice::<AtlasLayerRequest>(payload) {
return Ok(Inbound::AtlasRequest(req));
}
@@ -442,7 +492,10 @@ impl Plugin for BridgePlugin {
.add_systems(Update, receive_bridge_inputs.in_set(TickPhase::PreInput))
.add_systems(Update, send_bridge_snapshot.in_set(TickPhase::PostSnapshot))
.add_systems(Update, send_atlas_responses.in_set(TickPhase::PostSnapshot))
.add_systems(Update, send_star_map_responses.in_set(TickPhase::PostSnapshot))
.add_systems(
Update,
send_star_map_responses.in_set(TickPhase::PostSnapshot),
)
.add_systems(
Update,
send_city_names_responses.in_set(TickPhase::PostSnapshot),
@@ -605,4 +658,44 @@ mod inbound_tests {
// it's missing the required `up_to` field.
assert!(rmp_serde::from_slice::<AtlasLayerRequest>(&city_names_frame).is_err());
}
/// PR #176 review H1: a union frame carrying more than one shape's
/// discriminators must be REJECTED, not silently routed to whichever
/// shape `decode_inbound` happens to try first.
#[test]
fn ambiguous_union_frame_is_rejected() {
#[derive(serde::Serialize)]
struct StarAndCity {
star_map: bool,
city_names: bool,
body_id: String,
}
let frame = rmp_serde::to_vec_named(&StarAndCity {
star_map: true,
city_names: true,
body_id: "GJ1c".into(),
})
.unwrap();
assert!(
decode_inbound(&frame).is_err(),
"star_map+city_names union frame must be rejected"
);
#[derive(serde::Serialize)]
struct AtlasAndStar {
body_id: String,
up_to: CascadeLayer,
star_map: bool,
}
let frame = rmp_serde::to_vec_named(&AtlasAndStar {
body_id: "GJ1c".into(),
up_to: CascadeLayer::Topography,
star_map: true,
})
.unwrap();
assert!(
decode_inbound(&frame).is_err(),
"atlas+star_map union frame must be rejected"
);
}
}
server/src/main.rs
+3 -3
View File
@@ -211,9 +211,9 @@ fn main() {
star_map_data_path
);
}
app.insert_resource(settled_reach_server::atlas::atlas_data_proxy::StarMapDataPath(
star_map_data_path,
));
app.insert_resource(
settled_reach_server::atlas::atlas_data_proxy::StarMapDataPath(star_map_data_path),
);
// Settlement reader for Layer-3 placement (#955): reads a body's settlements
// from systems.db on a cache miss so the cascade work item stays DB-free.
server/tests/bridge_tcp.rs
+28 -3
View File
@@ -346,10 +346,14 @@ fn single_tick_drains_all_ready_inbound_frames() {
let listener = TcpListener::bind("127.0.0.1:0").expect("failed to bind");
let server_addr = listener.local_addr().expect("failed to get local address");
// Client: three frames back-to-back in one tick window — two input
// batches plus one atlas request (the D-225 demux path). Returns the
// stream so it stays open until assertions complete (no EOF race).
// Client: five frames back-to-back in one tick window — two input
// batches plus one of EACH request shape (atlas, star-map, city-names:
// the full D-225/T-949 demux surface over the real framing/poll path —
// PR #176 review H6). Returns the stream so it stays open until
// assertions complete (no EOF race).
let client_handle = thread::spawn(move || {
use settled_reach_server::atlas::atlas_data_proxy::{CityNamesRequest, StarMapRequest};
let mut stream = TcpStream::connect(server_addr).expect("failed to connect");
for tick in [20u64, 21] {
let inputs = vec![PlayerInput {
@@ -365,6 +369,15 @@ fn single_tick_drains_all_ready_inbound_frames() {
};
let payload = rmp_serde::to_vec_named(&req).expect("failed to serialize");
write_framed(&mut stream, &payload).expect("write atlas frame");
let sm = StarMapRequest { star_map: true };
let payload = rmp_serde::to_vec_named(&sm).expect("failed to serialize star map");
write_framed(&mut stream, &payload).expect("write star map frame");
let cn = CityNamesRequest {
city_names: true,
body_id: "GJ1c".into(),
};
let payload = rmp_serde::to_vec_named(&cn).expect("failed to serialize city names");
write_framed(&mut stream, &payload).expect("write city names frame");
stream
});
@@ -397,6 +410,18 @@ fn single_tick_drains_all_ready_inbound_frames() {
1,
"the atlas request must drain in the same tick"
);
assert_eq!(
world.resource::<StarMapRequestBuffer>().0.len(),
1,
"the star-map request must drain in the same tick (H6: real wire path)"
);
let city_names = &world.resource::<CityNamesRequestBuffer>().0;
assert_eq!(
city_names.len(),
1,
"the city-names request must drain in the same tick (H6: real wire path)"
);
assert_eq!(city_names[0].body_id, "GJ1c");
assert!(
world.resource::<ServerRunning>().0,
"draining must not shut the server down"