From 7cf573646ddf86b21e76fd7d89364c86173f1d17 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Thu, 19 Mar 2026 11:03:15 +0100 Subject: [PATCH] fix(workspace, cli): Fix config loss on resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Continuing a conversation without passing `--cfg` flags caused the conversation's model and tools to be silently reset to defaults on each invocation. The root cause was a sequencing issue: `apply_conversation_config` runs inside `load_partial_config` and needs to access the active conversation's stream config from disk. Previously, `load_conversations_from_disk` was called _after_ `load_partial_config`, so events weren't loaded yet. The merge fell back to an empty partial and overwrote whatever the conversation had stored. The fix splits the old `load_conversations_from_disk` into two methods. `load_conversation_index` loads conversation IDs, metadata, and `TombMap` entries — everything needed for lazy event access — and is now called _before_ `load_partial_config`. `ensure_active_conversation_stream` handles the one remaining config-dependent step (creating a default stream for fresh workspaces) and runs _after_ the final `AppConfig` is built. Signed-off-by: Jean Mertz --- crates/jp_cli/src/lib.rs | 13 +- crates/jp_workspace/src/lib.rs | 65 +++++++--- crates/jp_workspace/src/lib_tests.rs | 141 ++++++++++++++++++++-- crates/jp_workspace/src/sanitize.rs | 4 +- crates/jp_workspace/src/sanitize_tests.rs | 16 +-- 5 files changed, 205 insertions(+), 34 deletions(-) diff --git a/crates/jp_cli/src/lib.rs b/crates/jp_cli/src/lib.rs index 039a6106..e217aabb 100644 --- a/crates/jp_cli/src/lib.rs +++ b/crates/jp_cli/src/lib.rs @@ -358,11 +358,20 @@ fn run_inner(cli: Cli, format: OutputFormat) -> Result<()> { } } + // Load conversation IDs and metadata from disk so that + // `apply_conversation_config` (called inside `load_partial_config`) can + // access conversation events via lazy loading. + if let Err(error) = workspace.load_conversation_index() { + tracing::error!(error = ?error, "Failed to load conversation index."); + } + let partial = load_partial_config(&cli.command, Some(&workspace), &cli.globals.config)?; let config = Arc::new(build(partial)?); - if let Err(error) = workspace.load_conversations_from_disk(config.clone()) { - tracing::error!(error = ?error, "Failed to load workspace."); + // For fresh workspaces, create a default stream now that we have the + // final config. + if let Err(error) = workspace.ensure_active_conversation_stream(config.clone()) { + tracing::error!(error = ?error, "Failed to ensure active conversation stream."); } let runtime = build_runtime(cli.root.threads, "jp-worker")?; diff --git a/crates/jp_workspace/src/lib.rs b/crates/jp_workspace/src/lib.rs index 7b983a9a..97885f1f 100644 --- a/crates/jp_workspace/src/lib.rs +++ b/crates/jp_workspace/src/lib.rs @@ -154,31 +154,36 @@ impl Workspace { self.storage.as_ref().and_then(Storage::user_storage_path) } - /// Load the workspace state from the persisted storage. + /// Load conversation IDs, metadata, and `TombMap` entries from disk. /// - /// If the workspace is not persisted, this method will return an error. + /// This populates the workspace state so that conversation events can be + /// accessed via [`get_events`](Self::get_events) (which triggers lazy + /// loading from storage). For the active conversation, the stream is + /// eagerly loaded for performance. + /// + /// For fresh workspaces (no conversations on disk), this registers the + /// active conversation ID in the events `TombMap` but does **not** create a + /// default stream — call [`ensure_active_conversation_stream`] after + /// the final [`AppConfig`] is available for that. /// /// Call [`sanitize`](Self::sanitize) before this method to ensure the /// filesystem is in a consistent state. - pub fn load_conversations_from_disk(&mut self, config: Arc) -> Result<()> { - trace!("Loading state."); + /// + /// [`ensure_active_conversation_stream`]: Self::ensure_active_conversation_stream + pub fn load_conversation_index(&mut self) -> Result<()> { + trace!("Loading conversation index."); - let storage = self.storage.as_mut().ok_or(Error::MissingStorage)?; + let storage = self.storage.as_ref().ok_or(Error::MissingStorage)?; let conversation_ids = storage.load_all_conversation_ids(); if conversation_ids.is_empty() { debug!("No conversations found, workspace is fresh."); - // Ensure the active conversation has an events entry so the - // invariant (active_conversation_id → events) always holds. + // Register the active conversation ID so that `get_events` can + // find the entry (lazy loading will return `None` since there is + // no file on disk, which is expected for fresh workspaces). let active_id = self.active_conversation_id(); - let _err = self - .state - .local - .events - .entry(active_id) - .or_default() - .set(ConversationStream::new(config).with_created_at(active_id.timestamp())); + self.state.local.events.entry(active_id).or_default(); return Ok(()); } @@ -206,8 +211,7 @@ impl Workspace { .map(|id| (id, OnceCell::new())) .collect(); - // We can `set` without checking if the cell is already initialized, as - // we just initialized it above.let _err = events + // Eagerly load the active conversation stream for performance. let _err = events .entry(metadata.active_conversation_id) .or_default() @@ -227,6 +231,35 @@ impl Workspace { Ok(()) } + /// Ensure the active conversation has an event stream. + /// + /// For fresh workspaces where no stream exists on disk, this creates a + /// default [`ConversationStream`] with the given config as its base. + /// + /// For existing workspaces where the stream was already loaded (eagerly + /// by [`load_conversation_index`] or lazily via [`get_events`]), this is + /// a no-op. + /// + /// [`load_conversation_index`]: Self::load_conversation_index + /// [`get_events`]: Self::get_events + pub fn ensure_active_conversation_stream(&mut self, config: Arc) -> Result<()> { + let active_id = self.active_conversation_id(); + + if self.get_events(&active_id).is_some() { + return Ok(()); + } + + let _err = self + .state + .local + .events + .entry(active_id) + .or_default() + .set(ConversationStream::new(config).with_created_at(active_id.timestamp())); + + Ok(()) + } + /// Persists the current in-memory workspace state back to disk atomically. /// /// This is a no-op if persistence is disabled. diff --git a/crates/jp_workspace/src/lib_tests.rs b/crates/jp_workspace/src/lib_tests.rs index f7dbe66d..99865752 100644 --- a/crates/jp_workspace/src/lib_tests.rs +++ b/crates/jp_workspace/src/lib_tests.rs @@ -2,6 +2,12 @@ use std::{collections::HashMap, fs, time::Duration}; use camino_tempfile::tempdir; use datetime_literal::datetime; +use jp_config::{ + PartialConfig as _, + fs::load_partial, + model::id::{ModelIdOrAliasConfig, PartialModelIdOrAliasConfig, ProviderId}, + util::build, +}; use jp_conversation::ConversationsMetadata; use jp_storage::{ CONVERSATIONS_DIR, METADATA_FILE, @@ -234,7 +240,7 @@ fn test_workspace_cannot_remove_active_conversation() { } #[test] -fn test_load_succeeds_when_no_conversations_exist() { +fn test_load_index_fresh_workspace_then_ensure_stream() { let tmp = tempdir().unwrap(); let root = tmp.path().join("root"); let storage = root.join("storage"); @@ -247,16 +253,137 @@ fn test_load_succeeds_when_no_conversations_exist() { let mut workspace = Workspace::new(&root).persisted_at(&storage).unwrap(); workspace.disable_persistence(); - // A fresh workspace with no conversations on disk is valid — load - // should succeed with default state. - let config = Arc::new(AppConfig::new_test()); - workspace.load_conversations_from_disk(config).unwrap(); - - // The active conversation must have an events entry. + // Phase 1: load index — no conversations on disk, so the active + // conversation entry is registered but has no stream yet. + workspace.load_conversation_index().unwrap(); let active_id = workspace.active_conversation_id(); + assert!( + workspace.get_events(&active_id).is_none(), + "fresh workspace should have no stream before ensure_active_conversation_stream" + ); + + // Phase 2: create the default stream with the final config. + let config = Arc::new(AppConfig::new_test()); + workspace.ensure_active_conversation_stream(config).unwrap(); assert!(workspace.get_events(&active_id).is_some()); } +#[test] +fn test_load_index_existing_workspace_events_accessible() { + let tmp = tempdir().unwrap(); + let root = tmp.path().join("root"); + let storage_path = root.join("storage"); + + let config = Arc::new(AppConfig::new_test()); + let id = ConversationId::try_from(datetime!(2024-03-15 12:00:00 Z)).unwrap(); + + // Write a conversation to disk. + { + let mut ws = Workspace::new(&root).persisted_at(&storage_path).unwrap(); + ws.create_conversation_with_id(id, Conversation::default(), config.clone()); + ws.set_active_conversation_id(id, DateTime::::UNIX_EPOCH) + .unwrap(); + ws.persist().unwrap(); + } + + // Reload from scratch — only load_conversation_index, no config needed. + let mut ws = Workspace::new(&root).persisted_at(&storage_path).unwrap(); + ws.disable_persistence(); + ws.load_conversation_index().unwrap(); + + // Events should be accessible via lazy loading. + assert_eq!(ws.active_conversation_id(), id); + let events = ws.get_events(&id); + assert!( + events.is_some(), + "events must be lazily loadable after load_conversation_index" + ); + + // The stream's config should be retrievable (this is what + // apply_conversation_config relies on). + let stream_config = events.unwrap().config(); + assert!(stream_config.is_ok()); + + // ensure_active_conversation_stream should be a no-op. + ws.ensure_active_conversation_stream(config).unwrap(); + assert!(ws.get_events(&id).is_some()); +} + +/// Regression test for the bug where continuing a conversation without the +/// original config passed in via `--cfg` caused a spurious `ConfigDelta` that +/// reset the model and disabled all tools. +/// +/// The root cause was that `load_partial_config` ran before conversation events +/// were loaded from disk, so `apply_conversation_config` couldn't read the +/// stream config and fell back to an empty partial. +/// +/// This test exercises the full round-trip: +/// 1. Create a conversation with a custom model name, persist to disk. +/// 2. Reload with `load_conversation_index` only (no config needed). +/// 3. Simulate `apply_conversation_config`: merge stream config into a bare +/// partial (as if no custom config was passed). +/// 4. Build the final config and assert the custom model name survived. +/// 5. Assert that `get_config_delta_from_cli` would produce no delta. +#[test] +fn test_conversation_config_preserved_across_reload() { + let tmp = tempdir().unwrap(); + let root = tmp.path().join("root"); + let storage_path = root.join("storage"); + + // Build a config with a distinctive model name. + let mut custom_config = AppConfig::new_test(); + custom_config.assistant.model.id = + ModelIdOrAliasConfig::Id((ProviderId::Anthropic, "custom-model").try_into().unwrap()); + let id = ConversationId::try_from(datetime!(2024-05-20 10:00:00 Z)).unwrap(); + + // Persist a conversation that was created with the custom config. + { + let mut ws = Workspace::new(&root).persisted_at(&storage_path).unwrap(); + ws.create_conversation_with_id(id, Conversation::default(), Arc::new(custom_config)); + ws.set_active_conversation_id(id, DateTime::::UNIX_EPOCH) + .unwrap(); + ws.persist().unwrap(); + } + + // Simulate a second invocation WITHOUT the custom config. + let mut ws = Workspace::new(&root).persisted_at(&storage_path).unwrap(); + ws.disable_persistence(); + ws.load_conversation_index().unwrap(); + + // Simulate `apply_conversation_config`: merge the stream's config into a + // bare (default) partial, exactly as the CLI does when no `--cfg` flag is + // provided. We use new_test().to_partial() to represent the default config + // (file-based + env, but no custom config overlay). + let bare_partial = AppConfig::new_test().to_partial(); + let stream_config = ws + .get_events(&id) + .expect("events must be accessible") + .config() + .expect("valid config") + .to_partial(); + let merged = load_partial(bare_partial, stream_config).expect("merge ok"); + let final_config = build(merged).expect("valid config"); + + // The custom config's model name must survive the round-trip. + let resolved = final_config.assistant.model.id.resolved(); + assert_eq!( + resolved.name.as_ref(), + "custom-model", + "conversation config must be preserved when continuing without --cfg flag" + ); + + // The delta must NOT contain a model change — this was the core symptom of + // the bug, where the model reverted to the default. + let stream_partial = ws.get_events(&id).unwrap().config().unwrap().to_partial(); + let delta = stream_partial.delta(final_config.to_partial()); + assert_eq!( + delta.assistant.model.id, + PartialModelIdOrAliasConfig::empty(), + "config delta must not contain a model change when continuing a conversation without \ + overrides" + ); +} + #[test] fn test_workspace_persist_active_conversation() { let tmp = tempdir().unwrap(); diff --git a/crates/jp_workspace/src/sanitize.rs b/crates/jp_workspace/src/sanitize.rs index 0d07067f..df149ca9 100644 --- a/crates/jp_workspace/src/sanitize.rs +++ b/crates/jp_workspace/src/sanitize.rs @@ -50,10 +50,10 @@ impl Workspace { /// those that fail validation, and ensures the active conversation ID /// resolves to a valid conversation. Returns a report of what was repaired. /// - /// This should be called before [`load_conversations_from_disk`] to + /// This should be called before [`load_conversation_index`] to /// guarantee the filesystem is in a consistent state. /// - /// [`load_conversations_from_disk`]: Self::load_conversations_from_disk + /// [`load_conversation_index`]: Self::load_conversation_index pub fn sanitize(&mut self) -> Result { let storage = self.storage.as_ref().ok_or(Error::MissingStorage)?; diff --git a/crates/jp_workspace/src/sanitize_tests.rs b/crates/jp_workspace/src/sanitize_tests.rs index 841451c4..f72a35e0 100644 --- a/crates/jp_workspace/src/sanitize_tests.rs +++ b/crates/jp_workspace/src/sanitize_tests.rs @@ -79,7 +79,7 @@ fn test_reassigns_active_when_trashed() { assert!(report.active_reassigned); assert!(!report.default_created); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); assert_eq!(ws.active_conversation_id(), id2); } @@ -100,7 +100,7 @@ fn test_reassigns_to_most_recent_valid() { let report = ws.sanitize().unwrap(); assert!(report.active_reassigned); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); assert_eq!(ws.active_conversation_id(), id2); } @@ -123,7 +123,7 @@ fn test_reassigns_when_active_has_no_directory() { assert!(report.active_reassigned); assert!(!report.default_created); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); assert_eq!(ws.active_conversation_id(), id2); } @@ -143,7 +143,8 @@ fn test_default_created_when_all_trashed() { assert!(report.default_created); assert_eq!(report.trashed.len(), 1); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); + ws.ensure_active_conversation_stream(test_config()).unwrap(); } #[test] @@ -162,7 +163,7 @@ fn test_corrupt_global_metadata_reassigns_to_valid() { assert!(report.active_reassigned); assert!(!report.default_created); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); assert_eq!(ws.active_conversation_id(), id2); } @@ -178,7 +179,8 @@ fn test_corrupt_global_metadata_with_no_conversations() { assert!(!report.has_repairs()); assert!(!storage.conversations_metadata_exists()); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); + ws.ensure_active_conversation_stream(test_config()).unwrap(); } #[test] @@ -235,7 +237,7 @@ fn test_sanitize_then_load_with_mixed_valid_and_invalid() { assert!(report.active_reassigned); assert!(!report.default_created); - ws.load_conversations_from_disk(test_config()).unwrap(); + ws.load_conversation_index().unwrap(); assert_eq!(ws.active_conversation_id(), id3); assert!(ws.get_conversation(&id1).is_some()); }