From c8cb68b434cb8ade9663b3f576189b98e231d41f Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 3 Nov 2026 11:47:20 +0000 Subject: [PATCH] refactor(studio): always read a thread when the bot is mentioned in it The thread_context binding setting is removed. When the kind declares the history ability and the bot is mentioned in a thread, the host reads the thread once for a new chat, as before. The other guards stay: the ability check, the bound, the untrusted block, and a failure never blocks the answer. A binding stored by an earlier build still loads. The JSON store ignores the unknown field. The GORM row no longer maps the thread_context_off column; AutoMigrate does not drop it and it accepts NULL, so reads and writes keep working. No data is migrated or removed. Assisted-by: Claude Code:claude-sonnet-5-5 Signed-off-by: Ettore Di Giacinto --- core/http/endpoints/studio/connectors.go | 4 ++-- core/http/routes/studio_connectors_test.go | 12 ++++-------- core/schema/chats.go | 10 ++-------- .../studio/connectors/connectorstest/suite.go | 10 ---------- core/services/studio/connectors/gorm.go | 10 +++++----- .../studio/connectors/gorm_sqlite_test.go | 16 ++++++++++++++++ core/services/studio/connectors/json_test.go | 16 ++++++++++++++++ core/services/studio/connectors/types.go | 8 -------- core/services/studio/connectors_slack_test.go | 13 +++++-------- core/services/studio/connectors_thread_test.go | 11 ++++++----- core/services/studio/connectorsvc.go | 7 ------- core/services/studio/connectorthread.go | 10 +++++----- 12 files changed, 61 insertions(+), 66 deletions(-) diff --git a/core/http/endpoints/studio/connectors.go b/core/http/endpoints/studio/connectors.go index b6f3c93262..afc9db0c1b 100644 --- a/core/http/endpoints/studio/connectors.go +++ b/core/http/endpoints/studio/connectors.go @@ -24,7 +24,7 @@ func connectorOut(i studiosvc.ConnectorInfo) schema.ChatsConnector { } return schema.ChatsConnector{ID: i.ID, Name: i.Name, Kind: i.Kind, Addon: i.AddonID, Active: i.Active, Reason: i.Reason, AgentID: i.AgentID, Enabled: i.Enabled, AcceptAny: i.AcceptAny, Allow: allow, NotifyChat: i.NotifyChat, HasToken: i.HasToken, Created: i.Created, - ReplyMode: i.ReplyMode(), Conversation: i.ConversationMode(), WindowMinutes: i.WindowMinutes, SendFiles: i.SendsFiles(), ThreadContext: i.ReadsThread(), Options: i.Options, + ReplyMode: i.ReplyMode(), Conversation: i.ConversationMode(), WindowMinutes: i.WindowMinutes, SendFiles: i.SendsFiles(), Options: i.Options, Files: filesOut(i.Files), FileReceiveMaxBytes: i.FileReceiveMax, FileSendMaxBytes: i.FileSendMax} } @@ -46,7 +46,7 @@ func failAcceptAny(c echo.Context) error { func connectorIn(req schema.ChatsConnectorWrite) studiosvc.ConnectorInput { return studiosvc.ConnectorInput{Kind: req.Kind, Name: req.Name, Token: req.Token, AgentID: req.AgentID, Enabled: req.Enabled, AcceptAny: req.AcceptAny, Allow: req.Allow, NotifyChat: req.NotifyChat, - Reply: req.ReplyMode, Conversation: req.Conversation, WindowMinutes: req.WindowMinutes, SendFiles: req.SendFiles, ThreadContext: req.ThreadContext, Options: req.Options, + Reply: req.ReplyMode, Conversation: req.Conversation, WindowMinutes: req.WindowMinutes, SendFiles: req.SendFiles, Options: req.Options, FileReceiveMax: req.FileReceiveMaxBytes, FileSendMax: req.FileSendMaxBytes} } diff --git a/core/http/routes/studio_connectors_test.go b/core/http/routes/studio_connectors_test.go index c87d3f9143..32f16d8c6e 100644 --- a/core/http/routes/studio_connectors_test.go +++ b/core/http/routes/studio_connectors_test.go @@ -195,17 +195,13 @@ var _ = Describe("Chats connectors endpoints", func() { Expect(s.do("alice", http.MethodPost, "/api/chats/connectors/"+id+"/senders/99/allow", nil).Code).To(Equal(http.StatusBadRequest)) }) - It("stores the thread context switch: on for a new binding, kept by an update that omits it", func() { + It("has no thread_context field: the answer omits it and a request that sends it is not stored", func() { ag := agent("alice", "Context", "test-model") - rec := create("alice", map[string]any{"name": "Context bot", "token": specToken, "agent_id": ag}) + rec := create("alice", map[string]any{"name": "Context bot", "token": specToken, "agent_id": ag, "thread_context": false}) Expect(rec.Code).To(Equal(http.StatusCreated), rec.Body.String()) - Expect(decodeJSON(rec)).To(HaveKeyWithValue("thread_context", true), "the default") + Expect(decodeJSON(rec)).NotTo(HaveKey("thread_context")) id := decodeJSON(rec)["id"].(string) - rec = s.do("alice", http.MethodPut, "/api/chats/connectors/"+id, map[string]any{"name": "Context bot", "agent_id": ag, "thread_context": false}) - Expect(rec.Code).To(Equal(http.StatusOK), rec.Body.String()) - Expect(decodeJSON(rec)).To(HaveKeyWithValue("thread_context", false)) - rec = s.do("alice", http.MethodPut, "/api/chats/connectors/"+id, map[string]any{"name": "Context bot", "agent_id": ag}) - Expect(decodeJSON(rec)).To(HaveKeyWithValue("thread_context", false), "an update without the field keeps it") + Expect(decodeJSON(s.do("alice", http.MethodGet, "/api/chats/connectors/"+id, nil))).NotTo(HaveKey("thread_context")) }) It("refuses an option that the kind does not declare", func() { diff --git a/core/schema/chats.go b/core/schema/chats.go index 8e5dfc0864..b983c3fe06 100644 --- a/core/schema/chats.go +++ b/core/schema/chats.go @@ -1177,10 +1177,6 @@ type ChatsConnector struct { // SendFiles sends the files that a turn makes to the sender, after the // answer. SendFiles bool `json:"send_files"` - // ThreadContext lets the bot read the earlier messages of the thread in - // which it is mentioned. It has an effect only on a platform that can - // give them. - ThreadContext bool `json:"thread_context"` // Options are the choices made for the options of the kind, by option id. // An option that is absent has its default (see ChatsConnectorKind). Options map[string]string `json:"options,omitempty"` @@ -1238,7 +1234,8 @@ type ChatsConnectorKind struct { // Page is the add-on page that adds and edits bindings of this kind. Page string `json:"page,omitempty"` // ThreadHistory is true when the platform can give the earlier messages - // of a thread, so a binding of this kind has the thread_context option. + // of a thread. The host then reads them whenever the bot is mentioned in a + // thread. ThreadHistory bool `json:"thread_history,omitempty"` // Options are the choices the add-on lets each binding of this kind make. Options []ChatsConnectorOption `json:"options,omitempty"` @@ -1294,9 +1291,6 @@ type ChatsConnectorWrite struct { // SendFiles turns the sending of a turn's files on or off. Absent means // on for a new binding and unchanged for an update. SendFiles *bool `json:"send_files,omitempty"` - // ThreadContext turns the reading of the earlier messages of a thread on - // or off. Absent means on for a new binding and unchanged for an update. - ThreadContext *bool `json:"thread_context,omitempty"` // Options are the choices for the options of the kind, by option id. Absent // means the defaults for a new binding and unchanged for an update. Options map[string]string `json:"options,omitempty"` diff --git a/core/services/studio/connectors/connectorstest/suite.go b/core/services/studio/connectors/connectorstest/suite.go index 5c65dcf38b..0ba7631170 100644 --- a/core/services/studio/connectors/connectorstest/suite.go +++ b/core/services/studio/connectors/connectorstest/suite.go @@ -70,7 +70,6 @@ func Run(newStore NewStore) { got, _ = s.GetBinding(ctx, "modes") Expect(got.ReplyMode()).To(Equal(connectors.ReplyProgress)) Expect(got.SendsFiles()).To(BeTrue()) - Expect(got.ReadsThread()).To(BeTrue(), "a binding that says nothing reads the thread") }) It("keeps the options of the kind", func() { s := newStore() @@ -85,15 +84,6 @@ func Run(newStore NewStore) { Expect(again.Options["followThreads"]).To(Equal("never")) Expect(connectors.Validate(connectors.Binding{Name: "n", Kind: "k", AgentID: "a", Options: map[string]string{"": "x"}})).NotTo(Succeed()) }) - It("keeps the thread context switch", func() { - s := newStore() - b := binding("ctx", "u1", "Context") - b.ThreadContextOff = true - Expect(s.PutBinding(ctx, b)).To(Succeed()) - got, err := s.GetBinding(ctx, "ctx") - Expect(err).NotTo(HaveOccurred()) - Expect(got.ReadsThread()).To(BeFalse()) - }) It("keeps the file limits an owner set for a binding", func() { s := newStore() b := binding("limits", "u1", "Limits") diff --git a/core/services/studio/connectors/gorm.go b/core/services/studio/connectors/gorm.go index 7f573d5777..abb94650a3 100644 --- a/core/services/studio/connectors/gorm.go +++ b/core/services/studio/connectors/gorm.go @@ -28,9 +28,9 @@ type bindingRow struct { Conversation string `gorm:"size:16"` WindowMinutes int FilesOff bool - // ThreadContextOff is added by AutoMigrate; a row stored before it reads - // the thread. - ThreadContextOff bool + // A thread_context_off column that an earlier build added stays in the + // table, unread and unwritten: AutoMigrate never drops a column, and the + // column accepts NULL, so an insert without it works. // Options is JSON, added by AutoMigrate. Options string // The columns of the file limits are added by AutoMigrate; a row stored @@ -93,7 +93,7 @@ func NewGORM(db *gorm.DB) (Store, error) { func (r bindingRow) binding() (Binding, error) { b := Binding{ID: r.ID, Owner: r.Owner, Kind: r.Kind, AddonID: r.AddonID, Name: r.Name, AgentID: r.AgentID, Enabled: r.Enabled, AcceptAny: r.AcceptAny, - NotifyChat: r.NotifyChat, Reply: r.Reply, Conversation: r.Conversation, WindowMinutes: r.WindowMinutes, FilesOff: r.FilesOff, ThreadContextOff: r.ThreadContextOff, + NotifyChat: r.NotifyChat, Reply: r.Reply, Conversation: r.Conversation, WindowMinutes: r.WindowMinutes, FilesOff: r.FilesOff, FileReceiveMax: r.FileReceiveMax, FileSendMax: r.FileSendMax, Token: append([]byte(nil), r.Token...), Created: r.Created.UTC(), Updated: r.Updated.UTC()} if r.Allow != "" { @@ -122,7 +122,7 @@ func (s *gormStore) PutBinding(ctx context.Context, b Binding) error { } return s.db.WithContext(ctx).Save(&bindingRow{ID: b.ID, Owner: b.Owner, Kind: b.Kind, AddonID: b.AddonID, Name: b.Name, AgentID: b.AgentID, Enabled: b.Enabled, AcceptAny: b.AcceptAny, Allow: allow, NotifyChat: b.NotifyChat, Reply: b.Reply, Conversation: b.Conversation, WindowMinutes: b.WindowMinutes, - FilesOff: b.FilesOff, ThreadContextOff: b.ThreadContextOff, Options: options, FileReceiveMax: b.FileReceiveMax, FileSendMax: b.FileSendMax, + FilesOff: b.FilesOff, Options: options, FileReceiveMax: b.FileReceiveMax, FileSendMax: b.FileSendMax, Token: b.Token, Created: b.Created, Updated: b.Updated}).Error } diff --git a/core/services/studio/connectors/gorm_sqlite_test.go b/core/services/studio/connectors/gorm_sqlite_test.go index 4628aea4c5..2912bfaebd 100644 --- a/core/services/studio/connectors/gorm_sqlite_test.go +++ b/core/services/studio/connectors/gorm_sqlite_test.go @@ -1,6 +1,7 @@ package connectors_test import ( + "context" "path/filepath" . "github.com/onsi/ginkgo/v2" @@ -32,6 +33,21 @@ var _ = Describe("GORM on SQLite", func() { }) }) + It("loads and rewrites a row that an earlier build stored with a thread_context_off column", func() { + path := filepath.Join(GinkgoT().TempDir(), "c.db") + first, _ := connectors.NewGORM(openSQLite(path)) + connectorstest.Seed(first) + db := openSQLite(path) + Expect(db.Exec("ALTER TABLE studio_connectors ADD COLUMN thread_context_off numeric").Error).To(Succeed()) + Expect(db.Exec("UPDATE studio_connectors SET thread_context_off = 1").Error).To(Succeed()) + second, err := connectors.NewGORM(db) + Expect(err).NotTo(HaveOccurred()) + connectorstest.CheckSeed(second) + b, err := second.GetBinding(context.Background(), "seed") + Expect(err).NotTo(HaveOccurred()) + Expect(second.PutBinding(context.Background(), b)).To(Succeed(), "an insert or update works with the old column left in place") + }) + It("survives a reopen", func() { path := filepath.Join(GinkgoT().TempDir(), "c.db") first, _ := connectors.NewGORM(openSQLite(path)) diff --git a/core/services/studio/connectors/json_test.go b/core/services/studio/connectors/json_test.go index ce85c5eede..387bc63af7 100644 --- a/core/services/studio/connectors/json_test.go +++ b/core/services/studio/connectors/json_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -43,6 +44,21 @@ var _ = Describe("the JSON store", func() { } }) + It("loads a file that an earlier build wrote with the ThreadContextOff field", func() { + dir := GinkgoT().TempDir() + first, _ := connectors.NewJSON(dir) + connectorstest.Seed(first) + p := filepath.Join(dir, "connectors.json") + raw, err := os.ReadFile(p) + Expect(err).NotTo(HaveOccurred()) + patched := strings.Replace(string(raw), `"Name":`, `"ThreadContextOff":true,"Name":`, -1) + Expect(patched).NotTo(Equal(string(raw))) + Expect(os.WriteFile(p, []byte(patched), 0o600)).To(Succeed()) + second, err := connectors.NewJSON(dir) + Expect(err).NotTo(HaveOccurred()) + connectorstest.CheckSeed(second) + }) + It("refuses a key file that others can read", func() { dir := GinkgoT().TempDir() first, _ := connectors.NewJSON(dir) diff --git a/core/services/studio/connectors/types.go b/core/services/studio/connectors/types.go index c2d7e5f819..7c98993690 100644 --- a/core/services/studio/connectors/types.go +++ b/core/services/studio/connectors/types.go @@ -110,10 +110,6 @@ type Binding struct { // zero value sends them, so a binding stored before the option sends // them too. FilesOff bool - // ThreadContextOff keeps the host from reading the earlier messages of - // a thread when the bot is mentioned in it. The zero value reads them, - // on a platform whose kind declares the ability. - ThreadContextOff bool // Options are the choices the owner made for the options that the kind of // the binding declares (contract.ConnectorKind.Options), by option ID. An // option that is not here has its default. @@ -251,10 +247,6 @@ func (b Binding) ConversationMode() string { // SendsFiles tells whether the files a turn makes go to the sender. func (b Binding) SendsFiles() bool { return !b.FilesOff } -// ReadsThread tells whether the earlier messages of a thread are read when -// the bot is mentioned in it. -func (b Binding) ReadsThread() bool { return !b.ThreadContextOff } - // Allows tells whether a sender is on the allowlist. A match is by ID or, for an // @name entry, by username, ignoring case. func (b Binding) Allows(senderID, username string) bool { diff --git a/core/services/studio/connectors_slack_test.go b/core/services/studio/connectors_slack_test.go index 076a61b5cc..811a0debe2 100644 --- a/core/services/studio/connectors_slack_test.go +++ b/core/services/studio/connectors_slack_test.go @@ -148,15 +148,12 @@ var _ = Describe("connectors slack", func() { waitFor("the answer to the reply", func() bool { return len(r2.answers()) == 2 }) }) - It("slack mention outside a thread, or with the option off, reads nothing", func() { - off := false - r, _ := newSlackRigWith(connSay("ok"), studio.ConnectorInput{Allow: []string{"C1"}, ThreadContext: &off}) - r.slack.AddThreadMessage("C1", "100.000001", "100.000001", map[string]any{"user": "U1", "text": "earlier"}) - r.slack.Push(slackApp, slacktest.Mention("C1", "U1", "UBOT", "inside", "100.000001")) - waitFor("the answer", func() bool { return len(r.answers()) == 1 }) + It("slack mention outside a thread gets no earlier-messages block", func() { + r, _ := newSlackRigWith(connSay("ok"), studio.ConnectorInput{Allow: []string{"C1"}}) r.slack.Push(slackApp, slacktest.Mention("C1", "U1", "UBOT", "top level", "")) - waitFor("the second answer", func() bool { return len(r.answers()) == 2 }) - Expect(r.slack.Calls("conversations.replies")).To(BeZero()) + waitFor("the answer", func() bool { return len(r.answers()) == 1 }) + // The add-on makes the mention the root of its own thread, so the host may + // ask; the thread holds nothing but the message, so there is no block. chats, _ := r.svc.List(context.Background(), alice, 10) for _, c := range chats { for _, m := range r.h.Sent(c.ID) { diff --git a/core/services/studio/connectors_thread_test.go b/core/services/studio/connectors_thread_test.go index 226fb696a0..aa7d4ce48d 100644 --- a/core/services/studio/connectors_thread_test.go +++ b/core/services/studio/connectors_thread_test.go @@ -131,20 +131,21 @@ var _ = Describe("the earlier messages of a thread", func() { Expect(fake.calls()).To(HaveLen(1), "one history call per new chat") }) - It("asks nothing for a message outside a thread, or when the binding turns the reading off", func() { + It("asks nothing for a message outside a thread", func() { setup(&contract.ConnectorHistory{Thread: true}, studio.ConnectorInput{}) m := mention("m5", "") m.Direct = true ingest(m) answered(1) Expect(fake.calls()).To(BeEmpty()) + }) - off := false - setup(&contract.ConnectorHistory{Thread: true}, studio.ConnectorInput{ThreadContext: &off}) + It("always reads the thread when the kind declares the ability: no binding input changes it", func() { + setup(&contract.ConnectorHistory{Thread: true}, studio.ConnectorInput{Name: "Any", Enabled: true}) ingest(mention("m5", "T1")) answered(1) - Expect(fake.calls()).To(BeEmpty()) - Expect(r.h.Sent(chat())[0]).To(Equal("@bot can you retry?")) + Expect(fake.calls()).To(HaveLen(1)) + Expect(r.h.Sent(chat())[0]).To(ContainSubstring("Earlier")) }) It("never calls a kind that declares no history", func() { diff --git a/core/services/studio/connectorsvc.go b/core/services/studio/connectorsvc.go index a9680607ce..e90b8c6eb5 100644 --- a/core/services/studio/connectorsvc.go +++ b/core/services/studio/connectorsvc.go @@ -38,10 +38,6 @@ type ConnectorInput struct { // SendFiles sends the files a turn makes to the sender. Nil means on for // a new binding and unchanged for an update. SendFiles *bool - // ThreadContext lets the bot read the earlier messages of the thread in - // which it is mentioned. Nil means on for a new binding and unchanged for - // an update. - ThreadContext *bool // Options are the choices for the options of the kind. Nil means the // defaults for a new binding and unchanged for an update. Options map[string]string @@ -64,9 +60,6 @@ func (in ConnectorInput) apply(b *connectors.Binding) { if in.Options != nil { b.Options = maps.Clone(in.Options) } - if in.ThreadContext != nil { - b.ThreadContextOff = !*in.ThreadContext - } if in.FileReceiveMax != nil { b.FileReceiveMax = *in.FileReceiveMax } diff --git a/core/services/studio/connectorthread.go b/core/services/studio/connectorthread.go index e6826f7175..3cfff4f955 100644 --- a/core/services/studio/connectorthread.go +++ b/core/services/studio/connectorthread.go @@ -26,12 +26,12 @@ const ( // threadDirective reads the earlier messages of the thread of m from the // platform and returns the block the first turn of the new chat gets, or "". -// It asks only when the message is in a thread, the binding reads threads and -// the add-on of the kind declares the ability. A failure never blocks the -// answer: it is logged and the turn goes on without context. The caller calls -// it once, when the chat of the thread has just been made. +// It asks only when the message is in a thread and the add-on of the kind +// declares the ability; no binding setting turns it off. A failure never +// blocks the answer: it is logged and the turn goes on without context. The +// caller calls it once, when the chat of the thread has just been made. func (c *Connectors) threadDirective(ctx context.Context, b connectors.Binding, token string, m contract.ConnectorMessage) string { - if m.Thread == "" || !b.ReadsThread() { + if m.Thread == "" { return "" } kinds, err := c.o.Transport.Kinds(ctx)