fix: model family and apply_patch consistency (#3603)
## Summary Resolves a merge conflict between #3597 and #3560, and adds tests to double check our apply_patch configuration. ## Testing - [x] Added unit tests --------- Co-authored-by: dedrisian-oai <dedrisian@openai.com>
This commit is contained in:
@@ -41,8 +41,9 @@ impl Prompt {
|
|||||||
.unwrap_or(model.base_instructions.deref());
|
.unwrap_or(model.base_instructions.deref());
|
||||||
let mut sections: Vec<&str> = vec![base];
|
let mut sections: Vec<&str> = vec![base];
|
||||||
|
|
||||||
// When there are no custom instructions, add apply_patch_tool_instructions if either:
|
// When there are no custom instructions, add apply_patch_tool_instructions if:
|
||||||
// - the model needs special instructions (4.1), or
|
// - the model needs special instructions (4.1)
|
||||||
|
// AND
|
||||||
// - there is no apply_patch tool present
|
// - there is no apply_patch tool present
|
||||||
let is_apply_patch_tool_present = self.tools.iter().any(|tool| match tool {
|
let is_apply_patch_tool_present = self.tools.iter().any(|tool| match tool {
|
||||||
OpenAiTool::Function(f) => f.name == "apply_patch",
|
OpenAiTool::Function(f) => f.name == "apply_patch",
|
||||||
@@ -50,7 +51,8 @@ impl Prompt {
|
|||||||
_ => false,
|
_ => false,
|
||||||
});
|
});
|
||||||
if self.base_instructions_override.is_none()
|
if self.base_instructions_override.is_none()
|
||||||
&& (model.needs_special_apply_patch_instructions || !is_apply_patch_tool_present)
|
&& model.needs_special_apply_patch_instructions
|
||||||
|
&& !is_apply_patch_tool_present
|
||||||
{
|
{
|
||||||
sections.push(APPLY_PATCH_TOOL_INSTRUCTIONS);
|
sections.push(APPLY_PATCH_TOOL_INSTRUCTIONS);
|
||||||
}
|
}
|
||||||
@@ -174,22 +176,64 @@ impl Stream for ResponseStream {
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use crate::model_family::find_family_for_model;
|
use crate::model_family::find_family_for_model;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
|
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
|
struct InstructionsTestCase {
|
||||||
|
pub slug: &'static str,
|
||||||
|
pub expects_apply_patch_instructions: bool,
|
||||||
|
}
|
||||||
#[test]
|
#[test]
|
||||||
fn get_full_instructions_no_user_content() {
|
fn get_full_instructions_no_user_content() {
|
||||||
let prompt = Prompt {
|
let prompt = Prompt {
|
||||||
..Default::default()
|
..Default::default()
|
||||||
};
|
};
|
||||||
let model_family = find_family_for_model("gpt-4.1").expect("known model slug");
|
let test_cases = vec![
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "gpt-3.5",
|
||||||
|
expects_apply_patch_instructions: true,
|
||||||
|
},
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "gpt-4.1",
|
||||||
|
expects_apply_patch_instructions: true,
|
||||||
|
},
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "gpt-4o",
|
||||||
|
expects_apply_patch_instructions: true,
|
||||||
|
},
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "gpt-5",
|
||||||
|
expects_apply_patch_instructions: true,
|
||||||
|
},
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "codex-mini-latest",
|
||||||
|
expects_apply_patch_instructions: true,
|
||||||
|
},
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "gpt-oss:120b",
|
||||||
|
expects_apply_patch_instructions: false,
|
||||||
|
},
|
||||||
|
InstructionsTestCase {
|
||||||
|
slug: "swiftfox",
|
||||||
|
expects_apply_patch_instructions: false,
|
||||||
|
},
|
||||||
|
];
|
||||||
|
for test_case in test_cases {
|
||||||
|
let model_family = find_family_for_model(test_case.slug).expect("known model slug");
|
||||||
|
let expected = if test_case.expects_apply_patch_instructions {
|
||||||
|
format!(
|
||||||
|
"{}\n{}",
|
||||||
|
model_family.clone().base_instructions,
|
||||||
|
APPLY_PATCH_TOOL_INSTRUCTIONS
|
||||||
|
)
|
||||||
|
} else {
|
||||||
|
model_family.clone().base_instructions
|
||||||
|
};
|
||||||
|
|
||||||
let expected = format!(
|
let full = prompt.get_full_instructions(&model_family);
|
||||||
"{}\n{}",
|
assert_eq!(full, expected);
|
||||||
model_family.base_instructions, APPLY_PATCH_TOOL_INSTRUCTIONS
|
}
|
||||||
);
|
|
||||||
let full = prompt.get_full_instructions(&model_family);
|
|
||||||
assert_eq!(full, expected);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -66,23 +66,6 @@ macro_rules! model_family {
|
|||||||
}};
|
}};
|
||||||
}
|
}
|
||||||
|
|
||||||
macro_rules! simple_model_family {
|
|
||||||
(
|
|
||||||
$slug:expr, $family:expr
|
|
||||||
) => {{
|
|
||||||
Some(ModelFamily {
|
|
||||||
slug: $slug.to_string(),
|
|
||||||
family: $family.to_string(),
|
|
||||||
needs_special_apply_patch_instructions: false,
|
|
||||||
supports_reasoning_summaries: false,
|
|
||||||
reasoning_summary_format: ReasoningSummaryFormat::None,
|
|
||||||
uses_local_shell_tool: false,
|
|
||||||
apply_patch_tool_type: None,
|
|
||||||
base_instructions: BASE_INSTRUCTIONS.to_string(),
|
|
||||||
})
|
|
||||||
}};
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Returns a `ModelFamily` for the given model slug, or `None` if the slug
|
/// Returns a `ModelFamily` for the given model slug, or `None` if the slug
|
||||||
/// does not match any known model family.
|
/// does not match any known model family.
|
||||||
pub fn find_family_for_model(slug: &str) -> Option<ModelFamily> {
|
pub fn find_family_for_model(slug: &str) -> Option<ModelFamily> {
|
||||||
@@ -90,17 +73,20 @@ pub fn find_family_for_model(slug: &str) -> Option<ModelFamily> {
|
|||||||
model_family!(
|
model_family!(
|
||||||
slug, "o3",
|
slug, "o3",
|
||||||
supports_reasoning_summaries: true,
|
supports_reasoning_summaries: true,
|
||||||
|
needs_special_apply_patch_instructions: true,
|
||||||
)
|
)
|
||||||
} else if slug.starts_with("o4-mini") {
|
} else if slug.starts_with("o4-mini") {
|
||||||
model_family!(
|
model_family!(
|
||||||
slug, "o4-mini",
|
slug, "o4-mini",
|
||||||
supports_reasoning_summaries: true,
|
supports_reasoning_summaries: true,
|
||||||
|
needs_special_apply_patch_instructions: true,
|
||||||
)
|
)
|
||||||
} else if slug.starts_with("codex-mini-latest") {
|
} else if slug.starts_with("codex-mini-latest") {
|
||||||
model_family!(
|
model_family!(
|
||||||
slug, "codex-mini-latest",
|
slug, "codex-mini-latest",
|
||||||
supports_reasoning_summaries: true,
|
supports_reasoning_summaries: true,
|
||||||
uses_local_shell_tool: true,
|
uses_local_shell_tool: true,
|
||||||
|
needs_special_apply_patch_instructions: true,
|
||||||
)
|
)
|
||||||
} else if slug.starts_with("gpt-4.1") {
|
} else if slug.starts_with("gpt-4.1") {
|
||||||
model_family!(
|
model_family!(
|
||||||
@@ -110,26 +96,21 @@ pub fn find_family_for_model(slug: &str) -> Option<ModelFamily> {
|
|||||||
} else if slug.starts_with("gpt-oss") || slug.starts_with("openai/gpt-oss") {
|
} else if slug.starts_with("gpt-oss") || slug.starts_with("openai/gpt-oss") {
|
||||||
model_family!(slug, "gpt-oss", apply_patch_tool_type: Some(ApplyPatchToolType::Function))
|
model_family!(slug, "gpt-oss", apply_patch_tool_type: Some(ApplyPatchToolType::Function))
|
||||||
} else if slug.starts_with("gpt-4o") {
|
} else if slug.starts_with("gpt-4o") {
|
||||||
simple_model_family!(slug, "gpt-4o")
|
model_family!(slug, "gpt-4o", needs_special_apply_patch_instructions: true)
|
||||||
} else if slug.starts_with("gpt-3.5") {
|
} else if slug.starts_with("gpt-3.5") {
|
||||||
simple_model_family!(slug, "gpt-3.5")
|
model_family!(slug, "gpt-3.5", needs_special_apply_patch_instructions: true)
|
||||||
} else if slug.starts_with("codex-") {
|
} else if slug.starts_with("codex-") || slug.starts_with("swiftfox") {
|
||||||
model_family!(
|
model_family!(
|
||||||
slug, slug,
|
slug, slug,
|
||||||
supports_reasoning_summaries: true,
|
supports_reasoning_summaries: true,
|
||||||
reasoning_summary_format: ReasoningSummaryFormat::Experimental,
|
reasoning_summary_format: ReasoningSummaryFormat::Experimental,
|
||||||
|
base_instructions: SWIFTFOX_INSTRUCTIONS.to_string(),
|
||||||
)
|
)
|
||||||
} else if slug.starts_with("gpt-5") {
|
} else if slug.starts_with("gpt-5") {
|
||||||
model_family!(
|
model_family!(
|
||||||
slug, "gpt-5",
|
slug, "gpt-5",
|
||||||
supports_reasoning_summaries: true,
|
supports_reasoning_summaries: true,
|
||||||
)
|
needs_special_apply_patch_instructions: true,
|
||||||
} else if slug.starts_with("swiftfox") {
|
|
||||||
model_family!(
|
|
||||||
slug, "swiftfox",
|
|
||||||
supports_reasoning_summaries: true,
|
|
||||||
reasoning_summary_format: ReasoningSummaryFormat::Experimental,
|
|
||||||
base_instructions: SWIFTFOX_INSTRUCTIONS.to_string(),
|
|
||||||
)
|
)
|
||||||
} else {
|
} else {
|
||||||
None
|
None
|
||||||
|
|||||||
Reference in New Issue
Block a user