[PLAT-333] share/fork conversation support #8538
Conversation
pjain1
changed the title
runtime/ai/ai.go
Outdated
| // Load the session in the catalog | ||
| session, err := catalog.FindAISession(ctx, opts.SessionID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to find session %q: %w", opts.SessionID, err) | ||
| } | ||
|
|
||
| // Check access: for now, only allow users to access their own sessions or shared sessions with trimmed messages. | ||
| // Checking !SkipChecks to ensure access for superusers and for Rill Developer (where auth is disabled and SkipChecks is true). | ||
| if opts.Claims.UserID != session.OwnerID && !opts.Claims.SkipChecks && session.SharedUntilMessageID == "" { | ||
| return "", fmt.Errorf("access denied to session %q", session.ID) | ||
| } | ||
|
|
||
| retrieveUntilMessageID := session.SharedUntilMessageID | ||
| if session.OwnerID == opts.Claims.UserID || opts.Claims.SkipChecks { | ||
| // If the user owns the session or skipCheck enabled, they can see all messages. | ||
| retrieveUntilMessageID = "" | ||
| } | ||
|
|
||
| var messages []*Message | ||
| ms, err := catalog.FindAIMessages(ctx, opts.SessionID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to find messages for session %q: %w", opts.SessionID, err) | ||
| } | ||
| for _, m := range ms { | ||
| messages = append(messages, &Message{ | ||
| ID: m.ID, | ||
| ParentID: m.ParentID, | ||
| SessionID: m.SessionID, | ||
| Time: m.CreatedOn, | ||
| Index: m.Index, | ||
| Role: Role(m.Role), | ||
| Type: MessageType(m.Type), | ||
| Tool: m.Tool, | ||
| ContentType: MessageContentType(m.ContentType), | ||
| Content: m.Content, | ||
| }) | ||
| // only load messages up to and including that retrieveUntilMessageID; messages are ordered by "Index" ascending. | ||
| if m.ID == retrieveUntilMessageID { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this the same logic as in r.Session()? Can it use r.Session(...).Messages() instead to get the messages?
Or what do you think about making it a on Session, like s.ai.Session(...).Fork(newOwner)?
There was a problem hiding this comment.
Creating a Session does a lot more than what is needed here, like setting up the logger, creating activity client etc. and may add more things in future. Thus it seems unnecessary, mainly we want messages with some access checks so not much logic there anyways.
There was a problem hiding this comment.
Session() is actually meant to be the single entrypoint for accessing a session, to make sure we parse DB messages and enforce access checks uniformly. So it would be nice to avoid the duplication.
The logger/activity clients are cheap, so no need to worry about those. And if we need to add other non-cheap logic in the future, we can do it in a lazy-loading way to make sure Session remains useful as the single entrypoint for all use cases.
There was a problem hiding this comment.
Changed to load session and use messages from it. Again forking from existing session will require double copying of messages one from session message to drivers.AIMessage for inserting into catalog and also to ai.Message for loading the Session. Just avoiding unnecessary work but otherwise Fork method on a Session can be supported.
begelundmuller
left a comment
begelundmuller
left a comment
There was a problem hiding this comment.
@pjain1 I also responded with some suggestions on the unresolved comments above :)
Adds supports for sharing and forking shared conversations. Once a conversation is shared, anyone knowing the conversation id can access till the shared message. User can then start a new conversation from it which copies over the shared messages to new conversation.
Now after forking new user might not have same access as previous in which case they might get access errors on continuing conversation, we let LLM know about this by injecting this info in the system prompt for shared conversations. On testing I have seen replies like this from LLM
Unhandled case - However, there is an edge case around chart message. Existing charts does MV agg API calls which will fail if the new user does not have access to the underlying mv. So charts will just show spinner.
New APIs
ShareConversation, optionally acceptsshared_until_message_id, returns nothing: Only owners (or rill dev) may call. Validates the provided message exists in the caller’s view. Persistsshared_until_message_idas the last router result at or before the requested message; if omitted, uses the latest router result.ForkConversation, accepts nothing and returns newconversation_id: Clones the conversation; owners get all messages, non-owners only get messages up to the shared boundary.Checklist: