Skip to content

refactor: move openai request helpers #48

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.

Sign up for GitHub

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

Open
jamadeo wants to merge 1 commit into
base: provider-crate-extraction-phase-15b-openai-response-stream-decoder
Choose a base branch
from
Open

refactor: move openai request helpers #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 176 additions & 1 deletion crates/goose-providers/src/openai.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use anyhow::anyhow;
use async_stream::try_stream;
use futures::Stream;
use goose_types::{Message, MessageContent, MessageProviderMetadata, ProviderUsage, Usage};
use rmcp::model::{object, CallToolRequestParams, ErrorCode, ErrorData, Role};
use rmcp::model::{object, CallToolRequestParams, ErrorCode, ErrorData, Role, Tool};
use serde::{Deserialize, Serialize};
use serde_json::{json, Value};
use std::borrow::Cow;
Expand Down Expand Up @@ -575,6 +575,132 @@ where
}
}

pub fn is_reserved_request_param_key(key: &str) -> bool {
matches!(key, "messages" | "model" | "stream" | "stream_options")
}

pub fn format_tools(tools: &[Tool]) -> anyhow::Result<Vec<Value>> {
let mut tool_names = std::collections::HashSet::new();
let mut result = Vec::new();

for tool in tools {
if !tool_names.insert(&tool.name) {
return Err(anyhow!("Duplicate tool name: {}", tool.name));
}

result.push(json!({
"type": "function",
"function": {
"name": tool.name,
"description": tool.description,
"parameters": tool.input_schema,
}
}));
}

Ok(result)
}

pub fn validate_tool_schemas(tools: &mut [Value]) {
for tool in tools.iter_mut() {
if let Some(function) = tool.get_mut("function") {
if let Some(parameters) = function.get_mut("parameters") {
if parameters.is_object() {
ensure_valid_json_schema(parameters);
}
}
}
}
}

/// Ensures that the given JSON value follows the expected JSON Schema structure.
fn ensure_valid_json_schema(schema: &mut Value) {
if let Some(params_obj) = schema.as_object_mut() {
// Check if this is meant to be an object type schema
let is_object_type = params_obj
.get("type")
.and_then(|t| t.as_str())
.is_none_or(|t| t == "object"); // Default to true if no type is specified

// Only apply full schema validation to object types
if is_object_type {
// Ensure required fields exist with default values
params_obj.entry("properties").or_insert_with(|| json!({}));
params_obj.entry("required").or_insert_with(|| json!([]));
params_obj.entry("type").or_insert_with(|| json!("object"));

// Recursively validate properties if it exists
if let Some(properties) = params_obj.get_mut("properties") {
if let Some(properties_obj) = properties.as_object_mut() {
for (_key, prop) in properties_obj.iter_mut() {
normalize_nullable(prop);
if prop.is_object()
&& prop.get("type").and_then(|t| t.as_str()) == Some("object")
{
ensure_valid_json_schema(prop);
}
}
}
}
}
}
}

/// Normalizes nullable type representations that some providers (e.g. Vertex Gemini via Bifrost)
/// don't support:
/// - `"type": ["integer", "null"]` → `"type": "integer"` (drops the null variant)
/// - `"anyOf": [T, {"type": "null"}]` → T (unwraps to the non-null schema)
///
/// Optional-ness is already conveyed by the field being absent from `required`.
fn normalize_nullable(schema: &mut Value) {
let Some(obj) = schema.as_object_mut() else {
return;
};

// Handle type: ["T", "null"] array form (schemars 1.x style for nullable primitives)
if let Some(type_val) = obj.get("type").cloned() {
if let Some(types) = type_val.as_array() {
let non_null: Vec<&Value> = types
.iter()
.filter(|t| t.as_str() != Some("null"))
.collect();
if non_null.len() == 1 {
let scalar = non_null[0].clone();
obj.insert("type".to_string(), scalar);
return;
}
}
}

// Handle anyOf: [T, {type: "null"}] form — merge the non-null variant's fields
// into the current object (preserving sibling keys like "description" or "default")
// rather than replacing the whole schema.
if let Some(any_of) = obj.remove("anyOf") {
if let Some(variants) = any_of.as_array() {
if variants.len() == 2 {
let is_null = |v: &Value| v.get("type").and_then(|t| t.as_str()) == Some("null");
let non_null = if is_null(&variants[0]) {
Some(&variants[1])
} else if is_null(&variants[1]) {
Some(&variants[0])
} else {
None
};
if let Some(replacement) = non_null {
if let Some(replacement_obj) = replacement.as_object() {
for (k, v) in replacement_obj {
obj.entry(k.clone()).or_insert(v.clone());
}
return;
}
}
}
}
// Put it back if we couldn't simplify
obj.insert("anyOf".to_string(), any_of);
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -616,6 +742,55 @@ mod tests {
assert_eq!(usage.cache_write_input_tokens, Some(10));
}

#[test]
fn format_tools_rejects_duplicates() {
let tool1 = Tool::new(
"test_tool",
"Test tool",
json!({"type":"object"}).as_object().unwrap().clone(),
);
let tool2 = Tool::new(
"test_tool",
"Test tool",
json!({"type":"object"}).as_object().unwrap().clone(),
);

let error = format_tools(&[tool1, tool2]).unwrap_err();
assert!(error.to_string().contains("Duplicate tool name"));
}

#[test]
fn validate_tool_schemas_normalizes_nullable_fields() {
let mut tools = vec![json!({
"type": "function",
"function": {
"name": "shell",
"description": "run shell",
"parameters": {
"type": "object",
"properties": {
"command": { "type": "string" },
"timeout_secs": {
"description": "timeout in seconds",
"anyOf": [
{ "type": "integer", "format": "uint64", "minimum": 0 },
{ "type": "null" }
]
}
},
"required": ["command"]
}
}
})];

validate_tool_schemas(&mut tools);
let timeout_schema = &tools[0]["function"]["parameters"]["properties"]["timeout_secs"];
assert_eq!(timeout_schema["type"], "integer");
assert_eq!(timeout_schema["format"], "uint64");
assert_eq!(timeout_schema["description"], "timeout in seconds");
assert!(timeout_schema.get("anyOf").is_none());
}

#[tokio::test]
async fn streaming_response_extracts_inline_think_blocks() -> anyhow::Result<()> {
use futures::{pin_mut, stream, StreamExt};
Expand Down
132 changes: 5 additions & 127 deletions crates/goose/src/providers/formats/openai.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@ use crate::providers::utils::{
};
use anyhow::{anyhow, Error};
use chrono;
use goose_providers::openai::is_reserved_request_param_key;
use goose_providers::openai::merge_reasoning_text;
#[cfg(test)]
use goose_providers::openai::DeltaToolCallFunction;
pub use goose_providers::openai::{get_usage, response_to_streaming_message, OpenAiFormatOptions};
pub use goose_providers::openai::{
format_tools, get_usage, response_to_streaming_message, validate_tool_schemas,
OpenAiFormatOptions,
};
use goose_providers::text::split_think_blocks;
use goose_types::ModelConfig;
#[cfg(test)]
Expand All @@ -24,10 +28,6 @@ use serde_json::{json, Value};
use std::borrow::Cow;
use std::ops::Deref;

fn is_reserved_request_param_key(key: &str) -> bool {
matches!(key, "messages" | "model" | "stream" | "stream_options")
}

pub fn format_messages(messages: &[Message], image_format: &ImageFormat) -> Vec<Value> {
format_messages_with_options(
messages,
Expand Down Expand Up @@ -397,28 +397,6 @@ fn is_image_only_user_message(msg: &Value) -> bool {
})
}

pub fn format_tools(tools: &[Tool]) -> anyhow::Result<Vec<Value>> {
let mut tool_names = std::collections::HashSet::new();
let mut result = Vec::new();

for tool in tools {
if !tool_names.insert(&tool.name) {
return Err(anyhow!("Duplicate tool name: {}", tool.name));
}

result.push(json!({
"type": "function",
"function": {
"name": tool.name,
"description": tool.description,
"parameters": tool.input_schema,
}
}));
}

Ok(result)
}

/// Convert OpenAI's API response to internal Message format
pub fn response_to_message(response: &Value) -> anyhow::Result<Message> {
let Some(original) = response
Expand Down Expand Up @@ -555,106 +533,6 @@ pub fn response_to_message(response: &Value) -> anyhow::Result<Message> {

/// Validates and fixes tool schemas to ensure they have proper parameter structure.
/// If parameters exist, ensures they have properties and required fields, or removes parameters entirely.
pub fn validate_tool_schemas(tools: &mut [Value]) {
for tool in tools.iter_mut() {
if let Some(function) = tool.get_mut("function") {
if let Some(parameters) = function.get_mut("parameters") {
if parameters.is_object() {
ensure_valid_json_schema(parameters);
}
}
}
}
}

/// Ensures that the given JSON value follows the expected JSON Schema structure.
fn ensure_valid_json_schema(schema: &mut Value) {
if let Some(params_obj) = schema.as_object_mut() {
// Check if this is meant to be an object type schema
let is_object_type = params_obj
.get("type")
.and_then(|t| t.as_str())
.is_none_or(|t| t == "object"); // Default to true if no type is specified

// Only apply full schema validation to object types
if is_object_type {
// Ensure required fields exist with default values
params_obj.entry("properties").or_insert_with(|| json!({}));
params_obj.entry("required").or_insert_with(|| json!([]));
params_obj.entry("type").or_insert_with(|| json!("object"));

// Recursively validate properties if it exists
if let Some(properties) = params_obj.get_mut("properties") {
if let Some(properties_obj) = properties.as_object_mut() {
for (_key, prop) in properties_obj.iter_mut() {
normalize_nullable(prop);
if prop.is_object()
&& prop.get("type").and_then(|t| t.as_str()) == Some("object")
{
ensure_valid_json_schema(prop);
}
}
}
}
}
}
}

/// Normalizes nullable type representations that some providers (e.g. Vertex Gemini via Bifrost)
/// don't support:
/// - `"type": ["integer", "null"]` → `"type": "integer"` (drops the null variant)
/// - `"anyOf": [T, {"type": "null"}]` → T (unwraps to the non-null schema)
///
/// Optional-ness is already conveyed by the field being absent from `required`.
fn normalize_nullable(schema: &mut Value) {
let Some(obj) = schema.as_object_mut() else {
return;
};

// Handle type: ["T", "null"] array form (schemars 1.x style for nullable primitives)
if let Some(type_val) = obj.get("type").cloned() {
if let Some(types) = type_val.as_array() {
let non_null: Vec<&Value> = types
.iter()
.filter(|t| t.as_str() != Some("null"))
.collect();
if non_null.len() == 1 {
let scalar = non_null[0].clone();
obj.insert("type".to_string(), scalar);
return;
}
}
}

// Handle anyOf: [T, {type: "null"}] form — merge the non-null variant's fields
// into the current object (preserving sibling keys like "description" or "default")
// rather than replacing the whole schema.
if let Some(any_of) = obj.remove("anyOf") {
if let Some(variants) = any_of.as_array() {
if variants.len() == 2 {
let is_null = |v: &Value| v.get("type").and_then(|t| t.as_str()) == Some("null");
let non_null = if is_null(&variants[0]) {
Some(&variants[1])
} else if is_null(&variants[1]) {
Some(&variants[0])
} else {
None
};
if let Some(replacement) = non_null {
if let Some(replacement_obj) = replacement.as_object() {
for (k, v) in replacement_obj {
obj.entry(k.clone()).or_insert(v.clone());
}
return;
}
}
}
}
// Put it back if we couldn't simplify
obj.insert("anyOf".to_string(), any_of);
}
}

pub fn create_request(
model_config: &ModelConfig,
system: &str,
Expand Down