feat: make workspaceAgentAddChatContext use the chatd state machine - #26112
Conversation
8fa644f to
aef0c15
Compare
efdf24f to
1627cf8
Compare
hugodutka
changed the title
1627cf8 to
d0f134b
Compare
aef0c15 to
6c84888
Compare
| if errors.Is(err, chatstate.ErrMessageQueueFull) { | ||
| var queueFull *chatstate.MessageQueueFullError | ||
| detail := "" | ||
| if errors.As(err, &queueFull) { | ||
| detail = fmt.Sprintf("Maximum %d messages can be queued.", queueFull.Max) | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusTooManyRequests, codersdk.Response{ | ||
| Message: "Message queue is full.", | ||
| Detail: detail, | ||
| }) | ||
| return | ||
| } | ||
| if errors.Is(err, chatstate.ErrInvalidState) { | ||
| httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ | ||
| Message: "Chat is in an invalid state.", | ||
| }) | ||
| return | ||
| } | ||
| if errors.Is(err, chatstate.ErrTransitionNotAllowed) { | ||
| httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ | ||
| Message: "Chat is not in a state that accepts new context.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| if errors.Is(err, chatstate.ErrChatNotFound) { | ||
| writeAgentChatError(ctx, rw, errChatNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
suggestion, non-blocking: maybe rewrite this as a switch?
| func agentChatContextStateMessage( | ||
| content pqtype.NullRawMessage, | ||
| modelConfigID uuid.UUID, | ||
| ownerID uuid.UUID, | ||
| apiKeyID string, | ||
| ) chatstate.Message { | ||
| return chatstate.Message{ | ||
| Role: database.ChatMessageRoleUser, | ||
| Content: content, | ||
| Visibility: database.ChatMessageVisibilityBoth, | ||
| ModelConfigID: uuid.NullUUID{UUID: modelConfigID, Valid: modelConfigID != uuid.Nil}, | ||
| CreatedBy: uuid.NullUUID{UUID: ownerID, Valid: ownerID != uuid.Nil}, | ||
| ContentVersion: chatprompt.CurrentContentVersion, | ||
| APIKeyID: sql.NullString{String: apiKeyID, Valid: apiKeyID != ""}, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion, non-blocking: this is only used in one place, just inline it for clarity.
| if len(sendResult.InsertedMessages) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This seems like an error case? We're trying to send a message, but the result was that nothing was sent.
There was a problem hiding this comment.
From what I was able to gather, It's not an error. SendMessage will return no InsertedMessages if it queued up a message because it had to interrupt the chat and the chat is now in the interrupting state. So the last injected context won't be updated. But updating the last injected context column is described as "best effort" in other parts of the codebase, and its value doesn't have to be up to date. Looking at how it's used across the codebase, I think the "last updated context" column is a shoddy design and should be removed altogether.
d0f134b to
b17003c
Compare
6c84888 to
8641dd3
Compare
| } | ||
| if errors.Is(err, chatstate.ErrInvalidState) { | ||
| httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ | ||
| Message: "Chat is in an invalid state.", |
There was a problem hiding this comment.
Do we need some type of debug logging or detail here? How may someone go about resolving this issue?
There was a problem hiding this comment.
My current solution is to instruct admins in docs to use the /api/experimental/chats/{chat}/reconcile-invalid endpoint to get the chats out of an invalid state, but that's an awkward solution. I was thinking the archive/unarchive endpoints and the auto-archive loop could auto-apply invalid state reconciliation too.
Regarding logging, I think it would be best to add it in machine.Update whenever a transaction fails because of an invalid state error.
I would rather not do it now and address that in a follow up PR though - I added that as a Linear issue: https://linear.app/codercom/issue/CODAGT-584/automatic-invalid-state-reconciliation
8641dd3 to
d58c5ca
Compare
a0c779b to
636b780
Compare
636b780 to
6e2d222
Compare
d58c5ca to
1e945c4
Compare
6e2d222 to
4e54b39
Compare
1e945c4 to
218f6eb
Compare
4e54b39 to
e0343d2
Compare
218f6eb to
99da06d
Compare
e0343d2 to
1eeba2f
Compare
99da06d to
19ee932
Compare
1eeba2f to
f8b7421
Compare
19ee932 to
5cd93b7
Compare
f8b7421 to
4a27a74
Compare
c00a9ca to
323dc9b
Compare
3839cc9 to
a174ac4
Compare
323dc9b to
344ef41
Compare
a174ac4 to
f8e235d
Compare
3 participants
PR 6 of the chatd refactor. Originally not included in the RFC - I wasn't aware that
workspaceAgentAddChatContextexisted. It seems to be an internal endpoint that the workspace agent can call to inject additional context into the chat. I don't think it's used in production today, but I refactored it either way.Previous PR in the stack: #26111