fix(server): PR #132 review round 2 — 14 actionable comments addressed

Blockers (4):
- Wire cargo deny check into pre-pr-server (was dead config) (#1)
- ConfirmBookmark idempotency guard: SimError ProtocolError on retry (#2)
- D-080 amendment: transfer_npc_knowledge retained-but-dormant honest doc (#3)
- SelectedBookmark v0.2 transient scope; save/load deferred to #863 (#4)

Issues (8):
- ConfirmBookmark validation tests: unknown id, invalid location,
  valid path, double-confirm guard (#5)
- snapshot_with_bookmark_catalog fixture for client #618 decode tests (#6)
- generate_brands: replace 5 raw .unwrap() with eprintln+exit pattern (#7)
- npc_knowledge_transfer.rs: stale run_npc_conversations refs cleaned (#8)
- monologue.rs: residual D-078 "overheard conversations" doc removed (#9)
- 5 test files: orphan blank lines from removed conversation_* fields (#10)
- BookmarkCatalog: add PartialEq, Eq derives (matches sibling) (#11)

Nits (2):
- culture_tag doc: describe BookmarkRegistry::build_catalog behavior,
  remove "until #679 lands" placeholder (#13)
- generate_brands seed=1 canonical comment (#14)

Follow-ups filed:
- #862 — BookmarkPlugin::new(registry) injection (#12 deferred)
- #863 — Wire SelectedBookmark into SaveState (Sprint 37)

Pre-pr-server: fmt clean, clippy clean, deny clean, build clean.
nextest save_io failures pre-existing parallelism issue (sequential
cargo test --lib passes 20/20).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-04-19 18:04:46 +02:00
co-authored by Claude Opus 4.7
parent e86e53ec06
commit 13530910c9
14 changed files with 275 additions and 32 deletions
+7 -3
View File
@@ -3,7 +3,7 @@ GODOT := $(shell command -v godot4 2>/dev/null || command -v godot 2>/dev/null)
.PHONY: help setup build check-protocol client server game stop test lint lint-python setup-venv ci ci-client ci-server clean \
decisions-sync decisions-coverage decisions-active decisions-orphan \
db-backup db-install validate-content check-fact-ids setup-hooks \
audit atlas-verify economy-db atlas-generate \
audit deny atlas-verify economy-db atlas-generate \
pre-pr pre-pr-lint pre-pr-build pre-pr-test pre-pr-validate pre-pr-fixtures \
pre-pr-server pre-pr-client pre-pr-content \
fixtures-client fixtures-gauntlet golden-diff golden-update \
@@ -50,6 +50,7 @@ help:
@echo " make decisions-active List active decisions"
@echo " make decisions-orphan Decisions without implementing tickets"
@echo " make audit Run cargo audit (security advisory check)"
@echo " make deny Run cargo deny check (license/ban policy)"
@echo " make validate-content Validate content YAML against schemas"
@echo " make check-fact-ids Check fact_id references against knowledge catalogs"
@echo " make atlas-verify Verify atlas proposal JSONs (all in docs/atlas/proposals/)"
@@ -249,7 +250,7 @@ lint-client:
# --- Pre-PR verification ---
pre-pr: pre-pr-lint pre-pr-build pre-pr-test pre-pr-validate pre-pr-fixtures audit
pre-pr: pre-pr-lint pre-pr-build pre-pr-test pre-pr-validate pre-pr-fixtures audit deny
@echo ""
@echo "=== PRE-PR: ALL CHECKS PASSED ==="
@echo "Safe to create PR."
@@ -302,7 +303,7 @@ pre-pr-fixtures:
# Branch-specific variants (faster, scope-appropriate)
pre-pr-server: lint-server build-server test-server pre-pr-fixtures audit
pre-pr-server: lint-server build-server test-server pre-pr-fixtures audit deny
@echo "=== Server pre-PR: PASSED ==="
pre-pr-client: lint-client build-client test-client check-star-map
@@ -385,6 +386,9 @@ atlas-verify:
audit:
cd server && cargo audit
deny:
cd server && cargo deny check
checklist-validate:
@tooling/validate-checklist --check
+2 -1
View File
@@ -424,7 +424,8 @@ How the player observes and interacts with the world: camera, fog, line-of-sight
- **Raised by:** Workshop — unanimous
- **Dissent:** None
- **Implements:** Ticket #548
- **Cross-reference:** [D-041](architecture.md#d-041-knowledge-graph-data-model), [D-083](#d-083-contradiction-detection-pipeline), [D-078](#d-078-overheard-npc-conversation--passive-dialogue-panel-with-occlusion-filter) (D-078 scrapped per R-012; knowledge propagation system remains active independently)
- **Cross-reference:** [D-041](architecture.md#d-041-knowledge-graph-data-model), [D-083](#d-083-contradiction-detection-pipeline), [D-078](#d-078-overheard-npc-conversation--passive-dialogue-panel-with-occlusion-filter) (D-078 scrapped per R-012; `transfer_npc_knowledge` retained for Phase 5 rewire — see Amendment 2026-04-19)
- **Amendment (2026-04-19, R-012 / #848):** D-078 was scrapped (R-012) and the `run_npc_conversations` system was deleted in #848. The `transfer_npc_knowledge` system is **retained in-tree for Phase 5 rewire** but no longer fires in production — its `Added<NpcConversation>` trigger is now only inserted by test fixtures. The design (trust-gated transfer, dual-mutable KG access, `KnowsOf` confidence cap, `disclosure_blocked` honoring) is preserved; Phase 5 will wire a new proximity/dialogue trigger in its place. Until then, treat the system as dormant and guard against assuming it runs.
### D-081: Unprompted Disclosure Design
- **Date:** 2026-02-24
+16 -1
View File
@@ -216,6 +216,11 @@ Note: `ConfirmBookmark` is the **trigger** for transitioning from the character-
/// Populated when `ConfirmBookmark` is processed. `None` during the
/// character-creation phase (before confirm) and always `None` in a
/// fresh session.
///
/// **v0.2 scope: transient only.** Not serialized — save/load of
/// `SelectedBookmark` is deferred to Sprint 37 (follow-up ticket
/// filed alongside #614). Add `Serialize`/`Deserialize` derives and
/// wire into `SaveState` when that ticket is claimed.
#[derive(Resource, Debug, Clone, Default)]
pub struct SelectedBookmark {
pub bookmark_id: Option<String>,
@@ -224,7 +229,17 @@ pub struct SelectedBookmark {
```
Downstream systems (apartment generator, skill seeder) read from this
resource. Preserved across save/load (#553) as part of the SaveState.
resource.
**Save/load scope (v0.2 deferred):** `SelectedBookmark` is transient for
v0.2 — it lives in-memory from `ConfirmBookmark` through session end and
is not persisted. A reload after quit returns the player to the
character-creation screen. Promotion to persistent state (adding
`Serialize`/`Deserialize` and threading into `SaveState` / #553) is
tracked in a follow-up ticket for Sprint 37. `SelectedBookmark` must
carry an inline `// TODO(sprint-37): serialize — see #<follow-up ticket>`
comment in `server/src/bookmark/mod.rs` pointing at the follow-up so the
omission is greppable.
## 5. Content source — how bookmarks get into the registry
+23 -6
View File
@@ -58,7 +58,9 @@ struct Cli {
)]
output: PathBuf,
/// PRNG seed for deterministic generation
/// PRNG seed for deterministic generation.
/// Seed 1 is canonical — the committed output in db/ was produced at seed=1.
/// Use a different seed only for experimentation; committed output must stay seed=1.
#[arg(long, default_value = "1")]
seed: u64,
@@ -207,7 +209,10 @@ fn load_corps(conn: &Connection) -> Vec<Corp> {
LEFT JOIN star_systems ss ON co.headquarters_system = ss.system_id
ORDER BY co.corp_id",
)
.unwrap();
.unwrap_or_else(|e| {
eprintln!("error: failed to prepare corps query: {}", e);
process::exit(1);
});
stmt.query_map([], |row| {
Ok(Corp {
@@ -219,7 +224,10 @@ fn load_corps(conn: &Connection) -> Vec<Corp> {
shadow_economy_access: row.get::<_, i64>(5).unwrap_or(0) != 0,
})
})
.unwrap()
.unwrap_or_else(|e| {
eprintln!("error: failed to query corporations: {}", e);
process::exit(1);
})
.filter_map(|r| r.ok())
.collect()
}
@@ -227,9 +235,15 @@ fn load_corps(conn: &Connection) -> Vec<Corp> {
fn load_valid_commodity_ids(conn: &Connection) -> BTreeSet<String> {
let mut stmt = conn
.prepare("SELECT commodity_id FROM commodities")
.unwrap();
.unwrap_or_else(|e| {
eprintln!("error: failed to prepare commodities query: {}", e);
process::exit(1);
});
stmt.query_map([], |row| row.get::<_, String>(0))
.unwrap()
.unwrap_or_else(|e| {
eprintln!("error: failed to query commodities: {}", e);
process::exit(1);
})
.filter_map(|r| r.ok())
.collect()
}
@@ -626,7 +640,10 @@ fn main() {
process::exit(1);
});
conn.execute_batch("PRAGMA journal_mode=WAL; PRAGMA foreign_keys=ON;")
.unwrap();
.unwrap_or_else(|e| {
eprintln!("error: failed to configure DB pragmas: {}", e);
process::exit(1);
});
// Load corps and commodity IDs
println!(" [2/5] Loading corporations and commodities...");
+9 -2
View File
@@ -88,10 +88,17 @@ impl BookmarkRegistry {
}
/// The confirmed bookmark selection for the current session.
/// Populated when `ConfirmBookmark` is processed. `None` during the
/// character-creation phase (before confirm) and always `None` in a
/// fresh session.
///
/// **v0.2 scope: transient only.** Not serialized — save/load of
/// `SelectedBookmark` is deferred to Sprint 37 (follow-up ticket
/// filed alongside #614). Add `Serialize`/`Deserialize` derives and
/// wire into `SaveState` when that ticket is claimed.
///
/// `None` during the character-creation phase (before `ConfirmBookmark` is received).
/// Downstream systems (apartment generator, skill seeder) read from this resource.
/// Preserved across save/load (#553) as part of the SaveState.
// TODO(sprint-37): serialize — see #863
#[derive(Resource, Debug, Clone, Default)]
pub struct SelectedBookmark {
pub bookmark_id: Option<String>,
+3 -3
View File
@@ -257,15 +257,15 @@ pub struct BookmarkWire {
pub default_location: String,
pub allowed_locations: Vec<String>,
/// Parallel to `allowed_locations`: resolved culture tag per location (#679).
/// Populated by `BookmarkRegistry::build_catalog` once CultureResolverResource
/// is available. Empty string placeholder until #679 lands.
/// Populated by `BookmarkRegistry::build_catalog` using `CultureResolverResource`.
/// Empty string when the resolver has no mapping for a location.
pub allowed_locations_cultures: Vec<String>,
pub career: CareerKindWire,
pub starting_capital_tractus: i64,
}
/// Catalog of all available bookmarks sent to the client (#614).
#[derive(Debug, Clone, Serialize, Deserialize)]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct BookmarkCatalog {
pub bookmarks: Vec<BookmarkWire>,
}
+179
View File
@@ -1208,6 +1208,21 @@ fn handle_confirm_bookmark(
return;
}
if let Some(ref mut sel) = selected_bookmark {
if sel.bookmark_id.is_some() {
let msg = format!(
"ConfirmBookmark: bookmark already confirmed ({}), ignoring retry",
sel.bookmark_id.as_deref().unwrap_or("?")
);
tracing::warn!("{}", msg);
if let Some(ref mut buf) = sim_error_buf {
buf.push(SimError {
kind: SimErrorKind::ProtocolError,
message: msg,
tick,
});
}
return;
}
sel.bookmark_id = Some(bookmark_id.clone());
sel.starting_location_id = Some(starting_location_id.clone());
tracing::info!(
@@ -2559,4 +2574,168 @@ mod tests {
let hub_spawn = crate::test_world::constants::HUB.spawn;
assert_eq!(pos.x, hub_spawn.x, "teleport works while paused");
}
fn make_bookmark_world() -> bevy_ecs::world::World {
use crate::bookmark::types::{BookmarkDefinition, BookmarkId, CareerKind};
use crate::bookmark::{BookmarkRegistry, SelectedBookmark};
use crate::bridge::types::SimErrorBuffer;
let mut world = bevy_ecs::world::World::new();
world.insert_resource(InputQueue::default());
world.insert_resource(SimulationTime::default());
world.init_resource::<crate::knowledge::EntityRegistry>();
world.init_resource::<SelectedBookmark>();
world.init_resource::<SimErrorBuffer>();
let mut registry = BookmarkRegistry::default();
registry.insert(BookmarkDefinition {
id: BookmarkId("test_bookmark".to_string()),
title: "Test Bookmark".into(),
subtitle: String::new(),
flavor: String::new(),
default_location: "Loc A".into(),
allowed_locations: vec!["Loc A".into(), "Loc B".into()],
career: CareerKind::Tycoon,
starting_capital_tractus: 1_000,
available: true,
});
world.insert_resource(registry);
world
.spawn((PlayerCharacter, TilePosition::new(5, 5, 0)))
.id();
world
}
#[test]
fn confirm_bookmark_unknown_id_emits_sim_error() {
use crate::bridge::types::SimErrorBuffer;
let mut world = make_bookmark_world();
world.resource_mut::<InputQueue>().push(PlayerInput {
tick: 0,
action: PlayerAction::ConfirmBookmark {
bookmark_id: "no_such_bookmark".into(),
starting_location_id: "Loc A".into(),
},
});
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(process_player_input);
schedule.run(&mut world);
let errors = world.resource_mut::<SimErrorBuffer>().drain();
assert_eq!(
errors.len(),
1,
"expected one SimError for unknown bookmark"
);
assert_eq!(
errors[0].kind,
crate::bridge::types::SimErrorKind::ProtocolError
);
assert!(errors[0].message.contains("unknown bookmark_id"));
assert!(
world
.resource::<crate::bookmark::SelectedBookmark>()
.bookmark_id
.is_none(),
"SelectedBookmark must not be set on error"
);
}
#[test]
fn confirm_bookmark_invalid_location_emits_sim_error() {
use crate::bridge::types::SimErrorBuffer;
let mut world = make_bookmark_world();
world.resource_mut::<InputQueue>().push(PlayerInput {
tick: 0,
action: PlayerAction::ConfirmBookmark {
bookmark_id: "test_bookmark".into(),
starting_location_id: "Not A Location".into(),
},
});
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(process_player_input);
schedule.run(&mut world);
let errors = world.resource_mut::<SimErrorBuffer>().drain();
assert_eq!(
errors.len(),
1,
"expected one SimError for invalid location"
);
assert_eq!(
errors[0].kind,
crate::bridge::types::SimErrorKind::ProtocolError
);
assert!(errors[0].message.contains("not in allowed_locations"));
}
#[test]
fn confirm_bookmark_valid_inputs_populate_selected_bookmark() {
use crate::bridge::types::SimErrorBuffer;
let mut world = make_bookmark_world();
world.resource_mut::<InputQueue>().push(PlayerInput {
tick: 0,
action: PlayerAction::ConfirmBookmark {
bookmark_id: "test_bookmark".into(),
starting_location_id: "Loc B".into(),
},
});
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(process_player_input);
schedule.run(&mut world);
let errors = world.resource_mut::<SimErrorBuffer>().drain();
assert!(
errors.is_empty(),
"no errors expected for valid ConfirmBookmark"
);
let sel = world.resource::<crate::bookmark::SelectedBookmark>();
assert_eq!(sel.bookmark_id.as_deref(), Some("test_bookmark"));
assert_eq!(sel.starting_location_id.as_deref(), Some("Loc B"));
}
#[test]
fn confirm_bookmark_double_confirm_emits_sim_error() {
use crate::bridge::types::SimErrorBuffer;
let mut world = make_bookmark_world();
// Both confirms at tick=0: processed in order within the same run.
// First succeeds; second hits the idempotency guard.
world.resource_mut::<InputQueue>().push(PlayerInput {
tick: 0,
action: PlayerAction::ConfirmBookmark {
bookmark_id: "test_bookmark".into(),
starting_location_id: "Loc A".into(),
},
});
world.resource_mut::<InputQueue>().push(PlayerInput {
tick: 0,
action: PlayerAction::ConfirmBookmark {
bookmark_id: "test_bookmark".into(),
starting_location_id: "Loc B".into(),
},
});
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(process_player_input);
schedule.run(&mut world);
let errors = world.resource_mut::<SimErrorBuffer>().drain();
assert_eq!(errors.len(), 1, "double-confirm must emit ProtocolError");
assert_eq!(
errors[0].kind,
crate::bridge::types::SimErrorKind::ProtocolError
);
assert!(errors[0].message.contains("already confirmed"));
// Selection must remain the original, not overwritten
let sel = world.resource::<crate::bookmark::SelectedBookmark>();
assert_eq!(sel.starting_location_id.as_deref(), Some("Loc A"));
}
}
+2 -2
View File
@@ -395,8 +395,8 @@ fn sound_range_tiles(range: &crate::knowledge::types::SoundRange) -> u32 {
/// Event-driven monologue trigger system (#119, D-035).
///
/// Checks observation events, sound events, overheard conversations, and
/// completed dialogues for monologue-worthy triggers. Fires at most one
/// Checks observation events, sound events, and completed dialogues for
/// monologue-worthy triggers. Fires at most one
/// monologue per tick. Event-driven — no cooldown gate. Updates
/// `last_fired_tick` so v0.2 periodic triggers can respect the recency window.
///
@@ -115,7 +115,7 @@ impl TransferCandidate {
/// If the player is within VOICE_RANGE_TILES, they gain entity-level knowledge
/// about both NPCs at `Suspects` confidence (`Heard` source).
///
/// System ordering: after(run_npc_conversations).
/// Runs in [`TickPhase::Simulation`] with no ordering constraint.
#[allow(clippy::too_many_arguments)]
pub fn transfer_npc_knowledge(
time: Res<SimulationTime>,
@@ -203,10 +203,9 @@ pub fn transfer_npc_knowledge(
}
// --- Dual-mutable KG access ---
// Transfer is one-directional per conversation tick: speaker → partner.
// If both participants are Active NPCs, each fires as "speaker" in
// separate conversation pairs (run_npc_conversations creates symmetric
// pairs), so both directions are covered across two iterations.
// Transfer is one-directional per NpcConversation component: speaker → partner.
// When both participants have NpcConversation, each fires as "speaker"
// in a separate query row, so both directions are covered.
let Ok([speaker_kg, mut partner_kg]) =
kg_query.get_many_mut([speaker_entity, partner_entity])
-1
View File
@@ -63,7 +63,6 @@ fn snapshot_roundtrip_over_unix_socket() {
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
-1
View File
@@ -49,7 +49,6 @@ fn snapshot_roundtrip_over_tcp() {
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
-1
View File
@@ -288,7 +288,6 @@ fn snapshot_with_sim_errors_roundtrips() {
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
+30 -3
View File
@@ -39,7 +39,6 @@ fn fixture_snapshot(tick: u64, entities: Vec<VisibleEntity>) -> ObserverSnapshot
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
@@ -237,7 +236,6 @@ fn generate_msgpack_fixtures() {
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
@@ -377,7 +375,6 @@ fn generate_msgpack_fixtures() {
blocked_entities: vec![5, 6],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: Some(0xDEADBEEF),
@@ -505,3 +502,33 @@ fn generate_msgpack_fixtures() {
write_fixture(name, &rmp_serde::to_vec_named(&snapshot).unwrap());
}
}
/// Generate a snapshot fixture with a populated `BookmarkCatalog`.
///
/// Client (#618) uses this to validate GDScript MessagePack decode against
/// real `rmp_serde` output — field order and string encoding may diverge
/// from GDScript-constructed data.
#[test]
#[ignore] // Run manually: cargo test --test gen_fixtures -- --ignored
fn generate_snapshot_with_bookmark_catalog() {
let catalog = BookmarkCatalog {
bookmarks: vec![BookmarkWire {
id: "tycoon".into(),
title: "Tycoon".into(),
subtitle: "Small business owner on the make.".into(),
flavor: String::new(),
default_location: "GJ 35".into(),
allowed_locations: vec!["GJ 35".into()],
allowed_locations_cultures: vec!["frontier_industrial".into()],
career: CareerKindWire::Tycoon,
starting_capital_tractus: 5_000,
}],
};
let mut snapshot = fixture_snapshot(0, vec![]);
snapshot.bookmark_catalog = Some(catalog);
let bytes =
rmp_serde::to_vec_named(&snapshot).expect("serialize snapshot_with_bookmark_catalog");
write_fixture("snapshot_with_bookmark_catalog", &bytes);
}
-3
View File
@@ -27,7 +27,6 @@ fn test_snapshot(tick: u64, entities: Vec<VisibleEntity>) -> ObserverSnapshot {
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
@@ -292,7 +291,6 @@ fn snapshot_v2_fields_roundtrip() {
blocked_entities: vec![],
scan_events: vec![],
sound_events: vec![],
follow_state: None,
character_pressure: None,
rng_seed: None,
@@ -1453,7 +1451,6 @@ fn serde_default_fields_fill_in_when_missing_from_wire() {
"blocked_entities": [],
"scan_events": [],
"sound_events": [],
"follow_state": null,
"rng_seed": null
});