From dd00a9cd9e72261cf8b501f7674b8094557ad7a5 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Fri, 27 Feb 2026 19:02:43 +0100 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20address=20PR=20#75=20review?= =?UTF-8?q?=20=E2=80=94=20vision=20components,=20triangle=20validation,=20?= =?UTF-8?q?FNV-1a,=20KG=20gating?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. CRITICAL: spawn_template_npcs now inserts NpcVisionState, NpcMemory, PlayerAwareness on template-spawned NPCs — matches spawn_npc() pattern from PR #66. Without these, template NPCs were invisible to vision and awareness systems. 2. WARNING: generate_intra_template_triangles now calls validate_triangle_def before generating TriangleState — mirrors cross-template path. Updates test TriangleDefs and logistics-hub.yaml to pass all three quality checks (conflict viability, relationship coherence, interest divergence). 3. WARNING: state_hash in compute_observer_snapshot now uses FNV-1a instead of DefaultHasher — consistent with D-010 principle 4 and the pattern in TemplateId/TriangleId. Updates golden file for new hash value. 4. WARNING: TriangleCrisisEventWire.role_assignments now filtered against observer KnowledgeGraph — unknown NPCs redacted from wire event per D-010 principle 2 (information boundaries). 5. WARNING: ActiveTemplateInstances::insert now despawns previous instance entities before overwriting — prevents orphaned ECS entities. Co-Authored-By: Claude Opus 4.6 --- server/data/templates/logistics-hub.yaml | 8 ++- server/src/content/instantiation.rs | 17 ++++++ server/src/content/spawn.rs | 8 +++ server/src/content/template.rs | 60 +++++++++++++++++---- server/src/perception/observer/mod.rs | 37 +++++++++---- server/tests/golden/proof_room_tick_10.json | 2 +- 6 files changed, 108 insertions(+), 24 deletions(-) diff --git a/server/data/templates/logistics-hub.yaml b/server/data/templates/logistics-hub.yaml index 70cd30408..d256adbbe 100644 --- a/server/data/templates/logistics-hub.yaml +++ b/server/data/templates/logistics-hub.yaml @@ -171,9 +171,15 @@ triangles: - "ring-contact" conflict_type: AuthorityChallenge interest_axes: - - Relationships + - Want - Tolerance - Secret + relationship_constraints: + - with_role: "security-guard" + kind: Superior + required_trust: + min: 0 + max: 5 dialogue_pools: - location: "the-terminal" diff --git a/server/src/content/instantiation.rs b/server/src/content/instantiation.rs index 4749b48ca..88ebc9b5b 100644 --- a/server/src/content/instantiation.rs +++ b/server/src/content/instantiation.rs @@ -139,7 +139,24 @@ pub fn instantiate_template( }; // Register in ActiveTemplateInstances (init if absent). + // If a previous instance with the same ID exists, unload it first to + // prevent orphaned ECS entities (Hoshe review #2). world.init_resource::(); + let previous = world + .resource_mut::() + .remove(template_id); + if let Some(prev) = previous { + tracing::warn!( + "instantiate_template: overwriting live TemplateId({}) — despawning {} entities", + template_id.0, + prev.npc_entities.len() + prev.triangle_entities.len(), + ); + for entity in prev.npc_entities.iter().chain(prev.triangle_entities.iter()) { + if world.get_entity(*entity).is_ok() { + world.despawn(*entity); + } + } + } world .resource_mut::() .insert(instance.clone()); diff --git a/server/src/content/spawn.rs b/server/src/content/spawn.rs index f8bc2042e..7b63e3400 100644 --- a/server/src/content/spawn.rs +++ b/server/src/content/spawn.rs @@ -732,6 +732,14 @@ pub fn spawn_template_npcs( let entity = generate_npc(&role_def, world, rng); + // Vision + awareness components (#66) — must match spawn_npc(). + // Without these, vision/awareness systems silently skip template-spawned NPCs. + world.entity_mut(entity).insert(( + crate::npc::vision::NpcVisionState::default(), + crate::npc::vision::NpcMemory::default(), + crate::npc::awareness::PlayerAwareness::default(), + )); + // Register the entity in EntityRegistry and attach stable identity. let stable_id = world.resource_mut::().register(entity); world.entity_mut(entity).insert(( diff --git a/server/src/content/template.rs b/server/src/content/template.rs index 1183886c6..95eab01fa 100644 --- a/server/src/content/template.rs +++ b/server/src/content/template.rs @@ -682,6 +682,16 @@ pub fn generate_intra_template_triangles( let role_to_npc: BTreeMap = npc_roles.into_iter().collect(); for def in defs { + // Validate triangle definition before generating TriangleState (#109). + // Mirrors the cross-template path in generate_cross_template_triangles. + if let Err(e) = validate_triangle_def(def) { + result.warnings.push(format!( + "Triangle {:?}: skipped — validation failed: {}", + def.triangle_id, e + )); + continue; + } + let mut role_assignments = BTreeMap::new(); let mut assigned_npcs = BTreeSet::new(); let mut assignment_ok = true; @@ -1363,7 +1373,11 @@ mod tests { ], conflict_type: ConflictType::LoyaltyConflict, interest_axes: [NpcAxis::Want, NpcAxis::Secret, NpcAxis::Relationships], - relationship_constraints: vec![], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("waitstaff"), + kind: crate::npc::RelationshipKind::Colleague, + required_trust: TrustRange { min: 0, max: 5 }, + }], }, TriangleDef { triangle_id: TriangleId::from_seed_and_roles( @@ -1381,7 +1395,11 @@ mod tests { ], conflict_type: ConflictType::ResourceCompetition, interest_axes: [NpcAxis::Want, NpcAxis::Tolerance, NpcAxis::Contentment], - relationship_constraints: vec![], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("bouncer"), + kind: crate::npc::RelationshipKind::Colleague, + required_trust: TrustRange { min: 0, max: 3 }, + }], }, ]; @@ -1423,8 +1441,12 @@ mod tests { RoleId::new("missing_role"), // no NPC has this role ], conflict_type: ConflictType::SecretExposure, - interest_axes: [NpcAxis::Secret, NpcAxis::Secret, NpcAxis::Secret], - relationship_constraints: vec![], + interest_axes: [NpcAxis::Want, NpcAxis::Secret, NpcAxis::Tolerance], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("bouncer"), + kind: crate::npc::RelationshipKind::Colleague, + required_trust: TrustRange { min: 0, max: 5 }, + }], }, TriangleDef { triangle_id: TriangleId(101), @@ -1434,8 +1456,12 @@ mod tests { RoleId::new("also_missing"), ], conflict_type: ConflictType::LatentTension, - interest_axes: [NpcAxis::Contentment, NpcAxis::Contentment, NpcAxis::Tolerance], - relationship_constraints: vec![], + interest_axes: [NpcAxis::Want, NpcAxis::Contentment, NpcAxis::Tolerance], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("bouncer"), + kind: crate::npc::RelationshipKind::Rival, + required_trust: TrustRange { min: -5, max: 0 }, + }], }, ]; @@ -1475,8 +1501,12 @@ mod tests { RoleId::new("regular"), ], conflict_type: ConflictType::ResourceCompetition, - interest_axes: [NpcAxis::Want, NpcAxis::Want, NpcAxis::Want], - relationship_constraints: vec![], + interest_axes: [NpcAxis::Want, NpcAxis::Secret, NpcAxis::Tolerance], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("bouncer"), + kind: crate::npc::RelationshipKind::Colleague, + required_trust: TrustRange { min: 0, max: 5 }, + }], }]; // Should succeed with 1 triangle but log an error about < 2 @@ -1498,7 +1528,11 @@ mod tests { ], conflict_type: ConflictType::LoyaltyConflict, interest_axes: [NpcAxis::Want, NpcAxis::Secret, NpcAxis::Relationships], - relationship_constraints: vec![], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("b"), + kind: crate::npc::RelationshipKind::Colleague, + required_trust: TrustRange { min: 0, max: 5 }, + }], }, TriangleDef { triangle_id: TriangleId(301), @@ -1508,8 +1542,12 @@ mod tests { RoleId::new("d"), ], conflict_type: ConflictType::LatentTension, - interest_axes: [NpcAxis::Contentment, NpcAxis::Contentment, NpcAxis::Tolerance], - relationship_constraints: vec![], + interest_axes: [NpcAxis::Want, NpcAxis::Contentment, NpcAxis::Tolerance], + relationship_constraints: vec![RelationshipConstraint { + with_role: RoleId::new("c"), + kind: crate::npc::RelationshipKind::Rival, + required_trust: TrustRange { min: -5, max: 0 }, + }], }, ]; diff --git a/server/src/perception/observer/mod.rs b/server/src/perception/observer/mod.rs index 2101f6efc..011e65591 100644 --- a/server/src/perception/observer/mod.rs +++ b/server/src/perception/observer/mod.rs @@ -9,7 +9,6 @@ use bevy_ecs::prelude::*; use std::collections::BTreeSet; -use std::hash::{Hash, Hasher}; use crate::bridge::types::*; use crate::knowledge::graph::filter_by_access; @@ -394,24 +393,40 @@ pub fn compute_observer_snapshot( let save_result = buffer.pending_save_result.take(); // Drain triangle crisis events (#250) and convert to wire format. - let triangle_crisis_events = crisis_queue + // Drain triangle crisis events (#250) and filter role_assignments against + // observer KG (D-010 principle 2: information boundaries are universal). + // NPCs unknown to the observer are redacted from the wire event. + let triangle_crisis_events: Vec = crisis_queue .drain() .into_iter() - .map(TriangleCrisisEventWire::from) + .map(|e| { + let mut wire = TriangleCrisisEventWire::from(e); + wire.role_assignments.retain(|(_, npc_id)| { + observer_kg.knows_entity(&crate::knowledge::types::StableId(*npc_id)) + }); + wire + }) .collect(); // Compute state hash for desync detection (#85). // Hash inputs: player position (x, y, z), NPC count, tick number. - // Uses DefaultHasher for speed — not cryptographic, just comparison. + // Uses FNV-1a (64-bit) for determinism across Rust versions — DefaultHasher + // is explicitly prohibited by D-010 principle 4 (see template.rs module docs). let state_hash = { - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - time.tick.hash(&mut hasher); - observer_pos.x.hash(&mut hasher); - observer_pos.y.hash(&mut hasher); - observer_pos.z.hash(&mut hasher); + let mut hash: u64 = 0xcbf29ce484222325; // FNV-1a offset basis + let fnv_fold = |h: &mut u64, bytes: &[u8]| { + for &b in bytes { + *h ^= b as u64; + *h = h.wrapping_mul(0x100000001b3); // FNV-1a prime + } + }; + fnv_fold(&mut hash, &time.tick.to_le_bytes()); + fnv_fold(&mut hash, &observer_pos.x.to_le_bytes()); + fnv_fold(&mut hash, &observer_pos.y.to_le_bytes()); + fnv_fold(&mut hash, &observer_pos.z.to_le_bytes()); let npc_count = npc_count_query.iter().count() as u64; - npc_count.hash(&mut hasher); - Some(hasher.finish()) + fnv_fold(&mut hash, &npc_count.to_le_bytes()); + Some(hash) }; // Drain sim errors collected this tick (#85) diff --git a/server/tests/golden/proof_room_tick_10.json b/server/tests/golden/proof_room_tick_10.json index fbfd23df1..b6a0e5cf3 100644 --- a/server/tests/golden/proof_room_tick_10.json +++ b/server/tests/golden/proof_room_tick_10.json @@ -72,7 +72,7 @@ "rng_seed": 42, "scan_events": [], "sound_events": [], - "state_hash": 8423425600886013858, + "state_hash": 14452262397297540338, "tick": 8, "triangle_crisis_events": [], "version": 17,