read: attach the image, not a stub shaped like success
read base64'd every image into ToolOutput.raw and the registry dropped it, so vision never arrived — only "[Image: idle.png (283KB)]", which reads like success and is proceeded on. Images now ride as one user message after all tool results in a round; a model without vision is told plainly it did not see. Also stops "Error: Error:" doubling on prefixed failures.
This commit is contained in:
parent
713f3ac1b2
commit
41c858a5fc
4 changed files with 167 additions and 8 deletions
|
|
@ -1422,6 +1422,7 @@ pub async fn handle_memory_tool_with_context(
|
|||
command
|
||||
),
|
||||
is_error: true,
|
||||
image: None,
|
||||
};
|
||||
}
|
||||
};
|
||||
|
|
@ -1432,12 +1433,14 @@ pub async fn handle_memory_tool_with_context(
|
|||
tool_name: tool_name.to_string(),
|
||||
output,
|
||||
is_error: false,
|
||||
image: None,
|
||||
},
|
||||
Err(e) => ToolResult {
|
||||
tool_use_id,
|
||||
tool_name: tool_name.to_string(),
|
||||
output: format!("Error: {e}"),
|
||||
is_error: true,
|
||||
image: None,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -65,13 +65,48 @@ pub struct ToolDefinition {
|
|||
pub input_schema: serde_json::Value,
|
||||
}
|
||||
|
||||
/// An image a sensor actually looked at, carried out of the registry.
|
||||
///
|
||||
/// `ToolOutput` has always base64'd images into its `raw` field, and this
|
||||
/// struct is the reason that stopped being write-only. Before it, `read`
|
||||
/// encoded a PNG faithfully and `tool_result` dropped it on the floor, so the
|
||||
/// agent received `[Image: idle.png (283KB)]` — a stub shaped like success —
|
||||
/// and proceeded as though she had seen something.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub struct ToolImage {
|
||||
pub media_type: String,
|
||||
/// Base64, no data-URL prefix.
|
||||
pub data: String,
|
||||
}
|
||||
|
||||
impl ToolImage {
|
||||
/// Parse a `data:image/png;base64,...` URL, the shape `read` emits.
|
||||
/// Returns `None` for anything that is not a base64 image data URL —
|
||||
/// `raw` also carries non-image payloads (the `agent` sensor puts a
|
||||
/// request id there), so this must refuse rather than guess.
|
||||
pub fn from_data_url(raw: &str) -> Option<Self> {
|
||||
let rest = raw.strip_prefix("data:")?;
|
||||
let (media_type, payload) = rest.split_once(";base64,")?;
|
||||
if !media_type.starts_with("image/") || payload.is_empty() {
|
||||
return None;
|
||||
}
|
||||
Some(Self {
|
||||
media_type: media_type.to_string(),
|
||||
data: payload.to_string(),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/// Result of executing a tool — preserved from old interface.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
|
||||
pub struct ToolResult {
|
||||
pub tool_use_id: String,
|
||||
pub tool_name: String,
|
||||
pub output: String,
|
||||
pub is_error: bool,
|
||||
/// Present only when the sensor genuinely looked at an image.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub image: Option<ToolImage>,
|
||||
}
|
||||
|
||||
// ── Registry ────────────────────────────────────────────────────
|
||||
|
|
@ -193,6 +228,7 @@ impl Sensorium {
|
|||
.join(", ")
|
||||
),
|
||||
is_error: true,
|
||||
image: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -226,12 +262,14 @@ fn tool_result(tool_use_id: &str, name: &str, result: Result<ToolOutput, ToolErr
|
|||
tool_name: name.to_string(),
|
||||
output: output.content,
|
||||
is_error: output.is_error,
|
||||
image: output.raw.as_deref().and_then(ToolImage::from_data_url),
|
||||
},
|
||||
Err(err) => ToolResult {
|
||||
tool_use_id: tool_use_id.to_string(),
|
||||
tool_name: name.to_string(),
|
||||
output: err.to_string(),
|
||||
is_error: true,
|
||||
image: None,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
|
@ -274,6 +312,7 @@ pub async fn execute_tool_with_context(
|
|||
tool_name: tool_name.to_string(),
|
||||
output: format!("I couldn't understand the input: {e}"),
|
||||
is_error: true,
|
||||
image: None,
|
||||
};
|
||||
}
|
||||
};
|
||||
|
|
@ -369,3 +408,62 @@ mod tests {
|
|||
assert!(result.output.contains("don't have a sense"));
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod image_passthrough_tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn a_png_data_url_becomes_an_image_she_can_actually_receive() {
|
||||
let img = ToolImage::from_data_url("data:image/png;base64,iVBORw0KGgo=")
|
||||
.expect("a png data url is an image");
|
||||
assert_eq!(img.media_type, "image/png");
|
||||
assert_eq!(img.data, "iVBORw0KGgo=");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_agent_sensors_request_id_is_not_mistaken_for_a_picture() {
|
||||
// `raw` is a shared field: the `agent` sensor puts a request id in it.
|
||||
// Guessing here would attach garbage as an image block.
|
||||
assert!(ToolImage::from_data_url("req-01H8XYZ").is_none());
|
||||
assert!(ToolImage::from_data_url("").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_non_image_data_url_is_refused() {
|
||||
assert!(ToolImage::from_data_url("data:application/pdf;base64,JVBERi0=").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_payload_is_refused_rather_than_attached_as_a_blank_image() {
|
||||
assert!(ToolImage::from_data_url("data:image/png;base64,").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_failed_tool_carries_no_image() {
|
||||
let r = tool_result(
|
||||
"id-1",
|
||||
"read",
|
||||
Err(ToolError::io_error(
|
||||
std::path::PathBuf::from("/nope"),
|
||||
std::io::Error::new(std::io::ErrorKind::NotFound, "missing"),
|
||||
)),
|
||||
);
|
||||
assert!(r.is_error);
|
||||
assert!(r.image.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_sensor_that_looked_at_an_image_carries_it_out_of_the_registry() {
|
||||
// Before this, `read` base64'd the file into `raw` and the registry
|
||||
// dropped it — the agent got a stub shaped like success.
|
||||
let out = ToolOutput {
|
||||
content: "[Image: idle.png (283KB)]".to_string(),
|
||||
is_error: false,
|
||||
raw: Some("data:image/png;base64,iVBORw0KGgo=".to_string()),
|
||||
};
|
||||
let r = tool_result("id-2", "read", Ok(out));
|
||||
let img = r.image.expect("the image survives the registry boundary");
|
||||
assert_eq!(img.media_type, "image/png");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -245,12 +245,18 @@ When I specify a line range (like `file.rs:10-20`), I'm narrowing my attention t
|
|||
// Phase 2: Resolve path
|
||||
let resolved = ctx.resolve_path(&clean_path);
|
||||
|
||||
// Phase 3: Refuse memory territory — that's the memory sensor's domain
|
||||
// Phase 3: Refuse memory territory — that's the memory sensor's domain.
|
||||
// Except for binaries: the `memory` sensor reads text with frontmatter,
|
||||
// so referring a PNG there is a referral to a door that does not open.
|
||||
// Sending her round that loop and failing without a reason is worse
|
||||
// than refusing here and naming the way through.
|
||||
if !force && ctx.is_memory_path(&resolved) {
|
||||
return Err(ToolError::memory_boundary(
|
||||
resolved,
|
||||
let hint = if is_image(&resolved) || looks_binary(&resolved) {
|
||||
"This is a binary file in my memory territory. The `memory` sensor reads text with frontmatter and cannot open it — I should use `read` with `force: true` to reach it directly."
|
||||
} else {
|
||||
"This path is in my memory territory. I should use the `memory` sensor to read it — it handles frontmatter, git tracking, and structure."
|
||||
));
|
||||
};
|
||||
return Err(ToolError::memory_boundary(resolved, hint));
|
||||
}
|
||||
|
||||
// Phase 4: Image detection
|
||||
|
|
|
|||
|
|
@ -674,6 +674,12 @@ pub(crate) async fn run_turn(
|
|||
}
|
||||
|
||||
// Execute each tool and stream results back — now with per-agent context
|
||||
// Images a sensor genuinely looked at are collected here and sent as a
|
||||
// single user message *after* every tool result in this round — never
|
||||
// between them. The tool-use schema requires an assistant's tool_calls
|
||||
// to be answered by an unbroken run of tool results; a user message in
|
||||
// the middle breaks the pairing and the provider rejects the request.
|
||||
let mut round_images: Vec<crate::core::tools::ToolImage> = Vec::new();
|
||||
for tc in &response.tool_calls {
|
||||
let input_str = tc.arguments.to_string();
|
||||
persisted_blocks.push(ContentBlock::ToolUse {
|
||||
|
|
@ -695,16 +701,44 @@ pub(crate) async fn run_turn(
|
|||
round: tool_round,
|
||||
}))
|
||||
.await;
|
||||
let result =
|
||||
let mut result =
|
||||
crate::core::tools::execute_tool_with_context(&tc.name, &input_str, &tool_ctx)
|
||||
.await;
|
||||
dispatcher.emit_tool_end(&tc.name, &tc.id, result.is_error);
|
||||
|
||||
let output = if result.is_error {
|
||||
format!("Error: {}", result.output)
|
||||
// Take the image out before the text is assembled. Whether she can
|
||||
// actually see it is the model's property, not the sensor's, so the
|
||||
// sensor reports what it found and the turn decides what arrives.
|
||||
let image = result.image.take();
|
||||
|
||||
let base = if result.is_error {
|
||||
// `ToolError`'s own Display already opens with "Error:" for
|
||||
// several variants; prefixing unconditionally produced
|
||||
// `Error: Error: reading memory file`, which reads like two
|
||||
// failures stacked rather than one.
|
||||
if result.output.starts_with("Error:") {
|
||||
result.output
|
||||
} else {
|
||||
format!("Error: {}", result.output)
|
||||
}
|
||||
} else {
|
||||
result.output
|
||||
};
|
||||
// A stub that reads like success is worse than an honest refusal —
|
||||
// without this the agent receives `[Image: idle.png (283KB)]` and
|
||||
// proceeds as though she had looked at it.
|
||||
let output = match (&image, supports_images) {
|
||||
(Some(_), true) => format!("{base}\nThe image itself follows this round's results."),
|
||||
(Some(_), false) => format!(
|
||||
"{base}\nThis model has no vision, so the image did not reach me — I have not seen it."
|
||||
),
|
||||
(None, _) => base,
|
||||
};
|
||||
if supports_images {
|
||||
if let Some(img) = image {
|
||||
round_images.push(img);
|
||||
}
|
||||
}
|
||||
persisted_blocks.push(ContentBlock::ToolResult {
|
||||
tool_use_id: tc.id.clone(),
|
||||
tool_name: tc.name.clone(),
|
||||
|
|
@ -775,6 +809,24 @@ pub(crate) async fn run_turn(
|
|||
messages.push(BifrostMessage::tool_result(&tc.id, &tc.name, output));
|
||||
}
|
||||
|
||||
// Now that every tool result is bound, the images can follow.
|
||||
if !round_images.is_empty() {
|
||||
let parts: Vec<ContentPart> = round_images
|
||||
.iter()
|
||||
.map(|img| ContentPart::ImageUrl {
|
||||
image_url: ImageUrlSource {
|
||||
url: format!("data:{};base64,{}", img.media_type, img.data),
|
||||
},
|
||||
})
|
||||
.collect();
|
||||
let label = if round_images.len() == 1 {
|
||||
"The image I just read:".to_string()
|
||||
} else {
|
||||
format!("The {} images I just read:", round_images.len())
|
||||
};
|
||||
messages.push(BifrostMessage::multimodal_user(label, parts));
|
||||
}
|
||||
|
||||
// ── Mid-turn peek ────────────────────────────────────────────
|
||||
// Every checkpoint_interval rounds the subconscious peeks at the
|
||||
// live loop — same persistent agent, same memfs, full tool set.
|
||||
|
|
|
|||
Loading…
Reference in a new issue