From 3304ee30daeee9d7b6ff9d1e318523de1b001f82 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 14 Jul 2026 17:31:15 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#176=20review=20round?= =?UTF-8?q?=20=E2=80=94=20all=2012=20findings=20+=202=20recommendations=20?= =?UTF-8?q?addressed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- client/scripts/autoloads/sim_bridge.gd | 6 ++ client/scripts/protocol/protocol.gd | 8 +- client/tests/test_atlas_city_names.gd | 5 +- client/tests/test_atlas_data_delivery.gd | 53 ++++++++++- client/tests/test_system_index.gd | 25 +++-- client/ui/implant/widgets/system_index.gd | 17 ++++ server/src/atlas/attractor_matching.rs | 5 +- server/src/atlas/layer_proxy.rs | 14 ++- server/src/atlas/plugin.rs | 6 +- server/src/bridge/mod.rs | 107 ++++++++++++++++++++-- server/src/main.rs | 6 +- server/tests/bridge_tcp.rs | 31 ++++++- 12 files changed, 249 insertions(+), 34 deletions(-) diff --git a/client/scripts/autoloads/sim_bridge.gd b/client/scripts/autoloads/sim_bridge.gd index 2c9cb85a0..650f8011c 100644 --- a/client/scripts/autoloads/sim_bridge.gd +++ b/client/scripts/autoloads/sim_bridge.gd @@ -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: diff --git a/client/scripts/protocol/protocol.gd b/client/scripts/protocol/protocol.gd index 8b600196e..e0fddaebe 100644 --- a/client/scripts/protocol/protocol.gd +++ b/client/scripts/protocol/protocol.gd @@ -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 diff --git a/client/tests/test_atlas_city_names.gd b/client/tests/test_atlas_city_names.gd index 2bb806c21..faf9d067d 100644 --- a/client/tests/test_atlas_city_names.gd +++ b/client/tests/test_atlas_city_names.gd @@ -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( diff --git a/client/tests/test_atlas_data_delivery.gd b/client/tests/test_atlas_data_delivery.gd index 811414772..e0986150d 100644 --- a/client/tests/test_atlas_data_delivery.gd +++ b/client/tests/test_atlas_data_delivery.gd @@ -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() diff --git a/client/tests/test_system_index.gd b/client/tests/test_system_index.gd index 768b7cd47..a86fec42c 100644 --- a/client/tests/test_system_index.gd +++ b/client/tests/test_system_index.gd @@ -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: diff --git a/client/ui/implant/widgets/system_index.gd b/client/ui/implant/widgets/system_index.gd index 0e2378af3..164092e2d 100644 --- a/client/ui/implant/widgets/system_index.gd +++ b/client/ui/implant/widgets/system_index.gd @@ -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. diff --git a/server/src/atlas/attractor_matching.rs b/server/src/atlas/attractor_matching.rs index 5740b6ed8..a31f252fe 100644 --- a/server/src/atlas/attractor_matching.rs +++ b/server/src/atlas/attractor_matching.rs @@ -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, } diff --git a/server/src/atlas/layer_proxy.rs b/server/src/atlas/layer_proxy.rs index a62a5263b..621209562 100644 --- a/server/src/atlas/layer_proxy.rs +++ b/server/src/atlas/layer_proxy.rs @@ -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, diff --git a/server/src/atlas/plugin.rs b/server/src/atlas/plugin.rs index 01d1c909e..cce7eb40f 100644 --- a/server/src/atlas/plugin.rs +++ b/server/src/atlas/plugin.rs @@ -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(); diff --git a/server/src/bridge/mod.rs b/server/src/bridge/mod.rs index 497610325..8bd2e4ef2 100644 --- a/server/src/bridge/mod.rs +++ b/server/src/bridge/mod.rs @@ -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` 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, + up_to: Option, + star_map: Option, + city_names: Option, +} + /// Demux a received frame payload into an [`Inbound`] (D-225, T-949). Tries, /// in order: `Vec` (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 { if let Ok(inputs) = rmp_serde::from_slice::>(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::(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::(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::(&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" + ); + } } diff --git a/server/src/main.rs b/server/src/main.rs index 08b3104cd..131c1905b 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -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. diff --git a/server/tests/bridge_tcp.rs b/server/tests/bridge_tcp.rs index 2d53120a3..4c31b700a 100644 --- a/server/tests/bridge_tcp.rs +++ b/server/tests/bridge_tcp.rs @@ -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::().0.len(), + 1, + "the star-map request must drain in the same tick (H6: real wire path)" + ); + let city_names = &world.resource::().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::().0, "draining must not shut the server down"