Skip to content

Conversation

bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Aug 12, 2025

This PR does two things because after I got deep into the first one I started pulling on the thread to the second:

  • Makes ConversationManager the place where all in-memory conversations are created and stored. Previously, MessageProcessor in the codex-mcp-server crate was doing this via its session_map, but this is something that should be done in codex-core.
  • It unwinds the ctrl_c: tokio::sync::Notify that was threaded throughout our code. I think this made sense at one time, but now that we handle Ctrl-C within the TUI and have a proper Op::Interrupt event, I don't think this was quite right, so I removed it. For codex exec and codex proto, we now use tokio::signal::ctrl_c() directly, but we no longer make Notify a field of Codex or CodexConversation.

Changes of note:

  • Adds the files conversation_manager.rs and codex_conversation.rs to codex-core.
  • Codex and CodexSpawnOk are no longer exported from codex-core: other crates must use CodexConversation instead (which is created via ConversationManager).
  • core/src/codex_wrapper.rs has been deleted in favor of ConversationManager.
  • ConversationManager::new_conversation() returns NewConversation, which is in line with the new_conversation tool we want to add to the MCP server. Note NewConversation includes SessionConfiguredEvent, so we eliminate checks in cases like codex-rs/core/tests/client.rs to verify SessionConfiguredEvent is the first event because that is now internal to ConversationManager.
  • Quite a bit of code was deleted from codex-rs/mcp-server/src/message_processor.rs since it no longer has to manage multiple conversations itself: it goes through ConversationManager instead.
  • core/tests/live_agent.rs has been deleted because I had to update a bunch of tests and all the tests in here were ignored, and I don't think anyone ever ran them, so this was just technical debt, at this point.
  • Removed notify_on_sigint() from util.rs (and in a follow-up, I hope to refactor the blandly-named util.rs into more descriptive files).
  • In general, I started replacing local variables named codex as conversation, where appropriate, though admittedly I didn't do it through all the integration tests because that would have added a lot of noise to this PR.

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest force-pushed the pr2240 branch 7 times, most recently from bb9e75c to 7fdd266 Compare August 13, 2025 17:09
@bolinfest bolinfest marked this pull request as ready for review August 13, 2025 17:27
@bolinfest bolinfest force-pushed the pr2240 branch 2 times, most recently from 5155b06 to 18ecca4 Compare August 13, 2025 17:36
@bolinfest bolinfest added the codex-rust-review Perform a detailed review of Rust changes. label Aug 13, 2025
@github-actions github-actions bot added codex-rust-review-in-progress and removed codex-rust-review Perform a detailed review of Rust changes. labels Aug 13, 2025
Copy link

Summary

  • Introduces ConversationManager and CodexConversation to manage conversations; removes CodexWrapper and ctrl-c plumbing in core. Updates CLI and server code to use the new abstractions.

Notes

  • New error variants added for conversation lookup and first-event validation.
  • Ctrl-C handling moves from Notify tokens to tokio::signal::ctrl_c() usage.
  • Tests and exports updated to reflect new types.

Review
Overall, this is a clean API refactor toward a conversation-centric model. A few polish and safety suggestions:

  • codex-rs/cli/src/proto.rs

    • At L36–L43: Consider surfacing session_configured info or logging it to preserve prior UX parity. It’s currently constructed then dropped.
    • At L52 and L89: tokio::signal::ctrl_c() returns a Result—use _ = tokio::signal::ctrl_c() => is fine, but ensure other branches don’t swallow errors. Optionally log on interrupt for user feedback.
    • At L66: Error message uses {e:?}; prefer {e} unless Debug carries needed context.
  • codex-rs/core/src/codex.rs

    • At L572: abort is now private. Verify no external callers relied on it; otherwise document new shutdown behavior.
    • At L717: Removing interrupt-driven cancellation means long-running execs only stop when the channel closes. If intentional, add a short comment on rationale, since SIGINT now only affects top-level loops.
    • At L154: Spawned task no longer receives a cancel signal; consider a graceful shutdown path on Drop or a channel close comment for maintainers.
  • codex-rs/core/src/codex_conversation.rs

    • New wrapper is thin and good. Consider deriving Clone for CodexConversation if it’s meant to be held by Arcs across layers (saves re-wrapping patterns later).
  • codex-rs/core/src/conversation_manager.rs

    • At L36–L43: Nice first-event validation. The dedicated SessionConfiguredNotFirstEvent error is clear.
    • At L61–L69: Stores conversations in a RwLock<HashMap<Uuid, Arc<_>>>. Consider a DashMap if contention grows, but RwLock is fine to start.
    • At L83–L96: get_conversation returns CodexErr::ConversationNotFound(id); great. Consider adding a remove_conversation for lifecycle cleanup.
  • codex-rs/core/src/error.rs

    • New variants look good; ensure error enum ordering remains roughly grouped by domain.
  • codex-rs/core/src/exec.rs

    • At L363: Switched to tokio::signal::ctrl_c() inside consume_truncated_output. That globally traps SIGINT per task; be mindful if multiple execs run concurrently. An alternative is a per-call cancellation primitive passed in, but this might be acceptable.
  • codex-rs/core/src/lib.rs

    • Export changes are consistent. Removing codex_wrapper is the right call given ConversationManager.
  • codex-rs/mcp-server/src/conversation_loop.rs

    • Type change to Arc<CodexConversation> looks consistent. Check other handlers similarly updated.

Follow-ups (outside this PR scope)

  • Provide a minimal example in docs showing ConversationManager usage.
  • Add a shutdown API on ConversationManager to drop conversations and signal background tasks.

View workflow run

@bolinfest bolinfest force-pushed the pr2240 branch 3 times, most recently from 335c1af to 8fe92eb Compare August 13, 2025 18:07
@bolinfest bolinfest changed the title chore: introduce CodexServer as a clearinghouse for all conversations chore: introduce ConversationManager as a clearinghouse for all conversations Aug 13, 2025
@bolinfest bolinfest merged commit 08ed618 into main Aug 13, 2025
26 checks passed
@bolinfest bolinfest deleted the pr2240 branch August 13, 2025 20:38
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants