diff --git a/internal/conversation/conversation.go b/internal/conversation/conversation.go index 3ce4171..bb82dde 100644 --- a/internal/conversation/conversation.go +++ b/internal/conversation/conversation.go @@ -651,6 +651,17 @@ func (inf *Inference) Run() (err error) { slog.String("id", saved.Id), ) + // Give an untitled conversation its title before the model + // runs, so a slow completion does not delay the title. A + // failure here costs the title, not the run: the next user + // message tries again. + if err := c.setTitle(ctx, *req.Message); err != nil { + log.WarnContext( + ctx, "failed to derive conversation title", + slog.Any("error", err), + ) + } + // Continue from the updated history. history = append(history, *req.Message) } @@ -1147,7 +1158,22 @@ func (c *Conversation) InsertMessage( return nil, ErrBusy } - return c.insertMessage(ctx, nil, msg, nil, nil) + saved, err := c.insertMessage(ctx, nil, msg, nil, nil) + if err != nil { + return nil, err + } + + // A user message can give an untitled conversation its title. + // A failure here costs the title, not the saved message. + if msg.Role == llm.RoleUser { + if err := c.setTitle(ctx, msg); err != nil { + c.log.WarnContext( + ctx, "failed to derive conversation title", + slog.Any("error", err), + ) + } + } + return saved, nil } // UpdateMessage replaces a message's model-visible content; its role may diff --git a/internal/conversation/title.go b/internal/conversation/title.go new file mode 100644 index 0000000..8b26871 --- /dev/null +++ b/internal/conversation/title.go @@ -0,0 +1,67 @@ +package conversation + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" + "unicode/utf8" + + "code.chimeric.al/chimerical/odidere/internal/db" + "code.chimeric.al/chimerical/odidere/internal/llm" +) + +// MaxTitleLength is the maximum length of a conversation title in +// Unicode code points. +const MaxTitleLength = 256 + +// deriveTitle returns a title from the first line of text that +// contains non-whitespace characters. It replaces runs of +// whitespace on that line with single spaces and truncates to +// MaxTitleLength runes. It returns "" if text has no +// non-whitespace characters. +func deriveTitle(text string) string { + for line := range strings.SplitSeq(text, "\n") { + title := strings.Join(strings.Fields(line), " ") + if title == "" { + continue + } + if utf8.RuneCountInString(title) <= MaxTitleLength { + return title + } + runes := []rune(title) + return string(runes[:MaxTitleLength]) + } + return "" +} + +// setTitle derives a title from msg and sets the conversation +// title if the conversation has no title yet. It does nothing if +// msg has no non-whitespace text or if the conversation already +// has a title. +func (c *Conversation) setTitle( + ctx context.Context, + msg llm.Message, +) error { + title := deriveTitle(msg.Text()) + if title == "" { + return nil + } + _, err := c.dbw.UpdateConversationTitleIfUnset( + ctx, + db.UpdateConversationTitleIfUnsetParams{ + Id: c.id, + Title: title, + }, + ) + // sql.ErrNoRows means the conversation already has a title (or + // does not exist). + if errors.Is(err, sql.ErrNoRows) { + return nil + } + if err != nil { + return fmt.Errorf("update conversation title: %w", err) + } + return nil +} diff --git a/internal/conversation/title_test.go b/internal/conversation/title_test.go new file mode 100644 index 0000000..732fc10 --- /dev/null +++ b/internal/conversation/title_test.go @@ -0,0 +1,202 @@ +package conversation + +import ( + "fmt" + "net/http" + "strings" + "testing" + + "code.chimeric.al/chimerical/odidere/internal/db" + "code.chimeric.al/chimerical/odidere/internal/llm" +) + +func TestDeriveTitle(t *testing.T) { + tests := []struct { + name string + text string + want string + }{ + { + name: "single line", + text: "Check the cameras", + want: "Check the cameras", + }, + { + name: "blank lines come before the first line", + text: "\n \n\t\nCheck the cameras\nSecond line", + want: "Check the cameras", + }, + { + name: "spaces inside the line collapse to one", + text: "Can you\tcheck the cameras? ", + want: "Can you check the cameras?", + }, + { + name: "leading indentation is trimmed", + text: " def main():", + want: "def main():", + }, + { + name: "a line of MaxTitleLength characters stays whole", + text: strings.Repeat("x", MaxTitleLength), + want: strings.Repeat("x", MaxTitleLength), + }, + { + name: "a longer line cuts to MaxTitleLength characters", + text: strings.Repeat("x", 300), + want: strings.Repeat("x", MaxTitleLength), + }, + { + name: "the cut counts characters, not bytes", + text: strings.Repeat("文", 300), + want: strings.Repeat("文", MaxTitleLength), + }, + { + name: "empty text", + text: "", + want: "", + }, + { + name: "spaces only", + text: " \n\t \n", + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := deriveTitle(tt.text); got != tt.want { + t.Fatalf("deriveTitle(%q) = %q, want %q", tt.text, got, tt.want) + } + }) + } +} + +func title(t *testing.T, c *Conversation) *string { + t.Helper() + conv, err := c.dbr.SelectConversationId(t.Context(), c.id) + if err != nil { + t.Fatalf("SelectConversationId: %v", err) + } + return conv.Title +} + +func TestInsertMessageTitlesConversation(t *testing.T) { + ctx := t.Context() + + // The first user message sets the title from its first non-empty + // line. + c := newTestConversation(t, nil) + if _, err := c.InsertMessage( + ctx, *userMessage(" Check the cameras\nplease"), + ); err != nil { + t.Fatalf("InsertMessage: %v", err) + } + if got := title(t, c); got == nil || *got != "Check the cameras" { + t.Fatalf("Title = %v, want Check the cameras", got) + } + + // A later user message does not overwrite an existing title. + if _, err := c.InsertMessage( + ctx, *userMessage("A different subject"), + ); err != nil { + t.Fatalf("InsertMessage: %v", err) + } + if got := title(t, c); got == nil || *got != "Check the cameras" { + t.Fatalf("Title = %v, want Check the cameras", got) + } + + // Non-user messages do not set the title. + c2 := newTestConversation(t, nil) + assistant := llm.Message{ + Role: llm.RoleAssistant, + ContentParts: []llm.ContentPart{{ + Type: llm.ContentTypeText, + Text: "Hello there", + }}, + } + if _, err := c2.InsertMessage(ctx, assistant); err != nil { + t.Fatalf("InsertMessage: %v", err) + } + if got := title(t, c2); got != nil { + t.Fatalf("Title = %v, want nil", got) + } + + // A title set before the first user message is kept. + c3 := newTestConversation(t, nil) + if _, err := c3.dbw.UpdateConversationTitle( + ctx, + db.UpdateConversationTitleParams{ + Id: c3.id, + Title: "Nightly report", + }, + ); err != nil { + t.Fatalf("UpdateConversationTitle: %v", err) + } + if _, err := c3.InsertMessage( + ctx, *userMessage("A different subject"), + ); err != nil { + t.Fatalf("InsertMessage: %v", err) + } + if got := title(t, c3); got == nil || *got != "Nightly report" { + t.Fatalf("Title = %v, want Nightly report", got) + } +} + +func TestInferenceTitlesConversation(t *testing.T) { + var ( + ctx = t.Context() + client = newTestModelClient( + t, + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{ + "choices":[{ + "message":{"role":"assistant","content":"ok"}, + "finish_reason":"stop" + }] + }`) + }, + ) + ) + + // Inference with a new user message sets an unset title. + c := newTestConversation(t, nil) + if err := infer(ctx, c, client, InferenceRequest{ + Message: userMessage("Turn off the porch light"), + }); err != nil { + t.Fatalf("infer: %v", err) + } + if got := title(t, c); got == nil || *got != "Turn off the porch light" { + t.Fatalf("Title = %v, want Turn off the porch light", got) + } + + // Later inference does not overwrite the title. + if err := infer(ctx, c, client, InferenceRequest{ + Message: userMessage("A different subject"), + }); err != nil { + t.Fatalf("infer: %v", err) + } + if got := title(t, c); got == nil || *got != "Turn off the porch light" { + t.Fatalf("Title = %v, want Turn off the porch light", got) + } + + // Inference does not overwrite a title that was already set. + c2 := newTestConversation(t, nil) + if _, err := c2.dbw.UpdateConversationTitle( + ctx, + db.UpdateConversationTitleParams{ + Id: c2.id, + Title: "Nightly report", + }, + ); err != nil { + t.Fatalf("UpdateConversationTitle: %v", err) + } + if err := infer(ctx, c2, client, InferenceRequest{ + Message: userMessage("A different subject"), + }); err != nil { + t.Fatalf("infer: %v", err) + } + if got := title(t, c2); got == nil || *got != "Nightly report" { + t.Fatalf("Title = %v, want Nightly report", got) + } +} diff --git a/internal/db/queries.go b/internal/db/queries.go index ca35a86..57ee5e9 100644 --- a/internal/db/queries.go +++ b/internal/db/queries.go @@ -288,7 +288,7 @@ func (q *Queries) SelectConversationsUserId(ctx context.Context, userId string) const updateConversationTitle = ` UPDATE conversations -SET title = ? +SET title = ?, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ', 'now') WHERE id = ? RETURNING created_at, id, title, updated_at, user_id ` @@ -300,15 +300,9 @@ type UpdateConversationTitleParams struct { Title string } -// UpdateConversationTitle sets the title of one conversation and -// returns the updated row. -// -// The update keeps updated_at. That timestamp is the -// conversation's synchronization token for its message set, and a -// title is not a message: a rename must not move the conversation -// in the picker's ordering, which sorts by updated_at. -// -// A conversation that is missing returns sql.ErrNoRows. +// UpdateConversationTitle sets the title for a conversation, bumps +// updated_at, and returns the updated row. It returns sql.ErrNoRows +// if no conversation has that id. func (q *Queries) UpdateConversationTitle( ctx context.Context, arg UpdateConversationTitleParams, @@ -344,6 +338,61 @@ func (q *Queries) UpdateConversationTitle( return &c, nil } +const updateConversationTitleIfUnset = ` +UPDATE conversations +SET title = ?, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ', 'now') +WHERE id = ? AND title IS NULL +RETURNING created_at, id, title, updated_at, user_id +` + +// UpdateConversationTitleIfUnsetParams holds arguments for +// UpdateConversationTitleIfUnset. +type UpdateConversationTitleIfUnsetParams struct { + Id string + Title string +} + +// UpdateConversationTitleIfUnset sets the first title for a +// conversation when title is NULL, bumps updated_at, and returns +// the updated row. It prevents a caller from overwriting a title +// set by a user or a scheduled job. It returns sql.ErrNoRows if +// the conversation does not exist or already has a title; callers +// treat both cases as no-op. +func (q *Queries) UpdateConversationTitleIfUnset( + ctx context.Context, + arg UpdateConversationTitleIfUnsetParams, +) (*Conversation, error) { + row := q.db.QueryRowContext( + ctx, + updateConversationTitleIfUnset, + arg.Title, + arg.Id, + ) + + var ( + c Conversation + createdAt string + updatedAt string + err error + ) + if err := row.Scan( + &createdAt, + &c.Id, + &c.Title, + &updatedAt, + &c.UserId, + ); err != nil { + return nil, fmt.Errorf("UpdateConversationTitleIfUnset: %w", err) + } + if c.CreatedAt, err = time.Parse(time.RFC3339, createdAt); err != nil { + return nil, fmt.Errorf("UpdateConversationTitleIfUnset: parse created_at: %w", err) + } + if c.UpdatedAt, err = time.Parse(time.RFC3339, updatedAt); err != nil { + return nil, fmt.Errorf("UpdateConversationTitleIfUnset: parse updated_at: %w", err) + } + return &c, nil +} + const deleteConversationId = ` DELETE FROM conversations WHERE id = ? diff --git a/internal/db/queries_test.go b/internal/db/queries_test.go index cf0d0cb..4556ecf 100644 --- a/internal/db/queries_test.go +++ b/internal/db/queries_test.go @@ -136,20 +136,16 @@ func testConversation(t *testing.T, ctx context.Context, db *SQLite) { t.Fatalf("expected 1 conversation, got %d", len(conversations)) } - // Rename the conversation. Insert a message first: the insert - // trigger moves updated_at, so the rename can show that it - // keeps the timestamp. - if _, err := qw.InsertMessage(ctx, InsertMessageParams{ - ConversationId: c.Id, - Sequence: 1, - Role: MessageRoleUser, - Content: jsontext.Value(`{"role":"user","content":"Hello"}`), - }); err != nil { - t.Fatalf("InsertMessage: %v", err) - } - before, err := qr.SelectConversationId(ctx, c.Id) - if err != nil { - t.Fatalf("SelectConversationId: %v", err) + // Rename the conversation. The rename must refresh updated_at: + // plant a known-old timestamp, then check that the statement + // moved it. + const staleUpdatedAt = "2020-01-01T00:00:00Z" + if _, err := db.Writer.ExecContext( + ctx, + "UPDATE conversations SET updated_at = ? WHERE id = ?", + staleUpdatedAt, c.Id, + ); err != nil { + t.Fatalf("plant stale updated_at: %v", err) } renamed, err := qw.UpdateConversationTitle(ctx, UpdateConversationTitleParams{ Id: c.Id, @@ -161,10 +157,14 @@ func testConversation(t *testing.T, ctx context.Context, db *SQLite) { if renamed.Title == nil || *renamed.Title != "renamed" { t.Fatalf("Title = %v, want renamed", renamed.Title) } - if !renamed.UpdatedAt.Equal(before.UpdatedAt) { + stale, err := time.Parse(time.RFC3339, staleUpdatedAt) + if err != nil { + t.Fatalf("parse staleUpdatedAt: %v", err) + } + if !renamed.UpdatedAt.After(stale) { t.Fatalf( - "UpdatedAt = %v, want %v: a rename must not move updated_at", - renamed.UpdatedAt, before.UpdatedAt, + "UpdatedAt = %v, want a time after %v", + renamed.UpdatedAt, stale, ) } @@ -175,6 +175,64 @@ func testConversation(t *testing.T, ctx context.Context, db *SQLite) { }); !errors.Is(err, sql.ErrNoRows) { t.Fatalf("UpdateConversationTitle(missing) = %v, want sql.ErrNoRows", err) } + + // The guarded update sets the title of an untitled + // conversation, and does not touch one that is titled. It must + // refresh updated_at: plant the stale timestamp, then check + // that the statement moved it. + blank, err := qw.InsertConversation(ctx, InsertConversationParams{ + UserId: u.Id, + }) + if err != nil { + t.Fatalf("InsertConversation: %v", err) + } + if _, err := db.Writer.ExecContext( + ctx, + "UPDATE conversations SET updated_at = ? WHERE id = ?", + staleUpdatedAt, blank.Id, + ); err != nil { + t.Fatalf("plant stale updated_at: %v", err) + } + titled, err := qw.UpdateConversationTitleIfUnset( + ctx, + UpdateConversationTitleIfUnsetParams{ + Id: blank.Id, + Title: "derived", + }, + ) + if err != nil { + t.Fatalf("UpdateConversationTitleIfUnset: %v", err) + } + if titled.Title == nil || *titled.Title != "derived" { + t.Fatalf("Title = %v, want derived", titled.Title) + } + if !titled.UpdatedAt.After(stale) { + t.Fatalf( + "UpdatedAt = %v, want a time after %v", + titled.UpdatedAt, stale, + ) + } + for _, tt := range []struct { + name string + id string + }{ + {name: "titled twice", id: blank.Id}, + {name: "renamed conversation", id: c.Id}, + {name: "missing conversation", id: "missing"}, + } { + if _, err := qw.UpdateConversationTitleIfUnset( + ctx, + UpdateConversationTitleIfUnsetParams{ + Id: tt.id, + Title: "second", + }, + ); !errors.Is(err, sql.ErrNoRows) { + t.Fatalf( + "UpdateConversationTitleIfUnset(%s) = %v, want sql.ErrNoRows", + tt.name, err, + ) + } + } } func testMessaging(t *testing.T, ctx context.Context, db *SQLite) { diff --git a/internal/service/conversations.go b/internal/service/conversations.go index 7baac13..245d227 100644 --- a/internal/service/conversations.go +++ b/internal/service/conversations.go @@ -9,6 +9,7 @@ import ( "strings" "unicode/utf8" + "code.chimeric.al/chimerical/odidere/internal/conversation" "code.chimeric.al/chimerical/odidere/internal/db" ) @@ -129,17 +130,12 @@ func (svc *Service) getConversationHandler( } } -// maxTitleLength is the most characters, counted as Unicode code -// points, that a conversation title may hold. It keeps a title -// short enough for the picker and small in storage. -const maxTitleLength = 256 - // updateConversation renames one conversation. The handler trims // the title, then rejects an empty title or a title longer than -// maxTitleLength with 400. A conversation that does not exist, or -// belongs to another user, returns 404. The rename is allowed while -// inference runs: a title is not a message, and a concurrent -// inference does not read or write it. +// conversation.MaxTitleLength with 400. A conversation that does +// not exist, or belongs to another user, returns 404. The rename +// is allowed while inference runs: a title is not a message, and a +// concurrent inference does not read or write it. func (svc *Service) updateConversation(w http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() @@ -165,7 +161,7 @@ func (svc *Service) updateConversation(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid request", http.StatusBadRequest) return } - if utf8.RuneCountInString(req.Title) > maxTitleLength { + if utf8.RuneCountInString(req.Title) > conversation.MaxTitleLength { http.Error(w, "invalid request", http.StatusBadRequest) return } diff --git a/internal/service/static/js/ui.js b/internal/service/static/js/ui.js index daefbed..857c359 100644 --- a/internal/service/static/js/ui.js +++ b/internal/service/static/js/ui.js @@ -39,9 +39,9 @@ export function extractText(content) { .join('\n'); } -// Conversations created from this UI have no title. Their label -// falls back to the creation date, so the picker stays readable. -// Both dates render in the browser locale. +// CONVERSATION_DATE formats conversation timestamps in the browser +// locale. Until the server sets a title from the first user message, an +// untitled conversation uses its formatted creation date as its label. const CONVERSATION_DATE = new Intl.DateTimeFormat(undefined, { dateStyle: 'medium', timeStyle: 'short',