Skip to content

Commit e95162b

Browse files
authored
refactor: accumulator consumes EventFrame instead of inline JSON (vllm-project#52)
## Summary Refactors `ResponseAccumulator::process_sse_line` to use the typed `EventFrame` from the `events/` module (PR vllm-project#49) instead of inline JSON parsing. Pure behavioral refactor — same output for same input. **Changes:** - `process_sse_line` is now a thin wrapper: calls `normalize_sse_line()` → `process_event()` - New `pub(crate) fn process_event(&EventFrame)` — typed matching on `(SSEEventType, EventPayload)` pairs - Removes inline `serde_json::Value` field access, old `SSEEventType` import, `deserialize_from_value` import **Why:** Enables `StreamTee` (future) to call `process_event` directly with pre-normalized frames — avoiding double-parsing when forwarding SSE to client while accumulating for tool detection. **No behavioral change** — existing cassette-based integration tests pass unchanged. ## Test Plan - All existing tests pass unchanged (cassette-based streaming + non-streaming) - 5 new unit tests for `process_event` directly: - `ResponseCreated` sets response_id - `ResponseCreated` with empty id doesn't overwrite - `TextDelta` accumulates and attaches to message - `ResponseCompleted` extracts usage - Unknown events silently ignored - `cargo clippy -- -D warnings` clean - 160+ workspace tests pass --------- Signed-off-by: Ashwin Giridharan <girida@amazon.com>
1 parent dba572d commit e95162b

1 file changed

Lines changed: 170 additions & 31 deletions

File tree

crates/agentic-core/src/executor/accumulator.rs

Lines changed: 170 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,12 @@ use std::sync::mpsc;
1212

1313
use futures::{Stream, StreamExt};
1414

15+
use crate::events::{EventFrame, EventPayload, SSEEventType, normalize_sse_line};
1516
use crate::executor::error::{ExecutorError, ExecutorResult};
16-
use crate::types::event::{MessageStatus, ResponseStatus, SSEEventType};
17+
use crate::types::event::{MessageStatus, ResponseStatus};
1718
use crate::types::io::{OutputItem, OutputMessage, OutputTextContent, ResponseUsage};
1819
use crate::types::request_response::{IncompleteDetails, ResponsePayload};
19-
use crate::utils::common::{deserialize_from_str, deserialize_from_value, deserialize_from_value_opt};
20+
use crate::utils::common::{deserialize_from_str, deserialize_from_value_opt};
2021
use crate::utils::uuid7_str;
2122

2223
/// Accumulates LLM response chunks from streaming or non-streaming sources.
@@ -178,45 +179,43 @@ impl ResponseAccumulator {
178179
///
179180
/// Non-`data:` lines, `[DONE]`, and malformed JSON are silently skipped.
180181
fn process_sse_line(&mut self, line: &str) {
181-
let Some(data_str) = line.strip_prefix("data: ") else {
182-
return;
183-
};
184-
if data_str == "[DONE]" {
185-
return;
182+
if let Some(frame) = normalize_sse_line(line) {
183+
self.process_event(&frame);
186184
}
187-
let Ok(json) = deserialize_from_str::<serde_json::Value>(data_str) else {
188-
return;
189-
};
185+
}
190186

191-
match json["type"]
192-
.as_str()
193-
.map_or(SSEEventType::Other, |s| s.parse().unwrap_or_default())
194-
{
195-
SSEEventType::ResponseCreated => {
196-
if let Some(id) = json["response"]["id"].as_str() {
197-
self.response_id = id.to_string();
198-
}
187+
/// Processes a typed [`EventFrame`], updating accumulator state.
188+
///
189+
/// This is the core state machine — callers that already have a normalized
190+
/// frame (e.g. [`StreamTee`](future)) can call this directly without
191+
/// re-parsing from a raw line.
192+
pub(crate) fn process_event(&mut self, frame: &EventFrame) {
193+
match (&frame.event_type, &frame.payload) {
194+
(SSEEventType::ResponseCreated, EventPayload::Response { id, .. }) if !id.is_empty() => {
195+
self.response_id.clone_from(id);
199196
}
200-
SSEEventType::ResponseOutputItemAdded => {
197+
(SSEEventType::OutputItemAdded, EventPayload::OutputItemAdded { item_id, .. }) => {
201198
self.finalize_current_message();
202-
let item_id = json["item"]["id"]
203-
.as_str()
204-
.map_or_else(|| uuid7_str("msg_"), str::to_string);
205-
self.current_message = Some(OutputMessage::new(&item_id, MessageStatus::InProgress.as_str()));
199+
let id = if item_id.is_empty() {
200+
uuid7_str("msg_")
201+
} else {
202+
item_id.clone()
203+
};
204+
self.current_message = Some(OutputMessage::new(id, MessageStatus::InProgress.as_str()));
206205
}
207-
SSEEventType::ResponseOutputTextDelta => {
208-
if let Some(delta) = json["delta"].as_str() {
209-
self.accumulated_text.push_str(delta);
210-
}
206+
(SSEEventType::OutputTextDelta, EventPayload::TextDelta { delta, .. }) => {
207+
self.accumulated_text.push_str(delta);
211208
}
212-
SSEEventType::ResponseDone => {
209+
(SSEEventType::ResponseCompleted, EventPayload::Response { usage, .. }) => {
213210
self.finalize_current_message();
214211
self.status = ResponseStatus::Completed;
215-
if let Ok(usage) = deserialize_from_value::<ResponseUsage>(json["response"]["usage"].clone()) {
216-
self.usage = Some(usage);
212+
if let Some(u) = usage {
213+
if let Ok(parsed) = serde_json::from_value::<ResponseUsage>(u.clone()) {
214+
self.usage = Some(parsed);
215+
}
217216
}
218217
}
219-
SSEEventType::Other => {}
218+
_ => {}
220219
}
221220
}
222221

@@ -326,4 +325,144 @@ mod tests {
326325
assert_eq!(MessageStatus::Completed.as_str(), "completed");
327326
assert_eq!(MessageStatus::InProgress.as_str(), "in_progress");
328327
}
328+
329+
// --- process_event tests (exercises the refactored path directly) ---
330+
331+
/// Feeding a `ResponseCreated` `EventFrame` sets the `response_id` on the accumulator.
332+
#[test]
333+
fn test_process_event_response_created_sets_id() {
334+
let mut acc = ResponseAccumulator::new("resp_old".into(), None);
335+
let frame = EventFrame {
336+
event_type: SSEEventType::ResponseCreated,
337+
payload: EventPayload::Response {
338+
id: "resp_new".into(),
339+
status: "in_progress".into(),
340+
usage: None,
341+
},
342+
sequence_number: Some(0),
343+
};
344+
345+
acc.process_event(&frame);
346+
assert_eq!(acc.response_id, "resp_new");
347+
}
348+
349+
/// `ResponseCreated` with empty id should NOT overwrite the existing `response_id`.
350+
#[test]
351+
fn test_process_event_response_created_empty_id_no_overwrite() {
352+
let mut acc = ResponseAccumulator::new("resp_keep".into(), None);
353+
let frame = EventFrame {
354+
event_type: SSEEventType::ResponseCreated,
355+
payload: EventPayload::Response {
356+
id: String::new(),
357+
status: "in_progress".into(),
358+
usage: None,
359+
},
360+
sequence_number: Some(0),
361+
};
362+
363+
acc.process_event(&frame);
364+
assert_eq!(acc.response_id, "resp_keep");
365+
}
366+
367+
/// `TextDelta` events accumulate text which gets attached to the current message.
368+
#[test]
369+
fn test_process_event_text_delta_accumulates() {
370+
let mut acc = ResponseAccumulator::new("resp_1".into(), None);
371+
372+
// Start a message
373+
acc.process_event(&EventFrame {
374+
event_type: SSEEventType::OutputItemAdded,
375+
payload: EventPayload::OutputItemAdded {
376+
item_id: "msg_1".into(),
377+
item_type: "message".into(),
378+
output_index: 0,
379+
name: None,
380+
call_id: None,
381+
},
382+
sequence_number: Some(1),
383+
});
384+
385+
// Feed deltas
386+
acc.process_event(&EventFrame {
387+
event_type: SSEEventType::OutputTextDelta,
388+
payload: EventPayload::TextDelta {
389+
delta: "Hello".into(),
390+
item_id: "msg_1".into(),
391+
output_index: 0,
392+
content_index: 0,
393+
},
394+
sequence_number: Some(2),
395+
});
396+
acc.process_event(&EventFrame {
397+
event_type: SSEEventType::OutputTextDelta,
398+
payload: EventPayload::TextDelta {
399+
delta: " world".into(),
400+
item_id: "msg_1".into(),
401+
output_index: 0,
402+
content_index: 0,
403+
},
404+
sequence_number: Some(3),
405+
});
406+
407+
// Finalize
408+
acc.process_event(&EventFrame {
409+
event_type: SSEEventType::ResponseCompleted,
410+
payload: EventPayload::Response {
411+
id: "resp_1".into(),
412+
status: "completed".into(),
413+
usage: None,
414+
},
415+
sequence_number: Some(4),
416+
});
417+
418+
assert_eq!(acc.status, ResponseStatus::Completed);
419+
assert_eq!(acc.output.len(), 1);
420+
if let OutputItem::Message(msg) = &acc.output[0] {
421+
assert_eq!(msg.content[0].text, "Hello world");
422+
} else {
423+
panic!("expected Message");
424+
}
425+
}
426+
427+
/// `ResponseCompleted` with usage extracts token counts correctly.
428+
#[test]
429+
fn test_process_event_completed_with_usage() {
430+
let mut acc = ResponseAccumulator::new("resp_1".into(), None);
431+
let frame = EventFrame {
432+
event_type: SSEEventType::ResponseCompleted,
433+
payload: EventPayload::Response {
434+
id: "resp_1".into(),
435+
status: "completed".into(),
436+
usage: Some(serde_json::json!({
437+
"input_tokens": 10,
438+
"output_tokens": 5,
439+
"total_tokens": 15
440+
})),
441+
},
442+
sequence_number: Some(9),
443+
};
444+
445+
acc.process_event(&frame);
446+
assert_eq!(acc.status, ResponseStatus::Completed);
447+
assert!(acc.usage.is_some());
448+
assert_eq!(acc.usage.unwrap().total_tokens, 15);
449+
}
450+
451+
/// Unknown/unhandled event types are silently ignored — no panic or state change.
452+
/// Verifies the wildcard `_ => {}` arm works correctly.
453+
#[test]
454+
fn test_process_event_unknown_payload_ignored() {
455+
let mut acc = ResponseAccumulator::new("resp_1".into(), None);
456+
let frame = EventFrame {
457+
event_type: SSEEventType::ContentPartAdded,
458+
payload: EventPayload::Raw(serde_json::json!({"type": "response.content_part.added"})),
459+
sequence_number: Some(3),
460+
};
461+
462+
acc.process_event(&frame);
463+
// No state change — still initial state
464+
assert_eq!(acc.response_id, "resp_1");
465+
assert_eq!(acc.status, ResponseStatus::InProgress);
466+
assert!(acc.output.is_empty());
467+
}
329468
}

0 commit comments

Comments
 (0)