feat: introduce client event system#106
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an event-based inbound processing API (“event system v2”) across the core (libchat), Rust client (logos_chat), CLI, and FFI layers, replacing the previous ContentData/is_new_convo model with ordered Event lists.
Changes:
- Replace
Option<ContentData>returns withVec<Event>in core inbound processing (Context::handle_payload,Convo::handle_frame, inbox handlers) and map core events to app-facing client events. - Update Rust tests/examples and CLI to consume
Event::{ConversationStarted, MessageReceived}instead ofContentData. - Update the C FFI API to return an
EventList(vector of event rows) fromclient_receive, and adjust the C example accordingly.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| flake.nix | Adjust dev shell inputs (adds perl, relaxes arg pattern). |
| crates/client/tests/saro_and_raya.rs | Update client integration test to validate ordered Event output. |
| crates/client/src/lib.rs | Re-export new client Event and conversation kind types. |
| crates/client/src/event.rs | Add app-facing logos_chat::Event type. |
| crates/client/src/client.rs | Change ChatClient::receive to return Vec<Event> and map from core events. |
| crates/client/examples/message-exchange/main.rs | Update example to consume events. |
| crates/client-ffi/src/api.rs | Replace old “push inbound result” with EventList-based receive API. |
| crates/client-ffi/examples/message-exchange/src/main.c | Update C example to iterate/read events from EventList. |
| core/integration_tests_core/tests/private_integration.rs | Update core integration tests to assert on core Event stream. |
| core/integration_tests_core/tests/mls_integration.rs | Update MLS integration test harness to consume events and assert ConversationStarted. |
| core/conversations/src/types.rs | Remove ContentData from core types. |
| core/conversations/src/lib.rs | Export core Event and ConversationKind; stop exporting ContentData. |
| core/conversations/src/inbox/handler.rs | Emit [ConversationStarted, …] events for private invites. |
| core/conversations/src/inbox_v2.rs | Return events from PQ/MLS inbox handling (e.g., group welcome => ConversationStarted). |
| core/conversations/src/event.rs | Add core libchat::Event type for inbound observations. |
| core/conversations/src/conversation/privatev1.rs | Emit MessageReceived events instead of ContentData. |
| core/conversations/src/conversation/group_v1.rs | Emit MessageReceived events instead of ContentData. |
| core/conversations/src/conversation.rs | Update Convo trait to return Vec<Event>. |
| core/conversations/src/context.rs | Update Context::handle_payload/dispatch functions to return Vec<Event>. |
| bin/chat-cli/src/app.rs | Switch CLI inbound processing to an event handler. |
| .gitignore | Fix ignored path for built C example binary. |
Comments suppressed due to low confidence (1)
core/conversations/src/types.rs:56
ContentDatawas removed from the core types, but there are still in-repo references (e.g.core/conversations/src/conversation/group_test.rsimportstypes::ContentDataand implementsConvo::handle_framereturningResult<Option<ContentData>, _>). This will fail to compile unless those remaining call sites are migrated to the newVec<Event>-based API or removed.
}
// Internal type Definitions
// Used by Conversations to attach addresses to outbound encrypted payloads
pub struct AddressedEncryptedPayload {
pub delivery_address: String,
pub data: proto::EncryptedPayload,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b5b0341 to
2ca12a5
Compare
2ca12a5 to
cbe9e53
Compare
jazzz
left a comment
There was a problem hiding this comment.
This looks like a good change to me.
If @kaichaosun is onboard then so am I
jazzz
left a comment
There was a problem hiding this comment.
I think there is some clean up to address, but once you are happy with this I'm happy with this.
Some general feedback for the future #RelentlessImprovment
-
Having PRs span multiple crates is hard to follow and and takes significantly more time to review. e.g. This PR Updates the Core, Client, FFI and test application.
- Specifically the FFI updates are very easy for me to gloss over. I'm definitely not giving them the attention they deserve.
- If we can find a workflow that breaks these up that would be a huge improvement to the team.
- Crawl, walk, run -> Core, Client, Rest
-
Were starting a push for V0.2 so not an issue this month, but I'm starting to notice our functions are becoming deeper, busier, longer.
- I'm not suggesting we strictly follow the '5 line function principal', but a soft reminder that readability is important.
- Were looking for symmetry not necessarily short.
- Good steps in the right direction: 👍
3529cee to
addcde0
Compare
|
@jazzz @kaichaosun thanks for review. I’ve reiterated over the solution to address your feedback. I’ve also simplified the ADR a lot, as it contained too many implementation details. Please get familiar with it first when attempting a re-review. |
There was a problem hiding this comment.
Well done on the refactor 👍 .
I have also tried to build on your branch and see what other option exists, after some playaround, I got this commit in branch feat/ces-v2-kc with extra event MessageMissing.
It loses some generality with concrete types, but it offers more clarity. Feel free to take a look and see whether it makes sense in your view.
The underlying idea is that, before we have a mature protocol design, the code should not be overly abstracted.
Approved this PR since it's already working, further refactor can be handled later.
forAllSystems calls the lambda with {system, pkgs}; strict
destructuring requires `..` to ignore the system attribute.
`pkgs.perl` is needed because openssl-sys is pulled vendored via
libsqlite3-sys / rusqlite / chat-sqlite, and its `perl Configure`
step needs FindBin.pm, which Fedora's system perl doesn't ship.
addcde0 to
4bdaef3
Compare
jazzz
left a comment
There was a problem hiding this comment.
I'm liking the clean up, and the iteration.
This latest change has positive additions:
- Specific types for Specific paths.
- Client side events look great to me.
I have questions about the backend that I'd like some answers to.
There are also some Pebbles I'd like to understand the plan before an explicit approval.
With an action plan for future work I don't see a blocker here - specifically as the client side looks solid.
| pub struct InboundResult { | ||
| /// A new conversation appeared from this payload, if any. | ||
| pub new_conversation: Option<NewConversation>, | ||
| /// Observations from the frame inside this payload. | ||
| pub frame: FrameOutcome, | ||
| } |
There was a problem hiding this comment.
[?] Comparing the previous and current approach in a flattened state I have some questions.
ContentData
Struct {
convo_id
Vec<u8>
bool
}
InboundResult
Struct {
Option<(convo_id)>
Vec<convo_id, Vec<u8>>
}
Notes:
bool -->> Option< _ >:: I think this makes sense- Option is clearer with the associated convo_id
- However this isn't meaningfully different than before
- convo_id + Bytes -->> Vec< convo_id , Bytes > :: This I'm not so sure about
- Cardinality : Previously each inbound payload could generate at most one message. I'm not opposed to allowing multiple as a future proofing approach, but is there a usecase you can think of where this is needed?
- ConvoMultiplexing : Previously each payload was associated to a single conversation. This format allows for FrameOutput to contain different convo_ids. Can you explain why that is needed? It appears to be a significant change to the parsing rules.
There was a problem hiding this comment.
I'm having a hard time seeing how InboundResult is a notable improvement over
ContentData++
pub struct ProcessingOutcome { // Change name
convo_id: String, // Keeps processing simple as conversation_id is enforced by type.
content: Vec< Vec<u8> >, // Allow multiple messages
is_new_conversation: bool // Option flattens to bool, as the identifier is already being used
}
Am I missing something important?
There was a problem hiding this comment.
If the blocker is restricting what Conversations can do compared to Inboxes and other objects.
[Sand] Consider using a dedicated enum for each pathway, and then implementing From<_> to define a mapping.
There was a problem hiding this comment.
- Cardinality : Previously each inbound payload could generate at most one message. I'm not opposed to allowing multiple as a future proofing approach, but is there a usecase you can think of where this is needed?
I don't see usecase now and I agree it is premature. Switched back to one message.
ConvoMultiplexing : Previously each payload was associated to a single conversation. This format allows for FrameOutput to contain different convo_ids. Can you explain why that is needed? It appears to be a significant change to the parsing rules.
Same as above, premature. Removed.
There was a problem hiding this comment.
I'm having a hard time seeing how InboundResult is a notable improvement over
ContentData++
Am I missing something important?
No, you are not missing anything. The back-and-forth is what muddied it. The core went ContentData -> events -> back to a structured result that landed functionally equivalent to the original. Your dedicated PayloadOutcome enum (Empty / Convo / Inbox) is the real improvement, so I've adopted it.
4bdaef3 to
6f70600
Compare
| c if c == self.inbox.id() => self.dispatch_to_inbox(&env.payload).map(Into::into), | ||
| c if c == self.pq_inbox.id() => self.dispatch_to_inbox2(&env.payload).map(Into::into), |
There was a problem hiding this comment.
[Dust] Change return type of dispatch functions to handle the conversion into PayloadOutcome.
This would reduce the clutter at the callsite.
a2ac94c to
86617ff
Compare
- Core processing yields a `PayloadOutcome` enum — `Empty`, `Convo`, or `Inbox`. `ConvoOutcome` carries a conversation id and an optional decrypted `Content`; `InboxOutcome` adds a `NewConversation` (id + `ConversationClass`) for a peer-initiated conversation. - Client translates `PayloadOutcome` into app-facing `Vec<Event>` (`ConversationStarted`, `MessageReceived`) at the boundary, so the application loop sees discrete events rather than core types. - MLS group welcomes produce a `ConversationStarted` event with no initial content, fixing the silent-group-join case where the inbox layer dropped the observation. - C FFI exposes an `EventList` opaque type with indexed accessors and an `Invalid` sentinel for out-of-bounds / non-applicable reads. - Symmetric `Inbox` / `InboxV2` handlers: both return `Result<InboxOutcome, _>` and own the persistence + ephemeral-key cleanup for the conversations they create. - Updated and simplified `docs/adr/0001-client-event-system.md`.
86617ff to
4ee7ddf
Compare
Temporary. The two crates.io UA fixes (NixOS/nixpkgs#512735 for fetchCargoVendor's python-requests UA, NixOS/nixpkgs#524985 for importCargoLock's curl UA) haven't propagated to nixos-unstable yet. Switch to nixos-unstable-small and force logos-delivery to follow so the smoketest gets the same fix. Revert once nixos-unstable catches up. Refs: - rust-lang/crates.io#13482 - rust-lang/crates.io#13783 - https://crates.io/data-access
4ee7ddf to
038447f
Compare
PayloadOutcomeenum —Empty,Convo, orInbox.ConvoOutcomecarries a conversation id and an optionaldecrypted
Content;InboxOutcomeadds aNewConversation(id +
ConversationClass) for a peer-initiated conversation.PayloadOutcomeinto app-facingVec<Event>(
ConversationStarted,MessageReceived) at the boundary, so theapplication loop sees discrete events rather than core types.
ConversationStartedevent with noinitial content, fixing the silent-group-join case where the inbox
layer dropped the observation.
EventListopaque type with indexed accessors andan
Invalidsentinel for out-of-bounds / non-applicable reads.Inbox/InboxV2handlers: both returnResult<InboxOutcome, _>and own the persistence + ephemeral-keycleanup for the conversations they create.
docs/adr/0001-client-event-system.md.