Skip to content

Commit 1e724ea

Browse files
amitksingh1490amitksingh1490
authored
Revert "fix(forge_domain): merge reasoning text only when consecutive" (#2314)
1 parent b18cd1d commit 1e724ea

2 files changed

Lines changed: 147 additions & 159 deletions

File tree

‎crates/forge_domain/src/reasoning.rs‎

Lines changed: 147 additions & 158 deletions
Original file line numberDiff line numberDiff line change
@@ -42,97 +42,65 @@ impl Reasoning {
4242
}
4343

4444
pub fn from_parts(parts: Vec<Vec<ReasoningPart>>) -> Vec<ReasoningFull> {
45-
let mut result: Vec<ReasoningFull> = Vec::new();
46-
let mut current_text_parts: Vec<ReasoningPart> = Vec::new();
45+
// Flatten all parts and group by type
46+
let mut grouped: std::collections::HashMap<Option<String>, Vec<ReasoningPart>> =
47+
std::collections::HashMap::new();
4748

4849
for part_vec in parts {
4950
for part in part_vec {
50-
// According to OpenRouter SDK:
51-
// 1. Only 'reasoning.text' blocks are merged when consecutive.
52-
// 2. All other types (summary, encrypted, etc.) are appended as-is.
53-
// 3. IMPORTANT: If 'type_of' is None, but 'text' is present, it's treated as
54-
// 'reasoning.text'.
55-
let is_text = part.type_of.as_deref() == Some("reasoning.text")
56-
|| (part.type_of.is_none() && part.text.is_some());
57-
58-
if is_text {
59-
current_text_parts.push(part);
60-
} else {
61-
// Non-text type encountered. Flush any pending text parts first.
62-
if !current_text_parts.is_empty() {
63-
if let Some(merged) = Self::merge_parts(
64-
Some("reasoning.text".to_string()),
65-
&current_text_parts,
66-
) {
67-
result.push(merged);
68-
}
69-
current_text_parts.clear();
70-
}
71-
72-
// Add this non-text part as a separate block
73-
result.push(ReasoningFull {
74-
text: part.text,
75-
signature: part.signature,
76-
data: part.data,
77-
id: part.id,
78-
format: part.format,
79-
index: part.index,
80-
type_of: part.type_of,
81-
});
82-
}
51+
grouped.entry(part.type_of.clone()).or_default().push(part);
8352
}
8453
}
8554

86-
// Flush any remaining text parts
87-
if !current_text_parts.is_empty()
88-
&& let Some(merged) =
89-
Self::merge_parts(Some("reasoning.text".to_string()), &current_text_parts)
90-
{
91-
result.push(merged);
92-
}
93-
94-
result
95-
}
96-
97-
fn merge_parts(type_key: Option<String>, parts: &[ReasoningPart]) -> Option<ReasoningFull> {
98-
// Merge text from all parts
99-
let text = parts
100-
.iter()
101-
.filter_map(|p| p.text.as_deref())
102-
.collect::<String>();
103-
104-
// Get first non-empty value for each field
105-
let signature = parts.iter().find_map(|p| {
106-
p.signature
107-
.as_deref()
108-
.filter(|s| !s.is_empty())
109-
.map(String::from)
110-
});
111-
let id = parts
112-
.iter()
113-
.find_map(|p| p.id.as_deref().filter(|s| !s.is_empty()).map(String::from));
114-
let format = parts.iter().find_map(|p| {
115-
p.format
116-
.as_deref()
117-
.filter(|s| !s.is_empty())
118-
.map(String::from)
119-
});
120-
let index = parts.iter().find_map(|p| p.index);
121-
122-
// Only include if at least one field has data
123-
if text.is_empty() && signature.is_none() {
124-
return None;
125-
}
55+
grouped
56+
.into_iter()
57+
.filter_map(|(type_key, parts)| {
58+
// Merge text from all parts
59+
let text = parts
60+
.iter()
61+
.filter_map(|p| p.text.as_deref())
62+
.collect::<String>();
63+
64+
// Get first non-empty value for each field
65+
let signature = parts.iter().find_map(|p| {
66+
p.signature
67+
.as_deref()
68+
.filter(|s| !s.is_empty())
69+
.map(String::from)
70+
});
71+
let data = parts.iter().find_map(|p| {
72+
p.data
73+
.as_deref()
74+
.filter(|s| !s.is_empty())
75+
.map(String::from)
76+
});
77+
let id = parts
78+
.iter()
79+
.find_map(|p| p.id.as_deref().filter(|s| !s.is_empty()).map(String::from));
80+
let format = parts.iter().find_map(|p| {
81+
p.format
82+
.as_deref()
83+
.filter(|s| !s.is_empty())
84+
.map(String::from)
85+
});
86+
let index = parts.iter().find_map(|p| p.index);
87+
88+
// Only include if at least one field has data
89+
if text.is_empty() && signature.is_none() && data.is_none() {
90+
return None;
91+
}
12692

127-
Some(ReasoningFull {
128-
text: (!text.is_empty()).then_some(text),
129-
signature,
130-
data: None,
131-
id,
132-
format,
133-
index,
134-
type_of: type_key,
135-
})
93+
Some(ReasoningFull {
94+
text: (!text.is_empty()).then_some(text),
95+
signature,
96+
data,
97+
id,
98+
format,
99+
index,
100+
type_of: type_key,
101+
})
102+
})
103+
.collect()
136104
}
137105
}
138106

@@ -141,7 +109,7 @@ mod tests {
141109
use super::*;
142110

143111
#[test]
144-
fn test_reasoning_detail_from_parts_merges_consecutive_types() {
112+
fn test_reasoning_detail_from_parts_groups_by_type() {
145113
// Create a fixture with parts of different types across streaming deltas
146114
let fixture = vec![
147115
// First delta: reasoning.text
@@ -163,79 +131,107 @@ mod tests {
163131
id: Some("tool_call_id".to_string()),
164132
..Default::default()
165133
}],
166-
// Fourth delta: another reasoning.text appears (non-consecutive with first)
167-
vec![ReasoningPart {
168-
type_of: Some("reasoning.text".to_string()),
169-
text: Some("Part 3".to_string()),
170-
..Default::default()
171-
}],
172134
];
173135

174136
// Execute the function to get the actual result
175137
let actual = Reasoning::from_parts(fixture);
176138

177-
// Should have 3 entries: merged text 1+2, encrypted, and separate text 3
178-
assert_eq!(actual.len(), 3);
139+
// Both types should be separate entries
140+
assert_eq!(actual.len(), 2);
179141

180-
// Verify order and content
181-
assert_eq!(actual[0].type_of, Some("reasoning.text".to_string()));
182-
assert_eq!(actual[0].text, Some("Part 1 Part 2".to_string()));
142+
// Find each type
143+
let text_entry = actual
144+
.iter()
145+
.find(|r| r.type_of == Some("reasoning.text".to_string()))
146+
.expect("Should have reasoning.text entry");
147+
let encrypted_entry = actual
148+
.iter()
149+
.find(|r| r.type_of == Some("reasoning.encrypted".to_string()))
150+
.expect("Should have reasoning.encrypted entry");
183151

184-
assert_eq!(actual[1].type_of, Some("reasoning.encrypted".to_string()));
185-
assert_eq!(actual[1].data, Some("encrypted_data".to_string()));
152+
// Verify text entry has merged text
153+
assert_eq!(text_entry.text, Some("Part 1 Part 2".to_string()));
186154

187-
assert_eq!(actual[2].type_of, Some("reasoning.text".to_string()));
188-
assert_eq!(actual[2].text, Some("Part 3".to_string()));
155+
// Verify encrypted entry has data and id
156+
assert_eq!(encrypted_entry.data, Some("encrypted_data".to_string()));
157+
assert_eq!(encrypted_entry.id, Some("tool_call_id".to_string()));
189158
}
190159

191160
#[test]
192161
fn test_reasoning_detail_from_parts_with_different_lengths() {
193-
// Create a fixture with different types of reasoning
162+
// Create a fixture with different types to test grouping
194163
let fixture = vec![
164+
vec![
165+
ReasoningPart {
166+
type_of: Some("type1".to_string()),
167+
text: Some("a-text".to_string()),
168+
signature: Some("a-sig".to_string()),
169+
..Default::default()
170+
},
171+
ReasoningPart {
172+
type_of: Some("type2".to_string()),
173+
text: Some("b-text".to_string()),
174+
signature: Some("b-sig".to_string()),
175+
..Default::default()
176+
},
177+
],
195178
vec![ReasoningPart {
196179
type_of: Some("type1".to_string()),
197-
text: Some("a-text".to_string()),
198-
signature: Some("a-sig".to_string()),
199-
..Default::default()
200-
}],
201-
vec![ReasoningPart {
202-
type_of: Some("type2".to_string()),
203-
text: Some("b-text".to_string()),
204-
signature: Some("b-sig".to_string()),
205-
..Default::default()
206-
}],
207-
vec![ReasoningPart {
208-
type_of: Some("type2".to_string()),
209180
text: Some("c-text".to_string()),
210181
signature: Some("c-sig".to_string()),
211182
..Default::default()
212183
}],
184+
vec![
185+
ReasoningPart {
186+
type_of: Some("type1".to_string()),
187+
text: Some("d-text".to_string()),
188+
signature: Some("d-sig".to_string()),
189+
..Default::default()
190+
},
191+
ReasoningPart {
192+
type_of: Some("type2".to_string()),
193+
text: Some("e-text".to_string()),
194+
signature: Some("e-sig".to_string()),
195+
..Default::default()
196+
},
197+
ReasoningPart {
198+
type_of: Some("type3".to_string()),
199+
text: Some("f-text".to_string()),
200+
signature: Some("f-sig".to_string()),
201+
..Default::default()
202+
},
203+
],
213204
];
214205

215206
// Execute the function to get the actual result
216-
let actual = Reasoning::from_parts(fixture);
207+
let mut actual = Reasoning::from_parts(fixture);
208+
actual.sort_by(|a, b| a.type_of.cmp(&b.type_of)); // Sort by type for consistent ordering
217209

218-
// Non-text types are NEVER merged, even if consecutive
219-
let expected = vec![
210+
// Define the expected result - now grouped by type
211+
let mut expected = vec![
212+
// type1: a + c + d (text merged, signature is first non-empty)
220213
ReasoningFull {
221214
type_of: Some("type1".to_string()),
222-
text: Some("a-text".to_string()),
223-
signature: Some("a-sig".to_string()),
215+
text: Some("a-textc-textd-text".to_string()),
216+
signature: Some("a-sig".to_string()), // First non-empty signature
224217
..Default::default()
225218
},
219+
// type2: b + e (text merged, signature is first non-empty)
226220
ReasoningFull {
227221
type_of: Some("type2".to_string()),
228-
text: Some("b-text".to_string()),
229-
signature: Some("b-sig".to_string()),
222+
text: Some("b-texte-text".to_string()),
223+
signature: Some("b-sig".to_string()), // First non-empty signature
230224
..Default::default()
231225
},
226+
// type3: f
232227
ReasoningFull {
233-
type_of: Some("type2".to_string()),
234-
text: Some("c-text".to_string()),
235-
signature: Some("c-sig".to_string()),
228+
type_of: Some("type3".to_string()),
229+
text: Some("f-text".to_string()),
230+
signature: Some("f-sig".to_string()),
236231
..Default::default()
237232
},
238233
];
234+
expected.sort_by(|a, b| a.type_of.cmp(&b.type_of)); // Sort expected for consistent comparison
239235

240236
// Assert that the actual result matches the expected result
241237
assert_eq!(actual, expected);
@@ -265,29 +261,12 @@ mod tests {
265261
// Execute the function to get the actual result
266262
let actual = Reasoning::from_parts(fixture);
267263

268-
// parts with text but no type_of are treated as reasoning.text and merged if
269-
// consecutive. The middle part has NO text and NO type_of, so it breaks
270-
// the consecutive text merge.
271-
let expected = vec![
272-
ReasoningFull {
273-
text: Some("a-text".to_string()),
274-
signature: None,
275-
type_of: Some("reasoning.text".to_string()),
276-
..Default::default()
277-
},
278-
ReasoningFull {
279-
text: None,
280-
signature: Some("b-sig".to_string()),
281-
type_of: None,
282-
..Default::default()
283-
},
284-
ReasoningFull {
285-
text: Some("b-test".to_string()),
286-
signature: None,
287-
type_of: Some("reasoning.text".to_string()),
288-
..Default::default()
289-
},
290-
];
264+
// Define the expected result
265+
let expected = vec![ReasoningFull {
266+
text: Some("a-textb-test".to_string()),
267+
signature: Some("b-sig".to_string()),
268+
..Default::default()
269+
}];
291270

292271
// Assert that the actual result matches the expected result
293272
assert_eq!(actual, expected);
@@ -320,7 +299,7 @@ mod tests {
320299
},
321300
ReasoningPart {
322301
type_of: Some("reasoning.encrypted".to_string()),
323-
data: Some("complete-data".to_string()),
302+
text: Some("complete-text".to_string()),
324303
signature: Some("complete-sig".to_string()),
325304
..Default::default()
326305
},
@@ -334,23 +313,33 @@ mod tests {
334313
},
335314
ReasoningPart {
336315
type_of: Some("reasoning.encrypted".to_string()),
337-
data: Some("more-data2".to_string()),
316+
text: Some("more-text2".to_string()),
338317
signature: Some("more-sig".to_string()),
339318
..Default::default()
340319
},
341320
],
342321
];
343322

344-
let actual = Reasoning::from_parts(fixture);
345-
346-
// Encrypted reasoning blocks are NEVER merged.
347-
// Non-consecutive text blocks are NOT merged.
348-
// In this case: [text1, encrypted1, text2, encrypted2]
349-
assert_eq!(actual.len(), 4);
323+
let mut actual = Reasoning::from_parts(fixture);
324+
actual.sort_by(|a, b| a.type_of.cmp(&b.type_of)); // Sort by type for consistent ordering
350325

351-
assert_eq!(actual[0].text, Some("text-only".to_string()));
352-
assert_eq!(actual[1].data, Some("complete-data".to_string()));
353-
assert_eq!(actual[2].text, Some("more-text".to_string()));
354-
assert_eq!(actual[3].data, Some("more-data2".to_string()));
326+
// Now grouped by type: reasoning.text and reasoning.encrypted are separate
327+
// entries
328+
let mut expected = vec![
329+
ReasoningFull {
330+
type_of: Some("reasoning.text".to_string()),
331+
text: Some("text-onlymore-text".to_string()),
332+
signature: None, // No signature in reasoning.text type
333+
..Default::default()
334+
},
335+
ReasoningFull {
336+
type_of: Some("reasoning.encrypted".to_string()),
337+
text: Some("complete-textmore-text2".to_string()),
338+
signature: Some("complete-sig".to_string()), // First non-empty signature
339+
..Default::default()
340+
},
341+
];
342+
expected.sort_by(|a, b| a.type_of.cmp(&b.type_of)); // Sort expected as well for consistent comparison
343+
assert_eq!(actual, expected);
355344
}
356345
}

0 commit comments

Comments
 (0)