diff --git a/src/app.rs b/src/app.rs index e89b7e79..f7d5aabb 100644 --- a/src/app.rs +++ b/src/app.rs @@ -255,15 +255,18 @@ impl AppBuilder { self.libsql_db.take(); } - // Re-resolve config with OS credentials - if let Some(ref db) = self.db { - let toml_path = self.toml_path.as_deref(); - if let Ok(refreshed) = - Config::from_db_with_toml(db.as_ref(), "default", toml_path).await - { - self.config = refreshed; - tracing::debug!("LlmConfig re-resolved after OS credential injection"); - } + // Re-resolve only the LLM config with OS credentials. + let store: Option<&(dyn crate::db::SettingsStore + Sync)> = + self.db.as_ref().map(|db| db.as_ref() as _); + let toml_path = self.toml_path.as_deref(); + if let Err(e) = self + .config + .re_resolve_llm(store, "default", toml_path) + .await + { + tracing::warn!( + "Failed to re-resolve LLM config after OS credential injection: {e}" + ); } return Ok(()); @@ -308,18 +311,16 @@ impl AppBuilder { // Inject LLM API keys from encrypted storage crate::config::inject_llm_keys_from_secrets(secrets.as_ref(), "default").await; - // Re-resolve config with newly available keys - if let Some(ref db) = self.db { - let toml_path = self.toml_path.as_deref(); - match Config::from_db_with_toml(db.as_ref(), "default", toml_path).await { - Ok(refreshed) => { - self.config = refreshed; - tracing::debug!("LlmConfig re-resolved after secret injection"); - } - Err(e) => { - tracing::warn!("Failed to re-resolve config after secret injection: {}", e); - } - } + // Re-resolve only the LLM config with newly available keys. + let store: Option<&(dyn crate::db::SettingsStore + Sync)> = + self.db.as_ref().map(|db| db.as_ref() as _); + let toml_path = self.toml_path.as_deref(); + if let Err(e) = self + .config + .re_resolve_llm(store, "default", toml_path) + .await + { + tracing::warn!("Failed to re-resolve LLM config after secret injection: {e}"); } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 8e4b4254..1112d1ac 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -257,6 +257,32 @@ impl Config { Ok(()) } + /// Re-resolve only the LLM config after credential injection. + /// + /// Called by `AppBuilder::init_secrets()` after injecting API keys into + /// the env overlay. Only rebuilds `self.llm` — all other config fields + /// are unaffected, preserving values from the initial config load (or + /// from `Config::for_testing()` in test mode). + pub async fn re_resolve_llm( + &mut self, + store: Option<&(dyn crate::db::SettingsStore + Sync)>, + user_id: &str, + toml_path: Option<&std::path::Path>, + ) -> Result<(), ConfigError> { + let settings = if let Some(store) = store { + let mut s = match store.get_all_settings(user_id).await { + Ok(map) => Settings::from_db_map(&map), + Err(_) => Settings::default(), + }; + Self::apply_toml_overlay(&mut s, toml_path)?; + s + } else { + Settings::default() + }; + self.llm = LlmConfig::resolve(&settings)?; + Ok(()) + } + /// Build config from settings (shared by from_env and from_db). async fn build(settings: &Settings) -> Result { Ok(Self { diff --git a/tests/support/trace_llm.rs b/tests/support/trace_llm.rs index 804e8eab..e09ee9d9 100644 --- a/tests/support/trace_llm.rs +++ b/tests/support/trace_llm.rs @@ -513,32 +513,41 @@ impl LlmProvider for TraceLlm { } async fn complete(&self, request: CompletionRequest) -> Result { - let step = self.next_step(&request.messages)?; - match step.response { - TraceResponse::Text { - content, - input_tokens, - output_tokens, - } => Ok(CompletionResponse { - content, - input_tokens, - output_tokens, - finish_reason: FinishReason::Stop, - cache_read_input_tokens: 0, - cache_creation_input_tokens: 0, - }), - TraceResponse::ToolCalls { .. } => Err(LlmError::RequestFailed { - provider: self.model_name.clone(), - reason: "TraceLlm::complete() called but current step is a tool_calls response; \ - use complete_with_tools() instead" - .to_string(), - }), - TraceResponse::UserInput { .. } => Err(LlmError::RequestFailed { - provider: self.model_name.clone(), - reason: "TraceLlm::complete() encountered a user_input step; \ - these should have been filtered out during construction" - .to_string(), - }), + // complete() is called when Reasoning has force_text=true (no tools + // available). Skip any remaining ToolCalls steps in the trace and + // return the next Text step, since in real usage the LLM would + // produce text when no tools are offered. + loop { + let step = self.next_step(&request.messages)?; + match step.response { + TraceResponse::Text { + content, + input_tokens, + output_tokens, + } => { + return Ok(CompletionResponse { + content, + input_tokens, + output_tokens, + finish_reason: FinishReason::Stop, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + }); + } + TraceResponse::ToolCalls { .. } => { + // Skip tool_calls steps — complete() is called in + // force_text mode so the LLM can't use tools anyway. + continue; + } + TraceResponse::UserInput { .. } => { + return Err(LlmError::RequestFailed { + provider: self.model_name.clone(), + reason: "TraceLlm::complete() encountered a user_input step; \ + these should have been filtered out during construction" + .to_string(), + }); + } + } } } diff --git a/tests/support_unit_tests.rs b/tests/support_unit_tests.rs index 645746ea..4ac65c0f 100644 --- a/tests/support_unit_tests.rs +++ b/tests/support_unit_tests.rs @@ -571,22 +571,29 @@ mod trace_llm_tests { } #[tokio::test] - async fn complete_errors_on_tool_calls_step() { + async fn complete_skips_tool_calls_step() { + // complete() is called in force_text mode where tools aren't available. + // When the trace has a ToolCalls step followed by a Text step, complete() + // should skip the ToolCalls and return the Text response. let trace = LlmTrace::single_turn( "test-model", "hi", - vec![tool_calls_step(vec![simple_tool_call("echo")], 10, 5)], + vec![ + tool_calls_step(vec![simple_tool_call("echo")], 10, 5), + text_step("skipped past tools", 20, 8), + ], ); let llm = TraceLlm::from_trace(trace); - let result = llm.complete(make_completion_request("hi")).await; + let resp = llm + .complete(make_completion_request("hi")) + .await + .expect("complete() should skip ToolCalls and return the Text step"); - assert!(result.is_err()); - let err_msg = result.unwrap_err().to_string(); - assert!( - err_msg.contains("tool_calls"), - "Expected 'tool_calls' in error: {err_msg}" - ); + assert_eq!(resp.content, "skipped past tools"); + assert_eq!(resp.input_tokens, 20); + assert_eq!(resp.output_tokens, 8); + assert_eq!(resp.finish_reason, FinishReason::Stop); } #[tokio::test]