Skip to content

Commit 2e1eda5

Browse files
amitksingh1490amitksingh1490
authored
fix(forge_domain): merge reasoning text only when consecutive (#2307)
1 parent 5143163 commit 2e1eda5

2 files changed

Lines changed: 159 additions & 147 deletions

File tree

‎crates/forge_domain/src/reasoning.rs‎

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

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

4948
for part_vec in parts {
5049
for part in part_vec {
51-
grouped.entry(part.type_of.clone()).or_default().push(part);
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+
}
5283
}
5384
}
5485

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-
}
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+
}
9296

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()
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+
}
126+
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+
})
104136
}
105137
}
106138

@@ -109,7 +141,7 @@ mod tests {
109141
use super::*;
110142

111143
#[test]
112-
fn test_reasoning_detail_from_parts_groups_by_type() {
144+
fn test_reasoning_detail_from_parts_merges_consecutive_types() {
113145
// Create a fixture with parts of different types across streaming deltas
114146
let fixture = vec![
115147
// First delta: reasoning.text
@@ -131,107 +163,79 @@ mod tests {
131163
id: Some("tool_call_id".to_string()),
132164
..Default::default()
133165
}],
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+
}],
134172
];
135173

136174
// Execute the function to get the actual result
137175
let actual = Reasoning::from_parts(fixture);
138176

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

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");
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()));
151183

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

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()));
187+
assert_eq!(actual[2].type_of, Some("reasoning.text".to_string()));
188+
assert_eq!(actual[2].text, Some("Part 3".to_string()));
158189
}
159190

160191
#[test]
161192
fn test_reasoning_detail_from_parts_with_different_lengths() {
162-
// Create a fixture with different types to test grouping
193+
// Create a fixture with different types of reasoning
163194
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-
],
178195
vec![ReasoningPart {
179196
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()),
180209
text: Some("c-text".to_string()),
181210
signature: Some("c-sig".to_string()),
182211
..Default::default()
183212
}],
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-
],
204213
];
205214

206215
// Execute the function to get the actual result
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
216+
let actual = Reasoning::from_parts(fixture);
209217

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)
218+
// Non-text types are NEVER merged, even if consecutive
219+
let expected = vec![
213220
ReasoningFull {
214221
type_of: Some("type1".to_string()),
215-
text: Some("a-textc-textd-text".to_string()),
216-
signature: Some("a-sig".to_string()), // First non-empty signature
222+
text: Some("a-text".to_string()),
223+
signature: Some("a-sig".to_string()),
217224
..Default::default()
218225
},
219-
// type2: b + e (text merged, signature is first non-empty)
220226
ReasoningFull {
221227
type_of: Some("type2".to_string()),
222-
text: Some("b-texte-text".to_string()),
223-
signature: Some("b-sig".to_string()), // First non-empty signature
228+
text: Some("b-text".to_string()),
229+
signature: Some("b-sig".to_string()),
224230
..Default::default()
225231
},
226-
// type3: f
227232
ReasoningFull {
228-
type_of: Some("type3".to_string()),
229-
text: Some("f-text".to_string()),
230-
signature: Some("f-sig".to_string()),
233+
type_of: Some("type2".to_string()),
234+
text: Some("c-text".to_string()),
235+
signature: Some("c-sig".to_string()),
231236
..Default::default()
232237
},
233238
];
234-
expected.sort_by(|a, b| a.type_of.cmp(&b.type_of)); // Sort expected for consistent comparison
235239

236240
// Assert that the actual result matches the expected result
237241
assert_eq!(actual, expected);
@@ -261,12 +265,29 @@ mod tests {
261265
// Execute the function to get the actual result
262266
let actual = Reasoning::from_parts(fixture);
263267

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-
}];
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+
];
270291

271292
// Assert that the actual result matches the expected result
272293
assert_eq!(actual, expected);
@@ -299,7 +320,7 @@ mod tests {
299320
},
300321
ReasoningPart {
301322
type_of: Some("reasoning.encrypted".to_string()),
302-
text: Some("complete-text".to_string()),
323+
data: Some("complete-data".to_string()),
303324
signature: Some("complete-sig".to_string()),
304325
..Default::default()
305326
},
@@ -313,33 +334,23 @@ mod tests {
313334
},
314335
ReasoningPart {
315336
type_of: Some("reasoning.encrypted".to_string()),
316-
text: Some("more-text2".to_string()),
337+
data: Some("more-data2".to_string()),
317338
signature: Some("more-sig".to_string()),
318339
..Default::default()
319340
},
320341
],
321342
];
322343

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
344+
let actual = Reasoning::from_parts(fixture);
325345

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);
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);
350+
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()));
344355
}
345356
}

0 commit comments

Comments
 (0)