feat(simulation): persist SelectedBookmark across save/load (#863, #862)

Adds Serialize/Deserialize to SelectedBookmark and wires it into
SaveStateV1 so a loaded game remembers which bookmark and starting
location the player picked. Replaces the TODO at bookmark/mod.rs:95
(originally deferred to Sprint 37 alongside #614).

Also refactors BookmarkPlugin to accept an injected BookmarkRegistry
via BookmarkPlugin::new(registry) (#862). The Default constructor
still wires the canonical tycoon registry — injection is for tests
and future TOML loading. Flagged in PR #132 review as a follow-up.

Updates bookmark spec §4.4 to remove the v0.2-deferred scope note.

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

docs/architecture/sprint-36-bookmark-spec.md

@@ -213,15 +213,12 @@ Note: `ConfirmBookmark` is the **trigger** for transitioning from the character-
 
 ```rust
 /// The confirmed bookmark selection for the current session.
-/// Populated when `ConfirmBookmark` is processed. `None` during the
+/// Populated when `ConfirmBookmark` is processed. `None` fields during the
-/// character-creation phase (before confirm) and always `None` in a
+/// character-creation phase (before confirm) and in a fresh session.
-/// fresh session.
 ///
-/// **v0.2 scope: transient only.** Not serialized — save/load of
+/// Serialized into `SaveStateV1.selected_bookmark` (#863) so that a loaded
-/// `SelectedBookmark` is deferred to Sprint 37 (follow-up ticket
+/// game remembers which bookmark and starting location were chosen.
-/// filed alongside #614). Add `Serialize`/`Deserialize` derives and
+#[derive(Resource, Debug, Clone, Default, Serialize, Deserialize)]
-/// wire into `SaveState` when that ticket is claimed.
-#[derive(Resource, Debug, Clone, Default)]
 pub struct SelectedBookmark {
     pub bookmark_id: Option<String>,
     pub starting_location_id: Option<String>,
@@ -231,15 +228,11 @@ pub struct SelectedBookmark {
 Downstream systems (apartment generator, skill seeder) read from this
 resource.
 
-**Save/load scope (v0.2 deferred):** `SelectedBookmark` is transient for
+**Save/load scope (Sprint 37, #863):** `SelectedBookmark` is persisted into
-v0.2 — it lives in-memory from `ConfirmBookmark` through session end and
+`SaveStateV1.selected_bookmark`. After `load_from_file` completes, the resource
-is not persisted. A reload after quit returns the player to the
+reflects the bookmark confirmed at session-start. Saves created before Sprint 37
-character-creation screen. Promotion to persistent state (adding
+will deserialize the field as `SelectedBookmark::default()` (both fields `None`)
-`Serialize`/`Deserialize` and threading into `SaveState` /  #553) is
+via `#[serde(default)]` on the `SaveStateV1` field.
-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
 

server/src/bookmark/mod.rs

@@ -20,7 +20,11 @@ use crate::knowledge::{resolve_culture, CultureResolver};
 ///
 /// Built once during `BookmarkPlugin::build`, read-only at runtime.
 /// `BTreeMap` for deterministic iteration (D-010 principle 4, D-041).
-#[derive(Resource, Debug, Default)]
+///
+/// `Clone` is required by `BookmarkPlugin::build` which moves the registry into
+/// the Bevy `App` via `insert_resource` while retaining the value from `self`
+/// (#862 injection pattern).
+#[derive(Resource, Debug, Default, Clone)]
 pub struct BookmarkRegistry {
     entries: BTreeMap<String, BookmarkDefinition>,
 }
@@ -88,18 +92,15 @@ 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
+/// Populated when `ConfirmBookmark` is processed. `None` fields during the
-/// `SelectedBookmark` is deferred to Sprint 37 (follow-up ticket
+/// character-creation phase (before confirm) and in a fresh session.
-/// filed alongside #614). Add `Serialize`/`Deserialize` derives and
+///
-/// wire into `SaveState` when that ticket is claimed.
+/// Serialized into `SaveStateV1.selected_bookmark` (#863) so that a loaded
+/// game remembers which bookmark and starting location were chosen.
 ///
 /// Downstream systems (apartment generator, skill seeder) read from this resource.
-// TODO(sprint-37): serialize — see #863
+#[derive(Resource, Debug, Clone, Default, serde::Serialize, serde::Deserialize)]
-#[derive(Resource, Debug, Clone, Default)]
 pub struct SelectedBookmark {
     pub bookmark_id: Option<String>,
     pub starting_location_id: Option<String>,
@@ -109,14 +110,47 @@ pub struct SelectedBookmark {
 ///
 /// Registers `BookmarkRegistry`, `SelectedBookmark`, and the startup system that
 /// stages the initial catalog in `SnapshotBuffer` for tick-0 delivery.
-pub struct BookmarkPlugin;
+///
+/// **Default construction** uses the canonical tycoon registry:
+/// ```ignore
+/// app.add_plugins(BookmarkPlugin::default());
+/// ```
+///
+/// **Injection** substitutes a custom registry (for tests and future TOML loading):
+/// ```ignore
+/// let mut registry = BookmarkRegistry::default();
+/// // populate ...
+/// app.add_plugins(BookmarkPlugin::new(registry));
+/// ```
+pub struct BookmarkPlugin {
+    registry: BookmarkRegistry,
+}
+
+impl BookmarkPlugin {
+    /// Create a plugin with an injected registry.
+    ///
+    /// Useful in tests (inject a minimal registry with fixed entries) and
+    /// for future TOML loading (caller constructs the registry from disk,
+    /// then hands it to the plugin).
+    pub fn new(registry: BookmarkRegistry) -> Self {
+        Self { registry }
+    }
+}
+
+impl Default for BookmarkPlugin {
+    /// Default plugin pre-populates the canonical tycoon registry via
+    /// `register_default_bookmarks`. Equivalent to the v0.2 behaviour before
+    /// injection was introduced.
+    fn default() -> Self {
+        let mut registry = BookmarkRegistry::default();
+        register_default_bookmarks(&mut registry);
+        Self { registry }
+    }
+}
 
 impl Plugin for BookmarkPlugin {
     fn build(&self, app: &mut App) {
-        let mut registry = BookmarkRegistry::default();
+        app.insert_resource(self.registry.clone())
-        register_default_bookmarks(&mut registry);
-
-        app.insert_resource(registry)
             .init_resource::<SelectedBookmark>()
             .add_systems(Startup, prime_initial_catalog);
 

server/src/simulation/save_io.rs

@@ -14,6 +14,7 @@ use std::path::{Path, PathBuf};
 use bevy_ecs::prelude::*;
 use thiserror::Error;
 
+use crate::bookmark::SelectedBookmark;
 use crate::bridge::types::SaveLoadResultWire;
 use crate::bridge::types::SnapshotBuffer;
 use crate::knowledge::graph::KnowledgeGraph;
@@ -160,6 +161,10 @@ pub fn save_to_file(path: &Path, world: &mut World) -> Result<(), SaveLoadError>
         last_activation_tick: world
             .get_resource::<ActivationState>()
             .and_then(|a| a.last_activation_tick),
+        selected_bookmark: world
+            .get_resource::<SelectedBookmark>()
+            .cloned()
+            .unwrap_or_default(),
     };
 
     let bytes = state
@@ -285,6 +290,10 @@ pub fn load_from_file(path: &Path, world: &mut World) -> Result<(), SaveLoadErro
         last_activation_tick: state.last_activation_tick,
     });
 
+    // Restore bookmark selection (#863) — the player's confirmed bookmark and
+    // starting location survive save/load so downstream systems stay consistent.
+    world.insert_resource(state.selected_bookmark);
+
     // Reset event queues and transient buffers — prevent stale events/history
     // from the pre-load world leaking into the post-load simulation.
     world.insert_resource(ContaminationEventQueue::default());
@@ -415,8 +424,11 @@ mod tests {
     static COUNTER: AtomicU64 = AtomicU64::new(0);
 
     fn temp_path() -> PathBuf {
+        // Include the process ID so nextest processes (each starts COUNTER at 0)
+        // do not collide on the same filename when running concurrently.
+        let pid = std::process::id();
         let id = COUNTER.fetch_add(1, Ordering::Relaxed);
-        std::env::temp_dir().join(format!("settled_reach_save_io_test_{}.msgpack", id))
+        std::env::temp_dir().join(format!("settled_reach_save_io_test_{}_{}.msgpack", pid, id))
     }
 
     fn minimal_world() -> World {
@@ -623,6 +635,7 @@ mod tests {
             contamination_active: false,
             activated_count: 0,
             last_activation_tick: None,
+            selected_bookmark: SelectedBookmark::default(),
         };
         let bytes = bad_state.to_bytes().expect("serialize");
         let path = temp_path();
@@ -955,4 +968,72 @@ mod tests {
 
         let _ = std::fs::remove_file(&path);
     }
+
+    // -----------------------------------------------------------------------
+    // SelectedBookmark round-trip (#863)
+    // -----------------------------------------------------------------------
+
+    #[test]
+    fn save_load_preserves_selected_bookmark() {
+        let mut world = minimal_world();
+
+        // Set a bookmark selection before saving.
+        world.insert_resource(SelectedBookmark {
+            bookmark_id: Some("tycoon".to_string()),
+            starting_location_id: Some("new-stockholm".to_string()),
+        });
+
+        let path = temp_path();
+        save_to_file(&path, &mut world).expect("save");
+
+        // Clear the resource to prove load restores it, not the pre-existing value.
+        world.insert_resource(SelectedBookmark::default());
+        assert!(
+            world
+                .resource::<SelectedBookmark>()
+                .bookmark_id
+                .is_none(),
+            "bookmark must be cleared before load"
+        );
+
+        load_from_file(&path, &mut world).expect("load");
+
+        let restored = world.resource::<SelectedBookmark>();
+        assert_eq!(
+            restored.bookmark_id.as_deref(),
+            Some("tycoon"),
+            "bookmark_id must survive round-trip"
+        );
+        assert_eq!(
+            restored.starting_location_id.as_deref(),
+            Some("new-stockholm"),
+            "starting_location_id must survive round-trip"
+        );
+
+        let _ = std::fs::remove_file(&path);
+    }
+
+    #[test]
+    fn save_load_selected_bookmark_defaults_when_unset() {
+        // Save without a bookmark selection (default = both None).
+        // Load must produce SelectedBookmark::default(), not error.
+        let mut world = minimal_world();
+        // SelectedBookmark not explicitly inserted — should default to no selection.
+
+        let path = temp_path();
+        save_to_file(&path, &mut world).expect("save");
+        load_from_file(&path, &mut world).expect("load");
+
+        let restored = world.resource::<SelectedBookmark>();
+        assert!(
+            restored.bookmark_id.is_none(),
+            "unset bookmark_id must survive as None"
+        );
+        assert!(
+            restored.starting_location_id.is_none(),
+            "unset starting_location_id must survive as None"
+        );
+
+        let _ = std::fs::remove_file(&path);
+    }
 }

server/src/simulation/save_state.rs

@@ -51,6 +51,7 @@ use crate::npc::{
     PersonalityTraits, Relationships, Secret, SecretSeverity, SkillSet, TellSystem,
     ToleranceThreshold, Want, WantKind,
 };
+use crate::bookmark::SelectedBookmark;
 use crate::simulation::modification::Modification;
 use crate::simulation::movement::TilePosition;
 use crate::simulation::time::TickRate;
@@ -122,6 +123,11 @@ pub struct SaveStateV1 {
     /// `None` if no activation yet. Persisted alongside `activated_count`.
     #[serde(default)]
     pub last_activation_tick: Option<u64>,
+    /// Bookmark and starting location confirmed by the player at session start (#863, #614).
+    /// Both fields are `None` in saves created before #863 or before character creation
+    /// completes. Defaults to `SelectedBookmark::default()` for backward compatibility.
+    #[serde(default)]
+    pub selected_bookmark: SelectedBookmark,
 }
 
 /// Per-NPC state snapshot for `SaveStateV1`.
@@ -435,6 +441,7 @@ mod tests {
             contamination_active: false,
             activated_count: 0,
             last_activation_tick: None,
+            selected_bookmark: crate::bookmark::SelectedBookmark::default(),
         }
     }
 
@@ -854,6 +861,7 @@ mod tests {
             contamination_active: false,
             activated_count: 0,
             last_activation_tick: None,
+            selected_bookmark: crate::bookmark::SelectedBookmark::default(),
         };
 
         let bytes = save.to_bytes().expect("serialize");
@@ -878,6 +886,60 @@ mod tests {
         assert!(result.is_err(), "must panic without StableEntityId");
     }
 
+    // -----------------------------------------------------------------------
+    // SelectedBookmark round-trip (#863)
+    // -----------------------------------------------------------------------
+
+    #[test]
+    fn selected_bookmark_with_values_survives_roundtrip() {
+        // Spec (#863): SelectedBookmark persisted in SaveStateV1 must survive a
+        // full MessagePack serialize → deserialize cycle with field values intact.
+        let mut state = minimal_save_state();
+        state.selected_bookmark = crate::bookmark::SelectedBookmark {
+            bookmark_id: Some("tycoon".to_string()),
+            starting_location_id: Some("GJ 35".to_string()),
+        };
+
+        let bytes = state.to_bytes().expect("serialize");
+        let recovered = SaveStateV1::from_bytes(&bytes).expect("deserialize");
+
+        assert_eq!(
+            recovered.selected_bookmark.bookmark_id,
+            Some("tycoon".to_string()),
+            "bookmark_id must survive roundtrip"
+        );
+        assert_eq!(
+            recovered.selected_bookmark.starting_location_id,
+            Some("GJ 35".to_string()),
+            "starting_location_id must survive roundtrip"
+        );
+
+        // Idempotent re-serialize: bytes must be stable
+        let bytes2 = recovered.to_bytes().expect("re-serialize");
+        assert_eq!(bytes, bytes2, "SelectedBookmark roundtrip must be idempotent");
+    }
+
+    #[test]
+    fn selected_bookmark_default_survives_roundtrip() {
+        // Spec (#863): saves from before Sprint 37 (both fields None) must load
+        // cleanly via serde(default) on the SaveStateV1 field.
+        let state = minimal_save_state();
+        assert!(state.selected_bookmark.bookmark_id.is_none());
+        assert!(state.selected_bookmark.starting_location_id.is_none());
+
+        let bytes = state.to_bytes().expect("serialize");
+        let recovered = SaveStateV1::from_bytes(&bytes).expect("deserialize");
+
+        assert!(
+            recovered.selected_bookmark.bookmark_id.is_none(),
+            "default bookmark_id (None) must roundtrip"
+        );
+        assert!(
+            recovered.selected_bookmark.starting_location_id.is_none(),
+            "default starting_location_id (None) must roundtrip"
+        );
+    }
+
     // -----------------------------------------------------------------------
     // Modifications stub round-trip (#567, D-111/D-112)
     // -----------------------------------------------------------------------