add summary operation to codex-rs/core #1527
Conversation
aibrahim-oai
changed the title
codex-rs/core/src/codex.rs
Outdated
|
|
||
| // Create a summarization request as user input | ||
| const SUMMARIZATION_PROMPT: &str = concat!( | ||
| "Please provide a summary of our conversation so far, highlighting key points, ", |
There was a problem hiding this comment.
I would favor a raw string (r#) over concat!() here (unless you really want this to be one line of text?): https://doc.rust-lang.org/rust-by-example/std/str.html#literals-and-escapes
codex-rs/core/src/codex.rs
Outdated
| // Attempt to inject input into current task | ||
| if let Err(items) = sess.inject_input(summarization_prompt) { | ||
| // No current task, spawn a new one | ||
| let task = AgentTask::spawn(Arc::clone(sess), sub.id, items); |
There was a problem hiding this comment.
Does this work? I think this is the more canonical way to do it.
| let task = AgentTask::spawn(Arc::clone(sess), sub.id, items); | |
| let task = AgentTask::spawn(sess.clone(), sub.id, items); |
There was a problem hiding this comment.
This is how it's done here:
codex/codex-rs/core/src/codex.rs
Line 711 in bfeb8c9
There was a problem hiding this comment.
Does what I suggested not work?
|
|
||
| // Wait for session configured | ||
| loop { | ||
| let event = timeout(Duration::from_secs(5), codex.next_event()) |
There was a problem hiding this comment.
Consider using codex_wrapper.rs instead (and you can remove the loop):
codex/codex-rs/core/src/codex_wrapper.rs
Line 15 in bfeb8c9
| // } | ||
| let _sub_id = codex.submit(Op::SummarizeContext).await.unwrap(); | ||
|
|
||
| // Should receive a TaskStarted event indicating a new AgentTask was spawned |
There was a problem hiding this comment.
I think this comment is duplicative with the message to your assert!(), so I would favor keeping the one in assert!() over this one.
|
|
||
| #[tokio::test] | ||
| async fn test_summarize_context_injects_into_running_task() { | ||
| // Test that when a task IS running, SummarizeContext injects into the existing task |
There was a problem hiding this comment.
Again, prefer doc comment.
| // rather than spawning a new one. We shouldn't get another TaskStarted event | ||
| let result = timeout(Duration::from_millis(500), codex.next_event()).await; | ||
|
|
||
| // If we get an event, it should NOT be TaskStarted (since we're injecting into existing task) |
There was a problem hiding this comment.
Hmm, this does not feel like a strong verification. Can we instead test for what we should get instead of one possible thing we shouldn't get?
|
@aibrahim-oai can you please fix CI? seems like mostly warnings promoted to errors? |
Add operation to summarize the context so far.