-
Notifications
You must be signed in to change notification settings - Fork 16
feat(openai): prompt cache breakpoints, and bump tinytools for ToolExposure #146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
edda552
21bfda6
f169f37
7daaad2
adc94df
96053b7
f6d6496
485703f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,9 +97,14 @@ fn normalize_tool_call_id(id: &str) -> String { | |
| /// [`TinyAgentsError::Validation`] instead of being discarded. | ||
| pub(super) fn translate_message(message: &Message) -> Result<ChatMessageWire> { | ||
| let wire = match message { | ||
| Message::System(_) => ChatMessageWire { | ||
| // Routed through the same content translation as user messages rather | ||
| // than `message.text()`. `text()` concatenates only the text blocks, so | ||
| // it silently discards a `CacheBreakpoint` — which is precisely the | ||
| // message that carries one, since the system prompt is the stable | ||
| // prefix worth caching. | ||
| Message::System(system) => ChatMessageWire { | ||
| role: "system".to_string(), | ||
| content: Some(MessageContentWire::Text(message.text())), | ||
| content: Some(translate_user_content(&system.content)?), | ||
| tool_calls: Vec::new(), | ||
| tool_call_id: None, | ||
| }, | ||
|
|
@@ -158,19 +163,29 @@ pub(super) fn translate_message(message: &Message) -> Result<ChatMessageWire> { | |
| /// representation, so it fails closed with a validation error rather than being | ||
| /// silently dropped. | ||
| pub(super) fn translate_user_content(blocks: &[ContentBlock]) -> Result<MessageContentWire> { | ||
| let has_image = blocks | ||
| .iter() | ||
| .any(|block| matches!(block, ContentBlock::Image(_))); | ||
|
|
||
| if !has_image { | ||
| // Two things force the content-parts shape: an image, which has no string | ||
| // representation, and a declared cache breakpoint, which is an attribute of | ||
| // a *part* and therefore cannot be expressed on a bare string. | ||
| let needs_parts = blocks.iter().any(|block| { | ||
| matches!( | ||
| block, | ||
| ContentBlock::Image(_) | ContentBlock::CacheBreakpoint | ||
| ) | ||
| }); | ||
|
|
||
| if !needs_parts { | ||
| // No image: render as a single string, but still fail closed on blocks | ||
| // that cannot be represented. | ||
| let mut text = String::new(); | ||
| for block in blocks { | ||
| match block { | ||
| ContentBlock::Text(t) => text.push_str(t), | ||
| ContentBlock::Json(value) => text.push_str(&value.to_string()), | ||
| ContentBlock::Image(_) => unreachable!("guarded by has_image"), | ||
| ContentBlock::Image(_) => unreachable!("guarded by needs_parts"), | ||
| // Unreachable for the same reason, and deliberately not folded | ||
| // into the drop arm below: a breakpoint that silently vanished | ||
| // would leave the caller believing the request was cached. | ||
| ContentBlock::CacheBreakpoint => unreachable!("guarded by needs_parts"), | ||
| // OpenAI-compatible requests have no representation for | ||
| // reasoning blocks; they are dropped rather than failing the | ||
| // request (matching the assistant path, which serializes via | ||
|
|
@@ -187,9 +202,13 @@ pub(super) fn translate_user_content(blocks: &[ContentBlock]) -> Result<MessageC | |
| let mut parts = Vec::with_capacity(blocks.len()); | ||
| for block in blocks { | ||
| match block { | ||
| ContentBlock::Text(t) => parts.push(ContentPartWire::Text { text: t.clone() }), | ||
| ContentBlock::Text(t) => parts.push(ContentPartWire::Text { | ||
| text: t.clone(), | ||
| cache_control: None, | ||
| }), | ||
| ContentBlock::Json(value) => parts.push(ContentPartWire::Text { | ||
| text: value.to_string(), | ||
| cache_control: None, | ||
| }), | ||
| ContentBlock::Image(image) => parts.push(ContentPartWire::ImageUrl { | ||
| image_url: ImageUrlWire { | ||
|
|
@@ -199,14 +218,73 @@ pub(super) fn translate_user_content(blocks: &[ContentBlock]) -> Result<MessageC | |
| // See the string-rendering arm above: reasoning blocks have no | ||
| // OpenAI representation and are dropped, not failed. | ||
| ContentBlock::Thinking { .. } | ContentBlock::RedactedThinking { .. } => {} | ||
| // The marker attaches to the part it follows: the provider caches | ||
| // through the end of that block. A leading breakpoint (nothing to | ||
| // mark) is a caller mistake with no safe interpretation, so it is | ||
| // dropped rather than guessed at — caching the empty prefix and | ||
| // caching the whole message are both wrong. | ||
| ContentBlock::CacheBreakpoint => match parts.last_mut() { | ||
| Some(ContentPartWire::Text { cache_control, .. }) => { | ||
| *cache_control = Some(CacheControlWire::Ephemeral); | ||
| } | ||
| Some(ContentPartWire::ImageUrl { .. }) | None => { | ||
| tracing::warn!( | ||
| "[openai] ignoring a CacheBreakpoint with no preceding text part; \ | ||
| place it after the content it should make cacheable" | ||
| ); | ||
| } | ||
| }, | ||
| ContentBlock::ProviderExtension(_) => { | ||
| return Err(unrepresentable_block_error()); | ||
| } | ||
| } | ||
| } | ||
| enforce_breakpoint_limit(&mut parts); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When breakpoints are distributed across multiple messages, this caps each message independently because AGENTS.md reference: AGENTS.md:L62-L65 Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Enforce the breakpoint limit across the complete request. When an Anthropic-compatible endpoint receives four marked system parts and one marked user part, per-message enforcement preserves all five markers. The endpoint can reject the request because its contract permits at most four breakpoints per request. Count markers across all assembled 🤖 Prompt for AI Agents |
||
| Ok(MessageContentWire::Parts(parts)) | ||
| } | ||
|
|
||
| /// Anthropic accepts at most four `cache_control` breakpoints per request and | ||
| /// rejects the whole request with a 400 beyond that. | ||
| /// | ||
| /// Keep the **last** four. Each breakpoint caches from the start of the request | ||
| /// through its own block, so the later ones cover strictly longer prefixes and | ||
| /// are strictly more valuable; dropping from the front loses the least. A | ||
| /// caller declaring more than four has a prompt-assembly bug, so this warns | ||
| /// rather than trimming silently — but it trims, because a 400 on every turn is | ||
| /// a worse failure than a smaller cache. | ||
| fn enforce_breakpoint_limit(parts: &mut [ContentPartWire]) { | ||
| const MAX_BREAKPOINTS: usize = 4; | ||
| let marked: Vec<usize> = parts | ||
| .iter() | ||
| .enumerate() | ||
| .filter(|(_, part)| { | ||
| matches!( | ||
| part, | ||
| ContentPartWire::Text { | ||
| cache_control: Some(_), | ||
| .. | ||
| } | ||
| ) | ||
| }) | ||
| .map(|(index, _)| index) | ||
| .collect(); | ||
| if marked.len() <= MAX_BREAKPOINTS { | ||
| return; | ||
| } | ||
| let drop_count = marked.len() - MAX_BREAKPOINTS; | ||
| tracing::warn!( | ||
| declared = marked.len(), | ||
| kept = MAX_BREAKPOINTS, | ||
| "[openai] more cache breakpoints than the provider accepts; keeping the \ | ||
| last {MAX_BREAKPOINTS} (longest prefixes) and dropping the earliest {drop_count}" | ||
| ); | ||
| for &index in &marked[..drop_count] { | ||
| if let Some(ContentPartWire::Text { cache_control, .. }) = parts.get_mut(index) { | ||
| *cache_control = None; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Error returned when a content block cannot be represented in an OpenAI | ||
| /// request. Failing closed keeps the block from being silently dropped. | ||
| pub(super) fn unrepresentable_block_error() -> TinyAgentsError { | ||
|
|
||
| +1 −1 | crates/tinytools/src/classification/mod.rs | |
| +56 −0 | crates/tinytools/src/classification/types.rs | |
| +2 −2 | crates/tinytools/src/lib.rs | |
| +12 −1 | crates/tinytools/src/tool/types.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge tinyhumansai/tinyagents /tmp/coderabbit-repo-knowledge/tinyhumansai-tinyagents-e004d811Length of output: 361
🏁 Script executed:
Repository: tinyhumansai/tinyagents
Length of output: 50380
🏁 Script executed:
Repository: tinyhumansai/tinyagents
Length of output: 50380
🏁 Script executed:
Repository: tinyhumansai/tinyagents
Length of output: 41544
🏁 Script executed:
Repository: tinyhumansai/tinyagents
Length of output: 27872
🏁 Script executed:
Repository: tinyhumansai/tinyagents
Length of output: 19905
Gate
cache_controlserialization by provider capability.translate_messagesends both system and user content throughtranslate_user_contentwithout a provider capability. Therefore,ContentBlock::CacheBreakpointforces multipart content and emitscache_controlfor everyOpenAiModelendpoint. This violates theContentBlockcontract for providers that use automatic caching or do not support the marker; those providers must preserve the previous wire shape. Pass an explicit cache-control capability to translation and add target-specific serialization tests.🤖 Prompt for AI Agents