refactor(simulation): PR #137 review — audit override + regression tests + docs
Addresses Tyre, Hoshe, and lead review comments on PR #137: - **Audit doc amendment** (Tyre E1 / Hoshe H1 / Lead): add "Lead override (2026-04-21)" section at top of docs/architecture/sprint-37-878-audit.md. Rewrites the conclusion to "DECISION: STRIP" with the cascade-based rationale. Preserves the original audit body as the pre-override record. - **Regression tests** (Lead 2a-2b / Hoshe H2 / H3): add POSITIVE assertions of the new uniform behavior so silent reintroduction fails. - `phase2_container_verb_labels_uniform_regardless_of_player_state` — two trials (empty KG, POI-bearing KG) assert container verb labels equal Phase-1 defaults. - `monologue_pool_selection_uniform_no_archetype_key` — two observers with divergent MonologueState both draw from OBSERVE_NPC_LINES. - **Decision record amendments** (Lead 3 / Tyre S2): D-032, D-035, and D-057 amended with Phase 6 deferral wording. "Retired pending Phase 6, not deferred with scaffolding." Reintroduction gate: a confirmed Phase 6 character-model design. - **types.rs doc fixes** (Tyre S1 / Hoshe H5): StartupMessage protocol- flow comment updated to reflect no-version handshake (D-192). ObserverSnapshot version-history block grows a "Sprint 37 wire-format shifts" section documenting D-192 + #878 schema drops. - **observer/tests.rs:944 comment** (Hoshe H6): rewritten to cite cascade rationale instead of the stale D-032-SUPERSEDED premise. - **tests/run-atlas-determinism exit** (Hoshe H7): exit 0 when EXIT_CODE=2 (venv/DB missing = skip, not fail). Preserves skip semantics for tests/run-all on machines without the Python venv. Follow-up tickets filed: - #895 (server, low): expand check-systems-db-stamp GENERATOR_SOURCES to cover gemma_naming.py + naming_core.py (Tyre S3). - #896 (planning, low): add CLAUDE.md carveout for server wiki writes closing coverage gates (Tyre S4 / Hoshe H8). H4 investigation: v01_integration_playthrough.rs was not the only E2E handshake→tick→snapshot test; coverage preserved by bridge_ipc.rs, bridge_tcp.rs, and game_loop.rs (the latter is pre-existing-broken per #885). No replacement test needed. 1142/1142 lib tests pass. cargo clippy -- -D warnings clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.6
@@ -23,12 +23,11 @@ pub struct HandshakeMessage {}
|
||||
/// Startup message sent by the client after receiving HandshakeMessage (#175).
|
||||
/// Contains the world seed for deterministic simulation (D-010, D-029).
|
||||
///
|
||||
/// Protocol flow:
|
||||
/// Protocol flow (D-192: no version field, no validation step):
|
||||
/// 1. Server sends HandshakeMessage (server → client)
|
||||
/// 2. Client validates protocol_version
|
||||
/// 3. Client sends StartupMessage (client → server)
|
||||
/// 4. Server reads world_seed, initializes SimRng
|
||||
/// 5. Normal tick loop begins
|
||||
/// 2. Client sends StartupMessage (client → server)
|
||||
/// 3. Server reads world_seed, initializes SimRng
|
||||
/// 4. Normal tick loop begins
|
||||
///
|
||||
/// Wire format: MessagePack, same 4-byte length-prefixed framing.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
|
||||
@@ -65,13 +64,21 @@ pub struct StartupMessage {
|
||||
/// sim_errors (#85, structured error reporting to client).
|
||||
/// v18 adds: debug_response (#580, debug console server — command/response wire).
|
||||
/// v19 adds: current_ticker (#591). (character_archetype on StartupMessage was
|
||||
/// added in #587 and removed in Sprint 37 per D-032 purge / cascade cleanup.)
|
||||
/// added in #587 and removed in Sprint 37 — see "Sprint 37 wire-format shifts" below.)
|
||||
/// v20 adds: settings_response (#627, SQLite settings IPC).
|
||||
/// v21 adds: economy_snapshot (#822, D-181 7-signal snapshot per queried system),
|
||||
/// EconStateQuery PlayerAction variant (#822).
|
||||
/// v22 adds: bookmark_catalog (#614, D-115/D-117 CK3-style bookmark system),
|
||||
/// RequestBookmarkCatalog + ConfirmBookmark PlayerAction variants (#614).
|
||||
/// v23 removes: conversation_events, conversation_ended (D-078 scrapped per R-012).
|
||||
/// Sprint 37 wire-format shifts (no snapshot-version bump needed — PROTOCOL_VERSION lockstep gone):
|
||||
/// - D-192 (#874): `PROTOCOL_VERSION` field removed from `HandshakeMessage`. The
|
||||
/// handshake is now an empty marker ("server ready"); there is no negotiated
|
||||
/// version field on the wire. Genuine schema drift surfaces as MessagePack
|
||||
/// missing-field errors downstream — that is the intended signal per D-192.
|
||||
/// - #878 (cascade cleanup): `character_archetype` field removed from
|
||||
/// `StartupMessage`. This protocol break rides the D-192 drop.
|
||||
///
|
||||
/// Future fields: ambient sound events, HUD state (D-020 expansion).
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct ObserverSnapshot {
|
||||
|
||||
@@ -941,7 +941,11 @@ fn phase2_no_contradiction_for_active_knowledge() {
|
||||
|
||||
#[test]
|
||||
fn phase2_non_container_keeps_default_labels() {
|
||||
// Readable objects keep their default labels (post-archetype cleanup, D-032 SUPERSEDED).
|
||||
// Readable objects keep their Phase 1 verb labels unchanged through Phase 2.
|
||||
// Regression guard: archetype-verb differentiation is Phase 6 detail — not present
|
||||
// in the current server per the development cascade (CLAUDE.md). D-057 superseded.
|
||||
// If this test fails, a character-class relabelling branch was reintroduced before
|
||||
// Phase 6 scope is confirmed by the team lead.
|
||||
let mut world = setup_world(32, 32);
|
||||
let mut registry = EntityRegistry::new(0);
|
||||
|
||||
@@ -2596,3 +2600,124 @@ fn tell_state_none_when_npc_has_no_derived_tell_component() {
|
||||
"NPC without DerivedTellState component should have tell_state = None"
|
||||
);
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// Regression: Phase 2 container verb labels are uniform (D-057 / #878)
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
#[test]
|
||||
fn phase2_container_verb_labels_uniform_regardless_of_player_state() {
|
||||
// Regression guard (D-057 superseded, #878 cascade purge):
|
||||
// apply_phase2_verb_filter no longer has an archetype branch that relabels
|
||||
// container verbs. Labels must be the Phase-1 defaults — "Open", "Search",
|
||||
// "Observe" — regardless of the observer's KnowledgeGraph contents or
|
||||
// relationship state with other entities.
|
||||
//
|
||||
// This test FAILS if a character-class verb-label branch is reintroduced
|
||||
// without a confirmed Phase 6 scope decision from the team lead.
|
||||
|
||||
// --- Trial A: empty KnowledgeGraph ---
|
||||
{
|
||||
let mut world = setup_world(32, 32);
|
||||
let mut registry = EntityRegistry::new(0);
|
||||
|
||||
let container = world
|
||||
.spawn((
|
||||
TilePosition::new(16, 15, 0),
|
||||
crate::simulation::interaction::Interactable,
|
||||
ObjectType::Container,
|
||||
))
|
||||
.id();
|
||||
registry.register(container);
|
||||
|
||||
world.spawn((
|
||||
PlayerCharacter,
|
||||
TilePosition::new(16, 16, 0),
|
||||
Facing(FacingDirection::North),
|
||||
KnowledgeGraph::new(),
|
||||
NearbyInteractionBuffer::default(),
|
||||
MonologueBuffer::default(),
|
||||
));
|
||||
world.insert_resource(registry);
|
||||
|
||||
run_full_pipeline(&mut world);
|
||||
|
||||
let buffer = world.resource::<SnapshotBuffer>();
|
||||
let snapshot = buffer.snapshot.as_ref().unwrap();
|
||||
assert_eq!(snapshot.nearby_interactions.len(), 1);
|
||||
let labels: Vec<&str> = snapshot.nearby_interactions[0]
|
||||
.verbs
|
||||
.iter()
|
||||
.map(|v| v.label.as_str())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
labels,
|
||||
["Open", "Search", "Observe"],
|
||||
"Trial A (empty KG): container verb labels must equal Phase-1 defaults"
|
||||
);
|
||||
}
|
||||
|
||||
// --- Trial B: KG with PersonOfInterest NPC nearby ---
|
||||
// Player has a non-trivial knowledge state; container labels must still be
|
||||
// the Phase-1 defaults — Phase 2 NPC-specific logic must not bleed into
|
||||
// ObjectType::Container interactions.
|
||||
{
|
||||
let mut world = setup_world(32, 32);
|
||||
let mut registry = EntityRegistry::new(0);
|
||||
|
||||
let container = world
|
||||
.spawn((
|
||||
TilePosition::new(16, 15, 0),
|
||||
crate::simulation::interaction::Interactable,
|
||||
ObjectType::Container,
|
||||
))
|
||||
.id();
|
||||
registry.register(container);
|
||||
|
||||
// NPC out of close-range so Confront is not injected; still in KG as POI.
|
||||
let npc = world
|
||||
.spawn((
|
||||
crate::npc::Npc,
|
||||
TilePosition::new(16, 13, 0),
|
||||
crate::simulation::interaction::Interactable,
|
||||
))
|
||||
.id();
|
||||
let npc_sid = registry.register(npc);
|
||||
|
||||
let mut kg = KnowledgeGraph::new();
|
||||
kg.observe_entity(npc_sid, TilePosition::new(16, 13, 0), 5);
|
||||
kg.set_relationship(&npc_sid, RelationshipState::PersonOfInterest);
|
||||
|
||||
world.spawn((
|
||||
PlayerCharacter,
|
||||
TilePosition::new(16, 16, 0),
|
||||
Facing(FacingDirection::North),
|
||||
kg,
|
||||
NearbyInteractionBuffer::default(),
|
||||
MonologueBuffer::default(),
|
||||
));
|
||||
world.insert_resource(registry);
|
||||
|
||||
run_full_pipeline(&mut world);
|
||||
|
||||
let buffer = world.resource::<SnapshotBuffer>();
|
||||
let snapshot = buffer.snapshot.as_ref().unwrap();
|
||||
|
||||
let container_interaction = snapshot
|
||||
.nearby_interactions
|
||||
.iter()
|
||||
.find(|i| i.object_type == Some(ObjectType::Container))
|
||||
.expect("container interaction must be present");
|
||||
|
||||
let labels: Vec<&str> = container_interaction
|
||||
.verbs
|
||||
.iter()
|
||||
.map(|v| v.label.as_str())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
labels,
|
||||
["Open", "Search", "Observe"],
|
||||
"Trial B (POI NPC in KG): container verb labels must equal Phase-1 defaults"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2125,4 +2125,103 @@ mod tests {
|
||||
"queue should be empty after monologue consumed the event"
|
||||
);
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// Regression: monologue pool selection is uniform (D-032 / #878)
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
#[test]
|
||||
fn monologue_pool_selection_uniform_no_archetype_key() {
|
||||
// Regression guard (D-032 cascade purge, #878):
|
||||
// trigger_event_monologue previously partitioned pool selection by
|
||||
// CharacterArchetype key (MonologueState.character). That field is gone.
|
||||
// Pool selection is now by trigger string only — "observe_npc", "hear_sound",
|
||||
// "post_conversation" — and the line IDs are drawn exclusively from the
|
||||
// corresponding hardcoded constant (OBSERVE_NPC_LINES et al.).
|
||||
//
|
||||
// This test asserts the POSITIVE behaviour: an observe_npc trigger always
|
||||
// produces a line whose ID begins with "observe_npc_", regardless of any
|
||||
// additional observer state. It FAILS if an archetype-keyed dispatch path
|
||||
// is reintroduced (which would produce IDs outside that prefix or panic on
|
||||
// a missing archetype field).
|
||||
|
||||
// Known line IDs from OBSERVE_NPC_LINES (compile-checked below).
|
||||
const VALID_OBSERVE_NPC_IDS: &[&str] =
|
||||
&["observe_npc_01", "observe_npc_02", "observe_npc_03"];
|
||||
|
||||
// --- Observer A: minimal state (no extra components) ---
|
||||
let line_a = {
|
||||
let mut world = setup_event_world();
|
||||
let player = spawn_event_player(&mut world);
|
||||
|
||||
world
|
||||
.resource_mut::<ObservationEventQueue>()
|
||||
.push(ObservationEvent {
|
||||
tick: 1,
|
||||
trigger: ObservationTrigger::NewEntity {
|
||||
entity: StableId(10),
|
||||
location: TilePosition::new(12, 12, 0),
|
||||
},
|
||||
observer: player,
|
||||
});
|
||||
|
||||
run_event_system(&mut world);
|
||||
|
||||
let buf = world.get::<MonologueBuffer>(player).unwrap();
|
||||
buf.event
|
||||
.as_ref()
|
||||
.expect("observe_npc trigger must fire a monologue")
|
||||
.id
|
||||
.clone()
|
||||
};
|
||||
|
||||
// --- Observer B: player has heard a previous sound (last_fired_tick set) ---
|
||||
// Simulates a player with non-default MonologueState — the pool key must
|
||||
// still resolve to OBSERVE_NPC_LINES, not an archetype-partitioned variant.
|
||||
let line_b = {
|
||||
let mut world = setup_event_world();
|
||||
let player = spawn_event_player(&mut world);
|
||||
|
||||
// Pre-populate state to exercise a non-fresh observer
|
||||
world.resource_mut::<SimulationTime>().tick = 10;
|
||||
{
|
||||
let mut state = world.get_mut::<MonologueState>(player).unwrap();
|
||||
state.last_fired_tick = 3;
|
||||
}
|
||||
|
||||
world
|
||||
.resource_mut::<ObservationEventQueue>()
|
||||
.push(ObservationEvent {
|
||||
tick: 5,
|
||||
trigger: ObservationTrigger::NewEntity {
|
||||
entity: StableId(20),
|
||||
location: TilePosition::new(14, 14, 0),
|
||||
},
|
||||
observer: player,
|
||||
});
|
||||
|
||||
run_event_system(&mut world);
|
||||
|
||||
let buf = world.get::<MonologueBuffer>(player).unwrap();
|
||||
buf.event
|
||||
.as_ref()
|
||||
.expect("observe_npc trigger must fire for observer B")
|
||||
.id
|
||||
.clone()
|
||||
};
|
||||
|
||||
// Both observers must produce IDs from the unified observe_npc pool.
|
||||
assert!(
|
||||
VALID_OBSERVE_NPC_IDS.contains(&line_a.as_str()),
|
||||
"Observer A line_id '{}' is not from OBSERVE_NPC_LINES — \
|
||||
archetype-keyed pool dispatch may have been reintroduced",
|
||||
line_a
|
||||
);
|
||||
assert!(
|
||||
VALID_OBSERVE_NPC_IDS.contains(&line_b.as_str()),
|
||||
"Observer B line_id '{}' is not from OBSERVE_NPC_LINES — \
|
||||
archetype-keyed pool dispatch may have been reintroduced",
|
||||
line_b
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user