fix(server): address PR #23 review — 4 critical bugs, 3 warnings, 11 suggestions

Critical fixes:
- Add PendingRecognitionWire serialization roundtrip test
- Check Option return from delay.cancel() before logging
- InputQueue capacity limit (1000) with drop-oldest and warning
- TODO in observation.rs references ticket #450

Warning fixes:
- Hot-reload guards against invalid/empty content root
- Location header mismatch warning in line pool indexing
- walk_yaml() depth limit (100) against symlink loops
- Consecutive reload failure counter (warns after 5+)

Test additions:
- Negative prerequisite filtering test for monologue lines
- Integration test for pending_recognitions in observer snapshot
- Eavesdrop threshold ordering assertion (Careful < default)

Documentation:
- Playtesting expectation comments on delay constants
- is_pending() scalability note for future NPC cognitive delay
- ID format regex validation in line pool spec
- BTreeMap vs sort() ordering clarification in loader
- Multiplayer TODO in relationships.rs references D-010
- ContentSlug ticket #452 filed for entity slug resolution

394 tests, 0 failures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
2026-02-16 01:03:51 +01:00
co-authored by Claude Opus 4.6
parent 8a334b47f3
commit d4fdbf426e
11 changed files with 379 additions and 17 deletions
+13
View File
@@ -336,6 +336,19 @@ lines:
| `character` | single char | `s` = smuggler, `d` = detective. **Monologue only.** | `s`, `d` |
| `sequence` | 3-digit zero-padded | `001``999` | `001`, `042` |
**General validation regex** (matches both dialogue and monologue IDs):
```
^[a-z0-9-]+_(d|m)(_[a-z])?_\d{3}$
```
- `[a-z0-9-]+` — location slug (kebab-case, at least one character)
- `(d|m)` — pool type: `d` for dialogue, `m` for monologue
- `(_[a-z])?` — optional character segment (monologue only): `_s` or `_d`
- `\d{3}` — three-digit zero-padded sequence number
Use the pool-specific regexes in [Section 5.2](#52-regex-patterns) for strict per-type validation. This general regex is useful for quick format checks that accept either type.
> **D-035 deviation note:** D-035 specifies `{template}_{d|m|e}_{###}`. This spec refines that to `{location-slug}_{d|m}_{###}` (dialogue) and `{location-slug}_m_{s|d}_{###}` (monologue) for two reasons: (1) location-slug is more precise than template name and matches the directory hierarchy, and (2) monologue IDs include a character segment (`s`/`d`) to ensure uniqueness across the hard character partition (D-032). All existing authored content already uses this refined format. D-035 should be updated to reflect the implemented convention.
### 5.2 Regex patterns
+37 -5
View File
@@ -21,20 +21,34 @@ use crate::content::{ContentConfig, ContentStoreResource, LinePoolIndexResource}
/// At 10 tps (D-031), 20 ticks = 2 seconds.
const CHECK_INTERVAL_TICKS: u64 = 20;
/// Consecutive reload failures before escalating to a warning.
const FAILURE_WARN_THRESHOLD: u32 = 5;
/// Resource tracking content file timestamps for change detection.
#[derive(Resource, Debug)]
pub struct ContentWatcher {
file_timestamps: BTreeMap<PathBuf, SystemTime>,
ticks_since_check: u64,
/// Consecutive reload failures. Resets on success.
consecutive_failures: u32,
}
impl ContentWatcher {
/// Create a new watcher and perform initial timestamp scan.
/// Returns a watcher with no tracked files if content_root is invalid.
pub fn new(content_root: &Path) -> Self {
let mut watcher = Self {
file_timestamps: BTreeMap::new(),
ticks_since_check: 0,
consecutive_failures: 0,
};
if content_root.as_os_str().is_empty() || !content_root.is_dir() {
tracing::warn!(
"ContentWatcher: invalid content root {:?}, hot-reload disabled",
content_root,
);
return watcher;
}
watcher.scan(content_root);
watcher
}
@@ -42,7 +56,7 @@ impl ContentWatcher {
/// Scan content directory tree and record all YAML file timestamps.
fn scan(&mut self, content_root: &Path) {
self.file_timestamps.clear();
walk_yaml(content_root, &mut self.file_timestamps);
walk_yaml(content_root, &mut self.file_timestamps, 0);
tracing::debug!(
"ContentWatcher: tracking {} content files",
self.file_timestamps.len()
@@ -52,7 +66,7 @@ impl ContentWatcher {
/// Check for changes and rescan. Returns true if any files changed.
fn check_and_rescan(&mut self, content_root: &Path) -> bool {
let mut new_timestamps = BTreeMap::new();
walk_yaml(content_root, &mut new_timestamps);
walk_yaml(content_root, &mut new_timestamps, 0);
let changed = new_timestamps != self.file_timestamps;
if changed {
self.file_timestamps = new_timestamps;
@@ -66,15 +80,23 @@ impl ContentWatcher {
}
}
/// Maximum recursion depth for directory walking (guards against symlink loops).
const MAX_WALK_DEPTH: usize = 100;
/// Recursively walk a directory, recording .yaml file modification timestamps.
fn walk_yaml(dir: &Path, timestamps: &mut BTreeMap<PathBuf, SystemTime>) {
/// Stops recursing at MAX_WALK_DEPTH to guard against symlink loops.
fn walk_yaml(dir: &Path, timestamps: &mut BTreeMap<PathBuf, SystemTime>, depth: usize) {
if depth >= MAX_WALK_DEPTH {
tracing::warn!("walk_yaml: max depth {} reached at {:?}, stopping", MAX_WALK_DEPTH, dir);
return;
}
let Ok(entries) = std::fs::read_dir(dir) else {
return;
};
for entry in entries.filter_map(|e| e.ok()) {
let path = entry.path();
if path.is_dir() {
walk_yaml(&path, timestamps);
walk_yaml(&path, timestamps, depth + 1);
} else if path.extension().and_then(|e| e.to_str()) == Some("yaml") {
if let Ok(meta) = std::fs::metadata(&path) {
if let Ok(modified) = meta.modified() {
@@ -124,6 +146,7 @@ pub fn hot_reload_content(
let m_count = index.monologue_line_count();
store_res.0 = store;
index_res.0 = index;
watcher.consecutive_failures = 0;
tracing::info!(
"Content hot-reloaded: {} dialogue lines, {} monologue lines",
d_count,
@@ -131,7 +154,16 @@ pub fn hot_reload_content(
);
}
Err(e) => {
tracing::warn!("Content hot-reload failed (keeping previous): {}", e);
watcher.consecutive_failures += 1;
if watcher.consecutive_failures >= FAILURE_WARN_THRESHOLD {
tracing::warn!(
"Content hot-reload failed {} consecutive times (keeping previous): {}",
watcher.consecutive_failures,
e,
);
} else {
tracing::warn!("Content hot-reload failed (keeping previous): {}", e);
}
}
}
}
+132 -2
View File
@@ -349,6 +349,14 @@ impl LinePoolIndex {
}
fn index_dialogue_pool(&mut self, pool: &types::DialoguePool) {
if pool.location.is_empty() {
tracing::warn!(
"Skipping dialogue pool with empty location (role={})",
pool.role,
);
return;
}
let key = (pool.location.clone(), pool.role.clone());
let indexed_lines: Vec<IndexedDialogueLine> =
@@ -366,6 +374,14 @@ impl LinePoolIndex {
}
fn index_monologue_pool(&mut self, pool: &types::MonologuePool) {
if pool.location.is_empty() {
tracing::warn!(
"Skipping monologue pool with empty location (character={})",
pool.character,
);
return;
}
let character = match pool.character.parse::<Character>() {
Ok(c) => c,
Err(e) => {
@@ -534,8 +550,28 @@ fn parse_dialogue_line(line: &types::DialogueLine) -> Option<IndexedDialogueLine
return None;
}
let topic: Vec<Topic> = line.topic.iter().filter_map(|s| s.parse().ok()).collect();
let mood: Vec<Mood> = line.mood.iter().filter_map(|s| s.parse().ok()).collect();
let topic: Vec<Topic> = line
.topic
.iter()
.filter_map(|s| {
s.parse()
.map_err(|e: ParseEnumError| {
tracing::warn!("Line {}: {}", line.id, e);
})
.ok()
})
.collect();
let mood: Vec<Mood> = line
.mood
.iter()
.filter_map(|s| {
s.parse()
.map_err(|e: ParseEnumError| {
tracing::warn!("Line {}: {}", line.id, e);
})
.ok()
})
.collect();
Some(IndexedDialogueLine {
id: line.id.clone(),
@@ -1009,6 +1045,100 @@ mod tests {
assert!(results.is_empty());
}
#[test]
fn query_monologue_line_with_prerequisite_excluded_when_fact_absent() {
// H10: Verify prerequisite field is populated through query so callers
// can filter. query_monologue returns ALL matching lines (prerequisite
// evaluation is caller's responsibility per D-028), but a line with a
// prerequisite should carry that data through for the caller to check.
let mut store = ContentStore::default();
let monologue_pools = vec![types::MonologuePool {
character: "smuggler".to_string(),
location: "the-terminal".to_string(),
lines: vec![
// Line WITHOUT prerequisite — should always be available
types::MonologueLine {
id: "prereq_none".to_string(),
text: "No prereq line".to_string(),
trigger: "enter_location".to_string(),
prerequisites: None,
priority: Some(5),
cooldown: None,
tags: vec![],
},
// Line WITH prerequisite — caller must check before using
types::MonologueLine {
id: "prereq_fact".to_string(),
text: "Requires cargo_manifest_seen".to_string(),
trigger: "enter_location".to_string(),
prerequisites: Some(types::Prerequisites {
facts: vec![types::FactPrerequisite {
fact_id: "cargo_manifest_seen".to_string(),
min_confidence: "confirmed".to_string(),
}],
entity_attributes: vec![],
relationship: None,
}),
priority: Some(8),
cooldown: None,
tags: vec![],
},
],
}];
let district = DistrictContent {
monologue_pools,
..Default::default()
};
store
.districts
.insert("test.district".to_string(), district);
let index = LinePoolIndex::build(&store);
let results =
index.query_monologue(Character::Smuggler, "the-terminal", Trigger::EnterLocation);
// Both lines are returned (query doesn't filter prerequisites)
assert_eq!(results.len(), 2);
// Verify the prerequisite-bearing line carries its prerequisites through
let prereq_line = results.iter().find(|l| l.id == "prereq_fact").unwrap();
assert!(
prereq_line.prerequisites.is_some(),
"prerequisite field should be populated for caller to evaluate"
);
let prereqs = prereq_line.prerequisites.as_ref().unwrap();
assert_eq!(prereqs.facts.len(), 1);
assert_eq!(prereqs.facts[0].fact_id, "cargo_manifest_seen");
// The no-prerequisite line should have None
let no_prereq_line = results.iter().find(|l| l.id == "prereq_none").unwrap();
assert!(
no_prereq_line.prerequisites.is_none(),
"line without prerequisites should have None"
);
// Simulate caller-side filtering: if fact is absent, exclude the line
let player_known_facts: Vec<&str> = vec![]; // empty — fact not known
let available: Vec<_> = results
.iter()
.filter(|line| {
match &line.prerequisites {
None => true, // no prerequisites = always available
Some(prereqs) => prereqs
.facts
.iter()
.all(|f| player_known_facts.contains(&f.fact_id.as_str())),
}
})
.collect();
assert_eq!(
available.len(),
1,
"only the no-prerequisite line should pass when fact is absent"
);
assert_eq!(available[0].id, "prereq_none");
}
// -- Parse edge cases ----------------------------------------------------
#[test]
+5 -2
View File
@@ -93,8 +93,11 @@ fn discover_districts(campaign_path: &Path) -> Vec<PathBuf> {
if systems_path.is_dir() {
walk_for_districts(&systems_path, &mut districts);
}
// BTreeMap ordering guarantees deterministic district processing,
// but sort the discovery order too for consistency.
// Discovery order from fs::read_dir is platform-dependent. Sort the Vec
// here so districts load in a deterministic order regardless of OS.
// ContentStore.districts uses BTreeMap for deterministic *iteration* later,
// but sorted discovery ensures deterministic *load* order (and thus
// deterministic ID derivation and log output).
districts.sort();
districts
}
+2 -5
View File
@@ -26,11 +26,8 @@ pub struct RelationshipEdge {
/// BTreeMap<(subject, target), edge> for deterministic iteration (D-010).
/// Directed graph: edge (A, B) represents how A feels about B.
///
/// TODO(v0.2): This is a global omniscient resource — all entities share one
/// graph. This violates information boundaries (D-009/D-010) because any
/// system can read any relationship. For multiplayer, this needs per-observer
/// projection so each entity only sees relationships they should know about.
/// Acceptable for v0.1 single-player where the server is authoritative.
/// TODO(v0.2): RelationshipGraph is per-world. Multiplayer needs per-observer
/// relationship views (D-010).
#[derive(Resource, Debug, Clone, Default, Serialize, Deserialize)]
pub struct RelationshipGraph {
edges: BTreeMap<(StableId, StableId), RelationshipEdge>,
+14
View File
@@ -19,10 +19,22 @@ use crate::simulation::time::SimulationTime;
/// Base cognitive delay: 0.6 seconds = 6 ticks at 10 tps (D-031, D-060).
/// Tunable: expect playtesting adjustments.
///
/// Playtesting expectation: 0.6s should feel like a brief "processing" beat —
/// noticeable enough that new entities register as grey blobs before resolving,
/// but short enough to not feel sluggish. If playtesters report recognition
/// feels instant (reduce to test), or laggy (current value may be too high for
/// fast-paced encounters), adjust in 2-tick increments. The 2:1 ratio with
/// URGENT_DELAY_TICKS should be preserved.
pub const NORMAL_DELAY_TICKS: u64 = 6;
/// Urgent cognitive delay: 0.3 seconds = 3 ticks at 10 tps (D-031, D-060).
/// Triggered when observe_anomaly context is active.
///
/// Playtesting expectation: 0.3s should feel nearly instant but still register
/// visually as a "snap to attention" moment. If playtesters don't notice the
/// delay at all, consider whether the grey blob phase is too brief to read.
/// Must remain strictly less than NORMAL_DELAY_TICKS.
pub const URGENT_DELAY_TICKS: u64 = 3;
/// How the recognition was triggered, determines delay duration.
@@ -76,6 +88,8 @@ impl CognitiveDelay {
}
/// Check if an entity is already pending recognition.
// Note: O(n) scan over pending vec. Fine for v0.1 (typically <10 pending).
// If NPC cognitive delay is added, consider HashSet<StableId> index.
pub fn is_pending(&self, stable_id: &StableId) -> bool {
self.pending.iter().any(|p| p.stable_id == *stable_id)
}
+4 -3
View File
@@ -78,7 +78,7 @@ pub fn emit_observation_events(
} else if let Some(ref mut delay) = cognitive_delay {
// New entity + cognitive delay available: buffer recognition
if !delay.is_pending(&stable_id) {
// TODO: wire RecognitionTrigger::Urgent for observe_anomaly triggers
// TODO(#450): wire RecognitionTrigger::Urgent for observe_anomaly triggers
let trigger = RecognitionTrigger::Normal;
delay.push(PendingRecognition {
target: entity,
@@ -135,8 +135,9 @@ pub fn emit_observation_events(
delay.pending().iter().map(|p| p.stable_id).collect();
for sid in pending_ids {
if !visible_stable_ids.contains(&sid.0) {
delay.cancel(&sid);
tracing::debug!("Cognitive delay cancelled: stable_id={} left LOS", sid.0);
if delay.cancel(&sid).is_some() {
tracing::debug!("Cognitive delay cancelled: stable_id={} left LOS", sid.0);
}
}
}
}
+98
View File
@@ -1817,3 +1817,101 @@ fn sprint_anomaly_multiple_contradicted_npcs_only_first_queued() {
let queue = query.single(&world).unwrap();
assert!(queue.has_pending(), "one anomaly should be queued");
}
// -----------------------------------------------------------------------
// Pending recognitions in observer snapshot (#423, D-060)
// -----------------------------------------------------------------------
#[test]
fn pending_recognitions_appear_in_snapshot() {
// H11: When a player entity has a CognitiveDelay component with pending
// recognitions, compute_observer_snapshot should include them in
// pending_recognitions for the client to render as grey blobs.
use crate::perception::cognitive_delay::{
CognitiveDelay, PendingRecognition, RecognitionTrigger,
};
let mut world = setup_world(32, 32);
let mut registry = EntityRegistry::new(0);
// Target entity that is being "recognized"
let target = world.spawn_empty().id();
let target_sid = registry.register(target);
// Player with CognitiveDelay containing a pending recognition
let mut cd = CognitiveDelay::default();
cd.push(PendingRecognition {
target,
stable_id: target_sid,
position: TilePosition::new(16, 14, 0),
delay_until_tick: 110, // will complete at tick 110
trigger: RecognitionTrigger::Normal,
});
let player = world
.spawn((
PlayerCharacter,
TilePosition::new(16, 16, 0),
Facing::default(),
KnowledgeGraph::new(),
NearbyInteractionBuffer::default(),
MonologueBuffer::default(),
cd,
))
.id();
registry.register(player);
world.insert_resource(registry);
world.insert_resource({
let mut t = SimulationTime::default();
t.tick = 106; // 4 ticks remaining until recognition
t
});
run_observer_pipeline(&mut world);
let buffer = world.resource::<SnapshotBuffer>();
let snapshot = buffer.snapshot.as_ref().expect("snapshot should exist");
assert_eq!(
snapshot.pending_recognitions.len(),
1,
"should have one pending recognition in snapshot"
);
let pending = &snapshot.pending_recognitions[0];
assert_eq!(pending.entity_id, target_sid.0);
assert_eq!(pending.remaining_ticks, 4, "110 - 106 = 4 remaining");
assert_eq!(
pending.total_delay_ticks,
crate::perception::cognitive_delay::NORMAL_DELAY_TICKS,
"total delay should match Normal trigger"
);
// Position should be render coords of (16, 14, 0)
let (expected_x, expected_y, expected_z) = TilePosition::new(16, 14, 0).to_render_coords();
assert_eq!(pending.x, expected_x);
assert_eq!(pending.y, expected_y);
assert_eq!(pending.z, expected_z);
}
#[test]
fn no_cognitive_delay_component_means_empty_pending_recognitions() {
// H11 complement: player WITHOUT CognitiveDelay should produce
// an empty pending_recognitions vec (backward compatibility).
let mut world = setup_world(32, 32);
world.spawn((
PlayerCharacter,
TilePosition::new(16, 16, 0),
Facing::default(),
KnowledgeGraph::new(),
NearbyInteractionBuffer::default(),
MonologueBuffer::default(),
));
run_observer_pipeline(&mut world);
let buffer = world.resource::<SnapshotBuffer>();
let snapshot = buffer.snapshot.as_ref().unwrap();
assert!(
snapshot.pending_recognitions.is_empty(),
"no CognitiveDelay component should produce empty pending_recognitions"
);
}
+13
View File
@@ -14,6 +14,10 @@ use crate::simulation::time::{SimulationTime, TickRate};
use bevy_ecs::prelude::*;
use std::collections::VecDeque;
/// Maximum number of inputs the queue will hold before dropping oldest.
/// Prevents unbounded memory growth from input flooding.
pub const INPUT_QUEUE_CAPACITY: usize = 1000;
/// Queue of pending player inputs, ordered by tick
#[derive(Resource, Debug, Default)]
pub struct InputQueue {
@@ -24,6 +28,7 @@ impl InputQueue {
/// Add a new input to the queue.
/// Inputs must be pushed in tick order for deterministic processing.
/// Panics in debug builds if tick ordering is violated.
/// Drops oldest inputs when capacity is exceeded.
pub fn push(&mut self, input: PlayerInput) {
debug_assert!(
self.queue.back().is_none_or(|last| last.tick <= input.tick),
@@ -31,6 +36,14 @@ impl InputQueue {
self.queue.back().map_or(0, |last| last.tick),
input.tick,
);
if self.queue.len() >= INPUT_QUEUE_CAPACITY {
let dropped = self.queue.pop_front();
tracing::warn!(
"InputQueue at capacity ({}), dropping oldest input (tick={})",
INPUT_QUEUE_CAPACITY,
dropped.map_or(0, |d| d.tick),
);
}
self.queue.push_back(input);
}
+24
View File
@@ -91,6 +91,13 @@ pub fn update_listening_focus(
// Sprint stance: stationary but too high-alert to listen
let Some(threshold) = ListeningFocus::threshold_for_stance(stance) else {
if focus.eavesdrop_target.is_some() || focus.stationary_ticks > 0 {
tracing::debug!(
"Sprint stance resets eavesdrop: stationary_ticks={}, had_target={}",
focus.stationary_ticks,
focus.eavesdrop_target.is_some(),
);
}
focus.stationary_ticks = 0;
focus.eavesdrop_target = None;
continue;
@@ -426,6 +433,23 @@ mod tests {
assert_eq!(focus.stationary_ticks, EAVESDROP_THRESHOLD);
}
// -----------------------------------------------------------------------
// Constant invariants
// -----------------------------------------------------------------------
#[test]
fn eavesdrop_threshold_careful_less_than_normal() {
// T4: The careful threshold MUST be strictly less than the normal
// threshold — careful stance rewards patience with faster eavesdrop
// activation (D-053, D-018).
assert!(
EAVESDROP_THRESHOLD_CAREFUL < EAVESDROP_THRESHOLD,
"EAVESDROP_THRESHOLD_CAREFUL ({}) must be < EAVESDROP_THRESHOLD ({})",
EAVESDROP_THRESHOLD_CAREFUL,
EAVESDROP_THRESHOLD,
);
}
// -----------------------------------------------------------------------
// Edge cases
// -----------------------------------------------------------------------
+37
View File
@@ -480,6 +480,43 @@ fn snapshot_v6_full_inventory_roundtrip() {
assert_eq!(decoded.player_inventory[8].slot, 8);
}
/// PendingRecognitionWire round-trips through MessagePack (#423, D-060).
/// Guards against cognitive delay wire data corruption during serialization.
#[test]
fn pending_recognition_wire_roundtrip() {
let mut snapshot = test_snapshot(0, vec![]);
snapshot.pending_recognitions = vec![
PendingRecognitionWire {
entity_id: 42,
x: 10.5,
y: 20.0,
z: 0,
remaining_ticks: 4,
total_delay_ticks: 6,
},
PendingRecognitionWire {
entity_id: 99,
x: 15.0,
y: 8.5,
z: 1,
remaining_ticks: 1,
total_delay_ticks: 3,
},
];
let bytes = rmp_serde::to_vec_named(&snapshot).expect("serialize");
let decoded: ObserverSnapshot = rmp_serde::from_slice(&bytes).expect("deserialize");
assert_eq!(decoded.pending_recognitions.len(), 2);
assert_eq!(decoded.pending_recognitions[0].entity_id, 42);
assert_eq!(decoded.pending_recognitions[0].remaining_ticks, 4);
assert_eq!(decoded.pending_recognitions[0].total_delay_ticks, 6);
assert!((decoded.pending_recognitions[0].x - 10.5).abs() < f32::EPSILON);
assert_eq!(decoded.pending_recognitions[1].entity_id, 99);
assert_eq!(decoded.pending_recognitions[1].z, 1);
assert_eq!(decoded.pending_recognitions[1].total_delay_ticks, 3);
}
/// All VerbKind variants must survive MessagePack round-trip (#421, D-057).
/// Guards against serde mapping breakage when new verbs are added.
#[test]