mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd): use pasted-text attachments as chat title input (#27067)
Closes https://linear.app/codercom/issue/CODAGT-268 ## Problem The chat UI collapses large pastes (>=10 lines or >=1000 chars) into a synthetic `pasted-text-*.txt` attachment. A chat created with only such an attachment had no title input anywhere: the create path derived `titleSource` only from text and file-reference parts (so the chat was named "New Chat"), async auto-titling extracted text the same way and silently skipped generation, and the manual propose/regenerate paths returned an empty title for the same reason. The regular prompt path already inlines these files for the model; only the title paths were blind. ## Fix Add a single title-input derivation in `chatprompt` and use it everywhere: - `chatprompt.TitleText` joins text and file-reference parts (unchanged formatting), and falls back to synthetic pasted-text attachment content (truncated to a 16 KiB title budget) when they yield nothing. - `chatprompt.SyntheticPasteFileIDs` identifies paste attachments; `chatprompt.FallbackTitle` consolidates the previously duplicated `chatTitleFromMessage` / `fallbackChatTitle`. - Chat creation captures paste blob references while validating file parts (the file row was already loaded there) and derives `titleSource` via `TitleText`. Only the create path derives titles; message send and edit reuse the same validation without copying any blob data. - `GenerateChatTitleAsync` and the manual propose/regenerate paths resolve paste content via `titlePasteText`, which only queries when a visible user message has no other title text, so chats with typed text never incur a file fetch. - Title-path paste fetches are bounded: a new `GetChatFileDataPrefixesByIDs` query returns only a `substr` prefix (`chatprompt.TitlePasteBytePrefix`, 64 KiB = 4 bytes x the 16 Ki-rune title budget) so full blobs (up to 10 MiB each) never leave the database for titling, and `chatprompt.TitlePasteText` applies the same bound to the create path which already holds the loaded row. Deliberate side effect: because generation-time extraction now matches create-time `titleSource` exactly, file-reference-only chats also become eligible for AI titles. They were previously skipped by the same derivation mismatch. Non-goals: no frontend changes (attachment chip UX stays as is), and non-synthetic user-uploaded `.txt` files still yield "New Chat". ## Testing - Unit tests for `TitleText`, `TitlePasteText`, `SyntheticPasteFileIDs`, `FallbackTitle`, `titleInput`, `titlePasteText`, and paste-aware `extractManualTitleTurns`. - Real-database test for `GetChatFileDataPrefixesByIDs` (prefix shorter and longer than stored data) plus dbauthz coverage for the new query. - Integration tests: paste-only create gets a fallback title from the paste content, async title generation fires with the paste content as input, and `RegenerateChatTitle` works on a paste-only chat. > This PR was written by [Mux](https://mux.coder.com) on Mike's behalf.
This commit is contained in:
@@ -3239,6 +3239,34 @@ func (q *querier) GetChatFileByID(ctx context.Context, id uuid.UUID) (database.C
|
||||
return file, nil
|
||||
}
|
||||
|
||||
func (q *querier) GetChatFileDataPrefixesByIDs(ctx context.Context, arg database.GetChatFileDataPrefixesByIDsParams) ([]database.GetChatFileDataPrefixesByIDsRow, error) {
|
||||
rows, err := q.db.GetChatFileDataPrefixesByIDs(ctx, arg)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
var prepared rbac.PreparedAuthorized
|
||||
for _, row := range rows {
|
||||
fileAuthErr := q.authorizeContext(ctx, policy.ActionRead, row)
|
||||
if fileAuthErr == nil {
|
||||
continue
|
||||
}
|
||||
if prepared == nil {
|
||||
prepared, err = prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceChat.Type)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)
|
||||
}
|
||||
}
|
||||
chats, err := q.db.GetAuthorizedChatsByChatFileID(ctx, row.ID, prepared)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if len(chats) == 0 {
|
||||
return nil, fileAuthErr
|
||||
}
|
||||
}
|
||||
return rows, nil
|
||||
}
|
||||
|
||||
func (q *querier) GetChatFileMetadataByChatID(ctx context.Context, chatID uuid.UUID) ([]database.GetChatFileMetadataByChatIDRow, error) {
|
||||
if _, err := q.GetChatByID(ctx, chatID); err != nil {
|
||||
return nil, err
|
||||
|
||||
@@ -206,6 +206,25 @@ func TestChatFilesAllowLinkedChatReads(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, []database.ChatFile{file}, got)
|
||||
})
|
||||
|
||||
t.Run("GetChatFileDataPrefixesByIDs", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
db := dbmock.NewMockStore(ctrl)
|
||||
row := testutil.Fake(t, gofakeit.New(0), database.GetChatFileDataPrefixesByIDsRow{})
|
||||
arg := database.GetChatFileDataPrefixesByIDsParams{IDs: []uuid.UUID{row.ID}, PrefixBytes: 64}
|
||||
|
||||
db.EXPECT().Wrappers().Return([]string{}).AnyTimes()
|
||||
db.EXPECT().GetChatFileDataPrefixesByIDs(gomock.Any(), arg).Return([]database.GetChatFileDataPrefixesByIDsRow{row}, nil)
|
||||
db.EXPECT().GetAuthorizedChatsByChatFileID(gomock.Any(), row.ID, gomock.Any()).Return([]database.Chat{{ID: uuid.New()}}, nil)
|
||||
|
||||
q := dbauthz.New(db, authorizer, slogtest.Make(t, nil), coderdtest.AccessControlStorePointer())
|
||||
got, err := q.GetChatFileDataPrefixesByIDs(ctx, arg)
|
||||
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, []database.GetChatFileDataPrefixesByIDsRow{row}, got)
|
||||
})
|
||||
}
|
||||
|
||||
//nolint:tparallel,paralleltest // It toggles the global chat ACL flag.
|
||||
@@ -959,6 +978,13 @@ func (s *MethodTestSuite) TestChats() {
|
||||
dbm.EXPECT().GetAuthorizedChatsByChatFileID(gomock.Any(), file.ID, gomock.Any()).Return([]database.Chat{}, nil).AnyTimes()
|
||||
check.Args([]uuid.UUID{file.ID}).Asserts(rbac.ResourceChat.WithOwner(file.OwnerID.String()).InOrg(file.OrganizationID).WithID(file.ID), policy.ActionRead).Returns([]database.ChatFile{file})
|
||||
}))
|
||||
s.Run("GetChatFileDataPrefixesByIDs", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||
row := testutil.Fake(s.T(), faker, database.GetChatFileDataPrefixesByIDsRow{})
|
||||
arg := database.GetChatFileDataPrefixesByIDsParams{IDs: []uuid.UUID{row.ID}, PrefixBytes: 64}
|
||||
dbm.EXPECT().GetChatFileDataPrefixesByIDs(gomock.Any(), arg).Return([]database.GetChatFileDataPrefixesByIDsRow{row}, nil).AnyTimes()
|
||||
dbm.EXPECT().GetAuthorizedChatsByChatFileID(gomock.Any(), row.ID, gomock.Any()).Return([]database.Chat{}, nil).AnyTimes()
|
||||
check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(row.OwnerID.String()).InOrg(row.OrganizationID).WithID(row.ID), policy.ActionRead).Returns([]database.GetChatFileDataPrefixesByIDsRow{row})
|
||||
}))
|
||||
s.Run("GetChatFileMetadataByChatID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||
chat := testutil.Fake(s.T(), faker, database.Chat{})
|
||||
file := testutil.Fake(s.T(), faker, database.ChatFile{})
|
||||
|
||||
+8
@@ -1561,6 +1561,14 @@ func (m queryMetricsStore) GetChatFileByID(ctx context.Context, id uuid.UUID) (d
|
||||
return r0, r1
|
||||
}
|
||||
|
||||
func (m queryMetricsStore) GetChatFileDataPrefixesByIDs(ctx context.Context, arg database.GetChatFileDataPrefixesByIDsParams) ([]database.GetChatFileDataPrefixesByIDsRow, error) {
|
||||
start := time.Now()
|
||||
r0, r1 := m.s.GetChatFileDataPrefixesByIDs(ctx, arg)
|
||||
m.queryLatencies.WithLabelValues("GetChatFileDataPrefixesByIDs").Observe(time.Since(start).Seconds())
|
||||
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetChatFileDataPrefixesByIDs").Inc()
|
||||
return r0, r1
|
||||
}
|
||||
|
||||
func (m queryMetricsStore) GetChatFileMetadataByChatID(ctx context.Context, chatID uuid.UUID) ([]database.GetChatFileMetadataByChatIDRow, error) {
|
||||
start := time.Now()
|
||||
r0, r1 := m.s.GetChatFileMetadataByChatID(ctx, chatID)
|
||||
|
||||
Generated
+15
@@ -2878,6 +2878,21 @@ func (mr *MockStoreMockRecorder) GetChatFileByID(ctx, id any) *gomock.Call {
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatFileByID", reflect.TypeOf((*MockStore)(nil).GetChatFileByID), ctx, id)
|
||||
}
|
||||
|
||||
// GetChatFileDataPrefixesByIDs mocks base method.
|
||||
func (m *MockStore) GetChatFileDataPrefixesByIDs(ctx context.Context, arg database.GetChatFileDataPrefixesByIDsParams) ([]database.GetChatFileDataPrefixesByIDsRow, error) {
|
||||
m.ctrl.T.Helper()
|
||||
ret := m.ctrl.Call(m, "GetChatFileDataPrefixesByIDs", ctx, arg)
|
||||
ret0, _ := ret[0].([]database.GetChatFileDataPrefixesByIDsRow)
|
||||
ret1, _ := ret[1].(error)
|
||||
return ret0, ret1
|
||||
}
|
||||
|
||||
// GetChatFileDataPrefixesByIDs indicates an expected call of GetChatFileDataPrefixesByIDs.
|
||||
func (mr *MockStoreMockRecorder) GetChatFileDataPrefixesByIDs(ctx, arg any) *gomock.Call {
|
||||
mr.mock.ctrl.T.Helper()
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChatFileDataPrefixesByIDs", reflect.TypeOf((*MockStore)(nil).GetChatFileDataPrefixesByIDs), ctx, arg)
|
||||
}
|
||||
|
||||
// GetChatFileMetadataByChatID mocks base method.
|
||||
func (m *MockStore) GetChatFileMetadataByChatID(ctx context.Context, chatID uuid.UUID) ([]database.GetChatFileMetadataByChatIDRow, error) {
|
||||
m.ctrl.T.Helper()
|
||||
|
||||
@@ -248,6 +248,10 @@ func (c GetChatFileMetadataByChatIDRow) RBACObject() rbac.Object {
|
||||
return rbac.ResourceChat.WithID(c.ID).WithOwner(c.OwnerID.String()).InOrg(c.OrganizationID)
|
||||
}
|
||||
|
||||
func (c GetChatFileDataPrefixesByIDsRow) RBACObject() rbac.Object {
|
||||
return rbac.ResourceChat.WithID(c.ID).WithOwner(c.OwnerID.String()).InOrg(c.OrganizationID)
|
||||
}
|
||||
|
||||
func (s APIKeyScope) ToRBAC() rbac.ScopeName {
|
||||
switch s {
|
||||
case ApiKeyScopeCoderAll:
|
||||
|
||||
Generated
+4
@@ -417,6 +417,10 @@ type sqlcQuerier interface {
|
||||
// query does not walk up from a child.
|
||||
GetChatFamilyIDsByRootID(ctx context.Context, id uuid.UUID) ([]uuid.UUID, error)
|
||||
GetChatFileByID(ctx context.Context, id uuid.UUID) (ChatFile, error)
|
||||
// GetChatFileDataPrefixesByIDs returns a bounded prefix of each
|
||||
// file's content, keeping full blobs out of server memory. Owner and
|
||||
// organization columns support row-level authorization.
|
||||
GetChatFileDataPrefixesByIDs(ctx context.Context, arg GetChatFileDataPrefixesByIDsParams) ([]GetChatFileDataPrefixesByIDsRow, error)
|
||||
// GetChatFileMetadataByChatID returns lightweight file metadata for
|
||||
// all files linked to a chat. The data column is excluded to avoid
|
||||
// loading file content.
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package database_test
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"database/sql"
|
||||
"encoding/json"
|
||||
@@ -1918,6 +1919,56 @@ func TestGetAuthorizedChatsByChatFileIDACLSharing(t *testing.T) {
|
||||
require.Empty(t, rows[0].GroupACL)
|
||||
}
|
||||
|
||||
func TestGetChatFileDataPrefixesByIDs(t *testing.T) {
|
||||
t.Parallel()
|
||||
if testing.Short() {
|
||||
t.SkipNow()
|
||||
}
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitMedium)
|
||||
sqlDB := testSQLDB(t)
|
||||
err := migrations.Up(sqlDB)
|
||||
require.NoError(t, err)
|
||||
db := database.New(sqlDB)
|
||||
|
||||
owner := dbgen.User(t, db, database.User{})
|
||||
org := dbgen.Organization(t, db, database.Organization{})
|
||||
|
||||
longData := bytes.Repeat([]byte("a"), 100)
|
||||
longFile, err := db.InsertChatFile(ctx, database.InsertChatFileParams{
|
||||
OwnerID: owner.ID,
|
||||
OrganizationID: org.ID,
|
||||
Name: "long.txt",
|
||||
Mimetype: "text/plain",
|
||||
Data: longData,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
shortFile, err := db.InsertChatFile(ctx, database.InsertChatFileParams{
|
||||
OwnerID: owner.ID,
|
||||
OrganizationID: org.ID,
|
||||
Name: "short.txt",
|
||||
Mimetype: "text/plain",
|
||||
Data: []byte("tiny"),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
rows, err := db.GetChatFileDataPrefixesByIDs(ctx, database.GetChatFileDataPrefixesByIDsParams{
|
||||
IDs: []uuid.UUID{longFile.ID, shortFile.ID},
|
||||
PrefixBytes: 16,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, rows, 2)
|
||||
|
||||
prefixes := make(map[uuid.UUID]database.GetChatFileDataPrefixesByIDsRow, len(rows))
|
||||
for _, row := range rows {
|
||||
prefixes[row.ID] = row
|
||||
}
|
||||
require.Equal(t, longData[:16], prefixes[longFile.ID].DataPrefix)
|
||||
require.Equal(t, []byte("tiny"), prefixes[shortFile.ID].DataPrefix)
|
||||
require.Equal(t, owner.ID, prefixes[longFile.ID].OwnerID)
|
||||
require.Equal(t, org.ID, prefixes[longFile.ID].OrganizationID)
|
||||
}
|
||||
|
||||
func TestInsertWorkspaceAgentLogs(t *testing.T) {
|
||||
t.Parallel()
|
||||
if testing.Short() {
|
||||
|
||||
Generated
+49
@@ -4989,6 +4989,55 @@ func (q *sqlQuerier) GetChatFileByID(ctx context.Context, id uuid.UUID) (ChatFil
|
||||
return i, err
|
||||
}
|
||||
|
||||
const getChatFileDataPrefixesByIDs = `-- name: GetChatFileDataPrefixesByIDs :many
|
||||
SELECT id, owner_id, organization_id, substr(data, 1, $1::int) AS data_prefix
|
||||
FROM chat_files
|
||||
WHERE id = ANY($2::uuid[])
|
||||
`
|
||||
|
||||
type GetChatFileDataPrefixesByIDsParams struct {
|
||||
PrefixBytes int32 `db:"prefix_bytes" json:"prefix_bytes"`
|
||||
IDs []uuid.UUID `db:"ids" json:"ids"`
|
||||
}
|
||||
|
||||
type GetChatFileDataPrefixesByIDsRow struct {
|
||||
ID uuid.UUID `db:"id" json:"id"`
|
||||
OwnerID uuid.UUID `db:"owner_id" json:"owner_id"`
|
||||
OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"`
|
||||
DataPrefix []byte `db:"data_prefix" json:"data_prefix"`
|
||||
}
|
||||
|
||||
// GetChatFileDataPrefixesByIDs returns a bounded prefix of each
|
||||
// file's content, keeping full blobs out of server memory. Owner and
|
||||
// organization columns support row-level authorization.
|
||||
func (q *sqlQuerier) GetChatFileDataPrefixesByIDs(ctx context.Context, arg GetChatFileDataPrefixesByIDsParams) ([]GetChatFileDataPrefixesByIDsRow, error) {
|
||||
rows, err := q.db.QueryContext(ctx, getChatFileDataPrefixesByIDs, arg.PrefixBytes, pq.Array(arg.IDs))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer rows.Close()
|
||||
var items []GetChatFileDataPrefixesByIDsRow
|
||||
for rows.Next() {
|
||||
var i GetChatFileDataPrefixesByIDsRow
|
||||
if err := rows.Scan(
|
||||
&i.ID,
|
||||
&i.OwnerID,
|
||||
&i.OrganizationID,
|
||||
&i.DataPrefix,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
items = append(items, i)
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const getChatFileMetadataByChatID = `-- name: GetChatFileMetadataByChatID :many
|
||||
SELECT cf.id, cf.owner_id, cf.organization_id, cf.name, cf.mimetype, cf.created_at
|
||||
FROM chat_files cf
|
||||
|
||||
@@ -9,6 +9,14 @@ SELECT * FROM chat_files WHERE id = @id::uuid;
|
||||
-- name: GetChatFilesByIDs :many
|
||||
SELECT * FROM chat_files WHERE id = ANY(@ids::uuid[]);
|
||||
|
||||
-- name: GetChatFileDataPrefixesByIDs :many
|
||||
-- GetChatFileDataPrefixesByIDs returns a bounded prefix of each
|
||||
-- file's content, keeping full blobs out of server memory. Owner and
|
||||
-- organization columns support row-level authorization.
|
||||
SELECT id, owner_id, organization_id, substr(data, 1, @prefix_bytes::int) AS data_prefix
|
||||
FROM chat_files
|
||||
WHERE id = ANY(@ids::uuid[]);
|
||||
|
||||
-- name: GetChatFileMetadataByChatID :many
|
||||
-- GetChatFileMetadataByChatID returns lightweight file metadata for
|
||||
-- all files linked to a chat. The data column is excluded to avoid
|
||||
|
||||
Reference in New Issue
Block a user