fix(simulation): address PR #89 review — version comment, settings contract, docs, dead process recovery

Must-fix: protocol version comment now references PROTOCOL_VERSION
(no hardcoded number), settings delete is idempotent no-op.

Suggestions addressed: BehaviorModifier dedup claim dropped, hash
collision safety documented, FK pragma in test store, columns_to_value
consolidated, modifier_hint mismatch logging, OffDuty test coverage,
batching tradeoff documented, unknown value_type warning, occluded
text risk documented, dead child respawn in voice worker, INJECT
block ambiguity documented.

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

server/src/bridge/types.rs

@@ -84,7 +84,7 @@ pub struct StartupMessage {
 /// Future fields: ambient sound events, HUD state (D-020 expansion).
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct ObserverSnapshot {
-    /// Protocol version for forward compatibility. Current: 19.
+    /// Protocol version for forward compatibility. See [`PROTOCOL_VERSION`].
     pub version: u8,
     /// Simulation tick when this snapshot was produced
     pub tick: u64,

server/src/npc/blueprint.rs

@@ -158,7 +158,6 @@ impl Default for BehaviorContext {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct BehaviorModifier {
     /// Modifier category for matching against `BehaviorPrimitive.modifier_hint`.
-    /// Also used for dedup — at most one modifier per category per NPC.
     pub category: String,
     /// The modifier clause text. Concatenated to action text with a space.
     pub clause: String,
@@ -197,6 +196,10 @@ pub fn assemble_behaviors(
             let hinted: Vec<&BehaviorModifier> =
                 modifiers.iter().filter(|m| &m.category == hint).collect();
             if hinted.is_empty() {
+                tracing::trace!(
+                    "modifier_hint '{}' matched no category; falling back to all modifiers",
+                    hint
+                );
                 modifiers.iter().collect()
             } else {
                 hinted
@@ -674,6 +677,36 @@ mod tests {
         assert_eq!(all.len(), 3);
     }
 
+    #[test]
+    fn assemble_behaviors_off_duty_context_filter() {
+        use crate::simulation::rng::SimRng;
+        let mut rng = SimRng::new(42);
+
+        let primitives = vec![
+            BehaviorPrimitive {
+                action: "operates crane".into(),
+                context: BehaviorContext::OnShift,
+                modifier_hint: None,
+            },
+            BehaviorPrimitive {
+                action: "sleeps in bunk".into(),
+                context: BehaviorContext::OffDuty,
+                modifier_hint: None,
+            },
+            BehaviorPrimitive {
+                action: "stretches".into(),
+                context: BehaviorContext::Any,
+                modifier_hint: None,
+            },
+        ];
+
+        // OffDuty filter: should return OffDuty + Any, exclude OnShift
+        let off_duty = assemble_behaviors(&primitives, &[], Some(BehaviorContext::OffDuty), &mut rng);
+        assert_eq!(off_duty.len(), 2);
+        assert_eq!(off_duty[0], "sleeps in bunk");
+        assert_eq!(off_duty[1], "stretches");
+    }
+
     #[test]
     fn assemble_behaviors_hint_fallback_to_any_modifier() {
         use crate::simulation::rng::SimRng;

server/src/settings/mod.rs

@@ -139,6 +139,9 @@ pub fn process_settings_commands(
     };
 
     // Process all commands, last response wins (same tick batching).
+    // If multiple settings commands arrive in one tick, only the final response
+    // reaches the client. This is acceptable: the client sends one action per tick
+    // in normal use, and batch changes via RequestAll after bulk updates.
     for cmd in commands {
         match cmd {
             SettingsCommand::Change { ref key, ref value } => {
@@ -171,16 +174,13 @@ pub fn process_settings_commands(
                 });
             }
             SettingsCommand::Delete { ref key } => {
-                let deleted = store.delete(key);
+                // Idempotent: deleting a non-existent key is a silent no-op.
+                let _deleted = store.delete(key);
                 snapshot_buffer.pending_settings_response = Some(SettingsResponseWire {
                     success: true,
                     kind: "ack".into(),
                     settings: vec![],
-                    error: if deleted {
-                        None
-                    } else {
-                        Some(format!("setting not found: {}", key))
-                    },
+                    error: None,
                 });
             }
         }

server/src/settings/store.rs

@@ -49,6 +49,7 @@ impl SettingsStore {
     /// Open an in-memory database (for tests).
     pub fn open_in_memory() -> Result<Self, rusqlite::Error> {
         let conn = Connection::open_in_memory()?;
+        conn.execute_batch("PRAGMA foreign_keys = ON;")?;
         conn.execute(
             "CREATE TABLE settings (
                 player_id   TEXT    NOT NULL,
@@ -92,7 +93,7 @@ impl SettingsStore {
         match rows.next()? {
             Some(row) => {
                 let vtype: String = row.get(0)?;
-                Ok(Some(columns_to_value(&vtype, row)?))
+                Ok(Some(columns_to_value(&vtype, row, 1)?))
             }
             None => Ok(None),
         }
@@ -112,7 +113,7 @@ impl SettingsStore {
         while let Some(row) = rows.next()? {
             let key: String = row.get(0)?;
             let vtype: String = row.get(1)?;
-            let value = columns_to_value_offset(&vtype, row)?;
+            let value = columns_to_value(&vtype, row, 2)?;
             result.insert(key, value);
         }
         Ok(result)
@@ -158,33 +159,25 @@ fn value_to_columns(
     }
 }
 
-/// Read a SettingValue from a row where value columns start at index 1.
-/// Used by `get()` which selects (value_type, value_text, value_int, value_float, value_bool).
+/// Read a SettingValue from a row where value columns start at `offset`.
+/// `offset` is the index of the value_text column (value_int = offset+1, etc.).
+///
+/// Used by `get()` (offset=1) and `get_all()` (offset=2) which select different
+/// column prefixes before the value columns.
 fn columns_to_value(
     vtype: &str,
     row: &rusqlite::Row<'_>,
+    offset: usize,
 ) -> Result<SettingValue, rusqlite::Error> {
     match vtype {
-        "string" => Ok(SettingValue::String(row.get::<_, String>(1)?)),
-        "int" => Ok(SettingValue::Int(row.get::<_, i64>(2)?)),
-        "float" => Ok(SettingValue::Float(row.get::<_, f64>(3)?)),
-        "bool" => Ok(SettingValue::Bool(row.get::<_, bool>(4)?)),
-        other => Ok(SettingValue::String(format!("<unknown type: {}>", other))),
-    }
-}
-
-/// Read a SettingValue from a row where value columns start at index 2.
-/// Used by `get_all()` which selects (key, value_type, value_text, ...).
-fn columns_to_value_offset(
-    vtype: &str,
-    row: &rusqlite::Row<'_>,
-) -> Result<SettingValue, rusqlite::Error> {
-    match vtype {
-        "string" => Ok(SettingValue::String(row.get::<_, String>(2)?)),
-        "int" => Ok(SettingValue::Int(row.get::<_, i64>(3)?)),
-        "float" => Ok(SettingValue::Float(row.get::<_, f64>(4)?)),
-        "bool" => Ok(SettingValue::Bool(row.get::<_, bool>(5)?)),
-        other => Ok(SettingValue::String(format!("<unknown type: {}>", other))),
+        "string" => Ok(SettingValue::String(row.get::<_, String>(offset)?)),
+        "int" => Ok(SettingValue::Int(row.get::<_, i64>(offset + 1)?)),
+        "float" => Ok(SettingValue::Float(row.get::<_, f64>(offset + 2)?)),
+        "bool" => Ok(SettingValue::Bool(row.get::<_, bool>(offset + 3)?)),
+        other => {
+            tracing::warn!("unknown settings value_type '{}', treating as String", other);
+            Ok(SettingValue::String(format!("<unknown type: {}>", other)))
+        }
     }
 }
 

server/src/voice/integration.rs

@@ -136,6 +136,17 @@ pub fn voice_enrich_dialogue_response(
 /// not available at this point (occlusion has already been applied), so the
 /// `occluded_line` is treated as the base text for the voice lookup.
 /// This means the voice register wraps the already-occluded line.
+///
+/// ## Accepted risk: re-voicing of heavily occluded text
+///
+/// When many words are dropped by D-078 occlusion, the remaining text may
+/// be fragmentary ("... the ... came in ..."). Re-voicing such fragments can
+/// produce incoherent output. This is acceptable for two reasons:
+/// 1. Cache misses are common for conversation text (no pre-baking pipeline),
+///    so the base text fallback in `voiced_behavior()` fires most of the time.
+/// 2. Even incoherent voiced output is no worse than the already-degraded
+///    overheard line — the occlusion itself has already broken coherence.
+///    The player's inability to fully parse overheard speech is the mechanic.
 #[allow(clippy::type_complexity)]
 pub fn voice_enrich_conversation_events(
     voice_cache: Option<Res<VoiceCacheResource>>,
@@ -170,8 +181,13 @@ pub fn voice_enrich_conversation_events(
         };
 
         let tell_state = tell_state_opt.and_then(|t| t.category);
-        // Use speaker_id as the content derivation input — conversation lines
-        // don't have a stable line_id, so use a hash of the line text itself.
+        // Conversation lines don't have a stable line_id; derive content_index
+        // from the occluded line text. The full cache key is
+        // (culture_id, npc_stable_id, content_type, content_index, tell_state),
+        // so a u16 hash collision requires two different lines from the same
+        // speaker with the same tell state to hash identically — at ~65K
+        // possible values this is extremely rare, and the worst outcome is
+        // a stale voiced line being served instead of base text. Acceptable.
         let content_index = content_index_from_line_id(&event.occluded_line);
 
         let voiced = voiced_behavior(

server/src/voice/prompt_builder.rs

@@ -258,6 +258,30 @@ pub fn build_prompt(
     // 6. Occasional injections — imperative, positioned near TASK for 2B attention.
     // The composition engine already controls frequency — once an injection fires,
     // the model must execute it without discretion.
+    //
+    // ## Multiple INJECT blocks: known ambiguity
+    //
+    // When more than one injection fires on the same prompt, the model sees
+    // multiple blocks with the same `INJECT:` label and no numeric index:
+    //
+    //   INJECT: Include the phrase from this example in your output.
+    //   INPUT: discovers a critical part is missing
+    //   OUTPUT: Void take it. The coupling's not here.
+    //
+    //   INJECT: Include the phrase from this example in your output.
+    //   INPUT: (second injection)
+    //   OUTPUT: (second output)
+    //
+    // 2B models have no mechanism to distinguish them. In practice they tend
+    // to honour the last block (recency bias) and may ignore earlier ones.
+    // The REMEMBER reminder at the end of the prompt references only the first
+    // fired injection, which compounds the asymmetry.
+    //
+    // Accepted limitation: true multi-injection compliance is out of scope for
+    // a 2B model. The low per-injection frequency (typically ≤0.25) means
+    // simultaneous fires are rare. Cultures with multiple injections should
+    // keep the set small and frequencies low enough that co-firing is a
+    // statistical edge case rather than expected behaviour.
     let mut rng = ChaCha8Rng::seed_from_u64(seed);
     for (i, injection) in culture.occasional_injections.iter().enumerate() {
         // Gate off for suppressive tells

server/src/voice/worker.rs

@@ -311,8 +311,26 @@ fn worker_loop(id: usize, config: VoiceProcessConfig, ctx: WorkerContext) {
         }
 
         ctx.active_count.fetch_add(1, Ordering::Relaxed);
-        process_request(id, &mut pipe, &request, &ctx);
+        let pipe_alive = process_request(id, &mut pipe, &request, &ctx);
         ctx.active_count.fetch_sub(1, Ordering::Relaxed);
+
+        if !pipe_alive {
+            // sr-voice child has exited. Attempt a one-shot respawn so the
+            // worker can continue serving requests. If the respawn fails
+            // (binary missing, OOM, config error) we exit rather than
+            // spinning and flooding the log with identical error lines.
+            tracing::warn!(id, "sr-voice child process died — attempting respawn");
+            match VoicePipe::spawn(&config) {
+                Ok(new_pipe) => {
+                    tracing::info!(id, "sr-voice child respawned successfully");
+                    pipe = new_pipe;
+                }
+                Err(e) => {
+                    tracing::error!(id, error = %e, "sr-voice respawn failed — worker exiting");
+                    break;
+                }
+            }
+        }
     }
 
     tracing::debug!(id, "voice worker stopped");
@@ -320,17 +338,21 @@ fn worker_loop(id: usize, config: VoiceProcessConfig, ctx: WorkerContext) {
 }
 
 /// Process a single voice request: build prompt → infer → validate → cache.
+///
+/// Returns `true` if the pipe is still alive after processing, `false` if
+/// the sr-voice child process has exited. The caller is responsible for
+/// respawning or breaking the worker loop on `false`.
 fn process_request(
     worker_id: usize,
     pipe: &mut VoicePipe,
     request: &VoiceRequest,
     ctx: &WorkerContext,
-) {
+) -> bool {
     // Factual lines bypass the LLM entirely — serve base text directly.
     // 2B models corrupt numbers and invert denials (Spike 2 finding, D-138).
     if request.content_type == ContentType::Factual {
         cache_base_text(request, ctx);
-        return;
+        return true;
     }
 
     let culture = match ctx.cultures.get(&request.culture_id) {
@@ -341,7 +363,7 @@ fn process_request(
                 "unknown culture — serving base text"
             );
             cache_base_text(request, ctx);
-            return;
+            return true;
         }
     };
 
@@ -360,6 +382,7 @@ fn process_request(
     match result {
         Ok(text) if text.split_whitespace().count() >= MIN_TOKENS => {
             cache_result(request, &text, ctx);
+            true
         }
         Ok(_short_text) => {
             // Empty output guard: retry once with different seed
@@ -378,6 +401,18 @@ fn process_request(
             match pipe.generate(&retry_built.prompt, Some(request.seed.wrapping_add(1))) {
                 Ok(text) if text.split_whitespace().count() >= MIN_TOKENS => {
                     cache_result(request, &text, ctx);
+                    true
+                }
+                Err(e) => {
+                    let pipe_dead = pipe.child.try_wait().ok().flatten().is_some();
+                    tracing::warn!(
+                        worker_id,
+                        error = %e,
+                        pipe_dead,
+                        "sr-voice retry failed — serving base text"
+                    );
+                    cache_base_text(request, ctx);
+                    !pipe_dead
                 }
                 _ => {
                     tracing::debug!(
@@ -386,16 +421,23 @@ fn process_request(
                         "retry also short — caching base text"
                     );
                     cache_base_text(request, ctx);
+                    true
                 }
             }
         }
         Err(e) => {
+            // Check whether the child process has exited — if so, signal the
+            // caller to respawn. Without this, every subsequent request fails
+            // with the same error and floods the log indefinitely.
+            let pipe_dead = pipe.child.try_wait().ok().flatten().is_some();
             tracing::warn!(
                 worker_id,
                 error = %e,
+                pipe_dead,
                 "sr-voice request failed — serving base text"
             );
             cache_base_text(request, ctx);
+            !pipe_dead
         }
     }
 }