From 2a51a236ca2a6396b29d362fe6e7635d96874713 Mon Sep 17 00:00:00 2001 From: Fimeg Date: Thu, 13 Aug 2026 20:39:31 -0400 Subject: [PATCH] replay stored history through one projection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three implementations decided what a stored message becomes on the wire. core::session::replay_messages is now the only one; ImagePolicy is the sole legitimate difference between callers, and server/conversation.rs was dead scaffolding carrying a fourth wrong answer. Per-block replay split one assistant turn's several tool calls into adjacent messages, which OpenAI-shaped providers reject — one wire message per stored message is load-bearing, not cosmetic (2026-08-13). --- src/bridge/bifrost.rs | 16 +- src/core/session/mod.rs | 293 ++++++++++++++++++++++++++++--------- src/server/conversation.rs | 128 ---------------- src/server/mod.rs | 1 - src/server/turn.rs | 100 ++----------- 5 files changed, 252 insertions(+), 286 deletions(-) delete mode 100644 src/server/conversation.rs diff --git a/src/bridge/bifrost.rs b/src/bridge/bifrost.rs index 437b80d..efa8491 100644 --- a/src/bridge/bifrost.rs +++ b/src/bridge/bifrost.rs @@ -150,10 +150,13 @@ impl Message { } } - /// Multimodal user message with text + image content parts. - pub fn multimodal_user(_text: impl Into, parts: Vec) -> Self { + /// Multimodal message with text + image content parts, under any role. + /// + /// The role must be carried: an assistant message holding an image + /// replayed as `user` reads to the model as though the human said it. + pub fn multimodal(role: impl Into, parts: Vec) -> Self { Self { - role: "user".to_string(), + role: role.into(), content: ContentValue::Parts(parts), tool_calls: None, tool_call_id: None, @@ -162,6 +165,13 @@ impl Message { } } + /// Multimodal user message with text + image content parts. + /// + /// The text argument is ignored — the parts array is the whole content. + pub fn multimodal_user(_text: impl Into, parts: Vec) -> Self { + Self::multimodal("user", parts) + } + /// Assistant message that called tools. `content` may be empty. pub fn assistant_tool_calls(content: impl Into, calls: Vec) -> Self { Self { diff --git a/src/core/session/mod.rs b/src/core/session/mod.rs index a502ea9..a0fcb35 100644 --- a/src/core/session/mod.rs +++ b/src/core/session/mod.rs @@ -319,78 +319,110 @@ impl Session { Ok(Some(session)) } - /// Convert to Bifrost API message format (list of {role, content} maps). + /// Convert to Bifrost API message format. /// - /// NOTE: This produces text-only history. Real tool-call rounds are - /// handled inline inside the per-turn loop in `backend::local::run_turn` - /// (which builds proper assistant/tool messages with tool_call_id - /// linkage). This helper is for cross-turn context, where the model - /// only sees the assistant's final text — not the in-flight tool calls. + /// Delegates to [`replay_messages`] — the one projection of stored history + /// onto the wire. This wrapper degrades images, because the callers that + /// can see natively build their own policy from the agent's config. pub fn to_bifrost_messages(&self) -> Vec { - let mut messages = Vec::new(); - for msg in &self.messages { - for block in &msg.blocks { - match block { - ContentBlock::Text { text } => { - let role = match msg.role { - MessageRole::System => "system", - MessageRole::User => "user", - MessageRole::Assistant => "assistant", - // Cross-turn history is deliberately text-only. - // A role=tool message without tool_call_id is - // rejected by OpenAI-shaped providers, so even a - // legacy text block stored under Tool must replay - // as assistant context. - MessageRole::Tool => "assistant", - }; - messages.push(crate::bridge::bifrost::Message::text(role, text.clone())); - } - ContentBlock::ToolUse { name, input, .. } => { - // Tool calls flattened into assistant prose for - // cross-turn context — see fn-level note above. - messages.push(crate::bridge::bifrost::Message::text( - "assistant", - format!("Tool use: {name}({input})"), - )); - } - ContentBlock::ToolResult { - tool_name, - output, - is_error, - .. - } => { - let body = if *is_error { - format!("Error ({tool_name}): {output}") - } else { - format!("Result ({tool_name}): {output}") - }; - // Flatten into assistant prose rather than role=tool - // — without a tool_call_id link, role=tool is rejected - // by many providers on the next turn. - messages.push(crate::bridge::bifrost::Message::text("assistant", body)); - } - ContentBlock::Reasoning { reasoning } => { - messages.push(crate::bridge::bifrost::Message::text( - "assistant", - format!("[Reasoning]: {reasoning}"), - )); - } - ContentBlock::Image { media_type, .. } => { - // Cross-turn context: image is a text marker, not the - // full base64 payload. The per-turn path in run_turn - // sends the actual image data as ContentPart::ImageUrl. - messages.push(crate::bridge::bifrost::Message::text( - "user", - format!("[Image: {media_type}]"), - )); - } - } - } - } - messages + replay_messages(&self.messages, ImagePolicy::Degrade) } } +/// How a window carries an image. +/// +/// The only thing that legitimately varies between callers replaying history. +/// Everything else about the record is the same record. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ImagePolicy { + /// The model can see. Images ride as native multipart content. + Native, + /// The model cannot see. Images become a marker saying so plainly, rather + /// than a stub shaped like success. + Degrade, +} + +/// The one projection of stored history onto the wire. +/// +/// There is a single history. Both modes — her, and the subconscious a moment +/// later — replay the same record; a terminal, a voice and a web surface are +/// windows onto the same conversation. What varies is only what the glass can +/// carry, which is `images`. +/// +/// Two properties are load-bearing and were each learned from a failure: +/// +/// **One wire message per stored message.** Exploding a message into one wire +/// message per block splits a single assistant turn that called several tools +/// into adjacent assistant messages, which OpenAI-shaped providers reject. +/// +/// **No `role: "tool"` and no tool_call_id obligations.** A `role=tool` +/// message without its id is rejected outright, and a persisted `ToolUse` +/// whose result never landed — a turn killed mid-round, which has happened +/// twice — would replay as an orphaned half of a pair and fail the request. +/// So the *content* of a tool round survives as prose while the wire +/// obligations do not. Exact tool-call linkage matters inside the live loop, +/// where both halves are known to exist; it is brittle across persisted turns. +pub fn replay_messages( + messages: &[ConversationMessage], + images: ImagePolicy, +) -> Vec { + use crate::bridge::bifrost::{ContentPart, ImageUrlSource, Message}; + + messages + .iter() + .map(|m| { + let role = match m.role { + MessageRole::System => "system", + MessageRole::User => "user", + MessageRole::Assistant => "assistant", + // Replayed as assistant context: see the fn-level note. + MessageRole::Tool => "assistant", + }; + + let has_images = m + .blocks + .iter() + .any(|b| matches!(b, ContentBlock::Image { .. })); + + if has_images && images == ImagePolicy::Native { + // Every non-image block still contributes its prose, so an + // image in a message never costs the tool work beside it. + let parts: Vec = m + .blocks + .iter() + .filter_map(|b| match b { + ContentBlock::Image { media_type, data } => Some(ContentPart::ImageUrl { + image_url: ImageUrlSource { + url: format!("data:{media_type};base64,{data}"), + }, + }), + other => other.replay_text().map(|text| ContentPart::Text { text }), + }) + .collect(); + return Message::multimodal(role, parts); + } + + let content = m + .blocks + .iter() + .map(|b| match b { + // Degraded: named, and named as unseen. A model told it + // did not look can say so; a silent drop cannot. + ContentBlock::Image { media_type, .. } => { + Some(format!("[Image: {media_type} — not visible to this model]")) + } + other => other.replay_text(), + }) + .collect::>() + .into_iter() + .flatten() + .collect::>() + .join("\n"); + Message::text(role, content) + }) + .collect() +} + impl Default for Session { fn default() -> Self { Self::new("system") @@ -549,4 +581,131 @@ mod tests { .iter() .any(|m| m.content.as_text().contains("Result (read): A"))); } + + /// One stored message becomes exactly one wire message. + /// + /// The old per-block projection split a single assistant turn that called + /// several tools into adjacent assistant messages, which OpenAI-shaped + /// providers reject outright. + #[test] + fn a_message_that_called_two_tools_stays_one_message() { + let stored = vec![ConversationMessage { + role: MessageRole::Assistant, + blocks: vec![ + ContentBlock::Text { + text: "looking".into(), + }, + ContentBlock::ToolUse { + id: "call-a".into(), + name: "read".into(), + input: r#"{"path":"a"}"#.into(), + }, + ContentBlock::ToolUse { + id: "call-b".into(), + name: "grep".into(), + input: r#"{"pattern":"x"}"#.into(), + }, + ], + usage: None, + timestamp: None, + }]; + + for policy in [ImagePolicy::Native, ImagePolicy::Degrade] { + let msgs = replay_messages(&stored, policy); + assert_eq!(msgs.len(), 1, "{policy:?} exploded one message into many"); + let body = msgs[0].content.as_text(); + assert!(body.contains("looking"), "{body}"); + assert!(body.contains("read"), "{body}"); + assert!(body.contains("grep"), "{body}"); + } + } + + /// Both modes replay the same record. Only the glass differs. + #[test] + fn the_only_difference_between_policies_is_the_image() { + let stored = vec![ + ConversationMessage::user_text("hello"), + ConversationMessage { + role: MessageRole::Assistant, + blocks: vec![ + ContentBlock::Reasoning { + reasoning: "weighing".into(), + }, + ContentBlock::Text { + text: "hi there".into(), + }, + ], + usage: None, + timestamp: None, + }, + ]; + + let native = replay_messages(&stored, ImagePolicy::Native); + let degraded = replay_messages(&stored, ImagePolicy::Degrade); + assert_eq!(native.len(), degraded.len()); + for (a, b) in native.iter().zip(degraded.iter()) { + assert_eq!(a.role, b.role); + assert_eq!(a.content.as_text(), b.content.as_text()); + } + } + + /// A model that cannot see is told so, rather than handed a stub shaped + /// like success — and the tool work beside the image is not lost with it. + #[test] + fn a_degraded_image_says_it_was_not_seen() { + let stored = vec![ConversationMessage { + role: MessageRole::User, + blocks: vec![ + ContentBlock::Text { + text: "what is this".into(), + }, + ContentBlock::Image { + media_type: "image/png".into(), + data: "AAAA".into(), + }, + ], + usage: None, + timestamp: None, + }]; + + let body = replay_messages(&stored, ImagePolicy::Degrade)[0] + .content + .as_text(); + assert!(body.contains("what is this"), "{body}"); + assert!(body.contains("image/png"), "{body}"); + assert!(body.contains("not visible"), "{body}"); + assert!(!body.contains("AAAA"), "base64 must not ride in prose: {body}"); + } + + /// An assistant message carrying an image must not replay as the human's. + #[test] + fn a_native_image_keeps_the_role_that_produced_it() { + let stored = vec![ConversationMessage { + role: MessageRole::Assistant, + blocks: vec![ + ContentBlock::ToolResult { + tool_use_id: "call-1".into(), + tool_name: "read".into(), + output: "opened idle.png".into(), + is_error: false, + }, + ContentBlock::Image { + media_type: "image/png".into(), + data: "AAAA".into(), + }, + ], + usage: None, + timestamp: None, + }]; + + let msgs = replay_messages(&stored, ImagePolicy::Native); + assert_eq!(msgs.len(), 1); + assert_eq!( + msgs[0].role, "assistant", + "an image replayed under the wrong role reads as though the \ + human said it" + ); + // The tool work beside the image survives alongside it. + assert!(msgs[0].content.as_text().contains("opened idle.png")); + } } diff --git a/src/server/conversation.rs b/src/server/conversation.rs deleted file mode 100644 index 6047ba6..0000000 --- a/src/server/conversation.rs +++ /dev/null @@ -1,128 +0,0 @@ -#![allow(dead_code)] // WIP scaffolding not yet wired -//! Server Conversation Handler -//! -//! Simplified conversation flow for server mode — one turn without the full -//! tool loop. Used for lightweight server-side interactions (the SSE streaming -//! path uses `server::turn::run_turn` for the full tool loop). -//! -//! Currently not called by any active code path (May 2026); kept as a -//! lighter-weight alternative to `run_turn` for future use. - -use std::sync::Arc; - -use crate::bridge::bifrost::{ - ChatCompletionRequest, ContentPart, ImageUrlSource, Message as BifrostMessage, -}; -use crate::bridge::LlmProvider; -use crate::core::session::{ContentBlock, ConversationMessage, Session}; - -pub struct ServerConversation { - pub session: Session, - provider: Arc, - model: String, -} - -pub struct ServerTurnResult { - pub response_text: String, -} - -impl ServerConversation { - pub fn new(agent_name: &str, provider: Arc, model: String) -> Self { - Self { - session: Session::new(agent_name), - provider, - model, - } - } - - /// Simple turn without full tool loop (for now) - pub async fn turn(&mut self, user_input: &str) -> anyhow::Result { - // Add user message - self.session - .add_message(ConversationMessage::user_text(user_input)); - - // Build messages — flatten text blocks; ignore tool blocks - // until the server tool loop lands. - let messages: Vec = self - .session - .messages - .iter() - .map(|m| { - let role = match m.role { - crate::core::session::MessageRole::System => "system", - crate::core::session::MessageRole::User => "user", - crate::core::session::MessageRole::Assistant => "assistant", - crate::core::session::MessageRole::Tool => "tool", - }; - - let has_images = m - .blocks - .iter() - .any(|b| matches!(b, ContentBlock::Image { .. })); - - if has_images { - let parts: Vec = m - .blocks - .iter() - .filter_map(|b| match b { - ContentBlock::Text { text } => { - Some(ContentPart::Text { text: text.clone() }) - } - ContentBlock::Image { media_type, data } => { - let url = format!("data:{media_type};base64,{data}"); - Some(ContentPart::ImageUrl { - image_url: ImageUrlSource { url }, - }) - } - _ => None, - }) - .collect(); - - let text_content = m - .blocks - .iter() - .filter_map(|b| match b { - ContentBlock::Text { text } => Some(text.as_str()), - _ => None, - }) - .collect::>() - .join("\n"); - - BifrostMessage::multimodal_user(text_content, parts) - } else { - let content = m - .blocks - .iter() - .filter_map(|b| match b { - ContentBlock::Text { text } => Some(text.as_str()), - _ => None, - }) - .collect::>() - .join("\n"); - BifrostMessage::text(role, content) - } - }) - .collect(); - - // Call the active provider - let req = ChatCompletionRequest { - model: self.model.clone(), - messages, - stream: Some(false), - max_tokens: None, - temperature: None, - tools: None, - }; - - let response = self.provider.chat_completion(req).await?; - let content = response.content.clone(); - - // Store assistant response - self.session - .add_message(ConversationMessage::assistant_text(&content)); - - Ok(ServerTurnResult { - response_text: content, - }) - } -} diff --git a/src/server/mod.rs b/src/server/mod.rs index 8903cb6..ac20f99 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -12,7 +12,6 @@ use tokio::sync::{Mutex, RwLock}; pub mod agent_inventory; pub mod consciousness_engine; -pub mod conversation; pub mod db; pub mod device_registry; pub mod energy; diff --git a/src/server/turn.rs b/src/server/turn.rs index b189e41..9a4048c 100644 --- a/src/server/turn.rs +++ b/src/server/turn.rs @@ -10,7 +10,7 @@ use crate::bridge::bifrost::{ use crate::bridge::model_router::TokenCounter; use crate::core::compact::CompactionEngine; use crate::core::nervous::{EventBus, SensorEvent}; -use crate::core::session::{ContentBlock, ConversationMessage, MessageRole, TokenUsage}; +use crate::core::session::{ContentBlock, ConversationMessage, ImagePolicy, TokenUsage}; use crate::core::tools::defs::ToolContext; use crate::server::consciousness_engine::ConsciousnessEvent; use crate::server::SouveraineServer; @@ -115,97 +115,23 @@ pub(crate) async fn run_turn( // Snapshot history for the Bifrost call, then drop the dashmap ref before // any await — `Ref` is not Send across awaits. + // + // One projection, shared with every other replay of stored history + // (`core::session::replay_messages`). The agent's vision capability is the + // only thing this caller contributes. let initial_messages = { let session = server .sessions .get(&conversation_id) .ok_or_else(|| anyhow::anyhow!("Session not found: {}", conversation_id))?; - let messages: Vec = session - .messages - .iter() - .map(|m| { - let role = match m.role { - MessageRole::System => "system", - MessageRole::User => "user", - MessageRole::Assistant => "assistant", - // Cross-turn history is text-only, so a role=tool message - // would arrive without the tool_call_id that - // OpenAI-shaped providers require and be rejected. - // Replay it as assistant context instead — same ruling as - // Session::to_bifrost_messages. - MessageRole::Tool => "assistant", - }; - - // If the model doesn't support images, strip Image blocks - // and replace with text markers. - if !supports_images { - let has_images = m - .blocks - .iter() - .any(|b| matches!(b, ContentBlock::Image { .. })); - if has_images { - let mut text_parts: Vec = Vec::new(); - for b in &m.blocks { - if let Some(prose) = b.replay_text() { - text_parts.push(prose); - } - } - let content = text_parts.join("\n"); - // Append text markers for stripped images - let img_count = m - .blocks - .iter() - .filter(|b| matches!(b, ContentBlock::Image { .. })) - .count(); - let mut enriched = content; - for _ in 0..img_count { - enriched.push_str("\n[Image: attached by user]"); - } - return BifrostMessage::text(role, enriched); - } - } - - // Check if this message has image content blocks - let has_images = m - .blocks - .iter() - .any(|b| matches!(b, ContentBlock::Image { .. })); - - if has_images { - // Build multimodal content parts (OpenAI multi-part format). - // Every non-image block contributes its replay prose so an - // image in a message never costs the tool work beside it. - let parts: Vec = m - .blocks - .iter() - .filter_map(|b| match b { - ContentBlock::Image { media_type, data } => { - let url = format!("data:{media_type};base64,{data}"); - Some(ContentPart::ImageUrl { - image_url: ImageUrlSource { url }, - }) - } - other => other - .replay_text() - .map(|text| ContentPart::Text { text }), - }) - .collect(); - - // `multimodal_user` ignores its text argument — the parts - // array is the whole content. - BifrostMessage::multimodal_user(String::new(), parts) - } else { - let content = m - .blocks - .iter() - .filter_map(|b| b.replay_text()) - .collect::>() - .join("\n"); - BifrostMessage::text(role, content) - } - }) - .collect(); - messages + crate::core::session::replay_messages( + &session.messages, + if supports_images { + ImagePolicy::Native + } else { + ImagePolicy::Degrade + }, + ) }; let max_rounds = agent.llm_config.max_tool_rounds; let model = agent.llm_config.model.clone();