fix(simulation): address PR #87 review — name collision, validation, and polish

- Fix critical name collision: shuffle+pop for unique NPC names (#3)
- Validate population_density >= 1 in generator and validator (#4)
- Guard against empty given_names/roles with validator warnings (#5)
- Extract filler word cap to MAX_FILLER_WORDS constant (#6)
- Fix cultural behavior gate checking wrong field (#7)
- Document intentional one-directional relationships (#8)
- Validate min_npcs <= max_npcs in validator (#9)
- Fix validate-ron script realpath error handling (#10)
- Add TODO comments for spike-specific code duplication (#11, #12, #13)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

server/src/bin/generator_spike.rs

@@ -295,6 +295,11 @@ const ALL_TRAITS: [PersonalityTrait; 10] = [
     PersonalityTrait::Reclusive,
 ];
 
+// TODO(integration): Duplicates `traits_contradict` from npc/generate.rs.
+// Extract to a shared helper in npc/mod.rs when wiring the spike into the main pipeline.
+// TODO(#612 integration): duplicates the private `traits_contradict` in npc/generate.rs.
+// When wiring the spike into the main pipeline, expose that function as pub(crate)
+// and remove this copy. Kept separate for now to avoid spike coupling to ECS internals.
 fn traits_contradict(a: PersonalityTrait, b: PersonalityTrait) -> bool {
     use PersonalityTrait::*;
     matches!(
@@ -354,20 +359,49 @@ fn gen_traits(rng: &mut SimRng, culture: &CultureProfile) -> Vec<PersonalityTrai
 }
 
 // ---------------------------------------------------------------------------
-// Name generation
+// Name generation (without replacement — #3 fix)
 // ---------------------------------------------------------------------------
 
-fn gen_name(rng: &mut SimRng, culture: &CultureProfile) -> String {
+/// Build a shuffled name pool of up to `count` unique names.
-    let given_idx = rng.rng.random_range(0..culture.naming.given_names.len());
+///
-    let given = &culture.naming.given_names[given_idx];
+/// Uses Fisher-Yates on given_names indices so each name appears at most once.
-
+/// Returns fewer names than requested if the pool is smaller than `count`.
-    if culture.naming.family_name_used_socially && !culture.naming.family_names.is_empty() {
+/// Callers must use the returned `Vec`'s length as the actual NPC count.
-        let family_idx = rng.rng.random_range(0..culture.naming.family_names.len());
+///
-        let family = &culture.naming.family_names[family_idx];
+/// Exits if `given_names` is empty (validator should catch this upstream,
-        format!("{} {}", given, family)
+/// but we guard here defensively — #5 fix).
-    } else {
+fn build_name_pool(rng: &mut SimRng, culture: &CultureProfile, count: usize) -> Vec<String> {
-        given.clone()
+    if culture.naming.given_names.is_empty() {
+        eprintln!(
+            "Error: culture '{}' has no given_names — cannot generate NPCs.",
+            culture.id
+        );
+        process::exit(1);
     }
+
+    let pool_size = culture.naming.given_names.len();
+    let actual_count = count.min(pool_size);
+
+    // Partial Fisher-Yates — produces `actual_count` unique given-name indices.
+    let mut indices: Vec<usize> = (0..pool_size).collect();
+    for i in 0..actual_count {
+        let swap = rng.rng.random_range(i..pool_size);
+        indices.swap(i, swap);
+    }
+
+    indices[..actual_count]
+        .iter()
+        .map(|&i| {
+            let given = &culture.naming.given_names[i];
+            if culture.naming.family_name_used_socially && !culture.naming.family_names.is_empty() {
+                let family_idx = rng.rng.random_range(0..culture.naming.family_names.len());
+                let family = &culture.naming.family_names[family_idx];
+                format!("{} {}", given, family)
+            } else {
+                given.clone()
+            }
+        })
+        .collect()
 }
 
 // ---------------------------------------------------------------------------
@@ -375,6 +409,13 @@ fn gen_name(rng: &mut SimRng, culture: &CultureProfile) -> String {
 // ---------------------------------------------------------------------------
 
 fn pick_role<'a>(rng: &mut SimRng, zone: &'a ZoneSpec) -> &'a RoleSpec {
+    if zone.roles.is_empty() {
+        eprintln!(
+            "Error: zone '{}' has no roles — cannot generate NPCs.",
+            zone.zone_type
+        );
+        process::exit(1);
+    }
     let total_weight: u32 = zone.roles.iter().map(|r| r.weight as u32).sum();
     let roll = rng.rng.random_range(0..total_weight);
     let mut cumulative = 0u32;
@@ -400,13 +441,12 @@ fn gen_behaviors(rng: &mut SimRng, role: &RoleSpec, culture: &CultureProfile) ->
         behaviors.push(role.typical_behaviors[idx].clone());
     }
 
-    // Pick 1 culture-specific behavior (50% chance to add a second entry)
+    // 50% chance to add a cultural behavior from the speech greeting pool.
-    if !culture.values.description.is_empty() && rng.rng.random_range(0..2_u32) == 0 {
+    // Guard on greetings being non-empty — cultural_tendency draws from it.
-        if !culture.naming.given_names.is_empty() {
+    // (#7 fix: was incorrectly checking given_names, which is unrelated here)
-            // Use a behavioral tendency derived from cultural values description
+    if !culture.speech.greetings.is_empty() && rng.rng.random_range(0..2_u32) == 0 {
-            let cultural_behavior = cultural_tendency(rng, culture);
+        let cultural_behavior = cultural_tendency(rng, culture);
-            behaviors.push(cultural_behavior);
+        behaviors.push(cultural_behavior);
-        }
     }
 
     behaviors
@@ -427,10 +467,12 @@ fn cultural_tendency(rng: &mut SimRng, culture: &CultureProfile) -> String {
 // Cultural markers
 // ---------------------------------------------------------------------------
 
+/// Maximum filler words assigned to a single NPC from the culture pool.
+const MAX_FILLER_WORDS: usize = 2;
+
 fn gen_cultural_markers(rng: &mut SimRng, culture: &CultureProfile) -> CulturalMarkers {
-    // Pick 1-2 filler words
     let filler_count = if culture.speech.filler_words.len() > 1 {
-        rng.rng.random_range(1_usize..=2.min(culture.speech.filler_words.len()))
+        rng.rng.random_range(1_usize..=MAX_FILLER_WORDS.min(culture.speech.filler_words.len()))
     } else {
         culture.speech.filler_words.len()
     };
@@ -463,6 +505,12 @@ fn gen_cultural_markers(rng: &mut SimRng, culture: &CultureProfile) -> CulturalM
 // Relationship generation (name-based for spike — no StableId)
 // ---------------------------------------------------------------------------
 
+/// Generate 0–3 relationships for an NPC.
+///
+/// **One-directional by design:** each NPC independently picks their own
+/// relationships. If Tev knows Renn as a "colleague (positive)" but Renn
+/// has no entry for Tev, that is intentional — asymmetric awareness is
+/// a core mechanic (D-034). The output may look like bugs but is correct.
 fn gen_relationships(
     rng: &mut SimRng,
     this_name: &str,
@@ -488,6 +536,7 @@ fn gen_relationships(
         indices.swap(i, swap);
     }
 
+    // TODO(integration): Use RelationshipKind enum from npc/mod.rs instead of strings.
     let rel_kinds = ["colleague", "friend", "rival", "superior", "subordinate"];
     let valences = [
         RelationshipValence::Positive,
@@ -517,15 +566,13 @@ fn generate_npc_blueprint(
     rng: &mut SimRng,
     zone: &ZoneSpec,
     culture: &CultureProfile,
-    all_names: &[String],
+    name: String,
 ) -> NpcBlueprint {
-    let name = gen_name(rng, culture);
     let role = pick_role(rng, zone);
     let traits = gen_traits(rng, culture);
     let observable_behaviors = gen_behaviors(rng, role, culture);
     let cultural_markers = gen_cultural_markers(rng, culture);
-    // Relationships assigned in a second pass once all names are known
+    // Relationships assigned in a second pass once all names are known.
-    let _ = all_names; // populated after all NPCs are named
 
     NpcBlueprint {
         name,
@@ -541,6 +588,7 @@ fn generate_npc_blueprint(
 // Output formatting
 // ---------------------------------------------------------------------------
 
+// TODO(integration): Derive Display on PersonalityTrait and drop this function.
 fn trait_label(t: PersonalityTrait) -> &'static str {
     use PersonalityTrait::*;
     match t {
@@ -643,18 +691,40 @@ fn main() {
         (zone, culture)
     };
 
+    // Validate zone spec
+    if zone.population_density < 1 {
+        eprintln!(
+            "Error: population_density must be >= 1 (got {} in zone '{}')",
+            zone.population_density, zone.zone_type
+        );
+        process::exit(1);
+    }
+    if zone.roles.is_empty() {
+        eprintln!(
+            "Error: zone '{}' has no roles defined — cannot generate NPCs.",
+            zone.zone_type
+        );
+        process::exit(1);
+    }
+
     // Seed the RNG
     let mut rng = SimRng::new(args.seed);
 
-    // Determine NPC count from zone density
+    // Determine NPC count from zone density.
-    let npc_count = rng.rng.random_range(
+    // population_density already validated >= 1 above.
-        zone.population_density as usize..=(zone.population_density as usize * 2).max(1)
+    let density = zone.population_density as usize;
-    );
+    let npc_count = rng.rng.random_range(density..=(density * 2));
     let npc_count = npc_count.max(2); // at least 2 for relationship output
 
+    // Pre-generate unique names without replacement (#3 fix).
+    // build_name_pool caps at pool size — actual count may be less than requested.
+    let name_pool = build_name_pool(&mut rng, &culture, npc_count);
+    let _npc_count = name_pool.len(); // use capped count (for documentation)
+
     // First pass: generate all NPCs (no relationships yet)
-    let mut npcs: Vec<NpcBlueprint> = (0..npc_count)
+    let mut npcs: Vec<NpcBlueprint> = name_pool
-        .map(|_| generate_npc_blueprint(&mut rng, &zone, &culture, &[]))
+        .into_iter()
+        .map(|name| generate_npc_blueprint(&mut rng, &zone, &culture, name))
         .collect();
 
     // Collect all names for relationship pass

server/src/bin/validate_ron.rs

@@ -49,6 +49,27 @@ fn main() {
             Ok(spec) => {
                 println!("Valid ZoneSpec: {} ({})", spec.label, spec.zone_type);
                 println!("  {} roles, {} social sites", spec.roles.len(), spec.social_sites.len());
+                let mut warnings = 0;
+                if spec.roles.is_empty() {
+                    eprintln!("  WARNING: no roles defined — generator will reject this");
+                    warnings += 1;
+                }
+                if spec.population_density < 1 {
+                    eprintln!("  WARNING: population_density is 0 — generator requires >= 1");
+                    warnings += 1;
+                }
+                for site in &spec.social_sites {
+                    if site.min_npcs > site.max_npcs {
+                        eprintln!(
+                            "  WARNING: social site '{}' has min_npcs ({}) > max_npcs ({})",
+                            site.site_type, site.min_npcs, site.max_npcs
+                        );
+                        warnings += 1;
+                    }
+                }
+                if warnings > 0 {
+                    process::exit(1);
+                }
             }
             Err(e) => {
                 eprintln!("Invalid ZoneSpec in {}:", args.file);
@@ -69,6 +90,14 @@ fn main() {
                     profile.speech.filler_words.len(),
                     profile.values.favored_traits.len()
                 );
+                let mut warnings = 0;
+                if profile.naming.given_names.is_empty() {
+                    eprintln!("  WARNING: no given_names — generator will reject this");
+                    warnings += 1;
+                }
+                if warnings > 0 {
+                    process::exit(1);
+                }
             }
             Err(e) => {
                 eprintln!("Invalid CultureProfile in {}:", args.file);

tooling/validate-ron

@@ -20,7 +20,12 @@ if [ $# -lt 2 ]; then
     exit 1
 fi
 
-# Resolve file to absolute path before changing directory
+# Check file exists before resolving — realpath gives unhelpful errors otherwise
+if [ ! -f "$1" ]; then
+    echo "Error: file not found: $1"
+    exit 1
+fi
+
 FILE="$(realpath "$1")"
 SCHEMA="$2"