From f8fe5d680b3a325c7198ccce195f934a1a51e2fb Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 23 Apr 2026 19:03:33 +0300 Subject: [PATCH] fix(coderd): reject API operations on archived chats (#24633) Archived chats accept mutations (messages, edits, queued-message promotions, tool-result submissions) via the API, causing them to re-enter the processing pipeline. This violates the hard-stop design intent from PR #23758. Add archived checks at three layers: - HTTP handlers (postChatMessages, patchChatMessage, promoteChatQueuedMessage, postChatToolResults): return 400 after auth so callers get a clear error. - Daemon functions (SendMessage, EditMessage, PromoteQueued, SubmitToolResults): return ErrChatArchived after row lock, guarding against future callers that bypass the handler. - AcquireChats SQL: filter out archived chats so they are never acquired for processing. Fixes CODAGT-245 --- coderd/database/queries.sql.go | 1 + coderd/database/queries/chats.sql | 1 + coderd/exp_chats.go | 48 +++++++ coderd/exp_chats_test.go | 152 +++++++++++++++++++++ coderd/x/chatd/chatd.go | 20 +++ coderd/x/chatd/chatd_test.go | 215 ++++++++++++++++++++++++++++++ 6 files changed, 437 insertions(+) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 94f9f58589..1be67fee0d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5077,6 +5077,7 @@ WHERE chats WHERE status = 'pending'::chat_status + AND archived = false ORDER BY updated_at ASC FOR UPDATE diff --git a/coderd/database/queries/chats.sql b/coderd/database/queries/chats.sql index 0c1bd1bb02..3365e943bb 100644 --- a/coderd/database/queries/chats.sql +++ b/coderd/database/queries/chats.sql @@ -695,6 +695,7 @@ WHERE chats WHERE status = 'pending'::chat_status + AND archived = false ORDER BY updated_at ASC FOR UPDATE diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index baa46bc07a..d6c1153a20 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -2356,6 +2356,13 @@ func (api *API) postChatMessages(rw http.ResponseWriter, r *http.Request) { return } + if chat.Archived { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot send messages to an archived chat.", + }) + return + } + if api.chatDaemon == nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Chat processor is unavailable.", @@ -2453,6 +2460,12 @@ func (api *API) postChatMessages(rw http.ResponseWriter, r *http.Request) { if maybeWriteLimitErr(ctx, rw, sendErr) { return } + if xerrors.Is(sendErr, chatd.ErrChatArchived) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot send messages to an archived chat.", + }) + return + } if xerrors.Is(sendErr, chatd.ErrMessageQueueFull) { httpapi.Write(ctx, rw, http.StatusTooManyRequests, codersdk.Response{ Message: "Message queue is full.", @@ -2496,6 +2509,13 @@ func (api *API) patchChatMessage(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) chat := httpmw.ChatParam(r) + if chat.Archived { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot edit messages in an archived chat.", + }) + return + } + if api.chatDaemon == nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Chat processor is unavailable.", @@ -2540,6 +2560,10 @@ func (api *API) patchChatMessage(rw http.ResponseWriter, r *http.Request) { } switch { + case xerrors.Is(editErr, chatd.ErrChatArchived): + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot edit messages in an archived chat.", + }) case xerrors.Is(editErr, chatd.ErrEditedMessageNotFound): httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Chat message not found.", @@ -2625,6 +2649,13 @@ func (api *API) promoteChatQueuedMessage(rw http.ResponseWriter, r *http.Request return } + if chat.Archived { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot promote queued messages in an archived chat.", + }) + return + } + queuedMessageIDStr := chi.URLParam(r, "queuedMessage") queuedMessageID, err := strconv.ParseInt(queuedMessageIDStr, 10, 64) if err != nil { @@ -2653,6 +2684,12 @@ func (api *API) promoteChatQueuedMessage(rw http.ResponseWriter, r *http.Request if maybeWriteLimitErr(ctx, rw, txErr) { return } + if xerrors.Is(txErr, chatd.ErrChatArchived) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot promote queued messages in an archived chat.", + }) + return + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to promote queued message.", Detail: txErr.Error(), @@ -6726,6 +6763,13 @@ func (api *API) postChatToolResults(rw http.ResponseWriter, r *http.Request) { return } + if chat.Archived { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot submit tool results to an archived chat.", + }) + return + } + // Cap the raw request body to prevent excessive memory use. r.Body = http.MaxBytesReader(rw, r.Body, int64(2*maxSystemPromptLenBytes)) var req codersdk.SubmitToolResultsRequest @@ -6767,6 +6811,10 @@ func (api *API) postChatToolResults(rw http.ResponseWriter, r *http.Request) { var validationErr *chatd.ToolResultValidationError var conflictErr *chatd.ToolResultStatusConflictError switch { + case xerrors.Is(err, chatd.ErrChatArchived): + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Cannot submit tool results to an archived chat.", + }) case errors.As(err, &conflictErr): httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ Message: "Chat is not waiting for tool results.", diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index 7fe9beef69..505838c270 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -5786,6 +5786,38 @@ func TestPostChatMessages(t *testing.T) { }) requireSDKError(t, err, http.StatusNotFound) }) + + t.Run("ArchivedChat", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + _ = createChatModelConfig(t, client) + + chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "hello", + }}, + }) + require.NoError(t, err) + + err = client.UpdateChat(ctx, chat.ID, codersdk.UpdateChatRequest{ + Archived: ptr.Ref(true), + }) + require.NoError(t, err) + + _, err = client.CreateChatMessage(ctx, chat.ID, codersdk.CreateChatMessageRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "should fail", + }}, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Contains(t, sdkErr.Message, "archived") + }) } func TestChatMessageWithFileReferences(t *testing.T) { @@ -6950,6 +6982,50 @@ func TestPatchChatMessage(t *testing.T) { require.Len(t, chatResult.Files, codersdk.MaxChatFileIDs, "file count should not exceed the cap") }) + + t.Run("ArchivedChat", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + _ = createChatModelConfig(t, client) + + chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "hello before edit", + }}, + }) + require.NoError(t, err) + + messagesResult, err := client.GetChatMessages(ctx, chat.ID, nil) + require.NoError(t, err) + + var userMessageID int64 + for _, message := range messagesResult.Messages { + if message.Role == codersdk.ChatMessageRoleUser { + userMessageID = message.ID + break + } + } + require.NotZero(t, userMessageID) + + err = client.UpdateChat(ctx, chat.ID, codersdk.UpdateChatRequest{ + Archived: ptr.Ref(true), + }) + require.NoError(t, err) + + _, err = client.EditChatMessage(ctx, chat.ID, userMessageID, codersdk.EditChatMessageRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "should fail", + }}, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Contains(t, sdkErr.Message, "archived") + }) } func TestStreamChat(t *testing.T) { @@ -8021,6 +8097,56 @@ func TestPromoteChatQueuedMessage(t *testing.T) { defer promoteRes.Body.Close() require.Equal(t, http.StatusForbidden, promoteRes.StatusCode) }) + + t.Run("ArchivedChat", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createChatModelConfig(t, client) + + chat, err := db.InsertChat(dbauthz.AsSystemRestricted(ctx), database.InsertChatParams{ + OrganizationID: user.OrganizationID, + Status: database.ChatStatusWaiting, + ClientType: database.ChatClientTypeUi, + OwnerID: user.UserID, + LastModelConfigID: modelConfig.ID, + Title: "promote queued archived", + }) + require.NoError(t, err) + + queuedContent, err := json.Marshal([]codersdk.ChatMessagePart{ + codersdk.ChatMessageText("queued"), + }) + require.NoError(t, err) + queuedMessage, err := db.InsertChatQueuedMessage( + dbauthz.AsSystemRestricted(ctx), + database.InsertChatQueuedMessageParams{ + ChatID: chat.ID, + Content: queuedContent, + }, + ) + require.NoError(t, err) + + // Archive the chat. + _, err = db.ArchiveChatByID(dbauthz.AsSystemRestricted(ctx), chat.ID) + require.NoError(t, err) + + promoteRes, err := client.Request( + ctx, + http.MethodPost, + fmt.Sprintf("/api/experimental/chats/%s/queue/%d/promote", chat.ID, queuedMessage.ID), + nil, + ) + require.NoError(t, err) + defer promoteRes.Body.Close() + require.Equal(t, http.StatusBadRequest, promoteRes.StatusCode) + promoteErr := codersdk.ReadBodyAsError(promoteRes) + var promoteSDKErr *codersdk.Error + require.ErrorAs(t, promoteErr, &promoteSDKErr) + require.Contains(t, promoteSDKErr.Message, "archived") + }) } func TestChatUsageLimitOverrideRoutes(t *testing.T) { @@ -11339,6 +11465,32 @@ func TestSubmitToolResults(t *testing.T) { }) requireSDKError(t, err, http.StatusForbidden) }) + + t.Run("ArchivedChat", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createChatModelConfig(t, client) + + const toolName = "my_dynamic_tool" + toolCallIDs := []string{"call_archived"} + + chat := setupRequiresAction(ctx, t, db, user.UserID, user.OrganizationID, modelConfig.ID, toolName, toolCallIDs) + + // Archive the chat. + _, err := db.ArchiveChatByID(dbauthz.AsSystemRestricted(ctx), chat.ID) + require.NoError(t, err) + + err = client.SubmitToolResults(ctx, chat.ID, codersdk.SubmitToolResultsRequest{ + Results: []codersdk.ToolResult{ + {ToolCallID: "call_archived", Output: json.RawMessage(`"should fail"`)}, + }, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Contains(t, sdkErr.Message, "archived") + }) } func TestPostChats_DynamicToolValidation(t *testing.T) { diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 944e48f7d1..93b851f5c1 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -878,6 +878,10 @@ var ( ErrEditedMessageNotFound = xerrors.New("edited message not found") // ErrEditedMessageNotUser indicates a non-user message edit attempt. ErrEditedMessageNotUser = xerrors.New("only user messages can be edited") + // ErrChatArchived indicates the chat is archived and cannot + // accept modifications (messages, edits, promotions, or + // tool-result submissions). + ErrChatArchived = xerrors.New("chat is archived") // errChatTakenByOtherWorker is a sentinel used inside the // processChat cleanup transaction to signal that another @@ -1194,6 +1198,10 @@ func (p *Server) SendMessage( return xerrors.Errorf("lock chat: %w", err) } + if lockedChat.Archived { + return ErrChatArchived + } + // Enforce usage limits before queueing or inserting. if limitErr := p.checkUsageLimit(ctx, tx, lockedChat.OwnerID, uuid.NullUUID{UUID: lockedChat.OrganizationID, Valid: true}); limitErr != nil { return limitErr @@ -1384,6 +1392,10 @@ func (p *Server) EditMessage( return xerrors.Errorf("lock chat: %w", err) } + if lockedChat.Archived { + return ErrChatArchived + } + if limitErr := p.checkUsageLimit(ctx, tx, lockedChat.OwnerID, uuid.NullUUID{UUID: lockedChat.OrganizationID, Valid: true}); limitErr != nil { return limitErr } @@ -1743,6 +1755,11 @@ func (p *Server) PromoteQueued( if err != nil { return xerrors.Errorf("lock chat: %w", err) } + + if lockedChat.Archived { + return ErrChatArchived + } + modelConfigID := lockedChat.LastModelConfigID if opts.ModelConfigID != nil { modelConfigID = *opts.ModelConfigID @@ -1881,6 +1898,9 @@ func (p *Server) SubmitToolResults( if lockErr != nil { return xerrors.Errorf("lock chat for update: %w", lockErr) } + if locked.Archived { + return ErrChatArchived + } if locked.Status != database.ChatStatusRequiresAction { statusConflict = &ToolResultStatusConflictError{ ActualStatus: locked.Status, diff --git a/coderd/x/chatd/chatd_test.go b/coderd/x/chatd/chatd_test.go index def80c3312..3e27322eb5 100644 --- a/coderd/x/chatd/chatd_test.go +++ b/coderd/x/chatd/chatd_test.go @@ -7294,3 +7294,218 @@ func TestAgentContextFilesAndSkillsLoadedIntoChat(t *testing.T) { require.Zero(t, standalonePlanBlockCount, "plan-file-path block should be part of the main system prompt, not a standalone message") } + +func TestSendMessageRejectsArchivedChat(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + replica := newTestServer(t, db, ps, uuid.New()) + + ctx := testutil.Context(t, testutil.WaitLong) + user, org, model := seedChatDependencies(ctx, t, db) + + chat, err := replica.CreateChat(ctx, chatd.CreateOptions{ + OwnerID: user.ID, + OrganizationID: org.ID, + Title: "send-archived", + ModelConfigID: model.ID, + InitialUserContent: []codersdk.ChatMessagePart{codersdk.ChatMessageText("hello")}, + }) + require.NoError(t, err) + + err = replica.ArchiveChat(ctx, chat) + require.NoError(t, err) + + _, err = replica.SendMessage(ctx, chatd.SendMessageOptions{ + ChatID: chat.ID, + Content: []codersdk.ChatMessagePart{codersdk.ChatMessageText("should fail")}, + BusyBehavior: chatd.SendMessageBusyBehaviorQueue, + }) + require.ErrorIs(t, err, chatd.ErrChatArchived) +} + +func TestEditMessageRejectsArchivedChat(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + replica := newTestServer(t, db, ps, uuid.New()) + + ctx := testutil.Context(t, testutil.WaitLong) + user, org, model := seedChatDependencies(ctx, t, db) + + chat, err := replica.CreateChat(ctx, chatd.CreateOptions{ + OwnerID: user.ID, + OrganizationID: org.ID, + Title: "edit-archived", + ModelConfigID: model.ID, + InitialUserContent: []codersdk.ChatMessagePart{codersdk.ChatMessageText("original")}, + }) + require.NoError(t, err) + + messages, err := db.GetChatMessagesByChatID(ctx, database.GetChatMessagesByChatIDParams{ + ChatID: chat.ID, + AfterID: 0, + }) + require.NoError(t, err) + require.Len(t, messages, 1) + + err = replica.ArchiveChat(ctx, chat) + require.NoError(t, err) + + _, err = replica.EditMessage(ctx, chatd.EditMessageOptions{ + ChatID: chat.ID, + EditedMessageID: messages[0].ID, + Content: []codersdk.ChatMessagePart{codersdk.ChatMessageText("edited")}, + }) + require.ErrorIs(t, err, chatd.ErrChatArchived) +} + +func TestPromoteQueuedRejectsArchivedChat(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + replica := newTestServer(t, db, ps, uuid.New()) + + ctx := testutil.Context(t, testutil.WaitLong) + user, org, model := seedChatDependencies(ctx, t, db) + + chat, err := replica.CreateChat(ctx, chatd.CreateOptions{ + OwnerID: user.ID, + OrganizationID: org.ID, + Title: "promote-archived", + ModelConfigID: model.ID, + InitialUserContent: []codersdk.ChatMessagePart{codersdk.ChatMessageText("hello")}, + }) + require.NoError(t, err) + + // Queue a message by setting the chat to running first. + chat, err = db.UpdateChatStatus(ctx, database.UpdateChatStatusParams{ + ID: chat.ID, + Status: database.ChatStatusRunning, + WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, + HeartbeatAt: sql.NullTime{Time: time.Now(), Valid: true}, + }) + require.NoError(t, err) + + queuedResult, err := replica.SendMessage(ctx, chatd.SendMessageOptions{ + ChatID: chat.ID, + Content: []codersdk.ChatMessagePart{codersdk.ChatMessageText("queued")}, + BusyBehavior: chatd.SendMessageBusyBehaviorQueue, + }) + require.NoError(t, err) + require.True(t, queuedResult.Queued) + + // Move back to waiting, then archive. + chat, err = db.UpdateChatStatus(ctx, database.UpdateChatStatusParams{ + ID: chat.ID, + Status: database.ChatStatusWaiting, + WorkerID: uuid.NullUUID{}, + StartedAt: sql.NullTime{}, + HeartbeatAt: sql.NullTime{}, + }) + require.NoError(t, err) + + err = replica.ArchiveChat(ctx, chat) + require.NoError(t, err) + + _, err = replica.PromoteQueued(ctx, chatd.PromoteQueuedOptions{ + ChatID: chat.ID, + QueuedMessageID: queuedResult.QueuedMessage.ID, + CreatedBy: user.ID, + }) + require.ErrorIs(t, err, chatd.ErrChatArchived) +} + +func TestSubmitToolResultsRejectsArchivedChat(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + replica := newTestServer(t, db, ps, uuid.New()) + + ctx := testutil.Context(t, testutil.WaitLong) + user, org, model := seedChatDependencies(ctx, t, db) + + chat, err := replica.CreateChat(ctx, chatd.CreateOptions{ + OwnerID: user.ID, + OrganizationID: org.ID, + Title: "submit-tool-archived", + ModelConfigID: model.ID, + InitialUserContent: []codersdk.ChatMessagePart{codersdk.ChatMessageText("hello")}, + }) + require.NoError(t, err) + + err = replica.ArchiveChat(ctx, chat) + require.NoError(t, err) + + // Set requires_action so the test exercises a realistic + // scenario where SubmitToolResults would be called. + _, err = db.UpdateChatStatus(ctx, database.UpdateChatStatusParams{ + ID: chat.ID, + Status: database.ChatStatusRequiresAction, + }) + require.NoError(t, err) + + err = replica.SubmitToolResults(ctx, chatd.SubmitToolResultsOptions{ + ChatID: chat.ID, + UserID: user.ID, + ModelConfigID: model.ID, + Results: []codersdk.ToolResult{{ + ToolCallID: "fake-tool-call-id", + Output: json.RawMessage(`{"result":"ignored"}`), + }}, + }) + require.ErrorIs(t, err, chatd.ErrChatArchived) +} + +func TestAcquireChatsSkipsArchivedPendingChat(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + _ = newTestServer(t, db, ps, uuid.New()) + + ctx := testutil.Context(t, testutil.WaitLong) + user, org, model := seedChatDependencies(ctx, t, db) + + archivedChat, err := db.InsertChat(ctx, database.InsertChatParams{ + OwnerID: user.ID, + OrganizationID: org.ID, + Title: "acquire-skip-archived", + LastModelConfigID: model.ID, + Status: database.ChatStatusWaiting, + ClientType: database.ChatClientTypeUi, + }) + require.NoError(t, err) + + // Archive the chat, then force it to pending. + _, err = db.ArchiveChatByID(ctx, archivedChat.ID) + require.NoError(t, err) + + _, err = db.UpdateChatStatus(ctx, database.UpdateChatStatusParams{ + ID: archivedChat.ID, + Status: database.ChatStatusPending, + }) + require.NoError(t, err) + + // Insert a second, non-archived pending chat so the result + // slice is non-empty and the assertion is not vacuously true. + activeChat, err := db.InsertChat(ctx, database.InsertChatParams{ + OwnerID: user.ID, + OrganizationID: org.ID, + Title: "acquire-active", + LastModelConfigID: model.ID, + Status: database.ChatStatusPending, + ClientType: database.ChatClientTypeUi, + }) + require.NoError(t, err) + + now := time.Now() + acquired, err := db.AcquireChats(ctx, database.AcquireChatsParams{ + WorkerID: uuid.New(), + StartedAt: now, + NumChats: 10, + }) + require.NoError(t, err) + require.Len(t, acquired, 1, "only the non-archived chat should be acquired") + require.Equal(t, activeChat.ID, acquired[0].ID) +}