Skip to content

Commit b667383

Browse files
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 <[email protected]>
1 parent 1823906 commit b667383

File tree

2 files changed

+62
-37
lines changed

2 files changed

+62
-37
lines changed

codex-rs/core/src/client_common.rs

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,18 @@ impl Prompt {
4141
.unwrap_or(model.base_instructions.deref());
4242
let mut sections: Vec<&str> = vec![base];
4343

44-
// When there are no custom instructions, add apply_patch_tool_instructions if either:
45-
// - the model needs special instructions (4.1), or
44+
// When there are no custom instructions, add apply_patch_tool_instructions if:
45+
// - the model needs special instructions (4.1)
46+
// AND
4647
// - there is no apply_patch tool present
4748
let is_apply_patch_tool_present = self.tools.iter().any(|tool| match tool {
4849
OpenAiTool::Function(f) => f.name == "apply_patch",
4950
OpenAiTool::Freeform(f) => f.name == "apply_patch",
5051
_ => false,
5152
});
5253
if self.base_instructions_override.is_none()
53-
&& (model.needs_special_apply_patch_instructions || !is_apply_patch_tool_present)
54+
&& model.needs_special_apply_patch_instructions
55+
&& !is_apply_patch_tool_present
5456
{
5557
sections.push(APPLY_PATCH_TOOL_INSTRUCTIONS);
5658
}
@@ -174,22 +176,64 @@ impl Stream for ResponseStream {
174176
#[cfg(test)]
175177
mod tests {
176178
use crate::model_family::find_family_for_model;
179+
use pretty_assertions::assert_eq;
177180

178181
use super::*;
179182

183+
struct InstructionsTestCase {
184+
pub slug: &'static str,
185+
pub expects_apply_patch_instructions: bool,
186+
}
180187
#[test]
181188
fn get_full_instructions_no_user_content() {
182189
let prompt = Prompt {
183190
..Default::default()
184191
};
185-
let model_family = find_family_for_model("gpt-4.1").expect("known model slug");
192+
let test_cases = vec![
193+
InstructionsTestCase {
194+
slug: "gpt-3.5",
195+
expects_apply_patch_instructions: true,
196+
},
197+
InstructionsTestCase {
198+
slug: "gpt-4.1",
199+
expects_apply_patch_instructions: true,
200+
},
201+
InstructionsTestCase {
202+
slug: "gpt-4o",
203+
expects_apply_patch_instructions: true,
204+
},
205+
InstructionsTestCase {
206+
slug: "gpt-5",
207+
expects_apply_patch_instructions: true,
208+
},
209+
InstructionsTestCase {
210+
slug: "codex-mini-latest",
211+
expects_apply_patch_instructions: true,
212+
},
213+
InstructionsTestCase {
214+
slug: "gpt-oss:120b",
215+
expects_apply_patch_instructions: false,
216+
},
217+
InstructionsTestCase {
218+
slug: "swiftfox",
219+
expects_apply_patch_instructions: false,
220+
},
221+
];
222+
for test_case in test_cases {
223+
let model_family = find_family_for_model(test_case.slug).expect("known model slug");
224+
let expected = if test_case.expects_apply_patch_instructions {
225+
format!(
226+
"{}\n{}",
227+
model_family.clone().base_instructions,
228+
APPLY_PATCH_TOOL_INSTRUCTIONS
229+
)
230+
} else {
231+
model_family.clone().base_instructions
232+
};
186233

187-
let expected = format!(
188-
"{}\n{}",
189-
model_family.base_instructions, APPLY_PATCH_TOOL_INSTRUCTIONS
190-
);
191-
let full = prompt.get_full_instructions(&model_family);
192-
assert_eq!(full, expected);
234+
let full = prompt.get_full_instructions(&model_family);
235+
assert_eq!(full, expected);
236+
}
193237
}
194238

195239
#[test]

codex-rs/core/src/model_family.rs

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -66,41 +66,27 @@ macro_rules! model_family {
6666
}};
6767
}
6868

69-
macro_rules! simple_model_family {
70-
(
71-
$slug:expr, $family:expr
72-
) => {{
73-
Some(ModelFamily {
74-
slug: $slug.to_string(),
75-
family: $family.to_string(),
76-
needs_special_apply_patch_instructions: false,
77-
supports_reasoning_summaries: false,
78-
reasoning_summary_format: ReasoningSummaryFormat::None,
79-
uses_local_shell_tool: false,
80-
apply_patch_tool_type: None,
81-
base_instructions: BASE_INSTRUCTIONS.to_string(),
82-
})
83-
}};
84-
}
85-
8669
/// Returns a `ModelFamily` for the given model slug, or `None` if the slug
8770
/// does not match any known model family.
8871
pub fn find_family_for_model(slug: &str) -> Option<ModelFamily> {
8972
if slug.starts_with("o3") {
9073
model_family!(
9174
slug, "o3",
9275
supports_reasoning_summaries: true,
76+
needs_special_apply_patch_instructions: true,
9377
)
9478
} else if slug.starts_with("o4-mini") {
9579
model_family!(
9680
slug, "o4-mini",
9781
supports_reasoning_summaries: true,
82+
needs_special_apply_patch_instructions: true,
9883
)
9984
} else if slug.starts_with("codex-mini-latest") {
10085
model_family!(
10186
slug, "codex-mini-latest",
10287
supports_reasoning_summaries: true,
10388
uses_local_shell_tool: true,
89+
needs_special_apply_patch_instructions: true,
10490
)
10591
} else if slug.starts_with("gpt-4.1") {
10692
model_family!(
@@ -110,26 +96,21 @@ pub fn find_family_for_model(slug: &str) -> Option<ModelFamily> {
11096
} else if slug.starts_with("gpt-oss") || slug.starts_with("openai/gpt-oss") {
11197
model_family!(slug, "gpt-oss", apply_patch_tool_type: Some(ApplyPatchToolType::Function))
11298
} else if slug.starts_with("gpt-4o") {
113-
simple_model_family!(slug, "gpt-4o")
99+
model_family!(slug, "gpt-4o", needs_special_apply_patch_instructions: true)
114100
} else if slug.starts_with("gpt-3.5") {
115-
simple_model_family!(slug, "gpt-3.5")
116-
} else if slug.starts_with("codex-") {
101+
model_family!(slug, "gpt-3.5", needs_special_apply_patch_instructions: true)
102+
} else if slug.starts_with("codex-") || slug.starts_with("swiftfox") {
117103
model_family!(
118104
slug, slug,
119105
supports_reasoning_summaries: true,
120106
reasoning_summary_format: ReasoningSummaryFormat::Experimental,
107+
base_instructions: SWIFTFOX_INSTRUCTIONS.to_string(),
121108
)
122109
} else if slug.starts_with("gpt-5") {
123110
model_family!(
124111
slug, "gpt-5",
125112
supports_reasoning_summaries: true,
126-
)
127-
} else if slug.starts_with("swiftfox") {
128-
model_family!(
129-
slug, "swiftfox",
130-
supports_reasoning_summaries: true,
131-
reasoning_summary_format: ReasoningSummaryFormat::Experimental,
132-
base_instructions: SWIFTFOX_INSTRUCTIONS.to_string(),
113+
needs_special_apply_patch_instructions: true,
133114
)
134115
} else {
135116
None

0 commit comments

Comments
 (0)