insight-chat: extract bootstrap resolution helpers + unit-test them
resolve_bootstrap_system_prompt and resolve_bootstrap_backend run on every bootstrap turn — they pick the persisted system prompt and the chosen backend label. They were inline conditionals before; pulling them out makes the rules testable without spinning up the full streaming stack. 9 new tests cover: - system prompt fallback to BOOTSTRAP_DEFAULT_SYSTEM_PROMPT for None, empty string, whitespace-only - supplied non-empty prompts pass through verbatim, with interior newlines / spacing preserved (Apollo personas use multi-line tool listings) - backend defaults to "local" for None / empty - "local" / "hybrid" accepted case-insensitively with edge-trim - unknown labels return a descriptive error Total insight_chat tests: 24 (up from 15). No behaviour change.
This commit is contained in:
@@ -915,18 +915,7 @@ impl InsightChatService {
|
||||
normalized: String,
|
||||
tx: tokio::sync::mpsc::Sender<ChatStreamEvent>,
|
||||
) -> Result<()> {
|
||||
let effective_backend = req
|
||||
.backend
|
||||
.as_deref()
|
||||
.map(|s| s.trim().to_lowercase())
|
||||
.filter(|s| !s.is_empty())
|
||||
.unwrap_or_else(|| "local".to_string());
|
||||
if !matches!(effective_backend.as_str(), "local" | "hybrid") {
|
||||
bail!(
|
||||
"unknown backend '{}'; expected 'local' or 'hybrid'",
|
||||
effective_backend
|
||||
);
|
||||
}
|
||||
let effective_backend = resolve_bootstrap_backend(req.backend.as_deref())?;
|
||||
let is_hybrid = effective_backend == "hybrid";
|
||||
|
||||
let max_iterations = req
|
||||
@@ -976,13 +965,7 @@ impl InsightChatService {
|
||||
let tools = InsightGenerator::build_tool_definitions(gate_opts);
|
||||
|
||||
// Build initial messages: persona system + user-turn-with-image.
|
||||
let system_content = req
|
||||
.system_prompt
|
||||
.as_deref()
|
||||
.map(str::trim)
|
||||
.filter(|s| !s.is_empty())
|
||||
.unwrap_or(BOOTSTRAP_DEFAULT_SYSTEM_PROMPT)
|
||||
.to_string();
|
||||
let system_content = resolve_bootstrap_system_prompt(req.system_prompt.as_deref());
|
||||
let system_msg = ChatMessage::system(system_content);
|
||||
let mut user_msg = ChatMessage::user(user_content);
|
||||
if !is_hybrid && let Some(ref img) = image_base64 {
|
||||
@@ -1297,6 +1280,34 @@ const BOOTSTRAP_DEFAULT_SYSTEM_PROMPT: &str = "You are a helpful AI assistant an
|
||||
Use the available tools to gather context and answer their questions \
|
||||
in a conversational tone.";
|
||||
|
||||
/// Pick the system prompt for bootstrap. Trimmed-non-empty supplied wins;
|
||||
/// otherwise fall back to [`BOOTSTRAP_DEFAULT_SYSTEM_PROMPT`]. Returns an
|
||||
/// owned `String` because the bootstrap caller persists it on the new
|
||||
/// insight row.
|
||||
fn resolve_bootstrap_system_prompt(supplied: Option<&str>) -> String {
|
||||
supplied
|
||||
.map(str::trim)
|
||||
.filter(|s| !s.is_empty())
|
||||
.map(str::to_string)
|
||||
.unwrap_or_else(|| BOOTSTRAP_DEFAULT_SYSTEM_PROMPT.to_string())
|
||||
}
|
||||
|
||||
/// Pick the backend label for bootstrap. Bootstrap has no stored insight
|
||||
/// to defer to (that's continuation's behaviour), so the default is
|
||||
/// `"local"`. Returns an error if the supplied label is non-empty but
|
||||
/// not one of the recognised values — same surface as continuation's
|
||||
/// validation.
|
||||
fn resolve_bootstrap_backend(supplied: Option<&str>) -> Result<String> {
|
||||
let lower = supplied
|
||||
.map(|s| s.trim().to_lowercase())
|
||||
.filter(|s| !s.is_empty())
|
||||
.unwrap_or_else(|| "local".to_string());
|
||||
if !matches!(lower.as_str(), "local" | "hybrid") {
|
||||
bail!("unknown backend '{}'; expected 'local' or 'hybrid'", lower);
|
||||
}
|
||||
Ok(lower)
|
||||
}
|
||||
|
||||
/// Outcome of one streaming agentic loop pass. Shared between bootstrap
|
||||
/// and continuation.
|
||||
struct AgenticLoopOutcome {
|
||||
@@ -1840,4 +1851,73 @@ mod tests {
|
||||
assert_eq!(msgs.len(), 1);
|
||||
assert_eq!(msgs[0].role, "user");
|
||||
}
|
||||
|
||||
// ── Bootstrap prompt / backend resolution ─────────────────────────
|
||||
// Resolution helpers run on every bootstrap turn — they pick the
|
||||
// persisted system prompt and the chosen backend label. Bugs here
|
||||
// would silently swap the persona or miscategorise a backend.
|
||||
|
||||
#[test]
|
||||
fn bootstrap_system_prompt_falls_back_to_default_for_none() {
|
||||
let out = resolve_bootstrap_system_prompt(None);
|
||||
assert_eq!(out, BOOTSTRAP_DEFAULT_SYSTEM_PROMPT);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_system_prompt_falls_back_to_default_for_empty_string() {
|
||||
// Apollo currently sends `''` when no persona is selected.
|
||||
let out = resolve_bootstrap_system_prompt(Some(""));
|
||||
assert_eq!(out, BOOTSTRAP_DEFAULT_SYSTEM_PROMPT);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_system_prompt_falls_back_to_default_for_whitespace() {
|
||||
let out = resolve_bootstrap_system_prompt(Some(" \n\t "));
|
||||
assert_eq!(out, BOOTSTRAP_DEFAULT_SYSTEM_PROMPT);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_system_prompt_uses_supplied_when_non_empty() {
|
||||
let out = resolve_bootstrap_system_prompt(Some("you are a journal"));
|
||||
assert_eq!(out, "you are a journal");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_system_prompt_does_not_strip_inner_whitespace() {
|
||||
// Trim only happens at the edges — interior newlines and spacing
|
||||
// (which Apollo's persona uses for tool listings) must survive.
|
||||
let prompt = "line one\nline two\n bullet";
|
||||
let out = resolve_bootstrap_system_prompt(Some(prompt));
|
||||
assert_eq!(out, prompt);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_backend_defaults_to_local_when_none() {
|
||||
let out = resolve_bootstrap_backend(None).unwrap();
|
||||
assert_eq!(out, "local");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_backend_defaults_to_local_when_empty() {
|
||||
let out = resolve_bootstrap_backend(Some("")).unwrap();
|
||||
assert_eq!(out, "local");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_backend_accepts_local_and_hybrid_case_insensitively() {
|
||||
assert_eq!(resolve_bootstrap_backend(Some("LOCAL")).unwrap(), "local");
|
||||
assert_eq!(resolve_bootstrap_backend(Some("Hybrid")).unwrap(), "hybrid");
|
||||
assert_eq!(
|
||||
resolve_bootstrap_backend(Some(" local ")).unwrap(),
|
||||
"local"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_backend_rejects_unknown_label() {
|
||||
let err = resolve_bootstrap_backend(Some("openrouter")).unwrap_err();
|
||||
let msg = format!("{}", err);
|
||||
assert!(msg.contains("unknown backend"));
|
||||
assert!(msg.contains("openrouter"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user