From 51414b0b63c1563f98e5045b172b14188c27aa1b Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Fri, 13 Mar 2026 09:34:33 +0100 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20address=20PR=20#89=20review?= =?UTF-8?q?=20=E2=80=94=20version=20comment,=20settings=20contract,=20docs?= =?UTF-8?q?,=20dead=20process=20recovery?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- server/src/bridge/types.rs | 2 +- server/src/npc/blueprint.rs | 35 ++++++++++++++++++++- server/src/settings/mod.rs | 12 +++---- server/src/settings/store.rs | 41 ++++++++++-------------- server/src/voice/integration.rs | 20 ++++++++++-- server/src/voice/prompt_builder.rs | 24 ++++++++++++++ server/src/voice/worker.rs | 50 +++++++++++++++++++++++++++--- 7 files changed, 146 insertions(+), 38 deletions(-) diff --git a/server/src/bridge/types.rs b/server/src/bridge/types.rs index e15dad2f0..f1c116add 100644 --- a/server/src/bridge/types.rs +++ b/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, diff --git a/server/src/npc/blueprint.rs b/server/src/npc/blueprint.rs index f58ba78db..afa273ee0 100644 --- a/server/src/npc/blueprint.rs +++ b/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; diff --git a/server/src/settings/mod.rs b/server/src/settings/mod.rs index c4bb253dc..52f506e22 100644 --- a/server/src/settings/mod.rs +++ b/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, }); } } diff --git a/server/src/settings/store.rs b/server/src/settings/store.rs index 3f38e670f..58f176d67 100644 --- a/server/src/settings/store.rs +++ b/server/src/settings/store.rs @@ -49,6 +49,7 @@ impl SettingsStore { /// Open an in-memory database (for tests). pub fn open_in_memory() -> Result { 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 { 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!("", 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 { - 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!("", 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!("", other))) + } } } diff --git a/server/src/voice/integration.rs b/server/src/voice/integration.rs index d474e08cd..91fa08704 100644 --- a/server/src/voice/integration.rs +++ b/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>, @@ -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( diff --git a/server/src/voice/prompt_builder.rs b/server/src/voice/prompt_builder.rs index 8583c6747..92c72fc28 100644 --- a/server/src/voice/prompt_builder.rs +++ b/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 diff --git a/server/src/voice/worker.rs b/server/src/voice/worker.rs index e6983614e..7b5c6fbc2 100644 --- a/server/src/voice/worker.rs +++ b/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 } } }