mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-09 03:20:56 +00:00
Fix subagent/system prompt empty handling with TDD
This commit is contained in:
parent
59634a3936
commit
6280a822eb
4 changed files with 119 additions and 7 deletions
|
|
@ -677,7 +677,10 @@ impl Session {
|
|||
}
|
||||
|
||||
fn build_request(&self) -> Request {
|
||||
let mut messages = vec![Message::system(self.system_prompt.clone())];
|
||||
let mut messages = Vec::new();
|
||||
if !self.system_prompt.trim().is_empty() {
|
||||
messages.push(Message::system(self.system_prompt.clone()));
|
||||
}
|
||||
messages.extend(self.history.convert_to_messages());
|
||||
|
||||
let tools = self.provider_profile.tools();
|
||||
|
|
@ -722,7 +725,7 @@ mod tests {
|
|||
use crate::tool_registry::{RegisteredTool, ToolRegistry};
|
||||
use arc_llm::error::ProviderErrorDetail;
|
||||
use arc_llm::provider::{ProviderAdapter, StreamEventStream};
|
||||
use arc_llm::types::{Response, ToolDefinition};
|
||||
use arc_llm::types::{Response, Role, ToolDefinition};
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
|
||||
// --- Tests ---
|
||||
|
|
@ -1446,6 +1449,35 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn request_omits_system_message_when_prompt_empty() {
|
||||
let provider = Arc::new(CapturingLlmProvider::new());
|
||||
let provider_ref = provider.clone();
|
||||
let client = make_client(provider as Arc<dyn ProviderAdapter>).await;
|
||||
let profile = Arc::new(TestProfile::new());
|
||||
let env = Arc::new(MockSandbox::default());
|
||||
let mut session = Session::new(client, profile, env, SessionConfig::default());
|
||||
|
||||
// Intentionally skip initialize(): system prompt remains empty.
|
||||
session.process_input("test").await.unwrap();
|
||||
|
||||
let captured = provider_ref.captured_request.lock().unwrap();
|
||||
let request = captured
|
||||
.as_ref()
|
||||
.expect("request should have been captured");
|
||||
assert!(
|
||||
request
|
||||
.messages
|
||||
.iter()
|
||||
.all(|message| message.role != Role::System),
|
||||
"request should not contain an empty system message"
|
||||
);
|
||||
assert!(
|
||||
matches!(request.messages.first(), Some(message) if message.role == Role::User),
|
||||
"first request message should be user input"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tool_approval_denies_tool() {
|
||||
let mut registry = ToolRegistry::new();
|
||||
|
|
|
|||
|
|
@ -100,6 +100,7 @@ impl SubAgentManager {
|
|||
|
||||
let task_prompt_for_spawn = task_prompt.clone();
|
||||
let task = tokio::spawn(async move {
|
||||
session.initialize().await;
|
||||
session.process_input(&task_prompt_for_spawn).await?;
|
||||
let turns = session.history().turns();
|
||||
let last_text = turns.iter().rev().find_map(|t| match t {
|
||||
|
|
@ -369,7 +370,10 @@ pub fn make_close_agent_tool(manager: Arc<tokio::sync::Mutex<SubAgentManager>>)
|
|||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::config::SessionConfig;
|
||||
use crate::test_support::*;
|
||||
use arc_llm::provider::ProviderAdapter;
|
||||
use arc_llm::types::Role;
|
||||
|
||||
// --- Tests ---
|
||||
|
||||
|
|
@ -391,6 +395,36 @@ mod tests {
|
|||
assert!(manager.get(&agent_id).is_some());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn spawn_initializes_session_before_processing_input() {
|
||||
let mut manager = SubAgentManager::new(3);
|
||||
|
||||
let provider = Arc::new(CapturingLlmProvider::new());
|
||||
let provider_ref = provider.clone();
|
||||
let client = make_client(provider as Arc<dyn ProviderAdapter>).await;
|
||||
let profile = Arc::new(TestProfile::new());
|
||||
let env = Arc::new(MockSandbox::default());
|
||||
let session = Session::new(client, profile, env, SessionConfig::default());
|
||||
|
||||
let agent_id = manager.spawn(session, "Do something".into(), 0).unwrap();
|
||||
let _ = manager.wait(&agent_id).await.unwrap();
|
||||
|
||||
let captured = provider_ref.captured_request.lock().unwrap();
|
||||
let request = captured
|
||||
.as_ref()
|
||||
.expect("request should have been captured");
|
||||
let system_message = request
|
||||
.messages
|
||||
.iter()
|
||||
.find(|message| message.role == Role::System)
|
||||
.expect("subagent request should include system message");
|
||||
|
||||
assert!(
|
||||
!system_message.text().trim().is_empty(),
|
||||
"subagent system prompt should not be empty"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn depth_limit_enforced() {
|
||||
let mut manager = SubAgentManager::new(2);
|
||||
|
|
|
|||
|
|
@ -1044,11 +1044,13 @@ fn build_api_request(
|
|||
|
||||
let auto_cache = is_auto_cache_enabled(request.provider_options.as_ref());
|
||||
|
||||
let mut system_value = system.map(|s| {
|
||||
if auto_cache {
|
||||
system_with_cache_control(&s)
|
||||
let mut system_value = system.and_then(|s| {
|
||||
if s.trim().is_empty() {
|
||||
None
|
||||
} else if auto_cache {
|
||||
Some(system_with_cache_control(&s))
|
||||
} else {
|
||||
serde_json::Value::String(s)
|
||||
Some(serde_json::Value::String(s))
|
||||
}
|
||||
});
|
||||
|
||||
|
|
@ -1581,6 +1583,32 @@ mod tests {
|
|||
assert_eq!(arr[0]["cache_control"]["type"], "ephemeral");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_api_request_omits_whitespace_only_system_prompt() {
|
||||
let adapter = Adapter::new("test-key");
|
||||
let request = Request {
|
||||
model: "claude-sonnet-4-20250514".to_string(),
|
||||
messages: vec![Message::system(" \n\t"), Message::user("Hello")],
|
||||
provider: Some("anthropic".to_string()),
|
||||
tools: None,
|
||||
tool_choice: None,
|
||||
response_format: None,
|
||||
temperature: None,
|
||||
top_p: None,
|
||||
max_tokens: Some(128),
|
||||
stop_sequences: None,
|
||||
reasoning_effort: None,
|
||||
metadata: None,
|
||||
provider_options: None,
|
||||
};
|
||||
|
||||
let (api_request, _req_builder) = build_api_request(&adapter, &request, false);
|
||||
assert!(
|
||||
api_request.system.is_none(),
|
||||
"whitespace-only system prompts should be omitted"
|
||||
);
|
||||
}
|
||||
|
||||
fn make_request_with_format(format: crate::types::ResponseFormat) -> Request {
|
||||
Request {
|
||||
model: "claude-sonnet-4-20250514".to_string(),
|
||||
|
|
|
|||
|
|
@ -42,7 +42,10 @@ pub fn extract_system_prompt(messages: &[Message]) -> (Option<String>, Vec<&Mess
|
|||
let mut other = Vec::new();
|
||||
for msg in messages {
|
||||
if msg.role == Role::System || msg.role == Role::Developer {
|
||||
system_parts.push(msg.text());
|
||||
let text = msg.text();
|
||||
if !text.trim().is_empty() {
|
||||
system_parts.push(text);
|
||||
}
|
||||
} else {
|
||||
other.push(msg);
|
||||
}
|
||||
|
|
@ -471,6 +474,21 @@ mod tests {
|
|||
assert_eq!(other.len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_system_prompt_ignores_whitespace_system_and_developer() {
|
||||
let dev = Message {
|
||||
role: Role::Developer,
|
||||
content: vec![ContentPart::text(" \n\t ")],
|
||||
name: None,
|
||||
tool_call_id: None,
|
||||
};
|
||||
let msgs = vec![Message::system(" "), dev, Message::user("hi")];
|
||||
let (sys, other) = extract_system_prompt(&msgs);
|
||||
assert_eq!(sys, None);
|
||||
assert_eq!(other.len(), 1);
|
||||
assert_eq!(other[0].role, Role::User);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_system_prompt_empty() {
|
||||
let msgs: Vec<Message> = vec![];
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue